diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 2b5547a4ec0..1caa5fdbeb8 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -48,7 +48,7 @@ index 1ac5758bedd8cf54f32280dea4e4aeb5afdee30d..6047590bc9558dabacd3ece5114ed41c 'targets' : [ diff --git a/lib/conptyConnectFailure.test.js b/lib/conptyConnectFailure.test.js new file mode 100644 -index 0000000000000000000000000000000000000000..e3d4bab402845ec10647b8e2d8a2d85e1a96c295 +index 0000000000000000000000000000000000000000..11c92221f60565ddf28c96dacd5e1c6b70a13a8d --- /dev/null +++ b/lib/conptyConnectFailure.test.js @@ -0,0 +1,67 @@ @@ -78,7 +78,7 @@ index 0000000000000000000000000000000000000000..e3d4bab402845ec10647b8e2d8a2d85e + native_1.setTestConnectFailure(failure); + try { + for (var attempt = 1; attempt <= 3; attempt++) { -+ assert.throws(function () { return new windowsPtyAgent_1.WindowsPtyAgent(process.env.ComSpec || 'cmd.exe', '/C exit', environment_1, process.cwd(), 80, 30, false, true, false, false, true); }, new RegExp(expectedError)); ++ assert.throws(function () { return new windowsPtyAgent_1.WindowsPtyAgent(process.env.ComSpec || 'cmd.exe', '/C exit', environment_1, process.cwd(), 80, 30, false, true, true, false, true); }, new RegExp(expectedError)); + var resourceState = native_1.getTestResourceState(); + assert.strictEqual(resourceState.ptyCount, before.ptyCount); + assert.strictEqual(resourceState.closedPseudoConsoleCount, before.closedPseudoConsoleCount + attempt); @@ -353,7 +353,7 @@ index af5f343d17f012f330fe2292083f297768ff3b5e..6b558c789fc77b58c20cd9564d03e93e describe("WindowsTerminal (useConpty = " + useConpty + ", useConptyDll = " + useConptyDll + ")", function () { diff --git a/src/conptyConnectFailure.test.ts b/src/conptyConnectFailure.test.ts new file mode 100644 -index 0000000000000000000000000000000000000000..e41b702d342590ae39baec24b479c24d392e74b5 +index 0000000000000000000000000000000000000000..d4e50e2ba2c6abccf6aabdc242082a879e066427 --- /dev/null +++ b/src/conptyConnectFailure.test.ts @@ -0,0 +1,112 @@ @@ -409,7 +409,7 @@ index 0000000000000000000000000000000000000000..e41b702d342590ae39baec24b479c24d + 30, + false, + true, -+ false, ++ true, + false, + true + ), diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 17e5ef1461b..50c13919219 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -938,7 +938,7 @@ "Live process proof covers macOS local PTYs and native Windows agent/plain PTYs, but not Linux, WSL, SSH, or remote-runtime PTYs.", "Disconnected SSH relay death still requires reconnect-aware provider ownership.", "Daemon owner leases and durable retry inventory remain follow-up hardening.", - "Existing adopted Windows v24 sessions (agent, plain, or WSL) lack spawn-time Job capability metadata, so deadline-bearing worktree deletion fails closed until those sessions are closed or restarted under v25; fresh v25 plain and WSL behavior is unchanged.", + "Existing adopted Windows v24 sessions (agent, plain, or WSL) lack spawn-time Job capability metadata, so deadline-bearing worktree deletion fails closed. Closing or restarting the tracked session under v25 cannot retroactively adopt descendants that detached before v25; those older orphans may still require manual termination or an OS restart. Fresh v25 plain and WSL behavior is unchanged.", "SSH-hosted, WSL, and remote-runtime PTYs retain their host-specific teardown paths and are not placed in the local Windows Job.", "A process born in the capture second is SIGTERMed but not SIGKILLed because ps cannot prove its recycled-PID identity.", "A descendant orphaned before or during root ownership loss requires the separate crash-orphan sweep and is not recovered from a stale kill-time snapshot." diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c5eca411d12..ae657c63d82 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -21,7 +21,7 @@ patchedDependencies: hash: 9c1de9931d86864923ff53bc9d64474a86478085ac81ea4e432648c7e23d702c path: config/patches/@xterm__xterm@6.1.0-beta.287.patch node-pty@1.1.0: - hash: 833d517ede1f194ad5d67bb6b21b3bfd2535e89a4285b9a6bb34139ce4839432 + hash: df33dca8f8460a5183c15777601baece5d202e732f5ee7b91ffa37593e0cd89e path: config/patches/node-pty@1.1.0.patch importers: @@ -63,7 +63,7 @@ importers: version: 3.3.1 node-pty: specifier: ^1.1.0 - version: 1.1.0(patch_hash=833d517ede1f194ad5d67bb6b21b3bfd2535e89a4285b9a6bb34139ce4839432) + version: 1.1.0(patch_hash=df33dca8f8460a5183c15777601baece5d202e732f5ee7b91ffa37593e0cd89e) posthog-node: specifier: ^5.33.3 version: 5.33.3 @@ -12043,7 +12043,7 @@ snapshots: undici: 6.27.0 which: 6.0.1 - node-pty@1.1.0(patch_hash=833d517ede1f194ad5d67bb6b21b3bfd2535e89a4285b9a6bb34139ce4839432): + node-pty@1.1.0(patch_hash=df33dca8f8460a5183c15777601baece5d202e732f5ee7b91ffa37593e0cd89e): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/daemon/daemon-pty-session-routing.ts b/src/main/daemon/daemon-pty-session-routing.ts index 1ee6cdc29aa..c9b082578a1 100644 --- a/src/main/daemon/daemon-pty-session-routing.ts +++ b/src/main/daemon/daemon-pty-session-routing.ts @@ -13,6 +13,16 @@ export class DaemonPtySessionRouting { return this.ambiguousAdapters.has(sessionId) } + idsFor(adapter: DaemonPtyAdapter): string[] { + const ids = [...this.adapters].filter(([, owner]) => owner === adapter).map(([id]) => id) + for (const [id, owners] of this.ambiguousAdapters) { + if (owners.has(adapter)) { + ids.push(id) + } + } + return ids.sort() + } + add(sessionId: string, adapter: DaemonPtyAdapter): void { const ambiguous = this.ambiguousAdapters.get(sessionId) if (ambiguous) { diff --git a/src/main/daemon/degraded-daemon-pty-provider.test.ts b/src/main/daemon/degraded-daemon-pty-provider.test.ts index 188173816db..c3028126f55 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.test.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.test.ts @@ -168,6 +168,50 @@ describe('DegradedDaemonPtyProvider', () => { expect(fallback.write).not.toHaveBeenCalled() }) + it('refreshes recovered daemon routing after startup discovery transiently fails', async () => { + const current = createDaemonAdapter('daemon', ['recovered-session']) + const fallback = createProvider('fallback') + const provider = new DegradedDaemonPtyProvider({ current, legacy: [], fallback }) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.mocked(current.listProcesses).mockRejectedValueOnce(new Error('daemon still starting')) + + await provider.discoverDaemonSessions() + await provider.listProcesses({ deadlineMs: Date.now() + 1000 }) + await provider.shutdown('recovered-session', { + immediate: true, + deadlineMs: Date.now() + 1000 + }) + + expect(current.shutdown).toHaveBeenCalledWith('recovered-session', { + immediate: true, + deadlineMs: expect.any(Number) + }) + expect(fallback.shutdown).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it('fails deadline teardown closed for duplicate ownership across degraded daemons', async () => { + const current = createDaemonAdapter('current', ['duplicate-session']) + const legacy = createDaemonAdapter('legacy', ['duplicate-session']) + const fallback = createProvider('fallback') + const provider = new DegradedDaemonPtyProvider({ current, legacy: [legacy], fallback }) + + await provider.discoverDaemonSessions() + + await expect( + provider.shutdown('duplicate-session', { + immediate: true, + deadlineMs: Date.now() + 1000 + }) + ).rejects.toThrow('Ambiguous PTY session ownership across daemons: duplicate-session') + await expect(provider.listProcesses({ deadlineMs: Date.now() + 1000 })).rejects.toThrow( + 'Ambiguous PTY session ownership across daemons: duplicate-session' + ) + expect(current.shutdown).not.toHaveBeenCalled() + expect(legacy.shutdown).not.toHaveBeenCalled() + expect(fallback.shutdown).not.toHaveBeenCalled() + }) + it('routes authoritative recovery snapshots to the owning daemon', async () => { const current = createDaemonAdapter('daemon', ['daemon-session']) const fallback = createProvider('fallback') diff --git a/src/main/daemon/degraded-daemon-pty-provider.ts b/src/main/daemon/degraded-daemon-pty-provider.ts index 82fb2c49e98..d14f109af49 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -1,4 +1,8 @@ import type { DaemonPtyAdapter } from './daemon-pty-adapter' +import { + DegradedDaemonSessionRouting, + type DegradedManagedPtyProvider +} from './degraded-daemon-session-routing' import { shutdownDegradedFallbackSessions } from './degraded-daemon-fallback-shutdown' import type { IPtyProvider, @@ -10,11 +14,6 @@ import type { PtySpawnResult } from '../providers/types' -type ManagedPtyProvider = IPtyProvider & { - disconnectOnly?: () => Promise - dispose?: () => void -} - export class DegradedDaemonPtyProvider implements IPtyProvider { readonly routesFreshSpawnsToLocalProvider = true // Why: the preserved daemon answers protocol but cannot spawn fresh PTYs. @@ -24,8 +23,8 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { private current: DaemonPtyAdapter private legacy: DaemonPtyAdapter[] - private fallback: ManagedPtyProvider - private sessionProviders = new Map() + private fallback: DegradedManagedPtyProvider + private sessionRouting: DegradedDaemonSessionRouting private unsubscribers: (() => void)[] = [] private dataListeners: ((payload: PtyDataEvent) => void)[] = [] private exitListeners: ((payload: { id: string; code: number }) => void)[] = [] @@ -33,11 +32,12 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { constructor(opts: { current: DaemonPtyAdapter legacy: DaemonPtyAdapter[] - fallback: ManagedPtyProvider + fallback: DegradedManagedPtyProvider }) { this.current = opts.current this.legacy = opts.legacy this.fallback = opts.fallback + this.sessionRouting = new DegradedDaemonSessionRouting(this.fallback, this.allDaemonAdapters()) for (const provider of this.allProviders()) { this.unsubscribers.push( @@ -47,7 +47,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } }), provider.onExit((payload) => { - this.sessionProviders.delete(payload.id) + this.sessionRouting.remove(payload.id, provider) for (const listener of this.exitListeners) { listener(payload) } @@ -61,7 +61,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { try { const sessions = await adapter.listProcesses() for (const session of sessions) { - this.sessionProviders.set(session.id, adapter) + this.sessionRouting.add(session.id, adapter) } } catch (error) { console.warn('[daemon] Failed to discover degraded daemon sessions', error) @@ -70,10 +70,10 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } async spawn(opts: PtySpawnOptions): Promise { - const mapped = opts.sessionId ? this.sessionProviders.get(opts.sessionId) : undefined + const mapped = opts.sessionId ? this.sessionRouting.get(opts.sessionId) : undefined const target = mapped ?? this.fallback const result = await target.spawn(opts) - this.sessionProviders.set(result.id, target) + this.sessionRouting.add(result.id, target) return result } @@ -82,11 +82,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } hasPty(id: string): boolean { - const mapped = this.sessionProviders.get(id) - if (mapped) { - return mapped.hasPty?.(id) ?? true - } - return this.findProviderForExistingSession(id) !== null + return this.sessionRouting.hasPty(id) } write(id: string, data: string): void { @@ -113,9 +109,13 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { id: string, opts: { immediate?: boolean; keepHistory?: boolean; deadlineMs?: number } ): Promise { - await this.providerFor(id).shutdown(id, opts) + if (opts.deadlineMs !== undefined && this.sessionRouting.isAmbiguous(id)) { + throw this.ambiguousOwnershipError([id]) + } + const target = this.providerFor(id) + await target.shutdown(id, opts) if (!opts.keepHistory) { - this.sessionProviders.delete(id) + this.sessionRouting.remove(id, target) } } @@ -177,9 +177,12 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } async listProcesses(opts?: { deadlineMs?: number }): Promise { - const results = await Promise.all( - this.allProviders().map((provider) => provider.listProcesses(opts)) - ) + const providers = this.allProviders() + const results = await Promise.all(providers.map((provider) => provider.listProcesses(opts))) + const ambiguousIds = this.sessionRouting.refreshDaemonSessions(results.slice(1)) + if (ambiguousIds.length > 0) { + throw this.ambiguousOwnershipError(ambiguousIds) + } return results.flat() } @@ -260,11 +263,11 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { const result = await adapter.reconcileOnStartup(validWorktreeIds) for (const id of result.alive) { alive.push(id) - this.sessionProviders.set(id, adapter) + this.sessionRouting.add(id, adapter) } for (const id of result.killed) { killed.push(id) - this.sessionProviders.delete(id) + this.sessionRouting.remove(id, adapter) } } return { alive, killed } @@ -284,16 +287,16 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } async shutdownFallbackSessions(): Promise { - return shutdownDegradedFallbackSessions(this.sessionProviders, this.fallback) + return shutdownDegradedFallbackSessions(this.sessionRouting.providerMap(), this.fallback) } getCurrentDaemonSessionIds(): string[] { - return this.sessionIdsForProvider(this.current) + return this.sessionRouting.idsForDaemon(this.current) } fanoutCurrentDaemonSyntheticExits(code: number): void { for (const id of this.getCurrentDaemonSessionIds()) { - this.sessionProviders.delete(id) + this.sessionRouting.remove(id, this.current) // Why: sessions discovered from listProcesses may not exist in the // adapter's active-session set, but restart still kills that daemon. // oxlint-disable-next-line unicorn/no-useless-spread -- copy-safe: listeners may unsubscribe during iteration @@ -320,38 +323,19 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { return this.allDaemonAdapters() } - private providerFor(sessionId: string): ManagedPtyProvider { - return ( - this.sessionProviders.get(sessionId) ?? - this.findProviderForExistingSession(sessionId) ?? - this.fallback - ) - } - - private findProviderForExistingSession(sessionId: string): ManagedPtyProvider | null { - for (const provider of this.allProviders()) { - if (provider.hasPty?.(sessionId) === true) { - this.sessionProviders.set(sessionId, provider) - return provider - } - } - return null - } - - private sessionIdsForProvider(provider: ManagedPtyProvider): string[] { - return [...this.sessionProviders] - .filter(([, mappedProvider]) => mappedProvider === provider) - .map(([id]) => id) + private providerFor(sessionId: string): DegradedManagedPtyProvider { + return this.sessionRouting.providerFor(sessionId, this.current) } private daemonAdapterFor(sessionId: string): DaemonPtyAdapter | null { - const provider = this.sessionProviders.get(sessionId) - return provider && this.allDaemonAdapters().includes(provider as DaemonPtyAdapter) - ? (provider as DaemonPtyAdapter) - : null + return this.sessionRouting.daemonFor(sessionId) } - private allProviders(): ManagedPtyProvider[] { + private ambiguousOwnershipError(sessionIds: string[]): Error { + return new Error(`Ambiguous PTY session ownership across daemons: ${sessionIds.join(', ')}`) + } + + private allProviders(): DegradedManagedPtyProvider[] { return [this.fallback, ...this.allDaemonAdapters()] } diff --git a/src/main/daemon/degraded-daemon-session-routing.ts b/src/main/daemon/degraded-daemon-session-routing.ts new file mode 100644 index 00000000000..98d316a8f0e --- /dev/null +++ b/src/main/daemon/degraded-daemon-session-routing.ts @@ -0,0 +1,136 @@ +import type { DaemonPtyAdapter } from './daemon-pty-adapter' +import { DaemonPtySessionRouting } from './daemon-pty-session-routing' +import type { IPtyProvider, PtyProcessInfo } from '../providers/types' + +export type DegradedManagedPtyProvider = IPtyProvider & { + disconnectOnly?: () => Promise + dispose?: () => void +} + +export class DegradedDaemonSessionRouting { + private providers = new Map() + private daemonRouting = new DaemonPtySessionRouting() + + constructor( + private fallback: DegradedManagedPtyProvider, + private daemonAdapters: DaemonPtyAdapter[] + ) {} + + get(sessionId: string): DegradedManagedPtyProvider | undefined { + if (this.daemonRouting.isAmbiguous(sessionId)) { + return undefined + } + return this.daemonRouting.get(sessionId) ?? this.providers.get(sessionId) + } + + isAmbiguous(sessionId: string): boolean { + return this.daemonRouting.isAmbiguous(sessionId) + } + + add(sessionId: string, provider: DegradedManagedPtyProvider): void { + if (this.isDaemonAdapter(provider)) { + this.daemonRouting.add(sessionId, provider) + this.syncDaemonProvider(sessionId) + return + } + this.providers.set(sessionId, provider) + } + + remove(sessionId: string, provider: DegradedManagedPtyProvider): void { + if (this.isDaemonAdapter(provider)) { + this.daemonRouting.remove(sessionId, provider) + this.syncDaemonProvider(sessionId) + return + } + if (this.providers.get(sessionId) === provider) { + this.providers.delete(sessionId) + } + } + + refreshDaemonSessions(results: PtyProcessInfo[][]): string[] { + const ambiguousIds = this.daemonRouting.refreshLive(this.daemonAdapters, results) + for (const sessions of results) { + for (const session of sessions) { + this.syncDaemonProvider(session.id) + } + } + return ambiguousIds + } + + idsForDaemon(adapter: DaemonPtyAdapter): string[] { + return this.daemonRouting.idsFor(adapter) + } + + daemonFor(sessionId: string): DaemonPtyAdapter | null { + return this.daemonRouting.get(sessionId) ?? null + } + + hasPty(sessionId: string): boolean { + if (this.daemonRouting.isAmbiguous(sessionId)) { + return true + } + const routed = this.get(sessionId) + if (routed) { + return routed.hasPty?.(sessionId) ?? true + } + const discovered = this.discoverExistingProvider(sessionId) + return this.daemonRouting.isAmbiguous(sessionId) || discovered !== undefined + } + + providerFor( + sessionId: string, + ambiguousProvider: DegradedManagedPtyProvider + ): DegradedManagedPtyProvider { + if (this.daemonRouting.isAmbiguous(sessionId)) { + return ambiguousProvider + } + const routed = this.get(sessionId) + if (routed) { + return routed + } + const discovered = this.discoverExistingProvider(sessionId) + return this.daemonRouting.isAmbiguous(sessionId) + ? ambiguousProvider + : (discovered ?? this.fallback) + } + + providerMap(): Map { + return this.providers + } + + private discoverExistingProvider(sessionId: string): DegradedManagedPtyProvider | undefined { + for (const adapter of this.daemonAdapters) { + if (adapter.hasPty(sessionId)) { + this.add(sessionId, adapter) + } + } + if (this.daemonRouting.isAmbiguous(sessionId)) { + return undefined + } + const daemon = this.get(sessionId) + if (daemon) { + return daemon + } + if (this.fallback.hasPty?.(sessionId) === true) { + this.add(sessionId, this.fallback) + return this.fallback + } + return undefined + } + + private syncDaemonProvider(sessionId: string): void { + const owner = this.daemonRouting.get(sessionId) + if (owner) { + this.providers.set(sessionId, owner) + return + } + const mapped = this.providers.get(sessionId) + if (mapped && this.isDaemonAdapter(mapped)) { + this.providers.delete(sessionId) + } + } + + private isDaemonAdapter(provider: DegradedManagedPtyProvider): provider is DaemonPtyAdapter { + return this.daemonAdapters.includes(provider as DaemonPtyAdapter) + } +} diff --git a/src/main/daemon/terminal-session-teardown.test.ts b/src/main/daemon/terminal-session-teardown.test.ts index af3678fd6a6..0a4139d640b 100644 --- a/src/main/daemon/terminal-session-teardown.test.ts +++ b/src/main/daemon/terminal-session-teardown.test.ts @@ -99,6 +99,34 @@ describe('TerminalSessionTeardown Windows descendant termination', () => { expect(teardown.get('agent')).toBeUndefined() }) + it('observes an early root rejection while Job drain is still pending', async () => { + const session = createSession({ agent: true, nativeWindows: true }) + const rootError = new Error('root exit timed out before Job drain') + let resolveJobDrain!: (drained: boolean) => void + session.terminateJobTree.mockReturnValue( + new Promise((resolve) => { + resolveJobDrain = resolve + }) + ) + session.forceKillAndWaitForExit.mockRejectedValue(rootError) + sessions.set('agent', session) + const unhandledRejection = vi.fn() + process.on('unhandledRejection', unhandledRejection) + + try { + const killing = teardown.killSession('agent', session, true) + const rejection = expect(killing).rejects.toBe(rootError) + await new Promise((resolve) => setImmediate(resolve)) + expect(unhandledRejection).not.toHaveBeenCalled() + + resolveJobDrain(true) + await rejection + expect(unhandledRejection).not.toHaveBeenCalled() + } finally { + process.off('unhandledRejection', unhandledRejection) + } + }) + it('closes the root and fails closed when Job assignment was unavailable', async () => { const session = createSession({ agent: true, nativeWindows: true, hasJob: false }) sessions.set('agent', session) diff --git a/src/main/daemon/terminal-session-teardown.ts b/src/main/daemon/terminal-session-teardown.ts index 046318949d6..d1d58a84ff9 100644 --- a/src/main/daemon/terminal-session-teardown.ts +++ b/src/main/daemon/terminal-session-teardown.ts @@ -28,7 +28,9 @@ export class TerminalSessionTeardown { if (pending.rootSignalled && pending.session.isAlive) { // Why: the snapshot callback may have already sent the graceful root // signal in this turn; an immediate join must still escalate and wait. - pending.rootCompletion = pending.session.forceKillAndWaitForExit() + pending.rootCompletion = this.observeRootCompletion( + pending.session.forceKillAndWaitForExit() + ) } } return pending?.promise @@ -100,7 +102,7 @@ export class TerminalSessionTeardown { } entry.rootSignalled = true if (entry.immediate) { - entry.rootCompletion = session.forceKillAndWaitForExit() + entry.rootCompletion = this.observeRootCompletion(session.forceKillAndWaitForExit()) } else { session.signalTerminationRoot() } @@ -135,6 +137,13 @@ export class TerminalSessionTeardown { return operation } + private observeRootCompletion(completion: Promise): Promise { + // Why: Job/descendant preparation can outlive an already-rejected root + // wait. Observe it now while retaining the rejection for later aggregation. + void completion.catch(() => {}) + return completion + } + private async terminateWindowsTree(session: Session, killRoot: () => void): Promise { let nativeCompletion: Promise | undefined try {