-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[codex] exec-server: stream files in chunks #28354
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
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
7964543
exec-server: stream files in chunks
pakrym-oai 8108e89
exec-server: reuse request loop for file reads
pakrym-oai ae37730
exec-server: test file read handles
pakrym-oai 741b2b9
exec-server: test file reads over rpc
pakrym-oai f2c9b00
exec-server: allow more open file reads
pakrym-oai 66c1125
exec-server: use positional file reads
pakrym-oai 6fb7925
exec-server: assign file handles on the client
pakrym-oai 5ab7aba
exec-server: reject special files before streaming
pakrym-oai 8a5bf73
exec-server: expose file streams through filesystem
pakrym-oai 1ca386e
filesystem: require explicit stream support
pakrym-oai 79533b0
exec-server: keep file handles open after eof
pakrym-oai 25f4daf
exec-server: expose file streams only through filesystem
pakrym-oai f4d3c13
codex: expose file read protocol methods (#28354)
pakrym-oai 0277fea
Merge remote-tracking branch 'origin/main' into pakrym/fs-read-chunks…
pakrym-oai 701738c
codex: simplify file read streaming (#28354)
pakrym-oai ca49b47
codex: fix CI failure on PR #28354
pakrym-oai e43701d
codex: bound file read handle IDs (#28354)
pakrym-oai 699b25f
codex: validate opened file handles (#28354)
pakrym-oai ee57b6d
codex: fix CI failure on PR #28354
pakrym-oai 408b105
Merge remote-tracking branch 'origin/main' into pakrym/fs-read-chunks…
pakrym-oai File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| use std::collections::HashMap; | ||
| use std::fs::File; | ||
| use std::io; | ||
| use std::sync::Arc; | ||
|
|
||
| use codex_file_system::FILE_READ_CHUNK_SIZE; | ||
| use tokio::sync::Mutex; | ||
|
|
||
| const MAX_OPEN_FILE_READS: usize = 128; | ||
|
|
||
| #[derive(Debug, Eq, PartialEq)] | ||
| pub(crate) struct FileReadBlock { | ||
| pub(crate) bytes: Vec<u8>, | ||
| pub(crate) eof: bool, | ||
| } | ||
|
|
||
| #[derive(Clone, Default)] | ||
| pub(crate) struct FileReadHandleManager { | ||
| handles: Arc<Mutex<HashMap<String, Arc<File>>>>, | ||
| } | ||
|
|
||
| impl FileReadHandleManager { | ||
| pub(crate) async fn open( | ||
| &self, | ||
| handle_id: String, | ||
| file: tokio::fs::File, | ||
| ) -> io::Result<String> { | ||
| let file = Arc::new(file.into_std().await); | ||
| let mut handles = self.handles.lock().await; | ||
| if handles.contains_key(&handle_id) { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| format!("file read handle `{handle_id}` already exists"), | ||
| )); | ||
| } | ||
| if handles.len() >= MAX_OPEN_FILE_READS { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| format!("at most {MAX_OPEN_FILE_READS} file reads may be open per connection"), | ||
| )); | ||
| } | ||
| handles.insert(handle_id.clone(), file); | ||
| Ok(handle_id) | ||
| } | ||
|
|
||
| pub(crate) async fn read_block( | ||
| &self, | ||
| handle_id: &str, | ||
| offset: u64, | ||
| len: usize, | ||
| ) -> io::Result<FileReadBlock> { | ||
| validate_read_block_len(len)?; | ||
| let file = { | ||
| let handles = self.handles.lock().await; | ||
| handles | ||
| .get(handle_id) | ||
| .cloned() | ||
| .ok_or_else(|| unknown_handle_error(handle_id))? | ||
| }; | ||
| let result = | ||
| match tokio::task::spawn_blocking(move || read_block_at(&file, offset, len)).await { | ||
| Ok(result) => result, | ||
| Err(error) => Err(io::Error::other(format!( | ||
| "file read task stopped unexpectedly: {error}" | ||
| ))), | ||
| }; | ||
| if result.is_err() { | ||
| self.close(handle_id).await; | ||
| } | ||
| result | ||
| } | ||
|
|
||
| pub(crate) async fn close(&self, handle_id: &str) { | ||
| self.handles.lock().await.remove(handle_id); | ||
| } | ||
|
|
||
| pub(crate) async fn close_all(&self) { | ||
| self.handles.lock().await.clear(); | ||
| } | ||
| } | ||
|
|
||
| fn read_block_at(file: &File, offset: u64, len: usize) -> io::Result<FileReadBlock> { | ||
| let mut bytes = vec![0; len]; | ||
| let mut bytes_read = 0; | ||
| while bytes_read < len { | ||
| let read_offset = offset.checked_add(bytes_read as u64).ok_or_else(|| { | ||
| io::Error::new(io::ErrorKind::InvalidInput, "file read offset overflowed") | ||
| })?; | ||
| match read_file_at(file, &mut bytes[bytes_read..], read_offset) { | ||
| Ok(0) => break, | ||
| Ok(read) => bytes_read += read, | ||
| Err(error) if error.kind() == io::ErrorKind::Interrupted => {} | ||
| Err(error) => return Err(error), | ||
| } | ||
| } | ||
| bytes.truncate(bytes_read); | ||
| Ok(FileReadBlock { | ||
| eof: bytes_read < len, | ||
| bytes, | ||
| }) | ||
| } | ||
|
|
||
| #[cfg(unix)] | ||
| fn read_file_at(file: &File, bytes: &mut [u8], offset: u64) -> io::Result<usize> { | ||
| std::os::unix::fs::FileExt::read_at(file, bytes, offset) | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| fn read_file_at(file: &File, bytes: &mut [u8], offset: u64) -> io::Result<usize> { | ||
| std::os::windows::fs::FileExt::seek_read(file, bytes, offset) | ||
| } | ||
|
|
||
| fn validate_read_block_len(len: usize) -> io::Result<()> { | ||
| if !(1..=FILE_READ_CHUNK_SIZE).contains(&len) { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidInput, | ||
| format!("file read block length must be between 1 and {FILE_READ_CHUNK_SIZE}"), | ||
| )); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn unknown_handle_error(handle_id: &str) -> io::Error { | ||
| io::Error::new( | ||
| io::ErrorKind::NotFound, | ||
| format!("unknown file read handle `{handle_id}`"), | ||
| ) | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Ultra nit: the 128-entries cap doesn’t bound memory because
handleIdis an unrestricted caller string. stdio has no message-size cap so a gigantic id remain residentSince the client is using uuids anyway, we could simply enforce >= 16 bytes or something like this (even 32 if we want a margin)
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.
Fixed