Repository navigation
fix(server): structured shutdown with when_any cancellation - #444
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors MasterServer lifecycle: separates shutdown signaling from async cleanup, moves file watching into a coroutine, uses task groups for connection handling, coordinates modes with kota::when_any, preallocates worker pools, and adds sanitizer-aware test assertions for clean server exit. ChangesServer Shutdown and Async Lifecycle Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant MasterServer
participant Acceptor
participant LSPPeer
participant Indexer
participant Compiler
participant WorkerPool
Client->>MasterServer: connect / request
MasterServer->>Acceptor: accept_connections() (task_group)
Acceptor->>LSPPeer: spawn per-connection task
Client->>LSPPeer: LSP registration / requests
MasterServer->>Indexer: coordinate stop (on shutdown)
MasterServer->>Compiler: coordinate stop (on shutdown)
MasterServer->>WorkerPool: stop workers (on shutdown)
MasterServer->>MasterServer: shutdown_and_cleanup() persists index/cache and sets Exited
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 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 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 |
16bit-ykiko
marked this pull request as ready for review
June 1, 2026 01:31
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/agentic/test_agentic.py (1)
533-597:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure the server is still torn down on early test failures.
If
c.initialize(...)fails on Lines 570-575 while the subprocess is still alive, or anything raises before Line 592, thefinallyblock only cancels client tasks and leaves the spawned server running. That can leak a background server into later tests and mask the original failure.💡 Suggested fix
Add
_shutdown_clientto the import on Line 533, then fall back to it unless the explicit clean-exit assertion already ran:- from tests.conftest import _find_free_port, assert_server_exited_cleanly + from tests.conftest import ( + _find_free_port, + _shutdown_client, + assert_server_exited_cleanly, + )c = CliceClient() await c.start_io(*cmd) + clean_exit_asserted = False try: init_options = { "project": { "cache_dir": str(workspace / ".clice"), "idle_timeout_ms": 0, @@ rpc.sock.close() await assert_server_exited_cleanly(c._server, timeout=15.0) + clean_exit_asserted = True finally: - c._stop_event.set() - for task in c._async_tasks: - task.cancel() - await asyncio.sleep(0.1) + try: + if c._server.returncode is None and not clean_exit_asserted: + await _shutdown_client(c) + finally: + c._stop_event.set() + for task in c._async_tasks: + task.cancel() + await asyncio.sleep(0.1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/agentic/test_agentic.py` around lines 533 - 597, The test can leave the spawned server running if initialize() or other code raises before the clean-exit assertion; import the helper _shutdown_client and in the finally block (before cancelling tasks) check c._server and whether c._server.returncode is None, then attempt to call await assert_server_exited_cleanly(c._server, timeout=15.0) and if that fails or the server is still running call await _shutdown_client(c) to forcibly stop the server (reference CliceClient, c._server, initialize, assert_server_exited_cleanly, and _shutdown_client to locate changes).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/integration/agentic/test_agentic.py`:
- Around line 533-597: The test can leave the spawned server running if
initialize() or other code raises before the clean-exit assertion; import the
helper _shutdown_client and in the finally block (before cancelling tasks) check
c._server and whether c._server.returncode is None, then attempt to call await
assert_server_exited_cleanly(c._server, timeout=15.0) and if that fails or the
server is still running call await _shutdown_client(c) to forcibly stop the
server (reference CliceClient, c._server, initialize,
assert_server_exited_cleanly, and _shutdown_client to locate changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bcf29e7-2d75-4016-a417-7276787aaf62
📒 Files selected for processing (6)
src/server/service/agent_client.cppsrc/server/service/lsp_client.cppsrc/server/service/master_server.cppsrc/server/worker/worker_pool.cpptests/conftest.pytests/integration/agentic/test_agentic.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/server/service/master_server.cpp (1)
432-435: ⚡ Quick winRedundant peer closure handlers may cause double-close.
Lines 432-435 schedule a coroutine that closes
lsp_peeron shutdown. The new code at lines 460-464 definesclose_peer_on_shutdownwhich does the exact same thing, and it's invoked at lines 469 and 473 withinwhen_all. Both will execute on shutdown, callingpeer.close()twice.Additionally, context from
lsp_client.cppshowsLSPClientalready callspeer.close()on exit notification, so there could be threeclose()calls.Consider removing the pre-existing handler at lines 432-435 since the new
when_allorchestration already handles shutdown-triggered closure.Proposed fix
kota::ipc::JsonPeer lsp_peer(loop, std::move(final_transport)); LSPClient lsp_client(server, lsp_peer); - loop.schedule([](MasterServer& server, kota::ipc::JsonPeer& peer) -> kota::task<> { - co_await server.get_shutdown_event().wait(); - peer.close(); - }(server, lsp_peer)); kota::tcp::acceptor agent_acceptor;Also applies to: 460-464
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/service/master_server.cpp` around lines 432 - 435, Remove the redundant shutdown closure: delete the earlier loop.schedule(...) lambda that calls peer.close() (the anonymous coroutine that captures MasterServer& and kota::ipc::JsonPeer& and awaits server.get_shutdown_event().wait()), and rely on the new close_peer_on_shutdown helper used inside when_all; ensure only close_peer_on_shutdown (and LSPClient's exit path) perform peer.close() to avoid double-close.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/server/service/master_server.cpp`:
- Around line 432-435: Remove the redundant shutdown closure: delete the earlier
loop.schedule(...) lambda that calls peer.close() (the anonymous coroutine that
captures MasterServer& and kota::ipc::JsonPeer& and awaits
server.get_shutdown_event().wait()), and rely on the new close_peer_on_shutdown
helper used inside when_all; ensure only close_peer_on_shutdown (and LSPClient's
exit path) perform peer.close() to avoid double-close.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8ef350d-5d3e-40c8-80cd-5e9b379de160
📒 Files selected for processing (1)
src/server/service/master_server.cpp
Replace explicit peer.close()/acceptor.stop() shutdown handlers with when_any-based cancellation propagation, fix indexer monitor_resources task tracking with dedicated task_group, and bump kotatsu. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update kotatsu to ea7d99b which fixes cancel propagation for reentrantly-cancelled tasks (#158, #160). Use when_any-based structured shutdown so all tasks are properly cancelled before cleanup, avoiding ASAN use-after-free on exit. Add close_peer_on_shutdown workaround for pipe mode: the sentinel prevents cancel from reaching pending I/O when shutdown fires inline from peer.run()'s exit handler.
kotatsu 5e059a2 defers sync primitive resumes to the event loop idle tick, eliminating inline reentrancy. cancel() now propagates cleanly through when_any to the transport io_op, so the close_peer_on_shutdown workaround is no longer needed.
- Fix lifecycle state: schedule_shutdown sets ShuttingDown (not Exited), shutdown_and_cleanup sets Exited after cleanup completes. Guard only checks Exited to avoid blocking the exit notification when the LSP shutdown request already set ShuttingDown. - Restore connection cleanup: erase Connection from list when peer disconnects to prevent unbounded accumulation. - Wrap file_watcher_task in resilient_file_watcher so watcher creation failure doesn't become a when_any winner that shuts down the daemon. - Remove unused `this` capture in agent_client shutdown handler.
Picks up stabilized cancellation handling, grant abandonment for mutex/semaphore, and immediate drain of deferred resumes after the outermost coroutine resume returns.
16bit-ykiko
force-pushed
the
fix/server-shutdown-asan
branch
from
June 8, 2026 14:32
65a5a39 to
549f208
Compare
16bit-ykiko
changed the title
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/service/master_server.cpp`:
- Around line 339-345: The current run_connection coroutine erases the
Connection but never clears the LSP registration state, causing lsp_registered
to remain true after the owning client disconnects; before calling
connections.erase(pos) in run_connection (and the similar cleanup block at
353-389) check the Connection at *pos for its LSP ownership (e.g., a boolean
like lsp_registered or a non-null LSP client member) and if set, clear the
global/manager flag and reset/unregister the LSP client slot (e.g., null out the
LSPClient pointer or call the Connection/LSP manager unregister method) so
subsequent connections can acquire the LSP slot.
🪄 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: 3a942816-9992-47fb-8926-99dcb731ab92
📒 Files selected for processing (2)
cmake/package.cmakesrc/server/service/master_server.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/service/master_server.cpp`:
- Around line 339-345: The current run_connection coroutine erases the
Connection but never clears the LSP registration state, causing lsp_registered
to remain true after the owning client disconnects; before calling
connections.erase(pos) in run_connection (and the similar cleanup block at
353-389) check the Connection at *pos for its LSP ownership (e.g., a boolean
like lsp_registered or a non-null LSP client member) and if set, clear the
global/manager flag and reset/unregister the LSP client slot (e.g., null out the
LSPClient pointer or call the Connection/LSP manager unregister method) so
subsequent connections can acquire the LSP slot.
🪄 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: 3a942816-9992-47fb-8926-99dcb731ab92
📒 Files selected for processing (2)
cmake/package.cmakesrc/server/service/master_server.cpp
🛑 Comments failed to post (1)
src/server/service/master_server.cpp (1)
339-345:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset the LSP slot when the owning connection disconnects.
lsp_registeredflips totruefor the first accepted socket client and is never cleared when thatConnectionis erased. After that client disconnects, every later connection is built without anLSPClient, so editor reconnects lose all LSP handlers until the server restarts.Suggested fix
static kota::task<> run_connection(kota::ipc::JsonPeer* peer, std::list<Connection>& connections, - std::list<Connection>::iterator pos) { + std::list<Connection>::iterator pos, + bool& lsp_registered) { co_await peer->run(); LOG_INFO("Client disconnected"); + if(pos->lsp_client) { + lsp_registered = false; + } connections.erase(pos); } ... - group.spawn(run_connection(peer_ptr, connections, it)); + group.spawn(run_connection(peer_ptr, connections, it, lsp_registered));Also applies to: 353-389
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/service/master_server.cpp` around lines 339 - 345, The current run_connection coroutine erases the Connection but never clears the LSP registration state, causing lsp_registered to remain true after the owning client disconnects; before calling connections.erase(pos) in run_connection (and the similar cleanup block at 353-389) check the Connection at *pos for its LSP ownership (e.g., a boolean like lsp_registered or a non-null LSP client member) and if set, clear the global/manager flag and reset/unregister the LSP client slot (e.g., null out the LSPClient pointer or call the Connection/LSP manager unregister method) so subsequent connections can acquire the LSP slot.
Revert indexer monitor task to bg_tasks.spawn() to prevent UAF when background indexing is cancelled, and improve test_rpc_shutdown to check for sanitizer errors via assert_server_exited_cleanly().
Member
Author
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/agentic/test_agentic.py`:
- Around line 448-451: The cleanup (c._stop_event.set() and cancelling
c._async_tasks) must be executed regardless of assertion failures from
assert_server_exited_cleanly; wrap the await
assert_server_exited_cleanly(c._server) call in a try/finally and move
c._stop_event.set() and the loop that cancels each task in c._async_tasks into
the finally block (mirror the pattern used in test_shutdown_during_indexing) so
background tasks are always stopped even if the assertion raises.
🪄 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: af953f98-94e2-4c5f-a5b7-d220623e99c7
📒 Files selected for processing (1)
tests/integration/agentic/test_agentic.py
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The 3s default was too tight for CI environments, especially Debug builds where indexer/compiler cleanup takes longer. Bumped to 10s.
16bit-ykiko
added a commit
that referenced
this pull request
Jun 8, 2026
## Summary - **Structured shutdown via `when_any`**: All server modes (pipe, socket, daemon) now use `when_any` to race the main transport loop against `shutdown_event.wait()`. When shutdown is signaled, `when_any` cancels all sibling tasks — including pending I/O — then runs `shutdown_and_cleanup()` sequentially. This replaces the old pattern of fire-and-forget `loop.schedule()` + `loop.stop()`. - **Bump kotatsu to 2a8c147 (deferred sync resume)**: kotatsu's sync primitives (`event`, `mutex`, `semaphore`, `cv`) now defer waiter resumes to the event loop instead of resuming inline. This eliminates the reentrancy that previously required a sentinel workaround (`child == self`) and made cancel unable to propagate through a task whose coroutine frame was still on the call stack. - **Sanitizer-clean exit enforcement in tests**: `conftest.py` now checks every server exit for non-zero return code and sanitizer output (`AddressSanitizer`, `LeakSanitizer`, etc.) via `assert_server_exited_cleanly()`. Any sanitizer finding or unclean exit fails the test. - **Fix indexer monitor task lifetime**: `run_background_indexing` previously spawned the resource monitor into `bg_tasks` (a class member). Now uses a local `task_group` that is joined before returning, ensuring the monitor is fully stopped before the indexer reports completion. - **Fix worker_pool dangling reference**: `monitor_worker` held a reference to `workers[index]` across `co_await proc.wait()`, but the vector could reallocate during the wait. The reference is now taken after the await. ## Design decisions - **`resilient_file_watcher` wrapper (daemon mode)**: `file_watcher_task()` co_returns on creation failure. Without wrapping, this would become the `when_any` winner and trigger daemon shutdown. The wrapper suspends on `shutdown_event.wait()` after watcher exit, so only the real shutdown signal terminates the daemon. - **`schedule_shutdown` guards on `Exited`, not `ShuttingDown`**: The LSP `shutdown` request sets lifecycle to `ShuttingDown` before the `exit` notification calls `schedule_shutdown()`. If the guard checked `ShuttingDown`, the event would never be signaled. `event.set()` is idempotent, so repeated calls are safe. - **Accept loop wrapped in `task_group.spawn`**: The accept loop is spawned as a child of a `task_group` rather than running directly in `accept_connections`. This ensures `when_any` cancellation propagates through the task_group to both the accept loop and all active connection tasks. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * More reliable shutdown with staged cleanup and persisted index/cache * Resilient file-watching that reliably reloads workspace changes * **Improvements** * Reworked connection handling with per-connection tasks and a single lazy LSP client * Coordinated server modes and acceptor behavior; optional daemon file-watching * More predictable worker pool startup and monitoring * **Tests** * Stronger test assertions enforcing clean server exit and stderr checks <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Jun 12, 2026
1 task done
3 tasks done
This was referenced Jul 9, 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
when_any: All server modes (pipe, socket, daemon) now usewhen_anyto race the main transport loop againstshutdown_event.wait(). When shutdown is signaled,when_anycancels all sibling tasks — including pending I/O — then runsshutdown_and_cleanup()sequentially. This replaces the old pattern of fire-and-forgetloop.schedule()+loop.stop().event,mutex,semaphore,cv) now defer waiter resumes to the event loop instead of resuming inline. This eliminates the reentrancy that previously required a sentinel workaround (child == self) and made cancel unable to propagate through a task whose coroutine frame was still on the call stack.conftest.pynow checks every server exit for non-zero return code and sanitizer output (AddressSanitizer,LeakSanitizer, etc.) viaassert_server_exited_cleanly(). Any sanitizer finding or unclean exit fails the test.run_background_indexingpreviously spawned the resource monitor intobg_tasks(a class member). Now uses a localtask_groupthat is joined before returning, ensuring the monitor is fully stopped before the indexer reports completion.monitor_workerheld a reference toworkers[index]acrossco_await proc.wait(), but the vector could reallocate during the wait. The reference is now taken after the await.Design decisions
resilient_file_watcherwrapper (daemon mode):file_watcher_task()co_returns on creation failure. Without wrapping, this would become thewhen_anywinner and trigger daemon shutdown. The wrapper suspends onshutdown_event.wait()after watcher exit, so only the real shutdown signal terminates the daemon.schedule_shutdownguards onExited, notShuttingDown: The LSPshutdownrequest sets lifecycle toShuttingDownbefore theexitnotification callsschedule_shutdown(). If the guard checkedShuttingDown, the event would never be signaled.event.set()is idempotent, so repeated calls are safe.task_group.spawn: The accept loop is spawned as a child of atask_grouprather than running directly inaccept_connections. This ensureswhen_anycancellation propagates through the task_group to both the accept loop and all active connection tasks.Summary by CodeRabbit
Bug Fixes
Improvements
Tests