fix(cache): preserve macOS entries during ancestor churn - #1800
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Performance BenchmarksCompared
|
There was a problem hiding this comment.
Pull request overview
This PR improves ScanResultsCache correctness on macOS by introducing a Darwin-specific ancestor/path monitor that detects vnode replacement (rename/delete/revoke) without invalidating cache identities due to unrelated directory writes (e.g., temporary-file churn during scans).
Changes:
- Add a macOS
_DarwinPathMonitorbased onkqueuevnode events and wire it into ancestor-identity monitoring/verification. - Add Darwin-focused regression tests covering unrelated sibling churn and restored file/directory replacement, plus a unit test for Darwin monitor selection via monkeypatching.
- Document the behavior change in the root
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
modelaudit/cache/scan_results_cache.py |
Adds _DarwinPathMonitor and integrates it into AncestorIdentity monitoring on macOS. |
tests/cache/test_cache_correctness.py |
Adds macOS regressions for cache identity behavior under sibling churn and ancestor/file replacement, and verifies Darwin monitor selection. |
CHANGELOG.md |
Notes the macOS cache-identity behavior change under [Unreleased]. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex security review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb9b355668
ℹ️ 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".
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Summary
kqueuevnode monitor for scanned files and ancestor directories so unrelated sibling or temporary-file writes no longer invalidate safe cache identities.Why
Linux uses inotify and Windows holds path locks while caching, but macOS previously had no ancestor monitor and compared every ancestor directory modification timestamp instead. A normal compressed-model scan creates temporary files under a shared ancestor, changing that ancestor and causing every cache store to be rejected. The existing compressed-wrapper regression reproduced deterministically with
total_entries == 0instead of2.The Darwin monitor watches vnode deletion, rename, revocation, and attribute events. Ordinary directory writes remain harmless, while replacing and restoring either the scanned file or one of its ancestors, or changing watched access metadata, still invalidates the cache identity.
Validation
tests/cache: 241 passed, 6 platform skips before the final attribute-event follow-up; the focused follow-up cache run passed.kqueuecases remain platform-gated on the Linux devbox; deterministic monitor-construction, ownership, event-registration, failure, and interruption paths are exercised with mocks.