From 7f2af286392573faa2ad3c086cc7f91c9b1e6875 Mon Sep 17 00:00:00 2001 From: sim Date: Fri, 28 Aug 2026 19:20:11 -0400 Subject: [PATCH] fix(#4012): the reporter sink must be a regular file, not devNull MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third failure on this feature, and this one broke everything rather than just the diagnostic: 43 failures across every run-tests.cjs invocation. Error: EINVAL: invalid argument, fsync Emitted 'error' event on WriteStream instance Node opens a WriteStream for a --test-reporter-destination and FSYNCS it on close. fsync on /dev/null is EINVAL — it is a character device, not a regular file. So os.devNull is not a usable reporter destination at all, and every chunk crashed on exit. The sink only ever needed to be a regular file that stays empty, since the reporter writes its real output through appendFileSync to the path in GSD_RUN_TESTS_EVENTS_FILE. It is now one fixed file inside the existing events dir, pre-created rather than relying on the stream's create-on-open, and left empty by design. One sink for the whole run, not per chunk, so its path length stays constant — reporterOverhead feeds FIXED_OVERHEAD, which is computed once before chunking, and a variable-length path would silently mis-account the Windows argv ceiling. The comment that named devNull now says why the destination must be a regular file, so this does not get re-optimized back into the same crash. Reaching for devNull was the mistake: it looks like the obviously correct way to discard output, and it is, for a pipe or an fd — but not for something Node is going to fsync. Each of the three failures on this feature was a different edge of the same assumption, that a reporter destination behaves like ordinary output. The regression test asserts the closest externally observable consequence — a normal run must not surface the EINVAL/fsync text. The argv construction lives inside main() with no exported seam, and the sink is swept before a test could stat it; adding a seam purely to assert that is left out rather than reshaping production code for the test. Stated plainly rather than implied. The failing T1 is untouched and still red. Verification runs on the remote runner. Refs #4012 --- scripts/run-tests.cjs | 19 ++++++++++++++++--- tests/run-tests-harness.test.cjs | 25 +++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/scripts/run-tests.cjs b/scripts/run-tests.cjs index 83f715ccb..2828dc205 100644 --- a/scripts/run-tests.cjs +++ b/scripts/run-tests.cjs @@ -35,9 +35,9 @@ // See docs/TESTING-SUITES.md for full grouping policy. 'use strict'; -const { readdirSync, readFileSync, mkdtempSync, rmSync, unlinkSync } = require('fs'); +const { readdirSync, readFileSync, mkdtempSync, rmSync, unlinkSync, writeFileSync } = require('fs'); const { join, basename } = require('path'); -const { tmpdir, devNull } = require('os'); +const { tmpdir } = require('os'); const { execFileSync } = require('child_process'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); const { @@ -1079,11 +1079,24 @@ function main() { // (GSD_RUN_TESTS_EVENTS_FILE, set per-chunk below in execFileSync's `env`), // which does NOT count toward the Windows 32,767-char argv ceiling — only // this fixed sink destination does. + // + // The ndjson reporter yields nothing and ignores its own destination — the + // durable write path is GSD_RUN_TESTS_EVENTS_FILE above — but Node still + // opens this destination as a real fs.WriteStream and fsyncs it on close, + // so it MUST be a regular file. os.devNull (a character device on every + // platform) fails that fsync with EINVAL, crashing every chunk, not just + // the timeout path (confirmed live: 43/43 chunk failures, "EINVAL: invalid + // argument, fsync" on WriteStream close). Do not re-point this at devNull — + // it is the ONE destination flavor this feature cannot use. Pre-created + // once, alongside eventsDir, so it stays empty and its path length is + // identical across every chunk (see FIXED_OVERHEAD below). + const reporterSinkPath = join(eventsDir, 'reporter-sink.txt'); + writeFileSync(reporterSinkPath, ''); const reporterArgs = [ `--test-reporter=${humanReporter}`, '--test-reporter-destination=stdout', `--test-reporter=${reporterModulePath}`, - `--test-reporter-destination=${devNull}`, + `--test-reporter-destination=${reporterSinkPath}`, ]; const reporterOverhead = reporterArgs.reduce((sum, a) => sum + a.length + 1, 0); diff --git a/tests/run-tests-harness.test.cjs b/tests/run-tests-harness.test.cjs index 37a34417c..68d856bfc 100644 --- a/tests/run-tests-harness.test.cjs +++ b/tests/run-tests-harness.test.cjs @@ -852,6 +852,31 @@ test('noop', () => {}); ); }); + // T5 (regression): the human reporter's --test-reporter-destination + // pairing must be a regular file, not os.devNull. devNull is a character + // device; Node opens the reporter destination as an fs.WriteStream and + // fsyncs it on close, and fsync on a character device fails with EINVAL + // — surfaced as "Emitted 'error' event on WriteStream instance" / + // "EINVAL: invalid argument, fsync", which crashed EVERY chunk on the + // real remote run this regresses (43/43 failures), not only the timeout + // path. The argv construction lives entirely inside main() with no + // exported seam to unit-test directly (see NOTES), so this asserts the + // closest real, externally-observable consequence: a normal successful + // run must not surface that error text, and must still complete and + // exit 0 — both of which a reintroduced devNull destination would break + // on any platform where fsync(devNull) actually returns EINVAL (this + // suite's own bench platform, historically). + test('a successful run never surfaces the devNull fsync/EINVAL reporter crash', () => { + seed(tmpDir, ['a.test.cjs']); + const r = runHarness(tmpDir, []); + assert.strictEqual(r.status, 0, `expected a clean pass; STDERR:\n${r.stderr}`); + assert.doesNotMatch( + r.stderr, + /EINVAL|invalid argument, fsync|WriteStream instance/i, + `expected no reporter-destination fsync crash; STDERR:\n${r.stderr}`, + ); + }); + // T1: on a chunk timeout, the diagnostic must NAME the file that was // still executing — not merely list every file the chunk contained (the // pre-instrumentation behavior). A test that hangs INSIDE its own body