amadan
console: retry clearing a disabled feed's status until it lands (Astra code review R6)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+55
−7
internal/console/geoipfeed.go+28 −2internal/console/geoipfeed_test.go+26 −0internal/console/server.go+1 −5
CI passed — run details
diff --git a/internal/console/geoipfeed.go b/internal/console/geoipfeed.go| index 8ca4ea41f..9adfb682e 100644 |
| --- a/internal/console/geoipfeed.go |
| +++ b/internal/console/geoipfeed.go |
| @@ -362,8 +362,34 @@ func (f *geoipFeeder) run(ctx context.Context) { |
| // clearFeedStatus removes feed.json when no feed is configured, so a feed |
| // switched off never alerts anyone. |
| -func clearFeedStatus(ctx context.Context, s release.Store) { |
| +func clearFeedStatus(ctx context.Context, s release.Store) error { |
| if err := s.Delete(ctx, geoip.FeedKey); err != nil && !errors.Is(err, os.ErrNotExist) { |
| - log.Printf("console: geoip feed status could not be cleared") |
| + return err |
| + } |
| + return nil |
| +} |
| + |
| +// retryClearFeedStatus keeps trying until feed.json is gone or ctx ends: |
| +// a failed delete must not leave a disabled feed's alert firing for good |
| +// (code review R6-1). Each attempt is bounded; the wait doubles from |
| +// first to max. |
| +func retryClearFeedStatus(ctx context.Context, s release.Store, first, max time.Duration) { |
| + wait := first |
| + for { |
| + actx, cancel := context.WithTimeout(ctx, 30*time.Second) |
| + err := clearFeedStatus(actx, s) |
| + cancel() |
| + if err == nil { |
| + return |
| + } |
| + log.Printf("console: geoip feed status could not be cleared; retrying in %s", wait) |
| + select { |
| + case <-ctx.Done(): |
| + return |
| + case <-time.After(wait): |
| + } |
| + if wait *= 2; wait > max { |
| + wait = max |
| + } |
| } |
| } |
diff --git a/internal/console/geoipfeed_test.go b/internal/console/geoipfeed_test.go| index a5fec3f31..bca90b89b 100644 |
| --- a/internal/console/geoipfeed_test.go |
| +++ b/internal/console/geoipfeed_test.go |
| @@ -337,3 +337,29 @@ func TestFeedNeverOverwritesAnUnreadableHistory(t *testing.T) { |
| t.Fatalf("history rewritten after an unreadable read: %+v", st) |
| } |
| } |
| + |
| +type failingDeleteStore struct { |
| + release.Store |
| + fails atomic.Int32 |
| +} |
| + |
| +func (s *failingDeleteStore) Delete(ctx context.Context, key string) error { |
| + if s.fails.Add(-1) >= 0 { |
| + return errors.New("transient") |
| + } |
| + return s.Store.Delete(ctx, key) |
| +} |
| + |
| +// Code review R6-1: a failed clear is retried until it lands. |
| +func TestRetryClearFeedStatusRecoversWithoutARestart(t *testing.T) { |
| + base := release.NewDirStore(t.TempDir()) |
| + base.Put(context.Background(), geoip.FeedKey, []byte(`{"v":1}`), "") |
| + s := &failingDeleteStore{Store: base} |
| + s.fails.Store(2) |
| + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) |
| + defer cancel() |
| + retryClearFeedStatus(ctx, s, time.Millisecond, 4*time.Millisecond) |
| + if _, _, err := base.Get(context.Background(), geoip.FeedKey); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("feed.json survived the retries: %v", err) |
| + } |
| +} |
diff --git a/internal/console/server.go b/internal/console/server.go| index da7d6a3b2..3be6f94ed 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -1442,11 +1442,7 @@ func (s *Server) StartGeoipJobs(ctx context.Context) { |
| } else { |
| // Off the startup path, and bounded: a stalled bucket must not |
| // hold the console's start (code review R4-1). |
| - go func() { |
| - cctx, cancel := context.WithTimeout(ctx, 30*time.Second) |
| - defer cancel() |
| - clearFeedStatus(cctx, s.cfg.Releases) |
| - }() |
| + go retryClearFeedStatus(ctx, s.cfg.Releases, 30*time.Second, 10*time.Minute) |
| } |
| go func() { |
| for { |