Skip to content

Commit e3f44ca

Browse files
Fix plugin cache panic when cwd is unavailable (openai#18499)
## Summary Fixes openai#16637. (I hit this bug after 11h of work on a long-running task.) Plugin cache initialization could panic when an already-absolute cache path was normalized through `AbsolutePathBuf::from_absolute_path`, because that path still consulted `current_dir()`. This changes absolute-path normalization so already-absolute paths do not depend on cwd, and makes plugin cache root construction available as a fallible path through `PluginStore::try_new()`. Plugin cache subpaths now use `AbsolutePathBuf::join()` instead of re-absolutizing derived absolute paths.
1 parent 53b1570 commit e3f44ca

5 files changed

Lines changed: 80 additions & 20 deletions

File tree

codex-rs/core-plugins/src/loader.rs

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ pub fn refresh_curated_plugin_cache(
129129
plugin_version: &str,
130130
configured_curated_plugin_ids: &[PluginId],
131131
) -> Result<bool, String> {
132-
let store = PluginStore::new(codex_home.to_path_buf());
132+
let store = PluginStore::try_new(codex_home.to_path_buf()).map_err(|err| err.to_string())?;
133133
let curated_marketplace_path = AbsolutePathBuf::try_from(
134134
codex_home
135135
.join(".tmp/plugins")
@@ -234,7 +234,7 @@ fn refresh_non_curated_plugin_cache_with_mode(
234234
.map(PluginId::as_key)
235235
.collect::<HashSet<_>>();
236236

237-
let store = PluginStore::new(codex_home.to_path_buf());
237+
let store = PluginStore::try_new(codex_home.to_path_buf()).map_err(|err| err.to_string())?;
238238
let marketplace_outcome = list_marketplaces(additional_roots)
239239
.map_err(|err| format!("failed to discover marketplaces for cache refresh: {err}"))?;
240240
let mut plugin_sources = HashMap::<String, MarketplacePluginSource>::new();
@@ -742,7 +742,13 @@ pub async fn installed_plugin_telemetry_metadata(
742742
codex_home: &Path,
743743
plugin_id: &PluginId,
744744
) -> PluginTelemetryMetadata {
745-
let store = PluginStore::new(codex_home.to_path_buf());
745+
let store = match PluginStore::try_new(codex_home.to_path_buf()) {
746+
Ok(store) => store,
747+
Err(err) => {
748+
warn!("failed to resolve plugin cache root: {err}");
749+
return PluginTelemetryMetadata::from_plugin_id(plugin_id);
750+
}
751+
};
746752
let Some(plugin_root) = store.active_plugin_root(plugin_id) else {
747753
return PluginTelemetryMetadata::from_plugin_id(plugin_id);
748754
};

codex-rs/core-plugins/src/store.rs

Lines changed: 13 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,33 +28,29 @@ pub struct PluginStore {
2828

2929
impl PluginStore {
3030
pub fn new(codex_home: PathBuf) -> Self {
31-
Self {
32-
root: AbsolutePathBuf::try_from(codex_home.join(PLUGINS_CACHE_DIR))
33-
.unwrap_or_else(|err| panic!("plugin cache root should be absolute: {err}")),
34-
}
31+
Self::try_new(codex_home)
32+
.unwrap_or_else(|err| panic!("plugin cache root should be absolute: {err}"))
33+
}
34+
35+
pub fn try_new(codex_home: PathBuf) -> Result<Self, PluginStoreError> {
36+
let root = AbsolutePathBuf::from_absolute_path_checked(codex_home.join(PLUGINS_CACHE_DIR))
37+
.map_err(|err| PluginStoreError::io("failed to resolve plugin cache root", err))?;
38+
39+
Ok(Self { root })
3540
}
3641

3742
pub fn root(&self) -> &AbsolutePathBuf {
3843
&self.root
3944
}
4045

4146
pub fn plugin_base_root(&self, plugin_id: &PluginId) -> AbsolutePathBuf {
42-
AbsolutePathBuf::try_from(
43-
self.root
44-
.as_path()
45-
.join(&plugin_id.marketplace_name)
46-
.join(&plugin_id.plugin_name),
47-
)
48-
.unwrap_or_else(|err| panic!("plugin cache path should resolve to an absolute path: {err}"))
47+
self.root
48+
.join(&plugin_id.marketplace_name)
49+
.join(&plugin_id.plugin_name)
4950
}
5051

5152
pub fn plugin_root(&self, plugin_id: &PluginId, plugin_version: &str) -> AbsolutePathBuf {
52-
AbsolutePathBuf::try_from(
53-
self.plugin_base_root(plugin_id)
54-
.as_path()
55-
.join(plugin_version),
56-
)
57-
.unwrap_or_else(|err| panic!("plugin cache path should resolve to an absolute path: {err}"))
53+
self.plugin_base_root(plugin_id).join(plugin_version)
5854
}
5955

6056
pub fn active_plugin_version(&self, plugin_id: &PluginId) -> Option<String> {

codex-rs/core-plugins/src/store_tests.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,18 @@ fn write_plugin(root: &Path, dir_name: &str, manifest_name: &str) {
3333
);
3434
}
3535

36+
#[test]
37+
fn try_new_rejects_relative_codex_home() {
38+
let err = PluginStore::try_new(PathBuf::from("relative"))
39+
.expect_err("relative codex home should fail");
40+
let err = err.to_string().replace('\\', "/");
41+
42+
assert_eq!(
43+
err,
44+
"failed to resolve plugin cache root: path is not absolute: relative/plugins/cache"
45+
);
46+
}
47+
3648
#[test]
3749
fn install_copies_plugin_into_default_marketplace() {
3850
let tmp = tempdir().unwrap();

codex-rs/utils/absolute-path/src/absolutize.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ use std::path::Path;
1212
use std::path::PathBuf;
1313

1414
pub(super) fn absolutize(path: &Path) -> std::io::Result<PathBuf> {
15+
if path.is_absolute() {
16+
return Ok(normalize_path(path));
17+
}
18+
1519
Ok(absolutize_from(path, &std::env::current_dir()?))
1620
}
1721

codex-rs/utils/absolute-path/src/lib.rs

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,8 @@ mod tests {
328328
use crate::test_support::test_path_buf;
329329
use pretty_assertions::assert_eq;
330330
use std::fs;
331+
#[cfg(unix)]
332+
use std::process::Command;
331333
use tempfile::tempdir;
332334

333335
#[test]
@@ -341,6 +343,46 @@ mod tests {
341343
assert_eq!(abs_path_buf.as_path(), absolute_path.as_path());
342344
}
343345

346+
#[cfg(unix)]
347+
#[test]
348+
fn from_absolute_path_does_not_read_current_dir_when_path_is_absolute() {
349+
let status = Command::new(std::env::current_exe().expect("current test binary"))
350+
.arg("from_absolute_path_with_removed_current_dir_child")
351+
.arg("--ignored")
352+
.env("CODEX_ABSOLUTE_PATH_REMOVED_CWD_CHILD", "1")
353+
.status()
354+
.expect("run child test");
355+
356+
assert!(status.success());
357+
}
358+
359+
#[cfg(unix)]
360+
#[test]
361+
#[ignore]
362+
fn from_absolute_path_with_removed_current_dir_child() {
363+
if std::env::var_os("CODEX_ABSOLUTE_PATH_REMOVED_CWD_CHILD").is_none() {
364+
return;
365+
}
366+
367+
let original_cwd = std::env::current_dir().expect("original cwd");
368+
let temp_dir = tempdir().expect("temp dir");
369+
let removed_cwd = temp_dir.path().to_path_buf();
370+
std::env::set_current_dir(&removed_cwd).expect("enter temp dir");
371+
std::fs::remove_dir(&removed_cwd).expect("remove current dir");
372+
std::env::current_dir().expect_err("current dir should be unavailable");
373+
374+
let path = AbsolutePathBuf::from_absolute_path(test_path_buf(
375+
"/tmp/codex/../codex-home/plugins/cache",
376+
))
377+
.expect("absolute path should not require current dir");
378+
379+
std::env::set_current_dir(original_cwd).expect("restore cwd");
380+
assert_eq!(
381+
path.as_path(),
382+
test_path_buf("/tmp/codex-home/plugins/cache")
383+
);
384+
}
385+
344386
#[test]
345387
fn from_absolute_path_checked_rejects_relative_path() {
346388
let err = AbsolutePathBuf::from_absolute_path_checked("relative/path")

0 commit comments

Comments
 (0)