fix(core): resolve Medium code-review finding (Core-006)
BuildAddressSpaceAsync now checks _disposed (throws ObjectDisposedException) and tears down the previous alarm forwarder + clears the sink registry before re-walking, so a Galaxy-redeploy rebuild does not leak the old forwarder and double-deliver alarm transitions. Three regression tests added: double-build does not double-fire, sink count is correct after rebuild, and post-dispose call throws. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -36,12 +36,25 @@ public class GenericDriverNodeManager(IDriver driver) : IDisposable
|
|||||||
/// Populates the address space by streaming nodes from the driver into the supplied builder,
|
/// Populates the address space by streaming nodes from the driver into the supplied builder,
|
||||||
/// wraps the builder so alarm-condition sinks are captured, subscribes to the driver's
|
/// wraps the builder so alarm-condition sinks are captured, subscribes to the driver's
|
||||||
/// alarm event stream, and routes each transition to the matching sink by <c>SourceNodeId</c>.
|
/// alarm event stream, and routes each transition to the matching sink by <c>SourceNodeId</c>.
|
||||||
/// Driver exceptions are isolated per decision #12 — the driver's subtree is marked Faulted,
|
/// If called a second time (e.g. Galaxy redeploy via <c>IRediscoverable.OnRediscoveryNeeded</c>)
|
||||||
/// but other drivers remain available.
|
/// the previous alarm subscription is torn down and the sink registry is cleared before
|
||||||
|
/// re-walking, preventing double delivery of alarm transitions.
|
||||||
|
/// Exception isolation (marking the driver's subtree Faulted) is the caller's responsibility —
|
||||||
|
/// exceptions from <see cref="ITagDiscovery.DiscoverAsync"/> propagate to the caller.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public async Task BuildAddressSpaceAsync(IAddressSpaceBuilder builder, CancellationToken ct)
|
public async Task BuildAddressSpaceAsync(IAddressSpaceBuilder builder, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentNullException.ThrowIfNull(builder);
|
ArgumentNullException.ThrowIfNull(builder);
|
||||||
|
ObjectDisposedException.ThrowIf(_disposed, this);
|
||||||
|
|
||||||
|
// Tear down any previous alarm subscription before re-walking so a second call (e.g. on
|
||||||
|
// Galaxy redeploy) does not leave the old forwarder subscribed and double-fire events.
|
||||||
|
if (_alarmForwarder is not null && Driver is IAlarmSource existingSource)
|
||||||
|
{
|
||||||
|
existingSource.OnAlarmEvent -= _alarmForwarder;
|
||||||
|
_alarmForwarder = null;
|
||||||
|
}
|
||||||
|
_alarmSinks.Clear();
|
||||||
|
|
||||||
if (Driver is not ITagDiscovery discovery)
|
if (Driver is not ITagDiscovery discovery)
|
||||||
throw new NotSupportedException($"Driver '{Driver.DriverInstanceId}' does not implement ITagDiscovery.");
|
throw new NotSupportedException($"Driver '{Driver.DriverInstanceId}' does not implement ITagDiscovery.");
|
||||||
|
|||||||
@@ -88,6 +88,61 @@ public sealed class GenericDriverNodeManagerTests
|
|||||||
builder.Alarms["Tank.HiHi"].Received.Count.ShouldBe(0);
|
builder.Alarms["Tank.HiHi"].Received.Count.ShouldBe(0);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Core-006 regression: a second call to BuildAddressSpaceAsync (e.g. on Galaxy redeploy)
|
||||||
|
/// must unsubscribe the old alarm forwarder and clear the sink registry before re-walking,
|
||||||
|
/// so alarm transitions are not delivered twice.
|
||||||
|
/// </summary>
|
||||||
|
[Fact]
|
||||||
|
public async Task Second_BuildAddressSpaceAsync_Does_Not_Double_Fire_Alarms()
|
||||||
|
{
|
||||||
|
var driver = new FakeDriver();
|
||||||
|
var builder1 = new RecordingBuilder();
|
||||||
|
var builder2 = new RecordingBuilder();
|
||||||
|
using var nm = new GenericDriverNodeManager(driver);
|
||||||
|
|
||||||
|
await nm.BuildAddressSpaceAsync(builder1, CancellationToken.None);
|
||||||
|
await nm.BuildAddressSpaceAsync(builder2, CancellationToken.None); // redeploy
|
||||||
|
|
||||||
|
driver.RaiseAlarm(new AlarmEventArgs(
|
||||||
|
new FakeHandle("s1"), "Tank.HiHi", "c", "t", "m", AlarmSeverity.High, DateTime.UtcNow));
|
||||||
|
|
||||||
|
// Only the second builder's sink should have received the event.
|
||||||
|
builder2.Alarms["Tank.HiHi"].Received.Count.ShouldBe(1,
|
||||||
|
"second BuildAddressSpaceAsync must replace the subscription — not add to it");
|
||||||
|
|
||||||
|
// The first builder's sink should NOT have received it (old forwarder was detached).
|
||||||
|
builder1.Alarms.TryGetValue("Tank.HiHi", out var oldSink);
|
||||||
|
(oldSink?.Received.Count ?? 0).ShouldBe(0,
|
||||||
|
"the original alarm forwarder must be unsubscribed on the second build");
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task Second_BuildAddressSpaceAsync_Clears_Old_Sink_Registry()
|
||||||
|
{
|
||||||
|
var driver = new FakeDriver();
|
||||||
|
using var nm = new GenericDriverNodeManager(driver);
|
||||||
|
|
||||||
|
await nm.BuildAddressSpaceAsync(new RecordingBuilder(), CancellationToken.None);
|
||||||
|
var countAfterFirst = nm.TrackedAlarmSources.Count;
|
||||||
|
await nm.BuildAddressSpaceAsync(new RecordingBuilder(), CancellationToken.None);
|
||||||
|
var countAfterSecond = nm.TrackedAlarmSources.Count;
|
||||||
|
|
||||||
|
countAfterFirst.ShouldBe(2, "FakeDriver registers 2 alarm sources");
|
||||||
|
countAfterSecond.ShouldBe(2, "second build must re-register exactly the same sources, not accumulate");
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task BuildAddressSpaceAsync_After_Dispose_Throws_ObjectDisposedException()
|
||||||
|
{
|
||||||
|
var driver = new FakeDriver();
|
||||||
|
var nm = new GenericDriverNodeManager(driver);
|
||||||
|
nm.Dispose();
|
||||||
|
|
||||||
|
await Should.ThrowAsync<ObjectDisposedException>(() =>
|
||||||
|
nm.BuildAddressSpaceAsync(new RecordingBuilder(), CancellationToken.None));
|
||||||
|
}
|
||||||
|
|
||||||
// --- test doubles ---
|
// --- test doubles ---
|
||||||
|
|
||||||
private sealed class FakeDriver : IDriver, ITagDiscovery, IAlarmSource
|
private sealed class FakeDriver : IDriver, ITagDiscovery, IAlarmSource
|
||||||
|
|||||||
Reference in New Issue
Block a user