fix(datastore): stop npm credential writes naming the interim token part 1/2 (S04-A per-format columns plan: 11/11)

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/s04a-per-format-credential-columns-step-11a into main

Step 11a of the S04-A per-format credential columns plan: npm's credential write statements stop naming tmp_plaintext_auth_token, one release ahead of 11b dropping the column.

npmRemoteCredentialAssignments and the bind struct behind it now render the encrypted unit's four columns and no interim one, in the credential arm and the clear alike. insertNpmRemoteRepositoryStmt already named encrypted_auth_token rather than the interim column at Step 10's head, so this un-binds one statement rather than two. npm has no credential store with a statement of its own, so those are the whole write set.

What it changes for an operator: a set, replace, clear, or URL change on a pre-move npm row now leaves whatever bearer token the row already holds standing. Re-entering the credential seals a unit beside it instead of draining the row, so the boot leftover-plaintext count stops falling through the API, and only operator SQL or the parent repository's reap removes a value. The key-rotation runbook gains npm's own UPDATE for that. No API response changes: has_credentials still derives from wrapped_dek IS NOT NULL.

392 reviewable LOC across 15 files, so no split justification is owed.

The stack

Order MR Step State
1 this MR 11a, the un-bind: npm's credential writes stop naming the column open
2 !2681 (merged) 11b, the drop: the column and its readers go Draft
— !2678 (merged) the Status-table row for both halves open

!2681 (merged) is blocked on this MR through GitLab's merge-request dependency, so it cannot merge first, and it targets this branch so GitLab retargets it to main when this merges.

Merge order: 11a, then a release, then 11b. 11b's drop needs this un-bind released first; a drop riding the same release as the un-bind answers an in-flight npm credential write with 42703.

Pre-merge gate, not yet met

The plan's Acceptance for 11a names two readings of npm's leftover-plaintext boot line, and the first is owed before this merges. It also says 11a carries "the same two owners" as Step 5a (docs/plans/2026-09-03-s04a-per-format-credential-columns.md:2993 at main e599932f2), whose roles Step 5a sets under What this opens, and who owns it (:1341, same basis):

  1. Before 11a merges: leftover_plaintext_count for format=npm read and cleared, which for staging is the same event as before it deploys. Owner: the engineer shipping 11a, this MR's author (@dmeshcharakou).
  2. Immediately before the production manifest upload: read and cleared again. Not before 11b. Owner: whoever plays that upload; the plan names the role, not a person, so neither does this line.

Only the owners are recorded here. The staging figure itself is not taken, and it stays a gate rather than a note: this MR does not merge until that reading is posted. The gate is tracked on #1219 (closed) (note), because taking it needs either a direct database read over npmRemoteLeftoverPlaintextPredicate's column (internal/datastore/npm_remote_credential_table.go:45 at 12ae76e23) or a boot line from a deployed release, and both are operator actions rather than review work.

The window is what makes the ordering matter: the API drain exists only until this merges. Afterwards the same rows need hand-run SQL across all 64 partitions, and 11b's drop gates on that count reaching zero. No CI job enforces either reading.

Deviations from the plan's Files entry

Two files the plan scopes out of 11a are edited here, both because the plan's research is stale at this head. Step MRs do not edit the plan, so both are recorded here and owed to the plan's standing table MR:

  1. internal/datastore/credential_table.md — the plan says it "is Maven's alone and stays out of 11a". On main it carries an npm claim this change falsifies, that npmRemoteCredentialAssignments binds tmp_plaintext_auth_token NULL in either arm. Leaving the file alone would have left that sentence false.
  2. docs/specs/S15-npm-remote.md — the plan assigns the S15 amendment to 11b, in two places. The "Credential clearing on URL change" criterion is falsified by this change, so it is corrected here; the rest of S15 is left to 11b, per the residual below.

Shared documents, and the merge order that follows

No semantic order follows from the shared documents. The moved-path nulling clause in docs/dev/column-encryption.md names no format at this head, so 11a removes nothing from it. What 11a adds to docs/runbooks/key-rotation.md is npm's own entry in the operator-SQL supplement beside the never-narrowed API-clear sentence; an addition orders against nothing. Maven's un-bind merged while this branch was open, so the passages naming which formats have un-bound now name all three.

