refactor(npm): hosted read seam 1/3, the cache arm (S31 plan: 8/19)

Hosted document read seam, part 1 of 3: the cache arm

The stack

Merge in order. Each part targets the one above it, so !1984 (merged) and !1985 (merged) show only their own diff.

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

Measured with git diff origin/main...HEAD --numstat for this MR and from each MR's own diff for the other two, at the time of writing. !1984 (merged)'s figure grows once it rebases: this MR moved the ResponseWriter fake and the version and tag fixture rows into it, which is where they first have a reader (see Review follow-ups below).

What this part does

Step 8 of the S31 npm virtual plan extracts a library-callable hosted metadata read out of the two npm metadata ServeHTTP methods, so the virtual tier can reach a hosted document without going through an http.Handler.

This part introduces the seam's types and the arm that answers from the cache. HostedDocumentReader.Read takes a (namespace, package, kind) and resolves the fresh npm_metadata_files row for that kind alone, framing the outcome as a stream, a pre-signed redirect, or a not-modified.

The kind-keyed read is the point. A seam taking only the package would serve a full packument to an abbreviated Accept on the virtual arm, because npm_metadata_files is kind-keyed.

Both hosted routes are untouched and still serve their own duplicated arms, so this part changes no behavior. They migrate in !1985 (merged), which is where those duplicates are deleted and where the production line count goes negative.

Merge order, and the collision with !1990 (merged)

!1990 (merged) (feat(npm): converge the packument cache miss on the fenced rebuild, S11 step 3/7) is open against main and rewrites the code this stack extracts. Nothing in this MR conflicts with it: !1990 (merged)'s last packument_get.go hunk covers old lines 467 to 648, this MR's sits at old line 770, and !1990 (merged) removes only call sites of ifNoneMatch and etagFromSHA, not the definitions. Both merge and both compile, in either order.

Three collisions land later in the stack, and the order has to be settled before !1984 (merged) goes to review.

  • The packument miss. !1990 (merged) deletes buildPackument and allNpmVersions and converges the miss on the fenced rebuild. !1984 (merged) builds an inline-build arm into this seam and !1985 (merged) points the packument handler at it. These are opposite designs for the same path, so whichever lands second needs rework rather than a rebase.
  • The dist-tags self-heal bound. !1990 (merged) adds a rebuilt bool to DistTagsHandler.serveCacheHit that bounds the blob-missing fall-through to one attempt. HostedDocumentReader.fromCacheRow reproduces the older unbounded fall-through. If !1990 (merged) lands first, !1985 (merged) silently reverts that bound unless !1984 (merged) carries it into the seam. No pipeline reports this.
  • The counter's documented meaning. !1990 (merged) rewrites distTagsCacheBlobMissing's doc in metrics.go to a single line and adds operator-facing prose to internal/format/npm/metrics.md describing the series as serveCacheHit's fall-through, bounded to one attempt, and a floor on requests that answered 500 after a second miss. The seam's producer is none of those three: it is not serveCacheHit, it is unbounded, and it returns a miss rather than a 500. If !1990 (merged) lands first, all three clauses go false for half the series' producers the moment !1985 (merged) wires the seam in, in a file operators read. No pipeline reports this either.

The plan's step 8 Depends-on row currently reads "Refactors merged S11 code only", which is not true while !1990 (merged) is open. A step MR does not edit the plan file, so that correction belongs in the plan's own MR.

Deliberately absent, and why

Three things are missing on purpose and arrive in !1984 (merged) with the inline-build arm that needs them:

  • HostedDocumentRequest.PackageName and BaseURL, which only a render reads.
  • The ResponseWriter parameter on Read, needed only once a build can outlive the connection's write deadline.
  • The inline-build answer to a cache miss. Until then a cold read returns errHostedDocumentCacheMiss and the dist-tags self-heal on a missing blob returns errHostedDocumentCacheBlobGone, which wraps it: a caller absorbing either matches the miss, and a bound on the fall-through can still tell the two apart.

That sentinel and those two signatures change in !1984 (merged). The churn is a consequence of slicing the seam vertically rather than landing all of it at once, and it is why !1984 (merged) shows call-site edits in tests this MR adds.

HostedDocumentDeliveryUnspecified heads the delivery enum so the zero value cannot pass for a nil-Body stream. The two serve paths that reject it arrive with the handlers in !1985 (merged).

ifNoneMatchExact is the string-taking exact matcher the seam needs. packument_get.go's request-taking ifNoneMatch now delegates to it rather than carrying a second copy of the RFC 9110 list walk.

One ordering change !1985 (merged) will show

The seam validates blob_sha256 before it derives the ETag. PackumentHandler.serveCacheHit does the reverse: it builds the tag from the raw column and answers a conditional before calling sha256SumFromBytes, so a row whose digest is not 32 bytes can produce a 304 carrying a validator derived from an unvalidated column. DistTagsHandler.serveCacheHit already had the seam's order.

The seam's order is fail-closed, and the column carries a 32-byte octet_length CHECK, so this is reachable only on corrupt data. It is called out here so the packument delegation in !1985 (merged) is not read as an unexplained behavior change. TestHostedDocumentReader_Read_MalformedCacheDigestFailsClosed pins it.

Testing

The seam's unit suite reaches HostedDocumentReader directly rather than through a handler: the fresh-row and redirect arms per kind, blob-option pass-through in both directions, a cache-row fault as a non-miss, a package with no row and a package warm only under a different kind as misses, a malformed blob_sha256 failing closed on all three kinds, the in-seam conditional across four header shapes times three kinds plus the dist-tags-only divergences, the kind-domain rejection at both ends of its two-sided guard, and one cache read per read.

The two miss tests are the arm every cold read takes. Deleting cachedRow's datastore.ErrNotFound case leaves both failing and the rest of the suite green.

The existing hosted packument and dist-tags suites pass unchanged.

Diff size

1226 reviewable LOC, over the 500-LOC guardrail. Split by file group: production 294 added / 19 removed, tests 913 added / 0 removed.

The split is three ways and the largest part is this one at 1226. It cannot go lower without separating the seam from the test suite that covers it, which would ship untested production code. The test file is large because every arm is driven across all three NpmMetadataKind values as a positive hit rather than one value positively and the rest negatively.

Review follow-ups applied

  • Two tests now cover the plain cache miss, across all three kinds. Nothing drove that arm before.
  • cachedDelivery now branches on kind for the 307's validator, so the ETag-on-redirect split is enforced in code rather than left to a comment and to !1985 (merged)'s packument delegation. It joins the OpenBlob self-heal and the validator matcher as the third deliberate divergence between the kinds, and TestHostedDocumentReader_Read_RedirectsPerKind asserts the presence per kind.
  • HostedDocumentRequest's identifier pair records that the seam applies no visibility, repository, or ownership check.
  • Comments describing part 2's arm moved to future tense, and the pointer to it now sits once, on HostedDocumentReader.
  • cachedDelivery's doc names the effective delivery mode rather than a namespace setting; no npm path passes storage.WithDeliveryMode, so the instance default is what decides.
  • ifNoneMatch's doc no longer describes the body this MR moved into ifNoneMatchExact.
  • The hostedDocWriter fake, its ten assertWroteNoResponse calls, and the version, tag, and package.json fixture rows are gone. Read takes no ResponseWriter in this part, so the fake reached nothing and those assertions could not fail. They return with !1984 (merged), where they become live.

Carried to !1984 (merged)

  • Delete the distTagsCacheBlobMissing.Inc() inside DistTagsHandler.serveCacheHit rather than bypassing it when the handler delegates to the seam, or one fall-through counts twice. The symbol rather than a line number, because that call sits inside a !1990 (merged) hunk and moves if !1990 (merged) lands first.
  • Correct distTagsCacheBlobMissing's doc in metrics.go and its prose in internal/format/npm/metrics.md to name both producers and what each one does. Neither can be corrected here: the metrics.go block is 17 lines against a 1-line unexported cap, so the fix is the sidecar compression !1990 (merged) already performs, and doing it in this MR would collide with that rewrite head-on.
  • Carry !1990 (merged)'s one-attempt self-heal bound into the seam if !1990 (merged) lands first.
  • Construct one HostedDocumentReader per repository at handler-mount time once it holds a collapse group, and prove the sharing with a concurrency test. No constructor signature can detect a per-request instance, and halved coalescing fails no unit test.

Guardrails

  • Behavior-preserving refactor, so exempt from the Related to <issue> line and from the e2e scenario catalog.
  • No ADR conflict. ADR-005 holds and is the load-bearing one: the seam takes opts ...storage.BlobOpenOption and the hosted routes pass none, so the configured delivery mode still decides stream against 307. Pinned by the option pass-through test. ADR-023 holds: the new file imports internal/datastore, internal/datastore/jet/.../model, internal/storage, labkit and github.com/google/uuid, so no sibling format package and no raw SQL driver.
  • No schema, migration, config, OpenAPI, or Bruno change.
  • The plan's Status-table row is recorded separately; a step MR does not edit the plan file.
Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading