fix: fixes timeout for communication with provider-proxy#2820
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughTimeout handling in provider services is refactored to decouple per-attempt and total timeout values. Per-attempt timeouts are sent in request payloads, while total timeouts are multiplied by 5 in HTTP options. Timeout defaults are reduced from 30,000 ms to 15,000 ms, and tests are enhanced with fake timer controls for better timing assertions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (1)
apps/api/src/provider/services/provider/provider-proxy.service.spec.ts (1)
52-59:⚠️ Potential issue | 🟡 MinorStale test description: "when within max"
The
MAX_TIMEOUTcap was removed in this PR; there is no longer a "within max" condition. Update the description to reflect the actual behavior.✏️ Suggested fix
- it("passes timeout in both payload and config when within max", async () => { + it("passes timeout in payload and scales it by 5x in HTTP config", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/provider/services/provider/provider-proxy.service.spec.ts` around lines 52 - 59, Update the stale test title: change the it(...) description in the test that calls setup({ timeout }) and invokes service.request(url, options) (the test currently reads "passes timeout in both payload and config when within max") to accurately reflect current behavior (e.g., "passes timeout in both payload and config") so the test name matches the behavior exercised by httpClient.post mock and the expectations on service.request.
🧹 Nitpick comments (3)
apps/api/src/provider/services/provider/provider.service.spec.ts (1)
152-166: Consider usingrejectsinstead of manually unwrapping the errorThe
.catch(error => ({ error }))wrapper + manual cast pattern works, but the standard vitest idiom for asserting on a rejection isawait expect(promise).rejects.toThrow(...). It also removes the explicit, 15000timeout hint that is redundant when fake timers are active (real time does not elapse).♻️ Suggested refactor
- const result = service - .sendManifest({ - provider: provider.owner, - dseq, - manifest, - auth: await service.toProviderAuth({ walletId: wallet.id, provider: provider.owner }) - }) - .catch(error => ({ error })); - + const result = service.sendManifest({ + provider: provider.owner, + dseq, + manifest, + auth: await service.toProviderAuth({ walletId: wallet.id, provider: provider.owner }) + }); + await vi.runAllTimersAsync(); - const { error } = (await result) as { error: Error }; - expect(error.message).toContain("no lease for deployment"); + await expect(result).rejects.toThrow("no lease for deployment"); expect(providerProxyService.request).toHaveBeenCalledTimes(3); - }, 15000); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/provider/services/provider/provider.service.spec.ts` around lines 152 - 166, Replace the manual .catch wrapper and manual error extraction with Vitest's rejects idiom: call the promise returned by service.sendManifest(...) and assert with await expect(promise).rejects.toThrow("no lease for deployment") after advancing timers with await vi.runAllTimersAsync(); remove the surrounding .catch(error => ({ error })) and the redundant test timeout parameter (", 15000"); keep the existing assertion that providerProxyService.request was called 3 times to preserve call-count verification.apps/api/src/provider/services/provider/provider-proxy.service.ts (1)
54-59: Extract the5multiplier as a named constantThe scalar
5that converts per-attempt timeout to total-request timeout is a magic number. Naming it makes the intent clearer and ensures the value is changed in one place (the test for5 * timeoutwould also benefit).♻️ Suggested refactor
+ private static readonly TIMEOUT_ATTEMPT_MULTIPLIER = 5; + async request<T>(url: string, options: ProviderProxyPayload): Promise<T> { const { chainNetwork, providerIdentity, timeout, ...params } = options; const response = await this.#httpClient.post( "/", { ...params, method: options.method || "GET", url: providerIdentity.hostUri + url, providerAddress: providerIdentity.owner, network: chainNetwork, - timeout // this is per attempt timeout on provider-proxy side + timeout }, { ...(timeout && { - // this is total timeout for all attempts to communicate with the provider API, and it should be much higher than per attempt timeout - timeout: timeout * 5 + timeout: timeout * ProviderProxyService.TIMEOUT_ATTEMPT_MULTIPLIER }) } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/provider/services/provider/provider-proxy.service.ts` around lines 54 - 59, The literal multiplier 5 used to compute the total request timeout is a magic number; extract it to a named constant (e.g. TOTAL_TIMEOUT_MULTIPLIER) and replace the expression timeout * 5 with timeout * TOTAL_TIMEOUT_MULTIPLIER in the object that sets timeout (the code block that spreads ...(timeout && { timeout: timeout * 5 })). Also update any tests that assert on the numeric value to reference the new constant so the behavior stays consistent and the intent is clear.apps/api/src/provider/services/provider/provider.service.ts (1)
91-91: Inconsistent numeric literal formattingLine 91 uses
15_000(with separator) while line 140 uses15000(without). Prefer a consistent style across both call sites.✏️ Suggested fix
- timeout: 15000 + timeout: 15_000Also applies to: 140-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/provider/services/provider/provider.service.ts` at line 91, Two numeric literals for the same timeout are formatted inconsistently: one uses 15_000 and another uses 15000; pick a single style and make both call sites consistent by updating the other occurrence. Locate the timeout property in provider.service.ts (the options object where timeout: 15_000 is set) and the other call site that currently uses 15000, then change the 15000 to match your chosen style (either 15_000 or 15000) so both timeout literals are identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/api/src/provider/services/provider/provider-proxy.service.spec.ts`:
- Around line 52-59: Update the stale test title: change the it(...) description
in the test that calls setup({ timeout }) and invokes service.request(url,
options) (the test currently reads "passes timeout in both payload and config
when within max") to accurately reflect current behavior (e.g., "passes timeout
in both payload and config") so the test name matches the behavior exercised by
httpClient.post mock and the expectations on service.request.
---
Nitpick comments:
In `@apps/api/src/provider/services/provider/provider-proxy.service.ts`:
- Around line 54-59: The literal multiplier 5 used to compute the total request
timeout is a magic number; extract it to a named constant (e.g.
TOTAL_TIMEOUT_MULTIPLIER) and replace the expression timeout * 5 with timeout *
TOTAL_TIMEOUT_MULTIPLIER in the object that sets timeout (the code block that
spreads ...(timeout && { timeout: timeout * 5 })). Also update any tests that
assert on the numeric value to reference the new constant so the behavior stays
consistent and the intent is clear.
In `@apps/api/src/provider/services/provider/provider.service.spec.ts`:
- Around line 152-166: Replace the manual .catch wrapper and manual error
extraction with Vitest's rejects idiom: call the promise returned by
service.sendManifest(...) and assert with await
expect(promise).rejects.toThrow("no lease for deployment") after advancing
timers with await vi.runAllTimersAsync(); remove the surrounding .catch(error =>
({ error })) and the redundant test timeout parameter (", 15000"); keep the
existing assertion that providerProxyService.request was called 3 times to
preserve call-count verification.
In `@apps/api/src/provider/services/provider/provider.service.ts`:
- Line 91: Two numeric literals for the same timeout are formatted
inconsistently: one uses 15_000 and another uses 15000; pick a single style and
make both call sites consistent by updating the other occurrence. Locate the
timeout property in provider.service.ts (the options object where timeout:
15_000 is set) and the other call site that currently uses 15000, then change
the 15000 to match your chosen style (either 15_000 or 15000) so both timeout
literals are identical.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/api/src/provider/services/provider/provider-proxy.service.spec.tsapps/api/src/provider/services/provider/provider-proxy.service.tsapps/api/src/provider/services/provider/provider.service.spec.tsapps/api/src/provider/services/provider/provider.service.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2820 +/- ##
==========================================
- Coverage 54.24% 53.38% -0.87%
==========================================
Files 1020 985 -35
Lines 23637 22803 -834
Branches 5796 5699 -97
==========================================
- Hits 12823 12173 -650
+ Misses 9435 9251 -184
Partials 1379 1379
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
baktun14
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The same timeout was passed to provider-proxy (which is per attempt timeout in case provider API doesn't respond) and to actual request timeout. This resulted in provider-proxy retries being ignored, in case of provider API timing out on the 1st attempt.
What
Summary by CodeRabbit
Bug Fixes
Tests