From 42c20b40e251dcd9ec4362161d72ab1c72d77a23 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 16:26:22 +0000 Subject: [PATCH 1/8] Drain result pipe before disposing it on cancellation --- .../testing/testController/common/utils.ts | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index 9782487d940b..e41a011cdb1a 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -102,8 +102,27 @@ export async function startRunResultNamedPipe( if (cancellationToken) { disposables.push( cancellationToken?.onCancellationRequested(() => { - traceLog(`Test Result named pipe ${pipeName} cancelled`); - disposable.dispose(); + traceLog( + `Test Result named pipe ${pipeName} cancelled; draining buffered data before dispose`, + ); + // Do NOT dispose the reader immediately. In the debug path, cancellation + // fires as soon as the debug session terminates, but the result pipe may + // still have buffered messages that have not been delivered to the + // `reader.listen` callback yet. Disposing now would close the reader and + // drop those pending results. + // + // The reader's `onClose` event (registered below) will fire once the + // subprocess closes its end of the pipe and all buffered data has been + // drained, and that handler will dispose. Use a safety timeout to force + // disposal in case the pipe never closes naturally (e.g. subprocess hang). + setTimeout(() => { + if (disposables.length > 0) { + traceVerbose( + `Test Result named pipe ${pipeName} drain timeout, forcing dispose`, + ); + disposable.dispose(); + } + }, 5000); }), ); } From fa8111a6501e448955fdbe1aa2af74ee9c3532d0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 16:27:39 +0000 Subject: [PATCH 2/8] Use explicit disposed flag instead of disposables.length check --- src/client/testing/testController/common/utils.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index e41a011cdb1a..1919c260495c 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -92,8 +92,10 @@ export async function startRunResultNamedPipe( const reader = await createReaderPipe(pipeName, cancellationToken); traceVerbose(`Test Results named pipe ${pipeName} connected`); let disposables: Disposable[] = []; + let disposed = false; const disposable = new Disposable(() => { traceVerbose(`Test Results named pipe ${pipeName} disposed`); + disposed = true; disposables.forEach((d) => d.dispose()); disposables = []; deferredTillServerClose.resolve(); @@ -116,7 +118,7 @@ export async function startRunResultNamedPipe( // drained, and that handler will dispose. Use a safety timeout to force // disposal in case the pipe never closes naturally (e.g. subprocess hang). setTimeout(() => { - if (disposables.length > 0) { + if (!disposed) { traceVerbose( `Test Result named pipe ${pipeName} drain timeout, forcing dispose`, ); From d61fe3cd0de13e4207c1143eaaf28cf7f5386f1e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 18:47:53 +0000 Subject: [PATCH 3/8] Make result pipe drain fully event-driven (drop timeout fallback) --- .../testing/testController/common/utils.ts | 46 ++++++------------- .../unittest/testExecutionAdapter.ts | 12 +++-- 2 files changed, 24 insertions(+), 34 deletions(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index 1919c260495c..2f08ca2798cc 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -89,45 +89,28 @@ export async function startRunResultNamedPipe( traceVerbose('Starting Test Result named pipe'); const pipeName: string = generateRandomPipeName('python-test-results'); + // NOTE: `cancellationToken` is intentionally only forwarded to `createReaderPipe` + // (which uses it to cancel pipe *creation*). Once the reader is connected we do + // not wire a cancellation handler here. Disposing the reader on cancellation + // would close the socket while data was still buffered in the kernel pipe, and + // any results not yet delivered to the `reader.listen` callback would be lost. + // This is exactly the regression seen in the debug path, where cancellation + // fires the moment the debug session terminates. + // + // Instead, disposal is fully event-driven: when the subprocess closes its end + // of the pipe (either by exiting normally or by being killed via the caller's + // own cancellation handling), the OS delivers all remaining buffered bytes and + // then EOF, which fires `reader.onClose` below and triggers dispose. const reader = await createReaderPipe(pipeName, cancellationToken); traceVerbose(`Test Results named pipe ${pipeName} connected`); let disposables: Disposable[] = []; - let disposed = false; const disposable = new Disposable(() => { traceVerbose(`Test Results named pipe ${pipeName} disposed`); - disposed = true; disposables.forEach((d) => d.dispose()); disposables = []; deferredTillServerClose.resolve(); }); - if (cancellationToken) { - disposables.push( - cancellationToken?.onCancellationRequested(() => { - traceLog( - `Test Result named pipe ${pipeName} cancelled; draining buffered data before dispose`, - ); - // Do NOT dispose the reader immediately. In the debug path, cancellation - // fires as soon as the debug session terminates, but the result pipe may - // still have buffered messages that have not been delivered to the - // `reader.listen` callback yet. Disposing now would close the reader and - // drop those pending results. - // - // The reader's `onClose` event (registered below) will fire once the - // subprocess closes its end of the pipe and all buffered data has been - // drained, and that handler will dispose. Use a safety timeout to force - // disposal in case the pipe never closes naturally (e.g. subprocess hang). - setTimeout(() => { - if (!disposed) { - traceVerbose( - `Test Result named pipe ${pipeName} drain timeout, forcing dispose`, - ); - disposable.dispose(); - } - }, 5000); - }), - ); - } disposables.push( reader, reader.listen((data: Message) => { @@ -136,9 +119,10 @@ export async function startRunResultNamedPipe( dataReceivedCallback((data as ExecutionResultMessage).params as ExecutionTestPayload); }), reader.onClose(() => { - // this is called once the server close, once per run instance + // Fires once the subprocess has closed its end of the pipe and the OS + // has delivered all buffered data. This is the only path that disposes + // the reader, so no results can be dropped due to a premature close. traceVerbose(`Test Result named pipe ${pipeName} closed. Disposing of listener/s.`); - // dispose of all data listeners and cancelation listeners disposable.dispose(); }), reader.onError((error) => { diff --git a/src/client/testing/testController/unittest/testExecutionAdapter.ts b/src/client/testing/testController/unittest/testExecutionAdapter.ts index c7d21b768c5b..8857dabe38f7 100644 --- a/src/client/testing/testController/unittest/testExecutionAdapter.ts +++ b/src/client/testing/testController/unittest/testExecutionAdapter.ts @@ -70,9 +70,15 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter { cSource.token, // token to cancel ); runInstance.token.onCancellationRequested(() => { - console.log(`Test run cancelled, resolving 'till TillAllServerClose' deferred for ${uri.fsPath}.`); - // if canceled, stop listening for results - deferredTillServerClose.resolve(); + console.log(`Test run cancelled for ${uri.fsPath}; waiting for result pipe to drain.`); + // Do NOT resolve `deferredTillServerClose` here. Doing so would release + // the `await deferredTillServerClose.promise` in `finally` before the + // named pipe has finished draining any results buffered by the OS, + // and those results would be dropped. The pipe will drain on its own + // once the subprocess closes its end (which happens when the caller + // kills the subprocess below or when the debug session terminates), + // at which point `reader.onClose` in `startRunResultNamedPipe` fires + // and resolves the deferred. }); try { await this.runTestsNew( From b5525ee4040ba7fd7907dec73187c268d7e585ba Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 19:41:03 +0000 Subject: [PATCH 4/8] Add tests asserting result pipe drains buffered data after cancellation --- .../testing/testController/utils.unit.test.ts | 183 +++++++++++++++++- 1 file changed, 181 insertions(+), 2 deletions(-) diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index 3cba6fb697a5..8aeaf2032743 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -2,12 +2,21 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; import * as fs from 'fs'; import * as path from 'path'; -import { CancellationToken, TestController, TestItem, Uri, Range, Position } from 'vscode'; -import { writeTestIdsFile, populateTestTree } from '../../../client/testing/testController/common/utils'; +import { CancellationToken, CancellationTokenSource, TestController, TestItem, Uri, Range, Position } from 'vscode'; +import { Emitter, Event, MessageReader, PartialMessageInfo, Disposable as RpcDisposable, DataCallback } from 'vscode-jsonrpc'; +import { Message } from 'vscode-jsonrpc'; +import { + writeTestIdsFile, + populateTestTree, + startRunResultNamedPipe, +} from '../../../client/testing/testController/common/utils'; +import { createDeferred, Deferred } from '../../../client/common/utils/async'; +import * as namedPipes from '../../../client/common/pipes/namedPipes'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { DiscoveredTestNode, DiscoveredTestItem, + ExecutionTestPayload, ITestResultResolver, } from '../../../client/testing/testController/common/types'; import { RunTestTag, DebugTestTag } from '../../../client/testing/testController/common/testItemUtilities'; @@ -752,3 +761,173 @@ suite('populateTestTree tests', () => { assert.deepStrictEqual(mockTestItem2.range, new Range(new Position(6, 0), new Position(7, 0))); }); }); + +suite('startRunResultNamedPipe drain-on-cancel tests', () => { + let sandbox: sinon.SinonSandbox; + let createReaderPipeStub: sinon.SinonStub; + + /** + * Minimal fake `MessageReader` that lets the test drive `listen` callback + * and the `onClose` / `onError` events directly. Mirrors only the bits of + * the `vscode-jsonrpc` `MessageReader` interface that `startRunResultNamedPipe` + * touches. + */ + class FakeMessageReader implements MessageReader { + private _onClose = new Emitter(); + + private _onError = new Emitter(); + + private _onPartialMessage = new Emitter(); + + private _callback: DataCallback | undefined; + + public disposed = false; + + public onError: Event = this._onError.event; + + public onClose: Event = this._onClose.event; + + public onPartialMessage: Event = this._onPartialMessage.event; + + public listen(callback: DataCallback): RpcDisposable { + this._callback = callback; + return { dispose: () => { this._callback = undefined; } }; + } + + public dispose(): void { + this.disposed = true; + this._onClose.dispose(); + this._onError.dispose(); + this._onPartialMessage.dispose(); + } + + // Test helpers. + public emit(message: Message): void { + this._callback?.(message); + } + + public hasListener(): boolean { + return this._callback !== undefined; + } + + public fireClose(): void { + this._onClose.fire(); + } + } + + setup(() => { + sandbox = sinon.createSandbox(); + }); + + teardown(() => { + sandbox.restore(); + }); + + function makeMessage(payload: Partial): Message { + return ({ jsonrpc: '2.0', params: payload } as unknown) as Message; + } + + test('cancellation alone does NOT resolve deferredTillServerClose and does NOT detach the listener (drain not interrupted)', async () => { + const reader = new FakeMessageReader(); + createReaderPipeStub = sandbox.stub(namedPipes, 'createReaderPipe').resolves(reader); + + const received: ExecutionTestPayload[] = []; + const deferredTillServerClose: Deferred = createDeferred(); + const cancelSource = new CancellationTokenSource(); + + await startRunResultNamedPipe( + (payload) => received.push(payload), + deferredTillServerClose, + cancelSource.token, + ); + + assert.ok(createReaderPipeStub.calledOnce, 'createReaderPipe should be called once'); + assert.ok(reader.hasListener(), 'reader should have a listener registered before cancel'); + + // Trigger cancellation. + cancelSource.cancel(); + + // Yield to let any synchronous-then-microtask handlers run. + await new Promise((r) => setImmediate(r)); + + assert.strictEqual( + reader.disposed, + false, + 'reader must NOT be disposed by cancellation alone (otherwise buffered data would be lost)', + ); + assert.ok(reader.hasListener(), 'data listener must remain attached after cancel so the drain can continue'); + assert.strictEqual( + (deferredTillServerClose as Deferred).completed, + false, + 'deferredTillServerClose must NOT resolve on cancellation; it should only resolve when the pipe closes', + ); + + cancelSource.dispose(); + }); + + test('data emitted after cancellation is still delivered to the callback (drain works)', async () => { + const reader = new FakeMessageReader(); + sandbox.stub(namedPipes, 'createReaderPipe').resolves(reader); + + const received: ExecutionTestPayload[] = []; + const deferredTillServerClose: Deferred = createDeferred(); + const cancelSource = new CancellationTokenSource(); + + await startRunResultNamedPipe( + (payload) => received.push(payload), + deferredTillServerClose, + cancelSource.token, + ); + + // Simulate the debug-path race: subprocess is still flushing results + // when the debug session ends and cancellation fires. + cancelSource.cancel(); + await new Promise((r) => setImmediate(r)); + + // Buffered messages arrive after cancellation. + reader.emit(makeMessage({ cwd: 'a' })); + reader.emit(makeMessage({ cwd: 'b' })); + + assert.strictEqual(received.length, 2, 'messages emitted after cancel must still reach the callback'); + assert.deepStrictEqual( + received.map((p) => p.cwd), + ['a', 'b'], + 'all buffered results delivered in order', + ); + + // The subprocess finally closes its end of the pipe; the OS delivers + // EOF, which fires `onClose` and triggers disposal. + reader.fireClose(); + await deferredTillServerClose.promise; + + assert.strictEqual(reader.disposed, true, 'reader disposed via onClose path'); + + cancelSource.dispose(); + }); + + test('reader.onClose resolves deferredTillServerClose and disposes the reader (natural completion path, no cancellation)', async () => { + const reader = new FakeMessageReader(); + sandbox.stub(namedPipes, 'createReaderPipe').resolves(reader); + + const deferredTillServerClose: Deferred = createDeferred(); + + await startRunResultNamedPipe( + () => { + /* no-op */ + }, + deferredTillServerClose, + undefined, + ); + + assert.strictEqual( + (deferredTillServerClose as Deferred).completed, + false, + 'deferred unresolved before close', + ); + + reader.fireClose(); + await deferredTillServerClose.promise; + + assert.strictEqual(reader.disposed, true, 'reader disposed when onClose fires'); + }); +}); From 0ff58dcce4a46eb694c6dd61a1194fe7c90e77d5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 21:00:34 +0000 Subject: [PATCH 5/8] Tighten verbose comments added in this PR --- .../testing/testController/common/utils.ts | 17 ++--------------- .../unittest/testExecutionAdapter.ts | 11 +++-------- .../testing/testController/utils.unit.test.ts | 13 +++---------- 3 files changed, 8 insertions(+), 33 deletions(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index 2f08ca2798cc..1f910eff8718 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -89,18 +89,8 @@ export async function startRunResultNamedPipe( traceVerbose('Starting Test Result named pipe'); const pipeName: string = generateRandomPipeName('python-test-results'); - // NOTE: `cancellationToken` is intentionally only forwarded to `createReaderPipe` - // (which uses it to cancel pipe *creation*). Once the reader is connected we do - // not wire a cancellation handler here. Disposing the reader on cancellation - // would close the socket while data was still buffered in the kernel pipe, and - // any results not yet delivered to the `reader.listen` callback would be lost. - // This is exactly the regression seen in the debug path, where cancellation - // fires the moment the debug session terminates. - // - // Instead, disposal is fully event-driven: when the subprocess closes its end - // of the pipe (either by exiting normally or by being killed via the caller's - // own cancellation handling), the OS delivers all remaining buffered bytes and - // then EOF, which fires `reader.onClose` below and triggers dispose. + // `cancellationToken` only cancels pipe creation; disposal is driven by + // `reader.onClose` so buffered results are not dropped on cancel. const reader = await createReaderPipe(pipeName, cancellationToken); traceVerbose(`Test Results named pipe ${pipeName} connected`); let disposables: Disposable[] = []; @@ -119,9 +109,6 @@ export async function startRunResultNamedPipe( dataReceivedCallback((data as ExecutionResultMessage).params as ExecutionTestPayload); }), reader.onClose(() => { - // Fires once the subprocess has closed its end of the pipe and the OS - // has delivered all buffered data. This is the only path that disposes - // the reader, so no results can be dropped due to a premature close. traceVerbose(`Test Result named pipe ${pipeName} closed. Disposing of listener/s.`); disposable.dispose(); }), diff --git a/src/client/testing/testController/unittest/testExecutionAdapter.ts b/src/client/testing/testController/unittest/testExecutionAdapter.ts index 8857dabe38f7..8acc4c0dfed8 100644 --- a/src/client/testing/testController/unittest/testExecutionAdapter.ts +++ b/src/client/testing/testController/unittest/testExecutionAdapter.ts @@ -71,14 +71,9 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter { ); runInstance.token.onCancellationRequested(() => { console.log(`Test run cancelled for ${uri.fsPath}; waiting for result pipe to drain.`); - // Do NOT resolve `deferredTillServerClose` here. Doing so would release - // the `await deferredTillServerClose.promise` in `finally` before the - // named pipe has finished draining any results buffered by the OS, - // and those results would be dropped. The pipe will drain on its own - // once the subprocess closes its end (which happens when the caller - // kills the subprocess below or when the debug session terminates), - // at which point `reader.onClose` in `startRunResultNamedPipe` fires - // and resolves the deferred. + // Don't resolve the deferred here: the pipe must drain first. + // `reader.onClose` in `startRunResultNamedPipe` will resolve it + // once the subprocess closes its end of the pipe. }); try { await this.runTestsNew( diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index 8aeaf2032743..7c7055a752fd 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -766,12 +766,7 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { let sandbox: sinon.SinonSandbox; let createReaderPipeStub: sinon.SinonStub; - /** - * Minimal fake `MessageReader` that lets the test drive `listen` callback - * and the `onClose` / `onError` events directly. Mirrors only the bits of - * the `vscode-jsonrpc` `MessageReader` interface that `startRunResultNamedPipe` - * touches. - */ + // Minimal `MessageReader` fake exposing only what `startRunResultNamedPipe` uses. class FakeMessageReader implements MessageReader { private _onClose = new Emitter(); @@ -879,8 +874,7 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { cancelSource.token, ); - // Simulate the debug-path race: subprocess is still flushing results - // when the debug session ends and cancellation fires. + // Simulate the debug-path race: cancel fires while results are still buffered. cancelSource.cancel(); await new Promise((r) => setImmediate(r)); @@ -895,8 +889,7 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { 'all buffered results delivered in order', ); - // The subprocess finally closes its end of the pipe; the OS delivers - // EOF, which fires `onClose` and triggers disposal. + // Subprocess closes its end of the pipe -> onClose fires -> dispose. reader.fireClose(); await deferredTillServerClose.promise; From ca7be340d420a7740f35b62bb17e2d38b8d48bce Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Mon, 1 Jun 2026 09:47:47 -0700 Subject: [PATCH 6/8] Apply prettier formatting to utils.unit.test.ts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../testing/testController/utils.unit.test.ts | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index 7c7055a752fd..df4bb6c132de 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -3,7 +3,14 @@ import * as sinon from 'sinon'; import * as fs from 'fs'; import * as path from 'path'; import { CancellationToken, CancellationTokenSource, TestController, TestItem, Uri, Range, Position } from 'vscode'; -import { Emitter, Event, MessageReader, PartialMessageInfo, Disposable as RpcDisposable, DataCallback } from 'vscode-jsonrpc'; +import { + Emitter, + Event, + MessageReader, + PartialMessageInfo, + Disposable as RpcDisposable, + DataCallback, +} from 'vscode-jsonrpc'; import { Message } from 'vscode-jsonrpc'; import { writeTestIdsFile, @@ -786,7 +793,11 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { public listen(callback: DataCallback): RpcDisposable { this._callback = callback; - return { dispose: () => { this._callback = undefined; } }; + return { + dispose: () => { + this._callback = undefined; + }, + }; } public dispose(): void { @@ -830,11 +841,7 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { const deferredTillServerClose: Deferred = createDeferred(); const cancelSource = new CancellationTokenSource(); - await startRunResultNamedPipe( - (payload) => received.push(payload), - deferredTillServerClose, - cancelSource.token, - ); + await startRunResultNamedPipe((payload) => received.push(payload), deferredTillServerClose, cancelSource.token); assert.ok(createReaderPipeStub.calledOnce, 'createReaderPipe should be called once'); assert.ok(reader.hasListener(), 'reader should have a listener registered before cancel'); @@ -868,11 +875,7 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { const deferredTillServerClose: Deferred = createDeferred(); const cancelSource = new CancellationTokenSource(); - await startRunResultNamedPipe( - (payload) => received.push(payload), - deferredTillServerClose, - cancelSource.token, - ); + await startRunResultNamedPipe((payload) => received.push(payload), deferredTillServerClose, cancelSource.token); // Simulate the debug-path race: cancel fires while results are still buffered. cancelSource.cancel(); From c7694eb57dde0360a1a587334c2a04b6996381f6 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 7 Jun 2026 20:12:59 -0700 Subject: [PATCH 7/8] Add bounded drain timeout backstop for result pipe The pure event-driven drain in this PR can hang the adapter's finally block forever if reader.onClose never fires - which happens in CI on the segfault/coverage/large-workspace execution tests (abnormal subprocess exit, platform-specific named-pipe quirks). Add awaitDeferredWithTimeout helper and use it in both unittest and pytest execution adapters' finally blocks. By the time we hit finally, runTestsNew has already awaited the subprocess 'exit' event, so a few seconds is plenty for any buffered pipe data to flush. The timeout acts purely as a backstop - the normal path still resolves promptly via reader.onClose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../testing/testController/common/utils.ts | 28 +++++++++++++++++++ .../pytest/pytestExecutionAdapter.ts | 2 +- .../unittest/testExecutionAdapter.ts | 2 +- .../testing/testController/utils.unit.test.ts | 23 +++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index a791e563ef5d..9591a57e6627 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -28,6 +28,34 @@ export function createTestingDeferred(): Deferred { return createDeferred(); } +// Maximum time (ms) to wait for the result pipe to drain after the subprocess exits. +// Acts as a backstop in case `reader.onClose` never fires (e.g. abnormal subprocess exit, +// platform-specific named-pipe quirks) so the adapter's `finally` block can never hang. +export const RESULT_PIPE_DRAIN_TIMEOUT_MS = 5_000; + +/** + * Awaits `deferred.promise` but resolves after at most `timeoutMs` so callers + * cannot hang indefinitely if the underlying event source never fires. + */ +export async function awaitDeferredWithTimeout(deferred: Deferred, timeoutMs: number): Promise { + let timeoutHandle: NodeJS.Timeout | undefined; + try { + await Promise.race([ + deferred.promise, + new Promise((resolve) => { + timeoutHandle = setTimeout(() => { + traceVerbose(`awaitDeferredWithTimeout: timed out after ${timeoutMs}ms; resolving anyway.`); + resolve(); + }, timeoutMs); + }), + ]); + } finally { + if (timeoutHandle) { + clearTimeout(timeoutHandle); + } + } +} + interface ExecutionResultMessage extends Message { params: ExecutionTestPayload; } diff --git a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts index 102841c2e2dd..5215a7248630 100644 --- a/src/client/testing/testController/pytest/pytestExecutionAdapter.ts +++ b/src/client/testing/testController/pytest/pytestExecutionAdapter.ts @@ -76,7 +76,7 @@ export class PytestTestExecutionAdapter implements ITestExecutionAdapter { project, ); } finally { - await deferredTillServerClose.promise; + await utils.awaitDeferredWithTimeout(deferredTillServerClose, utils.RESULT_PIPE_DRAIN_TIMEOUT_MS); } } diff --git a/src/client/testing/testController/unittest/testExecutionAdapter.ts b/src/client/testing/testController/unittest/testExecutionAdapter.ts index 8acc4c0dfed8..8077f4fef662 100644 --- a/src/client/testing/testController/unittest/testExecutionAdapter.ts +++ b/src/client/testing/testController/unittest/testExecutionAdapter.ts @@ -90,7 +90,7 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter { } catch (error) { traceError(`Error in running unittest tests: ${error}`); } finally { - await deferredTillServerClose.promise; + await utils.awaitDeferredWithTimeout(deferredTillServerClose, utils.RESULT_PIPE_DRAIN_TIMEOUT_MS); } } diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index df4bb6c132de..5190c5b42eb9 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -16,6 +16,7 @@ import { writeTestIdsFile, populateTestTree, startRunResultNamedPipe, + awaitDeferredWithTimeout, } from '../../../client/testing/testController/common/utils'; import { createDeferred, Deferred } from '../../../client/common/utils/async'; import * as namedPipes from '../../../client/common/pipes/namedPipes'; @@ -927,3 +928,25 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { assert.strictEqual(reader.disposed, true, 'reader disposed when onClose fires'); }); }); + +suite('awaitDeferredWithTimeout', () => { + test('resolves promptly when the deferred resolves before the timeout', async () => { + const deferred = createDeferred(); + const started = Date.now(); + const waiter = awaitDeferredWithTimeout(deferred, 5000); + setTimeout(() => deferred.resolve(), 10); + await waiter; + const elapsed = Date.now() - started; + assert.ok(elapsed < 1000, `should resolve well before timeout, took ${elapsed}ms`); + }); + + test('resolves after the timeout when the deferred never settles (no hang)', async () => { + const deferred = createDeferred(); + const started = Date.now(); + await awaitDeferredWithTimeout(deferred, 50); + const elapsed = Date.now() - started; + assert.ok(elapsed >= 50, `should wait at least the timeout, took ${elapsed}ms`); + assert.ok(elapsed < 2000, `should not hang well beyond timeout, took ${elapsed}ms`); + assert.strictEqual(deferred.completed, false, 'underlying deferred remains unresolved'); + }); +}); From bbef6e29f0c089379e911e5d3a0e5b9d71ead93d Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Sun, 7 Jun 2026 23:39:15 -0700 Subject: [PATCH 8/8] Address Copilot review feedback - Use traceInfo instead of console.log in unittest cancellation handler (consistent with other test controller adapters; routes through the extension output channel). - Clarify awaitDeferredWithTimeout log message: the underlying deferred is not actually resolved on timeout, the helper just stops waiting. - Fill in required ExecutionTestPayload fields (status, error) in the test helper so emitted messages match the real runner schema. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/client/testing/testController/common/utils.ts | 4 +++- .../testController/unittest/testExecutionAdapter.ts | 2 +- src/test/testing/testController/utils.unit.test.ts | 10 +++++++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts index 9591a57e6627..583d57648548 100644 --- a/src/client/testing/testController/common/utils.ts +++ b/src/client/testing/testController/common/utils.ts @@ -44,7 +44,9 @@ export async function awaitDeferredWithTimeout(deferred: Deferred, timeout deferred.promise, new Promise((resolve) => { timeoutHandle = setTimeout(() => { - traceVerbose(`awaitDeferredWithTimeout: timed out after ${timeoutMs}ms; resolving anyway.`); + traceVerbose( + `awaitDeferredWithTimeout: deferred did not settle within ${timeoutMs}ms; giving up and continuing.`, + ); resolve(); }, timeoutMs); }), diff --git a/src/client/testing/testController/unittest/testExecutionAdapter.ts b/src/client/testing/testController/unittest/testExecutionAdapter.ts index 8077f4fef662..755da2e3c65a 100644 --- a/src/client/testing/testController/unittest/testExecutionAdapter.ts +++ b/src/client/testing/testController/unittest/testExecutionAdapter.ts @@ -70,7 +70,7 @@ export class UnittestTestExecutionAdapter implements ITestExecutionAdapter { cSource.token, // token to cancel ); runInstance.token.onCancellationRequested(() => { - console.log(`Test run cancelled for ${uri.fsPath}; waiting for result pipe to drain.`); + traceInfo(`Test run cancelled for ${uri.fsPath}; waiting for result pipe to drain.`); // Don't resolve the deferred here: the pipe must drain first. // `reader.onClose` in `startRunResultNamedPipe` will resolve it // once the subprocess closes its end of the pipe. diff --git a/src/test/testing/testController/utils.unit.test.ts b/src/test/testing/testController/utils.unit.test.ts index 5190c5b42eb9..0fae5ffbcee2 100644 --- a/src/test/testing/testController/utils.unit.test.ts +++ b/src/test/testing/testController/utils.unit.test.ts @@ -831,7 +831,15 @@ suite('startRunResultNamedPipe drain-on-cancel tests', () => { }); function makeMessage(payload: Partial): Message { - return ({ jsonrpc: '2.0', params: payload } as unknown) as Message; + // Fill in required ExecutionTestPayload fields so tests exercise a shape close + // to what real runners send and don't drift from the schema over time. + const full: ExecutionTestPayload = { + cwd: '', + status: 'success', + error: '', + ...payload, + }; + return ({ jsonrpc: '2.0', params: full } as unknown) as Message; } test('cancellation alone does NOT resolve deferredTillServerClose and does NOT detach the listener (drain not interrupted)', async () => {