amadan
The fifth review's small items: two tests, a deduplicated hold diagnostic, a corrected comment, and 19i's holds that only a person lifts
Both reviewers accepted the last diff with fixes, P3 only, and both confirmed that no credential exists before a grant's ledger write. Billing is off.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+340
−5
docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review5.md+147 −0docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md+15 −0docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review5.md+111 −0implementation/carloku-paid-launch/OUTSTANDING.md+1 −1internal/console/economicspass.go+10 −2internal/console/economicspass_test.go+52 −0internal/objstore/walk.go+4 −2
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review5.md b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review5.md| new file mode 100644 |
| index 000000000..0e3efcf08 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-storage-sole-holder-review5.md |
| @@ -0,0 +1,147 @@ |
| +Reviewed `99c0d923`, including `cd66eabf..99c0d923` and `c68ecddd..99c0d923`, as requested. All locations below refer to `99c0d923`. No files changed, tests run, or network commands used. Test outcomes are predictions from source inspection. |
| + |
| +1. **P3 — Two regression protections remain untested.** |
| + `internal/console/economicspass_test.go:2084`, `:2292`; implementation at `internal/objstore/walk.go:1840` and `internal/console/economicspass.go:2471`. |
| + |
| + The malformed-ledger test supplies broken JSON only. Removing `ledger.validate(bucket)` would leave it passing. A syntactically valid ledger with the wrong bucket, schema, or invalid intervals would then be accepted as ownership evidence. |
| + |
| + The retry tests keep the clock fixed throughout each pass. Computing the last-hour decision once at entry would still pass them. A pass starting before 23:00 and encountering a failed reading after 23:00 would regress to returning an error instead of recording the affected hold. |
| + |
| + **Smallest acceptable fix:** add a valid-JSON ledger rejected by validation, and a read hook that advances the clock across 23:00 before returning an error. These gaps were reported in reading four but are not resolved by its dispositions. |
| + |
| +2. **P3 — 19i still omits persistent holds and gives an insufficient recovery instruction.** |
| + `implementation/carloku-paid-launch/OUTSTANDING.md:68`; implementing paths at `internal/objstore/walk.go:1766`, `:1843`. |
| + |
| + A record under an invalid account/app name can hold a healthy store indefinitely. Normal record APIs reject that path, so ordinary grant or teardown cannot repair it. An orphan open ledger interval likewise holds another store indefinitely: teardown of the missing record returns immediately at `walk.go:910`. |
| + |
| + Neither shape is explicitly included in 19i’s persistent-hold list. Its instruction that an unfinished grant holds “until it is run again” is also insufficient: completing that grant can leave two live stores sharing the bucket, continuing the hold. |
| + |
| + **Smallest acceptable fix:** document both omitted shapes and their manual reconciliation requirements; say that resolving an unfinished grant must also resolve the sharing before measurement resumes. |
| + |
| +3. **P3 — The hold diagnostic double-counts another store.** |
| + `internal/console/economicspass.go:2493`. |
| + |
| + If B appears in both the record inventory and an open ledger interval, sizing A reports **two** other stores although only B exists. The hold itself is correct, but the diagnostic misdescribes the estate an operator must repair. |
| + |
| + **Smallest acceptable fix:** deduplicate by account/app before counting, or omit the count. |
| + |
| +**The disputed grant schedule** |
| + |
| +**The credential-ordering argument holds for the reported fresh-grant schedule.** It does not establish misattribution of V’s bytes: |
| + |
| +| Step | Code and consequence | |
| +|---|---| |
| +| Publish the grant block | `walk.go:315`, `:348`: the record can become visible before the ledger changes. | |
| +| Open ownership | `walk.go:452`: every grant path reaches this before provider mutation. Failure returns at `:455` or `:457`. | |
| +| Mark provider work | `walk.go:496`, `:506`: the walk count and delivery state are persisted after ownership opens. | |
| +| Create/update the user policy | `walk.go:545`: `EnsureUser` runs only after those steps. | |
| +| Healthy re-grant | `walk.go:551`: even the short-circuit follows the ownership check and policy update. | |
| +| Mint and deliver | `walk.go:571`, `:576`: both follow the successful ownership operation. | |
| +| Record success | `walk.go:591`: failure here can leave delivered credentials behind a grant-only record, but its ledger interval has already opened. | |
| + |
| +The important qualifications are: |
| + |
| +- **Resumed grant with an already-open interval:** `ownership.go:402` finds it; `:417` can return unchanged. Credentials may already exist, so “a failed ledger operation means no credential” is not universally true. However, `OpenOnLedger` includes that other store even without a generation change. |
| +- **Take-back of a closed interval:** teardown revoked the user before closing ownership (`walk.go:1184`, `:1198`). Grant appends a new interval before restoring access (`ownership.go:413`), changing the generation. |
| +- **Take-back with an open interval:** it already counts as another holder. Clearing `Leaving` or `Destroying` also changes the ledger. |
| +- **Older provisioned store without a ledger:** its bucket-naming record is counted by the inventory; a failed ledger operation during re-grant does not make that record disappear. |
| + |
| +Thus the fourth-reading schedule still misses a **new grant-only record**, but that failed first grant cannot have created or delivered credentials. I withdraw its characterization as a demonstrated P1 billing misattribution. The stricter record-based sole-holder rule still has this documented timing exception. |
| + |
| +The post-sizing reread at `economicspass.go:2506` checks disappearance, unusable records, teardown and estate replacement. The subsequent ledger comparison catches a holder that appeared and left during sizing. I found no additional credential-bearing escape through those checks. |
| + |
| +**Disposition checks** |
| + |
| +“Pass without fix” means the named protection is removed while retaining the current test. “None” means no dedicated regression test establishes that claim. |
| + |
| +| Disposition row | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| R1: Holder appears/leaves during sizing | Yes, for the ledger-recorded event. | **No:** final generation comparison is necessary. | |
| +| R1: Per-store inventories; transient reads | Yes for repeated remote inventories and retry policy. | **No:** current count and retry tests discriminate. | |
| +| R1: Invalid account/app names skipped | Yes for storage’s literal-key inventory. | **No** for the invalid-account case; no separate invalid-app case. | |
| +| R1: Unparseable record causes global hold | Implemented, now including all decoding failures. | **No:** removing the global hold permits the healthy store’s sizing. | |
| +| R1: Departed store’s retained record | Yes. | **No:** test requires a hold, then measurement after record removal. | |
| +| R1: Permanent holds documented | Partial; finding 2 remains. | None. | |
| +| R1: Cross-deployment holder | Accepted open; not re-reported. | None. | |
| +| R1: Tests strengthened | Yes for their named cases. | Negative cases **no**; sole-holder positive control **yes**, appropriately. | |
| +| R2: Holder comes/goes before a store’s turn | Yes. | **No:** pre-sizing generation comparison is necessary. | |
| +| R2: Failed readings and malformed ledgers | Yes for implemented behaviour. | **No** for retries and malformed JSON; **yes** if only ledger validation is removed. | |
| +| R2: Final-ledger failure commits a hold | Yes. | Old cited test **yes**; current nth-read case **no**. | |
| +| R2: Cross-deployment holder | Accepted open; not re-reported. | None. | |
| +| R2: Grant-only, refused-record, deletion and failure cases | Yes. | **No:** the added cases distinguish their protections. | |
| +| R2: Persistent holds and cost | Remote quadratic inventory reads fixed; documentation partial. | Count test **no**; no bounded-cost test. | |
| +| R3: Ledger baseline taken after record walk | Yes for the reported completed-grant schedule. | **No:** moving baseline reads after the walk defeats the test. | |
| +| R3: Own read, reread and sizing failures; final-read coverage | Yes, including decoding classification now. | **No** for retry/classification removal; **yes** for caching the cutoff at entry. | |
| +| R3: Unsupported bucket name sized | Yes. | **No:** returning successful empty ownership evidence permits sizing. | |
| +| R3: Last-hour scheduling/listing/write gaps | Accepted open; not re-reported. | No scheduling test. | |
| +| R3: Missing hold documentation and repeated work | Named additions documented; broader omissions remain. | None. | |
| +| R4: Late grant block followed by failed ledger write | Not fixed as a record-inventory gap; P1 misattribution argument does not stand for that schedule. | None. | |
| +| R4: Non-syntax/type decoding failures | Yes. | **No:** timestamp cases require the sentinel and immediate classification. | |
| +| R4: Older tests accidentally use last-hour fallback | Yes for the three identified tests. | **No:** with current clock pins, removing unusable-record classification fails them. | |
| +| R4: No test pins one inventory per pass | Yes. | **No:** an extra per-store inventory increases the declaration-only record’s read count. | |
| +| R4: Missing permanent holds and recurring daily cost | Opus’s named additions closed; Astra’s broader documentation finding remains partial. | None. | |
| +| R4: Obtain baseline ETags from listing | Optional optimization remains undone. | None. | |
| + |
| +**Persistent holds** |
| + |
| +These are conservative holds, not evidence that sizing should proceed despite unresolved ownership. Repair enables future measurements; it does not rewrite an already-created held day. |
| + |
| +| Shape | What releases it? | Does 19i explain it? | |
| +|---|---|---| |
| +| Departed store with retained record, closed intervals or no ledger | Finish the retained teardown when permitted. | Yes. | |
| +| `Coherent`-refused record | Repair its conflicting evidence; every named bucket counts meanwhile. | Yes, through 19g. | |
| +| Grant-only record | Resolve the unfinished estate **and any remaining sharing**. | Listed; recovery wording insufficient. | |
| +| Two live stores, including two apps of one account | End the sharing. | Yes. | |
| +| Undecodable record | Repair/remove it; until then every provisioned store is uncertain. | Yes. | |
| +| Record under invalid account/app names | Repair/remove the literal-key record and reconcile its estate. | Not explicitly as a persistent storage hold. | |
| +| Open interval without its store record | Reconcile the orphan ownership evidence. | No. | |
| +| Ledger failing decoding or validation | Repair the ledger. | Decoding named; validation not explicit. | |
| +| Unsupported bucket name | Migrate/repair the estate so ownership evidence can be checked. | Yes. | |
| +| Permanently failing record, ledger or bucket read | Restore readability; otherwise retries and last-hour holds recur daily. | Yes. | |
| + |
| +**Grant’s take-back comparison** |
| + |
| +The extraction into `OtherHolders` preserves Grant’s decisions and first-holder selection for reachable inputs. The complete branch does **not** preserve every error string, because `Inspect` changed separately. |
| + |
| +| Aspect | Before → after | |
| +|---|---| |
| +| Entry condition | Same non-operator take-back condition, `walk.go:303`. | |
| +| Record holders | Same `otherStoreHolders`, traversal order, invalid-name exclusions, refused-record handling and deleted-bucket skip. | |
| +| Read order | Record scan first, ledger second. Record-scan failure prevents the ledger read. | |
| +| Reconciliation | `left` filtering moves into `OtherHolders`; refused records remain exempt. Previously checking ledger-derived holders too was redundant: an open interval makes `left` false. | |
| +| Ledger holders | Previously foreign-account open intervals; now other-app open intervals, deduplicated. Grant skips the additional same-account entries. | |
| +| First refusal | Surviving record holders still precede ledger-only holders; deduplication preserves the first foreign holder. | |
| +| Errors from the extraction | Same scan/read wrappers, precedence and `ErrBucketHeld` text. | |
| +| Invalid bucket name | Helper now skips its ledger read, but Grant rejects that name earlier at `walk.go:222`; unreachable difference. | |
| + |
| +At `objstore.go:263`, decoding errors now include `ErrRecordMalformed` and retain the original decoding error through the second `%w`. Consequently: |
| + |
| +- Error text changes from `objstore: <key>: <decode error>` to `objstore: store record does not decode: <key>: <decode error>`. |
| +- `errors.Is`/`errors.As` still reach the original cause. |
| +- The wrapper now has multiple causes; single-error `errors.Unwrap` would behave differently. I found no repository caller relying on that. |
| +- Other `Get`/`Inspect` callers still take their existing error branches: refuse, propagate, log, or show the same generic response. **Only storage’s classification changes control flow.** |
| + |
| +**Cost** |
| + |
| +The brief’s once-per-store inventory would cost **Θ(N²) record reads** for N apps with stores. That is no longer the implementation: storage calls `ReadHolderInventory` once at `economicspass.go:200`, not `OtherHolders` per store. |
| + |
| +For N valid apps, each with a separately sized store, and L historical ledger objects, a successful pass performs approximately: |
| + |
| +- **3N record GETs:** inventory, own-record read, post-sizing reread. |
| +- **L + 2N ledger GETs:** baseline inventory and two checks per sized store. |
| +- Two account/app directory traversals, deployment-prefix sized listings, and complete tenant-bucket listings. |
| + |
| +There is no explicit app, ledger, object-count or pass-duration bound. L grows with retained ledger history. Successful day creation prevents later inventory work that day; failed attempts repeat work. |
| + |
| +Remote record reads are linear. In-memory work can remain **O(N²)** when many records name one bucket: insertion scans the existing holder list at `walk.go:1759`, and each `Others` call scans it again at `:1805`. |
| + |
| +**Claims the code contradicts** |
| + |
| +- `economicspass.go:2459` and 19i’s opening promise literal sole-holder status throughout collection. The acknowledged late grant-only record exception means that promise needs qualification. The credential-ordering argument establishes the narrower safety result above. |
| +- 19i’s “that store has no credential” is valid for the reported fresh grant, not every resumed grant whose ledger operation fails. Existing intervals/records are what protect resumed cases. |
| +- 19i says rerunning an unfinished grant releases its hold; completing a second live holder does not. |
| +- `walk.go:1622` says `OtherHolders` reads every store record. Its underlying scan skips invalid account/app names at `:1368` and `:1377`. |
| +- `economicspass_test.go:1725` says a departed store’s retained record does not count. The implementation and the test’s assertions count it. |
| +- `economicspass_test.go:2407` says an app’s record is read by the inventory and its own visit “and by nothing else.” Sized stores also receive the post-sizing reread; only the declaration-only probe has exactly two reads. |
| + |
| +**Verdict: accept with fixes.** |
| \ 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 e69927c47..6bd45df07 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-storage-sole-holder.md |
| @@ -62,3 +62,18 @@ Reports: `CODEX-FINDINGS-storage-sole-holder-review4.md`, `OPUS-REVIEW-storage-s |
| | Opus P3-5: the baseline could come from the listing's own ETags | Noted, not done. | none | none | |
| Mutation checks in a scratch copy, each failing its test: the unusable-record test always false; the sentinel not applied; a second inventory inside the per-store path. |
| + |
| +## Review five (the diff `cd66eabf..99c0d923`; Astra accepted with fixes, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-storage-sole-holder-review5.md`, `OPUS-REVIEW-storage-sole-holder-review5.md`. The first reading with no rejection. Every finding is P3. Both reviewers checked the answer to review four's P1 against the grant's order of operations and found it holds: no credential exists before the grant's ledger write has landed, on any grant path. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra: a ledger that parses and does not validate was untested; so was asking for the last hour at the moment of failure | Added. | `economicspass_test.go` | `TestStoragePassHoldsOnALedgerThatParsesAndDoesNotValidate`, `TestStoragePassAsksForTheLastHourWhenAReadingFails` | |
| +| Both: 19i omitted holds that only a person lifts, and "until it is run again" was not enough | 19i now names: a refused grant whose undo failed (removed by hand); a record under a name no build writes (removed by hand); an open ledger interval whose record is gone (reconciled by hand); and that finishing an unfinished grant leaves two stores sharing the bucket, which still holds. | `OUTSTANDING.md` | none | |
| +| Astra: the hold's log line counted one store twice when it was in both the records and the ledger | Fixed. | `economicspass.go` `sizeSoleHolder` | none | |
| +| Opus: `OtherHolders`' comment said it reads every record | Corrected: every record under a valid account and app name. | `walk.go` | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the ledger's validation removed; the last hour decided once at the start of the reading. |
| + |
| +This branch is a fix for one open item, read five times; the last reading accepted it with small fixes, which are in the commit that adds this section and are unread. It is not merged, and it is not the launch branch's approval. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review5.md b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review5.md| new file mode 100644 |
| index 000000000..b884858b7 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-storage-sole-holder-review5.md |
| @@ -0,0 +1,111 @@ |
| +# Second reading of `feat/carloku-storage-sole-holder-20261001` at `99c0d923` |
| + |
| +I found nothing at P1 or P2. Two P3s are below. I ran no tests; every claim here comes from reading the source. |
| + |
| +## Review four dispositions |
| + |
| +| Row | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| Astra P1: a grant block written after the walk, then the ledger write fails | Closed: it is not a mis-attribution. The argument is checked line by line below. | There is no test. | |
| +| Records that fail to decode for reasons other than syntax | Closed. `Inspect` wraps every decode failure (objstore.go:263), and `storeRecordUnusable` checks for it (economicspass.go:2530). | No. "a time that is not one" (`*time.ParseError`) and "a time that is a number" fail without the wrap: the pass returns an error mid-day. The other two shapes (broken syntax, wrong type) would pass, because the old `errors.As` code already caught them. The `errors.Is(ErrRecordMalformed)` assertion on `Get` also pins the wrap. | |
| +| Older tests ran as if the day were over | Closed. `storageMidDay` is applied in all three tests. | These are pins. They now fail if a hold falls back to the last-hour rule, for example if `ErrTwoBuckets` is dropped from `storeRecordUnusable`. | |
| +| Nothing pinned one inventory per pass | Closed. | No. A per-store inventory reads `bef/plain` once per provisioned store, so the count goes above 2. | |
| +| 19i missed permanent holds and daily cost | Closed, apart from one inaccurate clause (P3-2 below). | No test applies. | |
| +| Opus P3-5: take the baseline from the listing's ETags | Noted and not done. That is acceptable. | No test applies. | |
| + |
| +The rows from reviews one to three still hold at `99c0d923`. The ledger generation is checked before and after the sizing. The ledgers are read before the record walk. The inventory reads records by their literal keys. An unparsed record holds every store. Departed stores' records count, because the inventory never applies `left`. A malformed ledger, or a bucket name with no ledger, holds at once. The last-hour rule is asked at each failure. Every test the dispositions cite exists. |
| + |
| +## The P1 argument, checked against walk.go at `99c0d923` |
| + |
| +It holds. The order in `grant` is the same on every path: |
| + |
| +1. The grant block, plus any take-back count, is written (walk.go:315–351). |
| +2. A take-back runs `markBucketDestroyed` first, which is a ledger write. |
| +3. `openBucketOwner` runs (walk.go:452), unconditionally, before the `healthy` short-circuit. |
| +4. The Walks count is written. |
| +5. `EnsureUser` runs (walk.go:545), then `EnsureBucket`, `ReapKeys`, `MintKey` and `writeEnv`. |
| + |
| +So no credential is created or delivered on any grant path before the ledger write has landed. That covers `Grant`, `Regrant`, take-back, and minted versus adopted buckets. |
| + |
| +- **A resumed grant whose interval is already open.** `openBucketOwner` returns `errOwnershipUnchanged` and writes nothing, so the generation does not move. But that open interval is visible to the first `OpenOnLedger` at the store's turn, so the store is held. If the interval was opened after the inventory, the generation moved anyway. |
| +- **A lost response on the ledger write.** The write landed, so the generation moved. |
| +- **ETag reuse (an ABA change).** It cannot happen. Ledgers are append-only (`appendOnly`), and no production code deletes one; the only `Delete(BucketOwnershipKey)` calls are in tests. |
| +- **A teardown that deleted the bucket and then failed at `DeleteUser`.** The inventory skips that record. But its interval stays open, because `CloseBucketOwner` runs after `DeleteUser`, and `Destroying` still refuses any new grant of the name. The ledger covers what the record skip leaves. |
| +- **Rotate.** It needs a `Provisioned` record, which means an earlier grant opened an interval. |
| + |
| +Schedule for the reported shape: V writes its grant block for bucket B after the walk passed V, and V's ledger write fails. V then stops before `EnsureUser`. V has no user and no key, so it could not have written to B. The bytes sized are A's. Not a finding. |
| + |
| +## Findings |
| + |
| +### P3-1: 19i says a stale grant block lasts "until it is run again", but one shape never clears |
| + |
| +- **Where:** `OUTSTANDING.md:68` (19i); walk.go:315, 358, 428. |
| +- **Scenario:** |
| + 1. V's minted grant names B. The bucket check finds B already exists while V holds no interval, so the grant is refused, and `refused()`'s undo `Put` fails. V's grant block for B stays. |
| + 2. Running the grant again reads `rec.Grant != nil`, so `wroteGrant` is false. |
| + 3. The same check refuses again (walk.go:428), and `refused()` now does nothing. The block never goes. |
| + 4. A teardown of V would find B held by A and retain V's record. |
| + 5. A, the bucket's real holder, is held every day. |
| +- **How likely:** low. It needs an operator to adopt for A the exact name V would mint, plus one failed write to the deployment bucket. |
| +- **Smallest fix:** say in 19i that a grant block left by a *refused* grant whose undo failed is removed only by hand. Alternatively, in `grant`, treat a grant block with no `Provisioned` block and no interval on the ledger as this walk's own to undo. |
| + |
| +### P3-2: `OtherHolders`' comment claims more than the code does |
| + |
| +- **Where:** walk.go:1622. |
| +- **The claim:** "It reads every store record in the deployment." |
| +- **What the code does:** it goes through `storeHolders`, which skips records under account or app names that no build writes (walk.go:1368, 1377). So for Grant's take-back check, such a record does not hold the bucket. The behaviour predates this branch and the dispositions acknowledge it; the new comment states the opposite. |
| +- **Fix:** say "every record under a valid account and app name", as the inventory's own comment distinguishes. |
| + |
| +## Answers to the six questions |
| + |
| +1. **A store sized while another store holds its bucket.** I found no way. |
| + - A record with only a grant block is counted (`case rec.Grant != nil`). |
| + - A teardown note naming another bucket makes the record incoherent, and an incoherent record holds every bucket it names. |
| + - The inventory reads the store records under the same `ListDirs` the pass uses, so no app prefix is skipped. |
| + - Records under invalid account names are read and held, marked `refused`. |
| + - A teardown that noted its own bucket deleted is skipped, but the ledger covers it, as described above. |
| + - Races between the inventory and the sizing are closed by the generation reads. |
| + - The re-read after sizing checks `SameStore` and that there is no teardown block, and then the generation. |
| + - Another deployment is accepted as open. |
| +2. **Stores held for good.** These are: |
| + - a departed store's retained record (19a; ruling K); |
| + - an incoherent record; |
| + - an unparsed record anywhere, which holds every store; |
| + - two apps of one account on one bucket; |
| + - an invalid bucket name; |
| + - a malformed ledger; |
| + - a reading that keeps failing; |
| + - the refused-grant block in P3-1. |
| + |
| + 19i lists all of them except the last, which it mislabels as clearing when the grant is run again. |
| +3. **Grant's take-back check.** It behaves exactly as before. |
| + - Same order: the records (`otherStoreHolders`), then the ledger read, then the open intervals appended. |
| + - Same error wrapping: "list the stores holding", "read ownership of". |
| + - The `left` filter was applied to all holders before and only to record holders now. `left` returns false whenever an open interval exists, so filtering open-interval holders by it was always a no-op. |
| + - The old code filtered out intervals with `iv.Account != acct`. The new code only drops the exact `acct/app`, but the caller's loop still skips `h.Account == acct`. |
| + - Deduplicating with `seen` only drops an interval whose holder was already listed earlier, so the first other-account holder named in `ErrBucketHeld` is the same. |
| + - The `ValidBucketName` guard cannot be reached, because `grant` refuses an invalid name first. |
| + - The only difference is the error text for a malformed record further down: it gains the `ErrRecordMalformed` prefix. |
| +4. **Cost.** The question's premise, one record read per store per day, no longer holds: `OtherHolders` is used only by take-back. Per successful pass, with N app prefixes, R records, L ledger files and P provisioned stores: |
| + - about R + N + P record Gets; |
| + - 1 List and L Gets of ledgers, plus 2P more ledger Gets; |
| + - P bucket listings, as before; |
| + - `ListDirs` over `apps/` twice. |
| + |
| + That is linear and normally once a day. A failing reading repeats it every hour until 23:00. Nothing bounds N. L only grows, because ledgers are never deleted, and the inventory GETs whole ledger bodies to obtain ETags. Head or list ETags would do; this was already noted. |
| +5. **Tests that pass without their code.** None in this diff. Within the decode test, two of the four shapes would pass on the old code, as the table notes; the test as a whole still pins the change. |
| +6. **Claims the code contradicts.** These two: |
| + - walk.go:1622, "reads every store record" (P3-2). |
| + - OUTSTANDING 19i, "until it is run again" (P3-1). |
| + |
| + 19i's closing sentence, "the fixes after the fourth reading are unread", becomes stale after this reading. |
| + |
| +## Wrapping decode errors in `ErrRecordMalformed` |
| + |
| +It changes no other caller's behaviour. No production code checks the type of an error from `objstore.Get` or `Inspect`. With two `%w` verbs (Go 1.26), `errors.As` to the json error types still works. The API doors log the error and return fixed sentences, so only log and error text change. `storeHolders`, and through it take-back, minted-grant checks and teardown, still fails on a malformed record exactly as before. |
| + |
| +Separately, and older than this branch: `ownership.go:441` treats *any* `Get` error as "no store". So the app-delete door goes ahead without a teardown for an app whose store record is malformed. Worth its own item. |
| + |
| +## Verdict |
| + |
| +**Accept with fixes.** Both fixes are wording only: 19i and the comment at walk.go:1622. No code change is required. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index 75a76e159..436c5aa3f 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -65,7 +65,7 @@ Each is concrete. None is waived. |
| 19. **No review of the whole branch has passed.** Astra rejected rounds one to sixteen; Opus accepted each with fixes. Round seventeen read only the diff since round sixteen: Opus accepted, Astra accepted with fixes (one P2, fixed since and unconfirmed). That is not a pass of the branch: the last whole-branch reading was round sixteen's, and it was rejected. Every round has closed the shapes the round before reported and found neighbouring ones, in the store walks since round seven. Some concern records no walk of this build writes; others need only this build's ordinary walks (a failed rotation, a retained teardown, a scanning door racing a teardown), and two were faults in the previous round's own fixes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings were about records no walk of this build writes (a teardown note for another bucket, a record naming two buckets) being acted on by teardown, rotation and the scanning doors; a rotation started after a grant that failed at its last write; and a take-back that returned access to a bucket another account holds, through the member's own door. Round thirteen read those fixes: closed for their reported shapes, Astra rejected again (two P1, four P2) and Opus accepted with fixes (one P2, five P3). The new findings were paths that read such a record and acted without the check (the holder inventory, the storage pass, the declare door), a rotation's own failed last write, and a retained record blocking another account's take-back. The check now sits in the record's reader and writer. Round fourteen read those: closed for their reported shapes; Astra rejected (two P1, two P2), Opus accepted with fixes (two P2, two P3). Round fifteen read those: all closed; Astra rejected (one P1, one P2), Opus accepted with fixes (one P2, two P3), all about readers of a bucket that a retained teardown left to another account (a sweep in flight, the scan status, the storage pass). Round sixteen read those: all closed, and one of them had made a single store under teardown hold storage for every account. Astra rejected (one P1, two P2), Opus accepted with fixes (one P1, one P2, two P3). The storage hold is now per account; two findings are left open as 19f and 19i. Those fixes are on the launch branch and unconfirmed until the next round reads them. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| 19g. **Refusals whose doors say the wrong thing, and records only a person can repair.** A record that names two buckets (`ErrTwoBuckets`) or whose teardown note is about another bucket (`ErrUnfinishedTeardown`) is refused by the record's one reader and one writer, so nothing acts on it and nothing writes it; the settings page shows such a store as unreadable. No walk of this build writes either; whether an earlier build left such records in production has not been checked. There is no operator verb to repair one. The doors that meet these refusals answer with sentences written for other failures: the grant and provision doors say to run the grant again, the destroy door and the purge say to try again, the scanning doors say to try again or to re-run the grant. The same goes for a rotation refused because a grant must be run again first (`ErrGrantUnfinished`) and for a member's take-back refused because another account holds the bucket (`ErrBucketHeld`). Correct copy for each needs approval (A). |
| -19i. **A live bucket shared by two stores was sized in full for each of them.** Fixed on this branch (`feat/carloku-storage-sole-holder-20261001`), not on the launch branch: the daily storage pass sizes a store only when it is the sole holder of its bucket, by one inventory of the store records per pass and the bucket's ownership ledger, read before and after the sizing. Otherwise the store is held for its own account. Holds that last until a person acts: a store that left the bucket keeps its record, and holds the remaining store, until its retained teardown is finished, which nothing runs (19a); a record no walk can follow, until it is repaired by hand (19g); another store's grant that died after writing its grant block, until it is run again; two apps of one account on one bucket, always; any store record that will not parse holds every store. Whose bytes a departed store's leftover objects are needs Paul's ruling (approval K). Not covered: one bucket adopted in two deployments is invisible to each and is sized in full in both. The walks' own holder scan still skips records under names no build writes. A store on a bucket name this build keeps no ledger for is held, for good: its ownership evidence cannot be checked, and nothing short of moving the store to a valid name changes that. A ledger that will not parse holds its bucket's stores until it is repaired by hand. A grant block written after the inventory's walk, by a grant whose ledger write then failed, is not seen; that store has no credential, so the bytes sized are the bucket's own store's. A reading the store fails to return (a record, a ledger, a bucket listing) fails the pass for a retry until the day's last hour (23:00 UTC), when the day is written with the affected stores held; a pass that keeps failing repeats every listing and sizing each hour until then, and a reading that never succeeds (a bucket deleted out of band, a ledger the console may not read) does that every day. Open: the pass runs hourly, so the last hour has about one tick, and a failure of the listings it starts with, of the day's own write, or a pass that runs past that tick, still leaves the day unwritten, which holds storage for every account; it needs a short retry timer after 23:00. Reviewed four times (`DISPOSITIONS-storage-sole-holder.md`): Astra rejected each time, Opus accepted with fixes each time, the fourth time with P3 findings only; the fixes after the fourth reading are unread. |
| +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 and the sharing it then creates is itself resolved, or, for a grant that was refused and whose undo write failed, until the block is removed by hand; a record under an account or app name no build writes, which no walk can touch, until it is removed by hand; an open ledger interval of a store whose record is gone, until the ledger is reconciled by hand; two apps of one account on one bucket, always; any store record that will not parse holds every store. Whose bytes a departed store's leftover objects are needs Paul's ruling (approval K). Not covered: one bucket adopted in two deployments is invisible to each and is sized in full in both. The walks' own holder scan still skips records under names no build writes. A store on a bucket name this build keeps no ledger for is held, for good: its ownership evidence cannot be checked, and nothing short of moving the store to a valid name changes that. A ledger that will not parse holds its bucket's stores until it is repaired by hand. A grant block written after the inventory's walk, by a grant whose ledger write then failed, is not seen; that store has no credential, so the bytes sized are the bucket's own store's. A reading the store fails to return (a record, a ledger, a bucket listing) fails the pass for a retry until the day's last hour (23:00 UTC), when the day is written with the affected stores held; a pass that keeps failing repeats every listing and sizing each hour until then, and a reading that never succeeds (a bucket deleted out of band, a ledger the console may not read) does that every day. Open: the pass runs hourly, so the last hour has about one tick, and a failure of the listings it starts with, of the day's own write, or a pass that runs past that tick, still leaves the day unwritten, which holds storage for every account; it needs a short retry timer after 23:00. Reviewed five times (`DISPOSITIONS-storage-sole-holder.md`): Astra rejected the first four, Opus accepted each with fixes; the fifth reading, of the last diff, was accepted with fixes by both, P3 only. Those last fixes 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 5acce0040..3e66a4042 100644 |
| --- a/internal/console/economicspass.go |
| +++ b/internal/console/economicspass.go |
| @@ -2490,8 +2490,16 @@ func (s *Server) sizeSoleHolder(ctx context.Context, inventory *objstore.HolderI |
| 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 |
| + // One store can be in both: counted once. |
| + others := map[string]bool{} |
| + for _, h := range inventory.Others(bucket, acct, app) { |
| + others[h.Account+"/"+h.App] = true |
| + } |
| + for _, h := range onLedger { |
| + others[h.Account+"/"+h.App] = true |
| + } |
| + if len(others) > 0 { |
| + return fmt.Sprintf("it shares its bucket with %d other store(s)", len(others)), nil |
| } |
| n, serr := s.sizeStore(ctx, rec.Provisioned) |
| if serr != nil { |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 83920d1e9..f54898644 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -2442,3 +2442,55 @@ func TestStoragePassReadsItsInventoryOnce(t *testing.T) { |
| t.Fatalf("the declared-only app's record was read %d times in a pass over three stores; want 2 (the inventory and its own visit)", counting.seen) |
| } |
| } |
| + |
| +// A ledger that parses and is not a valid ledger for its bucket (it names |
| +// another bucket) is not ownership evidence. The stores on that bucket are |
| +// held, at once, like a ledger that will not parse. |
| +func TestStoragePassHoldsOnALedgerThatParsesAndDoesNotValidate(t *testing.T) { |
| + const bucket = "carlos-store-mine" |
| + fx, _ := soleStoreFixture(t, bucket) |
| + ctx := context.Background() |
| + storageMidDay(t, "2026-08-12") |
| + _, etag, err := fx.rel.Get(ctx, objstore.BucketOwnershipKey(bucket)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + wrong := `{"v":1,"bucket":"carlos-store-somebody-elses","intervals":[{"account_sqid":"` + fixtureAcct + `","app":"` + fixtureApp + `","from":"2026-08-10T09:00:00Z"}]}` |
| + if _, err := fx.rel.Put(ctx, objstore.BucketOwnershipKey(bucket), []byte(wrong), etag); err != nil { |
| + t.Fatal(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.Fatalf("the pass over a ledger that does not validate: %v", 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) |
| + } |
| +} |
| + |
| +// Whether there is time left in the day is asked when a reading fails, not |
| +// when the pass began. A pass that starts before the last hour and meets a |
| +// failed reading after 23:00 writes the day with the store held: failing |
| +// then would leave the day unwritten, with no tick left to write it. |
| +func TestStoragePassAsksForTheLastHourWhenAReadingFails(t *testing.T) { |
| + const bucket = "carlos-store-mine" |
| + fx, _ := soleStoreFixture(t, bucket) |
| + ctx := context.Background() |
| + prevClock := storageClock |
| + t.Cleanup(func() { storageClock = prevClock }) |
| + now := time.Date(2026, 8, 12, 22, 50, 0, 0, time.UTC) |
| + storageClock = func() time.Time { return now } |
| + // The ledger's second read (before the sizing) fails, and by then the |
| + // pass has run into the last hour. |
| + fx.srv.cfg.Releases = &nthGet{Store: fx.rel, key: objstore.BucketOwnershipKey(bucket), n: 2, fail: true, |
| + hook: func() { now = time.Date(2026, 8, 12, 23, 10, 0, 0, time.UTC) }} |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Fatalf("a reading that failed after 23:00 in a pass begun before it: %v; want the day written", err) |
| + } |
| + day, written := storageDay(t, fx, "2026-08-12") |
| + if app := day.Apps[fixtureAcct+"/"+fixtureApp]; !written || !app.StoreUnswept { |
| + t.Fatalf("day written %v, %+v; want the day written with the store held", written, app) |
| + } |
| +} |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index f372b325d..89cb089a5 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -1619,8 +1619,10 @@ func SameStore(a, b *Record) bool { |
| // interval at all has an unknown history, and holds. A record no walk can |
| // follow (Coherent) holds every bucket it names whatever the ledger shows. |
| // |
| -// It reads every store record in the deployment. For a bucket name this |
| -// build would refuse there is no ledger, and the records alone answer. |
| +// It reads every store record under a valid account and app name (the |
| +// walks' own scan, storeHolders, passes over any other; HolderInventory does |
| +// not). For a bucket name this build would refuse there is no ledger, and |
| +// the records alone answer. |
| func OtherHolders(ctx context.Context, s release.Store, bucket, acct, app string) ([]OwnerRef, error) { |
| holders, err := otherStoreHolders(ctx, s, bucket, acct, app) |
| if err != nil { |