From 8685cdb3fbd4a1bce6d76fbbb704737d54817842 Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 22 Jul 2026 21:54:33 -0700 Subject: [PATCH] fix(runtime): stop a throwing client-event listener from wedging worktree sleep + harden fan-out (#10052) --- src/main/runtime/orca-runtime.test.ts | 71 +++++++++++++++++++++++++++ src/main/runtime/orca-runtime.ts | 56 ++++++++++++++------- 2 files changed, 108 insertions(+), 19 deletions(-) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 4adce6c21..3c8066265 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -27951,6 +27951,77 @@ describe('OrcaRuntimeService', () => { } }) + it('releases the worktree terminal mutation when a wake client-event listener throws', async () => { + const runtime = new OrcaRuntimeService(store) + const secondListenerEvents: RuntimeClientEvent[] = [] + // Why: a broken paired-client relay can throw synchronously while delivering the wake + // notification. That must not abort the wake or (regression) leak the per-worktree terminal + // mutation acquired in acquireWorktreeTerminalSpawn, or every later sleep wedges for 12s. + runtime.onClientEvent((event) => { + if (event.type === 'worktreeTerminalSleepState' && event.phase === 'woken') { + throw new Error('relay_send_failed') + } + }) + runtime.onClientEvent((event) => secondListenerEvents.push(event)) + const processLists = [[{ id: 'pty-1', cwd: TEST_WORKTREE_PATH, title: 'Claude' }], [], []] + runtime.setPtyController({ + write: () => true, + kill: () => false, + stopAndWait: async (ptyId) => { + runtime.onPtyExit(ptyId, -1) + return true + }, + getForegroundProcess: async () => null, + listProcesses: async () => processLists.shift() ?? [] + }) + + // Sleep leaves the worktree in a 'sleeping' state so the next spawn emits the 'woken' event. + await runtime.sleepTerminalsForWorktree(`id:${TEST_WORKTREE_ID}`) + + // The wake acquires the mutation and emits 'woken'; a throwing subscriber must not surface. + const releaseSpawn = await runtime.acquireWorktreeTerminalSpawn(TEST_WORKTREE_ID) + releaseSpawn() + + // Isolation: the second subscriber still received the 'woken' event. + expect( + secondListenerEvents.some( + (event) => event.type === 'worktreeTerminalSleepState' && event.phase === 'woken' + ) + ).toBe(true) + + // Regression: the mutation was released, so a subsequent sleep converges instead of throwing + // terminal_worktree_sleep_timeout. + await expect( + runtime.sleepTerminalsForWorktree(`id:${TEST_WORKTREE_ID}`) + ).resolves.toMatchObject({ postStopVerified: true }) + }) + + it('isolates a throwing subscriber across runtime listener fan-out', () => { + const runtime = new OrcaRuntimeService(store) + const delivered: number[] = [] + // Why: the shared notifyRuntimeListeners guard must let sibling fan-outs (here mobile + // notifications) survive a throwing subscriber, not just the client-event path. + runtime.onNotificationDispatched(() => { + throw new Error('subscriber_send_failed') + }) + runtime.onNotificationDispatched((event) => { + delivered.push(event.notificationSeq ?? -1) + }) + + expect(() => + runtime.dispatchMobileNotification({ + type: 'notification', + source: 'test', + title: 'Test', + body: 'Body', + worktreeId: TEST_WORKTREE_ID + }) + ).not.toThrow() + + // The second subscriber still received the event despite the first throwing. + expect(delivered).toHaveLength(1) + }) + it('keeps the original committed disposition across an idempotent retry', async () => { const runtime = new OrcaRuntimeService(store) const events: RuntimeClientEvent[] = [] diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 6b69e3b06..625a60e9a 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -3534,9 +3534,9 @@ export class OrcaRuntimeService { } private emitClientEvent(event: RuntimeClientEvent): void { - for (const listener of this.clientEventListeners) { - listener(event) - } + // Why: a throwing subscriber here once escaped acquireWorktreeTerminalSpawn after it took the + // per-worktree terminal mutation, leaking it and wedging that worktree's sleep until restart. + notifyRuntimeListeners(this.clientEventListeners, (listener) => listener(event), 'client-event') } private notifyWorktreesChanged(repoId: string): void { @@ -7600,7 +7600,13 @@ export class OrcaRuntimeService { ...(cwdChanged && cwd !== null ? { cwd } : {}) } for (const listener of listeners) { - listener(data, meta) + try { + listener(data, meta) + } catch (error) { + // Why: inlined rather than via notifyRuntimeListeners to avoid a per-chunk closure + // allocation on the terminal-output hot path; isolation semantics match the helper. + console.error('[runtime] pty-data listener threw', error) + } } } return outputSequence @@ -8583,9 +8589,7 @@ export class OrcaRuntimeService { if (!listeners) { return } - for (const listener of listeners) { - listener({ mode, cols, rows }) - } + notifyRuntimeListeners(listeners, (listener) => listener({ mode, cols, rows }), 'fit-override') } serializeTerminalBuffer( @@ -9860,12 +9864,13 @@ export class OrcaRuntimeService { dispatchMobileNotification(event: MobileNotificationEvent): void { const seq = this.mobileNotificationReplay.record(event) - for (const listener of this.notificationListeners) { - // Why: surface the desktop-assigned seq to live listeners so they can - // watermark the last event delivered and feed it back to getMissedSince - // on reconnect (idempotent catch-up, no duplicate local pushes). - listener({ ...event, notificationSeq: seq }) - } + // Why: surface the desktop-assigned seq to live listeners so they can watermark the last event + // delivered and feed it back to getMissedSince on reconnect (idempotent catch-up, no dupes). + notifyRuntimeListeners( + this.notificationListeners, + (listener) => listener({ ...event, notificationSeq: seq }), + 'mobile-notification' + ) } // Returns notifications dispatched after lastSeenSeq. Idempotent: the same @@ -10791,9 +10796,7 @@ export class OrcaRuntimeService { this.notifier?.terminalDriverChanged(ptyId, next) const listeners = this.driverListeners.get(ptyId) if (listeners) { - for (const listener of listeners) { - listener(next) - } + notifyRuntimeListeners(listeners, (listener) => listener(next), 'pty-driver') } } @@ -12366,9 +12369,7 @@ export class OrcaRuntimeService { if (!listeners) { return } - for (const listener of listeners) { - listener(event) - } + notifyRuntimeListeners(listeners, (listener) => listener(event), 'pty-resize') } // Why: Section 7.2 โ€” the runtime detects agent exit directly and updates @@ -29693,6 +29694,23 @@ async function waitForWorktreeTerminalMutation( } } } + +// Why: listener fan-out is best-effort delivery. One subscriber throwing synchronously โ€” e.g. a +// paired-client relay whose stream is closed โ€” must never abort the emitting operation or leak +// state (a lock/mutation) the caller holds across the emit. Isolate every listener and log. +function notifyRuntimeListeners( + listeners: Iterable, + deliver: (listener: L) => void, + context: string +): void { + for (const listener of listeners) { + try { + deliver(listener) + } catch (error) { + console.error(`[runtime] ${context} listener threw`, error) + } + } +} // Why (ยง3.3): 30s freshness window reuses a recent fetch for repeat create/dispatch on the same repo+remote; short enough a changed remote is seen next action. const FETCH_FRESHNESS_MS = 30_000 // Why: bound fetches so a Windows credential-manager GUI hang (STA-1292) can't wedge worktree creation; parity with the exact-base refresh sibling.