fix(daemon): fail media runs when output file is missing (#2892)#3039
fix(daemon): fail media runs when output file is missing (#2892)#3039leno23 wants to merge 2 commits into
Conversation
Verify media task output exists on disk before marking tasks done, and downgrade succeeded chat runs on image/video/audio projects when no matching deliverable file was written.
lefarcen
added
size/L
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hi @leno23, thanks for picking up #2892 and spelling out the missing-output failure mode clearly. The Summary and Validation sections give reviewers useful context already; one PR-template bit is still missing: could you add the Surface area checklist and tick the daemon/runtime/API-ish boxes that apply? That helps the pool reviewer quickly scope that this is a high-risk daemon path even though it’s a bug fix.
I’ve routed this to @Siri-Ray for the pool review; once that checklist is filled in, the description side should be in good shape. 🙌
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c66f476cf5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const files = await listFiles(PROJECTS_DIR, projectId, { | ||
| metadata: projectRecord.metadata, | ||
| }); | ||
| const hasDeliverableFile = files.some((file) => file.kind === projectKind); |
There was a problem hiding this comment.
The close-handler treats a media run as successful whenever any project file already has kind === projectKind, so a run that exits 0 without producing new output will still pass if the project contains an older image/video/audio artifact. This is reproducible on repeat runs in non-empty media projects and defeats the intended "missing output" guard for the current execution. The check should be scoped to files created/updated during this run (for example by startedAt/task timestamp) instead of the entire historical file set.
Useful? React with 👍 / 👎.
| const files = await listFiles(PROJECTS_DIR, projectId, { | ||
| metadata: projectRecord.metadata, | ||
| }); | ||
| const hasDeliverableFile = files.some((file) => file.kind === projectKind); |
There was a problem hiding this comment.
This kind equality check can incorrectly fail successful audio runs when the generated file uses a valid audio extension that kindFor does not map to 'audio' (e.g. .flac, .aac, .ogg/Opus). renderOpenAISpeech can intentionally emit those formats, so the run may produce a real deliverable but still be downgraded to failed because file.kind becomes 'binary'.
Useful? React with 👍 / 👎.
lefarcen
left a comment
lefarcen
left a comment
There was a problem hiding this comment.
Hi @leno23, thanks for picking up #2892 and spelling out the missing-output failure mode clearly. The Summary and Validation sections give reviewers useful context already; one PR-template bit is still missing: could you add the Surface area checklist and tick the daemon/runtime/API-ish boxes that apply? That helps the pool reviewer quickly scope that this is a high-risk daemon path even though it’s a bug fix.
I’ve routed this to @Siri-Ray for the pool review; once that checklist is filled in, the description side should be in good shape. 🙌
Annotate the media task stub in media-deliverable-guard tests so error fields remain accessible after finalizeMediaTaskFromGenerateResult.
Siri-Ray
left a comment
Siri-Ray
left a comment
There was a problem hiding this comment.
@leno23 thanks for jumping on the missing-output path and adding focused daemon coverage. I found one remaining main-path issue in the close reconciliation: a repeat media run can still be marked successful based on an older deliverable in the same project, so this needs one more pass before merge.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| const files = await listFiles(PROJECTS_DIR, projectId, { | ||
| metadata: projectRecord.metadata, | ||
| }); | ||
| const hasDeliverableFile = files.some((file) => file.kind === projectKind); |
There was a problem hiding this comment.
This predicate checks the whole project file list, so any older file with the same kind lets the current run close as succeeded even when this run wrote nothing. That is a primary repeat-run path for media projects: listFiles() returns all historical files unless a since filter is passed, and the new test coverage only exercises hasDeliverableFile as a boolean without modeling an existing deliverable from a previous run. As a result, #2892 can still reproduce in a non-empty video/image/audio project after one successful generation.
Please scope the close reconciliation to output produced by this run, for example by recording the run/task start timestamp and calling listFiles(..., { metadata, since: startedAt }), or by comparing against the pre-run file snapshot / exact generated task output name before accepting success. Add a regression test where a project already contains an older media file and the current run exits cleanly without creating a new deliverable.
3 participants
Summary
done; missing output now becomes a failed task withMEDIA_OUTPUT_MISSING.succeededon an image/video/audio project, reconcile status against project files and fail with a clear error if no matching deliverable was written.Fixes #2892
Surface area
apps/daemon/src/media.ts—verifyMediaOutputOnDisk,finalizeMediaTaskFromGenerateResultapps/daemon/src/media-deliverable-guard.ts— pure close-status classifier for media projectsapps/daemon/src/media-routes.ts,apps/daemon/src/server.ts— wire verification into task completion + chat run close handlerapps/daemon/tests/media-deliverable-guard.test.tsValidation
cd apps/daemon && pnpm test tests/media-deliverable-guard.test.ts(8 passed)/claim
Made with Cursor