fix(producer): attribute and stop oversized plans early - #2773
Conversation
96355ab to
86b6fa0
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 86b6fa0970ca15e4129fdfce39eda4f6e6d143d4.
Solid attribution work. The two early aborts land at exactly the right seams — after the compiled tree stabilizes (pre-extract, on compiledDir with rootKind: "compiled") and after artifact promotion / stale-metadata scrub (pre-freeze, on planDir) — so a 12 GiB composition now dies in seconds instead of after multi-GiB extraction and hashing. The PlanTooLargeError message prefix is preserved verbatim, error.name/error.code are unchanged, and the breakdown labels are either fixed strings or one-way SHA-256 truncations so log/error lines never carry authored paths.
Verified the load-bearing bits directly:
FREEZE_OWNED_PLAN_FILEScompleteness.freezePlan(packages/producer/src/services/render/stages/freezePlan.ts:325-369) writes exactlymeta/composition.json,meta/encoder.json,meta/chunks.json, and top-levelplan.json. The four entries in the new list match one-to-one, so the reused-planDir path is airtight.- Symlink handling.
walkRegularFilesusesDirent.isDirectory()/isFile(), which returnfalsefor symlinks even when they point at directories/files; the defensivelstatSync().isFile()inrecordFileis belt-and-suspenders on top. The new symlink test covers both file-link and dir-link cases on Linux. - Path collision. The
compiled/assets/video-frames/customer/...test proves an authored path segment namedvideo-framesundercompiled/stays classified as compiled/source-media rather than as extracted frames — good defensive test. sourceMediaBytesas sub-bucket ofcompiledBytes. Interface doc + test math match —compiled + video-frames + audio + metadata + other = total, andsourceMediaBytes ⊆ compiledBytes. Nice call-out in the doc.
Concerns (non-blocking)
"pre-freeze"observedAtstring has no test coverage.plan() early size budgetassertserror.observedAt === "pre-extract"and the existing PLAN_TOO_LARGE test asserts"post-freeze", but the middle checkpoint isn't pinned anywhere. If a future refactor drops or renames the pre-freeze call, no test would fail. Cheap fix: extendplan() reused directory size checkto also cover the "reused planDir with 100k of stale files + limit that trips pre-freeze but not pre-extract" case, and assertobservedAt === "pre-freeze".- Symlink test silently degrades on Windows without Developer Mode. The
symlinkSynccalls are wrapped intry {} catch {}per the comment ("Windows without Developer Mode may reject symlink creation"). If Windows CI runs without symlink perms, the test still passes because there are no extra files to trip the count — but you've lost the actual symlink-skipping assertion. Considerexpect.assertions(N)or asymlinkSupportedcapability probe that skips the test rather than degrading it, so a real regression can't hide behind a Windows fallthrough.
Nit
videoFrameFileCountcounts extracted files, not necessarily individual frames — a video extracted as a single.mp4counts as 1, a per-frame.jpgsequence counts as N. The current naming reads like a frame count. Not worth churning code for; noting in case the field ends up in a dashboard where the delta would matter.
Question (pre-existing, not this PR)
PlanTooLargeErrorconstructor setsthis.name = "PlanTooLargeError", but the AWS CDK / SAMNON_RETRYABLE_PLANlist matches on the code string"PLAN_TOO_LARGE"(HyperframesRenderStack.ts:196-205,template.yaml:241). I couldn't find a name-normalization layer inpackages/aws-lambda/src/handler.tsthat rewrites"PlanTooLargeError"→"PLAN_TOO_LARGE"before the error surfaces to Step Functions —handlePlanand the outercatchjust re-throw. GCP's classifier atpackages/gcp-cloud-run/src/server.ts:586,590registers both"PlanTooLargeError"AND"PLAN_TOO_LARGE", which is the dual-naming pattern used forPlanV2IntegrityErrorin the #2788/#2789/#2790 arc. Is the AWS CDK list intentionally code-only for the legacy errors, or should"PlanTooLargeError"(and"FormatNotSupportedInDistributedError","FontFetchError") also be added so the retry classifier terminates correctly on the class-name surface? This is orthogonal to #2773 — the PR only changes when the error is thrown, not the classifier — but the "Non-retryable: the same planDir would trip the cap on every retry" comment inplan.tspresumes SF classifies it terminal. Would appreciate a pointer to whatever mechanism I'm missing, or confirmation this is a separate cleanup.
What I didn't verify
- End-to-end run of the pre-extract abort against a real oversized composition on Lambda (only read the code + tests + CI status).
- Whether any downstream telemetry consumer parses the error message and would trip on the new
Observed at .../Breakdown: ...suffix strings.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 86b6fa0970ca15e4129fdfce39eda4f6e6d143d4.
Verdict
APPROVE
Overview
Tight, well-scoped fail-fast improvement. The two new abort points sit on the correct seams — after runProbeStage returns (compiled tree is stable, compiledDir measured with rootKind: "compiled") and after artifact promotion + freeze-owned scrub (planDir measured with rootKind: "plan") — and both route through the same assertPlanSizeWithinLimit helper that also powers the retained post-freeze check. Single source of truth, no duplicated size math. The PlanTooLargeError contract is preserved additively: code, name, sizeBytes, limitBytes, and the message prefix [plan] planDir size X exceeds the configured ceiling Y (PLAN_TOO_LARGE). are unchanged; breakdown and observedAt are new optional fields; every downstream consumer keys off .code or the class name (packages/aws-lambda/src/handler.ts:150, packages/gcp-cloud-run/src/server.ts:183, packages/aws-lambda/src/cdk/HyperframesRenderStack.ts:197-198), so the suffix additions are safe. The dropped statSync import isolates the walker to lstatSync throughout, making symlink-skip an explicit invariant rather than a Dirent-side effect.
Peer coverage: james-russo-rames-d-jusso already left a thorough COMMENTED review confirming FREEZE_OWNED_PLAN_FILES completeness against freezePlan (packages/producer/src/services/render/stages/freezePlan.ts:325-371), symlink handling, the path-collision defensive test, and the sourceMediaBytes ⊆ compiledBytes sub-bucket accounting. Verified independently and agree on all four. His two non-blocking concerns (missing pre-freeze observedAt assertion; Windows symlink test degrading silently on missing perms) are cheap follow-ups worth folding in but don't gate merge. His cross-classifier question is resolved: HyperframesRenderStack.ts:196-205 already lists BOTH "PLAN_TOO_LARGE" and "PlanTooLargeError" in NON_RETRYABLE_PLAN, and GCP's classifier does the same at packages/gcp-cloud-run/src/server.ts:904,910 — the retry-terminal path is intact.
Findings below are independent P3 nits complementary to Rames' set; nothing blocking.
Findings
P3 — Hardcoded plan-file literals duplicate authoritative constants elsewhere
File: packages/producer/src/services/distributed/planSize.ts:53-58, packages/producer/src/services/distributed/plan.ts:587-592
planSize.ts's PLAN_ROOT_CATEGORY hardcodes "audio.aac", "plan.json", "meta", and "compiled" as literal string keys, and plan.ts's FREEZE_OWNED_PLAN_FILES hardcodes "plan.json", "meta/composition.json", "meta/encoder.json", "meta/chunks.json". The authoritative sources for two of these already exist: PLAN_AUDIO_RELATIVE_PATH = "audio.aac" in packages/producer/src/services/distributed/shared.ts:39 and freezePlan's own writes at packages/producer/src/services/render/stages/freezePlan.ts:328-371. If either producer file is ever renamed (e.g. audio.aac → audio.opus for a webm-native audio path), the classifier silently dumps the audio into the other bucket and the freeze-owned scrub misses the new metadata file. Failure scenario: a future contributor changes PLAN_AUDIO_RELATIVE_PATH to "audio.opus" in shared.ts, tests pass because integration coverage doesn't assert on audioBytes attribution, and error breakdowns for oversized plans now show audio=0 B, other=<large> — muddying the diagnostic signal this PR exists to provide. Cheap fix: import PLAN_AUDIO_RELATIVE_PATH and PLAN_VIDEOS_META_RELATIVE_PATH from shared.ts; consider extracting a FREEZE_PLAN_ARTIFACTS constant exported from freezePlan.ts and consumed by both plan.ts and planSize.ts so drift stays impossible.
P3 — 12-hex-char (48-bit) hash truncation risks collisions on large multi-video compositions
File: packages/producer/src/services/distributed/planSize.ts:60-62,71-74
hashComponent returns sha256(value).slice(0, 12) — 48 bits of entropy per label. Birthday-collision probability crosses ~1% at roughly 240k distinct video keys and ~50% at ~2M, so single-composition risk is effectively zero. But topComponents is deterministically sorted and bounded at 10, meaning a collision silently merges two authored video directories into one bucket with a summed sizeBytes and no signal that the merge happened. Failure scenario: a diagnostic dashboard aggregates topComponents across runs to spot bloat trends; a collision on a heavy composition causes two videos to appear as one, shifting the sizeBytes-per-video statistic upward and hiding the real per-video regression. Given the label is already opaque (customers can't decode the hash), extending to 16 hex chars (64 bits) removes the concern entirely at no readability cost.
P3 — Test coverage for reused-planDir scrub is partial
File: packages/producer/src/services/distributed/planSizeCap.test.ts:381-405 (plan() reused directory size check)
The reused-planDir test seeds only plan.json (100 KB) and meta/encoder.json (100 KB); the assertion that the plan succeeds under a 4 KB cap only proves those two files were scrubbed. FREEZE_OWNED_PLAN_FILES has four entries — plan.json, meta/composition.json, meta/encoder.json, meta/chunks.json — and a regression that dropped either composition.json or chunks.json from the list would still pass this test. Failure scenario: a refactor rewrites the constant as ["plan.json", "meta/encoder.json"] and the test stays green; a real reused-planDir case with stale meta/composition.json from a prior run then trips pre-freeze with a false-positive PLAN_TOO_LARGE. Seed all four freeze-owned files in the fixture and assert none survive after plan() returns. Rames' suggestion to also pin observedAt === "pre-freeze" on a companion test lands in the same slot — combine.
P3 — Hash stability across runs is untested
File: packages/producer/src/services/distributed/planSizeCap.test.ts:80-108
The breakdown test asserts labels.some((label) => label.startsWith("video-frames:")) and that no plaintext directory name leaks, but never asserts the hashed suffix is stable — the same input on two runs should produce the exact same 12-char suffix. Given createHash("sha256") is deterministic this is guaranteed by the runtime, so this is documentation-through-tests rather than a real gap, but pinning the exact hash for a known input (e.g. expect(label).toBe("video-frames:<fixed-12-char-suffix>")) protects against a future refactor that swaps hash algorithms or takes a longer/shorter slice without updating the interface. Skippable if you consider the truncation length locked; add if you consider it a public-ish contract.
P3 (observation, not a fix ask) — "candidate sidecar / canary lane" language vs unconditional code
File: packages/producer/src/services/distributed/plan.ts:973-979,1131-1137
The PR description frames this as "intended for the candidate sidecar and canary lane before broader rollout," but both new assertPlanSizeWithinLimit calls run for every plan() invocation with no config gate, feature flag, or environment check. Not flagging as a defect — the new pre-extract and pre-freeze branches only throw when the plan already exceeds the cap (the post-freeze check would have thrown for exactly the same plans, later, at higher cost), and under-limit plans see only the additional fs walk overhead. So the behavior change is a strict superset improvement over main on the failure path, and non-observable on the success path modulo latency. Noting so the PR description doesn't imply a rollout gate that isn't there — either drop the "canary lane" framing, or add a config knob (e.g. config.strictSizeCheckPhases?: Array<"pre-extract" | "pre-freeze" | "post-freeze">) if a real rollout-safety valve is intended.
What I didn't verify
- Real Lambda run of the pre-extract abort on an oversized composition (only code + tests + CI).
- Whether any log-shipping pipeline extracts fields from
PlanTooLargeError.messagevia regex (all in-repo consumers key off.code/ class name; external log parsers weren't in scope). - The in-progress
regression-shardson windows-latest at review time — Producer unit + integration tests are green, and the Windows symlink test'stry/catchdegrades to a size-only assertion if perms are missing (Rames' concern).
— Review by Via
miguel-heygen
left a comment
There was a problem hiding this comment.
Additive exact-head review of 86b6fa0970ca15e4129fdfce39eda4f6e6d143d4: the new pre-extract check measures a reused .plan-work/compiled tree, but plan() creates that directory without clearing it before the next cpSync/compile overlay (packages/producer/src/services/distributed/plan.ts:841-865). A prior failed attempt can therefore leave a large compiled asset that the later checkpoint measures at plan.ts:970-979, terminally rejecting a new small retry as PLAN_TOO_LARGE. writeCompiledArtifacts also overlays the same tree (packages/producer/src/services/render/shared.ts:146-175), while the reuse regression covers only freeze-owned metadata (packages/producer/src/services/distributed/planSizeCap.test.ts:322-345).
Please start every attempt with a fresh compiled work tree (or clear it before seeding) and add a reused-planDir regression containing a stale large compiled asset. Rames and Via already covered the non-blocking metadata, symlink-capability, literal/hash, fixture, and rollout notes, so I am not repeating those.
Verdict: Request changes.
Reasoning: A failed attempt can contaminate a reused transient tree and permanently reject a later valid retry; exact-head regression shards are also still nonterminal.
— Magi
|
Addressed the stale-worktree blocker on exact head 2da54bc.
Local validation:
The full plan-size test file has 14 passing cases including the new regression; its unrelated duration-guard case hits the existing sandbox-only browser probe error |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at 2da54bc5cdf9a4dd9e1852ae2d8e73ded661467a — delta from 86b6fa0970 is 31 lines across plan.ts and planSizeCap.test.ts.
Clean, targeted fix. The prior code seeded workDir only when absent (if (!existsSync(workDir)) mkdirSync(...)), which is fine on a fresh planDir but leaked into the size accounting on any retry where a prior attempt failed before its end-of-plan .plan-work cleanup: cpSync(projectDir, compiledDir, { recursive: true, dereference: true }) and the subsequent compileStage both overlay their outputs — they don't tombstone extra files under compiled/. Stale bytes from the prior attempt would sit under compiled/ and add to the measurePlanDirBytes(planDir) sweep, tripping the freshly-added planDirSizeLimitBytes check on inputs that actually fit.
New sequencing at plan.ts:839-847 — rmSync(workDir, { recursive: true, force: true }) then mkdirSync(workDir, { recursive: true }) — is the right shape:
force: trueswallows ENOENT so the first-attempt no-work-dir case is a no-op, no exception path added.recursive: truewipes the whole subtree so.plan-work/compiled/,.plan-work/downloads/, and any future planner-owned scratch subdirs all clear in one call — no per-subdir enumeration to drift when a new stage adds another scratch tree.- The comment identifies the invariant explicitly ("planner-owned scratch space") — cross-verified via
git grep '.plan-work'that onlyplan.tsand its own tests reference the path, and onlyplan.tswrites into it. So the unconditional wipe is safe: no other code path expects the directory to persist acrossplan()calls. - Runs BEFORE
mkdirSync(compiledDir, ...)andcpSync(projectDir, compiledDir, ...), so the re-seeded local-asset copy lands into a clean tree.
The new test at planSizeCap.test.ts:322-347 is a good structural pin — 100 KB stale asset in .plan-work/compiled/, 4 KB cap, assert (a) plan succeeds (planHash matches the SHA-256 shape), (b) stale asset does NOT appear at <planDir>/compiled/stale-large-asset.bin in the final planDir, (c) final measurePlanDirBytes(planDir) < 4096. All three assertions cover a different failure mode of the same bug (return-code-corrupt / stale-asset-served / stale-bytes-tripping-cap). The describe("plan() reused directory size check", ...) grouping puts it right next to the existing "ignores stale freeze-owned metadata" test — same reused-directory taxonomy.
R1 pre-existing observations are unchanged and remain follow-ups (not this PR's problem):
PlanTooLargeError.name = "PlanTooLargeError"throws still land in AWS Step Functions matching against the code-onlyNON_RETRYABLE_PLANlist — legacy-error exception to the dual-naming convention, awaiting your call on whether AWS lists get a legacy-name backfill.- Windows symlink coverage in
FREEZE_OWNED_PLAN_FILESwalker degrades silently when the test runner lacks symlink perms.
LGTM from my side.
2da54bc to
ebb02ca
Compare
|
Final Graphite rebase onto current |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at ebb02cafe7e1d1e067bc37c00fd3613e20230ee5 — verified pure rebase on new main (5bfda4e08737…).
git diff <merge-base>..<head> | git patch-id --stable returns 982f477ff15886448fe566a06dcdb835ed079173 on BOTH the prior 2da54bc5c and the new ebb02cafe7 — so #2773's own contribution is byte-identical to R2. All R2 findings stand; nothing new to review at the code layer.
LGTM.
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at exact head ebb02cafe7. Verified byte-clean equivalence to prior fix head 2da54bc5c via blob-SHA match on all three PR-scoped files — the Graphite rebase between them is pure main catch-up.
Verdict
APPROVE — Miguel's stale-worktree blocker is cleanly addressed. Fix is scoped, correct, and the regression exercises the exact failure mode.
Miguel's blocker verification
Miguel's finding at 86b6fa0970: plan() creates .plan-work/compiled without clearing it, so a failed attempt leaves compiled assets that the next attempt's pre-extract size check measures, terminally rejecting a would-be-small retry as PLAN_TOO_LARGE.
Fix at ebb02cafe7:
plan.ts:841-847—.plan-workis now scrubbed recursively at the START of everyplan()attempt, BEFORE thecpSync/compileStage overlay begins:const workDir = join(planDir, ".plan-work"); // `.plan-work` is planner-owned scratch space. A prior attempt can fail // before the end-of-plan cleanup and leave compiled assets behind; cpSync // and compileStage both overlay their outputs, so reusing that directory // would let stale bytes contaminate the new plan and its size check. rmSync(workDir, { recursive: true, force: true }); mkdirSync(workDir, { recursive: true });
- Cleanup happens BEFORE the existing end-of-plan
rmSync(workDir, …)at line 1121, so both pre- and post-attempt paths are covered.
Regression at planSizeCap.test.ts:322-346 ("clears stale compiled scratch from a failed prior attempt"):
- Seeds
.plan-work/compiled/stale-large-asset.binat 100 KB in a reused planDir. - Invokes
plan()on a small project with a 4 KB cap. - Asserts (a)
planHashis a valid sha256 hex string (success), (b)join(planDir, "compiled", "stale-large-asset.bin")does NOT exist afterplan()returns (stale asset is gone), (c)measurePlanDirBytes(planDir) < 4_096(final size under cap).
Verdict per finding: ADDRESSED.
Standards lens re-run
Per the mechanical checklist ([[standing-standards-lens-checklist-mechanical]]) — grep every changed file at exact head ebb02cafe7:
Files changed by this PR: plan.ts, planSize.ts, planSizeCap.test.ts.
Bare as T (excluding as const, as unknown as, annotated as any):
plan.ts:- lines 124-125 — English comment prose (
extract as PNG,extract as JPG); FALSE POSITIVE. - line 646 —
const dataObj = data as Record<string, unknown>;— bare cast. PRE-EXISTING (outside the+6/-1delta of this PR; sits in unrelated JSON-parsing code aroundreadProducerVersion). Not introduced by this fix. Flagging for completeness; a follow-up narrow-with-inwould remove it.
- lines 124-125 — English comment prose (
planSize.ts: ZERO bare casts.planSizeCap.test.ts:- lines 204, 258, 312 —
caught as PlanTooLargeError. PRE-EXISTING (outside the+25delta of this fix; all in older test bodies). - lines 421-423 —
(caught as Error).messagethree times. PRE-EXISTING (line 419 doesexpect(caught).toBeInstanceOf(Error)at runtime but TS narrowing isn't captured, hence the cast). Not introduced by this fix.
- lines 204, 258, 312 —
Non-null assertions (!., 
What
PLAN_TOO_LARGEcontract/message prefixWhy
The existing ceiling only runs after a multi-GiB plan has been fully materialized and hashed. Production examples range from roughly 3 GiB to 12.6 GiB. This gives us enough attribution to design Plan v2 from real data while avoiding obviously wasted extraction and hashing work.
Compatibility and rollout
plan.jsonformat changesThis does not interrupt a single FFmpeg extraction once it is running. A canaried extraction-budget monitor is a separate follow-up.
Validation