You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "He-Pin (via GitHub)" <gi...@apache.org> on 2026/10/24 15:16:55 UTC
[PR] Handle failed lease release on the Oldest transition in ClusterSingletonManager [pekko]
He-Pin opened a new pull request, #3699:
URL: https://github.com/apache/pekko/pull/3699
### Motivation
`ClusterSingletonManager` releases its lease from two places. The `AcquiringLease -> _` transition converts a failed `Future` into `ReleaseLeaseFailure`, but the `Oldest -> _` transition does not, so a failure there arrives as a `Status.Failure` that the FSM does not handle. When the lease backend is unavailable during a hand-over, no "failed to release lease" error is logged — exactly the situation where the diagnostic matters, because a lease that was never released can delay the next singleton activation on another node.
### Modification
- Mirror the `AcquiringLease` path in the `Oldest -> _` transition: `.recover { case t => ReleaseLeaseFailure(t) }`.
- `TestLease`: add `setNextReleaseResult` (defaults to `Future.successful(true)`, so existing behavior is unchanged).
- `ClusterSingletonLeaseSpec`: new case "log error when lease release fails leaving oldest" — triggers the transition via lease loss and asserts the `ReleaseLeaseFailure` handler logs the error.
### Result
A failed lease release on the oldest transition is handled and logged instead of escaping as an unhandled message. Observability fix; no behavior change on the success path.
### Tests
- `sbt "cluster-tools / Test / testOnly org.apache.pekko.cluster.singleton.ClusterSingletonLeaseSpec"`: 9/9 passed
- Directional check: reverted the fix, re-ran the new test only — failed as expected
- Full validation on CI
### References
Fixes #3685
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Handle failed lease release on the Oldest transition in ClusterSingletonManager [pekko]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #3699:
URL: https://github.com/apache/pekko/pull/3699#discussion_r4367733151
##########
cluster-tools/src/test/scala/org/apache/pekko/cluster/singleton/ClusterSingletonLeaseSpec.scala:
##########
@@ -217,6 +219,28 @@ class ClusterSingletonLeaseSpec extends PekkoSpec(ConfigFactory.parseString("""
}
}
+ "log error when lease release fails leaving oldest" in {
+ val lifecycleProbe = TestProbe()
+ val settings = nextSettings()
+ system.actorOf(
+ ClusterSingletonManager.props(Props(new ImportantSingleton(lifecycleProbe.ref)), PoisonPill, settings),
+ settings.singletonName)
+ val testLease = awaitAssert {
+ testLeaseExt.getTestLease(leaseNameFor(settings))
+ } // allow singleton manager to create the lease
+ testLease.probe.expectMsg(AcquireReq(leaseOwner))
+ testLease.initialPromise.complete(Success(true))
+ lifecycleProbe.expectMsg("preStart")
+ testLease.setNextReleaseResult(Future.failed(TestException("no release for you")))
Review Comment:
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.
##########
cluster-tools/src/main/scala/org/apache/pekko/cluster/singleton/ClusterSingletonManager.scala:
##########
@@ -1236,7 +1236,9 @@ class ClusterSingletonManager(singletonProps: Props, terminationMessage: Any, se
lease.foreach { l =>
logInfo("Releasing lease as leaving Oldest")
import context.dispatcher
- pipe(l.release().map(ReleaseLeaseResult(_))).to(self)
+ pipe(l.release().map[Any](ReleaseLeaseResult(_)).recover {
Review Comment:
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?
##########
cluster-tools/src/main/scala/org/apache/pekko/cluster/singleton/ClusterSingletonManager.scala:
##########
@@ -1236,7 +1236,9 @@ class ClusterSingletonManager(singletonProps: Props, terminationMessage: Any, se
lease.foreach { l =>
logInfo("Releasing lease as leaving Oldest")
import context.dispatcher
- pipe(l.release().map(ReleaseLeaseResult(_))).to(self)
+ pipe(l.release().map[Any](ReleaseLeaseResult(_)).recover {
Review Comment:
`.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.
##########
cluster-tools/src/main/scala/org/apache/pekko/cluster/singleton/ClusterSingletonManager.scala:
##########
@@ -1236,7 +1236,9 @@ class ClusterSingletonManager(singletonProps: Props, terminationMessage: Any, se
lease.foreach { l =>
logInfo("Releasing lease as leaving Oldest")
import context.dispatcher
- pipe(l.release().map(ReleaseLeaseResult(_))).to(self)
+ pipe(l.release().map[Any](ReleaseLeaseResult(_)).recover {
+ case t => ReleaseLeaseFailure(t)
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org