Repository navigation
fix(engine): auto worker count stays within the heap budget - #5149
Conversation
…n feed A 24 GB Mac exporting a 43 s portrait project with 22 videos picked 5 capture workers against a 4192 MB heap that feeds about 4, and the render died at 62% with JavaScript heap out of memory. The heap budget was only advisory; it now caps the automatic count (bound 'heap'). An explicit --workers N is untouched.
Edit accuracy: accurate 2059 (base branch 2059), smooth 1602 of thoseThe gate passes. Quarantined, measured but not gated (0) |
… fails Review findings on #5149: the os and v8 mocks were restored only if every assertion passed, and two comments still called the heap cap advisory.
jrusso1020
left a comment
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at ccd56ae3.
The cap is the right shape. It's one clamp after the contention cap, applied to auto sizing only. The explicit path returns earlier with "explicit", so --workers N is untouched, as the body says. heapBasedWorkers is floored at 1, so the clamp can't produce 0 workers. exceedsHeapAdvisory can now only be true for an explicit request.
Ran:
parallelCoordinator.test.ts: 58 of 58.- With the four-line clamp removed, "never auto-picks more workers than the V8 heap can feed" fails (57 of 58).
Non-blocking, worth knowing before it lands. The cap isn't specific to Electron. It applies to every auto-sized render on any Node whose heap is about 4 GB, which is stock Node's default. On my box, Node 22 reports heap_size_limit = 4144 MB, which gives (4144 − 1024) / 640 = 4 workers.
- CLI users:
defaultSafeMaxWorkersallows up to 16, so anyone on a large machine withoutNODE_OPTIONSwho auto-picked 5–16 workers will now run 4. - Cloud producer:
producer-internal'sDockerfile.proddoesn't set a heap size either, so its auto-sized renders take the same cap. I couldn't check how the deploy configures it.
The comment this PR removes warned about exactly this ("enforcing a guessed budget could silently cut worker counts fleet-wide", PRINFRA-341). The 640 MB per worker comes from one field report and doesn't scale with resolution. Capping is the safe direction for an OOM, so I'm not blocking on it. Before or right after merge, it would be good to either:
- check the existing
workersBoundBy/workers_heap_*render telemetry for how often the cloud and CLI auto-picked more than 4, or - give the cloud producer a larger
--max-old-space-sizeso it keeps its parallelism.
A system Node honors that flag, as your Electron measurement showed.
Nit: buildHeapAdvisoryWarning and its test now cover a branch that auto sizing can't reach, as the Notes say. Fine as a follow-up.
— Rames
Inside Electron the heap is a hard 4,192 MB cage, and that is where the field OOM behind the old figure happened (#5149: 5 auto workers on a heavy export). The new 320 MB figure is measured only in a system Node, so the desktop app keeps 640 MB (4 workers there) until a heavy desktop export is measured. Containers and plain Node keep 320.
…r's CPU quota (heygen-com#6641) * perf(cli): give render the heap its auto workers need in small containers Auto worker sizing caps capture workers at (V8 heap - 1024 MB) / 640 MB. Node sizes its default heap from visible memory, so in a 10 GB container the heap is 2,096 MB and every render captures with one worker, whatever the core count. When no --workers is given, no --docker, no heap was set by hand, and the heap is the only bound holding workers back, render relaunches itself once with --max-old-space-size set to what CPU and memory allow (capped at half of memory). The engine exposes heapMbForAutoWorkers() for this. * fix(cli): count a heap-relaunched render once, keep detached renders alive The relaunched child skips the CLI's command telemetry, update and skills notices, and app-history notice; the parent already records and prints them. A detached render no longer gets SIGHUP forwarded, which its default handler would treat as a kill. A heap set as --max_old_space_size counts as set by hand. * fix(engine): keep the desktop app's per-worker heap figure at 640 MB Inside Electron the heap is a hard 4,192 MB cage, and that is where the field OOM behind the old figure happened (heygen-com#5149: 5 auto workers on a heavy export). The new 320 MB figure is measured only in a system Node, so the desktop app keeps 640 MB (4 workers there) until a heavy desktop export is measured. Containers and plain Node keep 320. * fix(engine): know the packaged desktop app by its client tag, which runs the CLI on plain Node
What
Automatic capture-worker sizing now never exceeds what the render process's V8 heap can feed. A render that asks for
--workers Nis untouched.Why
A desktop export on a 24 GB Mac (43 s, 1080x1920, 22 videos, 67 audio clips, 41 images) auto-picked 5 workers. The producer's own warning said the 4192 MB heap supports about 4, and the render died at 62% with
JavaScript heap out of memory. The heap budget (heapBasedWorkers) was computed and reported but never enforced.The ceiling is real on every Mac running the desktop app: the app runs the CLI inside Electron's Node, whose V8 heap is a hard 4192 MB because of pointer compression (a 4 GB cage). Measured with Electron 44.2.0 in run-as-node mode:
NODE_OPTIONS=--max-old-space-size=2048is honored (2144 MB), but 6144 and 8192 are ignored and the limit stays 4192 MB;v8.setFlagsFromStringdoes the same. A system Node honors 8192 (8240 MB). So raising the heap from the app is not possible, and the worker count has to respect the budget.How
computeWorkerSizingcaps the final auto count atheapBasedWorkersand reports the bound asheap.exceedsHeapAdvisorycan now only be true for an explicit request.Verified
heap; an explicit 6 stays 6; an 8192 MB heap leaves the count at 5.parallelCoordinator.test.ts. With the cap removed the new test fails (red seen on that box).Notes
buildHeapAdvisoryWarningin captureCost.ts can no longer fire for an auto-sized render; left in place for a follow-up.Size
Under the 100-line floor on purpose: a lone urgent fix (desktop export heap OOM, P0) that no other open PR can carry.