test(s3/lifecycle): bundle dispatcher + engine edge-case coverage (#9413)

* test(s3/lifecycle): bundle dispatcher + engine edge-case coverage

Two-package bundle covering uncovered branches in production code that
the existing happy-path tests don't reach. Dispatcher 58.1% → 60.2%
and engine 81.0% → 81.7% (engine lift modest because most branches
were already hit; the nil-rule defensive case is otherwise unreachable
from a Compile flow).

dispatcher (4 tests):
- FilerPersister.Load with nil Store errors with a "nil Store"
  message rather than panicking at the Read call.
- FilerPersister.Save with nil Store same.
- FilerPersister.Load with a non-NotFound transport error wraps the
  shard ID into the message AND keeps the underlying error
  recoverable via errors.Is.
- FilerPersister.Load with successful empty []byte returns an empty
  map, not a JSON-decode error — pinning that an existing-but-empty
  cursor file is treated as "no entries".
- Tick initializes the retries map on first call without panic so a
  freshly-constructed Dispatcher works.
- Tick with already-canceled ctx re-queues the popped Match, returns
  zero, and never invokes the LifecycleDelete client — the Match
  must not be lost across worker restart.

engine (4 tests):
- rulePredicateSensitive(nil) returns false rather than panicking on
  the FilterTags dereference. The non-nil paths run through Compile,
  but a defensive nil-rule arrival isn't reachable that way.
- rule with no FilterTags / empty FilterTags map returns false (the
  check is len(FilterTags) > 0, so empty must classify as
  non-sensitive — pinning catches a flipped >= comparison).
- rule with a populated FilterTags returns true.

* fix(s3/lifecycle): Tick must requeue every drained Match on shutdown

Per codex review on #9413: Tick called Schedule.Drain to pop ALL due
matches at once, then iterated. If ctx canceled mid-loop, only the
current Match was re-added — everything past that index was silently
lost across the worker restart. With N due matches, up to N-1 were
dropped.

Fix: on cancellation, re-add due[i:] (current + remaining) before
returning. Matches already dispatched (due[:i]) stay processed; the
schedule is left exactly as it would be if Drain had returned only
the dispatched prefix.

Strengthen the existing test to enqueue three due matches and assert
sched.Len()==3 after a pre-canceled Tick. Pre-fix the test would have
seen Len()==1 because only the first popped Match was re-added.

weed/s3api/s3lifecycle/dispatcher/dispatcher.go

@@ -98,10 +98,16 @@ func (d *Dispatcher) Tick(ctx context.Context, now time.Time) int {
 		d.retries = map[retryKey]int{}
 	}
 	due := d.Schedule.Drain(now)
-	for _, m := range due {
+	for i, m := range due {
 		if err := ctx.Err(); err != nil {
-			// Re-queue and return; the caller is shutting down.
-			d.Schedule.Add(m)
+			// Caller is shutting down. Re-queue every remaining drained
+			// Match (current and any not yet visited) so they're not
+			// lost across the worker restart — the schedule already
+			// popped them all out, so the dispatcher owns putting them
+			// back.
+			for _, rem := range due[i:] {
+				d.Schedule.Add(rem)
+			}
 			return 0
 		}
 		d.dispatchOne(ctx, m, now)

weed/s3api/s3lifecycle/dispatcher/edge_cases_test.go

@@ -0,0 +1,134 @@
+package dispatcher
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+
+	"github.com/seaweedfs/seaweedfs/weed/pb/s3_lifecycle_pb"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3lifecycle"
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3lifecycle/router"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// errFilerStore satisfies FilerStore with a forced Read error so the
+// Load path's "non-NotFound transport error" branch is reachable.
+// Save mirrors the pattern; tests for the nil-Store branch don't need
+// a stub at all.
+type errFilerStore struct {
+	readErr  error
+	saveErr  error
+	readData []byte
+}
+
+func (e *errFilerStore) Read(_ context.Context, _, _ string) ([]byte, error) {
+	if e.readErr != nil {
+		return nil, e.readErr
+	}
+	return e.readData, nil
+}
+func (e *errFilerStore) Save(_ context.Context, _, _ string, _ []byte) error {
+	return e.saveErr
+}
+
+// ---------- FilerPersister edges ----------
+
+func TestFilerPersisterLoad_NilStoreErrors(t *testing.T) {
+	// The persister's Store dependency is required; loading with a nil
+	// Store must error rather than panic on the Read call.
+	p := &FilerPersister{}
+	_, err := p.Load(context.Background(), 0)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "nil Store")
+}
+
+func TestFilerPersisterSave_NilStoreErrors(t *testing.T) {
+	p := &FilerPersister{}
+	err := p.Save(context.Background(), 0, map[s3lifecycle.ActionKey]int64{})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "nil Store")
+}
+
+func TestFilerPersisterLoad_NonNotFoundErrorIsWrapped(t *testing.T) {
+	// A transport-level error (anything that isn't ErrNotFound) wraps
+	// with the shard ID context so operators can attribute it. Pin
+	// both that the error surfaces and that the original is recoverable
+	// via errors.Is so a caller can distinguish.
+	want := errors.New("filer-side bang")
+	p := &FilerPersister{Store: &errFilerStore{readErr: want}}
+	_, err := p.Load(context.Background(), 3)
+	require.Error(t, err)
+	assert.ErrorIs(t, err, want)
+	assert.Contains(t, err.Error(), "shard=3")
+}
+
+func TestFilerPersisterLoad_EmptyContentReturnsEmptyMap(t *testing.T) {
+	// Read returning a successful empty []byte means "file exists but
+	// is zero length"; the persister must treat this as "no entries"
+	// rather than try to JSON-decode an empty slice and return an
+	// error.
+	p := &FilerPersister{Store: &errFilerStore{readData: []byte{}}}
+	got, err := p.Load(context.Background(), 0)
+	require.NoError(t, err)
+	assert.NotNil(t, got)
+	assert.Empty(t, got)
+}
+
+// ---------- Tick edges ----------
+
+func TestTick_InitializesRetriesMapOnFirstCall(t *testing.T) {
+	// retries is a lazy map; a Dispatcher constructed without it must
+	// not panic on the first Tick. Pin that handleRetryLater can write
+	// into the map afterwards (proven by the dispatcher returning an
+	// integer rather than crashing).
+	d, sched := newDispatcher(&fakeClient{
+		respond: func(int) (*s3_lifecycle_pb.LifecycleDeleteResponse, error) {
+			return &s3_lifecycle_pb.LifecycleDeleteResponse{Outcome: s3_lifecycle_pb.LifecycleDeleteOutcome_DONE}, nil
+		},
+	})
+	require.Nil(t, d.retries, "preconditions: retries map starts nil")
+	t0 := time.Now()
+	sched.Add(router.Match{
+		Key:       s3lifecycle.ActionKey{Bucket: "bk", ActionKind: s3lifecycle.ActionKindExpirationDays},
+		ObjectKey: "k",
+		DueTime:   t0,
+	})
+	processed := d.Tick(context.Background(), t0.Add(time.Hour))
+	assert.Equal(t, 1, processed)
+	assert.NotNil(t, d.retries, "Tick must initialize the retries map")
+}
+
+func TestTick_CtxShutdownMidLoopRequeuesAndReturnsZero(t *testing.T) {
+	// If ctx is canceled before Tick starts dispatching, the entire
+	// drained batch must be re-queued. Drain pops ALL due matches at
+	// once, so a naive "re-add the current Match only" would silently
+	// lose every Match past the cancellation point. Three matches
+	// here exercises that the loop re-queues the current AND every
+	// remaining drained entry.
+	calls := 0
+	client := &fakeClient{
+		respond: func(int) (*s3_lifecycle_pb.LifecycleDeleteResponse, error) {
+			calls++
+			return &s3_lifecycle_pb.LifecycleDeleteResponse{Outcome: s3_lifecycle_pb.LifecycleDeleteOutcome_DONE}, nil
+		},
+	}
+	d, sched := newDispatcher(client)
+	t0 := time.Now()
+	for _, k := range []string{"k1", "k2", "k3"} {
+		sched.Add(router.Match{
+			Key:       s3lifecycle.ActionKey{Bucket: "bk", ActionKind: s3lifecycle.ActionKindExpirationDays},
+			ObjectKey: k,
+			DueTime:   t0,
+		})
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel() // shutdown before Tick even starts dispatching
+
+	processed := d.Tick(ctx, t0.Add(time.Hour))
+	assert.Equal(t, 0, processed, "shutdown must report zero processed")
+	assert.Equal(t, 0, calls, "shutdown must skip every dispatch call")
+	assert.Equal(t, 3, sched.Len(), "every drained Match must be re-queued — none lost")
+}

weed/s3api/s3lifecycle/engine/compile_helpers_test.go

@@ -0,0 +1,45 @@
+package engine
+
+import (
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/s3api/s3lifecycle"
+	"github.com/stretchr/testify/assert"
+)
+
+// rulePredicateSensitive's non-nil branches are exercised by the
+// existing Compile tests; this test pins the defensive nil-rule
+// branch directly. Compile shouldn't ever pass a nil rule, but a
+// future caller that does shouldn't panic.
+
+func TestRulePredicateSensitive_NilRuleReturnsFalse(t *testing.T) {
+	assert.False(t, rulePredicateSensitive(nil))
+}
+
+func TestRulePredicateSensitive_NoFilterTagsReturnsFalse(t *testing.T) {
+	// A rule without FilterTags is not predicate-sensitive; the
+	// router's MatchPredicateChange path skips actions whose rule
+	// returns false here.
+	rule := &s3lifecycle.Rule{ID: "r", Status: s3lifecycle.StatusEnabled, ExpirationDays: 7}
+	assert.False(t, rulePredicateSensitive(rule))
+}
+
+func TestRulePredicateSensitive_EmptyFilterTagsReturnsFalse(t *testing.T) {
+	// An empty (non-nil) FilterTags map must classify as non-sensitive
+	// — the function checks len, so empty == 0 == false.
+	rule := &s3lifecycle.Rule{
+		ID:         "r",
+		Status:     s3lifecycle.StatusEnabled,
+		FilterTags: map[string]string{},
+	}
+	assert.False(t, rulePredicateSensitive(rule))
+}
+
+func TestRulePredicateSensitive_PopulatedFilterTagsReturnsTrue(t *testing.T) {
+	rule := &s3lifecycle.Rule{
+		ID:         "r",
+		Status:     s3lifecycle.StatusEnabled,
+		FilterTags: map[string]string{"env": "prod"},
+	}
+	assert.True(t, rulePredicateSensitive(rule))
+}