feat(npm): virtual remote upstream adapter and prober (S31 plan: 6/19)
Virtual remote upstream adapter and prober
Part 2 of 2 of S31 Step 6. Part 1 added the batched cache reads; this part adds
the two seams internal/virtual resolves a remote position through.
Stacked merge requests
Both parts are row 6 of the S31 plan Status table. One row shipping as several merge requests is this plan's own recorded precedent: it notes that S15's Step 5 shipped as four MRs and Step 9 as five, with the Status row naming each.
| # | Merge request | Scope | Reviewable LOC | Depends on |
|---|---|---|---|---|
| 1 | !1976 (merged) | internal/datastore, the two batched cache reads |
1560 | main |
| 2 | This MR | internal/format/npm, the Phase 1 adapter and Phase 2 prober |
999 | !1976 (merged) |
!1976 (merged) has merged. VirtualRemoteUpstream maps
datastore.NpmVirtualRemoteLookup, which part 1 introduced, so this branch did
not compile against main until part 1 landed. It is now rebased onto main
and carries only its own commit; review it against main.
What this adds
The Phase 1 adapter
VirtualRemoteUpstream answers Lookup from the verdict the batched read has
already resolved, so a remote position costs the resolver no statement of its
own and presents the same shape a hosted position does. A nil entry is the Miss
the batched read reports by absence.
Its zero value is not usable. Lookup reports a bypass sentinel rather than a
Miss, because a Miss establishes an absence, and failing open would answer on
evidence that was never gathered. A blob_sha256 of the wrong stored width
fails that one position and not the whole resolution.
The Phase 2 prober
NewVirtualRemoteProber composes a bodyless HEAD against the base the
position's own row names, under the fail-small metadata body cap. It is composed
here rather than borrowed from npmremote.FlightRegistry, which builds its
request builder inside an unexported method.
The Doer is a constructor parameter and is never built here. The security
covers are split across two collaborators: the base-URL gate and the bearer live
in BuildRequest, but the resolved-address deny-list and the own-interface check
live in the Doer's dialer. A prober handed a plain http.Client compiles and
passes every test in this MR, so asserting that production wires the composition
root's shared upstreamhttp Doer belongs to Step 14, and the doc comment says so.
One accepted cost, stated rather than implied
The upstream row is read once per probe rather than snapshotted, which follows
from the plan's composition through NewLiveUpstreamConfigSource. That costs one
row read per probed position, so a Phase 2 fan-out across N positions is N reads
on top of part 1's single batched statement.
No batched read can replace it: FindRemoteDetailsByRepositoryIDs deliberately
never selects the token column, so a batched position read cannot supply the
bearer. The field comment records the cost instead of claiming a freshness
benefit the wiring does not deliver, since one prober serves one probe per
resolution and there is no later probe for a rotation to reach.
Testing
The prefetched verdict in every shape; a path the adapter holds no verdict for;
the zero value and a boxed nil both refused rather than read as a Miss; both
wiring faults refused at construction. Against an httptest upstream: 200 to
ProbeHit and 304 to ProbeFresh, each document kind's path, the stored
bearer and its absence, the row read on every probe, a base that cannot root
requests, a transport failure surfaced as positional data, and the fail-small
body cap. The per-probe read case also asserts the namespace and repository ids
arrive in the order the deps declare, because the two are the same type and a
transposition otherwise compiles and passes.
No e2e scenario is added: docs/testing/e2e/npm.md already lists virtual npm
repositories as owned by S31 and out of scope until the capability ships, and
Step 19 owns the catalog change. No conformance run applies, because this MR
registers no route and implements no npm protocol behaviour.
Size
999 reviewable LOC, over the 500 in development-model.md, split as source 164 / tests 763 / docs 72.
The source is 164 lines; the rest is the suite and one sidecar document.
Splitting the adapter from the
prober was considered and rejected: it lands at roughly 390/500, but the two
share the httptest upstream, the fake repository lookup and the doer builder,
so the smaller half would carry fixtures for a collaborator it does not exercise,
and internal/virtual needs both seams before either is reachable.
Review follow-ups on this branch
Two commits sit on top of the original one, neither changing behaviour.
docs(npm) withdraws a claim the code did not support. The constructor doc said
a blob_sha256 of the wrong stored width "fails that one position, never the
whole resolution", and nothing delivers that: the constructor returns a plain
error, and internal/virtual has no failed-for-data-error position a remote
kind can take, since NewUnexpectedKindUpstream panics on an in-range kind and
omitting a position lets the resolution answer a not-found nothing established.
The same commit corrects the Repos cost note, which said "one row read"
without saying that the read runs inside BuildRequest, spends the probe's own
deadline, and holds one of the resolver's bounded probe slots.
test(npm) closes a gap the keyed literal does not cover: the two same-typed
uuid.UUID fields were forwarded positionally into
NewLiveUpstreamConfigSource, and both fakes discarded their arguments, so a
transposition compiled and passed the whole suite.
internal/format/npm/virtual_remote_upstream.md is new, following the
packument_cache.md precedent in this package: it carries the per-resolution
read accounting, the probe_concurrency versus pool-size question a wiring step
has to answer, and one open question left deliberately unresolved here. A
BuildRequest failure is terminal for the whole resolution rather than
positional for the position, which is right for an unusable base URL and is what
TestVirtualRemoteProber_RefusesAStoredBaseThatCannotRootRequests pins. Now
that a row read sits inside BuildRequest, that class also covers a read that
timed out, and Resolver.probeOne's own doc comment says the opposite ("the
deadline expiring inside the probe is positional data ... never an error here").
Whether a deadline spent in the builder should join the positional class is
resolver semantics rather than this package's to settle, so it is written down
rather than changed.
Related to #887 (closed)