feat(npm): remote tarball cache-fill upsert (S15 Step 6, part 4/5)

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

📦 What this MR does

Part 4 of 5 of S15 Step 6 (datastore remote write — cache-fill): the tarball cache-fill upsert on npm_remote_files, the twin of part 3's metadata write.

  • UpsertNpmRemoteFileForBlob — same statement order and reasons as part 3 (parent-version FOR UPDATE lock, old-attachment read, fresh mint, repoint, in-transaction delete), with the tarball-specific differences:
    • The conflict target is the PARTIAL unique index (WHERE soft_deleted_at IS NULL), so a soft-deleted tarball never conflicts: re-caching inserts a fresh active row, and the old-attachment read gates soft deletion to match — a tombstone's attachment survives until GC, never dying with a re-cache.
    • A file-name length guard rejects names over the 255-character column CHECK with a sentinel before any SQL: npm's format rules bound the package-name half of a tarball name (≤ 214) but not the version half, so a legitimately-shaped name can exceed the limit and must not surface as an unclassified constraint PgError.
  • ErrParentNpmRemoteVersionMissing — the constraint-name-matched parent sentinel.
  • NpmRemoteFileStore.existingAttachment — the per-store old-attachment read, mirroring the metadata twin part 3's review settled on. It scans into model.NpmRemoteFiles rather than an alias-tagged anonymous struct, for the reason that review recorded: go-jet matches a result set to its dest by embedded table model, so a scan target shared across both tables would rest on alias tags matched against .AS() literals, a contract no compiler checks and whose failure mode is silent — an unmatched projection returns the zero value with a nil error, and the caller then deletes attachment id 0 and orphans the real one.
  • npm_remote_metadata_files.go gains a //nolint:dupl on repointNpmRemoteMetadataFile and a corrected file-header note. Part 3 merged with the header stating there was no clone to pair, which was true only while npm_remote_files.go had no write side; this part adds that write side, so dupl now pairs the two repoint helpers and both ends carry the suppression.
  • Guard, behavior, concurrency (cross-replica race), and partition-pruning tests, mirroring part 3's suite plus the soft-delete re-cache coverage.

⚙️ 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 sits on part 3 (reuses validUpstreamEtag) and part 2 (FK to npm_remote_versions).

🔗 References

  • Plan: docs/plans/2026-07-15-npm-remote.md — Step 6
  • Spec: docs/specs/S15-npm-remote.md — Cache fill, Tarball proxy, Freshness

🧪 Spec coverage

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

This part implements the datastore write half of the tarball cache fill. The Tarball proxy acceptance criteria are written at the request level, so the rows below map the half this MR owns and mark the rest out of scope for it — Steps 10-12 wire the handlers. Subtest paths are under TestNpmRemoteFileStore_UpsertNpmRemoteFileForBlob.

Acceptance criteria

# Criterion (Tarball proxy) Tests
AC-1 Cache miss, fill: writes npm_remote_files on complete receipt, and a truncated stream commits nothing /first fill creates an attachment and the tarball row, /a rolled-back fill leaves the prior row, etag, and attachment intact, /a cancelled context leaves the prior row, etag, and attachment intact
AC-6 Stale hit, upstream re-published: repoints blob_sha256, sets upstream_checked_at and upstream_etag, stops referencing the superseded blob /refill repoints to a new attachment, restamps etag and checked-at, and hard-deletes the old attachment, /refill with identical bytes swaps the attachment and keeps the blob referenced
AC-7 Stale hit, no stored ETag: the freshly fetched body replaces the cached blob /refill with no upstream etag clears the stored etag to NULL
AC-2, AC-3, AC-4, AC-5, AC-8 to AC-13 Fresh hit, HEAD, pinned upstream, 304 revalidation, single-flight, filename rejection, outbound segment safety Not in this part. Request-level behavior, or the 304 checked-at bump, which this MR does not add. Steps 10-12.

