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/5] 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/5] 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/5] 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/5] 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/5] 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;