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>
newTestScanBroadcaster builds the struct with only a database, so nothing in
the suite ever reached handleWriter, handleReader, or the Unsubscribe teardown
they share — which is precisely what 05b856b changed. These give the subscriber
a real WebSocket so the teardown actually runs.
The invariant is the one Unsubscribe documents: a dropped scanner unwinds both
goroutines and each calls Unsubscribe, so everything past the `found` guard must
happen exactly once. Closing `done` twice panics and takes the hold down with
it, and re-running the requeue UPDATE would unassign jobs a replacement scanner
had already claimed.
Three cases: Unsubscribe called twice on the same subscriber, handleWriter
releasing and closing the connection once done is closed, and the real shape of
a scanner vanishing — client closed, both goroutines unwinding into the same
subscriber, plus a late duplicate Unsubscribe after the fact.
Passes -race -count=5.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Webhook delivery was neither idempotent nor order-safe, and every failure
returned 400, which Stripe does not retry. A transient DB or hold error
therefore dropped a subscription change silently and permanently.
- New stripe_processed_events table: event_id as primary key dedups
redelivery, and event_created per customer drops stale out-of-order
deliveries.
- HandleWebhook distinguishes ErrWebhookSignature (400, no retry) from
ErrWebhookProcessing (500, Stripe redelivers). The event handlers
return errors instead of swallowing them. ErrBillingDisabled maps to
400: the route is mounted but billing is off, so redelivery can never
succeed and Stripe should stop rather than retry to exhaustion.
- Refuse to boot when billing is enabled with an empty
STRIPE_WEBHOOK_SECRET. Stripe HMACs with the empty key, so an
attacker can reproduce the signature and the endpoint is forgeable.
- UpdateCrewTierOnAllHolds retries each hold (3 attempts, linear
backoff, 5s per request) and returns a joined error so the webhook
can fail and let Stripe redeliver.
The fan-out contacts holds concurrently rather than in sequence. Serially,
one unreachable hold burns the caller's entire 10s budget on its own
retries (3 x 5s plus backoff) and the holds after it are never contacted;
because Stripe redelivers in the same order, a persistently-down first
hold means the rest are never updated at all.
On the hold, the signature-validated sub claim is now the source of truth
for updateCrewTier: a mismatched body userDid is rejected with 403 rather
than retargeting the grant to another DID. "Not crew on this hold" is a
200 no-op, since the appview fans updates out to every managed hold and a
subscriber is not crew everywhere.
That no-op has to be told apart from a storage failure. GetCrewMember
collapsed both into one generic error, so a CAR-store failure read as
"not a member", answered 200, and let the appview record the event as
processed — losing the tier grant permanently, which is exactly the
failure mode this commit exists to prevent. Missing records now carry an
ErrCrewMemberNotFound sentinel, and anything else returns 500 so Stripe
redelivers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The dashboard's top-users panel resolved a handle for every user with a
quota record, then sorted and truncated to ten. On a hold with ~500 crew
that is ~500 serial identity lookups to render ten rows.
Each lookup goes through the shared identity directory, whose HTTP client
allows 10s per request. One stalled lookup consumed the entire reverse
proxy budget, so the panel returned a partial body and the client hung up
mid-render:
admin/auth.go:161 "Failed to render template"
template=partials/top_users.html
error="write: broken pipe"
"GET /admin/api/top-users?limit=10" - 200 4096B in 10.005s
Sort and truncate first, then resolve only the surviving rows, so the
count is bounded by the limit rather than by hold size. Resolve those
concurrently under a 3s deadline: a slow lookup now degrades to a bare
DID instead of taking the whole request down with it.
The crew tab has the same underlying problem in a different shape, one
lazy-load request per row, and is not addressed here.
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>
Same class of bug as the firehose backfill, plus a second one found alongside
it. Both take down the whole hold process.
1. send on closed channel. Subscribe spawns drainPendingJobs in its own
goroutine, and it sends to sub.send without holding sb.mu. Unsubscribe
closed sub.send under the lock, so a scanner disconnecting during the drain
closed the channel out from under an in-flight send. The existing
`case <-sub.done` guard did not help: done meant "writer goroutine exited"
and was closed by handleWriter, which is a different event from
unsubscribing.
2. close of closed channel. Unsubscribe closed sub.send unconditionally, but
it is called from two places — handleWriter on write error, and
handleReader in its defer. A scanner dropping mid-write hits both, and the
slice-removal loop had no guard, so the second call fell straight through
to the close. The unassign UPDATE ran twice for the same reason, which
could return jobs a replacement scanner had already been handed.
sub.send is now never closed. done is repurposed to mean "this subscriber is
gone", closed only by Unsubscribe and guarded on whether the subscriber was
actually still registered. That makes drainPendingJobs' existing done case
correct, and handleWriter selects on done rather than ranging over send.
dispatchJob was already safe — it sends under sb.mu, which excludes
Unsubscribe.
hold01 is unaffected in practice (scanner disabled, no shared secret), but
seamark-hold runs the scanner continuously and is exposed on any scanner
restart.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A relay that connected with a stale cursor and then dropped mid-backfill took
the whole hold process down:
panic: send on closed channel
pds.sendBackfillMsg events.go:869
pds.backfillFromDatabase events.go:809
pds.backfillSubscriber events.go:716
Subscribe spawns backfillSubscriber in its own goroutine, and that goroutine
writes to sub.send without holding b.mu. Unsubscribe closed sub.send under the
lock, so a disconnect during backfill closed the channel out from under an
in-flight send. select cannot guard that — a send on a closed channel panics
unconditionally.
sub.send is now never closed. Subscriber gains a done channel that Unsubscribe
closes instead, and every sender that runs unlocked selects on it. The map
check in Unsubscribe keeps the close single-shot, which matters because both
readPump and handleSubscriber call it on the way out. handleSubscriber selects
on done rather than ranging over send, since nothing closes send any more.
Broadcast and BroadcastIdentity were already safe (they send under b.mu, which
excludes Unsubscribe). backfillFromMemory was safe too via b.mu.RLock, but now
routes through the shared helper so a disconnect aborts immediately instead of
stalling up to 5s per event while holding the read lock and blocking every
broadcast.
The bug dates to 2025-10, so every build since is affected. It only fires when
a backfill goroutine exists, which Subscribe skips when cursor == currentSeq —
that is why caught-up relays never triggered it and a hold whose relays are all
behind is exposed on every reconnect.
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>
checkPredecessor returned true for any hold with a non-empty successor field,
without verifying the successor was actually this hold.
This caused every hold running proactive scan discovery to queue manifests from
every other migrated hold on the network, producing 404 errors when the scanner
tried to fetch foreign blobs from its own S3.
Signed-off-by: Maarten Rijke <did:plc:fy4lwkc4hrd776vfkcrbzr5a>
- diff view gains a Packages tab with added/removed/changed/unchanged
package tables and purl-derived type/license/upstream links
- captain records verified against the DID's atcr_hold service before
caching (processor + batch backfill), preventing forged holds
- fix empty-handle updates clobbering cached handles and colliding on
the UNIQUE constraint
- move fillPrevCIDs into repo.go; DirectRepoOperator is now canonical,
repomgr kept as a test oracle
- surface read-only crew status in hold selector
- reconcile docs
1. Multiple registry domains + per-user domain preference
The biggest feature. The appview can serve several registry domains (e.g. buoy.cr, atcr.io),
and users can now pick which one shows up in their pull/push commands.
- Lexicon/record: adds registryDomain (and documents ociClient)
to the sailor profile (lexicons/.../profile.json, pkg/atproto/lexicon.go).
- DB: new registry_domain column on users (schema.sql + migration 0027),
with GetUserByDID/Handle reads, UpdateUserRegistryDomain writer,
and Jetstream caching it on profile updates (writes unconditionally so clearing propagates).
- UI/handlers: new UpdateRegistryDomainHandler + /api/profile/registry-domain route,
a <select> in the user settings panel (only shown when >1 domain configured), and resolveRegistryURL()
which falls back to the primary domain if the user's pref is stale/removed. Tests added for all of it.
2. default_hold_did removed → first managed_holds entry is the default
Consolidates two overlapping config fields into one. ServerConfig.DefaultHoldDID is gone;
PrimaryHoldDID() now returns managed_holds[0]. managed_holds is now REQUIRED.
Updated in config, validation, server wiring, test harness, example YAML, and the deploy template.
3. Admin long-running operations → generic background-job framework
New pkg/hold/admin/jobs.go introduces a reusable startJob/jobRegistry pattern
(a detached context.Background() job + a /admin/api/jobs/{key}/status polling endpoint).
This replaces the bespoke scan-backfill goroutine state machine, and now also wraps crew tier remap and crew import
all three previously looped synchronously on the request context and got 504'd/cancelled mid-run by the reverse proxy.
Forms switched from POST-redirect to htmx fragments (job_progress.html, job_result.html, crew_import_results.html)
the old crew_import_results.html page and scan_backfill_progress.html partial were deleted.
This is also captured as a new rule in CLAUDE.md.
4. Cascade-delete manifest on last-tag deletion
DeleteTagHandler now, after removing the last tag pointing to a digest, cascade-deletes the manifest itself
(PDS + DB + hold blob purge) — but only if it's not a child of a manifest list (multi-arch parent).
New GetTagDigest and ShouldCascadeDeleteManifest queries back it, plus cascade_delete_test.go.
Also switches tag rkey computation to the atproto.RepositoryTagToRKey helper.
5. Billing simplification
Drops the OwnerBadge config option (hold-owner supporter badge).
The user-profile template no longer special-cases an "owner" badge value (only "Captain").
Example tiers renamed to the nautical scheme (deckhand/bosun/quartermaster).
6. Build/deploy: go generate always runs via Make
make generate is now a phony target that always runs go generate ./... (regenerating cbor_gen, icon sprites, etc.),
and build-trixie depends on it. The deploy tooling (provision.go/update.go)
drops its own runGenerate calls since the Makefile handles it.
7. New cmd/firehose-tap tool (untracked)
A standalone CLI that subscribes to a com.atproto.sync.subscribeRepos endpoint and pretty-prints events,
with emphasis on Sync 1.1 compliance fields (per-op prev CIDs, commit prevData) and a --validate CI mode.
Fits with the recent "more sync1.1 compliant" commit.
Evan JarrettandClaude Opus 5
Maarten RijkeandTangled