[botdeniska]

Contribution Licensing (no CLA, inbound=outbound, DCO required)

This pull request contributes to PGSTY SILO (pgsty/silo). Code contributions
are accepted under AGPL-3.0-or-later, the same license as the server.
This project does not use a CLA or require a separate Apache-2.0 license grant.
By submitting this pull request I represent that I have the right to contribute
the code changes under this repository's
GNU Affero General Public License v3.0 or later
and retain copyright in my original work. Existing copyright and license
notices remain intact; separately licensed material keeps its applicable terms.
Every commit must carry a DCO Signed-off-by trailer
(git commit -s) certifying the
Developer Certificate of Origin — see
CONTRIBUTING.md.

Description

Fixes #252.

expireTransitionedObject (cmd/bucket-lifecycle.go:610) deletes the remote tier object of every transitioned version it expires, before it deletes the version:

	opts := ObjectOptions{
		Versioned:  globalBucketVersioningSys.PrefixEnabled(oi.Bucket, oi.Name),
		Expiration: ExpirationOptions{Expire: true},
	}
	...
	// Delete remote object from warm-tier
	err := deleteObjectFromRemoteTier(ctx, oi.TransitionedObject.Name, oi.TransitionedObject.VersionID, oi.TransitionedObject.Tier)
	if err == nil {
		// Skip adding free version since we successfully deleted the
		// remote object
		opts.SkipFreeVersion = true
	...
	// Now, delete object from hot-tier namespace
	if _, err := objectAPI.DeleteObject(ctx, oi.Bucket, oi.Name, opts); err != nil {

For DeleteAction, the Expiration of the current version, opts.VersionID is empty. In a versioned bucket DeleteObject then only adds a delete marker, and the expired version stays as a noncurrent version whose tier object is gone: it is listed, and GET ?versionId= returns 404 NoSuchKey. opts.VersionSuspended is never set either, so with versioning suspended DeleteObject tries to delete the null version. For a current version with a UUID and no null version below it, that fails with VersionNotFound after the tier object is already deleted, and the scanner repeats it every cycle. A null current version is removed without the null delete marker.

With this change:

	opts.VersionSuspended = globalBucketVersioningSys.PrefixSuspended(oi.Bucket, oi.Name)
	keepRemote := opts.VersionID == "" &&
		(opts.Versioned || (opts.VersionSuspended && oi.VersionID != "" && oi.VersionID != nullVersionID))
	if !keepRemote {
		// the remote delete and SkipFreeVersion, as before
	}

• VersionSuspended is set as in applyExpiryOnNonTransitionedObjects (cmd/data-scanner.go:1236), so a transitioned version expires into the same delete marker as a version that is not transitioned.
• When the expiry only adds a delete marker on top of the version, the tier object is kept. That is the case with an empty version ID and versioning enabled, or suspended with a non-null current version. SkipFreeVersion stays false there. When the version itself is deleted later, by NoncurrentVersionExpiration or DELETE ?versionId=, DeleteVersion adds a free version and the tier object is removed through it. This is the rule the user DELETE path already follows in objSweeper.shouldRemoveRemoteObject (cmd/tier-sweeper.go:108-121).
• Every other case deletes the tier object as before: an expiry with a version ID, an unversioned bucket, and a null current version with versioning suspended.

bucket	current version	main	this change
versioning enabled	any	delete marker; tier object deleted, the version is listed but returns 404	delete marker; tier object kept, the version reads back
versioning suspended	UUID	tier object deleted on every scanner cycle, VersionNotFound, no delete marker, the version returns 404	null delete marker; tier object kept, the version reads back
versioning suspended	null	version and tier object deleted, no delete marker	null delete marker replaces the version; tier object deleted
unversioned	null	object and tier object deleted	unchanged

Expiry of a restored copy returns before this code and is unchanged.

Motivation and Context

This is silent data loss. The scanner expires current versions on its own, and a GET, HEAD or listing of an expired object queues the same expiry, so it needs no failure, decommission or rebalance. Pool decommission and rebalance move transitioned versions correctly; the versions they move are lost later by this expiry like any other. The expired version stays listed with the tier's storage class, so the loss shows only when someone reads that version.

ExpiredObjectAllVersions on a transitioned version also goes through expireTransitionedObject with an empty version ID and only adds a delete marker in a versioned bucket. That is a separate problem, described in the issue and not changed here. With this change, those versions at least keep their data.

How to test this PR?

go test ./cmd -count=1 -v -run '^TestExpireTransitionedObjectKeepsNoncurrentData$'
go test ./cmd -count=1 -run 'Lifecycle|Expir|Transition|Tier'

TestExpireTransitionedObjectKeepsNoncurrentData (cmd/bucket-lifecycle_test.go) runs on the real two-pool fixture (migrationTestPools). An in-memory WarmBackend is put into globalTierConfigMgr.drivercache. Transitioned versions are written with DecomTieredObject, as version metadata with a completed transition, so reads go to the tier, and their data is put into the fake tier. The test evaluates an Expiration rule with evalActionFromLifecycle and calls expireTransitionedObject as the expiry worker does. It then checks the versions left in the namespace, whether the remote object of the expired version was removed, and that a kept version reads back. Subtests:

• versioned: v1, v2 transitioned; after the expiry [DM v2 v1], v2's remote object kept, v2 reads back.
• suspended: v1, v2 with UUIDs, versioning suspended; [DM v2 v1], v2 kept and readable.
• suspended-null: v2 is the null version; [DM v1], v2's remote object removed.
• unversioned: the object and its remote object are removed.

On main (trimmed):

--- FAIL: TestExpireTransitionedObjectKeepsNoncurrentData/versioned
    remote object of the expired version removed: true, want false
    v2 reads "<Object not found: tier/tier-expiry-versioned/v2>" after the expiry, want "obj v2"
--- FAIL: TestExpireTransitionedObjectKeepsNoncurrentData/suspended
    expiry: Version not found: tier-expiry-suspended/obj()
    versions after expiry: got [v2 v1], want [DM v2 v1]
    remote object of the expired version removed: true, want false
    v2 reads "<Object not found: tier/tier-expiry-suspended/v2>" after the expiry, want "obj v2"
--- FAIL: TestExpireTransitionedObjectKeepsNoncurrentData/suspended-null
    versions after expiry: got [v1], want [DM v1]
--- PASS: TestExpireTransitionedObjectKeepsNoncurrentData/unversioned

With the change all four pass. unversioned is a guard and passes on both.

Manual reproduction. The script from the issue: a hot server with 4 drives and a second SILO server as a minio tier (mc admin tier add minio), a Transition rule with Days=0, MINIO_SCANNER_SPEED=fastest, then an Expiration rule. A wider run on the same setup also covers the rules that must keep deleting tier data. Results on main b91e1c6f2 and on this branch:

bucket and rule	main	this change
versioned, 3 transitioned versions, Expiration	delete marker; v3 listed WARM, GET ?versionId= 404 NoSuchKey, tier object gone	delete marker; v1–v3 return 200, tier objects kept
same, expiry queued by a HEAD, scanner idle	v3 404, tier object gone	v3 200; a later DELETE ?versionId=v3 removes its tier object through the free version
versioning suspended, current version with a UUID, Expiration	no delete marker, v1 404, tier DELETE every scanner cycle (8 in 2 min)	null delete marker, v1 200
versioning suspended, null current version, Expiration	version removed, no marker, tier object deleted	null delete marker, tier object deleted
unversioned, Expiration	object and tier object deleted	same
versioned, NoncurrentVersionExpiration (NewerNoncurrentVersions=1; NoncurrentDays=1)	v1 and its tier object deleted, v2 and v3 200	same
versioned, not transitioned (control), Expiration	delete marker, v1–v3 200	same
versioned, ExpiredObjectAllVersions (separate problem)	delete marker only, v3 404	delete marker only, v1–v3 200

After every run with this change, each tier object is referenced by a version, and no version points to a missing tier object.

Overlap with #240. #240 (open) also edits expireTransitionedObject. It adds opts.SetEvalMetadataFn(...) just before the // Delete remote object from warm-tier comment and replaces the DeleteObject call. This change keeps that comment line and the DeleteObject call as they are, so the two do not touch the same lines. I applied the #240 diff to main and cherry-picked this commit on top, and also did it the other way round. Both orders apply without conflicts and give the same cmd/bucket-lifecycle.go. On the combined tree, go vet ./cmd, this test and #240's TestReplicateILMExpiryDeleteMarker and TestReplicateILMExpiryNoncurrentPurge pass. The closure from #240 reads opts when DeleteObject runs, after VersionSuspended is set, and checkReplicateDelete only looks at Versioned, so replication decisions are the same as with #240 alone. If a later revision of #240 does conflict here, the resolution is to keep its SetEvalMetadataFn block before the comment and this block after it.

Run locally on Go 1.27.3 (darwin/arm64), branch based on main b91e1c6f2:

• the make verifiers steps: golangci-lint v2.15.0 with .golangci.yml (0 issues), make check-gen rebrand-guard; go vet ./cmd; make build
• go test ./cmd -count=1 -timeout 40m: ok
• go test ./cmd -count=1 -run 'Lifecycle|Expir|Transition|Tier': ok

Compatibility impact

No API, configuration, storage metadata, metric, header, route or dependency changes.

• Tier space. A transitioned version that an Expiration turns noncurrent keeps its tier object until the version is deleted. This is what a DELETE without versionId already does. Buckets that rely on Expiration alone, without a noncurrent rule, keep those versions and their tier data, as they already keep noncurrent versions that are not transitioned.
• Versioning suspended. The expiry now writes the null delete marker, as it does for versions that are not transitioned. A null current version is replaced by the marker instead of being removed without one. A current version with a UUID gets the marker on top and stays as a noncurrent version.
• Existing damage. Versions whose tier objects were already deleted stay unreadable. This change only stops new losses.
• Rollback restores the old behavior. No data migration is involved.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Optimization (provides speedup with no functional changes)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• All commits are signed off (git commit -s) per the DCO
• Fixes a regression (If yes, please add commit-id or PR # here) (no: the unconditional remote delete predates the fork)
• Unit tests added/updated
• make verifiers passes
• Relevant package tests and make build pass
• Compatibility and rollback impact documented
• Internal documentation updated (not needed)
• Public documentation update opened in pgsty/silo.pgsty.com, if needed (not needed)