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:
fillBlobMissat entry pre-empts the streamed answer, which is the only place that can: a streamedGETcommits its200before 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.
answerUnfilledBlobMisscovers the client that leaves mid-fill, ahead of the mapping's own cache-fallback query, which would otherwise run on the same dead context.answerUnfilledBlobMissalso re-checks aftermapBlobMissFailure, 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 answers503at Warn while connected would answer500at ERROR once departed.serveFilledBlobcovers 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
503takes the relay's floor, not the probe-sweep interval S16 fixes for it.outcome.SelfDecidedselects that interval and this arm holds no health-check seam to derive it from, sowriteBlobMissOutcomerecords 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 andUpstreamRetryAfter'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.mddeclares 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'swriterow 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 answers501.- Both arms' entry cache lookups answer a cancelled read with a
500and anERRORand have no guard: the blob arm's inremote_blob_serve.goand the manifest arm's inremote_serve.go, greppable asoci remote blob read: cache lookup failedand its manifest twin. Both are live onmaintoday 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, andgo 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
logFieldOCIErrorCodeto 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