docs+test(closeout): final-review reservations — stale ACL prose, worker test gaps, config sample fix
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Successful in 1m14s
ci / java (push) Successful in 2m10s
ci / portable (push) Successful in 8m31s

This commit is contained in:
Joseph Doherty
2026-08-17 05:23:23 -04:00
parent f5a58d884b
commit b621d692d0
18 changed files with 167 additions and 46 deletions
@@ -40,11 +40,21 @@ public interface IGatewayAlarmService
/// <summary>
/// True when the worker's most recent reconcile fetch hit the provider's
/// per-fetch cap, so <see cref="CurrentAlarms"/> may be missing active
/// alarms. The monitor is otherwise healthy — this is not a fault, it is
/// a completeness caveat, which is why it is separate from
/// per-fetch cap, so the active-alarm set may be missing alarms. The
/// monitor is otherwise healthy — this is not a fault, it is a
/// completeness caveat, which is why it is separate from
/// <see cref="State"/> and <see cref="LastError"/>. Cleared by the first
/// reconcile whose fetch comes back under the cap.
/// <para>
/// Read it as "as of the last full reconcile, the fetch was capped", not as
/// a property of a particular <see cref="CurrentAlarms"/> array: the two are
/// separate reads, and live transitions keep moving the cached set between
/// reconciles. A consumer that reads both — the dashboard poll does — can
/// therefore straddle a reconcile, in which case its caveat describes the
/// adjacent generation and the banner is at worst one poll stale. That is
/// the intended granularity for a completeness hint; pairing them exactly
/// would need a combined accessor this seam deliberately does not have.
/// </para>
/// </summary>
bool SnapshotTruncated { get; }
@@ -20,9 +20,12 @@ namespace ZB.MOM.WW.MxGateway.Server.Dashboard.Hubs;
/// values are stripped from a redacted copy of the event before it reaches any
/// dashboard client. The source <see cref="MxEvent"/> is shared with the gRPC
/// event path and the reconnect replay ring, so it is never mutated in place —
/// the redaction is applied to a deep clone. This closes the value-leak seam at
/// the mirror independently of the still-outstanding per-session hub ACL
/// (see <see cref="EventsHub"/>).
/// the redaction is applied to a deep clone. This is the second of two
/// independent layers: <see cref="IDashboardSessionAcl"/> decides at the
/// subscribe seam <em>which</em> sessions a caller may observe at all (see
/// <see cref="EventsHub"/>), while the redaction decides what a permitted
/// subscriber sees — so the value-leak seam stays closed whatever the ACL
/// admits.
/// </remarks>
/// <param name="hubContext">Hub context used to send to the session's group.</param>
/// <param name="viewerRegistry">
@@ -23,9 +23,12 @@ public interface IDashboardSessionAcl
/// session identified by <paramref name="sessionId"/>.
/// </summary>
/// <param name="principal">
/// The dashboard caller. <see langword="null"/>, unauthenticated, or claim-less
/// principals (including the anonymous-localhost path) are treated as Viewers
/// holding an empty tag grant.
/// The dashboard caller. <see langword="null"/> is denied outright — there is no
/// caller to grant tags to, so it never reaches the untagged-session branch and is
/// refused even under <c>UntaggedSessionVisibility=AllViewers</c>. An
/// unauthenticated or claim-less principal (the anonymous-localhost path included)
/// is a Viewer holding an empty tag grant, which denies every tagged session but
/// still follows that branch.
/// </param>
/// <param name="sessionId">Session id the caller wants to observe.</param>
/// <returns><see langword="true"/> when the caller may observe the session; otherwise <see langword="false"/>.</returns>
@@ -1000,6 +1000,16 @@ public sealed class WorkerFrameProtocolTests
/// two passes over the same stream, and a double-release would either do that or throw
/// <see cref="SemaphoreFullException"/> out of a later drain. Contiguous 1..N sequences with no
/// duplicates and no trailing bytes is the observable form of both.
/// <para>
/// The contention has to be real, so the stream is gated rather than a plain
/// <see cref="MemoryStream"/>: against a synchronously-completing stream each call finishes its own
/// drain before the next one starts, and neither a lost lock race nor a detached acquisition ever
/// happens. Gating write 1 parks the drainer while all <c>2 * perClass</c> frames are queued, so
/// every one of those callers provably loses the race; gating the pass's first event write —
/// write 1 plus the <c>perClass</c> control frames — stops the pass with the control run flushed
/// and completed while the lock is still held, which is what makes the detached path deterministic
/// rather than merely likely: those callers can only have returned on their own completion.
/// </para>
/// </summary>
/// <returns>A task that represents the asynchronous operation.</returns>
[Fact]
@@ -1007,24 +1017,46 @@ public sealed class WorkerFrameProtocolTests
{
const int perClass = 40;
WorkerFrameProtocolOptions options = CreateOptions();
using MemoryStream stream = new();
using GatedWriteStream stream = new(secondGateWriteIndex: perClass + 2);
WorkerFrameWriter writer = new(stream, options);
Task[] writes = new Task[perClass * 2];
// The drainer: it takes the lock, then parks inside its own write with the lock held.
Task drainer = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control);
await AwaitWithTimeoutAsync(stream.FirstWriteStarted);
// Queued against a held lock, so all 2 * perClass callers contend and all of them lose: each
// one's frame is written by the drainer's pass, never by its own.
Task[] controlWrites = new Task[perClass];
Task[] eventWrites = new Task[perClass];
for (int index = 0; index < perClass; index++)
{
writes[index * 2] = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control);
writes[(index * 2) + 1] = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event);
controlWrites[index] = writer.WriteAsync(CreateGatewayHelloEnvelope(), WorkerFrameWritePriority.Control);
eventWrites[index] = writer.WriteAsync(CreateEventEnvelope(), WorkerFrameWritePriority.Event);
}
await AwaitWithTimeoutAsync(Task.WhenAll(writes));
Assert.All(controlWrites, write => Assert.False(write.IsCompleted));
Assert.All(eventWrites, write => Assert.False(write.IsCompleted));
// A detached acquisition drains whatever it finds and releases; this write goes through the
// same lock afterwards, so it can only succeed if the lock was left in a usable state.
stream.ReleaseFirstWrite();
await AwaitWithTimeoutAsync(stream.SecondGateWriteStarted);
// The boundary flush delivered every control frame, and the drainer is now parked on the first
// event write — so the lock cannot be free. Each of these callers therefore returned on its own
// completion with a live acquisition behind it: the detached path, taken perClass times.
await AwaitWithTimeoutAsync(Task.WhenAll(controlWrites));
Assert.False(drainer.IsCompleted);
Assert.All(eventWrites, write => Assert.False(write.IsCompleted));
stream.ReleaseSecondGateWrite();
await AwaitWithTimeoutAsync(Task.WhenAll(eventWrites));
await AwaitWithTimeoutAsync(drainer);
// Every detached acquisition drains what it finds and releases; this write goes through the
// same lock afterwards, so it can only succeed if none of them stranded or double-released it.
await AwaitWithTimeoutAsync(
writer.WriteAsync(CreateShutdownAckEnvelope(), WorkerFrameWritePriority.Control));
const int total = (perClass * 2) + 1;
const int total = (perClass * 2) + 2;
int controlCount = 0;
int eventCount = 0;
stream.Position = 0;
@@ -1043,7 +1075,7 @@ public sealed class WorkerFrameProtocolTests
}
}
Assert.Equal(perClass + 1, controlCount);
Assert.Equal(perClass + 2, controlCount);
Assert.Equal(perClass, eventCount);
// No frame was written twice and none was left queued.
Assert.Equal(stream.Length, stream.Position);
@@ -330,6 +330,44 @@ public sealed class FailoverAlarmConsumerTests
Assert.Equal(22, sut.AcknowledgeByName("a", "p", "g", "c", "n", "node", "dom", "full"));
}
/// <summary>
/// Proves that the snapshot truncation verdict is read from whichever child
/// is currently active, not cached from the primary: a capped primary reports
/// <see langword="true"/>, and after failover the standby's own verdict
/// replaces it. The verdict drives the dashboard's completeness caveat, so a
/// stale one would either keep a banner on screen for a feed that is now
/// complete or, worse, clear it for one that is not.
/// </summary>
[Fact]
public void SnapshotActiveAlarms_TruncationVerdictComesFromActiveChild()
{
FlakyPrimary primary = new FlakyPrimary { ThrowOnPoll = false, SnapshotTruncated = true };
StubStandby standby = new StubStandby { SnapshotTruncated = false };
FailoverSettings settings = new FailoverSettings(threshold: 1, probeIntervalSeconds: 0, stableProbes: 1);
using FailoverAlarmConsumer sut = new FailoverAlarmConsumer(primary, standby, settings);
sut.Subscribe(@"\\HOST\Galaxy!Area");
Assert.Equal(AlarmProviderMode.Alarmmgr, sut.Mode);
// Active = Primary → the primary's capped fetch surfaces.
_ = sut.SnapshotActiveAlarms(out bool truncatedOnPrimary);
Assert.True(truncatedOnPrimary);
// Force a failover by failing the primary past threshold.
primary.ThrowOnPoll = true;
sut.PollOnce(); // threshold=1 → switch to Subtag
Assert.Equal(AlarmProviderMode.Subtag, sut.Mode);
// Active = Standby → its own verdict, not the primary's leftover true.
_ = sut.SnapshotActiveAlarms(out bool truncatedOnStandby);
Assert.False(truncatedOnStandby);
// And the standby really is the source: flip its verdict and the answer follows.
standby.SnapshotTruncated = true;
_ = sut.SnapshotActiveAlarms(out bool truncatedAfterStandbyCaps);
Assert.True(truncatedAfterStandbyCaps);
}
/// <summary>
/// Proves that an intermittent failure during failback probing resets the
/// clean-probe counter to zero, requiring a fresh unbroken run of
@@ -168,13 +168,20 @@ public sealed class WorkerPipeSession
// Closing the transport is what actually ends a pipe read parked in the kernel: on net48
// NamedPipeClientStream.ReadAsync ignores its CancellationToken, so the message loop's
// cancellation can never reach one (WRK-31). It is deliberately the LAST teardown step,
// because in the ordinary case every frame this session will ever write has completed by
// the time control reaches here: WorkerFrameWriter.WriteAsync signals only after the
// frame is written AND flushed, and each exit path awaits its final write before
// unwinding — the shutdown ack and shutdown-timeout fault inside the loop's dispatch, the
// event-drain and oversized-event faults inside the drain task the loop awaits, the
// watchdog fault inside the heartbeat task the loop awaits, and the handshake fault
// inside CompleteStartupHandshakeAsync's catch.
// because in the ordinary case every frame this session will ever write has been written
// AND flushed by the time control reaches here: WorkerFrameWriter.WriteAsync signals only
// after both, and each exit path awaits its final write before unwinding — the shutdown
// ack and shutdown-timeout fault inside the loop's dispatch, the event-drain and
// oversized-event faults inside the drain task the loop awaits, the watchdog fault inside
// the heartbeat task the loop awaits, and the handshake fault inside
// CompleteStartupHandshakeAsync's catch.
//
// That is a statement about frames, not about the writer being idle. A caller that
// returned on its own completion while another drainer held the write lock leaves a
// detached lock acquisition behind (WorkerFrameWriter.DetachLockWait), so a drain pass can
// still be scheduled after every caller has unwound. It is harmless here — the queues are
// empty by then, and a pass with nothing to dequeue writes and flushes nothing — but the
// invariant to rely on is "no frame is left undelivered", not "no writer work remains".
//
// "Ordinary" is the honest word, not "always": the loop's wait on the heartbeat and
// drain tasks is budgeted (BackgroundTaskStopTimeout), and a stream write is genuinely
@@ -77,10 +77,15 @@ public interface IAlarmCommandHandler : IDisposable
/// rather than read from a separate property, both so the pair comes from
/// one atomic consumer read and because it is the only carrier left once
/// <paramref name="alarmFilterPrefix"/> (or an empty galaxy) filters the
/// records down to none. <see langword="false"/> when there is no active
/// subscription: no fetch has happened, so nothing is capped.
/// records down to none. Never assigned when there is no active
/// subscription — that case throws rather than reporting an empty,
/// never-capped set, so a query issued before <c>SubscribeAlarms</c> is a
/// caller error and not a silent all-clear.
/// </param>
/// <returns>The currently active alarms matching the filter.</returns>
/// <exception cref="InvalidOperationException">
/// Thrown when there is no active subscription.
/// </exception>
IReadOnlyList<ActiveAlarmSnapshot> QueryActive(string? alarmFilterPrefix, out bool snapshotTruncated);
/// <summary>