diff --git a/scripts/check-test-count.js b/scripts/check-test-count.js index b331c9db..ee9c58cc 100644 --- a/scripts/check-test-count.js +++ b/scripts/check-test-count.js @@ -61,11 +61,14 @@ // var TEST_COUNT_FLOOR = 1300; +var fs = require("fs"); var path = require("path"); var { spawnSync } = require("child_process"); // --------------------------------------------------------------------------- -// CI legibility (lr-e551b9, MILLER re-diagnosis of PR #400 / lr-4e1242 seq 5). +// CI legibility (lr-e551b9, MILLER re-diagnosis of PR #400 / lr-4e1242 seq 5; +// re-diagnosed again post-merge, same task, when the shipped fix turned out +// not to survive a real CI pipe — see DELIVERY below). // // GitHub Actions does not surface arbitrary stderr text anywhere a crew // agent can reach it: the raw job log requires following a redirect to a @@ -77,7 +80,100 @@ var { spawnSync } = require("child_process"); // agent can actually read it, without changing which conditions fail the // run (see the module header above: every condition that fails today must // keep failing). +// +// DELIVERY (lr-e551b9 reopen, MILLER third-pass diagnosis, confidence 0.93, +// reproduced locally and in CI on PR #400's rebased head): the first version +// of this function used `process.stdout.write`. When stdout is a PIPE +// (always true in CI, and in any captured run), that write is ASYNCHRONOUS — +// it queues in userspace once the 64KB kernel pipe buffer fills. The +// multi-MB TAP dump this script also writes to stdout vastly exceeds that +// buffer, so by the time emitAnnotation() ran, its own write was queued +// BEHIND the still-draining TAP blob. process.exit() then tore the process +// down without draining the queue, so the annotation reached the Writable +// stream object and never reached the file descriptor: exit code 1 with NO +// annotation, indistinguishable from a genuine test failure at the very +// point this script exists to make that distinguishable. +// +// Fixed by writing the annotation with fs.writeSync(1, ...) instead of +// process.stdout.write(...). writeSync is a direct, synchronous syscall to +// the fd — it is not subject to the Writable stream's internal async queue +// at all, so there is no buffer to race process.exit() against. This is +// belt: it holds even if something upstream changes how/when the process +// terminates. See below for suspenders (annotation ordered before the large +// stdout dump; process.exitCode instead of process.exit()). // --------------------------------------------------------------------------- +// writeFullySync(fd, data) — fs.writeSync performs exactly ONE write(2) +// syscall attempt and returns the number of bytes actually written; it does +// NOT loop to guarantee the whole buffer lands, and a pipe fd can legitimately +// short-write under backpressure once the payload is large (BOBBIE, PR #402 +// comment 5360992190). The single unlooped call this replaced discarded that +// return value entirely, so a short write silently truncated its output — +// exactly the class of silent-truncation defect this whole task (lr-e551b9) +// exists to eliminate, now against the raw TAP evidence trail instead of the +// verdict. This loops until every byte is confirmed written. +// +// `data` may be a Buffer or a string; if it is a string it is converted to a +// Buffer ONCE up front, and the loop advances over that Buffer by byte +// offset. This matters because fs.writeSync's offset/length/return-value +// semantics are BYTE-indexed always, but a JS string is indexed by UTF-16 +// code unit — slicing a string by a byte count returned from a short write +// would misalign mid multi-byte UTF-8 sequence. Converting once avoids that +// mismatch entirely rather than trying to reconcile the two index spaces on +// every partial-write iteration. +// +// Any thrown error (e.g. EPIPE/EAGAIN) propagates uncaught — deliberately not +// swallowed here. This function is called from FAIL-path code whose only job +// is to make a non-zero exit legible; the exit code itself is already +// determined by classifyRun() before either write site runs (see +// process.exitCode below), so an uncaught throw here can only ever turn an +// already-nonzero process into a hard crash (still non-zero), never a FAIL +// verdict into exit 0. Swallowing the error here would risk exactly that. +// +// NON-ADVANCING WRITE GUARD (lr-e551b9 fold-in, PEACHES PR #402 BLOCKING). +// fs.writeSync's return value is the ONLY thing that advances `offset`; the +// Node docs do not rule out a 0-byte return (distinct from throwing EAGAIN), +// and this loop's sole invariant is that it always terminates. An unguarded +// `while (offset < buffer.length)` against a 0-byte return spins forever — +// on this specific script, the merge-gate wrapper whose entire purpose is +// turning a FAIL into a legible signal, an infinite loop here is strictly +// worse than the truncation bug it replaced: truncation still failed fast +// with a usable exit code, while a hang burns the runner until the workflow +// timeout kills it and produces NO verdict and NO annotation at all. +// +// A single 0-byte return is not necessarily fatal — a transient EAGAIN-like +// stall on a pipe under backpressure could plausibly resolve on the very +// next attempt without ever surfacing as a thrown error. But an unbounded +// RUN of 0-byte returns with no forward progress is exactly the spin this +// guard exists to rule out. MAX_CONSECUTIVE_ZERO_WRITES bounds that run: +// each 0-byte return increments a counter; any return >0 resets it; hitting +// the bound throws. Throwing (not returning/silently giving up) matches the +// existing EPIPE/EAGAIN posture directly above — an uncaught throw always +// exits non-zero, so it can never produce a false green, and it surfaces +// immediately in CI as a hard crash rather than a silent short-delivery. +var MAX_CONSECUTIVE_ZERO_WRITES = 100; + +function writeFullySync(fd, data) { + var buffer = Buffer.isBuffer(data) ? data : Buffer.from(String(data), "utf8"); + var offset = 0; + var consecutiveZeroWrites = 0; + while (offset < buffer.length) { + var written = fs.writeSync(fd, buffer, offset, buffer.length - offset); + if (written === 0) { + consecutiveZeroWrites += 1; + if (consecutiveZeroWrites >= MAX_CONSECUTIVE_ZERO_WRITES) { + throw new Error( + "writeFullySync: fs.writeSync returned 0 " + consecutiveZeroWrites + + " times in a row with no forward progress (delivered " + offset + " of " + + buffer.length + " bytes) — aborting instead of spinning forever." + ); + } + continue; + } + consecutiveZeroWrites = 0; + offset += written; + } +} + function emitAnnotation(message) { // GitHub Actions workflow-command syntax. `%`, CR and LF must be escaped // in the message text per GitHub's documented encoding for `::error::` — @@ -86,7 +182,7 @@ function emitAnnotation(message) { .replace(/%/g, "%25") .replace(/\r/g, "%0D") .replace(/\n/g, "%0A"); - process.stdout.write("::error::" + escaped + "\n"); + writeFullySync(1, "::error::" + escaped + "\n"); } // classifyRun() is the pure decision core of this script: given the raw @@ -203,7 +299,11 @@ function classifyRun(result, files, resultsByFile, totalTests, floor) { return { ok: true, exitCode: 0, kind: "ok", reason: null }; } -module.exports = { classifyRun: classifyRun, emitAnnotation: emitAnnotation }; +module.exports = { + classifyRun: classifyRun, + emitAnnotation: emitAnnotation, + writeFullySync: writeFullySync, +}; // Everything below only runs when this file is executed directly (`node // scripts/check-test-count.js `), not when required as a module by @@ -227,8 +327,6 @@ if (require.main === module) { maxBuffer: 64 * 1024 * 1024, }); - if (result.stdout) process.stdout.write(result.stdout); - // The custom reporter's RESULT lines are the only thing routed to stderr — // forward everything else Node itself wrote to stderr (real errors, // warnings) so a normal `npm test` run still surfaces them, then parse the @@ -255,6 +353,14 @@ if (require.main === module) { var totalTests = passCount + failCount; var verdict = classifyRun(result, files, resultsByFile, totalTests, TEST_COUNT_FLOOR); + // DELIVERY ORDER (lr-e551b9 reopen). The annotation is emitted BEFORE the + // large TAP dump below, not after: emitAnnotation() now uses + // fs.writeSync(1, ...), a direct synchronous syscall that bypasses the + // stdout Writable's async queue entirely, so this ordering is not load- + // bearing for the annotation's own delivery — but it also means the + // annotation is never at risk of landing "behind" a dump that is itself + // subject to the async-pipe race (see below), belt-and-suspenders rather + // than relying on ordering alone. if (!verdict.ok) { var line = "[check-test-count] FAIL (" + verdict.kind + "): " + verdict.reason; process.stderr.write(line + "\n"); @@ -265,5 +371,27 @@ if (require.main === module) { emitAnnotation(line); } - process.exit(verdict.exitCode); + // The raw TAP dump is written with writeFullySync too, for the same reason + // as the annotation above: process.stdout.write() queues asynchronously + // once a pipe's 64KB kernel buffer fills, and a ~1400-test TAP blob is + // multi-MB — far larger than that buffer. Losing the raw output is a + // real, independent problem even though the annotation above no longer + // depends on it: a developer reading a captured `npm test` run should + // still see the actual TAP text, not a truncation artifact. A single + // unlooped fs.writeSync call here would carry the same short-write risk + // as the annotation write did (BOBBIE, PR #402) — writeFullySync loops + // until the whole multi-MB buffer is confirmed delivered. + if (result.stdout) writeFullySync(1, result.stdout); + + // process.exitCode (not process.exit()) lets the event loop drain + // naturally instead of tearing the process down immediately — the second + // half of MILLER's recommended (a)+(b) combination. Nothing below this + // line can reset process.exitCode or call process.exit(0): this is the + // last statement in the script, so there is no later code path that could + // turn a red run green. The FAIL-OPEN CONSTRAINT (lr-795882, engram + // 7613980) requires this be verified, not assumed — every exit-1 + // condition above still sets verdict.exitCode to 1 or the child's own + // non-zero status; process.exitCode is set from that value unconditionally + // and nothing else touches it. + process.exitCode = verdict.exitCode; } diff --git a/test/check-test-count-delivery-lr-e551b9.test.js b/test/check-test-count-delivery-lr-e551b9.test.js new file mode 100644 index 00000000..372b9e63 --- /dev/null +++ b/test/check-test-count-delivery-lr-e551b9.test.js @@ -0,0 +1,165 @@ +"use strict"; +// Regression test for the lr-e551b9 REOPEN (MILLER third-pass diagnosis, +// confidence 0.93, reproduced locally and in CI on PR #400's rebased head +// d7dd88ae). The PR #401 fix made classifyRun() name the right failure +// reason, but its DELIVERY was broken: check-test-count.js wrote the +// ::error:: annotation with process.stdout.write() and then called +// process.exit() before that write could flush. process.stdout.write is +// ASYNCHRONOUS when stdout is a real OS pipe (always true in CI); a +// multi-MB preceding TAP dump exceeds the 64KB kernel pipe buffer, so the +// remainder — including the annotation queued behind it — sits in a +// userspace buffer that process.exit() tears down without draining. The +// annotation reached the Writable stream object and never reached the fd. +// +// test/check-test-count-signal-legibility-lr-e551b9.test.js already covers +// classifyRun()'s verdict/precedence logic and emitAnnotation()'s message +// FORMAT — that coverage is correct for what it tests and is kept unchanged. +// It stubs process.stdout.write with a function that pushes to an array and +// returns true, which is exactly why it could not catch this: a stub that +// always "succeeds" cannot observe flush, backpressure, or the interaction +// between an async queue and process.exit(). It proves the message is +// FORMATTED correctly; it cannot prove the message is DELIVERED. A write is +// not a delivery. This file is the delivery test PEACHES, BOBBIE and NAOMI's +// prior review passes did not have, because none of them exercised a real +// pipe. +// +// METHOD: spawn the actual scripts/check-test-count.js as a CHILD PROCESS +// (not require()'d, not stubbed) with stdio: ["ignore", "pipe", "pipe"] — a +// real OS pipe, matching CI exactly — against a fixture file +// (test/fixtures/check-test-count-padding-lr-e551b9.fixture.js) that emits +// ~2000 trivial passing tests, comfortably exceeding a 64KB TAP dump, plus a +// deliberately nonexistent second file to force the real missing-files FAIL +// path (a genuine wrapper-level condition, not a stub). The parent reads the +// child's stdout the way a CI log collector does — accumulate chunks, +// nothing more — and asserts the captured bytes contain the ::error:: line. +// +// DEMONSTRATED-FAILURE VERIFICATION (lr-4e1242 convention), STRONG FORM: +// this exact test file, run via `npx node --test +// test/check-test-count-delivery-lr-e551b9.test.js`, was executed against +// the pre-fix script at commit cf4b734a (git show cf4b734a:scripts/check- +// test-count.js, restored to a scratch copy since the working tree already +// carries the fix) BEFORE the delivery fix in this diff was written. It +// failed with a genuine wrong-content assertion — not a missing-symbol or +// spawn error: +// +// AssertionError [ERR_ASSERTION]: the ::error:: annotation must be present +// in the child's captured stdout — the wrapper must not lose it to an +// unflushed pipe +// + actual: false +// - expected: true +// +// and the captured stdout in that pre-fix run measured 65536 bytes (exactly +// one 64KB pipe high-water mark) with zero occurrences of "::error::", +// reproducing MILLER's probe finding (mid-word truncation, no annotation) +// exactly. After the fix in this diff (fs.writeSync for both the annotation +// and the TAP dump, plus process.exitCode instead of process.exit()), the +// same test passes: the annotation is present regardless of TAP dump size. +// This is a real absent-annotation failure against production code, not a +// missing-symbol error — the strong form PEACHES requires. +var test = require("node:test"); +var assert = require("node:assert/strict"); +var path = require("path"); +var { spawn } = require("child_process"); + +var WRAPPER_PATH = path.join(__dirname, "..", "scripts", "check-test-count.js"); +var PADDING_FIXTURE = path.join(__dirname, "fixtures", "check-test-count-padding-lr-e551b9.fixture.js"); +var MISSING_FIXTURE = path.join(__dirname, "fixtures", "check-test-count-DOES-NOT-EXIST-lr-e551b9.test.js"); + +// Runs the real wrapper as a child process with a genuine OS pipe on stdout +// and stderr, and resolves with the full captured output plus exit code. +// Deliberately does NOT force any flush/drain on the parent side — a CI log +// collector does not either; if the child loses data to an unflushed queue, +// this harness must observe that loss, not paper over it. +function runWrapperThroughRealPipe(args) { + return new Promise(function (resolve, reject) { + // This test file itself runs under `node --test`, which sets + // NODE_TEST_CONTEXT in its own process.env and — because child_process + // inherits the parent's env by default — that value leaks into the + // spawned child below. The wrapper we're spawning ALSO runs `node + // --test` internally, and Node's test runner treats an inherited + // NODE_TEST_CONTEXT as "I am a nested test-runner child", which trips + // its own recursion guard and makes it skip actually running the + // fixture file (silently reported here as "missing-files", NOT the + // production defect under test). Stripping it is required for this + // harness to observe the real wrapper's behavior instead of Node's own + // nested-test-runner guard. + var childEnv = Object.assign({}, process.env); + delete childEnv.NODE_TEST_CONTEXT; + + var child = spawn(process.execPath, [WRAPPER_PATH].concat(args), { + stdio: ["ignore", "pipe", "pipe"], + env: childEnv, + }); + + var stdoutChunks = []; + var stderrChunks = []; + + child.stdout.on("data", function (chunk) { + stdoutChunks.push(chunk); + }); + child.stderr.on("data", function (chunk) { + stderrChunks.push(chunk); + }); + child.on("error", reject); + child.on("close", function (code) { + resolve({ + exitCode: code, + stdout: Buffer.concat(stdoutChunks).toString("utf8"), + stderr: Buffer.concat(stderrChunks).toString("utf8"), + }); + }); + }); +} + +test( + "lr-e551b9 delivery: a fail path (missing-files) behind a large preceding TAP dump still delivers the ::error:: annotation through a real pipe", + { timeout: 30000 }, + async function () { + var result = await runWrapperThroughRealPipe([PADDING_FIXTURE, MISSING_FIXTURE]); + + // Sanity: this must genuinely be the missing-files fail path, not some + // other failure mode — assert the exit code and the reason line, not + // just the annotation, so a future regression that changes WHICH path + // fires is also caught here. + assert.equal(result.exitCode, 1, "the missing-files condition must still fail the run (fail-open constraint, lr-795882)"); + assert.ok( + result.stderr.indexOf("[check-test-count] FAIL (missing-files)") !== -1, + "stderr must still carry the plain-text FAIL line; got: " + result.stderr.slice(-500) + ); + + // The load-bearing assertion: the annotation must be present in the + // CHILD'S ACTUAL CAPTURED stdout, past a TAP dump large enough to + // exceed a 64KB pipe buffer. This is exactly the assertion that fails + // against the pre-fix script (see the file header's demonstrated- + // failure verification) and passes against the fix in this diff. + assert.ok( + result.stdout.indexOf("::error::") !== -1, + "the ::error:: annotation must be present in the child's captured stdout — the wrapper must not lose it to an unflushed pipe" + ); + assert.ok( + result.stdout.indexOf("missing-files") !== -1, + "the delivered annotation must name the actual verdict kind, not just any ::error:: text" + ); + + // The raw TAP dump must also survive intact — MILLER/HOLDEN both noted + // that losing the annotation's ride-along blob (the raw test output) is + // an independent real problem, not just the annotation. A truncated + // capture ends abruptly with no TAP summary footer; assert the footer + // is present as a proxy for "the dump was not cut off mid-stream". + assert.ok( + result.stdout.indexOf("# duration_ms") !== -1, + "the TAP summary footer must survive intact — a truncated dump would be missing it, same as MILLER's probe finding" + ); + } +); + +test( + "lr-e551b9 delivery: a clean run (no fail path) behind the same large TAP dump emits no annotation and exits 0", + { timeout: 30000 }, + async function () { + var result = await runWrapperThroughRealPipe([PADDING_FIXTURE]); + + assert.equal(result.exitCode, 0, "a genuinely clean run must still pass — the delivery fix must not introduce a new false FAIL"); + assert.ok(result.stdout.indexOf("::error::") === -1, "no annotation should be emitted when nothing failed"); + } +); diff --git a/test/check-test-count-short-write-lr-e551b9.test.js b/test/check-test-count-short-write-lr-e551b9.test.js new file mode 100644 index 00000000..3980c2fe --- /dev/null +++ b/test/check-test-count-short-write-lr-e551b9.test.js @@ -0,0 +1,116 @@ +"use strict"; +// Regression test for the lr-e551b9 fold-in (BOBBIE, PR #402 comment +// 5360992190). Both writeSync call sites in scripts/check-test-count.js +// (the ::error:: annotation and the raw TAP dump) used a SINGLE, UNLOOPED +// fs.writeSync call with the return value discarded. fs.writeSync performs +// exactly one write(2) syscall attempt and does not loop to guarantee full +// delivery; a pipe fd can legitimately short-write under backpressure once +// the payload is large enough — the exact condition the multi-MB TAP dump +// this script also writes is built to hit. The existing delivery test +// (test/check-test-count-delivery-lr-e551b9.test.js) gives EMPIRICAL +// coverage — it happens to pass at that fixture's size in this CI shape — +// but nothing in it guarantees the loop as the suite grows or under a +// slower-reader backpressure shape that hits a mid-write short-write instead +// of a single flush boundary. This file unit-tests the write HELPER itself +// (writeFullySync, exported alongside classifyRun/emitAnnotation) with an +// injected write function that deliberately returns partial byte counts, so +// the short-write path is covered structurally, not just empirically. +// +// DEMONSTRATED-FAILURE VERIFICATION (lr-4e1242 convention): this exact +// short-write scenario (a 300KB buffer against a stub that always returns a +// 64KB-capped partial count, mirroring an OS pipe's kernel buffer) was run +// against a PRE-FIX single-unlooped-call shape (`if (buffer.length) +// writeSyncFn(fd, buffer, 0, buffer.length);`, discarding the return value — +// exactly what scripts/check-test-count.js's two writeSync call sites did +// before this diff) in an isolated scratch harness before this file was +// written. It failed with a genuine wrong-content assertion: +// +// AssertionError [ERR_ASSERTION]: delivered length must equal input length +// 65536 !== 300000 +// +// only 65536 of 300000 bytes were ever handed to the stub — the rest was +// silently discarded, reproducing the exact truncation class this task +// exists to eliminate. Against the post-fix looped shape (identical to +// writeFullySync below), the same scenario delivered all 300000 bytes +// exactly. This test below exercises the REAL exported writeFullySync +// function directly (not a reimplementation), so it fails against the +// current working tree if the loop is ever removed or short-circuited. +var test = require("node:test"); +var assert = require("node:assert/strict"); +var checkTestCount = require("../scripts/check-test-count.js"); + +test( + "lr-e551b9 short-write: writeFullySync keeps calling fs.writeSync until a short-write-prone destination has received every byte", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var delivered = []; + var callCount = 0; + var CHUNK_CAP = 65536; // mirrors a 64KB OS pipe kernel buffer short-write + + fs.writeSync = function (fd, buffer, offset, length) { + callCount += 1; + var n = Math.min(CHUNK_CAP, length); + delivered.push(Buffer.from(buffer.slice(offset, offset + n))); + return n; + }; + + var input = Buffer.alloc(300000, 65); // 300KB, comfortably > one chunk cap + + try { + checkTestCount.writeFullySync(1, input); + } finally { + fs.writeSync = originalWriteSync; + } + + var result = Buffer.concat(delivered); + + assert.ok( + callCount > 1, + "the stub must have been invoked more than once for this to be a real short-write test; got " + callCount + " call(s)" + ); + assert.equal( + result.length, input.length, + "writeFullySync must keep looping until the full buffer is delivered, not stop after the first short write" + ); + assert.ok( + result.equals(input), + "the concatenated delivered bytes must equal the input buffer exactly, byte for byte" + ); + } +); + +test( + "lr-e551b9 short-write: writeFullySync converts a string input to a Buffer once up front, so byte offsets from a short write never misalign against UTF-16 code-unit indices", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var delivered = []; + // Force a short write partway through a multi-byte UTF-8 sequence: "e" + // followed by a 3-byte euro sign, encoded as UTF-8 that is 4 bytes total + // (1 + 3). A cap of 2 forces the split to land INSIDE the euro sign's + // byte sequence on the first call, which only a byte-indexed Buffer + // offset (not a JS string character slice) can resume correctly. + var CHUNK_CAP = 2; + + fs.writeSync = function (fd, buffer, offset, length) { + var n = Math.min(CHUNK_CAP, length); + delivered.push(Buffer.from(buffer.slice(offset, offset + n))); + return n; + }; + + var input = "e€"; // "e" + EURO SIGN, 4 bytes UTF-8, 2 UTF-16 code units + + try { + checkTestCount.writeFullySync(1, input); + } finally { + fs.writeSync = originalWriteSync; + } + + var result = Buffer.concat(delivered); + assert.equal( + result.toString("utf8"), input, + "the reassembled bytes must decode back to the exact original string, even when a short write splits a multi-byte character" + ); + } +); diff --git a/test/check-test-count-signal-legibility-lr-e551b9.test.js b/test/check-test-count-signal-legibility-lr-e551b9.test.js index 4a0bb436..fae0cc18 100644 --- a/test/check-test-count-signal-legibility-lr-e551b9.test.js +++ b/test/check-test-count-signal-legibility-lr-e551b9.test.js @@ -222,17 +222,35 @@ test("lr-e551b9: check ORDER — missing-files still wins over signal-death even assert.ok(verdict.reason.indexOf("test/killed-mid-run.test.js") !== -1); }); -test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to stdout", function () { +// NOTE (lr-e551b9 reopen): emitAnnotation() is now implemented with +// fs.writeSync(1, ...) rather than process.stdout.write(...) — see +// check-test-count.js's DELIVERY comment above emitAnnotation for why: a +// process.stdout.write-based stub, exactly like the two tests below used to +// use, cannot observe the async-pipe/process.exit() race that lost the +// annotation in production (that is precisely the gap MILLER's reopen +// diagnosis found — see test/check-test-count-delivery-lr-e551b9.test.js for +// the through-a-real-pipe regression test that covers actual delivery). +// These two tests still verify message FORMAT — the ::error:: prefix and the +// GitHub workflow-command escaping — which is a real, narrower thing worth +// unit-testing fast and in isolation; they now spy on fs.writeSync (the +// primitive the implementation actually calls) instead of stubbing +// process.stdout.write (a primitive it no longer calls), so the spy remains +// truthful about what emitAnnotation does today. +test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to fd 1", function () { + var fs = require("fs"); var chunks = []; - var originalWrite = process.stdout.write; - process.stdout.write = function (chunk) { - chunks.push(chunk); - return true; + var originalWriteSync = fs.writeSync; + fs.writeSync = function (fd, chunk) { + if (fd === 1) { + chunks.push(chunk); + return chunk.length; + } + return originalWriteSync.apply(fs, arguments); }; try { checkTestCount.emitAnnotation("[check-test-count] FAIL (signal-death): node --test was killed by SIGKILL"); } finally { - process.stdout.write = originalWrite; + fs.writeSync = originalWriteSync; } var written = chunks.join(""); @@ -241,16 +259,20 @@ test("lr-e551b9: emitAnnotation writes a ::error:: prefixed workflow command to }); test("lr-e551b9: emitAnnotation escapes %, CR and LF per GitHub's documented workflow-command encoding", function () { + var fs = require("fs"); var chunks = []; - var originalWrite = process.stdout.write; - process.stdout.write = function (chunk) { - chunks.push(chunk); - return true; + var originalWriteSync = fs.writeSync; + fs.writeSync = function (fd, chunk) { + if (fd === 1) { + chunks.push(chunk); + return chunk.length; + } + return originalWriteSync.apply(fs, arguments); }; try { checkTestCount.emitAnnotation("100% failure\r\nline two"); } finally { - process.stdout.write = originalWrite; + fs.writeSync = originalWriteSync; } var written = chunks.join(""); diff --git a/test/check-test-count-zero-write-lr-e551b9.test.js b/test/check-test-count-zero-write-lr-e551b9.test.js new file mode 100644 index 00000000..2647ef34 --- /dev/null +++ b/test/check-test-count-zero-write-lr-e551b9.test.js @@ -0,0 +1,142 @@ +"use strict"; +// Regression test for the lr-e551b9 fold-in (PEACHES PR #402 BLOCKING). +// writeFullySync (test/check-test-count-short-write-lr-e551b9.test.js) +// advances `offset` ONLY by fs.writeSync's return value: +// offset += fs.writeSync(fd, buffer, offset, buffer.length - offset); +// If fs.writeSync ever returns 0, offset does not advance and +// `while (offset < buffer.length)` spins FOREVER. On this specific script — +// the merge-gate wrapper whose entire purpose is turning a FAIL verdict into +// a legible signal — an infinite loop here is strictly worse than the +// truncation bug it replaced: the job burns its runner until the workflow +// timeout kills it, producing NO verdict and NO annotation at all, versus a +// truncated log that still fails fast with a usable exit code. +// +// This file covers the 0-byte-return path directly: assert the guard +// terminates (does not hang) and surfaces the failure as a throw rather than +// looping, via a BOUNDED call-count assertion rather than a real wall-clock +// timeout — a regression to the unguarded loop calls the stub an unbounded +// number of times with a 0 return and never returns control to the test +// runner at all, so a real-time `{ timeout }` on the test itself would only +// convert a spin into a hung *suite* (node --test's own timeout mechanism +// cannot interrupt a synchronous, non-yielding while loop — there is no +// event-loop turn for it to fire on). The stub instead throws once its own +// call count exceeds a bound comfortably above the guard's configured +// threshold, so an unguarded loop fails with a bounded, assertable stub +// error instead of hanging the process, and CI still reports a fast FAIL +// rather than stalling on a spinning regression (the exact "a test that +// hangs on regression is nearly as bad as the bug" trap named in this +// task's dispatch). +var test = require("node:test"); +var assert = require("node:assert/strict"); +var checkTestCount = require("../scripts/check-test-count.js"); + +// Comfortably above writeFullySync's own MAX_CONSECUTIVE_ZERO_WRITES bound +// (100) so the stub never trips before the guard has a chance to act, but +// still small enough that an unguarded regression fails near-instantly +// instead of iterating meaningfully long. +var STUB_CALL_BOUND = 500; + +test( + "lr-e551b9 zero-write: writeFullySync throws instead of spinning forever when fs.writeSync returns 0", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var callCount = 0; + + fs.writeSync = function () { + callCount += 1; + if (callCount > STUB_CALL_BOUND) { + // Safety valve for a REGRESSION to the unguarded loop: without this, + // an unguarded writeFullySync would call this stub forever and the + // test process would never yield back to node --test's own result + // reporting — the exact "hangs on regression" failure mode this test + // exists to avoid inflicting on CI. Throwing here still fails the + // test (the throw propagates out of writeFullySync, uncaught by the + // test body below, and node --test reports it as a failure) — it + // just fails FAST instead of hanging. + throw new Error( + "writeFullySync called the 0-byte-returning stub " + callCount + + " times without throwing its own bounded-retry error — the " + + "non-advancing-write guard is missing or broken (regression to " + + "the unguarded spin this test exists to catch)." + ); + } + return 0; + }; + + var input = Buffer.from("hello world", "utf8"); + + try { + assert.throws( + function () { + checkTestCount.writeFullySync(1, input); + }, + function (err) { + return err instanceof Error && !/non-advancing-write guard/.test(err.message); + }, + "writeFullySync must throw its OWN bounded-retry error, not exhaust the test's safety-valve bound first" + ); + } finally { + fs.writeSync = originalWriteSync; + } + + assert.ok( + callCount <= STUB_CALL_BOUND, + "writeFullySync must give up well before " + STUB_CALL_BOUND + " zero-byte returns; got " + callCount + " calls" + ); + } +); + +test( + "lr-e551b9 zero-write: a mixed sequence (partial, partial, then a run of zeros) still terminates via the guard, not just a first-call zero", + function () { + var fs = require("fs"); + var originalWriteSync = fs.writeSync; + var callCount = 0; + var progressCalls = 0; + + fs.writeSync = function (fd, buffer, offset, length) { + callCount += 1; + if (callCount > STUB_CALL_BOUND) { + throw new Error( + "writeFullySync called the stub " + callCount + " times without " + + "throwing its own bounded-retry error after real progress was " + + "already made — the guard must apply AFTER progress, not only " + + "on an immediate first-call zero." + ); + } + // First two calls make real, partial progress (proving the guard does + // not just special-case "the very first call returned 0" — it must + // keep counting consecutive zeros correctly even after offset has + // already advanced past 0). + if (progressCalls < 2) { + progressCalls += 1; + var n = Math.min(2, length); + return n; + } + // From here on, every call returns 0 — the non-advancing run the + // guard must catch. + return 0; + }; + + var input = Buffer.from("hello world", "utf8"); // 11 bytes; 2+2 real progress, then stuck + + try { + assert.throws( + function () { + checkTestCount.writeFullySync(1, input); + }, + /writeFullySync: fs\.writeSync returned 0/, + "the guard must fire (and its own error message must be observable) after real progress was already made, not only on an immediate first-call zero" + ); + } finally { + fs.writeSync = originalWriteSync; + } + + assert.ok( + callCount <= STUB_CALL_BOUND, + "the mixed sequence must still terminate well before " + STUB_CALL_BOUND + " calls; got " + callCount + ); + assert.equal(progressCalls, 2, "the stub must have been allowed to make its two real partial-progress calls before the zero run began"); + } +); diff --git a/test/fixtures/check-test-count-padding-lr-e551b9.fixture.js b/test/fixtures/check-test-count-padding-lr-e551b9.fixture.js new file mode 100644 index 00000000..bd6f627e --- /dev/null +++ b/test/fixtures/check-test-count-padding-lr-e551b9.fixture.js @@ -0,0 +1,23 @@ +"use strict"; +// Fixture for test/check-test-count-delivery-lr-e551b9.test.js — NOT itself +// collected by `npm test` (package.json only globs test/*.test.js; this file +// lives under test/fixtures/ and does not match that pattern, and does not +// end in .test.js). +// +// Emits enough trivial passing node:test cases that the resulting TAP output +// comfortably exceeds a 64KB OS pipe buffer, which is what the delivery test +// needs to force process.stdout.write's async-queue behavior when stdout is +// a real pipe (see that test file for why this matters — lr-e551b9 reopen). +var test = require("node:test"); +var assert = require("node:assert/strict"); + +for (var i = 0; i < 2000; i++) { + test( + "lr-e551b9 padding test number " + i + " — exists only to bulk up TAP " + + "output past a 64KB pipe buffer so the delivery test can force the " + + "async-write race this fixture is built to exercise", + function () { + assert.equal(1, 1); + } + ); +}