amadan
Close round twelve: one check for records no walk can follow, a delivering mark on the grant, and a member's take-back refused over another account's bucket
Round twelve (Astra reject: one P1, two P2; Opus accept with fixes: one P2, three P3) found every round eleven finding closed for its reported shape, and found the paths round eleven had not covered. - objstore.Coherent: a record that names two buckets, or whose teardown note is about a bucket that is not its own, is refused before anything is touched by Grant, Teardown (before its declaration-only delete), both rotation verbs, the scanning doors and the sweep. The purge treats a record with a teardown block as an estate. - GrantBlock.Delivering: a grant that will reap and deliver says so in the write that counts it and takes the mark off when it records the result. While it stands a grant run again walks in full and a rotation is refused (ErrGrantUnfinished). - A take-back whose source is not an operator is refused when a store of another account holds the bucket. Each fix has a reproduction and was mutation-checked in a scratch copy. The round's dispositions and the round thirteen brief are added. No review round has passed. Billing is off; nothing attests.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
13 files changed,
+584
−93
docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md+21 −0docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round13.md+28 −0implementation/carloku-paid-launch/AGENT_HANDOFF.md+1 −1implementation/carloku-paid-launch/OUTSTANDING.md+2 −2internal/console/apiscan.go+15 −3internal/console/apiscan_test.go+46 −0internal/objstore/objstore.go+8 −0internal/objstore/ownership.go+15 −7internal/objstore/ownership_test.go+304 −50internal/objstore/sweep.go+6 −0internal/objstore/walk.go+130 −29internal/release/trash.go+5 −1internal/release/trash_test.go+3 −0
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 c77fe11c8..9d0cc2eda 100644 |
| --- a/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| +++ b/docs/superpowers/specs/CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md |
| @@ -387,3 +387,24 @@ Astra: one P1, two P2. Opus: one P2, one P3. Astra's F1 and Opus's N1 are the sa |
| | Both, claims: "every grant that creates anything is counted before it calls the provider"; "a walk requires the same estate" | Made true by F3 and F2, and the handoff reworded. | `AGENT_HANDOFF.md` item 5, `objstore.go` | none | |
| Mutation checks in a scratch copy, each failing its test: the provisioned bucket not preferred; the grant block written as minted; rotation comparing bucket and key only; the count moved after `EnsureUser`; the teardown-note refusal off; the two-bucket refusal off in Grant, and off in Teardown. Storage simulation: 3,000 seeds on this code. |
| + |
| +## Round twelve (reviewed `1395f7bf`; Astra rejected, Opus accepted with fixes) |
| + |
| +Reports: `CODEX-FINDINGS-carloku-paid-launch-overnight-round12.md`, `OPUS-REVIEW-carloku-paid-launch-overnight-round12.md`. |
| + |
| +Both found every round eleven finding closed for its reported shape. Astra: one P1, two P2. Opus: one P2, three P3. Astra's F3 and Opus's N2 are the same fault; Astra's F1 and Opus's N3 are two shapes of one. Nothing that attests, reaches Stripe with a switch unset, or makes an open late-request defect reachable. |
| + |
| +Round eleven refused two kinds of record that no walk writes, each in the walks where it had been reported. Round twelve found the other paths acting on them. The fix is one check (`objstore.Coherent`) that every path acting on a store record makes first. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra F1, Opus N3: a teardown takes a note about another bucket at its word, skips the record's own live bucket, writes the other bucket's deletion on its ledger and deletes the record; a teardown-only record with such a note is deleted by the declaration-only path or the purge | Accepted. A record whose teardown note names a bucket that is not its own, or notes a deletion while naming no bucket, is refused by Teardown before its declaration-only delete, and by every other path below. The purge treats a record with a teardown block as an estate, not a bare declaration. The sentinel no longer says to finish the teardown. | `walk.go` `Coherent`, `Teardown`; `ownership.go`; `release/trash.go` | `TestWalksRefuseATeardownNoteAboutAnotherBucket` (live bucket, note about another: teardown, grant and both rotation verbs refused; record, objects, keys and ledger unchanged), `TestGrantIsRefusedOverATeardownNoteForAnotherBucket` (now also runs the teardown), `TestPurgeAppRefusesProvisionedOrGrantedStore/teardown-only` | |
| +| Astra F3, Opus N2: rotation, the scanning doors and the sweep act on a record that names two buckets | Accepted. Both rotation verbs, the scanning record writer every scanning door goes through, the scan grant and revoke doors before they call the scanner, and the sweep all make the same check. | `walk.go` `Rotate`, `RotateFinish`, `leaseForRotation`; `sweep.go`; `console/apiscan.go` | `TestWalksRefuseARecordThatNamesTwoBuckets` (now both rotation verbs and the sweep, keys unchanged), `TestScanRecordWriterRefusesARecordNoWalkCanFollow` | |
| +| Astra F2: a rotation started after a grant that reaped the recorded key, delivered another and failed at its last write reaps the delivered key; the grant run again takes the healthy short-circuit and repairs nothing | Accepted. Opus read it as pre-existing and availability only; the stricter reading is taken. A grant that will reap and deliver says so in the write that counts it (`GrantBlock.Delivering`) and takes the mark off in the write that records the result. While the mark stands a grant run again walks in full, and both rotation verbs are refused (`ErrGrantUnfinished`) without a provider call. Health is now decided before the count, from reads. | `walk.go` `Grant`, `leaseForRotation`; `objstore.go`; `ownership.go` | `TestRotationIsRefusedAfterAGrantThatFailedAtItsLastWrite` (same deployment, everything delivered, both verbs; the re-run grant records a key the user has; rotation then works) | |
| +| Opus N1: the member's own door takes a store back over a bucket that a store of another account holds, and so regains access to what that account wrote after it left | Accepted. A take-back whose source is not an operator is refused (`ErrBucketHeld`) when a record or an open ledger interval of another account names the bucket, before anything is written. An operator's grant goes through. On this branch only: round eleven's root fix made the take-back keep the shared bucket. | `walk.go` `Grant` | `TestMembersTakeBackIsRefusedWhileAnotherAccountHoldsTheBucket` (holder by record, holder by ledger interval; no user, record unchanged, lease given back; operator source accepted) | |
| +| Opus N4: the adopted-bucket case of a round eleven test ran against a record the first case had already granted | Corrected: each case builds its own fixture. | `ownership_test.go` | `TestGrantOverATeardownKeepsTheRecordsOwnBucket` | |
| +| Both, claims: "before it calls the provider at all" | Corrected: two read-only probes come before the count; no call that changes anything does. | `AGENT_HANDOFF.md` item 5 | none | |
| +| Both, claims: 19g named only the grant door's wrong sentence; "finish the teardown" | Corrected. 19g lists every door, and the refusals' way out is an operator's repair by hand, for which there is no verb. Not fixed: the doors' sentences, which need approved copy. | `OUTSTANDING.md` 19g, `ownership.go` | none | |
| +| Opus, asked and not found: Grant's final-write retry could land a provisioned block over a newer grant block if the signing fence did not hold | Noted. It rests on the fence, which is observed on AWS only (19d). Not changed. | none | none | |
| + |
| +Mutation checks in a scratch copy, each failing its test: the note check off; the check removed from each rotation verb, from the sweep, from the scanning writer; the purge ignoring a teardown block; the delivering mark not written, not read by rotation, not read by the health check; the member take-back check off, and its ledger half off. Storage simulation: 3,000 seeds on this code. |
diff --git a/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round13.md b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round13.md| new file mode 100644 |
| index 000000000..01270e3b5 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-REVIEW-BRIEF-carloku-paid-launch-overnight-round13.md |
| @@ -0,0 +1,28 @@ |
| +# Review brief, round 13: 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 12 briefs for rulings 6 to 12, then `CODEX-DISPOSITIONS-carloku-paid-launch-overnight.md` (its last section covers round 12) 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 12 are `git diff 1395f7bf <commit>`. The code changes are in `internal/objstore` (`walk.go`, `ownership.go`, `objstore.go`, `sweep.go`, tests), `internal/console/apiscan.go` and its test, and `internal/release/trash.go` and its test. The rest is documents. |
| + |
| +Still the fixed snapshot: round 12's findings, nothing else. Nothing may attest. The quantity worker has two switches, both unset. Billing is off. |
| + |
| +Do not report again, as findings, what `OUTSTANDING.md` lists as open: items 6 and 6a, 7 and 9, 19a to 19g. Do report anything that makes one of them worse than the document says, and any claim about them the code contradicts. |
| + |
| +## First: a disposition table |
| + |
| +For every round 12 finding (Astra F1 to F3 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 |
| + |
| +- **One check for records no walk can follow** (`objstore.Coherent`). The claim: every path that acts on a store record, at the provider or on the record, makes this check first and leaves a refused record exactly as found. Find a path that does not: a door, a loop, a sweep, a purge, a reader that then writes. Find a record this build can write that the check refuses. Find a third shape of record no walk can follow that the check accepts and a walk then mishandles; say whether this build can write it. |
| +- **The delivering mark** (`GrantBlock.Delivering`). Find a grant that reaps or mints with the mark not yet on the record, or a successful grant that leaves it set. Find a store that is refused rotation for good, or a grant re-run that cannot clear the mark. Health is now decided before the count: find a harm in that order. Say what the mark means on a record written by an earlier build (it reads false). |
| +- **The member's take-back** (the refusal for a source that is not an operator). Find a way for a member's door to regain access to a bucket another account holds: another source value, a holder the check cannot see, a record that is not a take-back. Find a store that only an operator can now restore where that is wrong. |
| +- **The purge and a teardown block** (`release/trash.go`). Find a record with a teardown block that should be purged and now never is. |
| +- **Claims** in `AGENT_HANDOFF.md` item 5, `OUTSTANDING.md` 19 and 19g, and the round 12 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/implementation/carloku-paid-launch/AGENT_HANDOFF.md b/implementation/carloku-paid-launch/AGENT_HANDOFF.md| index 9fe01bb90..7b91f7f87 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 it calls the provider at all, the user and its policy included, so a healthy store granted again writes its record once. 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) is refused by grant and teardown before they touch anything (`ErrTwoBuckets`); a person decides. 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 that is about to reap and deliver keys says so in that same write (`GrantBlock.Delivering`) and takes the mark off when it records the result; while the mark stands a grant run again walks in full and a rotation is refused (`ErrGrantUnfinished`). A record's bucket is the one it is provisioned on, else the one its grant block names: the arguments choose a bucket only for a store that has none, so a grant never moves a store to another name. A record whose grant block and provisioned block name different buckets (an earlier build could write one), or whose teardown note is about a bucket that is not its own, is refused before anything is touched by every path that acts on a store record: grant, teardown, both rotation verbs, the scanning doors, the sweep and the purge (`objstore.Coherent`; `ErrTwoBuckets`, `ErrUnfinishedTeardown`). An operator repairs such a record by hand; there is no verb for it. A take-back over a bucket that a store of another account holds is refused unless an operator grants it. A failed walk's lease stands until it runs out, with one exception: the same app re-running the same kind of walk may take it at once. A declaration-only teardown and the app purge take no lease; they delete the store record only as the generation they read. The user belongs to the app and the lease to the name, so a late request of a failed teardown can still remove the policy of a store granted afterwards under another name (round eight, Opus N2, open). Every request of a walk, to S3, to IAM and to the deployment bucket that holds the records and ledgers, is signed with the instant its lease was taken (`internal/sigfence`), so AWS refuses a request a paused process sends after its lease could be taken over. The record and its scan sidecar are deleted as the generation that was read. The signing-time rule was checked read-only against S3 and IAM with a list and two gets (`TestFenceAgainstAWS`); no delete was sent. A declaration-only teardown takes no lease (there is no bucket name) and relies on the conditional delete alone. A teardown writes a block on its own record before it touches anything and decides by compare-and-swap on the ledger (for a bucket name this build would refuse there is no ledger, and it decides from the store records alone): if nobody else holds the bucket it marks the bucket as being destroyed; otherwise it leaves the bucket and its scanner, revokes its own credentials, and records that the scanner is owed. A grant by the same app over its own unfinished teardown takes the store back, runs in full, and voids the teardown's note that the bucket was deleted. A teardown that leaves a bucket to another account keeps its record and returns `ErrBucketRetained` (discussion 191). It can finish once that bucket has been destroyed, when someone runs it again; nothing runs it again by itself. For a bucket name this build would refuse there is no ledger: a store whose teardown has begun is not counted as a holder there, so the last of two sharers destroys the bucket. `ErrBucketRetained` has no member copy yet; it needs Paul's review. A randomized simulation (`walk_sim_test.go`) checks the invariants over sequential walks with provider and store faults, including failures after the effect, and walks frozen past their lease while another takes the name. It does not generate a second bucket name, rotations, stores with no ledger interval or disagreeing clocks; those have direct tests. |
| 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 fa8d2e65a..efe6a7bc5 100644 |
| --- a/implementation/carloku-paid-launch/OUTSTANDING.md |
| +++ b/implementation/carloku-paid-launch/OUTSTANDING.md |
| @@ -63,8 +63,8 @@ Each is concrete. None is waived. |
| ### Process |
| -19. **No review round has passed.** Astra rejected rounds one to eleven; Opus accepted each with fixes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings are 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 returns access to a bucket another account holds, through the member's own door. They are not fixed in `1395f7bf`. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| -19g. **Two store refusals tell the operator to run the grant again, which cannot work.** A record that names two buckets (`ErrTwoBuckets`) and a teardown note for a bucket that is not the record's (`ErrUnfinishedTeardown`) are refused before anything is touched, and the grant door answers "re-run the grant to resume". Neither state is written by this build; whether an earlier build left such records in production has not been checked. There is no operator verb to repair one, and correct copy needs approval (A). |
| +19. **No review round has passed.** Astra rejected rounds one to eleven; Opus accepted each with fixes. Round eleven found one P1 and two P2 in the store walks (a grant could move an older record to another bucket name; a waiting rotation ignored a grant that ran; the user and its policy were written before the grant was counted). Its fixes are on the launch branch (`1395f7bf`). Round twelve read them: every round-eleven finding is closed for its reported shape, Astra rejected again (one P1, two P2) and Opus accepted with fixes (one P2, three P3). The new findings were about records no walk of this build writes (a teardown note for another bucket, a record naming two buckets) being acted on by teardown, rotation and the scanning doors; a rotation started after a grant that failed at its last write; and a take-back that returned access to a bucket another account holds, through the member's own door. Their fixes are on the launch branch and unconfirmed until the next round reads them. Each round's fixes are on the launch branch, gated locally, and unconfirmed until the next round reads them. |
| +19g. **Refusals whose doors say the wrong thing, and records only a person can repair.** A record that names two buckets (`ErrTwoBuckets`) or whose teardown note is about another bucket (`ErrUnfinishedTeardown`) is refused by every path that acts on a store record, before anything is touched. No walk of this build writes either; whether an earlier build left such records in production has not been checked. There is no operator verb to repair one. The doors that meet these refusals answer with sentences written for other failures: the grant and provision doors say to run the grant again, the destroy door and the purge say to try again, the scanning doors say to try again or to re-run the grant. The same goes for a rotation refused because a grant must be run again first (`ErrGrantUnfinished`) and for a member's take-back refused because another account holds the bucket (`ErrBucketHeld`). Correct copy for each needs approval (A). |
| 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 3fae96628..223853695 100644 |
| --- a/internal/console/apiscan.go |
| +++ b/internal/console/apiscan.go |
| @@ -175,6 +175,13 @@ func (s *Server) writeScanRecord(ctx context.Context, acct *Account, app string, |
| if rec == nil { |
| return nil, objstore.ErrNotDeclared |
| } |
| + // A record no walk can follow is not changed by a scanning door |
| + // either: the re-grant that would carry the change to the backend |
| + // is refused for it, and a record edited on the way to a refusal |
| + // is one more thing for the person repairing it to undo. |
| + if err := objstore.Coherent(rec, acct.Sqid, app); err != nil { |
| + return nil, err |
| + } |
| if err := mutate(rec); err != nil { |
| return nil, err |
| } |
| @@ -340,8 +347,13 @@ func (s *Server) handleAPIScanGrant(w http.ResponseWriter, r *http.Request) { |
| } |
| // Enable on the backend BEFORE recording it. If the record said enabled |
| // and the backend was not, the sweep would run against a store nothing had |
| - // prepared; the other order merely repeats an idempotent enable. |
| - if err := s.cfg.StoreScanner.EnsureEnabled(ctx, rec.Provisioned.Bucket); err != nil { |
| + // prepared; the other order merely repeats an idempotent enable. A record |
| + // no walk can follow is refused before the backend is touched. |
| + err = objstore.Coherent(rec, acct.Sqid, app) |
| + if err == nil { |
| + err = s.cfg.StoreScanner.EnsureEnabled(ctx, rec.Provisioned.Bucket) |
| + } |
| + if err != nil { |
| log.Printf("console: api scan grant %s/%s: enable: %v", acct.Sqid, app, err) |
| jsonError(w, http.StatusBadGateway, "could not enable scanning on the store — try again") |
| return |
| @@ -396,7 +408,7 @@ func (s *Server) handleAPIScanRevoke(w http.ResponseWriter, r *http.Request) { |
| // EnsureDisabled failed must still clear the record, or the operator has |
| // no way to stop enforcement. |
| if rec, _, err := objstore.Get(ctx, s.cfg.Releases, acct.Sqid, app); err == nil && |
| - rec != nil && rec.Provisioned != nil && s.cfg.StoreScanner != nil { |
| + rec != nil && rec.Provisioned != nil && s.cfg.StoreScanner != nil && objstore.Coherent(rec, acct.Sqid, app) == nil { |
| if derr := s.cfg.StoreScanner.EnsureDisabled(ctx, rec.Provisioned.Bucket); derr != nil { |
| log.Printf("console: api scan revoke %s/%s: disable: %v", acct.Sqid, app, derr) |
| } |
diff --git a/internal/console/apiscan_test.go b/internal/console/apiscan_test.go| index 0dffa612f..d3706c70e 100644 |
| --- a/internal/console/apiscan_test.go |
| +++ b/internal/console/apiscan_test.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "errors" |
| "net/http" |
| + "reflect" |
| "strings" |
| "testing" |
| "time" |
| @@ -547,3 +548,48 @@ func TestScanStatusReportsTheDeploymentSwitch(t *testing.T) { |
| t.Fatal("the per-app record vanished from the reply") |
| } |
| } |
| + |
| +// A store record no walk can follow (it names two buckets, or its teardown |
| +// note is about another bucket) is not changed by a scanning door: the |
| +// re-grant that would carry the change to the backend is refused for it. |
| +func TestScanRecordWriterRefusesARecordNoWalkCanFollow(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + ctx := context.Background() |
| + acct, err := fx.st.AccountBySqid(fx.sqid) |
| + if err != nil || acct == nil { |
| + t.Fatal(err) |
| + } |
| + at := time.Date(2026, 9, 3, 10, 20, 0, 0, time.UTC) |
| + for name, c := range map[string]struct { |
| + rec *objstore.Record |
| + want error |
| + }{ |
| + "two buckets": {&objstore.Record{Declared: &objstore.Declared{EnvPrefix: "S3", At: at, By: "m@example.com"}, |
| + Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-y", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Bucket: "carlos-store-x", Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}}, objstore.ErrTwoBuckets}, |
| + "a teardown note about another bucket": {&objstore.Record{Declared: &objstore.Declared{EnvPrefix: "S3", At: at, By: "m@example.com"}, |
| + Grant: &objstore.GrantBlock{Backend: "aws", Bucket: "carlos-store-x", Source: objstore.GrantSourceOperator, At: at, By: "op"}, |
| + Provisioned: &objstore.Provisioned{Bucket: "carlos-store-x", Region: "eu-west-1", IAMUser: "u", KeyID: "K", At: at}, |
| + Teardown: &objstore.TeardownBlock{StartedAt: at, BucketDeletedAt: at, DeletedBucket: "carlos-store-y"}}, objstore.ErrUnfinishedTeardown}, |
| + } { |
| + app := "scanapp" |
| + _, etag, _ := objstore.Get(ctx, fx.rel, fx.sqid, app) |
| + if _, err := objstore.Put(ctx, fx.rel, fx.sqid, app, c.rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + before, _, _ := objstore.Get(ctx, fx.rel, fx.sqid, app) |
| + called := false |
| + _, err := fx.srv.writeScanRecord(ctx, acct, app, func(rec *objstore.Record) error { |
| + called = true |
| + rec.Scanning = &objstore.ScanBlock{} |
| + return nil |
| + }) |
| + if !errors.Is(err, c.want) || called { |
| + t.Errorf("%s: writeScanRecord = %v (mutated %v), want %v and no change", name, err, called, c.want) |
| + } |
| + after, _, _ := objstore.Get(ctx, fx.rel, fx.sqid, app) |
| + if !reflect.DeepEqual(before, after) { |
| + t.Errorf("%s: the record changed:\n%+v\n%+v", name, before, after) |
| + } |
| + } |
| +} |
diff --git a/internal/objstore/objstore.go b/internal/objstore/objstore.go| index 2d6639507..a7dbec728 100644 |
| --- a/internal/objstore/objstore.go |
| +++ b/internal/objstore/objstore.go |
| @@ -69,6 +69,14 @@ type GrantBlock struct { |
| // for its lease can tell that the store it was asked about is not the |
| // one standing now. |
| Walks int `json:"walks,omitempty"` |
| + // Delivering is set, in the same write that counts the walk, by a |
| + // grant that is about to reap the user's keys and mint and deliver a |
| + // new one, and cleared by the write that records the result. While it |
| + // stands the record may name a key that is gone and the app may hold |
| + // one the record does not name: a grant run again does not take the |
| + // healthy short-circuit, and a rotation is refused (ErrGrantUnfinished) |
| + // instead of reaping the key the app was given. |
| + Delivering bool `json:"delivering,omitempty"` |
| } |
| type Provisioned struct { |
diff --git a/internal/objstore/ownership.go b/internal/objstore/ownership.go| index fbf5aefbc..d191fb46e 100644 |
| --- a/internal/objstore/ownership.go |
| +++ b/internal/objstore/ownership.go |
| @@ -78,16 +78,24 @@ type OwnerRef struct { |
| Since time.Time `json:"since"` |
| } |
| -// ErrUnfinishedTeardown: a grant was asked for one bucket while the store's |
| -// record carries an unfinished teardown of another. Running the grant again |
| -// will be refused again; the way forward is to finish the teardown. |
| -var ErrUnfinishedTeardown = errors.New("objstore: unfinished teardown of another bucket") |
| +// ErrUnfinishedTeardown: the store's record carries a teardown note about |
| +// a bucket that is not the record's own. No walk writes one and none can |
| +// follow it: grant, teardown, rotation and the scanning paths all refuse |
| +// the record and leave it as found (walk.go Coherent). Running the walk |
| +// again is refused again; an operator repairs the record by hand. |
| +var ErrUnfinishedTeardown = errors.New("objstore: the record's teardown note is about another bucket") |
| + |
| +// ErrGrantUnfinished: a grant began delivering a key and did not record |
| +// the result (GrantBlock.Delivering). The key the record names may be |
| +// gone and the app may hold one the record does not name. A rotation is |
| +// refused until the grant has been run again, which repairs in full. |
| +var ErrGrantUnfinished = errors.New("objstore: an unfinished grant must be run again first") |
| // ErrTwoBuckets: the store's record names one bucket in its grant block and |
| // another in its provisioned block. A walk would have to pick one and leave |
| -// the other behind a record that no longer leads to it, so the walk is |
| -// refused before it touches anything and a person decides (walk.go |
| -// oneBucket). |
| +// the other behind a record that no longer leads to it, so every walk and |
| +// every scanning path refuses the record before touching anything, and an |
| +// operator repairs it by hand (walk.go Coherent). |
| var ErrTwoBuckets = errors.New("objstore: store record names two buckets") |
| // Refusals a walk gets from the ledger. Each is a state another walk put |
diff --git a/internal/objstore/ownership_test.go b/internal/objstore/ownership_test.go| index acfccf55d..e46af2e15 100644 |
| --- a/internal/objstore/ownership_test.go |
| +++ b/internal/objstore/ownership_test.go |
| @@ -12,6 +12,7 @@ import ( |
| "testing" |
| "time" |
| + "github.com/carlosframework/platform/internal/appconfig" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -2547,42 +2548,43 @@ func TestStaleTeardownDoesNotDestroyWhatAFailedGrantRetryMade(t *testing.T) { |
| // recorded on X's ledger and nowhere else, and the store is never moved to |
| // a name its record did not have. |
| func TestGrantOverATeardownKeepsTheRecordsOwnBucket(t *testing.T) { |
| - ctx := context.Background() |
| - s := release.NewDirStore(t.TempDir()) |
| - _, pub := testSealKeypair(t) |
| - p := NewMemProvisioner("eu-west-1") |
| - advance := leaseClock(t) |
| const legacyX = "carlos-store-legacy-x" |
| - // Y has a ledger: another account's store holds it. |
| - declare(t, s, "bac", "other", "OTHER_S3") |
| - if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, t0); err != nil { |
| - t.Fatal(err) |
| - } |
| - // The older record: provisioned on X, no grant block. |
| - user := UserName("bab", "titogo") |
| - if _, err := p.EnsureUser(ctx, user, "/carlos/", "arn:aws:s3:::"+legacyX, nil); err != nil { |
| - t.Fatal(err) |
| - } |
| - p.Buckets[legacyX] = map[string][]byte{} |
| - rec := &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, |
| - Provisioned: &Provisioned{Bucket: legacyX, Region: "eu-west-1", IAMUser: user, KeyID: "LEGACYKEY", At: t0}} |
| - if _, err := Put(ctx, s, "bab", "titogo", rec, ""); err != nil { |
| - t.Fatal(err) |
| - } |
| - p.FailNext = map[string]error{"DeleteUser": errors.New("injected DeleteUser")} |
| - if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| - t.Fatal("fixture: the teardown was meant to fail after deleting X") |
| - } |
| - p.FailNext = nil |
| - if _, exists := p.Buckets[legacyX]; exists { |
| - t.Fatal("fixture: X was not deleted") |
| - } |
| - if cur, _, _ := Get(ctx, s, "bab", "titogo"); cur == nil || cur.Teardown == nil || cur.Teardown.DeletedBucket != legacyX { |
| - t.Fatalf("fixture: the record does not say which bucket was deleted: %+v", cur) |
| - } |
| - advance(walkLease + time.Minute) |
| - // Whatever bucket the request names, minted or adopted. |
| + // Whatever bucket the request names, minted or adopted: each against a |
| + // record that has no grant block yet. |
| for _, adopt := range []string{"", ledgerBucket} { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + // Y has a ledger: another account's store holds it. |
| + declare(t, s, "bac", "other", "OTHER_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bac", "other", "op", GrantSourceOperator, ledgerBucket, t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The older record: provisioned on X, no grant block. |
| + user := UserName("bab", "titogo") |
| + if _, err := p.EnsureUser(ctx, user, "/carlos/", "arn:aws:s3:::"+legacyX, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.Buckets[legacyX] = map[string][]byte{} |
| + rec := &Record{Declared: &Declared{EnvPrefix: "TITO_S3", At: t0, By: "member@example.com"}, |
| + Provisioned: &Provisioned{Bucket: legacyX, Region: "eu-west-1", IAMUser: user, KeyID: "LEGACYKEY", At: t0}} |
| + if _, err := Put(ctx, s, "bab", "titogo", rec, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.FailNext = map[string]error{"DeleteUser": errors.New("injected DeleteUser")} |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); err == nil { |
| + t.Fatalf("adopt %q: fixture: the teardown was meant to fail after deleting X", adopt) |
| + } |
| + p.FailNext = nil |
| + if _, exists := p.Buckets[legacyX]; exists { |
| + t.Fatalf("adopt %q: fixture: X was not deleted", adopt) |
| + } |
| + if cur, _, _ := Get(ctx, s, "bab", "titogo"); cur == nil || cur.Grant != nil || cur.Teardown == nil || cur.Teardown.DeletedBucket != legacyX { |
| + t.Fatalf("adopt %q: fixture: want no grant block and a note that X was deleted: %+v", adopt, cur) |
| + } |
| + advance(walkLease + time.Minute) |
| got, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, adopt, t0.Add(time.Hour)) |
| if err != nil { |
| t.Fatalf("adopt %q: the take-back of X: %v", adopt, err) |
| @@ -2593,22 +2595,27 @@ func TestGrantOverATeardownKeepsTheRecordsOwnBucket(t *testing.T) { |
| if got.Grant.Minted { |
| t.Fatalf("adopt %q: a bucket the record already had was written down as minted by this grant", adopt) |
| } |
| - if got.Teardown != nil { |
| - t.Fatalf("adopt %q: the take-back left the teardown block: %+v", adopt, got.Teardown) |
| + if got.Teardown != nil || got.Grant.Delivering { |
| + t.Fatalf("adopt %q: the finished take-back left a teardown block or its delivering mark: %+v %+v", adopt, got.Teardown, got.Grant) |
| + } |
| + if _, exists := p.Buckets[legacyX]; !exists { |
| + t.Fatalf("adopt %q: the take-back did not bring X back", adopt) |
| + } |
| + ledger, _, lerr := ReadBucketOwnership(ctx, s, ledgerBucket) |
| + if lerr != nil || ledger == nil { |
| + t.Fatalf("adopt %q: Y's ledger: %v", adopt, lerr) |
| + } |
| + if ledger.DestroyedAt != nil { |
| + t.Fatalf("adopt %q: X's deletion was recorded on Y's ledger: %v", adopt, ledger.DestroyedAt) |
| + } |
| + for _, iv := range ledger.Intervals { |
| + if iv.Account == "bab" { |
| + t.Fatalf("adopt %q: the store was given an interval on Y: %+v", adopt, iv) |
| + } |
| + } |
| + if pol := p.Users[user].Policy; !strings.Contains(pol, legacyX) || strings.Contains(pol, ledgerBucket) { |
| + t.Fatalf("adopt %q: the user's policy reaches a bucket that is not the record's: %s", adopt, pol) |
| } |
| - } |
| - if _, exists := p.Buckets[legacyX]; !exists { |
| - t.Fatal("the take-back did not bring X back") |
| - } |
| - ledger, _, lerr := ReadBucketOwnership(ctx, s, ledgerBucket) |
| - if lerr != nil || ledger == nil { |
| - t.Fatalf("Y's ledger: %v", lerr) |
| - } |
| - if ledger.DestroyedAt != nil { |
| - t.Fatalf("X's deletion was recorded on Y's ledger: %v", ledger.DestroyedAt) |
| - } |
| - if pol := p.Users[user].Policy; !strings.Contains(pol, legacyX) || strings.Contains(pol, ledgerBucket) { |
| - t.Fatalf("the user's policy reaches a bucket that is not the record's: %s", pol) |
| } |
| } |
| @@ -2689,9 +2696,15 @@ func TestGrantIsRefusedOverATeardownNoteForAnotherBucket(t *testing.T) { |
| if !errors.Is(err, ErrUnfinishedTeardown) { |
| t.Fatalf("note %v: a grant for the minted name over a teardown of %s: %v, want ErrUnfinishedTeardown", withNote, elsewhere, err) |
| } |
| + // "Finish the teardown" is not the way out either: the teardown |
| + // would take the declaration-only path and delete the record, and |
| + // the bucket the note names would be forgotten. So is the purge. |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("note %v: a teardown of a record whose note names %s: %v, want ErrUnfinishedTeardown", withNote, elsewhere, err) |
| + } |
| cur, _, _ := Get(ctx, s, "bab", "titogo") |
| if cur == nil || cur.Grant != nil || cur.Teardown == nil || cur.Teardown.DeletedBucket != elsewhere || cur.Teardown.TakeBacks != 0 { |
| - t.Fatalf("note %v: the refused grant changed the record: %+v", withNote, cur) |
| + t.Fatalf("note %v: a refused walk changed or deleted the record: %+v", withNote, cur) |
| } |
| if len(p.Users) != 0 || len(p.Buckets) != 0 { |
| t.Fatalf("note %v: the refused grant reached the provider: users %d buckets %d", withNote, len(p.Users), len(p.Buckets)) |
| @@ -2857,16 +2870,42 @@ func TestWalksRefuseARecordThatNamesTwoBuckets(t *testing.T) { |
| } |
| before, _, _ := Get(ctx, s, "bab", "titogo") |
| policy := p.Users[user].Policy |
| + if _, _, err := p.MintKey(ctx, user); err != nil { |
| + t.Fatal(err) |
| + } |
| + keys := map[string]string{} |
| + for id, secret := range p.Users[user].Keys { |
| + keys[id] = secret |
| + } |
| if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); !errors.Is(err, ErrTwoBuckets) { |
| t.Fatalf("a grant over a record that names two buckets: %v, want ErrTwoBuckets", err) |
| } |
| if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrTwoBuckets) { |
| t.Fatalf("a teardown of a record that names two buckets: %v, want ErrTwoBuckets", err) |
| } |
| + if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", t0.Add(time.Hour)); !errors.Is(err, ErrTwoBuckets) { |
| + t.Fatalf("a rotation of a record that names two buckets: %v, want ErrTwoBuckets", err) |
| + } |
| + if _, err := RotateFinish(ctx, s, p, "bab", "titogo"); !errors.Is(err, ErrTwoBuckets) { |
| + t.Fatalf("a rotation finish of a record that names two buckets: %v, want ErrTwoBuckets", err) |
| + } |
| after, _, _ := Get(ctx, s, "bab", "titogo") |
| if !reflect.DeepEqual(before, after) { |
| t.Fatalf("a refused walk changed the record:\n%+v\n%+v", before, after) |
| } |
| + if !reflect.DeepEqual(p.Users[user].Keys, keys) { |
| + t.Fatalf("a refused walk changed the user's keys: %v, were %v", p.Users[user].Keys, keys) |
| + } |
| + // With scanning enabled, the sweep does not run on it either. |
| + scanning := *before |
| + scanning.Scanning = &ScanBlock{Enabled: &ScanEnabled{}} |
| + _, etag, _ := Get(ctx, s, "bab", "titogo") |
| + if _, err := Put(ctx, s, "bab", "titogo", &scanning, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + if side, err := RunSweep(ctx, s, nil, nil, "bab", "titogo", time.Second); !errors.Is(err, ErrTwoBuckets) || side != nil { |
| + t.Fatalf("a sweep of a record that names two buckets: %+v %v, want ErrTwoBuckets", side, err) |
| + } |
| if string(p.Buckets[legacyX]["old.pdf"]) != "in X" || string(p.Buckets[ledgerBucket]["new.pdf"]) != "in Y" { |
| t.Fatal("a refused walk destroyed objects") |
| } |
| @@ -2881,3 +2920,218 @@ func TestWalksRefuseARecordThatNamesTwoBuckets(t *testing.T) { |
| w.release(true) |
| } |
| } |
| + |
| +// A live bucket X, a record that names X in both blocks, and a teardown |
| +// note that says bucket Y was deleted. A teardown that took the note at its |
| +// word would skip X as already deleted, record Y's deletion on X's ledger, |
| +// and delete the record with X and its objects still there. Every walk |
| +// refuses the record and leaves it, the bucket and the ledger as found. |
| +func TestWalksRefuseATeardownNoteAboutAnotherBucket(t *testing.T) { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + leaseClock(t) |
| + declare(t, s, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + p.Buckets[ledgerBucket]["mine.pdf"] = []byte("this store's object") |
| + rec, etag, _ := Get(ctx, s, "bab", "titogo") |
| + rec.Teardown = &TeardownBlock{StartedAt: t0, BucketDeletedAt: t0.Add(time.Minute), DeletedBucket: "carlos-store-legacy-y"} |
| + if _, err := Put(ctx, s, "bab", "titogo", rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + before, _, _ := Get(ctx, s, "bab", "titogo") |
| + user := UserName("bab", "titogo") |
| + keys := map[string]string{} |
| + for id, secret := range p.Users[user].Keys { |
| + keys[id] = secret |
| + } |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("teardown: %v, want ErrUnfinishedTeardown", err) |
| + } |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("grant: %v, want ErrUnfinishedTeardown", err) |
| + } |
| + if _, err := Rotate(ctx, s, p, pub, "bab", "titogo", t0.Add(time.Hour)); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("rotate: %v, want ErrUnfinishedTeardown", err) |
| + } |
| + if _, err := RotateFinish(ctx, s, p, "bab", "titogo"); !errors.Is(err, ErrUnfinishedTeardown) { |
| + t.Fatalf("rotate finish: %v, want ErrUnfinishedTeardown", err) |
| + } |
| + after, _, _ := Get(ctx, s, "bab", "titogo") |
| + if !reflect.DeepEqual(before, after) { |
| + t.Fatalf("a refused walk changed the record:\n%+v\n%+v", before, after) |
| + } |
| + if string(p.Buckets[ledgerBucket]["mine.pdf"]) != "this store's object" { |
| + t.Fatal("a refused walk destroyed the store's object") |
| + } |
| + if !reflect.DeepEqual(p.Users[user].Keys, keys) { |
| + t.Fatalf("a refused walk changed the user's keys") |
| + } |
| + ledger := mustLedger(t, s, ledgerBucket) |
| + if ledger.DestroyedAt != nil || ledger.Destroying != nil { |
| + t.Fatalf("a refused walk wrote another bucket's deletion on this bucket's ledger: %+v", ledger) |
| + } |
| + if w, err := acquireBucketWalk(ctx, s, ledgerBucket, "bab", "second", "grant"); err != nil { |
| + t.Fatalf("a refused walk left the name reserved: %v", err) |
| + } else { |
| + w.release(true) |
| + } |
| +} |
| + |
| +// A grant repairs a store: it reaps the recorded key, mints and delivers a |
| +// new one, and fails at its last write. The record still names the old |
| +// key, and everything the app needs is delivered, so the store looks |
| +// healthy. A rotation started AFTER that (not one that was waiting) is |
| +// refused: it would reap the key the app was just given. The grant run |
| +// again does not take the healthy short-circuit; it repairs in full, and |
| +// the rotation then goes through. |
| +func TestRotationIsRefusedAfterAGrantThatFailedAtItsLastWrite(t *testing.T) { |
| + for _, verb := range []string{"rotate", "finish"} { |
| + ctx := context.Background() |
| + dir := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + declare(t, dir, "bab", "titogo", "TITO_S3") |
| + if _, err := Grant(ctx, dir, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0); err != nil { |
| + t.Fatal(err) |
| + } |
| + if verb == "finish" { |
| + if _, err := Rotate(ctx, dir, p, pub, "bab", "titogo", t0.Add(time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + user := UserName("bab", "titogo") |
| + // The repair: one delivered variable was clobbered, so the grant |
| + // walks again, with the same deployment. Its count is its first |
| + // write of the record and the result its second. |
| + if _, err := appconfig.Apply(ctx, dir, "bab", "titogo", |
| + map[string]appconfig.Var{"TITO_S3_SECRET_KEY": {Kind: "env", Value: "oops"}}, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + moved := testDeployment() |
| + failing := &failingPutStore{Store: dir, key: Key("bab", "titogo"), nth: 2} |
| + if _, err := Grant(ctx, failing, p, moved, pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(time.Hour)); err == nil || failing.seen < 2 { |
| + t.Fatalf("%s: fixture: the grant was meant to fail at its last write (err %v, writes %d)", verb, err, failing.seen) |
| + } |
| + cur, _, _ := Get(ctx, dir, "bab", "titogo") |
| + if cur == nil || cur.Grant == nil || !cur.Grant.Delivering { |
| + t.Fatalf("%s: the record does not say a grant began delivering and did not finish: %+v", verb, cur) |
| + } |
| + if _, recorded := p.Users[user].Keys[cur.Provisioned.KeyID]; recorded { |
| + t.Fatalf("%s: fixture: the recorded key was meant to be reaped by the failed grant", verb) |
| + } |
| + // To anything but the mark, the store looks healthy: the endpoint |
| + // is the deployment's and every variable is delivered. |
| + if missing, merr := EnvDelivered(ctx, dir, "bab", "titogo", "TITO_S3", false); merr != nil || len(missing) != 0 { |
| + t.Fatalf("%s: fixture: the failed grant was meant to have delivered everything: %v %v", verb, missing, merr) |
| + } |
| + keys := map[string]string{} |
| + for id, secret := range p.Users[user].Keys { |
| + keys[id] = secret |
| + } |
| + advance(walkLease + time.Minute) |
| + var err error |
| + if verb == "rotate" { |
| + _, err = Rotate(ctx, dir, p, pub, "bab", "titogo", t0.Add(2*time.Hour)) |
| + } else { |
| + _, err = RotateFinish(ctx, dir, p, "bab", "titogo") |
| + } |
| + if !errors.Is(err, ErrGrantUnfinished) { |
| + t.Fatalf("%s after a grant that failed at its last write: %v, want ErrGrantUnfinished", verb, err) |
| + } |
| + if !reflect.DeepEqual(p.Users[user].Keys, keys) { |
| + t.Fatalf("%s: the refused rotation changed the user's keys: %v, were %v", verb, p.Users[user].Keys, keys) |
| + } |
| + // The refusal reached no provider call: the name is free at once. |
| + if w, werr := acquireBucketWalk(ctx, dir, ledgerBucket, "bab", "second", "grant"); werr != nil { |
| + t.Fatalf("%s: the refused rotation left the name reserved: %v", verb, werr) |
| + } else { |
| + w.release(true) |
| + } |
| + // The grant again. Everything is delivered and the endpoint |
| + // matches, and it still walks in full: the record must come to |
| + // name a key the user has. |
| + repaired, err := Grant(ctx, dir, p, moved, pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(3*time.Hour)) |
| + if err != nil { |
| + t.Fatalf("%s: the grant run again: %v", verb, err) |
| + } |
| + if repaired.Grant.Delivering { |
| + t.Fatalf("%s: the finished grant left its delivering mark", verb) |
| + } |
| + if _, live := p.Users[user].Keys[repaired.Provisioned.KeyID]; !live { |
| + t.Fatalf("%s: the repaired record names a key the user does not have: %s of %v", verb, repaired.Provisioned.KeyID, p.Users[user].Keys) |
| + } |
| + if _, err := Rotate(ctx, dir, p, pub, "bab", "titogo", t0.Add(4*time.Hour)); err != nil { |
| + t.Fatalf("%s: a rotation after the repair: %v", verb, err) |
| + } |
| + } |
| +} |
| + |
| +// A store left a bucket to another account's store: its teardown revoked |
| +// its own access and reported the bucket retained. The member's own door |
| +// (an account-approved grant) must not give that access back by itself; |
| +// what the other account has written since is not this app's to read. An |
| +// operator's grant can. |
| +func TestMembersTakeBackIsRefusedWhileAnotherAccountHoldsTheBucket(t *testing.T) { |
| + for _, how := range []string{"by its record", "by a ledger interval"} { |
| + ctx := context.Background() |
| + s := release.NewDirStore(t.TempDir()) |
| + _, pub := testSealKeypair(t) |
| + p := NewMemProvisioner("eu-west-1") |
| + advance := leaseClock(t) |
| + const sharedX = "carlos-store-legacy-x" |
| + p.Buckets[sharedX] = map[string][]byte{"mine.pdf": []byte("this store's object")} |
| + for _, o := range []struct{ acct, app string }{{"bab", "titogo"}, {"bac", "other"}} { |
| + user := UserName(o.acct, o.app) |
| + if _, err := p.EnsureUser(ctx, user, "/carlos/", "arn:aws:s3:::"+sharedX, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + rec := &Record{Declared: &Declared{EnvPrefix: "S3", At: t0, By: "member@example.com"}, |
| + Provisioned: &Provisioned{Bucket: sharedX, Region: "eu-west-1", IAMUser: user, KeyID: "K" + o.acct, At: t0}} |
| + if _, err := Put(ctx, s, o.acct, o.app, rec, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if err := Teardown(ctx, s, p, nil, "bab", "titogo"); !errors.Is(err, ErrBucketRetained) { |
| + t.Fatalf("%s: fixture: the teardown that leaves X to another account: %v, want ErrBucketRetained", how, err) |
| + } |
| + if how == "by a ledger interval" { |
| + // The other account's hold is on the ledger and its record is |
| + // gone from view (as for a store whose record cannot be listed). |
| + if err := OpenBucketOwner(ctx, s, sharedX, "bac", "other", t0, t0, false); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, etag, _ := Get(ctx, s, "bac", "other") |
| + if err := release.DeleteIfMatch(ctx, s, Key("bac", "other"), etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + p.Buckets[sharedX]["theirs.pdf"] = []byte("written by the other account after this store left") |
| + before, _, _ := Get(ctx, s, "bab", "titogo") |
| + advance(time.Hour) |
| + _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "member@example.com", GrantSourceAccountApproval, "", t0.Add(time.Hour)) |
| + if !errors.Is(err, ErrBucketHeld) { |
| + t.Fatalf("%s: the member's take-back: %v, want ErrBucketHeld", how, err) |
| + } |
| + if u, ok := p.Users[UserName("bab", "titogo")]; ok { |
| + t.Fatalf("%s: the refused take-back made a user: %+v", how, u) |
| + } |
| + after, _, _ := Get(ctx, s, "bab", "titogo") |
| + if !reflect.DeepEqual(before, after) { |
| + t.Fatalf("%s: the refused take-back changed the record:\n%+v\n%+v", how, before, after) |
| + } |
| + if w, werr := acquireBucketWalk(ctx, s, sharedX, "bab", "second", "grant"); werr != nil { |
| + t.Fatalf("%s: the refused take-back left the name reserved: %v", how, werr) |
| + } else { |
| + w.release(true) |
| + } |
| + // An operator's grant is the door that can. |
| + if _, err := Grant(ctx, s, p, testDeployment(), pub, "bab", "titogo", "op", GrantSourceOperator, "", t0.Add(2*time.Hour)); err != nil { |
| + t.Fatalf("%s: an operator's take-back: %v", how, err) |
| + } |
| + } |
| +} |
diff --git a/internal/objstore/sweep.go b/internal/objstore/sweep.go| index 0af7fa4a2..6fd0be942 100644 |
| --- a/internal/objstore/sweep.go |
| +++ b/internal/objstore/sweep.go |
| @@ -132,6 +132,12 @@ func RunSweep(ctx context.Context, s release.Store, lister ObjectLister, sc Scan |
| if rec == nil || rec.Provisioned == nil || rec.Scanning == nil || rec.Scanning.Enabled == nil { |
| return nil, nil |
| } |
| + // A record no walk can follow is not swept: the bucket it is |
| + // provisioned on may not be the one the app uses, and coverage |
| + // published for it would be a claim about the wrong objects. |
| + if err := Coherent(rec, acct, app); err != nil { |
| + return nil, err |
| + } |
| // A trashed store is destroyed within 30 days. Scanning it bills for |
| // egress on data that is on its way out, and reports coverage for an app |
diff --git a/internal/objstore/walk.go b/internal/objstore/walk.go| index 08d0aa7d1..e2141a3f1 100644 |
| --- a/internal/objstore/walk.go |
| +++ b/internal/objstore/walk.go |
| @@ -156,7 +156,7 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| if rec == nil || rec.Declared == nil { |
| return nil, ErrNotDeclared |
| } |
| - if err := oneBucket(rec, acct, app); err != nil { |
| + if err := Coherent(rec, acct, app); err != nil { |
| return nil, err |
| } |
| // A grant over an unfinished teardown takes the store back. The record |
| @@ -266,6 +266,36 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| deletedBucket = tornBucket |
| } |
| } |
| + // A take-back gives this app access to the bucket again. When a store |
| + // of ANOTHER account holds the bucket (only an operator can have |
| + // arranged that), the teardown left the bucket to it and reported it |
| + // retained: what that account has written since is not this app's to |
| + // read. The member's own doors (account approval) must not undo that |
| + // by themselves; an operator's grant can. Checked before anything is |
| + // written, from the store records and the ledger, as a minting grant |
| + // checks them. |
| + if rec.Teardown != nil && source != GrantSourceOperator { |
| + holders, herr := otherStoreHolders(ctx, s, bucket, acct, app) |
| + if herr != nil { |
| + return nil, fmt.Errorf("objstore: list the stores holding %s: %w", bucket, herr) |
| + } |
| + ledger, _, lerr := ReadBucketOwnership(ctx, s, bucket) |
| + if lerr != nil { |
| + return nil, fmt.Errorf("objstore: read ownership of %s: %w", bucket, lerr) |
| + } |
| + if ledger != nil { |
| + for _, iv := range ledger.Intervals { |
| + if iv.open() && iv.Account != acct { |
| + holders = append(holders, OwnerRef{Account: iv.Account, App: iv.App, Since: iv.From}) |
| + } |
| + } |
| + } |
| + for _, h := range holders { |
| + if h.Account != acct { |
| + return nil, fmt.Errorf("%w: %s by %s/%s of another account; only an operator's grant gives %s/%s access to it again", ErrBucketHeld, bucket, h.Account, h.App, acct, app) |
| + } |
| + } |
| + } |
| wroteGrant := rec.Grant == nil |
| if wroteGrant { |
| rec.Grant = &GrantBlock{Backend: d.Backend, Bucket: bucket, Source: source, At: now, By: actor, Minted: adoptBucket == "" && rec.Provisioned == nil} |
| @@ -421,14 +451,42 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| // The console's declare door writes this same object. If it wrote in |
| // between, the count is put on the record as it now stands, provided |
| // it is still the same estate; anything else is a conflict. |
| + // |
| + // Whether the store is healthy is decided first, from reads alone: |
| + // "healthy" means the record says provisioned with THIS deployment's |
| + // endpoint AND every delivered var is present, of the right kind, in |
| + // config.json, AND no earlier grant is marked as having begun to |
| + // deliver without recording the result. Anything less and the walk |
| + // runs, because a missing var means the app cannot reach its store, |
| + // and an endpoint the deployment has since changed or dropped means it |
| + // is reaching for the wrong one, which only a re-walk (which rewrites |
| + // the set, and unsets _ENDPOINT when there is no longer an endpoint) |
| + // can put right. |
| + // |
| + // A grant that will run the walk says so in the same write that counts |
| + // it (GrantBlock.Delivering), and the write that records the result |
| + // takes the mark off. A grant that reaps the recorded key, delivers a |
| + // new one and fails at its last write therefore leaves a record that |
| + // says exactly that: the next grant does not mistake it for healthy, |
| + // and a rotation does not reap the key the app was given. |
| + healthy := false |
| + if !rebuilt && rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint && !rec.Grant.Delivering { |
| + missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, d.Endpoint != "") |
| + if err != nil { |
| + return nil, err |
| + } |
| + healthy = len(missing) == 0 |
| + } |
| before := estateOf(rec) |
| rec.Grant.Walks++ |
| + rec.Grant.Delivering = !healthy |
| if etag, err = Put(ctx, s, acct, app, rec, etag); errors.Is(err, release.ErrCASConflict) { |
| fresh, freshEtag, gerr := Get(ctx, s, acct, app) |
| if gerr != nil || fresh == nil || fresh.Declared == nil || fresh.Grant == nil || estateOf(fresh) != before { |
| return nil, err |
| } |
| fresh.Grant.Walks++ |
| + fresh.Grant.Delivering = fresh.Grant.Delivering || !healthy |
| if etag, err = Put(ctx, s, acct, app, fresh, freshEtag); err != nil { |
| return nil, err |
| } |
| @@ -462,21 +520,10 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| if err != nil { |
| return nil, fmt.Errorf("objstore: ensure user %s: %w", user, err) |
| } |
| - // Idempotence short-circuit: "healthy" means the record says provisioned |
| - // with THIS deployment's endpoint AND every delivered var is present, of |
| - // the right kind, in config.json. Anything less and the walk runs, because |
| - // a missing var means the app cannot reach its store — and an endpoint the |
| - // deployment has since changed or dropped means it is reaching for the |
| - // wrong one, which only a re-walk (which rewrites the set, and unsets |
| - // _ENDPOINT when there is no longer an endpoint) can put right. |
| - if !rebuilt && rec.Provisioned != nil && rec.Provisioned.Endpoint == d.Endpoint { |
| - missing, err := EnvDelivered(ctx, s, acct, app, rec.Declared.EnvPrefix, d.Endpoint != "") |
| - if err != nil { |
| - return nil, err |
| - } |
| - if len(missing) == 0 { |
| - return rec, nil |
| - } |
| + // Idempotence short-circuit, decided before the count above. EnsureUser |
| + // has run either way: it is the only place the policy is written. |
| + if healthy { |
| + return rec, nil |
| } |
| // 3. the bucket |
| region, err := p.EnsureBucket(ctx, bucket) |
| @@ -514,6 +561,7 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| rec.Provisioned = &Provisioned{Bucket: bucket, Region: region, Endpoint: d.Endpoint, |
| IAMUser: user, UserARN: arn, KeyID: id, PrevKeyID: prev, At: now} |
| rec.Teardown = nil // the store is whole again |
| + rec.Grant.Delivering = false |
| if _, err := Put(ctx, s, acct, app, rec, etag); err != nil { |
| if !errors.Is(err, release.ErrCASConflict) { |
| return nil, err |
| @@ -549,6 +597,9 @@ func Grant(ctx context.Context, s release.Store, p Provisioner, d Deployment, |
| } |
| fresh.Provisioned = rec.Provisioned |
| fresh.Teardown = nil |
| + if fresh.Grant != nil { |
| + fresh.Grant.Delivering = false |
| + } |
| if _, perr := Put(ctx, s, acct, app, fresh, freshEtag); perr != nil { |
| return nil, perr |
| } |
| @@ -610,6 +661,9 @@ func Rotate(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| if rec.Declared == nil { |
| return nil, ErrNotDeclared // no prefix to deliver into |
| } |
| + if err := Coherent(rec, acct, app); err != nil { |
| + return nil, err |
| + } |
| // A rotation deletes and mints keys of a user whose name is the same |
| // for every store this app will ever have. It is a walk like a grant |
| // or a teardown: it runs under the bucket's lease and inside its fence, |
| @@ -693,6 +747,9 @@ func RotateFinish(ctx context.Context, s release.Store, p Provisioner, acct, app |
| if rec == nil || rec.Provisioned == nil { |
| return nil, ErrNotProvisioned |
| } |
| + if err := Coherent(rec, acct, app); err != nil { |
| + return nil, err |
| + } |
| ctx, rec, etag, done, err := leaseForRotation(ctx, s, acct, app, rec) |
| if err != nil { |
| return nil, err |
| @@ -746,6 +803,13 @@ func leaseForRotation(ctx context.Context, s release.Store, acct, app string, fi |
| err = ErrNotProvisioned |
| case rec.Teardown != nil || estateOf(rec) != asked: |
| err = fmt.Errorf("objstore: store record for %s/%s changed while its rotation was starting; run it again: %w", acct, app, release.ErrCASConflict) |
| + case rec.Grant != nil && rec.Grant.Delivering: |
| + // A grant reaped the recorded key, may have delivered another, and |
| + // did not record it. Rotating now would reap the key the app was |
| + // given. The grant is run again first: it repairs in full. |
| + err = fmt.Errorf("%w: %s/%s", ErrGrantUnfinished, acct, app) |
| + default: |
| + err = Coherent(rec, acct, app) |
| } |
| if err != nil { |
| // Nothing was called under the lease: it is given back clean. |
| @@ -792,8 +856,12 @@ func Teardown(ctx context.Context, s release.Store, p Provisioner, sc Scanner, a |
| // The record is deleted last because it is the only thing naming the |
| // estate. A record that names two buckets cannot be followed to both: |
| // this walk would destroy one, delete the record, and leave the other |
| - // with nothing naming it. |
| - if err := oneBucket(rec, acct, app); err != nil { |
| + // with nothing naming it. A teardown note about a bucket that is not |
| + // the record's would have this walk skip the record's bucket as |
| + // already deleted. Both are refused here, before the declaration-only |
| + // delete as well: a record that carries such a note is not "declared, |
| + // never acted on". |
| + if err := Coherent(rec, acct, app); err != nil { |
| return err |
| } |
| // What this walk believes it is destroying — the guard on the final delete. |
| @@ -1219,7 +1287,7 @@ 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, ErrUnfinishedTeardown) || errors.Is(err, ErrTwoBuckets) || errors.Is(err, ErrGrantUnfinished) |
| } |
| // grantElapsed is how long a grant has been running. A seam, so a test can |
| @@ -1408,16 +1476,49 @@ func writeEnv(ctx context.Context, s release.Store, sealPub *ecdh.PublicKey, |
| return nil |
| } |
| -// oneBucket refuses a record whose grant block and provisioned block name |
| -// different buckets. No walk in this build writes one: a grant's bucket is |
| -// the record's provisioned bucket when it has one (bucketFor in Grant). An |
| -// earlier build could, by granting the minted name over an older record |
| -// that was provisioned elsewhere and had no grant block. Which of the two |
| -// holds the store's objects, and which the user's policy reaches, cannot be |
| -// told from the record, so neither Grant nor Teardown guesses. |
| -func oneBucket(rec *Record, acct, app string) error { |
| - if rec.Grant == nil || rec.Provisioned == nil || rec.Grant.Bucket == "" || rec.Provisioned.Bucket == "" || rec.Grant.Bucket == rec.Provisioned.Bucket { |
| +// Coherent refuses a record that no walk of this build writes and that no |
| +// walk can follow. Every path that acts on a store record, at the provider |
| +// or on the record itself, checks it first: Grant, Teardown (before its |
| +// declaration-only delete too), both rotation verbs, the scanning doors |
| +// and the sweep. A refused record is left exactly as found; an operator |
| +// repairs it by hand. |
| +// |
| +// Two shapes: |
| +// |
| +// - the grant block and the provisioned block name different buckets |
| +// (ErrTwoBuckets). A grant's bucket is the record's provisioned bucket |
| +// when it has one (bucketFor in Grant), so this build does not write |
| +// it; an earlier build could, by granting the minted name over an |
| +// older record provisioned elsewhere. Which of the two holds the |
| +// store's objects, and which the user's policy reaches, cannot be told |
| +// from the record. |
| +// - the teardown block notes a bucket that is not the record's own, or |
| +// notes a deletion on a record that names no bucket at all |
| +// (ErrUnfinishedTeardown). Teardown notes the bucket it works from, |
| +// which is the record's. A note about another bucket is evidence about |
| +// that bucket: a teardown that took it at its word would skip the |
| +// record's own bucket as already deleted and then delete the record. |
| +func Coherent(rec *Record, acct, app string) error { |
| + if rec == nil { |
| return nil |
| } |
| - return fmt.Errorf("%w: %s/%s is provisioned on %s and its grant names %s", ErrTwoBuckets, acct, app, rec.Provisioned.Bucket, rec.Grant.Bucket) |
| + 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) |
| + } |
| + if rec.Teardown != nil { |
| + own := "" |
| + switch { |
| + case rec.Provisioned != nil && rec.Provisioned.Bucket != "": |
| + own = rec.Provisioned.Bucket |
| + case rec.Grant != nil: |
| + own = rec.Grant.Bucket |
| + } |
| + switch noted := rec.Teardown.DeletedBucket; { |
| + case noted != "" && noted != own: |
| + return fmt.Errorf("%w: the teardown block of %s/%s notes bucket %s and the record's bucket is %q", ErrUnfinishedTeardown, acct, app, noted, own) |
| + case own == "" && !rec.Teardown.BucketDeletedAt.IsZero(): |
| + return fmt.Errorf("%w: the teardown block of %s/%s notes a deleted bucket and the record names none", ErrUnfinishedTeardown, acct, app) |
| + } |
| + } |
| + return nil |
| } |
diff --git a/internal/release/trash.go b/internal/release/trash.go| index b19a52236..526e6f199 100644 |
| --- a/internal/release/trash.go |
| +++ b/internal/release/trash.go |
| @@ -298,11 +298,15 @@ func purgeStoreDeclaration(ctx context.Context, s Store, acct, app, key string) |
| var rec struct { |
| Grant *json.RawMessage `json:"grant"` |
| Provisioned *json.RawMessage `json:"provisioned"` |
| + Teardown *json.RawMessage `json:"teardown"` |
| } |
| if jerr := json.Unmarshal(raw, &rec); jerr != nil { |
| return fmt.Errorf("release: %s/%s: unparseable %s — refusing to purge: %w: %v", acct, app, key, ErrStoreProvisioned, jerr) |
| } |
| - if rec.Provisioned != nil || rec.Grant != nil { |
| + // A teardown block counts too. A finished teardown deletes the record, |
| + // so a record still carrying one is an estate somebody began to take |
| + // apart, and its note may be the only thing naming a bucket. |
| + if rec.Provisioned != nil || rec.Grant != nil || rec.Teardown != nil { |
| return fmt.Errorf("%s/%s: %w", acct, app, ErrStoreProvisioned) |
| } |
| if err := DeleteIfMatch(ctx, s, key, etag); err != nil { |
diff --git a/internal/release/trash_test.go b/internal/release/trash_test.go| index 278940465..071babed0 100644 |
| --- a/internal/release/trash_test.go |
| +++ b/internal/release/trash_test.go |
| @@ -379,6 +379,9 @@ func TestPurgeAppRefusesProvisionedOrGrantedStore(t *testing.T) { |
| // A grant block with NO provisioned block: the earliest bucket-visible |
| // witness that backend estate may already exist. |
| {"grant-only", `{"schema":1,"app":"derp","declared":{"env_prefix":"CARLOS_STORE"},"grant":{"backend":"aws","by":"paul@keymail.dev"}}`}, |
| + // A teardown block and nothing else: somebody began to take an |
| + // estate apart, and the block's note may be all that names a bucket. |
| + {"teardown-only", `{"schema":1,"app":"derp","declared":{"env_prefix":"CARLOS_STORE"},"teardown":{"started_at":"2026-09-03T10:20:00Z","deleted_bucket":"carlos-store-legacy-x"}}`}, |
| // Unreadable is not "no store" — same direction as the production flag. |
| {"unparseable", `{"schema":1,`}, |
| } { |