chore(datastore): add the maven package and version tombstone markers

Why

Step 14 of the merged S17 Phase 4 plan: the first Maven deletion writers in the tree. A Maven package or version delete does bounded work in the request and leaves subtree removal to the lifecycle purger, so what the request writes is a single marker on the named row. These two methods are that write. The Maven DELETE handlers land in later steps and call them, so nothing invokes either method yet.

Spec: Removal is deferred behind a marker and What a delete removes.

What is worth a second look

No merge-order requirement, and that is checkable rather than assumed. The container marker needs its read predicates and its partial-index swap to land first, because a mark written ahead of them would still be readable. Maven needs neither. Every Maven read already filters the parent chain: the package list and detail filter their own row, FindMavenVersionByID runs a parent-package probe, FindMavenFileByID filters at every level of the chain, and the coordinate reads behind the protocol download path (FindPackageFile, FindPackageMetadataFile, FindPackageMetadataFileByPackage) all filter the parent package. Both unique indexes were born partial on soft_deleted_at IS NULL, so a mark leaves the index and a re-publish of the same coordinate inserts a fresh live row beside the tombstone. That is why this step's Depends on: is empty.

The TODO(s18-buffered-counters) comment on each statement builder is load-bearing, not a stray process reference. internal/datastore/maven_invariants_test.go is a fail-closed AST walk requiring that exact literal within five source lines above every jet UPDATE in maven_*.go. Both markers are tombstone writes rather than counters, so each carries the disclaimer updateMavenRemoteCredentialsStmt established for the same situation. The duplication across the two builders is what the invariant mandates, one marker per UPDATE site.

The error wraps carry no identifiers. internal/datastore/datastore.go requires that of a query method added after that rule landed, and warns that the older siblings next door are the wrong model to copy. Each doc comment now carries the caller's half: log namespace_id and the row id as structured fields at the call site.

Both markers scope on namespace_id and id alone. Tenant isolation holds, but an unverified id reaches any package or version in the namespace, including one in another Maven repository. The signature mirrors SoftDeleteNpmPackage as the plan directs, so the obligation lives in a CALLER CONTRACT: paragraph on each method instead, matching how the reads in these files state the same requirement.

Tests are in internal/datastore, not internal/managementapi. The plan's Acceptance asks for handler-level verification of the subtree invisibility, and the Maven DELETE handlers do not exist yet. The invisibility is asserted through the same store reads those handlers call. The two child-list statements (ListMavenVersionsByPackage, ListMavenFilesByVersion) filter only their own rows and join no parent, which both doc comments state as a caller contract, so a characterization subtest pins that boundary rather than leaving it to read as a hole.

Size. 1014 reviewable LOC, over the 500 ceiling, and the split is 168 production against 846 test. The plan's Research Findings measured this fixture tax across Phase 3 and budget for it. Splitting two 38-line methods across two MRs would cost more review context than it saves.

Spec coverage

Plan Step 14 obligations

