# Scanner bug report Findings from an audit of the ATCR vulnerability scanner and the hold side of the scanning system, September 2026, followed by a round of fixes. Seven agents produced the audit. Five proved behaviour by execution against a new in-process test harness (`scanner/internal/mockhold` and `scanner/internal/e2e`); two read the code without running it, one over the scanner module and one over the hold. Five more then fixed the blocker classes, test-first: each wrote tests expressing the desired behaviour, confirmed they failed for the right reason, and only then changed production code. Findings are marked **CONFIRMED** where someone reproduced them and **SUSPECTED** where they were reasoned from source. That distinction is load-bearing: treat SUSPECTED items as leads, not facts. ## How to read this **Section 1 is the current status**: what is fixed and what is still open. Read it first; it is the only part guaranteed to describe the code as it stands. Sections 2 to 6 are the consolidated analysis, updated as fixes landed. **Sections 7 onward are the original agent reports, preserved unedited as dated evidence.** They describe the code *as audited*, before any fix. Where a finding has since been fixed, the status table in section 1 says so and the analysis sections carry the detail. Do not treat an agent report as a description of current behaviour. A handful of "Reproduce:" lines in them name tests that have since been renamed; the renames are listed in section 6. Where several agents found the same defect independently, that is noted. It is worth weighting: three agents reached the dead vulnDB reload from different directions, and four reached the hold-side panic. --- ## 1. Status ### Fixed | | What | Where | |---|---|---| | **A** | A summary-less result crash-looped both the scanner and the hold. Nil `Summary` now means "not scanned for vulnerabilities", distinct from "scanned, found zero". The hold writes a record rather than orphaning the SBOM blob, and the appview renders an "SBOM only" state instead of a green **Clean** badge. | `scan/worker.go`, `scan_broadcaster.go` `handleResult`, 5 appview files | | **B** | The Grype database wedge. All three throttles were guarded by `vulnDB != nil`, so a scanner with no provider retried a full download on every scan under the exclusive lock, with no route back to health short of a restart. Replaced by one `vulnDBDecide` policy function over a state snapshot, consulted by both call sites. Reloads are load-then-swap. Also fixed: the 50-scan reload that had never executed, the missing stale-serve ceiling, corrupt-database self-healing, and context cancellation. | `scan/grype.go` | | **C** | Two independent fleet-wide halts. An unparseable frame was dropped in silence, stranding a row that held the hold's single dispatch slot forever; it is now answered `skipped` on first delivery. The 10-minute sweep leaked the in-flight digest and wrote no record, permanently retiring one image per timeout; it is now shaped like `handleError`. Plus `drainPendingJobs` sending jobs it had not claimed, and `hasActiveJobs` failing closed forever on a database error. | `client/hold.go`, `scan_broadcaster.go` | | **S** | Digest path traversal (CONFIRMED by execution: 50 bytes written outside the scan directory) and the total absence of content verification. New strict `scanner.ParseDigest`, validation at three boundaries, streaming hash-and-size verification, and `MaxImageSize` now enforced against real transferred bytes rather than the sizes a manifest claims. | `scanner/digest.go`, `scan/extractor.go`, `client/hold.go` | | **N** | Concurrency. The proactive capacity gate was depth-one hold-wide, defeating both extra workers and extra scanner processes. Dispatch depth is now the sum of worker counts scanners advertise on connect, the gate is proactive-scoped, and dispatch prefers the least-loaded scanner. Disconnects no longer hand off running work. Terminal handlers check `assigned_to`. Boot reconciliation added. | `scan_broadcaster.go`, `scanner/types.go`, `client/hold.go` | | **F4** | The hold's scanning deadline measured queueing, not scanning, because the scanner acks on receipt and `handleAck` never refreshed `assigned_at`. New `started` message sent by the worker that dequeues a job, with separate scanning (10m from `started_at`) and queueing (60m from `assigned_at`) budgets. | `scanner/types.go`, `client/hold.go`, `scan_broadcaster.go` | | **D** | Nothing bounded one scan job. New `scanner.job_timeout` (default 8m, under the hold's 10m scanning budget) starts when a worker dequeues, at the same instant the `started` message goes out. A context now reaches every blob request (`http.NewRequestWithContext`), Syft's cataloging and Grype's matcher (`FindMatchesContext`). A timeout is reported as a retryable `error`, and the scan directory is removed. Stereoscope's `img.Read()` takes no context and remains uninterruptible; `vuln.max_image_size` is its bound. | `scan/worker.go`, `scan/extractor.go`, `scan/syft.go`, `scan/grype.go`, `client/hold.go`, `internal/config/config.go` | Both modules pass `go test -race`. Three columns were added to `scan_jobs` (`origin`, `started_at`, `disconnected_at`) with per-column migration for existing holds, plus an `origin, status` index. ### Open, and worth doing next | 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 | | 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**: 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 | | LOW | Shutdown: `queue.Close()` drains rather than cancels, and `HoldClient.Close()` panics if called twice. (The blob-download half is fixed: requests now carry the job context, so `pool.Wait()` returns on cancellation.) | section 9 | | LOW | A webhook for a summary-less scan reports all-zero counts, because `db.Scan` has no status column. Only affects deployments running `vuln.enabled: false`. | section 7 | | 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 consequences follow, and both are intended. Recorded here because each looks like a rendering bug from the code alone, and the next reader should not "correct" them. The headline "N vulnerabilities" beside the four boxes is a larger number than they sum to on multi-severity distros. That is the point: the total is the finding count, the boxes are the ones worth acting on. 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`. This is correct, because **"clean" is not the same as "nothing worth reporting"**. A clean image has no findings at all; this one has findings, none of which rise to a flagged severity. Rendering it as Clean would assert something untrue, and the vulnerabilities tab lists exactly what the strip does not. **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 null config all remain retryable `error`s rather than permanent skips. They sit outside the digest and byte-verification boundary, and the 404 case needs a status-code decision that collides with other pending work. They are still infinite retry loops; see section 2. --- ## 2. Cross-cutting themes ### Retryable versus permanent is inverted The hold's stale loop retries `error` and never retries `skipped`. Any *deterministic* failure reported as an error is therefore an infinite retry loop at full scan cost. Fixed for: malformed digests of every shape, digest mismatch on downloaded bytes, short layers, images over `max_image_size` (both the claimed pre-check and the measured budget), and undecodable job frames. Still open for: blob 404 from a garbage-collected layer, a config blob that is not JSON, an index manifest with an empty config digest, and a null config. The inverse also held and is partly improved: `"scanner queue full"` was the *hold's own overload* recorded as a week-long failure against the user's image. It is now much harder to provoke, since the hold stops dispatching before a scanner's queue fills, but the classification itself is unchanged. ### Deadlines Fixed at the protocol level: the hold now measures scanning from when a worker reports starting, not from dispatch. Fixed inside the scanner too. `scanner.job_timeout` (default 8 minutes) is started by the worker that dequeues the job, at the same instant it sends `started`, so the scanner's budget and the hold's 10-minute one measure the same interval and the scanner's is the shorter. It reaches the blob requests (`http.NewRequestWithContext` on both the presign call and the body), Syft's cataloging, and Grype's matcher, which turned out to have a `FindMatchesContext` alongside the `FindMatches` that has no cancellation. A timeout is reported as an `error`, not a `skipped`: the cause is the host, not the image, and `max_image_size` already refuses the pathological cases before a byte moves. Threading the context also fixed shutdown, which used to wait out the HTTP client's five-minute per-request timeout. **One stage remains uninterruptible, and the bound is not honoured across it.** `img.Read()` is stereoscope's layer extraction, it takes no context, and it is 81% of a scan. The deadline is checked immediately before and after it, so a job that has already spent its budget does not go on to extract, and an overrun is noticed as soon as extraction returns — but in between, a pathologically slow decompression runs to completion and can outlast both the scanner's deadline and the hold's. Running it on an abandoned goroutine was rejected: it trades a bounded overrun for a leaked goroutine writing gigabytes into a directory the caller has already cleaned up. `vuln.max_image_size` is the real bound on that stage, which is the argument for keeping it tight on a small host. Misconfiguration is one-sided and warned about at boot: if `job_timeout` is set at or above the hold's scanning timeout, the hold reclaims and re-dispatches a row this scanner is still working on, which is the duplicate-scan behaviour the deadline exists to prevent. The scanner cannot read the hold's configuration, so the check is against the value the hold shipped with and is advisory. There is still no ping/pong, read deadline or read limit on either side of the scanner WebSocket. ### Silence Mostly closed. Undecodable frames are answered, the timeout sweep writes a record, and disconnects no longer strand work silently. Still open: results computed while the socket is down are dropped, because `sendJSON` guards only `conn == nil` and `conn` is never nil'd on disconnect. `SendResult` returns no error, so the worker cannot tell. ### Correctness of the scan verdict itself 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. The severity buckets not accounting for every counted vulnerability was reviewed and accepted as intended; see "Accepted, not defects" in section 1. ### Workarounds Three memory workarounds were measured. The 50-scan vulnDB reload had never executed and now does. The `runtime.GC()` before the cooldown was not separately measured. The 10 second cooldown does not reduce peak memory (800 MiB with, 837 MiB without, across six sustained scans, no ratchet either way) at a cost of 39% of wall time, but see below: it was never a memory workaround. ### The inter-job cooldown is not a memory workaround The comment reads "Cooldown between scans to reduce sustained memory pressure", which measurement disproves. The author's actual intent was to yield CPU and IO to co-tenant processes on a shared host, and it also returns idle RSS between jobs (210 versus 596 MiB). No systemd unit in `deploy/upcloud/systemd/` set `CPUWeight`, `IOWeight` or `Nice`, so before the deployment change in section 3 this sleep was the *only* mechanism giving the hold breathing room. Recorded because the comment is actively dangerous: it invites exactly the memory benchmark that was run here, and a reader trusting the result would delete something load-bearing for a reason the comment does not state. The comment should be corrected and the duration made configurable. --- ## 3. Deployment and resources ### Applied Unlike the code findings, these were applied during the audit, because they are configuration and the target host is 1 GiB shared with the hold. `deploy/upcloud/configs/scanner.yaml.tmpl`: `scanner.workers` 2 to 1; `vuln.max_image_size` added at 512 MiB (it was **absent**, so production ran the 2 GiB default); a comment recording that `tmp_dir` must stay off tmpfs given 3.8x extraction amplification. `deploy/upcloud/systemd/scanner.service.tmpl`: `MemoryHigh=640M`, `MemoryMax=768M`, `CPUWeight=20`, `IOWeight=20`. The ordering principle behind the numbers: **the cheap guard must fire before the violent one.** `max_image_size` rejects an image before a byte is downloaded; `MemoryMax` kills mid-scan, and the hold then returns the row to `pending`, the scanner restarts and is re-sent the same job, which is a permanent crash loop on one image. If the cgroup cap is ever observed firing, lower `max_image_size` rather than raising `MemoryMax`. ### Those numbers need rechecking **OPEN.** `MemoryHigh` and `MemoryMax` were sized from measurements that excluded Grype entirely. Later measurement with the real database found that although the database is 2.0 GB on disk it is mmap-backed, and loading it plus one match takes a process from 59 MiB to roughly 200 MiB resident, with per-scan peak at 233 to 263 MiB. Crucially, **`GOMEMLIMIT` cannot see any of this** — it bounds the heap, and mapped file pages are not heap (heap after a scan was 16 to 37 MiB) — but **the cgroup `MemoryMax` does count resident file pages.** So a 561 MiB `node:22`-class scan plus 140 to 200 MiB of resident database lands near 700 to 760 MiB against a 768 MiB cap. That is too tight, and it fails as an OOM kill mid-scan, which is the crash-loop shape above. Lower `max_image_size` rather than raising the cap. ### When scanners move to their own nodes The scanner is a pull-based worker: it dials the hold's WebSocket and downloads blobs via presigned URLs that point straight at S3, so layer bandwidth never crosses the hold. Moving it to another node is largely a matter of pointing `hold.url` at a public `wss://` endpoint. Because jobs persist in `scan_jobs` and `drainPendingJobs` hands over everything pending on connect, an intermittent scanner (a spot instance that drains a backlog and goes away) is a topology the design already supports. On that move: remove `CPUWeight` and `IOWeight` entirely, since they exist only to protect a co-tenant; raise `MemoryHigh`/`MemoryMax` and `max_image_size` to match the new host; and raise `scanner.workers`, which now genuinely helps. --- ## 4. Performance Measured on AMD 7900X3D, 24 threads, 61 GiB, NVMe. **This is a far more capable machine than the production host**, so absolute times do not transfer; peak memory, disk amplification and scaling shapes do. - **Stereoscope load and extract dominates**: 81% of a `node:22` scan, 12.8 to 13.3s of 15.9 to 16.3s. Cataloging 16%, encoding 1.5%, download 1.2% (over loopback, a floor rather than a forecast). Nobody has attacked this. - **Grype matching adds 0.35 to 0.5s**, roughly 15 to 37% of wall time on small and medium images. Every other number in this report excludes it. - **Opening an already-activated database is sub-second**, so a scanner with a good database on disk recovers in under a second. This validates the cold-start backoff policy. - **Concurrency scales near-linearly** in throughput (3.77, 7.34, 13.71 scans/min at 1, 2, 4 workers) and linearly in RSS (836, 1414, 3040 MiB). Note these were measured separately from the memory-limit scenario below and the two have not been reconciled. - **The 512 MiB `GOMEMLIMIT` is free at one worker and expensive beyond**: +1.7% for a single scan, +24% time and 5.2x GC cycles at two concurrent, +92% and 9.4x at four. It is process-wide, so it is divided among workers rather than multiplied by them, and it does not scale with `scanner.workers`. - **Disk amplification**: 3.8x for `node:22` (389 MiB to 1493 MiB), 823x for compressible content. - **No goroutine or file descriptor leaks** across 200 jobs and 4 reconnects, and temp directory cleanup is reliable on every error path reachable. Run the perf suite with `ATCR_SCANNER_PERF=1`; exact commands are in section 10. --- ## 5. What this audit did not cover - **Whether the scanner finds the *right* vulnerabilities.** Report verification now exists (section 6) and checks internal consistency, but no test asserts that a given image's findings are complete or correct against ground truth. - **The appview's rendering of scan results**, beyond the "SBOM only" state added by fix A and the severity-bucket defect in section 6. - **`scanner/internal/e2e` is not run by CI.** `make test` is `go test ./...` from the root module, which excludes the `atcr.io/scanner` module entirely. None of this coverage runs until that changes. - **Production-class hardware.** See section 4. --- ## 6. Report verification, and the test suites ### Test coverage added All green, clean under `-race`. The default suite is offline and fast (~27s). | Path | Covers | |---|---| | `scanner/internal/mockhold/` | In-process mock hold: the three endpoints the scanner touches, a `BlobSource` interface (OCI layout / synthetic / overlay), message transcript, blob request log, connection drop modes, blob fault hooks | | `scanner/internal/mockhold/testdata/corpus.json` | 84 real `io.atcr.manifest` records (57 image, 16 attestation, 7 index, 4 helm), fetched anonymously from a live PDS | | `scanner/internal/e2e/` | Pipeline, stuck scans, blob faults, blob integrity, protocol and lifecycle, concurrency, report verification, benchmarks | | `scanner/internal/scan/`, `internal/config/` | vulnDB lifecycle, skip classification, digest parsing, config validation | | `pkg/hold/pds/scan_broadcaster_*_test.go` | Hold-side state machine, stalls, disconnects, concurrency | Gated suites, both skipping cleanly when their fixtures are absent: ``` scanner/internal/mockhold/testdata/fetch-blobs.sh # real image layouts (needs buoy.cr creds) scanner/internal/mockhold/testdata/fetch-vulndb.sh # Grype DB + pinned public images, ~2.0 GB ATCR_SCANNER_VULNDB=1 go test ./internal/e2e -run TestVulnDB -v ATCR_SCANNER_PERF=1 go test ./internal/e2e -run TestPerf -v -timeout 40m ``` `fetch-vulndb.sh` loads the database through Grype's own curator rather than curling the archive, because the curator writes the `import.json` that `ValidateChecksum: true` requires; a hand-extracted database reproduces the corrupt-database finding by accident. ### Severity buckets do not account for every vulnerability **MEDIUM / CONFIRMED by execution.** `scan/grype.go` `countVulnerabilitiesBySeverity`; `scan_broadcaster.go:113-119`; `pkg/atproto/lexicon.go:749-753`; `pkg/appview/src/css/main.css:843-846`. Every match increments `Total`; only Critical/High/Medium/Low reach a bucket. Grype also emits Negligible and Unknown, and there is no fifth bucket anywhere in the chain: not the wire summary, not the `io.atcr.hold.scan` record, not the four CSS classes. | fixture | buckets | total | unaccounted | |---|---|---|---| | alpine:3.10 | 125 | 125 | 0 | | debian:11-slim | 105 | 211 | **106** (Negligible 84, Unknown 22) | | python (trixie) | 117 | 169 | **52** (Negligible 44, Unknown 8) | Alpine's secdb resolves every finding to an NVD severity, so a check against Alpine alone reports perfectly consistent. It only appears across ecosystems. Visible consequence: `partials/vuln-details.html:36` prints `{{ .Summary.Total }} vulnerabilities` beside four boxes summing to 105 on a Debian image. Edge case: an image whose findings are *entirely* Negligible or Unknown has `Total > 0`, skips the green "Clean" branch, and renders a coloured strip reading `0 0 0 0`. `pkg/appview/handlers/diff.go:296-306` reproduces the same defect independently, with a comment acknowledging it. Suggested fix: either carry a fifth count end to end, or keep four buckets and stop printing a total they do not sum to, deriving the headline from the four and showing the remainder as an explicit "N other". Whichever is chosen, `addToSevCount` in `diff.go` must move the same way. Repro: `ATCR_SCANNER_VULNDB=1 go test ./internal/e2e -run TestVulnDBSeverityBucketsAccountForEveryMatch -v` ### Rescanning identical content produces different digests **MEDIUM / CONFIRMED by execution** (storage consequence SUSPECTED). `scan/grype.go` (`"source": s.Source`), `scan/syft.go` (`stereoscopesource.ImageConfig{Reference: ociLayoutDir}`). Both the SBOM and the report embed the source's `Name`, which is the per-job temp directory. Rescanning unchanged content therefore yields byte-different blobs, so `sbomDigest` and `vulnDigest` change on every rescan, the hold uploads a fresh blob, and the previous one is orphaned. The stale loop rescans on a schedule, so this accumulates. Suggested fix: set the source reference to the manifest digest rather than the scratch path, which also makes stored reports self-describing. ### Reports are large, and the version is wrong **LOW / CONFIRMED.** alpine:3.10, 15 packages and 125 matches, produces a **1,812,438-byte** report: `match.Match` is marshalled whole, so each row ships the full CVE description, every CVSS vector, every CPE and the full `pkg.Package`. The appview reads six fields per match and parses this on every render. Changing the shape breaks every stored report, so it needs a version marker. The report descriptor hardcodes `"version": "v0.107.1"` against a `v0.118.0` dependency, now observed in real stored reports. Read it from `debug.ReadBuildInfo()`. The report's embedded `"summary"` is read by nothing: the hold treats the report as an opaque string and takes counts from the WebSocket `summary` field. ### The report's JSON shape is odd and worth pinning Top-level `matches` is lowercase (a map literal in `grype.go`) while `Vulnerability`, `Package`, `ID`, `Metadata` and `Fix` are PascalCase, because `match.Match` has no JSON tags. Adding tags upstream, or switching to Grype's presenter model, would silently blank every stored report. A test now pins this. ### Renamed tests Agent reports in sections 7 onward cite some test names that no longer exist: | Cited | Now | |---|---| | `TestUnparseableFramesAreDroppedInSilence` | `TestUnparseableFramesAreAnsweredWithSkipped`, plus `TestFramesWithNoUsableSeqAreDroppedSilently` | | `TestScanProcessingTimeout_LeavesTheDigestInFlightForever` | `TestScanProcessingTimeout_RetiresTheJobProperly` | | `TestScanDispatchQueue_BuriesJobsWhenTheScannerBufferFills` | `TestScanDispatchQueue_ReturnsUndeliverableJobsToPending` | | `TestScanAck_DoesNotRestartTheProcessingClock` | `TestScanAck_DoesNotStartTheScanningClock` | | `TestScanUnsubscribe_UnassignsJobsOnce` | `TestScanUnsubscribe_MarksItsOwnJobsOnce` | | `TestScanUnsubscribe_ReoffersAJobTheScannerIsStillRunning` | `TestScanUnsubscribe_HoldsAJobForAReconnectingScanner` | | `TestScanHasActiveJobs_*` | `TestScanActiveProactiveJobs_*` | | `TestStalledDownloadHasNoShortDeadline` | `TestStalledDownloadIsWaitedOutWithinTheJobBudget` | | `TestShutdownDoesNotInterruptInFlightDownload` | `TestShutdownAbortsAnInFlightDownload` (inverted; now in `deadline_test.go`) | The blob edge cases that pinned the absence of digest validation moved from `blob_edge_test.go` to `blob_integrity_test.go`, where they assert the fix. --- > **The sections below are the original agent reports, preserved unedited as > dated evidence.** They describe the code *as audited*, before any fix was > applied. Section 1 is the authority on current status. ## 7. Grype database refresh (execution) ##### Scanner vulnerability-database refresh: findings Scope: `scanner/internal/scan/grype.go`, `scanner/internal/scan/worker.go`, and the vendored `github.com/anchore/grype@v0.118.0` code they call. Tests added (all green, `go test -race ./internal/scan/` passes): `scanner/internal/scan/vulndb_refresh_test.go`. No e2e file was added; see "Seam needed" at the end for why. Every claim below was either executed or traced to an exact line in the module cache. No test performs a network fetch: the two tests that call `grype.LoadVulnerabilityDB` pass `update=false` and read a synthetic on-disk database built with the exported `v6.NewLowLevelDB` writer. --- #### 0. Ground truth: what the user's error actually is Not a finding, but it anchors the rest. The message users report as "grype is out of date, please update the database" is Grype's own text from `grype/db/v6/installation/curator.go:613`: ``` the vulnerability database was built %s ago (max allowed age is %s) ``` Reproduced with no network in `TestGrype_ExpiredOnDiskDBIsAHardError`: ``` grype: the vulnerability database was built 2 weeks ago (max allowed age is 2 weeks) no import metadata file at: .../6/import.json ``` Path into it, with `grypeDBConfig`'s `ValidateAge:true, MaxAllowedBuiltAge:14d` (`grype.go:153-160`) and `update=true`: 1. `curator.Update()` reads the on-disk description, `validateAge` fails, so it sets `current = nil` (`curator.go:219-224`) and always attempts a download — an expired DB is self-healing *if the upstream is reachable*. 2. If the upstream check or download fails, `Update` returns an error. `distribution.DefaultConfig()` has `RequireUpdateCheck:false`, so `LoadVulnerabilityDB` only logs it (`load_vulnerability_db.go:24-30`) and continues. 3. `c.Status()` then runs `validateAge` again on the unchanged on-disk DB and `LoadVulnerabilityDB` returns that error with **no provider** (`load_vulnerability_db.go:36-40`). So an expired DB plus a failing upstream is a hard load failure, and everything below is about what the scanner does with that. The scanner's own wrapping doubles the phrase, which is worth cleaning up while someone is in here. Observed in `TestWorkerPool_EveryScanFailsWhileTheDBIsUnloadable`: ``` failed to scan vulnerabilities: failed to load vulnerability database: failed to load vulnerability database: upstream unreachable ``` (`grype.go:59` wraps `grype.go:241`, which already says the same words.) --- #### 1. The 50-scan periodic reload is unreachable and has never run **Severity: HIGH — CONFIRMED** `scanner/internal/scan/grype.go:193-205` (branch), `grype.go:181` (the check that makes it unreachable) **What happens.** The periodic reload exists to "close and reopen DB every 50 scans to flush SQLite's page cache and mmap region". It sits under the write lock guarded by `vulnDB != nil && time.Since(vulnDBBuilt) < vulnDBRefreshAge` (line 193). The read-lock fast path at line 181 returns on *exactly that same condition*: ```go if vulnDB != nil && (time.Since(vulnDBBuilt) < vulnDBRefreshAge || ...) { vulnDBLock.RUnlock() return vulnDB, nil // fresh DB always exits here } ``` A fresh database therefore never reaches the write lock, `vulnDBScans` is never incremented, and the reload never fires. The only way into the branch is a race: a caller must pass the fast path while the DB is stale and then find it fresh a few instructions later because another goroutine completed a reload in the gap between `RUnlock()` and `Lock()`. This was true from the moment the branch was introduced (`136c0a0`) — the fast path had the same freshness test then — so the memory-pressure mitigation the scanner is documented to have has never operated in production. If the scanner is being restarted for memory growth, this is why. **How to reproduce.** `TestPeriodicReload_NeverRuns`: 200 `loadVulnDatabase` calls against a fresh provider leave `vulnDBScans == 0` and the loader uncalled, where the code's comment implies four reloads. **Suggested fix.** Move the scan counter to where scans actually pass — increment it on the read-lock fast path (it is already `atomic.Int64`, so no lock upgrade is needed), and let the fast path fall through to the write lock when `n%50 == 0`. Do not do this without fix 2 below: making the branch reachable as written would activate a worse bug. --- #### 2. A failed reload after `vulnDB = nil` strands the scanner, and there is no cold-start backoff **Severity: BLOCKER — CONFIRMED (the stranded state; the periodic-reload route into it is currently blocked by finding 1)** `scanner/internal/scan/grype.go:199-200`, `grype.go:212-214`, `grype.go:234-241` **What happens.** Every throttle in `loadVulnDatabase` is guarded by `vulnDB != nil`: - line 181 (fast-path backoff) — `vulnDB != nil && ...` - line 212 (write-lock backoff) — `vulnDB != nil && ...` - line 234 (serve-the-old-DB fallback) — `if vulnDB != nil` So with no provider in hand there is no throttle and no fallback: **every single scan runs a complete load attempt**, which in production is a 30s listing check (`distribution.DefaultConfig().CheckTimeout`) plus up to a 300s archive download (`UpdateTimeout`) plus hydration of a multi-hundred-MB SQLite file — all of it holding the exclusive lock. `vulnDBAttempt` is dutifully recorded at line 228 and then never consulted, because reading it requires a provider. Two ways to get there: - **Cold start** with an expired/absent on-disk DB and a failing upstream. The startup goroutine at `worker.go:66-73` logs "Vulnerability scanning will be disabled until database is available" — nothing is disabled; see finding 6 — and every job then fails and re-attempts. - **The periodic reload**, once finding 1 is fixed. Line 199-200 does `vulnDB.Close(); vulnDB = nil` *before* the load, so a reload that fails for any reason (network blip, 5xx from the CDN, disk full during decompression) destroys a perfectly working in-memory provider and drops the scanner into the no-provider, no-backoff state — from a state where it was scanning fine. The hold retries failed scans on its stale-scan loop, so failures feed the retry storm rather than draining it. **How to reproduce.** - `TestColdStartFailure_RetriesEveryCallWithNoBackoff` — 5 sequential calls, 5 loader invocations, no throttle. - `TestColdStartFailure_SerializesEveryWorkerThroughTheDownload` — 4 concurrent callers produce 4 sequential attempts; elapsed time is their sum. - `TestWorkerPool_EveryScanFailsWhileTheDBIsUnloadable` — through the real worker pool and a mock hold: every job comes back as an `error` message (which the hold retries) and the loader runs once per job. - The periodic-reload route is not directly reachable today (finding 1), so it is asserted by composition rather than end to end. **Suggested fix.** Three parts: 1. Drop the `vulnDB != nil` guard from the backoff checks so a cold start throttles too — with a shorter backoff (say 1-5 min) while there is no provider at all, since that state is worth escaping quickly. 2. Single-flight the load so N workers share one attempt instead of queueing N. 3. In the periodic reload, load the replacement *first* and only close the old provider once the new one is in hand — the same `store, err := load(); if err == nil { old.Close(); vulnDB = store }` shape already used at lines 254-258 on the normal path. Never null the working provider before you have its successor. --- #### 3. A refresh blocks every scan in the pool for the whole download **Severity: HIGH — CONFIRMED** `scanner/internal/scan/grype.go:187-188` (write lock spans the load), `grype.go:105-106` (read lock spans `FindMatches`) **What happens.** `loadVulnDatabase` takes `vulnDBLock.Lock()` at line 187 with `defer Unlock()`, and the `loadVulnDB` call at line 229 sits inside it. That call is the *entire* refresh: listing fetch, archive download, zstd decompression, hydration (index build over the whole DB), and the `replaceDB` directory swap. Grype bounds only the HTTP portion, at 300s. Meanwhile `scanVulnerabilities` holds `RLock` across `FindMatches` (`grype.go:105-114`). Go's `RWMutex` blocks new readers once a writer is queued, so during a refresh: - no worker can start matching, - no worker can even take the read-lock fast path to discover the DB is cached, - the whole pool is stalled for the refresh duration, once per refresh window. With `vulnDBRefreshAge` at 7 days this is rare on the happy path. It stops being rare in the finding-2 state, where the stall happens on *every scan*. **How to reproduce.** `TestRefresh_BlocksAllInFlightScans` measures a reader's wait on `vulnDBLock` while a refresh is in flight and reports it (~130ms for a 150ms stubbed load). Same lock, same acquisition `scanVulnerabilities` performs. **Suggested fix.** Do the download outside the lock. The lock only needs to cover the pointer swap: read `vulnDB` under `RLock`, run `loadVulnDB` with no lock held, then take `Lock()` just to compare-and-swap the provider and close the predecessor. Combine with the single-flight from finding 2 so only one goroutine downloads. Also pass the context through — see finding 7. --- #### 4. The stale-DB fallback has no upper bound, and success is reported either way **Severity: HIGH — CONFIRMED** `scanner/internal/scan/grype.go:212-214`, `grype.go:234-240` **What happens.** Once the upstream stops working, `loadVulnDatabase` serves the last-loaded provider forever. Nothing caps how old it may get: - Grype's 14-day `MaxAllowedBuiltAge` is only consulted by the curator when *opening* the file on disk. The already-open provider never re-checks — see finding 5. - `vulnDBRefreshAge` (7 days) only decides whether to *attempt* a reload. - `vulnDBRetryBackoff` (30 min) only throttles the attempts. The provider comes back with a `nil` error, so the scan succeeds, results are written to the hold, and nothing in the result marks them as coming from a stale database. Only a `slog.Warn` at line 235 records it. A scanner whose egress is blocked will keep publishing confident, wrong "0 critical" verdicts for months. Note this is the intended trade (scanning against a slightly old DB beats refusing to scan), taken past the point where it is a trade. **How to reproduce.** `TestStaleDBIsServedForever_WithNoCeiling` — a provider built 120 days ago is served across ten backoff windows with a nil error every time. **Suggested fix.** Give the fallback a ceiling and make it visible: - Refuse to serve past some hard age (Grype's 14 days is the natural line, since that is the guarantee `grypeDBConfig` already claims to want), returning the error rather than a silent success. - Below that ceiling, propagate the DB build time into `scanner.ScanResult` so the hold and appview can label a scan as run against an N-day-old database. The build time is already in hand as `vulnDBBuilt`. --- #### 5. The in-memory provider genuinely does not revalidate age — the comment is correct **Severity: LOW (informational) — CONFIRMED** `grype.go:209-211` (the comment), `grype/db/v6/vulnerability_provider.go:31-48` The comment claims "the in-memory provider doesn't re-validate build age on queries, so a stale-but-loaded DB still scans fine". It is accurate. `v6.NewVulnerabilityProvider` closes over a `Reader` and an architecture-alias map and nothing else — no `Config`, no `MaxAllowedBuiltAge`, no build timestamp. There is no field a query path could consult. Worth recording because it cuts both ways: it is exactly why the fallback in finding 4 works, and exactly why that fallback has no natural ceiling. Any fix for finding 4 must impose the ceiling in `loadVulnDatabase`; Grype will not do it. **Verified by** `TestGrype_InMemoryProviderDoesNotRevalidateAge`: a provider opened over a database stamped one year old answers queries normally. --- #### 6. A checksum / import-metadata failure is not self-healing **Severity: MEDIUM — CONFIRMED (mechanism); SUSPECTED as a production trigger** `grype.go:157` (`ValidateChecksum: true`), `grype/db/v6/installation/curator.go:174-183` and `:208-232` **What happens.** `grypeDBConfig` sets `ValidateChecksum: true`, and `curator.Status()` joins the checksum failure into the same error the age check produces. A database whose `import.json` is missing or corrupt fails to load even when its build timestamp is minutes old. `curator.Reader()` needs the file too, via `isRehydrationNeeded` — that path ignores `ValidateChecksum` entirely, so the file is mandatory regardless of config. The dangerous part is the asymmetry with finding 0: | on-disk state | `Update()` behaviour | self-heals? | |---|---|---| | expired (age) | `current = nil`, `isSupersededBy(nil, …)` is unconditionally true → always downloads | yes, if upstream reachable | | bad/missing import.json | `current` stays valid → downloads only if upstream is strictly newer | no | So a database that is current but whose import metadata was lost — interrupted `activate`, truncated write, container killed mid-hydrate, volume snapshot restore — fails every scan and no update fixes it until the upstream publishes something newer. Nothing in the scanner ever deletes the DB directory. Combined with finding 2 this becomes a permanent, full-speed retry storm. Note also `curator.replaceDB` (`curator.go:533-541`) does `Delete()` (RemoveAll) of the live DB directory *before* renaming the new one into place, so an interruption in that window leaves no database at all. **How to reproduce.** `TestGrype_FreshDBWithoutImportMetadataIsAlsoAHardError` — a database built an hour ago fails to load with `no import metadata file at: …/6/import.json`. **Suggested fix.** On a load failure whose cause is checksum/import-metadata (as opposed to age or network), delete the DB directory and retry once, which is what `grype db delete && grype db update` does for CLI users. Keep it to one retry per attempt window so it cannot become its own storm. --- #### 7. `loadVulnDatabase` ignores its context **Severity: MEDIUM — CONFIRMED** `scanner/internal/scan/grype.go:174` (parameter), never used in the body `ctx` is accepted and never consulted. A shutdown, or a hold that has abandoned the job, cannot abort an in-flight database refresh; the exclusive lock is held until Grype's own timeouts expire (up to 300s for the download alone, unbounded for hydration). Combined with finding 3 that is the whole pool. **How to reproduce.** `TestLoadVulnDatabase_IgnoresContextCancellation` — an already-cancelled context still runs a full load. **Suggested fix.** Grype's `LoadVulnerabilityDB` takes no context, so the honest options are (a) run the load in a goroutine and have `loadVulnDatabase` select on `ctx.Done()`, abandoning the result, or (b) at minimum stop accepting a parameter the function does not honour. (a) only becomes worthwhile once the download is out of the lock (finding 3). --- #### 8. "Vulnerability scanning will be disabled until database is available" is not true **Severity: MEDIUM — CONFIRMED** `scanner/internal/scan/worker.go:68-71` When the startup load fails, the pool logs that scanning "will be disabled until database is available". Nothing is disabled. `processJob` still checks only `wp.cfg.Vuln.Enabled` (`worker.go:245`), so every job runs the full pipeline — blob download, OCI layout rebuild, a complete Syft catalog — and then fails at the Grype step, discarding the SBOM it just spent minutes producing. Directly observable in `TestWorkerPool_EveryScanFailsWhileTheDBIsUnloadable`'s log output: the warning is emitted at startup and both subsequent jobs still download blobs and generate SBOMs before failing. Two costs: wasted work per job, and a lost SBOM. The SBOM does not depend on the vulnerability database at all and is already computed by the time the failure happens. **Suggested fix.** Either make the claim true (a flag that suppresses the Grype step while the DB is unavailable), or — better — send the SBOM-only result rather than an error, so the scan record carries the SBOM and can be re-scanned for vulnerabilities later. That change interacts with `result.Summary` being nil, so it must land together with the known `worker.go:129` blocker. --- #### 9. The vulnerability report claims a Grype version the scanner does not run **Severity: LOW — CONFIRMED** `scanner/internal/scan/grype.go:134` vs `scanner/go.mod:6` The report descriptor is hardcoded `"version": "v0.107.1"`; the module pins `github.com/anchore/grype v0.118.0`. Eleven minor versions of matcher and schema changes are misattributed in every stored scan record. **Suggested fix.** Read it from build info (`debug.ReadBuildInfo()` → the `github.com/anchore/grype` dep version) so it cannot drift again, or at minimum correct the constant and note next to it that it must be bumped with the dependency. --- #### Interaction summary: how the scanner gets wedged Two absorbing states, both reached without a restart and neither escapable without one under the right conditions: **Wedged refusing.** No provider (cold start, or post-finding-1 periodic reload) + upstream failing or on-disk DB unrecoverable (finding 6). Every scan: full load attempt, exclusive lock, no backoff, error to the hold, hold re-queues. Escapes only when a load finally succeeds. Findings 2, 3, 6. **Wedged serving.** Provider in hand + upstream failing. Every 30 minutes one probe fails; in between, an arbitrarily old database is served as success with no marker. Never escapes on its own and never complains. Findings 4, 5. The failure the user reports ("out of date, please update") is the *first* state surfacing. The second state is the more alarming one, because it does not surface at all. --- #### Seam needed (not added — production code untouched) `scanner/internal/e2e/vulndb_test.go` was **not** written. `loadVulnDB` (`grype.go:167`) is unexported, so the e2e harness cannot stub it, and the only alternative is a real multi-hundred-MB download from `grype.anchore.io`. The worker-pool-level scenario therefore lives in `scanner/internal/scan/vulndb_refresh_test.go` instead, where it can reach the package var. It wires the real `WorkerPool`, `HoldClient`, and `JobQueue` to `mockhold` exactly as the harness does. If e2e coverage is wanted, the minimal seam is to export the loader the way `JobCooldown` is exported (`worker.go:154`) — e.g. `var LoadVulnDB = grype.LoadVulnerabilityDB` with the same "tests only, production must not change it" comment — or to hang it off `WorkerPool` so it can be injected at construction. --- #### Note for the fix agent The known `worker.go:129` blocker (`result.Summary.Total` dereferenced when `Summary` is nil under `vuln.enabled=false`) was **worked around, not fixed**: `TestWorkerPool_EveryScanFailsWhileTheDBIsUnloadable` runs with `Vuln.Enabled = true` and every job terminates as an error, so no successful result is ever produced and the nil deref is not reached. That test will start exercising the success path if finding 8's suggested fix (send SBOM-only results) is applied, which is why the two must land together. One test-only issue found while writing these: `scan.JobCooldown` is read by the worker loop on every iteration (`worker.go:142`), so restoring it in a `t.Cleanup` without first waiting for the pool is a real data race that `-race` reports. The new test waits via `pool.Wait()` before restoring. `scanner/internal/e2e/harness.go:98-99` restores it without waiting and has the same latent race; it has not been hit yet only because those scenarios cancel and close in a different cleanup ordering. Worth fixing when someone is in that file — it is not mine to touch. ## 8. Stuck scans (execution) ##### Scanner "stuck" investigation — findings Scope: why a scan job stops making progress and never reaches a terminal state. Five candidate mechanisms were separated and tested. Four are real and demonstrated; one (the Grype lock) is a code reading only and is marked SUSPECTED. Tests added (nothing else was touched, no production code was modified): - `scanner/internal/e2e/stuck_test.go` — 5 scenarios, all passing. - `pkg/hold/pds/scan_broadcaster_stuck_test.go` — 8 scenarios, all passing. How to run them: ```bash cd /home/data/atcr.io && go test ./pkg/hold/pds/ -run TestScan -count=1 cd /home/data/atcr.io/scanner && go test ./internal/e2e -count=1 -race ##### Or, isolated from whatever else is landing in that package right now: cd /home/data/atcr.io/scanner/internal/e2e && go test -count=1 -race harness.go stuck_test.go ``` Every scanner-side scenario is built so no job ever reaches a *successful* scan, because a successful scan currently panics the test binary (finding 1) — `TestScanRealImage` has since been `t.Skip`ped for that reason by whoever owns `pipeline_test.go`, but the constraint still applies to anything new. --- #### 1. A scanner with `vuln.enabled=false` panics itself *and* the hold on every successful scan **Severity: BLOCKER — CONFIRMED** `scanner/internal/scan/worker.go:129` and `pkg/hold/pds/scan_broadcaster.go:642` `processJob` only sets `result.Summary` when `cfg.Vuln.Enabled` (worker.go:243). With vulnerability scanning off, every successful scan reaches `result.Summary.Total` on line 129 and takes the scanner process down. The scanner-side half was already known. The hold-side half is new and worse: `SendResult` (client/hold.go:169) runs *before* the panicking log line, so the hold does receive the summary-less result. `handleResult` guards its record branch with `if msg.Summary != nil` (scan_broadcaster.go:597) and then dereferences `msg.Summary.Critical` unguarded in its closing log line at scan_broadcaster.go:642. That runs on the subscriber's reader goroutine, not in an HTTP handler, so **one misconfigured scanner crashes the whole hold process** — every connected scanner is dropped, and every assigned/processing row is left to the reclaim loops. Reproduce: `TestScanHandleResult_PanicsOnResultWithoutSummary` (provokes the panic under `recover`, so the suite stays green). Scanner side: `cd scanner && go test ./internal/e2e -run TestScanRealImage` panics at worker.go:129 today. Fix (described): give `handleResult` a local `summary` with a zero value when `msg.Summary == nil`, and do the same in the scanner's log line. Grep for other unguarded `msg.Summary.` / `result.Summary.` derefs — scan_broadcaster.go:603 and :616 are inside the nil check and are fine. dfd604b: did not touch this. --- #### 2. Nothing in the scanner bounds a single job **Severity: HIGH — CONFIRMED** `scanner/internal/scan/worker.go:107`, `extractor.go:49`, `syft.go:33,46`, `grype.go:114` `processJob` takes a `ctx`, but the `ctx` it takes is the worker pool's, created in `main.go:74` and cancelled only at shutdown. There is no per-job deadline anywhere. And the phases do not honour cancellation even in principle: - `buildOCILayout` takes no context at all. Its only bound is `client.httpClient`'s 5-minute `Timeout` (hold.go:23), which is **per HTTP request**, and the requests are serial: config, then every layer. - `img.Read()` (syft.go:33) — stereoscope layer extraction — takes no context. A layer that decompresses effectively forever holds the worker forever. - `vulnerabilityMatcher.FindMatches` (grype.go:114) takes no context either. `ctx` in `scanVulnerabilities` reaches only the database load. Consequences, both demonstrated: - **Head-of-line blocking.** `scanner.workers` defaults to 1, so a wedged job stops every job behind it. `TestStuckJobBlocksEveryJobBehindIt` shows a second job acked immediately and then never started, with no terminal message for either. - **The one timeout that exists multiplies.** `TestBlobDownloadsAreSerialSo­TheirTimeoutsAdd` shows 4 blobs fetched strictly serially (peak concurrency 1). A 19-layer image therefore has a worst case of 20 × 5 minutes = 100 minutes on the download phase alone, against the hold's 10-minute deadline (finding 3). Fix (described): wrap each job in `context.WithTimeout` in `worker()`, thread that context into `buildOCILayout` (and through `downloadBlob` → `http.NewRequestWithContext`), and add a wall-clock guard around the Syft/Grype phases — those two cannot be cancelled from inside, so the practical options are a watchdog that fails the job and lets a supervisor restart the process, or running extraction in a subprocess. A per-blob byte cap on `io.Copy` in `DownloadBlob` would also bound the "layer is bigger than claimed" case, since nothing checks the downloaded bytes against the claimed size or digest. dfd604b: did not touch this. --- #### 3. The hold's 10-minute processing deadline is measured from dispatch, not from work start **Severity: HIGH — CONFIRMED** `pkg/hold/pds/scan_broadcaster.go:526` (handleAck), `:833-837` (timeout) The scanner acks on *receipt*, in `connectOnce` at client/hold.go:159, before `queue.Enqueue`. `handleAck` moves the row `assigned` → `processing` and touches nothing else, so `assigned_at` still holds the dispatch time. `reDispatchTimedOut` then fails any `processing` row with `assigned_at < now-10m`. The ten minutes therefore cover **queueing**, not scanning. Quantified, with the shipped defaults (`scanner.workers: 1`, `scanner.queue_size: 100`, `JobCooldown` 10s): - A worker's floor per job is the 10s cooldown alone. Job N cannot start before `10*(N-1)` seconds after the burst is acked, so **job 61 is past the deadline before a worker touches it, even if every scan were free**. - At a realistic 30s scan (40s per job) the ceiling is **15 jobs**. - The queue is 100 deep and every one of those 100 is acked on arrival, so a drain of a backlog marks up to 85 rows `processing` that are mathematically guaranteed to blow the deadline. `TestAcksLandLongBeforeTheWorkDoes` shows a 6-job burst acked within sub-millisecond of each other and then draining serially, and asserts the 60-job ceiling so a change to either constant surfaces here. `TestScanAck_DoesNotRestartTheProcessingClock` pins the hold half: ack does not move `assigned_at`, and a job acked one second ago is failed. Where the burst comes from: the proactive `dispatchLoop` gates on `waitForCapacity`, so it only ever has one job outstanding. The deep backlogs come from `drainPendingJobs` on reconnect and from push-triggered `Enqueue`. Fix (described): stamp a separate `started_at` (or reset `assigned_at`) in `handleAck`, and better, have the scanner ack only when a worker picks the job up — the ack currently means "received", which is not what the hold reads it as. Cheaper interim: make the scanner send a `heartbeat`/`started` message the hold uses to extend the deadline. dfd604b: did not touch this. dfd604b's own comment ("assigned and processing jobs need no such bound — the ack and processing timeouts always resolve them", scan_broadcaster.go:32) leans on this timeout being correct. --- #### 4. The processing timeout leaks the manifest digest out of the in-flight set forever **Severity: HIGH — CONFIRMED** `pkg/hold/pds/scan_broadcaster.go:833-841` Every proactive enqueue path adds the manifest digest to `sb.inflight` (:293, :1107, :1204) and relies on a terminal scanner message to remove it: `handleResult` :635, `handleError` :683, `handleSkipped` :728. The processing timeout is not one of those. It is a bare `UPDATE ... SET status='failed'` that never looks at the digest. So a job the scanner never answers for leaves its digest in the set permanently. `discoverUnscannedForUser` (:1107) and `runStalePass` (:1204) both `continue` when `addInflight` returns false, so **that image is never offered for scanning again for the life of the hold process**. No scan record is written either (unlike `handleError`, which writes a failed record), so nothing in the UI or the stale loop knows it was ever attempted. The leak is permanent only when the scanner never speaks for that seq again — `handleResult` has no status guard, so a late result flips `failed` → `completed` and releases the digest. The cases where it does not come back are exactly findings 2 and 6. Reproduce: `TestScanProcessingTimeout_LeavesTheDigestInFlightForever` and `TestScanResult_ArrivingAfterTheTimeoutHealsTheRow`. Fix (described): the timeout UPDATE should `RETURNING manifest_digest` (or SELECT first) and call `removeInflight` + `signalCompletion` for each row it fails, exactly as `handleError` does — and write a failed scan record so the attempt is visible. dfd604b: did not touch this. It is arguably *more* reachable after dfd604b — see finding 5. --- #### 5. The timeout frees dispatch capacity while the scanner is still wedged, so doomed jobs drip out every 10 minutes **Severity: HIGH — CONFIRMED** `pkg/hold/pds/scan_broadcaster.go:1564` (hasActiveJobs), `:833` `hasActiveJobs` counts `processing` rows with no age bound, so a wedged scan holds `waitForCapacity` still — until the 10-minute timeout marks it `failed`, at which point capacity is free, `dispatchLoop` pops the next candidate, and hands it to *the same scanner*, whose only worker is still stuck on the first job. Every ten minutes: one more job dispatched, acked, queued behind the wedge, failed by the timeout, and one more digest leaked (finding 4). After about 17 hours the scanner's 100-deep queue is full and new jobs come back as `scanner queue full` errors instead. Reproduce: `TestScanProcessingTimeout_ReleasesCapacityWhileTheScannerIsStillWedged`. **This is the part of the nine-day outage dfd604b did not fix.** dfd604b correctly diagnosed that one undispatchable *pending* row froze everything, and bounded `pending` in `hasActiveJobs` with `pendingStaleAfter`. It deliberately left `assigned`/`processing` unbounded on the argument that their timeouts always resolve them. They do resolve the *row*; they do not resolve the *scanner*. So for a scanner-side wedge the fix converts a visible freeze into a silent drip that also leaks in-flight digests. Both symptoms end in "no scans are produced", but the second one is harder to see, because `logStalledCapacity` (:1580) never fires — capacity is never blocked for the ten minutes it warns after. Fix (described): finding 4's fix plus something that notices a scanner which has had N consecutive jobs time out — drop that subscriber's socket so the worker pool restarts, or stop dispatching to it. --- #### 6. A terminal message computed while the socket is down is dropped silently **Severity: HIGH — CONFIRMED** `scanner/internal/client/hold.go:204-215`, `:47`, `:66` `sendJSON` guards only on `c.conn == nil`, and nothing ever nils `conn`: `connectOnce` assigns it on dial (:103) and the read loop just returns on error. So a result, error, skip or ack produced between a disconnect and the next dial is written to a closed connection; `WriteJSON` fails, `sendJSON` logs it and returns. `SendResult`/`SendError`/`SendSkipped` return nothing, so the worker cannot tell, does not retry, and moves on to the next job. The scan is done, and its answer is gone. The loss window is the reconnect backoff, which is a flat 5 seconds — `Connect` declares `var cursor int64 = -1` and never assigns it (so the cursor is dead) and its comment claims exponential backoff to 30s while the code sleeps 5s (:66). Reproduce: `TestResultComputedWhileTheSocketIsDownIsLost` — job acked, socket dropped mid-download, job completes, scanner redials, and no terminal message for that seq ever reaches the hold. The hold only learns about it via the 10-minute processing timeout, which then does finding 4 to it. Fix (described): make `sendJSON` return an error and have the worker retain the terminal message for redelivery after the next connect (a small outbox keyed by seq), or at minimum nil `conn` on disconnect and have `sendJSON` block briefly for a live connection. Note the mirror case: if the worker finishes *after* the redial, the message is delivered on the new socket, where the hold has already re-dispatched the job — see finding 7. dfd604b: did not touch this. --- #### 7. Re-dispatch after a disconnect runs the same scan twice, concurrently **Severity: MEDIUM — CONFIRMED (the prediction is correct)** `pkg/hold/pds/scan_broadcaster.go:385` (Unsubscribe), `:740` (drainPendingJobs), `scanner/internal/client/hold.go:162` (Enqueue with no dedupe) `Unsubscribe` flips the dropped subscriber's `assigned` and `processing` rows back to `pending`. Nothing tells the scanner: the job it was running is still running and the copies still in its priority queue are still queued. On reconnect `drainPendingJobs` hands the same seq straight back, and `queue.Enqueue` has no dedupe by seq or by manifest digest. Demonstrated by blob accounting, which is the only place it shows: `TestReDispatchAfterDisconnectScansTheSameImageTwice` (workers=2) sees the same config blob fetched twice with **peak concurrency 2** — two workers downloading and cataloguing the same image at once. The hold-side assumption is pinned separately by `TestScanUnsubscribe_ReoffersAJobTheScannerIsStillRunning`. With the default single worker the duplicate is serialized rather than concurrent, but it is still a full duplicate scan: double the S3 egress, double the CPU, and two results for one seq (the second overwrites the first's record). Fix (described): dedupe in the scanner by seq — a small set of in-queue and in-progress seqs, dropping a re-offer that is already known, or acking it immediately as a duplicate. The hold cannot solve this alone, since it has no way to recall a job it already sent. dfd604b: did not touch this. (It did add the `RowsAffected` guard in `dispatchJob` at :426, which prevents *two hold-side dispatchers* racing for one row; it does nothing about the scanner already holding a copy.) --- #### 8. `drainPendingJobs` marks rows assigned to a scanner it never sent them to **Severity: MEDIUM — CONFIRMED** `pkg/hold/pds/scan_broadcaster.go:775-792` `dispatchJob` handles a full `sub.send` correctly: its `default` branch resets the row to `pending` (:445). `drainPendingJobs` does not. It marks each row `assigned` first, then blocks up to 5 seconds on the send (:788) and gives up on timeout — leaving that row, and implicitly every row after it, marked `assigned` to a subscriber that was never handed them. Those rows sit until the 5-minute ack timeout reclaims them, holding dispatch capacity the whole time. Reproduce: `TestScanDispatchQueue_BuriesJobsWhenTheScannerBufferFills` — 4 pending jobs, a 2-deep buffer, and the result is 3 assigned / 1 pending where only 2 were actually sent. Fix (described): reset the row to `pending` in the timeout branch before returning, the way `dispatchJob` does. dfd604b: did not touch this. --- #### 9. A hung Grype match stalls every other worker via the vuln-DB lock **Severity: MEDIUM — SUSPECTED (code reading; not demonstrated)** `scanner/internal/scan/grype.go:98-115`, `:175-190` `scanVulnerabilities` holds `vulnDBLock.RLock()` across `FindMatches`, which takes no context and cannot be cancelled. `loadVulnDatabase` takes the *write* lock (:187) whenever the DB is stale or on the periodic every-50-scans reload (:194). Go's `RWMutex` blocks new readers once a writer is queued — the comment at :102 relies on exactly that — so a single wedged `FindMatches` blocks the queued reload, which blocks every other worker's `loadVulnDatabase` call, and the whole pool stops. Only relevant with `scanner.workers > 1`; the shipped default is 1. Not demonstrated: provoking it needs a real Grype database and a matcher that hangs. Listed so the fix agent bounds the matcher (finding 2) rather than only the download. dfd604b: did not touch this. --- #### 10. Reconnect backoff is a flat 5s and the resume cursor is dead **Severity: LOW — CONFIRMED (already known, restated for completeness)** `scanner/internal/client/hold.go:47`, `:66` `var cursor int64 = -1` is never reassigned, so `?cursor=` is never sent and `drainPendingJobs` always runs from 0. The comment above the sleep claims exponential backoff to 30s; the code sleeps a flat 5s. Both are visible in `TestResultComputedWhileTheSocketIsDownIsLost`, which waits out exactly 5s for the redial. --- #### 11. The harness's `JobCooldown` restore raced with live workers (test infrastructure — since fixed) **Severity: LOW — CONFIRMED, and fixed by the harness owner during this investigation** `scanner/internal/e2e/harness.go:99-122` vs `scanner/internal/scan/worker.go:142` `go test -race` failed on *any* e2e scenario, including the pre-existing `TestSkipsHelm`: the cleanup wrote `scan.JobCooldown` while a worker goroutine that outlived the test was still reading it in its cooldown select. Verified against `harness.go pipeline_test.go` alone, so it was not caused by the new tests. The harness cleanup now joins the pool (`cancel(); c.Close(); q.Close(); pool.Wait()`) before restoring the variable, and `go test -race harness.go stuck_test.go` is clean. Recorded only so nobody reintroduces the ordering. Note for anyone adding scenarios: `pool.Wait()` in cleanup means a test that stalls a blob download **must** release the stall before the harness tears down, or teardown blocks on the wedged worker. Register the release with `t.Cleanup` *after* `Start`, so it runs first. #### Summary against the five hypotheses in the brief | # | Hypothesis | Verdict | |---|---|---| | 1 | Worker blocked forever, no deadline | CONFIRMED (finding 2) — no per-job ctx at all; extraction and matching are uncancellable | | 2 | Hold bookkeeping wedged; what dfd604b did and did not fix | CONFIRMED (findings 4, 5) — dfd604b fixed the `pending` freeze only; `processing` still leaks in-flight digests and now drips doomed jobs | | 3 | Ack timing mismatch | CONFIRMED and quantified (finding 3) — ceiling is 60 jobs with free scans, ~15 realistically | | 4 | Disconnect produces duplicate concurrent scans | CONFIRMED (finding 7) — peak concurrency 2 on the same digest | | 5 | Results computed while the socket is down are dropped | CONFIRMED (finding 6) | Ordering for a fix agent: finding 1 first (it is a live crash on both processes and it blocks the e2e suite), then 4 + 5 together (they are one change in `reDispatchTimedOut`), then 2, then 6, then 3, then 7 and 8. ## 9. Blob and OCI layout edge cases (execution) ##### Scanner blob download / OCI layout findings Scope: `scanner/internal/scan/extractor.go` (buildOCILayout, downloadBlob, digestHex), `scanner/internal/client/hold.go` (GetBlobPresignedURL, DownloadBlob, httpClient), `scanner/internal/scan/worker.go` (size guard, skipReason). All evidence comes from `scanner/internal/e2e/blob_edge_test.go`, run with `cd scanner && go test ./internal/e2e/ -run '' -v`. Every test in that file passes (one is `t.Skip`ped, see B-01), and the file is clean under `-race`. Nothing here was fixed. Two known findings are assumed and not re-derived: the `worker.go:129` `result.Summary` nil-dereference BLOCKER, and the index-manifest retry loop. --- #### Retryable-vs-permanent summary (read this first) The hold retries `"error"` on the stale-scan loop (`pkg/hold/pds/scan_broadcaster.go:1145` `staleScanLoop` → `runStalePass`, which skips only `ScanStatusSkipped` records at line ~1215) and never retries `"skipped"`. Every failure below is reported as `"error"`, and every one of them is a **deterministic, permanent** property of the manifest record or of the stored blob. Each is therefore an unbounded retry loop in production: the hold re-offers the job on every stale pass, forever, and gets the same answer. Confirmed permanently-failing shapes that come back retryable: | shape | test | error the hold records | |---|---|---| | layer blob 404 (GC'd) | `TestMissingBlobIsRetryableError` | `failed to get presigned URL ...: hold returned status 404` | | layer bytes ≠ digest | `TestDigestIsNeverVerified` | `failed to load OCI image: failed to read layer=... tar : unexpected EOF` | | layer shorter than declared | `TestShortBlobIsAcceptedAndConfusesSyft` | `failed to load OCI image: unexpected EOF` | | config blob not JSON | `TestUnparseableConfigBlobIsRetryableError` | `failed to load OCI image: invalid character 't' ...` | | digest with no algorithm prefix | `TestMalformedDigestsAreAllRetryable` | `cannot parse hash: "efef..."` | | non-sha256 algorithm | `TestMalformedDigestsAreAllRetryable` | `unsupported hash: "sha512"` | | index manifest (already known) | `TestIndexManifestIsRetriedForever` | `config blob has empty digest` | Only one blob-stage failure is genuinely transient and correctly retryable: a 5xx from getBlob, and an expired presigned URL (B-05, B-06). --- #### B-01 — Digest path traversal writes downloaded bytes outside the blobs directory **Severity: HIGH (security)** — **CONFIRMED** `scanner/internal/scan/extractor.go:184` (`digestHex`), `scanner/internal/scan/extractor.go:173-174` (`downloadBlob`), `scanner/internal/client/hold.go:277` (`os.Create`). ##### What happens `digestHex` splits the digest on the first `":"` and returns the remainder verbatim, with no validation that it is a hex string: ```go func digestHex(digest string) string { if _, hex, ok := strings.Cut(digest, ":"); ok { return hex } return digest } ``` `downloadBlob` then does `filepath.Join(blobsDir, hex)` and hands that to `client.DownloadBlob`, which `os.Create`s it. `filepath.Join` cleans `..` segments *after* joining, so a digest of the form `sha256:../../../name` names a path outside the scan directory, and the response body is written there. Both `job.Config.Digest` and every `job.Layers[i].Digest` reach this path. Those strings originate in an `io.atcr.manifest` record in the *user's own PDS*, which the user can write directly (a push is not the only way a record gets there), so this is attacker-controlled input on a shared scanner host. This is the classic "unsanitised, externally supplied path segment used to build a filesystem path" (CWE-22) shape, made worse by the fact that the traversal string never has to look like a digest to anything upstream. ##### Blast radius (measured, not assumed) - The write lands wherever the traversal points. `os.Create` truncates, so an existing file the scanner user can write is clobbered. - The parent directory must already exist: a probe with `sha256:../../../no-such-dir/deep/x` returned `failed to create file: open .../no-such-dir/deep/x: no such file or directory`. There is no `MkdirAll`, so the attacker picks among existing directories. - The download must return 200 before `os.Create` is reached (status check at `client/hold.go:273` precedes it), so a bogus digest that 404s in S3 writes nothing. **Reachability of a 200 for a traversal digest in production is SUSPECTED, not confirmed.** Candidate routes, in decreasing plausibility: a BYOS hold that is malicious or compromised (it chooses both the digest it dispatches and the URL getBlob returns, so the file *content* is attacker-chosen too); the hold's own XRPC proxy fallback in `pkg/hold/pds/xrpc.go:1652`, which returns a 200 JSON body from getBlob itself when the S3 client is unavailable; and any S3/HTTP path normalisation that collapses `..` in the presigned key (`pkg/s3/types.go:459` `BlobPath` interpolates the digest straight into the key). - The written file is outside `scanDir`, so `buildOCILayout`'s cleanup never removes it. It persists after the scan. - Even where content is not attacker-chosen, the primitive is "overwrite an arbitrary existing-directory path with blob bytes as the scanner user". ##### Reproduce `TestDigestPathTraversalEscapesBlobsDir`. A layer descriptor with digest `sha256:../../../escaped-marker` (three levels up from `blobs/sha256` is exactly the scanner's configured tmp dir) causes 50 bytes to be written to `/escaped-marker`, which the test reads back and compares. The scan itself then fails, so nothing else about the run signals that a file was placed outside the layout. ##### Suggested fix (not applied) Validate the digest before it is ever used as a path: require `^[a-z0-9]+:[a-f0-9]{32,}$` (or parse with `go-digest`), reject anything else as a **skip** rather than an error (it can never succeed), and derive the on-disk name from the validated hex only — ideally `blobs//` per the OCI layout spec rather than always `blobs/sha256`. A belt-and-braces check that `filepath.Clean(destPath)` still has `blobsDir + string(os.PathSeparator)` as a prefix belongs in `downloadBlob` regardless. --- #### B-02 — Downloaded bytes are never checked against the digest or the declared size **Severity: HIGH** — **CONFIRMED** `scanner/internal/scan/extractor.go:172-181`, `scanner/internal/client/hold.go:266-288`. ##### What happens Nothing in the scanner hashes a downloaded blob, and nothing compares the bytes written against `BlobDescriptor.Size`. `DownloadBlob` copies the body to disk and returns; `buildOCILayout` writes the *declared* digest and size into the layout manifest it hands to Syft. Two distinct consequences: 1. **Integrity.** A hold that serves the wrong object (mixed-up S3 key, poisoned cache, compromised BYOS hold) produces an SBOM and a vulnerability report describing bytes that are not the image. A compliant OCI client verifies digests on pull and would refuse the same bytes, so the scan record can attest to content no client would ever run. The scanner is the only consumer in the system that trusts the hold's bytes unconditionally. 2. **Diagnosis.** When the bytes are simply corrupt, the failure surfaces from deep inside stereoscope: `failed to read layer="sha256:9ae8..." tar : unexpected EOF` for wrong content, plain `unexpected EOF` for a short layer. Neither mentions a digest or size mismatch, so an operator reading a failed scan record has no way to tell "the hold gave us the wrong bytes" from "this image is broken". And because it is an `"error"`, it is retried forever. Note the honest half: a body that is short *relative to its own Content-Length* is caught by `io.Copy` (`failed to write blob: unexpected EOF`) — `TestTruncatedBodyIsDetected` pins that. It is the internally-consistent short body, and the wrong-content body, that sail through. ##### Reproduce - `TestDigestIsNeverVerified` — non-gzip bytes served under a valid layer digest. - `TestShortBlobIsAcceptedAndConfusesSyft` — a quarter of the layer, served with a matching Content-Length, against a descriptor declaring the full size. - `TestTruncatedBodyIsDetected` — the case that *is* handled. ##### Suggested fix (not applied) Hash while copying (`io.MultiWriter` into a `sha256.New()`) and compare against the digest, and compare the byte count against `descriptor.Size` when it is non-zero. On mismatch, fail with a message naming the digest, the expected and actual hash/size. Treat a size/digest mismatch on a blob the hold *served* as retryable (it may be a transient storage fault) but consider a permanent skip after N attempts, since today it retries unboundedly. --- #### B-03 — MaxImageSize is measured against numbers the manifest supplies, not bytes transferred **Severity: MEDIUM** — **CONFIRMED** `scanner/internal/scan/worker.go:212-221`. ##### What happens ```go if wp.cfg.Vuln.MaxImageSize > 0 { var totalSize int64 for _, layer := range job.Layers { totalSize += layer.Size } totalSize += job.Config.Size if totalSize > wp.cfg.Vuln.MaxImageSize { ... } } ``` `layer.Size` comes from the same user-writable manifest record as the digests, and no counter downstream measures what actually arrives. A record declaring one byte per blob passes any ceiling and the scanner then writes the real bytes to its tmp volume. Combined with B-02 (no size verification) there is no bound at all on disk consumed by one scan job. Measured: with `MaxImageSize` = 1024 and both descriptors declaring 1 byte, the scanner transferred **65,696 bytes** — 64× the limit — without complaint. Scaling the served body is the attacker's choice; nothing in the pipeline notices. The `0 = no limit` behaviour is as documented and is confirmed separately (`TestMaxImageSizeZeroDisablesTheGuard`): the guard is inside `if > 0`, so a descriptor claiming 1 TiB is downloaded without a word. ##### Reproduce `TestMaxImageSizeCountsClaimedBytesNotTransferred`, `TestMaxImageSizeZeroDisablesTheGuard`. ##### Suggested fix (not applied) Keep the cheap pre-check (it is still worth rejecting an honestly-large image before downloading), but enforce the real ceiling during transfer: wrap the copy in an `io.LimitedReader` sized to the remaining budget, decrement the budget per blob, and abort the scan when it is exhausted. That also gives B-02's size check for free. --- #### B-04 — A stalled download has no deadline shorter than five minutes, and cannot be cancelled **Severity: MEDIUM** — **CONFIRMED** (behaviour), timeout value confirmed by inspection `scanner/internal/client/hold.go:23`, `:267`; `scanner/internal/scan/extractor.go:176-180`. ##### What happens `httpClient` is `&http.Client{Timeout: 5 * time.Minute}`, and neither `GetBlobPresignedURL` nor `DownloadBlob` builds a request with a context. `processJob` receives the worker's `ctx` but never passes it down. So: - The ceiling is **per request**, not per job. Downloads are strictly sequential (confirmed: `TestManyLayersAreFetchedSequentially` shows 20 ordered fetches for the corpus's widest manifest), so a hold that accepts connections and then stalls costs `5 min × (1 + len(layers))` for one job — 100 minutes for the 19-layer manifest in the corpus — during which that worker scans nothing else. With the shipped default of one worker, the whole scanner is stalled. - Cancelling the worker context (shutdown, SIGTERM, pool teardown) does **not** abort a download in flight. Shutdown waits out the HTTP timeout. Measured with a 2-second stall: the worker waited the full stall and only then reported `failed to write blob: unexpected EOF`. There is no shorter deadline. ##### Reproduce `TestStalledDownloadHasNoShortDeadline` (deliberately stalls 2s, not 5min). ##### Suggested fix (not applied) Thread `ctx` from `processJob` through `buildOCILayout` → `downloadBlob` → `client.GetBlobPresignedURL`/`DownloadBlob` using `http.NewRequestWithContext`, add a per-job deadline derived from the image size, and consider a `ResponseHeaderTimeout` plus a stall detector (no bytes for N seconds) on the transport rather than one flat whole-request timeout. **Seam needed for testing:** `client.httpClient` is an unexported package variable, so a test cannot shorten the 5-minute timeout. Exporting a `client.SetHTTPClient` (or taking the client as a parameter) would let a test prove the timeout fires rather than merely proving no shorter one exists. --- #### B-05 — DownloadBlob discards the response body from a failed download **Severity: LOW** — **CONFIRMED** `scanner/internal/client/hold.go:273-275`. ```go if resp.StatusCode != http.StatusOK { return fmt.Errorf("download returned status %d", resp.StatusCode) } ``` An expired presigned URL produces exactly `failed to download config blob: download returned status 403` in the hold's scan record: no URL, no host, no S3 `AccessDeniedRequest has expired`, and no indication of *which* blob when several layers are involved (the wrapping in `buildOCILayout` gives a layer index, but the config path gives nothing). `GetBlobPresignedURL` at `:250-253` does include the body; `DownloadBlob` should match it. Reproduce: `TestExpiredPresignedURL`. Fix: include a bounded read of the body (say 1 KiB) and the digest in the error, as the sibling function already does. --- #### B-06 — A blob 404 (garbage-collected layer) is retried forever **Severity: MEDIUM** — **CONFIRMED** `scanner/internal/scan/extractor.go:90-93`, dispatched at `worker.go:112-121`. A layer collected out from under a queued job makes getBlob answer 404, and the scanner reports `"error"`. The hold marks the scan failed (`scan_broadcaster.go:647` `handleError`), and the stale loop re-offers failed records on every pass. The blob is not coming back, so this is a permanent job that burns a dispatch slot and a worker on every rescan interval, forever. Reproduce: `TestMissingBlobIsRetryableError`. Fix: distinguish 404/410 from other download failures and return a `*SkipError` ("blob no longer present") so the hold records a permanent skip; keep 5xx and network errors retryable. The same treatment fits the malformed-digest cases in B-07, which can also never succeed. --- #### B-07 — Malformed digests reach the layout and fail late, as retryable errors **Severity: LOW** — **CONFIRMED** `scanner/internal/scan/extractor.go:184-189`, `:61`, `:79-94`. `digestHex` accepts anything. Consequences confirmed by test: - **No algorithm prefix** (`efef...` with no colon): fetched anyway, written to `blobs/sha256/efef...`, and stereoscope fails with `cannot parse hash: "efef..."`. - **Non-sha256 algorithm** (`sha512:cdcd...`): the blobs directory is hardcoded to `blobs/sha256` (`extractor.go:61`), so a sha512 blob is written to the sha256 directory while the manifest descriptor says sha512. Stereoscope fails with `unsupported hash: "sha512"`. If Syft ever gains sha512 support this becomes a silent "blob not found" instead, because the file is in the wrong directory. - **Empty digest among valid layers**: correctly skipped from both the download loop and the layout manifest (`TestEmptyLayerDigestIsSkipped` — exactly two blobs fetched). This one is handled. - **Zero-size layer**: fetched (the size field is not consulted at download time). See B-08 for what happens when the layout is otherwise valid. All the failing cases are permanent and all come back as `"error"`. Reproduce: `TestMalformedDigestsAreAllRetryable`, `TestEmptyLayerDigestIsSkipped`, `TestZeroSizeLayerIsFetched`. Fix: validate digests in `skipReason` (before any download) and return a `*SkipError`; use `blobs//` for the layout path. --- #### B-08 — A zero-byte layer scans clean rather than failing **Severity: LOW** — **CONFIRMED** (by running the skipped test once with the skip removed) `scanner/internal/scan/extractor.go:79-94`. With a valid config and a layer descriptor whose blob is zero bytes, stereoscope accepts the empty layer, Syft catalogs nothing, and the pipeline returns a successful result with an SBOM containing no packages. There is no signal anywhere that the image was empty: a clean scan and a scan of nothing are the same record. Anything that reads "0 vulnerabilities" off such a record is reading an artefact of a broken image. Reproduce: `TestZeroSizeLayerProducesACleanScan`, which is `t.Skip`ped **because running it crashes the test binary** on the known `worker.go:129` BLOCKER (a successful scan with `vuln.enabled=false` dereferences a nil `Summary`). Remove the skip once that is fixed. The skip reason names the bug. Fix: reject a layer whose transferred size is zero (or whose size disagrees with its descriptor — B-02 covers it), and consider failing a scan that produced zero packages *and* zero readable layers rather than reporting it as clean. --- #### B-09 — A duplicated layer digest is downloaded twice **Severity: LOW** — **CONFIRMED** `scanner/internal/scan/extractor.go:79-94`, `:107-119`. The download loop and the manifest builder both walk `job.Layers` with no de-duplication. A manifest listing the same digest twice fetches the blob twice over the network, writes it twice to the same path, and lists it twice in the layout manifest. A separate probe (valid config, duplicated real layer) showed Syft accepts the duplicate and the scan *succeeds*, so the only cost is wasted bandwidth and time — proportional to layer size, and chosen by whoever writes the manifest record. Reproduce: `TestDuplicateLayerIsDownloadedTwice` (asserts 2 fetches of one digest). The success-path half is not a test because a successful scan panics (worker.go:129). Fix: track digests already downloaded in `buildOCILayout` and skip the repeat fetch; leave the layout manifest as-is (the OCI spec permits repeated layers). --- #### B-10 — Blob downloads follow redirects anywhere, with the client's default 10-hop policy **Severity: LOW** — **CONFIRMED** `scanner/internal/client/hold.go:267`. `DownloadBlob` uses `httpClient.Get` with no `CheckRedirect` policy and no allow-list on the URL getBlob returned. Confirmed: - A redirect to a completely different host is followed, and the bytes from there are used (and, per B-02, never verified). **No credentials leak** — the request carries no `Authorization` header at all, which the test asserts. - A redirect loop costs 10 round trips per blob before erroring with `stopped after 10 redirects`, and the error names the URL, not the blob. This is not a vulnerability on its own (the hold is a trusted peer and the presigned URL is expected to be off-host), but it means the choice of server is entirely the hold's, and combined with B-02 the scanner will catalog whatever comes back. Reproduce: `TestDownloadFollowsRedirectToAnotherHost`, `TestRedirectChainStopsAtTen`. Fix: optional. If tightened, verify the digest (B-02) rather than restricting hosts, since presigned URLs legitimately point at arbitrary S3 endpoints. --- #### Non-findings (checked, and clean) Worth recording so nobody re-derives them: - **Temp directory cleanup is correct on every error path exercised here.** Every failure mode above leaves `Vuln.TmpDir` completely empty (`assertNoLeakedScanDirs` runs in all of them, including the `generateSBOM`-failure path in `TestTmpDirIsCleanAfterAFailureInsideSyft`). `buildOCILayout` calls `cleanup()` on each of its own error returns, and `processJob`'s `defer cleanup()` at `worker.go:229` covers the Syft and Grype stages. Stereoscope's own scratch space under the same `TMPDIR` is also cleaned. The one exception is the file written *outside* the scan directory in B-01, which cleanup cannot see. - **No per-layer file-descriptor leak.** Three consecutive 19-layer jobs (20 downloads each) held the process at a steady 13 open descriptors. `TestManyLayersAreFetchedSequentially` asserts this. - **A body shorter than its declared Content-Length is caught** by `io.Copy` (B-02 notes this); the blob does not reach Syft. - **`MaxImageSize: 0` means unlimited**, as documented. ## 10. Protocol, lifecycle and configuration (execution) ##### Scanner findings: message protocol, worker lifecycle, queue, configuration Scope: `scanner/internal/client/hold.go`, `scanner/internal/queue/priority_queue.go`, `scanner/internal/scan/worker.go`, `scanner/internal/config/config.go`, and the hold's side in `pkg/hold/pds/scan_broadcaster.go`. Tests: `scanner/internal/e2e/protocol_test.go` (12 tests, all green), `scanner/internal/config/config_edge_test.go` (5 tests, all green). Nothing is skipped: every finding below is pinned by a test that passes against the current behaviour, and each such test says in its failure message that the finding has been fixed if the behaviour changes. Run them with: ``` cd /home/data/atcr.io/scanner go test ./internal/config/ -count=1 go test ./internal/e2e/ -count=1 -run 'Unparseable|NullConfig|UnknownMessage|DuplicateSeq|QueueFull|ZeroQueueSize|ZeroWorkers|EmptyTmpDir|HighTier|QueueClose|Shutdown|HoldClientClose' ``` (The whole `internal/e2e` package cannot run as one binary today: `TestScanRealImage` hits the known worker.go:129 BLOCKER as soon as the blob fixtures are present, and the panic takes the test process down with it.) **Harness change, flagged as required:** I added two additive helpers to `scanner/internal/mockhold/mockhold.go` — `SendRaw([]byte) error` and `NextSeq() int64`. `SendJob` always marshals a well-formed frame, so without a raw writer the entire "frame the scanner cannot parse" surface (findings 1 and 2, the most serious ones here) is untestable. No existing mockhold behaviour was changed and no production code was touched. --- #### 1. A frame the scanner cannot parse is dropped in total silence, and one such job freezes proactive scanning for the whole hold **Severity: BLOCKER — CONFIRMED** `scanner/internal/client/hold.go:121-124` (frame), `:133-136` (config), `:139-142` (layers) `pkg/hold/pds/scan_broadcaster.go:1564-1576` (`hasActiveJobs`), `:829-903` (`reDispatchTimedOut`) **What happens.** `connectOnce` decodes three things: the frame, then `config`, then `layers`. Every one of the three failure branches is `slog.Error(...); continue`. Nothing is sent back — not an ack, not an error, not a skip. Note the ordering: the ack is at `:159`, *after* both sub-document unmarshals, so a job whose config or layers do not decode is never even acknowledged. The hold wrote `status='assigned'` before it wrote the frame. Its only escape is the five-minute `ackTimeout`, after which `reDispatchTimedOut` (ticking every 30s) resets the row to pending and immediately re-dispatches it — to the same scanner, which drops it again for exactly the same reason. A decoding disagreement is permanent, so this is an infinite five-minute retry loop with nothing in either log that names the job as stuck. The real damage is not the one job. `hasActiveJobs()` counts rows in `assigned` or `processing`, and `dispatchLoop` admits proactive scan candidates one at a time behind `waitForCapacity()`. A row that is in `assigned` essentially 100% of the time (5 minutes assigned, re-dispatched within 30s of timing out, assigned again) means `hasActiveJobs()` never returns false and **no proactive scan is ever dispatched again, deployment-wide**. The comment at `scan_broadcaster.go:32-35` says this exact failure mode already cost nine days of scanning once. **Reproduce.** `TestUnparseableFramesAreDroppedInSilence` — six frame shapes (not JSON, a JSON array, `config` as a string, `config` absent, `layers` as an object, `layers` absent). Each asserts no message of any kind for that seq within a second, and the test then proves the WebSocket is still healthy, which is what makes the loop infinite rather than self-limiting. **Suggested fix (not applied).** Every `continue` in the read loop that has a usable `raw.Seq` should call `c.SendSkipped(raw.Seq, ...)` first — an undecodable job is a permanent condition, so "skipped" is the correct verdict, not "error". A frame that does not decode far enough to yield a seq has no addressable job and can only be logged, but the hold should also grow a bound: a row re-dispatched N times without an ack belongs in `failed`, not back in the rotation, and `hasActiveJobs` should not let one poisoned row gate the entire dispatch loop. --- #### 2. The hold panics on a result with no summary, killing the hold process **Severity: BLOCKER — CONFIRMED (by inspection; not executable from the scanner module)** `pkg/hold/pds/scan_broadcaster.go:642` (also the guard it escapes at `:599`) **What happens.** `handleResult` correctly guards record creation with `if msg.Summary != nil` at `:599`. The closing log statement at `:637-643` is *outside* that guard and reads `msg.Summary.Critical`, `msg.Summary.High`, `msg.Summary.Total`. slog evaluates its arguments at the call site, so a `result` message with no `summary` is a nil pointer dereference inside `handleReader`, which runs as a bare goroutine started at `:348` with no recover anywhere in `pkg/hold` — the hold process dies. `summary` is `omitempty` on both sides (`scanner/types.go:76`, `scan_broadcaster.go:107`), and `SendResult` copies `result.Summary`, which `processJob` only sets when `cfg.Vuln.Enabled` (`worker.go:253`). So **a scanner running with `vuln.enabled: false` kills the hold on its first successful scan** — and note the send at `worker.go:124` happens *before* the scanner's own known panic at `:129`, so the poisoned frame is already on the wire. Both processes die, and this is reachable from a supported configuration flag, or from anyone holding the scanner shared secret. There is a second-order bug in the same block: when `Summary` is nil the job is still marked `completed` at `:624-627` with no scan record written, so discovery will rediscover that manifest as unscanned forever. **Reproduce.** Not reachable from the scanner module's tests — it needs a `ScanBroadcaster` with a DB and a PDS, and I was scoped to two test files, neither of which can live in `pkg/hold/pds`. The finding is static and unambiguous: the guarded and unguarded dereferences of the same pointer are 44 lines apart in one function. A hold-side test would be `handleResult(sub, ScannerMessage{Type: "result", Seq: n})` against a broadcaster with row `n` present. **Suggested fix (not applied).** Move the closing log inside the `msg.Summary != nil` branch, or read the counts from locals defaulted to zero. Separately: treat a result with no summary as a protocol error rather than a completion, so the row does not go to `completed` without a record. --- #### 3. `null` config (and anything else that decodes to a zero descriptor) is a permanent failure reported as retryable **Severity: HIGH — CONFIRMED** `scanner/internal/client/hold.go:133-136`, `scanner/internal/scan/extractor.go:70`, `scanner/internal/scan/worker.go:110-118` **What happens.** `"config": null` unmarshals into a zero `BlobDescriptor` without error, so unlike finding 1 the job *is* acked and does enter the pipeline. `skipReason` does not catch it (`""` is not an unscannable media type, and the layer list is non-empty), so it reaches `buildOCILayout`, which fails with "config blob has empty digest, cannot download". The worker reports that as `error`, and `handleError` marks the record failed — which `runStalePass` explicitly retries (`scan_broadcaster.go:1216-1221` retries everything except `skipped`). Nothing about a null config will ever change, so the job is re-scanned on every rescan interval forever. This is the same class as the pre-existing `TestIndexManifestIsRetriedForever`, and it is worth noting the two are the *only* things standing between the scanner and this loop: the hold's `HasScannableContent` guard, and nothing else. **Reproduce.** `TestNullConfigIsAckedThenFailsRetryably`. **Suggested fix (not applied).** Make `skipReason` reject an empty config digest with a `*SkipError`, so a structurally unscannable job is answered "skipped" (permanent) rather than "error" (retryable). That covers index manifests, null configs and absent configs in one guard. --- #### 4. Queue overflow is reported as a per-job scan failure instead of backpressure **Severity: HIGH — CONFIRMED** `scanner/internal/client/hold.go:159-167`, `scanner/internal/queue/priority_queue.go:99`, `pkg/hold/pds/scan_broadcaster.go:650-690` **What happens.** Past the high-water mark the client acks the job (hold: assigned → processing) and then immediately answers `error: scanner queue full`. The hold writes a **failed scan record into the user's scan history** for a condition that has nothing to do with their image, and failed records are retryable, so the same jobs come back on the rescan interval and overflow again while the backlog persists. **Not a hot loop, quantified.** `handleError` moves the row to `failed`, and `reDispatchTimedOut` only re-offers `assigned` and `pending` rows, so nothing re-offers it from `scan_jobs`. The retry comes from `runStalePass`, which sleeps `max(rescanInterval/2, 1h)` between passes and only re-queues records older than `rescanInterval`. So the cadence is hours, not milliseconds — but it is permanent, and every cycle writes another failure record to the PDS. **Reproduce.** `TestQueueFullIsReportedAsRetryableError` (queue_size 2, workers 0, four jobs: the last two are acked and then failed with "queue full"). **Suggested fix (not applied).** Do not ack a job you are about to reject; and give the protocol a distinct "busy"/"nack" verdict that returns the row to `pending` without writing a scan record, so capacity pressure stops appearing in users' scan history as failed scans. Failing that, at minimum send the rejection *before* the ack so the row never enters `processing`. --- #### 5. Nothing dedupes a job: a repeated seq is acked twice, scanned twice, and answered twice **Severity: MEDIUM — CONFIRMED** `scanner/internal/client/hold.go:159-167`, `scanner/internal/queue/priority_queue.go:88-107` **What happens.** Neither the client nor the queue tracks seqs or manifest digests, so a redelivered job is a full second download and Syft run. On the hold side the second **ack** is harmless: `handleAck` guards its UPDATE with `status = 'assigned' AND assigned_to = ?`, which the first ack already cleared, so the second matches zero rows (and still logs "Scan job acknowledged" — the handler does not check `RowsAffected`, so the log cannot distinguish the two cases). The second **terminal message** is not harmless: `handleResult`/`handleError`/`handleSkipped` re-run unconditionally and write a second scan record for the same manifest. Redelivery is reachable: `Unsubscribe` returns `assigned`/`processing` rows to `pending` on a disconnect, and if a result was already in flight when the socket dropped, the job comes back. **Reproduce.** `TestDuplicateSeqIsProcessedTwice`. **Suggested fix (not applied).** Keep a small in-flight set keyed by seq (and optionally manifest digest) in the client, ack the duplicate but do not enqueue it. On the hold side, guard the terminal handlers with `status NOT IN ('completed','failed')` the way `handleAck` guards itself. --- #### 6. Terminal messages are not checked against the subscriber the job was assigned to **Severity: MEDIUM — SUSPECTED (static; needs a hold-side test with two subscribers)** `pkg/hold/pds/scan_broadcaster.go:524-528` vs `:561-571`, `:653-658`, `:698-703` `handleAck` scopes its UPDATE to `assigned_to = sub.id`. None of `handleResult`, `handleError` or `handleSkipped` do: they look the row up by seq alone and complete, fail or skip it. So with two scanners connected, a result from scanner A closes out a job assigned to scanner B (whose own result then arrives for an already-completed row — see finding 5), and any process holding the shared secret can mark any seq completed or failed and write a scan record into a user's PDS. A result for a seq the hold never dispatched is handled correctly: `QueryRow` returns `sql.ErrNoRows`, and `handleResult` logs and returns at `:561-571`. `handleError` and `handleSkipped` are sloppier — they log the lookup failure but then run the UPDATE anyway (matching zero rows) and call `removeInflight("")` — harmless today, but only by accident. **Suggested fix (not applied).** Add `AND assigned_to = ?` to all three terminal handlers, and return early when the row lookup fails. --- #### 7. Result size: no read limit anywhere, so a big SBOM is a memory event rather than a truncation **Severity: MEDIUM — CONFIRMED (no limit exists) / SUSPECTED (OOM consequence)** `grep -rn SetReadLimit` over the repo returns nothing; gorilla's `readLimit` defaults to 0 and `conn.go:933` only enforces `readLimit > 0`. So the plausible silent-failure path in the brief does **not** exist: nothing truncates or closes the connection on a large result. What exists instead is unbounded buffering. The whole SBOM crosses as one text frame and is fully materialised several times over on each side: `sbomJSON []byte` → `string(result.SBOM)` (`client/hold.go:183-185`) → the `WriteJSON` encode buffer on the scanner; `ReadMessage`'s buffer → `json.Unmarshal` → `[]byte(msg.SBOM)` for the S3 upload on the hold. The scanner runs under a 512 MiB `GOMEMLIMIT` (`cmd/scanner/main.go:46`), which the GC cannot honour against a several-hundred-MB frame it is required to hold whole. Related, same area: neither side ever calls `SetWriteDeadline`, and `sendJSON` holds `c.mu` for the duration of the write (`client/hold.go:205-215`). If the hold stops reading, the scanner blocks in `WriteJSON` indefinitely with the mutex held, which also blocks the read loop's `SendAck` — the scanner wedges without disconnecting. I did not build a test for this; filling TCP buffers needs megabytes of real result. **Suggested fix (not applied).** Set an explicit `SetReadLimit` on both sides so an oversize result fails loudly and with a message that names the job, cap SBOM size in the scanner before sending (or upload it out-of-band and send a reference), and set a write deadline so a stalled peer cannot wedge the client. --- #### 8. `scanner.workers = 0` passes validation and strands every job it acks **Severity: HIGH — CONFIRMED** `scanner/internal/config/config.go:114-124` (validation), `scanner/internal/scan/worker.go:75-79` **What happens.** `LoadConfig` validates exactly two things: `hold.url` and `hold.secret` non-empty. `workers: 0` (or a negative value) loads clean, the `for` loop in `WorkerPool.Start` runs zero times, and the pool logs `workers=0` once at boot. The client still acks everything, so the hold moves each job to `processing` and waits out the ten-minute `processing` timeout before failing it — and while it waits, `hasActiveJobs()` is true, so proactive dispatch is frozen exactly as in finding 1. `/healthz` returns 200 throughout. **Reproduce.** `TestZeroWorkersAcksAndStrands` (ack arrives, no terminal in 2s, job still in the queue) and `TestLoadConfigAcceptsSilentlyBrokenValues/workers:_0`. **Suggested fix (not applied).** Reject `workers < 1` in `LoadConfig`, or clamp to 1 with a warning. --- #### 9. `scanner.queue_size = 0` passes validation and fails 100% of jobs **Severity: HIGH — CONFIRMED** `scanner/internal/config/config.go:76`, `scanner/internal/queue/priority_queue.go:99` `NewJobQueue(0)`'s `q.h.Len() >= q.maxSize` is true for an empty queue, so every Enqueue is refused and every job is acked then failed with "scanner queue full" (finding 4's consequences, applied to everything). Negative values behave identically. The scanner looks completely healthy. **Reproduce.** `TestZeroQueueSizeRejectsEveryJob`, `TestLoadConfigAcceptsSilentlyBrokenValues/queue_size:_0` and `/queue_size:_negative`. **Suggested fix (not applied).** Reject `queue_size < 1`. --- #### 10. `vuln.tmp_dir: ""` passes validation and fails every job, retryably **Severity: HIGH — CONFIRMED** `scanner/internal/config/config.go:81` (default), `scanner/internal/scan/worker.go:58-66`, `:207`, `:265` The shipped default is `/var/lib/atcr-scanner/tmp`, and an empty **env var** does not override it (Viper is built without `AllowEmptyEnv`, so `SCANNER_VULN_TMP_DIR=` reads as unset). The empty value is reachable from **YAML**: `vuln: {tmp_dir: ""}` loads clean. Two things then break. `WorkerPool.Start` skips the `TMPDIR` export entirely, which is the hazard the code comments at `:52-57` warn about (Grype's DB download and stereoscope's extraction land on a small tmpfs). But it never gets that far: `processJob` calls `ensureDir("")` at `:207`, and `os.MkdirAll("")` is an error, so **every job fails** with `failed to create tmp dir: mkdir : no such file or directory` — retryably, so all of them loop forever. **Reproduce.** `TestEmptyTmpDirFailsEveryJob` and `TestLoadConfigAcceptsSilentlyBrokenValues/tmp_dir:_empty`. `TestEmptyEnvVarDoesNotOverrideDefaults` pins the accidental protection on the env path. **Suggested fix (not applied).** Reject an empty `vuln.tmp_dir`, and fail the boot (not just each job) if the directory cannot be created. --- #### 11. A malformed config file is ignored whole, silently, in every ATCR service **Severity: HIGH — CONFIRMED** `pkg/config/viper.go:34` — `_ = v.ReadInConfig()` **What happens.** The error from reading the YAML file is discarded. A file that does not parse (a duplicate key, a bad indent, a stray tab) is skipped in its entirety, with no log line and no failure. The process then boots on defaults plus environment. In the shipped scanner deployment `hold.url` and `hold.secret` come from the environment, so even `LoadConfig`'s two checks pass, and the scanner comes up looking perfectly normal with `workers`, `queue_size`, `tmp_dir`, `db_path`, `max_image_size`, log level and log shipping *all* silently reverted to defaults. This is `pkg/config`, not scanner code, so the appview and the hold have the same hole. **Reproduce.** `TestMalformedYAMLIsIgnoredEntirely` (a duplicate-key file; `tmp_dir` and `workers` both come back as defaults). **Suggested fix (not applied).** Return the error from `ReadInConfig` unless it is `viper.ConfigFileNotFoundError` / `fs.ErrNotExist` — an explicitly named config file that cannot be parsed should be fatal. --- #### 12. Other configuration values accepted without complaint **Severity: MEDIUM — CONFIRMED** All pinned by `TestLoadConfigAcceptsSilentlyBrokenValues`: - `hold.url` that is not a URL (`hold.example:8080`): `url.Parse` accepts it, the ws/wss switch does not match, and `Connect` redials a bad address every 5 seconds forever. (`client/hold.go:79-86`) - `vuln.max_image_size: -1`: the guard is `if wp.cfg.Vuln.MaxImageSize > 0` (`worker.go:212`), so a negative ceiling silently means *no* ceiling — the opposite of what someone typing a negative number intends. - `vuln.db_path: ""` with `vuln.enabled: true`: `initializeVulnDatabase` fails in a goroutine and logs "Vulnerability scanning will be disabled until database is available" (`worker.go:69-76`). Nothing actually disables it, so every scan then fails in `scanVulnerabilities` — a retryable error, forever. The log line is misleading, which is worse than silence. - `log_level: "verbose"`: `InitLoggerWithShipper`'s default branch quietly falls back to info (`pkg/logging/logger.go:67-68`), so a typo in the level is invisible. - `server.addr: "nope:not-a-port"`: `ListenAndServe` fails in a goroutine and only logs; the scanner keeps running with no health endpoint, so an orchestrator's liveness probe fails while the process is fine, or (with no probe configured) nobody notices. Summary of the general question: **the only two invalid configurations that fail loudly are an empty `hold.url` and an empty `hold.secret`.** Everything else in the file either fails silently, or is ignored entirely (finding 11). --- #### 13. Priority is queue position, not preemption; the low tier can starve **Severity: LOW — CONFIRMED** `scanner/internal/queue/priority_queue.go:45-51`, `scanner/internal/scan/worker.go:88-142` The heap works: with one worker busy, an owner-tier job that arrives *after* a deckhand-tier job is dequeued first. `TestHighTierJumpsQueuedBacklog` proves that end to end through the real client, queue and worker. The limits are worth stating because "priority" oversells them. Priority is consulted only at `Dequeue`, so a high-tier job that arrives mid-scan waits for the current scan to finish *plus* the full `JobCooldown` (10s in production, `worker.go:154`). With multi-minute scans and the shipped `workers: 1`, tier ordering buys position in a queue, never preemption. And ordering is strict with no ageing: on a saturated scanner, every owner-tier job admitted during a scan is dequeued ahead of a deckhand job regardless of how long that job has waited, so the low tier starves for as long as the high-tier stream lasts. Whether that is a bug or the intended product behaviour is a policy question, but nothing in the code bounds it. **Suggested fix (not applied), if starvation is not intended.** Age the priority: fold enqueue time into `Less` so a job's effective priority improves as it waits. --- #### 14. Shutdown does not interrupt a scan, and `queue.Close()` drains rather than cancels **Severity: MEDIUM — CONFIRMED** `scanner/internal/client/hold.go:23` (context-free `httpClient`), `:266-288`, `scanner/internal/scan/worker.go:88-142`, `scanner/internal/queue/priority_queue.go:117-127` **What happens.** `processJob` takes a ctx and passes it to Syft and Grype, but the blob fetches go through `client.GetBlobPresignedURL` / `DownloadBlob`, which use a package-level `http.Client` and `http.NewRequest`/`Get` with **no request context at all**. Cancelling the pool's context mid-download changes nothing: the worker stays inside the fetch until the client's own five-minute timeout, and `WorkerPool.Wait()` — which `cmd/scanner` calls on SIGTERM at `main.go:114` — blocks for exactly as long. Under a typical 30-second termination grace period that is a SIGKILL, and the SIGKILL means `buildOCILayout`'s `defer cleanup()` never runs, so the `scan-*` directory is left behind on the scanner volume. (For a job that *does* finish, cleanup is correct: `cleanup()` is deferred immediately after `MkdirTemp` and every early return calls it.) `queue.Close()` compounds it. `Dequeue` returns nil only when the queue is closed **and** empty (`:123`), so closing a queue with a backlog hands every remaining job to a worker rather than dropping it. The worker's only ctx check is in the cooldown select at the *bottom* of the loop, so after cancellation each worker still runs one more full job to completion. `HoldClient.Close()` has already severed the socket by then, so that job's result is written into a dead connection and dropped with only a log line — the hold sees nothing and sits on the row until the ten-minute processing timeout. **Reproduce.** `TestShutdownDoesNotInterruptInFlightDownload` (runs the exact SIGTERM sequence from `cmd/scanner`, then proves `pool.Wait()` is still blocked 500ms later, and that it only returns once the download itself completes) and `TestQueueCloseDrainsRatherThanCancels`. **Suggested fix (not applied).** Thread ctx through `GetBlobPresignedURL` and `DownloadBlob` (`http.NewRequestWithContext`), check ctx at the top of the worker loop as well as the bottom, and give `JobQueue` a `Drain`/`Cancel` distinction so shutdown can discard a backlog it can no longer answer for. --- #### 15. `HoldClient.Close()` is not idempotent — a second call panics the process **Severity: LOW — CONFIRMED** `scanner/internal/client/hold.go:219-226` — `close(c.done)` with no guard. `cmd/scanner` calls it exactly once, which is the only reason this is not already an incident; I hit it immediately when a test's cleanup repeated the shutdown sequence. Any future reconnect supervisor or restart path that calls `Close` twice takes the process down with "close of closed channel". **Reproduce.** `TestHoldClientCloseIsNotIdempotent`. **Suggested fix (not applied).** `sync.Once`, or a `closed` flag under the existing mutex. --- #### Non-findings (checked, behaving correctly) - **Unknown message type from the hold** is logged and skipped, the connection survives, and nothing is sent back (`client/hold.go:126-129`). Correct today, since the hold only ever sends `"job"`; the note is that an older scanner will silently swallow any message type added later rather than refusing it. `TestUnknownMessageTypeIsIgnored`. - **Result for a seq the hold never dispatched** is handled: `handleResult` returns on `sql.ErrNoRows` (`scan_broadcaster.go:561-571`). `handleError`/`handleSkipped` run their UPDATE anyway, but it matches zero rows. See finding 6 for the part that is not safe. - **A skipped verdict is never retried** (`scan_broadcaster.go:1216-1221`), which is what makes "skipped" the right answer for every permanent condition in findings 1 and 3. ## 11. Performance and resources (execution) ##### ATCR scanner: performance and resource findings All benchmarks and load scenarios live in one file: `/home/data/atcr.io/scanner/internal/e2e/bench_test.go`. Nothing else was modified. No bug found here was fixed. --- #### 1. What performance testing is worth doing on this scanner, and how to run it The scanner is a single-image-at-a-time pipeline with four stages (blob download, OCI layout assembly, stereoscope load/extract, Syft catalog + SPDX encode), a 100-deep priority queue in front of it, and a process-wide soft memory limit around it. The things worth measuring are the ones where a production incident would come from a resource, not from a wrong answer: | Question | Scenario | Cost | |---|---|---| | Which stage dominates? | `TestPerfStageBreakdown`, `BenchmarkPipeline` | ~60 s | | Does memory ratchet across jobs? Does the 10 s cooldown earn its cost? | `TestPerfSustainedLoad` (run twice, `prod` and `none`) | ~3 min each | | What does the 512 MiB `SetMemoryLimit` cost? | `TestPerfMemoryLimit` | ~2.5 min | | How do time and memory scale with layers, bytes, file count? | `TestPerfSyntheticScaling` | ~20 s | | How much disk does a scan really use? | `TestPerfDiskAmplification`, `TestPerfTempCleanup` | ~10 s | | Does raising `scanner.workers` help, and what does it cost in RSS? | `TestPerfConcurrency` | ~100 s | | What does the vulnDB lock cost when a reload lands? | `TestPerfVulnDBLockModel` | ~4 s | | What is the last job in a burst waiting for? | `TestPerfQueueBurst`, `TestPerfWorkerCadence` | ~45 s | | Do goroutines or fds leak across jobs and reconnects? | `TestPerfJobChurn`, `TestPerfConnectionChurn` | ~25 s | | What does the *real* worker cost end to end? | `TestPerfRealWorkerScan` (subprocess) | ~16 s | Everything is off by default: each `TestPerf*` skips unless `ATCR_SCANNER_PERF=1`, and the benchmarks need `-bench`. Every image-dependent scenario skips cleanly when its fixture is absent, so a clean checkout still runs the ordinary suite in under a second. ##### Commands ```bash cd /home/data/atcr.io/scanner ##### fixtures (gitignored, a few hundred MB; skip and the scenarios skip too) cd internal/mockhold/testdata/blobs skopeo copy docker://docker.io/library/alpine:3.20 oci:perf-alpine:img skopeo copy docker://docker.io/library/python:3.12-slim oci:perf-python:img skopeo copy docker://docker.io/library/node:22 oci:perf-node:img cd - ./internal/mockhold/testdata/fetch-blobs.sh # hsm-secrets-operator, needs buoy.cr creds ##### everything except the two that must run one per process ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run 'TestPerf' -v -timeout 40m ##### the cooldown comparison: one process per mode, or the second mode ##### inherits the first one's heap ATCR_SCANNER_PERF=1 ATCR_SCANNER_PERF_COOLDOWN=prod \ go test ./internal/e2e/ -run TestPerfSustainedLoad -v -timeout 15m ATCR_SCANNER_PERF=1 ATCR_SCANNER_PERF_COOLDOWN=none \ go test ./internal/e2e/ -run TestPerfSustainedLoad -v -timeout 15m ##### stable per-op numbers ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run XXX -bench BenchmarkPipeline -benchtime 3x ``` ##### What could not be measured, and why - **The real `WorkerPool` on a successful scan.** `worker.go:129` dereferences `result.Summary.Total`, which is nil whenever `vuln.enabled` is false, so the first successful scan segfaults the process (known BLOCKER, not fixed here). Every throughput, sustained-memory and concurrency number below therefore comes from `pipelineOnce` in the bench file: a replica that runs the same four stages against the same libraries and the same mock hold, because `buildOCILayout` and `generateSBOM` are unexported and this file may not add a seam to them. `TestPerfRealWorkerScan` runs the real pool once in a child process and prints production's own `Scan pipeline completed duration=` line next to the replica's, so the two can be compared; they agree. - **Anything involving a real Grype database.** Never downloaded. The vulnDB lock is therefore characterised by a structural model (`TestPerfVulnDBLockModel`) plus code reading, not by running Grype. Every vulnerability-scanning number in production will be *additional* to what is reported here: none of these measurements include `FindMatches`. - **Blob download at production latency.** The mock hold serves over loopback, so download time here (~200 ms for 390 MiB, ≈2 GB/s) is a floor, not a forecast. Against S3 it is bandwidth-bound; see finding P-7. ##### Hardware and noise AMD Ryzen 9 7900X3D (12 cores / 24 threads), 61 GiB RAM, NVMe SSD, Linux 7.1.10, Go 1.26.7, load average ~1-3 during runs (shared developer machine, other agents active in the same repo). Timings below are from three passes per fixture unless stated; the pass-to-pass spread on the real images was under 3%, which is the honest error bar. Single numbers from a single pass are labelled indicative. --- #### 2. Measurements ##### 2.1 Per-stage cost (three passes per image, `TestPerfStageBreakdown`) | Fixture | Layers | Compressed | Total | Download | Load/extract | Catalog | Encode | Packages | SBOM | Peak tmp | Peak RSS | |---|---|---|---|---|---|---|---|---|---|---|---| | hsm-secrets-operator | 1 | 2.1 MiB | 0.68-0.73 s | 2 ms | 79-83 ms | 0.60-0.64 s | <1 ms | 1 | 3 KiB | 6.6 MiB | ~200 MiB | | alpine:3.20 | 1 | 3.5 MiB | 0.59-0.62 s | 3 ms | 112-119 ms | 0.48-0.50 s | 1 ms | 14 | 80 KiB | 11.2 MiB | ~200 MiB | | python:3.12-slim | 4 | 44.0 MiB | 2.25-2.36 s | 24 ms | 1.63-1.70 s | 0.56-0.60 s | 34 ms | 95 | 1.9 MiB | 169.5 MiB | 232-270 MiB | | node:22 | 8 | 389.5 MiB | 15.87-16.33 s | ~200 ms | 12.84-13.31 s | 2.50-2.70 s | ~250 ms | 613 | 14.0 MiB | 1492.6 MiB | 717-813 MiB | Share of a node:22 scan: **load/extract 81%**, catalog 16%, encode 1.5%, download 1.2% (loopback). For the two tiny images the fixed cost of Syft's catalog set (~0.5 s) is the whole scan. ##### 2.2 Scaling (`TestPerfSyntheticScaling`, one pass each, indicative) Layer count at a fixed 64 MiB of incompressible content: | Layers | Total | Load | Catalog | ΔHeap | |---|---|---|---|---| | 1 | 513 ms | 98 ms | 382 ms | 122 MiB | | 4 | 559 ms | 104 ms | 420 ms | 191 MiB | | 16 | 588 ms | 97 ms | 459 ms | 318 MiB | | 32 | 598 ms | 106 ms | 459 ms | 345 MiB | Time is essentially flat in layer count; heap is not (+~7 MiB per layer at constant bytes). The corpus's 19-layer images are therefore not a time risk. Total bytes at a fixed 4 layers: | Bytes | Total | Download | Load | Catalog | Peak tmp | |---|---|---|---|---|---| | 8 MiB | 430 ms | 6 ms | 13 ms | 411 ms | 16 MiB | | 32 MiB | 497 ms | 17 ms | 50 ms | 430 ms | 64 MiB | | 128 MiB | 689 ms | 63 ms | 199 ms | 427 ms | 256 MiB | | 512 MiB | 1.42 s | 225 ms | 755 ms | 435 ms | 1024 MiB | Load scales linearly with bytes (~1.5 ms/MiB); catalog is flat when there is nothing to catalog. Peak temp disk is consistently **2x the compressed bytes** for incompressible content, because the compressed layout and the extracted tree coexist. File count at a fixed 16 MiB across 4 layers: | Files/layer | Total files | Load | Total | |---|---|---|---| | 16 | 64 | 101 ms | 531 ms | | 256 | 1 024 | 142 ms | 652 ms | | 4 096 | 16 384 | 1 556 ms | 2.01 s | Entry count, not byte count, is what the load stage walks: 16k small files cost 15x the load time of 64 large ones at identical bytes. ##### 2.3 Concurrency (`TestPerfConcurrency`, node:22, 2 scans per worker) | Workers | Wall | Per scan | Throughput | Peak RSS | ΔRSS | |---|---|---|---|---|---| | 1 | 31.8 s | 15.9 s | 3.77 scans/min | 836 MiB | 640 MiB | | 2 | 32.7 s | 8.2 s | 7.34 scans/min | 1414 MiB | 1168 MiB | | 4 | 35.0 s | 4.4 s | 13.71 scans/min | 3040 MiB | 2464 MiB | Throughput scales almost linearly (0.97x and 0.91x of ideal) on a 24-thread box, and so does memory: **each additional concurrent scan of a 390 MiB image costs roughly another 600-800 MiB of RSS**. On a production VM with fewer cores the throughput half of that trade will not hold; the memory half will. ##### 2.4 Disk (`TestPerfDiskAmplification`, `TestPerfTempCleanup`, and the tmp column above) | Image | Compressed (what `MaxImageSize` checks) | Peak bytes under the scan tmp dir | Ratio | |---|---|---|---| | hsm-secrets-operator | 2.1 MiB | 6.6 MiB | 3.1x | | alpine:3.20 | 3.5 MiB | 11.2 MiB | 3.2x | | python:3.12-slim | 44.0 MiB | 169.5 MiB | 3.9x | | node:22 | 389.5 MiB | 1492.6 MiB | **3.8x** | | synthetic, 64 MiB of zeros | 0.08 MiB | 64.1 MiB | **816x** | | synthetic, 512 MiB of zeros | 0.62 MiB | 512.6 MiB | **823x** | Cleanup is reliable: after each of five consecutive scans the tmp dir was empty (0 entries, 0 bytes), and the zero-filled runs left 0 bytes behind. ##### 2.5 Sustained load and the inter-job cooldown (`TestPerfSustainedLoad`, node:22, 6 jobs, one process per mode) | Mode | Wall | Time spent scanning | Throughput | Peak RSS | Peak heap | RSS between jobs | VmHWM | |---|---|---|---|---|---|---|---| | `prod` (`runtime.GC()` + 10 s sleep) | 2 m 35.1 s | 1 m 35.1 s | 2.32 jobs/min | 800 MiB | 602 MiB | 210-284 MiB | 820 MiB | | `none` (nothing between jobs) | 1 m 35.8 s | 1 m 35.8 s | 3.76 jobs/min | 837 MiB | 606 MiB | 455-608 MiB | 833 MiB | Per-job scan time was identical in both modes (15.57-16.14 s, every job, both runs), and neither mode ratcheted: in `none`, RSS after six consecutive scans was 596 MiB against 608 MiB after the first, and heap after the last job (277 MiB) was below the first (316 MiB). Goroutines stayed at 7 and file descriptors at 11-12 throughout both runs. So the pause costs 39% of wall-clock throughput and moves peak memory by 4.6%, which is inside the run-to-run spread. What it does buy is idle footprint: RSS between jobs is 210 MiB with it and 596 MiB without, because Go's scavenger needs idle time to hand freed pages back to the OS. That matters if something else shares the box; it does nothing for the peak, which is what an OOM kill is decided on. ##### 2.6 Corpus shape (computed from `internal/mockhold/testdata/corpus.json`, not timed) 57 image manifests, 8.18 GiB of layer bytes in total, 3.80 GiB of them unique: a **2.15x** duplication factor across the corpus, and **5.9x** within `agent-gateway` alone (14 manifests, 4.16 GiB declared, 0.71 GiB unique). Every manifest is scanned independently, so shared layers are downloaded and extracted once per manifest. ##### 2.7 Queue, worker cadence, and allocation (`TestPerfQueueBurst`, `TestPerfWorkerCadence`, `BenchmarkPipeline`) - 110 jobs dispatched over the WebSocket in **1 ms**; the queue reached exactly its configured 100 and the remaining 10 came back as "scanner queue full". - Draining those 100 no-op jobs took **1.09 s** at a 10 ms cooldown: 11 ms per job, so the worker's own per-job overhead outside the cooldown is **1 ms**. - At the production cooldown the real worker's gaps were exactly **10.002 s** per job across four jobs, whatever the job did. - `BenchmarkPipeline/perf-alpine`: 594 ms/op, **385 MB allocated and 6.5 M allocations per op** to catalog a 3.5 MiB single-layer image. Allocation volume, not resident size, is what the GC has to keep up with; this is the number to watch if P-9's memory limit is ever tightened. --- #### 3. Findings ##### P-1. BLOCKER (known, not re-derived) — a successful scan panics the process `scanner/internal/scan/worker.go:129`. Recorded here only for its effect on measurement: it is why nothing below drives more than one successful scan through the real `WorkerPool`, and why the throughput, sustained-memory and concurrency numbers come from the `pipelineOnce` replica instead. Reproduced twice. Early in this session `go test ./internal/e2e/ -run TestScanRealImage` ended in `SIGSEGV ... worker.go:129` immediately after production logged `Scan pipeline completed duration=677.569153ms`; that test has since been changed by another agent to skip with "blocked on the nil result.Summary dereference at scan/worker.go:129", so it no longer demonstrates the crash. `TestPerfRealWorkerScan` in the bench file still does, in a child process that is allowed to die: ``` child fixture=perf-node wall=15.742s exit=exit status 2 production log line: INFO Scan pipeline completed repository=perf-node duration=15.593324819s child died as expected on the known BLOCKER: panic: runtime error: invalid memory address or nil pointer dereference ``` That child run is also the only end-to-end timing of the real `WorkerPool` on a real image in this report, and it agrees with the replica to within 3%. ##### P-2. MEDIUM / CONFIRMED — the 10 s inter-job cooldown costs 39% of throughput and does not lower peak memory `scanner/internal/scan/worker.go:135-143` (`result = nil; runtime.GC()`, then `<-time.After(JobCooldown)`, default 10 s at `worker.go:154`). **What happens.** The comment says the pause exists "to reduce sustained memory pressure", because "Syft/Grype allocate heavily and Go's GC needs idle time to catch up under sustained load". Measured over six consecutive node:22 scans in separate processes, with and without the pause: - peak RSS 800 MiB (with) vs 837 MiB (without) — a 4.6% difference, inside the run-to-run spread; - peak heap 602 MiB vs 606 MiB; - no ratchet in either mode: without the pause, RSS after six scans (596 MiB) was *below* RSS after the first (608 MiB), and heap after the last job (277 MiB) below the first (316 MiB); - per-scan time identical (15.57-16.14 s in both); - throughput 2.32 vs 3.76 jobs/min. What the pause does change is idle RSS between jobs: 210-284 MiB with it, 455-608 MiB without, because Go's scavenger only returns freed pages to the OS after the heap has been idle. That is a real effect, and it is the only one measured. It does not protect the peak, and the peak is what an OOM kill is decided on. `TestPerfWorkerCadence` measures the cost on the real worker directly: four no-op jobs (they fail before opening a socket) took 30.006 s, with inter-job gaps of exactly 10.002 s. A single worker therefore cannot exceed **6 jobs/min however cheap the jobs are**, and the cooldown is 39% of the wall time of a node:22 workload. **Reproduce.** ```bash ATCR_SCANNER_PERF=1 ATCR_SCANNER_PERF_COOLDOWN=prod go test ./internal/e2e/ -run TestPerfSustainedLoad -v -timeout 15m ATCR_SCANNER_PERF=1 ATCR_SCANNER_PERF_COOLDOWN=none go test ./internal/e2e/ -run TestPerfSustainedLoad -v -timeout 15m ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run TestPerfWorkerCadence -v ``` **Suggested fix (not applied).** Make the cooldown a config field defaulting to something far smaller (1 s or 0), and keep `runtime.GC()` plus a `debug.FreeOSMemory()` call, which returns the pages immediately instead of waiting for the scavenger. If the pause is retained for the idle-RSS effect, say so in the comment, because the current comment claims a peak-memory benefit that is not there. Caveat: this was measured with `vuln.enabled=false`, so Grype's own allocation is not in the picture; the same comparison should be re-run once P-1 is fixed and a database is available in a soak environment. ##### P-3. HIGH / CONFIRMED — a full queue outlives the hold's processing timeout, so the hold fails jobs the scanner is still working on `scanner/internal/client/hold.go:159` (ack sent on receipt, before the job is queued), `scanner/internal/scan/worker.go:154` (10 s cooldown), `scanner/internal/config/config.go` (`queue_size` default 100, `workers` default 1), against `pkg/hold/pds/scan_broadcaster.go:523-528` and `pkg/hold/pds/scan_broadcaster.go:833-838`. **What happens.** The scanner acks a job the moment it arrives on the socket, which moves the hold's row from `assigned` to `processing` *without* touching `assigned_at`. `reDispatchTimedOut` then marks any `processing` row whose `assigned_at` is older than ten minutes as `failed`. Nothing in the scanner bounds how long a job sits in the queue before it starts. Measured with the worker parked and the queue filled exactly to its configured depth (`TestPerfQueueBurst`): 110 jobs dispatched in 1 ms, the queue reached 100 of 100, the last 10 were rejected with "scanner queue full", and draining the 100 accepted no-op jobs took 1.09 s at a 10 ms cooldown — 11 ms per job, of which 1 ms is the worker's own overhead and the rest is the cooldown. Substituting the production 10 s cooldown into that measured per-job overhead: a full 100-deep queue of jobs that do *no work at all* drains in **16 m 40 s**, and queue position **59** is already past the ten minute deadline. With node:22-sized scans (measured 15.6-16.1 s each) the drain is **43 m 20 s** and the deadline is crossed at queue position **23**. So on any hold with a burst of more than ~20 scannable manifests — the corpus has 57 image manifests on one hold — the tail of the queue is marked failed while the scanner is still holding it, and the scanner later sends results for seqs the hold has already written off. **Reproduce.** `ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run TestPerfQueueBurst -v` **Suggested fix (not applied).** Three independent options, in increasing order of intrusiveness: (a) have the hold re-stamp `assigned_at` on ack, or add a separate `started_at` that the scanner sets when the job leaves the queue, so the ten minute budget covers scanning rather than queueing; (b) have the scanner refuse jobs it cannot start within the deadline — a queue admission check against `queue.Len() * expected per-job time` — so the hold can hold them pending instead; (c) shrink `queue_size` to something the worker count can actually drain inside ten minutes (with a 10 s cooldown and one worker, that is under 60 no-op jobs and under 23 real ones). (a) is the honest fix; (c) alone just moves the loss earlier. ##### P-4. MEDIUM / CONFIRMED — `MaxImageSize` bounds compressed bytes, and disk usage is 3.8x that for real images and 800x+ for compressible ones `scanner/internal/scan/worker.go:212-220` (the check), `scanner/internal/config/config.go:59` (2 GiB default). **What happens.** The guard sums the *declared compressed* sizes of the config and layers. What lands on disk is the compressed layout plus the extracted tree, and stereoscope extracts under `TMPDIR`, which `WorkerPool.Start` points at the same `vuln.tmp_dir`. Measured peak bytes under the scan tmp dir: | Image | Compressed | Peak on disk | Ratio | |---|---|---|---| | alpine:3.20 | 3.5 MiB | 11.2 MiB | 3.2x | | python:3.12-slim | 44.0 MiB | 169.5 MiB | 3.9x | | node:22 | 389.5 MiB | 1492.6 MiB | 3.8x | | 512 MiB of zeros in one layer | 0.62 MiB | 512.6 MiB | 823x | At the 2 GiB default, an ordinary image that passes the check can therefore need roughly 7.6 GiB of temp space, and a deliberately compressible one is bounded only by the compression ratio: 2 GiB of compressed zeros would extract to well over a terabyte. The guard also uses the sizes the *manifest declares*, not the bytes actually received, and `downloadBlob` (`scanner/internal/scan/extractor.go:172`) streams to disk with no running total, so a manifest that understates its layer sizes is not caught at all. **Reproduce.** `ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run 'TestPerfDiskAmplification|TestPerfStageBreakdown' -v` **Suggested fix (not applied).** Enforce a byte budget during the download itself (count bytes written and abort past the limit), and add a separate extracted-size ceiling, either by checking free space on `vuln.tmp_dir` before extraction or by capping total extracted bytes. Document that `max_image_size` is a *compressed* bound and that the volume needs roughly 4x it for ordinary images. ##### P-5. HIGH / CONFIRMED (by inspection; a test is proposed, not written) — the "every 50 scans" vulnDB reload is unreachable `scanner/internal/scan/grype.go:174-205`. **What happens.** `loadVulnDatabase` opens with a read-locked fast path: ```go if vulnDB != nil && (time.Since(vulnDBBuilt) < vulnDBRefreshAge || time.Since(vulnDBAttempt) < vulnDBRetryBackoff) { return vulnDB, nil // grype.go:181 } ``` The scan counter that drives the periodic reload lives *past* that return, in a block guarded by the same freshness test under the write lock: ```go if vulnDB != nil && time.Since(vulnDBBuilt) < vulnDBRefreshAge { // grype.go:193 n := vulnDBScans.Add(1) // grype.go:196 if n%50 == 0 { ... reload ... } ``` Reaching line 196 requires the freshness test to be *false* at line 181 and *true* at line 193 — that is, another goroutine must have completed a reload in the window between the `RUnlock` and the `Lock`. With the default `scanner.workers: 1` there is no other goroutine at all, and with the production template's `workers: 2` it takes 50 such races to fire the reload once. In ordinary operation `vulnDBScans` never increments, so `vulnDB.Close()`/reopen never happens. The comment says that reload exists to "flush SQLite's page cache and mmap region", i.e. it is a memory workaround. If the growth it was written for is real, the scanner still has it and always has: the workaround has never run. **Reproduce.** Not reachable from `internal/e2e` — `loadVulnDB` is unexported in package `scan`. The test belongs beside the existing stubs in `scanner/internal/scan/grype_test.go`, and would read: reset state, stub the loader with a DB built one hour ago, call `loadVulnDatabase` 200 times, then assert `vulnDBScans.Load() > 0`. It fails today, returning 0. **Suggested fix (not applied).** Increment the counter on the fast path (an `atomic.Int64`, so it costs nothing under the read lock) and take the write lock only when `n%50 == 0`. While doing so, decide whether the flush is still wanted at all: it has never executed, so any belief about its benefit is untested. If it is kept, the reload should also be measured, since it happens under the write lock (see P-6). ##### P-6. MEDIUM / CONFIRMED structurally, SUSPECTED in magnitude — a vulnDB reload stalls every worker for the whole download `scanner/internal/scan/grype.go:105-106` (read lock held across `FindMatches`) and `grype.go:187-259` (write lock held across `loadVulnDB`, which downloads, decompresses and opens the database). **What happens.** `sync.RWMutex` blocks new readers as soon as a writer is queued, so once one worker starts a database refresh, every other worker's next scan blocks until the download finishes. The model in `TestPerfVulnDBLockModel` reproduces the structure with 4 workers, 50 ms "scans" and a 2 s "reload": the writer acquired the lock in 2 ms and the worst reader stall was 2.001 s — **100% of the reload duration**. In production the reload is a Grype v6 database fetch, which `WorkerPool.Start` describes as "1 GB+" after decompression, so the stall is however long that takes, not 2 s. This is rare rather than frequent: with P-5 unfixed, the write lock is only reached at cold start, at the weekly `vulnDBRefreshAge` boundary, or after the 30 minute retry backoff on a stale database. It is listed because the blast radius is every worker, and because fixing P-5 would make it *routine*. **Reproduce.** `ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run TestPerfVulnDBLockModel -v` (a model of the lock discipline, not the real loader — the real path cannot be driven without downloading the database). **Suggested fix (not applied).** Download and open the new provider *outside* the lock, and take the write lock only to swap the pointer and close the old one. That turns a minutes-long exclusive section into a pointer assignment. The existing "serve the old provider while the new one loads" logic already assumes the old provider stays usable, so the swap is the only part that needs exclusion. ##### P-7. MEDIUM / CONFIRMED — 81% of a large scan is layer load/extract, and every manifest pays it again for layers it shares with its neighbours `scanner/internal/scan/syft.go:22-35` (stereoscope provide + `img.Read()`), `scanner/internal/scan/extractor.go:49` (`buildOCILayout` downloads per job). **What happens.** For node:22 the pipeline splits 12.84-13.31 s load, 2.50-2.70 s catalog, ~250 ms encode, ~200 ms download (loopback) out of 15.87-16.33 s total: **81% of the scan is stereoscope reading and extracting layers**. The synthetic sweeps isolate what drives it: load time is linear in bytes (~1.5 ms/MiB) and, at constant bytes, 15x worse for 16 384 small files than for 64 large ones (1.56 s vs 101 ms). Layer *count* alone barely matters (513 ms at 1 layer vs 598 ms at 32, same bytes). Nothing is cached between jobs. Layers are content-addressed, and the corpus shows how much that costs: 57 image manifests declare 8.18 GiB of layers of which 3.80 GiB are distinct (2.15x), and within `agent-gateway` alone 14 manifests declare 4.16 GiB of which 0.71 GiB are distinct (**5.9x**). Rescanning that repository's history downloads and extracts the same base layers fourteen times. **Reproduce.** `ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run 'TestPerfStageBreakdown|TestPerfSyntheticScaling' -v` **Suggested fix (not applied).** A content-addressed blob cache under `vuln.tmp_dir` keyed by layer digest, with an LRU bound, would remove the download and (if the extracted tree is cached too) most of the load stage for repeat scans of a repository's history. That is a design change, not a patch, and it interacts with P-4: a cache needs its own disk budget. ##### P-8. MEDIUM / CONFIRMED — the download path takes no context and no deadline, so a scan cannot be cancelled and has no upper bound `scanner/internal/scan/extractor.go:49` (`buildOCILayout(job, tmpDir, secret)` — no `context.Context`), `scanner/internal/scan/worker.go:230` (the `ctx` `processJob` receives is passed to Syft and Grype but never to the download), `scanner/internal/client/hold.go:23` (`http.Client{Timeout: 5 * time.Minute}`). **What happens.** The only bound on the download stage is a five minute per-request timeout, applied independently to every presign call and every blob body. A 19-layer image therefore has a worst case of 19 × 2 × 5 minutes before the pipeline gives up, and shutting the scanner down (`ctx` cancellation) does not interrupt a download in flight. There is also no retry: one failed blob fails the whole job, and the hold re-dispatches the entire image. This was not measured against a slow server — the mock serves over loopback — so the number above is read off the code, not observed. What was observed is that a single node:22 blob transfer is ~200 ms locally, so the five minute budget is only in play against a slow or stalled S3. **Suggested fix (not applied).** Thread the worker's `ctx` through `buildOCILayout` and `downloadBlob` and give each job an overall deadline derived from the hold's ten minute budget (see P-3), so a job that cannot finish in time fails fast rather than finishing work nobody will accept. A bounded retry on a blob download would also stop one transient 500 from costing a full re-download of the image. ##### P-9. MEDIUM / CONFIRMED — the 512 MiB soft memory limit is free at one worker and costs 24-92% once workers scan concurrently `scanner/cmd/scanner/main.go:46` (`debug.SetMemoryLimit(512 * 1024 * 1024)`), against `deploy/upcloud/configs/scanner.yaml.tmpl` which sets `scanner.workers: 2`. **What happens.** `GOMEMLIMIT` is process-wide, so it is divided among concurrent scans rather than multiplied by them. Measured on node:22 with the limit set to the production value and again with it off (`TestPerfMemoryLimit`): | Concurrent scans | Limit | Wall | Per scan | Peak RSS | GC cycles | GC CPU | |---|---|---|---|---|---|---| | 1 | off | 15.53-16.23 s | — | 748-806 MiB | 37-41 | 0.1-0.4% | | 1 | 512 MiB | 15.68-15.79 s | — | 561-562 MiB | 42-43 | 0.2-0.4% | | 2 | off | 16.33 s | 8.17 s | 1357 MiB | 44 | 0.5% | | 2 | 512 MiB | 20.31 s | 10.15 s | 687 MiB | 230 | 1.8% | | 4 | off | 17.56 s | 4.39 s | 2951 MiB | 53 | 1.8% | | 4 | 512 MiB | 33.77 s | 8.44 s | 1224 MiB | 497 | 5.0% | At one worker the limit is doing its job for free: it holds peak RSS to 561 MiB instead of 748 MiB and costs 1.7% of wall time, because most of the peak is collectible garbage rather than live data. My prior expectation that a 512 MiB limit under a ~545 MiB peak heap would provoke a GC death spiral was **wrong at one worker, and right at two**: with the production `workers: 2` setting, two concurrent node:22 scans run 24% slower and do 5.2x the GC cycles; at four they run 92% slower and do 9.4x the cycles. The limit is buying real RSS containment in exchange for that, so this is a tuning finding rather than a defect: what it should not be is invisible. **Reproduce.** `ATCR_SCANNER_PERF=1 go test ./internal/e2e/ -run TestPerfMemoryLimit -v -timeout 20m` **Suggested fix (not applied).** Make the limit configurable and scale it with `scanner.workers` (e.g. `512 MiB × workers`, still overridable by `GOMEMLIMIT`), or document that raising `workers` without raising `GOMEMLIMIT` trades throughput for RSS rather than gaining throughput. The 512 MiB constant is also undersized for a single node:22-class image: it is below the 561 MiB RSS the scan reaches even *with* the limit in force. ##### P-10. LOW / CONFIRMED — the result message carries the whole SBOM inline `scanner/internal/client/hold.go:177-190` (`SendResult` puts the SBOM in the WebSocket message), `pkg/hold/pds/scan_broadcaster.go:572-581` (the hold then uploads it to S3). node:22 produced a 14.0 MiB SPDX JSON document (613 packages); python:3.12-slim 1.9 MiB; alpine 80 KiB. That whole document crosses the WebSocket as one text frame and is buffered in memory on both ends. Neither side sets a read limit, so nothing breaks today, but SBOM size grows with package count and the hold holds it entirely in RAM before the S3 upload. Worth a cap, or a separate upload path, before an image with several thousand packages arrives. ##### P-11. Negative results worth recording - **No goroutine or descriptor leak.** Across 200 no-op jobs: goroutines 6→6, fds 11→11, heap 6.2→6.2 MiB. Across 4 reconnect cycles (alternating abrupt TCP close and clean close frame): goroutines 6→6, fds 11→11. Across six node:22 scans: goroutines steady at 7, fds 11-12. - **Temp directories are cleaned up reliably.** After each of five consecutive scans the tmp dir held 0 entries and 0 bytes, including the runs that extracted 512 MiB. - **The replica pipeline matches production.** The real `WorkerPool` logged `Scan pipeline completed duration=15.593324819s` for node:22 in the child process, against 15.53-16.33 s for the replica — the worker adds no measurable overhead beyond the cooldown. - **Concurrency scales, memory scales with it.** 1/2/4 concurrent scans give 3.77 / 7.34 / 13.71 scans/min (0.97x and 0.91x of linear on a 24-thread box) at 836 / 1414 / 3040 MiB peak RSS. --- #### 4. Priority, and what a soak test should cover next In the order I would act on them: 1. **P-1** (existing BLOCKER) — nothing else about the worker can be measured or operated until it is fixed. 2. **P-3** — the queue silently exceeds the hold's deadline at a burst size the corpus already exhibits, and the failure is invisible from the scanner side. 3. **P-5** — a memory workaround that has never executed is worse than no workaround, because it makes the problem look handled. 4. **P-2** and **P-9** together — they are the two throughput knobs, and each is currently set by a constant with no measurement behind it. Changing either without the other is a guess: dropping the cooldown raises sustained heap pressure, which the 512 MiB limit then converts into GC time. 5. **P-4**, **P-7**, **P-8**, **P-6**, **P-10** as capacity work. ##### What is still unmeasured - **Grype.** Every number here has `vuln.enabled=false`. `FindMatches` cost, the vulnerability database's resident footprint, and whether the mmap growth P-5's dead workaround was written for is real — none of that can be measured without a database, and it should be measured in a soak environment once P-1 is fixed. My expectation, stated as an expectation and not a measurement, is that Grype adds materially to both peak heap and scan time, which would make P-9's limit tighter than these numbers suggest. - **Real S3 latency.** Download here is loopback (~200 ms for 390 MiB). The same transfer at 100 MB/s takes ~4 s and would move the download stage from 1% of a scan to roughly a fifth of it, which changes where optimisation pays. - **A long soak.** The longest run here is six consecutive scans. A ratchet with a period longer than that would not show up. The evidence against a ratchet (heap and RSS both lower at job 6 than job 1, in both cooldown modes) is good but not a substitute for a multi-hour run. - **Multi-worker behaviour through the real `WorkerPool`.** All concurrency numbers come from the replica; the pool's own contention (the queue mutex, the shared WebSocket write mutex in `sendJSON`) was not exercised under load because P-1 prevents it. ## 12. Scanner module code review (reading) ##### ATCR Scanner — deep reading review Scope: every non-test file under `scanner/`, the scanner's tests, `Dockerfile.scanner`, `deploy/upcloud/configs/scanner.yaml.tmpl`, `deploy/upcloud/systemd/scanner.service.tmpl`, and the hold-side counterpart (`pkg/hold/pds/scan_broadcaster.go`, `pkg/hold/oci/xrpc.go`) where the protocol contract lives. Method: reading only. No production code was modified and no tests were written. Two things were executed and are marked CONFIRMED: `go vet ./...` + `go test -race ./...` inside `scanner/`, and a throwaway scratch program (outside the repo) to check `os.MkdirAll("")` and `filepath.Join` semantics. `go vet ./...` is clean. `go test -race ./...` currently **FAILS**: `internal/e2e` panics at `worker.go:129`, exactly finding 1. `internal/scan` and `internal/queue` pass under `-race`; no race was reported, but the e2e binary died before it could exercise much. --- #### Section 1 — assessment of the existing sweep ##### 1. `worker.go:129` nil deref of `result.Summary.Total` when `vuln.enabled=false` **AGREE — and understated.** CONFIRMED by running `go test -race ./...` in `scanner/`; the existing `TestScanRealImage` panics at exactly this line and takes the test binary down. `processJob` sets `result.Summary` only inside `if wp.cfg.Vuln.Enabled` (`worker.go:245-254`). The success branch at `worker.go:124-129` dereferences it unconditionally. Three things make the blast radius larger than "every successful scan panics": - **It is a whole-process kill, not a worker kill.** There is no `recover()` anywhere in the scanner. With `scanner.workers: 2` (the production value in `deploy/upcloud/configs/scanner.yaml.tmpl:14`) the panicking worker also destroys the other worker's in-flight scan and the WebSocket read loop. - **It is a permanent crash loop, not a one-off.** `systemd/scanner.service.tmpl` sets `Restart=on-failure` / `RestartSec=10`. Ten seconds between restarts never trips systemd's default start-limit burst (5 in 10s), so the unit restarts forever. Meanwhile the job it crashed on sits in `processing`; `reDispatchTimedOut` flips it to `failed` after 10 minutes (`scan_broadcaster.go:833-837`), a failed record is retryable by the stale loop (`scan_broadcaster.go:1204-1210`), and the next pass re-serves it. Steady-state: crash, restart, crash. - **`SendResult` is called *before* the panic** (`worker.go:124`, panic at 129). So the hold has already received and stored a result. Which leads to the trap below. **The obvious fix is itself a bug.** Guarding the log line and sending a summary-less result makes the *hold* panic: `handleResult` guards `if msg.Summary != nil` for the record write (`scan_broadcaster.go:599`) but the closing log at `scan_broadcaster.go:642-644` dereferences `msg.Summary.Critical/.High/.Total` outside that guard. A summary-less result therefore kills the hold process from inside a WebSocket reader goroutine. Whoever fixes `worker.go:129` must fix `scan_broadcaster.go:642` in the same change. See new finding **N1**. Same-shape nil derefs elsewhere: I found no other unconditional deref of an `omitempty` pointer in the scanner. `s.Artifacts.LinuxDistribution` is nil-checked at `syft.go:58` and `grype.go:63`. `m.Vulnerability.Metadata` is nil-checked at `grype.go:277`. `result.Summary` is the only one. ##### 2. Dead cursor; comment claims exponential backoff, code sleeps a flat 5s **AGREE on both. The cursor half is understated.** `client/hold.go:47` declares `var cursor int64 = -1`, passes it to `connectOnce(cursor)` at line 56, and never assigns to it. `connectOnce:91-93` therefore never sets the `cursor` query param. `client/hold.go:62` says "Exponential backoff with max 30s" directly above a flat `time.After(5 * time.Second)` at line 66 — a comment that lies, and the kind that marks a half-finished change. Why the cursor matters more than "dead variable": the hold reads it (`pkg/hold/pds/xrpc.go:1093-1101`) and passes it to `drainPendingJobs`, whose query is `WHERE status = 'pending' AND seq > ?` (`scan_broadcaster.go:757-761`). With no cursor the hold defaults to `-1`, so every reconnect drains **every** pending row from the beginning of time. That is the safe direction, but it means the resume protocol is entirely notional: the scanner has no way to say "I already handled up to N", the wire format has a field for it, the hold parses it, and nothing ever populates it. Anyone reading this assumes resumption works. Also worth noting the flat backoff is not merely cosmetic: if `hold.url` fails to parse (`connectOnce:73-75`) the loop retries a permanently-invalid URL every 5s forever, logging on each pass, with no escalation and no distinct signal from "hold is briefly down". ##### 3. `sendJSON` only guards `c.conn == nil`, which is never restored **AGREE, with severity moderated — but the return-type half is the real finding.** `c.conn` is set at `client/hold.go:105` and never set back to nil; `connectOnce`'s `defer conn.Close()` (line 102) leaves `c.conn` pointing at a closed socket. `sendJSON:208-215` therefore passes the nil guard, fails the write, logs, and returns. `SendResult`/`SendError`/ `SendSkipped`/`SendAck` all return `void` (lines 172-202), so the worker at `worker.go:115-124` cannot know its result was dropped. Moderating factor the finding omits: the loss window is bounded. On disconnect the hold's `Unsubscribe` re-marks that scanner's `assigned`/`processing` rows as `pending` (`scan_broadcaster.go:383-386`), and on reconnect `drainPendingJobs` re-offers them. So a dropped result costs a re-scan, not a permanently lost job. Rate it MEDIUM, not HIGH. Aggravating factor the finding omits: once the reconnect *has* completed, `c.conn` points at a live connection and the late result is delivered — while the hold has already re-dispatched the same job. Two workers then scan the same manifest and two `result` messages arrive for the same seq. `handleResult` has no idempotency guard; the second one re-uploads the SBOM and vuln blobs to S3 and rewrites the record. Wasted work and wasted storage, not corruption (rkey is the digest). The durable fix is the signature, not the nil check: these four methods should return `error`, and `nil`-ing `c.conn` under the mutex when `connectOnce` unwinds should be a secondary tidy-up. ##### 4. `buildOCILayout` never verifies downloaded bytes against the claimed digest **AGREE.** `extractor.go:172-181` → `client.DownloadBlob` (`client/hold.go:266-288`) does a plain `io.Copy(out, resp.Body)`. Nothing hashes the stream, nothing compares to `digest`, and nothing checks the byte count against `layer.Size` — which is then written verbatim into the synthesised manifest descriptor (`extractor.go:117`). Two consequences beyond "we trust the storage layer": - A truncated download (proxy hiccup, S3 partial) produces a *silently wrong* SBOM rather than an error, because the layer tar just ends early and Syft catalogues whatever it managed to read. For a security tool "fewer packages than reality" is the worst failure direction. - The size mismatch may instead surface as an opaque stereoscope/go-containerregistry error ("size mismatch"), which is reported to the hold as a retryable error and retried forever. A single `io.TeeReader` into `sha256.New()` in `DownloadBlob`, compared before the file is considered good, closes both. It also happens to close finding 5. ##### 5. `digestHex()` splits on `":"` and the remainder is joined onto the blobs dir **AGREE — and materially understated. This is the security finding in the module.** `extractor.go:184-189` returns everything after the first `:` with no validation. `extractor.go:173-174` does `filepath.Join(blobsDir, hex)` and hands it to `os.Create` via `DownloadBlob` (`client/hold.go:277`). CONFIRMED by scratch program that `filepath.Join` resolves `..` segments into a genuinely escaping absolute path — it does not sanitise. The reason this is worse than a hypothetical: **the attacker's own PDS is an unvalidated input path into this function.** The hold's proactive discovery loop reads `io.atcr.manifest` records straight out of a user's repo (`scan_broadcaster.go:1075-1090`), and a user can write arbitrary records to their own repo. The only gates applied are `holdDID == ours` (attacker-chosen), `len(Layers) > 0`, `Subject == nil`, `Config != nil` (`scan_broadcaster.go:1086-1089`). Digest *format* is never checked — not in the hold, not in `client/hold.go`, not in `extractor.go`. The push path is better protected (the distribution library validates digests on manifest PUT), but the discovery path bypasses it entirely. Reachability of the *write* still requires the blob fetch to return HTTP 200, because `os.Create` runs after the status check (`client/hold.go:274-277`). The hold turns the digest into the S3 key `docker/registry/v2/blobs/sha256///data` (`pkg/s3/types.go:459`, `xrpc.go:1586-1605`) with no normalisation, so a raw S3 backend most likely 404s. That is a deployment-dependent mitigation, not a control: any CDN or gateway in front that normalises `..` in the path (per MEMORY.md, production fronts storage with Bunny) turns the 404 into a 200 for a real object. Impact if reachable, under the *production* sandbox: `systemd/scanner.service.tmpl` grants `ReadWritePaths={{.DataDir}}` = `/var/lib/seamark`, which contains **both** `scanner/tmp` and `scanner/vulndb` (`configs/scanner.yaml.tmpl:18-19`). So an escaping write stays inside the sandbox but can land on the Grype vulnerability database — corrupting or replacing the DB every subsequent scan matches against. A security scanner that reports "0 vulnerabilities" is a worse outcome than one that crashes. Under `Dockerfile.scanner` (`FROM scratch`, no `USER`, runs as root) there is no sandbox at all. Class of problem, not an exploit: unvalidated, externally-sourced identifiers used as filesystem path components. Fix: reject any digest not matching `^[a-z0-9]+(?:[.+_-][a-z0-9]+)*:[a-zA-Z0-9=_-]+$` (the OCI grammar) at the `ScanJob` boundary in `client/hold.go` before enqueue, and independently refuse any `digestHex` result containing a path separator or `.` before it is joined. Verifying the content hash (finding 4) makes the write self-defeating even if the path check is bypassed. ##### 6. `MaxImageSize` sums claimed sizes; nothing bounds decompressed bytes **AGREE, and there is a third unbounded dimension the finding misses.** `worker.go:212-221` sums `layer.Size` + `job.Config.Size` — numbers from the same attacker-writable manifest record as the digests. A manifest can claim `size: 1` per layer and pass. The finding names the decompressed dimension (stereoscope extracting into `TMPDIR`, which `worker.go:52-63` deliberately points at the scanner data volume — so a zip-bomb layer fills the same partition the Grype DB and the hold's data live on). It misses that the **compressed transfer is equally unbounded**: `DownloadBlob` (`client/hold.go:283`) does a bare `io.Copy` with no `io.LimitReader`. The claimed size is never used as a transfer ceiling. The only backstop is the 5-minute `httpClient.Timeout` (`client/hold.go:23`) — which is a rate limiter, not a size limiter, and see N9 for how badly that interacts with honest large layers. Fix shape: pass the claimed size into `DownloadBlob`, copy through `io.LimitReader(resp.Body, claimed+1)`, error if it overruns; and enforce a separate decompressed-bytes budget by checking free space on `TmpDir` before and during extraction. ##### 7. Ack-on-receipt versus the hold's 10-minute processing deadline **AGREE, exactly as described.** `client/hold.go:159` sends the ack from the WebSocket read loop, before `queue.Enqueue` on line 162. The hold's `handleAck` moves the row `assigned` → `processing` (`scan_broadcaster.go:525-528`) and does **not** touch `assigned_at`. The timeout is `WHERE status = 'processing' AND assigned_at < now-10m` (`scan_broadcaster.go:833-837`), so the clock runs from `dispatchJob`'s assignment (`scan_broadcaster.go:417-420`), not from the start of work. Quantifying it: production is `workers: 2`, `queue_size: 100`, and `JobCooldown = 10s` (`worker.go:154`) between jobs. A single deep image can occupy a worker for minutes. A queue only ~10 jobs deep can therefore push a job's *start* past the 10-minute mark, at which point the hold declares it failed and re-dispatches it — while the scanner is still holding it and will eventually return a result for a seq the hold has already marked `failed`. `handleResult` will happily process that late result and set the row back to `completed`, so the states silently disagree. Cheapest correct fix: ack when the worker *dequeues*, not when the client receives; or have the hold refresh `assigned_at` on ack so the processing deadline measures processing. ##### 8. `"version": "v0.107.1"` hardcoded against grype v0.118.0 **AGREE, straightforward.** `grype.go:134` versus `scanner/go.mod:6` (`github.com/anchore/grype v0.118.0`). The descriptor is published into a public `io.atcr.hold.scan` record, so anyone reconciling a report against a Grype changelog or a matcher behaviour change is reading a lie. Related drift worth folding into the same fix: the hold stamps `"atcr-scanner-v1.0.0"` at `scan_broadcaster.go:605`/`638`/`670`, `Dockerfile.scanner` labels the image `0.1.0`, and `config.version` defaults to `"0.1"` — four independent, all-wrong version strings for one component. Ideally read the real version from `runtime/debug.ReadBuildInfo()` (the binary already imports `runtime/debug` in `main.go`) rather than hardcoding a fifth. ##### 9. Multi-arch index manifests fail as a retryable error **AGREE with the mechanism; the "only the hold's guard prevents this" claim is slightly overstated.** The mechanism is exactly right, and `TestIndexManifestIsRetriedForever` (`internal/e2e/pipeline_test.go:322-345`) already pins it: `skipReason` returns `""` because an index's config media type isn't in `unscannableConfigTypes` and the layer check is gated on `len(job.Layers) > 0` (`worker.go:174`); `buildOCILayout` then fails with "config blob has empty digest" (`extractor.go:68-71`); an error is retryable. The overstatement: there are **two** hold-side guards, not one, and they are independent implementations of the same rule — which is itself the risk. - Push path: `HasScannableContent()` = `!IsMultiArch() && !IsReferrer()` (`pkg/hold/oci/xrpc.go:238-240`), applied at `xrpc.go:436`. - Discovery path: an open-coded `len(manifest.Layers) == 0 || manifest.Subject != nil || manifest.Config == nil` (`scan_broadcaster.go:1087-1089`). Two copies of one invariant in two packages, with the scanner having no defence of its own, is the real finding. And the second copy is *not* equivalent to the first: it also rejects `Config == nil`, which the first does not. The right fix is still on the scanner side — make "empty config digest" a `*SkipError`, not an error — so that a regression in either hold guard degrades to a skip instead of an infinite retry. --- #### Section 2 — new findings Ordered by severity. ##### N1 — [BLOCKER] [SUSPECTED] Fixing `worker.go:129` naively panics the *hold* instead `pkg/hold/pds/scan_broadcaster.go:642-644` (trigger at `scanner/internal/scan/worker.go:124`) The hold's `handleResult` nil-guards `msg.Summary` for the record write at line 599 but dereferences `msg.Summary.Critical`, `.High`, `.Total` in the trailing `slog.Info` at 642-644, outside the guard. `ResultMessage.Summary` is `omitempty` (`scanner/types.go:76`), and the scanner constructs the message straight from `result.Summary` (`client/hold.go:181`) — which is nil whenever `vuln.enabled=false`. Today the scanner panics first (finding 1) so this never fires. The instant someone guards `worker.go:129` and lets a summary-less result onto the wire, the panic moves from the scanner to the hold, into a WebSocket reader goroutine, taking the registry's storage backend down with it. Why I believe it: the deref is plainly outside the `if msg.Summary != nil` block; grep of `msg.Summary` in that file shows 599 opening the guard and 642-644 well past its close at 621. Fix: guard the trailing log (or build the log attrs from a zero-value summary), in the *same* commit as the `worker.go:129` fix. Decide deliberately whether a summary-less result is legal at all — if it is not, the scanner should refuse to send one and the hold should reject it. ##### N2 — [HIGH] [SUSPECTED] No `recover()` anywhere: one malformed layer kills the process `scanner/internal/scan/worker.go:88-145`, `cmd/scanner/main.go:159-164` `worker()` calls `processJob` with no `defer recover()`, and `main` has none either. Everything `processJob` touches downstream — stereoscope's OCI layout reader, Syft's cataloguers, and the archive stack Syft pulls in (`go.mod` lists sevenzip, rardecode, squashfs, lzip, ppmd, xz, brotli, diskfs, rpmutils, sif, macholibre) — is parser code operating on **fully attacker-controlled bytes**. That is a large surface for an index-out-of-range or a nil map write. Any panic anywhere in that stack takes down the whole scanner, kills the sibling worker's in-flight scan and the WebSocket loop, and (per finding 1's analysis) enters a permanent systemd restart loop because the poisoned job is re-served after the hold's 10-minute processing timeout. A single crafted image is therefore a fleet-wide, self-sustaining DoS on scanning. Fix: `defer func() { if r := recover(); r != nil { … } }()` wrapping the per-job body, converting the panic into a `SendError` (or better a `SendSkipped`, since a panic on a given image is deterministic — see N4) and letting the worker continue. ##### N3 — [HIGH] [SUSPECTED] The periodic 50-scan vulnDB reload is unreachable; its comment lies `scanner/internal/scan/grype.go:193-205` The write-locked double-check is `if vulnDB != nil && time.Since(vulnDBBuilt) < vulnDBRefreshAge` (line 193), and the 50-scan counter/reload lives inside it (lines 196-201). But the read-locked fast path at line 181 already returns whenever `vulnDB != nil && (fresh || withinBackoff)`. Reaching the write lock with `vulnDB != nil` therefore *implies* `!fresh`, so line 193's condition is false by construction. `vulnDBScans` never increments and the reload never happens, except in a vanishing race window where another goroutine completes a reload between the fast path's `RUnlock` (line 185) and this goroutine's `Lock` (line 187). The comment at lines 197-198 states the reload happens "every 50 scans to flush SQLite's page cache and mmap region". It does not. That matters here specifically: `main.go:46` sets a 512 MiB soft memory limit, production runs 2 workers, and `worker.go:132-136` goes to the trouble of a forced `runtime.GC()` plus a 10-second cooldown per job — the whole file is shaped around memory pressure, and the one mechanism aimed at the largest retained allocation is inert. Corroboration: `grype_test.go` resets `vulnDBScans` in `resetVulnDBState` but no test asserts the 50-scan behaviour, consistent with it never having worked. Fix: increment the counter on the read-lock fast path (atomically, it already is an `atomic.Int64`) and take the write lock to reload when it trips, rather than burying the counter in a branch the fast path pre-empts. ##### N4 — [HIGH] [SUSPECTED] Enumerated deterministic failures reported as retryable errors `scanner/internal/scan/worker.go:107-122`, `extractor.go:68-71`, `worker.go:207-221` The hold retries `error` on the stale-scan loop (`scan_broadcaster.go:1204-1210` skips only `ScanStatusSkipped`) and never retries `skipped`. Every deterministic failure below is therefore an infinite retry loop that also writes a fresh public failure record each pass: 1. **Empty config digest** (`extractor.go:68-71`) — multi-arch index; finding 9. 2. **Image too large** (`worker.go:218-219`) — the claimed size does not change on retry. This is the clearest one: `TestOversizeImageIsRejectedBeforeDownload` asserts `error` today. 3. **Malformed digest** — a digest whose S3 key does not exist 404s on every attempt (`extractor.go:178`). 4. **`ensureDir` failure** (`worker.go:207-209`) — a misconfigured or unwritable `tmp_dir` fails identically forever; see N5. 5. **Cold-start vulnerability DB failure** (`grype.go:241`, surfaced via `worker.go:249`) — retries are appropriate here, but at the rescan interval with a permanent record written each time. 6. **`sha512`/non-sha256 digests** — see N7; deterministic. 7. **Layer/config size mismatch rejected by stereoscope** — deterministic for a given manifest. Only 5 is genuinely transient. Fix: classify. Anything determined by the *content of the job* rather than the *state of the world* should be a `*SkipError`. The `SkipError` type already exists (`worker.go:27-31`) and the transport already exists (`SendSkipped`); it is just under-used. ##### N5 — [HIGH] [SUSPECTED] `vuln.tmp_dir: ""` deterministically breaks every scan, silently `scanner/internal/scan/worker.go:58-63` and `worker.go:207-209, 265-267` CONFIRMED by scratch program: `os.MkdirAll("", 0755)` returns `mkdir : no such file or directory`. It does not mean "use the default". `Start` skips the TMPDIR export entirely when `TmpDir == ""` (`worker.go:58`), and then `processJob` calls `ensureDir("")` on every job (`worker.go:207`), which fails. Every job returns `failed to create tmp dir`, which is a retryable error (N4), forever. Nothing at startup validates it — `config.LoadConfig` only checks `hold.url` and `hold.secret` (`internal/config/config.go:116-121`). `""` is a natural thing for an operator to write meaning "use the system temp dir", and the `comment` tag ("Directory for temporary layer extraction", `config.go:56`) does nothing to warn them off. It is also the value you get by hand-writing a minimal YAML instead of running `config init`. Aggravating: even if `ensureDir` were fixed to fall through to the system default, `Dockerfile.scanner` builds `FROM scratch` — there is no `/tmp` in the image, so `os.MkdirTemp("", …)` (`extractor.go:50`) would fail too, and Grype's go-getter download would land nowhere. Fix: validate in `LoadConfig` that `vuln.tmp_dir` is non-empty and writable, and fail startup loudly rather than failing every job forever. ##### N6 — [HIGH] [SUSPECTED] Silent drops: a malformed job message strands the row on the hold `scanner/internal/client/hold.go:120-142` Four `continue` statements send nothing back at all: - line 123: message fails to unmarshal as `ScanJobRaw`; - line 128: `raw.Type != "job"`; - line 135: `raw.Config` fails to unmarshal; - line 141: `raw.Layers` fails to unmarshal. Cases 3 and 4 have already consumed a valid `raw.Seq` — the scanner knows precisely which job it is dropping and says nothing. Worse, the ack is only sent at line 159, *after* both unmarshals, so the hold's row stays `assigned`. `reDispatchTimedOut` re-offers it after the 5-minute `ackTimeout` (`scan_broadcaster.go:158`, `830`), and the same message is redelivered and dropped again — a 5-minute-period infinite loop with no scan record ever written and nothing in the hold's logs explaining why. This is the "worse than an error" case the brief names. Today the hold always marshals `Config`/`Layers` with `json.Marshal` (`scan_broadcaster.go:1379-1380`, `xrpc.go:449`) so these are unreachable *from the current hold*. That is a protocol-version assumption, not a guarantee. Fix: for any message carrying a parseable `seq`, respond — `SendSkipped` for a structurally undecodable job, since redelivery cannot change the outcome. ##### N7 — [MEDIUM] [SUSPECTED] The OCI layout hardcodes `blobs/sha256/` regardless of algorithm `scanner/internal/scan/extractor.go:61`, `173-174`, `184-189` `blobsDir` is `filepath.Join(scanDir, "blobs", "sha256")` unconditionally, while `digestHex` discards the algorithm. A `sha512:` descriptor (legal OCI, `image-spec` registers sha512) is written to `blobs/sha256/` while the manifest descriptor still says `sha512:` (`extractor.go:103`, `116`), so the layout reader looks in `blobs/sha512/` and finds nothing. Deterministic failure, reported as a retryable error (N4). Fix: derive the directory from the digest's algorithm component — which requires parsing the digest properly, which is the same change finding 5 needs. ##### N8 — [MEDIUM] [SUSPECTED] Dropping non-tar layers desynchronises the manifest from the config `scanner/internal/scan/extractor.go:107-119` vs `worker.go:174-193` `buildOCILayout` filters non-tar layers out of the synthesised manifest, but the **config blob is copied through untouched** (`extractor.go:100-104`) with its original `rootfs.diff_ids` array intact. For a mixed-content manifest — which `skipReason` explicitly allows through, and which `worker_skip_test.go:55-58` pins as "mixed layers keep the scannable one" — the resulting layout has a config claiming N diff_ids and a manifest listing M < N layers. go-containerregistry/stereoscope validate this relationship when constructing an image; the typical symptom is `mismatched image rootfs and manifest layers`. If it errors, that is another deterministic-failure-as-retryable-error. If it *doesn't* error, layers are attributed to the wrong history entries and the SBOM's layer provenance is wrong. What would settle it: feed the pipeline a real mixed manifest (an image whose manifest carries both a tar layer and an in-toto layer) and see whether stereoscope errors or silently mis-attributes. The e2e harness plus the corpus can express this; the "mixed" case currently exists only as a `skipReason` unit test, which never reaches `buildOCILayout`. ##### N9 — [MEDIUM] [SUSPECTED] The 5-minute HTTP timeout is a hard ceiling on total layer transfer `scanner/internal/client/hold.go:23`, `266-288` `http.Client.Timeout` covers the entire exchange including body read. A 1.5 GiB layer — well inside the 2 GiB `max_image_size` default (`config.go:82`) — needs a sustained ~5 MB/s just to finish. Any slower origin, cold CDN, or throttled S3 endpoint fails the download at exactly the 5-minute mark, every time, for that image. Reported as a retryable error, retried forever (N4), never succeeding. The comment at lines 21-22 frames the timeout as leak prevention, which is right, but a whole-request timeout is the wrong instrument: it conflates "stalled" with "large". A stall is better caught by a response-header timeout plus a per-read idle timeout on the transport, leaving total duration bounded by the size limit instead. ##### N10 — [MEDIUM] [SUSPECTED] The shared secret travels in the URL query string `scanner/internal/client/hold.go:90` (and `pkg/hold/pds/xrpc.go:1084-1089`) `q.Set("secret", c.secret)` puts the long-lived shared secret in the request line of the WebSocket upgrade. Request lines land in reverse-proxy access logs, CDN logs, and any hold-side HTTP middleware that logs URLs — none of which are treated as secret stores. The hold already accepts the same credential in an `X-Scanner-Secret` header (`xrpc.go:1086`), so the safe path exists and is simply not taken. The scanner's own log is careful (`client/hold.go:96` logs `u.Host` only), which suggests the risk was noticed on one side and not the other. Secondary, same credential: `ValidateScannerSecret` compares with `==` (`scan_broadcaster.go:930-932`) rather than `subtle.ConstantTimeCompare`. Fix: send the header, and drop the query-param branch from the hold once no old scanners remain. ##### N11 — [MEDIUM] [SUSPECTED] Nothing detects a silently dead WebSocket; `/healthz` lies about it `scanner/internal/client/hold.go:111-118`, `cmd/scanner/main.go:92-95` Grep confirms neither side sets `SetReadDeadline`, `SetPongHandler`, or sends pings (the only `WriteControl` in the tree is in `mockhold`). A connection killed by a NAT/idle-LB timeout or a network partition leaves `conn.ReadMessage()` blocked indefinitely: the scanner believes it is connected, `c.conn` is non-nil so sends "succeed" into the void, no reconnect is attempted, and the hold — which also has no keepalive — parks jobs against a subscriber that will never answer. Meanwhile `/healthz` returns 200 unconditionally: it reflects only that the HTTP goroutine is scheduled. It knows nothing about connection state, queue depth, worker liveness, or whether the vuln DB ever loaded. An orchestrator restarting on health failure will never restart the one failure mode that actually needs it. This is the exact "an idle scanner is indistinguishable from a wedged one" problem `main.go:54-56` claims to have solved for logging. Fix: `SetReadDeadline` + periodic ping with a pong handler that extends it; and make `/healthz` report connected-ness and last-job-completed age. ##### N12 — [MEDIUM] [SUSPECTED] Failure strings become public ATProto records `scanner/internal/scan/worker.go:121`, `extractor.go:63/92/178`, `client/hold.go:252` `SendError(job.Seq, err.Error())` ships the fully wrapped error, and the hold writes it verbatim into an `io.atcr.hold.scan` record via `NewFailedScanRecord` (`scan_broadcaster.go:600-608`) — a public record in the hold's repo, readable by anyone. What can end up in there: the scanner's temp directory path (`extractor.go:63`, `"failed to create temp directory: %w"` wraps the OS error including the path); internal hold error bodies verbatim (`client/hold.go:252` embeds the hold's response body, which is then wrapped twice more at `extractor.go:178` and `worker.go:227`); and the digest of every blob involved. Nothing is redacted or truncated. Fix: send a bounded, classified failure string to the hold and keep the full wrapped error in the scanner's own logs. ##### N13 — [MEDIUM] [SUSPECTED] No dedup or bound on duplicate seqs; queue full is a permanent-looking failure `scanner/internal/queue/priority_queue.go:91-111`, `client/hold.go:162-167` `Enqueue` does not check whether `job.Seq` is already queued or in flight. The hold re-dispatches after `ackTimeout` (5m) and on every reconnect, so the same seq can legitimately arrive twice; both copies are scanned and both results sent. With `workers: 2` and expensive images this doubles the cost of exactly the situation (a backlog) that caused it. Separately, `Enqueue` returns `false` for two very different conditions — queue full (line 99) and queue closed (line 95) — and the caller reports both as `"scanner queue full"` (`client/hold.go:166`). During shutdown, every in-flight job is thus reported to the hold as a capacity failure and written as a permanent public failure record, when the truthful answer is "this scanner is going away, give it to someone else". Fix: key the queue by seq for dedup; distinguish closed from full and, on closed, send nothing (let `Unsubscribe`'s re-pending do its job) rather than a misleading error. ##### N14 — [MEDIUM] [SUSPECTED] `initializeVulnDatabase`'s failure message describes behaviour that doesn't exist `scanner/internal/scan/worker.go:66-73` On failure the log says *"Vulnerability scanning will be disabled until database is available"*. Nothing is disabled. `wp.cfg.Vuln.Enabled` is untouched, so `processJob` still enters the Grype branch (`worker.go:245`), `scanVulnerabilities` still calls `loadVulnDatabase`, and each job fails with `failed to load vulnerability database` (`grype.go:59`) — a retryable error, retried forever (N4), with a public failure record per attempt. Also, this goroutine is untracked by `wp.wg` (contrast lines 75-78), so `pool.Wait()` returns while a multi-hundred-MB DB download is still running, and the process exits mid-download. Third, workers start immediately (line 75) and race the init: the first jobs each call `loadVulnDatabase` and serialise on the write lock behind the init's download, holding jobs for the full download duration — which counts against the hold's 10-minute processing deadline (finding 7). Fix: either genuinely degrade (scan SBOM-only and say so in the result) or fail startup. The current middle position is the one that helps nobody. And `wg.Add(1)` the goroutine. ##### N15 — [LOW] [SUSPECTED] `ctx` is threaded through the Grype path and used for nothing `scanner/internal/scan/grype.go:55`, `58`, `174`, `267` `scanVulnerabilities(ctx, …)` passes `ctx` to `loadVulnDatabase(ctx, …)`, which never reads it; `loadVulnDB` (`grype.LoadVulnerabilityDB`) takes no context; `FindMatches` takes a `grypePkg.Context`, not a `context.Context`. The parameter implies cancellation and delivers none. Consequences: a shutdown cannot interrupt a vulnerability DB download or a long match run, so `pool.Wait()` in `main.go:114` can block for the download's full duration with no ceiling — and `main` sets no shutdown timeout at all, so SIGTERM handling depends entirely on systemd's `TimeoutStopSec` SIGKILL. Fix: drop the parameter (honest), or wrap the load in a goroutine + `select` on `ctx.Done()` so the signature becomes true. ##### N16 — [LOW] [SUSPECTED] `Dequeue` ignores context; `Start(ctx)`'s cancellation contract is partial `scanner/internal/queue/priority_queue.go:115-129`, `worker.go:93-98` `Dequeue` blocks on a `sync.Cond` and can only be released by `Close()`. Cancelling the context passed to `WorkerPool.Start` releases a worker only at the cooldown `select` (`worker.go:139-143`); a worker parked in `Dequeue` stays parked forever. `main` gets this right by calling `q.Close()` (line 113), but the API's shape says otherwise, and the e2e harness's cleanup (`harness.go:110-112`) depends on the same undocumented ordering. Fix: document that `Close()` is mandatory, or give `Dequeue` a context-aware variant. ##### N17 — [LOW] [SUSPECTED] `strings.Contains(mediaType, "tar")` as the scannability test `scanner/internal/scan/extractor.go:85`, `111`; `worker.go:188` A substring test on an untrusted media type. It is correct for every real layer type I can think of (`…tar`, `…tar+gzip`, `…tar+zstd`, `…rootfs.diff.tar.gzip`, `…nondistributable.v1.tar+gzip`), and the two call sites agree with each other so the comment at `worker.go:181-182` is honest. But the predicate accepts anything containing the letters "tar" anywhere, and rejects a legitimately scannable layer whose type omits it. The three copies (two in `extractor.go`, one in `worker.go`) must not drift; they should be one function. ##### N18 — [LOW] [SUSPECTED] Dead fields, dead stores, vestigial cleanup - `ScanResult.SBOMDigest` / `.VulnDigest` (`types.go:49`, `51`) are computed (`syft.go:75`, `grype.go:145`) and never transmitted — `ResultMessage` has no digest fields (`types.go:71-77`). The hold recomputes nothing and re-uploads the bytes. Either send them (so the hold can verify) or stop computing them. - `ScanResult.ManifestDigest` (`types.go:47`) is set at `worker.go:239` and never read; the hold correlates by seq alone. - `result = nil` at `worker.go:135` and `sbomResult = nil` at `worker.go:255` are dead stores to locals about to leave scope. They cannot help the GC (the compiler already knows), and the comment at `worker.go:132-134` claims they do. - `ScanJob.UserDID` and `.Tag` are carried across the wire and used only in log lines. ##### N19 — [LOW] [SUSPECTED] Documentation and deployment-template drift - `CLAUDE.md` states twice that the scanner is **"env vars only, no YAML"** and *"Scanner config (env-only, no Viper)"*. `internal/config/config.go` is a Viper implementation with a YAML file, a `--config` flag (`main.go:151`), and a `config init` subcommand. Every other component's docs match its code; this one doesn't. - There is no `config-scanner.example.yaml` at the repo root, although `config-appview`, `config-hold` and `config-labeler` all exist and `atcr-scanner config init` can generate one. - `deploy/upcloud/configs/scanner.yaml.tmpl` omits `vuln.max_image_size` entirely, so the deployed config is out of sync with the config struct — the exact step CLAUDE.md's implementation checklist item 2 exists to prevent. (Behaviour is currently correct by accident: the Viper default supplies 2 GiB.) - `Dockerfile.scanner` declares `org.opencontainers.image.version="0.1.0"`; see finding 8 for the other three disagreeing version strings. ##### N20 — [LOW] [SUSPECTED] `index.json` omits the image-index `mediaType` `scanner/internal/scan/extractor.go:30-33`, `136-145` `ociIndex` has no `MediaType` field, so the generated `index.json` has none. OCI image-spec 1.1 says the index's `mediaType` property "SHOULD be used and MUST be `application/vnd.oci.image.index.v1+json`". Stereoscope tolerates the omission today (the e2e run above got a real SBOM out of a real image), so this is spec hygiene rather than a live defect — but it is the kind of omission that breaks on a dependency bump, and it would fail `oci-image-tool validate`. Also missing and worth considering: no `org.opencontainers.image.ref.name` annotation on the index's manifest descriptor, which is what gives the layout a usable reference name; `syft.go:41` passes the *directory path* as the reference instead, which is why source names in SBOMs are temp paths (see I5). ##### N21 — [LOW] [SUSPECTED] Untrusted strings reach log lines unsanitised `scanner/internal/scan/worker.go:100-105`, `126-129`, `extractor.go:72`, `86`, `89` `job.Repository`, `job.Tag`, `job.UserHandle`, `job.ManifestDigest` and `layer.MediaType` all originate in a user-writable PDS record and are logged directly. `slog`'s handlers quote and escape values, so this is not classic log injection — but these fields also flow to the remote log shipper configured in production (`main.go:57-65`), and nothing bounds their length. A manifest record with a megabyte-long `repository` is logged on every job. ##### N22 — [LOW] [SUSPECTED] No validation of numeric config; broken values fail silently `scanner/internal/config/config.go:107-123` `LoadConfig` validates two strings and nothing else: - `scanner.workers: 0` (or negative) starts no workers (`worker.go:75`). The client still connects, still acks every job, still enqueues — and the hold sees healthy acks while nothing is ever scanned. Worst possible shape: it looks alive. - `scanner.queue_size: 0` makes `Enqueue` always return false (`priority_queue.go:99`), so every job is rejected as "queue full" forever. - `vuln.max_image_size: -1` is treated as "no limit" by the `> 0` guard (`worker.go:212`), which is not what "-1" suggests to anyone. - `vuln.db_path: ""` → `os.MkdirAll("")` fails inside `loadVulnDatabase` (`grype.go:218`), same shape as N5. ##### N23 — [LOW] [SUSPECTED] The scanner sends its shared secret to whatever endpoint the job names `scanner/internal/scan/extractor.go:176` → `client/hold.go:230-242` `GetBlobPresignedURL` takes `job.HoldEndpoint` — a field from the job message — and sends `Authorization: Bearer ` to it, with no scheme check and no comparison against the configured `hold.url`. The value is set hold-side from its own config (`scan_broadcaster.go:290`), so it is same-trust-domain today; but a misconfigured `server.public_url` on the hold (e.g. left as `http://`) silently downgrades every credential-bearing request to cleartext, and the scanner has no say. Fix: require the endpoint's origin to match the configured hold URL, or at minimum refuse to attach the bearer token to a non-TLS URL. ##### N24 — [LOW] [SUSPECTED] Duplicate digests are downloaded once per occurrence `scanner/internal/scan/extractor.go:79-94` The layer loop does no digest dedup. Images that repeat a layer digest (common with shared base layers or repeated empty layers) fetch and rewrite the same file multiple times — wasted presigned URL round-trips and wasted bytes, and each repeat counts against the 5-minute HTTP window (N9). Trivial fix: a `map[string]bool` of already-fetched digests. ##### N25 — [LOW] [SUSPECTED] `Close()` panics if called twice `scanner/internal/client/hold.go:219-226` `close(c.done)` with no `sync.Once` guard. One call site today (`main.go:112`) plus one in the test harness, so it is latent — but the method reads as idempotent and isn't. --- #### Section 3 — improvements that are not bugs ##### I1 — There is no metric surface at all Not one counter, gauge, or histogram. Nothing reports jobs received, queue depth by tier, scan duration, download bytes, DB age, or failure counts by class. Every question the findings above raise ("how often does N9's timeout fire?", "is N13's duplicate dispatch actually happening?") is currently unanswerable in production without reading journald by hand. `/healthz` (`main.go:92-95`) is the natural place to start: make it a JSON status document rather than a literal `ok`. ##### I2 — Split `client.HoldClient` into transport and protocol The type currently owns URL construction, dialing, the reconnect loop, message decoding, ack policy, and the write mutex, and it exports package-level HTTP helpers (`GetBlobPresignedURL`, `DownloadBlob`) that have nothing to do with the WebSocket. That coupling is why findings 2, 3, N6, N10 and N13 all live in one 288-line file and why none of them are unit testable — there is no seam. A `Conn` (dial/read/write/reconnect) under a `Session` (decode, ack policy, enqueue) would make the ack-timing question in finding 7 a two-line change. ##### I3 — The digest is a value type waiting to be introduced `digestHex` (`extractor.go:184`) is the third place a digest is destructured by string surgery (the others: `s3.BlobPath`, `pkg/hold/pds` rkey handling). A parsed `Digest{Algo, Hex}` type with a validating constructor at the wire boundary would make finding 5, N7 and half of N4 structurally impossible rather than individually patched. `github.com/opencontainers/go-digest` is already an indirect dependency. ##### I4 — The wire protocol has no version and no schema `ScanJobRaw` / `ResultMessage` / `ErrorMessage` / `SkippedMessage` (`types.go`) are duplicated by hand on the hold side (`ScanJobEvent` / `ScannerMessage`, `scan_broadcaster.go:86-110`). Two independently maintained copies of one wire format, with no version field and no lexicon under `lexicons/` (unlike every other hold endpoint). N1 is precisely the class of bug this invites: one side's `omitempty` is the other side's unconditional deref. Either generate both from one source or add the lexicon and a version field. ##### I5 — SBOM source naming leaks temp paths `syft.go:41` sets `Reference: ociLayoutDir` — a path like `/var/lib/seamark/scanner/tmp/scan-2171136778`. That string is embedded in the SPDX document and published, so every published SBOM names a directory that stopped existing seconds later. The reference should be `:@` from the job. ##### I6 — `worker.go` is doing three jobs `processJob` mixes policy (skip rules, size limits), orchestration (download → SBOM → match), and transport (which message type to send). The dispatch `if/else` at lines 107-130 is where finding 1 lives, and the skip-vs-error classification of N4 has to be made in two places as a result. Pulling "classify this outcome into a message" into one function would give N4 a single edit site. ##### I7 — The cooldown is a blunt instrument `JobCooldown = 10s` unconditionally (`worker.go:154`), including after a job that was *skipped* before downloading anything (`worker.go:202-204`) — a path that allocates essentially nothing. A run of attestation-heavy repositories therefore idles 10 seconds per refusal, for no memory reason, while the hold's 10-minute processing deadline runs (finding 7). Gate the cooldown on whether a scan actually ran. ##### I8 — Tests cannot reach the two riskiest paths `internal/scan` has no test for `buildOCILayout` and `internal/client` has no tests at all (`go test` reports "no test files" for both `client` and `config`). Every finding in `extractor.go` and `client/hold.go` above is unverified for exactly that reason. `buildOCILayout` is already close to testable — it takes a `job`, a `tmpDir` and a `secret`, and `mockhold` can serve the blobs; the only obstacle is that the download helpers are package-level functions in `client` rather than an injectable interface. ##### I9 — Comment audit Comments that do not match the code, gathered for one pass: - `client/hold.go:62` — "Exponential backoff with max 30s" over a flat 5s sleep (finding 2). - `grype.go:197-198` — "Periodic reload ... every 50 scans" for a branch that never executes (N3). - `worker.go:70` — "Vulnerability scanning will be disabled until database is available"; nothing is disabled (N14). - `worker.go:132-134` — the `result = nil` / `runtime.GC()` comment credits the nil-store with freeing memory it cannot free (N18). - `client/hold.go:21-22` — describes the 5-minute timeout purely as leak prevention, without noting it also caps total transfer size (N9). - `extractor.go:35-48` — accurate and genuinely useful; called out as the counter-example. ## 13. Hold-side code review (reading) ##### Hold-side scan pipeline: code review Scope: `pkg/hold/pds/scan_broadcaster.go`, `pkg/hold/pds/scan.go`, the push-triggered enqueue path in `pkg/hold/oci/xrpc.go`, the scan record shape in `pkg/atproto/lexicon.go`, the WebSocket boundary against `scanner/types.go` and `scanner/internal/client/hold.go`, and how results surface in `pkg/appview`. Reading pass. No production code was modified and no tests were written. Two things were verified by execution and are marked CONFIRMED; everything else is SUSPECTED. Baseline: `go test -race -run TestScan ./pkg/hold/pds/` passes (6.3s). --- #### Section 1: the job state machine ##### 1.1 Storage One SQLite table, `scan_jobs` (`scan_broadcaster.go:257`). The status column is free text with five observed values: `pending`, `assigned`, `processing`, `completed`, `failed`. Two other pieces of state live outside the table and matter as much: - `sb.inflight` (`:71`) — an in-memory `map[digest]struct{}` that gates proactive discovery. Not persisted; empty on every boot. - `sub.send` (depth 20, `:329`) and the two queues `unscannedQueue` (500) / `staleQueue` (200) — in-memory, lost on restart. ##### 1.2 States and transitions ``` Enqueue (:296) [push notify :459 | proactive :1395] │ + addInflight(digest) (:293, return value ignored) ▼ ┌───────────┐ ┌──────────────│ pending │◄─────────────────────────────────┐ │ └─────┬─────┘ │ │ │ │ │ dispatchJob (:418, guarded by status='pending') │ │ drainPendingJobs (:775, guard present but RESULT IGNORED) │ │ ▼ │ │ ┌───────────┐ │ │ │ assigned │ assigned_at = dispatch time │ │ └─────┬─────┘ │ │ │ │ │ scanner "ack" │ handleAck (:525) │ │ (sent on RECEIPT, not on work start; │ │ assigned_at is NOT refreshed) │ │ ▼ │ │ ┌────────────┐ │ │ │ processing │ │ │ └─────┬──────┘ │ │ │ │ │ ┌────────────────┼─────────────────┐ │ │ │ "result" │ "skipped" │ "error" │ │ ▼ ▼ ▼ │ │ completed completed failed │ │ (:624) (:718) (:673) │ │ +scan record +skipped record +failed record │ │ (only if │ │ Summary!=nil) │ │ │ │ reDispatchTimedOut, assigned older than ackTimeout=5m │ └───(:888) status→pending, assigned_to/at→NULL ────────────────┘ │ │ reDispatchTimedOut, pending older than 1m → re-offered (:902 dispatchJob) │ │ Unsubscribe (:383): ALL rows assigned_to= in └───(pending|assigned|processing) → pending, assigned_to/at→NULL reDispatchTimedOut (:834): processing AND assigned_at < now-10m → failed. NO scan record. NO removeInflight. (terminal; the only edge out of processing that produces no record at all) dispatchJob buffer-full fallback (:445): assigned → pending, unguarded ``` Terminal states: `completed`, `failed`. Nothing ever deletes a row (Finding 11). ##### 1.3 Who performs each transition | Transition | Actor | Site | |---|---|---| | ∅ → pending | push notify handler / proactive dispatch loop | `oci/xrpc.go:459`, `scan_broadcaster.go:1395` → `:296` | | pending → assigned | `dispatchJob` (round-robin, holds `sb.mu`) | `:418` | | pending → assigned | `drainPendingJobs` (per new subscriber, no `sb.mu`) | `:775` | | assigned → pending | `dispatchJob` when the send buffer is full | `:445` | | assigned → processing | `handleAck`, on the reader goroutine | `:525` | | assigned → pending | `reDispatchTimedOut`, after `ackTimeout` (5m) | `:888` | | {pending,assigned,processing} → pending | `Unsubscribe`, per dead subscriber | `:383` | | processing → completed | `handleResult` | `:624` | | processing → completed | `handleSkipped` | `:718` | | processing → failed | `handleError` | `:673` | | processing → failed | `reDispatchTimedOut`, after 10m from **dispatch** | `:834` | Note the two dispatchers for `pending` and the two producers of `failed`. Both pairs are asymmetric, and both asymmetries are bugs (Findings 2 and 3). ##### 1.4 What dfd604b fixed, and what it did not The nine-day outage was: one `pending` row that no code path would ever dispatch again, counted forever by `hasActiveJobs()`, which `waitForCapacity()` spins on. dfd604b added three things: 1. `pendingStaleAfter` (15m) so `hasActiveJobs` stops counting an old pending row (`:1564`). This is the actual unwedge. 2. A pending reclaim in `reDispatchTimedOut` (`:848`) so a stale pending row gets re-offered every 30s instead of waiting for the next scanner connect. 3. A `RowsAffected` guard on `dispatchJob`'s assign UPDATE (`:429`), because (2) created a second dispatcher and two dispatchers can race for one row. What remains: - **(3) was applied to only one of the two dispatchers.** `drainPendingJobs` runs the same guarded UPDATE at `:775` and then sends regardless of whether it won (Finding 2). The race dfd604b closed is still open through the other door. - **The capacity gate is still global, not proactive-scoped.** `hasActiveJobs` (`:1566`) counts every row including push-triggered ones, so on a hold with steady paid-tier pushes the proactive loop never gets capacity. That is a throttle, not a wedge, but it means "one proactive job at a time" is really "zero while anyone is pushing". - **`hasActiveJobs` still returns `true` on any DB error** (`:1573`). The exact failure shape of the outage — proactive scanning silently off forever — is still reachable, now via a persistently failing query rather than a stuck row. - **The pending reclaim is only a re-offer, not a repair.** If the row is undispatchable for a reason inside the row (Finding 27: NULL `user_handle` makes `rows.Scan` fail in both the drain and the reclaim), it is skipped silently by `continue` at `:872` and `:764` forever. It no longer blocks anything, but it also never runs and nothing reports it. - **Nothing reconciles at boot.** Rows left `assigned`/`processing` by a restart hold capacity until their timeouts fire (5m/10m), and the `processing` ones then go to `failed` with no record (Finding 3). ##### 1.5 States a row can enter and never leave Strictly at the row level, every non-terminal state has an exit edge, so there is no permanently stuck row after dfd604b. The defects are one level up: - **`processing` → `failed` is a one-way door with no record and no inflight release** (Finding 3). The *manifest* is what gets stuck: no scan record exists, so the appview shows "Not scanned" forever, and the digest stays in `sb.inflight`, so discovery will refuse to re-queue it for the life of the process. - **`completed`/`failed` are absorbing for the row and the table is never pruned** (Finding 11). - **A row whose `rows.Scan` fails** (NULL `user_handle`, Finding 27) is skipped by both the drain and the reclaim, silently, forever. - **The `pending` row that outlives its subscriber**: `Unsubscribe` (`:383`) runs its requeue UPDATE, and *then* the still-running `drainPendingJobs` goroutine assigns more rows to that same dead subscriber id (`:775`, no membership check). Those rows sit `assigned` to a subscriber that no longer exists until the 5-minute ack timeout. --- #### Section 2: findings ##### F1 — BLOCKER — Nil `Summary` in a `result` message panics the hold process **CONFIRMED by inspection** (unconditional nil dereference; not executed). `pkg/hold/pds/scan_broadcaster.go:638-644` ```go slog.Info("Scan job completed", ... "critical", msg.Summary.Critical, // :642 "high", msg.Summary.High, // :643 "total", msg.Summary.Total) // :644 ``` `msg.Summary` is `*VulnerabilitySummary` (`:107`) and is explicitly nil-checked seventeen lines earlier at `:599`. The log statement is outside that guard. A `result` message with no `summary` key unmarshals to a nil pointer and dereferences it on `handleReader`'s goroutine, which is not recovered anywhere — the hold process dies. This is reachable from the shipped scanner, not just a hostile one. With `vuln.enabled=false`, `scanner/internal/scan/worker.go:245` never sets `result.Summary`, and `SendResult` (`scanner/internal/client/hold.go:177`) copies the nil straight through. The scanner then panics on its own next line (`worker.go:129`) — but only *after* the message is on the wire, so the hold gets it. Both processes crash-loop: the scanner restarts, the job is re-dispatched, the hold dies again. Anything else that produces a summary-less result (a Grype path that returns early, a third-party scanner, a truncated message) has the same effect. The shared secret is the only thing between the network and a remote hold kill. **Fix**: move the three fields into the `msg.Summary != nil` branch, or log a zero-value summary. Then handle F2 — the guard at `:599` is itself wrong. --- ##### F2 — HIGH — `drainPendingJobs` sends jobs it did not win, re-opening the double-dispatch dfd604b closed **SUSPECTED.** `pkg/hold/pds/scan_broadcaster.go:775-791` ```go _, err = sb.db.Exec(` UPDATE scan_jobs SET status = 'assigned', assigned_to = ?, assigned_at = ? WHERE seq = ? AND status = 'pending' `, sub.id, time.Now(), job.Seq) if err != nil { continue } select { case sub.send <- job: ``` The `AND status = 'pending'` guard is present but `RowsAffected` is never read, so a row already claimed by `dispatchJob` or by the re-dispatch loop is still pushed onto this scanner's queue. dfd604b added exactly this check to `dispatchJob` (`:429`) with the comment "Sending it anyway would scan it twice", and the regression test `TestScanDispatchJob_SkipsClaimedJob` covers only that path. The window is wide because `drainPendingJobs` runs unsynchronised on its own goroutine (`:351`) while `reDispatchTimedOut` fires every 30 seconds and `Enqueue` fires on every qualifying push. Concretely: scanner B connects and drains; the re-dispatch tick has already claimed row N for scanner A; B's UPDATE affects zero rows but B receives the job. Both scan it. B's ack is then silently dropped, because `handleAck` (`:527`) does guard on `assigned_to = ? AND status = 'assigned'` — so the row is now `assigned` to A, B is scanning it anyway, and whichever result lands first wins. The mirror-image case is worse: `dispatchJob` assigns row N to A, finds A's buffer full, and resets the row to `pending` at `:445` **with no status guard** — while B, having lost the UPDATE race, is already holding the job. The row goes back to pending, gets handed to a third scanner, and now three copies exist. **Fix**: read `RowsAffected` in `drainPendingJobs` exactly as `dispatchJob` does, and add `AND assigned_to = ?` to the buffer-full reset at `:445`. --- ##### F3 — HIGH — The processing timeout marks a job failed with no scan record and never releases the inflight mark **SUSPECTED.** `pkg/hold/pds/scan_broadcaster.go:833-842` ```go processingTimeout := time.Now().Add(-10 * time.Minute) res, err := sb.db.Exec(` UPDATE scan_jobs SET status = 'failed', completed_at = ? WHERE status = 'processing' AND assigned_at < ? `, time.Now(), processingTimeout) ``` Every other producer of a terminal state writes an `io.atcr.hold.scan` record — `handleResult` (`:600`), `handleError` (`:662`), `handleSkipped` (`:707`). This one writes nothing, and it also never calls `removeInflight`. Two user-visible consequences. **No record.** The appview distinguishes "hold unreachable" / "not scanned" / "skipped" / "scan failed" by fetching the scan record and reading `Status` (`pkg/appview/handlers/scan_result.go:53-72`); a 404 renders `NotScanned` (`:125`), which the badge shows as a grey "Not scanned" pill (`pkg/appview/templates/partials/vuln-badge.html:8`) and the panel as "Scans run automatically shortly after a push. Check back in a few minutes" (`partials/vulns-section.html:25`). A job the hold has definitively given up on is therefore indistinguishable from one that was enqueued thirty seconds ago. There is no "Scan failed" badge, no reason string, no timestamp. **Never retried, for the life of the process.** The digest was added to `sb.inflight` by `Enqueue` (`:293`) and is only removed by the three message handlers. After this UPDATE it is still in the map, so `discoverUnscannedForUser`'s `addInflight` check at `:1107` returns false and `continue`s — permanently. The stale loop cannot pick it up either (it iterates scan *records*, and there is none). So the manifest is excluded from both proactive paths until the hold restarts. The record only ever appears if the scanner it gave up on eventually replies anyway — which does work, because `handleResult` selects by `seq` with no status filter (`:561`) and overwrites `failed` with `completed`. That accidental recovery is the only thing keeping this from being a permanent hole, and it is exactly the case F4 shows is common. **Fix**: write a `NewFailedScanRecord` with a reason like "scanner did not report within 10 minutes" before flipping the status, and call `removeInflight` for each affected digest (the UPDATE must therefore select the digests first, as the sibling loop below it already does). --- ##### F4 — HIGH — `assigned_at` is stamped at dispatch and never refreshed, so a healthy backlogged scanner has its work cancelled and the dispatch throttle inverts **SUSPECTED.** `scan_broadcaster.go:419` (stamp), `:525` (ack does not refresh), `:833` (10-minute deadline measured from the stamp). The scanner acks on *receipt*, before any work: `connectOnce` reads the frame, calls `SendAck(job.Seq)`, and only then does `queue.Enqueue` (`scanner/internal/client/hold.go:159-162`). Default config is `workers: 1`, `queue_size: 100` (`scanner/internal/config/config.go:74-75`). So the elapsed time between `assigned_at` and the start of actual work is the depth of the scanner's local queue multiplied by the per-image scan time — Syft plus Grype plus layer downloads, routinely minutes for a real image. With one worker, the *second* job in a backlog is already at risk, and the fifth is certain to breach ten minutes without anything being wrong. The hold marks it failed (F3: no record, capacity released) while the scanner is still holding it. The second-order effect is the damaging one. `hasActiveJobs` (`:1566`) counts only `assigned`/`processing`, so the moment the row flips to `failed` the dispatch loop believes it has capacity and enqueues another proactive job (`:1285`, `:1322`). That job lands behind the same backlog, breaches ten minutes, is failed, frees capacity again. The "throttled to one proactive job at a time" guarantee in the comment at `:1262` inverts into an unbounded feed under exactly the condition it exists to prevent. A hold with a slow scanner will pump the scanner's 100-slot queue full and then start getting `error: scanner queue full` back (F8). **How often**: any hold whose scanner is not idle. The ten-minute budget is per-connection-backlog, not per-scan, and nothing anywhere caps the backlog. **Fix**: the ack should mean "I have it", and a separate signal should mean "I started it" — either add a `started` message type and refresh `assigned_at` on it, or have the scanner send periodic heartbeats for the job it is working on and reset the deadline on each. Failing that, scale the deadline by the number of jobs outstanding to that subscriber. What would settle the sizing: log the observed distribution of ack→result latency, which is not currently recorded anywhere. --- ##### F5 — HIGH — A five-second network blip permanently un-adopts a predecessor hold's manifests **SUSPECTED.** `scan_broadcaster.go:1468-1475` and `:1480-1545` ```go isPredecessor := sb.checkPredecessor(ctx, holdDID) sb.predecessorCache[holdDID] = isPredecessor // :1474 ``` `checkPredecessor` returns bare `false` for every failure mode: DID resolution failed (`:1487`), HTTP error (`:1505`), non-200 (`:1511`), unreadable body (`:1516`), JSON that does not parse (`:1523`, `:1528`). All of them are cached, and the cache is never invalidated or expired for the life of the process. Its timeout is 5 seconds (`:1481`). So one slow response from a predecessor hold during one discovery pass means every manifest that names that hold is treated as not ours, forever. The hold is likely retired — that is what makes it a predecessor — so "unreachable for five seconds" is its normal condition, and the cache is populated on the first pass after boot. This exact defect was found and fixed in the sibling implementation. `pkg/hold/gc/gc.go` carries a second return value: "The second return value reports whether the answer is definitive. It is false whenever the hold could not be reached or its reply could not be understood" (`gc.go:2027-2035`), the cache comment at `gc.go:257` says "Only definitive answers belong here. The cache is never reset, so a false recorded from an unreachable hold would outlive the outage", and `gc.go:2006-2019` routes non-definitive answers to a per-run `predecessorUnresolved` map instead. The scan broadcaster was never brought along. Blast radius here is milder than GC's (missed scans, not deleted blobs), but it is silent and permanent, and it is logged at `Debug` (`:1486`, `:1506`). Related: `predecessorCache` is read and written with no mutex (`:1468`, `:1474`). Today only the discovery goroutine reaches it, so there is no race yet, but nothing documents or enforces that. **Fix**: port GC's definitive/unresolved split verbatim. --- ##### F6 — HIGH — `handleResult` does S3 uploads and a PDS commit inline on the reader goroutine, with no timeout **SUSPECTED.** `scan_broadcaster.go:543-645`, called from `handleReader`'s loop at `:510`. `ctx := context.Background()` (`:544`) — no deadline anywhere. On that context it does two S3 `PutBytes` (`:575`, `:588`, via `pkg/hold/pds/profile.go:95`) and a `CreateScanRecord` (`:607`) which takes the repomgr per-user lock (`pkg/hold/pds/repomgr.go:345`) and writes a CAR delta. The hold is a single uid, so that lock is contended with every layer record, stats increment and Bluesky post the hold is writing. While this runs, `handleReader` is not calling `ReadMessage`. Acks, errors and results for every other job on that connection are stuck in the socket buffer; the scanner's writes eventually block on it. An S3 endpoint that accepts the connection and then stalls holds the whole scanner connection hostage indefinitely — and because the scanner is still nominally connected, `Unsubscribe` never runs, so nothing requeues the work. Jobs then age out through F4's path. Payload size compounds it: the SBOM and the Grype report arrive as JSON *strings* inside the frame, so a 40 MB SPDX document is ~40 MB of frame, ~40 MB of unmarshalled string, and another ~40 MB of `[]byte` at `:575`, all live simultaneously, per in-flight result. **Fix**: hand the result to a small worker pool (the `startJob` pattern in `pkg/hold/admin/jobs.go` and `gc.startBackground` are the house precedents) and give the context a deadline. Keep the DB status update ordered after the record write so a crash mid-upload leaves the row reclaimable. --- ##### F7 — HIGH — No WebSocket keepalive or read deadline on the hold side **SUSPECTED.** `scan_broadcaster.go:325-357` (Subscribe), `:452-481` (writer), `:484-521` (reader). The hold never calls `SetReadDeadline`, `SetReadLimit`, `SetPongHandler`, or sends a ping. The scanner never pings either (`scanner/internal/client/hold.go:111`). Neither side has a heartbeat. A half-open connection — NAT/conntrack expiry, a load balancer idle timeout, a scanner host that vanished — therefore leaves the subscriber registered in `sb.subscribers` indefinitely. `hasConnectedScanners` (`:1548`) returns true, `dispatchJob` picks it in the round-robin (`:414`), the write into the 20-deep buffer succeeds, and the writer's `WriteMessage` may sit in the kernel send buffer for a long time before the OS gives up (default TCP keepalive is on the order of two hours, and only if `SO_KEEPALIVE` is set). Every job routed to that subscriber goes to `assigned` and then times out five minutes later. With two scanners, round-robin sends half the fleet's work into the hole. The dfd604b commit message notes the hold had been up since Aug 14 and the scanner since Aug 21 "on the same websocket" — a long-lived idle connection is the normal state here, which is precisely when this bites. Missing read limit is the same line of code: `sub.conn.ReadMessage()` (`:488`) with no `SetReadLimit` will allocate whatever the peer sends. **Fix**: `conn.SetReadDeadline` refreshed by a `SetPongHandler`, a ping ticker in `handleWriter`, and `conn.SetReadLimit` sized to the largest acceptable SBOM. --- ##### F8 — HIGH — Transient scanner-side failures are recorded as permanent for a full rescan interval **SUSPECTED.** `scan_broadcaster.go:650-690` (all errors treated identically), `:1218` (stale loop's only exemption is `skipped`), default `rescan_interval` 168h (`docs/SBOM_SCANNING.md`). Everything the scanner can fail on becomes one `NewFailedScanRecord` with a free-text reason and no retry classification. Once written, the manifest *has* a scan record, so `discoverUnscannedForUser` (`:1112`) skips it and only the stale loop will ever look at it again — after `rescanInterval`, a week by default. Transient failures wrongly made week-long: - `"scanner queue full"` (`scanner/internal/client/hold.go:166`) — a pure backpressure signal, emitted precisely when the hold is over-dispatching (F4). The hold's own overload gets written into the user's scan history as a failure and suppresses retries for a week. - Blob download failures — a 5xx from S3 or an expired presigned URL during layer fetch. - Grype DB download/update failures on scanner start. - OOM or disk-full in `vuln.tmp_dir`. Deterministic failures that will be retried forever, roughly weekly, each costing a full image download plus Syft plus Grype: - Image larger than `vuln.max_image_size` (2 GiB default) — this is a `SendError`, not a `SendSkipped`, so it is retried indefinitely and will always fail. - A manifest whose blobs have been garbage-collected — the record persists after the blobs are gone. - Any malformed or unsupported artifact the scanner errors on rather than skipping. The dfd604b commit message describes exactly this class: an in-toto attestation whose config mediaType looks like an ordinary image config. Note also the asymmetry with `pkg/hold/pds/scan.go:126-133`, where the backfill writes `NewFailedScanRecord("backfilled: legacy record (no SBOM and zero counts)")` — so every legacy record on a backfilled hold is now in the perpetual weekly-retry set too. **Fix**: give the boundary a retryable/permanent distinction. Cheapest version: a `retryable bool` on `ErrorMessage`, with the scanner setting it false for size limits and unsupported artifacts; hold side, a permanent failure is treated like `skipped` by the stale loop, and a retryable one gets a short backoff rather than the full rescan interval. `ScanRecord.Reason` already exists to carry the explanation to the UI. --- ##### F9 — MEDIUM — A result with no summary is silently discarded and the job marked complete **SUSPECTED.** `scan_broadcaster.go:599` and `:624` Independent of the panic in F1: the `if msg.Summary != nil` guard means a summary-less result writes **no scan record at all**, yet `:624` still marks the row `completed` and `:635` releases the inflight mark. The SBOM and vuln report blobs were already uploaded at `:575`/`:588`, outside the guard, so they are now orphaned in S3 with nothing referencing them. The manifest ends up with no record, so discovery re-queues it on the next 4-hour pass, scans it again, discards it again — forever, at full scan cost, with the only trace being a growing set of orphaned blobs. **Fix**: treat a missing summary as an error (write a failed record) or as a zero summary, but never as a success. An SBOM without Grype counts is a legitimate outcome when `vuln.enabled=false` and arguably deserves its own record shape. --- ##### F10 — MEDIUM — Disconnect/reconnect produces duplicate concurrent scans of the same job **SUSPECTED.** `scan_broadcaster.go:383-391` (Unsubscribe requeue), `:351` (drain on reconnect), `scanner/internal/client/hold.go:46-68` (the scanner's queue outlives the connection). On any read or write error the hold flips that subscriber's `assigned` *and* `processing` rows back to `pending`. The scanner, meanwhile, loses only the connection: `connectOnce` returns, `Connect` sleeps 5s and redials, but the `queue.JobQueue` and the worker pool are owned by main and keep running. Jobs already in the local queue are still scanned, and their results are written to the dead `c.conn` — `sendJSON` (`hold.go:213`) logs the write error and drops the message on the floor. So after a reconnect: the hold re-drains the same `seq` to the same scanner, the scanner enqueues a second copy of a job it may still be running, and the image is scanned twice concurrently on one host. With `workers: 1` the duplicate is serialised behind the original, doubling the backlog and feeding F4. Is `handleResult` idempotent for a repeated `seq`? Nearly. The blob uploads are content-addressed (`profile.go:80-93`), so re-uploading writes the same key. `CreateScanRecord` upserts on the digest-derived rkey (`pkg/hold/pds/scan.go:178-187`), so no duplicate record. But each repeat costs two full S3 PUTs and a fresh CAR commit with a new CID and a firehose event, all under the repomgr lock, on the reader goroutine (F6). It is safe, not free. Also note the ordering hole this opens: `Unsubscribe` takes `sb.mu` and runs its requeue UPDATE, but the *already running* `drainPendingJobs` goroutine for that same subscriber does not hold `sb.mu` and does not check membership, so it can assign further rows to the dead subscriber id after the requeue has swept. Those rows sit `assigned` to a ghost until the 5-minute ack timeout. **Fix**: on reconnect the scanner should drop or re-key its outstanding queue, or the hold should include an epoch/lease token in the job and reject results whose lease has been revoked. Minimally, `drainPendingJobs` should re-check subscriber membership before each assign. --- ##### F11 — MEDIUM — `scan_jobs` is never pruned, and the capacity query scans it every 5 seconds **SUSPECTED.** `scan_broadcaster.go:1566-1570`, schema at `:257-277`. No code anywhere deletes from `scan_jobs` (grep for the table name returns only `scan_broadcaster.go` and its tests). Every push and every proactive scan for the life of the hold is a row. The stall test's own comment acknowledges the scale — "the table holds tens of thousands of them" (`scan_broadcaster_stall_test.go:71`). Against that table, `waitForCapacity` runs `hasActiveJobs()` in a loop with a 5-second fallback tick (`:1348`), and the query is: ```sql SELECT COUNT(*) FROM scan_jobs WHERE status IN ('assigned','processing') OR (status = 'pending' AND datetime(created_at) > datetime('now', ?)) ``` The `datetime(created_at)` call is not sargable, and the `OR` limits what `idx_scan_jobs_status` can do. The two indexes present (`:275`, `:276`) cover `status` and `(assigned_to, status)`; there is no index supporting `status='processing' AND assigned_at < ?` (`:836`) or the `ORDER BY seq` reclaim scan (`:849`) beyond the status prefix. **Fix**: a retention sweep for terminal rows older than some window (they are pure history; the scan *records* are the durable artifact), plus an index on `(status, assigned_at)` and `(status, created_at)`. --- ##### F12 — MEDIUM — Every hold walks every ATCR user's PDS every four hours, and on every scanner reconnect **SUSPECTED.** `scan_broadcaster.go:1010` → `:1041` → `:1067`, and `:354`. `fetchManifestDIDs` asks the relay for *all* DIDs with an `io.atcr.manifest` record (`:1047`) — network-wide, not hold-scoped. `discoverUnscannedForUser` then resolves each identity and paginates that user's entire manifest collection 100 at a time (`:1079`), and only *after* unmarshalling each record does it ask whether the manifest belongs to this hold (`:1097`). Every hold in the network therefore performs a full read sweep of every ATCR user's PDS, every four hours, to find its own manifests. `Subscribe` calls `triggerDiscovery()` unconditionally (`:354`), and the scanner reconnects every 5 seconds while it cannot connect (`scanner/internal/client/hold.go:66`). `discoverNow` has depth 1 so triggers coalesce, but a pass that ends while a trigger is pending starts another immediately — a crash-looping scanner turns discovery into a continuous loop with no rate limit. For each manifest that passes the filter, `GetScanRecord` (`:1112`) is a carstore read; for each *unknown hold DID* seen, a 5-second HTTP probe (F5). The project already knows third-party PDSes have spam daemons (`project_pds_spam_suspension`). These are reads, not writes, but they are unbounded, unthrottled, and multiplied by the number of holds. **Fix**: rate-limit `runDiscoveryPass` (a minimum interval between passes regardless of trigger); skip users whose manifests have never named this hold, cached across passes; and prefer the hold's own `io.atcr.hold.layer` records — which already join manifest AT-URIs to this hold — as the candidate source, falling back to the relay only for predecessor adoption. --- ##### F13 — MEDIUM — `resolveManifestForCandidate` walks a user's entire PDS for one digest, under a shared 30-second budget **SUSPECTED.** `scan_broadcaster.go:1412-1453`, budget set at `:1356`. Every stale candidate arrives with only a digest (`:1235-1239`), so the manifest must be re-resolved. The resolution is a linear scan: page through `io.atcr.manifest` 100 at a time and compare `manifest.Digest` (`:1437`). For a user with 2,000 manifests that is 20 sequential PDS round-trips to find one record whose rkey is *already known* — the rkey is the digest hex (`BuildManifestURI`, `lexicon.go:539`), so `com.atproto.repo.getRecord` would fetch it in one call. The 30-second context at `:1356` covers the staleness re-check, identity resolution and the whole walk. On a large repo it expires, `ListRecordsForRepo` errors, the function returns false at `:1428`, the candidate is dropped with a `Debug` log, and the inflight mark is released — so it comes back on the next stale pass and fails the same way, indefinitely. Large accounts silently stop being rescanned. The same walk is the cost of *every* deleted manifest that still has a scan record: `:1449` is reached only after paginating the whole collection. **Fix**: `getRecord` by rkey directly; fall back to the walk only on 404. Widen or remove the 30s cap on the resolution step. --- ##### F14 — MEDIUM — Strict priority starves the stale queue, and a blocked stale pass holds inflight marks **SUSPECTED.** `scan_broadcaster.go:1300-1320` (strict priority), `:1241-1247` (blocking send). `dispatchLoop` drains `unscannedQueue` entirely before ever looking at `staleQueue`. Discovery re-queues every never-scanned manifest on every 4-hour pass, so on a hold with a persistent unscanned backlog — which is exactly a hold whose scanner is slow, i.e. F4's hold — rescans never run at all. The `rescan_interval` becomes advisory. Meanwhile `runStalePass` blocks in `case sb.staleQueue <- candidate:` with only `stopCh` as an alternative, so it parks indefinitely once the 200-slot queue fills. It parks holding an inflight mark for the candidate in hand plus the 200 queued, and the pass never completes, so its `found > 0` summary log (`:1256`) never prints. From the outside the stale loop simply looks absent. `discoverUnscannedForUser` has the same unbounded blocking send at `:1127`, which also means the 30-minute discovery context at `:978` is fiction — the send does not select on it. **Fix**: age-weighted priority instead of strict priority (or a simple 4:1 ratio), and make both queue sends select on the pass context so a pass ends when its budget does. --- ##### F15 — MEDIUM — The scan record rkey is the digest alone, so two users pushing the same image share one record **SUSPECTED.** `pkg/atproto/lexicon.go:829` (`ScanRecordKey` = digest hex), `:774-776` (`Manifest` AT-URI is built from the *job's* userDID). `CreateScanRecord` upserts at `rkey = ` per hold. Container digests are content-addressed and shared: two users pushing the same base image, or one user pushing to two repositories, collide. Whichever scan lands last overwrites `repository`, `userDid` and the `manifest` AT-URI. Consequences: 1. The record served for user B's digest names user A's DID and repository name. The appview only renders counts, so nothing leaks in the UI, but the record itself is public on the hold's PDS via `com.atproto.repo.getRecord` (`docs/SBOM_SCANNING.md`), which discloses that A pushed that image and under what repository name. 2. Stale rescans of a shared digest are dispatched against `scanRecord.UserDID` (`:1238`). If that user has since deleted the manifest, `resolveManifestForCandidate` walks their PDS, does not find it, and drops the candidate (F13) — even though another user still has it. The digest is then unrescannable while the record still claims it was scanned. 3. `sb.inflight` is keyed by digest too (`:1611`), so a push of a shared digest by user B is deduped against a proactive scan for user A — correct for scan cost, but it means B's push-triggered scan silently does not happen. **Fix**: this is a schema decision, so flag rather than change casually. Either accept it and document that scan records are hold-scoped facts about a digest (in which case `repository`/`userDid`/`manifest` should be dropped or made a list), or key the rkey on `hash(userDID + digest)` and accept duplicate scan work. --- ##### F16 — MEDIUM — `handleError` and `handleSkipped` leak an inflight mark when their lookup fails **SUSPECTED.** `scan_broadcaster.go:653-683` and `:698-728`. ```go var manifestDigest, repository, userDID string err := sb.db.QueryRow(...).Scan(&manifestDigest, ...) if err != nil { slog.Error("Failed to get job details for failure record", ...) } else { ...write record... } ... sb.removeInflight(manifestDigest) // :683 — manifestDigest is "" on the error path ``` On the error branch `manifestDigest` is still the zero value, so `removeInflight("")` deletes a key that was never inserted and the real digest stays in the map forever — the F3 exclusion, by a different door. `handleResult` avoids it only because it `return`s early at `:569` — which leaves the row in `processing` instead, to be failed by the 10-minute sweep, which is F3 proper. The lookup fails if the row was deleted (nothing deletes rows today) or on any DB error, e.g. a `SQLITE_BUSY` past the 5-second timeout during a heavy CAR commit. **Fix**: carry the digest on the job in memory (it is already in `ScanJobEvent`) rather than re-reading it, or track inflight by `seq` and translate once. --- ##### F17 — MEDIUM — `hasActiveJobs` fails closed forever on a DB error **SUSPECTED.** `scan_broadcaster.go:1571-1574` ```go if err != nil { slog.Error("Failed to check active scan jobs", "error", err) return true // Assume busy on error } ``` `waitForCapacity` spins on this. A persistently failing query — a corrupted index, a locked DB, a closed connection after `Close()` on a shared handle — stops proactive scanning for the life of the process, with one error line per five seconds and no other signal. `logStalledCapacity` (`:1580`) will not fire either, because it runs the same DB and takes its own error branch at `:1592`. This is the same silent-indefinite-halt shape as the nine-day outage dfd604b fixed. **Fix**: fail open after N consecutive errors, and surface the condition on the health endpoint rather than only in logs. --- ##### F18 — MEDIUM — Reader, writer and drain goroutines are not tracked, so `Close()` can pull the DB out from under them **SUSPECTED.** `scan_broadcaster.go:913-922` (Close), `:345-351` (untracked goroutines). `sb.wg` covers only the four background loops (`:177`, `:182`). `handleWriter`, `handleReader` and `drainPendingJobs` are bare `go` statements. `Close()` closes `stopCh`, waits for the loops, and then closes the DB when `ownsDB`. Any in-flight `handleResult` is then doing S3 uploads and `db.Exec` against a closed handle — errors, a half-written result, and in the `NewScanBroadcasterWithDB` case (which is the production path, `pkg/hold/server.go:233`) a shared handle whose lifecycle is owned elsewhere. Nothing closes the subscriber connections on shutdown either, so the goroutines are only released when the peer notices — which, per F7, may be a long time. Related, minor: `Close()` closes `stopCh` unconditionally, so a second call panics (`:915`). **Fix**: add the three goroutines to the WaitGroup, close subscriber conns in `Close`, and guard `stopCh` with `sync.Once`. --- ##### F19 — MEDIUM — Relay failover is poisoned by the shared context **SUSPECTED.** `scan_broadcaster.go:1010-1037`, ctx from `:978`. `fetchManifestDIDs` loops over relays passing the same `ctx` to each. That ctx is the 30-minute discovery budget. If the first relay is not down but *slow* — the more common failure — it can consume the entire budget, at which point every subsequent relay attempt fails instantly with the already-expired context, `:1035` logs "all relays failed", and the pass returns nothing. Failover buys nothing in the case it matters most. `fetchManifestDIDsFrom` also accumulates every DID in memory with no bound (`:1044`, `:1053`) and no cursor-progress check, so a relay that returns the same cursor repeatedly spins until the context expires. **Fix**: a per-relay sub-context (say 2 minutes), and a guard that the cursor advanced. --- ##### F20 — MEDIUM — `Enqueue` ignores `addInflight`, creating duplicate rows for one digest **SUSPECTED.** `scan_broadcaster.go:293` ```go sb.addInflight(job.ManifestDigest) // bool return discarded ``` `addInflight` exists precisely to answer "is this digest already queued or being scanned" (`:1610`), and both proactive producers honour it (`:1107`, `:1204`). `Enqueue` does not. A push arriving for a digest the proactive loop is already scanning inserts a second row and dispatches a second job. Both scan; both call `CreateScanRecord` (upsert, so one record); the first to complete calls `removeInflight`, after which discovery is free to queue a *third* copy while the second is still running. Whether Enqueue should refuse or proceed is a judgement call — a push-triggered scan is arguably more urgent than the proactive one already running. But silently discarding the signal is not a judgement, and the round-trip is expensive. **Fix**: if already inflight, either skip the enqueue (log it) or cancel/supersede the proactive job. At minimum, record the collision. --- ##### F21 — MEDIUM — No back-channel for cancellation; the protocol has no way to say "stop" **SUSPECTED.** `scan_broadcaster.go:86-110` vs `scanner/types.go:23-95`. Message inventory. Hold → scanner: `job` only. Scanner → hold: `ack`, `result`, `error`, `skipped` — all four are handled at `:507-514`, and the scanner sends nothing else. The wire shapes match field-for-field (`ScanJobEvent` vs `ScanJobRaw`; `BlobReference`'s JSON tags at `lexicon.go:132-141` match `BlobDescriptor` at `scanner/types.go:39-43`). So no message is unhandled in either direction. The gap is the missing message. Every point at which the hold gives up on a job — `Unsubscribe`'s requeue (`:383`), the ack timeout (`:888`), the processing timeout (`:834`) — is invisible to the scanner, which keeps burning CPU and disk on work whose result will be redundant (F10) or arrive after the row was already failed (F3/F4). **Fix**: a `cancel {seq}` message from hold to scanner, and a lease/epoch on the job so the hold can reject results it no longer wants. --- ##### F22 — LOW — `Subscribe` reads `len(sb.subscribers)` outside the lock **SUSPECTED.** `scan_broadcaster.go:342` The mutation at `:335` is inside `sb.mu`; the log statement at `:338-342` is after the `Unlock` and reads the slice header again. Concurrent `Subscribe`/`Unsubscribe` makes it a genuine data race, reportable by `-race`. Today the count is only wrong in a log line, but a racing slice header read is undefined behaviour, and `Unsubscribe` reslices the same backing array at `:367`. The existing tests do not catch it because they never call `Subscribe` concurrently. **Fix**: capture the count inside the critical section. --- ##### F23 — LOW — `assigned_at` is stored as a local-offset RFC3339 string and compared lexicographically **CONFIRMED by execution.** A scratch program using the project's pinned `github.com/tursodatabase/go-libsql v0.0.0-20260424063416-3051e37e6e04` shows: ``` row 1 type=text assigned_at="2026-09-05T11:27:03.069534699-05:00" created_at="2026-09-05T16:27:03Z" rows older than cutoff (want '2'): 2 # comparison works in a stable offset created_at within 15m (want 2): 2 # datetime() parses both formats ``` So `assigned_at` (bound as a Go `time.Time` at `:421`, `:778`) is stored as TEXT with the *host's local UTC offset*, while `created_at`'s `CURRENT_TIMESTAMP` default is normalised to `...Z`. The comparisons at `:836` and `:847` are string comparisons between two values formatted the same way, so they are correct as long as the offset never changes. They stop being correct across a DST transition on a non-UTC host: after a fall-back, rows stamped `01:59:00-05:00` compare *greater* than a cutoff computed as `01:10:00-06:00`, so timed-out jobs are not reclaimed for an hour; at spring-forward the error runs the other way and healthy jobs are reclaimed an hour early. Production containers are typically UTC, which is why this has not bitten. **Fix**: store `assigned_at` as `time.Now().UTC()` (or as a Unix integer) and compare the same way, matching `created_at`. --- ##### F24 — LOW — Rows with a NULL `user_handle` are permanently undispatchable and silently skipped **SUSPECTED.** `:757-765` (drain) and `:866-873` (reclaim) scan `user_handle` into a plain `string`; `handleResult` at `:562` uses `COALESCE(user_handle,'')` and the column is nullable (`:263`). `Enqueue` always binds a value, so today's rows hold `''` rather than NULL and the question is only about rows from an older schema or an out-of-band insert. But if such a row exists, both dispatchers fail `rows.Scan` and `continue` — the drain logs `Failed to scan pending job row` at `:763`, the reclaim logs nothing at all (`:872`). The row is pending forever, never dispatched, never reported. The asymmetric COALESCE in `handleResult` suggests someone has seen a NULL here. **Fix**: `COALESCE(user_handle,'')` in all three queries, or `NOT NULL DEFAULT ''` on the column. --- ##### F25 — LOW — The `cursor` parameter is dead, and harmful if ever used **SUSPECTED.** `pkg/hold/pds/xrpc.go:1093-1102`, `scan_broadcaster.go:744`, `scanner/internal/client/hold.go:47`. The scanner initialises `cursor := int64(-1)` and never assigns it, so the query parameter is never sent and the hold's default of `-1` makes the drain's `WHERE ... AND seq > ?` a no-op. Fine today. But the semantics are wrong for what it looks like. `scan_jobs.seq` is not an event sequence; it is a job identity, and pending jobs are not monotonic in it (a row reclaimed by `Unsubscribe` keeps its old low seq). If the scanner ever started sending its last-seen seq — the obvious reading of the name, and what the sibling firehose `Subscribe(conn, cursor, userAgent)` at `xrpc.go:1072` actually means — every reclaimed older job would become permanently undrainable. **Fix**: remove the parameter from this endpoint, or rename it and document it as a lower bound on job identity, not a replay cursor. --- ##### F26 — LOW — `handleAck` neither verifies its effect nor refreshes the lease **SUSPECTED.** `scan_broadcaster.go:525-539` The UPDATE is correctly guarded on `assigned_to = ? AND status = 'assigned'`, but `RowsAffected` is discarded and the code logs `"Scan job acknowledged"` unconditionally. An ack for a job that was reclaimed, reassigned, or belongs to another subscriber (F2's scenario) is indistinguishable in the logs from a successful one. Given that the ack is the only positive confirmation the hold ever gets that a scanner has the work, that is the single most useful thing to count. See also F4 for the lease-refresh half of this. --- ##### F27 — LOW — `dispatchJob`'s buffer-full reset is unguarded **SUSPECTED.** `scan_broadcaster.go:445` ```go UPDATE scan_jobs SET status='pending', assigned_to=NULL, assigned_at=NULL WHERE seq = ? ``` No `AND assigned_to = ?`. Under F2's race, where another dispatcher already handed the row out, this resets a row that is legitimately assigned elsewhere. Also, the send attempt uses `default:` (`:441`) — a strictly non-blocking try — so a scanner whose 20-deep buffer is momentarily full loses the job to a full requeue cycle rather than waiting a few milliseconds. --- ##### F28 — LOW — Orphaned SBOM and vuln blobs are created and never referenced **SUSPECTED.** `:575`, `:588`, versus the record write at `:607`. Both blobs are uploaded before the record is written, and the record write can fail (`:608`) or be skipped entirely (F9) while the job is still marked completed. The resulting S3 objects are unreferenced by any record. `pkg/hold/gc` sweeps orphaned records but I did not find a sweep for ATProto blobs whose referencing record was never written; worth confirming before sizing this. **Fix**: write the record first with the blob refs computed locally (the CID is a pure function of the bytes, `profile.go:80-90`), or upload after a successful record write. --- ##### F29 — LOW — Proactive jobs lose tier and tag **SUSPECTED.** `scan_broadcaster.go:1396-1402` Every proactive job is hard-coded `Tier: "deckhand"` and carries no `Tag`. The scanner's priority queue maps that to the lowest priority (`scanner/internal/queue/priority_queue.go:29`), so a paying customer's weekly rescan queues behind every free-tier discovery job. The missing tag degrades the scanner's logs, which are the only place a job is human-identifiable. The owner DID and crew tier are both knowable at dispatch time (the hold has the crew records locally). --- ##### F30 — LOW — Discovery's scannability filter disagrees with the push path's **SUSPECTED.** `:1102` versus `pkg/hold/oci/xrpc.go:238`. Discovery skips a manifest when `len(Layers)==0 || Subject != nil || Config == nil`. `HasScannableContent()` skips when `IsMultiArch() || IsReferrer()` — it does not require a config. So a manifest with layers but no config is enqueued on push and never proactively; and conversely, `HasScannableContent` has no layer-count check, so a single-arch manifest with zero layers is enqueued on push and will fail or skip at the scanner. One predicate, used by both, would remove a class of "why did this scan / why didn't it" questions. --- #### Section 3: improvements that are not bugs **Observability.** The pipeline has no counters at all — everything is inferred from `slog` lines. The cheapest high-value additions, all derivable from data already in hand: - Ack→result latency distribution. This is the single number that would settle F4's sizing, and nothing records it today. - Counts by terminal transition, split by producer: `completed` via result, via skip; `failed` via scanner error, via the 10-minute sweep. The last of those is currently invisible except as one `Warn` with a count (`:841`). - Size of `sb.inflight` and depth of both queues, exported on the health endpoint. An inflight set that only grows is the signature of F3/F16 and is currently unobservable. - The scan-jobs table by status, on the admin UI. The hold's admin panel has a scan backfill page (`pkg/hold/admin/handlers_scan.go`) but nothing that shows the live queue. **Structure.** - `scan_broadcaster.go` is 1,642 lines carrying five separable concerns: the WebSocket subscriber lifecycle, the SQLite job store, the result-handling side effects, the three discovery loops, and the predecessor probe. The job store in particular deserves its own type with the status transitions as methods — most of Section 1's asymmetries (guarded UPDATE here, unguarded there; record written here, not there) are the kind that disappear when there is exactly one function per transition. - The predecessor probe is now duplicated between `scan_broadcaster.go:1480` and `pkg/hold/gc/gc.go:2037`, and they have diverged (F5). One implementation, in a shared package, with GC's definitive/unresolved contract. - `handleResult`/`handleError`/`handleSkipped` share the same three-step shape (look up the job, write a record, close the row) with three different treatments of the lookup failure. Unifying them would fix F16 and F9 as a side effect. **Tests worth adding later** (none written here): - `handleResult` with `msg.Summary == nil` — currently panics (F1). This is the one to write first; it is three lines. - `drainPendingJobs` against a row claimed between the SELECT and the UPDATE — the mirror of the existing `TestScanDispatchJob_SkipsClaimedJob` (F2). - The 10-minute processing sweep: assert a scan record exists afterwards and that the digest left `sb.inflight` (F3). Both assertions fail today. - `Unsubscribe` racing an in-flight `drainPendingJobs` *assign* (not just the send the existing test covers) — assert no row ends up assigned to a departed subscriber (F10). - `checkPredecessor` against a hold that times out, asserting the answer is not cached (F5). `pkg/hold/gc/predecessor_test.go` is a ready-made template. - A `-race` test that calls `Subscribe` and `Unsubscribe` concurrently (F22). - Round-trip of the `assigned_at` comparison across a simulated offset change (F23). **Documentation.** `docs/SBOM_SCANNING.md:148-150` describes the timeouts accurately but omits that the 10-minute sweep writes no scan record, so a reader would reasonably believe a failed job shows as "Scan failed" in the UI. It also does not mention that a disconnect re-dispatches work the scanner may still be running. Both are worth a sentence once F3 and F10 are decided.