Skip to content

Commit adaa4e4

Browse files
bsparkmaclaude
andauthored
Split the backend suite so no one process reads the whole test stream (#131)
The `Unable to deserialize cloned data due to invalid or unsupported version` failures are a bug in Node's own test runner, not in this repo. `node --test` streams results from a child process per file back to a parent that deserializes them, and the parent decodes each message's size from four bytes with a signed shift — so a size whose top byte has the high bit set reads as negative and the next slice comes from the wrong place. It surfaces as an uncaughtException against an arbitrary file with no assertion in it, and the reported test count DROPS, which is how it can be told apart from a real failure. Upstream fixed it in nodejs/node#64706 by making that decode unsigned. It is released in v24.20.0 and v26.7.0 and in NO release of Node 22 — 22.23.2 is the newest and does not carry it, and the PR has no v22 backport label. The runtime image is node:22-alpine, so CI stays on 22 deliberately: testing on a Node the container does not ship would be the worse trade. Pinning cannot fix this. So the suite runs as four `node --test` invocations instead of one. Each is its own parent reading its own stream, so no single process decodes the whole thing. That reduces exposure roughly in proportion; it does not remove the mechanism, and neither this commit message nor the script claims otherwise. Deliberately no retry. A retry that turns a genuine red into a green is worse than the flake it hides, and one loud enough to be safe is a bigger change than this — worth building only if sharding proves insufficient. Node's own `--test-shard` does the discovery and the partition, because a chunker that globbed for test files itself would have to reimplement Node's rules and the failure mode of getting them slightly wrong is a directory that silently stops being tested. The guards cover the one way this script could still drop tests: a shard that ran nothing fails the job, as does any non-zero exit or any failing test. The totals are printed and written to the job summary so a drop is visible rather than inferred. Also fixes a stale comment: it described Node 22.15, which is what the version floated to when it was written. CI runs 22.23.2 today. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent c621730 commit adaa4e4

4 files changed

Lines changed: 203 additions & 8 deletions

File tree

.github/workflows/build-test.yml

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ jobs:
4848
--health-interval 5s --health-timeout 5s --health-retries 20
4949
steps:
5050
- uses: actions/checkout@v4
51+
# FLOATING ON PURPOSE, and worth knowing it floats: this resolved to
52+
# 22.15 when the job was written and runs 22.23.2 today. Pinning a patch
53+
# version would freeze the security updates that come with the line and
54+
# would not help the test-runner bug below — every 22.x has it.
5155
- uses: actions/setup-node@v4
5256
with:
5357
node-version: '22'
@@ -71,14 +75,31 @@ jobs:
7175
- name: backend syntax check
7276
working-directory: backend
7377
run: node --check server.js
74-
# Serial on purpose. Node 22.15's parallel test runner intermittently dies with
75-
# "Unable to deserialize cloned data due to invalid or unsupported version"
76-
# (ERR_TEST_FAILURE) and blames an arbitrary file — it is a function of the test
77-
# FILE COUNT, not of any one test. --test-concurrency=1 removes the flake at the
78-
# cost of a longer run. Do not drop this to "speed CI up".
79-
- name: backend unit tests
78+
# SPLIT INTO SHARDS, AND STILL SERIAL WITHIN EACH.
79+
#
80+
# `node --test` streams results from a child process per file back to a
81+
# parent that deserializes them, and EVERY Node 22 has a bug in that
82+
# parent-side reader: the per-message size is decoded with a signed shift,
83+
# so a size whose top byte has the high bit set reads as negative. It
84+
# surfaces as `Unable to deserialize cloned data due to invalid or
85+
# unsupported version` against an arbitrary file, with no assertion in it
86+
# and a test count that DROPS.
87+
#
88+
# Upstream fixed it in nodejs/node#64706 — shipped in v24.20.0 and
89+
# v26.7.0, in NO release of Node 22 (22.23.2 is the newest and does not
90+
# carry it; the PR has no v22 backport label). The runtime image is
91+
# node:22-alpine, so this job stays on 22 deliberately: testing on a Node
92+
# the container does not ship would be the worse trade.
93+
#
94+
# Each shard is its own parent reading its own stream, so no single
95+
# process decodes the whole suite. That REDUCES exposure; it does not fix
96+
# the bug. There is deliberately no retry — see backend/scripts/shard-runner.mjs.
97+
#
98+
# `--test-concurrency=1` stays INSIDE each shard. Both together were the
99+
# earlier mitigation and neither has been shown to be the load-bearing one.
100+
- name: backend unit tests (sharded)
80101
working-directory: backend
81-
run: node --test --test-concurrency=1
102+
run: node scripts/shard-runner.mjs
82103

83104
# --- spine smoke test (12/12) against an ephemeral Postgres ---
84105
- name: prepare ephemeral DB (carein_app role + tenant DB)

CLAUDE.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,7 @@ parallel work.
746746
cd backend && npm ci
747747
node --check server.js # syntax gate
748748
node --test # unit tests, invoked directly (there is no npm test script)
749+
node scripts/shard-runner.mjs # the SAME suite, split across 4 `node --test` runs — CI runs this
749750

750751
# dashboard — pnpm, not npm
751752
cd new-dashboard && pnpm install --frozen-lockfile
@@ -755,6 +756,17 @@ pnpm run test # vitest run
755756

756757
There is **no lint script and no eslint dependency** anywhere in this repo.
757758

759+
**Why CI shards the backend suite.** Every Node 22 carries a parent-side bug in
760+
`node --test`'s IPC reader: a per-message size decoded with a signed shift, which
761+
surfaces as `Unable to deserialize cloned data due to invalid or unsupported
762+
version` against an arbitrary file, with no assertion in it and a test count that
763+
DROPS — that dropped count is how you tell it from a real failure. Upstream fixed
764+
it in `nodejs/node#64706`, released in **v24.20.0 / v26.7.0 and in no Node 22**;
765+
the runtime image is `node:22-alpine`, so CI stays on 22 on purpose. Sharding
766+
means no single parent decodes the whole stream — a smaller target, **not a
767+
fix**. `backend/scripts/shard-runner.mjs` has the whole story. `node --test`
768+
locally is still fine and still the fastest way to run one file.
769+
758770
`--frozen-lockfile` matters. `new-dashboard/tests/tc-contract-bundle.test.ts` re-runs
759771
esbuild over `backend/tc/contract.entry.ts` and **byte-compares** the result against the
760772
committed `backend/tc/contract.gen.cjs`. Because that bundle inlines all of zod, a

DEV_PROD_WORKFLOW.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ git commit --allow-empty -m "ci: retrigger" && git push
384384

385385
If a merge genuinely cannot wait, run the gate locally first — `pnpm run check` and
386386
`pnpm run test` in `new-dashboard/`, then `node --check server.js` and
387-
`node --test --test-concurrency=1` in `backend/` — so the merge is at least not blind, and
387+
`node scripts/shard-runner.mjs` in `backend/` (what CI runs) — so the merge is at least not blind, and
388388
say plainly in the PR that it merged without CI, so the next promotion runs `build-test`
389389
in staging-cd first.
390390

backend/scripts/shard-runner.mjs

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
#!/usr/bin/env node
2+
/**
3+
* Run the backend suite as SEVERAL `node --test` invocations instead of one.
4+
*
5+
* ═════════════════════════════════════════════════════════════════════════════
6+
* WHY THIS EXISTS
7+
* ═════════════════════════════════════════════════════════════════════════════
8+
* `node --test` runs each test file in a child process and streams the results
9+
* back over an IPC channel that the PARENT deserializes. On every Node 22 there
10+
* is a bug in that parent-side reader: the per-message size is decoded from four
11+
* bytes with `<<`, which is a SIGNED operation in JavaScript, so a size whose
12+
* top byte has the high bit set comes out negative and the next slice is taken
13+
* from the wrong place. What surfaces is:
14+
*
15+
* not ok N - <an arbitrary file>
16+
* failureType: 'uncaughtException'
17+
* error: 'Unable to deserialize cloned data due to invalid or unsupported version.'
18+
* stack: #processRawBuffer (node:internal/test_runner/runner.js)
19+
*
20+
* — a failure with no assertion in it, blaming a file that did nothing wrong,
21+
* and taking the rest of that file's tests down with it (the reported test count
22+
* DROPS, which is how you tell it from a real red).
23+
*
24+
* Upstream fixed it in `nodejs/node#64706` by making that decode unsigned. The
25+
* fix is in **v24.20.0 and v26.7.0**. It is in **no** release of Node 22 —
26+
* 22.23.2 is the newest and does not carry it, and the PR has no v22 backport
27+
* label. This repo's runtime image is `node:22-alpine`, so CI runs 22 on
28+
* purpose: testing on a Node the container does not ship would be a worse trade
29+
* than the flake.
30+
*
31+
* ═════════════════════════════════════════════════════════════════════════════
32+
* WHAT SHARDING ACTUALLY BUYS, STATED HONESTLY
33+
* ═════════════════════════════════════════════════════════════════════════════
34+
* Each shard is its own parent process reading its own IPC stream, so no single
35+
* parent decodes the whole suite's messages any more — roughly a quarter each.
36+
* The bug is per-message and probabilistic, so this REDUCES exposure roughly in
37+
* proportion. **It does not remove the mechanism, and this file does not claim
38+
* to have fixed anything.** The cure is Node ≥ 24.20 (or a v22 backport that
39+
* does not exist yet); until one of those, this is a smaller target.
40+
*
41+
* There is deliberately NO RETRY here. A retry that turns a genuine red into a
42+
* green is worse than the flake it hides, and a retry loud enough to be safe
43+
* (named in the summary, second failure still fails the job) is a bigger change
44+
* than this one — worth building only if sharding proves not to be enough.
45+
*
46+
* ═════════════════════════════════════════════════════════════════════════════
47+
* WHY `--test-shard` AND NOT A HAND-WRITTEN FILE LIST
48+
* ═════════════════════════════════════════════════════════════════════════════
49+
* Node does its own discovery and its own partitioning. A chunker that globbed
50+
* for test files itself would have to reimplement those rules — and the failure
51+
* mode of getting them slightly wrong is a directory that silently stops being
52+
* tested, which is far worse than an occasional red build. The guards below
53+
* exist for the one way this script itself could drop tests: a wrong index.
54+
*
55+
* ═════════════════════════════════════════════════════════════════════════════
56+
* AND WHY IT IS NOT CALLED `test-shards.mjs`
57+
* ═════════════════════════════════════════════════════════════════════════════
58+
* It was, for about ten minutes. A leading `test-` is one of Node's default
59+
* test-file patterns, so `node --test` discovered this runner as a TEST FILE, ran it
60+
* inside a shard, and that nested run found no files and exited 1 — a red build
61+
* caused by the thing meant to make builds less red. It also inflated the count
62+
* by one, which is what gave it away.
63+
*
64+
* Any name matching `test-*`, `*-test`, `*_test`, `*.test` or anything under a
65+
* `test/` directory will be executed rather than merely run. This is the same
66+
* discovery subtlety that makes hand-rolling the file list a bad idea.
67+
*
68+
* Usage: node scripts/shard-runner.mjs (from backend/)
69+
* TEST_SHARDS=6 node scripts/shard-runner.mjs
70+
*/
71+
72+
import { spawnSync } from 'node:child_process';
73+
import { appendFileSync } from 'node:fs';
74+
75+
/**
76+
* Four is a balance, not a magic number: enough that no parent carries most of
77+
* the stream, few enough that the per-process start-up cost stays small against
78+
* a ~60s suite. Overridable so a bisect can try other values without a commit.
79+
*/
80+
const TOTAL = Number.parseInt(process.env.TEST_SHARDS ?? '4', 10);
81+
if (!Number.isInteger(TOTAL) || TOTAL < 1) {
82+
console.error(`[shard-runner] TEST_SHARDS must be a positive integer, got ${process.env.TEST_SHARDS}`);
83+
process.exit(2);
84+
}
85+
86+
/** `# tests 2032` and friends, off the end of a shard's own summary. */
87+
function counters(output) {
88+
const read = (name) => {
89+
const m = output.match(new RegExp(`^# ${name} (\\d+)$`, 'm'));
90+
return m ? Number(m[1]) : null;
91+
};
92+
return { tests: read('tests'), pass: read('pass'), fail: read('fail'), skipped: read('skipped') };
93+
}
94+
95+
const started = Date.now();
96+
/** @type {{index: number, code: number|null, tests: number|null, pass: number|null, fail: number|null, skipped: number|null}[]} */
97+
const shards = [];
98+
99+
for (let index = 1; index <= TOTAL; index++) {
100+
console.log(`\n═══ shard ${index}/${TOTAL} ═══`);
101+
const run = spawnSync(
102+
process.execPath,
103+
['--test', '--test-concurrency=1', `--test-shard=${index}/${TOTAL}`],
104+
{ encoding: 'utf8', maxBuffer: 256 * 1024 * 1024 }
105+
);
106+
// Streamed after the fact rather than inherited, because the counters have to
107+
// be read back out of it. The whole output is printed either way, so a real
108+
// failure is as legible in the log as it was before.
109+
process.stdout.write(run.stdout ?? '');
110+
if (run.stderr) process.stderr.write(run.stderr);
111+
shards.push({ index, code: run.status, ...counters(run.stdout ?? '') });
112+
}
113+
114+
const sum = (key) => shards.reduce((a, s) => a + (s[key] ?? 0), 0);
115+
const totals = {
116+
tests: sum('tests'),
117+
pass: sum('pass'),
118+
fail: sum('fail'),
119+
skipped: sum('skipped'),
120+
};
121+
122+
const lines = [
123+
`backend suite, ${TOTAL} shards, ${((Date.now() - started) / 1000).toFixed(0)}s`,
124+
...shards.map(
125+
(s) =>
126+
` shard ${s.index}/${TOTAL}: exit ${s.code} · ${s.tests ?? '?'} tests · ` +
127+
`${s.pass ?? '?'} pass · ${s.fail ?? '?'} fail · ${s.skipped ?? '?'} skipped`
128+
),
129+
` TOTAL: ${totals.tests} tests · ${totals.pass} pass · ${totals.fail} fail · ${totals.skipped} skipped`,
130+
];
131+
console.log(`\n${lines.join('\n')}`);
132+
133+
/*
134+
* THE SUMMARY IS WRITTEN WHERE A PERSON WILL SEE IT WITHOUT OPENING THE LOG.
135+
*
136+
* The whole point of splitting the run is that the totals are now assembled by
137+
* this script rather than printed by Node — so if the assembling is wrong, it
138+
* has to be wrong somewhere visible.
139+
*/
140+
if (process.env.GITHUB_STEP_SUMMARY) {
141+
appendFileSync(process.env.GITHUB_STEP_SUMMARY, `### Backend tests\n\n\`\`\`\n${lines.join('\n')}\n\`\`\`\n`);
142+
}
143+
144+
const problems = [];
145+
for (const s of shards) {
146+
if (s.code !== 0) problems.push(`shard ${s.index}/${TOTAL} exited ${s.code}`);
147+
/*
148+
* A SHARD THAT RAN NOTHING IS THE ONE WAY THIS SCRIPT COULD SILENTLY DROP
149+
* TESTS — a wrong index, or an off-by-one in the loop, hands Node a shard it
150+
* has no files for and it exits 0 with an empty run. Node's own discovery
151+
* cannot produce an empty shard for this repo: there are ~111 test files and
152+
* four shards.
153+
*/
154+
if (!s.tests) problems.push(`shard ${s.index}/${TOTAL} reported no tests at all`);
155+
}
156+
if (totals.fail > 0) problems.push(`${totals.fail} failing test(s)`);
157+
158+
if (problems.length > 0) {
159+
console.error(`\n[shard-runner] FAILED: ${problems.join('; ')}`);
160+
process.exit(1);
161+
}
162+
console.log(`[shard-runner] ${TOTAL} shards, all green`);

0 commit comments

Comments
 (0)