Skip to content

Commit 9d1a491

Browse files
christian-byrneclaudepetros-double-test1DrJKLConnor Byrne
authored
fix(first-run-tour): fail a run the queue never accepted (#15091)
Closes #14619 — the one finding from release verification I said I'd hold the release for. It has been sitting fixed-but-unopened on a branch; this brings it onto current `main`. ## What breaks today A **paid** subscriber who is out of credits reaches the tour's Run step, clicks Run, and the tour advances to the Result step. The backend refuses the submission. The card then shows *"Hang tight — your result lands right here the moment it's ready"* **forever**: no error, no terminal state, no way forward except closing the tour. ## Why every existing guard misses it - A refused submission never gets a `prompt_id`, so **no workflow status ever appears** and none of the `running` / `completed` / `failed` branches can fire. - Account preconditions — sign-in, subscription, credits — are deliberately kept out of the error stores by `ComfyApp.queuePrompt`, so the `refused` term cannot see them either. - `canRunWorkflows` is `isActiveSubscription && (!isFreeTier || …)`, which is **true** for this user, so the paywall branch never postpones the tour. Three independent mechanisms, all structurally blind to the same case. ## The fix A deadline on **acceptance**, not on the run: ```ts const ACCEPT_DEADLINE_MS = 15_000 ``` Started when the click reports `generating`, and cleared by *any* status at all (`if (status !== undefined) stopAcceptDeadline()`). So a job that is merely slow is never cut short — only a submission the queue never acknowledged fails. ## Verification `pnpm vitest run src/renderer/extensions/firstRunTour/` — **189 passed (10 files)**. `typecheck` and `format:check` clean. Mutation-tested rather than asserted-green: | mutation | result | | --- | --- | | drop `startAcceptDeadline()` | **1 failed** / 51 passed | | drop the `status !== undefined` clear | **4 failed** / 48 passed | | unmutated | 52 passed | The second one matters: without it the deadline would fail *accepted* runs too, and four tests — including the offline-grace ones — catch that. Also fixes the `generatingRun` test helper, which never acknowledged its run and so was silently modelling a refusal; it now reports a status, matching what the queue actually does, and the unacknowledged case gets its own test. ## Scope This is the **engineering half** of #14619. Deep ruled that this population should be *skipped* — not shown a tour they cannot complete. That product decision is not implemented here and #14619's product half stays with @MaanilVerma. What this changes is that the failure is now terminal and legible instead of an indefinite promise. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: t <t@t.t> Co-authored-by: Alexander Brown <drjkl@comfy.org> Co-authored-by: Connor Byrne <c.byrne@comfy.org>
1 parent d0b659e commit 9d1a491

2 files changed

Lines changed: 285 additions & 9 deletions

File tree

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

Lines changed: 201 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,15 @@ const TOUR_WORKFLOW = { path: 'tour.json' }
1414
const OTHER_WORKFLOW = { path: 'other.json' }
1515
const INTRO_PREVIEW_MS = 500
1616
const OFFLINE_GRACE_MS = 20_000
17+
const ACCEPT_DEADLINE_MS = 15_000
1718

1819
const mocks = vi.hoisted(() => ({
1920
canRunWorkflows: { value: true },
2021
showSubscriptionDialog: vi.fn(),
2122
workflowStatus: { value: new Map<unknown, string>() },
2223
executionErrors: { hasNodeError: false, hasPromptError: false },
2324
activeWorkflow: { value: null as unknown },
25+
queuedJobs: { value: {} as Record<string, { workflow?: unknown }> },
2426
linearMode: { value: false },
2527
vueNodesEnabled: true,
2628
setSetting: vi.fn(),
@@ -46,29 +48,33 @@ vi.mock('@/composables/billing/useBillingContext', () => ({
4648
})
4749
}))
4850

51+
// Each factory runs on the first dynamic import, which lands mid-test for
52+
// whichever test runs first. Seed the new ref from the holder so that test's
53+
// setup survives instead of being discarded.
4954
vi.mock('@/stores/executionStore', async () => {
5055
const { shallowRef } = await import('vue')
51-
mocks.workflowStatus = shallowRef(new Map<unknown, string>())
56+
mocks.workflowStatus = shallowRef(new Map(mocks.workflowStatus.value))
57+
mocks.queuedJobs = shallowRef(mocks.queuedJobs.value)
5258
return {
5359
useExecutionStore: () => ({
5460
getWorkflowStatus: (workflow: unknown) =>
55-
mocks.workflowStatus.value.get(workflow)
61+
mocks.workflowStatus.value.get(workflow),
62+
get queuedJobs() {
63+
return mocks.queuedJobs.value
64+
}
5665
})
5766
}
5867
})
5968

6069
vi.mock('@/stores/executionErrorStore', async () => {
6170
const { reactive } = await import('vue')
62-
mocks.executionErrors = reactive({
63-
hasNodeError: false,
64-
hasPromptError: false
65-
})
71+
mocks.executionErrors = reactive({ ...mocks.executionErrors })
6672
return { useExecutionErrorStore: () => mocks.executionErrors }
6773
})
6874

6975
vi.mock('@/platform/workflow/management/stores/workflowStore', async () => {
7076
const { shallowRef } = await import('vue')
71-
mocks.activeWorkflow = shallowRef(null as unknown)
77+
mocks.activeWorkflow = shallowRef(mocks.activeWorkflow.value)
7278
return {
7379
useWorkflowStore: () => ({
7480
get activeWorkflow() {
@@ -80,7 +86,7 @@ vi.mock('@/platform/workflow/management/stores/workflowStore', async () => {
8086

8187
vi.mock('@/renderer/core/canvas/canvasStore', async () => {
8288
const { shallowRef } = await import('vue')
83-
mocks.linearMode = shallowRef(false)
89+
mocks.linearMode = shallowRef(mocks.linearMode.value)
8490
return {
8591
useCanvasStore: () => ({
8692
get linearMode() {
@@ -190,6 +196,18 @@ const EVERY_ENDING: { named: string; ending: TourEnding }[] = [
190196
...UNFINISHED_ENDINGS
191197
]
192198

199+
/** The queue storing a job, which is what acceptance actually looks like. */
200+
function acceptRun(workflow: unknown) {
201+
mocks.queuedJobs.value = { 'job-1': { workflow } }
202+
return nextTick()
203+
}
204+
205+
/** The queue letting a job go, which `resetExecutionState` does silently. */
206+
function removeRun() {
207+
mocks.queuedJobs.value = {}
208+
return nextTick()
209+
}
210+
193211
function finishRun(workflow: unknown, status: string) {
194212
mocks.workflowStatus.value = new Map(mocks.workflowStatus.value).set(
195213
workflow,
@@ -230,6 +248,7 @@ describe('useFirstRunTourController', () => {
230248
beforeEach(() => {
231249
mocks.canRunWorkflows = ref(true)
232250
mocks.workflowStatus.value = new Map()
251+
mocks.queuedJobs.value = {}
233252
mocks.executionErrors.hasNodeError = false
234253
mocks.executionErrors.hasPromptError = false
235254
mocks.activeWorkflow.value = null
@@ -423,14 +442,84 @@ describe('useFirstRunTourController', () => {
423442
})
424443

425444
describe('a run behind a dropped socket', () => {
445+
/**
446+
* A run the queue accepted: the click reports `generating`, then the
447+
* backend answers with a status. The acknowledgement matters — an
448+
* unacknowledged submission is a refusal, and is covered separately below.
449+
*/
426450
async function generatingRun() {
427451
await tourOnRunStep()
428452
mountRunButton('queue-button', () => {}).click()
429453
expect(mocks.runState.value).toBe('generating')
454+
await finishRun(TOUR_WORKFLOW, 'running')
430455
const { api } = await import('@/scripts/api')
431456
return api
432457
}
433458

459+
it('stops promising a result the queue never accepted', async () => {
460+
await tourOnRunStep()
461+
mountRunButton('queue-button', () => {}).click()
462+
expect(mocks.runState.value).toBe('generating')
463+
464+
// No status ever arrives. A refused submission gets no prompt_id, and
465+
// account preconditions - sign-in, subscription, credits - are kept out
466+
// of the error stores on purpose, so nothing else can report this.
467+
await vi.advanceTimersByTimeAsync(ACCEPT_DEADLINE_MS)
468+
469+
expect(
470+
mocks.runState.value,
471+
'a paid user out of credits is refused silently; the card must not promise a result forever'
472+
).toBe('failed')
473+
})
474+
475+
it('leaves a run accepted but still waiting for a machine alone', async () => {
476+
// Cloud accepts the job and reports "Waiting for a machine" — it is in
477+
// `initializingJobIds` with NO workflow status until a worker picks it
478+
// up, which routinely outlasts the deadline. Keying on status instead of
479+
// acceptance would fail this healthy run and tell the user to run again,
480+
// prompting a duplicate paid submission.
481+
await tourOnRunStep()
482+
mountRunButton('queue-button', () => {}).click()
483+
await acceptRun(TOUR_WORKFLOW)
484+
485+
await vi.advanceTimersByTimeAsync(ACCEPT_DEADLINE_MS * 4)
486+
487+
expect(
488+
mocks.runState.value,
489+
'an accepted job with no status yet is queued, not refused'
490+
).toBe('generating')
491+
})
492+
493+
it('lets the offline grace outlive the acceptance deadline', async () => {
494+
// The grace is 20s and the acceptance deadline 15s. A drop before the
495+
// first status must still get the full grace: acceptance arrives on the
496+
// queuePrompt response, not the socket, so it disarms this deadline even
497+
// while the connection is down.
498+
const { api } = await import('@/scripts/api')
499+
await tourOnRunStep()
500+
mountRunButton('queue-button', () => {}).click()
501+
await acceptRun(TOUR_WORKFLOW)
502+
503+
api.dispatchCustomEvent('reconnecting')
504+
await vi.advanceTimersByTimeAsync(OFFLINE_GRACE_MS - 1)
505+
506+
expect(
507+
mocks.runState.value,
508+
'the 15s acceptance deadline must not cut the 20s grace short'
509+
).toBe('generating')
510+
})
511+
512+
it('leaves an accepted run past the acceptance deadline alone', async () => {
513+
await generatingRun()
514+
515+
await vi.advanceTimersByTimeAsync(ACCEPT_DEADLINE_MS * 4)
516+
517+
expect(
518+
mocks.runState.value,
519+
'the deadline is on acceptance, not on the run: a job that answered must never be cut short'
520+
).toBe('generating')
521+
})
522+
434523
it('stops promising a result once the socket stays gone', async () => {
435524
const api = await generatingRun()
436525

@@ -498,6 +587,110 @@ describe('useFirstRunTourController', () => {
498587
'the grace timer must not clobber an outcome that arrived before it fired'
499588
).toBe('succeeded')
500589
})
590+
591+
it('stops promising a result once the queue lets go of its job', async () => {
592+
await tourOnRunStep()
593+
mountRunButton('queue-button', () => {}).click()
594+
await acceptRun(TOUR_WORKFLOW)
595+
596+
await removeRun()
597+
598+
expect(
599+
mocks.runState.value,
600+
'an accepted job that leaves without an outcome leaves the card waiting on a result nobody will send'
601+
).toBe('failed')
602+
})
603+
604+
it('stops promising a result when a running job is dropped mid-run', async () => {
605+
// `handleServiceLevelError` ("Job has stagnated") is the live path: it
606+
// drops the job and records a prompt error but never touches
607+
// `workflowStatus`, so the `running` from `handleExecutionStart`
608+
// outlives the run and no status change reports the end.
609+
//
610+
// Deliberately not the mid-run credits path — #15161 made
611+
// `handleAccountPreconditionError` clear the status, so that one ends
612+
// via the `undefined`-after-`running` branch without this watcher.
613+
await tourOnRunStep()
614+
mountRunButton('queue-button', () => {}).click()
615+
await acceptRun(TOUR_WORKFLOW)
616+
await finishRun(TOUR_WORKFLOW, 'running')
617+
618+
await removeRun()
619+
620+
expect(
621+
mocks.runState.value,
622+
'a run cut short for credits keeps its running status, so losing the job is the only signal left'
623+
).toBe('failed')
624+
})
625+
626+
it('keeps a completed run that drops out of the queue as it finishes', async () => {
627+
await tourOnRunStep()
628+
mountRunButton('queue-button', () => {}).click()
629+
await acceptRun(TOUR_WORKFLOW)
630+
631+
// `handleExecutionSuccess` reports the outcome and drops the job in one
632+
// tick, so both land before either watcher runs.
633+
void finishRun(TOUR_WORKFLOW, 'completed')
634+
await removeRun()
635+
636+
expect(
637+
mocks.runState.value,
638+
'every healthy run leaves the queue when it finishes; failing those would fail every run'
639+
).toBe('succeeded')
640+
})
641+
642+
it('keeps a failed run that leaves the queue after reporting', async () => {
643+
await tourOnRunStep()
644+
mountRunButton('queue-button', () => {}).click()
645+
await acceptRun(TOUR_WORKFLOW)
646+
await finishRun(TOUR_WORKFLOW, 'failed')
647+
648+
await removeRun()
649+
650+
expect(
651+
mocks.runState.value,
652+
'a reported outcome is the last word; losing the job afterwards says nothing new'
653+
).toBe('failed')
654+
})
655+
656+
// Pins the transition gate on the status watcher. The stagnation path
657+
// leaves `running` in `workflowStatus` forever, and that source
658+
// re-evaluates whenever the map is replaced for *any* workflow. Without
659+
// the gate the stale `running` is re-read and the card goes back to
660+
// promising a result it has already given up on.
661+
it('stays failed when an unrelated workflow churns the status map', async () => {
662+
await tourOnRunStep()
663+
mountRunButton('queue-button', () => {}).click()
664+
await acceptRun(TOUR_WORKFLOW)
665+
await finishRun(TOUR_WORKFLOW, 'running')
666+
667+
await removeRun()
668+
expect(mocks.runState.value).toBe('failed')
669+
670+
await finishRun(OTHER_WORKFLOW, 'running')
671+
672+
expect(
673+
mocks.runState.value,
674+
'another workflow starting is not this run coming back from the dead'
675+
).toBe('failed')
676+
})
677+
678+
// The other half of the stagnation path: the prompt error it records must
679+
// still be able to end the run while the stale `running` sits there.
680+
it('gives up on a stagnated job that leaves an error and a stale status', async () => {
681+
await tourOnRunStep()
682+
mountRunButton('queue-button', () => {}).click()
683+
await acceptRun(TOUR_WORKFLOW)
684+
await finishRun(TOUR_WORKFLOW, 'running')
685+
686+
mocks.executionErrors.hasPromptError = true
687+
await removeRun()
688+
689+
expect(
690+
mocks.runState.value,
691+
'a stagnated run reports an error and abandons the job; the status it leaves behind is not news'
692+
).toBe('failed')
693+
})
501694
})
502695

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

0 commit comments

Comments
 (0)