fix(factory): make dispatch claims durable and observable - #248
fix(factory): make dispatch claims durable and observable#248khaliqgant wants to merge 1 commit into
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Warning Review limit reached
Next review available in: 100 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d804202c3b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| implementingStateId = this.#states.idFor(issue.team, 'agentImplementing') | ||
| await this.#linear.setState(issue, implementingStateId) | ||
| } | ||
| implementingStateId = await this.#applyDispatchClaim(record, issue, comment) |
There was a problem hiding this comment.
When the default internal backend is using GitAgentWorktreeManager and this claim exhausts its three retries, the thrown error enters the generic dispatch-failure path, where #teardownFailedDispatchWorktrees releases the already spawned agents and removes their worktrees before saving the lifecycle as retryable. The saved records still contain each tracked.result, so #resumeDurableDispatch later calls #spawnAgent, which returns early for those receipts without checking the roster or respawning; if writeback recovers, the lifecycle is marked running even though no workers remain. Claim failures need either to retain the agents/worktrees or clear/reconcile the spawn receipts before durable recovery.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/orchestrator/batch-tracker.ts">
<violation number="1" location="src/orchestrator/batch-tracker.ts:20">
P1: The `restore()` method rebuilds an `InFlightIssue` from scratch and drops `dispatchClaim`, even though the new interface now includes it. During crash recovery the in-flight record is rebuilt via `inFlightRecordFromLifecycle(...)` → `batch.restore(...)` (factory.ts lines 2634, 3049, 3064, 3949, 13040). `dispatchClaim` is kept by `inFlightRecordFromLifecycle` but discarded by `restore`, so a verified/pending claim never survives durable resume. As a result `#writeInFlightRegistry()` copies `record.dispatchClaim` from the restored record only when it is present, so a clean dispatch claim set before a crash no longer appears in `factory status` after recovery — exactly the registry-kept-available-when-writeback-degraded behavior this PR promises. Copy `dispatchClaim` in `restore()` like `inFlightRecordFromLifecycle` does.</violation>
</file>
<file name="src/orchestrator/factory.ts">
<violation number="1" location="src/orchestrator/factory.ts:2715">
P2: `#dispatchClaimStatuses` only ever gets `.set()` entries (lines 2715, 4161, 4541, 4576, 5933) and is never pruned or bounded, so it retains one object per dispatched issue for the life of the process. In a long-running live daemon that dispatches many issues this grows without bound even after the issue leaves the in-flight batch. Remove entries when the in-flight issue is released (alongside the existing in-flight cleanup) or bound the map like the append-only fallback-eligibility caches.</violation>
<violation number="2" location="src/orchestrator/factory.ts:2726">
P1: After the dispatch claim writeback exhausts its three retries, the thrown error now propagates into the generic dispatch-failure path, where teardownFailedDispatchWorktrees tears down the already-spawned agents and worktrees before persisting the lifecycle as retryable. Because the saved lifecycle record still has tracked.result populated from the spawn, durable recovery's spawnAgent call will short-circuit as already-spawned instead of respawning, so if writeback later succeeds the lifecycle can be marked running with no actual workers left. Either preserve the spawned agents/worktrees on a claim failure, or clear/reconcile the spawn receipts before durable recovery retries the claim.</violation>
<violation number="3" location="src/orchestrator/factory.ts:4524">
P2: The GitHub dispatch comment write and its `hasCommentMarker` read-back are not atomic, and `postComment` is non-idempotent: if the comment lands but the immediately-following read-back doesn't show it (GitHub reads can lag fresh writes), `#retryDispatchWriteback` re-invokes `postComment`, posting a duplicate comment on the issue. Consider deduplicating the marker check before posting on each retry (already done at loop entry) and, if a read-back miss is detected after an `apply()`, retry only the verification instead of re-posting, or treat the 'already present' marker as success before calling `postComment`.</violation>
<violation number="4" location="src/orchestrator/factory.ts:4542">
P1: A crash after this registry write loses the claim state from the durable lifecycle because lifecycle persistence is deferred to a later dispatch phase. Recovery resets the claim and can repost a successful Linear comment; persist the lifecycle during each claim transition before proceeding.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| agents: Map<string, TrackedAgent> | ||
| invocationIds: Set<string> | ||
| result?: DispatchResult | ||
| dispatchClaim?: FactoryDispatchClaimStatus |
There was a problem hiding this comment.
P1: The restore() method rebuilds an InFlightIssue from scratch and drops dispatchClaim, even though the new interface now includes it. During crash recovery the in-flight record is rebuilt via inFlightRecordFromLifecycle(...) → batch.restore(...) (factory.ts lines 2634, 3049, 3064, 3949, 13040). dispatchClaim is kept by inFlightRecordFromLifecycle but discarded by restore, so a verified/pending claim never survives durable resume. As a result #writeInFlightRegistry() copies record.dispatchClaim from the restored record only when it is present, so a clean dispatch claim set before a crash no longer appears in factory status after recovery — exactly the registry-kept-available-when-writeback-degraded behavior this PR promises. Copy dispatchClaim in restore() like inFlightRecordFromLifecycle does.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/batch-tracker.ts, line 20:
<comment>The `restore()` method rebuilds an `InFlightIssue` from scratch and drops `dispatchClaim`, even though the new interface now includes it. During crash recovery the in-flight record is rebuilt via `inFlightRecordFromLifecycle(...)` → `batch.restore(...)` (factory.ts lines 2634, 3049, 3064, 3949, 13040). `dispatchClaim` is kept by `inFlightRecordFromLifecycle` but discarded by `restore`, so a verified/pending claim never survives durable resume. As a result `#writeInFlightRegistry()` copies `record.dispatchClaim` from the restored record only when it is present, so a clean dispatch claim set before a crash no longer appears in `factory status` after recovery — exactly the registry-kept-available-when-writeback-degraded behavior this PR promises. Copy `dispatchClaim` in `restore()` like `inFlightRecordFromLifecycle` does.</comment>
<file context>
@@ -17,6 +17,7 @@ export interface InFlightIssue {
agents: Map<string, TrackedAgent>
invocationIds: Set<string>
result?: DispatchResult
+ dispatchClaim?: FactoryDispatchClaimStatus
}
</file context>
| updatedAtMs: this.#clock.now(), | ||
| } | ||
| this.#dispatchClaimStatuses.set(issueKey(record.issue), record.dispatchClaim) | ||
| await this.#writeDispatchClaimRegistry(record.issue) |
There was a problem hiding this comment.
P1: A crash after this registry write loses the claim state from the durable lifecycle because lifecycle persistence is deferred to a later dispatch phase. Recovery resets the claim and can repost a successful Linear comment; persist the lifecycle during each claim transition before proceeding.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 4542:
<comment>A crash after this registry write loses the claim state from the durable lifecycle because lifecycle persistence is deferred to a later dispatch phase. Recovery resets the claim and can repost a successful Linear comment; persist the lifecycle during each claim transition before proceeding.</comment>
<file context>
@@ -4476,6 +4503,116 @@ export class FactoryLoop implements Factory {
+ updatedAtMs: this.#clock.now(),
+ }
+ this.#dispatchClaimStatuses.set(issueKey(record.issue), record.dispatchClaim)
+ await this.#writeDispatchClaimRegistry(record.issue)
+ return implementingStateId
+ }
</file context>
| implementingStateId = this.#states.idFor(issue.team, 'agentImplementing') | ||
| await this.#linear.setState(issue, implementingStateId) | ||
| } | ||
| implementingStateId = await this.#applyDispatchClaim(record, issue, comment) |
There was a problem hiding this comment.
P1: After the dispatch claim writeback exhausts its three retries, the thrown error now propagates into the generic dispatch-failure path, where teardownFailedDispatchWorktrees tears down the already-spawned agents and worktrees before persisting the lifecycle as retryable. Because the saved lifecycle record still has tracked.result populated from the spawn, durable recovery's spawnAgent call will short-circuit as already-spawned instead of respawning, so if writeback later succeeds the lifecycle can be marked running with no actual workers left. Either preserve the spawned agents/worktrees on a claim failure, or clear/reconcile the spawn receipts before durable recovery retries the claim.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 2726:
<comment>After the dispatch claim writeback exhausts its three retries, the thrown error now propagates into the generic dispatch-failure path, where teardownFailedDispatchWorktrees tears down the already-spawned agents and worktrees before persisting the lifecycle as retryable. Because the saved lifecycle record still has tracked.result populated from the spawn, durable recovery's spawnAgent call will short-circuit as already-spawned instead of respawning, so if writeback later succeeds the lifecycle can be marked running with no actual workers left. Either preserve the spawned agents/worktrees on a claim failure, or clear/reconcile the spawn receipts before durable recovery retries the claim.</comment>
<file context>
@@ -2711,17 +2723,7 @@ export class FactoryLoop implements Factory {
- implementingStateId = this.#states.idFor(issue.team, 'agentImplementing')
- await this.#linear.setState(issue, implementingStateId)
- }
+ implementingStateId = await this.#applyDispatchClaim(record, issue, comment)
this.#emit('writeback-verified', { issue: dispatchDecision.issue, path: issue.path })
}
</file context>
| record, | ||
| issue, | ||
| 'GitHub dispatch comment', | ||
| async () => this.#githubWriteback.postComment(issue, comment), |
There was a problem hiding this comment.
P2: The GitHub dispatch comment write and its hasCommentMarker read-back are not atomic, and postComment is non-idempotent: if the comment lands but the immediately-following read-back doesn't show it (GitHub reads can lag fresh writes), #retryDispatchWriteback re-invokes postComment, posting a duplicate comment on the issue. Consider deduplicating the marker check before posting on each retry (already done at loop entry) and, if a read-back miss is detected after an apply(), retry only the verification instead of re-posting, or treat the 'already present' marker as success before calling postComment.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 4524:
<comment>The GitHub dispatch comment write and its `hasCommentMarker` read-back are not atomic, and `postComment` is non-idempotent: if the comment lands but the immediately-following read-back doesn't show it (GitHub reads can lag fresh writes), `#retryDispatchWriteback` re-invokes `postComment`, posting a duplicate comment on the issue. Consider deduplicating the marker check before posting on each retry (already done at loop entry) and, if a read-back miss is detected after an `apply()`, retry only the verification instead of re-posting, or treat the 'already present' marker as success before calling `postComment`.</comment>
<file context>
@@ -4476,6 +4503,116 @@ export class FactoryLoop implements Factory {
+ record,
+ issue,
+ 'GitHub dispatch comment',
+ async () => this.#githubWriteback.postComment(issue, comment),
+ commentApplied,
+ )
</file context>
| state: 'pending', | ||
| updatedAtMs: this.#clock.now(), | ||
| } | ||
| this.#dispatchClaimStatuses.set(issueKey(record.issue), record.dispatchClaim) |
There was a problem hiding this comment.
P2: #dispatchClaimStatuses only ever gets .set() entries (lines 2715, 4161, 4541, 4576, 5933) and is never pruned or bounded, so it retains one object per dispatched issue for the life of the process. In a long-running live daemon that dispatches many issues this grows without bound even after the issue leaves the in-flight batch. Remove entries when the in-flight issue is released (alongside the existing in-flight cleanup) or bound the map like the append-only fallback-eligibility caches.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 2715:
<comment>`#dispatchClaimStatuses` only ever gets `.set()` entries (lines 2715, 4161, 4541, 4576, 5933) and is never pruned or bounded, so it retains one object per dispatched issue for the life of the process. In a long-running live daemon that dispatches many issues this grows without bound even after the issue leaves the in-flight batch. Remove entries when the in-flight issue is released (alongside the existing in-flight cleanup) or bound the map like the append-only fallback-eligibility caches.</comment>
<file context>
@@ -2702,6 +2707,13 @@ export class FactoryLoop implements Factory {
+ state: 'pending',
+ updatedAtMs: this.#clock.now(),
+ }
+ this.#dispatchClaimStatuses.set(issueKey(record.issue), record.dispatchClaim)
+ }
await this.#writeInFlightRegistry()
</file context>
Required: verify against the built CLI, not just unit testsDo not mark this done on green unit tests alone. Build this repo and exercise the real Critical context firstThe Factory daemon running in production is So before writing a fix, establish that the defect actually exists on current main: npm ci && npm run build
node bin/factory.mjs --help # sanity: built CLI runs
node -e "console.log(require('./package.json').version)" # expect 0.1.57If the defect does NOT reproduce on a build of current main, stop and say so on the issue. The correct fix is then "release and deploy 0.1.57", not a code change. Reporting that is a success, not a failure — do not invent a change to justify the dispatch. Reproduce → fix → re-verify# #242 — dispatch state must reach GitHub or fail loudly
node bin/factory.mjs dispatch <ISSUE_KEY> --config <path>
node bin/factory.mjs status --config <path>Prove: after Attach the actual terminal output for the failing run and the passing run. A diff, a type signature, or a green test is not evidence that the CLI behaves correctly. Notes
|
Re-scope required — the issue description was wrong and has been rewrittenI filed #242 claiming dispatch state was "dropped before enqueue", citing What actually happensDispatch is not atomic. It has at least three effects — spawn implementer, spawn reviewer, write the GitHub claim ( The discriminator is which instance dispatched, not anything about the issue:
For 7¾ hours those three had a running implementer, no reviewer at all, and no label or comment on GitHub. The The write never reached relayfile's queue because it died inside Factory. Do not go looking for evidence in relayfile's writeback subsystem — that was my error. What this means for your PR
Also noteThe production daemon has been upgraded Verification requirements are in the rewritten issue. Kill Factory mid-dispatch and prove recovery happens without a restart. |
1 participant
Summary
factory statusCoordination
Related read-path work is in #246 for #240. This PR stays scoped to write acknowledgement, retries, and independent status visibility. Both touch
src/orchestrator/factory.ts, so #246 should be merged or rebased deliberately; no #240 behavior is duplicated here. #245 for #241 is confined to mount supervision.Verification
npm run buildnpm test: 1,554 passed; the dist-entrypoint test hit its 5s timeout under full-suite loadsrc/__tests__/dist-entrypoints.test.ts: passed in 1.5sgit diff --checkCloses #242
Summary by cubic
Makes dispatch lifecycle writes claim-critical, durable, and observable. Previously best‑effort comment/label writes could be skipped; now Factory verifies provider state, retries, dead‑letters on exhaustion, persists claim status, and exposes it via
factory status(addresses #242).factory:in-progressbefore the dispatch comment, confirms the label via provider read‑back, and retries both writes up to 3 times; on exhaustion, logs an error, marks the claim asdegraded, and fails the dispatch.pending,verified,degraded) in both the durable lifecycle and the in‑flight registry; durable recovery reuses the same claim path so comment failures cannot disappear.factory statusnow reportsinFlightDispatchesfrom the local registry, grouped by issue with agent details and the current claim; visible even when GitHub writeback is degraded.GhCliGithubWritebackrequires read‑back confirmation of label edits; rejects unconfirmed writes.dispatchWritebackFailures,dispatchWritebackRetries,dispatchWritebackDeadLetters, anddispatchClaimRegistryWriteFailures.Migration
loop.registryPathin your Factory config to enableinFlightDispatchesinfactory status.Written for commit d804202. Summary will update on new commits.