Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/term-commands/dispatch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1238,6 +1238,39 @@ describe('#1600 spawn-pipeline silent-fail regression guards', () => {
});
});

describe('Group 4 — runWorkDispatch must never process.exit (kill-siblings prevention)', () => {
it('runWorkDispatch contains zero EXECUTABLE process.exit calls — they kill sibling spawns in Promise.allSettled', () => {
// ROOT CAUSE of the long-running #1589/#1600 phantom dispatch: when
// `autoOrchestrateCommand` runs `Promise.allSettled([runWorkDispatch, …])`
// for a parallel wave, ANY group's process.exit(1) terminates the entire
// node process — killing every sibling spawn mid-flight before audit
// events can fire. The fix: throw instead of exit. The single-group CLI
// caller `workDispatchCommand` already re-throws and the outer CLI
// handler does process.exit, so single-group semantics are preserved.
const fnAnchor = dispatchSrc.indexOf('async function runWorkDispatch');
expect(fnAnchor).toBeGreaterThan(-1);
// Body ends at the next top-level `export async function` or `async function`.
// Use a regex that anchors on lines starting with `}` followed by a blank line then a docstring.
const afterFn = dispatchSrc.slice(fnAnchor + 1);
const nextFnRel = afterFn.search(/\nasync function |\nexport async function |\nexport function |\nfunction /);
const fnEnd = nextFnRel !== -1 ? fnAnchor + 1 + nextFnRel : dispatchSrc.length;
const body = dispatchSrc.slice(fnAnchor, fnEnd);
// Strip comments before checking — `process.exit(1)` may legitimately
// appear in JSDoc / inline comments explaining the OLD behavior.
const stripped = body
.split('\n')
.filter((line) => !line.trim().startsWith('//') && !line.trim().startsWith('*'))
.join('\n');
// Hard guarantee: zero EXECUTABLE process.exit calls in non-comment code.
expect(stripped).not.toContain('process.exit');
// Soft guarantee: the function explicitly throws on each error path so
// Promise.allSettled in autoOrchestrateCommand collects rejections.
const throwCount = (stripped.match(/throw new Error\(/g) ?? []).length;
// At minimum: wish-not-found, group-not-found, startGroup-failed.
expect(throwCount).toBeGreaterThanOrEqual(3);
});
});

describe('Group 3 — wish.dispatch.failed surfacing', () => {
it('autoOrchestrateCommand emits wish.dispatch.failed per failed group', () => {
const fnAnchor = dispatchSrc.indexOf('async function autoOrchestrateCommand');
Expand Down
32 changes: 19 additions & 13 deletions src/term-commands/dispatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -656,38 +656,44 @@ async function runWorkDispatch(
wishPath: string,
ref: string,
): Promise<void> {
// 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.
Comment on lines +659 to +670

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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.

if (!existsSync(wishPath)) {
console.error(`❌ Wish not found: ${wishPath}`);
console.error(` Create it first: genie wish <agent> ${slug}`);
process.exit(1);
throw new Error(`Wish not found: ${wishPath}. Create it first: genie wish <agent> ${slug}`);
}

const content = await readFile(wishPath, 'utf-8');

// Extract the specific group section
const groupSection = extractGroup(content, group);
if (!groupSection) {
console.error(`❌ Group "${group}" not found in ${wishPath}`);
console.error(' Available groups:');
const groups = content.match(/^### Group [A-Za-z0-9]+:.*$/gm);
if (groups) {
for (const g of groups) console.error(` ${g}`);
}
process.exit(1);
const availableGroups = content.match(/^### Group [A-Za-z0-9]+:.*$/gm);
const availableList = availableGroups ? availableGroups.join(', ') : '(none)';
throw new Error(`Group "${group}" not found in ${wishPath}. Available: ${availableList}`);
}

// Auto-initialize state if missing (prevents polling loop when no state exists)
const groups = parseWishGroups(content);
await wishState.getOrCreateState(slug, groups);

// Start group in state machine (enforces dependencies)
// Start group in state machine (enforces dependencies). Throw on failure
// (NOT exit) — see top-of-function comment.
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);
}
Comment on lines 691 to 697

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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 + ")");


// Build context with wish-level info + group section
Expand Down
Loading