Skip to content

ClusterClient: arm the reconnect deadline per establishing phase via IWithTimers - #8510

Merged
Aaronontheweb merged 2 commits into
akkadotnet:devfrom
Aaronontheweb:fix/8508-clusterclient-reconnect-timer
Sep 5, 2026
Merged

Aaronontheweb merged 2 commits into
akkadotnet:devfrom
Aaronontheweb:fix/8508-clusterclient-reconnect-timer

Conversation

@Aaronontheweb

Copy link
Copy Markdown
Member

Fixes #8508.

Problem

ClusterClient armed a one-shot ReconnectTimeout on every message processed while establishing — the arming code sat in the Establishing receive delegate, which runs per message, and only the ActorIdentity success branch cancelled (and only its own timer). Every other message leaked one. Strays landing in Active dead-lettered harmlessly; a stray landing after re-entry into Establishing hit Context.Stop(Self) and killed a healthy client mid-reconnect. Masked in production because reconnect-timeout defaults to off; both ClusterClientHandoverSpec CI failures (builds 130927/130934) show the exact arithmetic — the fatal timer armed by the message before the previous successful connect, firing ~600ms into the new establishing phase.

Fix — IWithTimers, per review direction

Raw-scheduler cancelables are replaced by keyed timers (matching ClusterClientDiscovery, which already uses them):

  • The reconnect deadline is armed once per entry into the establishing phase (PreStart, Reestablish()) under a fixed key and cancelled on the transition to Active. A fixed key means re-arming replaces rather than accumulates; timers auto-cancel with the actor; and the timer wrapper discards messages from cancelled/replaced timers even if already in the mailbox — closing the race that a plain Cancel() cannot (it can't recall a delivered message).
  • Heartbeat and refresh-contacts moved to the same keyed discipline: one timer model for the actor, no manual PostStop cleanup left.
  • reconnect-timeout = off (the default) arms nothing.

Fail-first proof

Both new tests fail on the unfixed actor:

  1. Deterministic kill: client Active for 7s, receptionist stopped — the leaked timer (armed 10s earlier, in the previous phase) kills the client 2.997s into a fresh 10-second window. Same subtraction as both CI failures.
  2. Zero strays: an establish → active cycle on unfixed code publishes four unhandled ReconnectTimeouts; fixed, none.

Measured effect at the leak level

Same 10-round artery ClusterClientHandoverSpec soak under CPU load, counting stray ReconnectTimeout dead letters on the client node:

before: 5,6,6,5,6,0,6,6,6,6  = 52
after:  0,0,0,0,0,0,0,0,0,0  =  0

Handover itself unchanged (connects=2 every run, both before and after). The soak passes either way on a fast box — the kill needs a slow agent — so the deterministic tests carry the proof and the soak carries no-regression plus the leak observable.

API and behavior notes (flagged, not buried)

  • Approval delta on the sealed class: +IWithTimers, +Timers { get; set; }, +PreStart override, −PostStop override (it existed only to cancel the hand-managed timers; on a sealed class it was unreachable from consumer code). Recorded in BREAKING_CHANGES_V1.6.md.
  • Secondary behavior change, documented in the same entry: a refresh-contacts tick queued under an old cadence is discarded when the interval changes on a state transition, instead of being delivered against the new one — the same stale-timer class as the bug itself.

Verification

Check Result
New ClusterClientReconnectTimeoutSpec 2/2, both fail-first-proven
Full Akka.Cluster.Tools.Tests 100/100
Akka.API.Tests 18/18 after the flagged approval update
ClusterClientHandoverSpec artery ×10 under load / classic ×4 14/14, zero strays
-warnaserror on both touched projects 0 warnings

With this fix, ClusterClientHandoverSpec comes off the flake ledger as resolved — it was a true positive all along.

…lishing phase, not per message

ClusterClient.Establishing is used directly as the Receive delegate, so its prologue ran
on every message rather than once on state entry. Each message armed another one-shot
ReconnectTimeout timer and only the ActorIdentity-success branch cancelled - and it
cancelled only the timer armed in that same invocation. Every other message handled while
establishing (a Contacts reply, a buffered send, a refresh tick) left a live timer behind.

Strays that fire while the client is Active are unhandled and merely noisy. A stray that
fires after the client has gone back to establishing hits
"case ReconnectTimeout: Context.Stop(Self)" and kills a healthy client mid-reconnect, long
before its own deadline is due. Two CI runs of ClusterClientHandoverSpec show it: the
client was stopped 622ms and 548ms into a 3s window, and in both cases
(stop time - reconnect-timeout) lands on the previous phase's connect moment. Masked in
production only because akka.cluster.client.reconnect-timeout defaults to off. Defect
dates to the original 2015 port.

The deadline measures time spent in the CURRENT establishing phase, so it is now armed
once per entry into that phase - PreStart and Reestablish - and cancelled on the
transition to Active.

Uses IWithTimers rather than a field-held ICancelable, matching the sibling
ClusterClientDiscovery actor. A fixed key means starting the timer replaces any live one,
so deadlines cannot accumulate; timers are cancelled automatically on stop and restart, so
there is no PostStop cleanup to forget; and the scheduler discards messages from a
cancelled or replaced timer even when they are already in the mailbox. That last property
is why a key beats a cancelable here - Cancel() cannot recall a message that has already
been queued, so it leaves the very race this bug is made of. The heartbeat and
refresh-contacts timers move to the same discipline so the actor has one timer model.

reconnect-timeout = off remains a true no-op: no timer is armed at all.

Tests, both failing first against the unfixed actor:

  - Should_Survive_Stale_Reconnect_Deadline_When_Reestablishing drives the kill
    deterministically. With a 10s deadline the client sits Active for 7s, then the
    receptionist is stopped and nothing replaces it. Unfixed, a leftover phase-1 deadline
    fires 2.997s into the new phase and the watch probe sees Terminated; fixed, the client
    is still establishing when the 5s window closes, 5s short of the deadline that actually
    applies.
  - Should_Not_Leak_ReconnectTimeout_Timers watches the event stream across a full
    establish -> active -> re-establish cycle. Unfixed, the first abandoned deadline is
    published at 10.011s and the log shows four unhandled ReconnectTimeouts; fixed, none.

ClusterClientHandoverSpec is untouched and serves as the end-to-end regression test. It
already passed locally before the fix - the race needs a slow agent - so its soak is
no-regression evidence, not proof; the deterministic proof is the two tests above. What
the soak does show directly is the leak itself disappearing: across 10 artery rounds under
CPU load the client node logged 52 stray ReconnectTimeouts before the fix and 0 after,
with the same 2 connects per run either way.

Verification: new tests 2/2; Akka.Cluster.Tools.Tests 100/100; Akka.API.Tests 18/18;
ClusterClientHandoverSpec 10/10 artery under load and 4/4 classic; -warnaserror clean.

API approval files updated for a three-line delta on a sealed class: the IWithTimers
interface and its Timers property are added, and the PostStop override is gone because
timers now clean themselves up. Recorded in BREAKING_CHANGES_V1.6.md.

@Aaronontheweb Aaronontheweb left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aaronontheweb
Aaronontheweb merged commit 855e8d8 into akkadotnet:dev Sep 5, 2026
15 checks passed
@Aaronontheweb
Aaronontheweb deleted the fix/8508-clusterclient-reconnect-timer branch September 5, 2026 00:30
Aaronontheweb added a commit that referenced this pull request Sep 5, 2026
…ase, not per message (#8510) (#8511)

ClusterClient.Establishing is used directly as the Receive delegate, so its prologue ran
on every message rather than once on state entry. Each message armed another one-shot
ReconnectTimeout timer and only the ActorIdentity-success branch cancelled - and it
cancelled only the timer armed in that same invocation. Every other message handled while
establishing (a Contacts reply, a buffered send, a refresh tick) left a live timer behind.

Strays that fire while the client is Active are unhandled and merely noisy. A stray that
fires after the client has gone back to establishing hits
"case ReconnectTimeout: Context.Stop(Self)" and kills a healthy client mid-reconnect, long
before its own deadline is due. Two CI runs of ClusterClientHandoverSpec show it: the
client was stopped 622ms and 548ms into a 3s window, and in both cases
(stop time - reconnect-timeout) lands on the previous phase's connect moment. Masked in
production only because akka.cluster.client.reconnect-timeout defaults to off. Defect
dates to the original 2015 port.

The deadline measures time spent in the CURRENT establishing phase, so it is now armed
once per entry into that phase - PreStart and Reestablish - and cancelled on the
transition to Active.

Uses IWithTimers rather than a field-held ICancelable, matching the sibling
ClusterClientDiscovery actor. A fixed key means starting the timer replaces any live one,
so deadlines cannot accumulate; timers are cancelled automatically on stop and restart, so
there is no PostStop cleanup to forget; and the scheduler discards messages from a
cancelled or replaced timer even when they are already in the mailbox. That last property
is why a key beats a cancelable here - Cancel() cannot recall a message that has already
been queued, so it leaves the very race this bug is made of. The heartbeat and
refresh-contacts timers move to the same discipline so the actor has one timer model.

reconnect-timeout = off remains a true no-op: no timer is armed at all.

Tests, both failing first against the unfixed actor:

  - Should_Survive_Stale_Reconnect_Deadline_When_Reestablishing drives the kill
    deterministically. With a 10s deadline the client sits Active for 7s, then the
    receptionist is stopped and nothing replaces it. Unfixed, a leftover phase-1 deadline
    fires 2.997s into the new phase and the watch probe sees Terminated; fixed, the client
    is still establishing when the 5s window closes, 5s short of the deadline that actually
    applies.
  - Should_Not_Leak_ReconnectTimeout_Timers watches the event stream across a full
    establish -> active -> re-establish cycle. Unfixed, the first abandoned deadline is
    published at 10.011s and the log shows four unhandled ReconnectTimeouts; fixed, none.

ClusterClientHandoverSpec is untouched and serves as the end-to-end regression test. It
already passed locally before the fix - the race needs a slow agent - so its soak is
no-regression evidence, not proof; the deterministic proof is the two tests above. What
the soak does show directly is the leak itself disappearing: across 10 artery rounds under
CPU load the client node logged 52 stray ReconnectTimeouts before the fix and 0 after,
with the same 2 connects per run either way.

Verification: new tests 2/2; Akka.Cluster.Tools.Tests 100/100; Akka.API.Tests 18/18;
ClusterClientHandoverSpec 10/10 artery under load and 4/4 classic; -warnaserror clean.

API approval files updated for a three-line delta on a sealed class: the IWithTimers
interface and its Timers property are added, and the PostStop override is gone because
timers now clean themselves up. Recorded in BREAKING_CHANGES_V1.6.md.

Backport note (v1.5)

Cherry-picked from dev. v1.5's ClusterClient.cs was byte-identical to dev's pre-fix file,
so the actor change applied with no conflict; the bug is present on v1.5 exactly as
reported.

Three deviations from the dev commit, all forced by the branch:

  - BREAKING_CHANGES_V1.6.md does not exist on v1.5, so that hunk was dropped. The v1.5
    PR description carries the notes instead. The sentence "Recorded in
    BREAKING_CHANGES_V1.6.md" above therefore applies to dev only.
  - Both ClusterTools approval files conflicted because v1.5 orders the approved members
    differently (Props before Receive). Resolved to v1.5's own ordering, applying only the
    semantic delta: -PostStop, +IWithTimers, +Timers, +PreStart.
  - v1.5 has no artery transport, so the ClusterClientHandoverSpec soak quoted above was
    re-run classic-only on this branch.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ClusterClient: ReconnectTimeout timer leaked per message during Establishing can kill a healthy client on reconnect

1 participant