Diagnostics: Add INFO-level logging to SSO session init paths#522
Merged
Diagnostics: Add INFO-level logging to SSO session init paths#522
Conversation
Add diagnostic logging to three critical SSO code paths to help debug why certain clients (e.g. Claude) fail to trigger SSO connections after OAuth login completes. Logs inputs, filter breakdown, and callback state. Made-with: Cursor
…gs (#520) After a pod restart, stale Valkey sessions trigger massive log spam because onAuthenticated fires for sessions without a usable ID token. This commit: - Skips initSSOForSession when the ID token is empty (Fix A), preventing doomed downstream connections that spam 403 errors. - Extracts headerFunc into makeTokenForwardingHeaderFunc and rate-limits the WARN log to once per minute per session+server pair (Fix B), logging subsequent failures at DEBUG instead. - Adds unit tests for both fixes. Made-with: Cursor
- Use defer consistently in ExposedToolName/ExposedPromptName - Return errors from AsBool() in Valkey Touch/Exists instead of swallowing - Extract storeIDTokenForSSO helper to DRY up session creation/refresh handlers - Move encryptor creation into createStores for single-responsibility - Remove unused purgeServerNames method - Use assert.Contains/NotContains in headerFunc rate-limit tests Made-with: Cursor
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.
Summary
Adds diagnostic logging and fixes for log noise from stale Valkey sessions after pod restarts.
Diagnostic Logging (original)
initSSOForSession,SessionCreationHandler,onAuthenticated) to help debug why certain clients (e.g. Claude) fail to trigger downstream SSO connections after OAuth login completes.initSSOForSession: logs all inputs on entry and a filter breakdown (total servers, pending count, skip reasons).SessionCreationHandler: logs when fired with userID, familyID, and ID token presence.onAuthenticated: logs callback invocation with sessionID, userID, authAlive state, and ID token presence.Fix A: Gate SSO init on valid ID token (closes #520)
onAuthenticated, afterauthStore.Touch()returns false, checks whether the ID token is empty before callinginitSSOForSession().Fix B: Rate-limit headerFunc warning (closes #520)
headerFuncclosure fromEstablishConnectionWithTokenForwardingintomakeTokenForwardingHeaderFuncfor testability.Refactor: Code review cleanups
defer r.nameMu.Unlock()uniformly inExposedToolName/ExposedPromptName(was inconsistent withExposedResourceURI).Touch/Exists/IsAuthenticatednow return the error fromAsBool()instead of silently returning(false, nil).storeIDTokenForSSOhelper to eliminate duplicate token-storing code betweenSetSessionCreationHandlerandSetTokenRefreshHandler.createStores/storeBundle, removing the double type-assertion fromNewAggregatorServer.purgeServerNamesmethod (defined but never called).assert.True(t, strings.Contains(...))withassert.Contains/assert.NotContainsfor better failure messages.Test plan
make test)muster test --parallel 50)TestOnAuthenticated_SkipsSSO_WhenNoIDTokenverifies Fix A guard logicTestHeaderFunc_RateLimitsWarningverifies Fix B rate-limiting and recovery logging