Skip to content

Commit 70704d9

Browse files
christian-byrneConnor Byrne
andauthored
fix(first-run-tour): stop promising a result once acceptance is lost (#15172)
Stacked on #15091 — base is `fix/14619-tour-unaccepted-run`, merge after it. #15091 gave the first-run tour a 15s acceptance deadline, which covers a submission the backend refuses outright. It left one hole: a run that *was* accepted and then loses its job without an outcome. Two live paths do that, and **neither is the mid-run credits path this description originally led with** — thanks @benjcooley for catching that: | path | what it leaves behind | covered before this PR? | | --- | --- | --- | | accepted job dropped with **no status ever written** — the cloud "waiting for a machine" job cancelled or reconciled away | nothing | ❌ no branch can fire | | `handleServiceLevelError` ("Job has stagnated") — drops the job, records a prompt error, never touches `workflowStatus` | a stale `running` | ❌ the stale `running` shadowed the error branch | | ~~mid-run credits (`handleAccountPreconditionError`)~~ | ~~stale `running`~~ | ✅ **already covered** — #15161 clears the status, so it ends via the existing `undefined`-after-`running` branch | In the uncovered cases `runState` sits on `generating` forever and the card keeps promising "Hang tight, your result lands right here the moment it's ready." The controller had two disarms and no re-arm. This makes losing acceptance a signal in its own right: extend the existing `tourRunAccepted` watcher so true -> false fails the run when it is still `generating`. ```ts watch(tourRunAccepted, (accepted) => { if (accepted) stopAcceptDeadline() else if (runState.value === 'generating') runState.value = 'failed' }) ``` No re-armed deadline — this is immediate, not a second timer. The `'generating'` gate is what keeps healthy runs safe: every run leaves the queue when it finishes. `handleExecutionSuccess` and `handleExecutionError` write the terminal status and call `resetExecutionState` in the same synchronous handler, and the status watcher is registered before this one, so a finished run has already left `generating` by the time this fires. Test 3 below covers exactly that tick. Tests, in the existing `'a run behind a dropped socket'` describe: - accepted job removed with no status -> `failed` - accepted job, status `running`, removed (the stagnation case) -> `failed` - accepted job, status `completed` written in the same tick as the removal -> stays `succeeded` - accepted job, status `failed`, then removed -> stays `failed` The first two fail on the base branch (`expected 'generating' to be 'failed'`). Of the last two terminal-status guards, **only the `completed` one discriminates** — ungated, the `failed` case writes `'failed'` over `'failed'` and still passes. It is kept as documentation of intent, not as a guard; the earlier claim that both pinned the gate was wrong. Two tests added in review: - `stays failed when an unrelated workflow churns the status map` — pins the transition gate below. Fails without it (`expected 'generating' to be 'failed'`). - `gives up on a stagnated job that leaves an error and a stale status` — the other half of the stagnation path. Documents it; does not discriminate the gate. **Review fix:** the status watcher's `status === 'running'` branch was unconditional, and its source re-evaluates whenever the `workflowStatus` map is replaced (`mutateStatus` swaps the whole map, so *any* workflow's change re-runs it) or an error flag flips. With a stale `running` sitting there, each re-evaluation put the card back on "your result lands right here" after this watcher had already failed the run. It is now gated on an actual transition into `running`, which also removes the registration-order dependency the old comment overclaimed away. Complementary to #15161 (**merged 2026-08-13**), which fixes the same user-visible symptom from the store side by clearing the stale status. Independent of it: this is the controller refusing to promise regardless of whether the store cleans up. Both are wanted; neither duplicates the other. Requested by @benjcooley in review on #15091 (acceptance-loss watch for the mid-run credits variant) and by CodeRabbit (acceptance-loss coverage, terminal statuses stay terminal). ## Tests - 58 unit tests green in `useFirstRunTourController.test.ts` (54 existing + 4 new), full-file and each new test in isolation. - `pnpm lint`, `pnpm typecheck`, `pnpm format:check` pass. --------- Co-authored-by: Connor Byrne <c.byrne@comfy.org>
1 parent 2e0ba75 commit 70704d9

2 files changed

Lines changed: 138 additions & 1 deletion

File tree

src/renderer/extensions/firstRunTour/tour/useFirstRunTourController.test.ts

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,12 @@ function acceptRun(workflow: unknown) {
202202
return nextTick()
203203
}
204204

205+
/** The queue letting a job go, which `resetExecutionState` does silently. */
206+
function removeRun() {
207+
mocks.queuedJobs.value = {}
208+
return nextTick()
209+
}
210+
205211
function finishRun(workflow: unknown, status: string) {
206212
mocks.workflowStatus.value = new Map(mocks.workflowStatus.value).set(
207213
workflow,
@@ -506,6 +512,110 @@ describe('useFirstRunTourController', () => {
506512
'the grace timer must not clobber an outcome that arrived before it fired'
507513
).toBe('succeeded')
508514
})
515+
516+
it('stops promising a result once the queue lets go of its job', async () => {
517+
await tourOnRunStep()
518+
mountRunButton('queue-button', () => {}).click()
519+
await acceptRun(TOUR_WORKFLOW)
520+
521+
await removeRun()
522+
523+
expect(
524+
mocks.runState.value,
525+
'an accepted job that leaves without an outcome leaves the card waiting on a result nobody will send'
526+
).toBe('failed')
527+
})
528+
529+
it('stops promising a result when a running job is dropped mid-run', async () => {
530+
// `handleServiceLevelError` ("Job has stagnated") is the live path: it
531+
// drops the job and records a prompt error but never touches
532+
// `workflowStatus`, so the `running` from `handleExecutionStart`
533+
// outlives the run and no status change reports the end.
534+
//
535+
// Deliberately not the mid-run credits path — #15161 made
536+
// `handleAccountPreconditionError` clear the status, so that one ends
537+
// via the `undefined`-after-`running` branch without this watcher.
538+
await tourOnRunStep()
539+
mountRunButton('queue-button', () => {}).click()
540+
await acceptRun(TOUR_WORKFLOW)
541+
await finishRun(TOUR_WORKFLOW, 'running')
542+
543+
await removeRun()
544+
545+
expect(
546+
mocks.runState.value,
547+
'a run cut short for credits keeps its running status, so losing the job is the only signal left'
548+
).toBe('failed')
549+
})
550+
551+
it('keeps a completed run that drops out of the queue as it finishes', async () => {
552+
await tourOnRunStep()
553+
mountRunButton('queue-button', () => {}).click()
554+
await acceptRun(TOUR_WORKFLOW)
555+
556+
// `handleExecutionSuccess` reports the outcome and drops the job in one
557+
// tick, so both land before either watcher runs.
558+
void finishRun(TOUR_WORKFLOW, 'completed')
559+
await removeRun()
560+
561+
expect(
562+
mocks.runState.value,
563+
'every healthy run leaves the queue when it finishes; failing those would fail every run'
564+
).toBe('succeeded')
565+
})
566+
567+
it('keeps a failed run that leaves the queue after reporting', async () => {
568+
await tourOnRunStep()
569+
mountRunButton('queue-button', () => {}).click()
570+
await acceptRun(TOUR_WORKFLOW)
571+
await finishRun(TOUR_WORKFLOW, 'failed')
572+
573+
await removeRun()
574+
575+
expect(
576+
mocks.runState.value,
577+
'a reported outcome is the last word; losing the job afterwards says nothing new'
578+
).toBe('failed')
579+
})
580+
581+
// Pins the transition gate on the status watcher. The stagnation path
582+
// leaves `running` in `workflowStatus` forever, and that source
583+
// re-evaluates whenever the map is replaced for *any* workflow. Without
584+
// the gate the stale `running` is re-read and the card goes back to
585+
// promising a result it has already given up on.
586+
it('stays failed when an unrelated workflow churns the status map', async () => {
587+
await tourOnRunStep()
588+
mountRunButton('queue-button', () => {}).click()
589+
await acceptRun(TOUR_WORKFLOW)
590+
await finishRun(TOUR_WORKFLOW, 'running')
591+
592+
await removeRun()
593+
expect(mocks.runState.value).toBe('failed')
594+
595+
await finishRun(OTHER_WORKFLOW, 'running')
596+
597+
expect(
598+
mocks.runState.value,
599+
'another workflow starting is not this run coming back from the dead'
600+
).toBe('failed')
601+
})
602+
603+
// The other half of the stagnation path: the prompt error it records must
604+
// still be able to end the run while the stale `running` sits there.
605+
it('gives up on a stagnated job that leaves an error and a stale status', async () => {
606+
await tourOnRunStep()
607+
mountRunButton('queue-button', () => {}).click()
608+
await acceptRun(TOUR_WORKFLOW)
609+
await finishRun(TOUR_WORKFLOW, 'running')
610+
611+
mocks.executionErrors.hasPromptError = true
612+
await removeRun()
613+
614+
expect(
615+
mocks.runState.value,
616+
'a stagnated run reports an error and abandons the job; the status it leaves behind is not news'
617+
).toBe('failed')
618+
})
509619
})
510620

