refactor(npm): hosted read seam 3/3, the handler migration (S31 plan: 8/19)

Hosted document read seam, part 3 of 3: the handler migration

The stack

Merge in order. !1983 (merged) and !1984 (merged) have merged, so this MR targets main and the diff below is only its own.

#	MR	What it does	Reviewable LOC
1	!1983 (merged)	Seam types and the cache arm	1234
2	!1984 (merged)	The inline-build arm and its collapse group	1013
3	!1985 (merged) (this MR)	Both hosted handlers migrate onto the seam	1543
		Total	3790

!1983 (merged) and !1984 (merged) have both merged. This MR is the last of the three.

What this part does

The packument and dist-tags GET handlers stop carrying their own cache-blob and inline-build arms and read through the HostedDocumentReader the first two parts built. This is where the duplication the seam was extracted from actually goes away: the production files are 240 added against 838 removed, a net -598.

Each handler now resolves the package and its repository visibility per request, hands the seam a (namespace, package, kind), and frames what comes back. The deliberate divergences survive the move and keep the comments saying not to converge them:

• dist-tags honours * and RFC 9110 weak comparison; a packument demands an exact token.
• dist-tags self-heals a missing cache blob; a packument refuses to, because it is not cheaply reproducible.
• the packument arm dispatches a refill on an abandoned build; the dist-tags arm deliberately does not.

Behavior

No behavior changes, with one disclosed exception. Status codes, header sets, header ordering against WriteHeader, Content-Length framing per arm, the two load-shedding 503s and the refill asymmetry between them all match what the handlers did before, and both route suites still pass.

The exception: a fresh npm_metadata_files row whose blob_sha256 is not 32 bytes used to answer a matching If-None-Match with a 304 on the packument route, because the old serveCacheHit built the ETag from the raw column and checked the conditional before validating it. It is now a 500 on all three kinds, matching what dist-tags already did. The column carries CHECK (octet_length(blob_sha256) = 32), pinned in internal/datastore/migrations/npm_schema_integration_test.go, so no reachable row takes either path. MalformedCacheDigestFailsClosed pins the new answer.

Two hardening changes worth reviewing on their own

Each serve path rejects a delivery it cannot frame, the zero value included, releasing any body that document carried. HostedDocumentStream is not the zero Delivery, so a caller relaying a zero HostedDocument gets a 500 rather than a nil-Body panic or a bare 200. Nothing returns that shape today, and that is the point: the seam is exported for the S31 virtual caller, and a caller relaying these values is exactly who declares the struct and fills it on one branch only.

The packument body-write warning carries a document_source field. Before the seam, a truncated cache blob and a client abandoning a large freshly-rendered packument logged two distinct messages. Merging them left one message whose only field is a bare transport error, so a storage fault and ordinary client churn read identically, on a path where the 200 is already committed and the 5xx dashboards stay green.

Retired log strings

Six distinguishable dist-tags 500 messages collapse into npm dist-tags: read failed, and two packument body-write warnings into one. Nothing in this repo references the retired strings; the specific cause survives in the wrapped error under error_message. Flagging it in case an out-of-repo saved search or runbook keys on one:

• npm dist-tags: read cache row failed
• npm dist-tags: cache row has invalid blob_sha256
• npm dist-tags: open cache blob failed
• npm dist-tags: cache blob stream unavailable
• npm dist-tags: cache blob redirect URL unavailable
• npm dist-tags: inline build failed
• npm packument: streaming cache blob failed
• npm packument: writing inline-built body to client

Review fixes, second commit

A branch review found two seam-contract defects and a set of comments the migration invalidated. All of it is unreachable from the two hosted routes, which construct HostedDocument themselves; it matters because the seam is exported for the S31 virtual caller, which is exactly who relays a partially-filled one.

