Skip to content

fix(sdk): coordinate concurrent token refresh - #1133

Open
MatrixAdventurer wants to merge 6 commits into
matrixorigin:mainfrom
MatrixAdventurer:fix/sdk-concurrent-token-refresh
Open

MatrixAdventurer wants to merge 6 commits into
matrixorigin:mainfrom
MatrixAdventurer:fix/sdk-concurrent-token-refresh

Conversation

@MatrixAdventurer

@MatrixAdventurer MatrixAdventurer commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Coordinate token refresh per SDK instance across REST, buffered run events, live chat and Work streams. Fence in-flight work across credential replacement and order overlapping login/logout operations.
  • Treat access and refresh tokens as one pair. Explicit authorization does not use client-owned refresh credentials; raw mutation retries are opt-in and streaming retries follow an initial authentication rejection.
  • Let each refresh waiter cancel independently, remove its registration and preserve its abort reason without interrupting other waiters or replaying an aborted request.
  • Reuse the SDK transport in Web and update the affected test fixture.
  • Serialize logout against refresh using the existing account lock and revoke the exact session. Retain recorded tokens until both expiry and the existing minimum retention age so cleanup cannot remove an unexpired logout proof. Rotated tokens remain single-use for refresh; other sessions remain active.

Refs #1132.

Validation

  • Node 24.20.0: SDK ASTRA_SDK_E2E=1 npm run ci passed 500 tests with one unconfigured remote placeholder skipped; the additional abort-order regression passed in the focused suite. Typecheck, coverage and builds passed.
  • Web npm run ci: 882 passed; typecheck and production build passed.
  • Rust cargo fmt --all --check, cargo clippy --locked -p astra-services --all-targets -- -D warnings, and 108 auth unit tests passed.
  • Isolated MatrixOne integration: logout after rotation and cleanup, expired-record removal, refresh/logout races, and another session remaining valid passed. The ephemeral database was removed. The earlier auth-cache DB integration also passed before this cleanup-only Rust change.
  • JavaScript event-loop concurrency was exercised with deferred shared refreshes and cancellation races. Rust changes are a SQL retention predicate and tests, not shared-memory synchronization; a memory race detector cannot test database transaction ordering, which was exercised against MatrixOne.
  • GitHub CI for the new head is pending; local results do not imply it has 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.

Copy link
Copy Markdown
Contributor Author

由 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状态。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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) \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants