Skip to content

fix(producer): report unavailable frame checks in debug logs - #6200

Merged
miguel-heygen merged 4 commits into
heygen-com:mainfrom
user-github-me:fix/producer-frame-count-debug
Oct 25, 2026
Merged

miguel-heygen merged 4 commits into
heygen-com:mainfrom
user-github-me:fix/producer-frame-count-debug

Conversation

@user-github-me

@user-github-me user-github-me commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

What

Record artifact_frame_count_unavailable at 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 in render.log with --debug and 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

  • 359 unit tests across artifact transactions, orchestration, execution context, file logging, and console logging pass. All 9 real HLS pipeline tests and 4 rename-retry tests also pass. Five new diagnostic regression cases fail on the unchanged implementation. Coverage includes invalid counts, default silence, known-count shortfalls, duration failures, HLS, and logger failures.
  • Full workspace build, producer typecheck, CLI rebuild, lint/format, test reachability, normal commit hooks, and both comment checks pass.
  • Real built-CLI WebM renders with and without --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.
  • Producer documentation updated; Mint validation and broken-link checks pass.
  • Integrate current main’s retrying artifact rename while retaining the scoped logger. The real HLS pipeline test verifies the event includes the output, expected frames, and render job ID; removing the logger connection reproduces one failure with the eight existing render checks still passing.
  • Comments describe the validation constraint and diagnostic failure isolation.

@mintlify

mintlify Bot commented Oct 24, 2026 •

Copy link
Copy Markdown

Preview approval required

This pull request comes from a fork, so Mintlify won't build a preview until someone with write access approves commit f267ed4.

Approve preview

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --debug it writes every level to render.log while the default console logger stays at info.
  • The docs match the code (debug on the render request, --debug on the CLI, render.log in the debug work directory).

Two small things:

  1. The branch now conflicts with main in packages/producer/src/services/render/artifactTransaction.ts. A recent change on main rewrote the same import block (the node:fs import is now one line and renameWithRetry comes from @hyperframes/engine). Keeping main's imports and adding your ../../logger.js import should resolve it.
  2. Optional: the new tests build ArtifactTransaction directly, so nothing would fail if the log argument at renderOrchestrator.ts (the new 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 miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:fs import, renameWithRetry from @hyperframes/engine) with your logger import added, and nothing from main was lost.
  • The new case in hlsRender.integration.test.ts goes through createRenderJob and executeRenderJob with a real HLS render and checks the event carries the output path, expected frames and the job ID, so dropping the logger argument at the ArtifactTransaction call in renderOrchestrator.ts would 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.

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 25, 2026
Merged via the queue into heygen-com:main with commit 0c3387b Oct 25, 2026
120 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants