Repository navigation
Add handling of timeouts to ApiWebRequest - #9144
Merged
andrewlock merged 7 commits intoOct 22, 2026
Merged
Conversation
Merged
andrewlock
force-pushed
the
andrew/web-request-timeout
branch
from
August 27, 2026 15:43
ca71479 to
89b5894
Compare
andrewlock
changed the title
ApiWebRequestApiWebRequest
andrewlock
added
type:bug
area:tracer
Member
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89b58945db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
BenchmarksBenchmark execution time: 2026-10-22 10:29:53 Comparing candidate commit 7211fc7 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 71 metrics, 1 unstable metrics, 6 known flaky benchmarks, 120 flaky benchmarks without significant changes.
|
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
from
August 27, 2026 17:16
8c9cb73 to
388d2f2
Compare
andrewlock
force-pushed
the
andrew/web-request-timeout
branch
from
August 27, 2026 17:16
89b5894 to
c221aa9
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9144) and master. ✅ No regressions detected |
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
from
August 27, 2026 19:21
388d2f2 to
db88bf0
Compare
andrewlock
force-pushed
the
andrew/web-request-timeout
branch
from
August 27, 2026 19:21
c221aa9 to
0e63bc8
Compare
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
andrewlock
marked this pull request as ready for review
August 28, 2026 15:35
NachoEchevarria
left a comment
NachoEchevarria
left a comment
Collaborator
There was a problem hiding this comment.
Left one minor comment. Thanks!
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
from
October 19, 2026 14:21
db88bf0 to
55df10d
Compare
andrewlock
force-pushed
the
andrew/web-request-timeout
branch
from
October 19, 2026 14:21
01a2e42 to
7645af1
Compare
NachoEchevarria
approved these changes
Oct 19, 2026
Member
Author
|
/azp run |
ApiWebRequest now always enforces HttpWebRequest.Timeout for async sends, so the opt-in flag and the test asserting its opt-in behaviour are obsolete. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a race where the Abort() callback could file shortly after returning a "success" response
…eadline Without this, you can cause a request to hang forever if the write body hangs, and can also fail to unblock a hanging GetResponseAsync(). Make it safe.
andrewlock
force-pushed
the
andrew/refactor-web-request
branch
from
October 22, 2026 09:38
55df10d to
8a2cfee
Compare
andrewlock
force-pushed
the
andrew/web-request-timeout
branch
from
October 22, 2026 09:38
7feec82 to
7211fc7
Compare
andrewlock
changed the base branch from
andrew/refactor-web-request
to
andrew/remove-async-timeout-flag
October 22, 2026 09:38
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
More details
The timeout intentionally covers sending the request and receiving response headers; response-body reads remain outside its scope. Runtime verification was blocked because the checkout requires .NET 11, while the sandbox has .NET 8.
🤖 Bits Code Review · Commit 7211fc7
andrewlock
added a commit
that referenced
this pull request
Oct 22, 2026
) ## Summary of changes Removes the opt-in `enforceAsyncTimeout` flag (and the unused `_asyncTimeout` plumbing) from `ApiWebRequestFactory`/`ApiWebRequest`, its callers (`AgentTransportStrategy`, `FeatureFlagsEvpTransport`, tests), and deletes `ApiWebRequestFactoryTests.AsyncDeadlineAbortsStalledJsonSendOnlyWhenEnabled`. ## Reason for change #9235 added an opt-in `Timer` + `Abort()` deadline for async `HttpWebRequest` sends. The refactor in #9145 replaces the send path, and #9144 enforces `HttpWebRequest.Timeout` for every async send, so the flag no longer does anything. The deleted test asserted behaviour specific to the opt-in design: it disables `HttpWebRequest.Timeout`, expects a separate deadline to fire anyway, and expects a `WebException(RequestCanceled)`. The previous implementation had multiple issues: - Body writer stuck on something other than the request stream would hang indefinitely - Race condition when response returned just as the timer fires, killing a response that's already returned - Orphaned GetResponseAsync that succeeds after the abort is not handled ## Implementation details Sits between #9145 (base) and #9144 in the stack. ## Test coverage The deadline behaviour stays covered by: - `ApiRequestTests`, added in #9144 - `FlagEvaluationTransportTests.SlowTcpAgentTimesOutWithoutReplayingBatch` - `ExposureApiTimeoutTests.LegacyExposureRequestsAbortStalledConnection` On their own, #9145 and this PR have no async deadline on .NET Framework, so `SlowTcpAgentTimesOutWithoutReplayingBatch` fails on net48 until #9144 is applied. With the full stack applied, all transport and Feature Flags timeout tests pass locally on net48 and net10.0. ## Other details Merge together with #9144. Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Base automatically changed from
andrew/remove-async-timeout-flag
to
andrew/refactor-web-request
October 22, 2026 10:22
Contributor
There was a problem hiding this comment.
More details
Focused .NET 8 checks covered successful responses, HTTP errors, and cancellation without exposing a regression. Execution on .NET Framework, the primary affected runtime, remains unverified.
🤖 Bits Code Review · Commit 7211fc7
andrewlock
added a commit
that referenced
this pull request
Oct 22, 2026
## Summary of changes Refactoring of `ApiWebRequest` logic in preparation for follow up PR ## Reason for change I needed to do some work to add timeouts in #9144, and that required doing some cleanup first. Extracted here for simplicity of reviews ## Implementation details Commits can most easily be reviewed individually: - Avoid creating the multipart upload arrays every time we send a request, instead cache them. - Moved to a separate time to avoid creating them in cases where we never do a multipart upload - We need to listen for `OperationCancelledException` as well to know if a failure was a timeout (for telemetry purposes) - Reduce duplication in `ApiWebRequest` by pushing through a central `SendAsync()` - Add `#nullable enable` - Fix ci vis violations ## Test coverage Just a refactor, so basically covered by existing tests ## Other details Required by #9144 --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
Summary of changes
Fixes timeout handling for
ApiWebRequestReason for change
Basically, timeouts never worked in our
ApiWebRequest, because the APIs we call (GetRequestStreamAsync/GetResponseAsync) simply don't pay attention to it. That means we effectively had no timeout on any http calls for .NET Framework.Implementation details
Actually handle a timeout in
ApiWebRequest. Technically this still isn't quite the same as ourHttpClientimplementations, because forHttpClientwe're using a mode that buffers everything in memory immediately, and applies the timeout including that. That's more effort to do withApiWebRequest, as you need to explicitly pass the cancellation token through to the response etc, which all becomes kinda different toHttpClient. Ultimately, what this adds is better than what we have todayNote: My assumption is that we will not want to buffer everything in memory soon, seeing as we're always explicitly copying the body or stream decoding it. Today, we are doing more copying than we should. I expect we'll look at using
HttpCompletionOption.ResponseHeadersReadinstead soon, in which case, we will need to unify our handling of timeouts in any case. A follow up process.Test coverage
Added unit tests for the timeouts (which hopefully are not flaky), given I'm cheating by passing in a deterministic cancellation token instead of trying to "wait for it to fail". There's a bunch of edge cases around exactly when the timeout is cancelled which we handle. There's the occasional additional edge case that we can't test without a lot more work, so left that to manual testing instead
Other details
Stacked on:
ApiWebRequest#9145Discovered as part of: