status: stop lying about the next get's cost, and pin the frame consumers actually decode - #20
Merged
ualtinok merged 2 commits intoAug 29, 2026
Conversation
Three defects on the credential.status surface were found by an automated reviewer after the stale_pending field shipped in 8d3d81c and bc87553. All three are fixed here; the field is now a real latency predictor on the records it claims to describe, and the test for the wire shape pins the full on-wire frame rather than the inner body alone. The first defect was a live one. None of the seven `UPDATE credentials SET state = ...` paths in credentials-core::store clear the stale_pending column, so a record marked stale by a consumer 401 and then latched to needs_reauth (the path taken when the forced refresh fails) still carries stale_pending = 1 on the column. status published the column value as a latency prediction: stale_pending=true said "next get pays seconds" while the next get failed fast with needs_reauth and never touched the network. Measured on this deployment 2026-08-27: every four hours, for roughly a five-minute window, until the re-seal wrote state='active', stale_pending=0 in one statement and hid the lie. The fix lives at the read surface, not the store: when a handle resolved and the record state is Active, publish the real meta.stale_pending; when the state is anything else, publish false -- the next get performs no upstream exchange, so the prediction must say so. Absent is unchanged on the resolve-fail and meta-unreadable arms; that semantic ("this path could not see the record") must not move. A new test pins the non-Active case. The state is constructed through the production paths -- public report_auth_failure sets the mark on a refreshable id, then store.invalidate does exactly what the engine does when its forced refresh fails -- so the test is a real reading of the buggy state, not a hand-staged copy of it. Mutation-checked both ways: reverting the fix turns the assertion red with "left: Some(true), right: Some(false)", and the precondition that meta.stale_pending survives the state flip is asserted first so the test's failure mode names the live shape rather than the diff. The second defect was a test-only one. The existing status_publishes_the_stale_mark_without_calling_the_credential_unhealthy test seeded apikey:active (non-refreshable) and called store.mark_stale_if_version_reported directly with the observation kind "consumer_report_stale" -- a string only the refreshable arm of report_auth_failure emits. The public route branches on refreshability and the non-refreshable arm INVALIDATES rather than marks, so the production path cannot produce the state the test was constructing; the test was passing against a copy of the mark with no assertion behind it. The fix seeds oauth:stub (refreshable per default_refresh_adapter) and drives the mark through the public report_auth_failure route, the only call that can ever set the marker on a real handle. The three assertions (mark visible without calling get, ready stays true, last_error_code stays None) keep their original intent and meaning. Mutation-checked by inverting the refreshable arm in report_auth_failure to call invalidate_if_version_reported: the test goes red with "left: Some(false), right: Some(true)" and the precondition that meta.stale_pending is set fails too. Restore returns green. The third defect was a comment-vs-coverage gap. The test pinned only the inner error body `{"error":{"code":"not_found","class":"permanent"}}` while the comment claimed it was "the exact frame captured from a live daemon". The transport layer wraps that body in `{"result": ...}` inside handle_read_request, and renaming the outer key left the test green while every consumer decoder that routes on `result` broke. The pin is now extended to the full on-wire frame `{"result":{"error":{"code":"not_found","class":"permanent"}}}` and written as a single readable JSON literal so the byte sequence can be quoted verbatim into the consumer's fixture rather than re-derived from a producer. The producer is still the real GetOutcome::Err serialization, then wrapped with the same key the route builder uses; a reconstruction would only pin a reconstruction. Two specific diagnostics precede the equality check (the outer wrapper vanished; the class field vanished from the body), so the broad assert_eq! panic names the shape rather than asking the reader to diff two blobs. Mutation-checked by renaming the wrapper key to result_mutated (RED with the wrapper diagnostic), then changing the class value (RED with the broad drift message showing the exact difference). Restore returns green. Gate: 441 tests, exit 0, nine real-daemon e2e arms executing.
The shape pin compares `serde_json::Value`s, so it is order-independent. That is the right guarantee for a shape pin -- a key moving should not turn it red -- but it means the test is green for either field order, and a consumer holding a byte-string fixture is not covered by it. The wire order is not this struct's declaration order. `ErrorBody` declares `code` then `class`; the wire emits `class` then `code`, because the reply is built through a `serde_json::Value` and `serde_json::Map` is a `BTreeMap` unless the `preserve_order` feature is on, so keys ship alphabetically. Confirmed against the running daemon from both sides of the wire, and reproduced in isolation: the same struct serialized directly yields `code` first. So the current byte order is ACCIDENTAL. It holds only while the reply goes through a `Value`; serializing the struct straight to bytes would flip it with nothing to notice. This assertion converts that accident into a decision someone has to make deliberately. Found the way these things get found. The first consumer of this surface was handed a canonical literal to quote verbatim into their fixture. It was transcribed from the struct declaration rather than read off the wire, so it did not match production -- and the shape pin could not have caught that, because it was written not to. Deserialization does not care about key order; a byte-comparing fixture or a frame digest does. The consumer caught it on their first live probe, from a different process, against a claim I had just published. Mutation-checked: flipping the pinned order turns it red naming the byte change specifically, and the mutation script asserts its target text is present before editing, because the previous attempt silently matched nothing and reported a pass. Gate: exit 0, nine real-daemon e2e arms executing.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/credentials-module/src/read_surface.rs">
<violation number="1" location="crates/credentials-module/src/read_surface.rs:1234">
P3: The fix gates the "no upstream exchange" claim on `is_active` only, but a record can be `Active` while the daemon is fenced out. In that state `ready`/`lease_held` are already false (`!fenced_out && is_active`), the next `get` fails fast at the lease/fence check without an upstream exchange, yet `stale_pending` still publishes the raw column value here, so an Active record with `stale_pending = 1` (mark left from a pre-fence consumer 401) keeps claiming "next get pays seconds" — the exact lie this PR exists to stop. Consider folding the fenced-out condition into the gate so a non-ready daemon never advertises an exchange cost, e.g. publish `Some(false)` whenever `!ready`.</violation>
<violation number="2" location="crates/credentials-module/src/read_surface.rs:1539">
P2: This pin does not exercise `handle_read_request`, so changing the route wrapper can break consumers while `error_frame_shape_is_pinned` still passes. Build the assertion from the route handler or a shared route-serialization helper so it detects wrapper drift.</violation>
</file>
<file name="crates/credentials-module/src/main.rs">
<violation number="1" location="crates/credentials-module/src/main.rs:4597">
P2: This test claims to reproduce a failed refresh, but it calls unversioned `store.invalidate` instead of the engine's version-fenced invalidation path. Exercise the engine failure or call `invalidate_if_version_reported` so the regression test cannot diverge from production behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let got: serde_json::Value = | ||
| serde_json::to_value(&frame).expect("serialize the error outcome"); | ||
| let inner_value = serde_json::to_value(&inner).expect("serialize the error outcome"); | ||
| let got = serde_json::json!({ "result": inner_value }); |
There was a problem hiding this comment.
P2: This pin does not exercise handle_read_request, so changing the route wrapper can break consumers while error_frame_shape_is_pinned still passes. Build the assertion from the route handler or a shared route-serialization helper so it detects wrapper drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/read_surface.rs, line 1539:
<comment>This pin does not exercise `handle_read_request`, so changing the route wrapper can break consumers while `error_frame_shape_is_pinned` still passes. Build the assertion from the route handler or a shared route-serialization helper so it detects wrapper drift.</comment>
<file context>
@@ -1482,53 +1502,115 @@ mod error_class_tests {
- let got: serde_json::Value =
- serde_json::to_value(&frame).expect("serialize the error outcome");
+ let inner_value = serde_json::to_value(&inner).expect("serialize the error outcome");
+ let got = serde_json::json!({ "result": inner_value });
// ORDER IS LOAD-BEARING, and this is the second version. Written with the
</file context>
| // at the failure site; the column `stale_pending` is deliberately not touched by | ||
| // any of the seven state-update paths, which is the bug we are pinning here. | ||
| store | ||
| .invalidate("oauth:needs_reauth_after_stale") |
There was a problem hiding this comment.
P2: This test claims to reproduce a failed refresh, but it calls unversioned store.invalidate instead of the engine's version-fenced invalidation path. Exercise the engine failure or call invalidate_if_version_reported so the regression test cannot diverge from production behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/main.rs, line 4597:
<comment>This test claims to reproduce a failed refresh, but it calls unversioned `store.invalidate` instead of the engine's version-fenced invalidation path. Exercise the engine failure or call `invalidate_if_version_reported` so the regression test cannot diverge from production behavior.</comment>
<file context>
@@ -4493,6 +4521,127 @@ mod tests {
+ // at the failure site; the column `stale_pending` is deliberately not touched by
+ // any of the seven state-update paths, which is the bug we are pinning here.
+ store
+ .invalidate("oauth:needs_reauth_after_stale")
+ .expect("engine-style invalidate after failed refresh");
+
</file context>
Suggested change
| .invalidate("oauth:needs_reauth_after_stale") | |
| .invalidate_if_version_reported( | |
| "oauth:needs_reauth_after_stale", | |
| 1, | |
| AuditCtx::vault(AuditOp::Invalidate), | |
| Some(credentials_core::store::AuthObservation { | |
| kind: "refresh_failed", | |
| provider_status: None, | |
| detail: Some("invalid_grant"), | |
| }), | |
| ) |
| // Deliberately NOT folded into `ready`: a stale-marked record is | ||
| // still usable, it is merely expensive on the next read. Published | ||
| // only when Active; see the comment above for the bug this gates. | ||
| stale_pending: Some(if is_active { meta.stale_pending } else { false }), |
There was a problem hiding this comment.
P3: The fix gates the "no upstream exchange" claim on is_active only, but a record can be Active while the daemon is fenced out. In that state ready/lease_held are already false (!fenced_out && is_active), the next get fails fast at the lease/fence check without an upstream exchange, yet stale_pending still publishes the raw column value here, so an Active record with stale_pending = 1 (mark left from a pre-fence consumer 401) keeps claiming "next get pays seconds" — the exact lie this PR exists to stop. Consider folding the fenced-out condition into the gate so a non-ready daemon never advertises an exchange cost, e.g. publish Some(false) whenever !ready.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-module/src/read_surface.rs, line 1234:
<comment>The fix gates the "no upstream exchange" claim on `is_active` only, but a record can be `Active` while the daemon is fenced out. In that state `ready`/`lease_held` are already false (`!fenced_out && is_active`), the next `get` fails fast at the lease/fence check without an upstream exchange, yet `stale_pending` still publishes the raw column value here, so an Active record with `stale_pending = 1` (mark left from a pre-fence consumer 401) keeps claiming "next get pays seconds" — the exact lie this PR exists to stop. Consider folding the fenced-out condition into the gate so a non-ready daemon never advertises an exchange cost, e.g. publish `Some(false)` whenever `!ready`.</comment>
<file context>
@@ -1208,24 +1208,44 @@ impl ReadSurface {
+ // Deliberately NOT folded into `ready`: a stale-marked record is
+ // still usable, it is merely expensive on the next read. Published
+ // only when Active; see the comment above for the bug this gates.
+ stale_pending: Some(if is_active { meta.stale_pending } else { false }),
+ last_error_code: match meta.state {
+ credentials_core::store::RecordState::NeedsReauth
</file context>
Suggested change
| stale_pending: Some(if is_active { meta.stale_pending } else { false }), | |
| stale_pending: Some(if is_active && !fenced_out { meta.stale_pending } else { false }), |
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
2 participants
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.
Follow-up to #19. Three findings from that PR's review, plus one found by a consumer running the wire.
Sits cleanly on top of
3dfd7fe— that pins thecredential.statuskey set; this pins thecredential.geterror frame's bytes. Different surfaces, no overlap.1 — the field lied once a record left Active
stale_pendinganswers "will the nextgetbuy an upstream token exchange". None of the sevenSET statepaths in the store clear the column, so after a 401 report whose forced refresh then fails, the record sitsneeds_reauthwithstale_pending = 1— andstatusreportedtruefor a call that now fails fast atstore.rs:1158without touching the network.Live on this deployment: a ~5 minute window every four hours, cleared only because the re-seal writes
state = 'active', stale_pending = 0in one statement.Fixed at the read surface rather than the store. The column is honestly "a repair is pending"; what was wrong is publishing it as a cost prediction when the state makes that prediction false. Resolved handle + Active publishes the real value; resolved handle + anything else publishes
false; unresolved keeps the field absent, unchanged.2 — the test for it exercised a path production cannot take
It seeded
apikey:activeand calledmark_stale_if_version_reporteddirectly on the store. But the route branches on refreshability, andapikey:has no adapter — so production invalidates that record and never marks it stale. The test built a state (non-refreshable + Active +stale_pending = 1) the public path cannot produce, and passed the observation kind only the refreshable arm emits.Now seeds a refreshable OAuth fixture and drives the mark through
report_auth_failure. Same three assertions.Mutation-checked on the production code, not the test: inverting the refreshable arm turns both tests red. Mutating the test alone proves nothing here — the old wrong fixture passes against the old wrong code, which is exactly how it survived review.
3 — the shape pin stopped short of the wrapper
error_frame_shape_is_pinnedpinned the inner error body. The transport wraps it in{"result": ...}, so renaming that key left the test green while every decoder broke. Now pins the full frame, with the wrapper diagnostic firing before the equality check.4 — the bytes were not what the canonical literal said
Not from review. A consumer probed the running daemon and got a different byte order than the literal they had been handed to quote:
ErrorBodydeclarescodethenclass; the wire emits alphabetically, because the reply is built through aserde_json::Valueandserde_json::Mapis aBTreeMapwithoutpreserve_order. The literal had been transcribed from the struct declaration and called the wire.Semantically identical, and no deserializing consumer cares. A byte-comparing fixture or a frame digest does.
So the current order is accidental — it holds only while the reply passes through a
Value. This adds a string assertion alongside theValueone: the shape pin stays order-independent (correct for a shape pin), and the byte pin makes a serialization-path change a decision someone has to make rather than a silent flip.Verification
bash scripts/gate.shexit 0, nine real-daemon e2e arms executing.Every added test mutation-checked red→green. One note worth carrying: the first attempt at the byte-order mutation silently matched nothing (escaped for an ordinary string; the target is a raw string) and the test passed — indistinguishable from a real check. The rerun asserts its target text exists before editing.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Stops
statusfrom publishing a stalestale_pendingmark on records whose nextgetfails fast, and pins the full on-wire error frame — wrapper and byte order — so consumer fixtures match what production emits.Bug Fixes
stale_pendingnow returnsfalseon any resolved record that isn't Active; the leftover column value previously survived state flips because no state-update path clears it.report_auth_failureroute; the old fixture staged a non-refreshableapikeyrecord via a direct store call, a state production cannot produce.{"result": ...}envelope every route adds, not just the inner error body.serde_json::Value, whose map is aBTreeMap), so the canonical frame is{"result":{"error":{"class":"permanent","code":"not_found"}}}; the new byte assertion locks that order, and that alphabetical handling is incidental — it flips if the reply stops passing through aValue.Written for commit b8474c3. Summary will update on new commits.