fix(windows): close round 12 teardown findings

This commit is contained in:
OrcaWin
2026-07-20 07:11:47 -07:00
parent 41157f963f
commit df92cd16b4
5 changed files with 132 additions and 60 deletions
+68 -23
View File
@@ -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;
+3 -3
View File
@@ -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
@@ -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')
@@ -179,7 +179,7 @@ export class DegradedDaemonPtyProvider implements IPtyProvider {
async listProcesses(opts?: { deadlineMs?: number }): Promise<PtyProcessInfo[]> {
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[] {
@@ -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<string, DegradedManagedPtyProvider> {
@@ -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)
}