Duplicate subagent spawn when durability mode is "minions"

spawnSubagentDirect is called on line 313, and then a "waiting" job is immediately added to the queue (line 358). The MinionWorker will claim that "waiting" job and call spawnSubagentDirect again inside subagentSpawnHandler, spawning a second subagent for every call to sessions_spawn when durability is enabled.

The isRetry flag inside the handler (based on previousSessionKey being set) only changes a progress-phase label — it does not skip the spawn. Both the "fresh spawn" and "crash recovery" branches of the handler unconditionally call spawnSubagentDirect.

The PR description's "spawn-then-track" intent requires that the tracking row be inserted as status = 'active' (with a valid lock_token and lock_until) so the worker's normal claim loop cannot pick it up — only stall detection can re-queue it after a crash. Using minionQueue.add() (which always inserts status = 'waiting') makes every newly-spawned subagent immediately claimable and re-processed by the worker.

This is a comment left during a code review.
Path: src/agents/tools/sessions-spawn-tool.ts
Line: 348-379

Comment:
**Duplicate subagent spawn when durability mode is `"minions"`**

`spawnSubagentDirect` is called on line 313, and then a `"waiting"` job is immediately added to the queue (line 358). The MinionWorker will claim that `"waiting"` job and call `spawnSubagentDirect` **again** inside `subagentSpawnHandler`, spawning a second subagent for every call to `sessions_spawn` when durability is enabled.

The `isRetry` flag inside the handler (based on `previousSessionKey` being set) only changes a progress-phase label — it does not skip the spawn. Both the "fresh spawn" and "crash recovery" branches of the handler unconditionally call `spawnSubagentDirect`.

The PR description's "spawn-then-track" intent requires that the tracking row be inserted as `status = 'active'` (with a valid `lock_token` and `lock_until`) so the worker's normal claim loop cannot pick it up — only stall detection can re-queue it after a crash. Using `minionQueue.add()` (which always inserts `status = 'waiting'`) makes every newly-spawned subagent immediately claimable and re-processed by the worker.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 64fbe36. The tracking row is now inserted as status='active' with a valid lock_token and lock_until via direct SQL, so the worker's claim loop (which only claims 'waiting') skips it. Only stall detection can re-queue the row after a crash.

- minionQueue.add("subagent.spawn", {...})  // 'waiting' = worker claims = double-spawn
+ store.db.prepare('INSERT ... status=active, lock_token=?, lock_until=?')  // not claimable

String-interpolated SQL in removeJob

MINION_TERMINAL_STATUSES.join("','") is spliced directly into the SQL string. The values are hardcoded compile-time constants so there is no real injection risk today, but the pattern diverges from every other query in this file (which use ? placeholders). If the constant is ever extended or the join is copy-pasted elsewhere, the invariant breaks silently.

Prefer building placeholder lists the same way claim, cancelJob, and prune do:

This is a comment left during a code review.
Path: src/minions/queue.ts
Line: 418-426

Comment:
**String-interpolated SQL in `removeJob`**

`MINION_TERMINAL_STATUSES.join("','")` is spliced directly into the SQL string. The values are hardcoded compile-time constants so there is no real injection risk today, but the pattern diverges from every other query in this file (which use `?` placeholders). If the constant is ever extended or the join is copy-pasted elsewhere, the invariant breaks silently.

Prefer building placeholder lists the same way `claim`, `cancelJob`, and `prune` do:

