Skip to content

alerting: SDK typed Event::Status + health_watch example - #485

Merged
bkeroack merged 3 commits into
masterfrom
feature/a3-sdk-status
Aug 2, 2026
Merged

alerting: SDK typed Event::Status + health_watch example#485
bkeroack merged 3 commits into
masterfrom
feature/a3-sdk-status

Conversation

@bkeroack

Copy link
Copy Markdown
Contributor

PR 6 of the A3 alerting stack (SATD_ALERTING_DESIGN.md §10). Stacked on #484#483#482#481#480 — merge bottom-up.

Closes the loop: the node detects (#481), the wire carries (#480), webhooks push (#482/#483), and a Rust client can now consume health events with types instead of proto.

What

Categories::STATUS plus a typed Event::Status { kind, state, severity, message, details }.

Decisions worth reviewing

  • The enums are open. StatusKind, StatusState, and StatusSeverity each carry an Unknown(i32) arm. A condition a client build predates arrives as Unknown rather than failing to decode, and its severity and message stay meaningful — so a generic "log it, page on critical" handler keeps working when the node adds a taxonomy entry. That is the whole reason details is a string map and StatusKind is an open enum on the wire; the SDK would waste it by decoding strictly.
  • StatusSeverity derives Ord, so a severity floor is severity >= StatusSeverity::Warning rather than a match that has to be updated. An unrecognized severity sorts above Critical on purpose: a condition the client cannot name is not one to quietly filter out.
  • examples/health_watch.rs shows the shape a real integration wants — track raise/clear pairs to hold what is wrong right now, rather than counting alerts (a number that only ever grows). It also documents the two things that surprise people: the category is explicit-request only, and status events are not replayable, so a client connecting after a raise sees nothing until the condition changes (seed from getwarnings if you need the full picture on connect).

Acceptance (design §10 PR 6)

  • E2E against a live node, both directions: a STATUS subscription decodes a real disk_low with its details map and no cursor; a CHAIN-only subscription never receives one.
  • Example compiles and is documented in the SDK manual chapter.
  • clippy clean; full suite green except the 10 pre-existing test_address_index_backfill_* disk-guard failures on this machine.

The crates.io publish dry-run stays red until the 0.5.0 proto publish — known and pre-existing on every PR in this repo right now, unrelated to this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH

@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 1a53593 to 8c23001 Compare July 24, 2026 22:39
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 4a8d2b0 to d010d05 Compare July 24, 2026 22:39
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 8c23001 to e1238a4 Compare July 24, 2026 22:53
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from d010d05 to c0ac311 Compare July 24, 2026 22:53
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from e1238a4 to d8344e7 Compare July 25, 2026 01:27
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from c0ac311 to 18ca230 Compare July 25, 2026 01:27
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from d8344e7 to 1c0ab06 Compare July 25, 2026 15:46
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 18ca230 to cf3b351 Compare July 25, 2026 15:46
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 1c0ab06 to 29d1817 Compare July 25, 2026 22:15
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from cf3b351 to 32055f9 Compare July 25, 2026 22:15
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 29d1817 to 4867fcd Compare July 25, 2026 23:27
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 32055f9 to 1794109 Compare July 25, 2026 23:27
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 4867fcd to 6d430ff Compare July 30, 2026 00:51
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 1794109 to 511378c Compare July 30, 2026 00:51
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 6d430ff to 12e7b8d Compare July 30, 2026 02:05
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 511378c to 8716132 Compare July 30, 2026 02:05
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 12e7b8d to 1a52afa Compare July 30, 2026 05:36
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch 2 times, most recently from 829b586 to 880a619 Compare July 30, 2026 14:37
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 1a52afa to 62e9d27 Compare July 30, 2026 14:37
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 880a619 to 95cb804 Compare July 30, 2026 15:54
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch 2 times, most recently from 90e36f9 to ef73e3c Compare July 30, 2026 19:10
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 95cb804 to 0697556 Compare July 30, 2026 19:10
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from ef73e3c to d729fac Compare July 30, 2026 19:38
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 0697556 to 3a840e1 Compare July 30, 2026 19:38
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from d729fac to 1f18a9b Compare July 30, 2026 19:56
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 3a840e1 to 39b9f66 Compare July 30, 2026 19:56
@bkeroack

Copy link
Copy Markdown
Contributor Author

Deep review — findings (#485 + #486, plus #487/#490)

Independent review; reviewer-reported, not personally re-verified.

#485 — SDK typed Event::Status

1. The three new open enums lack #[non_exhaustive] — MEDIUM. Every other open enum in the crate has it (EvictReason, CursorRejectReason, WatchSetRejectReason, RescanRejectReason), as does Event. When satd adds STATUS_KIND_HEADERS_STALL, every downstream exhaustive match breaks at compile time — including this PR's own health_watch example. The crate is publish = true, so an additive node taxonomy entry would force a breaking SDK release, which is exactly what the Unknown(i32) arms were added to avoid.

2. The "silence is itself a signal" fix is inert — MEDIUM. HEARTBEAT is subscribed and then unconditionally discarded; nothing applies a deadline and message().await blocks indefinitely. If the node process is alive (so tonic keepalive pings still answer) but the publisher is wedged, the example blocks forever, prints nothing, and the operator reads "no output" as "healthy". A tokio::time::timeout sized to the heartbeat interval is what the comment describes.

3. The canonical "alerting shape" bypasses the crate's own resilience layer — MEDIUM. Plain client.subscribe(...) plus ? on message(): any transient stream error terminates the process. The node restarting is precisely when standing conditions are re-raised — and since status is not replayable, an unsupervised copy of this pattern is permanently off with no signal. resilient_subscribe takes the same SubscribeOptions.

4. The example prints "→ all clear" while a critical condition is standing — MEDIUM. active is seeded only from live events. disk_low raises at T0, the example connects at T1 and learns nothing, an unrelated condition clears at T3 → active empties → "all clear" while the disk is still filling. The rustdoc says to seed from getwarnings on connect; the code operators will copy does not.

5. That "seed from getwarnings on connect" instruction is unimplementable via resilient_subscribe — MEDIUM. ResilientSubscription::next hides reconnects entirely and the only synthetic notice (ReplayGap) is cursor-anchored, but status carries no cursor. A 90 s drop during which tip_stall raises and disk_low clears leaves the client's active-condition map silently wrong forever.

6/7 — LOW. STATUS_SEVERITY_UNSPECIFIED = 0 decodes to Unknown(0), which derived Ord sorts above Critical — so a later debug/notice level at proto value 4 would page everyone using the documented severity >= Warning filter. And sdk_status_absent_without_the_category_bit exits its loop on the first timeout, so a slow CHAIN stream makes the category-leak assertion vacuous while still going green.

#486 — reference push relay

Credential handling is genuinely good — hand-written redacting Debug, TOML errors stripped of the source line, perms checked on the open handle (no TOCTOU), deny_unknown_fields, loopback default, no TLS verification disabled anywhere, no device token or credential ever logged, ring-only committed lock. The README's caveats match the code unusually well.

1. The "whole-request deadline, headers included" comment is wrong — MEDIUM. tower_http::timeout::TimeoutLayer only starts once hyper invokes the axum service, i.e. after the request head is parsed; no http1_header_read_timeout is configured. A peer that can reach listen opens N connections and dribbles one header byte per minute — bypassing both the timeout layer and MAX_CONCURRENT_REQUESTS, exhausting fds/tasks so satd's real deliveries are refused and alerts stop silently. Body-phase slowloris is covered; header-phase is the case the comment names.

2. SIGTERM handling doesn't do what it says — MEDIUM. The push runs in a detached tokio::spawn that with_graceful_shutdown does not wait for; when serve returns, the runtime is dropped and in-flight pushes abort. systemctl restart during a critical disk_low push → push aborted, satd already got its 200, the id is in the dedup ring, no retry, operator never paged. A JoinSet/TaskTracker awaited after serve closes it.

3/4 — LOW. Neither token cache is invalidated on 401/403, so an APNs InvalidProviderToken after a key rotation drops every alert for the remaining ~45 min, recoverable only by restart. And the device token is interpolated into the URL path unvalidated, so a stray / or ? silently retargets the request — with no token ever logged, the only symptom is a generic rejection warning.

#487 / #490 — both clean

#487 is a single line, the only setup-python@ in the repo, python-version pinned explicitly, confined to the use_zigbuild musl path — and that path was actually exercised (tarball / aarch64-unknown-linux-musl passed on the dry-run), so musl-static is verified against v7 rather than assumed.

#490 is a real security bump, not version drift: Cargo.lock only, the delta is quinn-proto plus forced transitive moves (getrandom 0.3→0.4, rand 0.9→0.10, new rand_pcg), upstream includes the frame-assembler memory-exhaustion fix, and it arrived without a cargo entry in dependabot.yml — i.e. from security updates. TLS stays ring-only: quinn's rustls path is ring, and quinn only enters the graph via reqwest's optional http3 feature. deny.toml has multiple-versions = "warn" so the duplicate rand/getrandom majors can't fail the supply-chain gate. The one red check is test_rpc_mtls_round_trip failing with ConnectionRefused — the known regtest RPC startup race; needs a rerun, not a fix.

@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 1f18a9b to e492b00 Compare July 31, 2026 21:41
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 39b9f66 to 05be291 Compare July 31, 2026 21:41
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from e492b00 to 08da44a Compare August 1, 2026 04:06
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 05be291 to 7b9cfd5 Compare August 1, 2026 04:06
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 08da44a to 38b7ad1 Compare August 1, 2026 14:09
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 7b9cfd5 to 5dc98aa Compare August 1, 2026 14:09
@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from 38b7ad1 to d28d014 Compare August 1, 2026 17:01
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 5dc98aa to 68cd422 Compare August 1, 2026 17:01
@bkeroack

bkeroack commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Tier 1 + Tier 2 fixes (68cd4226)

Finding 1 — missing #[non_exhaustive] (the one with a deadline). Fixed on StatusKind, StatusState, StatusSeverity. Every other open enum in the crate already had it.

The Unknown(i32) arm is not sufficient on its own: a downstream match listing every variant including Unknown(_) is exhaustive, so a node-side addition would break it. The crate is publish = true, and adding the attribute after publication is itself breaking — so this had to land before the 0.5.0 cut or not at all.

That the example's own matches stopped compiling is the proof: it had exactly the exhaustive-match shape a consumer would write.

Finding 2 — the heartbeat fix was inert. Fixed. HEARTBEAT was subscribed with a comment about silence being a signal, then unconditionally discarded, with nothing applying a deadline. A node process that is alive but whose publisher is wedged still answers gRPC keepalives, so next() blocked forever, the example printed nothing, and "no output" read as "healthy" — failing open in the one direction alerting must not. There is now a 90 s deadline that exits non-zero.

Finding 3 — the canonical shape bypassed the crate's own resilience layer. Fixed: resilient_subscribe instead of plain subscribe with ? on message(). The node restarting is both the most common cause of a transient stream error and the moment every standing condition is re-raised, so an unsupervised copy was off for good after its first blip.

Findings 4 and 5 — "→ all clear" while a critical condition stands, and the unimplementable seeding instruction. These are the same problem and are fixed together, by telling the truth rather than pretending.

Seeding from getwarnings on connect genuinely cannot be made correct through ResilientSubscription: it hides reconnects, and its only synthetic notice (ReplayGap) is cursor-anchored while status carries no cursor. A drop straddling a raise and a clear leaves the picture silently wrong with nothing to key recovery off.

So the example now labels its set "observed by this client" and prints "nothing standing that this client has observed" rather than "all clear", and both the example and the Event::Status rustdoc say that getwarnings polled on a slow timer is the authority, with this stream as the low-latency edge signal on top. That also makes a missed transition self-correcting instead of permanent — which seeding-once-on-connect would not have achieved.

Deferred (Tier 3)

  • Finding 6 — Unknown(0) sorts above Critical. Note the ordering is documented as deliberate for unknown future values ("a condition this build cannot name is not one to quietly filter out"), and I agree with that. The real edge is narrower: STATUS_SEVERITY_UNSPECIFIED = 0 also decodes to Unknown(0), so an absent/default value reads louder than critical. Fixing it means a hand-written Ord on a published type, and no such proto value exists today. Worth doing before the taxonomy grows.
  • Finding 7 — sdk_status_absent_without_the_category_bit is vacuous (exits its loop on the first timeout). The assertion it fails to make is now covered by the catauth and ws unit tests added for the authorization gate on alerting: StatusEvent wire schema + status category (bit 16) #480.

@bkeroack
bkeroack force-pushed the feature/a3-webhooks-spec branch from d28d014 to fe430c1 Compare August 2, 2026 00:03
@bkeroack
bkeroack changed the base branch from feature/a3-webhooks-spec to master August 2, 2026 00:03
bkeroack and others added 3 commits August 1, 2026 18:03
Round-3 review fix: the `health_watch` example subscribed to `STATUS` alone and
printed "nothing printed means nothing is wrong".

Unknown category bits are ignored by design, so that subscription against a
pre-0.5.0 node is accepted, matches nothing, and yields an open connection that
stays silent forever — indistinguishable from a healthy node, and failing open
in the one direction alerting must not. It now subscribes to
`STATUS | HEARTBEAT`, so silence is itself a signal.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
The `details` rustdoc is what a client author reads while writing the lookup, so
two errors in it are two client bugs.

It described the map as "decimal strings" — a `cleared` event can carry a
`reason` token (`detector_disabled`, `mempool_cap_zero`), so a uniform integer
parse over the map fails on a perfectly ordinary event. And it advertised "the
watched path" as one of the fields, which the node deliberately withholds: the
path reaches every status subscriber and every webhook receiver, and an absolute
datadir path usually names the account the node runs under. A client author
would have written a lookup for a key that never arrives.

`StatusKind::TipStall` said "outside IBD". The detector deliberately does not
suppress during IBD and says why at length — the IBD predicate is the tip
header's age, not a sync flag, so a node that was caught up and then wedged
re-enters it exactly when paging matters.

Also corrected a stale `resilient_watch` comment claiming both carriers map
`categories: 0` to `u32::MAX`; they map it to the default mask, which excludes
the explicit-only `tweaks` and `status` bits — the whole point of those bits.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
**`#[non_exhaustive]` on `StatusKind`/`StatusState`/`StatusSeverity`.**
Every other open enum in this crate has it (`EvictReason`,
`CursorRejectReason`, `WatchSetRejectReason`, `RescanRejectReason`,
`Event` itself) and these three were missed. The `Unknown(i32)` arm is
not sufficient on its own: a downstream `match` that listed every variant
including `Unknown(_)` is exhaustive, so adding a kind node-side would
break it. The crate is `publish = true`, so shipping without this makes
an additive node change a breaking SDK release, permanently. Adding it
after publication is itself breaking — this is the one finding with a
deadline.

That the example's own matches stopped compiling is the proof: it had
exactly the exhaustive-match shape a downstream consumer would write.

**The example now reconnects.** It used a plain `subscribe` with `?` on
`message()`, so any transient stream error ended the process — and the
node restarting is both the most common cause and the moment every
standing condition is re-raised. An unsupervised copy was off for good
after its first blip, with no signal.

**And enforces the silence it subscribes to.** HEARTBEAT was requested
with a comment about silence being a signal, then unconditionally
discarded, and nothing applied a deadline. A node process that is alive
but whose publisher is wedged still answers gRPC keepalives, so `next()`
blocked forever, the example printed nothing, and an operator read "no
output" as "healthy" — failing open in the one direction alerting must
not. There is now a 90s deadline that exits non-zero.

**And it no longer prints "→ all clear" while a critical condition
stands.** The set was seeded only from live events, so a `disk_low`
raised before connect was invisible and any unrelated clear emptied the
set. The rustdoc told callers to seed from `getwarnings` on connect,
which is not implementable through `ResilientSubscription`: it hides
reconnects, and its only synthetic notice is cursor-anchored while status
carries no cursor — so a drop that straddles a raise and a clear leaves
the picture silently wrong with nothing to key off.

Rather than pretend, both the example and the rustdoc now say what is
true: this is a partial view labelled "observed by this client", and
`getwarnings` polled on a slow timer is the authority. That also makes a
missed transition self-correcting instead of permanent.
@bkeroack
bkeroack force-pushed the feature/a3-sdk-status branch from 68cd422 to 7758ebd Compare August 2, 2026 00:03
@bkeroack
bkeroack merged commit 706b334 into master Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant