feat(datastore): add npm's encrypted credential columns (S04-A per-format columns plan: 9/11)
What
Gives npm_remote_repositories its encrypted credential unit: four nullable
columns (wrapped_dek, ns_key_id, ns_key_version, encrypted_auth_token),
an all-or-none CHECK over exactly those four added NOT VALID and validated in
a second migration, a (namespace_id, ns_key_id) index, and a composite
foreign key to namespace_encryption_keys (id, namespace_id) with
ON DELETE RESTRICT.
Schema only. Nothing reads or writes the new columns yet: no production write
path sets them, and the only statements that write the four are the inline
INSERT and UPDATE SQL in the migrations suite. The names do occur in
non-generated Go outside the two regenerated jet files, but only inside
comments, so grepping one returns prose rather than writers. npm's
unit is four columns rather than Maven's and Container's five because npm
stores a single bearer token; the shape for all three is fixed in
S04-A's per-table instantiation,
and every constraint, index and FK name here matches that section.
tmp_plaintext_auth_token stays, outside the unit, and stays the live
credential store. Unlike the Maven and Container tables this one carries no
CHECK over its plaintext column, so this migration adds a constraint rather
than replacing a predecessor.
The reasoning behind each assertion lives in
internal/datastore/migrations/npm_remote_encrypted_credentials.md, because
check-comment-caps.sh caps a _test.go comment block at two lines.
Merge order
!2359 (merged) (step 6, Container's columns) merged on 2026-09-07. The plan's step 9
records Depends on: Step 6, for merge order only, and the two migrations here
are stamped 20260904160000 / 20260904160100, above container's
...140000 / ...140100, so lint:migration-ordering was satisfied in that
order and there is now nothing left to order against.
That dependency is now discharged. This branch is rebased on main at
dbf8b383f: the one conflict was migrations_checksum_test.go, where
knownHeadVersion resolves to 20260904160100, since npm's stamps sort above
container's ...140100. structure.sql did not conflict, which is the
case worth checking rather than trusting, because this branch's copy carried no
container credential constraints while main's carries 65 and the two sets land
in disjoint regions. Post-rebase it holds main's content plus this step's 586
lines with zero removals, and both credential constraint sets are present at 65
each. db:structure-check is the byte-exact gate and runs on this head for the
first time.
| Step | Change | MR |
|---|---|---|
| 1 | Bind the table and row identity into the column AAD | !2349 (merged) (merged) |
| 6 | Add Container's encrypted columns | !2359 (merged) (merged) |
| 9 | Add npm's encrypted columns | this MR |
Three open MRs also touch migrations_checksum_test.go and structure.sql:
!2411 (merged), !2277 (merged) and !1011 (closed). With this branch rebased, the hazard is between the
four of them rather than with main: knownHeadVersion is a single line and
structure.sql a generated dump, so whichever lands second must rebase and
regenerate, and no pipeline reports the overlap in advance. Do not settle it
from GitLab's conflict flag — !2277 (merged)'s flipped from conflicting to clean over a
few hours on 2026-09-08 with its own head unchanged, so the flag answers for the
moment it was read and not for merge time. Derived on 2026-09-08 by reading the
changed paths of the open MRs whose titles cover datastore, migrations or schema
work, so the list is complete to that bound rather than to a scan of every open
MR.
What this does not reach
Named here because each is silent, and each is checkable:
CountRowsForVersionstill counts no table. Its body is a constant0, andretire-key-version's pre-flight logs that count, so once a write path populates these columns the pre-flight can log0for a version the new FK then refuses to release. The refusal is the fail-closed direction; it is the diagnostic that under-reports. #417 (closed) owns extending it per table.deleteNamespaceKeysTxhas no sentinel for the new refusal. The retirement path maps23001/23503throughisKeyStillReferencedErr; the post-shred hard delete wraps generically, so a refusal there cannot be told from a transient failure. Unreachable while no write path setsns_key_id.- The credential FK has no classifier arm. S04-A has the caller key its
retry on the constraint name, and the fallback renders with
%v, which drops thePgErrorfrom the chain, so no caller canerrors.Asits way toConstraintName. The name itself still reaches a log, inside the violation message. The write path owes an arm.
Reviewable LOC
2157 reviewable LOC (2177 total, less 20 in the two // Code generated by go-jet DO NOT EDIT. files), measured at 5a5689ec7 against merge base
dbf8b383f, past the 500 the development model asks about.
Splitting would not help: the migration pair, the schema dump it produces, the
suite that pins it and the prose explaining that suite are one reviewable unit,
and separating them would put a schema change in one MR and its only evidence
in another.
| LOC | Group |
|---|---|
| 745 | tests (.go) |
| 586 | structure.sql — generated dump, 0 removals, 65 identical copies per object |
| 506 | sidecar docs (.md) |
| 308 | migrations (.sql), rationale comments included |
| 12 | production Go — one doc comment on an existing sentinel |
| 20 | jet output, generated, excluded from the count |
The two largest groups are the cheapest to read. structure.sql reduces to 65
copies each of the four columns and the CHECK (parent plus 64 partitions), 65
CREATE INDEX plus 65 ATTACH PARTITION, one ON ONLY index and one
ADD CONSTRAINT; judge it against the migrations rather than line by line.
Testing
test:integration runs PostgreSQL 16, 17 and 18; a local run covers 16.
| Assertion | Test |
|---|---|
| Exact column set, type, nullability | TestNPMRemoteSchema_Columns |
| All 2^4 subsets: 2 whole-record arms admitted, 14 partial refused | TestNPMRemoteConstraints_EncCredentialsAllOrNone |
CHECK covers npm's four, names no plaintext or basic-auth column, convalidated |
TestNPMRemoteSchema_EncCredentialsCheckIsValidated |
FK columns, referent, confdeltype, confupdtype, confmatchtype from the catalog |
TestNPMRemoteSchema_EncCredentialsFKShape |
| INSERT and UPDATE refused for an unresolvable key row, including cross-namespace | TestNPMRemoteConstraints_EncCredentialsFKRejectsUnknownKeyRow |
| Key-row delete refused, then permitted once the unit is cleared | TestNPMRemoteConstraints_EncCredentialsFKBlocksKeyRowDelete |
| Index clones on all 64 partitions, exact key columns | TestNPMRemoteSchema_EncCredentialsIndexOnEveryPartition |
| CHECK and FK clones on all 64 partitions | TestNPMRemoteSchema_EncCredentialsUnitReachesEveryPartition |
Two-file split, Down symmetry, no NO TRANSACTION, measured row count, and the Down's one SET/RESET lock_timeout pair bracketing every drop |
TestNPMRemoteEncCredentials_MigrationTextSplitsTheValidate |
Each pinned secondary index is present, has its key columns, is partial only where intended, and is not UNIQUE |
TestNPMRemoteSchema_SecondaryIndexesExist |
| Columns stay absent from the virtual tables | TestNPMVirtualRepositoriesSchema_Columns, ..._CarriesOnlyTheIDVersionCheck |
No docs/testing/ e2e scenario is added or affected: nothing reads or writes
the new columns, so no request-path behaviour changes and no scenario can
observe the unit. The e2e catalogs become relevant at step 10, which moves
npm's reads and writes behind RowEncryptor.
Migration safety
Applied against PostgreSQL 16.15. ADD COLUMN with no default is
metadata-only; the CHECK is added NOT VALID, so it binds every new write
immediately and defers the scan; the FK must be added validating because
PostgreSQL 16 and 17 refuse NOT VALID on a partitioned referencing table
(SQLSTATE 42809).
The split into two migrations buys the transaction boundary, not operator
scheduling: migrations.Up applies every pending migration in one pass and
exposes no UpTo or UpByOne, and mise run db:migrate is goose up, so
nothing here can run the two apart. Separate migrations are separate
transactions, which is what releases the ADD's ACCESS EXCLUSIVE before the
scan takes the lighter SHARE UPDATE EXCLUSIVE, and what lets a failed scan
retry without redoing the ADD.
Rolling deploys are safe in both directions: all four columns are nullable, the
old binary's INSERT lists its columns explicitly and leaves the unit all-NULL,
which satisfies the CHECK from the moment it lands. The Down names the one
condition under which reversal stops being safe — a binary that has stored
credentials in these columns — and bounds each of its lock acquisitions with
SET lock_timeout = '5s', because a rollback runs against a serving database
and this table is read on live paths. That bound is per acquisition, not per
section: three of the four dropping statements acquire across 65 relations (the
parent and its 64 partitions) and the FK DROP CONSTRAINT reaches
namespace_encryption_keys and its 64 partitions too, for 130, so a rollback
window is sized as a multiple of 5s rather than as 5s. No statement_timeout
is added, which keeps the failure mode a stall-then-rollback and keeps the
section with its sibling's precedent.
Database Review Evidence
Migrations
Note
Timings are from CI (db:migrate matrix, goose verbose) against an
empty database, in apply / rollback order per PG version.
Production-scale validation via Database Lab is not yet available. See
Database review evidence
for the matrix rationale and how to read the numbers.
| Migration | PG 16 | PG 17 | PG 18 |
|---|---|---|---|
20260904160000_add_npm_remote_encrypted_credentials.sql |
OK (391.21ms / 173.44ms) | OK (277.23ms / 168.85ms) | OK (422.63ms / 172.14ms) |
20260904160100_validate_npm_remote_enc_credentials_check.sql |
OK (14.74ms / EMPTY, 2.52ms) | OK (15.24ms / EMPTY, 3.46ms) | OK (17.79ms / EMPTY, 2.11ms) |
Collected from pipeline
#2825464906,
a merged-results pipeline on 3f2d105a5, which merges this branch's head
50e22818b into main 2b6f70fce. All three matrix legs passed.
Migration notes:
- The validating migration's rollback is reported as
EMPTYon all three legs rather than as a duration, because its-- +goose Downcarries comments and no statement, so the times in those cells are goose's own overhead and not work. That emptiness is deliberate, and theDownsays why: PostgreSQL has no statement that returns a validated constraint toNOT VALID, and a rollback therefore leaves the constraint validated over the same predicate, which enforces the unit on new writes identically. It is a deliberate divergence from step 6's equivalent,20260904140100_validate_container_remote_enc_credentials_check.sqlin !2359 (merged), whoseDowndoes drop the constraint and re-add itNOT VALID; npm'sDowncomment names that route and declines it as buying nothing back forACCESS EXCLUSIVEon 65 relations. - No version-specific regression. For both migrations the slowest leg is within 1.17x of the second-slowest (adding: 422.63ms on PG 18 against 391.21ms on PG 16, 1.08x; validating: 17.79ms on PG 18 against 15.24ms on PG 17, 1.17x), against the 2x that would flag one.
- Neither migration reaches 1s on any leg. The relevant ceiling is the
5-minute
upTimeoutininternal/datastore/migrations/runner.go, which caps the wholemigrations.Uppass (advisory-lock wait plus every pending migration, not one migration), and these two together add at most 440.42ms to that pass, on PG 18.
What this evidence does not cover, stated because the table reads as complete and is not:
- No query plans, because query mode does not apply. No changed
hand-written, non-test Go file adds or modifies a query-producing method.
The one file that reaches that filter,
internal/datastore/npm_remote_repositories_errors.go, is a doc-comment edit that dispatches no statement, and the two files underinternal/datastore/jet/are regeneratedDO NOT EDIToutput. - No lock measurement. These are apply and rollback durations against an
empty database. They do not measure the lock hold, nor the wait to be
granted it: neither
Upsets alock_timeout, both carrying therequire-timeout-settingssquawk exemption under #548, so that wait is bounded in scope but not in time. The adding migration'sDowndoes setlock_timeout = '5s', and no number here exercises it. - No at-scale validation. Lock-acquisition timing and row counts at
production scale need Database Lab, which this project does not have; see
the note above the table. The row-count claims in
## Migration safetyand in the migrations' own comments rest on the 2026-09-03 observation thatnpm_remote_repositoriesheld no production rows, not on anything measured here.
Related to #417 (closed)