Obligation Tests
A marked package leaves the package list and its detail reads as a miss TestMavenPackageStore_SoftDeleteMavenPackage_HidesTheSubtree/the_package_leaves_its_repository's_list_and_its_detail_reads_as_a_miss
The version and file reads beneath a marked package answer not-found at the parent legs ..._HidesTheSubtree/the_version_under_it_reads_as_a_miss_although_its_own_row_is_unmarked, ..._HidesTheSubtree/the_file_under_it_reads_as_a_miss
A marked version hides its files the same way TestMavenVersionStore_SoftDeleteMavenVersion_HidesTheSubtree/the_file_under_it_reads_as_a_miss, ..._HidesTheSubtree/the_version_leaves_its_package's_list_and_its_detail_reads_as_a_miss
The child-list statements carry no parent join (caller contract, pinned so it cannot be read as a hole) TestMavenPackageStore_SoftDeleteMavenPackage_HidesTheSubtree/the_child_lists_still_return_their_own_rows, TestMavenVersionStore_..._HidesTheSubtree/the_version's_file_list_still_returns_its_own_rows
Marker idempotency: a second call does not error and marks nothing new TestMavenPackageStore_SoftDeleteMavenPackage/a_second_call_marks_nothing_new, TestMavenVersionStore_SoftDeleteMavenVersion/a_second_call_marks_nothing_new, TestSoftDeleteMavenPackageStmt_SQL, TestSoftDeleteMavenVersionStmt_SQL
Subtree invisibility covers the version-less maven_files rows TestMavenPackageStore_SoftDeleteMavenPackage_LeavesVersionLessFilesInPlace (all three subtests)
No last-version-removes-package behavior TestMavenVersionStore_SoftDeleteMavenVersion_LeavesAnEmptiedPackageActive
No packument or orphan rule for Maven, and no counter write TestMavenTombstones_WriteNothingBeyondTheMarkedRow (both arms)
The mark composes in the caller's transaction TestMavenTombstones_ComposeInTheCallersTransaction (rollback and commit, both markers)
Tenant isolation ..._SoftDeleteMavenPackage/leaves_a_same-coordinate_package_in_another_namespace_alone, ..._SoftDeleteMavenVersion/leaves_a_same-version_row_in_another_namespace_alone
Argument guards reject before any round-trip TestMavenPackageStore_SoftDeleteMavenPackage_ArgumentGuards, TestMavenVersionStore_SoftDeleteMavenVersion_ArgumentGuards

Owned elsewhere

Row removal, the interrupted-reap criterion, and attachment release for tombstoned subtrees are the S20-A purger's. The 202, the 404 on a missing or foreign id, and the delete event are the Maven DELETE handler steps'. Until the purger lands, marked subtrees persist hidden, the state npm unpublish is already in and both specs accept.

Test plan

CI covers it. Locally: go test -tags=integration -count=1 -run Maven ./internal/datastore/ (48s against testcontainers Postgres), plus golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 ./internal/datastore/..., which is the only run that sees the integration-tagged file.

Three mutations were run against the implementation to confirm the suite fails a naive one. Dropping soft_deleted_at IS NULL fails the idempotency timestamp assertion, dropping namespace_id fails the cross-namespace pairing, and cascading a parent-package mark from the version marker fails the emptied-package negative.

Database Review Evidence

No migrations in this MR, so migration mode did not run. Query mode covers the two new marker statements.

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.

Both targets were seeded to 5000 rows in one partition rather than the 50 the write-target rule specifies. The rule's 50 rows sit below the planner's index-preference threshold, so it produced a Seq Scan for both markers and demonstrated nothing about the predicate at production cardinality. That is the same false negative the read-target rule cites for choosing 5000. Both cardinalities are reported below.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.SoftDeleteMavenPackage Update → Index Scan maven_packages_p01_pkey 1 / 1 8.30 0.136ms 9 / 0 1 of 64
datastore.SoftDeleteMavenVersion Update → Index Scan unique_maven_versions_id_pkg_id_ns_id 1 / 1 8.30 0.152ms 15 / 0 1 of 64

Row counts are the scan node's. Both roots are an Update with no RETURNING, so the root reports rows=0 by construction. Times are the warm execution, measured on a second EXECUTE against a different target row in the same session. No anomalies in either plan.

datastore.SoftDeleteMavenPackage

Summary: The plan matches the method's intent. namespace_id prunes to one partition of 64, the (id, namespace_id) primary key drives the row, and soft_deleted_at IS NULL applies as a Filter on the single matched row, which is what makes a repeat call touch nothing. Estimate matches reality (1 / 1) and the warm execution is 0.136ms over 3 buffer hits on the scan, all cache hits with no reads. No anomalies.

Seed shape: namespaces=1, repositories=1, maven_repositories=1, maven_packages=5000

Rendered SQL:

UPDATE public.maven_packages
SET soft_deleted_at = NOW()
WHERE ((maven_packages.namespace_id = $1::uuid) AND (maven_packages.id = $2::uuid)) AND (maven_packages.soft_deleted_at IS NULL);

Bound args: [c45bfe17-1f98-4626-b29c-1c7bfbc2f097, 65c49c6b-3f30-47de-b595-3f481a09ce8f] (the seeded namespace, and one seeded package id)

Plan (warm, EXPLAIN (ANALYZE, BUFFERS)):

 Update on maven_packages  (cost=0.28..8.30 rows=0 width=0) (actual time=0.049..0.049 rows=0 loops=1)
   Update on maven_packages_p01 maven_packages_1
   Buffers: shared hit=9
   ->  Index Scan using maven_packages_p01_pkey on maven_packages_p01 maven_packages_1  (cost=0.28..8.30 rows=1 width=18) (actual time=0.014..0.015 rows=1 loops=1)
         Index Cond: ((id = '65c49c6b-3f30-47de-b595-3f481a09ce8f'::uuid) AND (namespace_id = 'c45bfe17-1f98-4626-b29c-1c7bfbc2f097'::uuid))
         Filter: (soft_deleted_at IS NULL)
         Buffers: shared hit=3
 Planning Time: 0.086 ms
 Trigger for constraint fk_maven_packages_maven_repository_id_maven_repositories on maven_packages_p01: time=0.061 calls=1
 Trigger for constraint fk_maven_packages_namespace_id_namespaces on maven_packages_p01: time=0.006 calls=1
 Execution Time: 0.136 ms

Timings: planning 0.086ms, execution 0.136ms, total 0.222ms.

Referential-integrity triggers: both foreign-key triggers fire once per call and short-circuit, because the statement changes no key column. Their cost is a cold-session artifact: 2.074ms and 0.018ms on the first execution in a fresh session against 0.061ms and 0.006ms warm. The cold figure is plan-cache and trigger-setup work, not per-call overhead.

At the write-target rule's 50 rows: Seq Scan on maven_packages_p62, cost 1.75, 49 rows removed by filter, execution 1.560ms. Still one partition. The scan choice is the planner declining an index over a 50-row heap, not a missing index.

datastore.SoftDeleteMavenVersion

Summary: The plan matches the method's intent, with the same shape as the package marker: one partition of 64, a unique-index row seek, and soft_deleted_at IS NULL as a Filter on the matched row. The planner chose unique_maven_versions_id_pkg_id_ns_id over pk_maven_versions rather than the primary key. Both are unique and lead on id, so they are equally selective here at identical cost and buffer count, and the choice carries no consequence. Estimate matches reality (1 / 1), warm execution 0.152ms, all cache hits. No anomalies.

Seed shape: namespaces=1, repositories=1, maven_repositories=1, maven_packages=1, maven_versions=5000

Rendered SQL:

UPDATE public.maven_versions
SET soft_deleted_at = NOW()
WHERE ((maven_versions.namespace_id = $1::uuid) AND (maven_versions.id = $2::uuid)) AND (maven_versions.soft_deleted_at IS NULL);

Bound args: [97c18c5f-a2a8-4327-a0d9-37b9d873e34b, 61d168c0-e797-41f5-a758-d1f9fc3a1f45] (the seeded namespace, and one seeded version id)

Plan (warm, EXPLAIN (ANALYZE, BUFFERS)):

 Update on maven_versions  (cost=0.28..8.30 rows=0 width=0) (actual time=0.059..0.059 rows=0 loops=1)
   Update on maven_versions_p62 maven_versions_1
   Buffers: shared hit=15
   ->  Index Scan using maven_versions_p62_id_maven_package_id_namespace_id_idx on maven_versions_p62 maven_versions_1  (cost=0.28..8.30 rows=1 width=18) (actual time=0.015..0.016 rows=1 loops=1)
         Index Cond: ((id = '61d168c0-e797-41f5-a758-d1f9fc3a1f45'::uuid) AND (namespace_id = '97c18c5f-a2a8-4327-a0d9-37b9d873e34b'::uuid))
         Filter: (soft_deleted_at IS NULL)
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=1
 Planning Time: 0.096 ms
 Trigger for constraint fk_maven_versions_maven_package_id_maven_packages on maven_versions_p62: time=0.068 calls=1
 Trigger for constraint fk_maven_versions_namespace_id_namespaces on maven_versions_p62: time=0.007 calls=1
 Execution Time: 0.152 ms

Timings: planning 0.096ms, execution 0.152ms, total 0.248ms.

