feat(npm): npmremote cache store and health-sweep target resolution

🎯 What this MR does

Adds the cache-table seam (CacheStore) and the scheduled health sweep's per-format target resolution (HealthTargetResolver, NewHealthSweepSource) to internal/format/npm/npmremote. Step 2b of the 5-MR stack splitting an oversized branch.

Rebased onto main. !1482 (merged) has merged, so this MR now targets main and carries only its own work. The rebase was not mechanical: !1482 (merged)'s review split the single operations.go into one file per concern, so these additions land as new cache_store.go and health_sweep.go files instead of appending to operations.go, and Target.tarballVersion goes into target.go next to Validate. The shared test fakes move from operations_fakes_test.go to datastore_fakes_test.go for the same reason.

One behaviour change came out of that rebase. NewUpstreamRequestBuilder now takes an UpstreamConfigSource and reads the URL and credentials per request, so HealthTargetResolver.ProbeTarget composes a per-probe snapshot source instead of passing a URL and credential pair. See 2️⃣ HealthTargetResolver and NewHealthSweepSource below.

The same split left the plan stale: docs/plans/2026-07-15-npm-remote.md listed operations.go and operations_test.go as step 9's files, and neither path exists any more. The third commit corrects that list and records that the split happened in !1482 (merged)'s review, rather than swapping the names in silently.

📚 The stack:

Step MR What it delivers
0 !1479 (merged) Shared refsWithUsableURL enumeration helper
1 !1481 (merged) npm datastore health seams
2a !1482 (merged) npmremote.Target + UpstreamRequestBuilder (merged)
2b · this MR 👉 !1483 (merged) npmremote.CacheStore + health-sweep glue
3 !1484 (merged) Composition root: wiring, audit sink, breadcrumb

🧩 The two pieces

1️⃣ CacheStore

One resolution serves both the freshness read and the targeted bump, so the two cannot resolve a row by different predicates. A missing row at any level is a miss; a path that does not decode is a wiring fault and is reported. A fill writes the row and its parents in one transaction.

No timestamp a caller passes is forwarded, on either bump method or the fill. Each underlying statement stamps its column from the database clock the freshness predicate reads (pg.NOW() in internal/datastore/npm_remote_files.go and internal/datastore/npm_remote_metadata_files.go), so writing a caller's instant would judge a row against a clock it was never compared to.

BumpUpstreamCheckedAt reports remote.ErrCacheEntryNotFound on a vanished row while BumpLastDownloadedAt treats one as a no-op, because the first decides whether a blob reference may be served and the second is a lossy retention counter whose caller logs and drops failures.

2️⃣ HealthTargetResolver and NewHealthSweepSource

npm's sweep contribution. The probe target carries an empty health path, because npm contributes no format-specific one.

The request builder is composed per call over probeUpstreamConfig, a snapshot of the row just read. That is the single-resolution scope UpstreamConfigSource documents as valid, and the only scope it documents as valid, so a token cleared or a URL changed since the last sweep is honoured on the next probe rather than replayed from a cached builder. TestHealthTargetResolver_ProbeTargetDoesNotReplayClearedCredentials pins that.

A stored base URL that cannot root outbound requests is refused at resolution, through remote.ParseUpstreamBase. Both placements (here, or later inside the monitor's own request build) skip the remote and record nothing durable, so what the eager check buys is the failure named against the remote it belongs to. That is also where maven.MavenHealthTargetResolver reports it.

🔎 Review feedback

Finding Outcome
Duo: UpsertCacheEntry discards entry.UpstreamCheckedAt Intentional, but the doc comment did not say so. It now gives the database-clock reason, and also names the digest fields the npm cache tables have no column for.
AppSec: BumpLastDownloadedAt returns nil for a vanished row Behaviour kept. maven.RemoteCacheStore.BumpLastDownloadedAt takes the same posture, so the npm method matches the one other implementation. The ambiguity was in the seam doc: remote.CacheStore carried the sentinel requirement on BumpUpstreamCheckedAt only and neither required nor exempted it on the retention bump. The second commit states the exemption there.
AppSec: ErrTransformRejected wrapping may be absent Already in place, in code that landed earlier in the stack: transform.go:76 for ErrTarballValueTooLong and transform.go:112 for errUnconfiguredRewriter. Not in this diff.
AppSec: transient bare token string in npmUpstreamCredentials() Comment added, but not with the suggested wording. crypto.RedactedString holds its plaintext from construction and Reveal() returns the string already stored in it, so no call-site placement shortens the plaintext's lifetime. The comment says what the inline call does buy (no named variable in the package holds a bare copy) and leaves the lifetime claim accurate.

Verification

  • go build ./... and go vet ./internal/format/npm/... ./internal/remote/...: clean.
  • go test -race -count=1 ./internal/format/npm/... ./internal/remote/...: all green, including the 3 fuzz seed corpora.
  • golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 ./internal/format/npm/npmremote/... ./internal/remote/...: 0 issues.
  • golangci-lint run --build-tags=integration ... over the same packages reports one pre-existing paralleltest finding in internal/remote/healthjob_integration_test.go, a file this MR does not touch.

📏 Diff size

2729 insertions, past the 500 reviewable-LOC line in docs/dev/development-model.md. Split by file group:

Group Files LOC
Production cache_store.go 692, health_sweep.go 205, target.go +13, internal/remote/interfaces.go +10 920
Tests cache_store_test.go 946, datastore_fakes_test.go 505, health_sweep_test.go 358 1809

A further split along the two pieces would not get either part under the line: the cache-store half is about 1640 LOC and the health-sweep half about 570, and the two suites share datastore_fakes_test.go. The step is already the 2b of a 5-way split.

🧪 E2E scenario impact

None: no HTTP handler wires this package yet. The npm packument, dist-tags, and tarball handlers land in later steps of the parent plan.

Related to #346 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports

Loading
Loading