fix(oci): Step 15 part 2 - the departed client and the write deadline (S16 plan: 15/26)

Why

Two guards the blob fill path was missing, plus the comment and coverage debt a review of it turned up. Second of the three MRs that make up Step 15.

Both guards are ones the sibling proxy arms already document and implement, and both were found by reviewing part 1 below this one. The arm is inert until !1904 wires it, so no request reached either gap.

The chain

Step 15 ships as three MRs. Part 1 has merged, so this one targets main directly and !1904 targets this branch.

Part MR LOC What
part 3 !1904 837 Wires the arm into the composition root — the MR that turns it on
part 2 this MR 1,499 The guards a review found, plus the comment and coverage fixes
part 1 !1894 (merged) 2,414 The fill path itself. Merged as 15c574e3e

This MR is inert in production. oci.NewRemoteBlobHandler takes the fill seam as an option, and cmd/artifact-registry supplies no WithRemoteBlobFill, so every blob cache miss still answers the interim 501. Nothing reaches the fill path until !1904 supplies the seam, which is what makes it safe to land the arm and its guards separately. !1893 (merged) (the S16 spec amendment this arm implements) and !1896 (merged) (a WriteError fix this work surfaced) have both merged and neither is a code dependency of the rest.

The two guards

A client that left was answered as this service's fault. MapFetchOutcome's doc comment states the obligation on every arm that renders one of its decisions: stamp 499 and write nothing when the request context is already done. The fill path had no such guard. A hangup leaves context.Canceled, which remote.FallbackEligible reports as ineligible, so the mapping classified it 500 INTERNAL and logged at ERROR; where the cache fallback reported ErrUpstreamUnavailable instead, the abort was answered 503 with Retry-After, charging a client's own cancellation to the upstream-availability signal operators alert on.

The guard is remoteReadSubject.writeIfClientClosed, the one the manifest arm already calls at every window of its own, so both arms share one function instead of each carrying its own. It is called at every window on this path, and no one of them covers another:

  • fillBlobMiss at entry pre-empts the streamed answer, which is the only place that can: a streamed GET commits its 200 before it reads a byte.
  • The surface-build fault covers a client leaving while a misconfigured remote's fill surface is being built, which the entry guard cannot see.
  • answerUnfilledBlobMiss covers the client that leaves mid-fill, ahead of the mapping's own cache-fallback query, which would otherwise run on the same dead context.
  • answerUnfilledBlobMiss also re-checks after mapBlobMissFailure, whose cache-fallback consult is a database round trip: a client leaving during it has its cancellation joined into the cause, which is not fallback-eligible, so the same request that answers 503 at Warn while connected would answer 500 at ERROR once departed.
  • serveFilledBlob covers the read-back, both at entry and on its failure. The window that matters is a client leaving during the round trip, which is where the request blocks; the manifest arm's cached-payload read states the same rule.

A fill armed no response write deadline. Go arms the connection write deadline once, at handler entry, at now + server.timeouts.write, which every config in this repo sets to 10s bar one 30s drain-test fixture. A fill outlives that routinely, and the deferred answers outlive it worst: answerCommittedBlobMiss drains an entire upstream layer to io.Discard before it writes anything, so on a cold HEAD or a ranged miss the whole budget could elapse with no byte of the response sent. net/http then buffers the answer, the flush fails, and the client sees the connection close with no status line. That costs most on the response designed to stop the hammering: a 503 with Retry-After that never lands leaves the client retrying straight back into the same stalled upstream.

armBlobFillResponseDeadline is the npm remote reads' armRemoteReadResponseDeadline on this arm, called once in fillBlobMiss ahead of the fetch so one arm covers all three answers. The budget is token_exchange_timeout + request_total_timeout + remoteReadResponseGrace, the same sum WithManifestFill arms and for the same reason: a fill spends the two configured bounds in sequence, because BuildRequestWithAuth's challenge-discovery HEAD and token exchange run to completion before the artifact GET they signed. It carries no coalescing-wait term, which is where it differs from npm's: this arm composes no remote.SingleFlight, so no read waits behind a leader.

WithRemoteBlobFill takes both bounds alongside the seam rather than as options of their own, because a fill with no deadline cannot answer a slow layer at all, so they are one dependency and an arm cannot be handed part of it. Which bound is which cannot change the deadline, since the budget is their sum; each is checked separately so a non-positive one is named in the panic it earns.

Whose truncation it was. blobFillWriteDeadlineKill keeps this deployment's own truncation from being booked as client churn, and logBlobMissStreamFailure is the one list of what reaches each of its five arms. Order is load-bearing: streamRemoteReadBody wraps every non-EOF body error in errRelayUpstreamRead, and the body is internal/remote's cache-fill tee, so a refusal this service raised on the committed content arrives wrapped exactly like an upstream that stopped sending. The two arms keyed on the commit therefore run first.

The armed instant reaches all three answers. The deferred pair drain the layer and then copy from storage, and blobMissFill carries the instant into serveCachedBlob so blobStreamFailureLog can name the party for that copy too. Re-arming after the drain is deliberately not the mechanism: a fresh budget there would unbound how long one request can hold a connection. A warm read arms nothing and passes the zero instant, which the classifier reads as exactly that, so the live cached-read path keeps the churn line it had. #841 carries the same gap on the sibling arm, with the arithmetic for what an armed deadline does not bound.

The upstream arm admits only a live request context. A client hangup cancels the context the body read is bound to, so it arrives in that same shape, and naming the upstream would send an operator to request_total_timeout for an event the client owns. internal/format/npm's remoteTarballClientGone reads the same cancellation off the same side for the same reason.

What this MR states rather than closes

  • A minted 503 takes the relay's floor, not the probe-sweep interval S16 fixes for it. outcome.SelfDecided selects that interval and this arm holds no health-check seam to derive it from, so writeBlobMissOutcome records the divergence and points at #688, whose own text says the wiring is what it tracks. !1904 is where the interval is already in scope, so that is the MR that closes it; the behaviour is unreachable until then. MapFetchOutcome's and UpstreamRetryAfter's docs still describe the interval as the renderer's value, which is true of the manifest arm and not of this one — correcting a sentence inside either block makes the comment-caps gate demand compressing the whole 80- and 22-line block, so both are left for #688.
  • The deadline line is not rationed where npm windows its equivalent for the client that stops reading. That case is a failed write, so it lands on this arm rather than the upstream-read arm above it, and the churn npm's window exists to contain is churn this line still takes at request rate. Rationing needs a counter first — npm increments its own ahead of consulting the window, so no rationing can reach the count — and docs/specs/S16-container-remote.md declares none for this arm, which makes it a metrics-table amendment rather than this MR's to add. Step 17 is where that sits.
  • The budget is not transfer-sized. It is the upstream's bounds plus a tail, not a claim that a large layer to a slow client has a budget of its own, which is what npm's tarball relay says of its own budget too.
  • Two duplications found in review are filed rather than fixed here, because both land outside this MR's subject: #912 (the two write-deadline arming helpers in this package) and #913 (the kill predicate copied per format package, whose callers admit different endings).
  • docs/dev/configuration-reference.md's write row enumerates the code paths that re-arm the write deadline, and this MR adds a third format's. The row is operator-facing and describes reachable behaviour, so it is updated in !1904 along with the rest of the operator docs for this arm rather than here, where the path it would describe answers 501.
  • Both arms' entry cache lookups answer a cancelled read with a 500 and an ERROR and have no guard: the blob arm's in remote_blob_serve.go and the manifest arm's in remote_serve.go, greppable as oci remote blob read: cache lookup failed and its manifest twin. Both are live on main today and sit ahead of any fill, so they are untouched here and filed as #917.

Size

1,499 reviewable LOC, past the 500 that docs/dev/development-model.md asks for a split or a justification at. The split:

Group LOC
remote_blob_test.go 1,163
remote_blob_serve_fixtures_test.go, manifest_internal_test.go 99
remote_blob.go 199
remote_errors.go, remote_relay.go, remote_blob_serve.go 38

A further split does not help. The 237 lines of production code are one behavioural change — the guards, the armed deadline, and the classifier that attributes what it cuts short are the same decision seen from three sides — and splitting them would ship an armed deadline whose truncation reads as client churn, or a classifier with nothing to classify. The 1,262 test lines are the larger half and are reviewable by section: the suite carries banners per concern, and each new test names the arm it drives and was verified to fail without it.

Verification

  • go build ./..., go vet, go test ./internal/format/oci/... -count=1, and go test ./internal/format/oci/ -race: green.
  • golangci-lint run ./internal/format/oci/... with --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false: 0 issues.
  • scripts/ci/check-comment-caps.sh --base origin/main: OK.
  • Every new test was run with its fix reverted and confirmed to fail. Two needed a fixture hook to reach their window at all, because the code re-checks the request context before the call the window sits in: a cancellation staged anywhere but inside the blocked read, or inside the fallback consult, is seen at that earlier check instead.
  • The review's three mutation checks were reproduced before being acted on and are now red: dropping the grace, doubling a bound, and reverting logFieldOCIErrorCode to the flat spelling each left the package green, and each now fails.

No conformance run. The distribution-spec suite drives the hosted OCI surface; this MR changes the remote proxy arm's miss path only, and that path is unreachable until !1904.

No e2e scenario added or affected. docs/testing/ covers behaviour reachable through a running service, and every response this MR changes sits behind the unwired fill seam. The scenarios that cover a wired fill belong with !1904.

Related to #288

Edited by Sylvia Shen

Merge request reports

Loading
Loading