feat(datastore): the credential table's two reads for Maven (S04-A per-format columns plan: 3a/11)

Stack

Merge in this order. Each part is reviewable on its own and green on its own.

# MR What it lands
1 this MR The shared credential-table helper for maven_remote_repositories and both of its read statements: the sweep's selection read, and retire-key-version's pre-flight count
2 !2457 (merged) The guarded write that completes datastore.CredentialTable, the exported constructor, the interface widening, and the registration with the re-encrypt command

What this part does

The rotation sweep's seam is merged with no SQL-backed implementation behind it, and NamespaceEncryptionKeyStore.CountRowsForVersion has a body that counts no table. This part lands the shared helper over Maven's generated jet columns and both read statements over it.

The one behaviour change is the count. retire-key-version's pre-flight stops returning a constant 0 and counts maven_remote_repositories rows referencing the target version. The sweep cannot run yet: the type does not implement datastore.CredentialTable here, because the write is part 2.

That split is deliberate. The interface anchor and the exported constructor land with the write, so main never carries a type claiming a method it has not got. Until part 2 merges, nothing in production constructs this type and the integration suite is what exercises it.

Why the split is here and not elsewhere

The count depends only on mavenRemoteCredentialTableSpec(), not on the write, so it belongs with the reads. Splitting read/write also keeps each half near half the LOC, and keeps this part free of any cmd/artifact-registry production change.

What pins the reads

  • Selection is asserted on one fixture holding every neighbouring state at once: below the version bound, at it, above it, credential-free, and another namespace's row. The credential-free row is the production common case (an anonymous upstream needs no credentials) and has no DEK to re-wrap, so an implementation reaching for COALESCE or IS NOT DISTINCT FROM would list it.
  • Keyset order is proven against rows inserted out of id order (3, 1, 5, 2, 4), so a read with no ORDER BY cannot pass by seeding accident.
  • Caller-owned WrappedDEK on two rows carrying identical bytes, which is the fixture a reused scan buffer collapses.
  • The count matches on ns_key_id, not the denormalized ns_key_version, because ns_key_id is the column the foreign key is on. A count matching the version would disagree with the refusal it exists to predict in exactly the state where the answer matters, so a case pins a row whose version has drifted.
  • EXPLAIN plans all three call shapes: the mid-sweep batch, the terminal empty call, and the count.
  • Generated SQL is asserted directly, because CredentialKeyRow carries no field for a credential ciphertext: a projection selecting encrypted_username would compile, pass every behavioural case, and transport the credential through the operator process. Only the statement text shows it.

Argument guards reject at each statement's own boundary rather than at the loop that will call it: the list guards ctx, the namespace id, version and limit, and the count guards ctx and the namespace id. A zero namespace id would otherwise bind, match nothing and return an empty batch with no error, which the sweep reads as a swept table; a limit below 1 does the same through LIMIT 0; a version below 1 renders ns_key_version < 0 and does the same again; and the count answers 0, the value that clears a retirement, for a namespace it never checked. A nil ctx is the odd one out, because it does not degrade quietly: it reaches database/sql's ctx.Done() on a nil interface and panics the operator process, where RemoteCredentialStore over this same table returns an error.

The four that name a sweep-specific failure mode reuse credential_sweep.go's existing sentinels rather than twinning them. The two ctx guards take guards.go's package-shared errNilContext, which that file documents as the home for exactly this wiring mistake.

Both reads' error arms are pinned: a read collapsing to (nil, nil) reports a table completed for a namespace it never swept, and a count collapsing to (0, nil) reads as a version nothing references — which is exactly what clears a retirement.

newCredentialTable validates the whole spec, not just the client. An incomplete spec is reachable only by a future format's instantiation, and without the check it passes construction and fails at the first statement of a sweep instead.

