fix(siteruntime): stamp the ensure-started heal with the store-write generation + order the redeploy override reset ahead of the successor (arch-review 2026-08-19 P1-1 + P1-2)
P1-1 — the row-67 ensure-started self-heal re-read the durable row off-thread but captured no _storeWriteGeneration and never ran IsStoreSnapshotStale, unlike the boot load and the reconcile sweep. A Disable or Delete committing inside that window left the result carrying an enabled config that passed every guard (neither command sets any of the three pipeline markers ShouldAttemptEnsureStart re-checks), so the heal started an instance the operator had just stopped — PERMANENTLY, since the sweep iterates store rows only and has no runtime→store stop leg. The generation is now captured at dispatch, threaded on EnsureInstanceStartedResult and checked on landing, and the comment claiming the stored row already covered this is corrected. The heal reads through _configLoader, the same seam the boot load and sweep use (identical production call), so a test can hold the read open across a committed disable/delete. P1-2 — the "static overrides reset on redeployment" invariant was racy in both directions. ApplyDeployment created the successor Instance Actor first and only then dispatched the clear, queued behind a full config-JSON write, so the successor's PreStart override load routinely won and applied the PREDECESSOR's overrides — keys absent from the new config included — to in-memory state for the actor's lifetime while the disk ended cleared; a later restart then silently flipped the values. The clear is now dispatched BEFORE CreateInstanceActor and handed to the actor as a gate its override load chains behind; the persistence task joins the same task, so a failed clear still fails the deploy exactly as before. Leg 2: the predecessor's PostStop flush was fire-and-forget and could commit up to one coalescing window AFTER the clear (the Deployment Manager gates the clear on Terminated), durably resurrecting overrides — PostStop now waits for it, bounded at 5s with a Warning on overrun. Tests: new RedeployStaticOverrideResetTests (5) pins both legs plus the gate mechanism and first-deploy behaviour; three new cases in ReconcileStartsInstanceTests pin the discarded disable/delete heals and that an unraced heal still starts. InstanceActorTests' StaticOverride_ResetOnRedeployment comment claimed to simulate an ordering the production path did not have — corrected.
This commit is contained in:
+159
-2
@@ -90,7 +90,9 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable
|
||||
/// would no longer distinguish the two mechanisms.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private IActorRef CreateDeploymentManager(IServiceProvider? serviceProvider = null)
|
||||
private IActorRef CreateDeploymentManager(
|
||||
IServiceProvider? serviceProvider = null,
|
||||
Func<Task<List<DeployedInstance>>>? configLoader = null)
|
||||
{
|
||||
var health = new DeploymentManagerStartupProbe();
|
||||
var dm = ActorOf(Props.Create(() => new DeploymentManagerActor(
|
||||
@@ -104,7 +106,10 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable
|
||||
healthCollector: health,
|
||||
serviceProvider: serviceProvider,
|
||||
loggerFactory: null,
|
||||
configFetcher: null)));
|
||||
configFetcher: null,
|
||||
// In position: an expression tree rejects a named argument that skips a parameter.
|
||||
startupLoadRetryInterval: null,
|
||||
configLoader: configLoader)));
|
||||
|
||||
AwaitCondition(() => health.ReportCount > 0, TimeSpan.FromSeconds(20),
|
||||
TimeSpan.FromMilliseconds(25));
|
||||
@@ -382,6 +387,120 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable
|
||||
ExpectMsg<DeploymentStatusResponse>(TimeSpan.FromSeconds(5)).Status);
|
||||
}
|
||||
|
||||
// ── Row 101 applied to the heal: a result that outlived its snapshot ──
|
||||
|
||||
/// <summary>
|
||||
/// The heal's storage read is asynchronous, so a lifecycle command can commit while it is
|
||||
/// in flight — and the READ ITSELF cannot notice, because it already saw the enabled row and
|
||||
/// neither disable nor delete leaves any of the three pipeline markers
|
||||
/// <c>ShouldAttemptEnsureStart</c> re-checks. Starting from that superseded snapshot is
|
||||
/// PERMANENT: the instance reconcile sweep walks store rows only, so it has no
|
||||
/// runtime→store stop leg that would ever take the ghost instance back down.
|
||||
///
|
||||
/// <para>
|
||||
/// This test holds the read open, commits a disable behind it, then releases it — the
|
||||
/// interleaving the fix's store-write-generation stamp exists to reject (arch-review
|
||||
/// 2026-08-19 P1-1). <b>Fails before the fix</b>: the stale enabled snapshot passes every
|
||||
/// guard and the Instance Actor is created for a disabled instance.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task EnsureInstanceStarted_ResultLandingAfterADisable_IsDiscarded()
|
||||
{
|
||||
const string instanceName = "RacedDisablePump";
|
||||
var loader = new GatedConfigLoader(_storage);
|
||||
var dm = CreateDeploymentManager(configLoader: loader.LoadAsync);
|
||||
|
||||
await _storage.StoreDeployedConfigAsync(
|
||||
instanceName, MakeConfigJson(instanceName), "dep-rd", "rev-rd", true);
|
||||
|
||||
// The heal reads the (still enabled) row and parks on the gate.
|
||||
loader.Arm();
|
||||
dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile"));
|
||||
await loader.WaitUntilParkedAsync();
|
||||
|
||||
// The operator disables the instance; the write commits and the manager processes both
|
||||
// halves of the generation bump while the heal's snapshot is still in flight.
|
||||
dm.Tell(new DisableInstanceCommand("cmd-disable", instanceName, DateTimeOffset.UtcNow));
|
||||
ExpectMsg<InstanceLifecycleResponse>(TimeSpan.FromSeconds(5));
|
||||
await AwaitAssertAsync(async () =>
|
||||
{
|
||||
var configs = await _storage.GetAllDeployedConfigsAsync();
|
||||
Assert.False(configs.Single(c => c.InstanceUniqueName == instanceName).IsEnabled);
|
||||
}, TimeSpan.FromSeconds(5));
|
||||
|
||||
// Now let the superseded result land.
|
||||
loader.Release();
|
||||
await Task.Delay(1500);
|
||||
|
||||
Assert.False(
|
||||
await InstanceIsRunningAsync(dm, instanceName),
|
||||
"The heal started an instance the operator had just disabled — and nothing would "
|
||||
+ "ever stop it again (arch-review 2026-08-19 P1-1)");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The delete half of the same race. Worse than the disable case: the durable row is GONE,
|
||||
/// so the resurrected instance has no config row at all — the sweep cannot even see it to
|
||||
/// reason about, and the ghost survives until the node restarts.
|
||||
///
|
||||
/// <para><b>Fails before the fix</b> at the final assert.</para>
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task EnsureInstanceStarted_ResultLandingAfterADelete_IsDiscarded()
|
||||
{
|
||||
const string instanceName = "RacedDeletePump";
|
||||
var loader = new GatedConfigLoader(_storage);
|
||||
var dm = CreateDeploymentManager(configLoader: loader.LoadAsync);
|
||||
|
||||
await _storage.StoreDeployedConfigAsync(
|
||||
instanceName, MakeConfigJson(instanceName), "dep-rx", "rev-rx", true);
|
||||
|
||||
loader.Arm();
|
||||
dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile"));
|
||||
await loader.WaitUntilParkedAsync();
|
||||
|
||||
dm.Tell(new DeleteInstanceCommand("cmd-delete", instanceName, DateTimeOffset.UtcNow));
|
||||
ExpectMsg<InstanceLifecycleResponse>(TimeSpan.FromSeconds(5));
|
||||
await AwaitAssertAsync(async () =>
|
||||
{
|
||||
var configs = await _storage.GetAllDeployedConfigsAsync();
|
||||
Assert.DoesNotContain(configs, c => c.InstanceUniqueName == instanceName);
|
||||
}, TimeSpan.FromSeconds(5));
|
||||
|
||||
loader.Release();
|
||||
await Task.Delay(1500);
|
||||
|
||||
Assert.False(
|
||||
await InstanceIsRunningAsync(dm, instanceName),
|
||||
"The heal resurrected a deleted instance from a snapshot read before the delete "
|
||||
+ "(arch-review 2026-08-19 P1-1)");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The complement of the two tests above: with no mutation in the read's window the heal
|
||||
/// still starts the instance, so the generation check rejects superseded snapshots without
|
||||
/// disabling the row-67 repair itself.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task EnsureInstanceStarted_ResultLandingWithNoInterveningWrite_StillStarts()
|
||||
{
|
||||
const string instanceName = "UnracedPump";
|
||||
var loader = new GatedConfigLoader(_storage);
|
||||
var dm = CreateDeploymentManager(configLoader: loader.LoadAsync);
|
||||
|
||||
await _storage.StoreDeployedConfigAsync(
|
||||
instanceName, MakeConfigJson(instanceName), "dep-ok", "rev-ok", true);
|
||||
|
||||
loader.Arm();
|
||||
dm.Tell(new EnsureInstanceStartedCommand(instanceName, "startup reconcile"));
|
||||
await loader.WaitUntilParkedAsync();
|
||||
loader.Release();
|
||||
|
||||
await AwaitAssertAsync(async () =>
|
||||
Assert.True(await InstanceIsRunningAsync(dm, instanceName)), TimeSpan.FromSeconds(10));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Resolves the Instance Actor child of the Deployment Manager by path — the direct
|
||||
/// "the InstanceActor exists" check, complementing the debug-snapshot probe.
|
||||
@@ -390,6 +509,44 @@ public class ReconcileStartsInstanceTests : TestKit, IDisposable
|
||||
await Sys.ActorSelection(deploymentManager.Path / instanceName)
|
||||
.ResolveOne(TimeSpan.FromSeconds(5));
|
||||
|
||||
/// <summary>
|
||||
/// A deployed-config loader that can be held open mid-read, so a test can commit a
|
||||
/// lifecycle write while a heal's snapshot is provably in flight rather than hoping the
|
||||
/// two race the right way round.
|
||||
///
|
||||
/// <para>
|
||||
/// Reads real storage every time and only parks AFTER the read, so the returned snapshot is
|
||||
/// exactly what production would have seen at that instant. Inert until <see cref="Arm"/>
|
||||
/// is called, which keeps the Deployment Manager's boot load (which runs through this same
|
||||
/// seam) unblocked.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private sealed class GatedConfigLoader(SiteStorageService storage)
|
||||
{
|
||||
private TaskCompletionSource? _release;
|
||||
private readonly TaskCompletionSource _parked =
|
||||
new(TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
|
||||
public void Arm() => _release = new TaskCompletionSource(
|
||||
TaskCreationOptions.RunContinuationsAsynchronously);
|
||||
|
||||
public void Release() => _release!.TrySetResult();
|
||||
|
||||
public Task WaitUntilParkedAsync() => _parked.Task.WaitAsync(TimeSpan.FromSeconds(10));
|
||||
|
||||
public async Task<List<DeployedInstance>> LoadAsync()
|
||||
{
|
||||
var configs = await storage.GetAllDeployedConfigsAsync();
|
||||
var gate = _release;
|
||||
if (gate != null)
|
||||
{
|
||||
_parked.TrySetResult();
|
||||
await gate.Task;
|
||||
}
|
||||
return configs;
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// In-test fake <see cref="IDeploymentConfigFetcher"/>: runs a per-deploymentId behavior and
|
||||
/// records every call thread-safely (the fetch runs on a pool thread).
|
||||
|
||||
Reference in New Issue
Block a user