A hold DID that can never resolve is a property of stored user data, not a
fault on our side. The value comes from a user's own sailor profile
defaultHold, so any account can choose the appview's ERROR volume, and
nothing is cached on the failure path, so it re-logs on every request for
that user.
On production this was not a rounding error: two accounts pointing at
did:web:localhost%3A8080 produced 2956 of 2958 ERROR lines over seven days,
99.9%. The genuine rate underneath was about two a day, which made
level=ERROR useless as a signal or an alert threshold.
Classify at the resolution boundary instead of string-matching prose.
ErrHoldDIDPermanent marks a malformed identifier or a missing DID document;
those log at DEBUG while everything an operator could act on stays at ERROR.
didWebHostUnusable is conservative on purpose: it only claims the cases we
are sure about (percent-encoded ports, bare IPs, localhost), so an
unfamiliar failure stays loud rather than being quietly swallowed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
6758996 started verifying captain records against the publishing DID's
atcr_hold service before caching them, because any account can write an
io.atcr.hold.captain record into its own repo and only a real hold advertises
that service. It covered the two Jetstream writers -- the processor and the
batch backfill -- and left RemoteHoldAuthorizer.GetCaptainRecord alone.
That third path is reachable. A user's sailor profile decides which hold their
content routes to, blob authorization calls CheckReadAccess/CheckWriteAccess
with that DID, and both go through GetCaptainRecord. ResolveHoldURL falls back
to the DID's #atproto_pds endpoint when there is no #atcr_hold service, so a
user who points defaultHold at their own DID serves themselves a captain record
of their own writing -- and it was cached.
The row is the problem, not the fetch. GetAvailableHolds offers every
hold_captain_records row with allow_all_crew=1 to every user's hold picker, so
one unverified row puts an arbitrary DID in front of everyone as a place to
store blobs. GetAccessibleHoldDIDs reads the same table to scope visibility.
Gate the cache write only, not the authorization decision. Failing closed here
would turn a PLC resolution blip into a rejected push, and the freshly fetched
record is no less trustworthy than it was before this commit -- it just must not
become durable. This matches the processor's "skip rather than fail" handling,
where periodic backfill retries an unresolvable DID later.
hasHoldService becomes a package var so the negative case is testable at all:
the real implementation trusts any did:web in test mode, which is the shape
every test here uses. The three new tests are mutation-verified -- removing the
gate caches a row for both a non-hold DID and an unresolvable one, while the
inverse test keeps the gate from degrading into "never cache", which would cost
an XRPC round trip on every authorization while still looking like a pass.
Note in passing: TestFetchCaptainRecordFromXRPC discards its result
(`_ = record; _ = err`) and asserts nothing. Left alone here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
cacheDenial read denial_count, incremented it in Go, and wrote the result back.
Two overlapping denials for the same (hold, user) both read the same value and
both wrote the same value, so one increment vanished. The effect is that the
backoff ladder advances more slowly than configured, which means a denied client
keeps hammering the hold's PDS for longer than intended. Already reachable
across goroutines on one instance; routine with several behind a load balancer.
It is now a single INSERT ... ON CONFLICT DO UPDATE that increments in place.
next_retry_at moved into SQL as well, derived from the count the same statement
is producing, rather than computed in Go from a count that may already be stale
by the time the write lands. The CASE ladder is generated from
dbBackoffDurations so configuration still drives the backoff, and no request
data reaches the string.
The measured difference, with 20 concurrent denials: the old code recorded 12
where it should have recorded 21, losing 9. The new code loses none.
The surviving SELECT only picks a branch (first denial goes to memory only), so
a stale answer costs at most one skipped or one extra write, never a count.
Two implementation notes. datetime() truncates to whole seconds and the backoff
ladder is sub-second in tests, so timestamps use
strftime('%Y-%m-%dT%H:%M:%fZ', ...) instead; libSQL normalizes that to RFC 3339
and it scans back into time.Time with the right instant, which was verified
before relying on it. And a one-rung ladder emits a bare number rather than a
CASE, because "CASE ELSE x END" with no WHEN arm is a syntax error.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ClearAllDenials ran unconditionally at startup, and its database half is
"DELETE FROM hold_crew_denials" with no scoping at all. One instance, that is a
clean slate on deploy. Several instances, and a rolling deploy wipes the shared
table once per instance while every scale-out event wipes it again, so the
backoff that exists to stop a denied client hammering a hold's PDS keeps getting
reset out from under it.
The intent is worth keeping: a restart usually means a fix shipped, and someone
sitting on a backoff of up to an hour should get to retry rather than wait it
out. So it moves under the cleanup lease instead of being deleted, and now
happens once per deploy rather than once per instance.
Worth noting the in-memory half was always a no-op here. recentDenials belongs
to the process, and a process that has just started has an empty one, so the
table-wide DELETE was the only thing the startup call ever really did.
The cleanup worker moved down past the hold authorizer's construction, since it
now needs a handle on it. Reading s.HoldAuthorizer from the worker goroutine
while the constructor was still assigning it would have been a data race.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a7569a7 added credential-less pulls of public images. Three things about it
were wrong, all of them in how the appview handled the decision that belongs
to the hold.
**Scope handling was all-or-nothing.** IsPullOnlyScope required every
requested action to already be "pull", but clients routinely ask for more
than the operation needs — pull,push is common for a plain read, and some
ask for pull,push,delete up front. Those were rejected and challenged,
leaving a credential-less client no way to pull even a public image, which
is the entire feature. NarrowToPullOnly drops the write actions and issues a
token carrying "pull" and nothing else. Granting a subset is what the
distribution token spec expects. The allowlist property is preserved: "pull"
is the only action that survives, and "*" is deliberately not expanded into
it, since a wildcard request is not evidence the caller wants a read.
**The appview-side read gate was inert.** checkReadAccess passed p.ctx.DID,
the DID of the repository *owner*, not the requester. Any non-empty DID
satisfies a private hold's check, and the owner's is never empty, so it asked
"may the owner read their own hold", answered yes, and admitted everyone.
Worse, CheckReadAccessWithCaptain admitted any authenticated DID to a private
hold at all, on an explicitly-MVP assumption that holding a DID was close
enough to being a sailor. Every doc says otherwise (docs/hold.md:109 "Crew
with blob:read", CLAUDE.md:140, docs/BYOS.md:280) and so does the hold
(ValidateBlobReadAccess: owner, or crew carrying blob:read/blob:write). It
now takes isCrew and requires owner-or-crew, and callers only pay for the
crew lookup when it can change the answer — a public hold or an anonymous
caller is decided by the captain record alone. Nothing here loosens access;
it brings the local gate into agreement with the authority.
**Denials could not reach the client.** distribution's blobHandler.GetBlob
maps everything except ErrBlobUnknown to ErrorCodeUnknown, so a 401 raised
in the blob store left as a 500 — misreporting an auth failure as a server
fault, and giving BearerChallenge no 401 to attach WWW-Authenticate to, so
Docker was told "server error" instead of being prompted for credentials.
Clients that retry 5xx looped: 4.1s per case in the matrix, now 0.01s. The
check moves to Repository(), where an errcode.Error is passed through
verbatim by the registry app — the same mechanism a7569a7 used for
NAME_UNKNOWN. It fails open on a lookup error, since the hold is the
authority and a transient failure should not break public pulls.
Removes auth.allow_anonymous_pull. It could only ever withhold — captain.Public
is what grants — so it was a second flag for a decision the hold already owns,
and gating it appview-side was never the intent. Layer bytes 307 straight to
S3, so the appview is not even in the path whose cost might have justified an
operator-side lever.
Tests: TestAuthMatrix only ever ran against a public hold, and its pull cases
never fetched a layer — crane.Pull is lazy and img.Digest() needs only the
manifest, which ATCR serves from the user's PDS where it is world-readable, so
no pull row in the matrix touched blob authorization at all. Pulls now
materialize layer bytes, and testharness.WithPrivateHold plus
TestAuthMatrixPrivateHold cover public:false + allow_all_crew:true — the
production shape, where anyone with an account pulls and pushes and anonymous
gets nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Evan JarrettandClaude Opus 5