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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading