fix(factory): skip the orphan-recovery context on a dry-run sweep (#348) - #349
Conversation
`#githubOrphanRecoveryContext()` read `fleet.roster()` whenever the issue source is GitHub, regardless of `dryRun`. Since #343 a read-only CLI client refuses to mint a workspace identity, and `roster()` mints on demand — so the container's start-disabled `run-once --dry-run` gate logged `could not establish orphan-recovery safety context` on every sweep. The context was already provably unused on that path: `mayRecoverGithubOrphan` in `#performRunOnce` is `!dryRun`, and `#reconcileOrphanedGithubInProgress` refuses under `dryRun` before it so much as looks at the context. Gathering it was not free, though — it required an identity to decide what the sweep WOULD do. Now a dry run skips it and the report says so, via a new `IterationReport.orphanRecoveryDegraded` that distinguishes the benign `dry-run` skip from a live sweep's real `context-unavailable` degradation. The failure counter and warn stay for the live path, where an absent context IS a degradation. Tests are a must-fire / must-not-fire pair plus a control, all on one fixture: a dry-run sweep does not read the roster (measured with a fleet that answers normally, so the zero is the guard and not a refusal); a live sweep still reads it and still recovers the orphan; and a dry run against a client shaped like #343's refusal completes rather than logging a failure. Ablating the guard fails both must-not-fire arms; inverting it fails all three. NOT fixed here, and #348's diagnosis is wrong about it: this does not bring `run-once --dry-run` under its 10-minute budget, because #343 never took it over. See the PR comment for the measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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 |
|
@coderabbitai review Requested for exact head |
Report — factory-348-dryrun-0823What landedThe deliverable, and only the deliverable: Test arms and their mutation results are in the PR body. Both mutations were actually run, not reasoned about: ablating the guard fails the two must-not-fire arms on What I could not honestly deliverThis does not bring the start-disabled deploy under its 10-minute budget, and #343 never took it over. The full measurement is in the PR body; the three load-bearing facts:
I did not raise the budget, and nothing in the measurement suggests it is wrong. I also did not fix the fan-out: the two remedies (bounded concurrency vs. newest-first ordering plus a cap) trade off against #297's overload-sensitivity and against dependency-resolution correctness respectively, and that is a call for whoever owns the merge, not for a #348 lane to make unilaterally. Side items — reported, not changed
CIPer-job, run
The 5 failures are a red base, not this PR, and not #342. Main's own CI is already failing at my base commit Four of the five are in My three new tests ran in this CI job and are in the 2068 passed. Diff is confined to
RecommendationMerge this on its own terms — it is correct, tested, and removes a spurious failure signal from every dry run. Then, separately: reopen the 10-minute timeout against |
|
@coderabbitai review Requested for exact head |
|
Merging. This fixes the regression I introduced by merging #343, and it clears the bar cleanly. All 5 jobs green at The tests are what I asked for and the control is the part that matters: Without that second test the first one passes if orphan recovery is broken everywhere, which is a strictly worse bug than the one being fixed. Asserting both directions is what makes the skip provably scoped to dry runs. And you honoured the hard constraint: the 10-minute budget is untouched. I checked the whole diff for it. Raising the timeout would have hidden the regression rather than fixing it, and Note your CI was red earlier through no fault of yours: This restores the start-disabled rehearsal deploy — the gate that proves a full cycle runs against the real host without side effects, which we lost this evening and which I want back before the next release. |
#359) * fix(health): a deferred sweep must not erase the last real measurement CodeRabbit, Major on #358, and correct. `#recordReadinessSweepOutcome` folded a deferred report into the counts snapshot like any other. A deferred pass settles successfully in milliseconds having enumerated nothing, so its zeroes replaced the last enumerating sweep's numbers — and where another process holds the discovery lease for any length of time (the #347/#349 condition), EVERY pass would publish `candidates: 0` and the measurement this whole change exists to provide would be unrecoverable. The marker now lives apart from the counts. `candidates`/`dispatched`/`skipped` describe the last sweep that ENUMERATED; `discoveryDeferred` describes the most recent pass. Present together they say "these numbers are from an earlier pass"; present alone it says "nothing has enumerated yet, and a held lease is why" — which the previous shape could not express at all, because the projection dropped the marker whenever the trio was absent. Not taken from the review: freezing `lastCompletedAtMs` on deferral. The #295/#296 stall derivation reads that timestamp against `lastStartedAtMs`, so freezing it would leave `lastStarted > lastCompleted` on every deferred pass and report a daemon that is correctly deferring to another owner as hung after ten intervals — a false alarm on the exact surface this work exists to make trustworthy. It still moves; the test asserts that it does, and says why. Also CodeRabbit, trivial: `docs/deployed-diagnostics.md` listed four of the fourteen skip codes. All fourteen are now documented, grouped by what an operator should do about each, with the `other` fold-in stated. Tests: a sweep that enumerates, then deferred passes, asserting the counts survive, the marker appears, and `lastCompletedAtMs` still advances; the first-pass-defers case asserting the marker publishes with no counts at all; and a projection unit test for the same independence. Verified by ablation — restoring the old single-record write fails both deferral tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(health): date the retained counts with their own measurement timestamp Review hold on #359, and correct. Separating the deferral marker from the arithmetic stopped a deferred pass zeroing the last enumeration, but left the retained counts with no time coordinate: `#reconcileReadyIssues` advances `lastCompletedAtMs` on every settled pass including a deferred one, while the counts now stay put. So the payload paired arbitrarily old `candidates`/`dispatched`/`skipped` with an ever-fresh completion stamp, and a reader could see that the newest pass deferred but not whether the measurement was one interval or four days old — the freshness this surface exists to expose. It also made the contract contradict itself: `types.ts` said `lastCompletedAtMs` identified the pass the counts describe, and the docs said both that and the opposite two paragraphs later. `lastEnumeratedAtMs` is now part of the same atomic snapshot as the counts, written only when a pass actually enumerates, and projected through the authenticated status, the public health record and `factory diagnose`. Equal to `lastCompletedAtMs` on a daemon sweeping normally; where they differ, the gap is exactly how stale the counts are. The recorder takes the caller's completion stamp rather than reading the clock again, so on an enumerating pass the two cannot drift apart by a tick — which is what makes the equality assertable. `lastCompletedAtMs` still moves on deferral, unchanged: the #295/#296 stall derivation reads it against `lastStartedAtMs`, and freezing it would report a daemon correctly deferring to another owner as hung after ten intervals. Two codex P2s on the same head, both real: - The completion log drew `skipReasons` from the retained snapshot while drawing the counts from the current report, so a deferred pass printed `skipped: 0` beside a non-empty breakdown — a line contradicting its own arithmetic, on the surface a local operator reads. It now derives the breakdown from the report it is describing. - The docs sent `read-failed` and `dispatch-failed` to `lastErrorClass`. Those codes count per-item failures an otherwise-successful pass absorbed and continued past (#292/#297), and the success path clears `lastErrorClass` — so the guidance pointed at a field guaranteed absent in exactly that scenario. Reworded to name the container-log lines that do carry the detail, and to note that a rising `read-failed` beside `state: healthy` is the #297 signature. Tests: the enumeration stamp equals the completion stamp on a pass that enumerated, stays pinned across repeated deferrals while `lastCompletedAtMs` advances past it, and is absent until a sweep enumerates; plus a recording-logger test asserting every deferred completion line is internally consistent while the published surface still retains the real measurement. Verified by ablation — sourcing the stamp from `lastCompletedAtMs` fails the deferral test on the equality, and restoring the retained-breakdown log fails the new log test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(cli): scope deferral wording to latest pass * fix(health): keep sweep outcome attribution current * docs(factory): align sweep count terminology * docs(health): name enumerating sweep consistently * test(factory): wait for actual late placement race * fix(diagnose): classify legacy deferred counters * fix(diagnose): preserve rejected sweep evidence --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Fixes the deliverable in #348: a dry run must not need a workspace identity to decide what it would do.
What changed
#githubOrphanRecoveryContext()readfleet.roster()whenever the issue source is GitHub, regardless ofdryRun. Since #343 a read-only CLI client refuses to mint a workspace identity, androster()mints on demand — so the container's start-disabledrun-once --dry-rungate loggedcould not establish orphan-recovery safety context; preserving in-progress issueson every sweep.The context was already provably unused on that path:
mayRecoverGithubOrphanin#performRunOnceis!dryRun, and#reconcileOrphanedGithubInProgressreturns'dry run does not release an in-progress claim'before it so much as looks at the context. Skipping it is therefore behaviour-preserving for what the sweep records — it only stops the sweep needing an identity it never uses.IterationReport.orphanRecoveryDegradednow reports the state, distinguishing the two causes that are not the same event:'dry-run'— expected and benign, the sweep never intended to release a claim'context-unavailable'— a live sweep tried to build the context and could notThe
githubOrphanRecoveryContextFailurescounter and its warn stay on the live path, where an absent context genuinely is a degradation.Tests
A must-fire / must-not-fire pair plus a control, all on one orphan-shaped fixture:
roster()normally, so the zero is the guard and not a refusaldryRundiffers: roster is read, the orphan is recovered,orphanRecoveryDegradedis absentAblated (guard removed): both must-not-fire arms fail on
expected 1 to be +0, the control still passes. Inverted (!dryRun): all three fail. Verified locally, not assumed.I was asked to measure before assuming. I did, and the measurement does not support the issue's mechanism. Three findings, all from the deploy logs themselves.
1. The A/B is confounded — the 0.1.69 "success" never ran the sweep
#348 states that
phase: readyon run32661804498is "positive proof the sweep ran and finished". That run's own probe record says otherwise:performed no work. The probe exited in 17s because it lost a lease race, and the gate tolerated that as a pass. The 0.1.70 run (32660636060) claimed the lease (epoch 116) and ran the full sweep. The two runs differ in lease ownership, not just in version.phase: readydiscriminates "the probe returned tolerably", not "the sweep completed".2. The same 600s SIGTERM happens on 0.1.63, seven releases before #343
Scanning the last 40
Deploy Factoryruns, exactly four executed arunOnceDryRunprobe:3226236555232282623135326618044983266063606032282623135(2026-08-19, 0.1.63) predates #343 by four days and seven releases. It has no read-only client and no orphan-recovery warn — orphan recovery succeeded there — and it still hit the identical 600s SIGTERM with the identical trajectory. Every start-disabled deploy that actually ran the sweep has timed out, before and after #343. #343 did not regress this; it made an unrelated pre-existing failure the only remaining outcome, because the 0.1.69 comparison happened to skip the work.3. Where the 10 minutes actually goes: an uncapped sequential PR-candidate read
Both timed-out runs end the same way — and
stdoutBytes: 0,stderrBytes: 4544. The process emitted ~4.5KB and then went completely silent for the rest of the ten minutes. It hung mid-sweep; it did not finish and fail to exit.The last thing either run logged before going silent:
resolveIssuePrFromMount(src/orchestrator/factory.ts:18979) then does this, per dependency identity:Every PR in
AgentWorkforce/cloud, read one at a time, with no candidate cap and no progress logging. At ~150ms per remote read that is ~10 minutes of silence — which is exactly the observed signature. The siblingghresolver caps atPROBE_PR_GH_CANDIDATE_LIMIT = 200and warns when it truncates; the mount resolver has no equivalent.I have not fixed this here. The two obvious remedies trade off against each other and the choice is yours, not mine to make unilaterally in a #348 lane:
DEFAULT_LOCAL_MOUNT_MAX_CONCURRENCY = 4).ghresolver — cheap and bounded, but on a 2,831-PR repo a cap small enough to help could stop resolving an older PR, silently weakening the dependency-merged check.Either way it wants progress logging: a stage that can burn ten minutes should not be invisible.
I did not touch the 10-minute budget. Nothing in the measurement suggests it is wrong; the stall it caught is real.
The two side items
.integrations/discovery→missing required scope: fs:read— present in the 0.1.63 run too, so also not from fix(cli): stop read-only commands registering a relay agent (factory-cloud#55) #343.deriveDescriptorsFromMountemits paths relative to the local.integrationsmirror, and#doResolveIntegrationInstructions(factory.ts:1085) passes.integrations/discoverystraight to the cloudlistTree, where no such remote path exists andFACTORY_RELAYFILE_SCOPESgrants nothing for it. It is a local-mirror path used as a remote one. Non-fatal (caught, logged) — left alone rather than guessed at, since the correct remote prefix depends on the descriptor package's contract.unref'd timer, so it is not the ten minutes; it is noise plus a real relayfile-side symptom (the mount stops reconciling ~90s after each successful relaunch, so#superviseLocalMountre-heals it forever with no backoff and no cap). Worth its own issue. NoteLocalMountOptions.refreshStaleMountexists and no production caller ever sets it — a read-only command has no writeback to protect and arguably should.Suggested next steps
resolveIssuePrFromMount, not against fix(cli): stop read-only commands registering a relay agent (factory-cloud#55) #343, with finding 3 as the starting point.🤖 Generated with Claude Code
Summary by cubic
Skips building the GitHub orphan‑recovery safety context during dry‑run sweeps so
run-once --dry-runno longer attempts to mint a workspace identity. Previously dry runs read the fleet roster and failed under the read‑only client from #343; now dry runs preserve in‑progress issues and report this explicitly.#githubOrphanRecoveryContextondryRun; adds an info log andgithubOrphanRecoveryContextSkippedDryRuncounter; includesorphanRecoveryDegradedin the run summary.orphanRecoveryDegraded: 'dry-run'. Live sweeps are unchanged; if the context cannot be built, report sets'context-unavailable'and the existing warn/counter still signal a real degradation.IterationReport.orphanRecoveryDegradedis additive.Written for commit aadd633. Summary will update on new commits.