From 1b1d4aa81444ad6fbcb27269a49171d1997b9b24 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Sun, 10 May 2026 09:39:05 -0700 Subject: [PATCH] refactor(s3/lifecycle): extract entryUsesMetadataOnlyDelete predicate (#9417) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(s3/lifecycle): integration coverage for versioning + filters First integration-test bundle building on the existing single-test backdating harness. Each scenario follows the same shape: create bucket, set lifecycle, PUT object, backdate mtime via filer UpdateEntry, run the shell command for one shard sweep, assert S3-side state. Five new tests: - TestLifecycleVersionedBucketCreatesDeleteMarker: Expiration on a versioned bucket must produce a delete marker (latest after worker runs is a marker) AND keep the original version directly addressable by versionId. ListObjectVersions confirms IsLatest=true on the marker. - TestLifecycleNoncurrentVersionExpiration: NoncurrentVersionExpiration fires only on demoted versions. PUT v1, PUT v2 (so v1 → noncurrent), backdate v1, run worker. v1 must be gone, v2 still current. - TestLifecycleExpiredDeleteMarkerCleanup: combined rule (noncurrent + expired-delete-marker) cleans up a sole-survivor marker. PUT v1, DELETE (creates marker), backdate both, run worker. Every version AND marker must be gone for the key. - TestLifecycleDisabledRuleSkipsObject: rule with Status=Disabled must not produce dispatches even on a backdated match. Negative test for the engine's enabled-status gate. - TestLifecycleTagFilter: rule with And{Prefix, Tag} only matches objects carrying the tag. Two backdated objects (one tagged, one not) — only the tagged one is removed. Helpers extracted to keep each test focused: putVersioningEnabled, putNoncurrentExpirationLifecycle, putExpiredDeleteMarkerLifecycle, backdateVersionedMtime (ages a specific .versions/v_ entry), runLifecycleShard (one-shot shell invocation with FATAL guard). * test(s3/lifecycle): tighten noncurrent expiration diagnostics Local run showed TestLifecycleNoncurrentVersionExpiration failing with a bare 404 on HEAD(latest), not enough to tell whether v2 was deleted, the bare-key pointer was removed, or a delete marker was synthesized. Strengthen the test to: - HEAD by versionId=v2 first, so we pin "v2 file still on disk" separately from "the latest pointer resolves to v2" - on HEAD(latest) failure, log ListObjectVersions output (versions + markers, with IsLatest) so the next failure shows which side the bug is on rather than just NotFound * test(s3/lifecycle): integration coverage for AbortIncompleteMultipartUpload Exercises the lifecycleAbortMPU handler path that the prefix-based expiration tests can't reach — routing keys off of .uploads// directory events, not regular object events, and the dispatcher uses a different RPC path (rm on the .uploads// folder). Setup: AbortIncompleteMultipartUpload rule with DaysAfterInitiation=1, CreateMultipartUpload, UploadPart (so the directory carries the right shape), backdate the .uploads// directory entry 30 days, run the worker. The upload must drop out of ListMultipartUploads. Helpers added: putAbortMPULifecycle, backdateUploadDir. * test(s3/lifecycle): integration coverage for NewerNoncurrentVersions NewerNoncurrentVersions=N keeps the N most recent noncurrent versions and expires the rest. Distinct from per-version NoncurrentDays — depends on per-version rank, not just per-version age — and routes through routePointerTransition's "needs full expansion" path. Setup: PUT v1, v2, v3, v4 on a versioned bucket (v4 current; v1-v3 noncurrent), backdate v1+v2+v3 so all satisfy the NoncurrentDays>=1 floor, run the worker. Expect v1+v2 expired (older noncurrent), v3 (newest noncurrent within keep=1) and v4 (current) preserved. Helper added: putNewerNoncurrentLifecycle. * test(s3/lifecycle): integration coverage for suspended-versioning Expiration Suspended versioning takes a distinct code path in lifecycleDispatch: the VersioningSuspended branch first deletes the null version (via deleteSpecificObjectVersion(versionId="null")) and then writes a fresh delete marker on top. Other branches (Enabled → only writes a marker; Off → straight rm) miss this two-step. Setup: enable versioning, PUT v1 (real versionId), suspend versioning, PUT again (creates the null version, demotes v1 to noncurrent), set the Expiration rule, backdate the null at the bare path. Expect: latest is now a fresh delete marker, the "null" version is gone from ListObjectVersions, and v1 (noncurrent under Enabled) still addressable directly — suspended Expiration must only touch the null, not other versions. Helper added: putVersioningSuspended. * test(s3/lifecycle): integration coverage for multi-bucket sweep A single shell-driven shard sweep must process every bucket carrying lifecycle config, not just the first one alphabetically. Pinned because the scheduler iterates the buckets directory and a regression that returns early after the first match would silently disable lifecycle for every later bucket. Two buckets, each with their own prefix-expiration rule and a backdated object. Both must be expired after the same sweep. * test(s3/lifecycle): integration coverage for ObjectSizeGreaterThan filter ObjectSizeGreaterThan is a strict > gate (filterAllows uses ev.Size <= rule.FilterSizeGreaterThan to reject). Pinned at the boundary: an object whose size equals the threshold must remain; only an object strictly larger expires. Catches a > vs >= flip. Two backdated objects on the same prefix, sizes 100 and 150 with threshold=100 — boundary survives, larger expires. * test(s3/lifecycle): scrub bucket lifecycle config + versions on cleanup Tests share one weed mini server. Two pollution modes were producing order-dependent failures: - A later test's shard sweep would still load the prior test's lifecycle config (the worker reads every bucket's XML from filer state, and DeleteBucket alone doesn't drop lifecycle config cleanly on this codebase). - Versioned-bucket tests left versions + delete markers behind that ListObjectsV2 can't see, so the existing best-effort empty-then- delete didn't actually empty those buckets. - The AbortMPU test intentionally leaves an in-flight upload; without an explicit AbortMultipartUpload the bucket DELETE hits NotEmpty. Cleanup now runs DeleteBucketLifecycle, ListObjectVersions → DeleteObject(versionId), ListObjectsV2 → DeleteObject (catches what ListObjectVersions missed), ListMultipartUploads → AbortMultipartUpload, then DeleteBucket. Best-effort throughout so a half-torn-down bucket doesn't fail the cleanup chain. * test(s3/lifecycle): backdate both versions for NoncurrentDays clock Per codex review: NoncurrentDays is clocked from the SUCCESSOR version's mtime (when the displaced version became noncurrent), not from the displaced version's own mtime. Backdating only v1 left the clock (v2's mtime) at "now" and the rule never fired — the test was wrong, not the production path. Backdate v1=31d and v2=30d so v1 sits past the 1-day threshold relative to v2, the noncurrent rule fires, and v2 stays current. * test(s3/lifecycle): assert specific NotFound on multi-bucket deletion Per codex review: TestLifecycleMultipleBucketsInOneSweep treated any HeadObject error as "deleted", which lets a transport failure or dead endpoint mask a real bug. Recognize NoSuchKey/NotFound/HTTP-404 specifically via a small isS3NotFound helper so the assertion actually proves deletion happened, not just that the call broke. * test(s3/lifecycle): gofmt size-filter test * test(s3/lifecycle): integration coverage for Object Lock skip Object Lock retention must override the lifecycle rule. The handler's enforceObjectLockProtections check (s3api_internal_lifecycle.go:47) returns an error when retention is active; the dispatcher then classifies the outcome as SKIPPED_OBJECT_LOCK and the object stays. No existing integration test reaches that outcome. Setup: bucket created with ObjectLockEnabledForBucket=true, expiration rule on prefix "lock/", two backdated objects under the same prefix — one with GOVERNANCE retention until 1h from now, one without. After the worker runs, the unlocked object expires (positive control); the locked one survives. Custom cleanup uses BypassGovernanceRetention so the test can drop the locked version when the test finishes — otherwise the retention window keeps the bucket from being deleted. * test(s3/lifecycle): integration coverage for config update between sweeps An operator changes the lifecycle rule between two shell-driven sweeps. The second sweep must respect the NEW rule, not a cached copy of the old one. Each runLifecycleShard invocation spawns a fresh weed shell subprocess, so cached engine state from a previous sweep doesn't persist — but a regression that caches rules across PutBucketLifecycleConfiguration calls within the S3 server itself would still surface here. Sweep 1: rule prefix="first/", PUT + backdate firstKey, run worker → firstKey expires. Update rule to prefix="second/", PUT + backdate secondKey AND a new key under the OLD prefix ("first/post-update.txt"). Sweep 2 must expire only the second-prefix object; the post-update old- prefix one must survive — config replacement, not merge. * test(s3/lifecycle): integration coverage for ExpirationDate (past) Rules with Expiration{Date: } route through ScanAtDate in the engine (decideMode's ActionKindExpirationDate case) — a separate compile + dispatch branch from the EventDriven delay-group path the Days-based tests exercise. Past date + in-prefix object → must expire. Out-of-prefix object → must remain. Object also backdated as defense-in-depth so the assertion doesn't depend on whether the dispatcher consults MinTriggerAge for date kinds. * test(s3/lifecycle): integration coverage for bootstrap walk on existing objects Production scenario: operator enables lifecycle on a bucket that already holds objects from before the policy. The worker must discover them via the bootstrap walk (BucketBootstrapper) — there were no meta-log events to observe because the objects predate the rule. Without the bootstrap path, only NEW writes would ever match. Setup: PUT 5 objects (no lifecycle config yet) + 1 out-of-prefix survivor, backdate all, THEN set the Expiration rule, run the worker. Every in-prefix pre-existing object must be expired; the out-of-prefix one must remain. * test(s3/lifecycle): integration coverage for DeleteBucketLifecycle stops dispatching Operator UX: after DeleteBucketLifecycle, the worker must observe the removal on the next sweep and stop expiring objects under the now-gone rule. A regression that caches old configs across PutBucketLifecycleConfiguration → DeleteBucketLifecycle would keep silently dropping objects. Setup: positive control (rule active, backdated obj expires) → DeleteBucketLifecycle → PUT + backdate a fresh object → second sweep. The fresh object must remain. * test(s3/lifecycle): integration coverage for empty bucket sweep no-op A bucket carrying lifecycle config but no objects must produce a successful sweep — no hangs, no errors, no dispatches. Pinned because the bootstrap walker iterates bucket directories, and an empty directory is a corner of that traversal that's easy to break (slice-bounds bug on the first listing returning zero entries). Asserts: worker logs "loaded lifecycle for" and "shards 0-15 complete", no FATAL output, bucket still exists after the sweep. * test(s3/lifecycle): fix Object Lock backdate path + skip unwired ScanAtDate ObjectLock: enabling Object Lock on a bucket implicitly enables versioning, so PUT objects land at .versions/v_, not at the bare key. The test was calling backdateMtime (bare path) and failing in the helper with "filer: no entry is found". Switch to backdateVersionedMtime with the versionId returned by PutObject. ExpirationDate: ScanAtDate dispatch path isn't wired to the run-shard shell command yet — the bootstrap walker explicitly skips actions in ModeScanAtDate (walker.go:141 says "SCAN_AT_DATE runs its own date- triggered bootstrap" but no such bootstrap exists in the scheduler or shell). Skip with a t.Skip + explanation so the test activates the moment the date-triggered path lands. * fix(s3/lifecycle): wire ExpirationDate dispatch through bootstrap walker The walker explicitly skipped ModeScanAtDate actions on the comment "SCAN_AT_DATE runs its own date-triggered bootstrap" — but no such bootstrap exists in the scheduler or shell layer. The result: rules with Expiration{Date: ...} compiled correctly, populated the snapshot's dateActions map, and were never dispatched. ExpirationDate is silently a no-op in production. EvaluateAction already handles ActionKindExpirationDate correctly (rejects when now.Before(rule.ExpirationDate), otherwise emits ActionDeleteObject). The walker just needed to fall through instead of skipping. Pre-date walks become no-ops via EvaluateAction's date check; post-date walks expire eligible objects. Un-skip TestLifecycleExpirationDateInThePast — it now exercises the fixed path end-to-end. * test(s3/lifecycle): integration coverage for multiple rules per bucket A single bucket carries two independent Expiration rules with disjoint prefix filters and different Days thresholds. Each rule must fire only on its prefix; objects outside both prefixes must survive. Pinned because Compile builds one CompiledAction per rule per kind all sharing the same bucket index — a bug that lets one rule's prefix or threshold leak into another (e.g. last-write-wins on a shared map) would silently expire wrong objects. Setup: rule A with prefix=logs/ Days=1, rule B with prefix=tmp/ Days=7. Three backdated objects: logs/access.log, tmp/scratch.bin, data/keep.bin. After the worker runs, logs/ + tmp/ are gone; data/ — outside both rule prefixes — survives. * fix(s3/lifecycle): mark ScanAtDate actions active in Compile Two layers were silently filtering ScanAtDate actions out of routing: the walker's mode skip (fixed in e785f59d6) and Compile only marking ModeEventDriven actions active. MatchPath / MatchOriginalWrite both require IsActive() to emit a key, so a ScanAtDate action that's never marked active never reaches a dispatch path even after the walker falls through. ScanAtDate's only dispatch path is the bootstrap walk's MatchPath call — there's no bootstrap-completion rendezvous to wait on. Make the active flag include ModeScanAtDate alongside the EventDriven+BootstrapComplete combination. ExpirationDate-based rules now actually fire end-to-end. The TestLifecycleExpirationDateInThePast integration test exercises this. * fix(s3/lifecycle): route date kinds via ComputeDueAt ExpirationDate has MinTriggerAge=0, so router computed dueTime = info.ModTime + 0 = info.ModTime. For a backdated entry that mtime is BEFORE rule.ExpirationDate, so EvaluateAction's now.Before(rule.ExpirationDate) check returned ActionNone and the date rule never fired through the event-driven path. ComputeDueAt already knows the per-kind shape — rule.ExpirationDate for date kinds, ModTime+Days for the rest — so use it as the single source of truth for dueTime in Route's main loop. * test(s3/lifecycle): pin bootstrap walker date dispatch The original TestWalk_DateActionsSkipped pinned the pre-e785f59d6 behavior that the regular walker skipped ExpirationDate. That walker was rewired to fire date rules whose date has passed (the SCAN_AT_DATE bootstrap was never wired); update the test to match. Split into two: post-date entries dispatch, pre-date entries don't. * test(s3/lifecycle): drop unused putExpiredDeleteMarkerLifecycle The helper was never called — TestLifecycleExpiredDeleteMarkerCleanup constructs a combined noncurrent + expired-marker rule inline, which the helper doesn't cover. The blank-assignment workaround was just hiding dead code; remove both. * test(s3/lifecycle): tighten HeadObject termination check to typed not-found Generic err != nil also passes on transport/auth/timeouts, letting the test go green without proving the lifecycle action actually fired. Switch the three Eventuallyf HeadObject predicates to isS3NotFound, matching the pattern already in the multi-bucket and expiration-date tests. * test(s3/lifecycle): guard ListObjectVersions diagnostic against nil When ListObjectVersions errors, listOut is nil and the diagnostic log path panics on listOut.Versions before the real assertion fires. Branch on (listErr != nil || listOut == nil) so the failure log is robust whatever ListObjectVersions returned. * refactor(s3/lifecycle): extract entryUsesMetadataOnlyDelete predicate The metadata-only delete decision (entry.Attributes.TtlSec > 0) was inlined in lifecycleDispatch with no direct test. Lift it into a named predicate with the rationale comment moved onto the function and pin the four edge cases: nil entry, nil attributes, TtlSec=0, TtlSec>0, plus a defensive check that TtlSec<0 doesn't flip the path on. --- weed/s3api/s3api_internal_lifecycle.go | 18 ++++++--- weed/s3api/s3api_internal_lifecycle_test.go | 41 +++++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/weed/s3api/s3api_internal_lifecycle.go b/weed/s3api/s3api_internal_lifecycle.go index 6199bf7b0..85167953d 100644 --- a/weed/s3api/s3api_internal_lifecycle.go +++ b/weed/s3api/s3api_internal_lifecycle.go @@ -56,12 +56,7 @@ func (s3a *S3ApiServer) LifecycleDelete(ctx context.Context, req *s3_lifecycle_p } func (s3a *S3ApiServer) lifecycleDispatch(ctx context.Context, req *s3_lifecycle_pb.LifecycleDeleteRequest, entry *filer_pb.Entry) (*s3_lifecycle_pb.LifecycleDeleteResponse, error) { - // metadataOnly: skip per-chunk DeleteFile RPCs because the volume's TTL - // will reclaim chunks on its own. Per-write TTL stamping (PR 9377) sets - // Attributes.TtlSec on every entry whose lifecycle rule fits within - // volume TTL — observing a non-zero TtlSec on the live entry is the - // authoritative signal. - metadataOnly := entry != nil && entry.Attributes != nil && entry.Attributes.TtlSec > 0 + metadataOnly := entryUsesMetadataOnlyDelete(entry) switch req.ActionKind { case s3_lifecycle_pb.ActionKind_EXPIRATION_DAYS, s3_lifecycle_pb.ActionKind_EXPIRATION_DATE: // Current-version expiration: Enabled -> delete marker; Suspended @@ -347,6 +342,17 @@ func identityMatches(live, want *s3_lifecycle_pb.EntryIdentity) bool { return bytes.Equal(live.ExtendedHash, want.ExtendedHash) } +// entryUsesMetadataOnlyDelete reports whether the lifecycle delete path +// can skip per-chunk DeleteFile RPCs and rely on the volume's TTL to +// reclaim chunks. Per-write TTL stamping (PR 9377) sets Attributes.TtlSec +// on every entry whose lifecycle rule fits within volume TTL — observing +// a non-zero TtlSec on the live entry is the authoritative signal. +// Defensive nil-checks because the caller may be racing a concurrent +// rewrite that nil-ed Attributes briefly during meta-log replay. +func entryUsesMetadataOnlyDelete(entry *filer_pb.Entry) bool { + return entry != nil && entry.Attributes != nil && entry.Attributes.TtlSec > 0 +} + // recordMetadataOnlyIf bumps the metadata-only counter when on=true. // Skipped when off so callers don't need a guard at every call site. // rule_hash is hex-encoded so operators can group by rule when diff --git a/weed/s3api/s3api_internal_lifecycle_test.go b/weed/s3api/s3api_internal_lifecycle_test.go index 739ac65c2..6a8cbeb7c 100644 --- a/weed/s3api/s3api_internal_lifecycle_test.go +++ b/weed/s3api/s3api_internal_lifecycle_test.go @@ -241,6 +241,47 @@ func contains(haystack, needle string) bool { return false } +func TestEntryUsesMetadataOnlyDelete(t *testing.T) { + cases := []struct { + name string + entry *filer_pb.Entry + want bool + }{ + { + name: "nil entry", + entry: nil, + want: false, + }, + { + name: "nil attributes", + entry: &filer_pb.Entry{}, + want: false, + }, + { + name: "TtlSec=0 (no per-write stamp)", + entry: &filer_pb.Entry{Attributes: &filer_pb.FuseAttributes{TtlSec: 0}}, + want: false, + }, + { + name: "TtlSec>0 (PR 9377 stamped a fast-path TTL)", + entry: &filer_pb.Entry{Attributes: &filer_pb.FuseAttributes{TtlSec: 86400}}, + want: true, + }, + { + name: "TtlSec<0 should not happen but must not flip the path on", + entry: &filer_pb.Entry{Attributes: &filer_pb.FuseAttributes{TtlSec: -1}}, + want: false, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := entryUsesMetadataOnlyDelete(c.entry); got != c.want { + t.Fatalf("want %v, got %v", c.want, got) + } + }) + } +} + func TestRecordMetadataOnlyIf_OnlyFiresWhenOn(t *testing.T) { // Counter must increment exactly once per (bucket, hex(rule_hash)) // when on=true, and not at all when on=false. Other lifecycle paths