fix(#4012): the reporter sink must be a regular file, not devNull

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
This commit is contained in:
sim
2026-08-28 19:20:11 -04:00
parent 0abd137ec7
commit 7f2af28639
2 changed files with 41 additions and 3 deletions

View File

@@ -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);

View File

@@ -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