test: cover the per-stream frame-size bound in TcpFraming - #3524
Merged
pjfanning merged 1 commit intoSep 3, 2026
Conversation
Motivation: TcpFraming.ReadStreamId selects maximumLargeFrameSize only for ArteryTransport.LargeStreamId and maximumFrameSize otherwise (apache#3492), but no test exercised that selection: the existing bound tests all use a single maximumFrameSize with no maximumLargeFrameSize configured, so they cannot tell the per-stream selection apart from a bug that applied one bound to every stream. Modification: Add a perStreamBoundedFramingFlow fixture with distinct maximumFrameSize and maximumLargeFrameSize, and two tests: a frame over the ordinary maximum but within the large maximum is accepted on ArteryTransport.LargeStreamId and rejected on ArteryTransport.OrdinaryStreamId. Both frames carry their full declared payload, so a false accept cannot hide behind truncation. Result: The per-stream bound selection in ReadStreamId is covered directly. Tests: - sbt "remote/testOnly org.apache.pekko.remote.artery.tcp.TcpFramingSpec" - 16 passed - Checked the new tests discriminate: with ReadStreamId's selection replaced by an unconditional maximumLargeFrameSize, the ordinary-stream test fails ("Future.failed not completed with a throwable") while the large-stream test still passes; reverted after confirming - sbt "remote/scalafmtCheckAll" - clean References: None - test-coverage gap noticed while comparing TcpFraming.scala (hardened in apache#3492) against an unrelated Akka fix for the same class of issue
pjfanning
added a commit
to pjfanning/incubator-pekko
that referenced
this pull request
Sep 3, 2026
Motivation: Same test-coverage gap as on main: the bound tests from apache#3492 configure only maximumFrameSize, so they cannot tell the per-stream bound selection in ReadStreamId apart from a bug that applied one bound to every stream. Modification: Add the two tests from apache#3524: with distinct maximumFrameSize and maximumLargeFrameSize configured, a frame over the ordinary maximum but within the large maximum is accepted on ArteryTransport.LargeStreamId and rejected on ArteryTransport.OrdinaryStreamId. Both frames carry their full declared payload, so a false accept cannot hide behind truncation. Result: The per-stream bound selection is covered directly on 1.7.x as well. Tests: - sbt "remote/testOnly org.apache.pekko.remote.artery.tcp.TcpFramingSpec" - 17 passed - sbt "++ 2.12.21 remote/Test/compile" - clean - sbt "remote/scalafmtCheckAll" - clean References: Refs apache#3524
Philippus
approved these changes
Sep 3, 2026
pjfanning
added a commit
that referenced
this pull request
Sep 3, 2026
* fix: bound inbound Artery TCP frame length at framing (#3492) Motivation: TcpFraming read the 4-byte frame length from the wire and passed it straight to reader.take / ByteBuffer allocation with no bound. A peer - unauthenticated on the default tcp transport - could declare an oversized or negative frame and drive a large allocation. Because the decoder and deserializer stages downstream of the MergeHub are shared by every inbound connection, an OutOfMemoryError there is fatal to the shared stream and, after inbound-max-restarts, terminates the whole ActorSystem - turning one malformed connection into node loss. Modification: Pass the configured maximum-frame-size and maximum-large-frame-size into TcpFraming and reject a frame length that is negative or exceeds the maximum for the connection's stream, before any data is buffered. The large-message stream keeps its larger bound. Rejection is a FramingException, which tears down only that connection. Result: A malformed or oversized frame is rejected per-connection instead of allocating without limit and risking a fatal error on the shared inbound stream. Tests: - sbt "remote/testOnly org.apache.pekko.remote.artery.tcp.TcpFramingSpec" - 14 passed, incl. oversized, negative and at-limit frame cases - sbt "remote/testOnly org.apache.pekko.remote.artery.LargeMessagesStreamSpec" - 4 passed (legitimate large frames still delivered) - sbt "remote/mimaReportBinaryIssues" - no issues (TcpFraming is @internalapi; new params are defaulted) References: None - found while reviewing the draft threat model in #3478 * test: cover the per-stream frame-size bound in TcpFraming Motivation: Same test-coverage gap as on main: the bound tests from #3492 configure only maximumFrameSize, so they cannot tell the per-stream bound selection in ReadStreamId apart from a bug that applied one bound to every stream. Modification: Add the two tests from #3524: with distinct maximumFrameSize and maximumLargeFrameSize configured, a frame over the ordinary maximum but within the large maximum is accepted on ArteryTransport.LargeStreamId and rejected on ArteryTransport.OrdinaryStreamId. Both frames carry their full declared payload, so a false accept cannot hide behind truncation. Result: The per-stream bound selection is covered directly on 1.7.x as well. Tests: - sbt "remote/testOnly org.apache.pekko.remote.artery.tcp.TcpFramingSpec" - 17 passed - sbt "++ 2.12.21 remote/Test/compile" - clean - sbt "remote/scalafmtCheckAll" - clean References: Refs #3524
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.
Motivation
TcpFraming.ReadStreamIdselectsmaximumLargeFrameSizeonly forArteryTransport.LargeStreamIdandmaximumFrameSizeotherwise (#3492), but no testexercised that selection. The existing bound tests (
"reject a frame that exceeds the maximum frame size","accept a frame at exactly the maximum frame size") constructTcpFramingwith onlymaximumFrameSizeset —maximumLargeFrameSizedefaults toInt.MaxValue— so they cannot distinguish the correct per-stream selection from a bugthat applied one bound to every stream regardless of
streamId.Modification
Add a
perStreamBoundedFramingFlowfixture with distinctmaximumFrameSizeandmaximumLargeFrameSize, and two tests:ArteryTransport.LargeStreamIdArteryTransport.OrdinaryStreamIdBoth frames carry their full declared payload rather than just the header, so an
incorrectly-accepted frame can't be mistaken for a truncation failure — I hit exactly
that mistake while writing the reject test (an under-length frame fails from truncation
regardless of the bound logic) and caught it by deliberately breaking the production
selection and confirming the test still passed for the wrong reason, then fixed the test
to include the payload.
Result
The per-stream bound selection in
ReadStreamIdis covered directly, rather than onlyby the code being read during review.
Tests
sbt "remote/testOnly org.apache.pekko.remote.artery.tcp.TcpFramingSpec"— 16 passedReadStreamId's selection temporarilyreplaced by an unconditional
maximumLargeFrameSize, the ordinary-stream test failed(
Future.failed not completed with a throwable) while the large-stream test stillpassed; reverted after confirming
sbt "remote/scalafmtCheckAll"— cleanReferences
None - test-coverage gap noticed while comparing
TcpFraming.scala(hardened in #3492)against an unrelated fix for the same class of issue in a downstream project.