From 75c78768d2c04da11326adb2354ff32083fe60e8 Mon Sep 17 00:00:00 2001 From: Dmitriy Kovalenko Date: Tue, 20 Oct 2026 22:08:09 +0000 Subject: [PATCH] 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 --- .../core-plugins/src/marketplace_staging.rs | 34 ++++++++++++--- .../src/marketplace_staging_tests.rs | 41 +++++++++++++++---- 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/codex-rs/core-plugins/src/marketplace_staging.rs b/codex-rs/core-plugins/src/marketplace_staging.rs index 8a80a29ab8..4c64264ba0 100644 --- a/codex-rs/core-plugins/src/marketplace_staging.rs +++ b/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> { } } -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 { +fn is_abandoned_staging_dir( + entry: &fs::DirEntry, + prefix: &str, + now: SystemTime, +) -> io::Result { 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= max_age); } let owner_lock = entry.path().join(OWNER_LOCK); let Some(metadata) = symlink_metadata_if_exists(&owner_lock)? else { diff --git a/codex-rs/core-plugins/src/marketplace_staging_tests.rs b/codex-rs/core-plugins/src/marketplace_staging_tests.rs index 84fbd5fdf8..c5662f7fa3 100644 --- a/codex-rs/core-plugins/src/marketplace_staging_tests.rs +++ b/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(()) }