fix(factory): reconcile readiness claims after restart - #246
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: 113 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 (4)
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: 1e8de2c456
ℹ️ 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".
| } catch (error) { | ||
| // The roster was checked twice before fencing this lifecycle, so a | ||
| // missing/dead worker is the expected crash-recovery shape. Do not | ||
| // let a control-plane "agent not found" response make the durable | ||
| // claim immortal; a failed fresh spawn remains visible to the normal |
There was a problem hiding this comment.
When a roster snapshot omits a worker but release() fails with a timeout, authorization error, or backend failure, this catch treats the error exactly like a verified “agent not found”: it marks the agent terminal and proceeds to clear the lifecycle before redispatching. The original worker may therefore remain active while a replacement is spawned, producing duplicate work and conflicting writebacks. Only an explicitly recognized not-found response should be ignored; other release failures should preserve the lifecycle for retry.
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.
2 issues found across 4 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/factory.ts">
<violation number="1" location="src/orchestrator/factory.ts:1811">
P3: The new `recordSkip` is wired into every skip in `runOnce`, which is also the code path for startup backfill, the `run-once` CLI action, and the run loop — not just the periodic readiness reconcile. The emitted log text '[factory] readiness reconciliation skipped dispatch' therefore mislabels skips that occur during those non-reconcile runs. Make the log context-neutral (e.g. mention the run source) or only emit this message from the periodic sweep.</violation>
<violation number="2" location="src/orchestrator/factory.ts:2008">
P1: During an active dispatch with no roster-visible agent, this check is always false because `dispatch()` stores composite keys, not `issueKey` keys. The sweep can release that live lifecycle and redispatch the issue; detect any in-flight key for the issue before classifying the claim as orphaned.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const activeAgents = lifecycle.agents.filter((agent) => agent.releasedAtMs === undefined) | ||
| const hasLiveAgent = activeAgents.some((agent) => onlineAgents.has(agent.name)) | ||
| const exitRecoveryActive = activeAgents.some((agent) => this.#agentExitsInFlight.has(agent.name)) | ||
| const dispatchCallActive = this.#dispatchInFlight.has(issueKey(lifecycle.issue)) |
There was a problem hiding this comment.
P1: During an active dispatch with no roster-visible agent, this check is always false because dispatch() stores composite keys, not issueKey keys. The sweep can release that live lifecycle and redispatch the issue; detect any in-flight key for the issue before classifying the claim as orphaned.
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 2008:
<comment>During an active dispatch with no roster-visible agent, this check is always false because `dispatch()` stores composite keys, not `issueKey` keys. The sweep can release that live lifecycle and redispatch the issue; detect any in-flight key for the issue before classifying the claim as orphaned.</comment>
<file context>
@@ -1924,15 +1988,41 @@ export class FactoryLoop implements Factory {
+ const activeAgents = lifecycle.agents.filter((agent) => agent.releasedAtMs === undefined)
+ const hasLiveAgent = activeAgents.some((agent) => onlineAgents.has(agent.name))
+ const exitRecoveryActive = activeAgents.some((agent) => this.#agentExitsInFlight.has(agent.name))
+ const dispatchCallActive = this.#dispatchInFlight.has(issueKey(lifecycle.issue))
+ // The provider's `factory:in-progress` transition happens immediately
+ // before the durable lifecycle advances from dispatching to running.
</file context>
| const dispatchCallActive = this.#dispatchInFlight.has(issueKey(lifecycle.issue)) | |
| const dispatchCallActive = [...this.#dispatchInFlight.keys()].some((key) => | |
| key.startsWith(`${issueStateKey(lifecycle.issue)}:`), | |
| ) |
| const triaged: TriageDecision[] = [] | ||
| const dispatched: DispatchResult[] = [] | ||
| const skipped: IterationReport['skipped'] = [] | ||
| const recordSkip = (entry: IterationReport['skipped'][number]): void => { |
There was a problem hiding this comment.
P3: The new recordSkip is wired into every skip in runOnce, which is also the code path for startup backfill, the run-once CLI action, and the run loop — not just the periodic readiness reconcile. The emitted log text '[factory] readiness reconciliation skipped dispatch' therefore mislabels skips that occur during those non-reconcile runs. Make the log context-neutral (e.g. mention the run source) or only emit this message from the periodic sweep.
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 1811:
<comment>The new `recordSkip` is wired into every skip in `runOnce`, which is also the code path for startup backfill, the `run-once` CLI action, and the run loop — not just the periodic readiness reconcile. The emitted log text '[factory] readiness reconciliation skipped dispatch' therefore mislabels skips that occur during those non-reconcile runs. Make the log context-neutral (e.g. mention the run source) or only emit this message from the periodic sweep.</comment>
<file context>
@@ -1755,6 +1808,14 @@ export class FactoryLoop implements Factory {
const triaged: TriageDecision[] = []
const dispatched: DispatchResult[] = []
const skipped: IterationReport['skipped'] = []
+ const recordSkip = (entry: IterationReport['skipped'][number]): void => {
+ skipped.push(entry)
+ this.#logger.info?.('[factory] readiness reconciliation skipped dispatch', {
</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# #240 — an issue labeled while Factory is down must still dispatch
node bin/factory.mjs canary <ISSUE_KEY> --config <path> # dispatch-readiness check
node bin/factory.mjs run-once --config <path> --dry-run # one discovery->triage->dispatch cycle
node bin/factory.mjs reap-orphans --config <path> # orphaned claim releaseProve: stop Factory, label an issue, restart, and show 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
|
Partial: the orphaned-claim half is already fixed in mainIssue #240 has two halves. One of them does not exist in the code you are editing. Already fixed — do not reimplement. The running daemon is
Still open and unverified — this is where your effort belongs. #240 item 1: an issue labeled while Factory is down must still dispatch after restart, without a manual label re-emission. There is a discovery sweep in main independent of the live-event watermark, but I have not confirmed empirically that it covers this case. So please establish it directly, on a build of current main: npm ci && npm run build
# 1. stop Factory 2. add the readiness label to an issue 3. restart
node bin/factory.mjs run-once --config <path> --dry-run
node bin/factory.mjs canary <ISSUE_KEY> --config <path>Show the issue is picked up with no manual label edit. Attach real terminal output for the failing and passing runs. If it already works on main, close this PR and report "already fixed, needs deploy of 0.1.57" — that is a successful outcome, not a failure. Scope any code change to the half that genuinely does not work. |
1 participant
Change
Fixes #240.
Factory now treats the event watermark as a latency optimization instead of the only durable path to work:
liveSubscription.reconcileIntervalMs, default 60s).factory:in-progressis applied while the durable row is stilldispatching; missing-agent release errors are visible but cannot make the fenced claim immortal.The existing failure-handoff orphan reaper only receives records from a caught dispatch failure. A hard process death bypasses that handoff, while the surviving nonterminal lifecycle was previously classified as active forever. The new reconciliation path handles that hard-death shape directly.
Merge policy: never. This PR is opened for principal review and must not be auto-merged.
Verification
packagecheck passed on the current head SHA. (Pending GitHub CI.)factory-e2e-<head-sha>attestation artifact was inspected. (Pending uploaded CI artifact; the equivalent local attestation passed and was inspected.)Verification tier: Tier 4 — recovery/durability
Head SHA tested:
1e8de2c4568b955419783f77079387dcee527942Local verification:
npm run buildnpm run featuremap:check— 313 features, no advisoriesnpx vitest run src/orchestrator/factory.test.ts— 461/461 passeddispatchingclaim, and visible active-claim skip: 4/4 passedmise exec node@20 -- npm run verify:e2e— run-cost tests 6/6 and SHA-bound packaged lifecycle attestation 11/11 passedmise exec node@20 -- npm test— 1,553/1,555 passed in the parallel run; two unrelated 5-second timeout cases passed 13/13 when rerun together in isolationAdapter/preview evidence: N/A — no adapter implementation changed; source reconciliation is covered with issue-index/tree and live-event fakes.
Live canary issue / PR / run: Not run from this safe checkout. Required Tier 3/4 live evidence remains a reviewer/principal merge-gate obligation.
Failure-injection or load evidence: Unit failure injection covers an event below the successor watermark, readiness appearing without any event, a dead
dispatchinglifecycle, and control-planeagent not foundduring stale-worker release. No external load run was created.Cleanup receipt: No disposable external resources were created. Work was performed in isolated worktree
/Users/khaliqgant/Projects/AgentWorkforce/factory-issue-240; the original WIP checkout was not modified.Summary by cubic
Reconciles readiness independently of event watermarks and safely recovers orphaned GitHub in‑progress claims after restarts/crashes. Previously, readiness depended on live event watermarks and a caught‑failure handoff; hard process death could leave
factory:in-progressissues permanently active. Now, a periodic source‑of‑truth sweep backfills and releases orphaned durable claims before redispatch.reconcileIntervalMstoliveSubscriptionconfig andFactoryLiveSubscriptionOptions(default 60s) to run a periodic readiness sweep; event watermarks are now a latency optimization.reconcileIntervalMs. Review focus: new readiness sweep timer and orphan‑recovery flow in the factory loop; tests cover restart above watermark, eventless sweep, and deaddispatchingclaims.Written for commit 1e8de2c. Summary will update on new commits.