feat(npm): remote packument metadata cache reads (S15 Step 5, part 3/4)

David Fernandezrequested to merge
10io/s15-npm-remote-step-5-part3of4 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. Together the four parts are byte-identical to the change originally proposed in !1050 (closed), which this stack supersedes.

Stacked MRs (review/merge bottom-up)

📦 What this adds (part 3 of 4)

The packument/dist-tags cache reads with a freshness verdict: NpmRemoteMetadataFileByPackageAndKind returns an NpmRemoteMetadataFileLookup — the row's id, its blob coordinates and its ETag, plus a Fresh boolean computed in SQL against the database clock (the same clock the cache-fill writer stamps upstream_checked_at with). A stale row is still returned — carrying its ETag — so the handler can revalidate with If-None-Match rather than refetch blindly; only a missing row is the Miss. Metadata is never pinned: its validity window is > 0 by DB CHECK, so the guard rejects a non-positive window.

Two pieces of shared surface land here as well, both first-mover extractions the tarball read in part 4 wraps or reuses: NpmMetadataKind (internal/datastore/npm_metadata_kinds.go), which stops a kind being swapped with the cache window at a call site — it names the domain but does not close it, so the store's kind guard stays alongside it; and freshWithinHoursExpr (internal/datastore/npm_remote_freshness.go), the single aliased fresh projection both npm-remote cache reads build through, so the cache-clock semantics — including the pinned-window case, where a zero window collapses to a pinned-fresh projection — exist in one place.

🧪 Spec coverage

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

This part ships the datastore read only, so criteria whose behavior lives in the handler, the cache-fill writer, or the S13 upstream client name the part that owns them. An empty test column below is a scope boundary, not missing coverage. Subtests are named relative to TestNpmRemoteMetadataFileStore_NpmRemoteMetadataFileByPackageAndKind; the guard cases live in the _ArgumentGuards sibling.

Acceptance criteria

# Criterion Tests
Packument proxy 1 Cache miss: an uncached variant is the Miss the handler fetches on .../returns ErrNotFound when no row is cached (cache miss), .../returns ErrNotFound for a kind that is not cached
Packument proxy 4 Abbreviated packument: the kind=0 and kind=2 variants are cached independently .../caches all three kind variants independently
Packument proxy 5 Fresh hit: a lookup within metadata_cache_validity_hours serves from cache with no upstream call .../returns Fresh=true for a row within the metadata window, .../a row just inside the window edge is Fresh, .../applies a sub-day window as hours, not days
Packument proxy 6 Stale hit, etag matches: the stale row is returned carrying upstream_etag, so the handler can revalidate rather than refetch .../returns the row with Fresh=false when past the metadata window, .../a row just outside the window edge is stale, .../a row older than a sub-day window is stale
Packument proxy 7 Stale hit, etag differs: re-stream, re-transform, repoint the row Cache-fill writer. Not in this MR.
Packument proxy 8 A new upstream version surfaces after the metadata window; the window is rejected at 0, so metadata is never pinned .../rejects a non-positive metadata window, plus the three stale-verdict subtests above
Packument proxy 9 Dist-tags document: the kind=1 document is cached in its own row and a fresh hit serves from cache .../caches all three kind variants independently
Packument proxy 2, 3, 10 dist.tarball rewrite, dist.shasum/dist.integrity preservation, client-side tag resolution Packument transform and handler. Not in this MR — the cached document is opaque bytes to this store.
Packument proxy 11, 12 Single-flight, and leader-failure propagation S13 foundation. Not in this MR.
Tarball proxy, write unavailability, credentials and health — Other parts of the step and other stores. Not in this MR.

Error cases

