Add retry logic to World operations #648
Conversation
🦋 Changeset detectedLatest commit: a02365b The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (10 failed)mongodb (1 failed):
starter (8 failed):
turso (1 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR adds automatic retry logic to World operations to improve resilience against transient failures such as network issues and server errors.
- Implements a configurable retry mechanism using the
async-retrypackage with exponential backoff - Creates a proxy-based wrapper that automatically applies retry logic to all World methods
- Defines logic to distinguish retryable errors (network failures, 5xx errors, timeouts) from non-retryable ones (4xx client errors)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/package.json | Adds async-retry runtime dependency and @types/async-retry dev dependency |
| pnpm-lock.yaml | Updates lockfile with new dependencies and their transitive dependencies |
| packages/core/src/runtime/retry.ts | Implements core retry logic including error detection, retry wrapper function, and proxy-based automatic wrapping |
| packages/core/src/runtime/world.ts | Integrates retry logic by wrapping World instances with the retry proxy |
| .changeset/all-pens-heal.md | Documents the change for the changelog |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔧 Build Fix:
The withRetry function incorrectly returns the result of calling bail(), which returns void, but the callback signature requires returning a value of type T. The fix is to call bail() without returning it, then throw the error to satisfy the type checker.
View Details
📝 Patch Details
diff --git a/packages/core/src/runtime/retry.ts b/packages/core/src/runtime/retry.ts
index f0f7c94..8a4dae0 100644
--- a/packages/core/src/runtime/retry.ts
+++ b/packages/core/src/runtime/retry.ts
@@ -130,7 +130,9 @@ export async function withRetry<T>(
} catch (error) {
// If the error is not retryable, bail immediately
if (!isRetryableError(error)) {
- return bail(error as Error);
+ bail(error as Error);
+ // This throw is unreachable, but ensures correct return type
+ throw error;
}
// Otherwise, throw to trigger a retry
throw error;
Analysis
TypeScript compilation error: bail() return type incompatibility
What fails: TypeScript compiler fails on src/runtime/retry.ts line 126 due to incorrect return type for the bail() function callback.
How to reproduce:
cd packages/core
pnpm run buildResult:
src/runtime/retry.ts(126,3): error TS2322: Type 'void | T' is not assignable to type 'T'.
'T' could be instantiated with an arbitrary type which could be unrelated to 'void | T'.
Root cause: The async-retry library's bail() function has signature (e: Error) => void and does not return a value. However, the code was using return bail(error as Error), which attempts to return void when the function signature requires returning T. The fix calls bail() without returning its result, then throws the error to ensure the correct return type.
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
This looks fine to solve the immediate issue, assuming we change the update calls to be re-tried too, since those seem pretty important to re-try.
On a higher level, I don't like that we're re-defining the entire interface. Was it not possible to use a JS getter proxy?
| return true; | ||
| } | ||
|
|
||
| // Check for fetch-related errors or AbortError from timeouts |
There was a problem hiding this comment.
What is "AbortError from timeouts" referring to here?
| return withRetry(() => this.world.runs.get(id, params)); | ||
| }, | ||
| update: (id: string, data: UpdateWorkflowRunRequest) => { | ||
| // Non-idempotent write - no retry |
There was a problem hiding this comment.
Isn't update idempotent? We should retry IMO
| return withRetry(() => this.world.steps.get(runId, stepId, params)); | ||
| }, | ||
| update: (runId: string, stepId: string, data: UpdateStepRequest) => { | ||
| // Non-idempotent write - no retry |
| return withRetry(() => this.world.hooks.list(params)); | ||
| }, | ||
| dispose: (hookId: string, params?: GetHookParams) => { | ||
| // Non-idempotent write - no retry |
Added retry logic to
Worldoperations to improve resilience against transient failures.What changed?
async-retrypackagewithRetryfunction that wraps async operations with configurable retry logic