feat(npm): the metadata routes install their read observer (S15 observability plan: 1b/5)

What

Top half of plan step 1, stacked on !2075 (merged). That MR added the remote.ReadObserver seam and remoteDocumentObserver; this one installs it, books the fill, and corrects the catalogs the change falsifies.

Both routes build an observer, pass it in StandaloneOptions, and record the derived cache event once Serve returns. The verdict is booked twice and emitted once: ObserveLookup books what the row earns on its own, and the route's own call after Serve refines it. recordRemoteProxyCache assigns into the deferred record rather than emitting, so the later write replaces the earlier one.

That ordering is what keeps a read answered before a serve resolves inside the sum, which is the invariant this counter is built for. The routes broke it while they recorded only after a nil-error Serve: an upstream 404, a 503 with no cached row, and an oversize document or a coalescing failure inherited by a follower all booked nothing although the lookup had run. A leader's own oversize document is not in that set: the cap is enforced while streaming, so Serve returns nil and the old placement already booked the verdict.

A metadata copy that faults no longer books success. streamRemoteDocument runs the copy through download.go's sourceReadTracker and threads the copy, close and read errors out; recordRemoteDocumentCopy books the caller's fault code (upstream_unavailable on the relay, internal_server_error on the cache serve) when the source itself faulted, and leaves a client disconnect, or a client write that failed over a clean read, on the Warn-and-success path. That is the split recordRemoteTarballCopy already makes. The cap breach and the rejected dist.tarball transform both fault during the route's own io.Copy, which is why they were previously invisible on all three series. The Warn line keeps its level, so writeRemoteProxyError's note that a leader's cap breach ends with a truncated 200 and this path's WARN still holds.

serveRemoteUpstream books the packument cache fill from its own copy count, gated on a completed transfer and on the fill fetch's own Coalesced rather than the latched OR. The fill commits on the read that reaches io.EOF, so a copy that returned nil has already committed it and the close is joined in as a defensive term. The comment saying so sits on serveRemoteUpstream rather than on streamRemoteDocument, which both serve branches share and only one of which has a fill.

Catalog corrections

A branch review found four things wrong with the prose before this landed. All are fixed here rather than after:

• hit_stale is defined as the row a read found rather than the bytes it served, because a read answered before any serve books the verdict its lookup earned. The hit-ratio claim is qualified rather than dropped, since the sum invariant still holds.
• The exclusion set is stated once, as the two groups an npm_remote_proxy record tells apart, replacing a four-class count written for the tarball route that is wrong on these two: remote operations resolve after the record begins here (remote_packument.go:401 vs :319) and before it there (remote_tarball.go:261 vs :414), and three further classes were named nowhere. Four other sites now point at that one row instead of restating it.
• coalesced no longer claims a per-route difference that does not exist: recordRemoteProxyCoalesced latches on all three routes.
• revalidated now says which row it names, because under coalescing a caller whose own lookup missed can be served bytes another caller's 304 confirmed.

remote_metrics.go's const block loses its prose to metrics.md rather than carrying a corrected copy: a const ( group's doc caps at three lines and the block was fourteen.

The comments !2075 (merged) wrote in future tense go back into the present, now that the routes do install the observer: the file header, the blank-identifier guard, the surviving accessor, and remoteLookupCacheVerdict's own doc in remote_tarball.go.

Review round 2

A second branch review found two defects and six catalog claims this branch had falsified. All are fixed on the branch.

Blocking. TestRemoteMetadataRoutes_RefusalAheadOfTheLookupBooksNoVerdict was t.Parallel() while asserting a before/after delta on the process-global npm_remote_cache_events_total. That was safe while neither metadata route booked a verdict; installing the observer makes both book one, and TestRemoteReads_* drives both routes on fresh hits from the same parallel pass. -run over the two names with -count=20 failed with difference was -2 on the packument path and passes 20/20 with the parent serial. tparallel reports nothing once the parent is serial, so its token leaves the directive.

Blocking. A commit body on this branch misstates where the cache fill commits. See Squash below.

