Repository navigation
feature: implement server logic - #356
Closed
16bit-ykiko wants to merge 1 commit into
Closed
16bit-ykiko wants to merge 1 commit into
16bit-ykiko wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of the server architecture, replacing the monolithic worker implementation with a multi-process worker pool system. It adds configuration file parsing via TOML, a compilation service for recipe resolution, and separate stateful and stateless worker runtime implementations. The master server is redesigned using the PIMPL pattern to encapsulate complexity. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant MasterServer
participant WorkerPool
participant StatefulWorker
participant StatelessWorker
participant CompilationService
Client->>MasterServer: Initialize (with workspace)
MasterServer->>MasterServer: Parse clice.toml config
MasterServer->>WorkerPool: Create with options
MasterServer->>WorkerPool: start()
WorkerPool->>StatefulWorker: Spawn workers
WorkerPool->>StatelessWorker: Spawn workers
Client->>MasterServer: Compile request
MasterServer->>WorkerPool: compile(params)
WorkerPool->>CompilationService: resolve_recipe()
CompilationService-->>WorkerPool: Recipe with compile command
WorkerPool->>StatefulWorker: Send compile task
StatefulWorker->>StatefulWorker: Build AST, extract diagnostics
StatefulWorker-->>WorkerPool: CompileResult
WorkerPool-->>MasterServer: CompileResult
MasterServer-->>Client: Diagnostics response
Client->>MasterServer: Completion request
MasterServer->>WorkerPool: completion(params)
WorkerPool->>StatelessWorker: Send completion task
StatelessWorker->>CompilationService: resolve_recipe()
StatelessWorker->>StatelessWorker: Compile, generate completions
StatelessWorker-->>WorkerPool: CompletionResult
WorkerPool-->>MasterServer: CompletionResult
MasterServer-->>Client: Completions response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 2
🧹 Nitpick comments (17)
src/server/runtimer.cpp (2)
95-98: Inconsistent logging:std::printlnvsLOG_*macros.
run_worker_modeusesstd::println(stderr, ...)for error output (lines 97, 103), whilerun_master_sessionusesLOG_WARN/LOG_ERROR. If this is intentional (e.g., logging not yet initialized in worker mode), consider adding a brief comment. Otherwise, consider using the logging macros for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/runtimer.cpp` around lines 95 - 98, In run_worker_mode replace the raw std::println(stderr, ...) calls (used when checking resource_dir in function run_worker_mode and similar spots) with the project's logging macros (e.g., LOG_ERROR/LOG_WARN) used by run_master_session so logs are consistent; if logging really isn't initialized in worker mode, add a short comment above run_worker_mode explaining that and keep std::println, otherwise change the prints to LOG_ERROR/LOG_WARN and include resource_dir.error() text in the log call for context.
78-87: Simplify the connection acceptance error handling.The nested conditional checking for
acceptedis complex. Consider restructuring for clarity.♻️ Proposed simplification
auto accepted = accept_task.value(); listener = {}; - if(!accepted || !accepted->has_value()) { - if(accepted && !accepted->has_value()) { - LOG_ERROR("failed to accept connection: {}", accepted->error().message()); - } else { - LOG_ERROR("failed to accept connection: unknown error"); - } + if(!accepted) { + LOG_ERROR("failed to accept connection: task did not complete"); + return 1; + } + if(!accepted->has_value()) { + LOG_ERROR("failed to accept connection: {}", accepted->error().message()); return 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/runtimer.cpp` around lines 78 - 87, The nested checks around the accept result (accept_task, accepted, accepted->has_value()) are overly complex—simplify by evaluating the optional/expected once: if (!accepted.has_value()) { LOG_ERROR("failed to accept connection: unknown error"); return 1; } if (!accepted->has_value()) { LOG_ERROR("failed to accept connection: {}", accepted->error().message()); return 1; } and only then proceed to use accepted.value(); also move or preserve the listener = {} reset so it happens after a successful accept if that was the intent; reference the variables accept_task, accepted and the existing LOG_ERROR calls when making the change.src/server/config.cpp (2)
88-93: Consider adding range validation for integral parsing.The
parse_integralfunction usesstatic_castwithout bounds checking. For smaller integer types (e.g.,int8_t,uint16_t), TOML values exceeding the target type's range would silently overflow.♻️ Proposed fix with bounds checking
template <typename Int> auto parse_integral(Int& output, const toml::node& node) -> void { if(auto value = node.as_integer()) { - output = static_cast<Int>(value->get()); + auto v = value->get(); + if(v >= std::numeric_limits<Int>::min() && v <= std::numeric_limits<Int>::max()) { + output = static_cast<Int>(v); + } + // Silently ignore out-of-range values (or log a warning) } }Note: This requires
#include <limits>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/config.cpp` around lines 88 - 93, The parse_integral(Int& output, const toml::node& node) function currently static_casts without checking bounds; update it to validate the toml integer value against std::numeric_limits<Int>::min()/max() before assignment (use `#include` <limits> and std::is_signed to handle signed vs unsigned targets), and only assign when the value fits—otherwise report/handle the overflow case (e.g., return/throw/log an error) so smaller types like int8_t/uint16_t do not silently overflow.
176-198: Consider treating missing config file as a non-error.When
clice.tomldoesn't exist, the function returns an error. However, many tools treat a missing config file as "use defaults" rather than an error condition. Sincereplace_variablesis called regardless (applying defaults), consider returning success with an optional warning instead.♻️ Proposed change to treat missing config as success
auto ServerConfig::parse(std::string_view workspace_path) -> std::expected<void, std::string> { workspace = std::string(workspace_path); auto file = path::join(workspace, "clice.toml"); std::string error_message; if(fs::exists(file)) { auto parsed = toml::parse_file(file); if(parsed) { parse_file(*this, parsed.table()); } else { error_message = parsed.error().description(); } - } else { - error_message = "Config file doesn't exist!"; } replace_variables(*this, *this); - if(!error_message.empty()) { + if(!error_message.empty()) { // Only error on parse failure, not missing file return std::unexpected(std::move(error_message)); } return {}; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/config.cpp` around lines 176 - 198, The current ServerConfig::parse treats a missing clice.toml as an error by setting error_message and returning unexpected; instead, when fs::exists(file) is false you should not set error_message but proceed to call replace_variables and return success (optionally emit a warning/log). Update the logic in ServerConfig::parse: only populate error_message when toml::parse_file(file) fails (parsed.error()), do not set an error for the non-existent file path variable `file`, and ensure replace_variables(*this, *this) still runs so defaults are applied; keep the final check to return std::unexpected only when error_message is non-empty.CMakeLists.txt (1)
187-187: Consider using PRIVATE linking for tomlplusplus.The
tomlplusplus::tomlplusplusdependency is linked asPUBLICin CMakeLists.txt, which exposes it to all consumers ofclice-core. However, TOML types are not exposed in any public headers—config.honly exposes standard C++ types (std::string,std::vector, etc.), and alltoml::usage is confined toconfig.cpp. This dependency should bePRIVATEto avoid unnecessarily propagating an internal implementation detail to library consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 187, The tomlplusplus dependency is currently linked PUBLIC for the clice-core target which unnecessarily exposes it to consumers; update the target_link_libraries invocation that references tomlplusplus::tomlplusplus for target clice-core to use PRIVATE instead of PUBLIC so the dependency is internal (config.cpp uses toml:: while config.h only exposes std types), ensuring tomlplusplus doesn't propagate to downstream targets.src/server/worker_runtime.cpp (1)
47-61: Redundant state updates when recipe is unchanged.When
recipe->unchangedis true, the code updatesrecipe_state.source_pathandrecipe_state.recipe_revisionfromrecipe_state.recipe, but these fields should already have these values since the recipe is cached. This is harmless but unnecessary.♻️ Optional: Remove redundant assignments
if(recipe->unchanged) { if(!recipe_state.recipe) { co_return std::unexpected( "master returned unchanged compile recipe without local cache"); } - recipe_state.source_path = recipe_state.recipe->source_path; - recipe_state.recipe_revision = recipe_state.recipe->revision; - auto command = make_compile_command(*recipe_state.recipe); if(!command) { co_return std::unexpected(std::move(command.error())); } co_return std::move(*command); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_runtime.cpp` around lines 47 - 61, The branch handling recipe->unchanged performs unnecessary assignments to recipe_state.source_path and recipe_state.recipe_revision even though recipe_state.recipe already holds the cached values; remove those redundant lines (the two assignments to recipe_state.source_path and recipe_state.recipe_revision) inside the if(recipe->unchanged) block and leave the existing early-return logic that validates recipe_state.recipe and calls make_compile_command(*recipe_state.recipe).src/server/worker_stateless.cpp (1)
186-190: Dot-trigger adjustment for completion offset.The logic adjusts the offset backwards when the character before cursor is a
., which is a common pattern for member access completion. However, this only handles.and not->or::which are also common triggers in C++.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_stateless.cpp` around lines 186 - 190, The current offset adjustment in worker_stateless.cpp (using to_offset_utf16, local variable offset and text) only handles a single '.' before the cursor; extend this to also detect and back up over the C++ member operators "->" and scope operator "::". After computing offset, ensure you check bounds (offset >= 2) and then if the two characters immediately before offset form "->" or "::" subtract 2 from offset (for "->"/"::") or subtract 1 for '.' as currently done; keep using static_cast<std::size_t>(offset) when indexing text to avoid signed/unsigned errors and ensure safe checks before accessing text[offset-1] or text[offset-2].src/server/worker_stateful.cpp (1)
29-39: Debug/diagnostic hover result may confuse users.The
make_hover_resultfunction returns a fallback hover containing debug information ("clice hover snapshot: uri=..."). While useful for debugging, this might confuse end users in production. Consider making this behavior configurable or returningstd::nulloptinstead.♻️ Consider returning nullopt for cleaner UX
auto make_hover_result(std::string_view uri, int version, int line, int character) -> rpc::RequestTraits<rpc::HoverParams>::Result { - rpc::Hover hover; - hover.contents = rpc::MarkupContent{ - .kind = rpc::MarkupKind::Plaintext, - .value = "clice hover snapshot: uri=" + std::string(uri) + - ", version=" + std::to_string(version) + ", line=" + std::to_string(line) + - ", character=" + std::to_string(character), - }; - return hover; + (void)uri; + (void)version; + (void)line; + (void)character; + return std::nullopt; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_stateful.cpp` around lines 29 - 39, The fallback debug hover in make_hover_result currently returns a visible rpc::Hover with debug text (rpc::MarkupContent/MarkupKind) which may confuse users; update make_hover_result to avoid returning user-facing debug text by either returning std::nullopt (change the function to return an optional<rpc::RequestTraits<rpc::HoverParams>::Result>) or gating the debug payload behind a configurable flag (e.g., a boolean like enable_debug_hover) so production builds return nullopt while dev builds can return the existing rpc::Hover with the "clice hover snapshot" content; adjust callers to handle std::nullopt if you choose the optional route and keep references to make_hover_result, rpc::Hover, and rpc::MarkupContent to locate the change.src/server/config.h (1)
22-34: Consider documenting the RuleConfig pattern matching behavior.The
RuleConfigstruct supports pattern-based argument modification (remove/append), but the pattern matching semantics aren't documented in the header. Consider adding a brief comment or ensuring documentation exists elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/config.h` around lines 22 - 34, The header declares RuleConfig with members patterns, remove, and append but lacks documentation of how pattern matching works; add a concise comment above the struct (or above the patterns field) explaining the matching semantics used by RuleConfig (e.g., whether patterns are glob, regex, exact match, case sensitivity, and how they are applied to arguments), and mention how conflicts between remove and append are resolved if relevant; reference the RuleConfig struct and its patterns/remove/append members so callers and implementers know expected behavior.src/server/worker_runtime.h (1)
28-35: CompileCommand design note: pointer invalidation risk.The
CompileCommandstruct stores both owned strings (owned_arguments) and raw pointers (arguments). Thefinalize()method must be called after any modification toowned_argumentsto keep pointers valid. Consider adding a comment documenting this contract, or makingargumentsa computed property.📝 Consider adding documentation comment
struct CompileCommand { bool arguments_from_database = false; std::string directory; std::vector<std::string> owned_arguments; + /// Pointers into owned_arguments. Call finalize() after modifying owned_arguments. std::vector<const char*> arguments; void finalize(); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_runtime.h` around lines 28 - 35, The CompileCommand stores both owned_arguments and raw pointers in arguments which can be invalidated whenever owned_arguments changes; update the code to either (A) document the contract clearly by adding a comment above struct CompileCommand and above finalize() stating that finalize() must be called after any modification to owned_arguments to rebuild arguments, or (B) convert arguments into a computed accessor (e.g., a method get_arguments() or make arguments a private cached vector rebuilt inside finalize()) so pointers are always rebuilt from owned_arguments inside finalize(); refer to CompileCommand, owned_arguments, arguments, and finalize() when making the change.src/server/compilation_service.cpp (1)
80-104: Consider adding.Cextension for C++ detection.The
is_cpp_like_filefunction covers common C++ extensions but omits.C(uppercase), which is historically used on some Unix systems to denote C++ files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/compilation_service.cpp` around lines 80 - 104, The is_cpp_like_file function currently misses the historical uppercase extension ".C"; update the cpp_ext list in is_cpp_like_file to include ".C" or, better, normalize the file extension (e.g., convert extension to lowercase) and compare against a lowercase set that includes ".c" variants such as ".c" (if desired) and ensure ".c" vs ".C" handling—specifically modify is_cpp_like_file to either add ".C" to constexpr std::array<std::string_view, 13> cpp_ext or perform a case-insensitive compare by lowercasing extension() before the loop so files with ".C" are recognized as C++ sources.src/server/worker_pool.cpp (4)
341-360:pick_stateful_workerreturns 0 when pool is empty, which may be invalid.When
stateful_workersis empty, this function returns0, but index0may not be a valid stateful worker (or may not exist at all). The callers (assign_stateful_worker,compile,hover) callensure_stateful_worker()first, so this should be safe in practice, but the function's contract is unclear.Consider returning
std::optional<std::size_t>for consistency withselect_stateless_worker, or add a precondition comment.📝 Suggested documentation or signature change
Option 1 - Add precondition comment:
+ // Precondition: stateful_workers must not be empty auto pick_stateful_worker() const -> std::size_t { if(stateful_workers.empty()) { - return 0; + // Should never happen if callers ensure workers exist + return 0; }Option 2 - Return optional for consistency:
- auto pick_stateful_worker() const -> std::size_t { + auto pick_stateful_worker() const -> std::optional<std::size_t> { if(stateful_workers.empty()) { - return 0; + return std::nullopt; } // ... rest unchanged, return selected instead of returning 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_pool.cpp` around lines 341 - 360, pick_stateful_worker currently returns 0 when stateful_workers is empty which can be invalid; change its signature to return std::optional<std::size_t> (matching select_stateless_worker) and return std::nullopt when stateful_workers.empty(); update callers (assign_stateful_worker, compile, hover) to handle the optional or assert/ensure_stateful_worker beforehand, and adjust any uses of the returned index accordingly so no invalid index is assumed; keep function logic otherwise but return std::optional from pick_stateful_worker.
115-121: Same pattern: Consider moving default into switch body.Similar to
worker_role_name, the unreachablereturn "stateful"after the switch could be moved into adefaultcase for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_pool.cpp` around lines 115 - 121, The function worker_role_argument currently returns "stateful" after the switch; instead move that fallback into the switch as a default label to keep behavior consistent with worker_role_name and avoid an unreachable return. Modify the switch in worker_role_argument to add a default: case that returns "stateful" (preserving the existing returns for WorkerRole::Stateful and WorkerRole::Stateless) and remove the trailing return after the switch. Ensure the function signature and enum WorkerRole usages remain unchanged.
41-47: Adddefaultcase to switch statement.The switch statement covers all enum cases but lacks a
defaultcase, which can cause compiler warnings with-Wswitch-default. Adding an explicitunreachable()or thedefault: return "unknown";at the end of the switch body (inside the braces) would be cleaner.♻️ Suggested fix
static auto worker_role_name(WorkerRole role) -> std::string_view { switch(role) { case WorkerRole::Stateful: return "stateful"; case WorkerRole::Stateless: return "stateless"; + default: return "unknown"; } - return "unknown"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_pool.cpp` around lines 41 - 47, The switch in worker_role_name(WorkerRole) enumerates WorkerRole cases but lacks a default, causing -Wswitch-default warnings; update the switch body to include an explicit default branch (e.g., default: return "unknown"; or call unreachable()) so the function always returns and the compiler warning is silenced, referencing the worker_role_name function and the WorkerRole enum.
563-574: Consider storingOptionsby value instead of reference.
Implstoresconst Options& optionsat line 564, which creates a lifetime dependency on the caller. While the current call path inrun_master_sessionis safe (the Options object outlives the MasterServer), this reference pattern is fragile for future maintenance. If async operations or the event loop semantics change, dangling references become possible. Storing Options by value would eliminate this risk entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/worker_pool.cpp` around lines 563 - 574, Impl currently holds a const Options& options which creates a fragile lifetime dependency; change Impl to store Options by value (Options options) and update its constructor and any initialization sites (e.g., where Impl is constructed inside run_master_session / MasterServer) to pass/clone the Options into the Impl instance so the Impl owns its copy and cannot dangle; ensure any uses of the member continue to use the same name (options) and remove assumptions that the original caller outlives the worker pool.src/server/master_server.cpp (2)
377-401: Potential issue:on_did_changemay lose incremental changes.The current implementation iterates through all content changes but only keeps the last
textvalue. ForTextDocumentContentChangePartial, this may not correctly represent the final document state if multiple partial changes are applied - each partial change would need to be applied to a range, not just store its text.However, since the server advertises
TextDocumentSyncKind::Full(line 34), clients should only send whole-document changes, making this benign. Consider adding a comment to clarify this assumption.📝 Suggested comment for clarity
void on_did_change(const rpc::DidChangeTextDocumentParams& params) { if(!accept_document_notifications()) { return; } + // Server advertises Full sync, so we expect only whole-document changes. + // We take the last content in case of multiple events batched together. std::optional<std::string> latest_text; for(const auto& change: params.content_changes) {🤖 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 377 - 401, The on_did_change handler currently only keeps the last content change text which would lose incremental edits if partial/range-based changes were sent; update the function (on_did_change) to document the assumption that the server advertises TextDocumentSyncKind::Full and therefore clients will only send whole-document changes, or alternately explicitly ignore/apply only whole-document changes by checking for rpc::TextDocumentContentChangeWholeDocument; add a concise comment above the loop referencing TextDocumentSyncKind::Full and/or add a brief guard that skips partial changes so future maintainers understand why only the last full-document text is used.
69-76: Consider extractingnormalized_pathto avoid duplication.This function is duplicated from
src/server/compilation_service.cpp(lines 28-35). Consider moving it to a shared utility header (e.g.,support/filesystem.h) to avoid code duplication and ensure consistent behavior across the codebase.🤖 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 69 - 76, Extract the duplicated function normalized_path into a single shared utility header (e.g., create support/filesystem.h) and update both master_server.cpp and compilation_service.cpp to include that header and remove their local copies; ensure the new header declares/defines auto normalized_path(fs_std::path path) -> std::string with the same semantics (using fs_std::weakly_canonical, std::error_code, and fallback to path.lexically_normal()), and add any necessary includes/namespaces so callers in master_server.cpp and compilation_service.cpp compile without changes to their call sites.
🤖 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 460-495: ensure_stateless_pch can apply a stale PCH if the
document changed while co_await workers.build_pch(...) was running; after the
co_await returns, re-check the current document generation for snapshot.uri
(compare the live document/snapshot.generation to the original
snapshot.generation passed into ensure_stateless_pch) and if it differs, do not
update stateless_pch and return std::nullopt; ensure this generation check
occurs before assigning cache.output_path/cache.preamble/cache.preamble_bound
and before returning the StatelessPCHBinding so the cache never becomes
inconsistent with the live document.
In `@src/server/worker_pool.cpp`:
- Around line 494-529: The shutdown() coroutine currently calls co_await
worker.process.wait() and later worker.process.kill(SIGTERM) without any timeout
or escalation, which can hang indefinitely; update shutdown() to for each worker
(using worker_role_name and worker.pid to identify) perform a bounded shutdown:
first attempt to close_output(), then send SIGTERM
(worker.process.kill(SIGTERM)), wait for the process to exit with a per-worker
timeout (implement a timer/cancellable task that races against
worker.process.wait()), and if the wait times out send SIGKILL
(worker.process.kill(SIGKILL)) and wait again with a short timeout; ensure you
log the different outcomes (exited normally, exited after SIGTERM, killed by
SIGKILL, or failed to exit) and always avoid blocking forever on
worker.process.wait() by cancelling/aborting the wait when the timeout fires.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Line 187: The tomlplusplus dependency is currently linked PUBLIC for the
clice-core target which unnecessarily exposes it to consumers; update the
target_link_libraries invocation that references tomlplusplus::tomlplusplus for
target clice-core to use PRIVATE instead of PUBLIC so the dependency is internal
(config.cpp uses toml:: while config.h only exposes std types), ensuring
tomlplusplus doesn't propagate to downstream targets.
In `@src/server/compilation_service.cpp`:
- Around line 80-104: The is_cpp_like_file function currently misses the
historical uppercase extension ".C"; update the cpp_ext list in is_cpp_like_file
to include ".C" or, better, normalize the file extension (e.g., convert
extension to lowercase) and compare against a lowercase set that includes ".c"
variants such as ".c" (if desired) and ensure ".c" vs ".C" handling—specifically
modify is_cpp_like_file to either add ".C" to constexpr
std::array<std::string_view, 13> cpp_ext or perform a case-insensitive compare
by lowercasing extension() before the loop so files with ".C" are recognized as
C++ sources.
In `@src/server/config.cpp`:
- Around line 88-93: The parse_integral(Int& output, const toml::node& node)
function currently static_casts without checking bounds; update it to validate
the toml integer value against std::numeric_limits<Int>::min()/max() before
assignment (use `#include` <limits> and std::is_signed to handle signed vs
unsigned targets), and only assign when the value fits—otherwise report/handle
the overflow case (e.g., return/throw/log an error) so smaller types like
int8_t/uint16_t do not silently overflow.
- Around line 176-198: The current ServerConfig::parse treats a missing
clice.toml as an error by setting error_message and returning unexpected;
instead, when fs::exists(file) is false you should not set error_message but
proceed to call replace_variables and return success (optionally emit a
warning/log). Update the logic in ServerConfig::parse: only populate
error_message when toml::parse_file(file) fails (parsed.error()), do not set an
error for the non-existent file path variable `file`, and ensure
replace_variables(*this, *this) still runs so defaults are applied; keep the
final check to return std::unexpected only when error_message is non-empty.
In `@src/server/config.h`:
- Around line 22-34: The header declares RuleConfig with members patterns,
remove, and append but lacks documentation of how pattern matching works; add a
concise comment above the struct (or above the patterns field) explaining the
matching semantics used by RuleConfig (e.g., whether patterns are glob, regex,
exact match, case sensitivity, and how they are applied to arguments), and
mention how conflicts between remove and append are resolved if relevant;
reference the RuleConfig struct and its patterns/remove/append members so
callers and implementers know expected behavior.
In `@src/server/master_server.cpp`:
- Around line 377-401: The on_did_change handler currently only keeps the last
content change text which would lose incremental edits if partial/range-based
changes were sent; update the function (on_did_change) to document the
assumption that the server advertises TextDocumentSyncKind::Full and therefore
clients will only send whole-document changes, or alternately explicitly
ignore/apply only whole-document changes by checking for
rpc::TextDocumentContentChangeWholeDocument; add a concise comment above the
loop referencing TextDocumentSyncKind::Full and/or add a brief guard that skips
partial changes so future maintainers understand why only the last full-document
text is used.
- Around line 69-76: Extract the duplicated function normalized_path into a
single shared utility header (e.g., create support/filesystem.h) and update both
master_server.cpp and compilation_service.cpp to include that header and remove
their local copies; ensure the new header declares/defines auto
normalized_path(fs_std::path path) -> std::string with the same semantics (using
fs_std::weakly_canonical, std::error_code, and fallback to
path.lexically_normal()), and add any necessary includes/namespaces so callers
in master_server.cpp and compilation_service.cpp compile without changes to
their call sites.
In `@src/server/runtimer.cpp`:
- Around line 95-98: In run_worker_mode replace the raw std::println(stderr,
...) calls (used when checking resource_dir in function run_worker_mode and
similar spots) with the project's logging macros (e.g., LOG_ERROR/LOG_WARN) used
by run_master_session so logs are consistent; if logging really isn't
initialized in worker mode, add a short comment above run_worker_mode explaining
that and keep std::println, otherwise change the prints to LOG_ERROR/LOG_WARN
and include resource_dir.error() text in the log call for context.
- Around line 78-87: The nested checks around the accept result (accept_task,
accepted, accepted->has_value()) are overly complex—simplify by evaluating the
optional/expected once: if (!accepted.has_value()) { LOG_ERROR("failed to accept
connection: unknown error"); return 1; } if (!accepted->has_value()) {
LOG_ERROR("failed to accept connection: {}", accepted->error().message());
return 1; } and only then proceed to use accepted.value(); also move or preserve
the listener = {} reset so it happens after a successful accept if that was the
intent; reference the variables accept_task, accepted and the existing LOG_ERROR
calls when making the change.
In `@src/server/worker_pool.cpp`:
- Around line 341-360: pick_stateful_worker currently returns 0 when
stateful_workers is empty which can be invalid; change its signature to return
std::optional<std::size_t> (matching select_stateless_worker) and return
std::nullopt when stateful_workers.empty(); update callers
(assign_stateful_worker, compile, hover) to handle the optional or
assert/ensure_stateful_worker beforehand, and adjust any uses of the returned
index accordingly so no invalid index is assumed; keep function logic otherwise
but return std::optional from pick_stateful_worker.
- Around line 115-121: The function worker_role_argument currently returns
"stateful" after the switch; instead move that fallback into the switch as a
default label to keep behavior consistent with worker_role_name and avoid an
unreachable return. Modify the switch in worker_role_argument to add a default:
case that returns "stateful" (preserving the existing returns for
WorkerRole::Stateful and WorkerRole::Stateless) and remove the trailing return
after the switch. Ensure the function signature and enum WorkerRole usages
remain unchanged.
- Around line 41-47: The switch in worker_role_name(WorkerRole) enumerates
WorkerRole cases but lacks a default, causing -Wswitch-default warnings; update
the switch body to include an explicit default branch (e.g., default: return
"unknown"; or call unreachable()) so the function always returns and the
compiler warning is silenced, referencing the worker_role_name function and the
WorkerRole enum.
- Around line 563-574: Impl currently holds a const Options& options which
creates a fragile lifetime dependency; change Impl to store Options by value
(Options options) and update its constructor and any initialization sites (e.g.,
where Impl is constructed inside run_master_session / MasterServer) to
pass/clone the Options into the Impl instance so the Impl owns its copy and
cannot dangle; ensure any uses of the member continue to use the same name
(options) and remove assumptions that the original caller outlives the worker
pool.
In `@src/server/worker_runtime.cpp`:
- Around line 47-61: The branch handling recipe->unchanged performs unnecessary
assignments to recipe_state.source_path and recipe_state.recipe_revision even
though recipe_state.recipe already holds the cached values; remove those
redundant lines (the two assignments to recipe_state.source_path and
recipe_state.recipe_revision) inside the if(recipe->unchanged) block and leave
the existing early-return logic that validates recipe_state.recipe and calls
make_compile_command(*recipe_state.recipe).
In `@src/server/worker_runtime.h`:
- Around line 28-35: The CompileCommand stores both owned_arguments and raw
pointers in arguments which can be invalidated whenever owned_arguments changes;
update the code to either (A) document the contract clearly by adding a comment
above struct CompileCommand and above finalize() stating that finalize() must be
called after any modification to owned_arguments to rebuild arguments, or (B)
convert arguments into a computed accessor (e.g., a method get_arguments() or
make arguments a private cached vector rebuilt inside finalize()) so pointers
are always rebuilt from owned_arguments inside finalize(); refer to
CompileCommand, owned_arguments, arguments, and finalize() when making the
change.
In `@src/server/worker_stateful.cpp`:
- Around line 29-39: The fallback debug hover in make_hover_result currently
returns a visible rpc::Hover with debug text (rpc::MarkupContent/MarkupKind)
which may confuse users; update make_hover_result to avoid returning user-facing
debug text by either returning std::nullopt (change the function to return an
optional<rpc::RequestTraits<rpc::HoverParams>::Result>) or gating the debug
payload behind a configurable flag (e.g., a boolean like enable_debug_hover) so
production builds return nullopt while dev builds can return the existing
rpc::Hover with the "clice hover snapshot" content; adjust callers to handle
std::nullopt if you choose the optional route and keep references to
make_hover_result, rpc::Hover, and rpc::MarkupContent to locate the change.
In `@src/server/worker_stateless.cpp`:
- Around line 186-190: The current offset adjustment in worker_stateless.cpp
(using to_offset_utf16, local variable offset and text) only handles a single
'.' before the cursor; extend this to also detect and back up over the C++
member operators "->" and scope operator "::". After computing offset, ensure
you check bounds (offset >= 2) and then if the two characters immediately before
offset form "->" or "::" subtract 2 from offset (for "->"/"::") or subtract 1
for '.' as currently done; keep using static_cast<std::size_t>(offset) when
indexing text to avoid signed/unsigned errors and ensure safe checks before
accessing text[offset-1] or text[offset-2].
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
CMakeLists.txtsrc/clice.ccsrc/server/compilation_service.cppsrc/server/compilation_service.hsrc/server/config.cppsrc/server/config.hsrc/server/master_server.cppsrc/server/master_server.hsrc/server/protocol.hsrc/server/runtime.hsrc/server/runtimer.cppsrc/server/worker.cppsrc/server/worker_pool.cppsrc/server/worker_pool.hsrc/server/worker_runtime.cppsrc/server/worker_runtime.hsrc/server/worker_stateful.cppsrc/server/worker_stateless.cpptests/data/feature_requests/main.cpptests/integration/test_file_operation.py
💤 Files with no reviewable changes (1)
- src/server/worker.cpp
Comment on lines
+460
to
+495
| auto ensure_stateless_pch(const HoverRequestSnapshot& snapshot) | ||
| -> et::task<std::optional<StatelessPCHBinding>> { | ||
| auto preamble_bound = compute_preamble_bound(snapshot.text); | ||
| if(preamble_bound == 0) { | ||
| stateless_pch.erase(snapshot.uri); | ||
| co_return std::nullopt; | ||
| } | ||
|
|
||
| auto preamble = snapshot.text.substr(0, preamble_bound); | ||
| auto& cache = stateless_pch[snapshot.uri]; | ||
| const bool need_rebuild = cache.output_path.empty() || | ||
| cache.preamble_bound != preamble_bound || | ||
| cache.preamble != preamble; | ||
|
|
||
| if(need_rebuild) { | ||
| WorkerBuildPCHParams build_params{ | ||
| .uri = snapshot.uri, | ||
| .text = snapshot.text, | ||
| .output_path = cache.output_path, | ||
| }; | ||
| auto built = co_await workers.build_pch(std::move(build_params)); | ||
| if(!built || !built->built || built->output_path.empty()) { | ||
| stateless_pch.erase(snapshot.uri); | ||
| co_return std::nullopt; | ||
| } | ||
|
|
||
| cache.output_path = std::move(built->output_path); | ||
| cache.preamble = std::move(preamble); | ||
| cache.preamble_bound = preamble_bound; | ||
| } | ||
|
|
||
| co_return StatelessPCHBinding{ | ||
| .path = cache.output_path, | ||
| .preamble_bound = cache.preamble_bound, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Generation check is missing before PCH rebuild.
In ensure_stateless_pch, after the async build_pch call completes (line 480), the document may have changed. Unlike run_completion and run_signature_help, there's no generation check immediately after co_await workers.build_pch(...) returns. If the document changed during the build, the cached PCH may be stale for the subsequent request.
This is partially mitigated by the caller (run_completion/run_signature_help) performing a generation check after ensure_stateless_pch returns, but the PCH cache state may still be inconsistent with the current document.
🛡️ Proposed fix to add staleness check
auto built = co_await workers.build_pch(std::move(build_params));
if(!built || !built->built || built->output_path.empty()) {
stateless_pch.erase(snapshot.uri);
co_return std::nullopt;
}
+ // Check if document changed during build - if so, PCH may be stale
+ auto doc_iter = documents.find(snapshot.uri);
+ if(doc_iter == documents.end() || doc_iter->second.generation != snapshot.generation) {
+ // Document changed; the caller will detect this and abort
+ // Don't update cache with potentially stale PCH
+ co_return std::nullopt;
+ }
+
cache.output_path = std::move(built->output_path);🤖 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 460 - 495, ensure_stateless_pch
can apply a stale PCH if the document changed while co_await
workers.build_pch(...) was running; after the co_await returns, re-check the
current document generation for snapshot.uri (compare the live
document/snapshot.generation to the original snapshot.generation passed into
ensure_stateless_pch) and if it differs, do not update stateless_pch and return
std::nullopt; ensure this generation check occurs before assigning
cache.output_path/cache.preamble/cache.preamble_bound and before returning the
StatelessPCHBinding so the cache never becomes inconsistent with the live
document.
Comment on lines
+494
to
+529
| auto shutdown() -> et::task<> { | ||
| if(!started) { | ||
| co_return; | ||
| } | ||
| started = false; | ||
|
|
||
| for(auto& worker: workers) { | ||
| if(worker.peer) { | ||
| auto status = worker.peer->close_output(); | ||
| (void)status; | ||
| } | ||
| } | ||
|
|
||
| for(auto& worker: workers) { | ||
| auto role_name = worker_role_name(worker.role); | ||
| auto waited = co_await worker.process.wait(); | ||
| if(waited) { | ||
| LOG_INFO("[worker:{} pid={}] exited (shutdown): status={} signal={}", | ||
| role_name, | ||
| worker.pid, | ||
| static_cast<long long>(waited->status), | ||
| waited->term_signal); | ||
| continue; | ||
| } | ||
|
|
||
| auto kill_status = worker.process.kill(SIGTERM); | ||
| (void)kill_status; | ||
| auto waited_after_kill = co_await worker.process.wait(); | ||
| if(waited_after_kill) { | ||
| LOG_INFO("[worker:{} pid={}] exited (shutdown-kill): status={} signal={}", | ||
| role_name, | ||
| worker.pid, | ||
| static_cast<long long>(waited_after_kill->status), | ||
| waited_after_kill->term_signal); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the process-related files in the eventide directory
fd -t f "process" | grep -E "\.(cpp|h)$" | head -20Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Search for the worker structure definition
rg -n "struct worker\|class worker" --type cpp | head -20Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Look for the process wait method signature
rg -n "\.wait\(" --type cpp -A 3 | head -30Repository: clice-io/clice
Length of output: 1043
🏁 Script executed:
# Search for worker struct/class definition anywhere
rg -n "struct worker\|class worker\|using worker" --type cpp --type hRepository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Look at includes in worker_pool.cpp to find where process type comes from
head -50 src/server/worker_pool.cpp | grep -E "^#include"Repository: clice-io/clice
Length of output: 451
🏁 Script executed:
# Search for process class definition more broadly
rg -n "class process\|struct process\|template.*process" --type cpp --type h | head -20Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Check worker_pool.h for worker definition
cat src/server/worker_pool.h | head -100Repository: clice-io/clice
Length of output: 1741
🏁 Script executed:
# Find eventide process.h
find . -name "process.h" -o -name "process.hpp" | grep eventideRepository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Search for eventide directory
fd -t d "eventide" | head -10Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Look for the Impl struct definition in worker_pool.cpp
rg -n "struct Impl\|class Impl" --type cpp -A 50 src/server/worker_pool.cpp | head -100Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Search for eventide as submodule or external
find . -name ".gitmodules" -o -name "*.lock" | xargs grep -l eventide 2>/dev/nullRepository: clice-io/clice
Length of output: 42
🏁 Script executed:
# Check if there's a third-party or external directory
ls -la | grep -E "third|external|vendor|deps"Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Search in entire codebase for wait_for method
rg -n "wait_for" --type cpp --type hRepository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Read the entire worker_pool.cpp file to see Impl definition and worker structure
wc -l src/server/worker_pool.cppRepository: clice-io/clice
Length of output: 88
🏁 Script executed:
# Get Impl struct definition from worker_pool.cpp
sed -n '1,100p' src/server/worker_pool.cppRepository: clice-io/clice
Length of output: 3292
🏁 Script executed:
# Search for where worker is defined in worker_pool.cpp
rg -n "struct worker" --type cpp -iRepository: clice-io/clice
Length of output: 1173
🏁 Script executed:
# Look at the area around shutdown() implementation
sed -n '480,540p' src/server/worker_pool.cppRepository: clice-io/clice
Length of output: 2092
🏁 Script executed:
# Check CMakeLists.txt for eventide configuration
find . -name "CMakeLists.txt" -exec cat {} \; | head -100Repository: clice-io/clice
Length of output: 3324
🏁 Script executed:
# Check for git submodules configuration
cat .gitmodules 2>/dev/null || echo "No .gitmodules found"Repository: clice-io/clice
Length of output: 78
🏁 Script executed:
# Search for any documentation or README mentioning eventide
find . -name "README*" -o -name "CONTRIBUTING*" -o -name "*.md" | xargs grep -l eventide 2>/dev/null | head -5Repository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Look for any conan, vcpkg, or other package manager files
find . -name "conanfile.*" -o -name "vcpkg.json" -o -name "package.json" -o -name "Cargo.toml"Repository: clice-io/clice
Length of output: 132
🏁 Script executed:
# Check the package.cmake file that was included
cat cmake/package.cmake 2>/dev/null | head -200Repository: clice-io/clice
Length of output: 1619
🏁 Script executed:
# Search for FetchContent or external dependency declarations
rg -n "FetchContent\|find_package\|add_subdirectory" --type cmakeRepository: clice-io/clice
Length of output: 40
🏁 Script executed:
# Check if eventide is mentioned anywhere in the build system
rg -n "eventide" --type cmake --type txtRepository: clice-io/clice
Length of output: 529
🏁 Script executed:
# Look at the actual process class to understand its API better
# Search for any documentation or examples of et::process usage
rg -n "et::process\|eventide::process" --type cpp -A 5 | head -50Repository: clice-io/clice
Length of output: 40
🌐 Web query:
eventide library process wait_for timeout C++ async
💡 Result:
In C++ async code, the common “wait_for + timeout” patterns depend on which async abstraction you’re using:
1) Standard C++ (std::async / std::future)
- Call
future.wait_for(duration)and check the returnedstd::future_status:ready(finished),timeout(not finished yet), ordeferred(will only run when you callget()). [1]
wait_foris non-throwing; it just reports status. (You typically only callget()once you seeready.) [1]
2) C++/WinRT async (IAsyncOperation, etc.)
async.wait_for(5s)returns anAsyncStatus(e.g.,Completed,Started,Canceled,Error), and you can switch on it and only callGetResults()whenCompleted. [2]
3) A coroutine library that provides wait_for (example: netcan/asyncio)
co_await wait_for(task, duration)can throw a timeout exception (e.g.,TimeoutError) if the duration elapses before completion. [3]- If you need the underlying work to stop, you generally must cancel the task (a timeout wrapper alone may not stop the underlying operation unless the library ties timeout to cancellation). This library also shows explicit task cancellation patterns. [3]
Sources:
[1] C++ Crash Course (std::future_status, wait_for) (cpp-crash-course.netlify.app)
[2] Microsoft Learn (C++/WinRT wait_for returning AsyncStatus) (learn.microsoft.com)
[3] netcan/asyncio README (wait_for throwing TimeoutError, cancellation examples) (github.com)
Citations:
- 1: https://cpp-crash-course.netlify.app/chapters/chapter%2019%20concurrency%20and%20parallelism?utm_source=openai
- 2: https://learn.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/concurrency-2?utm_source=openai
- 3: https://github.com/netcan/asyncio?utm_source=openai
🌐 Web query:
site:github.com/clice-io/eventide process.h wait
💡 Result:
In clice-io/eventide, the process “wait” API is:
- Declared in
include/eventide/async/process.hastask<wait_result> wait();, wherewait_resultisresult<exit_status>andexit_statuscontains{ int64_t status; int term_signal; }. [1] - Implemented in
src/async/process.cpp:- If the
processis not initialized (!self), it returnserror::invalid_argument. [2] - If an exit status is already latched (
self->has_pending()), it returns the cached status immediately (self->peek_pending()). [2] - If another coroutine is already waiting (
self->has_waiter()), it returnserror::connection_already_in_progress. [2] - Otherwise it
co_awaits an internalprocess_awaitawaiter that arms a waiter and resumes whenuv_spawn’sexit_cbdelivers the exit status. [2]
- If the
Sources:
[1] include/eventide/async/process.h (raw) (raw.githubusercontent.com)
[2] src/async/process.cpp (raw) (raw.githubusercontent.com)
Citations:
- 1: https://raw.githubusercontent.com/clice-io/eventide/main/include/eventide/async/process.h
- 2: https://raw.githubusercontent.com/clice-io/eventide/main/src/async/process.cpp
Shutdown can hang indefinitely waiting for unresponsive workers.
The shutdown sequence waits for each worker process to exit without timeout. If a worker is stuck (infinite loop, blocked I/O), co_await worker.process.wait() at line 509 will block indefinitely. After sending SIGTERM at line 520, a second wait at line 521 blocks again if the worker ignores the signal.
Additionally, there's no escalation to SIGKILL—only SIGTERM is attempted. A robust shutdown needs either:
- A timeout per worker (requiring a timeout task since
eventide::process::wait()doesn't supportwait_for) - An escalation to SIGKILL after a grace period
- Or both
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server/worker_pool.cpp` around lines 494 - 529, The shutdown() coroutine
currently calls co_await worker.process.wait() and later
worker.process.kill(SIGTERM) without any timeout or escalation, which can hang
indefinitely; update shutdown() to for each worker (using worker_role_name and
worker.pid to identify) perform a bounded shutdown: first attempt to
close_output(), then send SIGTERM (worker.process.kill(SIGTERM)), wait for the
process to exit with a per-worker timeout (implement a timer/cancellable task
that races against worker.process.wait()), and if the wait times out send
SIGKILL (worker.process.kill(SIGKILL)) and wait again with a short timeout;
ensure you log the different outcomes (exited normally, exited after SIGTERM,
killed by SIGKILL, or failed to exit) and always avoid blocking forever on
worker.process.wait() by cancelling/aborting the wait when the timeout fires.
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 by CodeRabbit
Release Notes
New Features
--worker-roleand--stateless-worker-countfor customizing worker behavior.Tests