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_version inside 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 beginReadCommitted helper. Under REPEATABLE READ the blocked write aborts 40001 instead 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 repositories on every statement; these three join none of it. The ns_key_id foreign key is ON 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.
  • EXPLAIN pins 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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading