test(datastore): pin the delete-wins outcome under a package tombstone

What this delivers

A Maven hosted publish can commit a maven_versions row under a maven_packages tombstone. No document declared that outcome as intended. This merge request records the outcome, corrects the one merged sentence that denied the window, and pins the behavior with two test additions. It changes no production Go code.

The cause. MavenPackageStore.FindOrCreatePackage at internal/format/maven/upload.go:141 and MavenVersionStore.FindOrCreateVersion at :179 both run on the connection pool. Both run before the body streams and before the upload opens its own transaction at :573. FindOrCreateVersion reads no parent soft_deleted_at. A package mark that commits between those two statements leaves a live maven_versions row under a tombstoned parent.

The client is answered correctly. upsertFileRow calls MavenPackageStore.ReVerifyPackageAlive inside the commit transaction at internal/format/maven/upload.go:695 and holds that lock to commit. The transaction rolls back whole and writeCommitOutcome answers 404. No maven_files row and no blob_storage_attachments row stays behind.

What changes. Four document edits across two files, and two test-file edits.

File What changes
docs/specs/S20-a-lifecycle-closed-beta.md A package-tier row in ## Error Cases at :1003, beside the repository-tier row at :1002. It names the window, the residue, the routes that are in and out, and the condition under which the acceptance ends. The third ## Follow-ups bullet loses both of its stale halves, stays open, and keeps #535 as its tracker
docs/dev/storage-accounting.md The ### The Reaper seam short-page bullet loses the trailing "and nothing inserts under a tombstone", which the same file's Hosted Maven reapers section contradicts. The Maven version leg's closing sentence carries the recorded outcome instead of a pointer to this issue
internal/format/maven/upload_liveness_race_test.go One assertion in TestUpload_PackageMarkedMidCommit_FailsTheCommit counts the residue row at exactly one
internal/datastore/lifecycle_reap_maven_integration_test.go One new subtest of TestMavenPackageReaper_Reap stages the interleaving through the production writers: the package is created live, the mark lands, the version row commits under it, and the package reaper then drives the tombstone

Both test additions are characterization tests. No production behavior changes here, so nothing can fail first. The reaper subtest's insert under the tombstone is what a later fence inverts. The residue assertion holds either way, because its own insert commits while the package is still live.

What stays behind, and where it is observable. namespace_statistics.components_count reads one low against its own recompute. A rolled-back commit reaches no counter emit, while NamespaceReconcileStore.RecomputeComponentsCount counts the row physically. The next reconciliation pass corrects the counter upward, and MavenPackageReaper's next chunk then takes the row, so this trace is transient rather than permanent. The repository-level count does not carry the residue at all, because recomputeMavenVersionsStmt predicates on the parent package being live at internal/datastore/reconcile_repository.go:760.

Merged code already states that consequence, so this merge request cites it rather than asserting it again. The doc comment at internal/format/maven/upload_emit.go:139-148 reads: "On a mid-upload soft-delete of the version or the package only the second drifts, because recomputeMavenVersionsStmt predicates on both those levels being live and so drops the orphan from the repository count."

The decision this records, and the argument against it

The issue hands the reader a choice between two answers: fence the insert, or accept delete-wins and record it. The operator took the second answer with a tracked follow-up for the fence. That is what this merge request delivers.

The argument against it, which the operator had in front of them and overrode, is stated here plainly rather than softened. Fencing the insert is the better end state. It is the only answer that also satisfies #535's scope bullet 1. npm already fences this exact window: LockActiveNpmPackageForPublish at internal/datastore/npm_packages.go:374-395 takes the row lock before any DML in the publish transaction and holds it to commit.

What this merge request does is keep that end state reachable. A bare "record it" answer records the semantic and loses the fence. This answer records the semantic with a named condition and its event, and hands the fence to #1135.

One precision the reader needs on #535. Its bullet 1 has two clauses and the first is already merged: internal/format/maven/reconciler.go:671-674 records that the upload path does take the package FOR UPDATE lock. The second clause stays open until the fence lands: an equivalent serialization of the maven_versions INSERT against Reconcile, which is #1135's to deliver.

Coverage

Source: the acceptance items derived from issue #1044 (closed) during triage. There is no spec Acceptance Criteria section behind these rows. The spec's package-tier ## Error Cases row did not exist on main and lands in this merge request.

# Acceptance item Tests / evidence
A-1 A publish whose package is marked mid-commit answers 404 and leaves zero live maven_files rows and zero blob_storage_attachments rows Covered today, not by this merge request: the existing assertions in TestUpload_PackageMarkedMidCommit_FailsTheCommit (status, countLiveFiles, countNamespaceAttachments, and the INFO rollback record with digest and package id)
A-2 After that refused commit, exactly one live maven_versions row stands under the marked package This merge request. New assert.Equal(t, 1, countLiveVersionsDirect(t, fx.db, fx.ns, pkgID), ...) in TestUpload_PackageMarkedMidCommit_FailsTheCommit. It stages insert-before-mark and pins the end state, not the ordering
A-3 The tombstone drains without operator action: 23503 on fk_maven_versions_maven_package_id_maven_packages, the chunk rolls back whole, and the next chunk's state-blind version page takes the raced-in row This merge request. New subtest "a version committed under the tombstone refuses the package delete, and the next chunk drains" of TestMavenPackageReaper_Reap in internal/datastore/lifecycle_reap_maven_integration_test.go. It is also MavenPackageReaper's own contract, stated at docs/dev/storage-accounting.md:2439-2441 on this branch
A-4 git grep -n "nothing inserts under a tombstone" -- docs/ returns no hit Verified: no hits after the docs(dev) edit
A-5 ## Error Cases carries a package-tier row that names the maven_versions insert under a maven_packages tombstone, links work item 1135 as [#1135](.../work_items/1135), and states a condition and its event for the fence rather than a bare acceptance Verified. The row's closing sentence reads "The window is accepted for closed beta and tracked in #1135, which owns the serialization: the acceptance holds while no statement orders the maven_versions INSERT against the package mark, and it ends when that work item lands one"
A-6 The third ## Follow-ups bullet no longer contains "taking no package FOR UPDATE lock", no longer claims the comment assigns S20 the addition half, and still ends with a linked issue Verified: both strings return no hit, and the bullet's last line is Tracked in [#535](...)

Row A-3 differs from the table the implementation pass produced. That pass recorded item 3 as not asserted, against a reading of the issue body that stopped one sentence early. The issue's second answer asks for "one integration test that pins the interleaving: package resolved live, mark lands, version row commits, then the package reaper drives the tombstone", and the branch carries that test. The row is corrected rather than left standing, and the difference is named here rather than substituted in silence.

Acceptance item 3 names a constraint the subtest does not assert.

Acceptance item 3 names fk_maven_versions_maven_package_id_maven_packages, and the new subtest does not assert that name. The subtest cannot assert it at this seam. mapReapParentDeleteError in internal/datastore/lifecycle_scan.go matches the bare SQLSTATE and not a constraint name. That match is deliberate. The child tables are hash-partitioned, so PostgreSQL reports the declared constraint name on PG 18 and the per-partition name on PG 17 and earlier. The function then returns ErrReapParentPinned bare, so the *pgconn.PgError never leaves the store and no caller can read the name from the error.

The fixture pins which foreign key fires. No assertion does. maven_packages has two inbound child keys. When the package delete runs, the only child row that stands is the maven_versions row the publish transaction committed.

