Stop retrying http requests when script invocation result is failed #11210
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR stops retrying HTTP requests when the associated script invocation has failed elsewhere in the system (e.g., worker channel shutdown, host-tracked function timeout). It implements this by passing the script invocation context through the HTTP proxy pipeline and checking the invocation result status before attempting retries.
Key changes:
- Adds a new ScriptInvocationRequestTransformer to pass invocation context through HTTP requests
- Modifies RetryProxyHandler to check invocation result status before retrying
- Updates DefaultHttpProxyService to use the new transformer and include invocation context
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/WebJobs.Script/ScriptConstants.cs | Adds constant for HTTP proxy script invocation context |
| src/WebJobs.Script/Http/ScriptInvocationRequestTransformer.cs | New transformer that passes script invocation context to HTTP request options |
| src/WebJobs.Script/Http/RetryProxyHandler.cs | Enhanced to check invocation result status before retrying requests |
| src/WebJobs.Script/Http/DefaultHttpProxyService.cs | Updated to use new transformer and include invocation context in HTTP context |
| test/WebJobs.Script.Tests/HttpProxyService/DefaultHttpProxyServiceTests.cs | Moved to Http namespace |
| test/WebJobs.Script.Tests/HttpProxyService/ScriptInvocationRequestTransformerTests.cs | New comprehensive test file for the request transformer |
| if (resultSource is not null && resultSource.Task.IsFaulted) | ||
| { | ||
| throw resultSource.Task.Exception?.InnerException ?? new HttpRequestException("The function invocation tied to this HTTP request failed."); |
There was a problem hiding this comment.
The condition only checks for IsFaulted but doesn't handle IsCanceled state. If the invocation is canceled, retries should also stop. Consider checking resultSource.Task.IsCompleted && !resultSource.Task.IsCompletedSuccessfully to cover both faulted and canceled scenarios.
| if (resultSource is not null && resultSource.Task.IsFaulted) | |
| { | |
| throw resultSource.Task.Exception?.InnerException ?? new HttpRequestException("The function invocation tied to this HTTP request failed."); | |
| if (resultSource is not null && resultSource.Task.IsCompleted && !resultSource.Task.IsCompletedSuccessfully) | |
| { | |
| throw resultSource.Task.Exception?.InnerException ?? new HttpRequestException("The function invocation tied to this HTTP request failed or was canceled."); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Cancellation will be propagated to the HttpRequest and handled later in this file. I think we should leave the cancellation flow as is, and not add extra handling based on the script invocation result TCS.
Edit: Here seems to be the only location this tcs is set to canceled, and this is a path that would lead to cancellation flowing to the worker and http request.
| public override async ValueTask TransformRequestAsync(HttpContext httpContext, HttpRequestMessage proxyRequest, string destinationPrefix, CancellationToken cancellationToken) | ||
| { | ||
| // this preserves previous behavior (which called the default transformer) - base method is also called inside of here | ||
| await HttpTransformer.Default.TransformRequestAsync(httpContext, proxyRequest, destinationPrefix, cancellationToken); |
There was a problem hiding this comment.
I feel calling await base.TransformRequestAsync instead here provides more clarity. you can also remove the comment.
There was a problem hiding this comment.
It has to be HttpTransformer.Default.TransformRequestAsync - the base method does not add the X-Forwarded headers. I can remove the comment since tests should be able to catch if this is changed, but I wanted to make sure it was clear that choosing this method was intentional.
test/WebJobs.Script.Tests/HttpProxyService/ScriptInvocationRequestTransformerTests.cs
Show resolved
Hide resolved
| if (resultSource is not null && resultSource.Task.IsFaulted) | ||
| { | ||
| throw resultSource.Task.Exception.InnerException ?? | ||
| new Exception($"The function invocation tied to this HTTP request failed. Invocation ID: {scriptInvocationContext.ExecutionContext.InvocationId}"); |
There was a problem hiding this comment.
Still undecided on what exception to throw here - we don't want to throw something that will cause retries (see below lines), but what makes the most sense (some cancellation-related exception? do we change the check for HttpRequestException below so we don't retry in some cases?
Issue describing the changes in this PR
Breaking down #11159 and putting the HTTP changes in a separate PR. These changes make sure we stop retrying an http request if the correlated function invocation is failed by the host elsewhere (in cases of worker channel shutdown, host-tracked function timeout).
Pull request checklist
IMPORTANT: Currently, changes must be backported to the
in-procbranch to be included in Core Tools and non-Flex deployments.in-procbranch is not requiredrelease_notes.md