fix(remote): standalone serve suite and upstream response release (S13 Step 11, part 3/3)

David Fernandezrequested to merge
10io/s13-step-11-standalone-serve-part3of3 into main

📦 What this MR does

Part 3 of 3 of S13 virtual/remote foundation — Step 11: standalone remote request.

Parts 1/3 (!1290 (merged)) and 2/3 (!1291 (merged)) have merged, so this branch is rebased onto main and targets it directly.

It completes the Standalone.Serve suite beyond part 2/3's one-test-per-acceptance-row core, and carries one production fix raised in review (see Review follow-ups below).

🎯 What it covers

  • Leader-loss retry choreography: TestStandalone_Serve_LeaderCancellationRetriesOnce and TestStandalone_Serve_LeaderAbandonmentRetriesOnce pin the retry-once decision for both sentinels a single leader disconnect can surface as, so the outcome does not depend on race ordering.
  • Vanished-blob recovery budget: TestStandalone_Serve_FreshHitVanishedBlobRefetches, TestStandalone_Serve_RevalidatedBlobVanishedRefetches, TestStandalone_Serve_RevalidatedBlobVanishedTwiceIsCacheFill, and TestStandalone_Serve_FallbackBlobVanishedIsUnavailable — exactly one unconditional re-fetch per Serve, on every path — with TestStandalone_Serve_FreshHitIntegrityFaultFails pinning the split the recovery must not blur.
  • Non-transport error partition: TestStandalone_Serve_NonTransportFetchErrorsPropagate, TestStandalone_Serve_SingleFlightTimeoutPropagates, TestStandalone_Serve_RequestBuilderFailurePropagates, TestStandalone_Serve_LookupErrorPropagates, and TestStandalone_Serve_Upstream5xxNeverServesCache — local failures and deliberate upstream answers never route through the cache fallback.
  • Health-read degrade logging: TestStandalone_Serve_HealthReadAbandonedLogsDebug — the gate proceeds on an unreadable status, WARN for a degraded read, DEBUG when the caller's own context ended. Both health-read tests pin level and message together on one record: asserting the level alone passes on any record of that level the flow emits, and the fetch and cache paths log too.
  • Coalescing fidelity: TestStandalone_Serve_Follower304WithRefilledRowServes — a 304 vouches for the row, not the caller's pre-fetch snapshot.
  • Composed-flow coalescing proof: TestStandalone_Serve_CoalescesConcurrentServesOverRealClient — N concurrent Serves over the real upstreamhttp.Client and Doer (not fakes) produce exactly one upstream GET and one cache write, the proof Step 8 assigned to the first composition that wires the single flight into a flow.

Three tests part 2/3 already landed grow rather than gaining a near-duplicate beside them:

  • The two unusable-upstream-response tables gain the undecoded-content-coding row. It reaches the same class by a different route: the 204/206 and 304 rows are rejected on the response's own shape, while an encoded 200 is a nominally fine response whose body the client cannot use.
  • TestStandalone_Serve_StaleHitETagDiffersRefills gains the refilled row's blob reference, plus distinct stale and fresh bodies so that assertion is not vacuous.

🔨 Review follow-ups

  • Duo — response leak in upstreamhttp.Doer.Do. Fixed. A response returned alongside an error is now drained and closed through drainAndClose, the package's existing disposal for a body no caller will read — the same drain-then-close Fetcher.fetchOnce applies on the other side of the seam, so the seam has one policy rather than one per direction. Client.Do returns no such response — it cancels and returns a nil response on its single error path — so no live leak exists today, but remote.UpstreamDoer constrains only the nil-error shape. Doer now holds its client as an unexported interface so the release can be driven directly; NewDoer still takes the concrete *Client.
  • Duo — recordDownload reusing the 30s detached-write timeout. Already closed on main by part 2/3's own review: detachedDownloadBumpTimeout is its own 5s constant, with the reasoning Duo asked for recorded on it.
  • Review — the doer's disposal policy and the invariant behind it. Fixed. The release drains before closing (above), and TestClient_Do_ErrorReturnsNoResponse pins the invariant that made the branch unreachable — Client.Do reports no error with a response attached — over the assembled client, across every route into its error path. Until now that invariant lived only in a doc comment.
  • Review — concurrency scaffolding in the standalone suite. Fixed. The three coordination handshakes are bounded by awaitSignal, so a flow that never reaches the site closing the channel fails with a named message instead of hanging until the package timeout. The single-flight timeout test registers its slot-holder release with t.Cleanup(sync.OnceFunc(...)), so a failed assertion cannot strand that goroutine with its body open.
  • Review — plan-step coordinate in a test doc comment. Fixed. The composed-coalescing test's comment now states what the test proves and why no other test in the file can prove it.
  • The AppSec review is answered in its own thread.

✅ Testing

go test ./internal/remote/... -race passes, and golangci-lint run ./internal/remote/... is clean. The Doer fix is pinned by TestDoer_Do_ReleasesResponsePairedWithError, which asserts the disposal order and fails on a bare Close, and by TestClient_Do_ErrorReturnsNoResponse, which holds the invariant that keeps the released branch unreachable from the assembled client.

No e2e scenario catalog entry is added or affected: the Doer change guards a shape the assembled client cannot produce, so no client-observable behavior changes.

📖 References

:bricks: Stacked MRs (review/merge bottom-up)

Related to #332 (closed)

Edited by Dzmitry (Dima) Meshcharakou

Merge request reports