[He-Pin]

Motivation

Lease release failures can disappear when the oldest singleton manager hands over or stops. Release results sent to its mailbox can arrive after termination, and a synchronous exception from a lease implementation can interrupt the transition. A direct stop from Oldest also skips the transition hook entirely, leaving the lease held until expiry.

Modification

• Share one release helper between the existing transition paths and the final-stop path. Capture synchronous non-fatal exceptions and report unsuccessful results directly through the manager's existing thread-safe logger, preserving its logger identity.
• Release on final stop from Oldest, after the singleton child has terminated. Exclude the default preRestart call to postStop, retaining the existing lease ownership and reacquisition behavior across a restart.
• Cover failed futures, synchronous exceptions, late failures and false results, actual direct stops, exceptional restarts, and duplicate-release prevention. Make injected release results one-shot and explicitly clean up failed-release managers.
• Use an available remoting port so concurrent lease tests do not collide on the default port. Document the release diagnostics and lifecycle behavior.
• Retain the existing internal result message classes and handlers to avoid incidental binary changes; new release completions log directly.

Result

Release failures remain observable after manager termination, and synchronous non-fatal release exceptions do not abort handover. Direct stops release the lease once after child termination, while a normal restart introduces no extra release.

Waiting for child termination avoids releasing while the singleton is still running; a blocked child shutdown can still leave failover dependent on lease expiry. Release failures during node shutdown are reported by the same error path. Callbacks and diagnostics after the entire actor system has shut down remain best effort.

Tests

• JDK 17: sbt "cluster-tools / Test / testOnly org.apache.pekko.cluster.singleton.ClusterSingletonLeaseSpec" — 14/14 passed.
• Directional diagnostics check: substitute origin/main's manager source using temporary Compile / unmanagedSources, then run the spec with -- -z "after the manager has stopped" — both tests failed as expected.
• Directional direct-stop check: substitute the manager implementation before the final-stop hook, then run -- -z "stopped directly as oldest" — failed waiting for ReleaseReq, as expected.
• Restart-guard mutation check: remove only the guard from a temporary manager source and run -- -z "retain the lease across an exceptional restart" — failed with expected AcquireReq, found ReleaseReq, as expected.
• Broader singleton validation: sbt "cluster-tools / Test / testOnly org.apache.pekko.cluster.singleton.*Spec" "cluster-typed / Test / testOnly org.apache.pekko.cluster.typed.ClusterSingleton*Spec" — classic 20/20 and typed 2/2 passed.
• sbt "+cluster-tools / mimaReportBinaryIssues" — passed on Scala 2.13.18 and 3.3.8 with the final-stop change.
• sbt "cluster-tools / headerCreateAll" "coordination / headerCreateAll" "cluster-tools / headerCheckAll" "coordination / headerCheckAll" — passed on the final implementation.
• scalafmt --mode diff-ref=origin/main, scalafmt --list --mode diff-ref=origin/main, and git diff --check — passed.
• The initial focused run aborted before executing tests because port 17355 was occupied. The available-port configuration resolved that conflict.
• Shared integration validation with the other reviewed PRs, on JDK 17: sbt headerCreateAll +mimaReportBinaryIssues +headerCheckAll checkCodeStyle docs/paradox — passed. Cross-version checks passed on Scala 2.13.18 and 3.3.8.
• Repository-wide sbt sortImports could not complete because of an existing NoSuchMethodError: scala.meta.package$.Input() in unrelated multi-node-testkit/stream-tests Scalafix tasks. Unrelated generated import changes were discarded; the formatting and style checks above passed.
• Shared integration sbt -Dpekko.test.tags.exclude=performance,timing,long-running -Dpekko.test.multi-in-test=false validatePullRequest — completed, but did not pass on macOS/arm64. LevelDB JNI lacks a compatible arm64 library (four persistence test forks failed); NettyTransportSpec could not bind the unavailable local address 127.0.1.1; two existing TCP readiness tests timed out. Docker-backed DNS tests were skipped because Docker was unavailable.
• The two TCP failures passed when rerun in isolation against the final compiled integration code, without changing assertions or timeouts. The main-production baseline also exhibited a neighboring readiness timeout; those two exact cases did not fail in the repeated baseline runs. The full-run failures are retained in the validation record.

References

Fixes #3685

[pjfanning]

The fix looks right for the common case, and the new test is directional. A few points inline, plus one that sits outside the diff:

Lease never released when stop() is called directly from Oldest (ClusterSingletonManager.scala:982). When the node is self-downed while Oldest with singleton == None, or MemberRemoved(self) reaches whenUnhandled and calls stop(), the FSM stops without running onTransition. So Oldest -> _ never fires, release() is never called, and another node can't start the singleton until the lease times out. This predates this PR, but it is the same hook. Releasing in onTermination/postStop when the lease is still held would cover these paths.

[pjfanning]

The release result still goes to self, so it can be lost on the self-downed path. Oldest -> Stopping fires this release; if the singleton terminates quickly, Stopping calls stop() before the lease backend's future fails. ReleaseLeaseFailure then goes to dead letters, and because it extends DeadLetterSuppression nothing is logged. That is the downing/hand-over case where this diagnostic matters most. Could the failure be logged directly in the future's onComplete, using a thread-safe Logging(context.system, ...) adapter, rather than routed through the actor?

[pjfanning]

.recover only handles a failed Future. A Lease implementation whose release() throws synchronously (e.g. its client is not initialised) still throws inside onTransition and restarts the manager mid-hand-over, without this log line. Something like Future.fromTry(Try(l.release())).flatten would route that case to ReleaseLeaseFailure too.

[pjfanning]

This release/map/recover/pipe chain is now duplicated word for word in the AcquiringLease transition just above (lines 1227-1229). The two copies drifting apart is what caused this bug, so a small releaseLease() helper next to tryAcquireLease would keep them in sync, and the points in the other comments would then only need fixing once.

Also, tryAcquireLease (line 848) uses case NonFatal(t) => while both release paths use a catch-all case t =>. It would be good to use NonFatal consistently.

[pjfanning]

This leaves the TestLease set to fail every subsequent release(), and the manager keeps running: after the singleton terminates it re-acquires with the already-completed true result and goes back to Oldest. Any later release, e.g. during spec shutdown, would then log an unexpected TestException error outside the EventFilter. Resetting it with testLease.setNextReleaseResult(Future.successful(true)), or stopping the manager at the end of the test, would keep it isolated.