Repository navigation
fix: stream multipart request bodies - #19
2830500285 wants to merge 1 commit into
Conversation
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
Cleanup stops at the first upload whose Close() returns an error. With multiple multipart files, that means one failing reader prevents every later fileUpload from being closed, leaking the remaining owned readers on exactly the error path this PR is trying to clean up. Could we attempt all closes and return/join the errors afterwards? A two-file test with the first close failing would pin it.
Closes openai#18. This replaces openai#19 with a current-main implementation. ## Root cause The CLI opened file-backed values as readers, then synchronously copied those readers into a `bytes.Buffer` while constructing the multipart request option. Large uploads were therefore fully buffered before the request was sent, and CLI-owned file handles were not reliably closed. ## Design decision Multipart forms containing uploads are intentionally single-attempt streams: - encode lazily through `io.Pipe` with bounded memory - set `WithMaxRetries(0)` because an arbitrary reader cannot be guaranteed to produce the same bytes twice - return an explicit error for POST 307/308 responses that require replay - do not create upload-sized temporary snapshots or duplicate descriptors - retain `Content-Length` for trustworthy regular-file sizes; use chunked transfer for unknown-length readers Scalar-only multipart forms remain buffered and retain the SDK's normal retry behavior. This is a deliberate fail-closed boundary. Transparent upload replay would require either buffering/spooling the body or a first-class SDK replay contract. Reimplementing that retry state machine in CLI middleware introduced substantially more lifecycle and platform risk than the original memory/descriptor problem. A network failure during an upload now returns the original transport error after one attempt instead of retrying a body whose identity cannot be guaranteed. ## Correctness and cleanup - source read failures abort the pipe without emitting a successful terminal multipart boundary - exact-length regular-file readers report premature EOF if a file shrinks during the request - owned upload files close exactly once across success, encoding failure, cancellation, close-before-read, and option-construction failure - stdin and other unowned readers are not closed - extensionless `@value` fallback occurs only for genuine not-found errors ## Impact In the original local 128 MiB end-to-end reproduction, peak RSS dropped from approximately 348 MiB on `main` to approximately 26 MiB while the complete multipart payload reached the server. Behavioral tradeoff: multipart requests containing uploads no longer receive automatic SDK retries. Scalar-only multipart requests are unchanged. ## Validation - focused multipart regressions, including 307/308, 429/5xx single-attempt behavior, incomplete transport failures, Content-Length/chunked framing, premature EOF, cleanup, unavailable temp directories, low descriptor limits, and actual CLI receivers - focused race suite - stress runs for cancellation, incomplete transport failure, redirect rejection, and source shrinkage - full `./scripts/test` native suite and Windows compile gate - `./scripts/lint` - `./scripts/build` - `go vet ./...`
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Thanks for checking this against current main. If #65 closes every upload even when one close fails, that resolves the cleanup concern I raised here. |
4 participants
Closes #18.
Summary
io.Pipeinstead of buffering the full encoded payload in memoryfileUploadreaders after multipart encoding completesValidation
go test ./internal/apiform ./pkg/cmd -run 'TestEncode|TestMultipartRequestBodyStreamsAndClosesUploads|TestEmbedFiles|TestEmbedFilesStdin|TestEmbedFilesUploadMetadata'go test ./... -run 'TestEncode|TestMultipartRequestBodyStreamsAndClosesUploads|TestEmbedFiles|TestEmbedFilesStdin|TestEmbedFilesUploadMetadata'go build ./...