Skip to content

fix: bind direct shard cache to siblings - #1543

Merged
mldangelo-oai merged 4 commits into
mainfrom
mdangelo/codex/fix-direct-shard-cache-identity
Jun 9, 2026
Merged

mldangelo-oai merged 4 commits into
mainfrom
mdangelo/codex/fix-direct-shard-cache-identity

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • bind direct sharded-model cache entries to every sibling's resolved target and full secure: content hash
  • bind the cache identity to the selected model config so adding, changing, or removing config.json cannot return stale config-derived checks
  • bypass reusable cache entries for empty, partial, count-mismatched, suspect, duplicate-target, unavailable, sampled, malformed, or racing shard/config identities
  • strengthen cache configuration fingerprints from 64 to 128 bits
  • synchronize with current main

QA improvements

  • prove unchanged families and model configs reuse the cache
  • prove changed malicious sibling content produces a failed security check instead of a stale clean hit
  • cover config add/change/remove transitions, malformed family metadata, missing and non-regular members, symlink/hardlink aliases, weak hashes, and sampled-hash bypass
  • remove thread-order assumptions from parallel shard regressions

Validation

  • Head: 08413ffd4cce9cfc6c80f9dac3f5b31dc4eae127
  • 243 passed, 4 skipped across the associated advanced-file handler, large-file handler, and cache correctness modules
  • targeted Ruff check/format, mypy, and git diff --check: clean
  • full suite intentionally left to CI

@github-actions

github-actions Bot commented Jun 8, 2026 •

Copy link
Copy Markdown
Contributor

Workflow run and artifacts

Performance Benchmarks

Compared 12 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 12 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 1.341s -> 1.330s (-0.8%).

Workload Benchmark Target Size Files Baseline Current Change Status
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 597.4us 573.2us -4.1% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 520.8us 504.3us -3.2% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 471.15ms 462.76ms -1.8% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 109.52ms 110.95ms +1.3% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 369.07ms 364.88ms -1.1% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 106.24ms 107.36ms +1.1% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 117.77ms 117.02ms -0.6% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 488.8us 485.7us -0.6% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 524.4us 522.4us -0.4% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 66.53ms 66.77ms +0.4% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 508.4us 508.3us -0.0% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 98.14ms 98.13ms -0.0% stable

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review June 8, 2026 21:12

Copy link
Copy Markdown
Contributor Author

@codex Please sync this PR with current main and apply the remaining cache-safety hardening before merge.

The sibling-content fingerprint is the right fix, but _build_advanced_shard_family_cache_fingerprint currently fails open for malformed/empty shard lists and accepts any string that does not start with fingerprint: as a strong hash. That could allow future detector or hasher changes to fall back to metadata-only cache identity.

Required change:

  • Treat a non-None shard family as uncacheable when shards is not a non-empty list of non-empty strings.
  • Accept a sibling hash only when it is a non-empty secure: full hash; reject sampled fingerprints and unknown hash prefixes.
  • Preserve (None, True) only for shard_info is None, meaning there is no family to bind.
  • Add focused regressions for empty/malformed lists and fingerprint:, unknown-prefix, and empty secure: hash results.

I prepared this locally on top of current main as commit 8bdb8f3c, but shell GitHub credentials rejected the push. Focused validation passed: 68 advanced-file-handler tests, 13 large-file-handler tests, Ruff/format, mypy, and git diff --check.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

  • Hardened advanced shard-family cache fingerprinting so only shard_info is None preserves the cacheable (None, True) no-family case; detected shard families with missing, non-list, empty, or malformed shard lists now fail closed as uncacheable. modelaudit/utils/file/handlers.pyL73-L89
  • Restricted sibling shard hash acceptance to non-empty secure: hashes only, rejecting sampled fingerprint: values, unknown prefixes, non-strings, and empty secure: results. modelaudit/utils/file/handlers.pyL91-L100
  • Added focused regressions for malformed shard lists and rejected sibling hash results/prefixes, alongside the existing sampled-fingerprint regression. tests/utils/file/test_advanced_file_handler.pyL1450-L1526
  • Committed the changes as cf8e955 fix: harden advanced shard cache fingerprints and created the follow-up PR record.

Sync note

  • I attempted to fetch current main, but GitHub network access from this shell failed with CONNECT tunnel failed, response 403. The local checkout was already the PR merge-head shown in the supplied PR metadata.

Testing

  • ✅ uv run ruff format modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/utils/file/test_advanced_file_handler.py -q
  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/utils/file/test_large_file_handler.py -q
  • ✅ uv run ruff check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ uv run ruff format --check modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ uv run mypy modelaudit/ packages/modelaudit-picklescan/src packages/modelaudit-picklescan/tests tests/
  • ✅ git diff --check
  • ❌ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest -n auto -m "not slow and not integration" --maxfail=1 failed in unrelated CLI report-writer tests after 10205 passed, 589 skipped; failures were tests/test_cli.py::test_cli_report_writers_recheck_parent_links_on_fallback and tests/test_cli.py::test_cli_report_writers_reject_symlinked_parent_directory.

View task →

Copy link
Copy Markdown
Contributor Author

