Conversation
… Close
writeCloser.Close used to discard the value on <-wc.done whenever
wc.ctx.Err() was non-nil, returning only 'create operation cancelled:
context canceled'. This masks an authoritative server response (e.g.
403 Forbidden) when the response arrives before the upload pipe is
torn down — exactly the situation that occurs when the server denies
a write mid-stream.
Drain the upload pipe (CloseWithError on cancel, Close otherwise),
always wait for the request goroutine via <-wc.done, then prefer the
typed *HTTPStatusError when present. Fall through to the local ctx /
pipe-close error only when the server didn't send a typed status.
This restores the contract callers depend on: when Save fails because
of a 403, errors.As on *HTTPStatusError returns true, so the caller
can correctly classify the failure as a permission denial instead of
a generic error.
While the cancel path is being reshaped, two adjacent issues get
fixed in the same change so we don't carry them forward:
* Abort(cause) now propagates the caller's cause. Close reads
context.Cause(ctx) instead of ctx.Err(), so the pipe is closed
with — and the wrapped error contains — the cause passed to
Abort rather than a generic context.Canceled. When the parent
context cancels independently, Cause falls back to ctx.Err so
behaviour matches the previous code in that path.
* Close is now idempotent under concurrent calls. The previous
'closed bool' guard was racy and, more importantly, the second
caller would deadlock on the drained <-wc.done channel — a real
risk under patterns like deferred Close racing an explicit Abort.
Replaced with sync.Once so the close-and-wait runs at most once
and the second caller returns the cached error immediately.
Note: callers that match the literal string 'create operation
cancelled' to detect a cancelled-with-server-response Close will now
see 'request failed: ...' instead. This is intentional — the
documented contract is errors.As against *HTTPStatusError.
Tests:
* TestCreateClosePreservesStatusErrorOnCancelledCtx — controllable
RoundTripper deterministically reproduces the masked-403 race
(response delivered, then ctx cancelled, then Close). Without the
fix the test fails with the exact error string seen in production.
* TestCreateCloseIdempotent — second Close call must return without
blocking on the drained done channel.
f3fd538 to
40e459c
Compare
alecthomas
approved these changes
May 5, 2026
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.
writeCloser.Closediscards the value from<-wc.donewheneverwc.ctx.Err()is non-nil, returning onlycreate operation cancelled: context canceled. This masks an authoritative server response (e.g.403 Forbidden) when the response arrives before the upload pipe is torn down — exactly what happens when the server denies a write mid-stream.The fix:
CloseWithErroron cancel,Closeotherwise) so the request goroutine can finish.<-wc.doneso we can inspect the server's response.*HTTPStatusErrorwhen present; fall through to the local ctx / pipe-close error only when the server didn't send a typed status.Restores the contract callers depend on:
errors.As(err, &*HTTPStatusError{})works afterSaveeven when the response races a context cancellation.While the cancel path is being reshaped, two adjacent issues are fixed in the same change so we don't carry them forward:
Abort(cause)now propagates the caller's cause.Closereadscontext.Cause(ctx)instead ofctx.Err(), so the pipe is closed with — and the wrapped error contains — the cause passed toAbortrather than a genericcontext.Canceled. When the parent context cancels independently,Causefalls back toctx.Errso behaviour matches the previous code in that path.Closeis now idempotent under concurrent calls. The previousclosed boolguard was racy and, more importantly, the second caller would deadlock on the drained<-wc.donechannel — a real risk under patterns like a deferredCloseracing an explicitAbort. Replaced withsync.Onceso the close-and-wait runs at most once and the second caller returns the cached error immediately.Note: callers that match the literal string
create operation cancelledto detect a cancelled-with-server-responseClosewill now seerequest failed: ...instead. This is intentional — the documented contract iserrors.Asagainst*HTTPStatusError.Tests
TestCreateClosePreservesStatusErrorOnCancelledCtx— controllableRoundTripperdeterministically reproduces the masked-403 race (response delivered, then ctx cancelled, thenClose). Without the fix the test fails with the exact error string seen in production.TestCreateCloseIdempotent— secondClosecall must return without blocking on the drained done channel.