Repository navigation
perf(server): prompt shutdown and parallel integration tests - #498
Merged
Merged
Conversation
WorkerPool::stop() joined monitor_memory()'s bare 3s kota::sleep, so every shutdown (and each of the 262 integration-test teardowns) idled for up to 3 seconds. Express shutdown as cancellation instead: - WorkerPool: one stop_scope cancellation_source; monitor_memory is spawned via with_token and unwinds at its sleep. Merge the monitor and io groups into worker_tasks (same teardown verb: join, never cancel, so worker exits are observed and stderr drained to EOF). - MasterServer: replace the shutdown kota::event with a cancellation_source; run_serve_mode bounds its transport tasks with with_token(..., shutdown_token()). - Drop the dead shutting_down guards only reachable from the monitor. Integration suite: 950s -> 200s. Editor exit no longer stalls 3s.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMasterServer now uses cancellation tokens for serve-mode shutdown. WorkerPool unifies coroutine ownership under a cancellable task group with straggler escalation, while tests gain prompt-stop coverage and xdist-safe parallel integration infrastructure. ChangesCancellation-based shutdown refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant MasterServer
participant run_serve_mode
participant JsonPeer
participant Acceptor
Caller->>MasterServer: schedule_shutdown()
MasterServer->>MasterServer: shutdown_source.cancel()
run_serve_mode->>MasterServer: shutdown_token()
run_serve_mode->>JsonPeer: run under cancellation token
run_serve_mode->>Acceptor: accept_connections under cancellation token
MasterServer-->>JsonPeer: cancellation
MasterServer-->>Acceptor: cancellation
sequenceDiagram
participant Caller
participant WorkerPool
participant Monitor
participant worker_tasks
Caller->>WorkerPool: stop()
WorkerPool->>WorkerPool: stop_scope.cancel()
WorkerPool-->>Monitor: cancellation
WorkerPool->>worker_tasks: join()
WorkerPool->>WorkerPool: kill_stragglers()
worker_tasks-->>WorkerPool: all coroutines complete
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/worker/worker_pool.cpp (1)
188-195: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a hard-stop fallback in
stop()
stop()sends onlySIGTERMand then waits onworker_tasks.join(). If a worker ignoresSIGTERMor wedges before exit, shutdown can block forever. Add a bounded grace period withSIGKILLescalation or timeout the join.🤖 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/worker/worker_pool.cpp` around lines 188 - 195, `WorkerPool::stop()` only sends SIGTERM and then waits indefinitely on `worker_tasks.join()`, so add a bounded shutdown path here. After signaling `stateless_workers` and `stateful_workers`, wait for a short grace period, then escalate any still-alive workers with SIGKILL before completing the join; alternatively, make the join time out and force termination. Keep the fix localized to `WorkerPool::stop()` and use the existing `w.alive`, `w.proc.kill(...)`, and `worker_tasks.join()` flow.
🤖 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 `@src/server/worker/worker_pool.cpp`:
- Around line 188-195: `WorkerPool::stop()` only sends SIGTERM and then waits
indefinitely on `worker_tasks.join()`, so add a bounded shutdown path here.
After signaling `stateless_workers` and `stateful_workers`, wait for a short
grace period, then escalate any still-alive workers with SIGKILL before
completing the join; alternatively, make the join time out and force
termination. Keep the fix localized to `WorkerPool::stop()` and use the existing
`w.alive`, `w.proc.kill(...)`, and `worker_tasks.join()` flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 044c7709-4d2a-415c-b9c5-6beda66138f8
📒 Files selected for processing (5)
src/server/transport/master_server.cppsrc/server/transport/master_server.hsrc/server/worker/worker_pool.cppsrc/server/worker/worker_pool.htests/unit/server/worker_pool_tests.cpp
Integration suites now finish in 215-970s (was 950-1400s), so lower the step timeouts to catch a reintroduced per-test shutdown stall: native integration 25->20min (macos Debug still needs 16min — its residual slowness is a follow-up), cross integration 25->15min, smoke 15/10->5min (actual ~50s).
Tests each spawn their own server, so they parallelize cleanly: -n auto with --dist loadgroup, pinning tests that share a @workspace directory (which they mutate) to one worker via an auto-assigned xdist_group mark. CDB generation moves to the controller-side pytest_configure so workers don't race writing compile_commands.json. Also default tests to one worker of each kind (5 -> 3 processes per test) — per-test process spawn is the dominant integration-suite cost on macOS Debug (~2.3s/test constant, dyld + code-sign of large Debug binaries). Local suite: 196s serial -> 40s (-n auto), Debug+ASAN at -n 4: 127s.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/conftest.py (1)
181-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared init_options builder to avoid duplication.
The
clientandagenticfixtures duplicate the same init_options/project defaulting logic (cache_dir, worker counts). Theclientfixture includes an explanatory comment for the worker count rationale, but theagenticfixture omits it, risking divergence if one is updated without the other.♻️ Proposed helper extraction
+def _build_init_options(request: pytest.FixtureRequest, workspace: Path) -> dict: + """Build initialization options with test-appropriate defaults.""" + init_options_marker = request.node.get_closest_marker("init_options") + init_options = dict(init_options_marker.args[0]) if init_options_marker else {} + project = dict(init_options.get("project", {})) + # Force cache_dir into the workspace so .clice/ cleanup prevents stale PCH. + project.setdefault("cache_dir", str(workspace / ".clice")) + # One worker of each kind is enough for tests and halves the + # per-test process-spawn cost (5 -> 3 processes), which dominates + # suite time on macOS Debug. Tests needing more override via + # `@pytest.mark.init_options`. + project.setdefault("stateless_worker_count", 1) + project.setdefault("stateful_worker_count", 1) + init_options["project"] = project + return init_optionsThen in both fixtures:
- init_options_marker = request.node.get_closest_marker("init_options") - init_options = dict(init_options_marker.args[0]) if init_options_marker else {} - # Force cache_dir into the workspace so .clice/ cleanup prevents stale PCH. - project = dict(init_options.get("project", {})) - project.setdefault("cache_dir", str(workspace / ".clice")) - # One worker of each kind is enough for tests and halves the - # per-test process-spawn cost (5 -> 3 processes), which dominates - # suite time on macOS Debug. Tests needing more override via - # `@pytest.mark.init_options`. - project.setdefault("stateless_worker_count", 1) - project.setdefault("stateful_worker_count", 1) - init_options["project"] = project + init_options = _build_init_options(request, workspace) await c.initialize(workspace, initialization_options=init_options)🤖 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/conftest.py` around lines 181 - 182, Extract the duplicated init_options and project-default setup from the client and agentic fixtures into a shared helper in conftest.py, preserving the client fixture’s explanatory worker-count comment and current defaults for cache_dir, stateless_worker_count, and stateful_worker_count. Update both fixtures to call the helper so future changes remain centralized.
🤖 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 `@tests/conftest.py`:
- Around line 181-182: Extract the duplicated init_options and project-default
setup from the client and agentic fixtures into a shared helper in conftest.py,
preserving the client fixture’s explanatory worker-count comment and current
defaults for cache_dir, stateless_worker_count, and stateful_worker_count.
Update both fixtures to call the helper so future changes remain centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b8d7e1f-5bbe-4cf0-8014-6747dff568f7
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pixi.tomltests/conftest.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8dfc18a89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
status.total reports the in-flight round queue, which drains to zero when indexing completes — and the indexed_server fixture waits for exactly that. The 'total > 0' assertion only passed by racing into the middle of a round; on slow runners (macOS Debug + ASAN) the race is always lost.
stop() joins worker exit observers and pipe drains; a wedged worker that ignores SIGTERM would block that join forever. A watchdog task SIGKILLs still-alive workers after a 5s grace period; the normal path cancels it right after the join completes.
find_free_port drew from the kernel's shared bind(0) pool, so two concurrent xdist workers could grab the same port in the close-then- rebind gap; carve disjoint per-worker ranges instead. Also fold the duplicated client/agentic init_options defaulting into one helper.
Member
Author
|
Addressed all three review findings:
|
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/conftest.py`:
- Around line 107-122: Ensure build_init_options always enforces a
workspace-scoped cache directory by replacing the project cache_dir setdefault
with an unconditional assignment to workspace / ".clice"; retain or update the
comment to match this enforced behavior.
🪄 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: b00246ec-2e2d-4e14-980b-e2dbb0744732
📒 Files selected for processing (3)
src/server/worker/worker_pool.cppsrc/server/worker/worker_pool.htests/conftest.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/server/worker/worker_pool.h
- src/server/worker/worker_pool.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc4796d0da
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
16bit-ykiko
changed the title
setdefault let a test-supplied cache_dir escape the workspace and survive the .clice cleanup; every existing override passes the same workspace path anyway, so assign unconditionally to match the comment.
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
Started as a shutdown-latency fix and grew into a CI-speed overhaul. CI wall clock: 30–47 min → ~12.5 min; local integration suite: ~16 min → ~40 s; editor exit no longer stalls up to 3 s.
1. Shutdown expressed as cancellation (the core fix)
Every server shutdown idled up to 3 s:
WorkerPool::stop()joinedmonitor_memory(), which was parked on a bare 3 skota::sleepand only checked a flag after waking. That tax applied to every editor exit and every one of the 262 integration-test teardowns (~2.8 s × 262 per CI job).stop_scopecancellation source;monitor_memory()is spawned viawith_tokenand unwinds at its sleep the momentstop()fires. The monitor and IO groups merge into oneworker_tasksgroup (same teardown verb: join, never cancel — worker exits observed, stderr drained to EOF for crash/sanitizer reports). Deadshutting_downguards removed.kota::eventbecomes acancellation_source;run_serve_modebounds transport tasks withwith_token(..., shutdown_token()).StopIsPromptunit test guards the regression (stop must finish well under one poll interval).2. Parallel integration tests
pytest-xdistwith-n auto --dist loadgroup; tests sharing a@workspacedirectory are pinned to one worker via an auto-assignedxdist_groupmark (tryfirst hook). CDB generation moved to controller-sidepytest_configure.find_free_portnow draws from disjoint per-xdist-worker port ranges (bind(0)'s shared pool races across workers).test_cli_statusassertedstatus.total > 0, buttotalis the transient in-flight index queue that legitimately drains to zero.3. Regression guards
Step timeouts tightened so a reintroduced stall fails CI: native integration 25→20 min, cross integration 25→15 min, smoke 15/10→5 min.
Integration-test step times (CI)
All jobs ≤ ~10 min; total wall clock ~12.5 min.
Test plan
-n 4Debug+ASAN) / smoke 3/3 — all green locally