Skip to content

fix(test): cap local unit-test concurrency at 4 to avoid exhausting commit charge - #13187

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.51from
huuhungn:fix/local-test-concurrency-commit-exhaustion
Sep 11, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.51from
huuhungn:fix/local-test-concurrency-commit-exhaustion

Conversation

@anhtahaylove

Copy link
Copy Markdown
Contributor

Problem

The local test and test:unit scripts run with --test-concurrency=20 and --max-old-space-size=8192. Those multiply: up to 20 workers × 8 GB of heap reservation each. On a 32 GB developer machine with a ~53.8 GB commit limit, a full-suite run drives commit charge to the ceiling, and Windows starts refusing process creation — unrelated long-running Node processes die with 0xC0000142 (STATUS_DLL_INIT_FAILED, the usual symptom of a failed allocation at process start).

This is a local-only footgun. CI never hits it: ci.yml and quality.yml run test:unit:ci:shard, which already uses --test-concurrency=4 and spreads the suite across 8 GitHub-hosted runners with 16 GB each. The unsharded test/test:unit scripts are the ones a developer types by hand, and they are the only ones tuned for a machine that doesn't exist.

Fix

Set --test-concurrency=4 on both, matching test:unit:ci.

Also added the missing --test-force-exit to test. Without it the runner finishes every test and then hangs indefinitely on a live handle instead of exiting — test:unit and all the CI variants already pass this flag; test was the odd one out.

Verification

Full suite before and after, same machine, same base commit — the only difference is this package.json change:

commit charge peak wall time pass fail
--test-concurrency=20 ceiling (~53.8 GB), processes killed — — —
--test-concurrency=4 37.8 GB (~16 GB headroom) 1418 s 38114 791

Sampled commit charge every 9 s across the run. No unrelated process died, and the local gateway on port 20128 stayed up throughout.

