amadan
The storage pass reads the ledgers before the records, retries every failed reading, and holds a bucket whose ledger it cannot check
From the third review of this branch (Astra reject, Opus accept with fixes): - The inventory lists and reads every ownership ledger first and walks the records after, so a holder that came and went during the walk has moved a generation the baseline already holds. - A failed reading of a store's own record, of its record after the sizing, or of the bucket listing fails the pass for a retry while there is time left in the day, like the ledger readings. Whether there is time left is asked when the failure happens. - A bucket name this build keeps no ledger for is held, not sized. Open, in 19i: the last hour has about one tick, and some failures still leave the day unwritten. Each check has a reproduction and was mutation-tested in a scratch copy. Billing is off.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+505
−34
docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review3.md+127 −0docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md+14 −0docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review3.md+137 −0implementation/carloku-paid-launch/OUTSTANDING.md+1 −1internal/console/economicspass.go+41 −15internal/console/economicspass_test.go+152 −0internal/objstore/walk.go+33 −18
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review3.md b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review3.md| new file mode 100644 |
| index 000000000..1ff0d284e |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review3.md |
| @@ -0,0 +1,127 @@ |
| +Reviewed `e6015870`, including `55c11cf3..e6015870` and the full change from `c68ecddd`. All locations below refer to `e6015870`. Test counterfactuals are from source inspection; no tests or network commands were run. |
| + |
| +1. **P1 — The inventory’s generation baseline is taken too late to cover its record scan.** |
| + `internal/objstore/walk.go:1713`, `:1768`; `internal/console/economicspass.go:2480`. |
| + |
| + A holds bucket X. The inventory scans B before B has a grant. While the inventory continues through other apps, B receives X, writes objects, and tears down. B’s interval closes and its retained record remains. Only afterward does the inventory read X’s ledger generation. |
| + |
| + At A’s turn, the cached records omit B, B’s ledger interval is closed, and all generation comparisons match. A receives X’s full byte count, including B’s leftovers. B’s retained record should have held that measurement. |
| + |
| + A second gap survives even after the inventory finishes: Grant writes its grant block at `walk.go:349` before opening ownership. If it fails between those steps, a new grant-only holder exists without changing the ledger generation. Neither generation comparison detects it. |
| + |
| + **Smallest acceptable fix:** validate holder evidence across the record scan and measurement, including record mutations that precede ledger opening. For example, refresh the holder records under the bucket’s existing walk coordination and retain protection through sizing. Moving the ledger baseline earlier closes the first window but does not close the grant-block window. |
| + |
| +2. **P2 — Transient failures still become irreversible held days before 23:00.** |
| + `internal/console/economicspass.go:239`, `:2486`, `:2495`. |
| + |
| + At 10:00, the inventory succeeds. Any of these subsequent failures still commits `StoreUnswept`: |
| + |
| + - The app’s initial `objstore.Get` fails transiently. |
| + - The bucket listing fails transiently. |
| + - The own-record reread after sizing fails transiently. |
| + |
| + The third case is explicitly reported as “the store changed,” although no change was observed. The day is then created at `:283`; later successful ticks return at `:168` without repairing it. |
| + |
| + These paths predate parts of this change, but remain outside the new retry policy. **The final ledger reread itself is now correctly retried before the cutoff.** |
| + |
| + **Smallest acceptable fix:** distinguish transport failures from malformed/refused records and observed changes on every collection path. Retry transient record and sizing failures before the cutoff; preserve immediate holds for actual ambiguity. |
| + |
| +3. **P2 — The last-hour rule can still leave a day unwritten across midnight.** |
| + `internal/console/economicspass.go:200`, `:2463`; scheduling at `:44`, `:60`. |
| + |
| + `lastChance` is sampled once. Suppose a retry starts at 22:50, spends over an hour listing buckets, and encounters a ledger read failure at 00:01. It still uses `lastChance=false` and aborts. The next invocation computes the new date; yesterday is never retried. |
| + |
| + There are shorter failure paths too: the outer account listing, app listing and deployment-prefix sizing return errors unconditionally at `:182`, `:218` and `:224`, including during the final hour. A failure on the last attempt leaves no day. A missing storage day withholds completeness from every account (`:909`, `:987`). |
| + |
| + **It cannot successfully write the same day twice:** the date/key remain fixed, and `usage.CreateDayObject` uses a create-only conditional write (`internal/usage/statement.go:121`). An in-flight pass can write yesterday’s object after midnight, but that does not overwrite or duplicate it. |
| + |
| + **Smallest acceptable fix:** evaluate the cutoff when handling failures and explicitly retain unfinished day collection across rollover. Bound collection so finalization can occur, and preserve collected evidence rather than silently substituting tomorrow’s sizes for yesterday’s. |
| + |
| +4. **P2 — A refused bucket name bypasses ledger evidence entirely.** |
| + `internal/objstore/walk.go:1770`, `:1811`; `internal/console/economicspass.go:2480`. |
| + |
| + A coherent provisioned record can name a legacy bucket such as `Legacy_Bucket`; `Coherent` does not validate bucket names. The inventory skips its ledger, and both `OpenOnLedger` calls return `("", nil)` without reading anything. Sizing remains allowed. |
| + |
| + If historical or manually repaired ownership evidence exists at `usage/buckets/Legacy_Bucket.json`, including another holder whose record is absent, it is ignored. The comparisons are empty-string comparisons, not observations of that bucket’s ledger. |
| + |
| + **Smallest acceptable fix:** hold names for which ownership evidence cannot be checked, or provide a safe reader for their literal ledger keys. Grant’s refusal to create such evidence is insufficient grounds to assume none exists. |
| + |
| + For comparison, a **valid name with no ledger** does receive actual absence checks: inventory, before sizing and after sizing. A ledger created after the inventory’s absence read changes `""` to an ETag and causes a hold. That path is covered by the code. Creation before the inventory’s late baseline remains subject to finding 1. |
| + |
| +5. **P3 — The retry test does not exercise the final-read fix it is cited as covering.** |
| + `internal/console/economicspass_test.go:1989`, `:2022`; `internal/console/economicspass.go:2499`. |
| + |
| + The failure wrapper rejects the chosen key from its first read. For a ledger failure, the inventory sets `LedgerUnread`, and `sizeSoleHolder` exits at `:2469`. The test never reaches a failing final ledger read. |
| + |
| + Consequently, changing the final-read error handling back to “commit a hold immediately” would leave this regression test passing. The malformed-ledger test at `economicspass_test.go:2059` exercises invalid JSON only; removing ledger validation would also leave that test passing. Neither test advances the clock during collection. |
| + |
| + **Smallest acceptable fix:** inject failures separately at the inventory read, pre-size read, own-record reread and final ledger read. Add a syntactically valid but invalid ledger, and a clock transition through 23:00 and midnight. |
| + |
| +**Disposition checks.** “Without the fix” means removing the relevant protection from the current implementation unless otherwise stated. |
| + |
| +| Review-one row | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| Holder appears and leaves during sizing | Yes for that window; inventory windows remain open. | **No.** Removing the final generation comparison lets the injected size through (`economicspass_test.go:1904`). | |
| +| Repeated inventories and transient failures | Remote quadratic scanning is fixed; transient-failure handling is incomplete. | No request-count test. The newer retry test fails without its inventory-error policy, but misses later failures. | |
| +| Invalid account/app names skipped | Yes for storage’s literal-key inventory. Grant’s scan remains unchanged. | **No** for the invalid-account case at `:1864`. Invalid app names are not separately exercised. | |
| +| Unparseable record holds every store | Changed as documented, not eliminated. | **No** if the current global `Unparsed` hold is removed: the healthy store would be sized. This does not prove the logging behavior. | |
| +| Departed store’s leftovers | Yes for a retained record present when scanned. Finding 1 remains. | **No.** The test requires both the hold and release after record removal (`:1806`). It deletes the day object before retesting; existing held days are not repaired. | |
| +| Permanent holds documented | Partly; omissions and an inaccurate release condition remain below. | No test. | |
| +| Cross-deployment attribution | Accepted and open; not re-reported here. | No test. | |
| +| Tests strengthened | Yes, for the added shapes. | Negative cases fail without their protections. “A sole holder” passes without the holder check; it is a positive control. | |
| + |
| +| Review-two row | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| Holder comes and goes before its store’s turn | Closed **after the inventory finishes**, not during inventory construction or before ledger opening. | **No.** Removing the pre-sizing generation comparison fails `TestStoragePassSeesAHolderThatCameAndWentBeforeAStoresTurn`. | |
| +| Failed readings lose a day; malformed ledger blocks everyone | Partly. Malformed ledgers now hold their own bucket; retry/finalization remains incomplete. | **No** without the tested midday/last-hour policy or malformed-JSON handling. Validation and rollover are untested. | |
| +| Final ledger failure commits a hold | Code fixed for a pass whose cutoff decision is still current. | **Yes.** The cited test fails the inventory read instead; finding 5. | |
| +| Cross-deployment attribution | Accepted and open. | No test. | |
| +| Missing inventory-rule and failure cases | Listed inventory shapes are now covered; failure-stage coverage is incomplete. | **No** when the corresponding grant, teardown-note, grant-only or deleted-bucket rule is removed. Later-read regressions still pass. | |
| +| Permanent holds and cost | Partly documented; details below. | No cost-bound or persistent-hold recovery test. | |
| + |
| +**Persistent holds.** These have no automatic expiry. Repair only enables future days; a day already written as held remains held. |
| + |
| +| Shape | What must change? | Does 19i disclose it? | |
| +|---|---|---| |
| +| Retained departed record, with closed intervals or no ledger | Finish/remove the retained estate when permitted. Repeating teardown while a foreign holder remains can retain it again. | Yes. | |
| +| `Coherent`-refused record | Repair its conflicting bucket evidence. | Yes, through 19g. | |
| +| Grant-only record | Remove the holder or resolve sharing. Rerunning the grant alone can produce another active holder of the same bucket. | Listed, but its stated release condition is insufficient. | |
| +| Two live stores, including two apps of one account | End the sharing. | Yes. | |
| +| Unparseable record | Repair it; otherwise every eligible store is held daily. | Yes. | |
| +| Record under an invalid account/app name | Manual repair/removal; normal record APIs refuse the path. | Not explicitly described as a permanent storage hold. | |
| +| Open ledger interval without a store record | Reconcile the ownership evidence. Teardown returns immediately for a missing record (`walk.go:910`). | No. | |
| +| Malformed ledger | Repair it; its bucket’s stores are held daily. | No explicit entry in 19i’s persistent-hold list. | |
| + |
| +Stable inventories correctly include grant-only records, invalid account/app paths, and every bucket named by a `Coherent`-refused provisioned, grant or teardown block. Coherent records whose own bucket is recorded as deleted are excluded. |
| + |
| +**Grant’s take-back check is behaviorally unchanged for reachable inputs.** |
| + |
| +| Step | Before → after | |
| +|---|---| |
| +| Entry condition | Same non-operator take-back condition. | |
| +| Record holders | Same `otherStoreHolders`, traversal order, invalid-name exclusions and teardown/refused-record rules. | |
| +| Read order | Entire record scan first, ownership ledger second. A scan error prevents the ledger read. | |
| +| Reconciliation | `ledger.left` filtering moves into `OtherHolders`; refused records remain exempt. Filtering same-account records earlier cannot affect Grant’s result because Grant skips that account anyway. | |
| +| Ledger holders | Previously open foreign-account intervals; now other-app intervals, deduplicated. Extra same-account entries are skipped by Grant. | |
| +| First refusal | Surviving record holders still precede ledger-only holders. Deduplication preserves the first foreign holder; an open interval cannot satisfy `left`. | |
| +| Errors | Same scan/read wrappers, `%w`, order and `ErrBucketHeld` message. | |
| +| Invalid bucket names | The helper now skips their ledger read, but Grant already refuses them at `walk.go:222`. No reachable take-back result changes. | |
| + |
| +**Cost.** The original per-store scan would require approximately **N(N−1) other-record GETs** for N apps with stores. This implementation uses one inventory per pass: **N record GET attempts**, plus one generation GET per distinct valid named bucket. |
| + |
| +With N eligible stores on N distinct buckets, collection performs approximately **3N record GETs and 3N ledger GETs**, plus directory and paginated object listings. Successful day creation prevents repeat collection that day. Failed passes repeat work hourly. |
| + |
| +There is no app-count limit, object-count budget or pass-specific deadline. In-memory duplicate checking at `walk.go:1733` and `Others` enumeration at `:1794` also retain worst-case **O(N²)** work when many records name one bucket. The remote quadratic scan is fixed; collection is not bounded. |
| + |
| +**Claims the code contradicts:** |
| + |
| +- `OUTSTANDING.md:68` and `economicspass.go:2454`: sole ownership is not established throughout inventory and sizing; finding 1 supplies a normal-walk counterexample. |
| +- `OUTSTANDING.md:68` and `economicspass.go:187`: not every failed reading retries before 23:00, and the last-hour rule does not guarantee a written day. |
| +- `DISPOSITIONS-storage-sole-holder.md`, review-two final-read row: its named test does not exercise the final-read failure. |
| +- `walk.go:1679`: an unchanged generation covers events since the ledger baseline, not since the earlier record scan. |
| +- `walk.go:1803`: a name this build refuses is treated as having no ledger without checking. |
| +- `walk.go:1622`: `OtherHolders` does not read every record; its underlying scan skips invalid account/app names. |
| +- `economicspass_test.go:1721`: the comment says a departed store does not count. The implementation and revised test count its retained record. |
| +- `OUTSTANDING.md:68`: rerunning an unfinished grant does not necessarily release the hold; it can leave two active holders of the same bucket. |
| + |
| +**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 65e5c68b7..142c60399 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| @@ -33,3 +33,17 @@ Reports: `CODEX-FINDINGS-storage-sole-holder-review2.md`, `OPUS-REVIEW-storage-s |
| | 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. |
| + |
| +## Review three (reviewed `e6015870`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-storage-sole-holder-review3.md`, `OPUS-REVIEW-storage-sole-holder-review3.md`. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Both: the ledger generations were read after the record walk, so a store that was granted a bucket, wrote and left while the records were being read was already inside the baseline | Fixed. The inventory lists and reads every ledger first, then walks the records. Whatever was granted before that reading left a record for the walk; whatever is granted after it moves a recorded generation. | `objstore/walk.go` `ReadHolderInventory` | `TestStoragePassSeesAHolderThatCameAndWentDuringTheInventory` | |
| +| Both: three readings still wrote a held day mid-day (the store's own record, its record after the sizing, the sizing itself); the cited test did not reach the final ledger read | Fixed. Each of them fails the pass for a retry while there is time left in the day. A record that was returned and cannot be used (will not parse, or no walk can follow it) still holds at once. Whether there is time left is asked when the failure happens. | `console/economicspass.go` `storagePass`, `sizeSoleHolder`, `storeRecordUnusable` | `TestStoragePassRetriesEachOfAStoresReadings` (five readings, mid-day and in the last hour) | |
| +| Astra P2: a bucket name this build keeps no ledger for skipped the ledger check and was sized | Fixed. Its ownership evidence cannot be checked, so the store is held. | `OpenOnLedger` | `TestStoragePassHoldsAStoreWhoseBucketHasNoLedgerItCanRead` | |
| +| Both: the last-hour rule can still leave a day unwritten: the pass runs hourly, so the last hour has about one tick; a failure of the listings it starts with, or of the day's own write, is not covered; a pass that runs past the 23:00 tick leaves none | Accepted, partly fixed (the question is now asked at each failure). Open: the storage pass needs a short retry timer after 23:00 while the day is unwritten, and that is the economics loop's schedule, not this change. In 19i. | `OUTSTANDING.md` | none | |
| +| 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. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review3.md b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review3.md| new file mode 100644 |
| index 000000000..d730425f1 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review3.md |
| @@ -0,0 +1,137 @@ |
| +# Storage sole holder, third reading (e6015870 against base c68ecddd, and the change since 55c11cf3) |
| + |
| +## Dispositions: is each finding closed, and would its test fail without the fix? |
| + |
| +**Review one** |
| + |
| +| Row | Closed? | Would the test pass without the fix? | |
| +|---|---|---| |
| +| A holder appears during the sizing | Closed. | No. The test fails without the check after the sizing, because the store's own record is unchanged, so only the ledger check catches it. | |
| +| One inventory per store; one transient error holds for good | Partly closed. There is now one inventory per pass. But the store's own record read, the record re-read after the sizing, and the sizing itself still hold the day on a transient error (P2-1). | No for the inventory path. Those three paths have no test. | |
| +| A record under a name no build writes | Closed for the storage pass. | No. The test fails if invalid names are skipped. | |
| +| A record that will not parse | Closed as changed: every store is held. | No. Without the global hold, `mine` gets sized. | |
| +| A departed store's record | Closed as changed to a hold (approval K). | No. Without it, the ledger's `left()` excuses the other store and `mine` is sized. | |
| +| The 19i list of holds | Partly closed. It omits two shapes (P3-2). | No test. | |
| +| Holder in another deployment | Open, accepted. | n/a | |
| + |
| +**Review two** |
| + |
| +| Row | Closed? | Would the test pass without the fix? | |
| +|---|---|---| |
| +| A holder came and went before a store's turn | Closed between the generation read and the store's turn. A smaller window is still open during the inventory's own walk of the records (P2-3). | No. The test fails without the comparison before the sizing. | |
| +| A failed reading or malformed ledger lost the day | The malformed ledger is closed. A failed reading is closed for the inventory and for the ledger reads. The day can still go unwritten (P2-2). | No for both tests. | |
| +| Astra P2: the final ledger read | The code is fixed (`unreadable(gerr)`, economicspass.go:2501). | **Yes, the test would still pass.** The cited test fails *every* read of the ledger key, so the inventory catches it first and the final read is never reached (P3-1). | |
| +| Opus P3-4: inventory rules and a failing read | Closed. | No. Each case in `TestHolderInventoryRules` fails with its rule removed. | |
| +| Holds and cost in 19i | Partly closed. Cost is noted; two holds are missing (P3-2). | No test. | |
| + |
| +## Findings |
| + |
| +### P2-1: Three transient failures still write a held day while there is time to retry |
| +**Where:** `internal/console/economicspass.go:246` (own record read, `gerr`), `:2496` (record re-read after sizing, `aerr`), `:2488` (tenant sizing error, `serr`). |
| + |
| +**Scenario:** At 10:00 the store's own `objstore.Get` gets a 503 or throttle from S3. Or the `Get` after the sizing does. Or `ListObjectsV2` on the tenant bucket times out. In each case `StoreUnswept` is set and the day is written create-only, so that account's storage is held for the whole day. |
| + |
| +This contradicts the comment at :186–194 ("a store held on a failed reading would stay held", so the pass fails instead). It also contradicts 19i ("A reading the store fails to return fails the pass for a retry until the day's last hour"). The `aerr` path is in the branch's own new function. |
| + |
| +**Smallest fix:** In both record reads, split the error: |
| +- `ErrTwoBuckets`, `ErrUnfinishedTeardown` or a JSON error means hold. |
| +- Anything else, with `!lastChance`, returns the error so the pass retries. |
| + |
| +Do the same for `serr`, or state in 19i that a failed tenant listing is accepted as a held day. |
| + |
| +### P2-2: The last-hour rule can leave the day unwritten |
| +**Where:** economicspass.go:200, :180, :216, :222, :283; the ticker is hourly (`economicsPassInterval`, :34). |
| + |
| +The ticker's timing is set when the console starts, so the 23:00 hour holds about one tick. The day is lost in three cases: |
| +- **Uncovered failures:** that one pass hits a failure the rule does not cover, and the pass returns an error even with `lastChance` set. These are `ListDirs("apps/")`, the per-account `ListDirs`, the deployment `ListSizes`, or `CreateDayObject`. |
| +- **The last-hour flag is computed too early:** `lastChance` is set once, when the pass starts. A pass that starts at 22:59 and fails at 23:40 returns an error. If the next tick fires after midnight, it computes the next day's date. |
| +- **A long pass:** a pass that runs past the 23:xx tick (large tenant buckets) leaves only the pending tick, which fires after midnight. |
| + |
| +The code's own comment says a missing day holds storage for every account. The branch now fails the whole pass on any ledger or inventory read, which puts more weight on that one shot. |
| + |
| +**Writing a day twice is impossible:** the day object is create-only and checked first. |
| + |
| +**Smallest fix:** |
| +- Evaluate `storageLastChance` at the point of each failure, not at the start of the pass. |
| +- While the day is unwritten after 23:00, retry the storage pass on a short timer (minutes, not an hour). |
| + |
| +### P2-3: A holder that comes and goes during the inventory's walk is still missed |
| +**Where:** `internal/objstore/walk.go:1768`. The generations are read *after* the walk of every record. |
| + |
| +**Scenario:** Store V (`bcd/visitor`) has only a Declared record when the walk passes its directory. Then an operator grants V bucket B: a grant block is written and an interval opened. V writes data. V's teardown is retained, so its interval closes and its record is kept. All of this finishes before the loop at :1768 reads B's ledger. |
| + |
| +The inventory then has no V record, and its generation for B already includes V's open and close. At B's store's turn the generation is unchanged and V has no open interval, so the store is sized with V's leftover bytes. By ruling K those bytes should be held. |
| + |
| +The window is the length of the record walk: N sequential GETs, seconds to minutes at a few hundred apps. The disposition says the generation is recorded "when it is read". It is read after the records, not with them. |
| + |
| +**Smallest fix:** Take the generations before walking the records. List `BucketOwnershipPrefix` and GET each ledger before the walk, and use those ETags as `Generation`. A ledger that is absent at that point counts as `""`. Any grant after the snapshot then moves the generation, and anything before it left a record that the walk will see. |
| + |
| +### P3-1: The cited reproduction does not reach the final ledger read |
| +**Where:** `TestStoragePassRetriesAFailedReadingUntilTheDaysLastHour`. |
| + |
| +`failingGets` fails every read of the ledger key, so the inventory's `LedgerUnread` catches it first. If `unreadable(gerr)` at :2501 were changed to an immediate hold, the test would still pass. |
| + |
| +Similarly, in the "a store record" case, `theirs` is held through the own-record `gerr` path whatever the inventory does. |
| + |
| +**Fix:** Add a fault that fails only the *n*-th read of the ledger key (the read after the sizing), at 10:00. Assert that the pass returns an error and no day is written. |
| + |
| +### P3-2: Two holds that last until a person acts are missing from 19i |
| +**Where:** OUTSTANDING.md:68. |
| + |
| +The list of shapes that hold forever is accurate for: |
| +- a retained departed store (19a); |
| +- a record no walk can follow (19g, `Coherent`-refused); |
| +- a grant that died with its block written; |
| +- two apps of one account on one bucket; |
| +- an unparseable record. |
| + |
| +It omits two: |
| +1. **A malformed ledger** (`ErrLedgerMalformed`). Its bucket's stores are held every day until the ledger is repaired, and there is no verb to repair it. |
| +2. **A record under an account or app name no build writes.** It holds every bucket it names, and only a hand delete in S3 removes it. 19g covers `Coherent` refusals, not invalid names. |
| + |
| +**Fix:** Add both to the list. |
| + |
| +### P3-3: Cost is linear and unbounded; a failing pass repeats every tenant listing |
| +**Inventory per pass:** about 1 + A `ListDirs` + N `GET`s (one per app directory, most of them 404s) + B ledger `GET`s, where A is accounts, N is app directories and B is named buckets. |
| + |
| +**Per provisioned store:** one own-record GET, one deployment `ListSizes`, two ledger GETs, one record re-read, and a full `ListObjectsV2` of the tenant bucket. |
| + |
| +That is O(N) per pass. The brief's premise of O(N²) is gone: the full record walk per store (`OtherHolders`) is now only in Grant's take-back. Nothing bounds N. A pass that fails mid-day re-lists every tenant bucket ahead of the failing store, up to about 23 times a day; for a bucket of 10⁸ objects that is about 10⁵ LISTs per repeat. The pass has no deadline and blocks the statements and bill passes behind it. |
| + |
| +**Smallest fix:** Before sizing anything, check `LedgerUnread` for every provisioned store's bucket and fail early on any of them. |
| + |
| +## Grant's take-back check: unchanged |
| + |
| +I compared `walk.go:304–315` at c68ecddd with `OtherHolders` at :1624. The behaviour is identical: |
| + |
| +- **Holders considered:** the same `otherStoreHolders` (record holders, still skipping invalid names), then open intervals on the ledger. |
| +- **Order:** records first, then intervals, so the same first other-account holder is named in `ErrBucketHeld`. |
| +- **Reconciliation:** |
| + - The old code applied `left()` to the interval holders too, but `left()` is false whenever an open interval exists, so that was a no-op. |
| + - The new de-duplication cannot re-admit a holder that `left()` dropped, because that holder by definition has no open interval. |
| + - The old filter `iv.Account != acct` and the new filter "same app excluded, then same account skipped in the loop" are equivalent. |
| +- **The new `ValidBucketName` guard:** it is a no-op here, because Grant already validated the bucket at :222. |
| +- **Errors:** both error wrappings have the same text. |
| + |
| +## Answers to the three specific questions |
| + |
| +- **A day held on a transient failure while there was time to retry?** Yes, through three paths (P2-1). |
| +- **A store sized whose bucket's generation was never compared?** |
| + - No in the cases you named: |
| + - **No ledger:** `""` is compared with `""`, and a ledger created after the inventory moves it to an ETag, so the store is held. |
| + - **A name this build would refuse:** the generation is `""` on both sides, but Grant refuses those names, so no new holder can join. |
| + - **A store or bucket not in the inventory:** the generation is `""` against an existing ledger, so the store is held. |
| + - The comparison covers the right moments except for the walk window (P2-3). |
| +- **Can the last-hour rule write a day twice, or not at all?** Never twice: the day object is create-only. Not at all: yes (P2-2). |
| + |
| +## Claims the code contradicts |
| + |
| +1. **The storagePass comment at :186–194 and 19i** say a failed reading fails the pass for a retry. It does not for the own-record read, the re-read after sizing, or a failed tenant listing (P2-1). |
| +2. **The same comment** says that in the last hour "it writes the day". It does not if a `ListDirs`, `ListSizes` or `CreateDayObject` fails, or if no tick lands in that hour (P2-2). |
| +3. **The `HolderInventory.Generation` doc and the review-two disposition** say the generation is taken "when the inventory was read". It is read after the whole record walk (P2-3). |
| +4. **The `OtherHolders` doc at walk.go:1622** says it "reads every store record in the deployment". `storeHolders` skips invalid account and app names. This is known and predates the branch, but the doc is new. |
| +5. **The review-two disposition row for Astra P2 (the final ledger read)** cites a reproduction that never reaches that read (P3-1). |
| +6. **19i's list of holds that last until a person acts** is incomplete (P3-2). |
| +7. **The brief** says storagePass calls `OtherHolders`. It does not; it uses `ReadHolderInventory`, `Others` and `OpenOnLedger`. |
| + |
| +**Verdict: accept with fixes.** Fix P2-1, P2-2 and P2-3, and the P3-1 test, before merge. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index 0e953ff61..dcf8a4e5d 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 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. |
| +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. |
| 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 96c68acff..4ce98ccdf 100644 |
| --- a/internal/console/economicspass.go |
| +++ b/internal/console/economicspass.go |
| @@ -197,10 +197,9 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| // 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 { |
| - if !lastChance { |
| + if !storageLastChance(date) { |
| 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) |
| @@ -236,6 +235,12 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| if release.ValidAccount(acct) && release.ValidApp(app) { |
| rec, _, gerr = objstore.Get(ctx, s.cfg.Releases, acct, app) |
| } |
| + if gerr != nil && !storeRecordUnusable(gerr) && !storageLastChance(date) { |
| + // A reading the store failed to return, with time left in |
| + // the day: the whole pass is tried again, and no store is |
| + // held on it. |
| + return fmt.Errorf("storage pass: store record %s/%s: %w", acct, app, gerr) |
| + } |
| if gerr != nil { |
| // Absent is (nil, "", nil) — objstore.Get's own idiom — so a |
| // non-nil error here is a REAL failure (transient bucket |
| @@ -270,7 +275,7 @@ func (s *Server) storagePass(ctx context.Context, date string) error { |
| bucket := rec.Provisioned.Bucket |
| if holdEveryStore { |
| st.StoreUnswept = true |
| - } else if held, herr := s.sizeSoleHolder(ctx, inventory, rec, acct, app, bucket, lastChance, &st); herr != nil { |
| + } else if held, herr := s.sizeSoleHolder(ctx, inventory, rec, acct, app, bucket, date, &st); herr != nil { |
| return herr |
| } else if held != "" { |
| log.Printf("console: storage pass: %s/%s: %s; held", acct, app, held) |
| @@ -2457,21 +2462,26 @@ func storageLastChance(date string) bool { |
| // 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 |
| +func (s *Server) sizeSoleHolder(ctx context.Context, inventory *objstore.HolderInventory, rec *objstore.Record, acct, app, bucket, date string, st *usage.AppStorage) (held string, err error) { |
| + // unreadable turns a reading that failed into a hold or, with time |
| + // left in the day, into a retry of the pass. Whether there is time |
| + // left is asked when the failure happens, not when the pass began: a |
| + // long pass can cross into the day's last hour. |
| + unreadable := func(what string, lerr error, permanent bool) (string, error) { |
| + if permanent || storageLastChance(date) { |
| + return what + ": " + lerr.Error(), nil |
| } |
| - return "", fmt.Errorf("storage pass: %s/%s: %w", acct, app, lerr) |
| + return "", fmt.Errorf("storage pass: %s/%s: %s: %w", acct, app, what, lerr) |
| + } |
| + ledgerUnreadable := func(lerr error) (string, error) { |
| + return unreadable("the ownership ledger of its bucket could not be read", lerr, errors.Is(lerr, objstore.ErrLedgerMalformed)) |
| } |
| if inventory.LedgerUnread(bucket) { |
| - return unreadable(errors.New("the store did not return it when the pass began")) |
| + return ledgerUnreadable(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) |
| + return ledgerUnreadable(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 |
| @@ -2485,20 +2495,25 @@ func (s *Server) sizeSoleHolder(ctx context.Context, inventory *objstore.HolderI |
| } |
| n, serr := s.sizeStore(ctx, rec.Provisioned) |
| if serr != nil { |
| - return "its bucket could not be sized: " + serr.Error(), nil |
| + // A listing that failed is a reading that failed. |
| + return unreadable("its bucket could not be sized", serr, false) |
| } |
| // 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) { |
| + after, _, aerr := objstore.Get(ctx, s.cfg.Releases, acct, app) |
| + if aerr != nil && !storeRecordUnusable(aerr) { |
| + return unreadable("its record could not be read again after the sizing", aerr, false) |
| + } |
| + if 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) |
| + return ledgerUnreadable(gerr) |
| } |
| if now != generation { |
| return "ownership of its bucket changed while it was being sized", nil |
| @@ -2506,3 +2521,14 @@ func (s *Server) sizeSoleHolder(ctx context.Context, inventory *objstore.HolderI |
| st.StoreBytes = n |
| return "", nil |
| } |
| + |
| +// 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 |
| +// 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) |
| +} |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 1a208a1fe..87ef65ce1 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -2201,3 +2201,155 @@ func TestHolderInventoryRules(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// nthGet fails, or runs a hook before, the nth Get of one key. It still |
| +// answers sized listings, which the storage pass needs from its store. |
| +type nthGet struct { |
| + release.Store |
| + key string |
| + n int |
| + seen int |
| + fail bool |
| + hook func() |
| +} |
| + |
| +func (f *nthGet) Get(ctx context.Context, key string) ([]byte, string, error) { |
| + if key == f.key { |
| + f.seen++ |
| + if f.seen == f.n { |
| + if f.hook != nil { |
| + f.hook() |
| + } |
| + if f.fail { |
| + return nil, "", errors.New("store had a bad moment") |
| + } |
| + } |
| + } |
| + return f.Store.Get(ctx, key) |
| +} |
| + |
| +func (f *nthGet) ListSizes(ctx context.Context, prefix string) ([]release.KeySize, error) { |
| + return f.Store.(release.SizeLister).ListSizes(ctx, prefix) |
| +} |
| + |
| +func soleStoreFixture(t *testing.T, bucket string) (*consoleFixture, time.Time) { |
| + t.Helper() |
| + at := time.Date(2026, 8, 10, 9, 0, 0, 0, time.UTC) |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + rec := &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}, |
| + } |
| + putRawStore(t, fx, fixtureAcct, fixtureApp, rec) |
| + if objstore.ValidBucketName(bucket) { |
| + if err := objstore.OpenBucketOwner(ctx, fx.rel, bucket, fixtureAcct, fixtureApp, at, at, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { return 5 << 30, nil } |
| + return fx, at |
| +} |
| + |
| +// Every reading the pass makes for one store, failing once, mid-day: the |
| +// store's own record, its record again after the sizing, its bucket's |
| +// ledger before the sizing and after it, and the sizing itself. Each fails |
| +// the pass for a retry and writes no day. The same failures in the day's |
| +// last hour write the day with the store held. |
| +func TestStoragePassRetriesEachOfAStoresReadings(t *testing.T) { |
| + const bucket = "carlos-store-mine" |
| + prevClock := storageClock |
| + t.Cleanup(func() { storageClock = prevClock }) |
| + mine := fixtureAcct + "/" + fixtureApp |
| + for name, c := range map[string]struct { |
| + key string |
| + n int |
| + }{ |
| + // The record is read by the inventory, by the pass, and again |
| + // after the sizing. The ledger is read by the inventory, before |
| + // the sizing, and after it. |
| + "its own record": {objstore.Key(fixtureAcct, fixtureApp), 2}, |
| + "its record after the sizing": {objstore.Key(fixtureAcct, fixtureApp), 3}, |
| + "its ledger before the sizing": {objstore.BucketOwnershipKey(bucket), 2}, |
| + "its ledger after the sizing": {objstore.BucketOwnershipKey(bucket), 3}, |
| + "the sizing of its bucket fails": {"", 0}, |
| + } { |
| + for _, hour := range []int{10, 23} { |
| + fx, _ := soleStoreFixture(t, bucket) |
| + ctx := context.Background() |
| + if c.key == "" { |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { |
| + return 0, errors.New("listing timed out") |
| + } |
| + } else { |
| + fx.srv.cfg.Releases = &nthGet{Store: fx.rel, key: c.key, n: c.n, fail: true} |
| + } |
| + storageClock = func() time.Time { return time.Date(2026, 8, 12, hour, 5, 0, 0, time.UTC) } |
| + err := fx.srv.storagePass(ctx, "2026-08-12") |
| + day, written := storageDay(t, fx, "2026-08-12") |
| + if hour == 10 { |
| + if err == nil || written { |
| + t.Errorf("%s, mid-day: err %v, day written %v; want the pass to fail and write nothing", name, err, written) |
| + } |
| + continue |
| + } |
| + if err != nil || !written { |
| + t.Errorf("%s, last hour: err %v, day written %v; want the day written", name, err, written) |
| + continue |
| + } |
| + if app := day.Apps[mine]; !app.StoreUnswept || app.StoreBytes != 0 { |
| + t.Errorf("%s, last hour: %+v; want held", name, app) |
| + } |
| + } |
| + } |
| +} |
| + |
| +// A store is granted a bucket, writes to it and leaves again while the |
| +// inventory is still reading records, before it has reached that store's |
| +// own. The ledgers were read first, so the bucket's generation has moved |
| +// since, and the bucket's store is held. |
| +func TestStoragePassSeesAHolderThatCameAndWentDuringTheInventory(t *testing.T) { |
| + const bucket = "carlos-store-mine" |
| + fx, at := soleStoreFixture(t, bucket) |
| + 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) } |
| + // The first read of the store's own record is the inventory's. |
| + fx.srv.cfg.Releases = &nthGet{Store: fx.rel, key: objstore.Key(fixtureAcct, fixtureApp), n: 1, hook: func() { |
| + if err := objstore.OpenBucketOwner(ctx, fx.rel, bucket, "bef", "visitor", at.Add(time.Hour), at.Add(time.Hour), false); err != nil { |
| + t.Error(err) |
| + } |
| + if err := objstore.CloseBucketOwner(ctx, fx.rel, bucket, "bef", "visitor", at.Add(2*time.Hour)); err != nil { |
| + t.Error(err) |
| + } |
| + }} |
| + sized := 0 |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { sized++; 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 app := day.Apps[fixtureAcct+"/"+fixtureApp]; sized != 0 || !app.StoreUnswept || app.StoreBytes != 0 { |
| + t.Fatalf("sized %d time(s), %+v; want held, not sized", sized, app) |
| + } |
| +} |
| + |
| +// A bucket name this build keeps no ledger for cannot have its ownership |
| +// evidence checked at all. That is not "no other holder": the store is |
| +// held, not sized. |
| +func TestStoragePassHoldsAStoreWhoseBucketHasNoLedgerItCanRead(t *testing.T) { |
| + fx, _ := soleStoreFixture(t, "Legacy_Bucket") |
| + prevClock := storageClock |
| + t.Cleanup(func() { storageClock = prevClock }) |
| + storageClock = func() time.Time { return time.Date(2026, 8, 12, 10, 0, 0, 0, time.UTC) } |
| + sized := 0 |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { sized++; return 5 << 30, nil } |
| + if err := fx.srv.storagePass(context.Background(), "2026-08-12"); err != nil { |
| + t.Fatal(err) |
| + } |
| + day, _ := storageDay(t, fx, "2026-08-12") |
| + if app := day.Apps[fixtureAcct+"/"+fixtureApp]; sized != 0 || !app.StoreUnswept { |
| + t.Fatalf("sized %d time(s), %+v; want held, not sized", sized, app) |
| + } |
| +} |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index f4a89df30..f372b325d 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -1697,6 +1697,32 @@ func (inv *HolderInventory) LedgerUnread(bucket string) bool { |
| // ReadHolderInventory walks apps/<account>/<app>/store.json. |
| func ReadHolderInventory(ctx context.Context, s release.Store) (*HolderInventory, error) { |
| inv := &HolderInventory{byBucket: map[string][]OwnerRef{}, generation: map[string]string{}, ledgerUnread: map[string]bool{}} |
| + // The ledgers first, every one there is, and only then the records. |
| + // A grant opens its interval on the ledger before it writes anything |
| + // a tenant can use, so: whatever was granted before this reading has |
| + // left a record for the walk below to find, and whatever is granted |
| + // after it moves a generation recorded here. Taken after the walk, the |
| + // baseline would already contain a store that was granted the bucket, |
| + // wrote to it and left while the records were being read. |
| + ledgers, err := s.List(ctx, BucketOwnershipPrefix) |
| + if err != nil { |
| + return nil, fmt.Errorf("objstore: holder inventory: list %s: %w", BucketOwnershipPrefix, err) |
| + } |
| + for _, key := range ledgers { |
| + if !strings.HasSuffix(key, ".json") { |
| + continue |
| + } |
| + bucket := strings.TrimSuffix(strings.TrimPrefix(key, BucketOwnershipPrefix), ".json") |
| + _, etag, gerr := s.Get(ctx, key) |
| + if errors.Is(gerr, os.ErrNotExist) { |
| + continue |
| + } |
| + if gerr != nil { |
| + inv.ledgerUnread[bucket] = true |
| + continue |
| + } |
| + inv.generation[bucket] = etag |
| + } |
| accounts, err := s.ListDirs(ctx, "apps/") |
| if err != nil { |
| return nil, err |
| @@ -1765,21 +1791,6 @@ 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 |
| } |
| @@ -1805,11 +1816,15 @@ func (inv *HolderInventory) Others(bucket, acct, app string) []OwnerRef { |
| // ("" when there is no ledger, or the name is one this build would refuse |
| // 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. |
| +// ledger that will not parse or validate is ErrLedgerMalformed, and so is a |
| +// bucket name this build keeps no ledger for; 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 |
| + // A name this build would refuse has no ledger this code may read |
| + // (the key is built from the name). That is not the same as "no |
| + // other holder": the evidence cannot be checked at all. |
| + return nil, "", fmt.Errorf("%w: %q is not a bucket name this build keeps a ledger for", ErrLedgerMalformed, bucket) |
| } |
| raw, etag, err := s.Get(ctx, BucketOwnershipKey(bucket)) |
| if errors.Is(err, os.ErrNotExist) { |