amadan
Close round nine: a take-back is counted before it creates anything, a grant compares the estate, an unsupported name is not destroyed over another holder
Round nine (dispositions, "Round nine"): Astra two P1 and four P2, Opus five P3. - A grant over a teardown block counts itself on the record in its first write. A take-back that recreates and delivers and then fails no longer leaves a record that a stale teardown takes for the store it was asked about. - Grant keeps the estate from its first read and stops if the record under the lease differs, as teardown does. - A teardown of a bucket name this build would refuse reads the store records: another record naming the bucket means it is left, not destroyed. - The ledger is told a bucket was destroyed at the deletion, by a re-run that finds the note, and by a take-back before it voids the note. - Hashed lease keys live where no bucket name can. - An operation with a review of its own is not sent again while the review is open, however young it is. - The purge reads the store record once more after its marker has gone and reports a store granted during the purge. That reports the schedule; it does not prevent it, and OUTSTANDING.md says so.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
16 files changed,
+822
−41
docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md+25 −0docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round9.md+170 −0docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round9.md+187 −0implementation/carloku-paid-launch/AGENT_HANDOFF.md+2 −2implementation/carloku-paid-launch/OUTSTANDING.md+3 −2internal/console/billingquantity.go+14 −0internal/console/billingquantity_test.go+70 −0internal/objstore/fence.go+5 −1internal/objstore/objstore.go+7 −0internal/objstore/ownership.go+7 −11internal/objstore/ownership_test.go+227 −0internal/objstore/walk.go+64 −18internal/objstore/walk_sim_test.go+5 −1internal/objstore/walklock.go+6 −2internal/release/trash.go+27 −3internal/release/trash_test.go+3 −1
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 563628763..64cada8fb 100644 |
| --- a/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| +++ b/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| @@ -322,3 +322,28 @@ Fourteen findings in round six, nine in seven, five from Astra in eight. Neither |
| ### Mutation checks (scratch copy) |
| Each fails its test: the estate comparison removed; an unsupported name walked with no lease; the production store without its signer; the provisioner without its attempt watcher; the purge sweeping the record unconditionally; the purge deleting the declaration unconditionally; the review keyed by creation second; a replayed fence error filing nothing. |
| + |
| +## Round nine (reviewed `f5538467`, code `3d4104cb`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-carloku-paid-launch-overnight-round9.md`, `OPUS-REVIEW-carloku-paid-launch-overnight-round9.md`. |
| + |
| +Astra: two P1 and four P2. Opus: five P3. Neither found a way to attest, to reach Stripe with a switch unset, or to make an open late-request defect reachable. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra F1: a take-back that recreates and delivers, then fails at its last write, leaves a record that still compares equal, and a stale teardown destroys what it made | Accepted. A grant over a teardown block counts itself on the record (`TakeBacks`) in its first write, before it creates anything. The estate also carries whether the bucket was noted deleted. | `walk.go` `Grant`, `estateOf`; `objstore.go` `TeardownBlock` | `TestStaleTeardownDoesNotDestroyWhatAFailedTakeBackMade` (both shapes of the interrupted teardown) | |
| +| Opus N1: a stale grant takes back a store torn down after its request | Accepted. Grant keeps the estate from its first read and stops if the record under the lease differs. | `walk.go` `Grant` | `TestStaleGrantDoesNotTakeBackAStoreTornDownAfterItsRequest` | |
| +| Astra F2, Opus N4: an unsupported name is leased and then destroyed with no holder check | Accepted. The store records are read for such a name too. Another record naming the bucket means it is left, access revoked, and reported retained when that record is another account's. | `walk.go` `Teardown` | `TestTeardownOfAnUnsupportedNameLeavesABucketAnotherStoreHolds` | |
| +| Opus N3 (Astra's N13 remainder): a crash between the record's note and the ledger's, then a take-back, loses the destruction | Accepted. The ledger is told at the deletion, again by a re-run that finds the note, and by a take-back before it voids the note. The mark no longer depends on who the ledger names as destroying. | `ownership.go` `markBucketDestroyed`; `walk.go` | `TestDestructionReachesTheLedgerAfterACrashBetweenTheTwoWrites` | |
| +| Astra F4: a hashed lease key is a name a bucket could have | Accepted. Hashed leases live in a subdirectory no bucket name can reach. | `walklock.go` `walkLeaseKey` | `TestHashedLeaseKeysCannotCollideWithABucketName` | |
| +| Astra F5: a young operation keeps being sent while its review is open | Accepted. An operation with a review of its own is not sent: open, it waits; resolved, it goes to settlement. | `billingquantity.go` `resolveQuantityOperation` | `TestQuantityYoungOperationWithAnOpenReviewIsNotSentAgain` | |
| +| Astra F3, Opus N2: a purge reports success while a store granted at the very end remains | Partly. The purge reads the store record once more after the marker has gone and returns `ErrStoreProvisioned` if it carries estate, so nothing is reported as deleted and the console keeps the app's row. It does not prevent the schedule: a declare and a grant admitted while the app was live can still land between the last check and the marker's delete. Preventing it needs the doors to refuse a trashed app when they write, not only when they admit. Open. | `release/trash.go` | `TestPurgeAppNeverSweepsAStoreRecordGrantedDuringThePurge` (all five landing points now require the refusal) | |
| +| Astra F6: reviews filed under the creation-second reason are orphaned by the change to serials | Accepted as described, no migration written. That reason existed between two commits of this unmerged branch. The worker has never run anywhere, so no database holds such a review. Stated here so a later reader does not look for one. | none | none | |
| +| Opus N5: a sandbox pass leaves the worker's behaviour with a declining card undecided | Accepted. `OUTSTANDING.md` item 6 now says the 5b log must be read and decided on before the switch is set. The worker still sends no `payment_behavior` and records `charged` on any success. Open. | `OUTSTANDING.md` | none | |
| +| Opus, claims: stale comment on the record delete; the purge comment; `fence.go`'s unqualified sentence; the oracle test that did not delete the object | Accepted and corrected. The oracle test now deletes the object and the check finds it gone. | `walk.go`, `trash.go`, `fence.go`, `walk_sim_test.go` | `TestSimulationOracleSeesARegrantThatLosesData` | |
| + |
| +Also changed: the lease reads its clock after it reads the lease object, so a lease is judged from a time no earlier than the read. |
| + |
| +Mutation checks in a scratch copy, each failing its test: no take-back count; grant comparing names only; an unsupported name destroyed regardless of holders; an open review not stopping a send. |
| + |
| +Not done from this round: the simulations' remaining limits Astra lists (ownership sampled outside nested walks, a user's existence standing in for working credentials, the account-wide dating review, the accepted-held return skipping the final equality check); counting only walks that took the lease in the storage model. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round9.md b/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round9.md| new file mode 100644 |
| index 000000000..6c7c94429 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round9.md |
| @@ -0,0 +1,170 @@ |
| +# Round-nine review: `f5538467` |
| + |
| +## 1. Round-eight dispositions |
| + |
| +Reviewed `f5538467`, base `aed42815`, concentrating on `c3e87a94..f5538467`. This was a static review: no files changed, tests executed, or network commands run. Counterfactual test assessments below are based on reading the code and assertions, not performing mutations. |
| + |
| +All references identify files at `f5538467`. Abbreviations: **O** = `internal/objstore`, **C** = `internal/console`, **R** = `internal/release`, **B** = `internal/billingstripe`. |
| + |
| +| Round-eight finding | Disposition | Fix and regression | Would the regression pass without the fix? | |
| +|---|---|---|---| |
| +| Astra F1: purge sweeps a newly granted store record | **Closed for record deletion; partly closed for purge completion.** A late grant can still survive a successful purge; F3 below. | Conditional declaration deletion and sweep interception: `R/trash.go:223`, `:236`, `:269`. Test: `R/trash_test.go:515`. | **No** for the earlier landing points. The final-marker case explicitly permits success with the granted record still present. | |
| +| Astra F2 / Opus N6: unsupported or absent bucket names bypass leasing | **Closed for the reported bypass.** Unsupported names still bypass the shared-bucket destruction guard; F2 below. | `O/walklock.go:82`; unconditional lease acquisition at `O/walk.go:639`, `:742`. Test: `O/ownership_test.go:2112`. | **No.** The test requires competing walks to receive `ErrBucketBusy`. It does not test another store sharing the unsupported name. | |
| +| Astra F3 / Opus N4: one operation consumes another’s resolved review | **Closed for newly recorded operations with distinct serials.** Existing review identities are not migrated; F6 below. | Serial-based reason: `C/billingquantity.go:111`, `:736`, `:1273`. Test: `C/billingquantity_test.go:2372`. | **No.** The test creates two operations with the same creation second and requires separate reviews. | |
| +| Astra F4: remembered metadata-fence 500 files no review | **Closed for the tested aged-operation path.** The young-operation path has a separate recovery gap; F5 below. | `C/billingquantity.go:1294`. Test: `C/billingquantity_test.go:2438`. | **No.** Removing the replayed-server-error branch leaves the required review absent and permits further fence attempts. | |
| +| Astra F5: quantity simulation accepts any open review | **Closed for the unrelated-review counterexample.** | `C/billingquantity_sim_test.go:344`, `:369`. Oracle regression: `:391`. | **No** if the former “any review” rule is restored in the helper. **Yes** if the simulation’s call to that helper is bypassed: the helper’s regression does not exercise the integration. | |
| +| Opus N1: stale teardown destroys a same-name replacement | **Partly closed.** The internal first-read race is covered, but the console’s confirmed identity still stops at the handler. A failed take-back also escapes the new comparison; F1 below. | `O/walk.go:737`, `:780`, `:1065`. Test: `O/ownership_test.go:2188`. Console gap: `C/appsettings.go:990`, `:1037`. | **No** without the estate comparison. **Yes** with either remaining gap: the test starts inside `Teardown` and completes the replacement grant successfully. | |
| +| Opus N2: app IAM user versus bucket-name lease | **Accepted and left open**, as `OUTSTANDING.md` 19b says. | No fix. Existing same-app retry and carry behavior remains at `O/walklock.go:150`. | No regression establishes closure of the reported cross-name schedule. | |
| +| Opus N3: far-ahead clock reserves the name | **Accepted and left open**, as 19b says. | No fix. Existing clock test: `O/ownership_test.go:2029`. | **Yes.** That test covers an early takeover, not recovery from a far-future reservation. | |
| +| Opus N5: production constructor wiring is untested | **Closed.** | Production wiring: `R/store.go:555`; `O/aws.go:83`, `:99`. New regression: `O/fence_wiring_test.go:22`. | **No.** It constructs the production clients and checks fenced stamps, ordinary stamps, multiple attempts and raised doubt. | |
| +| Opus N7, quantity Held assertion | **Accepted and left open**, as the dispositions say. | `C/billingquantity_sim_test.go:223` still accepts any open dating review for the account. | **Yes.** The new oracle regression concerns unsettled-operation reviews, not dating-review attribution. | |
| +| Opus N7, quantity review resolution | **Closed as a missing generated event.** | Resolution step: `C/billingquantity_sim_test.go:261`. | **Potentially yes.** Removing this random event merely reduces coverage; no assertion requires its execution. | |
| +| Opus N7 / Astra storage simulation items | **Partly closed.** Re-grant data checks and state preservation were added. Mid-walk ownership, credential usability and replacement identity remain outside the oracle, as the dispositions acknowledge. | `O/walk_sim_test.go:269`, `:294`, `:343`, `:560`. Oracle regression: `:214`. | **No** if the re-grant data check is removed. **Yes** if the newly generated pre-lease replacement schedule is removed. | |
| +| Opus N8 / Astra sandbox-procedure items | **Partly closed; not run.** Added declined-card/error-replay probing and retries of race losers. Cleanup wording and goroutine request handling improved. A failed canceled-subscription reread still passes. | `B/sandbox_fence_test.go:344`, `:366`, `:421`; procedure begins at `:45`. | **Not established.** This is an unrun external procedure, not an independently tested regression. Its declined-card request using the worker’s actual parameters is logged rather than asserted at `:403`. | |
| +| Opus N13 remainder: destruction history lost between persistence writes | **Not closed.** | Store note: `O/walk.go:925`; ledger write: `:933`; take-back discards the note at `:238`. Existing test: `O/ownership_test.go:1967`. | **Yes with the reported gap intact.** The test fails at `DeleteUser`, after both persistence writes succeeded. | |
| + |
| +Two remaining round-eight scenarios warrant explicit disposition: |
| + |
| +- **Console identity:** a destroy request confirms bucket A at `C/appsettings.go:1000`. Another process replaces the store before `:1037`. `Teardown` receives only account/app, reads the replacement as its initial estate, and destroys it. The handler’s process-local lock does not bind the confirmed record across processes. Pass the expected estate into the walk and compare it under the lease. |
| +- **N13:** A retains a teardown; B destroys their shared bucket and persists `BucketDeletedAt`, but the ledger write fails. B’s take-back clears the store note and the ledger’s destroying marker. A’s subsequent teardown still holds against B’s replacement bucket because the destruction evidence was lost. Recover that evidence before take-back discards it. |
| + |
| +The round-eight claim items have these dispositions: |
| + |
| +| Claim item | Disposition | |
| +|---|---| |
| +| Every authority-spending walk is leased, including unsupported names | **Closed for lease acquisition**, with the distinct identity and destruction defects below. | |
| +| A retained teardown finishes after destruction | **Partly closed as wording:** manual invocation is now stated. N13 still defeats that invocation. | |
| +| Resolved reviews identify the operation | **Closed for new serial-based reviews; migration incomplete.** | |
| +| Human review is an exception to provider fencing | **Partly closed:** the code comment states it; the evidence document’s opening description still omits it. | |
| +| Retirement uses a post-fence provider read | **Closed as wording** in `C/billingquantity.go:665`, which now names the ended/changed-subscription exceptions. | |
| +| Every unsettled operation gets a review and a way out | **Partly closed:** the aged replayed-fence case is fixed; F5 and F6 remain. | |
| +| AWS probe establishes retry behavior | **Closed by narrowing the claim**, `O/fence_aws_test.go:29`. | |
| +| Escaped-key probe accepts unrelated errors | **Closed in the procedure**, `O/fence_aws_test.go:123`. No rerun was performed here. | |
| +| Sandbox cleanup deletes everything | **Closed as wording**, `B/sandbox_fence_test.go:33`. Cleanup remains best effort. | |
| +| Same-kind takeover cannot overlap old accepted requests | **Not corrected** in the broad introductory fence claims; the exception is separately documented as open. | |
| +| Conditional deletion and non-AWS signing windows are proven | **Accepted and left open**, accurately qualified by `OUTSTANDING.md` 19c and 19d. | |
| + |
| +## 2. New findings |
| + |
| +### F1. P1 — A failed take-back can replace the bucket without changing `estateOf` |
| + |
| +**Where:** `O/walk.go:238`, `:419`, `:422`, `:780`, `:1059`. |
| + |
| +**Scenario:** |
| + |
| +1. A teardown deletes the bucket, records `BucketDeletedAt`, then fails before finishing. Its record retains the original grant, provisioned key K1 and teardown block. |
| +2. T1 reads that record and pauses before acquiring its lease. |
| +3. A take-back grant clears `BucketDeletedAt`, recreates the bucket, mints K2 and delivers K2 to the app. Its final store-record write fails before taking effect. |
| +4. The app can now write into the recreated bucket with K2. The record still contains the original grant timestamp, K1 and a nonnil teardown block. |
| +5. After the grant’s lease expires, T1 resumes. `estateOf` compares equal: it contains neither the deletion note nor a persisted take-back generation. T1 now sees `bucketDone == false` and destroys the recreated bucket. |
| + |
| +Both walks can obey their leases and fences. The stale request is accepted because the replacement became usable before its identity was recorded. |
| + |
| +**Smallest acceptable fix:** Persist a take-back generation before recreating or delivering anything, and include it in the expected estate identity. Add a regression that fails the take-back’s final record write after successful credential delivery, writes new data, then resumes the stale teardown. |
| + |
| +### F2. P1 — Unsupported names are leased but still destroy a bucket another store holds |
| + |
| +**Where:** `O/walk.go:835`, `:850`, `:892`, `:910`. |
| + |
| +**Scenario:** Two accepted legacy records name the same backend bucket, such as `Legacy_Bucket`, which the backend accepts but `ValidBucketName` rejects. Each record names its own user. |
| + |
| +Teardown of A acquires the new hashed lease, but takes the `destroy = true` branch at `:850`. It never calls `otherStoreHolders` or the ownership decision. It empties and deletes the bucket while B remains provisioned and entitled to its data. Serialization does not establish A’s authority to destroy shared contents. |
| + |
| +The unsupported-name regression contains only one holder, so it permits this behavior. |
| + |
| +**Smallest acceptable fix:** Refuse destructive bucket operations for unsupported identities until ownership can be established, or apply an equivalent holder check and destruction decision for those identities. Test two records sharing one unsupported name. |
| + |
| +### F3. P2 — Purge can report success, remove the tombstone and release the app row while new estate remains |
| + |
| +**Where:** `R/trash.go:247`, `:250`; `C/ownership.go:532`; `R/trash_test.go:515`. |
| + |
| +**Scenario:** Declaration and provision requests were admitted while the app was live, then delayed in another process. Purge reaches its final absent-store check. Before it deletes the marker, those requests create and provision a store. |
| + |
| +The marker is then deleted unconditionally and `PurgeApp` returns success. The console proceeds to delete the app row. The backend estate and `store.json` remain, but another `PurgeApp` invocation returns `ErrNotTrashed`, and the box’s tombstone-driven auto-purge no longer selects the app. |
| + |
| +This is already represented by the new test’s “as the marker is about to go” case. That case checks record preservation but explicitly exempts the purge from returning `ErrStoreProvisioned`. |
| + |
| +Admission does not prevent this schedule: `C/apistore.go:262` checks the live app before the declaration write at `:340`; `C/apistore_provision.go:16` checks it before the later grant at `:54`. |
| + |
| +**Smallest acceptable fix:** Coordinate declaration/provision admission and purge completion across processes, so final marker removal cannot race an admitted estate-creating request. Extend the regression through the console completion path and require that surviving estate cannot accompany successful purge and row removal. |
| + |
| +### F4. P2 — Hashed lease keys collide with ordinary valid bucket names |
| + |
| +**Where:** `O/walklock.go:72`, `:82`, `:91`. |
| + |
| +**Scenario:** Let `H` be the first 20 bytes of SHA-256 of `"app\x00bab/one"`, encoded as hexadecimal. |
| + |
| +The bucketless store `bab/one` leases: |
| + |
| +```text |
| +objstore/walks/x-H.json |
| +``` |
| + |
| +An unrelated bucket literally named `x-H` is a valid 42-character bucket name and leases exactly the same object through `walkLockKey`. The same construction works for an unsupported bucket’s hash. |
| + |
| +No cryptographic collision is required. One unrelated store’s active or failed walk blocks the other’s teardown, grant or rotation. |
| + |
| +**Smallest acceptable fix:** Put fallback identities in a disjoint key namespace, such as a subdirectory unavailable to valid bucket names. Test a fallback identity against the valid bucket name derived from its hash. |
| + |
| +### F5. P2 — A young operation can keep sending while its unsettled review is open |
| + |
| +**Where:** `C/billingquantity.go:1083`, `:1204`, `:1212`, `:1222`, `:1273`. |
| + |
| +**Scenario:** A young quantity update is refused for `proration_date`. Its immediate fence attempt is refused too, so the worker files the operation’s unsettled review while leaving `FencedAt == 0`. |
| + |
| +On the next pass, the provider still shows `FromQuantity`. The operation is neither older than 22 hours nor marked fenced, so `resolveQuantityOperation` falls through to `sendQuantityOperation`. It does not consult the open review. It can keep sending quantity requests while the review says a person must settle the operation. |
| + |
| +Resolving that review is likewise not necessarily consumed: if subsequent quantity requests return an error that does not name `proration_date`, the worker keeps replaying them until the age limit sends it through settlement. |
| + |
| +The same routing delays recovery from a metadata-fence 500 taken during a young operation: subsequent quantity requests can receive parameter-mismatch errors instead of replaying the fence and reaching the newly added review branch. |
| + |
| +The regressions at `C/billingquantity_test.go:2037` and `:2438` force operations past the replay limit, bypassing this path. |
| + |
| +**Smallest acceptable fix:** Consult the operation’s own review before choosing any resend. An open review must stop sends; a resolved review must enter settlement and the required subsequent-read path independently of operation age. |
| + |
| +### F6. P2 — Changing review reasons to serials leaves persisted reviews disconnected from their operations |
| + |
| +**Where:** `C/billingquantity.go:111`, `:426`, `:456`, `:736`, `:1273`; `C/billingstore.go:809`. |
| + |
| +**Scenario:** An existing database contains an outstanding operation and its review under the previous reason: |
| + |
| +```text |
| +billing_quantity_operation_unsettled:<created_at> |
| +``` |
| + |
| +After upgrade, the worker looks exclusively for: |
| + |
| +```text |
| +billing_quantity_operation_unsettled:<serial> |
| +``` |
| + |
| +The migration adds `serial` with default zero where necessary but does not migrate or bind existing review rows and resolution history. |
| + |
| +Resolving the existing review therefore does not settle its operation. The worker can file another review under the new reason. Conversely, resolving the new review and completing the operation leaves the old unresolved review holding future invoice freezes. |
| + |
| +A serial-zero migrated row is not itself evidence of duplicate current operations: there is one operation row per account. The concrete defect is losing the relationship to its existing review. |
| + |
| +**Smallest acceptable fix:** Migrate or explicitly associate the outstanding operation’s legacy review and resolution history with its new identity. Do not restore an unrestricted creation-time fallback. Test upgrades with both open and resolved legacy reviews, including a serial-zero operation. |
| + |
| +### Simulation assessment |
| + |
| +The storage simulation now generates pre-lease replacement schedules, nested walks, lease expiry and provider/store failures before and after effects. It still cannot validate the replacement-identity property it now generates: that limitation is explicitly admitted in the dispositions. Ownership is sampled outside nested walks, and user existence substitutes for usable credentials. I did not establish an additional independent bookkeeping defect beyond those acknowledged limitations. |
| + |
| +The quantity simulation now rejects unrelated reviews during settlement and sometimes resolves operation reviews. Its dating-review check remains account-wide. Its accepted-held return at `C/billingquantity_sim_test.go:344` also bypasses the final quantity-equality checks; the dedicated oracle test exercises the helper rather than that complete settlement path. |
| + |
| +No additional bypass of the two quantity switches or production attestation-enabling call was found in the inspected paths. The expressly excluded open defects are not repeated here as new findings. |
| + |
| +## 3. Claims the code contradicts |
| + |
| +| Location | Claim | Contradiction or qualification | |
| +|---|---|---| |
| +| `implementation/carloku-paid-launch/AGENT_HANDOFF.md:35`; `OUTSTANDING.md:68` | The reread must identify the same estate, not merely the same name. | F1 shows distinct usable bucket incarnations comparing equal. Grant itself compares `bucketFor(rec)` only at `O/walk.go:215`; rotations compare bucket/key and teardown presence at `:653`, not the full grant identity. | |
| +| `AGENT_HANDOFF.md:35` | Teardown decides through the ledger and leaves a bucket another app holds. | Unsupported names take the unconditional destruction branch. F2. | |
| +| `AGENT_HANDOFF.md:35`; `OUTSTANDING.md:62` | A retained teardown can finish after the bucket was destroyed when invoked again. | N13’s persistence gap can erase the evidence that would release it. Another invocation is necessary but not sufficient. | |
| +| Dispositions, round-eight purge row | Estate appearing during purge causes refusal. | The final-marker regression permits successful purge with estate remaining. F3. The narrower claim that the record itself survives is supported. | |
| +| `OUTSTANDING.md:69` | Operations that cannot settle have a hold and a way out through a resolved review. | Young operations can bypass their review on resend, and legacy review resolutions are disconnected after upgrade. F5 and F6. | |
| +| Evidence document, opening fence description | Every unconfirmed acknowledgement first consumes the key through a metadata-only request. | Human resolution can substitute for provider fencing at `C/billingquantity.go:745`. The code comment now states this exception; the evidence introduction does not. | |
| +| `O/walklock.go:42`; `O/fence.go:33` | A failed lease cannot be taken over while its earlier requests could still be accepted. | Same-app, same-kind retry can take an ended lease immediately at `O/walklock.go:150`. This is the already documented open exception, not a new finding. | |
| +| Dispositions, sandbox-procedure row | The canceled-subscription check reads quantity again as verification. | `B/sandbox_fence_test.go:421` silently accepts a failed reread or an unexpected item count. A pass does not establish unchanged quantity in those cases. | |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round9.md b/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round9.md| new file mode 100644 |
| index 000000000..d5c201bb7 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round9.md |
| @@ -0,0 +1,187 @@ |
| +# Opus production and authority review, round 9: Carloku paid launch, `aed42815..f5538467` |
| + |
| +I read `c3e87a94..f5538467` with `git show` only. This was a read-only pass: no tests run, nothing changed, no network calls. Every "would the test fail without the fix?" answer comes from reading the code. The code tip is `3d4104cb`; `f5538467` adds documents only. |
| + |
| +**Production authority** |
| + |
| +- **Nothing attests.** `SetAttestation` and `SetAllClientsInstrumented` are defined at `usage/durabilityledger.go:1455` and `:1475`. Nothing outside tests calls them, and nothing under `internal/usage`, `internal/host` or `cmd` changed this round. |
| +- **The quantity worker cannot reach Stripe with either switch unset.** |
| + - There are still only two non-test provider calls: `UpdateQuantityWith` (`C/billingquantity.go:1059`) and `FenceQuantityKey` (`:1287`). |
| + - Both are reachable only through `billingQuantityPass`. That function returns before any read unless `quantityWorker()` (`:133-139`) sees a proration policy and `CONSOLE_BILLING_QUANTITY_FENCE=stripe-sandbox-verified` (`:823-826`). |
| + - `UpdateQuantity` still has no non-test caller. |
| +- **Billing a customer wrongly.** I found no reachable path. Billing is off. The new serial-keyed review settles only its own operation. |
| +- **Leaks.** No new log line or error string carries a key, a secret, a response body or a Stripe identifier. The lease key hashes the names it cannot embed. |
| +- **Customer data.** |
| + - The stale teardown of round 8 (N1) is closed. |
| + - I found no new path that destroys a customer's data. Two new paths resurrect or orphan estate (N1 and N2 below). Neither loses data. |
| + - One older path destroys data with no holder check, but only for hand-edited records (N4). |
| + |
| +Abbreviations: **O** = `internal/objstore`, **C** = `internal/console`, **B** = `internal/billingstripe`, **R** = `internal/release`. |
| + |
| +## 1. Disposition table (round 8) |
| + |
| +| Finding | Status | Fix | Test; fails without the fix? | |
| +|---|---|---|---| |
| +| Astra F1: the purge sweeps `store.json` over a grant that landed | **Closed for the four landing points named.** One landing point remains, and the test accepts it silently (N2). | `R/trash.go:223-225`, `:236-241`, `:245-247`; `purgeStoreDeclaration` `:266` | `R/trash_test.go:515`. Cases 1 to 4 **fail** with either the unconditional sweep or the unconditional delete. Case 5 asserts nothing about the error. | |
| +| Astra F2 / Opus N6: invalid or missing bucket names walk unleased | **Closed** for rotation and teardown. Teardown of an invalid name still destroys with no holder check (N4). | `O/walklock.go:82-92`; bypass removed at `O/walk.go:637`; teardown always leases `:741-746` | `O/ownership_test.go:2112` **fails** without the lease. The `""` case also fails if every bucketless record shares one key. | |
| +| Astra F3 / Opus N4: a resolved review can settle another operation | **Closed** | `C/billingquantity.go:111`, `:736`, `:1078`, `:1259`, `:1273` | `C/billingquantity_test.go:2372` **fails** with the creation-second reason. Both operations are forced into the same second. | |
| +| Astra F4: a replayed fence 500 files no review | **Closed** | `C/billingquantity.go:1294-1300`; 409 is no longer treated as a definite refusal | `:2438` **fails**: without the fix the provider is asked 5 times and no review is filed. | |
| +| Astra F5 / Opus N7 (Settles): any open review excuses pending additions | **Closed.** A person now resolves reviews in the simulation. The Held check is still matched by account, and that is accepted and left open. | `billingquantity_sim_test.go:259-271`, `quantityHeldBy` `:368` | `:391` **fails** if the helper reverts to "any open review". | |
| +| Opus N1: a stale teardown destroys a store re-minted under the same name | **Closed for teardown.** Grant still compares the name only (N1 below). The simulation cannot see the fault (dispositions say so). | `O/walk.go:737`, `:780-782`, `estateOf` `:1065` | `O/ownership_test.go:2182` **fails** without the estate comparison. The fixture asserts the same name and a new key. | |
| +| Opus N2: the user is per app, the lease per name | **Accepted, open** (`OUTSTANDING.md` 19b). Code unchanged. | — | — | |
| +| Opus N3: a far-ahead clock wedges a name | **Accepted, open** (19b). Code unchanged. | — | — | |
| +| Opus N5: production constructors untested for signer and watcher | **Closed.** Not run here. | `O/fence_wiring_test.go:22` | **Fails** if the signer or the watcher is removed from `NewS3Store`, the provisioner's S3 client or its IAM client. | |
| +| Opus N7: storage simulation bookkeeping | **Partly closed.** Added: re-grant data check, a pause before the lease with tear-down and re-grant, and fenced or refused walks keep the inner walk's state. Still missing: ownership checks mid-walk, a second clock, a second name, rotations. | `walk_sim_test.go:294-298`, `:561-581`, `:278-290`, `:340-358` | `:215` uses `\|\| simRegrantFault` to force the report. It proves the branch is reached, not that the presence check detects a real loss (claims, §3). | |
| +| Opus N8: sandbox procedure | **Partly closed. Not run.** Added: a declining card, a replayed-402 assertion, a fence re-sent after a 409, goroutine-safe sends, an honest cleanup comment. The worker's own decline outcome is logged, not asserted (N5). | `B/sandbox_fence_test.go:366-406`, `:346-350`, `:413-423` | n/a | |
| +| Astra N13 remainder: a crash between `BucketDeletedAt` and `markBucketDestroyed` is erased by a take-back | **Not closed and not dispositioned.** Round 8's dispositions do not mention it. | — | — (N3 below) | |
| +| Astra probe claims (retry, escaped key) | **Closed** by rewording and a tighter assertion | `O/fence_aws_test.go:28-37`, `:123` | n/a, not run | |
| + |
| +## 2. New findings, most severe first |
| + |
| +### N1. P3: a stale grant takes back a store that was torn down after its request |
| + |
| +**Where:** `O/walk.go:215`. Grant re-checks only `bucketFor(rec) == bucket` under the lease, then re-derives `rebuilt` from the fresh record (`:218`). |
| + |
| +**Scenario:** |
| + |
| +1. Grant G1 reads a declaration-only record and is paused before `acquireBucketWalk`. G1's retry on another process grants bucket M, and the app writes data. |
| +2. The member destroys the store. Teardown T deletes M and records `BucketDeletedAt`, then fails at `DeleteUser`. The record keeps its teardown block, and the lease is ended for 30 minutes. |
| +3. G1 wakes after that. The lease is free. Under the lease the record leads to the same name M. |
| +4. G1 runs as a take-back: |
| + - it voids `BucketDeletedAt` (`:236-238`); |
| + - it clears the ledger's `Destroying` marker (`O/ownership.go:340-345`); |
| + - it re-creates M, mints a key, delivers credentials and clears the teardown. |
| + |
| +The member's destroy is undone by a request made before it. Nothing is lost, but a billing bucket and live credentials return, and step 4 also triggers N3. |
| + |
| +This is the brief's "verb that compares names only". `AGENT_HANDOFF.md` item 5 says every walk re-reads and requires "the same estate (same grant, same key)", which does not hold for Grant. |
| + |
| +**Smallest fix:** |
| + |
| +- Take `estateOf(rec)` from Grant's first read. |
| +- Refuse with `ErrCASConflict` if the record under the lease differs. A legitimate re-run or take-back reads the same estate both times. |
| +- Test: pause G1 before the lease, grant, fail a teardown after `DeleteBucket`, resume G1. |
| + |
| +### N2. P3: a purge can report success and leave an estate record that no door reaches |
| + |
| +**Where:** |
| + |
| +- `R/trash.go:245-249`: the last `purgeStoreDeclaration`, then `s.Delete(marker)`. |
| +- The test's fifth case (`R/trash_test.go:530`, `:566`) lands a granted record at this point and accepts `err == nil`. |
| + |
| +**Scenario:** |
| + |
| +1. A declare and a grant were both admitted while the app was live (`C/apistore.go:236-243` checks trash only at admission). |
| +2. The declare lands after the purge deleted the declaration. It is a create-only `Put`, so it succeeds on the absent object. |
| +3. The grant runs in full and lands between the final check and the marker delete. |
| +4. `PurgeApp` returns nil. The marker is gone, and the sweep removed everything else. |
| + |
| +What is left: |
| + |
| +- `store.json` names a live bucket and IAM user, with sealed credentials in a fresh `config.json`. |
| +- The app no longer exists, so the store-destroy door (`accountApp`) and the purge door (which needs the trash marker) both 404. |
| +- The claim gate refuses the name. |
| + |
| +The record survives, which was round 8's requirement, but nothing can act on it and nobody is told. It needs two paused requests, which is the threat model this branch uses. |
| + |
| +**Smallest fix:** |
| + |
| +- After deleting the marker, read `store.json` once more. If it carries estate, return `ErrStoreProvisioned`, or a distinct error that names it, so the console's flash and the box's log report an orphan instead of "purged". |
| +- Make the fifth test case require that error. |
| + |
| +### N3. P3: the N13 remainder; a retained teardown may never learn its bucket was destroyed |
| + |
| +**Where:** |
| + |
| +- `O/walk.go:925-935`: `BucketDeletedAt` is `Put` first, `markBucketDestroyed` second. |
| +- A re-run with `bucketDone` never repeats the ledger mark (`destroy` stays false, `:833-854`). |
| +- Grant's take-back voids the note (`:236-238`) and `OpenBucketOwner` clears `Destroying` (`O/ownership.go:340-345`), and neither records `DestroyedAt`. |
| + |
| +**Scenario:** |
| + |
| +1. Account X's app A left the bucket and is retained. |
| +2. B destroys the bucket and persists `BucketDeletedAt`. |
| +3. B crashes before `markBucketDestroyed`. |
| +4. B's app takes the store back. The note is voided, the marker is cleared, and no `DestroyedAt` is written. |
| +5. A's re-run finds B's open interval and no `DestroyedAt` later than its leaving (`:976-990`). It returns `ErrBucketRetained` for as long as B holds the name. |
| + |
| +This is a hold with no exit and a review that is never filed. N1 gives it a second entry point. |
| + |
| +**Smallest fix:** |
| + |
| +- In the take-back, either Grant before voiding the note or `OpenBucketOwner` when it clears a `Destroying` that names this app: set `DestroyedAt` from the record's `BucketDeletedAt` first. |
| +- Have a `bucketDone` re-run repeat `markBucketDestroyed`, which is idempotent. |
| +- Test: crash between the two writes, take back, re-run A. |
| + |
| +### N4. P3: an invalid-name teardown is now leased, but still destroys the bucket with no holder check |
| + |
| +**Where:** `O/walk.go:850-854`. `destroy = true` for any non-empty name `ValidBucketName` refuses, with no `otherStoreHolders` read and no ledger. `confirm` also skips its re-check for such names (`:875-877`). |
| + |
| +**Scenario:** |
| + |
| +- Two records of different accounts name the same legacy invalid bucket I. Only a hand edit or a pre-validation adopt can produce that. |
| +- Teardown of one empties and deletes I. The other account's data goes with it. |
| +- The new hash lease (`walkLeaseKey`) makes the two teardowns exclusive. It does not stop the first from destroying the second's bucket. |
| + |
| +**Smallest fix:** |
| + |
| +- Run `otherStoreHolders` for invalid names too. It compares strings, so it works on any name. |
| +- Refuse to destroy, or revoke-only, when another record names the bucket. |
| +- Or refuse invalid-name teardown outright and route it to an operator. |
| + |
| +### N5. P3: a sandbox pass does not settle what the worker does with a declined card |
| + |
| +**Where:** `B/sandbox_fence_test.go:403-404`. The worker's own update with a declining card, using the provider's default `payment_behavior` (the worker sets none: `B/client.go` has no `payment_behavior`), is logged as `OBSERVED`. The log line says that a 200 with an unpaid invoice "is a finding for the worker". |
| + |
| +`OUTSTANDING.md` item 6 gates `CONSOLE_BILLING_QUANTITY_FENCE` on the test *passing*. Item 6a names the canceled-subscription log as a decision to read, but nothing names this one. |
| + |
| +**Scenario:** the run passes. The default update returns 200, the quantity moves, and the proration invoice is open and unpaid. The switch is set, and the worker records `charged` for seats whose invoice failed. The amount is right; the recorded outcome is wrong, and no review is filed. |
| + |
| +**Smallest fix:** do one of these: |
| + |
| +- add to item 6 that the 5b log must be read and decided on before the switch is set; or |
| +- assert the outcome in 5b; or |
| +- have the worker send an explicit `payment_behavior` and handle the 402. |
| + |
| +### Asked and not found |
| + |
| +- **Estate identity.** |
| + - `estateOf` covers bucket, key, grant instant and whether a teardown had begun. |
| + - Two genuinely different estates cannot compare equal unless IAM reissues a key id or the grant lands in the same nanosecond. |
| + - Rotation compares bucket, key and teardown (`O/walk.go:653`), which is enough because key ids are unique. |
| + - A concurrent failed teardown of the same estate makes the stale one stop with a "replaced" message. That is a misleading message, not a hold. |
| +- **Lease keys.** |
| + - No two walks on one record shape take different keys, except Grant against a hand-edited record whose `Grant.Bucket` is empty while `Provisioned` exists. Grant never writes that shape. |
| + - An operator-adopted valid name of the form `x-<40 hex>` would share a key with a hashed one. That is over-exclusion only. |
| + - No record shape reaches the provisioner unleased. The declaration-only teardown makes no provider call. |
| +- **Reviews by serial.** |
| + - Serials are one counter per account on a single row that is never deleted (`C/billingquantity.go:513-519`). |
| + - A pre-serial row has serial 0 once. The next operation is serial 1. |
| + - Reviews filed by an older build under `unsettled:<created_at>` would be orphaned (an extra open review, failing safe). No production database has any: the worker never ran. |
| + - `fileBillingReviewTx` reopens a row by key, but no two operations share a key. |
| +- **Holds without an exit (quantity).** |
| + - Refused fence, replayed 500 fence, and request held by its own key: each files once and moves on when resolved. |
| + - An unmarked remembered 500 retries until Stripe forgets the key. That is bounded, not stuck. |
| +- **A sixth simulation fault.** I found none that hides a violation. |
| + - One fragility: `a.walks` counts walks *started*, including nested walks refused with `ErrBucketBusy`. So "another walk of this app ran" can be true when nothing ran. |
| + - It cannot hide a violation today: a late `ErrCASConflict` or `ErrFenced` on the outer walk needs a writer that a refused walk is not. |
| + - Count only walks that took the lease. |
| +- **Fence and lease code** (`fence.go`, `aws.go`, `walklock.go` apart from `walkLeaseKey`): unchanged since my round 8 reading. Nothing new found. |
| + |
| +## 3. Claims the code contradicts |
| + |
| +| Location | Claim | What the code supports | |
| +|---|---|---| |
| +| `O/walk.go:1163-1173` (`deleteRecordIfUnchanged`) | "This narrows the window; it does not close it … only a CAS delete in the Store interface could fix that." | The function now deletes with `DeleteIfMatch` (`:1197`). The comment is stale and points readers at the console lock as the remaining guard. | |
| +| `R/trash.go:218-221` | "A grant that lands after it finds no declaration, or loses its write to a record that is gone." | A re-declare is a create-only `Put` on the absent object, and a grant after it lands. The test's fifth case does exactly this, and the purge returns success (N2). The round 8 dispositions' F1 row ("a record that carries estate … refuses the purge") does not hold at that point. | |
| +| `AGENT_HANDOFF.md` item 5 | "The record is read again under the lease and must still be the same estate (same grant, same key), not only the same name." | True for teardown. Grant compares the name only (N1). Rotation compares bucket and key. | |
| +| `O/fence.go:35-38` | "by the time another walk may take the name, every request the earlier walk could ever send is one the backend refuses." | The same app re-running the same kind takes an ended lease at once (`walklock.go:146-151`). `walklock.go` now states the exception; `fence.go` still does not (Opus N2, open). | |
| +| `AGENT_HANDOFF.md` "Next actions" 2; dispositions "every finding was accepted" | Every round 8 finding is accepted or recorded as not fixed. | Astra's N13 remainder (round 8 disposition row "partly closed … crash gap") appears in neither list (N3). | |
| +| Dispositions, storage oracle row: "with a test of that check" | The re-grant data check is tested. | `walk_sim_test.go:294` reports whenever `simRegrantFault` is set, whatever the bucket holds. The test shows the branch runs on a live re-grant, not that a lost object is detected. Inject the loss by deleting the object instead. | |
| +| `OUTSTANDING.md` item 6 | Passing `TestSandboxQuantityFence` is the gate for the second switch. | A pass leaves the worker's decline outcome unasserted (N5). | |
| + |
| +I found nothing in the evidence document, or in `OUTSTANDING.md` items 7, 9, 19b, 19c or 19d, that the code contradicts. Nothing this round makes items 7 or 9 reachable with both switches unset and billing off. |
| + |
| +## 4. Verdict |
| + |
| +**Accept with fixes.** Nothing attests, the worker cannot reach Stripe with either switch unset, and billing stays off. N1 and N3 should land with the storage changes. N2's error and test, N4, and the stale comments should be fixed before the claims in §3 are relied on. N5 must be resolved before `CONSOLE_BILLING_QUANTITY_FENCE` is set. |
diff --git a/implementation/carloku-paid-launch/AGENT_HANDOFF.md b/implementation/carloku-paid-launch/AGENT_HANDOFF.md| index f9be6a6f6..a6e2e9d91 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 (same grant, same key), not only the same name. 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: 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. `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, whether a teardown had begun and how many take-backs it has seen; rotation compares the bucket, the key and that no teardown has begun. 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: 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. `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). 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. |
| @@ -77,7 +77,7 @@ Merchant `acct_1UD7LXI4Nrr6AEsO`, Team Tito Limited, Ireland, EUR; Irish standar |
| ## Next actions, in order |
| 1. Apply approved copy from both reviews when `result.json` arrives. |
| -2. Independent review: Astra rejected rounds 1 to 8 (ten, eight, five, five, eleven, fourteen, nine and five findings); Opus accepted with fixes seven times (eight, eight, eight, nine, nine, thirteen, eight). Rounds seven and eight attacked the lease and the fences at their edges; every finding was accepted, and two of round eight's are recorded as not fixed (`docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md`, "Round eight"). A further round of both must pass on the exact tip before anything lands; do not read "fixed" as "accepted". |
| +2. Independent review: Astra rejected rounds 1 to 9 (ten, eight, five, five, eleven, fourteen, nine, five and six findings); Opus accepted with fixes eight times (eight, eight, eight, nine, nine, thirteen, eight, five). Rounds seven to nine attacked the lease and the fences at their edges; every finding was accepted, and those not fixed are named as open (`docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md`, "Round eight" and "Round nine"). A further round of both must pass on the exact tip before anything lands; do not read "fixed" as "accepted". |
| 3. Scoped approvals: Cost and Usage Report export; Stripe catalog, portal, webhook and the `rk_live` key; then a test-mode rehearsal and a real purchase. |
| 4. Build the principal inventory and the bucket ownership attestation (designed in `docs/superpowers/plans/`), quantity reduction at renewal, a `carlos` verb and documentation for the operator doors, then Free-tier enforcement. |
| 5. Build path B of the attestation plan (the platform's own replicator as sole writer). Until a box can prove its replication writes, that meter holds every month. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index c28f8def1..67c7ca12b 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -32,7 +32,7 @@ Each is concrete. None is waived. |
| ### Stripe |
| 5. **No catalog, portal configuration, webhook endpoint or runtime key exists.** The plan and a print-only script are written. Needs scoped approval, then a test-mode rehearsal, then a real purchase. |
| -6. **The quantity worker's fence is unverified against Stripe.** It rests on documented idempotency rules, modelled by a fake. `TestSandboxQuantityFence` must pass in a Stripe sandbox, under both proration policies, before `CONSOLE_BILLING_QUANTITY_FENCE` is set. Test mode is not authorized, so it has not been run. Two things it cannot settle and that stay open after it: what Stripe does with a key across an API key rotation, and how Stripe later reconciles a request it answered with a 500. |
| +6. **The quantity worker's fence is unverified against Stripe.** It rests on documented idempotency rules, modelled by a fake. `TestSandboxQuantityFence` must pass in a Stripe sandbox, under both proration policies, before `CONSOLE_BILLING_QUANTITY_FENCE` is set. Test mode is not authorized, so it has not been run. A pass is not enough by itself: check 5b logs what the worker's own update does with a declining card, and that log has to be read and decided on, because the worker records an addition as charged on any success and sends no `payment_behavior`. Two things it cannot settle and that stay open after it: what Stripe does with a key across an API key rotation, and how Stripe later reconciles a request it answered with a 500. |
| 6a. **An ended subscription's operation may need a person every time.** If Stripe refuses the fence request on a canceled subscription's item (the sandbox run logs what it answers), every canceled team subscription with an unconfirmed operation waits on a resolved review, and until then the account's quantity work is held, under a later subscription too. That needs a code decision from the sandbox log before the worker is switched on. |
| 7. **Defect, open: an old quantity request can run after Stripe has forgotten its key.** Stripe forgets keys after 24 hours. A sender frozen longer than that can still land a request for an operation that was retired and replaced. Today that is only detected after the write (`billing_quantity_lowered`), which is not a fix. It is being designed out on `feat/carloku-late-request-fencing-20261001` (`docs/superpowers/plans/2026-10-01-late-request-fencing.md`). The path that could cause it, the quantity worker, stays disabled until the design is built, reviewed and verified in a Stripe sandbox. Nobody is asked to accept this risk. |
| 8. **Stripe's treatment of a proration date outside the period** is assumed from documentation. Same sandbox run. |
| @@ -55,7 +55,8 @@ Each is concrete. None is waived. |
| ### Process |
| -19. **No review round has passed.** Astra rejected rounds one to eight; Opus accepted each with fixes. The fixes to round eight's findings are unreviewed. |
| +19. **No review round has passed.** Astra rejected rounds one to nine; Opus accepted each with fixes. The fixes to round nine's findings are unreviewed. |
| +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, so the estate is named and reachable, but it does not prevent it. The doors check liveness when they admit a request, not when they write. |
| 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. |
| 19c. **S3's conditional delete is unverified on a real backend.** The store walks and the app purge delete a record only as the generation they read. That has run against the directory store and an HTTP stub. No `If-Match` delete has been sent to AWS or to an S3-compatible store, and a backend that ignored the header would delete unconditionally. It needs one write to a scratch object, which is a production-adjacent write nobody has approved. |
| 19d. **The signing-time fence has been observed on AWS only.** An S3-compatible deployment or store endpoint has not been probed for its window. |
diff --git a/internal/console/billingquantity.go b/internal/console/billingquantity.go| index d3787302d..0f71bbfdc 100644 |
| --- a/internal/console/billingquantity.go |
| +++ b/internal/console/billingquantity.go |
| @@ -1219,6 +1219,20 @@ func (s *Server) resolveQuantityOperation(ctx context.Context, acct *Account, st |
| // run. It can never be sent again; it is retired. |
| return abandon() |
| } |
| + // An operation with a review of its own is not sent, whatever its age. |
| + // An open review means a person has been asked to settle it, and |
| + // sending again while they look would change what they are looking at. |
| + // A resolved one means they have: the operation goes to settlement, |
| + // not back to the provider. |
| + switch rev, err := s.store.billingReview(acct.ID, op.Subscription, billingQuantityUnsettledReason(op.Serial)); { |
| + case errors.Is(err, sql.ErrNoRows): |
| + case err != nil: |
| + return quantityStep{}, err |
| + case rev.ResolvedAt == 0: |
| + return quantityStep{done: true}, nil |
| + default: |
| + return abandon() |
| + } |
| return s.sendQuantityOperation(ctx, state, op, read) |
| } |
diff --git a/internal/console/billingquantity_test.go b/internal/console/billingquantity_test.go| index 8ad1d6d5e..0b20e6b45 100644 |
| --- a/internal/console/billingquantity_test.go |
| +++ b/internal/console/billingquantity_test.go |
| @@ -2478,3 +2478,73 @@ func TestQuantityFenceThatEndsInARememberedServerErrorIsHeldForAPerson(t *testin |
| t.Fatalf("something was sent: %+v", q.posts) |
| } |
| } |
| + |
| +// An operation with an open review is not sent again, however young it is. |
| +// Here a young operation is refused for its date, its fence is refused too, |
| +// and its review is filed. The provider still shows the old quantity and |
| +// the operation is nowhere near the replay limit, so without the review it |
| +// would be sent again on every pass. It is not. When a person resolves the |
| +// review the operation is settled and the addition is charged afresh. |
| +func TestQuantityYoungOperationWithAnOpenReviewIsNotSentAgain(t *testing.T) { |
| + fx, q, now := newQuantityFixture(t, billingProrationImmediate) |
| + ctx := context.Background() |
| + if err := fx.st.AddMember(OpsAccountID, "eleventh@example.test", RoleMember, now.Add(time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The provider's period has moved on (the console has not seen it yet), |
| + // so the request's date is refused; and the fence is refused as well. |
| + saved := q.start |
| + q.mu.Lock() |
| + q.refuseFence = true |
| + q.mu.Unlock() |
| + fx.srv.billing.quantityBeforeSend = func() { |
| + q.mu.Lock() |
| + q.start = time.Now().Add(time.Hour).UTC() |
| + q.mu.Unlock() |
| + } |
| + if err := fx.srv.billingQuantityPass(ctx, time.Now().UTC()); err == nil { |
| + t.Fatal("the refusals must surface") |
| + } |
| + fx.srv.billing.quantityBeforeSend = nil |
| + op, ok, _ := fx.st.quantityOperationFor(OpsAccountID) |
| + if !ok || op.AckedAt != 0 || op.FencedAt != 0 { |
| + t.Fatalf("fixture: want a young, unfenced, outstanding operation: %+v", op) |
| + } |
| + reason := billingQuantityUnsettledReason(op.Serial) |
| + if rev, err := fx.st.billingReview(OpsAccountID, "sub_team", reason); err != nil || rev.ResolvedAt != 0 { |
| + t.Fatalf("fixture: no open review for the operation: %+v %v", rev, err) |
| + } |
| + // The provider is back in the console's period and would now accept. |
| + q.mu.Lock() |
| + q.start = saved |
| + refusedSoFar := q.refused |
| + q.mu.Unlock() |
| + for i := 0; i < 4; i++ { |
| + if err := fx.srv.billingQuantityPass(ctx, time.Now().UTC()); err != nil { |
| + t.Fatalf("pass %d with the review open: %v", i, err) |
| + } |
| + } |
| + if len(q.posts) != 0 || q.refused != refusedSoFar || q.quantity != 10 { |
| + t.Fatalf("an operation with an open review was sent again: posts %+v refused %d -> %d", q.posts, refusedSoFar, q.refused) |
| + } |
| + if err := fx.st.SetOperator(reviewOperator, true); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: OpsAccountID, Subscription: "sub_team", Reason: reason, Occurrence: 1, Actor: reviewOperator, Note: "checked"}, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + q.mu.Lock() |
| + q.refuseFence = false |
| + q.mu.Unlock() |
| + for i := 0; i < 4; i++ { |
| + if err := fx.srv.billingQuantityPass(ctx, time.Now().UTC()); err != nil { |
| + t.Fatalf("pass %d after the review was resolved: %v", i, err) |
| + } |
| + } |
| + if len(q.posts) != 1 || q.posts[0]["key"] == op.Key || q.posts[0]["quantity"] != "11" { |
| + t.Fatalf("posts %+v: want the reviewed operation settled and the addition charged by a fresh one", q.posts) |
| + } |
| + if state, _ := fx.st.billingAccount(OpsAccountID); state.Quantity != 11 { |
| + t.Fatalf("billed %d, want 11", state.Quantity) |
| + } |
| +} |
diff --git a/internal/objstore/fence.go b/internal/objstore/fence.go| index a9b115042..8b4b99831 100644 |
| --- a/internal/objstore/fence.go |
| +++ b/internal/objstore/fence.go |
| @@ -36,7 +36,11 @@ import ( |
| // taken over until it has run out. So by the time another walk may take |
| // the name, every request the earlier walk could ever send is one the |
| // backend refuses. fenceSkew is how much longer than the window the lease |
| -// lasts. |
| +// lasts. One exception, stated in walklock.go: the same app re-running the |
| +// same kind of walk may take an ended lease at once. The name stays |
| +// reserved, but the user belongs to the app and not to the name, so an |
| +// earlier attempt's request on that user can still arrive beside the |
| +// re-run (an open finding, OUTSTANDING.md 19b). |
| // |
| // How much longer: a whole second window. Whoever takes the lease over |
| // judges "run out" by its own clock, and nothing here can check that clock |
diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go| index e1468a6ea..7a5678f5e 100644 |
| --- a/internal/objstore/objstore.go |
| +++ b/internal/objstore/objstore.go |
| @@ -160,6 +160,13 @@ type TeardownBlock struct { |
| // may create the bucket itself, so it voids this note before it does |
| // anything else, and a later teardown decides again from the ledger. |
| BucketDeletedAt time.Time `json:"bucket_deleted_at,omitzero"` |
| + // TakeBacks counts the grants that have begun over this teardown. It |
| + // is written before such a grant creates or delivers anything, so that |
| + // from that write on the record is a different estate to any walk that |
| + // read it before: the grant may put a bucket and credentials in place |
| + // and then fail before it records them, and a teardown asked about the |
| + // store as it was must not go on to destroy what the grant made. |
| + TakeBacks int `json:"take_backs,omitempty"` |
| } |
| func Key(acct, app string) string { return release.AppPrefix(acct, app) + "store.json" } |
diff --git a/internal/objstore/ownership.go b/internal/objstore/ownership.go| index 7633bfde7..64a4ef6c6 100644 |
| --- a/internal/objstore/ownership.go |
| +++ b/internal/objstore/ownership.go |
| @@ -508,23 +508,19 @@ func beginBucketTeardown(ctx context.Context, s release.Store, bucket, acct, app |
| return destroy, scannerOwed, other, foreign, err |
| } |
| -// markBucketDestroyed records on the ledger that acct/app's teardown has |
| -// deleted the bucket, at the moment it has. The marker must still be that |
| -// teardown's: a ledger that names nobody, or somebody else, as destroying |
| -// the bucket is not one this teardown may speak for. It is recorded at the |
| -// deletion and not only when the teardown closes its interval, because the |
| -// teardown may never get that far: its own app can take the store back, |
| +// markBucketDestroyed records on the ledger that the bucket was deleted at |
| +// `at`. It is a fact about the bucket, taken from the record of the teardown |
| +// that deleted it, and it is recorded whoever the ledger currently names as |
| +// destroying: the teardown's own app may have taken the store back since, |
| // which clears the marker, and stores that left this bucket to another |
| -// account would then never learn that it went. |
| -func markBucketDestroyed(ctx context.Context, s release.Store, bucket, acct, app string, at time.Time) error { |
| +// account would otherwise never learn that it went. It only ever moves the |
| +// instant forward. With no ledger there is nothing to tell. |
| +func markBucketDestroyed(ctx context.Context, s release.Store, bucket string, at time.Time) error { |
| at = at.UTC() |
| return writeBucketOwnership(ctx, s, bucket, func(b *BucketOwnership) (*BucketOwnership, error) { |
| if b == nil { |
| return nil, errOwnershipUnchanged |
| } |
| - if d := b.Destroying; d == nil || d.Account != acct || d.App != app { |
| - return nil, fmt.Errorf("objstore: ownership of %s changed during the teardown of %s/%s", bucket, acct, app) |
| - } |
| if b.DestroyedAt != nil && !b.DestroyedAt.Before(at) { |
| return nil, errOwnershipUnchanged |
| } |
diff --git a/internal/objstore/ownership_test.go b/internal/objstore/ownership_test.go| index b86ac4447..16c346db1 100644 |
| --- a/internal/objstore/ownership_test.go |
| +++ b/internal/objstore/ownership_test.go |
| @@ -7,6 +7,7 @@ import ( |
| "github.com/aws/aws-sdk-go-v2/aws" |
| "github.com/carlosframework/platform/internal/sigfence" |
| "net/http" |
| + "strings" |
| "testing" |
| "time" |
| @@ -2231,3 +2232,229 @@ func TestStaleTeardownDoesNotDestroyAStoreRecreatedUnderTheSameName(t *testing.T |
| t.Fatalf("the stale teardown marked the new store's bucket as being destroyed: %+v", ledger) |
| } |
| } |
| + |
| +// failingPutStore fails one write of one key: the nth Put of it, before it |
| +// is written. |
| +type failingPutStore struct { |
| + release.Store |
| + key string |
| + nth int |
| + seen int |
| +} |
| + |
| +func (f *failingPutStore) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) { |
| + if key == f.key { |
| + f.seen++ |
| + if f.seen == f.nth { |
| + return "", errors.New("injected store write") |
| + } |
| + } |
| + return f.Store.Put(ctx, key, body, ifMatch) |
| +} |
| + |
| +func (f *failingPutStore) DeleteIfMatch(ctx context.Context, key, etag string) error { |
| + return release.DeleteIfMatch(ctx, f.Store, key, etag) |
| +} |
| + |
| +// A teardown deletes the bucket and fails before it finishes. A second |
| +// teardown request reads that record and is held before its lease. A grant |
| +// then takes the store back: it creates the bucket again, mints a new key |
| +// and delivers it, and fails at its last write, so the record still shows |
| +// the old grant instant and the old key. The app writes data with the key |
| +// it was delivered. When the held teardown resumes, the record must not |
| +// look like the estate it was asked about: the take-back was counted on the |
| +// record before it created anything. |
| +func TestStaleTeardownDoesNotDestroyWhatAFailedTakeBackMade(t *testing.T) { |
| + // Two shapes of the interrupted teardown: one that had already deleted |
| + // the bucket (its note is voided by the take-back), and one that failed |
| + // before it deleted anything (nothing on the record changes but the |
| + // count of take-backs). |
| + for _, failAt := range []string{"DeleteUser", "EmptyBucket"} { |
| + 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") |
| + if _, err := Grant(ctx, dir, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.FailNext = map[string]error{failAt: errors.New("injected " + failAt)} |
| + if err := Teardown(ctx, dir, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatalf("%s: fixture: the first teardown was meant to fail", failAt) |
| + } |
| + p.FailNext = nil |
| + advance(walkLease + time.Minute) |
| + s := &hookStore{Store: dir, at: "get:" + walkLockKey(ledgerBucket)} |
| + s.hook = func() { |
| + // The take-back, failing at its final record write. |
| + 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.Errorf("%s: fixture: the take-back was meant to fail at its last write (err %v, writes %d)", failAt, err, failing.seen) |
| + } |
| + if _, ok := p.Buckets[ledgerBucket]; !ok { |
| + t.Errorf("%s: fixture: the bucket is not there after the take-back", failAt) |
| + return |
| + } |
| + p.Buckets[ledgerBucket]["invoice.pdf"] = []byte("written with the delivered key") |
| + advance(walkLease + time.Minute) |
| + } |
| + err := Teardown(ctx, s, p, nil, "bab", "titogo") |
| + if !s.done { |
| + t.Fatalf("%s: fixture: the held teardown never reached its lease", failAt) |
| + } |
| + if !errors.Is(err, release.ErrCASConflict) { |
| + t.Fatalf("%s: a teardown asked about the store before the take-back: %v, want it to stop", failAt, err) |
| + } |
| + if got := string(p.Buckets[ledgerBucket]["invoice.pdf"]); got != "written with the delivered key" { |
| + t.Fatalf("%s: what a failed take-back made was destroyed by a stale teardown (object now %q)", failAt, got) |
| + } |
| + } |
| +} |
| + |
| +// A grant reads a bare declaration and is held before its lease. Its retry |
| +// grants the store; the member then destroys it, and that teardown deletes |
| +// the bucket and fails before it finishes. The held grant wakes. By name |
| +// the record still leads to the same bucket, but it is not the declaration |
| +// the grant was asked about, and the grant must not run as a take-back that |
| +// undoes the member's destroy. |
| +func TestStaleGrantDoesNotTakeBackAStoreTornDownAfterItsRequest(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") |
| + s := &hookStore{Store: dir, at: "get:" + walkLockKey(ledgerBucket)} |
| + s.hook = func() { |
| + if _, err := Grant(ctx, dir, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Errorf("the retry that grants: %v", err) |
| + } |
| + p.FailNext = map[string]error{"DeleteUser": errors.New("injected DeleteUser")} |
| + if err := Teardown(ctx, dir, p, nil, "bab", "titogo"); err == nil { |
| + t.Error("fixture: the member's destroy was meant to fail after deleting the bucket") |
| + } |
| + p.FailNext = nil |
| + advance(walkLease + time.Minute) |
| + } |
| + _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)) |
| + if !s.done { |
| + t.Fatal("fixture: the held grant never reached its lease") |
| + } |
| + if !errors.Is(err, release.ErrCASConflict) { |
| + t.Fatalf("a grant made before the store was granted and torn down: %v, want it to stop", err) |
| + } |
| + if _, ok := p.Buckets[ledgerBucket]; ok { |
| + t.Fatal("the stale grant brought the destroyed bucket back") |
| + } |
| + rec, _, _ := Get(ctx, dir, "bab", "titogo") |
| + if rec == nil || rec.Teardown == nil || rec.Teardown.BucketDeletedAt.IsZero() || rec.Teardown.TakeBacks != 0 { |
| + t.Fatalf("the stale grant wrote to the record of the teardown in progress: %+v", rec) |
| + } |
| +} |
| + |
| +// The ledger learns that a bucket was destroyed even when the teardown that |
| +// deleted it died between noting that on its own record and telling the |
| +// ledger. A re-run tells it, and so does a grant that takes the store back. |
| +func TestDestructionReachesTheLedgerAfterACrashBetweenTheTwoWrites(t *testing.T) { |
| + for _, how := range []string{"re-run", "take-back"} { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := 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.FailNext = map[string]error{"DeleteUser": errors.New("injected DeleteUser")} |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatal("fixture: the teardown was meant to fail after deleting the bucket") |
| + } |
| + // As if it had died before the ledger was told. |
| + if err := writeBucketOwnership(ctx, s, ledgerBucket, func(b *BucketOwnership) (*BucketOwnership, error) { |
| + b.DestroyedAt = nil |
| + return b, nil |
| + }); err != nil { |
| + t.Fatal(err) |
| + } |
| + advance(walkLease + time.Minute) |
| + if how == "re-run" { |
| + p.FailNext = map[string]error{"DeleteUser": errors.New("injected DeleteUser")} |
| + _ = Teardown(ctx, s, p, nil, "bab", "titogo") |
| + } else { |
| + p.FailNext = nil |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); err != nil { |
| + t.Fatalf("the take-back: %v", err) |
| + } |
| + } |
| + ledger, _, err := ReadBucketOwnership(ctx, s, ledgerBucket) |
| + if err != nil || ledger == nil || ledger.DestroyedAt == nil { |
| + t.Fatalf("%s: the ledger was never told the bucket was destroyed: %+v %v", how, ledger, err) |
| + } |
| + } |
| +} |
| + |
| +// Two records name the same bucket, and its name is one this build would |
| +// refuse. The lease makes their walks take turns. It does not make the |
| +// bucket the first one's to destroy: a teardown of one store leaves the |
| +// bucket the other still holds, and says so when the other is another |
| +// account's. |
| +func TestTeardownOfAnUnsupportedNameLeavesABucketAnotherStoreHolds(t *testing.T) { |
| + const legacy = "Legacy_Bucket" |
| + for _, otherAcct := range []string{"bac", "bab"} { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + p.Buckets[legacy] = map[string][]byte{"theirs.pdf": []byte("the other store's data")} |
| + for _, o := range []struct{ acct, app string }{{"bab", "titogo"}, {otherAcct, "other"}} { |
| + user := UserName(o.acct, o.app) |
| + if _, err := p.EnsureUser(ctx, user, "/carlos/", "arn:aws:s3:::"+legacy, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + rec := &Record{Declared: &Declared{EnvPrefix: "S3", At: t0, By: "member@example.com"}, |
| + Grant: &GrantBlock{Backend: "aws", Bucket: legacy, Source: GrantSourceOperator, At: t0, By: "op"}, |
| + Provisioned: &Provisioned{Bucket: legacy, Region: "eu-west-1", IAMUser: user, KeyID: "K" + o.acct + o.app, At: t0}} |
| + if _, err := Put(ctx, s, o.acct, o.app, rec, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + err := Teardown(ctx, s, p, nil, "bab", "titogo") |
| + if got := string(p.Buckets[legacy]["theirs.pdf"]); got != "the other store's data" { |
| + t.Fatalf("other account %s: a teardown destroyed a bucket another store's record names (object now %q, err %v)", otherAcct, got, err) |
| + } |
| + if otherAcct != "bab" && !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("a bucket left to another account's store: %v, want ErrBucketRetained", err) |
| + } |
| + if otherAcct == "bab" && err != nil { |
| + t.Fatalf("a bucket left to a store of the same account: %v", err) |
| + } |
| + if _, ok := p.Users[UserName("bab", "titogo")]; ok { |
| + t.Fatal("the torn-down store kept its user") |
| + } |
| + if _, ok := p.Users[UserName(otherAcct, "other")]; !ok { |
| + t.Fatal("the other store lost its user") |
| + } |
| + } |
| +} |
| + |
| +// A lease taken under a hash lives where no bucket name can: a bucket |
| +// whose name happens to be the hash does not share its lease. |
| +func TestHashedLeaseKeysCannotCollideWithABucketName(t *testing.T) { |
| + for _, c := range []struct{ bucket, acct, app string }{{"", "bab", "one"}, {"Legacy_Bucket", "bab", "one"}} { |
| + key := walkLeaseKey(c.bucket, c.acct, c.app) |
| + name := strings.TrimSuffix(strings.TrimPrefix(key, "objstore/walks/"), ".json") |
| + if !strings.Contains(name, "/") { |
| + t.Fatalf("the fallback lease %q is a name a bucket could have", key) |
| + } |
| + for _, candidate := range []string{name, strings.ReplaceAll(name, "/", "-"), "x-" + name[strings.LastIndex(name, "/")+1:], name[strings.LastIndex(name, "/")+1:]} { |
| + if ValidBucketName(candidate) && walkLeaseKey(candidate, "bac", "other") == key { |
| + t.Fatalf("the bucket named %q shares the fallback lease %q", candidate, key) |
| + } |
| + } |
| + } |
| + if walkLeaseKey("carlos-store-a", "bab", "one") != walkLockKey("carlos-store-a") { |
| + t.Fatal("a valid bucket name is not leased under itself") |
| + } |
| +} |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index f21e8c328..c19446c0a 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -184,6 +184,11 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| return b |
| } |
| bucket := bucketFor(rec) |
| + // The estate this grant was asked about, as first read (Teardown keeps |
| + // the same). A grant made against a store that has since been torn |
| + // down, or taken back, by somebody else must not go on as a take-back |
| + // of whatever stands there now. |
| + asked := estateOf(rec) |
| // Before anything is created or recorded: the name is about to become the |
| // IAM policy's resource ARN, so it is checked whether it was minted from a |
| // deployment prefix or handed over by an operator adopting a bucket. |
| @@ -215,6 +220,9 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| if now := bucketFor(rec); now != bucket { |
| return nil, fmt.Errorf("objstore: store record for %s/%s changed while its walk was starting (it led to %s and now leads to %s); run it again: %w", acct, app, bucket, now, release.ErrCASConflict) |
| } |
| + if estateOf(rec) != asked { |
| + return nil, fmt.Errorf("objstore: the store of %s/%s changed while its grant was starting; the request was made against the store as it was, and nothing was touched: %w", acct, app, release.ErrCASConflict) |
| + } |
| rebuilt = rec.Teardown != nil |
| startKeyID = "" |
| if rec.Provisioned != nil { |
| @@ -233,11 +241,25 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // would be left with nobody holding it (simulation seed 1085). With |
| // the note gone the teardown decides again from the ledger, and a |
| // bucket that is not there costs it nothing. |
| - voided := rec.Teardown != nil && !rec.Teardown.BucketDeletedAt.IsZero() |
| - if voided { |
| - rec.Teardown.BucketDeletedAt = time.Time{} |
| + // |
| + // Before the note goes, what it says is put on the ledger: stores that |
| + // left this bucket to another account are waiting to learn that it was |
| + // destroyed, and after this grant nothing else will tell them. |
| + // |
| + // And the take-back is counted on the record, in the same write and |
| + // before anything is created: from here on the record is a different |
| + // estate to any walk that read it earlier (TeardownBlock.TakeBacks). |
| + takeBack := rec.Teardown != nil |
| + if takeBack { |
| + if at := rec.Teardown.BucketDeletedAt; !at.IsZero() { |
| + if err := markBucketDestroyed(ctx, s, bucket, at); err != nil { |
| + return nil, fmt.Errorf("objstore: record the destruction of %s: %w", bucket, err) |
| + } |
| + rec.Teardown.BucketDeletedAt = time.Time{} |
| + } |
| + rec.Teardown.TakeBacks++ |
| } |
| - if wroteGrant || voided { |
| + if wroteGrant || takeBack { |
| if etag, err = Put(ctx, s, acct, app, rec, etag); err != nil { |
| return nil, err |
| } |
| @@ -849,8 +871,33 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| } |
| } else if bucket != "" && !ValidBucketName(bucket) { |
| // A name Grant would refuse was never granted through this walk and |
| - // has no ledger. It is destroyed as it always was. |
| - destroy = true |
| + // has no ledger. The lease makes its walks take turns; it does not |
| + // say whose the bucket is. The store records do: if another record |
| + // names this bucket, this teardown leaves it, revokes its own |
| + // access, and (when that record is another account's) reports the |
| + // bucket as retained, exactly as for a name with a ledger. |
| + holders, herr := otherStoreHolders(ctx, s, bucket, acct, app) |
| + if herr != nil { |
| + return fmt.Errorf("objstore: list the stores holding %s: %w", bucket, herr) |
| + } |
| + destroy = len(holders) == 0 |
| + for _, h := range holders { |
| + if holder.Account == "" { |
| + holder = h |
| + } |
| + if h.Account != acct { |
| + foreign, holder = true, h |
| + } |
| + } |
| + if !destroy { |
| + log.Printf("objstore: teardown of %s/%s leaves bucket %s in place: %s/%s holds it", acct, app, bucket, holder.Account, holder.App) |
| + } |
| + } else if bucketDone && ValidBucketName(bucket) { |
| + // A re-run that finds the bucket already deleted: the ledger is told |
| + // again, in case the first run died between its two writes. |
| + if err := markBucketDestroyed(ctx, s, bucket, rec.Teardown.BucketDeletedAt); err != nil { |
| + return fmt.Errorf("objstore: record the destruction of %s: %w", bucket, err) |
| + } |
| } |
| // The scanner goes BEFORE the bucket, while the record that names it still |
| // exists. A Malware Protection plan (and its managed EventBridge rule) |
| @@ -930,7 +977,7 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| // account are waiting to learn that it is gone, and this teardown |
| // may never reach its close (its app can take the store back). |
| if ValidBucketName(bucket) { |
| - if err := markBucketDestroyed(ctx, s, bucket, acct, app, rec.Teardown.BucketDeletedAt); err != nil { |
| + if err := markBucketDestroyed(ctx, s, bucket, rec.Teardown.BucketDeletedAt); err != nil { |
| return fmt.Errorf("objstore: record the destruction of %s: %w", bucket, err) |
| } |
| } |
| @@ -1060,6 +1107,8 @@ type storeEstate struct { |
| bucket, keyID string |
| grantedAt int64 |
| tearingDown bool |
| + bucketDeleted bool |
| + takeBacks int |
| } |
| func estateOf(rec *Record) storeEstate { |
| @@ -1073,7 +1122,9 @@ func estateOf(rec *Record) storeEstate { |
| if rec.Provisioned != nil { |
| e.bucket, e.keyID = rec.Provisioned.Bucket, rec.Provisioned.KeyID |
| } |
| - e.tearingDown = rec.Teardown != nil |
| + if rec.Teardown != nil { |
| + e.tearingDown, e.bucketDeleted, e.takeBacks = true, !rec.Teardown.BucketDeletedAt.IsZero(), rec.Teardown.TakeBacks |
| + } |
| return e |
| } |
| @@ -1160,16 +1211,11 @@ func otherStoreHolders(ctx context.Context, s release.Store, bucket, acct, app s |
| // same delete. A record that has vanished under us is success, not conflict — |
| // another teardown got there first, which is the state we wanted. |
| // |
| -// This narrows the window; it does not close it. A grant landing between the |
| -// re-read and the Delete still loses its record, and only a CAS delete in the |
| -// Store interface could fix that. The console door serialises these verbs per |
| -// app (internal/console/storelock.go), so the remaining window is a fraction |
| -// of one round trip. |
| -// |
| -// That serialisation was asserted here long before it existed — no lock of any |
| -// kind guarded these walks until #381 — so if this reference ever stops |
| -// resolving, the sentence above is a claim about nothing and this window is a |
| -// whole walk wide again. |
| +// The delete is conditional on the generation this function reads |
| +// (release.DeleteIfMatch), so a grant that lands between the read and the |
| +// delete keeps its record: the backend refuses the delete. That rests on |
| +// the backend honouring the condition, which has been exercised against the |
| +// directory store and an HTTP stub and not against AWS. |
| func deleteRecordIfUnchanged(ctx context.Context, s release.Store, acct, app, startKeyID, startBucket string) error { |
| fresh, etag, err := Get(ctx, s, acct, app) |
| if err != nil { |
diff --git a/internal/objstore/walk_sim_test.go b/internal/objstore/walk_sim_test.go| index fe45351c3..1b7d4f238 100644 |
| --- a/internal/objstore/walk_sim_test.go |
| +++ b/internal/objstore/walk_sim_test.go |
| @@ -292,7 +292,11 @@ func simulateBucketName(t *testing.T, seed int64, steps int) (trace []string, pr |
| // A store that was already live is owed its data across a second |
| // grant. It is looked for before the model writes it again. |
| if was == "live" { |
| - if b, ok := mem.Buckets[ledgerBucket]; !ok || string(b[object(a)]) != a.name() || simRegrantFault { |
| + if simRegrantFault { |
| + // The oracle's own test: the grant lost the store's object. |
| + delete(mem.Buckets[ledgerBucket], object(a)) |
| + } |
| + if b, ok := mem.Buckets[ledgerBucket]; !ok || string(b[object(a)]) != a.name() { |
| mintLeak = fmt.Sprintf("Data: %s was live, was granted again, and its object is gone", a.name()) |
| } |
| } |
diff --git a/internal/objstore/walklock.go b/internal/objstore/walklock.go| index 7d1d100ce..cf8725eed 100644 |
| --- a/internal/objstore/walklock.go |
| +++ b/internal/objstore/walklock.go |
| @@ -87,8 +87,10 @@ func walkLeaseKey(bucket, acct, app string) string { |
| if bucket != "" { |
| what = "bucket\x00" + bucket |
| } |
| + // In a directory of its own: no bucket name contains a slash, so no |
| + // valid bucket's lease can be this object. |
| sum := sha256.Sum256([]byte(what)) |
| - return "objstore/walks/x-" + hex.EncodeToString(sum[:20]) + ".json" |
| + return "objstore/walks/by-hash/" + hex.EncodeToString(sum[:20]) + ".json" |
| } |
| type walkLock struct { |
| @@ -134,9 +136,11 @@ func acquireBucketWalk(ctx context.Context, s release.Store, bucket, acct, app, |
| } |
| key := walkLeaseKey(bucket, acct, app) |
| for attempt := 0; attempt < 3; attempt++ { |
| - now := walkClock() |
| var carry time.Time |
| prev, etag, err := s.Get(ctx, key) |
| + // The clock is read after the lease, so the lease is judged, and |
| + // this one stamped, no earlier than the read it is judged from. |
| + now := walkClock() |
| switch { |
| case errors.Is(err, os.ErrNotExist): |
| etag = "" |
diff --git a/internal/release/trash.go b/internal/release/trash.go| index b4bd4a671..b19a52236 100644 |
| --- a/internal/release/trash.go |
| +++ b/internal/release/trash.go |
| @@ -218,8 +218,10 @@ func PurgeApp(ctx context.Context, s Store, pub ed25519.PublicKey, acct, app str |
| // the guard just read (purgeStoreDeclaration). A grant that lands |
| // before that changes the generation and the purge refuses. A grant |
| // that lands after it finds no declaration, or loses its write to a |
| - // record that is gone. The same rule is applied to the key if the |
| - // listing shows it again, and once more before the marker goes. |
| + // record that is gone, unless a declare admitted earlier has put a |
| + // fresh declaration back for it. So the same rule is applied to the |
| + // key if the listing shows it again, once more before the marker goes, |
| + // and the record is read a last time after the marker has gone. |
| storeKey := prefix + "store.json" |
| if err := purgeStoreDeclaration(ctx, s, acct, app, storeKey); err != nil { |
| return err |
| @@ -246,7 +248,29 @@ func PurgeApp(ctx context.Context, s Store, pub ed25519.PublicKey, acct, app str |
| if err := purgeStoreDeclaration(ctx, s, acct, app, storeKey); err != nil { |
| return err |
| } |
| - return s.Delete(ctx, marker) |
| + if err := s.Delete(ctx, marker); err != nil { |
| + return err |
| + } |
| + // One read more, after the marker has gone. A declare and a grant that |
| + // were both admitted while the app was live can still land between the |
| + // check above and the marker's delete. Nothing here can stop that: the |
| + // doors check liveness when a request is admitted, not when it writes. |
| + // What this can do is not call it a purge. The record such a grant |
| + // wrote is still there (it is never swept), and this reports it, so |
| + // the caller says "an estate remains" and not "deleted", and keeps |
| + // whatever it would otherwise remove on success. |
| + if raw, _, err := s.Get(ctx, storeKey); err == nil { |
| + var rec struct { |
| + Grant *json.RawMessage `json:"grant"` |
| + Provisioned *json.RawMessage `json:"provisioned"` |
| + } |
| + if json.Unmarshal(raw, &rec) != nil || rec.Grant != nil || rec.Provisioned != nil { |
| + return fmt.Errorf("%s/%s: a store was granted while the app was being purged; its record stands and its estate must be torn down: %w", acct, app, ErrStoreProvisioned) |
| + } |
| + } else if !errors.Is(err, os.ErrNotExist) { |
| + return fmt.Errorf("release: %s/%s: cannot read %s after the purge: %w: %v", acct, app, storeKey, ErrStoreProvisioned, err) |
| + } |
| + return nil |
| } |
| // purgeStoreDeclaration removes an app's store record during a purge, and |
diff --git a/internal/release/trash_test.go b/internal/release/trash_test.go| index c2d5383ce..278940465 100644 |
| --- a/internal/release/trash_test.go |
| +++ b/internal/release/trash_test.go |
| @@ -563,7 +563,9 @@ func TestPurgeAppNeverSweepsAStoreRecordGrantedDuringThePurge(t *testing.T) { |
| if gerr != nil || string(raw) != granted { |
| t.Fatalf("the purge deleted the record of a store granted during it: %q %v (purge: %v)", raw, gerr, err) |
| } |
| - if tc.at != "delete:"+AppTombstoneKey("bab", "derp") && !errors.Is(err, ErrStoreProvisioned) { |
| + // Wherever it landed, the purge does not report success over a |
| + // store that was granted during it. |
| + if !errors.Is(err, ErrStoreProvisioned) { |
| t.Fatalf("PurgeApp = %v, want ErrStoreProvisioned", err) |
| } |
| }) |