feat(npm): the virtual read error mapping 2/3 (S31 plan: 12/19)
The stack
Step 12 is three MRs. They must merge in this order: part 3's Resolve
calls part 1's virtualFetchOptions, and its composed suite asserts envelopes
through part 2's mapping, so part 3 does not compile on main without both.
Parts 1 and 2 are independent of each other; the stack was linear only because
each MR targeted the one before it, which kept every diff to its own files.
| Order | MR | Part | Contents | LOC | State |
|---|---|---|---|---|---|
| 1 | !2325 (merged) | 1/3 | the read's remote.FetchOptions |
106 | merged |
| 2 | !2326 (merged) | 2/3 | the error mapping and its taxonomy | 1666 | this MR, retargeted to main |
| 3 | !2255 (merged) | 3/3 | the resolution facade and the composed suite | 1762 | open, targets this branch |
Part 1 merged on 2026-09-04, so this MR now targets main and part 3 is the
only one still stacked behind it. !2255 (merged) has not rebased past part 1 yet, so it
still carries virtual_fetch_options.go and conflicts with this branch there;
its next rebase clears that, and nothing about it is this MR's to resolve.
3442 LOC across the three at the split, which was the whole of what !2255 (merged) carried, and part 3's tree reproduced the reviewed head file for file. Part 2 has since taken 92 lines of review fixes, so the three now total 3534.
What this MR does
The one place a kind=virtual read failure becomes an npm error envelope,
mirroring the policy shape remote_read_errors.go uses for the kind=remote
routes. Part 2 of the three this step was split into.
It answers 28 values: nineteen resolution-side, the five hosted-winner
refusals the hosted document seam surfaces, and four faults in the client's own
path bytes. virtual_read_errors.md carries the table, the arm order, and the
rule each arm keeps; the prose does not fit a doc comment under the comment
caps.
The rules worth a reviewer's attention
No error outside the answerable-terminal class may map to 404, because npm
caches a 404 as a hard E404 and never retries it. Three verdicts established
an absence and may answer one. A relayed WinnerFetchFailed{404} and an
unparseable tarball name are absences someone established rather than ones
nobody did, so neither widens the rule.
Six of the nineteen are derived from remote.FallbackEligible, not listed.
fetchWinner returns through its default: arm exactly what that predicate
excludes and the caller's own context has not already claimed, so the predicate
is the enumeration and a hand-written list answers a newly added sentinel a
silent 500. TestVirtualReadErrors_TaxonomyIsDerivedFromFallbackEligible
parses internal/remote/errors.go with go/ast to keep the two in step, and
TestVirtualReadErrors_DerivedRowsAreExcludedAtRuntime cross-checks that read
against the predicate as it behaves.
The abandonment arm runs first, and the ordering is load-bearing.
virtual.Resolver.serveFallback wraps ErrUpstreamUnavailable,
ErrResolverCancelled and the fetch failure together when the caller leaves
during the fallback read, so a lower arm would write a 503 with a
Retry-After to a caller that is already gone.
TestVirtualReadErrors_AbandonmentOutranksASecondSentinel drives that compound
error on a live and a departed caller, because moving the arm wholesale does not
falsify the claim.
The target-fault arms answer bad input rather than the fall-through 500.
Without them a client-supplied malformed name answered 500 and wrote an
ERROR record, making bad input a source of this service's own 5xx rate. They
use the statuses RemoteTarballHandler.resolveTarget already uses.
Deviations, stated rather than left to be found
The WinnerFetchFailed Retry-After relay is not implemented. The spec asks
the row to relay the upstream response's own value where it carried one; this
relays none for any in-class status. The value exists one layer down, on
remote.FetchResult.UpstreamRetryAfter, but virtual.Resolver.fetchWinner
builds remote.WinnerFetchFailedError{Status: res.Status} and discards the
rest, so nothing reaches this mapping to relay. Closing it takes a field on that
error type plus a resolver-side change.
#687
owns the conflict, as it already does for the kind=remote rule.
A relayed 503 carries no Retry-After where the kind=remote route stamps
one on the identical status and code. The relay rule is what differs, and the
sidecar says so; TestVirtualReadErrors_WinnerFetchFailedRelaysTheUpstreamStatus
carries the 503 and the 204 because those are the two cases the distinction
rests on.
remote.ErrFillAbandoned on the coalescing row extends S13's rule rather
than following it. S13's "Retry-After on a coalescing failure" names
ErrLeaderCanceled and ErrSingleFlightTimeout, and the spec's
coalesced-follower row names SingleFlightTimeout alone; the sentinel is in
neither. It joins them for the reason remote_read_errors.go gives on the
kind=remote arm, and the sidecar now says so.
#778
owns the reconciliation, and its site list gains this file.
A departed caller keeps a named verdict here, where the kind=remote routes
answer 499. The request-context check sits below virtualReadAnswerFor, so
it reaches only what would otherwise take a 500: on a canceled context
ErrUpstreamUnavailable still answers 503 with its Retry-After and
ErrFileNotFoundOnUpstreams still answers 404.
RemotePackumentHandler.ServeHTTP, RemoteDistTagsHandler.ServeHTTP and
RemoteTarballHandler.writeReadFailure each call writeIfClientClosed ahead of
their whole mapping and so diverge. Hoisting the check would trade a real
verdict for a disconnect stamp, and nothing reads this route's recorded status
until the observability step, so the sidecar names the divergence and leaves the
choice to that step.
The 500 arm writes through writeVirtualServerError, not the plan's
writeRemoteProxyServerError. Reusing the latter logged as it wrote, so every
virtual 500 carried npm remote proxy: unexpected handler error, including
the three internal/virtual wiring sentinels only a virtual read can reach, and
the fall-through wrote that record and its own: two ERROR lines for one
failure, unsampled.
Budget expiry has no owner in the tree
No layer arms an aggregate resolution budget: virtual.Resolver holds only
probeConcurrency, probeTimeout, health and exclusionAudit;
ResolveBudgetExpired exists in no Go file; and
docs/dev/configuration-reference.md records that no read path applies
resolution.resolve_timeout. The spec's Budget-expiry row (E-11) is therefore
implemented nowhere, and
#1139 (closed)
owns arming it.
Its expiry verdict must be distinct from remote.ErrResolverCancelled. The
resolver classifies abandonment from ctx.Err() at every phase, so a budget
implemented as a context.WithTimeout derived inside Resolve would surface as
that sentinel for free, and the abandonment arm answers a still-connected client
with no envelope at all. The sidecar states the requirement and
writeVirtualReadError's doc comment states the caller's obligation on a
false return.
Reviewable LOC
1666 reviewable LOC, measured at 7dda4838d against main at 5d6d17fc3,
over the 500 ceiling in
development-model.md.
| Group | Files | LOC |
|---|---|---|
Production Go (virtual_read_errors.go) |
1 | 288 |
Tests (virtual_read_errors_test.go 964, export_test.go 19) |
2 | 983 |
Sidecar doc (virtual_read_errors.md) |
1 | 395 |
Production is 288, unchanged by the review round: of its 92 added lines, 51 are
in the suite and 41 in the sidecar. The overrun is the suite and the sidecar, and both are
deliberate: TestVirtualReadErrors_MapsEveryTaxonomyValue drives all 28 values
for status, code, Retry-After, Cache-Control, the record's level and count,
and the absence of a fall-through record; two credential suites drive a userinfo-carrying
*url.Error through all 28 from the log side and the body side; and the go/ast
derivation test is what stops a sentinel added to remote.FallbackEligible from
shipping a silent 500. Splitting further would cut the table from the mapping
it pins, which is the one thing that must not drift.
Spec coverage
Spec: docs/specs/S31-npm-virtual.md Plan: docs/plans/2026-08-21-s31-npm-virtual.md, Step 12
The E-numbers below were derived against the spec at this MR's head. !2249 (merged) inserted Budget expiry as E-11 and shifted every row after it.
| # | Condition | Tests |
|---|---|---|
| E-2 to E-4 | Absence established, no upstreams, all denied: 404 |
TestVirtualReadErrors_MapsEveryTaxonomyValue, TestVirtualReadErrors_NotFoundCodeComesFromTheRoute |
| E-5, E-6 | Unhealthy, and no absence with no cached entry: 503 with Retry-After |
/ErrAllUpstreamsUnhealthy, /ErrUpstreamUnavailable |
| E-8 | Winner's Fetch fails after a successful Probe | TestVirtualReadErrors_WinnerFetchFailedRelaysTheUpstreamStatus, seven statuses including 503 and the fail-closed 204 |
| E-9 | A hosted winner's metadata serve refused by its rebuild: 503 |
/errInlineBuildCapacityExceeded, /errInlineBuildTimedOut, /errRebuildShed, /errInlineBuildSuperseded, /errHostedDocumentRowMissingAfterFill |
| E-10 | Abandonment: no envelope, not a server error | /ErrResolverCancelled, TestVirtualReadErrors_ADepartedCallerIsStampedNotLeftAt200, TestVirtualReadErrors_ADepartedCallerDuringTheAssemblyIsNotA500, TestVirtualReadErrors_AbandonmentOutranksASecondSentinel |
| E-11 | Budget expiry | Not implemented in any layer. See above; #1139 (closed) owns it. |
| E-12 | Infrastructure: a cache read or write failed: 500 |
/ErrCacheFill |
| E-13 | Infrastructure: no upstream eligible with a data error: 500 |
/ErrAllUpstreamsIneligible |
| E-14 | Infrastructure: a coalesced follower outwaited its leader: 503 with Retry-After: 1. ErrLeaderCanceled and ErrFillAbandoned share the row; the third is an extension, see Deviations |
TestVirtualReadErrors_CoalescingRowsShareOneRetryAfter |
| E-15 | Infrastructure: the upstream request was rejected before send: 500 |
/ErrHeaderInjection |
| E-16 | Legacy GET /{package}/{tag}: 404 |
Not served here, per ADR 009. The route gap is #1127. |
| E-17, E-18 | Traversal and unparseable file names | /ErrUnsafeFileSegment, /ErrTarballNameUnparseable. Rejection at target construction is Step 17. |
| E-19 | Package name fails the npm regex: 422 |
/ErrUnsafePackageSegment, /ErrTargetPath |
Mapping halves of the Resolution and Allow/deny criteria are here too: R-6, R-7,
R-8, R-10, R-11 and R-13, and A-2, each through
TestVirtualReadErrors_MapsEveryTaxonomyValue. Their composed halves are part
3's.
Security considerations
| # | Concern | Tests |
|---|---|---|
| SEC-1 | No credentials of its own | TestVirtualReadErrors_LogsNoCredentialOnAnyArm and TestVirtualReadErrors_ServesNoCredentialOnAnyArm drive a userinfo-carrying *url.Error through all 28 rows: (*url.Error).Error() serializes its URL raw, so an arm that skipped redactedForLog would leak the stored npm_remote_repositories.url credential. |
| SEC-6 | Denied upstreams are indistinguishable in the response | /ErrAllUpstreamsDenied answers the same envelope as /ErrFileNotFoundOnUpstreams. |
Reachability
writeVirtualReadError has no production caller. All three virtual read slots
in handler.go are still on the interim 501, so nothing here is
production-reachable.
export_test.go's WriteVirtualReadError has no caller at this head either. It
is not dead code: part 3's composed suite sits in the external test package and
cannot reach the unexported virtualReadPolicy, so the wrapper lands with the
mapping rather than with its first user.
e2e scenarios
No scenario in docs/testing/e2e/npm.md is added or affected: that catalog carries no virtual section and no route reaches this code.
Conformance
No format-handler behaviour changes and no route registered, so the npm conformance harness has nothing new to exercise.
Related to #893 (closed)