fix: prevent worker crashes from null ASTConsumer, invalid FileID, and missing PCH cache dir - #435
Conversation
📝 WalkthroughWalkthroughAdds defensive checks for AST consumer creation and FileID handling, makes workspace loading synchronous, prevents PCH builds when the project cache directory is unset or cannot be created (clearing in-flight state and signaling waiters), and updates several codec/header includes across server and test files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server/master_server.cpp (1)
59-177:⚠️ Potential issue | 🟠 MajorSynchronous
load_workspace()now blocks the event loop duringinitialized.Previously
load_workspace()was akota::task<>dispatched vialoop.schedule(...), making workspace bootstrap a detached background operation (consistent with the pattern documented insrc/server/compiler.cpp:643-678and used insrc/server/indexer.cpp:634-637andsrc/server/worker_pool.cpp:100-112). After this change, theInitializedParamsnotification handler at line 334 calls it inline, so the entire bootstrap —create_directories,cleanup_cache/load_cache,compile_commands.jsonauto-scan viadirectory_iterator,workspace.cdb.load,scan_dependency_graph(which explicitly reportselapsed_ms),build_reverse_map,indexer.load, and the indexer enqueue loop over every CDB entry — runs on the event-loop thread before any other LSP message can be processed.For small projects this is fine and matches the PR's smoke-test observations, but on large monorepos this can stall the loop for many seconds, delay worker-pool responsiveness, and make
$/cancelRequest/shutdown/didOpenback up behind initialization. The PR rationale ("only synchronous filesystem operations") explains whyco_awaitis unnecessary, but doesn't address the blocking-on-loop concern that originally motivatedloop.schedule(...).Consider keeping the function
void(since it truly has no suspension points) but re-dispatching it as a detached task, e.g.:♻️ Suggested pattern
- load_workspace(); + loop.schedule([this]() -> kota::task<> { + load_workspace(); + co_return; + }());Or, if the scheduler supports plain callables, schedule it directly without the coroutine wrapper.
If the intent is specifically to run this before the worker pool starts handling requests (note:
pool.start(...)already happened earlier in the same handler), please add a short comment here documenting that trade-off so the deviation from the establishedloop.schedule()pattern isn't reverted later.Also applies to: 334-334
🤖 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 59 - 177, MasterServer::load_workspace is now invoked inline from the InitializedParams handler and performs long blocking work (create_directories, directory_iterator scan, workspace.cdb.load, scan_dependency_graph, indexer.enqueue, etc.), which blocks the event loop; change the call site so load_workspace runs as a detached background task instead of inline — e.g. dispatch a lambda that calls MasterServer::load_workspace via loop.schedule(...) or wrap it as kota::task<> and schedule it, leaving load_workspace as a void synchronous function; if the blocking behavior was intentional (run-before-workers), add a short comment at the InitializedParams handler noting the deliberate synchronous bootstrap and its trade-offs so future reviewers don't revert it.src/compile/compilation_unit.cpp (1)
83-114:⚠️ Potential issue | 🟡 MinorDownstream callers may silently propagate empty paths.
Returning an empty
StringReffor invalidFileIDfixes the SIGABRT, but several callers treatfile_path's result as always-valid. Since the method can return empty in two scenarios—invalidFileID(line 85) or whengetFileEntryRefForIDfails for a validFileID(line 92)—callers must guard accordingly:
src/index/include_graph.cpp:30—path_table.try_emplace(path, …)inserts unvalidated path as a key; an empty path becomes a corrupt graph entry.src/index/include_graph.cpp:54—graph.paths.emplace_back(unit.file_path(unit.interested_file()))inserts unvalidated path unconditionally.src/feature/diagnostics.cpp:57—to_uri(unit.file_path(raw.fid))produces a malformed URI in the LSP response if path is empty.src/feature/document_links.cpp:39,45— Guards onfid.isValid(), but this is insufficient; a validFileIDcan still yield an empty path if the file entry lookup fails.The
deps()method (lines 248–259) already guards with!path.empty(). Apply the same guard at these call sites or audit them to prevent corrupt downstream data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compile/compilation_unit.cpp` around lines 83 - 114, Several callers assume CompilationUnitRef::file_path(...) always returns a non-empty llvm::StringRef; since file_path can return an empty path for invalid FileID or missing FileEntry, update those call sites to guard against empty results (as deps() already does using !path.empty()). Specifically: in src/index/include_graph.cpp, check the returned path before calling path_table.try_emplace(...) and before graph.paths.emplace_back(unit.file_path(unit.interested_file())) and skip/handle when empty; in src/feature/diagnostics.cpp ensure to_uri(unit.file_path(raw.fid)) is only called when the path is non-empty; and in src/feature/document_links.cpp add the same empty-path guard in addition to the existing fid.isValid() checks — when empty, skip insertion or log/handle the missing-path case to avoid corrupt graph entries or malformed URIs.
🤖 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 493-500: The new unconditional create_directories call uses
workspace.config.project.cache_dir and can create a relative cache/pch in the
server CWD; restore the existing "empty-cache-dir" guard by checking if
workspace.config.project.cache_dir is empty (or otherwise indicates “do
nothing”) before computing pch_dir and calling
llvm::sys::fs::create_directories; if the cache_dir is empty, skip creating
pch_dir and avoid dispatching the PCH build (i.e., perform the same early exit
behavior you do elsewhere instead of proceeding to the create_directories,
preserving the existing resets on workspace.pch_cache[path_id].building,
completion->set(), and co_return false semantics where appropriate).
---
Outside diff comments:
In `@src/compile/compilation_unit.cpp`:
- Around line 83-114: Several callers assume CompilationUnitRef::file_path(...)
always returns a non-empty llvm::StringRef; since file_path can return an empty
path for invalid FileID or missing FileEntry, update those call sites to guard
against empty results (as deps() already does using !path.empty()).
Specifically: in src/index/include_graph.cpp, check the returned path before
calling path_table.try_emplace(...) and before
graph.paths.emplace_back(unit.file_path(unit.interested_file())) and skip/handle
when empty; in src/feature/diagnostics.cpp ensure
to_uri(unit.file_path(raw.fid)) is only called when the path is non-empty; and
in src/feature/document_links.cpp add the same empty-path guard in addition to
the existing fid.isValid() checks — when empty, skip insertion or log/handle the
missing-path case to avoid corrupt graph entries or malformed URIs.
In `@src/server/master_server.cpp`:
- Around line 59-177: MasterServer::load_workspace is now invoked inline from
the InitializedParams handler and performs long blocking work
(create_directories, directory_iterator scan, workspace.cdb.load,
scan_dependency_graph, indexer.enqueue, etc.), which blocks the event loop;
change the call site so load_workspace runs as a detached background task
instead of inline — e.g. dispatch a lambda that calls
MasterServer::load_workspace via loop.schedule(...) or wrap it as kota::task<>
and schedule it, leaving load_workspace as a void synchronous function; if the
blocking behavior was intentional (run-before-workers), add a short comment at
the InitializedParams handler noting the deliberate synchronous bootstrap and
its trade-offs so future reviewers don't revert it.
🪄 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: 05519fa7-f7a2-4bb3-b2ee-00acc27ff875
📒 Files selected for processing (5)
src/compile/compilation.cppsrc/compile/compilation_unit.cppsrc/server/compiler.cppsrc/server/master_server.cppsrc/server/master_server.h
…d missing PCH cache dir Three pre-existing bugs cause worker processes to crash (SEGV/SIGABRT), which becomes a reliability issue when combined with the respawn mechanism on low-core CI machines. - compilation.cpp: Check for null before passing WrapperFrontendAction's ASTConsumer to MultiplexConsumer (prevents SEGV in stateless workers during PCH builds when CreateASTConsumer fails) - compilation_unit.cpp: Return empty StringRef on invalid FileID instead of asserting (prevents SIGABRT in stateful workers when compilation produces an AST with no valid main file, e.g. synthesized default commands without system headers) - compiler.cpp: Create PCH cache directory before sending build request to stateless worker (prevents "No such file or directory" when load_workspace didn't run due to missing compile_commands.json - master_server: Make load_workspace a plain synchronous function since it contains no async operations EOF ) Made-with: Cursor
2dfdcf4 to
c9ac524
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/master_server.cpp (1)
59-177: Synchronous conversion is sound; be aware it still blocks the notification handler.The body has no suspension points, so converting to
voidis behaviorally equivalent to the previouskota::task<>(which was already executing synchronously end‑to‑end once resumed). That said,load_workspace()is now invoked inline from theinitializednotification callback (Line 334), so filesystem walks,workspace.cdb.load,scan_dependency_graph,build_module_map, andindexer.loadall run on the event-loop thread before the handler returns. For large workspaces this can delay subsequent LSP messages. If this proves noticeable, consider re-scheduling the heavy portion onto a worker/background task (keeping it non-coroutine is fine —loop.schedule([this]{ load_workspace(); })or similar).🤖 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 59 - 177, load_workspace() is now synchronous and runs expensive filesystem and indexing work inline on the event-loop (called from the initialized notification), which can block the notification handler; split the heavy work (the filesystem walks, workspace.cdb.load(...), scan_dependency_graph(...), workspace.build_module_map(), indexer.load(...)/enqueue/schedule, and compiler.init_compile_graph()) out of the inline callback and reschedule it onto a background worker or the event loop's scheduler (e.g. capture this in a lambda and call loop.schedule([this]{ /* heavy portion */ }); or dispatch to a thread pool) while keeping the lightweight early checks and config handling in load_workspace() so the notification handler returns quickly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/master_server.cpp`:
- Around line 59-177: load_workspace() is now synchronous and runs expensive
filesystem and indexing work inline on the event-loop (called from the
initialized notification), which can block the notification handler; split the
heavy work (the filesystem walks, workspace.cdb.load(...),
scan_dependency_graph(...), workspace.build_module_map(),
indexer.load(...)/enqueue/schedule, and compiler.init_compile_graph()) out of
the inline callback and reschedule it onto a background worker or the event
loop's scheduler (e.g. capture this in a lambda and call loop.schedule([this]{
/* heavy portion */ }); or dispatch to a thread pool) while keeping the
lightweight early checks and config handling in load_workspace() so the
notification handler returns quickly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcaf9e76-62c4-455e-b423-383b71c82a78
📒 Files selected for processing (5)
src/compile/compilation.cppsrc/compile/compilation_unit.cppsrc/server/compiler.cppsrc/server/master_server.cppsrc/server/master_server.h
✅ Files skipped from review due to trivial changes (2)
- src/compile/compilation_unit.cpp
- src/compile/compilation.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/server/compiler.cpp
kotatsu moved all codec headers into subdirectories; top-level files like codec/raw_value.h and codec/toml.h no longer exist. - codec/raw_value.h → removed (re-exported by json/json.h and bincode/bincode.h) - codec/toml.h → codec/toml/toml.h - codec/json/serializer.h → codec/json/json.h (use umbrella header) Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/master_server.h (1)
76-76: Avoid locking workspace loading into the event-loop path.
load_workspace()now has a synchronous API, but the implementation performs substantial filesystem/index work and is called directly from the initialized notification handler insrc/server/master_server.cpp. On large workspaces this can stall LSP message processing during startup; consider moving the heavy scan/load work onto a background worker or keeping an async boundary for yielding/cancellation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/master_server.h` at line 76, The synchronous load_workspace() call is doing heavy I/O on the event-loop path; change its usage so the expensive scan/index work runs off the main thread by converting load_workspace() to an asynchronous/background task or by introducing a worker thread/task queue and invoking it from the initialized notification handler in master_server.cpp; specifically, replace the direct call to MasterServer::load_workspace() in the initialized handler with a dispatch to a background worker (or make load_workspace_async() that returns a future/promised completion) and ensure any state mutations are synchronized or marshalled back to the event loop when ready so startup messages are not blocked and cancellation/yielding is possible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/server/master_server.h`:
- Line 76: The synchronous load_workspace() call is doing heavy I/O on the
event-loop path; change its usage so the expensive scan/index work runs off the
main thread by converting load_workspace() to an asynchronous/background task or
by introducing a worker thread/task queue and invoking it from the initialized
notification handler in master_server.cpp; specifically, replace the direct call
to MasterServer::load_workspace() in the initialized handler with a dispatch to
a background worker (or make load_workspace_async() that returns a
future/promised completion) and ensure any state mutations are synchronized or
marshalled back to the event loop when ready so startup messages are not blocked
and cancellation/yielding is possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08ea113c-064a-4545-9a2e-bf6699b8cec2
📒 Files selected for processing (9)
benchmarks/scan_benchmark.cppsrc/server/compiler.hsrc/server/config.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/worker_common.htests/unit/server/config_tests.cpptests/unit/server/stateful_worker_tests.cpptests/unit/server/stateless_worker_tests.cpp
💤 Files with no reviewable changes (1)
- tests/unit/server/stateless_worker_tests.cpp
✅ Files skipped from review due to trivial changes (7)
- tests/unit/server/stateful_worker_tests.cpp
- src/server/worker_common.h
- src/server/protocol.h
- src/server/config.cpp
- tests/unit/server/config_tests.cpp
- benchmarks/scan_benchmark.cpp
- src/server/compiler.h
1 participant
Summary
Three pre-existing bugs cause worker processes to crash with SEGV or SIGABRT. On the main branch these crashes are silent (workers die, requests fail fast with "transport closed", tests still pass because null responses are accepted). However when combined with #432's worker respawn mechanism, the crash-respawn-crash cycle on low-core CI machines causes request timeouts and smoke test hangs.
Fixes
ProxyAction::CreateASTConsumernow checks for null before passing toMultiplexConsumer. When the wrapped action'sCreateASTConsumerfails (e.g. missing system headers during PCH generation), this previously caused a null pointer dereference, SEGV, ASAN kills the stateless worker.file_path()returns emptyStringRefon invalidFileIDinstead of asserting. The assert fired whenIncludeGraph::from()calledfile_path(interested_file())on an AST compiled with synthesized default commands (no compile_commands.json, clang++ -std=c++20 fallback, no system headers, invalid main file ID), SIGABRT, stateful worker crash.ensure_pchnow creates the PCH cache directory before sending the build request. Previously, whenload_workspace()exited early (no compile_commands.json), the cache subdirectories were never created, causing every PCH write to fail with "No such file or directory".load_workspace()changed fromkota::task<>to plainvoid-- it contains only synchronous filesystem operations and no co_await, so the coroutine wrapper was unnecessary. Called directly instead of vialoop.schedule().Test plan
Codesmith can help with this PR — just tag
@codesmithor enable autofix.Summary by CodeRabbit
Bug Fixes
Refactor