Bound HTTP and active-contracts requests with timeouts - #396
Merged
Conversation
Axios disables the socket timer when no timeout is configured, so a Canton endpoint that accepts the TCP connection and then goes silent suspended the awaiting caller forever. - Default every request to a 600s socket-inactivity timeout. Canton's slowest documented blocking wait is the command service tracking timeout (5 minutes), so this is a hang detector with 2x headroom rather than a latency budget. - Allow overrides per client (HttpClientOptions, ClientConfig.timeoutMs, ApiConfig.timeoutMs, CantonConfig.timeoutMs) and per request (RequestConfig.timeoutMs). - Report a timeout as NetworkError instead of an ApiError with no status, so reads retry and mutations surface as ambiguous outcomes. - Bound the bearer token fetch with the same timeout; a silent auth endpoint hung the request before dispatch. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
WebSocketClient only set handshakeTimeout, so a peer that completed the handshake and then stopped sending left getActiveContracts awaiting onClose forever. - Add an opt-in WebSocketOptions.idleTimeoutMs that closes the socket and reports an error when no message arrives in time. - Apply a 600s default to the active-contracts snapshot, which is a bounded self-closing stream; callers can override it or pass 0 to wait indefinitely. Long-lived subscriptions keep their unbounded behavior because idle periods are expected there. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe SDK adds configurable HTTP socket-inactivity timeouts and WebSocket idle timeouts. Settings apply at Canton, client, request, authentication, and operation levels. Timeout errors and idle WebSocket closures receive explicit handling and test coverage. ChangesTimeout controls
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Canton
participant BaseClient
participant AuthenticationManager
participant HttpClient
participant Axios
Canton->>BaseClient: provide timeoutMs
BaseClient->>AuthenticationManager: authenticate with timeoutMs and signal
BaseClient->>HttpClient: configure client timeout
HttpClient->>AuthenticationManager: acquire bearer token with timeout
HttpClient->>Axios: dispatch request with resolved timeout
Axios-->>HttpClient: response or timeout error
sequenceDiagram
participant GetActiveContracts
participant WebSocketClient
participant WebSocket
GetActiveContracts->>WebSocketClient: open snapshot with idleTimeoutMs
WebSocketClient->>WebSocket: start watchdog on open
WebSocket-->>WebSocketClient: deliver inbound message
WebSocketClient->>WebSocketClient: reset watchdog
WebSocketClient->>WebSocket: close with code 4008 after idle timeout
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
Oversized timer values can trigger immediate failures, and OS-level timeouts can produce misleading configured-timeout messages.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds bounded HTTP and active-contract stream inactivity timeouts to prevent indefinitely hung SDK requests.
Changes:
- Adds configurable client, service, request, and WebSocket timeouts.
- Normalizes HTTP timeout failures and bounds authentication waits.
- Adds unit and real-server timeout coverage.
File summaries
| File | Description |
|---|---|
src/Canton.ts |
Exposes the client-wide timeout. |
src/core/BaseClient.ts |
Resolves service/client timeout precedence. |
src/core/types.ts |
Defines timeout configuration fields. |
src/core/http/HttpClient.ts |
Implements HTTP and authentication timeouts. |
src/core/ws/WebSocketClient.ts |
Implements WebSocket idle detection. |
src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts |
Bounds snapshot stream inactivity. |
test/unit/core/http-client-timeout.test.ts |
Tests HTTP timeout configuration. |
test/unit/core/http-client-hang.test.ts |
Verifies behavior against silent servers. |
test/unit/core/client-timeout-config.test.ts |
Tests configuration precedence. |
test/unit/core/websocket-client.test.ts |
Tests WebSocket idle handling. |
test/unit/operations/get-active-contracts.test.ts |
Tests snapshot timeout forwarding. |
test/unit/clients/validator-api-health.test.ts |
Updates transport expectations. |
test/unit/clients/scan-api.test.ts |
Updates transport expectations. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+643
to
+645
| if (!Number.isFinite(timeoutMs) || timeoutMs < 0) { | ||
| throw new ConfigurationError(`${label} timeoutMs must be a non-negative finite number`); | ||
| } |
Comment on lines
+887
to
+890
| private isTimeoutError(error: unknown): error is AxiosError { | ||
| if (!axios.isAxiosError(error) || error.response !== undefined) return false; | ||
| return error.code === 'ECONNABORTED' || error.code === 'ETIMEDOUT'; | ||
| } |
Comment on lines
+80
to
+82
| if (!Number.isFinite(idleTimeoutMs) || idleTimeoutMs < 0) { | ||
| throw new ConfigurationError('WebSocket idleTimeoutMs must be a non-negative finite number'); | ||
| } |
Comment on lines
+72
to
+73
| * Socket-inactivity timeout in milliseconds for every request to this API. Overrides {@link ClientConfig.timeoutMs} | ||
| * and defaults to `DEFAULT_HTTP_TIMEOUT_MS`. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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.
Inline comments:
In `@src/core/BaseClient.ts`:
- Around line 50-54: Update the authentication callback in BaseClient so OAuth2
token requests use the configured timeout, rather than relying on the timeout
applied to the general HttpClient. Pass the resolved timeoutMs from the
surrounding configuration into the authentication manager or its authenticate
request, preserving the existing behavior when no timeout is configured.
In `@src/core/http/HttpClient.ts`:
- Around line 38-52: Shorten the comment above the default timeout in
HttpClient.ts to describe only the current socket-inactivity behavior and that
RequestConfig.timeoutMs can override it, without calling the override a floor or
including historical rationale and timeout comparisons. In
test/unit/core/http-client-hang.test.ts, replace the outage reference with a
concise description of a server that accepts a request but does not respond.
- Around line 517-533: Update the fetchBearerToken method to propagate
cancellation when the timeout wins: modify the provider parameter signature to
accept an AbortSignal, create an AbortController in the method body, pass the
signal to the provider call, and abort the controller in the finally block or
when timeoutPromise rejects so pending authentication operations are cancelled
instead of suspended indefinitely. Add a test that confirms the provider
receives an aborted signal when the timeout occurs before the token resolves.
In `@src/core/types.ts`:
- Around line 71-75: Update the JSDoc for timeoutMs in ApiConfig at
src/core/types.ts:71-75, ClientConfig at src/core/types.ts:122-126, and
CantonConfig at src/Canton.ts:68-72 to state that a value of 0 disables the
socket-inactivity timer while preserving the existing default and precedence
documentation.
In `@src/core/ws/WebSocketClient.ts`:
- Around line 79-82: Add an upper bound check to reject idleTimeoutMs values
exceeding the Node.js setTimeout limit of 2,147,483,647. In
src/core/ws/WebSocketClient.ts at lines 79-82, extend the validation condition
to also reject values greater than 2147483647 in the existing ConfigurationError
check around the idleTimeoutMs parameter validation. In
src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts at lines
48-49, add .max(2147483647) to the Zod schema validation for idleTimeoutMs. In
test/unit/core/websocket-client.test.ts after line 532, add a test case that
verifies rejection of idleTimeoutMs value 2147483648. In
test/unit/operations/get-active-contracts.test.ts after line 86, add a test case
that confirms the operation rejects idleTimeoutMs value 2147483648 before
attempting to connect.
In `@test/unit/core/client-timeout-config.test.ts`:
- Around line 63-87: Extend the timeout tests around LedgerJsonApiClient and
Canton to cover zero-valued opt-outs: assert ClientConfig.timeoutMs: 0 reaches
Axios, ApiConfig.timeoutMs: 0 overrides a positive client timeout, and
CantonConfig.timeoutMs: 0 propagates zero to every service client. Use
createdTimeouts() to verify the exact zero values while preserving the existing
positive-timeout cases.
In `@test/unit/core/http-client-hang.test.ts`:
- Around line 27-38: The tests in the hanging-request cases must always release
resources when assertions fail. Update the tests around the visible server setup
and streaming timer so `server.close()` and `clearInterval()` run from `finally`
blocks, while preserving the existing assertions and request behavior.
- Around line 55-74: Update the streaming test around HttpClient and its
setInterval callback so the client timeout is shorter than the 300 ms
AbortSignal deadline, while response bytes continue arriving more frequently
than that timeout. Preserve the existing abort assertion and elapsed-time check
so the test verifies inactivity timeout is reset by incoming data and the total
deadline ultimately stops the request.
In `@test/unit/core/http-client-timeout.test.ts`:
- Around line 42-126: Extend the “HttpClient timeouts” suite with tests for
transport timeout outcomes: verify a response-less Axios timeout retries a
semantic read, and verify a dispatched POST timeout rejects with
UnknownMutationOutcomeError. Reuse the existing createClient, Axios mocks, and
retry configuration helpers, and assert the read retry behavior and POST error
type explicitly.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73650a00-d1c6-4074-98c0-b5e8e1266680
📒 Files selected for processing (13)
src/Canton.tssrc/clients/ledger-json-api/operations/v2/state/get-active-contracts.tssrc/core/BaseClient.tssrc/core/http/HttpClient.tssrc/core/types.tssrc/core/ws/WebSocketClient.tstest/unit/clients/scan-api.test.tstest/unit/clients/validator-api-health.test.tstest/unit/core/client-timeout-config.test.tstest/unit/core/http-client-hang.test.tstest/unit/core/http-client-timeout.test.tstest/unit/core/websocket-client.test.tstest/unit/operations/get-active-contracts.test.ts
Comment on lines
+517
to
+533
| /** Bound the token fetch too: a silent auth endpoint would otherwise suspend the request before it is dispatched. */ | ||
| private async fetchBearerToken(provider: () => Promise<string>, timeoutMs: number): Promise<string> { | ||
| const tokenPromise = provider(); | ||
| if (timeoutMs === 0) return tokenPromise; | ||
|
|
||
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| const timeoutPromise = new Promise<never>((_resolve, reject) => { | ||
| timer = setTimeout(() => { | ||
| reject(new NetworkError(`Bearer token request timed out after ${timeoutMs}ms`)); | ||
| }, timeoutMs); | ||
| }); | ||
|
|
||
| try { | ||
| return await Promise.race([tokenPromise, timeoutPromise]); | ||
| } finally { | ||
| clearTimeout(timer); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cancel the bearer-token operation when its timeout wins.
Line 530 rejects the caller, but Promise.race does not cancel tokenPromise. The provider accepts no AbortSignal, so a silent authentication operation remains pending after the SDK returns NetworkError. Repeated timeouts can retain pending auth I/O and consume connections. Make the provider contract cancellable and propagate a combined timeout/request signal into the authentication transport. Add a test that verifies cancellation.
🤖 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 `@src/core/http/HttpClient.ts` around lines 517 - 533, Update the
fetchBearerToken method to propagate cancellation when the timeout wins: modify
the provider parameter signature to accept an AbortSignal, create an
AbortController in the method body, pass the signal to the provider call, and
abort the controller in the finally block or when timeoutPromise rejects so
pending authentication operations are cancelled instead of suspended
indefinitely. Add a test that confirms the provider receives an aborted signal
when the timeout occurs before the token resolves.
Comment on lines
+71
to
+75
| /** | ||
| * Socket-inactivity timeout in milliseconds for every request to this API. Overrides {@link ClientConfig.timeoutMs} | ||
| * and defaults to `DEFAULT_HTTP_TIMEOUT_MS`. | ||
| */ | ||
| timeoutMs?: number; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the 0 timeout opt-out at each public configuration layer.
These interfaces accept 0 and preserve it as an unbounded wait, but their JSDoc only describes defaults and precedence.
src/core/types.ts#L71-L75: State thatApiConfig.timeoutMs: 0disables the socket-inactivity timer.src/core/types.ts#L122-L126: State thatClientConfig.timeoutMs: 0disables the socket-inactivity timer.src/Canton.ts#L68-L72: State thatCantonConfig.timeoutMs: 0disables the socket-inactivity timer.
📍 Affects 2 files
src/core/types.ts#L71-L75(this comment)src/core/types.ts#L122-L126src/Canton.ts#L68-L72
🤖 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 `@src/core/types.ts` around lines 71 - 75, Update the JSDoc for timeoutMs in
ApiConfig at src/core/types.ts:71-75, ClientConfig at src/core/types.ts:122-126,
and CantonConfig at src/Canton.ts:68-72 to state that a value of 0 disables the
socket-inactivity timer while preserving the existing default and precedence
documentation.
Comment on lines
+79
to
+82
| const idleTimeoutMs = options?.idleTimeoutMs ?? 0; | ||
| if (!Number.isFinite(idleTimeoutMs) || idleTimeoutMs < 0) { | ||
| throw new ConfigurationError('WebSocket idleTimeoutMs must be a non-negative finite number'); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the repository's declared Node target and all idle-timeout validators.
fd -a -t f 'package.json' . -x rg -n '"node"|engines|packageManager' {}
rg -n -C 4 'idleTimeoutMs|setTimeout\(' src testRepository: Fairmint/canton-node-sdk
Length of output: 16778
🌐 Web query:
Node.js setTimeout maximum delay limit 2147483647
💡 Result:
In Node.js, the setTimeout and setInterval functions have a maximum supported delay of 2,147,483,647 milliseconds (which is 2^31 - 1) [1][2][3]. If you provide a delay value greater than 2,147,483,647, Node.js will automatically set the delay to 1 millisecond [1][2][3]. This same behavior applies if the provided delay is less than 1 or is NaN [1][3]. This limit exists because of historical implementation details requiring the delay to fit within a 32-bit signed integer [4][5]. While there have been community discussions about potentially changing this behavior to support larger values or throw an error, Node.js currently maintains this limit to avoid breaking existing applications [5][6]. If you need to schedule a callback for a delay longer than 2,147,483,647 milliseconds, the recommended approach is to implement a wrapper function that chains multiple timers or uses a recursive strategy to break the long duration into smaller, supported intervals [4][6]. For example, a common workaround is to use a recursive function: const MAX_DELAY = 2147483647; function safeSetTimeout(callback, delay) { if (delay > MAX_DELAY) { return setTimeout( => { safeSetTimeout(callback, delay - MAX_DELAY); }, MAX_DELAY); } else { return setTimeout(callback, delay); } }
Citations:
- 1: https://nodejs.org/api/timers.html
- 2: https://nodejs.org/docs/latest-v23.x/api/timers.html
- 3: https://nodejs.org/docs/latest-v26.x/api/timers.html
- 4: https://stackoverflow.com/questions/3468607/why-does-settimeout-break-for-large-millisecond-delay-values
- 5: Confused setTimout behaviour in NodeJS nodejs/node#46596
- 6: doc: add note about timeout delay > TIMEOUT_MAX nodejs/node#3512
🏁 Script executed:
# Check for any existing MAX_* or TIMEOUT constants related to idle or timer
rg -n 'MAX.*TIMEOUT|MAX.*DELAY|TIMER.*MAX|2147483647' src test --type ts --type tsx
# Check the complete WebSocketClient validator context
sed -n '75,90p' src/core/ws/WebSocketClient.ts
# Check the complete get-active-contracts Zod schema
sed -n '44,51p' src/clients/ledger-json-api/operations/v2/state/get-active-contracts.tsRepository: Fairmint/canton-node-sdk
Length of output: 1314
🏁 Script executed:
# Look at how idleTimeoutMs is used in WebSocketClient to understand the full impact
sed -n '189,210p' src/core/ws/WebSocketClient.ts
# Check if there are any integration tests that might use very large values
rg -n 'idleTimeoutMs.*[0-9]{8,}' test --type ts
# Look for any constants file where we should define the maximum
fd -type f 'constant|config' src --extension ts | head -10Repository: Fairmint/canton-node-sdk
Length of output: 1189
🏁 Script executed:
# Search for constant definitions in the source
find src -name '*.ts' -exec grep -l 'const.*MAX\|const.*TIMEOUT' {} \; | head -10
# Check if there's a constants file
ls -la src/core/ src/clients/ | grep -i const
# Look for places where large timeout values might be defined
rg -n 'DEFAULT.*IDLE|600_000|COMPLETION' src --type ts | head -20Repository: Fairmint/canton-node-sdk
Length of output: 1092
Apply a maximum bound to reject timer delays above the Node.js setTimeout limit.
Number.isFinite accepts values above 2,147,483,647 (2³¹ − 1). Node.js clamps setTimeout delays above this maximum to 1 millisecond. An allowed value such as 2,147,483,648 causes the socket to close immediately instead of waiting.
Add the maximum check in both validators. Add tests that verify rejection of the maximum plus one at each location.
src/core/ws/WebSocketClient.tslines 79–82: Add an upper bound check rejecting values greater than 2,147,483,647.src/clients/ledger-json-api/operations/v2/state/get-active-contracts.tslines 48–49: Add.max(2147483647)to the Zod schema.test/unit/core/websocket-client.test.tsafter line 532: Add a test that confirms rejection of 2,147,483,648.test/unit/operations/get-active-contracts.test.tsafter line 86: Add a test that confirms the operation rejects 2,147,483,648 before connecting.
📍 Affects 4 files
src/core/ws/WebSocketClient.ts#L79-L82(this comment)src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts#L48-L49test/unit/core/websocket-client.test.ts#L514-L532test/unit/operations/get-active-contracts.test.ts#L72-L86
🤖 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 `@src/core/ws/WebSocketClient.ts` around lines 79 - 82, Add an upper bound
check to reject idleTimeoutMs values exceeding the Node.js setTimeout limit of
2,147,483,647. In src/core/ws/WebSocketClient.ts at lines 79-82, extend the
validation condition to also reject values greater than 2147483647 in the
existing ConfigurationError check around the idleTimeoutMs parameter validation.
In src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts at
lines 48-49, add .max(2147483647) to the Zod schema validation for
idleTimeoutMs. In test/unit/core/websocket-client.test.ts after line 532, add a
test case that verifies rejection of idleTimeoutMs value 2147483648. In
test/unit/operations/get-active-contracts.test.ts after line 86, add a test case
that confirms the operation rejects idleTimeoutMs value 2147483648 before
attempting to connect.
Comment on lines
+63
to
+87
| it('applies a client-wide timeout', () => { | ||
| new LedgerJsonApiClient(new CantonRuntime(createClientConfig({ timeoutMs: 30_000 }))); | ||
|
|
||
| expect(createdTimeouts()).toEqual([30_000]); | ||
| }); | ||
|
|
||
| it('prefers a per-API timeout over the client-wide timeout', () => { | ||
| new LedgerJsonApiClient(new CantonRuntime(createClientConfig({ timeoutMs: 30_000 }, 5_000))); | ||
|
|
||
| expect(createdTimeouts()).toEqual([5_000]); | ||
| }); | ||
|
|
||
| it('propagates the Canton-level timeout to every service client', () => { | ||
| new Canton({ | ||
| network: 'localnet', | ||
| authUrl: 'https://auth.example', | ||
| timeoutMs: 45_000, | ||
| apis: { | ||
| LEDGER_JSON_API: { apiUrl: 'https://ledger.example', auth }, | ||
| VALIDATOR_API: { apiUrl: 'https://validator.example', auth }, | ||
| SCAN_API: { apiUrl: 'https://scan.example', auth }, | ||
| }, | ||
| }); | ||
|
|
||
| expect(createdTimeouts()).toEqual([45_000, 45_000, 45_000]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Test zero-valued timeout precedence.
The cases use only positive values. 0 is a documented opt-out and must not be treated as missing. Add cases for ClientConfig.timeoutMs: 0, ApiConfig.timeoutMs: 0 over a positive client value, and CantonConfig.timeoutMs: 0; assert that Axios receives zero.
As per coding guidelines, **/*.{ts,tsx} requires: “Use runnable examples and tests as sources of truth for exact behavior when changing the Canton Node SDK.”
🤖 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 `@test/unit/core/client-timeout-config.test.ts` around lines 63 - 87, Extend
the timeout tests around LedgerJsonApiClient and Canton to cover zero-valued
opt-outs: assert ClientConfig.timeoutMs: 0 reaches Axios, ApiConfig.timeoutMs: 0
overrides a positive client timeout, and CantonConfig.timeoutMs: 0 propagates
zero to every service client. Use createdTimeouts() to verify the exact zero
values while preserving the existing positive-timeout cases.
Source: Coding guidelines
Comment on lines
+55
to
+74
| it('honors an AbortSignal deadline while the socket keeps trickling data', async () => { | ||
| // The inactivity timer never fires here because bytes keep arriving; only a hard deadline can stop this request. | ||
| const timers: Array<ReturnType<typeof setInterval>> = []; | ||
| const server = await startServer((_request, response) => { | ||
| response.writeHead(200, { 'Content-Type': 'application/json' }); | ||
| response.write('['); | ||
| timers.push( | ||
| setInterval(() => { | ||
| response.write(' '); | ||
| }, 20) | ||
| ); | ||
| }); | ||
| const client = new HttpClient(undefined, undefined, { timeoutMs: 60_000 }); | ||
| client.setRetryConfig({ maxRetries: 0, delayMs: 0 }); | ||
|
|
||
| const startedAt = Date.now(); | ||
| await expect( | ||
| client.makeGetRequest(`${server.url}/v2/state/active-contracts`, {}, { signal: AbortSignal.timeout(300) }) | ||
| ).rejects.toMatchObject({ name: 'AbortError' }); | ||
| expect(Date.now() - startedAt).toBeLessThan(5000); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the streaming test distinguish inactivity from a total deadline.
Line 67 sets 60,000 ms, while Line 72 aborts at 300 ms. Both timeout models remain pending at 300 ms, so the assertion cannot prove that received bytes reset the timer. Set the client timeout below the abort deadline and keep writes more frequent than that timeout.
As per coding guidelines, **/*.{ts,tsx} requires: “Use runnable examples and tests as sources of truth for exact behavior when changing the Canton Node SDK.”
🤖 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 `@test/unit/core/http-client-hang.test.ts` around lines 55 - 74, Update the
streaming test around HttpClient and its setInterval callback so the client
timeout is shorter than the 300 ms AbortSignal deadline, while response bytes
continue arriving more frequently than that timeout. Preserve the existing abort
assertion and elapsed-time check so the test verifies inactivity timeout is
reset by incoming data and the total deadline ultimately stops the request.
Source: Coding guidelines
Comment on lines
+42
to
+126
| describe('HttpClient timeouts', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('applies the default socket timeout to the axios instance', () => { | ||
| const client = createClient(); | ||
|
|
||
| expect(createdTimeouts()).toEqual([DEFAULT_HTTP_TIMEOUT_MS]); | ||
| expect(client.getDefaultTimeoutMs()).toBe(DEFAULT_HTTP_TIMEOUT_MS); | ||
| }); | ||
|
|
||
| it('defaults to a timeout larger than the Canton command tracking timeout', () => { | ||
| // Canton holds submit-and-wait responses open for up to CommandServiceConfig.DefaultDefaultTrackingTimeout (5min). | ||
| expect(DEFAULT_HTTP_TIMEOUT_MS).toBeGreaterThan(5 * 60 * 1000); | ||
| }); | ||
|
|
||
| it('applies a client-level timeout override', async () => { | ||
| const client = createClient(1234); | ||
|
|
||
| expect(createdTimeouts()).toEqual([1234]); | ||
|
|
||
| await client.makeGetRequest('https://ledger.example/v2/version'); | ||
|
|
||
| const [, config] = lastAxiosInstance().get.mock.calls[0] as [string, { timeout: number }]; | ||
| expect(config.timeout).toBe(1234); | ||
| }); | ||
|
|
||
| it('applies a per-request timeout override', async () => { | ||
| const client = createClient(1234); | ||
|
|
||
| await client.makeGetRequest('https://ledger.example/v2/version', { timeoutMs: 42 }); | ||
|
|
||
| const [, config] = lastAxiosInstance().get.mock.calls[0] as [string, { timeout: number }]; | ||
| expect(config.timeout).toBe(42); | ||
| }); | ||
|
|
||
| it('falls back to the client timeout when a request does not override it', async () => { | ||
| const client = createClient(); | ||
|
|
||
| await client.makePostRequest('https://ledger.example/v2/commands/submit', { commands: [] }); | ||
|
|
||
| const [, , config] = lastAxiosInstance().post.mock.calls[0] as [string, unknown, { timeout: number }]; | ||
| expect(config.timeout).toBe(DEFAULT_HTTP_TIMEOUT_MS); | ||
| }); | ||
|
|
||
| it('allows a request to opt out of the timeout', async () => { | ||
| const client = createClient(); | ||
|
|
||
| await client.makeGetRequest('https://ledger.example/v2/version', { timeoutMs: 0 }); | ||
|
|
||
| const [, config] = lastAxiosInstance().get.mock.calls[0] as [string, { timeout: number }]; | ||
| expect(config.timeout).toBe(0); | ||
| }); | ||
|
|
||
| it('rejects invalid client timeouts', () => { | ||
| expect(() => new HttpClient(undefined, undefined, { timeoutMs: -1 })).toThrow(ConfigurationError); | ||
| expect(() => new HttpClient(undefined, undefined, { timeoutMs: Number.NaN })).toThrow(ConfigurationError); | ||
| }); | ||
|
|
||
| it('rejects invalid per-request timeouts', async () => { | ||
| const client = createClient(); | ||
|
|
||
| await expect(client.makeGetRequest('https://ledger.example/v2/version', { timeoutMs: -1 })).rejects.toThrow( | ||
| ConfigurationError | ||
| ); | ||
| }); | ||
|
|
||
| it('bounds a bearer token provider that never resolves', async () => { | ||
| const client = new HttpClient( | ||
| undefined, | ||
| async () => | ||
| new Promise<string>(() => { | ||
| // A silent auth endpoint would otherwise suspend the request before it is dispatched. | ||
| }), | ||
| { timeoutMs: 50 } | ||
| ); | ||
| client.setRetryConfig({ maxRetries: 0, delayMs: 0 }); | ||
|
|
||
| await expect( | ||
| client.makeGetRequest('https://ledger.example/v2/version', { includeBearerToken: true }) | ||
| ).rejects.toThrow(new NetworkError('Bearer token request timed out after 50ms')); | ||
| expect(lastAxiosInstance().get).not.toHaveBeenCalled(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Test the timeout outcome contract.
This suite verifies configuration and a pre-dispatch token timeout, but it does not assert the changed transport outcomes. Add a response-less Axios timeout that retries a semantic read and a dispatched POST timeout that rejects as UnknownMutationOutcomeError.
As per coding guidelines, **/*.{ts,tsx} requires: “Use runnable examples and tests as sources of truth for exact behavior when changing the Canton Node SDK.”
🤖 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 `@test/unit/core/http-client-timeout.test.ts` around lines 42 - 126, Extend the
“HttpClient timeouts” suite with tests for transport timeout outcomes: verify a
response-less Axios timeout retries a semantic read, and verify a dispatched
POST timeout rejects with UnknownMutationOutcomeError. Reuse the existing
createClient, Axios mocks, and retry configuration helpers, and assert the read
retry behavior and POST error type explicitly.
Source: Coding guidelines
fetchBearerToken raced its internal setTimeout against the caller's work via Promise.race, but nothing canceled the loser. When a caller aborted while the race was still pending, the setTimeout kept a live timer handle alive for up to the full timeoutMs (600_000ms by default), even though the outer request had already settled. Thread the caller's AbortSignal into fetchBearerToken (via buildHeaders) and clear the timer immediately on abort, mirroring the existing onAbort/removeEventListener pattern already used by abortableSleep. Add a regression test that uses fake timers to assert the timer count drops to 0 immediately after abort; it fails on the prior code (jest.getTimerCount() stays at 1) and passes with the fix. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
No internal call site ever set ApiConfig.timeoutMs; only ClientConfig.timeoutMs/CantonConfig.timeoutMs is ever assigned, and DEFAULT_HTTP_TIMEOUT_MS already has documented 2x headroom over every per-service bound in the codebase. This was speculative surface with zero current motivation. Collapses timeout configuration to 2 levels: client-wide default (ClientConfig.timeoutMs/CantonConfig.timeoutMs) + per-request override (RequestConfig.timeoutMs). Updates client-timeout-config.test.ts to drop the per-service precedence test and assert timeouts via an actual per-request axios call rather than the axios.create() call args, and adds coverage for the per-request override. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
Every dispatched request already passes an explicit, always-resolved
'timeout' in its per-request axios config (see makeRequest/
dispatchRequest), so the axios.create({ timeout }) default set at
construction time was dead: axios never falls back to it. Drop it so
the per-request value is the single source of truth.
getDefaultTimeoutMs() had no consumer besides its own test, so remove
it too rather than keep unused public API.
Removes the now-redundant assertions in http-client-timeout.test.ts
that checked axios.create() call args and getDefaultTimeoutMs(), and
the tautological 'DEFAULT_HTTP_TIMEOUT_MS > 5*60*1000' test (two
hardcoded constants compared to each other proves nothing about actual
Canton behavior; the rationale is already documented on the
DEFAULT_HTTP_TIMEOUT_MS constant itself).
Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
Both constants existed independently at the same 600_000ms literal. Reference DEFAULT_HTTP_TIMEOUT_MS directly instead of duplicating the value, with a comment noting they may diverge later if the idle timeout needs service-specific tuning. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/BaseClient.ts (1)
50-54: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound
BaseClient.authenticate()with the client timeout.The public
BaseClient.authenticate()method directly callsAuthenticationManager.authenticate()at Line 63. An unresponsive auth server bypasses the timeout configured here and leaves that public operation pending. Apply the same client timeout andNetworkErrornormalization to this path. Preserve0as the opt-out. Add a hanging-authentication test forBaseClient.authenticate().🤖 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 `@src/core/BaseClient.ts` around lines 50 - 54, Update BaseClient.authenticate() to execute AuthenticationManager.authenticate() through the configured client timeout and normalize timeout failures as NetworkError, matching the existing HttpClient behavior. Preserve timeoutMs === 0 as the opt-out and ensure the public method does not remain pending against an unresponsive auth server. Add a test that verifies BaseClient.authenticate() rejects appropriately when authentication hangs.src/core/http/HttpClient.ts (1)
531-549: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the abort listener after a successful token fetch.
When
provider()resolves before abort,finallyclearstimerbut leavesonAbortregistered onsignal. Reusing a long-lived signal accumulates listeners for completed requests. KeeponAbortin outer scope and remove it infinally.🤖 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 `@src/core/http/HttpClient.ts` around lines 531 - 549, Update the timeout handling around the token request so the abort callback remains accessible after Promise.race completes, then remove it from signal in the finally block alongside clearing timer. Preserve the existing timeout and abort rejection behavior while ensuring successful token fetches do not leave listeners registered.
🤖 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.
Inline comments:
In `@src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts`:
- Around line 26-30: The comment above the timeout constant includes design
rationale and future-planning text that should be removed per coding guidelines.
Shorten the comment to describe only the current behavior: that it intentionally
reuses DEFAULT_HTTP_TIMEOUT_MS. Remove the explanation about detecting hung
connections and the statement about potential future divergence, keeping only a
brief statement of the current design choice.
In `@test/unit/core/http-client-retry.test.ts`:
- Around line 592-593: Shorten the comment near the token-fetch timeout
assertion to state only that abort must clear the timeout immediately. Remove
the historical explanation, regression context, and timeout duration details
while preserving the assertion unchanged.
---
Outside diff comments:
In `@src/core/BaseClient.ts`:
- Around line 50-54: Update BaseClient.authenticate() to execute
AuthenticationManager.authenticate() through the configured client timeout and
normalize timeout failures as NetworkError, matching the existing HttpClient
behavior. Preserve timeoutMs === 0 as the opt-out and ensure the public method
does not remain pending against an unresponsive auth server. Add a test that
verifies BaseClient.authenticate() rejects appropriately when authentication
hangs.
In `@src/core/http/HttpClient.ts`:
- Around line 531-549: Update the timeout handling around the token request so
the abort callback remains accessible after Promise.race completes, then remove
it from signal in the finally block alongside clearing timer. Preserve the
existing timeout and abort rejection behavior while ensuring successful token
fetches do not leave listeners registered.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 91dc195a-0420-41f6-b606-57d790e78dee
📒 Files selected for processing (8)
src/Canton.tssrc/clients/ledger-json-api/operations/v2/state/get-active-contracts.tssrc/core/BaseClient.tssrc/core/http/HttpClient.tssrc/core/types.tstest/unit/core/client-timeout-config.test.tstest/unit/core/http-client-retry.test.tstest/unit/core/http-client-timeout.test.ts
💤 Files with no reviewable changes (1)
- test/unit/core/http-client-timeout.test.ts
Comment on lines
+26
to
+30
| * | ||
| * Intentionally reuses {@link DEFAULT_HTTP_TIMEOUT_MS} rather than an independent literal: both exist to detect a hung | ||
| * connection at the same "this is clearly stuck" horizon. They may diverge in the future if either bound needs | ||
| * service-specific tuning. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Shorten the timeout comment to the current contract.
The comment includes design rationale and future-planning text. Keep only the current behavior.
Suggested change
/**
- * Intentionally reuses {`@link` DEFAULT_HTTP_TIMEOUT_MS} rather than an independent literal: both exist to detect a hung
- * connection at the same "this is clearly stuck" horizon. They may diverge in the future if either bound needs
- * service-specific tuning.
+ * Default idle timeout for active-contract WebSocket streams.
*/As per coding guidelines, **/*.{ts,tsx,js,jsx} comments must be brief and describe only the current state; they must not include historical explanations or verbose write-ups.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| * | |
| * Intentionally reuses {@link DEFAULT_HTTP_TIMEOUT_MS} rather than an independent literal: both exist to detect a hung | |
| * connection at the same "this is clearly stuck" horizon. They may diverge in the future if either bound needs | |
| * service-specific tuning. | |
| */ | |
| * | |
| * Default idle timeout for active-contract WebSocket streams. | |
| */ |
🤖 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 `@src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts`
around lines 26 - 30, The comment above the timeout constant includes design
rationale and future-planning text that should be removed per coding guidelines.
Shorten the comment to describe only the current behavior: that it intentionally
reuses DEFAULT_HTTP_TIMEOUT_MS. Remove the explanation about detecting hung
connections and the statement about potential future divergence, keeping only a
brief statement of the current design choice.
Source: Coding guidelines
BaseClient.authenticate() called AuthenticationManager.authenticate() directly, bypassing HttpClient's timeout entirely. The rest-client delegate it wraps makes its own axios calls with no timeout set, so a hung auth server left authenticate() pending forever. AuthenticationManager.authenticate() now accepts a timeoutMs (default DEFAULT_HTTP_TIMEOUT_MS) and races the delegate call against it, mirroring HttpClient.fetchBearerToken's pattern. BaseClient threads its configured timeoutMs into both the public authenticate() method and the bearer-token provider passed to HttpClient. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
fetchBearerToken registered its onAbort listener with { once: true }
but only removed it when abort actually fired. A successful (or
timed-out) token fetch on a long-lived, reused AbortSignal left the
listener attached, accumulating over repeated calls.
Clean up the listener in a finally after the race settles, regardless
of outcome.
Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
CodeRabbit nitpicks: drop speculative future-tuning text in get-active-contracts.ts and historical-bug wording in http-client-retry.test.ts, per the repo's concise-comments guideline. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/core/http/HttpClient.ts (1)
509-509: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass the resolved request timeout to the bearer-token provider.
HttpClientresolves the request timeout, butprovider()cannot receive it.BaseClienttherefore captures onlyclientConfig.timeoutMsforAuthenticationManager.authenticate().A request timeout of
0still expires at the client default. A request timeout greater than the client timeout also expires early during authentication. Change the provider contract to accepttimeoutMs, call it with the resolved value, and forward that value fromsrc/core/BaseClient.ts.
src/core/http/HttpClient.ts#L509-L509: call the bearer-token provider with the resolvedtimeoutMs.src/core/BaseClient.ts#L50-L55: forward the provider timeout toAuthenticationManager.authenticate()instead of capturing onlyclientConfig.timeoutMs.🤖 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 `@src/core/http/HttpClient.ts` at line 509, The bearer-token provider does not receive the resolved request timeout, causing authentication to use the client default instead of the actual resolved timeout. At src/core/http/HttpClient.ts line 509, update the fetchBearerToken call to pass the resolved timeoutMs value to the provider. At src/core/BaseClient.ts lines 50-55, update the AuthenticationManager.authenticate() call to forward the resolved timeout from the provider instead of using only clientConfig.timeoutMs, ensuring that request timeouts of 0 and values exceeding the client timeout are properly handled during authentication.src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts (1)
52-53: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCap
idleTimeoutMsat2_147_483_647ms. Node normalizes largersetTimeoutdelays to 1 ms, so a large timeout can close a silent stream almost immediately. Alternatively, implement chunked scheduling inWebSocketClient.🤖 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 `@src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts` around lines 52 - 53, Cap the idleTimeoutMs schema validation in the active-contracts configuration at 2_147_483_647 milliseconds, rejecting larger values while preserving the existing non-negative and optional behavior. Update the z.number() definition for idleTimeoutMs; do not change unrelated timeout handling.
🤖 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.
Inline comments:
In `@test/unit/core/authenticate-hang.test.ts`:
- Around line 69-80: The test does not verify that concurrent callers actually
share a single in-flight request, since two independent token requests would
also cause both authenticate calls to reject with NetworkError within the
timeout. Modify the startServer mock to track the number of requests it
receives, then add an assertion after the Promise.all rejects to verify the
server received exactly one request, confirming that both client.authenticate()
and other.authenticate() calls reused the same authentication request instead of
making independent calls.
- Around line 45-55: Wrap each test's execution logic in try/finally blocks to
ensure server instances are always closed. In
test/unit/core/authenticate-hang.test.ts at lines 45-55, move the startServer
call and all assertions into a try block, and place the server.close() call in a
finally block. Apply the same pattern at
test/unit/core/authenticate-hang.test.ts lines 58-66 where the timeout-message
server is created and used. Apply the same pattern at
test/unit/core/authenticate-hang.test.ts lines 69-81 for the concurrent-caller
server. Apply the same pattern at test/unit/core/http-client-hang.test.ts lines
81-96 for the repeated-token-fetch server. This ensures server.close() executes
even if test assertions fail, preventing hanging sockets from blocking Jest
shutdown.
---
Outside diff comments:
In `@src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts`:
- Around line 52-53: Cap the idleTimeoutMs schema validation in the
active-contracts configuration at 2_147_483_647 milliseconds, rejecting larger
values while preserving the existing non-negative and optional behavior. Update
the z.number() definition for idleTimeoutMs; do not change unrelated timeout
handling.
In `@src/core/http/HttpClient.ts`:
- Line 509: The bearer-token provider does not receive the resolved request
timeout, causing authentication to use the client default instead of the actual
resolved timeout. At src/core/http/HttpClient.ts line 509, update the
fetchBearerToken call to pass the resolved timeoutMs value to the provider. At
src/core/BaseClient.ts lines 50-55, update the
AuthenticationManager.authenticate() call to forward the resolved timeout from
the provider instead of using only clientConfig.timeoutMs, ensuring that request
timeouts of 0 and values exceeding the client timeout are properly handled
during authentication.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bced8b12-0b94-4d68-9191-586004d523a0
📒 Files selected for processing (7)
src/clients/ledger-json-api/operations/v2/state/get-active-contracts.tssrc/core/BaseClient.tssrc/core/auth/AuthenticationManager.tssrc/core/http/HttpClient.tstest/unit/core/authenticate-hang.test.tstest/unit/core/http-client-hang.test.tstest/unit/core/http-client-retry.test.ts
… abort withAuthTimeout raced the delegate.authenticate() call against a bare setTimeout with no way to cancel early. The bearer-token provider passed from BaseClient into HttpClient already receives the request's AbortSignal in fetchBearerToken, but never forwarded it into the provider call, so authenticate() had no signal to hook into and its timer kept running for the full timeoutMs after the caller aborted. Thread the signal through the whole chain: HttpClient.fetchBearerToken now calls provider(signal), BaseClient's provider closure and its public authenticate() forward it to AuthenticationManager.authenticate(), and withAuthTimeout clears its timer immediately on abort, mirroring fetchBearerToken's onAbort/removeEventListener/finally pattern. Also strengthen the concurrent-callers test in authenticate-hang.test.ts to assert the server received exactly one request (proving deduplication, not just that both calls settled), and wrap every test's server lifecycle in try/finally so a failing assertion cannot leak an open listening socket. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
A silent endpoint that always times out was being retried like any other transient transport failure, so maxRetries could multiply the configured timeoutMs wait by up to (maxRetries + 1)x. Retrying a genuine socket timeout doesn't plausibly help: the endpoint was already silent for the full configured window, so retrying within the same short backoff won't suddenly get a response. Fail fast instead so the caller sees the timeout immediately and can decide to retry themselves with full visibility into elapsed time. isRetryableError() now returns false for errors isTimeoutError() classifies as our own socket-inactivity timeout, before falling through to the existing axios status/network-error checks. Genuinely transient transport failures (connection reset, 5xx, etc.) are unaffected. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/core/auth/AuthenticationManager.ts`:
- Around line 74-86: Keep AbortSignal cancellation effective when timeoutMs is
zero: in src/core/auth/AuthenticationManager.ts lines 74-86, reject pre-aborted
signals before invoking delegate.authenticate(); in lines 155-180, race every
authentication wait against the signal and add the timeout race only when
timeoutMs > 0; in src/core/http/HttpClient.ts lines 529-556, likewise race
bearer-token retrieval against the signal even when timeoutMs === 0. Add tests
covering pre-aborted signals and aborting zero-timeout authentication and
bearer-token requests.
In `@src/core/BaseClient.ts`:
- Around line 59-63: Shorten the comments at src/core/BaseClient.ts lines 59-63
to state only that authenticate uses the configured timeout and accepts an
optional signal; at src/core/http/HttpClient.ts lines 522-528 state that
bearer-token retrieval is bounded and cleans up its timer and listener, and at
lines 809-813 state that socket timeouts are not retried; at
src/core/auth/AuthenticationManager.ts lines 65-72 state that the wrapper bounds
caller wait without canceling the delegate, and at lines 146-153 state that 0
disables the timeout timer and cleanup occurs on settlement. Remove rationale,
historical context, and repeated lifecycle details while preserving the behavior
and references to authenticate, bearer-token retrieval, socket timeout handling,
and the authentication wrapper.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b4ae904-48be-4ab5-9239-96b215d7d28c
📒 Files selected for processing (6)
src/core/BaseClient.tssrc/core/auth/AuthenticationManager.tssrc/core/http/HttpClient.tstest/unit/core/authenticate-hang.test.tstest/unit/core/http-client-hang.test.tstest/unit/core/http-client-retry.test.ts
Comment on lines
+59
to
+63
| /** | ||
| * Bounded by the client's configured `timeoutMs`, same as every other request this client makes. `signal` is | ||
| * optional since this is a standalone public method with no caller-supplied signal today; future callers that do | ||
| * have one can pass it through to stop waiting immediately on abort instead of the full `timeoutMs`. | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Shorten timeout comments.
Keep these comments to the current behavior. Remove implementation rationale and repeated lifecycle detail.
src/core/BaseClient.ts#L59-L63: state thatauthenticateuses the configured timeout and accepts an optional signal.src/core/http/HttpClient.ts#L522-L528: state that bearer-token retrieval is bounded and cleans up its timer and listener.src/core/http/HttpClient.ts#L809-L813: state that socket timeouts are not retried.src/core/auth/AuthenticationManager.ts#L65-L72: state that the wrapper bounds caller wait but does not cancel the delegate.src/core/auth/AuthenticationManager.ts#L146-L153: state that0disables the timeout timer and that cleanup occurs on settlement.
As per coding guidelines, **/*.{ts,tsx,js,jsx} requires: “Keep code comments brief and describe only the current state; do not include historical explanations or verbose write-ups.”
📍 Affects 3 files
src/core/BaseClient.ts#L59-L63(this comment)src/core/http/HttpClient.ts#L522-L528src/core/http/HttpClient.ts#L809-L813src/core/auth/AuthenticationManager.ts#L65-L72src/core/auth/AuthenticationManager.ts#L146-L153
🤖 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 `@src/core/BaseClient.ts` around lines 59 - 63, Shorten the comments at
src/core/BaseClient.ts lines 59-63 to state only that authenticate uses the
configured timeout and accepts an optional signal; at
src/core/http/HttpClient.ts lines 522-528 state that bearer-token retrieval is
bounded and cleans up its timer and listener, and at lines 809-813 state that
socket timeouts are not retried; at src/core/auth/AuthenticationManager.ts lines
65-72 state that the wrapper bounds caller wait without canceling the delegate,
and at lines 146-153 state that 0 disables the timeout timer and cleanup occurs
on settlement. Remove rationale, historical context, and repeated lifecycle
details while preserving the behavior and references to authenticate,
bearer-token retrieval, socket timeout handling, and the authentication wrapper.
Source: Coding guidelines
… bypassing abort handling isRetryableError() previously fast-failed axios-level socket timeouts but still retried bearer-token/auth timeouts across the full retry budget, since fetchBearerToken and AuthenticationManager.withAuthTimeout reject with a plain NetworkError that isTimeoutError() (an axios-only check) can't recognize. Add a TimeoutError subclass of NetworkError so both timeout paths can be tagged consistently, and have isRetryableError() treat it like an axios socket timeout: never retried. Existing instanceof NetworkError checks (e.g. ScanApiClient endpoint rotation) keep matching since TimeoutError extends NetworkError. Also fix withAuthTimeout and fetchBearerToken bypassing all abort handling when timeoutMs is 0: both now still wire up (and pre-check) the AbortSignal regardless of timeoutMs, only skipping the timer-based rejection. A signal that is already aborted before authenticate() is called now short-circuits before the delegate is ever invoked, and a signal that aborts mid-call is still honored even with no timeout budget configured. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
…tMs:0 abort handling - errors.test.ts: TimeoutError constructs correctly and stays instanceof NetworkError/CantonError while remaining distinguishable via instanceof. - http-client-retry.test.ts: a bearer-token provider rejecting with TimeoutError is not retried, mirroring the existing synthetic ECONNABORTED-is-not-retried test. - http-client-hang.test.ts: a bearer-token provider that hangs forever is bounded to roughly one timeoutMs across a multi-retry budget instead of (maxRetries + 1) * timeoutMs. - http-client-timeout.test.ts: fetchBearerToken with timeoutMs: 0 rejects a pre-aborted signal immediately without invoking the provider, and still cancels a hung fetch when the signal aborts mid-call. - authenticate-hang.test.ts: AuthenticationManager.authenticate with timeoutMs: 0 rejects a pre-aborted signal immediately without contacting the auth server, and still cancels a hung call when the signal aborts mid-call. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
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 (3)
src/core/http/HttpClient.ts (2)
681-684: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-532): Insertion of Sensitive Information into Log File
Reachability path
● Entry test/unit/core/http-client-timeout.test.ts │ ▼ ● Sink src/core/http/HttpClient.tsRedact the request URL in timeout errors.
error.config?.urlcan include query-string credentials. Usethis.redactEndpoint(error.config?.url ?? 'unknown')and add a regression test with a query-string secret.🤖 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 `@src/core/http/HttpClient.ts` around lines 681 - 684, Update the timeout error construction in HttpClient to pass error.config?.url ?? 'unknown' through this.redactEndpoint before interpolating it in the request context, and add a regression test covering a query-string secret to verify the secret is absent from the resulting timeout error.
537-553: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRegister cancellation before invoking the provider.
When
provider(signal)abortssignalsynchronously and returns a pending promise, the later listener does not receive the abort event. WithtimeoutMs: 0,fetchBearerTokencan remain pending. Register the listener first, or callthrowIfAborted(signal)again after invoking the provider. Add a regression test for synchronous provider cancellation.🤖 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 `@src/core/http/HttpClient.ts` around lines 537 - 553, Update fetchBearerToken’s cancellation flow around provider(signal) so the abort listener is registered before invoking the provider, or recheck the signal immediately afterward, ensuring synchronous provider cancellation rejects instead of leaving the promise pending. Preserve timeout behavior and add a regression test covering a provider that aborts synchronously.src/core/auth/AuthenticationManager.ts (1)
65-78: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winShorten the timeout comments.
Keep comments limited to active behavior. Remove implementation comparisons and regression history.
src/core/auth/AuthenticationManager.ts#L65-L78: state the caller wait bound and that abort does not start or join authentication.src/core/auth/AuthenticationManager.ts#L150-L159: state that zero disables only the timer and abort still rejects the caller wait.test/unit/core/authenticate-hang.test.ts#L161-L161: state that the abort listener bounds the wait.As per coding guidelines, “Keep code comments brief and describe only the current state; do not include historical explanations or verbose write-ups.”
🤖 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 `@src/core/auth/AuthenticationManager.ts` around lines 65 - 78, Shorten the comments at src/core/auth/AuthenticationManager.ts lines 65-78 to state only the caller wait bound and that abort prevents starting or joining authentication; at lines 150-159, state that zero disables only the timer while abort still rejects the caller wait; and at test/unit/core/authenticate-hang.test.ts line 161, state that the abort listener bounds the wait. Remove implementation comparisons, regression history, and verbose explanations without changing behavior.Source: Coding guidelines
🤖 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 `@src/core/auth/AuthenticationManager.ts`:
- Around line 65-78: Shorten the comments at
src/core/auth/AuthenticationManager.ts lines 65-78 to state only the caller wait
bound and that abort prevents starting or joining authentication; at lines
150-159, state that zero disables only the timer while abort still rejects the
caller wait; and at test/unit/core/authenticate-hang.test.ts line 161, state
that the abort listener bounds the wait. Remove implementation comparisons,
regression history, and verbose explanations without changing behavior.
In `@src/core/http/HttpClient.ts`:
- Around line 681-684: Update the timeout error construction in HttpClient to
pass error.config?.url ?? 'unknown' through this.redactEndpoint before
interpolating it in the request context, and add a regression test covering a
query-string secret to verify the secret is absent from the resulting timeout
error.
- Around line 537-553: Update fetchBearerToken’s cancellation flow around
provider(signal) so the abort listener is registered before invoking the
provider, or recheck the signal immediately afterward, ensuring synchronous
provider cancellation rejects instead of leaving the promise pending. Preserve
timeout behavior and add a regression test covering a provider that aborts
synchronously.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 390ae006-1ada-4afb-a7d9-72ee4800d3b5
📒 Files selected for processing (8)
src/core/auth/AuthenticationManager.tssrc/core/errors.tssrc/core/http/HttpClient.tstest/unit/core/authenticate-hang.test.tstest/unit/core/errors.test.tstest/unit/core/http-client-hang.test.tstest/unit/core/http-client-retry.test.tstest/unit/core/http-client-timeout.test.ts
Register the abort listener before invoking provider(signal) instead of after, so a provider that synchronously aborts signal before returning its still-pending promise is no longer missed. Previously, with timeoutMs: 0, this could hang the call forever. Adds a regression test covering synchronous provider cancellation. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
…y (MUST FIX #2) handleRequestError built its timeout NetworkError message directly from error.config?.url, which can carry query-string credentials/tokens. Route it through the existing redactEndpoint() helper instead. Also wraps every real-HTTP-server test in http-client-hang.test.ts in try/finally so a failing assertion cannot leave a listening socket behind, and adds a regression test asserting a timeout error message never contains a raw query-string secret. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
…#3) isTimeoutError treated any axios error with code ETIMEDOUT the same as our own configured socket-inactivity timeout, making it non-retryable. Empirically (axios 1.18.1, the version pinned in package.json), the Node http adapter's createTimeoutError() always uses ECONNABORTED for our own configured timeout, since we never set transitional.clarifyTimeoutError (its default is false). A raw ETIMEDOUT only ever comes from Node's own connect-phase failure (e.g. 'connect ETIMEDOUT <ip>:<port>'), a distinct and genuinely transient condition that should remain retryable, matching pre-PR behavior for that failure mode. isTimeoutError now checks for ECONNABORTED only. Adds a regression test simulating a connect-phase ETIMEDOUT (code: 'ETIMEDOUT', syscall: 'connect') and asserting it is retried like other transient network failures. Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
…D FIX) Node's setTimeout silently clamps delays above 2^31-1ms (~24.8 days) to 1ms, which would fire HttpClient's socket-inactivity timer, fetchBearerToken's internal timer, or WebSocketClient's idle timer almost immediately instead of respecting a misconfigured caller's very large timeoutMs/idleTimeoutMs. Reject values above that limit at construction/request time with a ConfigurationError instead. The existing 600,000ms default is far below this limit. Adds regression tests for both HttpClient (client-level and per-request timeoutMs) and WebSocketClient (idleTimeoutMs). Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
… FIX)
Shortens the authenticate() and withAuthTimeout() docstrings in
AuthenticationManager.ts to describe current behavior without
cross-referencing HttpClient.fetchBearerToken's implementation, and
trims a regression-history aside ('without the fix this would hang
forever') from a test comment, per this repo's comment-brevity
convention.
Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0ee40d7. Configure here.
|
|
||
| this.pendingAuthentication = authenticationPromise; | ||
| return authenticationPromise; | ||
| return this.withAuthTimeout(authenticationPromise, timeoutMs, signal); |
Contributor
There was a problem hiding this comment.
Auth stays wedged after timeout
High Severity
authenticate bounds only the caller's wait via withAuthTimeout and leaves pendingAuthentication set until the uncancellable delegate settles. Against a silent auth server (no socket timeout in @hardlydifficult/rest-client), that promise never settles, so later calls keep joining the same hung flight and never open a fresh token request—even after auth recovers—until clearToken or process restart.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0ee40d7. Configure here.
Contributor
|
@coderabbitai review |
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
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.
Why
HttpClientcreated its axios instance withaxios.create()and notimeout. Axios defaultstimeoutto0, which is disabled — confirmed in the installed axios adapter, which callsreq.setTimeout(0)and turns the socket timer off entirely. A Canton endpoint that accepts a TCP connection and then goes silent hangs theawaitforever.This is the root cause of a 5-day Fairmint rewards outage: worker processes stayed
onlineunder PM2 while blocked on a request that could never complete. Every consumer of this SDK has the same exposure — for Fairmint that's 22 workers, only some of which have any hang detection of their own.Changes
All additive: new optional config fields, a new optional constructor argument, one new exported constant.
CantonConfig.timeoutMs(client-wide),apis[SERVICE].timeoutMs(per service), andRequestConfig.timeoutMs(per request)NetworkErrornaming the configured timeout and the request, instead of anApiErrorreadingHTTP undefinedWebSocketOptions.idleTimeoutMsis opt-in onconnect; the timer starts on open, resets on every inbound message, and closes with code 4008.getActiveContractsdefaults it to 600,000 ms and accepts an override (0restores unbounded waiting).Why 600,000 ms
A too-short floor on a mutation turns a slow-but-successful submission into an
UnknownMutationOutcomeError, which is worse than a bounded hang. From the pinnedlibs/splicesubmodule at61a5360:CommandServiceConfig.DefaultDefaultTrackingTimeoutJsonApiConfig.defaultRequestTimeoutapplication.confCantonConfig.requestTimeoutThe longest legitimate silence on the wire is the command service holding a
submit-and-waitresponse open for up to 5 minutes, so 10 minutes is 2x headroom over the slowest thing Canton can legitimately do.Note this is a socket-inactivity timer (it resets on every byte received), not a total deadline, so a large ACS fetch or DAR upload that keeps streaming is unaffected. For a hard wall-clock deadline, pass
AbortSignal.timeout(ms)asoptions.signal— generated methods already forward it.Scope notes
subscribeToUpdatesandsubscribeToCompletionsstay unbounded on purpose: idle periods are normal on a long-lived subscription, and those callers hold aWebSocketSubscription.close(). ThreadingWebSocketOptionsthrough the operation factory is the follow-up if opt-in idle timeouts are wanted there.Generated wrappers needed no change — the generator already emits
(params, options?)carryingsignalfor every REST operation. The three params-only methods are all WebSocket-backed and never touch axios.Test plan
npm run buildandcheck:package-artifactspasshttp.createServerthat accepts the connection and never responds — the request rejects withNetworkErrorin under 5s instead of hangingAbortSignal.timeout(300)stops the requestAbortSignalverified reaching the axios config through a generated methodFollow-ups found, not fixed here
The OAuth token request in
@hardlydifficult/rest-clienthas no timeout of its own — the SDK'sawaiton it is now bounded, but that package's socket is not. Separately, a connection-refused error still surfaces asApiError: HTTP undefined; left alone to keep this change additive.Summary by CodeRabbit
New Features
Bug Fixes
Tests