XS⚠️ ◾ fix: don't let a post-EXECUTING_TASK error un-complete the workflow (#306)#961
XS⚠️ ◾ fix: don't let a post-EXECUTING_TASK error un-complete the workflow (#306)#961tomek-i wants to merge 1 commit into
Conversation
…306) Root cause: currentStage was never advanced to UPDATING_METADATA, so any exception escaping after EXECUTING_TASK completed (e.g. a network retry exhaustion during the best-effort portal submission or the metadata update) got attributed to the outer catch's currentStage, which was still pinned at EXECUTING_TASK. The outer catch then unconditionally re-failed that stage even though it had already completed — silently un-completing a run whose issue creation genuinely succeeded, and permanently blocking the Final Result panel since isWorkflowReadyForFinalOutput requires executing_task to stay "completed". - Advance currentStage to UPDATING_METADATA before that stage's block, matching every earlier stage's pattern. - Move the portal-submission's isAuthenticated() await inside its own try so no await in that gating condition can bypass local error handling. - Harden the outer catch with shouldFailStageOnUnexpectedError: only re-fail a stage that was genuinely in-flight ("in_progress"/"not_started") when the error hit, never one that already reached "completed"/"skipped". Closes #306
PR Metrics✔ Thanks for keeping your pull request small.
Metrics computed by PR Metrics. Add it to your Azure DevOps and GitHub PRs! |
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes a workflow-state attribution bug where an exception occurring after EXECUTING_TASK could incorrectly flip that already-completed stage back to failed, which in turn blocked the UI’s Final Result panel (issue #306).
Changes:
- Advance
currentStagetoUPDATING_METADATAbefore running that block so unexpected errors are attributed to the correct stage. - Move the portal-auth
isAuthenticated()awaitinside the portal submissiontryto prevent unguarded async evaluation from escaping. - Add
shouldFailStageOnUnexpectedError()and use it in the outer catch so only genuinely in-flight stages (not_started/in_progress) are failed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/backend/services/workflow/youtube-stage-decisions.ts | Adds shouldFailStageOnUnexpectedError() to prevent retroactively failing completed/skipped stages in outer catch scenarios. |
| src/backend/services/workflow/youtube-stage-decisions.test.ts | Adds regression tests for the new helper’s status-handling behavior. |
| src/backend/ipc/process-video-handlers.ts | Fixes stage attribution for metadata updates, hardens portal submission guard, and applies the new outer-catch decision helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it("covers every WorkflowStatus value so a future status can't silently change behaviour unnoticed", () => { | ||
| const allStatuses: WorkflowStatus[] = [ | ||
| "not_started", | ||
| "in_progress", | ||
| "completed", | ||
| "failed", | ||
| "skipped", | ||
| ]; | ||
| const results = allStatuses.map((status) => shouldFailStageOnUnexpectedError(status)); | ||
| expect(results).toEqual([true, true, false, false, false]); | ||
| }); |
|
🚀 Pre-release build is available for this PR: |
|
Hi there! This PR has been here a while. Did you know you should avoid merge debt? Please review and merge or close. Thanks! |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
muster review — PR #961Two independent lenses reviewed this diff in parallel:
Both lenses independently agree: the fix is structurally sound, addresses the actual root cause (not just the reported symptom), the new Bottom lineNo blocking, major, minor, or nit findings — review advisory only, nothing to act on. Consolidated: blocking 0, major 0, minor 0, nit 0. |
tomek-i
removed
the
armada:reviewing
|
✅ reviewed, addressed, green — awaiting human merge (auto-merge off). |
tomek-i
added
the
armada:reviewing
|
🔭 crows-nest: ready-PR pipeline started — review → address → re-validate → gated merge. |
tomek-i
added
armada:record
muster review — PR #961Two independent lenses reviewed this PR in parallel:
Verdict0 blocking, 0 major, 0 minor, 0 nit — no findings from either lens. The two lenses independently agree: the root-cause diagnosis is correct, the fix (advancing Bottom line: no blocking findings — review advisory only. Ready from a review-findings standpoint. Note (minor, non-blocking observation from codex-rescue, not filed as a finding): the |
tomek-i
removed
the
armada:reviewing
|
✅ crows-nest: reviewed — both lenses (code-review + codex-rescue) found zero blocking issues. Green and awaiting human merge (auto-merge off). |
Summary
After creating a YakShaver issue from a video, the app could show "failed after 3 attempts - cannot connect to API" and never show the Final Result panel — even though the issue was created and the YouTube metadata had genuinely been updated. A maintainer noted the "Updating Metadata" step never got its ✅ checkmark despite the metadata actually being applied.
Closes #306
Root cause
In
src/backend/ipc/process-video-handlers.ts,processVideoSourcetracks a localcurrentStagevariable that is advanced at the start of every stage (CONVERTING_AUDIO,TRANSCRIBING, …EXECUTING_TASK) so the outercatchcan attribute an escaping error to the right stage. It was never advanced toUPDATING_METADATA.So once
EXECUTING_TASKcompleted (the issue was created),currentStagestayed pinned atEXECUTING_TASKfor the rest of the run — through the best-effort portal submission and the YouTube metadata update. If any exception escaped to the outercatchduring that window (e.g. agoogleapis/gaxiosnetwork call that gives up after its default 3 retries — exactly "failed after 3 attempts" — during the metadata update, or an equivalent transient failure in the non-fatal portal submission), the outer catch did:Since
currentStagewas still"executing_task"and that stage's status was"completed"(not"failed"), this silently flipped a genuinely-completedEXECUTING_TASKback to"failed"— even though the backlog item had already been created. The renderer'sisWorkflowReadyForFinalOutput(src/ui/src/utils/index.ts) requiresexecuting_task.status === "completed"before it will ever show the Final Result panel, so this single mis-attribution both:EXECUTING_TASKstage had been silently un-completed).This is the same class of "silent success masked as failure" bug this codebase has fixed before for the upload/metadata stages (#672, #861, #808) — this time for the outer catch's stage attribution itself.
Fix
currentStagetoUPDATING_METADATAright before that stage's block runs, mirroring every other stage. Any stray exception in that region is now correctly attributed toUPDATING_METADATA(which the UI already tolerates as a terminal "failed" state) instead of the unrelated, already-completedEXECUTING_TASK.awaitfrom the portal-submissionifcondition (IdentityServerAuthService...isAuthenticated()was evaluated ahead of that block's owntry). It's now the first line inside thetry, closing a structural gap where a future change to that method (or condition) could throw withcurrentStagestill stuck atEXECUTING_TASK.shouldFailStageOnUnexpectedError(src/backend/services/workflow/youtube-stage-decisions.ts): only re-fail a stage that was genuinely"in_progress"or"not_started"when the error hit. A stage that already reached"completed"or"skipped"is never retroactively re-failed by an unrelated, later error. This closes the class of bug (not just today's known call sites) so a future stage that forgets to advancecurrentStagecan't reproduce this again.Bug repro evidence
I reproduced the panel-gating logic directly with a throwaway React Testing Library test against
FinalResultPanel, confirming the pure UI state machine correctly shows the panel onceupdating_metadatareaches any terminal status (including"failed"). That ruled out the UI layer and pointed the investigation squarely at the backend's stage bookkeeping.I then traced the exact code path:
metadataBuilder.build()andyoutube.updateVideoMetadata()calls are correctly wrapped by their own local try/catch and do fail the metadata stage cleanly on their own — butcurrentStagenever reachingUPDATING_METADATAmeant the outer catch was the actual defect: it could re-failEXECUTING_TASKbehind the scenes whenever anything else in that later window threw, independent of which specific dependency (portal submission, YouTube API, or an LLM call) was the trigger. Confirmed viagoogleapis-common'sapirequest.js(options.retry = trueby default) +gaxios's defaultretry: 3/noResponseRetries: 2config that a YouTube API call genuinely does give up after 3 attempts on a network blip, matching the reported error text.Since a full end-to-end repro requires simulating an Electron main-process video-processing run with mocked Google/LLM/portal APIs, I instead added regression coverage at the same seam the codebase already uses for this kind of stage-routing logic (
youtube-stage-decisions.ts, home ofapplyUploadStageOutcome/resolveMetadataStagefrom #672/#798):shouldFailStageOnUnexpectedErroris unit-tested against everyWorkflowStatusvalue, proving a stage that already completed/skipped is never re-failed while one still in-flight correctly still is.Testing performed
npm run build— cleannpx vitest run --reporter=verbose --exclude 'src/ui/**'— 690 passed (67 files), including newshouldFailStageOnUnexpectedErrorcoveragenpm --prefix src/ui test— 228 passed (32 files)npm run lint— cleanFiles changed
src/backend/ipc/process-video-handlers.ts— advancecurrentStage, move the auth check inside its try, use the new helper in the outer catchsrc/backend/services/workflow/youtube-stage-decisions.ts— newshouldFailStageOnUnexpectedErrorhelpersrc/backend/services/workflow/youtube-stage-decisions.test.ts— regression tests for the new helper🤖 Generated with Claude Code