Skip to content

fix(csharp): always clone request before send to prevent ObjectDisposedException on retry#16459

Merged
patrickthornton merged 4 commits into
mainfrom
patrick/csharp/fix-retry-object-disposed
Jun 11, 2026
Merged

fix(csharp): always clone request before send to prevent ObjectDisposedException on retry#16459
patrickthornton merged 4 commits into
mainfrom
patrick/csharp/fix-retry-object-disposed

Conversation

@patrickthornton

@patrickthornton patrickthornton commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Closes FER-11195

Summary

  • Basis-Theory hit ObjectDisposedException: Cannot access a disposed object. Object name: 'System.Net.Http.StringContent' from RawClient.CloneRequestAsync on the first retry of a JSON request.
  • Root cause: SendWithRetriesAsync sent the original HttpRequestMessage for the initial attempt and only cloned for subsequent retries. Under HTTP/2 (and some other configurations), HttpClient disposes the Content of every request it sends, so the original's Content was already disposed by the time the retry loop tried to clone it.
  • Fix: restructure the retryable-content branch to always send a clone — never the original. Cloning each attempt keeps the original Content alive across the entire retry loop.
  • Plus four review-driven additions: clamp negative MaxRetries to 0; short-circuit MaxRetries == 0 and non-retryable content to a no-clone fast path; thread CancellationToken into CloneRequestAsync's body buffering; new multi-retry regression test using a DelegatingHandler that disposes request.Content after send.

Customer stack trace

System.ObjectDisposedException: Cannot access a disposed object.
Object name: 'System.Net.Http.StringContent'.
   at System.Net.Http.HttpContent.CheckDisposed()
   at System.Net.Http.HttpContent.CopyToAsync(Stream stream, TransportContext context)
   at BasisTheory.Client.Core.RawClient.<CloneRequestAsync>d__12.MoveNext()
   at BasisTheory.Client.Core.RawClient.<SendWithRetriesAsync>d__13.MoveNext()
   at BasisTheory.Client.Core.RawClient.<SendRequestAsync>d__10.MoveNext()
   at BasisTheory.Client.ReactorsClient.<ReactAsyncCore>d__10.MoveNext()

Before vs after

Before — original sent on attempt 0, cloned only for retries; first send disposed the original's Content:

var response = await httpClient.SendAsync(request, ...);   // (A) ORIGINAL — HTTP/2 disposes its Content
// ...
for (var i = 0; i < maxRetries; i++) {
    if (!ShouldRetry(response)) break;
    using var retryRequest = await CloneRequestAsync(request);   // (B) original.Content already disposed → throws
    response = await httpClient.SendAsync(retryRequest, ...);
}

After — fast path for opt-out cases; always-clone loop for retryable content:

var maxRetries = Math.Max(0, options?.MaxRetries ?? Options.MaxRetries);
var isRetryableContent = IsRetryableContent(request);

if (!isRetryableContent || maxRetries == 0) {
    // Fast path: send the original directly, no clone.
    var response = await httpClient.SendAsync(request, ...);
    return new ApiResponse { ... };
}

// Always send a clone — never the original.
HttpResponseMessage? response = null;
for (var attempt = 0; attempt <= maxRetries; attempt++) {
    if (attempt > 0) {
        var delayMs = GetRetryDelayFromHeaders(response!, attempt - 1);
        await SystemTask.Delay(delayMs, cancellationToken);
    }
    using var attemptRequest = await CloneRequestAsync(request, cancellationToken);
    response = await httpClient.SendAsync(attemptRequest, ...);
    if (!ShouldRetry(response)) break;
}

Why our existing test missed it

