feat(npm): tarball download serve + Cache-Control (S11 Step 9, 3/6)
📦 What
S11 Step 9 (tarball download) is a six-MR stack, in dependency order (each targets the one above; GitLab auto-retargets to main as they merge):
- !535 (merged) - two-id Resolution for npm read handlers. (merged)
- !543 (merged) -
NpmVersionByPackageAndVersiondatastore finder (read-side prerequisite). (merged) - !536 (merged) - tarball download handler: serve + Cache-Control. <- this MR
- !562 (merged) -
last_downloaded_atbump on a served download. - !563 (merged) - perf: resolve the version by an indexed point lookup.
- !537 (merged) - conditional GET + strong validators.
This MR now targets main, rebased on main after !535 (merged) and !543 (merged) merged.
Slice 1 - the core serve path. Implements DownloadHandler for GET/HEAD /{slug}/npm/{repository_name}/{package_name}/-/{file_name}:
- Resolves the
npm_filesrow through package -> versions -> files (keyset-paginated), keyed onResolution.NpmRepositoryID. - Rejects path-traversing/malformed
{file_name}(400file_name_invalid) before any lookup; 404package_not_found/file_not_foundfor missing or soft-deleted rows (the blob is not served even if it survives in CAS). - Serves the blob via
storage.BlobStore.OpenBlob(307 redirect or 200 stream; S06 owns the wire format); the 200 stream setsContent-Lengthfrom the blob size instead of chunked encoding. - Sets the immutable, visibility-varying
Cache-Control(public, max-age=31536000, immutablefor public repositories,private, ...otherwise) andVary: Authorizationon the 200-stream and HEAD success branches — not the 307 redirect (whose signedLocationexpires; matches the OCI handler), and never on a 404/500 envelope (RFC 9111). - Hardens the tarball serve path with
X-Content-Type-Options: nosniff+Content-Disposition: attachment(matches the OCI blob handler). - Answers
HEADviaBlobStore.BlobInfo(existence + size, notOpenBlob): setsContent-Lengthand the same hardening headers, and 404s when the blob is absent from CAS soHEADagrees withGET.
The last_downloaded_at bump (!562 (merged)), the indexed point-lookup optimization (!563 (merged)), and the conditional-GET 304 short-circuit (!537 (merged)) layer on in the stacked follow-ups; this slice resolves the version with the keyset scan and performs no bump. Because that keyset scan is O(versions) on the hot path, !536 (merged) must not merge to production ahead of !563 (merged) - land them in the same merge train.
✅ Spec coverage
| Behaviour | Test |
|---|---|
AC 17 - served via BlobStore.OpenBlob |
TestDownloadHandler_GET_ServesTarballWithCacheHeaders, TestDownloadHandlerIntegration_GET_ServesTarball, TestDownloadHandler_GET_RedirectMode_ServesRedirect |
AC 18 - soft-deleted file -> 404 file_not_found |
TestDownloadHandler_SoftDeletedFile_Returns404, TestDownloadHandlerIntegration_SoftDeletedFile_Returns404 |
AC 34 - package missing -> 404 package_not_found |
TestDownloadHandler_MissingPackage_Returns404, TestDownloadHandlerIntegration_MissingPackage_Returns404 |
AC 53 - ..// in {file_name} -> 400 file_name_invalid |
TestDownloadHandler_PathTraversalFileName_Returns400 |
AC 56 - OpenBlob transient failure |
deferred to S07 fault-injection (TODO(fault-injection) at the surfacing site) |
Cache-Control varies by repositories.visibility |
TestDownloadHandler_CacheControl_VariesByVisibility |
HEAD existence check (BlobInfo) + Content-Length, no body |
TestDownloadHandler_HEAD_NoBody |
HEAD 404s when the blob is absent from CAS (agrees with GET) |
TestDownloadHandler_HEAD_BlobMissingFromCAS_Returns404 |
| multi-page keyset resolution (version + file-name cursor advance) | TestDownloadHandler_MultiPageKeyset_ResolvesFileOnSecondPage |
| finder / visibility errors -> 500, no raw-error leak | TestDownloadHandler_InternalErrors_Return500 |
error envelopes (404/500) omit the immutable Cache-Control |
TestDownloadHandler_ErrorResponses_OmitImmutableCacheControl |
| fail-fast on nil deps | TestNewDownloadHandler_PanicsOnNilDependencies |
⚠️ Known gap - handler not yet wired into the route table
DownloadHandler is fully implemented and tested, but nothing mounts it: internal/format/npm/handler.go still serves the 501 Not Implemented placeholder for the tarball route, and neither handler.go nor wire_npm.go constructs the handler. The tests drive it directly, so a live npm tarball fetch still returns 501.
This is an unassigned plan-level gap: the npm hosted plan creates handler.go with placeholders in Step 8 and never assigns the placeholder->real-handler swap to any handler step. It surfaces no later than Step 23 (real npm-CLI conformance) and will be closed via a plan amendment, not bolted onto this MR. Deliberately out of scope here.
🧪 Testing
go test ./internal/format/npm/...- passgolangci-lint run ./internal/format/npm/...- 0 issuesgo vet -tags integration ./internal/format/npm/...- clean (integration build compiles)- Integration tests (
//go:build integration) run in the merged-results pipeline.
Related to #127 (closed)