amadan
spec: draft 4 — close review 3
Two blockers, and one of them was already suspected: the queue index and the per-tick cap disagreed. §7.3 caps work per app per tick, so on a flooded app the drain deliberately leaves items behind — and §7.1 removed the index entry whenever the generation was unchanged, which it is, because no enqueue happened. The entry pointing at those items was deleted. Same on an abort mid-app, same on a per-item error. Removal now also requires that the pass fully drained the app: listing not truncated, every item resolved, no errors. The regression test is a cap-sized flood, which is the case the cap exists for. Scanner.Status could not feed Scan. Scan takes version and size; the drain has no listing page, so a Status returning a verdict alone left it needing its own HeadObject — the exact thing keeping the token inside the scanner forbids. Status now returns the ScanObject too. Draft 2 had this hole under a different name, which is twice I have drawn a seam that could not carry the call. The third finding traces back to a fix I made in the earlier feature. clamavScanner.tag returns success WITHOUT writing when an object already carries eight foreign tags, or when the pre-write tag read is denied. Both were deliberate — they stop one object stalling a sweep — and the sweep tolerates them by re-listing every 24h. A queue item does not: the drain would report clean, delete the item, and the status door would answer pending forever, so the app's declared source of truth never arrives and the two answers it did get disagree. Scan now reports whether the verdict was persisted, the item is kept terminal, and the door answers recorded:false. And draft 3's boot fatal was worse than the problem it solved. Console env is unit-file managed and a console that will not start is left down, so shipping the build before editing env meant a dead control plane and a trip to the box — the access this design exists to remove. The retired variable is now a deprecated wiring alias, and the record is seeded once from it. Draft 3 refused to seed on the grounds that auto-enabling a control that reads tenant bytes is wrong; that argument does not apply to a migration, where refusing to seed does not withhold consent but silently withdraws consent already given. Also: the consent chain was missing Provisioned and the trash check — items enqueued before TrashApp survive the 30-day cooloff with an intact record, so the drain would have read bytes from a store the sweep will not touch. Deletion is driven by absence, never by error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+171
−45
docs/superpowers/specs/2026-09-02-scan-on-demand-design.md+171 −45
diff --git a/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md b/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md| index 68317fdb6..56c1e8708 100644 |
| --- a/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md |
| +++ b/docs/superpowers/specs/2026-09-02-scan-on-demand-design.md |
| @@ -1,6 +1,6 @@ |
| # Scan on demand: design |
| -**Status:** draft 3 (closes reviews 1 and 2). |
| +**Status:** draft 4 (closes reviews 1, 2 and 3). |
| **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| @@ -74,7 +74,7 @@ Also update `svcrole.go:41-44` ("the union of the three above") and the CLI's |
| hardcoded role list in `services.go:164` — "mintable with no further change" is |
| true of the API, not of the CLI's usage string. |
| -**`Allow` unconditionally adds `Admin`** (`svcrole.go:100`), so an Admin |
| +**`Allow` unconditionally adds `Admin`** (`svcrole.go:109`), so an Admin |
| credential reaches the new doors by construction, and Admin *can* be |
| account-scoped. That is within one account, not a tenant-boundary hole, but |
| §11.5's property is scoped to role `Service` and must say so. |
| @@ -115,8 +115,10 @@ account-scoped credential reaches every app in its account. |
| There are **two** mint doors — the API (`apiservices.go:211`) and the browser |
| form (`svcpage.go:299`, which explicitly offers "an account-wide credential"). |
| So the refusal goes in **`NewServicePrincipal`** (`svcprincipal.go:166-172`, |
| -beside the existing role parse), not in a handler, or one door forgets it. |
| -Both doors surface the same 400. |
| +beside the existing role parse), not in a handler, or one door forgets it. It |
| +needs a named sentinel — `ErrAccountScopedService` — because the API door |
| +today matches only `ErrServiceNameTaken` (`apiservices.go:211-215`) and turns |
| +anything else into a generic failure. Both doors then surface the same 400. |
| --- |
| @@ -163,11 +165,18 @@ There is no clamd timeout variable today (`clamd.Timeouts{}` defaults, |
| **Still correctly branching on wiring, and unchanged:** `appsettings.go:838-840` |
| and `walk.go:466` (Teardown must release an engine that exists regardless of |
| policy), `apiscan.go:351` (revoke's best-effort `EnsureDisabled`), |
| -`apiscan.go:419` (enforce-on needs an engine to name a tag), |
| -`objstore.go:86`, `scanner.go:89`, `server.go:522-523`. |
| +`apiscan.go:419` (enforce-on needs an engine to name a tag). |
| -`docs/site/cli.md:474-488` documents the retired variables and changes with |
| -this. |
| +**Comments and message strings naming the variable, which are reworded rather |
| +than rewired:** `objstore.go:86`, `scanner.go:89`, `server.go:522-523`, and |
| +`main.go:772`'s message. |
| + |
| +Two documents change with this: `docs/site/cli.md:474-488`, which lists the |
| +variables, and **`docs/superpowers/plans/2026-09-01-carloku-scanning-rollout.md`**, |
| +whose step 4 sets `CARLOS_STORE_SCANNER=clamav` and whose rollback is "unset it |
| +and restart". Step 4 becomes `CARLOS_CLAMD_ADDR` plus `carlos system antivirus |
| +enable`; the rollback becomes `carlos system antivirus disable`, which needs no |
| +restart — the feature's own selling point. |
| **`enable` refuses with 503 when no Scanner is wired**, or the record can claim |
| enabled with nothing behind it. |
| @@ -181,18 +190,35 @@ nil Scanner as well. That is precisely the failure `main.go:751-753,766-768` |
| refuses to allow: *"a typo that silently disabled scanning would leave an |
| operator believing a compliance control is on when nothing is running."* |
| -So: |
| - |
| -- **boot `log.Fatal`s when `CARLOS_STORE_SCANNER` is set**, naming |
| - `CARLOS_CLAMD_ADDR` and `carlos system antivirus enable`. The console will |
| - not start until the operator has read the sentence. This *is* the migration |
| - mechanism — nobody upgrades past it by accident. |
| -- `CARLOS_CLAMD_SOCKET` is **retained as an alias**; setting both to different |
| - values is fatal. |
| -- the record is **not** auto-seeded from the retired variable. Auto-enabling a |
| - control that reads tenant bytes during an upgrade is the wrong default; the |
| - rollout runs `enable` as an explicit step. |
| -- boot logs a warning when a Scanner is wired and no record exists. |
| +**Draft 3 answered this with a boot `log.Fatal` on the retired variable. That |
| +was worse than the problem.** Console env is unit-file managed, and a console |
| +that fails to start is left *down*: the box's adopt pass repoints the symlink |
| +on rollback but does not restart the unit. An operator who ships the build |
| +before editing env gets a dead control plane and has to edit a unit file on the |
| +box — the exact access §4.1 says this design should remove the need for. The |
| +other order (edit env first) requires restarting the *old* binary, which then |
| +sees no engine and stops scanning anyway. One order costs a scanning outage; |
| +the other costs a console outage. |
| + |
| +So the migration preserves behaviour instead of demanding attention: |
| + |
| +- `CARLOS_STORE_SCANNER=clamav` is accepted as a **deprecated alias for |
| + wiring**, logged loudly on every boot. Any other value stays fatal, which is |
| + today's behaviour for an unknown engine. |
| +- `CARLOS_CLAMD_SOCKET` is retained as an alias for `CARLOS_CLAMD_ADDR`; |
| + setting both to different values is fatal. |
| +- **the record is seeded enabled, once, when the retired variable is set and |
| + `system/antivirus.json` is absent**, stamped `by: "migration from |
| + CARLOS_STORE_SCANNER"`. Draft 3 refused to seed on the grounds that |
| + auto-enabling a control that reads tenant bytes is the wrong default. That |
| + reasoning does not apply to a *migration*: the operator already enabled it, |
| + explicitly, and per-app grants still gate every byte. Refusing to seed does |
| + not withhold consent — it silently withdraws consent already given, which is |
| + the failure `main.go:751-753` exists to prevent. |
| +- seeding is idempotent: it happens only when no record exists, so an operator |
| + who later runs `disable` is not overridden on the next restart. |
| +- boot warns when a Scanner is wired and no record exists, and `system |
| + antivirus status` reports "engine wired, no record" as its own state. |
| ### 4.3 The record |
| @@ -287,15 +313,32 @@ exists to protect. |
| Both the enqueue **and the drain** refuse unless all of: |
| +- `rec != nil && rec.Provisioned != nil` → else `not_granted`, the state |
| + `apiscan.go:263-266` already names |
| +- the app is not in the trash — `release.ReadAppTombstone`, which the sweep |
| + checks at `sweep.go:135-141` → else the drain deletes the items |
| - `rec.Scanning != nil && rec.Scanning.Requested != nil` → else `not_requested` |
| - `!rec.Scanning.Requested.Encrypted` → else `encrypted` |
| - `rec.Scanning.Enabled != nil` → else `not_enabled` |
| - the deployment record is enabled → else `disabled` |
| -The drain re-checks because the record can change between enqueue and drain, |
| -and the drain is where bytes are actually read. The drain also applies |
| +Draft 3 omitted the first two. The trash one matters specifically for the |
| +drain: `HasAppIn` filters `deleted_at = 0` (`store.go:1208`) so a trashed app |
| +404s at the door, but items enqueued *before* `TrashApp` sit under the prefix |
| +for the 30-day cooloff with `store.json` intact and `Enabled` non-nil. Without |
| +the tombstone check the drain would read bytes from a store the sweep |
| +deliberately will not touch. |
| + |
| +The drain re-checks all of it because the record can change between enqueue and |
| +drain, and the drain is where bytes are actually read. It also applies |
| `RunSweep`'s engine-drift refusal (`sweep.go:175-178`). |
| +**On a read error — not an absence — the drain keeps the items and the index |
| +entry, logs, and retries next tick.** `objstore.Get` returns `(nil, "", nil)` |
| +for absent and an error for failure; §7.5's "delete when `store.json` is gone" |
| +must never be implemented as "delete on any error", or a transient bucket |
| +failure discards a tenant's queue. |
| + |
| ### 5.2 Read |
| ``` |
| @@ -320,15 +363,27 @@ reaches the scanner only through `Scanner` (`scanner.go`), which cannot ask |
| about one key. So `Scanner` gains: |
| ```go |
| -// Status answers what is known about the CURRENT version of one object: |
| -// the recorded verdict when a tag matches, VerdictUnknown when the tag is |
| -// absent or stale, ErrObjectGone when the object is not there. |
| -Status(ctx context.Context, bucket, key string) (Verdict, error) |
| +// Status answers what is known about the CURRENT version of one object, and |
| +// returns the object as a listing page would have described it so a caller |
| +// can go on to Scan it without a HeadObject of its own. |
| +// |
| +// Verdict is the recorded one when a tag matches the current version, |
| +// VerdictUnknown when the tag is absent or stale. ErrObjectGone when the |
| +// object is not there. |
| +Status(ctx context.Context, bucket, key string) (Verdict, ScanObject, error) |
| ``` |
| -with `memscanner.go` and `scanner_contract_test.go` following. Keeping the |
| -comparison inside the scanner is the point: no door and no app ever handles the |
| -token. |
| +**The `ScanObject` return is not decoration.** Draft 3 had `Status` return a |
| +verdict alone, and §7.4 has the drain call `Status` before `Scan` — but `Scan` |
| +takes `(bucket, key, version, size)` (`scanner.go:42`) and the drain has no |
| +listing page. Without the object it would need its own `HeadObject`, which is |
| +exactly what keeping the version token inside the scanner forbids. Draft 2 had |
| +the same hole under a different name. |
| + |
| +`memscanner.go` and `scanner_contract_test.go` follow. `MemScanner` has no |
| +object model, so it gains a seeding call — `PutObject(bucket, key, etag string, |
| +modified time.Time, size int64)` — and the contract test asserts an absent |
| +object gives `ErrObjectGone`. |
| **`wait` long-polls**, capped at 30s, woken by **in-process notification from |
| the drain** — a polling waiter costs one tag read per second per waiter. Cap |
| @@ -338,6 +393,32 @@ concurrent waiters **globally** as well as per app. |
| reasons, because an app treating them as success reproduces the dishonesty the |
| two-number rule exists to prevent. |
| +### 5.2.1 When a verdict is reached but cannot be recorded |
| + |
| +`clamavScanner.tag` returns success **without writing** in two landed cases: |
| +an object already carrying eight foreign tags (`clamav.go:306-308`, the ten-tag |
| +cap) and a pre-write tag read that is denied or gone (`clamav.go:289-291`). |
| +Both were deliberate — they stop one object stalling a whole sweep — and the |
| +sweep tolerates them because it re-lists every 24h. |
| + |
| +**A queue item does not.** The drain would get a real verdict from `Scan`, send |
| +`clean` in the callback, delete the item — and `Status` would keep answering |
| +`pending` forever, because the tag is not there. The app's declared source of |
| +truth (§5.3) would never arrive, and the two answers it did get would disagree. |
| + |
| +So `Scan` reports whether the verdict was persisted, and when it was not: |
| + |
| +- the drain **keeps the queue item**, writing the verdict into it and marking |
| + it terminal rather than deleting it |
| +- the status door falls back to a terminal queue item when no tag is present, |
| + and answers `{"state":"clean","recorded":false}` — the verdict, plus the fact |
| + that nothing durable holds it |
| +- the item is collected by TTL like any other |
| + |
| +Retrying does not help in either case, so the item is terminal rather than |
| +pending. `recorded:false` is the honest answer: this object was scanned, and |
| +the next sweep will scan it again because nothing on it says so. |
| + |
| ### 5.3 Pull is the source of truth |
| The tag on the object is the record; the status door reads it. Webhooks |
| @@ -406,14 +487,32 @@ open is no use to it. |
| `^[a-z][a-z0-9-]{0,31}$` (`release.go:242`), so neither can contain `/`. |
| - **written by `store scan grant`**, gated by `storeSealKey` |
| (`apistore.go:425`) — one config write on an operator verb, never per enqueue |
| -- header `Carlos-Signature: t=<unix>,k=<keyid>,v1=<hex>[,v1=<hex>]` over |
| - `t + "." + body`; `keyid` names the master |
| +- header `Carlos-Signature: t=<unix>,v1=<keyid>:<hex>[,v1=<keyid>:<hex>]` over |
| + `t + "." + body`. Each signature carries its own key id; draft 3's single |
| + `k=` could not label two of them, and an app does not need the id anyway — it |
| + holds one derived key and tries it against each `v1`. |
| - **rotation carries two masters.** `CARLOS_SCAN_WEBHOOK_KEY_PREVIOUS` is |
| - accepted; during rotation the console sends a signature under **each** master |
| - and the app accepts if any verifies. `carlos system antivirus |
| - rotate-webhook-key` enumerates scanning-enabled apps and re-seals them; the |
| - previous master is dropped when it reports done. Without the two-key window |
| - every app is broken between the console's rotation and its own re-seal. |
| + accepted; the console signs under **each** master and the app accepts if |
| + either verifies. Without that window every app is broken between the |
| + console's rotation and its own re-seal. |
| + |
| +**What rotation actually does, stated because it is not free.** |
| +`appconfig.Apply` bumps the generation, and the box's Sync **restarts the app |
| +unit** on a bundle change (`sync.go:33,42,331`). So: |
| + |
| +- `rotate-webhook-key` restarts every scanning-enabled app on the deployment. |
| + It paces the re-seal rather than doing them at once, and says so before it |
| + starts. |
| +- `store scan grant` restarts the tenant's app as a side effect, because that |
| + is where the key is written. The docs sentence for `grant` says so. |
| +- **"done" means sealed, not converged.** The console cannot see when a host |
| + has picked up the new bundle — `server.go:566-569` makes exactly this point |
| + for store rotation. So dropping `_PREVIOUS` is a separate operator step after |
| + convergence, not something `rotate-webhook-key` does. Dropping it early |
| + degrades to one lost callback per stale host, which is survivable because |
| + pull is the source of truth — but it should be a choice, not a surprise. |
| +- `rotate-webhook-key` is idempotent and re-runnable after a partial failure, |
| + and is also the backfill for apps granted before this ships. |
| - constant-time compare; skew window 5 minutes |
| - the body carries account, app, key, the **bare ETag** *and* the opaque |
| version. Draft 2 carried only the version, contradicting §5.2's rule that |
| @@ -469,14 +568,26 @@ type queueIndex struct { |
| } |
| ``` |
| -- enqueue: **write the object first**, then `gen[key]++` under the mutex |
| +- enqueue: **write the object first**, then `gen[key]++` under the mutex, and |
| + only then answer 202 — a 202 sent between the two invites a client to stop |
| + retrying an item nothing points at |
| - drain: read `observed := gen[key]`, list and drain, then remove the entry |
| - **only if `gen[key] == observed`** |
| + **only if `gen[key] == observed` AND the pass fully drained the app** — |
| + listing not truncated, every listed item resolved, no errors |
| - a crash between the object write and the increment strands that item until |
| the next console start |
| - **boot rebuilds the index with one full enumeration**, and in the same pass |
| - deletes items past their TTL — that is the only thing that collects a |
| - stranded item, so it is not optional |
| + deletes items past their TTL — the only thing that collects a stranded item, |
| + so it is not optional |
| + |
| +**The "fully drained" half is what draft 3 got wrong.** §7.3 caps work per app |
| +per tick, so on a flooded app the drain deliberately leaves items behind; with |
| +generation-equality as the only condition it would then delete the entry |
| +pointing at them. The same happens when the drain aborts mid-app on an engine |
| +error — the `RunSweep` posture at `sweep.go:222-240` that the drain inherits — |
| +or when `Status` errors on one item. Those items would sit until the next boot, |
| +and the callback and long-poll wake would be lost for every capped item, on |
| +precisely the app the cap exists for. |
| ### 7.2 The drain is its own goroutine |
| @@ -512,9 +623,12 @@ ceiling. |
| ### 7.5 Deletion and the purge race |
| The drain deletes an item when it has been scanned, when its TTL has passed, |
| -when `store.json` is gone, **and when the record no longer permits scanning** |
| -(`Scanning.Enabled == nil`, or the attestation is encrypted) — otherwise those |
| -items sit until TTL doing nothing. |
| +when `store.json` is gone, when **the app is in the trash** (§5.1.1), and when |
| +the record no longer permits scanning (`Scanning.Enabled == nil`, or the |
| +attestation is encrypted) — otherwise those items sit until TTL doing nothing. |
| +An item whose verdict could not be recorded is kept, terminal, per §5.2.1. |
| + |
| +Deletion is driven by **absence**, never by error. See §5.1.1. |
| "Inherits the purge machinery" inherits the hazard too: `sweep.go:346-355` — |
| any object written under a purged prefix makes the app name permanently |
| @@ -629,6 +743,8 @@ Each is a test. |
| 7. Version comparison is inside the scanner; no door or app sees the token. |
| 8. Enqueue and drain both enforce the consent chain (§5.1.1). |
| 9. Rate limits precede enqueue; a re-request of a judged key costs a tag read. |
| +10. A verdict that could not be persisted is reported as `recorded:false`, |
| + never as `pending` and never as a silent success (§5.2.1). |
| --- |
| @@ -658,9 +774,19 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| - unknown `system/antivirus.json` schema refuses |
| - metering counts on-demand scans and not sweep scans |
| - **the index**: no item strands while the process stays up (enqueue racing a |
| - drain's removal); an item written before a crash is drained after restart; |
| - TTL'd items are collected by the boot rebuild |
| + drain's removal); **an app flooded past the per-tick cap is fully drained |
| + across successive ticks** — the test that would have caught review 3's |
| + blocker; an item written before a crash is drained after restart; TTL'd items |
| + are collected by the boot rebuild |
| - an enqueue racing `PurgeApp` leaves no orphan for longer than one drain tick |
| +- a verdict reached but not persisted answers `recorded:false`, not `pending` |
| + (§5.2.1), for both the ten-tag and denied-tag-read cases |
| +- boot warns when a Scanner is wired with no record; `enable` 503s when no |
| + Scanner is wired |
| +- boot seeds the record once from the retired variable, and does **not** |
| + override an existing record (§4.2.1) |
| +- the drain keeps items and the index entry on a record **read error**, and |
| + deletes only on absence |
| --- |