fix(sandboxes): bound background output retrieval - #862
fix(sandboxes): bound background output retrieval#862DamianB-BitFlipper wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ca296c473
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| response = self.read_file( | ||
| sandbox_id, | ||
| path, | ||
| timeout=timeout, | ||
| timeout=request_timeout, | ||
| offset=-JOB_OUTPUT_TAIL_BYTES, | ||
| length=JOB_OUTPUT_TAIL_BYTES, | ||
| ) |
There was a problem hiding this comment.
Enforce the deadline around synchronous output reads
When a synchronous gateway read stalls or repeatedly times out, this call is not bounded by deadline: read_file invokes the retry-decorated _gateway_read_file_get up to four times, and every attempt may consume the full request_timeout plus retry delays. With the default settings, one stream can therefore block for roughly two minutes despite the advertised 45-second output deadline, and successful output may even be returned after that deadline. Wrap the complete synchronous read/retry operation in an actual deadline mechanism or make its retries consume the remaining budget.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8ca296c. Configure here.
| exit_code=exit_code, | ||
| stdout=stdout, | ||
| stderr=stderr, | ||
| stdout_truncated=stdout_truncated, |
There was a problem hiding this comment.
Output timeout hits exit checks
Medium Severity
timeout is documented as an output-retrieval deadline after completion is known, but get_background_job still forwards it to the exit-file read_file call and get_background_jobs still forwards it to the platform batch request. A short or zero deadline can fail completion detection itself instead of returning completed=True with stdout_error / stderr_error.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 8ca296c. Configure here.
| except APIError as exc: | ||
| return None, False, _format_exception_diagnostic(exc) | ||
| finally: | ||
| self._background_job_output_semaphore.release() |
There was a problem hiding this comment.
Sync deadline skips retries
Medium Severity
The async output path bounds the whole fetch with asyncio.wait_for, but sync only checks the deadline before calling read_file. That call still runs multi-attempt tenacity retries, so sync retrieval can run far past the configured output deadline while holding a _background_job_output_semaphore slot.
Reviewed by Cursor Bugbot for commit 8ca296c. Configure here.


Caps background-job output reads at 20 concurrent requests, retrieves stdout/stderr
sequentially, preserves completed exit codes when output retrieval fails, adds jittered retries and
errno diagnostics, and includes a 100-job concurrency test.
Verification:
Note
Medium Risk
Changes observable SDK behavior (timeout meaning, optional None stdout/stderr, new error fields) on a hot path used by batch VM job polling and gateway reads.
Overview
Hardens background job polling when many VM jobs finish at once: stdout/stderr tails are fetched sequentially under a client-wide cap of 20 concurrent gateway reads and a per-job output deadline (45s default;
get_background_job/get_background_jobstimeoutnow bounds retrieval after the exit file is seen, not eachread_file).Once an exit code is known,
completedandexit_codestay authoritative even if log reads fail or time out.BackgroundJobStatusgains optionalstdout_error/stderr_error; failed streams areNoneinstead of failing the whole status. Async output reads no longer run stdout/stderr in parallel.read_fileconnection errors use_format_exception_diagnostic(nested errno, e.g. EMFILE) and jittered exponential retries. README and tests cover concurrency limits, deadline behavior, and partial output failures.Reviewed by Cursor Bugbot for commit 8ca296c. Bugbot is set up for automated code reviews on this repo. Configure here.