Add panic supervision for the keeper and reconciliation background loops - #66
Merged
abayomicornelius merged 12 commits intoAug 18, 2026
Merged
Conversation
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).
Contributor
|
all ci passed |
3 tasks
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Both the subscription-keeper and batch-reconciliation background loops were launched with a bare
tokio::spawnand never supervised — a single panic anywhere in either call graph (anunwrap(), an array index, or the companionchrono::Duration::seconds()overflow neari64::MAXonreconciliation_stale_after_secs, which is directly reachable inrun_batch_reconciliation_loop) silently and permanently killed that loop for the rest of the process's lifetime, with no line in the structuredtracinglog 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 owntokio::spawn, so a panic there surfaces as aJoinErrorthe outer loop can log and continue past — the loop's ownloop { interval.tick().await; ... }never panics itself.supervise_loop: an outer, defense-in-depth layer — retains theJoinHandle(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'sbackground_loopsfield — 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.Errpath, unchanged) now log through the same structured JSONtracingpipeline as every other error in the codebase, not a bare stderr backtrace.Test plan
run_isolated_pass(success passes through; a panic is caught as aJoinError, not propagated to the caller)supervise_loop(restarts after a panicking task; restarts after a task that returns normally instead of running forever)cargo test— 38/38 passing (11 pre-existing#[ignore]ddb_testsrequire a live Postgres, matching this repo's existing convention — untouched by this change)cargo check— cleanThis 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-existingadvisoriesfailure (RUSTSEC-2026-0258, a transitiveh2vulnerability) that's unrelated to this change —Cargo.toml/Cargo.lockare untouched by this PR, and the same failure reproduces on a clean, unmodifiedupstream/maincheckout.Closes #50