Pool routers use Directive.Escalate by default when supervising routees - this causes unintended consequences

[Aaronontheweb]

Version Information
Version of Akka.NET? 1.4.46 and earlier, 1.5.0-alpha3 and earlier.
Which Akka.NET Modules? Akka

Describe the bug

Currently, the default SupervisionStrategy on all Pool routers uses Directive.Escalate, which causes the router to restart and kill all routees every time any of the routees restart. This can be problematic, as it means that all routees are restarted even if only one of them has failed.

Instead, I suggest changing the default SupervisionStrategy to return a Directive.Restart instead. This would cause only the failed routee to be restarted, rather than restarting all routees in the pool. This would allow for more granular control over how failures are handled, and can prevent unnecessary restarts of healthy routees.

To Reproduce

Reproduced with one of my unit tests on #6294

// unit test to assert that a pool router actor's children generate telemetry events, but not the router actor itself
[Fact]
public async Task ActorTelemetry_must_be_accurate_for_pool_router()
{
    // create a TelemetrySubscriber actor
    var subscriber = Sys.ActorOf(Props.Create<TelemetrySubscriber>(), "subscriber");
    
    // request current telemetry values (ensure that the actor has started, so counter values will be accurate)
    await subscriber.Ask<TelemetrySubscriber.GetTelemetry>(TelemetrySubscriber.GetTelemetryRequest.Instance);
    
    // create a pool router
    var router = Sys.ActorOf(Props.Create<ChildActor>().WithRouter(new RoundRobinPool(10)), "router");

    // awaitassert collecting data from the telemetry subscriber until we can see that 10 actors have been created
    await AwaitAssertAsync(async () =>
    {
        var telemetry = await subscriber.Ask<TelemetrySubscriber.GetTelemetry>(TelemetrySubscriber.GetTelemetryRequest.Instance);
        Assert.Equal(12, telemetry.ActorCreated);
        // assert no restarts or stops recorded
        Assert.Equal(0, telemetry.ActorRestarted);
        Assert.Equal(0, telemetry.ActorStopped);
    }, RemainingOrDefault);
    
    // send a message to the router to restart all children
    router.Tell(new Broadcast(RestartChildren.Instance));

    // await assert collecting data from the telemetry subscriber until we can see that 10 actors have been restarted
    await AwaitAssertAsync(async () =>
    {
        var telemetry = await subscriber.Ask<TelemetrySubscriber.GetTelemetry>(TelemetrySubscriber.GetTelemetryRequest.Instance);
        // assert that actor start count is still 10
        Assert.Equal(12, telemetry.ActorCreated);
        Assert.Equal(10, telemetry.ActorRestarted);
        // assert no stops recorded
        Assert.Equal(0, telemetry.ActorStopped);
    }, RemainingOrDefault);
    
    // GracefulStop router actor and assert that 10 actors have been stopped
    await router.GracefulStop(RemainingOrDefault);
    await AwaitAssertAsync(async () =>
    {
        var telemetry = await subscriber.Ask<TelemetrySubscriber.GetTelemetry>(TelemetrySubscriber.GetTelemetryRequest.Instance);
        // assert that actor start count is still 10
        Assert.Equal(12, telemetry.ActorCreated);
        Assert.Equal(10, telemetry.ActorRestarted);
        Assert.Equal(10, telemetry.ActorStopped);
    }, RemainingOrDefault);
}

Expected behavior

Each child should be restarted individually and independently from each other.

Actual behavior

Assert.Equal(10, telemetry.ActorRestarted); should be true, but instead the telemetry.ActorRestarted value is 110!

That's because the following line:

// send a message to the router to restart all children
  router.Tell(new Broadcast(RestartChildren.Instance));

Causes the router to restart all of its children 10 times apiece.

[to11mtm]

Can we add this as an arg to the routers if it is not there already?

TBH never realized this was a thing and explains a lot, a fix would be good but we should be careful to respect existing code that guards condition

[Aaronontheweb]

This can be configured via Router.WithSupervisionStrategy - but most users never specify that.

[Aaronontheweb]

FWIW I still think we should change the default here, even though that's technically a breaking behavioral change - but I expect that the impact of the change will be more positive than not.

[Aaronontheweb]

We're just going to reverse this in 1.4.47. I have no doubt it does more harm than good in most cases.

[Aaronontheweb]

Actually, we'll push this out to 1.4.48