feat(npm): remote package and version resolvers (S15 Step 5, part 2/4)

David Fernandezrequested to merge
10io/s15-npm-remote-step-5-part2of4 into main

📚 Stacked MRs

S15 Step 5 — the read side of the npm-remote datastore layer — is split into 4 stacked MRs to keep each within the review size target (~600 reviewable LoC). Each part targets the previous one (part 1 targets main); review and merge proceed bottom-up. The four parts together supersede the change originally proposed in !1050 (closed), and have since taken review feedback on top of it.

Stacked MRs (review/merge bottom-up)

📦 What this adds (part 2 of 4)

The package and version resolvers: NpmRemotePackageByName (full @scope/name) and NpmRemoteVersionByPackageAndVersion resolve a cached package or version to its binding id so the metadata/tarball lookups can key on it. Each returns a named resolution type (NpmRemotePackageResolution, NpmRemoteVersionResolution) carrying just that id, per architecture.md's "return domain types, not database rows" and part 1's NpmRemoteRepositoryResolution; the jet row stays inside the store. Each gates its own soft_deleted_at; a not-yet-cached row is a cache Miss (ErrNotFound), never a distinct error. Also marks the hosted package guard table nolint:dupl for the intentionally-parallel remote sibling added here.

🧪 Spec coverage

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

