feat(npm): the ordered upstream list read 1/2 (S31 plan: 4/19)

The first of Step 4's two fixed-count reads: the ordered upstream list a virtual npm resolution loads its composition from. Neither read is wired yet. Step 11 is the first caller, so nothing here is reachable in production on merge.

One statement returns the whole list. It drives from npm_virtual_repository_upstreams, inner-joins repositories for the upstream's own name, kind and tombstone, and left-joins both npm format children so a remote position's cache windows and health verdict arrive as columns rather than a statement, and a hosted position carries the npm_repositories.id the batched hosted reads will key on. npm_packages references that table and not repositories, so without the third join a later step has nothing to batch against. Each child's namespace equality sits in its ON clause, not the WHERE, where it would make the join inner and drop every position whose child row is absent.

The stack

Merge in order. Part 2 is gated behind this MR.

#	MR	Branch	Contents	LOC
1/2	this MR	dmeshcharakou/s31-npm-virtual-step-4a	the ordered upstream list read	1221
2/2	!2165 (merged)	dmeshcharakou/s31-npm-virtual-step-4b	the batched rules read, plus the two assertions that need both reads	864

!2165 (merged) is stacked on this branch and predates both review-fix commits here, so it needs a rebase onto this branch's new head before it is read again. Its LOC is its size at its current head, re-derived rather than carried forward.

The credential omission

Five columns of the remote binding and no more: url and tmp_plaintext_auth_token are omitted, which is the control that keeps a listed position from materializing an upstream bearer whether or not it wins. It is asserted on the rendered SQL (TestNpmVirtualUpstreamListStmt_ProjectsNoCredential) and again on the whole returned field set (TestNpmVirtualUpstreamRowsProjectNoCredential), so a field added to either type fails rather than silently widening the projection. The integration fixture deliberately seeds a bearer on the row a listed position resolves through, so the omission is a choice over present data rather than an absence of data.

What the list reports rather than drops

A position whose child row is missing, or whose repositories.kind is neither hosted nor remote, is reported rather than dropped: a caller has to fail it closed and record the exclusion, and a swallowed row leaves the audit stream nothing to record. Both child references mean "no such row was found" when nil, never "not an npm repository". Nothing in the schema ties npm_repositories to repositories.format, so the two can disagree, and the suite seeds exactly that row on each join. Every reading each nil half admits now has a subtest, the cross-format upstream included.

Soft-deleted upstreams are excluded as the list loads, before rule evaluation and before any health read. A soft-deleted virtual repository is a different matter: format, kind and parent liveness are the finder's gate, so this read is deliberately indifferent to the virtual parent's tombstone, and a subtest pins that rather than leaving the division of labour to the doc comment.

The bound, and why it clips

The bound sits at twice ADR-004's 20-upstream cap, so it cannot truncate a legal list. ADR-004 also requires that cap enforced synchronously at write time with a 422; no write path for npm_virtual_repository_upstreams exists yet, so a stored list can exceed it through direct SQL only.

The statement selects one row past the bound and the caller serves the first bound's worth. That extra row is what separates "the stored list has exactly the bound's worth" from "it has more": under a plain LIMIT, both return the same row count, and the summary then fires on a list that lost nothing. listNpmRemotesStmt in npm_remote_repositories.go reads one past its own ceiling for the same reason.

Past the bound the read clips the lowest-priority positions and emits a one-line WARN naming the namespace, the virtual repository and the count served. A clipped position resolves exactly as an absent one does, so without that line the only trace would be a 404 for a package the clipped upstreams hold, which npm caches as a hard E404.

returned_count is the positions served, which is always the bound when the line fires. It is not a measure of how many were dropped: a statement that stops at the bound cannot know the stored total, and buying that number costs a second COUNT on every resolution to describe an illegal state. The line fires once per call and this read runs once per resolution request, so one oversized list warns on every request against it. That is deliberately not the rate of logSkippedRemoteRows, which the summary is otherwise shaped after and which runs on the health sweep's interval; the sidecar states the real rate and why it is accepted rather than borrowing the sweep's reasoning.

All three bound directions are covered: a list at ADR-004's cap comes back whole and silent, a list at exactly the bound comes back whole and silent, and a list one past the bound is clipped from its lowest-priority end and says so. The middle one is the case a == predicate gets wrong; reverting the predicate and the LIMIT together fails it.

Acceptance criteria

The spec's ### Resolution list has been renumbered since the plan was written. The plan's Step 4 acceptance cites #14 (soft delete), #15 (query count) and #16 (closed) (health); those are now #15, #16 (closed) and #17 (closed). This table uses the spec's current numbering. Stated rather than silently corrected, because a step MR must not edit the plan.

