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
85 lines
3.3 KiB
C#
85 lines
3.3 KiB
C#
using ZB.MOM.WW.Secrets.Abstractions;
|
|
|
|
namespace ZB.MOM.WW.Secrets.Replicator.AkkaDotNet.Tests.Fakes;
|
|
|
|
/// <summary>
|
|
/// Wraps a store so every operation genuinely yields the thread before completing.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// This exists to close a blind spot that let a real bug through. A local SQLite store is fast
|
|
/// enough that <c>await</c> on it frequently completes <b>synchronously</b>, which leaves the
|
|
/// caller on the actor's own mailbox thread — and therefore leaves the actor context (a
|
|
/// <c>[ThreadStatic]</c>) intact. Code that illegally touches actor context after an <c>await</c>
|
|
/// then works by accident, and the whole suite passes.
|
|
/// </para>
|
|
/// <para>
|
|
/// Under a slower store, a contended one, or simply a differently-timed thread pool, the same
|
|
/// <c>await</c> goes properly asynchronous, the continuation lands on a pool thread with no actor
|
|
/// context, and the code throws. That is the worst shape of bug: green in tests, broken in
|
|
/// production, timing-dependent either way. Forcing the async path makes it deterministic.
|
|
/// </para>
|
|
/// </remarks>
|
|
/// <param name="inner">The store to wrap.</param>
|
|
public sealed class GenuinelyAsyncSecretStore(ISecretStore inner) : ISecretStore
|
|
{
|
|
private static async Task YieldRealAsync()
|
|
{
|
|
// Task.Yield alone can still be scheduled inline by some schedulers; a real (if tiny) delay
|
|
// guarantees the continuation is posted to the thread pool.
|
|
await Task.Delay(5).ConfigureAwait(false);
|
|
await Task.Yield();
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task<StoredSecret?> GetAsync(SecretName name, CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
return await inner.GetAsync(name, ct).ConfigureAwait(false);
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task UpsertAsync(StoredSecret row, CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
await inner.UpsertAsync(row, ct).ConfigureAwait(false);
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task<bool> DeleteAsync(SecretName name, string? actor, CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
return await inner.DeleteAsync(name, actor, ct).ConfigureAwait(false);
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task<IReadOnlyList<SecretMetadata>> ListAsync(bool includeDeleted, CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
return await inner.ListAsync(includeDeleted, ct).ConfigureAwait(false);
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task<IReadOnlyList<SecretManifestEntry>> GetManifestAsync(CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
return await inner.GetManifestAsync(ct).ConfigureAwait(false);
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task ApplyReplicatedAsync(StoredSecret row, CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
await inner.ApplyReplicatedAsync(row, ct).ConfigureAwait(false);
|
|
}
|
|
|
|
/// <inheritdoc />
|
|
public async Task<bool> ApplyRewrapAsync(
|
|
StoredSecret rewrappedRow, byte[] expectedCurrentWrappedDek, CancellationToken ct)
|
|
{
|
|
await YieldRealAsync().ConfigureAwait(false);
|
|
return await inner.ApplyRewrapAsync(rewrappedRow, expectedCurrentWrappedDek, ct)
|
|
.ConfigureAwait(false);
|
|
}
|
|
}
|