Repository navigation
feat(index): serve open-file header symbols from PCH preamble index - #501
Conversation
The PCH build already produced a full index of the preamble but the master dropped it. Persist it as a PreambleState blob paired with the PCH (CacheStore aux extension, .pch.idx): the preamble's symbol index, document links, inactive regions and open conditional stack live in one memory-mapped FlatBuffer, opened lazily and queried zero-copy. The pair commits as a single thread-pool job (never half-published), and the blob is verified off the event loop at commit time. Open sessions overlay these blobs onto index queries: header rows under the live buffer's context union with disk shards (per-location dedup), single-answer queries prefer the overlay, and the buffer's own preamble region (macros before the bound) resolves through the blob's main-file entry, gated on preamble sync so deferred PCH rebuilds never serve drifted coordinates. Overlay rows for files that are themselves open are excluded — their sessions are authoritative. This fixes go-to-definition/references into headers for open in-memory files whose TU the background indexer never sees, keeps results faithful to unsaved preamble edits (#ifdef branches no disk context activates), and preamble document links now survive restarts. Macros also get named symbol-table entries instead of the nameless default-constructed ones. cache_format_version bumps to 4.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a FlatBuffer-backed ChangesPreamble state and serialization
Paired cache and PCH build lifecycle
PCH overlay query resolution
Documentation and integration validation
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/service/feature_router.cpp (1)
40-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard preamble links against drift
src/server/service/feature_router.cpp:40-54—ast_dirtycan be false while the session’s preamble has moved on, sofind_preamble_links()can return stalelink.range/targetdata fordocument_links()and GoToDefinition. Use the same bound check aspreamble_in_sync()before serving these cached links.🤖 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/service/feature_router.cpp` around lines 40 - 54, find_preamble_links() can return links from a preamble that no longer matches the session. Add the same preamble-version/state bound check used by preamble_in_sync() before loading or returning cached links, and return nullptr when the session preamble has drifted; ensure document_links() and GoToDefinition only consume links passing this guard.
🧹 Nitpick comments (5)
src/index/tu_index.cpp (1)
84-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct unit test for macro symbol metadata.
Existing coverage (
test_preamble_macro_definitionintest_pch_overlay.py) exercises navigation but not the recordedname/kindthemselves. A smalltu_index-level orpreamble_state-level assertion onfind_symbol's returned name/kind for a macro symbol would directly protect this fix from regressing.🤖 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/tu_index.cpp` around lines 84 - 90, Add a focused unit test near the existing preamble macro coverage that calls find_symbol for a macro and asserts its recorded name equals the token spelling and its kind is SymbolKind::Macro; use the relevant tu_index or preamble_state test fixture so the metadata assignment in the symbol insertion path is directly validated.src/index/schema.fbs (1)
206-239: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider marking
main_fileas(required).serialize()always emits it, so this would align the schema with the writer and make malformed blobs fail verification earlier.tests/unit/index/preamble_state_tests.cpp::RejectVersionMismatchwould need amain_fileoffset.🤖 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/schema.fbs` around lines 206 - 239, Mark the PreambleState.main_file field as required to match serialize() always emitting it and enable earlier blob validation; update tests/unit/index/preamble_state_tests.cpp::RejectVersionMismatch to provide a main_file offset when constructing the fixture.src/server/protocol/worker.h (1)
106-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the field-usage reference comment for the new BuildPCH field.
index_output_pathis now a BuildPCH parameter but the selective-field summary at Lines 108-114 still readsBuildPCH: + content, preamble_bound, output_path. Worth addingindex_output_pathhere so this quick-reference stays accurate.🤖 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/protocol/worker.h` around lines 106 - 114, Update the selective-field summary comment for the unified task parameters to include index_output_path in the BuildPCH field list, alongside content, preamble_bound, and output_path.src/support/cache_store.cpp (1)
329-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider asserting the documented aux/primary suffix-collision invariant.
The scan classifies aux files by
ends_with(aux_ext)before primaries, which is correct for.pch/.pch.idx. However, the header documents "Must not be a suffix collision withextension" purely as a caller constraint — a future namespace whose extensions collide would silently misclassify primaries as aux (or delete real blobs as "orphan aux" at Lines 373-376). A cheapassertat registration would surface such a misconfiguration immediately rather than as silent data loss.🤖 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/support/cache_store.cpp` around lines 329 - 378, Validate the documented suffix-collision constraint when namespaces are registered, asserting that the configured aux extension cannot be a suffix of or otherwise collide with the primary extension. Locate the namespace registration/configuration logic and add a cheap debug assertion covering non-empty extensions before scanning can occur, so invalid configurations fail immediately rather than misclassifying or deleting blobs.src/server/service/query.cpp (1)
46-65: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftOverlay visitor re-runs once per open session sharing the same PCH.
visit_pch_overlaysiterates every open session and, for each one, resolvessession.pch_ref->keyand re-invokes the visitor against the (possibly identical)PreambleState. When several open buffers share one PCH (a common case — many TUs compiled with the same flags), every caller of this helper (find_symbol_info,query_relations,find_definition_location,collect_grouped_relations,collect_unique_targets,get_definition_text,collect_references) redundantly re-runsstate.lookup/state.find_symbolonce per session instead of once per unique blob. Correctness is preserved by the downstream dedup, but the cost scales with open-session count rather than unique-PCH count.Consider splitting "state-only" traversal (dedup by
pch_ref->key, invoke once) from the per-session main-file-region pass (which genuinely needs each session's ownline_map/ast_dirty/preamble_in_sync), so shared-PCH workspaces don't pay N× the lookup cost per query.🤖 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/service/query.cpp` around lines 46 - 65, Refactor IndexQuery::visit_pch_overlays so PCH state traversal is deduplicated by session.pch_ref->key and invokes the visitor once per unique PreambleState, while retaining a separate per-session pass for main-file-region logic requiring each session’s line_map, ast_dirty, or preamble_in_sync. Update callers such as find_symbol_info, query_relations, find_definition_location, collect_grouped_relations, collect_unique_targets, get_definition_text, and collect_references to use the appropriate traversal.
🤖 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/service/query.cpp`:
- Around line 783-811: The collect_references overlay handling only queries
header files and misses references from the preamble main-file region. Add a
state.lookup_main call alongside state.lookup, following the query_relations
pattern, and apply the same should-serve, position, deduplication, and result
construction logic used for overlay files.
- Around line 783-811: The overlay deduplication in the results collection loop
drops distinct references on the same line. Keep file-and-line keys for
pre-existing results, but use a finer-grained key including the overlay position
(such as pos->character or the relation byte offset) when tracking overlay-added
rows, so same-line distinct references are preserved while cross-source
duplicates remain filtered.
- Around line 660-690: Add a main-file lookup to the PCH overlay handling in
get_definition_text, mirroring find_definition_location and query_relations.
After or alongside the existing state.lookup call, invoke state.lookup_main for
RelationKind::Definition and apply the same file filtering, range validation,
line mapping, and DefinitionText construction so definitions in the preamble’s
main-file region are returned.
---
Outside diff comments:
In `@src/server/service/feature_router.cpp`:
- Around line 40-54: find_preamble_links() can return links from a preamble that
no longer matches the session. Add the same preamble-version/state bound check
used by preamble_in_sync() before loading or returning cached links, and return
nullptr when the session preamble has drifted; ensure document_links() and
GoToDefinition only consume links passing this guard.
---
Nitpick comments:
In `@src/index/schema.fbs`:
- Around line 206-239: Mark the PreambleState.main_file field as required to
match serialize() always emitting it and enable earlier blob validation; update
tests/unit/index/preamble_state_tests.cpp::RejectVersionMismatch to provide a
main_file offset when constructing the fixture.
In `@src/index/tu_index.cpp`:
- Around line 84-90: Add a focused unit test near the existing preamble macro
coverage that calls find_symbol for a macro and asserts its recorded name equals
the token spelling and its kind is SymbolKind::Macro; use the relevant tu_index
or preamble_state test fixture so the metadata assignment in the symbol
insertion path is directly validated.
In `@src/server/protocol/worker.h`:
- Around line 106-114: Update the selective-field summary comment for the
unified task parameters to include index_output_path in the BuildPCH field list,
alongside content, preamble_bound, and output_path.
In `@src/server/service/query.cpp`:
- Around line 46-65: Refactor IndexQuery::visit_pch_overlays so PCH state
traversal is deduplicated by session.pch_ref->key and invokes the visitor once
per unique PreambleState, while retaining a separate per-session pass for
main-file-region logic requiring each session’s line_map, ast_dirty, or
preamble_in_sync. Update callers such as find_symbol_info, query_relations,
find_definition_location, collect_grouped_relations, collect_unique_targets,
get_definition_text, and collect_references to use the appropriate traversal.
In `@src/support/cache_store.cpp`:
- Around line 329-378: Validate the documented suffix-collision constraint when
namespaces are registered, asserting that the configured aux extension cannot be
a suffix of or otherwise collide with the primary extension. Locate the
namespace registration/configuration logic and add a cheap debug assertion
covering non-empty extensions before scanning can occur, so invalid
configurations fail immediately rather than misclassifying or deleting blobs.
🪄 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: 90fe2bbc-9891-4604-8ae9-3db86683ae30
📒 Files selected for processing (30)
docs/en/design/incremental-parse.mddocs/en/design/symbol-index.mddocs/zh/design/incremental-parse.mddocs/zh/design/symbol-index.mdsrc/index/preamble_state.cppsrc/index/preamble_state.hsrc/index/schema.fbssrc/index/serialization.hsrc/index/tu_index.cppsrc/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/protocol/worker.hsrc/server/service/feature_router.cppsrc/server/service/query.cppsrc/server/service/query.hsrc/server/state/workspace.cppsrc/server/state/workspace.hsrc/server/transport/master_server.cppsrc/server/worker/stateless_worker.cppsrc/support/cache_store.cppsrc/support/cache_store.htests/integration/compilation/test_persistent_cache.pytests/integration/features/test_index.pytests/integration/features/test_pch_overlay.pytests/tools/checks.pytests/tools/workspace.pytests/unit/index/preamble_state_tests.cpptests/unit/server/pch_worker_tests.cpptests/unit/server/query_overlay_tests.cpptests/unit/support/cache_store_tests.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba8b75c78
ℹ️ 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".
Address review feedback: - DocumentLink now carries byte offsets like every other index datum; the LSP conversion happens once at the reply edge with the session's line map. The blob's LspRange struct is gone (links reuse Range), and directive-definition resolution compares offsets instead of juggling line/character pairs. preamble_format_version bumps to 2. - Drop the dead PCM index production in BuildPCM (nothing consumed it); the tu_index_data field stays as the Index result carrier. - Flatten the overlay query paths: one overlay_of() accessor replaces the repeated pch_cache-lookup-then-load nesting, two-part overlay visitors split into flat guard-style passes, and the dedup helper template unwinds back into plain functions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7ebb115a8
ℹ️ 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".
Review follow-ups on the PCH overlay's remaining query paths: - collect_references gains the overlay main-file pass (references in the buffer's preamble region, e.g. #if FOO) and its dedup keys become position-level, registered by every source — overlay repeats of a shard row still collapse, while distinct same-line references served only by the overlay are no longer dropped. - collect_unique_targets applies should_serve_overlay_file like every other overlay consumer: an open header's session is authoritative for its type relations, so another file's disk-snapshot overlay cannot resurface a stale base class. - should_serve_overlay_file also honors freshness clause 2: a header whose own disk content changed and awaits reindexing has its overlay rows suppressed, exactly like its shard contribution. - Test suites get a fresh instance per case, so the overlay suite's reset() was dead weight — replaced by a setup() that only fills the enable_indexing option skip_stale_contribution dereferences.
Two more review findings: - forward_document_links rechecks the session generation after the worker round-trip: a didChange landing mid-await leaves offsets that describe the pre-edit buffer, and the reply edge would map them onto the new text at wrong positions. preamble_in_sync moves onto Session and also gates the PCH link splice (find_preamble_links), closing the same mixed-view window for drifted preambles. - cache.json records the preamble_format_version each PCH pair was written with; a version change now drops the entry at load so the pair rebuilds on the next compile, instead of surfacing lazily on the first overlay query — which cannot trigger a rebuild and would leave overlay features dead until some edit recompiles the file.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/state/session.h (1)
140-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider caching
preamble_in_sync()result in callers that iterate overlays.
compute_preamble_bound(text)re-scans the buffer on every call. Downstream usage inquery.cppinvokespreamble_in_sync()inside avisit_pch_overlayslambda, which may call it once per overlay. Since the result is identical for a given session/text within a single query, the repeated scans are redundant.If the lambda is indeed invoked multiple times per query, consider hoisting the check out of the loop:
bool in_sync = !session.ast_dirty && session.preamble_in_sync(); visit_pch_overlays( [&](std::uint32_t id, const Session& session, const index::PreambleState& state) { if(!in_sync) return true; // ... });This is a minor performance concern — preamble scanning is typically O(preamble size) which is small — but worth verifying if queries are high-frequency.
🤖 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/state/session.h` around lines 140 - 148, The overlay query path should avoid recomputing preamble synchronization for every overlay. In the caller’s `visit_pch_overlays` flow, evaluate `session.preamble_in_sync()` once before entering the lambda, combined with the existing `ast_dirty` condition, and have the lambda reuse that cached result while preserving current behavior for out-of-sync sessions.
🤖 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/state/session.h`:
- Around line 140-148: The overlay query path should avoid recomputing preamble
synchronization for every overlay. In the caller’s `visit_pch_overlays` flow,
evaluate `session.preamble_in_sync()` once before entering the lambda, combined
with the existing `ast_dirty` condition, and have the lambda reuse that cached
result while preserving current behavior for out-of-sync sessions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 021b08f3-109a-4e47-90ed-136777ce39b7
📒 Files selected for processing (6)
src/index/preamble_state.hsrc/server/compiler/compiler.cppsrc/server/service/feature_router.cppsrc/server/service/query.cppsrc/server/state/session.hsrc/server/state/workspace.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- src/server/compiler/compiler.cpp
- src/index/preamble_state.h
- src/server/state/workspace.cpp
- src/server/service/feature_router.cpp
- src/server/service/query.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc3794279
ℹ️ 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".
Two more review findings: - Files with byte-identical preambles share one PCH (the key excludes the source path), but the blob's main-file entry carries file-local symbol identities — macro USRs embed the source path — so serving it to every sharer merged file-local preamble macros across files. Main entry lookups are now gated on the blob's own main path (the last entry of its path table), folded with the existing dirty/drift checks into one serves_main_entry rule. - Macro Definition relations now carry the macro's full extent, like declarations do: making macros discoverable by name left the agentic definition-text path returning nothing for them (target_symbol was 0, decoded as an empty range). get_definition_text also gains the overlay main-file pass so preamble macros resolve end to end.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec9c70af8f
ℹ️ 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".
Review feedback: the blob now stores the exact preamble text the PCH was built from (content + line starts on the preamble entry, like every other entry), and drift detection becomes a direct prefix comparison — serve preamble rows and links only while the buffer still starts with that text. This replaces the bound-equality proxy: Session::PCHRef loses its bound field and collapses into a plain pch_key string, Session::preamble_in_sync and its re-scan of the buffer go away. Naming and style cleanups from the same review: lookup_main becomes lookup_preamble (with main_path -> source_path, the schema field main_file -> preamble, serves_main_entry -> serves_preamble), and the links cache uses std::optional instead of unique_ptr. Blob format version bumps to 3.
Links are fetched at most once per documentLink request, so the lazy mutable cache bought nothing and cost a member plus lifetime rules. links() now returns the vector by value, and find_preamble_links follows suit — no more returning a pointer into shared state with a must-not-hold-across-await caveat.
Deep-review pass hunting carried-through assumptions: - visit_pch_overlays conflated two shapes: header/symbol passes ignored the session entirely (seven callbacks with unused parameters) and re-scanned one shared blob per sharing session, while preamble passes repeated the same gate by hand. Split into visit_overlays (distinct blobs, no session) and visit_preambles (gate folded in). - serves_preamble loses the dirty-flag condition: the prefix comparison validates the exact region the rows describe, which the dirty flag only approximates — body edits never move preamble rows, so preamble navigation now keeps working while typing. The same reasoning removes the ast_dirty wrapper around the document-link splice. - The builtin/empty path filters guarded inputs that cannot exist: the include graph only ever contains real file paths (unknown fids fall back to the source path), so blob entries never carry them. - cache.json stamped preamble_format_version per PCH entry, but one binary writes them all — now one field for the whole file. - The worker treated the blob output path as optional; the pair is mandatory, so the guard only masked misuse (one old test relied on it and now provides the path like every real caller).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: debffc13c4
ℹ️ 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".
Two review findings: - The agentic path+line locator only consulted sessions and shards, so a header reachable only through the overlay resolved for go-to-definition but not for readSymbol by file and line. lookup_file scans a blob's entries for one path and the locator falls through to it when the disk index has nothing. - Blob entries whose FileID has no include edge (the predefines buffer, a synthesized prefix injected via -include) cannot be attributed to a real path — path_id() falls back to the source file, misfiling rows the design never serves. serialize now drops them instead of relying on query-side filters to catch the misattribution.
The path/line locator fallback came from a review suggestion, not a real consumer, and it broke the design boundary halfway: overlay-only symbols became locatable by file and line while staying invisible to the name locator (whose overlay enumeration was rejected for noise and cost). One rule instead: overlays serve hash-anchored answering — cursor hit to hash to locations — and every discovery-shaped input (by name, by path and line) is the disk index's job. The edge-less-entry skip in serialize stays; that one is write-side correctness.
The overlay exists for LSP clients, which own the in-memory buffer; agents read files from disk, so serving them buffer-state rows breaks the correspondence between reported positions and what they read. get_definition_text and collect_references (agentic-only) return to their disk+session shape, taking the overlay-vs-shard dedup machinery with them. Macro definition text still works for agents — the definition ranges live in the disk index now. Shared building blocks (find_definition_location, find_symbol_info) keep their overlay sources for the LSP flows; agentic requests reach them only with hashes minted from disk data.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea0a599b49
ℹ️ 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".
Pairs commit primary-first, so a legitimate aux is never older than its primary. An aux left behind by a removal that failed while the file was held open (see reset_aux_locked) would otherwise be re-attached to a fresh primary after restart, pairing a new PCH with stale PreambleState data that no validity check catches — the key hashes the same preamble text both blobs were built from.
Agents read files from disk: positions from unsaved buffers would not match what they read, so buffer state must not leak into agentic answers. IndexQuery gains an options knob (IndexQueryOptions::disk_only) that turns off the buffer sources in one place each — the session and overlay visitors return nothing, and the shard fold stops excluding open files (their sessions are not consulted, so their shards answer). The agentic transport queries through a second, disk-only instance. The agentic-only entry points (locate_symbols, collect_references, get_definition_text) drop their session passes outright — only the disk-only instance ever calls them — and agent_client's own session lookups (symbol search sweep, documentSymbols) go with them, along with the now-unused with_session helper.
Background indexing skipped open files on the LSP-centric theory that their sessions serve them — but the shard is the file's disk truth, which disk-only consumers need regardless of any live buffer: a file opened before its first index round otherwise has no shard at all, and agentic search/definition on it return nothing. LSP queries already skip open files' shards in favor of their sessions, so indexing them changes nothing on that side. The Indexer loses its session-store dependency entirely. agentic/status now reports the current (or last) round's progress numbers instead of the live queue, which is compacted between rounds and would read as nothing ever having been indexed.
Indexing open files' disk snapshots only pays off for agents (the LSP side never reads an open file's shard — its session serves it), so the skip returns, now gated: the first agentic index query flips Indexer::index_open_files (sticky) and enqueues the currently open files to catch up. Skipping loses no debt — BufferClosed already re-checks a closed file's shard against the disk and re-enqueues, which also covers save-while-open edits and files opened before their first index round.
Every agentic fixture setup cost a uniform ~3.1s: the first agentic query triggers the open-file catch-up round, which sits behind the default 3s indexing idle timer, and the readiness poll only checked once a second on top. Configure a 10ms idle timeout and a 100ms poll in the two fixtures — the agentic suite drops from ~2min to ~25s and the whole integration run is back to ~40s.
setLastAccessAndModificationTime only overloads on int file descriptors; the native handle from openNativeFileForWrite is a HANDLE on Windows and broke both Windows builds.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f07df4bb14
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d8076b93a
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2b9c41cd3
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02e2e7c4f8
ℹ️ 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".
## Problem With a preamble PCH attached, `TUIndex::build` crashed the stateful worker (SIGSEGV in RelWithDebInfo, assert in Debug) on `didOpen` of many real-world files — 100% reproducible on e.g. `llvm/ADT/StringRef.cpp` and `BasicBlockUtils.h` (bughunt F18, Critical). Root cause: FileIDs loaded from the PCH (negative internal IDs) entered `file_indices` through two AST paths reachable from main-file top-level decls — 1. **Inherited function default arguments**: an out-of-line method definition inherits the default argument expression from the in-class declaration in a preamble header (`int find(int x = npos)` pattern). RecursiveASTVisitor traverses these without an inherited-guard (unlike default *template* arguments). 2. **Forward declarations + shared DefinitionData**: `class X;` in the main file reaches the definition's base specifiers, whose source ranges live in the preamble header. Meanwhile `IncludeGraph::file_table` derived its fid universe from replayed preprocessor directives — which never fire for preamble includes when a PCH is attached. `include_location_id` then dereferenced `end()`. Combined with the worker restart budget this killed all semantic features for the session (F19). ## Fix (three layers) - **Emission gate**: `Builder::file_index(fid)` is now the single write path into `file_indices`; interested-only builds drop rows outside the interested file. Zero information loss: the master only consumes `main_file_index` + `symbols` from such builds, and the preamble region is served by the PCH-side index (#501). Mirrors clangd's `FID == SM.getMainFileID()` filtering. - **Fact-driven graph**: `IncludeGraph::from(unit, indexed_fids)` is built after traversal, from the directive-seen includes (preserving `locations`' role as the TU dependency set) united with the fids the index actually recorded. Loaded fids resolve their include chains through the SourceManager, which preserves include locations across PCH boundaries. `CompilationUnitRef::files()` (and its FIXME) is deleted; the directive iteration moved into `from()`. - **Defense**: `include_location_id` returns a logged `-1` sentinel for unknown fids instead of asserting — the indexer must never crash the worker on user code. This also hardens the `PreambleState` serializer, which shares the lookup. Also documents two adjacent gaps found during the investigation: macro definitions in synthetic buffers (`<built-in>`, `<command line>`) are dropped with no go-to-definition feedback (FIXME in directive.cpp / include_graph.h), and background indexing compiles without the project's own PCH — slower indexing on PCH-built projects like LLVM (TODO in command.cpp). Note: an earlier revision of this branch also made `deps()` include `#embed`ed files; that change was split out into a follow-up PR so this one stays a pure crash fix (the last two commits add and then remove it). ## Testing - 5 new unit tests: the two emission paths (`PreambleDefaultArgument`, `PreambleBaseSpecifier`, each also asserting the full build resolves the loaded fid to the header's path), the macro path of the gate (`HeaderMacroDropped`), the defensive fallback (`UnknownFidFallback`), and loaded-fid chain recovery (`PreambleFidResolved`). The three crash-regression cases were each individually validated to abort the pre-fix code at the old assert. - Unit 893 (RelWithDebInfo + Debug/asserts), integration 270, smoke 3/3 — all green. - End-to-end repros (minimal fixture + `BasicBlockUtils.h` against the llvm CDB) flip from 100% CRASH to NOCRASH, with the defensive fallback confirmed never firing.
## Worker pool refactor Replaces the worker pool's ad-hoc bookkeeping with an explicit design: - **Slot state machine** (`Alive → Dying → Respawning`, terminal `Retired`/`Dead`) with a per-slot generation bumped on every death; all occupancy counts are derived from slot state instead of hand-maintained counters. - **No in-pool request retry.** A transport failure marks the slot dead immediately (plus a safety SIGKILL) and surfaces the new `dispatch_errc::worker_crashed`; callers own the policy — the compiler resends idempotent builds once, the indexer requeues the file (bounded at 3 attempts, so a poison file cannot drain the pool's crash budget; `worker_unavailable` is never requeued to avoid a dead-pool spin). - **Real memory preemption.** Under severe pressure the pool kills low-priority workers (immediately reclaiming their RSS), floors the low allowance, and respawns without crash accounting; the sender observes `cancelled`. Master-side accounting no longer diverges from worker reality. - **Crash budget with recovery.** Consecutive crash streak, reset after 30s healthy uptime, exponential respawn backoff (immediate/500ms/1s/... capped); spawn failures count into the streak and retry. Replaces the lifetime `max_restarts=2` that permanently killed slots and could be burned in milliseconds. - **Peer lifetime via `shared_ptr`** (senders hold their own reference), removing the unbounded `retired_peers` retention. - **Controllers split** into `tick_memory(ratio)` / `tick_scaling(ratio)` (unit-testable with injected ratios); scale-up grows `low_limit` by exactly one instead of resetting pressure/backoff state. - Stateful side: least-loaded assignment can no longer pin a document to a dead worker; crash-window sends fail fast with `worker_crashed`. Fixes found during the investigation and covered by new tests: retry-after-failure could land on the same dead worker (the `exclude` parameter was never forwarded), cancelled index requests were silently dropped, `scale_up` wiped AIMD state, dispatch was not triggered when a death freed a busy claim. ## Staleness fix (pre-existing flake) `test_staleness.py` flaked on main (2/4 local runs on RelWithDebInfo): `DepsSnapshot`'s fast path compared dep mtimes against a wall-clock `build_at`, so an edit made after the system clock stepped backwards (NTP sync, VM resume) got an "older" mtime and was silently skipped — stale ASTs/diagnostics until an unrelated invalidation. Now each dep records its nanosecond mtime at capture and is compared by equality, falling through to the content-hash check on any mismatch (the file tracker's existing scheme). Old `cache.json` files load unchanged and re-validate by hash once. 10 consecutive local runs of the suite pass. Note: the index-side staleness gate (`merged_index.cpp`, `mtime <= build_at`) has the same wall-clock hazard; fixing it needs an index schema change and is left as a follow-up. ## Testing - Unit: 884 pass (Debug + RelWithDebInfo); new coverage for backoff schedule, crash budget boundaries, healthy-uptime reset, preemption accounting, FIFO fairness, generation guards, transport-error classification, clock-rollback staleness. - Integration: 263 pass on both configs, including new `test_crash_during_indexing` (SIGKILL a stateless worker mid-round, assert full index convergence). - Smoke: 3/3 on both configs. ## Follow-up (2026-07-13) - Rebased onto current main (#501 preamble index, #505 TUIndex fid fix, #506 embed deps). The one conflict (`workspace.cpp` cache load/save) resolved in favor of #501's `.pch.idx` fields plus this branch's per-dep mtime scheme; legacy `cache.json` compatibility re-verified against the JSON codec (unknown fields ignored, absent `mtime` re-validates by hash once). - Addressed both review comments: - `preempt_low_priority` now cancels the preempt source **before** killing the worker. Investigation note: the old order was not an observable bug (kotatsu's `event.set()` only queues waiters and there is no suspension point between the calls), so this is a defensive ordering, documented as such. - The index requeue cap now counts only `worker_crashed`; a memory-pressure cancellation requeues without spending the poison-file budget. The policy moved into `Indexer::note_dispatch_failure` with a friend fixture and three new unit tests pinning it. - Cleanup: refreshed comments that still described the removed `DepsSnapshot::build_at`, and the pool class doc's off-by-one crash-budget wording. - Updated totals: unit 927 (Debug + RelWithDebInfo), integration 271, smoke 3/3 — all green locally. ## Document quarantine (F19) — 2026-07-14/15 The pool's per-slot crash budget cannot contain a poison *document*: the budget lives on slots, the poison lives in content, and one file could burn slot after slot until the whole pool was dead (bughunt F19). This branch adds content-side crash containment, hardened over ten codex review rounds (24 findings — 23 fixed, 1 documented acceptance) and consolidated into a single-writer architecture whose invariants are stated in `state/quarantine.h`: - **`Quarantine`**: per-document state machine with private fields — every transition is a method. Two ledger families with distinct disproof rules: the **compile streak** (cleared only by a successful compile of the same content, via a `Flight` token that pins "a landing clears exactly the evidence it inherited at takeoff") and **per-kind ledgers** (queries by `QueryKind`, stateless builds by `BuildKind`, document links) cleared only when *that kind* answers — a hover cannot launder semantic-tokens evidence, a compile cannot launder format strikes. Evidence is counted **per worker killed** (the retry's death is a second strike) and deduplicated by the dead incarnation's identity carried in `Error::data`, so one process death failing several in-flight requests is blamed at most once per document. - **Recovery licensing**: a real content change arms one probe; only the dispatch that can disprove the evidence (`recovery_compile` / `recovery_kind`) may spend it, concurrent racers see the kind as plainly blocked, and `ProbeGuard` hands the license back if the attempt provably never dispatched (cancellation unwind, capacity window) while a recorded strike keeps it spent. While quarantined, everything except the recovery dispatch is refused. - **Visibility**: `publish_quarantined` is the single materialization point for the quarantine diagnostic (announce-once per spell, including quarantines reached via completion/PCH paths that never run a compile); `publish_recovered` clears it when a stateless/query recovery lifts the quarantine without a compile. - **`CrashBudget`**: content-keyed budget for shared artifacts (PCH/PCM) that document quarantine cannot reach — every dependent stops re-triggering a build that keeps killing workers. Full lifecycle: blocks are five-minute cooldowns rather than verdicts (the poison may live in a header the key cannot see), a successful build clears the key's strikes, and the PCM budget key embeds the module's content hash so fixing the module unlocks immediately. - **Pool-side isolation** (`Suspect`): probes run `Isolated` on document-free workers (single-worker pools excepted) and never spend slot budget; recovery queries run `InPlace` — owner routing, budget-exempt, avoided by new-document assignment while in flight, with the counter unwound by RAII across cancellations. Dead slots revive after a cooldown and are preferred by scale-up; the scale-up ceiling counts every live process including retiring ones; the configured `min_stateless` floor survives above the startup count. The pool's responsibility contract (mechanism vs. policy, the `dispatch_errc` taxonomy, `worker_restarting` for blameless restart windows) is documented on the class. - Indexer debt hardening: failures attribute by launch ticket (stale crashes never spend fresh content's budget), giving up clears the pending slot even past a mid-flight deps-only downgrade, `worker_unavailable` requeues while slot revival is pending, and a failed content pass keeps its `ContentChanged` debt. - One documented acceptance: the same-generation eviction ABA (a live worker's stale-copy eviction draining after the path was assigned back to it) costs one spurious recompile in a one-notification-drain window; fixing it needs an ownership epoch in the worker protocol. Verified end to end by the F19 stress scenario (healthy files answer throughout, quarantined streak freezes, fixed file recovers via probe within ms, worker count never reaches zero), poison / poison-preamble integration tests, and 30+ type-level state-machine unit tests (several validated by reverting the fix and watching them fail). - Final totals: unit 976 (Debug + RelWithDebInfo), integration 279 (both configs), smoke 3/3 — all green locally and on CI (17 checks).
Problem
For open (in-memory) files, everything before the preamble bound is compiled into the PCH and invisible to the per-edit index: the headers' contents, the preamble region's own directives, and — crucially — the compilation context the buffer's preamble defines, which may match no disk TU the background indexer has ever seen. Header-internal data cannot be recovered from the consuming compile either: enumerating the TU's decls triggers full PCH deserialization, and referenced decls only bring in their declarations, never the references inside other decls' bodies.
Concrete symptoms:
#definebefore an#includeactivating an#ifdefbranch no disk context ever indexed).#definein the preamble region resolves to nothing.The PCH build actually produced a full index of the preamble all along (
handle_build_pch→serialize_tu_index), but the master dropped it on the floor.Design
PreambleState blob, paired with the PCH. The stateless worker serializes the preamble's state at build time — the only moment the freshly parsed preamble AST is in memory, so no PCH deserialization is ever needed:
PCHStatefields: document links, inactive regions, the open conditional stack.The blob is written directly to a store tmp path (no more multi-MB IPC payloads) — strictly after the PCH itself is flushed, so the pair's on-disk mtimes match its logical order — then opened by the master as a memory-mapped FlatBuffer and queried zero-copy, mirroring
MergedIndex's read path.CacheStore paired blobs. A namespace may declare an
aux_extension(pch→.pch.idx): one key owns two files forming a single entry — sized, aged and evicted together. The pair is only served complete: eviction removes aux before primary, a republished primary resets the stale aux, a missing half is a plain cache miss that rebuilds both, and restart adoption drops an aux older than its primary (removal residue). Both commits run as one thread-pool job, so a cancellation can never publish half a pair; the blob is also verified there, keeping the flatbuffer verification walk off the event loop.Query overlay (LSP). Open sessions overlay their PCH blobs onto index queries:
is_path_openbehavior; the Workspace invariant that an open file affects only itself is preserved).Agentic queries serve disk truth. Agents read files from disk, so buffer-derived state would hand them coordinates for text they cannot see. A plain
IndexQueryOptions{.disk_only}on the query layer gives the agent endpoint its ownIndexQuerythat skips sessions, overlays and preamble entries entirely — same code paths, one option, no parallel type hierarchy. Complementing that:index_open_filesand enqueues a catch-up for every open session, choosingContentChangedvsDepsOnlyby comparing disk content against the existing shard — a stale shard must not answer agents with pre-save rows while it waits.PCM is deliberately untouched: module units are ordinary disk files with CDB entries and belong to the normal background-indexing path (TODO left in
handle_build_pcm).Also fixed along the way
preamble_linkswere never written to cache.json — preamble document links now survive restarts via the blob (regression test included), andDocumentLinkcarries offsets internally with LSP conversion only at the reply edge.serialize_tu_indexcleared it) — kept now, enabling preamble-region cursor resolution.build()'s reference-files loop default-constructed them);handleMacroOccurrencenow records name/kind, and macro definition relations carry the full#defineextent.-includefiles are unaffected by the synthetic-buffer filter (clang records their include edge in the predefines buffer) — proven by a regression test.cache_format_version3 → 4 (tests/tools/workspace.pysynced).Testing
#ifdefbranch only the live preamble activates); no duplicate rows when shard and overlay coexist; preamble#definenavigation; links surviving restart with a verified PCH cache hit; deleted.pch.idxrebuilding the pair; on-disk header edits refreshing a same-key overlay; the agentic suite exercising disk-truth answers and the open-file catch-up.Design docs (
symbol-index.md,incremental-parse.md, en + zh) updated; the "PCH-induced index split" known limitation is now marked resolved.