Fix rare HAProxy crash resulting from a watchdog failure - #42
Merged
Conversation
11 tasks
As documented this provides some fallback protection against odd execution orderings that could otherwise cause a crash. This also brings us slightly back in line with flt_bwlim (of which the weir filter is modeled) which also does this.
I now better understand why this is required so it makes sense to reflect this here. Adding the assert also serves to communicate how its meant to function, lines up with flt_bwlim and covers us against future weird behaviour.
Here we're just swapping the order in which we check the various conditions before attempting to throttle a stream. The check for a missing remote_addr is only required so that we can safely call the rl_* functions so it got moved down there accordingly. The check for non-zero length was moved further down and in particular was separated from the check against next_allowed_send_tick expiring. This second one is most sigificant: We do sometimes get called with len=0 and by enabling a reset of the next send tick (and by extension the update to analyse_exp) we guard against situations where we're done processing the payload of this stream, but are still being called with zero length. In this situation we definitely do not want to delay processing any further and we need to make sure that analyse_exp is set accordingly. Without this update if we got throttled and after that only got called with length 0 for any reason, we would never reset analyse_exp to TICK_ETERNITY and would break our contract with HAProxy in doing so, causing weird behaviour like stuck streams and potentially crashes.
Unlike flt_bwlim our filter is enabled once and applies to both the request and the response. This means that the state on our filter is shared between the two. If we have a single counter to track the next send tick across both the request and the response, then if the two are running concurrently, they can incorrectly slow each other down. For example if you're streaming up request data while streaming back the same amount of response data and your upload limit is lower than your download limit, then the response data could be slowed down just because your request data is being streamed up faster than your upload limit.
This has a significantly clearer justification for being the correct callback for the HTTP case than end_analyse.
jacquesh
force-pushed
the
fix-weir-watchdog-failure
branch
from
June 9, 2026 09:28
710d42d to
3a988cb
Compare
BBoozmen
approved these changes
Jun 9, 2026
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
2 participants
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.
Description
For a while we've been seeing strange crashes in HAProxy that we've been unable to pin down. Recent investigation by @aokhovat and @BBoozmen highlighted that the problem lies in the way we update the channel's
analyse_exp(or don't, as the case may be). The fix is threefold (organised into separate commits with descriptive messages, this is just s summary):next_allowed_send_tickbetween the request and the response, to avoid weird sharing behaviour in the case where the response starts streaming before the request transfer finishes.analyse_expduring payload filtering even if the buffer we get given for forwarding is 0 bytes long (which is also something that flt_bwlim is already doing but we were not).I've also updated/added comments to reflect our new understanding.
Type of Change
Checklist