fix(datastore): stop the npm remote 304 bumps on a soft-deleted parent

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/540-npm-remote-bump-parent-gate into main

What this fixes

When an npm remote repository revalidates a cached file with its upstream and gets a 304 Not Modified, we bump that row's upstream_checked_at so it counts as fresh again. The bump also ran when a row above it had been soft-deleted, and it reported success. A tarball under a suppressed version or package, or a packument under a suppressed package, then read as freshly revalidated.

Two statements were involved:

  • NpmRemoteFileStore.BumpUpstreamCheckedAt checked only the tarball row's own tombstone, nothing above it.
  • NpmRemoteMetadataFileStore.BumpUpstreamCheckedAt checked nothing but the row's id. npm_remote_metadata_files has no soft_deleted_at column, and a soft delete is an UPDATE, so no foreign key would cascade one either.

How

Each WHERE clause gained a correlated EXISTS over its parents, filtered on soft_deleted_at IS NULL. A packument reaches its npm_remote_packages row; a tarball reaches its npm_remote_versions row and joins through to the package, so the whole npm-remote chain is gated. Every partitioned table in the statement repeats the namespace as a bound value of its own, including the one the join's equality already implies, so each still reaches one hash partition out of 64 instead of probing all of them.

The repository level is not part of the EXISTS, and that is deliberate. repositories.soft_deleted_at is reachable from here, through npm_remote_repositories, and repository delete sets it. FindRemoteByRepositoryID proves that row live at request start, but the bump lands after the upstream round trip, so the read does not cover the window. What settles it is the fill path: a bump a repository gate refused reports ErrNotFound, the cache-store adapter turns that into ErrCacheEntryNotFound, remote.Fetcher.Fetch retries once as an unconditional GET, and the fill behind that 200 re-freshens the same live rows. A parent gate is only worth having where the fill cannot undo it, and the package gate is: the fill's package upsert arbitrates on soft_deleted_at IS NULL, so it inserts a fresh row rather than re-freshening the tombstoned one.

No new error type. A statement that matches no row already falls into the existing affected == 0 branch, so a suppressed parent surfaces the same ErrNotFound a vanished row does, and the cache-store adapter above turns that into a re-fetch exactly as before.

Tests

Three new subtests: a tarball under a soft-deleted version, a tarball under a soft-deleted package with a live version between them, and a packument under a soft-deleted package. Each seeds a live cache row, asserts ErrNotFound, and reads the row back to prove the freshness clock did not move. All three fail against the pre-change statements and pass with the new ones, verified locally against PostgreSQL 16. The package subtest also fails with only the package predicate removed and the version gate left in place, so it is pinned to the level it names. The two partition-pruning tests now also assert that every table the parent probe reaches prunes to a single partition: three tables for the tarball statement, two for the packument one.

What this is worth

Worth stating plainly, because it sets the severity: no request serves wrong bytes today, before or after this change.

  • The read path already gates both parents (NpmRemotePackageByName and NpmRemoteVersionByPackageAndVersion both filter soft_deleted_at IS NULL), so a suppressed row is never resolved into a serve. The window the bump could be reached through is the race between an in-flight 304 and an eviction committing behind it.
  • Nothing else reads upstream_checked_at: only the read path's own freshness projection, which applies those gates, and the management projection, which keeps suppressed rows off its routes.
  • Nothing in internal/datastore ever clears soft_deleted_at on an npm-remote row, so no restore re-exposes a wrongly freshened one.

So this is an invariant fix (the statement now refuses what the read path would refuse) rather than a live incident. It takes the correlated-EXISTS shape bumpContainerRemoteTagCheckedAtStmt uses, though not its reach: that statement gates its parent repositories row too, and these two stop at the package, for the fill-path reason ## How gives.

