amadan
provision: a snapshot pass decides removals from the bucket's own walk (#336)
Final review findings 2 and 4. The per-row confirmations (record key, region and pool only) are gone. A Reconcile or ReconcileSidecars pass over a record view snapshot that reaches a removal candidate runs the record walk again over the live bucket (scanRecords / scanSidecars over freshStore) and decides every removal that pass from it, by the walk's own rules: invalid shapes and empty channels protected before scope, hosts matched across all keys, full sidecar placement (record, live sidecars manifest, inbound), the live trash marker. A walk that cannot read cleanly keeps every row. Passes with no candidate read nothing more; non-snapshot stores walk once as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5 files changed,
+399
−122
docs/superpowers/specs/2026-10-21-s3-change-index-design.md+17 −13internal/provision/reconcile.go+86 −35internal/provision/recordview_store.go+14 −62internal/provision/recordview_test.go+216 −0internal/provision/sidecar_reconcile.go+66 −12
diff --git a/docs/superpowers/specs/2026-10-21-s3-change-index-design.md b/docs/superpowers/specs/2026-10-21-s3-change-index-design.md| index 78430f753..7246dd8db 100644 |
| --- a/docs/superpowers/specs/2026-10-21-s3-change-index-design.md |
| +++ b/docs/superpowers/specs/2026-10-21-s3-change-index-design.md |
| @@ -295,19 +295,23 @@ those writers (box run/seed/move writes, direct-bucket CLI writes) rely on |
| the 5-minute floor; with the index they converge in seconds, and only |
| failures and old binaries wait for the backstop. |
| -**Removals are confirmed in the bucket.** The same writers mean absence in |
| -a snapshot is not the bucket's word: a record written without its bump is |
| -missing from a Complete snapshot until the next rebuild. So a removal that |
| -a snapshot proves by absence is confirmed against the live bucket before a |
| -row goes. That covers "record gone" in `Reconcile` and in |
| -`ReconcileSidecars`, and the trash-marker removal in `Reconcile`, which |
| -checks that the marker the snapshot holds is still in the bucket |
| -(`confirmRecordGone`, `confirmSidecarGone` and `confirmTrashed` in |
| -`internal/provision/recordview_store.go`). A record that is there and in |
| -the pass's scope keeps the row, and so does a live read that fails: the |
| -row stays until a pass can read the bucket cleanly. Over any store that is |
| -not a snapshot the checks answer at once without a read, because there the |
| -walk's absence already is the bucket's. |
| +**Removals are decided by the bucket's walk.** The same writers mean |
| +absence in a snapshot is not the bucket's word: a record written without |
| +its bump is missing from a Complete snapshot until the next rebuild. So a |
| +`Reconcile` or `ReconcileSidecars` pass over a snapshot that reaches a |
| +removal candidate proved by absence (record gone, a sidecar gone or no |
| +longer placeable, the trash marker) runs the record walk once more over the |
| +live bucket (`scanRecords`, `scanSidecars` over `freshStore`) and decides |
| +every removal in that pass from what that walk read, by the walk's own |
| +rules: invalid shapes and empty channels protected before scope, hosts |
| +matched across every key (a host re-homed to another key is present), full |
| +sidecar eligibility (region, pool, placement from the record and the live |
| +sidecars manifest, against this box's inbound), and the live trash marker. |
| +A walk that cannot read cleanly keeps every row that pass, as the walk |
| +always has. A snapshot pass with no removal candidate reads nothing more. |
| +Over any store that is not a snapshot the pass's own walk already is the |
| +bucket's, and nothing is read twice (final review R1, findings 2 and 4, |
| +amadan #336). |
| ### 4.5 What triggers a pass, and what it reads |
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index a2f08183c..cd21e7e54 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -819,16 +819,37 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| if cfg.Now == nil { |
| cfg.Now = time.Now |
| } |
| + sc, err := scanRecords(ctx, s, cfg) |
| + if err != nil { |
| + return err |
| + } |
| + return reconcileScanned(ctx, s, reg, cfg, sc) |
| +} |
| +// recordScan is the record-collection phase of a Reconcile pass: the |
| +// in-scope records by host, the hosts whose record is there but cannot be |
| +// used (protect), and whether anything failed to read (readFailed, which |
| +// disables removal for the whole pass). |
| +type recordScan struct { |
| + recs map[string]Record |
| + protect map[string]bool |
| + readFailed bool |
| +} |
| + |
| +// scanRecords is Reconcile's record walk over s. A pass over a record view |
| +// snapshot that has a removal to decide runs it again over the bucket |
| +// (freshStore), so both judge every record by exactly these rules (S3 |
| +// change-index design §4.4). |
| +func scanRecords(ctx context.Context, s release.Store, cfg ReconcileConfig) (recordScan, error) { |
| known, readFailed, err := knownAccounts(ctx, s) |
| if err != nil { |
| - return err |
| + return recordScan{}, err |
| } |
| // --- collect in-scope records -------------------------------------------- |
| keys, partial, err := listAppSubtree(ctx, s, known, "instances/") |
| if err != nil { |
| - return fmt.Errorf("provision: list apps: %w", err) |
| + return recordScan{}, fmt.Errorf("provision: list apps: %w", err) |
| } |
| if partial { |
| readFailed = true // unseen records must not read as retired |
| @@ -941,6 +962,13 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| recs[rec.Host] = rec |
| } |
| } |
| + return recordScan{recs: recs, protect: protect, readFailed: readFailed}, nil |
| +} |
| + |
| +// reconcileScanned is the rest of a Reconcile pass over s, from its record |
| +// walk. |
| +func reconcileScanned(ctx context.Context, s release.Store, reg *registry.Registry, cfg ReconcileConfig, sc recordScan) error { |
| + recs, protect, readFailed := sc.recs, sc.protect, sc.readFailed |
| // --- manifest seed cache, per (acct, app), lazily ------------------------- |
| // known=false (a genuine read failure, not "no manifest") means the seed |
| @@ -1093,31 +1121,38 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| // removable — true ONLY for a marker positively read and present, |
| // whose app is not flagged production. Removal is not reversible, so |
| // an unreadable marker (or an unreadable flag) must never reach it. |
| - trashCache := map[acctApp]trashState{} |
| - trashSeen := map[acctApp]bool{} |
| - trashFor := func(acct, app string) trashState { |
| - k := acctApp{acct, app} |
| - if trashSeen[k] { |
| + // |
| + // trashOver is that lookup over st, answering from pre where pre holds |
| + // the app: the pass's own over s and its prefetch, and the removal |
| + // phase's over the bucket when this pass reads a snapshot (below). |
| + trashOver := func(st release.Store, pre map[acctApp]tombRead) func(acct, app string) trashState { |
| + trashCache := map[acctApp]trashState{} |
| + trashSeen := map[acctApp]bool{} |
| + return func(acct, app string) trashState { |
| + k := acctApp{acct, app} |
| + if trashSeen[k] { |
| + return trashCache[k] |
| + } |
| + trashSeen[k] = true |
| + var tomb *release.AppTombstone |
| + var err error |
| + if r, ok := pre[k]; ok { |
| + tomb, err = r.tomb, r.err |
| + } else { |
| + tomb, err = release.ReadAppTombstone(ctx, st, acct, app) |
| + } |
| + switch { |
| + case err != nil: |
| + log.Printf("provision: trash marker for %s/%s unreadable: %v — not minting for this app", acct, app, err) |
| + trashCache[k] = trashState{declineMint: true} |
| + case tomb != nil: |
| + log.Printf("provision: %s/%s is in the trash — not minting instance routes for it", acct, app) |
| + trashCache[k] = trashState{declineMint: true, removable: productionClear(ctx, st, cfg.ReleasePub, acct, app)} |
| + } |
| return trashCache[k] |
| } |
| - trashSeen[k] = true |
| - var tomb *release.AppTombstone |
| - var err error |
| - if r, ok := tombs[k]; ok { |
| - tomb, err = r.tomb, r.err |
| - } else { |
| - tomb, err = release.ReadAppTombstone(ctx, s, acct, app) |
| - } |
| - switch { |
| - case err != nil: |
| - log.Printf("provision: trash marker for %s/%s unreadable: %v — not minting for this app", acct, app, err) |
| - trashCache[k] = trashState{declineMint: true} |
| - case tomb != nil: |
| - log.Printf("provision: %s/%s is in the trash — not minting instance routes for it", acct, app) |
| - trashCache[k] = trashState{declineMint: true, removable: productionClear(ctx, s, cfg.ReleasePub, acct, app)} |
| - } |
| - return trashCache[k] |
| } |
| + trashFor := trashOver(s, tombs) |
| trashedFor := func(acct, app string) bool { return trashFor(acct, app).declineMint } |
| // --- mint / converge / conflict ------------------------------------------- |
| @@ -1504,7 +1539,15 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| // trade from the other three: it would strand precisely the hibernating |
| // tenants the ladder was built for, in exchange for guarding an authority |
| // this column does not carry. |
| - if !readFailed { |
| + // |
| + // retire runs that phase over one set of facts: the records, the |
| + // protected hosts and the trash lookup. With stopAtFirst it removes |
| + // nothing and reports whether any row would go. A record view |
| + // snapshot's absence is not the bucket's (a write that missed its bump |
| + // is missing from a Complete snapshot until the backstop rebuild), so a |
| + // pass over a snapshot that would remove anything decides every removal |
| + // again from the bucket's own walk, below (S3 change-index design §4.4). |
| + retire := func(recs map[string]Record, protect map[string]bool, trashFor func(acct, app string) trashState, stopAtFirst bool) (wouldRemove bool) { |
| for _, rt := range routes { |
| // Removal authority is scoped to this pass's OWN kind. A sidecar |
| // row is Provisioned too — that flag is what grants a pass removal |
| @@ -1552,16 +1595,8 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| log.Printf("provision: not removing %s (%s): participant of a running operation", rt.Host, why) |
| continue |
| } |
| - // Over a record view snapshot, the absence (or the marker) is |
| - // confirmed in the bucket first: a write that missed its bump |
| - // is not in the snapshot (S3 change-index design §4.4). |
| - if why == "record gone" && !confirmRecordGone(ctx, s, rt, func(r Record) bool { |
| - return cfg.inScope(r.Region, r.Pool, everywhereClass(r)) |
| - }) { |
| - continue |
| - } |
| - if why == "app in the trash" && !confirmTrashed(ctx, s, rt.Account, rt.App) { |
| - continue |
| + if stopAtFirst { |
| + return true |
| } |
| if cfg.DryRun { |
| log.Printf("provision: dry-run: would remove retired %s route %s (%s)", rt.Kind, rt.Host, why) |
| @@ -1573,7 +1608,23 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| } |
| log.Printf("provision: removed retired %s route %s (%s)", rt.Kind, rt.Host, why) |
| } |
| + return false |
| + } |
| + if readFailed || !retire(recs, protect, trashFor, onSnapshot(s)) { |
| + return nil |
| + } |
| + // A snapshot pass with a removal to make: the record walk again, over |
| + // the bucket, and the removal phase over what it read, by the walk's |
| + // own rules (invalid shapes and empty channels protected before scope, |
| + // hosts matched across every key, the bucket's trash marker). Anything |
| + // it cannot read keeps every row this pass, as in the walk. |
| + live := freshStore(s) |
| + lsc, err := scanRecords(ctx, live, cfg) |
| + if err != nil || lsc.readFailed { |
| + log.Printf("provision: not removing any route this pass: the record view lacks a record or holds a trash marker, and the bucket's own walk could not be read cleanly%s", walkErrSuffix(err)) |
| + return nil |
| } |
| + retire(lsc.recs, lsc.protect, trashOver(live, nil), false) |
| return nil |
| } |
diff --git a/internal/provision/recordview_store.go b/internal/provision/recordview_store.go| index 00fa81881..8c1f74789 100644 |
| --- a/internal/provision/recordview_store.go |
| +++ b/internal/provision/recordview_store.go |
| @@ -5,7 +5,6 @@ import ( |
| "errors" |
| "fmt" |
| "io" |
| - "log" |
| "maps" |
| "os" |
| "slices" |
| @@ -13,7 +12,6 @@ import ( |
| "sync" |
| "github.com/carlosframework/platform/internal/domains" |
| - "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -323,72 +321,26 @@ func freshStore(s release.Store) release.Store { |
| return s |
| } |
| -// onSnapshot reports whether s is a record view snapshot store. |
| +// onSnapshot reports whether s is a record view snapshot store. A snapshot's |
| +// absence is not the bucket's decision: a record written without its bump (a |
| +// failed bump, an old binary, a direct-bucket writer) is missing from a |
| +// Complete snapshot until the backstop rebuild. So Reconcile and |
| +// ReconcileSidecars over a snapshot decide any removal from the bucket's own |
| +// walk (scanRecords, scanSidecars over freshStore), and over any other store |
| +// from the walk they already made. |
| func onSnapshot(s release.Store) bool { |
| _, ok := s.(*snapStore) |
| return ok |
| } |
| -// Absence in a snapshot is not the bucket's decision: a record written |
| -// without its bump (a failed bump, an old binary, a direct-bucket writer) is |
| -// missing from a Complete snapshot until the backstop rebuild. A removal |
| -// pass over a snapshot therefore confirms in the bucket before it retires a |
| -// row. Over any other store these answer true without reading: the walk's |
| -// absence is the bucket's. |
| - |
| -// confirmRecordGone confirms that the instance record behind rt is gone from |
| -// the bucket, or out of this pass's scope there (inScope). A record that is |
| -// there and in scope, or that cannot be read cleanly, keeps the row. |
| -func confirmRecordGone(ctx context.Context, s release.Store, rt registry.Route, inScope func(Record) bool) bool { |
| - if !onSnapshot(s) { |
| - return true |
| - } |
| - rec, err := GetRecord(ctx, freshStore(s), rt.Account, rt.App, rt.Host) |
| - switch { |
| - case errors.Is(err, os.ErrNotExist): |
| - return true |
| - case err != nil: |
| - log.Printf("provision: not removing %s: the record view lacks its record and the bucket's could not be read cleanly: %v", rt.Host, err) |
| - return false |
| - case !inScope(*rec): |
| - return true |
| +// walkErrSuffix renders the error of a bucket walk that could not be read |
| +// cleanly for a log line: nothing when the walk returned but read partially |
| +// (its own lines name what failed). |
| +func walkErrSuffix(err error) string { |
| + if err == nil { |
| + return "" |
| } |
| - log.Printf("provision: not removing %s: the record view lacks its record but the bucket has it; the view catches up at its next refresh or rebuild", rt.Host) |
| - return false |
| -} |
| - |
| -// confirmSidecarGone is confirmRecordGone for a sidecar row, whose Slug is |
| -// its record's name. |
| -func confirmSidecarGone(ctx context.Context, s release.Store, rt registry.Route, inScope func(SidecarRecord) bool) bool { |
| - if !onSnapshot(s) { |
| - return true |
| - } |
| - rec, err := GetSidecarRecord(ctx, freshStore(s), rt.Account, rt.App, rt.Slug) |
| - switch { |
| - case errors.Is(err, os.ErrNotExist): |
| - return true |
| - case err != nil: |
| - log.Printf("provision: not removing sidecar %s: the record view lacks its record and the bucket's could not be read cleanly: %v", rt.Host, err) |
| - return false |
| - case rec.Host != rt.Host || !inScope(*rec): |
| - return true |
| - } |
| - log.Printf("provision: not removing sidecar %s: the record view lacks its record but the bucket has it; the view catches up at its next refresh or rebuild", rt.Host) |
| - return false |
| -} |
| - |
| -// confirmTrashed confirms that the app's trash marker, which the snapshot |
| -// has, is still in the bucket. Absent or unreadable keeps the row. |
| -func confirmTrashed(ctx context.Context, s release.Store, acct, app string) bool { |
| - if !onSnapshot(s) { |
| - return true |
| - } |
| - tomb, err := release.ReadAppTombstone(ctx, freshStore(s), acct, app) |
| - if err != nil || tomb == nil { |
| - log.Printf("provision: not removing %s/%s's rows: the record view has its trash marker but the bucket does not (%v)", acct, app, err) |
| - return false |
| - } |
| - return true |
| + return ": " + err.Error() |
| } |
| type snapStore struct { |
diff --git a/internal/provision/recordview_test.go b/internal/provision/recordview_test.go| index 3d9685bdf..f0dd76f71 100644 |
| --- a/internal/provision/recordview_test.go |
| +++ b/internal/provision/recordview_test.go |
| @@ -1134,3 +1134,219 @@ func TestSnapshotAbsenceDoesNotRetireALiveSidecar(t *testing.T) { |
| t.Fatalf("a sidecar record really gone did not retire its row over the snapshot: %+v %v", rt, err) |
| } |
| } |
| + |
| +// removalWorld is one account with app shop (instances and sidecars |
| +// manifests) over a bucket whose writes bump the change index (live), with a |
| +// second handle on the same directory whose writes do not (bypass: a bump |
| +// that failed, an old binary, a direct-bucket writer). v follows live. |
| +type removalWorld struct { |
| + live *viewCountingStore |
| + bypass *release.DirStore |
| + acct string |
| + v *RecordView |
| +} |
| + |
| +func newRemovalWorld(t *testing.T) *removalWorld { |
| + t.Helper() |
| + root := t.TempDir() |
| + d := release.NewDirStore(root) |
| + d.EnableChangeIndex() |
| + w := &removalWorld{live: &viewCountingStore{DirStore: d}, bypass: release.NewDirStore(root)} |
| + w.acct = writeAccountManifest(t, w.live, 401, "Shop") |
| + if err := PutManifest(ctx, w.live, w.acct, "shop", Manifest{Schema: 1, App: "shop", Domain: "example.com"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := PutSidecarManifest(ctx, w.live, w.acct, "shop", SidecarManifest{Schema: 1, App: "shop", Args: []string{"run"}, Channel: "stable"}); err != nil { |
| + t.Fatal(err) |
| + } |
| + w.v = NewRecordView(w.live, release.NewChangeWatch(w.live)) |
| + return w |
| +} |
| + |
| +// snap is the view's current Complete snapshot as a store over live. |
| +func (w *removalWorld) snap(t *testing.T) release.Store { |
| + t.Helper() |
| + return completeSnapshot(t, w.v).Store(w.live) |
| +} |
| + |
| +func (w *removalWorld) record(app, slug, region, channel string) Record { |
| + return Record{Schema: 1, Host: slug + ".example.com", Slug: slug, App: app, Account: w.acct, |
| + Region: region, Channel: channel, CreatedAt: time.Now()} |
| +} |
| + |
| +// listings counts the record-family listings live answered since the last |
| +// reset: what a walk costs and a snapshot pass must not. |
| +func (w *removalWorld) listings() int { |
| + n := 0 |
| + for _, c := range w.live.got() { |
| + for _, m := range []string{"List ", "ListDirs ", "ListETags "} { |
| + if strings.HasPrefix(c, m+"accounts/") || strings.HasPrefix(c, m+"apps/") { |
| + n++ |
| + } |
| + } |
| + } |
| + return n |
| +} |
| + |
| +// Final review findings 2 and 4 (amadan #336): a removal a snapshot proves |
| +// by absence is decided by the walk's own rules over the live bucket. A |
| +// sidecar whose placement becomes no-inbound, in its record or in the app's |
| +// sidecars manifest, with the bump landing, leaves an inbound box: the live |
| +// walk does not place it either. Before, the confirmation asked only region |
| +// and pool, and kept the row for ever. |
| +func TestSnapshotSidecarPlacementChangeEvacuates(t *testing.T) { |
| + for _, tc := range []struct { |
| + name string |
| + apply func(t *testing.T, w *removalWorld, rec SidecarRecord) |
| + }{ |
| + {"record", func(t *testing.T, w *removalWorld, rec SidecarRecord) { |
| + rec.Placement = PlacementNoInbound |
| + putReplace(t, w.live, SidecarRecordKey(w.acct, "shop", "bot"), mustJSON(t, rec)) |
| + }}, |
| + {"manifest", func(t *testing.T, w *removalWorld, _ SidecarRecord) { |
| + putReplace(t, w.live, SidecarManifestKey(w.acct, "shop"), mustJSON(t, |
| + SidecarManifest{Schema: 1, App: "shop", Args: []string{"run"}, Channel: "stable", Placement: PlacementNoInbound})) |
| + }}, |
| + } { |
| + t.Run(tc.name, func(t *testing.T) { |
| + t.Parallel() |
| + w := newRemovalWorld(t) |
| + host := SidecarHost("bot", "shop", w.acct) |
| + rec := SidecarRecord{Schema: 1, Name: "bot", Host: host, App: "shop", Account: w.acct, Channel: "stable"} |
| + if err := CreateSidecarRecord(ctx, w.live, rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := ReconcileConfig{DataDir: t.TempDir()} // an inbound box |
| + if err := ReconcileSidecars(ctx, w.snap(t), reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, host) |
| + sn := w.snap(t) |
| + w.live.reset() |
| + if err := ReconcileSidecars(ctx, sn, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if n := w.listings(); n != 0 { |
| + t.Fatalf("a snapshot pass with nothing to remove listed the bucket: %v", w.live.got()) |
| + } |
| + |
| + tc.apply(t, w, rec) |
| + if err := ReconcileSidecars(ctx, w.snap(t), reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get(host); err != nil || rt != nil { |
| + t.Fatalf("a no-inbound sidecar stayed on an inbound box over the snapshot: %+v %v", rt, err) |
| + } |
| + }) |
| + } |
| +} |
| + |
| +// A record the snapshot lacks, whose live copy has an empty channel and a |
| +// region out of this pass's scope: the walk protects an unusable record |
| +// before it asks about scope, so the row stays. Before, the confirmation |
| +// decoded the record, found it out of scope, and removed the row. |
| +func TestSnapshotRemovalProtectsAnInvalidOutOfScopeRecord(t *testing.T) { |
| + t.Parallel() |
| + w := newRemovalWorld(t) |
| + sn := w.snap(t) // before the record: it never reaches the view |
| + rec := w.record("shop", "one", "ie", "stable") |
| + if err := CreateRecord(ctx, w.bypass, rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := ReconcileConfig{Region: "ie", DataDir: "/data"} |
| + if err := Reconcile(ctx, w.live, reg, cfg); err != nil { // a walking pass mints |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, rec.Host) |
| + bad := rec |
| + bad.Region, bad.Channel = "uk", "" |
| + putReplace(t, w.bypass, RecordKey(w.acct, "shop", rec.Host), mustJSON(t, bad)) |
| + if err := Reconcile(ctx, sn, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, rec.Host) // protected, as the walk protects it |
| + if err := Reconcile(ctx, w.live, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, rec.Host) // and the walk agrees |
| +} |
| + |
| +// A host whose record moved to another key (another app here) without a |
| +// bump: the walk matches hosts across the whole deployment, so the row |
| +// stays. Before, the confirmation read only the row's old key, found it |
| +// gone, and removed the row. Once the record is gone everywhere, the row |
| +// goes. |
| +func TestSnapshotRemovalKeepsARehomedHost(t *testing.T) { |
| + t.Parallel() |
| + w := newRemovalWorld(t) |
| + w.snap(t) // built before the record: no bypass write below reaches the view |
| + rec := w.record("shop", "one", "ie", "stable") |
| + if err := CreateRecord(ctx, w.bypass, rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := ReconcileConfig{Region: "ie", DataDir: "/data"} |
| + if err := Reconcile(ctx, w.live, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, rec.Host) |
| + if err := DeleteRecord(ctx, w.bypass, w.acct, "shop", rec.Host); err != nil { |
| + t.Fatal(err) |
| + } |
| + moved := w.record("cafe", "one", "ie", "stable") |
| + if err := CreateRecord(ctx, w.bypass, moved); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Reconcile(ctx, w.snap(t), reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, rec.Host) // kept: the bucket still has the host |
| + |
| + if err := DeleteRecord(ctx, w.bypass, w.acct, "cafe", rec.Host); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Reconcile(ctx, w.snap(t), reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get(rec.Host); err != nil || rt != nil { |
| + t.Fatalf("a host really gone did not retire its row over the snapshot: %+v %v", rt, err) |
| + } |
| +} |
| + |
| +// Disarmed (any store that is not a snapshot), removal is the walk's, from |
| +// the one walk the pass already made: no second walk. |
| +func TestRemovalOverTheBucketWalksOnce(t *testing.T) { |
| + t.Parallel() |
| + w := newRemovalWorld(t) |
| + rec := w.record("shop", "one", "ie", "stable") |
| + if err := CreateRecord(ctx, w.live, rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t) |
| + cfg := ReconcileConfig{Region: "ie", DataDir: "/data"} |
| + if err := Reconcile(ctx, w.live, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + mustGet(t, reg, rec.Host) |
| + if err := DeleteRecord(ctx, w.live, w.acct, "shop", rec.Host); err != nil { |
| + t.Fatal(err) |
| + } |
| + w.live.reset() |
| + if err := Reconcile(ctx, w.live, reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rt, err := reg.Get(rec.Host); err != nil || rt != nil { |
| + t.Fatalf("a gone record's row survived a walking pass: %+v %v", rt, err) |
| + } |
| + n := 0 |
| + for _, c := range w.live.got() { |
| + if c == "List accounts/" { |
| + n++ |
| + } |
| + } |
| + if n != 1 { |
| + t.Fatalf("a walking pass listed accounts/ %d time(s), want 1: %v", n, w.live.got()) |
| + } |
| +} |
diff --git a/internal/provision/sidecar_reconcile.go b/internal/provision/sidecar_reconcile.go| index 10bc2c53e..45cfec6b2 100644 |
| --- a/internal/provision/sidecar_reconcile.go |
| +++ b/internal/provision/sidecar_reconcile.go |
| @@ -192,10 +192,38 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| cfg.Now = time.Now |
| } |
| - known, readFailed, err := knownAccounts(ctx, s) |
| + sc, err := scanSidecars(ctx, s, cfg) |
| if err != nil { |
| return err |
| } |
| + return reconcileSidecarsScanned(ctx, s, reg, cfg, sc) |
| +} |
| + |
| +// sidecarScan is the record-collection phase of a ReconcileSidecars pass: the |
| +// placeable in-scope records by host, the hosts whose record is there but |
| +// cannot be used (protect), the per-app sidecars manifest lookup the pass |
| +// goes on using, and whether anything failed to read (readFailed, which |
| +// disables removal for the whole pass; a manifest read that fails later in |
| +// the pass sets it too). |
| +type sidecarScan struct { |
| + recs map[string]SidecarRecord |
| + protect map[string]bool |
| + readFailed bool |
| + manifestFor func(acct, app string) (SidecarManifest, bool) |
| +} |
| + |
| +// scanSidecars is ReconcileSidecars' record walk over s. A pass over a record |
| +// view snapshot that has a removal to decide runs it again over the bucket |
| +// (freshStore), so both judge every record by exactly these rules, |
| +// placement against the record and the app's sidecars manifest included |
| +// (S3 change-index design §4.4). |
| +func scanSidecars(ctx context.Context, s release.Store, cfg ReconcileConfig) (*sidecarScan, error) { |
| + sc := &sidecarScan{} |
| + known, readFailed, err := knownAccounts(ctx, s) |
| + if err != nil { |
| + return nil, err |
| + } |
| + sc.readFailed = readFailed |
| // --- manifest cache, per (acct, app), lazily ------------------------------ |
| // known=false covers BOTH "no manifest" and "the read failed", and in both |
| @@ -231,7 +259,7 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| return SidecarManifest{}, false |
| } |
| log.Printf("provision: sidecar manifest %s/%s: read failed: %v", acct, app, err) |
| - readFailed = true |
| + sc.readFailed = true |
| return SidecarManifest{}, false |
| } |
| manCache[k], manKnown[k] = *m, true |
| @@ -241,10 +269,10 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| // --- collect in-scope records -------------------------------------------- |
| keys, partial, err := listAppSubtree(ctx, s, known, "sidecars/") |
| if err != nil { |
| - return fmt.Errorf("provision: list apps: %w", err) |
| + return nil, fmt.Errorf("provision: list apps: %w", err) |
| } |
| if partial { |
| - readFailed = true // unseen records must not read as retired |
| + sc.readFailed = true // unseen records must not read as retired |
| } |
| recs := map[string]SidecarRecord{} // host -> record, in-scope only |
| // protect holds hosts whose record KEY parsed but whose body could not be |
| @@ -274,7 +302,7 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| // Its absence from recs below must not read as "the bucket retired |
| // it" — a transient Get failure disables removal for the whole |
| // pass, same as an unreadable account manifest. |
| - readFailed = true |
| + sc.readFailed = true |
| continue |
| } |
| var rec SidecarRecord |
| @@ -351,6 +379,14 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| } |
| } |
| + sc.recs, sc.protect, sc.manifestFor = recs, protect, manifestFor |
| + return sc, nil |
| +} |
| + |
| +// reconcileSidecarsScanned is the rest of a ReconcileSidecars pass over s, |
| +// from its record walk. |
| +func reconcileSidecarsScanned(ctx context.Context, s release.Store, reg *registry.Registry, cfg ReconcileConfig, sc *sidecarScan) error { |
| + recs, protect, manifestFor := sc.recs, sc.protect, sc.manifestFor |
| trashed := trashGate(ctx, s, "sidecar") |
| // --- mint / converge ------------------------------------------------------- |
| @@ -464,7 +500,13 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| // authority; a hand-added row never has it), scope (this box's own |
| // region/pool jurisdiction), and absence from the record set this pass just |
| // read (the bucket's own decision to retire the sidecar, not a guess). |
| - if !readFailed { |
| + // |
| + // retire runs that phase over one set of records and protected hosts. |
| + // With stopAtFirst it removes nothing and reports whether any row would |
| + // go: a record view snapshot's absence is not the bucket's, so a pass |
| + // over a snapshot that would remove anything decides every removal again |
| + // from the bucket's own walk, below (S3 change-index design §4.4). |
| + retire := func(recs map[string]SidecarRecord, protect map[string]bool, stopAtFirst bool) (wouldRemove bool) { |
| for _, rt := range routes { |
| if rt.Kind != SidecarKind || !rt.Provisioned { |
| continue |
| @@ -479,12 +521,8 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| log.Printf("provision: not removing sidecar %s: its record exists but could not be read cleanly this pass", rt.Host) |
| continue |
| } |
| - // Over a record view snapshot, the absence is confirmed in the |
| - // bucket first (confirmRecordGone's reason). |
| - if !confirmSidecarGone(ctx, s, rt, func(r SidecarRecord) bool { |
| - return r.Region == cfg.Region && poolServed(cfg.Pools, r.Pool) |
| - }) { |
| - continue |
| + if stopAtFirst { |
| + return true |
| } |
| if cfg.DryRun { |
| log.Printf("provision: dry-run: would remove retired sidecar route %s (record gone)", rt.Host) |
| @@ -496,7 +534,23 @@ func ReconcileSidecars(ctx context.Context, s release.Store, reg *registry.Regis |
| } |
| log.Printf("provision: removed retired sidecar route %s (record gone)", rt.Host) |
| } |
| + return false |
| + } |
| + if sc.readFailed || !retire(recs, protect, onSnapshot(s)) { |
| + return nil |
| + } |
| + // A snapshot pass with a removal to make: the record walk again, over |
| + // the bucket, and the removal phase over what it read, by the walk's own |
| + // rules (unusable records protected before scope, hosts matched across |
| + // every key, placement from the bucket's record and sidecars manifest |
| + // against this box's inbound). Anything it cannot read keeps every row |
| + // this pass, as in the walk. |
| + lsc, err := scanSidecars(ctx, freshStore(s), cfg) |
| + if err != nil || lsc.readFailed { |
| + log.Printf("provision: not removing any sidecar route this pass: the record view lacks a sidecar this box would run, and the bucket's own walk could not be read cleanly%s", walkErrSuffix(err)) |
| + return nil |
| } |
| + retire(lsc.recs, lsc.protect, false) |
| return nil |
| } |