merge(agent): whole-snapshot staleness guard — generalize row 101's definitions generation to one _storeWriteGeneration over both tables, bumped at BOTH ends of every actor-thread config mutation (dispatch + completion; ApplyDeployment/Disable/Delete advance actor state before the write, so completion-only left a window); stale sweep discarded whole (closes the proven backward-restart and delete-resurrection hazards), stale boot load re-dispatched with a capped fallback; CDC/reconciliation writes deliberately never bump — discovering them is the sweep's purpose
This commit is contained in:
@@ -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: <name>` 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*.
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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: <name>` 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.
|
||||
>
|
||||
|
||||
@@ -97,6 +97,19 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
|
||||
/// stale <see cref="RetryStartupLoad"/> re-running the load after success.
|
||||
/// </summary>
|
||||
private bool _startupLoadCompleted;
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <see cref="HandleStartupConfigsLoaded"/>.
|
||||
/// </summary>
|
||||
private const int MaxConsecutiveStaleStartupLoads = 5;
|
||||
|
||||
/// <summary>
|
||||
/// Consecutive boot-time store reads discarded as superseded. Reset the moment a fresh one
|
||||
/// lands, so the cap only ever counts an unbroken run.
|
||||
/// </summary>
|
||||
private int _consecutiveStaleStartupLoads;
|
||||
private readonly Dictionary<string, IActorRef> _instanceActors = new();
|
||||
/// <summary>
|
||||
/// 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<DeployPersistenceResult>(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<StoreWriteCompleted>(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<InstanceActorInitialized>(HandleInstanceActorInitialized);
|
||||
@@ -498,9 +515,10 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
|
||||
/// </summary>
|
||||
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
|
||||
/// </summary>
|
||||
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
|
||||
/// </summary>
|
||||
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);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -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
|
||||
/// </summary>
|
||||
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<string> _centrallyRemovedConnections = new(StringComparer.Ordinal);
|
||||
|
||||
/// <summary>
|
||||
/// 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 <see
|
||||
/// cref="HandleApplyArtifactDataConnectionsToDcl"/> 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
|
||||
/// <see cref="EnsureDclConnection"/> 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
|
||||
/// <see cref="ReadStoreSnapshotAsync"/> reads — <c>deployed_configurations</c> AND
|
||||
/// <c>data_connection_definitions</c> (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.
|
||||
///
|
||||
/// <para>
|
||||
/// <b>One counter for both tables, deliberately.</b> 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>What a stale snapshot would actually do</b> — each of these is a real defect the guard
|
||||
/// closes, not a theoretical one:
|
||||
/// <list type="bullet">
|
||||
/// <item>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.</item>
|
||||
/// <item>CONFIGS half, row-72 leg: a snapshot carrying the revision a deploy has just
|
||||
/// replaced makes <see cref="TryRestartStaleInstance"/> "repair" the instance BACKWARD —
|
||||
/// it redeploys the snapshot's own (old) config JSON through the redeploy path.
|
||||
/// <see cref="InstanceIsOwnedByADeployPipeline"/> does not cover this: it only holds while
|
||||
/// the deploy is in flight, and the stale snapshot can land after the pipeline released
|
||||
/// ownership.</item>
|
||||
/// <item>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
|
||||
/// <see cref="StartStoredInstance"/> RESURRECTS a just-stopped instance.</item>
|
||||
/// </list>
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Bumped twice per mutation — at initiation and at completion</b> (see
|
||||
/// <see cref="BumpStoreWriteGeneration"/>), 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Writes that must NOT bump.</b> Rows arriving by LocalDb CDC replication, by a snapshot
|
||||
/// resync, by an operator restoring the file, or by <c>SiteReconciliationActor</c>'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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private long _definitionsWriteGeneration;
|
||||
private long _storeWriteGeneration;
|
||||
|
||||
/// <summary>
|
||||
/// Row 101 guard in front of <see cref="ApplyConnectionDefinitionOverlay"/>: 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
|
||||
/// <see cref="_storeWriteGeneration"/> for why both ends are required.
|
||||
/// </summary>
|
||||
private void ApplyConnectionDefinitionOverlayIfFresh(
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? definitions,
|
||||
long generationAtRead,
|
||||
string source)
|
||||
/// <param name="source">Provenance for the Trace line; never affects behaviour.</param>
|
||||
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);
|
||||
/// <summary>
|
||||
/// The row-101 freshness test both snapshot consumers run BEFORE touching anything the
|
||||
/// snapshot carries. <see langword="true"/> 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 <see cref="_storeWriteGeneration"/> 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.
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -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
|
||||
/// </summary>
|
||||
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 <see cref="ApplyConnectionDefinitionOverlay"/>).
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="DefinitionsGenerationAtRead">
|
||||
/// Row 101: <see cref="_definitionsWriteGeneration"/> 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.
|
||||
/// <param name="StoreGenerationAtRead">
|
||||
/// Row 101: <see cref="_storeWriteGeneration"/> 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
|
||||
/// <see cref="HandleStartupConfigsLoaded"/> re-reads instead.
|
||||
/// </param>
|
||||
internal record StartupConfigsLoaded(
|
||||
List<DeployedInstance> Configs,
|
||||
string? Error,
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null,
|
||||
long DefinitionsGenerationAtRead = 0);
|
||||
long StoreGenerationAtRead = 0);
|
||||
|
||||
/// <summary>
|
||||
/// Internal singleton signal that re-attempts the startup deployed-config load
|
||||
@@ -3318,14 +3486,26 @@ public class DeploymentManagerActor : ReceiveActor, IWithTimers
|
||||
/// contract as <see cref="StartupConfigsLoaded"/>: null = read failed, keep the overlay.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
/// <param name="DefinitionsGenerationAtRead">
|
||||
/// <param name="StoreGenerationAtRead">
|
||||
/// Row 101: same stale-snapshot guard as <see cref="StartupConfigsLoaded"/> — 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.
|
||||
/// </param>
|
||||
internal record InstanceSweepLoaded(
|
||||
List<DeployedInstance> Configs,
|
||||
string? Error,
|
||||
IReadOnlyList<StoredDataConnectionDefinition>? ConnectionDefinitions = null,
|
||||
long DefinitionsGenerationAtRead = 0);
|
||||
long StoreGenerationAtRead = 0);
|
||||
|
||||
/// <summary>
|
||||
/// Row 101: piped back to self once an off-thread mutation of one of the two snapshot tables
|
||||
/// has COMMITTED, purely so <see cref="_storeWriteGeneration"/> 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.
|
||||
/// </summary>
|
||||
internal sealed record StoreWriteCompleted(string Source);
|
||||
|
||||
internal record DeployPersistenceResult(
|
||||
string DeploymentId, string InstanceName, bool Success, string? Error,
|
||||
|
||||
+6
-3
@@ -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<ArtifactDeploymentResponse>(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(() =>
|
||||
{
|
||||
|
||||
@@ -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 ──
|
||||
|
||||
/// <summary>
|
||||
/// A crafted sweep result standing in for a read that was dispatched earlier than the
|
||||
/// generation it is stamped with. <paramref name="generationAtRead"/> <c>0</c> 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 <see langword="null"/>
|
||||
/// (= "the definitions read failed, keep the overlay") so these tests exercise the CONFIG
|
||||
/// half of the snapshot in isolation.
|
||||
/// </summary>
|
||||
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);
|
||||
|
||||
/// <summary>
|
||||
/// <b>Deferred register row 101, the row-72 leg.</b> 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 — <see cref="DeploymentManagerActor"/>'s
|
||||
/// <c>InstanceIsOwnedByADeployPipeline</c> 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.
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Fails before the fix</b>: the sweep "repairs" the instance BACKWARD, re-deploying the
|
||||
/// snapshot's own superseded config JSON through the redeploy path (attribute back to
|
||||
/// <c>111</c>) 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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");
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
///
|
||||
/// <para>
|
||||
/// 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 <c>0</c> — the same literal the stale test uses, with the opposite
|
||||
/// meaning, which is the whole point of the guard.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <b>Deferred register row 101, the row-78 leg.</b> 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
|
||||
/// <c>_sweepAttemptedRevisions</c>, so the once-per-revision claim is freshly available and
|
||||
/// does not stand in the way either.
|
||||
///
|
||||
/// <para>
|
||||
/// <b>Fails before the fix</b>: 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.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[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<InstanceLifecycleResponse>(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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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 <c>0</c> is current here.
|
||||
/// </summary>
|
||||
[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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <b>Row 101, boot half.</b> 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.
|
||||
///
|
||||
/// <para>
|
||||
/// The race is made deterministic by a slow first read: a lifecycle command that mutates
|
||||
/// <c>deployed_configurations</c> 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.
|
||||
/// </para>
|
||||
///
|
||||
/// <para><b>Fails before the fix</b>: one read, and the pre-mutation snapshot is applied.</para>
|
||||
/// </summary>
|
||||
[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<DeploymentManagerActor>.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<InstanceLifecycleResponse>(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));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
private static Func<Task<List<DeployedInstance>>> SlowFirstConfigLoad(Action onRead)
|
||||
{
|
||||
var calls = 0;
|
||||
return async () =>
|
||||
{
|
||||
onRead();
|
||||
if (Interlocked.Increment(ref calls) == 1)
|
||||
await Task.Delay(TimeSpan.FromSeconds(2));
|
||||
return [];
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user