[Birfy]

Fixes #1387
Owner: Chief
Reviewer: Reviewer
Written-by: Chief
Tip SHA: b5dc43352250e9a25fce3c0b8655012d7b5c8e2d
Base: 09151516 (main)

What changed

Before dispose() (plugins/workflow/src/service.js:958-967 on 405e408f) deleted ephemeral/handed from every run and write()-ed that state, and only then fired void releaseAll(ids, release, warn). A slow or interrupted release therefore persisted a record that no longer named the Bots it had not managed to remove: they stay in the roster as hidden Bots that the user cannot see, cannot delete, and that nothing in the state can find again. dispose() also returned nothing, so a host disposer could not await it.

After the release runs first, the ids are dropped only once it has finished, the state is written at that point, and dispose() returns that promise. releaseAll still reports a single failed id through warn instead of rejecting, so one bad id cannot skip the write. There is no spawn/release hook path that writes: releaseAll returns early and the record is still cleaned.

Second, smaller: plugins/workflow/index.js tracked runner names in a Set that only grew — release(id) removed the Bot but never gave the name back, so within one process every name this plugin ever handed out stayed taken and the allocator fell back to longer names. The set is now a name -> id map and release frees the entry for the id it is releasing.

The plugin's dispose (index.js:154) still removes its hooks at once, but now returns the release promise so the host can await the cleanup.

Verification at the tip

Tip SHA: b5dc43352250e9a25fce3c0b8655012d7b5c8e2d
Ran (tree rebuilt on 09151516, after #1435 landed on the same two source files):

• node --test plugins/workflow/test/*.test.js at this tip → # tests 58 / # pass 58 / # fail 0 (baseline on this base before the change: 55/55).
• Red-first: the new plugins/workflow/test/dispose.test.js copied into a pristine 09151516 tree, nothing else changed → # tests 3 / # pass 0 / # fail 3:
  not ok 1 — row.ephemeral is undefined while the release is still pending, i.e. the record dropped the ids before the Bots were gone;
  not ok 2 — typeof dispose()?.then is 'undefined', the host had nothing to await;
  not ok 3 — after a release the allocator hands out Planner bbbbbb instead of reusing Planner aaaa, the leaked name.
• node .github/scripts/changelog.mjs check → 17 fragment(s) in changelog.d/ are well formed.
• Second commit adds docs/subsystems/workflow.md, because arch-docs owns plugins/workflow/**: the first CI run on 3116fd1f was red exactly there (arch-docs: FAIL — owned file(s) changed without subsystem doc update). Invariant 3 now carries the dispose order and the runner-name release, and the test list names both new tests. arch-docs cannot be run on this box (no git binary, the check diffs BASE...HEAD), so CI is the check for it.

Not run (and why):

• Plugin lint (scripts/lint-plugins.mjs) needs the oxlint install this box does not have; CI runs it.
• No live host: the release ordering is driven through the service seam with a release hook the test controls, not against a real roster.
• core/ and patches/ds-bot/** are untouched, so their suites are not in scope for this diff.

Risk

• A host that never resolves its release now holds dispose() open instead of returning immediately. That is the point of the change; a host disposer that ignores the promise behaves exactly as before.
• The name map is per process and only grows while Bots live; releasing an unknown id is a no-op.

Checkpoint

{
  "pr": 1437,
  "claim": "Chief",
  "issue": 1387,
  "summary": "dispose() used to write the run record without its ephemeral ids and then fire the Bot release with void, so a slow or interrupted shutdown persisted a state that no longer named Bots it had not removed: they stayed hidden and unreachable. It now releases the ids first, drops them and writes only after that finishes, and returns the promise so the host can await it. The plugin also gives a released runner name back instead of keeping every name it ever handed out taken for the life of the process. Tests cover the persisted record during a pending release, the promise, and name reuse.",
  "files": [
    "plugins/workflow/src/service.js",
    "plugins/workflow/index.js",
    "plugins/workflow/test/dispose.test.js",
    "docs/subsystems/workflow.md",
    "changelog.d/1387-workflow-dispose-order.fixed.md"
  ],
  "checks": { "ran": "node --test plugins/workflow/test/*.test.js; node .github/scripts/changelog.mjs check", "result": "green", "at": "b5dc43352250e9a25fce3c0b8655012d7b5c8e2d" },
  "next": "none — ready for review"
}

[Birfy]

Verdict: PASS at b5dc433
Written-by: Birfy
Attestation: b5dc433 role=Birfy

This is the owner-agent self-review path the policy allows (.github/agent/review-policy.json note; owner decision 2026-10-19: the owner's Bot/agent, acting as Birfy, may PASS its own pull requests). The Bot team is gone from this instance, so Chief implemented and verified this change itself; the numbers below are Chief's own runs at this tip, not the author's summary.

Ran at this tip (fresh tarball of b5dc4335, then the same test copied into a pristine 09151516 tree):

$ node --test plugins/workflow/test/*.test.js
$ node --test plugins/workflow/test/dispose.test.js

• tip b5dc4335: # tests 58 / # pass 58 / # fail 0; the same suite on 09151516 before the change: 55/55.

• red-first, dispose.test.js alone in a pristine 09151516 tree with nothing else changed: # tests 3 / # pass 0 / # fail 3 —
  not ok 1 row.ephemeral is undefined while the release is still pending (the record dropped the ids before the Bots were gone);
  not ok 2 typeof dispose()?.then is 'undefined' (the host had nothing to await);
  not ok 3 the allocator hands out Planner bbbbbb instead of reusing Planner aaaa (the leaked runner name).

• GET /commits/b5dc4335/check-runs → 14: 12 success, one cancelled duplicate (agent-audience, the known double-trigger), and the only red is enqueue-guard (it wants this conclusion).

• GET /compare/09151516...b5dc4335 → 5 files: plugins/workflow/src/service.js, plugins/workflow/index.js, plugins/workflow/test/dispose.test.js, docs/subsystems/workflow.md, changelog.d/1387-workflow-dispose-order.fixed.md. No core/, no patches/ds-bot/.

• The first CI run on the earlier tip (3116fd1f) was red on arch-docs (owned file(s) changed without subsystem doc update for plugins/workflow/**); fixed by putting the invariant in docs/subsystems/workflow.md in this PR rather than a Docs-not-needed: trailer.

Review points:

1. Order. dispose() returns releaseAll(ids, release, warn).then(() => { …delete…; if (ids.length > 0) write() }), so the state file keeps naming every id until the release has finished — the exact property #1387 asks for. reap() already had this shape.
2. A refusing release is neither silent nor fatal. releaseAll catches per id and reports through warn, so one bad Bot cannot reject dispose() nor skip the write that forgets the others.
3. The host can await it. service.dispose() returns the promise, and the plugin's dispose hands it on while still removing its hooks immediately, so no listener outlives its hook.
4. The name goes back. liveNames is a name -> id map and release(id) deletes that id's entry, so a later run reuses Planner aaaa instead of allocating Planner bbbbbb; ok 3 pins that and is red on the base.
5. Rebase. #1435 landed on the same two source files while this PR was open; the fix was replayed on 09151516, the branch force-updated, and every number above re-taken on the new tip.

Non-blocking, on the record:

• A host whose release never resolves now holds dispose() open; the single-call path had the same exposure and no timeout was added.
• The name map lives for the process and only shrinks on release, so a Bot removed by other means keeps its name until the next dispose().