[codex] exec-server: stream files in chunks#28354
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 741b2b9213
ℹ️ 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".
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ab7aba16a
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
jif-oai
left a comment
There was a problem hiding this comment.
Happy to discuss those points but I like the direction that this took
| ) -> FileSystemResult<tokio::fs::File> { | ||
| reject_sandbox_context(sandbox)?; | ||
| let path = path.to_abs_path()?; | ||
| if !tokio::fs::metadata(path.as_path()).await?.is_file() { |
There was a problem hiding this comment.
I guess you know it but this is still Windows unsafe right?
PathUri round-trips \\.\pipe\..., metadata() itself opens the path with default QOS, and is_file() does not prove FILE_TYPE_DISK
Thsi exposed to named-pipe impersonation etc
| format!("file read handle `{handle_id}` already exists"), | ||
| )); | ||
| } | ||
| if handles.len() >= MAX_OPEN_FILE_READS { |
There was a problem hiding this comment.
Ultra nit: the 128-entries cap doesn’t bound memory because handleId is an unrestricted caller string. stdio has no message-size cap so a gigantic id remain resident
Since the client is using uuids anyway, we could simply enforce >= 16 bytes or something like this (even 32 if we want a margin)
| ) -> FileSystemResult<tokio::fs::File> { | ||
| reject_sandbox_context(sandbox)?; | ||
| let path = path.to_abs_path()?; | ||
| if !tokio::fs::metadata(path.as_path()).await?.is_file() { |
There was a problem hiding this comment.
I also think there is something slightly racy here. metadata() validates one path target, then File::open() resolves it again but after an await
I don't think this is a big deal but it would be cleaner to open nonblocking and validated the opened descriptor instead of validating the path name (for unix ofc)
…-full-ci # Conflicts: # codex-rs/app-server/tests/suite/v2/external_agent_config.rs
Why
fs/readFilebuffers the entire file in one response, which makes large remote reads expensive and prevents callers from applying backpressure. We need an opt-in streaming path with bounded block sizes while preserving the existing single-call API for small and sandboxed reads.What changed
ExecServerClient::stream, returning a namedFileReadStreamthat implementsfutures::Streamand yields immutable 1 MiB byte blocks.fs/open,fs/readBlock, andfs/closeRPCs.fs/readBlockaccepts an explicit offset and length.fs/readFilebehavior is unchanged.Testing
just test -p codex-exec-server