Skip to content

fix(client): preserve typed *HTTPStatusError when ctx is cancelled at Close#291

Merged
worstell merged 1 commit intomainfrom
fix/preserve-status-error-on-close
May 5, 2026
Merged

fix(client): preserve typed *HTTPStatusError when ctx is cancelled at Close#291
worstell merged 1 commit intomainfrom
fix/preserve-status-error-on-close

Conversation

@worstell
Copy link
Copy Markdown
Contributor

@worstell worstell commented May 5, 2026

writeCloser.Close discards the value from <-wc.done whenever wc.ctx.Err() is 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 what happens when the server denies a write mid-stream.

The fix:

  • Drain the upload pipe (CloseWithError on cancel, Close otherwise) so the request goroutine can finish.
  • Always wait on <-wc.done so we can inspect the server's response.
  • 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.

Restores the contract callers depend on: errors.As(err, &*HTTPStatusError{}) works after Save even 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. 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 a 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.

… 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.
@worstell worstell force-pushed the fix/preserve-status-error-on-close branch from f3fd538 to 40e459c Compare May 5, 2026 18:17
@worstell worstell marked this pull request as ready for review May 5, 2026 18:23
@worstell worstell requested a review from a team as a code owner May 5, 2026 18:23
@worstell worstell requested review from stuartwdouglas and removed request for a team May 5, 2026 18:23
@worstell worstell merged commit 63e9b4f into main May 5, 2026
9 checks passed
@worstell worstell deleted the fix/preserve-status-error-on-close branch May 5, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants