Close round fourteen: a refused holder is never reconciled away, a scanning re-grant never takes a store back, and the rotation's mark is written at delivery
Round fourteen (Astra reject: two P1, two P2; Opus accept with fixes: two P2, two P3) found every round thirteen finding closed for its reported shape. Two of its findings were faults in round thirteen's own fixes. - A holder found through a record no walk can follow is marked and is not dropped because the ledger shows an old closed interval. - objstore.Regrant: the scanning doors' re-grant refuses a store whose teardown has begun, on the record read under the walk's lease. - Rotate writes the key-change mark just before delivery, so a failure before delivery is resumed with the app's key kept. A failed delivery keeps the mark: an unchanged config does not prove the write will not land. Such a rotation is repaired by the grant (OUTSTANDING 19h). - The mark is read from where the previous build of this branch kept it. - The sweep and the scan queue take nothing for a store under teardown. - A provisioned block with no bucket beside a grant that names one is refused. Each fix has a reproduction and was mutation-checked in a scratch copy. The round's reports, dispositions and the round fifteen brief are added. No review round has passed. Billing is off; nothing attests.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
15 files changed,
+741
−85
docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md+20 −0docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round14.md+123 −0docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round15.md+28 −0docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round14.md+131 −0implementation/carloku-paid-launch/AGENT_HANDOFF.md+1 −1implementation/carloku-paid-launch/OUTSTANDING.md+2 −1internal/console/apiscan.go+5 −2internal/console/apiscan_test.go+64 −3internal/objstore/objstore.go+16 −5internal/objstore/ownership.go+13 −1internal/objstore/ownership_test.go+238 −0internal/objstore/queue.go+2 −1internal/objstore/sweep.go+8 −0internal/objstore/walk.go+72 −61internal/objstore/walk_test.go+18 −10
CI passed — run details
diff --git a/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md b/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md| index 2676b6818..89b0eb15a 100644 |
| --- a/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| +++ b/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| @@ -433,3 +433,23 @@ Round twelve put one check in front of the paths that had been reported. Round t |
| | Both, noted: a record written by an earlier build carries no mark, and a failed delivery under that build still looks healthy | Accepted, not fixable from the record. Stated in the field's comment and the handoff. | `objstore.go` | none | |
| Mutation checks in a scratch copy, each failing its test: the reader's check off; the writer's check off; a refused record not counted as a holder; the grant block not made to name its bucket; the rotation's mark not written, and not read; the config comparison off; the closed-interval reconciliation off; the blank-note refusal off; the scanning writer's and the scan grant door's teardown refusals off; the purge not routing a teardown-only record. Storage simulation: 3,000 seeds on this code. |
| + |
| +## Round fourteen (reviewed `d5705a63`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-carloku-paid-launch-overnight-round14.md`, `OPUS-REVIEW-carloku-paid-launch-overnight-round14.md`. |
| + |
| +Both found every round thirteen finding closed for its reported shape, and Opus found the claim that nothing acts on or writes a refused record now holds. Astra: two P1, two P2. Opus: two P2, two P3. Two of them were faults in round thirteen's own fixes (the rotation's mark written too early; the mark moved without reading its earlier place). Nothing that attests, reaches Stripe with a switch unset, or makes an open late-request defect reachable. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra F1: a refused record's hold on a bucket is dropped when the ledger shows an old closed interval for that app, in the teardown's decision and in the member's take-back | Accepted. A holder found through a refused record is marked as such and is never reconciled against the ledger. | `ownership.go` `OwnerRef`, `beginBucketTeardown`; `walk.go` `storeHolders`, `Grant` | `TestARefusedRecordsHoldIsNotReconciledAwayByAnOldClosedInterval` (the bucket is retained; the member's take-back is refused) | |
| +| Astra F2: a scanning door writes the record, a teardown runs, and the door's re-grant then takes the store back with the operator's source | Accepted. The scanning doors re-grant through `objstore.Regrant`, which refuses a record carrying a teardown block as read under the walk's lease, before anything is written or called. | `walk.go` `Regrant`, `grant`; `console/apiscan.go` | `TestRegrantNeverTakesAStoreBack`, `TestScanRegrantDoesNotTakeBackAStoreTornDownInBetween` | |
| +| Astra F3: an unchanged config after a failed delivery does not prove the delivery will not land | Accepted. The comparison is removed. The mark stays on every failure from the delivery on. A rotation whose delivery failed is therefore repaired by the grant, which replaces the app's key with no overlap; before this branch such a rotation was resumed by running it again. Open as 19h. | `walk.go` `Rotate` | `TestRotateAfterAResealFailureIsRepairedByTheGrant` (was `TestRotateResumesAfterResealFailure`) | |
| +| Opus N1: the rotation's mark was written before its first step, so a failure before delivery forced a key swap with no overlap | Accepted; a fault of round thirteen's fix. The mark is written just before delivery. A rotation that failed earlier leaves no mark and is resumed with the app's key kept. | `walk.go` `Rotate` | `TestRotationThatFailsBeforeDeliveryIsResumedWithItsOverlap` (reap failure, mint failure) | |
| +| Astra F4: the mark moved from the grant block to the record and a record marked by the previous build of this branch read as unmarked | Accepted; a fault of round thirteen's fix. The reader carries the earlier field to the record's. | `objstore.go` `Inspect`, `GrantBlock.LegacyDelivering` | `TestTheMarkIsReadFromWhereAnEarlierBuildWroteIt` | |
| +| Opus N2: the scan pass sweeps a bucket a retained teardown left to another account and publishes coverage its member can read | Accepted. The sweep and the scan queue take nothing for a store whose teardown has begun. It predates this branch. | `sweep.go` `RunSweep`; `queue.go` `queuePermitted` | `TestAStoreUnderTeardownIsNotSweptOrQueued` (the lister is never called) | |
| +| Opus N3: a provisioned block with no bucket name beside a grant block that names one is accepted, and the teardown finds nothing to destroy | Accepted. Refused by the reader as a record that names two buckets. | `walk.go` `Coherent` | `TestAProvisionedBlockWithNoBucketBesideAGrantThatNamesOneIsRefused` | |
| +| Opus N4: the revoke door's guard against switching scanning off on a retained bucket is untested | Accepted. | `apiscan_test.go` | `TestScanDoorsDoNotReachTheScannerForARefusedRecord` (scanning pre-enabled; still enabled after the enforce and revoke doors) | |
| +| Both, claims: the rotation's doc, the reader's doc, "since round eleven the findings concern records no walk of this build writes" | Corrected. The last was false: several findings (a failed rotation, a retained teardown, the scanning re-grant) need only this build's ordinary walks. | `walk.go`, `objstore.go`, `OUTSTANDING.md` 19 | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the refused mark ignored in the teardown's decision, and in the take-back; the re-grant's refusal off; the scanning door using the ordinary grant; the earlier mark not read; the rotation's mark moved back before its first step; the sweep's and the queue's teardown checks off; the empty provisioned name accepted; the revoke door's guard off. Storage simulation: 3,000 seeds on this code. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round14.md b/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round14.md| new file mode 100644 |
| index 000000000..3e48c5e90 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-carloku-paid-launch-overnight-round14.md |
| @@ -0,0 +1,123 @@ |
| +# Round-fourteen review: `d5705a63` |
| + |
| +Reviewed against base `aed42815`, concentrating on `d0a21aa1..d5705a63`. Read-only: no files changed, tests run, network commands, Stripe CLI or AWS CLI. Test counterfactuals are based on code inspection. |
| + |
| +All locations refer to `d5705a63`. **O** = `internal/objstore`, **C** = `internal/console`, **R** = `internal/release`, **D** = `implementation/carloku-paid-launch`, **S** = `docs/superpowers/specs`. |
| + |
| +## 1. Round-thirteen dispositions |
| + |
| +| Finding or claim | Disposition | Fix and regression | Would the test pass without the fix? | |
| +|---|---|---|---| |
| +| **Astra F1:** holder discovery overlooks a refused record’s second bucket | **Partly closed.** Both names enter the inventory, but ledger reconciliation can discard the refused holder: new F1 below. | Inventory: `O/walk.go:1383`. `TestTeardownDoesNotDestroyABucketARefusedRecordNames`, `O/ownership_test.go:3228`. | **No** without inventorying the grant bucket. However, the fixture expressly has no interval for that holder; it misses the reconciliation failure. | |
| +| **Astra F2:** a resumed bucketless grant creates an unrecorded bucket | **Closed.** | The count write records the resolved bucket, including its CAS retry: `O/walk.go:494`, `:504`. `TestResumedGrantRecordsTheBucketItIsAboutToCreate`, `O/ownership_test.go:3262`. | **No.** The recorded-bucket assertion fails, and subsequent teardown leaves the bucket. | |
| +| **Astra F3:** daily storage sizing treats an incoherent record as measured | **Closed.** | `O/objstore.go:221`; existing read-error hold at `C/economicspass.go:201`. `TestStoragePassDoesNotMeasureARefusedRecord`, `C/economicspass_test.go:1501`. | **No.** Removing the reader check permits sizing and fails the unswept/no-sizing assertions. | |
| +| **Astra F4:** a legitimately closed retained holder prevents another account’s restoration | **Closed for the reported coherent-record sequence.** | `O/walk.go:300`; `O/ownership.go:123`. `TestMembersTakeBackIsNotHeldByARecordWhoseStoreHasLeft`, `O/ownership_test.go:3301`. | **No.** Without closed-interval reconciliation, the member’s restoration is refused. Its application to refused records is new F1. | |
| +| **Astra F5:** console purge skips a removable teardown-only record | **Closed.** | `C/ownership.go:441`, `:445`, `:454`; declaration-only teardown at `O/walk.go:912`. Tests: `C/deletion_test.go:849`; `O/ownership_test.go:3345`. | **No** for the console regression without the routing fix. **Yes** for the direct teardown regression: that removal path already worked. | |
| +| **Astra F6:** declaration rewrites a refused record | **Closed.** | Reader and writer checks: `O/objstore.go:221`, `:260`; declaration caller: `C/apistore.go:295`, `:340`. `TestDeclareDoorDoesNotRewriteARefusedRecord`, `C/apiscan_test.go:674`. | **No** without both protections. **Yes** with only the reader check removed: the writer still refuses the teardown-only fixture; the split-record fixture also meets the existing prefix restriction. | |
| +| **Opus N1:** rotation’s failed final write leaves delivered credentials unmarked | **Closed for the reported final-write failure.** New failure cases remain in F3 and F4 below. | Mark before changes: `O/walk.go:702`; clear on success: `:757`, `:777`; rotation refusal: `:856`. `TestRotationIsRefusedAfterARotationThatFailedAtItsLastWrite`, `O/ownership_test.go:3172`. | **No.** Removing the mark or ignoring it fails the regression. `TestRotateResumesAfterResealFailure`, `O/walk_test.go:708`, also fails if the new clearing path is removed while retaining the mark, but models an unapplied write rather than an ambiguous in-flight write. | |
| +| **Opus N2:** deletion timestamp without a bucket name is trusted | **Closed.** | `O/walk.go:1578`, through `Get`. `TestADeletionNoteWithNoBucketNameIsRefused`, `O/ownership_test.go:3367`. | **No.** The explicit reader-refusal assertion fails without the new condition. | |
| +| **Opus N3:** scanning implicitly takes back a retained store | **Partly closed.** Already-torn-down records are refused; teardown between the scanning write and re-grant is not: new F2. | `C/apiscan.go:189`, `:360`, `:421`. Tests: `C/apiscan_test.go:612`, `:641`. | **No** for the static teardown fixture without the writer check. The tests pass with the interleaving in F2 still possible. | |
| +| **Opus N4:** grant and teardown omit coherence checks on their second read | **Closed in code.** | Both second reads use checked `Get`: `O/walk.go:217`, `:957`; check at `O/objstore.go:221`. Existing regressions: `O/ownership_test.go:2849`, `:2928`; explicit `Get` assertion at `:2899`. | **No** if the common reader check is removed. **Yes** if only the under-lease reads bypass it: these fixtures are already incoherent on the first read and do not test a change between reads. | |
| +| **Opus N5:** declaration and storage pass omit coherence checks | **Closed.** | Same fixes and tests as Astra F3 and F6 above. | Storage regression: **no**. Declaration regression has the reader/writer masking described above. | |
| +| **Opus N6:** scanner door checks lack direct tests | **Partly closed.** Grant is meaningfully covered; revoke’s no-call property is not. | `TestScanDoorsDoNotReachTheScannerForARefusedRecord`, `C/apiscan_test.go:641`; guards at `C/apiscan.go:360`, `:421`. | **No** without grant’s teardown guard. **Yes** without revoke’s teardown guard: the scanner starts disabled, `EnsureDisabled` leaves it disabled, and the writer still refuses. | |
| +| **Astra and Opus claim:** every path refuses an incoherent record before acting | **Partly closed.** Direct reader/writer paths are protected; decisions using `Inspect` still lose the conservative hold. | `O/objstore.go:216`, `:259`; inventory at `O/walk.go:1376`. Reader/writer assertions at `O/ownership_test.go:2899`, `:2902`; holder regression at `:3228`. | Reader/writer assertions fail without their respective checks. All pass with new F1 present. | |
| +| **Astra claim:** bucketless grant recovery can leave estate behind final record deletion | **Closed for the reported shape.** | Bucket persistence at `O/walk.go:494`, `:504`; regression at `O/ownership_test.go:3262`. | **No**, as described for Astra F2. This does not establish every possible final-deletion claim. | |
| +| **Opus claim:** rerunning rotation safely converges after its failed final write | **Closed for that failure; broader assurance remains incomplete.** | Revised comment at `O/walk.go:658`; mark/refusal regression at `O/ownership_test.go:3172`. | **No** without the behavioral fix. The regression does not cover new F3. | |
| +| **Opus claim:** the mark spares the delivered key and repair preserves it | **Closed as documentation.** | `O/objstore.go:158`; `O/ownership.go:88`; `D/AGENT_HANDOFF.md:35` now expressly says repair reaps and replaces the key. | No wording test. The repair regression at `O/ownership_test.go:3172` checks a live recorded replacement, not preservation of the previous delivered key. | |
| +| **Both reviews’ take-back claim:** another account’s access cannot be restored implicitly | **Partly closed.** | Member holder check at `O/walk.go:274`; scanning guards above. Regressions at `O/ownership_test.go:3301`, `C/apiscan_test.go:641`. | The tests detect their static reported failures but pass with new F1 and F2 present. | |
| +| **Earlier-build records with no delivery evidence** | **Accepted and left open**, specifically for records predating the mark. | `O/objstore.go:162`; dispositions at `S/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md:433`. | No recovery regression or new evidence. Earlier records that **do contain** the nested mark are a distinct, recoverable case: new F4. | |
| +| **Inherited claims:** count precedes mutation; refusal copy and backend-fence limitations | **Count ordering closed; copy and fence limitations accepted and left open.** | Count at `O/walk.go:497`, before `EnsureUser` at `:536`; test at `O/ownership_test.go:2725`. Open limitations: `D/OUTSTANDING.md:67`, `:72`. | Count-ordering test fails if mutation precedes the failed count. No wording test or new backend verification. | |
| + |
| +The revoke regression’s weakness is specific: `C/apiscan_test.go:662` asserts only that scanning ends disabled. `O/memscanner.go:147` silently deletes an already-absent enabled entry. A call counter or an initially enabled revoke fixture is needed to establish that the backend was never called. |
| + |
| +## 2. New findings |
| + |
| +### F1. P1 — Ledger reconciliation discards the hold supplied by a refused record |
| + |
| +**Where:** `O/walk.go:1395`, `:300`; `O/ownership.go:489`, `:504`. |
| + |
| +**Scenario:** |
| + |
| +1. B has a legacy or edited split record: `Provisioned.Bucket=X`, `Grant.Bucket=Y`, with live credentials and objects on Y. |
| +2. Y’s ledger contains a closed interval from an **earlier lifetime** of B’s store. The later ambiguous grant is not represented by an open interval. |
| +3. A has a coherent operator-adopted store on Y. |
| +4. A tears down. The repaired inventory finds B’s reference to Y, but returns an ordinary `OwnerRef` with no indication that the record was refused. |
| +5. `beginBucketTeardown` marks B as `known` because *any* interval exists, then discards its inventory entry. With no other open holder, A empties and deletes Y, including B’s objects. |
| + |
| +The member take-back path has the corresponding access failure: `ledger.left` discards the same refused holder because its historical intervals are closed. A retained, adopted store can therefore regain access despite B’s unresolved record. |
| + |
| +A historical closure does not establish that a later, incoherent record represents that same closed estate. The new inventory deliberately loses the information needed to distinguish this from Astra F4’s legitimate retained record. |
| + |
| +**Reachability:** This requires an older or edited inconsistent record and incomplete history for its later estate. Its presence in production is unknown. It extends the explicitly protected legacy-record case; it does not require a current writer to originate the split record. |
| + |
| +**Smallest acceptable fix:** Preserve whether a holder came from a refused record, and retain that hold through teardown and take-back reconciliation. Do not discard it merely because the ledger contains historical intervals. Extend `O/ownership_test.go:3228` with an earlier closed interval, and test both destruction and member take-back. |
| + |
| +### F2. P1 — A scanning request can restore cross-account access after a concurrent teardown |
| + |
| +**Where:** `C/apiscan.go:500`, `:537`, `:563`, `:580`; `O/walk.go:274`. |
| + |
| +**Scenario, using ordinary current-build walks:** |
| + |
| +1. A and B, in different accounts, hold an operator-adopted bucket X. |
| +2. An operator requests `store scan enforce off` for A. `writeScanRecord` succeeds while A has no teardown block. |
| +3. Pause that request before `regrantForScanning`. |
| +4. A’s teardown revokes A’s credentials, closes its interval, retains X for B, and returns `ErrBucketRetained`. That result releases the lease cleanly (`O/walk.go:1232`, `:1325`). |
| +5. B writes another object. |
| +6. Resume the scanning request. `regrantForScanning` reads A’s retained record but checks only whether it is provisioned. It carries the operator source into a new `Grant`. |
| +7. Grant accepts the retained estate as its starting state, skips the non-operator holder restriction, and restores A’s credentials and access to B’s new object. |
| + |
| +The operator requested a scanning change, not restoration of access. Grant’s estate comparison cannot catch this schedule: both of **its** reads occur after teardown. `storeWalkGates` is a configuration/rate gate, not serialization (`C/apistore.go:476`). |
| + |
| +**Smallest acceptable fix:** Give scanning policy convergence a no-take-back condition enforced on Grant’s under-lease record, before mutations. An extra pre-call read alone leaves another race. Add an interleaving regression that completes retained teardown after the scanning record write but before re-grant, then asserts no credentials are minted or delivered. |
| + |
| +### F3. P2 — An unchanged config read cannot prove an errored delivery will never arrive |
| + |
| +**Where:** `O/walk.go:744`–`:753`; `internal/appconfig/store.go:187`; `R/store.go:636`, `:651`. |
| + |
| +**Scenario:** |
| + |
| +1. The record and delivered config name K1. |
| +2. Rotation marks the record, mints K2, and sends the conditional config write. |
| +3. The client receives a transport error while the remote write remains in flight. The request context remains usable. |
| +4. The comparison read still sees the old config generation. Rotation clears `Delivering` and returns the error. |
| +5. The delayed write then commits K2 against the still-current old config generation. The app can receive K2 on its next config refresh. |
| +6. Another rotation reads the unmarked record naming K1 and reaps K2 at `O/walk.go:727`. If minting or delivery then fails, the app has a dead credential. |
| + |
| +This needs neither an ignored CAS condition nor a request accepted after fence expiry: the delayed write can commit within the original window. Same-app, same-kind retries can also start before that window ends (`O/walklock.go:155`). |
| + |
| +`writeEnv` propagates an undifferentiated config-write error. A subsequent read establishes what exists **at that read**, not whether an earlier request can still commit. The walk’s own failure model acknowledges that distinction (`O/walk.go:1319`). |
| + |
| +**Smallest acceptable fix:** Clear the mark only when failure proves delivery was not attempted or cannot still take effect. Preserve it for ambiguous write errors unless a remote fence conclusively excludes a later commit. Add a store double whose config `Put` returns an error, remains pending through the comparison, and commits afterward. The existing reseal-failure test at `O/walk_test.go:708` does not model this. |
| + |
| +### F4. P2 — Moving the delivery mark silently drops positive evidence from the previous wire shape |
| + |
| +**Where:** `O/objstore.go:52`, `:164`, `:247`; consumers at `O/walk.go:480`, `:856`. |
| + |
| +**Scenario:** |
| + |
| +1. The previous reviewed build leaves a failed re-grant recorded as `"grant":{"delivering":true,...}`. The app received K2, while `Provisioned.KeyID` still names K1. |
| +2. Upgrade to this commit. |
| +3. `GrantBlock` no longer contains `Delivering`. Ordinary JSON decoding ignores the nested field, and the new top-level `Record.Delivering` remains false. |
| +4. Rotation is admitted and reaps the delivered K2. A subsequent mint/delivery failure leaves the app without working credentials. A re-grant can also take the healthy shortcut because the delivery variables exist and the mark decoded false. |
| + |
| +The `d0a21aa1..d5705a63` diff explicitly removes the nested field and introduces the top-level field without migration. This differs from the accepted limitation for builds **before any mark existed**: here the record contains positive evidence, and this reader discards it. |
| + |
| +**Reachability:** Persisted records from the preceding branch builds suffice. Whether a deployment ran those builds cannot be determined from the repository. |
| + |
| +**Smallest acceptable fix:** Decode the legacy nested mark and OR it into `Record.Delivering`; write the canonical representation afterward. Add a raw-JSON compatibility regression proving that both rotation verbs refuse an old marked record and that successful repair clears the migrated mark. |
| + |
| +## 3. Claims the code contradicts |
| + |
| +| Location | Claim | Contradiction | |
| +|---|---|---| |
| +| `D/AGENT_HANDOFF.md:35`; dispositions `S/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md:423`, `:432`; `D/OUTSTANDING.md:67` | Refused records hold every named bucket, and no operation acts past their ambiguity. | Inventory records the names, but reconciliation discards the refused holder when historical ledger entries exist. Another app can destroy its bucket or regain access: F1. | |
| +| `D/AGENT_HANDOFF.md:35`; dispositions `:427` | A scanning door never changes or takes back a store whose teardown has begun. | The guard precedes a separate re-grant. Teardown between them allows scanning to restore access: F2. | |
| +| `O/walk.go:738`; dispositions `:425`; `D/AGENT_HANDOFF.md:35` | An unchanged config comparison establishes that the new key was not delivered, permitting the mark to be removed. | An errored remote write can commit after the comparison. Equality is an observation, not cancellation or fencing: F3. | |
| +| `D/AGENT_HANDOFF.md:35`; dispositions `:433` | Earlier-build records carry no mark, so their unfinished delivery cannot be recovered from the record. | The preceding build stored the mark inside `grant`. That evidence remains recoverable until this decoder discards it: F4. The limitation remains valid for genuinely pre-mark records. | |
| +| `D/OUTSTANDING.md:66` | Since round eleven, neighboring findings concern records no walk of this build writes. | Current-build failed rotations and retained-teardown sequences are ordinary generated states, demonstrated by `O/ownership_test.go:3172`, `:3301`. F2 and F3 likewise require no malformed legacy record. | |
| + |
| +The documented mutation runs were not re-executed. Their listed mutations do not establish protection against these interleavings or wire-compatibility failures. |
| + |
| +## 4. Verdict |
| + |
| +**Reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round15.md b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round15.md| new file mode 100644 |
| index 000000000..92635cfb1 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round15.md |
| @@ -0,0 +1,28 @@ |
| +# Review brief, round 15: Carloku paid launch, overnight changes of 2026-10-01 |
| + |
| +Read the round 1 brief (`CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight.md`) for the fixed rulings and the output rules, the round 3 to 14 briefs for rulings 6 to 12, then `CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md` (its last section covers round 14) and `implementation/carloku-paid-launch/OUTSTANDING.md`. |
| + |
| +Review the commit named in the prompt. Read source only with `git show <commit>:<path>`. The changes since round 14 are `git diff d5705a63 <commit>`. The code changes are in `internal/objstore` (`objstore.go`, `walk.go`, `ownership.go`, `sweep.go`, `queue.go`, tests) and `internal/console/apiscan.go` and its test. The rest is documents. |
| + |
| +Still the fixed snapshot: round 14's findings, nothing else. Nothing may attest. The quantity worker has two switches, both unset. Billing is off. |
| + |
| +Do not report again, as findings, what `OUTSTANDING.md` lists as open: items 6 and 6a, 7 and 9, 19a to 19h. Do report anything that makes one of them worse than the document says, and any claim about them the code contradicts. |
| + |
| +## First: a disposition table |
| + |
| +For every round 14 finding (Astra F1 to F4 and the claims; Opus N1 to N4 and the claims): closed, partly closed, not closed, or accepted and left open as the dispositions say. File and line of the fix and of the test, and whether the test would pass without the fix. |
| + |
| +## Then: new findings only |
| + |
| +- **Refused holders** (`OwnerRef.refused`, `storeHolders`, `beginBucketTeardown`, the take-back check in `Grant`). The claim: a record no walk can follow holds every bucket it names, and no reconciliation against the ledger removes that hold. Find a bucket destroyed, or access given, past such a record. Find a holder list that loses the mark on its way to the decision. |
| +- **`Regrant`** (`grant` with `noTakeBack`; `console/apiscan.go` `regrantForScanning`). The claim: a scanning door cannot take a store back, whatever runs between its write and its re-grant. Find a schedule where it does: a teardown that begins after the lease is taken, a record whose teardown block was cleared, another caller that should use `Regrant` and uses `Grant`. |
| +- **The mark** (`Record.Delivering`). Rotate now writes it just before delivery and never clears it on a failed delivery. Find a key change the app could have received with the mark not on the record. Find a store that can be neither rotated nor repaired. Say whether the earlier place of the mark (`GrantBlock.LegacyDelivering`) can be written back out by this build. |
| +- **The sweep and the queue** (`RunSweep`, `queuePermitted`) for a store whose teardown has begun. Find another reader of a retained store's bucket that publishes what it read to the store's member. |
| +- **Claims** in `AGENT_HANDOFF.md` item 5, `OUTSTANDING.md` 19, 19g and 19h, and the round 14 dispositions. |
| + |
| +## What to return |
| + |
| +1. The disposition table. |
| +2. New findings, most severe first, in the round 1 format. |
| +3. Claims the code contradicts. |
| +4. A one-line verdict: accept, accept with fixes, or reject. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round14.md b/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round14.md| new file mode 100644 |
| index 000000000..6bc247e42 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-carloku-paid-launch-overnight-round14.md |
| @@ -0,0 +1,131 @@ |
| +# Opus production and authority review, round 14: Carloku paid launch, `aed42815..d5705a63` |
| + |
| +I read the code with `git show` only. Nothing was changed, no tests were run and no network calls were made. Each "fails without the fix?" answer comes from reading the test against the code. Since round 13 (`d0a21aa1..d5705a63`), the code changes are in `internal/objstore` (`objstore.go`, `walk.go`, `ownership.go`, `sweep.go`, tests) and `internal/console` (`apiscan.go`, `ownership.go`, tests). |
| + |
| +Abbreviations: **O** = `internal/objstore`, **C** = `internal/console`, **D** = `implementation/carloku-paid-launch`. |
| + |
| +**Production authority** |
| + |
| +- **Nothing attests.** `SetAttestation` and `SetAllClientsInstrumented` (`usage/durabilityledger.go:1455`, `:1475`) still have no non-test caller. |
| +- **The quantity worker cannot reach Stripe with either switch unset.** `quantityWorker()` (`C/billingquantity.go:131-137`) needs a proration behaviour and also needs `QuantityFence == "stripe-sandbox-verified"`. `cmd/console/billing.go:40` rejects any other fence value. |
| +- **None of these files changed this round:** `billingquantity.go`, `billingservice.go`, `internal/billingstripe`, `cmd/console`, `internal/usage`, `internal/sigfence`, `walklock.go`, `fence.go`, `aws.go`, `internal/release`. |
| + - My round 9 to 13 reading of the send lease, the key fence, the walk lease and the signing fence stands. |
| + - AWS is still the only backend where the fence has been observed (19d). |
| + - The sandbox test is unchanged and has not been run. |
| +- **Billing and data.** I found no path that bills wrongly or destroys a customer's data. |
| +- **Leaks.** One finding: |
| + - N2 below leaks object key names from another account's bucket through the scan status door. |
| + - It is older than this round, and it is in the area this round hardened. |
| + |
| +## 1. Disposition table (round 13) |
| + |
| +| Finding | Status | Fix | Test; fails without the fix? | |
| +|---|---|---|---| |
| +| **Astra F1**: holder inventory misses the grant bucket of a split record | **Closed.** | `storeHolders` reads with `Inspect`. A refused record counts for every bucket it names (`O/walk.go:1376-1398`). `beginBucketTeardown` then counts it as foreign when it has no interval. | `TestTeardownDoesNotDestroyABucketARefusedRecordNames` (`O/ownership_test.go:3228`): **fails**. Without the refused-record branch, Y is destroyed. With `Get` put back, the test gets `ErrTwoBuckets` instead of `ErrBucketRetained`. | |
| +| **Astra F2**: a resumed bucketless grant creates a bucket that teardown cannot find | **Closed for grants run by this build.** A bucketless grant left by an earlier build that had already created its bucket is still lost to teardown. That is not made worse, and nothing can tell it apart. | Grant block filled in during the count write (`O/walk.go:494-496`, `:504-506`) | `TestResumedGrantRecordsTheBucketItIsAboutToCreate` (`:3262`): **fails** at the `cur.Grant.Bucket` assertion. | |
| +| **Astra F3, F6; Opus N4, N5**: storage pass, declare door, and the under-lease reads skip the check | **Closed.** The check now sits in `Get` (`O/objstore.go:215-224`) and `Put` (`:259-262`). Every walk's under-lease read (`O/walk.go:217`, `:957`) and every caller now goes through it. I checked every non-test caller of `objstore.Get`: each one holds or fails on an error, and none treats it as "no store". | `objstore.go` | `TestStoragePassDoesNotMeasureARefusedRecord` (`C/economicspass_test.go:1501`): **fails**. `TestDeclareDoorDoesNotRewriteARefusedRecord` (`C/apiscan_test.go:674`): **fails** only with both checks removed. With only the `Get` check removed, `Put` still refuses and the test passes. | |
| +| **Astra F4**: a retained record blocks another account's take-back | **Closed.** | `BucketOwnership.left` (`O/ownership.go:123-135`), used at `O/walk.go:300`. Intervals close only in Teardown after `DeleteUser` (`:1189`), so a closed interval does mean the credentials were revoked. | `TestMembersTakeBackIsNotHeldByARecordWhoseStoreHasLeft` (`:3301`): **fails** with `ErrBucketHeld`. | |
| +| **Astra F5**: the purge cannot pass a teardown-only record | **Closed.** | `C/ownership.go:441-470` | `TestPurgeHandlesATeardownOnlyStoreRecord` (`C/deletion_test.go:849`): **fails**. `TestTeardownRemovesATeardownOnlyRecordThatNamesNothing` would pass without the change. It guards the new `Coherent` case against over-refusing. | |
| +| **Opus N1**: Rotate's own failed last write is unmarked | **Closed for the reported shape.** It also brings a regression for rotations that stop before delivery (new N1). | `Record.Delivering`; Rotate marks at `O/walk.go:701-715` and clears at `:755`, `:777` | `TestRotationIsRefusedAfterARotationThatFailedAtItsLastWrite` (`:3172`): **fails** either way, with the mark not written (the fixture's write count) or not read. | |
| +| **Opus N2**: blank-name deletion note | **Closed.** | `Coherent` (`O/walk.go:1578-1581`) | `TestADeletionNoteWithNoBucketNameIsRefused` (`:3367`): **fails**. | |
| +| **Opus N3**: an operator's scanning door takes a store back | **Closed for the doors.** The scan pass still sweeps such a store (new N2). | `C/apiscan.go:189`, `:360`, `:421` | `TestScanRecordWriterRefusesARecordNoWalkCanFollow`, `TestScanDoorsDoNotReachTheScannerForARefusedRecord`: **fail** for the writer and the grant door. The revoke door's teardown guard is not tested (N4). | |
| +| **Opus N6**: the scan doors' own checks are untested | **Partly closed.** The grant door is tested. The revoke door's guard is not (N4). | `C/apiscan_test.go:641` | See N4. | |
| +| **Claims**: "the mark stops reaping the key the app was given"; "repairs in full" | **Mostly closed.** `objstore.go:154-164`, `ownership.go:88-94` and handoff item 5 are corrected. The comment in `leaseForRotation` (`O/walk.go:857-859`) still says "Rotating now would reap the key the app was given … it repairs in full". | — | — | |
| +| **Claims**: "every path checks first" | **Closed.** It is true now that the check is in the reader and the writer. The one exception is `Inspect`, which is used only by `storeHolders`. | — | — | |
| +| **Earlier-build records carry no mark** | **Accepted and left open, as the dispositions say.** It is stated in the field's comment. | — | — | |
| + |
| +## 2. New findings, most severe first |
| + |
| +### N1. P2: a rotation that stops before delivery now needs a repair that deletes the app's live key with no overlap |
| + |
| +**Where:** the mark is written at `O/walk.go:701-715`. Steps 1 to 3 run at `:718-736` and return with the mark set. Only a failure at step 4 can take the mark off (`:744-753`). `leaseForRotation` refuses at `:856`, and the Grant reap is at `:542-547`. |
| + |
| +**Scenario:** |
| +1. A healthy store's app runs on K1. The record names K1. |
| +2. A rotation writes `Delivering=true`. Then `ReapKeys` or `MintKey` fails on a transient IAM error, or the walk's lease context runs out. No key the app holds has been touched, and nothing was delivered. |
| +3. The member runs the rotation again. It is refused with `ErrGrantUnfinished`, and the door's copy for that is wrong (19g). |
| +4. The only repair is a grant. Because of the mark it walks in full and runs `ReapKeys(keep=[PrevKeyID])`, which deletes K1. Every host and hibernating tenant still holds K1 until it adopts the new config, which is the outage rotation's overlap exists to prevent (`O/walk.go:633-639`). |
| + |
| +**Before this round:** the same failure left no mark. The re-run rotation kept K1 (`ReapKeys` keeps `pr.KeyID`) and converged with the overlap intact. This round turns any provider failure before delivery into a forced swap with no overlap. |
| + |
| +**Smallest fix:** write the mark just before step 4, after the mint. A failure before that point leaves K1 named and held, and the strays are reaped by the next rotation. Alternatively, clear the mark on every error returned before `writeEnv` is called. Add a test: fail `MintKey` once, then check that a second rotation succeeds and that K1 survived it. |
| + |
| +### N2. P2: the scan pass keeps sweeping a bucket a retained teardown left to another account, and the member can read the result |
| + |
| +**Where:** |
| +- `RunSweep` (`O/sweep.go:128-145`) and `queuePermitted` (`O/queue.go:481-500`) check the tombstone but not `rec.Teardown`. |
| +- `scanReply` returns the sidecar as `coverage` (`C/apiscan.go:84-95`). |
| +- The sidecar's `after` cursor is an object key (`O/sidecar.go:62-65`). |
| + |
| +**Scenario:** |
| +1. An operator adopted bucket X for A (account 1) and C (account 2). A has scanning enabled. |
| +2. A's member destroys the store from Settings. The app stays live. Teardown revokes A's user, closes A's interval and returns `ErrBucketRetained`. The record keeps `Provisioned{X}`, `Scanning.Enabled` and a teardown block. |
| +3. The hourly `scanPass` still finds A due. `RunSweep` lists and scans X with the deployment's lister and writes A's sidecar: object counts, infected counts, and `after` set to one of C's object keys. |
| +4. A's member runs `carlos store scan status` and reads object names and counts that C wrote after A's access ended. The retained hold exists to protect exactly that data (`O/walk.go:265-271`). |
| + |
| +This round made the scan doors refuse a store under teardown, but not the pass. The bug predates this round and is not in `OUTSTANDING.md`. |
| + |
| +**Smallest fix:** `RunSweep` and `queuePermitted` return nothing for `rec.Teardown != nil`, as they already do for a tombstone. Add a test: a retained teardown with scanning enabled, then one `scanPass`, then check that the sidecar did not change and the lister was not called. |
| + |
| +### N3. P3: `Coherent` and Teardown disagree about which bucket a record with an empty provisioned name belongs to |
| + |
| +**Where:** `Coherent` reads `own` as `Provisioned.Bucket` when it is non-empty, else `Grant.Bucket` (`O/walk.go:1570-1573`). Its two-bucket test skips an empty name (`:1564`). Teardown uses `Provisioned.Bucket` whenever `Provisioned != nil` (`:931-934`, `:1009-1013`). |
| + |
| +**Scenario:** |
| +1. The record is `Provisioned{Bucket:"", KeyID:K}` with `Grant{Bucket:Y}`. `Get` accepts it. |
| +2. Teardown leases on the app, takes `bucket=""` and skips every bucket step and the ledger close. |
| +3. It deletes the user and config, then deletes the record. Y, its objects and its open interval are left with nothing naming them. |
| + |
| +**Reachability:** this build never writes an empty provisioned name. Only a hand edit or an unknown earlier writer could produce one. |
| + |
| +**Smallest fix:** in `Coherent`, refuse `Provisioned != nil && Provisioned.Bucket == "" && Grant != nil && Grant.Bucket != ""`. Alternatively, make Teardown choose its bucket with `Coherent`'s rule. |
| + |
| +### N4. P3: the revoke door's teardown guard is untested |
| + |
| +**Where:** `C/apiscan.go:420-421`; test at `C/apiscan_test.go:641`. |
| + |
| +**Why it matters:** remove `rec.Teardown == nil` and an operator's `scan revoke` on a retained record calls `EnsureDisabled(X)`. That switches scanning off on a bucket another account still holds and is owed scanning on. The test only checks `IsEnabled`, so it does not notice. |
| + |
| +**Smallest fix:** in the "teardown that has begun" case, assert that the fake scanner received no `EnsureDisabled`. Add this guard to the mutation list. |
| + |
| +### Asked and not found |
| + |
| +- **A reader that acts without `Get`:** |
| + - `release/trash.go` parses `store.json` directly, but it refuses any record with a grant, provisioned or teardown block, which is stricter than `Coherent`. |
| + - `mail.Get` reads a different object. |
| + - The only non-test caller of `Inspect` is `storeHolders`, and it only counts holders. |
| +- **A record this build writes that `Put` refuses:** none. |
| + - Teardown's note names `Provisioned.Bucket`, or else `Grant.Bucket`. That matches `own` except in N3's shape, where Teardown writes no note at all. |
| + - Grant's final record names one bucket. |
| + - The take-back write clears the note. |
| +- **A key change with no mark:** |
| + - In Grant, every reap and mint comes after the count write. |
| + - In Rotate, the mark is written before step 1. |
| + - RotateFinish deletes the pending key with no mark. That is harmless: the record still names the live key correctly, and the deleted key is the one the member declared unused. |
| + - Teardown's `DeleteUser` is covered by the teardown block. |
| +- **A successful walk that leaves the mark set:** none. |
| + - Grant's healthy path only runs on a record with no mark. |
| + - The count retry could carry a mark only from a concurrent rotation, and a rotation needs the lease the grant holds. |
| +- **The config comparison:** `writeEnv` writes one object, `config.json` (`appconfig/store.go:121`, `:187`). Its etag is a hash of the content (`release/store.go:280`) or S3's. |
| + - The sealed halves are random, so a delivery that landed always changes the generation. |
| + - A lost response, a concurrent write or a failed read all keep the mark. |
| + - Only a byte-identical restore of the earlier config between the two reads could fool it, and nothing in the code does that. |
| +- **Holders and take-backs:** |
| + - A refused record that names only a bucket it noted as deleted still holds that bucket. That is conservative, and acceptable under the ambiguity rule. |
| + - A refused record with no interval blocks another account's teardown for good, which is correct until an operator repairs it. |
| + - `left` and `beginBucketTeardown`'s `known` agree. |
| + |
| +## 3. Claims the code contradicts |
| + |
| +| Location | Claim | What the code supports | |
| +|---|---|---| |
| +| `O/walk.go:658-663` (Rotate doc) | "A rotation that stopped anywhere in between leaves the mark, the next rotation is refused instead of reaping the key the app holds" | For a stop at steps 1 to 3, the next rotation would have kept the held key. The refusal forces a grant that reaps it (N1). | |
| +| `O/walk.go:857-859` (`leaseForRotation`) | "Rotating now would reap the key the app was given … it repairs in full" | This is the wording the dispositions say was corrected. The grant reaps the held key. The comment also still names only a grant as the source of the mark. | |
| +| `O/objstore.go:226-231` (`Inspect` doc) | `Inspect` is also for "showing the record to the person repairing it" | No such caller exists. The Settings card reads through `Get` and shows the store as unreadable (`C/appsettings.go:382`). | |
| +| `D/AGENT_HANDOFF.md` item 5 | "a scanning door never changes, and so never takes back, a store whose teardown has begun" | True for the doors. The scan pass still sweeps such a store and publishes coverage to its member (N2). | |
| + |
| +Nothing in `walklock.go`, `fence.go`, `aws.go`, the evidence document, the quantity worker's fence, or `OUTSTANDING.md` items 6, 6a, 7, 9 or 19a to 19g is contradicted. This round makes none of them worse. 19g's "nothing acts on it and nothing writes it" now holds. |
| + |
| +## 4. Verdict |
| + |
| +**Accept with fixes.** Nothing attests, the worker cannot reach Stripe with either switch unset, and nothing bills wrongly or destroys data. Every round 13 finding is closed for its reported shape. N1 (the rotation mark written too early) and N2 (the scan pass over a retained bucket) should land before the store walks are relied on. |
diff --git a/implementation/carloku-paid-launch/AGENT_HANDOFF.md b/implementation/carloku-paid-launch/AGENT_HANDOFF.md| index 12cd7269f..d0f24638b 100644 |
| --- a/implementation/carloku-paid-launch/AGENT_HANDOFF.md |
| +++ b/implementation/carloku-paid-launch/AGENT_HANDOFF.md |
| @@ -32,7 +32,7 @@ When a review is approved, apply each string byte for byte: the site by replacin |
| 2. Console GET `/a/{sqid}/billing` (`billingpage.go`, `templates/billing.html`): current plan, owner-only chooser posting to the existing checkout door, `?country=` price preview computed from the plans terms, error and pending banners, portal button once a customer exists, read-only for members, byte-identical 404 for non-members, closed state when billing is off. `plans.Countries()` exposes the embedded country list. |
| 3. Durability ledger: the epoch read race is fixed (epoch and created-at are immutable after recover) with a `-race` regression. An unmetered replication fallback on a box whose ledger opened now files an escape (`DurabilityLedger.Escaped`): the box-day is incomplete and the owner is excluded for the run. A box whose ledger failed to open has no publisher and files nothing. The console fold asks each box's published day to prove the union of that box's durability, metrics and edge windows; a box with no ledger directory, no day file, or a ledger whose coverage starts after the box began serving holds the day for everyone. There is no tolerance. Durability days are counted per account: a box-day attested with an exclusion set is complete for every account the box did not exclude. The move worker's checkpoint is recorded against the route's owner. Nothing attests: attestation is false everywhere, in this code and in production. The scoped attestation (path A) was built, reviewed in round six, found unable to prove an exclusive writer, and removed from this branch. It is kept on `feat/carloku-scoped-attestation-20261001` with its findings open. Replication-write completeness is a launch blocker, not a waived requirement. |
| 4. `internal/awsbilling`: Cost and Usage Report reader (versioned deliveries only), newest-delivery selection, digests, charged S3 transfer classifier. It holds on a charged usage line it does not count and on any non-usage line (fee, refund, credit, discount) with a cost on a transfer usage type. Every transfer usage type the flagship account wrote from July to September 2026 is pinned in a test. |
| -5. `internal/objstore/ownership.go`: append-only bucket ownership ledger, opened at grant, closed at teardown. It resolves an owner only for a ledger that claims completeness, and only for hours that begin at or after the instant it began vouching, so a store's partial first hour never resolves (a ledger that began vouching exactly on the hour answers for that hour). Completeness needs a grant block recorded as minting, nothing delivered to the app yet, a probe showing the bucket does not exist, and an inventory showing no principal's policy names the bucket (`GranteeProber`), and is dated when both probes had answered. The AWS provisioner cannot list principals yet, so on AWS no ledger claims completeness, new stores included, and store transfer holds. A minting grant is refused while another app holds the name by ledger interval or by store record, and for a bucket that already exists and it never held. Grant, teardown and both rotation verbs are walks. A walk that spends backend authority takes a lease (`walklock.go`) on the bucket name, or on a hash of it when the name is one this build would refuse, or on a hash of the app when the record names no bucket: an object taken by compare-and-swap, bound to the invocation, thirty minutes long, standing until it runs out if the walk failed or any of its requests needed a second attempt. The record is read again under the lease and must still be the same estate, not only the same name: teardown and grant compare the grant instant, the key, how many grants have reached the provider, whether a teardown had begun, whether it noted the bucket deleted and how many take-backs it has seen; rotation compares the same estate and requires that no teardown has begun. Every grant is counted on the record before any call that changes anything at the provider, the user and its policy included (two read-only probes, of the bucket and of who can reach it, come first), so a healthy store granted again writes its record once. A grant or a rotation that is about to change the user's keys marks the record first (`Record.Delivering`) and takes the mark off when it records the result; a rotation whose delivery left the app's config exactly as it was takes it off too. While the mark stands a grant run again walks in full and a rotation is refused (`ErrGrantUnfinished`). The mark does not save the key the app was holding: the repairing grant reaps and replaces it. A record from an earlier build carries no mark, which means not recorded, not finished. An older grant block that names no bucket is made to name it in the write that counts the walk. A record's bucket is the one it is provisioned on, else the one its grant block names: the arguments choose a bucket only for a store that has none, so a grant never moves a store to another name. A record whose grant block and provisioned block name different buckets (an earlier build could write one), or whose teardown note is about a bucket that is not its own, is refused by the record's one reader and one writer (`objstore.Get` and `objstore.Put`; the check is `objstore.Coherent`; `ErrTwoBuckets`, `ErrUnfinishedTeardown`), so no door, pass or walk can act on one or write one. The holder inventory reads such a record anyway (`objstore.Inspect`) and counts it as a holder of every bucket it names. An operator repairs such a record by hand; there is no verb for it. A take-back over a bucket that a store of another account holds is refused unless an operator grants it; a store whose ledger interval is closed does not count as holding it; a scanning door never changes, and so never takes back, a store whose teardown has begun. A failed walk's lease stands until it runs out, with one exception: the same app re-running the same kind of walk may take it at once. A declaration-only teardown and the app purge take no lease; they delete the store record only as the generation they read. The user belongs to the app and the lease to the name, so a late request of a failed teardown can still remove the policy of a store granted afterwards under another name (round eight, Opus N2, open). Every request of a walk, to S3, to IAM and to the deployment bucket that holds the records and ledgers, is signed with the instant its lease was taken (`internal/sigfence`), so AWS refuses a request a paused process sends after its lease could be taken over. The record and its scan sidecar are deleted as the generation that was read. The signing-time rule was checked read-only against S3 and IAM with a list and two gets (`TestFenceAgainstAWS`); no delete was sent. A declaration-only teardown takes no lease (there is no bucket name) and relies on the conditional delete alone. A teardown writes a block on its own record before it touches anything and decides by compare-and-swap on the ledger (for a bucket name this build would refuse there is no ledger, and it decides from the store records alone): if nobody else holds the bucket it marks the bucket as being destroyed; otherwise it leaves the bucket and its scanner, revokes its own credentials, and records that the scanner is owed. A grant by the same app over its own unfinished teardown takes the store back, runs in full, and voids the teardown's note that the bucket was deleted. A teardown that leaves a bucket to another account keeps its record and returns `ErrBucketRetained` (discussion 191). It can finish once that bucket has been destroyed, when someone runs it again; nothing runs it again by itself. For a bucket name this build would refuse there is no ledger: a store whose teardown has begun is not counted as a holder there, so the last of two sharers destroys the bucket. `ErrBucketRetained` has no member copy yet; it needs Paul's review. A randomized simulation (`walk_sim_test.go`) checks the invariants over sequential walks with provider and store faults, including failures after the effect, and walks frozen past their lease while another takes the name. It does not generate a second bucket name, rotations, stores with no ledger interval or disagreeing clocks; those have direct tests. |
| +5. `internal/objstore/ownership.go`: append-only bucket ownership ledger, opened at grant, closed at teardown. It resolves an owner only for a ledger that claims completeness, and only for hours that begin at or after the instant it began vouching, so a store's partial first hour never resolves (a ledger that began vouching exactly on the hour answers for that hour). Completeness needs a grant block recorded as minting, nothing delivered to the app yet, a probe showing the bucket does not exist, and an inventory showing no principal's policy names the bucket (`GranteeProber`), and is dated when both probes had answered. The AWS provisioner cannot list principals yet, so on AWS no ledger claims completeness, new stores included, and store transfer holds. A minting grant is refused while another app holds the name by ledger interval or by store record, and for a bucket that already exists and it never held. Grant, teardown and both rotation verbs are walks. A walk that spends backend authority takes a lease (`walklock.go`) on the bucket name, or on a hash of it when the name is one this build would refuse, or on a hash of the app when the record names no bucket: an object taken by compare-and-swap, bound to the invocation, thirty minutes long, standing until it runs out if the walk failed or any of its requests needed a second attempt. The record is read again under the lease and must still be the same estate, not only the same name: teardown and grant compare the grant instant, the key, how many grants have reached the provider, whether a teardown had begun, whether it noted the bucket deleted and how many take-backs it has seen; rotation compares the same estate and requires that no teardown has begun. Every grant is counted on the record before any call that changes anything at the provider, the user and its policy included (two read-only probes, of the bucket and of who can reach it, come first), so a healthy store granted again writes its record once. A grant marks the record before it changes the user's keys, and a rotation just before it delivers the new key (`Record.Delivering`); each takes the mark off when it records the result. A rotation that fails before delivery leaves no mark and is resumed by running it again, with the app's key kept. One whose delivery fails keeps the mark, because a failed write may still land. While the mark stands a grant run again walks in full and a rotation is refused (`ErrGrantUnfinished`). The mark does not save the key the app was holding: the repairing grant reaps and replaces it. A record from an earlier build carries no mark, which means not recorded, not finished. An older grant block that names no bucket is made to name it in the write that counts the walk. A record's bucket is the one it is provisioned on, else the one its grant block names: the arguments choose a bucket only for a store that has none, so a grant never moves a store to another name. A record whose grant block and provisioned block name different buckets (an earlier build could write one), or whose teardown note is about a bucket that is not its own, is refused by the record's one reader and one writer (`objstore.Get` and `objstore.Put`; the check is `objstore.Coherent`; `ErrTwoBuckets`, `ErrUnfinishedTeardown`), so no door, pass or walk can act on one or write one. The holder inventory reads such a record anyway (`objstore.Inspect`) and counts it as a holder of every bucket it names, whatever the ledger shows for that app. An operator repairs such a record by hand; there is no verb for it. A take-back over a bucket that a store of another account holds is refused unless an operator grants it; a store whose ledger interval is closed does not count as holding it; a scanning door never changes a store whose teardown has begun, its re-grant (`objstore.Regrant`) refuses one under the walk's own lease, and the scan pass does not sweep or queue one. A failed walk's lease stands until it runs out, with one exception: the same app re-running the same kind of walk may take it at once. A declaration-only teardown and the app purge take no lease; they delete the store record only as the generation they read. The user belongs to the app and the lease to the name, so a late request of a failed teardown can still remove the policy of a store granted afterwards under another name (round eight, Opus N2, open). Every request of a walk, to S3, to IAM and to the deployment bucket that holds the records and ledgers, is signed with the instant its lease was taken (`internal/sigfence`), so AWS refuses a request a paused process sends after its lease could be taken over. The record and its scan sidecar are deleted as the generation that was read. The signing-time rule was checked read-only against S3 and IAM with a list and two gets (`TestFenceAgainstAWS`); no delete was sent. A declaration-only teardown takes no lease (there is no bucket name) and relies on the conditional delete alone. A teardown writes a block on its own record before it touches anything and decides by compare-and-swap on the ledger (for a bucket name this build would refuse there is no ledger, and it decides from the store records alone): if nobody else holds the bucket it marks the bucket as being destroyed; otherwise it leaves the bucket and its scanner, revokes its own credentials, and records that the scanner is owed. A grant by the same app over its own unfinished teardown takes the store back, runs in full, and voids the teardown's note that the bucket was deleted. A teardown that leaves a bucket to another account keeps its record and returns `ErrBucketRetained` (discussion 191). It can finish once that bucket has been destroyed, when someone runs it again; nothing runs it again by itself. For a bucket name this build would refuse there is no ledger: a store whose teardown has begun is not counted as a holder there, so the last of two sharers destroys the bucket. `ErrBucketRetained` has no member copy yet; it needs Paul's review. A randomized simulation (`walk_sim_test.go`) checks the invariants over sequential walks with provider and store faults, including failures after the effect, and walks frozen past their lease while another takes the name. It does not generate a second bucket name, rotations, stores with no ledger interval or disagreeing clocks; those have direct tests. |
| 6. Statement folds: `console/durabilityfold.go` (`durability_writes_1k`, policy in `usage.BillableReplicationWrites`) and `console/storeegressfold.go` (`store_egress_gb`). `usage.MeterSource` names a source for both meters. Month evidence pins ownership ledgers by their month projection and knows the durability box directory. Statements carry the five billed meters by UTC day (`Statement.Days`), summing to the monthly totals; the other meters are not recorded by day. With a transfer report configured, the statements catch-up walks a 36-month window and sweeps every month that has an open statement, is missing a statement an account is owed (the platform's included), or is a gap between metered months. An account's statement that will not parse, or that cannot be read three passes running, is reported once per process and the month is classified without it. The platform's own statement is different: while it cannot be read the month stays unknown, for as long as that lasts. A month seen with no statement is remembered for six hours, a closed statement for the life of the process. |
| 7. Operator API doors (`billingoperator.go`): list and resolve billing review holds, list and set the house marker. No CLI verb yet. |
| 8. Team quantity worker (`billingquantity.go`) behind two switches, both unset: `CONSOLE_BILLING_PRORATION` (`immediate` or `next_invoice`, Paul's decision) and `CONSOLE_BILLING_QUANTITY_FENCE=stripe-sandbox-verified` (set only after `TestSandboxQuantityFence` has passed against a Stripe test-mode account; it has not been run). With either unset it makes no provider call. An operation is recorded by a compare-and-set on the billing row's revision, subscription, period, quantity and status, on the target and newest pending addition recomputed from the database, and on its own instant lying inside the period and not before the addition it charges for. The request dates its proration at that instant (`proration_date`). Sending and retiring exclude each other through a send lease on the operation's row, owned by one sender's token; an operation dated ahead of the observing clock is left alone. Every key ends in the account's operation serial. An operation nobody can show ran is acknowledged only after its idempotency key has been tried at the provider with a metadata-only request (the fence), or after a person has resolved the review it was held with: a resolved review stands in for the fence, on that person's word and with no provider check. If the fence request took the key, the operation never ran and may be retired, on a provider read taken after the fence when the subscription is current. If the key is held by the operation's own request, that request began and its outcome is not known: the operation is recorded as landed if the provider shows its target, and otherwise held with a review (`billing_quantity_operation_unsettled`) and never replaced until a person resolves it. A fence the provider refuses is held the same way. The fence rests on Stripe's documented key rules, is modelled by a fake in the tests, is not verified against Stripe, and lasts only as long as Stripe remembers a key (documented as at least 24 hours): `docs/superpowers/specs/2026-10-01-stripe-idempotency-evidence.md`. A quantity that goes down inside a period files `billing_quantity_lowered`. An operation is confirmed only by the provider's success answer to its own request. A confirmed landing takes effect at the operation's instant, so a seat's charge and its allowance start together; a quantity that merely matches an unconfirmed operation's target is recorded as `landed_unverified`, dated when observed, with a review. If the provider refuses a request and names `proration_date`, the operation's key is fenced at once and it is retired on the next read; if it does not name it, the operation waits out the 22-hour replay limit. None of the provider's side is confirmed: test mode is not authorized. Only additions of the subscription's current period can raise it; older ones are closed uncharged with a review; additions and operations of an ended subscription are closed. It does not yet lower the quantity at renewal. |
diff --git a/implementation/carloku-paid-launch/OUTSTANDING.md b/implementation/carloku-paid-launch/OUTSTANDING.md| index 9ca249d89..46bf9db85 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -63,8 +63,9 @@ Each is concrete. None is waived. |
| ### Process |
| -19. **No review round has passed.** Astra rejected rounds one to thirteen; Opus accepted each with fixes. Every round has closed the shapes the round before reported and found neighbouring ones; since round eleven they concern records that no walk of this build writes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings were about records no walk of this build writes (a teardown note for another bucket, a record naming two buckets) being acted on by teardown, rotation and the scanning doors; a rotation started after a grant that failed at its last write; and a take-back that returned access to a bucket another account holds, through the member's own door. Round thirteen read those fixes: closed for their reported shapes, Astra rejected again (two P1, four P2) and Opus accepted with fixes (one P2, five P3). The new findings were paths that read such a record and acted without the check (the holder inventory, the storage pass, the declare door), a rotation's own failed last write, and a retained record blocking another account's take-back. The check now sits in the record's reader and writer. Those fixes are on the launch branch and unconfirmed until the next round reads them. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| +19. **No review round has passed.** Astra rejected rounds one to fourteen; Opus accepted each with fixes. Every round has closed the shapes the round before reported and found neighbouring ones, in the store walks since round seven. Some concern records no walk of this build writes; others need only this build's ordinary walks (a failed rotation, a retained teardown, a scanning door racing a teardown), and two were faults in the previous round's own fixes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings were about records no walk of this build writes (a teardown note for another bucket, a record naming two buckets) being acted on by teardown, rotation and the scanning doors; a rotation started after a grant that failed at its last write; and a take-back that returned access to a bucket another account holds, through the member's own door. Round thirteen read those fixes: closed for their reported shapes, Astra rejected again (two P1, four P2) and Opus accepted with fixes (one P2, five P3). The new findings were paths that read such a record and acted without the check (the holder inventory, the storage pass, the declare door), a rotation's own failed last write, and a retained record blocking another account's take-back. The check now sits in the record's reader and writer. Round fourteen read those: closed for their reported shapes; Astra rejected (two P1, two P2), Opus accepted with fixes (two P2, two P3). Its fixes are on the launch branch and unconfirmed until the next round reads them. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| 19g. **Refusals whose doors say the wrong thing, and records only a person can repair.** A record that names two buckets (`ErrTwoBuckets`) or whose teardown note is about another bucket (`ErrUnfinishedTeardown`) is refused by the record's one reader and one writer, so nothing acts on it and nothing writes it; the settings page shows such a store as unreadable. No walk of this build writes either; whether an earlier build left such records in production has not been checked. There is no operator verb to repair one. The doors that meet these refusals answer with sentences written for other failures: the grant and provision doors say to run the grant again, the destroy door and the purge say to try again, the scanning doors say to try again or to re-run the grant. The same goes for a rotation refused because a grant must be run again first (`ErrGrantUnfinished`) and for a member's take-back refused because another account holds the bucket (`ErrBucketHeld`). Correct copy for each needs approval (A). |
| +19h. **A rotation whose delivery fails is repaired by a grant, which swaps the app's key with no overlap.** A failed delivery may still land, so the record stays marked, the next rotation is refused, and the grant run again replaces whichever key the app holds. Hosts hold a dead key until they adopt the new config. Before this branch such a rotation was resumed by running it again, which kept the overlap when the delivery had not landed and broke the app when it had. A repair that waits out the signing window and then reads which key was delivered would keep the overlap; it is not built. |
| 19f. **A store that leaves a bucket with an unsupported name does not hand on its scanner debt.** There is no ledger for such a name. The last holder would delete the bucket and leave a scanning plan behind. Today's scanner (ClamAV) has no plan to leave, so this is latent. |
| 19e. **A purge can end with a store that was granted during it.** A declare and a grant admitted while the app was live can land between the purge's last check and the removal of its marker. The purge now reports that as an error and never deletes the record, but it does not prevent it: the doors check liveness when they admit a request, not when they write. And after that error the estate is named only by its record and a log line. The app's console row is still trashed, so the settings page and the store-destroy door answer 404 and a second purge answers "not trashed", until the console restarts and restores the row from the record. The flash the member sees says "Nothing was deleted", which is false in this case; correct copy needs approval (A). |
| 19b. **Two round-eight findings are accepted and not fixed.** A late request of a failed teardown can remove the policy of a store the same app is granted afterwards under another name (the store goes dark; nothing is lost or leaked). A console whose clock is far ahead reserves a bucket name until that time, with no operator verb to clear it. |
diff --git a/internal/console/apiscan.go b/internal/console/apiscan.go| index 8550a5ff0..015fe258d 100644 |
| --- a/internal/console/apiscan.go |
| +++ b/internal/console/apiscan.go |
| @@ -577,8 +577,11 @@ func (s *Server) regrantForScanning(ctx context.Context, pub *ecdh.PublicKey, ac |
| if rec.Grant != nil && rec.Grant.Source != "" { |
| source = rec.Grant.Source |
| } |
| - _, err = objstore.Grant(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreDeployment, |
| - pub, acct.Sqid, app, addr, source, "", time.Now()) |
| + // Regrant, not Grant: a teardown can run between the scanning record's |
| + // write and this walk, and a scanning verb must not take the store back |
| + // from it. The refusal is made on the record read under the walk's lease. |
| + _, err = objstore.Regrant(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, s.cfg.StoreDeployment, |
| + pub, acct.Sqid, app, addr, source, time.Now()) |
| return err |
| } |
diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go| index 56ed74007..9aeb5209a 100644 |
| --- a/internal/console/apiscan_test.go |
| +++ b/internal/console/apiscan_test.go |
| @@ -9,6 +9,7 @@ import ( |
| "testing" |
| "time" |
| + "github.com/carlosframework/platform/internal/appconfig" |
| "github.com/carlosframework/platform/internal/objstore" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -651,17 +652,33 @@ func TestScanDoorsDoNotReachTheScannerForARefusedRecord(t *testing.T) { |
| c.rec.Scanning = cur.Scanning |
| putRawStore(t, fx, fx.sqid, "titogo", c.rec) |
| before := rawStoreBytes(t, fx, fx.sqid, "titogo") |
| + if status, out := postJSON(t, scanURL(fx, "titogo")+"/grant", map[string]any{}, token); status == http.StatusOK { |
| + t.Errorf("%s: scan/grant answered 200: %v", name, out) |
| + } |
| + for _, b := range []string{"carlos-store-x", "carlos-store-y"} { |
| + if sc.IsEnabled(b) { |
| + t.Errorf("%s: scanning was enabled on %s", name, b) |
| + } |
| + } |
| + // And the doors that switch scanning off do not reach the scanner |
| + // either: another account's store may hold that bucket and be |
| + // owed its scanning. |
| + for _, b := range []string{"carlos-store-x", "carlos-store-y"} { |
| + if err := sc.EnsureEnabled(context.Background(), b); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| for _, door := range []struct { |
| path string |
| body map[string]any |
| - }{{"/grant", map[string]any{}}, {"/enforce", map[string]any{"on": false}}, {"/revoke", map[string]any{}}} { |
| + }{{"/enforce", map[string]any{"on": false}}, {"/revoke", map[string]any{}}} { |
| if status, out := postJSON(t, scanURL(fx, "titogo")+door.path, door.body, token); status == http.StatusOK { |
| t.Errorf("%s: scan%s answered 200: %v", name, door.path, out) |
| } |
| } |
| for _, b := range []string{"carlos-store-x", "carlos-store-y"} { |
| - if sc.IsEnabled(b) { |
| - t.Errorf("%s: scanning was enabled on %s", name, b) |
| + if !sc.IsEnabled(b) { |
| + t.Errorf("%s: a scanning door switched scanning off on %s", name, b) |
| } |
| } |
| if after := rawStoreBytes(t, fx, fx.sqid, "titogo"); after != before { |
| @@ -691,3 +708,47 @@ func TestDeclareDoorDoesNotRewriteARefusedRecord(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// The scanning doors change the record and then re-grant, so the backend |
| +// follows. A teardown can land between the two. The re-grant is the kind |
| +// that never takes a store back: it refuses a record whose teardown has |
| +// begun, and mints and delivers nothing. |
| +func TestScanRegrantDoesNotTakeBackAStoreTornDownInBetween(t *testing.T) { |
| + fx, _, token := scanArmed(t, "code-scan-regrant-race") |
| + _ = token |
| + ctx := context.Background() |
| + acct, err := fx.st.AccountBySqid(fx.sqid) |
| + if err != nil || acct == nil { |
| + t.Fatal(err) |
| + } |
| + fake := fx.srv.cfg.StoreProvisioner.(*objstore.MemProvisioner) |
| + pub, err := appconfig.ParsePublicKey(fx.srv.cfg.SecretsPub, "CARLOS_SECRETS_PUBKEY") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // Healthy: the re-grant converges. |
| + if err := fx.srv.regrantForScanning(ctx, pub, acct, "titogo", "alice@keymail.dev"); err != nil { |
| + t.Fatalf("re-grant of a healthy store: %v", err) |
| + } |
| + // The teardown that lands in between, as its record stands afterwards |
| + // when it left the bucket to another store: block written, user gone. |
| + rec, _, err := objstore.Get(ctx, fx.rel, fx.sqid, "titogo") |
| + if err != nil || rec == nil || rec.Provisioned == nil { |
| + t.Fatalf("fixture: %+v %v", rec, err) |
| + } |
| + user := rec.Provisioned.IAMUser |
| + rec.Teardown = &objstore.TeardownBlock{StartedAt: time.Now().UTC()} |
| + putRawStore(t, fx, fx.sqid, "titogo", rec) |
| + keys := len(fake.Users[user].Keys) |
| + before := rawStoreBytes(t, fx, fx.sqid, "titogo") |
| + err = fx.srv.regrantForScanning(ctx, pub, acct, "titogo", "alice@keymail.dev") |
| + if !errors.Is(err, objstore.ErrStoreBeingTornDown) { |
| + t.Fatalf("re-grant over a teardown that began in between: %v, want ErrStoreBeingTornDown", err) |
| + } |
| + if got := len(fake.Users[user].Keys); got != keys { |
| + t.Fatalf("the re-grant changed the user's keys: %d, were %d", got, keys) |
| + } |
| + if after := rawStoreBytes(t, fx, fx.sqid, "titogo"); after != before { |
| + t.Fatalf("the re-grant changed the record:\n%s\n%s", before, after) |
| + } |
| +} |
diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go| index 1a0527c6c..a23575f93 100644 |
| --- a/internal/objstore/objstore.go |
| +++ b/internal/objstore/objstore.go |
| @@ -69,6 +69,10 @@ type GrantBlock struct { |
| // for its lease can tell that the store it was asked about is not the |
| // one standing now. |
| Walks int `json:"walks,omitempty"` |
| + // LegacyDelivering is where one earlier build of this branch kept the |
| + // key-change mark. It is read, carried to Record.Delivering by the |
| + // reader, and never written again. |
| + LegacyDelivering bool `json:"delivering,omitempty"` |
| } |
| type Provisioned struct { |
| @@ -224,11 +228,12 @@ func Get(ctx context.Context, s release.Store, acct, app string) (*Record, strin |
| return rec, etag, nil |
| } |
| -// Inspect reads the record as it is, coherent or not. It is for the two |
| -// things that have to see a record nothing may act on: deciding who holds |
| -// a bucket (a record that names a bucket holds it, whatever else is wrong |
| -// with it) and showing the record to the person repairing it. Nothing is |
| -// ever done to a store on the strength of a record read this way. |
| +// Inspect reads the record as it is, coherent or not. It is for the one |
| +// thing that has to see a record nothing may act on: deciding who holds a |
| +// bucket (a record that names a bucket holds it, whatever else is wrong |
| +// with it). Nothing is ever done to a store on the strength of a record |
| +// read this way. No door shows such a record yet: the settings page reads |
| +// through Get and shows the store as unreadable. |
| func Inspect(ctx context.Context, s release.Store, acct, app string) (*Record, string, error) { |
| if !release.ValidAccount(acct) { |
| return nil, "", fmt.Errorf("objstore: invalid account %q", acct) |
| @@ -247,6 +252,12 @@ func Inspect(ctx context.Context, s release.Store, acct, app string) (*Record, s |
| if err := json.Unmarshal(raw, &rec); err != nil { |
| return nil, "", fmt.Errorf("objstore: %s: %w", Key(acct, app), err) |
| } |
| + // The mark as an earlier build of this branch wrote it, inside the |
| + // grant block: the same evidence, and it is not dropped. |
| + if rec.Grant != nil && rec.Grant.LegacyDelivering { |
| + rec.Delivering = true |
| + rec.Grant.LegacyDelivering = false |
| + } |
| return &rec, etag, nil |
| } |
diff --git a/internal/objstore/ownership.go b/internal/objstore/ownership.go| index 09525ed8d..b0e0d313d 100644 |
| --- a/internal/objstore/ownership.go |
| +++ b/internal/objstore/ownership.go |
| @@ -76,6 +76,13 @@ type OwnerRef struct { |
| Account string `json:"account_sqid"` |
| App string `json:"app"` |
| Since time.Time `json:"since"` |
| + // refused marks a holder found through a record no walk can follow |
| + // (walk.go Coherent). What such a record holds cannot be reconciled |
| + // with the ledger: an interval the ledger shows as closed may belong |
| + // to an earlier lifetime of the store, and says nothing about the |
| + // estate the record now names. The hold stands whatever the ledger |
| + // shows. |
| + refused bool |
| } |
| // ErrUnfinishedTeardown: the store's record carries a teardown note about |
| @@ -85,6 +92,11 @@ type OwnerRef struct { |
| // again is refused again; an operator repairs the record by hand. |
| var ErrUnfinishedTeardown = errors.New("objstore: the record's teardown note is about another bucket") |
| +// ErrStoreBeingTornDown: a re-grant that only converges a store's policy |
| +// (Regrant, the scanning doors' second step) found that the store's |
| +// teardown has begun. Taking a store back is a grant's decision. |
| +var ErrStoreBeingTornDown = errors.New("objstore: the store is being torn down") |
| + |
| // ErrGrantUnfinished: a grant or a rotation began changing the user's keys |
| // and did not record the result (Record.Delivering). The key the record |
| // names may be gone and the app may hold one the record does not name. A |
| @@ -501,7 +513,7 @@ func beginBucketTeardown(ctx context.Context, s release.Store, bucket, acct, app |
| } |
| } |
| for _, h := range holders { |
| - if known[h.Account+"/"+h.App] { |
| + if known[h.Account+"/"+h.App] && !h.refused { |
| continue |
| } |
| if h.Account != acct { |
diff --git a/internal/objstore/ownership_test.go b/internal/objstore/ownership_test.go| index 898c0a138..8d1c41abb 100644 |
| --- a/internal/objstore/ownership_test.go |
| +++ b/internal/objstore/ownership_test.go |
| @@ -3255,6 +3255,149 @@ func TestTeardownDoesNotDestroyABucketARefusedRecordNames(t *testing.T) { |
| } |
| } |
| +// The same, when the ledger shows a closed interval for the app with the |
| +// refused record: an earlier lifetime of its store on that bucket. A closed |
| +// interval says that lifetime ended. It says nothing about what the |
| +// record names now, and the hold stands: the bucket is not destroyed, and |
| +// a member's take-back is not let through past it. |
| +func TestARefusedRecordsHoldIsNotReconciledAwayByAnOldClosedInterval(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + const sharedY = "carlos-store-shared-y" |
| + p.Buckets[sharedY] = map[string][]byte{"theirs.pdf": []byte("written by the store with the split record")} |
| + // B's earlier lifetime on Y, opened and closed on the ledger. |
| + if err := OpenBucketOwner(ctx, s, sharedY, "bac", "other", t0.Add(-48*time.Hour), t0.Add(-48*time.Hour), false); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := CloseBucketOwner(ctx, s, sharedY, "bac", "other", t0.Add(-24*time.Hour)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // B now: a split record that names Y, with no interval for it. |
| + putRaw(t, s, "bac", "other", &Record{Declared: &Declared{EnvPrefix: "S3", At: t0, By: "member@example.com"}, |
| + Grant: &GrantBlock{Backend: "aws", Bucket: sharedY, Source: GrantSourceOperator, At: t0, By: "op"}, |
| + Provisioned: &Provisioned{Bucket: "carlos-store-legacy-x", Region: "eu-west-1", IAMUser: UserName("bac", "other"), KeyID: "KB", At: t0}}) |
| + // A: a coherent store an operator adopted onto Y. |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, sharedY, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("the teardown: %v, want ErrBucketRetained", err) |
| + } |
| + if got := string(p.Buckets[sharedY]["theirs.pdf"]); got != "written by the store with the split record" { |
| + t.Fatalf("the other store's object in Y is gone (now %q)", got) |
| + } |
| + // A's member asks for the store back. B's refused record still holds Y. |
| + advance(time.Hour) |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "member@example.com", GrantSourceAccountApproval, "", t0.Add(time.Hour)); !errors.Is(err, ErrBucketHeld) { |
| + t.Fatalf("the member's take-back past a refused record: %v, want ErrBucketHeld", err) |
| + } |
| + if _, ok := p.Users[UserName("bab", "titogo")]; ok { |
| + t.Fatal("the refused take-back made a user") |
| + } |
| +} |
| + |
| +// A re-grant that only converges a store's policy never takes the store |
| +// back. The scanning doors look at the record, write it, and then re-grant; |
| +// a teardown can run in between. The refusal is made on the record the |
| +// walk reads under its lease: nothing is minted, delivered or written. |
| +func TestRegrantNeverTakesAStoreBack(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + const shared = "carlos-store-shared-archive" |
| + p.Buckets[shared] = map[string][]byte{} |
| + for _, o := range []struct{ acct, app, prefix string }{{"bab", "titogo", "TITO_S3"}, {"bac", "other", "OTHER_S3"}} { |
| + declare(t, s, o.acct, o.app, o.prefix) |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, o.acct, o.app, "op", GrantSourceOperator, shared, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + // A healthy store: the re-grant converges it. |
| + if _, err := Regrant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, t0.Add(time.Minute)); err != nil { |
| + t.Fatalf("a re-grant of a healthy store: %v", err) |
| + } |
| + // The teardown lands between the scanning door's write and its re-grant. |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("fixture: the teardown: %v, want ErrBucketRetained", err) |
| + } |
| + p.Buckets[shared]["theirs.pdf"] = []byte("written by the other account after this store left") |
| + before := inspect(t, s, "bab", "titogo") |
| + advance(time.Minute) |
| + if _, err := Regrant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, t0.Add(time.Hour)); !errors.Is(err, ErrStoreBeingTornDown) { |
| + t.Fatalf("a re-grant after the teardown: %v, want ErrStoreBeingTornDown", err) |
| + } |
| + if _, ok := p.Users[UserName("bab", "titogo")]; ok { |
| + t.Fatal("the refused re-grant made a user") |
| + } |
| + if after := inspect(t, s, "bab", "titogo"); !reflect.DeepEqual(before, after) { |
| + t.Fatalf("the refused re-grant changed the record:\n%+v\n%+v", before, after) |
| + } |
| + if w, err := acquireBucketWalk(ctx, s, shared, "bab", "second", "grant"); err != nil { |
| + t.Fatalf("the refused re-grant left the name reserved: %v", err) |
| + } else { |
| + w.release(true) |
| + } |
| +} |
| + |
| +// One earlier build of this branch kept the key-change mark inside the grant |
| +// block. A record it left marked is still marked to this build: both |
| +// rotation verbs refuse it, the grant repairs it, and the repaired record |
| +// carries the mark in neither place. |
| +func TestTheMarkIsReadFromWhereAnEarlierBuildWroteIt(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", t0.Add(time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + // Rewrite the record in the earlier shape: the mark under "grant". |
| + raw, etag, err := s.Get(ctx, Key("bab", "titogo")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var doc map[string]any |
| + if err := json.Unmarshal(raw, &doc); err != nil { |
| + t.Fatal(err) |
| + } |
| + delete(doc, "delivering") |
| + doc["grant"].(map[string]any)["delivering"] = true |
| + old, _ := json.Marshal(doc) |
| + if _, err := s.Put(ctx, Key("bab", "titogo"), old, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + if rec, _, err := Get(ctx, s, "bab", "titogo"); err != nil || rec == nil || !rec.Delivering { |
| + t.Fatalf("the mark was dropped on read: %+v %v", rec, err) |
| + } |
| + if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", t0.Add(time.Hour)); !errors.Is(err, ErrGrantUnfinished) { |
| + t.Fatalf("rotate over the earlier mark: %v, want ErrGrantUnfinished", err) |
| + } |
| + if _, err := RotateFinish(ctx, s, p, "bab", "titogo"); !errors.Is(err, ErrGrantUnfinished) { |
| + t.Fatalf("rotate finish over the earlier mark: %v, want ErrGrantUnfinished", err) |
| + } |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(2*time.Hour)); err != nil { |
| + t.Fatalf("the repairing grant: %v", err) |
| + } |
| + raw, _, err = s.Get(ctx, Key("bab", "titogo")) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if strings.Contains(string(raw), "delivering") { |
| + t.Fatalf("the repaired record still carries a mark:\n%s", raw) |
| + } |
| +} |
| + |
| // An older grant block that names no bucket (the name used to be derived |
| // each time). A grant resumed over it creates the bucket; before it does, |
| // the block is made to name it, in the write that counts the walk. So when |
| @@ -3394,3 +3537,98 @@ func TestADeletionNoteWithNoBucketNameIsRefused(t *testing.T) { |
| t.Fatalf("a destruction was written on the ledger of a live bucket: %+v", l) |
| } |
| } |
| + |
| +// A rotation that fails before it delivers anything (here the mint) has |
| +// not touched the key the app holds, and leaves no mark. Running it again |
| +// resumes: the held key is kept, as the overlap, and is not replaced by a |
| +// repairing grant. |
| +func TestRotationThatFailsBeforeDeliveryIsResumedWithItsOverlap(t *testing.T) { |
| + for _, failAt := range []string{"ReapKeys", "MintKey"} { |
| + 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") |
| + first, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + k1 := first.Provisioned.KeyID |
| + p.FailNext = map[string]error{failAt: errors.New("injected " + failAt)} |
| + if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", t0.Add(time.Hour)); err == nil { |
| + t.Fatalf("%s: fixture: the rotation was meant to fail", failAt) |
| + } |
| + p.FailNext = nil |
| + cur, _, err := Get(ctx, s, "bab", "titogo") |
| + if err != nil || cur == nil || cur.Delivering || cur.Provisioned.KeyID != k1 { |
| + t.Fatalf("%s: a rotation that delivered nothing marked or changed the record: %+v %v", failAt, cur, err) |
| + } |
| + advance(walkLease + time.Minute) |
| + rotated, err := Rotate(ctx, s, p, pub, "bab", "titogo", t0.Add(2*time.Hour)) |
| + if err != nil { |
| + t.Fatalf("%s: the rotation run again: %v", failAt, err) |
| + } |
| + user := UserName("bab", "titogo") |
| + if _, held := p.Users[user].Keys[k1]; !held || rotated.Provisioned.PrevKeyID != k1 || rotated.Delivering { |
| + t.Fatalf("%s: the resumed rotation did not keep the app's key as its overlap: %+v, keys %v", failAt, rotated.Provisioned, p.Users[user].Keys) |
| + } |
| + } |
| +} |
| + |
| +// neverLister fails the test if a sweep lists a bucket. |
| +type neverLister struct{ t *testing.T } |
| + |
| +func (n neverLister) ListPage(ctx context.Context, bucket, startAfter string, limit int32) ([]ScanObject, bool, error) { |
| + n.t.Errorf("a sweep listed %s", bucket) |
| + return nil, false, nil |
| +} |
| + |
| +// A store left its bucket to another account's store: its teardown revoked |
| +// its access and kept its record, with scanning still enabled on it. The |
| +// scan pass does not sweep it and the queue does not take work for it: the |
| +// objects in that bucket are the other account's now, and a sweep would |
| +// list them into coverage this app's member can read. |
| +func TestAStoreUnderTeardownIsNotSweptOrQueued(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + leaseClock(t) |
| + rec := &Record{Declared: &Declared{EnvPrefix: "S3", At: t0, By: "member@example.com"}, |
| + Grant: &GrantBlock{Backend: "aws", Bucket: "carlos-store-shared-x", Source: GrantSourceOperator, At: t0, By: "op"}, |
| + Provisioned: &Provisioned{Bucket: "carlos-store-shared-x", Region: "eu-west-1", IAMUser: UserName("bab", "titogo"), KeyID: "K", At: t0}, |
| + Scanning: &ScanBlock{Requested: &ScanRequested{}, Enabled: &ScanEnabled{}}, |
| + Teardown: &TeardownBlock{StartedAt: t0}} |
| + if _, err := Put(ctx, s, "bab", "titogo", rec, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + side, err := RunSweep(ctx, s, neverLister{t}, NewMemScanner(), "bab", "titogo", time.Second) |
| + if err != nil || side != nil { |
| + t.Fatalf("RunSweep of a store under teardown: %+v %v, want nothing", side, err) |
| + } |
| + if _, ok, err := queuePermitted(ctx, s, "bab", "titogo"); err != nil || ok { |
| + t.Fatalf("queuePermitted for a store under teardown: %v %v, want not permitted", ok, err) |
| + } |
| +} |
| + |
| +// A provisioned block with no bucket name beside a grant block that names |
| +// one: the teardown works from the provisioned block and would find |
| +// nothing to destroy, then delete the record. The reader refuses it. |
| +func TestAProvisionedBlockWithNoBucketBesideAGrantThatNamesOneIsRefused(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + p.Buckets[ledgerBucket] = map[string][]byte{"mine.pdf": []byte("this store's object")} |
| + putRaw(t, s, "bab", "titogo", &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, |
| + Grant: &GrantBlock{Backend: "aws", Bucket: ledgerBucket, Source: GrantSourceOperator, At: t0, By: "op"}, |
| + Provisioned: &Provisioned{Region: "eu-west-1", IAMUser: UserName("bab", "titogo"), KeyID: "K", At: t0}}) |
| + if got, _, err := Get(ctx, s, "bab", "titogo"); !errors.Is(err, ErrTwoBuckets) || got != nil { |
| + t.Fatalf("Get: %+v %v, want ErrTwoBuckets", got, err) |
| + } |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrTwoBuckets) { |
| + t.Fatalf("teardown: %v, want ErrTwoBuckets", err) |
| + } |
| + if inspect(t, s, "bab", "titogo") == nil || string(p.Buckets[ledgerBucket]["mine.pdf"]) != "this store's object" { |
| + t.Fatal("the refused teardown removed the record or the object") |
| + } |
| +} |
diff --git a/internal/objstore/queue.go b/internal/objstore/queue.go| index e696b5b4e..d7efbed81 100644 |
| --- a/internal/objstore/queue.go |
| +++ b/internal/objstore/queue.go |
| @@ -483,7 +483,8 @@ func queuePermitted(ctx context.Context, s release.Store, acct, app string) (*Re |
| if err != nil { |
| return nil, false, err |
| } |
| - if rec == nil || rec.Provisioned == nil { |
| + // Not provisioned, or being torn down (see RunSweep): nothing to scan. |
| + if rec == nil || rec.Provisioned == nil || rec.Teardown != nil { |
| return nil, false, nil |
| } |
| tomb, err := release.ReadAppTombstone(ctx, s, acct, app) |
diff --git a/internal/objstore/sweep.go b/internal/objstore/sweep.go| index 0af7fa4a2..01466a46e 100644 |
| --- a/internal/objstore/sweep.go |
| +++ b/internal/objstore/sweep.go |
| @@ -132,6 +132,14 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| if rec == nil || rec.Provisioned == nil || rec.Scanning == nil || rec.Scanning.Enabled == nil { |
| return nil, nil |
| } |
| + // A store whose teardown has begun is not swept. Its access to the |
| + // bucket is being, or has been, revoked; a teardown that left the |
| + // bucket to another account's store leaves this record standing, and a |
| + // sweep would list that account's objects into coverage this app's |
| + // member can read. |
| + if rec.Teardown != nil { |
| + return nil, nil |
| + } |
| // A trashed store is destroyed within 30 days. Scanning it bills for |
| // egress on data that is on its way out, and reports coverage for an app |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index 117b521d0..29233b6bf 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -142,6 +142,24 @@ func ValidBucketName(name string) bool { return bucketNameRe.MatchString(name) } |
| // it must not re-choose the bucket. |
| func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| sealPub *ecdh.PublicKey, acct, app, actor, source, adoptBucket string, now time.Time) (granted *Record, walkErr error) { |
| + return grant(ctx, s, p, d, sealPub, acct, app, actor, source, adoptBucket, now, false) |
| +} |
| + |
| +// Regrant converges an already granted store on its record: the policy is |
| +// rewritten, and anything not delivered is delivered. It is Grant for |
| +// callers that changed something else on the record (the scanning doors) |
| +// and need the backend to follow. It never takes a store back: if the |
| +// record read under the lease carries a teardown block, whenever that |
| +// teardown began, it is refused (ErrStoreBeingTornDown) before anything is |
| +// written or called. A caller's own look at the record beforehand cannot |
| +// promise that; a teardown can run between the look and the walk. |
| +func Regrant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| + sealPub *ecdh.PublicKey, acct, app, actor, source string, now time.Time) (*Record, error) { |
| + return grant(ctx, s, p, d, sealPub, acct, app, actor, source, "", now, true) |
| +} |
| + |
| +func grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| + sealPub *ecdh.PublicKey, acct, app, actor, source, adoptBucket string, now time.Time, noTakeBack bool) (granted *Record, walkErr error) { |
| // The caller's clock was read before this walk began. What the walk |
| // observes, it observes later: the completeness claim below is dated |
| // by the caller's clock plus the time this walk has run, taken AFTER |
| @@ -231,6 +249,9 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| 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) |
| } |
| + if noTakeBack && rec.Teardown != nil { |
| + return nil, fmt.Errorf("%w: %s/%s", ErrStoreBeingTornDown, acct, app) |
| + } |
| rebuilt = rec.Teardown != nil |
| startKeyID = "" |
| if rec.Provisioned != nil { |
| @@ -297,7 +318,7 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // keeps its record only until its retained teardown is run |
| // again. Only what the ledger shows counts for that: a store |
| // with no interval at all has an unknown history, and holds. |
| - if ledger != nil && ledger.left(h.Account, h.App) { |
| + if !h.refused && ledger != nil && ledger.left(h.Account, h.App) { |
| continue |
| } |
| return nil, fmt.Errorf("%w: %s by %s/%s of another account; only an operator's grant gives %s/%s access to it again", ErrBucketHeld, bucket, h.Account, h.App, acct, app) |
| @@ -655,12 +676,15 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // key still working. The reverse gap is deliberate — after a successful |
| // delivery whose Put then fails, the record names the old key while the app |
| // already holds the new one, and both are live, so the app keeps working. |
| -// It is not left unmarked: before step one the record is marked as having a |
| -// key change under way (Record.Delivering), and step five takes the mark |
| -// off. A rotation that stopped anywhere in between leaves the mark, the |
| -// next rotation is refused instead of reaping the key the app holds, and a |
| -// grant run again walks in full and leaves the record naming a key the user |
| -// has. |
| +// It is not left unmarked: just before step four the record is marked as |
| +// having a key change under way (Record.Delivering), and step five takes |
| +// the mark off. A rotation that stopped before that mark is resumed by |
| +// running it again, with the key the app holds kept. One that stopped after |
| +// it, a failed delivery included, leaves the mark: the next |
| +// rotation is refused, because it would reap whichever key the app now |
| +// holds without knowing which that is, and a grant run again walks in full. |
| +// That grant reaps and replaces the app's key; it is a repair, not an |
| +// overlap. |
| // |
| // The one shape of that gap which is NOT survivable is a lost CAS: the record |
| // would keep naming the old key permanently, and the next rotation's reap |
| @@ -693,26 +717,6 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| if rec.Declared == nil { |
| return nil, ErrNotDeclared |
| } |
| - // Written ahead of every key change: from here until the last write |
| - // the record may name a key that is not the one the app holds. The |
| - // declare door writes this same object; if it wrote in between, the |
| - // mark goes on the record as it now stands, provided it is still the |
| - // same estate. |
| - asked := estateOf(rec) |
| - rec.Delivering = true |
| - if etag, err = Put(ctx, s, acct, app, rec, etag); errors.Is(err, release.ErrCASConflict) { |
| - fresh, freshEtag, gerr := Get(ctx, s, acct, app) |
| - if gerr != nil || fresh == nil || fresh.Provisioned == nil || fresh.Declared == nil || fresh.Teardown != nil || fresh.Delivering || estateOf(fresh) != asked { |
| - return nil, err |
| - } |
| - fresh.Delivering = true |
| - if etag, err = Put(ctx, s, acct, app, fresh, freshEtag); err != nil { |
| - return nil, err |
| - } |
| - rec = fresh |
| - } else if err != nil { |
| - return nil, err |
| - } |
| pr := rec.Provisioned |
| startKeyID := pr.KeyID // the key the app is holding, and the retry guard |
| // 1. finish the pending rotation late. The reap below would collect this |
| @@ -732,24 +736,39 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| if err != nil { |
| return nil, fmt.Errorf("objstore: mint key for %s: %w", pr.IAMUser, err) |
| } |
| + // Written ahead of delivery, and not before: until here the app holds |
| + // the key the record names, and a rotation that failed at steps one to |
| + // three is resumed by running it again, which keeps that key and its |
| + // overlap (the reap collects the stray). From here until the last |
| + // write the app may hold a key the record does not name, and the |
| + // record says so (Record.Delivering). The declare door writes this |
| + // same object; if it wrote in between, the mark goes on the record as |
| + // it now stands, provided it is still the same estate. |
| + asked := estateOf(rec) |
| + rec.Delivering = true |
| + if etag, err = Put(ctx, s, acct, app, rec, etag); errors.Is(err, release.ErrCASConflict) { |
| + fresh, freshEtag, gerr := Get(ctx, s, acct, app) |
| + if gerr != nil || fresh == nil || fresh.Provisioned == nil || fresh.Declared == nil || fresh.Teardown != nil || fresh.Delivering || estateOf(fresh) != asked { |
| + return nil, err |
| + } |
| + fresh.Delivering = true |
| + if etag, err = Put(ctx, s, acct, app, fresh, freshEtag); err != nil { |
| + return nil, err |
| + } |
| + rec = fresh |
| + pr = rec.Provisioned |
| + } else if err != nil { |
| + return nil, err |
| + } |
| // 4. deliver — same names, same single CAS write, same validation of the |
| // credential halves before sealing (writeEnv owns that rail). |
| // |
| - // If delivery fails and the app's config is byte for byte what it was |
| - // before the attempt, the new key was not delivered: the app still |
| - // holds the key the record names, and the mark comes off so that |
| - // running the rotation again resumes (it reaps the stray key and mints |
| - // another). If the config changed, or cannot be read, nothing is |
| - // assumed and the mark stays. |
| - cfgBefore, cfgKnown := configGeneration(ctx, s, acct, app) |
| + // A delivery that returns an error may still have been sent, and a |
| + // write that was sent can land after any read made here. So the mark |
| + // stays on every failure from this point: nothing this walk can observe |
| + // proves the app's config will not come to hold the new key. |
| if err := writeEnv(ctx, s, sealPub, acct, app, rec.Declared.EnvPrefix, |
| pr.Bucket, pr.Region, pr.Endpoint, id, secret); err != nil { |
| - if cfgAfter, ok := configGeneration(ctx, s, acct, app); cfgKnown && ok && cfgAfter == cfgBefore { |
| - rec.Delivering = false |
| - if _, perr := Put(ctx, s, acct, app, rec, etag); perr != nil { |
| - err = errors.Join(err, fmt.Errorf("objstore: clear the key-change mark of %s/%s: %w", acct, app, perr)) |
| - } |
| - } |
| return nil, err |
| } |
| // 5. record: the key just retired from live becomes the pending one. |
| @@ -854,9 +873,11 @@ func leaseForRotation(ctx context.Context, s release.Store, acct, app string, fi |
| case rec.Teardown != nil || estateOf(rec) != asked: |
| err = fmt.Errorf("objstore: store record for %s/%s changed while its rotation was starting; run it again: %w", acct, app, release.ErrCASConflict) |
| case rec.Delivering: |
| - // A grant reaped the recorded key, may have delivered another, and |
| - // did not record it. Rotating now would reap the key the app was |
| - // given. The grant is run again first: it repairs in full. |
| + // A grant or a rotation began delivering a key and did not record |
| + // the result: the app may hold a key the record does not name, and |
| + // a rotation's reap keeps only the named one. It is refused, and |
| + // the grant is run again first. That grant replaces the app's key |
| + // and leaves the record naming one the user has. |
| err = fmt.Errorf("%w: %s/%s", ErrGrantUnfinished, acct, app) |
| } |
| if err != nil { |
| @@ -1324,7 +1345,8 @@ func estateOf(rec *Record) storeEstate { |
| func walkEndedClean(err error) bool { |
| return err == nil || errors.Is(err, ErrBucketRetained) || errors.Is(err, ErrBucketHeld) || |
| errors.Is(err, ErrBucketBeingDestroyed) || errors.Is(err, ErrNotDeclared) || errors.Is(err, ErrNoPendingFinish) || |
| - errors.Is(err, ErrUnfinishedTeardown) || errors.Is(err, ErrTwoBuckets) || errors.Is(err, ErrGrantUnfinished) |
| + errors.Is(err, ErrUnfinishedTeardown) || errors.Is(err, ErrTwoBuckets) || errors.Is(err, ErrGrantUnfinished) || |
| + errors.Is(err, ErrStoreBeingTornDown) |
| } |
| // grantElapsed is how long a grant has been running. A seam, so a test can |
| @@ -1392,7 +1414,7 @@ func storeHolders(ctx context.Context, s release.Store, bucket, acct, app string |
| named = true |
| } |
| if named { |
| - out = append(out, OwnerRef{Account: a, App: name}) |
| + out = append(out, OwnerRef{Account: a, App: name, refused: true}) |
| } |
| continue |
| } |
| @@ -1561,8 +1583,11 @@ func Coherent(rec *Record, acct, app string) error { |
| if rec == nil { |
| return nil |
| } |
| - if rec.Grant != nil && rec.Provisioned != nil && rec.Grant.Bucket != "" && rec.Provisioned.Bucket != "" && rec.Grant.Bucket != rec.Provisioned.Bucket { |
| - return fmt.Errorf("%w: %s/%s is provisioned on %s and its grant names %s", ErrTwoBuckets, acct, app, rec.Provisioned.Bucket, rec.Grant.Bucket) |
| + // A provisioned block always names its bucket, so an empty name there |
| + // beside a grant block that names one is the same disagreement: the |
| + // teardown works from the provisioned block and would find nothing. |
| + if rec.Grant != nil && rec.Provisioned != nil && rec.Grant.Bucket != "" && rec.Grant.Bucket != rec.Provisioned.Bucket { |
| + return fmt.Errorf("%w: %s/%s is provisioned on %q and its grant names %s", ErrTwoBuckets, acct, app, rec.Provisioned.Bucket, rec.Grant.Bucket) |
| } |
| if rec.Teardown != nil { |
| own := "" |
| @@ -1583,17 +1608,3 @@ func Coherent(rec *Record, acct, app string) error { |
| } |
| return nil |
| } |
| - |
| -// configGeneration is the generation of the app's config object as the |
| -// store reports it ("" when there is none). ok is false when it could not |
| -// be read. |
| -func configGeneration(ctx context.Context, s release.Store, acct, app string) (generation string, ok bool) { |
| - _, etag, err := s.Get(ctx, appconfig.Key(acct, app)) |
| - if errors.Is(err, os.ErrNotExist) { |
| - return "", true |
| - } |
| - if err != nil { |
| - return "", false |
| - } |
| - return etag, true |
| -} |
diff --git a/internal/objstore/walk_test.go b/internal/objstore/walk_test.go| index f42481ea5..5e3b0babd 100644 |
| --- a/internal/objstore/walk_test.go |
| +++ b/internal/objstore/walk_test.go |
| @@ -703,10 +703,13 @@ func TestRotateTwiceReapsPending(t *testing.T) { |
| } |
| // A rotate that mints and then fails to reseal leaves a key the record does |
| -// not name. Re-running must collect it (the reap) rather than mint into a full |
| -// estate, and the app must end up holding the key the record names. |
| -func TestRotateResumesAfterResealFailure(t *testing.T) { |
| +// not name, and a record that says a key change began (Delivering). The |
| +// failed reseal may still have been sent, so which key the app holds is not |
| +// known to the walk: running the rotation again is refused, the grant run |
| +// again repairs, and the app ends up holding the key the record names. |
| +func TestRotateAfterAResealFailureIsRepairedByTheGrant(t *testing.T) { |
| ctx := context.Background() |
| + advance := leaseClock(t) |
| priv, pub := testSealKeypair(t) |
| s := &failingStore{Store: release.NewDirStore(t.TempDir())} |
| p := NewMemProvisioner("eu-west-1") |
| @@ -732,16 +735,21 @@ func TestRotateResumesAfterResealFailure(t *testing.T) { |
| t.Fatalf("failed rotate moved the app's key to %q, want %q", got, oldID) |
| } |
| - rec, err := Rotate(ctx, s, p, pub, "bab", "titogo", time.Now().UTC()) |
| + if !half.Delivering { |
| + t.Fatal("the record does not say a key change began and was not recorded") |
| + } |
| + if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", time.Now().UTC()); !errors.Is(err, ErrGrantUnfinished) { |
| + t.Fatalf("a rotation after a failed reseal: %v, want ErrGrantUnfinished", err) |
| + } |
| + // The failed rotation's lease stands until it runs out. |
| + advance(walkLease + time.Minute) |
| + rec, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", time.Now().UTC()) |
| if err != nil { |
| - t.Fatalf("resume after reseal failure: %v", err) |
| + t.Fatalf("the grant after a failed reseal: %v", err) |
| } |
| keys := p.Users[user].Keys |
| - if len(keys) != 2 { |
| - t.Fatalf("keys after resume = %v, want exactly 2 (stray reaped)", keys) |
| - } |
| - if rec.Provisioned.PrevKeyID != oldID { |
| - t.Fatalf("PrevKeyID = %q, want %q", rec.Provisioned.PrevKeyID, oldID) |
| + if len(keys) != 1 || rec.Delivering { |
| + t.Fatalf("after the repair: keys %v, delivering %v; want one key and no mark", keys, rec.Delivering) |
| } |
| if _, ok := keys[rec.Provisioned.KeyID]; !ok { |
| t.Fatalf("recorded key %q is not live: %v", rec.Provisioned.KeyID, keys) |