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

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

Why

The management API needs an id-keyed whole-package npm delete, and the protocol-side unpublish path cannot serve one: NpmPackageUnpublishDeleter.UnpublishPackage is name-keyed and gated on a rev. What is reusable is the layer beneath it, so this composes the same store writers into an id-keyed peer that preserves the unpublish transaction shape, per Step 11 of the merged Phase 4 plan.

It closes the attachment leak rather than inheriting it. The unpublish path hard-deletes a package's npm_metadata_files rows and leaves their blob_storage_attachments rows standing, which is npm whole-package unpublish orphans blob_storag... (#507) • Unassigned. ADR-010 collects a blob once it has zero attachments, so a stranded attachment holds its blob out of reach of every later pass. This composer frees each one through the reference check the parent MR widened.

No handler calls it yet. That is why the type is chore: the delete handlers are later steps in the same plan.

What (the non-obvious parts)

The cache-row read is the composer's own statement, taken FOR UPDATE. No store method returns the (blob_storage_attachment_id, blob_sha256) pairs, and NpmMetadataFileByPackageAndKind cannot be reused for two independent reasons: it runs on the pool, so it cannot join the transaction, and its WHERE carries expires_at > NOW(), so it skips an expired cache row that still holds an attachment. NpmVersionUnpublishDeleter is the in-package precedent for a composer owning a read.

That read projects both primary-key columns. jet groups a multi-row scan by the destination model's primary key, and namespace_id is part of it, so projecting it without id collapses a package's three cache rows onto one and strands two attachments.

Rotate-first is pinned by an equality assertion, not an ordering probe. RotatePackumentRebuildTokenTx's predicate matches active rows only, so a rotation issued after the mark silently no-ops and leaves the token byte-identical. Asserting the token changed is what catches the inverted order.

The suite mirrors the protocol-side unpublish coverage, with one deliberate inversion. Where that suite asserts the repository counters decrement, this asserts no counter moves: a naive port of the protocol path would decrement them, and a management delete moves no repositories counter.

One out-of-step comment fix rides along. internal/datastore/npm_package_unpublish_deleter.go claimed versions_count's publish-side increment "ships later". It is live in IncrementNpmPackagePublishCounters. The column is still the wrong thing to read, for a different reason, and the comment now says which.

Reviewable LOC is about 1,140 against the parent branch, over the 500-LOC guardrail. Roughly a quarter is production code. The rest is the integration suite, which needs its own fixtures for the subtree, all three metadata-file kinds, the attachment survivors, a cascade crossing the 1,000-row batch boundary, and the rollback.

Spec coverage

Criteria are numbered positionally against S17's Acceptance Criteria. Every test below is in internal/datastore/npm_package_management_deleter_integration_test.go.