• HostedDocumentSource.String reported the unspecified zero value as cache. The constant's own doc and TestHostedDocument_ZeroValueIsUnset both say it must never read as a cache hit. It now names itself, so an unset Source cannot point an on-call at a storage fault that never happened.
• A HostedDocumentStream carrying no body reached io.Copy past a committed 200. The Delivery discriminator alone admits that shape, so the guard added above did not catch it, and the resulting panic truncated a response the 5xx dashboards never saw. Reverting the fix and re-running the table test reproduces the panic, so this was real rather than theoretical. It is now a 500 on both routes and a case in the same table test.
• The dist-tags body-write warning gains document_source, which the packument one already carried. Both comments describing the field said it separates a storage fault from a client disconnect; that overclaims, because a client disconnect is the common cause on either arm. They now say what the field actually buys: only a cache blob is read as it is written out, so an inline build's failure is always the client.
• Comments that survived the migration but stopped describing it are repointed at HostedDocumentReader: packument_get.go's header and its three seam docs, closeReader's doc and message (it now closes inline-built bodies too), and beforeDo's ordering.
• packument_cache.go's header named read-path refill call sites this stack moved. The success refill is buildInline in hosted_document_read.go and the abandoned-build refill is writeReadError in packument_get.go; disttags.go has no read-path refill at all. The one-line cap on that block means the prose moves to packument_cache.md, where the sidecar already described the dispatch sites correctly. distTagsCacheBlobMissing's runbook diagnosis moves to metrics.md for the same reason.

New tests: the String domain including the zero and an out-of-range value; document_source per arm on both routes, driven through failingBodyWriter; the disclosed 304-to-500 exception pinned at the packument route rather than only at the seam; and BenchmarkPackumentHandler_CacheHit, so the warm path has an allocs/op baseline (1926 B/op, 24 allocs/op at the time of writing).

Two decisions the review asked for

packumentBaseURL is now built on every packument GET, cache hits included. Before the seam it ran only on the cache-miss path. Escape analysis confirms one ~64-byte heap allocation, against a request that already does three database round trips and, on a hit, a blob open. Kept as is: the alternative is carrying Slug and the repository name on HostedDocumentRequest and assembling inside render, which pushes two more primitives across the seam boundary to save an allocation that does not show up next to the I/O.

The five collapsed dist-tags 500 causes do not get a read_stage field here. Grouping by msg can no longer separate a Postgres fault from an object-storage one, and the fix is a low-cardinality field derived from the failing stage. That needs five new sentinels on the seam's error contract, which the S31 virtual caller will also consume, so it belongs in its own change rather than in a behavior-preserving refactor. Tracked in #965 (closed).

Not in the plan's Step 8 text

Disclosed rather than left for a reviewer to find. Step 8 does not mention the in-seam conditional check, which is the largest design decision in the stack; the out-of-domain NpmMetadataKind rejection; or HostedDocument.Source. The first two came from an operator ruling and from AGENTS.md's Type and Seam Conventions respectively. Step 8's file list also names three production files where the stack touches six, and its acceptance says the hosted suites "pass unchanged" where each gains one test. A step MR may not edit the plan, so this is the record.

ifNoneMatchExact stays in the seam file where !1983 (merged) introduced it, so the handler's request-taking ifNoneMatch is deleted here rather than moved.

Diff size

1543 reviewable LOC, but 885 of that is deletion and the production half is a net -598. Split by file group: production 240 added / 838 removed, tests 347 added / 47 removed, sidecar docs 71 added / 0 removed. Re-derived against the merge base after the review-fix commit.

Guardrails

• Behavior-preserving refactor, so exempt from the Related to <issue> line and from the e2e scenario catalog.
• No ADR conflict. ADR-005 holds: both routes pass no blob options, so the configured delivery mode still decides stream against 307, pinned per route by TestPackument_CacheHitPassesNoBlobOpenOption and TestDistTagsHandler_CacheHitPassesNoBlobOpenOption.
• No schema, migration, config, OpenAPI, or Bruno change.
• No test was dropped from either route suite: 22 to 23 and 24 to 25 top-level Test functions, diffed by name.