Notes for the reviewer

  • Production does soft-delete these rows. NpmRemoteEvictor.EvictNpmRemotePackage and EvictNpmRemoteVersion are reached from DELETE .../npm/packages/{id} and DELETE .../npm/versions/{id} and from the bulk npm remote evict worker. Each marks the named row alone: a marked package keeps its versions live, which is exactly the row shape the tarball statement now has to gate, and why the fix does not stop at the immediate parent.
  • The doc comments shrank a lot. Both BumpUpstreamCheckedAt doc comments described this gap at length and pointed at #540 (closed), so the fix had to edit them, and editing a block is what makes scripts/ci/check-comment-caps.sh measure it. They were well over the 3-line cap for an exported method, so they are rewritten to the cap rather than trimmed. Same story for both statement builders (1 line, unexported) and for the two comments in the test files (2 lines). The "standalone so the pruning test EXPLAINs the exact statement" rationale does not fit a 1-line cap; it stays stated on the sibling builders in both files, which is where a reader of either file meets it.
  • No e2e scenario added. The eviction routes above are covered by e2e.npm.remote.evict-package and e2e.npm.remote.evict-file in docs/testing/e2e/npm.md, and the 304 half by e2e.npm.remote.packument-revalidate and e2e.npm.remote.tarball-revalidate. None of them changes behavior here: each drives the two sides in sequence, and the read path's own gates already make the eviction observable as a cache miss. The window this closes is the interleaving of the two, which no e2e scenario can drive deterministically, so the three integration subtests are the coverage. (There is no e2e.npm.remote.evict-version scenario for the version-level route; that gap is pre-existing and not created by this MR.)
  • ADR conformance. Checked against ADR-007 at its upstream version, not the local mirror: scripts/adr-freshness.sh reports the mirror stale by one commit that touches 007_database_schema.md. That commit amends the namespace serviceability bullet, which classifies write predicates by what the lookup does and leaves platform-driven writes unclassified. This MR changes no namespace predicate, so there is no deviation.
  • No overlap with open MRs. The five changed files were checked against the diffs of every open merge request; none of them touches these files.

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; server reported 17.11), 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.NpmRemoteFileStore.BumpUpstreamCheckedAt Update n/a 0 / 0 3.80 0.070ms 10 / 2 1/64, 1/64, 1/64
datastore.NpmRemoteMetadataFileStore.BumpUpstreamCheckedAt Update n/a 0 / 0 3.51 0.035ms 8 / 0 1/64, 1/64
datastore.NpmRemoteFileStore.BumpUpstreamCheckedAt

Summary: The plan matches the method's intent. The correlated EXISTS is pulled up into a Nested Loop Semi Join, and all three partitioned tables prune to one partition of 64 each (npm_remote_files_p46, npm_remote_versions_p46, npm_remote_packages_p46), which is what the two repeated namespace binds buy: the package's bind prunes its partition on a constant rather than through the join's equivalence. Every scan is a Seq Scan because a write target is seeded at 50 rows per the methodology, so a one-page partition is cheaper to scan than to index; the index question is not what this seed size answers, and the id predicate is a primary-key match at any cardinality. The four Trigger for constraint lines are the row's own child-side FK checks (attachment, namespace, version, blob), none of which this MR's EXISTS introduces. No anomalies.

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

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)) AND (EXISTS (
           SELECT npm_remote_versions.id AS "npm_remote_versions.id"
           FROM public.npm_remote_versions
                INNER JOIN public.npm_remote_packages ON ((npm_remote_packages.id = npm_remote_versions.npm_remote_package_id) AND (npm_remote_packages.namespace_id = npm_remote_versions.namespace_id))
           WHERE ((((npm_remote_versions.namespace_id = $3::uuid) AND (npm_remote_versions.id = npm_remote_files.npm_remote_version_id)) AND (npm_remote_versions.soft_deleted_at IS NULL)) AND (npm_remote_packages.namespace_id = $4::uuid)) AND (npm_remote_packages.soft_deleted_at IS NULL)
      ));

Bound args: [ac966497-7c26-46fd-b5f4-ac7a208fed45, b204b133-cb43-4b47-aba2-836767e9b569, ac966497-7c26-46fd-b5f4-ac7a208fed45, ac966497-7c26-46fd-b5f4-ac7a208fed45]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_remote_files  (cost=0.00..3.80 rows=0 width=0) (actual time=0.070..0.070 rows=0 loops=1)
   Update on npm_remote_files_p46 npm_remote_files_1
   Buffers: shared hit=10 read=2
   ->  Nested Loop Semi Join  (cost=0.00..3.80 rows=1 width=38) (actual time=0.007..0.009 rows=1 loops=1)
         Join Filter: (npm_remote_versions.id = npm_remote_files_1.npm_remote_version_id)
         Buffers: shared hit=3
         ->  Seq Scan on npm_remote_files_p46 npm_remote_files_1  (cost=0.00..1.75 rows=1 width=26) (actual time=0.003..0.004 rows=1 loops=1)
               Filter: ((soft_deleted_at IS NULL) AND (id = 'b204b133-cb43-4b47-aba2-836767e9b569'::uuid) AND (namespace_id = 'ac966497-7c26-46fd-b5f4-ac7a208fed45'::uuid))
               Rows Removed by Filter: 49
               Buffers: shared hit=1
         ->  Nested Loop  (cost=0.00..2.04 rows=1 width=36) (actual time=0.004..0.004 rows=1 loops=1)
               Join Filter: (npm_remote_packages.id = npm_remote_versions.npm_remote_package_id)
               Buffers: shared hit=2
               ->  Seq Scan on npm_remote_versions_p46 npm_remote_versions  (cost=0.00..1.01 rows=1 width=58) (actual time=0.002..0.002 rows=1 loops=1)
                     Filter: ((soft_deleted_at IS NULL) AND (namespace_id = 'ac966497-7c26-46fd-b5f4-ac7a208fed45'::uuid))
                     Buffers: shared hit=1
               ->  Seq Scan on npm_remote_packages_p46 npm_remote_packages  (cost=0.00..1.01 rows=1 width=42) (actual time=0.001..0.001 rows=1 loops=1)
                     Filter: ((soft_deleted_at IS NULL) AND (namespace_id = 'ac966497-7c26-46fd-b5f4-ac7a208fed45'::uuid))
                     Buffers: shared hit=1
 Planning:
   Buffers: shared hit=885
 Planning Time: 1.612 ms
 Trigger for constraint fk_npm_remote_files_blob_storage_attachment_id_bsa on npm_remote_files_p46: time=0.493 calls=1
 Trigger for constraint fk_npm_remote_files_namespace_id_namespaces on npm_remote_files_p46: time=0.162 calls=1
 Trigger for constraint fk_npm_remote_files_npm_remote_version_id on npm_remote_files_p46: time=0.053 calls=1
 Trigger for constraint fk_npm_remote_files_ns_id_blob_sha256_blobs on npm_remote_files_p46: time=0.408 calls=1
 Execution Time: 1.290 ms

