Joseph Doherty
15ef1f32a8
I dismissed this finding from the code review as a false positive, reasoning that Akka's ActorBase caches Self in a field and that three passing anti-entropy tests traverse the path. Both premises were wrong. Self resolves through Context, which is [ThreadStatic], and throws NotSupportedException once a continuation resumes on a thread-pool thread. The tests passed because a local SQLite store usually completes await SYNCHRONOUSLY, so the continuation stayed on the mailbox thread and the context was still intact. Correctness therefore depended on store latency and thread-pool timing: green here, broken under a slower or contended store, with the only symptom a per-peer warning every announce interval while nodes silently stopped converging. The live-broadcast fast path masked it further — only the anti-entropy repair path was dead. Captures self on the actor thread and passes it in. Adds GenuinelyAsyncSecretStore to force the async path, a regression test that fails on the unfixed code (20s timeout) and passes in 3s after, and ActorContextAfterAwaitTests pinning the underlying Akka behaviour so the wrong assumption cannot be made again. Audited every remaining Self/Sender access in the actor. 170 pass offline / 184 with the live SQL suite / 1 skip / 0 warnings. Claude-Session: https://claude.ai/code/session_01BL2Vu1ESDQ9SCN4gVKkdts
90 lines
3.3 KiB
C#
90 lines
3.3 KiB
C#
using Akka.Actor;
|
|
using Akka.TestKit.Xunit2;
|
|
|
|
namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests;
|
|
|
|
/// <summary>
|
|
/// Pins the Akka behaviour that <see cref="SecretReplicationActor"/> depends on: after a genuine
|
|
/// thread hop, actor context is <b>gone</b>, so <c>Self</c> cannot be read.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// This is here because assuming otherwise produced a real bug. <c>Self</c> resolves through
|
|
/// <c>Context</c>, which is <c>[ThreadStatic]</c>; a continuation that resumes on a thread-pool
|
|
/// thread has none, and reading it throws <see cref="NotSupportedException"/>.
|
|
/// </para>
|
|
/// <para>
|
|
/// What made it dangerous is that it does <em>not</em> reliably throw. When the awaited operation
|
|
/// completes synchronously — which a local SQLite store usually does — the continuation stays on the
|
|
/// actor's mailbox thread and the context is still intact, so the illegal read succeeds. Correctness
|
|
/// then depends on store latency and thread-pool timing. The rule the production code follows is
|
|
/// therefore unconditional: capture <c>Self</c> (and <c>Sender</c>) into locals while still on the
|
|
/// actor thread, and never touch actor context after an <c>await</c>.
|
|
/// </para>
|
|
/// </remarks>
|
|
public sealed class ActorContextAfterAwaitTests : TestKit
|
|
{
|
|
private sealed record Probe;
|
|
|
|
private sealed record Outcome(string? SelfError, string? ContextError, string? CapturedSelfPath);
|
|
|
|
private sealed class ProbeActor : ReceiveActor
|
|
{
|
|
public ProbeActor()
|
|
{
|
|
ReceiveAsync<Probe>(async probe =>
|
|
{
|
|
IActorRef replyTo = Sender;
|
|
|
|
// The correct pattern: capture while the context is still current.
|
|
IActorRef capturedSelf = Self;
|
|
|
|
// Force a real hop off the mailbox thread.
|
|
await Task.Delay(50).ConfigureAwait(false);
|
|
await Task.Yield();
|
|
|
|
string? selfError = null;
|
|
string? contextError = null;
|
|
|
|
try
|
|
{
|
|
ActorPath path = Self.Path;
|
|
GC.KeepAlive(path);
|
|
}
|
|
catch (Exception ex)
|
|
{
|
|
selfError = ex.GetType().Name;
|
|
}
|
|
|
|
try
|
|
{
|
|
IActorRef fromContext = Context.Self;
|
|
GC.KeepAlive(fromContext);
|
|
}
|
|
catch (Exception ex)
|
|
{
|
|
contextError = ex.GetType().Name;
|
|
}
|
|
|
|
replyTo.Tell(new Outcome(selfError, contextError, capturedSelf.Path.ToString()));
|
|
});
|
|
}
|
|
}
|
|
|
|
[Fact]
|
|
public void Self_and_Context_are_both_unreadable_after_a_real_thread_hop()
|
|
{
|
|
IActorRef probe = Sys.ActorOf(Props.Create(() => new ProbeActor()), "context-probe");
|
|
probe.Tell(new Probe());
|
|
|
|
Outcome outcome = ExpectMsg<Outcome>(TimeSpan.FromSeconds(10));
|
|
|
|
// Both throw — Self is not a cached field, it goes through the ThreadStatic context.
|
|
Assert.Equal("NotSupportedException", outcome.SelfError);
|
|
Assert.Equal("NotSupportedException", outcome.ContextError);
|
|
|
|
// ...but a reference captured beforehand stays valid, which is what the actor relies on.
|
|
Assert.EndsWith("/user/context-probe", outcome.CapturedSelfPath, StringComparison.Ordinal);
|
|
}
|
|
}
|