Spec AC (current numbering)	Covered here	Test
#15 a soft-deleted upstream is skipped	store half	..._WhatTheListLeavesOut/excludes a soft-deleted upstream of either kind ...
#14 an upstream of an unexpected kind fails closed	store half. The read reports the row so a caller can fail it closed	..._UpstreamsByVirtualRepository/reports an upstream whose kind is neither hosted nor remote ...
#16 (closed) per-read query count independent of upstream count	part 2. The assertion needs both reads	none here. !2165 (merged) carries it as ..._StatementCountIsIndependentOfUpstreamCount/two upstreams and /twenty upstreams, asserting one statement per read at both list sizes
#17 (closed) the health status costs no statement	not this step's. Asserted at Step 11 over a composed resolution: a store-level statement count passes while the composed read fails, so claiming it here would be the assertion that hides the defect	none

Query plan

TestNpmVirtualUpstreamList_PlanShape EXPLAINs the rendered statement and asserts single-partition pruning on all four joined tables. It lands here rather than in part 2, because this is the MR that introduces the four-table join and every comparable read in this package ships its plan assertion in its own MR. Under simple query protocol there is no server-side plan cache, so this replans per request, which is what makes the assertion worth having rather than a formality.

The two children are the interesting half: their namespace literal has to stay in the ON clause or the join turns inner, so their pruning is the one part not backed by a WHERE bind. The test passes, so it prunes.

What that test does not pin is index choice or the ordering, and the ## Database Review Evidence section below is where those are measured instead of asserted. Two things it shows that a pruning assertion cannot. The planner reaches the rows through unique_nvru_ns_id_repository_id_upstream_repository_id rather than the position index, both of which lead on the same two equality columns. And the ORDER BY is therefore not index-provided: the plan carries a Sort, which is why this suite does not assert the absence of one. That Sort is bounded by the same statement that creates it, since LIMIT caps its input at one row past the bound.

Files-list deviation from the plan

The plan's Step 4 Files list names three files, all in internal/datastore, and its accepted-smell section says a reviewer of this step "should not go looking for a multi-package diff the plan never supplies". Three files here fall outside that list, one of them in a second package:

• internal/datastore/npm_virtual_resolution_test.go. Implied by the Step 4 Tests list, which asks for the projection assertion on the rendered SQL, but absent from the Files list.
• internal/metrics/cardinality.go. This is the second package. One comment line, which now removes the inlined query-name count rather than raising it. Nothing enforces that number, main already disagreed with the value this MR was bumping, and three open MRs (!2135 (merged), !2136 (merged), !2144 (merged)) add sixteen names between them without touching the line. The file pointers stay; the rotting count goes.
• internal/datastore/npm_virtual_resolution_contract.md. Sidecar prose for what the comment caps cannot hold, following the existing npm_virtual_remote_lookup_join_order.md convention in the same package.

query_names.go insertion point, which the plan asks each step MR to name because Steps 5, 6 and 9 add adjacent groups to the same block: the new npm_virtual_repository_upstreams group goes between the npm_virtual_repositories group and the repositories group, at query_names.go:568. !2165 (merged) appends its npm_virtual_upstream_rules group directly after it. Checked against the three open MRs that touch this file: !2135 (merged) inserts near line 42, !2136 (merged) near line 180 and !2144 (merged) near line 583, so none of them shares a hunk with this one.

On the plan's single-package ruling for Step 4. The accepted-smell section lists Step 4 among four steps that are "single-package and carry no smell at all". The cardinality.go hunk makes that no longer true of this MR, so a reviewer of Steps 5, 8 or 13 should read that ruling as describing the plan's intent rather than this diff.

Size

1221 reviewable LOC, above the 500 the development model asks about. By file group: 254 source (npm_virtual_resolution.go 246, query_names.go 4, cardinality.go 4), 861 test, 106 sidecar doc. The plan applies the ceiling to reviewable source, where 254 is comfortably under it. Splitting further would separate this read from the assertions that pin its statement shape, its plan and its bound, which is the pairing under review, and the step is already split in two, which is what this stack is.

e2e scenarios

None. The plan assigns the whole e2e catalog to Step 19 and states that every other step's e2e statement is "none, because ...". This MR adds one datastore read with no caller and no route, so no docs/testing/ scenario is added or affected.

Not done here

• Step 4's row in the plan's Status table is filled by !2177 (merged), not by this MR. Guardrail 4 forbids a step MR from editing the plan file, so the row recording !2164 (merged) and !2165 (merged) goes in that dedicated docs(plans) MR. It does not gate this one: the two can merge in either order.
• No counter on the clip, so the only trace is the log line and no alert can group on it. Adding a metric is outside this read's scope.
• The two new log keys are not in the ## Log fields table in docs/dev/observability.md (namespace_id is already a baseline field there, so only npm_virtual_repository_id and returned_count are new). That table admits a key "when a dashboard or an alert is expected to group on them", and no alert groups on this clip, which is the bullet above. It also declares itself seeded rather than complete and already names one unlisted key that meets its bar. So this is a deliberate omission against the table's own criterion, not a gap: the clip summary is a go-look-now diagnostic, and the sidecar carries its rate and its meaning.

