feat: Socket automatic retry as the same of that of python-sdk#307
feat: Socket automatic retry as the same of that of python-sdk#307Abishek-Newar wants to merge 2 commits intoopenfga:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR updates default retry configuration in the request creation function to use SDK constants instead of zero values, enabling automatic retry behavior with non-zero minimum wait times by default. A corresponding test is updated to explicitly disable retries when testing expected failures. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @dyeam0 @SoulPancake , Please review it |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #307 +/- ##
=======================================
Coverage 89.67% 89.67%
=======================================
Files 25 25
Lines 1492 1492
Branches 279 256 -23
=======================================
Hits 1338 1338
Misses 94 94
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s retry behavior so that when users do not explicitly configure retryParams, requests will use the SDK’s default retry settings (3 retries with a 100ms minimum backoff), enabling automatic retries for transient failures like socket hang-ups.
Changes:
- Use
SdkConstants.DefaultMaxRetry/SdkConstants.DefaultMinWaitInMsas the fallback retry configuration increateRequestFunction. - Update a
listRelationstest to explicitly disable retries (maxRetry: 0) because the test only mocks a single 500 response.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| common.ts | Switches retry fallback values from 0 to SDK defaults so retries are enabled by default when not configured. |
| tests/client.test.ts | Disables retries for a test case that expects a single 500 response to fail immediately. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SoulPancake
left a comment
There was a problem hiding this comment.
Can you please also add a test for network error retries too?
| const maxRetry: number = retryParams?.maxRetry ?? SdkConstants.DefaultMaxRetry; | ||
| const minWaitInMs: number = retryParams?.minWaitInMs ?? SdkConstants.DefaultMinWaitInMs; | ||
|
|
||
| const start = performance.now(); |
There was a problem hiding this comment.
Line 422 in a7090e2
Can you also add it for the streaming requests
| user: "user:81684243-9356-4421-8fbf-a4f8d36aa31b", | ||
| object: "workspace:1", | ||
| relations: ["admin", "guest", "reader", "viewer"], | ||
| }); |
There was a problem hiding this comment.
The test change here is defensive (preventing flake) rather than assertive
Description
Network errors like "socket hang up" are not being automatically retried by the SDK, even though the retry mechanism exists. Users expect transient network failures to be handled gracefully without requiring explicit configuration.
What problem is being solved?
The SDK now uses the default retry parameters (maxRetry: 3, minWaitInMs: 100) when the user doesn't explicitly provide retryParams in their configuration.
How is it being solved?
What changes are made to solve it?
common.ts: Changed the fallback values for maxRetry and minWaitInMs from 0 toSdkConstants.DefaultMaxRetry (3)andSdkConstants.DefaultMinWaitInMs (100ms)respectively.tests/client.test.ts: Updated one test that expects an immediate 500 error to explicitly disable retries, since the test mock only provides a single response.With this change, the SDK will automatically retry:
References
Closes #305 #306
Review Checklist
mainSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.