Repository navigation
fix(server): unify dependency freshness on consumed-content snapshots - #507
Merged
Merged
Conversation
Per-dep {size, mtime_ns, hash} equality records replace the second-
truncated build_at watermark; hashes come from the worker's own compile
buffers, PCM deps are finally populated (incl. the module source), the
index merge verifies disk content against the indexed hash, and
need_update validates every compilation context.
📝 WalkthroughWalkthroughChangesDependency freshness pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Compiler
participant Workspace
participant Filesystem
participant Indexer
Compiler->>Workspace: capture dependency hashes and stat baselines
Workspace->>Filesystem: validate dependency metadata and content
Filesystem-->>Workspace: freshness result
Workspace-->>Compiler: cache reuse or rebuild decision
Indexer->>Filesystem: hash indexed TU paths
Filesystem-->>Indexer: current content hash
Indexer-->>Compiler: merge accepted or skipped
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 876fc7ec14
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/integration/compilation/test_persistent_cache.py (1)
165-234: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing anomaly checks in multi-session tests.
Both
test_old_cache_json_upgradesandtest_pcm_offline_edit_invalidatesbypass theclientfixture's automaticcheck_no_anomalyteardown by usingmake_client/shutdown_clientdirectly.test_old_cache_json_upgradesnever callsassert_no_anomalyfor either session, andtest_pcm_offline_edit_invalidatesonly calls it after session 1 (line 220), not after session 2. Given these tests exercise exactly the kind of stat/hash edge cases most likely to surface an internal crash, an anomaly check on every session would be a meaningful safety net, consistent with the fixture's own rationale.🛡️ Suggested addition
c2 = await make_client(executable, tmp_path) uri2, _ = await c2.open_and_wait(tmp_path / "main.cpp") assert_clean_compile(c2, uri2) # Loaded, hash-validated, reused — not rebuilt. assert list_pch_files(tmp_path)[0].stat().st_mtime == pch_mtime_s1 + assert_no_anomaly(c2, tmp_path) await shutdown_client(c2)c2 = await make_client(executable, tmp_path) mid_uri2, _ = await c2.open_and_wait(tmp_path / "mid.cppm") assert_has_errors(c2, mid_uri2, "Expected errors after offline interface edit") + assert_no_anomaly(c2, tmp_path) await shutdown_client(c2)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/compilation/test_persistent_cache.py` around lines 165 - 234, Make anomaly validation explicit for every client session in test_old_cache_json_upgrades and test_pcm_offline_edit_invalidates. Call assert_no_anomaly with the relevant client and workspace before each shutdown_client, including both c1 and c2 in the cache-upgrade test and c2 in the PCM invalidation test, while preserving the existing assertions and shutdown flow.src/index/merged_index.cpp (1)
105-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
hash_file— consider hoisting to a shared header.This is byte-identical to
workspace::hash_file(src/server/state/workspace.cpp), which the doc comment itself calls out. Sincefs::stat_baseline_before_ns/fs::mtime_nsare already shared viasrc/support/filesystem.h, moving this helper there too would give both subsystems a single source of truth for the freshness hash scheme.♻️ Suggested consolidation
-namespace { - -/// Hash a file's content with the same scheme the server layer uses for its -/// dependency snapshots (`workspace::hash_file`). Returns 0 on read failure. -std::uint64_t hash_file(llvm::StringRef path) { - auto buffer = llvm::MemoryBuffer::getFile(path); - if(!buffer) { - return 0; - } - return llvm::xxh3_64bits((*buffer)->getBuffer()); -} - -} // namespace +// hash_file now lives in support/filesystem.h (fs::hash_file), shared with +// workspace.cpp's dependency snapshot capture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index/merged_index.cpp` around lines 105 - 115, Consolidate the duplicate hash_file implementation by moving the shared file-content hashing helper into src/support/filesystem.h and its implementation location, then update the merged-index and workspace callers to reuse it. Remove the local anonymous-namespace hash_file in the merged-index code and preserve the existing zero-on-read-failure behavior and xxh3 hashing scheme.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/compiler/context_resolver.cpp`:
- Around line 566-585: Update resolve_header_context to re-stat each header
after reading its buffer, including the target-header path. Retain the fast-path
size and mtime only when pre-read and post-read metadata match and the post-read
mtime is at or before baseline_before_ns; otherwise store zeroed metadata while
preserving the buffer hash. Apply this consistently to the related
dependency-recording paths near the additional reported locations.
---
Nitpick comments:
In `@src/index/merged_index.cpp`:
- Around line 105-115: Consolidate the duplicate hash_file implementation by
moving the shared file-content hashing helper into src/support/filesystem.h and
its implementation location, then update the merged-index and workspace callers
to reuse it. Remove the local anonymous-namespace hash_file in the merged-index
code and preserve the existing zero-on-read-failure behavior and xxh3 hashing
scheme.
In `@tests/integration/compilation/test_persistent_cache.py`:
- Around line 165-234: Make anomaly validation explicit for every client session
in test_old_cache_json_upgrades and test_pcm_offline_edit_invalidates. Call
assert_no_anomaly with the relevant client and workspace before each
shutdown_client, including both c1 and c2 in the cache-upgrade test and c2 in
the PCM invalidation test, while preserving the existing assertions and shutdown
flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11104059-06b7-404d-8a9c-94988c262802
📒 Files selected for processing (35)
src/compile/compilation.cppsrc/compile/compilation.hsrc/compile/compilation_unit.cppsrc/compile/compilation_unit.hsrc/compile/dep_file.hsrc/index/include_graph.cppsrc/index/include_graph.hsrc/index/merged_index.cppsrc/index/merged_index.hsrc/index/schema.fbssrc/index/tu_index.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/compiler/context_resolver.cppsrc/server/compiler/context_resolver.hsrc/server/compiler/indexer.cppsrc/server/protocol/worker.hsrc/server/state/file_tracker.cppsrc/server/state/invalidator.hsrc/server/state/workspace.cppsrc/server/state/workspace.hsrc/server/transport/master_server.cppsrc/server/worker/stateful_worker.cppsrc/server/worker/stateless_worker.cppsrc/support/filesystem.htests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/unit/compile/compilation_tests.cpptests/unit/compile/directive_tests.cpptests/unit/index/merged_index_tests.cpptests/unit/server/context_resolver_tests.cpptests/unit/server/deps_snapshot_tests.cpptests/unit/server/indexer_tests.cpptests/unit/server/stateless_worker_tests.cpptests/unit/test/temp_dir.h
deps() resolves every path through real_path, so on macOS the recorded dep spelling is /private/var/... while TempDir and pytest hand out the /var/... symlink form. Exact string equality in BuildPCMRequest and test_pcm_cache_entry_has_deps only held on Linux; resolve both sides before comparing.
- Skip the whole TUIndex merge when the main file moved on: header shards merged before the main-file check could mix two generations, and the sweep stripped contributions the stale result no longer mentioned. - Drop dep-less PCM cache entries at load: they predate populated deps and would blindly serve a stale PCM after upgrade. - Stat chain files and the target after reading them, so a write racing the read lands inside the mtime guard and earns no fast path. - Document the still-missing-dep and identical-stat residuals.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/compiler/indexer.cpp (1)
126-140: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider avoiding the
std::stringcopy for header content.
header_bufis alive for the duration of theelseblock, soheader_contentcould reference its buffer directly instead of copying intoheader_content_storage. This avoids a heap allocation and copy per header shard.♻️ Optional refactor
auto header_path = workspace.path_pool.resolve(global_path_id); llvm::StringRef header_content; - std::string header_content_storage; auto header_buf = llvm::MemoryBuffer::getFile(header_path); if(header_buf) { - header_content_storage = (*header_buf)->getBuffer().str(); - header_content = header_content_storage; + header_content = (*header_buf)->getBuffer(); }The header arbitration logic (lines 133-140) — unconditional
content_matchescheck,touched.inserton mismatch to preserve prior contributions — looks correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/compiler/indexer.cpp` around lines 126 - 140, Update the header content setup near content_matches to reference the buffer returned by header_buf directly, removing header_content_storage and its heap-copy operation. Preserve the existing buffer lifetime through the content_matches call and retain the unconditional mismatch handling, touched insertion, and early return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/server/compiler/indexer.cpp`:
- Around line 126-140: Update the header content setup near content_matches to
reference the buffer returned by header_buf directly, removing
header_content_storage and its heap-copy operation. Preserve the existing buffer
lifetime through the content_matches call and retain the unconditional mismatch
handling, touched insertion, and early return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ec4ab7f-3cab-480f-b03b-63a0f863b3f4
📒 Files selected for processing (5)
src/server/compiler/context_resolver.cppsrc/server/compiler/indexer.cppsrc/server/state/workspace.cppsrc/server/state/workspace.htests/integration/compilation/test_persistent_cache.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/state/workspace.h
- src/server/compiler/context_resolver.cpp
- src/server/state/workspace.cpp
16bit-ykiko
added a commit
that referenced
this pull request
Jul 13, 2026
Both Windows CI jobs are red on main since #507 (its own merge run 29258064794 failed): `MergedIndex.NeedUpdateChecksAllContexts` and `MergedIndex.SerializedStampsValidate` fail deterministically with `need_update() expected true, got false`. ## Root cause Windows file times advance in ~16ms clock ticks. The failing assertions perform **same-size** rewrites (`int b2();` → `int b3();`, `int f();` → `int g();`) moments after the stamp was recorded; when the rewrite lands in the same tick, the file reproduces the stamp's size **and** mtime exactly, so the stat fast path — equality-based by design — rightly reports fresh, and the hash layer the assertions meant to exercise is never consulted. On Linux/macOS nanosecond timestamps always move, which is why only Windows fails. **Production is immune**: `merge` only records a stamp for files untouched since two seconds before the build (`stat_baseline_before_ns`), so a stamped file's later edit can never share its tick. The tests defeat that guard deliberately (`generous_build_at()` = now + 10s) to earn the fast path, which is what exposes them to the tick. ## Fix After each same-size rewrite, bump the file's mtime explicitly (`set_file_mtime`, +5s) — modelling the reality that a genuine edit arrives long after the stamp, and pinning those verdicts on the hash layer as intended. Test-only change; no product code touched.
This was referenced Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Compilation artifacts (PCH / PCM / AST / synthesized header preambles) shared one freshness snapshot whose two layers were both unsound, and the index shards carried their own, subtly different copy of the same check. Audited failure modes (bughunt F44 / F01 / F04 / F33):
build_atwatermark, both truncated to whole seconds, with<=. A header saved in the same second the dependent's compile finished satisfiesmtime <= build_atforever — the hash layer never runs, and the dependent keeps compiling against a stale PCH until the header changes again. Reproduced with a natural type-then-save rhythm; no adversarial timing needed.capture_deps_snapshotran on the master after the build and hashed the current disk. A header saved while the build was running yields a snapshot that blesses content the build never read; the poisoned entry persists via cache.json across restarts.compile()overload never filledout.deps, so every PCM snapshot was empty — and the PCM cache key embeds no content, so "graceful exit → edit a module interface offline → restart" deterministically serves a stale PCM. The module source itself also has to be a dep, which it never was.need_updatevalidated only the first compilation context of a shard (a header shard has one per including TU); merge paired the worker's rows with content re-read from disk at merge time (position misalignment when the file changed in between); the same watermark blindness applied to shard staleness.Design
One freshness vocabulary everywhere, aligned with the one implementation that was already right (FileTracker):
DepsSnapshotis now a list ofDepState { path_id, size, mtime_ns, hash, missing }. Layer 1 passes only when size and nanosecond mtime are equal to the record — backdated or preserved mtimes can no longer masquerade as fresh. Layer 2 hashes the disk against the recorded hash; a match is a touch, not an edit, and repairs the fast path in place.SourceManager) and ship{path, hash}pairs plusbuild_at(sampled before the compile) inCompileResult/BuildResult. A later disk read can no longer impersonate what the build saw.{size, mtime_ns}fast path only for deps whose mtime precedesbuild_atby a 2s filesystem-granularity guard (fs::stat_baseline_before_ns). Anything newer keeps only the consumed hash and re-earns its fast path through one hash comparison. The synthesized header-preamble chain (a third producer of these records) applies the same guard.out.deps = unit.deps()+ the canonicalized module source with its content hash).TUIndexships per-path consumed hashes;MergedIndex::mergerecordsDepStampfast paths under the same guard (new, backward-compatible flatbuffer field);need_updatevalidates every compilation context, in-memory and serialized; the indexer skips a merge whose rows no longer hash to the disk content and keeps the last-known snapshot serving instead of stripping it.build_atskipped, missing fields defaulted) and revalidate by hash once; old shards withoutdep_stampsdo the same. No cache or index version bump, nothing is rebuilt on upgrade.force_revalidate(previouslybuild_at = 0) now zeroes the per-dep fast paths while keeping the hashes, which is the same semantics expressed per record.Testing
DepsSnapshotunit suite: the full freshness truth table — same-second edit, backdated edit, touch-repairs-fast-path, poisoned-capture detection, no-baseline convergence, missing-file transitions, force_revalidate.IndexerMergeunit suite: a merge whose rows describe outdated disk content is skipped and the last-known snapshot (rows included) keeps serving; the settled reindex lands.MergedIndex: multi-contextneed_updateexercised for both contexts and through a serialized view; backdated edits; stamp round-trip.