Repository navigation
fix(sdk): coordinate concurrent token refresh - #1133
MatrixAdventurer wants to merge 6 commits into
Conversation
|
由 LW 授权的 ChatGPT Personal 智能体提交。 只读续接核验:当前head已从原交付cebc33e变为bd53e64111e63aa0f52152f5900cb7d8c0328e5e,新增c68e79d与bd53e641(SDK callback reentry、Web cookie/transport后续修订)。当前完整diff为9文件,已超出本任务每完整修复最多3文件限制;保留既有后续改动,不擅自回滚、拆分或省略测试,当前完整PR不能继续标记为符合本任务小范围约束。 实时CI:精确当前head的Static Checks run40200366444 success、Test Suite run40200366451 success、PR Title success;评论/inline列表暂无新增意见。原独立Approve只涵盖原cebc33e交付,不能延伸为当前9文件已独立Approve。后续代码变更需重新独立审查及范围决策;此记录不是自动合并授权。未修改代码、提交、分支或Issue状态。 |
由 LW 授权的 ChatGPT Personal 智能体提交。
83bf33f to
2bbde77
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for four independently reproduced issues in the current head: reentrant refresh can rotate without a bound; repeated logout of a consumed token flushes the shared authentication cache; the retention change materially increases database work; and delayed 401s can bypass the refresh notification outcome. Details and reproduction conditions are inline.
I also checked the prior issue scope and PR conversation. There are no existing inline review threads on this PR. The cross-request / multi-replica browser-session issue remains separately tracked by #1148.
| this.accessToken = data.access_token; | ||
| this.refreshTokenValue = data.refresh_token; | ||
| // Notification may reenter the client and need another refresh. | ||
| this.refreshAttempt = null; |
There was a problem hiding this comment.
[P1] Bound refresh when a notification reenters the client
onTokenRefresh is explicitly allowed to call this client, but clearing the attempt before awaiting it lets a 401 from that nested call start another refresh, whose notification repeats the same call. If /auth/refresh keeps succeeding while the requested resource keeps returning 401 (for example, an authorization/configuration failure), one original request produces an unbounded sequence of refreshes and token-row writes. I reproduced 13 refresh calls before the mock deliberately returned 503; without that artificial stop, nothing in this chain bounds it.
Please preserve supported callback reentry while bounding or suppressing recursive refresh from the notification chain, and cover a persistently unauthorized resource, not only a nested call that eventually recovers.
| let refresh_token_hash = sha256_hex(&request.refresh_token); | ||
| let stored = self | ||
| .fetch_refresh_token(&pool, &refresh_token_hash) | ||
| let _invalidation = self.authentication_cache.mutation(); |
There was a problem hiding this comment.
[P1] Do not flush every user's auth cache for replayed logout proofs
mutation() clears the entire process-local authentication cache both on entry and on drop, before this transaction establishes that the token's session has anything left to revoke. The new lookup accepts an unexpired consumed token, and repeated logout succeeds even after the session is already revoked. Therefore a former session holder can repeatedly post the same old refresh token to /auth/logout until it expires, forcing unrelated users' otherwise cached requests back onto the database each time. Previously the revoked-token lookup returned before entering this mutation guard.
I reproduced three successful logouts of one revoked token against an isolated MatrixOne and confirmed that an unrelated cached principal was evicted each time. Please validate and serialize the proof first, avoid global invalidation when no session state can change, and retain the existing in-flight-fill fence around a real revocation.
| let deleted = sqlx::query( | ||
| "DELETE FROM auth_refresh_tokens \ | ||
| WHERE (expires_at < NOW(6) OR is_revoked = 1) \ | ||
| WHERE expires_at < NOW(6) \ |
There was a problem hiding this comment.
[P2] Avoid the unbounded per-rotation history cost of logout proof
This changes revoked-token retention from the existing seven-day minimum to each token's full lifetime (30 days by default). With a Memoria session refreshing every 15 minutes, that is about 2,880 rows per continuously active session instead of 672; 42 such sessions retain about 120,960 rather than 28,224 rows. In an isolated MatrixOne fixture with the same indexes, the session MAX(expires_at) query's median rose from 1.39 ms to 2.14 ms, and its plan still scans the session's historical records. This is a small synthetic measurement, not a production latency claim.
The existing scheduler also runs one 1,000-row batch every six hours per process, so 42 continuously active 15-minute sessions already generate 4,032 rotations/day versus a 4,000-row/day cleanup ceiling; that throughput limitation predates this PR, but the longer retention materially amplifies the storage and scan cost. Please reconsider whether the existing signed, unexpired refresh JWT plus the canonical active session under the same account lock can supply the needed logout proof without retaining every consumed token, or otherwise establish a bounded cleanup design and measured acceptable cost without weakening revocation.
| let attempt = this.refreshAttempt; | ||
| if (attempt?.generation !== generation) { | ||
| // A delayed 401 may belong to a token another request already rotated. | ||
| if (rejectedToken !== this.accessToken) return Promise.resolve(true); |
There was a problem hiding this comment.
[P2] Do not let a delayed 401 bypass its rotation's notification result
Consider two requests sent with the old access token. The first rotates it and enters an asynchronous onTokenRefresh; while that notification is pending, the second's delayed 401 arrives. Because the refresh attempt was already cleared, this shortcut immediately replays the second request with the new token instead of joining the notification. I reproduced the second request returning 200 before the notification, followed by the notification failing and the first request returning its original 401.
The same case reproduces through WebRuntimeClient when cookie acquisition fails: a concurrent request succeeds even though the rotated pair was not persisted and the browser still holds the consumed refresh token. Please coordinate delayed old-token 401s with the notification outcome for that rotation while still permitting the callback itself to use the newly issued token.
Capability assessment: Unchanged: the change coordinates client authentication and session revocation without changing agent capabilities or policy.
Problem
Concurrent requests on one SDK client could race the server's single-use refresh token. Credential changes, stale refresh completions, live streams and Web's separate refresh implementation also had inconsistent ownership and replay behavior. A rotated refresh token could lose its logout proof to cleanup before the token expired, leaving its successor usable; a caller cancelled while waiting for a shared refresh could remain pending indefinitely.
Changes
Refs #1132.
Validation
ASTRA_SDK_E2E=1 npm run cipassed 500 tests with one unconfigured remote placeholder skipped; the additional abort-order regression passed in the focused suite. Typecheck, coverage and builds passed.npm run ci: 882 passed; typecheck and production build passed.cargo fmt --all --check,cargo clippy --locked -p astra-services --all-targets -- -D warnings, and 108 auth unit tests passed.Operational boundaries
Unexpired rotated-token rows remain stored longer so they can authenticate logout. The existing cleanup batch limit remains; high rotation volumes can outpace scheduled cleanup. An isolated MatrixOne EXPLAIN uses the expiration index but also reports a main-table scan and sort; this is not a claim of constant-cost cleanup.
Singleflight is per client. Independent Web requests or replicas can still compete for one browser refresh token, and Server Components cannot persist rotated cookies. This pre-existing cross-request behavior needs a shared authoritative browser-session design and is tracked in #1148.