docs(worker): reviewer follow-up comments and tests from the remediation reviews
ci / nightly-windev (push) Has been skipped
ci / windows-x86 (push) Successful in 1m18s
ci / java (push) Successful in 2m11s
ci / portable (push) Successful in 11m5s

The worker-side half of the review tail. Tests and comments only — nothing here
changes worker behavior, and none of it compiles on the macOS tree (net48/x86),
so it was reviewed line by line against the already-windev-validated files.

- MxAccessHandleRegistryTests gains the multi-candidate case behind
  MxAccessSession.TryGetCachedReadFor's fall-through: one tag under two item
  handles, the lower registered-but-unadvised and the higher advised. Asserted
  at the registry rather than the session because the session's read path needs
  a live MXAccess COM instance; what the registry owes the scan is the stable
  ascending candidate order and a per-item-handle (not per-tag) advice index,
  and both are pinned here along with the fall-through contract in prose.
- A single adversarial lifecycle test — register, advise, re-register the same
  item handle under a new tag, unadvise, unregister the server — asserting every
  index agrees after each step. The individual transitions were already covered;
  what was not was that they compose, and a stale entry in any one index
  resurrects a handle MXAccess has already retired.
- StaWaitHelperTests.WaitForSignalOrMessages_PreSignalledHandle_ReturnsImmediately
  drains pending messages first, like the other two wait tests. Without it a
  stale message can end the wait instead of the handle, failing the
  signal-consumed post-condition for an unrelated reason.
