Skip to content

fix(cli): dispatch turn cancellation ahead of the queue barrier - #3713

Open
cat0825 wants to merge 1 commit into
apache:mainfrom
cat0825:fix/3698-tui-interrupt-first
Open

fix(cli): dispatch turn cancellation ahead of the queue barrier#3713
cat0825 wants to merge 1 commit into
apache:mainfrom
cat0825:fix/3698-tui-interrupt-first

Conversation

@cat0825

@cat0825 cat0825 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

What

Double-Escape and Ctrl-C recognized the interrupt gesture but did not cancel anything until the client-side queue work had settled, and the TUI gave no sign the keypress had registered.

requestTurnInterrupt awaited settlePendingEnqueues() and retractQueued() before it ever reached driver.stop(). Those pending enqueues are turn.message.submit round trips, so anything that delays one — transport, Session admission, storage, a fallback retry — puts an unbounded wait in front of the abort. The strip meanwhile kept rendering Working… <elapsed>, so the only feedback for a recognized cancellation was the absence of change.

How

Cancellation goes out first, and the runtime owns the ordering. MakaSessionDriver gains an optional interruptTurn(): Promise<string>. RuntimeHostMakaSessionDriver implements it with the atomic turn.interrupt operation (mode control), which commits the queue stop fence, retracts, and aborts the owning turn in one call — the same authority apps/desktop already routes through (runtime-host-session-execution-ipc-main.tsruntime-host-client.ts).

Ordering stays exact because the fence, not client-side sequencing, decides each message's fate:

  • an enqueue that committed before the fence comes back in retracted and is refilled into the editor;
  • one that lost the race rejects and restores its own text through the existing enqueue catch.

Each message therefore survives exactly once, which is what the old retract-then-stop sequence was trying to buy with a client-side barrier — at the cost of the latency this issue reports. Drivers without interruptTurn() compose retractQueued() then stop() in that same order, so their semantics are unchanged.

Acceptance is visible in the tick it happens. interruptRequestedAt is stamped alongside interruptRequested and rendered as Cancelling… <elapsed>. It outranks Working… and a scheduled provider retry — the abort supersedes the retry, since the turn is no longer working towards anything the user asked for. The elapsed counter keeps a slow cleanup (a tool held through DEFAULT_PROCESS_TERMINATION_GRACE_MS) legible as progress rather than a hang. Acceptance is a local fact and deliberately does not wait on the authority: backend abort, tool cleanup, termination grace, and durable terminal publication all land after it.

Note on two changed test fakes

InterruptibleTurnDriver and SteeringTurnDriver modelled cancellation as edge-triggered — a bare resolve callback, and a turnEnded flag reset at async-generator body entry. An async generator does not run its body until the first next() call, so with the abort now dispatched earlier, stop() landed one microtask before the body ran and the release was dropped; the fake turn parked forever.

This is a fake-only artifact, not a product regression. The real driver creates its event buffer synchronously in preparePrompt (channel.eventsForTurn(turnId)), so an abort arriving before the drain pulls is still observed — level-triggered. Both fakes now arm their abort state in preparePrompt() to match. Verified by stashing the change and confirming main passes, and by reading preparePrompt/stop to confirm both versions no-op an Escape landing during turn creation.

Testing

npm --workspace maka-agent run test — 442 tests, 442 pass, 0 fail. Typecheck and biome check clean.

New coverage:

  • runtime-host-session-driver.test.tsinterruptTurn() emits exactly one turn.interrupt with originHostEpoch/sessionId/interruptId/turnId/runId and joins retracted[].content.text; no queue.retract or turn.stop accompanies it. A terminal root turn falls back to queue.retract alone.
  • pi-tui-runner.test.ts — the interrupt reaches the stop authority while an enqueue never settles; Cancelling… appears during convergence and clears after it; a driver exposing interruptTurn is used instead of composing retract and stop.
  • pi-transcript.test.ts — precedence over Working… and over a scheduled retry, including interruptElapsedMs: 0 (the common first-render case).

All three runner tests and the transcript test were negative-controlled: reverting only the ordering and the acceptance stamp makes each of them fail, so none is vacuous.

Existing tests already assert exactly-once queue preservation across an interrupt ('double-Escape interrupt refills the editor with the cleared queue', 'interrupt refills only messages still queued, not steering already consumed', 'interrupt refills CLI-held fallback text into the editor', 'input during the interrupt convergence window stays in the editor and opens no turn') and still pass, so that criterion is not duplicated here.

Relationship to #3633

#3633 refactors this same interrupt path but keeps the current ordering, adds no turn.interrupt routing, and adds no cancellation state to the activity strip — so it does not fix this issue. This PR is based on main and does not depend on it.

If #3633 lands first, the rebase is mechanical: it removes state.pendingFallback/takePendingFallbackSettled(), so the fallback term drops out of the refill and the body becomes refillEditorFromQueues(retracted). It does not touch renderMakaPiActivityStrip, session-driver.ts, or runtime-host-session-driver.ts. Happy to rebase in whichever order maintainers prefer.

