From fd3fce99558010dba03494e8d67dabfa6df924a3 Mon Sep 17 00:00:00 2001 From: James Date: Mon, 18 May 2026 20:37:25 +0000 Subject: [PATCH] refactor(producer): tighten resolveChunkPlan assertion + trim comments Address self-review findings: - assertPositiveInteger now only runs on the caller-supplied path so the error message names `configChunkSize` only when the caller actually passed one. Previously, the assertion fired against `resolvedChunkSize` on both paths and would have lied about the offending input. - Drop the call-site comment that narrated the diff/history; the function docstring already covers the contract. - Drop the internal-track name and date from the MIN_CHUNK_SIZE rationale and the test block header. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/services/distributed/plan.test.ts | 9 +++----- .../producer/src/services/distributed/plan.ts | 23 +++++++++---------- 2 files changed, 14 insertions(+), 18 deletions(-) diff --git a/packages/producer/src/services/distributed/plan.test.ts b/packages/producer/src/services/distributed/plan.test.ts index db0baa98d..966f34b9a 100644 --- a/packages/producer/src/services/distributed/plan.test.ts +++ b/packages/producer/src/services/distributed/plan.test.ts @@ -96,12 +96,9 @@ describe("resolveChunkPlan", () => { }); // ── Auto-size when configChunkSize is undefined ─────────────────────── - // Pre-fix, `plan()` defaulted `chunkSize` to 240 on a `?? DEFAULT_CHUNK_SIZE` - // line, so a 660-frame composition with `maxParallelChunks=16` ended up at - // 3 chunks (ceil(660/240)) regardless of the caller's fan-out intent. - // Surfaced by the lever-1 chunk-scaling benchmark on 2026-05-17. The - // auto-sizer now picks `max(MIN_CHUNK_SIZE, ceil(totalFrames / - // maxParallelChunks))` whenever the caller leaves `chunkSize` undefined. + // The auto-sizer picks `max(MIN_CHUNK_SIZE, ceil(totalFrames / + // maxParallelChunks))` whenever the caller leaves `chunkSize` undefined, + // honoring `maxParallelChunks` instead of clamping at a 240-frame default. it("explicit chunkSize wins: 660 frames + chunkSize=240 + maxParallelChunks=16 → 3 chunks", () => { // Regression guard for the "explicit number still works" half of the diff --git a/packages/producer/src/services/distributed/plan.ts b/packages/producer/src/services/distributed/plan.ts index 2fde350e0..e8cbcea62 100644 --- a/packages/producer/src/services/distributed/plan.ts +++ b/packages/producer/src/services/distributed/plan.ts @@ -198,9 +198,8 @@ export const DEFAULT_MAX_PARALLEL_CHUNKS = 16; /** * Floor for the auto-sized `chunkSize` when the caller leaves it * `undefined`. Anything smaller hits a per-chunk fixed-overhead wall - * (worker boot + plan download + ffmpeg init) that outweighs the - * parallelism gain, per the lever-1 chunk-scaling benchmark on - * 2026-05-17. + * (worker boot + plan download + planHash recompute + ffmpeg init) that + * outweighs the parallelism gain on tiny renders. */ export const MIN_CHUNK_SIZE = 10; /** @@ -379,11 +378,16 @@ export function resolveChunkPlan( // silently truncate. assertPositiveInteger("totalFrames", totalFrames); assertPositiveInteger("maxParallelChunks", maxParallelChunks); + // Validate the caller-supplied value with its real name so the error + // message points at the actual bad input. The auto-sized branch is + // provably a positive integer (totalFrames and maxParallelChunks are + // already validated above, MIN_CHUNK_SIZE is a positive integer + // constant), so it doesn't need re-checking. + if (configChunkSize !== undefined) { + assertPositiveInteger("configChunkSize", configChunkSize); + } const resolvedChunkSize = - configChunkSize !== undefined - ? configChunkSize - : Math.max(MIN_CHUNK_SIZE, Math.ceil(totalFrames / maxParallelChunks)); - assertPositiveInteger("configChunkSize", resolvedChunkSize); + configChunkSize ?? Math.max(MIN_CHUNK_SIZE, Math.ceil(totalFrames / maxParallelChunks)); const naiveCount = Math.ceil(totalFrames / resolvedChunkSize); const chunkCount = Math.min(maxParallelChunks, Math.max(1, naiveCount)); const effectiveChunkSize = Math.max(resolvedChunkSize, Math.ceil(totalFrames / chunkCount)); @@ -779,11 +783,6 @@ export async function plan( } // ── Chunking decisions + locked config ── - // Pass `config.chunkSize` through verbatim — `resolveChunkPlan` handles - // the `undefined` case by auto-sizing from `maxParallelChunks`, so a - // caller that bumps `maxParallelChunks` to 16 without setting - // `chunkSize` actually gets 16 chunks instead of silently clamping at - // the old 240-frame default. const maxParallel = config.maxParallelChunks ?? DEFAULT_MAX_PARALLEL_CHUNKS; const { chunkCount, effectiveChunkSize } = resolveChunkPlan( totalFrames,