fix: restore GC preservation timestamp stability (#22)

* fix: restore GC preservation timestamp stability

- keep stow from overwriting last_gc_mtime_nanos with wall clock time by
  reusing existing metadata timestamps
- clamp GC preservation cutoff to avoid future times, apply five-minute
  buffer, and skip preservation when age threshold is zero or stale
- add unit, integration, and gc tests covering delayed stow and stale
  timestamp scenarios

* fix lints

Author: brndnmtthws

src/commands.rs

@@ -28,6 +28,7 @@
 //! ```
 
 use std::path::{Path, PathBuf};
+use std::time::{Duration, SystemTime, UNIX_EPOCH};
 
 use rayon::prelude::*;
 
@@ -38,7 +39,9 @@ use crate::gc::{self, Gc};
 use crate::hashing::{get_file_size, hash_file};
 use crate::metadata::{clean_metadata, load_metadata, save_metadata};
 use crate::state::{FileState, StateMetadata};
-use crate::timestamp::{generate_monotonic_timestamp, restore_timestamps};
+use crate::timestamp::{
+    generate_monotonic_timestamp, restore_timestamps, saturating_duration_from_nanos,
+};
 
 /// Executes the anchor command - the main orchestrator.
 ///
@@ -265,22 +268,51 @@ pub fn stow(metadata_path: &Path, verbose: u8, quiet: bool, working_dir: &Path)
         }
     }
 
-    // Load existing metadata to get the previous max_mtime_nanos
+    // Load existing metadata so we can capture prior preservation timestamp
     let existing_metadata = match load_metadata(metadata_path) {
         Ok(metadata) => Some(metadata),
         Err(HoldError::DeserializationError { .. }) => None,
         Err(err) => return Err(err),
     };
 
