Repository navigation
Conversation
Motivation: Cancelling the transport (ciphertext) output of a TLS operator tore the session down after at most one inbound round: doOutbound moved straight to the completed phase and pumpFinished() then cancelled the transport input. Inbound records the operator had already received from the peer but not yet decrypted were discarded while the plaintext output saw a clean onComplete, so an application could not tell that received data had been abandoned. Every other teardown path is observable: transport input truncated without close_notify delivers received data and emits SessionTruncated, transport input failure fails the stage, and plaintext output cancellation drains accepted plaintext. Modification: - TlsGraphStage: emit SessionTruncated from the cancelled-transport-output branch before moving to CompletedPhase, so pumpFinished() still cancels the inputs and only completeStage() is deferred until the report is pushed. Skipped when the session was never established, the inbound side is already done, or the plaintext output is closed or holds a pending element. - TLSActor: same contract through a signallingInboundTruncation phase gated on plaintext output demand or closure. The read side is cancelled before the pump can park there, otherwise a subscribed transport input would overrun its buffer while waiting for demand. Reuses OutputBunch.demandAvailableFor rather than restating it. - Document the transport output cancellation contract in the stream IO guide. Result: Both engines still deliver the inbound data they can decrypt in the round that observes the cancellation, and now report SessionTruncated before completing instead of discarding the rest silently. Teardown timing is unchanged when the plaintext output is already closed, so the existing cancellation tests are unaffected. Tests: - stream-tests/Test/testOnly org.apache.pekko.stream.io.TlsTransportOutputCancelSpec: 18/18 pass; 14 of them fail without the production change - stream-tests/Test/testOnly *Tls*: 308 tests, 0 failures - stream/mimaReportBinaryIssues: no issues - docs/paradox: success - headerCheckAll: success - scalafmt 3.11.5 --test clean on changed files; git diff --check clean - Full suite left to CI References: Refs apache#3739
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
What changed in this PR
This PR makes transport-output cancellation observable to TLS consumers by reporting inbound truncation (SessionTruncated) when buffered inbound records become unreachable due to teardown triggered by cancelling the ciphertext output.
Changes:
- Emit
SessionTruncatedon the transport-output-cancel teardown path in bothTlsGraphStageand the legacyTLSActorimplementation. - Add a dedicated TLSActor phase to defer the truncation report until plaintext output demand (and proactively cancel read-side input to avoid buffering overrun).
- Add a new spec covering multiple buffering/demand/cancellation permutations and document the contract in the stream IO guide.
| File | Description |
|---|---|
| stream/src/main/scala/org/apache/pekko/stream/impl/io/TlsGraphStage.scala | Signals inbound truncation on TransportOut cancellation before completing. |
| stream/src/main/scala/org/apache/pekko/stream/impl/io/TLSActor.scala | Introduces a truncation-signalling phase for the legacy actor and cancels inputs to prevent buffer overrun while waiting for demand. |
| stream-tests/src/test/scala/org/apache/pekko/stream/io/TlsTransportOutputCancelSpec.scala | Adds comprehensive regression tests for truncation semantics under transport-output cancellation. |
| docs/src/main/paradox/stream/stream-io.md | Documents the new observable contract when transport output is cancelled. |
Review effort: Lite
Comment on lines
+292
to
+296
| val signallingInboundTruncation = TransferPhase(userOutDemand) { () => | ||
| if (tracing) log.debug("signalling SessionTruncated after TransportOut cancellation") | ||
| outputBunch.enqueue(UserOut, SessionTruncated) | ||
| nextPhase(completedPhase) | ||
| } |
Comment on lines
+370
to
+377
| private def inboundTruncationOwed(): Boolean = | ||
| if (!corkUser && !engine.isInboundDone && !userOutDemand.isCompleted) { | ||
| inputBunch.cancel() | ||
| try { | ||
| engine.closeInbound() | ||
| false | ||
| } catch { case _: SSLException => true } | ||
| } else false |
Comment on lines
+364
to
+377
| /** | ||
| * Closing the inbound side is what reveals whether the peer's close_notify is still missing, so | ||
| * the plaintext output can learn that inbound records buffered from here on are unreachable. The | ||
| * read side is cancelled first because delivering that report may have to wait for plaintext | ||
| * demand, and a subscribed transport input would overrun its buffer while the pump is parked. | ||
| */ | ||
| private def inboundTruncationOwed(): Boolean = | ||
| if (!corkUser && !engine.isInboundDone && !userOutDemand.isCompleted) { | ||
| inputBunch.cancel() | ||
| try { | ||
| engine.closeInbound() | ||
| false | ||
| } catch { case _: SSLException => true } | ||
| } else false |
He-Pin
marked this pull request as draft
October 30, 2026 19:26
Member
|
@He-Pin is this aimed at 2.0.0? Nothing else blocks a 2.0.0 release at the moment. If we choose to hold this to 2.1.0, then I can create the RC1 for 2.0.0. |
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
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
Cancelling the transport (ciphertext) output of a TLS operator tears the session down after at most one inbound round:
doOutboundmoves straight to the completed phase, andpumpFinished()then cancels the transport input. Inbound records the operator had already received from the peer but had not decrypted yet were discarded while the plaintext output saw a cleanonComplete, so an application could not tell that received data had been abandoned.Every other teardown path already has an observable contract:
close_notify: received data is delivered andSessionTruncatedis emitted;failTls;Reproduced on both engines and both protocols with an established session: two inbound records buffered while the plaintext output has no demand, then the transport output cancelled. Only the first record was delivered; the second was dropped with no signal at all. The control, with the transport output left open, delivers both.
This came out of auditing #3739. The scenario that issue describes does not reproduce: a single buffered record is delivered once plaintext demand arrives, and the stage does not complete early. The same teardown path does lose data once more than one record is buffered.
Modification
TlsGraphStage: emitSessionTruncatedfrom the cancelled-transport-output branch before moving toCompletedPhase, sopumpFinished()still cancels the inputs and onlycompleteStage()is deferred until the report has been pushed. Skipped when the session was never established, the inbound side is already done, or the plaintext output is closed or already holds a pending element.TLSActor: the same contract through asignallingInboundTruncationphase gated on plaintext output demand or closure. The read side is cancelled before the pump can park there, otherwise a still-subscribed transport input would keep buffering untilInput buffer overrunkilled the actor. ReusesOutputBunch.demandAvailableForinstead of restating it.Result
Both engines still deliver the inbound data they can decrypt in the round that observes the cancellation, and now report
SessionTruncatedbefore completing instead of discarding the rest silently. Teardown timing is unchanged when the plaintext output is already closed, so the three existingcipherOut.cancel()cases inTlsGraphStageIsolatedSpec, which all cancelplainOutfirst, are unaffected.Tests
stream-tests/Test/testOnly org.apache.pekko.stream.io.TlsTransportOutputCancelSpec— 18/18 pass. Without the production change 14 of them fail and only the 4 control cases pass, so the spec is directional. It covers two buffered records plus cancellation, one buffered record plus cancellation, three buffered records drained one demand unit at a time, the no-cancellation control, and a legacy-actor case pinning that the read side is cancelled while the report waits for plaintext demand.stream-tests/Test/testOnly *Tls*— 308 tests, 0 failures.stream/mimaReportBinaryIssues— no issues.docs/paradox— success.headerCheckAll— success.scalafmt3.11.5--testclean on the changed files;git diff --checkclean.References
Refs #3739