mirror of
https://github.com/stablyai/orca.git
synced 2026-10-25 00:03:59 +00:00
fix(terminal): require a proven exit before retiring a subscription's lease
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.
This commit is contained in:
@@ -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')
|
||||
}
|
||||
|
||||
@@ -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<string, { connected: boolean; lastExitCode: number | null }>
|
||||
}
|
||||
|
||||
/** 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<typeof createSubscriptionRegistryDouble>
|
||||
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')
|
||||
}
|
||||
)
|
||||
})
|
||||
@@ -4,6 +4,7 @@ import { OrcaRuntimeService } from './orca-runtime'
|
||||
type RuntimeInternals = {
|
||||
recordPtyWorktree: (ptyId: string, worktreeId: string, state?: { connected?: boolean }) => unknown
|
||||
ptyExitListenersByPtyId: Map<string, Set<unknown>>
|
||||
ptysById: Map<string, { connected: boolean; lastExitCode: number | null }>
|
||||
}
|
||||
|
||||
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()
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user