Repository navigation
Preserve accepted TLS plaintext on output cancellation - #3727
Conversation
pjfanning
left a comment
pjfanning
left a comment
There was a problem hiding this comment.
This changes the test to avoid what looks like a GraphStage TLS engine bug, rather than fixing the engine.
Pending plaintext is dropped when the user output is cancelled
- A small
SendBytesis not encrypted straight away.UserInputLogic.onPushqueues it and schedulesUserInputBatchTimer. - If the plaintext output is cancelled before that timer fires,
doInboundcallsengine.closeOutbound()and moves toFlushingOutboundPhase. The queued bytes are never encrypted, so the client receiveshell. - The legacy engine (
TLSActor.doInbound) has the same close-on-cancel logic. It passes because it encrypts each element as soon as it arrives, so nothing is left waiting when the cancel comes in.
This is not documented IgnoreComplete behaviour
The TLSClosing scaladoc only says which termination signal is ignored. It says nothing about discarding plaintext the handler has already emitted. A handler that writes its last bytes and stops reading at about the same moment is a normal server pattern: takeWhile, take, request/response. On the default bindAndHandleWithTls overload, such servers would lose trailing response bytes once graph-stage is enabled or becomes the default.
Suggestion
Fix this in TlsGraphStage. Before calling closeOutbound on user-output cancellation, encrypt and flush any queued user input, for example by cancelling the batch timer and draining userInput first. Keep this test on the 4-argument overload so it guards the fix.
ccdef5a to
ac631bb
Compare
He-Pin
changed the title
|
Agreed: this was a batching regression. 31f6681 fixes the engine and restores the original four-argument test. Accepted current-session data is drained without starting queued renegotiation or waiting for a new handshake. Both public DSL docs now describe this behavior and its limits. The 257 TLS tests and 10 focused cases on JDK 17/21/25 pass; all four inline threads are resolved. |
ac631bb to
31f6681
Compare
He-Pin
left a comment
He-Pin
left a comment
There was a problem hiding this comment.
Deep review, comparing against TLSActor.
The premise checks out. TLSActor wraps synchronously inside the same pump turn (TLSActor.scala:336-345), whereas TlsGraphStage defers small SendBytes to a zero-delay batch timer (TlsGraphStage.scala:719-726 and 739-750). In TcpSpec's echo case the server's takeWhile(_ != '\n') (TcpSpec.scala:923) cancels the TLS plainOut while 'o' is still sitting in the pending queue, so the base implementation drops it and the client sees hell. Draining already-accepted data before closeOutbound() is the right shape of fix, and reusing InboundClosedPhase instead of adding a new phase is a good call. Truncating at the first non-SendBytes in finishAcceptedInput() correctly implements "do not start a queued renegotiation", and the tests around it are well constructed - the handshakesStarted shouldBe 1 assertion in particular pins the intent down precisely.
Findings: one blocker-level functional gap and three suggestions inline, plus three nits.
Two notes that are not tied to a single line:
-
The PR description says "Restore the original four-argument
bindAndHandleWithTlstest", but the diff touches five files and none of them isTcpSpec.scala- the four-argument test atTcpSpec.scala:914-946is unchanged onmain. Please drop or correct that bullet. -
Conflict with #3728. The two branches are independent (neither contains the other), both are based on
a55a008, and both edit the same lines of the same three files:docs/src/main/paradox/stream/stream-io.md- both insert a different paragraph at the same position.TlsGraphStage.scala- this PR rewrites thedoInboundcancel branch,inboundClosedCompletedandphaseExecutable; #3728 rewritesallocateBuffers/doWrap/doUnwrapand the buffer helpers.TlsGraphStageIsolatedSpec.scala- both changeScriptedSSLEngine's constructor andnewHarness's signature incompatibly (this PR addsrequestHandshakeAfterApplication/closing/extraAttributes; #3728 addsrequiredOutputSpace/negotiatedPacketSize/negotiatedApplicationSize), and both append cases to the same"TlsGraphStage deferred close handling" must {block.
Worth agreeing an order and rebasing the second one.
Verification I ran locally with sbt "stream-tests/Test/testOnly org.apache.pekko.stream.io.TlsGraphStageDeferredCloseSpec":
- this PR's head, unmodified: all 10 tests pass (7 new + 3 pre-existing);
- this PR's head plus the 2 probe tests quoted inline:
succeeded 10, failed 2; mainplus theIgnoreCancelprobe:succeeded 3, failed 1, confirming that one is pre-existing.
| if (!isOutboundClosed && closing.ignoreCancel) { | ||
| nextPhase(InboundClosedPhase) | ||
| true | ||
| } else if (!isOutboundClosed && canDrainAcceptedUserInput && |
There was a problem hiding this comment.
The drain can be preempted by the transportInput.isDepleted && transportInput.isEmpty branch at the top of doInbound (line 314), which is evaluated before this branch and before the inboundHalfClosedMode && cancelledHandshakeReady branch below it.
Once we are in InboundClosedPhase, if the peer's transport input completes while the accepted batch is still waiting for cipherOut demand, that first branch fires: engine.closeInbound() -> completeOrFlush() -> FlushingOutboundPhase/CompletedPhase, and the retained SendBytes are then dropped by pumpFinished() -> userInput.cancel() -> clearPending().
Reproduced against this PR's head with a probe added to TlsGraphStageDeferredCloseSpec:
val harness = newHarness()
finishInitialHandshake(harness)
harness.plainIn.sendNext(SendBytes(ByteString("a")))
harness.plainIn.expectRequest()
harness.plainOut.cancel()
harness.plainIn.expectCancellation()
harness.cipherIn.sendComplete() // peer half-closes during the drain
harness.cipherOut.request(1)
harness.cipherOut.expectNext(ApplicationCiphertext)[info] - must PROBE keep draining accepted plaintext when the transport input completes *** FAILED *** (82 milliseconds)
[info] java.lang.AssertionError: assertion failed: expected OnNext(ByteString(90)), found OnComplete
This is not a regression (main drops the data too, and so does TLSActor), but it defeats the stated goal of the PR in a realistic ordering: a client that sends a request and then half-closes its write side, or a server whose cipherIn FIN is processed just before the handler cancels plainOut. TcpSpec's echo case only passes because Source.maybe keeps the client's plaintext input open forever.
Suggested direction: while canDrainAcceptedUserInput and user data is still queued, do the inbound close after the outbound drain - e.g. record "transport depleted" and stay in InboundClosedPhase so doOutbound can wrap the retained batch first, instead of jumping straight to FlushingOutboundPhase/CompletedPhase. Please add a regression test for this ordering either way.
|
|
||
| private def inboundClosedCompleted: Boolean = | ||
| outboundCompleted && inboundHalfClosedCompleted | ||
| private def inboundClosedCompleted: Boolean = (!closing.ignoreCancel && transportOutput.isCancelled) || |
There was a problem hiding this comment.
This new escape hatch is guarded by !closing.ignoreCancel, so it only fixes non-termination for EagerClose/IgnoreComplete. The identical hang still exists for IgnoreCancel and IgnoreBoth.
With closing = IgnoreCancel, cancelling plainOut takes the first branch of doInbound -> InboundClosedPhase. If cipherOut is then cancelled while cipherIn stays open and silent: outboundReady is false (transportOutput.isDemandAvailable is false once cancelled), inboundHalfClosedReady is false (no transport data), and cancelledHandshakeReady is false because closing.ignoreCancel is true - so the phase is not executable. And inboundClosedCompleted stays false because inboundHalfClosedCompleted needs transportInput.isCompleted || engine.isInboundDone. pumpFinished() is never reached and the stage lives forever.
Reproduced on both main and this PR's head, so it is pre-existing rather than introduced here:
val harness = newHarness(closing = IgnoreCancel)
finishInitialHandshake(harness)
harness.plainOut.cancel()
harness.cipherOut.cancel()
harness.cipherIn.expectCancellation() // never arrivesmain : [info] Tests: succeeded 3, failed 1
pr3727 : [info] Tests: succeeded 10, failed 2
[info] - must PROBE terminate when the transport output is cancelled under IgnoreCancel *** FAILED *** (3 seconds, 190 milliseconds)
[info] java.lang.AssertionError: assertion failed: timeout (3 seconds) during fishForSpecificMessage, hint: Expecting cancellation
Since this line is already being touched, consider dropping the guard and widening to isClosed (which also covers the errored/completed flags):
private def inboundClosedCompleted: Boolean = transportOutput.isClosed ||
(outboundCompleted && inboundHalfClosedCompleted)If cipherOut is gone there is nowhere left to write, so terminating is correct regardless of the closing policy. Happy for this to be a separate PR if you would rather keep this one scoped.
| cancelPendingBatchTimer() | ||
| if (!logic.isClosed(inlet)) logic.cancel(inlet) | ||
| // AfterDelay cancellation leaves the inlet open temporarily; completion must not abort the drain. | ||
| setHandler(inlet, ignoreTerminateInput) |
There was a problem hiding this comment.
ignoreTerminateInput ignores onPush, onUpstreamFinish and onUpstreamFailure.
Ignoring completion is the point of the change, but ignoring failure is a behaviour change: today UserInputSlot.onUpstreamFailure routes to failTls(ex), which fails cipherOut and aborts the connection. With CancellationStrategy.AfterDelay the inlet is genuinely still open here - GraphStageLogic.cancel installs EagerTerminateInput precisely so that termination signals are not ignored (see the do not ignore termination signals comment at GraphStage.scala:777-781), and this line replaces it. The net effect is that an upstream failure arriving during the drain is silently converted into a clean close_notify.
Suggest a small private handler that ignores onPush/onUpstreamFinish but still calls failTls on onUpstreamFailure.
| * secure connection without regard to whether bytes are remaining to be sent or | ||
| * received, respectively. Especially for a client the common idiom of attaching | ||
| * secure connection, subject to the closing policy described below. Pending bytes | ||
| * may be discarded. When cancellation closes the connection, the `legacy-actor` |
There was a problem hiding this comment.
Two problems with this wording (the same text is added to javadsl/TLS.scala):
Pending bytes may be discarded.is immediately contradicted by the following sentences, which say thegraph-stageengine does send already-accepted data. As written, a reader cannot tell what actually happens.- The drain bound is not stated.
finishAcceptedInput()stops new input but drains everything already accepted, which is up toUserInputQueueSize = 64queued messages (TlsGraphStage.scala:1081) plus whatever is inuserInBuffer, each of arbitrary size. The paragraph further down - "In order to terminate the connection the client will then need to cancel the plaintext output as soon as all expected bytes have been received" - now implies termination can be delayed by that drain, which is worth saying explicitly.
Also, graph-stage is not the default (reference.conf: pekko.stream.materializer.tls.engine = "legacy-actor"), and nothing here tells the reader which engine they are on or where to look. Consider naming the config key, the way the companion paragraph added in #3728 does.
Suggested shape: state the guarantee once ("bytes that have not been accepted by the operator may be discarded"), then the engine-specific drain behaviour and its bound, then point at pekko.stream.materializer.tls.engine.
| } | ||
| } | ||
|
|
||
| private def cancelledHandshakeReady: Boolean = |
There was a problem hiding this comment.
Nit: cancelledHandshakeReady reads as "a cancelled handshake is ready", but it actually means "cancellation must close outbound now because draining is not possible". Something like mustCloseOutboundOnCancel would make both call sites (phaseExecutable and doInbound) self-explanatory.
| cipherIn ~> tls.in2 | ||
| tls.out2 ~> plainOut | ||
| // Keep the subscriber boundary's cancellation independent of the TLS stage's attributes. | ||
| tls.out2 ~> Flow[SslTlsInbound].map(identity) ~> plainOut |
There was a problem hiding this comment.
This map(identity) is only needed for the AfterDelay case, so that the TestSink's own cancellation is not subject to the TLS stage's attributes - but it is now inserted into every harness, which changes the topology of the three pre-existing tests as well. Consider making it conditional on extraAttributes, or extending the comment to say that it is deliberate for all cases.
|
|
||
| The `SSLEngine` instance can then be used with the binding or outgoing connection factory methods. | ||
|
|
||
| When the closing policy makes plaintext-output cancellation close the connection, the GraphStage TLS |
There was a problem hiding this comment.
Two notes:
- This section is about constructing the
SSLEngine; closing-policy semantics would sit better next to theTLSClosing/TLS()documentation, and the paragraph never mentionspekko.stream.materializer.tls.engine, so a reader cannot tell whether it applies to them. - Fix TLS GraphStage buffer allocation and growth #3728 inserts a different paragraph at this exact position, so whichever PR merges second will conflict here and both paragraphs need to be kept.
Motivation: Transport half-close could preempt draining accepted plaintext, output cancellation could hang with IgnoreCancel/IgnoreBoth, and delayed input cancellation could swallow upstream failure. Modification: Defer inbound close until the accepted batch drains, suppress pump readiness while it awaits transport demand, and terminate when the transport output closes for any policy. Preserve upstream failures during delayed cancellation. Rename the cancellation predicate, limit the harness topology change to custom attributes, and align Scala/Java docs with the drain bound and engine key. Result: Accepted plaintext survives peer transport completion, cancelled outputs terminate, and delayed upstream failure still fails the transport. Tests: - Five new regression cases failed against the original PR implementation. - JDK 25: all 15 focused close/cancellation tests passed. - Native scalafmt and git diff --check passed. - Qoder stdout review: No must-fix findings after resolving TCP control concern. - Existing TcpSpec pre-acceptance race reproduces with origin/main TLS. - Full validation, docs, style, and binary compatibility run in remote CI. References: Refs apache#3727, Refs apache#2860.
Motivation: The TLS closing-policy guide needs to distinguish cancellation observed before peer transport completion from the existing inbound-close contract. Modification: Add the ordering qualification to the TLS closing policies section. Keep production behavior and existing test assertions unchanged. Result: The guide explains that the GraphStage drain covers cancellation observed first, while transport completion observed first retains the legacy discard contract. Tests: - sbt docs/paradox: passed; duplicate-anchor warnings in unchanged release notes. - TlsGraphStageDeferredCloseSpec: 15 passed on OpenJDK 25.0.2. - headerCreateAll and +headerCheckAll: passed. - git diff --check: passed. - Original graph-stage TcpSpec still intermittently returns hell; investigated separately with 20 baseline and 20 diagnostic repetitions per engine/revision. - Auditable Qoder stdout review: No must-fix findings. - validatePullRequest and MiMa: Not run - docs only; full validation is reserved for remote CI as requested. References: Refs apache#2860 and apache#3727.
MotivationThis corrects the 2026-10-28 23:35:00Z comment, including its reference to ModificationThe rechecked PR code revision is Repeated tests also show that the echo failure is not evidence that the final byte was already accepted when cancellation was observed. Every failing diagnostic run on both main and this head logged ResultThe accepted-plaintext drain fix remains valuable, but this PR does not fix the intermittent original TcpSpec failure. Track that cancellation-versus-delivery race separately in #3729; the precise scheduling boundary responsible for the ordering remains unlocalized. The peer-completion claim is also limited to cancellation being observed first. Peer transport-input completion observed first retains the existing discard contract, matching the legacy actor engine. Tests
ReferencesRefs #2860 and #3729. This correction concerns #3727; #3728 remains the separate buffer change. |
Motivation: Sender-side boundary batching can send plaintext-output cancellation before plaintext that the TLS handler has already delivered to its output boundary. Modification: Opt TLS input boundaries out of sender-side batching using an internal attribute, retaining asynchronous isolation and existing input-buffer sizes. Add deterministic TLS 1.2 and TLS 1.3 regressions and document the scope. Result: Delivered plaintext reaches the TLS boundary before cancellation. Preserve the original TCP assertion, closing policy, and TLS engine close semantics. Tests: - Regression: 0/2 before, 2/2 after; related TLS 252/252 and fusing 37/37. - Original TCP TLS test: 20/20 per engine, plus one sbt run per engine. - Integration with apache#3727/apache#3728: deferred close 15/15, TLS 252/252, another 20/20 TCP tests per engine. - headerCreateAll, +headerCheckAll, docs/paradox, native scalafmt check, and git diff --check passed. - Full validatePullRequest and cross-project MiMa reserved for remote CI per user instruction; CI not followed. References: Fixes apache#3729
Motivation: Sender-side boundary batching could deliver plaintext-output cancellation before plaintext already delivered by the same handler. TLS could also close outbound before encrypting plaintext accepted into its deferred batch. Modification: - Opt TLS input boundaries out of sender-side batching while retaining asynchronous isolation and the existing input-buffer sizes. - Stop accepting input and drain the established session's accepted data with backpressure. Preserve transport-close ordering, failures during delayed cancellation, and prompt termination when a new handshake would be required. - Add deterministic ordering and deferred-close regressions, document drain limits in both DSLs and the guide, and correct the IgnoreCancel API reference. Result: Boundary messages sent by a handler precede its subsequent cancellation. Accepted plaintext drains when cancellation is observed before peer transport completion, subject to the documented limits. Preserve the peer-completes-first discard contract, original TCP hello assertion, and default closing policy. Tests: - Unchanged production/test tree from bae8200: deferred-close and cancellation suites 17/17; original GraphStage TCP TLS case 1/1. - Ordering regression on main: 0/2 before the fix, 2/2 afterwards; related TLS suites 252/252, fusing/interpreter suites 37/37, TCP TLS 20/20 per engine. - Temporary apache#3727/apache#3728 integration: all 15 deferred-close cases and related TLS suites passed; an additional 20/20 TCP TLS runs per engine passed. - sbt docs/paradox and git diff --check passed after the documentation fix. - Existing headerCreateAll, +headerCheckAll, and native scalafmt checks passed; no Scala code, test, imports, or headers changed during squash/review. - Auditable Qoder stdout review and independent review: no must-fix findings. - Full validatePullRequest and cross-project MiMa left to remote validation per user instruction; CI results were not followed or claimed to pass. References: Fixes apache#3729. Refs apache#2860.
bae8200 to
5c42a2d
Compare
Motivation: Sender-side boundary batching could deliver plaintext-output cancellation before plaintext already delivered by the same handler. TLS could also close outbound before encrypting plaintext accepted into its deferred batch. Modification: - Opt TLS input boundaries out of sender-side batching while retaining asynchronous isolation and the existing input-buffer sizes. - Stop accepting input and drain the established session's accepted data with backpressure. Preserve transport-close ordering, failures during delayed cancellation, and prompt termination when a new handshake would be required. - Add deterministic ordering, deferred-close, and partially sent large-message regressions. Document drain limits in both DSLs and the guide, including the correct IgnoreCancel API reference. Result: Boundary messages sent by a handler precede its subsequent cancellation. Accepted plaintext drains when cancellation is observed before peer transport completion, subject to the documented limits. Preserve the peer-completes-first discard contract, original TCP hello assertion, and default closing policy. Tests: - Large-message cancellation regression for IgnoreComplete and EagerClose: 0/2 passed with the original main TlsGraphStage implementation, 2/2 with the current implementation. Deferred-close and cancellation suites: 19/19. - Previously recorded ordering regression on main: 0/2 before, 2/2 after; related TLS suites 252/252, fusing/interpreter suites 37/37, TCP TLS 20/20 per engine. The combined PR's original GraphStage TCP TLS case passed 1/1. - Temporary apache#3727/apache#3728 integration: all 15 original deferred-close cases and related TLS suites passed; another 20/20 TCP TLS runs per engine passed. - Native scalafmt and git diff --check passed for the new tests. Production code is unchanged by this test-only follow-up. - Existing docs/paradox, headerCreateAll and +headerCheckAll checks passed; documentation, imports, source-file headers, and public API are unchanged. - Auditable Qoder stdout review and independent review: no remaining must-fix findings after checking the test against the implementation and API definitions. - Full validatePullRequest and cross-project MiMa left to remote validation per user instruction; CI results were not followed or claimed to pass. References: Fixes apache#3729. Refs apache#2860.
5c42a2d to
5a887c7
Compare
Motivation: Small TCP pool buffers prevent TLS initialization and cannot satisfy output growth. Deferred ciphertext, session changes and heap allocation also need safe capacity and ownership handling. Modification: Use stage-owned heap buffers when the TCP pool is too small, sharing one initial allocation between three disjoint transport slices. Flush before growing, refresh session requirements and batching, and shrink drained output after a handshake lowers its capacity requirement. Validate pooled capacity and recycling. Rebase on main while preserving cancellation/draining coverage, add review and integration regressions, and document allocation tradeoffs. Result: TLS progresses with small TCP buffers without losing ciphertext or polluting the shared pool. Transient session capacity can be reclaimed without repeated growth, and initialization uses one backing heap allocation instead of three. Tests: - Two new review regressions failed before the fix and passed afterwards. - JDK 21: 310 related TLS/TCP tests passed, including 28 focused cases. - JDK 17 and 25: 28 focused cases passed on each runtime via ScalaTest. - Native scalafmt, headerCreateAll, +headerCheckAll and git diff --check passed. - Qoder stdout review and independent review: No must-fix findings. - Full local MiMa stopped at user request; full validation, docs, import/style and binary compatibility checks are delegated to remote CI. References: Refs apache#2860, Refs apache#3727, Refs apache#3728.
Motivation: TLS GraphStage cannot initialize when the TCP pool is smaller than the SSLEngine packet requirement, and fixed-size pooled buffers cannot satisfy growth. The partial-record regression test also races handshake processing. Modification: Use stage-owned heap buffers when needed, sharing the initial allocation between three disjoint transport slices. Flush before growing, preserve pending data, refresh session requirements and batching, and shrink drained output after renegotiation. Keep foreign buffers out of the TCP pool. Add buffer and cancellation regressions and document allocation behavior. Synchronize the test harness with session verification and check query counts after cancellation acknowledges completion of unwrap processing. Result: Small TCP buffers no longer prevent TLS progress. Buffer growth and recycling preserve data and ownership, and the partial-record test checks session query behavior without racing the stage. Tests: - JDK 17: 312 TLS/TCP tests passed, including 28 focused buffer/close cases. - Negative control: unconditional underflow session queries fail with 3 != 2; the temporary mutation was restored. - Native scalafmt, headerCreateAll, checkCodeStyle and git diff --check passed. - +mimaReportBinaryIssues passed on Scala 2.13.18 and 3.3.9. - sortImports encountered a Scalafix/Scalameta NoSuchMethodError; its unrelated edits were reverted and the native formatter check passed. - docs/paradox stopped at user request; validatePullRequest and the remaining Scala test matrix are delegated to remote CI. References: Refs apache#2860, Refs apache#3727, Refs apache#3728.
Motivation: TLS GraphStage cannot initialize when the TCP pool is smaller than the SSLEngine packet requirement, and fixed-size pooled buffers cannot satisfy growth. The partial-record regression test also races handshake processing. Modification: Use stage-owned heap buffers when needed, sharing the initial allocation between three disjoint transport slices. Flush before growing, preserve pending data, refresh session requirements and batching, and shrink drained output after renegotiation. Keep foreign buffers out of the TCP pool. Add buffer and cancellation regressions and document allocation behavior. Synchronize the test harness with session verification and check query counts after cancellation acknowledges completion of unwrap processing. Result: Small TCP buffers no longer prevent TLS progress. Buffer growth and recycling preserve data and ownership, and the partial-record test checks session query behavior without racing the stage. Tests: - JDK 17: 312 TLS/TCP tests passed, including 28 focused buffer/close cases. - Negative control: unconditional underflow session queries fail with 3 != 2; the temporary mutation was restored. - Native scalafmt, headerCreateAll, checkCodeStyle and git diff --check passed. - +mimaReportBinaryIssues passed on Scala 2.13.18 and 3.3.9. - sortImports encountered a Scalafix/Scalameta NoSuchMethodError; its unrelated edits were reverted and the native formatter check passed. - docs/paradox stopped at user request; validatePullRequest and the remaining Scala test matrix are delegated to remote CI. References: Refs #2860, Refs #3727, Refs #3728.
Motivation: When the plaintext output is cancelled, the GraphStage TLS engine drains plaintext it has already accepted. If a TLS 1.3 peer sent close_notify during that drain, unwrap returned CLOSED and the stage completed, discarding the accepted plaintext without sending it or its own close_notify, although TLS 1.3 close_notify only closes the peer's sending direction. Modification: - Stay in the inbound-closed drain when unwrap returns CLOSED while accepted plaintext is still being drained and the engine's outbound side is open. The drain then finishes and closes outbound as before. - Document that a TLS 1.3 close_notify does not end the drain, while a TLS 1.2 close_notify (which closes both SSLEngine directions) does. - Add real-SSLEngine tests that send close_notify during the drain for IgnoreComplete and EagerClose, on TLS 1.3 and TLS 1.2. Result: A TLS 1.3 peer that half-closes receives the accepted response followed by close_notify. TLS 1.2 behaviour is unchanged and terminates promptly. Tests: - sbt "stream-tests / Test / testOnly org.apache.pekko.stream.io.TlsGraphStageCancellationSpec": the two TLS 1.3 tests failed before the fix (partial response) and pass after it; 6/6 pass. - sbt "stream-tests / Test / testOnly org.apache.pekko.stream.io.Tls*": 281/281 passed. - Native scalafmt and git diff --check passed. References: Refs #3727
* Keep draining TLS plaintext after a TLS 1.3 KeyUpdate Motivation: On plaintext-output cancellation the GraphStage TLS engine drains plaintext it has already accepted, but stops if a new handshake is required. It treated any NEED_WRAP as a new handshake, so a TLS 1.3 KeyUpdate that requests a response, received before or during the drain, closed outbound and discarded the accepted plaintext. Modification: - Allow the drain to continue when an established TLS 1.3 session needs to wrap. TLS 1.3 has no renegotiation, so this is a post-handshake message such as a KeyUpdate response; the response is wrapped before the plaintext. - TLS 1.2 keeps stopping the drain, because NEED_WRAP may start a renegotiation. - Document the KeyUpdate case in the stream IO guide and both TLS DSLs. Result: A TLS 1.3 KeyUpdate no longer discards plaintext accepted before plaintext-output cancellation. Tests: - stream-tests/testOnly TlsGraphStageDeferredCloseSpec: 25 passed; the two new TLS 1.3 KeyUpdate tests fail without the fix (ByteString() was not equal to ByteString(92, 90)), the TLS 1.2 guard passes both ways - stream-tests/testOnly org.apache.pekko.stream.io.Tls*: 288 passed - scalafmt --mode diff-ref=upstream/main: clean References: Refs #3727 * Match SunJSSE's FINISHED status for scripted KeyUpdate wraps Motivation: Review noted the KeyUpdate tests rely on a scripted engine. A real SunJSSE TLS 1.3 KeyUpdate (forced with a low jdk.tls.keyLimits) reports NEED_WRAP after unwrap, and the response wrap returns FINISHED, not NOT_HANDSHAKING as scripted. FINISHED re-runs session verification. Modification: Return FINISHED from the scripted KeyUpdate response wrap, leaving the engine in NOT_HANDSHAKING afterwards, like SunJSSE. Result: The drain tests now cover the FINISHED path a real KeyUpdate takes. Tests: - stream-tests/testOnly org.apache.pekko.stream.io.TlsGraphStage*: 173 passed. - With the fix disabled, both TLS 1.3 KeyUpdate tests fail. - Standalone JDK 17 SunJSSE engine pair with low jdk.tls.keyLimits confirmed the NEED_WRAP then FINISHED sequence on TLSv1.3. References: Review of #3738
Add apache#3727, apache#3728, apache#3731, apache#3734, apache#3738 (TLS), apache#3730 (fanout publisher performance) and apache#3733 (shard lease release logging). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add apache#3727, apache#3728, apache#3731, apache#3734, apache#3738 (TLS), apache#3730 (fanout publisher performance) and apache#3733 (shard lease release logging).
Motivation
The GraphStage TLS engine could close outbound before encrypting plaintext it had already accepted into its deferred batch.
The original four-argument TcpSpec echo failure is a separate cancellation-versus-delivery race, tracked in #3729. This PR does not modify or restore that test and does not claim to eliminate its intermittent failure.
Modification
Result
Already-accepted plaintext is retained when plaintext-output cancellation is observed before the peer's transport input completes, subject to the documented drain limits. When peer transport-input completion is observed first, the existing inbound-close contract applies and accepted plaintext is discarded, matching the legacy actor engine. Legacy TLS behavior remains unchanged. Data not yet accepted when cancellation is observed is outside the drain guarantee, including data delivered afterwards.
Tests
a55a008fdfhad 0 legacy failures and 8 GraphStage failures (40%); PR205155160ahad 0 legacy failures and 13 GraphStage failures (65%). All failures returnedhellinstead ofhello.h/e/l/l -> cancellation -> o; successes acceptedhellobefore cancellation. Thus onlyhellhad been accepted when cancellation was observed, although a lateonPush(o)occurred afterwards. Legacy receipt/chop logs containedhello. All diagnostic changes were removed.TlsGraphStageDeferredCloseSpecpassed all 15 tests;sbt docs/paradox,headerCreateAll,+headerCheckAll, andgit diff --checkpassed. Paradox reported duplicate anchors in unchanged release-note pages. An additional sbt invocation of the original GraphStage TcpSpec still failed withhell.setexpression failed before tests; one preliminary suite aborted because a concurrent build changed version resources. Both were resolved before the clean serial batches.References
Refs #2860. TcpSpec cancellation/delivery follow-up: #3729. Buffer allocation and session-size growth are addressed separately in #3728.