fix(runtime): apply the tui-idle evidence ranking to mailbox delivery (#20578)

fix(runtime): retry a delivery that the idle gate refused

Gates delivery at the two points where each implementation commits to typing into
the pane, rather than at each caller, and parks-and-re-offers a refusal so late
idle evidence cannot strand a queued message.

Refs #6011
This commit is contained in:
Neil
2026-09-14 14:45:54 -07:00
committed by GitHub
parent d51747e4c4
commit 49fba59925
12 changed files with 311 additions and 5 deletions
@@ -138,7 +138,16 @@ export class OrcaRuntimeWithApplyTrackedPtyTitle extends OrcaRuntimeWithGetUnper
// an agent whose first live title is already idle (claude --resume at its
// prompt) then shows no transition — the row would strand, which is
// exactly #12536. Waiter semantics stay transition-only above.
if (agentStatus === 'idle' && (prevStatus !== 'idle' || !prevObservedLive)) {
// Why the title change joins the edge: a name-only frame routinely lands before the
// hook's `X ready`, and it consumes the working→idle transition. The later ready title
// is an idle→idle step, so gating delivery on `prevStatus !== 'idle'` meant the
// strongest evidence this pane will ever emit never reached delivery at all. The
// waiter branch above already re-offers on that step; the gate makes a repeat harmless.
if (
agentStatus === 'idle' &&
(prevStatus !== 'idle' || !prevObservedLive || prevLeafTitle !== recordedTitle) &&
this.checkDeliverySettledAndArmRecheck(leaf)
) {
this.deliverPendingMessagesForLeaf(leaf)
}
}
@@ -111,7 +111,8 @@ export class OrcaRuntimeWithDeliverPendingMessages extends OrcaRuntimeWithResolv
if (
currentLeaf?.ptyId === probedPtyId &&
currentLeaf.lastAgentStatus === 'idle' &&
currentLeaf.lastAgentStatusObservedLive
currentLeaf.lastAgentStatusObservedLive &&
this.checkDeliverySettledAndArmRecheck(currentLeaf)
) {
this.deliverPendingMessages(currentLeaf, {
mailboxHandle,
@@ -117,6 +117,76 @@ export class OrcaRuntimeWithResolveExitWaiters extends OrcaRuntimeWithBindPtyInc
})
}
/**
* Settled-enough-to-type check that also arms a retry when it says no.
*
* Why the retry: the wait path POLLS, so weak evidence that only becomes valid with the
* passage of time (a pane going quiet) eventually satisfies it. Delivery is edge-driven —
* a title transition, a graph sync, a new message — with no poll behind it, so a refusal
* at an edge is final unless another edge happens to arrive. A hookless Codex pane never
* emits an explicit `X ready`, so the refusal below would strand the queued message
* permanently once the pane fell quiet. One-shot timer, armed only for a leaf that
* actually refused, cleared as soon as any path delivers.
*/
protected checkDeliverySettledAndArmRecheck(leaf: { tabId: string; leafId: string }): boolean {
const leafKey = this.getLeafKey(leaf.tabId, leaf.leafId)
if (this.isAgentSettledForDelivery(leaf)) {
this.clearDeliveryRecheck(leafKey)
return true
}
this.armDeliveryRecheck(leafKey)
return false
}
protected clearDeliveryRecheck(leafKey: string): void {
const timer = this.deliveryRecheckTimersByLeafKey.get(leafKey)
if (timer) {
clearTimeout(timer)
this.deliveryRecheckTimersByLeafKey.delete(leafKey)
}
}
private armDeliveryRecheck(leafKey: string): void {
if (this.deliveryRecheckTimersByLeafKey.has(leafKey)) {
return
}
const live = this.leaves.get(leafKey)
// Why this delay: the only refusal that time alone can lift is tier 3 waiting on the
// stream to go quiet, so wake just after the window could have elapsed. A pane that is
// still producing output re-arms from its own fresher timestamp rather than spinning.
const elapsed = live?.lastOutputAt ? Date.now() - live.lastOutputAt : 0
const delay = Math.max(TUI_IDLE_QUIESCENCE_MS - elapsed, 0) + 50
const timer = setTimeout(() => {
this.deliveryRecheckTimersByLeafKey.delete(leafKey)
const current = this.leaves.get(leafKey)
if (!current) {
return
}
// Why the gate again here: delivery sites gate at the CALL, not inside
// deliverPendingMessagesForLeaf, so firing straight into it would hand the retry the
// very injection the gate exists to prevent. A pane that went busy again re-arms.
if (this.checkDeliverySettledAndArmRecheck(current)) {
this.deliverPendingMessagesForLeaf(current)
}
}, delay)
timer.unref?.()
this.deliveryRecheckTimersByLeafKey.set(leafKey, timer)
}
/**
* Whether this pane is settled enough to TYPE INTO.
*
* Why the same ranking as the wait path: mailbox delivery writes the pointer plus Enter
* into the pane, so acting on a name-only `Codex` title mid-turn injects keystrokes into
* a running agent's session. That is the #6011 mis-settlement in a path with a worse
* failure mode than a racing script. Liveness stays a separate requirement — callers
* keep their own `lastAgentStatusObservedLive` checks.
*/
protected isAgentSettledForDelivery(leaf: { tabId: string; leafId: string }): boolean {
const live = this.leaves.get(this.getLeafKey(leaf.tabId, leaf.leafId))
return live ? this.isTuiIdleSatisfiedForLeaf(live) : false
}
protected isTuiIdleSatisfiedForPty(pty: RuntimePtyWorktreeRecord): boolean {
return isTuiIdleSatisfied({
record: pty,
@@ -274,6 +274,9 @@ export class OrcaRuntimeWithRuntimeId {
return pty?.launchAgent ?? pty?.foregroundAgent ?? null
}
/** One-shot delivery retries, keyed by leaf. See checkDeliverySettledAndArmRecheck. */
protected deliveryRecheckTimersByLeafKey = new Map<string, ReturnType<typeof setTimeout>>()
protected leaves = new Map<string, RuntimeLeafRecord>()
// Why: PTY output is a per-keystroke hot path. Looking up affected leaves by
@@ -139,7 +139,11 @@ export class OrcaRuntimeWithSerializeAgentPromptSubmission extends OrcaRuntimeWi
leaf.lastAgentStatus = restoredStatus
if (restoredStatus === 'idle') {
this.resolveTuiIdleWaiters(leaf)
this.deliverPendingMessagesForLeaf(leaf)
// Why gated like every other delivery edge: a neutral-title restoration can
// reinstate `idle` from a name-only title, which is not evidence a turn ended.
if (this.checkDeliverySettledAndArmRecheck(leaf)) {
this.deliverPendingMessagesForLeaf(leaf)
}
}
}
}
@@ -203,6 +203,7 @@ export class OrcaRuntimeWithStopRequestedPtyIds extends OrcaRuntimeWithRuntimeId
getLeaf: (leafKey) => this.leaves.get(leafKey),
getLeafKey: (tabId, leafId) => this.getLeafKey(tabId, leafId),
getLiveLeafForHandle: (handle) => this.getLiveLeafForHandle(handle).leaf,
isAgentSettledForDelivery: (leaf) => this.checkDeliverySettledAndArmRecheck(leaf),
getMessageWaiters: (mailboxHandle) => this.messageWaiters.get(mailboxHandle),
getTabTitle: (tabId) => this.tabs.get(tabId)?.title,
getCliCommand: (terminalHandle) => this.getTerminalOrchestrationCliCommand(terminalHandle),
@@ -283,6 +283,7 @@ export class OrcaRuntimeWithSyncWindowGraph extends OrcaRuntimeWithAttachWindow
this._orchestrationDb &&
leaf.lastAgentStatus === 'idle' &&
leaf.lastAgentStatusObservedLive &&
this.checkDeliverySettledAndArmRecheck(leaf) &&
leaf.writable &&
(!graphWasReady ||
previousLeaf?.ptyId !== leaf.ptyId ||
@@ -22,6 +22,8 @@ export type PointerDeliveryDependencies<TWaiter extends OrchestrationMessageWait
getLeaf: (leafKey: string) => OrchestrationMailboxLeaf | undefined
getLeafKey: (tabId: string, leafId: string) => string
getLiveLeafForHandle: (handle: string) => OrchestrationMailboxLeaf
/** Whether the pane is settled enough to type the pointer plus Enter into it. */
isAgentSettledForDelivery: (leaf: OrchestrationMailboxLeaf) => boolean
getMessageWaiters: (mailboxHandle: string) => ReadonlySet<TWaiter> | undefined
getTabTitle: (tabId: string) => string | null | undefined
getCliCommand: (terminalHandle: string) => OrchestrationCliCommand
@@ -70,6 +70,16 @@ export class OrchestrationMailboxPointerDelivery<TWaiter extends OrchestrationMe
if (db.hasOutstandingMailboxDelivery?.(mailboxHandle)) {
return
}
// Why the gate lives HERE and not at each caller: this method is the single point at
// which this subsystem commits to typing the pointer into the pane, and it has four
// callers (handle delivery, post-probe redelivery, flight settle, and the notification
// coordinator's per-leaf path). Gating callers meant each new one silently bypassed the
// check; gating the commit point cannot be bypassed. Refusal parks and re-offers rather
// than dropping — `isAgentSettledForDelivery` arms the re-check.
if (!this.deps.isAgentSettledForDelivery(leaf)) {
this.parkRedelivery(mailboxHandle, options.reservedTypes)
return
}
if (leaf.ptyId) {
const deferredEnter = this.state.takeDeferredEnter(leaf.ptyId)
if (deferredEnter) {
@@ -27,6 +27,8 @@ function pointerDeps(db: OrchestrationDb, writePty: () => WriteSettlement) {
getLeaf: () => LEAF,
getLeafKey: () => 'tab-1:leaf-1',
getLiveLeafForHandle: () => LEAF,
// These cases exercise staging and Enter phases, not the idle gate; the pane is settled.
isAgentSettledForDelivery: () => true,
getMessageWaiters: () => undefined,
getTabTitle: () => null,
getCliCommand: () => 'orca' as const,
@@ -17,6 +17,19 @@ import {
type FirstPartyAgentStatus
} from './tui-idle-evidence'
import type { TuiAgent } from '../../shared/tui-agent'
/**
* Why null counts as quiet: a record with no output timestamp has produced nothing the
* RUNTIME OBSERVED since it was created. That is not the same as silence — the reachable
* case is a daemon-hosted pane whose bytes never reach the runtime, which may still be
* streaming. The trade is deliberate: "never settles" becomes "settles uncorroborated",
* the caller keeps its timeout, and delivery cannot reach this lane. Reading it as `0ms since output`
* inverted that — `0 >= quiescenceMs` is false forever, so an adopted pane that never
* emitted could not settle no matter how long the caller waited.
*/
function isQuietForQuiescence(lastOutputAt: number | null, quiescenceMs: number): boolean {
return lastOutputAt === null ? true : Date.now() - lastOutputAt >= quiescenceMs
}
import type { TerminalWaiter } from './runtime-terminal-contracts'
import type { RuntimeLeafRecord, RuntimePtyWorktreeRecord } from './runtime-terminal-state-records'
@@ -141,7 +154,7 @@ export class RuntimeTerminalIdlePolls {
if (
foreground &&
!isShellProcess(foreground) &&
(live.lastOutputAt ? Date.now() - live.lastOutputAt : 0) >= this.deps.quiescenceMs
isQuietForQuiescence(live.lastOutputAt, this.deps.quiescenceMs)
) {
this.stop(entry)
this.deps.resolve(waiter, buildTerminalWaitResult(waiter.handle, 'tui-idle', live))
@@ -206,7 +219,7 @@ export class RuntimeTerminalIdlePolls {
if (
foreground &&
!isShellProcess(foreground) &&
(pty.lastOutputAt ? Date.now() - pty.lastOutputAt : 0) >= this.deps.quiescenceMs
isQuietForQuiescence(pty.lastOutputAt, this.deps.quiescenceMs)
) {
this.stop(entry)
this.deps.resolve(waiter, buildPtyTerminalWaitResult(waiter.handle, 'tui-idle', pty))
@@ -0,0 +1,190 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { makeTuiIdleRuntime } from './tui-idle-wait-test-harness'
import type { RuntimeSyncWindowGraph } from '../../shared/runtime-types'
import type { OrcaRuntimeService } from './orca-runtime'
import type { TuiAgent } from '../../shared/tui-agent'
// Follow-ons to #6011. The evidence ranking that fixed the wait path did not reach two
// other consumers of the same signal: mailbox delivery, which TYPES INTO the pane, and
// the idle poll's quiescence gate, which read a missing output clock as "never quiet".
const WORKTREE_ID = 'repo-1::/tmp/followups'
const TAB_ID = 'c1c1c1c1-c1c1-4c1c-8c1c-c1c1c1c1c1c1'
const LEAF_ID = 'c2c2c2c2-c2c2-4c2c-8c2c-c2c2c2c2c2c2'
const PTY_ID = 'pty-followups'
const ESC = String.fromCharCode(27)
const BEL = String.fromCharCode(7)
const osc = (title: string) => `${ESC}]0;${title}${BEL}`
const agentStatus = (state: string, agentType: string) =>
`${ESC}]9999;{"state":"${state}","agentType":"${agentType}"}${BEL}`
const GRAPH: RuntimeSyncWindowGraph = {
tabs: [
{ tabId: TAB_ID, worktreeId: WORKTREE_ID, title: 'Agent', activeLeafId: LEAF_ID, layout: null }
],
leaves: [
{
tabId: TAB_ID,
worktreeId: WORKTREE_ID,
leafId: LEAF_ID,
paneRuntimeId: 1,
ptyId: PTY_ID,
paneTitle: null,
title: ''
}
]
}
async function makeRuntime(launchAgent: TuiAgent | null, foreground = 'codex') {
const runtime = makeTuiIdleRuntime({
repoPath: '/tmp/followups',
getForegroundProcess: async () => foreground
})
runtime.attachWindow(1)
runtime.syncWindowGraph(1, GRAPH)
runtime.registerPty(PTY_ID, WORKTREE_ID, null, {
tabId: TAB_ID,
leafId: LEAF_ID,
incarnationId: 'followups-inc',
...(launchAgent ? { agentLaunchAuthority: { launchToken: 'tok', launchAgent } } : {})
})
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
return { runtime, handle: terminals[0].handle }
}
/** Counts real delivery attempts. Spies on the delivery entry point, NOT on the gate
* under test — the gate runs for real and decides whether this is ever reached. */
function watchDelivery(runtime: OrcaRuntimeService) {
return vi
.spyOn(
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the delivery entry point is protected; the spy only needs its name and signature.
runtime as never as { deliverPendingMessagesForLeaf: (leaf: unknown) => void },
'deliverPendingMessagesForLeaf'
)
.mockImplementation(() => {})
}
// Why fake timers: the retry fires on a real 3s quiescence window, and asserting around it
// with wall-clock sleeps made the result depend on how promptly a loaded CI runner schedules
// an interval. The clock is the thing under test, so it has to be the deterministic part.
describe('mailbox delivery honours the tui-idle evidence ranking', () => {
beforeEach(() => {
vi.useFakeTimers()
})
afterEach(() => {
vi.useRealTimers()
})
it('does not deliver into a pane that is only showing its agent name mid-turn', async () => {
const { runtime } = await makeRuntime('codex')
const deliver = watchDelivery(runtime)
runtime.onPtyData(PTY_ID, `${osc('⠋ Codex')}working\n`, Date.now())
expect(deliver).not.toHaveBeenCalled()
// The busy agent repaints its title to the bare product name. That reads as `idle`
// for display, but it is emitted just as often mid-turn — typing into the pane here
// injects the pointer plus Enter into a running turn.
runtime.onPtyData(PTY_ID, `${osc('Codex')}still working\n`, Date.now())
expect(deliver).not.toHaveBeenCalled()
})
it('delivers once the agent states it is done', async () => {
const { runtime } = await makeRuntime('codex')
const deliver = watchDelivery(runtime)
runtime.onPtyData(PTY_ID, `${osc('⠋ Codex')}working\n`, Date.now())
runtime.onPtyData(PTY_ID, `${osc('Codex ready')}done\n`, Date.now())
expect(deliver).toHaveBeenCalled()
})
// Why this case exists: the wait path POLLS, so weak evidence that only becomes valid
// with time eventually satisfies it. Delivery is edge-driven with no poll behind it, so a
// refusal at an edge is final unless another edge arrives. A hookless Codex never emits an
// explicit `X ready`, so without a retry the queued message strands permanently once the
// pane falls quiet — trading a visible mis-delivery for an invisible lost message.
it('retries a refused delivery once the pane falls quiet', async () => {
const { runtime } = await makeRuntime('codex')
const deliver = watchDelivery(runtime)
runtime.onPtyData(PTY_ID, `${osc('\u280b Codex')}working\n`, Date.now())
runtime.onPtyData(PTY_ID, `${osc('Codex')}output\n`, Date.now())
expect(deliver).not.toHaveBeenCalled()
// Output stops. No further title frame and no renderer graph sync — a daemon-hosted
// pane has nobody publishing one, so nothing re-fires an edge on its own.
await vi.advanceTimersByTimeAsync(5_000)
expect(deliver).toHaveBeenCalled()
})
it('does not retry into a pane that went busy again', async () => {
const { runtime } = await makeRuntime('codex')
const deliver = watchDelivery(runtime)
runtime.onPtyData(PTY_ID, `${osc('Codex')}output\n`, Date.now())
// Keep the stream alive across the whole retry window.
// Deterministic streaming: one chunk every 250ms of virtual time, so the gap between
// chunks can never drift past the quiescence window the way a real interval can.
for (let tick = 0; tick < 20; tick += 1) {
runtime.onPtyData(PTY_ID, 'more output\n', Date.now())
await vi.advanceTimersByTimeAsync(250)
}
expect(deliver).not.toHaveBeenCalled()
})
// Case B, the mainline path: a hooked Codex emits a name-only frame BEFORE the hook's
// `Codex ready`. The name-only frame consumes the working->idle transition, leaving the
// ready title as an idle->idle step that delivery was never offered — so the strongest
// evidence the agent ever emits could not reach it.
it('delivers when the ready title arrives after a name-only frame', async () => {
const { runtime } = await makeRuntime('codex')
const deliver = watchDelivery(runtime)
runtime.onPtyData(PTY_ID, `${osc('\u280b Codex')}working\n`, Date.now())
runtime.onPtyData(PTY_ID, `${osc('Codex')}out\n`, Date.now())
expect(deliver).not.toHaveBeenCalled()
await vi.advanceTimersByTimeAsync(100)
runtime.onPtyData(PTY_ID, osc('Codex ready'), Date.now())
// Promptly, on the ready title itself — not after waiting out a quiescence window.
expect(deliver).toHaveBeenCalled()
})
// Case C: the agent's own status stream vetoes the idle title, then reports done with no
// edge behind it. `working` stays fresh for 30 minutes, so without a re-offer the veto
// outlives the turn it described.
it('delivers when a done status lands after the idle title was vetoed', async () => {
const { runtime } = await makeRuntime('claude')
const deliver = watchDelivery(runtime)
runtime.onPtyData(
PTY_ID,
`${agentStatus('working', 'claude')}${osc('\u280b Claude')}w\n`,
Date.now()
)
runtime.onPtyData(PTY_ID, `${osc('claude')}out\n`, Date.now())
expect(deliver).not.toHaveBeenCalled()
runtime.onPtyData(PTY_ID, agentStatus('done', 'claude'), Date.now())
await vi.advanceTimersByTimeAsync(4_500)
expect(deliver).toHaveBeenCalled()
})
it('still delivers for an agent whose name is its only rest signal', async () => {
const { runtime } = await makeRuntime('grok', 'grok')
const deliver = watchDelivery(runtime)
runtime.onPtyData(PTY_ID, `${osc('⠋ Grok')}working\n`, Date.now())
runtime.onPtyData(PTY_ID, `${osc('grok')}banner\n`, Date.now())
expect(deliver).toHaveBeenCalled()
})
})
describe('quiescence treats a missing output clock as quiet', () => {
it('settles a pane that has never produced output but holds a live agent process', async () => {
// No launch metadata: Orca did not start this agent, so the quiet-foreground lane is
// the only evidence available, and `lastOutputAt` is null because nothing ever arrived.
const { runtime, handle } = await makeRuntime(null, 'codex')
const leaves =
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: reading the runtime's own leaf map to assert the precondition this test depends on.
(runtime as never as { leaves: Map<string, { lastOutputAt: number | null }> }).leaves
expect([...leaves.values()][0].lastOutputAt).toBeNull()
await expect(
runtime.waitForTerminal(handle, { condition: 'tui-idle', timeoutMs: 8_000 })
).resolves.toMatchObject({ condition: 'tui-idle', satisfied: true })
}, 20_000)
})