Repository navigation
refactor: introduce Workspace/Session state model and clarify component responsibilities - #406
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughRefactors server state into a project-scoped Workspace (persistent caches, compile graph, merged indices) and per-file Sessions (in-memory text, compile state). Compiler and Indexer become session-aware facades operating on Workspace + Sessions; MasterServer now owns Sessions and manages LSP lifecycle and feature routing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (LSP)
participant Master as MasterServer
participant WS as Workspace
participant Sess as Session
participant Comp as Compiler
participant Index as Indexer
participant Worker as WorkerPool
Client->>Master: didOpen / request
Master->>Sess: create/update Session (text, version, generation)
Master->>Index: query or enqueue (pass Session*)
Master->>Comp: ensure_compiled / forward_query (pass Session&)
Comp->>WS: consult/store PCH/PCM state, compile_graph, pcm_paths
Comp->>Worker: forward build/query (stateless/stateful)
Worker-->>Comp: build result (async)
Comp-->>Sess: update file_index / pch_ref / ast_deps, clear ast_dirty
Comp->>WS: save_cache() / on_indexing_needed()
Master->>Client: publish diagnostics / responses
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 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: 6
🤖 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/compiler.cpp`:
- Around line 726-739: The current PCH cache key only uses preamble bytes
(preamble_hash) so different compile contexts can incorrectly reuse a .pch;
include the full compile context (e.g. current CDB entry id or switch_context
identifier, compiler flags/define macros, include search paths and
target/triple/target options) into the cache key/hash used when building
pch_path and when comparing against workspace.pch_cache entries; update the hash
computation (instead of only preamble_hash) and the stored st.hash check in
workspace.pch_cache, and ensure deps_changed(workspace.path_pool, st.deps) still
runs but combined with the new context-aware hash to prevent cross-context reuse
of PCHs.
- Around line 1285-1294: The include-completion branch drops the live session
context before invoking complete_include, causing search paths to be recomputed
from disk-only context; update the call so complete_include receives the active
session context (or ensure it reads from the existing session) rather than
relying on globals/disk state—e.g. propagate the session (or its relevant
context object) into complete_include(pctx, path) (and adjust the function
signature/usage accordingly) so
detect_completion_context/PositionMapper/mapper.to_offset continue to use the
live session when computing include completions.
- Around line 891-903: Compiler::on_file_saved currently only updates
compile_graph and PCM caches but leaves workspace.path_to_module stale; update
it when a file is saved by removing or refreshing module-path entries for the
affected IDs returned by workspace.compile_graph->update(path_id) (the same IDs
you erase from workspace.pcm_paths and workspace.pcm_cache) or by re-running
build_module_map() for the affected workspace to ensure fill_pcm_deps() and
complete_import() see the new module names; specifically modify
Compiler::on_file_saved to update workspace.path_to_module for each id in result
(or call build_module_map()/a targeted refresh) so module lookups reflect
add/rename/remove changes without restarting.
- Around line 963-964: When marking a session dirty (session.generation++ and
session.ast_dirty = true) you must also invalidate the session.file_index so
index-based handlers don't continue to read stale mapping/range data; update the
didChange path to call the same invalidation helper used by other dirtying paths
(or add a small helper if missing) to clear or reset session.file_index whenever
session.ast_dirty is set, ensuring functions that read session.file_index
(definition/reference/hierarchy handlers) will see the invalidated state and
trigger a recompile.
- Around line 1101-1102: The detached compile lambda stores a raw Session* from
sessions[path_id] and dereferences it across co_await points, risking
use-after-free; change the routine to either (A) capture a stable
std::shared_ptr<Session> (e.g. convert the sessions container lookup to obtain a
shared_ptr and store that in the lambda and in
Session::PendingCompile/session.compiling) or (B) re-lookup the session from
sessions by path_id immediately after each co_await and abort the task if the
session key is missing (checking didClose/didOpen effects), and ensure
session.compiling is cleared only when holding a valid session instance; update
references to sessions[path_id], Session::PendingCompile, and session.compiling
accordingly so no raw Session* is dereferenced after suspension.
In `@src/server/master_server.cpp`:
- Line 38: The lambda passed to indexer currently calls sessions.contains(id)
with a project-index ID, but sessions is keyed by workspace.path_pool IDs;
update the lambda used in indexer(...) so it first translates the project-index
ID to a path_pool ID (via the workspace.project_index -> path_pool translation
helper, e.g. workspace.project_index.to_path_id(id) or the equivalent
translation function) and then calls sessions.contains(translatedId); ensure
this change also addresses checks in Indexer::is_file_open to use the translated
path_pool ID when looking up sessions.
🪄 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: d8458d48-1762-43de-ade7-f5e4125a9ec7
📒 Files selected for processing (8)
src/server/compiler.cppsrc/server/compiler.hsrc/server/indexer.cppsrc/server/indexer.hsrc/server/master_server.cppsrc/server/master_server.hsrc/server/session.hsrc/server/workspace.h
Introduce two new data structures that clearly separate project-wide persistent state from per-open-file volatile state: - **Workspace**: owns all disk-derived shared state (dep_graph, compile_graph, path_to_module, pch_cache, pcm_cache, project_index, merged_indices, cdb, config, path_pool). Updated only at startup and on didSave. Has on_file_saved()/on_file_closed() methods. - **Session**: owns all per-open-file state (text, version, generation, ast_dirty, pch_ref, ast_deps, header_context, active_context, file_index). Includes path_id for self-identification. Created on didOpen, destroyed on didClose. Compiler and Indexer are stateless service objects holding references to Workspace, sessions map, and WorkerPool. They own no data. Document lifecycle (open/change/close/save) is handled directly by MasterServer on the sessions map — Compiler no longer has these methods. Header context switching is done by directly setting Session fields. All 465 unit tests pass. Full binary builds successfully. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
force-pushed
the
refactor/workspace-session
branch
from
April 7, 2026 18:13
35ae74e to
ff05b15
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/server/master_server.cpp (2)
419-420:⚠️ Potential issue | 🟠 MajorUse one invalidation helper for every dirtying path.
These branches only set
ast_dirty, butensure_compiled()rejects stale in-flight results viageneration, and index-based handlers still readsession.file_indexwithout checkingast_dirty. A save/context change can therefore accept an outdated compile result and keep old mapper/ranges live. Bumpgenerationand resetfile_indexwhenever you invalidate a session.Also applies to: 466-467, 476-477, 889-893
🤖 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 419 - 420, The invalidation branches only set session.ast_dirty but must also bump session.generation and reset session.file_index so ensure_compiled() can reject stale in-flight results and handlers don't read old indexes; update every place that sets session.ast_dirty (e.g., the blocks that currently do session.ast_dirty = true and the other similar occurrences flagged) to also increment session.generation (session.generation++) and clear session.file_index (reset to empty/invalid state) whenever you invalidate a session so compilation/results and index-based handlers behave consistently with ensure_compiled().
39-42:⚠️ Potential issue | 🟠 MajorTranslate project-index IDs before checking
sessions.
Indexercalls this callback with project-index path IDs, butsessionsis keyed byworkspace.path_poolIDs.sessions.contains(id)will miss active documents and let stale merged shards participate in open-file definition/reference queries.🤖 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 39 - 42, The indexer callback passed in MasterServer's constructor is receiving project-index path IDs but is directly calling sessions.contains(id); instead translate the incoming project-index ID to the workspace/path-pool key before checking sessions. Update the lambda used to construct indexer (the callback in indexer(..., [this](uint32_t id) { ... })) to map the project-index id via workspace.path_pool (or the workspace path-id translation helper) into the workspace.path_pool ID and then call sessions.contains(translatedId) so active documents are correctly detected.src/server/compiler.cpp (3)
1157-1161:⚠️ Potential issue | 🟠 MajorPass the live session into include completion.
This branch drops
session, andcomplete_include()then recomputes compile args with disk-only context. For headers without their own CDB entry, or afterclice/switchContext, include completion will use the wrong search paths.Minimal propagation fix
- co_return complete_include(pctx, path); + co_return complete_include(pctx, path, &session);-et::serde::RawValue Compiler::complete_include(const PreambleCompletionContext& ctx, - llvm::StringRef path) { +et::serde::RawValue Compiler::complete_include(const PreambleCompletionContext& ctx, + llvm::StringRef path, + Session* session) { std::string directory; std::vector<std::string> arguments; - if(!fill_compile_args(path, directory, arguments)) + if(!fill_compile_args(path, directory, arguments, session)) return serde_raw{"[]"};Also applies to: 1213-1218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 1157 - 1161, The include-completion branch currently drops the live session and calls complete_include(pctx, path), causing recomputation of compile args from disk only; change the call to pass the live Session (e.g., complete_include(session, pctx, path) or the existing session reference) so complete_include uses the session's in-memory compile args/context; update both occurrences that handle CompletionContext::IncludeQuoted/IncludeAngled (the detect_completion_context -> complete_include call near the block with pctx and the similar block around lines 1213-1218) and adjust complete_include's signature accordingly to accept and use the session.
734-746:⚠️ Potential issue | 🟠 MajorInclude compile context in the PCH cache key.
preamble_hashalone lets the same.pchbe reused across different CDB entries,switchContexthosts, defines, include paths, or target options. That can feed later compiles with a PCH built for a different command line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 734 - 746, The PCH cache currently keys only on preamble_hash (variable preamble_hash) which allows reuse across different compilation contexts; update the cache key generation (used for pch_path and workspace.pch_cache lookups) to include the compile context fields (e.g., switchContext/host indicator, defines/macros, include search paths, target options/flags and any command-line relevant settings) so the hash combines preamble_hash with a stable representation of those context fields before formatting pch_path and before comparing against st.hash in workspace.pch_cache; ensure deps_changed(workspace.path_pool, st.deps) still runs but only after confirming the combined context+content hash matches the cache entry.
938-943:⚠️ Potential issue | 🔴 Critical
Sessionaccess is still unsafe across suspension points.The outer coroutine keeps a
Session&, and the detached task still readssessafterco_awaitand afterfinish_compile()signals waiters. SincedidCloseerasessessions[path_id], reads likesession.ast_dirtyandsess->versioncan still hit freed storage. Re-lookup bypath_idbefore every resumed access, or switch the map to stable ownership.Also applies to: 959-982, 1007-1018, 1054-1067
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 938 - 943, The coroutine holds a Session& across suspension points (e.g., the loop around session.compiling and co_await pending->done.wait()), which is unsafe because sessions[path_id] can be erased (didClose) and free the referenced storage; update the code to re-lookup the session from the sessions map by path_id before every access after any co_await (or convert the sessions map to hold stable ownership such as shared_ptr and use a local shared_ptr copy), i.e., replace uses of the stale Session& (symbols: Session, session.compiling, pending->done.wait(), session.ast_dirty, sess->version, finish_compile(), didClose, sessions[path_id]) with a safe lookup/check pattern (find sessions[path_id], ensure non-null/existing) or hold a shared_ptr to the session for the duration of the suspended section before reading fields.src/server/workspace.cpp (1)
71-81:⚠️ Potential issue | 🟠 MajorRefresh the rest of the disk-derived
Workspacestate on save.
didSaveis the only Session → Workspace handoff, but this handler only updatescompile_graphand PCM caches.workspace.dep_graphandworkspace.path_to_modulestay at their startup snapshot, so saved include/module changes are invisible to header-context resolution, PCM dependency filling, and import completion until restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.cpp` around lines 71 - 81, on_file_saved currently only updates compile_graph and PCM caches leaving disk-derived Workspace state stale; update the Workspace::on_file_saved implementation to also refresh and invalidate entries in dep_graph and path_to_module for the saved path_id: read the new file/includes for path_id (or call the existing utility that computes includes/modules if available), remove or update any dep_graph edges and dependent nodes that reference path_id, and remove any path_to_module mappings pointing to the old snapshot so module resolution sees the new content; ensure these invalidations happen before/alongside compile_graph->update and also add dirtied ids produced by dep_graph invalidation to the returned vector so callers know which items changed.
🤖 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 175-177: The loop currently skips any server_path_id present in
sessions, which prevents handling saved files; change the skip logic so you only
continue when the session exists AND the file was not saved (i.e., check didSave
or the saved-file set). Concretely, update the condition around
sessions.contains(server_path_id) to also consult didSave (or the equivalent
saved-file indicator) so saved entries still get queued/merged into
ProjectIndex/merged shard even while a session is open; apply the same change to
the other occurrence around lines 463-470.
In `@src/server/workspace.cpp`:
- Around line 13-25: The binary search in lookup_occurrence is wrong because it
calls std::ranges::lower_bound projecting o.range.end while occurrences are
sorted by (begin, end, target); update lookup_occurrence to use a
correctness-first approach: either perform a linear scan over occs to find all
occurrences whose range.contains(offset) and pick the smallest span, or
build/use a separate cache (sorted by range.begin) and do a lower_bound on
range.begin then scan neighbors for matches; ensure you remove the projection of
o.range.end and reference the index::Occurrence vector (occs) and the
lookup_occurrence function in your change so the search respects the actual sort
order.
---
Duplicate comments:
In `@src/server/compiler.cpp`:
- Around line 1157-1161: The include-completion branch currently drops the live
session and calls complete_include(pctx, path), causing recomputation of compile
args from disk only; change the call to pass the live Session (e.g.,
complete_include(session, pctx, path) or the existing session reference) so
complete_include uses the session's in-memory compile args/context; update both
occurrences that handle CompletionContext::IncludeQuoted/IncludeAngled (the
detect_completion_context -> complete_include call near the block with pctx and
the similar block around lines 1213-1218) and adjust complete_include's
signature accordingly to accept and use the session.
- Around line 734-746: The PCH cache currently keys only on preamble_hash
(variable preamble_hash) which allows reuse across different compilation
contexts; update the cache key generation (used for pch_path and
workspace.pch_cache lookups) to include the compile context fields (e.g.,
switchContext/host indicator, defines/macros, include search paths, target
options/flags and any command-line relevant settings) so the hash combines
preamble_hash with a stable representation of those context fields before
formatting pch_path and before comparing against st.hash in workspace.pch_cache;
ensure deps_changed(workspace.path_pool, st.deps) still runs but only after
confirming the combined context+content hash matches the cache entry.
- Around line 938-943: The coroutine holds a Session& across suspension points
(e.g., the loop around session.compiling and co_await pending->done.wait()),
which is unsafe because sessions[path_id] can be erased (didClose) and free the
referenced storage; update the code to re-lookup the session from the sessions
map by path_id before every access after any co_await (or convert the sessions
map to hold stable ownership such as shared_ptr and use a local shared_ptr
copy), i.e., replace uses of the stale Session& (symbols: Session,
session.compiling, pending->done.wait(), session.ast_dirty, sess->version,
finish_compile(), didClose, sessions[path_id]) with a safe lookup/check pattern
(find sessions[path_id], ensure non-null/existing) or hold a shared_ptr to the
session for the duration of the suspended section before reading fields.
In `@src/server/master_server.cpp`:
- Around line 419-420: The invalidation branches only set session.ast_dirty but
must also bump session.generation and reset session.file_index so
ensure_compiled() can reject stale in-flight results and handlers don't read old
indexes; update every place that sets session.ast_dirty (e.g., the blocks that
currently do session.ast_dirty = true and the other similar occurrences flagged)
to also increment session.generation (session.generation++) and clear
session.file_index (reset to empty/invalid state) whenever you invalidate a
session so compilation/results and index-based handlers behave consistently with
ensure_compiled().
- Around line 39-42: The indexer callback passed in MasterServer's constructor
is receiving project-index path IDs but is directly calling
sessions.contains(id); instead translate the incoming project-index ID to the
workspace/path-pool key before checking sessions. Update the lambda used to
construct indexer (the callback in indexer(..., [this](uint32_t id) { ... })) to
map the project-index id via workspace.path_pool (or the workspace path-id
translation helper) into the workspace.path_pool ID and then call
sessions.contains(translatedId) so active documents are correctly detected.
In `@src/server/workspace.cpp`:
- Around line 71-81: on_file_saved currently only updates compile_graph and PCM
caches leaving disk-derived Workspace state stale; update the
Workspace::on_file_saved implementation to also refresh and invalidate entries
in dep_graph and path_to_module for the saved path_id: read the new
file/includes for path_id (or call the existing utility that computes
includes/modules if available), remove or update any dep_graph edges and
dependent nodes that reference path_id, and remove any path_to_module mappings
pointing to the old snapshot so module resolution sees the new content; ensure
these invalidations happen before/alongside compile_graph->update and also add
dirtied ids produced by dep_graph invalidation to the returned vector so callers
know which items changed.
🪄 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: 18afbdaf-de9e-4f0c-b3da-5de7b00314de
📒 Files selected for processing (9)
src/server/compiler.cppsrc/server/compiler.hsrc/server/indexer.cppsrc/server/indexer.hsrc/server/master_server.cppsrc/server/master_server.hsrc/server/session.hsrc/server/workspace.cppsrc/server/workspace.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/session.h
- src/server/master_server.h
- src/server/workspace.h
Move methods from Compiler to their natural owners: - Workspace: load_cache, save_cache, cleanup_cache, build_module_map, fill_pcm_deps, cancel_all - Free functions: uri_to_path, hash_file, capture_deps_snapshot, deps_changed - File-scope static: detect_completion_context Compiler now only contains genuine compilation logic (ensure_compiled, ensure_pch, ensure_deps, forward_query/build, header context resolution, include/import completion). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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/master_server.cpp (1)
806-833:⚠️ Potential issue | 🟠 Major
switchContextloses which compile command the user actually selected.Lines 806-833 can emit multiple
ContextItems (config#i``) for the same file, but they all serialize the same URI. Lines 870-893 reduce the selection back to a singlecontext_path_id, and `fill_header_context_args()` later just uses `lookup(...).front()`. In multi-entry CDBs, choosing config `#2/`#3 still resolves to the first compile command.Also applies to: 870-893
🤖 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 806 - 833, The switchContext flow loses which compile command was chosen because multiple ContextItem entries for the same file all carry the same URI; when resolving context_path_id the code always picks lookup(...).front(). Fix by encoding the compile-command index into the ContextItem and reading it back: when building items in the loop that creates ext::ContextItem (variable entry and all_items), attach the index i (e.g., in the item's uri as a fragment/query or in an available metadata field) so each ContextItem is uniquely identified; then update the code paths that reduce the selection to a single context_path_id and fill_header_context_args() to parse that index from the selected ContextItem and call workspace.cdb.lookup(path)[index] (instead of .front()), ensuring the chosen compile command (index) is preserved through switchContext and used by fill_header_context_args().
♻️ Duplicate comments (6)
src/server/master_server.cpp (2)
419-420:⚠️ Potential issue | 🟠 MajorReset
session.file_indexwhenever a session becomes dirty.These paths set
ast_dirtybut keep the previous in-memory index and mapper alive. The index-first handlers later in this file can still answer against stale ranges until a recompile overwrites them. A smallmark_session_dirty()helper would make this harder to miss on future paths too.Also applies to: 465-476, 889-893
🤖 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 419 - 420, When marking a session dirty (currently done by incrementing session.generation and setting session.ast_dirty = true), also reset the in-memory index/mapper so index-first handlers cannot serve stale ranges; implement a small helper mark_session_dirty(Session &session) that does session.generation++, session.ast_dirty = true, and clears session.file_index (and any associated mapper/state tied to it), and replace the existing scattered two-line updates with calls to mark_session_dirty() (these occur around the blocks using session.generation/session.ast_dirty).
175-177:⚠️ Potential issue | 🟠 MajorKeep saved files eligible for background indexing even while the session stays open.
Lines 175-177 skip every open file, and Lines 463-469 only enqueue dirty files when no session exists. After
didSave, the disk file is the workspace source of truth, so its merged shard/project index stays stale until close. Queue saved path_ids regardless, and only skip still-open files that have not been saved.Also applies to: 463-469
src/server/compiler.cpp (2)
918-922:⚠️ Potential issue | 🟠 MajorPass the live session into include completion.
Lines 918-922 detect include completion from the unsaved buffer, but
complete_include()on Lines 931-936 callsfill_compile_args()without aSession*. Headers without their own CDB entry, or files afterswitchContext, will complete against disk-only search paths instead of the active session context.Also applies to: 931-936
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 918 - 922, detect_completion_context is called with the unsaved buffer (session.text) but complete_include(...) is invoked without the live Session*, causing fill_compile_args() to use disk-only search paths; update the flow to thread the current Session* through include completion: change complete_include(...) to accept a Session* (or Session&), pass the existing session into the call site where detect_completion_context is used, and update complete_include to forward that Session* into fill_compile_args(...) (and any other helpers it calls, e.g., the code paths used after switchContext) so include completions use the active session's compile args instead of filesystem-only paths.
138-155:⚠️ Potential issue | 🟠 MajorValidate cached PCM/PCH against the current compile context.
The reuse checks on these lines only look at artifact deps.
pcm_pathis already derived from the current arguments, but it is never compared, and the PCH branch still keys only on preamble bytes. Changing defines, target options, or switched header context can silently reuse an incompatible cache entry.Also applies to: 495-507
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 138 - 155, The cached PCM/PCH validity check only verifies artifact deps and doesn't ensure the cached file matches the current compile context (arguments/derived pcm_path or preamble/compile options), so update the cache validation in the pcm lookup (workspace.pcm_cache usage around pcm_path/pcm_filename/path_id) to also compare the computed pcm_path (or pcm_filename/args_hash) and relevant compile-context identifiers (bp.arguments, target/options/preamble bytes) against the stored cache entry; if they differ, treat it as invalid and skip reuse, and apply the same check to the PCH branch which currently keys only on preamble bytes so it also verifies the computed path/arguments and target defines before returning a cached entry.src/server/workspace.cpp (2)
22-35:⚠️ Potential issue | 🟠 MajorBinary search doesn't match the
occurrencessort order.Line 25 lower-bounds on
o.range.end, butFileIndex::occurrencesis ordered by(begin, end, target). That probe can skip the containing occurrence and return the wrong symbol under the cursor. A correctness-first scan is safer here unless you keep a second index sorted for this lookup.Possible safe fallback
const static index::Occurrence* lookup_occurrence(const std::vector<index::Occurrence>& occs, std::uint32_t offset) { - auto it = std::ranges::lower_bound(occs, offset, {}, [](const index::Occurrence& o) { - return o.range.end; - }); const index::Occurrence* best = nullptr; - while(it != occs.end() && it->range.contains(offset)) { - if(!best || (it->range.end - it->range.begin) < (best->range.end - best->range.begin)) { - best = &*it; - } - ++it; + for(const auto& occ: occs) { + if(!occ.range.contains(offset)) + continue; + if(!best || (occ.range.end - occ.range.begin) < (best->range.end - best->range.begin)) { + best = &occ; + } } return best; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.cpp` around lines 22 - 35, The binary-search probe in lookup_occurrence is incorrect because it lower_bounds on o.range.end while occurrences are sorted by (begin, end, target), which can skip matching occurrences; replace the current lower_bound + forward loop with a correctness-first linear scan over the entire occs vector (iterate all index::Occurrence in occs), check o.range.contains(offset) and track the tightest (smallest end-begin) occurrence, and return that pointer (or nullptr) — keep the function name lookup_occurrence and the parameter occs to locate and modify the code.
81-91:⚠️ Potential issue | 🟠 MajorRefresh the disk-derived workspace graphs on
didSave.
on_file_saved()only dirtiescompile_graphand drops PCM cache entries.dep_graph, its reverse map, andpath_to_modulestay stale, so host resolution, import completion, and PCM lookup keep using the pre-save topology until restart. If you rebuildpath_to_modulehere, make sure stale IDs are cleared as well instead of only inserting new ones.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.cpp` around lines 81 - 91, on_file_saved currently only updates compile_graph and clears pcm_paths/pcm_cache, leaving dep_graph, its reverse mapping, and path_to_module stale; update Workspace::on_file_saved to also refresh dep_graph and the reverse map for the saved path (e.g., call dep_graph->update or equivalent and iterate its returned/affected IDs), then rebuild or update path_to_module entries for those affected IDs while removing any stale mappings (ensure you remove old IDs from path_to_module and from the reverse map before inserting new ones) and keep pcm_paths/pcm_cache clearing behavior for the same affected IDs; use the existing symbols compile_graph, dep_graph, path_to_module, reverse map, pcm_paths, and pcm_cache to locate and modify the logic.
🤖 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/compiler.cpp`:
- Around line 602-605: When workspace.pcm_paths lacks pid and
workspace.compile_graph->has_unit(pid) is true, the code calls compile_deps(pid)
which explicitly skips building pid; instead call the compile path that builds
the unit itself. Replace or augment the branch that currently calls
workspace.compile_graph->compile_deps(pid) so it invokes
workspace.compile_graph->compile(pid) (or the equivalent self-build method)
after checking has_unit(pid), ensuring the PCM for pid is produced and then
await that result before continuing.
In `@src/server/compiler.h`:
- Around line 76-93: The coroutine APIs (ensure_compiled, forward_query,
forward_build, handle_completion and the related ensure_pch/ensure_deps helpers)
must not take a borrowed Session& because they suspend and the referenced
Session may be destroyed; change their signatures to accept a stable identifier
(e.g. path_id or session_id) or a shared/stable handle and re-lookup or acquire
the Session after any suspension point, or alternatively store Session instances
behind shared_ptr/unique stable ownership and take/hold that ownership across
awaits; update ensure_compiled(Session&), forward_query(worker::QueryKind,
Session&), forward_query(worker::QueryKind, const protocol::Position&,
Session&), forward_build(worker::BuildKind, const protocol::Position&,
Session&), and handle_completion(const protocol::Position&, Session&)
accordingly so no coroutine parameter is a raw reference to Session.
In `@src/server/master_server.cpp`:
- Around line 472-478: Currently only sessions whose
header_context->host_path_id equals path_id are cleared, which misses
intermediate headers referenced in the preamble chain; fix by either (A) adding
a membership test on HeaderFileContext (e.g., implement
HeaderFileContext::IncludesPath(path_id) that checks the stored host→header
chain or preamble_path) and change the loop to clear when
header_context->IncludesPath(path_id) (and set session.ast_dirty = true), or (B)
conservatively clear all cached header_contexts on save (remove/reset
session.header_context for any session with a non-null header_context and set
session.ast_dirty = true) to ensure no stale preambles are reused.
---
Outside diff comments:
In `@src/server/master_server.cpp`:
- Around line 806-833: The switchContext flow loses which compile command was
chosen because multiple ContextItem entries for the same file all carry the same
URI; when resolving context_path_id the code always picks lookup(...).front().
Fix by encoding the compile-command index into the ContextItem and reading it
back: when building items in the loop that creates ext::ContextItem (variable
entry and all_items), attach the index i (e.g., in the item's uri as a
fragment/query or in an available metadata field) so each ContextItem is
uniquely identified; then update the code paths that reduce the selection to a
single context_path_id and fill_header_context_args() to parse that index from
the selected ContextItem and call workspace.cdb.lookup(path)[index] (instead of
.front()), ensuring the chosen compile command (index) is preserved through
switchContext and used by fill_header_context_args().
---
Duplicate comments:
In `@src/server/compiler.cpp`:
- Around line 918-922: detect_completion_context is called with the unsaved
buffer (session.text) but complete_include(...) is invoked without the live
Session*, causing fill_compile_args() to use disk-only search paths; update the
flow to thread the current Session* through include completion: change
complete_include(...) to accept a Session* (or Session&), pass the existing
session into the call site where detect_completion_context is used, and update
complete_include to forward that Session* into fill_compile_args(...) (and any
other helpers it calls, e.g., the code paths used after switchContext) so
include completions use the active session's compile args instead of
filesystem-only paths.
- Around line 138-155: The cached PCM/PCH validity check only verifies artifact
deps and doesn't ensure the cached file matches the current compile context
(arguments/derived pcm_path or preamble/compile options), so update the cache
validation in the pcm lookup (workspace.pcm_cache usage around
pcm_path/pcm_filename/path_id) to also compare the computed pcm_path (or
pcm_filename/args_hash) and relevant compile-context identifiers (bp.arguments,
target/options/preamble bytes) against the stored cache entry; if they differ,
treat it as invalid and skip reuse, and apply the same check to the PCH branch
which currently keys only on preamble bytes so it also verifies the computed
path/arguments and target defines before returning a cached entry.
In `@src/server/master_server.cpp`:
- Around line 419-420: When marking a session dirty (currently done by
incrementing session.generation and setting session.ast_dirty = true), also
reset the in-memory index/mapper so index-first handlers cannot serve stale
ranges; implement a small helper mark_session_dirty(Session &session) that does
session.generation++, session.ast_dirty = true, and clears session.file_index
(and any associated mapper/state tied to it), and replace the existing scattered
two-line updates with calls to mark_session_dirty() (these occur around the
blocks using session.generation/session.ast_dirty).
In `@src/server/workspace.cpp`:
- Around line 22-35: The binary-search probe in lookup_occurrence is incorrect
because it lower_bounds on o.range.end while occurrences are sorted by (begin,
end, target), which can skip matching occurrences; replace the current
lower_bound + forward loop with a correctness-first linear scan over the entire
occs vector (iterate all index::Occurrence in occs), check
o.range.contains(offset) and track the tightest (smallest end-begin) occurrence,
and return that pointer (or nullptr) — keep the function name lookup_occurrence
and the parameter occs to locate and modify the code.
- Around line 81-91: on_file_saved currently only updates compile_graph and
clears pcm_paths/pcm_cache, leaving dep_graph, its reverse mapping, and
path_to_module stale; update Workspace::on_file_saved to also refresh dep_graph
and the reverse map for the saved path (e.g., call dep_graph->update or
equivalent and iterate its returned/affected IDs), then rebuild or update
path_to_module entries for those affected IDs while removing any stale mappings
(ensure you remove old IDs from path_to_module and from the reverse map before
inserting new ones) and keep pcm_paths/pcm_cache clearing behavior for the same
affected IDs; use the existing symbols compile_graph, dep_graph, path_to_module,
reverse map, pcm_paths, and pcm_cache to locate and modify the logic.
🪄 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: 3c0fb33e-4a22-4620-ad36-55a40c62fca6
📒 Files selected for processing (5)
src/server/compiler.cppsrc/server/compiler.hsrc/server/master_server.cppsrc/server/workspace.cppsrc/server/workspace.h
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
These were debug scripts from the chained PCH investigation, not part of the workspace/session refactor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix potential dangling pointer in ensure_compiled: cache version before finish_compile() which may invalidate sess pointer - Fix misleading pch_cache comment (not content-addressed yet, add TODO) - Remove unused includes from compiler.cpp (chrono, type_traits, variant, raw_ostream, Chrono.h) - Use try_emplace for Session creation in didOpen - Keep is_file_open callback in Indexer (server path_id vs project path_id are different pools, callback bridges the gap) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dexer Indexer now owns the entire background indexing lifecycle: - index_queue, scheduling state, idle timer - enqueue() to add files, schedule() to trigger - run_background_indexing() coroutine MasterServer no longer has any indexing-related state or methods. Compiler.on_indexing_needed callback now calls indexer.schedule(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Move preamble completion detection, module import completion, and include path completion into standalone functions in syntax/completion.h. Compiler now calls these functions and wraps results in LSP types. Add 16 unit tests for detect_completion_context and complete_module_import. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
16bit-ykiko
changed the title
"Open files are never depended upon by other files. Dependencies always point to disk files." — the core invariant of the two-layer Workspace/Session architecture. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1. (Critical) Cache session fields before co_await in forward_query/ forward_build to prevent dangling reference if didClose erases the session during suspension. 2. (Major) Include compile arguments and directory in PCH cache hash so switchContext produces a different PCH instead of reusing one built with different flags. 3. (Major) Rescan module declarations on didSave and update path_to_module, so module renames are reflected without restart. 4. (Major) Bridge project-level path_id to server-level path_id in the is_file_open callback, since ProjectIndex and Workspace use separate PathPool instances with potentially different IDs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Clarify responsibilities and non-responsibilities for each component to reflect the final state of the refactoring. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, clear_diagnostics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
src/server/workspace.cpp (2)
22-35:⚠️ Potential issue | 🟠 MajorThis binary search does not match the occurrence sort order.
FileIndex::occurrencesare not ordered byrange.end, solower_boundhere can miss the symbol under the cursor or pick the wrong nested occurrence. Use a correctness-first scan here, or probe on a cache sorted for this lookup.Proposed fix
const static index::Occurrence* lookup_occurrence(const std::vector<index::Occurrence>& occs, std::uint32_t offset) { - auto it = std::ranges::lower_bound(occs, offset, {}, [](const index::Occurrence& o) { - return o.range.end; - }); const index::Occurrence* best = nullptr; - while(it != occs.end() && it->range.contains(offset)) { - if(!best || (it->range.end - it->range.begin) < (best->range.end - best->range.begin)) { - best = &*it; + for(const auto& occ: occs) { + if(!occ.range.contains(offset)) + continue; + if(!best || (occ.range.end - occ.range.begin) < (best->range.end - best->range.begin)) { + best = &occ; } - ++it; } return best; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.cpp` around lines 22 - 35, lookup_occurrence uses std::ranges::lower_bound assuming occs is sorted by range.end, but FileIndex::occurrences isn't, so the binary search can miss or mis-pick nested occurrences; replace the binary-search approach with a correctness-first linear scan over occs to find the innermost occurrence containing offset (iterate all entries in the vector and pick the one with range.contains(offset) and smallest (range.end - range.begin)), or alternatively call into a cache that is explicitly sorted by range.end before probing; update the lookup_occurrence function (and any callers if needed) to use this linear scan or the sorted cache to ensure correct results for nested occurrences.
75-85:⚠️ Potential issue | 🟠 MajorRefresh
path_to_moduleon save, and clear it before rebuilding.
on_file_saved()drops PCM cache entries but never refreshes the module-name map, andbuild_module_map()only inserts. After a module interface is added, renamed, or removed, import completion and PCM resolution can keep serving stale names until restart.Also applies to: 337-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.cpp` around lines 75 - 85, Workspace::on_file_saved currently erases pcm_paths and pcm_cache for updated path ids but never updates the path_to_module map, leaving stale module name mappings; modify Workspace::on_file_saved to also remove any path_to_module entries for the affected ids (or clear path_to_module entirely) before invoking build_module_map so module-name mappings are rebuilt; ensure the same change is applied to the other location that clears PCM caches (the similar pcm_paths/pcm_cache-clearing block around the second occurrence) so path_to_module is refreshed there as well.src/server/compiler.cpp (4)
448-463:⚠️ Potential issue | 🟠 MajorThe PCH cache key still ignores compile context.
This cache path/hash only depends on preamble bytes. After
switchContextor a compile-command flag change, the same buffer can reuse a.pchbuilt with different defines, include paths, or target options. Fold the effective compile arguments/context into the cache key and validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 448 - 463, The cache key currently only uses preamble_hash (computed into pch_path and compared via workspace.pch_cache entry) and thus can incorrectly reuse PCHs across different compile contexts; compute a deterministic hash of the effective compile context (includes flags, defines, include paths, target options produced by switchContext/compile-command) and fold that into the cache key and filename alongside preamble_hash, store it on the cached entry (e.g., add st.context_hash) and include it in Session::PCHRef, and update the reuse check to require st.context_hash == current_context_hash in addition to st.hash == preamble_hash and !deps_changed(...). Ensure deps_changed validation still runs against the same stored deps.
875-878:⚠️ Potential issue | 🟠 MajorPass the live session into include completion arg resolution.
Without
&session, headers without their own CDB entry andclice/switchContextfall back to disk-only context, so include completion can use the wrong search path set.Proposed fix
- if(!fill_compile_args(path, directory, arguments)) + if(!fill_compile_args(path, directory, arguments, &session)) co_return serde_raw{"[]"};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 875 - 878, The call to fill_compile_args(path, directory, arguments) should be updated to pass the live session so include-completion resolves with the in-memory context; change the invocation in the include-completion flow to call fill_compile_args(path, directory, arguments, &session) (or the appropriate session reference parameter that fill_compile_args accepts) so headers without a CDB entry and clice/switchContext use the session-aware search paths instead of falling back to disk-only context.
434-525:⚠️ Potential issue | 🔴 Critical
Session&is still crossing suspension points unsafely.The detached lambda now re-looks up
Session, but these helpers still keep references intosessionsacross their ownco_awaits.ensure_deps()readssession.textafter awaiting module builds,ensure_pch()writessession.pch_refafter awaiting the worker, andensure_compiled()readssession.ast_dirtyagain afterpending_compile->done.wait(). A concurrentdidCloseorDenseMaprehash can still turn those references into dangling ones. Re-lookup bypath_idafter each suspension or moveSessionto stable ownership.Also applies to: 531-582, 632-783
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 434 - 525, ensure_pch holds a reference to Session (Session& session) across co_await suspension points which can become dangling; after any co_await (e.g., after waiting on it->second.building->wait() and after co_await pool.send_stateless(bp)) re-resolve the session by path_id (or obtain a stable shared/owned Session pointer up-front) before reading or writing session members (e.g., session.pch_ref, session.text), and guard the assignment with a null/existence check so you never access the original Session& after suspension; update references in ensure_pch (and similarly in ensure_deps() and ensure_compiled()) to use the re-lookup or stable ownership approach.
552-558:⚠️ Potential issue | 🟠 MajorBuild the imported module itself here, not only its dependencies.
compile_deps(pid)skipspid, so a newly typed unsavedimport foo;can still leaveworkspace.pcm_paths[pid]empty and the compile runs without the required PCM. This branch needs the path that emitspiditself, not just its prerequisites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compiler.cpp` around lines 552 - 558, The loop over workspace.path_to_module currently only calls workspace.compile_graph->compile_deps(pid), which omits building the unit pid itself and leaves workspace.pcm_paths[pid] unset; change this branch to ensure the module unit is emitted too by invoking the compile method that builds the unit (e.g., call a function like workspace.compile_graph->compile_unit(pid) or invoke compile or compile_deps_then_build sequence) so that after the await the PCM path for pid is produced and stored in workspace.pcm_paths; update the block around workspace.compile_graph and compile_deps(pid) to call the unit-building API for pid (or call compile_deps(pid) followed by compile_unit(pid) if no single API exists).src/server/master_server.cpp (3)
349-360:⚠️ Potential issue | 🟠 MajorInvalidate the open-file index when the buffer becomes dirty.
Indexer::resolve_cursor()preferssession.file_indexand does not consultast_dirty, so this path can keep answering from the previous revision until the next compile finishes. Reset the cached open-file index here, and reuse the same invalidation helper on save/context-switch paths.Proposed fix
session.generation++; session.ast_dirty = true; + session.file_index.reset();🤖 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 - 360, When marking a session dirty (where session.generation++ and session.ast_dirty = true), also invalidate the cached open-file index so Indexer::resolve_cursor() does not keep using the old session.file_index; call the same invalidation helper you use on save/context-switch paths (or explicitly reset/clear session.file_index) in the same block where you set session.ast_dirty so the index is discarded until rebuilt.
402-408:⚠️ Potential issue | 🟠 MajorClearing only direct hosts leaves stale header preambles behind.
resolve_header_context()builds the cached preamble from the full host→header chain, but this loop only clears entries whosehost_path_idmatches the saved file. Saving an intermediate header can therefore reuse a stalepreamble_pathon the next compile. Track chain membership or conservatively clear all cachedheader_contexts here.🤖 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 402 - 408, The loop that only resets session.header_context when header_context->host_path_id == path_id leaves stale preambles because resolve_header_context() depends on the full host→header chain; update the invalidation to conservatively clear all cached header_contexts (or track full chain membership) so no session can reuse a stale preamble: iterate over sessions and for each session with a non-null header_context call session.header_context.reset() and set session.ast_dirty = true (use the same sessions/session/header_context/ast_dirty symbols and ensure resolve_header_context() consumers will rebuild the preamble).
40-46:⚠️ Potential issue | 🟠 MajorTranslate project-index IDs before probing
sessions.
Indexer::is_proj_path_open()is called withworkspace.project_indexpath IDs, but this lambda checks theworkspace.path_pool-keyedsessionsmap directly. Open files can still fall through to stale merged shards and produce duplicate or stale relation results.🤖 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 40 - 46, The lambda passed into Indexer (currently [this](uint32_t id) { return sessions.contains(id); }) is checking sessions with project-index IDs instead of the workspace.path_pool keys; change it to translate the incoming project-index ID to the workspace.path_pool key before probing sessions (e.g., uint32_t pathId = workspace.project_index_to_path_id(id) or the equivalent translation helper on workspace), then return sessions.contains(pathId), so that Indexer::is_proj_path_open() uses the correct path IDs and avoids stale/duplicate relation results.
🧹 Nitpick comments (2)
src/server/workspace.h (2)
229-230: Considerstd::optional<std::uint32_t>instead ofUINT32_MAXsentinel.Using a magic value for "no exclusion" works but
std::optionalwould be more expressive and type-safe.♻️ Suggested signature
- void fill_pcm_deps(std::unordered_map<std::string, std::string>& pcms, - std::uint32_t exclude_path_id = UINT32_MAX) const; + void fill_pcm_deps(std::unordered_map<std::string, std::string>& pcms, + std::optional<std::uint32_t> exclude_path_id = std::nullopt) const;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.h` around lines 229 - 230, Replace the sentinel-based parameter in fill_pcm_deps with an explicit std::optional: change the declaration of void fill_pcm_deps(std::unordered_map<std::string, std::string>& pcms, std::uint32_t exclude_path_id = UINT32_MAX) const; to accept std::optional<std::uint32_t> exclude_path_id = std::nullopt (and include <optional>), update the corresponding implementation to test exclude_path_id.has_value() instead of comparing to UINT32_MAX, and update all call sites of fill_pcm_deps to pass either std::nullopt for "no exclusion" or the numeric value (e.g., std::make_optional(id)); ensure headers are adjusted where needed.
40-44: Consider using a struct-of-arrays invariant comment.The parallel
path_idsandhashesvectors must remain in sync (same size, corresponding indices). This is implicit but could benefit from an assertion in capture/modification code to catch misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/workspace.h` around lines 40 - 44, Add an invariant comment inside struct DepsSnapshot noting that path_ids and hashes must remain parallel (same size, corresponding indices), and implement an assertion to enforce it: add a member validation helper (e.g., DepsSnapshot::validate()) that checks path_ids.size() == hashes.size() and use llvm::assert/always_assert-like checks; call validate() after any capture, construction, or modification operations that push/erase/assign to path_ids or hashes so mismatches are caught early. Ensure validate() is referenced from all places that build or mutate DepsSnapshot instances.
🤖 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 809-813: The code currently flips sit->second.ast_dirty but does
not update the compile generation, which allows in-flight compiles started under
the old context (see ensure_compiled()) to publish stale results; update
switchContext (where sit->second.active_context/header_context/pch_ref/ast_deps
are reset) to also bump the session/compile generation used by ensure_compiled()
(e.g., increment the Session::generation or a dedicated compile_revision on
sit->second) whenever non-text compile inputs change so any in-flight compile
will be considered stale and discarded.
---
Duplicate comments:
In `@src/server/compiler.cpp`:
- Around line 448-463: The cache key currently only uses preamble_hash (computed
into pch_path and compared via workspace.pch_cache entry) and thus can
incorrectly reuse PCHs across different compile contexts; compute a
deterministic hash of the effective compile context (includes flags, defines,
include paths, target options produced by switchContext/compile-command) and
fold that into the cache key and filename alongside preamble_hash, store it on
the cached entry (e.g., add st.context_hash) and include it in Session::PCHRef,
and update the reuse check to require st.context_hash == current_context_hash in
addition to st.hash == preamble_hash and !deps_changed(...). Ensure deps_changed
validation still runs against the same stored deps.
- Around line 875-878: The call to fill_compile_args(path, directory, arguments)
should be updated to pass the live session so include-completion resolves with
the in-memory context; change the invocation in the include-completion flow to
call fill_compile_args(path, directory, arguments, &session) (or the appropriate
session reference parameter that fill_compile_args accepts) so headers without a
CDB entry and clice/switchContext use the session-aware search paths instead of
falling back to disk-only context.
- Around line 434-525: ensure_pch holds a reference to Session (Session&
session) across co_await suspension points which can become dangling; after any
co_await (e.g., after waiting on it->second.building->wait() and after co_await
pool.send_stateless(bp)) re-resolve the session by path_id (or obtain a stable
shared/owned Session pointer up-front) before reading or writing session members
(e.g., session.pch_ref, session.text), and guard the assignment with a
null/existence check so you never access the original Session& after suspension;
update references in ensure_pch (and similarly in ensure_deps() and
ensure_compiled()) to use the re-lookup or stable ownership approach.
- Around line 552-558: The loop over workspace.path_to_module currently only
calls workspace.compile_graph->compile_deps(pid), which omits building the unit
pid itself and leaves workspace.pcm_paths[pid] unset; change this branch to
ensure the module unit is emitted too by invoking the compile method that builds
the unit (e.g., call a function like workspace.compile_graph->compile_unit(pid)
or invoke compile or compile_deps_then_build sequence) so that after the await
the PCM path for pid is produced and stored in workspace.pcm_paths; update the
block around workspace.compile_graph and compile_deps(pid) to call the
unit-building API for pid (or call compile_deps(pid) followed by
compile_unit(pid) if no single API exists).
In `@src/server/master_server.cpp`:
- Around line 349-360: When marking a session dirty (where session.generation++
and session.ast_dirty = true), also invalidate the cached open-file index so
Indexer::resolve_cursor() does not keep using the old session.file_index; call
the same invalidation helper you use on save/context-switch paths (or explicitly
reset/clear session.file_index) in the same block where you set
session.ast_dirty so the index is discarded until rebuilt.
- Around line 402-408: The loop that only resets session.header_context when
header_context->host_path_id == path_id leaves stale preambles because
resolve_header_context() depends on the full host→header chain; update the
invalidation to conservatively clear all cached header_contexts (or track full
chain membership) so no session can reuse a stale preamble: iterate over
sessions and for each session with a non-null header_context call
session.header_context.reset() and set session.ast_dirty = true (use the same
sessions/session/header_context/ast_dirty symbols and ensure
resolve_header_context() consumers will rebuild the preamble).
- Around line 40-46: The lambda passed into Indexer (currently [this](uint32_t
id) { return sessions.contains(id); }) is checking sessions with project-index
IDs instead of the workspace.path_pool keys; change it to translate the incoming
project-index ID to the workspace.path_pool key before probing sessions (e.g.,
uint32_t pathId = workspace.project_index_to_path_id(id) or the equivalent
translation helper on workspace), then return sessions.contains(pathId), so that
Indexer::is_proj_path_open() uses the correct path IDs and avoids
stale/duplicate relation results.
In `@src/server/workspace.cpp`:
- Around line 22-35: lookup_occurrence uses std::ranges::lower_bound assuming
occs is sorted by range.end, but FileIndex::occurrences isn't, so the binary
search can miss or mis-pick nested occurrences; replace the binary-search
approach with a correctness-first linear scan over occs to find the innermost
occurrence containing offset (iterate all entries in the vector and pick the one
with range.contains(offset) and smallest (range.end - range.begin)), or
alternatively call into a cache that is explicitly sorted by range.end before
probing; update the lookup_occurrence function (and any callers if needed) to
use this linear scan or the sorted cache to ensure correct results for nested
occurrences.
- Around line 75-85: Workspace::on_file_saved currently erases pcm_paths and
pcm_cache for updated path ids but never updates the path_to_module map, leaving
stale module name mappings; modify Workspace::on_file_saved to also remove any
path_to_module entries for the affected ids (or clear path_to_module entirely)
before invoking build_module_map so module-name mappings are rebuilt; ensure the
same change is applied to the other location that clears PCM caches (the similar
pcm_paths/pcm_cache-clearing block around the second occurrence) so
path_to_module is refreshed there as well.
---
Nitpick comments:
In `@src/server/workspace.h`:
- Around line 229-230: Replace the sentinel-based parameter in fill_pcm_deps
with an explicit std::optional: change the declaration of void
fill_pcm_deps(std::unordered_map<std::string, std::string>& pcms, std::uint32_t
exclude_path_id = UINT32_MAX) const; to accept std::optional<std::uint32_t>
exclude_path_id = std::nullopt (and include <optional>), update the
corresponding implementation to test exclude_path_id.has_value() instead of
comparing to UINT32_MAX, and update all call sites of fill_pcm_deps to pass
either std::nullopt for "no exclusion" or the numeric value (e.g.,
std::make_optional(id)); ensure headers are adjusted where needed.
- Around line 40-44: Add an invariant comment inside struct DepsSnapshot noting
that path_ids and hashes must remain parallel (same size, corresponding
indices), and implement an assertion to enforce it: add a member validation
helper (e.g., DepsSnapshot::validate()) that checks path_ids.size() ==
hashes.size() and use llvm::assert/always_assert-like checks; call validate()
after any capture, construction, or modification operations that
push/erase/assign to path_ids or hashes so mismatches are caught early. Ensure
validate() is referenced from all places that build or mutate DepsSnapshot
instances.
🪄 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: 538396a1-5298-4f3c-b672-f819fe668b16
📒 Files selected for processing (15)
src/server/compiler.cppsrc/server/compiler.hsrc/server/indexer.cppsrc/server/indexer.hsrc/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/session.hsrc/server/worker_pool.hsrc/server/workspace.cppsrc/server/workspace.hsrc/syntax/completion.cppsrc/syntax/completion.hsrc/syntax/dependency_graph.cpptests/unit/syntax/completion_tests.cpp
💤 Files with no reviewable changes (3)
- src/server/protocol.h
- src/server/worker_pool.h
- src/syntax/dependency_graph.cpp
✅ Files skipped from review due to trivial changes (1)
- src/server/session.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/master_server.h
Merge the two forward_query overloads into one with optional position and range parameters. Pass InlayHintParams.range through to the worker so it only computes hints for the visible region. Also remove LocalSourceRange user-defined constructors so it works as an aggregate with bincode serialization. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous change hashed compile arguments (including the source file path) into the preamble hash, causing files with identical preambles but different paths to generate separate PCH files. Revert to hashing only the preamble text, matching the behavior before this refactor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks done
2 tasks done
This was referenced Apr 22, 2026
3 tasks done
This was referenced Jun 29, 2026
This was referenced Jul 6, 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
Introduces a two-layer state model that cleanly separates disk-based project state from per-open-file editing state, and redistributes responsibilities across server components so each has a single, clear role.
New types
Workspace — all persistent, project-wide shared state:
Session — volatile per-open-file editing state:
Component responsibilities after refactor
What moved where
Into Workspace (from Compiler/MasterServer):
Into Session (from Compiler documents map):
Into Indexer (from MasterServer):
Into syntax/completion.h (from Compiler):
Inlined into MasterServer (from Compiler):
Deleted from Compiler (9 methods):
Tests
Diff stats
15 files changed, +1857, -1555
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests