Repository navigation
feat: two-layer staleness tracking with concurrent compilation dedup - #386
Merged
Merged
Conversation
Replace the didSave sledgehammer (clear all PCH hashes + mark all docs dirty) with precise mtime-based dependency tracking: - Workers return deps list in CompileResult/BuildPCHResult/BuildPCMResult - Master captures mtime snapshots after successful compilation - ensure_compiled() checks both AST and PCH dep mtimes on fast path - ensure_pch() validates dep mtimes alongside preamble hash - didSave no longer blanket-invalidates all open documents Add integration tests verifying header-change detection triggers recompilation without explicit didSave invalidation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughPer-file dependency snapshots (mtimes, content hashes, capture timestamps) were added; PCH caching unified into per-path Changes
Sequence DiagramsequenceDiagram
actor Client
participant Master as MasterServer
participant FS as FileSystem
participant Worker as Worker
Client->>Master: ensure_compiled(path_id)
Master->>Master: if doc.ast_dirty == false\ncheck ast_deps[path_id] & pch_states[path_id].deps
Master->>FS: stat files listed in snapshots
FS-->>Master: mtimes / existence
Master->>Master: deps_changed()? -> mark ast_dirty if changed
alt ast_dirty == true
Master->>Worker: compile_request(path_id)
Worker->>Worker: compile, collect deps list
Worker-->>Master: CompileResult{ok, deps, ...}
Master->>Master: capture_deps_snapshot(result.deps)\nstore ast_deps[path_id]
end
Master-->>Client: diagnostics / hover response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/master_server.cpp (1)
307-315:⚠️ Potential issue | 🟠 MajorClear
pch_depswhen preamble disappears.When
bound == 0, PCH path/hash state is cleared, butpch_deps[path_id]is retained. That leaves stale dependency snapshots for files that no longer use PCH and can cause unnecessary invalidation behavior.🛠️ Proposed fix
if(bound == 0) { // No preamble directives — PCH would be empty. Clear any stale entry. if(auto old_it = pch_paths.find(path_id); old_it != pch_paths.end()) { fs::remove(old_it->second); } pch_paths.erase(path_id); pch_bounds.erase(path_id); pch_hashes.erase(path_id); + pch_deps.erase(path_id); co_return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 307 - 315, When handling the case bound == 0 (no preamble directives) in the function that updates PCH state, you're clearing pch_paths, pch_bounds, and pch_hashes but forgetting to clear pch_deps, leaving stale dependency snapshots; update the same block that erases pch_paths/pch_bounds/pch_hashes for the given path_id to also erase pch_deps[path_id] (call pch_deps.erase(path_id)) so the dependency map is removed when the preamble disappears, keeping all four maps consistent.
🧹 Nitpick comments (1)
tests/integration/test_staleness.py (1)
123-130: Strengthen the fast-path test assertion.
assert hover is not Nonecan pass even if recompilation occurred. Consider asserting that no new diagnostics notification is emitted on the second hover.🛠️ Example strengthening
hover = await client.text_document_hover_async( HoverParams(text_document=_doc(uri), position=Position(line=0, character=4)) ) # "main" should be hoverable. assert hover is not None + no_diag_event = client.wait_for_diagnostics(uri) + await client.text_document_hover_async( + HoverParams(text_document=_doc(uri), position=Position(line=0, character=4)) + ) + with pytest.raises(asyncio.TimeoutError): + await asyncio.wait_for(no_diag_event.wait(), timeout=1.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_staleness.py` around lines 123 - 130, The current assertion `assert hover is not None` doesn't ensure the fast-path (no recompilation); update the test around the second hover (the call to client.text_document_hover_async with HoverParams/Position/_doc) to capture or subscribe to LSP notifications (specifically "textDocument/publishDiagnostics") and assert that no new diagnostics notification is emitted between the first and second hover; i.e., record diagnostics notifications before the second hover, perform the second hover, then assert the diagnostics notification count or last diagnostics timestamp did not change to prove no recompilation occurred.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 35-68: capture_deps_snapshot and deps_changed currently use
llvm::sys::toTimeT() which loses sub-second precision and can miss rapid edits;
change DepsSnapshot::mtimes to store
llvm::sys::TimePoint<std::chrono::nanoseconds> (or
llvm::sys::fs::file_time_type) and have capture_deps_snapshot push the raw file
timestamp (status.getLastModificationTime()) instead of toTimeT(), then have
deps_changed compare those file_time_type values directly; also in the
no-preamble path inside ensure_pch clear pch_deps in addition to pch_paths,
pch_bounds and pch_hashes so stale snapshots aren’t retained and
ensure_compiled’s checks behave correctly (update any equality/zero checks to
handle the new file_time_type).
In `@tests/integration/test_staleness.py`:
- Around line 62-63: In the async tests test_header_change_invalidates_ast and
test_header_change_invalidates_pch replace the blocking call time.sleep(1.1)
with a non-blocking await asyncio.sleep(1.1); locate the sleep calls inside
those test functions and change them to use await asyncio.sleep(...) so the
event loop isn't blocked (asyncio is already imported).
---
Outside diff comments:
In `@src/server/master_server.cpp`:
- Around line 307-315: When handling the case bound == 0 (no preamble
directives) in the function that updates PCH state, you're clearing pch_paths,
pch_bounds, and pch_hashes but forgetting to clear pch_deps, leaving stale
dependency snapshots; update the same block that erases
pch_paths/pch_bounds/pch_hashes for the given path_id to also erase
pch_deps[path_id] (call pch_deps.erase(path_id)) so the dependency map is
removed when the preamble disappears, keeping all four maps consistent.
---
Nitpick comments:
In `@tests/integration/test_staleness.py`:
- Around line 123-130: The current assertion `assert hover is not None` doesn't
ensure the fast-path (no recompilation); update the test around the second hover
(the call to client.text_document_hover_async with HoverParams/Position/_doc) to
capture or subscribe to LSP notifications (specifically
"textDocument/publishDiagnostics") and assert that no new diagnostics
notification is emitted between the first and second hover; i.e., record
diagnostics notifications before the second hover, perform the second hover,
then assert the diagnostics notification count or last diagnostics timestamp did
not change to prove no recompilation occurred.
🪄 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: be84e3ae-b824-4ba6-8436-4dda572410f2
📒 Files selected for processing (6)
src/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/stateful_worker.cppsrc/server/stateless_worker.cpptests/integration/test_staleness.py
Replace per-file mtime tracking with a two-layer approach:
- Layer 1 (fast): stat each dep, skip if mtime <= build_at
- Layer 2 (precise): re-hash content via xxh3_64bits for files
with newer mtime, skip rebuild if hash matches (touch-only change)
DepsSnapshot now stores {files, hashes, build_at} instead of
{files, mtimes}, enabling future ccache-like cross-repo reuse.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace std::vector<std::string> with std::vector<uint32_t> path_ids interned via PathPool, eliminating string copies. Resolve paths through path_pool.resolve() only when needed (stat/hash checks). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/master_server.cpp (1)
352-355:⚠️ Potential issue | 🔴 CriticalFailed PCH rebuilds can still look successful to concurrent waiters.
Line 354 treats
pch_paths.contains(path_id)as the rebuild result. If we entered the rebuild path because the cached PCH was stale and the new build then fails, the old path is still present, so waiting requests wake up withtrueandensure_deps()can hand the stale PCH right back to Clang. Clear the cached entry before signalling failure, or propagate an explicit success bit throughpch_building.🛠️ Minimal fix
if(!result.has_value() || !result.value().success) { LOG_WARN("PCH build failed for {}: {}", path, result.has_value() ? result.value().error : result.error().message); + if(auto old_it = pch_paths.find(path_id); old_it != pch_paths.end()) { + fs::remove(old_it->second); + } + pch_paths.erase(path_id); + pch_bounds.erase(path_id); + pch_hashes.erase(path_id); + pch_deps.erase(path_id); pch_building.erase(path_id); completion->set(); co_return false; }Also applies to: 373-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 352 - 355, The current waiter path using pch_building and then returning pch_paths.contains(path_id) can report success when a rebuild failed but the old stale entry remains; change the build/wait protocol so that pch_building communicates an explicit success/failure result (e.g., store a future/promise that completes with a bool) or ensure the builder removes/clears the stale entry from pch_paths before completing the waiters; specifically update the logic around pch_building, its wait() completion value, and the code paths that set/erase pch_paths (and calls to ensure_deps()/wait()) so waiters check the explicit success bit from pch_building rather than re-checking pch_paths.contains(path_id), and also apply the same fix to the similar block around lines 373-379.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 46-54: capture_deps_snapshot currently recomputes dep
hashes/timestamp on the master (using hash_file) after the worker finished,
which can race; instead compute and attach the DepsSnapshot (deps + hashes +
build_at) inside the worker and return it in the worker's result struct so the
master consumes that build-time snapshot. Concretely: move the hash_file loops
and build_at assignment into the worker code that produces the result payload,
populate a DepsSnapshot there, stop calling capture_deps_snapshot() on the
master (remove its uses), and update any code paths that persist artifacts to
take the DepsSnapshot from the worker result rather than recomputing hashes on
the master. Ensure function/struct names involved are DepsSnapshot,
capture_deps_snapshot (remove or repurpose), and hash_file so callers use the
worker-provided snapshot.
---
Outside diff comments:
In `@src/server/master_server.cpp`:
- Around line 352-355: The current waiter path using pch_building and then
returning pch_paths.contains(path_id) can report success when a rebuild failed
but the old stale entry remains; change the build/wait protocol so that
pch_building communicates an explicit success/failure result (e.g., store a
future/promise that completes with a bool) or ensure the builder removes/clears
the stale entry from pch_paths before completing the waiters; specifically
update the logic around pch_building, its wait() completion value, and the code
paths that set/erase pch_paths (and calls to ensure_deps()/wait()) so waiters
check the explicit success bit from pch_building rather than re-checking
pch_paths.contains(path_id), and also apply the same fix to the similar block
around lines 373-379.
🪄 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: 4278d6ca-d3de-4448-b30b-3986cf5ef3d8
📒 Files selected for processing (2)
src/server/master_server.cppsrc/server/master_server.h
Merge 5 separate per-file maps (pch_paths, pch_bounds, pch_hashes, pch_building, pch_deps) into a single llvm::DenseMap<uint32_t, PCHState>. Simplifies cleanup in didClose and makes the PCH lifecycle easier to reason about. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move SymbolInfo to namespace scope alongside DocumentState/DepsSnapshot/PCHState - Add doc comments to all MasterServer member fields and methods - Organize members into logical sections (index state, helpers, forwarding, queries) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/server/master_server.cpp (1)
580-582: Redundant check:result.has_value()is always true here.Line 570 already returns
falsewhen!result.has_value(), so the condition at line 580 is always satisfied.♻️ Suggested simplification
publish_diagnostics(uri_str, doc2.version, result.value().diagnostics); doc2.ast_dirty = false; - if(result.has_value()) { - ast_deps[path_id] = capture_deps_snapshot(path_pool, result.value().deps); - } + ast_deps[path_id] = capture_deps_snapshot(path_pool, result.value().deps); schedule_indexing();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 580 - 582, The if-check around result is redundant because earlier code returns when !result.has_value(); remove the surrounding if and directly assign ast_deps[path_id] = capture_deps_snapshot(path_pool, result.value().deps); ensuring you still reference result, ast_deps, capture_deps_snapshot, path_id and path_pool in that statement so behavior is unchanged but simplified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 66-88: The deps_changed function fails to detect a reappearing
file that previously had snap.hashes[i]==0 because the code skips hashing when
mtime <= snap.build_at; change the mtime check so that you only skip re-hashing
when the stored hash is non-zero. Concretely, in deps_changed (and referencing
snap.hashes, snap.build_at, hash_file, PathPool, DepsSnapshot), replace the
current "if(current_mtime <= snap.build_at) continue;" with a conditional that
continues only when current_mtime <= snap.build_at && snap.hashes[i] != 0,
ensuring files with a sentinel hash of 0 are always re-hashed and compared via
hash_file.
---
Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 580-582: The if-check around result is redundant because earlier
code returns when !result.has_value(); remove the surrounding if and directly
assign ast_deps[path_id] = capture_deps_snapshot(path_pool,
result.value().deps); ensuring you still reference result, ast_deps,
capture_deps_snapshot, path_id and path_pool in that statement so behavior is
unchanged but simplified.
🪄 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: 28a7fb45-7215-477c-8af3-ce34a57f6d14
📒 Files selected for processing (2)
src/server/master_server.cppsrc/server/master_server.h
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/server/master_server.cpp (2)
46-55:⚠️ Potential issue | 🟠 MajorCapture the dependency snapshot with the artifact, not after the worker returns.
capture_deps_snapshot()re-reads dependency files and stampsbuild_aton the master after the compile/PCH worker has already finished. If a dependency changes in that gap,st.deps/ast_deps[path_id]can describe the new filesystem state while the cached AST/PCH was built from the old contents, so the next fast-path reuse becomes stale.Also applies to: 388-389, 580-582
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 46 - 55, capture_deps_snapshot() is being called after the compile/PCH worker returns, allowing a race where deps are re-read and build_at is stamped later than the artifact; fix by invoking capture_deps_snapshot(PathPool& pool, const std::vector<std::string>& deps) at the moment the artifact/deps are produced (i.e., before the worker finishes and before any worker result is applied to st.deps/ast_deps), so the snapshot reflects the exact files used to build the artifact. Update the call sites that currently perform snapshotting after worker completion (the capture_deps_snapshot usages in master_server.cpp) to capture the snapshot immediately when the worker hands off its produced deps/artifact and then attach that snapshot to the stored artifact/state instead of re-reading files afterward.
54-55:⚠️ Potential issue | 🟠 MajorThe mtime fast path still has false negatives.
build_atandcurrent_mtimeare truncated to whole seconds, so a dependency edited later in the same second skips the re-hash path and is treated as unchanged. The same unconditionalcontinuealso hides files that were unreadable during the build (snap.hashes[i] == 0) and later reappear with a preserved or older mtime; this fast path needs higher-resolution timestamps, and it should only short-circuit when a non-zero baseline hash exists.Also applies to: 77-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 54 - 55, The fast-path mtime check truncates to seconds and also skips files with zero baseline hashes; change build_at and current_mtime to use high-resolution times (e.g., std::chrono::system_clock::time_point or std::filesystem::file_time_type) so subsecond differences are preserved when computing current_mtime vs build_at, and modify the short-circuit so it only continue's when snap.hashes[i] != 0 (i.e., a non-zero baseline hash exists) rather than unconditionally; apply the same fixes to the other occurrence referenced around the code handling current_mtime (also noted at the other block ~77-80) and ensure unreadable files (snap.hashes[i] == 0) do not get hidden by the fast path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/master_server.cpp`:
- Around line 340-345: The current reuse-check leaves st.path pointing at an old
PCH while a rebuild is in-flight, so if the rebuild fails waiters still see a
non-empty path and proceed with stale data; modify the rebuild logic around
pch_states and the reuse-check (symbols: pch_states, st, st.path, preamble_hash,
deps_changed, bound) so that you clear st.path (or set an explicit
st.build_success=false) before kicking off a rebuild and only set st.path (or
st.build_success=true) after the rebuild completes successfully; also update
waiter checks to require the build-success flag (or a non-empty path set only on
success) instead of just !st.path.empty(); apply the same change for the other
reuse/rebuild sites referenced (the other pch_state updates).
- Around line 380-389: didClose() currently erases pch_states[path_id] without
removing the on-disk temp PCH, and later suspended builds recreate the map entry
causing leaked files and stale cache; update didClose() (and the similar logic
at the other occurrence) to first look up the existing state (auto it =
pch_states.find(path_id)), if found remove the file at it->second.path
(fs::remove(it->second.path)) when path is non-empty, then cancel/clear any
pending building state and finally erase the map entry (pch_states.erase(it));
also ensure places that lazily recreate pch_states (the code that does auto& st
= pch_states[path_id]) check that the document is still open before creating
entries to avoid re-creating state for closed documents.
---
Duplicate comments:
In `@src/server/master_server.cpp`:
- Around line 46-55: capture_deps_snapshot() is being called after the
compile/PCH worker returns, allowing a race where deps are re-read and build_at
is stamped later than the artifact; fix by invoking
capture_deps_snapshot(PathPool& pool, const std::vector<std::string>& deps) at
the moment the artifact/deps are produced (i.e., before the worker finishes and
before any worker result is applied to st.deps/ast_deps), so the snapshot
reflects the exact files used to build the artifact. Update the call sites that
currently perform snapshotting after worker completion (the
capture_deps_snapshot usages in master_server.cpp) to capture the snapshot
immediately when the worker hands off its produced deps/artifact and then attach
that snapshot to the stored artifact/state instead of re-reading files
afterward.
- Around line 54-55: The fast-path mtime check truncates to seconds and also
skips files with zero baseline hashes; change build_at and current_mtime to use
high-resolution times (e.g., std::chrono::system_clock::time_point or
std::filesystem::file_time_type) so subsecond differences are preserved when
computing current_mtime vs build_at, and modify the short-circuit so it only
continue's when snap.hashes[i] != 0 (i.e., a non-zero baseline hash exists)
rather than unconditionally; apply the same fixes to the other occurrence
referenced around the code handling current_mtime (also noted at the other block
~77-80) and ensure unreadable files (snap.hashes[i] == 0) do not get hidden by
the fast path.
🪄 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: e4a752d2-c5d4-499e-83e1-78c63bd944d8
📒 Files selected for processing (2)
src/server/master_server.cppsrc/server/master_server.h
- capture_deps_snapshot: const std::vector<std::string>& -> llvm::ArrayRef<std::string> - DepsSnapshot: std::vector -> llvm::SmallVector for path_ids and hashes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/server/master_server.cpp (3)
77-80:⚠️ Potential issue | 🟡 MinorForce Layer 2 when the stored dep hash is the read-failure sentinel.
If
hash_file()returned0during the build, a dep that later appears with a preserved or older mtime hits the Line 79 early-continue and is never rehashed. That can keep a stale AST/PCH alive even though the file now exists.Suggested fix
- if(current_mtime <= snap.build_at) + if(current_mtime <= snap.build_at && snap.hashes[i] != 0) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 77 - 80, The mtime-only early-continue skips rehashing deps that previously failed (hash_file returned the sentinel 0), so change the Layer 1 check to force Layer 2 when the stored dep hash equals the read-failure sentinel: when evaluating current_mtime <= snap.build_at, also check the stored dependency hash (the value written by hash_file) and only continue if that stored hash is not the sentinel (e.g., != 0); in practice update the condition around current_mtime and snap.build_at to: if (current_mtime <= snap.build_at && stored_dep_hash != HASH_READ_FAILURE_SENTINEL) continue, ensuring hash_file() results of 0 trigger rehashing/Layer 2 logic (reference current_mtime, snap.build_at, hash_file()).
349-351:⚠️ Potential issue | 🟠 MajorFailed rebuilds and
didClose()can leavepch_statesin an invalid state.After
didClose()erases the entry, Lines 351, 374, and 380 usepch_states[path_id]and silently recreate a freshPCHStatefor a closed document. Separately, the oldst.pathis kept until a successful rebuild, so waiters can still returntrueand compile against a PCH that the reuse check already rejected. Re-find the entry after suspension, remove the on-disk file before erasing, and clear the invalidated path before launching the rebuild.For the LLVM version used by this repo, does llvm::DenseMap::operator[] insert a default-constructed value when the key is missing?Suggested fix
if(auto it = pch_states.find(path_id); it != pch_states.end() && it->second.building) { co_await it->second.building->wait(); - co_return !pch_states[path_id].path.empty(); + auto done_it = pch_states.find(path_id); + co_return done_it != pch_states.end() && !done_it->second.path.empty(); } // Register in-flight build so concurrent requests wait on us. auto completion = std::make_shared<et::event>(); - pch_states[path_id].building = completion; + auto& st = pch_states[path_id]; + if(!st.path.empty()) { + fs::remove(st.path); + st.path.clear(); + } + st.building = completion; @@ if(!result.has_value() || !result.value().success) { LOG_WARN("PCH build failed for {}: {}", path, result.has_value() ? result.value().error : result.error().message); - pch_states[path_id].building.reset(); + if(auto st_it = pch_states.find(path_id); st_it != pch_states.end()) { + st_it->second.building.reset(); + } completion->set(); co_return false; } - auto& st = pch_states[path_id]; + auto st_it = pch_states.find(path_id); + if(st_it == pch_states.end() || documents.find(path_id) == documents.end()) { + completion->set(); + co_return false; + } + auto& st = st_it->second; @@ - pch_states.erase(path_id); + if(auto pch_it = pch_states.find(path_id); pch_it != pch_states.end()) { + fs::remove(pch_it->second.path); + pch_states.erase(pch_it); + }Also applies to: 356-389, 1424-1426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 349 - 351, The code currently uses pch_states[path_id] after suspension and when erasing entries, which can silently re-create a default PCHState and return stale st.path; to fix this, after every co_await (e.g., after co_await it->second.building->wait() in the block that references pch_states and in the rebuild-launch paths) re-find the entry with pch_states.find(path_id) and bail if not found, avoid using operator[] (do not use pch_states[path_id]); when erasing an entry (e.g., in didClose()/rebuild failure paths) first remove the on-disk PCH file if present, then clear the PCHState.path (set to empty) before erasing or before launching a rebuild so waiters cannot observe a stale path, and ensure all modifications reference the PCHState via the iterator (it->second or the re-found iterator) rather than operator[].
46-55:⚠️ Potential issue | 🟠 Major
build_atis not a safe freshness baseline.
capture_deps_snapshot()runs after the worker has already finished, and Line 78 compares mtimes only after both sides have been truncated to whole seconds. A dep edit in that handoff window—or any same-second edit—can still pass the fast path and reuse an AST/PCH built from older contents. This baseline needs to be captured in the worker at build time and kept at sub-second precision.For the LLVM version used by this repo, does llvm::sys::toTimeT(status.getLastModificationTime()) truncate sub-second precision?Also applies to: 77-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 46 - 55, capture_deps_snapshot currently records build_at with second-resolution after the worker finished, allowing same-second edits to slip through; instead capture a high-resolution timestamp at build time inside the worker and store it in DepsSnapshot with sub-second precision (e.g. as a std::chrono::system_clock::time_point or an integer nanosecond/unix epoch field) so comparisons against modification times use that original high-res baseline; update capture_deps_snapshot (and any comparisons around build_at/use in lines ~77-80) to consume this high-resolution field and avoid using llvm::sys::toTimeT or any truncating conversion when deciding reuse, and ensure hash_file/path intern logic still runs but does not replace the early captured timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/master_server.cpp`:
- Around line 77-80: The mtime-only early-continue skips rehashing deps that
previously failed (hash_file returned the sentinel 0), so change the Layer 1
check to force Layer 2 when the stored dep hash equals the read-failure
sentinel: when evaluating current_mtime <= snap.build_at, also check the stored
dependency hash (the value written by hash_file) and only continue if that
stored hash is not the sentinel (e.g., != 0); in practice update the condition
around current_mtime and snap.build_at to: if (current_mtime <= snap.build_at &&
stored_dep_hash != HASH_READ_FAILURE_SENTINEL) continue, ensuring hash_file()
results of 0 trigger rehashing/Layer 2 logic (reference current_mtime,
snap.build_at, hash_file()).
- Around line 349-351: The code currently uses pch_states[path_id] after
suspension and when erasing entries, which can silently re-create a default
PCHState and return stale st.path; to fix this, after every co_await (e.g.,
after co_await it->second.building->wait() in the block that references
pch_states and in the rebuild-launch paths) re-find the entry with
pch_states.find(path_id) and bail if not found, avoid using operator[] (do not
use pch_states[path_id]); when erasing an entry (e.g., in didClose()/rebuild
failure paths) first remove the on-disk PCH file if present, then clear the
PCHState.path (set to empty) before erasing or before launching a rebuild so
waiters cannot observe a stale path, and ensure all modifications reference the
PCHState via the iterator (it->second or the re-found iterator) rather than
operator[].
- Around line 46-55: capture_deps_snapshot currently records build_at with
second-resolution after the worker finished, allowing same-second edits to slip
through; instead capture a high-resolution timestamp at build time inside the
worker and store it in DepsSnapshot with sub-second precision (e.g. as a
std::chrono::system_clock::time_point or an integer nanosecond/unix epoch field)
so comparisons against modification times use that original high-res baseline;
update capture_deps_snapshot (and any comparisons around build_at/use in lines
~77-80) to consume this high-resolution field and avoid using llvm::sys::toTimeT
or any truncating conversion when deciding reuse, and ensure hash_file/path
intern logic still runs but does not replace the early captured timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4827457-58a2-4c2a-86d4-69d476c99ec9
📒 Files selected for processing (2)
src/server/master_server.cppsrc/server/master_server.h
- Fix TOCTOU: capture build_at before hashing deps, not after - Remove dead if(result.has_value()) guard in ensure_compiled - Guard deps() call with unit.completed() in stateful worker - Fix time.sleep -> await asyncio.sleep in integration tests - Remove misleading doc comment on hash_file - Move import shutil to top of test file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/server/master_server.cpp (3)
1424-1425:⚠️ Potential issue | 🟠 Major
didCloseshould tear down the cached PCH, not just forget it.These lines drop the map entry without removing the temp file, and the async write-back paths touch
pch_states[path_id]again at Line 375 and Line 381. That leaks PCH files and lets a suspended build recreate cache state after the document is already closed. Remove the cached file before erasing, and have the post-build path discard results when the document no longer exists.♻️ Local cleanup for the close path
- pch_states.erase(path_id); + if(auto pch_it = pch_states.find(path_id); pch_it != pch_states.end()) { + if(!pch_it->second.path.empty()) + fs::remove(pch_it->second.path); + pch_states.erase(pch_it); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 1424 - 1425, In didClose, before erasing pch_states[path_id] and ast_deps[path_id], delete the associated cached PCH temp file (use the path stored in pch_states[path_id] or its temp-file handle) so the on-disk artifact is removed; then erase the map entries. Also update the async post-build/write-back paths that reference pch_states (the async callbacks that run after builds) to check whether pch_states contains path_id before writing back and simply discard results if the document was closed/entry removed, avoiding reinstating cache state for closed documents.
45-56:⚠️ Potential issue | 🟠 MajorThe dep snapshot can still bless a stale artifact.
At both call sites this snapshot is recorded only after the worker has already finished, and Lines 49 and 79-80 reduce the freshness check to whole seconds. A dep edit in the post-build gap, or two edits within the same second, can still be persisted as “fresh” for an AST/PCH built from older bytes. This needs to come from the worker’s build-time view, and the comparison should use full-resolution file times instead of
time_t.Also applies to: 79-80, 389-389, 581-581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 45 - 56, capture_deps_snapshot currently records build_at as a coarse time_t at the end of the worker run and hashes files then, allowing TOCTOU/stale-artifact races; change capture_deps_snapshot (and the DepsSnapshot type) to accept/record the worker-provided high-resolution build time (std::chrono::system_clock::time_point or std::filesystem::file_time_type) instead of calling to_time_t, and store per-file high-resolution mtimes (filesystem::last_write_time) alongside the path_ids and hashes; ensure callers invoke capture_deps_snapshot at build start (worker’s build-time view) before the worker finishes so the snapshot reflects the exact build-time, and keep hash_file and PathPool::intern usage unchanged while updating any comparisons to use the high-resolution time type rather than time_t.
350-352:⚠️ Potential issue | 🟠 MajorWakeups should revalidate against the current preamble.
Line 352 treats any non-empty path as success after waiting. If the preamble changed while the first build was running—or that rebuild failed after leaving the previous path in place—the waiter can compile once against a PCH we already know is stale. After
wait(), rerun the reuse check for the currentpreamble_hash/bound/deps instead of returning on!path.empty().Also applies to: 357-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.cpp` around lines 350 - 352, After waiting on the existing build, don't assume a non-empty path means the PCH is still valid; re-check the current pch_states entry for path_id against the live preamble parameters (preamble_hash, bound, deps) and only return success if those match the current request. In other words, within the block handling pch_states.find(path_id) and after co_await it->second.building->wait(), recompute or compare it->second.preamble_hash / it->second.bound / it->second.deps (or call the existing reuse-check helper if present) and only co_return true when that reuse check passes; otherwise fall through to trigger a new build or return false. Ensure the same revalidation logic is applied to the duplicate case around line 357 as well.
🧹 Nitpick comments (1)
tests/integration/test_staleness.py (1)
115-131:test_no_change_skips_recompiledoesn't assert the “skips recompile” part.A second hover that fully recompiles and returns the same result still passes here. Consider checking a deterministic side effect of recompilation—e.g. a compile counter/log hook, or that no diagnostics notification is published—so this test actually guards the fast path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_staleness.py` around lines 115 - 131, The test currently only verifies hover result but not that recompilation was skipped; update test_no_change_skips_recompile to assert a deterministic indicator that compilation did not occur after the second hover — e.g., before the second hover snapshot a compile counter or log hook exposed by the server (or the client test harness), call client.text_document_hover_async again, then assert the compile counter did not increase (or that no new "textDocument/publishDiagnostics" notification was emitted). Wire this check to existing test hooks (use the compile counter/log hook name or the client's diagnostics/notification collection used elsewhere in the test harness) so the test fails if a full recompile happens. Ensure you add only the assertion and minimal instrumentation in the test (test_no_change_skips_recompile) referring to those existing symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/server/master_server.cpp`:
- Around line 1424-1425: In didClose, before erasing pch_states[path_id] and
ast_deps[path_id], delete the associated cached PCH temp file (use the path
stored in pch_states[path_id] or its temp-file handle) so the on-disk artifact
is removed; then erase the map entries. Also update the async
post-build/write-back paths that reference pch_states (the async callbacks that
run after builds) to check whether pch_states contains path_id before writing
back and simply discard results if the document was closed/entry removed,
avoiding reinstating cache state for closed documents.
- Around line 45-56: capture_deps_snapshot currently records build_at as a
coarse time_t at the end of the worker run and hashes files then, allowing
TOCTOU/stale-artifact races; change capture_deps_snapshot (and the DepsSnapshot
type) to accept/record the worker-provided high-resolution build time
(std::chrono::system_clock::time_point or std::filesystem::file_time_type)
instead of calling to_time_t, and store per-file high-resolution mtimes
(filesystem::last_write_time) alongside the path_ids and hashes; ensure callers
invoke capture_deps_snapshot at build start (worker’s build-time view) before
the worker finishes so the snapshot reflects the exact build-time, and keep
hash_file and PathPool::intern usage unchanged while updating any comparisons to
use the high-resolution time type rather than time_t.
- Around line 350-352: After waiting on the existing build, don't assume a
non-empty path means the PCH is still valid; re-check the current pch_states
entry for path_id against the live preamble parameters (preamble_hash, bound,
deps) and only return success if those match the current request. In other
words, within the block handling pch_states.find(path_id) and after co_await
it->second.building->wait(), recompute or compare it->second.preamble_hash /
it->second.bound / it->second.deps (or call the existing reuse-check helper if
present) and only co_return true when that reuse check passes; otherwise fall
through to trigger a new build or return false. Ensure the same revalidation
logic is applied to the duplicate case around line 357 as well.
---
Nitpick comments:
In `@tests/integration/test_staleness.py`:
- Around line 115-131: The test currently only verifies hover result but not
that recompilation was skipped; update test_no_change_skips_recompile to assert
a deterministic indicator that compilation did not occur after the second hover
— e.g., before the second hover snapshot a compile counter or log hook exposed
by the server (or the client test harness), call
client.text_document_hover_async again, then assert the compile counter did not
increase (or that no new "textDocument/publishDiagnostics" notification was
emitted). Wire this check to existing test hooks (use the compile counter/log
hook name or the client's diagnostics/notification collection used elsewhere in
the test harness) so the test fails if a full recompile happens. Ensure you add
only the assertion and minimal instrumentation in the test
(test_no_change_skips_recompile) referring to those existing symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6bd4662-946e-4d29-bc2b-e30984b77e18
📒 Files selected for processing (4)
src/server/master_server.cppsrc/server/master_server.hsrc/server/stateful_worker.cpptests/integration/test_staleness.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/stateful_worker.cpp
Member
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
16bit-ykiko
changed the title
This was referenced Apr 5, 2026
Merged
Merged
4 of 5 tasks
This was referenced Jul 7, 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.
Summary
Replace the
didSavesledgehammer (pch_hashes.clear()+ mark-all-dirty) with precise per-file dependency tracking that avoids unnecessary recompilation.Two-layer staleness detection
After each successful compilation, a
DepsSnapshotis captured (interned path IDs + xxh3 content hashes + timestamp). On the next feature request,deps_changed()checks:build_at. If mtime is older → skip. Uses strict<so same-second modifications fall through.Special cases: files unreadable at build time (hash=0) always fall through to Layer 2; disappeared files are detected via stat failure.
Consolidated PCH state
Scatter of four maps (
pch_paths,pch_bounds,pch_hashes,pch_building) → singlePCHStatestruct withpath,bound,hash,deps,buildingfields.DepsSnapshotandSymbolInfomoved out ofMasterServerto namespace scope.Concurrent compilation dedup
ensure_compiled:DocumentState::compilingevent prevents duplicate AST compilations. Waitersco_awaitthe event and checkast_dirtyafter waking. When deps change is detected during an in-flight build,generationis bumped so the builder's generation check prevents it from incorrectly clearingast_dirty.ensure_pch:PCHState::buildingevent deduplicates PCH builds. Waiters re-validatepreamble_hashafter waking to handle edits during the wait. Thebound==0path waits for in-flight builds before erasing. Old PCH is deleted and path cleared before rebuild starts, so waiters never see a stale path on failure.didSavechangesRemoved the blanket
pch_hashes.clear()+ mark-all-dirty on save. Staleness is now detected lazily viadeps_changed()at the next feature request.didSavestill invalidatesCompileGraphdependents for module deps.FIXME noted
Rapid
didChangeedits (especially preamble changes) can cause the stateful worker to compile with stale/concatenated text. Root cause is in the worker, not in staleness tracking — noted as FIXME for a follow-up PR.Test plan
13 integration tests covering:
🤖 Generated with Claude Code