mirror of
https://github.com/openai/codex.git
synced 2026-11-12 21:20:34 +00:00
Reclaim abandoned trusted Git repositories (#61180)
## Why Temporary trusted Git repositories can remain on disk when a process exits without running directory cleanup. ## What changed Use `MarketplaceStagingDir` for trusted Git repositories under Codex home's `.tmp/.staging` directory. Clean up abandoned staging directories before creating a repository, while ownership locks protect repositories still in use. Reject a symlinked `.staging` directory to keep `GIT_DIR` inside Codex home. ## Testing Update the cross-process staging test to create trusted Git repositories and verify that cleanup preserves live owners, reclaims a repository after its process exits, and leaves a concurrent checkout intact. GitOrigin-RevId: f45b60db82814b4bcc09365f68d808c73d2d5378
This commit is contained in:
@@ -1,7 +1,8 @@
|
||||
use crate::marketplace_staging::MarketplaceStagingDir;
|
||||
use crate::marketplace_staging::cleanup_marketplace_staging;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::path::Path;
|
||||
use std::process::Command;
|
||||
use tempfile::TempDir;
|
||||
|
||||
// These variables can redirect Git to an untrusted repository or inject command-scoped settings.
|
||||
pub(crate) const REPOSITORY_LOCAL_GIT_ENVIRONMENT_VARIABLES: &[&str] = &[
|
||||
@@ -56,7 +57,7 @@ impl PluginGitMode {
|
||||
pub(crate) fn configure_trusted_git_repository(
|
||||
command: &mut Command,
|
||||
codex_home: &Path,
|
||||
) -> Result<TempDir, String> {
|
||||
) -> Result<MarketplaceStagingDir, String> {
|
||||
let canonical_home = AbsolutePathBuf::from_absolute_path(codex_home)
|
||||
.and_then(|path| path.canonicalize())
|
||||
.map_err(|err| format!("failed to resolve trusted Codex home: {err}"))?;
|
||||
@@ -70,12 +71,15 @@ pub(crate) fn configure_trusted_git_repository(
|
||||
return Err("trusted Git repository root escapes Codex home".to_string());
|
||||
}
|
||||
|
||||
let repository = tempfile::Builder::new()
|
||||
.prefix("git-")
|
||||
.tempdir_in(staging_root.as_path())
|
||||
// Keep the explicit GIT_DIR inside Codex home. The staging cleaner skips symlinks.
|
||||
if staging_root.join(".staging").as_path().is_symlink() {
|
||||
return Err("trusted Git staging directory is a symlink".to_string());
|
||||
}
|
||||
cleanup_marketplace_staging(staging_root.as_path());
|
||||
let repository = MarketplaceStagingDir::new(staging_root.as_path())
|
||||
.map_err(|err| format!("failed to create trusted Git repository: {err}"))?;
|
||||
for directory in ["objects", "refs"] {
|
||||
std::fs::create_dir(repository.path().join(directory))
|
||||
std::fs::create_dir_all(repository.path().join(directory))
|
||||
.map_err(|err| format!("failed to initialize trusted Git repository: {err}"))?;
|
||||
}
|
||||
std::fs::write(repository.path().join("HEAD"), "ref: refs/heads/main\n")
|
||||
|
||||
@@ -12,8 +12,11 @@ use std::process::Stdio;
|
||||
fn cleanup_preserves_live_process_and_reclaims_its_abandoned_checkout() -> Result<()> {
|
||||
const CHILD_HOME: &str = "CODEX_MARKETPLACE_STAGING_CHILD_HOME";
|
||||
if let Some(home) = std::env::var_os(CHILD_HOME) {
|
||||
let staging = MarketplaceStagingDir::new(Path::new(&home))?;
|
||||
fs::create_dir_all(staging.path())?;
|
||||
let staging = crate::configure_trusted_git_repository(
|
||||
&mut std::process::Command::new("git"),
|
||||
Path::new(&home),
|
||||
)
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
fs::write(staging.path().join("payload"), "in-flight clone")?;
|
||||
println!("staging-ready");
|
||||
std::io::stdout().flush()?;
|
||||
@@ -42,21 +45,28 @@ fn cleanup_preserves_live_process_and_reclaims_its_abandoned_checkout() -> Resul
|
||||
}
|
||||
}
|
||||
assert!(ready, "child should publish its checkout before cleanup");
|
||||
let staging_root = home.path().join(".staging");
|
||||
let tmp_root = home.path().join(".tmp");
|
||||
let staging_root = tmp_root.join(".staging");
|
||||
let active = fs::read_dir(&staging_root)?.next().unwrap()?.path();
|
||||
let payload = active.join("checkout/payload");
|
||||
|
||||
// Age must never override the lifetime lock, even across processes.
|
||||
remove_stale_staging_dirs(home.path(), SystemTime::now() + LEGACY_ADD_MAX_AGE)?;
|
||||
remove_stale_staging_dirs(&tmp_root, SystemTime::now() + LEGACY_ADD_MAX_AGE)?;
|
||||
assert_eq!(fs::read_to_string(&payload)?, "in-flight clone");
|
||||
let concurrent = MarketplaceStagingDir::new(home.path())?;
|
||||
let concurrent = MarketplaceStagingDir::new(&tmp_root)?;
|
||||
fs::create_dir_all(concurrent.path())?;
|
||||
fs::write(concurrent.path().join("payload"), "second clone")?;
|
||||
|
||||
child.stdin.take().unwrap().write_all(b"x")?;
|
||||
assert!(child.wait()?.success());
|
||||
assert!(payload.exists(), "process exit must bypass TempDir cleanup");
|
||||
cleanup_marketplace_staging(home.path());
|
||||
drop(
|
||||
crate::configure_trusted_git_repository(
|
||||
&mut std::process::Command::new("git"),
|
||||
home.path(),
|
||||
)
|
||||
.map_err(anyhow::Error::msg)?,
|
||||
);
|
||||
assert!(!active.exists());
|
||||
assert_eq!(
|
||||
fs::read_to_string(concurrent.path().join("payload"))?,
|
||||
|
||||
Reference in New Issue
Block a user