fix(site-secrets): keep a live node's mirror-status row from losing a race with the peer's prune
Rig finding on the upgrade restart: both nodes' rows were older than the stale threshold (the old build never refreshed them), so node-a's reporter pruned node-b's row while node-b wrote a fresh one, and the replicated delete won. The reporter now prunes nothing during its first refresh interval after start-up, and each mirror pass re-writes its own row if it no longer holds what the mirror last wrote.
This commit is contained in:
@@ -451,7 +451,7 @@ Site secrets ride the artifact deploy (design `docs/plans/2026-09-28-site-secret
|
|||||||
- **Isolation.** One row failing to apply/verify does not block any other row in the same pass.
|
- **Isolation.** One row failing to apply/verify does not block any other row in the same pass.
|
||||||
- **Cache invalidation.** A successful apply or delete invalidates the shared `ISecretResolver` cache before verifying, so the verify step (and any script/connection reading the secret) sees the fresh value immediately rather than waiting out the resolver's cache TTL.
|
- **Cache invalidation.** A successful apply or delete invalidates the shared `ISecretResolver` cache before verifying, so the verify step (and any script/connection reading the secret) sees the fresh value immediately rather than waiting out the resolver's cache TTL.
|
||||||
- **Never logs a secret value** — log lines and the reported `MirrorResult` carry only names, revisions, and `KekId`s.
|
- **Never logs a secret value** — log lines and the reported `MirrorResult` carry only names, revisions, and `KekId`s.
|
||||||
- **Mirror status on the health report (review finding M1).** A failure on the **standby** — above all a secret that will not decrypt because the standby's master key differs from central's — used to live only in that node's log, invisible until a failover broke its connections. After every pass the mirror records its node's current failure set in `secret_mirror_status`, but **only when the set (sorted name+message pairs) differs from what this process last wrote**, plus once on the first pass after start-up so a restart refreshes a stale row, plus at least every `SiteRuntime:SecretMirrorStatusRefreshInterval` (default **10 min**) as a heartbeat — never on every 15 s tick, because every write is a replicated CDC change. The write is best-effort: a failure logs a Warning (repeats at Debug), never affects the pass result, and is retried next pass. `SecretMirrorStatusReporter` (`SiteRuntime/Secrets/`, every site node, 30 s, immediate first probe) reads every node's row into `ISiteHealthCollector.SetSecretMirrorStatuses`, so the active node's report carries both nodes' status as `SiteHealthReport.SecretMirrorStatuses` (see Component-HealthMonitoring.md). On an **unreplicated** pair only the active node's own row is visible — the same replication prerequisite deployed secrets already have. **Stale rows are pruned:** the row is keyed by node name, so a decommissioned or renamed node used to leave its row (and any *Secret mirror failing* badge) behind forever. The reporter now leaves off the report, and deletes, any row whose `checked_at` is older than `SiteRuntime:SecretMirrorStatusStaleAfter` (default **1 h**, validated `≥ 2 ×` the refresh interval) — the delete is conditional on the `checked_at` it read, so a node that re-wrote its row in the meantime keeps it, and it replicates like any other write. A node down longer than that also drops off the list (its absence already shows in the cluster member count) and writes its row again on its first pass after it returns.
|
- **Mirror status on the health report (review finding M1).** A failure on the **standby** — above all a secret that will not decrypt because the standby's master key differs from central's — used to live only in that node's log, invisible until a failover broke its connections. After every pass the mirror records its node's current failure set in `secret_mirror_status`, but **only when the set (sorted name+message pairs) differs from what this process last wrote**, plus once on the first pass after start-up so a restart refreshes a stale row, plus at least every `SiteRuntime:SecretMirrorStatusRefreshInterval` (default **10 min**) as a heartbeat — never on every 15 s tick, because every write is a replicated CDC change. The write is best-effort: a failure logs a Warning (repeats at Debug), never affects the pass result, and is retried next pass. `SecretMirrorStatusReporter` (`SiteRuntime/Secrets/`, every site node, 30 s, immediate first probe) reads every node's row into `ISiteHealthCollector.SetSecretMirrorStatuses`, so the active node's report carries both nodes' status as `SiteHealthReport.SecretMirrorStatuses` (see Component-HealthMonitoring.md). On an **unreplicated** pair only the active node's own row is visible — the same replication prerequisite deployed secrets already have. **Stale rows are pruned:** the row is keyed by node name, so a decommissioned or renamed node used to leave its row (and any *Secret mirror failing* badge) behind forever. The reporter now leaves off the report, and deletes, any row whose `checked_at` is older than `SiteRuntime:SecretMirrorStatusStaleAfter` (default **1 h**, validated `≥ 2 ×` the refresh interval) — the delete is conditional on the `checked_at` it read, so a node that re-wrote its row in the meantime keeps it, and it replicates like any other write. A node down longer than that also drops off the list (its absence already shows in the cluster member count) and writes its row again on its first pass after it returns. Two guards keep a live node's row from being lost to the prune: the reporter prunes nothing during its first refresh interval after start-up (on a pair restarted together — an upgrade from a build without the heartbeat — every row is still old until each node's first write has replicated), and each mirror pass checks with a local read that its own row still holds what it last wrote, re-writing it otherwise (a peer's prune racing this node's write can replicate a delete over it; found on the rig).
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|||||||
@@ -1006,6 +1006,26 @@ public class SiteStorageService
|
|||||||
return results;
|
return results;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Returns the raw <c>checked_at</c> of <paramref name="nodeName"/>'s
|
||||||
|
/// <c>secret_mirror_status</c> row, or <see langword="null"/> when the node has no row — a
|
||||||
|
/// cheap local read the mirror uses to notice its row was deleted by the peer's prune.
|
||||||
|
/// </summary>
|
||||||
|
/// <param name="nodeName">The row key.</param>
|
||||||
|
/// <returns>The stored <c>checked_at</c> text, or <see langword="null"/>.</returns>
|
||||||
|
public async Task<string?> GetSecretMirrorStatusCheckedAtAsync(string nodeName)
|
||||||
|
{
|
||||||
|
ArgumentException.ThrowIfNullOrWhiteSpace(nodeName);
|
||||||
|
|
||||||
|
await using var connection = OpenConnection();
|
||||||
|
|
||||||
|
await using var command = connection.CreateCommand();
|
||||||
|
command.CommandText = "SELECT checked_at FROM secret_mirror_status WHERE node_name = @nodeName";
|
||||||
|
command.Parameters.AddWithValue("@nodeName", nodeName);
|
||||||
|
|
||||||
|
return await command.ExecuteScalarAsync() as string;
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Deletes <paramref name="nodeName"/>'s <c>secret_mirror_status</c> row, but only while it
|
/// Deletes <paramref name="nodeName"/>'s <c>secret_mirror_status</c> row, but only while it
|
||||||
/// still carries <paramref name="checkedAtUtc"/> — so a row the node re-wrote after the caller
|
/// still carries <paramref name="checkedAtUtc"/> — so a row the node re-wrote after the caller
|
||||||
|
|||||||
@@ -166,7 +166,10 @@ public sealed record MirrorResult(
|
|||||||
/// wrote — plus once on the first pass after start-up, so a restart refreshes a stale row, and
|
/// wrote — plus once on the first pass after start-up, so a restart refreshes a stale row, and
|
||||||
/// at least every <see cref="SiteRuntimeOptions.SecretMirrorStatusRefreshInterval"/> so the
|
/// at least every <see cref="SiteRuntimeOptions.SecretMirrorStatusRefreshInterval"/> so the
|
||||||
/// row's <c>checked_at</c> works as a heartbeat: <c>SecretMirrorStatusReporter</c> prunes the
|
/// row's <c>checked_at</c> works as a heartbeat: <c>SecretMirrorStatusReporter</c> prunes the
|
||||||
/// row of a node that has stopped refreshing it (decommissioned or renamed). The
|
/// row of a node that has stopped refreshing it (decommissioned or renamed). Each pass also
|
||||||
|
/// checks (a local read) that the row still holds what this process last wrote, and re-writes it
|
||||||
|
/// if not — a peer's prune racing this node's own write can replicate a delete over it, e.g. when
|
||||||
|
/// both nodes restart together with rows older than the stale threshold. The
|
||||||
/// write is best-effort: a failure logs a Warning (repeats at Debug) and is retried on the next
|
/// write is best-effort: a failure logs a Warning (repeats at Debug) and is retried on the next
|
||||||
/// pass, and never affects the pass's own <see cref="MirrorResult"/>.
|
/// pass, and never affects the pass's own <see cref="MirrorResult"/>.
|
||||||
/// </para>
|
/// </para>
|
||||||
@@ -365,7 +368,9 @@ public sealed class DeployedSecretsMirror : BackgroundService
|
|||||||
/// <summary>
|
/// <summary>
|
||||||
/// Writes this node's <c>secret_mirror_status</c> row when the pass's failure set differs
|
/// Writes this node's <c>secret_mirror_status</c> row when the pass's failure set differs
|
||||||
/// from the last one this process wrote, nothing has been written yet since start-up, or the
|
/// from the last one this process wrote, nothing has been written yet since start-up, or the
|
||||||
/// last write is older than <see cref="SiteRuntimeOptions.SecretMirrorStatusRefreshInterval"/>.
|
/// last write is older than <see cref="SiteRuntimeOptions.SecretMirrorStatusRefreshInterval"/>,
|
||||||
|
/// or the stored row no longer holds what this process wrote (deleted or overwritten by a
|
||||||
|
/// replicated change).
|
||||||
/// Best-effort: never throws, so the pass result is unaffected by a status-write fault.
|
/// Best-effort: never throws, so the pass result is unaffected by a status-write fault.
|
||||||
/// See the class remarks (review finding M1).
|
/// See the class remarks (review finding M1).
|
||||||
/// </summary>
|
/// </summary>
|
||||||
@@ -378,15 +383,18 @@ public sealed class DeployedSecretsMirror : BackgroundService
|
|||||||
.ToList();
|
.ToList();
|
||||||
|
|
||||||
var now = _time.GetUtcNow();
|
var now = _time.GetUtcNow();
|
||||||
if (_lastWrittenStatus is not null
|
|
||||||
&& _lastWrittenStatus.SequenceEqual(current)
|
|
||||||
&& now - _lastWrittenStatusAt < _options.Value.SecretMirrorStatusRefreshInterval)
|
|
||||||
{
|
|
||||||
return;
|
|
||||||
}
|
|
||||||
|
|
||||||
try
|
try
|
||||||
{
|
{
|
||||||
|
if (_lastWrittenStatus is not null
|
||||||
|
&& _lastWrittenStatus.SequenceEqual(current)
|
||||||
|
&& now - _lastWrittenStatusAt < _options.Value.SecretMirrorStatusRefreshInterval
|
||||||
|
&& await _storage.GetSecretMirrorStatusCheckedAtAsync(_nodeName).ConfigureAwait(false)
|
||||||
|
== _lastWrittenStatusAt.ToUniversalTime().ToString("O"))
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
await _storage.SetSecretMirrorStatusAsync(_nodeName, current, now).ConfigureAwait(false);
|
await _storage.SetSecretMirrorStatusAsync(_nodeName, current, now).ConfigureAwait(false);
|
||||||
_lastWrittenStatus = current;
|
_lastWrittenStatus = current;
|
||||||
_lastWrittenStatusAt = now;
|
_lastWrittenStatusAt = now;
|
||||||
|
|||||||
@@ -34,6 +34,9 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Secrets;
|
|||||||
/// keyed by node name), or down for that long. Such a row is left off the report and deleted
|
/// keyed by node name), or down for that long. Such a row is left off the report and deleted
|
||||||
/// (only while it still carries the stale <c>checked_at</c>, so a node that came back in the
|
/// (only while it still carries the stale <c>checked_at</c>, so a node that came back in the
|
||||||
/// meantime keeps its fresh row); a returning node writes its row again on its first pass.
|
/// meantime keeps its fresh row); a returning node writes its row again on its first pass.
|
||||||
|
/// Nothing is pruned during the reporter's first <c>pruneGrace</c> (the refresh interval) after
|
||||||
|
/// start-up: when both nodes of a pair restart together, every live node's row can still be old
|
||||||
|
/// until its mirror's first write has replicated, and pruning it then only races that write.
|
||||||
/// </para>
|
/// </para>
|
||||||
/// </remarks>
|
/// </remarks>
|
||||||
public sealed class SecretMirrorStatusReporter : BackgroundService
|
public sealed class SecretMirrorStatusReporter : BackgroundService
|
||||||
@@ -47,6 +50,7 @@ public sealed class SecretMirrorStatusReporter : BackgroundService
|
|||||||
private readonly TimeSpan _pollInterval;
|
private readonly TimeSpan _pollInterval;
|
||||||
private readonly TimeSpan _staleAfter;
|
private readonly TimeSpan _staleAfter;
|
||||||
private readonly TimeProvider _time;
|
private readonly TimeProvider _time;
|
||||||
|
private readonly DateTimeOffset _pruneFrom;
|
||||||
|
|
||||||
/// <summary>Initializes a new instance of <see cref="SecretMirrorStatusReporter"/>.</summary>
|
/// <summary>Initializes a new instance of <see cref="SecretMirrorStatusReporter"/>.</summary>
|
||||||
/// <param name="collector">The site health collector receiving the statuses.</param>
|
/// <param name="collector">The site health collector receiving the statuses.</param>
|
||||||
@@ -58,13 +62,18 @@ public sealed class SecretMirrorStatusReporter : BackgroundService
|
|||||||
/// <see cref="SiteRuntimeOptions.SecretMirrorStatusStaleAfter"/> default (1 hour).
|
/// <see cref="SiteRuntimeOptions.SecretMirrorStatusStaleAfter"/> default (1 hour).
|
||||||
/// </param>
|
/// </param>
|
||||||
/// <param name="time">Clock used to age rows; defaults to <see cref="TimeProvider.System"/>.</param>
|
/// <param name="time">Clock used to age rows; defaults to <see cref="TimeProvider.System"/>.</param>
|
||||||
|
/// <param name="pruneGrace">
|
||||||
|
/// How long after construction nothing is pruned; defaults to zero. Under DI it is the
|
||||||
|
/// <see cref="SiteRuntimeOptions.SecretMirrorStatusRefreshInterval"/>.
|
||||||
|
/// </param>
|
||||||
public SecretMirrorStatusReporter(
|
public SecretMirrorStatusReporter(
|
||||||
ISiteHealthCollector collector,
|
ISiteHealthCollector collector,
|
||||||
SiteStorageService storage,
|
SiteStorageService storage,
|
||||||
ILogger<SecretMirrorStatusReporter> logger,
|
ILogger<SecretMirrorStatusReporter> logger,
|
||||||
TimeSpan? pollInterval = null,
|
TimeSpan? pollInterval = null,
|
||||||
TimeSpan? staleAfter = null,
|
TimeSpan? staleAfter = null,
|
||||||
TimeProvider? time = null)
|
TimeProvider? time = null,
|
||||||
|
TimeSpan? pruneGrace = null)
|
||||||
{
|
{
|
||||||
_collector = collector ?? throw new ArgumentNullException(nameof(collector));
|
_collector = collector ?? throw new ArgumentNullException(nameof(collector));
|
||||||
_storage = storage ?? throw new ArgumentNullException(nameof(storage));
|
_storage = storage ?? throw new ArgumentNullException(nameof(storage));
|
||||||
@@ -72,6 +81,7 @@ public sealed class SecretMirrorStatusReporter : BackgroundService
|
|||||||
_pollInterval = pollInterval ?? DefaultPollInterval;
|
_pollInterval = pollInterval ?? DefaultPollInterval;
|
||||||
_staleAfter = staleAfter ?? new SiteRuntimeOptions().SecretMirrorStatusStaleAfter;
|
_staleAfter = staleAfter ?? new SiteRuntimeOptions().SecretMirrorStatusStaleAfter;
|
||||||
_time = time ?? TimeProvider.System;
|
_time = time ?? TimeProvider.System;
|
||||||
|
_pruneFrom = _time.GetUtcNow() + (pruneGrace ?? TimeSpan.Zero);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
@@ -105,11 +115,12 @@ public sealed class SecretMirrorStatusReporter : BackgroundService
|
|||||||
try
|
try
|
||||||
{
|
{
|
||||||
var statuses = await _storage.GetSecretMirrorStatusesAsync().ConfigureAwait(false);
|
var statuses = await _storage.GetSecretMirrorStatusesAsync().ConfigureAwait(false);
|
||||||
var cutoff = _time.GetUtcNow() - _staleAfter;
|
var now = _time.GetUtcNow();
|
||||||
|
var cutoff = now - _staleAfter;
|
||||||
var live = new List<SecretMirrorNodeStatus>(statuses.Count);
|
var live = new List<SecretMirrorNodeStatus>(statuses.Count);
|
||||||
foreach (var status in statuses)
|
foreach (var status in statuses)
|
||||||
{
|
{
|
||||||
if (status.CheckedAtUtc >= cutoff)
|
if (status.CheckedAtUtc >= cutoff || now < _pruneFrom)
|
||||||
{
|
{
|
||||||
live.Add(status);
|
live.Add(status);
|
||||||
continue;
|
continue;
|
||||||
|
|||||||
@@ -147,7 +147,8 @@ public static class ServiceCollectionExtensions
|
|||||||
sp.GetRequiredService<SiteStorageService>(),
|
sp.GetRequiredService<SiteStorageService>(),
|
||||||
sp.GetRequiredService<ILogger<SecretMirrorStatusReporter>>(),
|
sp.GetRequiredService<ILogger<SecretMirrorStatusReporter>>(),
|
||||||
staleAfter: sp.GetRequiredService<IOptions<SiteRuntimeOptions>>().Value.SecretMirrorStatusStaleAfter,
|
staleAfter: sp.GetRequiredService<IOptions<SiteRuntimeOptions>>().Value.SecretMirrorStatusStaleAfter,
|
||||||
time: sp.GetRequiredService<TimeProvider>()));
|
time: sp.GetRequiredService<TimeProvider>(),
|
||||||
|
pruneGrace: sp.GetRequiredService<IOptions<SiteRuntimeOptions>>().Value.SecretMirrorStatusRefreshInterval));
|
||||||
|
|
||||||
return services;
|
return services;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -715,6 +715,26 @@ public class DeployedSecretsMirrorTests : IDisposable
|
|||||||
Assert.Equal(time.Now, Assert.Single(await storage.GetSecretMirrorStatusesAsync()).CheckedAtUtc);
|
Assert.Equal(time.Now, Assert.Single(await storage.GetSecretMirrorStatusesAsync()).CheckedAtUtc);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Status_RowDeletedByAPeerPrune_IsRewrittenOnTheNextPass()
|
||||||
|
{
|
||||||
|
// Rig finding: when both nodes restart together, the peer's reporter can prune this
|
||||||
|
// node's old row concurrently with this node's fresh write, and the replicated delete
|
||||||
|
// wins. The mirror notices its row is gone and writes it again.
|
||||||
|
var time = new FakeTimeProvider(new DateTimeOffset(2026, 9, 28, 6, 0, 0, TimeSpan.Zero));
|
||||||
|
var (mirror, storage, _, _, _) = await CreateAsync(time: time, nodeIdentity: new FixedNodeIdentity("node-b"));
|
||||||
|
await mirror.ReconcileAsync(CancellationToken.None);
|
||||||
|
var written = Assert.Single(await storage.GetSecretMirrorStatusesAsync()).CheckedAtUtc;
|
||||||
|
Assert.True(await storage.DeleteSecretMirrorStatusIfUnchangedAsync("node-b", written));
|
||||||
|
|
||||||
|
time.Now = time.Now.AddSeconds(15);
|
||||||
|
await mirror.ReconcileAsync(CancellationToken.None);
|
||||||
|
|
||||||
|
var status = Assert.Single(await storage.GetSecretMirrorStatusesAsync());
|
||||||
|
Assert.Equal("node-b", status.NodeName);
|
||||||
|
Assert.Equal(time.Now, status.CheckedAtUtc);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task Status_FailureSetChange_RewritesTheRow_AndClearsWhenHealed()
|
public async Task Status_FailureSetChange_RewritesTheRow_AndClearsWhenHealed()
|
||||||
{
|
{
|
||||||
|
|||||||
+27
@@ -130,6 +130,27 @@ public class SecretMirrorStatusReporterTests : IAsyncLifetime, IDisposable
|
|||||||
Assert.Single(await _storage.GetSecretMirrorStatusesAsync());
|
Assert.Single(await _storage.GetSecretMirrorStatusesAsync());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Probe_DuringTheStartUpGrace_PrunesNothing_ThenPrunesAfterIt()
|
||||||
|
{
|
||||||
|
var time = new MutableTime(new DateTimeOffset(2026, 9, 29, 12, 0, 0, TimeSpan.Zero));
|
||||||
|
await _storage.SetSecretMirrorStatusAsync("node-b", [], time.Now.AddHours(-2));
|
||||||
|
var collector = new SiteHealthCollector();
|
||||||
|
var reporter = new SecretMirrorStatusReporter(
|
||||||
|
collector, _storage, NullLogger<SecretMirrorStatusReporter>.Instance,
|
||||||
|
staleAfter: TimeSpan.FromHours(1), time: time, pruneGrace: TimeSpan.FromMinutes(10));
|
||||||
|
|
||||||
|
time.Now = time.Now.AddMinutes(9);
|
||||||
|
await reporter.ProbeAsync();
|
||||||
|
Assert.Single(collector.CollectReport("site-a").SecretMirrorStatuses!);
|
||||||
|
Assert.Single(await _storage.GetSecretMirrorStatusesAsync());
|
||||||
|
|
||||||
|
time.Now = time.Now.AddMinutes(1);
|
||||||
|
await reporter.ProbeAsync();
|
||||||
|
Assert.Empty(collector.CollectReport("site-a").SecretMirrorStatuses!);
|
||||||
|
Assert.Empty(await _storage.GetSecretMirrorStatusesAsync());
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task ConditionalDelete_DoesNotRemoveARowTheNodeRewroteAfterTheRead()
|
public async Task ConditionalDelete_DoesNotRemoveARowTheNodeRewroteAfterTheRead()
|
||||||
{
|
{
|
||||||
@@ -145,6 +166,12 @@ public class SecretMirrorStatusReporterTests : IAsyncLifetime, IDisposable
|
|||||||
Assert.Empty(await _storage.GetSecretMirrorStatusesAsync());
|
Assert.Empty(await _storage.GetSecretMirrorStatusesAsync());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private sealed class MutableTime(DateTimeOffset now) : TimeProvider
|
||||||
|
{
|
||||||
|
public DateTimeOffset Now { get; set; } = now;
|
||||||
|
public override DateTimeOffset GetUtcNow() => Now;
|
||||||
|
}
|
||||||
|
|
||||||
private sealed class FixedTime(DateTimeOffset now) : TimeProvider
|
private sealed class FixedTime(DateTimeOffset now) : TimeProvider
|
||||||
{
|
{
|
||||||
public override DateTimeOffset GetUtcNow() => now;
|
public override DateTimeOffset GetUtcNow() => now;
|
||||||
|
|||||||
Reference in New Issue
Block a user