Drives "Scan for Orphans" through the admin panel and asserts the wiring the
Go tests structurally cannot: that the progress fragment swaps into
#gc-results and hands off to the preview fragment, that every advertised stat
renders a value, and that each table's row count agrees with the stat above
it. A GC that decides correctly and renders a blank panel still gets someone
to click delete on the wrong thing.
Then it hand-checks the claim itself, which is the part that matters: for
each distinct manifest behind an orphaned record, resolve the owner's PDS and
ask whether that manifest is really gone.
Two instrument bugs were found writing this, both in the script rather than
the product, and both worth keeping as comments:
* The three tables overlap on a Digest column, so classifying by "has
Digest but no RKey" swallowed Missing Records as orphaned blobs and
reported 3 blobs against a stat of 0. Classification is now by exact
header set.
* Asserting the manifest is ABSENT from the PDS is too strong. A manifest
can be alive and name a different hold, which is exactly what happens
when defaultHold is repointed and the image re-pushed. Those records are
legitimately orphaned here. The only state that means GC is staged to
destroy live data is a manifest that exists AND still names this hold.
Against the dev hold: 387 orphaned records over 68 distinct manifests, 25
sampled — 17 gone, 8 alive but now pointing at the production hold, 0 still
naming this hold. Orphaned blobs 0, referenced 15, and the counts agree with
the tables.
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>
oauth-refresh-e2e.sh covers the refresh mechanics; this covers the symptom the
range exists to stop — a signed-in browser being thrown out when the access
token rotates underneath it.
The test is only meaningful because ui_sessions is a table carrying an
oauth_session_id rather than an in-memory map, so restarting the appview to
clear the refresher's cache does not by itself log the browser out. Verified
against the row the browser actually uses: one oauth_sessions row, and the
ui_sessions row created by the login points at it. rev advances 1 to 2 while
/settings/user keeps rendering.
Drives the login itself. Given a handle it fills the field and clicks through
consent, which is the whole flow whenever the PDS already has a session. Two
things it must not do, both learned by doing them:
* Never navigate while waiting for a human. The first version re-issued
goto() every two seconds and wiped the login form out from under whoever
was typing into it.
* Never bail permanently at a password field. Returning there left the flow
stranded on the Authorize screen once the password had been submitted,
because nothing was left to click it. It now pauses and resumes.
ATCR_E2E_FRESH clears only the appview's cookies. Clearing all of them takes
the PDS session with it, which turns a handle-and-consent flow into a password
prompt and makes the login impossible to drive unattended.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
client_test.go reproduces the cancellation precisely, but in-process against an
httptest PDS. The failure this range fixes — mass sign-outs — happened against a
real one, through the real refresher and the real oauth_sessions row, so the
unit tests alone are a thinner sign-off than the batch deserves.
Staling the access token in the live row and restarting the appview (the
refresher caches sessions in memory, so editing the DB alone changes nothing)
forces the real refresh path. Four concurrent pulls then advance rev 1 to 3
rather than 1 to 4: the compare-and-swap collapses four racing refreshes into
two rotations, with the losers adopting the winner's token instead of each
burning one. Killing a pull 150ms into that window and retrying still succeeds.
Backs ui.db up first, since a burned refresh token would otherwise leave the
account signed out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Migrations are forward-only and the appview applies whatever is missing on
boot, so moving to the next batch does not need a reset at all — Air rebuilds
into the new code and the live DB migrates in place. Verified moving onto
val/04-oauth: 0031 appeared in schema_migrations on its own, on top of a
level-27 database, with the appview healthy afterwards.
That matters more than it sounds. ui.db holds the OAuth sessions and the
appview's signing keys, so the old wipe-on-every-switch cost an interactive
`docker-credential-atcr login` per batch, which is most of what made the stack
awkward to hand to an agent. Validating in stack order is all forward motion,
so in the normal case there is now no login at all.
A reset is still done when the live DB carries migrations the branch's code has
never heard of, which is what going backward means, and the per-set snapshot is
still banked so that case can restore rather than start empty.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batching reorders migrations. val/04-oauth carries e75b2e2 (commit 48 of the
range), whose migration is 0031, while 0028-0030 only arrive in batches 09, 12
and 14. So val/04 holds {..0027, 0031} and val/09 holds {..0027, 0028, 0031}.
Both have a max version of 0031, so keying the snapshot on the max would
restore val/04's database onto val/09 — a database missing 0028's schema while
schema_migrations claims otherwise.
Snapshots are now keyed on a fingerprint of every migration file present, which
distinguishes those two states. The max version is kept in the filename purely
so the directory stays readable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wiping ui.db on every switch also wipes the OAuth sessions and the appview's
oauth_p256/jwt_rsa keys, so each batch cost an interactive
`docker-credential-atcr login`. There are only five distinct migration levels
across the stack (27 for batches 00-08, 28 for 09-11, 29 for 12-13, 32 for 14,
34 for 15), so the DB is snapshotted per level and restored instead of
re-migrated. One login now serves every batch sharing a level.
Also skip the compose pin from val/01 onward: a7c7db6 lands there, so the
branch's own compose already has the shared netns and pinning would drag in
later batches' changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jobs_test.go covers the job framework thoroughly, but nothing covers the
wiring: whether the kickoff handler renders the progress fragment into the
right hx-target, and whether the loop actually outlives the request it was
started from. Both are what ab4a4eb changed, and both are invisible to Go
tests — a typo in an hx-target or a fragment that renders blank passes every
assertion we have.
The load-bearing check drives crew import rather than the tier remap. A
one-member remap completes in under a second, so closing the tab "mid-run"
proves nothing; import does a PDS write plus a network PLC lookup per entry,
which leaves a real window to close the browser and watch the job keep going.
It is caught mid-flight at a progress tick with no admin page open.
Seeded members are created on the local-only dev hold and removed in a
finally block. README records the environment traps found while building
this: 127.0.0.1 vs localhost, in-memory sessions dying on every hold rebuild,
UA/IP pinning that makes curl log you out, and the forward-only appview
migrations that require a per-batch DB reset.
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>
Credential-less pulls of public images. /auth/token issues a pull-only
token with an empty subject when no Basic auth is present; the
destination hold still enforces captain.Public, and push or delete always
challenges.
- token.IsPullOnlyScope and AuthMethodAnonymous;
Handler.issueAnonymousToken skips the authorizer gate and the
service-auth pre-mint, since there is no identity to reconcile and no
AppView-to-hold service token to bind. The token is still stamped
with the resolved registry domain, so anonymous pull works on
secondary front doors whose access controller demands their own
audience.
- auth.allow_anonymous_pull (default true) turns it fully off, restoring
the previous always-challenge behavior. Mirrored into the deploy
template, since the default means existing deploys pick this up.
- RegistryContext.Anonymous is plumbed from the middleware.
- ProxyBlobStore sends no Authorization header when the service token is
empty, and returns 401 rather than 403 for anonymous denials so Docker
prompts for credentials, including when a stale captain cache lets the
request through and the hold says private.
- BearerChallenge wraps the /v2/ subtree so a 401 raised deep in the
stack via errcode.ServeJSON still carries WWW-Authenticate.
Distribution's own scoped challenges are left alone.
IsPullOnlyScope allowlists the pull action instead of denylisting push and
delete. Distribution's actionSet.contains treats "*" as *every* action, so
a scope of `repository:victim/img:*` names neither denied string and would
have handed an unauthenticated caller a token valid for push and delete on
someone else's repository — clearing the authgate entirely, since anonymous
tokens deliberately skip it. Writes would still have failed further down
(no PDS credential), but the gate itself was bypassable. Now every
requested action must be exactly "pull". Covered by new claims tests.
Unresolvable identities return NAME_UNKNOWN instead of a bare error that
distribution renders as 500. This path was previously unreachable without
credentials; anonymous pull opens it to the internet, and a 5xx on
arbitrary input both misreports a bad request as a server fault and sends
clients that retry 5xx into a retry loop. That loop was real: in the auth
matrix, regclient spent 83s on a single case before this fix, and the
suite now runs in 5s.
Stat preserves an authorization verdict from getPresignedURL rather than
flattening it to ErrBlobUnknown. Distribution calls Stat before ServeBlob
on GET and HEAD, so without this an anonymous pull from a private hold
answered 404 and BearerChallenge had no 401 to annotate — the 401 path
above could never actually reach a client.
The auth matrix is updated to match: anonymous pull of the seeded public
repo now succeeds, anonymous push is denied against a real identity's
namespace (rather than an unresolvable one, which was testing name
resolution rather than authorization), and a new case pins the
NAME_UNKNOWN behavior for an unknown identity.
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>
DELETE /v2/<name>/manifests/<ref> answered UNSUPPORTED before ever
reaching the ATProto-backed stores: distribution v3.1.1's DeleteManifest
handler short-circuits unless app.deleteEnabled is set, which comes from
storage.delete.enabled. Set it (mirrored in the test harness). The
companion storage.EnableDelete option it appends only affects
distribution's built-in store, which RoutingRepository replaces, so it is
a no-op for us.
With the route reachable, make the stores do the right thing:
- ManifestStore.Delete purges the hold's per-layer, scan and image
config records on a detached context, since the DELETE handler
returns immediately and cancels the request context.
- TagStore.Untag resolves the digest before deleting the tag record,
then deletes the manifest if that was its last tag and it is not a
manifest list child, matching the web UI's delete-tag behavior so
deleting an only-tagged image doesn't orphan the manifest.
- cleanupUntaggedManifest becomes package-level over *RegistryContext
so both stores share one implementation.
- purgeOnHold moves out of handlers into pkg/appview/holdpurge so the
storage layer can call it: handlers already depends on storage via
middleware, so storage to handlers would be an import cycle.
- ProxyBlobStore.Delete returns distribution.ErrUnsupported, so the
always-registered blob DELETE route gives a clean OCI UNSUPPORTED
error instead of a generic 500. Layer bytes are reclaimed by the
hold's refcounted GC.
The cascade's still-tagged re-check pages through the tag records rather
than reading a single capped page. Tags for all of a user's repositories
share one collection, so one page is a per-account budget: past ~100 tags
a live tag fell off the end and the manifest was deleted while still
referenced. Incomplete enumeration now skips the delete, since an
orphaned manifest is recoverable and a deleted live one is not.
This also makes the over-quota delete grant added in 6e426dc load-bearing:
it hands out pull,delete tokens, which could not do anything while
distribution rejected every DELETE.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Evan JarrettandClaude Opus 5