fix(cli): dispatch turn cancellation ahead of the queue barrier - #3713
fix(cli): dispatch turn cancellation ahead of the queue barrier#3713cat0825 wants to merge 1 commit into
Conversation
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
|
@Astro-Han a review here would be appreciated whenever you have bandwidth. Status: CI is green and GitHub reports it as mergeable against current Since you have been the main reviewer on |
yunaremaia
left a comment
There was a problem hiding this comment.
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
- Never-settling enqueue cannot block dispatch -
StuckEnqueueDriverimplements the issue's own recipe (steer RPC that never settles) and asserts the stop authority is reached and the turn converges. - Same-tick acknowledgement without faking completion -
SlowStopDriverassertsCancelling…renders whileprogressStatesis still true, then flips only after real convergence; the strip also keeps elapsed time legible during a slow grace. - Single authority instead of composed calls - both levels covered:
InterruptAuthorityDriverproves the runner stops composingretractQueued+stop, and the driver-level test asserts exactly oneturn.interruptrequest (withoriginHostEpoch/ids) and zeroqueue.retract/turn.stop. The terminal-turn fallback (retract alone, queue may still hold entries) is tested too. - The activity-strip precedence tests pin the ordering I'd otherwise worry about: cancellation outranks a scheduled provider retry, and zero elapsed renders
Cancelling… 0srather than falling back toWorking…. - 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
- Fallback text after the fence. The old sequence took
takePendingFallbackSettled()beforestop(); 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?StuckEnqueueDrivercovers a stuck steer, but not a pending fallback racing the interrupt. Either a test with a pending-fallback driver or a sentence on why existingDeferredRetryDrivercoverage implies this would close the gap for me. - Authority RPC failure mid-flight. In the
catch, UI state resets and submit re-enables, but ifturn.interrupterrors after the Host fence committed (e.g. response lost), the client believes nothing happened while the queue is fenced. A repeated gesture gets a freshinterruptId- 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.
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.
requestTurnInterruptawaitedsettlePendingEnqueues()andretractQueued()before it ever reacheddriver.stop(). Those pending enqueues areturn.message.submitround 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 renderingWorking… <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.
MakaSessionDrivergains an optionalinterruptTurn(): Promise<string>.RuntimeHostMakaSessionDriverimplements it with the atomicturn.interruptoperation (modecontrol), which commits the queue stop fence, retracts, and aborts the owning turn in one call — the same authorityapps/desktopalready routes through (runtime-host-session-execution-ipc-main.ts→runtime-host-client.ts).Ordering stays exact because the fence, not client-side sequencing, decides each message's fate:
retractedand is refilled into the editor;Each message therefore survives exactly once, which is what the old
retract-then-stopsequence was trying to buy with a client-side barrier — at the cost of the latency this issue reports. Drivers withoutinterruptTurn()composeretractQueued()thenstop()in that same order, so their semantics are unchanged.Acceptance is visible in the tick it happens.
interruptRequestedAtis stamped alongsideinterruptRequestedand rendered asCancelling… <elapsed>. It outranksWorking…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 throughDEFAULT_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
InterruptibleTurnDriverandSteeringTurnDrivermodelled cancellation as edge-triggered — a bareresolvecallback, and aturnEndedflag reset at async-generator body entry. An async generator does not run its body until the firstnext()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 inpreparePrompt()to match. Verified by stashing the change and confirmingmainpasses, and by readingpreparePrompt/stopto 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 andbiome checkclean.New coverage:
runtime-host-session-driver.test.ts—interruptTurn()emits exactly oneturn.interruptwithoriginHostEpoch/sessionId/interruptId/turnId/runIdand joinsretracted[].content.text; noqueue.retractorturn.stopaccompanies it. A terminal root turn falls back toqueue.retractalone.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 exposinginterruptTurnis used instead of composing retract and stop.pi-transcript.test.ts— precedence overWorking…and over a scheduled retry, includinginterruptElapsedMs: 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.interruptrouting, and adds no cancellation state to the activity strip — so it does not fix this issue. This PR is based onmainand does not depend on it.If #3633 lands first, the rebase is mechanical: it removes
state.pendingFallback/takePendingFallbackSettled(), so thefallbackterm drops out of the refill and the body becomesrefillEditorFromQueues(retracted). It does not touchrenderMakaPiActivityStrip,session-driver.ts, orruntime-host-session-driver.ts. Happy to rebase in whichever order maintainers prefer.Out of scope
turn.interruptalready makes acceptance authoritative from the client's side, so no protocol change was needed for this fix.Fixes #3698