From 76bd0b49da69d53823210edc45c895b19a48318e Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:46:55 -0700 Subject: [PATCH] fix(terminal): require a proven exit before retiring a subscription's lease MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isPtyKnownExited read a PTY record as exited whenever `connected` was false: if (pty) { return !pty.connected } Its own leaf fallback, one line below, demands getTerminalState(leaf) === 'exited', which is only true once lastExitCode is set. So the same function proves absence on one path and infers it on the other, and the inferring path is the one that runs whenever a record exists. onPtyExit is the only writer of lastExitCode. The liveness sweep clears `connected` with no exit code for every PTY behind a dropped relay, so that state is a lost connection to a process that may still be running on the host — not an exit. Reading it as one makes subscribeToPtyExit fire its listener synchronously at subscribe time, which retires the lease and emits `end` for a live terminal. Mobile reads `end` as "PTY gone" and rearms; after three attempts it stops and leaves the composer on "Waiting for terminal…", which is where the 0.0.44 permanent lock comes from. Use the runtime's existing three-valued discriminator so both paths demand the same proof. 'unknown' now keeps watching, and the later real exit still fires the listener; a subscription that outlives its PTY is still bounded by the connection abort. Both callers are in subscribeToPtyExit — the subscribe-time fast path and the registration-race re-check — and both want proven-exited, so neither changes shape. --- src/main/runtime/orca-runtime.ts | 5 +- ...ubscribe-relay-drop-lease-survival.test.ts | 118 ++++++++++++++++++ .../runtime/terminal-pty-exit-waiter.test.ts | 54 ++++++++ 3 files changed, 176 insertions(+), 1 deletion(-) create mode 100644 src/main/runtime/rpc/terminal-subscribe-relay-drop-lease-survival.test.ts diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 83d3c138b97..9977ae8200f 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -35328,7 +35328,10 @@ export class OrcaRuntimeService { private isPtyKnownExited(ptyId: string): boolean { const pty = this.ptysById.get(ptyId) if (pty) { - return !pty.connected + // Why: `!connected` is an inference, not proof. The liveness sweep clears it with no + // exit code for every PTY of a dropped relay, so reading that as an exit retires the + // lease of a process still running on the host — 'unknown' must keep watching. + return getPtyTerminalState(pty) === 'exited' } return this.getLeavesForPty(ptyId).some((leaf) => getTerminalState(leaf) === 'exited') } diff --git a/src/main/runtime/rpc/terminal-subscribe-relay-drop-lease-survival.test.ts b/src/main/runtime/rpc/terminal-subscribe-relay-drop-lease-survival.test.ts new file mode 100644 index 00000000000..5cfab233011 --- /dev/null +++ b/src/main/runtime/rpc/terminal-subscribe-relay-drop-lease-survival.test.ts @@ -0,0 +1,118 @@ +// Why: the stale-handle survival suite doubles subscribeToPtyExit, so it pins the wiring but +// not the predicate that decides whether a PTY counts as exited. This wires that one call to +// a real OrcaRuntimeService, because a relay drop reaches the phone through the predicate. +import { describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from '../orca-runtime' +import type { RpcRequest } from './core' +import { RpcDispatcher } from './dispatcher' +import { TERMINAL_METHODS } from './methods/terminal' +import { createSubscriptionRegistryDouble } from './subscription-registry-test-double' + +const PTY_ID = 'pty-1' + +const subscriptionCases = [ + { + name: 'lease-only', + params: { + terminal: 'terminal-1', + client: { id: 'phone-1', type: 'mobile' }, + capabilities: { terminalBinaryStream: 1, mobileInputLeaseOnly: 1 } + } + }, + { + name: 'legacy JSON', + params: { terminal: 'terminal-1', client: { id: 'desktop-1', type: 'desktop' } } + }, + { + name: 'binary', + params: { + terminal: 'terminal-1', + client: { id: 'phone-1', type: 'mobile' }, + capabilities: { terminalBinaryStream: 1 } + } + } +] as const + +type PtyInternals = { + recordPtyWorktree: (ptyId: string, worktreeId: string, state?: { connected?: boolean }) => unknown + ptysById: Map +} + +/** A PTY whose relay dropped: connection lost, no exit code, process possibly still running. */ +function makeRelayDroppedRuntime(): OrcaRuntimeService { + const real = new OrcaRuntimeService() + const internals = real as unknown as PtyInternals + internals.recordPtyWorktree(PTY_ID, 'wt-1', { connected: true }) + const pty = internals.ptysById.get(PTY_ID)! + pty.connected = false + expect(pty.lastExitCode).toBeNull() + return real +} + +function createRuntime(real: OrcaRuntimeService): { + registry: ReturnType + runtime: OrcaRuntimeService +} { + const registry = createSubscriptionRegistryDouble() + const runtime = { + getRuntimeId: () => 'test-runtime', + requestRendererTerminalTabMount: () => false, + resolveLeafForHandle: vi.fn().mockReturnValue({ ptyId: PTY_ID }), + handleMobileSubscribe: vi.fn().mockResolvedValue(true), + handleMobileUnsubscribe: vi.fn(), + registerRemoteTerminalViewSubscriber: vi.fn(() => vi.fn()), + subscribeToTerminalData: vi.fn(() => vi.fn()), + readTerminal: vi.fn().mockResolvedValue({ tail: [], truncated: false }), + serializeTerminalBuffer: vi + .fn() + .mockResolvedValue({ data: 'snapshot', cols: 80, rows: 24, seq: 1 }), + getTerminalSize: vi.fn().mockReturnValue({ cols: 80, rows: 24 }), + getMobileDisplayMode: vi.fn().mockReturnValue('auto'), + getLayout: vi.fn().mockReturnValue({ seq: 1 }), + isTerminalAlternateScreen: vi.fn().mockReturnValue(false), + subscribeToTerminalResize: vi.fn(() => vi.fn()), + subscribeToFitOverrideChanges: vi.fn(() => vi.fn()), + registerSubscriptionCleanup: vi.fn(registry.registerSubscriptionCleanup), + registerOwnedSubscriptionCleanup: vi.fn(registry.registerOwnedSubscriptionCleanup), + cleanupSubscription: vi.fn(registry.cleanupSubscription), + waitForTerminal: vi.fn(() => Promise.reject(new Error('unexpected handle waiter'))), + // The one call under test: the real exit predicate, not a double's opinion of it. + subscribeToPtyExit: (ptyId: string, listener: () => void) => + real.subscribeToPtyExit(ptyId, listener) + } as unknown as OrcaRuntimeService + return { registry, runtime } +} + +function makeRequest(params: unknown): RpcRequest { + return { id: 'req-1', authToken: 'tok', method: 'terminal.subscribe', params } +} + +describe('terminal subscribe when the relay dropped without an exit code', () => { + it.each(subscriptionCases)( + 'does not end $name for a process that may still be running', + async ({ params }) => { + const real = makeRelayDroppedRuntime() + const { registry, runtime } = createRuntime(real) + const messages: string[] = [] + const types = (): unknown[] => messages.map((message) => JSON.parse(message).result?.type) + + void new RpcDispatcher({ runtime, methods: TERMINAL_METHODS }) + .dispatchStreaming(makeRequest(params), (message) => messages.push(message), { + connectionId: 'conn-1', + sendBinary: vi.fn(), + registerBinaryStreamHandler: vi.fn(() => vi.fn()) + }) + .catch(() => undefined) + + const subscriptionId = `terminal-1:${params.client.id}` + await vi.waitFor(() => expect(registry.peekCleanup(subscriptionId)).toBeDefined()) + expect(types()).not.toContain('end') + + // The host comes back and reports the real exit; only now may the lease retire. + real.onPtyExit(PTY_ID, 0) + + await vi.waitFor(() => expect(registry.peekCleanup(subscriptionId)).toBeUndefined()) + expect(types()).toContain('end') + } + ) +}) diff --git a/src/main/runtime/terminal-pty-exit-waiter.test.ts b/src/main/runtime/terminal-pty-exit-waiter.test.ts index 5f10449f34d..8b6daa3c2b0 100644 --- a/src/main/runtime/terminal-pty-exit-waiter.test.ts +++ b/src/main/runtime/terminal-pty-exit-waiter.test.ts @@ -4,6 +4,7 @@ import { OrcaRuntimeService } from './orca-runtime' type RuntimeInternals = { recordPtyWorktree: (ptyId: string, worktreeId: string, state?: { connected?: boolean }) => unknown ptyExitListenersByPtyId: Map> + ptysById: Map } function internals(runtime: OrcaRuntimeService): RuntimeInternals { @@ -56,3 +57,56 @@ describe('PTY exit subscription', () => { expect(internals(runtime).ptyExitListenersByPtyId.has('pty-1')).toBe(false) }) }) + +// Why: the liveness sweep clears `connected` with no exit code for every PTY behind a +// dropped relay, so a record in that state is a lost connection to a process that may +// still be running. Reading it as an exit retires a live lease and emits `end`, which the +// phone rearms against three times and then locks the composer on "Waiting for terminal…". +describe('PTY exit subscription demands proof of exit, not loss of connection', () => { + function dropTheConnection(runtime: OrcaRuntimeService): void { + const pty = internals(runtime).ptysById.get('pty-1')! + pty.connected = false + // Precondition: onPtyExit is the only writer of lastExitCode, and it never ran. + expect(pty.lastExitCode).toBeNull() + } + + it('does not fire for a disconnected PTY that never reported an exit code', () => { + const runtime = new OrcaRuntimeService() + registerLivePty(runtime) + dropTheConnection(runtime) + const listener = vi.fn() + + runtime.subscribeToPtyExit('pty-1', listener) + + expect(listener).not.toHaveBeenCalled() + expect(internals(runtime).ptyExitListenersByPtyId.get('pty-1')).toHaveLength(1) + }) + + it('still fires once the host comes back and reports the real exit', () => { + const runtime = new OrcaRuntimeService() + registerLivePty(runtime) + dropTheConnection(runtime) + const listener = vi.fn() + + runtime.subscribeToPtyExit('pty-1', listener) + expect(listener).not.toHaveBeenCalled() + + // Why: subscribing during the outage must not cost the subscriber the later exit. + runtime.onPtyExit('pty-1', 0) + + expect(listener).toHaveBeenCalledOnce() + }) + + it('fires immediately for a disconnected PTY that did report an exit code', () => { + const runtime = new OrcaRuntimeService() + registerLivePty(runtime) + const pty = internals(runtime).ptysById.get('pty-1')! + pty.connected = false + pty.lastExitCode = 0 + const listener = vi.fn() + + runtime.subscribeToPtyExit('pty-1', listener) + + expect(listener).toHaveBeenCalledOnce() + }) +})