Catalogs. observability.md's withdrawal rule claimed a fall-through whose own fetch fails leaves the lookup's verdict standing; the withdrawal keys on a fetch having followed the row, not on its outcome, so only a fall-through with no fetch behind it keeps the verdict. hit_fresh's second shape gains the row it omitted, the unhealthy-remote gate refusing a vanished-blob recovery behind a 503. The same row still described the coalesced latch this branch deleted along with joinedAFlight. remoteCacheEventsTotal's own doc still promised a hit ratio read straight off the four. Two test comments still carried the superseded "less its lookup failures". metrics.md now says why miss is "no usable row", and its new heading no longer swallows the transform histogram's paragraphs.

Tests. The mirrored tarball row the metadata suites had dropped, a revalidated row whose vanished blob a second fetch recovers, joins the shared shape table, which gains wantUpstreamAlso because that read leads two upstream calls. It differs from its mirror in one field: the tarball fixture scripts the 304 as coalesced, and this fixture drives a real SingleFlight over one caller, so both fetches lead. Three mutations of the row fail, so it is not vacuous.

Three findings deferred to one follow-up, #1022. classifyRemoteUpstream's doc still says an oversize metadata document is classified transport_error, which this branch struck from observability.md. Deleting the clause touches the block at remote_metrics.go:319-359, 41 lines rather than the 35 an earlier revision of this description claimed, and the caps gate then holds it to cap 1, so the clause cannot go without relocating the block to metrics.md.

The same relocation carries two more fixes, which is why they travel together rather than one per MR. Line 344 of that block says remoteUpstreamResponsesTotal's own comment carries the tracking item for giving a refusal a value of its own; that variable's doc is a one-liner with no work item, at :115 here and :146 on main, so the sentence points at nothing. The item it means is #826. And observeRemoteCacheFill's fourth parameter is named coalesced while the metadata call site passes suppressFillObservation(), which is also true when no body came down, so two readings sit behind one name; that doc is 23 lines under the same cap of 1.

The metadata-versus-tarball verdict divergence on remote_document_serve.md's mechanism 2 is unchanged for a different reason: it wants a deliberate ruling rather than a quiet edit.

Stack

Merged in order. Both branches shared the merge base 44c147701.

#	MR	Branch	Contents
1	!2075 (merged) (merged)	dmeshcharakou/npm-remote-observability-step-1a	the ReadObserver seam and remoteDocumentObserver
2	this MR	dmeshcharakou/npm-remote-observability-step-1b	the two routes install it, the fill, and the catalogs

Targets main, now that !2075 (merged) has merged. The branch is rebased onto that merge and carries step 1b alone.

The !2132 (merged) merge order is settled and discharged. !2132 (merged) (10io/s09-step13-npm-authz-wiring-part2of4) conflicted with this branch on docs/dev/observability.md, so whichever landed second had to resolve it and no pipeline reports that. !2132 (merged) merged first, and this branch was then rebased onto the main that carries it. The resolution kept this branch's four npm rows, kept !2132 (merged)'s npm_resolutions_total row byte-identical, and dropped only the superseded emitter-coverage clause this MR replaces; the file's metric-row count is unchanged across it. A later conflict on the same file is a new episode rather than this one reopening. A file-overlap scan run before that rebase found six further MRs touching this branch's files that merged cleanly, and !1598, which conflicts with main as well and so needs a rebase whatever this MR does.

The plan's own record of this step is on main. !2072 (merged) has merged, so the Status table there carries 1a and 1b as separate rows, records 1b as this MR, and divides step 1's Files list between the two halves. The title's 1b/5 marker is checkable against the plan itself rather than against an open MR.

What the rebase settled

!2075 (merged)'s own review changed four things this branch was written against, so the rebase had to reconcile them rather than take a side:

• The fill gate is suppressFillObservation, the name !2075 (merged) merged with. The call site in serveRemoteUpstream follows it.
• metrics.md's event bullet and observability.md's counter row keep the earned-verdict caveat !2075 (merged)'s review added, on top of this MR's own corrections: the exclusion set is still stated once, in the observability.md row, and the other sites point at it.
• remote_document_serve.md's "Which cache event a metadata read books" and ObserveLookup's note on why it books early were written in future tense by !2075 (merged)'s review, which this branch never saw. Both are now present tense, which is the same correction this MR already made on the other sites.
• The emitter-coverage claims move together: no npm_cache_path and no metadata npm_operation is a gap any more, so the probe is the only one left, and metrics.md, observability.md and the S15 spec all say so.

Tests

