feat(npm): resolver virtual tier (S31 plan: 9/19)

Dzmitry (Dima) Meshcharakourequested to merge
dmeshcharakou/s31-npm-virtual-step-9 into main

What this changes

npm.Resolver walked two tiers and then a third for kind=2 only, so a kind=1 request reached the dispatcher with nothing but its parent row. The virtual read path in internal/virtual has been merged since S13 and has had no caller, and Step 10 dispatches on the row this tier produces.

Resolution gains a Virtual field and the resolver a VirtualRepositoryFinder, mirroring Remote and RemoteRepositoryFinder arm for arm:

Kind Third tier Field populated
kind=0 hosted none neither
kind=1 virtual VirtualRepositoryFinder.FindVirtualByRepositoryID Virtual
kind=2 remote RemoteRepositoryFinder.FindRemoteByRepositoryID Remote

The finder is internal/datastore/npm_virtual_repository_finder.go, read-only, keyed on the repositories.id tier 2 resolved rather than on the name a second time. It composes the shared activeParentPredicate with a new npmVirtualParent spec, so the read gate and the write gate cannot drift about what an active npm virtual parent is. NewResolver takes a fourth required finder and panics on nil, which is why cmd/artifact-registry/wire_npm.go and the four npm.NewResolver call-site test files are in this step: without them the tree does not compile.

The file is deliberately not named npm_virtual_repositories.go. S17 Phase 6 Step 37 creates that file for the child-row create and delete, both steps are gated on Step 1, so both can be in flight at once and sharing the name would conflict across the whole body of whichever landed second. The issue records the same reasoning.

The kind=1 answer moves from 501 to 404, and no repository can observe it

Before this MR a kind=1 npm repository resolved successfully, reached dispatchByKind, and got the interim 501 not_implemented. After it, a kind=1 parent without its npm_virtual_repositories child row fails the resolve as ErrRepositoryNotFound, so the middleware answers 404 repository_not_found plus one Info record.

Nothing can reach that transition today, and the reason is worth stating rather than assuming:

  • repositoryCreateKindGate refuses every kind=virtual create with ErrRepositoryCreateVirtualKind (internal/datastore/repositories.go:1695).
  • The management API rejects kind: "virtual" with a 422 before that, and its predecessor accepted only hosted.
  • RepositoryStore.Update never writes kind.
  • No production code writes npm_virtual_repositories at all; this finder is its only reader.

So the shape this tier detects is reachable only by direct insert, or by a parent tombstoned between the two reads, which activeParentPredicate's soft-delete arm then rejects. During a rolling deploy the same request would get 501 from an old pod and 404 from a new one, unreachable for the same reason. The tier goes live when the virtual create path merges, which is Step 37 in a different plan and stack, so the merge order is recorded here rather than implied by a branch.

resolver.md states the condition and the event on both third-tier bullets, rather than the present tense, so it reads correctly on either side of that merge.

Five files beyond the issue's Files list

Each is justified, and each is named in a commit body already:

File Why
internal/format/npm/middleware.go resolveErrorCode needs nil_virtual_resolution as a distinct closed-set value, so the two contract breaches stay separable without matching message text
internal/format/npm/resolver.md the tier walk and the error contract outgrew the comment caps; three npm sidecars already exist
internal/datastore/npm_virtual_repository_finder_guards_test.go the argument, zero-value and construction guards, which touch no database and so carry no build tag
cmd/artifact-registry/wire_npm_test.go the adapter's error translation, mirroring the remote adapter's test
internal/datastore/repository_parent_gate_integration_test.go added in the review round, below

No drift against the spec: the npm_virtual_repositories section of S31 specifies exactly the three-column table and the (namespace_id, repository_id) unique index this finder keys on, and its "a virtual repository stores no cached content and no credentials" holds for NpmVirtualRepositoryResolution, which carries two ids and nothing else.

Governing ADRs

  • ADR-007. Soft deletion sits on the parent so every kind shares one deletion semantics, which is what the finder's parent gate applies. Its npm_virtual_repositories section specifies the (namespace_id, repository_id) unique index for looking a virtual repository up by its parent reference, which is exactly the key.
  • ADR-001. Namespace scoping is pinned on both sides of the join, the child from its own predicate and repositories from inside activeParentPredicate. A cross-namespace subtest asserts it.
  • ADR-023. Locked paths respected: datastore file in internal/datastore/, interface in internal/format/npm/, adapter in cmd/artifact-registry/, no raw SQL in production code, no cross-format import.

