fix(runtime): release terminal.subscribe exit-waiters on teardown (#7490)
Every terminal.subscribe / terminal.multiplex slot registers a runtime exit-waiter via waitForTerminal(condition:'exit'). With no AbortSignal that waiter sits in waitersByHandle until the PTY actually exits — but agent terminals routinely never exit for the life of a session. It is only ever cleared by real exit or a desktop renderer graph reload (markRendererReloading / markGraphUnavailable), neither of which a remote/mobile WebSocket reconnect triggers. So on long SSH/mobile sessions every reconnect and tab-switch re-subscribe leaked a waiter, and each captured its closed-connection handler context (including the dead ws), growing host-process memory monotonically. - multiplex: give each stream an AbortController and abort it in detachStream (the single teardown point, reached on slot unsubscribe, re-subscribe pre-detach, and connection close via closeMultiplex). Passing its signal to waitForTerminal removes the waiter at detach. The existing .catch no-ops because the stream is already deleted (streams.get(streamId) !== stream). - legacy json/binary subscribe: pass the per-connection dispatch signal so the waiter is removed on socket close/error. Regression test proves a signalled exit-waiter is released when its signal aborts (and 25 reconnect churns leave zero waiters), while an unsignalled one accumulates — the pre-fix behavior. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
parent
bb8b1c3859
commit
fe3fd94d13
|
|
@ -94,6 +94,12 @@ type TerminalMultiplexStream = {
|
|||
unsubscribeFit: () => void
|
||||
unsubscribeDriver: () => void
|
||||
unregisterBinaryHandler: () => void
|
||||
// Why: the exit-wait promise for this slot is only removed from the runtime's
|
||||
// waiter set on real PTY exit. Aborting this on detach releases it on slot
|
||||
// unsubscribe, tab-switch re-subscribe, and connection close instead of
|
||||
// leaking a waiter (and the closed-connection handler context it captures)
|
||||
// for the life of a never-exiting agent terminal.
|
||||
exitWaiterAbort: AbortController
|
||||
}
|
||||
|
||||
type TerminalOutputChunk = {
|
||||
|
|
@ -1256,6 +1262,9 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [
|
|||
stream.unsubscribeDriver()
|
||||
stream.unregisterBinaryHandler()
|
||||
streams.delete(streamId)
|
||||
// Why: release the runtime exit-waiter for this slot (see the field's
|
||||
// note). The .catch below no-ops because the stream is already deleted.
|
||||
stream.exitWaiterAbort.abort()
|
||||
if (stream.isMobile && stream.client?.id) {
|
||||
runtime.handleMobileUnsubscribe(stream.ptyId, stream.client.id)
|
||||
}
|
||||
|
|
@ -1472,7 +1481,8 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [
|
|||
unsubscribeResize: () => {},
|
||||
unsubscribeFit: () => {},
|
||||
unsubscribeDriver: () => {},
|
||||
unregisterBinaryHandler: () => {}
|
||||
unregisterBinaryHandler: () => {},
|
||||
exitWaiterAbort: new AbortController()
|
||||
}
|
||||
streams.set(request.streamId, stream)
|
||||
stream.unregisterBinaryHandler = registerBinaryStreamHandler(request.streamId, (frame) =>
|
||||
|
|
@ -1660,7 +1670,10 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [
|
|||
sendResizedFrame(stream, event)
|
||||
})
|
||||
void runtime
|
||||
.waitForTerminal(request.terminal, { condition: 'exit' })
|
||||
.waitForTerminal(request.terminal, {
|
||||
condition: 'exit',
|
||||
signal: stream.exitWaiterAbort.signal
|
||||
})
|
||||
.then(() => {
|
||||
if (streams.get(request.streamId) === stream) {
|
||||
detachStream(request.streamId, true)
|
||||
|
|
@ -1797,8 +1810,10 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [
|
|||
},
|
||||
connectionId
|
||||
)
|
||||
// Why: bind the exit-waiter to the connection dispatch signal so it is
|
||||
// removed on socket close/error instead of leaking until real exit.
|
||||
void runtime
|
||||
.waitForTerminal(params.terminal, { condition: 'exit' })
|
||||
.waitForTerminal(params.terminal, { condition: 'exit', signal })
|
||||
.then(() => runtime.cleanupSubscription(subscriptionId))
|
||||
.catch(() => runtime.cleanupSubscription(subscriptionId))
|
||||
})
|
||||
|
|
@ -1847,8 +1862,10 @@ export const TERMINAL_METHODS: RpcAnyMethod[] = [
|
|||
},
|
||||
connectionId
|
||||
)
|
||||
// Why: bind the exit-waiter to the connection dispatch signal so it is
|
||||
// removed on socket close/error instead of leaking until real exit.
|
||||
void runtime
|
||||
.waitForTerminal(params.terminal, { condition: 'exit' })
|
||||
.waitForTerminal(params.terminal, { condition: 'exit', signal })
|
||||
.then(() => runtime.cleanupSubscription(subscriptionId))
|
||||
.catch(() => runtime.cleanupSubscription(subscriptionId))
|
||||
const sendFrame = (
|
||||
|
|
|
|||
|
|
@ -0,0 +1,88 @@
|
|||
/**
|
||||
* Memory-leak regression: terminal.subscribe / terminal.multiplex register a
|
||||
* runtime exit-waiter (waitForTerminal condition:'exit') per subscribed slot.
|
||||
* Without an AbortSignal that waiter is only removed on real PTY exit, so for a
|
||||
* never-exiting agent terminal every remote/mobile reconnect and tab-switch
|
||||
* re-subscribe leaked a waiter (and the closed-connection handler context it
|
||||
* captures). The subscribe paths now pass a signal — this pins that a signalled
|
||||
* exit-waiter is released when the signal aborts, and an unsignalled one is not.
|
||||
*/
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { OrcaRuntimeService } from './orca-runtime'
|
||||
import type { RuntimeTerminalWait } from '../../shared/runtime-types'
|
||||
|
||||
type RuntimeInternals = {
|
||||
recordPtyWorktree: (ptyId: string, worktreeId: string, state?: { connected?: boolean }) => unknown
|
||||
handleByPtyId: Map<string, string>
|
||||
waitersByHandle: Map<string, Set<unknown>>
|
||||
}
|
||||
|
||||
function internals(runtime: OrcaRuntimeService): RuntimeInternals {
|
||||
return runtime as unknown as RuntimeInternals
|
||||
}
|
||||
|
||||
// Register a live, connected PTY that a handle resolves to, so waitForTerminal
|
||||
// with condition:'exit' registers a pending waiter instead of resolving early.
|
||||
function registerLivePty(runtime: OrcaRuntimeService, ptyId: string, handle: string): void {
|
||||
internals(runtime).recordPtyWorktree(ptyId, 'wt-live', { connected: true })
|
||||
internals(runtime).handleByPtyId.set(ptyId, handle)
|
||||
}
|
||||
|
||||
function waiterCount(runtime: OrcaRuntimeService, handle: string): number {
|
||||
return internals(runtime).waitersByHandle.get(handle)?.size ?? 0
|
||||
}
|
||||
|
||||
describe('terminal.subscribe exit-waiter leak regression', () => {
|
||||
it('releases a signalled exit-waiter when the signal aborts', async () => {
|
||||
const runtime = new OrcaRuntimeService()
|
||||
registerLivePty(runtime, 'pty-live', 'handle-live')
|
||||
|
||||
const controller = new AbortController()
|
||||
const wait = runtime.waitForTerminal('handle-live', {
|
||||
condition: 'exit',
|
||||
signal: controller.signal
|
||||
})
|
||||
// Swallow the abort rejection; we assert on the waiter set, not the result.
|
||||
const settled: Promise<RuntimeTerminalWait | 'aborted'> = wait.catch(() => 'aborted' as const)
|
||||
await Promise.resolve()
|
||||
|
||||
expect(waiterCount(runtime, 'handle-live')).toBe(1)
|
||||
|
||||
// Simulate detachStream / connection close aborting the slot's controller.
|
||||
controller.abort()
|
||||
await expect(settled).resolves.toBe('aborted')
|
||||
|
||||
expect(waiterCount(runtime, 'handle-live')).toBe(0)
|
||||
})
|
||||
|
||||
it('leaks an unsignalled exit-waiter across reconnects (documents the bug the fix prevents)', async () => {
|
||||
const runtime = new OrcaRuntimeService()
|
||||
registerLivePty(runtime, 'pty-live', 'handle-live')
|
||||
|
||||
// Three subscribes with no signal, as the pre-fix subscribe paths did.
|
||||
for (let i = 0; i < 3; i += 1) {
|
||||
void runtime.waitForTerminal('handle-live', { condition: 'exit' }).catch(() => {})
|
||||
}
|
||||
await Promise.resolve()
|
||||
|
||||
// Nothing frees them short of real PTY exit — they accumulate.
|
||||
expect(waiterCount(runtime, 'handle-live')).toBe(3)
|
||||
})
|
||||
|
||||
it('does not leak when many signalled subscribes churn (reconnect simulation)', async () => {
|
||||
const runtime = new OrcaRuntimeService()
|
||||
registerLivePty(runtime, 'pty-live', 'handle-live')
|
||||
|
||||
for (let i = 0; i < 25; i += 1) {
|
||||
const controller = new AbortController()
|
||||
const settled = runtime
|
||||
.waitForTerminal('handle-live', { condition: 'exit', signal: controller.signal })
|
||||
.catch(() => 'aborted' as const)
|
||||
await Promise.resolve()
|
||||
controller.abort()
|
||||
await settled
|
||||
}
|
||||
|
||||
expect(waiterCount(runtime, 'handle-live')).toBe(0)
|
||||
})
|
||||
})
|
||||
Loading…
Reference in New Issue