feat(npm): upstream assembly and memoized lookups (S31 plan: 11/19)

What

Step 11 of the S31 npm virtual plan: the loading slice. It turns a virtual repository and a target into []virtual.UpstreamSpec plus a virtual.Coordinate, with every Lookup batched and memoized, every health verdict prefetched off the list read's join, and every remote position's flight resolved lazily.

Nothing here is reachable from a route yet. Step 14 is the composition root and Steps 15 to 17 the handlers, so this MR lands the machinery and its tests, and the behaviour it describes is what happens once those land.

internal/virtual and internal/remote take one additive foundation touch, eight edits, described under Foundation touch below.

Merge gates

Two things gate this MR, and neither is a code change to it:

  1. !2191 (merged) should land first. It corrects the merged plan in eight places where Step 11's implementation contradicts it: two binding-missing orphan shapes rather than one, eight foundation edits rather than four, two virtual_memo.go adapters rather than three, a false nullable-last_health_status premise, the sizing, the Resolution AC renumbering, two spec amendments rather than one, and a sixth npm_virtual_* counter. Reviewing this MR against the uncorrected plan makes an accepted trade-off indistinguishable from an oversight.
  2. A spec ruling is outstanding, recorded as !2191 (merged)'s fourth Open Question. An npm virtual upstream list can name a repositories row of another format: the composite FK constrains only the namespace and the list read applies no format predicate. Such a row has an in-range kind and no npm format child, which is byte-for-byte a binding-missing orphan, so it is excluded as a data error, and because a data error vetoes the not-found for the whole list one such upstream turns every otherwise-404 read of that virtual repository into a 503 (a 500 when it is the only position). Neither S31 nor S13 rules on it: both enumerate the data-error causes as a closed list naming a corrupt rule and an out-of-range kind. internal/virtual/orphan_upstream_shapes.md documents both readings rather than special-casing either, and the recommended answer is rejection at the association write path, where the format is known.

Note on ordering: this branch was written stacked on Step 4 part 2 (!2165 (merged)). That has merged, so the branch is rebased onto main and there is no merge-order dependency left on it.

Foundation touch

Additive, and all eight edits land here rather than ahead of the step. No exported signature, panic condition, or enum value changes meaning, so nothing already merged behaves differently:

  • internal/virtual/upstream.go: NewMissingRemoteBindingUpstream and NewMissingHostedBindingUpstream, the fourth and fifth position constructors. Both keep the observed in-range kind rather than a fabricated out-of-range one.
  • internal/remote/audit.go: ExclusionRemoteBindingMissing and ExclusionHostedBindingMissing, appended after ExclusionUnexpectedKind.
  • internal/virtual/rules.go: ReasonRemoteBindingMissing and ReasonHostedBindingMissing, each classified from its own mark before rules are consulted.
  • internal/virtual/resolve.go: two loadAndFilter arms, each with its own ERROR log naming the table to repair.

The hosted shape is required, not optional: virtual_assembly.go has to guard row.NpmRepositoryID before dereferencing it, so a hosted position whose npm_repositories row is absent either reaches a constructor of its own or reaches a nil dereference. The three shapes need three distinct marks because EligibleUpstreams classifies on the presence of a mark alone, so two sharing one collapse into one reason with nothing failing. internal/virtual/orphan_upstream_shapes.md carries the argument once instead of in fragments across four files.

A spec amendment rides behind the two new reasons. It gates nothing, because an appended reason reaches no merged caller.

Review round already applied

/review-branch ran against this branch and returned REQUEST CHANGES with 16 WARNINGs. Fourteen are fixed in fix(npm): close the branch-review findings on the virtual assembly. The three that were behaviour rather than documentation:

  • Assemble returned a nil context.Context on all five error paths. The idiomatic ctx, ups, err := a.Assemble(ctx, req) then nils the caller's context exactly where the next line reports the failure, and log.FromContext(nil) panics. It returns ctx as passed, and a test threads a sentinel value through to prove it is the caller's own context and not merely non-nil.
  • An unset npm.public_registry_url was not refused. packumentBaseURL("", slug, name) returns a non-empty host-relative /{slug}/npm/{name}, so neither errNoVirtualFetcherRewriteBase nor npmremote's own empty-base check fires, and a metadata fill would have written host-relative dist.tarball values into the npm_remote_metadata_files rows the standalone remote route also serves. Now refused per remote metadata position as errVirtualPublicRegistryURLUnset, mirroring the standalone route's errPublicRegistryURLUnset: per request rather than at construction, because the field is optional and a hosted-only deployment must keep booting.
  • A requested position absent from the batched hosted map read as an authoritative absence. The zero remote.LookupResult is a Miss, resolution may answer 404 for it, and npm caches a 404 as a hard E404 it never retries, so a seam that dropped one id would make a published package permanently missing. Now refused as errVirtualHostedVerdictMissing; the converse violation was already refused as errVirtualHostedUnrequested.

