Close round thirteen: the record's reader and writer refuse a record no walk can follow, and a rotation marks its key change
Round thirteen (Astra reject: two P1, four P2; Opus accept with fixes: one P2, five P3) found every round twelve finding closed for its reported shape, and found paths that read such a record and acted without the check. - objstore.Get and objstore.Put make the check (Coherent). No door, pass or walk can act on such a record or write one. Inspect is the explicit reader for the holder inventory and for display. - The holder inventory counts a refused record as a holder of every bucket it names. - A grant resumed over a grant block that names no bucket records the name before it creates the bucket. - The key-change mark moved to the record (Record.Delivering) and Rotate writes it too. A rotation whose delivery left the app's config exactly as it was takes the mark off. - A deletion instant with no bucket name is refused. - Scanning doors refuse a store whose teardown has begun. - A take-back is not held by a record whose store's ledger interval is closed. - The console purge sends a teardown-only record through Teardown. Each fix has a reproduction and was mutation-checked in a scratch copy. The round's reports, dispositions and the round fourteen brief are added. No review round has passed. Billing is off; nothing attests.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
16 files changed,
+1031
−122
docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md+25 −0docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round13.md+147 −0docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round14.md+29 −0docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round13.md+147 −0implementation/carloku-paid-launch/AGENT_HANDOFF.md+1 −1implementation/carloku-paid-launch/OUTSTANDING.md+2 −2internal/console/apiscan.go+20 −10internal/console/apiscan_test.go+118 −20internal/console/deletion_test.go+36 −0internal/console/economicspass_test.go+35 −0internal/console/ownership.go+8 −3internal/objstore/objstore.go+39 −8internal/objstore/ownership.go+22 −4internal/objstore/ownership_test.go+282 −23internal/objstore/sweep.go+0 −6internal/objstore/walk.go+120 −45
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md b/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md| index 9d0cc2eda..2676b6818 100644 |
| --- a/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| +++ b/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| @@ -408,3 +408,28 @@ Round eleven refused two kinds of record that no walk writes, each in the walks |
| | Opus, asked and not found: Grant's final-write retry could land a provisioned block over a newer grant block if the signing fence did not hold | Noted. It rests on the fence, which is observed on AWS only (19d). Not changed. | none | none | |
| Mutation checks in a scratch copy, each failing its test: the note check off; the check removed from each rotation verb, from the sweep, from the scanning writer; the purge ignoring a teardown block; the delivering mark not written, not read by rotation, not read by the health check; the member take-back check off, and its ledger half off. Storage simulation: 3,000 seeds on this code. |
| + |
| +## Round thirteen (reviewed `d0a21aa1`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-carloku-paid-launch-overnight-round13.md`, `OPUS-REVIEW-carloku-paid-launch-overnight-round13.md`. |
| + |
| +Both found every round twelve finding closed for its reported shape. Astra: two P1, four P2. Opus: one P2, five P3. Nothing that attests, reaches Stripe with a switch unset, or makes an open late-request defect reachable. |
| + |
| +Round twelve put one check in front of the paths that had been reported. Round thirteen found paths that read a record and acted without it: the holder inventory, the storage pass, the declare door, the walks' second read. The check is now made by the record's one reader and one writer (`objstore.Get`, `objstore.Put`), so there is no path left to forget it; `objstore.Inspect` is the explicit reader for the two places that must see such a record. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra F3, F6; Opus N4, N5: the storage pass sizes a bucket from a record that names two; the declare door rewrites a refused record; Grant and Teardown check only their first read | Accepted. `Get` refuses a record no walk can follow, as it refuses one that will not parse; every caller already holds on a read error. `Put` refuses to write one. The explicit checks of round twelve are removed. | `objstore.go` `Get`, `Put`, `Inspect` | `TestStoragePassDoesNotMeasureARefusedRecord`, `TestDeclareDoorDoesNotRewriteARefusedRecord`, `TestWalksRefuseARecordThatNamesTwoBuckets` (now also `Get` and `Put`) | |
| +| Astra F1: another store's teardown decides it is the last holder of a bucket because the holder inventory reads only the provisioned bucket of a record that names two | Accepted. The inventory reads records as they are. A record no walk can follow counts as a holder of every bucket it names, and its teardown block excuses nothing. | `walk.go` `storeHolders` | `TestTeardownDoesNotDestroyABucketARefusedRecordNames` | |
| +| Astra F2: a grant resumed over an older grant block that names no bucket creates the minted bucket, and a teardown after its failed last write cannot find it | Accepted. The block is made to name the bucket in the write that counts the walk, before anything is created. | `walk.go` `Grant` | `TestResumedGrantRecordsTheBucketItIsAboutToCreate` | |
| +| Opus N1: a rotation that fails at its last write leaves the same state as the grant in round twelve, unmarked | Accepted. The mark moved from the grant block to the record (`Record.Delivering`) and Rotate writes it before its first key change. A rotation whose delivery left the app's config byte for byte as it was takes the mark off, so the existing resume after a failed delivery still works. | `walk.go` `Rotate`; `objstore.go` | `TestRotationIsRefusedAfterARotationThatFailedAtItsLastWrite`, `TestRotateResumesAfterResealFailure` (unchanged, still passes) | |
| +| Opus N2: a deletion instant with no bucket name beside it is read as "this record's bucket was deleted" | Accepted. Refused by the reader. Only builds of this branch before round ten could have written one; no production record has a teardown block. | `walk.go` `Coherent` | `TestADeletionNoteWithNoBucketNameIsRefused` | |
| +| Opus N3: an operator's scanning door takes a store back as a side effect of its re-grant | Accepted. The scanning record writer, and the scan grant and revoke doors before they call the scanner, refuse a store whose teardown has begun. | `console/apiscan.go` | `TestScanRecordWriterRefusesARecordNoWalkCanFollow`, `TestScanDoorsDoNotReachTheScannerForARefusedRecord` | |
| +| Astra F4: a retained record of a store that has left blocks another account's take-back | Accepted. A holder whose ledger interval is closed is not counted; a holder with no interval at all still is. | `walk.go` `Grant`; `ownership.go` `left` | `TestMembersTakeBackIsNotHeldByARecordWhoseStoreHasLeft` | |
| +| Astra F5: the purge can neither remove nor pass a teardown-only record that names nothing | Accepted. The console purge sends any record with a teardown block through Teardown first. | `console/ownership.go` | `TestPurgeHandlesATeardownOnlyStoreRecord` (removed when it names nothing, refused intact when it names a bucket), `TestTeardownRemovesATeardownOnlyRecordThatNamesNothing` | |
| +| Opus N6: the scan doors' own checks were untested | Accepted. | `apiscan_test.go` | `TestScanDoorsDoNotReachTheScannerForARefusedRecord` (three refused shapes, three doors, scanner not enabled, record unchanged) | |
| +| Opus, claims: the mark "stops reaping the key the app was given" and the grant "repairs in full" | Corrected. The mark makes the state visible and refuses the rotation; the repairing grant reaps and replaces the key the app held. | `objstore.go`, `ownership.go`, `AGENT_HANDOFF.md` | none | |
| +| Both, claims: "every path that acts on a store record checks first" | Made true by moving the check into the reader and the writer, and reworded. | `AGENT_HANDOFF.md` item 5, `OUTSTANDING.md` 19g | none | |
| +| Both, noted: a record written by an earlier build carries no mark, and a failed delivery under that build still looks healthy | Accepted, not fixable from the record. Stated in the field's comment and the handoff. | `objstore.go` | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the reader's check off; the writer's check off; a refused record not counted as a holder; the grant block not made to name its bucket; the rotation's mark not written, and not read; the config comparison off; the closed-interval reconciliation off; the blank-note refusal off; the scanning writer's and the scan grant door's teardown refusals off; the purge not routing a teardown-only record. Storage simulation: 3,000 seeds on this code. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round13.md b/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round13.md| new file mode 100644 |
| index 000000000..d676fff50 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round13.md |
| @@ -0,0 +1,147 @@ |
| +# Round-thirteen review: `d0a21aa1` |
| + |
| +Reviewed against base `aed42815`, concentrating on `1395f7bf..d0a21aa1`. Read-only review: no files changed, tests run, network commands, Stripe CLI or AWS CLI. Test counterfactuals below are based on reading. |
| + |
| +All locations refer to `d0a21aa1`. **O** = `internal/objstore`, **C** = `internal/console`, **R** = `internal/release`, **D** = `implementation/carloku-paid-launch`, **S** = `docs/superpowers/specs`. |
| + |
| +## 1. Round-twelve dispositions |
| + |
| +| Finding or claim | Disposition | Fix and test | Would the test pass without the fix? | |
| +|---|---|---|---| |
| +| Astra F1: teardown trusts another bucket’s deletion note | **Closed for the reported shape.** | `Coherent` at `O/walk.go:1508`, called before teardown’s declaration-only branch at `:864`. `TestWalksRefuseATeardownNoteAboutAnotherBucket`, `O/ownership_test.go:2929`. | **No.** Removing the teardown check lets it skip the live bucket and delete the record; the refusal assertion fails. | |
| +| Opus N3: teardown or purge discards a teardown-only record naming another bucket | **Closed for the reported shape.** | `O/walk.go:1517`; `R/trash.go:301`, `:309`. Tests: `TestGrantIsRefusedOverATeardownNoteForAnotherBucket`, `O/ownership_test.go:2676`, now invokes teardown at `:2702`; purge’s `teardown-only` case at `R/trash_test.go:384`. | **No.** Without the respective checks, teardown or purge deletes the record. | |
| +| Astra F2: rotation after a failed grant reaps the delivered key; re-grant incorrectly reports health | **Closed for grants performed by this build.** Earlier records have the limitation described below. | Mark persisted at `O/walk.go:482`; health checks it at `:473`; rotation refuses at `:806`; success clears it at `:564` and `:601`. `TestRotationIsRefusedAfterAGrantThatFailedAtItsLastWrite`, `O/ownership_test.go:2991`. | **No.** Omitting the mark fails `:3021`; ignoring it in rotation fails `:3043`; ignoring it in health leaves the repaired record naming a dead key, failing `:3065`. | |
| +| Astra F3 / Opus N2: rotation, scanning doors and sweep act on two-bucket records | **Closed for the reported entry points and stable-record scenarios.** | Rotation checks at `O/walk.go:664`, `:750`, `:812`; sweep at `O/sweep.go:138`; scanning at `C/apiscan.go:182`, `:352`, `:411`. Tests: `O/ownership_test.go:2852`; `C/apiscan_test.go:555`. | **No** with the complete respective protection removed. Removing only an outer rotation check still passes the two-bucket test because the shared lease helper checks again. The scanning-writer test would pass without the separate pre-provider checks in the grant/revoke handlers. | |
| +| Opus N1: member take-back restores access to another account’s bucket | **Closed for the two reported holder shapes.** Holder discovery has additional defects in F1 and F4 below. | Non-operator check at `O/walk.go:277`, including open intervals at `:287`. `TestMembersTakeBackIsRefusedWhileAnotherAccountHoldsTheBucket`, `O/ownership_test.go:3079`. | **No.** Removing the check admits the member; removing its ledger half admits the ledger-only fixture. | |
| +| Opus N4: adopted-bucket test reuses the first case’s already-granted record | **Closed.** | Fixture construction now occurs inside the loop at `O/ownership_test.go:2554`; the initial no-grant condition is asserted at `:2584`. | **No** if fixture reuse is restored while retaining the new assertion: the second iteration already has a grant block. | |
| +| Both reviews: “before it calls the provider at all” | **Closed in the handoff and dispositions.** | `D/AGENT_HANDOFF.md:35`; `S/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md:406`. Mutation ordering remains covered by `O/ownership_test.go:2728`. | The mutation-ordering test fails if `EnsureUser` precedes the count. It does not establish absence of read-only probes. No wording test. | |
| +| Both reviews: every path leaves an incoherent record untouched | **Not closed as a universal claim.** The named walk fixes work; other consumers remain outside the check. | Claim: `O/walk.go:1479`, `D/OUTSTANDING.md:67`, dispositions `:397`. Existing tests above cover particular entry points. | Those tests pass with the omissions in F1, F3 and F6 still present. | |
| +| Both reviews: “finish the teardown” is the recovery; only the grant door has misleading copy | **Documentation closed; door copy accepted and left open.** | Sentinel documentation at `O/ownership.go:81`; expanded door inventory at `D/OUTSTANDING.md:67`; dispositions `:407`. | No wording regression. The behavioral tests above establish refusal, not useful member guidance. | |
| +| Astra claim: everything named by the record is gone at final deletion | **Closed for the mismatched-note example, not generally.** | Original fix and test in the first row. The unconditional description remains at `O/walk.go:1250`; F2 supplies another counterexample. | The mismatched-note test fails without its fix but does not cover F2. | |
| +| Opus concern about final-write retry when the signing fence does not hold | **Accepted and left open as dispositioned.** | Dispositions `:408`; `D/OUTSTANDING.md:72`. | No new test or backend evidence. | |
| + |
| +## 2. New findings |
| + |
| +### F1. P1 — Holder discovery overlooks the second bucket of a refused record |
| + |
| +**Where:** `O/walk.go:1333`, `:1342`, `:1353`; teardown caller at `:1005`; `O/ownership.go:485`. |
| + |
| +**Scenario:** |
| + |
| +1. An older record for B has `Provisioned.Bucket=X`, `Grant.Bucket=Y`, and a live user whose policy reaches Y. B has no ownership interval on Y. This is the legacy split-record class the new refusal expressly protects. |
| +2. A has a coherent, operator-adopted store on Y. |
| +3. A runs teardown. `storeHolders` reads B but does not check coherence or inventory both names. Because `Provisioned` exists, it considers only X. |
| +4. Y’s ledger has no B interval to supply the missing hold. A is judged the last holder. |
| +5. Teardown empties and deletes Y, including B’s objects. |
| + |
| +Refusing direct operations on B does not protect its data from another store’s teardown. The new member take-back check also consumes this incomplete inventory at `O/walk.go:278`. |
| + |
| +**Reachability:** The inconsistent B record requires an older build or an edited record; this build need not create it. Its presence in production is unknown. |
| + |
| +**Smallest acceptable fix:** Consider every bucket a record names when discovering potential holders. If a matching record is incoherent, refuse the destructive or access-granting decision instead of silently selecting its provisioned name. Add a regression with a split legacy holder, no ledger interval, and another store tearing down its grant bucket. |
| + |
| +### F2. P1 — Resuming a bucketless grant creates a bucket that teardown cannot find |
| + |
| +**Where:** `O/walk.go:186`, `:299`, `:481`, `:529`, `:565`, `:971`, `:1251`. |
| + |
| +**Scenario:** |
| + |
| +1. Start with an older or edited record containing a valid declaration and a non-nil grant block whose `Bucket` is empty; `Provisioned` is nil. |
| +2. `Coherent` accepts it. Grant resolves the deployment’s minted name Y. |
| +3. Because a grant block already exists, `wroteGrant` is false. The resolved Y is never copied into that block. |
| +4. Grant writes the walk count and `Delivering=true`, creates Y and its user, and delivers credentials. |
| +5. Fail the final record write before it takes effect. The persisted record still names no bucket. |
| +6. Run teardown. It takes the no-bucket lease, reads the empty grant bucket, skips bucket destruction and ownership closure, deletes the user and configuration, and deletes the record successfully. |
| +7. Y and its objects remain without a store record. |
| + |
| +This build does not originate an empty grant bucket from a fresh declaration. It **does preserve and rewrite one during a resumed grant**, and creates the otherwise-unrecorded estate itself. This is an additional accepted record shape that the walks interpret inconsistently. |
| + |
| +**Smallest acceptable fix:** Before side effects, either refuse an unresolved grant block or persist its resolved bucket under the lease. Require every resumed grant that creates estate to leave teardown a recorded bucket name. Test the failed-final-write followed by teardown. |
| + |
| +### F3. P2 — The daily storage meter records refused records as successfully measured |
| + |
| +**Where:** `C/economicspass.go:201`, `:211`, `:224`, `:850`, `:1021`. |
| + |
| +**Scenario:** |
| + |
| +1. A store has `Provisioned.Bucket=X`, `Grant.Bucket=Y`; its actual delivered credentials use Y. |
| +2. The daily storage pass reads it without calling `Coherent`. |
| +3. It sizes X using the deployment’s credentials and records those bytes against the app, with `StoreUnswept=false`. |
| +4. The daily object is create-only. Later passes return immediately because it exists. |
| +5. Completeness accepts that storage measurement, and the statement folds X’s bytes into the account’s storage meter. |
| + |
| +For example, X can contain 100 GB while the app’s actual Y contains 1 GB. The record’s ambiguity becomes a durable attributed measurement rather than a hold. Billing remains off; the incorrect evidence is written independently of that switch. |
| + |
| +**Smallest acceptable fix:** Check coherence before `sizeStore`. Treat refusal as unswept and preserve incomplete storage evidence. Add a test asserting no tenant-bucket sizing call and no complete storage day for either refused shape. |
| + |
| +### F4. P2 — A retained record blocks another account’s restoration after its access has ended |
| + |
| +**Where:** `O/walk.go:277`, `:293`, `:1353`; compare `O/ownership.go:485`. |
| + |
| +**Scenario, using this build’s ordinary walks:** |
| + |
| +1. A and B, in different accounts, have operator-adopted stores on X. |
| +2. B tears down first. Its credentials are revoked, its interval closes, and its retained record remains. |
| +3. A tears down last. The teardown decision correctly disregards B’s record because the ledger already knows B’s closed interval. A deletes X and records its destruction, then fails deleting A’s user. |
| +4. A’s member retries through the provision door. |
| +5. The new take-back check collects B’s retained record and rejects A with `ErrBucketHeld`. Unlike the teardown decision, it never reconciles that record against B’s closed interval. |
| +6. The refusal repeats until B’s retained record is cleaned up, or an operator grants A. |
| + |
| +The ledger establishes that B’s access ended and X was destroyed. This extends open item 19a: an unfinished cleanup record now prevents **another account’s** legitimate restoration. |
| + |
| +**Smallest acceptable fix:** Reconcile record holders with ownership history before rejecting a take-back. Preserve holds for unknown histories and open intervals; distinguish a retained record whose access has demonstrably ended. Add the sequential two-account teardown and restoration regression. |
| + |
| +### F5. P2 — The console purge skips a teardown-only record that teardown could remove |
| + |
| +**Where:** `R/trash.go:309`; `C/ownership.go:441`, `:502`; `O/walk.go:875`. |
| + |
| +**Scenario:** |
| + |
| +1. A trashed app has a declaration and `Teardown{StartedAt: ...}`, with no grant, provisioned block, deleted-bucket name or deletion timestamp. |
| +2. `Coherent` accepts this record. Calling `Teardown` would remove it through the declaration-only branch. |
| +3. The console purge invokes teardown only when Grant or Provisioned exists, so it skips this record. |
| +4. The newly strengthened release purge refuses every non-nil teardown block. |
| +5. Every retry follows the same path and fails. |
| + |
| +This is a harmless legacy/edited teardown-only shape, distinct from the unresolved bucket evidence intentionally held by 19g. I found no ordinary fresh-store walk that originates it. |
| + |
| +**Smallest acceptable fix:** Route teardown-only records through `Teardown` before purging. Let its coherence and backend checks decide whether removal is safe. Test both the removable shape and the named-bucket shape that must remain refused. |
| + |
| +### F6. P2 — The declaration door still rewrites records that `Coherent` refuses |
| + |
| +**Where:** `C/apistore.go:295`, `:322`, `:338`; `O/walk.go:1517`. |
| + |
| +**Scenario:** |
| + |
| +1. A record contains a declaration and a teardown note naming X, but no Grant or Provisioned block. |
| +2. Grant and teardown refuse it with `ErrUnfinishedTeardown`. |
| +3. A member submits `store create` with another valid environment prefix. |
| +4. The declaration door checks neither coherence nor the teardown block. Its prefix lock checks only Grant and Provisioned. |
| +5. It changes the declaration’s prefix, author and timestamp, persists the record, and returns success. |
| + |
| +A record documented as frozen for operator repair is therefore still writable through a member door. |
| + |
| +**Smallest acceptable fix:** Call `Coherent` immediately after reading an existing record, before modifying its declaration. Test unchanged bytes and refusal for a teardown-only note and for a split record with a missing declaration. |
| + |
| +### Other requested checks |
| + |
| +- **Delivering:** I found no coherent current-build grant that reaches reap or mint before persisting the mark, or completes successfully while leaving it set. Both final-write paths clear it. On an earlier-build record, absence decodes as false: that means **no recorded mark**, not proof that its last grant completed. An older failed delivery can still look healthy and admit rotation. |
| +- **Health before count:** An `EnvDelivered` read error now also prevents the policy rewrite at `EnsureUser` (`O/walk.go:474`, `:519`). The request reports failure. I found no unmarked reap or mint caused by this ordering. |
| +- **Source values:** The take-back check treats every value other than the exact operator constant as non-operator. The member provision door supplies `GrantSourceAccountApproval` itself (`C/apistore_provision.go:54`); its request body cannot select operator authority. |
| +- **New refused records:** I found no ordinary sequence beginning with a fresh declaration and using this build’s writers that creates either existing `Coherent` refusal. F2 concerns an accepted legacy shape that this build rewrites and acts upon. |
| + |
| +## 3. Claims the code contradicts |
| + |
| +| Location | Claim | Contradiction | |
| +|---|---|---| |
| +| `D/AGENT_HANDOFF.md:35`; `D/OUTSTANDING.md:67`; round-twelve dispositions `S/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md:397` | Every path acting on a store record checks coherence first and leaves refused records untouched. | Holder discovery, daily storage measurement and declaration writing remain outside the check: F1, F3 and F6. | |
| +| `D/AGENT_HANDOFF.md:35`; dispositions `:404` | A non-operator take-back is refused when another account’s record names the bucket. | Discovery considers only the provisioned bucket when both blocks exist, overlooking the grant bucket: F1. It also treats demonstrably closed retained records as current holders: F4. | |
| +| `O/walk.go:1075`, `:1250` | A bucketless grant cannot come from the walk; everything named is gone at final deletion. | A resumed bucketless grant persists that shape while creating Y, and teardown subsequently loses Y’s store record: F2. | |
| + |
| +`OUTSTANDING.md` item 19 correctly calls the fixes unconfirmed. Its broader assurance in 19g remains unsupported. |
| + |
| +The tests cover less than the universal claims: the scanning regression exercises the shared record writer, not the pre-provider handler guards; the purge regression covers a teardown-only record that names a bucket, not the removable shape in F5; and holder regressions use one unambiguous bucket per record. The documented mutation-run results were not re-executed. |
| + |
| +## 4. Verdict |
| + |
| +**Reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round14.md b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round14.md| new file mode 100644 |
| index 000000000..52a2daae7 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round14.md |
| @@ -0,0 +1,29 @@ |
| +# Review brief, round 14: Carloku paid launch, overnight changes of 2026-10-01 |
| + |
| +Read the round 1 brief (`CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight.md`) for the fixed rulings and the output rules, the round 3 to 13 briefs for rulings 6 to 12, then `CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md` (its last section covers round 13) and `implementation/carloku-paid-launch/OUTSTANDING.md`. |
| + |
| +Review the commit named in the prompt. Read source only with `git show <commit>:<path>`. The changes since round 13 are `git diff d0a21aa1 <commit>`. The code changes are in `internal/objstore` (`objstore.go`, `walk.go`, `ownership.go`, `sweep.go`, tests), `internal/console` (`apiscan.go`, `ownership.go`, tests). The rest is documents. |
| + |
| +Still the fixed snapshot: round 13's findings, nothing else. Nothing may attest. The quantity worker has two switches, both unset. Billing is off. |
| + |
| +Do not report again, as findings, what `OUTSTANDING.md` lists as open: items 6 and 6a, 7 and 9, 19a to 19g. Do report anything that makes one of them worse than the document says, and any claim about them the code contradicts. |
| + |
| +## First: a disposition table |
| + |
| +For every round 13 finding (Astra F1 to F6 and the claims; Opus N1 to N6 and the claims): closed, partly closed, not closed, or accepted and left open as the dispositions say. File and line of the fix and of the test, and whether the test would pass without the fix. |
| + |
| +## Then: new findings only |
| + |
| +- **The check in the reader and the writer** (`objstore.Get`, `objstore.Put`, `objstore.Inspect`, `objstore.Coherent`). The claim: nothing acts on a store record that no walk can follow, and nothing writes one, because every door, pass and walk reads through `Get` and writes through `Put`. Find a reader of the store record that does not go through `Get` and then acts (a raw `store.Get` of the record's key, a JSON parse elsewhere, a listing that infers state). Find a caller of `Inspect` that acts on what it read. Find a caller of `Get` for which an error is not a hold: one that treats the error as "no store" and goes on. Find a record this build writes that `Put` now refuses, in a path that then leaves the provider and the record apart. |
| +- **The holder inventory** (`storeHolders`). A refused record counts as a holder of every bucket it names. Find a bucket destroyed, or access given, past such a record. Find a store that can now never be torn down because of another app's refused record, where that is wrong. |
| +- **The mark on the record** (`Record.Delivering`), now also written by `Rotate`. Find a key change (reap, delete, mint, deliver) made with the mark not on the record. Find a successful walk that leaves it set. The config comparison that takes the mark off after a failed delivery: find a delivery that did reach the app while the comparison says it did not. |
| +- **The take-back checks** (a non-operator source, holders reconciled against closed ledger intervals; scanning doors refusing a store under teardown). Find access regained, or wrongly refused for good. |
| +- **The purge** (`console/ownership.go`, `release/trash.go`) and a teardown block. |
| +- **Claims** in `AGENT_HANDOFF.md` item 5, `OUTSTANDING.md` 19 and 19g, and the round 13 dispositions. |
| + |
| +## What to return |
| + |
| +1. The disposition table. |
| +2. New findings, most severe first, in the round 1 format. |
| +3. Claims the code contradicts. |
| +4. A one-line verdict: accept, accept with fixes, or reject. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round13.md b/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round13.md| new file mode 100644 |
| index 000000000..f59b315ee |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round13.md |
| @@ -0,0 +1,147 @@ |
| +# Opus production and authority review, round 13: Carloku paid launch, `aed42815..d0a21aa1` |
| + |
| +This was a read-only pass using `git show` only. I changed nothing, ran no tests and made no network calls. Every "fails without the fix?" answer comes from reading the test against the code. Since round 12 (`1395f7bf..d0a21aa1`) the code changes are in `internal/objstore` (`walk.go`, `ownership.go`, `objstore.go`, `sweep.go`, tests), `internal/console/apiscan.go` and its test, and `internal/release/trash.go` and its test. The rest of the diff is documents. |
| + |
| +Abbreviations: **O** = `internal/objstore`, **C** = `internal/console`, **D** = `implementation/carloku-paid-launch`. |
| + |
| +**Production authority** |
| + |
| +- **Nothing attests.** `SetAttestation` and `SetAllClientsInstrumented` (`usage/durabilityledger.go:1455`, `:1475`) still have no non-test caller. |
| +- **The quantity worker cannot reach Stripe with either switch unset.** `quantityWorker()` (`C/billingquantity.go:133-139`) needs both a proration behaviour and `QuantityFence == "stripe-sandbox-verified"`. `billingQuantityPass` returns before any read when it is false (`:823-826`). `cmd/console/billing.go:40` rejects any other fence value. None of these changed this round: `billingquantity.go`, `billingservice.go`, `internal/billingstripe`, `cmd/console`, `internal/usage`, `internal/sigfence`. |
| +- **The sandbox test is unchanged and has not been run.** It is skipped without `CARLOS_STRIPE_SANDBOX_KEY`, and it refuses any key that is not `rk_test_`. |
| +- **The walk lease and the backend fence are unchanged.** `walklock.go`, `fence.go` and `aws.go` did not change. My round 9 to 12 reading stands, with AWS the only backend where the fence has been observed (19d). |
| +- **Leaks.** The new `ErrBucketHeld` text names another account's sqid and app. Every door that can meet it logs it and returns a fixed sentence (`C/apistore_provision.go:60-62`, `C/apistore.go:633-640`), so nothing leaks to the member. |
| +- **Wrong billing, destroyed data.** I found no path that bills wrongly or destroys a customer's data. N1 below is availability only. |
| + |
| +## 1. Disposition table (round 12) |
| + |
| +| Finding | Status | Fix | Test; fails without the fix? | |
| +|---|---|---|---| |
| +| **Astra F1 / Opus N3**: Teardown trusts a note about another bucket, skips the live X and deletes the record; a teardown-only record is purged | **Closed for the reported shape.** A note with a timestamp and a blank bucket name is still trusted (N2). | `Coherent` `O/walk.go:1501-1524`, called by Teardown at `:864` before the declaration-only delete. Purge: `internal/release/trash.go:301`, `:309`. Sentinel doc: `O/ownership.go:81-86`. | `TestWalksRefuseATeardownNoteAboutAnotherBucket` (`O/ownership_test.go:2929`): **fails**. Teardown would return nil after deleting the record. The Teardown leg added to `TestGrantIsRefusedOverATeardownNoteForAnotherBucket` (`:2676`): **fails**, because the declaration-only path deletes the record. `teardown-only` (`trash_test.go:384`): **fails** without the `Teardown` field. | |
| +| **Astra F2**: a rotation started after a grant failed at its last write reaps the delivered key | **Closed for Grant.** Rotate's own failed last write has the same shape and is untouched (N1). The required repair also reaps the delivered key (§3). | Mark set in the count write (`O/walk.go:472-492`) and cleared at `:564` and `:600-602`. Rotation is refused at `:806-810`. | `TestRotationIsRefusedAfterAGrantThatFailedAtItsLastWrite` (`:2991`), both verbs: **fails**. Without the mark the rotation proceeds and the keys change. | |
| +| **Astra F3 / Opus N2**: rotation, the scanning doors and the sweep act on a two-bucket record | **Closed.** The check covers the first read of every walk, and the under-lease read of rotation. Grant and Teardown do not re-check the under-lease read (N4). | `Rotate` `:664`, `RotateFinish` `:750`, `leaseForRotation` `:812`, `RunSweep` `O/sweep.go:138`, `writeScanRecord` `C/apiscan.go:182`, scan grant `:352`, scan revoke `:411` | `TestWalksRefuseARecordThatNamesTwoBuckets` (`:2852`) now covers both rotation verbs and the sweep: **fails**. `TestScanRecordWriterRefusesARecordNoWalkCanFollow` (`C/apiscan_test.go:555`): **fails**. The door-level checks at `C/apiscan.go:352` and `:411` have **no test** and are not in the mutation list. | |
| +| **Opus N1**: the member's take-back regains access to a bucket another account holds | **Closed for the member's doors.** An operator's scanning door can still do it as a side effect (N3). | `O/walk.go:277-298`, in `walkEndedClean` | `TestMembersTakeBackIsRefusedWhileAnotherAccountHoldsTheBucket` (`:3079`), holder by record and holder by ledger: **fails** for each half removed. | |
| +| **Opus N4**: the adopt case ran on an already-granted record | **Closed.** Each iteration builds its own fixture. | `O/ownership_test.go:2550-2620` | The adopt iteration now **fails** under a rule that prefers the argument. | |
| +| **Claims**: "before it calls the provider at all" | **Closed.** Item 5 now says two read-only probes come first. | `D/AGENT_HANDOFF.md` item 5 | — | |
| +| **Claims**: 19g named only the grant door; "finish the teardown" | **Closed as documentation.** The door copy is accepted and left open as 19g. | `D/OUTSTANDING.md` 19g; `O/ownership.go:81-86` | — | |
| +| **Opus, asked and not found**: Grant's final-write retry rests on the fence | **Accepted and left open, as the dispositions say.** Unchanged. | — | — | |
| + |
| +## 2. New findings, most severe first |
| + |
| +### N1. P2: Rotate's own failed last write leaves the shape Astra F2 described, and the delivering mark does not cover it |
| + |
| +**Where:** `O/walk.go:690`, `:700-706`; the doc claim at `:640-643`; Grant's health check at `:472-479`. |
| + |
| +**Scenario:** |
| +1. A healthy store names K1, with no pending key. |
| +2. Rotation R1 reaps all keys except K1, mints K2 and delivers K2. Its step-5 `Put` fails with a non-CAS error that did not apply. The record names K1, the app runs on K2, and nothing on the record says so (`Delivering` is a grant-block field that only Grant sets). |
| +3. After the lease runs out, the member re-runs the rotation (R2). The estate is unchanged and there is no mark, so R2's reap (`keep=[K1]`) deletes K2, the key every host is using. No overlap remains. |
| +4. If R2's mint or delivery then fails, the app holds a dead K2. K1's secret is no longer in config. |
| +5. A grant does not repair this. The endpoint matches, every variable is present and `Delivering` is false, so the grant takes the healthy short-circuit. Only another successful rotation brings the store back. |
| + |
| +This is pre-existing. It is the shape round 12's disposition took the stricter reading on for Grant, and the fix was applied to Grant alone. |
| + |
| +**Smallest fix:** Rotate sets the same mark before step 1 (or records the minted key ID before `writeEnv`) and clears it in step 5. `leaseForRotation` already refuses on the mark, and Grant then walks in full. Add the Grant test's twin with Rotate failing at its last write. At minimum, correct the comment at `:640-643` and list the gap in OUTSTANDING. |
| + |
| +### N2. P3: `Coherent` accepts a deletion note with a timestamp and a blank bucket name, and both walks read it as "X was deleted" |
| + |
| +**Where:** `O/walk.go:1514-1522`; Teardown `:1002`; Grant `:247-267`. |
| + |
| +**Scenario:** |
| +1. The record is `Provisioned{X}`, `Grant{X}`, with `Teardown{BucketDeletedAt: t, DeletedBucket: ""}`. X is live. |
| +2. `Coherent` passes, because a blank note is not "another bucket" and `own` is not blank. |
| +3. Teardown sets `bucketDone` and skips emptying and deleting X. It deletes the user, closes the ownership interval and deletes the record. X and its objects are left with no record. |
| +4. Grant instead takes `tornBucket = X` and calls `markBucketDestroyed(X)`. That writes a false `DestroyedAt`, which lifts other stores' retained holds (`:1190`). |
| + |
| +**Can this build write it?** No. Teardown always writes both fields together (`:1122`), and Grant clears both together (`:328`). Branch builds before `84311b9f` wrote the timestamp without the name. `TeardownBlock` is not on `aed42815` or `main`, so no production record can have it. Whether any dev deployment ran those builds cannot be told from the repository. |
| + |
| +**Smallest fix:** in `Coherent`, refuse `!BucketDeletedAt.IsZero() && DeletedBucket == ""` with `ErrUnfinishedTeardown`. |
| + |
| +### N3. P3: an operator's scanning door performs a take-back as a side effect, with no holder check |
| + |
| +**Where:** `C/apiscan.go:490-527` (enforce), `:417-434` (revoke), `regrantForScanning` `:552-571`; `O/walk.go:277`. |
| + |
| +**Scenario:** |
| +1. A's store was torn down and retained because account C holds the shared X. A's record keeps `Provisioned{X}` and a teardown block. |
| +2. A's grant block is operator-sourced, or A has none (legacy; `regrantForScanning` then defaults to operator). |
| +3. An operator runs `carlos store scan enforce off` or `scan revoke` for A. |
| +4. `writeScanRecord` accepts the record, because a coherent teardown passes `Coherent`. |
| +5. The regrant runs `Grant` with the operator source. That is a full take-back: it skips the holder check, re-mints and delivers keys to A, and rejoins C's interval. |
| + |
| +The operator asked to change scanning, not to restore A's access to what C has written since. |
| + |
| +**Smallest fix:** `writeScanRecord` (or `regrantForScanning`) refuses a record with `Teardown != nil`. Scanning has nothing to converge on a store being destroyed. |
| + |
| +### N4. P3: Grant and Teardown check `Coherent` on the first read only, not on the record read under the lease |
| + |
| +**Where:** Grant `:159` versus the re-read at `:220`; Teardown `:864` versus `:920`. |
| + |
| +`estateOf` (`:1265-1280`) uses `Provisioned.Bucket` over `Grant.Bucket` and keeps only a boolean for the note. So a change to `Grant.Bucket` while `Provisioned` stays the same, or to `DeletedBucket` while the timestamp stays, passes the estate comparison: |
| +- Grant then walks a two-bucket record. |
| +- Teardown takes a note about another bucket at its word, which is the round 12 F1 outcome. |
| + |
| +`leaseForRotation` does re-check (`:812`). Only a hand edit or an earlier-build writer can make the change between the two reads. |
| + |
| +**Smallest fix:** call `Coherent` on the under-lease record in both walks. Under a lease nothing has been called yet, so the refusal is clean. |
| + |
| +### N5. P3: two record paths outside the claimed set do not make the check |
| + |
| +**Where:** |
| +- `C/apistore.go:339-340`: the declare door writes `Declared` onto a record that `Coherent` refuses whenever `Declared` is nil. |
| +- `C/economicspass.go:201-217`: the storage pass reads `rec.Provisioned` of a refused record, sizes that bucket and writes `StoreBytes` into a create-only day object. |
| + |
| +For a two-bucket record, which bucket holds the store's objects "cannot be told from the record" (`O/walk.go:1490`). Sizing the provisioned one is a guess. Rulings 2 and 4 say a guess holds rather than bills. My round 12 N2 showed that main's re-grant rule can write such records, so some may exist in production. Billing is off. |
| + |
| +**Smallest fix:** the storage pass sets `StoreUnswept` when `Coherent` refuses the record. The declare door makes the check before its `Put`. Alternatively, narrow the claim (§3). |
| + |
| +### N6. P3: the scan grant and revoke doors' own `Coherent` checks are untested |
| + |
| +**Where:** `C/apiscan.go:352`, `:411`. |
| + |
| +Removing the check at `:352` lets `EnsureEnabled` reach the backend for a refused record. The subsequent `writeScanRecord` refusal then leaves the backend enabled with nothing recording it. The dispositions' mutation list names only "the scanning writer". |
| + |
| +**Smallest fix:** one door-level test per check, asserting that the fake scanner was not called. |
| + |
| +### Asked and not found |
| + |
| +- **A grant that reaps or mints before the mark is on the record.** |
| + - None. Every reap and mint (`:542`, `:545`) follows the count write (`:481-496`), including its CAS retry (`:489`). |
| + - A lost-response count returns before any provider call. |
| +- **A successful grant that leaves the mark set.** |
| + - None. The healthy return requires `!Delivering` on the record read under the lease (`:473`). |
| + - A count retry onto a `fresh` record carrying the mark would need a concurrent grant, and that grant's `Walks++` makes `estateOf` differ. |
| +- **A store refused rotation for good.** |
| + - Only while every grant re-run fails. The mark persists across failures, and any grant re-run walks in full and clears it on success. |
| + - A member whose account approval has been revoked cannot run the provision door, so an operator's grant is needed. The `ErrGrantUnfinished` copy is accepted and left open as 19g. |
| +- **Health decided before the count.** No harm found: |
| + - It is a deployment-store read under the lease. An error returns before the count and before any provider call. |
| + - The prefix cannot change in between, because the declare door refuses a prefix change once a grant block exists (`C/apistore.go:321`). |
| + - Scanning writes in between are carried by `rec = fresh`. |
| +- **The mark on an earlier build's record.** |
| + - It reads false. That means "unknown", not "finished". A grant that failed at its last write under an earlier build still leaves the round 12 F2 state, unmarked. |
| + - An earlier-build console writing the record (a rollback, or the declare and scan doors of `main`) drops the field when it re-marshals the record. |
| + - Neither can be detected from the record. |
| +- **The member's take-back.** |
| + - Grant has only two sources: `GrantSourceOperator` and `GrantSourceAccountApproval`. The check refuses anything that is not operator. |
| + - Holders that a record or an open interval cannot show (sharing across deployments, which use separate records and ledgers) are beyond this deployment's view. That cannot be determined from the repository. |
| + - A refusal by the open interval of a store whose bucket was already destroyed is masked anyway: that store's `Destroying` marker refuses every grant (`openBucketOwner`), operator or not. |
| +- **The purge and a teardown block.** |
| + - A benign teardown-only record (`Teardown{StartedAt}`, no note) is now refused by `release.PurgeApp`, but the console's destroy door clears it through Teardown's declaration-only path (`O/walk.go:875-876`). |
| + - No walk of this build writes that shape. Grant's `refused()` restores a legacy record to `Provisioned` plus teardown, never teardown-only. |
| + - The post-purge re-check (`trash.go:262-268`) ignores `teardown`. That is harmless for the same reason. |
| +- **A record this build writes that `Coherent` refuses.** None. Teardown notes `bucket`, which is `own` by construction (`:971-977`, `:1122`). Grant clears the note before creating anything. |
| + |
| +## 3. Claims the code contradicts |
| + |
| +| Location | Claim | What the code supports | |
| +|---|---|---| |
| +| `D/AGENT_HANDOFF.md` item 5; `O/walk.go:1494-1499` (`Coherent` doc); `D/OUTSTANDING.md` 19g; dispositions, round 12 rows 1–2 | Refused "before anything is touched by every path that acts on a store record" | The declare door writes the record and the storage pass bills from it without the check (N5). Grant and Teardown check the first read, not the record they act on (N4). The purge does not call `Coherent`; it refuses any teardown block, which is stronger. | |
| +| `O/walk.go:640-643` (Rotate doc) | After a failed final write "both are live, so the app keeps working and a re-run converges" | The re-run reaps the delivered key. If its mint or delivery then fails, nothing but another rotation repairs the store (N1). | |
| +| `O/objstore.go:72-78`; `O/ownership.go:88-92`; dispositions, Astra F2 row | The mark stops "reaping the key the app was given"; the grant run again "repairs in full" | The rotation is refused, but the grant re-run it requires runs `ReapKeys(keep=PrevKeyID)` (`:542`), which deletes the delivered key with no overlap. The mark makes the state visible and repairable; it does not spare the key. | |
| +| `D/AGENT_HANDOFF.md` item 5 | "A take-back over a bucket that a store of another account holds is refused unless an operator grants it" | True for an explicit grant. An operator's scanning door also performs such a take-back implicitly (N3). | |
| + |
| +Nothing in `walklock.go`, `fence.go`, `aws.go`, the evidence document, "The fence" in `billingquantity.go`, or `OUTSTANDING.md` items 6, 6a, 7, 9 or 19a to 19f is contradicted. Nothing this round makes any of them worse. |
| + |
| +## 4. Verdict |
| + |
| +**Accept with fixes.** Nothing attests, the worker cannot reach Stripe with either switch unset, nothing bills, leaks or destroys data, and every round 12 finding is closed for its reported shape. N1 (Rotate's own failed last write), N2 and N4 (`Coherent` gaps) and the wording in §3 should land before the store-walk claims are relied on. |
diff --git a/implementation/carloku-paid-launch/AGENT_HANDOFF.md b/implementation/carloku-paid-launch/AGENT_HANDOFF.md| index 7b91f7f87..12cd7269f 100644 |
| --- a/implementation/carloku-paid-launch/AGENT_HANDOFF.md |
| +++ b/implementation/carloku-paid-launch/AGENT_HANDOFF.md |
| @@ -32,7 +32,7 @@ When a review is approved, apply each string byte for byte: the site by replacin |
| 2. Console GET `/a/{sqid}/billing` (`billingpage.go`, `templates/billing.html`): current plan, owner-only chooser posting to the existing checkout door, `?country=` price preview computed from the plans terms, error and pending banners, portal button once a customer exists, read-only for members, byte-identical 404 for non-members, closed state when billing is off. `plans.Countries()` exposes the embedded country list. |
| 3. Durability ledger: the epoch read race is fixed (epoch and created-at are immutable after recover) with a `-race` regression. An unmetered replication fallback on a box whose ledger opened now files an escape (`DurabilityLedger.Escaped`): the box-day is incomplete and the owner is excluded for the run. A box whose ledger failed to open has no publisher and files nothing. The console fold asks each box's published day to prove the union of that box's durability, metrics and edge windows; a box with no ledger directory, no day file, or a ledger whose coverage starts after the box began serving holds the day for everyone. There is no tolerance. Durability days are counted per account: a box-day attested with an exclusion set is complete for every account the box did not exclude. The move worker's checkpoint is recorded against the route's owner. Nothing attests: attestation is false everywhere, in this code and in production. The scoped attestation (path A) was built, reviewed in round six, found unable to prove an exclusive writer, and removed from this branch. It is kept on `feat/carloku-scoped-attestation-20261001` with its findings open. Replication-write completeness is a launch blocker, not a waived requirement. |
| 4. `internal/awsbilling`: Cost and Usage Report reader (versioned deliveries only), newest-delivery selection, digests, charged S3 transfer classifier. It holds on a charged usage line it does not count and on any non-usage line (fee, refund, credit, discount) with a cost on a transfer usage type. Every transfer usage type the flagship account wrote from July to September 2026 is pinned in a test. |
| -5. `internal/objstore/ownership.go`: append-only bucket ownership ledger, opened at grant, closed at teardown. It resolves an owner only for a ledger that claims completeness, and only for hours that begin at or after the instant it began vouching, so a store's partial first hour never resolves (a ledger that began vouching exactly on the hour answers for that hour). Completeness needs a grant block recorded as minting, nothing delivered to the app yet, a probe showing the bucket does not exist, and an inventory showing no principal's policy names the bucket (`GranteeProber`), and is dated when both probes had answered. The AWS provisioner cannot list principals yet, so on AWS no ledger claims completeness, new stores included, and store transfer holds. A minting grant is refused while another app holds the name by ledger interval or by store record, and for a bucket that already exists and it never held. Grant, teardown and both rotation verbs are walks. A walk that spends backend authority takes a lease (`walklock.go`) on the bucket name, or on a hash of it when the name is one this build would refuse, or on a hash of the app when the record names no bucket: an object taken by compare-and-swap, bound to the invocation, thirty minutes long, standing until it runs out if the walk failed or any of its requests needed a second attempt. The record is read again under the lease and must still be the same estate, not only the same name: teardown and grant compare the grant instant, the key, how many grants have reached the provider, whether a teardown had begun, whether it noted the bucket deleted and how many take-backs it has seen; rotation compares the same estate and requires that no teardown has begun. Every grant is counted on the record before any call that changes anything at the provider, the user and its policy included (two read-only probes, of the bucket and of who can reach it, come first), so a healthy store granted again writes its record once. A grant that is about to reap and deliver keys says so in that same write (`GrantBlock.Delivering`) and takes the mark off when it records the result; while the mark stands a grant run again walks in full and a rotation is refused (`ErrGrantUnfinished`). A record's bucket is the one it is provisioned on, else the one its grant block names: the arguments choose a bucket only for a store that has none, so a grant never moves a store to another name. A record whose grant block and provisioned block name different buckets (an earlier build could write one), or whose teardown note is about a bucket that is not its own, is refused before anything is touched by every path that acts on a store record: grant, teardown, both rotation verbs, the scanning doors, the sweep and the purge (`objstore.Coherent`; `ErrTwoBuckets`, `ErrUnfinishedTeardown`). An operator repairs such a record by hand; there is no verb for it. A take-back over a bucket that a store of another account holds is refused unless an operator grants it. A failed walk's lease stands until it runs out, with one exception: the same app re-running the same kind of walk may take it at once. A declaration-only teardown and the app purge take no lease; they delete the store record only as the generation they read. The user belongs to the app and the lease to the name, so a late request of a failed teardown can still remove the policy of a store granted afterwards under another name (round eight, Opus N2, open). Every request of a walk, to S3, to IAM and to the deployment bucket that holds the records and ledgers, is signed with the instant its lease was taken (`internal/sigfence`), so AWS refuses a request a paused process sends after its lease could be taken over. The record and its scan sidecar are deleted as the generation that was read. The signing-time rule was checked read-only against S3 and IAM with a list and two gets (`TestFenceAgainstAWS`); no delete was sent. A declaration-only teardown takes no lease (there is no bucket name) and relies on the conditional delete alone. A teardown writes a block on its own record before it touches anything and decides by compare-and-swap on the ledger (for a bucket name this build would refuse there is no ledger, and it decides from the store records alone): if nobody else holds the bucket it marks the bucket as being destroyed; otherwise it leaves the bucket and its scanner, revokes its own credentials, and records that the scanner is owed. A grant by the same app over its own unfinished teardown takes the store back, runs in full, and voids the teardown's note that the bucket was deleted. A teardown that leaves a bucket to another account keeps its record and returns `ErrBucketRetained` (discussion 191). It can finish once that bucket has been destroyed, when someone runs it again; nothing runs it again by itself. For a bucket name this build would refuse there is no ledger: a store whose teardown has begun is not counted as a holder there, so the last of two sharers destroys the bucket. `ErrBucketRetained` has no member copy yet; it needs Paul's review. A randomized simulation (`walk_sim_test.go`) checks the invariants over sequential walks with provider and store faults, including failures after the effect, and walks frozen past their lease while another takes the name. It does not generate a second bucket name, rotations, stores with no ledger interval or disagreeing clocks; those have direct tests. |
| +5. `internal/objstore/ownership.go`: append-only bucket ownership ledger, opened at grant, closed at teardown. It resolves an owner only for a ledger that claims completeness, and only for hours that begin at or after the instant it began vouching, so a store's partial first hour never resolves (a ledger that began vouching exactly on the hour answers for that hour). Completeness needs a grant block recorded as minting, nothing delivered to the app yet, a probe showing the bucket does not exist, and an inventory showing no principal's policy names the bucket (`GranteeProber`), and is dated when both probes had answered. The AWS provisioner cannot list principals yet, so on AWS no ledger claims completeness, new stores included, and store transfer holds. A minting grant is refused while another app holds the name by ledger interval or by store record, and for a bucket that already exists and it never held. Grant, teardown and both rotation verbs are walks. A walk that spends backend authority takes a lease (`walklock.go`) on the bucket name, or on a hash of it when the name is one this build would refuse, or on a hash of the app when the record names no bucket: an object taken by compare-and-swap, bound to the invocation, thirty minutes long, standing until it runs out if the walk failed or any of its requests needed a second attempt. The record is read again under the lease and must still be the same estate, not only the same name: teardown and grant compare the grant instant, the key, how many grants have reached the provider, whether a teardown had begun, whether it noted the bucket deleted and how many take-backs it has seen; rotation compares the same estate and requires that no teardown has begun. Every grant is counted on the record before any call that changes anything at the provider, the user and its policy included (two read-only probes, of the bucket and of who can reach it, come first), so a healthy store granted again writes its record once. A grant or a rotation that is about to change the user's keys marks the record first (`Record.Delivering`) and takes the mark off when it records the result; a rotation whose delivery left the app's config exactly as it was takes it off too. While the mark stands a grant run again walks in full and a rotation is refused (`ErrGrantUnfinished`). The mark does not save the key the app was holding: the repairing grant reaps and replaces it. A record from an earlier build carries no mark, which means not recorded, not finished. An older grant block that names no bucket is made to name it in the write that counts the walk. A record's bucket is the one it is provisioned on, else the one its grant block names: the arguments choose a bucket only for a store that has none, so a grant never moves a store to another name. A record whose grant block and provisioned block name different buckets (an earlier build could write one), or whose teardown note is about a bucket that is not its own, is refused by the record's one reader and one writer (`objstore.Get` and `objstore.Put`; the check is `objstore.Coherent`; `ErrTwoBuckets`, `ErrUnfinishedTeardown`), so no door, pass or walk can act on one or write one. The holder inventory reads such a record anyway (`objstore.Inspect`) and counts it as a holder of every bucket it names. An operator repairs such a record by hand; there is no verb for it. A take-back over a bucket that a store of another account holds is refused unless an operator grants it; a store whose ledger interval is closed does not count as holding it; a scanning door never changes, and so never takes back, a store whose teardown has begun. A failed walk's lease stands until it runs out, with one exception: the same app re-running the same kind of walk may take it at once. A declaration-only teardown and the app purge take no lease; they delete the store record only as the generation they read. The user belongs to the app and the lease to the name, so a late request of a failed teardown can still remove the policy of a store granted afterwards under another name (round eight, Opus N2, open). Every request of a walk, to S3, to IAM and to the deployment bucket that holds the records and ledgers, is signed with the instant its lease was taken (`internal/sigfence`), so AWS refuses a request a paused process sends after its lease could be taken over. The record and its scan sidecar are deleted as the generation that was read. The signing-time rule was checked read-only against S3 and IAM with a list and two gets (`TestFenceAgainstAWS`); no delete was sent. A declaration-only teardown takes no lease (there is no bucket name) and relies on the conditional delete alone. A teardown writes a block on its own record before it touches anything and decides by compare-and-swap on the ledger (for a bucket name this build would refuse there is no ledger, and it decides from the store records alone): if nobody else holds the bucket it marks the bucket as being destroyed; otherwise it leaves the bucket and its scanner, revokes its own credentials, and records that the scanner is owed. A grant by the same app over its own unfinished teardown takes the store back, runs in full, and voids the teardown's note that the bucket was deleted. A teardown that leaves a bucket to another account keeps its record and returns `ErrBucketRetained` (discussion 191). It can finish once that bucket has been destroyed, when someone runs it again; nothing runs it again by itself. For a bucket name this build would refuse there is no ledger: a store whose teardown has begun is not counted as a holder there, so the last of two sharers destroys the bucket. `ErrBucketRetained` has no member copy yet; it needs Paul's review. A randomized simulation (`walk_sim_test.go`) checks the invariants over sequential walks with provider and store faults, including failures after the effect, and walks frozen past their lease while another takes the name. It does not generate a second bucket name, rotations, stores with no ledger interval or disagreeing clocks; those have direct tests. |
| 6. Statement folds: `console/durabilityfold.go` (`durability_writes_1k`, policy in `usage.BillableReplicationWrites`) and `console/storeegressfold.go` (`store_egress_gb`). `usage.MeterSource` names a source for both meters. Month evidence pins ownership ledgers by their month projection and knows the durability box directory. Statements carry the five billed meters by UTC day (`Statement.Days`), summing to the monthly totals; the other meters are not recorded by day. With a transfer report configured, the statements catch-up walks a 36-month window and sweeps every month that has an open statement, is missing a statement an account is owed (the platform's included), or is a gap between metered months. An account's statement that will not parse, or that cannot be read three passes running, is reported once per process and the month is classified without it. The platform's own statement is different: while it cannot be read the month stays unknown, for as long as that lasts. A month seen with no statement is remembered for six hours, a closed statement for the life of the process. |
| 7. Operator API doors (`billingoperator.go`): list and resolve billing review holds, list and set the house marker. No CLI verb yet. |
| 8. Team quantity worker (`billingquantity.go`) behind two switches, both unset: `CONSOLE_BILLING_PRORATION` (`immediate` or `next_invoice`, Paul's decision) and `CONSOLE_BILLING_QUANTITY_FENCE=stripe-sandbox-verified` (set only after `TestSandboxQuantityFence` has passed against a Stripe test-mode account; it has not been run). With either unset it makes no provider call. An operation is recorded by a compare-and-set on the billing row's revision, subscription, period, quantity and status, on the target and newest pending addition recomputed from the database, and on its own instant lying inside the period and not before the addition it charges for. The request dates its proration at that instant (`proration_date`). Sending and retiring exclude each other through a send lease on the operation's row, owned by one sender's token; an operation dated ahead of the observing clock is left alone. Every key ends in the account's operation serial. An operation nobody can show ran is acknowledged only after its idempotency key has been tried at the provider with a metadata-only request (the fence), or after a person has resolved the review it was held with: a resolved review stands in for the fence, on that person's word and with no provider check. If the fence request took the key, the operation never ran and may be retired, on a provider read taken after the fence when the subscription is current. If the key is held by the operation's own request, that request began and its outcome is not known: the operation is recorded as landed if the provider shows its target, and otherwise held with a review (`billing_quantity_operation_unsettled`) and never replaced until a person resolves it. A fence the provider refuses is held the same way. The fence rests on Stripe's documented key rules, is modelled by a fake in the tests, is not verified against Stripe, and lasts only as long as Stripe remembers a key (documented as at least 24 hours): `docs/superpowers/specs/2026-10-01-stripe-idempotency-evidence.md`. A quantity that goes down inside a period files `billing_quantity_lowered`. An operation is confirmed only by the provider's success answer to its own request. A confirmed landing takes effect at the operation's instant, so a seat's charge and its allowance start together; a quantity that merely matches an unconfirmed operation's target is recorded as `landed_unverified`, dated when observed, with a review. If the provider refuses a request and names `proration_date`, the operation's key is fenced at once and it is retired on the next read; if it does not name it, the operation waits out the 22-hour replay limit. None of the provider's side is confirmed: test mode is not authorized. Only additions of the subscription's current period can raise it; older ones are closed uncharged with a review; additions and operations of an ended subscription are closed. It does not yet lower the quantity at renewal. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index 8d95f7f6f..9ca249d89 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -63,8 +63,8 @@ Each is concrete. None is waived. |
| ### Process |
| -19. **No review round has passed.** Astra rejected rounds one to eleven; Opus accepted each with 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. Their 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 every path that acts on a store record, before anything is touched. 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). |
| +19. **No review round has passed.** Astra rejected rounds one to thirteen; Opus accepted each with fixes. Every round has closed the shapes the round before reported and found neighbouring ones; since round eleven they concern records that no walk of this build writes. 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. 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). |
| 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. |
| 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). |
| 19b. **Two round-eight findings are accepted and not fixed.** A late request of a failed teardown can remove the policy of a store the same app is granted afterwards under another name (the store goes dark; nothing is lost or leaked). A console whose clock is far ahead reserves a bucket name until that time, with no operator verb to clear it. |
diff --git a/internal/console/apiscan.go b/internal/console/apiscan.go| index 223853695..8550a5ff0 100644 |
| --- a/internal/console/apiscan.go |
| +++ b/internal/console/apiscan.go |
| @@ -163,6 +163,10 @@ func (s *Server) scanRecord(ctx context.Context, w http.ResponseWriter, acct *Ac |
| // a lost CAS. Every scan door changes one small block of a record other doors |
| // also write, and losing that race to a concurrent grant is ordinary, not |
| // exceptional. |
| +// errScanStoreTornDown: a scanning door was asked to change a store whose |
| +// teardown has begun. |
| +var errScanStoreTornDown = errors.New("console: the store is being torn down") |
| + |
| func (s *Server) writeScanRecord(ctx context.Context, acct *Account, app string, |
| mutate func(rec *objstore.Record) error) (*objstore.Record, error) { |
| @@ -175,12 +179,15 @@ func (s *Server) writeScanRecord(ctx context.Context, acct *Account, app string, |
| if rec == nil { |
| return nil, objstore.ErrNotDeclared |
| } |
| - // A record no walk can follow is not changed by a scanning door |
| - // either: the re-grant that would carry the change to the backend |
| - // is refused for it, and a record edited on the way to a refusal |
| - // is one more thing for the person repairing it to undo. |
| - if err := objstore.Coherent(rec, acct.Sqid, app); err != nil { |
| - return nil, err |
| + // A store that is being torn down has nothing for scanning to |
| + // converge on, and the re-grant that carries a scanning change to |
| + // the backend would take the store back as a side effect: new |
| + // keys, and access again to a bucket its teardown may have left to |
| + // another account. That is a grant's decision, never a scanning |
| + // door's. (A record no walk can follow never gets this far: |
| + // objstore.Get refuses it.) |
| + if rec.Teardown != nil { |
| + return nil, errScanStoreTornDown |
| } |
| if err := mutate(rec); err != nil { |
| return nil, err |
| @@ -347,9 +354,12 @@ func (s *Server) handleAPIScanGrant(w http.ResponseWriter, r *http.Request) { |
| } |
| // Enable on the backend BEFORE recording it. If the record said enabled |
| // and the backend was not, the sweep would run against a store nothing had |
| - // prepared; the other order merely repeats an idempotent enable. A record |
| - // no walk can follow is refused before the backend is touched. |
| - err = objstore.Coherent(rec, acct.Sqid, app) |
| + // prepared; the other order merely repeats an idempotent enable. A store |
| + // being torn down is refused before the backend is touched. |
| + err = nil |
| + if rec.Teardown != nil { |
| + err = errScanStoreTornDown |
| + } |
| if err == nil { |
| err = s.cfg.StoreScanner.EnsureEnabled(ctx, rec.Provisioned.Bucket) |
| } |
| @@ -408,7 +418,7 @@ func (s *Server) handleAPIScanRevoke(w http.ResponseWriter, r *http.Request) { |
| // EnsureDisabled failed must still clear the record, or the operator has |
| // no way to stop enforcement. |
| if rec, _, err := objstore.Get(ctx, s.cfg.Releases, acct.Sqid, app); err == nil && |
| - rec != nil && rec.Provisioned != nil && s.cfg.StoreScanner != nil && objstore.Coherent(rec, acct.Sqid, app) == nil { |
| + rec != nil && rec.Provisioned != nil && rec.Teardown == nil && s.cfg.StoreScanner != nil { |
| if derr := s.cfg.StoreScanner.EnsureDisabled(ctx, rec.Provisioned.Bucket); derr != nil { |
| log.Printf("console: api scan revoke %s/%s: disable: %v", acct.Sqid, app, derr) |
| } |
diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go| index d3706c70e..56ed74007 100644 |
| --- a/internal/console/apiscan_test.go |
| +++ b/internal/console/apiscan_test.go |
| @@ -2,9 +2,9 @@ package console |
| import ( |
| "context" |
| + "encoding/json" |
| "errors" |
| "net/http" |
| - "reflect" |
| "strings" |
| "testing" |
| "time" |
| @@ -549,35 +549,77 @@ func TestScanStatusReportsTheDeploymentSwitch(t *testing.T) { |
| } |
| } |
| -// A store record no walk can follow (it names two buckets, or its teardown |
| -// note is about another bucket) is not changed by a scanning door: the |
| -// re-grant that would carry the change to the backend is refused for it. |
| -func TestScanRecordWriterRefusesARecordNoWalkCanFollow(t *testing.T) { |
| - fx := newConsoleFixture(t) |
| +// putRawStore writes a store record as given, past objstore.Put's refusal of |
| +// records no walk can follow. |
| +func putRawStore(t *testing.T, fx *consoleFixture, acct, app string, rec *objstore.Record) { |
| + t.Helper() |
| + rec.Schema, rec.App = objstore.Schema, app |
| + body, err := json.Marshal(rec) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| ctx := context.Background() |
| - acct, err := fx.st.AccountBySqid(fx.sqid) |
| - if err != nil || acct == nil { |
| + _, etag, err := fx.rel.Get(ctx, objstore.Key(acct, app)) |
| + if err != nil { |
| + etag = "" |
| + } |
| + if _, err := fx.rel.Put(ctx, objstore.Key(acct, app), body, etag); err != nil { |
| t.Fatal(err) |
| } |
| +} |
| + |
| +func rawStoreBytes(t *testing.T, fx *consoleFixture, acct, app string) string { |
| + t.Helper() |
| + raw, _, err := fx.rel.Get(context.Background(), objstore.Key(acct, app)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return string(raw) |
| +} |
| + |
| +// The two shapes of store record no walk can follow, and one coherent |
| +// record whose teardown has begun. |
| +func refusedStoreRecords() map[string]struct { |
| + rec *objstore.Record |
| + want error |
| +} { |
| at := time.Date(2026, 9, 3, 10, 20, 0, 0, time.UTC) |
| - for name, c := range map[string]struct { |
| + declared := func() *objstore.Declared { |
| + return &objstore.Declared{EnvPrefix: "CARLOS_STORE", At: at, By: "m@example.com"} |
| + } |
| + return map[string]struct { |
| rec *objstore.Record |
| want error |
| }{ |
| - "two buckets": {&objstore.Record{Declared: &objstore.Declared{EnvPrefix: "S3", At: at, By: "m@example.com"}, |
| + "two buckets": {&objstore.Record{Declared: declared(), |
| Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-y", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| Provisioned: &objstore.Provisioned{Bucket: "carlos-store-x", Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}}, objstore.ErrTwoBuckets}, |
| - "a teardown note about another bucket": {&objstore.Record{Declared: &objstore.Declared{EnvPrefix: "S3", At: at, By: "m@example.com"}, |
| + "a teardown note about another bucket": {&objstore.Record{Declared: declared(), |
| Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-x", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| Provisioned: &objstore.Provisioned{Bucket: "carlos-store-x", Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}, |
| Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at, DeletedBucket: "carlos-store-y"}}, objstore.ErrUnfinishedTeardown}, |
| - } { |
| + "a teardown that has begun": {&objstore.Record{Declared: declared(), |
| + Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-x", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Bucket: "carlos-store-x", Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}, |
| + Teardown: &objstore.TeardownBlock{StartedAt: at}}, errScanStoreTornDown}, |
| + } |
| +} |
| + |
| +// A store record no walk can follow is not changed by a scanning door, and |
| +// neither is a store whose teardown has begun: the re-grant that carries a |
| +// scanning change to the backend would take that store back, new keys and |
| +// all, as a side effect of a scanning verb. |
| +func TestScanRecordWriterRefusesARecordNoWalkCanFollow(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + acct, err := fx.st.AccountBySqid(fx.sqid) |
| + if err != nil || acct == nil { |
| + t.Fatal(err) |
| + } |
| + for name, c := range refusedStoreRecords() { |
| app := "scanapp" |
| - _, etag, _ := objstore.Get(ctx, fx.rel, fx.sqid, app) |
| - if _, err := objstore.Put(ctx, fx.rel, fx.sqid, app, c.rec, etag); err != nil { |
| - t.Fatal(err) |
| - } |
| - before, _, _ := objstore.Get(ctx, fx.rel, fx.sqid, app) |
| + putRawStore(t, fx, fx.sqid, app, c.rec) |
| + before := rawStoreBytes(t, fx, fx.sqid, app) |
| called := false |
| _, err := fx.srv.writeScanRecord(ctx, acct, app, func(rec *objstore.Record) error { |
| called = true |
| @@ -587,9 +629,65 @@ func TestScanRecordWriterRefusesARecordNoWalkCanFollow(t *testing.T) { |
| if !errors.Is(err, c.want) || called { |
| t.Errorf("%s: writeScanRecord = %v (mutated %v), want %v and no change", name, err, called, c.want) |
| } |
| - after, _, _ := objstore.Get(ctx, fx.rel, fx.sqid, app) |
| - if !reflect.DeepEqual(before, after) { |
| - t.Errorf("%s: the record changed:\n%+v\n%+v", name, before, after) |
| + if after := rawStoreBytes(t, fx, fx.sqid, app); after != before { |
| + t.Errorf("%s: the record changed:\n%s\n%s", name, before, after) |
| + } |
| + } |
| +} |
| + |
| +// The scanning doors that call the scanner before they write the record do |
| +// not reach it for such a record: the operator's scan grant enables |
| +// nothing, and every door leaves the record byte for byte as it was. |
| +func TestScanDoorsDoNotReachTheScannerForARefusedRecord(t *testing.T) { |
| + for name, c := range refusedStoreRecords() { |
| + fx, sc, token := scanArmed(t, "code-scan-refused-"+strings.ReplaceAll(name, " ", "-")) |
| + requestScanning(t, fx, token, false) |
| + // Keep the member's request on the record: without it the grant |
| + // door stops earlier, for another reason. |
| + cur, _, err := objstore.Inspect(context.Background(), fx.rel, fx.sqid, "titogo") |
| + if err != nil || cur == nil || cur.Scanning == nil { |
| + t.Fatalf("%s: fixture: %+v %v", name, cur, err) |
| + } |
| + c.rec.Scanning = cur.Scanning |
| + putRawStore(t, fx, fx.sqid, "titogo", c.rec) |
| + before := rawStoreBytes(t, fx, fx.sqid, "titogo") |
| + for _, door := range []struct { |
| + path string |
| + body map[string]any |
| + }{{"/grant", map[string]any{}}, {"/enforce", map[string]any{"on": false}}, {"/revoke", map[string]any{}}} { |
| + if status, out := postJSON(t, scanURL(fx, "titogo")+door.path, door.body, token); status == http.StatusOK { |
| + t.Errorf("%s: scan%s answered 200: %v", name, door.path, out) |
| + } |
| + } |
| + for _, b := range []string{"carlos-store-x", "carlos-store-y"} { |
| + if sc.IsEnabled(b) { |
| + t.Errorf("%s: scanning was enabled on %s", name, b) |
| + } |
| + } |
| + if after := rawStoreBytes(t, fx, fx.sqid, "titogo"); after != before { |
| + t.Errorf("%s: a scanning door changed the record:\n%s\n%s", name, before, after) |
| + } |
| + } |
| +} |
| + |
| +// The member's declare door does not rewrite a record no walk can follow. |
| +func TestDeclareDoorDoesNotRewriteARefusedRecord(t *testing.T) { |
| + fx, _, token, _ := storeArmed(t, "code-declare-refused") |
| + at := time.Date(2026, 9, 3, 10, 20, 0, 0, time.UTC) |
| + for name, rec := range map[string]*objstore.Record{ |
| + "a teardown note and nothing else": {Declared: &objstore.Declared{EnvPrefix: "CARLOS_STORE", At: at, By: "m@example.com"}, |
| + Teardown: &objstore.TeardownBlock{StartedAt: at, DeletedBucket: "carlos-store-x"}}, |
| + "two buckets and no declaration": { |
| + Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-y", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Bucket: "carlos-store-x", Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}}, |
| + } { |
| + putRawStore(t, fx, fx.sqid, "titogo", rec) |
| + before := rawStoreBytes(t, fx, fx.sqid, "titogo") |
| + if status, out := postJSON(t, storeURL(fx, "titogo"), map[string]any{"env_prefix": "BLOBS"}, token); status == http.StatusOK { |
| + t.Errorf("%s: the declare door answered 200: %v", name, out) |
| + } |
| + if after := rawStoreBytes(t, fx, fx.sqid, "titogo"); after != before { |
| + t.Errorf("%s: the declare door rewrote the record:\n%s\n%s", name, before, after) |
| } |
| } |
| } |
diff --git a/internal/console/deletion_test.go b/internal/console/deletion_test.go| index a6ff58b4d..471c9c1ee 100644 |
| --- a/internal/console/deletion_test.go |
| +++ b/internal/console/deletion_test.go |
| @@ -841,3 +841,39 @@ func TestPurgeReleasesTheDomainClaim(t *testing.T) { |
| t.Fatalf("EnsureDomain after the purge = %v, want the claim released for anyone to take", err) |
| } |
| } |
| + |
| +// A record with a teardown block and nothing else names no estate. The |
| +// purge sends it through objstore.Teardown, which removes it, and then |
| +// completes. With a note that names a bucket the record is one no walk can |
| +// follow, and the purge is refused with the record intact. |
| +func TestPurgeHandlesATeardownOnlyStoreRecord(t *testing.T) { |
| + at := time.Date(2026, 9, 3, 10, 20, 0, 0, time.UTC) |
| + for name, c := range map[string]struct { |
| + teardown *objstore.TeardownBlock |
| + purged bool |
| + }{ |
| + "names nothing": {&objstore.TeardownBlock{StartedAt: at}, true}, |
| + "names a bucket": {&objstore.TeardownBlock{StartedAt: at, DeletedBucket: "carlos-store-legacy-x"}, false}, |
| + } { |
| + fx, c2, csrf := trashPageFixture(t) |
| + armStoreBackend(t, fx) |
| + putRawStore(t, fx, fx.sqid, "kass", &objstore.Record{ |
| + Declared: &objstore.Declared{EnvPrefix: "CARLOS_STORE", At: at, By: "m@example.com"}, Teardown: c.teardown}) |
| + status, _, body := postGated(t, fx, c2, fx.u("/trash/delete"), |
| + url.Values{"csrf": {csrf}, "app": {"kass"}, "confirm": {"kass"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("%s: purge = %d:\n%s", name, status, body) |
| + } |
| + rec, _, err := objstore.Inspect(context.Background(), fx.rel, fx.sqid, "kass") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + keys := appKeys(t, fx, "kass") |
| + if c.purged && (rec != nil || len(keys) != 0) { |
| + t.Errorf("%s: the purge did not complete: record %+v, keys %v", name, rec, keys) |
| + } |
| + if !c.purged && (rec == nil || rec.Teardown == nil || rec.Teardown.DeletedBucket != "carlos-store-legacy-x") { |
| + t.Errorf("%s: the purge removed or changed a record that names a bucket: %+v", name, rec) |
| + } |
| + } |
| +} |
diff --git a/internal/console/economicspass_test.go b/internal/console/economicspass_test.go| index 2e80ca5b6..c18b8e1c3 100644 |
| --- a/internal/console/economicspass_test.go |
| +++ b/internal/console/economicspass_test.go |
| @@ -1493,3 +1493,38 @@ func TestStatementsPassRetriesCloseAfterTransientReadFailure(t *testing.T) { |
| t.Fatal("August must close once the read failure clears") |
| } |
| } |
| + |
| +// A store record no walk can follow names two buckets, and which of them |
| +// holds the app's objects cannot be told from it. The daily storage pass |
| +// does not size one of them and write the answer into a create-only day: |
| +// the store is recorded as unswept, which holds the day. |
| +func TestStoragePassDoesNotMeasureARefusedRecord(t *testing.T) { |
| + for name, c := range refusedStoreRecords() { |
| + if errors.Is(c.want, errScanStoreTornDown) { |
| + continue // a coherent record: the pass measures it as before |
| + } |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + putRawStore(t, fx, fixtureAcct, fixtureApp, c.rec) |
| + sized := 0 |
| + fx.srv.storeSizer = func(context.Context, *objstore.Provisioned) (int64, error) { |
| + sized++ |
| + return 100 << 30, nil |
| + } |
| + if err := fx.srv.storagePass(ctx, "2026-08-12"); err != nil { |
| + t.Fatal(err) |
| + } |
| + raw, _, err := fx.rel.Get(ctx, usage.StorageDayKey("2026-08-12")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var day usage.StorageDay |
| + if err := json.Unmarshal(raw, &day); err != nil { |
| + t.Fatal(err) |
| + } |
| + app := day.Apps[fixtureAcct+"/"+fixtureApp] |
| + if sized != 0 || !app.StoreUnswept || app.StoreBytes != 0 { |
| + t.Errorf("%s: sized %d time(s), day entry %+v; want no sizing and an unswept store", name, sized, app) |
| + } |
| + } |
| +} |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| index 764793a13..599ce6a6f 100644 |
| --- a/internal/console/ownership.go |
| +++ b/internal/console/ownership.go |
| @@ -438,8 +438,11 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) { |
| // (objstore/walk.go:560's window, from the other end). |
| unlockStoreWalk := s.storeWalks.lock(acct.Sqid, app) |
| defer unlockStoreWalk() |
| - if rec, _, rerr := objstore.Get(r.Context(), s.cfg.Releases, acct.Sqid, app); rerr == nil && rec != nil && (rec.Provisioned != nil || rec.Grant != nil) { |
| - if s.cfg.StoreProvisioner == nil { |
| + if rec, _, rerr := objstore.Get(r.Context(), s.cfg.Releases, acct.Sqid, app); rerr == nil && rec != nil && (rec.Provisioned != nil || rec.Grant != nil || rec.Teardown != nil) { |
| + // A record with only a teardown block (no grant, nothing |
| + // provisioned) names no backend estate; Teardown removes it |
| + // without a backend. Anything else needs one. |
| + if s.cfg.StoreProvisioner == nil && (rec.Provisioned != nil || rec.Grant != nil) { |
| fail("“" + app + "” has a provisioned object store, and this console has no storage backend configured (CARLOS_STORE_BACKEND) — it cannot be deleted here until the store is torn down.") |
| return |
| } |
| @@ -462,7 +465,9 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) { |
| } else if rec.Grant != nil { |
| bucket = rec.Grant.Bucket |
| } |
| - s.audit(acct, app, addr, "store-destroy", "tore down the object store (bucket "+bucket+") during permanent deletion") |
| + if rec.Provisioned != nil || rec.Grant != nil { |
| + s.audit(acct, app, addr, "store-destroy", "tore down the object store (bucket "+bucket+") during permanent deletion") |
| + } |
| } else if rerr != nil { |
| log.Printf("console: purge store read %s/%s: %v", acct.Sqid, app, rerr) |
| fail("Could not read “" + app + "”'s object-store record — nothing was deleted; try again.") |
diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go| index a7dbec728..1a0527c6c 100644 |
| --- a/internal/objstore/objstore.go |
| +++ b/internal/objstore/objstore.go |
| @@ -69,14 +69,6 @@ type GrantBlock struct { |
| // for its lease can tell that the store it was asked about is not the |
| // one standing now. |
| Walks int `json:"walks,omitempty"` |
| - // Delivering is set, in the same write that counts the walk, by a |
| - // grant that is about to reap the user's keys and mint and deliver a |
| - // new one, and cleared by the write that records the result. While it |
| - // stands the record may name a key that is gone and the app may hold |
| - // one the record does not name: a grant run again does not take the |
| - // healthy short-circuit, and a rotation is refused (ErrGrantUnfinished) |
| - // instead of reaping the key the app was given. |
| - Delivering bool `json:"delivering,omitempty"` |
| } |
| type Provisioned struct { |
| @@ -159,6 +151,17 @@ type Record struct { |
| // Teardown is what an unfinished teardown has already done, so a |
| // re-run does not do it again to something that has changed since. |
| Teardown *TeardownBlock `json:"teardown,omitempty"` |
| + // Delivering is set by a grant or a rotation before it reaps the |
| + // user's keys and mints and delivers a new one, and cleared by the |
| + // write that records the result. While it stands the record may name a |
| + // key that is gone and the app may hold one the record does not name. |
| + // The mark does not spare that key: it makes the state visible, so a |
| + // rotation is refused (ErrGrantUnfinished) instead of quietly reaping |
| + // on top of it, and a grant run again does not take the healthy |
| + // short-circuit but walks in full and leaves the record naming a key |
| + // the user has. A record from a build before the mark reads false, |
| + // which means "not recorded", not "finished". |
| + Delivering bool `json:"delivering,omitempty"` |
| } |
| // TeardownBlock is a teardown's write-ahead record on the store it is |
| @@ -204,7 +207,29 @@ func EnvNames(prefix string) (bucket, region, endpoint, accessKey, secretKey str |
| // Get reads the record. Absent is (nil, "", nil) — same idiom as |
| // appconfig.getIfExists, because "no store" is a state, not an error. |
| +// |
| +// A record that no walk can follow (Coherent) is an error, like one that |
| +// will not parse: every caller already treats a read error as "cannot |
| +// tell", and holds. The check lives here, in the one reader, so that no |
| +// door, pass or walk can act on such a record by forgetting to make it. |
| +// Inspect is the reader for the paths that must see one anyway. |
| func Get(ctx context.Context, s release.Store, acct, app string) (*Record, string, error) { |
| + rec, etag, err := Inspect(ctx, s, acct, app) |
| + if err != nil { |
| + return nil, "", err |
| + } |
| + if err := Coherent(rec, acct, app); err != nil { |
| + return nil, "", err |
| + } |
| + return rec, etag, nil |
| +} |
| + |
| +// Inspect reads the record as it is, coherent or not. It is for the two |
| +// things that have to see a record nothing may act on: deciding who holds |
| +// a bucket (a record that names a bucket holds it, whatever else is wrong |
| +// with it) and showing the record to the person repairing it. Nothing is |
| +// ever done to a store on the strength of a record read this way. |
| +func Inspect(ctx context.Context, s release.Store, acct, app string) (*Record, string, error) { |
| if !release.ValidAccount(acct) { |
| return nil, "", fmt.Errorf("objstore: invalid account %q", acct) |
| } |
| @@ -228,7 +253,13 @@ func Get(ctx context.Context, s release.Store, acct, app string) (*Record, strin |
| // Put writes the record with the caller's CAS position; etag "" is |
| // create-only. Every writer is a read-modify-write, so a lost race is |
| // release.ErrCASConflict for the caller to surface, never to absorb. |
| +// |
| +// A record that Get would refuse is not written: no path of this build can |
| +// produce one, and this is what holds it to that. |
| func Put(ctx context.Context, s release.Store, acct, app string, rec *Record, etag string) (string, error) { |
| + if err := Coherent(rec, acct, app); err != nil { |
| + return "", err |
| + } |
| rec.Schema = Schema |
| rec.App = app |
| body, err := json.MarshalIndent(rec, "", " ") |
diff --git a/internal/objstore/ownership.go b/internal/objstore/ownership.go| index d191fb46e..09525ed8d 100644 |
| --- a/internal/objstore/ownership.go |
| +++ b/internal/objstore/ownership.go |
| @@ -85,10 +85,12 @@ type OwnerRef struct { |
| // again is refused again; an operator repairs the record by hand. |
| var ErrUnfinishedTeardown = errors.New("objstore: the record's teardown note is about another bucket") |
| -// ErrGrantUnfinished: a grant began delivering a key and did not record |
| -// the result (GrantBlock.Delivering). The key the record names may be |
| -// gone and the app may hold one the record does not name. A rotation is |
| -// refused until the grant has been run again, which repairs in full. |
| +// ErrGrantUnfinished: a grant or a rotation began changing the user's keys |
| +// and did not record the result (Record.Delivering). The key the record |
| +// names may be gone and the app may hold one the record does not name. A |
| +// rotation is refused until the grant has been run again; that walks in |
| +// full and leaves the record naming a key the user has. It does not keep |
| +// the key the app was holding: that one is reaped and replaced. |
| var ErrGrantUnfinished = errors.New("objstore: an unfinished grant must be run again first") |
| // ErrTwoBuckets: the store's record names one bucket in its grant block and |
| @@ -116,6 +118,22 @@ var ( |
| func (iv OwnerInterval) open() bool { return iv.Until == nil } |
| +// left reports that the ledger shows acct/app held the bucket and no longer |
| +// does: it has at least one interval and none of them is open. |
| +func (b *BucketOwnership) left(acct, app string) bool { |
| + held := false |
| + for _, iv := range b.Intervals { |
| + if iv.Account != acct || iv.App != app { |
| + continue |
| + } |
| + if iv.open() { |
| + return false |
| + } |
| + held = true |
| + } |
| + return held |
| +} |
| + |
| // overlaps reports whether the interval touches [from, to). |
| func (iv OwnerInterval) overlaps(from, to time.Time) bool { |
| if !iv.From.Before(to) { |
diff --git a/internal/objstore/ownership_test.go b/internal/objstore/ownership_test.go| index e46af2e15..898c0a138 100644 |
| --- a/internal/objstore/ownership_test.go |
| +++ b/internal/objstore/ownership_test.go |
| @@ -2595,7 +2595,7 @@ func TestGrantOverATeardownKeepsTheRecordsOwnBucket(t *testing.T) { |
| if got.Grant.Minted { |
| t.Fatalf("adopt %q: a bucket the record already had was written down as minted by this grant", adopt) |
| } |
| - if got.Teardown != nil || got.Grant.Delivering { |
| + if got.Teardown != nil || got.Delivering { |
| t.Fatalf("adopt %q: the finished take-back left a teardown block or its delivering mark: %+v %+v", adopt, got.Teardown, got.Grant) |
| } |
| if _, exists := p.Buckets[legacyX]; !exists { |
| @@ -2688,10 +2688,7 @@ func TestGrantIsRefusedOverATeardownNoteForAnotherBucket(t *testing.T) { |
| td.DeletedBucket = elsewhere |
| } |
| rec := &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, Teardown: td} |
| - _, etag, _ := Get(ctx, s, "bab", "titogo") |
| - if _, err := Put(ctx, s, "bab", "titogo", rec, etag); err != nil { |
| - t.Fatal(err) |
| - } |
| + putRaw(t, s, "bab", "titogo", rec) |
| _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)) |
| if !errors.Is(err, ErrUnfinishedTeardown) { |
| t.Fatalf("note %v: a grant for the minted name over a teardown of %s: %v, want ErrUnfinishedTeardown", withNote, elsewhere, err) |
| @@ -2702,7 +2699,7 @@ func TestGrantIsRefusedOverATeardownNoteForAnotherBucket(t *testing.T) { |
| if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) { |
| t.Fatalf("note %v: a teardown of a record whose note names %s: %v, want ErrUnfinishedTeardown", withNote, elsewhere, err) |
| } |
| - cur, _, _ := Get(ctx, s, "bab", "titogo") |
| + cur := inspect(t, s, "bab", "titogo") |
| if cur == nil || cur.Grant != nil || cur.Teardown == nil || cur.Teardown.DeletedBucket != elsewhere || cur.Teardown.TakeBacks != 0 { |
| t.Fatalf("note %v: a refused walk changed or deleted the record: %+v", withNote, cur) |
| } |
| @@ -2865,10 +2862,8 @@ func TestWalksRefuseARecordThatNamesTwoBuckets(t *testing.T) { |
| rec := &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, |
| Grant: &GrantBlock{Backend: "aws", Bucket: ledgerBucket, Source: GrantSourceOperator, At: t0, By: "op", Minted: true}, |
| Provisioned: &Provisioned{Bucket: legacyX, Region: "eu-west-1", IAMUser: user, KeyID: "LEGACYKEY", At: t0}} |
| - if _, err := Put(ctx, s, "bab", "titogo", rec, ""); err != nil { |
| - t.Fatal(err) |
| - } |
| - before, _, _ := Get(ctx, s, "bab", "titogo") |
| + putRaw(t, s, "bab", "titogo", rec) |
| + before := inspect(t, s, "bab", "titogo") |
| policy := p.Users[user].Policy |
| if _, _, err := p.MintKey(ctx, user); err != nil { |
| t.Fatal(err) |
| @@ -2889,8 +2884,8 @@ func TestWalksRefuseARecordThatNamesTwoBuckets(t *testing.T) { |
| if _, err := RotateFinish(ctx, s, p, "bab", "titogo"); !errors.Is(err, ErrTwoBuckets) { |
| t.Fatalf("a rotation finish of a record that names two buckets: %v, want ErrTwoBuckets", err) |
| } |
| - after, _, _ := Get(ctx, s, "bab", "titogo") |
| - if !reflect.DeepEqual(before, after) { |
| + after := inspect(t, s, "bab", "titogo") |
| + if before == nil || !reflect.DeepEqual(before, after) { |
| t.Fatalf("a refused walk changed the record:\n%+v\n%+v", before, after) |
| } |
| if !reflect.DeepEqual(p.Users[user].Keys, keys) { |
| @@ -2899,9 +2894,13 @@ func TestWalksRefuseARecordThatNamesTwoBuckets(t *testing.T) { |
| // With scanning enabled, the sweep does not run on it either. |
| scanning := *before |
| scanning.Scanning = &ScanBlock{Enabled: &ScanEnabled{}} |
| - _, etag, _ := Get(ctx, s, "bab", "titogo") |
| - if _, err := Put(ctx, s, "bab", "titogo", &scanning, etag); err != nil { |
| - t.Fatal(err) |
| + putRaw(t, s, "bab", "titogo", &scanning) |
| + // Every reader that acts on a record refuses it the same way. |
| + if got, _, err := Get(ctx, s, "bab", "titogo"); !errors.Is(err, ErrTwoBuckets) || got != nil { |
| + t.Fatalf("Get of a record that names two buckets: %+v %v, want ErrTwoBuckets", got, err) |
| + } |
| + if _, err := Put(ctx, s, "bab", "titogo", &scanning, ""); !errors.Is(err, ErrTwoBuckets) { |
| + t.Fatalf("Put of a record that names two buckets: %v, want ErrTwoBuckets", err) |
| } |
| if side, err := RunSweep(ctx, s, nil, nil, "bab", "titogo", time.Second); !errors.Is(err, ErrTwoBuckets) || side != nil { |
| t.Fatalf("a sweep of a record that names two buckets: %+v %v, want ErrTwoBuckets", side, err) |
| @@ -2939,10 +2938,9 @@ func TestWalksRefuseATeardownNoteAboutAnotherBucket(t *testing.T) { |
| p.Buckets[ledgerBucket]["mine.pdf"] = []byte("this store's object") |
| rec, etag, _ := Get(ctx, s, "bab", "titogo") |
| rec.Teardown = &TeardownBlock{StartedAt: t0, BucketDeletedAt: t0.Add(time.Minute), DeletedBucket: "carlos-store-legacy-y"} |
| - if _, err := Put(ctx, s, "bab", "titogo", rec, etag); err != nil { |
| - t.Fatal(err) |
| - } |
| - before, _, _ := Get(ctx, s, "bab", "titogo") |
| + _ = etag |
| + putRaw(t, s, "bab", "titogo", rec) |
| + before := inspect(t, s, "bab", "titogo") |
| user := UserName("bab", "titogo") |
| keys := map[string]string{} |
| for id, secret := range p.Users[user].Keys { |
| @@ -2960,8 +2958,8 @@ func TestWalksRefuseATeardownNoteAboutAnotherBucket(t *testing.T) { |
| if _, err := RotateFinish(ctx, s, p, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) { |
| t.Fatalf("rotate finish: %v, want ErrUnfinishedTeardown", err) |
| } |
| - after, _, _ := Get(ctx, s, "bab", "titogo") |
| - if !reflect.DeepEqual(before, after) { |
| + after := inspect(t, s, "bab", "titogo") |
| + if before == nil || !reflect.DeepEqual(before, after) { |
| t.Fatalf("a refused walk changed the record:\n%+v\n%+v", before, after) |
| } |
| if string(p.Buckets[ledgerBucket]["mine.pdf"]) != "this store's object" { |
| @@ -3018,7 +3016,7 @@ func TestRotationIsRefusedAfterAGrantThatFailedAtItsLastWrite(t *testing.T) { |
| t.Fatalf("%s: fixture: the grant was meant to fail at its last write (err %v, writes %d)", verb, err, failing.seen) |
| } |
| cur, _, _ := Get(ctx, dir, "bab", "titogo") |
| - if cur == nil || cur.Grant == nil || !cur.Grant.Delivering { |
| + if cur == nil || cur.Grant == nil || !cur.Delivering { |
| t.Fatalf("%s: the record does not say a grant began delivering and did not finish: %+v", verb, cur) |
| } |
| if _, recorded := p.Users[user].Keys[cur.Provisioned.KeyID]; recorded { |
| @@ -3059,7 +3057,7 @@ func TestRotationIsRefusedAfterAGrantThatFailedAtItsLastWrite(t *testing.T) { |
| if err != nil { |
| t.Fatalf("%s: the grant run again: %v", verb, err) |
| } |
| - if repaired.Grant.Delivering { |
| + if repaired.Delivering { |
| t.Fatalf("%s: the finished grant left its delivering mark", verb) |
| } |
| if _, live := p.Users[user].Keys[repaired.Provisioned.KeyID]; !live { |
| @@ -3135,3 +3133,264 @@ func TestMembersTakeBackIsRefusedWhileAnotherAccountHoldsTheBucket(t *testing.T) |
| } |
| } |
| } |
| + |
| +// putRaw writes a store record as given, past Put's refusal of records no |
| +// walk can follow: the tests that need such a record are about what an |
| +// earlier build or a hand edit left behind. |
| +func putRaw(t *testing.T, s release.Store, acct, app string, rec *Record) { |
| + t.Helper() |
| + rec.Schema, rec.App = Schema, app |
| + body, err := json.Marshal(rec) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + _, etag, err := s.Get(context.Background(), Key(acct, app)) |
| + if err != nil { |
| + etag = "" |
| + } |
| + if _, err := s.Put(context.Background(), Key(acct, app), body, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| +// inspect reads a store record as it is, coherent or not. |
| +func inspect(t *testing.T, s release.Store, acct, app string) *Record { |
| + t.Helper() |
| + rec, _, err := Inspect(context.Background(), s, acct, app) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + return rec |
| +} |
| + |
| +// A rotation reaps, mints and delivers a new key and fails at its last |
| +// write: the record names the old key and the app holds the new one. The |
| +// record says a key change began and was not recorded. Running the rotation |
| +// again is refused, because it would reap the key the app holds. A grant |
| +// run again walks in full, the record comes to name a key the user has, |
| +// and rotation works again. |
| +func TestRotationIsRefusedAfterARotationThatFailedAtItsLastWrite(t *testing.T) { |
| + ctx := context.Background() |
| + dir := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + declare(t, dir, "bab", "titogo", "TITO_S3") |
| + first, err := Grant(ctx, dir, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + user := UserName("bab", "titogo") |
| + // The mark is the rotation's first write of the record and the result |
| + // its second. |
| + failing := &failingPutStore{Store: dir, key: Key("bab", "titogo"), nth: 2} |
| + if _, err := Rotate(ctx, failing, p, pub, "bab", "titogo", t0.Add(time.Hour)); err == nil || failing.seen < 2 { |
| + t.Fatalf("fixture: the rotation was meant to fail at its last write (err %v, writes %d)", err, failing.seen) |
| + } |
| + cur, _, err := Get(ctx, dir, "bab", "titogo") |
| + if err != nil || cur == nil || !cur.Delivering || cur.Provisioned.KeyID != first.Provisioned.KeyID { |
| + t.Fatalf("the record after the failed rotation: %+v %v", cur, err) |
| + } |
| + if len(p.Users[user].Keys) != 2 { |
| + t.Fatalf("fixture: want the old key and the delivered one: %v", p.Users[user].Keys) |
| + } |
| + keys := map[string]string{} |
| + for id, secret := range p.Users[user].Keys { |
| + keys[id] = secret |
| + } |
| + advance(walkLease + time.Minute) |
| + if _, err := Rotate(ctx, dir, p, pub, "bab", "titogo", t0.Add(2*time.Hour)); !errors.Is(err, ErrGrantUnfinished) { |
| + t.Fatalf("a rotation after one that failed at its last write: %v, want ErrGrantUnfinished", err) |
| + } |
| + if !reflect.DeepEqual(p.Users[user].Keys, keys) { |
| + t.Fatalf("the refused rotation changed the user's keys: %v, were %v", p.Users[user].Keys, keys) |
| + } |
| + repaired, err := Grant(ctx, dir, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(3*time.Hour)) |
| + if err != nil { |
| + t.Fatalf("the grant run again: %v", err) |
| + } |
| + if repaired.Delivering { |
| + t.Fatal("the finished grant left the mark") |
| + } |
| + if _, live := p.Users[user].Keys[repaired.Provisioned.KeyID]; !live { |
| + t.Fatalf("the repaired record names a key the user does not have: %s of %v", repaired.Provisioned.KeyID, p.Users[user].Keys) |
| + } |
| + if _, err := Rotate(ctx, dir, p, pub, "bab", "titogo", t0.Add(4*time.Hour)); err != nil { |
| + t.Fatalf("a rotation after the repair: %v", err) |
| + } |
| +} |
| + |
| +// Another store's teardown decides whether it is the last holder of a |
| +// bucket from the store records and the ledger. A record nothing may act on |
| +// (here: provisioned on X, grant block naming Y, no interval on Y) still |
| +// counts as a holder of every bucket it names. The store that shares Y |
| +// does not destroy it. |
| +func TestTeardownDoesNotDestroyABucketARefusedRecordNames(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + const sharedY = "carlos-store-shared-y" |
| + p.Buckets[sharedY] = map[string][]byte{"theirs.pdf": []byte("written by the store with the split record")} |
| + // B: an older record, split between X and Y; its user's policy reaches Y. |
| + putRaw(t, s, "bac", "other", &Record{Declared: &Declared{EnvPrefix: "S3", At: t0, By: "member@example.com"}, |
| + Grant: &GrantBlock{Backend: "aws", Bucket: sharedY, Source: GrantSourceOperator, At: t0, By: "op"}, |
| + Provisioned: &Provisioned{Bucket: "carlos-store-legacy-x", Region: "eu-west-1", IAMUser: UserName("bac", "other"), KeyID: "KB", At: t0}}) |
| + // A: a coherent store an operator adopted onto Y. |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, sharedY, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + err := Teardown(ctx, s, p, nil, "bab", "titogo") |
| + if !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("the teardown of a store sharing Y with a refused record: %v, want ErrBucketRetained", err) |
| + } |
| + if got := string(p.Buckets[sharedY]["theirs.pdf"]); got != "written by the store with the split record" { |
| + t.Fatalf("the other store's object in Y is gone (now %q)", got) |
| + } |
| + // The same record holds X, the other bucket it names. |
| + if holders, herr := otherStoreHolders(ctx, s, "carlos-store-legacy-x", "bab", "titogo"); herr != nil || len(holders) != 1 { |
| + t.Fatalf("holders of X: %+v %v, want the split record", holders, herr) |
| + } |
| +} |
| + |
| +// An older grant block that names no bucket (the name used to be derived |
| +// each time). A grant resumed over it creates the bucket; before it does, |
| +// the block is made to name it, in the write that counts the walk. So when |
| +// that grant fails at its last write, a teardown still finds the bucket. |
| +func TestResumedGrantRecordsTheBucketItIsAboutToCreate(t *testing.T) { |
| + ctx := context.Background() |
| + dir := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + putRaw(t, dir, "bab", "titogo", &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, |
| + Grant: &GrantBlock{Backend: "aws", Source: GrantSourceOperator, At: t0, By: "op"}}) |
| + // The count is the grant's first write of the record, the result its |
| + // second. |
| + failing := &failingPutStore{Store: dir, key: Key("bab", "titogo"), nth: 2} |
| + if _, err := Grant(ctx, failing, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); err == nil || failing.seen < 2 { |
| + t.Fatalf("fixture: the grant was meant to fail at its last write (err %v, writes %d)", err, failing.seen) |
| + } |
| + if _, ok := p.Buckets[ledgerBucket]; !ok { |
| + t.Fatal("fixture: the grant was meant to have created the bucket") |
| + } |
| + p.Buckets[ledgerBucket]["invoice.pdf"] = []byte("written with the delivered key") |
| + cur, _, err := Get(ctx, dir, "bab", "titogo") |
| + if err != nil || cur == nil || cur.Grant == nil || cur.Grant.Bucket != ledgerBucket { |
| + t.Fatalf("the record does not name the bucket the grant created: %+v %v", cur, err) |
| + } |
| + advance(walkLease + time.Minute) |
| + if err := Teardown(ctx, dir, p, nil, "bab", "titogo"); err != nil { |
| + t.Fatalf("teardown: %v", err) |
| + } |
| + if _, ok := p.Buckets[ledgerBucket]; ok { |
| + t.Fatal("the teardown reported success and left the bucket the grant created") |
| + } |
| + if rec := inspect(t, dir, "bab", "titogo"); rec != nil { |
| + t.Fatalf("the teardown left the record: %+v", rec) |
| + } |
| +} |
| + |
| +// Two accounts share an operator-adopted bucket. The first tears its store |
| +// down and leaves the bucket to the second: its interval is closed and its |
| +// record stays (retained). The second then tears down, deletes the bucket |
| +// and fails before it finishes. Its member asks for the store again. The |
| +// first store's leftover record is not a hold: the ledger shows it left. |
| +func TestMembersTakeBackIsNotHeldByARecordWhoseStoreHasLeft(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + const shared = "carlos-store-shared-archive" |
| + p.Buckets[shared] = map[string][]byte{} |
| + for _, o := range []struct{ acct, app, prefix string }{{"bab", "titogo", "TITO_S3"}, {"bac", "other", "OTHER_S3"}} { |
| + declare(t, s, o.acct, o.app, o.prefix) |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, o.acct, o.app, "op", GrantSourceOperator, shared, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // The other account leaves first: retained, interval closed, record kept. |
| + if err := Teardown(ctx, s, p, nil, "bac", "other"); !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("fixture: the first teardown: %v, want ErrBucketRetained", err) |
| + } |
| + if rec := inspect(t, s, "bac", "other"); rec == nil || rec.Teardown == nil { |
| + t.Fatalf("fixture: the first store's record was meant to be kept: %+v", rec) |
| + } |
| + if !mustLedger(t, s, shared).left("bac", "other") { |
| + t.Fatalf("fixture: the ledger does not show the first store left: %+v", mustLedger(t, s, shared)) |
| + } |
| + // This store leaves last, and its teardown fails after the bucket. |
| + advance(time.Minute) |
| + p.FailNext = map[string]error{"DeleteUser": errors.New("injected DeleteUser")} |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatal("fixture: the second teardown was meant to fail after deleting the bucket") |
| + } |
| + p.FailNext = nil |
| + advance(walkLease + time.Minute) |
| + got, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "member@example.com", GrantSourceAccountApproval, "", t0.Add(time.Hour)) |
| + if err != nil { |
| + t.Fatalf("the member's take-back, with the other store gone: %v", err) |
| + } |
| + if got.Provisioned == nil || got.Provisioned.Bucket != shared || got.Teardown != nil { |
| + t.Fatalf("the take-back: %+v", got) |
| + } |
| +} |
| + |
| +// A record with a teardown block and nothing else (no grant, nothing |
| +// provisioned, no note) names no estate. Teardown removes it, and the |
| +// reader does not refuse it. With a note it is refused (see above). |
| +func TestTeardownRemovesATeardownOnlyRecordThatNamesNothing(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + putRaw(t, s, "bab", "titogo", &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, |
| + Teardown: &TeardownBlock{StartedAt: t0}}) |
| + if rec, _, err := Get(ctx, s, "bab", "titogo"); err != nil || rec == nil { |
| + t.Fatalf("the reader refused a record that names nothing: %+v %v", rec, err) |
| + } |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err != nil { |
| + t.Fatalf("teardown: %v", err) |
| + } |
| + if rec := inspect(t, s, "bab", "titogo"); rec != nil { |
| + t.Fatalf("the record is still there: %+v", rec) |
| + } |
| +} |
| + |
| +// A deletion instant with no bucket name beside it (only a build of this |
| +// branch before the name was added could have written one) does not say |
| +// which bucket it is about. A teardown would read it as "this record's |
| +// bucket is gone" and skip a live bucket. The reader refuses the record. |
| +func TestADeletionNoteWithNoBucketNameIsRefused(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.Buckets[ledgerBucket]["mine.pdf"] = []byte("this store's object") |
| + rec := inspect(t, s, "bab", "titogo") |
| + rec.Teardown = &TeardownBlock{StartedAt: t0, BucketDeletedAt: t0.Add(time.Minute)} |
| + putRaw(t, s, "bab", "titogo", rec) |
| + if got, _, err := Get(ctx, s, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) || got != nil { |
| + t.Fatalf("Get: %+v %v, want ErrUnfinishedTeardown", got, err) |
| + } |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("teardown: %v, want ErrUnfinishedTeardown", err) |
| + } |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("grant: %v, want ErrUnfinishedTeardown", err) |
| + } |
| + if string(p.Buckets[ledgerBucket]["mine.pdf"]) != "this store's object" { |
| + t.Fatal("the store's object is gone") |
| + } |
| + if l := mustLedger(t, s, ledgerBucket); l.DestroyedAt != nil { |
| + t.Fatalf("a destruction was written on the ledger of a live bucket: %+v", l) |
| + } |
| +} |
diff --git a/internal/objstore/sweep.go b/internal/objstore/sweep.go| index 6fd0be942..0af7fa4a2 100644 |
| --- a/internal/objstore/sweep.go |
| +++ b/internal/objstore/sweep.go |
| @@ -132,12 +132,6 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| if rec == nil || rec.Provisioned == nil || rec.Scanning == nil || rec.Scanning.Enabled == nil { |
| return nil, nil |
| } |
| - // A record no walk can follow is not swept: the bucket it is |
| - // provisioned on may not be the one the app uses, and coverage |
| - // published for it would be a claim about the wrong objects. |
| - if err := Coherent(rec, acct, app); err != nil { |
| - return nil, err |
| - } |
| // A trashed store is destroyed within 30 days. Scanning it bills for |
| // egress on data that is on its way out, and reports coverage for an app |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index e2141a3f1..117b521d0 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -156,9 +156,6 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| if rec == nil || rec.Declared == nil { |
| return nil, ErrNotDeclared |
| } |
| - if err := Coherent(rec, acct, app); err != nil { |
| - return nil, err |
| - } |
| // A grant over an unfinished teardown takes the store back. The record |
| // says a teardown began (TeardownBlock), so whatever its Provisioned |
| // block and delivered variables say, the user or the bucket may be |
| @@ -291,9 +288,19 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| } |
| } |
| for _, h := range holders { |
| - if h.Account != acct { |
| - return nil, fmt.Errorf("%w: %s by %s/%s of another account; only an operator's grant gives %s/%s access to it again", ErrBucketHeld, bucket, h.Account, h.App, acct, app) |
| + if h.Account == acct { |
| + continue |
| + } |
| + // A record that outlived its store's access is not a hold. A |
| + // store that left the bucket has a closed interval on the |
| + // ledger, written after its credentials were revoked, and |
| + // keeps its record only until its retained teardown is run |
| + // again. Only what the ledger shows counts for that: a store |
| + // with no interval at all has an unknown history, and holds. |
| + if ledger != nil && ledger.left(h.Account, h.App) { |
| + continue |
| } |
| + return nil, fmt.Errorf("%w: %s by %s/%s of another account; only an operator's grant gives %s/%s access to it again", ErrBucketHeld, bucket, h.Account, h.App, acct, app) |
| } |
| } |
| wroteGrant := rec.Grant == nil |
| @@ -464,13 +471,13 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // can put right. |
| // |
| // A grant that will run the walk says so in the same write that counts |
| - // it (GrantBlock.Delivering), and the write that records the result |
| + // it (Record.Delivering), and the write that records the result |
| // takes the mark off. A grant that reaps the recorded key, delivers a |
| // new one and fails at its last write therefore leaves a record that |
| // says exactly that: the next grant does not mistake it for healthy, |
| // and a rotation does not reap the key the app was given. |
| healthy := false |
| - if !rebuilt && rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint && !rec.Grant.Delivering { |
| + if !rebuilt && rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint && !rec.Delivering { |
| missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, d.Endpoint != "") |
| if err != nil { |
| return nil, err |
| @@ -479,14 +486,24 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| } |
| before := estateOf(rec) |
| rec.Grant.Walks++ |
| - rec.Grant.Delivering = !healthy |
| + rec.Delivering = !healthy |
| + // An older grant block may name no bucket: the name was derived each |
| + // time. From here on this walk may create that bucket, so the block is |
| + // made to name it in the same write, and a teardown can always find |
| + // what a grant created. |
| + if rec.Grant.Bucket == "" { |
| + rec.Grant.Bucket = bucket |
| + } |
| if etag, err = Put(ctx, s, acct, app, rec, etag); errors.Is(err, release.ErrCASConflict) { |
| fresh, freshEtag, gerr := Get(ctx, s, acct, app) |
| if gerr != nil || fresh == nil || fresh.Declared == nil || fresh.Grant == nil || estateOf(fresh) != before { |
| return nil, err |
| } |
| fresh.Grant.Walks++ |
| - fresh.Grant.Delivering = fresh.Grant.Delivering || !healthy |
| + fresh.Delivering = fresh.Delivering || !healthy |
| + if fresh.Grant.Bucket == "" { |
| + fresh.Grant.Bucket = bucket |
| + } |
| if etag, err = Put(ctx, s, acct, app, fresh, freshEtag); err != nil { |
| return nil, err |
| } |
| @@ -561,7 +578,7 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| rec.Provisioned = &Provisioned{Bucket: bucket, Region: region, Endpoint: d.Endpoint, |
| IAMUser: user, UserARN: arn, KeyID: id, PrevKeyID: prev, At: now} |
| rec.Teardown = nil // the store is whole again |
| - rec.Grant.Delivering = false |
| + rec.Delivering = false |
| if _, err := Put(ctx, s, acct, app, rec, etag); err != nil { |
| if !errors.Is(err, release.ErrCASConflict) { |
| return nil, err |
| @@ -597,9 +614,7 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| } |
| fresh.Provisioned = rec.Provisioned |
| fresh.Teardown = nil |
| - if fresh.Grant != nil { |
| - fresh.Grant.Delivering = false |
| - } |
| + fresh.Delivering = false |
| if _, perr := Put(ctx, s, acct, app, fresh, freshEtag); perr != nil { |
| return nil, perr |
| } |
| @@ -637,10 +652,15 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // record never names a key whose secret was never delivered. A record that |
| // claimed the new key after a FAILED delivery would point every reader at a |
| // key whose secret died with the write, and the next run would reap the ONE |
| -// key still working. The reverse gap is survivable and deliberate — after a |
| -// successful delivery whose Put then fails, the record names the old key |
| -// while the app already holds the new one, and both are live, so the app |
| -// keeps working and a re-run converges. |
| +// key still working. The reverse gap is deliberate — after a successful |
| +// delivery whose Put then fails, the record names the old key while the app |
| +// already holds the new one, and both are live, so the app keeps working. |
| +// It is not left unmarked: before step one the record is marked as having a |
| +// key change under way (Record.Delivering), and step five takes the mark |
| +// off. A rotation that stopped anywhere in between leaves the mark, the |
| +// next rotation is refused instead of reaping the key the app holds, and a |
| +// grant run again walks in full and leaves the record naming a key the user |
| +// has. |
| // |
| // The one shape of that gap which is NOT survivable is a lost CAS: the record |
| // would keep naming the old key permanently, and the next rotation's reap |
| @@ -661,9 +681,6 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| if rec.Declared == nil { |
| return nil, ErrNotDeclared // no prefix to deliver into |
| } |
| - if err := Coherent(rec, acct, app); err != nil { |
| - return nil, err |
| - } |
| // A rotation deletes and mints keys of a user whose name is the same |
| // for every store this app will ever have. It is a walk like a grant |
| // or a teardown: it runs under the bucket's lease and inside its fence, |
| @@ -676,6 +693,26 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| if rec.Declared == nil { |
| return nil, ErrNotDeclared |
| } |
| + // Written ahead of every key change: from here until the last write |
| + // the record may name a key that is not the one the app holds. The |
| + // declare door writes this same object; if it wrote in between, the |
| + // mark goes on the record as it now stands, provided it is still the |
| + // same estate. |
| + asked := estateOf(rec) |
| + rec.Delivering = true |
| + if etag, err = Put(ctx, s, acct, app, rec, etag); errors.Is(err, release.ErrCASConflict) { |
| + fresh, freshEtag, gerr := Get(ctx, s, acct, app) |
| + if gerr != nil || fresh == nil || fresh.Provisioned == nil || fresh.Declared == nil || fresh.Teardown != nil || fresh.Delivering || estateOf(fresh) != asked { |
| + return nil, err |
| + } |
| + fresh.Delivering = true |
| + if etag, err = Put(ctx, s, acct, app, fresh, freshEtag); err != nil { |
| + return nil, err |
| + } |
| + rec = fresh |
| + } else if err != nil { |
| + return nil, err |
| + } |
| pr := rec.Provisioned |
| startKeyID := pr.KeyID // the key the app is holding, and the retry guard |
| // 1. finish the pending rotation late. The reap below would collect this |
| @@ -697,12 +734,27 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| } |
| // 4. deliver — same names, same single CAS write, same validation of the |
| // credential halves before sealing (writeEnv owns that rail). |
| + // |
| + // If delivery fails and the app's config is byte for byte what it was |
| + // before the attempt, the new key was not delivered: the app still |
| + // holds the key the record names, and the mark comes off so that |
| + // running the rotation again resumes (it reaps the stray key and mints |
| + // another). If the config changed, or cannot be read, nothing is |
| + // assumed and the mark stays. |
| + cfgBefore, cfgKnown := configGeneration(ctx, s, acct, app) |
| if err := writeEnv(ctx, s, sealPub, acct, app, rec.Declared.EnvPrefix, |
| pr.Bucket, pr.Region, pr.Endpoint, id, secret); err != nil { |
| + if cfgAfter, ok := configGeneration(ctx, s, acct, app); cfgKnown && ok && cfgAfter == cfgBefore { |
| + rec.Delivering = false |
| + if _, perr := Put(ctx, s, acct, app, rec, etag); perr != nil { |
| + err = errors.Join(err, fmt.Errorf("objstore: clear the key-change mark of %s/%s: %w", acct, app, perr)) |
| + } |
| + } |
| return nil, err |
| } |
| // 5. record: the key just retired from live becomes the pending one. |
| pr.PrevKeyID, pr.KeyID, pr.At = pr.KeyID, id, now |
| + rec.Delivering = false |
| if _, err := Put(ctx, s, acct, app, rec, etag); err != nil { |
| if !errors.Is(err, release.ErrCASConflict) { |
| return nil, err |
| @@ -722,6 +774,7 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| return nil, err |
| } |
| fresh.Provisioned = pr |
| + fresh.Delivering = false |
| if _, perr := Put(ctx, s, acct, app, fresh, freshEtag); perr != nil { |
| return nil, perr |
| } |
| @@ -747,9 +800,6 @@ func RotateFinish(ctx context.Context, s release.Store, p Provisioner, acct, app |
| if rec == nil || rec.Provisioned == nil { |
| return nil, ErrNotProvisioned |
| } |
| - if err := Coherent(rec, acct, app); err != nil { |
| - return nil, err |
| - } |
| ctx, rec, etag, done, err := leaseForRotation(ctx, s, acct, app, rec) |
| if err != nil { |
| return nil, err |
| @@ -803,13 +853,11 @@ func leaseForRotation(ctx context.Context, s release.Store, acct, app string, fi |
| err = ErrNotProvisioned |
| case rec.Teardown != nil || estateOf(rec) != asked: |
| err = fmt.Errorf("objstore: store record for %s/%s changed while its rotation was starting; run it again: %w", acct, app, release.ErrCASConflict) |
| - case rec.Grant != nil && rec.Grant.Delivering: |
| + case rec.Delivering: |
| // A grant reaped the recorded key, may have delivered another, and |
| // did not record it. Rotating now would reap the key the app was |
| // given. The grant is run again first: it repairs in full. |
| err = fmt.Errorf("%w: %s/%s", ErrGrantUnfinished, acct, app) |
| - default: |
| - err = Coherent(rec, acct, app) |
| } |
| if err != nil { |
| // Nothing was called under the lease: it is given back clean. |
| @@ -853,17 +901,6 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| if rec == nil { |
| return nil |
| } |
| - // The record is deleted last because it is the only thing naming the |
| - // estate. A record that names two buckets cannot be followed to both: |
| - // this walk would destroy one, delete the record, and leave the other |
| - // with nothing naming it. A teardown note about a bucket that is not |
| - // the record's would have this walk skip the record's bucket as |
| - // already deleted. Both are refused here, before the declaration-only |
| - // delete as well: a record that carries such a note is not "declared, |
| - // never acted on". |
| - if err := Coherent(rec, acct, app); err != nil { |
| - return err |
| - } |
| // What this walk believes it is destroying — the guard on the final delete. |
| startKeyID, startBucket := "", "" |
| if rec.Provisioned != nil { |
| @@ -1330,13 +1367,35 @@ func storeHolders(ctx context.Context, s release.Store, bucket, acct, app string |
| if !release.ValidApp(name) || (a == acct && name == app) { |
| continue |
| } |
| - rec, _, err := Get(ctx, s, a, name) |
| + // Read as it is: a record nothing may act on still says which |
| + // buckets its app was given. For such a record every bucket |
| + // it names counts, and its teardown block excuses nothing: |
| + // which of them holds its objects cannot be told, and the |
| + // decision being made here is to destroy a bucket or to hand |
| + // out access to one. |
| + rec, _, err := Inspect(ctx, s, a, name) |
| if err != nil { |
| return nil, err |
| } |
| if rec == nil { |
| continue |
| } |
| + if Coherent(rec, a, name) != nil { |
| + named := false |
| + if rec.Provisioned != nil && rec.Provisioned.Bucket == bucket { |
| + named = true |
| + } |
| + if rec.Grant != nil && rec.Grant.Bucket == bucket { |
| + named = true |
| + } |
| + if rec.Teardown != nil && rec.Teardown.DeletedBucket == bucket { |
| + named = true |
| + } |
| + if named { |
| + out = append(out, OwnerRef{Account: a, App: name}) |
| + } |
| + continue |
| + } |
| held := "" |
| since := time.Time{} |
| switch { |
| @@ -1477,11 +1536,11 @@ func writeEnv(ctx context.Context, s release.Store, sealPub *ecdh.PublicKey, |
| } |
| // Coherent refuses a record that no walk of this build writes and that no |
| -// walk can follow. Every path that acts on a store record, at the provider |
| -// or on the record itself, checks it first: Grant, Teardown (before its |
| -// declaration-only delete too), both rotation verbs, the scanning doors |
| -// and the sweep. A refused record is left exactly as found; an operator |
| -// repairs it by hand. |
| +// walk can follow. It is made by Get, the one reader every door, pass and |
| +// walk uses, and by Put, so nothing acts on such a record and nothing |
| +// writes one. A refused record is left exactly as found; an operator |
| +// repairs it by hand. (Inspect reads one anyway, for the holder inventory |
| +// and for display.) |
| // |
| // Two shapes: |
| // |
| @@ -1516,9 +1575,25 @@ func Coherent(rec *Record, acct, app string) error { |
| switch noted := rec.Teardown.DeletedBucket; { |
| case noted != "" && noted != own: |
| return fmt.Errorf("%w: the teardown block of %s/%s notes bucket %s and the record's bucket is %q", ErrUnfinishedTeardown, acct, app, noted, own) |
| - case own == "" && !rec.Teardown.BucketDeletedAt.IsZero(): |
| - return fmt.Errorf("%w: the teardown block of %s/%s notes a deleted bucket and the record names none", ErrUnfinishedTeardown, acct, app) |
| + case !rec.Teardown.BucketDeletedAt.IsZero() && noted == "": |
| + // Teardown writes the instant and the name together. An |
| + // instant alone does not say which bucket it is about. |
| + return fmt.Errorf("%w: the teardown block of %s/%s notes a deletion and does not say of which bucket", ErrUnfinishedTeardown, acct, app) |
| } |
| } |
| return nil |
| } |
| + |
| +// configGeneration is the generation of the app's config object as the |
| +// store reports it ("" when there is none). ok is false when it could not |
| +// be read. |
| +func configGeneration(ctx context.Context, s release.Store, acct, app string) (generation string, ok bool) { |
| + _, etag, err := s.Get(ctx, appconfig.Key(acct, app)) |
| + if errors.Is(err, os.ErrNotExist) { |
| + return "", true |
| + } |
| + if err != nil { |
| + return "", false |
| + } |
| + return etag, true |
| +} |