A textual order does follow, and it is this branch's. git diff --name-only main...HEAD overlaps two open MRs, !2630 (merged) (Maven's drop, part 2/2, Draft) and !2662 (merged) (Container's drop, part 2/2, Draft). Re-derived with git merge-tree --write-tree, against origin/main at e599932f2 and this branch's HEAD at 12ae76e23:

MR vs origin/main vs this HEAD conflicting files
!2630 (merged) clean (0) CONFLICT (1) cmd/artifact-registry/wire_remote.go, docs/dev/column-encryption.md, docs/runbooks/key-rotation.md, internal/datastore/credential_table.md
!2662 (merged) clean (0) CONFLICT (1) docs/dev/column-encryption.md, docs/runbooks/key-rotation.md

This branch is clean against origin/main at the same basis. So neither sibling owes a rebase on its own account, both target main, and neither is stack lag: whichever of the three lands second resolves the conflict, and no pipeline reports the overlap. Coordinate the order with each MR's author, or rebase once they land. cmd/artifact-registry/wire_remote.go is in !2630 (merged)'s set because this branch now corrects npm's boot-report comment there.

Residual left standing

  • S15's other interim-column passages. S15 still calls the interim column the live credential store and still shows Authorization: Bearer <tmp_plaintext_auth_token> on the upstream request. Both were already stale once Step 10 moved npm's reads and writes; they belong to 11b's spec-amendment gate, and this MR corrects only the criterion its own change falsifies.
  • S13's npm and Container claims. S13-virtual-remote-foundation.md calls the interim columns the live credential store for Container and npm and says their reads and writes are not behind the framework. Stale since Steps 7 and 10, tracked under #417 (closed).
  • Container's copy of a rationale this un-bind class falsifies. container_remote_credentials_encryption_integration_test.go's leftover-count subtest still says the count has to fall "as rows are re-entered", which Container's own un-bind falsified before this MR; Maven's copy was reworded with Maven's. Left alone here rather than reaching into another format's suite, and the open Container drop half, !2662 (merged), is the natural place for it.
  • The drain runbook's trigger. interim-plaintext-credential-drain.md's When you need this and The gate fire on the release that moves a format's write path, which for npm was v2.52.0, not on the un-bind release this is. The count still has to be read before this merges; the plan's Acceptance is what says so, and the runbook's trigger does not yet name this release kind.

Spec coverage

Re-derived at 12ae76e23.

Criterion Where Test
Neither the credential arm nor the clear names the interim column npmRemoteCredentialAssignments TestNpmRemoteCredentialUnitBindsFor/neither_shape_names_the_interim_plaintext_column, rendered through the shipped builder
No rendered update statement names the column updateNpmRemoteRepositoryStmt TestUpdateNpmRemoteRepositoryStmt, 6 of its 8 cases via wantAbsent; the two window-only patches never reach the credential builder
A pre-move row keeps its token through every credential write update path TestNpmRemoteRepositoryStore_UpdateLeavesInterimPlaintext, 4 cases covering the URL change, the explicit clear, a set and a replace; each asserts the seeded token byte for byte, the url the patch leaves, and the unit nulled on the first two arms or populated and read back through the store on the last two
Re-entry no longer drains the drop gate's count boot report TestNpmRemoteRepositoryStore_LeftoverPlaintextCredentialsOverRealRows/a_credential_write_on_a_pre-move_row_leaves_the_count_where_it_was
InterimPlaintextWritePathMoved still matches the write path npm store TestNpmRemoteRepositoryStore_WritePathMovedMatchesTheWritePath, derived from a real create

Testing

No e2e scenario is added or affected. The change removes one column from a SET list on the management settings-update path; every observable response, status code and audit action is unchanged, so no scenario in docs/testing/ describes different behaviour after it. Conformance tests are not implicated either: no Maven, npm or Container/OCI protocol behaviour changes, only a management-API credential write's column list.

Database Review Evidence

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

What is measured, and why it is not a changed file. None of the three changed Go files holds a dispatch call whose own chain moved: npm_remote_credential_table.go's diff is a doc comment, cmd/artifact-registry/wire_remote.go's is the boot-report wiring comment, and every dispatching method in npm_remote_repositories.go is byte-identical to the merge base. What changed is npmRemoteCredentialAssignments, the SET-list builder, so the statement whose rendered SQL moves is the UPDATE updateNpmRemoteRepositoryStmt builds in the unchanged npm_remote_repositories_update.go.

npmRemoteUpdateAssignments has two switch arms that call the changed builder, the supplied-token arm and the clear arm. Crossed with the URL-change flag, which prepends url and appends the two health columns, those two arms render four distinct statements, and all four are below. A window-only patch (cache_validity_hours or metadata_cache_validity_hours alone) reaches neither arm and is not listed. The create's INSERT never bound tmp_plaintext_auth_token, so insertNpmRemoteRepositoryStmt renders exactly what it rendered before and is not listed either.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.updateNpmRemoteRepositoryStmt.AuthTokenSet Update n/a 0 / 0 1.75 0.328ms 19 / 2 1/64
datastore.updateNpmRemoteRepositoryStmt.AuthTokenSetAcrossURLChange Update n/a 0 / 0 1.75 0.064ms 21 / 0 1/64
datastore.updateNpmRemoteRepositoryStmt.CredentialClear Update n/a 0 / 0 1.75 0.079ms 16 / 0 1/64
datastore.updateNpmRemoteRepositoryStmt.URLChangedClear Update n/a 0 / 0 1.75 0.063ms 16 / 0 1/64

Two notes on reading the table. The root Update node returns no rows, so its plan/actual pair is 0 / 0 on all four arms; the driving scan estimates and finds exactly 1 row in every case. And the n/a index cell is the write-target seed size, not a missing index: at the 50 rows the recipe seeds for a write target, a Seq Scan of the single pruned partition costs less than the index. Re-seeded to 5000 rows in the target partition, on this same container, the statement plans as Index Scan using npm_remote_repositories_p48_pkey, with Index Cond: ((id = ...) AND (namespace_id = ...)) binding both predicate columns, Buffers: shared hit=3, and still 1 of 64 partitions scanned.

datastore.updateNpmRemoteRepositoryStmt.AuthTokenSet

Summary: Plan matches the method's intent. The namespace_id equality prunes to 1 of 64 partitions, and the row is found by the (namespace_id, id) pair the locked read resolved. The SET list carries exactly the four encrypted credential columns and no tmp_plaintext_auth_token, which is what this MR changes. Execution time is dominated by the three FK check triggers (2.621ms of 3.191ms), not by the statement. No anomalies.

Seed shape: namespaces=1, namespace_encryption_keys=1, repositories=50, npm_remote_repositories=50

Rendered SQL:

UPDATE public.npm_remote_repositories
SET wrapped_dek = $1::bytea,
    ns_key_id = $2::uuid,
    ns_key_version = $3::integer,
    encrypted_auth_token = $4::bytea
WHERE (npm_remote_repositories.namespace_id = $5::uuid) AND (npm_remote_repositories.id = $6::uuid);

Bound args: ['\xcdcd...cd' (32 bytes), 'dfd4dbbf-34a3-70b2-9158-6de9fe7ad889' (seeded namespace_encryption_keys.id), 1, '\xefef...ef' (48 bytes), '2ee843eb-75ef-7a7c-8561-2d29ea6623fe', '3a93b67b-761c-74a2-b51a-93156b24b37d']

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Update on npm_remote_repositories  (cost=0.00..1.75 rows=0 width=0) (actual time=0.328..0.328 rows=0 loops=1)
  Update on npm_remote_repositories_p46 npm_remote_repositories_1
  Buffers: shared hit=19 read=2
  ->  Seq Scan on npm_remote_repositories_p46 npm_remote_repositories_1  (cost=0.00..1.75 rows=1 width=94) (actual time=0.008..0.010 rows=1 loops=1)
        Filter: ((namespace_id = '2ee843eb-75ef-7a7c-8561-2d29ea6623fe'::uuid) AND (id = '3a93b67b-761c-74a2-b51a-93156b24b37d'::uuid))
        Rows Removed by Filter: 49
        Buffers: shared hit=1
Planning:
  Buffers: shared hit=265
Planning Time: 0.550 ms
Trigger for constraint fk_npm_remote_repositories_namespace_id_namespaces on npm_remote_repositories_p46: time=0.853 calls=1
Trigger for constraint fk_npm_remote_repositories_repository_id_repositories on npm_remote_repositories_p46: time=1.081 calls=1
Trigger for constraint fk_npm_remote_repositories_ns_key_id_ns_enc_keys on npm_remote_repositories_p46: time=0.687 calls=1
Execution Time: 3.191 ms

Timings: planning 0.550ms, execution 3.191ms, total 3.741ms.

datastore.updateNpmRemoteRepositoryStmt.AuthTokenSetAcrossURLChange

Summary: Plan matches the method's intent, and this is the widest statement the path can render: the re-point, the four credential columns with real values, and the two health-reset columns in one UPDATE, which is what keeps the re-point and the credential write in one transaction. It plans identically to the narrow set arm, pruning to 1 of 64 partitions and finding one row by (namespace_id, id). Eight columns set, none of them the interim column. Execution time is dominated by the three FK check triggers (4.405ms of 4.568ms). No anomalies.

Seed shape: namespaces=1, namespace_encryption_keys=1, repositories=50, npm_remote_repositories=50

Rendered SQL:

UPDATE public.npm_remote_repositories
SET url = $1::text,
    wrapped_dek = $2::bytea,
    ns_key_id = $3::uuid,
    ns_key_version = $4::integer,
    encrypted_auth_token = $5::bytea,
    last_health_status = $6,
    last_health_checked_at = NULL::timestamp with time zone
WHERE (npm_remote_repositories.namespace_id = $7::uuid) AND (npm_remote_repositories.id = $8::uuid);

Bound args: ['https://registry.yarnpkg.com/', '\xcdcd...cd' (32 bytes), '0eb5f50a-c4c7-7799-a8f4-252e54a2a276' (seeded namespace_encryption_keys.id), 1, '\xefef...ef' (48 bytes), 0, 'e493873e-f115-73ce-9c64-3fac06fce5d0', '90791984-9d5c-71ab-949a-a7765dd14d78']

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Update on npm_remote_repositories  (cost=0.00..1.75 rows=0 width=0) (actual time=0.064..0.064 rows=0 loops=1)
  Update on npm_remote_repositories_p44 npm_remote_repositories_1
  Buffers: shared hit=21
  ->  Seq Scan on npm_remote_repositories_p44 npm_remote_repositories_1  (cost=0.00..1.75 rows=1 width=136) (actual time=0.003..0.004 rows=1 loops=1)
        Filter: ((namespace_id = 'e493873e-f115-73ce-9c64-3fac06fce5d0'::uuid) AND (id = '90791984-9d5c-71ab-949a-a7765dd14d78'::uuid))
        Rows Removed by Filter: 49
        Buffers: shared hit=1
Planning:
  Buffers: shared hit=65
Planning Time: 0.196 ms
Trigger for constraint fk_npm_remote_repositories_namespace_id_namespaces on npm_remote_repositories_p44: time=0.009 calls=1
Trigger for constraint fk_npm_remote_repositories_repository_id_repositories on npm_remote_repositories_p44: time=4.071 calls=1
Trigger for constraint fk_npm_remote_repositories_ns_key_id_ns_enc_keys on npm_remote_repositories_p44: time=0.325 calls=1
Execution Time: 4.568 ms

Timings: planning 0.196ms, execution 4.568ms, total 4.764ms.

datastore.updateNpmRemoteRepositoryStmt.CredentialClear

Summary: Plan matches the method's intent, same shape as the set arms: prune to 1 of 64 partitions, then one row by (namespace_id, id). All four credential columns are set NULL, which is the check_npm_remote_repositories_enc_credentials_all_or_none constraint's other legal shape, and the interim column is absent from the SET list. Only two FK triggers fire here rather than three, because setting ns_key_id NULL leaves nothing for the encryption-key FK to check; they account for 4.454ms of the 4.642ms execution time. No anomalies.

Seed shape: namespaces=1, namespace_encryption_keys=1, repositories=50, npm_remote_repositories=50

Rendered SQL:

UPDATE public.npm_remote_repositories
SET wrapped_dek = NULL::bytea,
    ns_key_id = NULL::uuid,
    ns_key_version = NULL::integer,
    encrypted_auth_token = NULL::bytea
WHERE (npm_remote_repositories.namespace_id = $1::uuid) AND (npm_remote_repositories.id = $2::uuid);

Bound args: ['45e0033d-e53d-7fdd-ab6c-22e25fb830d0', '18c49e1c-e0fb-75d5-a086-d0028dc0a1d6']

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Update on npm_remote_repositories  (cost=0.00..1.75 rows=0 width=0) (actual time=0.079..0.079 rows=0 loops=1)
  Update on npm_remote_repositories_p60 npm_remote_repositories_1
  Buffers: shared hit=16
  ->  Seq Scan on npm_remote_repositories_p60 npm_remote_repositories_1  (cost=0.00..1.75 rows=1 width=94) (actual time=0.004..0.005 rows=1 loops=1)
        Filter: ((namespace_id = '45e0033d-e53d-7fdd-ab6c-22e25fb830d0'::uuid) AND (id = '18c49e1c-e0fb-75d5-a086-d0028dc0a1d6'::uuid))
        Rows Removed by Filter: 49
        Buffers: shared hit=1
Planning:
  Buffers: shared hit=66
Planning Time: 0.245 ms
Trigger for constraint fk_npm_remote_repositories_namespace_id_namespaces on npm_remote_repositories_p60: time=0.015 calls=1
Trigger for constraint fk_npm_remote_repositories_repository_id_repositories on npm_remote_repositories_p60: time=4.439 calls=1
Execution Time: 4.642 ms

Timings: planning 0.245ms, execution 4.642ms, total 4.887ms.

datastore.updateNpmRemoteRepositoryStmt.URLChangedClear

Summary: Plan matches the method's intent. This is the URL re-point with no token supplied, which clears the credential and resets the two health columns in the same statement, and it plans like the other three: prune to 1 of 64 partitions, one row by (namespace_id, id). Seven columns set, none of them the interim column. Two FK triggers, for the same reason as the plain clear, accounting for 4.008ms of the 4.161ms execution time. No anomalies.

Seed shape: namespaces=1, namespace_encryption_keys=1, repositories=50, npm_remote_repositories=50

Rendered SQL:

UPDATE public.npm_remote_repositories
SET url = $1::text,
    wrapped_dek = NULL::bytea,
    ns_key_id = NULL::uuid,
    ns_key_version = NULL::integer,
    encrypted_auth_token = NULL::bytea,
    last_health_status = $2,
    last_health_checked_at = NULL::timestamp with time zone
WHERE (npm_remote_repositories.namespace_id = $3::uuid) AND (npm_remote_repositories.id = $4::uuid);

Bound args: ['https://registry.yarnpkg.com/', 0, 'fd19ada3-1b5a-75fc-8f35-c00e065b41f9', '4c7c108c-e8fb-73fb-ada0-628c2f503f32']

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Update on npm_remote_repositories  (cost=0.00..1.75 rows=0 width=0) (actual time=0.063..0.063 rows=0 loops=1)
  Update on npm_remote_repositories_p38 npm_remote_repositories_1
  Buffers: shared hit=16
  ->  Seq Scan on npm_remote_repositories_p38 npm_remote_repositories_1  (cost=0.00..1.75 rows=1 width=136) (actual time=0.003..0.004 rows=1 loops=1)
        Filter: ((namespace_id = 'fd19ada3-1b5a-75fc-8f35-c00e065b41f9'::uuid) AND (id = '4c7c108c-e8fb-73fb-ada0-628c2f503f32'::uuid))
        Rows Removed by Filter: 49
        Buffers: shared hit=1
Planning:
  Buffers: shared hit=65
Planning Time: 0.244 ms
Trigger for constraint fk_npm_remote_repositories_namespace_id_namespaces on npm_remote_repositories_p38: time=0.013 calls=1
Trigger for constraint fk_npm_remote_repositories_repository_id_repositories on npm_remote_repositories_p38: time=3.995 calls=1
Execution Time: 4.161 ms

Timings: planning 0.244ms, execution 4.161ms, total 4.405ms.

No migration files changed, so migration mode did not run.

Related to #1219 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports