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
This commit is contained in:
Copilot
2026-03-28 06:34:07 -07:00
parent 2033ec28d7
commit c820280996
4 changed files with 23 additions and 18 deletions
+5 -15
View File
@@ -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}
}
}
+5
View File
@@ -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
+3 -3
View File
@@ -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)
}
@@ -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() {