amadan
console: storage pass fixes — real store-record errors, truncation guard
Review findings on the storage sweep (Task 9): - storagePass no longer swallows objstore.Get's error. Absent is (nil, "", nil) by that function's own idiom, so a non-nil error is a real failure (transient bucket read, malformed store.json) — previously treated the same as "no store", writing StoreBytes:0/StoreUnswept:false into the create-only day object, forever indistinguishable from an app that never had one. Now logs and marks StoreUnswept, same as a failing sizer, and keeps sweeping the rest of the apps. - sizeProvisionedStore now breaks its ListObjectsV2 pagination loop if a page claims IsTruncated but returns a nil NextContinuationToken, rather than refetching page one forever. The pass runs synchronously inside StartEconomicsPass, so an unguarded loop against a misbehaving S3-compatible endpoint would hang census/statements/bill-pull permanently, not just the storage sweep. Added TestStoragePassDegradesOnStoreRecordReadError (malformed store.json via a direct Put, asserts StoreUnswept). The truncation guard has no test seam of its own — sizeProvisionedStore builds its S3 client internally via objstore.S3ClientFor and is only reachable through the real client, not the storeSizer test seam — so it's covered by the guard + comment only, per review's own fallback.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 files changed,
+56
−1
internal/console/economicspass.go+19 −1internal/console/economicspass_test.go+37 −0
diff --git a/internal/console/economicspass.go b/internal/console/economicspass.go| index fe09f3f0d..f6be211fc 100644 |
| --- a/internal/console/economicspass.go |
| +++ b/internal/console/economicspass.go |
| @@ -149,7 +149,17 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| for _, ks := range sizes { |
| st.BucketBytes += ks.Size |
| } |
| - if rec, _, err := objstore.Get(ctx, s.cfg.Releases, acct, app); err == nil && rec != nil && rec.Provisioned != nil { |
| + rec, _, gerr := objstore.Get(ctx, s.cfg.Releases, acct, app) |
| + if gerr != nil { |
| + // Absent is (nil, "", nil) — objstore.Get's own idiom — so a |
| + // non-nil error here is a REAL failure (transient bucket |
| + // read, malformed store.json), not "no store". Treating it |
| + // as "no store" would write StoreBytes:0/StoreUnswept:false |
| + // into a create-only object, indistinguishable from an app |
| + // that never had a store and never correctable afterward. |
| + log.Printf("console: storage pass: store record %s/%s: %v", acct, app, gerr) |
| + st.StoreUnswept = true |
| + } else if rec != nil && rec.Provisioned != nil { |
| n, serr := s.sizeStore(ctx, rec.Provisioned) |
| if serr != nil { |
| log.Printf("console: storage pass: store %s/%s: %v", acct, app, serr) |
| @@ -198,6 +208,14 @@ func sizeProvisionedStore(ctx context.Context, p *objstore.Provisioned) (int64, |
| if !aws.ToBool(out.IsTruncated) { |
| break |
| } |
| + if out.NextContinuationToken == nil { |
| + // A misbehaving S3-compatible endpoint that claims IsTruncated |
| + // but omits the continuation token would otherwise refetch page |
| + // one forever: this pass runs synchronously inside |
| + // StartEconomicsPass, so that hang takes census/statements/ |
| + // bill-pull down with it, permanently. |
| + break |
| + } |
| token = out.NextContinuationToken |
| } |
| return total, nil |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 9d18c3b9d..c89e34cf7 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -219,3 +219,40 @@ func TestStoragePassMarksUnreachableStoreUnswept(t *testing.T) { |
| t.Fatalf("app.StoreBytes = %d, want 0 when unswept", app.StoreBytes) |
| } |
| } |
| + |
| +// TestStoragePassDegradesOnStoreRecordReadError: objstore.Get's "absent" |
| +// return is (nil, "", nil) — a non-nil error means a REAL failure (bucket |
| +// read, malformed store.json), not "no store". A pass that treated the two |
| +// alike would write StoreBytes:0/StoreUnswept:false into a create-only |
| +// object, forever indistinguishable from an app that never had a store. |
| +func TestStoragePassDegradesOnStoreRecordReadError(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + |
| + // Corrupt the record directly so objstore.Get's json.Unmarshal fails — |
| + // the fixture bucket makes this easier than injecting a wrapped store |
| + // whose Get errors, and it exercises the exact malformed-store.json case |
| + // the finding names. |
| + if _, err := fx.rel.Put(ctx, objstore.Key(fixtureAcct, fixtureApp), []byte("{not valid json"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if err := fx.srv.storagePass(ctx, "2026-08-11"); err != nil { |
| + t.Fatal(err) |
| + } |
| + raw, _, err := fx.rel.Get(ctx, usage.StorageDayKey("2026-08-11")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var day usage.StorageDay |
| + if err := json.Unmarshal(raw, &day); err != nil { |
| + t.Fatal(err) |
| + } |
| + app := day.Apps[fixtureAcct+"/"+fixtureApp] |
| + if !app.StoreUnswept { |
| + t.Fatalf("app with an unreadable store record = %+v, want StoreUnswept", app) |
| + } |
| + if app.StoreBytes != 0 { |
| + t.Fatalf("app.StoreBytes = %d, want 0 when unswept", app.StoreBytes) |
| + } |
| +} |