feat(npm): the metadata read observer and its standalone seam (S15 observability plan: 1a/5)

What

Bottom half of plan step 1, split from its route wiring so each MR is reviewable on its own.

remote.StandaloneOptions gains an Observer and forwards it in flowOptions(), so a caller of Standalone.Serve can see the cache row the read started from and each upstream Fetch. A ServeResult reports neither: it folds a fresh hit together with a stale row a 304 confirmed, and a miss together with a stale row re-fetched whole. FlowOptions.Observer and remote.ReadObserver already existed; a nil Observer observes nothing, so every existing caller behaves identically.

remoteDocumentObserver is the npm-side implementation. It derives a metadata read's cache verdict from the serve source together with the lookup row and the fetch outcomes it saw, and exposes two deliberately different coalescing answers:

  • the wide event's OR over every upstream call the read made, because either of a read's two fetches may have joined a flight;
  • the fill gate's narrower per-fill Coalesced, because observeRemoteCacheFill asks who wrote the bytes.

Those disagree on one shape the flow produces, a coalesced 304 followed by a led body, where the latch would drop an observation for a fill this read committed.

What the unresolved arm books

A read that resolved no remote.ServeSource still earned a verdict, and unresolvedVerdict decides which of three it books.

  • Nothing, when no row was ever reported. The lookup itself failed, so a zero remote.LookupResult would otherwise read as a miss.
  • miss, when the row the read started from turned out to be unserveable. A 304 followed by another fetch means serveRevalidated sent the read back to the upstream, which only a vanished blob does; and with no 304 in the read at all, a fresh row reaches a fetch only through Deliver's vanished-blob fall-through, so a fetch behind one means its bytes were gone.
  • revalidated, when a 304 confirmed a row, whichever row it arrived over, and no re-fetch followed. The serve then failed for a reason that did not send the read upstream, and remoteTarballDelivery.ServeCached books revalidated for that same shape, because the upstream settled the verdict whether or not the bytes came back.

fetchAfterRevalidation is what separates the last two. Without it a storage fault on the revalidated serve booked miss where the tarball route books revalidated, which is the cross-path disagreement this arm exists to prevent. The 304 arm is tested ahead of the vanished-fresh-row check for the same reason: tested the other way round, a 304 that arrived over a fresh row whose blob was gone was withdrawn to miss while the tarball route booked revalidated, and no table row combined the two to catch it.

Three mechanisms where the two npm read paths still disagree

The observer never sees remote.ReadDelivery.ServeCached fail, so it works from proxies for that outcome. remoteTarballDelivery is its own ServeCached and reads it directly. Three mechanical differences follow, and every shape where the two paths book a different event comes from one of them: the degraded upgrade, the withdrawal proxy, and the site each path books revalidated from. The metadata answer is the wrong one on the second, and the tarball's on the other two.

internal/format/npm/remote_document_serve.md carries the mechanisms, one worked shape each, the defensible answer, and the commit the trace was made at. Stating the mechanism rather than enumerating shapes is deliberate: an earlier revision of that section listed four shapes and claimed the list was complete, and it was short by at least one that this MR's own test table already asserted. Every shape needs a retention race or an infrastructure fault on top of an ordinary read. Closing all three means giving remote.ReadObserver the serve outcome so both paths read one predicate rather than two proxies, which is a larger change than this MR carries and would fix the tarball route too.

The seam's contract

remote.ReadObserver now states what remote.ReadDelivery already did, that every implementation is scoped to one read, and what neither said: a method must not block or panic. ObserveFetch runs while the flow still owns result.Body, so a panic there costs what ReadDelivery.ServeUpstream's own doc enumerates for the same window: a leaked body, a fill slot retired for the process's life, and a path every later Fetch burns its wait timeout on. The comment points at that enumeration rather than restating half of it. Nothing enforces the prohibition, as SingleFlight.runLeader does one layer down for the same invariant; a recover here would have to cover ServeUpstream too, which is a change to a shipped path and does not belong in this MR. StandaloneOptions.Observer repeats the contract, because a caller reads it off the field rather than off the interface.

The rename

remoteTarballCacheVerdict is renamed remoteLookupCacheVerdict, since the observer calls it too. It stays in remote_tarball.go rather than moving beside the remoteCacheEvent vocabulary: the comment-caps rule forces any block a diff touches to its cap, and deleting the function from that file touches remoteTarballFallbackEligible's 25-line block below it, whose cap as an unexported doc is 1. Renaming in place leaves that block alone.

