Skip to content

fix(workflows): report the workflow n8n created when activation fails - #5644

Open
bettercalln1ck wants to merge 1 commit into
Osmantic:mainfrom
bettercalln1ck:fix/3936-orphaned-workflow-activation
Open

bettercalln1ck wants to merge 1 commit into
Osmantic:mainfrom
bettercalln1ck:fix/3936-orphaned-workflow-activation

Conversation

@bettercalln1ck

Copy link
Copy Markdown
Contributor

Summary

Fixes #3936.

enable_workflow POSTs the workflow to n8n, then PATCHes it active. The PATCH sat inside the handler's outer try, so both of its failure modes reported something untrue:

A timeout or connection error escaped to the outer handlers:

except asyncio.TimeoutError:
    raise HTTPException(status_code=504, detail="n8n workflow add timed out")
except aiohttp.ClientError as e:
    raise HTTPException(status_code=503, detail=f"Cannot reach n8n: {e}")

Both read as "the import did not happen", and both discard n8n_id. The workflow did exist in n8n, and the operator had no id with which to find or remove it — the orphan this issue describes.

A non-200 PATCH was not an exception at all, so the handler fell through to:

return {"status": "success", ..., "activated": activated, "message": f"{wf_info['name']} is now active!"}

with activated=False. It reported success, and told the operator the workflow is now active when it was not. That half is not in the issue text but sits in the same three lines and is the more likely of the two to bite.

Activation now runs in its own scope. Either failure returns 502 naming the workflow n8n created, stating that it exists and is inactive, and directing the operator to activate it in the n8n admin panel — explicitly not to re-run enable, which on this branch imports a second copy rather than activating the first:

Act Workflow was imported into n8n as workflow n8n-77, but activating it failed
(n8n returned HTTP 500). The workflow exists and is inactive — activate it in
the n8n admin panel. Do not re-run enable: that creates a second copy rather
than activating this one.

The successful path keeps its existing 200 / "success" / activated: true contract, and a rejected create still surfaces n8n's own status code — those are different faults.

AI Assistance

AI-assisted: an AI coding assistant drafted the patch and the regression test and ran the validation recorded below. I reviewed the diff, chose the validation, and am accountable for the change.

Release Lane

  • Stable hotfix targeting release/2.6.x
  • Mainline change targeting main
  • Next-minor work targeting the next feature/minor release
  • Not sure; reviewer should help classify

Stable hotfix reason:

n/a

Changed Surface

  • Docs only
  • Tests only
  • Dashboard UI
  • Dashboard API / host agent
  • Installer / bootstrap / lifecycle
  • Docker Compose / service manifests
  • Model routing / Hermes / capabilities
  • Network exposure / auth / proxy
  • Dependencies / runtime wiring

Risk And Validation

  • Risk level: Low
  • Validation run:
    • git diff --check
    • Markdown/link sanity for docs
    • Focused tests listed below
    • Dashboard lint/test/build
    • Extension audit / compose validation
    • Release-grade fleet or scoped hardware validation
    • Stable-lane patch validation, if targeting release/2.6.x
    • Not required because: one handler; the happy path and the create-rejection path are both pinned by new tests alongside the two failure modes, and the full dashboard-api suite is green.

Commands/results:

$ pytest tests/test_workflows.py -q
49 passed

$ pytest tests/ -q                       # the 'api' CI job
2142 passed, 1 skipped

# both new failure-mode tests fail with the router restored from origin/main
$ git checkout origin/main -- ods/extensions/services/dashboard-api/routers/workflows.py
$ pytest tests/test_workflows.py -q
FAILED tests/test_workflows.py::test_activation_http_error_reports_the_created_workflow_id
FAILED tests/test_workflows.py::test_activation_timeout_reports_the_created_workflow_id
2 failed, 47 passed

$ make lint
All lint checks passed.
$ ruff check ods/ --select E,F,W --ignore E501,E701,E731,E741,E402
All checks passed!

# run on Python 3.11, matching the workflow

Operational Change Check

  • This is not an operational change.
  • This is an operational change and validation is recorded above.
  • This is an operational change and validation is intentionally deferred for:

Notes For Reviewers

  • Four tests: the non-200 activation, the timeout activation, the unchanged happy path, and a rejected create still returning n8n's status. The last two exist so the fix cannot be "turn every enable into a 502".
  • The timeout test asserts detail != "n8n workflow add timed out" specifically — that exact string was the old, id-less answer, so the assertion fails if the outer handler ever reclaims this path.
  • except HTTPException: raise is added ahead of the outer handlers. It is not strictly required today (HTTPException is neither TimeoutError nor ClientError), but the new 502 is raised from inside that try and I would rather the intent be explicit than rely on the exception hierarchy staying put.
  • 502 rather than 200-with-a-partial-status. The issue asks for an error response carrying the id, and nothing in the dashboard calls this endpoint (I checked dashboard/src/ — the only /enable caller is FeatureDiscovery.jsx, which hits /api/features/{id}/enable), so no UI depends on the old shape. If you would rather this be a 200 with "status": "partial" so a caller can distinguish "created but inactive" from "nothing happened", that is a small change.
  • Touches the same function as fix(workflows): match n8n workflows by exact name and stop duplicate imports #5554 (exact-name matching / idempotent enable) but different lines. If both land, whichever is second may want a trivial rebase. Worth noting that fix(workflows): match n8n workflows by exact name and stop duplicate imports #5554's idempotency check would also turn "re-run enable" into a safe recovery path, at which point the warning in this message could be softened.

enable_workflow POSTs the workflow to n8n, then PATCHes it active. The PATCH
sat inside the handler's outer try, so both of its failure modes lied:

  - a timeout or connection error escaped to `except asyncio.TimeoutError` /
    `except aiohttp.ClientError`, which answer "n8n workflow add timed out" or
    "Cannot reach n8n" and drop n8n_id. The workflow existed in n8n and the
    operator had no id to find or remove it with.
  - a non-200 PATCH was not an exception at all, so the handler returned
    {"status": "success", "activated": false} with the message
    "<name> is now active!" about a workflow that was not active.

Activation now runs in its own scope. Either failure returns 502 naming the
workflow n8n created, stating it exists and is inactive, and telling the
operator to activate it in n8n rather than re-running enable — which on this
branch would import a second copy instead of activating the first.

The successful path keeps its existing 200/"success"/activated contract, and a
rejected create keeps surfacing n8n's own status code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workflow creation can orphan workflows in n8n when activation fails

1 participant