fix(keycardai-mcp): synchronous OAuth completion cleanup, no stale auth challenges#205
Open
Larry-Osakwe wants to merge 4 commits into
Open
fix(keycardai-mcp): synchronous OAuth completion cleanup, no stale auth challenges#205Larry-Osakwe wants to merge 4 commits into
Larry-Osakwe wants to merge 4 commits into
Conversation
… returning Completion cleanup (PKCE state delete, pending-auth clear, and the completion-route delete in CompletionRouter) previously ran via bare asyncio.create_task on the background path. The event loop keeps only a weak reference to such tasks, so they could be garbage-collected before running (or cancelled at shutdown with the CancelledError swallowed), leaving a stale pending-auth record that connected sessions kept advertising as an auth challenge. Both cleanup steps are fast storage calls, so they now always run synchronously before the completion result is returned, for every coordinator: - oauth_completion_handler awaits its cleanup unconditionally; the run_cleanup_in_background parameter is deprecated and ignored (a DeprecationWarning fires when it is passed explicitly) - CompletionRouter.route_completion awaits the route-metadata delete instead of scheduling a fire-and-forget task - a failure deleting the PKCE state no longer skips clearing the pending-auth record; each cleanup step is attempted independently - CancelledError is no longer swallowed by cleanup helpers - AuthCoordinator.requires_synchronous_cleanup is deprecated (kept for backward compatibility, returns True); OAuthStrategy no longer consults it and LocalAuthCoordinator's override is removed Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ession Session.get_auth_challenge() was a pure storage read, so a pending-auth record that outlived a completed authorization (for example when cleanup did not run) kept being advertised as an auth challenge after the session auto-reconnected. Consumers of get_auth_challenges() had to filter it out themselves until the record's TTL expired, and custom storage backends that ignore ttl surfaced it indefinitely. A CONNECTED session has no pending challenge by definition: get_auth_challenge() now returns None in that state and lazily clears any stale stored record. All other statuses keep the storage read, so a session in AUTH_PENDING still surfaces its challenge. This is defense in depth on top of the synchronous completion cleanup; the lazy clear only helps in-process, which is fine for a secondary guard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📦 Release PreviewThis analysis shows the expected release impact: 📈 Expected Version Changes📋 Package Details[
{
"package_name": "keycardai-mcp",
"package_dir": "packages/mcp",
"has_changes": true,
"current_version": "0.26.0",
"next_version": "0.27.0",
"increment": "MINOR"
}
]📝 Changelog PreviewThis comment was automatically generated by the release preview workflow. |
…in the getter The CONNECTED short-circuit in Session.get_auth_challenge() clobbered fresh re-auth challenges. A mid-session 401 (token expiry on a live tool call) is handled by the httpx transport, which writes a new pending-auth record without ever changing the session status, so a long-lived CONNECTED session with an expired token had its fresh challenge deleted by the getter and get_auth_challenges() returned [], hiding the re-auth URL from the user. A destructive write inside a getter that polling APIs hit repeatedly was also the wrong shape. get_auth_challenge() is a plain storage read again. The stale-record clear moves to the transition into CONNECTED in _initialize_session: reaching CONNECTED means auth works, so any record stored at that point is stale by definition. The clear runs once at the transition, is best-effort (storage errors never fail the connection), and cannot clobber a mid-session re-auth challenge because that is written while the session is already CONNECTED. The auto-reconnect path in on_completion_handled flows through connect() into _initialize_session, so one hook covers both paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Completion cleanup runs synchronously on the OAuth callback path, so a hung storage delete (remote backends such as Redis or DynamoDB) would stall the user-facing callback response indefinitely. Each cleanup storage call (PKCE state delete, pending-auth clear, completion-route delete) is now wrapped in asyncio.wait_for with a 5 second bound. A timeout falls through to the existing logged-and-swallowed per-step handling, so a timed-out step never skips the remaining steps. asyncio.wait_for raises TimeoutError (asyncio.TimeoutError on Python 3.10, an alias of the builtin on 3.11+), an Exception subclass on every supported Python, while outer cancellation (CancelledError, a BaseException) still propagates. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
📦 Release PreviewThis analysis shows the expected release impact: 📈 Expected Version Changes📋 Package Details[
{
"package_name": "keycardai-mcp",
"package_dir": "packages/mcp",
"has_changes": true,
"current_version": "0.26.0",
"next_version": "0.27.0",
"increment": "MINOR"
}
]📝 Changelog PreviewThis comment was automatically generated by the release preview workflow. |
Larry-Osakwe
marked this pull request as ready for review
July 21, 2026 23:05
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.
Fixes ECO-106: a completed OAuth authorization could leave a stale
auth_pendingrecord, so a connected session kept advertising an auth challenge. Observed in the tutorial-slack-agent walkthrough (the bot kept posting the connect prompt after a successful authorization; worked around in tutorial-slack-agent#4).Root cause
Completion cleanup (delete PKCE state, clear
auth_pending) ran as a bareasyncio.create_task(...)with no reference kept, so the event loop held only a weak ref and the task could be garbage-collected before running, silently. The cleanup body also swallowedCancelledError, and a failure deleting the PKCE state skipped theauth_pendingclear entirely.CompletionRouter.route_completionhad a second instance of the same fire-and-forget pattern.Session.get_auth_challenge()is a pure storage read, so nothing else ever cleared the record (bounded only by the 5-minute TTL on stock backends; unbounded on custom backends that ignore TTL).Fix
Commit 1: cleanup is always synchronous.
oauth_completion_handlerandCompletionRouter.route_completionnow await cleanup before returning; the background path is gone. Each cleanup step runs independently, so a PKCE-delete failure no longer skipsclear_auth_pending, andCancelledErrorpropagates instead of being swallowed. Cost is two storage deletes before the completion response returns; negligible for stock backends and the correctness guarantee for remote ones.The toggle is deprecated, not deleted, because both halves are public documented surface:
run_cleanup_in_backgroundstays as a parameter, retypedbool | None = None, ignored, and warns (DeprecationWarning) when passed explicitly. TheNonedefault means completion routes persisted by an older version mid-upgrade (with the kwarg in storedhandler_kwargs) still work, just with a warning.AuthCoordinator.requires_synchronous_cleanupstays, documented as deprecated and no longer consulted, default flipped toTrue(the honest value).LocalAuthCoordinator's override is removed.Commit 2 (reworked in d737b81): defense in depth at the CONNECTED transition. The first version of this cleared stale records lazily inside
get_auth_challenge(); adversarial review caught a real regression in that design: a mid-session token expiry writes a freshauth_pendingrecord via the transport without ever flipping session status, so a CONNECTED session's fresh re-auth challenge would have been deleted and hidden by the getter. Nowget_auth_challenge()is a pure, side-effect-free read, and the stale-record clear happens once at the transition into CONNECTED (_initialize_session, the only place that sets CONNECTED): reaching CONNECTED means auth works, so any lingering record is stale by definition, and a challenge written after connection survives. Regression guards: a fresh record on a CONNECTED session is returned, survives repeated polls, and is never deleted by reading it.Bounded cleanup (b408494, also from review). The synchronous cleanup ops are each wrapped in
asyncio.wait_for(5s), timing out into the existing logged-and-swallowed per-step path, so a hung remote backend cannot stall the user-facing OAuth callback, a timed-out PKCE delete still does not skip theauth_pendingclear, and outer cancellation still propagates.Acceptance
After a completed authorization and auto-reconnect,
client.get_auth_challenges()returns[]with no consumer-side filtering. Fourteen new tests after the review rework, including: a simulated dropped-cleanup stale record is cleared by the connect transition and yields no challenge; a fresh record written while CONNECTED is surfaced and never deleted by reads (the review's regression case); cleanup completes before the completion handler returns (asserted with no event-loop yield, so a dropped task would fail); hung storage ops time out without skipping later steps; PKCE-delete failure still clearsauth_pending; the deprecated kwarg warns on both values and cleanup still runs.packages/mcp: 558 passed / 16 skipped (baseline 542+16, no regressions). ruff clean.
Note: generated docs under
docs/sdk/still show the old signatures (mdxify viajust sdk-ref-mcp); left out per closed-alpha docs policy, happy to regenerate if wanted.🤖 Generated with Claude Code