diff --git a/crates/credentials-module/src/main.rs b/crates/credentials-module/src/main.rs index b7d2bf4..fb2017c 100644 --- a/crates/credentials-module/src/main.rs +++ b/crates/credentials-module/src/main.rs @@ -4158,6 +4158,105 @@ mod tests { ); } + /// The mark that predicts a SLOW get on a record every other field calls healthy. + /// + /// Pins the exact reading that was invisible before this field existed: `ready: + /// true`, `last_error_code: null`, and the next `get` about to buy an upstream token + /// exchange. A consumer sizing a startup bound cannot get that from `ready`, because + /// `ready` is genuinely TRUE -- the mark exists so the next get refreshes rather than + /// refusing. + #[tokio::test] + async fn status_publishes_the_stale_mark_without_calling_the_credential_unhealthy() { + let (surface, store, _db) = tmp_surface_with_store(16); + let handle = credentials_core::store::mint_handle().expect("mint handle"); + store + .put_handle_hash( + &handle.hash, + "apikey:active", + AuditCtx::admin(AuditOp::MintHandle), + ) + .expect("put handle"); + + let clean = surface + .status( + 1, + &crate::read_surface::StatusParams { + handle: Some(handle.raw.clone()), + }, + ) + .await; + assert_eq!( + clean.stale_pending, + Some(false), + "a resolved handle must report the mark explicitly, not by omission" + ); + assert!(clean.ready, "precondition: the record starts healthy"); + + // Exactly what a consumer's 401 report does, at the version it was served. + let served = clean + .record_version + .expect("resolved handle reports version"); + store + .mark_stale_if_version_reported( + "apikey:active", + served, + AuditCtx::admin(AuditOp::ReportAuthFailure), + credentials_core::store::AuthObservation { + kind: "consumer_report_stale", + provider_status: Some(401), + detail: None, + }, + ) + .expect("mark stale"); + + let marked = surface + .status( + 1, + &crate::read_surface::StatusParams { + handle: Some(handle.raw.clone()), + }, + ) + .await; + assert_eq!( + marked.stale_pending, + Some(true), + "the mark must be visible WITHOUT calling get -- the whole point is to avoid \ + the call whose cost is in question" + ); + // The load-bearing half. If this ever flips to false, the field has been folded + // into health and a consumer will start treating a usable credential as broken. + assert!( + marked.ready, + "a stale-marked record is still USABLE -- expensive is not unhealthy" + ); + assert!( + marked.last_error_code.is_none(), + "a pending repair is not an error that has occurred" + ); + + // ABSENT, never defaulted false: claiming "no repair pending" for a record this + // path could not read would be an assertion with no basis behind it. + let overall = surface + .status(1, &crate::read_surface::StatusParams { handle: None }) + .await; + assert!( + overall.stale_pending.is_none(), + "overall readiness names no credential, so it can report no mark" + ); + let unknown = surface + .status( + 2, + &crate::read_surface::StatusParams { + handle: Some("ckh_definitely-not-a-handle".to_string()), + }, + ) + .await; + assert!( + unknown.stale_pending.is_none(), + "an unresolvable handle must not assert anything about a record" + ); + } + #[tokio::test] async fn status_handle_probe_runs_the_limiter() { let (surface, store, _db) = tmp_surface_with_store(15); diff --git a/crates/credentials-module/src/read_surface.rs b/crates/credentials-module/src/read_surface.rs index 806ceaf..e49b5a4 100644 --- a/crates/credentials-module/src/read_surface.rs +++ b/crates/credentials-module/src/read_surface.rs @@ -464,24 +464,9 @@ pub struct StatusResult { /// would defeat itself by moving the version it matched on). So a stable version /// with `ready: false` is a normal reading, not a stuck cursor. /// - /// WHAT `status` DELIBERATELY DOES NOT PUBLISH: `stale_pending`. For the whole - /// window between a version-matched report and the next `get`, this surface reports - /// a healthy credential while the store has already recorded the mark. Measured by - /// an external contributor on 2026-08-25: twelve samples over five minutes, all - /// `ready: true`, with the chain row already written. - /// - /// That is correct for what `ready` MEANS -- "would a get succeed" -- and it stays - /// true, because the mark exists precisely so the next get refreshes rather than - /// refusing. A get-path consumer receives the entire benefit without ever needing to - /// see the flag, which is why no consumer has asked for it. - /// - /// The gap is real for a consumer that POLLS status as its health surface: it cannot - /// learn a mark is outstanding, so it cannot distinguish "healthy" from "healthy, - /// with a repair pending". Not published today because no such consumer exists, and - /// a field added for a hypothetical caller is machinery nobody can test against a - /// real requirement. Revisit when one arrives -- the disclosure cost is small - /// (`ready: false` already tells a handle holder that someone observed a failure), - /// so the decision is about usefulness rather than safety. + /// See [`StatusResult::stale_pending`] for the mark that predicts a SLOW get on an + /// otherwise healthy record -- the two fields answer different questions and a + /// consumer sizing a timeout needs the other one. /// /// AND IT DOES NOT CATCH EVERY REPAIR -- read `ready`, not this, to answer "is it /// usable now". `reactivate` clears a wrong `needs_reauth` verdict WITHOUT touching @@ -497,6 +482,50 @@ pub struct StatusResult { /// tracks the MATERIAL; `ready` tracks the VERDICT; a repair can move either alone. #[serde(skip_serializing_if = "Option::is_none")] pub record_version: Option, + + /// A consumer reported the current token refused, so the NEXT `get` must refresh + /// before serving. Present only when a handle resolved. + /// + /// THIS IS A LATENCY PREDICTOR, NOT A HEALTH FIELD, and it exists because the two + /// are not the same question. `ready` answers "would a get succeed" -- and on a + /// stale-marked record the honest answer is YES, because the mark exists precisely + /// so the next get refreshes rather than refusing. What `ready` cannot say is what + /// that get will COST: + /// + /// state=active, stale_pending=false -> local read, sub-millisecond + /// state=active, stale_pending=TRUE -> forces an upstream exchange, seconds + /// state=needs_reauth -> fails fast, no upstream call + /// + /// So a record can read healthy on every other field while the next call is three + /// orders of magnitude slower than the one before it. Measured 2026-08-25 before + /// this field existed: twelve status samples over five minutes, every one + /// `ready: true, last_error_code: null`, with the mark already written to the store + /// and its chain row committed. Nothing on the wire distinguished them. + /// + /// WHY IT IS PUBLISHED NOW rather than earlier: the field was withheld deliberately + /// while no consumer polled this surface, on the grounds that a field added for a + /// hypothetical caller is machinery nobody can test against a real requirement. That + /// condition ended -- the first vault consumer warms a credential cache at startup + /// and needs to SKIP the accounts that would overrun its bound, rather than + /// discovering them by timing out. Skipping requires seeing the mark; the only other + /// way to observe it was `credential.get`, which is the very call whose cost is in + /// question. + /// + /// FREE TO SERVE, which is the whole reason this is a field and not a new verb: + /// `stale_pending` is a PLAINTEXT column, `store::meta()` already selects it, and + /// [`RecordMeta`] already carries it. This path was fetching the value and + /// discarding it before serialization -- exactly the state `record_version` was in + /// before it was published. No decrypt, no master key, no extra query. + /// + /// NO NEW DISCLOSURE: `ready: false` already tells a handle holder that someone + /// observed a failure on this credential. This says only that a repair is pending on + /// one that still works. + /// + /// ABSENT rather than `false` when no handle was presented or the handle did not + /// resolve -- a defaulted `false` would read as "no repair pending" for a revoked + /// handle, which is an assertion this path has no basis to make. + #[serde(skip_serializing_if = "Option::is_none")] + pub stale_pending: Option, } /// The read surface: the engine (for refresh-on-read), the per-connection limiter, @@ -1188,6 +1217,10 @@ impl ReadSurface { last_error_code: None, lease_held, record_version: None, + // No handle means no record, so there is no mark to report. Absent + // rather than false: this is overall daemon readiness, not a claim + // about any credential. + stale_pending: None, }; } Some(h) => h, @@ -1202,6 +1235,9 @@ impl ReadSurface { // A fenced-out daemon is not ready even for an Active credential. ready: !fenced_out && matches!(meta.state, credentials_core::store::RecordState::Active), + // Deliberately NOT folded into `ready`: a stale-marked record is + // still usable, it is merely expensive on the next read. + stale_pending: Some(meta.stale_pending), last_error_code: match meta.state { credentials_core::store::RecordState::NeedsReauth | credentials_core::store::RecordState::Retired => { @@ -1213,18 +1249,24 @@ impl ReadSurface { lease_held, record_version: Some(meta.record_version), }, + // Meta unreadable: absent, not false. Reporting "no repair pending" for + // a record we could not read would be an assertion with no basis. Err(_) => StatusResult { ready: false, last_error_code: Some(ReadError::NotFound), lease_held, record_version: None, + stale_pending: None, }, }, + // Unresolvable handle -- uniform not_found, and no claim about a record that + // may not exist. Err(_) => StatusResult { ready: false, last_error_code: Some(ReadError::NotFound), lease_held, record_version: None, + stale_pending: None, }, } } @@ -1460,6 +1502,59 @@ mod error_class_tests { ); } + /// Golden conformance for the FRAME SHAPE, which the class-string test above does + /// not cover and cannot: it pins the four `class` values while saying nothing about + /// the envelope they arrive in. Rename `class` to `error_class`, move the error a + /// level, or drop `class` from the body entirely, and that test stays green while + /// every consumer breaks. + /// + /// WHY THIS EXISTS AT ALL: a consumer typed a decoder from the published contract, + /// parsed a real error frame SUCCESSFULLY, and silently discarded `class` — serde + /// drops unknown fields without complaint, so a decoder that ignores the field it + /// was told to branch on looks identical to one that honours it. They then branched + /// on `code` through a closed enum, which turns the first added code into a parse + /// failure rather than an unknown-code branch. Neither is reachable from this side; + /// what IS reachable is guaranteeing the bytes never move under them. + /// + /// Serialized through the REAL producer type rather than a hand-built `json!`, so + /// this pins what the wire actually carries. A reconstruction would only pin the + /// reconstruction — the frame could drift and this would still pass. + /// + /// The literal is the exact frame captured from a live daemon and handed to that + /// consumer, who pinned it in their tree. Both directions now go red on drift. + #[test] + fn error_frame_shape_is_pinned() { + let frame = GetOutcome::Err { + error: ErrorBody { + code: ReadError::NotFound, + class: ErrorClass::Permanent, + }, + }; + let got: serde_json::Value = + serde_json::to_value(&frame).expect("serialize the error outcome"); + + // ORDER IS LOAD-BEARING, and this is the second version. Written with the + // equality first, the specific check below never ran: `assert_eq!` panics on any + // difference, so dropping `class` reported "the frame shape drifted" and left the + // reader to diff two blobs. The diagnostic existed only for the case it could not + // reach. A cheap, specific assertion must precede a broad one that subsumes it, + // or it is decoration. + assert!( + got["error"].get("class").is_some(), + "`class` vanished from the error body — the contract's branch-on-class rule \ + becomes unfollowable and consumers silently fall back to branching on `code`" + ); + + let want = serde_json::json!({ + "error": { "code": "not_found", "class": "permanent" } + }); + + assert_eq!( + got, want, + "the error frame shape drifted — consumers branch on these exact keys" + ); + } + /// An UNMAPPED store error degrades to a TRANSIENT code, never a permanent one. /// /// This is the property a cross-repo consumer's destructive behaviour rests on,