TestCarstoreConcurrentWritesTwoOpeners failed in CI on 2026-09-13 with
"database is locked" while the code was correct. It hammered the file with
five concurrent writers and asserted that no lock error surfaced within the
5 s busy_timeout, which on a loaded runner with every package testing in
parallel is a statement about the disk, not the code. The property it guards
(busy_timeout applied to every connection of every pool on a hold database,
c44a874) is now tested directly: one connection holds the write lock via
BEGIN IMMEDIATE, a second writer is shown to block rather than fail, and to
succeed once the lock is released. Both topologies are covered (the shared
OpenHoldDB pool, and a second opener on the same file, in both directions),
and a control shows a pool without busy_timeout fails immediately under the
same lock, so the passing tests are known to observe the mechanism.
The failure was also buried under the INFO lines every hold and PDS test
emits while booting. internal/testlog.Quiet swaps the default slog handler
for a discard handler unless the run is verbose or ATCR_TEST_LOGS is set,
and every package that produced that output now calls it from TestMain.
`go test` only shows a package's output when it fails, so this changes
nothing for passing runs and leaves a failing one readable.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Hho5da4daoCoPBJ9tCrL7s
`go fix` carries the modernize analyzers now, and the tree had drifted behind
them. This is the mechanical result, reviewed rather than trusted: the tool is
capable of rewriting code into something that no longer tests or does what it
did, so every non-test change was read individually and the concurrency-bearing
packages were re-run under -race.
Production code, four changes, all semantics-preserving:
- leases/manager.go: wg.Add(1) + go + defer wg.Done() becomes wg.Go. The
comment above that function turns on Add happening before the goroutine
starts, so that a Wait cannot return before the worker has run. wg.Go does
the Add synchronously on the calling goroutine, so the invariant it
describes still holds.
- auth/token/handler.go: strings.Fields -> strings.FieldsSeq, same splitting,
iterated rather than allocated.
- hold/gc/gc.go: a hand-written map copy -> maps.Copy.
- hold/pds/scan_broadcaster.go: three-clause loop -> range over int.
The rest are tests. The one worth naming is carstore_contention_test.go, where a
careless rewrite could have quietly stopped exercising contention: go fix
converted the reader and side-table goroutines to loopWG.Go but correctly
declined to touch the writer loop, which passes its index as a parameter. The
writer/reader/side-table shape and the stop channel are unchanged, so the test
still contends over the same carstore transactions.
Verified: go build for hold and appview, `make lint` 0 issues, the deploy and
credential-helper modules 0 issues, `make test` green across all 43 packages,
and -race green on leases, hold/pds, hold/gc and auth/token. The scanner module's
two lint findings are unchanged from HEAD and are in files go fix never touched.
Kept separate from the HTTP/2 commit so that one stays readable, and so this can
be reverted on its own if a modernization turns out to matter.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TA9D4DjaLZTvzQ7dJbu4eg
The aux-record sweep decided whether a record was old enough to collect from a
timestamp inside the record body, which for scan records is scannedAt. Every
rescan rewrites that, so the grace clock reset continuously and a hold with a
rescan_interval shorter than the grace period could never collect an orphaned
scan record at all.
Grace now measures how long a slot has been continuously observed orphaned
across analysis passes. The key deliberately excludes the CID so a rescan
rewriting the record in place does not restart the clock, and the check runs
last, after the manifest, reachability and co-ownership tests, so a record
judged live never accrues orphan age. A pass that errors partway commits
nothing rather than resetting the clock on slots it never reached.
The clock is in memory, so a restart forgets every observation and each
surviving orphan starts its grace again. That delays collection and can never
advance it, which is the safe direction, but it does mean a hold restarting
more often than the grace period will not collect aux orphans. Persisting it
wants a table in pkg/hold/db and is left for later.
Applied to both aux collections rather than only to scan records. For image
configs the new rule is strictly more conservative, since a record cannot be
observed before it is written, and a per-collection table of which timestamps
are safe to trust is a thing to maintain and to get quietly wrong.
One widening beyond the reported bug, called out rather than left to be found:
a record whose body timestamp is unparseable used to be kept forever, because
the zero time read as in-grace. It is now collected on the normal schedule,
having cleared every other guard plus a full grace period.
Also corrects the comment claiming these records hold no blob references. Scan
records carry sbomBlob and vulnReportBlob; they live under a prefix the blob
sweep never walks, so deleting the record frees nothing, and a future sweep for
that space must read those fields before the record goes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
Both paginated walks bailed on the first error, and the caller treats a
failed walk as "assume everything is referenced", so one blip skipped that
DID's storage for the whole run.
Measured before changing anything: every DID GC had classified unreachable
but healthy failed only one or two runs out of six, and replaying the exact
same listRecords calls afterwards returned 200 in 45-680 ms with no rate
limiting. Ordinary blips on small self-hosted PDSes, amplified into a
full-DID skip.
Share one listRecordsPage helper between fetchUserTags and
fetchUserManifestsFromEndpoint. The retry decision splits deliberately:
timeouts, connection reset, 5xx and 429 get another attempt, while DNS
failure, TLS failure, connection refused and any 4xx do not. Those are
stable facts about an endpoint, and retrying them would only slow the run
and keep a dead PDS looking alive longer. Unrecognised errors stay
permanent, so an unfamiliar failure degrades to today's behaviour rather
than hammering.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
264d332 fixed the behaviour but encoded it badly. manifestBelongsToHold
returned (true, false) for an unreachable hold — "yes, but not really" — a
return value that contradicts itself, and isPredecessorHold both applied the
fail-open policy and handed back the raw material for that policy. The
behaviour was right and the shape was wrong.
The underlying problem is that ownership has three states and the return type
had two:
ours - this hold's manifest, or a confirmed predecessor's
not ours - the hold answered, and it is someone else's
unknown - the hold did not answer; don't delete, but do not adopt
For the first two, "is it ours" and "should its blobs stay referenced" have
the same answer, so one bool worked and the design was never stressed. They
diverge only on unknown. Every version so far has had to collapse unknown
onto one of the other two: before 95d4f7c onto "not ours", which deleted a
live predecessor's blobs, and after it onto "ours", which adopted foreign
manifests and put ten phantom missing layer records on hold01. Same shape
error, opposite sides. That conflation is original, not something 95d4f7c
introduced: manifestBelongsToHold has fed knownManifests since the function
was written.
So name the state. manifestClaim has three values, classifyManifest and
classifyPredecessorHold report what they found and apply no policy, and the
one decision that matters — an unknown claim is carried for blob protection
but never adopted — now sits in the open at the call site instead of two
functions deep, which is how it leaked into ownership to begin with.
No behaviour change from 264d332; the three outcomes and the blob protection
are identical. The regression test was re-verified against this shape: it
fails, reporting the adoption, when claimUnknown is allowed to adopt.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
95d4f7c made the predecessor check fail open so a five-second blip against
a live predecessor could not drop its blobs out of the referenced set. That
was right, but the boolean it flipped does two jobs: manifestBelongsToHold
decides both "keep these blobs referenced" and "this manifest is ours", and
for an unreachable hold those want different answers.
The consequence showed up on hold01 the moment it started running this
code. Five stale dev manifests pointing at did:web:localhost%3A8080 and
did:web:172.28.0.3:8080 were adopted into knownManifests, and since hold01
had never stored them, every one of their ten layers was reported as a
missing layer record. Worse than the noise: reconcileMissingRecords acts on
exactly that list, so a Reconcile would have written io.atcr.hold.layer
records asserting hold01 stores blobs for a localhost hold.
These DIDs are loopback and RFC1918, so they can never resolve from a
server. This is not a transient outage that clears itself on the next run.
manifestBelongsToHold and isPredecessorHold now return (value, definitive),
matching the idiom checkPredecessor already uses. An indefinite answer still
carries the manifest so its blobs stay referenced, but marks it ProtectOnly,
and analyzeRecords protects its digests without adding it to knownManifests
— the same shape the in-grace takedown branch above it already uses.
Left alone deliberately: the legacy holdEndpoint path still treats a resolve
failure as a definitive "not ours". That predates 95d4f7c and fails closed
rather than open, so it is a different bug with a different blast radius.
The regression test was verified to fail without the fix, reporting the
adoption rather than a build error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KWoKzpgtBJ33sCyGxJGR7x
deleteOrphanedBlobs is the only part of GC that removes bytes and it had no
test. It could not have had one: object age decides whether a blob is
deletable, the in-process harness stamps every object with time.Now(), and
MockS3Client's ListObjectsV2 set no LastModified at all. Neither could express
"this blob is nine days old", so both halves of the grace rule went
unexercised — including the half that protects a push still in flight.
MockS3Client gains ObjectTimes, a per-key LastModified consulted by
ListObjectsV2. Keys with no entry list without a timestamp exactly as before,
so existing tests are unaffected. It also gains DeleteObjectError, matching
the error injection the other operations already had.
Four cases, three of which are reasons NOT to delete: an old unreferenced
blob goes; a young unreferenced blob stays; a referenced old blob stays; a
/link object is never treated as a blob. Plus a failure case pinning that one
undeletable object does not abort the walk, and that a blob which never left
storage is not counted as deleted or reported as reclaimed space.
Verified by mutation. Disabling the grace check deletes the young blob;
disabling the referenced check deletes the live one; both fail. Disabling the
/data suffix check changes nothing, because extractDigestFromPath anchors on
/data$ and rejects everything else — so that check is a redundant early-out
rather than a guard, and the test says so rather than implying otherwise.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fetchUserManifestsFromEndpoint, fetchUserTags and fetchUserProfile
interpolated the user DID straight into a query string. A did:web carrying a
port spells that port as a literal %3A, so the PDS received the parameter
decoded back to ":" — a different DID, matching no repo. listRecords then
answers 200 with an empty list and GC reads it as "this user has no
manifests": every blob they own drops out of the referenced set and is
deleted once past the seven-day blob grace.
There is no error and no status code to notice, which is the same soft
failure 95d4f7c fixed one function away in this file. getRecord for
manifests and checkPredecessorAt already escaped; three of the five call
sites did not.
Scope is local testing only. did:plc, which every production user has,
contains nothing that needs escaping, and did:web with a port is not really
valid in atproto — but it is what the dev stack runs on, so GC there sees a
referenced set of zero and considers every blob in the bucket collectable.
That made it impossible to validate the sweep end to end, which is how it
surfaced.
Found by test/integration/gc_test.go, added here: it pushes an image and
asserts GC accounts for every blob the push wrote. The blob grace period is
a package constant, so nothing pushed during a test can age past it and "no
orphans" is vacuous; the load-bearing assertion is referenced == total,
which grace does not touch. Against the unescaped code it reported
referenced=0 of 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2e55352 taught the scan broadcaster that a successor label is only
interesting when it points at us; the GC copy of the same check was left
accepting any non-empty successor. Confirmed still divergent at the head of
this stack: gc.go was a bare `if captain.Successor != ""` while
scan_broadcaster.go:1473 compares against sb.holdDID.
A hold that retired into some third hold is that hold's predecessor, not
ours, and its manifests are not a reason to keep blobs referenced here. GC
therefore now makes the same comparison the broadcaster does, against
gc.pds.DID().
This is the one change in the batch that makes GC delete more rather than
less, so it is deliberately its own commit and carries a floor. If this hold
cannot say who it is, ourHoldDID() returns "" and the old permissive answer
stands: we cannot conclude a successor is not us, and over-protecting merely
leaks blobs while guessing the other way destroys them. That branch has its
own test, because an empty DID silently turning every predecessor into a
stranger is exactly how this reconciliation would become the next blob-loss
bug.
The inconclusive-on-failure semantics 95d4f7c added are untouched: only the
answers from a hold that actually replied are affected.
Verified by mutation: forcing the comparison back to the permissive form
fails exactly one case, the successor naming a third hold, and leaves the
unknown-own-DID fallback passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
95d4f7c split the fetch-and-parse half of the predecessor probe into
checkPredecessorAt precisely so it could be tested against a local server,
but no test ever followed. The paths that had none are the ones that used to
delete another hold's blobs: a 500, a refused connection, a body that is not
JSON, and an envelope wrapping a garbage record all have to report
definitive=false, because only a definitive answer is allowed into the
process-lifetime predecessorCache.
Verified by mutation rather than by passing: flipping the six failure-path
returns in checkPredecessorAt to definitive=true, which is the pre-95d4f7c
semantic, fails all four cases.
Also pins the reset that the predecessorUnresolved field documents but
nothing enforced. Removing the clear at the top of analyzeRecords makes the
test fail, which is the point: without it one outage is permanent, every
later run short-circuits on the stale entry, and GC silently stops
reclaiming anything that hold's manifests touch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
checkPredecessor returned a bare false on every failure path (DNS failure,
dial error, non-200, 5s timeout, unparseable body), indistinguishable from a
hold affirmatively answering "I have no successor". isPredecessorHold then
cached that false in predecessorCache, which lives for the life of the
process and is never reset, so one blip during a single GC run permanently
unreferenced that hold's manifests. Those blobs are long past the 7-day
grace period that protects recent content, so the next run deleted them
outright with nothing to fall back on.
checkPredecessor now reports whether its answer is definitive, and only
definitive answers are cached. An inconclusive check keeps the hold's
manifests referenced and is recorded in predecessorUnresolved, which bounds
the cost to one timeout per run rather than one per manifest and is cleared
at the start of every analysis so a hold that was down once is re-checked
next time instead of written off.
This matches the convention the rest of the package already follows: a user
whose PDS cannot be reached has their records treated as referenced, never
as garbage. An outage must not be the reason content becomes deletable.
Non-200 counts as inconclusive on the same reasoning. A reachable service
that cannot produce its own captain record is malfunctioning, not answering,
and over-protecting an unrelated hold merely leaves some blobs unreclaimed.
Splits the fetch-and-parse half into checkPredecessorAt so it can be tested
against a local server without depending on DNS.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to f12d5e0. Extending the orphan sweep to io.atcr.hold.scan and
io.atcr.hold.image.config brought those collections under a delete path built
for layer records, and the two are not interchangeable. Layer rkeys are
generated per write, so a remembered rkey is a stable handle on one record.
Scan and image-config rkeys are atproto.ScanRecordKey(manifestDigest) — the bare
digest hex, with no DID — which is both reused across re-pushes and shared
between users who push identical images. "Delete rkey K" therefore stopped
meaning "delete the record I judged", and the sweep could destroy live data two
ways.
Both were confirmed by probe before being fixed, and each guard below has a
regression test that fails when that guard alone is reverted.
Shared records: two users pushing the same image collapse onto ONE record whose
body names only the last writer. When that user deleted their manifest the
record looked orphaned, and deleting it stripped the vulnerability scan and the
layer history/env/entrypoint from every co-owner's still-live image.
- knownDigests keeps a record while any successfully fetched user still holds
a manifest at that digest, not just the one the record names.
- knownDigests alone is not enough: it is built only from users we reached, so
a co-owner whose PDS was down this run is indistinguishable from one who
deleted their image. digestOwners closes that. It maps each digest to every
DID that pushed it here, derived from the hold's own layer records during
the walk analyzeRecords already performs, so it costs nothing extra and
needs no network. If any owner was unreachable, the record stays. This is
the sweep's existing principle — an unreachable PDS never causes a deletion —
extended from the record's named user to everyone the record serves.
Layer records are the right source because they are load-bearing for storage
accounting and billing, so they exist for anything a user is charged for.
Reused rkeys: a re-push upserts into the same digest-derived slot, so a record
marked orphaned by an earlier scan can be replaced by a LIVE one before the
delete runs. The admin delete button makes this wide — lastPreview is in-memory
for the life of the process, so an open tab keeps a stale scan actionable — and
doRun has the same race across its analyze-to-delete gap.
- Records now carry the CID they had when judged, and the delete re-reads the
slot to confirm it still holds that revision. Anything else (rewritten,
missing, unreadable, or a ref with no recorded CID) is skipped, not deleted.
- The CID check is record identity, not orphanhood, and identity alone is not
enough either. A re-push does not necessarily rewrite the SCAN record:
scan-on-push is tier-gated, and the broadcaster's discovery loop skips any
manifest that already has a scan record, so its CID survives a re-push
unchanged. manifestsWithLayerRecords covers that — a push writes layer
records to this hold, so their presence proves the manifest is back
regardless of what the scan concluded. Local, no PDS round trip. A manifest
whose layer records are themselves orphaned but not yet collected also lands
in the set, which only defers its aux record one cycle; one more round of a
leak beats deleting a live user's records.
- maxPreviewAgeForDelete (30m) refuses a stale preview outright. The
per-record checks are the real safety net; this stops the admin acting on a
picture that is hours old. Refusal surfaces through startBackground ->
lastError -> the polling progress fragment, so it is visible rather than a
silent no-op.
The four state maps auxRecordOrphaned consults are grouped into auxOrphanState.
Three are same-shaped maps that were trivial to transpose positionally, and
swapping knownDigests with fetchedUsers would have silently widened what the
sweep deletes.
Not addressed, all failure-to-collect rather than data loss: maxPreviewItems
caps the admin button's list while doRun uses the uncapped one, so a hold with
10000+ orphaned layer records surfaces no aux orphans in the preview;
DeleteManifestAuxRecord cannot distinguish "already gone" from "write failed";
and discoverUserDIDs never consults the aux collections, so a record whose owner
has no remaining layer records is kept indefinitely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A user who deletes a manifest record directly on their PDS was invisible
to the hold. purgeManifest only fires on appview-driven deletes, and the
GC orphan sweep only walked io.atcr.hold.layer, so scan and image config
records survived forever and the user's storage number never dropped.
analyzeRecords now also walks io.atcr.hold.scan and
io.atcr.hold.image.config, applying the same orphan test the layer sweep
uses: a record dies only when its owning PDS was reachable and
demonstrably lacks the manifest. Unreachable PDS, unparseable URI,
unparseable timestamp, and immature records all keep.
Splitting the grace period was required to make this useful, not
cosmetic. Records only need to outlast a push (blobs and layer records
are written before the manifest reaches the user's PDS), so they age out
at 24h. Blobs stay on the 7 day window, but since records now age out
faster than the blobs they name, a record can no longer serve as its
blob's clock. Blob age comes from S3 LastModified instead, which
WalkBlobs already received from ListObjectsV2 and was discarding.
Net effect: pruning an old manifest frees the user's storage on the next
nightly run rather than never, while the bytes are still reclaimed on
the same best-effort schedule as before.
DeleteManifestAuxRecord is deliberately narrow, accepting only the scan
and image config collections, so a bug in the sweep can't reach captain,
crew, or layer records.
Not addressed: the SBOM and vuln report blobs those scan records point
at. They share the /repos/{holdDID}/blobs/ prefix with the hold's avatar
and OG images, so collecting them needs a referenced-CID set built from
the hold's own records rather than a prefix walk.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1. Removing distribution/distribution from the Hold Service (biggest change)
The hold service previously used distribution's StorageDriver interface for all blob operations. This replaces it with direct AWS SDK v2 calls through ATCR's own pkg/s3.S3Service:
- New S3Service methods: Stat(), PutBytes(), Move(), Delete(), WalkBlobs(), ListPrefix() added to pkg/s3/types.go
- Pull zone fix: Presigned URLs are now generated against the real S3 endpoint, then the host is swapped to the CDN URL post-signing (previously the CDN URL was set as the endpoint, which
broke SigV4 signatures)
- All hold subsystems migrated: GC, OCI uploads, XRPC handlers, profile uploads, scan broadcaster, manifest posts — all now use *s3.S3Service instead of storagedriver.StorageDriver
- Config simplified: Removed configuration.Storage type and buildStorageConfigFromFields(); replaced with a simple S3Params() method
- Mock expanded: MockS3Client gains an in-memory object store + 5 new methods, replacing duplicate mockStorageDriver implementations in tests (~160 lines deleted from each test file)
2. Vulnerability Scan UI in AppView (new feature)
Displays scan results from the hold's PDS on the repository page:
- New lexicon: io/atcr/hold/scan.json with vulnReportBlob field for storing full Grype reports
- Two new HTMX endpoints: /api/scan-result (badge) and /api/vuln-details (modal with CVE table)
- New templates: vuln-badge.html (severity count chips) and vuln-details.html (full CVE table with NVD/GHSA links)
- Repository page: Lazy-loads scan badges per manifest via HTMX
- Tests: ~590 lines of test coverage for both handlers
3. S3 Diagnostic Tool
New cmd/s3-test/main.go (418 lines) — tests S3 connectivity with both SDK v1 and v2, including presigned URL generation, pull zone host swapping, and verbose signing debug output.
4. Deployment Tooling
- New syncServiceUnit() for comparing/updating systemd units on servers
- Update command now syncs config keys (adds missing keys from template) and service units with daemon-reload
5. DB Migration
0011_fix_captain_successor_column.yaml — rebuilds hold_captain_records to add the successor column that was missed in a previous migration.
6. Documentation
- APPVIEW-UI-FUTURE.md rewritten as a status-tracked feature inventory
- DISTRIBUTION.md renamed to CREDENTIAL_HELPER.md
- New REMOVING_DISTRIBUTION.md — 480-line analysis of fully removing distribution from the appview side
7. go.mod
aws-sdk-go v1 moved from indirect to direct (needed by cmd/s3-test).
Evan JarrettandClaude Fable 5.1