@codex Please apply the reviewed fix from local commits 2d521d98 and 9eee93c6 to this PR branch (the latter merges current main). The local Git credential cannot push (Invalid username or token).

Review findings fixed:

  • reject empty, partial, count-mismatched, duplicate-target, or otherwise malformed shard-family membership instead of creating a reusable cache key;
  • require every sibling content identity to be a full secure: hash, so sampled or unknown hashes bypass the cache;
  • make the parallel shard regression deterministic rather than depending on thread completion order (the exact cause of the Python 3.13 CI failure).

Focused validation on current main: 75 passed for tests/utils/file/test_advanced_file_handler.py and tests/utils/file/test_large_file_handler.py; Ruff check/format and mypy passed for the modified source and test files. Please push the equivalent changes to mdangelo/codex/fix-direct-shard-cache-identity.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

  • Hardened advanced shard-family cache fingerprinting so only shard_info is None keeps the cacheable no-family case; malformed, empty, partial, count-mismatched, suspect, or duplicate-target shard families now fail closed as uncacheable. modelaudit/utils/file/handlers.pyL78-L129
  • Required every shard-family content identity to be a non-empty secure: hash before contributing to a reusable cache key. modelaudit/utils/file/handlers.pyL113-L130
  • Added focused regressions for malformed shard lists, count mismatches, partial/suspect membership, duplicate resolved targets, sampled fingerprints, unknown prefixes, and empty/non-string secure hashes. tests/utils/file/test_advanced_file_handler.pyL1474-L1608
  • Made the parallel raw-detector shard regression independent of worker completion order by asserting the detector set rather than list ordering. tests/utils/file/test_advanced_file_handler.pyL1817-L1844
  • Committed the changes as 525ca26 fix: harden direct shard cache identity and created the follow-up PR record.

Testing

  • ✅ PROMPTFOO_DISABLE_TELEMETRY=1 uv run pytest tests/utils/file/test_advanced_file_handler.py tests/utils/file/test_large_file_handler.py -q — 90 passed, 2 skipped because h5py is not installed.
  • ✅ uv run ruff check modelaudit/utils/file/handlers.py tests/utils/file/test_advanced_file_handler.py tests/utils/file/test_large_file_handler.py
  • ✅ uv run ruff format --check modelaudit/utils/file/handlers.py tests/utils/file/test_advanced_file_handler.py tests/utils/file/test_large_file_handler.py
  • ✅ uv run mypy modelaudit/utils/file/handlers.py tests/utils/file/test_advanced_file_handler.py tests/utils/file/test_large_file_handler.py
  • ✅ git diff --check

View task →

Synchronize with main, fail closed on partial shard identities, and prove safe cache hits.
@mldangelo-oai
mldangelo-oai force-pushed the mdangelo/codex/fix-direct-shard-cache-identity branch from 7224887 to 90eda93 Compare June 9, 2026 04:45

Copy link
Copy Markdown
Contributor Author

Critical review complete at 90eda93fc0fe86fdb2ffc7bee3d899972b4c4518.

I tightened the family fingerprint so it cannot silently omit malformed shard entries or accept sampled/unknown hash formats. Reusable entries now require a complete non-empty exact-string shard list, unique resolved members, and full secure: hashes. I also strengthened the regression to call the unchanged family twice before mutation, proving the safe cache-hit path still works; the prior test would also have passed if caching had been disabled entirely.

Validation: 64 passed in the associated advanced-file handler module; scoped Ruff, format, mypy, and diff checks are clean. There are no review threads on this PR.

Copy link
Copy Markdown
Contributor Author

Addressed the outstanding cache-safety requests from comments 4654494331 and 4655763347 at head 175e4864d8b899feaba7a978d7aaa68a910442ba.

Key fixes:

  • fail closed for empty, partial, count-mismatched, suspect, unavailable, non-regular, symlink-alias, and hardlink-alias families
  • require non-empty secure: hashes for every sibling and bypass cache for sampled/unknown hashes
  • strengthen config hashes to 128 bits
  • make parallel assertions order-independent
  • prove a changed malicious sibling invalidates the clean cache result and emits the failed check

Focused validation after syncing current main: 244 passed, 2 skipped; scoped Ruff, format, mypy, and diff checks are clean. Exact remote tree matches the reviewed local tree.

@mldangelo-oai
mldangelo-oai merged commit 45bf167 into main Jun 9, 2026
23 of 27 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/fix-direct-shard-cache-identity branch June 9, 2026 10:18

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 08413ffd4c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

if shard_info is not None:
# A representative file key alone cannot describe sibling shard
# membership, target identity, or incomplete-family state.
version_config["advanced_shard_family"] = shard_info

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use only stable shard data in the cache key

When a sibling shard is touched or recreated with identical bytes, advanced_shard_family_cache_fingerprint remains content-stable, but the version context still also contains the full advanced_shard_family object with shard_targets mtime_ns/ctime_ns stats. Because build_cache_version_context() hashes both entries, the new stable fingerprint cannot produce a cache hit for unchanged shard contents; replace the volatile family object in the version context with the stable fingerprint or strip the stat fields.

Useful? React with 👍 / 👎.

@github-actions github-actions Bot mentioned this pull request Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant