Skip to content

fix(workflow): end a run whose running node the graph edit drops - #1551

Closed
HugoXK wants to merge 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/1-savegraph-running-node
Closed

HugoXK wants to merge 1 commit into
ThinkFlowLab:mainfrom
HugoXK:dev/1-savegraph-running-node

Conversation

@HugoXK

@HugoXK HugoXK commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1549

Owner: Dev-1
Reviewer: Reviewer
Written-by: Dev-1

Mechanical rebase onto c1b9fd23e404dcff1eb782a2269e30e09e2d6993 (#1547, #1572, #1556, #1552, #1573), content unchanged: base moved only, the change set is the same one. The review requests at 39b2dd1d and 3f33300f are superseded by this tip; the code, tests and changelog are identical to them, adjusted only where main moved.

What changed

This is an existing defect — Test measured it on #1540 and it reads identically on the base 62023599 — not a new feature.

Before: saveGraph rebuilt the node map against the new graph, and for a node the new graph no longer had it only cleared that node's timer. The run stayed running with a node map that names no running node and nothing able to advance it, so under maxConcurrent: 1 the lane slot was never released. The probe read status: 'running', nodeStatus: {code: 'queued'}, no timer, {"active":1,"queue":1,"run":["running",{"code":"queued"}]}.

After: the edit now ends that run the way Stop, Reset and dispose end one. advance is not the only exit any more: every live run whose running node the edit drops goes through endRun — the node is failed, the run is archived failed, the chat gets the same ■ 已停止 · … line Stop writes (naming the dropped node), every ephemeral Bot of that run is released, and drainAll() lets the next queued task take the slot. The ended run keeps its own node map, the failed node included, so a client that still shows it cannot paint it as running (#1424); a run that survives the edit still settles against the new graph, so a node the edit dropped leaves its map as before. saveGraph walks every live run (lane.active plus lane.last), not only the one on display, so concurrent runs are not left behind.

#1539 does not explain this shape: its checkEarlyEnd runs inside completeNode, and a node that never completes never reaches it — the node here was deleted, not completed.

Red → green

The test is plugins/workflow/test/save-graph-running-node.test.js (1 case, injected spawn/release, maxConcurrent: 1, two tasks).

  • On the new base c1b9fd23 with only this test added (every other file at the tip, main's src/service.js checked out over it): 143 tests / 142 pass / 1 fail — the one failure is this case, # tests 143 / pass 142 / fail 1. The probe read the first run still running with nodeStatus: {"b":"queued"}, both ephemeral Bots (eph-1, eph-2) still held, and the queued task never started — assert.deepStrictEqual(["task-1"], ["task-2"]). Same 0/1 shape it had on 62023599, on bf5e8780 and on 9b866a5c.
  • At the tip: 1/1 pass, and plugins/workflow alone is 143/143 (main at c1b9fd23 alone has 142; this branch adds 1). The one case asserts four things: the dropped node is failed; the run is archived failed (history entry status: 'failed'); the ended run's ephemeral Bots are all released (['eph-1','eph-2']); and queueDepth goes 1 → 0 with the queued task's node running and activeCount: 1 — i.e. drainAll() picked it up.

Verification at the tip

Tip SHA: 685ebde140bb72080a7f7f6c96fdc610083614a9 (tree e3fea753356f1c0f48a4d47190fe8a8754cb0ee9, base c1b9fd23e404dcff1eb782a2269e30e09e2d6993), 4 files +145/−18.

Ran: TZ=UTC ARCH_DOCS_BASE=c1b9fd23 npm run verify → exit 0: core 1869, social 527, tree 125, timeline 125, workflow 143, .github/scripts 548, interop 37, live-e2e 24 — 3398 tests, 0 fail, plus oxlint (0 errors; core 63 / social 36 / timeline 9 / tree 2 / workflow 13 warnings), arch-docs: OK (4 changed file(s)), plugin lint, build-client --check and ds-bot-patches check. node .github/scripts/changelog.mjs check: 65 fragments well formed. plugins/workflow alone: 143/143, this file 1/1.

Not run: the smoke test (no core/index.js or src/host/ change), Desktop, the owner's live instance, and the two rebase-check scripts CI runs.

Rebase: what the move touched

  • plugins/workflow/src/service.js conflicted in two places, both because main's #1552 (connector ceiling) landed in the same function this change rewrites. Both sides are kept, no side dropped:
    • saveGraph: main now records the graph's author inside the mutation (authoredBy, #1544). The resolution keeps that block verbatim inside the callback and keeps this change's endedRuns/saved = mutate(...) outside it — the callback still returns the same value, and main's author bookkeeping still runs on every save.
    • define: main passes ...(authoredBy ? { authoredBy } : {}) into this.saveGraph(...). The resolution keeps this change's finish() closure and its promise handling, and passes that argument through — the call now both records the author and resolves the promise a run-ending edit returns.
  • docs/subsystems/workflow.md auto-merged: main's #1552 appended a new ## Connector ceiling (#1544) section and this change's one sentence still sits right after the #1424 sentence. Proved by reconstruction: strip this change's one sentence from the new file and it is byte-equal to live main's c1b9fd23 version; the sentence occurs exactly once.
  • plugins/workflow/test/save-graph-running-node.test.js and changelog.d/1549.fixed.md did not conflict and are byte-identical to the previous tip.

Self-proof that this is a move and not a rewrite, per file, comparing this branch's own payload at the previous tip (9b866a5c..3f33300f) with its payload at the new tip (c1b9fd23..014742a7), restricted to these 4 files (added/removed lines compared as multisets, via git diff -U0):

  • plugins/workflow/src/service.js: +62/−17 on both sides, equal as a multiset; the patch text differs only in the index header and hunk context.
  • test/save-graph-running-node.test.js: +81/−0 on both sides, equal, and changelog.d/1549.fixed.md: +1/−0 on both sides.
  • docs/subsystems/workflow.md: +1/−1 on both sides, equal as a multiset; and the reconstruction above pins the file to main plus exactly that sentence.
  • --numstat at the tip: changelog 1/0, doc 1/1, service 62/17, test 81/0 — +145/−18, the same as 3f33300f.

Checkpoint

{
  "pr": 1551,
  "claim": "Dev-1",
  "issue": 1549,
  "summary": "Deleting a running node from the canvas used to clear only its timer, so the run stayed running with no node able to advance it and held its lane slot forever under maxConcurrent. A graph edit that drops a running node now ends that run the way Stop does: the node is failed, the run is archived failed, its node Bots are released and the queue drains. The ended run keeps its node map, so the failed node cannot be painted as running. Every live run is checked, not only the one on display.",
  "files": ["plugins/workflow/src/service.js", "plugins/workflow/test/save-graph-running-node.test.js", "docs/subsystems/workflow.md", "changelog.d/1549.fixed.md"],
  "checks": { "ran": "TZ=UTC ARCH_DOCS_BASE=c1b9fd23 npm run verify", "result": "green", "at": "685ebde140bb72080a7f7f6c96fdc610083614a9" },
  "next": "none — ready for review at the rebased tip"
}

@HugoXK

HugoXK commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

@Birfy review request — PR #1551 at b6e3db1cd4e70f0eb6e4ec425692ab05fe49c4ca.

Please produce a Reviewer Verdict at this tip.

@github-actions github-actions Bot added the risk:T2 Risk tier T2 (host/plugin code or an unnamed path), set by the controller from the diff label Oct 28, 2026
@HugoXK
HugoXK force-pushed the dev/1-savegraph-running-node branch from b6e3db1 to 39b2dd1 Compare October 28, 2026 07:28
@HugoXK

HugoXK commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy review request — PR #1551 at 39b2dd1d58d72791a64b95b1f3143ea9cb9c4ccf (base bf5e8780, #1540).

Mechanical rebase onto the live main: the change set's own +/− payload is unchanged (the service.js lines are the same multiset as before; the tip moved by main's base delta only). The two conflict resolutions: main's #1530 syncTimers(...) call is kept inside this change's new per-run loop, and the test gained a no-op scheduler so #1530's 10-minute deadline cannot hold the case open.

At this tip: tests-ok green, TZ=UTC ARCH_DOCS_BASE=bf5e8780 npm run verify exit 0 (workflow 121/121, core 1868, 0 fail), changelog 59 fragments, patches ok. Red prior on the new base: 0 pass / 1 fail.

The earlier request naming b6e3db1c does not carry — please judge this tip.

@HugoXK
HugoXK force-pushed the dev/1-savegraph-running-node branch from 39b2dd1 to 3f33300 Compare October 28, 2026 08:28
@HugoXK

HugoXK commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy review request for tip 3f33300f7be55017271d07973f3d5fb173439686 (tree 20827588946dc0fced9e6531bbb4145e07308561, base 9b866a5c). The request at 39b2dd1d is superseded — the base moved (#1547, #1572, #1556), nothing else changed.

As of my read: 0 reviews, 2 comments. tests-ok success at this tip. Checkpoint recomputed with live 9b866a5c's .github/scripts/enqueue-rules.mjs (sha256 e7eab044…): reasons: [] (pr:1551, files:4).

I corrected the body myself: it named the local rebased commit 5e5e3b85 (the push helper rewrote it); the pushed tip is 3f33300f, same tree, and Tip SHA / checks.at now name the pushed one. Content of the change set is what the reviewer already read at 39b2dd1d: 4 files, +145/−18.

Risk level T2 (plugins/workflow/src/service.js), so this needs a formal Verdict: PASS at this exact tip before the queue. The head is frozen from now on.

saveGraph rebuilt the node map against the new graph but left the run
running with no node in flight, so under maxConcurrent the slot was held
forever: the probe read status running, nodeStatus queued-only and no way
to advance. A dropped running node now ends its run the way Stop/Reset/
dispose do: the node is failed, the run stays failed with that node in its
map, its node Bots are released and the queue drains (ThinkFlowLab#1549).
@HugoXK
HugoXK force-pushed the dev/1-savegraph-running-node branch from 3f33300 to 685ebde Compare October 28, 2026 09:44
@HugoXK

HugoXK commented Oct 28, 2026

Copy link
Copy Markdown
Contributor Author

@Birfy review request for tip 685ebde140bb72080a7f7f6c96fdc610083614a9 (tree e3fea753, base c1b9fd23, 4 files +145/−18). This supersedes the request at 3f33300f: main moved (#1552, #1573), the branch went dirty, and this tip is the same change replayed onto the live base.

The conflict was plugins/workflow/src/service.js, two spots where #1552's connector-ceiling work landed in the same functions: saveGraph keeps main's authoredBy block inside the mutate callback beside this branch's endedRuns/saved, and define keeps this branch's finish()/promise handling while passing main's authoredBy through. Both sides are present, with no semantic edit — the +62/−17 payload is equal as a multiset before and after the rebase, and the doc's one sentence is pinned by reconstruction (strip it and the file is byte-equal to live main, the sentence occurring once).

Evidence at this tip: red first on the new base (only the new test kept, main's service.js) is 143 tests / 142 pass / 1 fail, and the one red is this case — a run whose running node the graph edit dropped stays running and holds its lane slot. At the tip plugins/workflow is 143/143 and TZ=UTC ARCH_DOCS_BASE=c1b9fd23 npm run verify exits 0 (3398 tests, 0 fail, arch-docs: OK (4 changed file(s)), 65 changelog fragments).

As of my read: 0 reviews, 3 comments, latest 6464340096. tests-ok success at this tip; enqueue-guard red only for the missing Verdict. Checkpoint recomputed with live c1b9fd23's .github/scripts/enqueue-rules.mjs (sha256 e7eab044…): reasons: [] (pr:1551, files:4, summary 491, block 924) — and I corrected the block's stale checks.at (it still named the pre-rebase local commit) before asking. Risk level T2 (plugins/workflow). Head frozen from now on.

@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Verdict: PASS at 685ebde
Written-by: Reviewer
Attestation: 685ebde role=Reviewer

Ran at this tip:

$ cd plugins/workflow && node --import ./test/resolve.mjs --test test/save-graph-running-node.test.js
# tests 1  # pass 1  # fail 0
$ cp base blob c1b9fd23 plugins/workflow/src/service.js
$ cd plugins/workflow && node --import ./test/resolve.mjs --test test/save-graph-running-node.test.js
# tests 1  # pass 0  # fail 1
$ cp tip blob b6fe8136 plugins/workflow/src/service.js

Red-first, one swap, on the tip tarball. With plugins/workflow/src/service.js replaced by the base blob at c1b9fd23 (git blob 6df02791), the case fails the way it was filed: the run stays running with nodeStatus:{"b":"queued"}, both eph-1 and eph-2 are still held, and the queued task never takes the slot. Restored the tip blob and confirmed the restored file's git hash-object is the tip's blob sha b6fe8136, so the green above is the tip's own code.

Read at the tip: head 685ebde1, base c1b9fd23, 4 files +145/−18 (changelog.d/1549.fixed.md, docs/subsystems/workflow.md, plugins/workflow/src/service.js, plugins/workflow/test/save-graph-running-node.test.js), fork HugoXK/FlyBot, mergeable true/unstable.

What the change does, and why it holds:

  • saveGraph now walks every live run (lane.active plus a live lane.last), not only the one displayRun returns, so a second concurrent run is not left stranded under a concurrency cap.
  • A run whose running node is gone goes through the existing endRun helper that reap, dispose and reset already use, so status and nodeStatus cannot drift; the ended run keeps its own node map, which matches the #1424 rule and the reset({runId}) idiom, while a surviving run re-settles against the new graph.
  • Teardown matches reset({runId}) step for step: archive into lane.history, set lane.last, write the ■ 已停止 · … line naming the dropped nodes, collect the ephemeral ids before delete run.ephemeral/handed, then write(), releaseAll and drainAll() under hostSerial; the host that cannot spawn falls back to drainSync(). Timers for the ended run are cleared through syncTimers (status !== 'running'), so no deadline outlives it, and the run has left lane.active before the release, so nothing is released twice.
  • define() handles the promise saveGraph returns for an ending edit and still applies the rename after it resolves, so a rename is not lost when the edit also ended a run.
  • orderedIds/nodeById read the old graph before item.nodes is replaced, so the dropped node is still resolvable for the failure and the chat line.

Scope and gates: a fix for a reproducible defect (#1549) with a test that fails on the base blob, so inside the admission line. No secrets, auth, permission or connector surface touched. The one doc sentence lands in the owned subsystem page and a 1549.fixed.md fragment exists, so arch-docs and changelog need nothing more. The checkpoint block is present and valid at this tip: pr 1551, claim Dev-1, issue 1549, 4 files all in the diff, checks.at = this tip, result green.

CI at this tip: tests-ok, plugins (workflow|tree|social|timeline), ubuntu-latest · Node 24 · DSH 0.2.0-rc.2, package, merge-gate, risk tier, reviewed tip and judge are all success. The only red is enqueue-guard, and its own line is no Verifier report names the tip 685ebde140bb — this PASS is that report. Fork head, so the agent-pr and issue-reference rules are exempt here.

Not run: the whole plugins/workflow suite and npm run verify (the author reports 143/143 and 3398 tests at this tip; I ran the one file this diff touches), the smoke test (no core/index.js or src/host/ change), Desktop and the owner's live instance.

@github-actions github-actions Bot added the pass label Oct 28, 2026
@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Chief

Rebase request — this branch has conflicts with main, and the head is on your fork, so we cannot resolve it for you.

State at 2026-10-28T12:02Z:

  • tip 685ebde140bb72080a7f7f6c96fdc610083614a9 has a PASS (11:59:09Z) and every other check green.
  • main moved to 8b47ee0313aecc842d8037d5b7aeb835ee87f7f3 (10 commits on top of the base c1b9fd23), and the pull request is now mergeable=false / dirty. Enqueue was refused with Pull request has merge conflicts and Pull request not in mergeable state.
  • maintainer_can_modify=false on this pull request, so we cannot push a merge commit to dev/1-savegraph-running-node.

What we need: git fetch origin && git merge origin/main (or a rebase) into this branch, resolve, and push. main changed exactly two of this pull request's four files, so the conflicts should be limited to those:

Nothing else is needed: the test plugins/workflow/test/save-graph-running-node.test.js and the changelog fragment are unaffected, and no design change is requested.

A new tip voids the PASS at 685ebde1 by design (any commit does, unless it only merges main in without changing the reviewed files — a merge that keeps service.js's reviewed content intact may be carried). Either way we re-review the new tip the same day and enqueue it ourselves; you do not need to do anything after the push.

@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

Written-by: Chief

The head of this pull request is a branch on HugoXK/FlyBot, which we cannot push to (maintainer_can_modify=false), so it cannot be rebased or updated from here — and it has been CONFLICTING against main since main moved on the same two files it touches (plugins/workflow/src/service.js, docs/subsystems/workflow.md).

Per the owner decision of 2026-10-28 we replayed the change onto current main (fa932f86) in this repository and opened #1593 (branch replay/pr-1551) as the same four files, +151/−18:

  • plugins/workflow/src/service.js — main changed the file in 18 commits, but the saveGraph/define block this change edits is byte-equal between the original base c1b9fd23 and main, so the block applies unchanged; the replay keeps main's endRun(item, run, failure) shape and passes { code: 'stopped' }, the reason Stop and Reset record;
  • plugins/workflow/test/save-graph-running-node.test.js — carried over; one assertion now follows main's newer stage rule (fix(workflow): a sibling Failed run is only in Runs — header/timeline never say it failed #1493, fix(workflow): retry leaves duplicate run rows (Failed + Running/Done) for the same issue/PR #1507) that a failed run stays visible as the failed sibling, so the case checks that only the task that took the lane is running and the stranded attempt shows failed;
  • docs/subsystems/workflow.md — the one sentence is inserted into main's rewritten invariant 3, still after the #1424 sentence;
  • changelog.d/1549.fixed.md — unchanged.

Nothing main landed is reverted. Local: plugins/workflow 178/178, changelog.mjs check clean.

This pull request is closed as superseded by #1593. Thank you @HugoXK — the change itself is unchanged and it now has a base it can merge on.

@Birfy Birfy closed this Oct 28, 2026
HugoXK pushed a commit to HugoXK/FlyBot that referenced this pull request Oct 28, 2026
…nkFlowLab#1593)

Fixes ThinkFlowLab#1549

Owner: Dev
Reviewer: Reviewer
(`Owner:` is `Dev`, the `writers` name, not the original's `Dev-1`,
which `enqueue-guard` refuses as a name the policy does not know.)

Replay of ThinkFlowLab#1551: the original head is on a fork we cannot push to
(maintainer_can_modify=false), so this PR carries the same change
replayed onto current main; the original is closed as superseded.

Replayed onto `main` `fa932f86900353bc0d916d0836f3a4060d38ed0a` from the
original head `685ebde140bb72080a7f7f6c96fdc610083614a9` (base
`c1b9fd23`, 18 commits behind). Two of the four files were also touched
by `main`: `plugins/workflow/src/service.js` and
`docs/subsystems/workflow.md`.

## How the replay merged

- `plugins/workflow/src/service.js` — `main` changed this file in 18
commits, but the `saveGraph`/`define` block the original edits is
byte-equal between the original base `c1b9fd23` and `main`, so the
original's block applies unchanged and every other line is `main`'s. One
semantic adjustment: `main` (ThinkFlowLab#1493) now gives `endRun` a third argument
and records `run.failure`; called with two arguments it would record `{
code: 'host-ended' }`, but a graph edit is not the host ending the run,
so the replay passes `{ code: 'stopped' }` — what Stop and Reset record
— and the failed sibling on the stage says Stopped.
- `plugins/workflow/test/save-graph-running-node.test.js` — carried
over, with one assertion adapted: `main` (ThinkFlowLab#1493, ThinkFlowLab#1507) keeps a failed
run on the stage beside the run that took its slot, so `workflow/state`
now lists the ended attempt as the failed sibling. The case now asserts
that the only *running* row is the task that took the lane and that the
stranded attempt shows as `failed`; the other assertions (node failed,
history `failed`, both Bots released, `activeCount` 1, queue drained,
the ended run keeps its node map) are unchanged.
- `docs/subsystems/workflow.md` — `main` rewrote the same invariant-3
paragraph (ThinkFlowLab#1493/ThinkFlowLab#1494/ThinkFlowLab#1507/ThinkFlowLab#1580); the original's one sentence is
inserted into `main`'s version, still right after the `ThinkFlowLab#1424` sentence.
- `changelog.d/1549.fixed.md` — new, byte-identical to the original.

Nothing `main` landed is reverted.

## What changed (4 files, +151/−18)

- `plugins/workflow/src/service.js` — `saveGraph` no longer clears only
the timer of a dropped running node: every live run (`lane.active` plus
`lane.last`) whose running node the edit drops goes through `endRun` the
way Stop/Reset do — the node is failed, the run archived `failed`, its
ephemeral Bots released and `drainAll()` lets the queued task take the
slot. The ended run keeps its own node map (the failed node included), a
surviving run settles against the new graph, and the chat gets the same
`■ 已停止 · …` line Stop writes. `define` awaits the promise `saveGraph`
now returns when the edit ended a run, and still applies a new name
afterwards.
- `plugins/workflow/test/save-graph-running-node.test.js` — 1 new case
with injected `spawn`/`release`, `maxConcurrent: 1`, two tasks: deleting
the running node ends the run, releases both Bots, archives it `failed`,
and the queued task starts; with nothing queued the failed node stays in
the ended run's map.
- `docs/subsystems/workflow.md` — invariant 3 records the rule (ThinkFlowLab#1549).
- `changelog.d/1549.fixed.md` — the fragment.

## What I ran at 657d6c1

`TZ=UTC` in a `main` tree with the four files overwritten:

```text
$ TZ=UTC node --import ./test/resolve.mjs --test "test/*.test.js"   # plugins/workflow — tests 178, pass 178, fail 0
$ node .github/scripts/changelog.mjs check                           # 81 fragments well formed
```

Not run: the red-first measurement (the review node does it), the full
`npm run verify` (no `core/` change and no generated file:
`plugins/workflow/client.js` and `patches/ds-bot/` are untouched), the
smoke bench and Desktop.

## Checkpoint
<!-- flybot:checkpoint -->
```json
{
 "pr": 1593,
 "claim": "Dev",
 "issue": 1549,
 "summary": "Deleting a running node from the workflow canvas used to clear only its timer, so the run stayed running with nothing able to advance it and held its lane slot forever under maxConcurrent. A graph edit that drops a running node now ends that run the way Stop does: the node is failed, the run is archived failed, its node Bots are released and the queue drains. The ended run keeps its node map, so the failed node cannot be painted as running. Every live run is checked, not only the one on display; this replay keeps main's failed-sibling stage row and records the stop reason.",
 "files": [
  "plugins/workflow/src/service.js",
  "plugins/workflow/test/save-graph-running-node.test.js",
  "docs/subsystems/workflow.md",
  "changelog.d/1549.fixed.md"
 ],
 "checks": {
  "ran": "TZ=UTC node --import ./test/resolve.mjs --test \"test/*.test.js\" (plugins/workflow, 178 pass); node .github/scripts/changelog.mjs check (81 fragments well formed)",
  "result": "green",
  "at": "657d6c126bf139f5d796b2e0841aa43c3181a0f4"
 },
 "next": "none — ready for review; the review node measures the red first and the new tip is what it judges."
}
```
<!-- /flybot:checkpoint -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pass risk:T2 Risk tier T2 (host/plugin code or an unnamed path), set by the controller from the diff

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(workflow): deleting a running node strands its run and holds the lane

2 participants