fix: bound startup recovery and release durable capacity#164
Conversation
📝 WalkthroughWalkthroughThe change bounds durable reconciled-agent recovery, prioritizes and throttles startup handling, improves exact-branch completion checks, deduplicates recovery by logical identity, and persists ChangesDurable recovery lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant StartupReconciler
participant FactoryLoop
participant FleetClient
participant DurableState
StartupReconciler->>FactoryLoop: queue prioritized reconciled exit
FactoryLoop->>FleetClient: resume or respawn logical agent
FleetClient-->>FactoryLoop: recovery result
FactoryLoop->>DurableState: persist abandoned lifecycle before promotion
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 updates the orchestrator's factory loop to bound agent recovery by the durable logical agent instead of the session reference, preventing agents that rotate session references or use no-session respawns from indefinitely consuming batch slots. It also ensures terminal dispatches are marked as 'abandoned' in the lifecycle state before promoting queued issues to avoid capacity stalls. The feedback recommends wrapping the await of an existing in-flight recovery promise in a try-catch block to prevent concurrent event handlers from re-throwing rejected promises and causing unhandled rejections.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/orchestrator/factory.ts (2)
5869-5906: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbandonment retry is silently dropped when the
abandoned-phase save fails, due to a shared retry-timer key.
#saveDispatchLifecycle(Line 3075) unconditionally callsthis.#scheduleDispatchLifecycleRetry(record)on every failure path (epoch missing or state save rejected) before returningfalse. That scheduler and#scheduleAbandonedDispatchRetry(Line 5891) both key off the same map (this.#dispatchLifecycleRetryTimers) using the sameissueKey(record.issue). So when the new fencing block's save fails:if (!await this.#saveDispatchLifecycle(record, 'abandoned', undefined, reason)) { this.#increment('abandonedDispatchReleaseRetries') this.#scheduleAbandonedDispatchRetry(record, reason) // no-op: timer already set by saveDispatchLifecycle above ... }
#scheduleAbandonedDispatchRetry's own guard (if (... this.#dispatchLifecycleRetryTimers.has(key)) return) sees the timer already claimed and returns immediately. The retry that actually fires is the generic one from#saveDispatchLifecycle, which calls#driveDispatchLifecycle(key)— and for a non-terminal phase likerunning, that path just reconciles tracked agents and returns, never re-invoking#abandonStuckDispatch. The durable row can be left stuck non-terminal indefinitely — exactly the durable-capacity leak this PR sets out to fix, in a narrower race window (lease/epoch loss during the abandon-fencing save).🔧 Suggested fix: clear the stale timer before scheduling the abandon-specific retry
if (!await this.#saveDispatchLifecycle(record, 'abandoned', undefined, reason)) { this.#increment('abandonedDispatchReleaseRetries') + const staleKey = issueKey(record.issue) + const staleTimer = this.#dispatchLifecycleRetryTimers.get(staleKey) + if (staleTimer) { + clearTimeout(staleTimer) + this.#dispatchLifecycleRetryTimers.delete(staleKey) + } this.#scheduleAbandonedDispatchRetry(record, reason) await this.#writeInFlightRegistry() return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrator/factory.ts` around lines 5869 - 5906, Clear the existing entry in this.#dispatchLifecycleRetryTimers for the issue key when the abandoned-phase save in `#abandonStuckDispatch` fails, then schedule the abandon-specific retry with `#scheduleAbandonedDispatchRetry`(record, reason). Preserve the retry counter, registry write, and early return, ensuring the retry invokes `#abandonStuckDispatch` rather than being consumed by the generic `#saveDispatchLifecycle` retry.
5245-5299: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a stable recovery key for resumed agents
namecan change during#resumeTrackedAgent, soissueKey:nameis not stable here. If a resume comes back under a new name, the next exit gets a fresh key and can re-enter the recovery loop. Key this off a stable identity (for examplesessionRef) or carry the original key through the rename.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrator/factory.ts` around lines 5245 - 5299, The recovery flow around `#resumeTrackedAgent` must use an identity that remains stable when the agent is renamed. Replace the recoveryKey construction or tracking logic with the agent’s stable sessionRef, or preserve the original key through resume, and ensure subsequent exit handling uses that same key rather than a newly derived issueKey:name value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/orchestrator/factory.ts`:
- Around line 5223-5236: Scope the recovery marker in the resumption check
around recoveryKey to the current run, preventing resumedExitKeys from leaking
across reopenings and causing later implementer redispatches to terminate
immediately. Incorporate the existing runId into the recovery key, or ensure the
reopen reset clears resumed markers, while preserving the one-recovery-per-run
behavior.
---
Outside diff comments:
In `@src/orchestrator/factory.ts`:
- Around line 5869-5906: Clear the existing entry in
this.#dispatchLifecycleRetryTimers for the issue key when the abandoned-phase
save in `#abandonStuckDispatch` fails, then schedule the abandon-specific retry
with `#scheduleAbandonedDispatchRetry`(record, reason). Preserve the retry
counter, registry write, and early return, ensuring the retry invokes
`#abandonStuckDispatch` rather than being consumed by the generic
`#saveDispatchLifecycle` retry.
- Around line 5245-5299: The recovery flow around `#resumeTrackedAgent` must use
an identity that remains stable when the agent is renamed. Replace the
recoveryKey construction or tracking logic with the agent’s stable sessionRef,
or preserve the original key through resume, and ensure subsequent exit handling
uses that same key rather than a newly derived issueKey:name value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5ac09323-4895-443e-bdf4-be9f6d4ec099
📒 Files selected for processing (2)
src/orchestrator/factory.test.tssrc/orchestrator/factory.ts
|
Addressed the two outside-diff CodeRabbit findings in 2d4433d:
Validation on 2d4433d: build; 55 files / 1,096 tests; 241-feature map; exact packed package E2E 9/9. |
|
Final exact-head live proof on 1edd4f8:
Local validation: build; 55 files / 1,097 tests; 241-feature map; exact packed E2E 9/9. All review threads are resolved. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/orchestrator/factory.ts (2)
5891-5938: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRetry-on-abandon-save-failure can re-release already-released agents.
#abandonStuckDispatchcleans up (releasing/terminating agents) and only afterward persists phase'abandoned'. If that persist fails (fence rejection, exactly the scenario the new#abandonedDispatchReasonsmap exists to retry), the durable lifecycle remains in its pre-abandon'running'phase with no agent markedreleasedAtMs. The generic retry (#driveDispatchLifecycle→ the newabandonedReasoncheck at L3328) reconstructsrecordfrom that stale lifecycle viainFlightRecordFromLifecycle, which includes every agent regardless of release state, and re-invokes#abandonStuckDispatch— re-running#releaseAndTerminateAgentson agents that were already successfully released in the first attempt.This is exactly the hazard
#finishDurableReleaseguards against elsewhere in this same file, by filteringagent.releasedAtMs !== undefinedbefore releasing.#abandonStuckDispatch's new retry path has no equivalent guard. The added regression test only passes because the fakeFleetClienttolerates a secondrelease()call for the same name; a real broker rejecting an unknown/already-released agent name (therelay#1116class of issue this file repeatedly guards against elsewhere) could makecleanupCompletefalse forever, permanently blocking the terminal'abandoned'persist and the queued-work promotion this PR is meant to guarantee.🛡️ Suggested fix — skip already-released agents on retry, mirroring `#finishDurableRelease`
async `#abandonStuckDispatch`(record: InFlightIssue, reason: string): Promise<void> { const key = issueKey(record.issue) this.#abandonedDispatchReasons.set(key, reason) - const agents = [...record.agents] + const lifecycle = await this.#state.getDispatchLifecycle(this.#workspaceId, key) + const released = new Set(lifecycle?.agents + .filter((agent) => agent.releasedAtMs !== undefined) + .map((agent) => agent.name)) + const agents = [...record.agents].filter(([name]) => !released.has(name)) for (const [agentName, tracked] of agents) { if (tracked.spec.role === 'implementer') continue this.#fleet.markAgentTerminal?.(agentName, `implementer-terminal:${reason}`) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrator/factory.ts` around lines 5891 - 5938, Update `#abandonStuckDispatch` to exclude agents whose releasedAtMs is already defined before calling `#releaseAndTerminateAgents`, matching the filtering used by `#finishDurableRelease`. Preserve cleanup for agents not yet released and ensure retries after a failed abandoned lifecycle save can proceed to persist the terminal phase.
1-1: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAbandon-retry path can re-release already-released agents; test relies on the fake tolerating it.
#abandonStuckDispatchreleases agents and only afterward persists phase'abandoned'. When that persist fails (fence rejection), the retry re-runs the whole function against a durable record that still lists every agent as not-yet-released (inFlightRecordFromLifecycledoesn't filter byreleasedAtMs), re-invoking#releaseAndTerminateAgentson agents already released in the prior attempt — unlike#finishDurableRelease, which explicitly guards this with areleasedset. The added tests pass only because the fakeFleetClienttolerates a repeatrelease()call for the same name; a real broker that rejects releasing an unknown/already-released name could stall this retry indefinitely and never reach'abandoned'.
src/orchestrator/factory.ts#L5891-5938: filterrecord.agentsagainstagent.releasedAtMs !== undefined(read from the durable lifecycle) before re-releasing on retry, mirroring#finishDurableRelease's pattern.src/orchestrator/factory.test.ts#L9895-9972: once the guard is added, confirm (or extend) these tests still pass, and consider assertingrelease()call counts to lock in the no-double-release behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/orchestrator/factory.ts` at line 1, Update `#abandonStuckDispatch` to exclude agents whose durable lifecycle has releasedAtMs set before invoking `#releaseAndTerminateAgents`, mirroring `#finishDurableRelease`’s released-agent guard. Preserve retries for unreleased agents and ensure the existing abandonment persistence flow still completes without re-releasing already-released agents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/orchestrator/factory.ts`:
- Around line 5891-5938: Update `#abandonStuckDispatch` to exclude agents whose
releasedAtMs is already defined before calling `#releaseAndTerminateAgents`,
matching the filtering used by `#finishDurableRelease`. Preserve cleanup for
agents not yet released and ensure retries after a failed abandoned lifecycle
save can proceed to persist the terminal phase.
- Line 1: Update `#abandonStuckDispatch` to exclude agents whose durable lifecycle
has releasedAtMs set before invoking `#releaseAndTerminateAgents`, mirroring
`#finishDurableRelease`’s released-agent guard. Preserve retries for unreleased
agents and ensure the existing abandonment persistence flow still completes
without re-releasing already-released agents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95c7a1f0-efb8-46c0-ae91-0e0f535f5917
📒 Files selected for processing (2)
src/orchestrator/factory.test.tssrc/orchestrator/factory.ts
1 participant
Summary
Live failure reproduced
Factory discovered ready issues and persisted queued rows, but five no-PR recovery lifecycles retained all durable capacity. Missing implementers exited immediately after recovery while reviewer sessions rotated IDs, producing an unbounded recovery loop. Cleanup removed the process-local batch entry but left the durable row in publishing/running, so promotion remained fenced. The mounted GitHub backfill also delayed recovery by scanning hundreds of historical issue records before an exact branch check. An unbounded 30-agent startup reconciliation then saturated the broker API.
Validation
Live canary
Exact PR head 1edd4f8 ran with Relayfile SDK 0.10.33 and the hardened Relay broker. Durable queued rows dropped from 16 to 12; Factory #145, #143, #147 and Cloud #2777 were promoted. The broker reports live implementer PIDs for #145, #143 and #147 plus their reviewers. Mount health is authoritatively recovered with zero degraded mounts.
Release gate
Hosted exact-head CI and package attestation are green, all review threads are resolved, and the live canary has promoted and spawned previously queued work.