at-container-registry/SCANNER_BUGS.md

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 errors 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:

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:

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.Skipped 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 '<name>' -v. Every test in that file passes (one is t.Skipped, 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:

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.Creates 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 <Vuln.TmpDir>/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/<algorithm>/<hex> 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

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.

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 <Code>AccessDenied</Code><Message>Request has expired</Message>, 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/<algorithm>/<hex> 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.Skipped 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

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.

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:

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:

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/<xx>/<hash>/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/<hex> while the manifest descriptor still says sha512:<hex> (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 <shared secret> 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 <repository>:<tag>@<manifestDigest> 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=<dead sub> 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

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

_, 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

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 continues — 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

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:

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 = <digest hex> 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.

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 returns 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

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

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

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.