Repository navigation
fix(producer): report unavailable frame checks in debug logs - #6200
miguel-heygen merged 4 commits into
Conversation
Preview approval requiredThis pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit |
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for this, it is a tidy change. A few things I checked:
- The event fires only after the duration checks pass and only when a frame count was expected but none usable came back, so a truncated duration still fails first, and PNG sequences (counted from files) never log it. The tests cover each of these, including the throwing-logger case.
- The job ID claim holds: the logger passed in from the orchestrator is the render's scoped logger, which adds the job ID, and with
--debugit writes every level torender.logwhile the default console logger stays at info. - The docs match the code (
debugon the render request,--debugon the CLI,render.login the debug work directory).
Two small things:
- The branch now conflicts with main in
packages/producer/src/services/render/artifactTransaction.ts. A recent change on main rewrote the same import block (thenode:fsimport is now one line andrenameWithRetrycomes from@hyperframes/engine). Keeping main's imports and adding your../../logger.jsimport should resolve it. - Optional: the new tests build
ArtifactTransactiondirectly, so nothing would fail if thelogargument atrenderOrchestrator.ts(thenew ArtifactTransaction(..., log)call) were dropped later. Your manual CLI render covers it today; a small test at that level would keep it covered.
Verdict: APPROVE
Reasoning: Correct, narrowly scoped, every claim in the description checks out against the code and checks are green at this head; it only needs a rebase for an import-line conflict.
miguel-heygen
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. Both points from last time are covered at this head:
- The import conflict is resolved the way main has it (single-line
node:fsimport,renameWithRetryfrom@hyperframes/engine) with your logger import added, and nothing from main was lost. - The new case in
hlsRender.integration.test.tsgoes throughcreateRenderJobandexecuteRenderJobwith a real HLS render and checks the event carries the output path, expected frames and the job ID, so dropping the logger argument at theArtifactTransactioncall inrenderOrchestrator.tswould now fail a test. It ran in CI (all 9 HLS tests passed, none skipped).
All checks are green at this head.
Verdict: APPROVE
Reasoning: Head f267ed4 resolves the conflict cleanly, adds an end-to-end test for the logger wiring that ran in CI, and every check is green.
What
Record
artifact_frame_count_unavailableat debug level when artifact validation expects a frame count but the container supplies no usable count. The event includes the output path, expected frames, and the render's scoped job ID. It is captured inrender.logwith--debugand is available to custom producer loggers.Why
WebM and HLS commonly omit frame-count metadata. A generated four-second WebM with 90 frames passes a 120-frame expectation because its duration still matches; currently this leaves no indication that the frame-count check was skipped. The diagnostic makes this validation limit traceable. Default console output stays quiet.
Related work
Refs #3484, including the current status comment. This follows the debug-level option accepted in #5242's review. PNG counting is already covered by that merged work. This contribution reports unavailable frame checks; verifying the missing frames themselves still depends on obtaining a usable count.
How
Pass the existing scoped render logger into the artifact transaction. Emit the event after the duration gate when a requested frame count is unavailable. Keep known-count, duration, dimension, and PNG checks intact; a failing diagnostic sink cannot change the validation result. Document the limitation and debug logging in the producer reference.
Test plan
--debug: both decode to 10 frames / 1 second. Debug records exactly one event with the job ID in the retained log; normal output and stdout contain no event. The existing fixture passes CLI lint and browser check.