Obligation	Test
AC #32: the npm package subtree is marked or removed per What a delete removes, npm_tags and npm_metadata_files included	..._MarksTheSubtree, ..._CascadesPastOneBatch
AC #38: whatever removes a row removes its attachment row in the same step	..._Attachments/removes the attachment behind every metadata-file kind (all three kind values as positive hits)
AC #38: a marked but unreaped artifact still holds its attachment	..._Attachments/keeps the attachment of every file it only marked, ..._Attachments/keeps an attachment a marked file shares with a removed cache row
Removal is deferred behind a marker: an npm package delete writes what the unpublish path writes	..._MarksTheSubtree, ..._RotatesTheTokenBeforeMarking
Delete semantics: a management delete writes no repositories counters	..._MovesNoCounters
The rotate-token-first invariant documented on RotatePackumentRebuildTokenTx, including the marked-package case the version unpublish leaves behind	..._RotatesTheTokenBeforeMarking, ..._ClearsTheCacheUnderAMarkedPackage
Idempotent on a repeat delete and on a package id no row matches	..._IsIdempotent (both cases)
Tenant isolation and blast radius	..._Scoping (both cases, the second sharing one package id across two namespaces so a missing namespace predicate reaches the other tenant's rows)
Atomicity: a failure after the mark leaves every row unchanged and the token unrotated	..._RollsBackOnAFailureAfterTheMark
A lost identifier is rejected before the transaction opens	..._ArgumentGuards

Four Phase 4 criteria are deliberately not covered here. AC #33 (an interrupted reap) is the purger's behavior and S20-A's plan verifies it. AC #36 (packument expiry on a version or file delete) and AC #37 (the last-active-version orphan rule) belong to the version and leaf composers, Steps 12 and 13. The 202, the 404 on a missing target, and the deletion event are the handler's, Step 22.

Test plan

go test -tags=integration -count=1 -run TestNpmPackageManagementDeleter ./internal/datastore/
go test -tags=integration -count=1 ./internal/datastore/   # neighbours, including the mirrored store suites
go-lint-ci --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/datastore/

The suite bootstraps its own Postgres through testcontainers, or uses ARTIFACT_REGISTRY_DATABASE_TEST_DSN when set. Two of the added tests were verified by mutation rather than by passing alone: forcing the cascade to a single batch fails ..._CascadesPastOneBatch, and adding an "already marked, nothing to do" short-circuit fails ..._ClearsTheCacheUnderAMarkedPackage and nothing else.

No conformance run: this changes no npm protocol behavior, and the composer is reachable from no route yet.

Context for LLM agents

Rejected alternatives.

• Add a pair-listing method to NpmMetadataFileStore instead of owning the read. Rejected on scope: npm_metadata_files.go is outside Step 11's declared file list and is shared with sibling steps stacking on the same parent, so an edit there collides with them. NpmVersionUnpublishDeleter already establishes that a composer may own a read it alone needs.
• Return versionsDeleted or sizeBytes, mirroring UnpublishPackage. Rejected: those returns exist so the protocol handler can decrement repositories counters, and a management delete writes none. Step 38 of the plan explicitly reserves widening composer returns for the accounting emit, so widening now would guess at a signature that step owns.
• Call ForceExpireNpmMetadataTx alongside the delete. Rejected as redundant: the rows are hard-deleted, and expiring a row about to be removed asserts nothing. Expiry is the version and file paths' rule, where the cache rows survive.
• Order the FOR UPDATE read to argue deadlock freedom. Rejected as an over-claim. What serializes this transaction against a concurrent packument rebuild is the token rotation landing first on the shared npm_packages row, not the cache-row lock order. Note that the npm writers do not share a single table order: the whole-package and single-version cascades disagree on npm_files versus npm_versions, a pre-existing inversion tracked in npm unpublish paths acquire npm_packages and np... (#490) • Unassigned.

Non-goals.

• No handler, route, or wiring. The composer has no caller until the delete handler steps land.
• No S22 accounting emit and no npm_packages counter writes. versions_count and tags_count stay stale on the marked row, which is what the protocol whole-package path also leaves, and no read returns a marked package.
• No purger. Marked rows persist hidden until S20-A's reaping sweep lands, the same state npm unpublish tombstones are already in.
• Two comments in shared files go stale because this composer exists, and both are deliberately left for a follow-up rather than edited here, because sibling steps stack on those files: blob_storage_attachments.go says ContainerManifestDeleter is the only caller of DeleteIfUnreferenced, and npm_metadata_files.go enumerates the writers ordered for the rotate-while-active invariant.

Database Review Evidence

This MR adds no migrations, so migration mode did not run. Query mode found exactly one new statement: the cache-row read removeCacheRows issues before it hard-deletes a package's npm_metadata_files rows. Every other statement the composer runs belongs to an existing store method this MR does not modify, so query mode records no row for them. The transaction they form is covered under Composed statements below.

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 the 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.NpmPackageManagementDeleter.removeCacheRows	LockRows over Index Scan	npm_metadata_files_p53_namespace_id_npm_package_id_kind_idx	3 / 3	11.87	0.025ms	6 / 0	1 of 64

datastore.NpmPackageManagementDeleter.removeCacheRows

Summary: The plan matches the method's intent. The read takes an Index Scan on the unique (namespace_id, npm_package_id, kind) index, the namespace_id equality prunes to one of the table's 64 hash partitions, and the FOR UPDATE surfaces as the LockRows node above it. The estimate matches reality exactly at 3 planned and 3 actual rows against 5,001 seeded cache rows in the target partition, with every buffer a hit and no reads. Carrying no LIMIT is not an unbounded read here: that unique index and the three-value CHECK on kind cap the result at three rows per package whatever the table's size, so the result cannot grow with the table. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_repositories=1, npm_packages=1667, blob_storage_blobs=5001, blob_storage_attachments=5001, npm_metadata_files=5001

Every row shares one namespace_id, so all 5,001 cache rows land in a single hash partition. That is what takes the seed past the cardinality where the planner would prefer a sequential scan, which is the point of the exercise: a smaller per-partition seed yields a Seq Scan plan even when the right index exists.

Rendered SQL:

SELECT npm_metadata_files.namespace_id AS "npm_metadata_files.namespace_id",
     npm_metadata_files.id AS "npm_metadata_files.id",
     npm_metadata_files.blob_storage_attachment_id AS "npm_metadata_files.blob_storage_attachment_id",
     npm_metadata_files.blob_sha256 AS "npm_metadata_files.blob_sha256"
FROM public.npm_metadata_files
WHERE (npm_metadata_files.namespace_id = $1::uuid) AND (npm_metadata_files.npm_package_id = $2::uuid)
FOR UPDATE;

Bound args: [88951991-6293-4f48-a643-ffa3cd48e978, 00000000-0000-7000-8000-000000000320]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 LockRows  (cost=0.28..11.87 rows=3 width=83) (actual time=0.007..0.009 rows=3 loops=1)
   Buffers: shared hit=6
   ->  Index Scan using npm_metadata_files_p53_namespace_id_npm_package_id_kind_idx on npm_metadata_files_p53 npm_metadata_files  (cost=0.28..11.84 rows=3 width=83) (actual time=0.005..0.006 rows=3 loops=1)
         Index Cond: ((namespace_id = '88951991-6293-4f48-a643-ffa3cd48e978'::uuid) AND (npm_package_id = '00000000-0000-7000-8000-000000000320'::uuid))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=9
 Planning Time: 0.108 ms
 Execution Time: 0.025 ms

Timings: planning 0.108ms, execution 0.025ms, total 0.133ms.

Composed statements (unchanged by this MR)

The composer's other statements come from existing store methods, so they are outside query mode's changed-file scope. They are worth one measurement anyway, because this composer is what makes them run in a single transaction, and the question a reviewer will have is whether that transaction's statement count stays bounded as a package grows.

It does. Running the suite's batch-crossing case (a package with 1,003 versions, against npmCascadeBatchSize of 1,000) on an ephemeral PostgreSQL 17.10 container with log_statement=all, one DeletePackage call issued:

Statement	Times issued	Why that number
UPDATE public.npm_packages	2	The rebuild-token rotation, then the package marker.
UPDATE public.npm_files	3	Two batches that marked rows, then the zero-row batch that ends the loop.
UPDATE public.npm_versions	3	Same shape: 1,003 versions over a 1,000-row batch size.
DELETE FROM public.npm_metadata_files	1	One statement removes all three cache kinds.
DELETE FROM blob_storage_attachments	3	One per cache kind, each through the reference check.

Statement count therefore grows with the number of batches rather than with the number of rows, which is the bound the store methods document. The npm_tags leg issues the same batched shape, and its count is not quoted here because the test fixture's own teardown issues single-row deletes against that table that the server log does not distinguish from the cascade's.

The captured DELETE FROM blob_storage_attachments also confirms at runtime that the guard which runs is the widened one from chore(datastore): count every table that refere... (!1450 - merged) • Hayley Swimelar • 19.3, whose NOT EXISTS arms enumerate all ten referencing tables rather than container_manifests alone.

Flags: none.

Related to #313 (closed)