That check does not reach the count, and the count is not routed through it. credentialRowsForKeyVersionStmt builds its term with no constructor on the path, so credentialRowsForKeyCount takes the three generated jet values it uses rather than a whole credentialTableSpec. pg.Table and pg.ColumnString are interfaces, so this does not make the incomplete state unrepresentable: an explicit nil argument still compiles. What it removes is the silent case, the one a second format reaches. A struct field nobody assigned is a nil interface the compiler says nothing about; an argument nobody passed is a compile error, and the values a format passes come from its own generated table package, where none of the three is ever nil. credential_table.md states it in those terms, and says why no case pins it.

Corrections this part carries

Two facts this part settles were stated wrongly in sibling documents, and each was repeated in several places. Both sets were re-derived at 8104c43b5.

The (namespace_id, ns_key_id) index does not serve the sweep's selection

The merged CredentialTable contract requires keyset order by row id, so the plan that serves the selection without a sort leads on the primary key (id, namespace_id): namespace_id and ns_key_version are applied inside the scan, and the namespace_id bind prunes to one partition because it is the hash partition key, not because any index leads with it. Confining the scan within the partition would need a (namespace_id, id) index that no migration creates.

Six in-tree sites carried the pre-correction wording, and all six are fixed here. The set was derived two ways at 9003f1efa, because the wording is not shared:

  • Greppable half. git grep -niE 'namespace_id.? prefix|prefix the sweep|bounds the prefix' finds maven_remote_encrypted_credentials.md:312, npm_remote_encrypted_credentials.md:401, container_remote_credential_columns.md:275, and the index comments in 20260904140000_add_container_remote_encrypted_credentials.sql, 20260904160000_add_npm_remote_encrypted_credentials.sql and 20260904170000_add_maven_remote_encrypted_credentials.sql.
  • Other-words half. git grep -niE 'sweep (reaches|does want)|rotation sweep .{0,40}(index|through)' adds maven_remote_repositories_schema_integration_test.go:96, whose comment shares no phrase with the first half.

The three migration files are already on main, so each comment-only edit carries its own Allow-Migration-Edit: trailer. None changes what its migration does to an already-migrated database: each edits only the comment above a CREATE INDEX. The trailer values are folded onto continuation lines, because gitlint's B1 caps a message line at 80 and Allow-Migration-Edit: 20260904140000_add_container_remote_encrypted_credentials.sql is 83 characters on its own, so the same-line form database-migrations.md documents cannot pass the commit hook for the longer filenames. git interpret-trailers --parse rejoins the folded value, and scripts/ci/check-migration-immutability.sh origin/main exits 0 with all three authorised. The .gitlint gap is left for its own MR.

One site survives, and a merge gate follows from it. docs/adr/007_database_schema.md:434 states the claim at the top of the chain, and docs/adr/ is synced from the handbook rather than edited in tree. The handbook copy was read directly rather than through the local mirror, because the review could not establish the mirror's freshness (scripts/adr-freshness.sh returned 401): at the handbook's origin/main 13bd54e8f3cb the same sentence sits at the same line 434, so the mirror is not stale for this claim, and that line's basis commit there is 417f7b765c ("Artifact Registry ADR 007: publish the credential key-wrap columns"). No handbook amendment MR is open yet. While none has merged, the published ADR still carries the pre-correction claim, so the amendment MR is a merge gate on this MR, not a follow-up to track.

The list plan's cost is a share of a partition, not a namespace's row count

Index entries examined per batch scale as the batch limit divided by the target namespace's share of its partition. A namespace at the 10,000-repository operational limit fills a batch in roughly the limit's worth of entries, so it is the cheapest case; a namespace holding a handful of rows in a well-populated partition walks the partition to find them. Corrected in credential_table.md, in docs/specs/S04-a-column-level-encryption.md's acceptance paragraph, and in the CredentialTable contract itself (credential_sweep.go and docs/dev/column-encryption.md).

That acceptance paragraph now prices the accepted trade-off on the partition's row count. No figure for it is published anywhere in tree, so the paragraph names the quantity and its derivation instead of a number: the sum over the namespaces that hash to the partition, which ADR-004's cap of 1,000 repositories per namespace per artifact type does not bound, because nothing caps how many namespaces land in one of the 64 partitions.

The 10,000 figure stays where it still bounds something: the ns_key_version cheap-filter rationale at credential_table.md:200-202, container_remote_credential_columns.md:285 and its migration comment, and the nonce budget at S04-a-column-level-encryption.md:1974. Where it survives as the cheapest case rather than as a bound, the text now says which it is.

The at-scale EXPLAIN residual has an owner. It is recorded as a note on #417 rather than filed as a new work item, and credential_table.md names #417 at the point the residual is stated.

CredentialTable's doc comment shrank to a pointer. Editing it makes scripts/ci/check-comment-caps.sh re-check the whole block, 21 lines against an exported name's cap of 3, with no waiver by design. It is now three lines pointing at ## Joining the rotation sweep in column-encryption.md, which already carried the same obligations in more detail and is where the corrected sargability wording lands. Each of the five was matched against that section before the compression: the atomic guard-and-write, caller-owned slices, row-id order past the cursor, the at-most-limit bound, and the index-support obligation with its minute bound, limit semantics and two EXPLAIN shapes.

Two sites keep the old wording on purpose, because a step MR does not edit a plan file (AGENTS.md Guardrails): docs/plans/2026-09-03-s04a-per-format-credential-columns.md:869 and docs/plans/2026-08-03-s04b-rotation-tooling.md:259. They belong to the plan's single writer.

Reviewable size

1384 LOC, over the 500 ceiling (development-model.md), by file group. Lines added against the merge base, re-derived at 19b47a5b3 with git diff --numstat $(git merge-base origin/main HEAD)..HEAD. Files under internal/testutil/ count as test support alongside *_test.go:

Group LOC
Production Go 207
Tests and test support 609
Markdown (sidecar, docs, spec) 545
SQL migration comments 23
Total 1384

Splitting further would cut inside credential_table_integration_test.go and its sidecar, which are one reviewable unit: the sidecar exists because scripts/ci/check-comment-caps.sh caps a _test.go comment block at two lines, so the rationale for each assertion has to live beside the suite rather than above it. Production Go is 207 lines of the total, and the comment-only corrections above are the 23 SQL lines plus 69 of the markdown lines.

Testing

Beyond the suite in the diff: go build ./..., go vet -tags=integration ./..., gofmt, scripts/ci/check-comment-caps.sh --base origin/main, the untagged suites, and the integration suites for internal/datastore and cmd/artifact-registry all run green on this branch alone, against a PostgreSQL with max_locks_per_transaction=1024.

Every guard and error arm added here was measured rather than assumed: deleted one at a time, with the named case confirmed to redden. The sidecar's mutation table records all twenty rows and says which pass measured each.

scripts/ci/check-migration-immutability.sh origin/main exits 0 with the three comment-only migration edits authorised, and scripts/ci/check-comment-caps.sh --base origin/main passes on the blocks this round touched.

CI's lint job never compiles //go:build integration files, so the delta is reported here rather than left unmeasured. Run at 19b47a5b3:

golangci-lint run --build-tags=integration --new-from-rev $(git merge-base origin/main HEAD) \
  --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false \
  ./internal/datastore/... ./cmd/artifact-registry/... ./internal/testutil/...

It reports 0 findings. An earlier round of this description recorded 6, all on two sites in this part's own new suite: a trailing blank line before the closing brace at credential_table_integration_test.go:363-364 and :392-393, reported once each by whitespace, wsl and wsl_v5. Both blank lines are removed in 19b47a5b3, which is why the count is now 0.

No docs/testing/ scenario is added or affected: the catalogs cover per-format user-facing request flows, and this part changes one operator command's diagnostic count with no HTTP surface.

Related to #417

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading