docs: fix bucket-writer audit — status-lease, restore-verify ruling, CLI migration writers
Review findings addressed: - Critical: internal/lease's status-lease election (litestream/lss3.Leaser, never release.Store or the AWS SDK) was unenumerated; add its row, verify the lease's deployment-bucket scope explicitly, and add lss3/litestream to the swept-idiom list. - Important: control/restore-verify.json's root cause was misdiagnosed as a CAS race; correct the analysis (Merge's fleet-membership filter drops cross-box hosts deterministically) and record the controller's ruling (per-box keys, issue #241). - Important: accounts.go's migration-verb writers (putOverwrite, store.Put in runAccountsMigrate/verifyMigratedPointers) were missing from the CLI direct-bucket sweep; add rows and correct the Self-review's completeness claim to an honest scope statement.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 file changed,
+179
−62
docs/superpowers/specs/2026-08-20-bucket-writer-audit.md+179 −62
diff --git a/docs/superpowers/specs/2026-08-20-bucket-writer-audit.md b/docs/superpowers/specs/2026-08-20-bucket-writer-audit.md| index b5c7d361c..4c5e4b3a7 100644 |
| --- a/docs/superpowers/specs/2026-08-20-bucket-writer-audit.md |
| +++ b/docs/superpowers/specs/2026-08-20-bucket-writer-audit.md |
| @@ -12,12 +12,21 @@ grep -rn "\.Put(\|\.Delete(\|putReplacing(" internal/ --include='*.go' | grep -v |
| ``` |
| 79 matches. Swept beyond that recipe for other store-write idioms: |
| -`s3.PutObject`, `PutObjectInput`, `DeleteObject(s)`, `.Upload(`, and a |
| -`.Write(` scan on store-shaped receivers (see Self-review). Grouped below by |
| -function/key, one row per distinct write site — where a CAS retry pattern |
| -puts the same key twice (a create attempt + one retry), both grep line |
| -numbers are cited on one row, matching the brief's own citation style |
| -(`internal/usage/edge.go:40` for a Put that's actually on line 93). |
| +`s3.PutObject`, `PutObjectInput`, `DeleteObject(s)`, `.Upload(`, a `.Write(` |
| +scan on store-shaped receivers, and — added after a review finding caught |
| +the gap — **`litestream/lss3.Leaser`** (see Findings §1: `lease.Acquire`'s |
| +lock is a conditional-write election that goes straight to litestream's own |
| +S3 client, never through `release.Store` or the raw AWS SDK's |
| +`PutObject`/`DeleteObject`, so none of the above idioms match it; the next |
| +auditor should grep `lss3\.\|litestream\.` too). Also swept the root |
| +`main` package (`*.go` at repo root, i.e. the `carlos` CLI) directly for |
| +`openStore()`-fed writers rather than trusting each `internal/` package's |
| +own caller list, which is what caught the migration-verb writers in |
| +Findings §6 below. Grouped below by function/key, one row per distinct |
| +write site — where a CAS retry pattern puts the same key twice (a create |
| +attempt + one retry), both grep line numbers are cited on one row, matching |
| +the brief's own citation style (`internal/usage/edge.go:40` for a Put |
| +that's actually on line 93). |
| **Ruling vocabulary**: `console-only` | `per-box keys` | `housekeeper` | |
| `elected already` | `NEEDS RULING`, plus two escapes this sweep needed and |
| @@ -29,7 +38,7 @@ safe but doesn't fit the five-word vocabulary cleanly. |
| | File:line | Key / prefix | Runner | Ruling | |
| |---|---|---|---| |
| -| `internal/status/fleet.go:572,580` (`PublishFleet`→`putOverwrite`) | `status/fleet.json` | lease-elected publisher | elected already | |
| +| `internal/host/agent.go:1895` → `internal/lease/lease.go:187-231` (`lease.Acquire`, `Held.Release`, `startRenewer`) | `<CARLOS_STATUS_PREFIX>/status-lease/lock.json` (litestream's `lss3.Leaser`: lock lands at `<prefix>/lock.json`, and the prefix passed in is `path.Join(CARLOS_STATUS_PREFIX, "status-lease")`) — conditional-write lock, TTL 15m, renewed at TTL/3, released on graceful stand-down | every non-fleet box, on every status tick, contending for THIS election; the winner renews for as long as it holds it | elected already — **this IS the status publisher's election mechanism**, not a writer needing one. Not caught by the `.Put(\|.Delete(` recipe or the supplementary S3-idiom sweep: it calls `lss3.NewLeaser().AcquireLease`/`ReleaseLease` directly, bypassing both `release.Store` and the raw AWS SDK's `PutObject`/`DeleteObject`. See Findings §1. | |
| | `internal/status/snapshot.go:540,548` (`WriteBox`) | `status/boxes/<label>.json` | every box, own key | per-box keys (already) | |
| | `internal/appconfig/store.go:187` (`applyAt`, called by `Apply`/`SetEnv`) | `apps/<acct>/<app>/config.json` or `.../environments/<env>.json` | console (`apiconfig.go`) | console-only | |
| | `internal/appconfig/store.go:288` (`SetExecDeliveryAt`) | same bundle keys | console (`apiconfig.go`) | console-only | |
| @@ -48,9 +57,9 @@ safe but doesn't fit the five-word vocabulary cleanly. |
| | `internal/host/snapshot_loop.go:105,126,134,144,230` | `control/registry-snapshot(s)/<box>.{db,json}` + generations | every box | per-box keys — **Task 1, done** (b466af4+02f449a) | |
| | `internal/edge/storedoor.go:190` (PUT) | `fleets/<fleet>/...` or `apps/<sqid>/<app>/logs/...` (ACL-scoped: `aclWrite`) | every box serving the store door, **proxying** the connecting fleet member's write | every box, but the write identity is the fleet member and CAS is enforced by the same shared `release.Store` regardless of which box proxies — safe as-is, no per-box key needed | |
| | `internal/edge/storedoor.go:211` (DELETE) | same ACL scope | every box (proxy) | same as above | |
| -| `internal/dbverify/dbverify.go:144` (`Publish`) | `control/restore-verify.json` — **one fixed key, no box component** | every box, via `carlos-hibernate-verify.timer` (systemd timer, every box — see `internal/host/units/carlos-hibernate-verify.timer`) | **NEEDS RULING** — see Findings §1 | |
| +| `internal/dbverify/dbverify.go:144` (`Publish`) | `control/restore-verify.json` — **one fixed key, no box component** | every box, via `carlos-hibernate-verify.timer` (systemd timer, every box — see `internal/host/units/carlos-hibernate-verify.timer`) | **RULED (controller, 2026-08-20): per-box keys** — `Merge`'s fleet-membership filter deterministically drops cross-box hosts on every publish (not a race); fix deferred to issue #241, must land before a second box arms this timer. See Findings §2 | |
| | `internal/release/prune.go:129,136` (`DeleteVersion`) | `releases/<acct>/<app>/<version>/*` | every box (`host/retention.go` `retentionPass`) | **housekeeper (Task 3 pending)** — matches brief's known-expected row | |
| -| `internal/domains/reconcile.go` → `internal/release/trash.go:237,241` (`PurgeApp`) | `releases/<acct>/<app>/*` (whole trashed app) + `apps/<acct>/<app>/deleted.json` marker | **every box** (`host/agent.go` → `domains.Reconcile`'s auto-purge step) **and** console (`ownership.go`) | doc-asserted race-proof for N boxes already (see Findings §2) — ruled safe, flagged for controller confirmation, not literally NEEDS RULING | |
| +| `internal/domains/reconcile.go` → `internal/release/trash.go:237,241` (`PurgeApp`) | `releases/<acct>/<app>/*` (whole trashed app) + `apps/<acct>/<app>/deleted.json` marker | **every box** (`host/agent.go` → `domains.Reconcile`'s auto-purge step) **and** console (`ownership.go`) | doc-asserted race-proof for N boxes already (see Findings §3) — ruled safe, flagged for controller confirmation, not literally NEEDS RULING | |
| | `internal/release/serial.go:63` (`BumpSerial`) | `control/serial` | console only (`internal/console/serialbump.go`) | console-only | |
| | `internal/release/release.go:598` (`putImmutable`, called by `Ship`) | `releases/<acct>/<app>/<version>/manifest.json` (content-addressed, create-only) | console (`apicli.go`, `server.go`) **+ CLI direct** (`carlos ship`, `openStore()`, `cli.go:1883`) | console-only (CLI direct path is create-only/idempotent-on-identical-bytes; see CLI direct-bucket note) | |
| | `internal/release/release.go:722` (`Promote`) | `apps/<acct>/<app>/channels/<channel>.json` | console (`apicli.go`, `server.go`) **+ CLI direct** (`carlos promote`, `PromoteOnLadder`, `cli.go:2083`) | console-only (+ CLI direct, CAS read-etag-then-Put) | |
| @@ -64,6 +73,8 @@ safe but doesn't fit the five-word vocabulary cleanly. |
| | `internal/release/trash.go:50` (`TrashApp`) | `apps/<acct>/<app>/deleted.json` | console (`appsettings.go`, `apicli.go`) **+ CLI direct** (`apps.go:271`) | console-only (+ CLI direct, create-only) | |
| | `internal/release/trash.go:99` (`RestoreApp`) | same | console (`ownership.go`, `apicli.go`) **+ CLI direct** (`apps.go:348`) | console-only (+ CLI direct) | |
| | `internal/release/store.go:104,108` (`prefixStore.Put/Delete`) | pass-through — rebases the key onto a fleet's own prefix, then delegates to the wrapped `Store` | n/a — wrapper, not a distinct write | N/A (wrapper; the underlying write is whichever real store the fleet box holds — customer-fleets namespacing, out of platform-box scope) | |
| +| `accounts.go:398,707` → `accounts.go:508-521` (`putOverwrite`, root package) | `releases/**` manifest/channel-pointer keys under the destination account (`runAccountsMigrate`'s copy loop and `verifyMigratedPointers`'s re-converge) | operator-invoked CLI verb (`carlos accounts migrate`), direct `openStore()` against `CARLOS_DEPLOYMENT_BUCKET` | ad hoc operator action, last-writer-wins by design (read-etag-then-Put, no create-only guard) — see Findings §6 | |
| +| `accounts.go:409` (`store.Put`, root package) | same migration copy loop, create-only branch for non-channel-pointer objects | same CLI verb | same as above — see Findings §6 | |
| | `internal/provision/provision.go:548` (`PutManifest`) | `apps/<acct>/<app>/instances.json` | console — **no HTTP caller found**; only caller is CLI direct (`instances_cli.go:202`, `carlos instances enable`, store-mode) | console-only in spirit (CLI direct-bucket, CAS upsert-with-retry) — see CLI direct-bucket note | |
| | `internal/provision/provision.go:573` (`PutManifestIfMatch`) | same | console (`apiinstances.go`) | console-only | |
| | `internal/provision/provision.go:585` (`CreateRecord`) | `apps/<acct>/<app>/instances/<host>.json` | console (`domainattach.go`, `apiinstances.go`) | console-only | |
| @@ -86,7 +97,7 @@ safe but doesn't fit the five-word vocabulary cleanly. |
| | `internal/console/economicspass.go:1056` (`putActiveScenario`) | `economics.ActiveKey` | console only | console-only | |
| | `internal/usage/statement.go:55` (`UpsertStatement`) | `usage/accounts/<acct>/<month>.json` | console (`economicspass.go`) | console-only | |
| | `internal/usage/statement.go:71` (`CreateDayObject`) | `usage/census/...` / `usage/storage/...` day objects | console (`economicspass.go`) | console-only — matches brief's known-expected row | |
| -| `internal/edge/wildcard.go:483` (`w.cache.Put`, wildcard cert PEM) | `autocert.Cache` key `wildcardPEMKey(parent)` | every box that runs the wildcard manager | **N/A today** — see Findings §3 (cache backend is local disk; bucket ("s3") backend is reserved/unimplemented) | |
| +| `internal/edge/wildcard.go:483` (`w.cache.Put`, wildcard cert PEM) | `autocert.Cache` key `wildcardPEMKey(parent)` | every box that runs the wildcard manager | **N/A today** — see Findings §4 (cache backend is local disk; bucket ("s3") backend is reserved/unimplemented) | |
| | `internal/edge/wildcard.go:591` (`w.cache.Put`, ACME account key) | `wildcard-account.key` | every box | N/A today — same as above | |
| | `internal/edge/edge.go:977` (`e.cache.Put`) | in-process HTTP response cache (`*Cache`), keyed by request | every box, in-memory only | N/A — not a bucket write | |
| | `internal/edge/compress.go:277` (`gzipPool.Put`) | `sync.Pool` recycling a `*gzip.Writer` | every box, in-memory only | N/A — not a bucket write; unrelated `Put` overload entirely | |
| @@ -117,41 +128,76 @@ have to rediscover the CLI path themselves. |
| ## Findings (surprises, worth the controller's attention) |
| -### 1. `control/restore-verify.json` — NEEDS RULING |
| - |
| -`internal/dbverify/dbverify.go` publishes ONE fixed key with no per-box |
| -component, and the doc comment places it "beside `control/serial` and |
| -`control/registry-snapshot.json`" — i.e. explicitly aware of the same |
| -key-collision family Task 1 fixed for snapshots, yet it wasn't migrated. |
| - |
| -The write path (`internal/ops/ops.go`'s `runHibernateVerify`) does |
| -`Load` (Get) → `Merge` (fold this run's verdicts into the prior record, |
| -in-process) → `Publish`. `Publish`'s retry loop (`internal/dbverify/dbverify.go:120-144`) |
| -re-fetches the ETAG on each of 3 attempts but does **not** re-fetch or |
| -re-merge the BODY — the marshaled `rec` is fixed once, before the loop. If |
| -two boxes' `carlos-hibernate-verify.timer` runs race (confirmed installed |
| -on every box: `internal/host/units/carlos-hibernate-verify.timer`), the |
| -loser's retry re-reads a fresh etag and blindly overwrites the winner's |
| -just-published record with its own OLDER merge — silently dropping |
| -whatever the winner had just added, with no error surfaced (the retry |
| -"succeeds"). |
| - |
| -This is the same fixed-key shape #158 fixed for snapshots, but with a worse |
| -failure mode: #158's collision surfaced as an outright overwrite each box |
| -could at least notice was contended; this one merges first, so the loser's |
| -write looks locally correct and the data loss is invisible until someone |
| -diffs `Hosts` against what a box's own log claimed to publish. I'm not |
| -confident whether the right fix is (a) box-scoped keys |
| -(`control/restore-verify/<box>.json`, mirroring snapshots, with a reader |
| -that folds them for display) or (b) keeping one merged key but making |
| -`Publish` re-Load-and-re-Merge inside each retry attempt rather than |
| -retrying a stale body — that's a design call, not something to invent here. |
| -Flagging for the controller before Task 3/4 lock in the housekeeper-lease |
| -boundary, since this key doesn't fit "deployment-global duty" (nothing |
| -elects one issuer) OR "per-box data" (results are meant to be read as one |
| -fleet-wide table) cleanly. |
| - |
| -### 2. `release.PurgeApp` has two callers, one of them every-box — ruled safe, flagged for confirmation |
| +### 1. `internal/lease` — the status-lease election was missing from this audit entirely (review Finding 1, fixed) |
| + |
| +The status publisher's election — the mechanism the spec's Part 1 table |
| +calls out by name ("The status publisher's CAS-lease is the one election |
| +mechanism, reused; no second election pattern may be introduced") and asks |
| +this stage-1 audit to verify is deployment-scoped — was absent from the |
| +first pass of this doc. Not an oversight of judgment: it's a genuine |
| +sweep-idiom gap. `internal/host/agent.go:1895` calls `lease.Acquire(ctx, |
| +bucket, path.Join(prefix, "status-lease"), region)`; `internal/lease/lease.go`'s |
| +`acquireTTL` (187-215) hands the bucket/path straight to |
| +`github.com/benbjohnson/litestream/s3.Leaser` — `leaser.Bucket`, |
| +`leaser.Path`, `leaser.AcquireLease(ctx)` — which does its own conditional |
| +`PutObject`/`GetObject` against `<prefix>/lock.json` inside the litestream |
| +library, never through `release.Store`, never through a call site spelled |
| +`.Put(` or `.Delete(` on anything this package owns, and never through the |
| +raw AWS SDK types (`PutObjectInput` etc.) the supplementary sweep grepped |
| +for. `startRenewer` rewrites the lock every `TTL/3` (15m/3 = 5m in |
| +production, `LeaseTTL` in `lease.go`); `Held.Release` (`lease.go:233-239`) |
| +deletes it on graceful stand-down. None of the audit's grep patterns — |
| +recipe or supplementary — could have matched any of that; the idiom list |
| +above now names `lss3.Leaser`/`litestream.` explicitly so the next auditor |
| +greps for it. |
| + |
| +**Deployment-scope verification** (what the spec's Part 1 table asked |
| +for): the bucket passed to `lease.Acquire` is `resolveStatusBucket`'s |
| +result (`internal/host/agent.go:1736`, `ResolveStatusBucket` at |
| +`agent.go:1650-1653`) — `CARLOS_STATUS_BUCKET` if set, else the fallback to |
| +`CARLOS_DEPLOYMENT_BUCKET`. In the default (undocumented-override-free) |
| +deployment shape every box in a deployment resolves to the SAME bucket — |
| +the one control bucket the whole writer map above assumes — so the lock |
| +key `<CARLOS_STATUS_PREFIX>/status-lease/lock.json` names exactly one |
| +object in exactly one bucket, and litestream's conditional-write lease |
| +guarantees exactly one holder among every box (fleet boxes excepted — |
| +`shouldSkipPublisherElection` keeps them out of the contention entirely) |
| +resolving that bucket at once. That IS the "deployment-scoped, therefore |
| +exactly one holder per deployment" property Task 3's housekeeper lease |
| +inherits by reusing this same mechanism, stated explicitly rather than |
| +left implicit. Ruled `elected already` — this is the election itself, not |
| +a writer that needs one. |
| + |
| +### 2. `control/restore-verify.json` — RULED (controller, 2026-08-20) |
| + |
| +**Corrected analysis** (the original draft misdiagnosed this as a race |
| +condition; it is not — see review Finding 2). `Merge` |
| +(`internal/dbverify/dbverify.go:65-84`) does not merge prior state with new |
| +state additively: it starts a FRESH `Record{Hosts: map[string]Verdict{}}` |
| +and copies forward only the prior hosts that are also `inFleet` for THIS |
| +run — and `fleet` is built from the calling box's OWN registry (every |
| +`h.Host` in `hosts, err := HibernatingHosts(reg)` at the call site, |
| +`internal/ops/ops.go`'s `runHibernateVerify`), never from what any other |
| +box's registry holds. So a host that box A's registry knows about but box |
| +B's doesn't is DROPPED the moment box B publishes — not on a race, not |
| +sometimes: EVERY time B publishes after A, deterministically, whether or |
| +not the two timers ever overlap in wall-clock time. Two boxes with |
| +different hibernating-instance sets deterministically wipe each other's |
| +verdicts on alternating publishes. `Record.Box` is a single scalar too |
| +(`Box string`, not a set), so the record can never even name more than the |
| +most recent publisher. My original fix option (b) — make `Publish` |
| +re-Load-and-re-Merge inside its CAS retry — does **not** fix this defect: |
| +the loss isn't caused by a stale retry body, it's caused by `Merge`'s own |
| +fleet-membership filter discarding cross-box hosts by design, on every |
| +call, CAS or no CAS. |
| + |
| +**Controller ruling, verbatim**: "RULED (controller, 2026-08-20): per-box |
| +keys — `control/restore-verify/<box>.json`, snapshots' shape; option (b) |
| +invalid because the Merge fleet-drop loss is deterministic; implementation |
| +deferred to issue #241 (must land before a second platform box arms the |
| +verify timer — pre-stage-5)." |
| + |
| +### 3. `release.PurgeApp` has two callers, one of them every-box — ruled safe, flagged for confirmation |
| `internal/domains/reconcile.go`'s auto-purge step (run from |
| `internal/host/agent.go`'s `domains.Reconcile` pass, **every box, every |
| @@ -174,7 +220,7 @@ housekeeper-lease scope should explicitly decide whether this stays outside |
| the lease (my read: yes, it's structurally different from keep-newest-N |
| prune) or gets folded in for uniformity. |
| -### 3. Wildcard cert issuance (`internal/edge/wildcard.go`) is not a bucket writer today — but is the exact seam Part 2 will need |
| +### 4. Wildcard cert issuance (`internal/edge/wildcard.go`) is not a bucket writer today — but is the exact seam Part 2 will need |
| The grep caught `w.cache.Put(...)` twice (wildcard cert PEM, ACME account |
| key). `w.cache` is a `golang.org/x/crypto/acme/autocert.Cache`, and |
| @@ -197,7 +243,7 @@ mechanism) or whether wildcard stays on per-box `DirCache` forever |
| (acceptable only if wildcard and the new `control/certs/**` machinery never |
| need to agree on a cert for the same hostname). |
| -### 4. `WriteFleetBinding` — CAS-arbitrated, not lease/per-box, ruled safe |
| +### 5. `WriteFleetBinding` — CAS-arbitrated, not lease/per-box, ruled safe |
| `internal/host/fleetbind.go` writes `fleetBindingKey(fleet, host)` — a |
| single key with no platform-box component, from every box's activator |
| @@ -213,6 +259,48 @@ once more at startup"). Ruled safe as designed; flagged because it's a |
| pattern the writer map's four-row vocabulary doesn't have a name for, and a |
| future writer-map revision might want one. |
| +### 6. Root-package CLI migration writers — missed by the first pass, ruled safe (review Finding 3, fixed) |
| + |
| +The first pass of this audit checked `openStore()`-fed writers reachable |
| +from `cli.go`, `accounts.go`'s account-mint path, `apps.go`, |
| +`appconfig_cli.go`, and `instances_cli.go` (the CLI direct-bucket note |
| +above), but missed `accounts.go`'s OTHER root-package writers: |
| +`runAccountsMigrate` (`accounts.go:310` onward) and its verification pass |
| +`verifyMigratedPointers` (`accounts.go:657` onward) — the multi-account |
| +data-move verb CLAUDE.md calls out as "the one deliberate data move the |
| +multi-account rollout needs." Two call sites: |
| + |
| +- `accounts.go:398` and `:707` call `putOverwrite` (defined |
| + `accounts.go:508-521`): read-etag-then-Put, ONE retry, no create-only |
| + guard — genuinely last-writer-wins, the same shape as |
| + `status.WriteBox`/`host.putAlertState` elsewhere in this doc, but here |
| + driven by an operator's shell rather than a box's own periodic loop. |
| + Used for channel pointers (`runAccountsMigrate`'s copy loop when the |
| + destination already has a qualified pointer that must be converged |
| + rather than blindly overwritten) and for `verifyMigratedPointers`'s |
| + re-copy when a promote raced the migration mid-copy. |
| +- `accounts.go:409` calls `store.Put(ctx, c.to, body, "")` directly |
| + (create-only) for the ordinary manifest-copy case, falling back to a |
| + byte-for-byte comparison against whatever already exists on a CAS |
| + conflict — refuses to proceed (`log.Fatalf`) rather than silently |
| + overwrite if the existing object's content differs. |
| + |
| +Both write `releases/**` objects (manifests, channel pointers) under |
| +`CARLOS_DEPLOYMENT_BUCKET`, opened directly via `openStore()` — the same |
| +CLI direct-bucket shape as `ship`/`promote`/`rollback` elsewhere in this |
| +doc, not a box service loop. Ruled safe for the N-platform-boxes question |
| +this audit exists to answer, for the same reason as the rest of the |
| +CLI-direct-bucket note: it doesn't run persistently on any of the N boxes, |
| +so box count doesn't multiply its contention. It is NOT safe against two |
| +operators running `carlos accounts migrate` concurrently against the SAME |
| +app/account pair — `putOverwrite`'s read-etag-then-Put has exactly one |
| +retry and no election, so two operators racing would have the same |
| +last-writer-wins exposure two people editing the same file over SSH would. |
| +That exposure is already true today, independent of this spec's box-count |
| +question, and isn't something stage 1 introduces or needs to fix — flagged |
| +here only so a future reader tracing `releases/**` writers has the |
| +complete list, per the same reasoning as the CLI-direct-bucket note above. |
| + |
| ## Self-review — could a writer exist that none of the greps caught? |
| 1. **Listed every `release.Store` implementor and read every method that |
| @@ -244,7 +332,7 @@ future writer-map revision might want one. |
| bucket keys). |
| 4. **Checked every `release.Store`-typed function for callers outside what |
| the grep's line already showed**, rather than trusting the first caller |
| - found — this is how findings §1, §2, and the CLI-direct-bucket note were |
| + found — this is how findings §2 (restore-verify) and §3 (PurgeApp) were |
| caught: `dbverify.Publish` looked console-adjacent from its own package |
| doc comment until tracing to `internal/ops/ops.go` showed a systemd timer |
| installed fleet-wide; `release.PurgeApp` looked console-only until |
| @@ -252,14 +340,41 @@ future writer-map revision might want one. |
| several `release.*`/`appconfig.*`/`domains.*` functions I initially |
| assumed were console-exclusive (based on `deploy.go` going through the |
| console HTTP API) turned out to have a second CLI direct-bucket caller |
| - via `openStore()` once I grepped for it explicitly across the CLI's root |
| - package. |
| -5. **Did not find** any additional deployment-bucket writer beyond the |
| - above. The two areas most likely to hide one — a second `Store` |
| - implementation, and non-`Store`-interface S3 calls — were both |
| - enumerated and both came back accounted for (fleet-api client |
| - delegates to the store door; objstore's S3 calls are a different |
| - bucket entirely). |
| + via `openStore()` once I grepped for it explicitly. |
| +5. **What review found this pass had NOT closed, and how each was |
| + closed**: (a) the first draft's `openStore()` sweep covered `cli.go`, |
| + `appconfig_cli.go`, `apps.go`, `instances_cli.go`, and the account-mint |
| + path in `accounts.go`, but not `accounts.go`'s OTHER root-package |
| + writers (`runAccountsMigrate`/`verifyMigratedPointers`) — closed by |
| + sweeping the entire root `main` package for `openStore()` call sites |
| + rather than the subset the first pass happened to open (Findings §6); |
| + (b) the status-lease election writes through |
| + `github.com/benbjohnson/litestream/s3.Leaser` directly, which is not a |
| + `release.Store` call, not a raw-AWS-SDK call, and not an `.Write(` on |
| + anything store-shaped — none of the swept idioms could have matched it, |
| + so it was invisible to every check in items 1-4 above until traced |
| + explicitly from `internal/host/agent.go`'s election logic — closed by |
| + adding `lss3.Leaser`/`litestream.` to the swept-idiom list (Findings |
| + §1). |
| +6. **Honest scope statement, corrected from the first draft's overclaim** |
| + ("did not find any additional deployment-bucket writer beyond the |
| + above" was too strong a claim for what was actually checked): this |
| + audit's actual coverage is (i) the brief's recipe grep across |
| + `internal/`, (ii) the supplementary sweeps named at the top of this doc |
| + (S3-SDK idioms, `.Write(` on store-shaped receivers, and now |
| + `lss3.Leaser`/`litestream.`), (iii) every `release.Store`-typed |
| + function's full caller list, traced rather than assumed, and (iv) a |
| + root-`main`-package sweep for `openStore()` call sites. It is not a |
| + claim that every conceivable write idiom in the codebase was |
| + enumerated — a fourth idiom this audit hasn't thought to grep for could |
| + exist the same way `lss3.Leaser` did before review Finding 1. What (i)-(iv) |
| + together rule out: any writer reachable through `release.Store`'s three |
| + implementations (all read), any direct AWS-SDK S3 call against the |
| + deployment bucket specifically (as opposed to the tenant object-store |
| + bucket, which is a different bucket and out of scope by definition), and |
| + any CLI verb that opens the deployment bucket directly. A future |
| + auditor extending this doc should keep growing the swept-idiom list |
| + rather than trust that (i)-(iv) as currently scoped is exhaustive. |
| ## Cross-check against the spec's Part 1 table |
| @@ -267,13 +382,15 @@ future writer-map revision might want one. |
| |---|---| |
| | `apps/**`, `domains/*`, `control/serial`, restart-gen — console | Confirmed, with the CLI-direct-bucket caveat above (does not change the ruling) | |
| | `control/registry-snapshot*` — per-box keys | Confirmed done (Task 1) | |
| -| `status/**` — lease-elected publisher | Confirmed (`status/fleet.json` elected; `status/boxes/<label>.json` already per-box) | |
| -| `releases/**` retention prune — housekeeper (Task 3) | Confirmed current state is every-box (`host/retention.go`); ruled `housekeeper (Task 3 pending)` as instructed. **Plus** a second every-box destructive path on `releases/**` the table doesn't mention — Finding §2 | |
| +| `status/**` — lease-elected publisher | Confirmed (`status/fleet.json` elected; `status/boxes/<label>.json` already per-box); **the election mechanism itself — `internal/lease`'s `status-lease` — is now enumerated and its deployment-scope verified explicitly, per the spec's own instruction to verify this. See Findings §1** | |
| +| `releases/**` retention prune — housekeeper (Task 3) | Confirmed current state is every-box (`host/retention.go`); ruled `housekeeper (Task 3 pending)` as instructed. **Plus** a second every-box destructive path on `releases/**` the table doesn't mention — Finding §3. **Plus** operator-invoked CLI migration writers (`accounts.go`'s `putOverwrite`/`store.Put`) — Finding §6 | |
| | usage metering day objects — audit box-scoping | Confirmed already box-scoped (`usage/edge/<box>/days/`) | |
| -| `control/certs/**`, `control/acme-challenges/**` — housekeeper (Part 2) | Not yet built (correctly absent from this tree) — but Finding §3 flags a live, unbuilt-for-N-boxes adjacent cert-issuance path (wildcard) worth Part 2/Task 4 awareness | |
| +| `control/certs/**`, `control/acme-challenges/**` — housekeeper (Part 2) | Not yet built (correctly absent from this tree) — but Finding §4 flags a live, unbuilt-for-N-boxes adjacent cert-issuance path (wildcard) worth Part 2/Task 4 awareness | |
| | `control/edges/<box>.json` — each box, own key | Not yet built (Part 3, correctly absent) | |
| | `control/edge-mesh-key` — housekeeper create-only | Not yet built (Part 3, correctly absent) | |
| -No writer discovered outside the spec's table lacks a ruling here — two |
| -(§1, `control/restore-verify.json`) genuinely need the controller's |
| -decision; the rest (§2, §3, §4) are ruled with reasoning attached. |
| +One writer discovered outside the spec's table was left genuinely unruled |
| +by the audit itself and required the controller's decision: `control/restore- |
| +verify.json` (Findings §2) — now RULED (per-box keys, issue #241). The |
| +rest (§1 status-lease, §3 PurgeApp, §4 wildcard, §5 fleet-binding, §6 CLI |
| +migration writers) are ruled with reasoning attached, not left open. |