chore(datastore): compose the id-keyed npm version delete

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

Why

The management API addresses an npm version by id, so it cannot call the S11 version-unpublish deleter: that deleter is rev-gated and derives its target by set difference against a submitted packument, both protocol concerns the management surface does not have. What is reusable is the layer beneath it, so this composes the same store writers in the same order and the same transaction shape. The two surfaces then cannot disagree on what a version delete leaves behind, which is the equivalence the spec asks for.

Step 12 of the S17 Phase 4 plan. No route reaches the composer yet, so the service's behavior is unchanged. Step 24 lands the handler that calls it.

What

Three ordering and vocabulary choices a reviewer would otherwise have to reverse-engineer.

• A target the chain probe does not resolve is a no-op success, not an error. ActiveNpmVersionExistsTx scopes by namespace, package, and version and filters marked rows, so an already-marked version, a version id no row carries, and a version belonging to another package collapse to one outcome: write nothing, commit, return (0, false, nil). That is what makes the composer idempotent on an already-marked target, which the plan requires, and it reuses the existing probe rather than adding a fourth existence-probe variant to this package. The probe checks the namespace and package legs only, so a consistent (package, version) pair from another repository of the same namespace does resolve here. The repository leg is the handler's resolve, which is where the spec puts it, and the doc comment says so.
• The probe precedes the rotation. It is a plain non-locking SELECT, so it cannot invert the npm_packages-before-npm_versions write-lock order documented on RotatePackumentRebuildTokenTx, and putting it first is what leaves a mis-paired call's packument rebuild token untouched. A test asserts the neighbour package's token is unchanged, so an implementation that rotated before probing fails it.
• The rotation still precedes the package mark. RotatePackumentRebuildTokenTx matches active package rows only, so a rotation below SoftDeleteNpmPackage would rotate zero rows on the path that empties the package and leave the previous fence live. The ordering is observable only on that path, which is where the test pins it.

The composer writes no counter column and removes no blob_storage_attachments row. tagsRemoved and packageDeleted are returned for the caller that settles versions_count and tags_count, and a version delete marks npm_files rather than removing them, so their attachments stay by design.

865 of the 1161 added lines are tests, so the diff is over the 500-LOC guardrail on raw count. The production surface is 296 lines: one composer with two methods. The plan sizes this phase at one step per MR, and four of this composer's properties are invisible in the end state (the rotation's position, the probe's, that every write rides the caller's transaction, and that no counter moves), so each needs its own fixture and its own mutation to justify it.

Test plan

go test -count=1 ./internal/datastore/                                                  # guards, no database
go test -tags=integration -run TestNpmVersionManagementDeleter -count=1 ./internal/datastore/

The integration suite needs a database: testcontainers works out of the box, or set ARTIFACT_REGISTRY_DATABASE_TEST_DSN. Because .golangci.yaml sets no build tags, the integration file was linted separately with --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0.

Context for LLM agents

Rationale

• A chain probe with no marked-row filter, returning a not-found sentinel, so the composer could tell "already marked" from "wrong package". Rejected: an error on an already-marked target is not idempotent, which the plan's Step 12 requires it to be, and it would add a fourth existence-probe variant beside ActiveNpmVersionExistsTx, FindNpmVersionByID, and the package-level probes. Both collapsed cases mean "nothing here to delete", and no caller distinguishes them: the handler resolves through the parent chain first, and a bulk worker logs an inapplicable entry as a skip either way.
• RunInTx. Rejected: it calls BeginTx with nil TxOptions and therefore sets no isolation level, while this step requires an explicit READ COMMITTED. The hand-rolled envelope also matches NpmVersionUnpublishDeleter.unpublishVersionTx line for line, which is the shape the equivalence argument rests on.
• Probing after the rotation, to keep the rotation literally the transaction's first statement. Rejected: on the no-op path the transaction commits, so a mis-paired call would persist a rotation for a package it never touched. The invariant that matters is the write-lock order, and a non-locking SELECT does not participate in it.
• An explicit tx.Rollback() on the no-op path. Rejected: the deferred rollback returns early while err is nil, so an early return there strands the transaction. Nesting the write sequence behind the probe keeps exactly one Commit serving both paths.
• Asserting the packument expiry by reading npm_metadata_files.expires_at raw. Rejected in favour of NpmMetadataFileByPackageAndKind, which carries expires_at > NOW() and is the read the packument handler itself falls through on, so the assertion proves a cache miss rather than a column value.
• Deriving packageDeleted from SoftDeleteNpmPackage's row count instead of from the active-version count, so it would mean "this call marked the package". Rejected here because it needs that store method to start returning RowsAffected, which is outside this step's file fence and is called by sibling step 11's composer, so widening it would break that branch on rebase. NpmVersionUnpublishDeleter.unpublishVersionTx derives the same flag the same way, so the imprecision is inherited equivalence rather than new. The doc comment states the weaker guarantee and warns that a caller driving a once-per-artifact side effect off the flag needs its own idempotency key, which matters for the handler's one-event-per-artifact obligation.

Non-goals

• Wiring the composer into cmd/artifact-registry/ or any route. Step 24 owns the npm version DELETE handler, and a composer with no caller is inert.
• The versions_count and tags_count decrements. npm's buffered dispatch helper is unexported and format-local, so the management surface gets its own; the plan places it in the scaffold step, and this composer's returns are its inputs.
• Attachment removal. This delete marks npm_files and removes only npm_tags, which owns no attachment, so there is nothing for the widened reverse-reference check to do here.
• A packument HTTP read. internal/datastore cannot drive one without importing the format package, which the layering forbids.
• Adding this composer to the "all six handlers" set in internal/datastore/npm_write_token_rotation_integration_test.go. Sibling steps 11 and 13 land composers with the same rotation obligation in the same package, and all three editing that one shared file would collide; each pins its own rotation in its own suite instead.

Spec coverage

Spec: docs/specs/S17-rest-management-api.md. Plan: Step 12. Acceptance criteria are numbered by position; the Phase 4 block opens at 30. Rows owned by the handler, bulk, and container steps are listed once at the end rather than enumerated.

#	Criterion	Tests
AC-32	Each delete removes the full subtree and the parent row goes last: an npm version delete removes the dist-tags pointing at it	..._Subtree — version and both files marked, its dist-tag row gone, the sibling version's rows untouched
AC-36	An npm version delete expires the package's npm_metadata_files cache in the transaction that marks the rows, so a packument read after the delete does not list the removed version	..._ExpiresThePackumentCache, both subtests. All three cache kinds asserted as a hit before and a miss after through NpmMetadataFileByPackageAndKind, the freshness-filtered read the packument handler falls through on. The npm file half is Step 13's, the bulk-job half Step 36's
AC-37	Deleting an npm package's last active version removes the package	..._OrphanRule, both subtests: the last active version marks the package, one of several does not. The delete_all half is Step 36's, the Maven negative Step 14's
AC-38	Whatever removes a row removes its attachment row; a marked but unreaped artifact still holds its attachment	Not this step's write. This delete marks npm_versions and npm_files and removes only npm_tags, which owns no attachment, so it reclaims nothing. ..._Subtree and ..._ExpiresThePackumentCache assert the npm_metadata_files row count is unchanged, which is a row-survival claim only; attachment survival under a marked row is asserted by Step 10, in the parent MR
AC-49	A management delete leaves versions_count and tags_count equal to what the equivalent protocol operation would leave	The composer supplies the caller's inputs and writes no counter itself. ..._Subtree asserts tagsRemoved == 1 with a sibling dist-tag present, so a per-package count fails, and both counter columns unchanged on the surviving-sibling path, its only subtest. ..._OrphanRule asserts both counters unchanged on the package-emptied path at :385-391, and pins packageDeleted both ways. ..._Idempotency pins (0, false) on a repeat. The dispatch that applies them is a later managementapi step
B-1	"What a delete removes", npm version row: npm_tags pointing at that version, npm_files for it, then the version	..._Subtree — both files, not only the first, and the dist-tag removed outright because npm_tags carries no marker column
B-2	npm_metadata_files references the package only, so it survives a version delete	..._Subtree (row count unchanged), ..._ExpiresThePackumentCache (rows present and expired, not removed)
B-3	"Removal is deferred behind a marker": an npm version delete writes what the unpublish path writes, and row removal waits for the purger	..._Subtree (version and files marked rather than removed), ..._OrphanRule (the package marked, not removed)
B-4	The rotation runs first in the transaction, and its predicate is a silent no-op once the package row is marked	..._RotatesBeforeMarkingThePackage — primes a live fence and asserts the committed token is neither the fence nor NULL, on the orphan path, the only path where the ordering is observable
B-5	The probe precedes the rotation, so a call that resolves nothing invalidates no cache	..._UnresolvedTargetWritesNothing — the addressed package's packument_rebuild_token still equals the primed fence
P-1	One READ COMMITTED transaction carrying the unpublish deleter's envelope	..._DeleteResolvedVersionTx_EveryWriteRidesTheCallersTransaction — drives the write sequence on a test-owned transaction and rolls it back, so any statement swapped to the pool handle would auto-commit and fail the post-rollback assertions. ..._CanceledContext covers the error arm. The isolation level itself is not observable from a sequential test and was compared by eye against NpmVersionUnpublishDeleter.unpublishVersionTx
P-5	Idempotent on an already-marked target	..._Idempotency, both the surviving-sibling and the package-already-marked arms
E-1	A malformed call: nil context, or a zero namespace, package, or version id	..._Guards, four cases on four sentinels, driven through a zero-value struct literal so a guard that ran after the transaction opened would panic instead of returning
E-2	A target outside the addressed parent chain	Not an error by design. ..._UnresolvedTargetWritesNothing requires no error, zero writes, and a control delete on the pairing that does resolve, so a composer that was merely inert fails
S-1	Tenant isolation: every statement is scoped by namespace_id, and a version id outside the addressed package resolves to nothing even when the UUID is guessed correctly	..._UnresolvedTargetWritesNothing — a version id belonging to another package of the same namespace writes nothing and leaves the neighbour intact, and a consistent pair from another namespace is rejected by the namespace leg. The probe checks the namespace and package legs only; the repository leg is the handler's resolve, so a consistent pair from another repository of the same namespace does resolve here
S-2	Existence hiding on writes: an unresolved target is a no-op that states nothing about whether it exists	..._UnresolvedTargetWritesNothing — the same return value for a version that exists elsewhere and one that exists nowhere
AC-33	An interrupted reap leaves the target marked and the next pass completes the subtree	S20-A's purger, per the plan's Dependencies. No test here
AC-31, AC-50, and the route-level 400, 404, 409, 413, 422, 503, and 405 rows	Status codes, the missing-target 404, and one deletion event per named artifact	Step 24's handler. This composer has no request surface, and its unresolved-target no-op is the benign arm the handler's chain resolve maps
AC-51 and the delete_all halves of AC-36 and AC-37	Bulk delete applying its entries in a job	Worker steps (Steps 35-37)
AC-1 to AC-30, AC-34, AC-35, AC-39 to AC-48, AC-52 to AC-80	The Phase 1 and Phase 3 surface, the container and Maven families, the contract documents, and every bulk and tag-upsert criterion. The range closes at AC-80 rather than AC-67 because rebasing onto the advanced base brought the Phase 8 statistics criteria into the list, and AC-68 through AC-80 are unattributed here	Not this step. No route, contract, container, or Maven surface changes here

All test names are prefixed TestNpmVersionManagementDeleter_ followed by the method segment the test drives, either DeleteVersion_ or DeleteResolvedVersionTx_, in internal/datastore/npm_version_management_deleter_integration_test.go. The exceptions are ..._Guards and the constructor's nil-client panic, which sit in the untagged internal/datastore/npm_version_management_deleter_test.go.

Related to #313 (closed)