alerting: reference APNs/FCM push relay in contrib/ - #486
Conversation
4a8d2b0 to
d010d05
Compare
c178230 to
d7f5e7f
Compare
|
Round-1 review fixes. High — Medium — ack-before-push is at-most-once end to end. The dedupe/ACK ordering itself is right, and the id collision that made it dangerous is fixed in #483. The README now states the residual cost plainly: after a 200, satd will not resend, so a push that then fails at APNs/FCM is gone. It also says what not to do about it — moving the ACK after the push trades the loss for head-of-line blocking on the whole hook; persist an outbox before acknowledging instead. |
d010d05 to
c0ac311
Compare
d7f5e7f to
7916e0e
Compare
c0ac311 to
18ca230
Compare
7916e0e to
e81b4fa
Compare
18ca230 to
cf3b351
Compare
e81b4fa to
75f672b
Compare
cf3b351 to
32055f9
Compare
75f672b to
1061ae6
Compare
32055f9 to
1794109
Compare
1061ae6 to
7fc49a7
Compare
1794109 to
511378c
Compare
7fc49a7 to
0be7e3c
Compare
511378c to
8716132
Compare
0be7e3c to
5a97f0a
Compare
8716132 to
829b586
Compare
5a97f0a to
a6de96a
Compare
829b586 to
880a619
Compare
495935b to
a8f72d2
Compare
880a619 to
95cb804
Compare
… of artifacts Deep-review fixes for the health detectors, plus a repo-hygiene P0 that this branch introduced. **831 MiB of build artifacts were committed here and pushed.** Commit 04cb6da ("read reorg depth from the log") was meant to touch four files; a stray `git add -A` swept in 2201 files under `contrib/push-relay/target/` — a 79 MB debug binary, 28 MB of rlibs, and `.rustc_info.json` recording local toolchain paths. They were unambiguously accidental: the push-relay *source* does not exist until four PRs later, so this branch carried a compiled binary for a crate it did not contain. Nothing built or linted those paths, so CI stayed green. satd squash-merges, so this never entered master's history — but the squashed *tree* would have, permanently, in a public repo. `git rm -r --cached` here, and the `/contrib/push-relay/target` ignore rule moves down from #486 to sit ahead of the crate it protects, since .gitignore cannot untrack what is already tracked. **`deep_reorg` was gated on a wall-clock high-water mark, so a backwards clock step silenced it entirely.** `ReorgWatermark` seeded `SystemTime::now()` at task start and dropped any record stamped earlier. `ReorgRecord::ts_unix_secs` is also `SystemTime::now()`, so one NTP correction of a fast RTC after boot — or a hypervisor resync after live migration, or `date -s` — meant every reorg for the next N minutes was discarded. The watermark never reset and `deep_reorg` is an edge, so those alerts were not delayed; they were gone. Replaced with `ReorgSeen`, a bounded set of `(ts, old_tip, new_tip)` identities that depends on no clock. It is seeded from the log at startup, which also removes the reason the watermark existed. **The 300 s lookback was a second way to lose an edge forever.** Any delay of this task past the window — API-runtime saturation, a `statvfs` on a hung mount, a VM pause — dropped older reorgs out of view permanently, with no catch-up path. The window bought nothing now that de-duplication is exact, so the scan covers the whole ring (≤256 records). **`statvfs` now runs on `spawn_blocking`.** `disk_watch_path` defaults to `blocksdir`, which operators routinely point at NFS/iSCSI, and `statvfs` on a hung mount blocks uninterruptibly. Inline, it parked an API worker and froze *every* detector — including the two above — for as long as the mount stayed wedged. **The shutdown branch could spin at 100% CPU.** `_ = shutdown.changed() => if *shutdown.borrow() { return }` never returns once the last sender drops: `changed()` then fails immediately and forever while `borrow()` still reads false. Latent in the current wiring, one refactor from real; every other shutdown handler in the tree returns unconditionally, and now so does this one. Also: `disk_low` logged an identical WARN every poll (5,760/day while the condition holds) — now edge-only; the config reference claimed `tip_stall` is IBD-suppressed (the code deliberately does not suppress it, and says why at length) and that `alertpeerfloor` defaults to 0 on signet (only regtest); removed a stale doc paragraph describing a field that no longer exists and a lag comment whose stated reason was wrong. Tests: `a_reorg_is_reported_even_when_the_clock_steps_backwards` is control-verified — reinstating the watermark makes it fail on the missing second event. Plus rescan-idempotence and startup-seeding coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
95cb804 to
0697556
Compare
a8f72d2 to
ec24ff3
Compare
0697556 to
3a840e1
Compare
ec24ff3 to
5bdcdab
Compare
3a840e1 to
39b9f66
Compare
5bdcdab to
e5b4d34
Compare
39b9f66 to
05be291
Compare
e5b4d34 to
cf5063b
Compare
05be291 to
7b9cfd5
Compare
cf5063b to
304bec1
Compare
7b9cfd5 to
5dc98aa
Compare
304bec1 to
c43ecc8
Compare
… of artifacts Deep-review fixes for the health detectors, plus a repo-hygiene P0 that this branch introduced. **831 MiB of build artifacts were committed here and pushed.** Commit 04cb6da ("read reorg depth from the log") was meant to touch four files; a stray `git add -A` swept in 2201 files under `contrib/push-relay/target/` — a 79 MB debug binary, 28 MB of rlibs, and `.rustc_info.json` recording local toolchain paths. They were unambiguously accidental: the push-relay *source* does not exist until four PRs later, so this branch carried a compiled binary for a crate it did not contain. Nothing built or linted those paths, so CI stayed green. satd squash-merges, so this never entered master's history — but the squashed *tree* would have, permanently, in a public repo. `git rm -r --cached` here, and the `/contrib/push-relay/target` ignore rule moves down from #486 to sit ahead of the crate it protects, since .gitignore cannot untrack what is already tracked. **`deep_reorg` was gated on a wall-clock high-water mark, so a backwards clock step silenced it entirely.** `ReorgWatermark` seeded `SystemTime::now()` at task start and dropped any record stamped earlier. `ReorgRecord::ts_unix_secs` is also `SystemTime::now()`, so one NTP correction of a fast RTC after boot — or a hypervisor resync after live migration, or `date -s` — meant every reorg for the next N minutes was discarded. The watermark never reset and `deep_reorg` is an edge, so those alerts were not delayed; they were gone. Replaced with `ReorgSeen`, a bounded set of `(ts, old_tip, new_tip)` identities that depends on no clock. It is seeded from the log at startup, which also removes the reason the watermark existed. **The 300 s lookback was a second way to lose an edge forever.** Any delay of this task past the window — API-runtime saturation, a `statvfs` on a hung mount, a VM pause — dropped older reorgs out of view permanently, with no catch-up path. The window bought nothing now that de-duplication is exact, so the scan covers the whole ring (≤256 records). **`statvfs` now runs on `spawn_blocking`.** `disk_watch_path` defaults to `blocksdir`, which operators routinely point at NFS/iSCSI, and `statvfs` on a hung mount blocks uninterruptibly. Inline, it parked an API worker and froze *every* detector — including the two above — for as long as the mount stayed wedged. **The shutdown branch could spin at 100% CPU.** `_ = shutdown.changed() => if *shutdown.borrow() { return }` never returns once the last sender drops: `changed()` then fails immediately and forever while `borrow()` still reads false. Latent in the current wiring, one refactor from real; every other shutdown handler in the tree returns unconditionally, and now so does this one. Also: `disk_low` logged an identical WARN every poll (5,760/day while the condition holds) — now edge-only; the config reference claimed `tip_stall` is IBD-suppressed (the code deliberately does not suppress it, and says why at length) and that `alertpeerfloor` defaults to 0 on signet (only regtest); removed a stale doc paragraph describing a field that no longer exists and a lag comment whose stated reason was wrong. Tests: `a_reorg_is_reported_even_when_the_clock_steps_backwards` is control-verified — reinstating the watermark makes it fail on the missing second event. Plus rescan-idempotence and startup-seeding coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
5dc98aa to
68cd422
Compare
c43ecc8 to
a78421a
Compare
Tier 1 fix + the full deferral list for the stackFinding 1 — header-phase slowloris (MEDIUM). Fixed in
Body-phase slowloris was already covered; the header phase, which
Deferred findings across the whole stackRecorded here so the decisions survive the merge. Everything Tier 1 and Tier 2 is fixed in-branch; these are what is left.
Decided, not a defect: |
|
Deferred items are now filed as issues, so they survive the merge:
Not filed: the vacuous Also not filed: |
… of artifacts Deep-review fixes for the health detectors, plus a repo-hygiene P0 that this branch introduced. **831 MiB of build artifacts were committed here and pushed.** Commit 04cb6da ("read reorg depth from the log") was meant to touch four files; a stray `git add -A` swept in 2201 files under `contrib/push-relay/target/` — a 79 MB debug binary, 28 MB of rlibs, and `.rustc_info.json` recording local toolchain paths. They were unambiguously accidental: the push-relay *source* does not exist until four PRs later, so this branch carried a compiled binary for a crate it did not contain. Nothing built or linted those paths, so CI stayed green. satd squash-merges, so this never entered master's history — but the squashed *tree* would have, permanently, in a public repo. `git rm -r --cached` here, and the `/contrib/push-relay/target` ignore rule moves down from #486 to sit ahead of the crate it protects, since .gitignore cannot untrack what is already tracked. **`deep_reorg` was gated on a wall-clock high-water mark, so a backwards clock step silenced it entirely.** `ReorgWatermark` seeded `SystemTime::now()` at task start and dropped any record stamped earlier. `ReorgRecord::ts_unix_secs` is also `SystemTime::now()`, so one NTP correction of a fast RTC after boot — or a hypervisor resync after live migration, or `date -s` — meant every reorg for the next N minutes was discarded. The watermark never reset and `deep_reorg` is an edge, so those alerts were not delayed; they were gone. Replaced with `ReorgSeen`, a bounded set of `(ts, old_tip, new_tip)` identities that depends on no clock. It is seeded from the log at startup, which also removes the reason the watermark existed. **The 300 s lookback was a second way to lose an edge forever.** Any delay of this task past the window — API-runtime saturation, a `statvfs` on a hung mount, a VM pause — dropped older reorgs out of view permanently, with no catch-up path. The window bought nothing now that de-duplication is exact, so the scan covers the whole ring (≤256 records). **`statvfs` now runs on `spawn_blocking`.** `disk_watch_path` defaults to `blocksdir`, which operators routinely point at NFS/iSCSI, and `statvfs` on a hung mount blocks uninterruptibly. Inline, it parked an API worker and froze *every* detector — including the two above — for as long as the mount stayed wedged. **The shutdown branch could spin at 100% CPU.** `_ = shutdown.changed() => if *shutdown.borrow() { return }` never returns once the last sender drops: `changed()` then fails immediately and forever while `borrow()` still reads false. Latent in the current wiring, one refactor from real; every other shutdown handler in the tree returns unconditionally, and now so does this one. Also: `disk_low` logged an identical WARN every poll (5,760/day while the condition holds) — now edge-only; the config reference claimed `tip_stall` is IBD-suppressed (the code deliberately does not suppress it, and says why at length) and that `alertpeerfloor` defaults to 0 on signet (only regtest); removed a stale doc paragraph describing a field that no longer exists and a lag comment whose stated reason was wrong. Tests: `a_reorg_is_reported_even_when_the_clock_steps_backwards` is control-verified — reinstating the watermark makes it fail on the missing second event. Plus rescan-idempotence and startup-seeding coverage. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
* alerting: node-health detectors, warnings mirroring, and gauges
PR 2 of the A3 alerting stack (SATD_ALERTING_DESIGN.md §3). PR 1 added the
StatusEvent wire schema with no emitters; this adds the detectors.
satd now watches six conditions about itself — ibd_complete, tip_stall,
disk_low, mempool_congested, peer_floor, deep_reorg — and reports each
through three surfaces simultaneously: a `status` streaming event, an entry
in `getwarnings` (which fires the Core-compatible `alertnotify` hook), and a
`satd_alert_active{kind}` gauge. All three are driven from one state
machine, so they cannot disagree about node state.
Design points:
- Level-triggered, not edge-spammed. Every standing condition raises once
on entry and clears once on recovery, with a fixed hysteresis gap between
the two lines (a ratio, a hold time, or both) — clearing at the same value
that raises turns a metric hovering at the threshold into a pager storm.
ibd_complete and deep_reorg describe things that happened rather than
states that persist, so they are one-shot edges with no clear.
- Durability by re-evaluation. Status events are not replayable, so what
makes health alerting at-least-once across a restart is that the task
re-evaluates from scratch and re-raises anything still standing.
- Runs on the API runtime, never the consensus core: a stall detector whose
poll can be delayed by block connection is the one thing it must not be.
- Thresholds are hot-reloadable. An operator retuning an alert is usually
doing it *because* it is firing; taking the node down to silence a pager
is the wrong trade. Disabling a threshold also clears a standing
condition rather than stranding a warning nothing will retract.
- deep_reorg reports the *true* depth. ChainEvent::Reorg carries the old and
new tip heights but not the fork point, so the detector counts the reorg's
disconnects — exactly `old_height - fork_height` — rather than reporting
the threshold it crossed. A truncation reorg (invalidateblock) has no
replacement chain to connect, so a count that goes quiet is finalized by
the poll loop instead of waiting for a connect that may never come.
- Only conditions worth attention become warnings: `ibd_complete` is info
and would otherwise sit in getwarnings forever with nothing to clear it.
Also generalizes `free_disk_bytes` out of the three index runners that had
each grown a private copy, and adds `satd_tip_last_connect_age_seconds` /
`satd_disk_free_bytes` — disk headroom previously had no metric or alerting
at all, which is how a silent disk-fill wedged a dogfood node in May.
Config: alerttipstallseconds=3600, alertdiskfreemb=10240,
alertmempoolfullpct=90, alertpeerfloor=3, alertreorgdepth=3; 0 disables.
E2E coverage over a real socket for tip_stall raise+clear, disk_low via a
live SIGHUP threshold change, deep_reorg's derived depth, and the
explicit-only category guarantee.
* alerting: node-health detectors, warnings mirroring, and gauges
Round-3 review fixes folded in:
- `disk_low` and `mempool_congested` repeated the dead-band bug already fixed
for `tip_stall`: both clear only on a hysteresis-widened predicate evaluated
against the *current* threshold, so retuning a threshold to quiet a firing
alert leaves the unchanged reading between the new raise and clear lines,
where neither branch runs. For `mempool_congested` this was inescapable —
`alertmempoolfullpct` clamps at 100 and the clear line is 0.75x the raise
line, so past 75% occupancy no setting could clear it. A condition now
records the threshold it was raised against and clears when that threshold
moves such that it would no longer raise. The anti-flap gap still governs a
reading that recovers on its own.
- The `tip_stall` IBD guard was unlatched. `is_initial_block_download()` is a
function of wall-clock time against the tip header, so a node whose tip stops
advancing crosses back into "IBD" 24h later and freezes the detector: every
later poll returns before reaching the raise or clear branch, and a SIGHUP
retune applies to the atomic and does nothing. Latch it — once caught up, a
node is never in IBD again for this detector.
- `deep_reorg` abandoned its in-flight count on broadcast lag. The chain-event
ring holds 64 entries and a reorg emits one event per disconnected and
reconnected block, so lag becomes likely right around the depth where the
alert starts to matter — making it least reliable for the largest reorgs.
Fall back to the committed reorg-log record, which has the true fork height.
- `-alertnotify` fired only for the *first* `deep_reorg` of a process: the
warning never clears, so the first-time-only dedup suppressed every later and
possibly much deeper one, while the streaming and webhook surfaces reported
all of them. Edge observations now notify on every occurrence
(`record_recurring`); standing conditions keep the dedup.
- `check_disk` tested `statvfs` before the `floor == 0` disabled-clear, so an
unmounted volume left a raised alert unclearable by any means including the
documented escape hatch. `check_mempool` returned on `maxmempool=0` without
clearing, stranding a raised alert the same way.
- `satd_alert_active` emitted a `# HELP`/`# TYPE` pair per series rather than
per family. That is invalid text format, and strict parsers (`promtool check
metrics`, `expfmt.TextParser`) reject the *entire page* on the duplicate,
taking every unrelated satd metric with it. Split `metric` into a header and
a sample writer, and add a test that asserts one header per family.
- `disk_low` carried the absolute watched path as a wire detail, so it reached
every `status` subscriber and any push-notification body. It goes to the log
instead.
- `alertpeerfloor` defaulted to 3 with no startup grace, so any node with fewer
than 3 peers raised a permanent critical warning 60s in — including every
regtest and single-node dev deployment. Default it to 0 on regtest and
signet, and on the other networks hold off raising until 90s after startup or
the first peer, whichever comes first.
Each behavioral fix has a test verified to fail without it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
* alerting: fix the health detectors' own review-round regressions
Round 4 of external review, scoped to the ~594 lines this branch added
during round 3 — code written as fixes and never itself reviewed.
`clear_if_threshold_relaxed` defeated the hysteresis it was written to
protect. The remembered threshold was stored only on the raise edge, so
retuning *down* while a condition stood left the slot stale, and the next
reading inside the hysteresis band read as "the operator moved the line":
raise at 93% against 90%, retune to 80%, ease to 78% — above the new clear
line of 60% — and the alert cleared, then re-raised on the next block. Now
refreshed on every evaluation where the raise predicate holds.
The Lagged reorg-log fallback rested on a false ordering claim. Its comment
said the log is written before any chain event is emitted; `perform_reorg`
does the reverse, and `ReorgLog::record` fsyncs before pushing to the ring
`history()` reads. So the record was typically absent at that instant, and
the newest one present belonged to an earlier reorg — reported with that
reorg's depth and fork point, after which state went Idle and the real reorg
was never reported. Now deferred a poll and matched on the abandoned tip
height from the marker; unmatched counts report as a lower bound rather than
vanishing, marked `depth_exact: false`.
The `floor == 0` return moved above the `statvfs` call, so
`satd_disk_free_bytes` was never populated with the alert disabled — losing
the gauge for operators who set `alertdiskfreemb=0` precisely because they
alert on it in Prometheus. Sample first, then decide.
The `threshold == 0` return sat above the `left_ibd` latch, so a node running
with `tip_stall` off never latched; enabling it on an already-wedged node
(tip >24h stale, hence "in IBD" again) wedged the detector permanently — the
exact failure the latch was added to prevent.
The peer startup grace ran concurrently with the hold rather than deferring
it, so it merely moved the raise to t=90s; and latching `saw_first_peer`
mid-grace fired the alert in the very poll the first peer arrived. The hold
now starts when the grace ends or the first peer lands.
`maxmempool=0` reported `reason=detector_disabled`, which means "the operator
turned this off" — a consumer suppressing that would swallow a zeroed mempool
cap. Now `mempool_cap_zero`.
Signet's peer floor goes back to 3. Signet is a public network with real
peers; a detector defaulted off is indistinguishable from a healthy one, as
`satd_alert_active{kind="peer_floor"}` reads 0 either way. Regtest keeps 0.
Tests: `check_disk` and `check_peers` and `check_tip_stall` are split into
value-taking halves, mirroring `check_mempool_values`, and the test-local
`sample_disk` reimplementation is gone — it mirrored the detector's branch
structure, so deleting the real `clear_if_threshold_relaxed` arm left every
disk test green. Six new tests, each verified to fail against the code it
guards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
* alerting: read reorg depth from the log, not from counting events
`deep_reorg` reconstructed depth by counting `BlockDisconnected` events off
the chain broadcast, through a three-state machine with a lag-recovery
path. The design says otherwise — "depth from `ReorgRecord`" — and the
design is right: `perform_reorg` already writes the exact fork height,
depth, and reconnected chain, and fsyncs the record before pushing it to
the ring `history()` reads. Counting reconstructs by hand a number the node
recorded exactly, and gets it wrong in ways that all bite hardest at depth:
- The bus ring holds 64 entries and a reorg of depth D emits `2D + 2`
events in one await-free burst, so a deep reorg truncates its own count —
or drops the `Reorg` marker, after which the tracker sat in `Idle` and
discarded the whole reorg silently.
- The lag fallback searched the log with `.find()`, and `history()` returns
oldest-first. Two reorgs abandoning the same tip within the 300 s window
— an ordinary tip race, or back-to-back `invalidateblock` — reported the
earlier one's depth, which below threshold meant no event at all.
- `to_height` came from the first `BlockConnected` after the disconnect
run. Reconnects are emitted oldest-first, so every reorg with a
replacement chain reported `fork_height + 1` — *below* the old tip.
- A second reorg arriving mid-count overwrote the first, losing it.
Replaced with a watermark over the log: each poll reports records not seen
before, keyed on `(ts, old_tip)` because two reorgs can share a second. The
watermark is seeded from the clock at startup, since `deep_reorg` is an
edge event and D3 re-raises standing conditions across a restart, not
edges. `depth_exact` is gone — there is no inexact path left.
Separately, `tip_stall` no longer suppresses on IBD. The suppression was
latched against the predicate flapping, but the latch is per-process and a
restart is an operator's first move during a stall: a node restarted while
already wedged never observes a non-IBD tip, so the latch never armed and
the detector went silent permanently. `age_secs` already encodes the right
signal — a node that is really syncing connects blocks and stays under the
threshold on its own.
Seven new tests, each with its negative control verified: reverting the fix
fails the test.
* alerting: unbreak the reorg detector's clock dependence; drop 831 MiB of artifacts
Deep-review fixes for the health detectors, plus a repo-hygiene P0 that this
branch introduced.
**831 MiB of build artifacts were committed here and pushed.** Commit 04cb6da
("read reorg depth from the log") was meant to touch four files; a stray
`git add -A` swept in 2201 files under `contrib/push-relay/target/` — a 79 MB
debug binary, 28 MB of rlibs, and `.rustc_info.json` recording local toolchain
paths. They were unambiguously accidental: the push-relay *source* does not
exist until four PRs later, so this branch carried a compiled binary for a crate
it did not contain. Nothing built or linted those paths, so CI stayed green.
satd squash-merges, so this never entered master's history — but the squashed
*tree* would have, permanently, in a public repo. `git rm -r --cached` here, and
the `/contrib/push-relay/target` ignore rule moves down from #486 to sit ahead
of the crate it protects, since .gitignore cannot untrack what is already
tracked.
**`deep_reorg` was gated on a wall-clock high-water mark, so a backwards clock
step silenced it entirely.** `ReorgWatermark` seeded `SystemTime::now()` at task
start and dropped any record stamped earlier. `ReorgRecord::ts_unix_secs` is
also `SystemTime::now()`, so one NTP correction of a fast RTC after boot — or a
hypervisor resync after live migration, or `date -s` — meant every reorg for the
next N minutes was discarded. The watermark never reset and `deep_reorg` is an
edge, so those alerts were not delayed; they were gone. Replaced with
`ReorgSeen`, a bounded set of `(ts, old_tip, new_tip)` identities that depends on
no clock. It is seeded from the log at startup, which also removes the reason
the watermark existed.
**The 300 s lookback was a second way to lose an edge forever.** Any delay of
this task past the window — API-runtime saturation, a `statvfs` on a hung mount,
a VM pause — dropped older reorgs out of view permanently, with no catch-up
path. The window bought nothing now that de-duplication is exact, so the scan
covers the whole ring (≤256 records).
**`statvfs` now runs on `spawn_blocking`.** `disk_watch_path` defaults to
`blocksdir`, which operators routinely point at NFS/iSCSI, and `statvfs` on a
hung mount blocks uninterruptibly. Inline, it parked an API worker and froze
*every* detector — including the two above — for as long as the mount stayed
wedged.
**The shutdown branch could spin at 100% CPU.** `_ = shutdown.changed() => if
*shutdown.borrow() { return }` never returns once the last sender drops:
`changed()` then fails immediately and forever while `borrow()` still reads
false. Latent in the current wiring, one refactor from real; every other
shutdown handler in the tree returns unconditionally, and now so does this one.
Also: `disk_low` logged an identical WARN every poll (5,760/day while the
condition holds) — now edge-only; the config reference claimed `tip_stall` is
IBD-suppressed (the code deliberately does not suppress it, and says why at
length) and that `alertpeerfloor` defaults to 0 on signet (only regtest);
removed a stale doc paragraph describing a field that no longer exists and a lag
comment whose stated reason was wrong.
Tests: `a_reorg_is_reported_even_when_the_clock_steps_backwards` is
control-verified — reinstating the watermark makes it fail on the missing second
event. Plus rescan-idempotence and startup-seeding coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
* alerting: a deep reorg is an event, not a standing error
A reorg several blocks deep is a routine part of signet and testnet4, not an
incident, and satd was treating it as a permanent error state.
`emit()` mirrored every edge observation into `NodeWarnings`, and
`StatusKind::DeepReorg.severity()` is `Critical`, which maps to
`Severity::Error`. Nothing ever calls `clear` for an event that has no resolved
state — by design, "it happened, and nothing un-happens it" — so the first reorg
at or past `alertreorgdepth` (default 3) put a permanent `Error` in the
registry. From that moment `has_errors()` was true for the life of the process,
`getblockchaininfo.warnings` was never empty again, and the TUI held its
blocking modal open. On a chain where 3-deep reorgs are ordinary that is the
steady state within minutes of starting, and the operator has no way to
acknowledge or clear it.
This contradicts the warnings module's own contract, stated at the top of
`warnings.rs`: warnings "represent *current* state", "history-style events
(reorgs, fee-estimate windows, etc.) have their own persistent logs", and "every
emitted warning indicates a bug that should be fixed". A deep reorg is history,
it has its own persistent log (`ReorgLog` / `getreorghistory`), and it is not a
bug. It is also the exact failure the detectors avoid elsewhere — the
`peer_floor` default is network-conditional specifically so regtest does not
carry "a critical warning that can never clear".
Edge kinds now fire `-alertnotify` without recording anything, via a new
`NodeWarnings::notify_event`. The page still happens on **every** occurrence
(each reorg is a distinct event, not a restatement of one condition), and the
`status` event on the streaming API and webhooks is unchanged — including its
`critical` severity, so a `min_severity = "critical"` hook still receives it.
What goes away is only the standing entry that nothing could clear.
`record_recurring` existed solely to serve this one call site — it opted an id
out of the first-time-only `alertnotify` dedup *because* its warning never
cleared — so it is deleted rather than left as a trap.
Tests are control-verified: reinstating the recording behavior makes
`notify_event_fires_the_hook_without_recording_a_warning` fail on
`count() == 0` (left: 1, right: 0) and `every_occurrence_of_an_event_pages` fail
on the second occurrence.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
* alerting: alertreorgdepth defaults by network, like the peer floor
Depth 3 means completely different things on different chains, and the previous
default paged on all of them.
On mainnet a 3-block reorg costs real hashrate and invalidates transactions
merchants have begun treating as settled; waking someone is right. Signet,
testnet and testnet4 are not economically secured, and reorgs a few blocks deep
are an ordinary consequence of thin, volatile hashrate — so the default ran
`-alertnotify` for the network behaving exactly as designed. An alert that fires
during normal operation is one operators learn to ignore, and ignoring it costs
them the mainnet alert too, which is the one that mattered.
`reorg_depth_for(network)` mirrors the `peer_floor_for(network)` precedent
immediately above it, for the same reason it exists:
- mainnet `3`
- signet / testnet / testnet4 `10` — **raised, not disabled.** Past the
6-confirmation convention a wallet has been told something false, and that is
worth reporting on any chain. Disabling would also make the detector's silence
indistinguishable from health, which is the argument `peer_floor_for` already
makes for not defaulting signet off.
- regtest `0` — its test suites reorg deliberately and constantly;
`invalidateblock` and competing-chain tests are the point of the harness.
- an unrecognized future network takes the test-network value: new networks are
overwhelmingly test networks, and guessing that way fails mild.
This pairs with the previous commit. That one stopped a deep reorg becoming a
permanent `getwarnings` entry; this one stops it paging on chains where it is
routine. Together: on a test network an ordinary reorg now does nothing but emit
a `status` event that subscribers can filter, and a genuinely anomalous one
still reports everywhere.
`health_alert_threshold_defaults_and_parsing` caught the regtest change on its
own and is updated to assert the new intent across all three cases.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
* alerting: the peer floor follows -connect, not only the network
`-connect=` pins the node to exactly the addresses named and suppresses
both DNS seeding and the fixed seeds (satd/src/main.rs), so a node given
one or two upstreams can never reach the stock floor of 3. Nothing on the
node would add a peer, so the raise stood forever: in `getwarnings`, in
`-alertnotify`, in the TUI modal, and in `getblockchaininfo.warnings` —
which wallet software renders to end users. `satd_alert_active{kind=
"peer_floor"}` pinned high stops distinguishing peer-starved from
configured-that-way, so the detector carried no signal on exactly the
nodes it was loudest about.
This is the regtest trap in different clothes, and `peer_floor_for` was
already written against it — it just keyed off the network when the
condition it describes is a property of the configuration. So the default
now takes the `-connect` count too, capped at the stock floor.
Capping rather than disabling keeps the alert doing the one useful thing
it still can here: reporting a `-connect=` node that had all its
configured peers and then lost one. An explicit `alertpeerfloor`
overrides in either direction, including to 0.
The default had to move out of the `Config` struct literal, which has
already moved `connect` by the time it reaches that field.
Both new tests were run against the previous behavior and fail there.
* alerting: tip stall is off on regtest, like the floor and the depth
Regtest blocks exist only when something calls `generatetoaddress`, so an
idle chain is its resting state rather than a stall. `last_connect` is
seeded at detector start and advanced only by `BlockConnected`, so a node
left running for an hour between test runs raises `tip_stall` — which is
`Critical`, so it pins `getwarnings`, holds `has_errors()` true, and puts
up the TUI's blocking modal, on a chain doing exactly what it should.
This is the third threshold in the set to need it and the last one that
did not have it; `peer_floor_for` and `reorg_depth_for` already turned
regtest off for the same reason, and `peer_floor_for`'s doc comment calls
it "a poor greeting for every developer's first run".
Every other network keeps the hour, test networks included. Unlike a
reorg a few blocks deep, going an hour without a block is not an ordinary
property of thin hashrate — signet and testnet4 are still expected to
make blocks, so relaxing them the way `reorg_depth_for` does would be
answering a different question.
The mainnet raise rate is left alone: at a 600s mean interval,
P(gap > 3600s) = e^-6, so a healthy node raises about every 2.8 days.
That is documented on TIP_STALL_SECS as a deliberate trade and it stays
the operator's to tune.
The regtest assertion in `health_alert_threshold_defaults_and_parsing`
previously asserted 3600 and passed, so it is a direct negative check on
the new value.
* alerting: a wedged filesystem must not take down the whole detector loop
`statvfs` on a hard NFS mount whose server has gone away does not
return. `spawn_blocking(..).await` moves the syscall off the detector's
thread but still parks the detector on the JoinHandle, so tip_stall,
deep_reorg, check_mempool and check_peers stopped running, `chain_rx`
stopped being drained, and every gauge froze at its last value — meaning
an external Prometheus rule on tip age could not fire either. A wedged
mount silently disabled the entire alerting subsystem, which is the one
condition alerting exists for. The comment claimed spawn_blocking gave
isolation it does not: it spares the API worker, not the detectors.
A timeout around the join is not enough on its own. `timeout` abandons
the handle but cannot cancel a blocking task, so each poll would strand
another thread and exhaust the bounded blocking pool within hours —
trading a wedged detector for a wedged runtime.
So the probe is now bounded AND carried across polls: each tick awaits
the outstanding handle by `&mut` reference for a 2s budget (well under
the 15s interval, so a filesystem that answers at all is read inline) and
otherwise keeps it for the next tick. One unresponsive mount costs
exactly one stuck thread, forever, and leaves disk_low on its last known
verdict rather than clearing it — an unresponsive mount is not evidence
the disk drained. Logged loudly once, then hourly.
The budget is a parameter so the regression test can use 1ms; the
detector loop passes DISK_PROBE_BUDGET.
Also corrects the three places that said tip_stall is gated on IBD —
the release notes asserted both readings in consecutive sentences, and
config.rs's field doc and CLI help said "outside IBD".
`check_tip_stall_values` deliberately ignores IBD and explains why.
The wedge test was run against the inline await and fails there with
"check_disk must not block on an unresponsive filesystem: Elapsed(())".
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
68cd422 to
7758ebd
Compare
Round-3 review fixes folded in:
- **APNs could not have worked at all.** `reqwest` was declared with
`default-features = false` and never re-added `http2`, which is a default
feature and is not implied by `json` or `rustls-tls` — the committed lockfile
had no `h2` at all, so the client offered only HTTP/1.1 in ALPN. Apple's
provider API is HTTP/2-only. The flagship use case, an iOS lock-screen alert,
would have failed at the transport layer on every push, with nothing but a
generic error in the log. No test could catch it: the push tests assert
payload shape and never open a socket.
- **The reorg notification silently lost its height, and the test asserted a
body satd cannot produce.** `ChainEvent::Reorg` carries `from_height` /
`to_height`, not `height`, so the decoder's `height` was `None` on every real
reorg and the push degraded to "The active chain tip changed." The unit test
fed a hand-written `{"kind":"reorg","height":812345}` fixture — the relay's own
invention — and asserted the height appeared, staying green over a broken
path. Decoder fixed; the test now uses satd's real body shape.
- **v2 signature verification** (pairs with the dispatcher and spec commits):
the HMAC now covers the timestamp, delivery id, and hook id as well as the
body, and a delivery outside a 600-second freshness window is refused. This is
what makes the relay's existing dedup safe: previously the delivery id it
keys on was unsigned and predictable, so anyone holding one captured delivery
could replay it under forged future ids and pre-fill the dedup ring, causing
the genuine alerts to be discarded on arrival. The published spec vectors are
updated and still asserted here by an implementation independent of
`satd-alert`.
- **`relay.toml` is now permission-checked (0600)**, matching what satd does for
its own alertfile. It holds the same HMAC secret plus the paths to the APNs
`.p8` and the FCM service-account JSON, and the README told operators to `cp`
the example — which lands at 0644 under a normal umask. The README now says
`chmod 600`.
- **Unknown config keys are refused** (`deny_unknown_fields`). A typo'd
`producton = true` silently left `production` at `false`, so the relay talked
to the APNs *sandbox* and no notification ever reached a real device, with
nothing anywhere explaining why. This matches the alertfile parser's stated
posture two directories away: an accepted-but-ignored rule is worse than a
refused one.
- CI now passes `--locked`, so the committed lockfile is actually proven to
build rather than being silently bypassed by a fresh resolve.
- README: `min_severity` is a status floor only — reorg and gap notifications
bypass it — and the reorg row describes the from/to heights it now reports.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
satd no longer sends a `lagged` body, so the relay's Lagged variant and its notification were dead. An unknown body already decodes to `Other` and produces nothing, which is the right behaviour and is now what the test asserts. The README said the relay pushes an "Alert delivery gap" notification. It cannot: a receiver has no way to know what it was never sent. Drops are the node's `satd_alertwebhook_dropped_total` counter, and that is where an operator should alert on them.
This is reference code published to be forked into wallet vendors' production systems, so a weakness here propagates. **No server-side timeouts, body limit, or connection cap.** `axum::serve` sets none of its own, and the `Bytes` extractor must buffer the whole body *before* the signature can be checked — so both of these were reachable by an unauthenticated peer. `POST /hook` plus one header byte per minute held a connection and its task forever (nothing reaped it); and axum's implicit 2 MB default meant N parked connections cost up to 2 MB of resident heap each before a byte of HMAC ran. An OOM-killed relay is a dark alerting channel, which is the state an attacker about to do something noisy wants it in. Now: an explicit 64 KB body limit (an alert envelope is a few hundred bytes, and satd caps `message` and every `details` value), a 15 s whole-request timeout, and a 64-request concurrency cap. **A provider token was minted per delivery.** Apple documents at most one provider token per 20 minutes and answers `429 TooManyProviderTokenUpdates` beyond that — and a flapping condition produces raise/clear pairs minutes apart, so four alerts in an hour trips it. The relay logged the 429 as "device token stale or credentials wrong" and returned Ok, having already ACKed to satd, so push stopped exactly during the incident generating the alerts and the log blamed the wrong thing. FCM was worse per delivery: a full RSA-signed assertion *and* a round trip to `oauth2.googleapis.com` in the alert path, discarding the `expires_in` it was handed. Both tokens are now cached and refreshed with margin, which also removes two blocking `std::fs` reads from inside `async fn`s. **One unreachable device starved the rest.** `?` on `.send()` inside the per-device loop meant a transport error or 10 s timeout on device #1 returned from the function and devices #2..N never got the notification. Non-2xx *responses* were already handled correctly; transport errors were not. Both providers now log and continue, and distinguish provider throttling/5xx from a stale device token. **The secret could reach stderr, and the perms check guarded the wrong file.** `toml::de::Error`'s Display quotes the offending source line verbatim, and the line most likely to be malformed here is `satd_secret = "..."`; `main` prints the anyhow chain, so a botched hand-edit put the shared HMAC key in the journal. And `check_perms` covered `relay.toml` but not the APNs `.p8` or the FCM service-account JSON it points at — a world-readable `.p8` lets any local user mint provider tokens for the operator's app. Both credentials are now checked too. `Config`'s derived `Debug` printed the secret in the clear; it is hand-written and redacting, because the first thing a forker adds is `tracing::debug!(?cfg)`. **The config load had a TOCTOU the doc comment denied.** It checked permissions on an open handle — correctly — and then re-opened the path to read the contents, so the file validated and the file used could differ. The comment claimed "it cannot be raced". It now reads from the same handle. Also: SIGTERM is handled (under systemd `systemctl restart` sends SIGTERM, and the relay only waited on Ctrl-C, so it died mid-request dropping pushes it had already acknowledged); `apns-collapse-id` is truncated to Apple's 64-byte limit, past which the *whole request* fails rather than degrading; and three docs claimed the relay turns delivery gaps into notifications — satd does not send gap notices, and a receiver cannot know what it was never sent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH
`with_graceful_shutdown` waits for in-flight *requests*, and a push is
deliberately not one: the handler acknowledges satd first and delivers on a
detached `tokio::spawn`, so the node's serial per-hook queue does not sit
behind Apple's and Google's latency. That means returning from `serve`
dropped the runtime and aborted those tasks at their await points.
So `systemctl restart` during a `critical` push left satd holding a 200,
the delivery id in its dedup ring, no retry coming, and nobody paged —
precisely the outcome the SIGTERM handler's own comment says it prevents
("dropping any push it had already acknowledged to satd").
Count pushes from acknowledgement to completion and wait for the count to
reach zero after `serve` returns, bounded by SHUTDOWN_DRAIN (15 s, below
systemd's default TimeoutStopSec) and warning if it expires with work
outstanding. The guard is a `Drop` type so an early return inside the task
still decrements.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GJqZbJtTUo7K7G9wJcrvHv
`tower_http`'s TimeoutLayer wraps the axum service, and hyper invokes that service only after it has parsed the request head. So a peer that opens a connection and dribbles one header byte per minute passes through no layer at all: not the timeout, not DefaultBodyLimit, not MAX_CONCURRENT_REQUESTS. It just holds a task and a file descriptor. N of them exhaust both, satd's real deliveries start being refused, and the alerts this relay exists to deliver stop arriving — silently, since the node never receives a response it could retry. Body-phase slowloris was already covered. The header phase, which the constant's own comment named as the case it handled, was not. `axum::serve` exposes no header-read timeout, so serving now runs the accept loop over `hyper_util`'s auto builder with `http1().header_read_timeout(..)`. Everything else — graceful shutdown via `GracefulShutdown`, a task per connection, the same app — is what `axum::serve` was doing. REQUEST_TIMEOUT keeps its job (body + handler) and its doc now says so instead of claiming headers too. Shutdown is bounded by the same SHUTDOWN_DRAIN the push drain uses, so a stuck connection cannot hold the process past what systemd waits for. Two tests: a half-sent request must be hung up on, and a complete one must still be served (so the deadline is not just closing everything). The first was run with the deadline effectively disabled and fails there with "the server kept a half-sent request open past the header deadline".
a78421a to
b8ed2df
Compare
The parallel piece of the A3 alerting stack (
SATD_ALERTING_DESIGN.md§5, decision D9). Based on #485 so it carries the whole stack's context, but it touches onlycontrib/push-relay/, the workspaceexcludelist, one new path-gated workflow, and docs — nothing in the node.Not release-gated (like the Go SDK).
What
A ~600-line service that receives satd's alert webhooks and forwards the ones worth waking someone for as APNs / FCM push notifications, using the operator's own Apple and Google credentials.
Why it's outside the workspace
That's the design point, not a packaging detail: a Bitcoin node has no business holding a push-provider credential, and the JWT/OAuth stack that comes with one has no business in its dependency tree or its
cargo-denysurface.Cargo.tomlgains it toexclude, and its CI is path-gated so it neither gates node changes nor rebuilds on them.It's reference-grade, and says so
Device registration and per-user routing are wallet-vendor concerns and don't belong in an example. Three things in the receive path are worth copying verbatim, and are commented as such:
X-Satd-Delivery. satd delivers at-least-once, so the same id arrives again whenever a response is lost after you acted on it.Mapping choices
Otherrather than failing — satd adds these additively, and a decode error would take down alerting exactly when it's needed.Acceptance (design §10, relay)
docs/api/webhooks.md, asserted here by an implementation independent of satd's. Two implementations agreeing is the point of publishing them.🤖 Generated with Claude Code
https://claude.ai/code/session_018AfAm3VVN8gBeYcTL5BJqH