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 msTimings: 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 msTimings: 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 fromSoftDeleteNpmPackage's inlineUPDATE. 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 thesoft_deleted_at IS NULLleg, which is the one clause whose absence is invisible to every other assertion. - Signature returns
erroralone, nomarked 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_idparameter. The plan directs mirroringSoftDeleteNpmPackage, 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:duplon the two methods. Measured rather than assumed:duplreports only two findings in the tagged run, both on pre-existing credential test files. The extracted builders keep each method under the token threshold. Theduplentries on the two twin integration tests are the opposite case, also measured: stripping them makesduplfire on the namespace-isolation subtests. - Rejected narrowing
maven_invariants_test.go's matcher to key on counter columns rather than allUPDATEsites, 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.xmlwhen 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-levelmaven-metadata.xmlkeeps listing the marked version, whose download answers not-found, until the next deploy to the package rewrites that row. The reconciler would drop it (ListLiveVersionsfilters the marker), but nothing schedules one:EnqueueReconcilehas no production caller today, and its doc names the S20 lifecycle cascade as the one that will call it. - Removing the version-less
maven_filesrows. The package-levelmaven-metadata.xmlrow and its checksum siblings hang off the package with a NULLmaven_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_countor 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)withnamespace_idpinning 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.gogates onCountLiveVersions, 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) thenmaven_packages(FOR UPDATE) viaReVerifyPackageAlive, 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 theErrNotFoundtoErrNoMetadatapath it already documents. FindOrCreatePackagealready documents the race this writer creates: a mark committing between its conflict read and its fallback SELECT surfaces asErrNotFound.
Related to #313 (closed)