fix(producer): type video extraction failures - #2776
Conversation
aa2b128 to
13e1704
Compare
5f05376 to
d80be86
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R1 review — hyperframes PR #2776
Head: 13e17046c698a03134d5e33afe68b3f0af01c78d
Title: fix(producer): type video extraction failures
Base: fix/atomic-video-download-retry (child of #2774)
Delta: +1009 / -59, 12 files (largest in the stack)
Verdict
APPROVE with 2 nits — the taxonomy + producer-safe error surface is well-designed, the primary lens (multi-shape throw discipline → wire discriminator) is honored on the JSON/SSE producer surface, and the entire new failure gate is default-off with a bounded 0|1 retry ceiling. The known wire-drift on the Step Functions + GCP Cloud Run boundaries is explicitly documented in the PR body as a rollout dependency ("companion transport mappings" gate the enforce lane), not a defect in this PR.
No P0. No P1. Two P2 nits below.
Adversarial pass — evidence walk
1. Multi-shape throw discipline (PRIMARY LENS)
Discriminator on the producer HTTP surface is .code, not .name. extractSafeRenderErrorCode in packages/producer/src/server.ts reads err.code, allowlists against a bounded Set of two literals, and never parses .message. The three throw shapes it must accept converge cleanly:
| Shape | Test coverage | Assertion form |
|---|---|---|
Class-instance throw (new VideoExtractionStageError(...)) |
server.errorCode.test.ts:7-16 |
.toBe("VIDEO_SOURCE_UNRENDERABLE") / .toBe("VIDEO_EXTRACTION_FAILED") |
Code-only bag throw ({code: "VIDEO_SOURCE_UNRENDERABLE"}) |
server.errorCode.test.ts:19-22 |
.toBe("VIDEO_SOURCE_UNRENDERABLE") |
| Message-embedded / name-alias / arbitrary code | server.errorCode.test.ts:25-30 |
.toBeUndefined() (rejected) |
- Assertions use the canonical string constant (
"VIDEO_SOURCE_UNRENDERABLE"), noterr.nameorerr.code— no trivial-pass smell. - Message-embedded probe (
new Error("failed [VIDEO_SOURCE_UNRENDERABLE; secret=/tmp/x]")) is explicitly rejected — the wire never parses message text, so a leaked signed-URL/path in a stringified error can never smuggle a synthetic code. - The one "shape" not tested is a name-alias throw (
new Error(); e.name = "VIDEO_SOURCE_UNRENDERABLE"). This is correct-by-design:extractSafeRenderErrorCodeinspects.codenot.name, so name-alias is not a valid discriminator on this wire.
VideoSourceExtractionError (engine layer) uses a duck-typed sentinel (hyperframesVideoSourceExtractionError: true) for cross-realm safe detection via isVideoSourceExtractionError — reasonable pattern; contained.
buildVideoExtractionStageError fails closed on legacy failure shapes with no .kind / no .retryable: kind defaults to "internal", retryable defaults to false (via .every((f) => f.retryable === true) — undefined coerces to non-retryable). extractVideosStage.test.ts:151-168 pins this.
2. Dispatch chain verification
Traced end-to-end for the two flow paths that reach a wire boundary:
A. Movio / standalone producer HTTP → JSON/SSE. extractVideosStage.ts sets failureToEnforce → renderOrchestrator.ts:2147 throws after checkpoint → server.ts catch handler calls extractSafeRenderErrorCode(error) → code field appears in SSE type: "error" payload and in the failure JSON. Sanitized: the code is the allowlisted literal, never the raw .message. This wire is CORRECT.
B. Distributed plan → AWS Lambda / GCP Cloud Run → orchestrator. plan.ts:951 throws VideoExtractionStageError. This propagates out of the Lambda / Cloud Run handler, whose retry discriminators are:
packages/aws-lambda/src/cdk/HyperframesRenderStack.ts:196NON_RETRYABLE_PLAN— matches on Step FunctionsErrorEquals, which isError.namepackages/aws-lambda/src/handler.ts:145normalizeTerminalErrorName— rewrites.name := .codefor a hand-curated list (PLAN_PROTOCOL_UNSUPPORTED,PLAN_TOO_LARGE,PLAN_V2_INTEGRITY_UNRECOVERABLE)packages/gcp-cloud-run/src/server.ts:944NON_RETRYABLE_ERROR_NAMES— dispatches onerr.name
None of these three lists include VideoExtractionStageError, VIDEO_SOURCE_UNRENDERABLE, or VIDEO_EXTRACTION_FAILED. So if enforce mode is enabled today, VideoExtractionStageError (name is literally "VideoExtractionStageError") will:
- Never be normalized in
normalizeTerminalErrorName - Never match
NON_RETRYABLE_PLAN→ Step Functions retries the plan step up to 4× with exponential backoff - Never match
NON_RETRYABLE_ERROR_NAMES→ Cloud Run returns 500 → Cloud Workflows retries
This is the exact wire-drift trap the primary lens is designed to catch — but the PR body pre-empts it verbatim:
The internal sidecar and Experiment Framework must treat both exhausted stage codes as workflow-terminal after the producer-local budget. Candidate enforcement must not be enabled until those companion mappings are deployed, or Temporal can multiply producer attempts.
The rollout section labels deploy internal + EF terminal transport mappings as step 4 (before enforce is turned on at step 5). Given the default is off, no throw actually reaches the wire in production behavior today — the drift is a rollout dependency, not a landed defect. Not a finding I would block on. See Nit A below for the peripheral suggestion.
3. NON_RETRYABLE classification of new codes
Not applicable — the class deliberately splits deterministic vs exhausted-transient into two distinct code values (VIDEO_SOURCE_UNRENDERABLE vs VIDEO_EXTRACTION_FAILED), each carrying an explicit .retryable boolean. The producer-local retry policy is respected in the engine (runVideoExtractionWithRetry throws when !classified.retryable || retries >= budget). Correct.
4. observability.ts +2 lines
Adds transientRetries?: number to RenderExtractionObservability. Matches: (a) phaseBreakdown.transientRetries accumulated in extractAllVideoFrames and runExtractVideosStage (HDR probe retries), (b) piped through in renderOrchestrator.ts:2150 and summarizeExtractionObservability at line 234, (c) ?? null at the final observability emission. The label is derived directly from the actual retry-count call sites — no observability-label-vs-actual-path drift.
5. server.ts +21 lines
Two-site: (a) extractSafeRenderErrorCode implementation + SAFE_RENDER_ERROR_CODES allowlist, (b) two catch handlers append errorCode to the SSE write*Failure and to the JSON error body. Uses the allowlisted constant not err.name. Reads only from .code — never parses message text. Explicit test at server.errorCode.test.ts:25-30 pins the "no message parsing" invariant. No wire-drift here.
6. plan.ts +1 line
if (extractResult.failureToEnforce) throw extractResult.failureToEnforce;Textually correct — throws only if the stage populated failureToEnforce (which only happens under HF_VIDEO_EXTRACTION_FAILURE_MODE=enforce). Preserves default behavior (off → null → no throw). The wire-boundary side of this throw is the dependency documented in the PR body (see §2B / Nit A).
7. Test coverage of the actual paths
videoFrameExtractor.test.ts+157: exercisesclassifyVideoExtractionError(404 → non-retryable, 503 → retryable),runVideoExtractionWithRetry(one retry with partial-cleanup verification viaexistsSync(partialPath).toBe(false)), cancellation before operation (abortedAttempts === 0), non-retryable failures (deterministic + budget=0), and NaN budget (Number.isFinitefail-closed). Also two end-to-end assertions with the real ffmpeg fixture: media-start-beyond-duration throwsmedia_start_out_of_rangebefore invoking FFmpeg, and legacy metadata rejection is preserved unlesscollectProbeFailures: trueis explicitly enabled. All exercise the real paths, no mock trivialization.extractVideosStage.test.ts+151: coversresolveVideoExtractionPolicydefault-off,assertVideoExtractionSucceededsuccess, deterministic message-sanitization (.not.toContain("/tmp/"),.not.toContain("Signature")), exhausted-transient path collapses duplicatekind, and the legacy-failure fail-closed path.urlDownloader.test.ts+12: verifies the newonTransientRetrycallback fires once with the classifiedUrlDownloadErrorwhen a 503 is retried. The producer-side effect (recording the retry intotransientRetriescounter) is not directly asserted here but is downstream of this callback.
8. Retry budget interaction
- Downloader has its own
maxTransientRetries = 1insidedownloadWithRetry(unchanged by this PR). - Extraction wrapper adds another
runVideoExtractionWithRetrylayer with its own 0|1 budget. - Phase 1 (download): only urlDownloader's internal retry applies — not wrapped in the extraction wrapper. Max 2 HTTP attempts.
- Phase 2/3 (probe/extract): wrapped in
runVideoExtractionWithRetry— up to 2 ffprobe / ffmpeg invocations per source, with output-dir cleanup between attempts. boundedTransientRetryBudget(NaN | undefined | negative | 0)= 0 — verified by test.boundedTransientRetryBudget(1 | 5 | Infinity)= 1 — clamped. Total budget is tightly bounded.
9. Freshness / mergeability
mergeable_state: unstable per gh api /repos/heygen-com/hyperframes/pulls/2776. Not a blocker for review — CI settling on the stack is expected while #2774 (parent) shifts.
Findings
Nit A (P2, documented rollout dependency, not a landed defect)
The Step Functions NON_RETRYABLE_PLAN list (HyperframesRenderStack.ts:196), the AWS Lambda handler's normalizeTerminalErrorName (handler.ts:145), and the Cloud Run NON_RETRYABLE_ERROR_NAMES (gcp-cloud-run/src/server.ts:892) each currently key off Error.name — none contains VideoExtractionStageError nor either of the two new codes. If HF_VIDEO_EXTRACTION_FAILURE_MODE=enforce is enabled before the "companion transport mappings" ship (per the PR body's rollout §4), a deterministic VIDEO_SOURCE_UNRENDERABLE throw from plan.ts will be retried by Step Functions/Workflows (4× with backoff) instead of failing fast.
The PR body explicitly gates the enforce rollout on this dependency, so treating this as a P2 note not a blocker. Two low-cost hardenings would kill the trap statically:
- In
handler.ts:normalizeTerminalErrorName, extend the code list to include"VIDEO_SOURCE_UNRENDERABLE"and"VIDEO_EXTRACTION_FAILED"so.name := .coderewrite happens automatically once the state-machine list catches up. - Consider naming the class
VIDEO_SOURCE_UNRENDERABLE(or settingthis.name = codein the ctor) soError.nameandcodealign — that also matches the existing pattern forPLAN_HASH_MISMATCHwhich setserror.name = "PLAN_HASH_MISMATCH"at every throw site (handler.ts:693, 840, 848). This would let the state-machine list use either the class name or the code with equivalent effect, closing the drift lane entirely.
Both are follow-ups; not a change to this PR.
Nit B (P2, minor consistency)
In extractVideosStage.ts:283-292, the HDR-probe enforce branch throws a fresh VideoExtractionStageError synchronously from inside the Promise.all(...) — before any extraction-stage telemetry checkpoint is emitted. Compare against the frame-extraction path where failureToEnforce is captured and re-thrown by renderOrchestrator.ts:2151 after the logEvent("extraction_finalized", ...) observability row is written (line 2137-2149).
The comment on failureToEnforce explicitly says: Callers throw this only after their extraction telemetry checkpoint has been emitted. The HDR probe path violates that invariant — enforce-mode probe failures cost the observability row.
Two remediations:
- Preferred: capture the classified HDR probe failure into a similar
hdrProbeFailureToEnforce: VideoExtractionStageError | nullvariable, populate it in the catch, and defer the throw torunExtractVideosStage's return path (aftervideoExtractMsfinalization). - Or: at minimum, document the exception in the comment on
failureToEnforceso future readers know HDR probe throws pre-checkpoint by design.
Follow-up. Doesn't block this PR — the enforce lane hasn't shipped yet.
Observations (informational, no ask)
- classifyVideoExtractionError HTTP status fallback (line 175 onward in
videoFrameExtractor.ts) classifies 401/403 assource_rejected/ non-retryable. In the signed-URL / short-TTL world, 401 is sometimes a transient class ("signed URL expired between issue and probe"). But the primary path uses the typedUrlDownloadError.kindfrom urlDownloader, not this regex fallback; this path only fires for legacy raw-Errorpropagations. Fine. applyVideoExtractionFailurePolicyobservemode logs a warning but does not throw. Its log-only side effect isn't directly asserted by unit tests (log spy). Given the mode is a deliberate no-op-with-telemetry canary, an integration test that resolvesresolveVideoExtractionPolicy({HF_VIDEO_EXTRACTION_FAILURE_MODE: "observe"})and asserts the log line would tighten the loop, but not required.- Sorted failure summaries (
buildVideoExtractionStageError) — kinds sorted alphabetically for deterministic error message shape. Assertions inextractVideosStage.test.ts:117-120verify this. Good. extractDirectMisspartial-dir cleanup now also runsrmSync(partialDir, { recursive: true, force: true })before the first attempt (not just before retries). Combined with the on-retry cleanup + mkdir, ensures each attempt starts against a clean directory. Small correctness upgrade even for the defaultmaxTransientRetries=0path.
Summary paragraph
The PR types video-extraction failures into a bounded taxonomy, plumbs a producer-safe .code-based discriminator across the JSON/SSE render surface, and gates every behavioral change (typed aggregation + at-most-one retry + enforce failure gate) behind either HF_VIDEO_EXTRACTION_FAILURE_MODE ∈ {observe,enforce} or an explicit collectProbeFailures: true option — the default is byte-for-byte legacy. Multi-shape throw discipline is honored on the producer HTTP wire (class-instance + code-only throws converge on the same allowlisted code; message-embedded/arbitrary codes are rejected; assertions use canonical string constants). The trans-cloud wire-drift risk (Step Functions/Cloud Run's .name-keyed dispatch is unaware of VideoExtractionStageError) is real but explicitly documented in the PR body as a rollout dependency (enforce lane is gated on companion transport mappings), so it doesn't affect the default-off ship state. Two P2 nits: (A) the AWS Lambda normalizeTerminalErrorName and Step Functions/Cloud Run non-retryable lists are follow-up work called out in the rollout plan but easy to pre-land; (B) HDR-probe enforce failures throw before the extraction telemetry checkpoint, violating the invariant documented on failureToEnforce. APPROVE for merge default-off.
— Review by Via
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 13e17046c698a03134d5e33afe68b3f0af01c78d.
Stable-by-default plumbing is airtight. The default-off path is byte-identical to legacy behavior: resolveVideoExtractionPolicy fails closed on unrecognized values, boundedTransientRetryBudget clamps NaN/negatives/floats to 0, HF_VIDEO_EXTRACTION_MAX_RETRIES accepts only "1" after .trim(), and every hot path branches on maxTransientRetries === 0 ? directCall() : runVideoExtractionWithRetry() so the retry wrapper doesn't rewrap the call graph unless the candidate lane is enabled. The taxonomy is complete and doesn't leak — VideoExtractionStageError.message is built from ${kind}=${count} pairs only, and the extractSafeRenderErrorCode allowlist consciously refuses to parse arbitrary code strings or scan the message (the "does not forward arbitrary codes or parse message text" test pins that). The assertVideoExtractionSucceeded "signed URL / /tmp path stays out of the terminal error" test is exactly the right regression to lock in.
Cross-checked the retry-counter contract against downloadWithRetry and runVideoExtractionWithRetry: both increment onRetry when a retry is scheduled — including the case where the retry attempt itself exhausts the budget (attempt=0 fails → increment → attempt=1 fails → throw). So phaseBreakdown.transientRetries reflects "retries scheduled and consumed," which matches the PR body claim.
Concerns (non-blocking; both same observability-vs-terminal-throw shape, safe to defer until enforce flips)
plan.tsthrowsfailureToEnforcebefore extraction observability is emitted.packages/producer/src/services/distributed/plan.ts:920-929callsrunExtractVideosStageand immediately throwsextractResult.failureToEnforceon the next line. Comparepackages/producer/src/services/renderOrchestrator.ts:2147-2151: the in-process render path emits the fullextractionObservabilityrow (includingtransientRetries) FIRST, then throws. Once step 5 of your rollout flips candidate toenforce, the distributed planner (Lambda / GCP Cloud Run) will lose the per-failure telemetry that renderOrchestrator retains — exactly at the moment triage will need it. Cheap fix: emit the same observability checkpoint (or the subset that applies to the distributed planner) between therunExtractVideosStagereturn and thefailureToEnforcethrow, mirroringrenderOrchestrator.ts:2147.- HDR-probe enforce failure bypasses the stage's own observability path too. In
extractVideosStage.tsaround the HDR probe (catch (error)block afterrunVideoExtractionWithRetry(() => extractMediaMetadata(...))),throw new VideoExtractionStageError(...)in enforce mode exitsrunExtractVideosStagebeforeextractionResult.phaseBreakdown.transientRetriesis written and before the caller's observability checkpoint fires. Same class of gap as above, but earlier in the stage. Cheap fix: catch the HDR probe error, stash ahdrProbeFailureToEnforcesibling offailureToEnforce, and let the same caller pattern that throws for extraction failures throw for HDR probe failures too — so a single call site owns the "emit observability, then throw" ordering.
Nits
assertVideoExtractionSucceededis exported but appears test-only. The exported helper is used only byextractVideosStage.test.ts; production paths go throughapplyVideoExtractionFailurePolicy→failureToEnforce. If it's kept as a future public boundary (e.g. for the internal sidecar), a one-line docblock naming that intent would keep it from getting cargo-culted onto the enforce path.runVideoExtractionWithRetryhas twosignal?.abortedchecks per iteration (pre-op and post-error). Minor redundancy, but the post-error check preserves the classified diagnostic on thecancelledthrow — arguably worth the duplication.
Question
- Terminal-classifier registration timing. PR body says "internal sidecar and Experiment Framework must treat both exhausted stage codes as workflow-terminal … candidate enforcement must not be enabled until those companion mappings are deployed." Just want to make sure the AWS SAM
NON_RETRYABLE_*+ AWS CDKHyperframesRenderStack.ts+ GCP Cloud RunNON_RETRYABLE_ERROR_NAMESalso get updated (and per thePlanV2IntegrityErrordual-naming pattern from the #2788/#2789/#2790 arc, both the class name"VideoExtractionStageError"AND the code strings"VIDEO_SOURCE_UNRENDERABLE"/"VIDEO_EXTRACTION_FAILED"land in those lists) — otherwise the AWS Step Functions retry classifier will match on neither and retry-storm through the full backoff. Assuming that's the plan for step 4/5; flagging so it doesn't slip.
What I didn't verify
- Behavior when
HF_VIDEO_EXTRACTION_FAILURE_MODE=enforceand a real render hits a mixed transient+deterministic failure set (I only read the retryable-summary logic inbuildVideoExtractionStageError—retryable = all failures are retryable === true, which correctly biases toVIDEO_SOURCE_UNRENDERABLEon any deterministic entry). - Cross-package audit that
classifyVideoExtractionError's legacy string-matching branches ("[URLDownloader] Download timeout","ffprobe deadline", etc.) match the exact strings still emitted at head after #2774 (the URL-downloader messages there use"Download timeout after ${timeoutMs / 1000}s"and"Download failed: ${message}"without the[URLDownloader]prefix — but that's fine because theUrlDownloadErrortyped branch fires first for downloader errors, so the string-match branch only matters for pre-migration legacy callers).
13e1704 to
d0098b9
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at d0098b9fdb66ccf4d594f34dd00f6415a0d2822b — verified pure rebase on #2774 R2 (07b607350e). HF#2776's own contribution slice (parent-head → tip) is patch-id-identical to the R1 slice (ce07556297400dd47ec7a0c1296518c027458ef6), so none of the child's own code changed.
Nothing to re-review at the code layer. My R1 findings all apply verbatim at the same file:line:
- Non-blocking observability-vs-throw ordering:
plan.ts:920-929still throwsextractResult.failureToEnforcebefore emitting extraction observability — mirror therenderOrchestrator.ts:2147-2151"emit checkpoint, then throw" ordering before flipping candidate toenforce. - Non-blocking HDR-probe enforce-path gap in
extractVideosStage.ts— HDR probecatchthrowsVideoExtractionStageErrorbeforephaseBreakdown.transientRetriesis written, so the same "one call site owns the throw" fix closes both gaps. - Reminder: AWS SAM
template.yaml, AWS CDKHyperframesRenderStack.tsNON_RETRYABLE_{PLAN,CHUNK,ASSEMBLE}, and GCP Cloud RunNON_RETRYABLE_ERROR_NAMESall need both the class name ("VideoExtractionStageError") AND the code strings ("VIDEO_SOURCE_UNRENDERABLE","VIDEO_EXTRACTION_FAILED") added before step 5 of the rollout flips enforce — per the post-#2788 dual-naming pattern.
All still safe to defer while the flag defaults off. LGTM from my side, ready when its own CI settles and merge trigger comes.
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Reviewed exact head d0098b9fdb66ccf4d594f34dd00f6415a0d2822b against parent #2774 head 07b607350e1fb6e722f94e47d161801b9ffceec3.
The default-off rollout shape, bounded 0|1 retry budget, safe kind/count-only producer error, and .code allowlist on the JSON/SSE boundary all look sound. Via and Rames already covered the enforce-lane observability and downstream terminal-classifier dependencies, so I am not repeating them.
One additive documented-standard blocker remains in new production code:
-
Important — new bare assertions violate
CONTRIBUTING.md:47-54.videoFrameExtractor.ts:201-202checks the sentinel through(error as { hyperframesVideoSourceExtractionError?: unknown }).server.ts:132reads.codethrough(error as { code?: unknown }).server.ts:134then casts the narrowed string again to the literal union solely to satisfySet.has.
These are avoidable. The sentinel guard can use
"hyperframesVideoSourceExtractionError" in errorand compare the now-unknown property totrue. The server helper can use the same object/non-null/"code" in errorguard, then make the allowlist aReadonlySet<string>(or use a typed predicate) so the already-validated string can be checked without a second assertion.
The two new test assertions at extractVideosStage.test.ts:196-197 repeat the same pattern after toBeInstanceOf(Error); convert those to an actual instanceof guard as well so the test's runtime proof narrows the type.
I attempted the focused suites in a clean worktree; repository package artifacts must be built in dependency order before Vitest can resolve the workspace exports, so I am relying on the exact-head CI matrix for the final execution signal rather than claiming those local attempts as test results.
Verdict: Request changes.
Reasoning: The feature architecture is safe while default-off, but the PR adds several bare production/test assertions contrary to the repository's explicit type-safety standard; all have small guard-based fixes.
— Magi
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Two additional taxonomy defects remain at exact head d0098b9fdb66ccf4d594f34dd00f6415a0d2822b.
- Blocker: Preliminary HDR probes run under
Promise.all, and each enforce-mode catch immediately throws a one-member stage error (packages/producer/src/services/render/stages/extractVideosStage.ts:243-294). A transient rejection can win the race while another source has a deterministic failure, bypassing the aggregate “any non-retryable wins” classifier atextractVideosStage.ts:175-193. The engine's typed collection path demonstrates the aggregate seam (packages/engine/src/services/videoFrameExtractor.ts:1124-1154). Collect/all-settle preliminary failures, apply the aggregate classifier, and cover mixed transient-plus-deterministic ordering. - Blocker: FFmpeg launch maps only
ENOENTto terminal unavailable; every other spawn error becomes retryableffmpeg_transient(packages/engine/src/services/videoFrameExtractor.ts:616-635). DeterministicEACCES,ENOEXEC, and unknown launch failures therefore spend the candidate retry budget. Retry only known transient errnos and add errno-specific retry coverage.
Via and Rames already covered the workflow-terminal mapping dependency and HDR-probe telemetry timing, so I am not repeating those. My prior exact-head review separately covers the new bare type assertions. This child also depends on corrected #2774: merge the parent first, then rebase/refresh this PR and run the full exact-head CI matrix.
Verdict: Request changes.
Reasoning: Both paths can retry failures the declared contract treats as terminal, and the child cannot merge safely until corrected #2774 lands and a rebased full CI matrix completes.
— Magi
|
Addressed all five requested type-safety assertions at cf37b42. Production now uses real object/property/string guards for the cross-realm sentinel and safe wire code; the tests use an instanceof guard before reading Error.message. No behavior or rollout changes. Revalidated 105 focused engine tests, 15 focused producer tests, both package typechecks, lint, format, tracked-artifact, and Fallow gates. |
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at cf37b42422c714eb84ef5ebff9b78f5df190fe00 — delta from d0098b9fdb is 11 lines across 3 files.
Assertion-standards delta is clean. All five findings fixed structurally, not by escape hatch:
videoFrameExtractor.ts:198-203—isVideoSourceExtractionErrorsentinel check now uses"hyperframesVideoSourceExtractionError" in error && error.hyperframesVideoSourceExtractionError === true. Same semantics as the oldas-cast read (both returnundefined === true → falseon a missing property), but no cast.server.ts:118-121—SAFE_RENDER_ERROR_CODESretypedSet<string>(droppedas conston the array literal). Enables the cast-free.has(code)call below. Very minor signature loosening:extractSafeRenderErrorCodenow returnsstring | undefinedrather than the narrower"VIDEO_SOURCE_UNRENDERABLE" | "VIDEO_EXTRACTION_FAILED" | undefined, but the runtime allowlist is identical, and no caller in the diff relies on the tighter union type.server.ts:129-136—extractSafeRenderErrorCodenow readserror.codedirectly (safe because the preceding"code" in errorguard already narrowed the type) and passescodeto.has()without a cast. Same runtime allowlist, same terminal-message-scrubbing behavior. The three prior assertions the "signed URL / /tmp path stays out of the terminal error" regression pinned are all preserved.extractVideosStage.test.ts:193-201— replaced(caught as Error).messagewithif (!(caught instanceof Error)) throw new Error(...)thencaught.message. Test now fails hard rather than passing through anas-cast; the two.not.toContainassertions run on a narrowedErrortype.
Byte-audited that none of the substantive files from R1/R2 (plan.ts, extractVideosStage.ts runtime path, renderOrchestrator.ts) moved — my prior non-blocking findings still apply verbatim at the same file:line:
plan.ts:920-929still throwsextractResult.failureToEnforcebefore extraction observability emit; mirrorrenderOrchestrator.ts:2147-2151ordering before enforcing.extractVideosStage.tsHDR-probecatchstill throwsVideoExtractionStageErrorbeforephaseBreakdown.transientRetriesis written.- SAM/CDK/GCP terminal-list registration reminder for both
"VideoExtractionStageError"class name and"VIDEO_SOURCE_UNRENDERABLE"/"VIDEO_EXTRACTION_FAILED"code strings before step 5 rollout flips enforce.
All three still safe to defer while HF_VIDEO_EXTRACTION_FAILURE_MODE defaults off. LGTM from my side.
07b6073 to
4948966
Compare
cf37b42 to
36c4b63
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R2 exact-head review — hyperframes PR #2776
Head: cf37b42422c714eb84ef5ebff9b78f5df190fe00
Prior head: d0098b9fdb66ccf4d594f34dd00f6415a0d2822b
Delta: +11 / -11, 3 files (videoFrameExtractor.ts, server.ts, extractVideosStage.test.ts)
Verdict
COMMENTED — the Standards-lens fix I was asked to verify is structurally clean (all five bare as T / .message-without-guard findings from Magi's first CHANGES_REQUESTED are ADDRESSED at this head, no residual Standards violations introduced). However, Magi filed a second CHANGES_REQUESTED review at d0098b9fdb — separate from the Standards findings — flagging two taxonomy Blockers (HDR-probe Promise.all race + FFmpeg spawn-error errno mapping). The fix at cf37b42422 does not touch either surface; both blockers are NOT_ADDRESSED. Magi has not re-reviewed at cf37b42422 yet — both CHANGES_REQUESTED remain on record. My R1 seat is now neither blocking (my Standards findings are fixed) nor stamping (Magi's taxonomy blockers hold).
Miguel's findings verification
First CHANGES_REQUESTED review (Standards-lens, 2026-07-26T18:03:06Z @ d0098b9fdb)
Miguel enumerated the following at d0098b9fdb:
| # | Miguel's cite | Fix at cf37b42422 |
Verdict |
|---|---|---|---|
| 1 | videoFrameExtractor.ts:201-202 — sentinel check uses (error as { hyperframesVideoSourceExtractionError?: unknown }) |
videoFrameExtractor.ts:201-202 — replaced with "hyperframesVideoSourceExtractionError" in error && error.hyperframesVideoSourceExtractionError === true (proper in guard, no cast) |
ADDRESSED |
| 2 | server.ts:132 — (error as { code?: unknown }).code |
server.ts:132 — reads error.code directly after the preceding "code" in error guard narrows the type. No cast. |
ADDRESSED |
| 3 | server.ts:134 — casts narrowed string to literal union to satisfy Set.has |
server.ts:121 — SAFE_RENDER_ERROR_CODES retyped new Set<string>([...]) (dropped as const); has(code) now called without a second cast at server.ts:133. Runtime allowlist identical. |
ADDRESSED |
| 4 | extractVideosStage.test.ts:196 — (caught as Error).message (assertion 1) |
extractVideosStage.test.ts:196-199 — if (!(caught instanceof Error)) throw new Error("expected VideoExtractionStageError") narrows caught before caught.message is read |
ADDRESSED |
| 5 | extractVideosStage.test.ts:197 — (caught as Error).message (assertion 2) |
Same instanceof guard covers both .message reads at lines 199-200 |
ADDRESSED |
All five findings from Magi's first CHANGES_REQUESTED are structurally fixed (guards, not escape hatches). No as unknown as T, no ESLint disable, no test-only bypass.
Second CHANGES_REQUESTED review (taxonomy Blockers, 2026-07-26T18:05:30Z @ d0098b9fdb)
| # | Miguel's cite | State at cf37b42422 |
Verdict |
|---|---|---|---|
| B1 | extractVideosStage.ts:243-294 — HDR-probe Promise.all catch throws one-member VideoExtractionStageError synchronously; a transient rejection can win the race and preempt a deterministic failure elsewhere in the same batch, bypassing the aggregate "any non-retryable wins" classifier at extractVideosStage.ts:175-193. |
Body unchanged at cf37b42422 (lines 236-297 in this head). Enforce-mode catch still throws one-member VideoExtractionStageError(classified.retryable ? "VIDEO_EXTRACTION_FAILED" : "VIDEO_SOURCE_UNRENDERABLE", ...) synchronously inside the Promise.all map. No allSettled / aggregate-classifier seam introduced. |
NOT_ADDRESSED |
| B2 | videoFrameExtractor.ts:616-635 — FFmpeg spawn error maps only ENOENT → non-retryable ffmpeg_unavailable; every other spawn errno (EACCES, ENOEXEC, unknown) drops into retryable ffmpeg_transient, spending the candidate retry budget on deterministic failures. |
Body unchanged at cf37b42422 (lines 620-635 in this head). Still code === "ENOENT" → terminal, else → ffmpeg_transient retryable. No errno-specific coverage added. |
NOT_ADDRESSED |
Magi's second review labels these "Blocker" not nit, and his second-review verdict remains "Request changes." Neither is touched by the fix diff. Both are gated by the default-off enforce lane + bounded 0|1 retry budget, so they're deferrable in the same class as my R1 Nit A / Nit B — but Magi has not yet dismissed them, so his CHANGES_REQUESTED holds.
Standards lens re-run on all three changed files at exact head cf37b42422
Bare as T (excluding as unknown as T and as const)
videoFrameExtractor.ts:121: comment prose — "classified as VFR and routed" (false positive; word "as" in English)
videoFrameExtractor.ts:621: (processResult.error as NodeJS.ErrnoException | undefined)?.code === "ENOENT"
server.ts: (none)
extractVideosStage.test.ts:46: } as ExtractedFrames; // in makeExtracted scaffold
videoFrameExtractor.ts:121— comment; false positive.videoFrameExtractor.ts:621— bareas NodeJS.ErrnoException | undefinedcast. Pre-existing (present at parent07b607350eline 334, before this stack); confirmed viagh api /contents/…?ref=07b607350egrep. Not introduced by PR #2776's diff, so it's out of R2 scope as a Standards regression. However, this is the exact block Magi's Blocker B flags for errno-widening — so it lives at the intersection of a residual Standards fixture and an unaddressed taxonomy blocker. Any errno-widening fix should tighten this cast at the same time (e.g.if (typeof processResult.error === "object" && processResult.error !== null && "code" in processResult.error && typeof processResult.error.code === "string" && KNOWN_TERMINAL_ERRNOS.has(processResult.error.code))).extractVideosStage.test.ts:46— bareas ExtractedFramesscaffold. Pre-existing (present at parent07b607350eline 35). Test fixture pattern that predates the stack; not a new violation for this PR. Would be worth cleaning up in a follow-up (build a proper factory or export a public constructor from@hyperframes/engine), but flagging it here only for completeness — Magi did not raise it, Rames did not raise it, and the PR did not touch this scaffold.
No new bare as T was introduced by the fix delta.
Non-null assertions (!., .- Producer boundary throws
VideoExtractionStageErrorwithcode(VIDEO_SOURCE_UNRENDERABLE|VIDEO_EXTRACTION_FAILED),retryable, andfailures: [{kind, count}]— bounded message shape guaranteed not to contain paths or signed URLs (theassertVideoExtractionSucceededtest pins that no/tmp/orSignatureleaks). Message format:Video extraction failed for N source(s) [CODE; kind1=count,kind2=count]— deterministic, replayable, safe to forward.
The classifyVideoExtractionError mega-classifier covers UrlDownloadError (all 4 kinds), string-pattern legacy paths, HTTP status codes (404/410 → not_found, 408/429/5xx → transient), ffprobe deadline/absence, and fall-through to internal. All non-transient by default (fail-closed).
Bounded retry budget clamps boundedTransientRetryBudget(value: number | undefined): 0 | 1. Test fails closed to zero retries for a non-finite runtime retry budget locks the NaN case at 0 attempts — safe.
Producer error-code preservation via extractSafeRenderErrorCode. New SAFE_RENDER_ERROR_CODES Set<string> allowlist accepts only the two bounded producer codes; test does not forward arbitrary codes or parse message text pins that message-parsing is refused (even a message like "failed [VIDEO_SOURCE_UNRENDERABLE; secret=/tmp/x]" returns undefined). SSE and JSON error paths in server.ts:538-556 and :700-716 now forward the safe code when present. Sensible pattern.
Non-blocking observations (deferred until enforce flip)
- R2 Concern 1 (
plan.tsthrowsfailureToEnforcebefore observability) is still the same shape. The newplan.ts:948-950addsif (extractResult.failureToEnforce) throw extractResult.failureToEnforce;but there's no equivalent torenderOrchestrator.ts:2130-2151's observability checkpoint emit before it. Distributed planner (Lambda / GCP Cloud Run) will still lose per-failure telemetry that renderOrchestrator retains, once step 5 flips enforce. Cheap fix: mirror theperfSummary/observabilityCheckpointemit shape betweenrunExtractVideosStagereturn and thefailureToEnforcethrow. hdrProbeTransientRetriesis lost if HDR probe throws. The local counter only folds intoextractionResult.phaseBreakdown.transientRetriesatrunExtractVideosStageline 419 — afterthrowHdrProbeFailureshas potentially exited. In enforce mode + HDR probe failure, the operator sees the failure taxonomy but not the retry count from probes that succeeded before the failing one. Minor observability gap.- SAM/CDK/GCP terminal-list registration reminder still open.
HyperframesRenderStack.tsNON_RETRYABLE_{PLAN,CHUNK,ASSEMBLE},template.yamlErrorEqualslists, andserver.ts:900-922NON_RETRYABLE_ERROR_NAMESneed both the class name ("VideoExtractionStageError") AND the code strings ("VIDEO_SOURCE_UNRENDERABLE","VIDEO_EXTRACTION_FAILED") before step 5 rollout flips enforce — per the post-#2788 dual-naming pattern (botherror.name-shape anderror.code-shape throws register terminally).
All three still safe to defer while HF_VIDEO_EXTRACTION_FAILURE_MODE defaults off.
LGTM from my side.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R3 exact-head review — hyperframes PR #2776
Head: 54793b946eca033f689c2b1147ad7c5b0a7c14fd
Prior head: 36c4b6369457013a4b91904c2fb254e3115b7c57
Both Miguel-cited blockers structurally addressed. Standards lens re-run clean over the fix delta at head.
B1 — Concurrent HDR probe failures — ADDRESSED
packages/producer/src/services/render/stages/extractVideosStage.ts:
- Per-source
catch (error)(.ts:314-324) now returns{ error, classified }forobserve/enforce, only re-throws whenfailureMode === "off".Promise.allcannot short-circuit on the first probe rejection in candidate mode — this is behaviorally identical toallSettledfor that path. - Post-settle aggregation
probeFailures.filter(isHdrProbeFailure)produces the full set (.ts:284-327). throwHdrProbeFailures(.ts:223-233) →buildHdrProbeStageError(.ts:196-212) usesfailures.every((failure) => failure.retryable)to pick the code. Any non-retryable failure in the set flips the whole batch to terminalVIDEO_SOURCE_UNRENDERABLEregardless of completion order.- Order-independent regression at
extractVideosStage.test.ts:249-269:it.eachruns both orderings (download_transientfirst vs.source_missingfirst), both assertcode: "VIDEO_SOURCE_UNRENDERABLE", retryable: false.
B2 — FFmpeg spawn errno mapping — ADDRESSED
packages/engine/src/services/videoFrameExtractor.ts:686-711:
TRANSIENT_FFMPEG_SPAWN_CODES = new Set(["EAGAIN", "EMFILE", "ENFILE"]). Explicit allow-list.ENOENT→ffmpeg_unavailable(terminal). Every other value oferror.code—EACCES,ENOEXEC,EPERM, missing/empty, or any unknown errno — falls through toffmpeg_failedwithretryable: false. Default-deny, the inverse of the prior default-allow shape.- Regression at
videoFrameExtractor.errorClassification.test.ts:1-20pinsENOENT/EACCES/ENOEXEC/UNKNOWNas terminal andEAGAIN/EMFILE/ENFILEas retryable.EPERMis covered structurally by theUNKNOWNcase but not named; not a blocker.
Standards lens re-run
Grepped bare as T (excl. as const / as unknown as T), non-null !. / ![ / !;, angle-bracket casts, and .message reads across every file in the fix delta at 54793b946e. Newly added code in the B1/B2 slice: zero bare as T, zero non-null assertions, zero angle-bracket casts, zero unguarded .message. All hits (renderOrchestrator.ts DOM element casts, plan.ts:615 data-shape helper, extractVideosStage.ts:365 bounded-index !, extractVideosStage.test.ts:47 and videoFrameExtractor.test.ts:1672 fixture } as ExtractedFrames) are pre-existing and untouched by the fix commits — verified against the + slice of the diff. caught.message in extractVideosStage.test.ts:200-201 is inside a if (!(caught instanceof Error)) throw guard; all error.message reads in server.ts, renderOrchestrator.ts, plan.ts, urlDownloader.ts, and videoFrameExtractor.ts are instanceof Error-guarded.
R1 Nit A
Still deferred as documented rollout dependency. Step Functions NON_RETRYABLE_PLAN/_CHUNK/_ASSEMBLE at packages/producer/src/services/distributed/aws-stack.ts:196-239 still don't include VIDEO_SOURCE_UNRENDERABLE/VIDEO_EXTRACTION_FAILED — anticipated at R1 and R2. SAFE_RENDER_ERROR_CODES in server.ts:121-124 does include both codes, so the HTTP/SSE boundary is complete. Not blocking.
Peer state
Miguel has not re-reviewed at 54793b946e yet; his most recent review at 36c4b63694 is CHANGES_REQUESTED. James posted an issue comment at 18:34:59Z on 2026-07-26 summarizing the fixes and validation (producer focused tests 17/17, engine classification 7/7, both package typechecks, lint/format, full pre-commit/Fallow pass) — consistent with what I verified in code. My APPROVE stands independently.
Verdict: APPROVE
Reasoning: Both blockers structurally addressed with order-independent regression coverage; default-deny errno allow-list is the correct inversion; standards delta clean; deferred R1 Nit A is out of scope by design.
— Review by Via
54793b9 to
6265516
Compare
There was a problem hiding this comment.
Exact-head re-review at 6265516ee4d4c33ba045f05636412a84f3754618.
Both prior code blockers are resolved cleanly:
extractVideosStage.ts:284-327now lets every candidate-mode HDR probe settle into a typed outcome before classifying the batch.buildHdrProbeStageErrorusesfailures.every(...), so any deterministic source failure makes the aggregate terminal regardless of completion order. The two-order regression atextractVideosStage.test.ts:249-268pins that invariant.videoFrameExtractor.ts:686-710now retries only the explicit resource-pressure errno allowlist (EAGAIN,EMFILE,ENFILE).ENOENTremainsffmpeg_unavailable;EACCES,ENOEXEC, missing, and unknown codes fail closed as terminalffmpeg_failed. The seven-case classification table pins both sides.
The narrowed error reads and new tests are standards-clean. Focused verification of the patch-identical child slice passes 24/24 under Vitest.
This is a pure Graphite restack of the reviewed child slice onto corrected #2774 8dd0c68c68: the stable patch ID is identical (4e7f917e…), and the only whole-head delta from 54793b946e is the parent's three-line 240.0.0.0/4 fix plus regression. The parent probe now blocks both 240.0.0.1 and 255.255.255.255.
Verdict: COMMENT
Reasoning: Both implementation blockers and the parent dependency are resolved. This head is code-clear; I am holding the replacement approval only until its fresh required CI is terminal green.
— Magi
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at 6265516ee4d4c33ba045f05636412a84f3754618 — verified pure rebase on new #2774 head (8dd0c68c68).
git diff <#2774-head>..<#2776-head> | git patch-id --stable returns 4e7f917e6f8221fc4f5bc5fbe7110469f8b3a117 on BOTH the prior 54793b946e and the new 6265516ee4 — so #2776's own contribution is byte-identical to R5. All R5 findings stand, including the three deferred rollout-blockers (plan.ts observability-before-throw, probe-retry counter loss on failure, SAM/CDK/GCP dual-naming registration) still safe to defer while default off.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Byte-clean re-stamp at 6265516ee4. Verified blob-SHA equivalence against prior R3 head 54793b946e:
videoFrameExtractor.ts— MATCH (bloba3d7df8699…)extractVideosStage.ts— MATCH (blob5b0248be61…)server.ts— MATCH (blob9a3a2f88c3…)urlDownloader.ts— DIFFERS. This is expected — #2776 is stacked on #2774, and the Graphite restack propagates #2774's Class-E240.0.0.0/4fix into the child's copy ofurlDownloader.ts. Verified by diffing the two blobs: the only delta is the single["240.0.0.0", 4]subnet addition — same content #2774 R4 verified at8dd0c68c68.
Prior R3 APPROVE at 54793b946e stands for #2776's own scope (HDR-probe aggregate + errno allow-list). No new #2776-scoped changes introduced by the rebase.
— Review by Via
6265516 to
c0690c6
Compare
8dd0c68 to
2e84fae
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at c0690c60e2 — verified pure rebase on merged-main (a8ee81f3d8…, containing #2773).
git diff <merge-base>..<head> | git patch-id --stable returns 38a95a12e05db564dac33e1033765373dc52fb0c on BOTH 6265516ee4 and c0690c60e2 — #2776's own contribution byte-identical to R6. R6 findings stand; three deferred rollout-blockers (plan.ts observability-before-throw, probe-retry counter loss on failure, SAM/CDK/GCP dual-naming registration) still safe to defer while default off. Noted James will rebase again on merged-main after #2774 lands before the final stamp.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Byte-clean re-stamp at c0690c60e2. Pure Graphite rebase after #2773 merged to main.
Verified blob-SHA equivalence on all 12 PR-scoped files vs prior R3 head 6265516ee4:
- 11 files MATCH by blob-SHA (
urlDownloader.ts,urlDownloader.test.ts,videoFrameExtractor.ts,videoFrameExtractor.test.ts,videoFrameExtractor.errorClassification.test.ts,extractVideosStage.ts,extractVideosStage.test.ts,server.ts,server.errorCode.test.ts,observability.ts,renderOrchestrator.ts,engine/src/index.ts) plan.tsDIFFERS — expected. #2773 merged and modifiedplan.tsextensively (added planSize integration +.plan-workscrub +PlanTooLargeErrorbreakdown). #2776's own delta onplan.tsremains the same single+1line atplan.ts:999:if (extractResult.failureToEnforce) throw extractResult.failureToEnforce;. Verified by inspecting the patch at c0690c6 — #2776's own hunk is unchanged.
No content drift on #2776's own scope; rebase is pure post-#2773 main catch-up on plan.ts, plus #2776's original +1 line preserved. Prior R3 APPROVE stands. Miguel's two Blockers (HDR-probe race B1, FFmpeg errno allow-list B2) are still cleanly addressed at this content.
— Review by Via
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head approval at c0690c60e2549371adb69e2041f24e3ff1af0d98.
Both prior blockers remain resolved: candidate-mode HDR probes settle before aggregate classification, and FFmpeg launch retries use a narrow resource-pressure errno allowlist with deterministic failures terminal. Focused verification of this slice passes 24/24.
This Graphite head moved only with its corrected parent after #2773 merged. The #2776 child stable patch ID is unchanged from the reviewed slice (4e7f917e…), and its exact base is current #2774 2e84faeb28.
Verdict: APPROVE
Reasoning: The blocker fixes are regression-locked, the child contribution is patch-identical after the dependency restack, and no new #2776 behavior entered the exact head.
— Magi
c0690c6 to
33ca1de
Compare
The base branch was changed.
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Re-reviewed at 33ca1de0631be66bfa5c591a231256c69667226e — verified pure rebase on merged-main (814f9cd977…, containing #2774).
git diff <merge-base>..<head> | git patch-id --stable returns 4e7f917e6f8221fc4f5bc5fbe7110469f8b3a117 on BOTH the prior c0690c60e2 and the new 33ca1de063 — #2776's own contribution byte-identical to R7. R7 findings stand; three deferred rollout-blockers (plan.ts observability-before-throw, probe-retry counter loss on failure, SAM/CDK/GCP dual-naming registration) still safe to defer while default off.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Byte-clean re-stamp at 33ca1de063. Pure post-#2774-merge Graphite rebase.
Verified blob-SHA equivalence across all 13 PR-scoped files vs prior head c0690c60e2 — 13/13 MATCH:
- All engine + producer files (
urlDownloader.*,videoFrameExtractor.*,videoFrameExtractor.errorClassification.test.ts,extractVideosStage.*,server.*,plan.ts+1hunk,observability.ts,renderOrchestrator.ts,engine/src/index.ts) unchanged.
Rebase is genuinely patch-identical — no content drift on #2776's own scope, and the parent-merge propagation of #2774 is now fully absorbed into main.
Prior R3 APPROVE + prior byte-clean re-stamp both stand. Miguel's Blockers B1 (HDR-probe aggregate) + B2 (FFmpeg errno allow-list) remain cleanly addressed at this content.
— Review by Via
miguel-heygen
left a comment
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head re-stamp at 33ca1de0631be66bfa5c591a231256c69667226e.
I independently verified this is a pure post-#2774-merge rebase of the already-reviewed #2776 child slice:
- Stable patch ID is identical before and after the rebase:
4e7f917e6f8221fc4f5bc5fbe7110469f8b3a117. - The prior and current child diffs contain the same 13 files.
- All 13 PR-scoped files match the prior reviewed head
c0690c60e2by blob SHA. - The exact-head required matrix is terminal green, including Build, Test, Typecheck, runtime contract, and both Windows jobs.
The previously resolved blockers remain regression-locked: candidate-mode HDR probes settle before aggregate classification, and FFmpeg launch retries use the narrow resource-pressure errno allowlist with deterministic launch failures terminal.
Verdict: APPROVE
Reasoning: No child-slice content drift occurred during the final restack, and every required exact-head check is green.
— Magi
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
R8 (pure-rebase-on-merged-#2774; patch-id 4e7f917e… identical to R7 slice) stands. Fresh CI settled clean. Applying stamp per James's ask at 1785095419.012049.
Merge activity
|
## Summary - classify per-source video download/probe/decode/extraction failures with a bounded taxonomy and safe producer-facing summaries - add candidate-only, at-most-one transient retry with cleanup and retry telemetry - preserve default engine/producer behavior when the policy is off - carry allowlisted extraction error codes through blocking JSON and SSE responses ## Stack Depends on heygen-com#2774 for atomic remote downloads and its single owned download retry. This PR is intentionally based on `fix/atomic-video-download-retry`; rebase/change the base to `main` after heygen-com#2774 merges. ## Default compatibility `HF_VIDEO_EXTRACTION_FAILURE_MODE` defaults to `off` and forces `maxTransientRetries=0`. With the feature off: - metadata probe failures keep the legacy Promise rejection - grouped extraction keeps the existing grouped-to-direct fallback - no new producer failure gate is enforced - render-plan schema, Plan v1 artifacts, chunk routing, and distributed execution are unchanged Typed metadata aggregation is explicit and enabled only by the candidate enforce lane. ## Retry ownership - remote downloads: exactly one retry owned by heygen-com#2774 - metadata/FFmpeg extraction: at most one retry only when `HF_VIDEO_EXTRACTION_MAX_RETRIES=1` - invalid, missing, rejected, out-of-range, zero-output, cancellation, and unknown/internal failures do not retry - non-finite or invalid runtime retry budgets fail closed to zero - the superset optimization is never retried; on failure it preserves direct-member fallback, and only the individual ranges can use the bounded retry - retry counters increment when a retry is scheduled, including exhausted retries The internal sidecar and Experiment Framework must treat both exhausted stage codes as workflow-terminal after the producer-local budget. Candidate enforcement must not be enabled until those companion mappings are deployed, or Temporal can multiply producer attempts. ## Failure contract - `VIDEO_SOURCE_UNRENDERABLE`: at least one deterministic/unknown source failure - `VIDEO_EXTRACTION_FAILED`: all source failures are transient but the producer-local budget is exhausted Only the allowlisted code and kind/count summaries cross JSON/SSE. Raw diagnostics remain engine-local because they may contain signed URLs or local paths. ## Rollout 1. merge and deploy with stable/candidate both `off` 2. candidate `observe`, retries 0 3. candidate `observe`, retries 1 4. deploy internal + EF terminal transport mappings 5. candidate `enforce`, retries 1 6. keep stable off until success delta, retry counts, extraction latency, CPU/disk, and queue backlog are acceptable ## Validation - engine focused suites: 105 passed - producer focused suites: 15 passed - full engine suite: 1,176 passed, 3 skipped - full producer unit lane: 32 Vitest files / 393 tests plus all classified Bun unit tests - engine and producer typechecks passed - oxlint, oxfmt, Fallow, tracked-artifact, and commit hooks passed - independent review: approved for merge default-off; candidate enforcement held on companion transport rollout
4 participants
Summary
Stack
Depends on #2774 for atomic remote downloads and its single owned download retry. This PR is intentionally based on
fix/atomic-video-download-retry; rebase/change the base tomainafter #2774 merges.Default compatibility
HF_VIDEO_EXTRACTION_FAILURE_MODEdefaults tooffand forcesmaxTransientRetries=0.With the feature off:
Typed metadata aggregation is explicit and enabled only by the candidate enforce lane.
Retry ownership
HF_VIDEO_EXTRACTION_MAX_RETRIES=1The internal sidecar and Experiment Framework must treat both exhausted stage codes as workflow-terminal after the producer-local budget. Candidate enforcement must not be enabled until those companion mappings are deployed, or Temporal can multiply producer attempts.
Failure contract
VIDEO_SOURCE_UNRENDERABLE: at least one deterministic/unknown source failureVIDEO_EXTRACTION_FAILED: all source failures are transient but the producer-local budget is exhaustedOnly the allowlisted code and kind/count summaries cross JSON/SSE. Raw diagnostics remain engine-local because they may contain signed URLs or local paths.
Rollout
offobserve, retries 0observe, retries 1enforce, retries 1Validation