feat(datastore): the credential table's guarded write, and register it (S04-A per-format columns plan: 3b/11)
Stack
| # | MR | What it lands |
|---|---|---|
| 1 | !2456 (merged) (merged) | 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 | this MR | The guarded write that completes datastore.CredentialTable, the exported constructor, the interface widening, and the registration with the re-encrypt command |
Part 1 is merged, so this MR now targets main and the order is settled. It
adds UpdateRowKey to the type part 1 introduced, and widens the
CredentialTable interface part 1's statements are written against.
Rebased onto main after part 1 merged
The rebase brought in part 1's later review rounds. Two of them add guards:
a nil-context guard on ListRowsBelowVersion and on the pre-flight count, and
a version < 1 guard on ListRowsBelowVersion. Unguarded, a nil ctx does
not degrade: database/sql calls ctx.Done() on a nil interface and the
operator process dies.
The write reaches ExecContext on the same path, so it takes the same guard
here, with its own subtest in TestMavenRemoteCredentialTable_ArgumentGuards.
Measured, not assumed: with the guard deleted, that subtest panics inside
database/sql.(*DB).ExecContext. The guard is the only code this rebase adds
beyond part 2's own change.
Two of part 1's corrections also travel into the shared sidecar and are kept
over the branch's older wording: the costliest namespace shape for the
EXPLAIN obligation is a small namespace inside a well-populated partition
rather than the largest namespace the table holds, and the table name in the
error text serves the operator's reading of the failure rather than the abort
log, which sources it from TableName().
What this part does
It completes the shared helper and wires it in: the guarded write, the interface
anchor and the exported constructor that together make credentialTable a
datastore.CredentialTable, and the registration of Maven's table with the
operator's re-encrypt command. After this, a namespace sweep re-wraps real rows
for the first time.
The write takes the namespace id
namespace_id is maven_remote_repositories' hash partition key, so without it
no conforming implementation can prune. Measured: the unpruned write probes all
64 partitions and their index locks to reach one row; the bound write probes
one.
It is also what makes the write tenant-scoped rather than trusting the id space.
id is not unique across namespaces — the primary key is (id, namespace_id)
per partition and no index constrains id alone — so a row id present in two
namespaces is a shape the schema permits. Server-generated UUIDv7 makes it
unreachable in practice, which is why this is scoping by construction rather
than a defect that was live.
Widening a merged interface is what obliges this part to move its existing
implementors in the same commit: the statement-bound decorator in
cmd/artifact-registry and the four test fakes. S04-B published the narrower
signature in its ## Credential table seam section, so that listing is
amended here. The same section's "partition-pruned by namespace_id" sentence
needed no change — it becomes true of both statements once the write binds it.
What the write's cases pin
- The guard comparison and the write are one atomic statement. A straggler
re-keys the row at the same
ns_key_versioninside an uncommitted transaction, the write-back blocks on the row lock, and on commit the guard is decided against the committed row:updated=false, the straggler's bytes intact. A read-then-write implementation passes every committed-state case and loses this one, which is why the case exists beside them. - That interleaving runs under READ COMMITTED explicitly, through the
package's
beginReadCommittedhelper. Under REPEATABLE READ the blocked write aborts40001instead of re-checking, so the interleaving under test never happens and the assertions describe an outcome the database will not produce. - A same-version straggler re-key is a skip, a deleted row is a skip, a sibling row with identical guard bytes keeps its wrap, and the same row id in another namespace is not reached — the case only the namespace bind can decide.
- No statement gates on the parent repository, and a case says so. The
sibling credential store joins
repositorieson every statement; these three join none of it. Thens_key_idforeign key isON DELETE RESTRICT, so a soft-deleted parent's row still blocks its key version's retirement and the sweep has to re-wrap it. A case seeds a soft-deleted parent and asserts the row is listed and re-wrapped, the count case soft-deletes one of its two version-1 parents, and the sidecar names the three parent shapes the absence admits. EXPLAINpins one partition and all three binds reaching the statement. Which index serves it is deliberately not asserted: with the namespace id bound the planner swaps between the primary key and(namespace_id, ns_key_id)on row counts the fixture cannot hold steady.
The registration is covered twice, because nothing else in the tree reads that line: once on the registry's contents, and once by sweeping real Maven rows end to end through it and asserting a re-run reports zero.
Operator-facing corrections
docs/runbooks/key-rotation.md said, in the present tense and calling it the
current deployment state, that no credential table is registered — and its
troubleshooting table called the zero-tables log line Expected, action
None. Both become wrong the moment this merges, and an on-call reading them
mid-rotation would be told the normal output is the abnormal case. Both are
rewritten.
The zero-registration arm itself stays: S04-B's acceptance criteria require that
sweep to exit 0 and log, and it stays at Info because this package reserves Warn
for refusals (requireWarnRefusalAttrs asserts every WARN line carries refusal
attributes). What it gains is a note that an empty production registry is a
composition defect rather than a completed rotation.
namespace_encryption_keys_rotation.md gains the lock order the two commands
take. Retirement takes the FOR UPDATE barrier over the key rows and then locks
referencing credential rows through its RESTRICT check; this write holds the
credential row and then takes FOR KEY SHARE on the key row it stamps. Run
concurrently on one namespace they can deadlock, and isKeyStillReferencedErr
deliberately does not map 40P01, so the recovery is a re-run. Not reachable
until a write path populates the columns; this part is what makes the shape
structurally possible.
Reviewable size
1034 added lines against main, over the 500 ceiling
(development-model.md), by file group:
| Group | LOC |
|---|---|
| Production Go | 123 |
| Tests and test support | 608 |
| Markdown (sidecar, runbook, docs, spec) | 303 |
| Total | 1034 |
This is already the smaller half of a split made to get under the ceiling: part
1 as merged added 1384 lines, so the pair is 2418. Cutting further would
separate UpdateRowKey from the cases that prove its guard is atomic, which is
the one property no other case in the suite reaches.
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, against a PostgreSQL with
max_locks_per_transaction=1024.
The namespace bind, the write's two guards and both new error arms were
measured rather than assumed: deleted one at a time, with the named case
confirmed to redden. The absent parent gate was measured the other way round,
by composing activeParentPredicate into each of the three statements in turn
and confirming which case reddens. The sidecar's mutation table records every row and says
which pass measured each.
No docs/testing/ scenario is added or affected: the catalogs cover per-format
user-facing request flows, and this part changes one operator command with no
HTTP surface.
Related to #417 (closed)