A PDS that grants fewer scopes than requested produced a session that
worked until the first write it wasn't allowed, which the PDS answered
with a 403. We classified that 403 as a revoked session, deleted it, and
returned a 500 "unknown error"; Docker's retries then failed with "no
session found". Logging in again got the same partial grant, so the user
looped (#30, an older tranquil PDS that left a scope off its consent
screen).
- Login refuses a partial grant. The callback checks the granted scopes
cover what was requested and, if not, deletes the new session and shows
a page listing what's missing. It runs before the old-session cleanup,
so a refused login leaves a working session alone. "Try again" goes
back through the login page so return_to (e.g. the device page) holds.
- MissingScopes compares scopes by what they grant, not by spelling: an
include: expanded or echoed back, collections split or reordered,
wildcards, transition:generic. Extra grants are fine. It replaces the
exact-match ScopesMatch at login, on resume, and in the boot sweep, which
now evicts only sessions missing something.
- A 403 never deletes a session. InsufficientScope comes out of
isAuthError and IsSessionInvalidError, and isOAuthError no longer treats
every 403 as dead. PDSes spell this differently (tranquil:
InsufficientScope, the reference PDS: ScopeMissingError), so nothing
keys on the name.
- A PDS 403 on a manifest or tag write reaches Docker as DENIED with the
PDS's own reason. The UI write handlers (star, tag and manifest delete,
repo avatar and description) answer 403 with the reason too.
- The OAuth error, missing-permissions and success pages render in the
site layout via an injected PageRenderer; pkg/auth/oauth keeps its
inline templates as a fallback.
Verified live against a reference PDS with a forced partial grant: login
refused, an existing session kept, a push denied twice on the same
session with the PDS's message, the boot sweep evicting the partial
session, and a full login pushing normally.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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
8487258 added the deprecation notice to the shared helm metadata partial, on
the understanding that both the Overview and Chart tabs rendered it. Only the
Chart tab does. The Overview panel renders the README and never touches chart
metadata, so a deprecated chart carried no warning on the default landing tab.
The notice is extracted into a shared helm-deprecation-notice block, so the
copy lives in one place, and the Overview panel renders it server-side as the
first element in the panel. Deprecation decides whether you should use the
chart at all, so it belongs in the first paint rather than arriving a beat
later from a lazy fetch.
Getting the data there costs nothing extra. The page already made a blocking
hold call for layer count, and for a chart that call was wasted: a helm config
blob is Chart.yaml, which has no history key, so the count always came back 0
and fell through to the database. That call is now FetchHelmChartMeta instead,
against the same XRPC endpoint, so a chart page makes one hold call rather than
two and the displayed layer count is unchanged. A container image never fetches
chart metadata and its path is byte-for-byte the old code.
Failure follows the layer-count precedent: log at warn, leave the metadata nil,
render the page. An unreachable hold means no notice, not a broken repository
page. The tradeoff against the lazy version is that a slow-but-up hold now
delays the whole page, bounded by the same 10s the page already accepted.
A container image emits no element at all rather than an empty one, so the
space-y-4 stack spacing is untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
Deleting a tag while over quota could leave the user in the worst available
state. DeleteTagHandler deleted the tag and manifest rows first, then called
PurgeOnHold, which bounded itself at 10s against the *request* context. The
UpCloud load balancer in front of the appview cuts at its default backend
timeout at about the same moment, wins the race, hands the client a 504 and
cancels that context, killing the purge partway. The appview logged a warning
and returned 200.
So: gateway error, nothing freed, still locked out, the image gone from the UI
so the purge cannot be retried through it, blobs orphaned until the hold's GC,
and the appview considering it a success. Measured on production at
10.002367218s.
Purges now go to a fixed pool of 4 workers rooted at context.Background(), so
they survive the request ending. Following the shape of the hold's startJob
helper, minus the progress fragment, since nobody is watching a purge.
The buffer is bounded at 256 and sheds with an ERROR rather than growing: an
unbounded queue turns a slow hold into an appview memory leak. Submissions are
deduplicated on holdDID|manifestURI so a double-clicked delete does one purge
and one service-token fetch. The channel send happens under the mutex that
guards close, so a concurrent drain cannot send on a closed channel, and the
drain is wired into both exit paths before logging shuts down.
Failures are now classified and surfaced instead of swallowed: transient ones
retry three times under a 90s budget (the hold's purge is idempotent), an
unauthorized third-party hold logs at DEBUG since it is expected, and anything
else that exhausts its retries logs at ERROR naming the manifest and hold, which
is enough to re-drive by hand.
Deliberately not reordered. Purge-first-then-delete requires waiting for the
purge to know whether to delete, which puts the 10s call straight back on the
request. So the orphaned-blob window remains, materially narrower but real: a
purge that exhausts its retries still leaves blobs referenced by nothing until
the hold's GC, and there is no row left to say so. Closing that needs a durable
pending-purge record, which was judged out of proportion here.
server.go in this commit also carries one line belonging to the next one, the
token handler's display-name wiring, since the two changes landed in the same
file concurrently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
io.atcr.hold.stats.daily was handled in the backfill collection list and in
processor.go's dispatch, but missing from isRelevantCollection, which gates
events before ProcessRecord ever sees them. So daily stats records arrived over
the socket and were discarded at the worker, and the trend charts that read them
got nothing live. This is the "present in one list, missing from the other"
shape CLAUDE.md's firehose checklist warns about. Checked the whole class: this
was the only gap. LayerCollection and ImageConfigCollection are absent
deliberately, having no processor handler, and the test now pins that intent.
Note this is currently masked by the relay outage, so fixing the relay alone
would not have restored the charts.
Separately, the README resolution tried "main" and fell back to "master", but
DeriveReadmeURL never fetches: it parses the source URL and interpolates the
branch, returning empty only for an unsupported platform, which is
branch-independent. So if the main call returned empty the master call returned
empty for the same reason, and the fallback could never fire. Removed, with a
comment recording that a branch fallback has to happen at fetch time after a
404. The other two call sites already do exactly that.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
seamark.dev's /install and /settings/devices told users to pipe
seamark.dev/static/install.sh into bash. That file was the unmodified ATCR
script: it announced itself as the "ATCR Credential Helper Installer",
installed docker-credential-atcr, and finished by telling the user to configure
credHelpers for atcr.io, the wrong registry for that deployment. Anyone
following the documented setup ended up pointed at another service. The
templates hardcoded docker-credential-atcr, "atcr" and ~/.atcr/device.json
alongside a correctly themed {{ .RegistryURL }}.
The scripts are now rendered from config by a handler, rather than forked per
brand. A theme overlay was the alternative and was worse: it needed a full copy
of both install.sh and install.ps1 per brand, four scripts to keep in sync, and
the operator asked for these values to come from config.
credential_helper.name is the single knob. Docker resolves a credHelpers value
x by exec'ing docker-credential-x, so the credHelpers value, the binary suffix
and the config directory are genuinely one word, not three that can drift. It
is validated against a strict pattern because it is interpolated into a shell
script.
install.sh renders byte-identical to the deleted static file under the atcr
default, so existing installs are unaffected. install.ps1 differs by one line,
where a stale usage comment named a path the script is not served at.
Two behaviour changes worth noting: these two URLs drop from a one-year
Cache-Control to five minutes, since the body now depends on deployment config;
and credential_helper.tangled_repo becomes a real overridable default. It was
previously assigned over unconditionally and read by nothing, while the shipped
script used a different URL form.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
Four of the twelve home-page repos showed "We couldn't load the README, it may
be rate-limited or private". The URL in those cases was not configured by
anyone: it was derived from org.opencontainers.image.source, a label images
inherit from their base image, so the raw URL named an unrelated project and
404ed. A 404 on a URL the appview guessed is an expected outcome the owner
cannot act on.
The failure flag is now set only when the owner actually pointed us at the URL,
via the io.atcr.readme annotation. A derived URL that fails renders as if there
were no README. Both paths keep their debug log, now carrying an "explicit"
field so the two cases stay distinguishable.
Render failures are suppressed for derived URLs too. The panel's copy and its
"Edit README" action address an owner who configured a source; on a derived URL
there is no configured source, and content that failed to render is very likely
another project's README anyway.
This is the alarming half of the finding. Rendering the wrong project's README
when the fetch succeeds is the larger half and is untouched: there is no
reliable way to detect an inherited label, since the only signal is
org.opencontainers.image.base.name, which is not always set.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
On a public repo, to anonymous visitors, the digest banner read "2 fixes, 12
Critical / 27 High / 26 Medium vulns, -22 layers, -53.6 MB" while the same page
showed 244 vulnerabilities and Layers (22), and its own "View diff" link landed
on "Layers 22 -> 22, every layer Unchanged".
Platform matching only ran when both sides were manifest lists. The comment
after that block said the mismatched case would "fall through and show a basic
banner without layer/vuln details", but no such branch was ever written and
nothing guarded the fallthrough, so execution continued into the layer and vuln
computation with the unresolved originals still in place. The newer side was
the multi-arch index, which carries no layers of its own and is not scanned, so
all 22 layers of the other side read as removed and the vuln delta was computed
against an absent scan.
Returns 204 for either mismatch direction, as the no-common-platform path
already does.
The promised "basic banner" is not implementable as the function stands, which
is presumably why it never appeared: the template renders only NewerTag,
DiffURL and Summary, and DiffSummary is nothing but deltas, so a delta-less
banner collapses to the tag name and would be suppressed by the existing
"nothing meaningful changed" guard anyway. The comment is replaced with one
that says what is actually true.
Showing a real banner here would mean resolving the index to the child matching
the single-arch side's platform, and that side's os/arch is not in the appview
DB at all: Platforms is populated only for manifest lists and the manifests
table has no os/arch columns. It would need either a config fetch from the hold
at render time or denormalising os/arch during ingest. Not attempted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
The URL check accepted http:// while telling the user "must be https", and
guarded no addresses at all. POST /api/webhooks with http://127.0.0.1:9/hook
returned 200 and created the webhook, so both scheduled deliveries and the
synchronous Test button would dial arbitrary destinations from the appview
host, on demand, for any authenticated user. Loopback, link-local (including
the cloud metadata endpoint at 169.254.169.254) and RFC1918 were all reachable.
Enforces https, and refuses non-public destinations.
The load-bearing half is the dial-time check, not the creation-time one. An
attacker controls their own DNS, so a hostname that resolves publicly when the
webhook is created can resolve to loopback when it is delivered, and a
creation-time check cannot see a redirect either. The guard is therefore a
net.Dialer Control hook on the delivery client, which inspects the resolved
address on every connection attempt. Transport.Proxy is explicitly nil:
honouring HTTP(S)_PROXY would route around the Control hook and hand the
bypass straight back. Redirects are re-validated per hop and capped at 3.
The creation-time check stays so the user gets an immediate, comprehensible
error instead of a silent delivery failure later.
IPv4-mapped IPv6 is unmapped before every check, so ::ffff:127.0.0.1 and
friends hit the IPv4 rules. Ranges with no net.IP helper are listed explicitly:
CGNAT, NAT64, ::/96, TEST-NET and reserved space.
Both outbound paths are covered, since the scheduled dispatcher and the Test
button both funnel through attemptDelivery. The dispatcher's other client is
deliberately left unguarded: it fetches quota stats from holds, which
legitimately live on private addresses, and those URLs are not user-supplied.
Note this removes the ability to point a webhook at a localhost receiver in
local development. There is deliberately no environment-variable escape hatch,
since a security toggle read from the environment is the same bypass wearing a
nicer coat.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
The UI and the dispatcher asked the same question and got different
answers. getWebhookLimits short-circuited on a disabled billing manager and
returned unlimited without consulting it, while server.go hands
BillingManager.GetWebhookLimits straight to NewDispatcher, bypassing that
short-circuit entirely. With billing compiled out the stub answers
non-captains with (1, false).
So a non-captain saw "N / unlimited webhooks configured", could create as
many as they liked, and only the oldest was ever delivered. allTriggers was
false on that same path, so even the surviving one was restricted to
FreeTriggerMask; a webhook set to a scan trigger fired nothing at all, with
no message anywhere and only an INFO line server-side.
Route both paths through the manager so they cannot drift. The intended
non-billing policy is one webhook with TriggerFirst | TriggerPush |
TriggerQuota, which is what the stub already returned and what the shipped
config's Free tier specifies (max_webhooks: 1, webhook_all_triggers: false),
so enabling billing leaves free users exactly where they were and only
unlocks upward. That also removes a downgrade cliff: nobody can accumulate
webhooks under a phantom unlimited and lose them when billing turns on.
Drop the dead webhookLimits{Max: 1}, overwritten on the following line, and
give a nil manager the same policy rather than a third answer.
Latent, not live: production has zero webhooks configured today.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
The interactive prompt named codeResp.VerificationURI while openBrowser was
handed verificationURL, the one with ?user_code= appended. Pressing Enter
therefore always worked, which is why this went unnoticed; copying the
printed URL instead landed on /device with no code.
That matters more than it looks, because the branch tests the wrong thing.
isTerminal(os.Stdin) asks whether stdin is a TTY, not whether a browser
exists, so an SSH session on a headless box takes the headed path and is
told to press Enter to open a browser it does not have. The non-interactive
branch, which already printed the full URL, is only reached by piping stdin.
Printing the code-carrying URL in both branches makes that mismatch moot
rather than requiring a smarter predicate.
Also give /device without a code the styled device-error page instead of
bare text/plain, matching the expired-code path beside it, and stop
renderError panicking when Templates is nil.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
digest_content.go branched on Error == "never-scanned" to pick the
"not scanned yet" copy, but nothing anywhere produced that string: both
FetchVulnDetails and FetchSbomDetails returned the human sentence
"No scan record found" for a missing record. So vulnReason and sbomReason
could never be "not-scanned", the friendly branches in vulns-section.html
and sbom-section.html were dead code, and every unscanned image fell
through to fetch-failed.
Free-tier accounts have scan_on_push off, so this was every image they
push, told "Scan data couldn't be loaded... try refreshing in a minute"
about something that had never been scanned and never would be by
refreshing. The digest page showed the raw internal string instead.
Replace the prose sentinel with a NotScanned bool the 404 path actually
sets, and give other non-200 statuses a distinct message so a 500 from the
hold stops being indistinguishable from an absent record. The detail
templates branch on it before Error, so nothing leaks the internal value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UAqi2hS2dhZoatqcWoYZQk
The link was hardcoded before fa34da0 too — it pointed at
bsky.app/profile/atcr.io. That commit was right to switch to a DID, since
handles change and a stale handle link breaks silently, but the DID went into
components/footer.html, a shared template. Every self-hoster's footer therefore
links to the project's Bluesky account.
Now ui.bluesky_profile, following source_url in the same footer exactly:
config field with a default, plumbed through UIDependencies and PageData, and
guarded with {{ with }} so an unset value omits the link rather than rendering
something wrong. It takes a handle or a DID; the comment says to prefer a DID
for the reason fa34da0 changed it.
Defaulting to the project account matches source_url's logic — both name the
upstream project rather than the operator — and self-hosters who want their own
or none set one line.
The aria-label switched to $.ClientShortName: `with` rebinds the dot, so the
label would otherwise have silently rendered empty.
Two things worth recording:
* `{{ with }}` hides the link on an empty value, but you cannot get an empty
value from the environment. Viper runs with AllowEmptyEnv(false), so an
empty env var reads as unset and the default wins. Only "" in YAML works.
That applies to every string field in this config, not just this one, and
the comment now says so.
* config-appview.example.yaml must NOT be regenerated with `config init`,
despite what the checklist in CLAUDE.md says. The file is hand-curated well
past the defaults, and regenerating replaces real Stripe price IDs with
price_xxx placeholders and blanks registry_domains, managed_holds, theme
and the tier names. Added by hand instead.
Verified live both ways: the configured value renders, and `bluesky_profile: ""`
in YAML drops the link while leaving the Source link intact.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
Same defect as the OCI path, two more places. The io.atcr.manifest record is
keyed by digest alone, so one record backs every repository of a DID holding
identical content, and both of these deleted it without asking whether another
repository still wants it — then purged the layers on the hold.
DeleteManifestHandler already removes this repository's tags before deleting
the record, so a tag remaining at that point can only belong to another
repository. It now checks IsManifestTaggedAnyRepo there and keeps the shared
record when one does, reporting sharedRecordKept so the caller can tell the
difference between "deleted" and "deliberately left alone".
DeleteUntaggedManifestsHandler is the subtler one. Its digest list comes from
GetAllUntaggedManifestDigests, whose tag join is scoped to one repository
(m.repository = t.repository), so a digest tagged only in a DIFFERENT
repository is reported as untagged and swept. The query is a reasonable
per-repository view and a dangerous delete list; the guard goes at the delete,
not in the query, matching how DeleteTagHandler already works. Skips are
counted separately from failures, because a skip is the guard working and
folding it into "failed" would make a correct run look broken.
Both fail closed. Leaving a manifest behind is recoverable; deleting one
another repository is still serving is not.
The new db test pins both halves of the interaction: that the query really does
report a cross-repo-tagged digest as untagged, so a change there is noticed,
and that IsManifestTaggedAnyRepo answers DID-wide, which is the thing actually
standing between the sweep and another repo's live image.
DeleteTagHandler needed no change — 2580dcd already routed it through
ShouldCascadeDeleteManifest, which is where the correct policy was written down.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
Completes the swap 0033 set up. layers and manifest_references move onto
manifest_key and manifests.id is gone, which removes the last node-allocated
identifier in the AppView schema.
Statement order in 0034 is load-bearing. With foreign keys on, DROP TABLE
performs an implicit DELETE FROM, so dropping manifests while layers still holds
an ON DELETE CASCADE reference deletes every layer row. Migration 0009 did
exactly that; it went unnoticed because the Jetstream backfill rebuilds layers
from PDS records, so the damage healed itself. PRAGMA foreign_keys is no help:
it is a no-op inside a transaction and migrations run in one. So the new
children are built pointing at manifests_new, the old children are dropped
first, and only then is the old manifests table dropped, by which point nothing
references it. Verified both behaviors before relying on them.
manifest_key is declared NOT NULL as well as PRIMARY KEY, because in SQLite a
PRIMARY KEY column still accepts NULL unless it is INTEGER PRIMARY KEY. That
constraint immediately caught four test helpers inserting manifests without one.
Five queries used MAX(id) as "the newest manifest in this repo", which I had
previously reported as absent after grepping only for ORDER BY. A derived key
has no ordering, so recency now comes from created_at with manifest_key as a
deterministic tiebreak. This is a real behavior change, and a fix: the two
disagree whenever a manifest is indexed out of order, which the backfill does
routinely, and created_at is the push time these queries always wanted. Both
directions are tested, including that ties resolve the same way every run.
InsertManifest and BatchInsertManifests no longer read anything back. The key is
derived from (did, repository, digest), so the writer knows it before the
statement runs: the select-back, its per-DID IN list, and the "manifest missing
id after batch insert" branch all go away, along with the UNIQUE-conflict
fallback that existed only to recover a rowid.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
io.atcr.manifest rkeys are the digest alone (digestToRKey), so a single
record backs every repository of a user holding identical content. Both
paths that cascade-delete that record checked for remaining tags scoped to
one repository, which asks the wrong question: a tag in another repo keeps
the shared record alive just as much as a tag in this one.
With me/a:v1 and me/b:v1 at the same digest, deleting me/a:v1 saw no
remaining tags in repo a, deleted the shared PDS record, and purged the
layers on the hold. me/b:v1 was left pointing at content that no longer
exists, and the firehose delete handler then cleared the rows for every
repo (DeleteManifest with an empty repository argument).
The collision predates this, but it was reachable only behind the opt-in
AutoRemoveUntagged profile flag. 1b91768 enabled OCI manifest DELETE and
made TagStore.Untag cascade unconditionally, which turned a latent metadata
collision into blob-level data loss on an ordinary skopeo/crane delete.
- cleanupUntaggedManifest no longer filters candidate tags to
rctx.Repository. The surrounding comment already noted that the tag
collection is account-wide; the filter contradicted it.
- ShouldCascadeDeleteManifest takes the tag question DID-wide via a new
IsManifestTaggedAnyRepo, and drops its now-meaningless repository
parameter.
- IsManifestTagged stays repository-scoped and keeps its caller: the
delete-manifest confirmation prompt is genuinely asking about the one
repo whose tags the user is about to remove.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The webhook limit was only checked at creation, so losing entitlement (a
hold switch or a plan downgrade) left previously-created webhooks firing
paid behavior forever.
- Dispatcher takes a WebhookLimiter, consulted on every dispatch. It
caps the list to the current allowance, keeping the oldest N to match
what the creation gate would have permitted, and masks paid trigger
bits.
- GetWebhooksForUser orders by created_at ASC, id ASC so that cap is
deterministic. ListWebhooks gets the same tiebreak: it feeds the
settings UI, and without it the list a user sees could disagree with
the one the dispatcher truncates.
- webhooks.FreeTriggerMask is shared by the creation gate and the
dispatch backstop so the two cannot drift.
Capping is logged when it actually truncates. The webhooks stay visible in
settings, so from the user's side delivery would otherwise just stop with
no signal — and the same line is the only evidence if the limiter itself
degraded, since a billing lookup failure falls back to free-tier limits
and would quietly demote a paying user mid-dispatch.
Two cost fixes, both because this puts the entitlement lookup on a hot
path it was never on before:
findCustomerByDID now consults the customer cache instead of always
issuing a Stripe customer search. GetWebhookLimits reaches it via
GetSubscriptionInfo on every delivery, so uncached it meant a
rate-limited Search API call for every push and every scan record of
every user with a webhook configured.
DispatchForQuota checks whether the user has any quota webhook at all
before fetching the allowance. The original code filtered first precisely
so the common path (no quota webhooks) did no work; taking the allowance
up front would have spent the expensive lookup on every push. The cap
itself is still computed over the full list, since the count limit spans
all webhook types.
Note DeliverTest is deliberately not capped: it is an explicit,
user-initiated "send test" from the settings page, not automatic delivery.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Entitlements were keyed on the Stripe subscription alone, so a subscriber
who switched to a self-hosted hold kept paying for features the appview
cannot deliver, and could still reach checkout.
- billing.ActiveHoldChecker and Manager.onManagedHold gate every
entitlement. An empty default hold counts as managed: the user has no
explicit preference and falls back to the operator's primary managed
hold.
- The checker reads the primary DB, not the read replica. A hold switch
writes default_hold_did to the primary, and replica lag would keep
paid features alive after a switch away.
- db.GetUserDefaultHoldDID is the clean default-hold signal, unlike
GetUserHoldDID which falls back to a manifest hold_endpoint (a URL,
not a DID).
- Jetstream fails closed: an unresolvable hold reference is cached raw
rather than left empty, since an empty value reads as managed.
- UI: the billing tab is hidden on self-hosted, a cancel/manage banner
appears when a self-hosted user still has an active plan, the image
advisor returns managed_hold_required instead of upgrade_required,
and the checkout route returns 403. The portal stays open so existing
subscribers can still cancel.
Two consistency fixes fall out of wiring this up:
The settings UI reads the resolved default_hold_did rather than the raw
profile.DefaultHold. The profile field is the record value as written and
may be a URL-form reference; jetstream resolves it to a DID on the way
into the DB, and the server-side gate reads that resolved value. Comparing
the raw form against managed DIDs would show the "you are self-hosted"
banner and hide billing from a user whose entitlements say otherwise.
HasAIAdvisor falls back to the free tier's AIAdvisor setting when
off-managed instead of a hard false, matching GetWebhookLimits. Losing a
managed hold should drop a user to free-tier entitlements, not below them.
BEHAVIOR CHANGE for existing paying users on self-hosted holds: they lose
the AI advisor, supporter badge and paid webhook limits as soon as this
deploys, while Stripe keeps charging them. The only notice is the banner
on /settings/storage, which they have to visit to see. Decide on a
migration (notification, or a one-time reconciliation over active
subscriptions) before shipping this.
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>
DeleteManifestHandler built the tag rkey as "repo:tag" while the write
path uses RepositoryTagToRKey, which is "repo_tag" with "/" encoded as
"~". For a nested repo like stream/cache the two never match, so the
cascade leaves the tag record on the PDS while removing the local cache
row, and the tag reappears on the next backfill. Depending on the
variant it either no-ops (deleteRecord is idempotent) or fails outright
on an rkey containing a slash.
Every other io.atcr.tag call site already routes through the helper.
This was the last hand-built one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 6e77311. Listing the operated holds on /privacy put
resolveHoldDisplayName in the render path, where it makes up to two sequential
network calls per DID: the DID document fetch, then handle verification. /privacy
is public and unauthenticated, and the server sets no HTTP read or write timeout,
so an unreachable hold or a plc.directory outage stalled the page for every
visitor. The identity directory's negative-cache TTL is short, so the stall
recurred rather than settling after the first hit.
The DIDs come from config and never change while the process runs, so resolution
happens once in a background goroutine and lands in an atomic.Pointer. The
handler is constructed once at route registration, so the cache is process-wide.
Until resolution completes the page renders offline names, which are already
correct for did:web holds since those decode straight from the DID.
Dropped the did:plc truncation. resolveHoldDisplayName's last fallback cut a DID
to 24 characters plus an ellipsis, which is shorter than a did:plc, so the result
could not be resolved back to a hold. That is tolerable in a settings dropdown
and actively misleading in a privacy policy naming the services we operate.
Shortening for display belongs in the template. The non-network fallbacks are now
in holdDisplayNameOffline so the background resolver and the render path share
them.
Also fixed the surrounding copy, which scoped coverage to *.<site> domains while
the list immediately above it could contain holds on other domains — the two
sentences contradicted each other. It now refers to the listed holds, and only
makes that claim when there is a real list; with no managed holds configured the
template still shows an illustrative placeholder, which must not be presented as
fact in a legal document.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 37bab32. That commit stopped deleting OAuth sessions on transient
errors, which fixed spurious sign-outs but overshot on one path: a genuinely dead
session stopped being evicted at all, turning a forced re-login into a permanent
failure loop.
GetOrFetchServiceToken flattened every non-200 from getServiceAuth into
fmt.Errorf("service auth failed with status %d: %s"). IsSessionInvalidError then
had nothing structured to inspect, and its string fallback could not help: it
looks for the OAuth 2.0 code invalid_token, while atproto emits the XRPC name
InvalidToken. The difference is the underscore, not the case, so lowercasing
never bridged it. A revoked session came back 401 InvalidToken and was classified
transient, so /auth/token returned 503 forever and the user was never prompted to
re-authenticate.
The non-200 branch now wraps an *atclient.APIError carrying the status and the
parsed atproto error name, which is what the existing structured checks in
IsSessionInvalidError already know how to read. Transient shapes stay transient:
atprotoErrorName returns "" for a non-JSON body, so 500s with HTML, 502s, and
429s do not evict.
ExpiredToken is deliberately not treated as a dead session. It means "refresh
me", and deleting on it would sign the user out of every UI session over an
ordinary access-token expiry a refresh would have fixed. isAuthError omits it for
the same reason; the two classifiers have to agree about the same condition.
The comment on the string fallback claimed it was a looser spelling of the
structured check. It is not — it handles a different error family. indigo's
RefreshTokens returns OAuth token-endpoint failures as a bare fmt.Errorf carrying
the auth server's snake_case code verbatim ("token refresh failed (HTTP 400):
invalid_grant"), never a typed error, so a string match is the only thing that
can classify a refresh failure, which is the invalid_grant replay case 37bab32
exists to detect. Both comments now say which family they cover.
Two hardening items on the same theme:
use_dpop_nonce no longer counts as an auth error in the appview's isOAuthError.
It is a routine handshake step indigo retries with the server-supplied nonce, and
treating it as fatal signed users out over ordinary nonce rotation. It can still
escape when a server sends that error with no DPoP-Nonce header, leaving indigo
nothing to retry with; a stuck session there is preferable to signing everyone
out in the common case, and the comment says so rather than claiming it cannot
happen.
Detached session deletes are bounded by SessionDeleteTimeout. They run on
context.WithoutCancel so a canceled request cannot leave the cleanup half-done,
which also stripped the only deadline they had — a wedged database write blocked
the goroutine with no way to shed it. Matches the bound already on the detached
persist callback. The unparseable-token-endpoint warning is now deduped per
endpoint rather than once per process, since that path fails open by returning
the client unwrapped, silently reinstating the refresh burn.
The refreshDetachTimeout comment now notes the cap is per-POST: the DPoP-nonce
retry means one refresh can issue two, holding the per-DID lock for up to twice
the stated value.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When a Docker client canceled a slow /auth/token request mid-refresh, the
token-refresh POST was aborted client-side but completed on the PDS, which
rotated the refresh token. The rotated token was never received or persisted,
so the next refresh replayed the consumed token, got invalid_grant, and the
session (OAuth + UI) was deleted, signing the user out everywhere.
- Detach refresh POSTs from the inbound request context via a per-session
RoundTripper (WithoutCancel + 30s cap); once a refresh starts it completes
- Persist session updates (rotated tokens, DPoP nonces) on a detached context
- Gate session deletion on IsSessionInvalidError: cancellation, timeouts, and
transport errors no longer delete sessions; genuine invalid_grant still does
- Add phase timing to /auth/token and per-DID lock wait warnings to attribute
the ~14s pre-refresh stalls that push requests past Docker's deadline
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- 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.5