fix(prime-sandboxes): stream file transfers - #859
Conversation
|
Reviewer doubt: the async upload path now passes a regular file object to httpx so multipart data can stream, but that read happens inside the async request. Please confirm that this tradeoff is preferred over adding a custom async multipart stream, and that the 16 MiB internal chunk size is appropriate for the gateway's current transfer behavior. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ac94925f3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with open(local_file_path, "wb") as f: | ||
| f.write(response.content) | ||
| os.makedirs(dir_path or ".", exist_ok=True) | ||
| temp_file = tempfile.NamedTemporaryFile( |
There was a problem hiding this comment.
On POSIX systems, NamedTemporaryFile creates the temporary file with mode 0600, and os.replace carries that mode to the destination. Consequently, every successful download now changes an existing destination's permissions to 0600 and creates new downloads without the caller's normal umask-derived permissions; executable files or files consumed by another user/process can stop working. The sync and async paths should set the temporary file's mode appropriately before replacing the destination.
Useful? React with 👍 / 👎.
| with open(local_file_path, "rb") as file: | ||
| files = {"file": (os.path.basename(local_file_path), file)} | ||
| response = await self._gateway_post( | ||
| url, headers=headers, timeout=effective_timeout, files=files, params=params |
There was a problem hiding this comment.
When AsyncSandboxClient.upload_file is used with a slow or network-backed local file, passing this ordinary synchronous file object to HTTPX's async multipart encoder causes its .read() calls to execute on the event-loop thread. Unlike the previous aiofiles implementation, these reads can stall every other coroutine sharing the loop during the upload; the multipart source should perform reads in a worker thread or otherwise provide genuinely asynchronous file I/O.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #824
Validation
python -m pytest packages/prime-sandboxes/tests -q --ignore=packages/prime-sandboxes/tests/test_file_operations.py --ignore=packages/prime-sandboxes/tests/test_cmd_timeout.py --ignore=packages/prime-sandboxes/tests/test_command_execution.py --ignore=packages/prime-sandboxes/tests/test_sandbox_operations.py(212 passed)ruff check --config packages/prime-sandboxes/pyproject.toml --select E,F,I ...(passed)ruff format --config packages/prime-sandboxes/pyproject.toml --check ...(passed)python -m compileall -q packages/prime-sandboxes/src packages/prime-sandboxes/tests(passed)Live sandbox integration tests were not run because they provision resources and require an API key.