From 9b68aaee6a387507fbda644e3f4f96712e03ff0d Mon Sep 17 00:00:00 2001 From: fi3ework Date: Mon, 24 Aug 2026 19:14:17 +0800 Subject: [PATCH 1/3] fix(vscode): keep worker teardown races out of notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two intermittent error notifications traced to the rstest worker's child-process 'error' handler, which treated every 'error' event as a spawn failure the user must fix: - "write EPIPE": a birpc message racing the worker's death. send() was called without a callback, so a lost race became an 'error' event and a notification. Pass a callback and log instead — the 'exit' handler already owns reporting an exit nobody asked for. - "spawn node ENOENT": Node blames the executable when it is the spawn cwd that is gone (a project directory deleted under a live master by a branch switch or a build wiping fixtures). That is a stale-project state detection will reconcile, not a broken runtime: log the real cause, skip the notification and the crashed status. A genuine spawn failure with the cwd intact — the wrong-nodeExecutable case the notification exists for — keeps notifying. Post-spawn 'error' events are absorbed, matching LanguageServerProcessOwner's shape in the lint/fmt stacks. --- packages/vscode/src/stacks/test/master.ts | 68 ++++++++--- .../vscode/tests/stacks/test/master.test.ts | 107 ++++++++++++++++-- 2 files changed, 149 insertions(+), 26 deletions(-) diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index ad35d21..d51a043 100644 --- a/packages/vscode/src/stacks/test/master.ts +++ b/packages/vscode/src/stacks/test/master.ts @@ -1,4 +1,5 @@ import { type ChildProcess, spawn } from 'node:child_process'; +import { existsSync } from 'node:fs'; import net from 'node:net'; import path, { dirname } from 'node:path'; import { type BirpcReturn, createBirpc } from 'birpc'; @@ -643,6 +644,12 @@ export class RstestApi { }); } + // One wording for the pre-spawn guard and the 'error'-handler fallback it + // leaves for the delete-after-check race. + private missingCwdMessage(): string { + return `worker spawn skipped: project directory ${this.cwd} no longer exists; the project will be re-detected if it comes back`; + } + public async createChildProcess( testRunReporter = new TestRunReporter(), startDebugging?: boolean, @@ -689,6 +696,18 @@ export class RstestApi { 'worker spawn aborted: this master was disposed while its Node runtime was being resolved', ); } + // A project directory deleted under a live master (branch switch, + // `git clean`, a build wiping fixtures) is a stale-project state, not a + // runtime the user must fix: detection watches the config file and will + // drop or re-add the project. Refusing here, before the spawn, spares + // the caller a worker whose every call rejects with an opaque + // "[birpc] rpc is closed" — and spares the user Node's misreading of a + // missing cwd as "spawn node ENOENT". + if (!existsSync(this.cwd)) { + const message = this.missingCwdMessage(); + logger.warn(message); + throw new Error(message); + } const nodeEnv = getConfigValue('nodeEnv', this.workspace); const debugNodeEnv = startDebugging ? getConfigValue('debugNodeEnv', this.workspace) @@ -732,9 +751,15 @@ export class RstestApi { const worker = createBirpc(testRunReporter, { // Target the local process rather than the shared field, which is - // reassigned on every spawn; skip once the IPC channel is gone. + // reassigned on every spawn; skip once the IPC channel is gone. The + // callback matters: without one, Node surfaces a failed write — a + // message losing the race against the worker's death — as a process + // 'error' event instead. post: (data) => { - if (rstestProcess.connected) rstestProcess.send(data); + if (rstestProcess.connected) + rstestProcess.send(data, (error) => { + if (error) logger.debug('IPC send to worker failed', error); + }); }, on: (fn) => rstestProcess.on('message', fn), bind: 'functions', @@ -754,23 +779,38 @@ export class RstestApi { configFilePath: this.configFilePath, }); + let spawned = false; rstestProcess.on('spawn', () => { + spawned = true; status.workerSpawned(this.statusSource); }); rstestProcess.on('error', (error) => { - logger.error('Worker process error', error); - // The status-aggregation adaptation: a worker that never came up is the - // `crashed` state of the shared status bar. The notification is kept because - // a failed spawn is almost always a wrong `nodeExecutable` the user has - // to fix, and the status bar alone is easy to miss mid-run. - status.crashed( - `worker process failed: ${error.message}`, - this.statusSource, - ); - vscode.window.showErrorMessage( - `Rstest worker process failed: ${error.message}`, - ); + if (spawned) { + // Post-spawn errors (a failed kill(), an IPC write losing the race + // against the worker's death) are teardown noise with nothing for + // the user to fix; the 'exit' handler already reports an exit nobody + // asked for. Same shape as `LanguageServerProcessOwner`'s handler. + logger.debug('Worker process error after spawn', error); + } else if (!existsSync(this.cwd)) { + // The cwd was deleted between the pre-spawn guard and the spawn — + // Node blames the executable ("spawn node ENOENT") when it is the + // cwd that is gone. Same stale-project state, same quiet report. + logger.warn(this.missingCwdMessage()); + } else { + logger.error('Worker process error', error); + // The status-aggregation adaptation: a worker that never came up is the + // `crashed` state of the shared status bar. The notification is kept because + // a failed spawn is almost always a wrong `nodeExecutable` the user has + // to fix, and the status bar alone is easy to miss mid-run. + status.crashed( + `worker process failed: ${error.message}`, + this.statusSource, + ); + vscode.window.showErrorMessage( + `Rstest worker process failed: ${error.message}`, + ); + } // Reject any in-flight birpc calls instead of letting them hang; $close // runs the `off` handler, which removes the process from the Set. if (!worker.$closed) worker.$close(); diff --git a/packages/vscode/tests/stacks/test/master.test.ts b/packages/vscode/tests/stacks/test/master.test.ts index 8d024d0..56bc483 100644 --- a/packages/vscode/tests/stacks/test/master.test.ts +++ b/packages/vscode/tests/stacks/test/master.test.ts @@ -1,3 +1,4 @@ +import type { ChildProcess } from 'node:child_process'; import fs from 'node:fs'; import { createRequire } from 'node:module'; import os from 'node:os'; @@ -11,7 +12,7 @@ import { resetUserNodeCaches, } from '../../../src/shared/nodeResolution'; import { status } from '../../../src/stacks/test/status'; -import type { StatusReporter } from '../../../src/types'; +import type { StackState, StatusReporter } from '../../../src/types'; import { createStatusRecorder } from './statusRecorder'; // The Rstest runner injects its own `@rstest/core` into every resolution path so @@ -117,6 +118,18 @@ rs.mock('vscode', () => { // the workspace `node_modules` and `@rstest/core` is genuinely missing. const noCoreDir = os.tmpdir(); +// The opposite fixture: a cwd where `@rstest/core` resolves, for suites whose +// case under test sits past the resolution step. +const packageDir = path.resolve(__dirname, '../../..'); + +// Seeding the memo is how the probe is injected: `resolveWorkerNodeCommand` +// takes no probe option (it is called from deep inside a spawn path), and the +// memo is keyed by executable path, so a seeded entry is the answer it gets. +const seedNodeProbe = (executable: string, probe: NodeProbe) => + configuredNodeBelowFloor(executable, { + probe: () => Promise.resolve(probe), + }); + // Settings are a module-level bag every suite writes into; clearing them per // test keeps one suite's configuration from leaking into the next. afterEach(() => { @@ -357,13 +370,7 @@ describe('RstestApi with a configured nodeExecutable', () => { versionMismatch: (detail) => mismatches.push(detail), }; - // Seeding the memo is how the probe is injected: `resolveWorkerNodeCommand` - // takes no probe option (it is called from deep inside a spawn path), and the - // memo is keyed by executable path, so a seeded entry is the answer it gets. - const seedProbe = (probe: NodeProbe) => - configuredNodeBelowFloor(configuredNode, { - probe: () => Promise.resolve(probe), - }); + const seedProbe = (probe: NodeProbe) => seedNodeProbe(configuredNode, probe); // Reaching the private method keeps these cases on the decision under test // instead of spawning a real worker process for each one. @@ -438,13 +445,89 @@ describe('RstestApi with a configured nodeExecutable', () => { it('should refuse to spawn a worker after dispose', async () => { // The seed keeps the configured executable's probe off the real spawn - // path; the package dir (not `process.cwd()`) is a cwd where - // `@rstest/core` resolves, so the abort observed is the disposed check - // and not an earlier resolution failure. + // path, so the abort observed is the disposed check and not an earlier + // resolution failure. await seedProbe({ kind: 'ok', version: '24.0.0' }); - const api = createApi(path.resolve(__dirname, '../../..')); + const api = createApi(packageDir); const spawning = api.createChildProcess(); api.dispose(); await expect(spawning).rejects.toThrow('disposed'); }); }); + +// Worker spawn failures: only the wrong-executable case is the user's to fix +// (and keeps its notification); the rest is absorbed — the rationale lives on +// the guard and the 'error' handler in `master.ts`. +describe('RstestApi worker spawn failures', () => { + let api: RstestApi | undefined; + let reported: StackState[]; + + const crashes = () => reported.filter((state) => state.kind === 'crashed'); + + const seedConfiguredNode = (executable: string) => { + settings['rstack.nodeExecutable'] = executable; + return seedNodeProbe(executable, { kind: 'ok', version: '24.0.0' }); + }; + + beforeEach(() => { + shownMessages.length = 0; + loggedWarnings.length = 0; + resetUserNodeCaches(); + const recorder = createStatusRecorder(); + reported = recorder.reported; + status.bind(recorder.reporter); + }); + + afterEach(() => { + api?.dispose(); + api = undefined; + status.unbind(); + resetUserNodeCaches(); + }); + + it('should log, not notify, when the spawn cwd no longer exists', async () => { + await seedConfiguredNode(process.execPath); + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'rstest-gone-')); + api = createApi(cwd, packageDir); + fs.rmSync(cwd, { recursive: true, force: true }); + + await expect(api.createChildProcess()).rejects.toThrow('no longer exists'); + + expect(shownMessages).toEqual([]); + expect(crashes()).toEqual([]); + expect(loggedWarnings.join('\n')).toContain(cwd); + }); + + it('should keep notifying when the executable itself fails to spawn', async () => { + await seedConfiguredNode(path.join(os.tmpdir(), 'no-such-node-xyz')); + api = createApi(packageDir); + + await api.createChildProcess(); + await expect + .poll(() => shownMessages[0] ?? '', { timeout: 5000 }) + .toContain('Rstest worker process failed'); + + expect(crashes()).toHaveLength(1); + }); + + it('should absorb a post-spawn error instead of notifying', async () => { + await seedConfiguredNode(process.execPath); + // `--eval` wins over the worker script path, so the child is a plain + // long-lived node — the point is the handler, not the worker protocol. + settings.nodeExecArgs = ['--eval', 'setInterval(() => {}, 1000)']; + api = createApi(packageDir); + + await api.createChildProcess(); + const child = [...((api as any).childProcesses as Set)][0]!; + // The handler's spawned latch is set by the master's own 'spawn' + // listener, which registered first and therefore runs first. + await new Promise((resolve) => { + child.once('spawn', () => resolve()); + }); + + child.emit('error', new Error('write EPIPE')); + + expect(shownMessages).toEqual([]); + expect(crashes()).toEqual([]); + }); +}); From f643b0ea492bf0ceb425fac9f257c43db08a1c11 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Tue, 25 Aug 2026 11:06:44 +0800 Subject: [PATCH 2/3] fix(vscode): tighten the missing-cwd classification on worker spawns Two review findings on the pre-spawn guard: - The guard ran after package resolution, so a deleted project directory was first misread as "@rstest/core is not installed" (or notified for a rstestPackagePath inside it). It now sits directly after the disposed fast-fail, before any resolution. - existsSync also returns false for a cwd hidden by a permission failure (EACCES on it or an ancestor), which would have absorbed a genuine, user-actionable spawn failure. The check now stats the cwd and treats only ENOENT/ENOTDIR as "really not there". --- packages/vscode/src/stacks/test/master.ts | 43 +++++++++++++------ .../vscode/tests/stacks/test/master.test.ts | 7 ++- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index d51a043..19816bf 100644 --- a/packages/vscode/src/stacks/test/master.ts +++ b/packages/vscode/src/stacks/test/master.ts @@ -1,5 +1,5 @@ import { type ChildProcess, spawn } from 'node:child_process'; -import { existsSync } from 'node:fs'; +import { statSync } from 'node:fs'; import net from 'node:net'; import path, { dirname } from 'node:path'; import { type BirpcReturn, createBirpc } from 'birpc'; @@ -650,6 +650,19 @@ export class RstestApi { return `worker spawn skipped: project directory ${this.cwd} no longer exists; the project will be re-detected if it comes back`; } + // Deliberately narrower than `!existsSync`: a cwd hidden by a permission + // failure (EACCES on it or an ancestor) is a state the user must fix, so it + // has to keep failing loudly — only "really not there" may be absorbed. + private cwdIsGone(): boolean { + try { + statSync(this.cwd); + return false; + } catch (error) { + const { code } = error as NodeJS.ErrnoException; + return code === 'ENOENT' || code === 'ENOTDIR'; + } + } + public async createChildProcess( testRunReporter = new TestRunReporter(), startDebugging?: boolean, @@ -661,6 +674,20 @@ export class RstestApi { if (this.disposed) { throw new Error('worker spawn aborted: this master is disposed'); } + // A project directory deleted under a live master (branch switch, + // `git clean`, a build wiping fixtures) is a stale-project state, not a + // runtime the user must fix: detection watches the config file and will + // drop or re-add the project. Refusing here — before package resolution, + // which would misread the deleted directory as "@rstest/core is not + // installed" (or notify for a `rstestPackagePath` inside it) — spares + // the caller a worker whose every call rejects with an opaque + // "[birpc] rpc is closed", and spares the user Node's misreading of a + // missing cwd as "spawn node ENOENT". + if (this.cwdIsGone()) { + const message = this.missingCwdMessage(); + logger.warn(message); + throw new Error(message); + } const rstestPath = this.resolveRstestPath(); if (!rstestPath) { throw new ReportedRstestResolutionError(); @@ -696,18 +723,6 @@ export class RstestApi { 'worker spawn aborted: this master was disposed while its Node runtime was being resolved', ); } - // A project directory deleted under a live master (branch switch, - // `git clean`, a build wiping fixtures) is a stale-project state, not a - // runtime the user must fix: detection watches the config file and will - // drop or re-add the project. Refusing here, before the spawn, spares - // the caller a worker whose every call rejects with an opaque - // "[birpc] rpc is closed" — and spares the user Node's misreading of a - // missing cwd as "spawn node ENOENT". - if (!existsSync(this.cwd)) { - const message = this.missingCwdMessage(); - logger.warn(message); - throw new Error(message); - } const nodeEnv = getConfigValue('nodeEnv', this.workspace); const debugNodeEnv = startDebugging ? getConfigValue('debugNodeEnv', this.workspace) @@ -792,7 +807,7 @@ export class RstestApi { // the user to fix; the 'exit' handler already reports an exit nobody // asked for. Same shape as `LanguageServerProcessOwner`'s handler. logger.debug('Worker process error after spawn', error); - } else if (!existsSync(this.cwd)) { + } else if (this.cwdIsGone()) { // The cwd was deleted between the pre-spawn guard and the spawn — // Node blames the executable ("spawn node ENOENT") when it is the // cwd that is gone. Same stale-project state, same quiet report. diff --git a/packages/vscode/tests/stacks/test/master.test.ts b/packages/vscode/tests/stacks/test/master.test.ts index 56bc483..d801518 100644 --- a/packages/vscode/tests/stacks/test/master.test.ts +++ b/packages/vscode/tests/stacks/test/master.test.ts @@ -488,13 +488,16 @@ describe('RstestApi worker spawn failures', () => { it('should log, not notify, when the spawn cwd no longer exists', async () => { await seedConfiguredNode(process.execPath); const cwd = fs.mkdtempSync(path.join(os.tmpdir(), 'rstest-gone-')); - api = createApi(cwd, packageDir); + // Default resolution anchors at the (deleted) cwd on purpose: the guard + // must fire before package resolution, which would otherwise misread the + // deleted directory as "@rstest/core is not installed". + api = createApi(cwd); fs.rmSync(cwd, { recursive: true, force: true }); await expect(api.createChildProcess()).rejects.toThrow('no longer exists'); expect(shownMessages).toEqual([]); - expect(crashes()).toEqual([]); + expect(reported).toEqual([]); expect(loggedWarnings.join('\n')).toContain(cwd); }); From c1d6a7b3d12113620799b8e5f183847e45d7e507 Mon Sep 17 00:00:00 2001 From: fi3ework Date: Tue, 25 Aug 2026 11:13:44 +0800 Subject: [PATCH 3/3] test(vscode): make the post-spawn handshake race-proof MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test awaited the child's 'spawn' event from a listener attached after createChildProcess() resolved; Node gives no timing guarantee for 'spawn' relative to that continuation, so a prompt emit could leave the promise pending until the suite timeout. Await the child's first stdout chunk instead: stream data is buffered until a listener attaches, and 'spawn' — which sets the handler's latch — precedes all other events. --- packages/vscode/tests/stacks/test/master.test.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/vscode/tests/stacks/test/master.test.ts b/packages/vscode/tests/stacks/test/master.test.ts index d801518..ba91a11 100644 --- a/packages/vscode/tests/stacks/test/master.test.ts +++ b/packages/vscode/tests/stacks/test/master.test.ts @@ -517,15 +517,21 @@ describe('RstestApi worker spawn failures', () => { await seedConfiguredNode(process.execPath); // `--eval` wins over the worker script path, so the child is a plain // long-lived node — the point is the handler, not the worker protocol. - settings.nodeExecArgs = ['--eval', 'setInterval(() => {}, 1000)']; + settings.nodeExecArgs = [ + '--eval', + 'console.log("worker-up"); setInterval(() => {}, 1000)', + ]; api = createApi(packageDir); await api.createChildProcess(); const child = [...((api as any).childProcesses as Set)][0]!; - // The handler's spawned latch is set by the master's own 'spawn' - // listener, which registered first and therefore runs first. + // Await the child's first stdout chunk, not the 'spawn' event: Node gives + // no timing guarantee for 'spawn' relative to this continuation, while + // stream data is buffered until a listener attaches — and 'spawn' (which + // precedes all other events, setting the handler's latch) is guaranteed + // delivered by the time data flows. await new Promise((resolve) => { - child.once('spawn', () => resolve()); + child.stdout?.on('data', () => resolve()); }); child.emit('error', new Error('write EPIPE'));