Both defects were found by #491's live gate against a real HistorianGateway and filed rather than fixed at the time, because each one's fix is a design decision rather than a patch. #537 — both nodes of a redundant pair captured every value, so the historian received duplicate live samples for every historized tag (measured: the gateway's write counter advanced twice per value and halved when one central was stopped). The recorder now reads a capture gate per value, fed from the new PrimaryGatePolicy.ShouldRecordHistorizedValues through IHistorizationRoleView — a third verdict on the same RedundancyRoleView object, registered unconditionally on driver nodes rather than alongside the alarm section's IRedundancyRoleView (which only exists when that section is enabled; piggybacking is the #511 registration accident). Two things are deliberate and easy to get backwards: * It gates the CAPTURE, not the drain — the opposite of the alarm sink. That queue is in LocalDb and replicates, so the Secondary already holds the Primary's rows. The historization outbox is a node-local FasterLog replicating to nobody, so a drain-only gate would leave the standby appending forever to a queue no one empties: unbounded disk, then a flood of stale duplicates the moment it is promoted. Entries a node already captured are still drained by that node. * The peer it stands down for must RECORD THE SAME REFS, not merely hold the same rows — a Primary in this node's own per-cluster mesh. A node carrying no cluster-scoped role cannot establish that and never stands down: under a fleet-wide election the elected Primary is usually running a different cluster's configuration, and deferring to it would end this cluster's historization silently, with the drain reporting success on an empty outbox forever. That is the scope mismatch that once suspended the alarm drain on every node of the rig. An unknown role captures, matching the alarm drain — a duplicate sample is recoverable, a hole in the trend is not. #538 — the drain writes every per-tag group and then acks all-or-nothing (deliberate: RemoveAsync truncates THROUGH an id, so partial acking could drop older unwritten entries). One permanently failing tag therefore pinned the FIFO head while every healthy tag in the batch was re-submitted to the historian on every pass, indefinitely — duplicate samples and an outbox that never truncated, observed live at 8.3 MB from a single unprovisionable tag. The drain now tracks consecutive failures per historian tag and abandons that tag's entries once it exceeds MaxWriteAttempts (new, default 5), so the head advances. The counter only advances on a pass where some OTHER tag wrote successfully: that is what separates "this tag is poison" from "the backend is down". A count-only quarantine looks identical on the poison-tag case and silently discards the whole backlog during an ordinary gateway restart, turning a duplicate-samples bug into a lost-history one. Abandonment is also deferred until every failing tag is over budget, so nothing is discarded on a pass that could not have advanced the head anyway. Each abandonment logs a Warning naming the tag; the total rides RecorderStatus.AbandonedEntries. Capacity also gains a non-zero default (500,000 entries) as the disk backstop for the one case the quarantine deliberately cannot resolve — a historian refusing every write, where retaining is correct. Set 0 to restore unbounded retention. Every new test was falsified by reverting the production change it guards, and each one reddens alone: * drop the backend-reachable evidence -> only A_backend_that_fails_every_tag_abandons_nothing * never abandon -> only A_permanently_failing_tag_is_abandoned_so_healthy_tags_stop_being_rewritten * latch the capture gate at construction -> only The_capture_gate_is_re_read_per_value... * treat any Primary as a scope peer -> only the two scope-mismatch cases Full suite green. (Four OpcUaClient.Browser failures during the run were the opc-plc fixture being down on the shared Docker host; brought it back up and they pass.) Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo
This commit is contained in:
Joseph Doherty
+243
-2
@@ -386,6 +386,227 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit
|
||||
mux.ExpectNoMsg(TimeSpan.FromMilliseconds(300)); // no PreStart re-register == no restart
|
||||
}
|
||||
|
||||
// ---------------- Gitea #538: one poison tag must not pin the FIFO head ----------------
|
||||
|
||||
/// <summary>
|
||||
/// A tag that keeps failing while other tags write successfully is abandoned, so the head
|
||||
/// advances and the HEALTHY tags stop being re-submitted (Gitea #538).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The bug: the ack is all-or-nothing over the peeked prefix, but every per-tag group is
|
||||
/// written before that decision. So one permanently-failing tag left the head pinned while the
|
||||
/// good tags' values were re-written to the historian on every drain pass, forever — duplicate
|
||||
/// samples, and an outbox that never truncated (observed live at 8.3 MB).
|
||||
/// <para>
|
||||
/// The load-bearing assertion is the BOUND on how many times the healthy tag was submitted,
|
||||
/// not that the outbox eventually empties. Without the fix the outbox never empties, but a
|
||||
/// test that only waits for empty would simply time out and look like a flake; asserting the
|
||||
/// re-write count states the actual defect.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task A_permanently_failing_tag_is_abandoned_so_healthy_tags_stop_being_rewritten()
|
||||
{
|
||||
var mux = CreateTestProbe();
|
||||
var writer = new FakeValueWriter { Succeed = true };
|
||||
writer.SetFailing("Bad.Tag", true);
|
||||
var outbox = new InMemoryOutbox();
|
||||
|
||||
const int maxAttempts = 2;
|
||||
IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props(
|
||||
mux.Ref, writer, outbox, historizedRefs: new[] { "Good.Tag", "Bad.Tag" },
|
||||
minBackoff: TimeSpan.FromMilliseconds(20),
|
||||
maxBackoff: TimeSpan.FromMilliseconds(50),
|
||||
drainInterval: TimeSpan.FromMilliseconds(20),
|
||||
maxWriteAttempts: maxAttempts));
|
||||
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Good.Tag", 1.0, DateTime.UtcNow));
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Bad.Tag", 2.0, DateTime.UtcNow));
|
||||
|
||||
// The head advances: both entries leave the outbox — the good one written, the bad one discarded.
|
||||
await AwaitAssertAsync(async () =>
|
||||
{
|
||||
var status = await rec.Ask<ContinuousHistorizationRecorder.RecorderStatus>(
|
||||
ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3));
|
||||
Assert.Equal(0, status.QueuedDepth);
|
||||
Assert.Equal(1, status.AbandonedEntries);
|
||||
}, TimeSpan.FromSeconds(10));
|
||||
|
||||
// And the healthy tag was submitted a BOUNDED number of times — once per blocked pass plus the
|
||||
// pass that unblocked, never indefinitely. Snapshot it, let several more drain intervals elapse,
|
||||
// and prove it has stopped growing: an unbounded re-write would still be climbing.
|
||||
int rewrites = writer.Snapshot().Count(w => w.Tag == "Good.Tag");
|
||||
Assert.InRange(rewrites, 1, maxAttempts + 1);
|
||||
await Task.Delay(TimeSpan.FromMilliseconds(300));
|
||||
Assert.Equal(rewrites, writer.Snapshot().Count(w => w.Tag == "Good.Tag"));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// When NOTHING writes, nothing is abandoned — the outage path must still retain everything.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// This is the falsification control for the test above, and it is the reason the fix keys on
|
||||
/// "another tag succeeded this pass" rather than on a bare per-tag failure count. A count-only
|
||||
/// quarantine looks identical on the poison-tag case and silently DISCARDS the whole backlog
|
||||
/// during an ordinary gateway restart — turning a bug about duplicate samples into one about
|
||||
/// lost history. Delete the <c>succeeded == 0</c> guard and this goes red while the other stays
|
||||
/// green.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task A_backend_that_fails_every_tag_abandons_nothing()
|
||||
{
|
||||
var mux = CreateTestProbe();
|
||||
var writer = new FakeValueWriter { Succeed = false }; // the whole backend is down
|
||||
var outbox = new InMemoryOutbox();
|
||||
|
||||
IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props(
|
||||
mux.Ref, writer, outbox, historizedRefs: new[] { "A", "B" },
|
||||
minBackoff: TimeSpan.FromMilliseconds(20),
|
||||
maxBackoff: TimeSpan.FromMilliseconds(50),
|
||||
drainInterval: TimeSpan.FromMilliseconds(20),
|
||||
maxWriteAttempts: 1)); // the tightest budget there is — still must not discard
|
||||
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("A", 1.0, DateTime.UtcNow));
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("B", 2.0, DateTime.UtcNow));
|
||||
|
||||
// Wait until the drain has retried well past the attempt budget, so "nothing abandoned" is a
|
||||
// statement about a recorder that had every chance to abandon — not about one that hasn't run.
|
||||
await AwaitAssertAsync(
|
||||
() =>
|
||||
{
|
||||
Assert.True(writer.CallCount >= 6, "the drain must have retried several times");
|
||||
return Task.CompletedTask;
|
||||
},
|
||||
TimeSpan.FromSeconds(10));
|
||||
|
||||
var status = await rec.Ask<ContinuousHistorizationRecorder.RecorderStatus>(
|
||||
ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3));
|
||||
Assert.Equal(0, status.AbandonedEntries);
|
||||
Assert.Equal(2, status.QueuedDepth); // both values still durably queued for when it returns
|
||||
}
|
||||
|
||||
/// <summary>A tag that recovers inside its budget is never abandoned, and its counter resets.</summary>
|
||||
[Fact]
|
||||
public async Task A_tag_that_recovers_within_its_budget_is_not_abandoned()
|
||||
{
|
||||
var mux = CreateTestProbe();
|
||||
var writer = new FakeValueWriter { Succeed = true };
|
||||
writer.SetFailing("Flaky.Tag", true);
|
||||
var outbox = new InMemoryOutbox();
|
||||
|
||||
IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props(
|
||||
mux.Ref, writer, outbox, historizedRefs: new[] { "Good.Tag", "Flaky.Tag" },
|
||||
minBackoff: TimeSpan.FromMilliseconds(20),
|
||||
maxBackoff: TimeSpan.FromMilliseconds(50),
|
||||
drainInterval: TimeSpan.FromMilliseconds(20),
|
||||
maxWriteAttempts: 50)); // a budget the test will not exhaust
|
||||
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Good.Tag", 1.0, DateTime.UtcNow));
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Flaky.Tag", 2.0, DateTime.UtcNow));
|
||||
|
||||
// It has genuinely failed a few times before recovering — otherwise this proves nothing.
|
||||
await AwaitAssertAsync(
|
||||
() =>
|
||||
{
|
||||
Assert.True(writer.Snapshot().Count(w => w.Tag == "Flaky.Tag") >= 2);
|
||||
return Task.CompletedTask;
|
||||
},
|
||||
TimeSpan.FromSeconds(10));
|
||||
|
||||
writer.SetFailing("Flaky.Tag", false);
|
||||
|
||||
await AwaitAssertAsync(async () =>
|
||||
{
|
||||
var status = await rec.Ask<ContinuousHistorizationRecorder.RecorderStatus>(
|
||||
ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3));
|
||||
Assert.Equal(0, status.QueuedDepth);
|
||||
Assert.Equal(0, status.AbandonedEntries); // recovered, so nothing was ever discarded
|
||||
}, TimeSpan.FromSeconds(10));
|
||||
}
|
||||
|
||||
// ---------------- Gitea #537: only the recording Primary captures ----------------
|
||||
|
||||
/// <summary>
|
||||
/// A closed capture gate drops the value instead of appending it (Gitea #537).
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Both nodes of a redundant pair serve the same address space and see the same mux fan-out, so
|
||||
/// an ungated recorder writes every sample twice.
|
||||
/// <para>
|
||||
/// The load-bearing assertion is <c>SuppressedNotPrimary == 1</c>, a POSITIVE observation
|
||||
/// that the value arrived and was refused. "The outbox is empty" and "the writer saw
|
||||
/// nothing" are both true before the recorder has processed anything at all, so a test built
|
||||
/// on them alone passes against a recorder that never ran.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task A_closed_capture_gate_suppresses_the_value()
|
||||
{
|
||||
var mux = CreateTestProbe();
|
||||
var writer = new FakeValueWriter();
|
||||
var outbox = new InMemoryOutbox();
|
||||
|
||||
IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props(
|
||||
mux.Ref, writer, outbox, historizedRefs: new[] { "Pump1.Temp" },
|
||||
captureGate: () => false));
|
||||
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Pump1.Temp", 42.0, DateTime.UtcNow));
|
||||
|
||||
await AwaitAssertAsync(async () =>
|
||||
{
|
||||
var status = await rec.Ask<ContinuousHistorizationRecorder.RecorderStatus>(
|
||||
ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3));
|
||||
Assert.Equal(1, status.SuppressedNotPrimary);
|
||||
Assert.Equal(0, status.TotalRecorded);
|
||||
});
|
||||
Assert.Empty(writer.Snapshot());
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// The gate is read per value, so a promotion resumes capture on the very next sample.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Latching the verdict at construction (or on the first value) would compile, pass the
|
||||
/// closed-gate test above, and leave a newly promoted node recording nothing until it restarted
|
||||
/// — a silent, permanent gap that only a failover reveals. Flipping the gate mid-test is what
|
||||
/// distinguishes the two implementations.
|
||||
/// </remarks>
|
||||
[Fact]
|
||||
public async Task The_capture_gate_is_re_read_per_value_so_promotion_resumes_capture()
|
||||
{
|
||||
var mux = CreateTestProbe();
|
||||
var writer = new FakeValueWriter { Succeed = true };
|
||||
var outbox = new InMemoryOutbox();
|
||||
var open = false;
|
||||
|
||||
IActorRef rec = Sys.ActorOf(ContinuousHistorizationRecorder.Props(
|
||||
mux.Ref, writer, outbox, historizedRefs: new[] { "Pump1.Temp" },
|
||||
drainInterval: TimeSpan.FromMilliseconds(20),
|
||||
captureGate: () => Volatile.Read(ref open)));
|
||||
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Pump1.Temp", 11.0, DateTime.UtcNow));
|
||||
await AwaitAssertAsync(async () =>
|
||||
{
|
||||
var suppressed = await rec.Ask<ContinuousHistorizationRecorder.RecorderStatus>(
|
||||
ContinuousHistorizationRecorder.GetStatus.Instance, TimeSpan.FromSeconds(3));
|
||||
Assert.Equal(1, suppressed.SuppressedNotPrimary);
|
||||
});
|
||||
|
||||
Volatile.Write(ref open, true); // promoted
|
||||
rec.Tell(new VirtualTagActor.DependencyValueChanged("Pump1.Temp", 22.0, DateTime.UtcNow));
|
||||
|
||||
await AwaitAssertAsync(() =>
|
||||
{
|
||||
// Only the post-promotion value: the suppressed one was never appended, so it cannot appear
|
||||
// late. Asserting on the CONTENT rather than the count is what rules out a gate that simply
|
||||
// buffered the first value and released it.
|
||||
var written = writer.Snapshot();
|
||||
Assert.Contains(written, w => w.Value == 22.0);
|
||||
Assert.DoesNotContain(written, w => w.Value == 11.0);
|
||||
return Task.CompletedTask;
|
||||
}, TimeSpan.FromSeconds(10));
|
||||
}
|
||||
|
||||
/// <summary>In-memory <see cref="IHistorianValueWriter"/> double: records every value written and
|
||||
/// returns <see cref="Succeed"/> as the ack. Thread-safe — the recorder drains off the actor thread.</summary>
|
||||
private sealed class FakeValueWriter : IHistorianValueWriter
|
||||
@@ -401,6 +622,25 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit
|
||||
/// leaving the plain <see cref="Succeed"/> behaviour (FailFirstN==0) untouched.</summary>
|
||||
public int FailFirstN { get; init; }
|
||||
|
||||
/// <summary>Historian tags whose writes always fail, whatever <see cref="Succeed"/> says — the
|
||||
/// poison-tag shape Gitea #538 is about (typically a tag the historian has no definition for,
|
||||
/// because EnsureTags only provisions newly-ADDED historized tags). Other tags in the same drain
|
||||
/// pass still succeed, and that contrast is the evidence the recorder keys on.</summary>
|
||||
private readonly HashSet<string> _failTags = new(StringComparer.Ordinal);
|
||||
|
||||
/// <summary>Starts or stops failing one tag. Callable mid-test so a case can prove a tag that
|
||||
/// recovers within its budget is never abandoned.</summary>
|
||||
/// <param name="tag">The historian tag.</param>
|
||||
/// <param name="failing">Whether writes for it should fail.</param>
|
||||
public void SetFailing(string tag, bool failing)
|
||||
{
|
||||
lock (_gate)
|
||||
{
|
||||
if (failing) _failTags.Add(tag);
|
||||
else _failTags.Remove(tag);
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Lifetime count of <see cref="WriteLiveValuesAsync"/> invocations (proves a retry ran).</summary>
|
||||
public int CallCount
|
||||
{
|
||||
@@ -432,8 +672,9 @@ public sealed class ContinuousHistorizationRecorderTests : TestKit
|
||||
_written.Add(new WrittenValue(tag, v.Value, v.Quality, v.TimestampUtc));
|
||||
}
|
||||
|
||||
// Fail the first FailFirstN calls; thereafter honour Succeed.
|
||||
bool result = _calls > FailFirstN && Succeed;
|
||||
// Fail the first FailFirstN calls; thereafter honour Succeed — unless this specific tag
|
||||
// is currently marked failing, which overrides both.
|
||||
bool result = _calls > FailFirstN && Succeed && !_failTags.Contains(tag);
|
||||
return Task.FromResult(result);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user