feat(npm): remote 304 revalidation bumps (S15 Step 6, part 5/5) (!1188) · Merge requests · GitLab.org / ops / Artifact Registry

📦 What this MR does

Part 5 of 5 of S15 Step 6 (datastore remote write — cache-fill): the 304-revalidation bumps on both blob-cache stores, plus the plan's Step 6 scope amendment.

BumpUpstreamCheckedAt (metadata and tarball stores) — the 304 write: the upstream confirmed the cached content is current, so only the freshness clock resets — no blob repoint, no etag change. A bump that matches no (active) row returns a wrapped ErrNotFound, per the S13 CacheStore seam contract: the row can vanish between the handler's Lookup and the bump, and reporting success would serve a blob reference with no cache row behind it. Step 9's adapter maps the sentinel to remote.ErrCacheEntryNotFound, firing the Fetcher's one-shot re-fetch. The tarball variant gates soft_deleted_at IS NULL, so a tombstone reports the same way and its clock never advances.
The plan's Step 6 scope/acceptance amendment recording the buffered size_bytes counter (ADR-007's storage-accounting amendment) that part 2 implements — shipped with the step per the drift carve-out.

⚙️ 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 top of the stack. Parts 1-4 have merged, so it now targets main directly and carries only its own commit; the store structs it extends are already there.

🔗 References

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

🧪 Spec coverage

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

This part implements the datastore write half of the 304 revalidation on both cache tables. Both proxies' criteria are written at the request level, so the rows below map the half this MR owns and mark the rest out of scope — Step 9 wires the adapter, Steps 10-12 the handlers. Subtests are under TestNpmRemoteFileStore_BumpUpstreamCheckedAt and TestNpmRemoteMetadataFileStore_BumpUpstreamCheckedAt unless named in full.

Acceptance criteria

#	Criterion	Tests
Packument proxy AC-6	Stale hit, etag matches: revalidation gets `304`, bumps `upstream_checked_at`, serves the cached blob with no body re-fetch	metadata `/advances upstream_checked_at and touches nothing else` — asserts the clock advances and attachment, digest and etag are untouched
Tarball proxy AC-5	Stale hit, etag matches: revalidation gets `304`, bumps `upstream_checked_at`, serves the existing blob	tarball `/advances upstream_checked_at and touches nothing else` — same assertions
Packument AC-1 to AC-5, AC-7+; Tarball AC-1 to AC-4, AC-6+	Cache miss and fill, rewrite, fresh hit, `HEAD`, pinned upstream, `200` re-publish, single-flight	Not in this part. Parts 1-4 own the fill writes; the request-level halves are Steps 9-12.
Both, request half of AC-6 / AC-5	Issuing the conditional `If-None-Match`, reading the `304`, serving the blob	Not in this part. Steps 9-12.

Error cases

Case	Behavior	Tests
Row deleted between the handler's Lookup and the bump	Wrapped `ErrNotFound`, never `nil` — the seam signal the adapter maps to `remote.ErrCacheEntryNotFound` to fire the one-shot re-fetch	`/returns ErrNotFound on a missing row` (both suites)
Tombstoned tarball row	Wrapped `ErrNotFound`, and the clock never advances so a later restore does not read as revalidated	tarball `/returns ErrNotFound on a soft-deleted row and never advances its clock`. No metadata twin: `npm_remote_metadata_files` has no `soft_deleted_at` column.
Soft-deleted parent row	Returns `nil` today on both stores, advancing the clock on a suppressed package or version	None. Known gap, recorded on both method doc comments; latent because nothing in production soft-deletes an npm-remote row.
Transient driver failure on the `UPDATE`	Wrapped with namespace and row id, and never satisfies `errors.Is(err, ErrNotFound)`	`TestNpmRemoteFileStore_BumpUpstreamCheckedAt_WrapsTransientError`, `TestNpmRemoteMetadataFileStore_BumpUpstreamCheckedAt_WrapsTransientError`
Argument guards: nil context, nil db, zero namespace, zero row id	Per-store sentinel before any DB round-trip	`TestNpmStores_BumpArgumentGuards` — eight rows, four per remote store
`RowsAffected` reports an error	Wrapped with operation context	Not covered. Every scripted driver in the package returns a nil `sql.Result` alongside its error, so the branch is unreachable from a unit test — the same position the identical wraps in `npm_tags.go` and `container_tag.go` take.

Security considerations

