chore(datastore): compose the npm file and dist-tag leaf deletes

Hayley Swimelarrequested to merge
313/step-13-npm-file-dist-tag-composers into main

Stacked on chore(datastore): count every table that refere... (!1450 - merged) • Hayley Swimelar • 19.3, merges after it.

Why

The management API needs a delete for every addressable artifact. The two npm leaves, a file and a dist-tag, are the targets a request can finish synchronously: each is one row, nothing references it, and a file's blob attachment goes with it. This adds the datastore composers those two routes will call, on the widened attachment reference check the parent MR lands. No route reaches them yet, so nothing a caller can observe changes.

Step 13 of the S17 Phase 4 plan.

What is worth a second look

The force-expiry runs before the attachment reclaim, inverting the order the plan's Step 13 sentence lists. The canonical lock order documented on RotatePackumentRebuildTokenTx is npm_packages, then npm_versions/npm_files/npm_tags, then npm_metadata_files, then blob_storage_attachments, and the packument rebuild's own upsert takes npm_metadata_files then attachments. Reclaiming first would put this composer opposite the rebuild and reintroduce the ABBA deadlock that comment exists to prevent. Both statements commit together, so only the lock order differs.

DeleteNpmFile carries no soft_deleted_at predicate, deliberately. A marked row still holds its attachment, so filtering marked rows here would strand it where nothing can reach it, which under ADR-010 leaves the blob uncollectable. A unit test pins the predicate's absence.

packageIDByVersion and lockTagNameByID live in the composer rather than on NpmVersionStore and NpmTagStore. Each reads the one column a following statement in the same transaction needs, under a predicate chosen for that statement, so neither shape is one those stores' own callers would want. Steps 11, 12, and 15 stack on this same parent and touch the neighboring store files, so publishing these two there would collide for no gain. Named as an accepted trade-off rather than left for each reviewer to rediscover.

DeleteFile returns error alone while DeleteDistTagByID returns existed. The file handler resolves through the chain-verifying detail read first and takes its 404 from there. The dist-tag handler has no separate resolve, so it needs the bit.

Four invariants gained guards after mutation testing. Deleting the FOR UPDATE from the tag resolve, or the namespace_id predicate from either resolve, left every subtest green. So did swapping ForceExpireNpmMetadataTx for DeleteNpmMetadataFiles, which compiles (same store, identical signature) and destroys three cache rows per package while stranding their attachments. Each mutation now fails.

Size: 1,731 reviewable LOC, about 600 production against 1,130 tests. Splitting the store primitive from its composer would leave a bottom MR whose one method has no caller, which costs a reviewer more than the line count saves.

The plan's Status row is deliberately empty. Steps 11, 12, 13, and 15 stack on this same parent and would collide on those rows, so they are filled once in a follow-up.

Spec coverage

Spec: S17. Phase 4 numbering is positional from AC #30.

# Criterion Coverage
AC-31 Delete returns 202, the target then 404s, a missing target 404s Composer slice: the row goes, and existed is the split the dist-tag handler's 404 reads. Status codes are Steps 25 and 26.
AC-32 Each delete removes its subtree, which for these two leaves is the row only Sibling file of the same version, same-named file of another version, and same-named tag of another package all survive.
AC-36 An npm file delete expires the package's npm_metadata_files cache in the same transaction Asserted on every hit across all three metadata kinds, and the cache is left fresh on every miss arm. The version half is Step 12.
AC-37 An npm package's last active version removes the package Step 12's composer. No leaf delete can empty a package.
AC-38 Whatever removes a row removes its blob_storage_attachments row in the same step, and a marked but unreaped artifact still holds its attachment Attachment gone on the hit, including under a marked version and a marked package. An attachment a marked sibling row still references survives.
AC-39 No delete of a non-empty artifact returns 409 A leaf has no children and nothing outside its subtree references it, so no arm can conflict. Handler-level, Steps 25 and 26.
AC-49 A management delete leaves versions_count and tags_count where the equivalent protocol operation leaves them The composer writes no counter, pinned. The buffered decrement is Step 26's.
AC-50 One artifact_registry_artifact_deleted per named artifact, and none for tag operations Handler-level, Steps 25 and 26. No composer emits an event.

Error cases and security considerations are covered in the same shape: every miss is a nil-error no-op rather than a sentinel, so status mapping stays the handler's, and every statement binds namespace_id. Those two tables are in commit 738185b5's body.

Test plan

go test ./internal/datastore/
go test -tags=integration -count=1 ./internal/datastore/
golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/datastore/

TestNpmLeafDeleter_DeleteFile and TestNpmLeafDeleter_DeleteDistTagByID carry the behavior. TestDeleteNpmFileStmt pins the generated SQL, where the version scope, the absent soft_deleted_at predicate, and the RETURNING that carries the attachment coordinates are visible. Integration tests need no local Postgres: with ARTIFACT_REGISTRY_DATABASE_TEST_DSN unset, testutil.SetupIsolatedTestDB provisions a container.

Context for LLM agents

