drpcstream: fail-stop on receive-cap overrun instead of blocking the reader - #94
Open
suj-krishnan wants to merge 1 commit into
Open
drpcstream: fail-stop on receive-cap overrun instead of blocking the reader#94suj-krishnan wants to merge 1 commit into
suj-krishnan wants to merge 1 commit into
Conversation
…reader The byte-accounted receive queue blocked the producer (manageReader) when a message would exceed the per-stream cap. A compliant sender never reaches the cap -- flow control bounds it -- but a peer that ignores flow control (a bug or, in mixed-version clusters, a legacy peer) can, and blocking manageReader stalls frame delivery for every stream on the connection. A single flow-control bug should not be able to wedge a whole connection. Enforce the cap with a stream-local fail-stop. Under a byte budget Enqueue no longer blocks: a message that would exceed the cap returns false, and handlePacket terminates just that stream with a new ReceiveCapError (mapped to codes.ResourceExhausted) and sends the peer an abortive KindError. The shared reader is never blocked and other streams are unaffected. Flow control off keeps the legacy slot-blocking behavior unchanged. The abortive-terminate-and-notify sequence is extracted from handleAssembleError into sendAbortiveError and shared by both per-stream policy violations (oversized message, cap overrun). This does not bound zero-credit (empty) message floods: an empty message consumes no window and never exceeds a byte cap, so it is neither blocked nor failed. That is a separate follow-up (a per-message minimum charged symmetrically in send credit, grants, and the queue). Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.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 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.
Stacked on #93 (base:
sujatha/flow-control-recv-bytes). Implements CRDB-65749Problem
After #93 the receive queue is byte-accounted, but it blocks the producer (
manageReader) when a message would exceed the per-stream cap. This fix adds checks so that we neve wedge a whole connection due to a single stream exceeding its message cap.Change
Enforce the cap with a stream-local fail-stop:
ringBuffer.Enqueueno longer blocks — it returnsfalsewhen a message would exceed the cap.handlePacketthen terminates only that stream with a newdrpc.ReceiveCapError(mapped tocodes.ResourceExhausted) and sends the peer an abortiveKindError.HandleFramereturnsnil, so the shared reader keeps running and other streams are unaffected.Scope / rationale
drpc peers are trusted CockroachDB nodes, so this is not adversarial defense — it's bug-containment and operability: a distinct logged error and one failed (retryable) RPC instead of a silently wedged connection.