feat(npm): npm_remote_packages cache-fill writes (S15 Step 6, part 1/5)

David Fernandezrequested to merge
10io/npm-remote-step-6-part1of5 into main

📦 What this MR does

Part 1 of 5 of S15 Step 6 (datastore remote write — cache-fill): the npm_remote_packages write path.

  • UpsertNpmRemotePackage — the idempotent lazy upsert (INSERT ... ON CONFLICT DO NOTHING with a same-handle conflict read-back, per S15 Name resolution). It runs inside the cache-fill transaction Step 9's callback owns, so the method takes a jet-compatible qrm.DB handle rather than opening its own. The conflict target is the partial unique index, so a soft-deleted row never conflicts: re-caching inserts a fresh active row next to the tombstone.
  • ErrParentNpmRemoteRepositoryMissing — an FK violation on the parent remote repository maps to an unwrapped, constraint-name-matched sentinel, so a ghost namespace is not misclassified and PgError.Detail cannot leak key columns into logs or responses.
  • BumpLastDownloadedAt — the buffered retention stamp: pool-executed, soft-delete gated, and a zero-row match is a benign no-op (the counter is lossy by design).
  • The shared test scaffolding the stack above reuses: the write-suite namespace sweep and row readers (integration). The unit guards use the package's existing recordingDB harness, so a guard that fails to fire shows up as a recorded DB round-trip rather than a panic.

⚙️ Why stacked

Step 6's full diff is ~3.4k reviewable LoC, so it ships as 5 stacked MRs to keep each within the review size limit (≈600 ideal), each part targeting the previous (part 1 → main), reviewed and merged bottom-up. This part is the bottom: packages here, versions in part 2, the two blob repoint upserts in parts 3–4, and the 304 revalidation bumps in part 5.

🧪 Spec coverage

Spec: docs/specs/S15-npm-remote.md

This part is the npm_remote_packages slice of the datastore write layer. S15's acceptance criteria are stated at the HTTP boundary, so most are owned by later parts and steps; the rows below are the ones this MR's code can satisfy. AC numbering follows the spec's own per-subsection numbering.

Acceptance criteria