Out of scope

  • Splitting acceptance from terminal convergence in the host protocol (issue item 3, conditional) — the atomic turn.interrupt already makes acceptance authoritative from the client's side, so no protocol change was needed for this fix.
  • Keybinding discoverability (tracked in proposal(cli): make Steer, Queue and Interrupt easier to reach from the composer #3538).
  • Shortening the SIGTERM grace — the counter now makes it visible; whether 2s is the right value is a separate call.

Fixes #3698

Double-Escape and Ctrl-C recognized the interrupt gesture but did not
cancel anything until the client-side queue work had settled. The
interrupt path awaited `settlePendingEnqueues()` and `retractQueued()`
before it ever reached `driver.stop()`, so a pending
`turn.message.submit` round trip that hung on transport, Session
admission, storage, or a fallback retry put an unbounded wait in front of
the abort. The TUI meanwhile kept rendering `Working…`, leaving the user
with no evidence the keypress had registered.

Reverse the order and give the runtime the authority. `MakaSessionDriver`
gains an optional `interruptTurn()`; the Runtime Host driver implements
it with the atomic `turn.interrupt` operation, which commits the queue
stop fence, retracts, and aborts the owning turn as one control-mode
call. Cancellation now goes out first, and ordering is still exact
because the fence — not client-side sequencing — decides each message's
fate: an enqueue that committed before the fence returns in `retracted`,
and one that lost the race rejects and restores its own text through the
existing enqueue catch. Drivers without `interruptTurn()` compose
`retractQueued()` then `stop()`, preserving today's semantics.

Acceptance is also now visible immediately. `interruptRequestedAt` is
stamped in the same tick as gesture recognition and rendered by the
activity strip as `Cancelling… <elapsed>`, which outranks both `Working…`
and a scheduled provider retry; the counter keeps a slow cleanup, such as
a tool held through its process termination grace, legible as progress
rather than a hang.

Two existing runner fakes modelled cancellation as edge-triggered — a
bare `resolve` callback, and a flag reset at async-generator body entry —
so an abort landing before the drain pulled its first event was dropped.
An async generator does not run its body until the first `next()` call,
which the reordering exposed. The real Host channel buffers durable
events from `eventsForTurn()` at turn creation, so it is level-triggered;
the fakes now arm their abort state in `preparePrompt()` to match.

Fixes apache#3698
@cat0825

cat0825 commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han a review here would be appreciated whenever you have bandwidth.

Status: CI is green and GitHub reports it as mergeable against current main. The fix dispatches turn cancellation ahead of the queue barrier in the CLI session driver, so a cancel no longer waits behind queued work. Most of the +497 is test coverage across pi-transcript, pi-tui-runner, and runtime-host-session-driver.

Since you have been the main reviewer on packages/cli lately, you are probably the right person for the cancellation-ordering semantics. Glad to adjust the approach if you would rather see the barrier handled differently.

@yunaremaia yunaremaia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against #3698's acceptance criteria, with an independent local run of all three touched suites (applied the head diff onto main, built maka-agent, node --test): pi-tui-runner 135/135, pi-transcript 78/78, runtime-host-session-driver 52/52 - all green, plus CI success on the head SHA.

What I verified maps to the criteria

  1. Never-settling enqueue cannot block dispatch - StuckEnqueueDriver implements the issue's own recipe (steer RPC that never settles) and asserts the stop authority is reached and the turn converges.
  2. Same-tick acknowledgement without faking completion - SlowStopDriver asserts Cancelling… renders while progressStates is still true, then flips only after real convergence; the strip also keeps elapsed time legible during a slow grace.
  3. Single authority instead of composed calls - both levels covered: InterruptAuthorityDriver proves the runner stops composing retractQueued+stop, and the driver-level test asserts exactly one turn.interrupt request (with originHostEpoch/ids) and zero queue.retract/turn.stop. The terminal-turn fallback (retract alone, queue may still hold entries) is tested too.
  4. The activity-strip precedence tests pin the ordering I'd otherwise worry about: cancellation outranks a scheduled provider retry, and zero elapsed renders Cancelling… 0s rather than falling back to Working….
  5. Nice catch beyond the issue: making the test drivers' abort level-triggered (a stop landing before the first event pull is still observed) removes a latent race from the harness itself.

Two questions, neither blocking

  1. Fallback text after the fence. The old sequence took takePendingFallbackSettled() before stop(); now the turn is already aborted when it runs. If a fallback retry was pending at gesture time, does it settle promptly post-abort so the refill still happens? StuckEnqueueDriver covers a stuck steer, but not a pending fallback racing the interrupt. Either a test with a pending-fallback driver or a sentence on why existing DeferredRetryDriver coverage implies this would close the gap for me.
  2. Authority RPC failure mid-flight. In the catch, UI state resets and submit re-enables, but if turn.interrupt errors after the Host fence committed (e.g. response lost), the client believes nothing happened while the queue is fenced. A repeated gesture gets a fresh interruptId - is the operation idempotent enough that this self-heals? Worth a line in the driver docs if so.

Both are documentation/test-completeness items; the ordering fix itself looks correct and well-tested. Thanks especially for keeping the graceful-process-cleanup concern out of scope per the issue's item 5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cli): TUI interrupt waits behind queue RPCs and terminal cleanup while still rendering Working

2 participants