Database Review Evidence

Query mode only. This MR adds no migration, so migration mode did not run.

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17.11 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.UpstreamsByVirtualRepository	Limit	unique_nvru_ns_id_repository_id_upstream_repository_id	20 / 20	57.41	0.161ms	31 / 0	1/64, 1/64, 1/64, 1/64

No partition fan-out: every one of the four joined tables pruned to a single partition, so the [!WARNING] block the skill reserves for fan-out is omitted. The plan predicted this evidence "is expected to raise the partition fan-out anomalies rather than come back clean". It does not, and the reason is worth stating rather than leaving as a silent disagreement: the fan-out risk the plan names belongs to a read keyed with = ANY(...) on a non-unique column, which is part 2's batched rules read, and this list read binds namespace_id as an equality on all four relations, including the two children whose namespace literal sits in their ON clause.

Query notes:

• datastore.UpstreamsByVirtualRepository: the ORDER BY position is not index-provided. The planner reaches the rows through unique_nvru_ns_id_repository_id_upstream_repository_id and sorts, rather than through unique_nvru_ns_id_repository_id_position, whose order would have served directly; both indexes lead on (namespace_id, npm_virtual_repository_id), so both satisfy the equality predicates and the planner is free to pick either. Measured, not inferred: forcing an order-preserving plan (enable_hashjoin=off, enable_mergejoin=off) does switch to the position index and drops the Sort node, at 0.122ms against the default plan's 0.161ms. The fan-out-free pruning is identical either way. This stands rather than being fixed, because the Sort's input is bounded by the same statement that creates it: LIMIT caps it at one row past the bound, so it can never grow with the table. It is recorded because the sort is invisible in the CI plan assertion, which pins pruning only.

datastore.UpstreamsByVirtualRepository

Summary: The plan matches the method's intent. An Index Scan on npm_virtual_repository_upstreams finds the one virtual repository's 20 positions inside a single partition, the three joined tables each prune to one partition of 64, and plan and actual rows agree exactly at every node. The one thing to note is that the ordering rides a Sort rather than the position index, which costs nothing here because the statement's own LIMIT bounds the sort input. No anomalies.

Seed shape: namespaces=1, repositories=270, npm_repositories=270, npm_remote_repositories=10, npm_virtual_repositories=250, npm_virtual_repository_upstreams=5000

250 virtual repositories each listing all 20 upstreams is what puts 5000 rows in one partition while keeping the queried list at ADR-004's cap, so the index has to discriminate one virtual repository out of 250 rather than scan a table whose every row matches. Ten of the twenty upstreams are remote-kind and carry an npm_remote_repositories binding, and all 270 repositories carry an npm_repositories row, so neither LEFT JOIN prunes for the wrong reason.

Rendered SQL:

SELECT npm_virtual_repository_upstreams.id AS "npm_virtual_repository_upstreams.id",
     npm_virtual_repository_upstreams.upstream_repository_id AS "npm_virtual_repository_upstreams.upstream_repository_id",
     npm_virtual_repository_upstreams.position AS "npm_virtual_repository_upstreams.position",
     repositories.name AS "repositories.name",
     repositories.kind AS "repositories.kind",
     npm_repositories.id AS "npm_repositories.id",
     npm_remote_repositories.id AS "npm_remote_repositories.id",
     npm_remote_repositories.repository_id AS "npm_remote_repositories.repository_id",
     npm_remote_repositories.cache_validity_hours AS "npm_remote_repositories.cache_validity_hours",
     npm_remote_repositories.metadata_cache_validity_hours AS "npm_remote_repositories.metadata_cache_validity_hours",
     npm_remote_repositories.last_health_status AS "npm_remote_repositories.last_health_status"
FROM public.npm_virtual_repository_upstreams
     INNER JOIN public.repositories ON ((repositories.namespace_id = npm_virtual_repository_upstreams.namespace_id) AND (repositories.id = npm_virtual_repository_upstreams.upstream_repository_id))
     LEFT JOIN public.npm_remote_repositories ON ((npm_remote_repositories.namespace_id = $1::uuid) AND (npm_remote_repositories.repository_id = npm_virtual_repository_upstreams.upstream_repository_id))
     LEFT JOIN public.npm_repositories ON ((npm_repositories.namespace_id = $2::uuid) AND (npm_repositories.repository_id = npm_virtual_repository_upstreams.upstream_repository_id))
