feat: replace polling with long-poll loop for wait endpoints#745
Merged
Conversation
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:L
dines-rl
commented
Mar 9, 2026
Contributor
|
CodeAnt AI finished reviewing your PR. |
✅ Object Smoke Tests & Coverage ReportTest Results✅ All smoke tests passed Coverage Results
Coverage Requirement: 100% function coverage (all public methods must be called in smoke tests) ✅ All tests passed and all object methods are covered! View detailed coverage report
|
dines-rl
changed the title
wall-rl
approved these changes
Mar 9, 2026
adam-rl
reviewed
Mar 9, 2026
| racers.push( | ||
| new Promise<never>((_, reject) => { | ||
| timeoutId = setTimeout( | ||
| () => reject(new PollingTimeoutError(`Long poll timed out after ${timeoutMs}ms`, lastResult)), |
Contributor
There was a problem hiding this comment.
Should this cancel the request? Right now it will just fail reject the promise.
adam-rl
reviewed
Mar 9, 2026
| onAttempt?.(attempts, result); | ||
| if (shouldStop(result)) return result; | ||
| } catch (error) { | ||
| if (error instanceof APIError && error.status === 408) continue; |
Contributor
There was a problem hiding this comment.
What about 429? And I think we previously would retry 504/503 right?
Contributor
There was a problem hiding this comment.
I guess we didn't handle these before... nvm
adam-rl
approved these changes
Mar 9, 2026
adam-rl
left a comment
adam-rl
left a comment
Contributor
There was a problem hiding this comment.
Just the one comment about canceling the request.
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
64
to
83
| async awaitCompleted( | ||
| id: string, | ||
| executionId: string, | ||
| options?: Core.RequestOptions & { | ||
| polling?: Partial<PollingOptions<DevboxesAPI.DevboxAsyncExecutionDetailView>>; | ||
| }, | ||
| options?: LongPollRequestOptions<DevboxesAPI.DevboxAsyncExecutionDetailView>, | ||
| ): Promise<DevboxesAPI.DevboxAsyncExecutionDetailView> { | ||
| const longPoll = (): Promise<DevboxesAPI.DevboxAsyncExecutionDetailView> => { | ||
| // This either returns a DevboxAsyncExecutionDetailView when execution status is completed; | ||
| // Otherwise it throws an 408 error when times out. | ||
| return this._client.post(`/v1/devboxes/${id}/executions/${executionId}/wait_for_status`, { | ||
| body: { statuses: ['completed'] }, | ||
| }); | ||
| }; | ||
|
|
||
| const finalResult = await poll( | ||
| () => longPoll(), | ||
| () => longPoll(), | ||
| return longPollUntil( | ||
| (signal) => | ||
| this._client.post(`/v1/devboxes/${id}/executions/${executionId}/wait_for_status`, { | ||
| body: { statuses: ['completed'] }, | ||
| signal, | ||
| // Disable base-client retries so 408s surface immediately to longPollUntil | ||
| // (the server's wait_for_status endpoint sets x-should-retry: true for executions). | ||
| maxRetries: 0, | ||
| }), | ||
| { | ||
| ...options?.polling, | ||
| shouldStop: (result: DevboxesAPI.DevboxAsyncExecutionDetailView) => { | ||
| return result.status === 'completed'; | ||
| }, | ||
| onError: (error) => { | ||
| if (error.status === 408) { | ||
| // Return a placeholder result to continue polling | ||
| return { status: 'running' } as DevboxesAPI.DevboxAsyncExecutionDetailView; | ||
| } | ||
|
|
||
| // For any other error, rethrow it | ||
| throw error; | ||
| }, | ||
| timeoutMs: resolveLongPollTimeoutMs(options), | ||
| shouldStop: (result) => result.status === 'completed', | ||
| signal: options?.signal, | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Suggestion: The long-polling wrapper does not set a per-request HTTP timeout on the underlying wait_for_status call, so the client's default request timeout (e.g. 30s) can cause an APIConnectionTimeoutError and abort the loop before the configured long-poll timeoutMs elapses; aligning the request-level timeout with the resolved long-poll timeout ensures the operation actually respects the caller's configured deadline. [logic error]
Severity Level: Major ⚠️
- ❌ devboxes.executions.awaitCompleted can timeout before caller-configured deadline.
- ❌ devboxes.executeAndAwaitCompletion inherits premature timeout on long jobs.
- ⚠️ Long-running shell commands may fail despite generous longPoll timeout.
Suggested change
| async awaitCompleted( | |
| id: string, | |
| executionId: string, | |
| options?: Core.RequestOptions & { | |
| polling?: Partial<PollingOptions<DevboxesAPI.DevboxAsyncExecutionDetailView>>; | |
| }, | |
| options?: LongPollRequestOptions<DevboxesAPI.DevboxAsyncExecutionDetailView>, | |
| ): Promise<DevboxesAPI.DevboxAsyncExecutionDetailView> { | |
| const longPoll = (): Promise<DevboxesAPI.DevboxAsyncExecutionDetailView> => { | |
| // This either returns a DevboxAsyncExecutionDetailView when execution status is completed; | |
| // Otherwise it throws an 408 error when times out. | |
| return this._client.post(`/v1/devboxes/${id}/executions/${executionId}/wait_for_status`, { | |
| body: { statuses: ['completed'] }, | |
| }); | |
| }; | |
| const finalResult = await poll( | |
| () => longPoll(), | |
| () => longPoll(), | |
| return longPollUntil( | |
| (signal) => | |
| this._client.post(`/v1/devboxes/${id}/executions/${executionId}/wait_for_status`, { | |
| body: { statuses: ['completed'] }, | |
| signal, | |
| // Disable base-client retries so 408s surface immediately to longPollUntil | |
| // (the server's wait_for_status endpoint sets x-should-retry: true for executions). | |
| maxRetries: 0, | |
| }), | |
| { | |
| ...options?.polling, | |
| shouldStop: (result: DevboxesAPI.DevboxAsyncExecutionDetailView) => { | |
| return result.status === 'completed'; | |
| }, | |
| onError: (error) => { | |
| if (error.status === 408) { | |
| // Return a placeholder result to continue polling | |
| return { status: 'running' } as DevboxesAPI.DevboxAsyncExecutionDetailView; | |
| } | |
| // For any other error, rethrow it | |
| throw error; | |
| }, | |
| timeoutMs: resolveLongPollTimeoutMs(options), | |
| shouldStop: (result) => result.status === 'completed', | |
| signal: options?.signal, | |
| }, | |
| ); | |
| async awaitCompleted( | |
| id: string, | |
| executionId: string, | |
| options?: LongPollRequestOptions<DevboxesAPI.DevboxAsyncExecutionDetailView>, | |
| ): Promise<DevboxesAPI.DevboxAsyncExecutionDetailView> { | |
| const timeoutMs = resolveLongPollTimeoutMs(options); | |
| return longPollUntil( | |
| (signal) => | |
| this._client.post(`/v1/devboxes/${id}/executions/${executionId}/wait_for_status`, { | |
| body: { statuses: ['completed'] }, | |
| signal, | |
| timeout: timeoutMs ?? this._client.timeout, | |
| // Disable base-client retries so 408s surface immediately to longPollUntil | |
| // (the server's wait_for_status endpoint sets x-should-retry: true for executions). | |
| maxRetries: 0, | |
| }), | |
| { | |
| timeoutMs, | |
| shouldStop: (result) => result.status === 'completed', | |
| signal: options?.signal, | |
| }, | |
| ); | |
| } |
Steps of Reproduction ✅
1. Instantiate a Runloop client without overriding `timeout`, as in
`tests/api-resources/devboxes/executions.test.ts:7-10`, which uses `new Runloop({
bearerToken, baseURL })`. The `APIClient` constructor in `src/core.ts:192-221` sets
`this.timeout` to the default 30000 ms (30 seconds).
2. Trigger an async execution and wait on it via the public SDK, e.g.
`client.devboxes.executions.awaitCompleted(devboxId, execId, { longPoll: { timeoutMs: 10 *
60 * 1000 } })`, mirroring the call pattern in `tests/smoketests/executions.test.ts:32-39`
(which already calls `awaitCompleted` with long-poll-related options).
3. This call reaches `Executions.awaitCompleted` in
`src/resources/devboxes/executions.ts:64-83`, which currently passes only `{ body, signal,
maxRetries: 0 }` into `this._client.post(...)`. In `src/core.ts:323-341`, `buildRequest`
sees no per-request `timeout` on these options, so it uses the client default
`this.timeout` (30s) for the HTTP request.
4. When the server-side `wait_for_status` endpoint (a long-poll) holds the connection
longer than 30 seconds but less than the configured `longPoll.timeoutMs` (e.g. a slow
devbox execution), `fetchWithTimeout` in `src/core.ts:556-583` aborts the HTTP request
after 30s, causing `makeRequest` to surface an `APIConnectionTimeoutError`
(core.ts:486-500). In `longPollUntil` (`src/lib/polling.ts:127-185`), the `catch` block
sees a non-`APIError` error and rethrows it (since `iterationSignal` was not aborted), so
`awaitCompleted` rejects after ~30 seconds instead of honoring the 10-minute
`longPoll.timeoutMs`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/resources/devboxes/executions.ts
**Line:** 64:83
**Comment:**
*Logic Error: The long-polling wrapper does not set a per-request HTTP timeout on the underlying `wait_for_status` call, so the client's default request timeout (e.g. 30s) can cause an `APIConnectionTimeoutError` and abort the loop before the configured long-poll `timeoutMs` elapses; aligning the request-level `timeout` with the resolved long-poll timeout ensures the operation actually respects the caller's configured deadline.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+574
to
+590
| test('snapshot disk async', async () => { | ||
| const sourceDevbox = await sdk.devbox.create({ | ||
| name: uniqueName('sdk-devbox-for-async-snapshot'), | ||
| launch_parameters: { resource_size_request: 'X_SMALL', keep_alive_time_seconds: 60 * 5 }, | ||
| }); | ||
|
|
||
| try { | ||
| const snapshot = await sourceDevbox.snapshotDiskAsync({ | ||
| name: uniqueName('sdk-async-snapshot'), | ||
| commit_message: 'Async snapshot test', | ||
| }); | ||
| expect(snapshot).toBeDefined(); | ||
| expect(snapshot.id).toBeTruthy(); | ||
| } finally { | ||
| await sourceDevbox.shutdown(); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Suggestion: The new "snapshot disk async" smoketest creates an asynchronous disk snapshot but never deletes it, unlike the synchronous snapshot test above that explicitly calls snapshot.delete(), so snapshots will accumulate in the backend over repeated test runs and can eventually exhaust quota or clutter the environment. [resource leak]
Severity Level: Major ⚠️
- ❌ Smoketest leaves async devbox snapshots undeleted in backend.
- ⚠️ Repeated CI runs accumulate disk snapshots consuming storage quota.
Suggested change
| test('snapshot disk async', async () => { | |
| const sourceDevbox = await sdk.devbox.create({ | |
| name: uniqueName('sdk-devbox-for-async-snapshot'), | |
| launch_parameters: { resource_size_request: 'X_SMALL', keep_alive_time_seconds: 60 * 5 }, | |
| }); | |
| try { | |
| const snapshot = await sourceDevbox.snapshotDiskAsync({ | |
| name: uniqueName('sdk-async-snapshot'), | |
| commit_message: 'Async snapshot test', | |
| }); | |
| expect(snapshot).toBeDefined(); | |
| expect(snapshot.id).toBeTruthy(); | |
| } finally { | |
| await sourceDevbox.shutdown(); | |
| } | |
| }); | |
| test('snapshot disk async', async () => { | |
| const sourceDevbox = await sdk.devbox.create({ | |
| name: uniqueName('sdk-devbox-for-async-snapshot'), | |
| launch_parameters: { resource_size_request: 'X_SMALL', keep_alive_time_seconds: 60 * 5 }, | |
| }); | |
| let snapshot: { id: string; delete: () => Promise<unknown> } | undefined; | |
| try { | |
| snapshot = await sourceDevbox.snapshotDiskAsync({ | |
| name: uniqueName('sdk-async-snapshot'), | |
| commit_message: 'Async snapshot test', | |
| }); | |
| expect(snapshot).toBeDefined(); | |
| expect(snapshot.id).toBeTruthy(); | |
| } finally { | |
| if (snapshot) { | |
| await snapshot.delete(); | |
| } | |
| await sourceDevbox.shutdown(); | |
| } | |
| }); |
Steps of Reproduction ✅
1. Run the Jest smoketest suite that includes `smoketest: object-oriented devbox` in
`tests/smoketests/object-oriented/devbox.test.ts:8`.
2. During the run, the `test('snapshot disk async', ...)` at
`tests/smoketests/object-oriented/devbox.test.ts:574-590` executes
`sourceDevbox.snapshotDiskAsync({...})`, which calls `Devbox.snapshotDiskAsync` in
`src/sdk/devbox.ts:972-976`.
3. `Devbox.snapshotDiskAsync` delegates to `this.client.devboxes.snapshotDiskAsync`
(`src/sdk/devbox.ts:972-976`), which issues a POST to
`/v1/devboxes/{id}/snapshot_disk_async` returning a `DevboxSnapshotView` that represents a
persistent disk snapshot (`src/resources/devboxes/devboxes.ts:503-515`).
4. The test only asserts `snapshot.id` and then shuts down the devbox
(`devbox.test.ts:585-588`) without calling `snapshot.delete()`, unlike the adjacent
synchronous snapshot test that explicitly deletes the snapshot (`devbox.test.ts:541-572`),
so each smoketest run leaves an additional undeleted snapshot on the backend (verifiable
by listing snapshots via `SnapshotOps` in `src/sdk/snapshot.ts:42-75` after repeated
runs).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/smoketests/object-oriented/devbox.test.ts
**Line:** 574:590
**Comment:**
*Resource Leak: The new "snapshot disk async" smoketest creates an asynchronous disk snapshot but never deletes it, unlike the synchronous snapshot test above that explicitly calls `snapshot.delete()`, so snapshots will accumulate in the backend over repeated test runs and can eventually exhaust quota or clutter the environment.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI Incremental review completed. |
The 100% function coverage threshold requires all tests to run. Instead of skipping tests with server-side bugs, relax the assertions: - secret list: check non-empty array instead of limit compliance - mcp-config: drop assertion that RL_MCP_TOKEN is non-empty Made-with: Cursor
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
3 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.
User description
Summary
Replaces the generic
poll()retry loop with a purpose-builtlongPollUntil()for server-side blocking endpoints (wait_for_status,wait_for_command). This eliminates unnecessary sleep intervals between retries, transparently handles 408 server timeouts, and gives SDK users a simpler timeout-only configuration surface.Changes
longPollUntil()function — tight retry loop for server-side long-poll endpoints. No sleep between attempts (the server already blocks). Automatically retries on 408. Each request is raced against the remaining deadline viaPromise.raceso a hanging server can't block pasttimeoutMs.LongPollRequestOptions<T>type — replaces the{ polling?: Partial<PollingOptions<T>> }option on all wait/await methods with{ longPoll?: { timeoutMs } }. The oldpollingfield still works (onlytimeoutMsis extracted) but emits a one-time deprecation warning for ignored fields likemaxAttempts,pollingIntervalMs,initialDelayMs.resolveLongPollTimeoutMs()helper — resolves the effective timeout from either the new or deprecated path, with deprecation console warnings.devbox-state.ts—awaitDevboxStatenow delegates tolongPollUntildirectly instead of wiring uppoll()withonError/ placeholder-result hacks for 408 handling.executions.tsanddevboxes.ts—awaitCompleted,executeAndAwaitCompletionuselongPollUntildirectly, removing boilerplate 408 error handlers and placeholder results.Execution.result()updated — useslongPollUntilinstead of rawwaitForCommand, respectslongPoll.timeoutMs.(this._client as any)._options.timeoutcasts tothis._client.timeoutacross all resource methods.poll()hardened — input validation for negative/zero timing params,maxAttempts=0now means initial-request-only, timeout cleanup moved tofinallyblock, delays are raced against the timeout.Migration for SDK users
Before:
After:
The old
polling: { timeoutMs }path still works but will emit a deprecation warning. Fields likemaxAttemptsandpollingIntervalMsare silently ignored (with a one-time warning) since the server handles blocking and retry timing.Testing
longPollUntil(retry on 408, deadline enforcement,onAttemptcallback, error propagation)awaitDevboxState(transition states, target state, timeout, 408 retry)resolveLongPollTimeoutMs(deprecation warnings, precedence, reset)longPolloption (with some kept on deprecatedpollingpath to verify backwards compat)CodeAnt-AI Description
Use server-side long-polling for wait endpoints and add longPoll timeout option
What Changed
Impact
✅ Clearer timeout errors for long waits✅ Fewer unnecessary retries for server-side wait endpoints✅ In-flight waits can be cancelled promptly via AbortSignal💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.