refactor(datastore): refuse the connection pool on Maven ReVerify*Alive

What this delivers

A run-time guard on three merged Maven store methods. MavenRepositoryStore.ReVerifyRepositoryAlive, MavenPackageStore.ReVerifyPackageAlive and MavenVersionStore.ReVerifyVersionAlive now refuse a *sql.DB connection pool. Each returns a sentinel of its own store: errMavenRepositoryNotInTransaction, errMavenPackageNotInTransaction and errMavenVersionNotInTransaction. The new arm sits after the db == nil arm and before the zero-UUID arms, so the pool is refused before any statement runs. Merged precedent !2268 landed the same guard on the npm and container ReVerifyRepositoryAlive methods and left the Maven three to this issue.

The cause. The three methods declare the handle as db qrm.DB, and both *sql.Tx and *sql.DB satisfy that interface. The contract of each method is a row lock held to the caller's commit, and that is a property of the dynamic type. The compiler cannot check it. The guard chain tested ctx, db and the UUIDs, and never the dynamic type. On a pool, database/sql runs the SELECT in an implicit transaction of its own. That transaction commits when the statement returns, so the lock ends with the statement and fences nothing for the caller. The method still returns nil, and the caller reads a verdict that nothing holds.

The lock mode is not the same in all three. ReVerifyRepositoryAlive takes FOR SHARE. ReVerifyPackageAlive and ReVerifyVersionAlive take FOR UPDATE. Both modes are transaction-scoped, so a pool defeats all three for the same reason.

The guard is latent, not live. Every production call site passes a transaction today, so this change alters no behavior that the running service can reach. The issue states the same thing and asks for the guard as defense in depth. This is why the description carries no runtime evidence. A manual run of the service drives the same code paths before and after the change.

Two corrections to the issue

Research that contradicts the issue is stated here rather than applied silently.

The opening sentence names one lock mode for all three methods. It says the three methods take a SELECT ... FOR SHARE on a parent row, and that is accurate for one of the three. The measured modes are the ones named above. Each method's own doc comment already states its mode, and the exclusive mode on the package and version methods is deliberate. The ask itself is unaltered, because a pool ends the lock with the statement in either mode.

The line reference for ReVerifyVersionAlive has drifted. The body cites internal/datastore/maven_versions.go:858. At origin/main 7307bd94a the method sits at line 860. A merged change added a gitlabUserID *string parameter to MavenVersionStore.FindOrCreateVersion above it and moved it down two lines. The body's number is correct at the sha the body names, so this is drift and not an error in the issue.

Coverage table

Verbatim from the authorship commit.

Coverage table. No plan governs this issue, so the rows are the issue
card's own `acceptance` items, in order.

Legend:
R-AG = TestMavenRepositoryStore_ReVerifyRepositoryAlive_ArgumentGuards
P-AG = TestMavenPackageStore_ReVerifyPackageAlive_ArgumentGuards
V-AG = TestMavenVersionStore_ReVerifyVersionAlive_ArgumentGuards
R-IT = TestMavenRepositoryStore_ReVerifyRepositoryAlive (integration)
P-IT = TestMavenPackageStore_ReVerifyPackageAlive (integration)
V-IT = TestMavenVersionStore_ReVerifyVersionAlive (integration)

| # | Covered by |
|---|---|
| 1 | R-AG "connection pool" row; fails on the unguarded method |
| 2 | P-AG "connection pool" row; fails on the unguarded method |
| 3 | V-AG "connection pool" row; fails on the unguarded method |
| 4 | Three distinct sentinels, one per store file, table-prefixed |
| 5 | Placement is the implementation's; no test can assert it here |
| 6 | R-AG, P-AG, V-AG each carry one bare `&sql.DB{}` row |
| 7 | R-AG, P-AG, V-AG runners each gained a NotErrorIs assertion |
| 8 | R-IT 4, P-IT 3, V-IT 4 sites take beginTx; the grep is empty |
| 9 | Nine changed files, all under internal/datastore; db stays qrm.DB |
| 10 | FOR SHARE named for the repository, FOR UPDATE for the other two |
| 11 | R-IT, P-IT, V-IT each gained a transient-failure subtest |
| 12 | Those three subtests: tx on the live context, cancel second |

Row 5 states its own limit. The placement of the new arm between the nil-db arm and the zero-UUID arms is a property of the source. No test in this suite can tell it apart from a correct arm placed elsewhere. A reviewer checks it by reading the three methods.

What failed before the change, and what did not

The reproduction is the three connection-pool rows. Each of the three argument-guard tables gained one row that binds db to a bare &sql.DB{} with otherwise valid arguments. Before the guard, no arm refused the handle, so it reached the statement. A zero-value *sql.DB has no connector, so database/sql.(*DB).conn dereferences a nil pointer and the test panics. That panic is the proof, because a guarded method never reaches the statement. After the guard, all three rows return their store's sentinel.

