diff --git a/SCANNER_BUGS.md b/SCANNER_BUGS.md index deaa494..716d4d3 100644 --- a/SCANNER_BUGS.md +++ b/SCANNER_BUGS.md @@ -56,11 +56,10 @@ existing holds, plus an `origin, status` index. | Severity | What | Detail | |---|---|---| | HIGH | **No keepalive, read deadline or read limit on the scanner WebSocket.** Now *more* load-bearing than when audited: a half-open connection holds its advertised capacity out of the dispatch budget and keeps its rows marked as its own until the timeouts fire. | section 4 | -| HIGH | **Severity buckets do not sum to the reported total.** Measured: debian:11-slim reports 211 vulnerabilities beside four boxes summing to 105. User-visible and wrong. | section 6 | | MEDIUM | **Rescanning identical content produces different digests**, so every rescan uploads a fresh blob and orphans the previous one. Accumulates on the stale-scan schedule. | section 6 | | MEDIUM | **`checkPredecessor` caches a `false` from an unreachable hold forever.** `pkg/hold/gc/gc.go:2027` already carries the corrected logic for the same problem; the scan broadcaster was never brought along. | section 5 | | MEDIUM | **`handleResult` does two S3 uploads and a CAR commit inline on the reader goroutine**, on `context.Background()` with no deadline. That scanner's messages are not read while it runs. | section 5 | -| MEDIUM | **Vertical concurrency is still constrained**: `GOMEMLIMIT` is process-wide and does not scale with `scanner.workers`, a database refresh still stalls one process's workers behind the write lock, and the scanner queue has no seq or digest dedupe. Scaling out sidesteps all three; scaling up does not. | section 4 | +| MEDIUM | **Vertical concurrency is still constrained**: a database refresh stalls one process's workers behind the write lock, and the scanner queue has no seq or digest dedupe. Scaling out sidesteps both; scaling up does not. (`GOMEMLIMIT` is a separate matter, see "Accepted" below.) | section 4 | | MEDIUM | **The cgroup memory ceiling needs rechecking.** See section 3. | section 3 | | LOW | Reports are ~14.5 KB per rendered row (1.8 MB for 125 matches), parsed on every render. The report descriptor hardcodes Grype `v0.107.1` against a `v0.118.0` dependency. The report's embedded `summary` is read by nothing. | section 6 | | LOW | `pkg/config/viper.go:34` discards `ReadInConfig()`'s error, so a malformed YAML file is silently ignored whole, in scanner, hold and appview alike. | section 9 | @@ -69,6 +68,39 @@ existing holds, plus an `origin, status` index. | LOW | `assigned_at` is stored as local-offset RFC3339 while `created_at` normalises to `...Z`. Correct today, breaks across a DST transition on a non-UTC host. | section 12 | | — | Test infrastructure: `e2e.Start` writes the package-level `scan.JobCooldown`, so two harnesses alive in one test race. Bound each harness in a `t.Run` subtest, or make the cooldown per-pool. | section 6 | +### Accepted, not defects + +**Severity buckets do not sum to the reported total.** Measured: debian:11-slim +reports 211 vulnerabilities beside four boxes summing to 105, because Grype's +Negligible and Unknown severities have no bucket anywhere in the chain. Section +6 has the full data. + +**Decision: current behaviour is as intended.** Two more colours on the +vulnerability strip would clutter the UI, and Negligible and Unknown are not +worth flagging in most cases. The four boxes are a deliberate summary of what +merits attention, not an exhaustive partition of the total, and the detail table +already lists every finding including the unbucketed ones. + +Two narrow consequences remain visible and are recorded so the next reader does +not mistake them for oversights: the headline "N vulnerabilities" beside the +four boxes is a larger number than they sum to on multi-severity distros, and an +image whose findings are *entirely* Negligible or Unknown has `Total > 0`, so it +skips the "Clean" branch and renders a coloured strip reading `0 0 0 0`. If +either is ever worth addressing, the cheap route is to derive the headline from +the four counts rather than adding a fifth bucket, which needs no lexicon or +protocol change. + +**The 512 MiB `GOMEMLIMIT` does not scale with `scanner.workers`.** Process-wide +rather than per-worker, so it is divided among concurrent scans: +1.7% for a +single scan, +24% time and 5.2x GC cycles at two, +92% and 9.4x at four. + +**Decision: keep as is for now.** It exists as a safeguard for a +memory-constrained production host, not as a tuning parameter. The intended +direction is to **remove the limit** once scanners run on their own nodes, +rather than to scale it with the worker count. Until then it is doing the job it +was added for. See section 3 for the deployment context, and note the separate +open question there about the cgroup ceiling, which is a different thing. + ### Deliberately not fixed Blob 404 from a garbage-collected layer, a config blob that is not JSON, and a @@ -129,8 +161,10 @@ The most serious class in this report after the crashes, and the least fixed. Content verification landed, so the scanner no longer vouches for bytes a compliant OCI client would reject. But a zero-byte layer still scans clean, -producing an empty SBOM and a healthy-looking result, and the severity buckets -shown to users do not account for every vulnerability counted. +producing an empty SBOM and a healthy-looking result. + +The severity buckets not accounting for every counted vulnerability was +reviewed and accepted as intended; see "Accepted, not defects" in section 1. ### Workarounds