amadan

carlos / platform Public

provision: share the record-key parsing between the two lister (Plan 8 Task 1 fix round 1)

recordHosts is the one place that decides what a record key looks like.
Also: note the manifest-less and soft-deleted account blind spot on
ListAllRecordsStrict, test the tolerant fill of the unreadable set, and use
a real sqid in the ListRecordsStrict tests.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 580cd4a1898572536714e9df753002d9372dd741 parent a7bb1d9
4 files changed, +64 −46
  • internal/provision/provision.go +23 −24
  • internal/provision/provision_test.go +17 −22
  • internal/provision/reconcile.go +4 −0
  • internal/provision/reconcile_test.go +20 −0

CI passed — run details

diff --git a/internal/provision/provision.go b/internal/provision/provision.go
index df8e38e4c..22e7c51ca 100644
--- a/internal/provision/provision.go
+++ b/internal/provision/provision.go
@@ -1955,6 +1955,27 @@ func GetRecordETag(ctx context.Context, s release.Store, acct, app, host string)
return &rec, etag, nil
}
+// recordHosts picks the record hosts out of a listing of RecordsPrefix:
+// the keys of the form <prefix><host>.json, in listing order. Anything else
+// (a key nested below the level, a name without .json) is not a record at
+// this level. Both ListRecords and ListRecordsStrict read through it, so
+// the two never disagree about what a record key looks like.
+func recordHosts(keys []string, prefix string) []string {
+ var hosts []string
+ for _, key := range keys {
+ rest, ok := strings.CutPrefix(key, prefix)
+ if !ok || rest == "" || strings.Contains(rest, "/") {
+ continue
+ }
+ host, ok := strings.CutSuffix(rest, ".json")
+ if !ok || host == "" {
+ continue
+ }
+ hosts = append(hosts, host)
+ }
+ return hosts
+}
+
// ListRecords lists an app's instance records, sorted by host, capped at
// max with truncated reporting whether more existed. The cap is decided on
// the KEY count — how many instances/<host>.json objects exist under the
@@ -1979,18 +2000,7 @@ func ListRecords(ctx context.Context, s release.Store, acct, app string, max int
if err != nil {
return nil, false, err
}
- var hosts []string
- for _, key := range keys {
- rest, ok := strings.CutPrefix(key, prefix)
- if !ok || rest == "" || strings.Contains(rest, "/") {
- continue // not a record at this level (e.g. a nested key never written here)
- }
- host, ok := strings.CutSuffix(rest, ".json")
- if !ok || host == "" {
- continue
- }
- hosts = append(hosts, host)
- }
+ hosts := recordHosts(keys, prefix)
sort.Strings(hosts)
if len(hosts) > max {
truncated = true
@@ -2037,18 +2047,7 @@ func ListRecordsStrict(ctx context.Context, s release.Store, acct, app string, m
if err != nil {
return nil, nil, fmt.Errorf("provision: list records of %s/%s: %w", acct, app, err)
}
- var hosts []string
- for _, key := range keys {
- rest, ok := strings.CutPrefix(key, prefix)
- if !ok || rest == "" || strings.Contains(rest, "/") {
- continue
- }
- host, ok := strings.CutSuffix(rest, ".json")
- if !ok || host == "" {
- continue
- }
- hosts = append(hosts, host)
- }
+ hosts := recordHosts(keys, prefix)
if len(hosts) > max {
return nil, nil, ErrRecordsTruncated
}
diff --git a/internal/provision/provision_test.go b/internal/provision/provision_test.go
index c0fcdc962..95ecc93a7 100644
--- a/internal/provision/provision_test.go
+++ b/internal/provision/provision_test.go
@@ -1316,19 +1316,15 @@ func TestManifestPrivateStorageIsAdditive(t *testing.T) {
}
func TestListRecordsStrict(t *testing.T) {
- ctx := context.Background()
- s := release.NewDirStore(t.TempDir())
+ s, sqid := shopSetup(t)
for _, h := range []string{"c.example.com", "a.example.com", "b.example.com"} {
- rec := Record{Schema: 1, Host: h, Slug: h[:1], App: "shop", Account: "bad", Channel: "stable", CreatedAt: time.Now().UTC()}
+ rec := Record{Schema: 1, Host: h, Slug: h[:1], App: "shop", Account: sqid, Channel: "stable", CreatedAt: time.Now().UTC()}
if err := CreateRecord(ctx, s, rec); err != nil {
t.Fatal(err)
}
}
- if err := PutManifest(ctx, s, "bad", "shop", Manifest{Schema: 1, App: "shop", Domain: "example.com"}); err != nil {
- t.Fatal(err)
- }
- recs, unreadable, err := ListRecordsStrict(ctx, s, "bad", "shop", 3)
+ recs, unreadable, err := ListRecordsStrict(ctx, s, sqid, "shop", 3)
if err != nil || len(unreadable) != 0 || unreadable == nil || len(recs) != 3 {
t.Fatalf("clean listing = %+v, %#v, %v", recs, unreadable, err)
}
@@ -1340,59 +1336,58 @@ func TestListRecordsStrict(t *testing.T) {
// The cap is an error, not a flag: one key over, and at 0 or below.
for _, max := range []int{2, 0, -1} {
- recs, unreadable, err := ListRecordsStrict(ctx, s, "bad", "shop", max)
+ recs, unreadable, err := ListRecordsStrict(ctx, s, sqid, "shop", max)
if !errors.Is(err, ErrRecordsTruncated) || recs != nil || unreadable != nil {
t.Fatalf("max %d = %+v, %v, %v; want ErrRecordsTruncated and nothing else", max, recs, unreadable, err)
}
}
// An app with no records is complete at any cap.
- if recs, unreadable, err := ListRecordsStrict(ctx, s, "bad", "empty", 0); err != nil || len(recs) != 0 || len(unreadable) != 0 {
+ if recs, unreadable, err := ListRecordsStrict(ctx, s, sqid, "empty", 0); err != nil || len(recs) != 0 || len(unreadable) != 0 {
t.Fatalf("empty app = %+v, %v, %v", recs, unreadable, err)
}
// A body that does not decode, and one that names another host, are
// reported by the host in their key. ListRecords skips them.
- if _, err := s.Put(ctx, RecordKey("bad", "shop", "z.example.com"), []byte("not json"), ""); err != nil {
+ if _, err := s.Put(ctx, RecordKey(sqid, "shop", "z.example.com"), []byte("not json"), ""); err != nil {
t.Fatal(err)
}
- liar := Record{Schema: 1, Host: "elsewhere.example.com", Slug: "y", App: "shop", Account: "bad", Channel: "stable", CreatedAt: time.Now().UTC()}
+ liar := Record{Schema: 1, Host: "elsewhere.example.com", Slug: "y", App: "shop", Account: sqid, Channel: "stable", CreatedAt: time.Now().UTC()}
raw, err := json.Marshal(liar)
if err != nil {
t.Fatal(err)
}
- if _, err := s.Put(ctx, RecordKey("bad", "shop", "y.example.com"), raw, ""); err != nil {
+ if _, err := s.Put(ctx, RecordKey(sqid, "shop", "y.example.com"), raw, ""); err != nil {
t.Fatal(err)
}
- recs, unreadable, err = ListRecordsStrict(ctx, s, "bad", "shop", 10)
+ recs, unreadable, err = ListRecordsStrict(ctx, s, sqid, "shop", 10)
if err != nil || len(recs) != 3 || len(unreadable) != 2 || !unreadable["z.example.com"] || !unreadable["y.example.com"] {
t.Fatalf("with two bad bodies = %d recs, %v, %v", len(recs), unreadable, err)
}
- if plain, _, err := ListRecords(ctx, s, "bad", "shop", 10); err != nil || len(plain) != 3 {
+ if plain, _, err := ListRecords(ctx, s, sqid, "shop", 10); err != nil || len(plain) != 3 {
t.Fatalf("ListRecords must keep skipping them: %d, %v", len(plain), err)
}
}
func TestListRecordsStrictFailsOnAnyReadFailure(t *testing.T) {
- ctx := context.Background()
- inner := release.NewDirStore(t.TempDir())
+ inner, sqid := shopSetup(t)
for _, h := range []string{"a.example.com", "b.example.com"} {
- rec := Record{Schema: 1, Host: h, Slug: h[:1], App: "shop", Account: "bad", Channel: "stable", CreatedAt: time.Now().UTC()}
+ rec := Record{Schema: 1, Host: h, Slug: h[:1], App: "shop", Account: sqid, Channel: "stable", CreatedAt: time.Now().UTC()}
if err := CreateRecord(ctx, inner, rec); err != nil {
t.Fatal(err)
}
}
// ListRecords drops the record it cannot Get; the strict listing fails.
fs := &failStore{Store: inner}
- fs.failReadsFor(RecordKey("bad", "shop", "b.example.com"))
- if plain, _, err := ListRecords(ctx, fs, "bad", "shop", 10); err != nil || len(plain) != 1 {
+ fs.failReadsFor(RecordKey(sqid, "shop", "b.example.com"))
+ if plain, _, err := ListRecords(ctx, fs, sqid, "shop", 10); err != nil || len(plain) != 1 {
t.Fatalf("ListRecords over an unreadable record = %d, %v", len(plain), err)
}
- if recs, unreadable, err := ListRecordsStrict(ctx, fs, "bad", "shop", 10); err == nil || recs != nil || unreadable != nil {
+ if recs, unreadable, err := ListRecordsStrict(ctx, fs, sqid, "shop", 10); err == nil || recs != nil || unreadable != nil {
t.Fatalf("ListRecordsStrict over an unreadable record = %+v, %v, %v; want an error", recs, unreadable, err)
}
// A listing that fails is an error too.
- ls := &bucketShaped{Store: inner, failList: RecordsPrefix("bad", "shop")}
- if recs, unreadable, err := ListRecordsStrict(ctx, ls, "bad", "shop", 10); err == nil || recs != nil || unreadable != nil {
+ ls := &bucketShaped{Store: inner, failList: RecordsPrefix(sqid, "shop")}
+ if recs, unreadable, err := ListRecordsStrict(ctx, ls, sqid, "shop", 10); err == nil || recs != nil || unreadable != nil {
t.Fatalf("ListRecordsStrict over a failed listing = %+v, %v, %v; want an error", recs, unreadable, err)
}
}
diff --git a/internal/provision/reconcile.go b/internal/provision/reconcile.go
index 2a6ea373f..118cad568 100644
--- a/internal/provision/reconcile.go
+++ b/internal/provision/reconcile.go
@@ -1822,6 +1822,10 @@ func ListAllRecords(ctx context.Context, s release.Store) ([]Record, error) {
// success. The records returned are exactly ListAllRecords' set, and every
// error is ListAllRecords' error; a caller deleting on absence must keep
// everything that belongs to a host in unreadable.
+//
+// One more blind spot, shared with ListAllRecords and Reconcile: records
+// under an account with no manifest, or whose manifest is soft-deleted, are
+// not in recs and not in unreadable. They are invisible, not unreadable.
func ListAllRecordsStrict(ctx context.Context, s release.Store) (recs []Record, unreadable map[string]bool, err error) {
recs, _, unreadable, err = readAllRecords(ctx, s, true)
if err != nil {
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go
index 64750ee2f..2d4e17e5c 100644
--- a/internal/provision/reconcile_test.go
+++ b/internal/provision/reconcile_test.go
@@ -3627,6 +3627,26 @@ func TestListAllRecordsStrictNamesUnreadableHosts(t *testing.T) {
}
}
+// The tolerant reader fills the same set. No wrapper exposes it, so this
+// reads readAllRecords directly: a record that reads but does not decode is
+// skipped, named, and does not make the set incomplete (it was read).
+func TestReadAllRecordsTolerantNamesUnreadableHosts(t *testing.T) {
+ s, sqid := shopSetup(t)
+ for _, slug := range []string{"good", "garbled"} {
+ if err := CreateRecord(ctx, s, strictShopRecord(sqid, slug)); err != nil {
+ t.Fatal(err)
+ }
+ }
+ putReplace(t, s, RecordKey(sqid, "shop", "garbled.shop.example.com"), []byte("{not json"))
+ recs, complete, unreadable, err := readAllRecords(ctx, s, false)
+ if err != nil || !complete || len(recs) != 1 || recs[0].Host != "good.shop.example.com" {
+ t.Fatalf("recs %+v complete %v err %v", recs, complete, err)
+ }
+ if len(unreadable) != 1 || !unreadable["garbled.shop.example.com"] {
+ t.Fatalf("unreadable %v, want garbled.shop.example.com", unreadable)
+ }
+}
+
func TestListAllRecordsStrictCleanSetHasNoUnreadableHosts(t *testing.T) {
s, sqid := shopSetup(t)
if err := CreateRecord(ctx, s, strictShopRecord(sqid, "one")); err != nil {