From df92cd16b4dbb76d5b6f914ad4238c3c5d7f595c Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Mon, 20 Jul 2026 07:11:47 -0700 Subject: [PATCH] fix(windows): close round 12 teardown findings --- config/patches/node-pty@1.1.0.patch | 91 ++++++++++++++----- pnpm-lock.yaml | 6 +- .../degraded-daemon-pty-provider.test.ts | 36 +++++++- .../daemon/degraded-daemon-pty-provider.ts | 6 +- .../daemon/degraded-daemon-session-routing.ts | 53 +++++------ 5 files changed, 132 insertions(+), 60 deletions(-) diff --git a/config/patches/node-pty@1.1.0.patch b/config/patches/node-pty@1.1.0.patch index 1caa5fdbeb8..702d2b569ab 100644 --- a/config/patches/node-pty@1.1.0.patch +++ b/config/patches/node-pty@1.1.0.patch @@ -48,10 +48,10 @@ index 1ac5758bedd8cf54f32280dea4e4aeb5afdee30d..6047590bc9558dabacd3ece5114ed41c 'targets' : [ diff --git a/lib/conptyConnectFailure.test.js b/lib/conptyConnectFailure.test.js new file mode 100644 -index 0000000000000000000000000000000000000000..11c92221f60565ddf28c96dacd5e1c6b70a13a8d +index 0000000000000000000000000000000000000000..b149dd006735ddf64ea3f2b167c0271f9bcdb2f5 --- /dev/null +++ b/lib/conptyConnectFailure.test.js -@@ -0,0 +1,67 @@ +@@ -0,0 +1,68 @@ +"use strict"; +/** + * Copyright (c) 2026, Stably AI (MIT License). @@ -109,14 +109,15 @@ index 0000000000000000000000000000000000000000..11c92221f60565ddf28c96dacd5e1c6b + } + else { + assert.ok(hasTestHooks_1(), 'ConPTY test fault injection was required but is unavailable'); ++ var leakedResourceWatchdog = setTimeout(function () { ++ throw new Error('ConPTY failed-connect cleanup left JS resources alive'); ++ }, 5000); ++ leakedResourceWatchdog.unref(); + failureCases_1.forEach(function (_a) { + var failure = _a[0], expectedError = _a[1]; + return runFailureCase_1(failure, expectedError); + }); + console.log('ConPTY failed-connect cleanup: 2 passed'); -+ // Why: failed constructors leave JS pipe workers for GC; native resource -+ // assertions are complete, so the standalone CI runner can exit now. -+ process.exit(0); + } +} diff --git a/lib/conpty_console_list_agent.js b/lib/conpty_console_list_agent.js @@ -160,7 +161,7 @@ index 1ec12f796a822c78fba9ad7f6448c3987e325c23..24cbb71bce12f8e6ec617918df1e16ba var DEFAULT_NAME = 'xterm'; var DESTROY_SOCKET_TIMEOUT_MS = 200; diff --git a/lib/windowsPtyAgent.js b/lib/windowsPtyAgent.js -index a358ffb177357e177661033c1b092f9c9d0e5f5a..187d72f22bb9ee8dbf444465d0febb302be4aa54 100644 +index a358ffb177357e177661033c1b092f9c9d0e5f5a..530171dc8000f3acdd6d5a82fcdc00fce65cbb2c 100644 --- a/lib/windowsPtyAgent.js +++ b/lib/windowsPtyAgent.js @@ -26,17 +26,23 @@ var FLUSH_DATA_INTERVAL = 1000; @@ -188,16 +189,38 @@ index a358ffb177357e177661033c1b092f9c9d0e5f5a..187d72f22bb9ee8dbf444465d0febb30 if (this._useConpty) { if (!conptyNative) { conptyNative = utils_1.loadNativeModule('conpty').module; -@@ -86,7 +92,7 @@ var WindowsPtyAgent = /** @class */ (function () { +@@ -72,7 +78,7 @@ var WindowsPtyAgent = /** @class */ (function () { + this._outSocket.setEncoding('utf8'); + // The conout socket must be ready out on another thread to avoid deadlocks + this._conoutSocketWorker = new windowsConoutConnection_1.ConoutConnection(term.conout, this._useConptyDll); +- this._conoutSocketWorker.onReady(function () { ++ var conoutReady = this._conoutSocketWorker.onReady(function () { + _this._conoutSocketWorker.connectSocket(_this._outSocket); + }); + this._outSocket.on('connect', function () { +@@ -86,8 +92,19 @@ var WindowsPtyAgent = /** @class */ (function () { }); this._inSocket.setEncoding('utf8'); if (this._useConpty) { - var connect = this._ptyNative.connect(this._pty, commandLine, cwd, env, this._useConptyDll, function (c) { return _this._$onProcessExit(c); }); -+ var connect = this._ptyNative.connect(this._pty, commandLine, cwd, env, this._useConptyDll, this._windowsAgentJob, function (c) { return _this._$onProcessExit(c); }); - this._innerPid = connect.pid; +- this._innerPid = connect.pid; ++ try { ++ var connect = this._ptyNative.connect(this._pty, commandLine, cwd, env, this._useConptyDll, this._windowsAgentJob, function (c) { return _this._$onProcessExit(c); }); ++ this._innerPid = connect.pid; ++ } ++ catch (error) { ++ // Why: native connect failures happen after JS pipe resources exist; ++ // stop READY from reconnecting the abandoned output socket during cleanup. ++ conoutReady.dispose(); ++ this._inSocket.destroy(); ++ this._outSocket.destroy(); ++ this._conoutSocketWorker.dispose(); ++ throw error; ++ } } } -@@ -115,6 +121,11 @@ var WindowsPtyAgent = /** @class */ (function () { + Object.defineProperty(WindowsPtyAgent.prototype, "inSocket", { +@@ -115,6 +132,11 @@ var WindowsPtyAgent = /** @class */ (function () { enumerable: false, configurable: true }); @@ -209,7 +232,7 @@ index a358ffb177357e177661033c1b092f9c9d0e5f5a..187d72f22bb9ee8dbf444465d0febb30 WindowsPtyAgent.prototype.resize = function (cols, rows) { if (this._useConpty) { if (this._exitCode !== undefined) { -@@ -130,6 +141,23 @@ var WindowsPtyAgent = /** @class */ (function () { +@@ -130,6 +152,23 @@ var WindowsPtyAgent = /** @class */ (function () { this._ptyNative.clear(this._pty, this._useConptyDll); } }; @@ -233,7 +256,7 @@ index a358ffb177357e177661033c1b092f9c9d0e5f5a..187d72f22bb9ee8dbf444465d0febb30 WindowsPtyAgent.prototype.kill = function () { var _this = this; // Tell the agent to kill the pty, this releases handles to the process -@@ -221,6 +249,10 @@ var WindowsPtyAgent = /** @class */ (function () { +@@ -221,6 +260,10 @@ var WindowsPtyAgent = /** @class */ (function () { WindowsPtyAgent.prototype._$onProcessExit = function (exitCode) { var _this = this; this._exitCode = exitCode; @@ -353,10 +376,10 @@ 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..d4e50e2ba2c6abccf6aabdc242082a879e066427 +index 0000000000000000000000000000000000000000..c9a657fbcae63e66b5930b7b52aaa98e6e906f24 --- /dev/null +++ b/src/conptyConnectFailure.test.ts -@@ -0,0 +1,112 @@ +@@ -0,0 +1,113 @@ +/** + * Copyright (c) 2026, Stably AI (MIT License). + */ @@ -462,11 +485,12 @@ index 0000000000000000000000000000000000000000..d4e50e2ba2c6abccf6aabdc242082a87 + }); + } else { + assert.ok(hasTestHooks(), 'ConPTY test fault injection was required but is unavailable'); ++ const leakedResourceWatchdog = setTimeout(() => { ++ throw new Error('ConPTY failed-connect cleanup left JS resources alive'); ++ }, 5000); ++ leakedResourceWatchdog.unref(); + failureCases.forEach(([failure, expectedError]) => runFailureCase(failure, expectedError)); + console.log('ConPTY failed-connect cleanup: 2 passed'); -+ // Why: failed constructors leave JS pipe workers for GC; native resource -+ // assertions are complete, so the standalone CI runner can exit now. -+ process.exit(0); + } +} diff --git a/src/conpty_console_list_agent.ts b/src/conpty_console_list_agent.ts @@ -1425,7 +1449,7 @@ index dc2104b34fd396722a9e30821ff0b7dea18b4295..23510c3d7c85ecc911b94719832abe5f describe('Plain strings', () => { it('doesn\'t quote plain string', () => { diff --git a/src/windowsPtyAgent.ts b/src/windowsPtyAgent.ts -index d7054449516f0c9a62af351c2caa17331206d530..5cc174373171329f36860103edab95323da89ed7 100644 +index d7054449516f0c9a62af351c2caa17331206d530..9742be105e477384855daa2f7d9a1483478bd331 100644 --- a/src/windowsPtyAgent.ts +++ b/src/windowsPtyAgent.ts @@ -34,6 +34,9 @@ export class WindowsPtyAgent { @@ -1462,16 +1486,37 @@ index d7054449516f0c9a62af351c2caa17331206d530..5cc174373171329f36860103edab9532 if (this._useConpty) { if (!conptyNative) { conptyNative = loadNativeModule('conpty').module; -@@ -116,7 +123,7 @@ export class WindowsPtyAgent { +@@ -100,7 +107,7 @@ export class WindowsPtyAgent { + this._outSocket.setEncoding('utf8'); + // The conout socket must be ready out on another thread to avoid deadlocks + this._conoutSocketWorker = new ConoutConnection(term.conout, this._useConptyDll); +- this._conoutSocketWorker.onReady(() => { ++ const conoutReady = this._conoutSocketWorker.onReady(() => { + this._conoutSocketWorker.connectSocket(this._outSocket); + }); + this._outSocket.on('connect', () => { +@@ -116,8 +123,18 @@ export class WindowsPtyAgent { this._inSocket.setEncoding('utf8'); if (this._useConpty) { - const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, c => this._$onProcessExit(c)); -+ const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, this._windowsAgentJob, c => this._$onProcessExit(c)); - this._innerPid = connect.pid; +- this._innerPid = connect.pid; ++ try { ++ const connect = (this._ptyNative as IConptyNative).connect(this._pty, commandLine, cwd, env, this._useConptyDll, this._windowsAgentJob, c => this._$onProcessExit(c)); ++ this._innerPid = connect.pid; ++ } catch (error) { ++ // Why: native connect failures happen after JS pipe resources exist; ++ // stop READY from reconnecting the abandoned output socket during cleanup. ++ conoutReady.dispose(); ++ this._inSocket.destroy(); ++ this._outSocket.destroy(); ++ this._conoutSocketWorker.dispose(); ++ throw error; ++ } } } -@@ -138,6 +145,24 @@ export class WindowsPtyAgent { + +@@ -138,6 +155,24 @@ export class WindowsPtyAgent { } } @@ -1496,7 +1541,7 @@ index d7054449516f0c9a62af351c2caa17331206d530..5cc174373171329f36860103edab9532 public kill(): void { // Tell the agent to kill the pty, this releases handles to the process if (this._useConpty) { -@@ -223,6 +248,10 @@ export class WindowsPtyAgent { +@@ -223,6 +258,10 @@ export class WindowsPtyAgent { */ private _$onProcessExit(exitCode: number): void { this._exitCode = exitCode; diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index ae657c63d82..45765585f80 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: df33dca8f8460a5183c15777601baece5d202e732f5ee7b91ffa37593e0cd89e + hash: d4382e83147336cfe41bd6174e27ae922723cab8da1da6939d16c74b71a46820 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=df33dca8f8460a5183c15777601baece5d202e732f5ee7b91ffa37593e0cd89e) + version: 1.1.0(patch_hash=d4382e83147336cfe41bd6174e27ae922723cab8da1da6939d16c74b71a46820) 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=df33dca8f8460a5183c15777601baece5d202e732f5ee7b91ffa37593e0cd89e): + node-pty@1.1.0(patch_hash=d4382e83147336cfe41bd6174e27ae922723cab8da1da6939d16c74b71a46820): dependencies: node-addon-api: 7.1.1 diff --git a/src/main/daemon/degraded-daemon-pty-provider.test.ts b/src/main/daemon/degraded-daemon-pty-provider.test.ts index c3028126f55..e443ea5b5e4 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.test.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.test.ts @@ -203,15 +203,47 @@ describe('DegradedDaemonPtyProvider', () => { immediate: true, deadlineMs: Date.now() + 1000 }) - ).rejects.toThrow('Ambiguous PTY session ownership across daemons: duplicate-session') + ).rejects.toThrow( + 'Ambiguous PTY session ownership across degraded providers: duplicate-session' + ) await expect(provider.listProcesses({ deadlineMs: Date.now() + 1000 })).rejects.toThrow( - 'Ambiguous PTY session ownership across daemons: duplicate-session' + 'Ambiguous PTY session ownership across degraded providers: duplicate-session' ) expect(current.shutdown).not.toHaveBeenCalled() expect(legacy.shutdown).not.toHaveBeenCalled() expect(fallback.shutdown).not.toHaveBeenCalled() }) + it('fails closed when recovered daemon ownership duplicates a fallback session', async () => { + const current = createDaemonAdapter('daemon', ['duplicate-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.spawn({ sessionId: 'duplicate-session', cols: 80, rows: 24 }) + + await expect(provider.listProcesses({ deadlineMs: Date.now() + 1000 })).rejects.toThrow( + 'Ambiguous PTY session ownership across degraded providers: duplicate-session' + ) + await expect( + provider.shutdown('duplicate-session', { + immediate: true, + deadlineMs: Date.now() + 1000 + }) + ).rejects.toThrow( + 'Ambiguous PTY session ownership across degraded providers: duplicate-session' + ) + expect(current.shutdown).not.toHaveBeenCalled() + expect(fallback.shutdown).not.toHaveBeenCalled() + + // Why: ambiguity must not erase the local owner needed by daemon-restart cleanup. + await expect(provider.shutdownFallbackSessions()).resolves.toBe(1) + expect(fallback.shutdown).toHaveBeenCalledWith('duplicate-session', { immediate: true }) + warn.mockRestore() + }) + 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 d14f109af49..8a8ae448f03 100644 --- a/src/main/daemon/degraded-daemon-pty-provider.ts +++ b/src/main/daemon/degraded-daemon-pty-provider.ts @@ -179,7 +179,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { async listProcesses(opts?: { deadlineMs?: number }): Promise { const providers = this.allProviders() const results = await Promise.all(providers.map((provider) => provider.listProcesses(opts))) - const ambiguousIds = this.sessionRouting.refreshDaemonSessions(results.slice(1)) + const ambiguousIds = this.sessionRouting.refreshSessions(results[0], results.slice(1)) if (ambiguousIds.length > 0) { throw this.ambiguousOwnershipError(ambiguousIds) } @@ -332,7 +332,9 @@ export class DegradedDaemonPtyProvider implements IPtyProvider { } private ambiguousOwnershipError(sessionIds: string[]): Error { - return new Error(`Ambiguous PTY session ownership across daemons: ${sessionIds.join(', ')}`) + return new Error( + `Ambiguous PTY session ownership across degraded providers: ${sessionIds.join(', ')}` + ) } private allProviders(): DegradedManagedPtyProvider[] { diff --git a/src/main/daemon/degraded-daemon-session-routing.ts b/src/main/daemon/degraded-daemon-session-routing.ts index 98d316a8f0e..6e4669eac7f 100644 --- a/src/main/daemon/degraded-daemon-session-routing.ts +++ b/src/main/daemon/degraded-daemon-session-routing.ts @@ -17,20 +17,22 @@ export class DegradedDaemonSessionRouting { ) {} get(sessionId: string): DegradedManagedPtyProvider | undefined { - if (this.daemonRouting.isAmbiguous(sessionId)) { + if (this.isAmbiguous(sessionId)) { return undefined } return this.daemonRouting.get(sessionId) ?? this.providers.get(sessionId) } isAmbiguous(sessionId: string): boolean { - return this.daemonRouting.isAmbiguous(sessionId) + return ( + this.daemonRouting.isAmbiguous(sessionId) || + (this.providers.has(sessionId) && this.daemonRouting.get(sessionId) !== undefined) + ) } add(sessionId: string, provider: DegradedManagedPtyProvider): void { if (this.isDaemonAdapter(provider)) { this.daemonRouting.add(sessionId, provider) - this.syncDaemonProvider(sessionId) return } this.providers.set(sessionId, provider) @@ -39,7 +41,6 @@ export class DegradedDaemonSessionRouting { 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) { @@ -47,14 +48,20 @@ export class DegradedDaemonSessionRouting { } } - 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) + refreshSessions(fallbackSessions: PtyProcessInfo[], daemonResults: PtyProcessInfo[][]): string[] { + for (const session of fallbackSessions) { + this.providers.set(session.id, this.fallback) + } + const ambiguousIds = new Set(this.daemonRouting.refreshLive(this.daemonAdapters, daemonResults)) + for (const sessionId of this.providers.keys()) { + if ( + this.daemonRouting.get(sessionId) !== undefined || + this.daemonRouting.isAmbiguous(sessionId) + ) { + ambiguousIds.add(sessionId) } } - return ambiguousIds + return [...ambiguousIds].sort() } idsForDaemon(adapter: DaemonPtyAdapter): string[] { @@ -62,11 +69,11 @@ export class DegradedDaemonSessionRouting { } daemonFor(sessionId: string): DaemonPtyAdapter | null { - return this.daemonRouting.get(sessionId) ?? null + return this.isAmbiguous(sessionId) ? null : (this.daemonRouting.get(sessionId) ?? null) } hasPty(sessionId: string): boolean { - if (this.daemonRouting.isAmbiguous(sessionId)) { + if (this.isAmbiguous(sessionId)) { return true } const routed = this.get(sessionId) @@ -74,14 +81,14 @@ export class DegradedDaemonSessionRouting { return routed.hasPty?.(sessionId) ?? true } const discovered = this.discoverExistingProvider(sessionId) - return this.daemonRouting.isAmbiguous(sessionId) || discovered !== undefined + return this.isAmbiguous(sessionId) || discovered !== undefined } providerFor( sessionId: string, ambiguousProvider: DegradedManagedPtyProvider ): DegradedManagedPtyProvider { - if (this.daemonRouting.isAmbiguous(sessionId)) { + if (this.isAmbiguous(sessionId)) { return ambiguousProvider } const routed = this.get(sessionId) @@ -89,9 +96,7 @@ export class DegradedDaemonSessionRouting { return routed } const discovered = this.discoverExistingProvider(sessionId) - return this.daemonRouting.isAmbiguous(sessionId) - ? ambiguousProvider - : (discovered ?? this.fallback) + return this.isAmbiguous(sessionId) ? ambiguousProvider : (discovered ?? this.fallback) } providerMap(): Map { @@ -104,7 +109,7 @@ export class DegradedDaemonSessionRouting { this.add(sessionId, adapter) } } - if (this.daemonRouting.isAmbiguous(sessionId)) { + if (this.isAmbiguous(sessionId)) { return undefined } const daemon = this.get(sessionId) @@ -118,18 +123,6 @@ export class DegradedDaemonSessionRouting { 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) }