Timings: planning 1.612ms, execution 1.290ms, total 2.902ms.

datastore.NpmRemoteMetadataFileStore.BumpUpstreamCheckedAt

Summary: The plan matches the method's intent. The correlated EXISTS becomes a Nested Loop against npm_remote_packages, and both partitioned tables prune to one partition of 64 (npm_remote_metadata_files_p01, npm_remote_packages_p01). Scans are Seq Scans for the same seed-size reason as the tarball twin, at 50 rows in a one-page partition. No anomalies.

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

The (namespace_id, npm_remote_package_id, kind) unique index means the 49 sibling rows need 49 more packages rather than 49 more rows under one, so the package count tracks the document count here.

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)) AND (EXISTS (
           SELECT npm_remote_packages.id AS "npm_remote_packages.id"
           FROM public.npm_remote_packages
           WHERE ((npm_remote_packages.namespace_id = $3::uuid) AND (npm_remote_packages.id = npm_remote_metadata_files.npm_remote_package_id)) AND (npm_remote_packages.soft_deleted_at IS NULL)
      ));

Bound args: [4de59e41-1a7f-43d3-9fa2-97ee164366c7, 28ec98df-bef0-4a7a-83ab-bd67b9144d97, 4de59e41-1a7f-43d3-9fa2-97ee164366c7]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Update on npm_remote_metadata_files  (cost=0.00..3.51 rows=0 width=0) (actual time=0.035..0.035 rows=0 loops=1)
   Update on npm_remote_metadata_files_p01 npm_remote_metadata_files_1
   Buffers: shared hit=8
   ->  Nested Loop  (cost=0.00..3.51 rows=1 width=28) (actual time=0.005..0.006 rows=1 loops=1)
         Buffers: shared hit=2
         ->  Seq Scan on npm_remote_metadata_files_p01 npm_remote_metadata_files_1  (cost=0.00..1.75 rows=1 width=26) (actual time=0.002..0.004 rows=1 loops=1)
               Filter: ((namespace_id = '4de59e41-1a7f-43d3-9fa2-97ee164366c7'::uuid) AND (id = '28ec98df-bef0-4a7a-83ab-bd67b9144d97'::uuid))
               Rows Removed by Filter: 49
               Buffers: shared hit=1
         ->  Seq Scan on npm_remote_packages_p01 npm_remote_packages  (cost=0.00..1.75 rows=1 width=26) (actual time=0.002..0.002 rows=1 loops=1)
               Filter: ((soft_deleted_at IS NULL) AND (namespace_id = '4de59e41-1a7f-43d3-9fa2-97ee164366c7'::uuid) AND (npm_remote_metadata_files_1.npm_remote_package_id = id))
               Buffers: shared hit=1
 Planning:
   Buffers: shared hit=362 read=1
 Planning Time: 0.833 ms
 Trigger for constraint fk_npm_remote_metadata_files_blob_storage_attachment_id_bsa on npm_remote_metadata_files_p01: time=0.774 calls=1
 Trigger for constraint fk_npm_remote_metadata_files_namespace_id_namespaces on npm_remote_metadata_files_p01: time=0.008 calls=1
 Trigger for constraint fk_npm_remote_metadata_files_npm_remote_package_id on npm_remote_metadata_files_p01: time=0.051 calls=1
 Trigger for constraint fk_npm_remote_metadata_files_ns_id_blob_sha256_blobs on npm_remote_metadata_files_p01: time=0.976 calls=1
 Execution Time: 1.883 ms

Timings: planning 0.833ms, execution 1.883ms, total 2.716ms.

Related to #540 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports