amadan
spec: revise self-serve storage design after review (#381)
Three blockers, all verified in code: Grant writes its grant block at step 1, before any backend call, so a failed walk leaves grant-without-provisioned. The draft's "skip if already granted" condition would have skipped the retry and put the member back in the operator queue the design exists to remove. The walk is idempotent and short-circuits itself; the door must not second-guess it. apiAccountApp resolves the account against the CALLER's memberships, so approve/decline behind it would require every operator to join every account they approve. apimail.go already documents this deadlock for the pause door; approval takes that shape instead — operator bit, then the env list, then a viewer-independent AccountBySqid. walk.go claims the console serialises store walks per app. It does not — there is no such lock. Today that needs two simultaneous grants; making create drive walks makes it ordinary, and the losing walk can leave the record naming a key the next rotation reaps out from under a running app. A keyed mutex and a Rotate-style CAS retry on Grant land first, as their own commits, because both are fixes to existing behaviour. Also from review: the entitlement moves out of accounts/ (that prefix is in the fleet read ACL, and decline reasons are operator commentary about a tenant); create becomes one CLI command over two doors, following email enable's actual client-side shape; the grant walk needs the 60s timeout, not the 15s one; and two printed sentences that promise an operator grant become false in an approved account.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+382
−266
docs/superpowers/specs/2026-09-02-object-storage-self-serve-design.md+382 −266
diff --git a/docs/superpowers/specs/2026-09-02-object-storage-self-serve-design.md b/docs/superpowers/specs/2026-09-02-object-storage-self-serve-design.md| index 572dd5c71..0c1a5d262 100644 |
| --- a/docs/superpowers/specs/2026-09-02-object-storage-self-serve-design.md |
| +++ b/docs/superpowers/specs/2026-09-02-object-storage-self-serve-design.md |
| @@ -1,6 +1,6 @@ |
| # Self-serve object storage: an account applies, an operator approves |
| -**Date:** 2026-09-02 |
| +**Date:** 2026-09-02 (revised 2026-09-03 after review) |
| **Status:** approved in principle (Paul, 2026-09-02, in the brainstorm this |
| document records). Four decisions were taken there and are marked **(ruling)**; |
| everything else is marked **(decision)** and can be overturned cheaply. |
| @@ -9,6 +9,12 @@ everything else is marked **(decision)** and can be overturned cheaply. |
| `2026-08-10-object-storage-primitive-design.md`, referenced below as *the |
| primitive spec*). |
| +**Revision note.** A review on 2026-09-03 found three blockers in the first |
| +draft, all verified in code. §7 described a failure state the walk does not |
| +produce; §5 inherited a membership requirement that would deadlock approval; |
| +and §9 turned a latent concurrency bug into an everyday one. All three are |
| +fixed below, and §9 now carries work this spec did not originally budget for. |
| + |
| ## 1. The problem, as found |
| On 2026-09-02 James reported that carloku had no object storage. He was right, |
| @@ -26,8 +32,7 @@ act on neither: |
| 1. **The deployment had no backend at all.** `CARLOS_STORE_BACKEND`, plus the |
| `CARLOS_STORE_REGION` and `CARLOS_STORE_BUCKET_PREFIX` that |
| `storeBackend()` makes fatal at boot if the first is set without them |
| - (`cmd/console/main.go:492-524`), live only in the console's root env. Turning |
| - object storage on for a whole deployment is an SSM edit and a restart. That |
| + (`cmd/console/main.go:492-524`), live only in the console's root env. That |
| is #368's problem in its most expensive instance: the primitive has shipped |
| since #55 and the flagship has never had it switched on, because switching it |
| on is not a `carlos` verb. |
| @@ -36,83 +41,89 @@ act on neither: |
| <a>`, once, per app, forever. |
| This spec fixes (2) and makes (1) visible. (1)'s actual fix is #368 and is a |
| -non-goal here (§11). |
| +non-goal here (§12). |
| ### What is already right, and must stay right |
| -Worth stating plainly, because the temptation is to rebuild things that work. |
| - |
| -**The record's shape is right.** Three blocks for three actors — the member |
| -declares, an operator grants, the console's walk records what it provisioned — |
| -is exactly the tri-state a waiting member needs, and `null`-or-present is what |
| -makes "declared, waiting" distinguishable from "provisioned" |
| +**The record's shape is right.** Three blocks for three actors, `null`-or-present, |
| +is what makes "declared, waiting" distinguishable from "provisioned" |
| (`internal/objstore/objstore.go`, `Record`'s doc comment). |
| **The status door already answers the right question.** `backend` is in the |
| reply *specifically* so a member can tell "queued behind an operator" from |
| "this deployment has no object storage and never will" |
| -(`internal/console/apistore.go:105-112`). That reasoning is this spec's |
| -reasoning, one level up. |
| +(`internal/console/apistore.go:105-112`). That is this spec's reasoning, one |
| +level up. |
| -**The metering is already built, and this spec adds none.** The economics pass |
| -walks every app's store record daily, sizes the provisioned bucket |
| +**The metering is already built, and this spec adds almost none.** The economics |
| +pass walks every app's store record daily, sizes the provisioned bucket |
| (`sizeProvisionedStore`, `internal/console/economicspass.go:990`), records |
| -`StoreBytes` per app and distinguishes "could not measure" from "zero" with |
| +`StoreBytes` and distinguishes "could not measure" from "zero" with |
| `StoreUnswept`. That rolls into `MeterStorageGBMonth` on |
| `usage/accounts/<acct>/<month>.json`, which `PriceAccount` already prices at |
| `StorageGBMonthUSD` against a free tier defaulting to 1 GB |
| -(`internal/economics/scenario.go:162`). **"Unlimited but metered" is therefore a |
| -statement about what already happens, not work to be done.** When the credit |
| -card arrives it reads a statement that has been written every day since. |
| +(`internal/economics/scenario.go:162`). **"Unlimited but metered" is a |
| +statement about what already happens.** When the credit card arrives it reads a |
| +statement written every day since. (§11 adds one additive boolean to it.) |
| -**The money gate is right in principle.** `grant` creates a bucket, an IAM |
| -user, an inline policy and an access key in the *deployment's* cloud account. |
| -`internal/console/apistore.go:14` calls it the deployment's first explicit |
| -operator-identity gate. Nothing here removes that gate. It moves the decision |
| -it guards from per-app to per-account, so it is made deliberately once instead |
| -of being a queue nobody is watching. |
| +**The money gate is right in principle.** `grant` creates a bucket, an IAM user, |
| +an inline policy and an access key in the *deployment's* cloud account. |
| +`apistore.go:14` calls it the deployment's first explicit operator-identity |
| +gate. Nothing here removes that gate; it moves the decision it guards from |
| +per-app to per-account, so it is made deliberately once instead of being a |
| +queue nobody is watching. |
| ## 2. The shape, in one paragraph |
| An account applies for object storage. An operator approves the account. From |
| then on, any app in that account gets a store from `carlos store create` alone |
| -— the walk runs inline and the member never learns the word "grant". The |
| -per-app operator `grant` verb stays exactly as it is, for unapproved accounts, |
| -for `--bucket` adoption, and for repair. |
| +— one member command — and never learns the word "grant". The per-app operator |
| +`grant` verb stays exactly as it is, for unapproved accounts, for `--bucket` |
| +adoption, and for repair. |
| ## 3. Rulings taken in the brainstorm |
| -- **(ruling)** The entitlement is **per account**, not per app. The code |
| - already agrees: `MintedBucket` is `d.BucketPrefix + acct + "-" + app` |
| - (`internal/objstore/walk.go:62`) and `UserName` is |
| - `"carlos-store-" + acct + "-" + app` (`walk.go:70`), so every bucket and |
| - every principal this deployment mints is already named into its account. |
| +- **(ruling)** The entitlement is **per account**, not per app. `MintedBucket` |
| + is `d.BucketPrefix + acct + "-" + app` (`internal/objstore/walk.go:62`) and |
| + `UserName` is `"carlos-store-" + acct + "-" + app` (`walk.go:70`), so every |
| + bucket and principal is already named into its account. (§10 records where |
| + that stops being an invariant.) |
| - **(ruling)** Approval entitles an account to **unlimited stores, metered**. |
| - No quota, no bucket cap. §10 records the risk this accepts. |
| -- **(ruling)** **Account owners only** may apply. Applying is an account-level |
| - commitment and will later be a billing one, so it sits with the authority |
| - that already gates transfer, delete and destroy (`RoleOwner`, |
| - `internal/console/store.go:573`). |
| -- **(ruling)** In an approved account, `carlos store create` **provisions |
| - inline** — one command, no second step. This mirrors `carlos email enable`, |
| - which already declares, ensures and delivers in one verb. |
| - |
| -## 4. The entitlement record: `accounts/<sqid>/store.json` |
| - |
| -A new object, sibling to `accounts/<sqid>/account.json` and parallel in spirit |
| -to the per-app `apps/<acct>/<app>/store.json`. |
| + No quota, no bucket cap. §11 records the risk this accepts. |
| +- **(ruling)** **Account owners only** may apply (`RoleOwner`, |
| + `internal/console/store.go:573`; enforced by the existing `apiOwnerAccount`, |
| + `apiservices.go:103-112`). |
| +- **(ruling)** In an approved account, **`carlos store create` is the whole |
| + member experience** — one command, no second verb, no waiting. §6 records |
| + that this is one CLI command over two doors, which is an implementation |
| + detail of the ruling, not a departure from it. |
| + |
| +## 4. The entitlement record |
| + |
| +**(decision) Location: `store/entitlements/<sqid>.json`, deliberately NOT under |
| +`accounts/`.** The obvious home was `accounts/<sqid>/store.json`, and the |
| +review caught why it is wrong: `accounts/<sqid>/**` is inside the fleet READ |
| +ACL (`internal/edge/storedoor.go:358-362, 505-510`), so every box credential of |
| +that account could read the applicant's note, the operator's address and the |
| +decline reason. Decline reasons are operator commentary about a tenant; they |
| +must not be tenant-readable. A top-level prefix keeps it console-only. |
| + |
| +> **Implementation gate:** before the record lands, confirm by test that |
| +> `store/entitlements/` is unreachable through the edge store door — read |
| +> `canonicalKey`/`aclRead` rather than trusting the prefix's shape. The review |
| +> flagged this as suspected-not-confirmed (S3). |
| ```go |
| const EntitlementSchema = 1 |
| -// Entitlement is accounts/<sqid>/store.json — two blocks for two actors |
| +// Entitlement is store/entitlements/<sqid>.json — two blocks for two actors |
| // (an owner applies, an operator decides), the account-level twin of Record's |
| // three-for-three. |
| type Entitlement struct { |
| - Schema int `json:"schema"` |
| - Account string `json:"account"` |
| - Applied *Applied `json:"applied,omitempty"` |
| - Decision *Decision `json:"decision,omitempty"` |
| + Schema int `json:"schema"` |
| + Account string `json:"account"` |
| + Applied *Applied `json:"applied,omitempty"` |
| + Decision *Decision `json:"decision,omitempty"` |
| } |
| type Applied struct { |
| @@ -131,127 +142,168 @@ type Decision struct { |
| **(decision) State is derived, never stored.** No object = never applied. |
| `applied` with no `decision` = pending. `decision` present = decided, and |
| -`Approved` says which way. This is the same null-or-present tri-state |
| -`Record` uses, and for the same reason: a `state string` field can disagree |
| -with the blocks beside it, and then two readers disagree about the account. |
| +`Approved` says which way. Same reasoning as `Record`: a `state string` can |
| +disagree with the blocks beside it, and then two readers disagree about the |
| +account. |
| -**(decision) A declined owner may re-apply.** `apply` overwrites `applied` and |
| -clears `decision`, under CAS. Circumstances change and a permanent no is a |
| -support ticket. The anti-spam control is the one already in the codebase — the |
| -shared `write` budget keyed on the address, exactly as `handleAPIStoreCreate` |
| -uses it (`apistore.go:184`) — not a special rule invented here. |
| +**(decision) `GetEntitlement`/`PutEntitlement` mirror the existing `Get`/`Put`** |
| +(`objstore.go:84`, `:108`), including their `(nil, "", nil)`-means-absent idiom |
| +and CAS-on-etag, so a caller that knows one knows the other. |
| -**(decision) `Bucket`-style CAS via a new `objstore.GetEntitlement` / |
| -`PutEntitlement` pair**, mirroring the existing `Get`/`Put` (`objstore.go:84`, |
| -`:108`) including their `(nil, "", nil)`-means-absent idiom, so callers that |
| -already know one know the other. |
| +**(decision) A declined owner may re-apply.** `apply` overwrites `applied` and |
| +clears `decision`, under CAS. The anti-spam control is the shared `write` |
| +budget keyed on the address, already used by `handleAPIStoreCreate` |
| +(`apistore.go:184`) — not a new rule. |
| **Why the bucket and not the console database.** `MaterializeAccounts` is |
| explicit that the bucket is the control plane and the console is not the only |
| -minter: `carlos accounts create` writes `accounts/<sqid>/account.json` and a |
| -registry row without ever opening the console database, and the console |
| -catches up from those objects (`internal/console/materialize.go:14-30`). An |
| -entitlement held only in console SQL would be invisible bucket-side and would |
| -not survive a console rebuilt by materialisation. |
| +minter (`internal/console/materialize.go:14-30`). An entitlement held only in |
| +console SQL would be invisible bucket-side and would not survive a console |
| +rebuilt by materialisation. |
| -**Why a new object and not a field on `account.json`.** That manifest is read |
| -by boxes and by the edge (`internal/edge/storedoor.go:358`, |
| -`domains.AccountManifest`). Widening it makes every box parse entitlement data |
| -no box needs. |
| +**Orphaning is not a risk** (verified in review): apply needs the console to |
| +know the account, so an entitlement cannot precede materialisation; a deleted |
| +account's entitlement lingers inert because resurrection is refused |
| +(`materialize.go:88-105`); and the only `accounts/` key parser cuts on |
| +`/account.json` (`internal/domains/reconcile.go:547`). |
| ## 5. Authority |
| | Verb | Who | Gate | |
| |---|---|---| |
| -| apply | account **owner** | member gate, then `RoleOwner` | |
| -| approve / decline | deployment operator | `CARLOS_STORE_OPERATORS` | |
| -| approvals (list) | deployment operator | `CARLOS_STORE_OPERATORS` | |
| -| grant (per-app, unchanged) | deployment operator | `CARLOS_STORE_OPERATORS` | |
| - |
| -**(decision) Approving reuses `CARLOS_STORE_OPERATORS` rather than minting a |
| -second list.** It is the same authority — spending this deployment's cloud |
| -bill — and `MailOperators`' doc comment (`internal/console/server.go:577-585`) |
| -sets the precedent for when a *separate* list is warranted: when the authority |
| -is over a different thing (sending reputation, not money). This is the same |
| -thing. Empty stays fail-closed. |
| - |
| -**The 404-before-403 discipline carries over.** On the per-account doors the |
| -member gate runs first, so a stranger gets the same 404 as for an unknown |
| -account and learns nothing (`apistore.go:15-17`). The `approvals` list door has |
| -no account in its path, so it is a plain operator door that 404s non-operators, |
| -following `authcli.go:696`'s sibling idiom. |
| - |
| -**`storeOperator`'s exact-match rule is load-bearing and must be reused, not |
| -re-implemented.** Its doc comment records why `strings.EqualFold` is wrong |
| -here: U+017F (ſ) folds to "s" and survives `ToLower`, so `owner@example.teſt` |
| -would match `owner@example.test` and be handed the authority to spend money |
| -(`apistore.go:360-375`). New doors call `s.storeOperator`. |
| +| apply | account **owner** | `apiOwnerAccount` (member gate, then `RoleOwner`) | |
| +| approve / decline / approvals | deployment operator | fleet-operator bit **and** `CARLOS_STORE_OPERATORS` | |
| +| grant (per-app, unchanged) | deployment operator | unchanged | |
| + |
| +**(decision) Approve/decline/approvals follow the mail PAUSE door's shape, not |
| +`apiAccountApp`'s.** This is blocker 2, and `apimail.go:26-37` already states |
| +the reasoning for the identical case: |
| + |
| +> A pause is applied AGAINST a tenant, not asked for by one — the abusive |
| +> tenant has no reason to add the operator investigating them as a member, and |
| +> only that tenant's own owner can add one, so member-then-allowlist (store's |
| +> grant shape) would deadlock the abuse brake in the exact scenario it exists |
| +> for. |
| + |
| +An operator approving an account has no reason to be a member of it either. |
| +`apiAccountApp` resolves `{account}` against the **caller's own memberships** |
| +(`apicli.go:207`, `AccountsFor`), so the first draft's "member gate first" |
| +would have required every operator to join every account they approve. The |
| +per-app grant door has that property today and is why it is an override rather |
| +than the main path. |
| + |
| +So: `cliUser` → fleet-operator bit → `CARLOS_STORE_OPERATORS` → bare, |
| +byte-identical `http.NotFound`; then resolve the account **viewer-independently** |
| +via `AccountBySqid`, exactly as `mailPauseTarget` does (`apimail.go:212-228`). |
| + |
| +**(decision) Both the fleet bit and the env list are required**, mirroring |
| +mail: `store.IsOperator` (`store.go:1619`) then `s.storeOperator`. The env list |
| +**narrows** the fleet bit, never widens it. This also resolves the review's |
| +finding 8: the operator queue page lives under `/fleet` behind |
| +`operatorOnly`/`IsOperator` (`server.go:2075-2090`), so with this rule the |
| +people who can *see* the queue and the people who can *act* on it are the same |
| +population, minus nobody. |
| + |
| +**`storeOperator`'s exact-match rule is reused, never re-implemented.** Its doc |
| +comment records why `strings.EqualFold` is wrong: U+017F (ſ) folds to "s" and |
| +survives `ToLower`, so `owner@example.teſt` would match `owner@example.test` |
| +and be handed authority to spend money (`apistore.go:360-375`). |
| + |
| +**(decision) Service credentials.** Apply, approve, decline and approvals |
| +register with `svcrole.Set{}` — closed to service principals entirely. Note |
| +explicitly that under this spec an owner-minted `svcrole.Admin` credential's |
| +`create` can mint cloud estate, because `POST .../store` is Admin-reachable |
| +today (`authcli.go:425-440`); that is accepted, and `Declared.By` is set from |
| +`auditActor` so the record names the credential. `TestEveryCLIDoorNamesItsRoles` |
| +and `TestEveryClosedDoorRefusesAServiceCredential` cover the new patterns. |
| ## 6. What changes at declare time |
| -`handleAPIStoreCreate` keeps every gate and every refusal it has today, in the |
| -same order, and gains one branch at the end. |
| - |
| -After the declaration is written and its CAS has succeeded, if **all** of: |
| - |
| -- the deployment has a backend (`s.cfg.StoreProvisioner != nil`), and |
| -- the account's entitlement is approved, and |
| -- the record is not already granted, |
| - |
| -then the handler runs the same `objstore.Grant` the grant door runs, inline, |
| -with the same `storeWalkGates` and `storeSealKey` checks that door makes |
| -(`apistore.go:383-430`), and returns the provisioned reply. Otherwise: today's |
| -behaviour, byte for byte. |
| - |
| -**(decision) The grant block records why it exists.** `objstore.GrantBlock` |
| -gains `Source string`, mirroring `mail.GrantBlock.Source`, whose doc comment |
| -already established the vocabulary and the reason — *"records WHY the grant |
| -exists ("tier:free", "tier:pro", "operator") so a cap can be recomputed when a |
| -tier changes without silently discarding an operator's deliberate override"* |
| -(`internal/mail/mail.go:43-45`). Values here: `"operator"` for today's per-app |
| -path, `"account-approval"` for the new one. `By` is set to the operator who |
| -approved the **account** — a real person who really did authorise this spend, |
| -without the record pretending they ran a per-app grant. |
| - |
| -> **`Source` MUST be additive, and a rename here is a fleet-wide silent |
| -> failure.** `release.PurgeApp` parses `grant` and `provisioned` out of these |
| -> bytes with its own local struct — `release` cannot import `objstore`, |
| -> because `objstore` imports `release` — and refuses the box-side auto-purge |
| -> when either block is present. `Record`'s PARSER LIST comment says additive |
| -> changes are safe and these two key names are not free to move; |
| -> `TestPurgeGuardSeesOurRecord` in `walk_test.go` exists to fail loudly. |
| -> Adding a field inside `grant` is safe. Renaming `grant` is not. |
| - |
| -A record written before `Source` existed decodes it as `""`, which reads |
| -correctly as "granted by the operator path, before we recorded why" — the same |
| -additive-field reasoning `mail.Provisioned.PauseKind` documents. |
| - |
| -## 7. When inline provisioning fails |
| - |
| -`create` is cheap today and always succeeds. It will not be, and this is the |
| -part most likely to be got wrong. |
| - |
| -**(decision) Declaration first, committed, then walk.** The order is: |
| - |
| -1. Write the declaration and commit its CAS. This is the state an operator |
| - needs in order to act, and it must survive everything after it. |
| -2. Run the walk. |
| -3. If the walk fails — AWS unreachable, the console's role missing |
| - `s3:CreateBucket` or `iam:CreateUser`, a bucket name already taken — answer |
| - with the declaration reported as landed, the provisioning failure named, and |
| - a **non-2xx status**; the CLI exits non-zero. |
| - |
| -The record is then left declared-not-granted: a state the system already |
| -understands, that `carlos store status` already renders, and that a re-run of |
| -`create` or an operator `grant` resumes, because the walks are idempotent by |
| -construction (the package doc's "idempotent walks", and `Grant`'s own "a re-run |
| -never re-chooses the bucket", `walk.go:144`). **No new stuck state is |
| -introduced**, and the record never claims a bucket that does not exist. |
| - |
| -The failure sentence follows the existing rule: the caller gets a fixed |
| -sentence, the detail is logged, because a store error carries bucket names, |
| -endpoints and request ids (`apistore.go`'s existing `log.Printf` + |
| -`jsonError` pairing). |
| +**(decision) One CLI command, two doors.** `carlos store create` calls the |
| +existing declare door, then — when the reply says the account is approved and |
| +the deployment has a backend — a new `POST /a/{sqid}/apps/{app}/store/provision` |
| +door, member-gated and entitlement-gated. The member still runs one command |
| +(§3's ruling); the wire has two steps. |
| + |
| +This follows `carlos email enable`, which the first draft cited as precedent for |
| +an *inline server walk* and which is in fact client-side orchestration: |
| +`mail_cli.go:635-703` calls declare, ensure, poll and enable as separate doors, |
| +and the server 409s a provision against an undeclared app (`apimail.go:792-848`). |
| +Two doors also fix §8's honesty problem for free: door one's success prints |
| +"declared", door two's failure prints the provisioning error, and neither has to |
| +carry the other's outcome in a single status code. |
| + |
| +The provision door's gates, in order: `apiAccountApp` (member) → entitlement |
| +approved → `storeWalkGates` → `storeSealKey` → `objstore.Grant`. It uses |
| +`storeGrantTimeout` (60s, `apistore.go:339`), **not** `storeTimeout` (15s, |
| +`:212`) — a grant walk makes four backend round trips and the review found the |
| +first draft would have run it under the short one. |
| + |
| +**(decision) The walk runs whenever the account is approved and a backend |
| +exists — there is no "already granted" short-circuit in the door.** This is |
| +blocker 1. `Grant` writes the grant block at step 1, *before* any backend call |
| +(`walk.go:157-162`), so a walk that fails at `EnsureBucket`/`EnsureUser`/ |
| +`MintKey`/`writeEnv` leaves `Grant != nil, Provisioned == nil` — pinned by |
| +`apistore_test.go:1101-1130`. The first draft's "not already granted" condition |
| +would have skipped the retry, and the prefix lock at `apistore.go:241` would |
| +have engaged: **one transient AWS error would have put the member back in the |
| +operator queue this spec exists to remove.** `Grant` is idempotent and |
| +short-circuits itself when the record is provisioned *and* every delivered var |
| +is present (`walk.go:175-200`), which is the correct and only no-op condition. |
| + |
| +**(decision) `Grant` gains a `source` parameter**, written into |
| +`GrantBlock.Source` alongside `By`. This is a signature change touching every |
| +caller — not the "additive, inert" stage-1 item the first draft claimed. |
| +Mirrors `mail.GrantBlock.Source`, whose doc comment established the vocabulary: |
| + |
| +> records WHY the grant exists ("tier:free", "tier:pro", "operator") so a cap |
| +> can be recomputed when a tier changes without silently discarding an |
| +> operator's deliberate override — `internal/mail/mail.go:43-45` |
| + |
| +Values: `"operator"` for the per-app path, `"account-approval"` for this one, |
| +with `By` = the operator who approved the **account**. Not `omitempty`, matching |
| +`mail.GrantBlock.Source` (`mail.go:47`); a record written before the field |
| +existed decodes `""`, which reads correctly as "granted before we recorded why". |
| + |
| +> **`Source` must be additive; renaming `grant` is a fleet-wide silent |
| +> failure.** `release.PurgeApp` parses `grant`/`provisioned` as |
| +> `*json.RawMessage` (`internal/release/trash.go:211-222`) and refuses box-side |
| +> auto-purge when either is present. **Verified in review:** a field *inside* |
| +> `grant` is invisible to it and `TestPurgeGuardSeesOurRecord` stays green. The |
| +> other parsers of these bytes — `appsettings.go:309,808`, |
| +> `economicspass.go:171`, `ownership.go:407`, and the CLI's hand-rolled |
| +> `apiStoreGrantBlock` (`apimode.go:1552`) — all ignore unknown fields. No |
| +> non-Go parser exists. |
| + |
| +**(decision) A same-prefix re-declare becomes a no-op write.** |
| +`apistore.go:257` rewrites `at`/`by` unconditionally today; every such rewrite |
| +can CAS-fail an in-flight `Grant`'s step-1 or step-6 Put. With `create` now |
| +driving walks, that stops being theoretical (see §9). |
| + |
| +## 7. When provisioning fails |
| + |
| +The provision door answers non-2xx with a fixed sentence, the detail logged — |
| +the pairing `apistore.go` already uses everywhere, because a store error |
| +carries bucket names, endpoints and request ids. `apimode.go:1572-1580` turns |
| +any non-200 into `apiErrorFromBody` and `store.go:150-153` `log.Fatalf`s it, so |
| +the sentence *is* the entire user-visible outcome and must stand alone. |
| + |
| +Because the declaration landed through a **separate, already-succeeded door**, |
| +the CLI has already printed that it landed. There is no single response |
| +pretending to be both a success and a failure. |
| + |
| +The record is left `Grant != nil, Provisioned == nil` — grant block written, |
| +estate incomplete — which `carlos store status` already renders and which a |
| +re-run of `carlos store create` resumes, because §6 removed the short-circuit |
| +that would have blocked it and the walks are idempotent by construction |
| +(`walk.go:144`: "a re-run never re-chooses the bucket"). |
| + |
| +**(decision) Name the post-destroy window in the sentence.** `Teardown` deletes |
| +the record last (`walk.go:641`) and the next `create` re-mints the same |
| +deterministic name (`walk.go:208`); `aws.go:186` swallows only |
| +`BucketAlreadyOwnedByYou`, so a name AWS has not yet released fails the walk in |
| +the member's face — in a destroy/re-create loop they can now drive themselves. |
| +The failure sentence says so, and so does `docs/site/cli.md`. |
| ## 8. Surface |
| @@ -264,129 +316,193 @@ carlos store approve --account <a> operator |
| carlos store decline --account <a> --reason "..." operator |
| ``` |
| -`carlos store status --app <a>` gains one line naming the account's |
| -entitlement state, so a waiting member distinguishes: |
| +`carlos store status --app <a>` renders four entitlement states: |
| -- *not applied* — "this account is not approved for object storage — `carlos |
| - store apply`" |
| +- *not applied* — "this account is not approved for object storage — `carlos store apply`" |
| - *pending* — "applied by <owner> on <date>, waiting for a deployment operator" |
| -- *approved* — nothing extra; their store provisioned on declare |
| +- *approved* — no extra line; their store provisions on declare |
| - *declined* — the reason, and that they may apply again |
| -This is the same no-infra-access reasoning that put `backend` in the reply: the |
| -command alone must complete the job (AGENTS.md), and "declared, nothing |
| -happening" with no way to tell those four apart fails it. |
| +**(decision) Two existing printed sentences must change, not just gain a |
| +neighbour.** `store.go:261` always says "nothing is provisioned until a |
| +deployment operator grants it", and `store.go:348` says "waiting on a |
| +deployment operator (carlos store grant)" — pinned by `store_test.go:491`. |
| +Both are false in an approved account. `printStoreCreate` also needs a |
| +provisioned branch it does not have. |
| -**(decision) The entitlement rides on the existing store reply** (`storeReply`, |
| -`apistore.go:113`) as a new `entitlement` key, rather than requiring a second |
| -round trip. `storeReply` is deliberately the ONE answer shape both doors give |
| -so they can never drift; a second shape would reintroduce exactly the drift |
| -that comment is guarding against. |
| +**(decision) The entitlement rides on the existing `storeReply`** |
| +(`apistore.go:113`) as a new `entitlement` key. That function is deliberately |
| +the ONE answer shape both doors give so they cannot drift; a second shape would |
| +reintroduce exactly the drift it guards against. |
| ### Console |
| -- **Account settings** — an Object storage panel. Owners see the state and, when |
| - not applied, an apply control with the optional note. Non-owner members see |
| - the state read-only. This is where `appsettings.go:829`'s existing "This |
| - console has no storage backend configured" refusal already lives, so the two |
| - sentences sit together. |
| -- **Operator view** — pending applications, oldest first, each with account, |
| - applicant, note, and approve/decline. |
| - |
| -## 9. Notification |
| - |
| -This is the half that actually fixes the reported bug: James waited four days |
| -because nothing told anyone he was waiting. It is therefore **not optional**. |
| - |
| -**It is also more constrained than it looks.** `alertRecipients` mails the |
| -console's OWN operators (`s.store.Operators()`) and needs a per-operator |
| -Keymail OAuth grant, splitting off those it cannot write as into |
| -`run.missing` (`internal/console/alertwatch.go:151-168`). `CARLOS_STORE_OPERATORS` |
| -is a list of addresses with no such grant implied. **The two populations are |
| -not the same and may not overlap at all.** |
| - |
| -**(decision) The console queue is the primary surface; mail is the nudge.** |
| - |
| -- On apply: best-effort keymail to each address that is on |
| - `CARLOS_STORE_OPERATORS` **and** has a grant token. Addresses on the list |
| - with no grant are logged the way `run.missing` already is — never a silent |
| - drop, and never a failed write. |
| -- On decision: best-effort keymail to the applying owner. |
| -- A send failure **never** fails the write. The application is durable in the |
| - bucket the moment it is written; mail is a courtesy on top of it. |
| -- Because mail may reach nobody, the operator view carries a pending count |
| - where an operator will actually see it, so the queue is discoverable without |
| - any mail at all. |
| - |
| -## 10. The risk this accepts, deliberately |
| +- **Account settings** (`handleAccountSettings`, `server.go:2278-2298`, which |
| + already has `role` in hand) — an Object storage panel: owners see state and an |
| + apply control, other members see state read-only. *(The first draft put this |
| + on app settings, confusing it with the per-app destroy door at |
| + `appsettings.go:829`.)* |
| +- **Operator view** under `/fleet` — pending applications oldest first, with |
| + account, applicant, note, approve/decline, and the §11 bucket count. |
| + |
| +## 9. Concurrency: a gap this spec must close before it can ship |
| + |
| +**This is blocker 3, and it is a bug that exists today.** `walk.go:662-664` |
| +asserts "the console door serialises these verbs per app". **No such lock |
| +exists** — there is no mutex anywhere in `internal/console` guarding the store |
| +walks. Today two operators would have to run `grant` simultaneously to hit it. |
| +This spec makes `create` drive walks, and `apistore.go:176-179` explicitly says |
| +`create` is re-run "from a second terminal, from a deploy script" — so it turns |
| +a rare race into an ordinary one. |
| + |
| +The failure is not benign. Two concurrent walks on one app: both pass the |
| +handler read, the second's `Get` sees a grant block with `Provisioned == nil` |
| +and runs the full walk in parallel; `ReapKeys` (`walk.go:343`) deletes the |
| +other walk's fresh key; both `writeEnv`; and the loser's final Put CAS-fails |
| +**with no retry** — `Grant` has none, unlike `Rotate`'s guarded one |
| +(`walk.go:303-330`). The record's `KeyID` can then differ from the delivered |
| +key, and the next `Rotate`'s `ReapKeys(keep=[record key])` deletes the key the |
| +app is running on. `walk.go:404-410` calls that outcome "NOT survivable". |
| + |
| +**(decision) Three fixes, all before the §6 branch lands:** |
| + |
| +1. A keyed per-`(acct, app)` mutex in the console, held across **every** store |
| + walk — provision, grant, rotate, finish, destroy, purge. This makes |
| + `walk.go:662`'s comment true instead of aspirational. |
| +2. A `Rotate`-style guarded CAS retry on `Grant`'s step-6 Put |
| + (`walk.go:216-219`), re-reading and retrying once when the fresh record |
| + still names the state this walk started from. |
| +3. The same-prefix re-declare no-op from §6. |
| + |
| +Fixes 1 and 2 are corrections to existing code and are independently |
| +valuable — they should land as their own commits, ahead of the feature, so |
| +they can be reviewed and reverted separately. |
| + |
| +## 10. Revocation, movement, and what the entitlement does *not* control |
| + |
| +**(decision) `decline` is permitted over an approved decision, and is not a |
| +teardown.** It (a) stops new provisioning, (b) returns drift self-repair |
| +(`store.go:364`'s "re-run grant") to an operator step, and (c) **never** |
| +destroys existing estate — provisioned stores keep their buckets, credentials |
| +and grant blocks. An owner may re-apply afterwards, which clears the decision |
| +back to pending. |
| + |
| +**(decision) The entitlement gates provisioning, never existence.** |
| +`handleAppMoveAccount` (`appsettings.go:~918-988`) moves the console row only, |
| +while bucket and IAM names carry the original sqid (`walk.go:208-215`). So an |
| +app moved into an **unapproved** account keeps its working store, and §3's |
| +"every bucket is named into its account" stops being an invariant after a move. |
| +Metering follows the key prefix (`economicspass.go:161-190`), so the bill |
| +follows the app. All of this is accepted and stated rather than fixed. |
| + |
| +## 11. The risk this accepts, deliberately |
| "Unlimited, metered" means **AWS's soft limit of ~100 buckets per account |
| -becomes the deployment's real ceiling**, discovered at provision time as a raw |
| -AWS error in front of whichever tenant happens to declare next. |
| +becomes the deployment's real ceiling**, hit at provision time as a raw AWS |
| +error in front of whichever tenant declares next. |
| **(decision) Do not gate on it. Surface it.** The approvals view shows how many |
| -buckets the deployment has minted, so an operator approving the account that |
| -takes it past a comfortable threshold can see it before saying yes rather than |
| -after. A hard cap is a quota by another name and was ruled out in §3. |
| +buckets the deployment has minted, so an operator can see the number before |
| +saying yes. Sourcing it needs one additive field: `usage.AppStorage` |
| +(`internal/usage/storage.go:7-11`) cannot distinguish "provisioned, empty" from |
| +"no store" because `StoreBytes` is `omitempty`, so the daily pass gains |
| +`StoreProvisioned bool` (`economicspass.go:161-190`). A hard cap is a quota by |
| +another name and was ruled out in §3. |
| -## 11. Non-goals |
| +## 12. Non-goals |
| -- **Credit-card gating.** Later. The statement it will read is already written |
| - daily (§1); nothing here needs to anticipate its shape. |
| +- **Credit-card gating.** Later; the statement it will read is already written |
| + daily. |
| - **Fixing `CARLOS_STORE_BACKEND` as a root env edit.** That is #368. This spec |
| - makes the consequence visible and adds no new root env vars of its own. |
| + adds no new root env vars. |
| - **Quotas or bucket caps.** Ruled out in §3. |
| +- **Tearing down estate on decline.** §10. |
| - **A generalised capability-keyed entitlement system.** Email's tier |
| - vocabulary (`"tier:free"`, `"tier:pro"`) suggests where this ends up, but |
| - building the general machine before its second caller exists means guessing |
| - at email's requirements. `Entitlement` is shaped so that migration is a |
| - later addition, not a rewrite; no speculative capability column ships now. |
| - |
| -## 12. Testing |
| - |
| -Following the primitive spec's §11 split — walks against `MemProvisioner`, |
| -doors against the console fixture, printers against literal replies with no |
| -stub server in the loop. |
| - |
| -**Record.** Round-trip; the four derived states; absent reads as |
| -`(nil, "", nil)`; CAS conflict on concurrent apply. |
| - |
| -**Authority.** A non-owner member's apply is refused. A non-operator's |
| -`approvals` gets 404, not 403. A stranger still gets the account gate's 404 on |
| -the per-account doors. `storeOperator`'s ſ-vs-s case has a test — the existing |
| -one must keep passing and the new doors must route through the same function. |
| - |
| -**Declare-time branches**, the heart of it: |
| - |
| -- approved + backend → provisioned inline, `Source == "account-approval"`, |
| - `By ==` the approving operator. |
| -- approved + **no** backend → declared, today's backend warning, nothing |
| - minted. |
| -- not approved → declared, pointed at `apply`, nothing minted. |
| -- already granted → the walk does not run a second time. |
| -- prefix-change guard still refuses in every one of the above. |
| - |
| -**Failure.** A walk that fails after the declaration commits leaves the |
| -declaration present, answers non-2xx, and a re-run resumes to a provisioned |
| -store with the same bucket. |
| - |
| -**Regression.** `TestPurgeGuardSeesOurRecord` still passes with `Source` |
| -present — the guard must still see `grant` and refuse the purge. |
| - |
| -**Printers.** Each of the four entitlement states renders its own sentence, and |
| -the approved state adds no line. Tested against a literal reply, per |
| -`store_test.go`'s existing pattern. |
| - |
| -## 13. Rollout order |
| - |
| -1. `Entitlement` record + accessors + `GrantBlock.Source` (additive, inert). |
| -2. Console doors: apply, approvals, approve, decline. Still no behaviour change |
| - at declare time. |
| -3. The declare-time branch. This is the moment behaviour changes; everything |
| - before it is dormant. |
| -4. CLI verbs and the status line. |
| -5. Console UI and notification. |
| -6. `docs/site/cli.md`. |
| - |
| -Each stage builds and tests clean on its own, and stages 1-2 are safe to land |
| -before the branch in stage 3 exists. |
| + vocabulary suggests where this ends up, but building the general machine |
| + before its second caller exists means guessing at email's requirements. |
| + `Entitlement` is shaped so that migration is an addition, not a rewrite. |
| + |
| +## 13. Testing |
| + |
| +Walks against `MemProvisioner`, doors against the console fixture, printers |
| +against literal replies with no stub server — the primitive spec's §11 split. |
| + |
| +**Record.** Round-trip; four derived states; absent reads `(nil, "", nil)`; CAS |
| +conflict on concurrent apply. **Plus: `store/entitlements/` is not readable |
| +through the edge store door** (§4's implementation gate). |
| + |
| +**Authority.** Non-owner apply → 403 via `apiOwnerAccount`. **A non-member |
| +operator can approve** — the blocker-2 regression test. Non-operator on |
| +approvals → bare 404, not 403. Fleet-bit-without-env-list and |
| +env-list-without-fleet-bit both refused. `storeOperator`'s ſ-vs-s test still |
| +passes and the new doors route through it. Service credentials refused on all |
| +four new doors. |
| + |
| +**Declare/provision branches.** Approved + backend → provisioned, |
| +`Source == "account-approval"`, `By ==` the approving operator. Approved + no |
| +backend → declared, existing backend warning, nothing minted. Not approved → |
| +declared, pointed at `apply`, nothing minted. Provisioned **and delivered** → |
| +no-op, no new key. **Grant-written-but-not-provisioned → the walk RUNS and |
| +completes** (blocker 1's regression). Prefix-change guard still refuses |
| +throughout. |
| + |
| +**Concurrency (§9).** Two simultaneous provisions of one app serialise, mint |
| +one key, and leave the record's `KeyID` equal to the delivered key. Grant's |
| +step-6 CAS retry covers the interleaving that used to lose. A same-prefix |
| +re-declare during an in-flight walk does not CAS-fail it. |
| + |
| +**Failure.** A walk failing after the grant block is written leaves |
| +`Grant != nil, Provisioned == nil`, answers non-2xx, and a re-run converges to |
| +a provisioned store with the same bucket. |
| + |
| +**Regression.** `TestPurgeGuardSeesOurRecord` green with `Source` present. |
| + |
| +**Printers.** Four entitlement states; the two changed sentences |
| +(`store.go:261`, `:348`); `printStoreCreate`'s new provisioned branch. |
| + |
| +## 14. Rollout order |
| + |
| +Stages 1 and 2 are bug fixes to existing code and land first, separately. |
| + |
| +1. **Grant's CAS retry** (§9 fix 2) — its own commit. |
| +2. **The console's per-app store walk mutex** (§9 fix 1) — its own commit. |
| +3. `Entitlement` record + accessors + edge-ACL test; `GrantBlock.Source` and |
| + `Grant`'s new parameter (signature change, all callers). |
| +4. Console doors: apply, approvals, approve, decline. No behaviour change yet. |
| +5. The provision door and the CLI's two-door `create`. **Behaviour changes |
| + here**; everything before is dormant. |
| +6. CLI verbs, the status line, the two changed sentences. |
| +7. Console UI, notification, `StoreProvisioned` and the bucket count. |
| +8. `docs/site/cli.md`. |
| + |
| +## 15. Notification |
| + |
| +James waited four days because nothing told anyone. This is not optional. |
| + |
| +**It is narrower than it looks, and the review confirmed the mechanism.** |
| +`alertRecipients` (`alertwatch.go:151-168`) uses `store.Operators()` — users |
| +with the fleet-operator bit (`store.go:1634`) — plus `GrantToken` (`:1702`), |
| +and `SendApp` delivers only "to the granting user, using the stored grant |
| +token" (`keymail.go:232-238`). **Mail reaches an address only if that person |
| +completed the console OAuth ceremony.** |
| + |
| +**(decision) The console queue is the primary surface; mail is the nudge.** |
| + |
| +- On apply: for each `cfg.StoreOperators` entry, canonicalise with |
| + `ToLower(TrimSpace)` — matching `storeOperator` (`apistore.go:377`), since |
| + `GrantToken` is keyed on the canonical address — then `GrantToken`, then |
| + `s.sendApp`, guarded for `s.sendApp == nil` (`alertwatch.go:103`). Addresses |
| + with no grant are logged as `alertRun.missing` already is: never a silent |
| + drop, never a failed write. Best-effort off the request goroutine, following |
| + `notifyCLIIssued` (`authcli.go:948-990`). |
| +- On decision: best-effort keymail to the applying owner. |
| +- A send failure never fails the write. The application is durable in the |
| + bucket the moment it is written. |
| +- Because mail may reach nobody, the operator view carries the pending count |
| + where §5's population will actually see it. |
| + |
| +**Open, deliberately deferred to implementation** (review S1): whether |
| +`s.audit(acct, app, …)` (`server.go:2692`) renders an app-less row for these |
| +account-level events. If it does not, apply/approve/decline audit with the app |
| +field empty and the events view gains a case; this is a small question best |
| +answered against the events schema, not guessed at here. |