feat(server): end-to-end request cancellation - #511
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change propagates LSP request cancellation through feature routing and compiler worker dispatch, interrupts superseded compilations, preserves cancellation across stateless retries, removes dependency bookkeeping, and adds unit and integration coverage. ChangesCancellation and compile supersession
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LSPClient
participant FeatureRouter
participant Compiler
participant StatefulWorker
LSPClient->>FeatureRouter: feature request with cancellation
FeatureRouter->>Compiler: forward feature with token
Compiler->>StatefulWorker: dispatch request with request_options
LSPClient-->>FeatureRouter: cancel request
StatefulWorker-->>Compiler: cancelled response
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 |
![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: 5bad4a7c52
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/unit/server/compiler_tests.cpp`:
- Around line 485-494: Update the cancellation assertions to verify the explicit
RequestCancelled result rather than merely completion or failure: in
tests/unit/server/compiler_tests.cpp lines 485-494, retain the kota::with_token
result in the cancelled_waiter flow and assert it is RequestCancelled; in
tests/unit/server/cancel_chain_tests.cpp lines 53-59 and 87-91, retain each
worker reply/error and require RequestCancelled instead of checking only
!result.has_value() or boolean flags.
🪄 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: 9bee3daa-35c8-4ab9-9f29-c44156c56ed5
📒 Files selected for processing (12)
src/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/protocol/worker.hsrc/server/service/feature_router.cppsrc/server/service/feature_router.hsrc/server/state/session.hsrc/server/transport/lsp_client.cppsrc/server/worker/stateful_worker.cpptests/integration/features/test_cancellation.pytests/unit/server/cancel_chain_tests.cpptests/unit/server/compiler_tests.cpptests/unit/server/stateful_worker_tests.cpp
codex review: the CancelCompile notification only fired when a later ensure_compiled observed the stale round; an edit with no follow-up request left the stale parse running and its waiters blocked. Single emission point Compiler::interrupt_superseded, called from didChange and the supersede point.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/server/compiler_tests.cpp (1)
418-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMake supersession interruption observable rather than time-based.
Both tests allow 60 seconds and only assert eventual completion. If
interrupt_supersededor the supersede notification becomes a no-op, the large parse can finish naturally and these tests still pass. Add deterministic notification/interruption instrumentation or a controlled blocked worker so completion requiresCancelCompile.Also applies to: 508-517
🤖 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/unit/server/compiler_tests.cpp` around lines 418 - 427, Update both supersession tests around waiter_done and waiter_ok to remove the time-based 60-second polling and use deterministic instrumentation or a controlled blocked worker. Ensure the worker cannot complete naturally, assert that supersede notification/interruption occurs, and require CancelCompile to release it before joining and validating completion.
🤖 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/unit/server/compiler_tests.cpp`:
- Around line 418-427: Update both supersession tests around waiter_done and
waiter_ok to remove the time-based 60-second polling and use deterministic
instrumentation or a controlled blocked worker. Ensure the worker cannot
complete naturally, assert that supersede notification/interruption occurs, and
require CancelCompile to release it before joining and validating completion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 771e7891-8e3e-4137-add2-ed45eab586d1
📒 Files selected for processing (5)
src/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/transport/lsp_client.cpptests/unit/server/cancel_chain_tests.cpptests/unit/server/compiler_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/server/cancel_chain_tests.cpp
- src/server/transport/lsp_client.cpp
- src/server/compiler/compiler.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17326b3ae7
ℹ️ 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".
codex round 2: an edit landing while the round is in dependency prep only sent CancelCompile (a worker no-op pre-dispatch); the module-graph waits kept the waiters blocked. abandon_superseded = interrupt + deps scope cancel, for the edit path where no replacement round follows. Also yield before the synchronous include-completion scan so a piped $/cancelRequest tears the frame before the directory walk.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/server/compiler_tests.cpp (1)
533-535: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider setting
ast_dirtyfor consistency.To fully simulate the state changes of a
didChangeevent and maintain consistency withEditInterruptsStaleCompile, consider explicitly settingsession->ast_dirty = true;here as well, even if it happens to still be true from the initial session state.💡 Proposed change
// The edit lands while the slow compile is in flight. session->text = "int fixed;\n"; session->generation += 1; + session->ast_dirty = true;🤖 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/unit/server/compiler_tests.cpp` around lines 533 - 535, Update the test session state setup near the text and generation assignments to also set session->ast_dirty = true, ensuring it fully simulates a didChange event and remains consistent with EditInterruptsStaleCompile.
🤖 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/unit/server/compiler_tests.cpp`:
- Around line 533-535: Update the test session state setup near the text and
generation assignments to also set session->ast_dirty = true, ensuring it fully
simulates a didChange event and remains consistent with
EditInterruptsStaleCompile.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4474b7e-b294-4163-9f54-2a58fa0dadc9
📒 Files selected for processing (5)
src/server/compiler/compiler.cppsrc/server/compiler/compiler.hsrc/server/service/feature_router.cppsrc/server/transport/lsp_client.cpptests/unit/server/compiler_tests.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- src/server/transport/lsp_client.cpp
- src/server/service/feature_router.cpp
- src/server/compiler/compiler.cpp
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 913273b3a7
ℹ️ 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".
codex round 3: the yield left pctx/offset computed from the pre-edit buffer; a didChange landing during the suspension made the include scan serve candidates and TextEdit ranges for text that no longer exists. Move the suspension before every buffer read: the synchronous remainder serves one consistent snapshot, and every completion path gains the early-cancel window.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/features/test_cancellation.py (1)
110-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCumulative sequential cancellation delay risks flakiness as the shared compile finishes.
Each of the 10 requests is dispatched, slept on for 0.1s, then cancelled — sequentially, one after another (Line 165-166). By the time later iterations run, ~1s+ has elapsed since the shared compile started. If clang finishes parsing the 200k-decl file within that cumulative window (e.g. a fast release build or lightly-loaded CI runner), the tail requests would get real answers instead of
RequestCancelled, since a completed shared compile answers instantly from the cached AST and is never itself client-cancelled per this PR's stated contract. This directly threatens the test's premise ("compile is still churning through the slow body") for the later entries in the loop.Consider dispatching all 10 requests concurrently and cancelling them together shortly after, so every request's exposure window starts near the beginning of the compile rather than accumulating with loop position.
♻️ Suggested refactor to reduce cumulative timing risk
- for method, params in requests: - await cancel_and_expect(client, method, params) + tasks = [] + ids = [] + for method, params in requests: + msg_id = str(uuid.uuid4()) + ids.append(msg_id) + tasks.append( + asyncio.ensure_future( + client.protocol.send_request_async(method, params, msg_id=msg_id) + ) + ) + await asyncio.sleep(0.1) + for msg_id in ids: + client.protocol.notify("$/cancelRequest", CancelParams(id=msg_id)) + for task in tasks: + with pytest.raises(Exception) as exc: + await asyncio.wait_for(task, timeout=30) + assert getattr(exc.value, "code", None) == -32800🤖 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/features/test_cancellation.py` around lines 110 - 172, Update test_cancelled_requests_while_compiling to dispatch all requests concurrently and cancel them together after the initial delay, rather than awaiting cancel_and_expect sequentially in the loop. Preserve the assertion that every request returns RequestCancelled while the shared compile remains active, then keep the final hover assertion verifying the completed AST is usable.
🤖 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/integration/features/test_cancellation.py`:
- Around line 110-172: Update test_cancelled_requests_while_compiling to
dispatch all requests concurrently and cancel them together after the initial
delay, rather than awaiting cancel_and_expect sequentially in the loop. Preserve
the assertion that every request returns RequestCancelled while the shared
compile remains active, then keep the final hover assertion verifying the
completed AST is usable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bb50a66-6d53-41f4-848d-05cfbae5ec04
📒 Files selected for processing (1)
tests/integration/features/test_cancellation.py
macOS arm64 finished the shared 200k parse mid-sweep, so the seventh cancel found a ready AST and got a normal reply. Each pulling request now edits first and launches its own parse: the cancel window is per round, not cumulative.
1 participant
Background
clice's compilation is pull-based: feature requests pull the document's AST via a shared compile round, compile rounds pull shared PCH/PCM artifacts. Until now, cancellation stopped at the master's front door — kotatsu's peer layer tore down the handler frame and replied
RequestCancelled, but the work already dispatched to worker processes always ran to completion. The expensive case is completion: editors cancel the in-flight completion on every keystroke, each one is a full stateless parse of the buffer snapshot, and — because those builds are keyed to a text snapshot rather than the document generation — the edit/supersede machinery never cancels them. The client's$/cancelRequestis the only death signal that path has. Zombie parses serialized on the stateless pool and the completion the user actually wanted queued behind them.This PR makes cancellation reach the clang parse itself, on both paths, without weakening the crash-containment invariants from #502.
What changed
FeatureRouter(12 methods) into the compiler forwards and is passed asrequest_options.tokenon the request's own worker sends. When the client cancels, the send resumes withRequestCancelledand emits a wire cancel; the worker-side handler cancellation flips the compile stop flag (CompilationParams::stop, polled after every top-level declaration), and the parse dies at the next declaration instead of running to completion.CancelCompilenotification. When an edit makes an in-flight compile stale — detected at the supersede point, or immediately ondidChange(abandon_superseded, which also cancels the stale round's module-dependency waits since no replacement round follows) — the master notifies the worker, which sets the published stop flag for that document. The request is deliberately not wire-cancelled: it runs to a normal (incomplete) reply that the master discards at its generation gate.$/cancelRequesttears the frame before the directory walk starts, and a piped edit lands before the completion context is computed, so the scan serves one consistent snapshot.Invariants
deps_scopereleases only that round's interest in the module graph.with_tokenfuses resume-with-cancel, so a wire cancel racing a worker death would returnRequestCancelledand the death would never reach the document's quarantine ledger. The notification interrupts the parse while the master still observes the request's real outcome — including a crash.CancelCompilebefore the replacement compile can enter the pipe (spawn is eager, so this ordering is load-bearing), and the worker publishes each round's stop flag on request arrival, before its strand wait. Pipe FIFO makes a mistargeted cancel impossible; a cancel landing after the round finished sets a dead flag — a no-op.Tests
Unit (7 new):
CancelChain.HandlerCancelChainsThrough(the with_token resumption boundary emits the wire cancel that interrupts a 200k-declaration parse),StatefulWorker.CancelNotificationInterruptsCompile(notification path, pinned by reply content — an interrupted parse reports no deps and no index — not wall clock),CompilerGuards.ClientCancelSparesCompile(cancelling one waiter must not kill the other waiter's result),CompilerGuards.SupersededCompileCancelled/EditInterruptsStaleCompile(supersede and edit-path liveness),CompilerGuards.AbandonCancelsDepsScope(the two supersede entry points' deps_scope contract), plus tightenedRequestCancelled-specific assertions.Integration (4, real client over LSP with a 200k-declaration file blocking compilation): cancelled completion and signature help at end-of-file positions (clang truncates the parse at the completion point, so end-of-file forces the full parse — the cancel provably lands mid-parse), all ten forwarded features cancelled while the document compile is in flight with a closing hover proving the shared compile survived every cancel, and an edit-mid-compile test proving the superseded request unblocks promptly and the next request answers on the new content.
Known limits (documented, deliberate)
Summary by CodeRabbit