Repository navigation
feat: integrate PCH into MasterServer build drain - #381
Merged
Merged
Conversation
…rding Add `ensure_pch()` to MasterServer that builds/reuses precompiled headers via stateless workers. PCH staleness is detected by hashing the preamble content (xxh3_64bits) and comparing with the cached hash. The PCH is used in both the build drain loop (stateful compile) and stateless forwarding (completion/signatureHelp). Key changes: - Add `preamble_bound` field to `BuildPCHParams` so the stateless worker truncates remapped content at the preamble boundary - Fix `BuildPCHResult` to return the PCH file path - Add PCH state maps (pch_paths, pch_bounds, pch_hashes) to MasterServer - Clean up PCH state on didClose; invalidate hash on didSave - Add unit tests (PreambleHash, PCHWorker) and integration tests (test_pch) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughMasterServer computes a preamble bound and xxh3 hash, builds or reuses per-file PCH via stateless workers with in‑flight coordination, caches pch paths/bounds/hashes, attaches PCH info to compile/forwarded requests, and invalidates caches on didSave/didClose. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Master as MasterServer
participant Pool as StatelessWorkerPool
participant Stateful as StatefulWorker
participant FS as Filesystem
Client->>Master: didOpen / run_build_drain / forward_stateless
Master->>Master: compute_preamble_bound + xxh3 hash
alt cached & hash matches
Master-->>Client: use cached PCH info
else missing or stale
Master->>Pool: BuildPCHParams{file, dir, args, content, preamble_bound}
Pool->>FS: compile header -> produce PCH
FS-->>Pool: pch_path
Pool-->>Master: BuildPCHResult{pch_path}
Master->>Master: store pch_path, pch_hash, pch_bound
end
Master->>Stateful: CompileParams{..., pch={pch_path,preamble_bound}}
Stateful->>FS: compile source using PCH
FS-->>Stateful: diagnostics/result
Stateful-->>Master: CompileResult
Master-->>Client: publishDiagnostics / hover / completion
Client->>Master: didSave / didClose
Master->>Master: invalidate/erase cache entries
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: 4
🧹 Nitpick comments (3)
tests/unit/server/pch_worker_tests.cpp (2)
105-106: Consider RAII-based cleanup for the PCH temp file.If an assertion fails before line 106, the PCH file leaks. A scope guard would ensure cleanup regardless of test outcome:
♻️ Suggested scope guard pattern
// After line 70: auto pch_cleanup = llvm::make_scope_exit([&] { if (!pch_path.empty()) std::remove(pch_path.c_str()); });Then remove line 106.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/server/pch_worker_tests.cpp` around lines 105 - 106, Replace the manual cleanup call for the PCH temp file with an RAII scope guard: create a scope exit (e.g., pch_cleanup using llvm::make_scope_exit) after pch_path is initialized so it always calls std::remove(pch_path.c_str()) if pch_path is non-empty, and then remove the explicit std::remove(pch_path.c_str()) call currently present in the test; reference the pch_path variable and the new pch_cleanup scope guard to locate the change.
36-37: WorkerHandle instances should use RAII or explicit cleanup to avoid orphaned processes on test failure.The test creates local
WorkerHandleinstances (lines 36–37, 73–74) that spawn child processes but have no destructor. If an assertion fails afterspawn(), theWorkerHandlegoes out of scope without explicitly terminating the worker process, potentially leaving it orphaned. Compare withWorkerPool::stop(), which properly closes pipes, sendsSIGTERM, and waits for exit. Add a destructor toWorkerHandle(inworker_test_helpers.h) that callsproc.kill(SIGTERM)andco_await proc.wait(), or use a scope guard to ensure cleanup on test failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/server/pch_worker_tests.cpp` around lines 36 - 37, The test spawns worker processes via WorkerHandle::spawn but lacks guaranteed cleanup; modify WorkerHandle (in worker_test_helpers.h) to perform RAII cleanup by adding a destructor or scope guard that ensures the child is terminated and reaped: call proc.kill(SIGTERM) (or equivalent) and then await/reap the process with proc.wait() (mirroring WorkerPool::stop's close pipes, send SIGTERM, wait logic), so WorkerHandle always kills and waits for its child on destruction or failure.tests/integration/test_pch.py (1)
86-94: Replaceasyncio.sleep(1.0)with event-based wait to avoid flaky test.Line 86 creates a diagnostics event but line 94 uses a hardcoded sleep instead of awaiting it. This is inconsistent with
test_pch_body_edit_triggers_recompileand makes the test flaky under load.♻️ Proposed fix
event = client.wait_for_diagnostics(uri) client.text_document_did_change( DidChangeTextDocumentParams( text_document=VersionedTextDocumentIdentifier(uri=uri, version=1), content_changes=[TextDocumentContentChangeWholeDocument(text=new_content)], ) ) - # Brief wait for the change to be processed. - await asyncio.sleep(1.0) + # Wait for the change to be processed. + await asyncio.wait_for(event.wait(), timeout=30.0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_pch.py` around lines 86 - 94, The test currently calls client.wait_for_diagnostics(uri) but ignores its returned event and uses asyncio.sleep(1.0), causing flakiness; replace the hardcoded sleep by awaiting the diagnostics event returned earlier (the result of client.wait_for_diagnostics(uri)) after sending the change via DidChangeTextDocumentParams/VersionedTextDocumentIdentifier/TextDocumentContentChangeWholeDocument so the test waits deterministically for diagnostics completion instead of a fixed delay.
🤖 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 393-436: Add an in-flight guard to prevent concurrent builds for
the same path_id: introduce a member llvm::DenseSet<std::uint32_t> pch_building
and a std::mutex pch_mutex in the class, and in ensure_pch check under pch_mutex
whether path_id is in pch_building (if so, release the lock and wait until it is
removed or simply return true/await the existing build result as your policy),
else insert path_id into pch_building before calling
pool.send_stateless(pch_params); ensure you remove path_id from pch_building in
every exit path (on success and on failure) so concurrent callers see the
updated state; keep all references to pch_paths, pch_bounds, pch_hashes
unchanged but ensure updates happen after removing the in-flight guard or while
still holding correct synchronization.
In `@src/server/stateless_worker.cpp`:
- Line 74: Validate params.preamble_bound before calling cp.add_remapped_file:
check that params.preamble_bound is either the allowed sentinel value (e.g., the
codebase’s “no preamble” constant) or <= params.content.size(); if it’s larger
and not the sentinel, clamp it or skip/return an error path so you never call
cp.add_remapped_file with an out-of-range preamble_bound. Update the call site
that invokes cp.add_remapped_file (the line using params.file, params.content,
params.preamble_bound) to perform this guard and use the sentinel/size-safe
value when invoking the method.
In `@tests/integration/test_pch.py`:
- Line 22: The pch_test workspace lacks build configuration so tests fail; add a
CMakeLists.txt to the pch_test test data (following the pattern used in
tests/data/modules/) so the workspace fixture can generate
compile_commands.json, or update the workspace fixture in conftest.py to
special-case "pch_test" (like the existing handling for "hello_world") and emit
an appropriate compile_commands.json/compilation flags to enable include
resolution and PCH testing; reference the workspace name "pch_test" and the
workspace fixture in conftest.py/hello_world handling when making the change.
In `@tests/unit/server/stateless_worker_tests.cpp`:
- Around line 104-106: Change the first check so the test aborts if the optional
is empty: replace the EXPECT_TRUE(result.has_value()) with
ASSERT_TRUE(result.has_value()) so subsequent calls to result.value() (used in
result.value().success and result.value().pch_path.empty()) are safe; optionally
consider making result.value().success an ASSERT_TRUE as well to avoid further
dereference on failure.
---
Nitpick comments:
In `@tests/integration/test_pch.py`:
- Around line 86-94: The test currently calls client.wait_for_diagnostics(uri)
but ignores its returned event and uses asyncio.sleep(1.0), causing flakiness;
replace the hardcoded sleep by awaiting the diagnostics event returned earlier
(the result of client.wait_for_diagnostics(uri)) after sending the change via
DidChangeTextDocumentParams/VersionedTextDocumentIdentifier/TextDocumentContentChangeWholeDocument
so the test waits deterministically for diagnostics completion instead of a
fixed delay.
In `@tests/unit/server/pch_worker_tests.cpp`:
- Around line 105-106: Replace the manual cleanup call for the PCH temp file
with an RAII scope guard: create a scope exit (e.g., pch_cleanup using
llvm::make_scope_exit) after pch_path is initialized so it always calls
std::remove(pch_path.c_str()) if pch_path is non-empty, and then remove the
explicit std::remove(pch_path.c_str()) call currently present in the test;
reference the pch_path variable and the new pch_cleanup scope guard to locate
the change.
- Around line 36-37: The test spawns worker processes via WorkerHandle::spawn
but lacks guaranteed cleanup; modify WorkerHandle (in worker_test_helpers.h) to
perform RAII cleanup by adding a destructor or scope guard that ensures the
child is terminated and reaped: call proc.kill(SIGTERM) (or equivalent) and then
await/reap the process with proc.wait() (mirroring WorkerPool::stop's close
pipes, send SIGTERM, wait logic), so WorkerHandle always kills and waits for its
child on destruction or failure.
🪄 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: 93723cfd-e9bb-4c75-8025-00e9c0d5ca02
📒 Files selected for processing (11)
src/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/stateless_worker.cpptests/data/pch_test/common.htests/data/pch_test/main.cpptests/data/pch_test/no_includes.cpptests/integration/test_pch.pytests/unit/compile/compilation_tests.cpptests/unit/server/pch_worker_tests.cpptests/unit/server/stateless_worker_tests.cpp
- Prevent duplicate concurrent PCH builds via pch_building guard set - Guard against UB in PCH worker test when result has no value Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
feature/pch-integration
branch
from
April 1, 2026 08:31
0a77021 to
e0b4ec8
Compare
- Fix ensure_pch race: update pch_paths/bounds/hashes before signaling waiters, so they see the completed state - Remove waiter's bound override that could mismatch the actual PCH - Add self-PCM exclusion in forward_stateless to prevent clang "multiple module declarations" errors - Clean up old PCH temp file when replacing with a new build - Clean up temp file on PCH compilation failure in stateless worker Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/server/master_server.cpp (1)
774-776:⚠️ Potential issue | 🟡 Minor
didCloseforgets the cache entry but leaves the temp file behind.Erasing
pch_paths[path_id]here drops the only handle to the generated PCH, so repeated open/close cycles leak files in the cache/temp directory. Do a best-effort remove before clearing the metadata.🧹 Proposed cleanup
- pch_paths.erase(path_id); + if(auto pch_it = pch_paths.find(path_id); pch_it != pch_paths.end()) { + fs::remove(pch_it->second); + pch_paths.erase(path_id); + } pch_bounds.erase(path_id); pch_hashes.erase(path_id);🤖 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 774 - 776, In didClose, before erasing the metadata entries (pch_paths, pch_bounds, pch_hashes) for path_id, attempt a best-effort removal of the underlying temporary PCH file referenced by pch_paths[path_id] (e.g., check that the path exists and call std::filesystem::remove or equivalent), log or ignore any errors, then erase the entries; this ensures the on-disk temp is cleaned up even if erasure of the metadata follows or fails.
🤖 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 813-814: The current save logic only erases pch_hashes for the
saved main file (pch_hashes.erase(path_id)) which doesn't catch headers included
by other translation units; update the save handler so that after writing a file
you invalidate any PCHs for open documents that could depend on that file (use a
reverse-include lookup if one exists to find TUs that include the saved file and
erase their entries from pch_hashes), and if no reverse-include map is available
implement the safer fallback of clearing the entire pch_hashes map so no TU can
reuse a stale PCH; refer to ensure_pch() (which keys on preamble bytes) and
CompilationParams::add_remapped_file() to understand why only removing the saved
file's key is insufficient.
- Around line 397-457: ensure_pch() can leave stale PCH entries across preamble
changes; fix by always validating the preamble_hash on every exit path and
clearing stale metadata when invalid. Specifically: when bound == 0, erase
pch_paths[p], pch_hashes[p], pch_bounds[p] (use path_id) before returning; when
waiting on an in-flight build (the block that uses
pch_building[path_id]->wait()), recompute preamble_hash for the current text and
return false (and clear any stale pch_paths/pch_hashes/pch_bounds) if it no
longer matches; on build failure (where result is false) erase
pch_paths/pch_hashes/pch_bounds for path_id before signaling completion; and
after a successful build but before publishing it (just before assigning
pch_paths[path_id] = result.value().pch_path), re-check that the current
preamble_hash still equals the build’s preamble_hash and if not, delete the
produced PCH and do not install it (erase building entry, set completion, and
return false). Ensure all erasures reference the same path_id keys (pch_paths,
pch_hashes, pch_bounds, pch_building) so no stale entry remains.
- Around line 541-558: After the co_await ensure_pch(...) suspension, the code
uses doc.text which may be invalidated; change the PositionMapper instantiation
to use the already-captured wp.text instead. Locate the PositionMapper
construction (symbol: PositionMapper mapper(doc.text,
lsp::PositionEncoding::UTF16)) and replace doc.text with wp.text so it reads
PositionMapper mapper(wp.text, lsp::PositionEncoding::UTF16), ensuring you
reference the wp variable populated before ensure_pch.
---
Duplicate comments:
In `@src/server/master_server.cpp`:
- Around line 774-776: In didClose, before erasing the metadata entries
(pch_paths, pch_bounds, pch_hashes) for path_id, attempt a best-effort removal
of the underlying temporary PCH file referenced by pch_paths[path_id] (e.g.,
check that the path exists and call std::filesystem::remove or equivalent), log
or ignore any errors, then erase the entries; this ensures the on-disk temp is
cleaned up even if erasure of the metadata follows or fails.
🪄 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: 1c226bb9-5b62-4dbd-a64d-bcd75e8494f6
📒 Files selected for processing (2)
src/server/master_server.cppsrc/server/stateless_worker.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/stateless_worker.cpp
- Clear stale PCH entries when bound == 0 (all #includes removed) - Use wp.text instead of doc.text after co_await to avoid use-after-free if document is closed during suspension - Invalidate all PCH hashes on save, not just the saved file's, since the saved file may be a header included by other TUs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 780-782: The PCH path entry is erased from pch_paths without
deleting the actual file, leaking temp files—before calling
pch_paths.erase(path_id) (in the same place where pch_bounds.erase(path_id) and
pch_hashes.erase(path_id) are invoked), retrieve the path string from
pch_paths[path_id], check for file existence (e.g., std::filesystem::exists or
equivalent), attempt to delete the file (e.g., std::filesystem::remove or
unlink), and log or handle any error if deletion fails, then proceed to erase
pch_paths, pch_bounds, and pch_hashes for path_id.
- Around line 461-464: The wake-up race occurs because
pch_building.erase(path_id) is performed before notifying waiters; between erase
and completion->set() a new requester can observe no entry and insert a fresh
one, so old waiters wake on the wrong event. Fix by swapping the operations in
the completion sequence: call completion->set() first to notify all waiters,
then remove the in-flight entry via pch_building.erase(path_id), ensuring
waiters observe a consistent pch_building state; update the code around the
completion handling where completion->set() and pch_building.erase(path_id) are
invoked.
🪄 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: d190ba94-b029-4b42-98f6-ecd1aee155e9
📒 Files selected for processing (1)
src/server/master_server.cpp
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Apr 3, 2026
This was referenced Apr 22, 2026
This was referenced Jun 27, 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
ensure_pch()helper to MasterServer that builds/reuses precompiled headers via stateless workers, with preamble hash-based staleness detection (xxh3_64bits)BuildPCHParamsto carrypreamble_boundso the stateless worker truncates content at the preamble boundary (fixes redefinition errors when PCH included full file)run_build_drain(stateful compile path) andforward_stateless(completion/signatureHelp path)didCloseand hash invalidation ondidSaveTest plan
test_pch.py: diagnostics on open, body edit recompile, no-include file, hover with PCH, completion with PCH)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests