amadan
provision: strict record listings name what they could not read (Plan 8 Task 1)
ListAllRecordsStrict returns ListAllRecords' set plus the hosts whose record key parsed but whose body did not decode or disagreed with its path. readAllRecords fills that set in both modes; ListAllRecords and ListAllRecordsTolerant behave as before. ListRecordsStrict is the per-app lister for authority checks: a listing or Get failure is an error, a truncated listing is ErrRecordsTruncated, and a malformed or path-disagreeing body is reported by host, not skipped.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
4 files changed,
+259
−10
internal/provision/provision.go+63 −0internal/provision/provision_test.go+82 −0internal/provision/reconcile.go+38 −10internal/provision/reconcile_test.go+76 −0
diff --git a/internal/provision/provision.go b/internal/provision/provision.go| index 56bac8d73..df8e38e4c 100644 |
| --- a/internal/provision/provision.go |
| +++ b/internal/provision/provision.go |
| @@ -2006,6 +2006,69 @@ func ListRecords(ctx context.Context, s release.Store, acct, app string, max int |
| return recs, truncated, nil |
| } |
| +// ErrRecordsTruncated is ListRecordsStrict's answer when the app holds more |
| +// record keys than the caller's cap. |
| +var ErrRecordsTruncated = errors.New("provision: the app has more records than the cap allows") |
| + |
| +// ListRecordsStrict is ListRecords for a caller that decides something from |
| +// the whole record set, or from a record's absence: whether another record |
| +// shares an environment, whether an import is in flight. ListRecords is |
| +// built for display and quietly drops what it cannot show; this one never |
| +// does, and the difference is in three places. |
| +// |
| +// - A listing that fails, or a record key that will not Get (any error, |
| +// including not-found: the key was listed a moment ago), is an error and |
| +// no records come back. ListRecords skips such a record. |
| +// - More record keys than max is the error ErrRecordsTruncated, not a |
| +// flag the caller might forget to read. max < 0 is clamped to 0, as in |
| +// ListRecords, so an app with any record fails at a cap of 0. |
| +// - A record that reads but does not decode, or whose body disagrees with |
| +// its own path, is not returned and not skipped silently: its host (the |
| +// host in the key) is in unreadable. A caller must treat each such host |
| +// as a record that could be anything, and fail closed on it. |
| +// |
| +// recs is sorted by host. unreadable is non-nil whenever err is nil. |
| +func ListRecordsStrict(ctx context.Context, s release.Store, acct, app string, max int) (recs []Record, unreadable map[string]bool, err error) { |
| + if max < 0 { |
| + max = 0 |
| + } |
| + prefix := RecordsPrefix(acct, app) |
| + keys, err := s.List(ctx, prefix) |
| + 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) |
| + } |
| + if len(hosts) > max { |
| + return nil, nil, ErrRecordsTruncated |
| + } |
| + sort.Strings(hosts) |
| + unreadable = map[string]bool{} |
| + for _, host := range hosts { |
| + raw, _, err := s.Get(ctx, RecordKey(acct, app, host)) |
| + if err != nil { |
| + return nil, nil, fmt.Errorf("provision: read record %s: %w", RecordKey(acct, app, host), err) |
| + } |
| + rec, err := DecodeRecord(raw) |
| + if err != nil || rec.Account != acct || rec.App != app || rec.Host != host { |
| + unreadable[host] = true |
| + continue |
| + } |
| + recs = append(recs, rec) |
| + } |
| + return recs, unreadable, nil |
| +} |
| + |
| // SocketDir is where a hibernating instance's control socket lives — the |
| // same tmpfs every other exec-managed route already uses |
| // (host/bootstrap.go's tmpfiles.d entry). |
diff --git a/internal/provision/provision_test.go b/internal/provision/provision_test.go| index e7ba07d95..c0fcdc962 100644 |
| --- a/internal/provision/provision_test.go |
| +++ b/internal/provision/provision_test.go |
| @@ -1314,3 +1314,85 @@ func TestManifestPrivateStorageIsAdditive(t *testing.T) { |
| t.Errorf("an app without the policy still writes the key: %s", raw) |
| } |
| } |
| + |
| +func TestListRecordsStrict(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + 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()} |
| + 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) |
| + if err != nil || len(unreadable) != 0 || unreadable == nil || len(recs) != 3 { |
| + t.Fatalf("clean listing = %+v, %#v, %v", recs, unreadable, err) |
| + } |
| + for i, want := range []string{"a.example.com", "b.example.com", "c.example.com"} { |
| + if recs[i].Host != want { |
| + t.Errorf("recs[%d].Host = %q, want %q", i, recs[i].Host, want) |
| + } |
| + } |
| + |
| + // 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) |
| + 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 { |
| + 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 { |
| + t.Fatal(err) |
| + } |
| + liar := Record{Schema: 1, Host: "elsewhere.example.com", Slug: "y", App: "shop", Account: "bad", 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 { |
| + t.Fatal(err) |
| + } |
| + recs, unreadable, err = ListRecordsStrict(ctx, s, "bad", "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 { |
| + 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()) |
| + 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()} |
| + 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 { |
| + 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 { |
| + 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 { |
| + 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 fa89b5e6c..2a6ea373f 100644 |
| --- a/internal/provision/reconcile.go |
| +++ b/internal/provision/reconcile.go |
| @@ -1804,10 +1804,32 @@ func trashGate(ctx context.Context, s release.Store, what string) func(acct, app |
| // skipped and logged (Reconcile's `protect` treatment, minus the registry |
| // bookkeeping). One malformed record must not blind a caller to every other. |
| func ListAllRecords(ctx context.Context, s release.Store) ([]Record, error) { |
| - recs, _, err := readAllRecords(ctx, s, true) |
| + recs, _, _, err := readAllRecords(ctx, s, true) |
| return recs, err |
| } |
| +// ListAllRecordsStrict is ListAllRecords with one more answer: the hosts it |
| +// skipped. ListAllRecords is already fail-closed on every READ failure (a |
| +// listing that fails, a manifest or a record that will not Get is an error |
| +// and no records come back), but it logs and drops a record that reads and |
| +// then does not decode, or whose body disagrees with its own path, so the |
| +// set it returns is silently short of those records. A caller that draws a |
| +// conclusion from a record's absence (the seeds sweep deciding no record |
| +// names a snapshot) cannot tell that gap from a record that is not there. |
| +// ListAllRecordsStrict names the gap: unreadable holds the host of each |
| +// record key whose body did not decode or disagreed with its path (keyed by |
| +// the host in the path, never the body's). unreadable is non-nil on |
| +// 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. |
| +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 { |
| + return nil, nil, err |
| + } |
| + return recs, unreadable, nil |
| +} |
| + |
| // ListAllRecordsTolerant is ListAllRecords for a caller that acts on each |
| // record it reads, one at a time, and draws nothing from a record's absence |
| // (the run sweep's expiry and lost-run phases). An account manifest that |
| @@ -1817,21 +1839,25 @@ func ListAllRecords(ctx context.Context, s release.Store) ([]Record, error) { |
| // when nothing could be read. A caller that also draws conclusions from |
| // absence must act on that only when complete is true. |
| func ListAllRecordsTolerant(ctx context.Context, s release.Store) (recs []Record, complete bool, err error) { |
| - return readAllRecords(ctx, s, false) |
| + recs, complete, _, err = readAllRecords(ctx, s, false) |
| + return recs, complete, err |
| } |
| // readAllRecords is both: strict fails on any read failure, as |
| // ListAllRecords documents; tolerant logs it, skips what it could not read |
| -// and reports complete false. |
| -func readAllRecords(ctx context.Context, s release.Store, strict bool) (recs []Record, complete bool, err error) { |
| +// and reports complete false. In both modes unreadable holds the path host |
| +// of every record that read but did not decode or disagreed with its path |
| +// (skipped, logged); it is non-nil whenever err is nil. |
| +func readAllRecords(ctx context.Context, s release.Store, strict bool) (recs []Record, complete bool, unreadable map[string]bool, err error) { |
| complete = true |
| + unreadable = map[string]bool{} |
| known, readFailed, err := knownAccounts(ctx, s) |
| if err != nil { |
| - return nil, false, err |
| + return nil, false, nil, err |
| } |
| if readFailed { |
| if strict { |
| - return nil, false, errors.New("provision: an account manifest could not be read cleanly — refusing to report a partial record set") |
| + return nil, false, nil, errors.New("provision: an account manifest could not be read cleanly — refusing to report a partial record set") |
| } |
| log.Printf("provision: an account manifest could not be read cleanly: that account's records are skipped") |
| complete = false |
| @@ -1840,11 +1866,11 @@ func readAllRecords(ctx context.Context, s release.Store, strict bool) (recs []R |
| // from it — is stable call to call, whatever order the store lists in. |
| keys, partial, err := listAppSubtree(ctx, s, known, "instances/") |
| if err != nil { |
| - return nil, false, fmt.Errorf("provision: list apps: %w", err) |
| + return nil, false, nil, fmt.Errorf("provision: list apps: %w", err) |
| } |
| if partial { |
| if strict { |
| - return nil, false, errors.New("provision: an app's records could not be listed — refusing to report a partial record set") |
| + return nil, false, nil, errors.New("provision: an app's records could not be listed — refusing to report a partial record set") |
| } |
| log.Printf("provision: an app's records could not be listed: they are skipped") |
| complete = false |
| @@ -1862,7 +1888,7 @@ func readAllRecords(ctx context.Context, s release.Store, strict bool) (recs []R |
| raw, err := bodies[key].body, bodies[key].err |
| if err != nil { |
| if strict { |
| - return nil, false, fmt.Errorf("provision: read record %s: %w", key, err) |
| + return nil, false, nil, fmt.Errorf("provision: read record %s: %w", key, err) |
| } |
| log.Printf("provision: skipping record %s: unreadable: %v", key, err) |
| complete = false |
| @@ -1871,17 +1897,19 @@ func readAllRecords(ctx context.Context, s release.Store, strict bool) (recs []R |
| rec, err := DecodeRecord(raw) |
| if err != nil { |
| log.Printf("provision: skipping record %s: malformed: %v", key, err) |
| + unreadable[host] = true |
| continue |
| } |
| if rec.Host != host || rec.Account != acct || rec.App != app { |
| log.Printf("provision: skipping record %s: disagrees with its own path (host=%q account=%q app=%q)", |
| key, rec.Host, rec.Account, rec.App) |
| + unreadable[host] = true |
| continue |
| } |
| recs = append(recs, rec) |
| } |
| } |
| - return recs, complete, nil |
| + return recs, complete, unreadable, nil |
| } |
| // listAppSubtree returns the keys a record reader needs to see: every key |
diff --git a/internal/provision/reconcile_test.go b/internal/provision/reconcile_test.go| index 89c09984a..64750ee2f 100644 |
| --- a/internal/provision/reconcile_test.go |
| +++ b/internal/provision/reconcile_test.go |
| @@ -3584,3 +3584,79 @@ func TestReconcileHandsRunsToRunSeen(t *testing.T) { |
| t.Fatalf("RunSeen saw %q", seen) |
| } |
| } |
| + |
| +// strictShopRecord is a record of app "shop" under sqid, on a host under |
| +// example.com, for the strict-listing tests below. |
| +func strictShopRecord(sqid, slug string) Record { |
| + return Record{Schema: 1, Host: slug + ".shop.example.com", Slug: slug, App: "shop", |
| + Account: sqid, Region: "ie", Channel: "stable", CreatedAt: time.Now()} |
| +} |
| + |
| +func TestListAllRecordsStrictNamesUnreadableHosts(t *testing.T) { |
| + s, sqid := shopSetup(t) |
| + for _, slug := range []string{"good", "garbled", "liar"} { |
| + 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")) |
| + // A body that decodes but names another host than its path does. |
| + liar := strictShopRecord(sqid, "other") |
| + putReplace(t, s, RecordKey(sqid, "shop", "liar.shop.example.com"), mustJSON(t, liar)) |
| + |
| + recs, unreadable, err := ListAllRecordsStrict(ctx, s) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(recs) != 1 || recs[0].Host != "good.shop.example.com" { |
| + t.Fatalf("recs %+v, want only good.shop.example.com", recs) |
| + } |
| + if len(unreadable) != 2 || !unreadable["garbled.shop.example.com"] || !unreadable["liar.shop.example.com"] { |
| + t.Fatalf("unreadable %v, want garbled and liar keyed by their path hosts", unreadable) |
| + } |
| + |
| + // The wrappers behave as they always did: the same records, no |
| + // unreadable host, no error. |
| + plain, err := ListAllRecords(ctx, s) |
| + if err != nil || len(plain) != 1 || plain[0].Host != "good.shop.example.com" { |
| + t.Fatalf("ListAllRecords = %+v, %v", plain, err) |
| + } |
| + tol, complete, err := ListAllRecordsTolerant(ctx, s) |
| + if err != nil || !complete || len(tol) != 1 || tol[0].Host != "good.shop.example.com" { |
| + t.Fatalf("ListAllRecordsTolerant = %+v, complete %v, %v", tol, complete, err) |
| + } |
| +} |
| + |
| +func TestListAllRecordsStrictCleanSetHasNoUnreadableHosts(t *testing.T) { |
| + s, sqid := shopSetup(t) |
| + if err := CreateRecord(ctx, s, strictShopRecord(sqid, "one")); err != nil { |
| + t.Fatal(err) |
| + } |
| + recs, unreadable, err := ListAllRecordsStrict(ctx, s) |
| + if err != nil || len(recs) != 1 { |
| + t.Fatalf("recs %+v err %v", recs, err) |
| + } |
| + if unreadable == nil || len(unreadable) != 0 { |
| + t.Fatalf("unreadable = %#v, want an empty non-nil map", unreadable) |
| + } |
| +} |
| + |
| +// A read failure is still an error, with no records and no map: the strict |
| +// listing adds to ListAllRecords' answer and never weakens it. |
| +func TestListAllRecordsStrictKeepsFailingClosed(t *testing.T) { |
| + inner, sqid := shopSetup(t) |
| + if err := CreateRecord(ctx, inner, strictShopRecord(sqid, "one")); err != nil { |
| + t.Fatal(err) |
| + } |
| + s := &bucketShaped{Store: inner, failList: RecordsPrefix(sqid, "shop")} |
| + recs, unreadable, err := ListAllRecordsStrict(ctx, s) |
| + if err == nil || recs != nil || unreadable != nil { |
| + t.Fatalf("a failed app listing = %+v, %v, %v; want an error and nothing else", recs, unreadable, err) |
| + } |
| + fs := &failStore{Store: inner} |
| + fs.failReadsFor(RecordKey(sqid, "shop", "one.shop.example.com")) |
| + recs, unreadable, err = ListAllRecordsStrict(ctx, fs) |
| + if err == nil || recs != nil || unreadable != nil { |
| + t.Fatalf("a record that will not Get = %+v, %v, %v; want an error and nothing else", recs, unreadable, err) |
| + } |
| +} |