From 5c8e8752d30ecf7f347e785271acd7792547fb7c Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Tue, 8 Sep 2026 23:48:04 -0700 Subject: [PATCH 1/2] fix(engine): cap auto workers by parent heap budget --- .../services/parallelCoordinator.heap.test.ts | 65 +++++++++++++++++++ .../src/services/parallelCoordinator.ts | 22 ++++--- .../src/services/render/captureCost.ts | 7 +- 3 files changed, 81 insertions(+), 13 deletions(-) create mode 100644 packages/engine/src/services/parallelCoordinator.heap.test.ts diff --git a/packages/engine/src/services/parallelCoordinator.heap.test.ts b/packages/engine/src/services/parallelCoordinator.heap.test.ts new file mode 100644 index 0000000000..a920e63863 --- /dev/null +++ b/packages/engine/src/services/parallelCoordinator.heap.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it, vi } from "vitest"; +import { getHeapStatistics } from "v8"; +import { computeWorkerSizing } from "./parallelCoordinator.js"; + +vi.mock("os", async (importOriginal) => ({ + ...(await importOriginal()), + cpus: () => + Array.from({ length: 18 }, () => ({ + model: "test", + speed: 3000, + times: { user: 0, nice: 0, sys: 0, idle: 0, irq: 0 }, + })), +})); +vi.mock("./systemMemory.js", () => ({ getSystemTotalMb: () => 24576 })); +vi.mock("v8", async (importOriginal) => { + const original = await importOriginal(); + return { ...original, getHeapStatistics: vi.fn(original.getHeapStatistics) }; +}); + +function withHeapLimit(mb: number): void { + vi.mocked(getHeapStatistics).mockReturnValueOnce({ + ...getHeapStatistics(), + heap_size_limit: mb * 1024 * 1024, + }); +} + +describe("parent heap worker cap", () => { + it("caps the reported 18-core/24GB host at four workers with a 4GB heap", () => { + withHeapLimit(4096); + const result = computeWorkerSizing(1800); + expect(result).toMatchObject({ + workers: 4, + heapBasedWorkers: 4, + boundBy: "heap", + exceedsHeapAdvisory: false, + }); + }); + + it("allows one worker below the heap reserve despite the parallel floor", () => { + withHeapLimit(512); + expect(computeWorkerSizing(1800)).toMatchObject({ workers: 1, boundBy: "heap" }); + }); + + it("keeps the CPU contention cap when the heap has sufficient room", () => { + withHeapLimit(16384); + expect(computeWorkerSizing(1800)).toMatchObject({ workers: 6 }); + }); + + it("honors explicit workers and retains the exceeded-budget diagnostic", () => { + withHeapLimit(1024); + expect(computeWorkerSizing(1800, 6)).toMatchObject({ + workers: 6, + boundBy: "explicit", + exceedsHeapAdvisory: true, + }); + }); + + it("does not let configured concurrency bypass the heap budget", () => { + withHeapLimit(1024); + expect(computeWorkerSizing(1800, undefined, { concurrency: 12 })).toMatchObject({ + workers: 1, + boundBy: "heap", + }); + }); +}); diff --git a/packages/engine/src/services/parallelCoordinator.ts b/packages/engine/src/services/parallelCoordinator.ts index 45098d2eab..1ddc1f1f6a 100644 --- a/packages/engine/src/services/parallelCoordinator.ts +++ b/packages/engine/src/services/parallelCoordinator.ts @@ -141,10 +141,8 @@ const MEMORY_PER_WORKER_MB = 1536; const HEAP_RESERVED_MB = 1024; // Parent-process V8 heap consumed per worker (protocol buffers + in-flight // frame buffers). Derived from the field OOM: 6 workers exhausted a ~4GB -// default heap ⇒ >~500MB/worker + base. ponytail: advisory-only until the -// workers_heap_* telemetry added alongside this constant validates the figure -// — enforcing a guessed budget could silently cut worker counts fleet-wide. -// TODO(PRINFRA-341): decide enforcement after ~2 weeks of fleet soak. +// default heap ⇒ >~500MB/worker + base. Validate this estimate against the +// workers_heap_* fleet telemetry before rollout (PRINFRA-341). const HEAP_PER_WORKER_MB = 640; const MIN_WORKERS = 1; const MAX_WORKER_DIAGNOSTIC_LINES = 8; @@ -272,6 +270,7 @@ export type WorkerSizingBound = | "too_few_frames" | "cpu" | "memory" + | "heap" | "frames" | "max_workers" | "min_parallel_floor" @@ -290,9 +289,8 @@ export interface WorkerSizing { frameBasedWorkers: number; effectiveMaxWorkers: number; /** - * ADVISORY, not enforced (see HEAP_PER_WORKER_MB): how many workers the - * parent process's V8 heap could feed. Compare against `workers` in - * telemetry to validate the budget before enforcement. + * Auto-sizing cap: how many workers the parent process's V8 heap could + * feed. Explicit worker requests may exceed this budget. */ heapBasedWorkers: number; /** V8 `heap_size_limit` for the parent process, MB. */ @@ -300,7 +298,7 @@ export interface WorkerSizing { totalMemoryMb: number; cpuCount: number; captureCostMultiplier: number; - /** true when the chosen count exceeds the advisory heap budget. */ + /** true when an explicit worker count exceeds the heap budget. */ exceedsHeapAdvisory: boolean; } @@ -405,6 +403,14 @@ export function computeWorkerSizing( } } + // Apply after the two-worker parallel floor and CPU contention cap so a + // small parent heap can select one worker even for a long render. Chrome's + // RSS budget above does not account for buffers retained by the parent. + if (finalWorkers > heapBasedWorkers) { + finalWorkers = heapBasedWorkers; + boundBy = "heap"; + } + return finish(finalWorkers, boundBy, effectiveMaxWorkers); } diff --git a/packages/producer/src/services/render/captureCost.ts b/packages/producer/src/services/render/captureCost.ts index db00538d92..085d2ceac4 100644 --- a/packages/producer/src/services/render/captureCost.ts +++ b/packages/producer/src/services/render/captureCost.ts @@ -116,11 +116,8 @@ function combineCaptureCostEstimates( * - Auto-sized renders only (`requestedWorkers === undefined`) — the field * failure was auto sizing, and an explicit `--workers N` is the operator's * own call. - * - Not enforced as a cap yet — the per-worker budget constant is derived - * from one field report; the `workers_heap_*` telemetry emitted with the - * sizing decides whether to enforce (see the TODO on HEAP_PER_WORKER_MB in - * @hyperframes/engine's parallelCoordinator). The message gives the - * operator the actionable knobs today. + * - Auto-sizing now applies the heap cap. Keep this defensive warning for + * sizing values supplied by older callers or a future alternate policy. * * Pure so the message shape + firing condition are unit-testable with a * synthetic `WorkerSizing` (the real one depends on the host's heap). From df2058690e552f214fb8ec1ed1418e8a9397411f Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 9 Sep 2026 22:52:45 -0700 Subject: [PATCH 2/2] docs(engine): reconcile heap-cap comments with enforced sizing Review feedback on #3803. The HEAP_PER_WORKER_MB comment still read "validate this estimate before rollout" while the sizing path now enforces the cap, so the next reader would see pending validation next to live enforcement. State that the cap is enforced, that an explicit `--workers N` bypasses it, and record the two fleet-wide thresholds the constants imply: a default ~4GB heap selects 4 workers for every auto-sized render regardless of core count, and a heap_size_limit under ~1664MB selects 1 worker, overriding the two-worker parallel floor. PRINFRA-341 remains the pre-merge validation gate. Also correct the buildHeapAdvisoryWarning doc block. It claimed the warning stayed live as a defensive path, but its `requestedWorkers !== undefined` guard and the newly enforced cap are mutually exclusive: the auto path can no longer exceed heapBasedWorkers and the explicit path is guarded out, so the warning is unreachable for every real render. Keep it, because reverting the cap to advisory is PRINFRA-341's open question, and note that the exceedsHeapAdvisory flag itself stays live through CLI telemetry. Comments only; no behavior change. Co-Authored-By: Claude Opus 5 (1M context) --- .../engine/src/services/parallelCoordinator.ts | 14 ++++++++++++-- .../producer/src/services/render/captureCost.ts | 16 ++++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/packages/engine/src/services/parallelCoordinator.ts b/packages/engine/src/services/parallelCoordinator.ts index 1ddc1f1f6a..55a9e95edf 100644 --- a/packages/engine/src/services/parallelCoordinator.ts +++ b/packages/engine/src/services/parallelCoordinator.ts @@ -141,8 +141,18 @@ const MEMORY_PER_WORKER_MB = 1536; const HEAP_RESERVED_MB = 1024; // Parent-process V8 heap consumed per worker (protocol buffers + in-flight // frame buffers). Derived from the field OOM: 6 workers exhausted a ~4GB -// default heap ⇒ >~500MB/worker + base. Validate this estimate against the -// workers_heap_* fleet telemetry before rollout (PRINFRA-341). +// default heap ⇒ >~500MB/worker + base. ENFORCED as a cap on auto-sizing +// below; an explicit `--workers N` bypasses it and stays authoritative. +// +// This and HEAP_RESERVED_MB come from a single field report, so know what +// they cost fleet-wide before trusting or changing them: +// - a default ~4GB Node heap selects 4 workers for EVERY auto-sized render +// regardless of core count — a 32-core host the contention path would +// size to 10 also lands at 4; +// - any host whose heap_size_limit is under ~1664MB selects 1 worker, which +// overrides the two-worker parallel floor and serializes long renders. +// PRINFRA-341 validates both figures against workers_heap_* fleet telemetry +// before this leaves draft; replace this note with what validated them. const HEAP_PER_WORKER_MB = 640; const MIN_WORKERS = 1; const MAX_WORKER_DIAGNOSTIC_LINES = 8; diff --git a/packages/producer/src/services/render/captureCost.ts b/packages/producer/src/services/render/captureCost.ts index 085d2ceac4..4cb8dc9a54 100644 --- a/packages/producer/src/services/render/captureCost.ts +++ b/packages/producer/src/services/render/captureCost.ts @@ -116,8 +116,20 @@ function combineCaptureCostEstimates( * - Auto-sized renders only (`requestedWorkers === undefined`) — the field * failure was auto sizing, and an explicit `--workers N` is the operator's * own call. - * - Auto-sizing now applies the heap cap. Keep this defensive warning for - * sizing values supplied by older callers or a future alternate policy. + * - Only when the chosen count exceeds the heap budget. + * + * Those two conditions are now mutually exclusive, so this returns undefined + * for every real render: auto-sizing enforces the heap cap, so the auto path + * can no longer exceed `heapBasedWorkers`, and the explicit `--workers N` + * path — the only one that still can — is guarded out by the first condition. + * + * Kept rather than deleted because enforcement is PRINFRA-341's open + * question: if fleet telemetry rejects the 640MB/worker + 1024MB reserve + * figures and the cap reverts to advisory, this is the live path again. + * Delete it once enforcement ships validated. The `exceedsHeapAdvisory` flag + * it reads stays live either way — CLI telemetry reports it as + * `workersExceedHeapAdvisory`, and it is still true for an over-budget + * explicit `--workers N`. * * Pure so the message shape + firing condition are unit-testable with a * synthetic `WorkerSizing` (the real one depends on the host's heap).