From 3d8131d7ea6758f31ee7191c0a936016eaa548a1 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:22:10 -0700 Subject: [PATCH] fix(runtime): reject leaf terminal sends only on controller-proven PTY absence (#12578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(runtime): reject leaf terminal sends only on controller-proven PTY absence orca terminal send to a leaf whose ptyId no provider in this process owns was a silent no-op reported as success: the graph mirror answers writable=true, every provider write to an unknown id is accepted fire-and-forget, and bytesWritten is computed from the payload rather than delivery. sendTerminal and sendTerminalAgentPrompt now consult a controller liveness probe when the provider does not synchronously know the id (hasPty), and throw terminal_not_writable only on an exact false — unknown liveness, probe errors, SSH/remote scopes, and probe-less providers never reject (#12393's rule: null is not absence), so a restored daemon session still accepts writes before its pane remounts. Push-on-idle orchestration delivery gains the same gate so a proven-dead leaf keeps its messages queued instead of marking them delivered into a void. The pty controller now exposes probePtyLiveness, routed like write: a provider probe is preferred, the in-process local provider's refusal is authoritative (sole owner), and remote-scoped or SSH ids without a probe answer null after awaiting the cold-start daemon swap. Proven-absent verdicts cache 15s per ptyId with in-flight dedupe, superseded the moment the provider re-learns the id. * fix(runtime): arm one probe-deferred delivery continuation per pty Review (GPT verifier) confirmed: triggers arriving during one in-flight absence probe each attached a continuation to the deduped probe promise, and since Claude-target delivered_at stamps only after the delayed Enter, every continuation re-read the same unread rows — double payload injection and two armed Enters. Single-flight the deferred continuation per pty; the one armed continuation re-reads fresh rows when it fires, so nothing is lost, and the guard clears on settle so later triggers defer again. The narrower pre-existing 500ms sync-path window is unchanged and out of scope. * fix(runtime): single-flight the whole orchestration delivery window per pty The probe-continuation guard cleared at probe settle, but Claude-target delivered_at stamps only in the delayed-Enter callback ~500ms later — a trigger landing in that gap armed a fresh probe cycle, re-read the same un-stamped rows, and re-injected the payload. The identical window existed on the pure sync path pre-PR (two triggers within 500ms double-deliver). Hold a per-pty delivery-in-flight flag from before the payload write until delivery settles: entry-checked before reading unread rows, cleared through one settle point covering the failed write, the sync-stamped coordinator and Cursor branches, any sync throw, and the delayed-Enter callback on submit, refusal, and throw alike. A trigger arriving mid-flight is not dropped — it parks the latest leaf per ptyId and re-runs delivery once on settle, so rows inserted mid-flight deliver without waiting for the next idle event. The probe single-flight stays; the new guard subsumes its post-settle gap, and no trigger site bypasses it. Both strengthened tests are red on the previous commit (first subject injected twice) and green here: in-window re-trigger on the probe path and sync-path double-trigger each deliver the first batch exactly once, with the parked second row delivering alone after settle. * fix(runtime): retire the armed delivery Enter on pty exit; guard fire-time on current state Two variants of one root cause — the delayed-Enter callback outliving the session it was armed for: 1. Cold restore respawns under the same session id. onPtyExit never cancelled the armed Enter or the in-flight delivery state, and onPtySpawned flips the same leaf writable again — so an exit + same-id respawn inside the 500ms window let the stale callback inject \r into the replacement session and stamp rows it never received, then settle against a newer same-id flight. 2. Graph resync replaces leaf objects, so onPtyExit flips writable=false only on the current replacement; a callback trusting its closed-over snapshot still read writable=true and fired after exit with no respawn. The flight record now carries its armed Enter timer and serves as settle identity: onPtyExit clears the timer and drops the flight and any parked re-delivery without stamping (rows stay unstamped and re-deliver on the replacement's next idle — the existing contract), and settle no-ops unless its own flight is still current, so a stale settle can never clear a newer same-id flight or flush its parked trigger. At fire time the callback re-resolves the leaf by key and requires the same ptyId binding and current writability instead of reading the closure snapshot. All three regressions are red on the previous commit: same-id respawn saw \r plus a false delivered_at stamp, exit leaked the flight and parked state, and the orphaned-snapshot resync variant fired Enter after exit. --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- src/main/ipc/pty.test.ts | 74 +++ src/main/ipc/pty.ts | 28 + src/main/runtime/orca-runtime.ts | 254 +++++++-- .../terminal-send-stale-leaf-liveness.test.ts | 512 ++++++++++++++++++ 4 files changed, 833 insertions(+), 35 deletions(-) create mode 100644 src/main/runtime/terminal-send-stale-leaf-liveness.test.ts diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index 797cfa441..fc73725f8 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -750,12 +750,14 @@ describe('registerPtyHandlers', () => { spawn: (args: Record) => Promise write: (ptyId: string, data: string) => boolean resize: (ptyId: string, cols: number, rows: number) => boolean + probePtyLiveness: (ptyId: string) => Promise } { let controller: | { spawn: (args: Record) => Promise write: (ptyId: string, data: string) => boolean resize: (ptyId: string, cols: number, rows: number) => boolean + probePtyLiveness: (ptyId: string) => Promise } | undefined const runtime = { @@ -801,6 +803,78 @@ describe('registerPtyHandlers', () => { clearProviderPtyState(ptyId) }) + describe('controller probePtyLiveness routing', () => { + it('proves absence for an id the in-process local provider never owned', async () => { + setLocalPtyProvider(new LocalPtyProvider()) + const controller = registerAgentClaimController() + + await expect(controller.probePtyLiveness('pty-from-prior-run')).resolves.toBe(false) + }) + + it('delegates to a provider-exposed probe and preserves its answer', async () => { + const provider = { + ...createAgentClaimProvider({}), + probePtyLiveness: vi.fn(async () => true) + } + setLocalPtyProvider(provider as never) + const controller = registerAgentClaimController() + + await expect(controller.probePtyLiveness('daemon-owned')).resolves.toBe(true) + expect(provider.probePtyLiveness).toHaveBeenCalledWith('daemon-owned') + }) + + it('answers unknown for a probe-less provider that is not the in-process one', async () => { + // Why: only the in-process provider is its own sole owner; any other + // probe-less provider's ignorance is doubt, not absence. + setLocalPtyProvider(createAgentClaimProvider({}) as never) + const controller = registerAgentClaimController() + + await expect(controller.probePtyLiveness('pty-unknown')).resolves.toBeNull() + }) + + it('answers unknown for SSH-owned ids whose provider has no probe', async () => { + const connectionId = 'ssh-probe-1' + const ptyId = `ssh:${connectionId}@@remote-pty` + setLocalPtyProvider(new LocalPtyProvider()) + registerSshPtyProvider(connectionId, createAgentClaimProvider({}) as never) + setPtyOwnership(ptyId, connectionId) + const controller = registerAgentClaimController() + try { + await expect(controller.probePtyLiveness(ptyId)).resolves.toBeNull() + + unregisterSshPtyProvider(connectionId) + // A disconnected SSH provider is an error path, and errors never prove absence. + await expect(controller.probePtyLiveness(ptyId)).resolves.toBeNull() + } finally { + unregisterSshPtyProvider(connectionId) + clearPtyOwnershipForConnection(connectionId) + clearProviderPtyState(ptyId) + } + }) + + it('answers unknown for remote-scoped ids without consulting local providers', async () => { + // Why: a locally routed provider would answer confidently — and wrongly — + // for a PTY that lives on a remote Orca host. + setLocalPtyProvider(new LocalPtyProvider()) + const controller = registerAgentClaimController() + + await expect(controller.probePtyLiveness('remote:some-remote-pty')).resolves.toBeNull() + }) + + it('answers unknown when the provider probe throws', async () => { + const provider = { + ...createAgentClaimProvider({}), + probePtyLiveness: vi.fn(async () => { + throw new Error('probe transport down') + }) + } + setLocalPtyProvider(provider as never) + const controller = registerAgentClaimController() + + await expect(controller.probePtyLiveness('daemon-owned')).resolves.toBeNull() + }) + }) + it('does not dispatch a runtime PTY spawn after its client disconnects', async () => { const provider = createAgentClaimProvider({}) setLocalPtyProvider(provider as never) diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index a59f70e8a..7c20eb17e 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -5067,6 +5067,34 @@ export function registerPtyHandlers( return false } }, + probePtyLiveness: async (ptyId) => { + try { + // Why: no locally routed provider can authoritatively answer for a + // remote host's PTY, so remote-scoped ids stay unknown, never absent. + if (ptyId.startsWith('remote:')) { + return null + } + const connectionId = ptyOwnership.get(ptyId) ?? parseAppSshPtyId(ptyId)?.connectionId + // Why: during cold start the daemon swap is in flight; the pre-swap + // fallback would answer absent for every daemon-owned id. + const startupPromise = getLocalPtyProviderStartupPromise(connectionId) + if (startupPromise) { + await startupPromise + } + const provider = getProviderForPty(ptyId) + if (provider.probePtyLiveness) { + return await provider.probePtyLiveness(ptyId) + } + // Why: the in-process provider is its own sole owner (#12393), so its + // refusal is authoritative; every other probe-less provider is doubt. + if (provider instanceof LocalPtyProvider) { + return provider.hasPty(ptyId) + } + return null + } catch { + return null + } + }, kill: (ptyId) => { let connectionId: string | null | undefined = ptyOwnership.get(ptyId) const parsedSshId = connectionId === undefined ? parseAppSshPtyId(ptyId) : null diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index a1d90d2cf..d5584eae3 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1660,6 +1660,8 @@ type RuntimePtyController = { signal?: AbortSignal ): Promise getSize?(ptyId: string): { cols: number; rows: number } | null + /** False only when the owning provider proved the PTY absent; null = unknown (never a denial). */ + probePtyLiveness?(ptyId: string): Promise } type PtyControllerTerminalIdentity = Readonly<{ @@ -1721,6 +1723,10 @@ const RECENT_PTY_PATH_CANDIDATE_LIMIT = 1024 const RECENT_PTY_PATH_CANDIDATE_MAX_BYTES = 4 * 1024 const RECENT_PTY_PATH_CANDIDATE_TOTAL_BYTES = 64 * 1024 const SSH_PANE_RECOVERY_GRACE_MS = 30_000 +// Why: long enough that a keystroke burst to a proven-dead leaf probes once, +// short enough that a recreated session id regains writability quickly even if +// its runtime record (which also invalidates the verdict) is late. +const PROVEN_ABSENT_LEAF_PTY_TTL_MS = 15_000 function isClientDisconnectedError(error: unknown): boolean { return error instanceof Error && error.message === 'client_disconnected' @@ -13202,6 +13208,10 @@ export class OrcaRuntimeService { clearTimeout(pendingSoft.timer) this.pendingSoftLeavers.delete(ptyId) } + // Why: a cold restore can respawn under the same session id within the + // delayed-Enter window; the armed Enter would inject \r into the + // replacement and stamp rows it never received. + this.retirePendingMessageDeliveryForPty(ptyId) if (this.terminalFitOverrides.has(ptyId)) { this.terminalFitOverrides.delete(ptyId) @@ -16079,6 +16089,67 @@ export class OrcaRuntimeService { return visibleRead } + // Why a cache: leaf-branch sends may arrive per keystroke; one proven-absent + // verdict per ptyId serves the burst instead of a probe round-trip each call. + private readonly provenAbsentLeafPtyVerdicts = new Map() + private readonly leafPtyAbsenceProbes = new Map>() + // Why: probe dedupe shares one promise across callers, but each caller's + // continuation would re-deliver the same unread rows; arm one per pty. + private readonly probeDeferredDeliveryPtyIds = new Set() + + private controllerKnowsPtyIsLive(ptyId: string): boolean { + try { + return this.ptyController?.hasPty?.(ptyId) === true + } catch { + // Why: liveness lookup failures are doubt; doubt never gates a write. + return false + } + } + + /** True only on controller-proven absence; live, unknown, and probe errors all answer false. */ + private isLeafPtyProvenAbsent(ptyId: string): Promise { + // Why hasPty and not ptysById: graph sync mirrors a connected record for + // every leaf ptyId — including a prior process's — so runtime records can't + // distinguish live from stale. The controller's exact-id hasPty is the + // provider's own synchronous inventory: a known id is alive, skip probing + // and supersede any cached verdict (the id came back). + if (this.controllerKnowsPtyIsLive(ptyId)) { + this.provenAbsentLeafPtyVerdicts.delete(ptyId) + return Promise.resolve(false) + } + const verdictAt = this.provenAbsentLeafPtyVerdicts.get(ptyId) + if (verdictAt !== undefined) { + if (Date.now() - verdictAt < PROVEN_ABSENT_LEAF_PTY_TTL_MS) { + return Promise.resolve(true) + } + this.provenAbsentLeafPtyVerdicts.delete(ptyId) + } + const probeLiveness = this.ptyController?.probePtyLiveness?.bind(this.ptyController) + if (!probeLiveness) { + return Promise.resolve(false) + } + const inFlight = this.leafPtyAbsenceProbes.get(ptyId) + if (inFlight) { + return inFlight + } + const probe = (async () => { + try { + if ((await probeLiveness(ptyId)) !== false) { + return false + } + this.provenAbsentLeafPtyVerdicts.set(ptyId, Date.now()) + return true + } catch { + // Why: a failed probe is unknown, and unknown never rejects a write. + return false + } finally { + this.leafPtyAbsenceProbes.delete(ptyId) + } + })() + this.leafPtyAbsenceProbes.set(ptyId, probe) + return probe + } + async sendTerminal( handle: string, action: { @@ -16120,6 +16191,13 @@ export class OrcaRuntimeService { throw new Error('invalid_terminal_send') } await assertTerminalInputWithinLimitWithYield(action.text) + // Why: leaf.writable mirrors the renderer graph, which can still answer for + // a prior process's ptyId — and provider writes to unknown ids are accepted + // no-ops. Only controller-proven absence rejects; unknown proceeds (a + // restored daemon session takes writes before its pane remounts). + if (await this.isLeafPtyProvenAbsent(leaf.ptyId)) { + throw new Error('terminal_not_writable') + } await this.writeTerminalAction(leaf.ptyId, action, payload, options) @@ -16155,6 +16233,11 @@ export class OrcaRuntimeService { throw new Error('terminal_not_writable') } await assertTerminalInputWithinLimitWithYield(payload) + // Why: same absence gate as sendTerminal — a stale graph mirror must not + // accept a prompt into a void; unknown liveness still proceeds. + if (await this.isLeafPtyProvenAbsent(leaf.ptyId)) { + throw new Error('terminal_not_writable') + } await this.writeTerminalAgentPrompt(leaf.ptyId, payload, options) return { handle, accepted: true, bytesWritten } } @@ -31232,8 +31315,49 @@ export class OrcaRuntimeService { return null } + // Why: delivered_at for Claude targets stamps only in the delayed-Enter + // callback, so the whole write→settle span must be single-flight per pty — + // a second read inside it would re-inject the same unread rows. Triggers + // landing mid-flight park the latest leaf and re-run once on settle. The + // flight object is the settle identity: a stale settle surviving an exit + // retire must not clear a newer same-id flight or flush its parked trigger. + private readonly messageDeliveryFlightsByPtyId = new Map< + string, + { enterTimer: ReturnType | null } + >() + + private readonly parkedMessageRedeliveryLeavesByPtyId = new Map() + + private settlePendingMessageDelivery( + ptyId: string, + flight: { enterTimer: ReturnType | null } + ): void { + if (this.messageDeliveryFlightsByPtyId.get(ptyId) !== flight) { + return + } + this.messageDeliveryFlightsByPtyId.delete(ptyId) + const parkedLeaf = this.parkedMessageRedeliveryLeavesByPtyId.get(ptyId) + if (!parkedLeaf) { + return + } + this.parkedMessageRedeliveryLeavesByPtyId.delete(ptyId) + this.deliverPendingMessages(parkedLeaf) + } + + // Why: an Enter armed for a dead session must not fire into a same-id cold + // restore — it would inject \r and stamp rows the replacement never saw. + // Retire without stamping; the rows re-deliver on the replacement's next idle. + private retirePendingMessageDeliveryForPty(ptyId: string): void { + const flight = this.messageDeliveryFlightsByPtyId.get(ptyId) + if (flight?.enterTimer != null) { + clearTimeout(flight.enterTimer) + } + this.messageDeliveryFlightsByPtyId.delete(ptyId) + this.parkedMessageRedeliveryLeavesByPtyId.delete(ptyId) + } + // Why: push-on-idle delivery is event-driven (no polling) because the runtime owns both the message store and terminal status detection. - private deliverPendingMessages(leaf: RuntimeLeafRecord): void { + private deliverPendingMessages(leaf: RuntimeLeafRecord, skipAbsenceProbe = false): void { if (!this._orchestrationDb) { return } @@ -31243,6 +31367,12 @@ export class OrcaRuntimeService { return } + // Why before reading rows: rows read mid-flight are the not-yet-stamped ones. + if (leaf.ptyId && this.messageDeliveryFlightsByPtyId.has(leaf.ptyId)) { + this.parkedMessageRedeliveryLeavesByPtyId.set(leaf.ptyId, leaf) + return + } + const unread = this._orchestrationDb.getUndeliveredUnreadMessages(handle) if (unread.length === 0) { return @@ -31252,41 +31382,95 @@ export class OrcaRuntimeService { return } - const payload = formatMessagesForInjection(unread) - const wrote = this.ptyController?.write(leaf.ptyId, payload) ?? false - if (!wrote) { - return - } - - // The active coordinator prompt is user-owned input, so push-on-idle must not synthesize Enter. - if (this._orchestrationDb.getActiveCoordinatorRun()?.coordinator_handle === handle) { - this._orchestrationDb.markAsDelivered(unread.map((m) => m.id)) - return - } - - const tabTitle = this.tabs.get(leaf.tabId)?.title - if (isCursorAgentOrchestrationTarget(leaf, tabTitle)) { - // Why: Cursor Agent treats injected PTY text as editable prompt input, so submitting must stay under user control. - this._orchestrationDb.markAsDelivered(unread.map((m) => m.id)) - return - } - - // Why: Claude Code treats a large PTY write as a paste and swallows a \r in the same write; send Enter separately after a delay, stamping delivered_at only once \r is confirmed. - // Important (design doc §3.2, feedback #2): stamp delivered_at, not read — read means "a check-caller consumed this"; flipping it would hide the message from check --unread. - const ptyId = leaf.ptyId - setTimeout(() => { - try { - if (!leaf.writable) { - return - } - const submitted = this.ptyController?.write(ptyId, '\r') ?? false - if (submitted) { - this._orchestrationDb?.markAsDelivered(unread.map((m) => m.id)) - } - } catch { - // Terminal may have closed during the delay — messages stay queued (delivered_at NULL) and re-deliver on next idle. + if ( + !skipAbsenceProbe && + this.ptyController?.probePtyLiveness && + !this.controllerKnowsPtyIsLive(leaf.ptyId) + ) { + // Why: a fire-and-forget write to a prior process's ptyId reports success + // and would mark these delivered while losing them. Proven absence keeps + // them queued for a future surface; unknown liveness still delivers. + const probedPtyId = leaf.ptyId + // Why: triggers arriving mid-probe must not each arm a continuation — the + // Claude Enter delay stamps delivered_at late, so every continuation would + // re-read the same unread rows and double-deliver. The single armed + // continuation re-reads fresh rows when it fires, so nothing is lost. + if (this.probeDeferredDeliveryPtyIds.has(probedPtyId)) { + return } - }, 500) + this.probeDeferredDeliveryPtyIds.add(probedPtyId) + void this.isLeafPtyProvenAbsent(probedPtyId) + .then((absent) => { + this.probeDeferredDeliveryPtyIds.delete(probedPtyId) + if (!absent && leaf.ptyId === probedPtyId) { + this.deliverPendingMessages(leaf, true) + } + }) + .catch(() => { + this.probeDeferredDeliveryPtyIds.delete(probedPtyId) + }) + return + } + + const deliveryPtyId = leaf.ptyId + const flight: { enterTimer: ReturnType | null } = { enterTimer: null } + this.messageDeliveryFlightsByPtyId.set(deliveryPtyId, flight) + // Why: every sync outcome — failed write, sync-stamped branch, or a throw — + // must end the flight here, or a leaked flag parks this pty's deliveries + // forever. Only an armed Enter hands settling to its own callback. + let settlesInEnterCallback = false + try { + const payload = formatMessagesForInjection(unread) + const wrote = this.ptyController?.write(deliveryPtyId, payload) ?? false + if (!wrote) { + return + } + + // The active coordinator prompt is user-owned input, so push-on-idle must not synthesize Enter. + if (this._orchestrationDb.getActiveCoordinatorRun()?.coordinator_handle === handle) { + this._orchestrationDb.markAsDelivered(unread.map((m) => m.id)) + return + } + + const tabTitle = this.tabs.get(leaf.tabId)?.title + if (isCursorAgentOrchestrationTarget(leaf, tabTitle)) { + // Why: Cursor Agent treats injected PTY text as editable prompt input, so submitting must stay under user control. + this._orchestrationDb.markAsDelivered(unread.map((m) => m.id)) + return + } + + // Why: Claude Code treats a large PTY write as a paste and swallows a \r in the same write; send Enter separately after a delay, stamping delivered_at only once \r is confirmed. + // Important (design doc §3.2, feedback #2): stamp delivered_at, not read — read means "a check-caller consumed this"; flipping it would hide the message from check --unread. + flight.enterTimer = setTimeout(() => { + try { + // Why current state, not the closure: graph resync replaces leaf + // objects, so the captured record can read writable=true after the + // pty died, and an exit retire may have superseded this flight. + if (this.messageDeliveryFlightsByPtyId.get(deliveryPtyId) !== flight) { + return + } + const currentLeaf = this.leaves.get(this.getLeafKey(leaf.tabId, leaf.leafId)) + if (!currentLeaf || currentLeaf.ptyId !== deliveryPtyId || !currentLeaf.writable) { + return + } + const submitted = this.ptyController?.write(deliveryPtyId, '\r') ?? false + if (submitted) { + this._orchestrationDb?.markAsDelivered(unread.map((m) => m.id)) + } + } catch { + // Terminal may have closed during the delay — messages stay queued (delivered_at NULL) and re-deliver on next idle. + } finally { + // Why finally: every outcome — submit, refusal, throw — ends the flight, + // and settle re-runs any trigger parked during it so nothing strands. + this.settlePendingMessageDelivery(deliveryPtyId, flight) + } + }, 500) + settlesInEnterCallback = true + } finally { + if (!settlesInEnterCallback) { + this.settlePendingMessageDelivery(deliveryPtyId, flight) + } + } } private resolveWaiter(waiter: TerminalWaiter, result: RuntimeTerminalWait): void { diff --git a/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts new file mode 100644 index 000000000..01aed591d --- /dev/null +++ b/src/main/runtime/terminal-send-stale-leaf-liveness.test.ts @@ -0,0 +1,512 @@ +import { describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from './orca-runtime' +import { getDefaultWorkspaceSession } from '../../shared/constants' +import type { WorkspaceSessionState } from '../../shared/types' + +// STA repro (silent-send incident): `orca terminal send` to a leaf whose ptyId +// no provider in this process owns was a silent no-op reported as success — +// the stale graph mirror answers writable=true and provider writes to unknown +// ids are accepted fire-and-forget. The leaf branch must reject ONLY on +// controller-proven absence; unknown liveness never rejects (a restored daemon +// session legitimately accepts writes before its pane remounts). + +const WORKTREE_ID = 'repo-1::/tmp/probe-worktree' +const LEAF_ID = '11111111-1111-4111-8111-111111111111' +const STALE_PTY_ID = 'pty-stale-from-prior-run' + +function makeStore() { + const session: WorkspaceSessionState = getDefaultWorkspaceSession() + return { + getWorkspaceSession: vi.fn(() => session), + setWorkspaceSession: vi.fn(), + getRepos: vi.fn(() => [ + { + id: 'repo-1', + path: '/tmp/probe-worktree', + displayName: 'probe', + badgeColor: '#000000', + addedAt: 0 + } + ]), + getAllWorktreeMeta: vi.fn(() => ({})), + getWorktreeMeta: vi.fn(() => undefined), + setWorktreeMeta: vi.fn(), + removeWorktreeMeta: vi.fn(), + getSettings: vi.fn(() => ({ workspaceDir: '/tmp/workspaces' })), + getProjects: vi.fn(() => []) + } +} + +async function makeRuntimeWithLeafHandle(options: { + leafPtyId?: string + probePtyLiveness?: (ptyId: string) => Promise + hasPty?: (ptyId: string) => boolean | null +}): Promise<{ + runtime: OrcaRuntimeService + handle: string + write: ReturnType +}> { + const runtime = new OrcaRuntimeService(makeStore() as never) + const write = vi.fn(() => true) + runtime.setPtyController({ + spawn: vi.fn(async () => ({ id: 'never' })), + write, + kill: () => true, + getForegroundProcess: async () => null, + listProcesses: vi.fn(async () => []), + ...(options.hasPty ? { hasPty: options.hasPty } : {}), + ...(options.probePtyLiveness ? { probePtyLiveness: options.probePtyLiveness } : {}) + } as never) + runtime.attachWindow(1) + publishLeafGraph(runtime, options.leafPtyId ?? STALE_PTY_ID) + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + return { runtime, handle: terminals[0].handle, write } +} + +// Re-invocable: every graph resync replaces leaf records with fresh objects. +function publishLeafGraph(runtime: OrcaRuntimeService, leafPtyId: string): void { + runtime.syncWindowGraph(1, { + tabs: [ + { + tabId: 'tab-1', + worktreeId: WORKTREE_ID, + title: 'Codex', + activeLeafId: LEAF_ID, + layout: null + } + ], + leaves: [ + { + tabId: 'tab-1', + worktreeId: WORKTREE_ID, + leafId: LEAF_ID, + paneRuntimeId: 1, + ptyId: leafPtyId, + paneTitle: null, + title: '' + } + ] + }) +} + +describe('sendTerminal absence gate for leaf-branch writes', () => { + it('rejects only on controller-proven absence and never dispatches the write', async () => { + const probe = vi.fn(async () => false) + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ probePtyLiveness: probe }) + + await expect(runtime.sendTerminal(handle, { text: 'ping' })).rejects.toThrow( + 'terminal_not_writable' + ) + + expect(probe).toHaveBeenCalledWith(STALE_PTY_ID) + expect(write).not.toHaveBeenCalled() + }) + + it('gates agent prompt sends behind the same proven-absence check', async () => { + const probe = vi.fn(async () => false) + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ probePtyLiveness: probe }) + + await expect(runtime.sendTerminalAgentPrompt(handle, 'do the thing')).rejects.toThrow( + 'terminal_not_writable' + ) + + expect(write).not.toHaveBeenCalled() + }) + + it('proceeds when the probe answers unknown (null) — unknown is not absence', async () => { + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ + probePtyLiveness: async () => null + }) + + await expect(runtime.sendTerminal(handle, { text: 'ping' })).resolves.toMatchObject({ + handle, + accepted: true + }) + + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, 'ping') + }) + + it('treats a throwing probe as unknown and proceeds', async () => { + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ + probePtyLiveness: async () => { + throw new Error('probe transport down') + } + }) + + await expect(runtime.sendTerminal(handle, { text: 'ping' })).resolves.toMatchObject({ + accepted: true + }) + + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, 'ping') + }) + + it('proceeds when the probe answers live (restored session before its pane remounts)', async () => { + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ + probePtyLiveness: async () => true + }) + + await expect(runtime.sendTerminal(handle, { text: 'ping' })).resolves.toMatchObject({ + accepted: true + }) + + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, 'ping') + }) + + it('proceeds unchanged when the controller exposes no probe', async () => { + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({}) + + await expect(runtime.sendTerminal(handle, { text: 'ping' })).resolves.toMatchObject({ + accepted: true + }) + + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, 'ping') + }) + + it('never probes when the provider synchronously knows the id (live pty)', async () => { + const probe = vi.fn(async () => false) + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ + probePtyLiveness: probe, + hasPty: (ptyId) => ptyId === STALE_PTY_ID + }) + + await expect(runtime.sendTerminal(handle, { text: 'ping' })).resolves.toMatchObject({ + accepted: true + }) + + expect(probe).not.toHaveBeenCalled() + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, 'ping') + }) + + it('reuses a proven-absent verdict across repeated sends instead of re-probing', async () => { + const probe = vi.fn(async () => false) + const { runtime, handle } = await makeRuntimeWithLeafHandle({ probePtyLiveness: probe }) + + await expect(runtime.sendTerminal(handle, { text: 'a' })).rejects.toThrow( + 'terminal_not_writable' + ) + await expect(runtime.sendTerminal(handle, { text: 'b' })).rejects.toThrow( + 'terminal_not_writable' + ) + + expect(probe).toHaveBeenCalledTimes(1) + }) + + it('drops the cached absent verdict once the provider re-learns the id', async () => { + const probe = vi.fn(async () => false) + const livePtyIds = new Set() + const { runtime, handle, write } = await makeRuntimeWithLeafHandle({ + probePtyLiveness: probe, + hasPty: (ptyId) => livePtyIds.has(ptyId) + }) + + await expect(runtime.sendTerminal(handle, { text: 'a' })).rejects.toThrow( + 'terminal_not_writable' + ) + // Same id recreated by a fresh spawn: provider knowledge must beat the verdict. + livePtyIds.add(STALE_PTY_ID) + + await expect(runtime.sendTerminal(handle, { text: 'b' })).resolves.toMatchObject({ + accepted: true + }) + expect(probe).toHaveBeenCalledTimes(1) + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, 'b') + }) +}) + +type StoredMessageRow = { + id: string + run_id: string + from_handle: string + to_handle: string + subject: string + body: string + type: string + priority: string + thread_id: string | null + payload: string | null + read: number + sequence: number + created_at: string + delivered_at: string | null + sender_pane_key: null +} + +function makeOrchestrationDbStub(toHandle: () => string) { + const rows: StoredMessageRow[] = [] + const markAsDelivered = vi.fn((ids: string[]) => { + for (const row of rows) { + if (ids.includes(row.id)) { + row.delivered_at = 'now' + } + } + }) + return { + rows, + markAsDelivered, + insert(subject: string): void { + rows.push({ + id: `msg_${rows.length + 1}`, + run_id: 'run_test', + from_handle: 'term_sender', + to_handle: toHandle(), + subject, + body: '', + type: 'status', + priority: 'normal', + thread_id: null, + payload: null, + read: 0, + sequence: rows.length + 1, + created_at: 'now', + delivered_at: null, + sender_pane_key: null + }) + }, + db: { + getUndeliveredUnreadMessages: (handle: string) => + rows.filter((row) => row.to_handle === handle && !row.delivered_at), + getActiveCoordinatorRun: () => null, + // Consulted by onPtyExit's dispatch-failure path. + getActiveDispatchForTerminal: () => null, + markAsDelivered, + close: () => {} + } + } +} + +describe('push-on-idle orchestration delivery absence gate', () => { + async function makeIdleLeafWithoutPtyRecord(options: { + probePtyLiveness: (ptyId: string) => Promise + hasPty?: (ptyId: string) => boolean | null + }) { + const { runtime, handle, write } = await makeRuntimeWithLeafHandle(options) + const stub = makeOrchestrationDbStub(() => handle) + runtime.setOrchestrationDb(stub.db as never) + // Title transitions mark the leaf idle; without a hasPty answering true the + // provider never knew this id, modeling a leaf restored from a prior process. + runtime.onPtyData(STALE_PTY_ID, '\x1b]0;Codex working\x07', 100) + runtime.onPtyData(STALE_PTY_ID, '\x1b]0;Codex done\x07', 101) + return { runtime, handle, write, stub } + } + + it('keeps messages queued instead of marking a proven-absent pty delivered', async () => { + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: async () => false + }) + stub.insert('lost forever?') + + runtime.deliverPendingMessagesForHandle(handle) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(write).not.toHaveBeenCalled() + expect(stub.markAsDelivered).not.toHaveBeenCalled() + expect(stub.rows[0].delivered_at).toBeNull() + }) + + it('still delivers on unknown liveness after the probe resolves', async () => { + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: async () => null + }) + stub.insert('hello') + + runtime.deliverPendingMessagesForHandle(handle) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(write).toHaveBeenCalledWith(STALE_PTY_ID, expect.stringContaining('Subject: hello')) + }) + + // Why: delivered_at stamps only in the delayed-Enter callback, so the whole + // write→settle span — not just the probe — must be single-flight; a trigger + // landing inside the 500ms window would re-read the same un-stamped rows. + it('delivers once across concurrent probe triggers and an in-window re-trigger, then flushes parked rows', async () => { + vi.useFakeTimers() + try { + let resolveProbe!: (value: boolean | null) => void + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: () => + new Promise((resolve) => { + resolveProbe = resolve + }) + }) + stub.insert('exactly once') + + runtime.deliverPendingMessagesForHandle(handle) + runtime.deliverPendingMessagesForHandle(handle) + runtime.deliverPendingMessagesForHandle(handle) + resolveProbe(null) + await vi.advanceTimersByTimeAsync(0) + + const firstSubjectWrites = () => + write.mock.calls.filter( + ([, data]) => typeof data === 'string' && data.includes('Subject: exactly once') + ) + expect(firstSubjectWrites()).toHaveLength(1) + + // Re-trigger INSIDE the 500ms Enter window: the first batch is written but + // not yet stamped, so a fresh probe cycle would re-inject it. (On the + // fixed code no new probe is armed — the trigger parks; resolveProbe then + // re-resolves the settled first probe, a no-op.) + stub.insert('second message') + runtime.deliverPendingMessagesForHandle(handle) + resolveProbe(null) + await vi.advanceTimersByTimeAsync(0) + expect(firstSubjectWrites()).toHaveLength(1) + + // Enter fires, delivered_at stamps, the flight settles, and the parked + // trigger re-runs on its own — arming a fresh probe for the new row. + await vi.advanceTimersByTimeAsync(500) + resolveProbe(null) + await vi.advanceTimersByTimeAsync(0) + + const secondSubjectWrites = write.mock.calls.filter( + ([, data]) => typeof data === 'string' && data.includes('Subject: second message') + ) + expect(secondSubjectWrites).toHaveLength(1) + expect(firstSubjectWrites()).toHaveLength(1) + + await vi.advanceTimersByTimeAsync(500) + expect(stub.rows.every((row) => row.delivered_at !== null)).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('sync-path double-trigger inside the Enter window delivers the first batch once and parks the rest', async () => { + vi.useFakeTimers() + try { + const probe = vi.fn(async () => null) + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: probe, + // Provider knows the id: delivery takes the pure synchronous path. + hasPty: (ptyId) => ptyId === STALE_PTY_ID + }) + stub.insert('first') + + runtime.deliverPendingMessagesForHandle(handle) + stub.insert('second') + runtime.deliverPendingMessagesForHandle(handle) + + const firstSubjectWrites = () => + write.mock.calls.filter( + ([, data]) => typeof data === 'string' && data.includes('Subject: first') + ) + expect(firstSubjectWrites()).toHaveLength(1) + expect(probe).not.toHaveBeenCalled() + + // Settle flushes the parked trigger; the second row delivers alone — + // its batch must not re-contain the already-stamped first row. + await vi.advanceTimersByTimeAsync(500) + const secondOnlyWrites = write.mock.calls.filter( + ([, data]) => + typeof data === 'string' && + data.includes('Subject: second') && + !data.includes('Subject: first') + ) + expect(secondOnlyWrites).toHaveLength(1) + expect(firstSubjectWrites()).toHaveLength(1) + + await vi.advanceTimersByTimeAsync(500) + expect(stub.rows.every((row) => row.delivered_at !== null)).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + // Why: cold restore respawns under the SAME session id. An Enter armed for + // the dead incarnation must not fire into the replacement — it would inject + // \r and stamp rows the new session never received. + it('retires an armed Enter when the pty exits and respawns under the same id inside the window', async () => { + vi.useFakeTimers() + try { + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: async () => null, + hasPty: (ptyId) => ptyId === STALE_PTY_ID + }) + stub.insert('for the old session') + + runtime.deliverPendingMessagesForHandle(handle) + expect(write).toHaveBeenCalledTimes(1) + + runtime.onPtyExit(STALE_PTY_ID, 0) + runtime.onPtySpawned(STALE_PTY_ID) + + await vi.advanceTimersByTimeAsync(500) + expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) + expect(stub.markAsDelivered).not.toHaveBeenCalled() + expect(stub.rows[0].delivered_at).toBeNull() + + // The replacement's own delivery starts a fresh flight and completes. + runtime.deliverPendingMessagesForHandle(handle) + const payloadWrites = write.mock.calls.filter( + ([, data]) => typeof data === 'string' && data.includes('Subject: for the old session') + ) + expect(payloadWrites).toHaveLength(2) + await vi.advanceTimersByTimeAsync(500) + expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(1) + expect(stub.rows[0].delivered_at).not.toBeNull() + } finally { + vi.useRealTimers() + } + }) + + it('cleans all delivery state on exit without id reuse — no leak, no stray settle effects', async () => { + vi.useFakeTimers() + try { + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: async () => null, + hasPty: (ptyId) => ptyId === STALE_PTY_ID + }) + const internals = runtime as unknown as { + messageDeliveryFlightsByPtyId: Map + parkedMessageRedeliveryLeavesByPtyId: Map + } + stub.insert('first') + runtime.deliverPendingMessagesForHandle(handle) + stub.insert('second') + runtime.deliverPendingMessagesForHandle(handle) + expect(internals.messageDeliveryFlightsByPtyId.size).toBe(1) + expect(internals.parkedMessageRedeliveryLeavesByPtyId.size).toBe(1) + + runtime.onPtyExit(STALE_PTY_ID, 0) + expect(internals.messageDeliveryFlightsByPtyId.size).toBe(0) + expect(internals.parkedMessageRedeliveryLeavesByPtyId.size).toBe(0) + + await vi.advanceTimersByTimeAsync(500) + expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) + expect(stub.markAsDelivered).not.toHaveBeenCalled() + // No stray settle flushed the parked trigger into the dead pty. + expect(write).toHaveBeenCalledTimes(1) + expect(stub.rows.every((row) => row.delivered_at === null)).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + // Why: graph resync replaces leaf objects, so onPtyExit flips writable only + // on the replacement — a callback trusting its closure snapshot would still + // read writable=true and inject Enter after the exit, without any respawn. + it('does not fire a stale Enter through an orphaned leaf snapshot after resync and exit', async () => { + vi.useFakeTimers() + try { + const { runtime, handle, write, stub } = await makeIdleLeafWithoutPtyRecord({ + probePtyLiveness: async () => null, + hasPty: (ptyId) => ptyId === STALE_PTY_ID + }) + stub.insert('orphaned snapshot') + + runtime.deliverPendingMessagesForHandle(handle) + expect(write).toHaveBeenCalledTimes(1) + + // Replace the leaf object the armed callback closed over, then exit. + publishLeafGraph(runtime, STALE_PTY_ID) + runtime.onPtyExit(STALE_PTY_ID, 0) + + await vi.advanceTimersByTimeAsync(500) + expect(write.mock.calls.filter(([, data]) => data === '\r')).toHaveLength(0) + expect(stub.markAsDelivered).not.toHaveBeenCalled() + expect(stub.rows[0].delivered_at).toBeNull() + } finally { + vi.useRealTimers() + } + }) +})