Two claims the review falsified are worth calling out because they were load-bearing:

  • The fixed-statement-count claim did not hold on a cold read. Every Phase 2 probe builds its request through LiveUpstreamConfigSource, which reads its own position's row, so a cold 20-remote read pays one statement per probed position. virtual_assembly_contract.md now scopes the claim to the load and Phase 1 and names that seam as deliberately open (it is the per-request credential materialization and cannot be batched without holding every position's credentials for the whole request). vaFixture.statements() counts those reads, and TestVirtualUpstreamAssembly_ColdReadPaysOnePerProbedPosition asserts the fixed part stays fixed at 2 and 20 while the probed part grows.
  • The health-memo counter's documented baseline described wiring that does not exist. The standalone handlers take the durable store as their health seam directly, so on today's wiring every fall-through is the regression and rate(...) > 0 is the alert. All four documentation sites plus a test comment now state the condition and its event.

Tests

New in the review round: constructor-refusal tables for NewVirtualUpstreamAssembly (11 rows, one per seam including each CacheStores field) and NewVirtualRemoteFetcher (3), each asserting its own sentinel; the exported zero value and boxed nil of VirtualHealthStatusReader; two positions in one request keeping their own rewrite bases (the previous test ran two single-position fixtures, which pass under a request-global rewriter too); and one flight per remote repository across positions and namespaces.

Acceptance coverage:

Criterion (cited by name; the spec list has been renumbered since the plan merged) Evidence
An upstream of an unexpected kind fails closed TestResolve_UnexpectedKindUpstream_FailsClosed, TestVirtualUpstreamAssembly_ClassifiesEachKind
A soft-deleted upstream is skipped TestVirtualUpstreamAssembly_SoftDeletedUpstreamIsSkipped
Per-read query count is independent of upstream count TestVirtualUpstreamAssembly_StatementCountIsIndependentOfUpstreamCount plus _ColdReadPaysOnePerProbedPosition
The health status costs no statement TestVirtualUpstreamAssembly_HealthCostsNoStatement, TestVirtualHealthStatusReader_TwentyPositionsIssueNoStatement

All three orphan shapes are also asserted in one list, by TestVirtualUpstreamAssembly_AllThreeOrphanShapesInOneList and TestResolve_AllThreeOrphanShapes_KeepTheirOwnReasons, because every single-shape case still passes when two marks collapse.

Size justification

Past the 500 reviewable-LOC ceiling, and the plan's Step 11 Seam interrogation bullet carries the argument: the list load, the kind classification and the UpstreamSpec construction share the loaded rows, so splitting them passes a half-built slice across an MR boundary; the eight foundation edits alone would land constructors and reasons nothing calls; and the health adapter alone would land a wrapper nothing installs, since Step 14 is downstream.

Split by file group, measured at edc6336d against origin/main:

Group Files Added Removed
Production Go 7 977 188
Tests 11 2,606 0
Markdown (sidecars and catalogs) 5 452 1
Total 23 4,035 189

Non-blank non-comment added lines: 628 source, 1,833 test. Two files are two thirds of the production total, virtual_assembly.go (+512) and virtual_memo.go (+239); the remaining five are the additive foundation edits at +209/−188 and one collector registration.

e2e scenarios

No scenario is added or affected. docs/testing/ has no npm virtual catalog, and the plan assigns the end-to-end harness and the catalog entries to Step 19. Nothing in this MR is reachable from a route, so there is no client-visible behaviour to script yet.

Configuration, API, and schema

None. No migration, no SQL, no internal/config or proto change, no route, status code, header or response shape, and no api/openapi or api/bruno change. The two reads this slice consumes landed with Step 4.

What remains

  • The rapid property test is a plain rapid.Check over a drawn slice rather than rapid's state-machine form, which the plan and docs/dev/go-testing.md both name for virtual registry upstream resolution order. All three drawn dimensions and the lowest-position-success invariant are asserted, so the substance is covered and only the technique differs.
  • No counter yet for data-error exclusions, which is what would make the three per-read ERROR lines safe to ration. It belongs with the resolution metric family rather than in internal/virtual, where a limiter would ration the line for every format over the foundation. orphan_upstream_shapes.md records the accepted volume and that these records are unsampled, since internal/logging's sampler only touches access records.
  • The assembly-to-registry composition (someone handing in a per-request FlightRegistry) cannot be tested until Step 14 exists to wire it. The new test pins the registry's own keying.
  • Several data-error enumerations in resolve.go, and the doc on remote.ErrAllUpstreamsIneligible, still list the two causes that existed before the binding shapes. Those blocks are 15 to 21 comment lines against caps of 1 and 3, so correcting one clause means deleting the block; the current list lives in orphan_upstream_shapes.md, which says so and why.

Open-MR overlap

Scanned all 91 open MRs against this branch's 23 files. Eight overlap, and no pipeline reports a conflict of this kind, so whichever lands second rebases:

  • !2166 (merged) is the one to watch. It touches five of the same files, including internal/format/npm/metrics.go and metrics_test.go, where both MRs append to a list: npmCollectors and the drift guard's expected-name set. That is a textual conflict, not a semantic one, but it is the shape that conflicts silently.
  • !2123 (merged), !2187 (merged), !2193 (merged) touch docs/dev/observability.md and internal/format/npm/metrics.md. Both are append-only tables here, so the conflict is positional.
  • !1598, !2040 (merged), !2103 (merged), !2152 (merged) touch docs/dev/observability.md only.

Related to #892 (closed)

Merge request reports

Loading
Loading