Uh oh!
There was an error while loading. Please reload this page.
fix(factory): replay existing Slack triage answers - #36
Merged
Conversation
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR refactors the Slack triage escalation path in ChangesSlack Triage Escalation Reply Replay
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Jul 14, 2026
This was referenced Aug 20, 2026
Closed
khaliqgant added a commit
that referenced
this pull request
Aug 20, 2026
Review follow-ups on #300, from codex and cubic. P1 — a frozen snapshot could report green forever. The daemon stamps the health block at write time, so its `ageMs` is 0 and `stale` false *in the file*; a container still serving a heartbeat whose daemon has died kept answering "dispatching". The container recomputes liveness from `updatedAtMs` against its own clock on every request, so that verdict (HTTP status / `ok`) now outranks anything the block claims. Deliberately not recomputed against the reader's clock: skew would be reported as stall. P1 — the fleet control-plane circuit was missing entirely. An open circuit fails every spawn and resume fast, so it gates dispatch as hard as a failing sweep. Added as a third redacted subsystem: state, counters and `retryAtMs`, never its `lastError`, which names the broker socket path. P1 — a hung startup backfill was invisible. That pass recorded no timestamps, so the derived state had nothing to derive from and read `healthy` forever — and it is the pass most likely to hang (#36 measured 61 minutes there on a cold container). It now stamps start and settle, timestamps only; failure accounting stays with the reconcile loop that owns the threshold. P1 — an incomplete block is not a healthy one. "No degraded subsystem listed" on a block that never reported the readiness sweep is an absence of evidence; the verdict is now `cannot tell`. P2 — event-driven short-sleep mode answers `/healthz` at the Worker without probing the container, on purpose, so anonymous polling cannot defeat scale-to-zero. That response is Worker liveness; reading it as a dispatching Factory would be the exact false green this work exists to remove. P2 — a listener that is still `starting` has no subscription registered, so it is amber, not green. Only `subscribed`/`polling` count as dispatch-capable, and only on an instance whose readiness loop is actually running. P2 — `intervalMs: 0` made every in-flight pass instantly stalled and `missedPasses` Infinity (JSON `null`). A cadence must be positive. P2 — a finite number is not a valid date: `new Date(1e300).toISOString()` throws, and a renderer asked to explain an outage must never be the thing that throws. Timestamps outside the ECMA-262 range are dropped at the boundary, and the renderer guards too. P2 — C1 control characters (U+0080–U+009F) now stripped alongside C0 before remote text reaches a terminal. P2 — `deps.env` is threaded into the diagnose parser, so an injected environment's FACTORY_EVIDENCE_TOKEN is honoured. P3 — the documented `/healthz` example was not producible: `ageMs` is always 0 at write time and its epochs did not cohere against one `now`. Rewritten against a single reference instant. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532
khaliqgant added a commit
that referenced
this pull request
Aug 20, 2026
A container in `booting`, `rendering-config` or `preflight` answers `ok: false` and HTTP 503 exactly as a wedged one does, so the liveness short-circuit added in 73a2e3a told anyone diagnosing a starting instance that their Factory process was gone — sending them to the wrong problem, during the window when being sent to the wrong problem costs the most. The phase is in the response already. It now appears in the rendering, and a bootstrap phase gets its own verdict that says "not yet" and names the reason a cold start is slow (#36's 61-minute Relayfile mirror hydration) instead of "stale heartbeat or dead process". Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532
khaliqgant added a commit
that referenced
this pull request
Aug 20, 2026
…dispatching (#295) (#300) * feat(diagnostics): give a deployed Factory an operator-reachable answer (#295) During the 2026-08-19/20 outage the field naming the cause existed the whole time and was unreachable for ~10 hours. `/healthz` published subsystem state strings and returned `ok: true` while a dispatch-gating subsystem was degraded; `/evidence` carried `readinessReconcile.lastError` but is gated by a token minted per deploy and destroyed at the end of the run that mints it. Recovering the cause required minting a new Worker secret — a production write. Three changes: 1. A public health projection (`publicHealthFromHeartbeat`) that the daemon writes into the loop heartbeat as `health`, for the container to serve verbatim on `/healthz`. It carries `consecutiveFailures`, an allowlisted `lastErrorClass`, `intervalMs`, `lastStartedAtMs`/`lastCompletedAtMs` and a derived `inFlightMs`/`missedPasses`. `lastError` itself never crosses: the record is built by construction — closed enums and coerced numbers, nothing spread — and the class goes through the `telemetryErrorClass` allowlist that shipped in #293, now extracted to `src/observability/error-class.ts` and shared with the two places that had copied it. 2. A derived `stalled` state. `#scheduleReadinessReconcile` re-arms only inside `sweep.finally(...)`, and a hang takes neither the success nor the failure path — so a wedged pass leaves every settled field reading `healthy` forever. The relative order of `lastStarted` and `lastCompleted` is the only evidence, and `state` is now derived from it rather than last-write-wins. 3. `factory diagnose --deployed <url>` — the command a lane brief can name. It needs no credential, answers "is this instance dispatching, and if not why" in one line, and exits non-zero when the answer is no. `--token` (or `FACTORY_EVIDENCE_TOKEN`) additionally reads the gated `/evidence` message. `ok` deliberately stays a liveness bit: `/healthz` is the Cloudflare Container ping endpoint, so a non-200 recycles the container — destroying the evidence and restarting the cold-start hydration #36 measured at 61 minutes. The amber lives in `status`/`degradedSubsystems`, which no platform interprets and a monitor can alert on. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnose): read the health block where the container serves it (#295) factory-cloud's `/healthz` projects the daemon heartbeat through `publicHeartbeat()`, so the block lands at `heartbeat.health`, not at the document root. Read both: a proxy that hoists it to the top level still works, and the delivered command sees the real deployed shape. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnostics): close the false-green gaps found in review (#295) Review follow-ups on #300, from codex and cubic. P1 — a frozen snapshot could report green forever. The daemon stamps the health block at write time, so its `ageMs` is 0 and `stale` false *in the file*; a container still serving a heartbeat whose daemon has died kept answering "dispatching". The container recomputes liveness from `updatedAtMs` against its own clock on every request, so that verdict (HTTP status / `ok`) now outranks anything the block claims. Deliberately not recomputed against the reader's clock: skew would be reported as stall. P1 — the fleet control-plane circuit was missing entirely. An open circuit fails every spawn and resume fast, so it gates dispatch as hard as a failing sweep. Added as a third redacted subsystem: state, counters and `retryAtMs`, never its `lastError`, which names the broker socket path. P1 — a hung startup backfill was invisible. That pass recorded no timestamps, so the derived state had nothing to derive from and read `healthy` forever — and it is the pass most likely to hang (#36 measured 61 minutes there on a cold container). It now stamps start and settle, timestamps only; failure accounting stays with the reconcile loop that owns the threshold. P1 — an incomplete block is not a healthy one. "No degraded subsystem listed" on a block that never reported the readiness sweep is an absence of evidence; the verdict is now `cannot tell`. P2 — event-driven short-sleep mode answers `/healthz` at the Worker without probing the container, on purpose, so anonymous polling cannot defeat scale-to-zero. That response is Worker liveness; reading it as a dispatching Factory would be the exact false green this work exists to remove. P2 — a listener that is still `starting` has no subscription registered, so it is amber, not green. Only `subscribed`/`polling` count as dispatch-capable, and only on an instance whose readiness loop is actually running. P2 — `intervalMs: 0` made every in-flight pass instantly stalled and `missedPasses` Infinity (JSON `null`). A cadence must be positive. P2 — a finite number is not a valid date: `new Date(1e300).toISOString()` throws, and a renderer asked to explain an outage must never be the thing that throws. Timestamps outside the ECMA-262 range are dropped at the boundary, and the renderer guards too. P2 — C1 control characters (U+0080–U+009F) now stripped alongside C0 before remote text reaches a terminal. P2 — `deps.env` is threaded into the diagnose parser, so an injected environment's FACTORY_EVIDENCE_TOKEN is honoured. P3 — the documented `/healthz` example was not producible: `ageMs` is always 0 at write time and its epochs did not cohere against one `now`. Rewritten against a single reference instant. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnose): tell a booting instance apart from a wedged one (#295) A container in `booting`, `rendering-config` or `preflight` answers `ok: false` and HTTP 503 exactly as a wedged one does, so the liveness short-circuit added in 73a2e3a told anyone diagnosing a starting instance that their Factory process was gone — sending them to the wrong problem, during the window when being sent to the wrong problem costs the most. The phase is in the response already. It now appears in the rendering, and a bootstrap phase gets its own verdict that says "not yet" and names the reason a cold start is slow (#36's 61-minute Relayfile mirror hydration) instead of "stale heartbeat or dead process". Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * docs: say what the frozen ageMs/stale actually are (#295) Review follow-up on #300 (P3, cubic). "Were true at write time" reads as though these were measurements that went out of date. They are constants of the write — `ageMs` is always 0 and `stale` always false in the file, at any age — which is the whole reason freshness has to come from `updatedAtMs` against the serving process's clock. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnose): stop blaming the instance version for an unprobed response (#295) Found by running the built CLI against a short-sleep stub rather than a unit test. Two rendering defects, both of the send-the-operator-to-the-wrong-problem kind this command exists to prevent: - A response the Worker answered without probing the container was rendered as "instance predates #295", inviting an upgrade of a Factory that is fine. The absence of the block says nothing about the instance's version when nobody asked the instance. It now names the mode instead, and the same distinction applies to the verdict for any response carrying no state strings at all — including a container still booting. - `phase` printed twice once the liveness work started rendering it. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnose): say unknown, not undefined, for a missing legacy state (#295) Review follow-up on #300 (P3, cubic), and a regression from my own refactor in 8cbbde3: gating the "predates #295" verdict on either state string being present meant the other could reach the template as undefined and render as the literal "undefined". A diagnostic that prints "undefined" at an operator during an outage is worse than one that admits it does not know. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnostics): stop the diagnostic from asserting more than it knows (#295) CodeRabbit's first real pass on this PR (its earlier green checks were rate limits, not reviews). Nine findings, all the same family as the issue itself. The one that matters most: the health projection ran unguarded on the heartbeat write path, and `status()` called the derivations directly. A throw in either would fail every heartbeat write — which is precisely what the crash reaper and /healthz read to decide the daemon is alive. The diagnostic would have caused the outage it exists to explain. Both are now guarded: a projection failure costs the diagnostics block and nothing else, and is logged. The omitted block is itself legible, because `factory diagnose` reports a missing one rather than a false green. The rest: - `live: false` for ANY non-200 let a gateway 404, an auth proxy 401 or a load balancer 502 produce "the instance reports itself not live... the Factory process is gone" about a container that was never asked. Only a 503 or an explicit `ok: false` is the instance speaking; anything else is now "cannot tell: the endpoint answered HTTP n and carried no Factory health". - A `status` the block did not report read as `unknown` and printed "a subsystem is degraded". Not knowing is not the same statement as knowing something is wrong. - Every non-200 from /evidence blamed the token. A 404 means the deployment has no /evidence route and a 5xx means the endpoint failed; both sent an operator to rotate a credential that works. - `factory diagnose <url> <token>` is a plausible slip, and the unknown-argument error echoed the value into stderr and from there into CI logs. It now names the position. - The CLI tests omitted a hermetic `env`, so an ambient FACTORY_EVIDENCE_TOKEN made the command also request /evidence and fail the suite. Verified: with the token exported, the old tests fail and the new ones pass. - The reader accepted `intervalMs: 0` and negative durations from a remote process, undoing the invariant the writer enforces two functions away. - The docs claimed every field is "a closed enum or a coerced number", which is not true of the booleans, the array or the bounded text; and two fences had no language identifier. Declined: replacing the `telemetryErrorClass` pattern with a finite list of known class names. Reasoning in the thread — it is the #293 allowlist shared by every telemetry surface, a finite list would collapse our own dispatch error classes to `Error` and destroy the signal #295 asks for, and the pattern already excludes paths, URLs, tokens and whitespace. If the residual channel is a concern it should be fixed at the source for all surfaces, not forked here. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532 * fix(diagnose): close the gaps left by my own last round (#295) Review follow-up on #300 (P2 x3, cubic). Two of these are incomplete fixes I landed in ec4cb33. - The no-echo protection covered the second-positional and unknown-option cases but not the url slot, which is exactly where a mistyped `--token` argument lands: `factory diagnose <token>` and `--deployed <token>` both reached the scheme check, which printed the value to stderr and from there to CI logs. The message now states the requirement without the value, and points at --token. - Treating any 200 or 503 as "the instance answered" let a gateway error page or a load balancer's own 503 be read as the container speaking. The container's health response always carries a top-level `ok`; that boolean, not the status code, is now what distinguishes the instance from whatever else can answer on that URL. - `missedPasses` accepted a fractional value from a remote record, so a report could say "1.5 missed passes". It is a count of whole passes; `inFlightMs` stays fractional because a duration genuinely is. Refs #295 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 11854cb0-33de-4b9d-8ba3-2d34da892532
khaliqgant added a commit
that referenced
this pull request
Aug 20, 2026
…rk it abandons (#296) A `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, and both state-writing paths run on settle — success sets `lastCompletedAtMs`, failure increments `consecutiveFailures` — so a hang took neither. Production reported `state=healthy, consecutiveFailures=0, lastError=none` for 104 minutes while dispatching nothing. Only a process restart recovered it. Give the sweep a deadline. Expiry rejects, which routes it into the existing failure path that already increments the counter, records `lastError`, marks `degraded` past the threshold, and re-arms the loop. No parallel recovery machinery: the existing one was simply unreachable from a hang. The deadline bounds the wait, not the sweep — `runOnce()` owns a durable discovery lease and abandoning it mid-flight is not safe — so the abandoned work stays live and has to be owned. `stop()` drains it, so shutdown still outlives the sweep it started, and the in-flight age counts from when that work actually began rather than restarting on every expiry. The tracked value is the wait itself, never `#runOnceInFlight`: a mismatched-`dryRun` sweep is waited BEHIND rather than coalesced onto, so that handle can name unrelated work. Retention needs no collection — every live abandoned wait shares one `dryRun`, so they converge on one sweep and settle together. The deadline defaults to 90 minutes, NOT a small multiple of the 60s interval: #36 measured a real cold-mirror reconcile at 3,665,173 ms (61 minutes) because container disk is ephemeral and the Relayfile mirror rehydrates on every boot. A deadline under realistic worst-case hydration would convert a slow boot into a crash loop, which is worse than the bug. Rebased onto #299 and #300, which landed first and both touch this subsystem. The overlap with #300 was semantic, not textual: it had already added a derived `stalled` state over the same health block. Rather than ship two state machines, this drops its own derivation and its duplicate STALL_INTERVALS constant and keeps #300's, which is the better one — it is shared, guarded, and works out-of-process from a heartbeat. What #300 could not see is contributed as an input instead: after a deadline expiry the wait writes a settle timestamp while its sweep is still stuck, so timestamp order alone reports "nothing in flight". The daemon publishes `inFlightSinceMs`, and `readinessReconcileInFlightMs` prefers it, falling back to the order inference for heartbeats without it. One representation, fed by exact data. Against #299, the bounded deadline and the new broker-rebind recovery are shown to coexist: a sweep that recovers from a rebind completes instead of being killed, and an unreachable broker still fails the pass through the pre-existing error path without the deadline firing at all. Squashed from five commits: the original fix plus four rounds of review findings from Codex and cubic, each reproduced with a fail-first test before being fixed. PR #301 carries the round-by-round detail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: f3e68b71-b0ba-4ace-8b08-35fb74bc03ad
khaliqgant added a commit
that referenced
this pull request
Aug 20, 2026
…ot stop the loop (#296) (#301) * fix(orchestrator): bound the readiness reconcile sweep and own the work it abandons (#296) A `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, and both state-writing paths run on settle — success sets `lastCompletedAtMs`, failure increments `consecutiveFailures` — so a hang took neither. Production reported `state=healthy, consecutiveFailures=0, lastError=none` for 104 minutes while dispatching nothing. Only a process restart recovered it. Give the sweep a deadline. Expiry rejects, which routes it into the existing failure path that already increments the counter, records `lastError`, marks `degraded` past the threshold, and re-arms the loop. No parallel recovery machinery: the existing one was simply unreachable from a hang. The deadline bounds the wait, not the sweep — `runOnce()` owns a durable discovery lease and abandoning it mid-flight is not safe — so the abandoned work stays live and has to be owned. `stop()` drains it, so shutdown still outlives the sweep it started, and the in-flight age counts from when that work actually began rather than restarting on every expiry. The tracked value is the wait itself, never `#runOnceInFlight`: a mismatched-`dryRun` sweep is waited BEHIND rather than coalesced onto, so that handle can name unrelated work. Retention needs no collection — every live abandoned wait shares one `dryRun`, so they converge on one sweep and settle together. The deadline defaults to 90 minutes, NOT a small multiple of the 60s interval: #36 measured a real cold-mirror reconcile at 3,665,173 ms (61 minutes) because container disk is ephemeral and the Relayfile mirror rehydrates on every boot. A deadline under realistic worst-case hydration would convert a slow boot into a crash loop, which is worse than the bug. Rebased onto #299 and #300, which landed first and both touch this subsystem. The overlap with #300 was semantic, not textual: it had already added a derived `stalled` state over the same health block. Rather than ship two state machines, this drops its own derivation and its duplicate STALL_INTERVALS constant and keeps #300's, which is the better one — it is shared, guarded, and works out-of-process from a heartbeat. What #300 could not see is contributed as an input instead: after a deadline expiry the wait writes a settle timestamp while its sweep is still stuck, so timestamp order alone reports "nothing in flight". The daemon publishes `inFlightSinceMs`, and `readinessReconcileInFlightMs` prefers it, falling back to the order inference for heartbeats without it. One representation, fed by exact data. Against #299, the bounded deadline and the new broker-rebind recovery are shown to coexist: a sweep that recovers from a rebind completes instead of being killed, and an unreachable broker still fails the pass through the pre-existing error path without the deadline firing at all. Squashed from five commits: the original fix plus four rounds of review findings from Codex and cubic, each reproduced with a fail-first test before being fixed. PR #301 carries the round-by-round detail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: f3e68b71-b0ba-4ace-8b08-35fb74bc03ad * docs(diagnostics): document the bounded sweep and inFlightSinceMs (#296) The deployed-diagnostics guide from #300 tells an operator that `lastStarted > lastCompleted` is the ONLY evidence a pass is in flight. After #296 that is outdated, and in one case actively wrong: once a wait ends on its deadline it records a failure while the sweep underneath it keeps running, so timestamp order reports nothing in flight during exactly the wedge this page exists to diagnose. Document `inFlightSinceMs` as the authoritative field, with the order comparison as the fallback for heartbeats that predate it. Also document how long a stall can last, since the page now describes a state that is bounded: the wait fails at 90 minutes and the loop re-arms, while the sweep keeps ageing because it holds a durable discovery lease. A `stalled` that never produces a rising `consecutiveFailures` means the loop is not running at all — a restart, not a wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: f3e68b71-b0ba-4ace-8b08-35fb74bc03ad
khaliqgant added a commit
that referenced
this pull request
Aug 23, 2026
…dge reconcile (#351) `readinessReconcile` started a cycle at 20:29:40Z on the live container and had not finished it 22 minutes later. Heartbeat still ticking, fleet agent online, `fleetControlPlane: closed`, and `consecutiveFailures: 0` — it was not erroring, it was blocked inside a call, and a failure counter cannot see a hang. Every other dependency boundary in the cycle is already bounded: the fleet roster probe at 5s through `FleetControlPlaneCircuit`, GitHub REST reads at 30s via `AbortSignal.timeout`, spawn and resume by the spawn-ack deadline. Relayfile was not. `@relayfile/sdk`'s `performRequest` attaches a signal to its `fetch` only when the caller supplies one, and `RelayfileCloudMountClient` never did — so every `readFile`, `listTree` and `ensureSubRoot` was a bare `fetch()` that could wait forever. `listTree` also walked an unbounded cursor loop, and `ensureSubRoot` accepted a `timeoutMs` and discarded it, so `#ensureGithubIngestionReady` passing 90_000 bounded nothing. The #296 sweep deadline could not cover this. It is 90 minutes by design — below realistic cold-mirror hydration a slow boot becomes a crash loop — and it rejects only the *wait*: `runOnce()` keeps its discovery lease, so the next cycle coalesces onto the same wedged promise. Nothing but a restart recovered. This bounds the call instead: - `RelayfileCloudMountClient` derives a deadline per operation and passes its signal to the SDK, so the request is cancelled rather than abandoned — an abandoned wait leaves the socket and the SDK's retry loop live and hands the next cycle the same in-flight read. One deadline covers the whole `listTree` walk, and `ensureSubRoot` honours the `timeoutMs` it used to drop. - `#withRelayfileOperation` races the same budget as a backstop for mounts that cannot honour a signal, and throws `RelayfileOperationTimeoutError` naming the operation and phase. That reaches the existing failure path: `consecutiveFailures` rises, `lastError` says what it was waiting on, and `lastErrorClass` publishes through the existing allowlist. - A timeout now escapes the swallowing catches alongside a relayfile 429 (#297). Without that, `#githubIssuePaths` folded it into an empty result and a wedged dependency became a *successful* sweep that discovered zero issues — the same silence wearing a different costume. - Failing the call unwinds the pass, which releases the discovery lease, so the next cycle starts clean instead of joining the hang. `liveSubscription.relayfileOperationTimeoutMs` defaults to 5 minutes, two orders of magnitude below the sweep deadline. That is safe precisely where the sweep deadline is not: #36's 61-minute cold-mirror reconcile is spread across thousands of calls, so a per-call bound does not re-create the crash-loop risk. Tests: a reconcile whose `listTree` never resolves aborts, counts, names the call, starts the next cycle, and recovers with no restart — with a control that moves the bound out of reach and asserts the loop then freezes, so the test cannot pass for another reason. Verified by ablation: removing the bound fails the test at the counter assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
khaliqgant added a commit
that referenced
this pull request
Aug 23, 2026
…dge reconcile (#354) * fix(orchestrator): bound each relayfile call so a hung read cannot wedge reconcile (#351) `readinessReconcile` started a cycle at 20:29:40Z on the live container and had not finished it 22 minutes later. Heartbeat still ticking, fleet agent online, `fleetControlPlane: closed`, and `consecutiveFailures: 0` — it was not erroring, it was blocked inside a call, and a failure counter cannot see a hang. Every other dependency boundary in the cycle is already bounded: the fleet roster probe at 5s through `FleetControlPlaneCircuit`, GitHub REST reads at 30s via `AbortSignal.timeout`, spawn and resume by the spawn-ack deadline. Relayfile was not. `@relayfile/sdk`'s `performRequest` attaches a signal to its `fetch` only when the caller supplies one, and `RelayfileCloudMountClient` never did — so every `readFile`, `listTree` and `ensureSubRoot` was a bare `fetch()` that could wait forever. `listTree` also walked an unbounded cursor loop, and `ensureSubRoot` accepted a `timeoutMs` and discarded it, so `#ensureGithubIngestionReady` passing 90_000 bounded nothing. The #296 sweep deadline could not cover this. It is 90 minutes by design — below realistic cold-mirror hydration a slow boot becomes a crash loop — and it rejects only the *wait*: `runOnce()` keeps its discovery lease, so the next cycle coalesces onto the same wedged promise. Nothing but a restart recovered. This bounds the call instead: - `RelayfileCloudMountClient` derives a deadline per operation and passes its signal to the SDK, so the request is cancelled rather than abandoned — an abandoned wait leaves the socket and the SDK's retry loop live and hands the next cycle the same in-flight read. One deadline covers the whole `listTree` walk, and `ensureSubRoot` honours the `timeoutMs` it used to drop. - `#withRelayfileOperation` races the same budget as a backstop for mounts that cannot honour a signal, and throws `RelayfileOperationTimeoutError` naming the operation and phase. That reaches the existing failure path: `consecutiveFailures` rises, `lastError` says what it was waiting on, and `lastErrorClass` publishes through the existing allowlist. - A timeout now escapes the swallowing catches alongside a relayfile 429 (#297). Without that, `#githubIssuePaths` folded it into an empty result and a wedged dependency became a *successful* sweep that discovered zero issues — the same silence wearing a different costume. - Failing the call unwinds the pass, which releases the discovery lease, so the next cycle starts clean instead of joining the hang. `liveSubscription.relayfileOperationTimeoutMs` defaults to 5 minutes, two orders of magnitude below the sweep deadline. That is safe precisely where the sweep deadline is not: #36's 61-minute cold-mirror reconcile is spread across thousands of calls, so a per-call bound does not re-create the crash-loop risk. Tests: a reconcile whose `listTree` never resolves aborts, counts, names the call, starts the next cycle, and recovers with no restart — with a control that moves the bound out of reach and asserts the loop then freezes, so the test cannot pass for another reason. Verified by ablation: removing the bound fails the test at the counter assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(mount): cap ensureSubRoot at the tighter budget and name the phase on transport timeouts Two P2s from codex on #354, both real. `ensureSubRoot`'s explicit `timeoutMs` replaced the client-wide budget instead of capping it. With `relayfileOperationTimeoutMs` configured below 72s, the reconcile caller's hard-coded 90_000 left the transport running past the orchestrator's 1.25x backstop — so the backstop abandoned the wait rather than the transport cancelling the call, which is precisely the behaviour this change exists to avoid. It now takes the tighter of the two. The transport deadline is designed to win the race, and the mount does not know which phase it was serving, so `lastError` read `relayfile listTree did not respond within 300000ms` with no way to tell one of many list/read contexts from another. The orchestrator now enriches a phase-less transport timeout with the phase it knows as the error crosses its boundary, keeping the original as `cause`. That restores the message the PR and the diagnostics doc describe. Both covered, and both verified by ablation: reverting the cap times the ensureSubRoot test out, and reverting the enrichment fails the new orchestrator test on the phase assertion specifically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
khaliqgant added a commit
that referenced
this pull request
Aug 25, 2026
Three unbounded calls have wedged this sweep in a single day, on three different transports. Each was real, each was bounded, and each time the wedge came back one layer down: the FACTORY_STATE Durable Object calls (factory-cloud#78), the relayfile change-feed tail reads (#368, shipped and verified in 0.1.75), then the retry of the now-bounded call. This does not bound a fourth. It makes the class of failure survivable. The property it establishes: NO SWEEP CAN BE IN FLIGHT FOR LONGER THAN ITS BUDGET, whatever it is waiting on. Elapsed time is charged against ONE timer for the whole pass, so it does not matter which await is slow, how many there are, or how many times the sweep retries one of them. The next unbounded call degrades a sweep instead of ending dispatch. WHY A PER-CALL BOUND CANNOT DO THIS. `relayfileOperationTimeoutMs` bounds one relayfile call and cannot see the retry loop around it or a call on another transport. `reconcileTimeoutMs` bounds the CALLER'S WAIT from outside `runOnce()`, so expiry leaves the sweep running and every later cycle coalesces onto the same wedged promise (factory.ts `runOnce()`, the `#runOnceInFlight` branch) — which is why the deployed daemon never recovers. The budget expires from INSIDE `#runOnceWithDiscoveryFence`, so the sweep unwinds, the lease goes back, `#runOnceInFlight` clears, and the next cycle claims a fresh lease. MECHANISM, PLAINLY. `budget.run()` is a race, not a cancellation — the same limitation #368 documented, stated for the same reason. CAN: abandon an in-flight await, from any transport, and unwind the sweep. CANNOT: stop the abandoned work. The socket stays open, the SDK's own retry loop keeps running, and a side effect already in flight still lands. PARTIAL: `budget.signal` aborts at expiry, so anything honouring an AbortSignal is really cancelled — nothing in the sweep consumes it yet (the relayfile client mints its own per-call signal and that file is owned by another lane this week); it is exported so wiring it is one line. `assertNotExpired()` is a between-await check and is worth nothing against a call that never returns, but it does make an abandoned pass unwind at its next loop iteration rather than run to completion beside its replacement. TEARDOWN IS BOUNDED SEPARATELY. On the path that matters the budget is spent by construction, so teardown cannot run under it or the lease would never be released — and releasing it is the half that makes the next cycle clean. An unbounded release would re-create this wedge one layer down. It gets a 30 s deadline; an abandoned release costs an orphaned lease for one expiry window, which a later sweep reclaims (`claim.reclaimedLease`). DEFAULT IS THE EXISTING ENVELOPE, DELIBERATELY. `sweepBudgetMs` defaults to `reconcileTimeoutMs` (90 min) and is clamped to it, so no sweep that survives today is killed by this. The value is a policy dial, the mechanism is the fix. Tightening it has a real cost: the checkpoint commits only at the end, so a budget below realistic cold-mirror hydration (#36 measured 61 min in production) makes a slow boot a loop that never progresses. TESTS (11), must-fire/must-not-fire for each: - must-fire, end to end: a sweep whose first post-claim call never returns is aborted at its budget naming the phase, the lease release is OBSERVED on the store, and the next cycle runs a fresh sweep and dispatches. Fail-first verified by mechanism: with only factory.ts reverted it fails after 4038 ms with "sweep never settled" — the pass never settles, exactly as production. - must-fire, primitive: three 40 ms calls under a 120 ms budget — the third is rejected because the SWEEP is out of time, not because it is slow; a bounded-but-always-failing call inside an unbounded retry loop ends at the budget (the L3 shape) after more than one attempt; the signal aborts; a spent budget refuses to start new work against the dependency it gave up on. - must-not-fire: a healthy sweep under a snug 30 s budget produces results IDENTICAL to an unbounded control (pulled, dispatched, skipped, spawns) — without this the trivial wrong fix, abort everything, passes; a caller's own failure still surfaces as itself and is never re-clothed as a budget expiry; with `sweepBudgetMs: 0` the same hung call stays pending, so every rejection above is attributable to the budget and not to the wrapper. WHAT THIS DOES NOT COVER. - It does not make anything faster or find the hanging call. A wedged dependency still costs one whole budget per cycle. - It does not cancel. See MECHANISM above. - The abandoned pass runs concurrently with the sweep that replaces it if it ever unsticks. It cannot commit a checkpoint (the store's epoch guard) but its in-flight side effects still land. - Two `stop()`/shutdown paths and the `#runOnceWithReadinessDeadline` abandoned-wait bookkeeping are unchanged; a budget expiry reaches them as an ordinary sweep failure. - The default changes no timing. Recovery inside 90 minutes needs either a tighter `sweepBudgetMs` or the L3 retry bound the other lane owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2 Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2
kjgbot pushed a commit
that referenced
this pull request
Aug 25, 2026
…#374) * fix(orchestrator): bound the whole sweep, not one more call inside it Three unbounded calls have wedged this sweep in a single day, on three different transports. Each was real, each was bounded, and each time the wedge came back one layer down: the FACTORY_STATE Durable Object calls (factory-cloud#78), the relayfile change-feed tail reads (#368, shipped and verified in 0.1.75), then the retry of the now-bounded call. This does not bound a fourth. It makes the class of failure survivable. The property it establishes: NO SWEEP CAN BE IN FLIGHT FOR LONGER THAN ITS BUDGET, whatever it is waiting on. Elapsed time is charged against ONE timer for the whole pass, so it does not matter which await is slow, how many there are, or how many times the sweep retries one of them. The next unbounded call degrades a sweep instead of ending dispatch. WHY A PER-CALL BOUND CANNOT DO THIS. `relayfileOperationTimeoutMs` bounds one relayfile call and cannot see the retry loop around it or a call on another transport. `reconcileTimeoutMs` bounds the CALLER'S WAIT from outside `runOnce()`, so expiry leaves the sweep running and every later cycle coalesces onto the same wedged promise (factory.ts `runOnce()`, the `#runOnceInFlight` branch) — which is why the deployed daemon never recovers. The budget expires from INSIDE `#runOnceWithDiscoveryFence`, so the sweep unwinds, the lease goes back, `#runOnceInFlight` clears, and the next cycle claims a fresh lease. MECHANISM, PLAINLY. `budget.run()` is a race, not a cancellation — the same limitation #368 documented, stated for the same reason. CAN: abandon an in-flight await, from any transport, and unwind the sweep. CANNOT: stop the abandoned work. The socket stays open, the SDK's own retry loop keeps running, and a side effect already in flight still lands. PARTIAL: `budget.signal` aborts at expiry, so anything honouring an AbortSignal is really cancelled — nothing in the sweep consumes it yet (the relayfile client mints its own per-call signal and that file is owned by another lane this week); it is exported so wiring it is one line. `assertNotExpired()` is a between-await check and is worth nothing against a call that never returns, but it does make an abandoned pass unwind at its next loop iteration rather than run to completion beside its replacement. TEARDOWN IS BOUNDED SEPARATELY. On the path that matters the budget is spent by construction, so teardown cannot run under it or the lease would never be released — and releasing it is the half that makes the next cycle clean. An unbounded release would re-create this wedge one layer down. It gets a 30 s deadline; an abandoned release costs an orphaned lease for one expiry window, which a later sweep reclaims (`claim.reclaimedLease`). DEFAULT IS THE EXISTING ENVELOPE, DELIBERATELY. `sweepBudgetMs` defaults to `reconcileTimeoutMs` (90 min) and is clamped to it, so no sweep that survives today is killed by this. The value is a policy dial, the mechanism is the fix. Tightening it has a real cost: the checkpoint commits only at the end, so a budget below realistic cold-mirror hydration (#36 measured 61 min in production) makes a slow boot a loop that never progresses. TESTS (11), must-fire/must-not-fire for each: - must-fire, end to end: a sweep whose first post-claim call never returns is aborted at its budget naming the phase, the lease release is OBSERVED on the store, and the next cycle runs a fresh sweep and dispatches. Fail-first verified by mechanism: with only factory.ts reverted it fails after 4038 ms with "sweep never settled" — the pass never settles, exactly as production. - must-fire, primitive: three 40 ms calls under a 120 ms budget — the third is rejected because the SWEEP is out of time, not because it is slow; a bounded-but-always-failing call inside an unbounded retry loop ends at the budget (the L3 shape) after more than one attempt; the signal aborts; a spent budget refuses to start new work against the dependency it gave up on. - must-not-fire: a healthy sweep under a snug 30 s budget produces results IDENTICAL to an unbounded control (pulled, dispatched, skipped, spawns) — without this the trivial wrong fix, abort everything, passes; a caller's own failure still surfaces as itself and is never re-clothed as a budget expiry; with `sweepBudgetMs: 0` the same hung call stays pending, so every rejection above is attributable to the budget and not to the wrapper. WHAT THIS DOES NOT COVER. - It does not make anything faster or find the hanging call. A wedged dependency still costs one whole budget per cycle. - It does not cancel. See MECHANISM above. - The abandoned pass runs concurrently with the sweep that replaces it if it ever unsticks. It cannot commit a checkpoint (the store's epoch guard) but its in-flight side effects still land. - Two `stop()`/shutdown paths and the `#runOnceWithReadinessDeadline` abandoned-wait bookkeeping are unchanged; a budget expiry reaches them as an ordinary sweep failure. - The default changes no timing. Recovery inside 90 minutes needs either a tighter `sweepBudgetMs` or the L3 retry bound the other lane owns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2 Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2 * test(orchestrator): point the abandoned-wait suite at the disabled-budget backstop The seven `bounded readiness reconciliation` cases assert on a sweep that the readiness deadline abandoned and that is STILL RUNNING. The aggregate budget makes that state unreachable at its default — it aborts the sweep at or before that deadline, so there is nothing left in flight to observe. That is the fix, not a regression. Each now passes `sweepBudgetMs: 0`, which selects the pre-#372 backstop those assertions are actually about: the #296/#301 abandoned-wait accounting, still the behaviour when the budget is disabled and still the shape a sweep degrades to if a teardown path cannot be abandoned. `0` as the disable value is the same control idiom #368 used for `operationTimeoutMs`. Adds the positive counterpart, which the redirected cases can no longer state: a live daemon whose first post-claim call never returns still completes `start()` and `stop()`, because the sweep is aborted rather than abandoned. Fail-first verified by mechanism against `origin/main`'s factory.ts: it fails after 4044 ms with `start never returned`. That is deliverable B demonstrated rather than argued — `#deferLiveEventDrain = false` sits in a `finally` around that unbounded `runOnce()` (main factory.ts:1983/2021/2039), so a wedged startup backfill also kills the live-event dispatch path, which is why a hung sweep meant zero dispatch instead of stale dispatch. Also documents a gap the shutdown test exposed and this PR does NOT close: `#startLiveSubscription` reads the event high-watermark before the backfill and outside any sweep, so that read is bounded only by the per-call relayfile deadline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2 * fix(orchestrator): answer the review — derive the default, fence the abandoned pass Five findings, all valid at 5c656f9, each with its own must-fire/must-not-fire. 1. THE DANGEROUS ONE. A fixed 90-minute `sweepBudgetMs` default is ABOVE any config that had already tightened `reconcileTimeoutMs`, so the cross-field check rejected it and `FactoryConfigSchema.parse` threw — Factory would not have started. It also silently capped a config that loosened the timeout above 90 minutes. The omitted budget is now derived from its SIBLING in a `.transform()`, never from a constant, and `resolvedSweepBudgetMs` is the one rule the schema and the orchestrator's `start()` clamp both use. 2. An abandoned `#performRunOnce` could write a stale tree listing into the REPLACEMENT sweep's checkpoint: `#rememberDiscoveryTree` reads the shared `#discoverySession` fresh, and by the time a late continuation resolves that is the next sweep's. `#isStaleDiscoveryContinuation()` compares the `discoveryEnumerationPass` epoch — an AsyncLocalStorage store, so it follows the async continuation and still carries the epoch that ISSUED the read — against the live one. It is the same fence the tree-read counters already used. Applied to the checkpoint write and to overload attribution, so a 429 that arrives after its sweep was abandoned cannot drive the replacement's ratchet. 3. The dispatch loop gets the same budget guard as the read loop, so a pass abandoned during enumeration cannot dispatch after its lease went back. 4. A lease claimed after the budget gave up on the claim was stranded: nobody would renew, commit or release it, so every later sweep deferred for a whole lease window. A compensating release is now attached to the abandoned claim. Fail-first verified by mechanism — with the compensation ablated the test fails with `stranded lease was never released`. 5. Unref'd deadline timers let Node exit before the budget fires. Under a one-shot `runOnce()` whose only pending work is a promise nothing else references, the command would return without reporting the wedge or releasing the lease. Both deadline timers are referenced now; they live for at most one budget and `dispose()` clears them from a `finally`. Also, on the review's reading of a comment: the pre-backfill watermark read is bounded neither by the sweep budget NOR by anything in the orchestrator — `#currentEventHighWatermark` (factory.ts:2192) awaits the mount directly under a bare try/catch. What bounds it in production is one layer lower, the deployed client's own `#bounded()` (relayfile-cloud-mount-client.ts:1057, #368). The comment now says which layer, because a `MountClient` without that deadline has no bound here at all. And the e2e must-fire no longer risks blaming a phase string for a timing stall: the budget has 400 ms of headroom over two in-memory calls, and `hungCalls` is asserted before the phase so a mis-timed run names the real cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2 * fix(orchestrator): give shutdown a lever on the sweep budget Two findings at 2c2dd86, both this PR's own lesson recurring inside this PR. 1. THE BOUND BECAME THE WEDGE. `stop()` deliberately outlives the sweep it started (#301, the `#readinessReconcileAbandonedWait` drain), so a wedged sweep makes shutdown exactly as long as the sweep budget — 90 minutes at the default. Referencing the timer did not create that (before this PR the drain was unbounded, so shutdown was unbounded too), but it is the same trap the teardown deadline already answers one layer down, and an operator restarting a wedged container is the person who pays. NOT fixed by `unref()`. That is the trivially wrong version: it also lets Node exit before the budget fires, so a one-shot `runOnce()` returns having neither reported the wedge nor released the lease — the P2 that made the timer referenced in the first place. The two asks are in tension and only a shutdown-specific path satisfies both. `stop()` now arms a grace timer over the drain; after `STOP_TEARDOWN_TIMEOUT_MS` it calls `budget.expire()` on every in-flight sweep, routing them into the ordinary abort path — lease released, teardown bounded — instead of holding the process. The grace is what keeps an ordinary restart from discarding a sweep that was about to commit. A sweep that starts while `#stopping` is already set is expired immediately, so it cannot hand shutdown a fresh 90-minute budget. 2. The lease claim is now issued INSIDE the budget callback, so a spent budget rejects the phase without opening a lease it could only hand straight back. A lease taken after expiry makes every later sweep defer — the same "later cycles wait on a pass that is already over" failure this PR's own comparison names in `reconcileTimeoutMs`. Pairs, and their fail-first, verified by ablation: - must-fire: a live daemon whose PERIODIC sweep wedges under a 60 s budget still completes `stop()` inside 4 s. With the grace ablated it hangs to the vitest timeout — shutdown waiting out the budget, which is the defect. - must-not-fire: the budget timer appears in `process.getActiveResourcesInfo()` while a sweep runs and is gone the moment `dispose()` runs. This is what fails for the `unref()` version — that list contains only resources KEEPING THE EVENT LOOP ALIVE, so an unref'd timer never appears — and it also pins the other half: a settled sweep leaves nothing behind, which is what makes a referenced 90-minute timer affordable. - must-fire: a sweep aborted in the fleet-probe phase opens no lease and releases none. Scoped honestly in the test: every entry into `#claimDiscoverySweepUnderBudget` is preceded by a `budget.run` that throws first, so "already spent on entry" is a microtask race rather than a reachable state, and moving the claim inside the callback closes it by construction. The guarantee that does the work — `budget.run` never invokes its thunk once spent — is asserted directly on the primitive. - must-not-fire: a healthy sweep still claims exactly once and dispatches. The trivially wrong way to stop a spent budget claiming is to stop claiming. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: b1177efc-90da-4ff1-bda0-ef5de1b475e2 * fix(orchestrator): arm the shutdown lever before shutdown's first await Answers both open review threads on #374. `stop()` armed the sweep-budget grace timer only after awaiting `#heldAgentDeadlineSweepInFlight`, so an unrelated in-flight held-agent sweep silently extended a wedged discovery sweep's reprieve from `grace` to `held-agent sweep duration + grace` — unbounded if that sweep never returns, which is precisely the bound this change exists to provide. The grace is a timer; arming it costs nothing, so it now starts the clock at the moment shutdown starts, and the teardown it guards moved inside its `try` so the timer is still cleared on every path. Covered by a new must-fire that observes WHEN the lever arms rather than that it exists: a discovery sweep is wedged, a held-agent sweep is parked mid-release through the fleet, and the shutdown counter is read 3.2s into `stop()` — past the 2.5s grace, far short of the 60s budget. It fails `expected undefined to be 1` against the previous ordering. Also wraps the one test that arms the real 90-minute timer in `try/finally`. Without it a failing assertion left a *referenced* `Timeout` in the worker, so the failure would present as a hung suite instead of a named assertion. Proven with a temporary must-fire / must-not-fire pair: the old shape leaves the timer active after a throw, the new one does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 372b13bc-44a2-45a5-b5cc-aa228ccca39d * test(orchestrator): poll for the shutdown lever instead of racing its grace The new must-fire read the counter after a fixed 3.2s wait against a 2.5s grace — a few hundred milliseconds of headroom, which on a loaded worker is a new flake. This suite already carries two (#342, #373), and adding a third inside the PR whose subject is a wedge is the wrong trade. Polling costs the discrimination nothing: the held-agent sweep stays parked until the test releases it, so against the previous ordering `stop()` never reaches the arming call at all and the poll can only end in its own deadline. Re-measured both directions — pre-fix ordering: exit=1, "the shutdown lever never armed while an unrelated held-agent sweep was in flight"; with the fix: exit=0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 372b13bc-44a2-45a5-b5cc-aa228ccca39d
miyaontherelay added a commit
that referenced
this pull request
Aug 25, 2026
…e cadence that cannot
THIS COMMIT DOES NOT ADD A BOUND. It publishes the ones that already exist,
because their absence from the health stanza has now been read twice as their
absence from the code — including in the brief that asked for this fix.
WHAT THE STANZA SAID. A wedged 0.1.76 published:
"readinessReconcile": {
"state": "healthy", "consecutiveFailures": 0,
"failureThreshold": 3, "inFlightMs": 268232, "intervalMs": 60000
}
`intervalMs` is a scheduler tick and cannot preempt anything. Next to an
`inFlightMs` climbing 1:1 with wall clock it is indistinguishable from an
unbounded hang, and there was no field that could tell the two apart. The
reading taken from it — "there is no timeoutMs, so nothing bounds this" — is
false, and it is the reading this stanza invites.
WHAT IS ACTUALLY BOUNDING THAT PASS. Three deadlines, all live on this path in
0.1.76: `relayfileOperationTimeoutMs` per call (#351/#368),
`readinessReconcileTimeoutMs` on the caller's wait (#296), and the aggregate
`sweepBudgetMs` from #374 — `#reconcileReadyIssues` -> `#runOnceWithReadinessDeadline`
-> `runOnce()` -> `#runOnceWithDiscoveryFence` -> `startDiscoverySweepBudget`.
`readinessReconcile` IS the discovery sweep's health stanza; sweep-budget.ts
names it as such. The pass was bounded. It was bounded at 90 minutes, because
`sweepBudgetMs` derives from `reconcileTimeoutMs`, so at 268 s it had 89
minutes left to run and no field said so.
Two numbers now ship: `timeoutMs` (ends the wait) and `sweepBudgetMs` (unwinds
the sweep and hands the lease back). The second is the one that answers "when
does this recover", which is the question every reader of this stanza has
actually been asking.
`missedPasses` also moves onto the heartbeat record. It already existed on the
public projection (#295/#300) and was absent from the heartbeat stanza — which
is the surface an operator opens first, and the one every report so far has
quoted.
NOT A REPORTING BUG, AND DELIBERATELY NOT CHANGED. `state: "healthy"` at
268 s is correct. `derivedReadinessReconcileState` re-derives `stalled` from
`inFlightMs > intervalMs * READINESS_RECONCILE_STALL_INTERVALS`, and that
constant is 10 — so the flip was due at 600 s and the observation window
(14:37Z-14:41Z) closed 5.5 minutes early. Lowering it is the trivially wrong
fix: public-health.ts documents #36's 61-minute post-boot hydration as the
reason a small multiple cries wolf on every cold container.
TESTS, both against the real production numbers:
- must-fire: a heartbeat carrying the bounds publishes both, and reports
missedPasses 4 for the exact 268232/60000 pass above. Fail-first verified by
ablation — with only public-health.ts and types.ts reverted it fails
`expected undefined to be 5400000`.
- must-not-fire: a recorded `0` or negative bound is dropped rather than
republished as an instant deadline, and an instance predating the fields
still projects `healthy` with both absent. This one passes before and after
by construction: it is the guard on the trivially wrong version, not a
demonstration of the fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7miyaontherelay added a commit
that referenced
this pull request
Aug 25, 2026
…e sweep bounds that already exist (#379) * fix(health): publish the bounds that can preempt a sweep, not just the cadence that cannot THIS COMMIT DOES NOT ADD A BOUND. It publishes the ones that already exist, because their absence from the health stanza has now been read twice as their absence from the code — including in the brief that asked for this fix. WHAT THE STANZA SAID. A wedged 0.1.76 published: "readinessReconcile": { "state": "healthy", "consecutiveFailures": 0, "failureThreshold": 3, "inFlightMs": 268232, "intervalMs": 60000 } `intervalMs` is a scheduler tick and cannot preempt anything. Next to an `inFlightMs` climbing 1:1 with wall clock it is indistinguishable from an unbounded hang, and there was no field that could tell the two apart. The reading taken from it — "there is no timeoutMs, so nothing bounds this" — is false, and it is the reading this stanza invites. WHAT IS ACTUALLY BOUNDING THAT PASS. Three deadlines, all live on this path in 0.1.76: `relayfileOperationTimeoutMs` per call (#351/#368), `readinessReconcileTimeoutMs` on the caller's wait (#296), and the aggregate `sweepBudgetMs` from #374 — `#reconcileReadyIssues` -> `#runOnceWithReadinessDeadline` -> `runOnce()` -> `#runOnceWithDiscoveryFence` -> `startDiscoverySweepBudget`. `readinessReconcile` IS the discovery sweep's health stanza; sweep-budget.ts names it as such. The pass was bounded. It was bounded at 90 minutes, because `sweepBudgetMs` derives from `reconcileTimeoutMs`, so at 268 s it had 89 minutes left to run and no field said so. Two numbers now ship: `timeoutMs` (ends the wait) and `sweepBudgetMs` (unwinds the sweep and hands the lease back). The second is the one that answers "when does this recover", which is the question every reader of this stanza has actually been asking. `missedPasses` also moves onto the heartbeat record. It already existed on the public projection (#295/#300) and was absent from the heartbeat stanza — which is the surface an operator opens first, and the one every report so far has quoted. NOT A REPORTING BUG, AND DELIBERATELY NOT CHANGED. `state: "healthy"` at 268 s is correct. `derivedReadinessReconcileState` re-derives `stalled` from `inFlightMs > intervalMs * READINESS_RECONCILE_STALL_INTERVALS`, and that constant is 10 — so the flip was due at 600 s and the observation window (14:37Z-14:41Z) closed 5.5 minutes early. Lowering it is the trivially wrong fix: public-health.ts documents #36's 61-minute post-boot hydration as the reason a small multiple cries wolf on every cold container. TESTS, both against the real production numbers: - must-fire: a heartbeat carrying the bounds publishes both, and reports missedPasses 4 for the exact 268232/60000 pass above. Fail-first verified by ablation — with only public-health.ts and types.ts reverted it fails `expected undefined to be 5400000`. - must-not-fire: a recorded `0` or negative bound is dropped rather than republished as an instant deadline, and an instance predating the fields still projects `healthy` with both absent. This one passes before and after by construction: it is the guard on the trivially wrong version, not a demonstration of the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 * fix(orchestrator): bound the completion release retry, which is what was actually spinning The `no pid available to terminate ... during completion` lines repeating 15+ times in one evidence payload are a CO-SYMPTOM, not the cause. Fixing the PID classification would have changed nothing, and this commit explains why before it changes anything. WHY THE MISSING PID IS NOT THE LOOP. `#releaseAndTerminateAgents` logs that line when `#terminationRoots` returns `{ pids: [], status: 'unresolved' }`, then falls through. Nothing on that branch reaches `failed[]` — only a throw from `#fleet.release()` that is not `isAgentAlreadyGoneOnRelease` does. So the three agents were re-attempted because their RELEASE kept failing, and the no-PID line was printed once per agent per attempt on the way past. WHY THE LOOP NEVER ENDED. `#finishDurableRelease` does not throw on a failed release: it returns `false` and calls `#scheduleReleaseRetry`, which re-arms at `DISPATCH_LIFECYCLE_RETRY_MS` — 1 000 ms, unbounded. Every re-arm therefore arrives on the RESOLVED path, which is why the `.catch()` in both schedulers never bounded it and why a bound written there would have been a fix that never fired. The budget is charged at the scheduling point instead. WHAT A PASS COSTS, WHICH IS WHY 1 Hz FOREVER IS NOT FREE. Each pass calls `#terminationRoots` once per agent inside the release AND once per agent again inside `#writeInFlightRegistry` — a process-table scan each — plus a durable lifecycle read and write. For the three agents in the report that is order ten scans and several state operations per second, indefinitely. #303 already measured this exact shape once, at 1477 state GETs in 111 s, and bounded the RATE of the capacity-wait re-arm in response. It deliberately left the COUNT unbounded there, because waiting for capacity is legitimate. DESIGN CHOICE: (a) BOUNDED RETRIES, NOT (b) RECLASSIFY NO-PID. Not chosen under uncertainty — the code already tells the two cases apart, and it says (b) is wrong. `#terminationRoots` returns `'missing'` for confirmed-gone (a remote placement, or a process scan that came back missing AND a resolver that agreed) and `'unresolved'` for could-not-determine (no resolver and no recorded pids, an AMBIGUOUS scan, a resolver that returned nothing, or one that threw). The error only fires on `'unresolved'`. Treating that as already-terminated would mean skipping termination of a process that may well be alive — an ambiguous scan is literally "more than one candidate matched" — leaving orphans holding worktrees and slots. And it would not have stopped the spin regardless, per the first section. Release is also the opposite shape from #303's capacity wait, which is what makes bounding the count right here and wrong there: it is the last step of a work unit that is already finished — issue closed, writeback acknowledged, batch slot returned — so a release that has failed ten times is not waiting for anything. Ten attempts at the 1 s floor is ~10 s of genuine retry, which covers a control-plane blip or a lease handover and does not cover a permanent failure. SCOPED SO IT CANNOT ABANDON WORK THAT WAS NEVER FAILING: - Only release re-arms spend the budget. `#scheduleDispatchLifecycleRetry` takes an explicit `releaseAttempt` flag, so a `DispatchLifecycleCapacityError` or `DispatchLifecycleOwnedElsewhereError` — both legitimate waits on someone else — still retries forever, exactly as #303 intended. - Progress refunds the budget, so ten bounds CONSECUTIVE no-progress passes rather than capping a slow multi-agent release. This terminates: an agent released once is checkpointed and skipped next pass, so the remaining set strictly shrinks and a refund can only be earned finitely often. - The durable lifecycle is RETAINED on exhaustion. A takeover or a restart re-drives it from the persisted phase. This bounds one process's spin; it does not declare the work unit clean. - Keyed by `dispatchLifecycleKey`, so the budget follows the work unit rather than an agent, a surface or a dispatcher — the AR-448 identity rule. Exhaustion is logged at `error`, not `warn`, and increments `dispatchLifecycleReleaseAbandoned`. Every layer of this failure so far has been invisible until somebody read stderr by hand, and a work unit whose cleanup this process has permanently given up on must not be inferable only from the absence of further log lines. TESTS (3), against a fleet that reproduces the production shape exactly — `release()` throws for `issue-done` and `resolveAgentPid` returns `'unresolved'`, so the same no-PID line is emitted on every pass: - must-fire: the dead-letter counter reaches 1, the exhaustion error is logged, and three further seconds of wall clock buy no additional release attempts. Fail-first verified by ablation: with factory.ts reverted it fails after 40 543 ms with `expected undefined to be 1` — the wait can only end in its own deadline, because the loop re-arms for as long as the process lives. That is a property of the loop, not of any number chosen in the test. - must-not-fire: a release that succeeds still completes the work unit and releases each agent exactly once, with the counter unset. The trivially wrong way to stop a retry loop is to stop retrying. - must-not-fire: a release that fails several times and then succeeds still completes, with the counter unset — the transient case the retry exists for. Both must-not-fires passed under the ablation too, which is what makes them guards rather than restatements of the fix. RETRY CADENCE IS NOW AN INJECTABLE PORT, and that is a test-stability fix in its own right rather than a convenience. Exhausting a ten-attempt budget at the real 1 s floor costs ten real seconds per case; the first version of this suite did exactly that and added 41 s to `factory.test.ts`. Run beside two other files it pushed an already-300 s combination over an edge and four UNRELATED tests began failing on timing — the reopen-fence and Slack-reply-route cases — while the same three files passed on `origin/main` and `factory.test.ts` alone passed 631/631 on the branch. Buying a fourth flake in this suite (it already carries #342 and #373) to test a fix for a spin is the wrong trade. `dispatchLifecycleRetryMs` follows the existing convention for exactly this — `babysitterWakeUnreachableRetryMs`, `babysitterWakeUnreachableEscalateMs`, `startupAgentExitDrainTimeoutMs` are all test-only port overrides of a built-in timing. Only the delay between attempts moves; the BUDGET under test is the real one. Overhead is now +4 s, the four unrelated failures are gone (709/709 on the same three files), and the ablation still fails with `expected undefined to be 1` — unambiguously, because `dispatchLifecycleReleaseAbandoned` does not exist on `origin/main` at any cadence. The transient case sets a failure count on the fake rather than flipping a flag from the test body, so it cannot race the cadence it runs under. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 * fix(orchestrator): make the release bound actually fire on the durable path Answers three P1 findings on #379. The first is the important one: the bound as first written DID NOT FIRE in production, and the review caught it. 1. THE BUDGET RESET ON THE PATH THAT MATTERS, SO THE BOUND NEVER FIRED. `#driveDispatchLifecycle` discards `#finishDurableRelease`'s boolean in its `phase === 'releasing'` branch, and that method returns `false` rather than throwing on a failed release. So a FAILED release makes the drive RESOLVE, and the scheduler's success handler ran on every re-arm — where it called `#clearReleaseAttempts`. The counter was zeroed once per pass and could never reach the cap. This is the same never-fires shape the first version of this commit correctly rejected in the `.catch()`, moved one layer over into the `.then()`. Diagnosing the resolved path as the live one and then putting the refund on it was the error. The refund is removed from the scheduler entirely. It now happens only where success is actually known: `#finishDurableRelease` clears the budget on real per-agent progress and again when the work unit completes. WHY THE ORIGINAL TESTS MISSED IT. `#usesDurableDispatchLifecycle()` is `durableOwnership ?? placementLocality === 'remote'`, and `FakeFleetClient` places locally, so all three original cases exercised `#scheduleReleaseRetry`'s own timer — which has no success handler and therefore no reset. The deployed Factory places remotely. The suite proved a property of the path production does not take. New must-fire on the DURABLE path (`RemoteLifecycleFleetClient` + `InMemoryStateStore`), asserting the counter SURVIVES ACROSS RE-ARMS rather than that a dead-letter is reachable by some path. Fail-first verified by ablation: restore the `#clearReleaseAttempts(key)` line and only that case fails, `expected undefined to be 1` after 10 125 ms, while the three local cases still pass — which is what pins the discrimination to the durable path. 2. THE WRONG BUDGET WAS CHARGED. The generic arm of the drive's `.catch()` re-arms for dispatch, publishing and recovery failures as well as releases, and it charged all of them. That would dead-letter a work unit that was never stuck in a release loop. Charging is now confined to `#scheduleReleaseRetry`, whose every caller is a release failure: the three inside `#finishDurableRelease`, and `#completeIssue`'s catch once `releaseReasonForRetry` is set. The generic re-arm passes no charge at all. Pinned by a call-site audit rather than by a behavioural test, and that is deliberate. I could not reach that arm from a realistic fixture — forcing durable lifecycle reads to throw makes the agent-exit handler fail before any lifecycle retry is scheduled, so a test built that way passes whether or not the narrowing is present. Confirmed by ablation: with `releaseAttempt = true` restored, the fixture-based version still passed, and instrumenting it showed zero `durable dispatch lifecycle retry failed` warnings — the branch was never entered. Shipping that would have been a test that proves nothing, so the audit states the structure instead. Known gap, stated plainly: a release failure that THREW out of `#finishDurableRelease` would reach the generic arm and re-arm unbounded. Every failure path in that method returns `false` and schedules its own retry, so this is not a reachable shape today, and if one appears it degrades to the pre-existing unbounded behaviour rather than to a wrong dead-letter. 3. THE DEAD-LETTER LEAKED THE SLOT. Trading an unbounded 1 Hz spin for a permanently leaked in-flight record is not obviously the better failure: a spin is loud and self-describing, while a leaked slot silently reduces dispatch capacity until the process is restarted. Local completion never calls `batch.complete`, so exhaustion left the work unit in flight forever. `#releaseDeadLetteredSlot` now hands the batch slot back, drops any uncompensated claim, rewrites the in-flight registry, and admits whatever was queued behind it — a freed slot nothing is admitted into is only half the repair. The durable lifecycle is still deliberately RETAINED in `releasing`, so a successor or restart re-drives the same cleanup with a fresh budget; freeing a process-local slot is not a terminal phase and does not declare the work clean. The work unit therefore ends up recoverable, never merely abandoned. Must-fire asserts the slot is released after exhaustion. Fail-first by ablation: stub the call out and it fails with the work unit still in flight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
AR-287 had already been answered in Slack before the upgraded factory process started. The watcher seeded existing Slack events and ignored them, so only a brand-new reply would unblock dispatch. This preserves old-reply suppression for normal dispatched agent threads, but lets triage escalation threads consume the existing human clarification.
Testing