Repository navigation
fix(engine): size render workers from the container CPU limit - #6066
miguel-heygen wants to merge 2 commits into
Conversation
Worker count, video-extraction slots and the shader thread pool read the host core count, so a container limited to 6 cores on a 32-vCPU host sized itself for 32. Read the cgroup CPU quota (v2 cpu.max, v1 cfs quota/period) next to the existing cgroup memory probe and use the smaller of it and availableParallelism().
Edit accuracy: accurate 271 (base branch 271), smooth 177 of thoseThe gate passes. Quarantined, measured but not gated (0) |
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 94197df6 (one commit on main 2ef60f3f).
What holds
- The fix works in a real cgroup v2 container. I ran
getUsableCpuCount()from this head on our pod (cgroup2fs,cpu.max=400000 100000, 32 host vCPUs).getCgroupCpuLimit()4,getUsableCpuCount()4,os.cpus().length32. Before this PR,computeWorkerSizingand the extraction slots would have sized for 32 here. That covers the cgroup v2 path the test plan left unchecked. - Taking the min with
availableParallelism()is the right shape. The container-rootcpu.maxread misses a container without a private cgroup namespace. On v2, libuv already resolves the process's own cgroup foravailableParallelism(), so the cap still applies there. On v1, the explicit quota read covers what libuv misses. - The parser fails safe.
max,-1, malformed values and a zero period all return null, which falls back to the host count. The result is floored and at least 1, the probe is cached, and the notice goes to stderr only. - Locally:
systemCpu.test.tsandparallelCoordinator.test.tspass, 66/66.videoFrameExtractor.concurrency.test.tsneeds an ffmpeg with zscale, which my box lacks; CI covers it.
Nits (non-blocking)
- The header comment is only half right.
systemCpu.ts:1says "os.cpus()andavailableParallelism()ignore a cgroup CPU quota". On cgroup v2, Node 22's libuv 1.51 does honourcpu.max: on the pod above,availableParallelism()is 4 whilecpus()is 32. The v1 measurement in the PR body is right. Suggested wording: "os.cpus()ignores a cgroup CPU quota, andavailableParallelism()ignores a cgroup v1 one." - One sizing caller is left on the host count.
samplingBrowserCountinpackages/cli/src/utils/checkBrowser.ts:350still defaultscorestoos.availableParallelism(), so on a v1 host with a 1-core quotacheckstill starts 2 sampling Chromes. It's capped at 2, so the impact is small, but it's the one CPU-sizing default this PR doesn't route through the helper.handler.ts:605's Lambda cap is also onavailableParallelism(). I haven't checked whether Lambda exposes a CPU quota, so I can't say whether that matters for the cost measurement. - The notice can fire before any render.
EXTRACTION_SLOTSis computed whenvideoFrameExtractor.ts:882is imported, so in a container the stderr notice prints whenever the engine is imported, even with no render (e.g.check). It's harmless for--json, since it goes to stderr.systemMemory.tsonly warns when it's called.
CI: green at 94197df6, with all 8 required checks passing (Build, Typecheck, Test, Test: runtime contract, Tests on windows-latest, Render on windows-latest, Semantic PR title, regression) and Comments green too. Tests on windows-latest: engine-cli first failed: two describeProject.test.ts "skill query one-liners" tests timed out at 46.7s and 27.3s against a 20s limit. They spawn bash one-liners and never reach getUsableCpuCount, which returns null right away off Linux. The re-run passed.
Verdict: APPROVE
Reasoning: Every worker-sizing caller this PR names now reads one cgroup-aware count, and I confirmed it on a real cgroup v2 container. Hosts without a quota or CPU affinity keep their old count.
— Via
What
Render sizing now uses the cores the process may actually use. Worker count, video-extraction slots and the shader-transition thread pool read the host's core count (
os.cpus().length); they now readgetUsableCpuCount(): the host'savailableParallelism()capped by the cgroup CPU quota (v2cpu.max, v1cpu.cfs_quota_us/cpu.cfs_period_us), floored, at least 1. The CLI's "auto workers (N cores detected)" line reports the same number.Why
A container limited to 6 cores on a 32-vCPU host sized itself for 32. The same container image picked a different worker count depending on which host it landed on: for a 450-frame render, 3 workers on an 8-vCPU host and up to 6 on a 16+ vCPU host (4 where the default V8 heap binds first), with the same 6-core limit. Node does not cover this:
availableParallelism()honours a cpuset but not a CFS quota (Node 22.23.4 / libuv 1.51 in a--cpus 2container on an 8-core host returns 8).Related work
Refs #1373 (the same fix for memory:
getSystemTotalMbalready respects the cgroup memory limit). The newsystemCpu.tsfollowssystemMemory.ts: same container-root paths, cached probe, one stderr notice, no reads off Linux.How
packages/engine/src/services/systemCpu.ts:parseCgroupCpuLimit,getCgroupCpuLimit,getUsableCpuCount, exported from the engine root and as the@hyperframes/engine/system-cpusubpath (package-subpaths.json, thensync:package-subpaths).parallelCoordinator.ts(auto max,computeWorkerSizing,getSystemResources),videoFrameExtractor.ts(extraction slots), producershaderTransitionWorkerPool.ts(thread clamp), CLIpresent.ts. Hardware telemetry (perfSummary, CLI telemetry) still reports the host.floor(6 / 2.5)), on any host. Explicit--workers/PRODUCER_MAX_WORKERSare unchanged. Machines with no quota (laptops, plain VMs) are unchanged.Test plan
systemCpu.test.ts(11: v2/v1 parsing, unlimited and malformed values, floor and minimum, larger-than-host quota, cache, stderr-only notice, unreadable file, non-Linux).parallelCoordinator.test.tsadds the repro (32-vCPU host, 6-core limit, 450 frames:cpuCount6, 2 workers, bound by contention). Before the fix it failed withexpected 32 to be 6. The heap test andvideoFrameExtractor.concurrency.test.tsnow fake the CPU count through the helper instead ofos.cpus.os.cpus()turns 2 parallelCoordinator tests red; the helper ignoring the quota turns 4 systemCpu tests red; the extractor test given 8 CPUs instead of 2 turns 4 of its tests red.dist/services/systemCpu.jsinnode:22.23.4-bookworm-slimon an 8-core cgroup v1 host:--cpus 2gives 2;--cpus 4 --cpuset-cpus 0-2gives 3.tsc --noEmitin engine, producer and cli; oxlint, oxfmt,check:package-subpaths, comment checks: all clean.