Rejected alternatives.

  • Fold the version resolve into the DELETE with USING npm_versions. One statement instead of two, but the package id is then known only after the file row is gone, and the rotation has to precede every other write. Learning it from the delete inverts the lock order.
  • Delete the dist-tag by id instead of resolve-then-delete-by-name. Fewer statements, but the plan specifies reusing NpmTagStore.DeleteNpmTag, and adding a by-id delete means editing npm_tags.go, which sibling steps are working next to. The FOR UPDATE on the resolve is what makes the by-name delete remove exactly the addressed row.
  • RunInTx instead of a hand-rolled envelope. Shorter, but it opens with BeginTx(ctx, nil) and states no isolation level, and the sibling management composers in Steps 11 and 12 name READ COMMITTED. The cost of the hand-rolled form is duplication: a6b579d5 added the recover-and-roll-back arm runInSQLTx already carries, so the two envelopes now hold the same discipline and have to stay in step by hand.
  • Return DeleteIfUnreferenced's row count. No consumer exists, and a count nothing reads invites a caller to treat zero as an error when it also means "already absent".

Non-goals. No handler, route, wiring, consumer interface, or migration: Steps 25 and 26 own the two routes. No tags_count decrement and no usage event, both of which are the handler's, and a test pins that this layer leaves the counter alone. No EXPLAIN evidence: all three statements are single-row lookups on the primary key with namespace_id bound, so every plan is a single-partition index scan.

Follow-ups this MR does not take, each needing a file outside its declared set.

  • The rule wording on RotatePackumentRebuildTokenTx states its boundary as "before it touches any other table", while what deadlocks is a row lock. An unlocked read ahead of the rotation is admitted, and the comment here records why.
  • DeleteIfUnreferenced's doc named ContainerManifestDeleter as its only caller. The parent branch took this one in 852a2fb0, which states the contract instead.
  • blobSHA256's doc reasons from container_manifests alone, though npm_files carries the same 32-byte CHECK.
  • npm_files.go keeps five sprint citations in doc comments this MR did not touch.

Database Review Evidence

No migration files changed, so migration mode did not run. Query mode covers the three statements this MR adds.

Four further statements ride the composer's transactions without being planned here, because their SQL is unchanged and lives in files this MR does not touch: RotatePackumentRebuildTokenTx, ForceExpireNpmMetadataTx, NpmTagStore.DeleteNpmTag, and BlobStorageAttachmentStore.DeleteIfUnreferenced (the last widened on the parent branch, planned under chore(datastore): count every table that refere... (!1450 - merged) • Hayley Swimelar • 19.3).

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17.10 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.NpmFileStore.DeleteNpmFile Delete -> Index Scan npm_files_p51_pkey 1 / 1 8.30 0.176ms 5 / 0 1 of 64
datastore.NpmLeafDeleter.lockTagNameByID Limit -> LockRows -> Index Scan npm_tags_p07_pkey 1 / 1 8.31 0.030ms 4 / 0 1 of 64
datastore.NpmLeafDeleter.packageIDByVersion Limit -> Index Scan npm_versions_p62_pkey 1 / 1 8.30 0.021ms 3 / 0 1 of 64
datastore.NpmFileStore.DeleteNpmFile

Summary: Plan matches the method's intent. The delete finds its row through the partition's primary key on (id, namespace_id), prunes to one of 64 partitions, and applies npm_version_id as a filter, which is the scoping arm rather than the access path. Estimates match actuals (1 / 1) with no heap reads. Reported at two cardinalities on purpose: at 50 rows the planner picks a Seq Scan over a single-page partition, which is a small-table artifact rather than a missing index, and the same statement takes the index once the partition holds 5000 rows. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_repositories=1, npm_packages=1, npm_versions=1, blob_storage_blobs=5000, blob_storage_attachments=5000, npm_files=5000

Rendered SQL:

DELETE FROM public.npm_files
WHERE ((npm_files.namespace_id = $1::uuid) AND (npm_files.npm_version_id = $2::uuid)) AND (npm_files.id = $3::uuid)
RETURNING npm_files.blob_storage_attachment_id AS "npm_files.blob_storage_attachment_id",
          npm_files.blob_sha256 AS "npm_files.blob_sha256";

Bound args: [1b6e5292-a240-4099-ac04-ce8f89cc1d41, e0aaa15b-a1b9-49c9-879f-d6abcad12131, 03dc67ff-4b62-49db-a790-3da24003a44d]

Plan (EXPLAIN (ANALYZE, BUFFERS) output, 5000 rows in the target partition):

 Delete on npm_files  (cost=0.28..8.30 rows=1 width=10) (actual time=0.017..0.018 rows=1 loops=1)
   Delete on npm_files_p51 npm_files_1
   Buffers: shared hit=5
   ->  Index Scan using npm_files_p51_pkey on npm_files_p51 npm_files_1  (cost=0.28..8.30 rows=1 width=10) (actual time=0.010..0.010 rows=1 loops=1)
         Index Cond: ((id = '03dc67ff-4b62-49db-a790-3da24003a44d'::uuid) AND (namespace_id = '1b6e5292-a240-4099-ac04-ce8f89cc1d41'::uuid))
         Filter: (npm_version_id = 'e0aaa15b-a1b9-49c9-879f-d6abcad12131'::uuid)
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=88
 Planning Time: 0.485 ms
 Execution Time: 0.176 ms

Plan at the write-target default of 50 rows, kept to show why the larger seed is the representative one:

 Delete on npm_files  (cost=0.00..1.88 rows=1 width=10) (actual time=0.019..0.022 rows=1 loops=1)
   Delete on npm_files_p02 npm_files_1
   Buffers: shared hit=3
   ->  Seq Scan on npm_files_p02 npm_files_1  (cost=0.00..1.88 rows=1 width=10) (actual time=0.010..0.013 rows=1 loops=1)
         Filter: ((namespace_id = 'f38e5eb3-6548-42ab-82df-91ff2aaffa1d'::uuid) AND (npm_version_id = 'a9c4079e-9994-4aec-9571-687d546ab0b4'::uuid) AND (id = '80f88c9e-a021-42ab-9c00-c244040aadf1'::uuid))
         Rows Removed by Filter: 49
         Buffers: shared hit=1
 Planning:
   Buffers: shared hit=273
 Planning Time: 1.981 ms
 Execution Time: 0.313 ms

Timings: planning 0.485ms, execution 0.176ms, total 0.661ms.

datastore.NpmLeafDeleter.lockTagNameByID

Summary: Plan matches the method's intent. The row lock shows as a LockRows node above an Index Scan on the partition's primary key, confirming the FOR UPDATE that ties the following by-name delete to the row this resolve read. npm_package_id lands as a filter rather than an index condition, which is the cross-package scoping arm and needs no index of its own, since the primary key already identifies one row. One partition of 64, estimates match actuals, no heap reads. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_repositories=1, npm_packages=1, npm_versions=1, npm_tags=5000

Rendered SQL:

SELECT npm_tags.name AS "npm_tags.name"
FROM public.npm_tags
WHERE ((npm_tags.namespace_id = $1::uuid) AND (npm_tags.npm_package_id = $2::uuid)) AND (npm_tags.id = $3::uuid)
LIMIT $4
FOR UPDATE;

Bound args: [16460551-bf22-4efd-9d01-e7ce4db2b502, 08cf3eb8-a023-4bab-a8b6-01e9b3f588b8, 2beebba1-f1cc-43c5-bc22-e74340dca1cb, 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.31 rows=1 width=21) (actual time=0.013..0.014 rows=1 loops=1)
   Buffers: shared hit=4
   ->  LockRows  (cost=0.28..8.31 rows=1 width=21) (actual time=0.013..0.013 rows=1 loops=1)
         Buffers: shared hit=4
         ->  Index Scan using npm_tags_p07_pkey on npm_tags_p07 npm_tags  (cost=0.28..8.30 rows=1 width=21) (actual time=0.010..0.010 rows=1 loops=1)
               Index Cond: ((id = '2beebba1-f1cc-43c5-bc22-e74340dca1cb'::uuid) AND (namespace_id = '16460551-bf22-4efd-9d01-e7ce4db2b502'::uuid))
               Filter: (npm_package_id = '08cf3eb8-a023-4bab-a8b6-01e9b3f588b8'::uuid)
               Buffers: shared hit=3
 Planning:
   Buffers: shared hit=256
 Planning Time: 0.942 ms
 Execution Time: 0.030 ms

Timings: planning 0.942ms, execution 0.030ms, total 0.972ms.

datastore.NpmLeafDeleter.packageIDByVersion

Summary: Plan matches the method's intent. The resolve reads one column through the partition's primary key with the namespace literal pruning to one of 64 partitions, and takes no row lock, which is what lets the transaction reach npm_packages for the rotation without having locked npm_versions first. Estimates match actuals at 5000 seeded versions, execution stays at 0.021ms, and no buffer is read from disk. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_repositories=1, npm_packages=1, npm_versions=5000

Rendered SQL:

SELECT npm_versions.npm_package_id AS "npm_versions.npm_package_id"
FROM public.npm_versions
WHERE (npm_versions.namespace_id = $1::uuid) AND (npm_versions.id = $2::uuid)
LIMIT $3;

Bound args: [f177fe70-5e78-4e0a-be3f-42849f1642fe, 58a56cc5-8823-4720-adea-598b370a2877, 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.30 rows=1 width=16) (actual time=0.009..0.010 rows=1 loops=1)
   Buffers: shared hit=3
   ->  Index Scan using npm_versions_p62_pkey on npm_versions_p62 npm_versions  (cost=0.28..8.30 rows=1 width=16) (actual time=0.009..0.009 rows=1 loops=1)
         Index Cond: ((id = '58a56cc5-8823-4720-adea-598b370a2877'::uuid) AND (namespace_id = 'f177fe70-5e78-4e0a-be3f-42849f1642fe'::uuid))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=280
 Planning Time: 0.926 ms
 Execution Time: 0.021 ms

Timings: planning 0.926ms, execution 0.021ms, total 0.947ms.

Related to #313 (closed)

Edited by Hayley Swimelar

Merge request reports