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
COALESCEorIS NOT DISTINCT FROMwould list it. - Keyset order is proven against rows inserted out of id order (3, 1, 5, 2,
4), so a read with no
ORDER BYcannot pass by seeding accident. - Caller-owned
WrappedDEKon two rows carrying identical bytes, which is the fixture a reused scan buffer collapses. - The count matches on
ns_key_id, not the denormalizedns_key_version, becausens_key_idis 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. EXPLAINplans all three call shapes: the mid-sweep batch, the terminal empty call, and the count.- Generated SQL is asserted directly, because
CredentialKeyRowcarries no field for a credential ciphertext: a projection selectingencrypted_usernamewould 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'findsmaven_remote_encrypted_credentials.md:312,npm_remote_encrypted_credentials.md:401,container_remote_credential_columns.md:275, and the index comments in20260904140000_add_container_remote_encrypted_credentials.sql,20260904160000_add_npm_remote_encrypted_credentials.sqland20260904170000_add_maven_remote_encrypted_credentials.sql. - Other-words half.
git grep -niE 'sweep (reaches|does want)|rotation sweep .{0,40}(index|through)'addsmaven_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