fix(orchestrator): bound the readiness reconcile sweep so a hang cannot stop the loop (#296)

[khaliqgant]

Fixes #296.

The defect

#reconcileReadyIssues() had no bounded deadline. A single runOnce() that never settles stopped the readiness reconcile loop permanently and silently:

• #scheduleReadinessReconcile re-arms only from sweep.finally(...), so a pending promise is never rescheduled.
• Both state-writing paths run on settle — success sets lastCompletedAtMs and clears lastError; failure sets lastFailureAtMs and increments consecutiveFailures. A hang takes neither, so the last successful pass's values stand forever.

Production reported state=healthy, consecutiveFailures=0, lastError=none for 104 minutes while dispatching nothing. Only a process restart recovered it.

The fix

1. Bound the sweep. #runOnceWithReadinessDeadline() races runOnce() against reconcileTimeoutMs. On expiry it rejects, which routes into the existing failure path — the one that already increments consecutiveFailures, records lastError, marks degraded past the threshold, and re-arms the loop. No parallel recovery machinery was built; the existing one was simply unreachable from a hang.

The sweep itself is not cancelled — runOnce() owns a durable discovery lease, and abandoning it mid-flight is not safe. Expiry rejects the wait and leaves the pass to finish. That is sufficient: the rejection is what re-arms the timer. A later pass coalesces onto the still-running runOnce() (existing behavior) and fails on its own deadline too, so a persistent hang keeps counting up to degraded instead of going quiet.

2. Own the abandoned wait. Because the deadline bounds the wait and not the sweep, the abandoned work is still live. It is tracked and drained by stop(), so shutdown still outlives the sweep it started, and the reported state keeps ageing from when that work actually began. Details in "Review rounds" below — this half of the design is entirely the product of the bot review.

3. Derive the reported state. state is computed on read rather than last-write-wins, adding the missing stalled value for work in flight beyond READINESS_RECONCILE_STALL_INTERVALS (3) × the interval. degraded deliberately stays on top of the ladder: it is what existing monitors alert on, and a stall arriving on top of repeated failures must not silence that alarm.

The must-not-fire control, and why the deadline is 90 minutes

DEFAULT_READINESS_RECONCILE_TIMEOUT_MS = 90 * 60_000. It is not a small multiple of the 60s interval, and the comment on the constant says so in as many words.

#36 measured a real cold-mirror reconcile at 3,665,173 ms (61 minutes) in production, because container disk is ephemeral and the Relayfile mirror rehydrates on every boot. A deadline under realistic worst-case hydration converts a slow boot into a crash loop, which is strictly worse than the bug being fixed. 90 minutes leaves ~47% headroom over that measurement.

The two thresholds are decoupled on purpose: killing a pass must tolerate a 61-minute hydration, but reporting that nothing has dispatched for three intervals costs nothing. That is why stalled fires at 3 minutes while the deadline sits at 90 — the safe deadline does not cost observability.

Config also refuses reconcileTimeoutMs < reconcileIntervalMs (schema superRefine), and start() overrides — which bypass the schema — re-apply the same floor.

Tests, fail-first

All in describe('bounded readiness reconciliation'). Against the unmodified source:

 ❯ src/orchestrator/factory.test.ts (537 tests | 3 failed | 533 skipped)
   × rejects a never-settling sweep on its deadline and schedules the next pass
   × reports a pass still in flight past the stall threshold as stalled rather than healthy
   × defaults the sweep deadline above the measured worst-case cold-mirror hydration

FAIL > rejects a never-settling sweep on its deadline and schedules the next pass
AssertionError: expected 0 to be greater than or equal to 1
 ❯ expect(status.readinessReconcile?.consecutiveFailures ?? 0).toBeGreaterThanOrEqual(1)

FAIL > reports a pass still in flight past the stall threshold as stalled rather than healthy
AssertionError: expected { state: 'healthy', …(3) } to match object { state: 'stalled', …(2) }
  {
    "consecutiveFailures": 0,
    "lastStartedAtMs": 1787245293837,
-   "state": "stalled",
+   "state": "healthy",
  }

FAIL > defaults the sweep deadline above the measured worst-case cold-mirror hydration
TypeError: actual value must be number or bigint, received "undefined"
 ❯ expect(config().liveSubscription.reconcileTimeoutMs).toBeGreaterThan(3_665_173)

Each fails for the right mechanism, not just the right colour:

• must-fire — a runOnce() that never settles leaves consecutiveFailures at 0 and the sweep counter frozen: the failure path is never reached and the loop is never rescheduled. It then asserts the loop reaches degraded.
• stall reporting — the pre-change actual is literally the production evidence shape: state: 'healthy', consecutiveFailures: 0, a lastStartedAtMs from a pass that never completed, and no lastCompletedAtMs.
• deadline floor — the fix(factory): replay existing Slack triage answers #36 regression guard. This is what fails if anyone later "simplifies" the default to n * reconcileIntervalMs.

must-not-fire (does not kill a slow-but-completing sweep that runs for many intervals) passes before and after, as a control should. A pass that hydrates for 600ms against a 50ms interval — twelve intervals deep, far past the stall-report threshold — completes, dispatches, and records zero readinessReconcileErrors. It asserts completion from the sweep's own log line so the following 50ms pass cannot overwrite the evidence.

I verified the control has teeth rather than assuming it: flipping its reconcileTimeoutMs from 4_000 to 200 makes it fail with expected 2 to be +0 — the slow pass gets killed twice.

Review rounds

Four findings from Codex and cubic, every one valid, every one reproduced with a fail-first test before being fixed. The five original commits were squashed into one to rebase onto a main that had moved; the round-by-round detail lives here and in the answered threads. They are worth reading as a set: the deadline was the easy half, and correctly owning the work it abandons was the hard half.

#	Finding	Fail-first
1 (P1, both bots)	stop() awaited only the deadline wrapper, so shutdown released leases and disposed ports underneath a still-running sweep. The existing #stopping fence in #isPassFatalFailure does not cover it — that check is only reached once the sweep throws, and the dangerous case is the one where the dependency recovers and the sweep continues cleanly.	expected true to be false — stop() had already resolved with the sweep hung
2 (P2)	Clearing the in-flight timestamp when the wait ended discarded the evidence the work was still stuck, so a wedged Factory reported the milder retrying.	expected 'degraded' to be 'stalled'
3 (P2)	Keeping one record per abandoned wait grew without bound in exactly the never-settling case this PR is about.	expected length 1, received 3
4 (P1)	runOnce() does not coalesce a mismatched dryRun — it waits behind that sweep — so #runOnceInFlight at expiry could name an unrelated sweep, and the readiness pass then started its own work untracked after stop() believed it had drained everything. This falsified reasoning I had given on the round-three thread.	expected true to be false — shutdown completed with the readiness sweep still running

Finding 1 also had evidence I already held and under-read: my own fail-first tests leaked abandoned sweeps past teardown and caused two unrelated full-suite failures. I fixed that in the tests at the time instead of following it back to the product.

The settled design is two fields, not a collection. Every live abandoned wait is a runOnce() with the same dryRun, so whatever they are queued behind, the first one out starts the sweep and the rest coalesce onto it — they converge and settle together. The newest wait is therefore a sufficient drain target and the earliest start is the honest age, which satisfies findings 2, 3 and 4 at once. The tracked value is always the wait itself, never a global handle.

Verification

• npm test — 1837 passed, 1 skipped, 0 failed (baseline at this branch point: 1828, plus 9 new).
• npm run build — clean.
• An early full-suite run showed two unrelated-looking failures. I did not wave them off: baseline main was green, so I traced them to the leak described above, which turned out to be finding 1 in miniature.
• One CI package failure on the first commit was holds the terminal Slack receipt lease for as long as the provider write runs — a fake-timer spin test running in dispatch-owner mode, which never schedules a reconcile. Rather than calling it a flake on inference, I checked history: recent runs on main show the same profile, a different single test failing each time. Re-run passed, and every commit since has passed package first try.

Rebased onto #299 and #300

Both landed first and both touch this subsystem. The textual conflicts were the small part; the overlap with #300 was semantic.

#300 had already added a derived stalled state over the same health block. Two state machines describing one subsystem is the failure mode to avoid, so this drops its own derivation and its duplicate READINESS_RECONCILE_STALL_INTERVALS (mine 3, theirs 10 — same name, different value, different file) and keeps #300's. Theirs is the better one: shared module, guarded against throwing on the heartbeat path, and it works out-of-process from a heartbeat record, which is the whole point of #295.

What #300 could not see is contributed as an input rather than a rival output. Its readinessReconcileInFlightMs infers "in flight" from timestamp order — lastStartedAtMs newer than both settle timestamps. After a deadline expiry that inference is blind: the wait writes lastFailureAtMs while its runOnce() is still stuck, so order says "nothing in flight" during exactly the wedge #295 exists to expose. The daemon now publishes inFlightSinceMs — the earliest sweep still running, which it knows and a reader cannot infer — and readinessReconcileInFlightMs prefers it, falling back to the order inference for heartbeats that lack it. One representation, fed by exact data.

Two consequences I accepted rather than fought, and one position I changed:

• The stall threshold is now 10 intervals, not 3. feat(diagnostics): make a deployed Factory able to say why it is not dispatching (#295) #300's constant, with its own documented reasoning against fix(factory): replay existing Slack triage answers #36. Mine is deleted.
• stalled now outranks degraded. I had argued the opposite in this PR ("a stall must not silence an alarm keyed on degraded"), and feat(diagnostics): make a deployed Factory able to say why it is not dispatching (#295) #300's precedence is the merged contract. It is also the better call: stalled is the more specific diagnosis and consecutiveFailures ships alongside, so nothing an alarm needs is lost. My comment asserting the old precedence was falsified by the merge and is gone rather than left to mislead.
• One test changed with it: the must-fire now asserts consecutiveFailures >= failureThreshold (the claim that actually matters — the failure path was reached and counted) instead of the literal string degraded.

publishes an abandoned sweep to the deployed health projection as stalled is the guard for the seam. It asserts the discriminator directly: with only the three timestamps readinessReconcileInFlightMs returns undefined — it has lost the stuck sweep — while the published inFlightSinceMs finds it, and publicHealthFromHeartbeat over the real heartbeat file (what factory diagnose --deployed reads) reports stalled. The daemon's view and the operator's view reach the same conclusion.

Against #299, the bounded deadline and the new broker-rebind recovery are shown to coexist, in describe('against the #299 broker-rebind recovery'):

• A sweep whose roster call spends 600ms in reconnect-and-retry — twelve reconcile intervals — completes and dispatches, with readinessReconcileDeadlineExceeded at 0. fix(fleet): re-read connection.json and reconnect when the broker rebinds (#291) #299's retry is one-shot and read-only by construction ("repeated failures cannot turn into a reconnect loop"), so it cannot outrun a deadline sized for fix(factory): replay existing Slack triage answers #36's 61 minutes.
• An unreachable broker still fails the pass through the pre-existing error path with the deadline never firing, and the loop keeps sweeping. The deadline is the backstop for a hang, not a substitute for an error.

Full suite after the rebase: 1899 passed, 1 skipped, 0 failed — including #300's public-health and diagnose suites, unmodified.

Explicitly not attempted

Item 3 of the issue — auditing every provider/roster/mount call inside runOnce() for its own timeout — is left as follow-up. The deadline here is the backstop the issue describes it as, and bounding each individual dependency call carries the same crash-loop hazard as a too-small sweep deadline; it deserves its own change with its own worst-case measurements.

Do not merge or deploy on my account.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@khaliqgant, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 58 minutes

Limit 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.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 86cad8fa-22a6-4473-bcf9-eeba5e0e43ab

📥 Commits

Reviewing files that changed from the base of the PR and between 3e11bd5 and 4215a7c.

📒 Files selected for processing (6)

• docs/deployed-diagnostics.md
• src/config/schema.ts
• src/orchestrator/factory.test.ts
• src/orchestrator/factory.ts
• src/orchestrator/public-health.ts
• src/types.ts

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/296-bound-reconcile-sweep

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2b57adf19

ℹ️ 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".

[chatgpt-codex-connector]

Drain the abandoned sweep before completing shutdown

When a periodic pass reaches this deadline and stop() runs before the original runOnce() settles, #readinessReconcileInFlight already refers to the settled deadline wrapper, so shutdown does not await the still-live #runOnceInFlight. Shutdown can consequently clear state and dispose the fleet while this sweep remains active; if the blocked dependency then recovers, the sweep can resume dispatching through disposed ports or mutate state after stop() has resolved. Track the underlying abandoned sweep and either drain it during shutdown or fence its continuation once stopping begins.

Useful? React with 👍 / 👎.

[khaliqgant]

Valid, and fixed in e215355 — thank you, this was a real hole and I had already tripped over it without recognising it.

You are right that #readinessReconcileInFlight refers to the settled deadline wrapper, so shutdown was not awaiting the live #runOnceInFlight. I checked the partial fence before deciding: #isPassFatalFailure does treat #stopping as pass-fatal, but only once something in the sweep throws. That is exactly the case you describe — the blocked dependency recovers — so the sweep proceeds without error and never reaches that check, and can dispatch through disposed ports.

Corroboration that this is not theoretical: my fail-first tests leaked abandoned sweeps into subsequent tests and produced two unrelated failures in the full suite. I fixed that in the tests at the time; it was the same defect in the product, which I should have followed back then.

Took the drain option rather than the fence. #readinessReconcileAbandonedSweep is recorded when the deadline fires and awaited in stop(), right after #readinessReconcileInFlight. That restores exactly the pre-deadline shutdown contract — stop() outlives the sweep it started — rather than inventing a new one. It does mean a shutdown during a true hang waits as long as it did before this PR; that is the status quo for shutdown, and changing it is a separate decision from fixing the reconcile loop.

Fencing every continuation was the alternative I rejected: it means auditing every mutation reachable from runOnce() and is the same open-ended audit the issue explicitly defers as item 3.

New test: does not complete shutdown while a sweep abandoned by the deadline is still running. Fail-first was expected true to be false — stop() had already resolved with the sweep still hung.

[cubic-dev-ai]

All reported issues were addressed across 4 files

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>

[cubic-dev-ai]

All reported issues were addressed across 2 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic</sub>