feat(maven): remote cache store — cache-fill writes (S14 Step 9)

What

Lands the write half of the remote.CacheStore implementation for Maven — Step 9 of the S14 Maven remote vertical slice, on Step 8's wrapper/backing-store split. The three interface write methods match the merged S13 signatures and own their transactions through datastore.RunInTx, delegating to unexported qrm.DB-accepting internals a larger transaction can compose (the two-tier shape Step 7 established):

  • UpsertCacheEntry — the cache fill. Lazily finds-or-creates the maven_remote_packages / maven_remote_versions parents inside the fill transaction (no auto-create-on-push path exists on a remote; first-cache is the only creation trigger), then upserts maven_remote_files with the blob reference and all four digest columns. A replacement fill repoints blob_storage_attachment_id / blob_sha256 so the superseded blob stops being referenced, rewrites every digest column, and sets upstream_etag / upstream_checked_at.
  • BumpUpstreamCheckedAt — the 304 revalidation bump; advances only upstream_checked_at, never the blob reference or digests, and reports a vanished row as remote.ErrCacheEntryNotFound.
  • BumpLastDownloadedAt — the download-signal bumps for the package and version rows.

The wrapper in internal/format/maven/remote_store.go keeps the seam contract: it parses the path, converts entry.SHA1 to checksum.SHA1Sum, and fails closed on a nil or short SHA-1 before touching the database — the sha1 column is NOT NULL with a length CHECK, so the fetch pipeline's pre-tee call site (which passes zero-value digests) is rejected rather than persisted. That makes S13's tee computation a hard merge-order prerequisite for wiring this store into Fetch, which is Track B's gate, not this step's.

The CacheEntry amendment and the counter deferral

Two operator-approved scope amendments ride this branch, both recorded in the plan and — as of this MR — annotated in the spec:

  • remote.CacheEntry digest-field widening (pulled in from S13): SHA1 and SHA512 as []byte, nullable MD5 as []byte (nil under FIPS). Only the type widening moves in; S13's tee still computes the digests and remains Track B's external prerequisite. The existing fetch-pipeline call site and remotetest fakes use named-field construction and compile unchanged. The accepted cost: an S14 step shapes an S13 type, so S13's tee work must conform to the field types chosen here.
  • downloads_count deferred out of the plan. The merged remote.CacheStore.BumpLastDownloadedAt seam carries no archive-class or HEAD/GET signal, so no store method can express S14's counting rule; the last_downloaded_at bumps ship as interim direct UPDATEs carrying the TODO(s18-buffered-counters) swap marker (AST-enforced), mirroring the merged npm-remote precedent. The counter half of Artifact proxy criteria 1/4/11 and Metadata proxy 8 lands with the buffered-write client work or a seam-widening follow-up. docs/specs/S14-maven-remote.md is annotated at the cache-hit flow, criterion 4, the counting Resolution, the S18 Dependencies bullet, and the digest-seam Resolution so the spec stops asserting an unamended contract.

Tests

Every acceptance clause has a named asserting test:

Acceptance clause Test
CacheEntry carries the three digest fields; fetch-pipeline call site and fakes compile unchanged; short-SHA1 guard rejects zero-value digests TestCacheEntry_DigestFields (internal/remote/cache_entry_test.go); internal/remote/fetch.go and remotetest untouched by the diff; guard pinned by TestRemoteCacheStore_UpsertCacheEntry_PreBackingGuards
Fill creates package, version, and file rows lazily and idempotently; second fill creates no duplicates TestMavenRemoteCacheStore_UpsertCacheEntry_CreatesRowsLazily, _PackageLevelFileHasNoVersion, _SecondFillCreatesNoDuplicates
Replacement fill repoints the blob, rewrites all four digest columns, updates etag and checked-at _ReplacementRepointsBlobAndRewritesDigests, with assertDistinctDigests so the rewrite assertions are not vacuous
qrm.DB internal driven inside a rolled-back transaction leaves the prior row and blob reference untouched, writes no partial row _RolledBackTransactionLeavesRowsUntouched
304 bump advances upstream_checked_at without touching the blob reference or digests TestMavenRemoteCacheStore_BumpUpstreamCheckedAt_AdvancesTimestampWithoutTouchingBlobOrDigests, plus _NoRowReturnsErrCacheEntryNotFound and _RolledBackTransactionLeavesTimestampUntouched
Entry with no MD5 (FIPS) leaves md5 NULL _NilMD5LeavesColumnNull
Wrapper maps the widened fields to the right columns end to end TestRemoteCacheStore_WriteMethods_ThroughTheSeam (distinct random value per column), TestRemoteCacheStore_WriteMethodsParseAtTheSeam, TestMavenRemoteSHA1Sum

MR size

~3,770 changed lines, over the 500 LOC ceiling in the development model, which asks for a split or a justification here.

Justification: the production surface is ~1,370 lines (maven_remote_cache.go +1020/−25, remote_store.go +292/−25, interfaces.go +20, checksum.go +28/−3, maven_remote_repositories.go +10/−4). The remaining bulk is the same-step test suites the plan template requires (~2,200 lines, most of it integration seed/assert boilerplate — the same pattern Steps 7 and 8 landed — plus the review-driven concurrency and partition-pruning integration suites) and the plan/spec amendments (~200). No split is available that does not separate the write methods from their tests.

End-to-end scenario catalogs

No scenario added or invalidated. docs/testing/ holds no Maven catalog yet — authoring the first one is a separate docs concern, per the plan's testing strategy — and this step's write methods are not wired to any request path (Steps 14–16 wire them through the fill handlers), so there is no observable behavior a scenario could exercise. Step 18's hermetic proxy harness is the automated end-to-end coverage for the S14 paths.

Conformance

No protocol behavior changes: this step is store-layer only and nothing it lands is reachable from a request path yet, so the hosted Maven surface is byte-identical. Conformance for the proxy paths runs with Steps 14–16 and Step 18's harness.

Notes for reviewers

Commit-by-commit:

  • 355682ac4 — the plan amendment pulling the CacheEntry widening into this step (operator-approved; precedes the code commits as declared).
  • 75a739281 / 374f7d09c — the test-first suite and the write methods (the MR's substance).
  • 035ba8ebf — the code-simplifier pass over the write internals.
  • b8b19a25a — the plan amendment deferring downloads_count out of the plan.
  • af2fa4b31, 543c48053 — comment hygiene (symbol citations, stale-comment corrections).
  • 32f06b11d, 75553f7e3 — review-driven test hardening: the distinct-value md5 rewrite guard, the write-path guard/wrap pins, and the through-the-wrapper mapping test.
  • a030bd2e2 — the plan's landed-size record and the corrected pre-tee fail-closed claim.
  • 0b358c94b — hoists one subtest's seeding to the test body so the diff adds no new contextcheck finding under --build-tags=integration (the file's 13 pre-existing findings from Step 8 are unchanged; CI does not lint integration-tagged files).
  • 8d709332c / cfa1e9e9e — the spec annotation and the plan's landed file-set record (this MR's validation fallout).

Pre-push validation (validate-step, five categories): Acceptance and ADR adherence PASS; the three WARNs it raised — plan file-set drift, the new contextcheck finding, and the unamended spec text — are resolved by the last three commits above. Integration lint (golangci-lint --build-tags=integration --max-same-issues=0) is clean for the new code, and the through-the-wrapper integration test passes against testcontainers Postgres.

Related to #286 (closed)

Edited by Moaz Khalifa

Merge request reports

Loading
Loading