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:
- !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.goadapters rather than three, a false nullable-last_health_statuspremise, the sizing, the Resolution AC renumbering, two spec amendments rather than one, and a sixthnpm_virtual_*counter. Reviewing this MR against the uncorrected plan makes an accepted trade-off indistinguishable from an oversight. - A spec ruling is outstanding, recorded as !2191 (merged)'s fourth Open Question. An npm virtual upstream list can name a
repositoriesrow of another format: the composite FK constrains only the namespace and the list read applies no format predicate. Such a row has an in-rangekindand 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-404read of that virtual repository into a503(a500when 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-rangekind.internal/virtual/orphan_upstream_shapes.mddocuments 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:NewMissingRemoteBindingUpstreamandNewMissingHostedBindingUpstream, 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:ExclusionRemoteBindingMissingandExclusionHostedBindingMissing, appended afterExclusionUnexpectedKind.internal/virtual/rules.go:ReasonRemoteBindingMissingandReasonHostedBindingMissing, each classified from its own mark before rules are consulted.internal/virtual/resolve.go: twoloadAndFilterarms, 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:
Assemblereturned a nilcontext.Contexton all five error paths. The idiomaticctx, ups, err := a.Assemble(ctx, req)then nils the caller's context exactly where the next line reports the failure, andlog.FromContext(nil)panics. It returnsctxas 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_urlwas not refused.packumentBaseURL("", slug, name)returns a non-empty host-relative/{slug}/npm/{name}, so neithererrNoVirtualFetcherRewriteBasenor npmremote's own empty-base check fires, and a metadata fill would have written host-relativedist.tarballvalues into thenpm_remote_metadata_filesrows the standalone remote route also serves. Now refused per remote metadata position aserrVirtualPublicRegistryURLUnset, mirroring the standalone route'serrPublicRegistryURLUnset: 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.LookupResultis a Miss, resolution may answer404for it, and npm caches a404as a hardE404it never retries, so a seam that dropped one id would make a published package permanently missing. Now refused aserrVirtualHostedVerdictMissing; the converse violation was already refused aserrVirtualHostedUnrequested.
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.mdnow 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, andTestVirtualUpstreamAssembly_ColdReadPaysOnePerProbedPositionasserts 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(...) > 0is 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
rapidproperty test is a plainrapid.Checkover a drawn slice rather thanrapid's state-machine form, which the plan anddocs/dev/go-testing.mdboth 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.mdrecords the accepted volume and that these records are unsampled, sinceinternal/logging's sampler only touchesaccessrecords. - 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 onremote.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 inorphan_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.goandmetrics_test.go, where both MRs append to a list:npmCollectorsand 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.mdandinternal/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.mdonly.
Related to #892 (closed)