amadan
The storage pass compares each bucket's ledger generation with the inventory's, and a failed reading no longer loses the day
From the second review of this branch (Astra reject, Opus accept with fixes): - The inventory records each named bucket's ledger generation. A store is sized only if that generation is unchanged when its turn comes and when the sizing is done, so a holder that came and went between the inventory and the store's turn is seen. - A ledger that will not parse holds the stores on its bucket at once. A reading the store failed to return fails the pass, for a retry, until the day's last hour; then the day is written with the affected stores held, each for its own account, so a failure that lasts the day does not hold storage for every account. Each check has a reproduction and was mutation-tested in a scratch copy. Not covered, as before: one bucket adopted in two deployments. Billing is off.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+623
−53
docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review2.md+100 −0docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md+15 −0docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review2.md+106 −0implementation/carloku-paid-launch/OUTSTANDING.md+1 −1internal/console/economicspass.go+96 −44internal/console/economicspass_test.go+246 −0internal/objstore/walk.go+59 −8
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review2.md b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review2.md| new file mode 100644 |
| index 000000000..8ffda0ada |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review2.md |
| @@ -0,0 +1,100 @@ |
| +All locations refer to `55c11cf3`. Conclusions are from source inspection of both requested diffs and their surrounding code. No tests or network commands were run. |
| + |
| +1. **P1 — The ledger check does not cover changes between the inventory and its first ledger read.** |
| + `internal/console/economicspass.go:190`, `:255`, `:283`; `internal/objstore/walk.go:1777`. |
| + |
| + A holds bucket X. The inventory reads B’s record before B has a grant. Before A reaches its first `OpenOnLedger`, an operator grants X to B, B writes objects, and B’s teardown revokes its credentials and closes its interval while retaining its record. |
| + |
| + A’s first ledger read now sees B’s **closed** interval, so `onLedger` excludes B. The cached inventory also excludes B. Nothing changes during sizing: A’s record and both ledger generations match. The pass records X’s full size against A, including B’s leftovers, although B’s retained record should hold that measurement. |
| + |
| + There is another uncovered path: Grant writes its grant block at `walk.go:349`, **before** opening the ledger at `:452`. A grant that writes that block after the inventory and fails before opening its interval changes the holder records without changing the ledger. The new generation check cannot detect it. |
| + |
| + **Smallest acceptable fix:** validate holder evidence across the inventory **and** sizing, including grant-block writes before ledger opening. A shared holder revision covering those mutations would work; alternatively, refresh the relevant holder evidence under bucket coordination and keep that protection through measurement. Merely comparing the ledger immediately before and after sizing is insufficient. |
| + |
| +2. **P1 — Cross-deployment adoption still permits both accounts to receive the whole bucket’s bytes. Accepted previously, still unfixed.** |
| + `internal/objstore/walk.go:1676`, `:1770`; `internal/console/economicspass.go:266`. |
| + |
| + Deployment D1 records A holding X; D2 records B holding the same physical bucket. Each inventory and ledger lookup uses only its supplied release store. Both deployments find a sole local holder and record all of X. |
| + |
| + A different **minted bucket-name prefix** is not itself missed. The failure is separate control-store namespaces or deployment prefixes. |
| + |
| + **Smallest acceptable fix:** require shared ownership evidence or verified exclusive deployment ownership before measuring an adopted bucket. Until that exists, hold adopted buckets whose exclusivity cannot be established. `OUTSTANDING.md:68` correctly acknowledges this gap; documenting it does not close it. |
| + |
| +3. **P2 — Failing the whole pass can permanently remove a storage source day from every account.** |
| + `internal/console/economicspass.go:190`, `:259`; scheduler at `:44`, `:60`. |
| + |
| + Suppose the console starts at 23:30 UTC with today’s storage day absent. One inventory GET fails transiently. The pass returns without writing the day. At 00:30 the error has cleared, but the next tick collects the **new** date. There is no retry of the missed date. |
| + |
| + A permanently malformed ledger is worse: `ReadBucketOwnership` rejects it, and the first ledger check aborts the entire deployment’s storage pass every day. The affected bucket is known, yet unrelated accounts lose their measurements too. |
| + |
| + An absent day does not increment shared `SourceDays[SourceStorage]` (`economicspass.go:924`, `:1002`). Consequently **every account**, including accounts without tenant stores, loses that source day. The fold skips its storage values, and `Statement.MeterComplete` remains false for `storage_gb_month` for the month (`internal/usage/statement.go:171`). Repairing the read failure tomorrow does not restore yesterday. |
| + |
| + **Smallest acceptable fix:** preserve successful account measurements and retry unresolved collection without discarding the whole deployment day. Keep bucket-specific failures scoped to affected accounts, and explicitly handle pending collection across UTC rollover. Tomorrow’s live bucket size must not be silently substituted for yesterday’s measurement. |
| + |
| +4. **P2 — A transient failure in the final ledger read still becomes an irreversible held day.** |
| + `internal/console/economicspass.go:283`, `:290`, `:298`. |
| + |
| + The inventory, initial ledger read, sizing, and own-record reread all succeed. The final ledger GET fails once. The combined `gerr != nil || now != generation` condition treats that failure as an ownership change, writes `StoreUnswept`, and commits the create-only day. |
| + |
| + Subsequent ticks return at `:168`, even after the failure clears. This contradicts the disposition’s claim that an unreadable ledger fails the pass and permits retry. The own-record reads at `:223` and `:272` also retain the older commit-a-hold behavior for transient errors. |
| + |
| + **Smallest acceptable fix:** distinguish read failures from observed ownership changes. Retry failed reads without committing a permanent hold, together with the day-preservation correction above. Add separate failure cases for the initial and final ledger reads. |
| + |
| +**Disposition checks.** “Would pass” below is a source-derived counterfactual, not a reported test run. |
| + |
| +| Disposition row | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| Holder appears and leaves during sizing | **Closed for the exact tested window; incomplete overall.** The inventory-to-ledger gap remains. | **No** if the generation comparison is removed. The test at `economicspass_test.go:1904` changes the ledger inside `storeSizer`; it never exercises the earlier gap or an actual Grant’s record-before-ledger ordering. | |
| +| Repeated inventory reads and transient read errors | **Partly.** Repeated remote inventory reads are fixed. Unbounded work and permanent read-failure consequences remain. | There is **no request-count or transient-read-error test**. The listed behavioral cases cannot establish either property. | |
| +| Invalid account/app names skipped | **Closed for storage’s inventory.** Literal keys are read. Grant’s older scan remains unchanged. | **No** for the invalid-account test at `:1864` if invalid names are skipped again. It does not separately exercise an invalid app name. | |
| +| Unparseable record holds unrelated stores | **Not removed.** It is now an explicit, logged global hold. | **Yes against `d5464eb1`**: its inventory error already held both stores. **No** if only the current `Unparsed` guard is removed, or against the base with no holder check. The test does not assert the logging change. | |
| +| Departed store’s leftovers billed to remaining holder | **Closed for an already-present retained record.** The timing case in finding 1 remains. | **No.** The revised test at `:1806` now requires the remaining store to be held. Its release half deletes both the departed record and the day object; it does not demonstrate repair of an existing create-only day. | |
| +| Permanent holds omitted from 19i | **Documentation largely corrected; holds remain.** Further shapes are noted below. | No reproduction test is attached to this row. | |
| +| Cross-deployment holder invisible | **Open, explicitly accepted rather than fixed.** | No test. | |
| +| Tests strengthened | **Yes, with limitations.** Three new subtests and an expanded departed-store subtest. | “A sole holder” still passes with the entire holder check removed; it is a positive control. Shared-record, ledger-only, and revised departed-record cases fail without that check. | |
| + |
| +The existing `TestUnsweptStorageHoldsOnlyItsOwnAccount` at `economicspass_test.go:1572` constructs storage-day objects directly. It verifies the downstream fold, but would still pass if collection discarded every account’s day as in finding 3. |
| + |
| +**Holders and persistent holds.** In a stable inventory, grant-only records are included (`walk.go:1740`). Invalid account/app paths and `Coherent`-refused records contribute every bucket named in their provisioned, grant, and teardown blocks (`:1716`). Thus a bucket named only in a refused teardown note is covered. A coherent record with its own bucket-deletion timestamp is excluded (`:1733`). |
| + |
| +The following holds have no automatic expiry: |
| + |
| +| Shape | What releases future measurements? | Does 19i disclose it? | |
| +|---|---|---| |
| +| Retained departed record, with a closed ledger interval or no ledger | Completion/removal of the retained record. Repeating teardown while another foreign holder remains need not remove it. | Yes. | |
| +| Record refused by `Coherent` | Manual repair establishing what it holds. Closed intervals do not excuse it. | Yes, through 19g. | |
| +| Another store’s unfinished grant block | Successful repair or teardown of that estate. | Yes. | |
| +| Two apps of one account sharing a bucket | Ending the sharing. Both currently hold despite sharing an account. | Yes. | |
| +| Unparseable record | Repair; otherwise every eligible provisioned store is held daily. | Yes. | |
| +| Record under an invalid account/app name | Manual repair/removal; normal record APIs reject those names. | The skipped-walk limitation is mentioned, but this storage hold’s release condition is not explicit. | |
| +| Open ledger interval with no corresponding record | Explicit ledger reconciliation. `Teardown` returns immediately when the record is absent (`walk.go:910`). | Not explicitly listed. | |
| + |
| +These are conservative holds, not proof that the remaining store’s bytes are attributable. Already-written held days remain held even after the underlying condition is repaired. |
| + |
| +**Grant’s take-back check is behaviorally unchanged for reachable inputs.** Comparing the removed block against `walk.go:303` and `:1624`: |
| + |
| +| Step | Before → after | |
| +|---|---| |
| +| Entry | Same unfinished-teardown and non-operator condition. | |
| +| Record candidates | Same `otherStoreHolders`, traversal order, name exclusions, teardown handling, and refused-record handling. | |
| +| Reads | Complete record scan first, ledger second. Inventory failure prevents the ledger read. | |
| +| Reconciliation | `ledger.left` filtering moves from Grant into `OtherHolders`; refused records remain exempt. | |
| +| Ledger candidates | Previously all open foreign-account intervals; now other-app intervals are included and deduplicated. Grant still skips its own account. | |
| +| First reported holder | Surviving record holders precede ledger-only holders. Deduplication preserves the first foreign holder reported. An open interval cannot satisfy `left`. | |
| +| Errors | Inventory and ledger wrappers retain their text and `%w`; `ErrBucketHeld` retains its message. | |
| +| Invalid bucket names | The helper skips their ledger read, but Grant already rejects them at `walk.go:222`, so this changes no reachable take-back result. | |
| + |
| +**Cost.** For N valid apps, each with an eligible store, the earlier implementation made N(N−1) other-record GET attempts per pass. The current inventory makes **N** record GET attempts, including absent-record attempts for apps without stores. The main pass separately reads each valid app’s record and rereads successfully sized stores: at most roughly **3N record GETs**, plus up to **2N ledger GETs**, directory listings, and object listings. |
| + |
| +There is still no app-count budget, inventory-size limit, or pass-specific deadline. The hourly interval is not a timeout. Moreover, duplicate checking in `ReadHolderInventory` (`walk.go:1709`) and enumeration in `Others` (`:1750`) retain worst-case **O(N²) in-memory work** when N records name one bucket. The remote quadratic scan is fixed; “bounded” would still be inaccurate. |
| + |
| +**Claims the code contradicts:** |
| + |
| +- **`OUTSTANDING.md:68`: “sizes a store only when it is the sole holder.”** Findings 1 and 2 prevent that guarantee. |
| +- **`DISPOSITIONS-storage-sole-holder.md:12`: an unreadable ledger fails the pass and retries.** The final ledger read commits a hold instead; an initial failure can lose the date permanently. |
| +- **`economicspass.go:187`: “The pass runs again on the next tick.”** It runs for the next tick’s date, which need not be the failed date. |
| +- **`economicspass_test.go:1721`: a departed store “does not count as holding it.”** The implementation and revised departed-store test now require the opposite while its retained record stands. |
| +- **`walk.go:1622`: `OtherHolders` “reads every store record in the deployment.”** Its underlying scan still skips invalid account/app names. |
| +- **`DISPOSITIONS-storage-sole-holder.md:18`: “four cases added.”** The incremental diff adds three subtests and expands one existing subtest. |
| + |
| +**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 03761816a..65e5c68b7 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| @@ -18,3 +18,18 @@ Reports: `CODEX-FINDINGS-storage-sole-holder-review1.md`, `OPUS-REVIEW-storage-s |
| | Tests | The departed-store case now asserts the hold and the release once the record is gone; four cases added. | `economicspass_test.go` | above | |
| Mutation checks in a scratch copy, each failing its test: the unparsed-record hold off; the ledger generation not compared; invalid names skipped; a record with a teardown block not counted; ledger-only holders ignored. |
| + |
| +## Review two (reviewed `55c11cf3`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-storage-sole-holder-review2.md`, `OPUS-REVIEW-storage-sole-holder-review2.md`. Both found the first review's findings closed for the windows they named, and Grant's take-back check unchanged. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Both: a store granted the bucket, writing to it and leaving again between the inventory and the bucket's own store's turn is in neither the inventory nor the ledger's open intervals | Fixed. The inventory records each named bucket's ledger generation when it is read. A store is sized only if its bucket's generation is still that one when its turn comes, and again when the sizing is done. No clocks are compared. | `objstore/walk.go` `ReadHolderInventory`, `Generation`; `console/economicspass.go` `sizeSoleHolder` | `TestStoragePassSeesAHolderThatCameAndWentBeforeAStoresTurn` | |
| +| Both: failing the pass on a read error can leave the day unwritten for good, which holds storage for every account; and a ledger that will not parse did the same every day | Fixed. A ledger that is there and will not parse or validate holds the stores on its bucket at once and the pass goes on. A reading the store failed to return fails the pass, for a retry, until the day's last hour (23:00 UTC); then the day is written with the stores it cannot vouch for held, each for its own account. A failed ledger reading concerns that bucket's stores only; a failed record reading, every store. | `storagePass`, `storageLastChance`, `sizeSoleHolder`; `OpenOnLedger`, `ErrLedgerMalformed` | `TestStoragePassRetriesAFailedReadingUntilTheDaysLastHour`, `TestStoragePassHoldsTheStoresOfAMalformedLedger` | |
| +| Astra P2: a transient failure of the final ledger read became an irreversible held day | Fixed by the same rule: mid-day it fails the pass for a retry. | `sizeSoleHolder` | the same | |
| +| Astra P1: a holder under another deployment's control prefix is invisible | Accepted, not fixed, as before. It cannot be closed from inside one deployment. Open in 19i. | none | none | |
| +| Opus P3-4: no case for a refused record naming the bucket through its grant block or its teardown note, for a grant-only record, or for the deleted-bucket skip; none for a failing read | Added. | `economicspass_test.go` | `TestHolderInventoryRules`, and the two tests above | |
| +| Opus P3-3, P3-5: holds that last until a person acts; cost | 19i lists the holds. Cost is one inventory per pass plus two ledger reads per store; a pass that keeps failing repeats its sizing each tick until the last hour. Noted in 19i. | `OUTSTANDING.md` | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the generation not compared before sizing; the pass never writing in the last hour; the pass never failing mid-day; a malformed ledger treated as a failed reading; each of the three inventory rules removed; the teardown-note bucket not counted. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review2.md b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review2.md| new file mode 100644 |
| index 000000000..6afc75bec |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review2.md |
| @@ -0,0 +1,106 @@ |
| +# Second review: `feat/carloku-storage-sole-holder-20261001` at `55c11cf3` |
| + |
| +## Dispositions, row by row |
| + |
| +| Row | Closed? | Would its test pass without the fix? | |
| +| --- | --- | --- | |
| +| A holder appears while the bucket is being sized | **Closed for the reported shape.** The holder opens and closes its interval inside `sizeStore`, and the ledger ETag check at `economicspass.go:283` catches it. One neighbouring window is still open (finding 2). | No. The record re-read passes (`SameStore`), and only the generation compare holds the store. | |
| +| One inventory per pass; a transient error held a day for good | **Closed** for the O(N²) cost and the transient hold. It now fails the whole pass instead, which brings a new problem (finding 1). | No test exists. The disposition says so. | |
| +| Astra P1: a record under a name no build writes | **Closed for the storage pass.** `ReadHolderInventory` reads literal keys. The walks' `storeHolders` still skips such names; the disposition and 19i say so. | No. Skipping invalid names would size `mine`. | |
| +| Astra P2: a malformed record held every store | **Changed as described.** `Unparsed` holds every provisioned store, logs once and is listed in 19i. | No, for `mine`. `theirs` would be held anyway through the existing `gerr` branch. | |
| +| Opus P2: a store that left was not counted | **Changed to hold.** A coherent teardown record without `BucketDeletedAt` counts. Approval K exists in the approvals table. | No. Both halves (hold, then release once the record is gone) would fail. | |
| +| Holds that last for good, listed in 19i | **Closed.** One gap remains (finding 5). | Not applicable | |
| +| A holder under another deployment prefix | **Accepted and open,** stated in 19i. It is true: `prefixStore` scopes `ListDirs`. | None | |
| +| Tests | Partly closed (finding 4). | See finding 4 | |
| + |
| +## Findings |
| + |
| +### P2-1. A read failure that never clears loses the whole deployment's storage day, every day, and breaks the per-account ruling |
| + |
| +- **Where:** `internal/console/economicspass.go:255-260` (also `:190-193`, and `objstore.ReadHolderInventory` at `walk.go` in the `s.Get` error return). |
| +- **Scenario:** |
| + - One bucket's ledger fails `json.Unmarshal` or `validate(bucket)`. Example: its `Bucket` field does not match, or a by-hand edit left it malformed. `ReadBucketOwnership` returns an error. `OpenOnLedger` passes it on, and `storagePass` returns before `CreateDayObject`. |
| + - The same happens when the store returns a lasting error for one `store.json` (AccessDenied on that object, for example). |
| + - The pass runs with `date = usage.DayOf(now)` (`economicspass.go:60`), so day D is only ever tried during day D. If the fault outlasts the day, `usage/storage/D.json` is never written. |
| + - In the fold, `readDayStorage` then returns neither `Decoded` nor `Failed`, so `storageOK` is false for **every account**. No account's `SourceStorage` counts that day, and every account's `storage_gb_month` is incomplete for the month. This repeats daily until someone repairs the ledger. |
| + - It is safe in direction (it holds, never over-bills). But it is exactly what round sixteen ruled out: one store's problem holding every account. |
| + - Each failed tick also repeats every `ListSizes` and every whole-bucket sizing up to the failing store, up to 24 times a day. |
| +- **Your direct question:** yes. A failed pass can leave the day unwritten for good. The trigger is any failure that lasts past the day's last tick, or a transient failure that hits every tick of that day. Every account then loses that storage source day. |
| +- **Smallest fix:** |
| + - For a ledger that will not parse or validate, hold that one store (`StoreUnswept`) and continue. Every other holder of that bucket reads the same ledger and is held too, and holders that have records are already in the inventory. |
| + - Fail the pass only on transport errors. |
| + - Apply the same split to the inventory. A key that returns a lasting non-NotExist error can be treated like `Unparsed` after N ticks, or the day written with `StoreUnswept` set for every provisioned store, which is per-account and recoverable. |
| + - Say in 19i and in the comments that a pass failed for a whole day is never retried for that day. |
| + |
| +### P2-2. A store that is granted the bucket, writes to it and leaves in the window between the inventory and this store's first ledger read is missed, and its leftover bytes are sized to this store |
| + |
| +- **Where:** `internal/console/economicspass.go:190` (inventory at pass start), `:255` (`OpenOnLedger` before sizing, open intervals only), `objstore.OpenOnLedger`. |
| +- **Scenario:** |
| + 1. The inventory is read at T0. |
| + 2. The pass then sizes stores one after another, each a whole-bucket listing, so store X's turn can come much later. |
| + 3. In between, an operator grants X's bucket B to Y of another account. Y's record and its open interval are written after T0. Y writes objects. |
| + 4. Y's teardown is retained, because X holds B. It closes Y's interval and keeps Y's record. |
| + 5. X's first `OpenOnLedger` sees no open interval. Y's record is not in the T0 inventory. The generation is read after Y left, so it compares equal. |
| + 6. X is sized with Y's leftover objects. |
| + |
| + This is the departed-store shape the branch holds on in every other window. The first review's finding is closed only for the sizing window itself. |
| +- **Smallest fix:** keep the inventory read instant T0. In `OpenOnLedger`, treat any other app's interval with `Until == nil || !Until.Before(T0)` as a holder. Alternatively, read the inventory again after sizing and compare. |
| + |
| +### P3-3. Holds that last until a person acts |
| + |
| +19i lists most of them correctly: |
| + |
| +- a retained departed record (19a); |
| +- an incoherent record (19g); |
| +- a grant block with no completed grant; |
| +- two apps of one account on one bucket; |
| +- any record that will not parse. |
| + |
| +Not listed: |
| + |
| +- A record under an invalid account or app name holds until it is repaired by hand. The inventory marks it `refused`, but 19i's wording ("a record no walk can follow") reads as `Coherent` only. |
| +- The whole-deployment missing day from finding 1. That is not a hold, but it is worse than one. |
| + |
| +**Fix:** add both to 19i. |
| + |
| +### P3-4. Tests that would still pass with the code they cover removed |
| + |
| +`economicspass_test.go`, in `TestStoragePassSizesOnlyASoleHolderOfItsBucket`. No case covers: |
| + |
| +- An incoherent record whose `Grant.Bucket` or `Teardown.DeletedBucket` names the shared bucket while its `Provisioned` names another. Removing `add(rec.Grant.Bucket, true)` or `add(rec.Teardown.DeletedBucket, true)` fails nothing. |
| +- A record with only a grant block. Removing `case rec.Grant != nil` fails nothing. |
| +- The `BucketDeletedAt` skip. Counting a deleted-bucket record, which would hold for ever, fails nothing. |
| +- The pass failing on a read error, or on an `OpenOnLedger` error. |
| + |
| +**Fix:** one case each. A store wrapper that returns an error for one key covers the last. |
| + |
| +### P3-5. Cost |
| + |
| +- **The inventory** is 1 + A `ListDirs` + R `Get`s per pass, where A is account directories and R is app directories. Every app without a store is a 404 GET. |
| +- **Each provisioned store (P of them)** adds 2 ledger GETs and 2 record GETs (one of the record GETs existed before). |
| +- **Per successful pass that is about 1 + A + R + 4P requests: linear in N, not N².** The create-only day object bounds it to once a day. |
| +- **Nothing bounds a failing pass.** It repeats hourly, redoing all sizing up to the failure point (finding 1), and has no deadline. |
| +- **Grant's take-back** stays at O(R) per call, the same as before. |
| + |
| +**Fix:** none needed beyond finding 1. Optionally read every needed ledger before any sizing so failures come early and cheaply. |
| + |
| +## Grant's take-back check: unchanged |
| + |
| +Compared line by line (`walk.go:300-313` before and after): |
| + |
| +- **Holders considered:** records via `otherStoreHolders` (`countLeaving=true`, invalid names skipped), then the ledger's open intervals. Same in both versions. |
| +- **The `ValidBucketName` guard:** `OtherHolders` only reads the ledger when the name is valid, where the old code always read it. Grant has already refused invalid names at `walk.go:222`, so for Grant this branch is never taken. |
| +- **Old filter:** open intervals with `iv.Account != acct`. **New filter:** everything except exactly `acct/app`, deduplicated by `seen`. The extra same-account entries are skipped by the loop's `h.Account == acct`. Deduplication cannot change which holder comes first: a record holder skipped as `left` has no open interval, and a kept holder appears first in both versions. |
| +- **Reconciliation:** `!h.refused && ledger != nil && ledger.left(...)`, applied only to record holders in both versions. Ledger holders cannot be `left`. |
| +- **Order:** records, then the ledger. The first other-account holder named in `ErrBucketHeld` is the same. |
| +- **Errors:** both wrappers ("list the stores holding %s", "read ownership of %s") and their order are identical. Grant now returns them unwrapped, as `OtherHolders` builds them. |
| + |
| +## Claims the code contradicts |
| + |
| +1. `economicspass.go:184-189` and `:257-258` say "the pass runs again on the next tick" and "not a hold for this one store". The pass is only retried within the same UTC day. For a ledger that will not parse, the failure is permanent and withholds every account's day (finding 1). |
| +2. `economicspass_test.go`, the comment above `TestStoragePassSizesOnlyASoleHolderOfItsBucket`, says "a store that left the bucket … does not count as holding it". The test and the code do the opposite: it counts until its record is gone. |
| +3. The `OtherHolders` doc comment in `walk.go` says "It reads every store record in the deployment". `storeHolders` skips invalid account and app names. The disposition admits this; the comment does not. |
| +4. The `OpenOnLedger` doc comment says "an unchanged generation means nobody was given the bucket in between". That holds only between the two reads. It is silent on grants between the inventory and the first read, and the code relies on that window being covered (finding 2). |
| +5. 19i says the holds that last until a person acts are fully listed. It leaves out invalid-name records and the unwritten-day case (finding 3). |
| + |
| +**Verdict: accept with fixes.** Findings 1 and 2 are needed before 19i can be called closed. Findings 3 to 5 are documentation and tests. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index 1deb617bc..0e953ff61 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. Reviewed once (`DISPOSITIONS-storage-sole-holder.md`); the fixes since 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 reading the store fails to return 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 its sizing each tick until then. Reviewed twice (`DISPOSITIONS-storage-sole-holder.md`): Astra rejected both times, Opus accepted with fixes both times; the fixes after the second 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 cf8dd9f03..96c68acff 100644 |
| --- a/internal/console/economicspass.go |
| +++ b/internal/console/economicspass.go |
| @@ -181,19 +181,35 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| if err != nil { |
| return err |
| } |
| - // Who holds which bucket, read once for the whole pass. A record the |
| - // store could not return fails the pass before the day is written: the |
| - // day is create-only, and a store held on a reading that failed would |
| - // stay held. The pass runs again on the next tick. A record that is |
| - // there and will not parse is different: it will not parse next tick |
| - // either, so the day is written with every store held (below). |
| + // Who holds which bucket, read once for the whole pass, with each |
| + // bucket's ledger generation at that moment. |
| + // |
| + // A reading the store failed to return is tried again on the next tick: |
| + // the pass fails before the day is written, because the day is |
| + // create-only and a store held on a failed reading would stay held. |
| + // But a day is only ever written during that day, and a failure that |
| + // lasted through it would leave no storage day at all, which holds |
| + // storage for EVERY account. So in the day's last hour the pass stops |
| + // waiting: it writes the day, with every store it could not vouch for |
| + // held, each for its own account. |
| + // |
| + // A record or a ledger that is there and will not parse is different |
| + // from the start: it will not parse next tick either. It holds what it |
| + // makes uncertain, at once: a record, every store (nobody can say which |
| + // bucket it names); a ledger, the stores on that bucket. |
| + lastChance := storageLastChance(date) |
| inventory, err := objstore.ReadHolderInventory(ctx, s.cfg.Releases) |
| if err != nil { |
| - return fmt.Errorf("storage pass: %w", err) |
| + if !lastChance { |
| + return fmt.Errorf("storage pass: %w", err) |
| + } |
| + log.Printf("console: storage pass: %v; the day is ending, so it is written with every store held", err) |
| + inventory = nil |
| } |
| - if len(inventory.Unparsed) > 0 { |
| + if inventory != nil && len(inventory.Unparsed) > 0 { |
| log.Printf("console: storage pass: store record(s) that will not parse: %v; no store can be shown to be the only holder of its bucket, so every store is held today", inventory.Unparsed) |
| } |
| + holdEveryStore := inventory == nil || len(inventory.Unparsed) > 0 |
| day := usage.StorageDay{V: 1, Date: date, Apps: map[string]usage.AppStorage{}} |
| for _, acctPrefix := range acctPrefixes { |
| acct := strings.TrimSuffix(strings.TrimPrefix(acctPrefix, "apps/"), "/") |
| @@ -252,44 +268,13 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| // counts until then: its objects may still be in the |
| // bucket, and whose those bytes are has not been decided. |
| bucket := rec.Provisioned.Bucket |
| - onLedger, generation, lerr := objstore.OpenOnLedger(ctx, s.cfg.Releases, bucket, acct, app) |
| - if lerr != nil { |
| - // Not a hold for this one store: the day is not written |
| - // on a reading that failed, and the pass runs again. |
| - return fmt.Errorf("storage pass: %s/%s: %w", acct, app, lerr) |
| - } |
| - if len(inventory.Unparsed) > 0 { |
| - // Some store record in the deployment will not parse, |
| - // so which bucket it names is unknown, and no store |
| - // can be shown to be the only holder of its bucket. |
| - st.StoreUnswept = true |
| - } else if others := len(inventory.Others(bucket, acct, app)) + len(onLedger); others > 0 { |
| - log.Printf("console: storage pass: %s/%s shares its bucket with %d other store(s); held", acct, app, others) |
| - st.StoreUnswept = true |
| - } else if n, serr := s.sizeStore(ctx, rec.Provisioned); serr != nil { |
| - log.Printf("console: storage pass: store %s/%s: %v", acct, app, serr) |
| - st.StoreUnswept = true |
| - } else if after, _, aerr := objstore.Get(ctx, s.cfg.Releases, acct, app); aerr != nil || |
| - after == nil || after.Teardown != nil || !objstore.SameStore(rec, after) { |
| - // Sizing lists the whole bucket and takes time. If the |
| - // record read afterwards is not the same store (a |
| - // teardown began; the store was torn down and made |
| - // again, or taken back, under the same bucket name; |
| - // its key was rotated; it is gone or unreadable), what |
| - // was listed may be another account's objects: the |
| - // size is discarded and the day held for this account. |
| - log.Printf("console: storage pass: store %s/%s changed while it was being sized; held", acct, app) |
| + if holdEveryStore { |
| st.StoreUnswept = true |
| - } else if _, now, gerr := objstore.OpenOnLedger(ctx, s.cfg.Releases, bucket, acct, app); gerr != nil || now != generation { |
| - // The bucket's ledger moved while it was being sized: a |
| - // grant opens its interval there before it delivers a |
| - // credential, so somebody may have been given the |
| - // bucket, and may have written to it, in between (and |
| - // may since have left). The size is discarded. |
| - log.Printf("console: storage pass: ownership of %s/%s's bucket changed while it was being sized; held", acct, app) |
| + } else if held, herr := s.sizeSoleHolder(ctx, inventory, rec, acct, app, bucket, lastChance, &st); herr != nil { |
| + return herr |
| + } else if held != "" { |
| + log.Printf("console: storage pass: %s/%s: %s; held", acct, app, held) |
| st.StoreUnswept = true |
| - } else { |
| - st.StoreBytes = n |
| } |
| } |
| day.Apps[acct+"/"+app] = st |
| @@ -2454,3 +2439,70 @@ func (s *Server) handleAPIEconomicsBackhaul(w http.ResponseWriter, r *http.Reque |
| } |
| writeJSON(w, http.StatusOK, buildBackhaulReport(v)) |
| } |
| + |
| +// storageClock is the storage pass's clock, a seam for tests. |
| +var storageClock = time.Now |
| + |
| +// storageLastChance reports that this is the last hour in which date's |
| +// storage day can be written (the pass runs only during the day it is |
| +// for), or that the day is already over. |
| +func storageLastChance(date string) bool { |
| + now := storageClock().UTC() |
| + return usage.DayOf(now) != date || now.Hour() >= 23 |
| +} |
| + |
| +// sizeSoleHolder sizes a provisioned store's bucket and records the bytes |
| +// in st, if and only if the store can be shown to be the bucket's only |
| +// holder from the moment the inventory was read until the sizing is done. |
| +// Otherwise it returns why the store is held ("" when it was sized). An |
| +// error is a reading the store failed to return while there is still time |
| +// to try the whole pass again. |
| +func (s *Server) sizeSoleHolder(ctx context.Context, inventory *objstore.HolderInventory, rec *objstore.Record, acct, app, bucket string, lastChance bool, st *usage.AppStorage) (held string, err error) { |
| + // unreadable turns a ledger reading that failed into a hold or, with |
| + // time left in the day, into a retry of the pass. |
| + unreadable := func(lerr error) (string, error) { |
| + if errors.Is(lerr, objstore.ErrLedgerMalformed) || lastChance { |
| + return "the ownership ledger of its bucket could not be read: " + lerr.Error(), nil |
| + } |
| + return "", fmt.Errorf("storage pass: %s/%s: %w", acct, app, lerr) |
| + } |
| + if inventory.LedgerUnread(bucket) { |
| + return unreadable(errors.New("the store did not return it when the pass began")) |
| + } |
| + onLedger, generation, lerr := objstore.OpenOnLedger(ctx, s.cfg.Releases, bucket, acct, app) |
| + if lerr != nil { |
| + return unreadable(lerr) |
| + } |
| + // Nobody was given the bucket between the inventory and now. A grant |
| + // opens its interval on the ledger before it delivers a credential, so |
| + // a store that was granted the bucket, wrote to it and left again in |
| + // between has still moved the generation. |
| + if generation != inventory.Generation(bucket) { |
| + return "ownership of its bucket changed since the pass began", nil |
| + } |
| + if others := len(inventory.Others(bucket, acct, app)) + len(onLedger); others > 0 { |
| + return fmt.Sprintf("it shares its bucket with %d other store(s)", others), nil |
| + } |
| + n, serr := s.sizeStore(ctx, rec.Provisioned) |
| + if serr != nil { |
| + return "its bucket could not be sized: " + serr.Error(), nil |
| + } |
| + // Sizing lists the whole bucket and takes time. If the record read |
| + // afterwards is not the same store (a teardown began; the store was |
| + // torn down and made again, or taken back, under the same bucket name; |
| + // its key was rotated; it is gone or unreadable), what was listed may |
| + // be another account's objects: the size is discarded. |
| + if after, _, aerr := objstore.Get(ctx, s.cfg.Releases, acct, app); aerr != nil || after == nil || after.Teardown != nil || !objstore.SameStore(rec, after) { |
| + return "the store changed while it was being sized", nil |
| + } |
| + // And nobody was given the bucket while it was being sized. |
| + _, now, gerr := objstore.OpenOnLedger(ctx, s.cfg.Releases, bucket, acct, app) |
| + if gerr != nil { |
| + return unreadable(gerr) |
| + } |
| + if now != generation { |
| + return "ownership of its bucket changed while it was being sized", nil |
| + } |
| + st.StoreBytes = n |
| + return "", nil |
| +} |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 875b858c9..1a208a1fe 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -1955,3 +1955,249 @@ func TestStoragePassSizesOnlyASoleHolderOfItsBucket(t *testing.T) { |
| } |
| }) |
| } |
| + |
| +// sizedFailingGets is failingGets that still answers sized listings, which |
| +// the storage pass needs from its store. |
| +type sizedFailingGets struct{ failingGets } |
| + |
| +func (f sizedFailingGets) ListSizes(ctx context.Context, prefix string) ([]release.KeySize, error) { |
| + return f.Store.(release.SizeLister).ListSizes(ctx, prefix) |
| +} |
| + |
| +func storageDay(t *testing.T, fx *consoleFixture, date string) (usage.StorageDay, bool) { |
| + t.Helper() |
| + raw, _, err := fx.rel.Get(context.Background(), usage.StorageDayKey(date)) |
| + if errors.Is(err, os.ErrNotExist) { |
| + return usage.StorageDay{}, false |
| + } |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var day usage.StorageDay |
| + if err := json.Unmarshal(raw, &day); err != nil { |
| + t.Fatal(err) |
| + } |
| + return day, true |
| +} |
| + |
| +// A reading the store fails to return (a store record, or a bucket's |
| +// ledger) is tried again on the next tick: the pass fails and writes no |
| +// day, so nothing is held on a reading that failed. In the day's last hour |
| +// it stops waiting, because a day that is never written holds storage for |
| +// every account: it writes the day with the stores it cannot vouch for |
| +// held, each for its own account, and sizes the rest. |
| +func TestStoragePassRetriesAFailedReadingUntilTheDaysLastHour(t *testing.T) { |
| + at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| + const mineBucket, theirBucket, other = "carlos-store-mine", "carlos-store-theirs", "bcd" |
| + provisioned := func(bucket string) *objstore.Record { |
| + return &objstore.Record{ |
| + Grant: &objstore.GrantBlock{Backend: "aws", Bucket: bucket, Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Bucket: bucket, Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}, |
| + } |
| + } |
| + prevClock := storageClock |
| + t.Cleanup(func() { storageClock = prevClock }) |
| + mine, theirs := fixtureAcct+"/"+fixtureApp, other+"/shop" |
| + for name, failKey := range map[string]string{ |
| + "a store record": objstore.Key(other, "shop"), |
| + "a bucket's ledger": objstore.BucketOwnershipKey(theirBucket), |
| + } { |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + if _, err := fx.rel.Put(ctx, release.AppPrefix(other, "shop")+"marker", []byte("x"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := objstore.Put(ctx, fx.rel, fixtureAcct, fixtureApp, provisioned(mineBucket), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := objstore.Put(ctx, fx.rel, other, "shop", provisioned(theirBucket), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, o := range []struct{ bucket, acct, app string }{{mineBucket, fixtureAcct, fixtureApp}, {theirBucket, other, "shop"}} { |
| + if err := objstore.OpenBucketOwner(ctx, fx.rel, o.bucket, o.acct, o.app, at, at, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { return 5 << 30, nil } |
| + fx.srv.cfg.Releases = sizedFailingGets{failingGets{Store: fx.rel, fail: failKey}} |
| + |
| + // Mid-day: the pass fails and writes nothing. |
| + storageClock = func() time.Time { return time.Date(2026, 8, 12, 10, 0, 0, 0, time.UTC) } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err == nil { |
| + t.Fatalf("%s: the pass reported success over a reading the store failed to return", name) |
| + } |
| + if _, written := storageDay(t, fx, "2026-08-12"); written { |
| + t.Fatalf("%s: a day was written on a failed reading with the day not over", name) |
| + } |
| + // The last hour: the day is written, held where it must be. |
| + storageClock = func() time.Time { return time.Date(2026, 8, 12, 23, 5, 0, 0, time.UTC) } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Fatalf("%s: the pass in the day's last hour: %v", name, err) |
| + } |
| + day, written := storageDay(t, fx, "2026-08-12") |
| + if !written { |
| + t.Fatalf("%s: no day was written in the last hour", name) |
| + } |
| + if app := day.Apps[theirs]; !app.StoreUnswept || app.StoreBytes != 0 { |
| + t.Errorf("%s: the store whose reading failed: %+v; want held", name, app) |
| + } |
| + if failKey == objstore.BucketOwnershipKey(theirBucket) { |
| + // Only that bucket's stores are in doubt. |
| + if app := day.Apps[mine]; app.StoreUnswept || app.StoreBytes != 5<<30 { |
| + t.Errorf("%s: a store on another bucket: %+v; want measured", name, app) |
| + } |
| + } else if app := day.Apps[mine]; !app.StoreUnswept { |
| + // A store record that cannot be read may name any bucket. |
| + t.Errorf("%s: a store sized while another record could not be read: %+v", name, app) |
| + } |
| + } |
| +} |
| + |
| +// A ledger that is there and will not parse will not parse next tick |
| +// either. The stores on its bucket are held at once, the pass goes on, and |
| +// a store on another bucket is sized. |
| +func TestStoragePassHoldsTheStoresOfAMalformedLedger(t *testing.T) { |
| + at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| + const mineBucket, theirBucket, other = "carlos-store-mine", "carlos-store-theirs", "bcd" |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + prevClock := storageClock |
| + t.Cleanup(func() { storageClock = prevClock }) |
| + storageClock = func() time.Time { return time.Date(2026, 8, 12, 10, 0, 0, 0, time.UTC) } |
| + if _, err := fx.rel.Put(ctx, release.AppPrefix(other, "shop")+"marker", []byte("x"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, o := range []struct{ bucket, acct, app string }{{mineBucket, fixtureAcct, fixtureApp}, {theirBucket, other, "shop"}} { |
| + 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) |
| + } |
| + } |
| + if _, err := fx.rel.Put(ctx, objstore.BucketOwnershipKey(theirBucket), []byte("{not json"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { return 5 << 30, nil } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Fatalf("the pass over a malformed ledger: %v", err) |
| + } |
| + day, written := storageDay(t, fx, "2026-08-12") |
| + if !written { |
| + t.Fatal("no day was written") |
| + } |
| + if app := day.Apps[other+"/shop"]; !app.StoreUnswept || app.StoreBytes != 0 { |
| + t.Errorf("the store on the bucket with the malformed ledger: %+v; want held", app) |
| + } |
| + if app := day.Apps[fixtureAcct+"/"+fixtureApp]; app.StoreUnswept || app.StoreBytes != 5<<30 { |
| + t.Errorf("a store on another bucket: %+v; want measured", app) |
| + } |
| +} |
| + |
| +// The inventory is read once, at the start of the pass, and each store's |
| +// turn comes later. A store that is granted a bucket, writes to it and |
| +// leaves again in between is in neither the inventory nor the ledger's open |
| +// intervals when the bucket's own store is reached. The ledger's generation |
| +// has moved since the inventory, and that holds the store. |
| +func TestStoragePassSeesAHolderThatCameAndWentBeforeAStoresTurn(t *testing.T) { |
| + at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| + const mineBucket, theirBucket, other = "carlos-store-mine", "carlos-store-theirs", "bcd" |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + if _, err := fx.rel.Put(ctx, release.AppPrefix(other, "shop")+"marker", []byte("x"), ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + for _, o := range []struct{ bucket, acct, app string }{{mineBucket, fixtureAcct, fixtureApp}, {theirBucket, other, "shop"}} { |
| + 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) |
| + } |
| + if err := objstore.OpenBucketOwner(ctx, fx.rel, o.bucket, o.acct, o.app, at, at, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + var sized []string |
| + fx.srv.storeSizer = func(_ context.Context, p *objstore.Provisioned) (int64, error) { |
| + sized = append(sized, p.Bucket) |
| + if len(sized) == 1 { |
| + // While the first store is being sized, a third store is |
| + // granted whichever bucket has not had its turn, writes to |
| + // it, and leaves. |
| + next := theirBucket |
| + if p.Bucket == theirBucket { |
| + next = mineBucket |
| + } |
| + if err := objstore.OpenBucketOwner(ctx, fx.rel, next, "bef", "visitor", at.Add(time.Hour), at.Add(time.Hour), false); err != nil { |
| + t.Error(err) |
| + } |
| + if err := objstore.CloseBucketOwner(ctx, fx.rel, next, "bef", "visitor", at.Add(2*time.Hour)); err != nil { |
| + t.Error(err) |
| + } |
| + } |
| + return 5 << 30, nil |
| + } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Fatal(err) |
| + } |
| + day, _ := storageDay(t, fx, "2026-08-12") |
| + if len(sized) != 1 { |
| + t.Fatalf("sized %v; want only the first store's bucket: the second was visited in between", sized) |
| + } |
| + first, second := fixtureAcct+"/"+fixtureApp, other+"/shop" |
| + if sized[0] == theirBucket { |
| + first, second = second, first |
| + } |
| + if app := day.Apps[first]; app.StoreUnswept || app.StoreBytes != 5<<30 { |
| + t.Errorf("the first store: %+v; want measured", app) |
| + } |
| + if app := day.Apps[second]; !app.StoreUnswept || app.StoreBytes != 0 { |
| + t.Errorf("the store whose bucket was visited before its turn: %+v; want held", app) |
| + } |
| +} |
| + |
| +// The inventory's own rules, one case each: a record nothing may act on |
| +// holds every bucket it names; a record with only a grant block holds its |
| +// bucket; a store whose teardown noted its bucket deleted holds nothing. |
| +func TestHolderInventoryRules(t *testing.T) { |
| + at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + put := func(acct, app string, rec *objstore.Record) { |
| + t.Helper() |
| + putRawStore(t, fx, acct, app, rec) |
| + } |
| + grant := func(bucket string) *objstore.GrantBlock { |
| + return &objstore.GrantBlock{Backend: "aws", Bucket: bucket, Source: objstore.GrantSourceOperator, At: at, By: "op"} |
| + } |
| + prov := func(bucket string) *objstore.Provisioned { |
| + return &objstore.Provisioned{Bucket: bucket, Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at} |
| + } |
| + // Two buckets in one record: holds both. |
| + put("bcd", "split", &objstore.Record{Grant: grant("carlos-store-y"), Provisioned: prov("carlos-store-x")}) |
| + // A teardown note about another bucket: holds that one too. |
| + put("bcd", "noted", &objstore.Record{Grant: grant("carlos-store-p"), Provisioned: prov("carlos-store-p"), |
| + Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at, DeletedBucket: "carlos-store-q"}}) |
| + // A grant block and nothing provisioned: a grant that died part way. |
| + put("bcd", "half", &objstore.Record{Grant: grant("carlos-store-g")}) |
| + // Its own bucket noted deleted: holds nothing. |
| + put("bcd", "gone", &objstore.Record{Grant: grant("carlos-store-d"), Provisioned: prov("carlos-store-d"), |
| + Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at, DeletedBucket: "carlos-store-d"}}) |
| + inv, err := objstore.ReadHolderInventory(ctx, fx.rel) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + for bucket, want := range map[string]int{ |
| + "carlos-store-x": 1, "carlos-store-y": 1, |
| + "carlos-store-p": 1, "carlos-store-q": 1, |
| + "carlos-store-g": 1, |
| + "carlos-store-d": 0, |
| + } { |
| + if got := len(inv.Others(bucket, "bab", "asking")); got != want { |
| + t.Errorf("%s: %d holder(s), want %d", bucket, got, want) |
| + } |
| + } |
| +} |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index 36f952476..f4a89df30 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -1666,13 +1666,37 @@ func OtherHolders(ctx context.Context, s release.Store, bucket, acct, app string |
| // store can be shown to be the only holder of anything. |
| type HolderInventory struct { |
| byBucket map[string][]OwnerRef |
| + // generation is each named bucket's ownership ledger generation as it |
| + // stood when the inventory was read ("" for no ledger). |
| + generation map[string]string |
| + // ledgerUnread marks the buckets whose ledger the store failed to |
| + // return when the inventory was read. |
| + ledgerUnread map[string]bool |
| // Unparsed lists the keys of store records that will not parse. |
| Unparsed []string |
| } |
| +// Generation is bucket's ownership ledger generation as the inventory found |
| +// it. A caller that acts on the bucket later reads the ledger again |
| +// (OpenOnLedger) and compares: a grant opens its interval on the ledger |
| +// before it delivers a credential, so an unchanged generation means nobody |
| +// has been given the bucket since the inventory was read, including |
| +// somebody who was given it and has left again. |
| +func (inv *HolderInventory) Generation(bucket string) string { |
| + return inv.generation[bucket] |
| +} |
| + |
| +// LedgerUnread reports that the store failed to return bucket's ledger when |
| +// the inventory was read, so there is no generation to compare against: |
| +// what has happened to the bucket since cannot be shown. It concerns that |
| +// bucket's stores only. |
| +func (inv *HolderInventory) LedgerUnread(bucket string) bool { |
| + return inv.ledgerUnread[bucket] |
| +} |
| + |
| // ReadHolderInventory walks apps/<account>/<app>/store.json. |
| func ReadHolderInventory(ctx context.Context, s release.Store) (*HolderInventory, error) { |
| - inv := &HolderInventory{byBucket: map[string][]OwnerRef{}} |
| + inv := &HolderInventory{byBucket: map[string][]OwnerRef{}, generation: map[string]string{}, ledgerUnread: map[string]bool{}} |
| accounts, err := s.ListDirs(ctx, "apps/") |
| if err != nil { |
| return nil, err |
| @@ -1741,9 +1765,29 @@ func ReadHolderInventory(ctx context.Context, s release.Store) (*HolderInventory |
| } |
| } |
| } |
| + // Each named bucket's ledger generation, now, with the records. |
| + for bucket := range inv.byBucket { |
| + if !ValidBucketName(bucket) { |
| + continue |
| + } |
| + _, etag, err := s.Get(ctx, BucketOwnershipKey(bucket)) |
| + if errors.Is(err, os.ErrNotExist) { |
| + continue |
| + } |
| + if err != nil { |
| + inv.ledgerUnread[bucket] = true |
| + continue |
| + } |
| + inv.generation[bucket] = etag |
| + } |
| return inv, nil |
| } |
| +// ErrLedgerMalformed: a bucket's ownership ledger is there and will not |
| +// parse or validate. It will not parse on the next reading either, which |
| +// is what separates it from a reading the store failed to return. |
| +var ErrLedgerMalformed = errors.New("objstore: ownership ledger will not parse or validate") |
| + |
| // Others names the stores, other than acct/app, whose records name bucket. |
| func (inv *HolderInventory) Others(bucket, acct, app string) []OwnerRef { |
| var out []OwnerRef |
| @@ -1759,20 +1803,27 @@ func (inv *HolderInventory) Others(bucket, acct, app string) []OwnerRef { |
| // OpenOnLedger names the apps, other than acct/app, with an open interval |
| // on bucket's ownership ledger, and returns the ledger's generation as read |
| // ("" when there is no ledger, or the name is one this build would refuse |
| -// and so has none). A caller that measures the bucket afterwards reads the |
| -// generation again: a grant opens its interval on the ledger before it |
| -// delivers a credential, so an unchanged generation means nobody was given |
| -// the bucket in between. |
| +// and so has none). The generation is compared with the one the inventory |
| +// found (HolderInventory.Generation), before a measurement and after it. A |
| +// ledger that will not parse or validate is ErrLedgerMalformed; any other |
| +// error is a reading the store failed to return. |
| func OpenOnLedger(ctx context.Context, s release.Store, bucket, acct, app string) (others []OwnerRef, generation string, err error) { |
| if !ValidBucketName(bucket) { |
| return nil, "", nil |
| } |
| - ledger, etag, err := ReadBucketOwnership(ctx, s, bucket) |
| + raw, etag, err := s.Get(ctx, BucketOwnershipKey(bucket)) |
| + if errors.Is(err, os.ErrNotExist) { |
| + return nil, "", nil |
| + } |
| if err != nil { |
| return nil, "", fmt.Errorf("objstore: read ownership of %s: %w", bucket, err) |
| } |
| - if ledger == nil { |
| - return nil, "", nil |
| + var ledger BucketOwnership |
| + if jerr := json.Unmarshal(raw, &ledger); jerr != nil { |
| + return nil, etag, fmt.Errorf("%w: %s: %v", ErrLedgerMalformed, BucketOwnershipKey(bucket), jerr) |
| + } |
| + if verr := ledger.validate(bucket); verr != nil { |
| + return nil, etag, fmt.Errorf("%w: %v", ErrLedgerMalformed, verr) |
| } |
| for _, iv := range ledger.Intervals { |
| if iv.open() && !(iv.Account == acct && iv.App == app) { |