SendRequestAsync_ShouldPreserveJsonBody_OnRetry (added in the prior fix #12914) exercises POST JSON → 500 → 200 against WireMock — exactly the failing scenario. But WireMock loopback HTTP/1.1 + SocketsHttpHandler doesn't dispose request.Content after SendAsync returns, so the test passes whether the bug is present or not. I verified this with a diagnostic DelegatingHandler that captured the disposal state.

The new regression test wraps HttpClientHandler in a ContentDisposingHandler that explicitly disposes request.Content after base.SendAsync returns — reproducing production HTTP/2 behavior reliably. Confirmed: without this PR's fix, the new test throws the exact customer stack; with the fix, all 16 RetriesTests pass.

Test plan

  • Local: ported template into seed/csharp-sdk/bytes-upload, ran dotnet test --filter RetriesTests → 16/16 pass
  • Reverted the fix temporarily, re-ran the new regression test → fails with ObjectDisposedException at CloneRequestAsync → CopyToAsync (matches customer stack exactly)
  • Re-applied the fix, all 16 tests pass
  • CI: update-seed.yml regenerates all 136 csharp-sdk fixtures, runs the full test suite
  • Verify with Basis-Theory after release (their reproduction is non-local — they were unable to repro outside production)

🤖 Generated with Claude Code


Open in Devin Review

…edException on retry

Restructure RawClient.SendWithRetriesAsync so the retryable-content path always
sends a clone, never the original HttpRequestMessage. Under HTTP/2, HttpClient
disposes request.Content after sending, which broke CloneRequestAsync on retry.

Also: clamp negative MaxRetries to 0; short-circuit MaxRetries=0 and
non-retryable content through a no-clone fast path; thread CancellationToken
into CloneRequestAsync's body buffering.
@patrickthornton patrickthornton self-assigned this Jun 10, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Seed test outputs not updated to reflect template changes

The REVIEW.md states: "When a template file changes, corresponding seed test outputs should also be updated (either via seed test or bulk update)." The RawClient.Template.cs and RetriesTests.Template.cs templates were modified, but the generated copies in seed/csharp-sdk/*/src/*/Core/RawClient.cs and seed/csharp-sdk/*/src/*.Test/Core/RawClientTests/RetriesTests.cs still contain the old code (e.g., no Math.Max, no ContentDisposingHandler class, no cancellation token in CloneRequestAsync). These should be regenerated before merge.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

SDK Generation Benchmark Results

Comparing PR branch against median of 5 nightly run(s) on main (latest: 2026-06-10T05:30:02Z).

Full benchmark table (click to expand)
Generator Spec main (generator) main (E2E) PR (generator) Delta
csharp-sdk square 69s (n=5) 110s (n=5) 63s -6s (-8.7%)

main (generator): generator-only time via --skip-scripts (includes Docker image build, container startup, IR parsing, and code generation — this is the same Docker-based flow customers use via fern generate). main (E2E): full customer-observable time including build/test scripts (nightly baseline, informational). Delta is computed against generator-only baseline.
⚠️ = generation exited with a non-zero exit code (timing may not reflect a successful run).
Baseline from nightly runs on main (latest: 2026-06-10T05:30:02Z). Trigger benchmark-baseline to refresh.
Last updated: 2026-06-10 22:33 UTC

…thRetriesAsync

The fast-path block declares `var response` inside the `if (!isRetryableContent
|| maxRetries == 0)` branch; the retry loop declared `HttpResponseMessage?
response = null` in the enclosing method body. C# treats the entire method as
one declaration space, so the two `response` locals collide with CS0136 even
though their scopes don't overlap. Rename the outer one to `retryResponse`.
Regenerates RawClient.cs and RetriesTests.cs (plus .fern/metadata.json bumps)
for all 178 csharp-sdk fixtures to pick up the SendWithRetriesAsync rewrite,
the CloneRequestAsync CancellationToken plumbing, and the new
ContentDisposingHandler-based regression tests.
@fern-support fern-support requested a review from amckinney as a code owner June 10, 2026 21:52
…etries code

The verbose multi-line comments in SendWithRetriesAsync and RetriesTests ship
into every generated SDK and multiply across all customer repos. Reduce the
load-bearing WHY to two lines in RawClient and one each in the regression
tests; the rest is in the changelog and the git history.
@patrickthornton patrickthornton merged commit e8f7a95 into main Jun 11, 2026
98 of 117 checks passed
@patrickthornton patrickthornton deleted the patrick/csharp/fix-retry-object-disposed branch June 11, 2026 03:41
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