fix(orchestrator): prevent runaway session spawning

[dimakis]

Summary

• Spawn-orphan race condition caused 50 sessions ($16) to spawn in 5 minutes targeting an already-merged PR
• Root cause: orphan detection reclaimed tasks before spawned sessions registered in the registry, creating a respawn loop
• Added 3-layer safety net: spawn grace period (60s), depth limit reduced 50→5, global rate limiter (5/min)

Root Cause

1. Task marked active, spawnSession() fires async (fire-and-forget)
2. Next tick() runs orphan detection — checks getActiveSessionIds() against registry
3. Spawned session hasn't registered yet → task's session_id not in active set → classified as orphan
4. Task reset to pending → getNextExecutable finds it → spawns another session
5. MAX_SPAWN_DEPTH=50 was the only guard — and it reset between tick chains

Safety Layers

Layer	Guard	Value
Grace period	recentSpawns map — orphan detection skips tasks spawned within window	60s
Depth limit	MAX_SPAWN_DEPTH per tick chain	5 (was 50)
Rate limiter	spawnTimestamps sliding window — blocks spawns across all tick chains	5 per 60s

Test plan

• 49/49 tests pass including 3 new tests
• Depth limit test: 10 tasks created, only 5 spawn
• Grace period test: orphan detection skips recently-spawned task
• Rate limit test: second tick chain blocked by sliding window

🤖 Generated with Claude Code

[dimakis]

Centaur Review

Found 6 issue(s) (2 critical) (3 warning).

packages/client/src/slices/tasks.ts

The core orchestrator fix (ID namespace, grace period, rate limiting, kill switch) is solid and well-tested, but spawnEnabled was not propagated through the @mitzo/client package — the field is missing from the client's LoopStatus type, protocol parser, store loader, and initial state, so the UI toggle will never reflect the server's actual spawn-enabled state.

• 🔴 bugs (L42): LoopStatus in @mitzo/client is missing the spawnEnabled field. The server sends it, but three places in the client package silently drop it: (1) protocol-parser.ts:474-484 constructs the loop_status object without spawnEnabled, (2) store.ts:531-545 (loadLoopStatus) maps the REST response without spawnEnabled, (3) INITIAL_TASKS_STATE has no default. The frontend's TaskBoardSection reads loopStatus.spawnEnabled from the store, but it will always be undefined — the toggle button will permanently show the disabled state (red ⛔) regardless of the server's actual value. Setting it via the API call works (fire-and-forget), but the UI will never reflect the true state back. [fixable]

packages/client/src/protocol-parser.ts

The core orchestrator fix (ID namespace, grace period, rate limiting, kill switch) is solid and well-tested, but spawnEnabled was not propagated through the @mitzo/client package — the field is missing from the client's LoopStatus type, protocol parser, store loader, and initial state, so the UI toggle will never reflect the server's actual spawn-enabled state.

• 🔴 bugs (L483): The loop_status WS message parser does not include spawnEnabled in the constructed status object. When the server broadcasts a status update (e.g. after setSpawnEnabled(true)), the client will overwrite its store with a LoopStatus that has spawnEnabled: undefined, losing the field even if it was somehow set before. [fixable]

packages/client/src/api-client.ts

The core orchestrator fix (ID namespace, grace period, rate limiting, kill switch) is solid and well-tested, but spawnEnabled was not propagated through the @mitzo/client package — the field is missing from the client's LoopStatus type, protocol parser, store loader, and initial state, so the UI toggle will never reflect the server's actual spawn-enabled state.

• 🟡 bugs (L278): getLoopStatus() return type does not include spawnEnabled. While the JSON response from the server does contain it, TypeScript callers in store.ts:loadLoopStatus won't see the field and it's not mapped into the store state. [fixable]

server/app.ts

The core orchestrator fix (ID namespace, grace period, rate limiting, kill switch) is solid and well-tested, but spawnEnabled was not propagated through the @mitzo/client package — the field is missing from the client's LoopStatus type, protocol parser, store loader, and initial state, so the UI toggle will never reflect the server's actual spawn-enabled state.

• 🟡 missing_tests (L801): The new POST /api/loop/spawn endpoint has no HTTP-level integration test. The orchestrator unit tests cover setSpawnEnabled() directly, but the endpoint's input validation (missing body, non-boolean enabled, orchestrator-not-initialized 503) is untested. [fixable]

server/task-orchestrator.ts

The core orchestrator fix (ID namespace, grace period, rate limiting, kill switch) is solid and well-tested, but spawnEnabled was not propagated through the @mitzo/client package — the field is missing from the client's LoopStatus type, protocol parser, store loader, and initial state, so the UI toggle will never reflect the server's actual spawn-enabled state.

• 🟡 unsafe_assumptions (L418): The rate limit retry timer calculation const retryIn = SPAWN_RATE_WINDOW_MS - oldestAge + 100 assumes spawnTimestamps[0] exists and is the oldest. This is safe given the filter just ran and the length check passed, but if Date.now() returns a value less than spawnTimestamps[0] (e.g. system clock adjustment), oldestAge becomes negative and retryIn exceeds the window. Consider clamping: Math.max(100, retryIn). [fixable]

frontend/src/components/TaskBoardSection.tsx

The core orchestrator fix (ID namespace, grace period, rate limiting, kill switch) is solid and well-tested, but spawnEnabled was not propagated through the @mitzo/client package — the field is missing from the client's LoopStatus type, protocol parser, store loader, and initial state, so the UI toggle will never reflect the server's actual spawn-enabled state.

• 🔵 style (L68): Inline style={{ color: ... }} for the spawn toggle button. The rest of the section uses CSS classes (cc-section-action-btn). Consider a CSS class or CSS variable for consistency, especially since the color values (#b48cff, #f87171) are hardcoded and may not match the app's theme system. [fixable]

---

Reviewed at bbaefd1

[dimakis]

Centaur Review

Found 5 issue(s) (2 warning).

server/task-orchestrator.ts

Solid multi-layered defense against runaway spawning (kill switch + rate limit + grace period + depth limit). The main concern is start() not clearing spawn tracking state, which could leak rate-limit data across goals when starting from paused state. The safe-by-default kill switch is a significant behavior change that should be documented.

• 🟡 bugs (L108): start() does not clear recentSpawns, spawnTimestamps, or rateLimitRetryTimer when starting a new goal. While stop() clears them, start() can be entered from the paused state (it only guards against running). If a new goal is started while paused, stale spawn tracking from the previous goal carries over — the new goal could be immediately rate-limited or have its tasks protected by a grace period meant for the old goal's tasks. Either clear this state in start() or guard against paused the same way stop() is called first. [fixable]
• 🔵 unsafe_assumptions (L418): this.spawnTimestamps[0] is safe here (the length check guarantees >= 5 entries after filtering), but if SPAWN_RATE_LIMIT were ever set to 0, this would access an empty array. Consider a guard or documenting the invariant that SPAWN_RATE_LIMIT >= 1. Low priority since these are private static constants. [fixable]
• 🔵 style (L419): The retry delay calculation Math.max(100, SPAWN_RATE_WINDOW_MS - oldestAge + 100) is correct but would benefit from a local variable name like msUntilOldestExpires to make the intent clearer. Minor readability nit. [fixable]
• 🟡 regressions (L402): The default session policy changed from spawn (unless explicitly 'reuse') to reuse (unless _spawnEnabled is true). Since _spawnEnabled defaults to false and is not persisted, every server restart silently disables spawn-based orchestration. This is likely intentional (safe-by-default kill switch), but it's a behavior change for users who were relying on spawn being the default. Existing tests had to add orch.setSpawnEnabled(true) across 15+ test cases, confirming the scope of this change. Ensure this is called out in release notes.

server/__tests__/task-orchestrator.test.ts

Solid multi-layered defense against runaway spawning (kill switch + rate limit + grace period + depth limit). The main concern is start() not clearing spawn tracking state, which could leak rate-limit data across goals when starting from paused state. The safe-by-default kill switch is a significant behavior change that should be documented.

• 🔵 missing_tests: No test verifies that stop() clears the new spawn tracking state (recentSpawns, spawnTimestamps, rateLimitRetryTimer). The existing stop test checks clearTaskContext but not the spawn safety mechanisms. A test that spawns tasks, calls stop(), then starts a new goal would ensure rate-limit state doesn't leak across goals. [fixable]

---

Reviewed at d82ea2a

[dimakis]

Centaur Review

Found 6 issue(s) (2 warning).

frontend/src/styles/desktop.css

Solid incident fix — the ID namespace mismatch root cause, grace period, rate limiter, and kill switch are well-designed and thoroughly tested. Minor findings around mobile CSS scoping and defensive clarity in the rate limiter.

• 🔵 style (L711): Spawn button color styles (.cc-spawn-enabled/.cc-spawn-disabled) are only defined inside the @media (min-width: 768px) desktop media query. On mobile viewports, the spawn toggle button will render unstyled (inheriting the base .cc-section-action-btn color), so the enabled/disabled visual distinction is lost. Since Mitzo is described as mobile-first, consider adding these color rules to global.css or to a non-media-scoped block. [fixable]

server/task-orchestrator.ts

Solid incident fix — the ID namespace mismatch root cause, grace period, rate limiter, and kill switch are well-designed and thoroughly tested. Minor findings around mobile CSS scoping and defensive clarity in the rate limiter.

• 🟡 unsafe_assumptions (L431): msUntilOldestExpires assumes this.spawnTimestamps[0] exists when the rate limit is hit. This is safe today because the if guard ensures spawnTimestamps.length >= SPAWN_RATE_LIMIT (which is 5, > 0). However, the filter() on line 417 prunes stale entries before this check, so in theory the array is always non-empty at this point. Still, an explicit guard or comment would make this clearer, since the correctness depends on the invariant that filter() leaves at least SPAWN_RATE_LIMIT entries when the if fires. [fixable]
• 🔵 style (L431): Line exceeds ~100 characters (const msUntilOldestExpires = Math.max(100, TaskOrchestrator.SPAWN_RATE_WINDOW_MS - oldestAge + 100);). Consider extracting to a shorter expression for readability. [fixable]
• 🟡 regressions (L47): MAX_SPAWN_DEPTH was reduced from 50 to 5. This means a single tick chain can only spawn 5 tasks before deferring. Combined with SPAWN_RATE_LIMIT = 5 per 60-second window, this means that if a goal has many spawn-policy subtasks (e.g. 20), only 5 will be spawned in the first minute. This is a deliberate safety improvement, but the two constants being equal (both 5) means MAX_SPAWN_DEPTH is effectively redundant — the rate limiter will always prevent a 6th spawn in the same tick chain. Consider documenting that the depth limit is the per-tick-chain ceiling while the rate limit is the cross-chain ceiling, or unifying them.

server/__tests__/task-orchestrator.test.ts

Solid incident fix — the ID namespace mismatch root cause, grace period, rate limiter, and kill switch are well-designed and thoroughly tested. Minor findings around mobile CSS scoping and defensive clarity in the rate limiter.

• 🔵 missing_tests: No test verifies that setSpawnEnabled can be toggled to false while spawned tasks are in-flight, confirming that already-spawned tasks continue (aren't killed) but no new spawns occur. The 'can be toggled while loop is running' test only covers disabled→enabled, not the reverse direction mid-spawn. [fixable]

packages/client/src/store.ts

Solid incident fix — the ID namespace mismatch root cause, grace period, rate limiter, and kill switch are well-designed and thoroughly tested. Minor findings around mobile CSS scoping and defensive clarity in the rate limiter.

• 🔵 bugs (L583): setSpawnEnabled() calls the API but does not optimistically update local loopStatus.spawnEnabled. The UI relies on the server's WebSocket broadcast to reflect the change. This is fine for correctness (other loop actions follow the same pattern), but it means the toggle button state won't visually flip until the WS broadcast arrives — a brief flash of the old state. Minor UX nit. [fixable]

---

Reviewed at a59e82f

[dimakis]

🔵 style: Spawn button color styles (.cc-spawn-enabled/.cc-spawn-disabled) are only defined inside the @media (min-width: 768px) desktop media query. On mobile viewports, the spawn toggle button will render unstyled (inheriting the base .cc-section-action-btn color), so the enabled/disabled visual distinction is lost. Since Mitzo is described as mobile-first, consider adding these color rules to global.css or to a non-media-scoped block. [fixable]

[dimakis]

🟡 unsafe_assumptions: msUntilOldestExpires assumes this.spawnTimestamps[0] exists when the rate limit is hit. This is safe today because the if guard ensures spawnTimestamps.length >= SPAWN_RATE_LIMIT (which is 5, > 0). However, the filter() on line 417 prunes stale entries before this check, so in theory the array is always non-empty at this point. Still, an explicit guard or comment would make this clearer, since the correctness depends on the invariant that filter() leaves at least SPAWN_RATE_LIMIT entries when the if fires. [fixable]

[dimakis]

🔵 style: Line exceeds ~100 characters (const msUntilOldestExpires = Math.max(100, TaskOrchestrator.SPAWN_RATE_WINDOW_MS - oldestAge + 100);). Consider extracting to a shorter expression for readability. [fixable]

[dimakis]

🟡 regressions: MAX_SPAWN_DEPTH was reduced from 50 to 5. This means a single tick chain can only spawn 5 tasks before deferring. Combined with SPAWN_RATE_LIMIT = 5 per 60-second window, this means that if a goal has many spawn-policy subtasks (e.g. 20), only 5 will be spawned in the first minute. This is a deliberate safety improvement, but the two constants being equal (both 5) means MAX_SPAWN_DEPTH is effectively redundant — the rate limiter will always prevent a 6th spawn in the same tick chain. Consider documenting that the depth limit is the per-tick-chain ceiling while the rate limit is the cross-chain ceiling, or unifying them.

[dimakis]

🔵 bugs: setSpawnEnabled() calls the API but does not optimistically update local loopStatus.spawnEnabled. The UI relies on the server's WebSocket broadcast to reflect the change. This is fine for correctness (other loop actions follow the same pattern), but it means the toggle button state won't visually flip until the WS broadcast arrives — a brief flash of the old state. Minor UX nit. [fixable]