state: expose a JSON-safe summary of pending registration cache entries (#2714) - #3411
Open
MsfPablo wants to merge 1 commit into
Open
state: expose a JSON-safe summary of pending registration cache entries (#2714)#3411MsfPablo wants to merge 1 commit into
MsfPablo wants to merge 1 commit into
Conversation
DebugRegistrationCache() already stopped marshaling raw AuthRequest values (fixing the reported chan-in-JSON panic), but it also stopped saying anything about individual entries, leaving unmet the issue's stated expectation of being able to "display the current registration cache". AuthRequest carries unexported fields backed by a channel, so it can't be marshaled directly. Add a per-entry summary (auth_id, flow type: registration/ssh-check/plain) built from AuthRequest's existing public accessors (IsRegistration/IsSSHCheck), which never touch the channel. Peek is used instead of Get so inspecting the cache for debugging doesn't itself affect LRU recency/eviction order. Renamed the existing TestDebugRegistrationCache, which actually exercised NodeStore.DebugString (a different type), to TestNodeStoreDebugRegistrationCache, and added TestStateDebugRegistrationCache to cover State.DebugRegistrationCache itself. Relates to juanfont#2714
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
1 participant
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.
Context
#2714 originally reported that
DebugRegistrationCachepanicked/errored on JSON marshaling because it returned rawtypes.AuthRequestvalues, which carry an unexportedchan AuthVerdictfield.That literal panic no longer reproduces on current
main—DebugRegistrationCachealready stopped returning the raw entries. But in doing so it also stopped saying anything about the individual pending entries, so the issue's stated "Expected Behavior" (being able to inspect what's actually in the registration cache) is still unmet: the debug endpoint currently only reports cache-level metadata (size, expiration, max entries), not what's in it.What this does
Adds a small per-entry summary to
DebugRegistrationCache's output — anauth_idand atype(plain/registration/ssh-check, derived fromAuthRequest.IsRegistration()/IsSSHCheck(), which never touch the unexported channel field).Peekis used instead ofGetso inspecting the cache for debugging doesn't itself perturb LRU recency/eviction order.I also found the existing
TestDebugRegistrationCacheindebug_test.godidn't actually exerciseState.DebugRegistrationCacheat all — it exercisedNodeStore.DebugString, an unrelated type that happens to share a similarly-named debug method. Renamed that test toTestNodeStoreDebugRegistrationCacheto reflect what it actually covers, and addedTestStateDebugRegistrationCachefor real regression coverage of this method (empty cache, plus one entry of each flow type).Note on scope
I noticed
AGENTS.mdasks contributors to check in about scope on ambiguous issues before implementing. #2714's original literal bug is already fixed, so I want to be upfront that this is a proposal for closing the gap between "bug fixed" and the issue's stated expected behavior — not a claim that this is the only or definitive way to do that. Happy to adjust the shape of the debug output (e.g. more/fewer fields) or close this if there's a different intended direction.Testing
go build ./...go test ./hscontrol/state/...(full package, passing)go test ./hscontrol/state/... -run DebugRegistrationCache -v(both new/renamed tests passing)gofmt -lclean on both changed filesgolangci-lint runclean on both changed filesCloses #2714 if this shape is acceptable — otherwise happy to iterate.