./scripts/adr-freshness.sh reports the mirror stale by three upstream commits, ba269c554, 9a56bb4d5 and 3640b6d88, all touching 007_database_schema.md. They cover the size_bytes decrement at the delete, a maven_packages index for tombstone discovery, and artifact-level walks reading the namespace shadow, none of which this change goes near; ADR-007's npm_virtual_repositories shape and its (namespace_id, repository_id) unique index match the finder's key as the mirror already has them. One internal ADR never mirrors locally, so a reviewer with handbook access should confirm against it.

Queries

One statement, npm_virtual_repositories_select_by_repository_id, unique in the catalog and instrumented through the standard instrumentQuery seam that TestEveryStatementIsInstrumented enforces.

WHERE (namespace_id, repository_id) = ($1, $2) rides unique_nvr_ns_id_repository_id, so at most one row can match, and LIMIT(1) is a literal with no caller-fed provenance. Both partitioned sides carry HASH(namespace_id) as a constant, the child from its own predicate and repositories from inside the shared gate, so each prunes to one of 64 partitions.

TestNpmVirtualRepositoryFinder_PlanShape EXPLAINs the same constructor the store executes and asserts single-partition pruning on both sides, which is parity with the remote sibling's TestNpmRemoteRepositoryStore_FindRemoteByRepositoryID_PrunesToOnePartition. Neither asserts index usage, so no coverage was dropped in the mirroring.

Only qrm.ErrNoRows maps to ErrNotFound, so a database outage on this tier surfaces as a 500 rather than a cacheable 404. An integration subtest pins that with a cancelled context.

One number to watch, not caused here: internal/metrics/cardinality.go budgets labelName at 500, and the two catalogs now hold 473 and 12, so this MR moves the count to 485. Nothing counts either catalog against the budget and the audit sees only observed values, so the ceiling would be crossed in production rather than by a failing test. Raising it needs a docs/specs/S03-b-metrics.md amendment in the same change, since that table still records up to ~200 for name and the budget map's header cites it as the source. This branch tried the raise and reverted it; see the third round below.

Tests

  • TestResolver_Resolve_VirtualTierByKind: all three kinds as positive rows with call counts. The hosted and remote happy paths both assert virtualFinder.callCount == 0, so a tier that fired for the wrong kind fails rather than passing quietly.
  • All five remote-tier resolver tests have virtual twins, including the (nil, nil) contract breach, which fails closed on its own sentinel rather than building a nil-Virtual resolution.
  • The finder's four guards each have their own sentinel assertion, and the constructor has both the nil-panic and a positive "keeps the client" case, so the panic cannot be satisfied by a constructor that always panics. The zero value is pinned as a named error rather than a nil-client dereference, per the exported-zero-value rule in AGENTS.md.
  • The integration suite was diffed subtest by subtest against TestNpmRemoteRepositoryStore_FindRemoteByRepositoryID for dropped subtests, not only added ones. None dropped; the only unmirrored cases are the two auth-token ones, and this table has no token column. Three are added: the npm_repositories sibling row the create path writes, the absent child row, and a maven parent carrying an npm virtual binding.
  • Both non-virtual kind values get positive rejection cases rather than one standing for both, which is the enumerated-column rule.
  • The tier-miss records are asserted on level, exact message, tier, and both id fields. remoteTierMiss.msg is byte-identical to the literal it replaced, so existing operator log queries still match.

e2e scenarios

None added or affected. The plan assigns both docs/testing/e2e/npm.md and the mise run conformance:npm harness to Step 19, which it calls the plan's single owning step for the catalog, and this step changes no reachable npm wire-protocol behavior.

Reviewable LOC

1525 across 15 files, over the 500 guidance, so here is the split by group. Re-derived from git diff origin/main...HEAD --numstat at 9583fc228:

Group Added Removed Total
Production Go (5 files) 286 102 388
Tests (8 files) 854 78 932
Docs (2 sidecars) 205 0 205

The plan's "How the ceiling is applied" section applies the ceiling to reviewable source following the S15 measurement, under which 388 is well inside it. Test-to-source ratio on added lines is 2.99:1.

internal/metrics/cardinality.go is no longer in the diff. The second round raised its query-name budget and the third round reverted that, so the two commits cancel and the file's net change is empty.

The issue estimated Source ~230 and Test ~520. Source came in near it; tests ran over, mostly in the two integration suites and the by-kind matrix.

File overlap with open merge requests

git diff --name-only origin/main...HEAD checked against all 74 open MRs. Seven share a file. Only one shares a line:

MR Shared file Hunks
!2057 (merged) internal/format/npm/resolver.go theirs old 110-116; mine inserts at old 109. Adjacent; conflicted before the rebase, clean now
!2059 (merged), !2051 (merged), !2040 (merged), !2035 (merged), !2030 (merged) internal/datastore/query_names.go theirs 54, 185-200, 200-215, 312-319, 372-384, 405; mine inserts at 558. Disjoint
!1011 (closed) (Draft) cmd/artifact-registry/wire_npm.go, wire_npm_test.go theirs 223-229, 264-270, 300-306; mine 102, 351, 629, and 320. Disjoint

The resolver.go overlap was a real conflict, from main's movement rather than !2057 (merged)'s: DeliveryModeOverride merged into the tail of Resolution after this branch's old merge base, and this branch appends Virtual to the same position. The branch has since been rebased onto main, and the resolution is the one this section anticipated: Resolution carries both fields, they are independent, and neither comment refers to the other.

git merge-tree --write-tree origin/main HEAD now exits 0, so there is no conflict left to resolve and no merge order to respect against !2057 (merged), which also merges cleanly against both this branch and main. The rebase carried one conflict that was not textual: main added TestResolver_Resolve_CarriesDeliveryModeOverride, which calls npm.NewResolver with three finders, and this branch makes it four. That call site was fixed in the commit that introduces the fourth parameter, so no commit on the branch is left with a call that does not compile.

What the review round changed

Two commits after the implementation, both behaviour-preserving:

  • docs(npm): complete the resolver third-tier enumerations. The tier turned four comments into half-lists. Two were fixed when it landed; these are the other two. Resolver and NewResolver still named the remote row and the remote finder alone, and both blocks predate the comment caps and were not in the tier's own diff, so check-comment-caps.sh grandfathered them and nothing flagged them. Touching them takes each to the 3-line exported cap, which is where the (resolver.md) pointer earns its place. The same commit fixes the resolver.md framing described above, including a parity claim that borrowed a precedent whose create path does the opposite of what the sentence needed.
  • test(datastore): drive every production parent spec through the gate net. repository_parent_gate.go claims TestParentRepositoryIsActive drives every production spec against a row seeded from independent literals, which is what covers a transposed constant inside a parentFormat / parentKind conversion: the rendered SQL is identical for every pair and only the bound args differ. npmVirtualParent had no row, so the claim went false and the npm/virtual pair had no positive hit at the gate level. The maven row was an older gap in the same claim, building its own literal rather than binding mavenRemoteParent, so a transposition inside that declaration was never reached. All three production specs are now driven and the sentence is true as written.

Deferred, with reasons

  • activeParentPredicate's composer list. Its doc names "the npm remote reads" and this MR adds a composer it does not name. The drift is older than this MR, since the maven credential and details reads already compose it too. Fixing it in place would force all 14 lines of that block to the 3-line cap, so it still belongs in its own refactor MR. What this MR does add is internal/datastore/repository_parent_gate.md, which states both consumer shapes correctly, so the list a reader can reach is no longer the wrong one.
  • The third round trip itself. npm_virtual_repositories is three columns, all keys, and the statement's only product is the binding surrogate plus the fact that it exists; its parent gate re-establishes format, kind and soft-delete state that tier 2 already settled. Cost is zero today because kind=1 is unreachable, and the query is currently unconsumed as well as unreached: nothing outside the resolver's own nil-invariant check reads Resolution.Virtual, and dispatchByKind answers kind=1 with 501. Step 10 turns the row into an answer, and inherits a query already paid for rather than adding one. The alternative is to LEFT JOIN the binding into tier 2's statement, which is the rationale the plan's Step 4 uses for npm_repositories ("a third LEFT JOIN npm_repositories, which costs no statement"). Worth deciding deliberately, with a measurement, before virtual read traffic ships rather than inheriting it; the seam mirrors the shipped remote tier and uniformity across tiers has real value.

What the second review round changed

