amadan
provision: the hold write's record re-read reads the bucket, not the record view snapshot (#336)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+51
−1
internal/provision/reconcile.go+2 −1internal/provision/recordview_store.go+11 −0internal/provision/recordview_test.go+38 −0
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go| index 6c9f1276c..74c5aabdc 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -1709,7 +1709,8 @@ func holdExisting(ctx context.Context, s release.Store, reg *registry.Registry, |
| return |
| } |
| WithHostHoldLock(rec.Host, func() { |
| - fresh, err := GetRecord(ctx, s, rec.Account, rec.App, rec.Host) |
| + // From the bucket, not a record view snapshot: the listing may be one. |
| + fresh, err := GetRecord(ctx, freshStore(s), rec.Account, rec.App, rec.Host) |
| if err != nil { |
| log.Printf("provision: %s: not holding this pass: re-read the record: %v", rec.Host, err) |
| return |
diff --git a/internal/provision/recordview_store.go b/internal/provision/recordview_store.go| index 913edf4e8..67a2e9ae0 100644 |
| --- a/internal/provision/recordview_store.go |
| +++ b/internal/provision/recordview_store.go |
| @@ -310,6 +310,17 @@ func (sn *ViewSnapshot) Store(live release.Store) release.Store { |
| return &snapStore{sn: sn, live: live} |
| } |
| +// freshStore is the store a reader that must see the bucket as it is now |
| +// reads through: live under a snapshot store, s itself otherwise. A guard |
| +// that re-reads a record to check that nothing moved since the listing |
| +// (holdExisting) would only read the listing's own snapshot back. |
| +func freshStore(s release.Store) release.Store { |
| + if ss, ok := s.(*snapStore); ok { |
| + return ss.live |
| + } |
| + return s |
| +} |
| + |
| type snapStore struct { |
| sn *ViewSnapshot |
| live release.Store |
diff --git a/internal/provision/recordview_test.go b/internal/provision/recordview_test.go| index 30592441b..6ed7d6b25 100644 |
| --- a/internal/provision/recordview_test.go |
| +++ b/internal/provision/recordview_test.go |
| @@ -903,3 +903,41 @@ func TestViewScopeApp(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// A pass over a snapshot keeps the hold write's re-read fresh: the record is |
| +// re-read from the bucket, not from the snapshot the listing came from, so a |
| +// move that went on after the view was refreshed is not held again (the |
| +// guard TestReconcileHoldRereadsTheRecordBeforeHoldingARow pins on the walk). |
| +func TestSnapshotHoldRereadReadsTheBucket(t *testing.T) { |
| + s, sqid := setup(t) |
| + rec := movingRecord(sqid) |
| + if err := CreateRecord(ctx, s, rec); err != nil { |
| + t.Fatal(err) |
| + } |
| + v := NewRecordView(s, release.NewChangeWatch(s)) |
| + if err := v.Rebuild(ctx); err != nil { |
| + t.Fatal(err) |
| + } |
| + committed := rec |
| + m := *rec.Move |
| + m.State = MoveCommitted |
| + committed.Move = &m |
| + key := RecordKey(rec.Account, rec.App, rec.Host) |
| + _, etag, err := s.Get(ctx, key) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := s.Put(ctx, key, mustJSON(t, committed), etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + reg := regWith(t, RouteFor(rec, "/data", `["seed"]`, false, nil)) // the target's row, opened |
| + var wrote holdLog |
| + cfg := ReconcileConfig{Region: "uk", Pools: []string{"tito"}, DataDir: "/data", HoldWritten: wrote.written, |
| + Hold: func(r Record) (string, string, bool) { return "landing", r.Move.ID, r.Move.State == MoveLanding }} |
| + if err := Reconcile(ctx, v.Snapshot().Store(s), reg, cfg); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got := mustGet(t, reg, rec.Host); got.Status != "ready" || got.HoldOp != "" || len(wrote.hosts) != 0 { |
| + t.Fatalf("an opened row was held again from the snapshot's record: %+v %v", got, wrote.hosts) |
| + } |
| +} |