Repository navigation
Conversation
deadcaf3
requested review from
agnosticdev,
kkuk24,
rpaulo and
tfpauly
as code owners
October 4, 2026 20:08
rpaulo
reviewed
Oct 5, 2026
| // Every frame that gets this far can queue RETIRE_CONNECTION_ID frames | ||
| // below, and those only leave the queue once they are sent. | ||
| let queuedRetireCount = withPendingItemsForKeyState { $0.retireConnectionIDs.count } | ||
| if queuedRetireCount > 2 * remoteCIDs.activeConnectionIDLimit { |
- Close with CONNECTION_ID_LIMIT_ERROR in `processNewConnectionIDFrame` once twice the active connection ID limit of RETIRE_CONNECTION_ID frames are queued, as RFC 9000 section 5.1.2 allows - Add a test covering the cap
deadcaf3
force-pushed
the
fix-cap-retire-connid-frames
branch
from
October 5, 2026 18:23
6f86e72 to
4e7cd90
Compare
rpaulo
approved these changes
Oct 6, 2026
rpaulo
self-requested a review
October 6, 2026 21:10
rpaulo
requested changes
Oct 6, 2026
rpaulo
left a comment
rpaulo
left a comment
Contributor
There was a problem hiding this comment.
Our internal review process flagged two issues:
- if the frame doesn't retire any CID but the queue is at 128, we will close the connection incorrectly
- if the queue is at 127, we can still overshoot when the frame being processed retires many CIDs.
- Close with CONNECTION_ID_LIMIT_ERROR only when the frame being processed would take the RETIRE_CONNECTION_ID queue past twice the active connection ID limit, so the queue cannot overshoot the limit - Accept a frame that retires nothing even with the queue at the limit - Add tests covering both cases
Author
|
Fixed both in baee899: the guard now counts what the frame would queue before queueing it, and closes only if queued + new > 2 x active_connection_id_limit. |
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.
Problem
processNewConnectionIDFramequeues a RETIRE_CONNECTION_ID frame for every connection ID it retires, and nothing bounds that queue. RFC 9000 §5.1.2 says an endpoint SHOULD limit it and MAY close with CONNECTION_ID_LIMIT_ERROR when the limit is exceeded.Fix
CONNECTION_ID_LIMIT_ERRORwhen a NEW_CONNECTION_ID frame arrives while at least twice the active connection ID limit of RETIRE_CONNECTION_ID frames are queued (128 with the current limit of 64). Twice the limit is the minimum the RFC asks an endpoint to allow.Testing
testRetireConnectionIDQueueIsCappedinConnectionIDRotationTests.