From f7571a261e509c35672ef87a8936df93ed913128 Mon Sep 17 00:00:00 2001 From: efiten Date: Fri, 5 Jun 2026 00:37:37 +0200 Subject: [PATCH] fix(#1546): remove dead server-side backfill flag (stuck backfilling=true) (#1583) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Closes #1546. `/api/stats` reported `{"backfilling":true,"backfillProgress":0}` on every fully-converged server, and `X-CoreScope-Status: backfilling` was sent on every request. Root cause: the `Store` had three atomic fields — `backfillComplete` / `backfillTotal` / `backfillProcessed` — read by `handleStats` and `backfillStatusMiddleware`, but **nothing ever wrote to them**. They are leftovers from the server-side async backfill added in #612/#614. That work moved to the **ingestor** in #1289 (server is now read-only) and the writer `backfillResolvedPathsAsync` was deleted, orphaning the readers. `backfillComplete.Load()` therefore always returned `false`, so `backfilling := !false` was permanently `true`. This is the leftover of an intentional architecture change, not an unfinished feature — the server no longer does backfill by design, so the correct fix is to delete the dead flag (per triage recommendation; zero consumers). ## Changes - `store.go` — drop the 3 dead atomic fields. - `routes.go` — drop `backfillStatusMiddleware` (+ its registration) and the backfill-progress computation in `handleStats`. - `types.go` — drop `Backfilling` / `BackfillProgress` from `StatsResponse`. **API change:** `/api/stats` no longer emits `backfilling` / `backfillProgress`; the `X-CoreScope-Status` header is removed. Verified no frontend or other consumer reads them. - `resolved_index.go` — remove stale comment referencing the deleted `backfillResolvedPathsAsync`. ## Test Regression assertion added to `TestStatsEndpoint` (#1546): asserts the response no longer carries `backfilling` / `backfillProgress` and that `X-CoreScope-Status` is unset. Verified red→green — against pre-fix code all three assertions fail; with the fix they pass. Full `cmd/server` suite green locally. ## Out of scope If a real server-side backfill/migration status indicator is wanted, that's a new feature on top of the ingestor stats pipe — tracked separately, not by reviving these dead fields. Co-authored-by: Claude Opus 4.8 --- cmd/server/resolved_index.go | 2 -- cmd/server/routes.go | 28 ---------------------------- cmd/server/routes_test.go | 13 +++++++++++++ cmd/server/store.go | 6 ------ cmd/server/types.go | 2 -- 5 files changed, 13 insertions(+), 38 deletions(-) diff --git a/cmd/server/resolved_index.go b/cmd/server/resolved_index.go index 4e942e11..ccc90190 100644 --- a/cmd/server/resolved_index.go +++ b/cmd/server/resolved_index.go @@ -8,8 +8,6 @@ package main // • fetchResolvedPathForObs takes lruMu independently — callers under s.mu // must NOT call it directly; instead collect IDs under s.mu, release, then // do LRU ops under lruMu separately. -// • The backfill path (backfillResolvedPathsAsync) follows this by collecting -// obsIDs to invalidate under s.mu, releasing it, then taking lruMu. import ( "database/sql" diff --git a/cmd/server/routes.go b/cmd/server/routes.go index a7518707..85a8fe5e 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -161,9 +161,6 @@ func (s *Server) RegisterRoutes(r *mux.Router) { // Performance instrumentation middleware r.Use(s.perfMiddleware) - // Backfill status header middleware - r.Use(s.backfillStatusMiddleware) - // /api/* responses must not be cached by upstream CDNs (#1551). // Cloudflare/nginx/Varnish default zone policies cache // application/json for 15min–4h when no Cache-Control is set, @@ -298,17 +295,6 @@ func noStoreAPIMiddleware(next http.Handler) http.Handler { }) } -func (s *Server) backfillStatusMiddleware(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if s.store != nil && s.store.backfillComplete.Load() { - w.Header().Set("X-CoreScope-Status", "ready") - } else { - w.Header().Set("X-CoreScope-Status", "backfilling") - } - next.ServeHTTP(w, r) - }) -} - func (s *Server) perfMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if !strings.HasPrefix(r.URL.Path, "/api/") { @@ -740,18 +726,6 @@ func (s *Server) handleStats(w http.ResponseWriter, r *http.Request) { } counts := s.db.GetRoleCounts() - // Compute backfill progress - backfilling := s.store != nil && !s.store.backfillComplete.Load() - var backfillProgress float64 - if backfilling && s.store != nil && s.store.backfillTotal.Load() > 0 { - backfillProgress = float64(s.store.backfillProcessed.Load()) / float64(s.store.backfillTotal.Load()) - if backfillProgress > 1 { - backfillProgress = 1 - } - } else if !backfilling { - backfillProgress = 1 - } - // Memory accounting (#832). storeDataMB is the in-store packet byte // estimate (the old "trackedMB"); processRSSMB / goHeapInuseMB / goSysMB // give ops the breakdown needed to reason about real RSS. All values @@ -781,8 +755,6 @@ func (s *Server) handleStats(w http.ResponseWriter, r *http.Request) { Companions: counts["companions"], Sensors: counts["sensors"], }, - Backfilling: backfilling, - BackfillProgress: backfillProgress, SignatureDrops: s.db.GetSignatureDropCount(), HashMigrationComplete: s.store != nil && s.store.hashMigrationComplete.Load(), diff --git a/cmd/server/routes_test.go b/cmd/server/routes_test.go index b9a15d5c..27b80a6e 100644 --- a/cmd/server/routes_test.go +++ b/cmd/server/routes_test.go @@ -230,6 +230,19 @@ func TestStatsEndpoint(t *testing.T) { if bt, ok := body["buildTime"]; !ok || bt == nil { t.Error("expected non-nil buildTime field in stats response") } + // Regression (#1546): server-side async backfill was removed in #1289 + // (backfill moved to the ingestor). The dead "backfilling"/"backfillProgress" + // fields had no writer and reported backfilling:true forever. They must not + // reappear in the response, and the X-CoreScope-Status header is gone with them. + if _, ok := body["backfilling"]; ok { + t.Error("stats response must not carry the removed backfilling flag (#1546)") + } + if _, ok := body["backfillProgress"]; ok { + t.Error("stats response must not carry the removed backfillProgress field (#1546)") + } + if got := w.Header().Get("X-CoreScope-Status"); got != "" { + t.Errorf("X-CoreScope-Status header must not be set (#1546), got %q", got) + } } func TestPacketsEndpoint(t *testing.T) { diff --git a/cmd/server/store.go b/cmd/server/store.go index 4ea15e6d..3491b6a4 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -341,12 +341,6 @@ type PacketStore struct { // Clock skew detection engine. clockSkew *ClockSkewEngine - // Async backfill state: set after backfillResolvedPathsAsync completes. - backfillComplete atomic.Bool - // Progress tracking for async backfill (total pending and processed so far). - backfillTotal atomic.Int64 // set once at start of async backfill - backfillProcessed atomic.Int64 - // Bounded cold load: oldest packet timestamp loaded into memory. // Empty string means all data is in memory (no limit applied). oldestLoaded string diff --git a/cmd/server/types.go b/cmd/server/types.go index 4eff2b6b..f457d036 100644 --- a/cmd/server/types.go +++ b/cmd/server/types.go @@ -68,8 +68,6 @@ type StatsResponse struct { Commit string `json:"commit"` BuildTime string `json:"buildTime"` Counts RoleCounts `json:"counts"` - Backfilling bool `json:"backfilling"` - BackfillProgress float64 `json:"backfillProgress"` SignatureDrops int64 `json:"signatureDrops,omitempty"` HashMigrationComplete bool `json:"hashMigrationComplete"`