Tombstone discovery: maven_versions already carries index_maven_versions_on_ns_id_soft_deleted_at (partial, WHERE soft_deleted_at IS NOT NULL), so the rows this marker writes are already indexed for the purger's scan. maven_packages carries no such index yet. Adding it is Step 5 of the same plan, a separate MR, and it does not gate this write path.

Context for LLM agents

Design rationale and rejected alternatives

  • Statement builders extracted as package-private softDeleteMavenPackageStmt / softDeleteMavenVersionStmt, diverging from SoftDeleteNpmPackage's inline UPDATE. These two files already group standalone builders at the end (listMavenPackagesStmt, findMavenVersionByIDStmt, mavenPackageInRepositoryProbeStmt) so unit tests can assert on the SQL the store issues. That buys a database-free check on the soft_deleted_at IS NULL leg, which is the one clause whose absence is invisible to every other assertion.
  • Signature returns error alone, no marked bool. The container marker returns a flag because its handler needs the hit/miss split. The Maven handlers resolve the target first, so the count carries nothing. Rejected adding it speculatively.
  • No maven_repository_id parameter. The plan directs mirroring SoftDeleteNpmPackage, whose shape is identical, and the committed tests bind it. The repository-membership obligation is stated in each doc comment instead. Revisit if a caller ever marks without resolving.
  • No //nolint:dupl on the two methods. Measured rather than assumed: dupl reports only two findings in the tagged run, both on pre-existing credential test files. The extracted builders keep each method under the token threshold. The dupl entries on the two twin integration tests are the opposite case, also measured: stripping them makes dupl fire on the namespace-isolation subtests.
  • Rejected narrowing maven_invariants_test.go's matcher to key on counter columns rather than all UPDATE sites, which would have avoided the marker comments. It weakens a deliberately fail-closed guard and edits a test file outside this step's file list. Worth a follow-up issue: two of the seven marked sites in the package are now non-counter writes, which erodes the marker's value as a grep target for the eventual buffered-counter swap.

Non-goals

  • Expiring maven-metadata.xml when a version is marked. npm force-expires its packument on a version delete. The spec states Maven carries no equivalent rule (a Maven delete marks the named row only), and a negative test pins it. The stored package-level maven-metadata.xml keeps listing the marked version, whose download answers not-found, until the next deploy to the package rewrites that row. The reconciler would drop it (ListLiveVersions filters the marker), but nothing schedules one: EnqueueReconcile has no production caller today, and its doc names the S20 lifecycle cascade as the one that will call it.
  • Removing the version-less maven_files rows. The package-level maven-metadata.xml row and its checksum siblings hang off the package with a NULL maven_version_id, so no per-level mark covers them and they are unaddressable by id. Reaching them is the purger's foreign-key walk. The tests assert they survive the mark unmarked and stay unaddressable through every read.
  • A versions_count or repository counter decrement. Management deletes write no repository counters directly. The accounting emit is a later step of this plan, gated on S22's emit API.
  • An EXPLAIN test. The plan scopes EXPLAIN work to steps where an index changed. Both statements are single-row seeks on the primary key (id, namespace_id) with namespace_id pinning the hash partition.
  • Filling the plan's Status table. Nine sibling step branches of this plan are in flight, so one conductor commit fills the wave's rows rather than nine branches editing the same table.

Behavior worth knowing downstream

  • A mark immediately frees a slot against the per-package version cap: internal/format/maven/upload.go gates on CountLiveVersions, which filters the marker. Publish headroom returns before the purger runs.
  • The marker takes a row-exclusive lock and holds no other. The Maven reconciler takes repositories (FOR SHARE) then maven_packages (FOR UPDATE) via ReVerifyPackageAlive, so there is no lock-order cycle. A mark racing a reconcile serializes: the reconcile either completes against a row the mark then hides, or blocks and afterwards finds the package soft-deleted, which is the ErrNotFound to ErrNoMetadata path it already documents.
  • FindOrCreatePackage already documents the race this writer creates: a mark committing between its conflict read and its fallback SELECT surfaces as ErrNotFound.

Related to #313 (closed)

Edited by Hayley Swimelar

Merge request reports

Loading
Loading