511621
describe('run outcome', () => {

src/renderer/extensions/firstRunTour/tour/useFirstRunTourController.ts

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,16 @@ function useFirstRunTourControllerInternal() {
7575
],
7676
([status, refused], previous) => {
7777
if (status !== undefined) stopAcceptDeadline()
78-
if (status === 'running') runState.value = 'generating'
78+
// Only an actual transition into `running` starts the wait. This source
79+
// re-evaluates whenever the `workflowStatus` map is replaced — which
80+
// `mutateStatus` does for *any* workflow — or whenever an error flag
81+
// flips. Paths that drop a job without clearing its status leave
82+
// `running` behind forever (`handleServiceLevelError` is the live one),
83+
// so an unconditional branch here would re-read that stale value and put
84+
// the card back on "your result lands right here" after the watcher
85+
// below has already failed the run.
86+
if (status === 'running' && previous?.[0] !== 'running')
87+
runState.value = 'generating'
7988
else if (status === 'completed') runState.value = 'succeeded'
8089
else if (status === 'failed') runState.value = 'failed'
8190
// A refused run never queues; a stopped one drops its status rather than
@@ -118,9 +127,27 @@ function useFirstRunTourControllerInternal() {
118127
* Acceptance is not the only disarm. `resetExecutionState` drops a job from
119128
* `queuedJobs` without clearing its status, so a run can report a status
120129
* while this reads false. A refusal produces neither signal.
130+
*
131+
* Losing acceptance is itself a signal, not a re-armed deadline. Two paths
132+
* drop the job without ever writing an outcome:
133+
*
134+
* - an accepted job that disappears with **no status written at all** — the
135+
* cloud "waiting for a machine" job that is cancelled or reconciled away
136+
* - `handleServiceLevelError` ("Job has stagnated"), which drops the job and
137+
* records a prompt error but never touches `workflowStatus`, so the
138+
* `running` written by `handleExecutionStart` outlives the run
139+
*
140+
* Not the mid-run credits path: #15161 made
141+
* `handleAccountPreconditionError` clear the status, so that one already
142+
* ends via the `undefined`-after-`running` branch above.
143+
*
144+
* A finished run leaves the queue too, but reports a terminal status in the
145+
* same flush, and the terminal branches above overwrite unconditionally — so
146+
* the outcome wins whichever watcher runs first.
121147
*/
122148
watch(tourRunAccepted, (accepted) => {
123149
if (accepted) stopAcceptDeadline()
150+
else if (runState.value === 'generating') runState.value = 'failed'
124151
})
125152

126153
let acceptTimer: ReturnType<typeof setTimeout> | undefined

0 commit comments

Comments
 (0)