- GatewayTesting.md records the two findings from the Task 24 windev gate:
  SecretsStorePathGuardTests.CreateBuilder_AcceptsSecretsStoreOutsideContentRoot_AndCreatesIt
  fails deterministically on Windows on main too (SQLite pooling holds secrets.db
  open across the cleanup's recursive delete; pre-existing, tracked separately),
  and the StaWaitHelper timing tests' flake signature on a loaded box is a
  message wake — the helper working as designed — not a broken wait.
This commit is contained in:
Joseph Doherty
2026-08-15 17:56:16 -04:00
parent dc2df628e3
commit f4b065b9f6
3 changed files with 109 additions and 0 deletions
@@ -256,6 +256,93 @@ public sealed class MxAccessHandleRegistryTests
Assert.True(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Plain));
}
/// <summary>
/// Pins the registry half of <c>MxAccessSession.TryGetCachedReadFor</c>'s fall-through
/// contract. That scan walks <see cref="MxAccessHandleRegistry.GetItemHandlesForDefinition"/>
/// in order and skips any candidate carrying neither a plain nor a supervisory advice,
/// because an added-but-unadvised item will never receive a fresh <c>OnDataChange</c> and so
/// can only serve a stale cache entry. The registry has to make that skip possible: the
/// duplicate registrations of one tag must come back in a stable ascending order, and the
/// advice index must answer per item handle rather than per tag. Asserted here rather than
/// on the session because the session's read path needs a live MXAccess COM instance.
/// </summary>
[Fact]
public void GetItemHandlesForDefinition_MultipleCandidates_AdviceIndexDiscriminatesTheAdvisedOne()
{
MxAccessHandleRegistry registry = new();
registry.RegisterServerHandle(serverHandle: 1, clientName: "client");
// Same tag under two item handles: 10 is added but never advised, 20 is advised. Registered
// out of order so the ascending-order guarantee is doing real work.
registry.RegisterItemHandle(1, itemHandle: 20, "Tank1.PV", string.Empty, hasItemContext: false);
registry.RegisterItemHandle(1, itemHandle: 10, "Tank1.PV", string.Empty, hasItemContext: false);
registry.RegisterAdviceHandle(1, itemHandle: 20, MxAccessAdviceKind.Plain);
IReadOnlyList<int> candidates = registry.GetItemHandlesForDefinition(1, "Tank1.PV");
Assert.Equal(new[] { 10, 20 }, candidates);
// The unadvised candidate is visited first and skipped; the advised one is the survivor.
Assert.False(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Plain));
Assert.False(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Supervisory));
Assert.True(registry.ContainsAdviceHandle(1, 20, MxAccessAdviceKind.Plain));
// Supervisory alone qualifies too, so a later advise on 10 makes it the first survivor.
registry.RegisterAdviceHandle(1, itemHandle: 10, MxAccessAdviceKind.Supervisory);
Assert.True(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Supervisory));
}
/// <summary>
/// One adversarial lifecycle run over every index at once: register, advise, re-register the
/// same item handle under a new tag, unadvise, then tear the server down. Each individual
/// transition is covered above; this pins that they compose — the reverse definition index,
/// the per-item advice index and the per-server removal index must agree after every step,
/// since a stale entry in any one of them resurrects a handle MXAccess has already retired.
/// </summary>
[Fact]
public void RegisterAdviseReregisterUnadviseUnregister_LeavesEveryIndexConsistent()
{
MxAccessHandleRegistry registry = new();
registry.RegisterServerHandle(serverHandle: 1, clientName: "client");
// Register.
registry.RegisterItemHandle(1, itemHandle: 10, "Tank1.PV", string.Empty, hasItemContext: false);
Assert.True(registry.ContainsItemHandle(1, 10));
Assert.Equal(new[] { 10 }, registry.GetItemHandlesForDefinition(1, "Tank1.PV"));
Assert.Empty(registry.AdviceHandles);
// Advise.
registry.RegisterAdviceHandle(1, itemHandle: 10, MxAccessAdviceKind.Plain);
Assert.True(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Plain));
Assert.Single(registry.AdviceHandles);
// Re-register the SAME item handle under a new tag. The advice is keyed on the item handle,
// not the tag, so it survives — but the old definition entry must not.
registry.RegisterItemHandle(1, itemHandle: 10, "Tank2.PV", string.Empty, hasItemContext: false);
Assert.Empty(registry.GetItemHandlesForDefinition(1, "Tank1.PV"));
Assert.Equal(new[] { 10 }, registry.GetItemHandlesForDefinition(1, "Tank2.PV"));
Assert.Single(registry.ItemHandles);
Assert.True(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Plain));
// Unadvise: the item stays registered and still resolves by its current tag.
registry.RemoveAdviceHandles(1, itemHandle: 10);
Assert.False(registry.ContainsAdviceHandle(1, 10, MxAccessAdviceKind.Plain));
Assert.Empty(registry.AdviceHandles);
Assert.True(registry.ContainsItemHandle(1, 10));
Assert.Equal(new[] { 10 }, registry.GetItemHandlesForDefinition(1, "Tank2.PV"));
// Unregister the server: every index drains, including the definition index the re-register
// rewrote.
registry.UnregisterServerHandle(1);
Assert.False(registry.ContainsServerHandle(1));
Assert.False(registry.ContainsItemHandle(1, 10));
Assert.Empty(registry.GetItemHandlesForDefinition(1, "Tank1.PV"));
Assert.Empty(registry.GetItemHandlesForDefinition(1, "Tank2.PV"));
Assert.Empty(registry.ServerHandles);
Assert.Empty(registry.ItemHandles);
Assert.Empty(registry.AdviceHandles);
}
private static List<TResult> Map<TSource, TResult>(
IReadOnlyList<TSource> source,
Func<TSource, TResult> selector)
@@ -90,6 +90,11 @@ public sealed class StaWaitHelperTests
{
using AutoResetEvent signal = new(initialState: true);
// Drained for the same reason as the other two waits: a stale message could end this wait
// instead of the handle, which would leave the signal un-consumed and fail the
// post-condition below for a reason that has nothing to do with pre-signalling.
new StaMessagePump().PumpPendingMessages();
Stopwatch elapsed = Stopwatch.StartNew();
StaWaitHelper.WaitForSignalOrMessages(signal, 30_000);
elapsed.Stop();