fix(npm): align versions_count and tags_count with ADR-007 on unpublish

🎯 What this MR does

Fixes the unpublish-path counter defects from #549 (closed) item 4, settling versions_count on ADR-007's semantics: the counter includes soft-deleted versions and moves only at hard-delete. Step 3 (final) of the three-MR stack.

📚 The stack:

Step	MR	Status	What it delivers
1	!1401 (merged)	merged	npm_versions.size_bytes column + index + backfill
2	!1402 (merged)	merged	Maintenance on publish & cache-fill, reconciliation recompute, size in the API
3 · this MR	👉	open	versions_count / tags_count semantics fixes on unpublish

✅ No longer stacked: steps 1 and 2 have merged, so this MR targets main and its diff is the step-3 commit alone.

⚖️ The decision this MR implements

ADR-007 says versions_count "counts the package's npm_versions rows including soft-deleted ones, decrementing only when garbage collection hard-deletes a row", with an explicit anti-cap-gaming rationale: a publisher who could exclude tombstones from the 25,000-version cap could cycle unpublish + republish to stay under it forever while every tombstone keeps its storage. The implemented S11 unpublish decremented at soft-delete instead — a divergence S17's follow-ups flagged as "needs reconciling". This MR reconciles it in ADR-007's favor (settled with the DRI on #549 (closed)).

That flips the issue's framing of item 4, and the research is worth recording: the whole-package path's missing versions_count decrement was never a defect — it is the ADR-conformant behavior. The real defects were the mirror image:

• 🐛 Single-version unpublish decremented versions_count at soft-delete → the decrement is removed. emitCounterDecrements now dispatches only the tags decrement. The handler seam is renamed NpmPackageTagsCountMaintainer: it lost versions_count and gained the reset below, so it is no longer a decrementer at all. DecrementNpmPackageVersionsCount stays in the datastore, redocumented: the lifecycle purger (S20-A) becomes its caller when its reap hard-deletes a still-live package's versions.
• 🐛 Whole-package unpublish hard-deleted every npm_tags row without moving tags_count → the handler now emits a post-commit ResetNpmPackageTagsCount. A reset to zero, not a counted decrement: zero is the exact recompute of the terminal state (all tag rows are gone, and restore does not bring tags back), so the reset also self-heals any drift the buffered counter had accumulated — a decrement would carry it into the tombstone forever. ResetNpmPackageTagsCount's doc comment also records the window the reset cannot close on its own. Round 3 corrected its scope: the PUT and the cascade already serialize on the npm_packages row lock (both take it as their first statement, through RotatePackumentRebuildTokenTx), so only the detached counter writes are unordered — see ## 🔁 Review round 3 below.
• 🐛 Single-version unpublish of a package's last active version left the same stale tags_count → that path reaches the identical terminal state (package row tombstoned, no npm_tags rows left), but packageDeleted skipped every post-commit counter write, so the reset above made the two paths disagree on identical state. ServeHTTP now picks an arm instead of skipping: afterCascadeCommit emits the same reset. Unconditional there, not gated on tagsRemoved — the stale count can come from versions unpublished earlier. The packument rebuild stays skipped, which is the one side effect the tombstone really rules out (NpmPackageByID filters soft_deleted_at IS NULL; the reset carries no such filter, which is what lets it reach the tombstone).
• 📝 The deleter's stale comment ("the publish-side increment ships later, so it reads ~0 today" — it shipped) is rewritten to state the ADR-007 rule.
• 📝 NpmPackageUnpublisher's doc said the cascade "cascades soft-deletes to npm_versions, npm_files, and npm_tags". npm_tags has no soft_deleted_at column and cascadeDeleteNpmTags hard-deletes those rows — which is the whole reason tags_count gets a reset and not a decrement — so the comment now says which tables are soft-deleted and which are hard-deleted.

📐 Cap behavior consequence

Until the purger lands, versions_count only grows. That is the fail-safe direction for the 25k cap: over-counting can block a pathological publisher early, never admit gaming. The purger-side decrement is recorded as a follow-up on #549 (closed) — the dependency is S20-A's reaper, not S28 as the issue originally said (S20-A: "the purger is the per-artifact hard-deleter"; S28 keeps only blob reclamation).

📄 Spec corrections riding along

• S22-storage-accounting.md: the "Accounting layers" sentence claimed version counters are maintained "synchronously, in the same transaction as the underlying file rows" — corrected to ADR-007's post-commit contract for the package-level counters only, with the correction named in place (#549 (closed) declares ADR-007 the authority). The corrected sentence is also scoped to what ships: it originally read as a claim about Maven too, and maven_versions carries no size_bytes column, so Maven's half is named as intended design rather than current behavior.
• S17-rest-management-api.md: the follow-up carrying the versions_count divergence now records it as reconciled.

✅ Verification

• Unit + integration assertions flipped, not dropped: the success-path tests now positively pin that no versions_count event is dispatched at soft-delete (the recording fake deliberately keeps the method beyond the interface to prove absence); the whole-package integration test pins versions_count kept + tags_count reset to 0 on the tombstone, and the last-active-version integration test now does the same (seeded versions_count = 3, tags_count = 4, asserted 3 / 0; its fixture seeds no tags, so it also covers the tagsRemoved == 0 case). ResetNpmPackageTagsCount's argument guards are a row in the package's existing TagsCountAdjuster table rather than a suite of their own, plus a new datastore integration suite.
• golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 over ./internal/datastore/... ./internal/format/npm/... ./cmd/..., cross-referenced line by line against the lines this MR adds. Two findings the MR itself caused are fixed: thelper on the new test's subtest closure (silenced with a function-level directive naming docs/dev/go-testing.md, which reserves t for the outer test), and dogsled from the widened UnpublishPackage return (fixed by asserting the returned package id rather than adding a third blank). A dupl on the reset's own guard table is gone with that table, folded into the existing one. Six contextcheck findings remain on added lines, all raised through the shared seedNamespace/seedNpmPackageFixture helpers: the identical message already appears 420 times in internal/datastore/npm_write_integration_test.go, so the new subtests inherit that file's baseline rather than adding a new kind of violation.
• Full go test -tags=integration ./internal/datastore/ and ./internal/format/npm/ suites pass locally against PostgreSQL 17.
• 🧪 npm conformance (real npm CLI e2e): all steps pass on the full stack, including the unpublish flows this MR changes.

🧪 E2E scenario impact

docs/testing/e2e/npm.md landed on main (571b4e55) while this MR was in review, so the earlier "no npm catalog exists" note is stale. acebff4d adds e2e.npm.lifecycle.unpublish-package-counters: the existing unpublish rows assert installability and packument presence, not the counters the management API returns, which is exactly what this MR settles. The row states why the two counters diverge, that both writes land after the request commits (so the read needs a retry), and that a shed write can lose the value (#632 (closed)). Its "No event" row is added to the usage-data table too, which lists every scenario.

🔁 Review round 2 (66a138ff)

@jdrpereira's round-1 findings, all addressed:

Finding	Fix
blocking last-active-version keeps a stale tags_count	The third 🐛 above: afterCascadeCommit + unit and integration coverage
blocking versions_count's client contract asserts the opposite	api/openapi/v1.yaml and internal/managementapi/package_resources.go rewritten — it includes tombstones and only grows, so it sits above the version list rather than lagging it. S17:457 carried the identical sentence and is fixed too
blocking the S22 correction overshoots for size_bytes	Scoped to the package-level counters; the paragraph now says npm_versions.size_bytes is written on the version INSERT alongside its file row, so it needs no drift or recompute tolerance
blocking four comments still promise the versions_count decrement	All four rewritten; the deleter's contract now states both arms
escalate the anti-gaming rationale is bypassable today	Verified and recorded on #549 (closed) (both the bypass and its mirror cost). The seam's doc now scopes the protection to the single-version path
remember two-vs-three counter events	Header, ServeHTTP doc, and the const block (the third label is an npm_packages column from disttags.go)
remember two doc comments call versions_count a counter nobody writes	Both now say why CountActiveNpmVersionsByPackage is used instead
suggestion the race note misses a reachable ordering	Rewritten as the full cross-product (see the second 🐛 above)
suggestion duplicated test scaffolding	readNpmPackageTagsCount and the existing TagsCountAdjuster table reused; helper, 35-line table, and //nolint:dupl dropped

Process note 🔍: this stack ships without a prior plan MR by explicit operator decision — the plan-level context lives in these MR descriptions (see !1401 (merged) for the full background).

/cc @vespian_gl — draft !1351 (merged)'s Step 17 touches the same two unpublish handlers for the repository-level emits; sequencing note so we don't churn the same lines under review.

🔁 Review round 3 (cd505a60)

@jdrpereira's escalate on the race thread ("where/when is the lock going to happen?") turned out to have a better answer than a follow-up: the lock already exists. Both the dist-tag PUT and the unpublish cascade update the npm_packages row as their first statement (RotatePackumentRebuildTokenTx), so the transactions serialize, and a PUT ordered after the cascade refuses on its in-transaction active-version re-check. cd505a60:

• adds TestNpmPackageUnpublishDeleter_SerializesConcurrentDistTagPut, pinning the serialization against a real database (lock-wait asserted via pg_stat_activity; negative control verified by removing the rotation locally);
• rewrites ResetNpmPackageTagsCount's doc, which wrongly claimed no path locks the row.

The ordering half is then closed in 0461802f: both buffered increments skip a soft-deleted package, with IncrementNpmPackagePublishCounters gating only its tags_count arm through a CASE (versions_count must keep reaching the tombstone per ADR-007), covered by TestNpmPackageStore_BufferedIncrementsSkipTombstones. What remains is loss rather than ordering — bufferedUpdate sheds a write when its in-flight cap is saturated, which no predicate reaches. That is general rather than npm-specific and is tracked on #632 (closed); the npm-side record stays on #549 (closed).

Database Review Evidence

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml; server reported 17.10), 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.

No migrations in this MR, so migration mode did not run. One new query-producing method: ResetNpmPackageTagsCount. The only other change to internal/datastore/npm_packages.go is the DecrementNpmPackageVersionsCount doc comment, whose jet chain is byte-identical to the merge-base, so it is not re-planned here.

The method is reported at two cardinalities because it is an UPDATE. The 50-row shape is the skill's default for a write target (1 target row plus 49 siblings); at that size the target partition is a single page and the planner correctly prefers a scan of it, which says nothing about production behavior. The 5000-row shape is what answers the reviewer's question.

Method	Plan node	Index	Rows (plan / actual)	Cost	Time	Buffers (hit / read)	Partitions
NpmPackageStore.ResetNpmPackageTagsCount (50 rows)	Update → Seq Scan	n/a	1 / 1	1.75	0.117ms	4 / 0	1 of 64
NpmPackageStore.ResetNpmPackageTagsCount (5000 rows)	Update → Index Scan	npm_packages_p59_pkey	1 / 1	8.30	0.118ms	15 / 0	1 of 64

Query notes:

• No anomalies. The Seq Scan in the 50-row row is not a flag: the skill flags a Seq Scan only against 5000+ seeded rows, and at that size this statement uses the primary key. Both rows prune to one partition of 64.
• The first EXECUTE in a cold container charges up to 6.8ms to the fk_npm_packages_npm_repository_id_npm_repositories constraint trigger. On repeat executions in the same transaction it settles to 0.02-0.10ms (measured over three consecutive executions). The plans below are warm runs, so the reported execution times are steady state rather than first-call.

NpmPackageStore.ResetNpmPackageTagsCount (50 rows, skill default for a write target)

Summary: Plan matches the method's intent — a single-row counter reset located by (namespace_id, id). The planner pruned to one partition of 64 and then chose a Seq Scan over that partition's single page, filtering out the 49 siblings; at 50 rows an index descent costs more than reading the page, so this is the planner being right rather than a missing index. Estimate matches reality (1 / 1). No anomalies.

Seed shape: namespaces=1, repositories=1, npm_repositories=1, npm_packages=50

Rendered SQL:

UPDATE public.npm_packages
SET tags_count = $1::integer
WHERE (npm_packages.namespace_id = $2::uuid) AND (npm_packages.id = $3::uuid);

Bound args: [0, c2cda5ac-155e-49d1-9947-0d00fab08662, 2b960f9a-5b9e-4bc0-83bd-6cba2e56e1cf]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_packages  (cost=0.00..1.75 rows=0 width=0) (actual time=0.116..0.117 rows=0 loops=1)
   Update on npm_packages_p36 npm_packages_1
   Buffers: shared hit=4
   ->  Seq Scan on npm_packages_p36 npm_packages_1  (cost=0.00..1.75 rows=1 width=14) (actual time=0.023..0.027 rows=1 loops=1)
         Filter: ((namespace_id = 'c2cda5ac-155e-49d1-9947-0d00fab08662'::uuid) AND (id = '2b960f9a-5b9e-4bc0-83bd-6cba2e56e1cf'::uuid))
         Rows Removed by Filter: 49
         Buffers: shared hit=1
 Planning Time: 0.310 ms
 Trigger for constraint fk_npm_packages_namespace_id_namespaces on npm_packages_p36: time=0.044 calls=1
 Trigger for constraint fk_npm_packages_npm_repository_id_npm_repositories on npm_packages_p36: time=0.196 calls=1
 Execution Time: 0.450 ms

Timings: planning 0.310ms, execution 0.450ms, total 0.760ms.

NpmPackageStore.ResetNpmPackageTagsCount (5000 rows in one partition)

Summary: At production-shaped cardinality the plan is an Index Scan using npm_packages_p59_pkey, the partition-local pk_npm_packages on (id, namespace_id), whose Index Cond covers both predicate columns with no residual filter. The namespace_id literal prunes to one partition of 64, the estimate matches reality (1 / 1), and execution stays at 0.24ms with 15 buffer hits and zero reads. No anomalies.

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

Rendered SQL:

UPDATE public.npm_packages
SET tags_count = $1::integer
WHERE (npm_packages.namespace_id = $2::uuid) AND (npm_packages.id = $3::uuid);

Bound args: [0, 900ef1d6-1007-4046-95e3-4c1f1d056182, 60004e17-02f5-4f2f-bd6b-7ee574489008]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_packages  (cost=0.28..8.30 rows=0 width=0) (actual time=0.118..0.118 rows=0 loops=1)
   Update on npm_packages_p59 npm_packages_1
   Buffers: shared hit=15
   ->  Index Scan using npm_packages_p59_pkey on npm_packages_p59 npm_packages_1  (cost=0.28..8.30 rows=1 width=14) (actual time=0.025..0.026 rows=1 loops=1)
         Index Cond: ((id = '60004e17-02f5-4f2f-bd6b-7ee574489008'::uuid) AND (namespace_id = '900ef1d6-1007-4046-95e3-4c1f1d056182'::uuid))
         Buffers: shared hit=3
 Planning Time: 0.126 ms
 Trigger for constraint fk_npm_packages_namespace_id_namespaces on npm_packages_p59: time=0.022 calls=1
 Trigger for constraint fk_npm_packages_npm_repository_id_npm_repositories on npm_packages_p59: time=0.068 calls=1
 Execution Time: 0.242 ms

Timings: planning 0.126ms, execution 0.242ms, total 0.368ms.

Related to #549 (closed)