Data Model behavior rather than a Tarball proxy criterion: the partial unique index makes a soft-deleted row non-conflicting, covered by /a soft-deleted row does not conflict: re-caching inserts a fresh active row and spares the tombstone's attachment. Partition pruning for both statements is covered by TestNpmRemoteFileStore_UpsertNpmRemoteFileForBlob_PrunesToOnePartition and TestNpmRemoteFileStore_LockParentVersionForFill_PrunesToOnePartition.

Error cases

The spec's Error Cases table is HTTP-level and no row is reachable from this layer. The store's own error contract, which those rows are mapped from later:

Condition Contract Tests
Parent version absent ErrParentNpmRemoteVersionMissing, returned unwrapped /returns ErrParentNpmRemoteVersionMissing for a missing parent version
Parent version owned by another namespace (composite FK) the same sentinel, on the cross-tenant rejection /rejects another namespace's remote version (composite FK)
CAS blob row absent ErrParentBlobMissing /surfaces ErrParentBlobMissing when the CAS blob row is absent
Both parents absent blob sentinel wins, since the mint precedes the upsert /blob sentinel wins when the blob and the parent version are both missing
Namespace absent neither parent sentinel /ghost namespace is not misclassified as a parent sentinel
Nil ctx or db, zero UUIDs, empty or invalid file name, invalid etag typed sentinels before any round-trip TestNpmRemoteFileStore_UpsertNpmRemoteFileForBlob_ArgumentGuards, TestNpmRemoteFileStore_UpsertNpmRemoteFileForBlob_NilDB, TestNpmRemoteFileStore_existingAttachment_NilDB, TestValidFileName
Concurrent fills of one key across replicas serialize on the parent lock, orphan no attachment /concurrent fills of the same key serialize on the parent lock and orphan no attachment

Security considerations

# Concern Tests
S-1 Error-payload hygiene: the FK sentinel must not carry PgError.Detail or PgError.Where into Error() /returns ErrParentNpmRemoteVersionMissing for a missing parent version and /rejects another namespace's remote version (composite FK), which assert NotErrorAs(*pgconn.PgError) and that the FK key and the other namespace id are absent from Error()
S-2 Cross-tenant isolation: namespace B must not hang a cached tarball off namespace A's version row /rejects another namespace's remote version (composite FK)
S-3 Outbound path-segment safety: this store is the last gate for a caller not routed through npmrules.ValidateFileName TestValidFileName, plus the invalid-UTF-8, NUL and CR/LF rows in TestNpmRemoteFileStore_UpsertNpmRemoteFileForBlob_ArgumentGuards
S-4 Credential hygiene, SSRF, cross-origin redirect token stripping, tarball integrity Not in this part: no upstream client, no credentials, no relayed bytes. S13-owned, or Steps 10-12.

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

No migrations in this MR, so migration mode did not run.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.NpmRemoteFileStore.existingAttachment Limit → Index Scan unique_npm_remote_files_ns_id_ver_id_file_name 1 / 1 8.30 0.028ms 3 / 0 1 of 64
datastore.NpmRemoteFileStore.repointNpmRemoteFile.parent-lock LockRows → Index Scan pk_npm_remote_versions 1 / 1 8.31 0.032ms 4 / 0 1 of 64
datastore.NpmRemoteFileStore.repointNpmRemoteFile.upsert-insert Insert (ON CONFLICT DO UPDATE) arbiter unique_npm_remote_files_ns_id_ver_id_file_name n/a / 1 inserted 0.01 1.722ms 74 / 0 1 of 64
datastore.NpmRemoteFileStore.repointNpmRemoteFile.upsert-conflict Insert (ON CONFLICT DO UPDATE) arbiter unique_npm_remote_files_ns_id_ver_id_file_name n/a / 1 conflicting 0.01 0.423ms 48 / 2 1 of 64
datastore.NpmRemoteFileStore.existingAttachment

Summary: Plan matches the method's intent. The planner picks the partition-local instance of the PARTIAL unique index, whose WHERE soft_deleted_at IS NULL predicate is exactly the gate the statement carries, so the soft-delete filter costs no extra work; the namespace_id literal prunes to one of 64 partitions. Estimate matches reality (1 / 1) at 5000 seeded rows in the target partition. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_remote_repositories=1, npm_remote_packages=1, npm_remote_versions=5000, blob_storage_blobs=5001, blob_storage_attachments=5001, npm_remote_files=5000

