golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 4 Packages Projects Releases Wiki Activity

fix(#4172): add --merge-async to test:coverage:scripts-floor (#4173)

Browse Source 
* fix(#4172): add --merge-async to test:coverage:scripts-floor

The "Coverage gate (merged shards)" test.yml job has OOM-crashed (exit
134, SIGABRT) on every push to next since 4dfc46b. test:coverage:scripts-floor
was the only c8 coverage-merge invocation in package.json still missing
--merge-async: c8's default sync merge path (Report._getMergedProcessCov)
loads every raw V8 coverage file from the merged 3-shard coverage/tmp
directory into memory as one array before merging, instead of folding
them in one at a time, and now blows through the job's 8192 MB heap
ceiling.

This is the same bug class as #4068 (fixed in 4d70b4dc4), which added
--merge-async to test:coverage:unit and test:coverage:report -- the two
other scripts sharing this merge/report code path -- but missed this
third sibling, which reads the exact same merged coverage/tmp data in
the same job. check-coverage and report both dispatch through c8's
shared getCoverageMapFromAllCoverageFiles()/mergeAsync branch, so the
fix is identical in shape to #4068's.

Extends the existing #4068 regression guard (tests/c8-merge-async-flag.test.cjs)
to also assert test:coverage:scripts-floor carries --merge-async, closing
the coverage gap #4068 left on this sibling script.

Fixes #4172

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(#4172): backfill changeset PR number (pr:0 -> 4173)

* fix(#4172): route scripts-floor through `c8 report`, not `check-coverage`

The prior commit (1a3d6b75e) added --merge-async to
test:coverage:scripts-floor, but the Coverage gate job still OOM-crashed
identically on PR #4173's CI run
(https://github.com/open-gsd/gsd-core/actions/runs/33587951779/job/100118358975).

Root cause of that miss: c8@11.0.0's `check-coverage` CLI subcommand
handler (node_modules/c8/lib/commands/check-coverage.js) never forwards
argv.mergeAsync into the Report constructor -- only the `report`
subcommand's handler (node_modules/c8/lib/commands/report.js) and the
default command (which also calls into report.js) do. Verified directly
by constructing Report the same way each handler does: the check-coverage
path yields report.mergeAsync === undefined even with --merge-async on
the command line, while the report path yields true.

Fix: route test:coverage:scripts-floor through `c8 report --check-coverage`
instead of `c8 check-coverage`. report.js's outputReport() calls the same
checkCoverages() threshold-checking helper when --check-coverage is
truthy, so behavior (and exit code on threshold failure) is unchanged --
only the code path taken to get there now actually honors --merge-async.

Extends the tests/c8-merge-async-flag.test.cjs regression guard with an
explicit assertion that scripts-floor invokes `c8 report`, not
`check-coverage`, so this can't silently regress back to the broken form.

Fixes #4172

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* docs(#4172): correct changeset to describe the actual root cause (check-coverage vs report subcommand gap, not just the missing flag)

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-09-02 06:39:15 -04:00
committed by GitHub
parent 5c7243e54b
commit fa107c0461
3 changed files with 79 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/sturdy-orcas-snooze.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4173
---
**Fixed the coverage gate OOM-crashing on every push to `next`.** The scripts/ coverage-floor check invoked c8's `check-coverage` subcommand, whose handler silently drops the async-merge flag even when it's passed (unlike its `report` sibling, which honors it) — the same OOM class as #4068, but this third script slipped through that fix because adding the flag alone wasn't enough here. Routing the check through `c8 report --check-coverage` instead makes the async-merge flag actually take effect, so coverage now merges incrementally instead of loading every shard's raw data into memory at once and blowing the 8GB CI heap ceiling. (#4172)

2
package.json
View File

@@ -148,7 +148,7 @@
"test:qa": "node scripts/run-tests.cjs --suite qa",
"test:affected": "node scripts/run-affected-tests.cjs",
"test:coverage": "c8 --check-coverage --lines 70 --branches 60 --reporter text --include 'gsd-core/bin/lib/**/*.cjs' --exclude 'tests/**' --all node scripts/run-tests.cjs",
"test:coverage:scripts-floor": "c8 check-coverage --lines 55 --include 'scripts/**/*.cjs' --exclude 'tests/**' --all",
"test:coverage:scripts-floor": "c8 report --check-coverage --lines 55 --merge-async --include 'scripts/**/*.cjs' --exclude 'tests/**' --all",
"test:coverage:unit": "c8 --reporter text --reporter json-summary --merge-async --include 'gsd-core/bin/lib/**/*.cjs' --exclude 'tests/**' --all node scripts/run-tests.cjs --suite unit && node scripts/check-coverage-gate.cjs",
"test:coverage:unit:raw": "c8 --reporter none node scripts/run-tests.cjs --suite unit",
"test:coverage:report": "c8 report --reporter text --reporter json-summary --merge-async --include 'gsd-core/bin/lib/**/*.cjs' --exclude 'tests/**' --all && node scripts/check-coverage-gate.cjs",

91
tests/c8-merge-async-flag.test.cjs
View File

@@ -1,24 +1,50 @@
'use strict';
// Regression guard for #4068: c8's default (sync) coverage-merge phase loads every
// raw V8 coverage file for the whole run into memory as one array before merging
// (`Report._getMergedProcessCov`), which OOM-crashed `npm run test:coverage:unit` in
// the `release.yml` `finalize` dry-run of 1.12.0 (exit 134, SIGABRT) once the unit
// suite grew to 1785 tests / 15 chunks -- a recurrence of #199, which hit the same
// class at ~466 tests and was "fixed" by raising the heap ceiling instead of the
// merge shape. `--merge-async` switches to `_getMergedProcessCovAsync`, which reads
// and merges one raw file at a time (documented root-cause verification, including a
// real memory-shape repro against this repo's actual pinned c8@11.0.0, lives in
// Regression guard for #4068 and #4172: c8's default (sync) coverage-merge phase
// loads every raw V8 coverage file for the whole run into memory as one array before
// merging (`Report._getMergedProcessCov`), which OOM-crashed `npm run
// test:coverage:unit` in the `release.yml` `finalize` dry-run of 1.12.0 (exit 134,
// SIGABRT) once the unit suite grew to 1785 tests / 15 chunks -- a recurrence of
// #199, which hit the same class at ~466 tests and was "fixed" by raising the heap
// ceiling instead of the merge shape. `--merge-async` switches to
// `_getMergedProcessCovAsync`, which reads and merges one raw file at a time
// (documented root-cause verification, including a real memory-shape repro against
// this repo's actual pinned c8@11.0.0, lives in
// .gsd/bug/fix-4068-coverage-merge-oom/10-diagnosis.md).
//
// #4068's fix covered test:coverage:unit and test:coverage:report but missed the
// third script reading the same merged coverage/tmp data in the same
// "Coverage gate (merged shards)" test.yml job: test:coverage:scripts-floor. That
// gap OOM-crashed the coverage gate on every push to next starting at 4dfc46b, once
// #4068's own new test file plus other suite growth pushed the un-flagged sync merge
// over the 8192 MB heap ceiling (#4172).
//
// IMPORTANT c8@11.0.0 gotcha (verified against node_modules/c8/lib/commands/
// check-coverage.js and report.js directly): the `check-coverage` CLI subcommand's
// handler does NOT forward `argv.mergeAsync` into the `Report(...)` constructor --
// only the `report` subcommand's handler (and the default command, which also calls
// into report.js's outputReport) does. So `c8 check-coverage --merge-async ...`
// silently ignores the flag and still OOMs -- confirmed live on PR #4173's CI run
// after simply adding the flag to the check-coverage invocation did not fix the
// crash. The actual fix routes test:coverage:scripts-floor through the `report`
// subcommand with `--check-coverage` (a boolean flag, not a subcommand) instead of
// the `check-coverage` subcommand directly: `c8 report --check-coverage --lines 55
// --merge-async --include ...`. This exercises the exact same threshold-checking
// logic (report.js's outputReport calls the same checkCoverages() helper from
// check-coverage.js when --check-coverage is truthy) but through the handler that
// actually wires mergeAsync.
//
// This test cannot behaviorally reproduce the OOM itself: `gsd-test` never invokes
// `npm run test:coverage:unit` (it runs `node --test` directly), and a heap-ceiling
// crossover point is not a stable, portable assertion across this repo's OS x Node
// matrix on shared benches (see .gsd/bug/fix-4068-coverage-merge-oom/50-test-matrix.md
// for the full seam analysis). What IS stable and worth guarding: the flag must not
// silently disappear from the two scripts that need it, and must not be added to a
// script whose merge/report phase never runs (a no-op that would misleadingly imply
// coverage here too).
// these npm scripts (it runs `node --test` directly), and a heap-ceiling crossover
// point is not a stable, portable assertion across this repo's OS x Node matrix on
// shared benches (see .gsd/bug/fix-4068-coverage-merge-oom/50-test-matrix.md for the
// full seam analysis). What IS stable and worth guarding: the flag must not silently
// disappear from the scripts that need it, must not be added to a script whose
// merge/report phase never runs (a no-op that would misleadingly imply coverage
// here too), and -- new for #4172 -- test:coverage:scripts-floor must keep routing
// through the `report` subcommand rather than reverting to the `check-coverage`
// subcommand, which would silently re-introduce the OOM despite --merge-async still
// being present in the script string.
const { describe, test } = require('node:test');
const assert = require('node:assert/strict');
@@ -27,7 +53,7 @@ const path = require('node:path');
const pkg = require(path.join(__dirname, '..', 'package.json'));
const scripts = pkg.scripts;
describe('coverage-merge-async-flag (#4068)', () => {
describe('coverage-merge-async-flag (#4068, #4172)', () => {
test('test:coverage:unit carries --merge-async', () => {
assert.match(
scripts['test:coverage:unit'],
@@ -46,8 +72,37 @@ describe('coverage-merge-async-flag (#4068)', () => {
);
});
test('test:coverage:scripts-floor carries --merge-async', () => {
assert.match(
scripts['test:coverage:scripts-floor'],
/(?:^|\s)c8\s.*--merge-async/,
'test:coverage:scripts-floor (the coverage-gate job scripts/ floor check in ' +
'test.yml) must pass --merge-async to c8, or it reverts to loading every ' +
'raw V8 coverage file into memory at once, the same class as #4068 (#4172)'
);
});
test('test:coverage:scripts-floor routes through the report subcommand, not check-coverage', () => {
const script = scripts['test:coverage:scripts-floor'];
assert.match(
script,
/(?:^|\s)c8\s+report\b/,
'test:coverage:scripts-floor must invoke `c8 report --check-coverage ...`, not ' +
'`c8 check-coverage ...` -- c8@11.0.0\'s check-coverage subcommand handler ' +
'does not forward --merge-async into the Report constructor (verified against ' +
'node_modules/c8/lib/commands/check-coverage.js), so that form silently drops ' +
'the flag and still OOMs even with --merge-async present in the string (#4172)'
);
assert.match(
script,
/--check-coverage\b/,
'test:coverage:scripts-floor must pass --check-coverage to `c8 report` so the ' +
'coverage threshold is still enforced, not just reported'
);
});
test('--merge-async lands in the c8 invocation, not after the node runner', () => {
for (const key of ['test:coverage:unit', 'test:coverage:report']) {
for (const key of ['test:coverage:unit', 'test:coverage:report', 'test:coverage:scripts-floor']) {
const script = scripts[key];
const flagIndex = script.indexOf('--merge-async');
const nodeIndex = script.indexOf(' node ');