-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add proactive token refresh for WebSocket connections #264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WebSocket connections fetch an OAuth2 token at connection time but have no mechanism to refresh when the token expires during long-running subscriptions. This causes ACCESS_TOKEN_EXPIRED errors after ~1 hour. Changes: - AuthenticationManager: Track tokenIssuedAt, add getTokenExpiryTime(), getTokenIssuedAt(), getTokenLifetimeMs() methods - BaseClient: Add wrapper methods for token timing - WebSocketClient: Add calculateTokenRefreshTime() using max(50% lifetime, 2min before expiry), schedule proactive refresh via onTokenRefreshNeeded - SubscribeToUpdates: Accept onTokenExpiring and onTokenRefreshNeeded callbacks Applications can now receive notification before token expires and handle reconnection gracefully, resuming from their last processed offset.
|
Claude finished @HardlyDifficult's task —— View job Issues
Improvements
Full AnalysisOverall AssessmentThis is a well-designed solution to a real problem. The proactive token refresh mechanism addresses the Code QualityStrengths:
Documentation:
Token Refresh AlgorithmThe
Algorithm verification:
Security Considerations
Performance Considerations
ArchitectureFlow is well-designed:
The callback-based approach allows applications to implement their own reconnection strategies while the SDK handles the timing complexity. Test Coverage AssessmentMissing tests identified:
Existing test infrastructure:
Implementation DetailsWebSocketClient improvements:
Type safety:
Minor Suggestions
Integration ConsiderationsThe PR correctly identifies that integration testing requires manual verification with short-lived tokens. The callback-based design makes it easy to test the timing logic separately from the full WebSocket flow. ConclusionThis is a high-quality implementation that solves the stated problem with a robust, well-designed approach. The code follows best practices, maintains backward compatibility, and provides a clean API for consumers. The main gap is test coverage for the new functionality. Todo List:
|
|
Claude finished @HardlyDifficult's task —— View job Issues
Full AnalysisArchitecture & Design ✅The implementation follows a clean separation of concerns:
The token refresh strategy ( Code Quality ✅Strengths:
Implementation Details:
Security Considerations ✅
Performance Considerations ✅
Error Handling ✅
Test Coverage
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements proactive token refresh for WebSocket connections to prevent ACCESS_TOKEN_EXPIRED errors during long-running subscriptions. Tokens are refreshed at max(50% of lifetime, 2 minutes before expiry) to balance short-lived token protection with adequate buffer time.
Changes:
- Added token timing tracking (
tokenIssuedAt,tokenExpiryTime,tokenLifetimeMs) to authentication layer - Implemented token refresh scheduling in WebSocket client with configurable callbacks
- Exposed token refresh hooks through subscribe-to-updates API
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/core/auth/AuthenticationManager.ts | Track tokenIssuedAt timestamp and expose getters for token timing information |
| src/core/BaseClient.ts | Add wrapper methods to expose token timing from AuthenticationManager |
| src/core/ws/WebSocketClient.ts | Implement calculateTokenRefreshTime() function and proactive refresh scheduling with callbacks |
| src/clients/ledger-json-api/operations/v2/updates/subscribe-to-updates.ts | Accept and forward onTokenExpiring/onTokenRefreshNeeded callbacks to WebSocketClient |
| export function calculateTokenRefreshTime(tokenIssuedAt: number, tokenExpiresAt: number): number { | ||
| const lifetimeMs = tokenExpiresAt - tokenIssuedAt; | ||
| const twoMinutesMs = 2 * 60 * 1000; | ||
|
|
||
| const halfLifetime = tokenIssuedAt + Math.floor(lifetimeMs / 2); | ||
| const twoMinutesBefore = tokenExpiresAt - twoMinutesMs; | ||
|
|
||
| return Math.max(halfLifetime, twoMinutesBefore); | ||
| } |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function does not validate that tokenExpiresAt is after tokenIssuedAt. If tokenExpiresAt <= tokenIssuedAt, the calculation will produce a negative or zero lifetime, resulting in incorrect refresh times. Add validation to ensure tokenExpiresAt > tokenIssuedAt and throw an error if not.
| tokenRefreshTimer = setTimeout(() => { | ||
| void (async () => { | ||
| // Notify that token is expiring | ||
| if (options.onTokenExpiring) { | ||
| options.onTokenExpiring(); | ||
| } | ||
|
|
||
| await log('token_refresh_triggered', { reason: 'proactive_refresh' }); | ||
|
|
||
| // Get close params from callback | ||
| const closeParams = options.onTokenRefreshNeeded?.(); | ||
| const closeCode = closeParams?.code ?? 4000; | ||
| const closeReason = closeParams?.reason ?? 'Token refresh required'; | ||
|
|
||
| // Close the socket to trigger reconnection from the caller | ||
| socket.close(closeCode, closeReason); | ||
| })(); | ||
| }, delayMs); |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the onTokenExpiring callback throws an error, the subsequent onTokenRefreshNeeded callback and socket close will not execute, leaving the connection in an inconsistent state. Wrap the onTokenExpiring call in a try-catch block to ensure the refresh flow continues even if this callback fails.
| * @param tokenExpiresAt - Timestamp when token expires (ms since epoch) | ||
| * @returns Timestamp when refresh should be scheduled (ms since epoch) | ||
| */ | ||
| export function calculateTokenRefreshTime(tokenIssuedAt: number, tokenExpiresAt: number): number { |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculateTokenRefreshTime function implements critical token refresh logic but lacks test coverage. Add unit tests for edge cases: 60-second tokens (50% rule), 5-minute tokens (2-min buffer), 1-hour tokens (2-min buffer), and tokens that have already expired or have negative lifetimes.
| public getTokenExpiryTime(): number | null { | ||
| return this.tokenExpiry; | ||
| } | ||
|
|
||
| /** Returns the timestamp when the current token was issued, in milliseconds since epoch, or null if not available. */ | ||
| public getTokenIssuedAt(): number | null { | ||
| return this.tokenIssuedAt; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the token lifetime in milliseconds, or null if not available. Calculated as the difference between token | ||
| * expiry time and issue time. | ||
| */ | ||
| public getTokenLifetimeMs(): number | null { | ||
| if (this.tokenIssuedAt === null || this.tokenExpiry === null) { | ||
| return null; | ||
| } | ||
| return this.tokenExpiry - this.tokenIssuedAt; | ||
| } |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new token timing methods in AuthenticationManager lack test coverage. Other core modules like errors, http-client, logging, and polling have test files. Add tests for the new getters including edge cases where tokens are set via different authentication paths (OAuth2, tokenGenerator, bearerToken).
| let wsOptions: WebSocketOptions | undefined; | ||
| if (params.onTokenExpiring !== undefined || params.onTokenRefreshNeeded !== undefined) { | ||
| const options: WebSocketOptions = {}; | ||
| if (params.onTokenExpiring !== undefined) { | ||
| options.onTokenExpiring = params.onTokenExpiring; | ||
| } | ||
| if (params.onTokenRefreshNeeded !== undefined) { | ||
| options.onTokenRefreshNeeded = params.onTokenRefreshNeeded; | ||
| } | ||
| wsOptions = options; | ||
| } |
Copilot
AI
Jan 15, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional construction of wsOptions is verbose. Simplify by directly constructing the object with optional properties: const wsOptions = (params.onTokenExpiring || params.onTokenRefreshNeeded) ? { onTokenExpiring: params.onTokenExpiring, onTokenRefreshNeeded: params.onTokenRefreshNeeded } : undefined;. This is cleaner while achieving the same result.
| let wsOptions: WebSocketOptions | undefined; | |
| if (params.onTokenExpiring !== undefined || params.onTokenRefreshNeeded !== undefined) { | |
| const options: WebSocketOptions = {}; | |
| if (params.onTokenExpiring !== undefined) { | |
| options.onTokenExpiring = params.onTokenExpiring; | |
| } | |
| if (params.onTokenRefreshNeeded !== undefined) { | |
| options.onTokenRefreshNeeded = params.onTokenRefreshNeeded; | |
| } | |
| wsOptions = options; | |
| } | |
| const wsOptions: WebSocketOptions | undefined = | |
| params.onTokenExpiring !== undefined || params.onTokenRefreshNeeded !== undefined | |
| ? { | |
| onTokenExpiring: params.onTokenExpiring, | |
| onTokenRefreshNeeded: params.onTokenRefreshNeeded, | |
| } | |
| : undefined; |
| const refreshTime = calculateTokenRefreshTime(tokenIssuedAt, tokenExpiresAt); | ||
| const delayMs = Math.max(0, refreshTime - Date.now()); | ||
|
|
||
| if (delayMs > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Token refresh skipped when already past refresh time
Medium Severity
When delayMs equals 0 (meaning the calculated refresh time has already passed but the token hasn't expired yet), the condition delayMs > 0 is false and no timer is scheduled. The onTokenExpiring and onTokenRefreshNeeded callbacks are silently skipped, leaving the connection to fail unexpectedly when the token expires. Using >= instead of > would allow setTimeout(fn, 0) to trigger the callbacks immediately on the next event loop tick.
|
|
||
| await log('connect', { headers: token ? { Authorization: '[REDACTED]' } : undefined, protocols }); | ||
|
|
||
| // Schedule proactive token refresh if we have token timing and callbacks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Event handlers attached after async yield risks missed events
Medium Severity
The socket is created at line 90, but event handlers aren't attached until line 141. The new await log() calls at lines 105 and 113 yield to the event loop between these points. If the connection completes quickly (e.g., localhost) while logging performs I/O, the open event can fire before its handler is attached, causing it to be missed. The initial request message is never sent, leaving the WebSocket connected but non-functional.
Additional Locations (1)
|
@cursor review all the feedback from other bots in the PR here. Fix where you can make improvements otherwise tell me why they are wrong. |
1 similar comment
|
@cursor review all the feedback from other bots in the PR here. Fix where you can make improvements otherwise tell me why they are wrong. |
Summary
WebSocket connections in
canton-node-sdkfetch an OAuth2 token at connection time but have no mechanism to refresh when the token expires during long-running subscriptions. This causesACCESS_TOKEN_EXPIREDerrors after ~1 hour.This PR adds proactive token refresh support:
tokenIssuedAt, addgetTokenExpiryTime(),getTokenIssuedAt(),getTokenLifetimeMs()methodscalculateTokenRefreshTime()usingmax(50% lifetime, 2min before expiry), schedule proactive refresh viaonTokenRefreshNeededcallbackonTokenExpiringandonTokenRefreshNeededcallbacksToken Refresh Timing
The refresh time is calculated as:
max(50% of lifetime, 2 minutes before expiry)Usage Example
Test plan
Note
Introduces proactive token refresh to prevent mid-stream auth expiry in long-running WebSocket subscriptions.
core/ws/WebSocketClient: newWebSocketOptionswithonTokenExpiring/onTokenRefreshNeeded; schedules refresh viacalculateTokenRefreshTime(max 50% lifetime, 2min before expiry); logs schedule/trigger; closes socket with optional code/reason; clears timers on close.core/auth/AuthenticationManager: tracktokenIssuedAt/tokenExpiry; set on auth and generator; exposegetTokenExpiryTime(),getTokenIssuedAt(),getTokenLifetimeMs(); reset onclearToken().core/BaseClient: expose wrappers for token timing getters used byWebSocketClient.clients/.../subscribe-to-updates.ts: accepts and forwardsonTokenExpiring/onTokenRefreshNeededtoWebSocketClient; maintains existing message/error handling.Written by Cursor Bugbot for commit 34e1c59. This will update automatically on new commits. Configure here.