Skip to content

De-flake RemoteNodeDeathWatchSpec: barrier after the subject stops, resend-derived bound, identify asserts the actor exists - #8581

Merged
Aaronontheweb merged 2 commits into
devfrom
fix/remote-deathwatch-spec-barrier-after-stop
Sep 10, 2026
Merged

Aaronontheweb merged 2 commits into
devfrom
fix/remote-deathwatch-spec-barrier-after-stop

Conversation

@Aaronontheweb

@Aaronontheweb Aaronontheweb commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

What changes

RemoteNodeDeathWatchSpec (the Slow and Fast variants) gets a barrier between the subject being stopped and the remote watcher's wait for Terminated, so that wait opens on the real event instead of on a timer that happens to be the same length as the other node's sleep.

  • Phase 1. second now watches the subject from its own test actor, stops it, awaits the local Terminated, and only then enters a new subject-stopped-1 barrier. first and third enter the same barrier, and first waits for WrappedTerminated after it. The actor cell notifies remote watchers before local ones, so the local Terminated on second proves the DeathWatchNotification bound for first was already handed to remoting.
  • The bound. first's wait uses an explicit 6 s, derived from akka.remote.resend-interval (2 s): two resend cycles for a lost or unacked system message plus one hop and scheduling slack. It is not derived from the watch failure detector, which never fires here because the peer stays alive and heartbeating. The same bound replaces the flat 3 s default at the phase 4 wait. That barrier is entered right after Sys.Stop, so it proves the stop was issued rather than observed, a weaker point than the new phase 1 shape; the 6 s covers the extra guardian hop. Passed undilated, since the timeout parameter dilates itself. The value reads the classic resend interval; Artery resends system messages every 1 s, so the same budget covers it with more room.
  • Identify. The helper asserts that ActorIdentity.Subject is not null, naming the node and actor path. Upstream has this assertion; the .NET port had dropped it, so a missing actor surfaced later as an unrelated Ack timeout.
  • Async migration of what was left in the file: the remote-watcher lookup and the identify helper (including NodeAsync in place of the blocking Node), one RunOn, and one ReceiveN.

Why

Build 131425, Windows multi-node, classic transport, on #8554 (whose Artery change is not on this code path): RemoteNodeDeathWatchSlowSpec failed on first with a 3 s timeout waiting for WrappedTerminated in phase 1. Nothing arrived at all.

The phase had first and second each sleep 3 s independently after the watch-established-1 barrier, then second stopped the subject while first waited the flat 3 s single-expect default. Nothing ordered the stop before the wait, so the window had to absorb the skew between two Task.Delay(3000) timers on a loaded agent plus the remote notification hop, and a DeathWatchNotification that is lost once is only resent every 2 s. A 3 s window could not survive a single lost system message by construction. Upstream has the same shape, so this is a test-authoring gap rather than a product regression.

Two other 3 s waits in the file (the hello6 and hello7 phases) have a similar shape but no stop involved, and mirror upstream. They are left for a separate change rather than folded in here.

Later commit

The second commit answers review. The phase 1 comment named the local watcher as the remote one; the remote watcher recorded on the subject is first's /system/remote-watcher, which re-issues the watch over the wire and forwards the notification locally. The bound's comment now says which transport its resend value belongs to. A 1 s bound that the first commit had added to the nested expect inside AssertCleanup is removed again: the predicate overload asserts rather than filters, so the retry loop was already content-driven, and a timed-out attempt could offset later requests from their replies.

How it was checked

dotnet build src/core/Akka.Remote.Tests.MultiNode -c Release -warnaserror clean. The three RemoteNodeDeathWatch* specs (six tests) run through the multi-node adapter: classic transport four times, Artery three times, all passed, about 80 s per run; once more on classic after the second commit, 6 of 6. The first commit also passed a full CI run on two heads. The grep for synchronous TestKit calls, blocking waits, and sleeps on the file returns only the actor-side Context.Watch and a doc comment. Test-only change. No ledger entry.

@Aaronontheweb
Aaronontheweb force-pushed the fix/remote-deathwatch-spec-barrier-after-stop branch from efc4628 to 302df84 Compare September 10, 2026 19:50
… the watcher's wait starts on the real event; resend-derived bound; identify asserts the actor exists

Phase 1 had first waiting a flat 3s (single-expect-default) for WrappedTerminated
while second independently slept 3s before calling Sys.Stop, with no barrier
between the two sleeps and the stop. The window had to absorb both timers'
drift plus the remote notification hop, and a lost DeathWatchNotification is
only resent every 2s. second now does a local WatchAsync + Stop +
ExpectTerminatedAsync before entering a new "subject-stopped-1" barrier;
TellWatchersWeDied notifies remote watchers before local ones, so the local
Terminated proves the remote notification already left. first's wait now
opens after that barrier and uses an explicit 6s bound derived from
akka.remote.resend-interval (two resend cycles plus a hop), not the failure
detector (the peer stays alive and heartbeating here, so that path never
fires). Applied the same derived bound to the equivalent wait in phase 4,
which already had the correct barrier shape.

Restored the identify helper's dropped assertion (ActorIdentity.Subject must
not be null, naming the actor path) so a missing actor fails at the identify
instead of surfacing as an unrelated Ack timeout downstream.

Also migrated the file's remaining synchronous TestKit calls to their async
counterparts: the remote-watcher lookup, the identify helper, RunOn, and
ReceiveN, plus an explicit bound on AssertCleanup's previously-unbounded
nested expect.
…, note Artery's resend interval, drop the AssertCleanup bound

The remote watcher recorded on the subject is first's /system/remote-watcher,
which re-issues the ProbeActor's watch over the wire; the comment named the
local watcher. The 6s bound reads the classic resend-interval; Artery resends
at 1s, so the same budget covers it. The 1s bound on AssertCleanup's nested
expect is removed: the predicate overload asserts rather than filters, so the
retry loop was already content-driven, and a timed-out attempt could offset
later requests from their replies.
@Aaronontheweb
Aaronontheweb force-pushed the fix/remote-deathwatch-spec-barrier-after-stop branch from 302df84 to b68cde7 Compare September 10, 2026 20:45
@Aaronontheweb
Aaronontheweb merged commit 1ad1abe into dev Sep 10, 2026
15 checks passed
@Aaronontheweb
Aaronontheweb deleted the fix/remote-deathwatch-spec-barrier-after-stop branch September 10, 2026 21:20
Aaronontheweb added a commit that referenced this pull request Sep 12, 2026
…esend-derived bound, identify asserts the actor exists (#8581)

* De-flake RemoteNodeDeathWatchSpec: barrier after the subject stops so the watcher's wait starts on the real event; resend-derived bound; identify asserts the actor exists

Phase 1 had first waiting a flat 3s (single-expect-default) for WrappedTerminated
while second independently slept 3s before calling Sys.Stop, with no barrier
between the two sleeps and the stop. The window had to absorb both timers'
drift plus the remote notification hop, and a lost DeathWatchNotification is
only resent every 2s. second now does a local WatchAsync + Stop +
ExpectTerminatedAsync before entering a new "subject-stopped-1" barrier;
TellWatchersWeDied notifies remote watchers before local ones, so the local
Terminated proves the remote notification already left. first's wait now
opens after that barrier and uses an explicit 6s bound derived from
akka.remote.resend-interval (two resend cycles plus a hop), not the failure
detector (the peer stays alive and heartbeating here, so that path never
fires). Applied the same derived bound to the equivalent wait in phase 4,
which already had the correct barrier shape.

Restored the identify helper's dropped assertion (ActorIdentity.Subject must
not be null, naming the actor path) so a missing actor fails at the identify
instead of surfacing as an unrelated Ack timeout downstream.

Also migrated the file's remaining synchronous TestKit calls to their async
counterparts: the remote-watcher lookup, the identify helper, RunOn, and
ReceiveN, plus an explicit bound on AssertCleanup's previously-unbounded
nested expect.

* RemoteNodeDeathWatchSpec: name the real remote watcher in the comment, note Artery's resend interval, drop the AssertCleanup bound

The remote watcher recorded on the subject is first's /system/remote-watcher,
which re-issues the ProbeActor's watch over the wire; the comment named the
local watcher. The 6s bound reads the classic resend-interval; Artery resends
at 1s, so the same budget covers it. The 1s bound on AssertCleanup's nested
expect is removed: the predicate overload asserts rather than filters, so the
retry loop was already content-driven, and a timed-out attempt could offset
later requests from their replies.

(cherry picked from commit 1ad1abe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant