Reclaim aged legacy marketplace staging directories (#55690)

## Why

Legacy marketplace checkouts have no owner lock and were skipped by staging cleanup. Age thresholds allow cleanup to reclaim them while leaving recent clones from older binaries alone.

## What changed

Remove legacy `marketplace-add-` directories after 24 hours and `marketplace-upgrade-` directories after 10 minutes, based on directory modification time. Keep owner locks authoritative for current staging directories.

## Testing

Extend cleanup tests to cover both legacy age thresholds, preservation of recent legacy clones and live locked checkouts, and skipping legacy symlinks.

GitOrigin-RevId: 77b55bfe50ab29f5ceae46a80276b2c9e361107f

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

@@ -2,6 +2,7 @@
 //! A coordination lock protects publication of each checkout's lifetime lock. Checkouts
 //! live below their owner directory so Git and activation cannot replace the lock file.
 //! Cleanup claims a limited batch under coordination, then deletes it outside that lock.
+//! Legacy directories use age thresholds because older binaries publish no owner lock.
 
 use codex_utils_path::symlink_metadata_if_exists;
 use std::fs;
@@ -9,9 +10,16 @@ use std::fs::File;
 use std::io;
 use std::path::Path;
 use std::path::PathBuf;
+use std::time::Duration;
+use std::time::SystemTime;
 use tempfile::TempDir;
 use tracing::warn;
 
+const LEGACY_ADD_MAX_AGE: Duration = Duration::from_secs(24 * 60 * 60);
+// Upgrades have a 30-second timeout per Git command; leave ample room for all phases.
+const LEGACY_UPGRADE_MAX_AGE: Duration = Duration::from_secs(10 * 60);
+const LEGACY_ADD_PREFIX: &str = "marketplace-add-";
+const LEGACY_UPGRADE_PREFIX: &str = "marketplace-upgrade-";
 const MAX_CLEANUP_DIRECTORIES: usize = 4;
 const COORDINATION_LOCK: &str = ".staging.lock";
 const CLEANUP_LOCK: &str = ".staging-cleanup.lock";
@@ -62,7 +70,7 @@ impl MarketplaceStagingDir {
 }
 
 pub(crate) fn cleanup_marketplace_staging(install_root: &Path) {
-    if let Err(err) = remove_stale_staging_dirs(install_root) {
+    if let Err(err) = remove_stale_staging_dirs(install_root, SystemTime::now()) {
         warn!("failed to clean up marketplace staging directories: {err}");
     }
 }
@@ -84,7 +92,7 @@ fn try_lock(lock: File) -> io::Result<Option<File>> {
     }
 }
 
-fn remove_stale_staging_dirs(install_root: &Path) -> io::Result<()> {
+fn remove_stale_staging_dirs(install_root: &Path, now: SystemTime) -> io::Result<()> {
     let staging_root = MarketplaceStagingDir::staging_root(install_root);
     let Some(metadata) = symlink_metadata_if_exists(&staging_root)? else {
         return Ok(());
@@ -105,7 +113,7 @@ fn remove_stale_staging_dirs(install_root: &Path) -> io::Result<()> {
         let entry = entry?;
         let path = entry.path();
         // Continue past an unreadable entry so one failure cannot prevent reclamation.
-        match is_abandoned_staging_dir(&entry, &prefix) {
+        match is_abandoned_staging_dir(&entry, &prefix, now) {
             Ok(true) => abandoned.push(path),
             Ok(false) => {}
             Err(err) if err.kind() == io::ErrorKind::NotFound => {}
@@ -130,7 +138,11 @@ fn remove_stale_staging_dirs(install_root: &Path) -> io::Result<()> {
     Ok(())
 }
 
-fn is_abandoned_staging_dir(entry: &fs::DirEntry, prefix: &str) -> io::Result<bool> {
+fn is_abandoned_staging_dir(
+    entry: &fs::DirEntry,
+    prefix: &str,
+    now: SystemTime,
+) -> io::Result<bool> {
     if !entry.file_type()?.is_dir() {
         return Ok(false);
     }
@@ -139,7 +151,19 @@ fn is_abandoned_staging_dir(entry: &fs::DirEntry, prefix: &str) -> io::Result<bo
         return Ok(false);
     };
     if !name.starts_with(prefix) {
-        return Ok(false);
+        // Older binaries did not publish ownership. Leave recent directories
+        // alone so an upgrade can coexist with their in-flight clones.
+        let max_age = if name.starts_with(LEGACY_ADD_PREFIX) {
+            LEGACY_ADD_MAX_AGE
+        } else if name.starts_with(LEGACY_UPGRADE_PREFIX) {
+            LEGACY_UPGRADE_MAX_AGE
+        } else {
+            return Ok(false);
+        };
+        return Ok(now
+            .duration_since(entry.metadata()?.modified()?)
+            .unwrap_or_default()
+            >= max_age);
     }
     let owner_lock = entry.path().join(OWNER_LOCK);
     let Some(metadata) = symlink_metadata_if_exists(&owner_lock)? else {

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

@@ -1,4 +1,4 @@
-//! Exercises staging ownership across processes and reclamation of abandoned checkouts.
+//! Exercises staging ownership across processes and age-based recovery of legacy checkouts.
 
 use super::*;
 use anyhow::Result;
@@ -46,8 +46,8 @@ fn cleanup_preserves_live_process_and_reclaims_its_abandoned_checkout() -> Resul
     let active = fs::read_dir(&staging_root)?.next().unwrap()?.path();
     let payload = active.join("checkout/payload");
 
-    // Cleanup must preserve a checkout owned by another process.
-    remove_stale_staging_dirs(home.path())?;
+    // Age must never override the lifetime lock, even across processes.
+    remove_stale_staging_dirs(home.path(), SystemTime::now() + LEGACY_ADD_MAX_AGE)?;
     assert_eq!(fs::read_to_string(&payload)?, "in-flight clone");
     let concurrent = MarketplaceStagingDir::new(home.path())?;
     fs::create_dir_all(concurrent.path())?;
@@ -68,24 +68,48 @@ fn cleanup_preserves_live_process_and_reclaims_its_abandoned_checkout() -> Resul
 }
 
 #[test]
-fn cleanup_reclaims_unpublished_and_preserves_unrelated_entries() -> Result<()> {
+fn cleanup_reclaims_unpublished_and_aged_legacy_clones() -> Result<()> {
     let home = tempfile::tempdir()?;
     let staging_root = home.path().join(".staging");
+    let legacy_add = staging_root.join("marketplace-add-old");
+    let legacy_upgrade = staging_root.join("marketplace-upgrade-old");
     let unrelated = staging_root.join("unrelated");
     let foreign = staging_root.join("marketplace-other-os-active");
     let unpublished = staging_root.join(format!(
         "{}unpublished",
         MarketplaceStagingDir::directory_prefix()
     ));
-    for path in [&unrelated, &foreign, &unpublished] {
+    for path in [
+        &legacy_add,
+        &legacy_upgrade,
+        &unrelated,
+        &foreign,
+        &unpublished,
+    ] {
         fs::create_dir_all(path)?;
     }
+    fs::write(legacy_add.join("payload"), "old add")?;
+    fs::write(legacy_upgrade.join("payload"), "old upgrade")?;
     let unrelated_file =
         staging_root.join(format!("{}file", MarketplaceStagingDir::directory_prefix()));
     fs::write(&unrelated_file, "keep")?;
 
-    remove_stale_staging_dirs(home.path())?;
+    let now = SystemTime::now();
+    remove_stale_staging_dirs(home.path(), now)?;
     assert!(!unpublished.exists());
+    assert!(legacy_add.exists());
+    assert!(legacy_upgrade.exists());
+    remove_stale_staging_dirs(
+        home.path(),
+        now + LEGACY_UPGRADE_MAX_AGE + Duration::from_secs(1),
+    )?;
+    assert!(!legacy_upgrade.exists());
+    assert!(legacy_add.exists());
+    remove_stale_staging_dirs(
+        home.path(),
+        now + LEGACY_ADD_MAX_AGE + Duration::from_secs(1),
+    )?;
+    assert!(!legacy_add.exists());
     assert!(unrelated.exists());
     assert!(foreign.exists());
     assert_eq!(fs::read_to_string(unrelated_file)?, "keep");
@@ -105,6 +129,8 @@ fn cleanup_does_not_follow_symlinks() -> Result<()> {
         MarketplaceStagingDir::directory_prefix()
     ));
     std::os::unix::fs::symlink(outside.path(), &alias)?;
+    let legacy_alias = staging_root.join("marketplace-upgrade-link");
+    std::os::unix::fs::symlink(outside.path(), &legacy_alias)?;
     let invalid_owner =
         staging_root.join(format!("{}link", MarketplaceStagingDir::directory_prefix()));
     fs::create_dir_all(&invalid_owner)?;
@@ -113,9 +139,10 @@ fn cleanup_does_not_follow_symlinks() -> Result<()> {
         invalid_owner.join(OWNER_LOCK),
     )?;
 
-    remove_stale_staging_dirs(home.path())?;
+    remove_stale_staging_dirs(home.path(), SystemTime::now() + LEGACY_ADD_MAX_AGE)?;
     assert_eq!(fs::read_to_string(outside.path().join("payload"))?, "keep");
     assert!(alias.is_symlink());
+    assert!(legacy_alias.is_symlink());
     assert!(invalid_owner.exists());
     Ok(())
 }