amadan
A store record that does not decode holds at once, whatever the decoder's complaint; the older storage tests no longer run as if the day were over
From the fourth review of this branch (Astra reject, Opus accept with fixes, P3 only): - objstore.Inspect wraps every decoding failure in ErrRecordMalformed. The storage pass holds on it at once: a time that is not a time was read as a failed reading and failed the pass every hour. - The older storage-pass tests pin the clock to mid-day, so the holds they assert are not the last-hour fallback's. - One inventory per pass is pinned by a test. - 19i names the holds that never lift by themselves and the daily cost of a reading that never succeeds. Astra's P1 (a grant-only record written after the inventory's walk) is answered in the dispositions: that store has no credential, because a grant opens its ledger interval before it creates the user. Billing is off.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+378
−8
docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review4.md+138 −0docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md+15 −0docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review4.md+123 −0implementation/carloku-paid-launch/OUTSTANDING.md+1 −1internal/console/economicspass.go+3 −6internal/console/economicspass_test.go+89 −0internal/objstore/objstore.go+9 −1
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review4.md b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review4.md| new file mode 100644 |
| index 000000000..07b52948f |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review4.md |
| @@ -0,0 +1,138 @@ |
| +Reviewed `cd66eabf`, including `e6015870..cd66eabf` and `c68ecddd..cd66eabf`. All locations below refer to `cd66eabf`. No files changed, tests run, or network commands used. Test outcomes below are conclusions from source inspection. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P1 — A grant-only holder can still escape both the inventory and the ledger comparisons.** |
| + `internal/objstore/walk.go:1739`, `:1789`; `internal/console/economicspass.go:2490`. |
| + |
| + A concrete schedule: |
| + |
| + - A is provisioned on bucket B. The inventory reads B’s ledger generation. |
| + - The record walk reads visitor V’s declaration, which names no bucket yet. |
| + - An operator grants B to V. Grant writes V’s grant block at `walk.go:348–350`. |
| + - Grant’s subsequent ledger operation at `:452` fails before changing the ledger, leaving the grant block standing. |
| + - A’s turn sees neither V in the inventory nor V on the ledger. Both generation comparisons succeed; A’s unchanged record passes the reread, and B’s bytes are recorded against A. |
| + |
| + This is a normal Grant failure path. It does **not** establish that V received credentials; it establishes the grant-only ambiguity that this branch explicitly treats as a holder elsewhere. The pass cannot distinguish this record from other unfinished grants by ignoring its existence. |
| + |
| + Reading ledgers first therefore closes the completed-grant race from review three, but does not cover every record-based holder. The new inventory-race test opens and closes ledger intervals directly, so it cannot expose this window. |
| + |
| + **Smallest acceptable fix:** make a persistent ledger holder intent visible before publishing a bucket-naming grant block, preserving conservative evidence across failures. Alternatively, synchronize sizing with bucket walks and refresh holder evidence under that exclusion. Another ledger-generation comparison alone cannot detect a record-only change. |
| + |
| +2. **P2 — Malformed timestamps are treated as failed reads, defeating the immediate malformed-record disposition.** |
| + `internal/console/economicspass.go:2529`, with the erroneous retry at `:238`. |
| + |
| + Suppose an existing `store.json` contains a grant timestamp such as `"at":"bad"`. The inventory successfully reads the object, fails to decode it, and adds it to `Unparsed`. The pass intends to write a day with stores held. |
| + |
| + When the pass reaches that app, `objstore.Get` returns the timestamp decoding error from `Inspect` (`internal/objstore/objstore.go:254`). That error is a `time.ParseError`, not either JSON error type recognized by `storeRecordUnusable`. At 10:00 the entire pass therefore returns without writing the day. Every retry repeats this until the last-hour fallback. |
| + |
| + Non-string timestamps can likewise produce custom decoding errors outside the two recognized JSON types. This contradicts the stated distinction between “bytes returned but unusable” and “reading failed.” |
| + |
| + **Smallest acceptable fix:** wrap all record-decoding failures in an objstore malformed-record sentinel inside `Inspect`; classify that sentinel alongside the coherence errors. Add malformed timestamp cases, including an unrelated healthy account. |
| + |
| +3. **P3 — The tests still do not protect ledger validation or checking the cutoff at failure time.** |
| + `internal/console/economicspass_test.go:2079`, `:2287`. |
| + |
| + `TestStoragePassHoldsTheStoresOfAMalformedLedger` supplies invalid JSON only. Removing `ledger.validate(bucket)` at `walk.go:1840` would leave that test passing, despite permitting syntactically valid invalid ownership evidence. |
| + |
| + `TestStoragePassRetriesEachOfAStoresReadings` fixes the clock at either 10:05 or 23:05 for the entire pass. Sampling the cutoff once at entry would still pass every case, despite restoring the failure when a pass crosses 23:00. |
| + |
| + The old retry test at `:1989` also still passes without final-ledger-error handling because it fails the inventory read first. **The new nth-read test now supplies the missing final-read coverage**, so that particular coverage defect is closed overall. |
| + |
| + **Smallest acceptable fix:** add a syntactically valid ledger rejected by validation, and advance the clock across 23:00 inside a read hook before injecting its failure. |
| + |
| +4. **P3 — The disposition says persistent holds were documented, but 19i still omits several and gives an insufficient release condition.** |
| + `implementation/carloku-paid-launch/OUTSTANDING.md:68`; `docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md:47`. |
| + |
| + A malformed ledger, an invalid-path record, or an orphan open interval can keep a healthy remaining store held indefinitely. None is explicitly described in 19i’s persistent-hold list. Its statement that an unfinished grant holds “until it is run again” is insufficient: a successful rerun can leave two live stores sharing the bucket, so the hold continues. |
| + |
| + **Smallest acceptable fix:** document the shapes and actual release conditions below. These conservative holds should not be removed merely to make measurement proceed. |
| + |
| + | Persistent shape | What releases future days? | Does 19i say so? | |
| + |---|---|---| |
| + | Retained departed record, with closed intervals or no ledger | Finish the retained teardown when permitted; merely rerunning it while a foreign holder remains can retain it again. | Yes, broadly. | |
| + | `Coherent`-refused record | Repair its conflicting evidence. Every named bucket remains held meanwhile. | Yes, through 19g. | |
| + | Grant-only record | Resolve the unfinished estate **and the sharing**. Completing the grant alone need not help. | Listed; release condition inaccurate. | |
| + | Two live stores, including two apps of one account | End the sharing. | Yes. | |
| + | Unparseable record | Repair it; it otherwise makes every provisioned store uncertain. | Yes; finding 2 affects finalization. | |
| + | Record under an invalid account/app name | Manual repair/removal; normal record APIs reject that path. | No explicit persistent-hold disclosure. | |
| + | Open ledger interval without its store record | Reconcile the orphan interval; `Teardown` returns immediately for a missing record at `walk.go:910`. | No. | |
| + | Malformed ledger | Repair the ledger; its bucket’s stores remain held. | No explicit disclosure. | |
| + | Unsupported bucket name | Repair/migrate the estate so ownership evidence can be checked. | Yes, newly added. | |
| + |
| + Repair enables future measurements. It does not rewrite an already committed held day. |
| + |
| +**Disposition checks** |
| + |
| +“Would pass without the fix” means removing the corresponding protection while retaining the test. “No test” means the claimed property is not established by a dedicated regression case. |
| + |
| +| Disposition row | Closed at `cd66eabf`? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| R1: Holder appears/leaves during sizing | Yes for ledger-recorded grants; record-only window remains in finding 1. | **No.** Removing the final generation comparison admits the injected size. | |
| +| R1: Per-store inventories and transient failures | Repeated remote inventories fixed; ordinary read failures now retry. Collection remains unbounded. | No request-count test. Later retry tests **would fail** without their retry policy. | |
| +| R1: Invalid account/app names skipped | Yes for storage’s literal-key inventory; Grant’s scan remains unchanged. | **No** for the tested invalid account. Invalid app names lack a separate case. | |
| +| R1: Malformed record causes global hold | Implemented for tested JSON errors; incomplete for other decoding errors, finding 2. | **No** for the malformed-JSON case if the global hold is removed. | |
| +| R1: Departed store’s leftover bytes | Yes while its retained record is inventoried. | **No.** The test requires the hold, then measurement after record removal. | |
| +| R1: Permanent holds documented | Partly; finding 4 remains. | No test. | |
| +| R1: Cross-deployment holder | Accepted open; not re-reported. | No test. | |
| +| R1: Tests strengthened | Yes for the added negative cases. | Negative cases **would fail**; “a sole holder” **would pass**, appropriately as a positive control. | |
| +| R2: Holder comes/goes before a store’s turn | Yes for the ledger-recorded event. | **No.** The pre-sizing generation comparison is necessary. | |
| +| R2: Failed readings and malformed ledgers | Named transport and malformed-ledger paths fixed, within the accepted scheduling limits. Finding 2 remains for record decoding. | **No** for tested retry/finalization and malformed-JSON behavior. Removing ledger validation **would still pass**. | |
| +| R2: Final ledger read commits a hold | Closed, now with coverage. | Old cited test: **yes**. New nth-read test: **no**. | |
| +| R2: Cross-deployment holder | Accepted open; not re-reported. | No test. | |
| +| R2: Missing inventory-rule/failure cases | Closed for the listed shapes. | **No.** Grant-only, refused grant/provisioned/teardown references, and deleted-bucket exclusion have discriminating assertions. | |
| +| R2: Persistent holds and cost | Remote quadratic reads fixed; documentation incomplete. | No cost-bound or recovery test. | |
| +| R3: Ledger baseline taken after record walk | Closed for that schedule; finding 1 is a remaining record-publication window. | **No.** Moving ledger reads back after the walk defeats the new during-inventory test. | |
| +| R3: Three reads still hold mid-day; final read untested | Ordinary transport paths closed; final read now reached. Decoding classification remains incomplete. | **No** for reverting each retry path. **Yes** for sampling the clock only once. | |
| +| R3: Unsupported bucket name bypasses evidence | Closed: `ErrLedgerMalformed` causes a hold before sizing. | **No.** Returning success for `Legacy_Bucket` permits sizing. | |
| +| R3: Last-hour scheduling/listing/day-write gaps | Accepted open; not re-reported. Cutoff is now checked at each handled failure. | No scheduling test; fixed-clock tests do not prove resampling. | |
| +| R3: Missing persistent-hold documentation; repeated work | Repeated listing/sizing documented; missing hold descriptions remain. | No test. | |
| + |
| +**Ledger listing, ordering, and failed readings** |
| + |
| +- **Pagination:** `S3Store.List` delegates to `ListMod`, which consumes every paginator page (`internal/release/store.go:657`, `:715`). There is no first-page truncation. |
| +- **Keys without `.json`:** the inventory skips them, but canonical ownership readers and writers use `BucketOwnershipKey`, which always appends `.json` (`ownership.go:56`). Such a key is not an ownership ledger recognized elsewhere by this build. |
| +- **Ledger created between listing and record walk:** its baseline remains `""`. The direct pre-sizing read obtains its ETag and causes a hold. A new key missed behind a pagination cursor is conservative for the same reason. A completed open/close does not restore absence: ledger intervals are retained. |
| +- **Ledger-first ordering:** a grant opening after the bucket’s baseline changes its generation. A grant opening before it is visible through its open interval or retained record; a subsequent close changes the generation. The remaining counterexample is the grant block published **before any ledger change**, finding 1. |
| +- **Mid-day failed readings:** I found no remaining direct transport-error path among the inventory reads, own-record read, bucket sizing, own-record reread, and final ledger read that immediately commits a hold. They retry before the cutoff. Returned malformed evidence and observed ownership changes still intentionally commit holds mid-day. Finding 2 is the reverse error: malformed evidence incorrectly retries. |
| + |
| +**Grant’s take-back check** |
| + |
| +The refactor is behaviorally unchanged for reachable Grant inputs. |
| + |
| +| Step | Before → after | |
| +|---|---| |
| +| Entry condition | Same non-operator take-back condition. | |
| +| Record holders | Same `otherStoreHolders`, traversal order, invalid-name exclusions, refused-record treatment, and deleted-bucket exclusion. | |
| +| Read order | Entire record scan first, ledger second. A scan error prevents the ledger read. | |
| +| Reconciliation | `ledger.left` moves into `OtherHolders`; refused records remain exempt. Same-account filtering moves partly later but cannot change Grant’s decision. | |
| +| Ledger holders | Previously open foreign-account intervals; now other-app intervals with deduplication. Grant skips additional same-account entries. | |
| +| First refusal | Surviving record holders still precede ledger-only holders. Deduplication preserves the first foreign holder. An open interval makes `left` false. | |
| +| Errors | Same scan/read wrappers, wrapped causes, precedence, and `ErrBucketHeld` message. | |
| +| Invalid bucket names | The helper skips their ledger read, but Grant already rejects them at `walk.go:222`. No reachable behavior changes. | |
| + |
| +**Cost** |
| + |
| +The brief’s per-store full inventory would require approximately **N² record reads** for N apps with stores. That is gone. |
| + |
| +At this revision, one successful pass over N valid apps, all with separately sized stores, performs approximately: |
| + |
| +- **3N record GETs:** inventory, own record, post-sizing reread. |
| +- **L + 2N ledger GETs**, where L is **every listed ledger**, including historical buckets—not merely current stores. |
| +- Two account/app directory traversals, deployment-prefix listings, and full tenant-bucket listings. |
| + |
| +There is no app-count, ledger-count, object-count, or pass-specific time budget. Failed attempts repeat collection; successful day creation prevents subsequent collection that day. The new ledger-first scan therefore scales with accumulated ledger history as well as active apps. |
| + |
| +Remote record reads are linear. In-memory work can still be **O(N²)** when many records name one bucket: insertion scans existing holders at `walk.go:1759`, and each `Others` call scans them again at `:1805`. |
| + |
| +**Claims the code contradicts** |
| + |
| +- `OUTSTANDING.md:68` and `economicspass.go:2459` claim sole ownership throughout collection; finding 1 supplies a record-based counterexample. |
| +- `economicspass.go:2525` describes all returned unusable records, but its classifier omits timestamp decoding failures. |
| +- `DISPOSITIONS-storage-sole-holder.md:47` says the missing persistent holds are now in 19i. They remain absent. |
| +- `OUTSTANDING.md:68` implies rerunning an unfinished grant releases its hold. It can instead complete a second live holder. |
| +- `walk.go:1622` says `OtherHolders` reads every store record. Its underlying scan skips invalid account/app names at `:1368` and `:1377`. |
| +- `economicspass_test.go:1721` says a departed store does not count. The revised implementation and test count its retained record. |
| +- `economicspass.go:2493` adds record-holder and ledger-holder counts without deduplicating them. One other store represented in both produces a log claiming two other stores; the hold decision itself remains correct. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md| index 142c60399..e69927c47 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| @@ -47,3 +47,18 @@ Reports: `CODEX-FINDINGS-storage-sole-holder-review3.md`, `OPUS-REVIEW-storage-s |
| | Opus P3-2, P3-3: two holds missing from 19i; a failing pass repeats every listing and sizing each hour | 19i says so. A store whose bucket cannot be listed now fails the pass every hour until the last one, repeating the others' sizing: accepted for now, noted. | `OUTSTANDING.md` | none | |
| Mutation checks in a scratch copy, each failing its test: the ledgers read after the walk; each of the three readings holding instead of retrying; the final ledger read holding; a name with no ledger sized. |
| + |
| +## Review four (the diff `e6015870..cd66eabf`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-storage-sole-holder-review4.md`, `OPUS-REVIEW-storage-sole-holder-review4.md`. Opus's findings are all P3. Astra: one P1, one P2, two P3. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra P1: an operator grants bucket B to store V after the record walk has passed V's declaration; the grant writes its grant block and its ledger write then fails. V is in neither the inventory nor the ledger, B's generation has not moved, and B's own store A is sized. | Not accepted as a mis-attribution; recorded as a known limit of the evidence. The claim the pass makes is that nobody else could have written to B from the baseline to the end of the sizing. A grant opens its ledger interval before it creates the user or delivers any credential (`openBucketOwner` precedes `EnsureUser`), so V, whose ledger write failed, holds no credential and has written nothing: the bytes are A's. The unchanged generation is that evidence. It is true that a grant-only record counts as a holder when the inventory sees it, which is conservative; that the pass is less conservative for one written after the walk is stated in 19i. Closing it by rule would need the sizing to run under the bucket's walk lease. | `objstore/walk.go` `grant` (grant block, then ledger, then user) | none | |
| +| Astra P2, Opus P3-1: a record whose decoding fails for a reason other than syntax or type (a time that is not a time) was treated as a failed reading, so the pass failed every hour until the last one | Fixed. `Inspect` wraps every decoding failure in `ErrRecordMalformed`, and the pass holds on that at once. | `objstore/objstore.go` `Inspect`; `console/economicspass.go` `storeRecordUnusable` | `TestStoragePassHoldsAtOnceOnARecordThatDoesNotDecode` (four shapes, mid-day) | |
| +| Opus P3-2, Astra P3: the older tests ran as if the day were over, so their holds were the last-hour fallback's | Fixed. They pin the clock to 10:00 on their date. | `economicspass_test.go` `storageMidDay` | the three tests named in the report | |
| +| Opus P3-4: nothing pinned one inventory per pass | Added. | `economicspass_test.go` | `TestStoragePassReadsItsInventoryOnce` | |
| +| Opus P3-3, Astra P3: 19i missed permanent holds and the daily cost of a reading that never succeeds | 19i now says: a malformed ledger holds its bucket's stores until repaired by hand; a bucket name with no ledger is held for good; a reading that fails every time (a bucket deleted out of band, a ledger the console may not read) fails the pass every hour of every day and repeats the other stores' sizing. | `OUTSTANDING.md` | none | |
| +| Opus P3-5: the baseline could come from the listing's own ETags | Noted, not done. | none | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the unusable-record test always false; the sentinel not applied; a second inventory inside the per-store path. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review4.md b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review4.md| new file mode 100644 |
| index 000000000..0ae6a6975 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review4.md |
| @@ -0,0 +1,123 @@ |
| +# Storage sole holder: Opus review of `cd66eabf`, the fourth reading |
| + |
| +I found no P1 or P2. Every disposition row is closed or accepted as open. Four P3s remain. Three are in tests and comments; one is a 19i wording gap. A fifth note on cost has no fix needed. |
| + |
| +## Prior dispositions |
| + |
| +| Row | Closed? | Would its test pass without the fix? | |
| +| --- | --- | --- | |
| +| R1 holder appears during sizing | Closed: the ledger generation is compared after the sizing | No: the store would be sized | |
| +| R1 one inventory per pass / transient reads | Closed | Nothing pins "once": calling the inventory once per store passes every test (P3-4) | |
| +| R1 record under an invalid name | Closed for storage. The walks' own `storeHolders` scan still skips such names (19i says so) | No | |
| +| R1 unparsed record holds everything | Closed | No | |
| +| R1 departed store's record holds | Closed per ruling K | No | |
| +| R1 holds missing from 19i | Partly: see P3-3 | n/a | |
| +| R1, R2 another deployment's prefix | Accepted, open | n/a | |
| +| R2 came and went before a store's turn | Closed | No | |
| +| R2 retry until the last hour / malformed ledger | Closed | No. Both tests pin the clock mid-day | |
| +| R2 final ledger read retries | Closed. `"its ledger after the sizing"` (n=3) now reaches it | No | |
| +| R2 inventory rules | Closed | No (mutation-checked) | |
| +| R3 ledgers read before the walk | Closed | No: with the ledgers read after the walk, the baseline contains the visitor and the store is sized | |
| +| R3 three readings retry mid-day | Closed for failed readings. "A record that cannot be used still holds at once" is untested mid-day and incomplete (P3-1, P3-2) | The retry half fails without the fix. The "unusable" half passes without it | |
| +| R3 bucket name with no ledger | Closed | No | |
| +| R3 last-hour tick | Accepted, not re-reported | n/a | |
| + |
| +## The three specific questions |
| + |
| +**A holder in neither the baseline nor the walk: none found.** |
| +- Grant writes the grant block to the record (`Put`) before `openBucketOwner` opens the interval. |
| +- So an interval opened before the ledger reading has a record that already exists when the walk runs. |
| +- An interval opened after the reading moves the generation. |
| +- A record can vanish between the ledger reading and the walk in two ways: |
| + - A refused grant undoes its grant block, but a refused grant never opened an interval. |
| + - A finished teardown deletes the record. Against another account's holder, teardown is retained and keeps the record. Teardown finishes only when the bucket is destroyed or the remaining holder is the same account, so no bytes cross accounts. |
| +- A coherent record whose teardown noted its bucket deleted is skipped by the walk. If that teardown crashed before closing its interval, the open interval still holds the store. A take-back that clears the note writes the record before it opens its interval. |
| +- Ledgers only grow (`appendOnly`). They are never deleted outside tests, and the ETag tracks the content. So no generation can go A→B→A. |
| + |
| +**Can listing the ledger prefix miss a ledger: no.** |
| +- `S3Store.List` → `ListMod` uses the full paginator, and `DirStore.List` returns its error, which the inventory checks. |
| +- Every ledger key ends in `.json` (`BucketOwnershipKey`). Other keys under the prefix are not ledgers, and walk locks live under `objstore/walks/`. |
| +- A ledger created after the listing has generation `""` in the inventory, then an ETag at `OpenOnLedger`. That mismatch holds the store for the day. |
| +- A bucket named `x.json` maps back correctly, because only one `.json` is trimmed. |
| + |
| +**Does any reading still write a held day mid-day: no failed reading does.** |
| +- Every failed reading returns an error mid-day: |
| + - the inventory listing and record reads (`economicspass.go:200`); |
| + - the store's own record (`:238`); |
| + - `LedgerUnread`; |
| + - both `OpenOnLedger` reads; |
| + - `sizeStore`; |
| + - the record read after the sizing. |
| +- Mid-day holds come only from decisions or permanent states: |
| + - an unparsed or refused record; |
| + - a malformed ledger; |
| + - a bucket name with no ledger; |
| + - a generation that moved, or other holders; |
| + - a teardown that has begun. |
| +- The reverse fault exists: see P3-1. |
| + |
| +## Findings |
| + |
| +**P3-1: `storeRecordUnusable` misses record errors that are just as permanent** (`internal/console/economicspass.go:2529`, used at `:238` and in `sizeSoleHolder`). |
| +- **Scenario:** a `store.json` has `"at": "garbage"` in its grant block, or any non-RFC3339 time. `json.Unmarshal` returns `Time.UnmarshalJSON`'s error unwrapped: a `*time.ParseError`, or a plain error for a non-string value. That is neither a `*json.SyntaxError` nor a `*json.UnmarshalTypeError`. |
| + - The inventory lists the record in `Unparsed`, so every store is held. |
| + - But that app's own `objstore.Get` fails with a "transient" error, so the pass fails every hour until 23:00. |
| + - The day then depends on the one last-hour tick. The comment at `:196` and the R3 disposition both say such a record "holds at once". |
| + - The outcome is close to the intended one, since every store is held either way. But the day may not be written at all, and that contradicts the stated rule. |
| +- **Fix:** treat any error from `Inspect` after a successful `s.Get` as unusable. For example, have `Inspect` wrap decode failures in an `objstore.ErrRecordMalformed` sentinel and test for it, instead of listing json error types. |
| + |
| +**P3-2: mid-day holds for an unusable record are untested; every older storage-pass test runs as if the day were over.** |
| +- The real clock gives `storageLastChance("2026-08-12") == true`, because the day is not today. That covers: |
| + - `TestStoragePassDoesNotMeasureARefusedRecord` (`economicspass_test.go:1501`); |
| + - `TestStoragePassDegradesOnStoreRecordReadError` (`:329`); |
| + - `TestStoragePassSizesOnlyASoleHolderOfItsBucket` / "a record that will not parse" (`:1883`). |
| +- So making `storeRecordUnusable` return `false` passes the whole suite: each hold is the last-hour hold. |
| +- **Fix:** pin `storageClock` to 10:00 on the test date in those tests, and assert `err == nil` together with the hold. |
| + |
| +**P3-3: 19i is missing permanent holds** (`OUTSTANDING.md:68`). |
| +- Not in "holds that last until a person acts": a malformed ledger holds every store on its bucket until it is repaired by hand. |
| +- Not said to be permanent: "a bucket name this build keeps no ledger for is held". No hand repair exists short of moving the store to a valid name. |
| +- Not said: a permanently failing reading is treated as transient every day, forever. Examples are a bucket deleted out of band (`NoSuchBucket`) and a ledger the console gets `AccessDenied` on. Each makes the whole pass fail 23 times a day, for every account, and repeats the sizing of all the other stores. This is the accepted R3 cost, but 19i says "until then", not "every day". |
| +- **Fix:** three sentences in 19i. |
| + |
| +**P3-4: no test pins "one inventory per pass".** Calling `ReadHolderInventory` inside the per-store loop passes every test. |
| +- **Fix:** count the `Get`s of a third store's record across one pass with two stores, and assert exactly 1. |
| + |
| +**P3-5 (cost, question 4): the premise is out of date.** `storagePass` no longer calls `OtherHolders`. |
| +- **Per pass:** |
| + - The inventory costs 1 paginated List of `usage/buckets/`, plus L ledger Gets, plus 1 + A `ListDirs`, plus P record Gets. |
| + - Each provisioned store then adds 2 ledger Gets, 2 record Gets and a full listing of its bucket. |
| + - That is linear in L + P + objects. |
| +- **Bounds:** |
| + - The create-only day stops further passes once the day is written. |
| + - Nothing bounds a pass that keeps failing (P3-3). |
| + - L is every bucket that ever had a ledger, since ledgers are never deleted, so it grows with history, not with live stores. |
| +- `OtherHolders` still reads all P records per call, as before, but only on a member-door take-back grant. |
| +- **Optional fix:** take the baseline ETags from the ListObjectsV2 listing, which returns them, instead of L separate Gets. |
| + |
| +## Question 3: Grant's take-back check is unchanged |
| + |
| +Compared line by line (`walk.go:303`, against `c68ecddd`): |
| + |
| +- **Holders, in order:** records from `otherStoreHolders` (unchanged, still skips invalid names), then open ledger intervals. Same as before. |
| +- **Ledger intervals:** |
| + - Before, the filter excluded the whole account (`iv.Account != acct`). |
| + - Now it excludes only this exact app, and deduplicates. |
| + - Grant then skips `h.Account == acct`, so the result is the same. |
| +- **`left` reconciliation:** |
| + - Before, it ran over every holder. Now it runs over record holders only. |
| + - A ledger holder is an open interval, and `left()` returns false for any app with an open interval. |
| + - Same result. |
| +- **First holder named in the error:** still the first record holder, since the dedup keeps the record entry. Same `ErrBucketHeld` text. |
| +- **Errors:** both wrappers are byte-identical: `"objstore: list the stores holding %s: %w"` and `"objstore: read ownership of %s: %w"`. |
| +- **Only difference:** an invalid bucket name now skips the ledger read instead of erroring. Grant cannot reach this: it refuses invalid names at `:222`, before the lease. |
| + |
| +## Claims the code contradicts |
| + |
| +1. `walk.go:1622` (`OtherHolders` doc): "It reads every store record in the deployment". `storeHolders` skips accounts and apps with invalid names, so "a record no walk can follow holds every bucket it names" holds only for valid names. The same comment's "the records alone answer" for an invalid bucket name contradicts the stance `OpenOnLedger` takes. That path is unreachable from Grant. |
| +2. `economicspass.go:196–199` and DISPOSITIONS R3 row 2: "a record … that is there and will not parse … holds … at once". This is false for a record that fails with a time parse error (P3-1). |
| +3. 19i: "Holds that last until a person acts" leaves out the malformed ledger, and presents the invalid-bucket-name hold as fixable (P3-3). |
| + |
| +## Verdict |
| + |
| +**Accept with fixes** (P3-1 to P3-4. P3-5 is optional.) |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index dcf8a4e5d..75a76e159 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -65,7 +65,7 @@ Each is concrete. None is waived. |
| 19. **No review of the whole branch has passed.** Astra rejected rounds one to sixteen; Opus accepted each with fixes. Round seventeen read only the diff since round sixteen: Opus accepted, Astra accepted with fixes (one P2, fixed since and unconfirmed). That is not a pass of the branch: the last whole-branch reading was round sixteen's, and it was rejected. Every round has closed the shapes the round before reported and found neighbouring ones, in the store walks since round seven. Some concern records no walk of this build writes; others need only this build's ordinary walks (a failed rotation, a retained teardown, a scanning door racing a teardown), and two were faults in the previous round's own fixes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings were about records no walk of this build writes (a teardown note for another bucket, a record naming two buckets) being acted on by teardown, rotation and the scanning doors; a rotation started after a grant that failed at its last write; and a take-back that returned access to a bucket another account holds, through the member's own door. Round thirteen read those fixes: closed for their reported shapes, Astra rejected again (two P1, four P2) and Opus accepted with fixes (one P2, five P3). The new findings were paths that read such a record and acted without the check (the holder inventory, the storage pass, the declare door), a rotation's own failed last write, and a retained record blocking another account's take-back. The check now sits in the record's reader and writer. Round fourteen read those: closed for their reported shapes; Astra rejected (two P1, two P2), Opus accepted with fixes (two P2, two P3). Round fifteen read those: all closed; Astra rejected (one P1, one P2), Opus accepted with fixes (one P2, two P3), all about readers of a bucket that a retained teardown left to another account (a sweep in flight, the scan status, the storage pass). Round sixteen read those: all closed, and one of them had made a single store under teardown hold storage for every account. Astra rejected (one P1, two P2), Opus accepted with fixes (one P1, one P2, two P3). The storage hold is now per account; two findings are left open as 19f and 19i. Those fixes are on the launch branch and unconfirmed until the next round reads them. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| 19g. **Refusals whose doors say the wrong thing, and records only a person can repair.** A record that names two buckets (`ErrTwoBuckets`) or whose teardown note is about another bucket (`ErrUnfinishedTeardown`) is refused by the record's one reader and one writer, so nothing acts on it and nothing writes it; the settings page shows such a store as unreadable. No walk of this build writes either; whether an earlier build left such records in production has not been checked. There is no operator verb to repair one. The doors that meet these refusals answer with sentences written for other failures: the grant and provision doors say to run the grant again, the destroy door and the purge say to try again, the scanning doors say to try again or to re-run the grant. The same goes for a rotation refused because a grant must be run again first (`ErrGrantUnfinished`) and for a member's take-back refused because another account holds the bucket (`ErrBucketHeld`). Correct copy for each needs approval (A). |
| -19i. **A live bucket shared by two stores was sized in full for each of them.** Fixed on this branch (`feat/carloku-storage-sole-holder-20261001`), not on the launch branch: the daily storage pass sizes a store only when it is the sole holder of its bucket, by one inventory of the store records per pass and the bucket's ownership ledger, read before and after the sizing. Otherwise the store is held for its own account. Holds that last until a person acts: a store that left the bucket keeps its record, and holds the remaining store, until its retained teardown is finished, which nothing runs (19a); a record no walk can follow, until it is repaired by hand (19g); another store's grant that died after writing its grant block, until it is run again; two apps of one account on one bucket, always; any store record that will not parse holds every store. Whose bytes a departed store's leftover objects are needs Paul's ruling (approval K). Not covered: one bucket adopted in two deployments is invisible to each and is sized in full in both. The walks' own holder scan still skips records under names no build writes. A store on a bucket name this build keeps no ledger for is held: its ownership evidence cannot be checked. A reading the store fails to return (a record, a ledger, a bucket listing) fails the pass for a retry until the day's last hour (23:00 UTC), when the day is written with the affected stores held; a pass that keeps failing repeats every listing and sizing each hour until then. Open: the pass runs hourly, so the last hour has about one tick, and a failure of the listings it starts with, of the day's own write, or a pass that runs past that tick, still leaves the day unwritten, which holds storage for every account; it needs a short retry timer after 23:00. Reviewed three times (`DISPOSITIONS-storage-sole-holder.md`): Astra rejected each time, Opus accepted with fixes each time; the fixes after the third reading are unread. |
| +19i. **A live bucket shared by two stores was sized in full for each of them.** Fixed on this branch (`feat/carloku-storage-sole-holder-20261001`), not on the launch branch: the daily storage pass sizes a store only when it is the sole holder of its bucket, by one inventory of the store records per pass and the bucket's ownership ledger, read before and after the sizing. Otherwise the store is held for its own account. Holds that last until a person acts: a store that left the bucket keeps its record, and holds the remaining store, until its retained teardown is finished, which nothing runs (19a); a record no walk can follow, until it is repaired by hand (19g); another store's grant that died after writing its grant block, until it is run again; two apps of one account on one bucket, always; any store record that will not parse holds every store. Whose bytes a departed store's leftover objects are needs Paul's ruling (approval K). Not covered: one bucket adopted in two deployments is invisible to each and is sized in full in both. The walks' own holder scan still skips records under names no build writes. A store on a bucket name this build keeps no ledger for is held, for good: its ownership evidence cannot be checked, and nothing short of moving the store to a valid name changes that. A ledger that will not parse holds its bucket's stores until it is repaired by hand. A grant block written after the inventory's walk, by a grant whose ledger write then failed, is not seen; that store has no credential, so the bytes sized are the bucket's own store's. A reading the store fails to return (a record, a ledger, a bucket listing) fails the pass for a retry until the day's last hour (23:00 UTC), when the day is written with the affected stores held; a pass that keeps failing repeats every listing and sizing each hour until then, and a reading that never succeeds (a bucket deleted out of band, a ledger the console may not read) does that every day. Open: the pass runs hourly, so the last hour has about one tick, and a failure of the listings it starts with, of the day's own write, or a pass that runs past that tick, still leaves the day unwritten, which holds storage for every account; it needs a short retry timer after 23:00. Reviewed four times (`DISPOSITIONS-storage-sole-holder.md`): Astra rejected each time, Opus accepted with fixes each time, the fourth time with P3 findings only; the fixes after the fourth reading are unread. |
| 19h. **A rotation whose delivery fails is repaired by a grant, which swaps the app's key with no overlap.** A failed delivery may still land, so the record stays marked, the next rotation is refused, and the grant run again replaces whichever key the app holds. Hosts hold a dead key until they adopt the new config. Before this branch such a rotation was resumed by running it again, which kept the overlap when the delivery had not landed and broke the app when it had. A repair that waits out the signing window and then reads which key was delivered would keep the overlap where the window is enforced (observed on AWS only, 19d); it is not built. |
| 19f. **A store that leaves a bucket with an unsupported name does not hand on its scanner debt.** There is no ledger for such a name. The last holder would delete the bucket and leave a scanning plan behind. Today's scanner (ClamAV) has no plan to leave, so this is latent. Also latent for the same reason: the scan grant door checks that no teardown has begun and then enables scanning as two steps, so a retained teardown landing between them would leave scanning enabled on the other account's bucket with nothing recording that it is owed. And the scan queue's drain checks permission once per batch, so a drain admitted before a retained teardown could return the other account's object metadata; nothing calls the drain or enqueues today, and it must be fixed before the queue is wired. |
| 19e. **A purge can end with a store that was granted during it.** A declare and a grant admitted while the app was live can land between the purge's last check and the removal of its marker. The purge now reports that as an error and never deletes the record, but it does not prevent it: the doors check liveness when they admit a request, not when they write. And after that error the estate is named only by its record and a log line. The app's console row is still trashed, so the settings page and the store-destroy door answer 404 and a second purge answers "not trashed", until the console restarts and restores the row from the record. The flash the member sees says "Nothing was deleted", which is false in this case; correct copy needs approval (A). |
diff --git a/internal/console/economicspass.go b/internal/console/economicspass.go| index 4ce98ccdf..5acce0040 100644 |
| --- a/internal/console/economicspass.go |
| +++ b/internal/console/economicspass.go |
| @@ -2523,12 +2523,9 @@ func (s *Server) sizeSoleHolder(ctx context.Context, inventory *objstore.HolderI |
| } |
| // storeRecordUnusable reports that a store record was returned and cannot |
| -// be used: it will not parse, or it is one no walk can follow. That will be |
| -// so on the next reading too, so it holds the store at once. Any other |
| +// be used: it does not decode, or it is one no walk can follow. That will |
| +// be so on the next reading too, so it holds the store at once. Any other |
| // error is a reading the store failed to return, which is tried again. |
| func storeRecordUnusable(err error) bool { |
| - var syntax *json.SyntaxError |
| - var badType *json.UnmarshalTypeError |
| - return errors.Is(err, objstore.ErrTwoBuckets) || errors.Is(err, objstore.ErrUnfinishedTeardown) || |
| - errors.As(err, &syntax) || errors.As(err, &badType) |
| + return errors.Is(err, objstore.ErrRecordMalformed) || errors.Is(err, objstore.ErrTwoBuckets) || errors.Is(err, objstore.ErrUnfinishedTeardown) |
| } |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 87ef65ce1..83920d1e9 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -337,6 +337,9 @@ func TestStoragePassDegradesOnStoreRecordReadError(t *testing.T) { |
| if _, err := fx.rel.Put(ctx, objstore.Key(fixtureAcct, fixtureApp), []byte("{not valid json"), ""); err != nil { |
| t.Fatal(err) |
| } |
| + // Mid-day, so the hold below is the hold for an unusable record and |
| + // not the day's last-hour fallback. |
| + storageMidDay(t, "2026-08-11") |
| if err := fx.srv.storagePass(ctx, "2026-08-11"); err != nil { |
| t.Fatal(err) |
| @@ -1505,6 +1508,7 @@ func TestStoragePassDoesNotMeasureARefusedRecord(t *testing.T) { |
| } |
| fx := newConsoleFixture(t) |
| ctx := context.Background() |
| + storageMidDay(t, "2026-08-12") |
| putRawStore(t, fx, fixtureAcct, fixtureApp, c.rec) |
| sized := 0 |
| fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { |
| @@ -1883,6 +1887,7 @@ func TestStoragePassSizesOnlyASoleHolderOfItsBucket(t *testing.T) { |
| t.Run("a record that will not parse", func(t *testing.T) { |
| fx := newConsoleFixture(t) |
| ctx := context.Background() |
| + storageMidDay(t, "2026-08-12") |
| seedOther(t, fx) |
| if _, err := objstore.Put(ctx, fx.rel, fixtureAcct, fixtureApp, provisioned(shared), ""); err != nil { |
| t.Fatal(err) |
| @@ -2353,3 +2358,87 @@ func TestStoragePassHoldsAStoreWhoseBucketHasNoLedgerItCanRead(t *testing.T) { |
| t.Fatalf("sized %d time(s), %+v; want held, not sized", sized, app) |
| } |
| } |
| + |
| +// storageMidDay pins the storage pass's clock to 10:00 UTC on date for the |
| +// rest of the test, so that a hold it asserts is not the last-hour |
| +// fallback, which holds on anything. |
| +func storageMidDay(t *testing.T, date string) { |
| + t.Helper() |
| + day, err := time.Parse("2006-01-02", date) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + prev := storageClock |
| + storageClock = func() time.Time { return day.Add(10 * time.Hour) } |
| + t.Cleanup(func() { storageClock = prev }) |
| +} |
| + |
| +// A store record that was returned and is not a record holds at once, in |
| +// the middle of the day, with the pass succeeding: it will not decode on |
| +// the next reading either. That is every way of not decoding, a time that |
| +// is not a time included, not only broken syntax. |
| +func TestStoragePassHoldsAtOnceOnARecordThatDoesNotDecode(t *testing.T) { |
| + for name, body := range map[string]string{ |
| + "broken syntax": `{not valid json`, |
| + "a time that is not one": `{"schema":1,"grant":{"backend":"aws","bucket":"carlos-store-x","at":"garbage"}}`, |
| + "a time that is a number": `{"schema":1,"grant":{"backend":"aws","bucket":"carlos-store-x","at":7}}`, |
| + "a block of the wrong type": `{"schema":1,"provisioned":"yes"}`, |
| + } { |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + storageMidDay(t, "2026-08-12") |
| + if _, err := fx.rel.Put(ctx, objstore.Key(fixtureAcct, fixtureApp), []byte(body), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, _, err := objstore.Get(ctx, fx.rel, fixtureAcct, fixtureApp); !errors.Is(err, objstore.ErrRecordMalformed) { |
| + t.Fatalf("%s: objstore.Get = %v, want ErrRecordMalformed", name, err) |
| + } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Errorf("%s: the pass failed mid-day on a record that will never decode: %v", name, err) |
| + continue |
| + } |
| + day, written := storageDay(t, fx, "2026-08-12") |
| + if app := day.Apps[fixtureAcct+"/"+fixtureApp]; !written || !app.StoreUnswept { |
| + t.Errorf("%s: day written %v, %+v; want the day written with the store held", name, written, app) |
| + } |
| + } |
| +} |
| + |
| +// The holder inventory is read once for the whole pass, not once for each |
| +// store: an app's record is read by the inventory and by the pass's own |
| +// visit to that app, and by nothing else, however many stores there are. |
| +func TestStoragePassReadsItsInventoryOnce(t *testing.T) { |
| + at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + storageMidDay(t, "2026-08-12") |
| + for _, o := range []struct{ acct, app, bucket string }{{fixtureAcct, fixtureApp, "carlos-store-one"}, {"bcd", "shop", "carlos-store-two"}, {"bcd", "blog", "carlos-store-three"}} { |
| + if _, err := fx.rel.Put(ctx, release.AppPrefix(o.acct, o.app)+"marker", []byte("x"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + rec := &objstore.Record{ |
| + Grant: &objstore.GrantBlock{Backend: "aws", Bucket: o.bucket, Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Bucket: o.bucket, Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}, |
| + } |
| + if _, err := objstore.Put(ctx, fx.rel, o.acct, o.app, rec, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // A fourth app with a declaration only: it is never sized, so its |
| + // record is read exactly twice. |
| + if _, err := fx.rel.Put(ctx, release.AppPrefix("bef", "plain")+"marker", []byte("x"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := objstore.Put(ctx, fx.rel, "bef", "plain", &objstore.Record{Declared: &objstore.Declared{EnvPrefix: "S3", At: at, By: "m@example.com"}}, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + counting := &nthGet{Store: fx.rel, key: objstore.Key("bef", "plain")} |
| + fx.srv.cfg.Releases = counting |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { return 1 << 30, nil } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Fatal(err) |
| + } |
| + if counting.seen != 2 { |
| + t.Fatalf("the declared-only app's record was read %d times in a pass over three stores; want 2 (the inventory and its own visit)", counting.seen) |
| + } |
| +} |
diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go| index 197baab67..51bd99ce9 100644 |
| --- a/internal/objstore/objstore.go |
| +++ b/internal/objstore/objstore.go |
| @@ -230,6 +230,11 @@ func Get(ctx context.Context, s release.Store, acct, app string) (*Record, strin |
| return rec, etag, nil |
| } |
| +// ErrRecordMalformed: a store record was returned by the store and does not |
| +// decode. It separates "the bytes are there and are not a record", which |
| +// will be so on every reading, from a reading the store failed to return. |
| +var ErrRecordMalformed = errors.New("objstore: store record does not decode") |
| + |
| // Inspect reads the record as it is, coherent or not. It is for the one |
| // thing that has to see a record nothing may act on: deciding who holds a |
| // bucket (a record that names a bucket holds it, whatever else is wrong |
| @@ -252,7 +257,10 @@ func Inspect(ctx context.Context, s release.Store, acct, app string) (*Record, s |
| } |
| var rec Record |
| if err := json.Unmarshal(raw, &rec); err != nil { |
| - return nil, "", fmt.Errorf("objstore: %s: %w", Key(acct, app), err) |
| + // Whatever the decoder's complaint (syntax, a wrong type, a time |
| + // that is not a time), the bytes were returned and are not a |
| + // record. That is so on the next reading too. |
| + return nil, "", fmt.Errorf("%w: %s: %w", ErrRecordMalformed, Key(acct, app), err) |
| } |
| // The mark as an earlier build of this branch wrote it, inside the |
| // grant block: the same evidence, and it is not dropped. |