mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
perf(exec-server): batch filesystem metadata requests
(cherry picked from commit e22fbb31b4f86a04127be09acb2301905d2b03e9)
This commit is contained in:
@@ -21,6 +21,7 @@ use codex_config::default_project_root_markers;
|
||||
use codex_config::merge_toml_values;
|
||||
use codex_config::project_root_markers_from_config;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::FS_GET_METADATA_BATCH_MAX_PATHS;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
|
||||
@@ -11,6 +11,7 @@ use codex_utils_path_uri::PathUri;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
|
||||
use crate::JSONRPCErrorError;
|
||||
use crate::ProcessId;
|
||||
|
||||
pub const INITIALIZE_METHOD: &str = "initialize";
|
||||
@@ -31,6 +32,7 @@ pub const FS_CLOSE_METHOD: &str = "fs/close";
|
||||
pub const FS_WRITE_FILE_METHOD: &str = "fs/writeFile";
|
||||
pub const FS_CREATE_DIRECTORY_METHOD: &str = "fs/createDirectory";
|
||||
pub const FS_GET_METADATA_METHOD: &str = "fs/getMetadata";
|
||||
pub const FS_GET_METADATA_BATCH_METHOD: &str = "fs/getMetadataBatch";
|
||||
pub const FS_CANONICALIZE_METHOD: &str = "fs/canonicalize";
|
||||
pub const FS_READ_DIRECTORY_METHOD: &str = "fs/readDirectory";
|
||||
pub const FS_WALK_METHOD: &str = "fs/walk";
|
||||
@@ -342,6 +344,32 @@ pub struct FsGetMetadataResponse {
|
||||
pub modified_at_ms: i64,
|
||||
}
|
||||
|
||||
/// Maximum number of paths accepted by one `fs/getMetadataBatch` request.
|
||||
pub const FS_GET_METADATA_BATCH_MAX_PATHS: usize = 256;
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsGetMetadataBatchParams {
|
||||
/// Paths are evaluated and returned in this order.
|
||||
pub paths: Vec<PathUri>,
|
||||
/// One sandbox context shared by every path in the batch.
|
||||
pub sandbox: Option<FileSystemSandboxContext>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
#[serde(tag = "type", rename_all = "camelCase")]
|
||||
pub enum FsGetMetadataBatchResult {
|
||||
Metadata { metadata: FsGetMetadataResponse },
|
||||
Error { error: JSONRPCErrorError },
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsGetMetadataBatchResponse {
|
||||
/// One result for each requested path, in request order.
|
||||
pub results: Vec<FsGetMetadataBatchResult>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsCanonicalizeParams {
|
||||
@@ -571,6 +599,7 @@ mod tests {
|
||||
use super::EnvironmentInfo;
|
||||
use super::ExecExitedNotification;
|
||||
use super::ExecParams;
|
||||
use super::FsGetMetadataBatchResponse;
|
||||
use super::FsReadFileParams;
|
||||
use super::HttpRequestParams;
|
||||
use super::InitializeResponse;
|
||||
@@ -692,6 +721,22 @@ mod tests {
|
||||
.expect_err("native absolute sandbox cwd should not deserialize as a URI");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn metadata_batch_error_result_round_trips() {
|
||||
let serialized = serde_json::json!({
|
||||
"results": [{
|
||||
"type": "error",
|
||||
"error": { "code": -32004, "message": "missing" },
|
||||
}],
|
||||
});
|
||||
let response = serde_json::from_value::<FsGetMetadataBatchResponse>(serialized.clone())
|
||||
.expect("deserialize metadata batch");
|
||||
assert_eq!(
|
||||
serde_json::to_value(response).expect("serialize metadata batch"),
|
||||
serialized
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn http_request_timeout_treats_omitted_and_null_as_no_timeout() {
|
||||
let omitted: HttpRequestParams = serde_json::from_value(serde_json::json!({
|
||||
|
||||
@@ -53,6 +53,7 @@ use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_CLOSE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_BATCH_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
use crate::protocol::FS_OPEN_METHOD;
|
||||
use crate::protocol::FS_READ_BLOCK_METHOD;
|
||||
@@ -69,6 +70,8 @@ use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataBatchParams;
|
||||
use crate::protocol::FsGetMetadataBatchResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsOpenParams;
|
||||
@@ -706,6 +709,13 @@ impl ExecServerClient {
|
||||
self.call(FS_GET_METADATA_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_get_metadata_batch(
|
||||
&self,
|
||||
params: FsGetMetadataBatchParams,
|
||||
) -> Result<FsGetMetadataBatchResponse, ExecServerError> {
|
||||
self.call(FS_GET_METADATA_BATCH_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
|
||||
@@ -101,6 +101,7 @@ pub use protocol::ExecOutputDeltaNotification;
|
||||
pub use protocol::ExecOutputStream;
|
||||
pub use protocol::ExecParams;
|
||||
pub use protocol::ExecResponse;
|
||||
pub use protocol::FS_GET_METADATA_BATCH_MAX_PATHS;
|
||||
pub use protocol::FsCanonicalizeParams;
|
||||
pub use protocol::FsCanonicalizeResponse;
|
||||
pub use protocol::FsCloseParams;
|
||||
@@ -109,6 +110,9 @@ pub use protocol::FsCopyParams;
|
||||
pub use protocol::FsCopyResponse;
|
||||
pub use protocol::FsCreateDirectoryParams;
|
||||
pub use protocol::FsCreateDirectoryResponse;
|
||||
pub use protocol::FsGetMetadataBatchParams;
|
||||
pub use protocol::FsGetMetadataBatchResponse;
|
||||
pub use protocol::FsGetMetadataBatchResult;
|
||||
pub use protocol::FsGetMetadataParams;
|
||||
pub use protocol::FsGetMetadataResponse;
|
||||
pub use protocol::FsOpenParams;
|
||||
|
||||
@@ -21,6 +21,8 @@ use crate::client::LazyRemoteExecServerClient;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataBatchParams;
|
||||
use crate::protocol::FsGetMetadataBatchResult;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
@@ -152,14 +154,61 @@ impl RemoteFileSystem {
|
||||
})
|
||||
.await
|
||||
.map_err(map_remote_error)?;
|
||||
Ok(FileMetadata {
|
||||
is_directory: response.is_directory,
|
||||
is_file: response.is_file,
|
||||
is_symlink: response.is_symlink,
|
||||
size: response.size,
|
||||
created_at_ms: response.created_at_ms,
|
||||
modified_at_ms: response.modified_at_ms,
|
||||
})
|
||||
Ok(file_metadata_from_response(response))
|
||||
}
|
||||
|
||||
async fn get_metadata_batch(
|
||||
&self,
|
||||
paths: &[PathUri],
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<Vec<FileSystemResult<FileMetadata>>> {
|
||||
trace!(path_count = paths.len(), "remote fs get_metadata_batch");
|
||||
let client = self.client.get().await.map_err(map_remote_error)?;
|
||||
let response = match client
|
||||
.fs_get_metadata_batch(FsGetMetadataBatchParams {
|
||||
paths: paths.to_vec(),
|
||||
sandbox: remote_sandbox_context(sandbox),
|
||||
})
|
||||
.await
|
||||
{
|
||||
Ok(response) => response,
|
||||
Err(ExecServerError::Server {
|
||||
code: METHOD_NOT_FOUND_ERROR_CODE,
|
||||
..
|
||||
}) => {
|
||||
let mut results = Vec::with_capacity(paths.len());
|
||||
for path in paths {
|
||||
results.push(self.get_metadata(path, sandbox).await);
|
||||
}
|
||||
return Ok(results);
|
||||
}
|
||||
Err(error) => return Err(map_remote_error(error)),
|
||||
};
|
||||
if response.results.len() != paths.len() {
|
||||
return Err(io::Error::new(
|
||||
io::ErrorKind::InvalidData,
|
||||
format!(
|
||||
"remote fs/getMetadataBatch returned {} results for {} paths",
|
||||
response.results.len(),
|
||||
paths.len()
|
||||
),
|
||||
));
|
||||
}
|
||||
Ok(response
|
||||
.results
|
||||
.into_iter()
|
||||
.map(|result| match result {
|
||||
FsGetMetadataBatchResult::Metadata { metadata } => {
|
||||
Ok(file_metadata_from_response(metadata))
|
||||
}
|
||||
FsGetMetadataBatchResult::Error { error } => {
|
||||
Err(map_remote_error(ExecServerError::Server {
|
||||
code: error.code,
|
||||
message: error.message,
|
||||
}))
|
||||
}
|
||||
})
|
||||
.collect())
|
||||
}
|
||||
|
||||
async fn read_directory(
|
||||
@@ -313,6 +362,14 @@ impl ExecutorFileSystem for RemoteFileSystem {
|
||||
Box::pin(RemoteFileSystem::get_metadata(self, path, sandbox))
|
||||
}
|
||||
|
||||
fn get_metadata_batch<'a>(
|
||||
&'a self,
|
||||
paths: &'a [PathUri],
|
||||
sandbox: Option<&'a FileSystemSandboxContext>,
|
||||
) -> ExecutorFileSystemFuture<'a, Vec<FileSystemResult<FileMetadata>>> {
|
||||
Box::pin(RemoteFileSystem::get_metadata_batch(self, paths, sandbox))
|
||||
}
|
||||
|
||||
fn read_directory<'a>(
|
||||
&'a self,
|
||||
path: &'a PathUri,
|
||||
@@ -364,6 +421,17 @@ fn remote_sandbox_context(
|
||||
.map(FileSystemSandboxContext::drop_cwd_if_unused)
|
||||
}
|
||||
|
||||
fn file_metadata_from_response(response: crate::protocol::FsGetMetadataResponse) -> FileMetadata {
|
||||
FileMetadata {
|
||||
is_directory: response.is_directory,
|
||||
is_file: response.is_file,
|
||||
is_symlink: response.is_symlink,
|
||||
size: response.size,
|
||||
created_at_ms: response.created_at_ms,
|
||||
modified_at_ms: response.modified_at_ms,
|
||||
}
|
||||
}
|
||||
|
||||
fn map_remote_error(error: ExecServerError) -> io::Error {
|
||||
match error {
|
||||
ExecServerError::Server { code, message } if code == NOT_FOUND_ERROR_CODE => {
|
||||
|
||||
@@ -11,6 +11,7 @@ use crate::ExecutorFileSystem;
|
||||
use crate::RemoveOptions;
|
||||
use crate::file_read::FileReadHandleManager;
|
||||
use crate::local_file_system::LocalFileSystem;
|
||||
use crate::protocol::FS_GET_METADATA_BATCH_MAX_PATHS;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
@@ -20,6 +21,9 @@ use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataBatchParams;
|
||||
use crate::protocol::FsGetMetadataBatchResponse;
|
||||
use crate::protocol::FsGetMetadataBatchResult;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsOpenParams;
|
||||
@@ -169,6 +173,38 @@ impl FileSystemHandler {
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) async fn get_metadata_batch(
|
||||
&self,
|
||||
params: FsGetMetadataBatchParams,
|
||||
) -> Result<FsGetMetadataBatchResponse, JSONRPCErrorError> {
|
||||
if params.paths.len() > FS_GET_METADATA_BATCH_MAX_PATHS {
|
||||
return Err(invalid_request(format!(
|
||||
"metadata batch must not exceed {FS_GET_METADATA_BATCH_MAX_PATHS} paths"
|
||||
)));
|
||||
}
|
||||
let mut results = Vec::with_capacity(params.paths.len());
|
||||
for path in params.paths {
|
||||
let result = self
|
||||
.file_system
|
||||
.get_metadata(&path, params.sandbox.as_ref())
|
||||
.await
|
||||
.map(|metadata| FsGetMetadataResponse {
|
||||
is_directory: metadata.is_directory,
|
||||
is_file: metadata.is_file,
|
||||
is_symlink: metadata.is_symlink,
|
||||
size: metadata.size,
|
||||
created_at_ms: metadata.created_at_ms,
|
||||
modified_at_ms: metadata.modified_at_ms,
|
||||
})
|
||||
.map_err(map_fs_error);
|
||||
match result {
|
||||
Ok(metadata) => results.push(FsGetMetadataBatchResult::Metadata { metadata }),
|
||||
Err(error) => results.push(FsGetMetadataBatchResult::Error { error }),
|
||||
}
|
||||
}
|
||||
Ok(FsGetMetadataBatchResponse { results })
|
||||
}
|
||||
|
||||
pub(crate) async fn canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
@@ -274,6 +310,8 @@ mod tests {
|
||||
|
||||
use super::*;
|
||||
use crate::FileSystemSandboxContext;
|
||||
use crate::protocol::FS_GET_METADATA_BATCH_MAX_PATHS;
|
||||
use crate::protocol::FsGetMetadataBatchParams;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
use crate::protocol::FsWriteFileParams;
|
||||
|
||||
@@ -342,4 +380,31 @@ mod tests {
|
||||
assert_eq!(response.data_base64, STANDARD.encode("ok"));
|
||||
}
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn metadata_batch_rejects_too_many_paths_before_dispatch() {
|
||||
let handler = FileSystemHandler::new(
|
||||
ExecServerRuntimePaths::new(
|
||||
std::env::current_exe().expect("current exe"),
|
||||
/*codex_linux_sandbox_exe*/ None,
|
||||
)
|
||||
.expect("runtime paths"),
|
||||
);
|
||||
let path = PathUri::from_host_native_path(std::env::current_dir().expect("current dir"))
|
||||
.expect("current dir URI");
|
||||
|
||||
let error = handler
|
||||
.get_metadata_batch(FsGetMetadataBatchParams {
|
||||
paths: vec![path; FS_GET_METADATA_BATCH_MAX_PATHS + 1],
|
||||
sandbox: None,
|
||||
})
|
||||
.await
|
||||
.expect_err("oversized metadata batch should fail");
|
||||
|
||||
assert_eq!(error.code, -32600);
|
||||
assert_eq!(
|
||||
error.message,
|
||||
format!("metadata batch must not exceed {FS_GET_METADATA_BATCH_MAX_PATHS} paths")
|
||||
);
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
@@ -25,6 +25,8 @@ use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataBatchParams;
|
||||
use crate::protocol::FsGetMetadataBatchResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsOpenParams;
|
||||
@@ -300,6 +302,14 @@ impl ExecServerHandler {
|
||||
self.file_system.get_metadata(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_get_metadata_batch(
|
||||
&self,
|
||||
params: FsGetMetadataBatchParams,
|
||||
) -> Result<FsGetMetadataBatchResponse, JSONRPCErrorError> {
|
||||
self.require_initialized_for("filesystem")?;
|
||||
self.file_system.get_metadata_batch(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
|
||||
@@ -11,6 +11,7 @@ use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_CLOSE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_BATCH_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
use crate::protocol::FS_OPEN_METHOD;
|
||||
use crate::protocol::FS_READ_BLOCK_METHOD;
|
||||
@@ -23,6 +24,7 @@ use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCloseParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataBatchParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsOpenParams;
|
||||
use crate::protocol::FsReadBlockParams;
|
||||
@@ -137,6 +139,12 @@ pub(crate) fn build_router() -> RpcRouter<ExecServerHandler> {
|
||||
handler.fs_get_metadata(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_GET_METADATA_BATCH_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsGetMetadataBatchParams| async move {
|
||||
handler.fs_get_metadata_batch(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_CANONICALIZE_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsCanonicalizeParams| async move {
|
||||
|
||||
@@ -112,6 +112,52 @@ async fn file_system_get_metadata_reports_files_and_directories(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(FileSystemImplementation::Local ; "local")]
|
||||
#[test_case(FileSystemImplementation::Remote ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_get_metadata_batch_preserves_order_and_errors(
|
||||
implementation: FileSystemImplementation,
|
||||
) -> Result<()> {
|
||||
let context = create_file_system_context(implementation).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let file_path = tmp.path().join("note.txt");
|
||||
let missing_path = tmp.path().join("missing.txt");
|
||||
let directory_path = tmp.path().join("notes");
|
||||
std::fs::write(&file_path, "hello")?;
|
||||
std::fs::create_dir(&directory_path)?;
|
||||
let paths = [&file_path, &missing_path, &directory_path]
|
||||
.into_iter()
|
||||
.map(PathUri::from_host_native_path)
|
||||
.collect::<std::io::Result<Vec<_>>>()?;
|
||||
let expected_file = file_system
|
||||
.get_metadata(&paths[0], /*sandbox*/ None)
|
||||
.await?;
|
||||
let expected_directory = file_system
|
||||
.get_metadata(&paths[2], /*sandbox*/ None)
|
||||
.await?;
|
||||
|
||||
let results = file_system
|
||||
.get_metadata_batch(&paths, /*sandbox*/ None)
|
||||
.await
|
||||
.with_context(|| format!("mode={implementation}"))?;
|
||||
let actual = results
|
||||
.into_iter()
|
||||
.map(|result| result.map_err(|error| error.kind()))
|
||||
.collect::<Vec<_>>();
|
||||
|
||||
assert_eq!(
|
||||
actual,
|
||||
vec![
|
||||
Ok(expected_file),
|
||||
Err(std::io::ErrorKind::NotFound),
|
||||
Ok(expected_directory),
|
||||
]
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(FileSystemImplementation::Local ; "local")]
|
||||
#[test_case(FileSystemImplementation::Remote ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
|
||||
@@ -306,6 +306,24 @@ pub trait ExecutorFileSystem: Send + Sync {
|
||||
sandbox: Option<&'a FileSystemSandboxContext>,
|
||||
) -> ExecutorFileSystemFuture<'a, FileMetadata>;
|
||||
|
||||
/// Reads metadata for multiple paths, preserving request order and per-path errors.
|
||||
///
|
||||
/// Implementations with an optimized batch transport can override this method. The default
|
||||
/// preserves compatibility by issuing the existing single-path operation for each path.
|
||||
fn get_metadata_batch<'a>(
|
||||
&'a self,
|
||||
paths: &'a [PathUri],
|
||||
sandbox: Option<&'a FileSystemSandboxContext>,
|
||||
) -> ExecutorFileSystemFuture<'a, Vec<FileSystemResult<FileMetadata>>> {
|
||||
Box::pin(async move {
|
||||
let mut results = Vec::with_capacity(paths.len());
|
||||
for path in paths {
|
||||
results.push(self.get_metadata(path, sandbox).await);
|
||||
}
|
||||
Ok(results)
|
||||
})
|
||||
}
|
||||
|
||||
fn read_directory<'a>(
|
||||
&'a self,
|
||||
path: &'a PathUri,
|
||||
|
||||
Reference in New Issue
Block a user