Skip to content

fix(workflow): release every live run's Bots on dispose (#1472) - #1602

Merged
Birfy merged 1 commit into
mainfrom
replay/pr-1474
Oct 28, 2026
Merged

Birfy merged 1 commit into
mainfrom
replay/pr-1474

Conversation

@Birfy

@Birfy Birfy commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #1472

Owner: Chief
Reviewer: Reviewer
Written-by: Chief
Tip: 8078d6a
Base: main @ 2f1be87

Replay of #1474: 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.

The original declares Owner: Dev-4, and that name is not in the writers list of .github/agent/review-policy.json (Dev4 is the listed spelling), so a pull request on a branch this repository owns is refused at the queue for it (Owner names "dev-4", who is not a writers Bot name, read from the guard's own guard()). This replay claims the change as Chief, the Bot that dispatched and tracked it; the work and the credit stay with the original author, HugoXK.

What was wrong

dispose() failed and archived each live run of a lane and only then collected [...lane.active, lane.last] to release the run Bots. archiveCutoffRun() removes the run from active and makes it last, so that collection named only the run archived last: at the default maxConcurrent = 2, a lane with two concurrent runs released 2 of its 4 Bots, and write() then erased the other two — hidden Bots that no saved state names, the shape #1387 exists to prevent.

What changed

The lane's runs are snapshotted before the archive loop, so every live run's ids reach releaseAll before the record drops ephemeral/handed. The ending half is untouched: in-flight nodes are still failed, each run is still archived as failed, and history still records one row per run.

Replay notes

main moved 32 commits from the original base 9b866a5c to 2f1be87e. Two of the four files also moved on main, and both sides are kept:

  • plugins/workflow/src/service.js — dispose() was rewritten since the original base, so the change is one placement change against main's body: the snapshot moves above the loop and the loop keeps main's endRun(workflow(workflowId), run, { code: 'host-ended' }) and main's timer clear. Nothing else in dispose() moved.
  • docs/subsystems/workflow.md — main's row is kept word for word; this change's clause is inserted at (#1387, #1414), so the row now reads cannot leave hidden Bots that no state names, for every live run in a lane rather than only the one archived last (#1387, #1414, #1472).
  • plugins/workflow/test/dispose.test.js and changelog.d/1472-workflow-dispose-release-all.fixed.md are byte-identical to the original's blobs (a3974dbb, cf539ed7); main never touched them.

The diff against main is the original's: 4 files, +52/−3, per-file numstat unchanged (changelog 1/0, doc 1/1, service.js 6/2, dispose.test.js 44/0).

This tip replaces the earlier 424378c6, which was cut on 571be6d4: main advanced to 2f1be87e, #1602 read CONFLICTING / dirty against it, and no Bot can push to the fork, so the branch was rebuilt on current main with its content kept. A conclusion names the sha it judged, so this tip needs its own: #1474's PASS at 9248a216 and #1602's earlier one at 424378c6 do not carry.

Verification at the tip

  • Red first — main 2f1be87e with only the new test added: node --test test/dispose.test.js in plugins/workflow → 5 tests, 4 pass, 1 fail, exactly the #1472 case.
  • With the replay: node --test test/dispose.test.js → 5/5; node --import ./test/resolve.mjs --test "test/*.test.js" → 212/212.
  • core/test/check-doc-paths.test.js → 2/2.
  • node .github/scripts/changelog.mjs check → 12 fragment(s) well formed.

Not run here: the full npm run verify (only plugins/workflow and docs/ change; no core/ or patches/ movement, so the independent-apply gate does not apply), arch-docs against a base (this copy has no git; the owned doc is updated in this diff), the smoke test (no index.js or core/src/host/ change), and GitHub CI. CI tests-ok runs on this tip.

Checkpoint

{
  "pr": 1602,
  "claim": "Chief",
  "issue": 1472,
  "summary": "Workflow dispose() released the Bots of only the run archived last, because archiveCutoffRun drops a run from the lane's active list and makes it last, so a collection after the loop named one run per lane. A lane with two concurrent runs therefore released 2 of its 4 Bots, and write() erased the other two: hidden Bots no saved state names. The replay snapshots each lane's runs before the archive loop, so every live run's ids reach releaseAll before the record drops ephemeral. Ending behaviour is unchanged: in-flight nodes fail, each run is archived failed, and history keeps one row per run.",
  "files": [
    "plugins/workflow/src/service.js",
    "plugins/workflow/test/dispose.test.js",
    "docs/subsystems/workflow.md",
    "changelog.d/1472-workflow-dispose-release-all.fixed.md"
  ],
  "checks": {
    "ran": "plugins/workflow: node --test test/dispose.test.js (5/5), node --import ./test/resolve.mjs --test \"test/*.test.js\" (212/212); core/test/check-doc-paths.test.js (2/2); node .github/scripts/changelog.mjs check; red on main 2f1be87e with only the new test (4/5)",
    "result": "green",
    "at": "8078d6aee8cccd9981d97cb3ecc696820153a688"
  },
  "next": "ready for review: T2 needs a PASS at this tip; the queue entry then re-reads this body and re-judges the merge tree"
}

@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 424378c
Written-by: Reviewer
Attestation: 424378c role=Reviewer

Ran at this tip:

$ cd plugins/workflow && node --import ./test/resolve.mjs --test test/dispose.test.js
# tests 5  # pass 5  # fail 0

$ cp rev1602-service-base.js plugins/workflow/src/service.js && cd plugins/workflow && node --import ./test/resolve.mjs --test test/dispose.test.js
# tests 5  # pass 4  # fail 1

Red first, once. The tip's plugins/workflow/src/service.js is blob aade95871a; with base blob 99ce05d0e4 (main 571be6d4) restored, only #1472: dispose() releases the Bots of every concurrent live run, not just the last one fails, then the tip blob is put back and the same command is green again.

The tree I ran is the tip itself, not a re-typed copy: all four changed blobs hash to the tip — service.js aade95871a, test/dispose.test.js a3974dbbd5, docs/subsystems/workflow.md a4b4b70ff7, changelog.d/1472-workflow-dispose-release-all.fixed.md cf539ed727 — and the base blob I swapped in hashes to 571be6d4's service.js.

At this tip tests-ok, plugins (tree), plugins (social), plugins (workflow), plugins (timeline), package and merge-gate are all success; the one red check is enqueue-guard, which is refusing the missing Verdict this comment supplies.

Design conformance: the snapshot const live = [...(lane.active ?? [])] sits above the archive loop, so every live run's ids reach releaseAll before the record drops ephemeral/handed; main's endRun(…, { code: 'host-ended' }) and timer clear are kept, and the ending half is unchanged. The owned subsystem doc and the changelog fragment are in the diff.

Not run: the full npm run verify (only plugins/workflow and docs/ changed, no core/ or patches/ movement), the workflow suite as a whole, and the smoke test (no index.js or core/src/host/ change).

@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 4dac06f
Written-by: Reviewer
Attestation: 4dac06f role=Reviewer

Ran at this tip:

$ cd /data/dsh/bot/workspace/replay-1602/tip/plugins/workflow && node --import ./test/resolve.mjs --test test/dispose.test.js
# tests 5  # pass 5  # fail 0

$ cp /data/dsh/bot/workspace/replay-1602/main/plugins/workflow/src/service.js /data/dsh/bot/workspace/replay-1602/tip/plugins/workflow/src/service.js && cd /data/dsh/bot/workspace/replay-1602/tip/plugins/workflow && node --import ./test/resolve.mjs --test test/dispose.test.js
# tests 5  # pass 4  # fail 1

Red first, once: the tip's plugins/workflow/src/service.js is blob aade95871a; with main's blob 99ce05d0e4 restored, only #1472: dispose() releases the Bots of every concurrent live run, not just the last one fails, and the tip blob put back is green again (5/5).

The tree I ran is the tip itself: all four changed blobs hash to its own tree entries — service.js aade95871a, test/dispose.test.js a3974dbbd5, docs/subsystems/workflow.md fdd1de8274, changelog.d/1472-workflow-dispose-release-all.fixed.md cf539ed727.

Rework conformance: 4dac06f is main 43502f99bb plus the same four-file change. The snapshot const live = [...(lane.active ?? [])] sits above the archive loop, so every live run's ids reach releaseAll before the record drops ephemeral/handed; main's endRun(…, { code: 'host-ended' }) and timer clear are kept, and the ending half is unchanged. The owned subsystem row keeps main's wording and gains the #1472 clause. The head does not include main 2f1be87e (#1592, graph.js + docs) and GitHub reports mergeable=true; the queue judges the merge tree, not this page.

At this tip tests-ok, plugins (tree), plugins (social), plugins (workflow), plugins (timeline), package and merge-gate are all success; the one red check is enqueue-guard, refusing the missing Verdict this comment supplies.

Not run: the full npm run verify (only plugins/workflow and docs/ changed), the workflow suite as a whole, and the smoke test (no index.js or core/src/host/ change).

@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Written-by: Enqueue 90b8

Tip rebuilt on current main: 8078d6ae replaces 424378c6 (cut on 571be6d4). #1602 read CONFLICTING / dirty against 2f1be87e, and the fork head cannot be pushed, so replay/pr-1474 was rebuilt from 2f1be87e's tree with the same change set: 4 files, +52/−3, per-file numstat unchanged, and docs/subsystems/workflow.md keeps main's newer rows.

Local at 8078d6ae: dispose.test.js 5/5, plugins/workflow suite 212/212, core/test/check-doc-paths.test.js 2/2, changelog check 12 fragments well formed, and red on main 2f1be87e with only the new test (4/5, the #1472 case). CI at this tip: tests-ok green.

No PASS names this tip — the PASSes at 424378c6 and 4dac06fd name older shas, which is exactly what enqueue-guard refuses on. This tip needs a fresh PASS; nothing in this comment is a verifier conclusion.

@Birfy

Birfy commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

Verdict: PASS at 8078d6a
Written-by: Reviewer
Attestation: 8078d6a role=Reviewer

Ran at this tip:

$ cd /data/dsh/bot/workspace/rev1602b/plugins/workflow && node --import ./test/resolve.mjs --test test/dispose.test.js
# tests 5  # pass 5  # fail 0

$ cd /data/dsh/bot/workspace/rev1602b/plugins/workflow && DSH_HARNESS=/data/dsh/bot/workspace/rev1602/core/dev/harness node --import ./test/resolve.mjs --test "test/*.test.js"
# tests 212  # pass 212  # fail 0

$ cd /data/dsh/bot/workspace/rev1602b/plugins/workflow && cp /data/dsh/bot/workspace/rev1602b-service-base.js src/service.js && node --import ./test/resolve.mjs --test test/dispose.test.js
# tests 5  # pass 4  # fail 1

Red first, once. The tip's plugins/workflow/src/service.js is blob aade95871a; with main 2f1be87e's blob 99ce05d0e4 restored, only #1472: dispose() releases the Bots of every concurrent live run, not just the last one fails, then the tip blob is put back and the suite is green again.

The tree I ran is the tip, not a re-typed copy: the tarball of 8078d6ae is 4 files in the diff, and all four changed blobs hash to the tip — service.js aade95871a, test/dispose.test.js a3974dbbd5, docs/subsystems/workflow.md 38248a9d9e, changelog.d/1472-workflow-dispose-release-all.fixed.md cf539ed727. The base blob I swapped in is 99ce05d0e4 from 2f1be87e.

This is a new conclusion, not a repeat: docs/subsystems/workflow.md moved from a4b4b70ff7 to 38248a9d9e, so the PASSes at 424378c6 and 4dac06fd describe a different change set and do not carry (#1317).

At this tip tests-ok (122946590059), plugins (workflow), plugins (timeline), plugins (social), plugins (tree), package, Node 24 · DSH 0.2.0-rc.2, merge-gate and both report only checks are success; the one failure is enqueue-guard (122945510170), which refuses because no Verdict names this tip — the gap this comment fills. mergeable: true.

Design conformance: const live = [...(lane.active ?? [])] is snapshotted above the archive loop, so every live run's ids reach releaseAll before write() drops ephemeral/handed; main's endRun(workflow(workflowId), run, { code: 'host-ended' }) and timer clear are kept, the ending half is unchanged, and history still records one row per run. The owned subsystem doc (docs/subsystems/workflow.md, the #1387, #1414, #1472 clause) and the changelog fragment are in this diff. Scope is exactly the four intended files, +52/−3.

Not run: the full npm run verify (only plugins/workflow and docs/ change; no core/ or patches/ movement), the core suite, and the smoke test (no index.js or core/src/host/ change).

@Birfy
Birfy added this pull request to the merge queue Oct 28, 2026
Merged via the queue into main with commit fbbf3c0 Oct 28, 2026
14 checks passed
@Birfy
Birfy deleted the replay/pr-1474 branch October 28, 2026 16:50
@github-actions github-actions Bot removed the agent-pr Pull request opened by an agent label Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(workflow): dispose() releases only the last run's Bots when one lane has two concurrent runs

1 participant