-
Notifications
You must be signed in to change notification settings - Fork 768
Turn on automatic retries for sandbox requests #1022
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
fc6741e
5b3fca7
66ff8e0
a8e7ae3
503bd4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,39 @@ function getStdoutSize() { | |
| } | ||
| } | ||
|
|
||
| function isRetryableError(err: unknown): boolean { | ||
| // Retry on SDK TimeoutError | ||
| if (err instanceof (e2b as any).TimeoutError) return true | ||
|
|
||
| // Some environments throw AbortError for aborted/timeout fetches | ||
| if (err && typeof err === 'object' && (err as any).name === 'AbortError') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you can infer the type as err as Error (in below occurrences also) |
||
| return true | ||
|
|
||
| // Network/system-level transient errors commonly exposed via code property | ||
| const code = (err as any)?.code ?? (err as any)?.cause?.code | ||
| const retryableCodes = new Set([ | ||
| 'ECONNRESET', | ||
| 'ECONNREFUSED', | ||
| 'ECONNABORTED', | ||
| 'EPIPE', | ||
| 'ETIMEDOUT', | ||
| 'ENOTFOUND', | ||
| 'EAI_AGAIN', | ||
| 'EHOSTUNREACH', | ||
| 'EADDRINUSE', | ||
| ]) | ||
| if (typeof code === 'string' && retryableCodes.has(code)) return true | ||
|
|
||
| // Undici/Fetch may surface as TypeError: fetch failed with nested cause | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you check this? |
||
| if ((err as any) instanceof TypeError) { | ||
| const msg = String((err as any).message || '').toLowerCase() | ||
| if (msg.includes('fetch failed') || msg.includes('network error')) | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
||
| export async function spawnConnectedTerminal(sandbox: e2b.Sandbox) { | ||
| // Clear local terminal emulator before starting terminal | ||
| // process.stdout.write('\x1b[2J\x1b[0f') | ||
|
|
@@ -26,7 +59,17 @@ export async function spawnConnectedTerminal(sandbox: e2b.Sandbox) { | |
|
|
||
| const inputQueue = new BatchedQueue<Buffer>(async (batch) => { | ||
| const combined = Buffer.concat(batch) | ||
| await sandbox.pty.sendInput(terminalSession.pid, combined) | ||
|
|
||
| const maxRetries = 3 | ||
| for (let retry = 0; ; retry++) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. try to avoid infinite loops |
||
| try { | ||
| await sandbox.pty.sendInput(terminalSession.pid, combined) | ||
| break | ||
|
Comment on lines
+63
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Retrying Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting point. @ValentaTomas wdyt about adding a "request number", idempotency key, or something similar so that the backend avoids duplicate processing?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might make sense here! Alternatively, the client long running connection that we don't have implemented in the SDK might be also a solution. |
||
| } catch (err) { | ||
| // Do not retry on errors that come with valid HTTP/gRPC responses | ||
| if (!isRetryableError(err) || retry >= maxRetries) throw err | ||
| } | ||
| } | ||
| }, FLUSH_INPUT_INTERVAL_MS) | ||
|
|
||
| const resizeListener = process.stdout.on('resize', () => | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(e2b as any) unnecessary - just import TimeoutError from e2b