Repository navigation
fix: abort incomplete streamed responses - #245
Open
perfloop[bot] wants to merge 2 commits into
Open
perfloop[bot] wants to merge 2 commits into
perfloop[bot] wants to merge 2 commits into
Conversation
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
Signed-off-by: Perfloop Agent <agent@perfloop.ai>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
An upstream body-copy error could previously leave the client with a clean success containing only a prefix. TAG now surfaces the error and aborts a committed direct-streaming response.
The abort happens after operation bookkeeping. A received upstream 2xx still triggers PUT/DELETE cache invalidation or the tiered PUT marker. The write-through tee remains reserved until its asynchronous cache write releases the reservation. HTTP/1.0 uses an abortive TCP close because normal connection close completes an unknown-length body; HTTP/1.1 and HTTP/2 use the standard abort path.
The routed signing-mode ListBuckets test covers HTTP/1.0, HTTP/1.0 over TLS, HTTP/1.1, and HTTP/2 with a chunked upstream 200 missing its terminal chunk. Complete known-length and unknown-length responses, HTTP/1.0 complete responses, and pre-header signature failures remain controls. Response-capture operations use a separate path and are not changed. Production frequency and SDK-specific effects remain unmeasured.
The recorded repair comparison is violated at
d460159ba9d215c83b4d5c344b8388618c60face; the final assertion is satisfied. Reproduce it withsh proof/verify_routed_truncation.sh.Validation included
make build,make test,make test-integration,make test-race,make test-coverage, andmake lint-ci.Recorded proof ran with retained support that is not included in this pull request.
Other target checks are S3 Compatibility Tests / s3-compat-tests (default), S3 Compatibility Tests / s3-compat-tests (tiered), S3 Compatibility Tests (Cluster) / s3-compat-tests-cluster (default), Lint PR Title / lint, and CLA Assistant / cla.
Generated by Perfloop. Measurements and checks. Reproduce this comparison (tenant access required).
Note
Medium Risk
Changes committed-response handling for all direct-streaming paths and alters when PUT/DELETE invalidate cache relative to client-visible errors; incorrect abort logic could break clients or leave stale cache entries.
Overview
Fixes a case where a mid-stream upstream body failure could still look like a successful HTTP response to clients. The forwarder now returns body-copy errors after status and headers are already sent, and
handleWithErroraborts committed direct-streaming responses (TCP reset on HTTP/1.0/TLS,http.ErrAbortHandlerotherwise) instead of appending S3 error XML or finishing framing around a prefix.Post-forward mutation bookkeeping no longer treats a delivery error as “forward failed”: when upstream already returned 2xx, PUT/DELETE still run cache re-invalidation, tiered upstream markers, and write-through tee work keyed off
statusRecorder, while clients may still see a transport/body-read failure. Docs add a Response delivery section describing this split.Adds integration tests for truncated ListBuckets across HTTP/1.0, TLS, HTTP/1.1, and HTTP/2, plus unit tests for forwarder meta-on-copy-error and invalidation/marker behavior when the response body does not fully reach the client.
Reviewed by Cursor Bugbot for commit 712e2b5. Bugbot is set up for automated code reviews on this repo. Configure here.