(MOT-4100) fix(harness): stop button — prompt cancellation end to end#528
Merged
Conversation
provider-kimi predates #525's llm-router!: slim streaming delta frames change and still built full AssistantMessage snapshots on every delta, which no longer compiles against the new Option<AssistantMessage> contract. Mirrors the pattern already applied to the other six providers.
harness::stop set a durable abort flag that run_step only observed at
step boundaries and after generation returned — router.chat had no local
cancellation, so a stop could wait out a full generation (up to
router_timeout_ms) or an entire tool-execution phase, and repeat stops
redid the whole cascade every click.
- TurnCancels (locks.rs): per-turn level-triggered watch registry, fired
lock-free by stop.rs; keyed by turn_id so a stale fire can't cancel a
newer turn. Single-process, same caveat as SessionLocks.
- router.chat gains an abort arm: cuts the await immediately, persists
the partial as Aborted, re-fires router::abort (a stop can race ahead
of the router's stream registration and no-op), kills the held-open
trigger task.
- run_step: bail before dispatching generation when the cancel fired
during context assembly; check the signal between tool calls (the
durable write blocks on the session lock for that whole phase).
- stop.rs: idempotency guard (repeat stops become one read), immediate
"stopping" phase reason on the existing status channel, cancel signal
fired before anything that awaits.
- finalize_cancelled: durable "stopped by user." notice entry
(e_{turn}_stopped) so the transcript doesn't just end mid-thought;
argument-less tool-call blocks truncated by the cut are dropped from
the persisted partial (they render as empty cards and can never run).
The stop button gave no feedback and stayed clickable after a click:
local isStreaming cleared instantly on abort but serverWorking held the
indicator, so users clicked repeatedly (each click re-firing
harness::stop) with nothing acknowledging the stop.
- ChatView: `stopping` state — set on first stop, guards repeat clicks,
cleared when the streaming indicator drops.
- Composer: stop button disables with a spinner while stopping.
- translate: the cancelled stop-reason event carries
entryId e_{turn}_stopped, matching the harness's new durable notice so
the live event and the reconciled transcript row dedupe.
router::abort closed the relay reader and left the provider to notice on its next channel write — up to 2×PING_INTERVAL (60s) of billed upstream generation during a silent stretch (e.g. Anthropic server-buffering a large tool_use input). - chat.rs: the inflight abort closer now also fires provider::<id>::abort (detached, best-effort; providers without the function are a harmless error) using the request_id the provider already receives as resolution_key. - provider_scaffold::aborts: StreamAborts registry (request_id → live upstream cancel, level-triggered watch) + a ready-made make_abort handler for providers to register. - pump_abortable: pump with an abort arm — returns immediately on the signal even mid-silence, dropping the upstream receiver, which aborts the upstream task and its in-flight HTTP request. - PING_INTERVAL 30s → 2s: the failed-ping write remains the passive backstop when the active abort is lost; this bounds that path to ~4s. - ProviderAbortRequest/ProviderAbortResponse wire types.
Wires the scaffold's StreamAborts/pump_abortable into every provider: the stream registers its resolution_key (the router's request_id) while the upstream is live, provider::<id>::abort signals it, and the pump returns immediately — cancelling the upstream HTTP stream instead of letting it bill tokens until a write fails. Same shape everywhere: ABORT_ID/ABORT_DESC in the surface catalog, registration right after stream, resolution_key match around the live pump (plain pump when absent), schema goldens regenerated. provider-xai shares one pump_stream helper across its two stream paths. Cargo.locks pick up the already-committed llm-router 1.2.0 pin.
- The "stopping" status ack moves under the session lock, after the terminal re-check: issued lock-free it could land after a concurrent finalizer's "done" and strand the session on "working" (the locked re-read returns stopping:false without restoring). fail_turn now takes the session lock too — it was the one lock-free finalizer, the same hole by another door (its only caller runs after run_step returned, so no deadlock). - chat's pump wakeup uses notify_one (latched permit) instead of notify_waiters: a pre-fired cancel can reach the abort arm before the spawned pump polls its notified() future, and an unlatched notify is lost — pump.await would hang on next_binary until natural EOF. - The abort arm prefers final_message over the tracker partial: a stop landing between a terminal Done frame and EOF must not replace the complete assistant entry with the stale pre-Done partial.
A rejected backend.abortRun left `stopping` stuck true while the server still reported working — the reset effect waits on the streaming indicator, so the disabled button could never retry. Reset on rejection.
Review found two gaps in the provider::<id>::abort wiring, fixed uniformly across all seven providers: - The detached upstream task only observed a dropped receiver at its next send, so a silent upstream — parked in a chunk read with nothing to send — kept the HTTP stream and its billed generation alive until the next frame or the read timeout. spawn_upstream (and xai's responses variant) now races the call against Sender::closed(), so receiver drop cancels the HTTP future immediately. - The abort registry entry was created only at pump time, so an abort landing during provider setup (credential resolve/config) hit an unknown id and was lost, letting the request start after its own cancellation. The stream fn now subscribes at entry (level-triggered watch latches early aborts), skips spawning the upstream when already fired, and cleans up at a single point in the make_stream wrapper. Also marks provider::kimi::abort internal like the other six, keeping the control-plane callback out of the agent-facing catalog.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 46 skipped (no docs/).
Four for four. Nicely done. |
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis change adds end-to-end cancellation for streamed turns, from stop-button state and durable stop notices through harness cancellation, router abort propagation, provider abort endpoints, and upstream stream termination across supported providers. ChangesStream cancellation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ChatView
participant Harness
participant Router
participant Provider
participant Upstream
User->>ChatView: Press stop
ChatView->>Harness: abortRun(turn_id)
Harness->>Router: cancel chat with abort receiver
Router->>Provider: provider::<provider>::abort(request_id)
Provider->>Upstream: signal and close upstream stream
Upstream-->>Provider: stop streaming
Provider-->>Router: aborted outcome
Router-->>Harness: cancelled turn
Harness-->>ChatView: durable stopped notice
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 4
🤖 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 `@console/web/src/components/chat/ChatView.tsx`:
- Around line 1288-1297: Update the stop handler around setStopping so the
stopping guard reads the current stopping value from the callback closure
instead of using a state updater. Keep the early return when already stopping,
then perform abortRef.current?.abort(), backend.abortRun?.(sessionId), and the
failure reset in the event handler body; use setStopping only to update state.
In `@provider-anthropic/src/stream_fn.rs`:
- Around line 35-37: Register the abort watch before the first await that opens
the sink, then pass the receiver into the downstream stream-call function. Apply
this to provider-anthropic/src/stream_fn.rs (lines 35-37),
provider-kimi/src/stream_fn.rs (lines 36-38),
provider-openai-codex/src/stream_fn.rs (lines 38-40),
provider-openai/src/stream_fn.rs (lines 51-53), and
provider-zai/src/stream_fn.rs (lines 34-36); ensure the Kimi flow removes the
watch if sink opening fails.
In `@provider-llamacpp/src/stream_fn.rs`:
- Around line 15-40: Replace the manual post-run cleanup in make_stream with an
RAII Drop guard that owns the optional request_id and removes it from
StreamAborts when the async scope is exited, including cancellation while
run_stream_call is suspended. Ensure normal completion and early returns remain
safe, and avoid duplicate removal when an abort has already consumed the entry.
In `@provider-xai/src/stream_fn.rs`:
- Around line 45-49: Subscribe to the abort registry before the first await in
the stream closure, before calling open_sink, and retain the receiver for the
request lifecycle. Pass that receiver into run_stream_call and remove its
delayed aborts.watch subscription; ensure any open_sink failure removes the
registry entry before returning the error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f85b4b8c-a51c-41a0-a5d6-aceb357185f2
⛔ Files ignored due to path filters (7)
provider-anthropic/Cargo.lockis excluded by!**/*.lockprovider-kimi/Cargo.lockis excluded by!**/*.lockprovider-llamacpp/Cargo.lockis excluded by!**/*.lockprovider-openai-codex/Cargo.lockis excluded by!**/*.lockprovider-openai/Cargo.lockis excluded by!**/*.lockprovider-xai/Cargo.lockis excluded by!**/*.lockprovider-zai/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (58)
console/web/src/components/chat/ChatView.tsxconsole/web/src/components/chat/Composer.tsxconsole/web/src/lib/backend/translate.test.tsconsole/web/src/lib/backend/translate.tsharness/src/clients/router.rsharness/src/deps.rsharness/src/functions/stop.rsharness/src/locks.rsharness/src/turn_loop.rsllm-router/src/chat/chat.rsllm-router/src/provider_scaffold/aborts.rsllm-router/src/provider_scaffold/mod.rsllm-router/src/provider_scaffold/pump.rsllm-router/src/types/router.rsprovider-anthropic/src/register.rsprovider-anthropic/src/stream_fn.rsprovider-anthropic/src/surface.rsprovider-anthropic/src/upstream.rsprovider-anthropic/tests/golden/schemas/provider.anthropic.abort.jsonprovider-anthropic/tests/schemas.rsprovider-kimi/src/register.rsprovider-kimi/src/sse.rsprovider-kimi/src/stream_fn.rsprovider-kimi/src/surface.rsprovider-kimi/src/upstream.rsprovider-kimi/tests/golden/schemas/provider.kimi.abort.jsonprovider-kimi/tests/schemas.rsprovider-llamacpp/src/register.rsprovider-llamacpp/src/stream_fn.rsprovider-llamacpp/src/surface.rsprovider-llamacpp/src/upstream.rsprovider-llamacpp/tests/golden/schemas/provider.llamacpp.abort.jsonprovider-llamacpp/tests/schemas.rsprovider-openai-codex/src/register.rsprovider-openai-codex/src/stream_fn.rsprovider-openai-codex/src/surface.rsprovider-openai-codex/src/upstream.rsprovider-openai-codex/tests/golden/schemas/provider.openai-codex.abort.jsonprovider-openai-codex/tests/schemas.rsprovider-openai/src/register.rsprovider-openai/src/stream_fn.rsprovider-openai/src/surface.rsprovider-openai/src/upstream.rsprovider-openai/tests/golden/schemas/provider.openai.abort.jsonprovider-openai/tests/schemas.rsprovider-xai/src/register.rsprovider-xai/src/responses.rsprovider-xai/src/stream_fn.rsprovider-xai/src/surface.rsprovider-xai/src/upstream.rsprovider-xai/tests/golden/schemas/provider.xai.abort.jsonprovider-xai/tests/schemas.rsprovider-zai/src/register.rsprovider-zai/src/stream_fn.rsprovider-zai/src/surface.rsprovider-zai/src/upstream.rsprovider-zai/tests/golden/schemas/provider.zai.abort.jsonprovider-zai/tests/schemas.rs
React forbids side effects inside a state updater — Strict Mode double-invokes updaters in dev, which would fire abort()/abortRun twice. Dedupe on a ref instead (also synchronous, so two clicks in one frame can't both pass), keeping abort()/abortRun and the failure reset in the event-handler body.
Two review findings across all seven provider stream fns: - The abort watch was created only after `open_sink(...).await`, so a stop landing during sink setup hit an unknown request id, was consumed, and the upstream then started with a fresh untriggered watch. Register before the first await so the (level-triggered) signal latches. - Sequential `aborts.remove(rid)` after the call is skipped if the executor drops the handler future mid-await, leaking the registry entry. Replace with an RAII `AbortGuard` (StreamAborts::register) that deregisters on Drop — covering early returns and cancellation.
andersonleal
changed the title
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
2 participants
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.
Problem
Clicking stop (■) fired
harness::stop(~4ms ack) but the turn kept running for seconds — up torouter_timeout_ms(320s) worst case — with no UI feedback and a still-clickable button (users click 6+ times), no transcript record of the stop, and the provider's upstream HTTP stream kept generating billed tokens after the abort.What changed
harness — stop actually stops:
TurnCancels: per-turn, level-triggered in-process cancel registry, fired lock-free byharness::stoprouter.chatgains an abort arm: cuts the await immediately, persists the partial asAborted, re-firesrouter::abort(a stop can race ahead of the router's stream registration), kills the held-open trigger taskstopped by user.notice (e_{turn}_stopped); argument-less tool-call blocks truncated by the cut are dropped from the persisted partialconsole — feedback + dedupe:
llm-router + all 7 providers — the provider stream dies too:
router::abortfans out to a newprovider::<id>::abort(request correlation via the existingresolution_key)StreamAbortsregistry +pump_abortable: the abort cuts the pump even mid-silence; upstream spawns raceSender::closed()so the parked HTTP future cancels instantly instead of at its next sendPING_INTERVAL30s → 2s as the passive backstop when the active abort is lostAlso carries
fix(provider-kimi): emit slim (partial: None) delta frames(cherry-pick) — kimi didn't compile against the slim-delta contract on this base.A post-implementation review round (commits
fa1710bd/6e5b552e/e8dddbd5) closed the races it found: stopping-ack vs finalizer status ordering (incl. makingfail_turntake the session lock), a losable pump wakeup (notify_one), terminal-frame-vs-partial preference on abort, and the two provider gaps above.Verification
cargo clippy+ tests green on harness (230), llm-router (107), and all seven provider crates; consoletsc+ 1021 vitestharness::stoptrace per click (was 6+), turn cancels in <1s (server-driven case <100ms), spinner while stopping, durable notice survives reload,provider::anthropic::abortfires in ~300μs besiderouter::abort, remaining queued tool calls never run (verified with 3×sleep 8)Summary by CodeRabbit
New Features
Bug Fixes