fabric status is broken in production: carry the reconnect total as u64 - #41
Merged
Conversation
I shipped LatencySummary.total_micros as a u128 in PR 36. fabric status then
failed with "u128 is not supported" on any daemon that had probed a peer, which
is every daemon with a peer, a few minutes after start.
u64 microseconds overflows after roughly 584000 years, so this is not a real
downgrade. It is the type the wire can carry.
WHY IT REACHED PRODUCTION. Every check that should have caught it ran against
EMPTY state, which is the one case that works. The unit tests asserted on the
struct. The integration test called the response builder directly and never
serialized it. My own hand check used a fresh node with no peers. And the
rollout acceptance ran at install, before the first probe populated the map. The
break is time-delayed by construction, so a green acceptance could not have
caught it.
WHY THE OBVIOUS TEST DOES NOT CATCH IT EITHER, which is the part worth keeping.
A plain serde_json round-trip of the telemetry snapshot PASSES while the command
is broken. serde_json handles a u128 in a normal struct. ControlResponse is an
INTERNALLY TAGGED enum, so serde routes it through its Content buffer, and that
buffer has no u128 variant. The failure is on DECODE in the client.
So the regression test serializes the real ControlResponse, not the inner
struct, and it starts from a POPULATED map. I verified it fails on the u128
type with the exact production error, Error("u128 is not supported"), and passes
on u64. The plain-JSON test is kept and relabelled as necessary but not
sufficient, so nobody mistakes it for the guard.
A fixed client decodes correctly against an unfixed running daemon, because the
daemon serializes the value fine and only the client's decode failed.
197 lib tests green.
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.
Fixes a live regression I introduced in #36.
fabric statusfails on Silber right now withError: u128 is not supported.The bug
LatencySummary.total_microswas au128.u64microseconds overflows after roughly 584,000 years, so this is not a real downgrade — it is the type the wire can carry.Read-only impact: only the operator command. The daemon, sessions, sync, shell, and exec are unaffected, and the telemetry file keeps recording correctly. cos independently confirmed it exits 1, not 0, so scripts fail loudly rather than reading a false green.
Why it reached production
Every check that should have caught it ran against empty state, which is the one case that works:
The break is time-delayed by construction. Root's acceptance was honest and could not have caught it.
Why the obvious test does not catch it either
This is the part worth keeping. A plain
serde_jsonround-trip of the telemetry snapshot passes while the command is broken — I wrote that test first and watched it pass. serde_json handlesu128in a normal struct.ControlResponseis an internally tagged enum (#[serde(tag = "type")]), so serde routes it through itsContentbuffer, and that buffer has nou128variant. The failure is on decode, in the client.So the regression test:
ControlResponse, not the inner structVerified both directions: it fails on the
u128type with the exact production error —Error("u128 is not supported", line: 0, column: 0)— and passes onu64.The plain-JSON test is kept but relabelled necessary but not sufficient, so nobody mistakes it for the guard.
Rollout note
A fixed client decodes correctly against an unfixed running daemon — the daemon serializes the value fine, only the client's decode failed. So installing the new binary repairs
fabric statuswithout requiring a daemon restart. Root's call.Tests
197 lib tests green. Changed files pass rustfmt.