Uh oh!
There was an error while loading. Please reload this page.
fix(sources): [OBE-11555] release the request-limiter permit before the ack write - #143
Open
JuanMantica45 wants to merge 2 commits into
Open
fix(sources): [OBE-11555] release the request-limiter permit before the ack write#143JuanMantica45 wants to merge 2 commits into
JuanMantica45 wants to merge 2 commits into
Conversation
handle_stream held its RequestLimiterPermit across stream.write_all of the acknowledgement. The permit exists to bound in-flight decoded events, and that purpose is already served once send_batch and receiver.await complete, so a peer that stopped draining its receive window could park a permit for as long as it liked and starve every other connection on the source. The permit is now released first. The ack write additionally gets a 30-second timeout so the connection itself is reclaimed: write_all only needs to hand a few bytes to the kernel send buffer, so it cannot block unless the peer has already stopped reading long enough to fill it -- retransmission and congestion are absorbed by the kernel and cannot trigger the timeout. Replaces a placeholder test that asserted nothing. The ack write is extracted into `write_ack`, which is testable over tokio::io::duplex, with coverage for the success, timeout, slow-but-progressing, and hangup paths plus the permit ordering the fix depends on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 freeto 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.
What
handle_streamheld itsRequestLimiterPermitacrossstream.write_allof the acknowledgement. A peer that stopped draining its receive window could park a permit indefinitely and starve every other connection on the source.The permit is now released first. The ack write additionally gets a 30-second timeout so the connection itself is reclaimed.
Why 30s is safe
write_allonly needs to hand a few bytes to the kernel send buffer — it does not wait for the peer's TCP ACK. With a defaultSO_SNDBUF, a 6-byte ack cannot block unless the peer has already stopped reading long enough to fill the buffer. Retransmission, RTT and congestion are absorbed by the kernel and cannot trigger the timeout, so it fires only against a genuinely stuck peer. It also sits below the sinks' 60s request timeout.Testing
Replaces a placeholder test that asserted nothing (empty body, "verified by code inspection"). The ack write is extracted into
write_ack, testable overtokio::io::duplex, with coverage for the success, timeout, slow-but-progressing and hangup paths plus the permit ordering the fix depends on. 9 passing.Jira: OBE-11555