Repository navigation
fix(server): retract corrupt PCH pairs instead of trusting them - #517
Merged
Merged
Conversation
A corrupt .pch or .pch.idx on disk was trusted for the life of the cache store: deps snapshots are the only invalidation channel, so a blob the frontend could not read was never written back as invalid. The document went silently dead (empty diagnostics published as current, dirty flag cleared) and no restart healed it. - CompileResult now carries CompileStatus (Done/Cancelled/SetupFail) plus pch_suspect: the parse failed and its diagnostics blame the consumed PCH — naming its path, or an AST-deserialization error (category-based; that family's messages do not reliably carry the path) naming no other prebuilt input. - run_compile gains an artifact quality gate: a pch_suspect reply retracts the pair (store + cache) and reruns the round once, so the pair is rebuilt and real diagnostics recover within one request. A worker crash while consuming a PCH retracts the pair too — deep corruption aborts the AST reader before any diagnostic can anchor — with recovery on the next request; a genuinely poisonous document still pays its own quarantine budget. - Non-Done replies are no longer settled as success: the dirty flag stays set, deps/index are not recorded, and the gap is published as versionless empty diagnostics. - Workspace::preamble_state is now the single consumption gate for .pch.idx blobs: an unreadable blob retracts the on-disk pair so later sessions rebuild instead of silently degrading forever.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change adds deserialization-error classification, explicit compile statuses, centralized PCH state loading, corrupted-artifact invalidation and retry handling, plus unit and integration coverage for PCH corruption and setup failures. ChangesPCH diagnostics and compile result handling
PCH cache consumption and recovery
Recovery validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant StatefulWorker
participant Compiler
participant Workspace
participant PCHStore
StatefulWorker->>Compiler: Return pch_suspect or non-Done status
Compiler->>Workspace: Invalidate consumed PCH key
Workspace->>PCHStore: Retract PCH and index blobs
Compiler->>StatefulWorker: Retry compilation once
StatefulWorker-->>Compiler: Return rebuilt compilation result
🚥 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 |
![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
🤖 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/compiler.cpp`:
- Around line 1025-1031: Capture the PCH key associated with params.pch.first
before send_stateful can suspend, rather than reading the mutable
session->pch_key afterward. In the worker-crashed branch and related
diagnostic-based invalidation, use the captured key to retract only the pair
consumed by this dispatch, while preserving the existing empty-key and reset
behavior.
- Around line 1064-1077: Update the suspect-PCH handling in the compile attempt
loop so a suspect pair is always invalidated and session->pch_key is cleared
whenever pch_suspect, a PCH key, and a nonempty params.pch.first are present.
Use artifact_retried only to gate the retry and --attempt/continue path; when
the retry budget is exhausted, fall through to the non-result handling path so
dirty state remains set.
In `@tests/integration/compilation/test_persistent_cache.py`:
- Around line 610-614: Before the pytest.skip branch in the persistent-cache
test, shut down the manually created c2 server and its associated I/O tasks
using the existing cleanup mechanism. Ensure cleanup runs before skipping the
semantically dead mid-file corruption case, while preserving the current skip
behavior.
In `@tests/integration/compilation/test_staleness.py`:
- Around line 637-640: Extend the retry verification around wait_for_recompile
to assert that the retried request publishes no diagnostics for uri, rather than
only confirming recompilation occurred. Preserve the existing client.diagnostics
cleanup and wait, and validate the empty result after the retry completes.
🪄 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: a6882c71-4169-4d71-8fe0-cfb9dd65a72c
📒 Files selected for processing (14)
src/compile/diagnostic.cppsrc/compile/diagnostic.hsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/protocol/worker.hsrc/server/service/feature_router.cppsrc/server/service/query.cppsrc/server/state/workspace.cppsrc/server/state/workspace.hsrc/server/worker/stateful_worker.cpptests/integration/compilation/test_persistent_cache.pytests/integration/compilation/test_staleness.pytests/tools/workspace.pytests/unit/compile/compilation_tests.cpp
![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: 7b5bdcddd5
ℹ️ 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".
The middle-corruption vector intentionally crashes a worker; Debug builds abort the master on the resulting WorkerCrash anomaly unless CLICE_ANOMALY_NO_TRAP is set, the same pattern test_crash_recovery.py uses. Reproduced the Debug CI failure locally (master SIGABRT during recovery), green after; full Debug integration suite passes.
- Snapshot the dispatched pch key per round: pch_key can be rewritten while the send is suspended, and blaming the current key could delete an unrelated pair. Reset the session key only when it still matches. - Retract a blamed pair even after the retry budget is spent (a blamed pair never survives); the round then proceeds to publish its real fatal diagnostics or the honest empty gap. - Drop unusable units (neither complete nor fatal) in the stateful worker after the reply is built: they can never serve a query but pin the consumed artifacts — on Windows a mapped PCH cannot be replaced, blocking the rebuild of a retracted pair. - Tests: shut the second session down before pytest.skip in the dead-bytes case; assert the retried non-result stays an empty gap.
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.
Background
Compilation artifacts (the PCH and its paired index blob) are validated purely through dependency snapshots: as long as the source files they were built from are unchanged, the blobs are trusted. Nothing ever checks that the bytes on disk are still readable, and a consumption failure was never written back as invalidation. Three consequences:
.pchcorrupted on disk (bit rot, partial write, external interference) was trusted for the life of the cache store. The stateful compile failed before parsing, the master treated the incomplete reply as success — published empty diagnostics as current, cleared the dirty flag, recorded an empty deps snapshot that nothing could ever invalidate — and the file went silently dead. Restarts did not heal it.CompileResulthad no success signal at all, so any non-result (setup failure, interrupted parse) was settled as a valid product..pch.idxwas detected when first opened, but only the in-memory path was cleared. The on-disk pair kept posing as complete, so every later session re-adopted it from the cache metadata and silently served degraded results (no preamble overlay, no preamble links) forever.Investigating the repro also surfaced a third failure shape: corruption past the validation-covered region can abort the AST reader outright (
report_fatal_errorin the bitstream reader), killing the worker process. And notably, sparse flips in semantically dead bytes are consumed without any error at all — PCH blobs carry no whole-file checksum — which shaped the test vectors.Changes
CompileResultnow carriesCompileStatus(Done/Cancelled/SetupFail) plus apch_suspectflag: the parse failed and its diagnostics blame the consumed PCH — they name the blob's path, or they are AST-deserialization errors (matched by clang's diagnostic category via the newDiagnosticID::is_deserialization_error(), since that family's messages do not reliably carry the path, e.g.malformed or corrupted precompiled file: 'Blob ends too soon') that name no other prebuilt input. A bare setup failure without that attribution deliberately does not blame the PCH: retracting a healthy shared pair over a broken module input or bad invocation would rebuild it on every request.run_compilegains an artifact quality gate: on apch_suspectreply it retracts the pair (store + in-memory cache) and reruns the round once —ensure_pchmisses and rebuilds both halves, so real diagnostics recover within the same request. A worker crash while consuming a PCH retracts the pair too (deep corruption aborts the reader before any diagnostic can anchor); recovery lands on the next request, and a genuinely poisonous document still pays its own quarantine budget.Donereplies are no longer settled: the dirty flag stays set, deps/index are not recorded, and the gap is published as versionless empty diagnostics instead of a stale list posing as current.Workspace::preamble_state()is now the single consumption gate for the index blob: when it turns out unreadable, the on-disk pair is retracted as well, so the nextensure_pch(this session or any later one) treats the key as a miss and rebuilds the pair.Testing
Compiler.CorruptPCHAttributablepins the attribution contract — whole-file garbage and truncation must yield a setup failure or fatal error whose diagnostics blame the blob (path or deserialization category), never a completed parse.Compiler.StopCompilationnow also pins theCancelledstatus mapping.test_corrupt_pch_rebuilt_on_restart(parametrized: whole-file garbage → attributable failure healed inline; mid-file flip → may kill the worker, healed on the next request; flips that land in semantically dead bytes are consumed harmlessly and skip),test_corrupt_pch_idx_retracted(pair retracted and rebuilt, valid index blob back on disk),test_setup_fail_keeps_dirty(a non-result with no PCH to blame publishes an honest empty gap and recompiles on the next request instead of settling).Known residuals
middlevector exercises it on this LLVM build; the unit test deliberately excludes shapes that would abort the test process.Donepath the previous buffer's file index is retained rather than reset; it is unreachable while the dirty flag is set (all consumers gate on it), matching the existing dispatch-crash path.Summary by CodeRabbit
Bug Fixes
Tests