A /review-branch pass over the rebased branch returned no blocking findings. Security, reliability and API compatibility were clean. Everything it did raise was in prose the code ships with, or in this description, and all of it is fixed here:

  • docs(npm): name every reading the tier-3 not-found list leaves open. The tier 3 list in resolver.md announced itself as complete and then asserted the kind=2 miss "comes from a hard-deleted parent". Both third-tier reads compose activeParentPredicate, whose soft-delete arm rejects a parent tombstoned between tier 2 and tier 3, and RepositoryStore.SoftDelete has no kind gate and is reachable from DELETE /repositories/{id}, so the tombstone race is the cause actually reachable in production and it was the one omitted.
  • feat(npm): give the tier-miss records a structured tier field. Lifts the deferral above. logTierMiss takes a tierMissRecord pairing each pinned message with its tier value, so the two cannot drift, and emits tier beside the ids. The message text is unchanged, because it is the wire format an existing log query matches. The remote tier's comment came to the two-line function-body cap in the same pass: it still said "a second tier" and "the second query", the numbering the branch had already moved off.
  • docs(datastore): move the parent gate suite's reasoning to a sidecar. TestParentRepositoryIsActive's header called npm remote the gate's "only production caller today"; there are two, and npmVirtualParent is not a caller of the write gate at all. The _test.go cap is two lines, so the seeding rule and the transposition gap move to repository_parent_gate.md rather than being deleted. The refusal cases also stopped hand-rolling a parentSpec the positive case binds from production.
  • test(datastore): pin the virtual finder's index condition. The plan-shape test asserted partition pruning and stopped, so a predicate change that defeated unique_nvr_ns_id_repository_id and fell back to a partition-local sequential scan would still have passed. It now also requires an Index Cond binding both key columns, through the suite's existing idKeyedChildIndexCondRe and explainAnalyzeStmt.
  • chore(metrics): raise the query-name label budget. Took labelName from 500 to 800. Reverted in the third round below, so the branch leaves the budget where it found it. Both commits stay in the history, because the project merges rather than squashes.

Four commit messages were also reworded, since they reach main verbatim under merge commits. The feat(npm): resolver virtual tier body said "Three files the plan's Step 9 list does not name" and led with query_names.go, which the plan names at line 1988; two commits carried a bare refactor: against a history that scopes it; and the partition-pruning commit was scoped npm for a change that lands in internal/datastore.

One item is outside the diff: the AppSec reviewer session (6720773) posted "failed (dropped)" three minutes after starting and left no findings thread, so its silence is not a pass. It has been re-triggered.

What the third review round changed

A second /review-branch pass, over 1f476618d, returned no blocking findings. Security, reliability, performance and API compatibility were clean. It raised four warnings, none of which changes request behaviour, and they are answered here.

  • docs(npm): describe the tier-miss records the code emits. logTierMiss's doc comment points at resolver.md, and that file still described the helper as "called with the tier's own message constant" and never mentioned the tier field at all. The second round replaced the constants with tierMissRecord and added the field, so a reader following the pointer got a contract this branch had already replaced.
  • docs(datastore): correct the parent gate sidecar's two claims. The read-path bullet named npm_virtual_repository_finder.go alone, while activeParentPredicate has seven composition sites across six files, and the sibling bullet's outright count ("There are two today") made the asymmetry read as a complete list. That left npmRemoteParent and mavenRemoteParent looking write-path only, which activeParentPredicate's own doc comment contradicts. The same commit fixes the transposition paragraph, which read as though the maven-remote and npm-virtual rows compensate for the npm remote pair's blind spot; each row only catches a transposition in its own declaration.
  • test: fix two stale references in the virtual tier suites. The remote tier-miss assertions still said "the warn" about a record the same block pins at slog.LevelInfo, and the plan-shape test credited the ANALYZE with pinning enable_seqscan=off, which is explainAnalyzeStmt's doing.
  • revert(metrics): restore the query-name label budget to 500. The raise put the entry four times above docs/specs/S03-b-metrics.md, whose table records up to ~200 for name, while the budget map's own header claims the counts come from that spec and every other entry matches its row. The branch needs no headroom, adding one name against 485 declared and a 500 ceiling, so the raise goes back out and belongs in a spec amendment with the growth argument attached.

The fourth item the pass raised was the ## Database Review Evidence section the plan requires for this step in query mode. It is present, at the end of this description.

Why this closes the issue

Every item of the issue's ## Acceptance has evidence here: a kind=1 repository resolves with a populated Virtual field (TestResolver_Resolve_VirtualTierByKind), kind=0 and kind=2 leave it nil (same test, with call counts), and a kind=1 repository with no child row fails rather than resolving with a nil field (TestResolver_Resolve_VirtualBindingMissing and the finder's own absent-child subtest).

One reading to flag, because the wording invites it. The acceptance says such a repository "fails loudly". What ships is an existence-hiding 404 plus one Info record, not a loud failure. The clause's own contrast is with "resolving with a nil field", and on that reading it is satisfied; Warn was rejected because a concurrent delete produces the identical record, so it would alert on a race that works as designed, and resolver.md records that. If you read "loudly" as requiring more than an Info record, say so and this becomes Related to.

The plan row lands elsewhere

Step MRs do not edit the plan file, so the Status table row for step 9 goes in a docs(plans) MR rather than here.

Closes #890 (closed)

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 the 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.FindVirtualByRepositoryID Limit npm_virtual_repositories_p43_namespace_id_repository_id_idx (partition-local unique_nvr_ns_id_repository_id), repositories_p43_pkey (partition-local pk_repositories) 1 / 1 16.62 0.011ms 6 / 0 1/64, 1/64
datastore.FindVirtualByRepositoryID

