feat: the dispatch barrier — N members, one joined result (P3 panel) - #268
Merged
Conversation
The one genuinely new dispatch primitive in the collaborative-session design
(§7 step 3). Today's dispatcher does per-task event injection; a review
panel needs an N-way join, because synthesis has to see every verdict at
once. N completions arriving one at a time give the orchestrator N chances
to synthesize from partial input — a panel silently degraded into a race.
## The barrier
`dispatch_tasks` gains `group_id` + `group_ordinal` (both additive; NULL
means standalone, which is every task queued before panels existed and
keeps its current behaviour exactly). Members are inserted in ONE
IMMEDIATE transaction: a crash mid-fan-out would otherwise leave a group
smaller than the panel the owner approved, and the barrier would fire on a
quorum nobody asked for.
`#barrierAbsorb` swallows each member's individual completion; the member
that observes the LAST terminal transition emits one merged event.
It joins on **all-terminal, not all-done**. Waiting for success would hang
a goal on its weakest member, and §7 is explicit that disagreement is shown
rather than hidden — a failed reviewer belongs in the merged digest as a
failure, not dropped and not blocking its peers forever.
The merge is mechanical only: collect, label, order. §7 forbids a silent
auto-vote, so there is no tallying here — the judgement is the
orchestrator's synthesis step or a human's. `group_ordinal` is stored
rather than derived because every member shares one `created_at`, so a
timestamp sort falls back to the random UUID and the "member N of 3" labels
would shuffle between reads.
## What the barrier forced: grouped sends join on WORK, not delivery
This is the part that isn't obvious. An ungrouped `send` completes the
moment the prompt is handed over — `dispatchComplete("delivered to
session …")`. A barrier over that fires the instant all N briefs are
delivered, before a single reviewer has read anything. The panel would join
on nothing.
So a GROUPED send is now watched to completion like a spawn: mark running,
watch the target, finish on its idle transition. Ungrouped sends are
untouched — nobody is joining on those. That change drags in three of the
dispatcher's crash-safety invariants, and each one fails silently:
1. **`#finishWorkerTask` destroyed the session unconditionally** on its
done path. Grouped sends are the FIRST sends ever routed through there,
and a send's target is a long-lived ROLE-CHILD — so every joining panel
would have dismantled its own fleet. Now guarded on `kind === "spawn"`,
with a contrast test proving a spawned worker is still destroyed so the
guard can't be over-applied into a leak.
2. **Re-delivery is not idempotent.** After a restart the task is requeued
and re-executed; blindly re-sending hands the reviewer its brief twice.
`workerSessionId` being set is the marker that delivery already
happened, so `#startGroupedSend` re-WATCHES instead.
3. **The turn may have finished while the daemon was down.** Re-watching an
already-idle target waits for a transition that never comes, until the
lease expires and burns an attempt. So current status decides: still
working → watch; already settled → finish now; gone → fail
non-retryably, and the barrier joins with that member marked failed.
## fleet_panel
Send-class by construction, which is what puts it behind the R3 approval
gate and — for a collaborative session — carries the goal's cost roll-up
into that prompt for free. Granted to the orchestrator, scoped in the DEPS
like its siblings: every target must be one of its own children, checked
where a second caller of that closure cannot route around it.
Unknown or foreign targets fail the WHOLE fan-out rather than running a
smaller panel. The owner approved a panel of N, and quietly reaching a
2-of-3 panel is exactly the silent degradation §7 warns about.
The orchestrator's constitution now states both halves of the contract:
prefer a panel when the answers belong together and do not synthesize
before the join arrives; then read each role's artifact, merge, and SHOW
disagreement rather than resolving it silently.
## Verification
Daemon suite 2149 to 2163. Six mutations, each caught by exactly the tests
that should catch it: the destroy guard removed (a joined panel tears down
its role-children), grouped sends completing on delivery, the barrier
firing on the first terminal member, waiting for all-DONE instead of
all-terminal, a restart re-sending the brief, and the panel scoping
removed.
One test caught a false claim in my own comment: `dispatchGroupMembers`
documented fan-out order while ordering by `created_at` — identical across
a group, so it fell back to random UUID order. That is what `group_ordinal`
exists for.
Not in this PR: the pipeline `panel` phase kind (§8 item 4). That is engine
work in `pipeline/` rather than dispatcher work, and it now has a barrier to
build on.
…side CREATE TABLE On an existing database `CREATE TABLE IF NOT EXISTS` is a no-op, so a partial index declared inside that block ran against a table without the column and SQLite failed the whole migration: "no such column: group_id". The daemon then refused to open a database it had been using — every existing install would have been bricked on upgrade. Every unit test builds a FRESH database, where CREATE declares every column and the ALTER path never runs, so the entire suite was structurally blind to it. A live upgrade probe (old binary writes the DB, new binary opens it) caught it. `dispatch-store.test.ts` now reproduces the upgrade path by dropping the columns from a built database and reopening it, so the additive-migration path is covered by tests rather than by remembering to run a probe. Mutation-checked: reinstating the original ordering fails all three.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Collaborator
Author
|
pr-shepherd: CI is green at All 4 required/reported checks concluded SUCCESS on the PR head SHA (
The three Notably the new migration tests in Branch hygiene: 2 commits ahead of Remaining gate is human review only: the |
akashjavelin
approved these changes
Jul 29, 2026
…hree barrier defects an audit found A full audit of the collaborative-sessions feature turned up one Critical and two High defects. All four fixes here; the Critical one also repairs already-merged behaviour from #267. ## Critical — an orchestrator never received its own dispatch results `deliverEvents` resolved its recipient with `#conductorFor()` — the tenant's CONDUCTOR — for every event. But a collaboration orchestrator queues its own tasks (`fleet_send` since #267, `fleet_panel` here) and they are attributed `orchestrator:<goalId>`. With no conductor on the daemon — the common case, since one is created only on an explicit `session.create` with `role:"conductor"`, never at boot — the host returned false and the events were retried forever. Reproduced: a joined panel left `pending: 1 ["group_done"]` after a tick, permanently. With a conductor present it was wrong differently: the panel's result landed in the CONDUCTOR's session and the orchestrator still never learned its panel had joined. So the entire §7 barrier→synthesis loop was unreachable. Everything upstream worked; the last mile went to the wrong session. Events now route by `task.createdBy`: an orchestrator-attributed task delivers to that goal session, everything else to the conductor as before. Attribution is parsed through `goalIdFromCreatedBy`, paired with the `orchestratorCreatedBy` that writes it, because this bug WAS the drift between an inline template string on one side and a `startsWith` on the other. `deliverEvents` now returns the ids it delivered rather than a boolean. With several possible recipients, "one is mid-turn" is a normal state, and all-or-nothing would either stall an idle recipient or re-deliver its events later as duplicates. Events whose collaboration no longer exists are retired with an audit line instead of being retried forever. ## High — the barrier swallowed the "member is wedged" notice `#emitEvent` is also how a `waiting_approval` wedge is surfaced, and `#barrierAbsorb` swallowed it: the group was not terminal, so the notice was absorbed and never written. A panel member that exhausted its tool budget went unreported and the panel hung to lease expiry — and that notice is the one message whose entire purpose is to reach a human. The barrier now absorbs only THIS member's terminal completion. Keying on the member's own status rather than on the event type keeps the rule in one place: a non-terminal task has not completed, so it is not the barrier's business. ## High — two live tasks targeting one session evicted each other `#watched` was `Map<sessionId, taskId>`. Every key used to be a freshly created spawn worker, unique by construction — but a grouped send watches a PRE-EXISTING session, and the same role-child can be the target of two live dispatches (a second panel, or a plain `fleet_send` alongside a running one). The second registration silently evicted the first, whose task then sat in `running` until the lease expired, hanging its barrier for the whole lease. Now `Map<sessionId, Set<taskId>>` behind `#watch`/`#unwatch`/`tasksForSession`, with every watcher completing on the shared transition and every watcher's lease renewed. ## Medium — duplicate-delivery window on a grouped send `dispatchMarkRunning` now runs BEFORE `sendToSession`. `running` is already a reclaimable state so ordering it first costs nothing, whereas marking after left a window where a crash between delivery and the marker made the retry re-send — the reviewer working its brief twice with the digest describing only the second pass. ## Why the suite could not see any of this `dispatcher.test.ts` drives a FakeHost whose `deliverEvents` always accepts, so every barrier test passed against events that were undeliverable in production. A fake that rubber-stamps the last mile cannot see a last-mile bug. The new routing tests run against the REAL host in `dispatch-host.test.ts`. Daemon suite 2166 to 2172. Six mutations caught by exactly the right tests: conductor-only routing, all-or-nothing event marking, the watched collision, the absorbed wedge notice, plus the earlier barrier set. The mark-running ordering is NOT covered: it is a two-statement crash window with no seam to inject a crash, so it rests on argument rather than a test. Mutation testing also caught a flaw in these very tests. The first draft asserted `pending().length === 0`, which a RETIRED event satisfies just as well as a delivered one — so reverting to conductor-only routing passed. They now assert a completed turn on the specific recipient session, since draining a queue is not delivering to anybody.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The N-way join from
docs/collaborative-session-design.md§7 — "the one genuinely new dispatch primitive (today dispatch is per-task event injection, not an N-way join)" — plus thefleet_panelverb that uses it and the synthesis contract that consumes it.Two commits: the barrier, and a migration fix that a live upgrade probe caught.
1.
f05bce2— the barrierdispatch_tasksgainsgroup_id+group_ordinal, both additive. NULL means standalone, which is every task queued before panels existed — those keep emitting their own completion event, unchanged.#barrierAbsorbswallows each member's individual completion; whichever member observes the last terminal transition emits one merged event.Design decisions worth reviewing rather than skimming:
IMMEDIATEtransaction. A crash mid-fan-out would leave a group smaller than the panel the owner approved, and the barrier would then fire on a quorum nobody asked for.group_ordinalis stored, not derived. Every member shares onecreated_at, so a timestamp sort falls back to the random UUID and the "member N of 3" labels would shuffle between reads. My own comment claimed fan-out order while the code didn't deliver it; a test caught the lie.What the barrier forced: grouped sends join on WORK, not delivery
This is the part that isn't obvious from the design doc. An ungrouped
sendcompletes the moment the prompt is handed over —dispatchComplete("delivered to session …"). A barrier over that fires the instant all N briefs are delivered, before a single reviewer has read anything. The panel would join on nothing.So a grouped send is now watched to completion like a spawn. Ungrouped sends are untouched — nobody joins on those. That change pulls in three crash-safety invariants, and each fails silently:
#finishWorkerTaskdestroyed the session unconditionally on its done pathkind === "spawn", with a contrast test proving a spawned worker is still destroyed so the guard can't be over-applied into a leak.workerSessionIdbeing set marks that delivery already happened, so it re-watches.fleet_panelSend-class by construction, which is what puts it behind the R3 approval gate and — for a collaborative session — carries the goal's cost roll-up into that prompt for free. Granted to the orchestrator and scoped in the deps like its siblings: every target must be one of its own children, checked where a second caller of that closure cannot route around it.
Unknown or foreign targets fail the whole fan-out rather than running a smaller panel. The owner approved a panel of N, and quietly reaching a 2-of-3 panel is exactly the silent degradation §7 warns about.
The orchestrator's constitution now states both halves of the contract: prefer a panel when the answers belong together and do not synthesize before the join arrives; then read each role's artifact, merge, and show disagreement rather than resolving it silently.
2.
900df32— the migration bug, and why the suite couldn't see itI declared the partial index on
group_idinside theCREATE TABLE IF NOT EXISTSblock. On an existing database that block is a no-op, soCREATE INDEX … ON dispatch_tasks(group_id)ran beforeALTER TABLE ADD COLUMNand SQLite failed the whole migration:The daemon then refuses to open a database it had been using happily. Every existing install would have been bricked on upgrade.
Every unit test builds a fresh database, where
CREATEdeclares every column and the ALTER path never runs — so all 2163 tests were structurally blind to it. What found it was a live probe: build the previous binary, let it write a database, then open that file with the new one.The fix moves the index creation after the ALTERs. More importantly,
dispatch-store.test.tsnow reproduces the upgrade path — it strips the columns from a built database and reopens it — so this class of bug is covered by tests rather than by remembering to run a probe. Mutation-checked: reinstating the original ordering fails all three of the new cases.Verification
Daemon suite 2149 → 2166. biome + typecheck clean; web untouched at 370.
Seven mutations, each caught by exactly the tests that should catch it:
NEVER destroys a panel member — they are long-lived role-childrenwaits for the target's TURN, not for deliverystays silent until the last member finishesone failed member cannot hang the panelre-watches after a restart instead of re-sendinga panel must target only its own childrenCREATE TABLEMeasured in a detached worktree at the commit, not in my working tree — there was concurrent uncommitted work in the repo, so a tree-based measurement wouldn't have been a clean read of this change alone.
Live-verified old-binary → new-binary upgrade: columns added in place, the pre-upgrade session survives, the index lands on the upgraded database, a collaboration creates on it, 3 children spawn, and a group of 3 persists with stable ordinals 1/2/3.
Not in this PR
The pipeline
panelphase kind (§8 item 4) — engine work inpipeline/rather than dispatcher work, and it now has a barrier to build on. Also unchanged:reads/writesas live scheduling edges (P4), and the per-collaboration token budget (P6), which is the enforcement counterpart to the cost visibility shipped in #267.🤖 Generated with Claude Code