From ae8e5ade3c781b8979c7efc9e845d76e3a9209e0 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Mon, 14 Sep 2026 14:55:12 -0700 Subject: [PATCH] fix(online): record decode POST reachability for the caller to classify MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OnlineSource now records the HTTP outcome of its actual /decode POST — a Status(code) for any answer (200/404/422/429/5xx) or Transport for a connect/timeout/DNS/TLS failure — in a per-thread slot, readable via the new take_last_decode_reachability() / DecodeReachability. This lets a caller (autorip) classify a no-key result from the REAL decode instead of firing a second, redundant probe against the POST-only endpoint. No change to key resolution itself. Targets 1.7.2. --- CHANGELOG.md | 6 +++ src/lib.rs | 5 +- src/online.rs | 124 +++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 133 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9fef0e..46edfac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## [1.7.2] — UNRELEASED + +### Added + +- `OnlineSource` now records the reachability outcome of its actual `/decode` POST (an HTTP `Status(code)` for any answer, including 200/404/422/429/5xx, or `Transport` for a connect/timeout/DNS/TLS failure) in a per-thread slot, readable via the new `take_last_decode_reachability()` / `DecodeReachability`. This lets a caller (autorip) classify a no-key result from the REAL decode's outcome instead of firing a second, redundant probe against the POST-only endpoint. No behavioural change to key resolution itself. + ## [1.7.1] — UNRELEASED ### Changed diff --git a/src/lib.rs b/src/lib.rs index 15a754f..ff9bcd4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -17,7 +17,10 @@ mod paths; pub use keydb::{KeydbSource, UpdateResult}; pub use keydb_format::{DiscEntry, KeyDb}; -pub use online::{MIN_SAMPLE_UNITS, OnlineSource, validate_keyserver_url}; +pub use online::{ + DecodeReachability, MIN_SAMPLE_UNITS, OnlineSource, take_last_decode_reachability, + validate_keyserver_url, +}; pub use paths::{default_keydb_path, existing_keydb_path, keydb_search_paths}; // Re-exported for downstream convenience so apps need only depend on this crate diff --git a/src/online.rs b/src/online.rs index c64533b..3222ed8 100644 --- a/src/online.rs +++ b/src/online.rs @@ -315,6 +315,10 @@ impl OnlineSource { // a terminal `UK` or a `VUK` derived locally. `Ok`/`Err` draw the // miss-vs-outage distinction — see docs/online-query-contract.md. fn query(&self, ctx: &dyn ResolveCtx) -> Result, Error> { + // Reset the per-thread decode-reachability slot so it reflects ONLY this + // query afterwards: `Some(..)` once a POST answers/transport-fails, `None` + // if we short-circuit before the network. See `take_last_decode_reachability`. + clear_decode_reachability(); // No configured service: nothing to resolve. if self.base_url.is_empty() { return Ok(Vec::new()); @@ -392,6 +396,10 @@ impl OnlineSource { // Did-not-RESOLVE is the service unreachable, not a bad URL — see // docs/online-guardfail.md. Err((GuardFail::Unreachable, _)) => { + // The host did not resolve — the service never answered, so this + // is a transport-class outcome for the decode-reachability slot + // (transient/down), the same verdict a refused connection gets. + record_decode_reachability(DecodeReachability::Transport); tracing::warn!( target: "freemkv::keysource", phase = "keyserver_post", @@ -429,6 +437,48 @@ impl OnlineSource { } } +/// The reachability outcome of a single online `/decode` POST — the raw signal +/// a caller needs to tell a *genuine no-key* (the service answered, e.g. a 200 +/// with no entry or a definitive 422/404) from a *transient outage* (it did not +/// — transport failure or a 5xx) WITHOUT firing a second probe. Recorded per +/// thread by [`OnlineSource`] on every decode attempt; read (and cleared) with +/// [`take_last_decode_reachability`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DecodeReachability { + /// The service answered with this HTTP status — any 2xx/3xx/4xx/5xx, + /// including a 200 no-key, a 404/422 ("licensed but unresolved"), a 429, or + /// a 502. The caller maps the code to a verdict. + Status(u16), + /// No HTTP answer at all — connection refused, timeout, DNS/TLS failure, so + /// the request was sent (or attempted) but nothing on the other end replied. + Transport, +} + +thread_local! { + // The reachability of the most recent decode POST on THIS thread. A + // thread-local (not a struct field) because resolution runs synchronously on + // the caller's thread while `OnlineSource` is boxed behind `dyn KeySource`. + static LAST_DECODE_REACHABILITY: std::cell::Cell> = + const { std::cell::Cell::new(None) }; +} + +fn clear_decode_reachability() { + LAST_DECODE_REACHABILITY.with(|c| c.set(None)); +} + +fn record_decode_reachability(outcome: DecodeReachability) { + LAST_DECODE_REACHABILITY.with(|c| c.set(Some(outcome))); +} + +/// Take — read and clear — the reachability outcome of the most recent online +/// `/decode` POST made ON THIS THREAD, or `None` if no decode reached the +/// network since the last take (the online source was not attempted, or a +/// short-circuit path that never POSTed). Lets a caller classify a no-key result +/// from the REAL decode's HTTP outcome instead of a second, redundant probe. +pub fn take_last_decode_reachability() -> Option { + LAST_DECODE_REACHABILITY.with(std::cell::Cell::take) +} + // Map a key-service HTTP status into the operator action it implies: 401/403 // fix credentials, 429 back off, 5xx wait — none of them is "no key" (the // genuine miss is a 200 with an empty body), the original bug this fixes. @@ -449,8 +499,21 @@ fn interpret_reply( elapsed_ms: u64, ) -> Result, Error> { let mut resp = match sent { - Ok(r) => r, + Ok(r) => { + // The service answered — record its status for the reachability slot + // (a 200/404/422 is "up"; a 5xx is "down"), read by the caller so a + // no-key needs no second probe. + record_decode_reachability(DecodeReachability::Status(r.status().as_u16())); + r + } Err(e) => { + // A 4xx/5xx is still an ANSWER (record its status); a transport + // error is not (record `Transport`). Kept separate from the + // error-mapping below, which is unchanged. + record_decode_reachability(match &e { + ureq::Error::StatusCode(code) => DecodeReachability::Status(*code), + _ => DecodeReachability::Transport, + }); // 401/403/429/5xx demand different operator actions; collapsing // them is why a 502 was read as "no key". Each arm RETURNS the // classified error, never an empty vec, so it survives past here. @@ -1005,6 +1068,65 @@ mod tests { ); } + // The decode POST records its reachability so the caller can classify a + // no-key from the REAL answer instead of firing a second empty probe: an + // HTTP answer (200/404/422) → `Status(code)`, a transport failure → + // `Transport`. This is the signal that removes autorip's redundant probe. + #[test] + fn interpret_reply_records_the_decode_reachability() { + // A 200 with no entry — the service answered; record its status. + let _ = interpret_reply(Ok(reply(200, "{}")), &BareCtx, 1); + assert_eq!( + take_last_decode_reachability(), + Some(DecodeReachability::Status(200)), + "a 200 answer must record Status(200)" + ); + // take clears the slot — a second take sees nothing. + assert_eq!( + take_last_decode_reachability(), + None, + "take must clear the slot" + ); + + // A definitive 422 ("licensed but unresolved") / 404 is still an ANSWER: + // record the status so the caller reads it as reachable (genuine no-key). + for status in [404u16, 422] { + let _ = interpret_reply(Err(ureq::Error::StatusCode(status)), &BareCtx, 1); + assert_eq!( + take_last_decode_reachability(), + Some(DecodeReachability::Status(status)), + "HTTP {status} must record Status({status}) — a reachable answer" + ); + } + + // A 5xx is an answer too — Status(502); the caller maps 5xx to down. + let _ = interpret_reply(Err(ureq::Error::StatusCode(502)), &BareCtx, 1); + assert_eq!( + take_last_decode_reachability(), + Some(DecodeReachability::Status(502)), + "a 5xx must record its status, not Transport" + ); + + // A transport failure (refused connection) records `Transport` — no + // HTTP answer, so the caller treats it as a transient outage. + let config = Config::builder() + .timeout_connect(Some(Duration::from_secs(2))) + .build(); + let sent = ureq::Agent::new_with_config(config) + .post("http://127.0.0.1:1/") + .send("{}"); + assert!( + !matches!(sent, Ok(_) | Err(ureq::Error::StatusCode(_))), + "a refused connection must be a transport error" + ); + let _ = interpret_reply(sent, &BareCtx, 1); + assert_eq!( + take_last_decode_reachability(), + Some(DecodeReachability::Transport), + "a transport failure must record Transport, never a Status" + ); + } + /// A reply the client cannot read is the service failing mid-answer, not an /// answer of "no key". #[test]