mirror of
https://github.com/openai/codex.git
synced 2026-10-17 17:08:55 +00:00
Avoid redundant filesystem probes (#36393)
## What changed - Load `environments.toml` in one operation, falling back to the default environment provider only when the read reports that the file is missing. - Attempt the default daemon socket connection directly instead of checking the socket path first. - Preserve each file search entry's type from the directory walker, avoiding a second metadata lookup when producing matches and correctly classifying followed directory symlinks. - Write atomic-file contents through the existing temporary-file handle rather than reopening its path. ## Testing - Add Unix coverage for classifying a followed directory symlink as a directory in file search results. GitOrigin-RevId: aa6739cac23953ca8b9a3fcb53e43e72c41cfdf9
This commit is contained in:
@@ -197,16 +197,10 @@ pub(crate) fn environment_provider_from_codex_home(
|
||||
codex_home: &Path,
|
||||
) -> Result<Box<dyn EnvironmentProvider>, ExecServerError> {
|
||||
let path = codex_home.join(ENVIRONMENTS_TOML_FILE);
|
||||
if !path.try_exists().map_err(|err| {
|
||||
ExecServerError::Protocol(format!(
|
||||
"failed to inspect environment config `{}`: {err}",
|
||||
path.display()
|
||||
))
|
||||
})? {
|
||||
let Some(environments) = load_environments_toml(&path)? else {
|
||||
return Ok(Box::new(DefaultEnvironmentProvider::from_env()));
|
||||
}
|
||||
};
|
||||
|
||||
let environments = load_environments_toml(&path)?;
|
||||
Ok(Box::new(TomlEnvironmentProvider::new_with_config_dir(
|
||||
environments,
|
||||
Some(codex_home),
|
||||
@@ -295,20 +289,27 @@ fn validate_websocket_url(url: String) -> Result<String, ExecServerError> {
|
||||
Ok(url.to_string())
|
||||
}
|
||||
|
||||
fn load_environments_toml(path: &Path) -> Result<EnvironmentsToml, ExecServerError> {
|
||||
let contents = std::fs::read_to_string(path).map_err(|err| {
|
||||
ExecServerError::Protocol(format!(
|
||||
"failed to read environment config `{}`: {err}",
|
||||
path.display()
|
||||
))
|
||||
})?;
|
||||
/// Returns `None` when the config is missing; other I/O and parse failures remain errors.
|
||||
fn load_environments_toml(path: &Path) -> Result<Option<EnvironmentsToml>, ExecServerError> {
|
||||
let contents = match std::fs::read_to_string(path) {
|
||||
Ok(contents) => contents,
|
||||
Err(err) if err.kind() == std::io::ErrorKind::NotFound => return Ok(None),
|
||||
Err(err) => {
|
||||
return Err(ExecServerError::Protocol(format!(
|
||||
"failed to read environment config `{}`: {err}",
|
||||
path.display()
|
||||
)));
|
||||
}
|
||||
};
|
||||
|
||||
toml::from_str(&contents).map_err(|err| {
|
||||
ExecServerError::Protocol(format!(
|
||||
"failed to parse environment config `{}`: {err}",
|
||||
path.display()
|
||||
))
|
||||
})
|
||||
toml::from_str(&contents)
|
||||
.map_err(|err| {
|
||||
ExecServerError::Protocol(format!(
|
||||
"failed to parse environment config `{}`: {err}",
|
||||
path.display()
|
||||
))
|
||||
})
|
||||
.map(Some)
|
||||
}
|
||||
|
||||
mod option_duration_secs {
|
||||
@@ -756,7 +757,9 @@ CODEX_LOG = "debug"
|
||||
)
|
||||
.expect("write environments.toml");
|
||||
|
||||
let environments = load_environments_toml(&path).expect("environments.toml");
|
||||
let environments = load_environments_toml(&path)
|
||||
.expect("environments.toml")
|
||||
.expect("environments.toml should exist");
|
||||
|
||||
assert_eq!(environments.default.as_deref(), Some("ssh-dev"));
|
||||
assert_eq!(environments.include_local, Some(false));
|
||||
|
||||
@@ -67,6 +67,12 @@ pub enum MatchType {
|
||||
Directory,
|
||||
}
|
||||
|
||||
/// Carries the entry type observed by the walker so matched paths are not restatted.
|
||||
struct IndexedEntry {
|
||||
full_path: Arc<str>,
|
||||
match_type: MatchType,
|
||||
}
|
||||
|
||||
impl FileMatch {
|
||||
pub fn full_path(&self) -> PathBuf {
|
||||
self.root.join(&self.path)
|
||||
@@ -411,7 +417,7 @@ fn get_file_path<'a>(path: &'a Path, search_directories: &[PathBuf]) -> Option<(
|
||||
fn walker_worker(
|
||||
inner: Arc<SessionInner>,
|
||||
override_matcher: Option<ignore::overrides::Override>,
|
||||
injector: Injector<Arc<str>>,
|
||||
injector: Injector<IndexedEntry>,
|
||||
) {
|
||||
let Some(first_root) = inner.search_directories.first() else {
|
||||
let _ = inner.work_tx.send(WorkSignal::WalkComplete);
|
||||
@@ -463,9 +469,19 @@ fn walker_worker(
|
||||
return ignore::WalkState::Continue;
|
||||
};
|
||||
if let Some((_, relative_path)) = get_file_path(path, &search_directories) {
|
||||
injector.push(Arc::from(full_path), |_, cols| {
|
||||
cols[0] = Utf32String::from(relative_path);
|
||||
});
|
||||
let match_type = match entry.file_type() {
|
||||
Some(file_type) if file_type.is_dir() => MatchType::Directory,
|
||||
_ => MatchType::File,
|
||||
};
|
||||
injector.push(
|
||||
IndexedEntry {
|
||||
full_path: Arc::from(full_path),
|
||||
match_type,
|
||||
},
|
||||
|_, cols| {
|
||||
cols[0] = Utf32String::from(relative_path);
|
||||
},
|
||||
);
|
||||
}
|
||||
n += 1;
|
||||
if n >= CHECK_INTERVAL {
|
||||
@@ -483,7 +499,7 @@ fn walker_worker(
|
||||
fn matcher_worker(
|
||||
inner: Arc<SessionInner>,
|
||||
work_rx: Receiver<WorkSignal>,
|
||||
mut nucleo: Nucleo<Arc<str>>,
|
||||
mut nucleo: Nucleo<IndexedEntry>,
|
||||
) -> anyhow::Result<()> {
|
||||
const TICK_TIMEOUT_MS: u64 = 10;
|
||||
let config = Config::DEFAULT.match_paths();
|
||||
@@ -547,7 +563,7 @@ fn matcher_worker(
|
||||
.take(limit)
|
||||
.filter_map(|match_| {
|
||||
let item = snapshot.get_item(match_.idx)?;
|
||||
let full_path = item.data.as_ref();
|
||||
let full_path = item.data.full_path.as_ref();
|
||||
let (root_idx, relative_path) = get_file_path(Path::new(full_path), &inner.search_directories)?;
|
||||
let indices = if let Some(indices_matcher) = indices_matcher.as_mut() {
|
||||
let mut idx_vec = Vec::<u32>::new();
|
||||
@@ -559,15 +575,10 @@ fn matcher_worker(
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let match_type = if Path::new(full_path).is_dir() {
|
||||
MatchType::Directory
|
||||
} else {
|
||||
MatchType::File
|
||||
};
|
||||
Some(FileMatch {
|
||||
score: match_.score,
|
||||
path: PathBuf::from(relative_path),
|
||||
match_type,
|
||||
match_type: item.data.match_type,
|
||||
root: inner.search_directories[root_idx].clone(),
|
||||
indices,
|
||||
})
|
||||
@@ -1012,6 +1023,29 @@ mod tests {
|
||||
}));
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn run_classifies_followed_directory_symlink_as_directory() {
|
||||
use std::os::unix::fs::symlink;
|
||||
|
||||
let dir = tempfile::tempdir().unwrap();
|
||||
fs::create_dir(dir.path().join("guides")).unwrap();
|
||||
symlink(dir.path().join("guides"), dir.path().join("guides-link")).unwrap();
|
||||
|
||||
let results = run(
|
||||
"guides-link",
|
||||
vec![dir.path().to_path_buf()],
|
||||
FileSearchOptions::default(),
|
||||
/*cancel_flag*/ None,
|
||||
)
|
||||
.expect("run ok");
|
||||
|
||||
assert!(results.matches.iter().any(|file_match| {
|
||||
file_match.path == Path::new("guides-link")
|
||||
&& file_match.match_type == MatchType::Directory
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn cancel_exits_run() {
|
||||
let dir = create_temp_tree(/*file_count*/ 200);
|
||||
|
||||
@@ -414,10 +414,6 @@ async fn connect_remote_app_server(
|
||||
#[cfg(unix)]
|
||||
async fn maybe_probe_default_daemon_socket(codex_home: &Path) -> Option<AbsolutePathBuf> {
|
||||
let socket_path = codex_app_server_client::app_server_control_socket_path(codex_home).ok()?;
|
||||
if !socket_path.as_path().try_exists().unwrap_or(false) {
|
||||
return None;
|
||||
}
|
||||
|
||||
match tokio::time::timeout(
|
||||
AUTO_CONNECT_DAEMON_CONNECT_TIMEOUT,
|
||||
tokio::net::UnixStream::connect(socket_path.as_path()),
|
||||
|
||||
@@ -6,6 +6,7 @@ pub use env::is_wsl;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::collections::HashSet;
|
||||
use std::io;
|
||||
use std::io::Write;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::NamedTempFile;
|
||||
@@ -126,8 +127,8 @@ pub fn write_atomically(write_path: &Path, contents: &str) -> io::Result<()> {
|
||||
)
|
||||
})?;
|
||||
std::fs::create_dir_all(parent)?;
|
||||
let tmp = NamedTempFile::new_in(parent)?;
|
||||
std::fs::write(tmp.path(), contents)?;
|
||||
let mut tmp = NamedTempFile::new_in(parent)?;
|
||||
tmp.write_all(contents.as_bytes())?;
|
||||
tmp.persist(write_path)?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user