Fix Message failed: ' Timeout' SWARN in SHTTPSManager::postPoll#2562
Open
Beamanator wants to merge 5 commits intomainfrom
Open
Fix Message failed: ' Timeout' SWARN in SHTTPSManager::postPoll#2562Beamanator wants to merge 5 commits intomainfrom
Beamanator wants to merge 5 commits intomainfrom
Conversation
Remove the broken 200/204/202 check that prevented _onRecv() from being called for valid non-2xx responses. Any complete HTTP response should be passed to the subclass for handling — the existing comment explicitly noted this logic was wrong. Non-standard methodLines (e.g., a load balancer returning " Timeout" instead of a proper HTTP status line) now log at SHMMM severity instead of firing a SWARN and coercing the response to 500. Fixes: Expensify/Expensify#610175 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Preserved the fix: remove the restrictive 200/204/202 check and call _onRecv() for all complete responses. Non-standard methodLines log at SHMMM instead of SWARN. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
Contributor
Author
|
@MelvinBot review this PR |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Verify that the caller-specified fallback code is respected when the methodLine cannot be parsed as a valid HTTP status. Made-with: Cursor
Contributor
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: c4f5923972
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The comment stated 'if we fail to find such a code, or can't parse it as an integer, we default to 400' but the code left transaction.response at 0. This made wait loops like 'while (!transaction->response)' stall on non-standard method lines (e.g. ' Timeout'). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e empty If SParseHTTP returns nonzero bytes but leaves methodLine empty (e.g. a bare CRLF response), _onRecv was skipped, leaving transaction.response==0 and stalling any 'while (!transaction->response)' wait loop. Also clarify in comment that non-standard responses (no Content-Length, unparseable status) only reach completeRequest=true once the socket closes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
@codex review |
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
1 participant
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.
Explanation of Change
SHTTPSManager::postPoll()was only calling_onRecv()for responses containing" 200","204", or"202"in the methodLine, or responses with body content. Any other complete response (including non-standard methodLines like" Timeout"from load balancers) fell into an else branch that firedSWARN("Message failed: '...")and coerced the response to 500.The existing code comment even called this out explicitly: "Coercing anything that's not 200 to 500 makes no sense, and should be abandoned with the above."
Before: Non-standard HTTP responses (e.g., load balancer returning
" Timeout"instead ofHTTP/1.1 504 Gateway Timeout) triggeredSWARN("Message failed: '...'")and forced response=500, bypassing all_onRecv()subclass handling.After: All complete responses are passed to
_onRecv()for proper subclass handling. Non-standard methodLines (not starting withHTTP/) log atSHMMMseverity instead ofSWARN. Subclasses (Stripe, Hubspot, etc.) parse response codes as designed.Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/610175
Tests
Added
testGetHTTPResponseCodetoLibStuffTestverifying:" Timeout","Timeout","") return 400 as a safe defaultdefaultStatusCodeparameter is used when parsing failsInternal Testing Reminder: when changing bedrock, please compile auth against your new changes