Comfy-Org / Comfy-Org/ComfyUI_frontend

Stale 'running' workflow status after service-level and cloud-validation execution errors

Open
#15,171 1 comment 1 reaction 1 assignee Claimed by @benjcooley View on GitHub
area:queue Potential Bug
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.