Skip to content

Preserve failure nf status set by recordFinalError in buildZuulHttpResponse #2131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lindseyreynolds wants to merge 1 commit into
base: master
Choose a base branch
from
+1 −1
Open

Preserve failure nf status set by recordFinalError in buildZuulHttpResponse #2131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion zuul-core/src/main/java/com/netflix/zuul/filters/endpoint/ProxyEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -910,7 +910,7 @@ private HttpResponseMessage buildZuulHttpResponse(
// Collect some info about the received response.
origin.recordFinalResponse(zuulResponse);
origin.recordFinalError(zuulRequest, ex);
StatusCategoryUtils.setStatusCategory(zuulCtx, statusCategory);
StatusCategoryUtils.storeStatusCategoryIfNotAlreadyFailure(zuulCtx, statusCategory);
Copy link
Copy Markdown
Collaborator

@gavinbunney gavinbunney May 18, 2026

Choose a reason for hiding this comment

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

I wonder if this handles retries correctly? Since I think this block is called twice for a retry scenario, first time through sets it to error X, second time through the error might be Y now?

Copy link
Copy Markdown
Contributor Author

@lindseyreynolds lindseyreynolds May 19, 2026

Choose a reason for hiding this comment

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

Yep it does. I was able to test this by running wall-e, but not running the sidecar. Then I sent a curl and put a break point on this line and in postErrorProcessing to confirm the failure. Then I started the sidecar while it was paused and continued the request. When it got to this line, statusCategory had been updated to SUCCESS, since that was the actual status of the current request. It did not hold onto the previous retry value.

Copy link
Copy Markdown
Contributor Author

@lindseyreynolds lindseyreynolds May 19, 2026

Choose a reason for hiding this comment

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

I also have an integration test that shows this in my timeout branch, but cant push it until this oss change merges because I am using the oss directory with this change in place.

zuulCtx.setError(ex);
zuulCtx.put("origin_http_status", Integer.toString(respStatus));

Expand Down
Loading