fix(workflow): a start that did not happen must not answer 200 (stacked on #71)

[rimusz]

Important

Stacked on #71 — merge that first.

This branch is cut from #71, so the diff shows its two commits as well. Only 835367f9 belongs to this PR. Once #71 lands, this reduces to that single commit; if #71 is revised, I will rebase.

It depends on the WorkflowProcessUnavailableError class and the isWorkflowProcessUnavailable predicate introduced there. Building it on main would have meant inventing both twice.

What

activateWorkflow and deactivateWorkflow answered every IPC failure with return { error: error.message }. The route handlers pass the result straight to res.json(result) — and res.json with no status is 200.

So a start that never happened answered success, with an error-shaped body.

Why it mattered

Every caller gates on the status code, not the body:

caller	check
WorkflowEngine.vue:268	if (!response.ok) throw
Workflows.vue:910,933	if (!response.ok) throw
agnt.js SDK	axios — rejects on non-2xx

response.ok was true, so none of those failure branches ran. The UI reported the workflow started. Nothing was armed, no trigger was listening, and the only trace was a console.error in the backend log.

This is the same defect #71 fixed for fetchWorkflowState, in a worse place. That one corrupted a status read. These corrupt the Start and Stop buttons — the two actions where "did it work?" is the entire question.

How

• Both bridge methods log and rethrow, matching fetchWorkflowState and restartActiveWorkflows in the same class. All three IPC methods now report an unreachable process the same way — a test asserts exactly that, since the class previously answered the same failure two different ways depending on which method you called.
• Both route handlers answer 503 with the reason when the process cannot be reached, and 500 carrying the child's diagnosis when the process answered with a failure. Identical shape to fix(workflow): an unreachable workflow process is not a workflow state #71's /status handler.
• deleteWorkflow isolates its deactivate call. It runs before the row delete, so letting the new throw escape would have made a workflow undeletable while the workflow process was down. The row goes either way, exactly as before — what changes is that the reason is logged rather than discarded.

It also resurrects dead code

The retry around the post-save reactivate has never been able to fire:

try {
  await WorkflowProcessBridge.activateWorkflow(updatedWorkflow, userId);
} catch (reactivateError) {                    // ← unreachable: activate never threw
  console.error('Failed to reactivate workflow after save:', reactivateError.message);
  try {
    await WorkflowProcessBridge.activateWorkflow(updatedWorkflow, userId);
  } catch (restoreError) {                     // ← also unreachable
    console.error('Could not restore workflow active state:', restoreError.message);
  }
}

Same pattern as the not ready branch found in #71 — handling written for an error that structurally could not arrive. Both are live now, and covered by tests.

Worth noting the interaction with #71: when the process is unreachable, fetchWorkflowState now throws first and the outer catch skips the restart check entirely, so these retries are only reached when the process is up and genuinely refused. No added latency against a down process.

Testing

26 tests across 3 files. 12 fail without this change.

The third file is the one that matters most. The other two test each half in isolation — the bridge throws, the handler maps a throw to 503/500 — but neither reproduces the actual defect, which only existed where the halves met:

bridge swallows -> returns { error: message } -> res.json(result) -> 200

A route test with a mocked bridge cannot see that, and a bridge test has no res to inspect. So WorkflowService.startStopIntegration.test.js mocks the models, database and realtime layer and deliberately leaves WorkflowProcessBridge real. Against the base commit it fails with:

AssertionError: expected 200 not to be 200
AssertionError: expected true to be false      ← error-shaped body with a success status

Mutation tested — 8 mutants, 8 killed, 0 survived:

mutant	result
restore the activate swallow	killed (6)
restore the deactivate swallow	killed (3)
drop the 503 branch on start	killed (3)
drop the 503 branch on stop	killed (2)
let the delete deactivate failure escape	killed (3)
stop logging why the delete could not confirm	killed (1)
stop forwarding the diagnosis on a start failure	killed (1)
drop reason from the 503 body	killed (2)

Full backend suite: 295 passed / 295 files, zero failures. (The UpdateScheduler.status flake noted on #70 and #71 did not fire on this run — further evidence it is order-dependent rather than caused by any of these changes.)

Notes for the reviewer

• This is a behaviour change at the API boundary. POST /workflows/:id/start and /stop can now answer 503 or 500 where they previously answered 200. That is the point — those 200s were false — but it deserves a deliberate nod. All three known callers already handle non-2xx correctly, so no client change is needed.
• A product question I deliberately did not decide: should deleting a workflow fail when its triggers cannot be confirmed stopped? Today it proceeds, and I preserved that. The argument for failing is orphaned live triggers; the argument against is a workflow you cannot remove while the process is down. Worth a separate discussion rather than a silent change here.
• activateWorkflow is also called from the two restart paths in saveWorkflow/updateWorkflow. Both already sat inside try/catch, so their control flow is unchanged — only their error messages improve.

[Copilot]

🟡 Changes recommended

activateWorkflow can still resolve { error: ... } from the child (e.g., “already queued/running”), which will again flow to a 200 response unless treated as a thrown failure.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Aligns workflow Start/Stop (and Status) API behavior with actual IPC outcomes by distinguishing an unreachable workflow process from child-reported failures, ensuring callers don’t treat failures as successes due to always-200 JSON responses.

Changes:

• Make WorkflowProcessBridge.activateWorkflow / deactivateWorkflow / fetchWorkflowState log and rethrow IPC failures (instead of returning error-shaped objects).
• Map “workflow process unreachable” to 503 (with reason) and child-reported failures to 500 in WorkflowService start/stop/status handlers; keep “not-ready” as 200 initializing for /status.
• Add targeted unit + integration tests to reproduce the prior “200 with error-shaped body” regression and pin the new contracts.

File summaries

File	Description
backend/src/workflow/WorkflowProcessBridge.js	Introduces typed “process unavailable” error + predicate; switches IPC helpers to log-and-rethrow for start/stop/status.
backend/src/services/WorkflowService.js	Updates route handlers to return 503/500 appropriately for start/stop/status and improves logging; isolates delete from stop failures.
backend/src/workflow/WorkflowProcessBridge.unavailable.test.js	Tests typed transport failures vs child-reported failures for sendMessage/fetchWorkflowState.
backend/src/workflow/WorkflowProcessBridge.startStop.test.js	Tests start/stop throw on unreachable process and preserve child diagnosis.
backend/src/services/WorkflowService.fetchWorkflowState.test.js	Tests /status handler’s 200 initializing vs 503 unavailable vs 500 diagnosis behavior.
backend/src/services/WorkflowService.startStop.test.js	Tests /start and /stop handler status mapping and that delete remains successful when stop can’t be confirmed.
backend/src/services/WorkflowService.startStopIntegration.test.js	End-to-end reproduction test ensuring start/stop don’t answer 200 when the bridge can’t reach the process.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rimusz]

Good catch, and it was a real hole in the contract this PR's title states — thank you. Verified before changing anything, and there are two such paths, not one:

// ProcessManager.activateWorkflow
return { error: 'Workflow is already queued or running', workflowId };  // early return
return { error: 'Failed to enqueue workflow', workflowId };             // catch block

Both resolve, so neither reaches the catch block I fixed, and res.json(result) sent both as 200. Making the method throw covered the transport failures and missed these entirely.

Fixed in aaa83606. One deviation from your suggestion, deliberately: I did not throw on a resolved { error }, because the two cases are not the same kind of event. "Already queued or running" is a refusal in the current state with nothing broken; "failed to enqueue" is a real failure. Collapsing both into a 500 would report a healthy, already-running workflow as a server error.

So the handler inspects the payload and maps it — but on a code, not the message text, since matching on English is the coupling the previous commit removed and not worth reintroducing one layer up:

result	status
code: 'ALREADY_ACTIVE'	409
any other code	500
no code (older shape)	500

ProcessManager now stamps those codes. deactivateWorkflow has no equivalent path — it only ever returns { message, isActive } — verified rather than assumed, so no guard was added there.

Six tests cover it, all failing without the guard. Mutation testing on the new code also caught a weakness in my own test: the "discriminates on the code, not the text" case fed the handler "this workflow is already up and running", which still contained the keyword — so a text-matching mutant passed it and the test proved nothing. Reworded to share no keyword, plus the inverse case, in 60b91ff2. 11 mutants, 11 killed.

Worth adding: the underlying design is the real problem — ProcessManager signalling failure through a resolved payload is what made this invisible in the first place. Converting those to thrown errors would be the proper fix, but it changes the IPC contract and belongs in its own PR.

[rimusz]

The red Backend check is a pre-existing flake, and I have found its root cause.

CI reports 1 failed | 294 passed (295). The single failure is backend/src/plugins/UpdateScheduler.status.test.js > replaces the previous pass rather than accumulating, which imports nothing this PR touches.

It is a millisecond-resolution timestamp race, not an ordering problem:

// UpdateScheduler.js:161
const summary = { checkedAt: new Date().toISOString(), ... };

// the test, two ticks back to back with nothing between them
await scheduler.tick();  const first  = await scheduler.getStatus();
await scheduler.tick();  const second = await scheduler.getStatus();
expect(second.checkedAt).not.toBe(first.checkedAt);   // ← fails when both land in the same ms

toISOString() has millisecond resolution, so the assertion only passes when the two ticks happen to straddle a millisecond boundary. On this machine, two back-to-back new Date().toISOString() calls are identical in ~100% of 200k trials — the test passes only when something slow enough intervenes.

That matches every observation: it fails intermittently, passes in isolation, and flipped between two consecutive full-suite runs of the same code while I was preparing this PR (295 passed, then 294 passed / 1 failed). It also failed on untouched origin/main when I checked during #70.

Deliberately not fixed here — it is unrelated to this change and belongs in its own PR. The fix is small: either inject a clock, or assert on an incrementing counter rather than a wall-clock string. Happy to open that separately if useful.

Everything else on this run is green, including the full Playwright suite.

[rimusz]

Fixed — this PR is now fully green. And one correction to my comment above.

I said the UpdateScheduler test "passes in isolation", which pointed at ordering. That was based on a single isolated run. Measuring it properly on pristine main:

unmodified test, 30 isolated runs  ->  23 failed

At a ~77% failure rate, one passing run is a 23% event, not evidence. It fails in isolation too — the millisecond-clock diagnosis was right, the "order-dependent" framing was not.

The failure is exactly as predicted:

AssertionError: expected '2026-08-22T11:43:29.404Z' not to be '2026-08-22T11:43:29.404Z'

Fixed in #74, standalone against main so it can merge on its own and unblock every other PR — it fakes Date for that one test (only Date, so the scheduler's real async filesystem work is untouched) and stamps the two passes six hours apart. The assertion also gets stronger rather than merely stable: it now pins both exact timestamps instead of their inequality, which additionally catches a summary that carried the old timestamp forward — the very case the test exists to detect, and one not.toBe would have passed.

That commit is cherry-picked here as d17fdb04 so this PR is green now rather than waiting on #74. It is the identical patch, so it dedupes when #74 merges.

Verification: fixed test 15/15 green; full backend suite 295/295 on this branch, and 290/290 twice on the #74 branch.

All 5 checks green here now.