db: cover the DID guard on the batched tag delete

TestDeleteTagsNotInListScopedToDID passes a nil keep list, which takes the
early return and deletes with `DELETE FROM tags WHERE did = ?`. So the scoping
it proves is the shortcut's, not the one inside the chunk loop that f186760
rewrote, and the batched path's `did = ?` had no test at all. Removing it in a
scratch worktree left every existing test green.

The new case gives a second user the same repository:tag pairs and passes a
non-empty keep list, so the chunk loop runs and its delete set collides with
the other user's rows. Verified against the defect: with the guard replaced by
a no-op predicate, 104 of the other user's 105 tags are destroyed.

Two users owning the same repository:tag is the ordinary case rather than a
contrived one, and the blast radius is one user's tag sync silently deleting
another's rows for every name they share.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011rmjvU2gSRL9wFnmqVsWaF
This commit is contained in:
Evan Jarrett
2026-08-25 16:34:26 -05:00
co-authored by Claude Opus 5
parent 18c77ace28
commit f0b28c04f5
+64
View File
@@ -148,6 +148,70 @@ func TestDeleteTagsNotInListScopedToDID(t *testing.T) {
}
}
// TestDeleteTagsNotInListBatchPathScopedToDID covers the DID guard on the
// batched delete, which the test above cannot reach.
//
// A nil keep list takes the early return, so the scoping proven there is the one
// on `DELETE FROM tags WHERE did = ?`, not the one inside the chunk loop. Two
// users owning the same repository:tag is the ordinary case, not a contrived
// one, and if the chunk loop ever stopped scoping by DID then one user's tag
// sync would delete the other's rows for every name they happen to share.
func TestDeleteTagsNotInListBatchPathScopedToDID(t *testing.T) {
database := revTestDB(t)
const (
repository = "myapp"
total = BatchSize + 5 // more than one chunk, so every chunk is checked
otherDID = "did:plc:someoneelse"
)
did := seedTags(t, database, repository, total)
// The other user owns exactly the same repository:tag pairs.
if err := UpsertUser(database, &User{
DID: otherDID,
Handle: "other.example.com",
PDSEndpoint: "https://pds.example.com",
LastSeen: time.Now(),
}); err != nil {
t.Fatalf("UpsertUser: %v", err)
}
for i := range total {
if err := UpsertTag(database, &Tag{
DID: otherDID,
Repository: repository,
Tag: fmt.Sprintf("v%d", i),
Digest: fmt.Sprintf("sha256:%064d", i),
CreatedAt: time.Now(),
}); err != nil {
t.Fatalf("UpsertTag for the other user: %v", err)
}
}
// Keep one tag, so the keep list is non-empty and the delete set collides
// with every one of the other user's rows but one.
keep := []struct{ Repository, Tag string }{{repository, "v0"}}
if err := DeleteTagsNotInList(database, did, keep); err != nil {
t.Fatalf("DeleteTagsNotInList: %v", err)
}
remaining, err := GetTagsForDID(database, did)
if err != nil {
t.Fatalf("GetTagsForDID: %v", err)
}
if len(remaining) != 1 {
t.Errorf("owner kept %d tags, want 1", len(remaining))
}
otherTags, err := GetTagsForDID(database, otherDID)
if err != nil {
t.Fatalf("GetTagsForDID for the other user: %v", err)
}
if len(otherTags) != total {
t.Errorf("another user's tags were deleted by the batched path: %d remain, want %d",
len(otherTags), total)
}
}
// TestTagNaturalKeyRejectsDuplicates: (did, repository, tag) is the primary key
// now, so a repeated upsert must update in place rather than adding a row.
func TestTagNaturalKeyRejectsDuplicates(t *testing.T) {