Skip to content

fix(engine): cap auto workers by parent heap budget - #3803

Draft
vanceingalls wants to merge 1 commit into
mainfrom
fix/prinfra-338-heap-worker-cap
Draft

fix(engine): cap auto workers by parent heap budget#3803
vanceingalls wants to merge 1 commit into
mainfrom
fix/prinfra-338-heap-worker-cap

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

The reported 18-core/24GB host selected six capture workers despite a roughly 4GB parent V8 heap. Raising the Chrome RSS budget alone still selects six. Apply the existing parent-heap estimate after the parallel floor and contention cap; that host now selects four workers, and a heap below the reserve selects one. Explicit --workers remains authoritative, and sizing telemetry reports boundBy: heap.

Related: PRINFRA-338, PRINFRA-341. Diagnosis confidence: 80% for heap pressure/auto-sizing involvement; this is not proof that every captured-frame OOM follows the same model.

Draft / merge gate: PRINFRA-341 deliberately deferred enforcement because 640MB/worker and the 1024MB reserve come from one field report. Before merging, validate the existing workers_heap_* telemetry against OOMs and successful renders, and benchmark representative compositions. This proposal can materially reduce concurrency on low-default-heap hosts. It does not address the separate large-font compilation OOM.

Validation: 56 coordinator tests passed, including five deterministic heap-budget cases; engine and producer typechecks, scoped oxlint/oxfmt, and the fallow new-issue gate passed. No fleet performance claim is made.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment-only: this is a draft, so no stamp from me. Read the full parallelCoordinator.ts sizing path and the new test at head.

Your own description already names the central risk — that 640MB/worker and the 1024MB reserve come from one field report, that PRINFRA-341 deliberately deferred enforcement, and that this can materially reduce concurrency on low-default-heap hosts. I'm not going to restate that back at you. Everything below is what I found on top of it.

1. The in-source comment will contradict the code the moment this merges

The replacement comment on HEAP_PER_WORKER_MB reads "Validate this estimate against the workers_heap_ fleet telemetry before rollout (PRINFRA-341)"* while :409-412 now enforces the cap. The merge gate lives in the PR description, but the description is not what the next reader sees — the comment is, and it will say validation is still pending while enforcement is live. The old comment was self-consistent because it said "advisory-only" and the code was advisory. Two ways to keep that property: gate enforcement behind a config knob so the comment stays true until you flip it, or land it with the comment replaced by what validated the figure.

2. The fleet-visible thresholds are wider than the reported host's 6 → 4

The cap is Math.max(1, Math.floor((heapLimitMb - 1024) / 640)) (:323-326), so:

  • On a default ~4GB Node heap it is 4 workers for every auto-sized render, regardless of core count. A 32-core host that the contention path sizes to 10 (per the comment's own "32 cores → 10 workers") also lands at 4. That is a much broader statement than the 18-core host going 6 → 4.
  • Any host whose heap_size_limit is below ~1664MB gets exactly 1 worker, because that is where the floor takes over. That also overrides the two-worker parallel floor by design (:403-412, pinned by "allows one worker below the heap reserve despite the parallel floor"), so a long render on such a host becomes fully serial.

Both numbers are worth putting in the benchmark plan explicitly, since they are what the fleet will actually experience.

3. exceedsHeapAdvisory becomes unreachable for auto-sized renders

:347 computes workers > heapBasedWorkers. Once enforcement guarantees finalWorkers <= heapBasedWorkers, that can only be true via the explicit-requested path, so the captureCost.ts warning is live only for explicit --workers. Your updated comment says as much, and keeping it is reasonable — flagging only because "defensive warning for a future alternate policy" is exactly the shape that reads as dead code to whoever touches it next. A one-line note naming the explicit path as its only live caller would prevent that.

4. Interaction with #3801, since these were sent together

Safe, and worth stating why: #3801's initialization retry sets currentWorkers = getNextRetryWorkerCount(currentWorkers) directly and never re-enters computeWorkerSizing, and it only ever halves — so the heap cap cannot be re-raised on retry, and the two changes compose in the same direction. The thing to notice is that they stack: on a default-heap host this PR sizes to 4, and a Network.enable timeout then halves to 2, so the retry concurrency the field sees is set by both PRs together rather than by either one.

5. The test is genuinely deterministic — I checked the two things that usually make this shape lie

  • parallelCoordinator.ts:8 imports cpus from bare "os", which is what vi.mock("os", …) intercepts. Had it imported node:os, the mock would silently not apply and the CPU-cap case would be scored against this host's real core count.
  • getHeapStatistics is called exactly once per computeWorkerSizing (:322), so mockReturnValueOnce is sufficient and no assertion falls through to the real host heap.

Both fine as written. Mentioning it so the next person doesn't have to re-derive it.

Also minor: WorkerSizingBound is publicly exported (packages/engine/src/index.ts:258) and "heap" is a new member. The only consumer is telemetry passthrough (packages/cli/src/commands/render.ts:1525) — no exhaustive switch to break — so it is additive for JS consumers and mildly breaking only for a TS consumer switching exhaustively over the union.

Head reviewed: 5c8e8752d30ecf7f347e785271acd7792547fb7c

Verdict: COMMENT
Reasoning: The mechanism is correct, the floor prevents a zero-worker path, and the tests control their inputs properly — but it is a draft and the enforcement decision is the open question you already gated it on. Items 1 and 2 are the ones I would want resolved before it leaves draft.

— Rames Jusso

@vanceingalls
vanceingalls marked this pull request as ready for review September 9, 2026 07:45
@vanceingalls
vanceingalls marked this pull request as draft September 9, 2026 07:45
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.

2 participants