Conversation
|
@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.
Astro-Han
left a comment
There was a problem hiding this comment.
I reviewed this head and found no blocking issues. No P0-P2.
Checks on 056f55f are test: success.
简体中文
该头未发现阻断问题。2fe8c05 to
16bd288
Compare
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
16bd288 to
cebca11
Compare
|
Rebased onto current Six files conflicted. How each was resolved:
Verified locally (npm workspaces, not pnpm/turbo, so
I also checked the ordering assertion is not vacuous: swapping the two awaits in the built runner so the barrier precedes dispatch makes @yunaremaia both of your non-blocking points resolved themselves against current
@Astro-Han this is the rebase you asked contributors to do themselves; it is on current |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving the TUI onto the shared atomic interrupt authority and for making cancellation visible immediately. Two concurrency/recovery details still appear to leave the acceptance criteria partially open. This is an AI-assisted review; I independently traced the exact TUI, connection scheduler, Host admission gate, and current tests. These are suggestions from an outside perspective, so please do push back if I missed a stronger queue or draft-recovery invariant.
| // Ordering is still exact, because the authority serializes against its | ||
| // own fence: an enqueue that committed before the fence comes back in | ||
| // `retracted`, and one that lost the race rejects and restores its own | ||
| // text through the enqueue catch — each message survives exactly once. |
There was a problem hiding this comment.
Thanks for spelling out the exactly-once intent. The “lost the race” half does not currently restore the text to the composer: submitMessage() clears the editor before enqueue, and its rejection handler only removes the transient row and leaves the text in hidden editor history. If turn.interrupt commits its fence first and the subsequent submit returns session_busy, the user’s queued message disappears from the active draft; they must know to recover it from history. Could that catch explicitly restore this message (while still avoiding duplication when the Host retracts it) and cover the fence-wins race? I would rate this P2 because history offers manual recovery, but it does not yet meet the issue’s visible exactly-once preservation criterion. Please push back if hidden history is the intended contract.
| // One Host operation commits the stop fence, retracts, and aborts the turn. | ||
| // `interruptId` keys it, so a repeated gesture whose response was lost | ||
| // replays the same outcome instead of aborting anything a second time. | ||
| const result = await this.#request('turn.interrupt', { |
There was a problem hiding this comment.
Thanks for using the atomic Host operation here. It still does not appear to have end-to-end control-plane priority. RuntimeHostConnection classifies every operation except host.status as a domain request and places it behind the shared 8-in-flight FIFO, so eight delayed domain calls can keep this frame client-side. Once it reaches the Host, turn.interrupt also enters the same per-Session FIFO SessionAdmissionGate as turn.message.submit; a submit blocked in preparation/storage keeps the stop fence behind it. Thus a supported delayed enqueue can still prevent cancellation from being processed even though the runner dispatches this Promise first. I believe that remains P1 against #3698’s core cancellation guarantee. Could the control operation have a real priority lane/reservation while preserving the atomic before-or-after message outcome, with a production connection + delayed Host submit regression? Please push back if those queues have a hard bound I missed.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the rebase — the branch is clean and test is green. The conflict resolution looks right.
But the rebase doesn't touch either of the two findings from the 08-30 review, and I want to be explicit rather than let them sit implied. I re-verified both against current main just now, since main has moved a lot since then.
P1 still open — turn.interrupt has no control-plane priority, end to end.
Both halves are unchanged on main:
- Client:
packages/runtime-host/src/client/connection.ts—const isDomainRequest = operation !== 'host.status'.host.statusis the only exempt operation;turn.interruptis a domain request and queues behindRUNTIME_HOST_MAX_IN_FLIGHT_DOMAIN_REQUESTS. - Host:
SessionAdmissionGateserializes per Session through#tails, a plain promise chain in#runQueued. There is no priority concept in it, so aturn.message.submitsitting in preparation or storage holds the stop fence behind it.
Dispatching the interrupt Promise first in the runner orders the call, not the delivery. So the supported delayed-enqueue path can still keep cancellation from being processed, which is the guarantee #3698 is about.
The underlying shape is worth saying plainly: an interrupt queued behind a submit is semantically backwards. Interruption means not waiting for what is in front of it — if it takes its turn in the same FIFO, it is a request, not an interrupt. That is why I don't think this can be tuned; it needs the operation to be classified as control plane on both sides (the client exemption host.status already has, and an admission entry point on the Host that does not queue behind Session work).
P2 still open — a message lost to the fence race leaves the composer.
submitMessage() clears the editor before enqueue, and the rejection handler removes the transient row without restoring the text, so when turn.interrupt wins the fence and the submit comes back session_busy, the user's message is only in hidden editor history. The issue asks for visible exactly-once preservation; history is manual recovery, not that.
What I'd like from you, and either answer is fine. Push back if I've got the queueing wrong — you have context on this path I don't. Otherwise: are you taking the control-plane lane on in this PR, or would you rather land what you have and let #3698 stay open? If it's the latter, say so in the PR description — what this change does deliver (the TUI on the shared atomic interrupt authority, cancellation visible immediately) is real and worth having, but the description currently reads as if the queue barrier problem is solved, and it isn't yet.
me2seeks
left a comment
There was a problem hiding this comment.
Automated review (Command Code) — not an approval
Moving the abort ahead of the queue barrier is the right direction, and routing the interrupt through the host's turn.interrupt (rather than composing retract + stop) is the right authority. But the ordering argument rests on a restore that does not exist.
P1 (Must-Fix) — a mid-turn message that loses the race against the stop fence is dropped, not restored.
The new order dispatches the interrupt first and settles pending enqueues afterwards. The intent is that a submit which loses the race is returned to the user:
"one that lost the race rejects and restores its own text through the enqueue catch — each message survives exactly once."
The catch does not restore anything:
// packages/cli/src/pi-tui-runner.ts:1132-1137
.catch((error) => {
// The Message never became anything, so its row goes with the failure
// notice that replaces it. The text stays in editor history for a retry.
removeTransientUserMessage(messageId);
reportError(error);
})It removes the transient row and reports an error. There is no draft refill and no re-queue, so the text survives only in input history — not as the message the user typed.
Failure mode: turn.interrupt commits the queue stop fence, after which a turn.message.submit that reaches the coordinator while the turn is still active returns session_busy ("Message admission is closed for the active generation"). That rejection is re-thrown to the runner and lands in the catch above: row removed, error shown, text not restored. The window is not narrow — it spans fence-commit to terminal, i.e. the whole abort convergence. A submit issued before the gesture but delayed by transport or admission — the exact scenario this PR exists to fix — is therefore discarded. On main this path was unreachable because settlePendingEnqueues() ran before the queue mutation; the reordering makes it reachable.
Smallest sound fix: on that rejection, while the interrupt convergence is active, restore the draft before removing the row (if (interruptRequested) restoreDraft(text);) — the message was never retracted, so this still preserves exactly-once. Alternatively have turn.interrupt include not-yet-admitted in-flight submissions in retracted.
Related variant, same root: if the delayed submit lands after the turn reaches terminal, it starts a successor turn from the cancelled text instead of returning it for re-editing — the same guarantee violated the other way.
P2 (Should-Fix) — the refill is still serialized behind the barrier the PR claims to bypass.
interruptTurnThroughDriver() now returns the retracted queue before settlePendingEnqueues(), but acceptRetraction(retracted) — which retires the transient rows and refills the editor — still runs after awaiting the settlement. If a pending submit never settles (exactly what the new test's driver models), the host has retracted the queue but the user's editor is never refilled and the queued rows are never retired. The abort moved ahead of the barrier; the refill did not. Call acceptRetraction immediately after the authority returns — nothing about the refill depends on enqueue settlement, since rows are appended synchronously at submit time.
P2 (Should-Fix) — cancel during the turn.start window degrades to retract-only and never aborts.
interruptTurn reads the session snapshot and, when there is no live root turn, silently falls back to retractQueued(). The runner sets its turn-running state before the prompt is prepared, while snapshot.rootTurn is fed by subscription frames rather than the turn.start response — so an interrupt recognized in that window sees an absent or stale turn, never aborts, and leaves interruptRequested latched: submits stay disabled and the strip stays on "Cancelling…" for the rest of a turn that is still running. The reordering makes this worse, because there is now no await between the gesture and the snapshot read, so the frame can never catch up. Either wait for the prepared turn identity and retry the interrupt, or do not latch when nothing was aborted. The timing is not verified here, but the branch itself has no test.
P3 (Nice-to-have) — the interruptId comment overstates replay protection. The id is regenerated per call and turn.interrupt is a control operation (not auto-retried), so a repeated gesture cannot replay it; the runner's latch is what prevents a double abort. Either retain the id for the duration of the convergence or correct the comment.
Review-relevant risks. This changes turn-cancellation and message-preservation behavior, so independent human review is required under CONTRIBUTING.md. No security, licensing, release or governance effect identified.
Required conclusion.
- Optimal for the actual problem? No. The reordering is correct in principle, but it relies on a restore that does not exist, leaves the refill behind the barrier, and widens a cancel-before-visible window.
- Production code that can be deleted? The
interruptTurnfallback in the runner is unreachable in-tree (the only real driver implements the method) and exists for interface compatibility and test fakes. - Low-quality tests to delete or replace?
none identifiedas deletable — but the new runner tests certify reachability, not the guarantees argued in the description. Extend them to cover the lost-race restoration, the refill under a hung enqueue, and the no-live-turn branch. - Deeper refactor required? Not large. The cancellation authority (or a single runner-side settle-and-restore step) should own "restore the text of anything not admitted", rather than leaning on a generic error catch for exactly-once queue preservation.
- Ready to merge? Not as-is — the P1 is a concrete correctness gap against the issue's own acceptance criterion.
- Residual risks / verification gaps: I did not run the suite, so the cancel-before-start timing is reasoned rather than observed.
turn.interruptkeeps a stableturnId/runIdidentity check on the host, so a cancel cannot land on a newer turn; the client's handling of that conflict rejection is untested.
Approval boundary. This is automated review; it is not an approval. Per CONTRIBUTING.md, the merge decision requires an independent human review. No approve was submitted.
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