Consideration	Tests
Namespace isolation. These are the family's first writes keyed by a bare row id, so the `namespace_id` equality is the only tenancy control on them	`/returns ErrNotFound for another namespace's file id`, `/returns ErrNotFound for another namespace's metadata file id` — each seeds under namespace A, bumps A's real row id under namespace B, and reads back that A's clock did not move
Partition pruning. Both are writes on tables hash-partitioned 64 ways; losing the `namespace_id` pin would take row locks across every partition on the hot revalidation path	`TestNpmRemoteFileStore_BumpUpstreamCheckedAt_PrunesToOnePartition`, `TestNpmRemoteMetadataFileStore_BumpUpstreamCheckedAt_PrunesToOnePartition`

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

Migration mode does not apply: this MR adds no migration.

Method	Plan node	Index	Rows (plan / actual)	Cost	Time	Buffers (hit / read)	Partitions
`datastore.NpmRemoteFileStore.BumpUpstreamCheckedAt`	Update	`npm_remote_files_p12_pkey`	0 / 0 root; 1 / 1 driving Index Scan	8.30	0.255ms	57 / 0	1 of 64 (`npm_remote_files_p12`)
`datastore.NpmRemoteMetadataFileStore.BumpUpstreamCheckedAt`	Update	`npm_remote_metadata_files_p12_pkey`	0 / 0 root; 1 / 1 driving Index Scan	8.30	0.210ms	55 / 0	1 of 64 (`npm_remote_metadata_files_p12`)

Both are UPDATEs without RETURNING, so the root Update node reports rows=0 by construction; the row counts that carry signal are on the driving Index Scan, quoted in each details block.

datastore.NpmRemoteFileStore.BumpUpstreamCheckedAt

Summary: The plan matches the method's intent — a point write on one cache row. The planner prunes to a single partition (npm_remote_files_p12 of 64) from the namespace_id equality and drives the update off npm_remote_files_p12_pkey, the partition's unique (id, namespace_id) primary key, with both predicate columns in the Index Cond and soft_deleted_at IS NULL applied as a cheap Filter; estimate and reality agree exactly (1 / 1) with no heap reads. Seeding deviates from the skill's "1 row + 49 siblings" rule for write targets: 5000 rows were seeded into one partition, all sharing the test namespace_id, because at ~50 rows the planner picks a Seq Scan on a 64-way hash-partitioned table even when the right index exists — the same false negative the skill warns about for read targets. No anomalies.

Rendered SQL:

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

Bound args: [aaaaaaaa-0000-4000-8000-000000000001, aaaaaaaa-0000-4000-8000-000000000006]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_remote_files  (cost=0.28..8.30 rows=0 width=0) (actual time=0.255..0.255 rows=0 loops=1)
   Update on npm_remote_files_p12 npm_remote_files_1
   Buffers: shared hit=57
   ->  Index Scan using npm_remote_files_p12_pkey on npm_remote_files_p12 npm_remote_files_1  (cost=0.28..8.30 rows=1 width=18) (actual time=0.025..0.025 rows=1 loops=1)
         Index Cond: ((id = 'aaaaaaaa-0000-4000-8000-000000000006'::uuid) AND (namespace_id = 'aaaaaaaa-0000-4000-8000-000000000001'::uuid))
         Filter: (soft_deleted_at IS NULL)
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=513
 Planning Time: 1.573 ms
 Execution Time: 0.421 ms

Timings: planning 1.573ms, execution 0.421ms, total 1.994ms. The planning figure is first-plan-in-session catalog warm-up over a 64-way partitioned target; re-planning the identical statement later in the same session costs 0.076 ms and 0.060 ms with Planning: Buffers: shared hit=7, and the root node's buffer count falls from 57 to 7.

RowsAffected contract: verified directly against the seeded table. The WHERE matches 1 row for a live row addressed by the correct (namespace_id, id); it matches 0 rows for an unknown id, for the correct id under a different namespace_id, and for a row whose soft_deleted_at was set — so each of those reaches the affected == 0 branch and returns the wrapped ErrNotFound.

datastore.NpmRemoteMetadataFileStore.BumpUpstreamCheckedAt

