diff --git a/packages/vscode/src/stacks/test/master.ts b/packages/vscode/src/stacks/test/master.ts index ad35d21..19816bf 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 { statSync } from 'node:fs'; import net from 'node:net'; import path, { dirname } from 'node:path'; import { type BirpcReturn, createBirpc } from 'birpc'; @@ -643,6 +644,25 @@ 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`; + } + + // 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, @@ -654,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(); @@ -732,9 +766,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 +794,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 (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. + 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..ba91a11 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,98 @@ 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-')); + // 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(reported).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', + 'console.log("worker-up"); setInterval(() => {}, 1000)', + ]; + api = createApi(packageDir); + + await api.createChildProcess(); + const child = [...((api as any).childProcesses as Set)][0]!; + // 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.stdout?.on('data', () => resolve()); + }); + + child.emit('error', new Error('write EPIPE')); + + expect(shownMessages).toEqual([]); + expect(crashes()).toEqual([]); + }); +});