mirror of
https://github.com/seaweedfs/seaweedfs.git
synced 2026-09-20 13:30:46 +02:00
fix(s3/lifecycle): walker dispatcher nil-response guard + retention-comment
Two PR-review fixes on 9459: 1. WalkerDispatcher.Delete used to panic on a (nil, nil) RPC return — add a defensive nil-response check so the walk halts cleanly instead. Spotted by coderabbit. 2. The retentionWindow=maxTTL comment in runShard claimed PromotedHash "stays empty" in fallback mode, which gemini correctly pointed out is only true once rules are active. During bootstrap (rules compiled but IsActive=false) MaxEffectiveTTL is 0 while PromotedHash counts every non-disabled rule, so promoted becomes non-empty and the next post-activation run hits the recovery branch. That's the intended bootstrap walk — rewrite the comment to explain it rather than misstate the invariant. Test: pins nil-response → error path on WalkerDispatcher.
This commit is contained in:
@@ -172,11 +172,16 @@ func runShard(ctx context.Context, cfg Config, snap *engine.Snapshot, runNow tim
|
||||
// one-time rewind to runNow - maxTTL, self-healing on save.
|
||||
rsh := engine.ReplayContentHash(snap)
|
||||
maxTTL := engine.MaxEffectiveTTL(snap)
|
||||
// Operator-supplied retention falls back to maxTTL, which forces
|
||||
// promoted-empty (no rule's TTL can exceed the max). Once the
|
||||
// walker handles walk-bound rules, the handler will pass the real
|
||||
// meta-log retention here and PromotedHash starts catching
|
||||
// partition flips.
|
||||
// Operator-supplied retention falls back to maxTTL. In steady
|
||||
// state every active replay rule has TTL <= maxTTL by construction,
|
||||
// so promoted is empty and the partition-flip trigger is dormant.
|
||||
// During bootstrap (rules compiled but not yet active) maxTTL is
|
||||
// 0, retentionWindow is 0, and every rule with TTL > 0 lands in
|
||||
// the walk partition; the resulting non-empty promoted forces a
|
||||
// recovery walk on the first run after rules activate, which is
|
||||
// the intended bootstrap behavior. Once the handler plumbs the
|
||||
// real meta-log retention here, PromotedHash starts catching
|
||||
// retention-driven partition flips in addition.
|
||||
retentionWindow := cfg.RetentionWindow
|
||||
if retentionWindow <= 0 {
|
||||
retentionWindow = maxTTL
|
||||
|
||||
@@ -59,6 +59,12 @@ func (d *WalkerDispatcher) Delete(ctx context.Context, action *engine.CompiledAc
|
||||
if err != nil {
|
||||
return fmt.Errorf("walker dispatch %s/%s %s: %w", action.Bucket, objectPath, action.Key.ActionKind, err)
|
||||
}
|
||||
if resp == nil {
|
||||
// A misbehaving server stub returning (nil, nil) would panic on
|
||||
// the switch below. Surface as an error so the walk halts at
|
||||
// this entry, preserving the in-flight cursor's correctness.
|
||||
return fmt.Errorf("walker dispatch %s/%s %s: nil response", action.Bucket, objectPath, action.Key.ActionKind)
|
||||
}
|
||||
switch resp.Outcome {
|
||||
case s3_lifecycle_pb.LifecycleDeleteOutcome_DONE,
|
||||
s3_lifecycle_pb.LifecycleDeleteOutcome_NOOP_RESOLVED,
|
||||
|
||||
@@ -20,6 +20,7 @@ type walkerStubClient struct {
|
||||
outcome s3_lifecycle_pb.LifecycleDeleteOutcome
|
||||
err error
|
||||
reason string
|
||||
nilResp bool // return (nil, nil) — pin the dispatcher's defensive guard
|
||||
}
|
||||
|
||||
func (c *walkerStubClient) LifecycleDelete(_ context.Context, req *s3_lifecycle_pb.LifecycleDeleteRequest) (*s3_lifecycle_pb.LifecycleDeleteResponse, error) {
|
||||
@@ -27,6 +28,9 @@ func (c *walkerStubClient) LifecycleDelete(_ context.Context, req *s3_lifecycle_
|
||||
if c.err != nil {
|
||||
return nil, c.err
|
||||
}
|
||||
if c.nilResp {
|
||||
return nil, nil
|
||||
}
|
||||
return &s3_lifecycle_pb.LifecycleDeleteResponse{Outcome: c.outcome, Reason: c.reason}, nil
|
||||
}
|
||||
|
||||
@@ -135,6 +139,16 @@ func TestWalkerDispatcher_TransportErrorReturnsWrappedError(t *testing.T) {
|
||||
assert.Contains(t, err.Error(), "transport boom")
|
||||
}
|
||||
|
||||
func TestWalkerDispatcher_NilResponseReturnsError(t *testing.T) {
|
||||
// A server returning (nil, nil) would otherwise panic on the
|
||||
// outcome switch.
|
||||
c := &walkerStubClient{nilResp: true}
|
||||
d := &WalkerDispatcher{Client: c}
|
||||
err := d.Delete(context.Background(), sampleAction(t, s3lifecycle.ActionKindExpirationDays), &bootstrap.Entry{Path: "obj"})
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), "nil response")
|
||||
}
|
||||
|
||||
func TestWalkerDispatcher_NilGuardsReturnError(t *testing.T) {
|
||||
d := &WalkerDispatcher{Client: &walkerStubClient{}}
|
||||
require.Error(t, d.Delete(context.Background(), nil, &bootstrap.Entry{Path: "obj"}))
|
||||
|
||||
Reference in New Issue
Block a user