The three transient-database-failure subtests are not the reproduction. Each of the three integration suites gained one, and all three pass before and after the guard by design. They pin the qrm.ErrNoRows mapping, so that a transient failure is not reported as ErrNotFound. They are mirror-gap coverage inside this unit's scope. Do not read "some new tests passed before the change" as a reproduction that did not reproduce.

The rest of the test change. The 11 integration call sites that passed client.DB() to these three methods now pass a *sql.Tx opened on the test client. The split is 4 in the repositories suite, 3 in the packages suite and 4 in the versions suite. Each of the three runners gained a require.NotErrorIs(tt, err, ErrNotFound, ...) assertion alongside its existing require.ErrorIs.

Where the refused-pool contract is stated

The contract sits on a trailing comment on each db qrm.DB parameter line. Each of the three signatures is reflowed one parameter per line to make room for it. The three comment lines measure 144, 141 and 141 columns against the lll ceiling of 160. Merged precedent for this pattern, in the same package, is internal/datastore/npm_metadata_files.go:224 on NpmMetadataFileStore.DeleteNpmMetadataFiles.

The exported doc comment is not the site, and the reason is the comment caps. docs/dev/go-style.md caps an exported doc comment at 3 lines, and it charges every line of a block once an edit lands in that block. Two of the three blocks are already over the cap: 8 lines on ReVerifyRepositoryAlive and 19 lines on ReVerifyPackageAlive. The third, ReVerifyVersionAlive, is 3 lines and sits exactly on the cap, so all three have zero headroom. They are legal only while the diff leaves them untouched. One added line charges the whole block, and the packages block then has to come down to 3 lines. That compression erases 16 lines of live rationale about why the lock is FOR UPDATE and not FOR SHARE. The guide's second outcome for this case is to relocate the claim to a site with budget. It names a trailing comment on the declaration line as one such site. A trailing comment shares a line with code, so scripts/ci/check-comment-caps.sh never counts it as a block line.

Two things are measured at this branch head. The caps gate returns OK. And no doc block has a line of margin: the ReVerifyRepositoryAlive block ends on the line before its func, and the other two are separated from their func only by a bare // and a //nolint line, which the caps gate neither counts against the cap nor reads as a break in the block. The effective margin is zero in all three, so an edit that grows any of the three signatures upward needs the caps gate run again.

The transient-failure comment differs from both merged copies, on purpose

Each of the three new Maven subtests carries a two-line comment. Line 2 arrived as a verbatim copy from main, and it claims a causality the code does not have.

The copied line reads: "The transaction opens on the live context, so the cancelled one reaches the statement rather than the pool guard ahead of it." The premise and the conclusion are both true and the connective is false. beginTx (internal/datastore/npm_write_integration_test.go:118) takes no context and opens the transaction on t.Context() internally. The live context is therefore a property of the helper, and not of the statement order. The pool guard reads if _, pool := db.(*sql.DB); pool and never inspects a context, so what clears the guard is the handle's dynamic type.

The three Maven files now read: "The handle is a transaction, so the cancelled context fails the statement itself rather than the pool guard answering ahead of it."

Both merged copies of the old line stay as they are, at internal/datastore/npm_repositories_integration_test.go:600 and internal/datastore/container_repository_integration_test.go:467. A correction to the two copies on main is not this merge request's to make. So the three Maven files diverge from both merged copies on purpose, and a reviewer comparing them is not looking at an oversight.

Pre-existing comments this change leaves alone

internal/datastore/maven_repositories_integration_test.go:267 and internal/datastore/maven_repositories.go:252 name S20. Both lines are already on origin/main and this branch edits neither one. AGENTS.md bars a sprint identifier in a Go comment, and work item #489 tracks that cleanup repository-wide. The signature reflow moved the second line from 246 to 252, so it sits next to the diff without being part of it.

Each of the three integration files has a holds a FOR ... lock subtest. The opening comment block of those subtests runs 8, 5 and 5 lines, against the 2-line cap for a test file. The diff touches none of the three blocks, and the caps gate charges only the blocks a diff touches.

Lint

No //nolint directive is added anywhere on this branch. The new beginTx call lines emit contextcheck without suppression. That matches their unchanged neighbors in the same functions, and it matches !2268, which introduced the same beginTx(tt, client.DB()) construct with no suppression. A function-level directive would silence dozens of pre-existing findings in the same package, and that is a lint-policy change wider than this unit.

Use mise exec -- golangci-lint and not a golangci-lint on the PATH. A 2.12.2 binary stops on config parse with unknown linters: 'exhaustruct_v5' and reports no findings at all, which reads as agreement.

Commit hygiene

The branch was authored test-first. The tests landed in their own commit before the guard. That commit used the project's sanctioned pre-commit bypass, because its tests fail the go-test hook by design. Every later commit ran the full hook chain, with no bypass and no SKIP=. The branch is squashed to one commit for this merge request, so those authorship commits are not in its commit list.

End-to-end scenario catalog

Nothing in docs/testing/ is added or affected, and the reason is stated here rather than left silent. The change adds no endpoint, route, middleware, migration, storage path or configuration semantic. It does not change how the service builds, boots or dispatches a request. It guards an argument on three datastore methods, which is not Maven protocol behavior, so the format conformance suites do not reach it either. The catalog guardrail applies to behavior-changing types, and the type here is refactor.

Reviewable size

9 files changed, 208 insertions and 61 deletions, all under internal/datastore/. Measured with git diff --shortstat origin/main...HEAD at branch head 062690866 against origin/main 7307bd94a, which is also the merge base. The squash before this merge request does not change the tree, so these are the post-squash figures.

The split by file group, insertions and then deletions:

• Three store files: 60 and 9.
• Three unit test files: 36 and 13.
• Three integration test files: 112 and 39.

Reviewable LOC is the numstat total of 269, with no vendored, generated or binary file in the diff. That is under the 500-line ceiling in docs/dev/development-model.md, so neither a split nor a size justification is owed.

Merge order against !2331

There is no dependency between this branch and !2331, and no merge order is required for correctness. One file collides, and whichever merge request lands second owns the manual merge.

The measurements, and the heads they were taken at. Two git merge-tree --write-tree --name-only runs, with git 2.48.1, at 2026-09-06T20:46Z:

Run	Result
origin/main 7307bd94a against !2331 head 4ceddcee3	exit 0, no conflicting file
this branch head 062690866 against !2331 head 4ceddcee3	exit 1, one conflicting file, internal/datastore/maven_versions.go, one hunk

At that time !2331 was open, not a draft, with merge_status can_be_merged, has_conflicts false and detailed_merge_status not_approved. So !2331 merges into the base on its own, and the whole remaining conflict comes from the order of these two merge requests.

A later reader must measure these fields again before they act on this section. The head sha of !2331 comes first, because it moved once on 2026-09-06 already. An earlier version of this section measured a0e1cb099, and a rebase replaced it with 4ceddcee3. The other three are the merge fields of !2331, this branch head, and both merge-tree results. Each figure here belongs to the two heads named beside it, and it says nothing about any later pair.

The conflict. It is the errMavenVersion* sentinel var block. Both sides add a sentinel immediately after errMavenVersionNilDB. This branch adds errMavenVersionNotInTransaction, which refuses a *sql.DB pool on ReVerifyVersionAlive. !2331 adds errMavenVersionTransactionHandle, which refuses a *sql.Tx on FindOrCreateVersion. The two guards point in opposite directions.

Three things the second merge request must get right.

1. Keep both sentinels. An answer that keeps one of them silently removes a guard.
2. Do not trust the tests of one side alone. Each guard has its tests in files the other side does not touch, so a half answer still passes in the wrong place.
3. Do not cross the wiring. The two names are near-homonyms with inverse meanings, and a merge that keeps both values under the wrong methods still compiles. The three argument-guard tables catch this case, because each row names the sentinel it expects.

Note

git merge-tree compares two endpoints, so one conflicting file is a floor and not a ceiling for what a rebase hits. A rebase replays each commit in turn, and a commit can collide on a block that the endpoint comparison merges without a mark.

Issue

Related to #1166

This merge request takes the Related to form and not the form that ends the issue on merge. The reason comes from the validation pass. The issue states no acceptance criteria of its own. All 12 items checked are the validation run's reading of the issue Scope list and of one answered escalation. They are not the author's words. Every one of the 12 has named evidence in the change and the verdict is PASS. To end the issue on a derived reading takes away the author's chance to disagree with that reading.

Work item #1188 (closed) carries what remains: the wrong-format negative, the cross-namespace negative and the remote repository kind, all in the Maven repository suite. Its own body puts the transient-database-failure subtest in this unit, and this branch carries it.

Database Review Evidence

Queries

Note

/db-review-prep ran in query mode against head 062690866. No new or changed query-producing method was found, so there is no plan to collect. The stmt chain of each of the three ReVerify*Alive methods is byte-identical to the merge base 7307bd94a, and the lock modes are unchanged (FOR SHARE OF repositories, and FOR UPDATE on the package and version methods). The change adds a handle-type guard ahead of an unchanged SELECT. Migration mode did not run: the diff carries no file under internal/datastore/migrations/sql/. See Database review evidence for the methodology.

This is a bot message 🤖 — /smurfit