Results are unchanged: 413 of 415 distinct failing test names are identical across the two runs, differing by 2 in each direction — ordinary flakiness, not a behaviour change. (Those 791 failures are pre-existing on this base on Windows, mostly spawn/ENOENT from tooling this machine doesn't have installed. They are unrelated to this change.)

Wall time is not meaningfully worse: 1418 s here versus 1499 s for the pre-change baseline run. The 20-way setting was buying nothing — the suite is I/O-bound long before it is CPU-bound, and the oversubscription was pure memory pressure.

@anhtahaylove

Copy link
Copy Markdown
Contributor Author

Self-review note, and a pre-existing bug this change does not fix.

Two things worth a maintainer'''s eye:

1. This PR also adds --test-force-exit to test. That is a behaviour change beyond what the title promises, so calling it out explicitly: without the flag the runner completes every test and then hangs indefinitely on a live handle instead of exiting (that is how I hit this in the first place). Every other runner — test:unit, test:unit:ci, :ci:shard, :fast, :shard:1/2 — already passes it; test was the only one that did not. Happy to split it into its own PR if you would rather keep this one purely about concurrency.

Worth noting the mild irony: --test-force-exit masks exactly the class of bug the rest of this audit is about (a live handle keeping the process alive). It is the right call for a test runner — a hang gives you no signal at all — but it does mean the suite cannot be used to detect leaks by watching whether it exits.

2. Pre-existing, untouched here: test is missing the serial quarantine step. test:unit, test:unit:ci, test:unit:fast and test:coverage:runner all end with && npm run test:unit:serial, and tests/unit/test-serial-quarantine.test.ts asserts exactly that for those four. test is absent from that list and does not chain the serial pass — so a developer typing plain npm test silently skips the four quarantined files from #6347 (glm-coding-plan-monthly-3580, quota-division-blocks, provider-health-autopilot, combo-health-autopilot).

Either test should chain the serial step like its siblings, or — if it is a deliberate legacy alias — it should be removed so nobody reaches for it. I have left it alone because it is out of scope here and the fix depends on which of those you intend. The quarantine guard still passes 4/4 with this PR applied.

@HouMinXi

Copy link
Copy Markdown
Contributor

Thanks for capping local unit-test concurrency. I did not fold this into the local recut of release/v3.8.51.

Conflict was only package.json. Incoming dropped the local runner from 20 workers to 4; the recut keeps 20 so the deploy-tree gate finishes in one pass. No other files dropped.

Purpose of the recut: ship a production image, not change local test pacing. No objection to the change on developer laptops; it is not a production-image change, so it stays out of this recut.

@diegosouzapw
diegosouzapw merged commit 178d252 into diegosouzapw:release/v3.8.51 Sep 11, 2026
8 of 16 checks passed
patrykkopycinski added a commit to patrykkopycinski/OmniRoute that referenced this pull request Sep 16, 2026
The dedupe made `test` delegate to `test:unit`, but it also silently
reverted the deliberate local concurrency cap from diegosouzapw#13187 ("cap local
unit-test concurrency at 4 to avoid exhausting commit charge") back to 20.

Measured on m1max (16 cores), same suite and same tree, only the flag differs:
  concurrency=20 -> 356 cancelled, 356 "event loop has already resolved" bailouts
  concurrency=4  -> 0 cancelled, 0 bailouts
So 20 does not just slow the run down, it makes the runner abandon tests and
still print a summary -- a false green. Restore 4; termination is preserved via
delegation to test:unit, which already carries --test-force-exit.
patrykkopycinski added a commit to patrykkopycinski/OmniRoute that referenced this pull request Sep 16, 2026
The diegosouzapw#13187 batch commit (178d252) already gave `test` both `--test-force-exit` flags and the trailing `&& npm run test:unit:serial` step, so the delegation fix was redundant. It also broke tests/unit/test-serial-quarantine.test.ts, which asserts every parallel runner script ends with the serial step (base 4/4 -> head 3/4). package.json is now byte-identical to base; this PR is the stryker tap.testFiles fix only.
diegosouzapw pushed a commit that referenced this pull request Sep 18, 2026
…iles (#13357)

* fix(test): make npm run test terminate and restore RAYCAST env-doc sync

Two independent defects, both in the test/dev entrypoint layer.

1. `npm run test` never terminated. It was a hand-maintained copy of
   `test:unit` that had drifted: it omitted `--test-force-exit` on BOTH
   node invocations and dropped the trailing `&& npm run test:unit:serial`.
   Per AGENTS.md ('Database Handles in Tests'), unreleased SQLite handles
   make Node's native runner hang indefinitely — every sibling script
   (`test:unit`, `test:unit:ci`, `test:unit:ci:shard`) already carried the
   flag; only `test` did not. Measured on m1max at 84c6ad7, same suite
   both arms: without the flag the runner was killed at the 420s ceiling
   (exit 137, no summary line, 23 orphaned node processes); with it the
   runner exited on its own in 419s leaving 1. `test` now delegates to
   `test:unit` so the two cannot drift again, which also makes the serial
   suite reachable from `npm run test` for the first time.

2. Removing the RAYCAST_* rows from ENVIRONMENT.md (#9) broke
   check-env-doc-sync. `parseEnvExampleVars` matches `^#?\s*(VAR)=`, so it
   counts COMMENTED-OUT vars: the four entries still sat at
   .env.example:1263-1266 and became `envMissingDoc` drift the moment their
   docs disappeared. The #9 verification only ran the fabricated-docs gate
   and missed this one. The block is dead either way — it documents
   open-sse/services/raycast.ts and scripts/raycast/usage-benchmark.mjs,
   both deleted with the GPL-derived provider in #11691, and no live code
   reads the vars — so it is removed rather than re-documented.

envMissingDoc is now []. The remaining codeMissingEnv failure
(CURSOR_AGENT_BINARY, CURSOR_MAX_FRAME_BYTES, OMNIROOT) is pre-existing
drift on the base, absent from this diff, and left alone.

* chore(stryker): register 3 covering unit tests missing from tap.testFiles

check:mutation-test-coverage --strict fails identically on pristine
release/v3.8.51 (f1e7148) with an empty diff — base debt blocking this PR.

- combo-identical-error-streak.test.ts -> comboPredicates.ts
- 13601-header-drop-count-surfaced.test.ts -> responseHeaders.ts
- semantic-cache-no-truncated-writes.test.ts -> semanticCache.ts

* fix(test): keep the #13187 concurrency-4 cap in test:unit

The dedupe made `test` delegate to `test:unit`, but it also silently
reverted the deliberate local concurrency cap from #13187 ("cap local
unit-test concurrency at 4 to avoid exhausting commit charge") back to 20.

Measured on m1max (16 cores), same suite and same tree, only the flag differs:
  concurrency=20 -> 356 cancelled, 356 "event loop has already resolved" bailouts
  concurrency=4  -> 0 cancelled, 0 bailouts
So 20 does not just slow the run down, it makes the runner abandon tests and
still print a summary -- a false green. Restore 4; termination is preserved via
delegation to test:unit, which already carries --test-force-exit.

* revert(test): drop redundant test-script delegation

The #13187 batch commit (178d252) already gave `test` both `--test-force-exit` flags and the trailing `&& npm run test:unit:serial` step, so the delegation fix was redundant. It also broke tests/unit/test-serial-quarantine.test.ts, which asserts every parallel runner script ends with the serial step (base 4/4 -> head 3/4). package.json is now byte-identical to base; this PR is the stryker tap.testFiles fix only.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…ommit charge (diegosouzapw#13187)

Aligns the two hand-typed local scripts with `test:unit:ci`, which already ran at concurrency 4; `--test-force-exit` was likewise the one flag `test` was missing. CI scripts are untouched.

---

Validated in one consolidated worktree cut from `release/v3.8.51`, boarded together with the other 13 PRs of this batch — zero merge conflicts between them.

- `typecheck:core` clean
- complexity 2799 / baseline 3218 and cognitive-complexity 1265 / baseline 1437 — both under baseline
- 71 focused assertions green across the 13 test files this batch adds or touches

⚠️ base-red inherited: diegosouzapw#12732 — `Docs Gates (fast-path)`, `Merge integrity`, `No new ESLint warnings`, `Unit Tests fast-path` and `Fast Quality Gates` all reproduce on the pure `release/v3.8.51` tip (provider count 356 vs the 358 the modules define, SKILL.md drift, and `open-sse/utils/stream.ts` at 3115 > frozen 3098). None of them touch this diff.

Thanks @anhtahaylove — the root-cause write-up, the measured before/after numbers and the red-before-green proof on every one of these made the batch reviewable as a unit.
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…iles (diegosouzapw#13357)

* fix(test): make npm run test terminate and restore RAYCAST env-doc sync

Two independent defects, both in the test/dev entrypoint layer.

1. `npm run test` never terminated. It was a hand-maintained copy of
   `test:unit` that had drifted: it omitted `--test-force-exit` on BOTH
   node invocations and dropped the trailing `&& npm run test:unit:serial`.
   Per AGENTS.md ('Database Handles in Tests'), unreleased SQLite handles
   make Node's native runner hang indefinitely — every sibling script
   (`test:unit`, `test:unit:ci`, `test:unit:ci:shard`) already carried the
   flag; only `test` did not. Measured on m1max at 84c6ad7, same suite
   both arms: without the flag the runner was killed at the 420s ceiling
   (exit 137, no summary line, 23 orphaned node processes); with it the
   runner exited on its own in 419s leaving 1. `test` now delegates to
   `test:unit` so the two cannot drift again, which also makes the serial
   suite reachable from `npm run test` for the first time.

2. Removing the RAYCAST_* rows from ENVIRONMENT.md (diegosouzapw#9) broke
   check-env-doc-sync. `parseEnvExampleVars` matches `^#?\s*(VAR)=`, so it
   counts COMMENTED-OUT vars: the four entries still sat at
   .env.example:1263-1266 and became `envMissingDoc` drift the moment their
   docs disappeared. The diegosouzapw#9 verification only ran the fabricated-docs gate
   and missed this one. The block is dead either way — it documents
   open-sse/services/raycast.ts and scripts/raycast/usage-benchmark.mjs,
   both deleted with the GPL-derived provider in diegosouzapw#11691, and no live code
   reads the vars — so it is removed rather than re-documented.

envMissingDoc is now []. The remaining codeMissingEnv failure
(CURSOR_AGENT_BINARY, CURSOR_MAX_FRAME_BYTES, OMNIROOT) is pre-existing
drift on the base, absent from this diff, and left alone.

* chore(stryker): register 3 covering unit tests missing from tap.testFiles

check:mutation-test-coverage --strict fails identically on pristine
release/v3.8.51 (7339a9f) with an empty diff — base debt blocking this PR.

- combo-identical-error-streak.test.ts -> comboPredicates.ts
- 13601-header-drop-count-surfaced.test.ts -> responseHeaders.ts
- semantic-cache-no-truncated-writes.test.ts -> semanticCache.ts

* fix(test): keep the diegosouzapw#13187 concurrency-4 cap in test:unit

The dedupe made `test` delegate to `test:unit`, but it also silently
reverted the deliberate local concurrency cap from diegosouzapw#13187 ("cap local
unit-test concurrency at 4 to avoid exhausting commit charge") back to 20.

Measured on m1max (16 cores), same suite and same tree, only the flag differs:
  concurrency=20 -> 356 cancelled, 356 "event loop has already resolved" bailouts
  concurrency=4  -> 0 cancelled, 0 bailouts
So 20 does not just slow the run down, it makes the runner abandon tests and
still print a summary -- a false green. Restore 4; termination is preserved via
delegation to test:unit, which already carries --test-force-exit.

* revert(test): drop redundant test-script delegation

The diegosouzapw#13187 batch commit (5d3c7d6) already gave `test` both `--test-force-exit` flags and the trailing `&& npm run test:unit:serial` step, so the delegation fix was redundant. It also broke tests/unit/test-serial-quarantine.test.ts, which asserts every parallel runner script ends with the serial step (base 4/4 -> head 3/4). package.json is now byte-identical to base; this PR is the stryker tap.testFiles fix only.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants