Skip to content

Add panic supervision for the keeper and reconciliation background loops - #66

Merged
abayomicornelius merged 12 commits into
StellarSend:mainfrom
Smoothjane:fix/keeper-panic-supervision-50
Aug 18, 2026
Merged

Add panic supervision for the keeper and reconciliation background loops#66
abayomicornelius merged 12 commits into
StellarSend:mainfrom
Smoothjane:fix/keeper-panic-supervision-50

Conversation

@Smoothjane

Copy link
Copy Markdown
Contributor

Summary

Both the subscription-keeper and batch-reconciliation background loops were launched with a bare tokio::spawn and never supervised — a single panic anywhere in either call graph (an unwrap(), an array index, or the companion chrono::Duration::seconds() overflow near i64::MAX on reconciliation_stale_after_secs, which is directly reachable in run_batch_reconciliation_loop) silently and permanently killed that loop for the rest of the process's lifetime, with no line in the structured tracing log stream — just an absent stderr backtrace most containerized/log-shipped deployments would never surface distinctly from noise.

Implemented both layers the issue asks for:

  • run_isolated_pass: wraps each loop's per-pass body (service construction + the actual call) in its own tokio::spawn, so a panic there surfaces as a JoinError the outer loop can log and continue past — the loop's own loop { interval.tick().await; ... } never panics itself.
  • supervise_loop: an outer, defense-in-depth layer — retains the JoinHandle (previously dropped entirely) and restarts the task with a capped exponential backoff (1s → 60s, resetting after a 30s+ healthy run) if it ever terminates for any reason, rather than vanishing.
  • BackgroundLoopHealth: tracks each loop's last-completed-tick unix timestamp (updated even when a pass panics — the loop is still alive), exposed via /health's background_loops field — ties into Keeper background job has no metrics, alerting, or observability into failure trends #25's observability scope as the issue's acceptance criteria suggests, without building the fuller metrics/alerting system that's explicitly Keeper background job has no metrics, alerting, or observability into failure trends #25's job, not this issue's.
  • Both loops' panics (and the recoverable-Err path, unchanged) now log through the same structured JSON tracing pipeline as every other error in the codebase, not a bare stderr backtrace.

Test plan

  • Unit tests for run_isolated_pass (success passes through; a panic is caught as a JoinError, not propagated to the caller)
  • Unit tests for supervise_loop (restarts after a panicking task; restarts after a task that returns normally instead of running forever)
  • An end-to-end-style test mirroring the real loops' exact tick/pass/match structure (not just the helper in isolation) — deliberately panics on one pass and asserts every subsequent tick still runs, per the issue's acceptance criteria
  • cargo test — 38/38 passing (11 pre-existing #[ignore]d db_tests require a live Postgres, matching this repo's existing convention — untouched by this change)
  • cargo check — clean

This repo's only CI workflow is cargo-deny (supply-chain/license audit) — no test/build/lint workflow exists. Ran it locally: it reports one pre-existing advisories failure (RUSTSEC-2026-0258, a transitive h2 vulnerability) that's unrelated to this change — Cargo.toml/Cargo.lock are untouched by this PR, and the same failure reproduces on a clean, unmodified upstream/main checkout.

Closes #50

Resolves RUSTSEC-2026-0258 (h2 unbounded empty DATA frames), which
CI's cargo-deny (advisories) job was failing on for this PR. reqwest
0.11 pulled h2 0.3.27 via the old hyper 0.14 line, and no patched
0.3.x release of h2 exists — the only fix is moving off that line
entirely. reqwest 0.12 uses hyper 1.x internally (already the version
this workspace's own axum/wiremock use), which resolves to the
already-patched h2 0.4.16.

This is the reqwest half of deny.toml's own already-documented
"Tracked follow-up: upgrade sqlx 0.7 -> 0.8 AND reqwest 0.11 -> 0.12"
— sqlx's independent, separate rustls 0.21 line is untouched, so the
four pre-existing ignored advisories tied to that line are correctly
still present and still ignored; only the h2 advisory this reqwest
bump actually fixes is removed. The bump also relicenses the pulled-in
webpki-roots (CA root bundle) from MPL-2.0 to CDLA-Permissive-2.0 for
new-enough versions, so deny.toml's license allowlist now covers both,
since the old sqlx-side webpki-roots@0.25.x is still MPL-2.0.

Verified: cargo test (38/38, unchanged), cargo deny check (advisories
ok, bans ok, licenses ok, sources ok).
@abayomicornelius

Copy link
Copy Markdown
Contributor

all ci passed

@abayomicornelius
abayomicornelius merged commit c0c11ba into StellarSend:main Aug 18, 2026
2 checks passed
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.

Keeper and reconciliation background loops have no panic supervision — one panic silently and permanently kills that loop

2 participants