Summary: Plan matches the method's intent. A Limit over a Nested Loop: the (namespace_id, repository_id) bind drives an Index Scan on the partition-local instance of the unique index unique_nvr_ns_id_repository_id, and the joined repositories row is fetched by primary key with the shared parent gate (format = 2, kind = 1, soft_deleted_at IS NULL) applied as a Filter. Both hash-partitioned sides prune to one partition of 64 (_p43 on each), the estimate matches reality at 1 / 1 row, and execution is 0.021 ms against 5000 seeded rows per side with read=0 buffers. No anomalies.

One property worth naming, which this statement does not cause and cannot avoid: planning dominates execution here, because the chain is two tables of 64 partitions each and the pgx query mode is QueryExecModeSimpleProtocol (labkit's default, which PgBouncer transaction pooling requires), so pruning is planned on every execution rather than once. Read the absolute planning number below as orientation, not as a production cost: it comes from a single run in a laptop container, and TestContainerRemoteCacheStore_ChainWidth documents that repeated sampling of this shape does not converge.

Seed shape: namespaces=1, repositories=5000, npm_virtual_repositories=5000

All seeded rows share one namespace_id, so both hash-partitioned tables put every row in a single partition. The 5000 repositories rows are all npm-format, kind=virtual, and active, which is the production shape for a row this statement is meant to find and the least flattering one for the read: the parent gate discriminates nothing, so the index carries the whole lookup. Each repositories row carries one npm_virtual_repositories child. The bound repository_id is taken from the middle of the seeded range.

Rendered SQL:

SELECT npm_virtual_repositories.id AS "npm_virtual_repositories.id",
     npm_virtual_repositories.repository_id AS "npm_virtual_repositories.repository_id"
FROM public.npm_virtual_repositories
     INNER JOIN public.repositories ON ((repositories.id = npm_virtual_repositories.repository_id) AND (repositories.namespace_id = npm_virtual_repositories.namespace_id))
WHERE ((npm_virtual_repositories.namespace_id = $1::uuid) AND (npm_virtual_repositories.repository_id = $2::uuid)) AND ((((repositories.namespace_id = $3::uuid) AND (repositories.format = $4)) AND (repositories.kind = $5)) AND (repositories.soft_deleted_at IS NULL))
LIMIT $6;

Bound args: [e3b6c55d-a0e1-434f-b0b3-158af2deab59, 5dc8dbf2-e0e7-4dde-b508-c06c845de80a, e3b6c55d-a0e1-434f-b0b3-158af2deab59, 2, 1, 1]

$4 is RepositoryFormatNpm and $5 is RepositoryKindVirtual, both supplied by the shared parent gate rather than by the caller. $6 is the LIMIT(1) literal, which go-jet renders as a positional placeholder.

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.56..16.62 rows=1 width=32) (actual time=0.010..0.011 rows=1 loops=1)
  Buffers: shared hit=6
  ->  Nested Loop  (cost=0.56..16.62 rows=1 width=32) (actual time=0.010..0.010 rows=1 loops=1)
        Buffers: shared hit=6
        ->  Index Scan using npm_virtual_repositories_p43_namespace_id_repository_id_idx on npm_virtual_repositories_p43 npm_virtual_repositories  (cost=0.28..8.30 rows=1 width=48) (actual time=0.005..0.005 rows=1 loops=1)
              Index Cond: ((namespace_id = 'e3b6c55d-a0e1-434f-b0b3-158af2deab59'::uuid) AND (repository_id = '5dc8dbf2-e0e7-4dde-b508-c06c845de80a'::uuid))
              Buffers: shared hit=3
        ->  Index Scan using repositories_p43_pkey on repositories_p43 repositories  (cost=0.28..8.31 rows=1 width=32) (actual time=0.004..0.004 rows=1 loops=1)
              Index Cond: ((id = '5dc8dbf2-e0e7-4dde-b508-c06c845de80a'::uuid) AND (namespace_id = 'e3b6c55d-a0e1-434f-b0b3-158af2deab59'::uuid))
              Filter: ((soft_deleted_at IS NULL) AND (format = '2'::smallint) AND (kind = '1'::smallint))
              Buffers: shared hit=3
Planning:
  Buffers: shared hit=643
Planning Time: 1.104 ms
Execution Time: 0.021 ms

Timings: planning 1.104ms, execution 0.021ms, total 1.125ms.

Migration mode did not run: this merge request adds no file under internal/datastore/migrations/sql/.

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports