Skip to content

fix(cache): update scan-result entries atomically on hits - #1809

Open
mldangelo-oai wants to merge 1 commit into
mainfrom
fix/cache-atomic-hit-writebacks
Open

fix(cache): update scan-result entries atomically on hits#1809
mldangelo-oai wants to merge 1 commit into
mainfrom
fix/cache-atomic-hit-writebacks

Conversation

@mldangelo-oai

Copy link
Copy Markdown
Contributor

Summary

  • Keep published scan-result JSON readable while normal and keyed cache hits update access metadata by writing to a private temporary file and atomically replacing the published entry.
  • Preserve validated cache hits and clean up temporary files when best-effort access bookkeeping encounters an operating-system error.
  • Add failing-before/passing-after regression coverage for overlapping reads and interrupted writes through both lookup paths.

Verification

  • Four targeted regression cases failed before the fix and pass after it.
  • Cache correctness, cache configuration, large-file handlers, compressed scanners, and Joblib scanners: 443 passed, 7 skipped, 1 deselected. The deselected macOS/HDF5 case fails identically on clean current main.
  • Repository-wide Ruff lint and format checks pass.
  • Scoped mypy passes after suppressing the existing union-attr baseline; full mypy reports the same 14 errors in six files on this branch and clean current main.
  • Full parallel fast-suite smoke reached 7,511 passes before existing macOS cache/YAML-routing flakes; both exact nodes pass independently on this branch and clean current main.

@mldangelo-oai

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Another round soon, please!

Reviewed commit: f52fadc203

ℹ️ 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".

@github-actions

Copy link
Copy Markdown
Contributor

Workflow run and artifacts

Performance Benchmarks

Compared 13 shared benchmarks with a regression threshold of 15%.
Status: 0 regressions, 0 improved, 13 stable, 0 new, 0 missing.
Aggregate shared-benchmark median: 4.353s -> 4.429s (+1.7%).

Workload Benchmark Target Size Files Baseline Current Change Status
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_hex] nested_hex 130 B 1 299.1us 329.3us +10.1% stable
rejected-basic-auth-candidates tests/benchmarks/test_scan_benchmarks.py::test_rejected_basic_auth_candidates_scan_linearly - 371.1 KiB 1 2.507s 2.581s +2.9% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_raw] nested_raw 78 B 1 279.4us 272.4us -2.5% stable
direct-malicious-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_direct_malicious_upload malicious_reduce 52 B 1 222.7us 227.0us +1.9% stable
chunked-upload-stream tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_chunked_upload_stream chunked_stream 278.2 KiB 1 116.31ms 114.09ms -1.9% stable
duplicate-heavy-registry tests/benchmarks/test_scan_benchmarks.py::test_scan_duplicate_registry_snapshot registry-snapshot 915.2 KiB 13 572.48ms 577.89ms +0.9% stable
padded-multi-stream-upload tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_padded_multi_stream_upload multi_stream_padded 4.1 KiB 1 332.0us 335.0us +0.9% stable
nested-payload-review tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_nested_payload_review[nested_base64] nested_base64 98 B 1 291.0us 293.0us +0.7% stable
mixed-model-repository tests/benchmarks/test_scan_benchmarks.py::test_scan_release_candidate_repository release-candidate 547.3 KiB 32 636.54ms 634.01ms -0.4% stable
warm-cache-rescan tests/benchmarks/test_scan_benchmarks.py::test_scan_warm_cached_repository_rescan release-candidate 547.3 KiB 32 153.68ms 154.21ms +0.3% stable
suspicious-pickle-intake tests/benchmarks/test_scan_benchmarks.py::test_scan_suspicious_pickle_intake suspicious-intake 183.8 KiB 4 149.72ms 150.11ms +0.3% stable
single-checkpoint-preflight tests/benchmarks/test_scan_benchmarks.py::test_scan_single_checkpoint_before_load single_checkpoint.pkl 183.0 KiB 1 105.70ms 105.47ms -0.2% stable
clean-training-checkpoint tests/benchmarks/test_picklescan_benchmarks.py::test_picklescan_clean_training_checkpoint safe_large 278.2 KiB 1 110.77ms 110.87ms +0.1% stable

@chatgpt-codex-connector

Copy link
Copy Markdown

Security review completed. No security issues were found in this pull request.

Reviewed commit: f52fadc203

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant