fix(babysitter): retry and reconcile PR-open snapshot reads so a failed read cannot orphan a PR - #250
fix(babysitter): retry and reconcile PR-open snapshot reads so a failed read cannot orphan a PR#250khaliqgant wants to merge 1 commit into
Conversation
…ot cannot orphan a PR A single failed PR-snapshot read at PR-open time permanently orphaned that PR from the babysitter. #handlePrChange was the only path that spawns a babysitter; one `fetch failed` from the mount ended it with a `debug` log and no retry, no dead-letter and no recovery. With no babysitter there is no owner, so every later event for that PR — reviews, comments, CI results, conflict state — was discarded by #routeBabysitterEvent, also at `debug`. The result was a completely silent failure at default log level. Retry: the PR-open read now retries with backoff before giving up. On exhaustion the path is dead-lettered rather than dropped. Reconcile: #sweepOrphanedBabysitterPrs runs off the existing completion-sweep timer (which previously early-returned in babysitter mode). It drains the dead-letter book and adopts still-ownerless in-flight records. Adoption replays the PR meta through #handlePrChange rather than spawning directly, so a reconcile is a second delivery of a missed event and remains subject to the same ownership, weak-match and path/payload-identity guards that event would have faced. Dead-letter retries are bounded per sweep and rotate; adoption runs on a slower cadence since it probes for PRs. Visibility: the read failure logs at `warn` and escalates to `error` (rate limited) once sweep retries also fail; an unowned PR event warns once per PR identity, then falls back to debug. A periodic summary reports babysitterPrSnapshotReadFailures, babysitterEventsIgnoredUnownedPr, babysitterFlatEventsUnreadable and dead-letter depth with the oldest stuck path. The AR-408 weak-match assertion moves from `toBe(1)` to `toBeGreaterThanOrEqual(1)`: the sweep replays that PR meta, so the guard is now exercised once per delivery attempt. It rejects every time, which is the property under test. Refs #243 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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: 84 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 (2)
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: 30ad6b5b2a
ℹ️ 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".
| const batch = await this.#batch() | ||
| for (const record of batch.inFlight) { | ||
| if (this.#stopping) return | ||
| if (record.dryRun || this.#hasBabysitterForIssue(record.issue)) continue |
There was a problem hiding this comment.
When an in-flight issue has multiple PRs and one already has a babysitter, this issue-level check skips the record entirely, so a second PR whose open event was missed is never examined or adopted. The ownership map and durable lifecycle explicitly support one babysitter per PR across multiple repositories, but after the first owner exists every later sweep continues past the whole issue and subsequent events for the orphaned PR remain unowned; reconciliation needs to compare each receipt's repo/number against existing ownership rather than treating any babysitter as sufficient.
Useful? React with 👍 / 👎.
| // ownership, weak-match and path/payload-identity guards as a live | ||
| // PR-open event — a reconcile sweep is a second delivery of a missed | ||
| // event, never a way around the checks that event would have faced. | ||
| await this.#handlePrChange(path) |
There was a problem hiding this comment.
On an adoption-due sweep during a broad readFile outage, this call retries every ownerless record returned from durable PR receipts, including paths just processed by the 16-entry dead-letter drain. Each #handlePrChange can sleep for 250 ms and 500 ms before failing, so 256 affected records can block the awaited run-loop/live-timer sweep for over three minutes despite BABYSITTER_PR_SNAPSHOT_DRAIN_PER_SWEEP; skip already-dead-lettered paths here or apply the same per-sweep bound to adoption reads.
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.
4 issues found across 2 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:1737">
P2: While a mount stays faulted, every run-loop/timer sweep drains up to 16 dead-lettered paths, and each path costs up to 3 reads plus 750ms of backoff sleeps (#readPrSnapshotContent). The escalate counter only throttles the error log, not the retries, so each sweep can block the dispatch loop for roughly a dozen seconds, and the sweep re-runs on every loop iteration (dead-letter retries are deliberately not gated by the adoption cadence). Consider skipping the backoff sleep during sweep-mode retries, or throttling sweep-mode dead-letter retries independently of the log-escalation counter, so a sustained mount outage slows, rather than stalls, the loop.</violation>
<violation number="2" location="src/orchestrator/factory.ts:1767">
P2: During an adoption-due sweep with a broad `readFile` outage, `#adoptOrphanedBabysitterPrs` calls `#handlePrChange` for every ownerless in-flight record's PR path with no per-sweep bound, unlike the dead-letter drain which caps itself at `BABYSITTER_PR_SNAPSHOT_DRAIN_PER_SWEEP`. Since each failing read can sleep up to 750ms (250ms + 500ms backoff) before dead-lettering, a large batch of ownerless records can block the awaited sweep for minutes. Apply the same per-sweep bound here, or skip paths already dead-lettered by `#retryDeadLetteredPrSnapshots` in the same sweep.</violation>
<violation number="3" location="src/orchestrator/factory.ts:1773">
P2: Every adoption run iterates over the full in-flight batch and, for each ownerless record, calls #orphanedPrMetaPaths, which walks #mount.listTree over every githubPullRoots(root) per candidate PR and, for non-durable-lifecycle records, issues an #openPrForIssue probe. On a 60s cadence this re-scans every in-flight issue even when no PR-open gap is plausibly present. Consider short-circuiting the scan when there is no reason to suspect an orphan (e.g., the record has no PR-open receipt and no dead-letter path, or gate the listTree walk on a cheap precondition) so the periodic reconciliation does not become repeated mount/gh work across a large fleet.</violation>
<violation number="4" location="src/orchestrator/factory.ts:1780">
P1: For a multi-repository issue, an existing babysitter for one PR suppresses adoption of every other ownerless PR. Track ownership by each `(repo, prNumber)` candidate and continue reconciling the remaining paths.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| this.#increment('babysitterOrphanCandidatesScanned') | ||
| try { | ||
| for (const path of await this.#orphanedPrMetaPaths(record)) { | ||
| if (this.#stopping || this.#hasBabysitterForIssue(record.issue)) break |
There was a problem hiding this comment.
P1: For a multi-repository issue, an existing babysitter for one PR suppresses adoption of every other ownerless PR. Track ownership by each (repo, prNumber) candidate and continue reconciling the remaining paths.
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 1780:
<comment>For a multi-repository issue, an existing babysitter for one PR suppresses adoption of every other ownerless PR. Track ownership by each `(repo, prNumber)` candidate and continue reconciling the remaining paths.</comment>
<file context>
@@ -1662,6 +1710,202 @@ export class FactoryLoop implements Factory {
+ this.#increment('babysitterOrphanCandidatesScanned')
+ try {
+ for (const path of await this.#orphanedPrMetaPaths(record)) {
+ if (this.#stopping || this.#hasBabysitterForIssue(record.issue)) break
+ // Replay the PR meta through the normal PR-open handler rather than
+ // spawning directly. Adoption must be subject to exactly the same
</file context>
|
|
||
| async #adoptOrphanedBabysitterPrs(): Promise<void> { | ||
| const batch = await this.#batch() | ||
| for (const record of batch.inFlight) { |
There was a problem hiding this comment.
P2: Every adoption run iterates over the full in-flight batch and, for each ownerless record, calls #orphanedPrMetaPaths, which walks #mount.listTree over every githubPullRoots(root) per candidate PR and, for non-durable-lifecycle records, issues an #openPrForIssue probe. On a 60s cadence this re-scans every in-flight issue even when no PR-open gap is plausibly present. Consider short-circuiting the scan when there is no reason to suspect an orphan (e.g., the record has no PR-open receipt and no dead-letter path, or gate the listTree walk on a cheap precondition) so the periodic reconciliation does not become repeated mount/gh work across a large fleet.
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 1773:
<comment>Every adoption run iterates over the full in-flight batch and, for each ownerless record, calls #orphanedPrMetaPaths, which walks #mount.listTree over every githubPullRoots(root) per candidate PR and, for non-durable-lifecycle records, issues an #openPrForIssue probe. On a 60s cadence this re-scans every in-flight issue even when no PR-open gap is plausibly present. Consider short-circuiting the scan when there is no reason to suspect an orphan (e.g., the record has no PR-open receipt and no dead-letter path, or gate the listTree walk on a cheap precondition) so the periodic reconciliation does not become repeated mount/gh work across a large fleet.</comment>
<file context>
@@ -1662,6 +1710,202 @@ export class FactoryLoop implements Factory {
+
+ async #adoptOrphanedBabysitterPrs(): Promise<void> {
+ const batch = await this.#batch()
+ for (const record of batch.inFlight) {
+ if (this.#stopping) return
+ if (record.dryRun || this.#hasBabysitterForIssue(record.issue)) continue
</file context>
| this.#babysitterOrphanSweepActive = true | ||
| try { | ||
| this.#increment('babysitterOrphanSweepRuns') | ||
| await this.#retryDeadLetteredPrSnapshots() |
There was a problem hiding this comment.
P2: While a mount stays faulted, every run-loop/timer sweep drains up to 16 dead-lettered paths, and each path costs up to 3 reads plus 750ms of backoff sleeps (#readPrSnapshotContent). The escalate counter only throttles the error log, not the retries, so each sweep can block the dispatch loop for roughly a dozen seconds, and the sweep re-runs on every loop iteration (dead-letter retries are deliberately not gated by the adoption cadence). Consider skipping the backoff sleep during sweep-mode retries, or throttling sweep-mode dead-letter retries independently of the log-escalation counter, so a sustained mount outage slows, rather than stalls, the loop.
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 1737:
<comment>While a mount stays faulted, every run-loop/timer sweep drains up to 16 dead-lettered paths, and each path costs up to 3 reads plus 750ms of backoff sleeps (#readPrSnapshotContent). The escalate counter only throttles the error log, not the retries, so each sweep can block the dispatch loop for roughly a dozen seconds, and the sweep re-runs on every loop iteration (dead-letter retries are deliberately not gated by the adoption cadence). Consider skipping the backoff sleep during sweep-mode retries, or throttling sweep-mode dead-letter retries independently of the log-escalation counter, so a sustained mount outage slows, rather than stalls, the loop.</comment>
<file context>
@@ -1662,6 +1710,202 @@ export class FactoryLoop implements Factory {
+ this.#babysitterOrphanSweepActive = true
+ try {
+ this.#increment('babysitterOrphanSweepRuns')
+ await this.#retryDeadLetteredPrSnapshots()
+ if (adoptionDue) {
+ this.#babysitterOrphanSweepLastRunMs = now
</file context>
| // #handlePrChange clears the entry itself on a successful read and | ||
| // re-records the failure (bumping `failures`, escalating the log) if the | ||
| // mount is still faulted, so the whole retry contract lives in one place. | ||
| await this.#handlePrChange(path) |
There was a problem hiding this comment.
P2: During an adoption-due sweep with a broad readFile outage, #adoptOrphanedBabysitterPrs calls #handlePrChange for every ownerless in-flight record's PR path with no per-sweep bound, unlike the dead-letter drain which caps itself at BABYSITTER_PR_SNAPSHOT_DRAIN_PER_SWEEP. Since each failing read can sleep up to 750ms (250ms + 500ms backoff) before dead-lettering, a large batch of ownerless records can block the awaited sweep for minutes. Apply the same per-sweep bound here, or skip paths already dead-lettered by #retryDeadLetteredPrSnapshots in the same 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 1767:
<comment>During an adoption-due sweep with a broad `readFile` outage, `#adoptOrphanedBabysitterPrs` calls `#handlePrChange` for every ownerless in-flight record's PR path with no per-sweep bound, unlike the dead-letter drain which caps itself at `BABYSITTER_PR_SNAPSHOT_DRAIN_PER_SWEEP`. Since each failing read can sleep up to 750ms (250ms + 500ms backoff) before dead-lettering, a large batch of ownerless records can block the awaited sweep for minutes. Apply the same per-sweep bound here, or skip paths already dead-lettered by `#retryDeadLetteredPrSnapshots` in the same sweep.</comment>
<file context>
@@ -1662,6 +1710,202 @@ export class FactoryLoop implements Factory {
+ // #handlePrChange clears the entry itself on a successful read and
+ // re-records the failure (bumping `failures`, escalating the log) if the
+ // mount is still faulted, so the whole retry contract lives in one place.
+ await this.#handlePrChange(path)
+ }
+ }
</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# #243 — an orphaned PR must be recoverable
node bin/factory.mjs babysit 3024 --config <path> # AgentWorkforce/cloud#3024
Note: 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
|
1 participant
Fixes #243.
The problem
#handlePrChangeis the only path that spawns a babysitter. It read the PR meta through the mount and, on any failure, logged atdebugand returned — no retry, no dead-letter, no recovery path. That read is failing in production ("error":"fetch failed", 364 occurrences).No spawn means no owner, so
#routeBabysitterEventthen discarded every subsequent event for that PR — reviews, comments, CI results, conflict state — also atdebug. At default log level the babysitter failed completely silently.What changed
1. The PR-open read retries.
#readPrSnapshotContentretries 3× with linear backoff (250ms, 500ms). A transient mount fault no longer decides permanently that a PR is unshepherded. On exhaustion the path is dead-lettered instead of dropped.Only the
readFileis retried.parsePullSnapshotnever throws — anundefinedresult is a structurally mismatched payload, which a retry cannot fix — so it stays outside the retry and is not dead-lettered.2. Orphaned PRs are recoverable.
#sweepOrphanedBabysitterPrshangs off the existing completion-sweep timer, which previously just early-returned when the babysitter was enabled. It:Adoption replays the PR meta through
#handlePrChangerather than spawning directly. This was deliberate: a reconcile sweep is a second delivery of a missed event, never a way around the checks that event would have faced. It therefore stays subject to the existing ownership, weak-match (< 30) and path/payload-identity guards. An earlier draft called#ensureBabysitterForIssuedirectly and broke two existing guard tests (AR-408 body-only reference, AR-430 payload/path number mismatch) by spawning babysitters those guards exist to prevent — replaying through the validated path fixes that by construction.If a PR is known from a dispatch receipt but its meta has not materialized in the mount, adoption skips it (counted as
babysitterOrphanPrMetaUnavailable) and retries next sweep, rather than spawning off an unvalidated receipt.3. Failures are visible at default log level.
could not read PR snapshot:debug→warn, escalating toerroronce sweep retries also fail. The escalated log is rate limited (every 20th failure) so a durably faulted mount cannot flood.ignored unowned PR event: warns once per PR identity (capped at 64), then falls back todebug. The cap gates the warn itself, so an unbounded set of identities cannot become an unbounded warn stream.4. Counters are surfaced. A periodic warn reports
babysitterPrSnapshotReadFailures,babysitterEventsIgnoredUnownedPr,babysitterFlatEventsUnreadable, adoption count, and dead-letter depth — plus the oldest stuck path, its error and its age, which is what an operator actually needs to act on.babysitterPrSnapshotDeadLetterDepthis also published intostatus().counters.Bounding
Tests
Four new regression tests, plus the full suite green (1556 passed, 83 files).
still spawns the babysitter when the PR-open snapshot read fails transiently— first read throwsfetch failed; the babysitter still spawns and nothing is dead-lettered.dead-letters an exhausted PR snapshot read and recovers it on the reconcile sweep— mount stays faulted, read exhausts, no spawn,warnemitted; mount recovers, sweep adopts. The probe resolver returnsundefinedhere so recovery can only come from the dead-letter drain.adopts an already-open PR that no babysitter owns on the reconcile sweep— no PR-open event is emitted at all; the sweep adopts, and a second sweep does not respawn.warns once per unowned PR identity instead of silently dropping its events at debug.One existing assertion changed: AR-408's
babysitterPrDiscoveryWeakMatchIgnoredmoves fromtoBe(1)totoBeGreaterThanOrEqual(1). The sweep replays that PR meta, so the guard is exercised once per delivery attempt. It rejects every time; the no-spawn assertion is unchanged.On verifying against
cloud#3024The issue asked to verify against
AgentWorkforce/cloud#3024. That PR has since been closed (state: CLOSED, stillCONFLICTING/DIRTY), so I could not verify against it live.What I did verify: its head branch is
factory/3022-chief-org-live-population, andfactoryBranchMatchesIssue('factory/3022-chief-org-live-population', '3022')matches thefactory/<issue>-prefix, scoring 30 — at the weak-match floor, so it passes. Given an in-flight record for that issue and its PR meta in the mount, the sweep would have adopted it. Theadopts an already-open PR that no babysitter ownstest reproduces exactly that shape.Scope
Per the issue: branched from
origin/main(the localcodex/222-routed-pr-babysitter-v2WIP was left untouched),mergePolicy: never— stopping at review. Kept distinct from #222 (babysitter scope) and #221 (babysitter write identity); this only addresses in-scope PRs never being routed at all.🤖 Generated with Claude Code
Summary by cubic
Retries and reconciles PR-open snapshot reads so a failed read can’t orphan a PR. Previously, a single mount read failure at PR open logged at debug, never spawned a babysitter, and caused all later PR events to be dropped; now reads retry with backoff, dead-letter on exhaustion, and a sweep reconciles orphaned PRs. Addresses #243 by making orphaned PRs recoverable and failures observable.
readFileis retried;parsePullSnapshotremains non-throwing and is not dead-lettered.#handlePrChange(respects ownership, weak-match, and path/payload identity guards). Cadence 60s; dead-letter retries run each sweep. Bounds: dead-letter book 256; drain 16 per sweep.babysitterPrSnapshotReadFailures,babysitterPrSnapshotReadRetries,babysitterPrSnapshotReadRetrySucceeded,babysitterEventsIgnoredUnownedPr,babysitterFlatEventsUnreadable,babysitterOrphanedPrsAdopted, plusbabysitterPrSnapshotDeadLetterDepthinstatus().counters.toBeGreaterThanOrEqual(1)since the sweep replays the guarded path.Written for commit 30ad6b5. Summary will update on new commits.