diff --git a/docs/components/SiteRuntime.md b/docs/components/SiteRuntime.md index abc2fe6c..fa75c1f5 100644 --- a/docs/components/SiteRuntime.md +++ b/docs/components/SiteRuntime.md @@ -91,7 +91,7 @@ Central sends a `DeployInstanceCommand` carrying a JSON `FlattenedConfiguration` > **`CreateInstanceActor` is the single funnel for the connection push too (register row 85)**: the `EnsureDclConnections` call moved *into* `CreateInstanceActor` on 2026-08-17. It used to sit at each caller, and only two of the four callers made it — the boot-time batch and this deploy path. `StartStoredInstance` (row-67 ensure-started and the row-78 sweep) and `HandleEnableResult` did not, so a self-healed or newly-enabled instance came up with no connection actors and every tag subscribe was answered `Unknown connection: ` indefinitely. See the *Instance reconcile sweep* section and `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. -> **Which config source wins (register rows 89 + 90, 2026-08-17)**: `EnsureDclConnections` no longer trusts the flattened snapshot blindly. `data_connection_definitions` is read on the same off-thread task as the deployed configs, at boot and on every sweep tick, into an actor-thread-confined **arbitration overlay**: a connection name with a row there is configured from that row, and only a name with no row uses the instance's flattened `Connections` entry (which is the correct source on a deploy-only site, where that table is legitimately empty). The winner is resolved **before** the per-name hash is computed, which is what makes the two-source ping-pong — the reason a store-level sweep was rejected in §7.4 — unrepresentable rather than merely avoided. Removals additionally leave an in-memory tombstone so a centrally deleted connection is not resurrected by the next instance start. A monotonic write-generation guard (row 101) discards a snapshot whose off-thread read raced an artifact apply — its stale absences would otherwise tombstone and stop just-applied live connections that nothing re-creates until an instance redeploy. **The register's premise was wrong in two ways** (the standby's DCL is empty rather than stale, so the real defect is create-time on the next singleton incarnation and is not replication-specific; and a re-issued create never reconfigured a *live* connection actor at all). See `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. +> **Which config source wins (register rows 89 + 90, 2026-08-17)**: `EnsureDclConnections` no longer trusts the flattened snapshot blindly. `data_connection_definitions` is read on the same off-thread task as the deployed configs, at boot and on every sweep tick, into an actor-thread-confined **arbitration overlay**: a connection name with a row there is configured from that row, and only a name with no row uses the instance's flattened `Connections` entry (which is the correct source on a deploy-only site, where that table is legitimately empty). The winner is resolved **before** the per-name hash is computed, which is what makes the two-source ping-pong — the reason a store-level sweep was rejected in §7.4 — unrepresentable rather than merely avoided. Removals additionally leave an in-memory tombstone so a centrally deleted connection is not resurrected by the next instance start. A monotonic store-write-generation guard (row 101) discards — **whole** — any snapshot whose off-thread read raced one of this node's own mutations of *either* table, bumping the generation both when a write is dispatched and when its completion message lands so no in-flight read can look fresh. It covers both halves: a stale definitions half would tombstone and stop just-applied live connections that nothing re-creates until an instance redeploy, and a stale configs half would restart an instance **backward** onto the revision a deploy just replaced or resurrect one a disable/delete just stopped (the deploy-pipeline-ownership check does not reach either, since it only holds while a deploy is in flight). Writes that never pass through the actor — CDC replication, snapshot resync, a restored file, the startup reconcile fetch — deliberately do **not** bump, because discovering those is the sweep's whole purpose. A stale sweep result is dropped; a stale boot load is **re-read** instead, since startup must complete. **The register's premise was wrong in two ways** (the standby's DCL is empty rather than stale, so the real defect is create-time on the next singleton incarnation and is not replication-specific; and a re-issued create never reconfigured a *live* connection actor at all). See `docs/plans/2026-08-17-dcl-connection-arbitration-decision.md`. > **A changed config reaches the LIVE connection (register row 97)**: resolving the winner decides *what* to push; a third command was needed to make a push against an already-live connection mean anything, because `DataConnectionManagerActor.HandleCreateConnection` warns and returns for a name it already holds. `EnsureDclConnection` now has three outcomes off the one per-name hash slot — **unchanged → nothing**, **new → `CreateConnectionCommand`**, **changed → `ReconfigureConnectionCommand`** — and the connection actor applies the last of these in place by swapping its adapter through the failover machinery and re-issuing its subscriptions. Log line: `Reconfigured DCL connection {Connection} (protocol={Protocol}) in place` (Information), replacing the "will not take effect" Warning rows 89/90 added. See `DataConnectionLayer.md` → *Reconfiguring a live connection*. diff --git a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md index d3a2e9a3..23db553e 100644 --- a/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md +++ b/docs/plans/2026-08-17-dcl-connection-arbitration-decision.md @@ -412,3 +412,113 @@ Applies to both snapshot consumers (boot load and sweep). Deterministic reproduc test that the legitimate row-90 disappearance still fires on a FRESH empty snapshot: `StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections` (fails before the fix) and `FreshEmptySweepSnapshot_StillRemovesDisappearedConnections`. + +## 11. Postscript — the same race on the CONFIG half of the snapshot (register row 101 follow-on, 2026-08-18) + +§10 fixed the DEFINITIONS half of `ReadStoreSnapshotAsync`'s snapshot. The **instance-config** +half of the same snapshot has the identical TOCTOU, and it was left protected only by machinery +whose reach is time-limited. This section extends §10 rather than revising it; everything §10 +says about the definitions half still stands. + +### 11.1 The two hazards, both verified real against the code + +Both were reproduced by neutralising the freshness check and re-running the new regression tests +(see §11.5), so neither is theoretical. + +**Hazard 1 — backward restart (row-72 leg).** A read dispatched before a deploy persists its new +row can land after the deploy has completed *and* after the pipeline released the instance. The +snapshot then carries the revision the deploy replaced while `_runningRevisions` carries the new +one — the exact shape `TryRestartStaleInstance` repairs, in the wrong direction. It is not a +harmless restart: the repair passes **the snapshot's own `ConfigJson`** to `HandleDeploy`, so the +superseded config is literally re-deployed. The pre-existing guards do not reach it: +`InstanceIsOwnedByADeployPipeline` covers only `_deployWarms` / `_terminatingActorsByName` / +`_inFlightDeployments`, all of which are cleared when the deploy finishes; and +`TryClaimSweepAttempt` does bound restarts to one per (instance, revision), but the *old* revision +has never been claimed, so the claim is available and the instance simply stays reverted. +Observed fail-before site event: *"Instance BackwardRestartPump restarted by deployment-manager +sweep self-heal — running revision rev-2 is stale, applying stored revision rev-1"*. + +**Hazard 2 — resurrection (row-78 leg).** A read dispatched before a delete or a disable commits +still shows an enabled row while the actor is already stopped (both handlers stop the actor +synchronously and write off-thread). `StartStoredInstance` then starts an instance the operator +just removed. Delete is the worse of the two because it explicitly clears +`_sweepAttemptedRevisions` for the instance, handing the sweep a fresh repair claim. Fail-before: +the deleted instance was running again after the stale snapshot landed. + +**Hazard 3 — the boot load's other consumers.** `HandleStartupConfigsLoaded` also rebuilds +`_deployedInstanceNames` from the snapshot and hands its enabled configs to the staggered startup +batch. A deploy or delete processed while the boot read is in flight therefore skews the +deployed-instance count and can hand the batch a superseded config; `CreateInstanceActor`'s +already-exists guard blunts the second effect but not the first. + +### 11.2 One generation for both tables + +`_definitionsWriteGeneration` is renamed `_storeWriteGeneration` and now counts mutations of BOTH +snapshot tables; the message parameter `DefinitionsGenerationAtRead` becomes +`StoreGenerationAtRead` on both internal records (neither is a wire message). One counter is the +right shape because the snapshot is a single consistent read and is discarded whole either way — +a per-table counter would only buy the ability to apply half a snapshot that the other half's +mutation already invalidated. + +### 11.3 Bump at BOTH ends of every mutation + +§10 bumped once, in the handler that runs after the write. That is sufficient when the actor's +own state advances in that same handler, but the config mutations do not all work that way — +`ApplyDeployment` creates the Instance Actor and stamps the running revision *before* the +persist, and `HandleDisable`/`HandleDelete` stop the actor *before* the write. So the generation +is now bumped twice per mutation: once when the off-thread write is **dispatched**, once when its +**completion** message lands. The pair leaves no window — a read dispatched before the mutation is +discarded by the first bump, a read dispatched inside the write window is discarded by the second, +and a read dispatched after the second necessarily sees the committed row. Two paths had no +actor-thread completion message at all (disable/delete pipe their reply to the *caller*, and the +init-failure rollback pipes nothing), so a minimal internal `StoreWriteCompleted(Source)` message +exists purely to carry the completion bump for them. + +Bump sites: `ApplyDeployment` + `HandleDeployPersistenceResult`; `HandleEnable` + +`HandleEnableResult`; `HandleDisable` + `StoreWriteCompleted`; `HandleDelete` + +`StoreWriteCompleted`; `RollbackPersistedConfig` + `StoreWriteCompleted`; `HandleDeployArtifacts` ++ `HandleApplyArtifactDataConnectionsToDcl` (the §10 bump, kept). The deploy and enable completion +bumps are **unconditional on success** — a failed write may still have committed the row before a +later step threw, so "failed" is not "nothing changed". Cost of over-bumping is at most one +discarded sweep tick or one extra boot read. + +**Deliberately NOT bumped:** `SiteReconciliationActor`'s `StoreDeployedConfigIfNewerAsync` (a +different actor), LocalDb CDC replication applies, snapshot resyncs, and an operator restoring the +file. None passes through this actor, and discovering exactly those divergences is the sweep's +entire reason to exist (rows 72 + 78). Bumping for them — were it even reachable — would blind the +sweep to its own use case. + +### 11.4 Sweep drops, boot re-reads + +The sweep consumer discards a stale result **whole** (definitions half and configs half, no +overlay apply, no start, no restart) at Debug, because there is nothing to complete and the next +tick is already a fresh read. The boot consumer cannot do that — a node whose startup read is +dropped stays a silent zero-instance node — so it **re-dispatches `LoadDeployedConfigs()`**, which +captures the current generation. Convergence argument: staleness requires a fresh mutation inside +each attempt's window, which is externally driven and finite. The belt-and-braces cap is +`MaxConsecutiveStaleStartupLoads` = 5, after which the snapshot is applied anyway with a Warning +(a boot that never finishes is strictly worse than one starting from a slightly superseded view +the sweep converges on its next tick). A superseded boot snapshot arriving *after* +`_startupLoadCompleted` — the S5 `RetryStartupLoad` timer racing a success — is dropped rather +than re-read, so the retry timer and the re-dispatch cannot feed each other. + +### 11.5 Tests + +In `InstanceSweepSelfHealTests`, all hand-delivering crafted `InstanceSweepLoaded` / +`StartupConfigsLoaded` messages with an explicit generation: + +- `StaleSweepSnapshot_CarryingASupersededRevision_DoesNotRestartBackward` — fails before. +- `StaleSweepSnapshot_ShowingADeletedInstance_DoesNotResurrectIt` — fails before (full + deploy-then-delete shape, not the weaker never-started equivalent). +- `StaleBootSnapshot_IsReReadRatherThanApplied` — fails before (one read instead of two); a slow + first read plus a lifecycle command processed during it makes the race deterministic, and the + test also pins that a LATE superseded snapshot is dropped rather than looping. +- `FreshSweepSnapshot_CarryingANewerRevision_StillRestartsTheInstance` and + `FreshSweepSnapshot_ShowingAStoredEnabledInstance_StillStartsIt` — the guard twins, pinning that + legitimate row-72 and row-78 repairs survive. Both start the instance from the BOOT LOAD rather + than a deploy command, precisely so no mutation passes through the actor and generation `0` is + genuinely current. + +`FreshEmptySweepSnapshot_StillRemovesDisappearedConnections` (§10) still pins the definitions +half; its hardcoded current generation moves from `1` to `2` because one artifact apply now bumps +at both ends. diff --git a/docs/requirements/Component-SiteRuntime.md b/docs/requirements/Component-SiteRuntime.md index 2d0db4a6..c63e04a3 100644 --- a/docs/requirements/Component-SiteRuntime.md +++ b/docs/requirements/Component-SiteRuntime.md @@ -106,7 +106,7 @@ flowchart TD > > The **row-72 decision is to converge, not merely warn**: the stored row *is* what central successfully deployed, so an actor serving the config central believes was replaced is the actual fault. Reusing `HandleDeploy` means the compile gate, warm-then-gate ordering, redeploy-supersede, the static-override / native-alarm reset and all-or-nothing-per-instance are the production ones — and a stop still does **not** cancel an in-flight script run, exactly as for a central redeploy. > -> Guards, all shared with the row-67 handler: a **disabled** instance is never started; the sweep **defers to any deploy pipeline that owns the instance** (compile warm in flight, predecessor still terminating, deployment awaiting its persistence/init join) so it can never interleave with a real redeploy; it is **inert until boot-time Instance Actor creation has finished**, so a tick between staggered batches does not "heal" instances the boot load is about to create; it makes **one repair attempt per (instance, revision) pair**, which is what stops a periodic self-heal becoming a restart loop for an actor that dies during init or a stored config the compile gate rejects (a rejection leaves the running instance untouched — the gate returns before anything is stopped); and reads never overlap. A sweep that finds nothing is **completely silent**. A repair emits the same Warning `deployment` site-event pattern as row 67, naming the trigger (`deployment-manager sweep self-heal`) so it is distinguishable from a startup-reconcile heal, and a stale-revision restart names **both** revisions; a rejected restart adds an Error event. The synthetic redeploy replies to the singleton itself rather than to `NoSender`, because dead letters are a monitored site health metric. +> Guards, all shared with the row-67 handler: a **disabled** instance is never started; the sweep **defers to any deploy pipeline that owns the instance** (compile warm in flight, predecessor still terminating, deployment awaiting its persistence/init join) so it can never interleave with a real redeploy; it is **inert until boot-time Instance Actor creation has finished**, so a tick between staggered batches does not "heal" instances the boot load is about to create; it makes **one repair attempt per (instance, revision) pair**, which is what stops a periodic self-heal becoming a restart loop for an actor that dies during init or a stored config the compile gate rejects (a rejection leaves the running instance untouched — the gate returns before anything is stopped); and reads never overlap. One guard is **not** in that list because it needed adding later: deferring to an in-flight deploy pipeline says nothing about a snapshot that landed *after* the pipeline finished, which is the stale-snapshot race — a tick whose read raced this node's own deploy/enable/disable/delete is discarded whole by the store-write generation check (see **Stale-snapshot guard** below). A sweep that finds nothing is **completely silent**. A repair emits the same Warning `deployment` site-event pattern as row 67, naming the trigger (`deployment-manager sweep self-heal`) so it is distinguishable from a startup-reconcile heal, and a stale-revision restart names **both** revisions; a rejected restart adds an Error event. The synthetic redeploy replies to the singleton itself rather than to `NoSender`, because dead letters are a monitored site health metric. > **Starting an Instance Actor also creates its DCL connections — the ensure lives in the creation funnel, not at call sites (register row 85)**: a DCL connection actor is **never** created from the `data_connection_definitions` table — that stayed true when rows 89/90 gave the table a reader, because the overlay it feeds is an *arbiter*, not a creation source, and at the time of row 85 nothing read the table at all. A connection actor exists only as a side effect of starting an instance, built from the `Connections` map inside that instance's **own flattened config** via `EnsureDclConnections` — plus the separate system-wide artifact-deploy path. That ensure used to be each *caller's* responsibility, and only two of the four callers made it (the boot-time staggered batch and `ApplyDeployment`). The two that did not were **`StartStoredInstance`** — the path shared by the row-67 ensure-started handler and the row-78 sweep — and **`HandleEnableResult`**. Both therefore produced a running instance whose every tag subscribe was answered `Unknown connection: ` by the DCL manager, indefinitely, because nothing else on the node would ever create that connection actor. Proven live 2026-08-16: a replication-seeded node's instances were correctly started by the sweep and then failed every subscribe for 6+ minutes until a second pair restart let the boot load create the connections. **It was never replication-specific** — the boot-time load reads only *enabled* configs, so a node that boots with an instance disabled never creates its connections either, and enabling it afterwards hit the identical dead end on an ordinary unreplicated site. `EnsureDclConnections` now sits at the top of **`CreateInstanceActor`**, already the single Instance Actor creation funnel (it is where the running revision is stamped for the same reason), so all four paths get it and a fifth cannot forget. It is placed *before* the already-exists guard so the two paths that already called it keep their exact previous semantics, and it stays silent in steady state because `EnsureDclConnection` is hash-guarded on the last config pushed per connection name. Deliberately **not** done at the time: a sweep pass over `data_connection_definitions` — it would not have fixed row 85 (that table is empty on a site that has only ever received instance deploys), and re-hashing from it would fight the deploy path over the same per-name hash slot, recreating connection actors and dropping live subscriptions on a loop. See `docs/plans/2026-08-15-instance-sweep-self-heal-design.md` §7. That pass is now present, in a form that resolves the conflict rather than creating it — see **DCL connection arbitration** immediately below. @@ -118,7 +118,17 @@ flowchart TD > > **The ping-pong is killed by keying the hash on the winner.** The conflict §7.4 identified is a property of *where the hash is computed*, not of the sweep: every ensure path now resolves the winning configuration **before** hashing or pushing, so for any name only one configuration can ever occupy the hash slot regardless of which path calls. The flip is not guarded against — it is unrepresentable. `HandleApplyArtifactDataConnectionsToDcl` writes its rows into the overlay in the same pass it pushes them, closing the same flip arriving through the artifact-deploy door. > -> **Stale-snapshot guard (row 101).** The snapshot is read off the actor thread but compared against the overlay only when its piped message lands — so a read that starts before an artifact apply's store writes can land *after* `HandleApplyArtifactDataConnectionsToDcl` populated the overlay, and its stale data would "prove" the just-applied names disappeared: tombstoned, live connection actors stopped, and — because the removal evicts the hash slot the overlay's push is gated on — **nothing re-creates them until an instance redeploy**. Each snapshot read therefore captures a monotonic definitions-write generation on the actor thread at dispatch (`_definitionsWriteGeneration`, bumped by every artifact apply), and a result carrying a stale generation is discarded **whole** — additions included, since a stale row pushed at a live connection would reconfigure it backward and flip-flop with the next fresh read. Discarding is safe: the artifact apply already updated the overlay itself, and the next tick reads fresh. Surfaced as a test flake (a raced sweep removed `PlcA` where the test expected `PlcGone`); the deterministic reproduction is `DclConnectionArbitrationTests.StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections`. +> **Stale-snapshot guard — covers the WHOLE snapshot, both halves and both consumers (row 101).** `ReadStoreSnapshotAsync` reads `deployed_configurations` and `data_connection_definitions` off the actor thread but is consumed only when its piped message lands, so a read dispatched before a mutation can land after the actor's own state has already moved past it. Every read therefore captures a single monotonic **store-write generation** on the actor thread at dispatch (`_storeWriteGeneration`), and a snapshot carrying a stale generation is discarded **whole**. One counter covers both tables on purpose: the snapshot is one consistent read and is discarded whole anyway, so a per-table counter would only buy the chance to apply half a snapshot the other half's mutation already invalidated. +> +> Each half would do real damage. **Definitions half:** stale absences "prove" just-applied names disappeared — tombstoned, live connection actors stopped, and because the removal evicts the hash slot the overlay's push is gated on, **nothing re-creates them until an instance redeploy**; stale rows push superseded configs at live connections and flip-flop with the next fresh read. **Configs half, row-72 leg:** a snapshot carrying the revision a deploy just replaced makes the stale-revision repair run **backward**, re-applying the snapshot's own superseded config JSON through the redeploy path — and `InstanceIsOwnedByADeployPipeline` is no defence, because it only holds while a deploy is *in flight* and the stale snapshot can land after the pipeline released the instance. **Configs half, row-78 leg:** a snapshot read before an undeploy/delete or a disable committed still shows an enabled row while the actor is already stopped, so the start repair **resurrects a just-stopped instance** — and delete specifically clears `_sweepAttemptedRevisions`, so the once-per-revision claim is freshly available and does not stand in the way either. All three were reproduced by neutralising the guard and re-running the regression tests. +> +> **The generation is bumped twice per mutation — when the off-thread write is dispatched and again when its completion message lands on the actor thread.** The pair is what leaves no window: an initiation-only bump would let a read dispatched *between* the decision and the commit compare equal and be applied even though it predates the write, and a completion-only bump would miss a read dispatched before the handler ran. With both, a read dispatched before the mutation is discarded, a read dispatched during it is discarded, and a read dispatched after the completion bump necessarily sees the committed row. Bump sites are every actor-thread path that mutates either table: deploy persist (dispatch + `DeployPersistenceResult`), enable (dispatch + `EnableResult`), disable and delete and the init-failure rollback (dispatch + a `StoreWriteCompleted` self-message that exists purely to bump after the commit), and the artifact apply (dispatch + `ApplyArtifactDataConnectionsToDcl`). +> +> **Writes that must NOT bump**, and this is load-bearing: rows arriving by LocalDb CDC replication, by a snapshot resync, by an operator restoring the file, or by `SiteReconciliationActor`'s startup fetch never pass through this actor — and discovering exactly those is the sweep's entire reason to exist (rows 72 + 78). They are not stale relative to actor state; they are the state the actor has yet to learn about. +> +> **The two consumers differ.** A stale **sweep** result is dropped whole at Debug — no overlay apply, no start, no restart — because there is nothing to complete and the next tick is already a fresh read. A stale **boot** load cannot be dropped (a node whose startup read is discarded stays a silent zero-instance node), so it **re-dispatches `LoadDeployedConfigs()`**, capturing the current generation. Staleness requires a fresh mutation inside each attempt's window, so this converges in practice; as a livelock backstop the fifth consecutive superseded boot read is applied anyway with a Warning, on the grounds that a boot that never finishes is worse than one starting from a slightly superseded view that the sweep converges on its next tick. A superseded boot snapshot arriving *after* the boot has completed (the S5 retry timer racing a success) is dropped rather than re-read. +> +> Surfaced originally as a test flake (a raced sweep removed `PlcA` where the test expected `PlcGone`). Deterministic reproductions: `DclConnectionArbitrationTests.StaleSweepSnapshot_RacingAnArtifactApply_DoesNotRemoveJustAppliedConnections` for the definitions half, and in `InstanceSweepSelfHealTests` — `StaleSweepSnapshot_CarryingASupersededRevision_DoesNotRestartBackward`, `StaleSweepSnapshot_ShowingADeletedInstance_DoesNotResurrectIt`, `StaleBootSnapshot_IsReReadRatherThanApplied`, each paired with a `Fresh…` twin pinning that the legitimate row-72 restart and row-78 start still happen. > > **Removal (row 90).** Both removal paths share one helper and now record a **tombstone** for the incarnation: the artifact reconcile's `RemovedConnectionNames`, and the sweep noticing that a name it had adopted no longer has a row (a removal it can *prove* — the store owned it a tick ago and does not now). `EnsureDclConnections` skips a tombstoned name, so a removal survives the next instance start instead of being undone by it — a pre-existing bug, since the removal evicted the hash slot but left no memory of why. Tombstones are **in-memory only** by decision: a durable marker would be a second divergent authority, and flattened configs already resurrect a removed connection across a restart. A definition row reappearing clears the tombstone. > diff --git a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs index e4ca149e..6c633e4f 100644 --- a/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs +++ b/src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/DeploymentManagerActor.cs @@ -97,6 +97,19 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// stale re-running the load after success. /// private bool _startupLoadCompleted; + + /// + /// How many consecutive boot-time store reads may be re-dispatched for being superseded + /// mid-flight (row 101) before one is applied anyway. Purely a livelock backstop — see + /// . + /// + private const int MaxConsecutiveStaleStartupLoads = 5; + + /// + /// Consecutive boot-time store reads discarded as superseded. Reset the moment a fresh one + /// lands, so the cap only ever counts an unbroken run. + /// + private int _consecutiveStaleStartupLoads; private readonly Dictionary _instanceActors = new(); /// /// Tracks Instance Actors that are terminating as part of a redeployment, keyed by @@ -441,6 +454,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // Internal deploy persistence result Receive(HandleDeployPersistenceResult); + // Row 101: an off-thread mutation of a snapshot table has committed. The only work is + // the actor-thread generation bump — see _storeWriteGeneration. + Receive(msg => BumpStoreWriteGeneration(msg.Source)); + // Instance Actor init confirmation — the second half of the deploy-success // join (S6). Success is only reported once both this and persistence land. Receive(HandleInstanceActorInitialized); @@ -498,9 +515,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void LoadDeployedConfigs() { - // Row 101: capture on the actor thread BEFORE the off-thread read starts, so a landing - // handler can tell whether an artifact apply mutated the overlay while it was in flight. - var generationAtRead = _definitionsWriteGeneration; + // Row 101: capture on the actor thread BEFORE the off-thread read starts, so the landing + // handler can tell whether this actor mutated either snapshot table while it was in + // flight. + var generationAtRead = _storeWriteGeneration; ReadStoreSnapshotAsync().ContinueWith(t => { if (t.IsCompletedSuccessfully) @@ -601,6 +619,39 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers return; } + // Row 101, boot half: a snapshot invalidated while it was in flight cannot simply be + // DROPPED the way the sweep's is — the boot load has to complete or the node stays a + // silent zero-instance node — so it is re-read instead. The re-dispatch captures the + // current generation, so it can only be stale again if yet another mutation lands in + // ITS window; that requires a fresh deploy/enable/disable/delete per attempt, which is + // externally driven and finite in practice. The counter is the belt-and-braces cap + // against a pathological arrival rate: after MaxConsecutiveStaleStartupLoads attempts + // the snapshot is applied anyway with a Warning, because a boot that never finishes is + // strictly worse than one that starts from a slightly superseded view — the instance + // reconcile sweep re-reads the store every tick and converges it afterwards. + if (IsStoreSnapshotStale(msg.StoreGenerationAtRead, "boot load")) + { + // A completed boot load means this is a late duplicate (the S5 retry timer racing a + // success), not the boot the node is waiting on. Re-reading for it would loop + // pointlessly; the sweep owns convergence from here. + if (_startupLoadCompleted) + return; + + if (++_consecutiveStaleStartupLoads <= MaxConsecutiveStaleStartupLoads) + { + LoadDeployedConfigs(); + return; + } + + _logger.LogWarning( + "The boot-time deployed-config read has come back superseded {Attempts} times in " + + "a row — applying it anyway so startup completes; the instance reconcile sweep " + + "converges any difference on its next tick", + _consecutiveStaleStartupLoads); + } + + _consecutiveStaleStartupLoads = 0; + // Mark the startup load complete so a stale retry timer is ignored. _startupLoadCompleted = true; @@ -610,8 +661,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // on the peer node rebuilds the whole DCL from flattened snapshots, and without the // overlay in place first it would rebuild them STALE and only correct itself a sweep // tick later — a needless create-then-diverge on every failover. - ApplyConnectionDefinitionOverlayIfFresh( - msg.ConnectionDefinitions, msg.DefinitionsGenerationAtRead, "boot load"); + ApplyConnectionDefinitionOverlay(msg.ConnectionDefinitions); var enabledConfigs = msg.Configs.Where(c => c.IsEnabled).ToList(); _deployedInstanceNames.Clear(); @@ -1164,6 +1214,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers IsRedeploy = isRedeploy }; + // Row 101 (initiation half): a deployed_configurations write is about to start off the + // actor thread, so every store read currently in flight predates it. + BumpStoreWriteGeneration("deploy persist dispatch"); + // Persist to SQLite and clear static overrides asynchronously Task.Run(async () => { @@ -1211,6 +1265,12 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void HandleDeployPersistenceResult(DeployPersistenceResult result) { + // Row 101 (completion half): this message is created only after the store write has + // finished, so bumping here is the point that provably happens-after the commit. Bumped + // unconditionally — a FAILED write may still have committed the config row before a + // later step threw, so "failed" is not "nothing changed". + BumpStoreWriteGeneration("deploy persist result"); + // If the S6 Terminated fallback already failed this deployment (the Instance // Actor died during init), the in-flight entry is gone — swallow the now-stale // persistence result so the deployer is answered exactly once. @@ -1311,13 +1371,22 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void RollbackPersistedConfig(string instanceName) { - Task.Run(() => _storage.RemoveDeployedConfigAsync(instanceName)).ContinueWith(t => + // Row 101 (initiation half) — the row is about to disappear from under any in-flight read. + BumpStoreWriteGeneration("init-failure rollback dispatch"); + + var removal = Task.Run(() => _storage.RemoveDeployedConfigAsync(instanceName)); + removal.ContinueWith(t => { if (!t.IsCompletedSuccessfully) _logger.LogError(t.Exception?.GetBaseException(), "Failed to roll back the persisted config for {Instance} after an init failure", instanceName); }); + // Row 101 (completion half): this path has no other actor-thread completion message, so + // one exists purely to bump the generation after the DELETE has committed. Without it a + // sweep read dispatched between the two points above would still see the rolled-back row + // and start an instance whose deployment had just failed. + removal.ContinueWith(_ => new StoreWriteCompleted("init-failure rollback")).PipeTo(Self); } /// @@ -1361,7 +1430,19 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers UpdateInstanceCounts(); var sender = replyTo; - _storage.SetInstanceEnabledAsync(instanceName, false).ContinueWith(t => + + // Row 101 (initiation half): the actor has just stopped the Instance Actor while the + // durable row still reads enabled, which is exactly the shape the row-78 sweep repairs. + // Any store read in flight would therefore "prove" a divergence that does not exist and + // restart the instance the operator just disabled. + BumpStoreWriteGeneration("disable dispatch"); + + var disableWrite = _storage.SetInstanceEnabledAsync(instanceName, false); + // Row 101 (completion half) — see StoreWriteCompleted. The is_enabled UPDATE has landed + // by the time this message is created, closing the window between the bump above and the + // commit. + disableWrite.ContinueWith(_ => new StoreWriteCompleted("disable")).PipeTo(Self); + disableWrite.ContinueWith(t => { if (t.IsCompletedSuccessfully) { @@ -1399,6 +1480,9 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers var sender = replyTo; + // Row 101 (initiation half) — an is_enabled UPDATE is about to start off-thread. + BumpStoreWriteGeneration("enable dispatch"); + Task.Run(async () => { await _storage.SetInstanceEnabledAsync(instanceName, true); @@ -1418,6 +1502,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void HandleEnableResult(EnableResult result) { + // Row 101 (completion half): the is_enabled UPDATE has committed by the time this + // message exists. Unconditional for the same reason as the deploy result. + BumpStoreWriteGeneration("enable result"); + var instanceName = result.Command.InstanceUniqueName; if (result.Error != null || result.Config == null) @@ -1635,7 +1723,7 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers _instanceSweepInFlight = true; // Row 101: capture on the actor thread BEFORE the off-thread read starts. - var generationAtRead = _definitionsWriteGeneration; + var generationAtRead = _storeWriteGeneration; ReadStoreSnapshotAsync().ContinueWith(t => t.IsCompletedSuccessfully ? new InstanceSweepLoaded(t.Result.Configs, null, t.Result.Definitions, generationAtRead) : new InstanceSweepLoaded([], t.Exception?.GetBaseException().Message)) @@ -1662,14 +1750,23 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers return; } + // Row 101, sweep half: this node mutated one of the two tables while the read was in + // flight, so the snapshot predates state the actor has already advanced past. Discard it + // WHOLE — no overlay apply, no row-78 start, no row-72 restart. Every repair this sweep + // makes is derived from "the store says X, the runtime says Y", and a snapshot older + // than the runtime inverts that comparison: it would restart an instance BACKWARD onto + // the revision a deploy just replaced, or resurrect one a disable/delete just stopped. + // Nothing is lost by waiting — the next tick's read starts after the write. + if (IsStoreSnapshotStale(msg.StoreGenerationAtRead, "instance sweep")) + return; + // Rows 89 + 90: refresh the DCL arbitration overlay FIRST, so any instance this same // sweep pass goes on to start resolves its connections against the current definitions // rather than against the snapshot frozen into its flattened config. This is the // convergence path for a definition row that arrived by LocalDb CDC replication: no new // cross-node message exists (or is needed) — whichever node runs the singleton reads the // row out of its own file on the next tick. - ApplyConnectionDefinitionOverlayIfFresh( - msg.ConnectionDefinitions, msg.DefinitionsGenerationAtRead, "instance sweep"); + ApplyConnectionDefinitionOverlay(msg.ConnectionDefinitions); foreach (var config in msg.Configs) { @@ -1888,7 +1985,18 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers UpdateInstanceCounts(); var sender = replyTo; - _storage.RemoveDeployedConfigAsync(instanceName).ContinueWith(t => + + // Row 101 (initiation half): the actor has already forgotten the instance (actor stopped, + // _deployedInstanceNames/_runningRevisions/_sweepAttemptedRevisions cleared) while the + // durable row is still there — and clearing _sweepAttemptedRevisions specifically hands + // the sweep a fresh repair claim, so an in-flight read landing now would resurrect the + // instance the operator just deleted. + BumpStoreWriteGeneration("delete dispatch"); + + var deleteWrite = _storage.RemoveDeployedConfigAsync(instanceName); + // Row 101 (completion half) — see StoreWriteCompleted. + deleteWrite.ContinueWith(_ => new StoreWriteCompleted("delete")).PipeTo(Self); + deleteWrite.ContinueWith(t => { if (t.IsCompletedSuccessfully) { @@ -2228,45 +2336,96 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers private readonly HashSet _centrallyRemovedConnections = new(StringComparer.Ordinal); /// - /// Monotonic count of actor-thread definition mutations from artifact applies (deferred - /// register row 101). A store snapshot is read OFF the actor thread and compared against - /// the overlay only when its piped message lands — so a read that started before an - /// artifact apply's store writes can land AFTER populated the overlay, and its stale - /// data would then "prove" the just-applied names disappeared, tombstoning and stopping - /// live connections that nothing re-creates until an instance redeploy. Each snapshot read - /// captures this value on the actor thread at dispatch; a result carrying a stale - /// generation skips the overlay apply entirely (additions too — a stale row pushed via - /// would reconfigure a live connection BACKWARD and set - /// up a flip-flop with the next fresh read). The next tick reads fresh and converges. + /// Monotonic count of this actor's own mutations of the two tables + /// reads — deployed_configurations AND + /// data_connection_definitions (deferred register row 101, extended to the config + /// half). A store snapshot is read OFF the actor thread but consumed only when its piped + /// message lands, so a read dispatched before a mutation can land after the actor's own + /// state has already advanced past it. Every consumer therefore captures this value on the + /// actor thread at read dispatch and discards the WHOLE snapshot on a mismatch. + /// + /// + /// One counter for both tables, deliberately. The snapshot is one consistent read and + /// is discarded whole, so a per-table counter would buy nothing but the chance to apply half + /// a snapshot that the other half's mutation already invalidated. + /// + /// + /// + /// What a stale snapshot would actually do — each of these is a real defect the guard + /// closes, not a theoretical one: + /// + /// DEFINITIONS half: absences "prove" just-applied names disappeared, tombstoning and + /// STOPPING live connections that nothing re-creates until an instance redeploy; rows push + /// superseded configs onto live connections and flip-flop with the next fresh read. + /// CONFIGS half, row-72 leg: a snapshot carrying the revision a deploy has just + /// replaced makes "repair" the instance BACKWARD — + /// it redeploys the snapshot's own (old) config JSON through the redeploy path. + /// does not cover this: it only holds while + /// the deploy is in flight, and the stale snapshot can land after the pipeline released + /// ownership. + /// CONFIGS half, row-78 leg: a snapshot read before an undeploy/delete or a disable + /// committed still shows an enabled row while the actor is already stopped, so + /// RESURRECTS a just-stopped instance. + /// + /// + /// + /// + /// Bumped twice per mutation — at initiation and at completion (see + /// ), which is what makes the guard airtight rather + /// than merely usually-right. The store write runs off the actor thread, so a read + /// dispatched between "the actor decided to mutate" and "the write committed" would compare + /// EQUAL against an initiation-only bump and be applied even though it predates the write. + /// Bumping again on the completion message — which is created only after the write has + /// committed — makes any such in-window read compare stale. Consequently: a read dispatched + /// before the mutation is discarded, a read dispatched during it is discarded, and a read + /// dispatched after the completion bump necessarily sees the committed row. + /// + /// + /// + /// Writes that must NOT bump. Rows arriving by LocalDb CDC replication, by a snapshot + /// resync, by an operator restoring the file, or by SiteReconciliationActor's + /// startup fetch never pass through this actor at all — and discovering exactly those is the + /// instance reconcile sweep's whole reason to exist (register rows 72 + 78). They are not + /// "stale relative to actor state"; they are the state the actor has yet to learn about. + /// Bumping for them (were it even possible) would blind the sweep to its own use case. + /// /// - private long _definitionsWriteGeneration; + private long _storeWriteGeneration; /// - /// Row 101 guard in front of : applies the - /// snapshot only when no artifact apply mutated the definitions while the read was in - /// flight. A stale snapshot is discarded WHOLE — its absences would spuriously tombstone - /// and stop just-applied live connections (which nothing re-creates until an instance - /// redeploy), and its rows would push superseded configs onto live connections. Discarding - /// is safe because the artifact apply already updated the overlay itself and the next - /// tick's read starts after the write, so convergence is one tick away. + /// Records that this actor has mutated one of the two snapshot tables, invalidating every + /// store read currently in flight. Called on the actor thread at BOTH ends of a mutation — + /// when the off-thread write is dispatched and again when its completion message lands. See + /// for why both ends are required. /// - private void ApplyConnectionDefinitionOverlayIfFresh( - IReadOnlyList? definitions, - long generationAtRead, - string source) + /// Provenance for the Trace line; never affects behaviour. + private void BumpStoreWriteGeneration(string source) { - if (generationAtRead != _definitionsWriteGeneration) - { - _logger.LogDebug( - "Discarding the {Source} data-connection definitions snapshot — an artifact " - + "apply changed the definitions while the read was in flight (generation " - + "{ReadGeneration} vs {CurrentGeneration}); the next sweep tick reads fresh", - source, generationAtRead, _definitionsWriteGeneration); - return; - } + _storeWriteGeneration++; + _logger.LogTrace( + "Store write generation bumped to {Generation} by {Source}", _storeWriteGeneration, source); + } - ApplyConnectionDefinitionOverlay(definitions); + /// + /// The row-101 freshness test both snapshot consumers run BEFORE touching anything the + /// snapshot carries. means this actor mutated one of the two tables + /// while the read was in flight, so the snapshot predates state the actor has already + /// advanced past and must be discarded whole (see for + /// what each half would otherwise do). Logged at Debug: on a healthy site this fires only + /// when a deploy/enable/disable/delete happens to overlap a sweep tick, which is normal and + /// self-correcting. + /// + private bool IsStoreSnapshotStale(long generationAtRead, string source) + { + if (generationAtRead == _storeWriteGeneration) + return false; + + _logger.LogDebug( + "Discarding the {Source} store snapshot — this node mutated the deployed configs or " + + "the connection definitions while the read was in flight (generation " + + "{ReadGeneration} vs {CurrentGeneration})", + source, generationAtRead, _storeWriteGeneration); + return true; } /// @@ -2920,6 +3079,12 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers // data-connections branch below dispatches via this captured ref. var self = Self; + // Row 101 (initiation half): the artifact apply rewrites data_connection_definitions + // off-thread. The completion half is the ApplyArtifactDataConnectionsToDcl handler; this + // bump covers the window before it, where a read would otherwise land carrying the + // pre-apply table and be treated as current. + BumpStoreWriteGeneration("artifact apply dispatch"); + Task.Run(async () => { try @@ -3121,9 +3286,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// private void HandleApplyArtifactDataConnectionsToDcl(ApplyArtifactDataConnectionsToDcl msg) { - // Row 101: any snapshot read dispatched before this point carries data that predates - // these mutations — mark it stale so its overlay half is discarded when it lands. - _definitionsWriteGeneration++; + // Row 101 (completion half): the definitions rows are committed by the time this message + // is dispatched, so any snapshot read that started earlier carries data predating them — + // mark it stale so it is discarded when it lands. + BumpStoreWriteGeneration("artifact apply"); foreach (var dc in msg.DataConnections) { @@ -3216,16 +3382,18 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// is the deploy-only site shape (see ). /// /// - /// - /// Row 101: captured on the actor thread when the - /// read was dispatched. A mismatch on landing means an artifact apply mutated the overlay - /// while this read was in flight — the snapshot's definitions half is stale and is skipped. + /// + /// Row 101: captured on the actor thread when the read + /// was dispatched. A mismatch on landing means this actor mutated the deployed configs or + /// the connection definitions while the read was in flight, so the WHOLE snapshot predates + /// the actor's own state. The boot load cannot simply drop it (startup has to finish), so + /// re-reads instead. /// internal record StartupConfigsLoaded( List Configs, string? Error, IReadOnlyList? ConnectionDefinitions = null, - long DefinitionsGenerationAtRead = 0); + long StoreGenerationAtRead = 0); /// /// Internal singleton signal that re-attempts the startup deployed-config load @@ -3318,14 +3486,26 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers /// contract as : null = read failed, keep the overlay. /// /// - /// + /// /// Row 101: same stale-snapshot guard as — see there. + /// The sweep's copy is DROPPED on a mismatch rather than re-read: there is nothing to + /// complete, and the next tick is already a fresh read. /// internal record InstanceSweepLoaded( List Configs, string? Error, IReadOnlyList? ConnectionDefinitions = null, - long DefinitionsGenerationAtRead = 0); + long StoreGenerationAtRead = 0); + + /// + /// Row 101: piped back to self once an off-thread mutation of one of the two snapshot tables + /// has COMMITTED, purely so can be bumped on the actor + /// thread at a point that provably happens-after the write. Used by the mutation paths that + /// have no other actor-thread completion message of their own (disable, delete, the + /// init-failure rollback); paths that already pipe a result bump in that handler instead. + /// Carries only a provenance string, for the Trace line. + /// + internal sealed record StoreWriteCompleted(string Source); internal record DeployPersistenceResult( string DeploymentId, string InstanceName, bool Success, string? Error, diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs index e77b0177..42706887 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/DclConnectionArbitrationTests.cs @@ -596,7 +596,7 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable // Generation 0 = captured before the apply bumped it; empty list = the pre-write table. dm.Tell(new DeploymentManagerActor.InstanceSweepLoaded( - [], null, ConnectionDefinitions: [], DefinitionsGenerationAtRead: 0)); + [], null, ConnectionDefinitions: [], StoreGenerationAtRead: 0)); await Task.Delay(TimeSpan.FromSeconds(2)); Assert.DoesNotContain(_removes, r => r.ConnectionName == ConnectionName); @@ -619,9 +619,12 @@ public class DclConnectionArbitrationTests : TestKit, IDisposable Assert.True(ExpectMsg(TimeSpan.FromSeconds(20)).Success); await AwaitCreateCountAsync(1, "the artifact apply never created the connection"); - // Exactly one artifact apply has run, so the current write generation is 1. + // Exactly one artifact apply has run, and every store mutation bumps the generation + // TWICE — once when the off-thread write is dispatched and once when its completion + // message lands (that pair is what leaves no window in which an in-flight read looks + // fresh; see DeploymentManagerActor._storeWriteGeneration) — so the current value is 2. dm.Tell(new DeploymentManagerActor.InstanceSweepLoaded( - [], null, ConnectionDefinitions: [], DefinitionsGenerationAtRead: 1)); + [], null, ConnectionDefinitions: [], StoreGenerationAtRead: 2)); await AwaitAssertAsync(() => { diff --git a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs index 582daca0..66bb941d 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceSweepSelfHealTests.cs @@ -4,6 +4,7 @@ using Akka.TestKit.Xunit2; using Microsoft.Extensions.Logging.Abstractions; using ZB.MOM.WW.ScadaBridge.Commons.Messages.DebugView; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Deployment; +using ZB.MOM.WW.ScadaBridge.Commons.Messages.Lifecycle; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; using ZB.MOM.WW.ScadaBridge.Commons.Types.Flattening; using ZB.MOM.WW.ScadaBridge.SiteRuntime.Actors; @@ -510,4 +511,257 @@ public class InstanceSweepSelfHealTests : TestKit, IDisposable Assert.True(await InstanceIsRunningAsync(dm, instanceName)); } + // ── Row 101 (config half): a sweep snapshot older than the actor's own state ── + + /// + /// A crafted sweep result standing in for a read that was dispatched earlier than the + /// generation it is stamped with. 0 is the value a + /// read dispatched before this actor had mutated anything would carry; anything the actor has + /// done since makes it stale. Connection definitions are passed as + /// (= "the definitions read failed, keep the overlay") so these tests exercise the CONFIG + /// half of the snapshot in isolation. + /// + private static DeploymentManagerActor.InstanceSweepLoaded SweepSnapshot( + string instanceName, string revision, string configJson, long generationAtRead, + bool isEnabled = true) => + new( + [ + new DeployedInstance + { + InstanceUniqueName = instanceName, + ConfigJson = configJson, + DeploymentId = $"dep-{revision}", + RevisionHash = revision, + IsEnabled = isEnabled, + DeployedAt = DateTimeOffset.UtcNow.ToString("O") + } + ], + null, + ConnectionDefinitions: null, + StoreGenerationAtRead: generationAtRead); + + /// + /// Deferred register row 101, the row-72 leg. A sweep read dispatched before a deploy + /// persisted its new row can land after that deploy has completed AND after the deploy + /// pipeline released the instance — 's + /// InstanceIsOwnedByADeployPipeline only covers a deploy still in flight, so it is no + /// defence here. The snapshot then carries the revision the deploy just REPLACED while the + /// running actor carries the new one, which is bit-for-bit the divergence the row-72 repair + /// looks for — in the wrong direction. + /// + /// + /// Fails before the fix: the sweep "repairs" the instance BACKWARD, re-deploying the + /// snapshot's own superseded config JSON through the redeploy path (attribute back to + /// 111) and announcing it as a self-heal. Nothing retries it either — the per-revision + /// claim marks the OLD revision as attempted, so the instance simply stays reverted. + /// + /// + [Fact] + public async Task StaleSweepSnapshot_CarryingASupersededRevision_DoesNotRestartBackward() + { + const string instanceName = "BackwardRestartPump"; + var siteLog = new FakeSiteEventLogger(); + // Sweep effectively disabled: the crafted message must be the only sweep result in play. + var dm = CreateBootedDeploymentManager(siteLog, sweepSeconds: 3600); + + await DeployAndAwaitAsync( + dm, instanceName, "rev-1", MakeConfigJson(instanceName, attributeValue: "111")); + await DeployAndAwaitAsync( + dm, instanceName, "rev-2", MakeConfigJson(instanceName, attributeValue: "222")); + + var afterDeploys = await ResolveInstanceChildAsync(dm, instanceName); + + // Generation 0 = captured before either deploy mutated the store. + dm.Tell(SweepSnapshot( + instanceName, "rev-1", MakeConfigJson(instanceName, attributeValue: "111"), + generationAtRead: 0)); + + await Task.Delay(SeveralSweeps); + + Assert.Empty(SelfHealEvents(siteLog, instanceName)); + Assert.Equal(afterDeploys, await ResolveInstanceChildAsync(dm, instanceName)); + var snapshot = await SnapshotAsync(dm, instanceName); + Assert.Contains(snapshot.AttributeValues, a => + a.AttributeName == "TestAttr" && a.Value?.ToString() == "222"); + } + + /// + /// The row-101 guard must not blunt the row-72 repair it sits in front of: the SAME message + /// stamped with the CURRENT generation is a genuine answer and must still converge the + /// runtime on the stored revision. + /// + /// + /// The instance is started by the BOOT LOAD from a pre-written row rather than by a deploy + /// command, precisely so no store mutation passes through the actor and the current + /// generation is still 0 — the same literal the stale test uses, with the opposite + /// meaning, which is the whole point of the guard. + /// + /// + [Fact] + public async Task FreshSweepSnapshot_CarryingANewerRevision_StillRestartsTheInstance() + { + const string instanceName = "FreshRestartPump"; + var siteLog = new FakeSiteEventLogger(); + + await _storage.StoreDeployedConfigAsync( + instanceName, MakeConfigJson(instanceName, attributeValue: "111"), + "dep-rev-1", "rev-1", true); + + var dm = CreateBootedDeploymentManager(siteLog, sweepSeconds: 3600); + await AwaitAssertAsync(async () => + Assert.True(await InstanceIsRunningAsync(dm, instanceName)), TimeSpan.FromSeconds(20)); + + dm.Tell(SweepSnapshot( + instanceName, "rev-2", MakeConfigJson(instanceName, attributeValue: "222"), + generationAtRead: 0)); + + await AwaitAssertAsync(async () => + { + var snapshot = await SnapshotAsync(dm, instanceName); + Assert.False(snapshot.InstanceNotFound); + Assert.Contains(snapshot.AttributeValues, a => + a.AttributeName == "TestAttr" && a.Value?.ToString() == "222"); + }, TimeSpan.FromSeconds(25)); + + Assert.Contains(SelfHealEvents(siteLog, instanceName), e => + e.Severity == "Warning" && + e.Message.Contains("rev-1", StringComparison.Ordinal) && + e.Message.Contains("rev-2", StringComparison.Ordinal)); + } + + /// + /// Deferred register row 101, the row-78 leg. A sweep read dispatched before a delete + /// committed still shows the instance's row — enabled, with a config — while the actor has + /// already stopped it and dropped its bookkeeping. The delete specifically clears + /// _sweepAttemptedRevisions, so the once-per-revision claim is freshly available and + /// does not stand in the way either. + /// + /// + /// Fails before the fix: the sweep RESURRECTS the instance the operator just deleted, + /// re-adds it to the deployed-name set, and announces the resurrection as a self-heal. + /// + /// + [Fact] + public async Task StaleSweepSnapshot_ShowingADeletedInstance_DoesNotResurrectIt() + { + const string instanceName = "ResurrectedPump"; + var siteLog = new FakeSiteEventLogger(); + var dm = CreateBootedDeploymentManager(siteLog, sweepSeconds: 3600); + + await DeployAndAwaitAsync(dm, instanceName, "rev-1", MakeConfigJson(instanceName)); + + dm.Tell(new DeleteInstanceCommand("cmd-del-1", instanceName, DateTimeOffset.UtcNow)); + Assert.True(ExpectMsg(TimeSpan.FromSeconds(15)).Success); + await AwaitAssertAsync(async () => + Assert.False(await InstanceIsRunningAsync(dm, instanceName)), TimeSpan.FromSeconds(15)); + + // Generation 0 = captured before the deploy and the delete mutated the store. + dm.Tell(SweepSnapshot( + instanceName, "rev-1", MakeConfigJson(instanceName), generationAtRead: 0)); + + await Task.Delay(SeveralSweeps); + + Assert.False(await InstanceIsRunningAsync(dm, instanceName)); + Assert.Empty(SelfHealEvents(siteLog, instanceName)); + } + + /// + /// The mirror of the resurrection test: the same enabled-config-with-no-actor snapshot, + /// stamped with the CURRENT generation, must still perform the row-78 start. Nothing has + /// mutated the store through this actor, so 0 is current here. + /// + [Fact] + public async Task FreshSweepSnapshot_ShowingAStoredEnabledInstance_StillStartsIt() + { + const string instanceName = "FreshStartPump"; + var siteLog = new FakeSiteEventLogger(); + var dm = CreateBootedDeploymentManager(siteLog, sweepSeconds: 3600); + + dm.Tell(SweepSnapshot( + instanceName, "rev-1", MakeConfigJson(instanceName), generationAtRead: 0)); + + await AwaitAssertAsync(async () => + Assert.True(await InstanceIsRunningAsync(dm, instanceName)), TimeSpan.FromSeconds(20)); + Assert.Single(SelfHealEvents(siteLog, instanceName)); + } + + /// + /// Row 101, boot half. The boot-time load has to COMPLETE — a node whose startup read + /// is simply dropped stays a silent zero-instance node — so a superseded boot snapshot is + /// re-read rather than discarded like the sweep's. + /// + /// + /// The race is made deterministic by a slow first read: a lifecycle command that mutates + /// deployed_configurations is processed while it is in flight, so the snapshot lands + /// stamped with a generation the actor has already moved past. The observable is the read + /// itself — the loader must be called a SECOND time — and, just as importantly, exactly twice: + /// the re-dispatch captures the post-mutation generation, so it converges instead of looping. + /// + /// + /// Fails before the fix: one read, and the pre-mutation snapshot is applied. + /// + [Fact] + public async Task StaleBootSnapshot_IsReReadRatherThanApplied() + { + var reads = 0; + var health = new DeploymentManagerStartupProbe(); + + var configLoader = SlowFirstConfigLoad(() => Interlocked.Increment(ref reads)); + var dm = ActorOf(Props.Create(() => new DeploymentManagerActor( + _storage, + _compilationService, + _sharedScriptLibrary, + null, + new SiteRuntimeOptions { InstanceSweepIntervalSeconds = 3600 }, + NullLogger.Instance, + dclManager: null, + healthCollector: health, + serviceProvider: null, + loggerFactory: null, + configFetcher: null, + startupLoadRetryInterval: null, + configLoader: configLoader, + externalSystemCache: null, + scriptScheduler: null, + connectionDefinitionLoader: null))); + + // Mutates deployed_configurations (a no-op UPDATE for an unknown instance is still a + // write dispatched from the actor thread) while the first read is still in flight. + dm.Tell(new DisableInstanceCommand("cmd-boot-race", "NoSuchInstance", DateTimeOffset.UtcNow)); + ExpectMsg(TimeSpan.FromSeconds(15)); + + await AwaitAssertAsync(() => + { + Assert.Equal(2, Volatile.Read(ref reads)); + return Task.CompletedTask; + }, TimeSpan.FromSeconds(20)); + + // The second read is fresh, so the boot completes and stops re-reading... + await Task.Delay(TimeSpan.FromSeconds(2)); + Assert.Equal(2, Volatile.Read(ref reads)); + + // ...and a LATE superseded snapshot (the S5 retry timer racing a success) is dropped + // rather than triggering another read, because the boot it would complete is done. + dm.Tell(new DeploymentManagerActor.StartupConfigsLoaded( + [], null, ConnectionDefinitions: null, StoreGenerationAtRead: -1)); + await Task.Delay(TimeSpan.FromSeconds(2)); + Assert.Equal(2, Volatile.Read(ref reads)); + } + + /// + /// A deployed-config loader whose FIRST call takes long enough for a lifecycle command to be + /// processed while it is in flight; later calls answer immediately. Always returns an empty + /// site — these tests assert on how many reads happen, not on what they carry. + /// + private static Func>> SlowFirstConfigLoad(Action onRead) + { + var calls = 0; + return async () => + { + onRead(); + if (Interlocked.Increment(ref calls) == 1) + await Task.Delay(TimeSpan.FromSeconds(2)); + return []; + }; + } }