mirror of
https://tangled.org/evan.jarrett.net/at-container-registry
synced 2026-09-02 00:06:58 +00:00
test: cover the nested-repo tag rkey on the delete paths
c035f50fixed a hand-built tag rkey in DeleteManifestHandler and shipped with no test. The hazard is not specific to that handler: io.atcr.tag rkeys come from RepositoryTagToRKey, which encodes "/" as "~", so any code building one by hand targets a record that does not exist — and deleteRecord being idempotent makes that a silent no-op. The local view looks right and the tag returns on the next backfill. The by-digest path now builds tag rkeys too (594d73b), so it could reintroduce exactly this bug. TestManifestDelete_NestedRepoTagRKey pins it there: push to stream/cache, delete by digest, and assert the tag is no longer listed. Listing is what catches a survivor — TagStore.All reads the records back from the PDS and filters by repository, so a stale one is still reported. Mutation-verified by hand-building the rkey as "repo:tag": the nested test fails with the tag still listed, and TestManifestDelete passes unchanged. That second half is the point — every existing delete test uses a flat repository name, and a flat name cannot reproduce this bug at all. batch11-nested-rkey.mjs drives the same property through the UI handler thatc035f50actually fixed, asserting against the PDS record rather than the page, since the page looks correct either way until a backfill runs. It needs an interactive appview login in the Playwright profile and is not yet run; the session that exists belongs to a different browser profile. Two instrument notes are baked in: probe /settings rather than the repo page to detect a session, because /r/ renders for anonymous visitors and can never report a missing one, and use maxRedirects:0, because RequireAuth 302s and a followed redirect surfaces as a confusing 405 on DELETE /login. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
This commit is contained in:
co-authored by
Claude Opus 5
parent
0b212a527f
commit
b17ebb69a5
@@ -0,0 +1,86 @@
|
||||
// batch11-nested-rkey.mjs — the drive check for c035f50.
|
||||
//
|
||||
// DeleteManifestHandler used to hand-build the tag rkey as "repo:tag" while the
|
||||
// write path uses RepositoryTagToRKey ("repo_tag", with "/" encoded as "~").
|
||||
// For a nested repository like stream/cache the two never match, so the delete
|
||||
// removed the local cache row while leaving the io.atcr.tag record on the PDS —
|
||||
// and the tag reappeared on the next backfill.
|
||||
//
|
||||
// A flat repo name cannot reproduce this: the bug lives entirely in the "/"→"~"
|
||||
// encoding. The assertion is therefore against the PDS record, not the page:
|
||||
// the UI looks correct either way until a backfill runs.
|
||||
//
|
||||
// Needs the signed-in profile (lib.mjs PROFILE) because the delete endpoint is
|
||||
// session-authenticated. Drive it via ctx.request so the cookie jar and UA come
|
||||
// from the browser — see README, a mismatched UA deletes the session.
|
||||
import { chromium } from '@playwright/test';
|
||||
import { APPVIEW, PROFILE } from './lib.mjs';
|
||||
|
||||
const HANDLE = process.env.ATCR_E2E_HANDLE ?? 'evan.jarrett.net';
|
||||
const DID = process.env.ATCR_E2E_DID ?? 'did:plc:pddp4xt5lgnv2qsegbzzs4xg';
|
||||
const PDS = process.env.ATCR_E2E_PDS ?? 'https://jarrett.app';
|
||||
const REPO = process.env.ATCR_E2E_NESTED_REPO ?? 'stream/cache';
|
||||
const TAG = 'v1';
|
||||
|
||||
let pass = 0, fail = 0;
|
||||
const check = (label, ok, detail = '') => {
|
||||
console.log(` ${ok ? 'ok ' : 'FAIL'} ${label}${detail ? ' — ' + detail : ''}`);
|
||||
ok ? pass++ : fail++;
|
||||
};
|
||||
|
||||
const tagRecords = async () => {
|
||||
const r = await fetch(`${PDS}/xrpc/com.atproto.repo.listRecords?repo=${DID}&collection=io.atcr.tag&limit=100`);
|
||||
const d = await r.json();
|
||||
return (d.records ?? []).filter((rec) => rec.value?.repository === REPO);
|
||||
};
|
||||
|
||||
const before = await tagRecords();
|
||||
check(`the nested repo has a tag record to delete`, before.length > 0,
|
||||
before.map((r) => r.uri.split('/').pop()).join(', ') || 'none — push it first');
|
||||
if (before.length === 0) process.exit(1);
|
||||
// Confirms the write path encodes the slash, which is what the delete path has
|
||||
// to match. If this ever reads "stream/cache_v1" the encoding changed and the
|
||||
// rest of this check is meaningless.
|
||||
check('the rkey is slash-encoded', before[0].uri.split('/').pop().includes('~'),
|
||||
before[0].uri.split('/').pop());
|
||||
|
||||
const ctx = await chromium.launchPersistentContext(PROFILE, {
|
||||
headless: process.env.ATCR_E2E_HEADLESS === '1',
|
||||
viewport: null,
|
||||
});
|
||||
const page = ctx.pages()[0] ?? (await ctx.newPage());
|
||||
// Probe /settings, NOT the repo page: /r/ renders for anonymous visitors, so
|
||||
// "did not redirect to login" there is true whether or not there is a session,
|
||||
// and the delete then fails with a confusing 405 (RequireAuth 302s, the client
|
||||
// follows it, and DELETE /login is 405).
|
||||
await page.goto(`${APPVIEW}/settings`, { waitUntil: 'networkidle' });
|
||||
const signedIn = !/\/login/.test(page.url());
|
||||
check('signed in (probed against /settings, which requires auth)', signedIn, page.url());
|
||||
if (!signedIn) {
|
||||
console.error('\n Session is gone. Sign in at ' + APPVIEW + ' in the profile at ' + PROFILE);
|
||||
await ctx.close();
|
||||
process.exit(2);
|
||||
}
|
||||
await page.goto(`${APPVIEW}/r/${HANDLE}/${REPO}`, { waitUntil: 'networkidle' });
|
||||
|
||||
const digest = (await page.content()).match(/sha256:[a-f0-9]{64}/)?.[0];
|
||||
check('found the manifest digest on the page', !!digest, digest?.slice(0, 19) + '…');
|
||||
|
||||
// confirm=true: the manifest is tagged, and without it the handler returns 409
|
||||
// asking for confirmation rather than deleting.
|
||||
const resp = await ctx.request.delete(`${APPVIEW}/api/manifests`, {
|
||||
data: { repo: REPO, digest, confirm: true },
|
||||
maxRedirects: 0,
|
||||
});
|
||||
check('DELETE /api/manifests accepted', resp.ok(), `HTTP ${resp.status()} ${(await resp.text()).slice(0, 120)}`);
|
||||
|
||||
// The PDS is the authority here. The local cache row goes either way; only the
|
||||
// record surviving tells you the rkey was wrong.
|
||||
await new Promise((r) => setTimeout(r, 3000));
|
||||
const after = await tagRecords();
|
||||
check('the io.atcr.tag record is gone from the PDS', after.length === 0,
|
||||
after.length ? `still present: ${after.map((r) => r.uri.split('/').pop()).join(', ')}` : 'removed');
|
||||
|
||||
console.log(`\npassed ${pass}, failed ${fail}`);
|
||||
await ctx.close();
|
||||
process.exit(fail === 0 ? 0 : 1);
|
||||
@@ -5,6 +5,7 @@ package integration
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"slices"
|
||||
"testing"
|
||||
|
||||
"github.com/google/go-containerregistry/pkg/crane"
|
||||
@@ -173,3 +174,61 @@ func TestManifestDelete_SharedDigestAcrossRepos(t *testing.T) {
|
||||
t.Fatalf("repo B is no longer pullable after repo A was deleted: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// TestManifestDelete_NestedRepoTagRKey covers the encoding hazard c035f50 fixed
|
||||
// in the UI handler, on the by-digest path.
|
||||
//
|
||||
// io.atcr.tag rkeys come from RepositoryTagToRKey, which encodes "/" as "~", so
|
||||
// stream/cache:v1 is stored as "stream~cache_v1". Any code that hand-builds the
|
||||
// rkey ("repo:tag", "repo_tag") targets a record that does not exist, and
|
||||
// because deleteRecord is idempotent it fails silently: the local view looks
|
||||
// right and the tag comes back on the next backfill.
|
||||
//
|
||||
// A flat repository name cannot reproduce it — the bug is entirely in the "/"
|
||||
// encoding — which is why every earlier delete test missed it. Listing tags
|
||||
// after the delete is what catches a surviving record: TagStore.All reads the
|
||||
// records back from the PDS and filters by repository, so a stale one is still
|
||||
// reported.
|
||||
func TestManifestDelete_NestedRepoTagRKey(t *testing.T) {
|
||||
h := testharness.New(t)
|
||||
alice := h.AddSailor("alice.test")
|
||||
creds := h.RegistryCreds(alice)
|
||||
authOpts := []crane.Option{crane.WithAuth(toAuthn(creds)), crane.Insecure}
|
||||
|
||||
repo := fmt.Sprintf("%s/%s/stream/cache", h.AppViewHostPort(), alice.Handle())
|
||||
tagRef := repo + ":v1"
|
||||
|
||||
img, err := random.Image(1<<20, 2)
|
||||
if err != nil {
|
||||
t.Fatalf("build image: %v", err)
|
||||
}
|
||||
if err := crane.Push(img, tagRef, authOpts...); err != nil {
|
||||
t.Fatalf("push nested: %v", err)
|
||||
}
|
||||
|
||||
tags, err := crane.ListTags(repo, authOpts...)
|
||||
if err != nil {
|
||||
t.Fatalf("list tags before delete: %v", err)
|
||||
}
|
||||
if !slices.Contains(tags, "v1") {
|
||||
t.Fatalf("fixture is wrong: v1 not listed before delete, got %v", tags)
|
||||
}
|
||||
|
||||
dgst, err := img.Digest()
|
||||
if err != nil {
|
||||
t.Fatalf("digest: %v", err)
|
||||
}
|
||||
if err := crane.Delete(fmt.Sprintf("%s@%s", repo, dgst.String()), authOpts...); err != nil {
|
||||
t.Fatalf("delete nested by digest: %v", err)
|
||||
}
|
||||
|
||||
tags, err = crane.ListTags(repo, authOpts...)
|
||||
if err != nil {
|
||||
t.Fatalf("list tags after delete: %v", err)
|
||||
}
|
||||
if slices.Contains(tags, "v1") {
|
||||
t.Errorf("tag v1 survived the delete on a nested repository: %v — the tag rkey did not match "+
|
||||
"the slash-encoded key the write path uses, so the record is still on the PDS and will "+
|
||||
"reappear on the next backfill", tags)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user