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)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading