quic: Remove flaky ASSERT in ShouldCreateOutgoingBidirectionalStream#44500
Merged
phlax merged 2 commits intoenvoyproxy:mainfrom Apr 17, 2026
Merged
quic: Remove flaky ASSERT in ShouldCreateOutgoingBidirectionalStream#44500phlax merged 2 commits intoenvoyproxy:mainfrom
ShouldCreateOutgoingBidirectionalStream#44500phlax merged 2 commits intoenvoyproxy:mainfrom
Conversation
Member
Author
|
/retest |
Member
Author
|
@jwendell @ravenblackx this is probably next in terms of defo fixing a problem that has been around ages |
Fix: envoyproxy#41526 Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
force-pushed
the
flake-buffer-account-no
branch
from
16ef255 to
4cba998
Compare
ravenblackx
approved these changes
Apr 17, 2026
jwendell
approved these changes
Apr 17, 2026
Member
jwendell
left a comment
jwendell
left a comment
There was a problem hiding this comment.
The one-line fix is correct and well-motivated. The ASSERT was wrong from the day it was written — it contradicts the explicit design of the override. I'd suggest updating the PR title to reflect that this is a production code fix, and optionally adding a focused unit test. Otherwise, looks good to merge.
phlax
changed the title
ShouldCreateOutgoingBidirectionalStream
Member
Author
|
/retest flakey flakes |
Member
Author
|
/retest |
krinkinmu
pushed a commit
to grnmeira/envoy
that referenced
this pull request
Apr 20, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
reubent-goog
pushed a commit
to reubent-goog/envoy
that referenced
this pull request
Apr 21, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Signed-off-by: Reuben Tanner <reubent@google.com>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 24, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 24, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 24, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 27, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 27, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
that referenced
this pull request
Apr 27, 2026
#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes #41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
This was referenced Apr 27, 2026
Copilot AI
pushed a commit
that referenced
this pull request
Apr 27, 2026
#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes #41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: phlax <454682+phlax@users.noreply.github.com>
Copilot AI
pushed a commit
that referenced
this pull request
Apr 27, 2026
#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes #41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: phlax <454682+phlax@users.noreply.github.com>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 27, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: phlax <454682+phlax@users.noreply.github.com> Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 29, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: phlax <454682+phlax@users.noreply.github.com> Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 29, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: phlax <454682+phlax@users.noreply.github.com> Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
to phlax/envoy
that referenced
this pull request
Apr 29, 2026
envoyproxy#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes envoyproxy#41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
that referenced
this pull request
Apr 30, 2026
#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes #41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io>
phlax
added a commit
that referenced
this pull request
Apr 30, 2026
#44500) QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream. The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment. Fixes #41526 --------- Signed-off-by: Ryan Northey <ryan@synca.io> Co-authored-by: phlax <454682+phlax@users.noreply.github.com> Signed-off-by: Ryan Northey <ryan@synca.io>
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
4 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.
QuicSpdyClientSession::ShouldCreateOutgoingBidirectionalStream() can legitimately return false, but we return true unconditionally to avoid a nullptr deref in QuicHttpClientConnectionImpl::newStream.
The existing ASSERT on the parent's return value was therefore incorrect and triggered flakes (e.g. in buffer_accounting_integration_test on MSAN). Drop the ASSERT and update the comment.
Fixes #41526