The subtest covers the 23503 half of the item transitively. ErrReapParentPinned has exactly one producer, and it sits behind one SQLSTATE gate, so require.ErrorIs on that sentinel can only pass on a 23503. The item's "chunk rolls back whole" clause carries no assertion in this file, and the fixture cannot produce one. The tombstoned package has no children, so the refused chunk removed nothing that a rollback can undo. The subtest asserts an empty ReapTotals and both rows still standing. The property belongs to the chunk driver, where runPurgeChunk wraps every reap in datastore.RunInTx. Merged work pins it in TestPurgeWorker_Work_AFailedChunkRollsBackWhole, at internal/lifecycle/chunk_integration_test.go:227.

The lock the subtest waits on is new to this file.

The file's four other lock-wait subtests all stage one reap chunk against another. The second chunk waits on row locks the first chunk still holds. Their startBlocked calls sit at internal/datastore/lifecycle_reap_maven_integration_test.go:623, :1073, :1158 and :1250. The new subtest calls startBlocked at :1338, and it is this file's first wait on an INSERT's referential-integrity share lock. FOR KEY SHARE appears nowhere in the file. The shape has precedent one file over, in TestContainerRemoteManifestReaper_RefusesAConcurrentlyPinnedManifest at internal/datastore/lifecycle_reap_container_remote_integration_test.go:678. That test stages a cache fill's tag INSERT against a manifest delete, through the same startBlocked harness. A reviewer has no in-file precedent to diff this subtest against, so it earns its own read.

Checks. What ran, and what did not:

  • go vet under -tags=integration,development_stubs is clean on ./internal/datastore/ and ./internal/format/maven/, measured 2026-09-03T17:49Z at d67b43713.
  • Both test files carry //go:build integration, which CI lint cannot see. golangci-lint run --build-tags=integration --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false therefore ran by hand on the same two packages, measured 2026-09-03T17:51Z at d67b43713. Those two packages carry a large pre-existing baseline under that tag, 5445 findings in total under golangci-lint 2.13.2 with an isolated GOLANGCI_LINT_CACHE, and no finding sits on a line this branch adds. Six sit in internal/datastore/lifecycle_reap_maven_integration_test.go and all six are dupl. The highest of them covers 1204-1280, and the block this branch adds starts at :1282. internal/format/maven/upload_liveness_race_test.go draws none.
  • The new datastore subtest was run on its own and passed.
  • The full ./internal/datastore/ tagged suite was run by hand: go test -tags=integration -count=1 ./internal/datastore/ reported ok in 468.446 seconds, measured 2026-09-03T14:50Z at e66be1ce7. The branch's own diff is byte-identical at d67b43713: git patch-id --stable reads 258c13017d9c against both merge bases. ./internal/format/maven/ was not re-run, because upload_liveness_race_test.go is unchanged since the run that last covered it.
  • markdownlint-cli2 and lychee were run by hand on the two edited documents and reported no error.
  • rumdl is absent from this environment, and no hook in this repository runs it.

e2e scenario catalogs

No scenario is added, and no scenario in docs/testing/ is affected.

docs/testing/e2e/maven.md carries five lifecycle scenarios: delete-version, delete-package, deploy-after-delete, soft-delete-invisible and management-delete-storage-counters. None of them covers a concurrent publish against a package mark.

The reason no scenario is added is that the residue is invisible to the Maven artifact read surfaces, which is where a client journey looks. Those reads filter the parent chain. addressableMavenPackage at internal/datastore/maven_packages.go:738-747 requires the package row to carry a null soft_deleted_at. FindMavenVersionByID at internal/datastore/maven_versions.go:671 wraps ErrNotFound for "a version whose package is soft-deleted", among the other breaks in that chain. A journey therefore has nothing to assert on.

That claim holds for the artifact reads alone, and this merge request does not widen it. components_count is served publicly on the NamespaceStatistics schema at api/openapi/v1.yaml:4477, and its own description there says it counts soft-deleted rows. The residue is observable on that column until the reaper drains the row.

docs/testing/e2e/docker.md:159 and docs/testing/e2e/oci.md:158 are the in-tree precedent for this shape. Both rows record a state that no docker or oras journey can reach and leave it to the datastore and management-API integration suites.