# Criterion Tests
Design — Name resolution npm_remote_packages rows are created lazily inside the transaction that commits the first successful cache fill, never through an auto-create-on-push path; a package with no cached row is a Miss, not an error TestNpmRemotePackageStore_UpsertNpmRemotePackage: inserts a new package and returns its id, returns the existing row id on conflict (idempotent upsert) (both driven through a caller-owned tx via beginTx, so the row commits with the caller's transaction; the conflict subtest re-upserts under a different scope and asserts the stored row is unmodified, so DO NOTHING cannot drift into DO UPDATE)
Data model — npm_remote_packages The lazy upsert writes the canonical @scope/name and the nullable scope segment with its sigil stores the scope with the @ sigil, inserts a new package and returns its id
Data model — partial unique index The WHERE soft_deleted_at IS NULL arbiter allows re-caching after soft deletion: the upsert inserts a fresh active row rather than resurrecting or returning the tombstone a soft-deleted row does not conflict: re-caching inserts a fresh active row (asserts the fresh id differs and both rows coexist)
Tarball proxy AC-1 (Cache miss, fill) The cache fill writes the npm_remote_packages parent of npm_remote_files; this MR supplies that parent write, streaming and the child rows are owned by parts 3–5 and Steps 10–12 TestNpmRemotePackageStore_UpsertNpmRemotePackage (all subtests)
Tarball proxy AC-2 (Fresh cache hit bumps last_downloaded_at) The package-side retention stamp is settable and soft-delete gated; the GET-versus-HEAD gating that decides when it fires is owned by the tarball handler (Step 11) TestNpmRemotePackageStore_BumpLastDownloadedAt: sets last_downloaded_at on an active package, is a no-op on a soft-deleted package, is a no-op on a missing package
Packument proxy, dist-tags, Write unavailability, Credentials and health, Error mapping Not in this MR — HTTP-boundary behavior owned by parts 2–5 and Steps 9–12

Error cases

# Condition Tests
Missing parent remote repository An FK violation on npm_remote_repositories surfaces the unwrapped, constraint-name-matched ErrParentNpmRemoteRepositoryMissing so callers can branch on it returns ErrParentNpmRemoteRepositoryMissing for a missing parent repository
Ghost namespace not misclassified A valid-but-nonexistent namespace violates both the namespaces FK and the composite repository FK; the namespaces FK fires first, so the error takes the generic wrap and does not classify as the repository sentinel ghost namespace is not misclassified as ErrParentNpmRemoteRepositoryMissing (asserts the wrap prefix, that errors.As still reaches the *pgconn.PgError, SQLSTATE 23503, and ConstraintName == fk_npm_remote_packages_namespace_id_namespaces)
Transient DB failure on the INSERT An error that is neither the repository FK nor qrm.ErrNoRows is %w-wrapped with the INSERT context and never surfaces as the parent sentinel TestNpmRemotePackageStore_UpsertNpmRemotePackage_WrapsTransientError: non-FK pgError (unique violation), non-pgconn error (network)
Conflict read-back When ON CONFLICT DO NOTHING suppresses the RETURNING, the read-back's own failures keep their own context prefix: no visible row maps to ErrNotFound, a transient failure is %w-wrapped, and neither surfaces as the parent sentinel TestNpmRemotePackageStore_UpsertNpmRemotePackage_WrapsConflictReadBackError: read-back sees no row (concurrent uncommitted insert), read-back fails transiently (a sequencedDB scripts a distinct error per round trip)
Transient DB failure on the bump A driver failure on the UPDATE is %w-wrapped with the bump context TestNpmRemotePackageStore_BumpLastDownloadedAt_WrapsTransientError
Lossy retention counter A missing or soft-deleted package is a zero-row no-op rather than an error, because the bump runs off the request path through the format layer's buffered wrapper is a no-op on a soft-deleted package, is a no-op on a missing package
Caller wiring mistakes Nil context, nil qrm.DB, zero-UUID namespace, zero-UUID repository id, zero-UUID package id, and an empty package name each reject with this store's own sentinel before any DB dispatch TestNpmRemotePackageStore_UpsertNpmRemotePackage_ArgumentGuards and TestNpmRemotePackageStore_BumpLastDownloadedAt_ArgumentGuards; the nil-qrm.DB case is a row in each table, carried by the per-case db field
HTTP error envelope (405 unsupported, 404, 503 upstream_unavailable, upstream 5xx, file_name_invalid) Not in this MR — no reachable request path until Steps 10–12

Security considerations

# Concern Tests
Error-payload hygiene ErrParentNpmRemoteRepositoryMissing is returned bare rather than %w-wrapped around the pgconn error, so PgError.Detail/Where — which echo the violated FK's key columns (the npm_remote_repository_id and the caller-supplied namespace UUID) — cannot reach logs or HTTP responses through Error() returns ErrParentNpmRemoteRepositoryMissing for a missing parent repository asserts the sentinel and, positively, that errors.As finds no *pgconn.PgError on that path; the complement (the generic path does preserve the PgError for diagnosability) is pinned by ghost namespace is not misclassified as ErrParentNpmRemoteRepositoryMissing
Namespace isolation (ADR-001) Every statement carries namespace_id explicitly — as the partition key, in the conflict arbiter, and in the bump predicate — so no write or read-back can cross a namespace boundary does not bump a package owned by another namespace bumps nsA's package id under nsB and asserts nsA's stamp stays NULL, so the NamespaceID.EQ predicate is not deletable; rejects another namespace's remote repository (composite FK) pins the composite parent FK with nsA's real repository id under nsB. The Database Review Evidence plans below show single-partition pruning on each statement
Credential hygiene, credentials at rest, SSRF, cross-origin redirect token stripping, outbound path-segment safety, tarball integrity Not in this MR — this layer holds no credential, issues no upstream request, and builds no URL

🔗 References

  • Plan: docs/plans/2026-07-15-npm-remote.md — Step 6
  • Spec: docs/specs/S15-npm-remote.md — Name resolution, Data model

🔬 e2e scenarios

No scenario added or affected: datastore write layer with no reachable request path until the proxy handlers (Steps 10–12) wire the cache fill end-to-end.

Related to #343 (closed)

📚 Stacked MRs (review/merge bottom-up)

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.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.NpmRemotePackageStore.BumpLastDownloadedAt Update npm_remote_packages_p60_pkey (n/a at 50 rows; see details) 1 / 1 1.75 2.546ms 20 / 2 1
datastore.UpsertNpmRemotePackage Insert unique_npm_remote_packages_ns_id_repo_id_name (conflict arbiter) 1 / 1 0.01 2.184ms 56 / 3 1
datastore.UpsertNpmRemotePackage.ConflictReadBack Limit → Index Scan npm_remote_packages_p39_namespace_id_npm_remote_repository__idx 1 / 1 8.30 0.018ms 3 / 0 1
datastore.UpsertNpmRemotePackage.ScopeSet Insert unique_npm_remote_packages_ns_id_repo_id_name (conflict arbiter) 1 / 1 0.01 0.691ms 45 / 0 1
datastore.NpmRemotePackageStore.BumpLastDownloadedAt

Summary: Plan matches the method's intent — a single-row targeted UPDATE, pruned to one of 64 partitions by the namespace_id literal, with soft_deleted_at IS NULL applied as a filter so a tombstoned package is a zero-row no-op. At the skill's UPDATE seed size (1 target + 49 siblings) the planner picks a Seq Scan over the 50-row partition, which is the cheaper path at that cardinality rather than a missing index: re-running the identical statement with 5000 rows in the partition switches to Index Scan using npm_remote_packages_p63_pkey (Index Cond: id = ... AND namespace_id = ..., 0.342ms execution, Buffers: shared hit=34). No anomalies.

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

Rendered SQL:

UPDATE public.npm_remote_packages
SET last_downloaded_at = NOW()
WHERE ((npm_remote_packages.namespace_id = $1::uuid) AND (npm_remote_packages.id = $2::uuid)) AND (npm_remote_packages.soft_deleted_at IS NULL);

Bound args: [cb84ff84-306c-4864-9640-fe43b4aeeece, 917689ef-9e52-4883-a77b-745a3f96090c]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_remote_packages  (cost=0.00..1.75 rows=0 width=0) (actual time=0.158..0.158 rows=0 loops=1)
   Update on npm_remote_packages_p60 npm_remote_packages_1
   Buffers: shared hit=20 read=2
   ->  Seq Scan on npm_remote_packages_p60 npm_remote_packages_1  (cost=0.00..1.75 rows=1 width=18) (actual time=0.005..0.007 rows=1 loops=1)
         Filter: ((soft_deleted_at IS NULL) AND (namespace_id = 'cb84ff84-306c-4864-9640-fe43b4aeeece'::uuid) AND (id = '917689ef-9e52-4883-a77b-745a3f96090c'::uuid))
         Rows Removed by Filter: 49
         Buffers: shared hit=1
 Planning:
   Buffers: shared hit=116
 Planning Time: 0.590 ms
 Trigger for constraint fk_npm_remote_packages_namespace_id_namespaces on npm_remote_packages_p60: time=0.031 calls=1
 Trigger for constraint fk_npm_remote_packages_npm_remote_repository_id on npm_remote_packages_p60: time=2.170 calls=1
 Execution Time: 2.546 ms

Same statement, 5000 rows in the partition (index-path confirmation):

 Update on npm_remote_packages  (cost=0.28..8.30 rows=0 width=0) (actual time=0.121..0.121 rows=0 loops=1)
   Update on npm_remote_packages_p63 npm_remote_packages_1
   Buffers: shared hit=34
   ->  Index Scan using npm_remote_packages_p63_pkey on npm_remote_packages_p63 npm_remote_packages_1  (cost=0.28..8.30 rows=1 width=18) (actual time=0.008..0.008 rows=1 loops=1)
         Index Cond: ((id = '2f14ef70-d3d8-4d45-b49a-54733e18e016'::uuid) AND (namespace_id = '56e92683-0ac3-44ed-83aa-560ec26ea88d'::uuid))
         Filter: (soft_deleted_at IS NULL)
         Buffers: shared hit=3
 Planning Time: 0.465 ms
 Execution Time: 0.342 ms

Timings: planning 0.590ms, execution 2.546ms, total 3.136ms. Execution is dominated by the two FK-validation triggers (2.170ms + 0.031ms) on a cold seeded table, not by the row lookup.

datastore.UpsertNpmRemotePackage

Summary: Plan matches the method's intent: the arbiter Postgres selected is unique_npm_remote_packages_ns_id_repo_id_name — the partial index carrying WHERE soft_deleted_at IS NULL — which is what makes a soft-deleted row not conflict and re-caching insert a fresh active row. The namespace_id value routes the insert to exactly one of 64 partitions (npm_remote_packages_p32). On the steady-state path (the row already cached, which the method treats as the expected outcome rather than an error) the same statement reports Tuples Inserted: 0, Conflicting Tuples: 1 and skips both FK triggers, finishing in 0.251ms at 5000 rows in the partition. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_remote_repositories=1, npm_remote_packages=0

Rendered SQL:

INSERT INTO public.npm_remote_packages (id, namespace_id, npm_remote_repository_id, name, scope)
VALUES ($1::uuid, $2::uuid, $3::uuid, $4::text, NULL)
ON CONFLICT (namespace_id, npm_remote_repository_id, name) WHERE soft_deleted_at IS NULL DO NOTHING
RETURNING npm_remote_packages.id AS "npm_remote_packages.id";

Bound args: [<gen_random_uuid()>, 69f9d2ac-25c3-48d6-897a-22995ed6d9b9, 1b8858ec-90f6-4c42-940a-ac102f36ab34, lodash]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Insert on npm_remote_packages  (cost=0.00..0.01 rows=1 width=128) (actual time=0.456..0.457 rows=1 loops=1)
   Conflict Resolution: NOTHING
   Conflict Arbiter Indexes: unique_npm_remote_packages_ns_id_repo_id_name
   Tuples Inserted: 1
   Conflicting Tuples: 0
   Buffers: shared hit=56 read=3 dirtied=7 written=4
   ->  Result  (cost=0.00..0.01 rows=1 width=128) (actual time=0.001..0.001 rows=1 loops=1)
 Planning:
   Buffers: shared hit=53
 Planning Time: 0.198 ms
 Trigger for constraint fk_npm_remote_packages_namespace_id_namespaces on npm_remote_packages_p32: time=0.544 calls=1
 Trigger for constraint fk_npm_remote_packages_npm_remote_repository_id on npm_remote_packages_p32: time=0.749 calls=1
 Execution Time: 2.184 ms

Same statement on the conflict-hit (steady-state) path, 5000 rows in the partition:

 Insert on npm_remote_packages  (cost=0.00..0.01 rows=1 width=128) (actual time=0.232..0.233 rows=0 loops=1)
   Conflict Resolution: NOTHING
   Conflict Arbiter Indexes: unique_npm_remote_packages_ns_id_repo_id_name
   Tuples Inserted: 0
   Conflicting Tuples: 1
   Buffers: shared hit=78
   ->  Result  (cost=0.00..0.01 rows=1 width=128) (actual time=0.001..0.001 rows=1 loops=1)
 Planning Time: 0.197 ms
 Execution Time: 0.251 ms

Timings: planning 0.198ms, execution 2.184ms, total 2.382ms. As with the bump, execution is mostly the two FK triggers (1.293ms combined) firing on the first insert into a cold partition.

datastore.UpsertNpmRemotePackage.ConflictReadBack

Summary: Plan matches the method's intent: on a conflict the read-back resolves the existing row through Limit → Index Scan over the composite (namespace_id, npm_remote_repository_id, name) index, with all three equality predicates pushed into Index Cond and the namespace_id literal pruning to one of 64 partitions. Estimate matches reality exactly (1 / 1) and execution is 0.018ms against 5000 rows in the partition, reading 3 shared buffers with no disk reads. The statement is the same builder the resolution read uses (findNpmRemotePackageByNameStmt), so it inherits that read's existing partition-pruning test. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_remote_repositories=1, npm_remote_packages=5000

Rendered SQL:

SELECT npm_remote_packages.id AS "npm_remote_packages.id"
FROM public.npm_remote_packages
WHERE (((npm_remote_packages.namespace_id = $1::uuid) AND (npm_remote_packages.npm_remote_repository_id = $2::uuid)) AND (npm_remote_packages.name = $3::text)) AND (npm_remote_packages.soft_deleted_at IS NULL)
LIMIT $4;

Bound args: [69f9d2ac-25c3-48d6-897a-22995ed6d9b9, 1b8858ec-90f6-4c42-940a-ac102f36ab34, review-prep-pkg-002500, 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.30 rows=1 width=16) (actual time=0.010..0.010 rows=1 loops=1)
   Buffers: shared hit=3
   ->  Index Scan using npm_remote_packages_p39_namespace_id_npm_remote_repository__idx on npm_remote_packages_p39 npm_remote_packages  (cost=0.28..8.30 rows=1 width=16) (actual time=0.009..0.009 rows=1 loops=1)
         Index Cond: ((namespace_id = '69f9d2ac-25c3-48d6-897a-22995ed6d9b9'::uuid) AND (npm_remote_repository_id = '1b8858ec-90f6-4c42-940a-ac102f36ab34'::uuid) AND (name = 'review-prep-pkg-002500'::text))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=133
 Planning Time: 0.494 ms
 Execution Time: 0.018 ms

Timings: planning 0.494ms, execution 0.018ms, total 0.512ms.

datastore.UpsertNpmRemotePackage.ScopeSet

Summary: The scoped-package branch of the same upsert — nullableStringExpr renders the scope column as $5::text instead of the inline NULL the unscoped branch produces. Plan shape is identical to the unscoped branch (same conflict arbiter, same single-partition routing, same Result source node), confirming the nullable-scope rendering does not change how the statement executes. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_remote_repositories=1, npm_remote_packages=0

Rendered SQL:

INSERT INTO public.npm_remote_packages (id, namespace_id, npm_remote_repository_id, name, scope)
VALUES ($1::uuid, $2::uuid, $3::uuid, $4::text, $5::text)
ON CONFLICT (namespace_id, npm_remote_repository_id, name) WHERE soft_deleted_at IS NULL DO NOTHING
RETURNING npm_remote_packages.id AS "npm_remote_packages.id";

Bound args: [<gen_random_uuid()>, <namespace uuid>, <npm_remote_repository uuid>, @gitlab-org/foo, @gitlab-org]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Insert on npm_remote_packages  (cost=0.00..0.01 rows=1 width=128) (actual time=0.175..0.175 rows=1 loops=1)
   Conflict Resolution: NOTHING
   Conflict Arbiter Indexes: unique_npm_remote_packages_ns_id_repo_id_name
   Tuples Inserted: 1
   Conflicting Tuples: 0
   Buffers: shared hit=45
   ->  Result  (cost=0.00..0.01 rows=1 width=128) (actual time=0.001..0.001 rows=1 loops=1)
 Planning:
   Buffers: shared hit=53
 Planning Time: 0.179 ms
 Trigger for constraint fk_npm_remote_packages_namespace_id_namespaces on npm_remote_packages_p32: time=0.128 calls=1
 Trigger for constraint fk_npm_remote_packages_npm_remote_repository_id on npm_remote_packages_p32: time=0.288 calls=1
 Execution Time: 0.691 ms

Timings: planning 0.179ms, execution 0.691ms, total 0.870ms.

Edited by David Fernandez

Merge request reports