Fix concurrent RPC fallback skipping healthy endpoints#448
Conversation
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
🦋 Changeset detectedLatest commit: 7db4875 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesTransport fallback correction
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/jsonRpc/transports/fallback.ts (1)
18-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider wrapping
this.iwith modulo for bounded growth.
this.iadvances by 1 on each failure but is never wrapped with% this.transports.length. While the modular arithmetic at line 15 keeps selection correct,this.ican grow well beyond the valid index range, making the// Current transport indexcomment on line 4 slightly misleading. Wrapping keeps the value bounded and self-consistent.♻️ Optional refactor
- this.i += 1; + this.i = (this.i + 1) % this.transports.length;🤖 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 `@packages/core/src/jsonRpc/transports/fallback.ts` at line 18, Update the failure-advance logic around this.i so it wraps with modulo this.transports.length after incrementing. Keep transport selection behavior unchanged while ensuring this.i remains bounded to the valid transport index range and stays consistent with its “Current transport index” role.
🤖 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.
Nitpick comments:
In `@packages/core/src/jsonRpc/transports/fallback.ts`:
- Line 18: Update the failure-advance logic around this.i so it wraps with
modulo this.transports.length after incrementing. Keep transport selection
behavior unchanged while ensuring this.i remains bounded to the valid transport
index range and stays consistent with its “Current transport index” role.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d90d3fa6-b71b-4a0b-bbe5-f0786f66eab2
📒 Files selected for processing (2)
packages/core/src/jsonRpc/transports/fallback.test.tspackages/core/src/jsonRpc/transports/fallback.ts
|
@copilot assign to |
Done in the latest commit. |
|
OMG |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/core/src/jsonRpc/transports/fallback.ts`:
- Around line 10-26: Validate that the fallback transport list is non-empty
before entering the retry loop in the request method containing startI and
lastErr. If no transports are configured, throw an actionable Error instead of
allowing the final throw lastErr to throw undefined; preserve the existing retry
and error propagation behavior for non-empty configurations.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: ee3aece3-785a-4b34-b0ad-3757476234eb
📒 Files selected for processing (1)
packages/core/src/jsonRpc/transports/fallback.ts
There was a problem hiding this comment.
Pull request overview
This PR fixes a concurrency bug in TransportFallback where concurrent requests sharing the same instance could retry a failed transport again (wrapping the shared index) instead of advancing to a healthy fallback endpoint, leading to avoidable request failures.
Changes:
- Refactors
TransportFallback.request()to snapshot the starting index per call and iterate deterministically over transports. - Adds a Vitest suite covering success, sequential fallback, all-fail behavior, concurrent fallback behavior, and “skip known-bad” behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/jsonRpc/transports/fallback.ts | Updates fallback iteration logic to be per-request (concurrency-safe). |
| packages/core/src/jsonRpc/transports/fallback.test.ts | Adds regression tests for sequential and concurrent fallback behavior. |
|
@coderabbitai rate limit |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/core/src/jsonRpc/transports/fallback.ts:25
this.iis no longer advanced on transport failure. That means requests that start after a failure (but before a later success in another in-flight request) will still snapshot the old index and hit the known-bad transport first, which also contradicts the PR description thatthis.iis incremented on failure to help future requests skip failing endpoints. Advancing the shared index in the catch block is safe now that retry selection is based on the per-callstartIsnapshot.
this.i = i;
return res;
} catch (err) {
lastErr = err;
}
packages/core/src/jsonRpc/transports/fallback.ts:13
- The new explicit error path for
transports.length === 0(throwing "TransportFallback requires at least one transport") isn’t covered by tests yet. Adding a small unit test would prevent regressions back to a less-informative runtime error (e.g., trying to call.requestonundefined).
const startI = this.i;
let lastErr: unknown = new Error(
"TransportFallback requires at least one transport",
);
1b1ea8a to
90f533b
Compare
90f533b to
7db4875
Compare
Concurrent requests sharing the same
TransportFallbackinstance could route retries back to a failed endpoint instead of advancing to a healthy fallback, causing requests to reject without ever reaching a working RPC.Root cause:
TransportFallback.request()readthis.ion every retry iteration. Two concurrent requests both failing on transport A would each incrementthis.i, causing one of them to wrap back to A on its retry instead of advancing to B.Fix (
packages/core/src/jsonRpc/transports/fallback.ts):this.iasstartIat the start of eachrequest()call(startI + tried) % lengthto iterate transports — isolated per-call, unaffected by concurrent mutationsthis.iis still incremented on failure so future requests skip known-bad transportsTests (
packages/core/src/jsonRpc/transports/fallback.test.ts):fallback.test.tscovering single-transport success, sequential fallback, all-fail throw, concurrent requests both succeeding via fallback, and the "skip known-bad" advancement behaviour.