Scope and process notes

Diff size. 4 files, 106 insertions and 6 deletions against the merge base e604815a4, measured 2026-09-03T17:48Z. The 500-LOC guardrail does not fire, so this description carries no split justification.

No plan governs this issue. The issue carries type::bug, so the plan-MR guardrail does not apply and the title carries no step marker.

One decision, recorded in a spec and in a dev doc. This merge request changes docs/specs/ and docs/dev/ together, and it adds no production code. Both edited passages describe code that is already merged on main. MavenVersionStore.FindOrCreateVersion runs on the connection pool, and the package reap chunk takes 23503 and rolls back. No new implementation exists here for a dev doc to pair with. Each document states the condition that ends the acceptance, because a cross-link does not carry that condition to a reader who greps one file.

The pointer to this issue moves rather than disappears. On main, docs/dev/storage-accounting.md ends the Maven version leg with a pointer to work item 1044 for a choice between two outcomes. That choice is settled. This merge request replaces the sentence with the recorded outcome, a link to the S20-A error-cases table, and the #1135 fence pointer. After the merge, no file under docs/ names work item 1044. The S20-A row and this dev-doc sentence hold the record of the decision.

Three whitespace-only deletions, forced by the formatter. internal/datastore/lifecycle_reap_maven_integration_test.go loses one blank line at each of the base lines 848, 863 and 992. The copy of this file on main is not gofmt-clean, and the go-fmt pre-commit hook rewrote it when the branch first touched the file. gofmt -d on the main copy produces exactly these three hunks and nothing else. No assertion, comment or test body changes with them. The third figure was re-measured for this description rather than copied from the validation report, which carries 990 in one place and 992 in another.

ADR conformance. Checked by reading the mirrored ADRs under docs/adr/, and this change conforms. No mirrored ADR decides this window, and no mirrored ADR requires the fence. ./scripts/adr-freshness.sh exits 0 and reports "mirror current: no upstream decision commits since the last local change", measured 2026-09-03T12:13Z. One internal ADR never mirrors and is read in the internal handbook. ADR-007 carries the strongest clause, at docs/adr/007_database_schema.md:876: "a package delete marks the package row and writes no child row, so the versions beneath a tombstoned package carry no soft_deleted_at of their own, and neither a version-level scan nor a repository walk reaches that package while its repository is still live." A live maven_versions row under a tombstoned maven_packages row is that same post-delete shape, and the new spec row cites it.

Merge order

This branch takes placement A in ## Error Cases: a new row inserted at :1003, directly under the repository-tier row at :1002. That placement is what produces the conflict below, and it was taken so that a reader who meets one tier meets the other.

Merge request !2268 (merged) and this branch both edit ## Error Cases in docs/specs/S20-a-lifecycle-closed-beta.md. !2268 (merged) rewrites the repository-tier row at :1002 in place. This branch inserts a package-tier row at :1003 and changes no existing line of that table. git merge-tree reports a real content conflict in that one region. Whoever merges second resolves it by hand: keep !2268 (merged)'s rewritten :1002 row, then put this branch's row after it. The two rows describe different tiers, and neither replaces the other. origin/main against this branch is clean.

The conflict is mechanical, and this branch introduces it by taking placement A. There is no required merge order. Neither change depends on the other's content, so the cost is a rebase in that one table.

Re-measured 2026-09-03T17:48Z: !2268 (merged) is open and is not a draft, at head 263e6c36c. git merge-tree --write-tree --name-only 263e6c36c HEAD exits 1 and names docs/specs/S20-a-lifecycle-closed-beta.md as the one conflicted file. git merge-tree --write-tree --name-only origin/main HEAD exits 0 at origin/main 16bc85e39.

Related to #1044 (closed)

What remains is the fence itself: #1135 owns it, and #535's second scope clause stays open until it lands.

This is a bot message 🤖 — /smurfit

Edited by Pawel Rozlach

Merge request reports

Loading
Loading