feat(health): publish why a dispatch attempt failed, as a bounded enum - #361
Conversation
`skipReasons` (#358) told us the live container skips 27 candidates every sweep and that 5 of them are `dispatch-failed` — with the control-plane breaker closed, the fleet agent online and `readinessReconcile` healthy. That bucket is a count. It says the sweep got all the way to dispatching and dispatch threw; it does not say what threw, and the message that would say so goes to the daemon's stdout, which does not reach the deployed container's operator. This is the second level of the same breakdown, built the way #358 built the first: - `dispatchFailures` is the total, published as a zero once a sweep completes. `skipReasons` omits zero counts, so on that field alone "every dispatch succeeded" and "this producer has never heard of dispatch failures" are the same absence — and 0.1.72 is in production being exactly the second thing. - `dispatchFailureReasons` splits it by a closed vocabulary. Counts only, keys rebuilt from this side's own list rather than taken from the record, unknown codes folded into `other` so the parts still sum. The code is recorded at the skip site from the thrown value, never parsed back out of the operator-facing `reason` — a reworded message would silently empty a bucket. Classification follows the cause chain, because `contextualError` and the control-plane guard both rethrow wrapped. When nothing named matches, the *phase* that threw is the answer: `unclassified-gate` / `-triage` / `-dispatch`. A unit that failed in triage never reached the fleet, and on a surface carrying no messages the phase is the only thing left that still says who should look. #292's own tests now pin those codes: the `TypeError: fetch failed` case from #291 is `unclassified-triage`, not a fleet fault. `dispatchFailures` is deliberately NOT joined to the all-or-nothing candidates/dispatched/skipped trio. Requiring it would drop a 0.1.72 daemon's whole sweep block — deleting the counters that are currently the only view of the outage. Tests drive the real writer: a live daemon whose real dispatch really throws. Ablated five ways — absent coerced to zero, zero dropped as uninteresting, unknown codes dropped instead of folded, the skip site not classifying, and the incoming string used as an object key — each caught by the assertion that names it. Refs #355, #358 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai review Requested for exact head |
|
Warning Review limit reachedNext included review available in 50 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a52b7c63b5
ℹ️ 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".
|
Two things before this merges, and the first one is my doing. 1. Rebase — the conflict is mine
Watch for one thing while resolving. #359 added 2. The live P2 at
|
Review follow-up (P2, codex). The overload branch called `relayfileOverload(error)`, which reads the flat error object and does not walk `cause` — so a 429 arriving through `#spawnAgent`'s `contextualError` was published as `unclassified-dispatch`. The observation is correct. The suggested fix — traverse the cause chain here — would make this surface worse, not better. `relayfileOverload` is the same predicate that decides which counter increments, whether the unit counts toward the pass-abort fuse, and whether the durable overload ratchet advances. A health record reporting `relayfile-overloaded` for a unit the loop handled as an unexplained fault would tell an operator the overload machinery was engaged when it was not. So the code now takes the loop's verdict as an argument instead of re-deriving it, and cannot disagree with it. Widening `relayfileOverload` itself is the real fix for the underlying gap; it changes shedding behaviour, so it is not this PR's to make. Two tests pin both directions: an unwrapped 429 (through triage, the one dispatch path that does not wrap what it throws) publishes `relayfile-overloaded`, and the same 429 through the spawn — where the wrapper hides it from the loop too — publishes `unclassified-dispatch`. Applying the suggested traversal fails the second one. Refs #355 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@coderabbitai review Requested for exact head |
|
@coderabbitai review Please review exact current head 082675c after the bounded wrapped-overload fix and additive main merge. |
|
Exact head 082675c also merges current main additively: dispatch failure totals/breakdown travel with the retained enumerating sweep, lastEnumeratedAtMs dates that snapshot, discoveryDeferred remains independent for the most recent pass, and enumerationCountsInvalid remains distinct from a genuine no-enumeration deferral. Combined health/timing tests 70/70; build and feature-map 323 pass; diff check is clean. |
|
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Merging. This is the last instrument of the incident and it is built to the standard the previous four set. The P2 fix is better than what I asked for. I asked you to traverse wrapped causes; you traversed them bounded: const RELAYFILE_OVERLOAD_CAUSE_DEPTH = 4
const relayfileOverload = (error, depth = 0) => {
if (depth > RELAYFILE_OVERLOAD_CAUSE_DEPTH) return undefined
...
return relayfileOverload(flat.cause, depth + 1)
}Unbounded Tests I checked specifically:
And you preserved Bar cleared: 5/5 green at The rebase you had to do was my fault — I merged #341 and #359 under you after flagging the sequencing risk myself. Thanks for absorbing it cleanly. Next: release, bump, deploy, then one |
1 participant
Closes the last unknown in the #355 dispatch outage:
dispatch-failed: 5, every sweep, with everything upstream green — and no way to see why.What the surface says today
The 34-minute non-deferred sweep measured on the live container:
dispatch-failed: 5is a bucket count. It says the sweep enumerated fine, selected five eligible units, attempted dispatch, and dispatch threw — five times, withfleetControlPlaneclosed, the fleet agent online andreadinessReconcilehealthy. It does not say what threw. The daemon knows;perItemDispatchSkipReasonbuilds the sentence and hands it to the operator log, which is container stdout, which does not reachwrangler tail.What this adds
The second level of the same breakdown, following #358 exactly:
dispatchFailuresskipReasons['dispatch-failed']carries, published in its own right so it can be a zerodispatchFailureReasonsNew module
src/orchestrator/dispatch-failure-reason.tsholds the vocabulary, the normaliser and the counter — the same three thingssweep-skip-reason.tsholds.The four hard constraints
1. Numbers only.
dispatchFailureReasonsis rebuilt key by key from this module's own copy of the list; an incoming string is never used as an object key. That is the leak vector #358 closed, and the same test shape guards it here — a breakdown keyed byAR-350 /linear/issues/AR-350__uuid.jsonandError: connect ECONNREFUSED 10.0.0.4:443publishes as{ 'spawn-ack-timeout': 4, other: 5 }.2. Zero is distinguishable from absent.
dispatchFailuresusesoptionalCount, nevercounter(). Three readings, three shapes:0→ a sweep completed and no dispatch it attempted failedn > 0→ with a breakdown that sums tonskipReasonsalone cannot express the middle one, because it drops zero buckets — which is why the total exists next to the breakdown rather than being inferred from it.3. Unknown codes fold into
other. So the parts still sum todispatchFailures, and a reader comparing them detects a newer producer rather than concluding the counter is broken. Copied from factory-cloud#74.4. No timeout or budget touched.
One deliberate departure from #358
dispatchFailuresis not joined to the all-or-nothingcandidates/dispatched/skippedtrio. Requiring it would make a 0.1.72 daemon — the one in production right now — fail the guard and lose its whole sweep block, deleting the counters that are currently the only view of the outage. It is independently optional instead, and the test pins that: a record carrying the trio and not this field still publishes the trio.The vocabulary
Named causes first, classified by type through the cause chain (
contextualErrorand the control-plane guard both rethrow wrapped, so reading only the outermost error would lose every name):relayfile-overloaded,live-state-changed,late-placement-released,lifecycle-terminal,lifecycle-owned-elsewhere,control-plane-open,timed-out,spawn-ack-timeout,fleet-identity-read-only,agent-registration-failedThen the phase codes —
unclassified-gate,unclassified-triage,unclassified-dispatch— andother.The phase codes are not a failure of the vocabulary; they are its most useful answer when nothing named matches. A unit that failed in triage never reached the fleet. On a surface that carries no messages, the phase is the only thing left that still says who should look. #292's own tests now pin them, and the result is worth seeing: the
TypeError: fetch failedcase from #291 classifies asunclassified-triage, and the open-circuit dry-run case isunclassified-triagetoo — its injected fault is the "unrelated per-item fault", not the circuit. Those were conflated in one bucket before.Five of the codes are matched by allowlisted error class name rather than
instanceof, documented at the map: their classes live inrelay-fleet-client.ts, and adding that import edge tofactory.tswould pull the relay SDK into the orchestrator's module graph to buy precision over a code-controlled identifier. A rename there degrades one bucket tounclassified-dispatch; it cannot leak and it cannot break the sum.Tests
src/orchestrator/dispatch-failure-reasons.test.tsdrives the real writer — a live daemon whose realspawnreally throws — rather than hand-settingfailureCodeon a report fixture, which would prove the projection copies a field it was handed and nothing about the sweep that has to produce it.Ablated five ways, each caught by the assertion that names it and with the right failure message:
counter()instead ofoptionalCount(absent → 0)three different readingsexpected true to be falseexpected undefined to be +0expected { 'spawn-ack-timeout': 4 } to deeply equal { …, other: 5 }failureCodeexpected { other: 1 } to deeply equal { 'spawn-ack-timeout': 1 }expected '…' not to contain 'AR-350'The must-fire/must-not-fire pair is explicitly non-interchangeable — the CONTROL asserts each throws when aimed at the other sweep, so a pass is evidence about the sweep rather than about a constant.
Local verification
tsc --noEmit -p tsconfig.build.json— cleanpublic-health+sweep-counters+health-projection-guard+diagnose+dispatch-failure-reasons— 87/87factory.test.ts— 583/583 (last run). An earlier run failed onlyreaps a first dispatch whose spawn never returns (#303), which is item 3 on the flake: shared timing substrate in src/orchestrator/factory.test.ts fails 3 different tests nondeterministically #342 tally.Four assertions in
factory.test.tsneeded updating:toContainEqualonreport.skippedis exact, so the newfailureCodekey broke them. They now pin the codes, which is free coverage of the classifier from #292's own scenarios.Also rendered in
factory diagnose, on its own line and including the zero, for the same reason it is published at all.Refs #355, #358
Summary by cubic
Publishes structured reasons for failed dispatch attempts on the health surface. Previously
skipReasons['dispatch-failed']was only a count; now we emitdispatchFailures(zero-capable total) anddispatchFailureReasons(bounded, counts-only breakdown). Also fixes classification to detect provider overloads even when wrapped by context, reducing “unclassified” cases.src/orchestrator/dispatch-failure-reason.ts; classifies at the skip site with bounded cause-chain matching, including phase codesunclassified-gate/triage/dispatchand allowlisted error class names; walks wrapped relayfile overloads.other; counts only—no issue keys, paths, or messages cross.dispatchFailuresis optional and independent of thecandidates/dispatched/skippedtrio; zero survives round-trips; breakdown is shown only when the total is present.factory.ts, tracks attempt phase, recordsfailureCodeondispatch-failedskips, aggregates viafactoryDispatchFailureReasonCounts, and exports helpers fromsrc/index.ts.public-health.ts) to normalize totals and breakdowns (drops orphan/invalid data) and renders them insrc/cli/diagnose.ts.types.ts) to includedispatchFailures,dispatchFailureReasons, and per-itemfailureCode; updates tests and adds end-to-end coverage, including wrapped overload classification.Written for commit 082675c. Summary will update on new commits.