WHERE (((npm_virtual_repository_upstreams.namespace_id = $3::uuid) AND (npm_virtual_repository_upstreams.npm_virtual_repository_id = $4::uuid)) AND (repositories.namespace_id = $5::uuid)) AND (repositories.soft_deleted_at IS NULL)
ORDER BY npm_virtual_repository_upstreams.position ASC
LIMIT $6;

Bound args: [<namespace_id>, <namespace_id>, <namespace_id>, <npm_virtual_repositories.id>, <namespace_id>, 41] — the namespace is bound at four positions (both children's ON clauses, the driving table's WHERE, and repositories' own repeated WHERE predicate), and 41 is the npmVirtualUpstreamListLimit + 1 that jet renders as a placeholder.

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

 Limit  (cost=57.36..57.41 rows=20 width=117) (actual time=0.124..0.126 rows=20 loops=1)
   Buffers: shared hit=31
   ->  Sort  (cost=57.36..57.41 rows=20 width=117) (actual time=0.123..0.125 rows=20 loops=1)
         Sort Key: npm_virtual_repository_upstreams."position"
         Sort Method: quicksort  Memory: 28kB
         Buffers: shared hit=31
         ->  Hash Left Join  (cost=47.28..56.93 rows=20 width=117) (actual time=0.087..0.118 rows=20 loops=1)
               Hash Cond: (npm_virtual_repository_upstreams.upstream_repository_id = npm_remote_repositories.repository_id)
               Buffers: shared hit=31
               ->  Hash Join  (cost=46.03..55.61 rows=20 width=79) (actual time=0.079..0.107 rows=20 loops=1)
                     Hash Cond: (repositories.id = npm_virtual_repository_upstreams.upstream_repository_id)
                     Buffers: shared hit=30
                     ->  Seq Scan on repositories_p24 repositories  (cost=0.00..8.38 rows=270 width=59) (actual time=0.005..0.023 rows=270 loops=1)
                           Filter: ((soft_deleted_at IS NULL) AND (namespace_id = 'a66bd91d-3780-4f31-a0ff-b378b1702a93'::uuid))
                           Buffers: shared hit=5
                     ->  Hash  (cost=45.78..45.78 rows=20 width=68) (actual time=0.068..0.068 rows=20 loops=1)
                           Buckets: 1024  Batches: 1  Memory Usage: 10kB
                           Buffers: shared hit=25
                           ->  Hash Right Join  (cost=38.68..45.78 rows=20 width=68) (actual time=0.036..0.063 rows=20 loops=1)
                                 Hash Cond: (npm_repositories.repository_id = npm_virtual_repository_upstreams.upstream_repository_id)
                                 Buffers: shared hit=25
                                 ->  Seq Scan on npm_repositories_p24 npm_repositories  (cost=0.00..6.38 rows=270 width=32) (actual time=0.002..0.017 rows=270 loops=1)
                                       Filter: (namespace_id = 'a66bd91d-3780-4f31-a0ff-b378b1702a93'::uuid)
                                       Buffers: shared hit=3
                                 ->  Hash  (cost=38.43..38.43 rows=20 width=52) (actual time=0.029..0.030 rows=20 loops=1)
                                       Buckets: 1024  Batches: 1  Memory Usage: 10kB
                                       Buffers: shared hit=22
                                       ->  Index Scan using npm_virtual_repository_upstr_namespace_id_npm_virtual_rep_idx24 on npm_virtual_repository_upstreams_p24 npm_virtual_repository_upstreams  (cost=0.28..38.43 rows=20 width=52) (actual time=0.014..0.025 rows=20 loops=1)
                                             Index Cond: ((namespace_id = 'a66bd91d-3780-4f31-a0ff-b378b1702a93'::uuid) AND (npm_virtual_repository_id = 'cfe3d548-41f4-4ac9-817a-87fcf54685fe'::uuid))
                                             Buffers: shared hit=22
               ->  Hash  (cost=1.12..1.12 rows=10 width=38) (actual time=0.006..0.007 rows=10 loops=1)
                     Buckets: 1024  Batches: 1  Memory Usage: 9kB
                     Buffers: shared hit=1
                     ->  Seq Scan on npm_remote_repositories_p24 npm_remote_repositories  (cost=0.00..1.12 rows=10 width=38) (actual time=0.003..0.004 rows=10 loops=1)
                           Filter: (namespace_id = 'a66bd91d-3780-4f31-a0ff-b378b1702a93'::uuid)
                           Buffers: shared hit=1
 Planning:
   Buffers: shared hit=1286 read=1
 Planning Time: 2.649 ms
 Execution Time: 0.161 ms

Timings: planning 2.649ms, execution 0.161ms, total 2.810ms.

Related to #885 (closed)