amadan
spec: draft 5 — close review 4
Verdict was "not ready to plan from, close". Two blockers sharing one root: I treated "tag present at current version" as the only durable fact about an object, and it is not. The landed tag reader deliberately hides denied and failed, mapping them back to unknown so the sweep retries them. That is correct as sweep retry policy and wrong as an answer to "what happened to this object" — left in place, a failed scan reads as pending forever, because the drain deletes the item as scanned and nothing ever changes the answer. Verdict keeps the re-issue rule; Status reports what the tag says. Two different questions, and separating them is the fix. The other half: draft 4 said a non-persisted verdict keeps its queue item "terminal". Items are ULID-named and the door asks by key, so that meant listing the whole queue prefix on every poll — on the flooded app the spec itself sizes at 172k items a day. Now a key-derived result object under the app prefix: one GET on the pending path, durable across restarts, and PurgeApp still covers it. Draft 4 also hid a Scanner signature change inside a sentence. Scan returns (Verdict, bool, error) now, the sweep takes _ for the flag, and its counts are unaffected because Fresh and Reused count verdicts reached. Three more that would have bitten during implementation. A deployment that set only the old variable would still have got a nil Scanner and silently stopped scanning — the exact hole the migration exists to close — because the deprecated alias was described in one section and omitted from the list in another. The seed had no home: storeScannerFor runs before openStore, so it goes at the top of the first pass tick, create only, and a read error neither seeds nor fatals. And "fully drained — listing not truncated" was vacuous, because release.Store.List never truncates; the test is a listing that returned zero items, and the drain needs a bounded oldest-first listing that does not exist yet. Also corrected: draft 4 claimed the box's adopt pass does not restart a unit on rollback. WithStartUnit exists precisely to do that. The decision it was supporting stands on its other leg.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+132
−43
docs/superpowers/specs/2026-09-02-scan-on-demand-design.md+132 −43
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 56c1e8708..87531b300 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 4 (closes reviews 1, 2 and 3). |
| +**Status:** draft 5 (closes reviews 1-4). |
| **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| @@ -114,7 +114,7 @@ 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`, |
| +So the refusal goes in **`NewServicePrincipal`** (`svcprincipal.go:170-172`, |
| 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 |
| @@ -147,7 +147,12 @@ There is no clamd timeout variable today (`clamd.Timeouts{}` defaults, |
| **Every place that keys off `CARLOS_STORE_SCANNER` or a nil Scanner:** |
| 1. `storeScannerFor` (`main.go:755-758`) builds the Scanner whenever |
| - `CARLOS_CLAMD_ADDR` (or the retained `CARLOS_CLAMD_SOCKET` alias) is set. |
| + `CARLOS_CLAMD_ADDR`, the retained `CARLOS_CLAMD_SOCKET` alias, **or the |
| + deprecated `CARLOS_STORE_SCANNER=clamav`** is set — the last of those |
| + retaining today's socket default (`/run/clamav/clamd.ctl`, `main.go:774`). |
| + Without that third case a deployment that set only the old variable gets a |
| + nil Scanner, no goroutine (`scanpass.go:37-39`) and nothing to seed a record |
| + from: scanning stops silently, which is what §4.2.1 exists to prevent. |
| Safe because `NewClamAVScanner` does not ping (`clamav.go:56-70`). |
| `main.go:772`'s fatal when no store backend is configured stays. |
| 2. `StartScanPass` (`scanpass.go:37-39`) loops whenever `Scanner != nil` and |
| @@ -155,7 +160,7 @@ There is no clamd timeout variable today (`clamd.Timeouts{}` defaults, |
| tick does nothing — not that there is no goroutine. That is what makes |
| `enable` restartless. A per-tick GET of one small object is negligible |
| against the enumeration the tick already does. |
| -3. `scan grant` (`apiscan.go:251`) and `scan sweep` (`scanpass.go:198`) check |
| +3. `scan grant` (`apiscan.go:252`) and `scan sweep` (`scanpass.go:198`) check |
| the **record**, and 503 naming `carlos system antivirus enable`. |
| 4. `RunSweep`'s refusal (`sweep.go:163`) checks the record. |
| 5. `apiscan.go:71-81` — the `engine_error` branch — reports *disabled* as its |
| @@ -191,14 +196,18 @@ refuses to allow: *"a typo that silently disabled scanning would leave an |
| operator believing a compliance control is on when nothing is running."* |
| **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 |
| +was worse than the problem.** Console env is unit-file managed, so an operator |
| +who ships the build before editing env gets a console that will not start and |
| +has to edit a unit file on the box — the exact access §4.1 says this design |
| +should remove the need for. (Draft 4 justified this partly by claiming the |
| +box's adopt pass does not restart the unit on rollback. **That is wrong** — |
| +`WithStartUnit` (`adopt.go:204-215`) exists to start the unit after a |
| +rolled-back failed restart, and the host agent passes it in production |
| +(`host/agent.go:602,1155,2960`), so a fataling console is rolled back *and* |
| +started on the old build. The decision stands on the other leg alone.) 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. |
| +the other costs a trip to the box. |
| So the migration preserves behaviour instead of demanding attention: |
| @@ -215,8 +224,19 @@ So the migration preserves behaviour instead of demanding attention: |
| 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. |
| +- **where the seed happens**: at the top of the first `StartScanPass` tick — |
| + the same record read §4.2 item 2 already performs — not in |
| + `storeScannerFor`, which runs at `main.go:262` before `openStore()` at |
| + `:269` and has no release store to write to. |
| +- the write is **create-only** (`Put(…, "")`, `store.go:38,516` — the |
| + `IfNoneMatch:*` that `TrashApp` already relies on, `trash.go:51-54`), so two |
| + consoles or a restart loop converge; `ErrCASConflict` means "someone else |
| + seeded it" and is success. |
| +- seeding is therefore idempotent: it happens only when no record exists, so an |
| + operator who later runs `disable` is not overridden on the next restart. |
| +- **a read error at boot neither seeds nor fatals** — log and retry on the next |
| + tick. Seeding on a failed read would enable scanning on a deployment that may |
| + have disabled it. |
| - boot warns when a Scanner is wired and no record exists, and `system |
| antivirus status` reports "engine wired, no record" as its own state. |
| @@ -250,7 +270,7 @@ type and a new door, not a new magic string. |
| ### 4.4 The doors |
| ``` |
| -carlos system antivirus status|enable|disable |
| +carlos system antivirus status|enable|disable|rotate-webhook-key |
| ``` |
| A deployment-wide door has **no `{account}` segment**, so `apiPrincipalAccount` |
| @@ -290,7 +310,7 @@ enabled record. |
| POST /api/cli/apps/{account}/{app}/store/scan/object |
| {"key": "uploads/x.pdf", "callback_path": "/hooks/scan"} // callback_path optional |
| → 202 {"state":"pending"} |
| -→ 409 {"state":"unavailable","reason":"not_requested"|"encrypted"|"not_enabled"} |
| +→ 409 {"state":"unavailable","reason":"not_granted"|"not_requested"|"encrypted"|"not_enabled"} |
| → 503 {"state":"unavailable","reason":"disabled"|"no_engine"} |
| → 429 on rate limit |
| ``` |
| @@ -346,6 +366,8 @@ GET .../store/scan/object?key=uploads/x.pdf&wait=15s |
| → {"state":"clean","at":"...","engine":"clamav","signature":"..."} |
| → {"state":"infected","signature":"Eicar-Test"} |
| → {"state":"skipped","reason":"too_large"} |
| +→ {"state":"failed","detail":"..."} // the engine read it and could not judge it |
| +→ {"state":"denied"} // the console may not read it |
| → {"state":"pending"} |
| → {"state":"gone"} // object does not exist |
| → {"state":"unavailable","reason":"..."} // as §5.1 |
| @@ -373,6 +395,15 @@ about one key. So `Scanner` gains: |
| Status(ctx context.Context, bucket, key string) (Verdict, ScanObject, error) |
| ``` |
| +**`Status` reads the tag as written; it does not apply `Verdict`'s re-issue |
| +rule.** The landed reader maps a stored `denied` or `failed` tag back to |
| +`VerdictUnknown` (`clamav.go:160-163`) so the sweep retries it — correct as |
| +*sweep retry policy*, and wrong as *an answer to a question about an object*. |
| +Left in place here, a failed scan reads as `pending` forever: the drain deletes |
| +the item as scanned, and nothing ever changes the answer until a sweep re-fails |
| +it. `Verdict` keeps the re-issue rule; `Status` reports what the tag says. |
| +Different questions, and the split is the point. |
| + |
| **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 |
| @@ -380,10 +411,12 @@ 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`. |
| +`Status` needs `HeadObject`, which `scanS3API` (`clamav.go:25-29`) and |
| +`stubScanS3` (`clamav_test.go:24`) both gain. `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 |
| @@ -406,18 +439,46 @@ sweep tolerates them because it re-lists every 24h. |
| `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 `Scanner` interface changes**, which draft 4 hid inside a sentence: |
| + |
| +```go |
| +Scan(ctx, bucket, key, version string, size int64) (Verdict, bool, error) |
| +// ^ recorded |
| +``` |
| + |
| +The sweep's call site (`sweep.go:235`) takes `_` for the flag and its counts |
| +are unaffected — Fresh and Reused count verdicts *reached*, which is what |
| +`clamav.go:302-303` already says it is doing. `MemScanner.Scan` |
| +(`memscanner.go:102`) gains an `Unrecordable(key)` hook so the case is testable |
| +without a live S3 stub. |
| + |
| +**Where the answer goes when the tag cannot hold it.** Draft 4 said "keep the |
| +queue item, terminal" — but items are ULID-named and the door asks by key, so |
| +that would mean listing the whole queue prefix on every poll. Instead the drain |
| +writes a **key-derived result object** and deletes the queue item as normal: |
| -- 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 |
| +``` |
| +apps/<acct>/<app>/scan-results/<sha256(key)>.json |
| +``` |
| + |
| +- one GET for the door, on the `pending` path only |
| +- durable across restarts, so §11.10 holds without soft state |
| +- under the app prefix, so `PurgeApp` and the claim gate cover it (§7.1's |
| + argument for keeping the queue there applies unchanged) |
| +- deleted by the drain as soon as a later scan *does* persist a verdict, and |
| + collected by TTL otherwise |
| + |
| +The door reads the tag first (`Status`); on `VerdictUnknown` it reads the |
| +result object; absent, it answers `pending`. A result object answers |
| +`{"state":"clean","recorded":false}` — the verdict, plus the fact that nothing |
| +on the object itself says so, which is why the next sweep will scan it again. |
| -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. |
| +Retrying does not help in either case, so this is terminal rather than pending. |
| + |
| +Note the denied-tag-read branch (`clamav.go:289-291`) is nearly unreachable |
| +from the drain: `Status` reads tags first and returns `VerdictDenied` |
| +(`clamav.go:130-134`), so `Scan` is never called. The ten-tag cap |
| +(`clamav.go:306-308`) is the live case. |
| ### 5.3 Pull is the source of truth |
| @@ -512,7 +573,10 @@ unit** on a bundle change (`sync.go:33,42,331`). So: |
| 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. |
| + and is also the backfill for apps granted before this ships. A re-run with an |
| + unchanged master re-seals the same plaintext and therefore does **not** |
| + restart anything: Sync restarts only when the rendered bundle changes |
| + (`sync.go:340+`). |
| - 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 |
| @@ -571,17 +635,25 @@ type queueIndex struct { |
| - 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` AND the pass fully drained the app** — |
| - listing not truncated, every listed item resolved, no errors |
| +- drain: read `observed := gen[key]`, drain, then remove the entry **only if |
| + `gen[key] == observed` and the listing returned zero items**. Draft 4 said |
| + "fully drained — listing not truncated"; `release.Store.List` never truncates |
| + (`store.go:555-569`), so that clause was vacuous. Zero-items is the test that |
| + actually means "nothing is left", and it costs one empty listing per tick |
| + until the entry goes. |
| +- the drain needs a **bounded, oldest-first listing**, which does not exist |
| + today: `List` returns every key under a prefix (`store.go:42-55`), so a |
| + flooded app's whole queue is enumerated every 30s regardless of §7.3's cap. |
| + Add `ListPage(prefix, startAfter string, limit int)` to the release store, or |
| + page the S3 lister with `MaxKeys`. |
| - 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 — 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 |
| +**The removal condition 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 — |
| @@ -622,13 +694,23 @@ 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, 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. |
| +This is the single deletion list; §5.1.1's "refuse" means "does not read |
| +bytes", not "does not delete". |
| + |
| +**Delete the item** when it has been scanned (including §5.2.1, where a result |
| +object is written first); when its TTL has passed; when `store.json` is absent; |
| +when the app is in the trash; and when the record no longer permits scanning — |
| +`Provisioned == nil`, `Requested == nil`, the attestation is encrypted, or |
| +`Scanning.Enabled == nil`. None of those can change back in a way that makes |
| +the item useful. |
| + |
| +**Keep the item** when the deployment record is `disabled`, on engine drift, on |
| +an unreachable engine, and on any read error. All of those are conditions an |
| +operator can reverse. |
| -Deletion is driven by **absence**, never by error. See §5.1.1. |
| +Deletion is driven by **absence**, never by error — and absence at the release |
| +store layer is `os.ErrNotExist` (`trash.go:64`), not a nil value, so queue |
| +items and the system record must test for it explicitly. |
| "Inherits the purge machinery" inherits the hazard too: `sweep.go:346-355` — |
| any object written under a purged prefix makes the app name permanently |
| @@ -744,7 +826,8 @@ Each is a test. |
| 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). |
| + never as `pending` and never as a silent success (§5.2.1); and a `failed` |
| + or `denied` verdict is reported as itself, never as `pending` (§5.2). |
| --- |
| @@ -770,7 +853,9 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| touching per-app records — and per-app `scan status` then reports the |
| deployment state rather than claiming health |
| - `request`, `revoke`, `enforce off` still work while disabled |
| -- boot fatals when `CARLOS_STORE_SCANNER` is set (§4.2.1) |
| +- boot accepts `CARLOS_STORE_SCANNER=clamav` as a deprecated wiring alias, |
| + retaining the socket default; any other value stays fatal (§4.2.1) |
| +- a `failed` and a `denied` verdict each answer as themselves, not `pending` |
| - 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 |
| @@ -800,4 +885,8 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| killing the console mid-queue. |
| - **Boot rebuild cost.** One full enumeration at start on a deployment with |
| many apps. Measure it. |
| +- **Index size is bounded** by the number of apps that have enqueued since boot |
| + and not yet been seen empty — at most the number of provisioned stores, a few |
| + dozen bytes each. Trashed and purged apps clear on their next empty listing, |
| + since `PurgeApp` deletes the items (`trash.go:229-238`). |
| - **Long-poll occupancy.** Bounded wait, global and per-app waiter caps. |