This part is the datastore read layer for the cached package and version entities. 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
Name resolution — cache Miss "A read against a package with no cached row yet is a cache Miss, not a distinct error": an uncached package or version resolves as ErrNotFound, never a distinct error TestNpmRemotePackageStore_NpmRemotePackageByName: …when no package matches the name (cache miss); TestNpmRemoteVersionStore_NpmRemoteVersionByPackageAndVersion: …when no version matches (cache miss)
Name resolution — scoped packages A scoped package resolves by its full @scope/name form, the name predicate carrying the @scope/ prefix resolves a scoped package by its full @scope/name form
Name resolution — case-sensitive Names and versions are matched verbatim; the service does not normalize case at lookup time (S11 validation rules, reused unchanged) name lookup is case-sensitive, version lookup is case-sensitive
Name resolution — namespace-scoped (ADR-001) A cached package or version in one namespace never resolves under another namespace's id does not resolve a package from a different namespace, does not resolve a version from a different namespace
Name resolution — repository- and package-scoped Two remote repositories in one namespace may both proxy lodash, and two packages may both carry 4.17.21; each lookup is keyed by its parent binding id does not return a package from a different repository in the same namespace, does not match a version under a different package
Data model — binding-id resolution Each read resolves to its binding surrogate (npm_remote_packages.id, npm_remote_versions.id), the id the child metadata-file and tarball-file lookups key on resolves an unscoped package to its binding id, resolves a cached version to its binding id (whole-struct cmp.Diff against the resolution, so a field added to it fails until asserted)
Data model — partial unique indexes The partial condition "allows re-caching after soft deletion", so an active row coexisting with a soft-deleted one at the same key resolves to the active row resolves to the active row when a soft-deleted row shares its key (both stores)
Data model — hash partitioning namespace_id is pinned so each read prunes to 1 of 64 partitions on the hot packument and tarball paths TestNpmRemotePackageStore_NpmRemotePackageByName_PrunesToOnePartition, TestNpmRemoteVersionStore_NpmRemoteVersionByPackageAndVersion_PrunesToOnePartition (EXPLAIN over the store's own statement builder)
Packument proxy, dist-tags, Tarball proxy, Write unavailability, Credentials and health, Error mapping Not in this MR — HTTP-boundary behavior owned by parts 3–4 and later steps

Error cases

# Condition Tests
Cache miss vs. soft-deleted A soft-deleted package or version resolves as ErrNotFound, indistinguishable from never-cached, so a soft-deleted row is never served returns ErrNotFound for a soft-deleted package, returns ErrNotFound for a soft-deleted version
Transient DB failure A non-qrm.ErrNoRows DB error surfaces as itself, never as ErrNotFound, so a transient failure does not drive a spurious upstream fetch a transient DB failure is not misreported as ErrNotFound (both stores)
Caller wiring mistakes Nil context, zero-UUID namespace, zero-UUID parent id, and empty name/version each reject with the store's own sentinel before any DB access; a nil client panics at construction TestNpmRemotePackageStore_NpmRemotePackageByName_ArgumentGuards, TestNpmRemoteVersionStore_NpmRemoteVersionByPackageAndVersion_ArgumentGuards, TestNewNpmRemotePackageStore_NilClientPanics, TestNewNpmRemoteVersionStore_NilClientPanics
Parent repository soft-deleted Gated upstream by FindRemoteByNameInNamespace (part 1), the only finder producing an npmRemoteRepositoryID; recorded in both doc comments, with the caller obligation not to synthesize the id Part 1: …ErrNotFound when the parent repository is soft-deleted
HTTP error envelope (405 unsupported, 404, 503 upstream_unavailable, upstream 5xx, Retry-After) Not in this MR — owned by parts 3–4 and later steps

Security considerations

# Concern Tests
Tenant isolation (ADR-001) A parent binding id alone is never sufficient to read a cached row from another namespace; namespace_id is the leading predicate on both reads does not resolve a package from a different namespace, does not resolve a version from a different namespace
Cross-repository cache leakage A cached packument from one remote repository is never served for another repository in the same namespace does not return a package from a different repository in the same namespace
Credential hygiene, Credentials at rest Not in this MR — the credential lives on npm_remote_repositories (part 1) and is never read here
SSRF, cross-origin redirect token stripping, outbound path-segment safety, error-payload hygiene Not in this MR — this layer issues no upstream request and builds no URL

🔗 References

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.

Plans, timings and row widths were captured before eedca285 narrowed both SELECT lists to the single id column each resolution carries. The WHERE clauses are byte-identical, so partition pruning and index choice are unaffected — id is not a key column of either partial unique index, so no index-only scan becomes available — and dropping projected columns can only reduce width and cost. Treat the recorded numbers as an upper bound rather than a different plan shape. The rendered SQL in each row's details is the current builder output.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.NpmRemotePackageStore.NpmRemotePackageByName Limit → Index Scan npm_remote_packages_p54_namespace_id_npm_remote_repository__idx 1 / 1 8.30 0.011ms 3 / 0 1 of 64
datastore.NpmRemoteVersionStore.NpmRemoteVersionByPackageAndVersion Limit → Index Scan npm_remote_versions_p02_namespace_id_npm_remote_package_id__idx 1 / 1 8.30 0.011ms 3 / 0 1 of 64
datastore.NpmRemotePackageStore.NpmRemotePackageByName

Summary: The plan matches the method's intent. The driving scan is an Index Scan over the partition-local child of the partial unique index unique_npm_remote_packages_ns_id_repo_id_name — (namespace_id, npm_remote_repository_id, name) WHERE soft_deleted_at IS NULL — whose Index Cond absorbs all three equality predicates while the index's own partial predicate absorbs the soft_deleted_at IS NULL gate, so the plan carries no Filter and no Sort and the LIMIT never has to discard a row. Passing namespace_id prunes to exactly one of the table's 64 hash partitions (npm_remote_packages_p54), the estimate matches reality exactly (1 / 1), and all 3 execution-time buffer accesses are cache hits with no reads. No anomalies against the flagged criteria, with the same caveat recorded in part 1 of this stack: planning dominates execution by 32x (0.670ms against 0.021ms) and planning touches shared hit=279 opening 64 partitions' catalog entries before pruning discards 63 of them, so the realistic per-request cost is the 0.691ms total rather than the 0.011ms in the Time column above. That is still well inside the 100 ms budget and warrants no code change.

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: ['ffc3675e-a4cd-4123-92d3-112690b3ba00', '488c3743-24a1-4262-b1e6-996f8837d7c0', '@review-prep/pkg-002500', 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.30 rows=1 width=101) (actual time=0.010..0.011 rows=1 loops=1)
   Buffers: shared hit=3
   ->  Index Scan using npm_remote_packages_p54_namespace_id_npm_remote_repository__idx on npm_remote_packages_p54 npm_remote_packages  (cost=0.28..8.30 rows=1 width=101) (actual time=0.010..0.010 rows=1 loops=1)
         Index Cond: ((namespace_id = 'ffc3675e-a4cd-4123-92d3-112690b3ba00'::uuid) AND (npm_remote_repository_id = '488c3743-24a1-4262-b1e6-996f8837d7c0'::uuid) AND (name = '@review-prep/pkg-002500'::text))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=279
 Planning Time: 0.670 ms
 Execution Time: 0.021 ms

Timings: planning 0.670ms, execution 0.021ms, total 0.691ms.

datastore.NpmRemoteVersionStore.NpmRemoteVersionByPackageAndVersion

Summary: The plan matches the method's intent and is the same shape as its package-resolver sibling above. The driving scan is an Index Scan over the partition-local child of the partial unique index unique_npm_remote_versions_ns_id_pkg_id_version — (namespace_id, npm_remote_package_id, version) WHERE soft_deleted_at IS NULL — with all three equality predicates in the Index Cond and the soft-delete gate satisfied by the index's partial predicate, leaving no Filter and no Sort; notably the planner picks this partial unique index over the two other candidates rooted at the same leading columns (index_npm_remote_versions_on_ns_id_pkg_id and the size_bytes DESC variant), which is the correct choice because only the unique one is selective on version. The namespace_id bind prunes to one of 64 hash partitions (npm_remote_versions_p02), estimates match reality exactly (1 / 1), and all 3 execution-time buffers are hits with no reads. No anomalies against the flagged criteria; as above, planning dominates execution (0.788ms against 0.019ms, shared hit=300 while opening all 64 partitions before pruning), making 0.807ms the realistic per-request cost and still leaving ample headroom.

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

Rendered SQL:

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

Bound args: ['390f85c6-3975-4c1d-a1a8-338805d2c431', '81ae5111-c103-49c4-8ee5-8eb567a58121', '1.0.2500', 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.30 rows=1 width=88) (actual time=0.011..0.011 rows=1 loops=1)
   Buffers: shared hit=3
   ->  Index Scan using npm_remote_versions_p02_namespace_id_npm_remote_package_id__idx on npm_remote_versions_p02 npm_remote_versions  (cost=0.28..8.30 rows=1 width=88) (actual time=0.010..0.010 rows=1 loops=1)
         Index Cond: ((namespace_id = '390f85c6-3975-4c1d-a1a8-338805d2c431'::uuid) AND (npm_remote_package_id = '81ae5111-c103-49c4-8ee5-8eb567a58121'::uuid) AND (version = '1.0.2500'::text))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=300
 Planning Time: 0.788 ms
 Execution Time: 0.019 ms

Timings: planning 0.788ms, execution 0.019ms, total 0.807ms.

Related to #342 (closed)

Edited by David Fernandez

Merge request reports