Summary: The plan matches the method's intent and mirrors its tarball twin — a point write pruned to a single partition (npm_remote_metadata_files_p12 of 64) off npm_remote_metadata_files_p12_pkey, the unique (id, namespace_id) primary key, with both predicate columns in the Index Cond, 1 / 1 estimate versus actual, and no heap reads. It carries no soft_deleted_at IS NULL gate and correctly so: npm_remote_metadata_files has no soft_deleted_at column, confirmed against the migrated schema. Seeding deviates from the skill's "1 row + 49 siblings" rule for write targets: 5000 rows were seeded into one partition, all sharing the test namespace_id, because at ~50 rows the planner picks a Seq Scan on a 64-way hash-partitioned table even when the right index exists. No anomalies.

Seed shape: namespaces=1, repositories=1, npm_remote_repositories=1, npm_remote_packages=5000, npm_remote_versions=1, blob_storage_blobs=1, blob_storage_attachments=1, npm_remote_files=5000, npm_remote_metadata_files=5000 (one shared committed seed; the target table here is npm_remote_metadata_files, 5000 rows in npm_remote_metadata_files_p12). The 5000 parent packages are forced by the target's own shape, not by the ancestor rule: unique_npm_remote_metadata_files_ns_id_pkg_id_kind plus CHECK (kind = ANY (ARRAY[0, 1, 2])) caps the table at three rows per package, so 5000 target rows in one partition need 5000 distinct packages in that namespace. ANALYZE ran on every seeded table after the seed committed.

Rendered SQL:

UPDATE public.npm_remote_metadata_files
SET upstream_checked_at = NOW()
WHERE (npm_remote_metadata_files.namespace_id = $1::uuid) AND (npm_remote_metadata_files.id = $2::uuid);

Bound args: [aaaaaaaa-0000-4000-8000-000000000001, aaaaaaaa-0000-4000-8000-000000000007]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_remote_metadata_files  (cost=0.28..8.30 rows=0 width=0) (actual time=0.210..0.210 rows=0 loops=1)
   Update on npm_remote_metadata_files_p12 npm_remote_metadata_files_1
   Buffers: shared hit=55
   ->  Index Scan using npm_remote_metadata_files_p12_pkey on npm_remote_metadata_files_p12 npm_remote_metadata_files_1  (cost=0.28..8.30 rows=1 width=18) (actual time=0.016..0.017 rows=1 loops=1)
         Index Cond: ((id = 'aaaaaaaa-0000-4000-8000-000000000007'::uuid) AND (namespace_id = 'aaaaaaaa-0000-4000-8000-000000000001'::uuid))
         Buffers: shared hit=3
 Planning:
   Buffers: shared hit=482
 Planning Time: 1.258 ms
 Execution Time: 0.324 ms

Timings: planning 1.258ms, execution 0.324ms, total 1.582ms. As with the tarball twin, the planning figure is first-plan-in-session catalog warm-up over a 64-way partitioned target, not steady-state cost.

RowsAffected contract: verified directly against the seeded table. The WHERE matches 1 row for the correct (namespace_id, id) and 0 rows for an unknown id, so the miss reaches the affected == 0 branch and returns the wrapped ErrNotFound.

Query notes: no anomalies. Two candidates were raised during the run and both dismissed after re-checking against the seed shape rather than reported as findings.

FK trigger time on the UPDATE — seeding and running EXPLAIN inside one transaction showed four Trigger for constraint fk_npm_remote_files_* lines totalling ~2.5ms, even though the statement touches only upstream_checked_at and no FK column. That is an artifact of seeding in the same transaction: RI_FKey_fk_upd_check_required forces the referential check when the old row was inserted by the current transaction, bypassing the usual unchanged-key skip. Re-running with the seed committed first removed the triggers entirely — the plans above are from the committed-seed run. No production RI cost exists on this write.
Planning time exceeding execution time (1.573ms vs 0.421ms, 513 planning buffers) — cold catalog cost for first touch of a 64-way partitioned target in a fresh session. Re-planning the identical statement in the same session cost 0.076ms and 0.060ms with 7 planning buffers.

Checked and clear on the rest: no Seq Scan at 5000 seeded rows; single-partition pruning on both, confirmed by count(DISTINCT tableoid) = 1; both predicate columns land in the Index Cond of the unique (id, namespace_id) primary key, so each lookup is a unique point probe; plan-versus-actual 1 / 1 on both driving scans; read=0 on every node; no Sort; and neither statement is a row-returning SELECT, so the unbounded-SELECT rule does not apply.

Edited Aug 06, 2026 by David Fernandez

feat(npm): remote 304 revalidation bumps (S15 Step 6, part 5/5)