fix(workflow): an unreachable workflow process is not a workflow state - #71
Conversation
fetchWorkflowState answered every IPC failure with
`{ status: 'error', error: error.message }`. That is wrong twice.
'error' is a REAL workflow status. ProcessWorker sets it on a workflow
whose engine failed, and ProcessManager reads it back out of the database.
So "I could not reach the workflow process" and "this workflow failed"
arrived as the same value, and no caller could tell them apart. Both
restart checks in WorkflowService test
`['running','listening','queued'].includes(status)`, so an infrastructure
failure quietly took the not-active branch and the restart was skipped
without anyone being told why.
It also discarded the child's own diagnosis. sendMessage rejects for two
different reasons: the process was unreachable (never spawned, not ready,
failed to initialise, no answer in time), or the child ANSWERED
`{ success: false, error }`. That second message names the actual fault —
`Workflow wf-1 cannot be executed: node "n1" is missing "text"` — and it
was replaced by the single word `error` before any caller saw it. That is
why such failures could not be diagnosed from the API and had to be dug
out of the workflow-process log.
The handler's own catch had been unreachable the whole time. It tests
`error.message.includes('not ready')` and was added on 2026-03-10, two
months after the swallow landed in c9923d2 (2026-01-20): handling written
for an error that structurally could not arrive.
- sendMessage now rejects transport failures with a typed
WorkflowProcessUnavailableError carrying a reason of not-spawned,
not-ready, init-failed or timeout. A failure the child reported stays a
plain Error with the child's message intact.
- fetchWorkflowState logs and rethrows, matching restartActiveWorkflows
in the same class. All three callers already sit inside a try/catch, so
control flow is unchanged where it was already correct.
- GET /workflows/:id/status keeps the 200 'initializing' answer the
original author intended for a process that is still starting — that
branch is now actually reachable — answers 503 'unavailable' with the
reason when the process cannot be reached, and forwards the child's
diagnosis in `details` instead of a generic string.
- Both restart-check catches now report the real error instead of
asserting "not ready" for every cause, including a failure of the
restart itself.
22 tests. Without this change 11 of the 13 bridge tests fail and the 9
route tests cannot load at all, since the typed error does not exist.
…available Self-review: the 'init-failed' path threw away the very thing this change exists to preserve. sendMessage caught the readyPromise rejection and raised a fresh WorkflowProcessUnavailableError without attaching it, so "Workflow process failed to initialize" replaced whatever actually went wrong during spawn — the same erasure, one level down. The error now carries `cause`. The other three reasons are synthesised from a state check and have no underlying error, so `cause` stays absent rather than being set to undefined. 23 tests, +1.
|
Two CI notes for whoever picks this up. The first Playwright run failed; it was a flake, not this change.
Copilot could not review this PR — it posted "Copilot encountered an error and was unable to review this pull request." I do not have permission to re-request a review on this repo ( |
|
Correcting myself: the I called it "a genuine latent problem" on the strength of two checks, and both were bad:
I then "confirmed" it by grepping The module is inlined and the alias resolves. The real explanation for the warning: So: cosmetic warning, working setup, nothing to fix. Apologies for the noise — the rest of that comment (the Playwright flake analysis and the Copilot re-request) still stands. |
activateWorkflow did `res.json(result)` with no status, so a start that never armed answered 200 and every caller gating on `response.ok` reported success. Rebased onto #71 before merge; the diff collapsed from 9 files to its own 6.
Prepares the client and the tenant path for the token-proof flip, and writes
down what the /auth surface now actually does.
remoteTokenVerifier — THE BLOCKER
A hosted tenant delegates verification to api.agnt.gg, and every non-2xx from
the issuer was classified "unreachable". Once token-proof enforcement is
turned on, a token without a proof claim starts getting 401 there — which
would have meant:
- the stale-grace window kept serving a REFUSED token for up to 30 more
minutes, on exactly the installs that are reachable from the internet;
- the refusal was counted as remoteFail, so a wave of legitimate denials
would read on the dashboard as "the issuer is down";
- it was never cached as a denial, so a rejected client re-asked on every
request.
401/403 is now a denial: no grace, cached, counted as remoteDeny. 429 and 5xx
still mean UNKNOWN, because a rate-limited issuer must not log everyone out.
Four new cases fail against the previous code.
OAuth callback copy
POST /auth/callback answers a stable `reason`; the client rendered
`errorData.error` verbatim, which is what put a SQLite constraint string in
front of a user. New services/oauthCallbackErrors.js maps each reason to a
sentence that ends in an instruction, shared by the two consumers. A body with
no reason falls back rather than surfacing prose nobody wrote, and a transport
failure is passed through untouched — axios already describes it accurately.
OAuthCallback.spec.js asserted the raw server string appeared on screen, so it
was pinning the defect. It now asserts the opposite, plus that no driver string
can reach the screen and that a non-JSON error body still renders.
NOT DONE, DELIBERATELY: `reason: 'proof'` was not added to the axios
interceptor. That interceptor is scoped to BASE_URL, the local backend verifies
with the shared secret and cannot produce that reason, and every path that can
already resolves correctly — desktop's fetchUserData classifies the remote 401
as http_401 (definitive), and a hosted tenant now returns 'invalid' via the fix
above. Adding it would have been inert, which is the same mistake #76 made.
Documentation
- The two middlewares are distinguished: `authenticateToken` identifies and
continues, `requireAuth` refuses. The doc previously said "Required" for both.
- The `provider:nonce:origin` state format, and why the nonce is in the middle.
- The full callback reason vocabulary, with which statuses are retryable.
- Remote Feedback Routes: live and entirely undocumented until now.
- Stream base path corrected from /streams to /stream — verified against
production, /streams 404s.
- The /start, /stop and /:id/status contract change from #71 and #73, including
that the SDK is axios and will now throw where it used to swallow.
- 4 further undocumented live routes closed. A parse-the-router audit now
reports 35/35 documented and 0 broken TOC anchors.
Backend 4735 tests (1 pre-existing flake in runJournal.heartbeat, filed,
passes 13/13 in isolation). Frontend 3943, all pass.
Follow-up to #70, which named this as the reason that bug took so long to diagnose. Independent of it — test-merged both ways,
Auto-merging backend/src/services/WorkflowService.js / Automatic merge went well.What
WorkflowProcessBridge.fetchWorkflowStateanswered every IPC failure with{ status: 'error', error: error.message }. That is wrong twice over, and this replaces it with a distinction the callers can act on.Why
1.
'error'is a real workflow statusProcessWorkersets a workflow's status to'error'when its engine fails (ProcessWorker.js:33,61,109), andProcessManager.fetchWorkflowStatereads that value back out of the database for an inactive workflow. So the bridge was answering "I could not reach the workflow process" with the identical value that means "this workflow failed". No caller could tell them apart.Both restart checks in
WorkflowServicetest['running','listening','queued'].includes(currentState.status). On an infrastructure failure the status is'error', which is not in that list, so the branch is quietly skipped: a live workflow silently keeps running its old definition after a save, and the log line blamed a cause it had not checked.The API is worse —
GET /workflows/:id/statusreturned 200 with{status:'error'}, so a client saw a failed workflow rather than an unreachable service.2. It discarded the child's own diagnosis
sendMessagerejects for two structurally different reasons:{ success: false, error }The second case carries messages like
Workflow wf-1 cannot be executed: node "n1" is missing "text". That text was replaced by the single worderrorbefore any caller saw it. This is precisely why the #70 bug could not be diagnosed from the API and had to be dug out of the workflow-process log.3. The handler's own catch was unreachable
WorkflowService.fetchWorkflowStatealready had a catch testingerror.message.includes('not ready')and returning{status:'initializing'}. It could never fire, because the bridge caught everything. Git dates the two: the swallow landed inc9923d20(2026-01-20), the handler in2887115c(2026-03-10). Someone wrote handling for an error that structurally could not arrive, and nothing said so.How
sendMessagerejects transport failures with a typedWorkflowProcessUnavailableErrorcarryingreasonofnot-spawned/not-ready/init-failed/timeout. A failure the child reported stays a plainErrorwith its message intact.fetchWorkflowStatelogs and rethrows — matchingrestartActiveWorkflowsin the same class, which already does exactly this. All three callers already sit inside atry/catch, so control flow is unchanged wherever it was already correct.GET /workflows/:id/statusnow distinguishes:not-ready→ 200{status:'initializing'}— unchanged intent, just finally reachable{status:'unavailable', reason, details}details(house style, matchingsaveWorkflow)isWorkflowProcessUnavailable(error)is exported so nobody has to sniff message text again. The old check matched one of the four transport failures and would have silently stopped matching on a reword.Testing
22 tests across 2 files. Without the change, 11 of the 13 bridge tests fail and the 9 route tests cannot load at all (the typed error does not exist). The bridge tests drive the real bridge with a mocked child, following
WorkflowProcessBridge.lifecycle.test.js. The route tests deliberately do not mock the predicate or the error class — the wiring between them and the handler is part of what is under test.The baseline reproduces the bug verbatim:
Mutation tested — 8 mutants, 8 killed, 0 survived:
Full backend suite: 291 passed / 292 files, including the existing
WorkflowProcessBridge.lifecycle.test.jswhich exercisessendMessagedirectly. The single failure,UpdateScheduler.status.test.js > replaces the previous pass rather than accumulating, is pre-existing and unrelated — verified failing identically on untouchedorigin/mainwhile investigating #70, passes in isolation, imports nothing this PR touches, and does not fire on CI.Notes for the reviewer
/workflows/:id/statuscan now return non-2xx. Both frontend pollers already handle it: each doesif (!response.ok) throw, and the surviving poller reschedules from its catch — the same thing it previously did ondata.status === 'error', which is in its continue-polling list. So client behaviour is unchanged in the failure case either way; what changes is that the failure is now legible.activateWorkflowanddeactivateWorkflowswallow the same way, returning{ error: error.message }. That shape at least does not impersonate a status, so it is a milder version of this bug and a bigger blast radius. Happy to do it as a third PR.catchblocks still skip the restart when the process is unreachable. Retrying there is a behaviour change I did not want to fold into an error-reporting fix.