amadan
spec: draft 6 — close review 5
One blocker, and it is the same mistake in a new place. Draft 5 introduced the result object to stop a verdict going missing, and did not bind it to a version. So: v1 scanned, tag not written because the ten-tag cap fired, result object says clean; tenant overwrites with v2; the tag is still absent, the door falls through to the result object, and answers clean for bytes nobody has read. That is precisely the laundering §5.2 exists to prevent, reintroduced through the mechanism meant to close it. The result object now carries the version and the door compares it. That is the third time in this spec a fix of mine has recreated the bug class it was fixing elsewhere. The pattern is always the same shape: a new durable fact about an object, introduced without carrying the version across the seam. Two gaps a plan-writer would have had to invent. The result object had no collector — "collected by TTL" named nothing, and the boot rebuild enumerated only the queue prefix — and no purge guard, so the writeSidecar race applies with worse consequences: a stranded queue item is drained next tick, a stranded result object is found by nothing and makes the app name permanently unclaimable. It is now written behind the same store.json re-read, deleted with the app's queue items, and swept by age at boot. TTL is 24h for both kinds: one sweep cycle, after which a real verdict has had its chance. Also: the drain now applies the re-issue rule itself. Without it an object the sweep last tagged failed would be answered from that tag forever and never rescanned on request — the on-demand path strictly less able than the hourly one it exists to improve on. And ListPage becomes an optional PageLister asserted like SizeLister, rather than a method on release.Store, which has eleven implementations. Numbers the plan needs, now stated: 50 items per app per tick, 24h TTL.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+100
−30
docs/superpowers/specs/2026-09-02-scan-on-demand-design.md+100 −30
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 87531b300..6910a9069 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 5 (closes reviews 1-4). |
| +**Status:** draft 6 (closes reviews 1-5). |
| **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| @@ -228,8 +228,9 @@ So the migration preserves behaviour instead of demanding attention: |
| 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 |
| +- the write is **create-only** (`Put(…, "")`, `release/store.go:38,516` — the |
| + `IfNoneMatch:*` that `TrashApp` already relies on, |
| + `release/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 |
| @@ -237,8 +238,18 @@ So the migration preserves behaviour instead of demanding attention: |
| - **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. |
| +- **the seed fires only while the retired variable is still set**, so the |
| + upgrade order matters and the rollout must say it: deploy the new build with |
| + `CARLOS_STORE_SCANNER` still in place (one tick seeds the record), *then* |
| + swap it for `CARLOS_CLAMD_ADDR`. Swapping in the same restart as the upgrade |
| + seeds nothing and stops scanning — warned every tick, but stopped. Running |
| + `carlos system antivirus enable` also settles it at any point. |
| +- `main.go` carries "the deprecated variable was set" to the first tick as a |
| + `Config` field (`ScanSeedFromDeprecatedEnv bool`); the pass does not read the |
| + environment itself. |
| +- the first tick warns when a Scanner is wired and no record exists — not boot, |
| + which has no store — and `system antivirus status` reports "engine wired, no |
| + record" as its own state. |
| ### 4.3 The record |
| @@ -343,7 +354,7 @@ Both the enqueue **and the drain** refuse unless all of: |
| - the deployment record is enabled → else `disabled` |
| 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 |
| +drain: `HasAppIn` filters `deleted_at = 0` (`console/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 |
| @@ -450,7 +461,13 @@ 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. |
| +without a live S3 stub. The third return value touches roughly fifteen call |
| +sites in `clamav_test.go` and the contract test, not only the sweep. |
| +`MemScanner.Verdict` (`memscanner.go:89-100`) does **not** apply the re-issue |
| +rule today, so it gains it — otherwise the `Verdict`/`Status` split cannot be |
| +exercised against the fake — and `scanner_contract_test.go` pins both halves. |
| +The plan will extract a shared `readTag` helper: `Verdict` is called only from |
| +`sweep.go:215` and no such helper exists yet. |
| **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 |
| @@ -459,26 +476,52 @@ writes a **key-derived result object** and deletes the queue item as normal: |
| ``` |
| apps/<acct>/<app>/scan-results/<sha256(key)>.json |
| +{"schema":1, "key":"uploads/x.pdf", "verdict":"clean", |
| + "version":"<etag>@<RFC3339>", "at":"..."} |
| ``` |
| +**It carries the version, and that is not optional.** Draft 5 keyed it by |
| +`sha256(key)` with the verdict alone — which reintroduces, through the new |
| +mechanism, exactly the bug §5.2 exists to prevent: v1 scanned, tag not written |
| +because the cap fired, result object says clean; the tenant overwrites with v2; |
| +the tag is still absent, so the door falls through to the result object and |
| +answers `clean` for bytes nobody has read. The door compares the recorded |
| +version against the `ScanObject` that `Status` already returns and answers |
| +`pending` on a mismatch. The token stays inside the console throughout, which |
| +is what §5.2 requires. |
| + |
| - 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 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. |
| +**Written with the same guard as a sidecar.** The drain re-reads `store.json` |
| +immediately before writing, the way `writeSidecar` does (`sweep.go:358-367`), |
| +because the identical race applies: read the record, scan for seconds, a |
| +teardown and `PurgeApp` land, and a create-only write then *succeeds* under a |
| +purged prefix. A stranded queue item is drained on the next tick because its |
| +index entry survives; **a stranded result object is found by nothing**, and it |
| +makes the app name permanently unclaimable (`server.go:2921`). |
| + |
| +**Collected**, three ways — draft 5 said "by TTL" and named no collector: |
| + |
| +- deleted by the drain as soon as a later scan persists a real verdict for that |
| + key |
| +- deleted with the app's queue items whenever §7.5 deletes them for absence, |
| + tombstone, or a record that no longer permits scanning |
| +- swept by the boot rebuild (§7.1), which lists `scan-results/` as well as |
| + `scan-queue/` and collects anything older than the TTL by `ListMod` age — |
| + the name carries no timestamp, so age has to come from the store |
| + |
| +**TTL for both kinds is 24h**, one sweep cycle: after that the ordinary sweep |
| +has had its chance to write a real verdict, and a stale on-demand answer is |
| +worth less than no answer. |
| + |
| +**`infected` with `recorded:false` means enforcement is NOT in effect.** The |
| +Deny statement matches `s3:ExistingObjectTag/carlos-scan=infected` |
| +(`aws.go:159-165`), and a verdict that could not be tagged does not trigger it. |
| +The app can still read the object and must act on the verdict itself. This goes |
| +in the docs sentence too, not only here. |
| ### 5.3 Pull is the source of truth |
| @@ -638,14 +681,19 @@ type queueIndex struct { |
| - 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 |
| + (`release/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`. |
| + today: `List` returns every key under a prefix (`release/store.go:42-55`), so |
| + a flooded app's whole queue is enumerated every 30s regardless of §7.3's cap. |
| + This is an **optional interface**, `release.PageLister`, type-asserted at the |
| + call site exactly as `release.SizeLister` (`release/store.go:181-185`) is at |
| + `economicspass.go:141` — *not* a new method on `release.Store`, which has |
| + eleven implementations (`DirStore`, `S3Store`, `prefixStore`, |
| + `fleetapi.store` and seven test fakes). `DirStore` can serve it because |
| + `ListMod` is key-sorted (`release/store.go:290`), and S3 has `StartAfter` + |
| + `MaxKeys`, already used at `sweep.go:71-78`. |
| - 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 |
| @@ -673,8 +721,9 @@ state shared with the sweep is object tags, idempotent per version. |
| ### 7.3 Fairness |
| -Round-robin across apps with a per-tick cap per app, not FIFO across the |
| -deployment. FIFO lets one tenant's ten thousand objects delay every other |
| +Round-robin across apps, **50 items per app per tick** — at a 30s tick that is |
| +100/minute, close to the door's own 120/minute limit, so a well-behaved app is |
| +never throttled by the drain. Not FIFO across the deployment. FIFO lets one tenant's ten thousand objects delay every other |
| tenant's single urgent upload. |
| ### 7.4 Amplification |
| @@ -684,8 +733,16 @@ A rate limit alone does not close this: at `svcWriteBurst` (120/min, |
| ceiling. |
| - the drain calls `Status` **before** `Scan`, as `RunSweep` calls `Verdict` |
| - first (`sweep.go:215`), so a key already judged at its current version costs |
| - a tag read and nothing else |
| + first (`sweep.go:215`), so a key already judged clean, infected or skipped at |
| + its current version costs a tag read and nothing else |
| +- but the drain **applies the re-issue rule itself**: it scans when `Status` |
| + returns `Unknown`, `Failed` or `Denied`. Those two are transient by the |
| + landed code's own reasoning — "a permissions fix, a Glacier restore, an |
| + engine that was having a bad day" (`clamav.go:151-163`) — and the sweep |
| + re-issues them. Without this the on-demand path would be *less* able than the |
| + hourly one: an object the sweep last tagged `failed` would be answered from |
| + that tag forever and never rescanned on request. The cost is one failed |
| + `GetObject` on an object that is still denied. |
| - items **deduplicated by key per tick** |
| - per-app token bucket — the existing `svcWrite` limiter, keyed on principal |
| name (`svcprincipal.go:375-380`), not a new one. Exceeding it is a 429, never |
| @@ -865,7 +922,13 @@ brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| 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 |
| + (§5.2.1) |
| +- **a result object written for v1 does not answer for v2** — the door |
| + compares versions and says `pending` |
| +- a result object is not written under a prefix purged mid-scan, and is |
| + collected by the boot rebuild once past TTL |
| +- the drain rescans an object whose tag says `failed` or `denied`, rather than |
| + answering from the tag (§7.4) |
| - 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** |
| @@ -885,6 +948,13 @@ 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. |
| +- **Two new object kinds under the app prefix cost something.** `storagePass` |
| + (`economicspass.go:163`) folds queue and result bytes into the tenant's |
| + `BucketBytes`, and the three full `List("apps/")` passes |
| + (`provision/reconcile.go:586,1050`, `domains/reconcile.go:87`, |
| + `materialize.go:292`) grow with a flooded queue until the TTL collects it. |
| + All of them skip unknown key shapes correctly, so this is cost, not |
| + breakage — named so it is measured. |
| - **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, |