fix: skip background token refresh for disabled/unconfigured MCP clients and guarantee non-nil logger in sync workers - #4848
Conversation
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Confidence Score: 4/5Safe to merge with the minor provider-nil guard asymmetry noted; the missing check is pre-existing and realistic call sites always pass a valid provider. The query change and logger-guard refactor are correct and well-covered by new tests. The only gap is that NewTokenRefreshWorker still dereferences provider.configStore without first checking provider == nil, inconsistent with the symmetric worker also edited in this PR. framework/oauth2/sync.go — NewTokenRefreshWorker constructor lacks the provider == nil guard present in NewPerUserOAuthSweepWorker. Important Files Changed
Reviews (2): Last reviewed commit: "fix: gates auto refresh of oauth tokens ..." | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds an enabled-MCP-client requirement to ChangesOAuth Token Refresh Eligibility
OAuth2 Worker Logging Cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/oauth2/sync.go (1)
161-198: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
PerUserOAuthSweepWorkercan panic on nil logger — constructor wasn't updated to matchTokenRefreshWorker's fix.
NewTokenRefreshWorkernow defaults a nilloggertobifrost.NewNoOpLogger()(lines 25-27), which is what makes it safe to drop thew.logger != nilguards inStart/Stop/refreshExpiredTokens.NewPerUserOAuthSweepWorkerwas not given the same treatment — it still stores the caller-suppliedloggeras-is (line 174:logger: logger), including when it'snil(the only nil-check, lines 162-166, only fires whenprovider/configStoreis nil, not whenloggeris nil).Since this PR also removes the
w.logger != nilguards fromPerUserOAuthSweepWorker.Start(line 183),Stop(line 197),sweepExpiredFlows(line 228), andsweepOrphanedTokens(line 242), any caller constructing this worker withprovider != nilandlogger == nilwill panic with a nil-interface method call the momentStart()runs.🐛 Proposed fix — mirror the TokenRefreshWorker default
func NewPerUserOAuthSweepWorker(provider *OAuth2Provider, orphanRetention time.Duration, logger schemas.Logger) *PerUserOAuthSweepWorker { + if logger == nil { + logger = bifrost.NewNoOpLogger() + } if provider == nil || provider.configStore == nil { - if logger != nil { - logger.Warn("per-user OAuth sweep worker not started: provider or config store is nil") - } + logger.Warn("per-user OAuth sweep worker not started: provider or config store is nil") return nil } return &PerUserOAuthSweepWorker{Please run this to confirm no current caller relies on passing a nil logger here (which would currently be silently tolerated but crash after this PR):
#!/bin/bash rg -nP -A3 'NewPerUserOAuthSweepWorker\(' --type=goAlso applies to: 225-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/oauth2/sync.go` around lines 161 - 198, `PerUserOAuthSweepWorker` still stores a nil logger and will panic after the new `w.logger != nil` guards were removed from `Start`/`Stop`/sweep methods. Update `NewPerUserOAuthSweepWorker` to mirror `NewTokenRefreshWorker` by defaulting a nil `logger` to `bifrost.NewNoOpLogger()` before assigning it to the worker struct. Keep the existing provider/configStore nil handling, and ensure `Start`, `Stop`, `sweepExpiredFlows`, and `sweepOrphanedTokens` can safely call `w.logger` without nil checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@framework/oauth2/sync.go`:
- Around line 161-198: `PerUserOAuthSweepWorker` still stores a nil logger and
will panic after the new `w.logger != nil` guards were removed from
`Start`/`Stop`/sweep methods. Update `NewPerUserOAuthSweepWorker` to mirror
`NewTokenRefreshWorker` by defaulting a nil `logger` to
`bifrost.NewNoOpLogger()` before assigning it to the worker struct. Keep the
existing provider/configStore nil handling, and ensure `Start`, `Stop`,
`sweepExpiredFlows`, and `sweepOrphanedTokens` can safely call `w.logger`
without nil checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f8bbdc03-93aa-4ca8-8b44-a704e59d98ea
📒 Files selected for processing (3)
framework/configstore/rdb.goframework/configstore/rdb_oauth2_test.goframework/oauth2/sync.go
a77ddfd to
95ae623
Compare
Merge activity
|
* upstream/dev: feat(mcp): add per-MCP-server tool execution timeout (maximhq#4472) fix: billing on failed responses stream requests anthropic and bedrock (maximhq#4842) fix: gemini openai through signature compatibility (maximhq#4810) fix: cancelled state in logs (maximhq#4831) fix: perplexity responses api compatibility (maximhq#4813) docs: clarify two-layer token refresh behavior and disabled-client refresh token expiry (maximhq#4849) fix: skip background token refresh for disabled/unconfigured MCP clients and guarantee non-nil logger in sync workers (maximhq#4848)
2 participants
Summary
Background OAuth token refresh was running indefinitely for tokens whose MCP clients were all disabled or removed. This meant the identity provider was being called on every tick for connections that nothing was actively consuming. This PR restricts
GetExpiringOauthTokensto only return tokens whoseoauth_configis referenced by at least one enabled MCP client, so background refresh stops for idle connections. When a client is re-enabled or a new one is attached,GetAccessTokenhandles the inline refresh on first use.Additionally, the
TokenRefreshWorkerandPerUserOAuthSweepWorkernow guarantee a non-nil logger by falling back to a no-op logger at construction time, removing all the scatteredif w.logger != nilguards throughout the sync worker code. Token refresh failures that recur on every tick (transient network errors) are now logged atDebuginstead ofErrorto avoid log spam, since permanent rejections are already surfaced by theoauth_configstatus flipping toexpired.Changes
GetExpiringOauthTokensnow includes anEXISTSsubquery requiring at least one non-disabledconfig_mcp_clientsrow joined throughoauth_configsto the token. Tokens with no config, or configs with only disabled clients, are excluded from background refresh.NewTokenRefreshWorkerassigns a no-op logger when the caller passesnil, allowing allif w.logger != nilguards insync.goto be removed unconditionally.ErrortoDebugfor transient failures; a comment explains that permanent failures are already captured via themarkExpiredIfPermanentpath.TestGetExpiringOauthTokens_ExcludesTerminalConfigsupdated to attach enabled MCP clients so the terminal-status condition remains the sole deciding factor in that test.TestGetExpiringOauthTokens_RequiresEnabledClientcovers the enabled-client requirement across four scenarios: enabled client, disabled-only client, mixed enabled/disabled clients on a shared config, and a config with no client rows.seedExpiringTokenFixturesandexpiringTokenIDsextracted to reduce duplication between the two test cases.Type of change
Affected areas
How to test
Expected: both
TestGetExpiringOauthTokens_ExcludesTerminalConfigsandTestGetExpiringOauthTokens_RequiresEnabledClientpass. Tokens with only disabled clients or no client rows must not appear in the refresh worker's selection.Breaking changes
Related issues
Security considerations
Reduces unnecessary outbound calls to identity providers for disabled or detached OAuth connections, limiting credential exposure surface during background refresh cycles.
Checklist
docs/contributing/README.mdand followed the guidelines