Repository navigation
De-flake DistributedPubSubRestartSpec: the restarted node makes first contact, and the survivor waits on it - #8552
Merged
Aaronontheweb merged 4 commits intoSep 10, 2026
Conversation
Aaronontheweb
added a commit
that referenced
this pull request
Sep 9, 2026
…d, use the conductor's async shutdown, keep the port assertion inside the cleanup Addresses review findings on PR #8552: - Shutdown.PreStart no longer sends the ready ping once; it resends every 500ms on an undilated self-scheduled timer until first's ReadyPingForwarder acks it back to the sender, cancelling the timer on ack (and defensively in PostStop). first still waits once for the first ping it sees and ignores later resends. This removes the dependency on any Artery handshake-stage fix - a lost ping is simply retried instead of failing the 60s wait. - Replace TestConductor.Shutdown(...) with the ShutdownAsync(RoleName, ...) overload, keeping the same 30s bound. - Move the restarted-address port assertion inside the try/finally that owns newSystem, so a failed assertion still terminates it. - State the measured worst case behind the 60s ready-ping wait and re-derive newSystem.WhenTerminated's bound (120s -> 155s) against the current 110s worst-case pipeline, restoring its original 45s of slack.
… contact, and the survivor waits on it first's association to third's old incarnation carries no signal that a same-address restart even started; the survivor's association only learns the new incarnation exists from a frame the restarted node itself sends. The old test started its 45s clock at Shutdown()'s return, before any of third's restart cost had begun, and none of the ten prior fixes changed who makes first contact after the restart - they all kept first polling a stale association instead. Now third's Shutdown actor pings a forwarder on first the moment it exists (PreStart), so first waits on that ping (dilated 60s) before running a short 20s closed-loop kill over ActorSelection.Tell (only an ActorSelectionMessage pierces a quarantined association). The ping's own inbound handshake is what heals first's association as a side effect. barrier-timeout goes to 180s to fit the new worst case with headroom, and a bound-address assertion on third settles, on the next Linux failure, whether the fresh listener came up on the pinned port.
…d, use the conductor's async shutdown, keep the port assertion inside the cleanup Addresses review findings on PR #8552: - Shutdown.PreStart no longer sends the ready ping once; it resends every 500ms on an undilated self-scheduled timer until first's ReadyPingForwarder acks it back to the sender, cancelling the timer on ack (and defensively in PostStop). first still waits once for the first ping it sees and ignores later resends. This removes the dependency on any Artery handshake-stage fix - a lost ping is simply retried instead of failing the 60s wait. - Replace TestConductor.Shutdown(...) with the ShutdownAsync(RoleName, ...) overload, keeping the same 30s bound. - Move the restarted-address port assertion inside the try/finally that owns newSystem, so a failed assertion still terminates it. - State the measured worst case behind the 60s ready-ping wait and re-derive newSystem.WhenTerminated's bound (120s -> 155s) against the current 110s worst-case pipeline, restoring its original 45s of slack.
Aaronontheweb
force-pushed
the
fix/distributed-pubsub-restart-reverse-contact
branch
from
September 9, 2026 18:18
ee32c39 to
8c7a270
Compare
…estarted system terminates Build 131332 (this PR's own CI run, Linux Artery) showed third's Shutdown actor replying "shutdown-ack" and calling Context.System.Terminate() inline in the same handler; Artery aborted third's outbound streams before the ack could flush, so it never reached first. First's closed-loop kill then retried "shutdown" every 500ms against an already-exited process and its 20s bound timed out. Local runs only passed because the ack happened to win that race on a fast machine. Slide the terminate behind a 2s timer instead of calling it inline - same shape as the Subject actor in RemoteNodeRestartDeathWatchSpec (PR #8557) - so a live system, and the warmed-up lane under it, always outlasts whichever "shutdown" attempt first's loop last sends. Cancel the timer in PostStop.
…e; restore the 120 s termination wait; last sync call migrated
Aaronontheweb
commented
Sep 10, 2026
Aaronontheweb
left a comment
Member
Author
There was a problem hiding this comment.
LGTM - this spec has consistently been the worst offender in the entire test suite for years and the addition of the Artery MNTR specs exposed some additional cases where things like quarantine didn't pierce it correctly.
Aaronontheweb
deleted the
fix/distributed-pubsub-restart-reverse-contact
branch
September 10, 2026 13:13
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
… contact, and the survivor waits on it (#8552) * De-flake DistributedPubSubRestartSpec: the restarted node makes first contact, and the survivor waits on it first's association to third's old incarnation carries no signal that a same-address restart even started; the survivor's association only learns the new incarnation exists from a frame the restarted node itself sends. The old test started its 45s clock at Shutdown()'s return, before any of third's restart cost had begun, and none of the ten prior fixes changed who makes first contact after the restart - they all kept first polling a stale association instead. Now third's Shutdown actor pings a forwarder on first the moment it exists (PreStart), so first waits on that ping (dilated 60s) before running a short 20s closed-loop kill over ActorSelection.Tell (only an ActorSelectionMessage pierces a quarantined association). The ping's own inbound handshake is what heals first's association as a side effect. barrier-timeout goes to 180s to fit the new worst case with headroom, and a bound-address assertion on third settles, on the next Linux failure, whether the fresh listener came up on the pinned port. * DistributedPubSubRestartSpec: repeat the ready ping until acknowledged, use the conductor's async shutdown, keep the port assertion inside the cleanup Addresses review findings on PR #8552: - Shutdown.PreStart no longer sends the ready ping once; it resends every 500ms on an undilated self-scheduled timer until first's ReadyPingForwarder acks it back to the sender, cancelling the timer on ack (and defensively in PostStop). first still waits once for the first ping it sees and ignores later resends. This removes the dependency on any Artery handshake-stage fix - a lost ping is simply retried instead of failing the 60s wait. - Replace TestConductor.Shutdown(...) with the ShutdownAsync(RoleName, ...) overload, keeping the same 30s bound. - Move the restarted-address port assertion inside the try/finally that owns newSystem, so a failed assertion still terminates it. - State the measured worst case behind the 60s ready-ping wait and re-derive newSystem.WhenTerminated's bound (120s -> 155s) against the current 110s worst-case pipeline, restoring its original 45s of slack. * DistributedPubSubRestartSpec: let the shutdown ack leave before the restarted system terminates Build 131332 (this PR's own CI run, Linux Artery) showed third's Shutdown actor replying "shutdown-ack" and calling Context.System.Terminate() inline in the same handler; Artery aborted third's outbound streams before the ack could flush, so it never reached first. First's closed-loop kill then retried "shutdown" every 500ms against an already-exited process and its 20s bound timed out. Local runs only passed because the ack happened to win that race on a fast machine. Slide the terminate behind a 2s timer instead of calling it inline - same shape as the Subject actor in RemoteNodeRestartDeathWatchSpec (PR #8557) - so a live system, and the warmed-up lane under it, always outlasts whichever "shutdown" attempt first's loop last sends. Cancel the timer in PostStop. * DistributedPubSubRestartSpec: terminate timer outlives the retry cycle; restore the 120 s termination wait; last sync call migrated (cherry picked from commit 2cc9d1e)
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
DistributedPubSubRestartSpec.cs (backported from #8552) dual-pinned the fresh ActorSystem's port for both transports, including akka.remote.artery.canonical.port, and its surrounding comments explained the fix using Artery-specific classes (OutboundHandshakeStage, InboundHandshakeStage.HandleReq, ArteryRemoting.Send) and a CI run captured on the Linux Artery lane. v1.5 has no Artery transport at all (src/core/Akka.Remote/Artery does not exist on this branch), so the second config line is dead HOCON and the class names in the comments point at code that isn't there. Dropped the artery.canonical.port line (only dot-netty.tcp.port is needed on a classic-only branch) and reworded the affected comments to describe the same reasoning in transport-neutral terms, without naming Artery classes. No behavior change: the spec exercises the classic DotNetty transport exactly as it did before this commit. Checked every other file touched by this backport for "artery"/"Artery" mentions; the remaining ones (ClusterDeathWatchSpec.cs, NodeChurnSpec.cs, RemoteDeliverySpec.cs, RemoteNodeDeathWatchSpec.cs) only reference Artery in passing, as comparison context for a classic-transport fix, and set no Artery configuration - left unchanged.
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
DistributedPubSubRestartSpec.cs (backported from #8552) dual-pinned the fresh ActorSystem's port for both transports, including akka.remote.artery.canonical.port, and its surrounding comments explained the fix using Artery-specific classes (OutboundHandshakeStage, InboundHandshakeStage.HandleReq, ArteryRemoting.Send) and a CI run captured on the Linux Artery lane. v1.5 has no Artery transport at all (src/core/Akka.Remote/Artery does not exist on this branch), so the second config line is dead HOCON and the class names in the comments point at code that isn't there. Dropped the artery.canonical.port line (only dot-netty.tcp.port is needed on a classic-only branch) and reworded the affected comments to describe the same reasoning in transport-neutral terms, without naming Artery classes. No behavior change: the spec exercises the classic DotNetty transport exactly as it did before this commit. Checked every other file touched by this backport for "artery"/"Artery" mentions; the remaining ones (ClusterDeathWatchSpec.cs, NodeChurnSpec.cs, RemoteDeliverySpec.cs, RemoteNodeDeathWatchSpec.cs) only reference Artery in passing, as comparison context for a classic-transport fix, and set no Artery configuration - left unchanged.
Aaronontheweb
added a commit
that referenced
this pull request
Sep 12, 2026
DistributedPubSubRestartSpec.cs (backported from #8552) dual-pinned the fresh ActorSystem's port for both transports, including akka.remote.artery.canonical.port, and its surrounding comments explained the fix using Artery-specific classes (OutboundHandshakeStage, InboundHandshakeStage.HandleReq, ArteryRemoting.Send) and a CI run captured on the Linux Artery lane. v1.5 has no Artery transport at all (src/core/Akka.Remote/Artery does not exist on this branch), so the second config line is dead HOCON and the class names in the comments point at code that isn't there. Dropped the artery.canonical.port line (only dot-netty.tcp.port is needed on a classic-only branch) and reworded the affected comments to describe the same reasoning in transport-neutral terms, without naming Artery classes. No behavior change: the spec exercises the classic DotNetty transport exactly as it did before this commit. Checked every other file touched by this backport for "artery"/"Artery" mentions; the remaining ones (ClusterDeathWatchSpec.cs, NodeChurnSpec.cs, RemoteDeliverySpec.cs, RemoteNodeDeathWatchSpec.cs) only reference Artery in passing, as comparison context for a classic-transport fix, and set no Artery configuration - left unchanged.
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.
What changes
DistributedPubSubRestartSpecreverses who makes first contact after the restart.thirdsystem's ownShutdownactor pings a forwarder onfirstfrom itsPreStart, so "the ping arrived" and "the actor exists" are one fact. It repeats the ping every 500 ms on a scheduler timer untilfirstacks it, and cancels the timer on the ack and inPostStop. A single ping is not enough on its own: it rides the ordinary lane, and Artery's handshake stage holds one element there while the handshake to the new incarnation is gating. If that stage stops first, the held element is lost.Shutdownactor acknowledges a kill and then terminates its system 5 s later on a sliding timer, re-armed by every repeated kill, instead of terminating inline. The acknowledgement has to leave on a live transport, and the timer has to outlive one full retry cycle of the kill loop, 2 s of waiting plus a 500 ms interval, so a lost ack still gets a second attempt at a live target.firstacks every ping straight back to its sender and hands the signal to a probe.firstwaits on that probe once, with a 60 s bound, and ignores later pings. Only then does it send the shutdown command, withActorSelection.Telland a fresh probe per attempt over a 20 s loop, because only a selection message crosses a quarantined association.thirdasserts, right after its restart and inside the block that owns the new system, that it is bound to the pinned port, so a failed assertion still shuts that system down and the next failure on the Linux lane settles whether the listener came up where it should.thirdstays at 120 s. Its clock starts afterthirdcreates theShutdownactor, whose start is what sends the ready ping, sofirst's 30 s shutdown cap and its ready wait are already spent by then. What remains offirst's pipeline is the 20 s kill loop, its last 2 s attempt, and the 5 s terminate timer, about 27 s, so 120 s carries about 90 s of margin.The earlier fixes in this file stay: the port pin, the transport failure-detector bound, and the double-sampled delta count. The
ResolveOnewith a fresh temporary actor is replaced by the selection send, which carries the same freshness on the probe.Why
This spec failed six times in three days on the Artery lanes (builds 131137, 131156, 131161, 131174, 131187, 131188), always node
first, always aResolveOneagainst the restarted node timing out after 17 attempts inside a 45 s window. It has an open issue since 2016 and ten fix commits since 2025, some of which reversed each other.None of them changed who makes first contact.
first's association tothird's address can only learnthird's new identity from a framethirdsends: promotion runs from the inbound handshake stage, and after an identity change the ordinary lane needs a second inbound event because the association state resets its handshake flag. There is no cluster event or reachability change to await, because the fresh system joins itself by design. The old test started its 45 s clock the moment the conductor's shutdown call returned, beforethirdhad begun its 8 to 13 s graceful shutdown, about 6 s before its actors existed, and up to 21 s of re-association. The window overlapped the downtime almost entirely. Now the survivor waits on the one signal that means the restart is complete, and that signal is also what heals the association. The measured worst case for that sequence is 12.9 s of shutdown, 6 s before the actors exist, and 21 s of re-association, 39.9 s in total, so the 60 s bound leaves about 20 s of margin.Two Artery defects made the re-association slow and are fixed in #8554: the reconnect backoff was bypassed, measured at five control reconnects per second against a configured one, and a handshake stage could lose an element it had taken from the control queue. Two more are filed as issues, #8550 and #8551. The spec does not depend on any of them, because a lost ping is resent, not fatal.
How it was checked
dotnet build src/contrib/cluster/Akka.Cluster.Tools.Tests.MultiNode -c Release -warnaserrorclean. The spec run locally through the multi-node adapter at the current head: classic transport, 3 of 3 passed in 10 s; Artery, the lane that fails on CI, 3 of 3 passed twice, 28 s each. The grep for synchronous TestKit calls on the file returns only comments, the localShutdownactor class, and the pre-existingShutdown(ActorSystem)override.Third commit: what this PR's own CI run showed
Build 131332, Linux Artery, all three node logs read from the failed-run artifact. The ready ping did its job: the restarted
thirdbound the pinned port 5 s after the kill, created itsShutdownactor at 57.947, andfirsthad the ping and sent the kill within 30 ms. Thenthirdreplied and calledTerminatein the same handler. Artery aborted its outbound streams before any flush, the acknowledgement's stream among them, so the ack never left.third's process exited with a pass 50 ms later, andfirstretried the kill against a closed port for 20 s and failed. The local runs passed because the ack won that race on a fast machine.The
Shutdownactor now replies and arms a sliding terminate timer, cancelled inPostStop, the same shape as theSubjectactor in #8557. The Artery flush fix in #8554 would usually carry the ack out as well, but this spec must not depend on it. Run three times on each transport after the change: 3 of 3 passed each time, 11 to 27 s classic, 12 to 13 s Artery. A temporary log line, removed before the commit, confirmed the ack leaving 2.0 s before Artery's teardown onthird.Fourth commit: what the adversarial review found
The first version of the timer was 2 s, shorter than the 2.5 s retry cycle it exists to outlive, so a lost ack would still have killed the target half a second before the next attempt. It is 5 s now, with the arithmetic in the comment. The termination wait had been raised to 155 s on a sum that double-counted
first's already-spent waits; it is back at 120 s with the corrected arithmetic above. And one synchronous call had survived the migration, theNode(to)lookup in the join helper, nowNodeAsync. Run three times on each transport after the change: 3 of 3 passed each time, 20 to 33 s.