Every read shape on both routes, including the two HEAD rows the mirrored tarball suite carries and both metadata suites had dropped; the error exits that book their lookup verdict; the lookup failure that books nothing; and one shared SingleFlight collapsing a herd into a single upstream call and a single fill.

Both halves of the HEAD pair were confirmed to fail against a broken implementation rather than assumed: dropping the HEAD arm of streamRemoteDocument's moved reset fails them on bytes, and returning early from serveRemoteUpstream on a HEAD fails the cold-HEAD row on the fill count. Each fails on both routes.

TestRemoteMetadataRoutes_RecordNoCacheVerdict asserted the rule this MR reverses and stayed green only because its fixture refuses the read before the cache lookup. Renamed to say that, along with its header, its cachePath field doc and both assertion messages.

Size

1660 reviewable LOC at a3921ba8a: 1296 test, 212 production Go, 152 docs, from git diff origin/main...HEAD --numstat. Splitting further would separate the route wiring from the suites that cover it, which AGENTS.md guardrail 6 asks against; the production half is 212 lines across seven files.

Review round 1

A branch review found the observer installed in a position that made half the derivation unreachable, plus nine catalog and test gaps. All are fixed here.

Blocking. recordRemoteProxyCache(ctx, observer.cacheEvent(result.Source)) sat below the if err != nil return. remote.ServeError is the zero ServeSource and internal/remote/serve.go says a nil-error Serve never produces it, so unresolvedVerdict and the fetchAfterRevalidation field were dead: every error exit reported its unrefined lookup verdict, diverging from the tarball route in the hit-inflating direction. remote_document_serve.md's mechanism table already describes the refined answers, which is what identifies the placement rather than the table as the defect. The call now sits above the branch on both routes, and a new error-shape row pins it: moving the call back makes that row report hit_stale instead of miss.

coalesced stamp timing, which the plan's step-1 Note assigns to this step. ObserveFetch stamps recordRemoteProxyCoalesced as each fetch lands instead of the route reading a latch after Serve returned, so a panic between the last fetch and the response no longer renders coalesced=false for a read that joined a flight. The latch field and its accessor go away; the record's own OR is what makes a per-fetch stamp equivalent.

Tests. Three rows the mirrored tarball suite carries and both metadata suites had dropped: a fresh row whose blob vanished, an upstream 502 under outcome=server_error, and the 304 whose recovery re-fetch fails. Plus TestRemotePackumentRoute_ConcurrentDistinctReadsKeepTheirOwnVerdict, the per-request composition test the plan's Acceptance criterion names, which the coalescing test does not discharge: that one proves the SingleFlight is shared, this one proves the observer is not. Handing every read one shared observer makes it report three data races and one read's verdict on another. The herd test's upstream-call precondition relaxes to Less, matching the sibling coalescing test.

Catalogs. hit_fresh carries the same "answered before any serve resolved" shape as hit_stale, since a fresh row whose blob fails to open on anything but a vanished blob answers 500 and books it, so an object-store brownout reads as a perfect hit ratio; the qualifier now covers all four values. An oversize metadata document does not book transport_error, because the cap is enforced while streaming: the fetch books ok and the breach surfaces as a short body and a Warn line. A failing cache lookup is every route's post-record exit, not the metadata routes', because remote.StandaloneFlow.Deliver returns on it before it reports the row. The fill histogram's _count is a floor on committed fills. The tarball route's 400 joins the rejected-name list. The spec no longer claims remote.HealthProbeObserver does not exist, since it is on main. Two sites still carried the superseded "less its lookup failures", one citation pointed at a doc block this branch emptied, and the moved const prose now has a named section to point at.

Squash. A commit body on this branch says "the close is what commits the fill". The code and the comment beside it say the opposite: the fill commits on the read that reaches io.EOF, and the close reports an abandoned one. Rewording that body needs an interactive rebase, so it is left in place and squash carries the fix instead: only this description reaches main. squash_option on this project is default_on and is togglable at merge, so confirm the squash checkbox when merging rather than relying on the default.

Checks

go test ./internal/format/npm/... ./internal/remote/... green, golangci-lint 0 issues, comment-caps clean, gofmt/go vet clean, and markdownlint + Vale + lychee pass on all four changed documents.

No e2e scenario changes: docs/testing/e2e/npm.md records client-observable behavior, and a metric series is not that.

Related to #800 (closed)