No behavior change

No route installs the observer yet, so this MR emits no new series and changes no response.

Five comments describe that relationship in future tense, per the guardrail on forward references, and the wiring MR puts them into the present: the file header, the blank-identifier guard, the ObserveLookup booking comment, and both accessors. All five are in remote_document_observe.go. remoteLookupCacheVerdict's own doc was the sixth and is now present tense instead, because the tarball path books it today and only the metadata path is unwired.

internal/format/npm/metrics.md carried that stale reason in two places twelve lines apart, and both are corrected here: the upstream-responses row and the paragraph on a metadata record's coalesced. The seam now hands a remote.FetchResult to any Standalone.Serve caller, so what is missing is the emitter, not the fact. Both say "neither metadata route" rather than "no route", because the tarball route does install an observer, through remote.FlowOptions. remote_metrics.go's remoteCachePath block carries the same stale reason and is deliberately left alone: it is a blank-joined block of 21 comment lines, 19 of which count once the two bare // separators are dropped, against an unexported cap of 1. Touching one line forces a rewrite of prose unrelated to this MR. !2076 (merged) is where that reason goes when the series go live.

Stack

Merge in order. !2075 (merged) and !2076 (merged) share the merge base 44c147701.

# MR Branch Contents
0 !2072 (merged) dmeshcharakou/s15-observability-plan-status-row-3 the plan's Status table: splits step 1 into 1a and 1b, and takes the step-MR denominator to /5
1 this MR dmeshcharakou/npm-remote-observability-step-1a the ReadObserver seam and remoteDocumentObserver
2 !2076 (merged) dmeshcharakou/npm-remote-observability-step-1b the two routes install it, the fill, and the catalogs

!2072 (merged) must land at or before this MR. This MR's title reads 1a/5, and until !2072 (merged) merges the plan's Status table has four rows numbered 1 to 4 with no 1a, and its own marker sentence says /4. Merged the other way round, main carries a step-MR title whose step id and denominator match no table. !2072 (merged) and this MR target main; !2076 (merged) targets this branch, so GitLab will not let it merge first. The order between !2072 (merged) and this MR is the one nothing enforces, which is why it is stated here rather than left implied by the stack.

Tests

  • TestStandalone_Serve_ObservesTheLookupAndEachFetch: 7 rows over the forwarding.
  • TestStandalone_Serve_NilObserverObservesNothing.
  • TestStandalone_Serve_CoalescesConcurrentServesOverRealClient now installs one observer per Serve and asserts the leader's Coalesced is false and each follower's true. Both of npm's coalescing answers turn on that field, and every other test in the pair fabricates it, so this is the one place a real value is proved to reach an observer through the new seam. It is also the composition proof the sibling remote_tarball_coalescing_observability_test.go already had.
  • TestRemoteDocumentObserver_*: an 18-row derivation table naming the StandaloneFlow arm each row comes from, because the mapping is coupled to flow behavior internal/remote can change without npm failing to compile, plus the coalescing latch and the per-fill gate. The eighteenth row is the 304 over a vanished fresh row: it is the one shape the branch order in unresolvedVerdict decides, and it fails against the previous order.
  • TestRemoteDocumentObserver_BooksNothingWhenNoLookupWasReported covers the one exit no table row can reach, because every row drives cacheEvent through a helper that always reports a row. Before it, the observed field and its guard could be deleted with the suite still green.

Every assertion added in review was confirmed red against the unfixed code rather than assumed: the revalidated row and the two-fetch withdrawal row against the old unconditional withdrawal, the no-lookup test against the deleted guard, and the coalescing assertions against a flowOptions() that dropped the Observer.

Size

1071 reviewable LOC: 799 test Go, 188 production Go, 84 Markdown. Under docs/dev/development-model.md's ask, the production half is small and the tests carry the diff; the derivation table is the bulk, at one row per branch of the derivation.

Checks

go test ./internal/format/npm/... ./internal/remote/... green, golangci-lint 0 issues, comment-caps clean against the merge base, gofmt and go vet clean. All run against this branch alone, not the combined stack.

No e2e scenario changes: docs/testing/e2e/npm.md records client-observable behavior, and this MR changes none.

Related to #800 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading