fix(first-run-tour): stop promising a result once acceptance is lost - #15172
Conversation
A job dropped from queuedJobs without a terminal status left runState on 'generating' forever. resetExecutionState clears the job and the workflow mapping but not the status, so handleAccountPreconditionError - the mid-run INSUFFICIENT_CREDITS path - leaves the card promising a result that never arrives. Fail the run when acceptance goes true -> false while still generating. Gated on 'generating' so the queue drop every healthy run makes on its way out stays harmless: a finished run writes its terminal status in the same tick, and the status watcher is registered first.
🎭 Playwright: ✅ 1802 passed, 0 failed · 3 flaky📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle Size
⚡ Performance Report
Absolute values
Raw data{
"timestamp": "2026-08-17T17:01:44.546Z",
"gitSha": "7d5e23919582c07fe87a24b7e5df0c7ff41e143b",
"branch": "fix/tour-acceptance-loss-watch",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2089.179999999999,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.32,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 390.14,
"heapDeltaBytes": 5931676,
"heapUsedBytes": 70271400,
"domNodes": 18,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 17.535000000000004,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2026.7880000000105,
"styleRecalcs": 9,
"styleRecalcDurationMs": 6.181000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 458.92999999999995,
"heapDeltaBytes": 5907588,
"heapUsedBytes": 70701324,
"domNodes": 18,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 14.950999999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 2072.938999999991,
"styleRecalcs": 80,
"styleRecalcDurationMs": 48.679,
"layouts": 12,
"layoutDurationMs": 5.010000000000001,
"taskDurationMs": 1066.103,
"heapDeltaBytes": -12653552,
"heapUsedBytes": 51758644,
"domNodes": -280,
"jsHeapTotalBytes": 24088576,
"scriptDurationMs": 134.015,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1819.7299999999927,
"styleRecalcs": 75,
"styleRecalcDurationMs": 32.404,
"layouts": 12,
"layoutDurationMs": 3.379,
"taskDurationMs": 866.84,
"heapDeltaBytes": -14443880,
"heapUsedBytes": 50235276,
"domNodes": -282,
"jsHeapTotalBytes": 23564288,
"scriptDurationMs": 111.54599999999999,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1737.8209999999967,
"styleRecalcs": 32,
"styleRecalcDurationMs": 16.381,
"layouts": 6,
"layoutDurationMs": 0.7210000000000001,
"taskDurationMs": 375.116,
"heapDeltaBytes": 8778208,
"heapUsedBytes": 73400024,
"domNodes": 79,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 18.342999999999996,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1719.7949999999764,
"styleRecalcs": 32,
"styleRecalcDurationMs": 15.450000000000001,
"layouts": 6,
"layoutDurationMs": 0.7360000000000001,
"taskDurationMs": 373.19800000000004,
"heapDeltaBytes": 8767268,
"heapUsedBytes": 73371456,
"domNodes": 77,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 18.33,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 624.9990000000025,
"styleRecalcs": 13,
"styleRecalcDurationMs": 8.662,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 385.964,
"heapDeltaBytes": -11283432,
"heapUsedBytes": 53120320,
"domNodes": 22,
"jsHeapTotalBytes": 25690112,
"scriptDurationMs": 57.861999999999995,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 577.8889999999706,
"styleRecalcs": 11,
"styleRecalcDurationMs": 7.2940000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 376.00500000000005,
"heapDeltaBytes": -11251380,
"heapUsedBytes": 53136696,
"domNodes": 18,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 55.93100000000001,
"eventListeners": 0,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2025.9629999999902,
"styleRecalcs": 9,
"styleRecalcDurationMs": 6.98,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 656.949,
"heapDeltaBytes": 6908572,
"heapUsedBytes": 66431252,
"domNodes": -281,
"jsHeapTotalBytes": 3506176,
"scriptDurationMs": 90.13300000000002,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2024.8189999999795,
"styleRecalcs": 9,
"styleRecalcDurationMs": 6.938,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 672.0410000000002,
"heapDeltaBytes": 4450260,
"heapUsedBytes": 63962068,
"domNodes": -281,
"jsHeapTotalBytes": 3244032,
"scriptDurationMs": 92.13000000000001,
"eventListeners": -179,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-pan",
"durationMs": 2146.724000000006,
"styleRecalcs": 68,
"styleRecalcDurationMs": 14.186,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1233.6529999999998,
"heapDeltaBytes": 18663808,
"heapUsedBytes": 79712432,
"domNodes": -282,
"jsHeapTotalBytes": 2912256,
"scriptDurationMs": 403.126,
"eventListeners": -177,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2152.0649999999932,
"styleRecalcs": 68,
"styleRecalcDurationMs": 14.039000000000003,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1233.781,
"heapDeltaBytes": -5428480,
"heapUsedBytes": 55892324,
"domNodes": -285,
"jsHeapTotalBytes": 5603328,
"scriptDurationMs": 404.598,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3194.626000000028,
"styleRecalcs": 66,
"styleRecalcDurationMs": 16.242,
"layouts": 60,
"layoutDurationMs": 8.204,
"taskDurationMs": 1484.9460000000001,
"heapDeltaBytes": -2566824,
"heapUsedBytes": 59563568,
"domNodes": -287,
"jsHeapTotalBytes": 5865472,
"scriptDurationMs": 517.0749999999999,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3178.9210000000594,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.270000000000003,
"layouts": 60,
"layoutDurationMs": 8.046,
"taskDurationMs": 1507.566,
"heapDeltaBytes": -3844236,
"heapUsedBytes": 58313980,
"domNodes": -288,
"jsHeapTotalBytes": 6651904,
"scriptDurationMs": 514.421,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2029.504999999972,
"styleRecalcs": 8,
"styleRecalcDurationMs": 5.9430000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 668.7539999999999,
"heapDeltaBytes": 6801504,
"heapUsedBytes": 67616792,
"domNodes": -284,
"jsHeapTotalBytes": 3768320,
"scriptDurationMs": 94.19100000000002,
"eventListeners": -149,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2036.8400000000975,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.512999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 663.2950000000001,
"heapDeltaBytes": 6375760,
"heapUsedBytes": 66968216,
"domNodes": -284,
"jsHeapTotalBytes": 4554752,
"scriptDurationMs": 92.72899999999998,
"eventListeners": -177,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 598.6409999999864,
"styleRecalcs": 47,
"styleRecalcDurationMs": 10.408000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 413.271,
"heapDeltaBytes": -10640248,
"heapUsedBytes": 53856356,
"domNodes": 20,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 129.423,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 592.8099999999858,
"styleRecalcs": 48,
"styleRecalcDurationMs": 11.813,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 409.837,
"heapDeltaBytes": -10974268,
"heapUsedBytes": 53525964,
"domNodes": 22,
"jsHeapTotalBytes": 25952256,
"scriptDurationMs": 125.12200000000001,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-idle",
"durationMs": 2039.3229999999676,
"styleRecalcs": 10,
"styleRecalcDurationMs": 7.826999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 452.37,
"heapDeltaBytes": 5983104,
"heapUsedBytes": 70457832,
"domNodes": 20,
"jsHeapTotalBytes": 24379392,
"scriptDurationMs": 13.873999999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-idle",
"durationMs": 2015.8270000000016,
"styleRecalcs": 10,
"styleRecalcDurationMs": 6.860999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 484.079,
"heapDeltaBytes": 5915708,
"heapUsedBytes": 70403948,
"domNodes": 20,
"jsHeapTotalBytes": 24641536,
"scriptDurationMs": 12.973,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1702.150999999958,
"styleRecalcs": 76,
"styleRecalcDurationMs": 34.809,
"layouts": 16,
"layoutDurationMs": 4.076,
"taskDurationMs": 751.346,
"heapDeltaBytes": -3762592,
"heapUsedBytes": 60811260,
"domNodes": 64,
"jsHeapTotalBytes": 26214400,
"scriptDurationMs": 83.658,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1741.2419999999997,
"styleRecalcs": 79,
"styleRecalcDurationMs": 33.892,
"layouts": 16,
"layoutDurationMs": 4.199,
"taskDurationMs": 776.097,
"heapDeltaBytes": -17889448,
"heapUsedBytes": 46547356,
"domNodes": -280,
"jsHeapTotalBytes": 23564288,
"scriptDurationMs": 84.066,
"eventListeners": -153,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-transition-enter",
"durationMs": 1400.8360000000266,
"styleRecalcs": 18,
"styleRecalcDurationMs": 28.711000000000002,
"layouts": 14,
"layoutDurationMs": 11.418000000000001,
"taskDurationMs": 899.5830000000002,
"heapDeltaBytes": 31038012,
"heapUsedBytes": 99351972,
"domNodes": 13673,
"jsHeapTotalBytes": 15990784,
"scriptDurationMs": 35.911000000000016,
"eventListeners": 2375,
"totalBlockingTimeMs": 124,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "viewport-pan-sweep",
"durationMs": 8239.688000000002,
"styleRecalcs": 250,
"styleRecalcDurationMs": 39.584,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4354.425,
"heapDeltaBytes": 11819724,
"heapUsedBytes": 71481196,
"domNodes": -282,
"jsHeapTotalBytes": 6320128,
"scriptDurationMs": 1312.156,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "viewport-pan-sweep",
"durationMs": 8225.202999999965,
"styleRecalcs": 249,
"styleRecalcDurationMs": 40.539,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4662.804999999999,
"heapDeltaBytes": 16950104,
"heapUsedBytes": 76835672,
"domNodes": -283,
"jsHeapTotalBytes": 10514432,
"scriptDurationMs": 1572.792,
"eventListeners": -133,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 13436.350000000004,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 13399.739,
"heapDeltaBytes": -55990768,
"heapUsedBytes": 170540916,
"domNodes": -8312,
"jsHeapTotalBytes": -2826240,
"scriptDurationMs": 601.7009999999999,
"eventListeners": -16391,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 17505.62000000002,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17492.032,
"heapDeltaBytes": -36868348,
"heapUsedBytes": 165880960,
"domNodes": -8312,
"jsHeapTotalBytes": -10424320,
"scriptDurationMs": 566.63,
"eventListeners": -16389,
"totalBlockingTimeMs": 6,
"frameDurationMs": 17.77333333333336,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 21966.953999999987,
"styleRecalcs": 158,
"styleRecalcDurationMs": 22.05300000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 21932.564999999995,
"heapDeltaBytes": -51228692,
"heapUsedBytes": 168451752,
"domNodes": -8312,
"jsHeapTotalBytes": -7802880,
"scriptDurationMs": 914.269,
"eventListeners": -16379,
"totalBlockingTimeMs": 460,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 22372.145000000048,
"styleRecalcs": 165,
"styleRecalcDurationMs": 22.46799999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 22321.267,
"heapDeltaBytes": -28968816,
"heapUsedBytes": 177330288,
"domNodes": -8312,
"jsHeapTotalBytes": -8138752,
"scriptDurationMs": 981.3320000000001,
"eventListeners": -16385,
"totalBlockingTimeMs": 453,
"frameDurationMs": 17.77333333333336,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 109.4229999999925,
"styleRecalcs": 8,
"styleRecalcDurationMs": 14.016999999999998,
"layouts": 2,
"layoutDurationMs": 0.6529999999999999,
"taskDurationMs": 75.179,
"heapDeltaBytes": 3085896,
"heapUsedBytes": 66620660,
"domNodes": 111,
"jsHeapTotalBytes": 3407872,
"scriptDurationMs": 5.812000000000001,
"eventListeners": 49,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 516.2970000000087,
"styleRecalcs": 18,
"styleRecalcDurationMs": 26.526999999999997,
"layouts": 4,
"layoutDurationMs": 1.802,
"taskDurationMs": 138.357,
"heapDeltaBytes": 5180972,
"heapUsedBytes": 68933616,
"domNodes": 147,
"jsHeapTotalBytes": 5242880,
"scriptDurationMs": 10.829,
"eventListeners": 97,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
}
]
} |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…loss The note claimed the status watcher wins because it is registered first. Verified otherwise: moving it below the acceptance watch leaves all 58 tests green, because it applies the terminal status from the same flush either way. Order is not what makes this safe, so stop implying a reordering would break it. Addresses review feedback: #15091 (comment)
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## fix/14619-tour-unaccepted-run #15172 +/- ##
===============================================================
Coverage 81.34% 81.34%
===============================================================
Files 1882 1882
Lines 107863 107865 +2
Branches 33860 33390 -470
===============================================================
+ Hits 87740 87742 +2
Misses 19724 19724
Partials 399 399
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
benjcooley
left a comment
There was a problem hiding this comment.
Reviewed the one-line watcher change against executionStore (resetExecutionState, handleExecutionSuccess/Error, handleAccountPreconditionError, handleServiceLevelError, flushPendingWorkflowStatus) and the two watchers' flush ordering.
Verdict: the fix is sound and safe. The 'generating' gate holds on every real path I traced, and because the status watcher applies terminal statuses unconditionally, an early 'failed' self-corrects if an outcome lands in a later flush. No blockers.
| Severity | Count |
|---|---|
| BLOCKER | 0 |
| SHOULD FIX | 3 |
| TRIVIAL | 1 |
What I'd want addressed:
- The stale-
runningentry can undo this line. Nothing clearsworkflowStatuson the stagnation path, and the status watcher'sstatus === 'running' → 'generating'branch is unconditional and re-runs on any workflow's status change (mutateStatusreplaces the whole map) or any error-flag flip. One extra condition on that branch closes it. - The registration order is load-bearing and untested. In the stale-
runningcase this watcher must run after the status watcher. Test 3 passes with either order, so nothing pins it. - The stated motivation is stale. #15161 already clears the status on
handleAccountPreconditionError, so the mid-run credits variant the description leads with is covered onmainby the existingundefined-after-runningbranch. The hole this actually closes is (a) an accepted job dropped with no status ever written — the cloud "waiting for a machine" job that gets cancelled or reconciled away, which is genuinely new and worth having — and (b)handleServiceLevelError, which still leavesrunningbehind. Worth correcting the description so the next reader doesn't re-derive it.
Nice work on the gate itself — pinning it with the same-tick completed case (test 3) is the right instinct, and tourRunAccepted keying on the queue rather than the status remains the correct call for cloud allocation.
| */ | ||
| watch(tourRunAccepted, (accepted) => { | ||
| if (accepted) stopAcceptDeadline() | ||
| else if (runState.value === 'generating') runState.value = 'failed' |
There was a problem hiding this comment.
SHOULD FIX — the status watcher can undo this afterwards.
On the paths that drop a job without clearing its status, workflowStatus keeps a 'running' entry for the tour workflow forever. handleServiceLevelError ("Job has stagnated") is the live example: it writes the terminal status with showStatus: false (so applyWorkflowStatus is skipped), then calls resetExecutionState, which does not touch workflowStatus.
The status watcher's first branch is unconditional:
if (status === 'running') runState.value = 'generating'and its source getter re-evaluates whenever the workflowStatus shallowRef is replaced — mutateStatus swaps the whole map, so any workflow's status change re-runs it, not just the tour's — or whenever hasNodeError / hasPromptError flips (clearAllErrors() at the top of the next queuePrompt, clearExecutionStartErrors() on any execution_start). Each of those re-reads the stale 'running' and puts the card back on "Hang tight, your result lands right here" after this line already failed it.
One-line fix in the status watcher — only enter generating on an actual transition:
if (status === 'running' && previous?.[0] !== 'running')
runState.value = 'generating'| * `resetExecutionState` drops the job without writing an outcome on the | ||
| * mid-run credits path. A finished run leaves the queue too, but reports a | ||
| * terminal status in the same flush, which the watch above applies regardless | ||
| * of which of the two runs first. |
There was a problem hiding this comment.
SHOULD FIX — "regardless of which of the two runs first" is not true in the case this PR is actually for, and nothing tests the order.
It holds for completed/failed, because those branches overwrite unconditionally. It does not hold for the stale-running drop: there the status watcher takes status === 'running' → 'generating', so this watcher must run after it or the card stays promising. That works today only because this watch is registered second.
Test 3 doesn't pin it — with the two watchers swapped it still ends 'succeeded', since the terminal branch is unconditional. A test that would: status stays 'running', hasPromptError = true, and the job removed in the same tick → expect 'failed'. That is the stagnation path, and it is the only place the registration order is load-bearing.
| it('stops promising a result when a running job is dropped mid-run', async () => { | ||
| // An API node that charges credits mid-run lands in | ||
| // `handleAccountPreconditionError`, which drops the job but leaves the | ||
| // stale `running` status behind, so no status change reports the end. |
There was a problem hiding this comment.
SHOULD FIX — wrong handler; this comment is already false on main.
#15161 (merged) added clearWorkflowStatus to handleAccountPreconditionError, so the credits path no longer leaves a stale running. It now goes running → undefined, and the existing status === undefined && previous?.[0] === 'running' branch already fails the run without this PR.
The scenario the test covers is still real, but via handleServiceLevelError ("Job has stagnated"): it sets the status with showStatus: false and never clears it, so running survives the drop. Retarget the comment — and the same claim in the PR description — or it reads as false the moment this rebases onto main.
| ).toBe('succeeded') | ||
| }) | ||
|
|
||
| it('keeps a failed run that leaves the queue after reporting', async () => { |
There was a problem hiding this comment.
TRIVIAL — this test cannot fail on the behaviour it claims to pin.
The description says the last two tests "both fail against an ungated else runState.value = 'failed' variant". This one doesn't: ungated, the removal writes 'failed' over a 'failed' and the assertion still passes. Only test 3 (completed) discriminates. Either drop it or reword the claim.
benjcooley
left a comment
There was a problem hiding this comment.
Please address should fixes. But no blockers.
…nning Addresses @benjcooley's three SHOULD FIX items. The status watcher's `status === 'running'` branch was unconditional, and its source re-evaluates whenever the `workflowStatus` map is replaced — which `mutateStatus` does for *any* workflow — or whenever an error flag flips. On paths that drop a job without clearing its status the stale `running` survives, so each of those re-evaluations put the card back on "your result lands right here" after the acceptance-loss watcher had already failed the run. Gated on an actual transition. The comments named the wrong handler. #15161 made `handleAccountPreconditionError` clear the status, so the mid-run credits path now ends via the existing `undefined`-after-`running` branch. The paths this watcher actually covers are an accepted job dropped with no status ever written, and `handleServiceLevelError`, which drops the job and records a prompt error but never touches `workflowStatus`. The gate also removes the registration-order dependency the old comment overclaimed away: with it, the terminal branches are the only ones that write, and they overwrite unconditionally. Tests: 60 green. `stays failed when an unrelated workflow churns the status map` fails without the gate (`expected 'generating' to be 'failed'`).
|
All four addressed in 61bd7dd. Thanks — item 1 is a real bug I would not have found, and item 3 is the second time in this stack I have shipped a description that stopped being true underneath me. 1. SHOULD FIX — the status watcher can undo this. Fixed, and you are right about the mechanism: if (status === 'running' && previous?.[0] !== 'running')
runState.value = 'generating'Pinned by a new test, 2. SHOULD FIX — registration order is load-bearing and untested. The gate in item 1 dissolves the dependency rather than pinning it: once the 3. SHOULD FIX — the stated motivation is stale. Corrected in the description, the test comment, and the controller comment. You are right that #15161 covers the credits path via the existing One correction to your description of the mechanism, which does not change the conclusion: 4. TRIVIAL — the 60 tests green, Still stacked on #15091, which is blocked on @MaanilVerma's changes-requested, so this cannot merge ahead of it. |
70704d9
into
fix/14619-tour-unaccepted-run
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:
handleServiceLevelError("Job has stagnated") — drops the job, records a prompt error, never touchesworkflowStatusrunningrunningshadowed the error branchmid-run credits (handleAccountPreconditionError)stalerunningundefined-after-runningbranchIn the uncovered cases
runStatesits ongeneratingforever 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
tourRunAcceptedwatcher so true -> false fails the run when it is stillgenerating.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.handleExecutionSuccessandhandleExecutionErrorwrite the terminal status and callresetExecutionStatein the same synchronous handler, and the status watcher is registered before this one, so a finished run has already leftgeneratingby the time this fires. Test 3 below covers exactly that tick.Tests, in the existing
'a run behind a dropped socket'describe:failedrunning, removed (the stagnation case) ->failedcompletedwritten in the same tick as the removal -> stayssucceededfailed, then removed -> staysfailedThe first two fail on the base branch (
expected 'generating' to be 'failed'). Of the last two terminal-status guards, only thecompletedone discriminates — ungated, thefailedcase 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 theworkflowStatusmap is replaced (mutateStatusswaps the whole map, so any workflow's change re-runs it) or an error flag flips. With a stalerunningsitting 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 intorunning, 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
useFirstRunTourController.test.ts(54 existing + 4 new), full-file and each new test in isolation.pnpm lint,pnpm typecheck,pnpm format:checkpass.