From c8202809962b1aadad9b0a49e6e24c5817d60366 Mon Sep 17 00:00:00 2001
From: Copilot <copilot@github.com>
Date: Sat, 28 Mar 2026 06:34:07 -0700
Subject: [PATCH] s3lifecycle: address review feedback

- Fix version_time.go overflow: guard timestampPart > MaxInt64 before
  the inversion subtraction to prevent uint64 wrap
- Make all expiry checks inclusive (!now.Before instead of now.After)
  so actions trigger at the exact scheduled instant
- Add NoncurrentIndex to ObjectInfo so Evaluate() can properly handle
  NewerNoncurrentVersions via ShouldExpireNoncurrentVersion()
- Add test for high-bit overflow version ID
---
 weed/s3api/s3lifecycle/evaluator.go         | 20 +++++---------------
 weed/s3api/s3lifecycle/rule.go              |  5 +++++
 weed/s3api/s3lifecycle/version_time.go      |  6 +++---
 weed/s3api/s3lifecycle/version_time_test.go | 10 ++++++++++
 4 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/weed/s3api/s3lifecycle/evaluator.go b/weed/s3api/s3lifecycle/evaluator.go
index 368e1e4c1..afb64e194 100644
--- a/weed/s3api/s3lifecycle/evaluator.go
+++ b/weed/s3api/s3lifecycle/evaluator.go
@@ -30,17 +30,7 @@ func Evaluate(rules []Rule, obj ObjectInfo, now time.Time) EvalResult {
 	// Phase 2: NoncurrentVersionExpiration
 	if !obj.IsLatest && !obj.SuccessorModTime.IsZero() {
 		for _, rule := range rules {
-			if rule.Status != "Enabled" {
-				continue
-			}
-			if rule.NoncurrentVersionExpirationDays <= 0 {
-				continue
-			}
-			if !matchesFilter(rule, obj) {
-				continue
-			}
-			expiryTime := expectedExpiryTime(obj.SuccessorModTime, rule.NoncurrentVersionExpirationDays)
-			if now.After(expiryTime) {
+			if ShouldExpireNoncurrentVersion(rule, obj, obj.NoncurrentIndex, now) {
 				return EvalResult{Action: ActionDeleteVersion, RuleID: rule.ID}
 			}
 		}
@@ -56,13 +46,13 @@ func Evaluate(rules []Rule, obj ObjectInfo, now time.Time) EvalResult {
 				continue
 			}
 			// Date-based expiration
-			if !rule.ExpirationDate.IsZero() && now.After(rule.ExpirationDate) {
+			if !rule.ExpirationDate.IsZero() && !now.Before(rule.ExpirationDate) {
 				return EvalResult{Action: ActionDeleteObject, RuleID: rule.ID}
 			}
 			// Days-based expiration
 			if rule.ExpirationDays > 0 {
 				expiryTime := expectedExpiryTime(obj.ModTime, rule.ExpirationDays)
-				if now.After(expiryTime) {
+				if !now.Before(expiryTime) {
 					return EvalResult{Action: ActionDeleteObject, RuleID: rule.ID}
 				}
 			}
@@ -89,7 +79,7 @@ func ShouldExpireNoncurrentVersion(rule Rule, obj ObjectInfo, noncurrentIndex in
 
 	// Check age threshold.
 	expiryTime := expectedExpiryTime(obj.SuccessorModTime, rule.NoncurrentVersionExpirationDays)
-	if !now.After(expiryTime) {
+	if now.Before(expiryTime) {
 		return false
 	}
 
@@ -115,7 +105,7 @@ func EvaluateMPUAbort(rules []Rule, uploadKey string, createdAt time.Time, now t
 			continue
 		}
 		cutoff := createdAt.Add(time.Duration(rule.AbortMPUDaysAfterInitiation) * 24 * time.Hour)
-		if now.After(cutoff) {
+		if !now.Before(cutoff) {
 			return EvalResult{Action: ActionAbortMultipartUpload, RuleID: rule.ID}
 		}
 	}
diff --git a/weed/s3api/s3lifecycle/rule.go b/weed/s3api/s3lifecycle/rule.go
index b5d118f66..4900e5c02 100644
--- a/weed/s3api/s3lifecycle/rule.go
+++ b/weed/s3api/s3lifecycle/rule.go
@@ -60,6 +60,11 @@ type ObjectInfo struct {
 	// ID timestamp. Zero value for the latest version.
 	SuccessorModTime time.Time
 
+	// NoncurrentIndex is the 0-based position among non-current versions
+	// sorted newest-first (0 = newest non-current version). Used by
+	// NewerNoncurrentVersions evaluation. -1 or unset for current versions.
+	NoncurrentIndex int
+
 	// Tags are the object's user-defined tags, extracted from the entry's
 	// Extended metadata (keys prefixed with "X-Amz-Tagging-").
 	Tags map[string]string
diff --git a/weed/s3api/s3lifecycle/version_time.go b/weed/s3api/s3lifecycle/version_time.go
index 4e60924b0..d4f4c5f94 100644
--- a/weed/s3api/s3lifecycle/version_time.go
+++ b/weed/s3api/s3lifecycle/version_time.go
@@ -31,12 +31,12 @@ func getVersionTimestampNanos(versionId string) int64 {
 	if err != nil {
 		return 0
 	}
+	if timestampPart > math.MaxInt64 {
+		return 0
+	}
 	if timestampPart > versionIdFormatThreshold {
 		// New format: inverted timestamp, convert back.
 		return int64(math.MaxInt64 - timestampPart)
 	}
-	if timestampPart > math.MaxInt64 {
-		return 0
-	}
 	return int64(timestampPart)
 }
diff --git a/weed/s3api/s3lifecycle/version_time_test.go b/weed/s3api/s3lifecycle/version_time_test.go
index 8cfbf1108..460cbec58 100644
--- a/weed/s3api/s3lifecycle/version_time_test.go
+++ b/weed/s3api/s3lifecycle/version_time_test.go
@@ -55,6 +55,16 @@ func TestGetVersionTimestamp(t *testing.T) {
 		}
 	})
 
+	t.Run("high_bit_overflow_returns_zero", func(t *testing.T) {
+		// Version ID with first 16 hex chars > math.MaxInt64 should return zero,
+		// not a wrapped negative timestamp.
+		versionId := "80000000000000000000000000000000"
+		got := GetVersionTimestamp(versionId)
+		if !got.IsZero() {
+			t.Errorf("expected zero time for overflow version ID, got %v", got)
+		}
+	})
+
 	t.Run("invalid_hex", func(t *testing.T) {
 		got := GetVersionTimestamp("zzzzzzzzzzzzzzzz0000000000000000")
 		if !got.IsZero() {