fix(dispatch): runWorkDispatch must throw not exit — preserves sibling spawns in wave dispatch - #1602
Conversation
…ing spawns ROOT CAUSE of the long-running #1589/#1600 phantom dispatch saga, found empirically on .25 after PR #1601's audit observability shipped: `runWorkDispatch` called `process.exit(1)` on three error paths (wish not found, group not found, startGroup failed). When `autoOrchestrateCommand` runs `Promise.allSettled([runWorkDispatch × N])` for a parallel wave, ANY group's process.exit terminates the entire node process — killing every sibling spawn mid-flight before: - handleWorkerSpawn's worker.spawn audit event fires - launchTmuxSpawn reaches the new validateSpawnedPane check - any of the new worker.spawn.failed / worker.spawn.ok events fire This explains why .24 + .25 still phantom-dispatched despite the `wish.dispatch.work` event firing correctly: the dispatch event landed during the brief window between Group N's PG state mutation and Group N's spawn pipeline being killed by Group N+1's exit(). The fix: throw `new Error(...)` on each runWorkDispatch error path. Both callers handle the throw correctly: - `workDispatchCommand` (single-group CLI): the outer commander handler in registerDispatchCommands already wraps in try/catch + process.exit, so single-group semantics are preserved. - `autoOrchestrateCommand` (wave): Promise.allSettled collects rejections into the `failed` array, my Round 2 (#1601) wish.dispatch.failed event fires per group, and stderr summary prints with the [ErrorClass] prefix. Empirical confirmation: Before: `genie work tui-bottom-bar-opentui` exits silently after Group 5's dependency error kills the process. No worker.spawn / .ok / .failed events fire for in-flight Group 1. After (this fix): Group 5's startGroup throws; Promise.allSettled collects the rejection. Group 1's spawn pipeline runs to completion, emitting either worker.spawn.ok (success) or worker.spawn.failed (real spawn failure surfaced). Tests: - 1 new regression-guard in dispatch.test.ts (#1600 Group 4): asserts zero EXECUTABLE process.exit calls in runWorkDispatch (comment text mentioning the OLD behavior is excluded), and >=3 throw-new-Error statements covering wish-not-found, group-not-found, startGroup-failed. - Total: 86 pass / 0 fail in dispatch.test.ts. - bun run typecheck: clean. Followup to #1599 + #1601. Should close the root-cause iteration on the long-running #1589/#1600 phantom-dispatch saga. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the runWorkDispatch function to throw errors instead of calling process.exit(1) to prevent sibling process termination during parallel execution, and adds a regression test to enforce this. Feedback suggests that the fix is incomplete as downstream functions still contain exit calls, and recommends removing a redundant try-catch block that discards original error information.
| // CRITICAL (#1600 Round 3): never call `process.exit(1)` from this function. | ||
| // `runWorkDispatch` is invoked from TWO callers: | ||
| // 1. `workDispatchCommand` (single-group CLI) — wraps in try/catch and the | ||
| // outer CLI handler calls process.exit(1) with the error message. | ||
| // 2. `autoOrchestrateCommand` (wave dispatch via Promise.allSettled) — | ||
| // collects rejections and reports per-group failures. | ||
| // If we exit() here from within a parallel wave, we kill the whole node | ||
| // process mid-dispatch — including SIBLING spawns that are mid-flight in | ||
| // handleWorkerSpawn. That's THE silent-fail surface from #1589/#1600: | ||
| // any group's startGroup failure (dependency not done, already in_progress) | ||
| // killed every other group's spawn before audit events could fire. Throw | ||
| // instead so the wave-dispatcher's allSettled boundary handles it. |
There was a problem hiding this comment.
While this PR correctly replaces direct process.exit(1) calls in runWorkDispatch with throws, the goal of preventing sibling spawn termination is not fully achieved. The function still calls handleWorkerSpawn (line 743), which contains several process.exit(1) paths in its downstream call chain (specifically in resolveTeamAndResumeOrExit and launchTmuxSpawn within agents.ts). If any group dispatch hits one of these paths, the entire Node process will still be terminated, killing all sibling spawns mid-flight. To fully resolve the issue, those downstream functions should also be refactored to throw errors instead of exiting.
| try { | ||
| await wishState.startGroup(slug, group, agentName); | ||
| console.log(`✅ Group "${group}" set to in_progress (assigned to ${agentName})`); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| console.error(`❌ ${message}`); | ||
| process.exit(1); | ||
| throw new Error(message); | ||
| } |
There was a problem hiding this comment.
This try-catch block is redundant as it simply catches an error to re-throw it as a new generic Error. This discards the original error's stack trace and type. Since wishState.startGroup is known to throw Error objects and the callers of runWorkDispatch already handle errors (either by printing them or by inspecting the error class), it is better to let the original error bubble up naturally. Note that if you apply this change, you will also need to update the regression guard test in dispatch.test.ts which currently asserts a minimum count of explicit throw new Error( calls.
await wishState.startGroup(slug, group, agentName);
console.log("✅ Group " + group + " set to in_progress (assigned to " + agentName + ")");
1 participant
Summary
ROOT CAUSE of the long-running #1589/#1600 phantom-dispatch saga, found empirically on
4.260430.25after PR #1601's audit observability shipped.runWorkDispatchcalledprocess.exit(1)on three error paths (wish not found, group not found, startGroup failed). WhenautoOrchestrateCommandrunsPromise.allSettled([runWorkDispatch × N])for a parallel wave, ANY group'sprocess.exitterminates the entire node process — killing every sibling spawn mid-flight before:handleWorkerSpawn'sworker.spawnaudit event fireslaunchTmuxSpawnreaches the newvalidateSpawnedPanecheck from fix(spawn): #1600 — surface silent-fail spawn pipeline (validation + audit events) #1601worker.spawn.failed/worker.spawn.okevents fireThis is exactly why .24 + .25 still phantom-dispatched despite the
wish.dispatch.workevent firing correctly: the dispatch event landed during the brief window between Group N's PG state mutation and Group N's spawn pipeline being killed by Group N+1's exit().Repro on .25 (with #1601 audit code installed)
The
wish.dispatch.workevent fires (Group 1 reached dispatch.ts) but neitherworker.spawn(attempt) nor any.ok/.failedterminal event fires — proof that handleWorkerSpawn was killed mid-flight by Group 5'sprocess.exit(1).Fix
Replace each
process.exit(1)inrunWorkDispatchwiththrow new Error(...). Both callers handle the throw correctly:workDispatchCommand(single-group CLI): the outer commander handler inregisterDispatchCommandsalready wraps in try/catch +process.exit, so single-group semantics are preserved.autoOrchestrateCommand(wave):Promise.allSettledcollects rejections into thefailedarray; PR fix(spawn): #1600 — surface silent-fail spawn pipeline (validation + audit events) #1601'swish.dispatch.failedevent fires per group; stderr summary prints with the[ErrorClass]prefix.Test plan
bun test src/term-commands/dispatch.test.ts— 86 pass / 0 fail (was 85 + 1 new Group-4 regression-guard asserting zero EXECUTABLEprocess.exitcalls inrunWorkDispatch, with comment text excluded).bun run typecheck— clean.genie work tui-bottom-bar-opentuiproducesworker.spawn.okfor surviving groups ORworker.spawn.failedwith structured reason for actual spawn failures. Group 5's "dependency not done" no longer kills siblings.Files
Saga
This is the third PR in the #1589 → #1600 → (this) chain:
runWorkDispatchkilling sibling spawns🤖 Generated with Claude Code