Rendered SQL:

SELECT npm_remote_files.blob_storage_attachment_id AS "npm_remote_files.blob_storage_attachment_id",
     npm_remote_files.blob_sha256 AS "npm_remote_files.blob_sha256"
FROM public.npm_remote_files
WHERE (((npm_remote_files.namespace_id = $1::uuid) AND (npm_remote_files.npm_remote_version_id = $2::uuid)) AND (npm_remote_files.file_name = $3::text)) AND (npm_remote_files.soft_deleted_at IS NULL)
LIMIT $4;

Bound args: ['ca496685-e00f-4985-a4d4-bc51ded8b0a1', '07b7553c-bec9-44a8-a775-e1aa6495cf65', 'lodash-4.17.2500.tgz', 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=0.28..8.30 rows=1 width=41) (actual time=0.017..0.017 rows=1 loops=1)
   Buffers: shared hit=3
   ->  Index Scan using npm_remote_files_p49_namespace_id_npm_remote_version_id_fil_idx on npm_remote_files_p49 npm_remote_files  (cost=0.28..8.30 rows=1 width=41) (actual time=0.016..0.016 rows=1 loops=1)
         Index Cond: ((namespace_id = 'ca496685-e00f-4985-a4d4-bc51ded8b0a1'::uuid) AND (npm_remote_version_id = '07b7553c-bec9-44a8-a775-e1aa6495cf65'::uuid) AND (file_name = 'lodash-4.17.2500.tgz'::text))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=421
 Planning Time: 1.211 ms
 Execution Time: 0.028 ms

Timings: planning 1.211ms, execution 0.028ms, total 1.239ms. The planning buffers (421) and planning time are the one-time partition-metadata load on a cold backend, not a per-call cost.

datastore.NpmRemoteFileStore.repointNpmRemoteFile.parent-lock

Summary: Plan matches the method's intent: LockRows over an Index Scan on the primary key, pruned to one of 64 partitions by the namespace_id literal. The lock is a single-row SELECT ... FOR UPDATE on the parent version, so it takes one row lock and reads 4 buffers regardless of how many versions the namespace holds — the serialization point costs the same at any cardinality. Estimate matches reality (1 / 1). No anomalies.

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 "id"
FROM public.npm_remote_versions
WHERE (npm_remote_versions.namespace_id = $1::uuid) AND (npm_remote_versions.id = $2::uuid)
FOR UPDATE;

Bound args: ['ca496685-e00f-4985-a4d4-bc51ded8b0a1', '07b7553c-bec9-44a8-a775-e1aa6495cf65']

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 LockRows  (cost=0.28..8.31 rows=1 width=26) (actual time=0.023..0.024 rows=1 loops=1)
   Buffers: shared hit=4
   ->  Index Scan using npm_remote_versions_p49_pkey on npm_remote_versions_p49 npm_remote_versions  (cost=0.28..8.30 rows=1 width=26) (actual time=0.009..0.010 rows=1 loops=1)
         Index Cond: ((id = '07b7553c-bec9-44a8-a775-e1aa6495cf65'::uuid) AND (namespace_id = 'ca496685-e00f-4985-a4d4-bc51ded8b0a1'::uuid))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=7
 Planning Time: 0.097 ms
 Execution Time: 0.032 ms

Timings: planning 0.097ms, execution 0.032ms, total 0.129ms.

datastore.NpmRemoteFileStore.repointNpmRemoteFile.upsert-insert

Summary: The first-fill path. The planner resolves the conflict arbiter to unique_npm_remote_files_ns_id_ver_id_file_name — the PARTIAL index — which is what makes a soft-deleted row non-conflicting, and the row lands in one of 64 partitions. Most of the 1.7ms is the four FK constraint triggers (attachment, namespace, version, blob), not the insert itself. No anomalies.

Seed shape: npm_remote_files=5000 (plus the ancestor chain), executed inside a rolled-back transaction

Rendered SQL:

INSERT INTO public.npm_remote_files (id, namespace_id, npm_remote_version_id, blob_storage_attachment_id, upstream_checked_at, file_name, blob_sha256, upstream_etag)
VALUES ($1::uuid, $2::uuid, $3::uuid, $4, NOW(), $5::text, $6::bytea, $7::text)
ON CONFLICT (namespace_id, npm_remote_version_id, file_name) WHERE soft_deleted_at IS NULL DO UPDATE
       SET blob_storage_attachment_id = $8,
           blob_sha256 = $9::bytea,
           upstream_etag = $10::text,
           upstream_checked_at = NOW();

Bound args: [<new uuid>, 'ca496685-e00f-4985-a4d4-bc51ded8b0a1', '07b7553c-bec9-44a8-a775-e1aa6495cf65', <attachment id>, 'lodash-4.17.NEW.tgz', <32-byte sha256>, '"etag-new"', <attachment id>, <32-byte sha256>, '"etag-new"']

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Insert on npm_remote_files  (cost=0.00..0.01 rows=0 width=0) (actual time=0.293..0.293 rows=0 loops=1)
   Conflict Resolution: UPDATE
   Conflict Arbiter Indexes: unique_npm_remote_files_ns_id_ver_id_file_name
   Tuples Inserted: 1
   Conflicting Tuples: 0
   Buffers: shared hit=74
   ->  Result  (cost=0.00..0.01 rows=1 width=168) (actual time=0.001..0.001 rows=1 loops=1)
 Planning:
   Buffers: shared hit=6
 Planning Time: 0.165 ms
 Trigger for constraint fk_npm_remote_files_blob_storage_attachment_id_bsa on npm_remote_files_p49: time=0.239 calls=1
 Trigger for constraint fk_npm_remote_files_namespace_id_namespaces on npm_remote_files_p49: time=0.088 calls=1
 Trigger for constraint fk_npm_remote_files_npm_remote_version_id on npm_remote_files_p49: time=0.099 calls=1
 Trigger for constraint fk_npm_remote_files_ns_id_blob_sha256_blobs on npm_remote_files_p49: time=0.975 calls=1
 Execution Time: 1.722 ms

Timings: planning 0.165ms, execution 1.722ms, total 1.887ms.

datastore.NpmRemoteFileStore.repointNpmRemoteFile.upsert-conflict

Summary: The refill path, same statement against an existing active key: Conflicting Tuples: 1, resolved by the DO UPDATE branch that repoints the row. It is cheaper than the insert path (0.42ms vs 1.72ms) because only two FK triggers fire — the namespace and version references are unchanged by the update, so only the attachment and blob FKs are re-checked. No anomalies.

Seed shape: npm_remote_files=5000 (plus the ancestor chain), executed inside a rolled-back transaction

Rendered SQL: identical to the insert-path row above.

Bound args: as above, with file_name = 'lodash-4.17.2500.tgz' (an existing active row) and upstream_etag = '"etag-refill"'

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Insert on npm_remote_files  (cost=0.00..0.01 rows=0 width=0) (actual time=0.261..0.261 rows=0 loops=1)
   Conflict Resolution: UPDATE
   Conflict Arbiter Indexes: unique_npm_remote_files_ns_id_ver_id_file_name
   Tuples Inserted: 0
   Conflicting Tuples: 1
   Buffers: shared hit=48 read=2
   ->  Result  (cost=0.00..0.01 rows=1 width=168) (actual time=0.001..0.001 rows=1 loops=1)
 Planning Time: 0.060 ms
 Trigger for constraint fk_npm_remote_files_blob_storage_attachment_id_bsa on npm_remote_files_p49: time=0.084 calls=1
 Trigger for constraint fk_npm_remote_files_ns_id_blob_sha256_blobs on npm_remote_files_p49: time=0.056 calls=1
 Execution Time: 0.423 ms

Timings: planning 0.060ms, execution 0.423ms, total 0.483ms.

No anomalies were flagged: every statement prunes to a single partition, uses the index its predicate shape calls for, and matches its row estimate.

Edited by David Fernandez

Merge request reports