```suggestion
  removeJob(id: number): boolean {
    const placeholders = MINION_TERMINAL_STATUSES.map(() => "?").join(",");
    const rows = this.store.db
      .prepare(
        `DELETE FROM minion_jobs WHERE id = ? AND status IN (${placeholders}) RETURNING id`,
      )
      .all(id, ...MINION_TERMINAL_STATUSES);
    return rows.length > 0;
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 64fbe36. Switched to ? placeholders matching every other query in the file:

const placeholders = MINION_TERMINAL_STATUSES.map(() => "?").join(",");
const rows = this.store.db
  .prepare(`DELETE FROM minion_jobs WHERE id = ? AND status IN (${placeholders}) RETURNING id`)
  .all(id, ...MINION_TERMINAL_STATUSES);

SIGTERM/SIGINT listener accumulates on multiple start() calls

shutdown is registered on the global process each time start() is called, but only removed in the finally block. If start() throws before the while loop (e.g., the first promoteDelayed() throws), the finally block still runs and removes the listener — so that path is fine. The real concern is testing: if tests create multiple MinionWorker instances and call start(), Node.js emits MaxListenersExceededWarning. The maybeStartMinionWorker singleton guard prevents this in production, but unit tests that construct workers directly will hit it. Consider using a once flag or calling process.setMaxListeners in the worker constructor to surface the issue clearly.

This is a comment left during a code review.
Path: src/minions/worker.ts
Line: 67-71

Comment:
**SIGTERM/SIGINT listener accumulates on multiple `start()` calls**

`shutdown` is registered on the global `process` each time `start()` is called, but only removed in the `finally` block. If `start()` throws before the `while` loop (e.g., the first `promoteDelayed()` throws), the `finally` block still runs and removes the listener — so that path is fine. The real concern is testing: if tests create multiple `MinionWorker` instances and call `start()`, Node.js emits `MaxListenersExceededWarning`. The `maybeStartMinionWorker` singleton guard prevents this in production, but unit tests that construct workers directly will hit it. Consider using a `once` flag or calling `process.setMaxListeners` in the worker constructor to surface the issue clearly.

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 64fbe36. Two changes:

1. process.on → process.once so listeners don't accumulate across test instances.
2. Added re-entry guard at the top of start(): if (this.running) return; prevents double-start entirely.

resolveParentIfDone excludes 'failed' from terminal child statuses

The NOT EXISTS clause checks status NOT IN ('completed', 'dead', 'cancelled'). The 'failed' status is in MINION_TERMINAL_STATUSES and is produced by applyChildFailPolicy's direct SQL update when on_child_fail = 'fail_parent'. A parent whose child just failed via that policy will already be set to 'failed' itself, so the resolution is moot in that branch — but for on_child_fail = 'ignore' or 'continue', a 'failed' child leaves the parent stuck in 'waiting-children' forever because 'failed' is not treated as terminal here. Including 'failed' in the exclusion list (alongside 'dead' and 'cancelled') aligns this with the MINION_TERMINAL_STATUSES definition.

The same gap exists in the public resolveParent method (line 722).

This is a comment left during a code review.
Path: src/minions/queue.ts
Line: 806-816

Comment:
**`resolveParentIfDone` excludes `'failed'` from terminal child statuses**

The NOT EXISTS clause checks `status NOT IN ('completed', 'dead', 'cancelled')`. The `'failed'` status is in `MINION_TERMINAL_STATUSES` and is produced by `applyChildFailPolicy`'s direct SQL update when `on_child_fail = 'fail_parent'`. A parent whose child just failed via that policy will already be set to `'failed'` itself, so the resolution is moot in that branch — but for `on_child_fail = 'ignore'` or `'continue'`, a `'failed'` child leaves the parent stuck in `'waiting-children'` forever because `'failed'` is not treated as terminal here. Including `'failed'` in the exclusion list (alongside `'dead'` and `'cancelled'`) aligns this with the `MINION_TERMINAL_STATUSES` definition.

The same gap exists in the public `resolveParent` method (line 722).

How can I resolve this? If you propose a fix, please make it concise.

Fixed in 64fbe36. Added 'failed' to the NOT IN clause in all three locations: resolveParentIfDone, the inline resolution in completeJob, and the public resolveParent method. A parent with on_child_fail='ignore' or 'continue' no longer gets stuck in 'waiting-children' when a child fails.

- AND status NOT IN ('completed', 'dead', 'cancelled')
+ AND status NOT IN ('completed', 'failed', 'dead', 'cancelled')

Call minion worker startup during gateway initialization

This adds maybeStartMinionWorker() but nothing in the repository invokes it, so the worker loop never starts at runtime. In that state, enqueued minion jobs are never claimed and the new durability behaviors (stall/timeout recovery, retries, handler execution) do not run, which means crash-recovery is effectively disabled despite enabling minion writes.

Useful? React with 👍 / 👎.

Fixed in 604c942. Wired maybeStartMinionWorker() into src/gateway/server-startup-early.ts via dynamic import, right after startTaskRegistryMaintenance():

import("../minions/gateway-startup.js")
  .then((m) => m.maybeStartMinionWorker())
  .catch(() => {});

Dynamic import keeps the gateway startup path lazy (minions module only loads if the import succeeds). The worker now actually starts alongside the gateway.

Persist task-to-minion identity instead of process-local map

The upsert path depends on the in-memory taskToMinionId map to decide whether to UPDATE or INSERT. After any process restart this map is empty, so existing tasks are inserted again instead of updated, creating duplicate shadow rows and leaving older rows stale (and undeletable via syncTaskDeleteToMinions, which also relies on that map). This will skew minion status/stats over time.

Useful? React with 👍 / 👎.

Fixed in 604c942. Replaced the process-local Map<string, number> with DB-level dedup via idempotency_key = 'task:{taskId}'. On upsert, the sync module queries SELECT id FROM minion_jobs WHERE idempotency_key = ? before deciding INSERT vs UPDATE. Survives restarts, no duplicate rows.

Also switched syncTaskDeleteToMinions to delete by idempotency_key instead of the old map lookup.

Replace bare require in ESM minion sync loader

This module is executed in an ESM package (type: module), so require(...) throws ReferenceError; the error is caught, storeInitAttempted is set, and all subsequent syncTaskToMinions/syncTaskDeleteToMinions calls are permanently no-ops for that process. In practice, task-registry shadow writes to minion_jobs never happen in production after the first call path.

Useful? React with 👍 / 👎.

Fixed in e318c34. Replaced require() with await import() and made getStore(), syncTaskToMinions(), and syncTaskDeleteToMinions() async. Shadow writes now actually work in the ESM runtime.

Forward requester context when respawning subagent jobs

Crash-recovery respawns call spawnSubagentDirect with an empty context object, so requester identity metadata (session/channel/account/thread) is dropped on retry. Any recovered job can be relaunched under default/main-session context instead of the original caller, which changes ownership checks and routing behavior for resumed subagents.

Useful? React with 👍 / 👎.

Fixed in e318c34. The handler now destructures requesterSessionKey, requesterChannel, requesterAccountId, requesterTo, requesterThreadId from job.data and forwards them to spawnSubagentDirect's second argument. Crash-recovered subagents retain their original requester identity.

Track only accepted spawn results in minion_jobs

The tracking insert runs for every non-error result, including status: "forbidden". Forbidden responses typically have no runnable run/session identifiers, yet this still writes an active minion row, which later appears as stalled/dead noise and can trigger incorrect recovery behavior if stall policy is relaxed. Restricting tracking to accepted spawns avoids poisoning queue state.

Useful? React with 👍 / 👎.

Fixed in e318c34. Changed result.status !== "error" to result.status === "accepted". Forbidden/rejected spawns no longer write tracking rows.

Import legacy tasks into claimable handler-compatible states

Migration writes live legacy tasks as attached with legacy.<runtime> names, but worker claims only waiting jobs whose names match registered handlers (subagent.spawn, acp.spawn, etc.). In this commit there is no runtime transition path that makes these imported rows claimable, so archived in-flight legacy tasks are stranded instead of recoverable.

Useful? React with 👍 / 👎.

Fixed in e318c34. Two changes:

1. Migration now maps runtime → registered handler name (subagent→subagent.spawn, acp→acp.spawn, cli→cli.spawn, cron→cron.tick) so imported rows match what the worker claims.
2. Status changed from attached to waiting so imported rows are immediately claimable by the worker for recovery.

Requeue first stalled attempt before dead-lettering jobs

handleStalled uses counter + 1 < maxStalled, which means jobs with the default max_stalled = 1 are marked dead on their very first expired lock instead of being re-queued once. This breaks the advertised crash-recovery path for default-tracked subagent rows (for example those inserted from sessions-spawn-tool), because a single gateway crash never gets a rescue attempt.

Useful? React with 👍 / 👎.

Fixed in 54dbce3. Changed max_stalled default from 1 to 2 in schema DDL. With the old default, 0+1 < 1 was false, so the first stall went straight to dead-letter with no re-queue attempt. Now: first stall re-queues (0+1 < 2 = true), second stall dead-letters (1+1 < 2 = false). Crash recovery works on first attempt.

Propagate parent policy when timeout dead-letters a child

handleTimeouts directly updates matching rows to dead and returns them, but unlike failJob it never applies on_child_fail handling or parent-resolution logic. When a child job with parent_job_id times out, the parent can remain stuck in waiting-children indefinitely (or miss fail_parent behavior), which blocks fan-out workflows from completing correctly.

Useful? React with 👍 / 👎.

Fixed in 54dbce3. handleTimeouts now runs inside a transaction and calls applyChildFailPolicy() for each timed-out job that has a parentJobId. Parents with on_child_fail='fail_parent' get marked failed; parents with 'ignore'/'continue' get resolution checks. Fan-out workflows no longer get stuck.

Keep task lost-reconciliation enabled for non-minion runs

This guard disables shouldMarkLost entirely once minionsOwnLostDetection is enabled at gateway startup, so the task-registry sweep no longer marks any active tasks as lost. However, shadow-synced registry rows are written as plain active records without lock heartbeats (task-minion-sync), so minion stall/timeout sweeps cannot replace this for ACP/CLI/cron tasks; those tasks can stay running forever after their backing session disappears.

Useful? React with 👍 / 👎.

Fixed in 54dbce3. Removed the minionsOwnLostDetection flag entirely. The existing task-registry sweep now runs as a safety net alongside minions (belt + suspenders). Minion stall detection is faster (sub-second heartbeat vs 5-min grace), but the old sweep catches ACP/CLI/cron tasks that don't have minion lock heartbeats yet.

Run parent-failure handling for stalled jobs marked dead

When handleStalled() dead-letters an expired active job, it only updates the child row and returns it; it never invokes applyChildFailPolicy/parent resolution for rows with parent_job_id. In a fan-out tree, a stalled child that hits max stalled count will become dead while the parent remains waiting-children, so fail_parent/remove_dep behavior is skipped and the parent can remain blocked indefinitely after crash recovery.

Useful? React with 👍 / 👎.

Resolve parent state after cancelling child subtrees

cancelJob() marks descendants as cancelled but does not trigger parent resolution afterward. If an operator cancels a non-root child that is the last unfinished dependency, the parent stays in waiting-children forever because no later child completion will call resolveParentIfDone, effectively wedging that workflow.

Useful? React with 👍 / 👎.

Stop tracking accepted spawns as expiring active jobs

In minions durability mode, this insert creates an active job with a 30s lock, but there is no corresponding heartbeat or completion path for the original live subagent run. Once lock_until expires, handleStalled() will treat it as orphaned and requeue it, so normal long-running (or even already-finished) spawns get retried as crash recovery and a duplicate subagent.spawn is launched without any gateway crash. This tracking row needs either lock renewal/terminal updates from the live run lifecycle or a non-claimable status until true orphan reconciliation.

Useful? React with 👍 / 👎.

ignore/continue child-fail policies never resolve the parent

applyChildFailPolicy calls resolveParentIfDone only for the remove_dep branch. For ignore and continue policies the function returns without doing anything, so the parent stays in 'waiting-children' permanently if all of its children end in 'failed' or 'dead'.

The previous thread's fix (adding 'failed' to the NOT IN clause of resolveParentIfDone) is necessary but not sufficient: it only matters when resolveParentIfDone is actually called. When the last child fails with ignore or continue policy, resolveParentIfDone is never reached, leaving the parent stuck forever.

// fix: add the missing branch
  } else if (policy === "ignore" || policy === "continue") {
    resolveParentIfDone(db, parentId, now);
  }

This is a comment left during a code review.
Path: src/minions/queue.ts
Line: 827-848

Comment:
**`ignore`/`continue` child-fail policies never resolve the parent**

`applyChildFailPolicy` calls `resolveParentIfDone` only for the `remove_dep` branch. For `ignore` and `continue` policies the function returns without doing anything, so the parent stays in `'waiting-children'` permanently if all of its children end in `'failed'` or `'dead'`.

The previous thread's fix (adding `'failed'` to the `NOT IN` clause of `resolveParentIfDone`) is necessary but not sufficient: it only matters when `resolveParentIfDone` is actually called. When the last child fails with `ignore` or `continue` policy, `resolveParentIfDone` is never reached, leaving the parent stuck forever.

```ts
// fix: add the missing branch
  } else if (policy === "ignore" || policy === "continue") {
    resolveParentIfDone(db, parentId, now);
  }
```

How can I resolve this? If you propose a fix, please make it concise.