Comfy-Org / Comfy-Org/ComfyUI_frontend
Stale 'running' workflow status after service-level and cloud-validation execution errors
- Dominant language
- TypeScript
- Stars
- 2k
- Forks
- 704
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 512
Description
Two `execution_error` early-return branches in `src/stores/executionStore.ts` leave a stale `'running'` workflow status behind after the run has ended. Anything reading `getWorkflowStatus` then shows a run that is still going when it is already dead.
This is the same defect class as the account-precondition case fixed in #15161. That PR deliberately changed only the one handler, because the other two have an existing test pinning the current behavior and changing them is a product call rather than a cleanup.
### Mechanism
`handleExecutionError` dispatches to three early-return handlers. Each calls `clearInitializationByJobId` then `resetExecutionState`, and `resetExecutionState` (around `:795-806`) deletes `queuedJobs[jobId]` and the `jobIdToWorkflow` entry but never touches `workflowStatus`:
| Handler | Clears `workflowStatus`? |
| -------------------------------- | ------------------------ |
| `handleAccountPreconditionError` | yes, as of #15161 |
| `handleServiceLevelError` | no |
| `handleCloudValidationError` | no |
`handleExecutionInterrupted` (`:492-493`) is the precedent for the fix:
```ts
const workflow = jobIdToWorkflow.get(jobId)
if (workflow) clearWorkflowStatus(workflow)
```
Ordering matters: the workflow has to be read out of `jobIdToWorkflow` before `resetExecutionState` deletes the entry.
### The decision needed
`src/stores/executionStore.test.ts:900`, `tracks service-level failures without writing failed`, asserts:
```ts
expect(store.getWorkflowStatus(workflowA)).toBe('running')
```
after a `StagnationError` ("Job has stagnated").
The test's name is about not writing `failed`, and the `trackExecutionOutcome` assertion is clearly its point. Whether the surviving `'running'` is a deliberate guarantee or an unexamined side effect that got pinned is not recoverable from the test itself. A stagnated job is terminal, so leaving the card at "running" indefinitely looks like the same bug, but that needs confirming rather than assuming.
Same question for `handleCloudValidationError`, which has no equivalent pinned assertion.
### Asks
- Confirm whether the `'running'` assertion at `executionStore.test.ts:900` is intentional.
- If not, apply the `handleExecutionInterrupted` shape to both remaining handlers and update that assertion.
- If it is intentional, document why on the handler so the next person does not read it as an oversight.
Found while addressing review feedback on #15091. Related: #15161.
Contributor guide
Assessment
This issue has not been assessed yet.