Collect s3 test objects left behind by runs that died

A per-run key space stops concurrent runs from deleting each other's objects,
but it also means nothing ever revisits the keys of a run that was killed
before its cleanup, so those leak forever. The fixed prefix this replaced at
least swept itself on the next run.

The first test to construct an s3 store now sweeps the shared test root once
per binary, deleting anything older than two hours: a test takes seconds and a
CI job minutes, so nothing that old can belong to a live run. Failures to list
or delete are logged, not fatal, since this is hygiene and not the test.

The age filter is a pure function with its own tests, including that an object
whose modification time could not be read is left alone rather than treated as
infinitely old.

This is a backstop. A lifecycle rule on the bucket (expire testpkg-* after a
day) is the real fix, since it also covers a bucket nobody runs tests against
for a while and cannot be skipped by a crash.

attachment/store_s3_test.go

@@ -15,15 +15,91 @@ import (
 	"heckel.io/ntfy/v2/util"
 )
 
-// testRunID separates concurrent runs that share the test bucket
-var testRunID = util.RandomString(10)
+const (
+	// testRoot is the shared root under which every test's key space lives, so that one listing
+	// can find what previous runs left behind
+	testRoot = "testpkg-attachment"
+
+	// staleTestObjectAge is how old a test object has to be before it is assumed to belong to a
+	// run that died: a test takes seconds, a CI job minutes, so nothing live is this old
+	staleTestObjectAge = 2 * time.Hour
+)
+
+var (
+	// testRunID separates concurrent runs that share the test bucket
+	testRunID = util.RandomString(10)
+
+	// sweepOnce runs the stale-object sweep once per test binary
+	sweepOnce sync.Once
+)
 
 // testKeyPrefix returns a key space belonging to this test alone. The bucket is shared with
 // every other CI run, and deleteAllObjects wipes everything under the prefix it is given, so
 // tests sharing one prefix delete each other's objects mid-run (seen as NoSuchKey 404s on an
 // unrelated pull request).
 func testKeyPrefix(t *testing.T, configuredPrefix string) string {
-	return path.Join(configuredPrefix, "testpkg-attachment", testRunID, t.Name())
+	return path.Join(configuredPrefix, testRoot, testRunID, t.Name())
+}
+
+// sweepStaleTestObjects deletes objects left behind by runs that were killed before their
+// cleanup ran. Per-run key spaces keep concurrent runs from deleting each other's objects, but
+// they also mean nothing ever revisits a dead run's keys, so something has to collect them.
+// Anything under the shared test root older than staleTestObjectAge cannot belong to a live run.
+//
+// This is a backstop for a bucket that has no expiration rule. Prefer a lifecycle rule on the
+// bucket (expire the test root after a day): that also covers the case where nobody runs these
+// tests for a while, and it cannot be skipped by a crash.
+func sweepStaleTestObjects(t *testing.T, config *s3.Config) {
+	t.Helper()
+	rootConfig := *config // The sweep looks at every run's keys, not just this test's
+	rootConfig.Prefix = path.Join(config.Prefix, testRoot)
+	client := s3.New(&rootConfig)
+	objects, err := client.ListObjectsV2(context.Background())
+	if err != nil {
+		t.Logf("cannot list stale test objects, skipping sweep: %s", err) // Hygiene, not the test
+		return
+	}
+	stale := staleKeys(objects, time.Now())
+	if len(stale) == 0 {
+		return
+	}
+	t.Logf("deleting %d object(s) left behind by earlier runs", len(stale))
+	if err := client.DeleteObjects(context.Background(), stale); err != nil {
+		t.Logf("cannot delete stale test objects: %s", err)
+	}
+}
+
+// staleKeys returns the keys of objects old enough to belong to a run that died. An object whose
+// modification time could not be read is left alone: a zero time would otherwise look infinitely
+// old and take every live run's objects with it.
+func staleKeys(objects []*s3.Object, now time.Time) []string {
+	stale := make([]string, 0)
+	for _, obj := range objects {
+		if !obj.LastModified.IsZero() && now.Sub(obj.LastModified) > staleTestObjectAge {
+			stale = append(stale, obj.Key)
+		}
+	}
+	return stale
+}
+
+func TestStaleKeys_OnlyCollectsWhatNoLiveRunCouldOwn(t *testing.T) {
+	// The sweep runs while other CI jobs are mid-test, so the age cutoff is the only thing
+	// keeping it from deleting a live run's objects
+	now := time.Now()
+	objects := []*s3.Object{
+		{Key: "live/just-written", LastModified: now},
+		{Key: "live/minutes-old", LastModified: now.Add(-10 * time.Minute)},
+		{Key: "dead/hours-old", LastModified: now.Add(-3 * time.Hour)},
+		{Key: "dead/days-old", LastModified: now.Add(-72 * time.Hour)},
+	}
+	require.Equal(t, []string{"dead/hours-old", "dead/days-old"}, staleKeys(objects, now))
+}
+
+func TestStaleKeys_IgnoresObjectsWithoutAModTime(t *testing.T) {
+	// LastModified is parsed from the listing and left zero if it could not be read; deleting on
+	// a zero time would delete everything
+	objects := []*s3.Object{{Key: "unknown-age", LastModified: time.Time{}}}
+	require.Empty(t, staleKeys(objects, time.Now()))
 }
 
 func TestS3Store_WriteWithPrefix(t *testing.T) {
@@ -33,7 +109,8 @@ func TestS3Store_WriteWithPrefix(t *testing.T) {
 	}
 	cfg, err := s3.ParseURL(s3URL)
 	require.Nil(t, err)
-	cfg.Prefix = testKeyPrefix(t, "test-prefix")
+	sweepOnce.Do(func() { sweepStaleTestObjects(t, cfg) })
+	cfg.Prefix = testKeyPrefix(t, cfg.Prefix) // The key space under test IS a prefix
 	client := s3.New(cfg)
 	deleteAllObjects(t, client)
 	backend := newS3Backend(client)
@@ -66,6 +143,7 @@ func newTestRealS3Store(t *testing.T, totalSizeLimit int64) (*Store, *modTimeOve
 	}
 	cfg, err := s3.ParseURL(s3URL)
 	require.Nil(t, err)
+	sweepOnce.Do(func() { sweepStaleTestObjects(t, cfg) })
 	cfg.Prefix = testKeyPrefix(t, cfg.Prefix)
 	client := s3.New(cfg)
 	inner := newS3Backend(client)