fix(oci): accept streamed blob bodies on PATCH, PUT, and single POST

Hayley Swimelarrequested to merge
1021/oci-blob-upload-chunked-patch into main

Summary

A docker daemon on the classic image store (overlay2, the daemon default) sends each blob as one PATCH with Transfer-Encoding: chunked and no Content-Length. The registry refused every such request with 400 SIZE_INVALID, so those daemons could not push at all. Closed Beta launch blocker, OCI blob upload refuses the chunked PATCH a cla... (#1021 - closed) • Hayley Swimelar.

This MR accepts streamed bodies on PATCH, PUT, and single POST. The non-obvious parts:

  • Session accounting now derives from bytes actually read, on every arm. The new oci.upload.length_declared wide-event field records which framing the client used, so oci.bytes_received is unambiguous on streamed exits.
  • The S12 spec edit rides along. The spec itself mandated the refusal ("Content-Length MUST be present and > 0 when Content-Range is absent"), contrary to the issue's reading, and the S12 DRI is me, so the rule and the handler change together.
  • Body faults are classified by origin, not by error shape. Body-read errors are tagged at the source, so a staging-write io.ErrUnexpectedEOF stays a logged 500 while the body's own is a 400 with its envelope, which a half-closed client still reading receives (on HTTP/1.1 a truncated body and a FIN are the same read). A connection reset or a fired read deadline is a 499, keyed on the tag together with the request context net/http cancels on any connection read error. Chunked framing the reader cannot decode is a 400 where the old code fell through to 500. A non-body error is a 499 only when its chain carries the request's own cancellation, so a CAS loss after a hangup stays a 404 and a staging fault stays a logged 500. A declared PUT whose body truncates now returns 400 where it returned 500. Deliberate: a body shorter than its own Content-Length is a client fault, not a server error.
  • PATCH checks alignment before the cumulative-size rule, as PUT already did, so a misaligned chunk gets the recoverable 416 whatever its span and the size rule's end + 1 is exactly offset + Content-Length. Both pre-read size checks compare against the remaining headroom rather than summing, so a Content-Length near MaxInt64 cannot wrap past them.
  • An empty streamed chunk stays 400 SIZE_INVALID, the same refusal the declared Content-Length: 0 form gets. distribution's registry accepts and clamps instead. S12 now records the divergence: a buffering proxy converts one framing into the other, so the framings must agree.
  • The stub session tears down on a partial-accept ReadFrom like pgSession does, so the handler tests exercise production posture, and the shared storage conformance case pins that posture against both implementations.

2669 reviewable LOC at 1c7a321e: production 611 (upload.go 527, store.go 58, stub/session.go 6, testsuites/chunked_upload.go 20), tests 1995, docs 63. Splitting would not help. The spec rule, the handler, and the test pins are one contract change, and each piece without the others asserts behavior the tree contradicts.

Governing ADRs

ADR-004 (blob limit 50 GB) and ADR-008 (two-phase upload). Both hold: a body with no declared length streams under http.MaxBytesReader set to the session's remaining headroom and gets its 413 mid-read, and the upload flow is unchanged.

Testing

  • Real server: TestUploadPATCH_ChunkedTransferEncodingOverRealServer drives genuine chunked framing (ContentLength -1) through net/http. TestUploadPATCH_ChunkedClientDisconnectOverRealServer sends a real FIN (and reads the 400 envelope back on the half-closed connection) and a real RST mid-chunk, TestUploadPATCH_ChunkedReadDeadlineOverRealServer stalls past a 200 ms read deadline, and TestUploadPATCH_ChunkedMalformedFramingOverRealServer sends a garbage chunk-size line, all through net/http's own reader. All -race -count=10.
  • Conformance: 75 passed / 0 failed / 5 skipped, failure and skip lists identical before and after.
  • Full -race suite green, OCI package re-verified after each rebase.

docs/testing/e2e/docker.md gains the classic-image-store push journey. oci.md deliberately gains no row: the upload handler is format-shared, and the buffering load-balancer variant is already covered there. No catalog scenario covers the fault taxonomy: the catalogs are client journeys, and the taxonomy is observable only through error responses and logs. The containerd DOCKER_DRIVER pin retirement is e2e-harness work on the sahmed/e2e-caproni-authz branch, not this MR.

Spec coverage

# Criterion / error case Tests / proof
AC 1 Streamed PATCH → 202 + Location + Range 0-(N-1); bodyless PUT → 201; blob readable TestUploadPATCH_ChunkedTransferEncodingAccepted
AC 2 Empty streamed chunk → 400 SIZE_INVALID, no Range; GET 204 no Range; retry → 202 TestUploadPATCH_ChunkedEmptyBodyReturns400
AC 3 No Range 0--1 on any PATCH exit streamed 202 / empty 400 / over-cap 413; TestUploadPATCH_OffsetMismatchReturns416, TestUploadPATCH_FirstChunkMismatchOmitsRange
AC 5 oci.bytes_received truthful on every streamed exit TestUploadWideEvent_PatchChunked202, _PatchChunkedOverCap413, _PatchStreamedCloseConflictRecordsBytes, _PutChunked201, _PutStreamedOverCap413, _SinglePostChunked201
AC 5 oci.upload.length_declared marks the framing: absent on initiate and bodyless PUT, true on declared arms, false on streamed; the declared pre-read 413 carries the claim assertions across upload_wideevent_test.go, TestUploadWideEvent_PatchDeclaredOverCap413
AC 6 Declared arms unchanged on the request/response surface, except the finalize client-fault taxonomy (deliberate, both framings) and 416 before 413 on a misaligned chunk pre-existing declared-arm suite, TestUploadPATCH_NoContentRangeMissingLengthReturns400, TestUploadPUT_DeclaredClientDisconnectReturns499, TestUploadPATCH_MisalignedRangeBeyondCapReturns416
AC 8 Real server: ContentLength -1, TE chunked, -race -count=10 TestUploadPATCH_ChunkedTransferEncodingOverRealServer
AC 11 Conformance failure list identical 75 passed / 0 failed / 5 skipped before and after
EC CR without declared length → 400 (PATCH and PUT), recorded as the third deliberate framing divergence TestUploadPATCH_ChunkedWithContentRangeReturns400, TestUploadPUT_ChunkedWithContentRangeReturns400
EC Chunked PUT / single-POST accepted (single POST links the blob exactly once); zero-byte chunked PUT commits empty TestUploadPUT_ChunkedTransferEncodingAccepted, _ChunkedFinalChunkAfterPatch, _ChunkedZeroByteBodyCommitsEmpty, TestUploadPOST_SingleShotChunkedTransferEncodingAccepted
EC Disconnect shapes: FIN → 400 with the short-read envelope (delivered to a half-closed client over net/http), RST and fired read deadline → 499, on declared and streamed bodies, unit and over net/http TestUploadPATCH_ClientDisconnectShapes, TestUploadPATCH_ChunkedClientDisconnectShapes, TestUploadPUT_StreamedClientDisconnectShapes, TestUploadPUT_DeclaredClientDisconnectShapes, TestUploadPOST_SingleShotStreamedClientDisconnectReturns499, TestUpload_ReadDeadlineFiredReturns499, TestUploadPATCH_ChunkedClientDisconnectOverRealServer, TestUploadPATCH_ChunkedReadDeadlineOverRealServer, TestUploadWideEvent_Patch499ClientClosed
EC A hangup after a complete chunk cancels the context mid-persist: a CAS loss stays 404, a staging fault stays a logged 500, a Close the dead context refused is 499, a backend's own cancellation under a live request is a logged 500 TestUploadPATCH_CloseConflictAfterHangupReturns404, TestUploadPUT_StagingFaultAfterHangupReturns500, TestUploadPATCH_CloseRefusedByDeadContextReturns499, TestUploadPATCH_CloseCanceledUnderLiveRequestReturns500
EC Close CAS loss → 404 w/ count; over-cap → 413 mid-read TestUploadWideEvent_PatchStreamedCloseConflictRecordsBytes, TestUploadPATCH_ChunkedOverCapReturns413
EC Short read with the client connected → 400 with the framing-neutral detail (streamed PATCH, streamed PUT, single POST); declared PUT truncation 500→400 TestUploadPATCH_ChunkedTruncatedBodyReturns400, TestUploadPUT_StreamedTruncatedBodyReturns400, TestUploadPOST_SingleShotStreamedTruncatedBodyReturns400, TestUploadPUT_TruncatedBodyReturns400
EC Unreadable body with the client connected (malformed chunked framing) → 400 quoting the reader's reason, unit and over net/http TestUploadPATCH_UnreadableBodyReturns400, TestUploadPUT_StreamedUnreadableBodyReturns400, TestUploadPATCH_ChunkedMalformedFramingOverRealServer
EC Body fault origin: a staging-write io.ErrUnexpectedEOF stays a logged 500 (PATCH and PUT) TestUploadPATCH_BackendShortReadReturns500, TestUploadPUT_BackendShortReadReturns500
EC Content-Length near MaxInt64 on a non-zero offset → 413 pre-read, body unread (PATCH resumable, PUT canceled) TestUploadPATCH_DeclaredLengthNearMaxInt64Returns413PreRead, TestUploadPUT_DeclaredLengthNearMaxInt64Returns413PreRead
EC Streamed finalize client faults: over-cap → 413 (PUT and single POST) TestUploadPUT_StreamedOverCapReturns413, TestUploadPOST_SingleShotStreamedOverCapReturns413
EC Over-cap teardown quiet on a dead request context or a self-terminated session, logged when Cancel fails under a live request whatever its error chain carries TestUploadPUT_StreamedOverCapCanceledCtxCancelQuiet, _StreamedOverCapTerminatedCancelQuiet, _StreamedOverCapCancelFailureLogsError, _StreamedOverCapCancelTimeoutLogsError
EC Partial accept tears the session down on both storage implementations: 404 after a streamed 413 or a short-read 400 testsuites.testReadFromReaderError (stub and pg), TestUploadPATCH_ChunkedOverCapReturns413, TestUploadPATCH_TruncatedBodyReturns400
EC Session filled to exactly the cap: fill → 202, next chunk → 413, empty chunk at cap → 400 TestUploadPATCH_ChunkedSessionAtCap
EC Framing equivalence (deterministic + property) TestUploadPATCH_DeclaredAndStreamedFramingAgree, TestUploadPATCH_ChunkedAfterDeclaredChunk, TestPropertyChunkedReassembly
Context for LLM agents

Rationale

  • Match distribution's zero-byte chunk behavior (202 with Range: 0-0). Rejected: the declared form (Content-Length: 0) was always refused, and a buffering proxy converts a streamed chunk into a declared one, so the framings must agree on the refusal. S12 records the divergence.
  • Change only the handler and leave the spec, per the issue's claim that the spec already permitted streaming. Rejected: the spec mandated the refusal, so rule and handler change together.
  • Classify body faults by error shape (errors.Is on io.ErrUnexpectedEOF, context.Canceled). Rejected: a GCS short read carries the same io.ErrUnexpectedEOF as a truncated body, and a real disconnect never carries context.Canceled from the body read (net/http returns io.ErrUnexpectedEOF for a FIN and a *net.OpError for an RST, and cancels the request context in both cases). Chosen: tag body-read errors at the source.
  • Key the 499 on the request context alone. Rejected after measurement: net/http also cancels the context when a client hangs up after a complete body (the background read sees the FIN), so a context-only 499 swallowed a CAS-loss 404 and a staging 500. Chosen: the 499 needs the body tag (the read itself failed) or the request's own cancellation in the error chain.
  • Answer a FIN mid-body with 499. Rejected: on HTTP/1.1 a truncated body and a half-close are the same read, and a half-closed client still reading would lose its envelope. The short-read 400 the pre-existing PATCH arm already gave stays, measured over net/http.
  • Accept Content-Range on a length-less chunk and verify the span after the read. Rejected for this MR: a mismatch found after the bytes are staged costs the session, and the refusal is the shipped behavior. Recorded as the third deliberate framing divergence, with the topology dependence stated.
  • Chosen: accounting from bytes actually read on every arm, with oci.upload.length_declared marking the framing on each upload wide event.

Consequences: an undeclared over-cap body streams up to the session's remaining headroom into staging before its 413, and that partial accept tears the session down. A request context canceled by server shutdown mid-body-read also exits 499, by design: the client cannot be answered either way.

Non-goals

Related to #1021 (closed)

Edited by Hayley Swimelar

Merge request reports