TestHoldPDSConcurrentRepoWritesAndSideTable failed in CI on 2026-09-14
with "database is locked" out of a carstore blocks INSERT, with the code
correct. It was the pds-level twin of the test 0ae9ca1 rewrote: four
record writers, two read loops and a tight autocommit side-table loop
hammered one file and asserted that no lock error surfaced, which on a
loaded runner is a statement about the disk. SQLite's busy handler is not
fair, so a writer can be starved past the 5 s busy_timeout while the side
loop keeps retaking the lock. It did not reproduce locally in 24 runs
under fsync-heavy load.
TestHoldPDSRepoWriteWaitsForSideTableLock tests the mechanism instead: a
second pool on the same file holds the write lock, a repomgr PutRecord is
shown to block rather than fail, to succeed once the lock is released,
and the repo and record are readable afterwards. The pool-level property
(busy_timeout on every connection of every pool, plus a control without
it) is already covered in pkg/hold/db; this keeps the repomgr path and
the post-contention readability check on top of it.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPiKVQcxGYwxAbGnZv2tir
Four scan broadcaster tests failed in CI with "database is locked" on a
status query. The helper opened the scan database with a bare sql.Open,
while NewScanBroadcaster goes through holddb.OpenLocalDB. That left the
test pool with no busy_timeout on any connection and the file in
rollback-journal mode, so a test polling a job's status every 2ms on one
pooled connection raced the storage goroutine's commit on another, and a
read that landed inside the commit failed immediately instead of waiting.
The window is sub-millisecond on a local disk and reproduced only under
fsync-heavy load here, but the CI runner's disk hits it regularly.
Both hand-opened scan databases now go through OpenLocalDB. Under the
same disk load, 120 runs of the four tests pass where one in sixty failed
before.
The CI workflow also now passes -tags testmode, matching make test.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GPiKVQcxGYwxAbGnZv2tir
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
server.test_mode survived the build-tag refactor only to feed five
behavioral branches: the registry's fall-back to the default hold when
the user's hold is unreachable, backfill warning suppression for
external holds, the appview listener close on shutdown, the hold's
relay-crawl skip, and the hold's appview-issuer tolerance. Every one of
them is a "this is a local development build" decision, which is what
the tag already says, and local development has to build with the tag
or nothing resolves. So they read atproto.TestModeBuild now, and the
flag, SetTestMode, IsTestMode, the middleware option, the backfill
constructor parameter, the never-read field on RemoteHoldAuthorizer,
the example and template YAML lines, and the docker-compose env vars
are gone. The registry keeps the fallback as a field seeded from the
constant so the production-path tests can pin it off under the tag.
The 24 SetTestMode calls in tests were dead already: stripping them and
running the affected packages tagged changed nothing.
Tests that resolve a loopback did:web used to t.Fatal naming the tag,
which left a bare `go test ./...` permanently red in five packages.
They now live under `//go:build testmode`: whole-file constraints where
every test needs it, and sibling *_testmode_test.go files holding the
moved tests plus their fixtures where a file mixed. The harness carries
the constraint too, with its package doc in an untagged doc.go so the
package still exists without it. An untagged run compiles those tests
out and passes; make test keeps the tag and runs everything.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwYzaG3Yy7uA8FbZ5qk3tQ
indigo's identity directory refuses HTTP and IP-hosted did:web, and its
OAuth client is growing an SSRF-guarded transport that refuses loopback
and private addresses. Local development and the test suites need both,
and the workarounds were scattered: two did:web fallbacks in the
resolver, a hand-rolled appview key fetch on the hold, and the OAuth
client left on indigo's defaults so any test driving it against an
httptest server depended on the transport staying permissive.
Move every departure from indigo's defaults into one file pair in
pkg/atproto: indigo_prod.go (!testmode) returns indigo's directory and
OAuth client unchanged; indigo_local.go (testmode) wraps the directory
so a did:web naming an IP, localhost, or a host with a port resolves
over plain HTTP, and gives the OAuth client plain HTTP clients. All six
identity and OAuth constructor call sites go through NewDirectory and
NewOAuthClientApp. The resolver fallbacks, DIDWebToURL, and the hold's
scheme-guessing key fetch are gone; the hold resolves the appview key
through the directory, preferring #appview, and purges and retries once
on a signature failure so a re-keyed appview is not masked by the
24-hour cache.
There is no runtime switch for this: a production binary cannot be
configured to resolve local DIDs. The runtime test_mode flag still
gates the remaining behavioral branches only.
Tests, the harness, make dev, Air, Dockerfile.dev, and docker-compose
build with the tag; fixtures that need loopback did:web fail fast
naming it. Test hold servers now serve a did.json via pkg/testpds so
they resolve as real holds under the tag.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UwYzaG3Yy7uA8FbZ5qk3tQ
distribution calls Stat before every blob GET and HEAD. The appview's
Stat asked the hold for a presigned HEAD URL and then HEADed S3 with it
purely to read Content-Length for the descriptor: two round trips to
learn one number.
The hold's getBlob response for OCI digests on GET and HEAD now carries
"size". It comes from the records index when a layer record exists (a
SQLite lookup on a new digest index, no network) and from a HeadObject
otherwise, which is where config blobs land. If storage says the object
does not exist the hold answers 404 instead of signing a URL that can
only fail. The PUT and ATProto CID paths are untouched.
The appview builds the descriptor from the reported size and makes no
S3 request. When the field is absent it HEADs the presigned URL as
before, so a new appview works against a hold that has not been
upgraded, and an old appview ignores the extra field. A hold 404 maps
to ErrBlobUnknown.
Tests prove the index answered by leaving the mock bucket empty and
counting zero HeadObject calls, prove the fallback with exactly one, and
count requests reaching the fake S3 origin on the appview side rather
than trusting the returned size.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5
ValidateBlobWriteAccess, ValidateBlobReadAccess, ValidateOwnerOrCrewAdmin
and getCrewTier each listed every crew record to find one member: open a
carstore session, walk the MST, CBOR-decode each record, compare DIDs.
That ran on every multipart call from the appview, including the part
URL request for every 10MB, and on every getBlob presign, so on a hold
with hundreds of crew each part cost hundreds of decodes.
lookupCrewMember tries the deterministic rkey first (one record read)
and only falls back to the walk on a not-found miss. The fallback is
required: records created before the hash-rkey scheme sit at a TID rkey,
and the boot-time migration that rekeyed them only existed between
e0a2dda and b2d6842, so a hold that upgraded across that window still
has them. Members hit the O(1) path; only genuine non-members pay for
the walk, and they are denied anyway.
Every authorization decision and error string is unchanged. Tests cover
the deterministic hit, a legacy TID-keyed member found only through the
fallback, a non-member, and a storage error surfacing as an error rather
than a silent denial.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Yf1ZVA7sXYhQNb9tCo1m5
`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
com.atproto.sync.getBlob took its S3 operation straight from ?method= with no
allowlist and passed it to GetPresignedURL, whose switch mints a presigned PUT.
The ATProto sub-handler is public per the ATProto spec, and the OCI one gates
on ValidateBlobReadAccess, which passes anonymous callers on a public hold by
design. So an anonymous caller could obtain a fifteen-minute presigned PUT into
the hold's own bucket, over the avatar, the OG-card thumbnails, stored SBOMs
and vulnerability reports, or the OCI layer space. Holds sharing a bucket
widened it.
The defect is that a read check was gating a write. PUT now requires captain or
crew with blob:write via the existing ValidateBlobWriteAccess, rather than a
new parallel gate. Reads are untouched: ATProto GET and HEAD stay public,
OCI GET and HEAD keep the read check. An unknown method is a 400 rather than
falling through to a 500 from the presign switch.
The identifiers reached object storage keys unvalidated, which the tests showed
was not theoretical: cid=../../../../etc/passwd presigned a key containing that
path, and an OCI digest of sha256:aa/bb presigned across a shard boundary. A
digest is now sha256 plus exactly 64 lowercase hex, shaped after the scanner's
ParseDigest, and a CID must decode and round-trip to its canonical encoding.
Validation runs before the auth gate, so a malformed identifier is a 400 even
for an authorised caller.
Verified before changing anything that no legitimate caller reaches PUT
presigning. Pushes upload through the multipart endpoints, which live inside
requireBlobWriteAccess and use PresignUploadPart; every getBlob caller in the
appview, the scanner and the handlers sends GET, HEAD or nothing, and the
distribution route can only produce GET or HEAD.
One existing test asserted that an unauthenticated PUT returns a presigned URL.
It was pinning the vulnerability as intended behaviour and is replaced by
coverage of both the refused and the authorised cases.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
A PRAGMA is per-connection, and database/sql opens one connection per
concurrent caller. hold_db.go and scan_broadcaster.go each set
busy_timeout = 5000 with a one-shot query against the pool, which configures
whichever connection happened to serve it and leaves every other writer at
zero. Probing the live pools showed exactly that: conn 0 at 5000, conns 1 and 2
at 0. That is the database is locked on COMMIT, and the carstore had no
busy_timeout and no WAL on any connection at all.
Opening a database now goes through a connector that runs the PRAGMA on every
connection the pool creates, ported from the appview, which had already solved
this. Applied to the carstore, the shared hold DB, the records index, the event
broadcaster and the scan broadcaster. In-memory databases are left unwrapped,
since libsql gives each connection its own, and the embedded replica path still
skips PRAGMAs because a remote rejects them.
WAL is not a new risk: production was already WAL, because journal mode is a
persistent property of the file and OpenHoldDB has been setting it. This makes
the carstore's own opener agree rather than leaving standalone and test
databases in rollback-journal mode. A database that refuses WAL logs a warning
instead of failing boot, so a network mount cannot stop the hold starting.
NewHoldPDS's file mode opened the same file a second time for the records
index; it now shares the carstore's pool. Two pools on one file buy nothing,
since SQLite's write lock is per database, so the second only adds contenders.
Production already shared a pool and was unaffected by that half.
Note for anyone reading OpenHoldDB: its foreign_keys pragma has the same
one-connection scope, but libsql reports foreign_keys on for every fresh
connection anyway, so it is decorative rather than broken.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
The stale-scan loop rescans an image on a schedule and handleResult uploaded
both artifacts every time, so each pass wrote a fresh SBOM blob and orphaned
the one before it. Nothing reclaims those, because GC walks only the OCI blob
prefix and scan artifacts live under /repos.
Keep the stored blobs when the record already on file was written by this same
scanner version and carries a vulnerability-report reference equal to the one
this report would be stored under, then keep each blob individually and only if
it is still in S3.
The report reference alone is not quite enough evidence on its own: the report
lists matched packages, not every package, so an SBOM could gain a package with
no known CVEs and leave the report byte-identical. Content is pinned by the
manifest digest, so that can only happen across a scanner or Syft change, which
is what the version clause closes. It is inert until that constant moves, and
one comparison is cheaper than remembering to add it at the moment it first
matters.
Every failure path declines to reuse, which costs an upload and never costs
correctness, including the reuse check's own timeout, which is carved out of
the upload budget rather than given its own so a slow read cannot eat the
deadline it stands in for. A record whose earlier upload failed heals on the
next rescan, keeping the report and rewriting only the missing SBOM.
scannedAt still advances. runStalePass selects on it, so freezing it would make
every deduplicated record permanently stale and rescanned forever, which costs
far more than the bytes saved.
A scanner running with vulnerability scanning off sends no report, so there is
no stable digest to compare and its SBOM still churns. Closing that means
either parsing SPDX in the hold to normalise a timestamp and a UUID, or a
lexicon change; a test pins the gap rather than leaving it to be rediscovered.
Note for anyone extending GC to /repos later: a live SBOM object used to be
rewritten on every rescan and so was always young. It now keeps its original
mtime for the life of the content, so an age-based rule there would delete
referenced blobs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
Three defects that all turn on whether the hold knows what a scanner is doing.
Neither end had a keepalive, a read deadline, or a read limit, so a half-open
connection was invisible until some other timeout fired. That got worse with
capacity-aware dispatch: a dead-but-connected scanner holds its advertised
worker count out of the budget and keeps winning jobs. Both ends now ping every
30s against a 90s read deadline, so three unanswered pings condemn a
connection. Detection takes about 90 seconds, after which the existing
reconnect grace reclaims the rows, against the 60-minute queueing timeout that
was previously the only escape. Liveness decides when a scanner is gone; the
grace window decides when its work is reassignable.
Read limits are asymmetric and deliberately generous, because exceeding one
closes the connection rather than truncating, which would turn a large but
legitimate result into a permanent retry loop. Write deadlines were absent
everywhere; the scanner in particular held a mutex across an unbounded write,
so a wedged write silenced it without disconnecting it.
handleResult did two S3 uploads and a CAR commit inline on the reader
goroutine, so a slow S3 looked exactly like a dead scanner. Terminal messages
now go to a per-subscriber storage goroutine while acks and starts stay on the
reader. One goroutine, not a pool: every path ends in CreateScanRecord, which
serialises on the repo lock anyway, and ordering is worth more than parallelism
that cannot be used. Uploads and the record write get separate budgets, so a
stalled upload cannot spend the time the record needs and the record write
stays unconditional.
checkPredecessor cached an inconclusive answer as a definitive negative in a
map that is never reset, so one unreachable hold meant its manifests were never
scanned again for the life of the process. gc.go already carried the corrected
logic for the same problem; this follows it rather than inventing a second
approach, and also stops treating an unparseable captain record as definitive.
The test PDS had to move off ":memory:", which go-libsql scopes per connection:
writing a scan record from any goroutine but the caller's got a connection with
no tables. That was invisible while every record write happened inline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
An audit of the scan pipeline and the hold side of scanning found several
ways scanning stops without saying so. Each fix here was written test-first:
a test expressing the wanted behaviour, confirmed failing for the right
reason, then the change.
A summary-less result crash-looped both processes. worker.go dereferenced
result.Summary unconditionally, but processJob only sets it when Grype runs,
and SendResult puts the nil on the wire before the scanner dies on it, so
handleResult's unguarded log killed the hold too. A nil Summary now means
"not scanned for vulnerabilities", deliberately distinct from "scanned, found
zero" — inventing a zeroed summary would report every image as clean when
Grype never ran. The hold writes a record rather than orphaning the uploaded
SBOM, and the appview renders an "SBOM only" state instead of a green Clean
badge.
The Grype database could wedge with no way back short of a restart. All three
throttles in loadVulnDatabase were guarded by vulnDB != nil, so a scanner
holding no provider retried a full download on every scan under the exclusive
lock. Two earlier attempts at this bug each added one more condition to the
same chain; this replaces the chain with a single decision function over a
state snapshot, consulted by both call sites so they cannot disagree. That
disagreement was itself a bug: the 50-scan reload had never once executed.
Two independent halts. An unparseable frame was dropped in silence, stranding
a row that held the hold's only 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.
A digest went unvalidated into filepath.Join and os.Create, so a layer digest
of sha256:../../../x wrote outside the scan directory, and nothing verified
that downloaded bytes hashed to the digest naming them. Digests come from
records in a user's own PDS. Both are fixed together: verification is what
makes an escaping write self-defeating.
Concurrency did not work on either axis. The proactive capacity gate was
depth-one hold-wide, so neither extra workers nor extra scanner processes
received work. Depth is now the sum of the worker counts scanners advertise on
connect, the gate is scoped to proactive work, and dispatch prefers the
least-loaded scanner. Disconnects no longer hand a running scan to someone
else: a scanner keeps a stable per-process identity and reclaims its own rows
within a grace window, while a process that truly restarted returns with a new
identity and has its work reclaimed, which is correct because the restart did
lose it.
The hold's scanning deadline measured queueing rather than scanning, because
the scanner acks on receipt and handleAck never refreshed assigned_at. A new
"started" message, sent by the worker that dequeues the job, separates the two
budgets. An older scanner never sends it and falls under the queueing budget,
which is more forgiving than the deadline it gets today.
Adds an in-process mock hold and an e2e harness that runs the real client,
queue and worker pool, seeded with 84 real manifest records fetched from a
live PDS. Real image layouts and the Grype database are fetched by scripts and
gitignored; suites needing them skip cleanly, so the default run stays offline
and fast.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01U1Km3N3uUmeGaj7VbaM8PF
Vulnerability scanning produced nothing across the whole deployment for
nine days, from 2026-08-25 01:20:48 until a scanner restart on 2026-09-03.
The scanner was connected and idle, the hold's discovery pass kept
reporting unscannedFound=15 every four hours, and no scan_jobs row was
created in that entire window.
hasActiveJobs counted pending, assigned and processing rows globally with
no age bound, and waitForCapacity spins while it is true. dispatchLoop
calls it before popping any candidate, so a single pending row that never
reached a terminal state reported "busy" forever: discovery kept pushing
candidates into unscannedQueue and nothing ever popped them. That is why
the symptom was an empty queue rather than a growing one.
Nothing papered over it because push-triggered enqueue only fires for
owner or a tier with scan_on_push, which in production means pro alone.
All 210 manifests pushed to this hold in that window came from free,
supporter, or accounts with no crew row, so the frozen proactive loop was
the only source of jobs.
Nor could it recover on its own. Only Enqueue and drainPendingJobs
dispatch a pending row, and drainPendingJobs runs only when a scanner
newly connects; reDispatchTimedOut considered assigned rows only. The
hold had been up since Aug 14 and the scanner since Aug 21 on the same
websocket, so the drain path had not run since the row appeared.
So bound the capacity gate to pending rows younger than pendingStaleAfter,
give reDispatchTimedOut a pending reclaim, and check RowsAffected on the
assign UPDATE now that two dispatchers can race for a row. waitForCapacity
warns and names the blocking jobs after ten minutes without capacity,
because the failure mode above was completely silent.
Two adjacent fixes for the same outage. The scanner never called
InitLogger, so log_level and log_shipper were dead config and an idle
scanner was mute, which is what made nine days invisible. And skipReason
now also skips a job whose layers contain nothing tar-shaped: the job that
wedged this queue was an in-toto attestation whose config mediaType is an
ordinary image config, so the existing config-type check missed it and
buildOCILayout would have handed Syft an empty image.
The regression tests were verified against the old logic first: three of
them fail on it and pass on the fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPWkeCKcbtGoyXyyeMhSps
The hold end of the billing fan-out had no test at all -- only the
ErrCrewMemberNotFound sentinel was covered. Its answer decides whether the
Stripe webhook records an event as processed or retries it, so each status it
can return means something different upstream and is covered separately: the
applied path (asserting the stored crew record actually changed, not just the
response body), not-crew as a successful no-op, the 403 on a body userDid that
disagrees with the signed subject, an empty body userDid falling back to the
token subject, 401 unsigned, 400 with no tiers configured, and rank clamping.
Each was mutation-verified. One of them corrected the test's own comment:
removing the 403 guard does not let a body retarget a grant, because every step
after it keys off the token's sub claim and req.UserDID is read nowhere else.
The guard makes a disagreeing body loud rather than silently ignored, and the
stored-tier assertion is the regression guard for the day something reaches for
that unsigned field when it needs "which user".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
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>
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>
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
Evan JarrettandClaude Fable 5.1
Maarten RijkeandTangled