Condition Tests
Transport failure, cache entry exists: serve the cached copy, fresh or stale The datastore half is that an expired row is returned rather than collapsing to a miss: .../returns the row with Fresh=false when past the metadata window, .../a stale row with no upstream etag returns a nil UpstreamEtag. Status and header mapping: handler. Not in this MR.
Upstream 404 for a packument, upstream 5xx, transport failure with no cache entry (503 upstream_unavailable) Handler and S13 upstream client. Not in this MR — this store issues no upstream request.
Write verb on a remote repository (405 unsupported), {file_name} validation, package-name regex Routing and handler. Not in this MR.
A transient database failure must never be reported as a cache Miss (store-level invariant, not a spec row) .../a transient DB failure is not misreported as ErrNotFound
A wiring bug in the call must not be reported as a cache Miss: nil context, zero namespace, zero package id, out-of-domain kind, non-positive window ..._ArgumentGuards (nil context, zero uuid.UUID, rejects a zero package id, negative kind, kind above maximum, rejects a non-positive metadata window). NpmMetadataKind prevents a kind/window swap at the call site; the guard bounds the domain, since a named integer type in Go admits every value of its underlying type.
The shared freshness projection must not render an empty interval for a zero window (which would read every row stale) TestFreshWithinHoursExpr (rendered-SQL assertion; the pinned branch is unreachable from this store's suite, whose guard rejects a zero window). The pinned-upstream criterion it serves is the tarball read's.

Security considerations

Concern Tests
Namespace isolation (ADR-001): a cached document must never be readable under another namespace's id, and namespace_id is what prunes the hash-partitioned table .../does not return metadata cached in a different namespace; plan shape: TestNpmRemoteMetadataFileStore_NpmRemoteMetadataFileByPackageAndKind_PrunesToOnePartition
Package isolation: metadata cached for one package must never be served for another in the same namespace .../does not return metadata cached under a different package
Kind isolation: a document of one kind must never satisfy a lookup for another .../returns ErrNotFound for a kind that is not cached, .../caches all three kind variants independently
An operator-suppressed package must not serve or revalidate metadata Gated upstream, at the parent package resolution: npm_remote_metadata_files carries no soft_deleted_at, and NpmRemotePackageByName (part 2) is the only production source of the package id — TestNpmRemotePackageStore_NpmRemotePackageByName/returns ErrNotFound for a soft-deleted package. The store's doc comment records the reasoning.
The freshness verdict must not be recomputable on a skewed application clock The verdict is computed in SQL against the database clock and upstream_checked_at is not exported, so a caller has nothing to recompute from: .../returns Fresh=true for a row within the metadata window (whole-struct diff over the lookup)
Credential hygiene, credentials at rest S13 and part 1. Not in this MR — this store reads no credential column.
SSRF, cross-origin redirect token stripping, outbound path-segment safety, tarball-URL rewrite, client-enforced tarball integrity, error-payload hygiene S13 upstream client, packument transform, and handler. Not in this MR — no outbound request and no client-visible payload here.

🔗 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.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.NpmRemoteMetadataFileStore.NpmRemoteMetadataFileByPackageAndKind Limit → Index Scan npm_remote_metadata_files_p17_namespace_id_npm_remote_packa_idx 1 / 1 8.31 0.038ms 6 / 0 1 of 64
datastore.NpmRemoteMetadataFileStore.NpmRemoteMetadataFileByPackageAndKind

Summary: The plan matches the method's intent. The driving scan is an Index Scan over the partition-local child of the unique index unique_npm_remote_metadata_files_ns_id_pkg_id_kind — (namespace_id, npm_remote_package_id, kind) — whose Index Cond absorbs all three equality predicates, so the plan carries no Filter and no Sort and the LIMIT never discards a row; the fresh boolean is a projection over the row the index already located, so computing it against the database clock adds no scan work. Passing namespace_id prunes to exactly one of the table's 64 hash partitions (npm_remote_metadata_files_p17), the estimate matches reality exactly (1 / 1), and all 6 execution-time buffer accesses are cache hits with no reads, against 5001 seeded metadata rows spread over 3 kinds for each of 1667 packages in that one partition. The narrowed four-column SELECT shows up as the plan's row width (74 bytes, against 145 when the projection was the whole model), and it does not change the access path: the omitted columns were never index-covered, so the scan still fetches the heap tuple the index points at. No anomalies against the flagged criteria. Two properties worth recording rather than flagging: the validity window is inlined by the builder as an INTERVAL '1 DAY' literal instead of a bind parameter, so a repository configured with a different metadata_cache_validity_hours produces a distinct statement text (and its own plan-cache entry) — harmless at the handful of distinct window values a deployment uses, and it lets the planner see the constant; and planning (0.251ms) still exceeds execution (0.051ms), so the realistic per-request cost is the 0.302ms total rather than the 0.038ms in the Time column, which leaves ample headroom inside the 100 ms budget.

Seed shape: namespaces=1, repositories=1, npm_remote_repositories=1, blob_storage_blobs=1, blob_storage_attachments=1, npm_remote_packages=1667, npm_remote_metadata_files=5001

Rendered SQL:

SELECT npm_remote_metadata_files.id AS "npm_remote_metadata_files.id",
     npm_remote_metadata_files.blob_storage_attachment_id AS "npm_remote_metadata_files.blob_storage_attachment_id",
     npm_remote_metadata_files.blob_sha256 AS "npm_remote_metadata_files.blob_sha256",
     npm_remote_metadata_files.upstream_etag AS "npm_remote_metadata_files.upstream_etag",
     (npm_remote_metadata_files.upstream_checked_at > (NOW() - INTERVAL '1 DAY')) AS "fresh"
FROM public.npm_remote_metadata_files
WHERE ((npm_remote_metadata_files.namespace_id = $1::uuid) AND (npm_remote_metadata_files.npm_remote_package_id = $2::uuid)) AND (npm_remote_metadata_files.kind = $3)
LIMIT $4;

Bound args: ['f9891b93-93d7-4ebf-b174-2150b25237ef', 'fc65bee7-04b1-4f36-b2e7-cdc0c2bcee14', 0, 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.31 rows=1 width=74) (actual time=0.038..0.038 rows=1 loops=1)
   Buffers: shared hit=6
   ->  Index Scan using npm_remote_metadata_files_p17_namespace_id_npm_remote_packa_idx on npm_remote_metadata_files_p17 npm_remote_metadata_files  (cost=0.28..8.31 rows=1 width=74) (actual time=0.037..0.037 rows=1 loops=1)
         Index Cond: ((namespace_id = 'f9891b93-93d7-4ebf-b174-2150b25237ef'::uuid) AND (npm_remote_package_id = 'fc65bee7-04b1-4f36-b2e7-cdc0c2bcee14'::uuid) AND (kind = '0'::bigint))
         Buffers: shared hit=6
 Planning:
   Buffers: shared hit=42
 Planning Time: 0.251 ms
 Execution Time: 0.051 ms

Timings: planning 0.251ms, execution 0.051ms, total 0.302ms.

Related to #342 (closed)

Edited by David Fernandez

Merge request reports