-    if let Some(existing) = existing_metadata {
-        // Preserve the max mtime from the current build as the last_gc_mtime_nanos
-        new_metadata.last_gc_mtime_nanos = existing.max_mtime_nanos();
-        if !quiet
-            && verbose > 0
-            && let Some(mtime) = new_metadata.last_gc_mtime_nanos
-        {
-            eprintln!("Preserving previous build timestamp for GC: {mtime} nanos");
+    // Establish a preservation timestamp for GC without clobbering historical
+    // build information. Prefer the previously recorded value, falling back to
+    // the most recent tracked file mtime. Only use the current clock if we have
+    // no metadata at all (first run).
+    let existing_preservation = existing_metadata.as_ref().and_then(|existing| {
+        existing
+            .last_gc_mtime_nanos
+            .or_else(|| existing.max_mtime_nanos())
+    });
+
+    let new_max_mtime = new_metadata.max_mtime_nanos();
+
+    let preservation_nanos = match (existing_preservation, new_max_mtime) {
+        (Some(existing), Some(new_max)) => existing.max(new_max),
+        (Some(existing), None) => existing,
+        (None, Some(new_max)) => new_max,
+        (None, None) => SystemTime::now()
+            .duration_since(UNIX_EPOCH)
+            .unwrap_or(Duration::ZERO)
+            .as_nanos(),
+    };
+
+    new_metadata.last_gc_mtime_nanos = Some(preservation_nanos);
+
+    if !quiet && verbose > 0 {
+        let (preserved_duration, saturated) = saturating_duration_from_nanos(preservation_nanos);
+        if saturated {
+            eprintln!(
+                "Warning: preservation timestamp ({preservation_nanos}) exceeds representable \
+                 range; clamping to ~year 2554.",
+            );
         }
+        let preserved_time = UNIX_EPOCH + preserved_duration;
+        let elapsed = SystemTime::now()
+            .duration_since(preserved_time)
+            .unwrap_or(Duration::ZERO)
+            .as_secs();
+        eprintln!("Preserving build timestamp for GC: {preservation_nanos} nanos ({elapsed}s ago)",);
     }
 
     // Metadata path should already be absolute from CLI layer
@@ -919,7 +951,9 @@ pub fn execute_with_dir(cli: &Cli, working_dir: Option<&Path>) -> Result<()> {
 #[cfg(test)]
 mod tests {
     use std::fs;
+    use std::time::{Duration, SystemTime, UNIX_EPOCH};
 
+    use filetime::FileTime;
     use tempfile::TempDir;
 
     use super::*;
@@ -1030,4 +1064,34 @@ mod tests {
         let err = stow(&metadata_path, 0, false, temp_dir.path()).unwrap_err();
         assert!(matches!(err, HoldError::ConfigError { .. }));
     }
+
+    #[test]
+    fn test_stow_preserves_last_gc_timestamp_when_time_advances() {
+        let temp_dir = setup_git_repo();
+        let metadata_path = temp_dir.path().join("test.metadata");
+        let tracked_file = temp_dir.path().join("test.txt");
+
+        // Simulate a build finishing an hour ago by backdating the tracked file.
+        let one_hour_ago = SystemTime::now() - Duration::from_secs(3600);
+        filetime::set_file_mtime(&tracked_file, FileTime::from_system_time(one_hour_ago)).unwrap();
+
+        stow(&metadata_path, 0, false, temp_dir.path()).unwrap();
+        let first_metadata = load_metadata(&metadata_path).unwrap();
+        let first_preservation = first_metadata
+            .last_gc_mtime_nanos
+            .expect("stow should set last_gc_mtime_nanos");
+        let expected_nanos = one_hour_ago.duration_since(UNIX_EPOCH).unwrap().as_nanos();
+        assert_eq!(first_preservation, expected_nanos);
+
+        // Allow the wall clock to move forward before running stow again.
+        std::thread::sleep(Duration::from_millis(10));
+
+        stow(&metadata_path, 0, false, temp_dir.path()).unwrap();
+        let second_metadata = load_metadata(&metadata_path).unwrap();
+        let second_preservation = second_metadata
+            .last_gc_mtime_nanos
+            .expect("stow should keep last_gc_mtime_nanos set");
+
+        assert_eq!(second_preservation, expected_nanos);
+    }
 }

src/gc.rs

@@ -43,6 +43,7 @@ use std::time::SystemTime;
 use rayon::prelude::*;
 
 use crate::error::{HoldError, Result};
+use crate::timestamp::saturating_duration_from_nanos;
 
 /// Garbage collection
 #[derive(Debug)]
@@ -1086,35 +1087,69 @@ pub fn select_artifacts_for_removal(
 
     // First, filter out artifacts from the previous build if we have that timestamp
     if let Some(previous_mtime_nanos) = previous_build_mtime_nanos {
-        // Convert to SystemTime for comparison
-        let previous_mtime =
-            SystemTime::UNIX_EPOCH + std::time::Duration::from_nanos(previous_mtime_nanos as u64);
-
-        // Add a small buffer (1 second) to account for timestamp precision/drift
-        let buffer = std::time::Duration::from_secs(1);
-        let cutoff_time = previous_mtime
-            .checked_sub(buffer)
-            .unwrap_or(SystemTime::UNIX_EPOCH);
-
-        let (preserved, eligible): (Vec<_>, Vec<_>) = remaining_artifacts
-            .into_iter()
-            .partition(|artifact| artifact.newest_mtime >= cutoff_time);
-
-        if !quiet && !preserved.is_empty() {
-            let preserved_size: u64 = preserved.iter().map(|a| a.total_size).sum();
+        let (duration, saturated) = saturating_duration_from_nanos(previous_mtime_nanos);
+        if saturated && !quiet {
             eprintln!(
-                "  Preserving {} artifacts ({}) from previous build",
-                preserved.len(),
-                format_size(preserved_size)
+                "Warning: previous_build_mtime_nanos ({previous_mtime_nanos}) exceeds \
+                 representable range; clamping to ~year 2554.",
             );
-            if verbose > 1 {
-                for artifact in &preserved {
-                    eprintln!("    Preserving: {}-{}", artifact.name, artifact.hash);
+        }
+
+        let mut previous_mtime = SystemTime::UNIX_EPOCH + duration;
+        let now = SystemTime::now();
+        if previous_mtime > now {
+            previous_mtime = now;
+        }
+
+        if age_threshold_days == 0 {
+            if !quiet && verbose > 1 {
+                eprintln!("  Skipping previous build preservation because age threshold is 0 days");
+            }
+        } else {
+            let age_threshold =
+                std::time::Duration::from_secs(age_threshold_days as u64 * 24 * 60 * 60);
+            let elapsed_since_previous = now
+                .duration_since(previous_mtime)
+                .unwrap_or(std::time::Duration::ZERO);
+
+            let previous_is_stale = elapsed_since_previous > age_threshold;
+
+            if previous_is_stale {
+                if !quiet && verbose > 0 {
+                    eprintln!(
+                        "  Previous build timestamp is {elapsed_since_previous:?} old; exceeding \
+                         threshold, skipping preservation",
+                    );
+                }
+            } else {
+                // Add a generous buffer to account for clock drift and build finishing before
+                // GC.
+                let buffer = std::time::Duration::from_secs(5 * 60);
+                let cutoff_time = previous_mtime
+                    .checked_sub(buffer)
+                    .unwrap_or(SystemTime::UNIX_EPOCH);
+
+                let (preserved, eligible): (Vec<_>, Vec<_>) = remaining_artifacts
+                    .into_iter()
+                    .partition(|artifact| artifact.newest_mtime >= cutoff_time);
+
+                if !quiet && !preserved.is_empty() {
+                    let preserved_size: u64 = preserved.iter().map(|a| a.total_size).sum();
+                    eprintln!(
+                        "  Preserving {} artifacts ({}) from previous build",
+                        preserved.len(),
+                        format_size(preserved_size)
+                    );
+                    if verbose > 1 {
+                        for artifact in &preserved {
+                            eprintln!("    Preserving: {}-{}", artifact.name, artifact.hash);
+                        }
+                    }
                 }
+
+                remaining_artifacts = eligible;
             }
         }
-
-        remaining_artifacts = eligible;
     }
 
     // Step 1: Apply size-based cleanup if needed
@@ -1295,6 +1330,8 @@ fn calculate_directory_size(path: &Path) -> Result<u64> {
 
 #[cfg(test)]
 mod tests {
+    use std::time::{Duration, SystemTime};
+
     use super::*;
 
     #[test]
@@ -1438,4 +1475,89 @@ mod tests {
                 .any(|a| a.name == "very-old-crate")
         );
     }
+
+    #[test]
+    fn test_select_artifacts_skips_stale_previous_timestamp() {
+        let now = SystemTime::now();
+        let ten_days_ago = now - Duration::from_secs(10 * 24 * 60 * 60);
+        let two_days_ago = now - Duration::from_secs(2 * 24 * 60 * 60);
+        let stale_previous = now - Duration::from_secs(30 * 24 * 60 * 60);
+
+        let stale_nanos = stale_previous
+            .duration_since(SystemTime::UNIX_EPOCH)
+            .unwrap()
+            .as_nanos();
+
+        let artifacts = vec![
+            CrateArtifact {
+                name: "old-crate".to_string(),
+                hash: "aaaaaaaaaaaaaaaa".to_string(),
+                artifacts: vec![],
+                total_size: 2 * 1024 * 1024,
+                newest_mtime: ten_days_ago,
+            },
+            CrateArtifact {
+                name: "recent-crate".to_string(),
+                hash: "bbbbbbbbbbbbbbbb".to_string(),
+                artifacts: vec![],
+                total_size: 2 * 1024 * 1024,
+                newest_mtime: two_days_ago,
+            },
+        ];
+
+        let to_remove = select_artifacts_for_removal(
+            &artifacts,
+            4 * 1024 * 1024,
+            None,
+            7,
+            Some(stale_nanos),
+            0,
+            false,
+        );
+
+        assert_eq!(to_remove.len(), 1);
+        assert_eq!(to_remove[0].name, "old-crate");
+    }
+
+    #[test]
+    fn test_select_artifacts_preserves_recent_previous_timestamp_with_buffer() {
+        let now = SystemTime::now();
+        let two_minutes_ago = now - Duration::from_secs(2 * 60);
+        let eight_days_ago = now - Duration::from_secs(8 * 24 * 60 * 60);
+
+        let previous_build_nanos = now
+            .duration_since(SystemTime::UNIX_EPOCH)
+            .unwrap()
+            .as_nanos();
+
+        let artifacts = vec![
+            CrateArtifact {
+                name: "recent-build".to_string(),
+                hash: "cccccccccccccccc".to_string(),
+                artifacts: vec![],
+                total_size: 3 * 1024 * 1024,
+                newest_mtime: two_minutes_ago,
+            },
+            CrateArtifact {
+                name: "older-build".to_string(),
+                hash: "dddddddddddddddd".to_string(),
+                artifacts: vec![],
+                total_size: 3 * 1024 * 1024,
+                newest_mtime: eight_days_ago,
+            },
+        ];
+
+        let to_remove = select_artifacts_for_removal(
+            &artifacts,
+            6 * 1024 * 1024,
+            Some(1024 * 1024),
+            7,
+            Some(previous_build_nanos),
+            0,
+            false,
+        );
+
+        assert_eq!(to_remove.len(), 1);
+        assert_eq!(to_remove[0].name, "older-build");
+    }
 }

src/timestamp.rs

@@ -3,12 +3,38 @@ use std::fs::OpenOptions;
 use std::path::Path;
 use std::time::{Duration, SystemTime, UNIX_EPOCH};
 
+const NANOS_PER_SECOND: u128 = 1_000_000_000;
+
+/// Compute a duration from nanoseconds with saturation at [`Duration::MAX`].
+///
+/// Returns the saturated duration along with a flag indicating whether the
+/// input exceeded the representable range.
+pub fn saturating_duration_from_nanos(nanos: u128) -> (Duration, bool) {
+    let seconds = nanos / NANOS_PER_SECOND;
+    if seconds > u64::MAX as u128 {
+        return (Duration::MAX, true);
+    }
+
+    let nanos_remainder = (nanos % NANOS_PER_SECOND) as u32;
+    (Duration::new(seconds as u64, nanos_remainder), false)
+}
+
+/// Compute a [`SystemTime`] from nanoseconds with saturation at
+/// `UNIX_EPOCH + Duration::MAX`.
+///
+/// Returns the saturated timestamp along with a flag indicating whether the
+/// input exceeded the representable range.
+pub fn saturating_system_time_from_nanos(nanos: u128) -> (SystemTime, bool) {
+    let (duration, saturated) = saturating_duration_from_nanos(nanos);
+    (UNIX_EPOCH + duration, saturated)
+}
+
 use crate::error::{HoldError, Result};
 use crate::state::{FileState, StateMetadata};
 
 /// Convert nanoseconds since UNIX_EPOCH to SystemTime
 fn nanos_to_system_time(nanos: u128) -> SystemTime {
-    UNIX_EPOCH + Duration::from_nanos(nanos as u64)
+    saturating_system_time_from_nanos(nanos).0
 }
 
 /// Convert SystemTime to nanoseconds since UNIX_EPOCH

tests/gc_algorithm_tests.rs

@@ -337,7 +337,7 @@ fn test_combined_selection_preserves_previous_build_artifacts() {
 
 #[test]
 fn test_combined_selection_timestamp_buffer_edge_case() {
-    // Test the 1-second buffer for timestamp comparison
+    // Test the preservation buffer for timestamp comparison
 
     let now = SystemTime::now();
     let base_time = now.checked_sub(Duration::from_secs(3600)).unwrap();
@@ -354,7 +354,7 @@ fn test_combined_selection_timestamp_buffer_edge_case() {
     artifacts[0].newest_mtime = base_time; // Exactly at cutoff
     artifacts[1].newest_mtime = base_time.checked_sub(Duration::from_millis(500)).unwrap(); // 500ms before
     artifacts[2].newest_mtime = base_time.checked_add(Duration::from_millis(500)).unwrap(); // 500ms after
-    artifacts[3].newest_mtime = base_time.checked_sub(Duration::from_secs(2)).unwrap(); // 2s before
+    artifacts[3].newest_mtime = base_time.checked_sub(Duration::from_secs(6 * 60)).unwrap(); // 6 minutes before
 
     let previous_build_nanos = base_time
         .duration_since(SystemTime::UNIX_EPOCH)
@@ -371,8 +371,8 @@ fn test_combined_selection_timestamp_buffer_edge_case() {
         false,
     );
 
-    // With 1-second buffer, artifacts at or near the cutoff should be preserved
-    // Only well_before_cutoff should be selected
+    // With the 5-minute buffer, artifacts near the cutoff should be preserved; only
+    // those older than the buffer should be selected.
     assert_eq!(selected.len(), 1);
     assert_eq!(selected[0].name, "well_before_cutoff");
 }