spec: draft 3 — close review 2
Three blockers. One of them is mine to own. The consent chain had a door straight through it. The landed sweep refuses to read tenant bytes without the member's attestation, skips stores attested client-encrypted, and scan grant refuses without a request — on the stated ground that an operator must not start reading someone's objects on their own. Draft 2's enqueue checked only the deployment switch, so an app's own credential could make the console fetch and scan a store that was never requested or was attested encrypted. Enqueue AND drain now enforce all four conditions, the drain because the record can change between the two and the drain is where bytes are actually read. The upgrade would have silently stopped scanning. The pass reads a record that exists on no current deployment, and CARLOS_CLAMD_SOCKET was renamed underneath operators. That is exactly the failure main.go already refuses to permit. Boot now fatals when the retired variable is set, naming its replacement and the enable command — the fatal IS the migration, since nobody upgrades past it by accident. The record is not auto-seeded: silently enabling a control that reads tenant bytes during an upgrade is the wrong default. The queue index had a stranding race with no removal rule stated: drain lists empty, enqueue writes, enqueue marks, drain unmarks, item invisible. Now a generation counter under a mutex, object written before the mark, removal only if the generation is unchanged — and the boot rebuild collects stranded and expired items, which makes it load-bearing rather than an optimisation. Draft 2's claim that losing the index "costs latency, never items" was also wrong twice over: the fallback sweep is 24h not hourly, and the lost thing is the callback. Smaller corrections: two mint doors exist, so the account-scope refusal goes in NewServicePrincipal where neither can forget it; Allow() admits Admin by construction, so the isolation property is scoped to role Service and says so; the Scanner interface needs a Status method because scanS3API is the scanner's private seam and the door only holds a Scanner; the webhook key CAN be delivered like a store credential — what cannot be reused is keeping nothing, since the console must sign later — and rotation needs a two-master window or every app breaks between the console rotating and its own re-seal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 file changed,
+382
−282
docs/superpowers/specs/2026-09-02-scan-on-demand-design.md+382 −282
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 b4d2ba51e..68317fdb6 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,23 +1,23 @@ |
| # Scan on demand: design |
| -**Status:** draft 2 (closes review 1). |
| +**Status:** draft 3 (closes reviews 1 and 2). |
| **Builds on:** `2026-09-01-object-storage-virus-scanning-design.md` — the sweep, |
| the `Scanner` seam, verdict tags, enforcement. Landed and reviewed. |
| **It contradicts that spec in one place, deliberately.** Prior spec §4.1 |
| records: *"No TCP option in v1: an unauthenticated scanning service reachable |
| over a network is a worse problem than the one being solved. **(decision)**"*. |
| -§9 here reverses it. The reversal is argued there rather than assumed. |
| +§9 reverses it, and argues the reversal rather than assuming it. |
| --- |
| ## 1. What is being asked |
| -The shipped scanner answers "is this bucket clean?" on an hourly cycle. It does |
| -not answer "is *this object* clean, now?" — the question an app has when |
| -someone has just uploaded a file it is about to serve. The current docs tell |
| -app authors to solve that themselves, which is the platform declining work it |
| -is better placed to do. |
| +The shipped scanner answers "is this bucket clean?" hourly. It does not answer |
| +"is *this object* clean, now?" — the question an app has when someone has just |
| +uploaded a file it is about to serve. The current docs tell app authors to |
| +solve that themselves, which is the platform declining work it is better placed |
| +to do. |
| **The delay is scheduling, not scanning.** `clamd` answers on a typical upload |
| in well under a second. So this is queueing and notification. |
| @@ -27,21 +27,20 @@ in well under a second. So this is queueing and notification. |
| > Object `<key>` was scanned at `<T>` against `<engine>/<signature-db>` and |
| > found clean. |
| -plus the sentence that makes the first one worth anything, unchanged: |
| +plus the sentence that makes the first one worth anything: |
| > `N` objects in this store carry no verdict. |
| On-demand scanning shortens T. It does not make scanning synchronous with |
| -upload: **CARLOS never sees the PUT**, and nothing here changes that. |
| +upload: **CARLOS never sees the PUT**. |
| --- |
| ## 2. Scope |
| -In: a new service role; `carlos system antivirus`; a priority scan door and a |
| -long-polling status door; a queue that survives restarts and cannot be starved |
| -by one tenant; an opt-in signed webhook; metering, recorded and unpriced; |
| -clamd over TCP on a services box. |
| +In: `svcrole.Service`; `carlos system antivirus`; a priority scan door and a |
| +long-polling status door; a queue; an opt-in signed webhook; metering, recorded |
| +and unpriced; clamd over TCP on a services box. |
| Out (§12): synchronous scan on PUT, GuardDuty, Gotenberg as a capability, |
| pricing. |
| @@ -52,66 +51,72 @@ pricing. |
| ### 3.1 Why not `Instance` |
| -`svcrole.go:34-36` describes `Instance` as *"the LEGACY app-token authority, |
| -defined by observation of today's call sites rather than by its name."* A role |
| -defined by what happens to call it is not one to hang a new API class on, and a |
| -token minted to read logs must not silently gain the ability to spend |
| -deployment CPU. |
| +`svcrole.go:34-36` calls `Instance` *"the LEGACY app-token authority, defined by |
| +observation of today's call sites rather than by its name."* A role defined by |
| +what happens to call it is not one to hang a new API class on, and a token |
| +minted to read logs must not silently gain the ability to spend deployment CPU. |
| -This is enforced, not merely intended: |
| -`TestInstanceRoleMatchesTodaysAppTokenReach` (`svcroutes_test.go:262-300`) pins |
| +Enforced, not merely intended: |
| +`TestInstanceRoleMatchesTodaysAppTokenReach` (`svcroutes_test.go:262-291`) pins |
| `Instance` to exactly nine doors and fails with "widening the legacy role". |
| ### 3.2 `svcrole.Service` |
| -A fifth role: **an app, at runtime, asking a platform service to do work on its |
| -behalf** — work the platform performs on the app's own data that costs the |
| -deployment resources. Scanning is the first; PDF rendering is the next. |
| +**An app, at runtime, asking a platform service to do work on its behalf** — |
| +work the platform performs on the app's own data that costs the deployment |
| +resources. Scanning is the first; PDF rendering is next. |
| ```go |
| Service Role = "service" |
| ``` |
| -Also update `svcrole.go:41-44`, which describes `Admin` as "the union of the |
| -three above". |
| +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. |
| -### 3.3 There is no migration — this replaced draft 1's §3.3 |
| +**`Allow` unconditionally adds `Admin`** (`svcrole.go:100`), 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. |
| -Draft 1 flagged this as the document's biggest risk and proposed a |
| -transitional `Allow(Service, Instance)`. Both the risk and the fallback were |
| -wrong. How credentials actually work: |
| +### 3.3 There is no migration |
| + |
| +Draft 1 called this the document's biggest risk and proposed |
| +`Allow(Service, Instance)`. Both were wrong. |
| | kind | minted by | roles | rotatable | |
| |---|---|---|---| |
| | person (`kind=""`) | `carlos auth login` | none — never meets the role gate (`authcli.go:425`) | n/a | |
| -| legacy app token (`kind="app"`) | a person, `POST .../instance-token` | `Instance`, **hardcoded** at `svcprincipal.go:124` | **no** (`svcprincipal.go:244-246`) | |
| -| named service (`kind="service"`) | account **owner**, `POST /api/cli/services/{account}` | any role `svcrole.Parse`s (`apiservices.go:180`) | yes | |
| +| legacy app token (`kind="app"`) | a person, `POST .../instance-token` | `Instance`, hardcoded (`svcprincipal.go:124`) | **no** (`svcprincipal.go:244-246`) | |
| +| named service (`kind="service"`) | account **owner** (`apiservices.go:102-112,143`) | any role `svcrole.Parse`s (`apiservices.go:180`) | yes | |
| -**Adding `Service` to the catalogue makes it mintable with no further change** |
| -— the mint door, the mint form (`svcpage.go:176`) and `doorsFor` all read |
| -`svcrole.All()`. |
| +Adding `Service` to the catalogue makes it mintable: the mint door, the mint |
| +form (`svcpage.go:176`) and `doorsFor` all read `svcrole.All()`. |
| -Nothing migrates. A legacy `kind='app'` token can never become `Service`, so an |
| -app that wants on-demand scanning is issued a *new* credential: |
| +Nothing migrates. A `kind='app'` token can never become `Service`, so an app |
| +that wants on-demand scanning is issued a *new* credential: |
| ``` |
| carlos services create -account <a> -app <app> -role service |
| carlos secrets set -app <app> CARLOS_SCAN_TOKEN=<...> |
| ``` |
| -which is the same delivery step it already performed for its instance token. |
| -Per-app, opt-in, one command. **The `Allow(Service, Instance)` fallback is |
| -deleted**: it cannot terminate (legacy tokens do not rotate into anything) and |
| -it would fail the pinned test above — the exact widening §3.1 argues against. |
| +the same delivery step it already performed for its instance token. |
| +**`Allow(Service, Instance)` is deleted**: it cannot terminate (legacy tokens |
| +do not rotate into anything) and it would fail the pinned test above — the |
| +exact widening §3.1 argues against. |
| -### 3.4 Scope must be per-app at mint time |
| +### 3.4 Scope must be per-app, enforced in one place |
| -`NewServicePrincipal` permits `app == ""` (`svcprincipal.go:140-141`), and |
| -`apiAccountAppBy` binds only when `p.App != ""` (`apicli.go:70`). An |
| -account-scoped credential therefore reaches every app in the account. |
| +`NewServicePrincipal` permits `app == ""` (`svcprincipal.go:140-141`) and |
| +`apiAccountAppBy` binds only when `p.App != ""` (`apicli.go:70`), so an |
| +account-scoped credential reaches every app in its account. |
| -**The mint door refuses account scope for role `Service`.** Without that, |
| -§11.4 is false. |
| +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. |
| --- |
| @@ -120,45 +125,80 @@ account-scoped credential therefore reaches every app in the account. |
| ### 4.1 Wiring versus policy |
| `CARLOS_STORE_SCANNER` conflates *there is a clamd at this address* with |
| -*scanning is on*. Wiring belongs in the unit file. Policy does not: turning a |
| +*scanning is on*. Wiring belongs in the unit file; policy does not. Turning a |
| compliance control on should not mean editing a systemd unit and restarting the |
| control plane, and today the only way to know whether scanning is healthy is to |
| -log into the box — which the no-infra-access rule says it must not be. |
| +log into the box — which the no-infra-access rule forbids. |
| | what | where | |
| |---|---| |
| -| `CARLOS_CLAMD_ADDR` (`host:port` or a socket path) | env, boot | |
| -| `CARLOS_CLAMD_MAX_BYTES`, timeouts | env, boot | |
| -| `CARLOS_SCAN_WEBHOOK_KEY` (§6.3) | env, boot | |
| +| `CARLOS_CLAMD_ADDR` — `host:port`, or a path when it begins `/` | env, boot | |
| +| `CARLOS_CLAMD_MAX_BYTES` | env, boot | |
| +| `CARLOS_SCAN_WEBHOOK_KEY` (+ `_PREVIOUS`, §6.3) | env, boot | |
| | enabled / disabled, engine, at, by | **record** | |
| -### 4.2 What replaces the env var, concretely |
| +There is no clamd timeout variable today (`clamd.Timeouts{}` defaults, |
| +`clamav.go:71`); this spec does not add one. |
| + |
| +### 4.2 What replaces the env var |
| -Draft 1 retired `CARLOS_STORE_SCANNER` without saying what took over. Six |
| -places key off it. All of them change: |
| +**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`** is set. This is safe because `NewClamAVScanner` does |
| - not ping (`clamav.go:56-70`) — a Scanner can be constructed from wiring |
| - alone. The `main.go:772` fatal when no store backend is configured stays. |
| -2. `StartScanPass` (`scanpass.go:37-39`) loops whenever `Scanner != nil`, and |
| - **reads `system/antivirus.json` at the top of each tick**. Disabled means |
| - the tick does nothing — not that there is no goroutine. That is what makes |
| - `enable` restartless. |
| -3. `scan grant` and `scan sweep` check the **record**, not the nil-ness of the |
| - Scanner, and 503 naming `carlos system antivirus enable`. |
| -4. The `apiscan.go:14-20` rule is preserved verbatim for the disabled state: |
| - `request`, `revoke` and `enforce off` must keep working when scanning is |
| - off, or disabling strands every enforcing record. |
| -5. `RunSweep`'s refusal (`sweep.go:163`) becomes a check against the record. |
| -6. Error strings naming `CARLOS_STORE_SCANNER` (`apiscan.go:45`) name the |
| - command instead. |
| + `CARLOS_CLAMD_ADDR` (or the retained `CARLOS_CLAMD_SOCKET` alias) is set. |
| + 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 |
| + **reads `system/antivirus.json` at the top of each tick**. Disabled means the |
| + 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 |
| + 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 |
| + own state, distinct from *unreachable*. |
| +6. `apiscan.go:45`'s message names the command, not the variable. |
| + |
| +**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`. |
| + |
| +`docs/site/cli.md:474-488` documents the retired variables and changes with |
| +this. |
| + |
| +**`enable` refuses with 503 when no Scanner is wired**, or the record can claim |
| +enabled with nothing behind it. |
| + |
| +### 4.2.1 Migration — an upgrade must not silently stop scanning |
| + |
| +Today `CARLOS_STORE_SCANNER=clamav` is the only gate. After this change the |
| +pass reads a record that **exists on no current deployment**, so every tick |
| +would do nothing, and an operator who did not touch the unit file would get a |
| +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. |
| ### 4.3 The record |
| -`system/antivirus.json` — deployment-scoped, sibling in spirit to |
| -`infraview.InventoryKey` (`internal/infraview/infraview.go:25`), but the helper |
| -lives beside `release.FeaturesKey` (`internal/release/features.go:53`): |
| +`system/antivirus.json`, deployment-scoped — sibling in spirit to |
| +`infraview.InventoryKey` (`internal/infraview/infraview.go:25`), with the |
| +helper beside `release.FeaturesKey` (`internal/release/features.go:53`): |
| ```go |
| // package release |
| @@ -174,46 +214,45 @@ type AntivirusSettings struct { |
| ``` |
| CAS read/write, schema pinned, unknown schema refuses — the rules |
| -`objstore.Record` and `ScanSidecar` follow, for the same reason: this one |
| -drives whether tenant bytes get read. |
| +`objstore.Record` and `ScanSidecar` follow, and for the same reason: this one |
| +decides whether tenant bytes get read. |
| -**Typed record per feature, not a generic settings map.** A map is |
| -`features.json` by another name — uninterpreted and impossible to schema-pin. |
| -`system/pdf.json` later is a new type and a new door, not a new magic string. |
| +**A typed record per feature, not a generic map.** A map is `features.json` by |
| +another name: uninterpreted and unpinnable. `system/pdf.json` later is a new |
| +type and a new door, not a new magic string. |
| -### 4.4 The doors, and the authority problem |
| +### 4.4 The doors |
| ``` |
| carlos system antivirus status|enable|disable |
| ``` |
| -**A deployment-wide door has no `{account}` segment**, so |
| -`apiPrincipalAccount` (`apicli.go:176-187`) cannot resolve it. These need a |
| -person-only resolver, registered with `svcrole.Set{}` (no service principal may |
| -reach them) and added to `closedDoors` or `svcroutes_test.go:83+` fails on the |
| -equality check. |
| +A deployment-wide door has **no `{account}` segment**, so `apiPrincipalAccount` |
| +(`apicli.go:176-187`) cannot resolve it. Precedent exists: `GET /api/cli/apps` |
| +(`authcli.go:471`) is already a no-`{account}` door. These need a person-only |
| +resolver, registered `svcrole.Set{}` so no service principal reaches them, and |
| +added to `closedDoors` (`svcroutes_test.go:83`) or |
| +`TestEveryCLIDoorNamesItsRoles` (`svcroutes_test.go:140-172`) fails. |
| -Authority is then `storeOperator(addr)` — the `CARLOS_STORE_OPERATORS` address |
| -list that already governs `store grant` (`apistore.go:520`) and every scan |
| -operator verb (`apiscan.go:245, 333, 396, 514`). Draft 1 said "not `Admin` |
| -alone", which confused a service role with person authority: operators are |
| -addresses (`server.go:558-571`) and a service principal's actor is |
| -`service:…`/`app:…` (`apicli.go:89-100`), which can never match one. |
| +Authority is `storeOperator(addr)` — the `CARLOS_STORE_OPERATORS` list that |
| +already governs `store grant` (`apistore.go:520`) and every scan operator verb. |
| +Draft 1's "not `Admin` alone" confused a service role with person authority: |
| +operators are addresses (`server.go:558-571`), a service actor is |
| +`service:`/`app:` (`apicli.go:89-100`), and the two can never match. |
| `status` answers "is scanning healthy" without SSH: engine version, whether it |
| -answers at all, signature age, queue depth. |
| +answers, signature age, queue depth. |
| ### 4.5 Two layers |
| -Deployment enable does not scan anything. Per-app `store scan request` (member) |
| -and `store scan grant` (operator) are unchanged. Collapsing the layers would |
| -either scan every bucket the moment an operator enabled the engine, or make the |
| -deployment switch meaningless. |
| +Deployment enable scans nothing by itself. Per-app `store scan request` |
| +(member) and `store scan grant` (operator) are unchanged. |
| -**Per-app `store scan status` must surface the deployment state.** Otherwise a |
| -record saying `enabled: clamav` reads as healthy while the deployment switch is |
| -off and nothing runs — a new way for the status output to lie, which is the one |
| -thing the shipped design was careful about. |
| +**Per-app `store scan status` must surface the deployment state**, or a record |
| +saying `enabled: clamav` reads as healthy while nothing runs — a new way for |
| +the status output to lie. Note `apiscan_test.go:254` asserts `engine == |
| +"clamav"` on a fixture with a Scanner and no record; that fixture must write an |
| +enabled record. |
| --- |
| @@ -223,17 +262,39 @@ thing the shipped design was careful about. |
| ``` |
| POST /api/cli/apps/{account}/{app}/store/scan/object |
| -{"key": "uploads/x.pdf"} |
| +{"key": "uploads/x.pdf", "callback_path": "/hooks/scan"} // callback_path optional |
| → 202 {"state":"pending"} |
| -→ 503 {"state":"unavailable","reason":"disabled"} |
| +→ 409 {"state":"unavailable","reason":"not_requested"|"encrypted"|"not_enabled"} |
| +→ 503 {"state":"unavailable","reason":"disabled"|"no_engine"} |
| +→ 429 on rate limit |
| ``` |
| Role `Service`. It enqueues; it does not scan inline. Inline scanning would |
| hold a console goroutine for the duration and hand one tenant a trivial way to |
| occupy them all. |
| -**Enqueue refuses while scanning is disabled**, or items accumulate forever |
| -against a drain that will never run. |
| +### 5.1.1 The consent chain applies here too |
| + |
| +**This was the hole in draft 2.** The landed sweep refuses to read tenant bytes |
| +without the member's attestation (`sweep.go:151-160`), skips stores attested |
| +encrypted (`:147-149`), and `scan grant` refuses without a request |
| +(`apiscan.go:268-285`) on the stated ground that *an operator must not be able |
| +to start that on their own*. Draft 2's enqueue checked only the deployment |
| +switch — so an app's own credential could make the console fetch and scan |
| +objects in a store that was never requested, never granted, or attested |
| +client-encrypted. A new door straight through the property the whole feature |
| +exists to protect. |
| + |
| +Both the enqueue **and the drain** refuse unless all of: |
| + |
| +- `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 |
| +`RunSweep`'s engine-drift refusal (`sweep.go:175-178`). |
| ### 5.2 Read |
| @@ -243,74 +304,85 @@ GET .../store/scan/object?key=uploads/x.pdf&wait=15s |
| → {"state":"infected","signature":"Eicar-Test"} |
| → {"state":"skipped","reason":"too_large"} |
| → {"state":"pending"} |
| -→ {"state":"unavailable","reason":"no_engine"|"disabled"} |
| +→ {"state":"gone"} // object does not exist |
| +→ {"state":"unavailable","reason":"..."} // as §5.1 |
| ``` |
| -**The console does the version comparison.** The tag records `etag@RFC3339` |
| -(`sweep.go:52-54`). An app must not reconstruct that: it is an internal format, |
| -and an app comparing it wrongly gets the laundering bug *as a correctness bug* |
| -— upload v2 over a key whose v1 was tagged clean, read the stale tag, serve the |
| -payload. The door compares and answers `pending` when the tag is absent **or** |
| -stale. |
| +**The console does the version comparison, and the token never leaves it.** The |
| +tag records `etag@RFC3339` (`sweep.go:49-51`). An app must not reconstruct |
| +that: it is an internal format, and an app comparing it wrongly gets the |
| +laundering bug *as a correctness bug* — upload v2 over a key whose v1 was |
| +tagged clean, read the stale tag, serve the payload. |
| -This needs `HeadObject`, which `scanS3API` (`clamav.go:26-30`) does not have. |
| -The seam widens; the permission is already there. |
| +Draft 2 said "the seam widens" for `HeadObject`. That understates it: |
| +`scanS3API` (`clamav.go:25-29`) is the **scanner's private** seam, and the door |
| +reaches the scanner only through `Scanner` (`scanner.go`), which cannot ask |
| +about one key. So `Scanner` gains: |
| -**`wait` long-polls**, capped at 30s. Waking is **in-process notification from |
| -the drain**, not per-waiter polling of S3 — a polling waiter costs one |
| -`GetObjectTagging` per second per waiter. Cap concurrent waiters **globally** |
| -as well as per app; otherwise long-poll is a cheap way to occupy the console. |
| +```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) |
| +``` |
| + |
| +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. |
| + |
| +**`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 |
| +concurrent waiters **globally** as well as per app. |
| -**`skipped` and `unavailable` are not `clean`.** Distinct states with reasons, |
| -because an app treating them as success reproduces exactly the dishonesty the |
| +**`skipped`, `gone` and `unavailable` are not `clean`.** Distinct states with |
| +reasons, because an app treating them as success reproduces the dishonesty the |
| two-number rule exists to prevent. |
| ### 5.3 Pull is the source of truth |
| The tag on the object is the record; the status door reads it. Webhooks |
| -accelerate this and never replace it (§6.4). |
| +accelerate that and never replace it (§6.4). |
| --- |
| ## 6. The push path |
| -### 6.1 Why it is needed |
| +### 6.1 Why |
| Long-poll covers the app being inside a request. **Presigned direct-to-S3 |
| uploads break that**: the browser PUTs straight to the bucket and the app is |
| -never in a request at all. Common enough that designing without it is a |
| -mistake. |
| +never in a request. Common enough that designing without it is a mistake. |
| ### 6.2 Where a callback may go |
| -Draft 1 said taking a path rather than a URL meant "the parameter that creates |
| -the vulnerability does not exist". **That was overstated.** The host is still |
| -tenant-influenced: the existing outbound fetcher picks the first ready |
| -non-static route (`instances.go:86-97`), which can be a tenant's own custom |
| -domain whose DNS they control. And Go's default client follows redirects — a |
| -tenant server answering `302 → http://10.x/` converts a signed POST into a GET |
| -against an internal address. The codebase already knows this hazard |
| -(`sesevents.go:143-154` sets `CheckRedirect`). |
| +Draft 1 claimed taking a path rather than a URL meant the vulnerability "does |
| +not exist". **Overstated.** The host is still tenant-influenced: the outbound |
| +fetcher picks the first ready non-static route (`instances.go:88-97`), which can |
| +be a tenant's custom domain whose DNS they control. And Go follows redirects by |
| +default — a tenant server answering `302 → http://10.x/` turns a signed POST |
| +into a GET at an internal address. The codebase already knows this |
| +(`sesevents.go:143-158`). |
| So, all of: |
| -- POST only to the **platform-assigned host** (`<app>.<sqid>.<AppsDomain>`), |
| - never a custom domain |
| +- POST only to the **platform-assigned host**, `<app>.<sqid>.<AppsDomain>` |
| + (`apiinstances.go:566,1250`) — never a custom domain. **An app that has only |
| + its own domain gets no callbacks**; it uses long-poll. Said plainly in the |
| + docs. |
| - **https only** |
| - `CheckRedirect` **refuses every redirect** |
| -- bounded timeout, `LimitReader` on the response body |
| +- bounded timeout, `LimitReader` on the response |
| -**Path validation, stated as a rule** rather than an intention. Refuse: |
| -absolute URLs (`https://evil/x`); protocol-relative `//evil/x` and `/\evil/x`; |
| -anything not beginning with exactly one `/`; raw or percent-encoded CR/LF; |
| -fragments; `..` segments; longer than 1KB. Then: |
| +**Path validation as a rule.** Refuse: absolute URLs; protocol-relative |
| +`//evil/x` and `/\evil/x`; anything not beginning with exactly one `/`; raw or |
| +percent-encoded CR/LF; fragments; `..` segments; over 1KB. Then: |
| ```go |
| -u, err := url.Parse(p) // must yield Scheme=="" Host=="" User==nil Opaque=="" |
| +u, err := url.Parse(p) // must yield Scheme=="" Host=="" User==nil Opaque=="" |
| target := url.URL{Scheme: "https", Host: appHost, Path: u.Path, RawQuery: u.RawQuery} |
| ``` |
| -Built structurally — **never by string concatenation**. |
| +Built structurally — **never string concatenation**. |
| ### 6.3 Signing |
| @@ -318,32 +390,46 @@ An unsigned callback is a hole wearing a convenience's clothes: anyone who |
| learns the path POSTs `{"verdict":"clean"}` and the app serves a payload |
| believing the platform cleared it. |
| -Draft 1 said the key would be delivered "the same way store credentials are". |
| -**It cannot be**: a store secret exists only sealed in the app's `config.json` |
| -and never console-side (`apistore.go:21`), whereas the console must *hold* the |
| -key in order to sign. |
| - |
| -- one boot secret, `CARLOS_SCAN_WEBHOOK_KEY` |
| -- **per-app key derived at sign time**: `HMAC(master, acct+"/"+app)`. Nothing |
| - new is stored, and per-app isolation falls out — without it a single |
| - deployment-wide secret lets app A forge callbacks to app B |
| -- the derived key is delivered sealed into the app's config |
| -- `Carlos-Signature: t=<unix>,k=<keyid>,v1=<hex>` over `t + "." + body`; |
| - `keyid` allows master rotation |
| -- the body carries account, app, key and object version, so a signature cannot |
| - be replayed onto a different object |
| -- constant-time compare; documented skew window (5 minutes) |
| - |
| -Considered and rejected: Gotenberg's replay-my-headers trick. Neat, and it |
| -avoids a signing scheme, but it gives no body integrity, no replay protection, |
| -and it puts a tenant secret in request logs. |
| +**What can and cannot be reused from store credentials.** Draft 2 said the key |
| +"cannot be delivered the same way". That was wrong about delivery and right |
| +about retention. Delivery is exactly the store path — `appconfig.Seal` against |
| +`CARLOS_SECRETS_PUBKEY` then one `appconfig.Apply`, which bumps the serial and |
| +reconverges the fleet (`walk.go:602-640`). What cannot be reused is *keeping |
| +nothing*: the console must be able to sign later, and a sealed value it cannot |
| +open is no use to it. |
| + |
| +- one boot master, `CARLOS_SCAN_WEBHOOK_KEY` |
| +- **per-app key derived at sign time**: `HMAC(master, sqid + "/" + app)`. |
| + Nothing extra is stored and per-app isolation falls out — without it a single |
| + deployment secret lets app A forge callbacks to app B. The delimiter is |
| + injection-safe: a sqid is `^[a-z]{3,16}$` (`qualified.go:56`) and an app is |
| + `^[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 |
| +- **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. |
| +- 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 |
| + apps must not interpret it — the app has the ETag from its own PUT response |
| + and can match on that. |
| + |
| +Considered and rejected: Gotenberg's replay-my-headers trick. Neat, avoids a |
| +signing scheme, but gives no body integrity, no replay protection, and puts a |
| +tenant secret in request logs. |
| ### 6.4 Delivery is best effort |
| Retries with exponential backoff, a small bounded count, then stop. **No |
| dead-letter queue**: the verdict is durably on the object and the status door |
| always answers, so a dropped callback costs one poll rather than stranding a |
| -file. That is the dividend of building pull first — retries can be lazy. |
| +file. That is the dividend of building pull first. |
| Callbacks are at-least-once; apps must be idempotent. |
| @@ -351,154 +437,164 @@ Callbacks are at-least-once; apps must be idempotent. |
| ## 7. The queue |
| -### 7.1 Location, and the contradiction draft 1 shipped |
| +### 7.1 Location and index |
| -Draft 1 put items at `apps/<acct>/<app>/scan-queue/` *and* claimed the queue |
| -phase would avoid full enumeration. Those are incompatible: S3 cannot list |
| -`*/scan-queue/*`, so finding non-empty queues **is** the full |
| -`ListDirs("apps/")` walk. |
| - |
| -**Decision: keep items in the app prefix, and add an index.** |
| +Items live in the app prefix: |
| ``` |
| apps/<acct>/<app>/scan-queue/<ulid>.json |
| ``` |
| -- the enqueue door adds `acct/app` to an **in-memory set of apps with pending |
| - items**; the drain lists only those prefixes |
| -- the set is rebuilt by **one full enumeration at boot**, so the failure mode |
| - of losing it is a single startup cost, not a lost item |
| -- items carry a **TTL**; expired ones are deleted rather than scanned |
| +One object per item, not one queue object: distinct keys never collide, so an |
| +enqueue needs no coordination. ULIDs sort by time, so a listing is the queue in |
| +order. |
| -The alternative — a top-level `scan-queue/<acct>/<app>/` that one `ListDirs` |
| -finds — was rejected because `PurgeApp` lists only the app prefix |
| -(`trash.go:228-235`) and the name-claim gate (`server.go:2875+`) never sees |
| -outside it. Queue items would survive app deletion and be invisible to both. |
| +A top-level `scan-queue/<acct>/<app>/` was rejected: `PurgeApp` lists only the |
| +app prefix (`trash.go:229-238`) and the name-claim gate (`server.go:2875+`) |
| +never looks outside it, so items would survive app deletion invisibly. |
| -One object per item, not one queue object: distinct keys never collide, so an |
| -enqueue needs no coordination. ULIDs sort by time, so a prefix listing is the |
| -queue in order. |
| +But S3 cannot list `*/scan-queue/*`, so finding non-empty queues would be the |
| +full `ListDirs("apps/")` walk. Hence an index — and draft 2 left its removal |
| +rule unstated, which is a stranding race: |
| + |
| +> drain lists (empty) → enqueue writes the item → enqueue marks the app → |
| +> drain removes the mark. The item is now in S3 with nothing pointing at it. |
| + |
| +**The rule, stated:** |
| + |
| +```go |
| +type queueIndex struct { |
| + mu sync.Mutex |
| + gen map[string]uint64 // "acct/app" → generation |
| +} |
| +``` |
| + |
| +- enqueue: **write the object first**, then `gen[key]++` under the mutex |
| +- drain: read `observed := gen[key]`, list and drain, then remove the entry |
| + **only if `gen[key] == observed`** |
| +- 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 |
| ### 7.2 The drain is its own goroutine |
| -Draft 1 put a queue phase "before the sweep on each tick" inside the existing |
| -pass. **That cannot deliver seconds of latency.** `scanPass` sweeps apps |
| -sequentially, each for the full budget (`scanpass.go:86-103`), and the prior |
| -spec accepts ticks running back to back when N apps × budget exceeds the |
| -interval. A queue phase behind that is starved for up to N × 60s. |
| +Draft 1 put a queue phase inside the existing pass. That cannot deliver seconds |
| +of latency: `scanPass` sweeps apps sequentially, each for a full budget |
| +(`scanpass.go:86-103`), and the prior spec accepts ticks running back to back. |
| +A queue phase behind that starves for up to N × 60s. |
| -The drain gets its **own goroutine and its own ticker** (30s). It never touches |
| -the sidecar; the only state it shares with the sweep is object tags, which are |
| -idempotent per version. The sweep keeps its hourly cycle and its budget. |
| +Its own goroutine, its own ticker (30s). It never touches the sidecar; the only |
| +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 enqueuing ten thousand objects delay every |
| -other tenant's single urgent upload. |
| +deployment. FIFO lets one tenant's ten thousand objects delay every other |
| +tenant's single urgent upload. |
| ### 7.4 Amplification |
| -A rate limit alone does not close this. At `svcWriteBurst` (120/min, |
| -`authcli.go:139`) one app can enqueue ~172k items a day, each up to the size |
| +A rate limit alone does not close this: at `svcWriteBurst` (120/min, |
| +`authcli.go:140`) one app can enqueue ~172k items a day, each up to the size |
| ceiling. |
| -- the drain calls `Verdict(version)` **before** `Scan`, exactly as `RunSweep` |
| - does (`sweep.go:227`), so a key already judged at that version costs a tag |
| - read and nothing else |
| -- items are **deduplicated by key per tick** |
| -- per-app token bucket on the door; exceeding it is a 429, not a silent drop — |
| - an app being throttled needs to know, because its upload flow depends on the |
| - answer |
| - |
| -### 7.5 The purge race |
| - |
| -"Inherits the purge machinery" inherits the hazard as well as the sweep. |
| -`sweep.go:346-355`: any object written under a purged prefix makes the app name |
| -permanently unclaimable, and an enqueue racing `PurgeApp` leaves an item |
| -behind. |
| - |
| -The drain deletes items whose `store.json` has gone, and this spec accepts the |
| -same one-round-trip race `writeSidecar` already documents. |
| +- 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 |
| +- 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 |
| + a silent drop: an app being throttled needs to know, because its upload flow |
| + depends on the answer |
| + |
| +### 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. |
| + |
| +"Inherits the purge machinery" inherits the hazard too: `sweep.go:346-355` — |
| +any object written under a purged prefix makes the app name permanently |
| +unclaimable, and an enqueue racing `PurgeApp` leaves one behind. The orphan |
| +persists for **up to one drain tick**, during which `claimApp` refuses the |
| +name. This spec accepts that, the way `writeSidecar` accepts its |
| +one-round-trip race; §13's test asserts "not longer than one tick", not "never". |
| --- |
| ## 8. Metering |
| -`usage.go:26-38` already carries `MeterBackhaulGB` recorded and explicitly |
| -unpriced. Scanning follows that idiom — but **not into an existing object**. |
| -`EdgeDay` is per-box (`edge.go:59-69`) and `StorageDay` is written once daily |
| -by a sweep rather than incremented (`storage.go:13-18`); neither takes |
| -increments from a door. |
| +`usage.go:26-38` carries `MeterBackhaulGB` recorded and explicitly unpriced. |
| +Scanning follows that idiom — but not into an existing object. `EdgeDay` is |
| +per-box (`edge.go:41-46`) and `StorageDay` is written once daily by a sweep |
| +rather than incremented (`storage.go:13-18`); neither takes increments from a |
| +door. |
| - a new `usage/scan/<date>.json`, CAS-merged the way `UpsertEdgeDay` does |
| - (`edge.go:108-139`) |
| -- new meter names in `usage.go`, which are **frozen once merged** (`usage.go:4-5`) |
| + (`edge.go:85-116`) |
| +- **folded once per drain tick, not per object** — each fold is a GET plus a |
| + conditional PUT of a deployment-wide object |
| +- new meter names in `usage.go`, **frozen once merged** (`usage.go:4-5`) |
| - counts objects and bytes scanned **on demand**, per account per day |
| -- ships unpriced |
| - |
| -Sweep-driven scanning is **not** attributed to a tenant: the deployment |
| -operator asked for it, not them. Only work an app requested is billed to it. |
| +- unpriced |
| -Counting from day one is the point — retrofitting attribution after apps depend |
| -on the endpoint means guessing at history or starting the meter mid-flight. |
| +Sweep-driven scanning is **not** attributed to a tenant: the operator asked for |
| +it, not them. |
| --- |
| ## 9. Where clamd lives |
| -### 9.1 The argument draft 1 made was wrong |
| - |
| -Draft 1 said the console "shovels every tenant object through its own memory". |
| -It does not: `clamd.go:173-175,187` streams 64KB chunks and the prior spec |
| -mandates never buffering. The `MemoryMax` in the rollout plan is on **clamd's** |
| -cgroup, protecting the console from clamd's ~1.5GB signature RSS — it is not |
| -mitigation for a byte-shovelling smell, because there is not one. |
| +### 9.1 Draft 1's argument was wrong |
| -### 9.2 The argument that actually holds |
| +It said the console "shovels every tenant object through its own memory". It |
| +does not: `clamd.go:76-79,173-187` streams 64KB chunks and never buffers. The |
| +`MemoryMax` in the rollout plan is on **clamd's** cgroup, protecting the |
| +console from clamd's ~1.5GB signature RSS — not mitigation for a |
| +byte-shovelling smell, because there is not one. |
| -Moving clamd to a services box: |
| +### 9.2 The argument that holds |
| -- **relocates the resident cost.** ~1.5GB of signatures and the scanning CPU |
| - leave a control-plane box, permanently, for a feature that is idle by |
| - default. That is the whole of the benefit. |
| -- **costs doubled network traffic.** The console still fetches from S3 and |
| - streams onward (§9.4), so every byte now crosses the network twice. |
| -- **does not remove tenant bytes from the console.** They still transit it. |
| +Moving clamd to a services box **relocates the resident cost** — ~1.5GB of |
| +signatures and the scanning CPU leave a control-plane box, permanently, for a |
| +feature idle by default. That is the whole benefit. It **costs doubled network |
| +traffic**, since the console still fetches from S3 and streams onward (§9.4), |
| +and it **does not remove tenant bytes from the console**; they still transit |
| +it. |
| -So this is a resource-placement decision, not a data-flow one. It is worth |
| -doing, and it is worth stating at that size. |
| +A resource-placement decision, worth doing, worth stating at that size. |
| ### 9.3 Reversing the prior decision |
| Prior spec §4.1: *"No TCP option in v1: an unauthenticated scanning service |
| reachable over a network is a worse problem than the one being solved."* |
| -That objection is correct and is not answered by wanting a services box. It is |
| +That objection is right and is not answered by wanting a services box. It is |
| answered by the network: |
| -- **clamd has no authentication whatsoever.** Anything reaching the port can |
| - have it scan arbitrary bytes. |
| -- so: a private interface only (Tailscale or equivalent), **never** a public |
| - port, and the services box firewalls the port to the console's address |
| -- this is a hard constraint on the infra ask, not a recommendation |
| +- **clamd has no authentication whatsoever** |
| +- private interface only (Tailscale or equivalent), **never** a public port, |
| + and the services box firewalls the port to the console's address |
| +- a hard constraint on the infra ask, not a recommendation |
| -`internal/clamd` has a single `"unix"` literal (`clamd.go:130`); framing, |
| -deadlines and reply parsing are transport-independent. `New` gains a network |
| -parameter. |
| +`internal/clamd` has one `"unix"` literal (`clamd.go:130`); framing, deadlines |
| +and parsing are transport-independent. `New` gains a network parameter, and |
| +`CARLOS_CLAMD_ADDR` beginning with `/` means unix. |
| ### 9.4 The console keeps fetching |
| -Rather than giving the services box its own S3 read and tag permissions. It |
| -moves bytes twice, and it keeps tenant-data credentials on exactly one box. |
| -Revisit with numbers, not on principle. |
| +Rather than giving the services box S3 read and tag permissions. Bytes move |
| +twice; tenant-data credentials stay on one box. Revisit with numbers. |
| --- |
| ## 10. Surface |
| ``` |
| -carlos system antivirus status|enable|disable operator, deployment-wide |
| +carlos system antivirus status|enable|disable|rotate-webhook-key |
| carlos store scan request|status member, per app (shipped) |
| carlos store scan grant|revoke|sweep|enforce operator, per app (shipped) |
| ``` |
| @@ -510,68 +606,72 @@ POST .../store/scan/object request a scan |
| GET .../store/scan/object?key=&wait= read the verdict |
| ``` |
| -`store scan status` gains queue depth for that app and the deployment-level |
| -state (§4.5); `system antivirus status` gains it deployment-wide. "Why is my |
| -scan slow" should be answerable without a log. |
| +`store scan status` gains queue depth and the deployment state (§4.5); |
| +`system antivirus status` gains queue depth deployment-wide. |
| --- |
| ## 11. Security properties |
| -Each is a test, not a hope. |
| +Each is a test. |
| -1. No callback reaches a host the app was not assigned — for absolute URLs, |
| - `//host`, `/\host`, `@`-authority tricks, traversal, CRLF, and encoded |
| - variants of each. |
| +1. No callback reaches a host the app was not assigned — absolute URLs, |
| + `//host`, `/\host`, `@`-authority, traversal, CRLF, and encoded variants. |
| 2. No redirect is followed. |
| -3. A tampered body fails verification; a signature from app A does not verify |
| - for app B; a stale timestamp is refused. |
| +3. A tampered body fails; app A's signature does not verify for app B; a stale |
| + timestamp is refused; both masters verify during rotation. |
| 4. The app policy still has no `s3:PutObjectTagging` (`aws.go:152-165`). |
| -5. A `Service` credential cannot be minted account-scoped (§3.4), and cannot |
| - act on another app. |
| +5. A role-`Service` credential cannot be minted account-scoped (§3.4) and |
| + cannot act on another app. (Admin reaches these doors by construction, |
| + §3.2 — this property is about `Service`.) |
| 6. A `Service` credential cannot reach `system antivirus`, `scan grant`, |
| `revoke`, `sweep` or `enforce`. |
| -7. Version comparison is console-side; a stale tag is never served as fresh. |
| -8. Rate limits are enforced before enqueue, and re-requesting a judged key |
| - costs a tag read rather than a fetch. |
| +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. |
| --- |
| ## 12. Out of scope |
| -- **Synchronous scan on PUT.** CARLOS does not see the PUT; giving it that |
| - means proxying tenant data through the control plane. |
| -- **Gotenberg as a capability.** `Service` is designed so it can reuse the |
| - role; PDF rendering is its own brainstorm. |
| -- **GuardDuty.** Still gated on the tagging-cost measurement. |
| -- **Pricing.** Units recorded; a price sheet is a separate decision. |
| +Synchronous scan on PUT (CARLOS does not see the PUT); Gotenberg as a |
| +capability (the role is designed to be reused, the rendering service is its own |
| +brainstorm); GuardDuty (gated on the tagging measurement); pricing. |
| --- |
| ## 13. Testing |
| -- callback host and path validation, per §11.1, as a table of hostile inputs |
| -- redirects refused |
| -- signature: tamper, cross-app, replay, skew |
| -- `pending` returned for a stale tag, never the stale verdict |
| +- callback host and path validation as a table of hostile inputs; redirects |
| + refused |
| +- signature: tamper, cross-app, replay, skew, two-master rotation |
| +- `pending` for a stale tag, never the stale verdict; `gone` for a missing key |
| +- **the consent chain**: enqueue and drain both refuse `not_requested`, |
| + `encrypted`, `not_enabled`, `disabled`, and engine drift |
| - one app flooding the queue does not delay another's item beyond one tick |
| - a dropped callback leaves the verdict readable through the status door |
| -- `system antivirus disable` stops scanning **without a restart** and without |
| +- `system antivirus disable` stops scanning without a restart and without |
| 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) |
| - unknown `system/antivirus.json` schema refuses |
| -- metering counts on-demand scans and does **not** count sweep scans |
| -- an enqueue racing `PurgeApp` leaves no orphan that blocks the name |
| +- 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 |
| +- an enqueue racing `PurgeApp` leaves no orphan for longer than one drain tick |
| --- |
| ## 14. Risks |
| -- **Tick cost.** The drain lists only indexed prefixes, so the 120× enumeration |
| - draft 1 implied does not occur — but the index is in memory, and the boot |
| - rebuild is a full enumeration. Measure it on a deployment with many apps. |
| +- **The index is soft state, and losing an entry loses the callback.** The |
| + fallback is the ordinary sweep, which re-sweeps a completed bucket only every |
| + `ScanInterval` — **24h by default** (`scanpass.go:31,171`), not hourly. The |
| + object gets a verdict eventually and a polling app sees it; the webhook is |
| + simply not sent, and the queue item is collected at the next boot. Test by |
| + killing the console mid-queue. |
| +- **Boot rebuild cost.** One full enumeration at start on a deployment with |
| + many apps. Measure it. |
| - **Long-poll occupancy.** Bounded wait, global and per-app waiter caps. |
| -- **The index is soft state.** Losing it loses latency, never items: the sweep |
| - still finds unscanned objects on its hourly cycle. That is the designed |
| - fallback and should be tested by killing the console mid-queue. |