From 8b5bd5f57d35796649f6e9a3199a399342f0aab9 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Mon, 9 Nov 2026 11:19:11 +0800 Subject: [PATCH 1/2] fix(test): bound the core test runner's fan-out and give every test file a deadline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Written-by: Chief Fixes #2725 Owner: Dev Reviewer: Reviewer `core/package.json` ran the suite as `node --import ./test/harness-resolve.mjs --test "test/*.test.js"`, so `node --test` fanned out to `availableParallelism() - 1` (191 children on a 192-core box) and no child had a deadline. On this box that is the issue's own report: 241 `spawn … EAGAIN` failures, 163 red top-level tests and no final `# tests` summary after 480 s. The script now passes `--test-concurrency=4 --test-timeout=120000`; the same command finishes in 64 s with 3128 tests, 3124 pass and 0 `EAGAIN`. The `--import` resolver and the `"test/*.test.js"` glob are unchanged, so CI's plain leg (`run: npm test` in `.github/actions/flybot-tests/action.yml`) inherits both flags without a workflow change. `core/test/test-script-bounds.test.js` guards it: fan-out at most 8, a deadline between 60 and 300 s, the resolver and the glob still present, node actually honouring `--test-timeout` (it spawns a deliberately stuck fixture and reads the cancellation), and CI still calling `npm test` rather than spelling out its own `node --test`. `patches/ds-bot/0007-version-and-tooling.patch` carries the matching `-/+` line because `core/package.json` is an upstream file, and `patches/ds-bot/files/test/test-script-bounds.test.js` mirrors the new test. Ran at this tip (extracted `main` tarball at `2ef64199a4229ce68dc9c8caa9e7088ef446a77a`, no git on this box): - new test file on the untouched tree: `node --test test/test-script-bounds.test.js` -> `# tests 5 / # pass 3 / # fail 2` (the two flags missing); same tree with the change -> `5/5` - the unmodified command on this 192-core box: killed at 480 s, exit 124, 241 `EAGAIN`, 315 ok / 163 not ok, no summary - `cd core && TZ=UTC npm test` after the change -> `# tests 3128 / # pass 3124 / # fail 2 / duration_ms 64013`, wall 65 s, 0 `EAGAIN`; both failures are `core/test/check-arch-docs.test.js` `not ok 313 empty diff passes` / `not ok 316 the gate never runs git through a shell`, which fail the same way on untouched main because this box has no `git` — the file and `core/scripts/check-arch-docs.mjs` are byte-identical to main - `node scripts/ds-bot-patches.mjs check` -> `10 patches apply cleanly in order`, `core/ equals ds-bot/ + patches/ds-bot/`; `patches/ds-bot/files/` mirror byte-identical (`cmp`) - `node core/scripts/build-client.mjs --check` exit 0; `oxlint` 0 errors, 87 pre-existing warnings; plugin suites 619/139/161/491 green; `.github/scripts` tests 664 tests / 662 pass / 0 fail - not run: `npm run verify` as one command (`npm run lint` ends at `arch-docs: cannot diff against origin/main`, and `ds-bot-patches canonical` needs git); the smoke test (no `index.js`/`src/host/` change) Docs-not-needed: neither changed path is an owned path in `docs/subsystems/` — `core/package.json` and a new `core/test/*.test.js` file are not in any page's `owned_paths`, and no subsystem behaviour changed. No `changelog.d` fragment either: this changes a developer's test invocation, nothing a FlyBot user sees. --- core/package.json | 2 +- core/test/test-script-bounds.test.js | 81 +++++++++++++++++++ patches/ds-bot/0007-version-and-tooling.patch | 3 +- .../files/test/test-script-bounds.test.js | 81 +++++++++++++++++++ 4 files changed, 165 insertions(+), 2 deletions(-) create mode 100644 core/test/test-script-bounds.test.js create mode 100644 patches/ds-bot/files/test/test-script-bounds.test.js diff --git a/core/package.json b/core/package.json index e1bc451ca..1d730d9b7 100644 --- a/core/package.json +++ b/core/package.json @@ -45,7 +45,7 @@ ], "scripts": { "build": "node scripts/build-client.mjs", - "test": "node --import ./test/harness-resolve.mjs --test \"test/*.test.js\"", + "test": "node --import ./test/harness-resolve.mjs --test --test-concurrency=4 --test-timeout=120000 \"test/*.test.js\"", "lint": "node scripts/lint.mjs", "arch-docs": "node scripts/check-arch-docs.mjs", "harness": "npm install --prefix dev/harness --legacy-peer-deps=false --no-package-lock --no-fund --no-audit", diff --git a/core/test/test-script-bounds.test.js b/core/test/test-script-bounds.test.js new file mode 100644 index 000000000..9bf3c5c56 --- /dev/null +++ b/core/test/test-script-bounds.test.js @@ -0,0 +1,81 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs' +import { execFileSync } from 'node:child_process' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +// `node --test` fans out to `availableParallelism() - 1` files at once — 191 on a 192-core box — +// and a child whose spawn fails (`EAGAIN`) never exits, so the run wedges at the same output for as +// long as the shell stays open (#2725). The `test` script therefore has to name a small fan-out and +// a per-file deadline itself rather than inherit the host's core count. +const root = new URL('../../', import.meta.url) +const read = path => readFileSync(new URL(path, root), 'utf8') +const script = JSON.parse(read('core/package.json')).scripts.test +const flag = name => { + const match = script.match(new RegExp(`--${name}=(\\d+)(?:\\s|$)`)) + return match ? Number(match[1]) : null +} + +test('the test script bounds its fan-out instead of inheriting the core count', () => { + const concurrency = flag('test-concurrency') + assert.notEqual(concurrency, null, 'npm test passes no --test-concurrency, so it starts one child per core') + assert.ok(concurrency >= 1, `--test-concurrency=${concurrency} starts no test file`) + assert.ok(concurrency <= 8, `--test-concurrency=${concurrency} is not a small constant; a high-core host spawns that many at once`) +}) + +test('the test script gives every test file a deadline', () => { + const timeout = flag('test-timeout') + assert.notEqual(timeout, null, 'npm test passes no --test-timeout, so one stuck file holds the run open') + assert.ok(timeout >= 60000, `--test-timeout=${timeout} is too short for the slower host files`) + assert.ok(timeout <= 300000, `--test-timeout=${timeout} is longer than a run of the whole suite`) +}) + +test('the test script still runs the harness resolver over the whole suite', () => { + assert.ok(script.includes('--import ./test/harness-resolve.mjs'), 'the harness resolver is what makes `@deepseek-ai/*` load in the tests') + assert.ok(script.includes('"test/*.test.js"'), 'the glob is the whole suite; a narrower one would hide files') +}) + +// The flag has to reach node's own parser, not just sit in package.json: this runs the same deadline +// over a file that would otherwise never return, and reads the cancellation it produces. +test('a stuck test file fails inside the deadline instead of hanging', () => { + const dir = mkdtempSync(join(tmpdir(), 'flybot-stuck-')) + try { + writeFileSync(join(dir, 'stuck.test.js'), [ + "import { test } from 'node:test'", + "test('deliberately stuck', () => new Promise(resolve => setTimeout(resolve, 60000)))", + '', + ].join('\n')) + let output = '' + let status = 0 + try { + output = execFileSync(process.execPath, ['--test', '--test-timeout=750', join(dir, 'stuck.test.js')], { + encoding: 'utf8', + // A node that ignored the flag would hang here; killing it makes this test fail instead. + timeout: 30000, + stdio: ['ignore', 'pipe', 'pipe'], + // Inherited from the runner around this file, it makes the child run as a test child of + // this one: it exits 0 at once and never parses `--test-timeout` at all. + env: { ...process.env, NODE_TEST_CONTEXT: undefined }, + }) + } catch (error) { + output = `${error.stdout ?? ''}${error.stderr ?? ''}` + status = error.status + } + assert.notEqual(status, 0, 'the stuck file has to fail, not pass or be killed from outside') + assert.match(output, /test timed out after 750ms/, 'node has to cancel it through the deadline and name the failure') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +// The plain CI path is `npm test`, which is where the bounds live; a step that spells out its own +// `node --test` would go back to starting one child per core (#2723 owns the coverage invocation). +test('CI runs the plain suite through npm test, so it inherits the bounds', () => { + const lines = read('.github/actions/flybot-tests/action.yml').split('\n') + const start = lines.findIndex(line => line.trim() === '- name: Unit tests') + assert.notEqual(start, -1, 'the plain unit-test step is gone') + const step = lines.slice(start, start + 6).map(line => line.trim()) + assert.ok(step.includes("if: inputs.coverage != 'true'"), 'the plain step no longer guards on coverage') + assert.ok(step.includes('run: npm test'), 'the plain CI step must call npm test, not its own node --test invocation') +}) diff --git a/patches/ds-bot/0007-version-and-tooling.patch b/patches/ds-bot/0007-version-and-tooling.patch index 23ec8e5c8..13f447b85 100644 --- a/patches/ds-bot/0007-version-and-tooling.patch +++ b/patches/ds-bot/0007-version-and-tooling.patch @@ -778,7 +778,8 @@ diff --git a/package.json b/package.json ], "scripts": { "build": "node scripts/build-client.mjs", - "test": "node --import ./test/harness-resolve.mjs --test \"test/*.test.js\"", +- "test": "node --import ./test/harness-resolve.mjs --test \"test/*.test.js\"", ++ "test": "node --import ./test/harness-resolve.mjs --test --test-concurrency=4 --test-timeout=120000 \"test/*.test.js\"", "lint": "node scripts/lint.mjs", + "arch-docs": "node scripts/check-arch-docs.mjs", "harness": "npm install --prefix dev/harness --legacy-peer-deps=false --no-package-lock --no-fund --no-audit", diff --git a/patches/ds-bot/files/test/test-script-bounds.test.js b/patches/ds-bot/files/test/test-script-bounds.test.js new file mode 100644 index 000000000..9bf3c5c56 --- /dev/null +++ b/patches/ds-bot/files/test/test-script-bounds.test.js @@ -0,0 +1,81 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { readFileSync, mkdtempSync, writeFileSync, rmSync } from 'node:fs' +import { execFileSync } from 'node:child_process' +import { tmpdir } from 'node:os' +import { join } from 'node:path' + +// `node --test` fans out to `availableParallelism() - 1` files at once — 191 on a 192-core box — +// and a child whose spawn fails (`EAGAIN`) never exits, so the run wedges at the same output for as +// long as the shell stays open (#2725). The `test` script therefore has to name a small fan-out and +// a per-file deadline itself rather than inherit the host's core count. +const root = new URL('../../', import.meta.url) +const read = path => readFileSync(new URL(path, root), 'utf8') +const script = JSON.parse(read('core/package.json')).scripts.test +const flag = name => { + const match = script.match(new RegExp(`--${name}=(\\d+)(?:\\s|$)`)) + return match ? Number(match[1]) : null +} + +test('the test script bounds its fan-out instead of inheriting the core count', () => { + const concurrency = flag('test-concurrency') + assert.notEqual(concurrency, null, 'npm test passes no --test-concurrency, so it starts one child per core') + assert.ok(concurrency >= 1, `--test-concurrency=${concurrency} starts no test file`) + assert.ok(concurrency <= 8, `--test-concurrency=${concurrency} is not a small constant; a high-core host spawns that many at once`) +}) + +test('the test script gives every test file a deadline', () => { + const timeout = flag('test-timeout') + assert.notEqual(timeout, null, 'npm test passes no --test-timeout, so one stuck file holds the run open') + assert.ok(timeout >= 60000, `--test-timeout=${timeout} is too short for the slower host files`) + assert.ok(timeout <= 300000, `--test-timeout=${timeout} is longer than a run of the whole suite`) +}) + +test('the test script still runs the harness resolver over the whole suite', () => { + assert.ok(script.includes('--import ./test/harness-resolve.mjs'), 'the harness resolver is what makes `@deepseek-ai/*` load in the tests') + assert.ok(script.includes('"test/*.test.js"'), 'the glob is the whole suite; a narrower one would hide files') +}) + +// The flag has to reach node's own parser, not just sit in package.json: this runs the same deadline +// over a file that would otherwise never return, and reads the cancellation it produces. +test('a stuck test file fails inside the deadline instead of hanging', () => { + const dir = mkdtempSync(join(tmpdir(), 'flybot-stuck-')) + try { + writeFileSync(join(dir, 'stuck.test.js'), [ + "import { test } from 'node:test'", + "test('deliberately stuck', () => new Promise(resolve => setTimeout(resolve, 60000)))", + '', + ].join('\n')) + let output = '' + let status = 0 + try { + output = execFileSync(process.execPath, ['--test', '--test-timeout=750', join(dir, 'stuck.test.js')], { + encoding: 'utf8', + // A node that ignored the flag would hang here; killing it makes this test fail instead. + timeout: 30000, + stdio: ['ignore', 'pipe', 'pipe'], + // Inherited from the runner around this file, it makes the child run as a test child of + // this one: it exits 0 at once and never parses `--test-timeout` at all. + env: { ...process.env, NODE_TEST_CONTEXT: undefined }, + }) + } catch (error) { + output = `${error.stdout ?? ''}${error.stderr ?? ''}` + status = error.status + } + assert.notEqual(status, 0, 'the stuck file has to fail, not pass or be killed from outside') + assert.match(output, /test timed out after 750ms/, 'node has to cancel it through the deadline and name the failure') + } finally { + rmSync(dir, { recursive: true, force: true }) + } +}) + +// The plain CI path is `npm test`, which is where the bounds live; a step that spells out its own +// `node --test` would go back to starting one child per core (#2723 owns the coverage invocation). +test('CI runs the plain suite through npm test, so it inherits the bounds', () => { + const lines = read('.github/actions/flybot-tests/action.yml').split('\n') + const start = lines.findIndex(line => line.trim() === '- name: Unit tests') + assert.notEqual(start, -1, 'the plain unit-test step is gone') + const step = lines.slice(start, start + 6).map(line => line.trim()) + assert.ok(step.includes("if: inputs.coverage != 'true'"), 'the plain step no longer guards on coverage') + assert.ok(step.includes('run: npm test'), 'the plain CI step must call npm test, not its own node --test invocation') +}) From 20f5cc304675e5af3c023bef04a7ea0356ded368 Mon Sep 17 00:00:00 2001 From: Danyang Chen Date: Mon, 9 Nov 2026 11:26:27 +0800 Subject: [PATCH 2/2] test(test-script-bounds): read the deadline cancellation as either node version reports it Node 22 names the deadline ("test timed out after 750ms"); node 24 reports the cancellation as the stuck test's own duration and then waits for the pending timer, so the assertion that matched only the first wording failed on the Node 24 CI leg. The test now requires a failure report and proves the failure came from the 750ms deadline rather than from the 60s the fixture asks for. --- core/test/test-script-bounds.test.js | 15 +++++++++------ .../ds-bot/files/test/test-script-bounds.test.js | 15 +++++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/core/test/test-script-bounds.test.js b/core/test/test-script-bounds.test.js index 9bf3c5c56..c15926ea2 100644 --- a/core/test/test-script-bounds.test.js +++ b/core/test/test-script-bounds.test.js @@ -47,12 +47,11 @@ test('a stuck test file fails inside the deadline instead of hanging', () => { '', ].join('\n')) let output = '' - let status = 0 try { output = execFileSync(process.execPath, ['--test', '--test-timeout=750', join(dir, 'stuck.test.js')], { encoding: 'utf8', - // A node that ignored the flag would hang here; killing it makes this test fail instead. - timeout: 30000, + // A node that ignored the flag would hang here instead of failing this test. + timeout: 20000, stdio: ['ignore', 'pipe', 'pipe'], // Inherited from the runner around this file, it makes the child run as a test child of // this one: it exits 0 at once and never parses `--test-timeout` at all. @@ -60,10 +59,14 @@ test('a stuck test file fails inside the deadline instead of hanging', () => { }) } catch (error) { output = `${error.stdout ?? ''}${error.stderr ?? ''}` - status = error.status } - assert.notEqual(status, 0, 'the stuck file has to fail, not pass or be killed from outside') - assert.match(output, /test timed out after 750ms/, 'node has to cancel it through the deadline and name the failure') + assert.match(output, /(?:not ok|✖)/, 'node has to report the stuck file as failed, not leave it pending') + // The wording is a node detail: 22 says "test timed out after 750ms", 24 reports the cancellation + // as the test's own duration. Both are the deadline; the fixture asks for 60s. + const named = /test timed out after 750ms/.test(output) + const reported = output.match(/✖ deliberately stuck \((\d+(?:\.\d+)?)ms\)/) + const cancelled = reported ? Number(reported[1]) < 5000 : false + assert.ok(named || cancelled, 'the failure has to come from the deadline, not from the fixture finishing on its own') } finally { rmSync(dir, { recursive: true, force: true }) } diff --git a/patches/ds-bot/files/test/test-script-bounds.test.js b/patches/ds-bot/files/test/test-script-bounds.test.js index 9bf3c5c56..c15926ea2 100644 --- a/patches/ds-bot/files/test/test-script-bounds.test.js +++ b/patches/ds-bot/files/test/test-script-bounds.test.js @@ -47,12 +47,11 @@ test('a stuck test file fails inside the deadline instead of hanging', () => { '', ].join('\n')) let output = '' - let status = 0 try { output = execFileSync(process.execPath, ['--test', '--test-timeout=750', join(dir, 'stuck.test.js')], { encoding: 'utf8', - // A node that ignored the flag would hang here; killing it makes this test fail instead. - timeout: 30000, + // A node that ignored the flag would hang here instead of failing this test. + timeout: 20000, stdio: ['ignore', 'pipe', 'pipe'], // Inherited from the runner around this file, it makes the child run as a test child of // this one: it exits 0 at once and never parses `--test-timeout` at all. @@ -60,10 +59,14 @@ test('a stuck test file fails inside the deadline instead of hanging', () => { }) } catch (error) { output = `${error.stdout ?? ''}${error.stderr ?? ''}` - status = error.status } - assert.notEqual(status, 0, 'the stuck file has to fail, not pass or be killed from outside') - assert.match(output, /test timed out after 750ms/, 'node has to cancel it through the deadline and name the failure') + assert.match(output, /(?:not ok|✖)/, 'node has to report the stuck file as failed, not leave it pending') + // The wording is a node detail: 22 says "test timed out after 750ms", 24 reports the cancellation + // as the test's own duration. Both are the deadline; the fixture asks for 60s. + const named = /test timed out after 750ms/.test(output) + const reported = output.match(/✖ deliberately stuck \((\d+(?:\.\d+)?)ms\)/) + const cancelled = reported ? Number(reported[1]) < 5000 : false + assert.ok(named || cancelled, 'the failure has to come from the deadline, not from the fixture finishing on its own') } finally { rmSync(dir, { recursive: true, force: true }) }