From 0dc2327eea320677b9704080f8631e58d9a18e2c Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 13:36:11 -0600 Subject: [PATCH 01/16] feat(device): surface background failures via ErrorOccurred and close the last silent-spin path Adds a device-level error event so read, parse, dispatch and per-frame decode failures are observable instead of silent, and closes the one remaining place the reader loop could spin forever with no data, no error and no status change. - IDevice.ErrorOccurred + DeviceErrorEventArgs / DeviceErrorSource - DaqifiDevice subscribes to the message consumer's ErrorOccurred (protobuf and text consumers), logs every failure, and raises it under a documented throttle (first occurrence immediate, then at most one per 5s per source+exception type, with the collapsed count reported) - DaqifiStreamingDevice keeps per-frame decode isolation but now counts failures (DecodeFailureCount, reset per streaming session) and raises the event - StreamMessageConsumer escalates a permanently unreadable stream instead of backing off silently forever Purely observational: nothing here changes stream behaviour, retry policy or ConnectionStatus. The transports keep sole ownership of declaring a link lost. closes #377 closes #394 closes #378 Co-Authored-By: Claude Opus 5 --- docs/DEVICE_INTERFACES.md | 78 ++++ .../ConnectionLossEscalationTests.cs | 355 +++++++++++++++ .../Device/DeviceErrorSurfaceTests.cs | 429 ++++++++++++++++++ .../Device/DeviceErrorThrottleTests.cs | 179 ++++++++ .../Firmware/FirmwareUpdateServiceTests.cs | 14 + .../Consumers/StreamMessageConsumer.cs | 12 +- src/Daqifi.Core/Device/DaqifiDevice.cs | 125 +++++ .../Device/DaqifiStreamingDevice.cs | 36 +- .../Device/DeviceErrorEventArgs.cs | 84 ++++ src/Daqifi.Core/Device/DeviceErrorSource.cs | 35 ++ src/Daqifi.Core/Device/DeviceErrorThrottle.cs | 172 +++++++ src/Daqifi.Core/Device/IDevice.cs | 11 + 12 files changed, 1526 insertions(+), 4 deletions(-) create mode 100644 src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs create mode 100644 src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs create mode 100644 src/Daqifi.Core.Tests/Device/DeviceErrorThrottleTests.cs create mode 100644 src/Daqifi.Core/Device/DeviceErrorEventArgs.cs create mode 100644 src/Daqifi.Core/Device/DeviceErrorSource.cs create mode 100644 src/Daqifi.Core/Device/DeviceErrorThrottle.cs diff --git a/docs/DEVICE_INTERFACES.md b/docs/DEVICE_INTERFACES.md index 4c234f61..28631fc2 100644 --- a/docs/DEVICE_INTERFACES.md +++ b/docs/DEVICE_INTERFACES.md @@ -50,6 +50,7 @@ The base interface for all DAQiFi devices, providing fundamental connection and - `Send(IOutboundMessage)` - Send commands to device - `StatusChanged` event - Connection status notifications - `MessageReceived` event - Incoming data notifications +- `ErrorOccurred` event - Background read/parse/decode failures (see [Error Surface](#error-surface)) ### IStreamingDevice @@ -490,6 +491,10 @@ device.StatusChanged += (_, e) => Custom transports can opt into the same escalation by implementing `ITransportHealthSink`; the reader and writer loops report every read/write outcome to a transport that does. +The failures that feed this escalation are also reported individually, as they happen, on +`IDevice.ErrorOccurred` — see [Error Surface](#error-surface). That event is diagnostics only; +`ConnectionStatus.Lost` remains the single signal that means "the connection is over". + ### Working with Device Metadata After initialization, device metadata is populated: @@ -770,6 +775,77 @@ device.SendFailed += (_, e) => A single failed write does not stop the queue: the producer keeps draining the remaining messages regardless of whether anything observes the failure. +## Error Surface + +Reading from the device and decoding its frames both happen on background threads, where an +exception has nobody to throw to. Those failures used to be invisible: a stream that could not be +read and a stream that could not be decoded both looked exactly like a device sending nothing. +`IDevice.ErrorOccurred` is the one place to answer *"why am I getting no samples"*. + +```csharp +device.ErrorOccurred += (_, e) => +{ + Console.WriteLine($"[{e.Source}] {e.Error.Message}"); + if (e.SuppressedCount > 0) + { + Console.WriteLine($" ...and {e.SuppressedCount} more like it since the last report"); + } +}; +``` + +`DeviceErrorEventArgs.Source` says which stage failed: + +| Source | What it means | +|---|---| +| `MessageConsumer` | A read from the transport stream failed, a frame could not be parsed, or a `MessageReceived` subscriber threw | +| `StreamDecode` | A streaming frame could not be decoded into channel samples. That frame is dropped; the stream continues | + +### Observational, never escalating + +Raising this event changes nothing about what the device does. There is no tear-down, no retry, no +`Status` change, and per-frame isolation is unchanged — a single malformed frame is still dropped on +its own without disturbing the stream. Deciding that a link is genuinely dead is a separate +mechanism with its own signal: `ConnectionStatus.Lost` on `StatusChanged` (see +[Detecting a dropped connection](#detecting-a-dropped-connection)). The two often fire together on a +real drop — the same failing reads feed both — but neither implies the other, and an `ErrorOccurred` +on its own is not a reason to reconnect. + +Every error is also written to the device's `ILogger` at warning level, so it stays visible with no +subscriber attached. + +### Throttle policy + +A systematic failure repeats at the frame rate, so raises are collapsed per **bucket**, where a +bucket is (`Source`, exception type): + +- The **first** occurrence in a bucket is always raised, immediately. +- After that, a bucket raises **at most once every five seconds**. Occurrences in between are + counted and reported as `SuppressedCount` on the next raise — so a storm shows up as a number + rather than as thousands of events. +- Buckets are independent: a *new* kind of failure is raised at once even while another kind is + being collapsed. +- Connecting resets the throttle, so a reconnect reports its first failure immediately. + +### Decode failure counter + +`DaqifiStreamingDevice.DecodeFailureCount` is the always-on companion to the event: the number of +frames whose decode threw and was discarded during the current streaming session. It is reset by +`StartStreaming()`, and a healthy stream leaves it at zero — so a non-zero value while samples are +missing is the fastest confirmation that frames are arriving but not decoding. + +```csharp +device.StartStreaming(); +// ... +if (device.DecodeFailureCount > 0) +{ + Console.WriteLine($"{device.DecodeFailureCount} frame(s) failed to decode this session"); +} +``` + +`ErrorOccurred` is raised on a background thread, so handlers should do the minimum and push real +work elsewhere. A handler that throws is caught and ignored — it can never disturb reading or +streaming. + ## Features - **Simple Factory API**: Single-call connection with `DaqifiDeviceFactory` @@ -777,6 +853,8 @@ messages regardless of whether anything observes the failure. unit connected over two transports - **Clean Abstraction**: Hardware details hidden behind well-defined interfaces - **Event-Driven**: Status changes and messages handled via events +- **Observable Failures**: Background read and decode errors surface on `ErrorOccurred` instead of + failing silently - **Type Safety**: Generic message types provide compile-time safety - **Retry Support**: Built-in connection retry with exponential backoff - **Thread-Safe Sending**: Background message queue for thread-safe command sending diff --git a/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs b/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs new file mode 100644 index 00000000..6a3a6409 --- /dev/null +++ b/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs @@ -0,0 +1,355 @@ +using Daqifi.Core.Communication.Consumers; +using Daqifi.Core.Communication.Transport; +using Daqifi.Core.Device; + +namespace Daqifi.Core.Tests.Communication.Transport; + +/// +/// Issues #377 and #394: a connection that dies mid-stream must reach the device as +/// rather than leaving it reporting Connected forever, +/// and an intentional disconnect must never be mistaken for one. +/// +/// +/// The TCP half — a peer that closes the socket, driven over a real loopback connection — lives in +/// . These cover the serial-shaped path (a read +/// that starts throwing) end to end through a device, and the one remaining place the reader loop +/// could still spin silently forever: a stream that has stopped being readable at all. +/// +public class ConnectionLossEscalationTests +{ + [Fact] + public void AMidStreamReadFailure_TransitionsTheDeviceToLost() + { + // The serial-unplug shape: reads start throwing and never recover. + using var transport = new WatchedFailingTransport(); + using var device = new DaqifiDevice("Unplugged Device", transport); + + var lost = new ManualResetEventSlim(false); + device.StatusChanged += (_, e) => + { + if (e.Status == ConnectionStatus.Lost) + { + lost.Set(); + } + }; + + device.Connect(); + Assert.Equal(ConnectionStatus.Connected, device.Status); + + transport.FailingStream.FailReads = true; + + Assert.True(lost.Wait(TimeSpan.FromSeconds(10)), "the device never reported ConnectionStatus.Lost"); + Assert.Equal(ConnectionStatus.Lost, device.Status); + Assert.False(device.IsConnected); + } + + [Fact] + public void AMidStreamReadFailure_AlsoReachesTheDeviceErrorSurface() + { + // #377 escalates; #378 explains. Both are fed by the same reader-loop failure. + using var transport = new WatchedFailingTransport(); + using var device = new DaqifiDevice("Unplugged Device", transport); + + var raised = new ManualResetEventSlim(false); + DeviceErrorEventArgs? captured = null; + device.ErrorOccurred += (_, e) => + { + captured = e; + raised.Set(); + }; + + device.Connect(); + transport.FailingStream.FailReads = true; + + Assert.True(raised.Wait(TimeSpan.FromSeconds(10))); + Assert.Equal(DeviceErrorSource.MessageConsumer, captured!.Source); + } + + [Fact] + public void ASingleFailedRead_DoesNotReportLost() + { + using var transport = new WatchedFailingTransport(); + using var device = new DaqifiDevice("Blipping Device", transport); + + var statuses = new List(); + device.StatusChanged += (_, e) => + { + lock (statuses) + { + statuses.Add(e.Status); + } + }; + + device.Connect(); + + // One failure, then the stream goes back to idling — a glitch, not a disconnect. Escalation + // needs a run of five, so nothing may be reported. (That a *successful* read clears an + // accumulated run is covered by StreamMessageConsumerHealthReportingTests.) + transport.FailingStream.FailOnce(); + + Thread.Sleep(600); + + lock (statuses) + { + Assert.DoesNotContain(ConnectionStatus.Lost, statuses); + } + + Assert.Equal(ConnectionStatus.Connected, device.Status); + } + + [Fact] + public void AnIntentionalDisconnect_NeverReportsLost_EvenThoughTeardownFailsTheReads() + { + using var transport = new WatchedFailingTransport(); + using var device = new DaqifiDevice("Departing Device", transport); + + device.Connect(); + Assert.Equal(ConnectionStatus.Connected, device.Status); + + var statuses = new List(); + device.StatusChanged += (_, e) => + { + lock (statuses) + { + statuses.Add(e.Status); + } + }; + + // Closing the handle is what makes the in-flight reads fail, exactly as a real transport + // teardown does. None of that may be reported as a loss. + device.Disconnect(); + + Thread.Sleep(600); + + lock (statuses) + { + Assert.DoesNotContain(ConnectionStatus.Lost, statuses); + Assert.Contains(ConnectionStatus.Disconnected, statuses); + } + + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + } + + [Fact] + public void AStreamThatIsNoLongerReadable_IsReportedInsteadOfSpunOnSilently() + { + // The last silent-spin path in the reader loop (issue #377): a stream that reports itself + // unreadable never becomes readable again, so backing off and looping forever produced no + // data, no error and no status change simultaneously. + using var stream = new UnreadableStream(); + var sink = new RecordingHealthSink(); + var errors = 0; + + using var consumer = new StreamMessageConsumer( + stream, new LineBasedMessageParser(), healthSink: sink); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + + Assert.True(WaitUntil( + () => sink.FaultCount >= TransportConnectionWatchdog.ConsecutiveFaultThreshold, + TimeSpan.FromSeconds(10)), + $"expected the unreadable stream to be escalated, saw {sink.FaultCount} fault(s)"); + Assert.True(Volatile.Read(ref errors) >= 1); + + consumer.StopSafely(timeoutMs: 2000); + } + + private static bool WaitUntil(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (DateTime.UtcNow < deadline) + { + if (condition()) + { + return true; + } + + Thread.Sleep(10); + } + + return condition(); + } + + /// + /// Records what the reader loop reports, standing in for a real transport. + /// + private sealed class RecordingHealthSink : ITransportHealthSink + { + private int _faultCount; + + public int FaultCount => Volatile.Read(ref _faultCount); + + public void ReportIoFault(Exception error) => Interlocked.Increment(ref _faultCount); + + public void ReportIoSuccess() + { + } + } + + /// + /// A transport whose stream can be made to fail, wired to the same + /// the real serial and TCP transports use — so this + /// exercises the production escalation rules, not a test-only approximation of them. + /// + private sealed class WatchedFailingTransport : IStreamTransport, ITransportHealthSink + { + private readonly TransportConnectionWatchdog _watchdog; + private bool _isConnected; + private bool _disposed; + + public WatchedFailingTransport() + { + _watchdog = new TransportConnectionWatchdog("Test transport", HandleConnectionLost); + } + + public FailableStream FailingStream { get; } = new(); + + public Stream Stream => _disposed + ? throw new ObjectDisposedException(nameof(WatchedFailingTransport)) + : FailingStream; + + public bool IsConnected => _isConnected && !_disposed; + + public string ConnectionInfo => IsConnected ? "Test: Connected" : "Test: Disconnected"; + + public event EventHandler? StatusChanged; + + public Task ConnectAsync() => ConnectAsync(null); + + public Task ConnectAsync(ConnectionRetryOptions? retryOptions) + { + _isConnected = true; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); + _watchdog.Arm(); + return Task.CompletedTask; + } + + public Task DisconnectAsync() + { + // Disarm before tearing anything down, exactly as the real transports do: closing the + // handle makes in-flight reads fail, and none of that is a lost connection. + _watchdog.Disarm(); + + _isConnected = false; + FailingStream.FailReads = true; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + return Task.CompletedTask; + } + + public void Connect() => ConnectAsync().GetAwaiter().GetResult(); + + public void Disconnect() => DisconnectAsync().GetAwaiter().GetResult(); + + public void ReportIoFault(Exception error) => _watchdog.RecordFault(error); + + public void ReportIoSuccess() => _watchdog.RecordSuccess(); + + private void HandleConnectionLost(Exception error) + { + _isConnected = false; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo, error)); + } + + public void Dispose() + { + if (_disposed) + { + return; + } + + _disposed = true; + _isConnected = false; + _watchdog.Dispose(); + FailingStream.Dispose(); + } + } + + /// + /// A stream that idles quietly until told to fail its reads. + /// + internal sealed class FailableStream : Stream + { + private int _failuresRemaining = -1; + + public volatile bool FailReads; + + /// + /// Fails exactly one read, then goes back to idling — a glitch rather than a disconnect. + /// + public void FailOnce() => Interlocked.Exchange(ref _failuresRemaining, 1); + + public override int Read(byte[] buffer, int offset, int count) + { + if (FailReads) + { + Thread.Sleep(5); + throw new IOException("the device is gone"); + } + + if (Volatile.Read(ref _failuresRemaining) > 0) + { + Interlocked.Decrement(ref _failuresRemaining); + throw new IOException("a transient read glitch"); + } + + // Idle: the device has nothing to say right now. This is not a socket, so a zero-byte + // read is "nothing yet" and is never reported as a fault. + Thread.Sleep(20); + return 0; + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) + { + } + } + + /// + /// A stream that is open but permanently unreadable — the shape a closed or disposed underlying + /// handle presents to the reader loop. + /// + private sealed class UnreadableStream : Stream + { + public override bool CanRead => false; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override int Read(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } +} diff --git a/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs b/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs new file mode 100644 index 00000000..1579e6a0 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs @@ -0,0 +1,429 @@ +using Daqifi.Core.Channel; +using Daqifi.Core.Communication.Messages; +using Daqifi.Core.Communication.Transport; +using Daqifi.Core.Device; +using System.Net; +using System.Text; + +namespace Daqifi.Core.Tests.Device; + +/// +/// Issue #378: background failures used to be invisible — the message consumer raised +/// ErrorOccurred into an event with no subscribers, and a per-frame decode failure was +/// swallowed by an empty catch. Both now reach without +/// changing anything about how the device behaves. +/// +public class DeviceErrorSurfaceTests +{ + #region Message consumer errors + + [Fact] + public void AFailingReadLoop_ReachesAnErrorOccurredSubscriber() + { + using var transport = new ScriptedStreamTransport(); + using var device = new DaqifiDevice("Erroring Device", transport); + + var raised = new ManualResetEventSlim(false); + DeviceErrorEventArgs? captured = null; + device.ErrorOccurred += (_, e) => + { + captured = e; + raised.Set(); + }; + + device.Connect(); + transport.ScriptedStream.FailReads = true; + + Assert.True(raised.Wait(TimeSpan.FromSeconds(10)), "the read failure never reached a subscriber"); + Assert.Equal(DeviceErrorSource.MessageConsumer, captured!.Source); + Assert.IsType(captured.Error); + } + + [Fact] + public void AHealthyIdleDevice_RaisesNoErrors() + { + // The bench case: a connected device that simply has nothing to say must stay quiet. + // A read timeout is what an idle device looks like, not a failure. + using var transport = new ScriptedStreamTransport(); + using var device = new DaqifiDevice("Quiet Device", transport); + + var errors = 0; + device.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + device.Connect(); + transport.ScriptedStream.TimeoutReads = true; + + Thread.Sleep(500); + + Assert.Equal(0, Volatile.Read(ref errors)); + Assert.Equal(ConnectionStatus.Connected, device.Status); + } + + [Fact] + public void AThrowingErrorSubscriber_DoesNotDisturbTheReaderLoop() + { + using var transport = new ScriptedStreamTransport(); + using var device = new DaqifiDevice("Erroring Device", transport); + + var raised = new ManualResetEventSlim(false); + device.ErrorOccurred += (_, _) => + { + raised.Set(); + throw new InvalidOperationException("a badly behaved subscriber"); + }; + + device.Connect(); + transport.ScriptedStream.FailReads = true; + Assert.True(raised.Wait(TimeSpan.FromSeconds(10))); + + // The reader survived the throwing handler: it is still issuing reads, and the device was + // not knocked out of Connected by it. + var readsSoFar = transport.ScriptedStream.ReadCount; + Assert.True(WaitUntil(() => transport.ScriptedStream.ReadCount > readsSoFar + 2, TimeSpan.FromSeconds(10)), + "the reader loop stopped issuing reads after a subscriber threw"); + Assert.Equal(ConnectionStatus.Connected, device.Status); + } + + [Fact] + public void AfterDisconnect_TheConsumerErrorSurfaceIsDetached() + { + using var transport = new ScriptedStreamTransport(); + using var device = new DaqifiDevice("Erroring Device", transport); + + var errors = 0; + device.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + device.Connect(); + device.Disconnect(); + + var afterDisconnect = Volatile.Read(ref errors); + transport.ScriptedStream.FailReads = true; + Thread.Sleep(400); + + Assert.Equal(afterDisconnect, Volatile.Read(ref errors)); + } + + #endregion + + #region Stream decode errors + + [Fact] + public void ASystematicallyThrowingDecode_StaysObservableWhileTheStreamKeepsRunning() + { + var device = CreateStreamingDevice(); + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.IsEnabled = true; + + // A subscriber that throws on every sample is the realistic shape of a decode that fails + // on every frame: it propagates out of the per-channel push and into the frame's catch. + channel.SampleReceived += (_, _) => throw new InvalidOperationException("decode consumer is broken"); + + var errors = new List(); + device.ErrorOccurred += (_, e) => + { + lock (errors) + { + errors.Add(e); + } + }; + + var rawFrames = 0; + device.StreamMessageReceived += _ => Interlocked.Increment(ref rawFrames); + + device.StartStreaming(); + + const int frameCount = 200; + for (var i = 1; i <= frameCount; i++) + { + device.InvokeStreamMessage(AnalogFrame((uint)(i * 1000), 1.0f)); + } + + // Isolation is unchanged: every frame was still delivered, and the stream is still running. + Assert.Equal(frameCount, Volatile.Read(ref rawFrames)); + Assert.True(device.IsStreaming); + + // But the failure is no longer silent. + Assert.Equal(frameCount, device.DecodeFailureCount); + + lock (errors) + { + var error = Assert.Single(errors); + Assert.Equal(DeviceErrorSource.StreamDecode, error.Source); + Assert.IsType(error.Error); + } + } + + [Fact] + public void ADecodeStorm_IsBoundedByTheThrottle() + { + // The volume guarantee from the acceptance criteria: thousands of identical failures must + // not become thousands of events. + var device = CreateStreamingDevice(); + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.IsEnabled = true; + channel.SampleReceived += (_, _) => throw new InvalidOperationException("decode consumer is broken"); + + var raises = 0; + device.ErrorOccurred += (_, _) => Interlocked.Increment(ref raises); + + device.StartStreaming(); + + const int frameCount = 5000; + for (var i = 1; i <= frameCount; i++) + { + device.InvokeStreamMessage(AnalogFrame((uint)(i * 1000), 1.0f)); + } + + Assert.Equal(frameCount, device.DecodeFailureCount); + + // Bounded, not proportional. The policy allows one raise per five seconds per bucket, so a + // fast machine sees exactly one; the upper bound leaves room for a slow one without turning + // this into a timing test. + Assert.InRange(Volatile.Read(ref raises), 1, 3); + } + + [Fact] + public void AHealthyDecode_LeavesTheFailureCounterAtZeroAndRaisesNothing() + { + var device = CreateStreamingDevice(); + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.IsEnabled = true; + + var samples = 0; + channel.SampleReceived += (_, _) => Interlocked.Increment(ref samples); + + var raises = 0; + device.ErrorOccurred += (_, _) => Interlocked.Increment(ref raises); + + device.StartStreaming(); + for (var i = 1; i <= 50; i++) + { + device.InvokeStreamMessage(AnalogFrame((uint)(i * 1000), 1.0f)); + } + + Assert.Equal(50, Volatile.Read(ref samples)); + Assert.Equal(0, device.DecodeFailureCount); + Assert.Equal(0, Volatile.Read(ref raises)); + } + + [Fact] + public void TheDecodeFailureCounter_DescribesTheCurrentSession() + { + var device = CreateStreamingDevice(); + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.IsEnabled = true; + channel.SampleReceived += (_, _) => throw new InvalidOperationException("decode consumer is broken"); + + device.StartStreaming(); + device.InvokeStreamMessage(AnalogFrame(1000, 1.0f)); + device.InvokeStreamMessage(AnalogFrame(2000, 1.0f)); + Assert.Equal(2, device.DecodeFailureCount); + + device.StopStreaming(); + device.StartStreaming(); + + Assert.Equal(0, device.DecodeFailureCount); + } + + [Fact] + public void AFrameThatArrivesOutsideAStreamingSession_IsNotCountedAsADecodeFailure() + { + // Frames outside a session are re-raised but never decoded, so nothing can fail. + var device = CreateStreamingDevice(); + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.IsEnabled = true; + channel.SampleReceived += (_, _) => throw new InvalidOperationException("decode consumer is broken"); + + device.InvokeStreamMessage(AnalogFrame(1000, 1.0f)); + + Assert.Equal(0, device.DecodeFailureCount); + } + + #endregion + + #region Helpers + + private static bool WaitUntil(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (DateTime.UtcNow < deadline) + { + if (condition()) + { + return true; + } + + Thread.Sleep(10); + } + + return condition(); + } + + private static DecodableStreamingDevice CreateStreamingDevice() + { + var device = new DecodableStreamingDevice("Decode Device"); + device.Connect(); + + var status = new DaqifiOutMessage + { + AnalogInPortNum = 1, + AnalogInRes = 65535, + }; + status.AnalogInPortRange.Add(1.0f); + + device.PopulateChannelsFromStatus(status); + return device; + } + + private static DaqifiOutMessage AnalogFrame(uint timestamp, float value) + { + var frame = new DaqifiOutMessage { MsgTimeStamp = timestamp }; + frame.AnalogInDataFloat.Add(value); + return frame; + } + + /// + /// A streaming device with no transport, exposing the protected stream handler so frames can be + /// injected directly and swallowing outbound SCPI. + /// + private sealed class DecodableStreamingDevice : DaqifiStreamingDevice + { + public DecodableStreamingDevice(string name, IPAddress? ipAddress = null) : base(name, ipAddress) + { + } + + public void InvokeStreamMessage(DaqifiOutMessage message) => OnStreamMessageReceived(message); + + public override void Send(IOutboundMessage message) + { + } + } + + /// + /// A transport over a stream whose reads can be made to fail or time out on demand, so a device + /// can be driven through a failing read loop without hardware. + /// + private sealed class ScriptedStreamTransport : IStreamTransport + { + private bool _isConnected; + private bool _disposed; + + public ScriptedStream ScriptedStream { get; } = new(); + + public Stream Stream => _disposed + ? throw new ObjectDisposedException(nameof(ScriptedStreamTransport)) + : ScriptedStream; + + public bool IsConnected => _isConnected && !_disposed; + + public string ConnectionInfo => _isConnected ? "Scripted: Connected" : "Scripted: Disconnected"; + + public event EventHandler? StatusChanged; + + public Task ConnectAsync() => ConnectAsync(null); + + public Task ConnectAsync(ConnectionRetryOptions? retryOptions) + { + _isConnected = true; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); + return Task.CompletedTask; + } + + public Task DisconnectAsync() + { + _isConnected = false; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + return Task.CompletedTask; + } + + public void Connect() => ConnectAsync().GetAwaiter().GetResult(); + + public void Disconnect() => DisconnectAsync().GetAwaiter().GetResult(); + + public void Dispose() + { + if (_disposed) + { + return; + } + + _isConnected = false; + _disposed = true; + ScriptedStream.Dispose(); + } + } + + /// + /// A stream whose reads can be switched between delivering scripted bytes, timing out (an idle + /// device), and failing (a device that has gone away). + /// + internal sealed class ScriptedStream : Stream + { + private readonly Queue _pending = new(); + private readonly object _gate = new(); + private int _readCount; + + public volatile bool FailReads; + public volatile bool TimeoutReads; + + public int ReadCount => Volatile.Read(ref _readCount); + + public override int Read(byte[] buffer, int offset, int count) + { + Interlocked.Increment(ref _readCount); + + if (TimeoutReads) + { + Thread.Sleep(5); + throw new TimeoutException("no data within the read timeout"); + } + + if (FailReads) + { + Thread.Sleep(5); + throw new IOException("the device is gone"); + } + + lock (_gate) + { + if (_pending.Count == 0) + { + Thread.Sleep(5); + return 0; + } + + var chunk = _pending.Dequeue(); + var length = Math.Min(chunk.Length, count); + Array.Copy(chunk, 0, buffer, offset, length); + return length; + } + } + + public override void Write(byte[] buffer, int offset, int count) + { + // Outbound SCPI goes nowhere; these tests only exercise the read side. + _ = Encoding.UTF8.GetString(buffer, offset, count); + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + } + + #endregion +} diff --git a/src/Daqifi.Core.Tests/Device/DeviceErrorThrottleTests.cs b/src/Daqifi.Core.Tests/Device/DeviceErrorThrottleTests.cs new file mode 100644 index 00000000..ab827d51 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/DeviceErrorThrottleTests.cs @@ -0,0 +1,179 @@ +using Daqifi.Core.Device; + +namespace Daqifi.Core.Tests.Device; + +/// +/// Issue #378: the error surface has to stay useful under a systematic failure that repeats at the +/// frame rate. These pin the documented policy — first occurrence always through, repeats collapsed +/// per (source, exception type) with the collapsed count reported on the next raise. +/// +public class DeviceErrorThrottleTests +{ + private static readonly TimeSpan ShortInterval = TimeSpan.FromMilliseconds(150); + + [Fact] + public void TheFirstOccurrence_IsAlwaysRaised() + { + var throttle = new DeviceErrorThrottle(ShortInterval); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new InvalidOperationException(), out var suppressed)); + Assert.Equal(0, suppressed); + } + + [Fact] + public void RepeatsWithinTheInterval_AreCollapsed() + { + var throttle = new DeviceErrorThrottle(TimeSpan.FromMinutes(1)); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new InvalidOperationException(), out _)); + + for (var i = 0; i < 1000; i++) + { + Assert.False(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new InvalidOperationException(), out _)); + } + } + + [Fact] + public void TheNextRaiseAfterTheInterval_ReportsHowManyWereCollapsed() + { + var throttle = new DeviceErrorThrottle(ShortInterval); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out _)); + + const int collapsed = 25; + for (var i = 0; i < collapsed; i++) + { + Assert.False(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out _)); + } + + Thread.Sleep(ShortInterval + ShortInterval); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out var suppressed)); + Assert.Equal(collapsed, suppressed); + + // The count is consumed by the raise that reports it, not carried forward. + Thread.Sleep(ShortInterval + ShortInterval); + Assert.True(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out var afterReport)); + Assert.Equal(0, afterReport); + } + + [Fact] + public void ADifferentExceptionType_IsNotDelayedBehindAnOngoingStorm() + { + // The whole point of bucketing: a new failure mode appearing during a storm of another one + // is exactly the thing an operator needs to see, and it must not wait for a window to open. + var throttle = new DeviceErrorThrottle(TimeSpan.FromMinutes(1)); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new InvalidOperationException(), out _)); + Assert.False(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new InvalidOperationException(), out _)); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new IndexOutOfRangeException(), out _)); + } + + [Fact] + public void ADifferentSource_IsNotDelayedBehindAnOngoingStorm() + { + var throttle = new DeviceErrorThrottle(TimeSpan.FromMinutes(1)); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new IOException(), out _)); + Assert.False(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new IOException(), out _)); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out _)); + } + + [Fact] + public void AZeroInterval_DisablesCollapsingEntirely() + { + var throttle = new DeviceErrorThrottle(TimeSpan.Zero); + + for (var i = 0; i < 100; i++) + { + Assert.True(throttle.ShouldRaise(DeviceErrorSource.StreamDecode, new IOException(), out var suppressed)); + Assert.Equal(0, suppressed); + } + } + + [Fact] + public void Reset_LetsTheNextOccurrenceThroughImmediately() + { + var throttle = new DeviceErrorThrottle(TimeSpan.FromMinutes(1)); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out _)); + Assert.False(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out _)); + + throttle.Reset(); + + Assert.True(throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, new IOException(), out _)); + } + + [Fact] + public void ManyDistinctFailureTypes_DoNotGrowTheBucketTableWithoutBound() + { + var throttle = new DeviceErrorThrottle(TimeSpan.FromMinutes(1)); + + // More distinct types than the cap, driven through twice: past the cap they share the + // overflow bucket, so the second pass must be collapsed rather than raised again. + var errors = BuildDistinctErrors(); + + foreach (var error in errors) + { + throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, error, out _); + } + + var raisedOnSecondPass = errors.Count(e => throttle.ShouldRaise(DeviceErrorSource.MessageConsumer, e, out _)); + Assert.Equal(0, raisedOnSecondPass); + } + + /// + /// Builds a list of exceptions with distinct runtime types, so each would claim its own bucket. + /// + private static List BuildDistinctErrors() + { + var errors = new List + { + new IOException(), + new EndOfStreamException(), + new FileNotFoundException(), + new DirectoryNotFoundException(), + new PathTooLongException(), + new FileLoadException(), + new InvalidOperationException(), + new IndexOutOfRangeException(), + new FormatException(), + new NotSupportedException(), + new NotImplementedException(), + new TimeoutException(), + new ArgumentException(), + new ArgumentNullException(), + new ArgumentOutOfRangeException(), + new OverflowException(), + new ObjectDisposedException("stream"), + new NullReferenceException(), + new InvalidCastException(), + new RankException(), + new ArithmeticException(), + new DivideByZeroException(), + new KeyNotFoundException(), + new PlatformNotSupportedException(), + new UnauthorizedAccessException(), + new ApplicationException(), + new SystemException(), + new MissingFieldException(), + new MissingMethodException(), + new MissingMemberException(), + new BadImageFormatException(), + new TypeLoadException(), + new DataMisalignedException(), + new InsufficientMemoryException(), + new OutOfMemoryException(), + new AggregateException(), + new OperationCanceledException(), + new ArrayTypeMismatchException(), + new MethodAccessException(), + new FieldAccessException(), + }; + + Assert.True(errors.Count > DeviceErrorThrottle.MaxTrackedBuckets, "the fixture must exceed the cap"); + return errors; + } +} diff --git a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs index a1744f85..0abeb970 100644 --- a/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs @@ -2915,6 +2915,7 @@ public CancelingLanChipInfoStreamingDevice(string name, CancellationTokenSource public bool IsStreaming { get; private set; } public event EventHandler? StatusChanged { add { } remove { } } public event EventHandler? MessageReceived { add { } remove { } } + public event EventHandler? ErrorOccurred { add { } remove { } } public void Connect() => IsConnected = true; public void Disconnect() => IsConnected = false; @@ -3019,6 +3020,7 @@ public SlowFakeLanChipInfoStreamingDevice(string name, TimeSpan attemptLatency) public bool IsStreaming { get; private set; } public event EventHandler? StatusChanged { add { } remove { } } public event EventHandler? MessageReceived { add { } remove { } } + public event EventHandler? ErrorOccurred { add { } remove { } } public void Connect() => IsConnected = true; public void Disconnect() => IsConnected = false; public void Send(IOutboundMessage message) { } @@ -3755,6 +3757,12 @@ public event EventHandler? MessageReceived remove { } } + public event EventHandler? ErrorOccurred + { + add { } + remove { } + } + public void Connect() { ConnectAttempts++; @@ -3872,6 +3880,12 @@ public event EventHandler? MessageReceived remove { } } + public event EventHandler? ErrorOccurred + { + add { } + remove { } + } + public void Connect() { IsConnected = true; diff --git a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs index 4539b336..6cce4215 100644 --- a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs +++ b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs @@ -331,10 +331,18 @@ private void ProcessMessages() PerformClear(); } - // Check if data is available to avoid blocking + // A stream that reports itself unreadable never becomes readable again — that is a + // closed or disposed stream, not a momentary lull. Report it instead of spinning + // here forever producing no data, no error and no status change, which is the + // failure mode issue #377 was filed for. Backed off at the same cadence as a + // failing read so the escalation timing matches. if (!_stream.CanRead) { - Thread.Sleep(10); + var unreadable = new IOException( + "The stream is no longer readable; the underlying connection has been closed."); + _healthSink?.ReportIoFault(unreadable); + OnErrorOccurred(unreadable); + Thread.Sleep(100); continue; } diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 3e28d755..27eb90f6 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -453,6 +453,55 @@ private set /// public event EventHandler>? SendFailed; + /// + /// Occurs when something fails on one of the device's background threads: a read from the + /// transport stream, a parse, a dispatch to a subscriber, or the decode of a streaming frame. + /// + /// + /// + /// Silence used to be the only symptom of these failures (issue #378) — a stream that cannot + /// be read and a stream that cannot be decoded both looked exactly like a device sending + /// nothing. This is the one place to answer "why am I getting no samples". + /// + /// + /// Observational only. Raising this never changes device behaviour: no tear-down, no + /// retry, no change, and a single bad frame is still isolated so the + /// stream survives it. Deciding that a link is actually dead is separate, and arrives as + /// on (issue #377). Every + /// error raised here is also written to the device's ILogger, so it stays visible with + /// no subscriber attached. + /// + /// + /// Throttle policy. A systematic failure repeats at the frame rate, so raises are + /// collapsed per bucket, where a bucket is + /// (, exception type): + /// + /// + /// The first occurrence in a bucket is always raised, immediately. + /// + /// After that, a bucket raises at most once every five seconds. Occurrences in between are + /// counted and reported as on the next + /// raise, so a storm is visible as a number rather than as thousands of events. + /// + /// + /// Buckets are independent: a new kind of failure is raised at once even while another kind + /// is being collapsed. + /// + /// + /// + /// Raised on a background thread (the reader loop, or whichever thread decoded the frame), so + /// handlers should do the minimum and push real work elsewhere. A handler that throws is + /// caught and ignored — it can never disturb reading or streaming. + /// + /// + public event EventHandler? ErrorOccurred; + + /// + /// Collapses repeated background failures so a systematic fault stays visible without + /// storming . See that event for the documented policy. + /// + private readonly DeviceErrorThrottle _errorThrottle = new(); + /// /// Initializes a new instance of the class. /// @@ -510,6 +559,10 @@ public void Connect() Status = ConnectionStatus.Connecting; State = DeviceState.Connecting; + // A reconnect is a new session: its first background failure should be reported + // immediately rather than collapsed into a throttle window the previous session opened. + _errorThrottle.Reset(); + try { // Connect transport if available @@ -535,6 +588,14 @@ public void Connect() new ProtobufMessageParser(), healthSink: healthSink); } + + // Read/parse/dispatch failures used to be raised into an event with no + // subscribers (issue #378). Subscribe here rather than alongside + // MessageReceived: that one is attached and detached around every consumer + // swap, and error visibility must not have holes in it. '-=' first keeps a + // reconnect on the same consumer instance from double-subscribing. + _messageConsumer.ErrorOccurred -= OnConsumerErrorOccurred; + _messageConsumer.ErrorOccurred += OnConsumerErrorOccurred; } // Start message producer and consumer if available @@ -597,6 +658,7 @@ public void Disconnect() if (_messageConsumer != null) { _messageConsumer.MessageReceived -= OnInboundMessageReceived; + _messageConsumer.ErrorOccurred -= OnConsumerErrorOccurred; } if (_messageProducer != null) @@ -1025,6 +1087,11 @@ private async Task> ExecuteTextCommandCoreAsync( collectedLines.Add(e.Message.Data); }; + // The protobuf consumer is stopped for the duration of this exchange, so + // without this a read failure during a text command (an unplug mid-SD-listing, + // say) would be the one background failure with nowhere to go (issue #378). + textConsumer.ErrorOccurred += OnConsumerErrorOccurred; + textConsumer.Start(); // ConfigureAwait(false): the lock is held, so resuming on a captured // sync context (e.g. UI thread) would deadlock if that thread calls Disconnect(). @@ -1431,6 +1498,64 @@ private void OnMessageSendFailed(object? sender, MessageSendFailedEventArgs SendFailed?.Invoke(this, e)); } + /// + /// Forwards a message-consumer failure (a failed read, a parse error, or a subscriber that + /// threw) to . + /// + /// + /// Nothing in Core subscribed to before + /// issue #378, so these failures were raised into an empty event and lost. Forwarding them + /// is purely additive — the consumer's own back-off and the transport's drop escalation are + /// unchanged by whether anyone is listening here. + /// + private void OnConsumerErrorOccurred(object? sender, MessageConsumerErrorEventArgs e) + { + RaiseDeviceError(DeviceErrorSource.MessageConsumer, e.Error, e.RawData); + } + + /// + /// Logs a background failure and raises for it, subject to the + /// throttle policy documented on that event. + /// + /// The pipeline stage that failed. + /// The exception that was caught. + /// The bytes being processed at the time, if the stage had any. + /// + /// Never throws. It runs on background threads inside catch blocks whose entire purpose is + /// to keep reading and decoding alive, so neither a throwing logger nor a throwing + /// subscriber may escape — the same isolation SendFailed and the classified-event + /// raisers use. + /// + protected void RaiseDeviceError(DeviceErrorSource source, Exception error, byte[]? rawData = null) + { + if (error == null) + { + return; + } + + if (!_errorThrottle.ShouldRaise(source, error, out var suppressedCount)) + { + return; + } + + SafeLog(() => _logger.LogWarning( + error, + "[{Source}] Device '{DeviceName}' background failure ({SuppressedCount} like failure(s) suppressed since the last report).", + source, + Name, + suppressedCount)); + + var handler = ErrorOccurred; + if (handler == null) + { + return; + } + + // Same guard as the logger above: a subscriber that throws must not take down the + // reader loop or the decode path this was raised from. + SafeLog(() => handler(this, new DeviceErrorEventArgs(source, error, suppressedCount, rawData))); + } + /// /// Disposes the device and releases resources. /// diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index d2fcaa7a..185f1928 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -147,6 +147,30 @@ public class DaqifiStreamingDevice : DaqifiDevice, IStreamingDevice, INetworkCon /// private int _suppressedWarmupFrameCount; + /// + /// Backing counter for . + /// + private long _decodeFailureCount; + + /// + /// Gets the number of streaming frames whose decode threw and was discarded since the + /// current streaming session began. + /// + /// + /// + /// Per-frame decoding is deliberately best-effort — a single malformed frame must never tear + /// down the stream — but that isolation used to be completely silent (issue #378): a decode + /// that failed on every frame produced zero samples and zero diagnostics, indistinguishable + /// from a device sending nothing. This counter is the cheap always-on version of that + /// signal; carries the exception behind it. + /// + /// + /// Reset by , so it describes the current session. A healthy + /// stream leaves it at zero. + /// + /// + public long DecodeFailureCount => Interlocked.Read(ref _decodeFailureCount); + /// /// Gets a value indicating whether the device is currently streaming data. /// @@ -358,6 +382,7 @@ public void StartStreaming() // observed warmup frame). _awaitingFirstFullAnalogFrame = CountEnabledAnalogChannels(SnapshotChannels()) > 0; _suppressedWarmupFrameCount = 0; + Interlocked.Exchange(ref _decodeFailureCount, 0); IsStreaming = true; Send(ScpiMessageProducer.StartStreaming(StreamingFrequency)); @@ -484,10 +509,17 @@ protected override void OnStreamMessageReceived(DaqifiOutMessage message) { DecodeStreamFrame(message); } - catch (Exception) + catch (Exception ex) { // A single malformed frame must never tear down the stream or starve other - // consumers; decoding is best-effort per frame. + // consumers; decoding is best-effort per frame. That isolation stays exactly as it + // was — the frame is dropped and the loop continues — but it is no longer silent + // (issue #378): a decode that throws on every frame yields no samples, which used + // to be indistinguishable from a device sending nothing at all. Both the counter + // and the (throttled) event are observation only; neither changes what happens to + // this frame or the next one. + Interlocked.Increment(ref _decodeFailureCount); + RaiseDeviceError(DeviceErrorSource.StreamDecode, ex); } } diff --git a/src/Daqifi.Core/Device/DeviceErrorEventArgs.cs b/src/Daqifi.Core/Device/DeviceErrorEventArgs.cs new file mode 100644 index 00000000..ce3b91f9 --- /dev/null +++ b/src/Daqifi.Core/Device/DeviceErrorEventArgs.cs @@ -0,0 +1,84 @@ +namespace Daqifi.Core.Device; + +/// +/// Describes a failure that happened on one of a device's background threads — the message +/// consumer's read loop, or the per-frame stream decoder. +/// +/// +/// +/// Purely observational (issue #378). Raising this event never changes what the device does: no +/// tear-down, no retry, no status change, and a single bad frame is still isolated so the stream +/// survives it. Escalating a genuinely dead link to +/// is the transports' job and is reported through +/// . +/// +/// +/// Delivery is throttled per and exception type — see +/// for the policy — so may be +/// non-zero on the events that do get through. +/// +/// +public class DeviceErrorEventArgs : EventArgs +{ + /// + /// Initializes a new instance of the class. + /// + /// Which part of the pipeline failed. + /// The exception that was caught. + /// + /// How many like failures were collapsed by the throttle since the previous raise. Zero when + /// nothing was suppressed. + /// + /// The raw bytes being processed when the failure occurred, if available. + /// Thrown when is null. + /// Thrown when is negative. + public DeviceErrorEventArgs( + DeviceErrorSource source, + Exception error, + int suppressedCount = 0, + byte[]? rawData = null) + { + ArgumentNullException.ThrowIfNull(error); + if (suppressedCount < 0) + { + throw new ArgumentOutOfRangeException( + nameof(suppressedCount), suppressedCount, "Suppressed count cannot be negative."); + } + + Source = source; + Error = error; + SuppressedCount = suppressedCount; + RawData = rawData; + Timestamp = DateTime.UtcNow; + } + + /// + /// Gets the part of the device pipeline that produced the failure. + /// + public DeviceErrorSource Source { get; } + + /// + /// Gets the exception that was caught. + /// + public Exception Error { get; } + + /// + /// Gets the number of like failures (same , same exception type) that the + /// throttle collapsed since the previous raise, or zero if none were. + /// + /// + /// A large value is the signal that matters: it means the failure is systematic rather than a + /// one-off, which is exactly the case that used to be invisible. + /// + public int SuppressedCount { get; } + + /// + /// Gets the raw bytes being processed when the failure occurred, if the failing stage had any. + /// + public byte[]? RawData { get; } + + /// + /// Gets the UTC time at which the failure was observed. + /// + public DateTime Timestamp { get; } +} diff --git a/src/Daqifi.Core/Device/DeviceErrorSource.cs b/src/Daqifi.Core/Device/DeviceErrorSource.cs new file mode 100644 index 00000000..84f5df6c --- /dev/null +++ b/src/Daqifi.Core/Device/DeviceErrorSource.cs @@ -0,0 +1,35 @@ +namespace Daqifi.Core.Device; + +/// +/// Identifies which part of a device's background pipeline produced a +/// . +/// +/// +/// The source is the first thing a diagnostic needs: "no samples" caused by a stream that cannot be +/// read is a different problem from a stream that reads fine but cannot be decoded, and before +/// issue #378 both looked identical from outside the library (silence). +/// +public enum DeviceErrorSource +{ + /// + /// The source could not be classified. + /// + Unknown = 0, + + /// + /// The background message consumer: a failed read from the transport stream, a parse failure, + /// or an exception thrown while dispatching a parsed message to subscribers. + /// + /// + /// A persistent run of read failures is also what the transport escalates into + /// (issue #377). Seeing this source is therefore not by + /// itself proof the link is gone — watch for that. + /// + MessageConsumer = 1, + + /// + /// Per-frame decoding of a streaming data frame into channel samples + /// (). The frame is dropped; the stream keeps running. + /// + StreamDecode = 2, +} diff --git a/src/Daqifi.Core/Device/DeviceErrorThrottle.cs b/src/Daqifi.Core/Device/DeviceErrorThrottle.cs new file mode 100644 index 00000000..921fd916 --- /dev/null +++ b/src/Daqifi.Core/Device/DeviceErrorThrottle.cs @@ -0,0 +1,172 @@ +using System.Collections.Concurrent; +using System.Diagnostics; + +namespace Daqifi.Core.Device; + +/// +/// Bounds how often a repeating background failure is raised as a +/// , so a systematic failure stays visible without storming the +/// subscriber (issue #378). +/// +/// +/// +/// The failure this exists for is a decode that throws on every frame. At a few kHz that +/// is thousands of raises per second — enough that a naive subscriber (a log sink, a UI marshal) +/// becomes the bottleneck and the "observability" feature degrades the streaming it was added to +/// explain. Collapsing repeats keeps the first report immediate and the rest cheap. +/// +/// +/// The policy, per bucket ( + exception type): +/// +/// +/// The first occurrence always passes, immediately. +/// +/// Later occurrences pass at most once per ; the ones in between are counted +/// and reported as on the next one that passes. +/// +/// +/// Buckets are per exception type, so a new kind of failure is never delayed behind an +/// ongoing storm of a different one. +/// +/// +/// +/// Bucket count is capped at ; everything past the cap shares a +/// single overflow bucket. Exception types come from code rather than from the wire, so the cap is +/// unreachable in practice — it exists so this can never grow without bound. +/// +/// +internal sealed class DeviceErrorThrottle +{ + /// + /// Default minimum spacing between raises of the same bucket. + /// + /// + /// Long enough that a per-frame failure at kHz rates collapses to a trickle, short enough that + /// a human watching a log still sees the problem is ongoing rather than a single stale line. + /// + internal static readonly TimeSpan DefaultInterval = TimeSpan.FromSeconds(5); + + /// + /// Maximum number of distinct buckets tracked before everything else shares one. + /// + internal const int MaxTrackedBuckets = 32; + + private const string OverflowKey = ""; + + private readonly ConcurrentDictionary _buckets = new(StringComparer.Ordinal); + private readonly TimeSpan _interval; + + /// + /// Initializes a new throttle. + /// + /// + /// Minimum spacing between raises of the same bucket. Defaults to . + /// disables collapsing entirely (every occurrence passes). + /// + /// Thrown when is negative. + public DeviceErrorThrottle(TimeSpan? interval = null) + { + _interval = interval ?? DefaultInterval; + if (_interval < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(interval), _interval, "Throttle interval cannot be negative."); + } + } + + /// + /// Gets the minimum spacing between raises of the same bucket. + /// + public TimeSpan Interval => _interval; + + /// + /// Decides whether this occurrence should be raised. + /// + /// The pipeline stage that failed. + /// The exception that was caught. + /// + /// When this returns true, the number of like occurrences collapsed since the previous + /// raise (zero if none). Meaningless when it returns false. + /// + /// true to raise, false to collapse this occurrence into the count. + public bool ShouldRaise(DeviceErrorSource source, Exception error, out int suppressedCount) + { + ArgumentNullException.ThrowIfNull(error); + + if (_interval == TimeSpan.Zero) + { + suppressedCount = 0; + return true; + } + + var bucket = GetBucket($"{(int)source}:{error.GetType().FullName}"); + + lock (bucket) + { + var now = Stopwatch.GetTimestamp(); + + if (bucket.HasRaised && Stopwatch.GetElapsedTime(bucket.LastRaised, now) < _interval) + { + // Saturate rather than overflow: a run long enough to wrap int is already + // "enormous", and a negative count would be worse than an inexact one. + if (bucket.Suppressed < int.MaxValue) + { + bucket.Suppressed++; + } + + suppressedCount = 0; + return false; + } + + suppressedCount = bucket.Suppressed; + bucket.Suppressed = 0; + bucket.LastRaised = now; + bucket.HasRaised = true; + return true; + } + } + + /// + /// Forgets all accumulated state, so the next occurrence of anything is raised immediately. + /// + /// + /// Called when a device connects: a reconnect is a new session, and its first failure should be + /// reported at once rather than collapsed into a window opened by the previous session. + /// + public void Reset() => _buckets.Clear(); + + /// + /// Resolves the bucket for a key, folding anything past into a + /// shared overflow bucket. + /// + /// + /// The count check is deliberately not atomic with the insert: racing callers may push the + /// table a few entries past the cap. That is harmless — the cap exists to stop unbounded + /// growth, not to be an exact quota — and paying for a lock on every background error to make + /// it exact would be the wrong trade. + /// + private Bucket GetBucket(string key) + { + if (_buckets.TryGetValue(key, out var existing)) + { + return existing; + } + + if (_buckets.Count >= MaxTrackedBuckets) + { + key = OverflowKey; + } + + return _buckets.GetOrAdd(key, _ => new Bucket()); + } + + /// + /// Per-key state. Mutated only under a lock on the instance itself. + /// + private sealed class Bucket + { + public long LastRaised; + public bool HasRaised; + public int Suppressed; + } +} diff --git a/src/Daqifi.Core/Device/IDevice.cs b/src/Daqifi.Core/Device/IDevice.cs index 672d8b51..89481fb2 100644 --- a/src/Daqifi.Core/Device/IDevice.cs +++ b/src/Daqifi.Core/Device/IDevice.cs @@ -41,6 +41,17 @@ public interface IDevice /// event EventHandler MessageReceived; + /// + /// Occurs when something fails on one of the device's background threads — a read from the + /// transport stream, a parse, a dispatch to a subscriber, or the decode of a streaming frame. + /// + /// + /// Observational only: it reports what went wrong and changes nothing about what the device + /// does (issue #378). Raises are throttled per source and exception type — see + /// for the policy and the guarantees. + /// + event EventHandler ErrorOccurred; + /// /// Connects to the device. /// From a8c9b1d8ca43b91014141f90ed1d67d26efe8478 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 13:56:27 -0600 Subject: [PATCH 02/16] feat(device): opt-in auto-reconnect and streaming session resume after a drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dropped connection could only ever be reported, never recovered: the caller had to rebuild the whole session by hand. Devices now do it themselves, if asked. - ReconnectOptions (Enabled/MaxAttempts/backoff/ResumeStreaming) on DaqifiDevice, off by default, so today's behaviour — surface Lost and stop — is unchanged - On Lost: snapshot the session, then reconnect the transport, re-run InitializeAsync and replay the enabled-channel set and any running stream - Progress on ReconnectAttempt / Reconnected / ReconnectFailed plus the existing Retrying and Failed statuses; giving up also logs and raises ErrorOccurred with the new DeviceErrorSource.Reconnect - Cancellable via CancelReconnect(); a caller-issued Connect/Disconnect/Dispose always supersedes an in-flight loop Same-endpoint only: a device that moved port or address needs a fresh connect, and cross-transport failover is out of scope. closes #379 Co-Authored-By: Claude Opus 5 --- docs/DEVICE_INTERFACES.md | 94 +- .../Device/DeviceReconnectTests.cs | 876 ++++++++++++++++++ .../Device/ReconnectOptionsTests.cs | 114 +++ src/Daqifi.Core/Device/ConnectionStatus.cs | 10 +- src/Daqifi.Core/Device/DaqifiDevice.cs | 463 ++++++++- .../Device/DaqifiStreamingDevice.cs | 119 +++ src/Daqifi.Core/Device/DeviceErrorSource.cs | 12 + .../Device/DeviceReconnectFailedException.cs | 35 + .../Device/ReconnectAttemptEventArgs.cs | 49 + .../Device/ReconnectFailedEventArgs.cs | 48 + src/Daqifi.Core/Device/ReconnectOptions.cs | 189 ++++ .../Device/ReconnectedEventArgs.cs | 45 + 12 files changed, 2050 insertions(+), 4 deletions(-) create mode 100644 src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs create mode 100644 src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs create mode 100644 src/Daqifi.Core/Device/DeviceReconnectFailedException.cs create mode 100644 src/Daqifi.Core/Device/ReconnectAttemptEventArgs.cs create mode 100644 src/Daqifi.Core/Device/ReconnectFailedEventArgs.cs create mode 100644 src/Daqifi.Core/Device/ReconnectOptions.cs create mode 100644 src/Daqifi.Core/Device/ReconnectedEventArgs.cs diff --git a/docs/DEVICE_INTERFACES.md b/docs/DEVICE_INTERFACES.md index 28631fc2..e0fe5f0b 100644 --- a/docs/DEVICE_INTERFACES.md +++ b/docs/DEVICE_INTERFACES.md @@ -436,12 +436,21 @@ device.StatusChanged += (sender, args) => Console.WriteLine("Device connected successfully"); break; case ConnectionStatus.Lost: - Console.WriteLine("Connection lost - may attempt reconnection"); + Console.WriteLine("Connection lost"); + break; + case ConnectionStatus.Retrying: + Console.WriteLine("Reconnecting..."); // only with reconnect enabled + break; + case ConnectionStatus.Failed: + Console.WriteLine("Reconnection gave up"); break; } }; ``` +See [Reconnecting automatically after a drop](#reconnecting-automatically-after-a-drop) for the +`Retrying` and `Failed` states, which a device only ever reports once reconnection is turned on. + ### Detecting a dropped connection `ConnectionStatus.Lost` means the connection ended without anyone asking it to — the USB cable was @@ -495,6 +504,87 @@ The failures that feed this escalation are also reported individually, as they h `IDevice.ErrorOccurred` — see [Error Surface](#error-surface). That event is diagnostics only; `ConnectionStatus.Lost` remains the single signal that means "the connection is over". +### Reconnecting automatically after a drop + +By default a drop is where the story ends: `Lost` is reported and nothing else happens. Set +`ReconnectOptions` and the device will rebuild the session by itself — reconnect the transport, +re-initialize, put the channel configuration back, and restart a stream that was interrupted — with +no code from you in the loop. + +```csharp +device.ReconnectOptions = ReconnectOptions.Default; // 5 attempts, 1 s backing off to 30 s + +device.Reconnected += (_, e) => + Console.WriteLine($"back after {e.Outage.TotalSeconds:0.#}s (attempt {e.AttemptNumber})"); + +device.ReconnectFailed += (_, e) => + Console.WriteLine($"gave up after {e.AttemptsMade} attempts: {e.LastError?.Message}"); +``` + +`ReconnectOptions.Fast` and `ReconnectOptions.Resilient` are ready-made policies for links that +blip briefly and for unattended long runs respectively; build your own for anything else. +`ReconnectOptions.Disabled` (the default) says so explicitly. + +**What you can watch.** `ReconnectAttempt` fires before each attempt with its number and the wait +that precedes it; `Reconnected` fires once the session is fully back; `ReconnectFailed` fires when +it stops without one. The status follows along, so a UI can show progress without subscribing to +anything new: + +| Status | Meaning | +|---|---| +| `Lost` | The drop was detected. Also where a cancelled reconnect leaves the device. | +| `Retrying` | Waiting out the backoff before the next attempt. | +| `Connected` | The session is back, configuration and stream included. | +| `Failed` | Every attempt failed. Terminal — nothing more will be tried. | + +Running out of attempts is deliberately hard to miss: as well as `ReconnectFailed` and the `Failed` +status, it is logged as an error and raised on `ErrorOccurred` with source +`DeviceErrorSource.Reconnect`, carrying a `DeviceReconnectFailedException` whose inner exception is +whatever ended the final attempt. + +**What gets restored.** Only what the library itself owns: + +- the set of enabled channels (analog and digital), +- the streaming frequency, and +- an active stream, unless `ResumeStreaming` is turned off. + +**What does not.** Everything else is the device's own state, and Core does not presume to know +what it should be after an outage of unknown length: + +- DIO directions and output levels, PWM enable/duty/frequency, analog outputs, and calibration + written only to device RAM; +- an SD card logging session — the device keeps logging or does not, entirely on its own; +- **any operation that was in flight.** An SD card download interrupted by a drop fails, and is + neither resumed nor retried; run it again once `Reconnected` says the device is back. + +A resumed stream is a genuinely new session: timestamp reconstruction re-anchors and the gap +detector resets, because the device's tick counter may well have restarted while it was away. +`Reconnected.Outage` is the measure of the interruption, not a `GapDetected` event. + +**Same endpoint only.** Reconnection re-opens the endpoint the device was already using. It cannot +follow a device that moved: a serial device that comes back on a different port path, or one whose +IP address changed after a reboot, is a new endpoint and needs a fresh `DaqifiDeviceFactory` +connect. Failing over from one transport to another (USB to WiFi, say) is out of scope. + +**Stopping it.** `CancelReconnect()` stops the loop at its next checkpoint and leaves the device on +`Lost`; `Disconnect()` and `Dispose()` do the same and then tear down. A caller-issued `Connect()` +or `Disconnect()` always wins — the loop unwinds without touching the session the caller +established. + +```csharp +device.ReconnectOptions = new ReconnectOptions +{ + Enabled = true, + MaxAttempts = 10, + InitialDelay = TimeSpan.FromSeconds(2), + MaxDelay = TimeSpan.FromMinutes(1), + ResumeStreaming = true +}; +``` + +All three events are raised on a background thread, and a handler that throws is caught and +ignored — it cannot stop a reconnect in progress. + ### Working with Device Metadata After initialization, device metadata is populated: @@ -855,6 +945,8 @@ streaming. - **Event-Driven**: Status changes and messages handled via events - **Observable Failures**: Background read and decode errors surface on `ErrorOccurred` instead of failing silently +- **Opt-in Auto-Reconnect**: A dropped connection can rebuild itself — transport, initialization, + channel configuration and stream — with no consumer code - **Type Safety**: Generic message types provide compile-time safety - **Retry Support**: Built-in connection retry with exponential backoff - **Thread-Safe Sending**: Background message queue for thread-safe command sending diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs new file mode 100644 index 00000000..190c6c39 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -0,0 +1,876 @@ +using Daqifi.Core.Channel; +using Daqifi.Core.Communication.Messages; +using Daqifi.Core.Communication.Transport; +using Daqifi.Core.Device; +using Google.Protobuf; + +namespace Daqifi.Core.Tests.Device; + +/// +/// Issue #379: after a mid-stream drop, a device with reconnect enabled has to rebuild the whole +/// session by itself — transport, initialization, channel configuration, and the stream — with no +/// consumer code involved. With reconnect left at its default it must do exactly what it has always +/// done: report and stop. +/// +/// +/// Driven by a scripted transport that can be dropped on command and told to refuse the next N +/// reconnects, so the loop's success, retry, give-up and cancellation paths are all reachable +/// without hardware. The one test that goes the long way round — real read failures escalated by +/// the production — is +/// . +/// +public class DeviceReconnectTests +{ + private static readonly TimeSpan EventTimeout = TimeSpan.FromSeconds(15); + + /// A policy that reconnects promptly, so tests do not spend their time waiting. + private static ReconnectOptions FastPolicy(int maxAttempts = 4) => new() + { + Enabled = true, + MaxAttempts = maxAttempts, + InitialDelay = TimeSpan.FromMilliseconds(10), + MaxDelay = TimeSpan.FromMilliseconds(60), + BackoffMultiplier = 2.0 + }; + + #region Default behaviour is unchanged + + [Fact] + public void WithReconnectAtItsDefault_ADropStopsAtLost() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Default Device", transport); + + ConnectAndInitialize(device); + + // Subscribed after the connect, so the recorded transitions are only what the drop caused. + var statuses = new List(); + device.StatusChanged += (_, e) => + { + lock (statuses) + { + statuses.Add(e.Status); + } + }; + + var reconnectEvents = 0; + device.ReconnectAttempt += (_, _) => Interlocked.Increment(ref reconnectEvents); + device.Reconnected += (_, _) => Interlocked.Increment(ref reconnectEvents); + device.ReconnectFailed += (_, _) => Interlocked.Increment(ref reconnectEvents); + + var connectsBeforeDrop = transport.ConnectCount; + + transport.SimulateDrop(); + + // Long enough that any reconnect worth having would have started by now. + Thread.Sleep(500); + + Assert.Equal(ConnectionStatus.Lost, device.Status); + Assert.False(device.IsReconnecting); + Assert.Equal(0, Volatile.Read(ref reconnectEvents)); + Assert.Equal(connectsBeforeDrop, transport.ConnectCount); + Assert.Equal(1, device.InitializeCount); + + lock (statuses) + { + // Exactly one transition, to Lost. No Retrying, no Failed, nothing else. + Assert.Equal(new[] { ConnectionStatus.Lost }, statuses); + } + } + + [Fact] + public void ReconnectIsOffOnAFreshDevice() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Fresh Device", transport); + + Assert.False(device.ReconnectOptions.Enabled); + Assert.False(device.IsReconnecting); + } + + #endregion + + #region The session comes back + + [Fact] + public async Task AfterADrop_TheSessionIsRebuiltWithNoConsumerInvolvement() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Resuming Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + var analog1 = device.Channels.Single(c => c.Type == ChannelType.Analog && c.ChannelNumber == 1); + var analog3 = device.Channels.Single(c => c.Type == ChannelType.Analog && c.ChannelNumber == 3); + var digital0 = device.Channels.Single(c => c.Type == ChannelType.Digital && c.ChannelNumber == 0); + device.EnableChannels(new[] { analog1, analog3, digital0 }); + + device.StreamingFrequency = 250; + device.StartStreaming(); + + var reconnected = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + + transport.SimulateDrop(); + + var result = await reconnected; + + // The device is connected, re-initialized and streaming again — nobody was asked to help. + Assert.Equal(ConnectionStatus.Connected, device.Status); + Assert.True(device.IsConnected); + Assert.Equal(2, device.InitializeCount); + Assert.True(device.IsStreaming); + Assert.True(result.StreamingResumed); + Assert.Equal(1, result.AttemptNumber); + Assert.True(result.Outage > TimeSpan.Zero); + Assert.False(device.IsReconnecting); + + // The channel configuration was replayed onto the reconnected device. It has to have been + // sent, not merely remembered: the scripted device reports every analog channel disabled + // when it comes back, exactly as a rebooted one does. + var sent = device.SentCommands; + var expectedMask = (1u << 1) | (1u << 3); + Assert.Contains($"ENAble:VOLTage:DC {expectedMask}", sent); + Assert.Contains("DIO:PORt:ENAble 1", sent); + + // ...and the stream restarted at the frequency it was running at before the drop. + Assert.Contains("SYSTem:StartStreamData 250", sent); + + Assert.True(device.Channels.Single(c => c.Type == ChannelType.Analog && c.ChannelNumber == 1).IsEnabled); + Assert.True(device.Channels.Single(c => c.Type == ChannelType.Analog && c.ChannelNumber == 3).IsEnabled); + Assert.False(device.Channels.Single(c => c.Type == ChannelType.Analog && c.ChannelNumber == 0).IsEnabled); + Assert.True(device.Channels.Single(c => c.Type == ChannelType.Digital && c.ChannelNumber == 0).IsEnabled); + } + + [Fact] + public async Task ADeviceThatWasNotStreaming_ComesBackIdle() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Idle Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + device.EnableChannel(device.Channels.First(c => c.Type == ChannelType.Analog)); + + var reconnected = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + + transport.SimulateDrop(); + var result = await reconnected; + + Assert.False(result.StreamingResumed); + Assert.False(device.IsStreaming); + Assert.DoesNotContain(device.SentCommands, c => c.StartsWith("SYSTem:StartStreamData", StringComparison.Ordinal)); + + // The channel configuration is restored regardless of whether a stream was running. + Assert.Contains("ENAble:VOLTage:DC 1", device.SentCommands); + } + + [Fact] + public async Task WithResumeStreamingOff_TheChannelsComeBackButTheStreamDoesNot() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Manual Resume Device", transport); + var policy = FastPolicy(); + policy.ResumeStreaming = false; + device.ReconnectOptions = policy; + + ConnectAndInitialize(device); + device.EnableChannel(device.Channels.First(c => c.Type == ChannelType.Analog)); + device.StartStreaming(); + + var reconnected = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + + transport.SimulateDrop(); + var result = await reconnected; + + Assert.False(result.StreamingResumed); + Assert.Contains("ENAble:VOLTage:DC 1", device.SentCommands); + Assert.DoesNotContain(device.SentCommands, c => c.StartsWith("SYSTem:StartStreamData", StringComparison.Ordinal)); + } + + [Fact] + public async Task ADeviceThatRefusesTheFirstAttempts_IsRetriedUntilItAnswers() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Slow-To-Return Device", transport); + device.ReconnectOptions = FastPolicy(maxAttempts: 5); + + ConnectAndInitialize(device); + device.EnableChannel(device.Channels.First(c => c.Type == ChannelType.Analog)); + + var attempts = new List(); + device.ReconnectAttempt += (_, e) => + { + lock (attempts) + { + attempts.Add(e); + } + }; + + var reconnected = WaitFor(h => device.Reconnected += h); + + // The device is still coming back up: the next two connects are refused. + transport.FailNextConnects(2); + transport.SimulateDrop(); + + var result = await reconnected; + + Assert.Equal(3, result.AttemptNumber); + + lock (attempts) + { + Assert.Equal(3, attempts.Count); + Assert.Equal(new[] { 1, 2, 3 }, attempts.Select(a => a.AttemptNumber)); + Assert.All(attempts, a => Assert.Equal(5, a.MaxAttempts)); + + // Backing off, not hammering: each wait is longer than the last. + Assert.True(attempts[1].Delay > attempts[0].Delay); + Assert.True(attempts[2].Delay > attempts[1].Delay); + + // The first attempt has nothing to report; the later ones say why the previous failed. + Assert.Null(attempts[0].PreviousError); + Assert.NotNull(attempts[1].PreviousError); + Assert.NotNull(attempts[2].PreviousError); + } + } + + [Fact] + public async Task AMidStreamReadFailure_DrivesTheWholeLoopEndToEnd() + { + // The long way round: no simulated drop, just reads that start failing. The production + // watchdog escalates them to Lost, which is what the reconnect loop hangs off. + using var transport = new ScriptedReconnectTransport(useWatchdog: true); + using var device = new ScriptedStreamingDevice("Unplugged Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + device.EnableChannel(device.Channels.First(c => c.Type == ChannelType.Analog)); + device.StartStreaming(); + + var reconnected = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + + transport.FailReadsUntilNextConnect(); + + var result = await reconnected; + + Assert.True(result.StreamingResumed); + Assert.Equal(ConnectionStatus.Connected, device.Status); + Assert.Contains("ENAble:VOLTage:DC 1", device.SentCommands); + Assert.Contains(device.SentCommands, c => c.StartsWith("SYSTem:StartStreamData", StringComparison.Ordinal)); + } + + #endregion + + #region Giving up + + [Fact] + public async Task WhenEveryAttemptFails_ItGivesUpLoudly() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Gone Device", transport); + device.ReconnectOptions = FastPolicy(maxAttempts: 3); + + ConnectAndInitialize(device); + + DeviceErrorEventArgs? deviceError = null; + device.ErrorOccurred += (_, e) => deviceError = e; + + var failed = WaitFor(h => device.ReconnectFailed += h); + + transport.FailNextConnects(int.MaxValue); + transport.SimulateDrop(); + + var result = await failed; + + Assert.Equal(3, result.AttemptsMade); + Assert.False(result.WasCanceled); + Assert.NotNull(result.LastError); + + // Terminal, and impossible to miss: a distinct status plus the device error surface. + Assert.Equal(ConnectionStatus.Failed, device.Status); + Assert.False(device.IsConnected); + + Assert.NotNull(deviceError); + Assert.Equal(DeviceErrorSource.Reconnect, deviceError!.Source); + var reconnectFailure = Assert.IsType(deviceError.Error); + Assert.Equal(3, reconnectFailure.AttemptsMade); + Assert.Equal("Gone Device", reconnectFailure.DeviceName); + Assert.NotNull(reconnectFailure.InnerException); + + // It really did stop: no further attempts after the report. + var connectsAtGiveUp = transport.ConnectCount; + Thread.Sleep(300); + Assert.Equal(connectsAtGiveUp, transport.ConnectCount); + Assert.False(device.IsReconnecting); + } + + [Fact] + public async Task AnInitializationThatKeepsFailing_AlsoExhaustsTheAttempts() + { + // The transport comes back every time; it is the device that will not initialize. The loop + // must not treat a reachable-but-unusable device as a success. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Unresponsive Device", transport); + device.ReconnectOptions = FastPolicy(maxAttempts: 2); + + ConnectAndInitialize(device); + + // Set after the first (successful) initialization, so only the reconnect's attempts fail. + device.InitializeFailure = _ => + new TimeoutException("the device never reported its channel configuration"); + + var failed = WaitFor(h => device.ReconnectFailed += h); + transport.SimulateDrop(); + + var result = await failed; + + Assert.Equal(2, result.AttemptsMade); + Assert.IsType(result.LastError); + Assert.Equal(ConnectionStatus.Failed, device.Status); + + // Nothing half-open is left behind: the transport it managed to open was closed again. + Assert.False(transport.IsConnected); + } + + #endregion + + #region Cancellation + + [Fact] + public async Task CancelReconnect_StopsTheLoopAndLeavesTheDeviceLost() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Abandoned Device", transport); + var policy = FastPolicy(maxAttempts: 20); + policy.InitialDelay = TimeSpan.FromSeconds(30); + policy.MaxDelay = TimeSpan.FromSeconds(30); + device.ReconnectOptions = policy; + + ConnectAndInitialize(device); + + var failed = WaitFor(h => device.ReconnectFailed += h); + + transport.FailNextConnects(int.MaxValue); + transport.SimulateDrop(); + + // Wait until the loop is parked in its 30 s backoff, so cancellation lands at a known point + // rather than racing the loop's own teardown. + WaitUntilRetrying(device); + Assert.True(device.IsReconnecting); + + device.CancelReconnect(); + + var result = await failed; + + // The 30 s backoff means cancellation, not exhaustion, is what ended this. + Assert.True(result.WasCanceled); + Assert.Equal(1, result.AttemptsMade); + Assert.Equal(ConnectionStatus.Lost, device.Status); + Assert.False(device.IsReconnecting); + Assert.False(device.IsConnected); + } + + [Fact] + public async Task Disconnect_DuringAReconnect_WinsAndTheLoopStopsQuietly() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Departing Device", transport); + var policy = FastPolicy(maxAttempts: 20); + policy.InitialDelay = TimeSpan.FromSeconds(30); + policy.MaxDelay = TimeSpan.FromSeconds(30); + device.ReconnectOptions = policy; + + ConnectAndInitialize(device); + + var failed = WaitFor(h => device.ReconnectFailed += h); + + transport.FailNextConnects(int.MaxValue); + transport.SimulateDrop(); + WaitUntilRetrying(device); + + device.Disconnect(); + + var result = await failed; + Assert.True(result.WasCanceled); + + // The caller's teardown owns the outcome: the loop must not have overwritten it, nor + // re-opened the transport behind it. + Thread.Sleep(300); + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(transport.IsConnected); + Assert.False(device.IsReconnecting); + } + + [Fact] + public void DisposingDuringAReconnect_DoesNotThrow() + { + var transport = new ScriptedReconnectTransport(); + var device = new ScriptedStreamingDevice("Disposed Device", transport); + device.ReconnectOptions = FastPolicy(maxAttempts: 20); + + ConnectAndInitialize(device); + + transport.FailNextConnects(int.MaxValue); + transport.SimulateDrop(); + WaitUntilRetrying(device); + + device.Dispose(); + transport.Dispose(); + + // Whatever the loop was in the middle of, it unwinds without surfacing anything. + Thread.Sleep(400); + } + + #endregion + + #region Robustness + + [Fact] + public async Task AThrowingReconnectSubscriber_DoesNotStopTheLoop() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Badly Observed Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + device.ReconnectAttempt += (_, _) => throw new InvalidOperationException("a badly behaved subscriber"); + + var reconnected = WaitFor(h => device.Reconnected += h); + transport.SimulateDrop(); + + var result = await reconnected; + Assert.Equal(1, result.AttemptNumber); + Assert.Equal(ConnectionStatus.Connected, device.Status); + } + + [Fact] + public async Task ASecondDropAfterARecovery_StartsAFreshLoop() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Flaky Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + device.EnableChannel(device.Channels.First(c => c.Type == ChannelType.Analog)); + device.StartStreaming(); + + var first = WaitFor(h => device.Reconnected += h); + transport.SimulateDrop(); + await first; + + // Reconnected is raised from inside the loop; let it finish unwinding so the second drop is + // unambiguously a fresh one rather than one folded into the loop still in flight. + WaitUntil(() => !device.IsReconnecting, "the first reconnect loop never finished"); + + var second = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + transport.SimulateDrop(); + var result = await second; + + Assert.True(result.StreamingResumed); + Assert.Equal(3, device.InitializeCount); + Assert.Contains("ENAble:VOLTage:DC 1", device.SentCommands); + } + + [Fact] + public void SettingReconnectOptionsToNull_IsRejected() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Picky Device", transport); + + Assert.Throws(() => device.ReconnectOptions = null!); + } + + [Fact] + public void CancelReconnect_WithNothingRunning_IsHarmless() + { + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Quiet Device", transport); + + device.CancelReconnect(); + device.CancelReconnect(); + + Assert.False(device.IsReconnecting); + } + + #endregion + + #region Helpers + + private static void ConnectAndInitialize(ScriptedStreamingDevice device) + { + device.Connect(); + device.InitializeAsync().GetAwaiter().GetResult(); + } + + /// + /// Blocks until the reconnect loop has torn the dead session down and is waiting out its + /// backoff, which is the only point at which a test can cancel without racing that teardown. + /// + private static void WaitUntilRetrying(DaqifiStreamingDevice device) => + WaitUntil( + () => device.Status == ConnectionStatus.Retrying, + "the reconnect loop never reached its backoff wait"); + + private static void WaitUntil(Func condition, string because) + { + var deadline = DateTime.UtcNow + EventTimeout; + while (DateTime.UtcNow < deadline) + { + if (condition()) + { + return; + } + + Thread.Sleep(5); + } + + Assert.True(condition(), because); + } + + /// + /// Subscribes to an event and returns a task that completes with its first payload, so a test + /// can arm the wait before provoking the drop that satisfies it. + /// + private static Task WaitFor(Action> subscribe) + where TArgs : EventArgs + { + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + subscribe((_, e) => tcs.TrySetResult(e)); + + return WaitWithTimeoutAsync(tcs.Task); + } + + private static async Task WaitWithTimeoutAsync(Task task) + { + var completed = await Task.WhenAny(task, Task.Delay(EventTimeout)).ConfigureAwait(false); + Assert.True(completed == task, $"timed out waiting for {typeof(TArgs).Name}"); + return await task.ConfigureAwait(false); + } + + /// + /// A streaming device with no real SCPI: commands are captured rather than written, and + /// initialization synthesizes the status message a device would have sent. + /// + private sealed class ScriptedStreamingDevice : DaqifiStreamingDevice + { + private readonly List _sent = new(); + private int _initializeCount; + + public ScriptedStreamingDevice(string name, IStreamTransport transport) + : base(name, transport) + { + } + + /// Number of analog channels the scripted device reports. + public int AnalogChannelCount { get; init; } = 4; + + /// Number of digital channels the scripted device reports. + public int DigitalChannelCount { get; init; } = 2; + + /// + /// Optional failure injector, keyed by 1-based initialization count, so a test can let the + /// first (pre-drop) initialization succeed and fail every later one. + /// + public Func? InitializeFailure { get; set; } + + public int InitializeCount => Volatile.Read(ref _initializeCount); + + public IReadOnlyList SentCommands + { + get + { + lock (_sent) + { + return _sent.ToArray(); + } + } + } + + public void ClearSentCommands() + { + lock (_sent) + { + _sent.Clear(); + } + } + + public override void Send(IOutboundMessage message) + { + if (message is IOutboundMessage stringMessage) + { + lock (_sent) + { + _sent.Add(stringMessage.Data); + } + } + } + + public override Task InitializeAsync( + TimeSpan? channelPopulationTimeout = null, + CancellationToken cancellationToken = default) + { + var attempt = Interlocked.Increment(ref _initializeCount); + + cancellationToken.ThrowIfCancellationRequested(); + + if (!IsConnected) + { + return Task.FromException(new DeviceNotConnectedException()); + } + + var failure = InitializeFailure?.Invoke(attempt); + if (failure != null) + { + return Task.FromException(failure); + } + + PopulateChannelsFromStatus(BuildStatusMessage()); + return Task.CompletedTask; + } + + /// + /// The status a device sends after a fresh boot: channels present, and — crucially — + /// analog_in_port_enabled reported as all-zero. A restored enable set therefore + /// cannot be an in-memory leftover; it has to have been re-applied. + /// + private DaqifiOutMessage BuildStatusMessage() + { + var status = new DaqifiOutMessage + { + AnalogInPortNum = (uint)AnalogChannelCount, + AnalogInRes = 65535, + DigitalPortNum = (uint)DigitalChannelCount, + AnalogInPortEnabled = ByteString.CopyFrom(new byte[] { 0x00, 0x00 }) + }; + + for (var i = 0; i < AnalogChannelCount; i++) + { + status.AnalogInPortRange.Add(1.0f); + status.AnalogInCalM.Add(1.0f); + status.AnalogInCalB.Add(0.0f); + status.AnalogInIntScaleM.Add(1.0f); + } + + return status; + } + } + + /// + /// A transport that can be dropped on command and told to refuse the next N reconnects, so the + /// whole reconnect loop is reachable without hardware. Each connect hands out a fresh + /// stream, matching the real serial transport, whose BaseStream is a new instance after a + /// reopen. + /// + private sealed class ScriptedReconnectTransport : IStreamTransport, ITransportHealthSink + { + private readonly object _gate = new(); + private readonly TransportConnectionWatchdog? _watchdog; + private IdleStream _stream = new(); + private bool _isConnected; + private bool _disposed; + private int _connectFailuresRemaining; + private int _connectCount; + + public ScriptedReconnectTransport(bool useWatchdog = false) + { + if (useWatchdog) + { + _watchdog = new TransportConnectionWatchdog("Scripted transport", HandleConnectionLost); + } + } + + public int ConnectCount => Volatile.Read(ref _connectCount); + + public Stream Stream + { + get + { + lock (_gate) + { + ObjectDisposedException.ThrowIf(_disposed, this); + return _stream; + } + } + } + + public bool IsConnected + { + get + { + lock (_gate) + { + return _isConnected && !_disposed; + } + } + } + + public string ConnectionInfo => IsConnected ? "Scripted: Connected" : "Scripted: Disconnected"; + + public event EventHandler? StatusChanged; + + /// Refuses the next connect attempts. + public void FailNextConnects(int count) + { + lock (_gate) + { + _connectFailuresRemaining = count; + } + } + + /// + /// Makes reads start failing, as a pulled cable does, until the next successful connect + /// hands out a fresh stream. + /// + public void FailReadsUntilNextConnect() + { + lock (_gate) + { + _stream.FailReads = true; + } + } + + /// Reports a drop the way a transport's own detection would. + public void SimulateDrop() + { + lock (_gate) + { + _isConnected = false; + } + + _watchdog?.Disarm(); + StatusChanged?.Invoke( + this, + new TransportStatusEventArgs(false, ConnectionInfo, new IOException("the device went away"))); + } + + public Task ConnectAsync() => ConnectAsync(null); + + public Task ConnectAsync(ConnectionRetryOptions? retryOptions) + { + Interlocked.Increment(ref _connectCount); + + lock (_gate) + { + ObjectDisposedException.ThrowIf(_disposed, this); + + if (_connectFailuresRemaining > 0) + { + _connectFailuresRemaining--; + throw new IOException("the device is not back yet"); + } + + // A reopened transport is a new stream; binding a consumer to the old one is + // exactly the bug the device's Disconnect/Connect cycle exists to avoid. + _stream.Dispose(); + _stream = new IdleStream(); + _isConnected = true; + } + + _watchdog?.Arm(); + StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); + return Task.CompletedTask; + } + + public Task DisconnectAsync() + { + // Disarm first: closing the handle is what makes in-flight reads fail, and none of that + // is a lost connection. + _watchdog?.Disarm(); + + lock (_gate) + { + _isConnected = false; + } + + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + return Task.CompletedTask; + } + + public void Connect() => ConnectAsync().GetAwaiter().GetResult(); + + public void Disconnect() => DisconnectAsync().GetAwaiter().GetResult(); + + public void ReportIoFault(Exception error) => _watchdog?.RecordFault(error); + + public void ReportIoSuccess() => _watchdog?.RecordSuccess(); + + private void HandleConnectionLost(Exception error) + { + lock (_gate) + { + _isConnected = false; + } + + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo, error)); + } + + public void Dispose() + { + lock (_gate) + { + if (_disposed) + { + return; + } + + _disposed = true; + _isConnected = false; + _stream.Dispose(); + } + + _watchdog?.Dispose(); + } + } + + /// + /// A stream that idles quietly — the shape a connected device with nothing to say presents to + /// the reader loop — until told to fail its reads. + /// + private sealed class IdleStream : Stream + { + public volatile bool FailReads; + + public override int Read(byte[] buffer, int offset, int count) + { + if (FailReads) + { + Thread.Sleep(5); + throw new IOException("the device is gone"); + } + + // Not a socket: a zero-byte read means "nothing yet", never a fault. + Thread.Sleep(20); + return 0; + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) + { + } + } + + #endregion +} diff --git a/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs b/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs new file mode 100644 index 00000000..4f355034 --- /dev/null +++ b/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs @@ -0,0 +1,114 @@ +using Daqifi.Core.Device; + +namespace Daqifi.Core.Tests.Device; + +/// +/// The reconnect policy itself (issue #379): its defaults, its backoff arithmetic, and the values +/// it refuses. A policy that accepted "zero attempts" or a negative delay would turn into a device +/// that quietly never reconnects, or a loop that never waits. +/// +public class ReconnectOptionsTests +{ + [Fact] + public void ANewPolicyIsOff() + { + var options = new ReconnectOptions(); + + Assert.False(options.Enabled); + Assert.True(options.ResumeStreaming); + Assert.Equal(5, options.MaxAttempts); + } + + [Fact] + public void ThePresetsSayWhatTheyMean() + { + Assert.False(ReconnectOptions.Disabled.Enabled); + Assert.True(ReconnectOptions.Default.Enabled); + Assert.True(ReconnectOptions.Fast.Enabled); + Assert.True(ReconnectOptions.Resilient.Enabled); + + // Resilient keeps trying for far longer than Fast does. + Assert.True(ReconnectOptions.Resilient.MaxAttempts > ReconnectOptions.Fast.MaxAttempts); + Assert.True(ReconnectOptions.Resilient.MaxDelay > ReconnectOptions.Fast.MaxDelay); + } + + [Fact] + public void TheFirstAttemptStillWaits() + { + // Unlike the initial-connect retry policy: at the instant a drop is detected the endpoint is + // gone by definition, so trying immediately is guaranteed to fail. + var options = new ReconnectOptions { InitialDelay = TimeSpan.FromSeconds(2) }; + + Assert.Equal(TimeSpan.FromSeconds(2), options.CalculateDelay(1)); + } + + [Fact] + public void TheDelayBacksOffAndThenStopsGrowing() + { + var options = new ReconnectOptions + { + InitialDelay = TimeSpan.FromSeconds(1), + BackoffMultiplier = 2.0, + MaxDelay = TimeSpan.FromSeconds(5) + }; + + Assert.Equal(TimeSpan.FromSeconds(1), options.CalculateDelay(1)); + Assert.Equal(TimeSpan.FromSeconds(2), options.CalculateDelay(2)); + Assert.Equal(TimeSpan.FromSeconds(4), options.CalculateDelay(3)); + + // Capped from here on, however far the attempt count runs. + Assert.Equal(TimeSpan.FromSeconds(5), options.CalculateDelay(4)); + Assert.Equal(TimeSpan.FromSeconds(5), options.CalculateDelay(50)); + Assert.Equal(TimeSpan.FromSeconds(5), options.CalculateDelay(10_000)); + } + + [Fact] + public void AMultiplierOfOneGivesAFixedDelay() + { + var options = new ReconnectOptions + { + InitialDelay = TimeSpan.FromMilliseconds(750), + BackoffMultiplier = 1.0 + }; + + Assert.Equal(TimeSpan.FromMilliseconds(750), options.CalculateDelay(1)); + Assert.Equal(TimeSpan.FromMilliseconds(750), options.CalculateDelay(9)); + } + + [Fact] + public void AZeroInitialDelayStaysZero() + { + var options = new ReconnectOptions { InitialDelay = TimeSpan.Zero }; + + Assert.Equal(TimeSpan.Zero, options.CalculateDelay(1)); + Assert.Equal(TimeSpan.Zero, options.CalculateDelay(7)); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void APolicyThatWouldNeverTry_IsRejected(int maxAttempts) + { + Assert.Throws( + () => new ReconnectOptions { MaxAttempts = maxAttempts }); + } + + [Fact] + public void NegativeDelaysAreRejected() + { + Assert.Throws( + () => new ReconnectOptions { InitialDelay = TimeSpan.FromSeconds(-1) }); + Assert.Throws( + () => new ReconnectOptions { MaxDelay = TimeSpan.FromSeconds(-1) }); + } + + [Theory] + [InlineData(0.5)] + [InlineData(0.0)] + [InlineData(double.NaN)] + public void ABackoffThatShrinksOrIsNotANumber_IsRejected(double multiplier) + { + Assert.Throws( + () => new ReconnectOptions { BackoffMultiplier = multiplier }); + } +} diff --git a/src/Daqifi.Core/Device/ConnectionStatus.cs b/src/Daqifi.Core/Device/ConnectionStatus.cs index 90ebb408..b4345946 100644 --- a/src/Daqifi.Core/Device/ConnectionStatus.cs +++ b/src/Daqifi.Core/Device/ConnectionStatus.cs @@ -26,12 +26,18 @@ public enum ConnectionStatus Lost, /// - /// The device is retrying connection after a failure. + /// The device is retrying connection after a failure — in particular, it is between + /// automatic reconnect attempts after a drop (see ). Only + /// ever seen when reconnect is enabled; with the default policy a drop stops at + /// . /// Retrying, /// - /// The device connection failed after all retry attempts. + /// The device connection failed after all retry attempts — automatic reconnection ran out + /// of attempts and gave up (see ). Terminal: + /// nothing further will be attempted without a new Connect(). Only ever seen when + /// reconnect is enabled. /// Failed } diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 27eb90f6..cd00d069 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -554,7 +554,21 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n /// /// Connects to the device. /// + /// + /// A caller-issued connect supersedes any automatic reconnect in progress: the loop is + /// cancelled and unwinds without touching the session this call establishes. + /// public void Connect() + { + SupersedeReconnect(); + ConnectCore(); + } + + /// + /// The body of , without the reconnect-supersede step — the reconnect + /// loop calls this so it does not cancel itself. + /// + private void ConnectCore() { Status = ConnectionStatus.Connecting; State = DeviceState.Connecting; @@ -633,6 +647,23 @@ public void Connect() /// disconnect should drive this off a Task.Run. /// public void Disconnect() + { + // A user-issued teardown always beats an automatic reconnect: stop the loop before + // tearing anything down, so it cannot re-open the transport behind the caller's back. + SupersedeReconnect(); + DisconnectCore(ConnectionStatus.Disconnected); + } + + /// + /// The body of , parameterized by the status the device settles on. + /// + /// + /// The status to report once teardown is done. + /// for a caller-issued disconnect; for the teardown + /// the reconnect loop performs between attempts, which must not look to consumers like the + /// session ended on purpose. + /// + private void DisconnectCore(ConnectionStatus finalStatus) { _isDisconnecting = true; // Best-effort coordination with ExecuteTextCommandAsync — @@ -687,7 +718,7 @@ public void Disconnect() } finally { - Status = ConnectionStatus.Disconnected; + Status = finalStatus; State = DeviceState.Disconnected; _isInitialized = false; _isDisconnecting = false; @@ -1472,11 +1503,441 @@ private void OnTransportStatusChanged(object? sender, TransportStatusEventArgs e // not during an intentional Disconnect() call if (Status == ConnectionStatus.Connected && !_isDisconnecting) { + // Snapshot the session BEFORE anything observes the drop. Raising Lost runs + // consumer handlers synchronously on this thread, and a handler is entirely + // entitled to start tearing the device down — by which point what the session + // looked like is no longer recoverable. No-op unless reconnect is enabled, so + // the default path does exactly what it did before (issue #379). + // + // Skipped while a reconnect is already running: a drop during an attempt would + // otherwise overwrite the snapshot of the session being restored with the empty + // half-built one, and the loop would go on to restore nothing. + if (ReconnectOptions.Enabled && !IsReconnecting) + { + try + { + CaptureSessionSnapshot(); + } + catch (Exception ex) + { + SafeLog(() => _logger.LogWarning( + ex, "[Reconnect] Capturing the session state after a drop failed; the session cannot be restored.")); + } + } + Status = ConnectionStatus.Lost; + + BeginReconnectIfEnabled(); + } + } + } + + #region Automatic reconnection (issue #379) + + private ReconnectOptions _reconnectOptions = new(); + + /// + /// Gets or sets the policy for re-establishing a session after + /// . Disabled by default: a drop is reported and nothing + /// else happens, exactly as it always has been. + /// + /// + /// + /// With set, a detected drop starts a background + /// loop that reconnects the same endpoint, re-runs , + /// and restores the session state Core owns — the channel enable set, the streaming + /// frequency, and an interrupted stream. Progress arrives on + /// / / and as + /// between attempts. + /// + /// + /// Deliberately not restored: anything the device owns rather than Core (DIO + /// directions and output levels, PWM state, analog outputs, calibration written to RAM), an + /// SD logging session, and any in-flight operation — an SD download interrupted by a drop + /// fails, and is not resumed or retried. + /// + /// + /// Same-endpoint only. A serial device that comes back on a different port path, or a + /// device whose IP address changed, is a new endpoint and needs a fresh + /// DaqifiDeviceFactory connect. Failing over to a different transport is out of + /// scope entirely. + /// + /// + /// Thrown when set to null. + public ReconnectOptions ReconnectOptions + { + get => _reconnectOptions; + set + { + ArgumentNullException.ThrowIfNull(value); + _reconnectOptions = value; + } + } + + /// + /// Occurs before each automatic reconnect attempt, carrying the attempt number and the + /// backoff wait that precedes it. + /// + /// + /// Raised on a background thread, like every event in this group. Handlers should do the + /// minimum; one that throws is caught and ignored. + /// + public event EventHandler? ReconnectAttempt; + + /// + /// Occurs once an automatic reconnect has restored the session — the device is connected, + /// re-initialized, its channel configuration re-applied, and an interrupted stream running + /// again. + /// + public event EventHandler? Reconnected; + + /// + /// Occurs when automatic reconnection stops without restoring the session, because it ran + /// out of attempts or was cancelled. + /// + /// + /// Running out of attempts is also raised on with + /// and leaves the device on + /// , so giving up is impossible to miss even with + /// nothing subscribed here. Cancellation is not an error and does neither. + /// + public event EventHandler? ReconnectFailed; + + // 0 = idle, 1 = a reconnect loop is running. Guards against a second loop being started by + // the Lost that a failing attempt's own teardown can produce. + private int _reconnectRunning; + + private CancellationTokenSource? _reconnectCts; + + // Bumped by every caller-issued Connect/Disconnect/Dispose. The reconnect loop captures it + // when it starts and re-checks before each step that touches device state; a change means + // the caller has moved on and the loop must unwind without touching anything. This is what + // keeps the loop from re-opening a transport the caller just closed, without Disconnect() + // having to block on a loop that may be calling back into it. + private int _sessionEpoch; + + /// + /// Gets a value indicating whether an automatic reconnect is currently in progress. + /// + public bool IsReconnecting => Volatile.Read(ref _reconnectRunning) != 0; + + /// + /// Stops any automatic reconnect in progress. Safe to call at any time, including when + /// nothing is reconnecting. + /// + /// + /// The loop unwinds at its next checkpoint — it does not interrupt a connect attempt + /// already in flight — and reports with + /// set. The device is left on + /// : the connection really is gone, and nothing is + /// trying to bring it back. This returns immediately rather than waiting for the loop, so + /// it is safe to call from a device event handler. + /// + public void CancelReconnect() + { + try + { + _reconnectCts?.Cancel(); + } + catch (ObjectDisposedException) + { + // The loop finished and disposed its own token source. Nothing to cancel. + } + } + + /// + /// Cancels any reconnect in progress and declares the session it was rebuilding + /// obsolete, so the unwinding loop leaves the caller's new session strictly alone. + /// + private void SupersedeReconnect() + { + Interlocked.Increment(ref _sessionEpoch); + CancelReconnect(); + } + + /// + /// Starts the reconnect loop on a background thread, if the policy allows one and none is + /// already running. + /// + /// + /// Called from the transport's status callback, which runs on the reader loop or the + /// liveness timer — so the work is handed to the thread pool rather than done inline. + /// + private void BeginReconnectIfEnabled() + { + if (!ReconnectOptions.Enabled || _transport == null || _disposed || _isDisconnecting) + { + return; + } + + // One loop at a time. A failing attempt tears the transport down again, which can + // produce another Lost; without this that would fork a second loop racing the first. + if (Interlocked.CompareExchange(ref _reconnectRunning, 1, 0) != 0) + { + return; + } + + var cts = new CancellationTokenSource(); + _reconnectCts = cts; + + var epoch = Volatile.Read(ref _sessionEpoch); + var options = ReconnectOptions; + + _ = Task.Run(async () => + { + var wasCanceled = false; + + try + { + await RunReconnectLoopAsync(options, epoch, cts.Token).ConfigureAwait(false); } + catch (Exception ex) + { + // The loop handles its own failures; anything escaping is a bug in it, and must + // not become an unobserved task exception. + SafeLog(() => _logger.LogError(ex, "[Reconnect] The reconnect loop terminated unexpectedly.")); + } + finally + { + wasCanceled = cts.IsCancellationRequested; + + // Clear the token source before releasing the running flag, so a loop started + // by the next drop cannot have its own source nulled out from under it. + _reconnectCts = null; + cts.Dispose(); + Interlocked.Exchange(ref _reconnectRunning, 0); + } + + // A drop that landed in the moments between this loop finishing its work and + // releasing the running flag would have been skipped by the single-flight guard, + // stranding the device on Lost with nothing trying to bring it back. Pick it up. + // Exhausting the attempts settles on Failed, and cancellation is excluded above, so + // neither can retrigger here. + if (!wasCanceled && Status == ConnectionStatus.Lost) + { + BeginReconnectIfEnabled(); + } + }); + } + + /// + /// Attempts, with backoff, to rebuild the session that was just lost. + /// + /// + /// The policy snapshotted when the drop was detected, so a mid-flight change to + /// cannot alter the loop's terms underneath it. + /// + /// The session epoch at the time of the drop. + /// Cancelled by . + private async Task RunReconnectLoopAsync( + ReconnectOptions options, + int epoch, + CancellationToken cancellationToken) + { + var startedAt = DateTime.UtcNow; + Exception? lastError = null; + var attempt = 0; + + SafeLog(() => _logger.LogInformation( + "[Reconnect] Device '{DeviceName}' lost its connection; reconnecting (up to {MaxAttempts} attempt(s)).", + Name, + options.MaxAttempts)); + + while (attempt < options.MaxAttempts) + { + attempt++; + + if (IsSessionStale(epoch) || cancellationToken.IsCancellationRequested) + { + ReportReconnectStopped(epoch, attempt - 1, lastError, wasCanceled: true); + return; + } + + var delay = options.CalculateDelay(attempt); + RaiseReconnectEvent(ReconnectAttempt, new ReconnectAttemptEventArgs( + attempt, options.MaxAttempts, delay, lastError), nameof(ReconnectAttempt)); + + try + { + // Tear down what is left of the dead session first: the producer and consumer + // are still bound to a stream that is gone, and Connect() only rebuilds them + // once they have been nulled. Reported as Retrying, not Disconnected — nobody + // asked for this teardown. + DisconnectCore(ConnectionStatus.Retrying); + + if (delay > TimeSpan.Zero) + { + await Task.Delay(delay, cancellationToken).ConfigureAwait(false); + } + + if (IsSessionStale(epoch)) + { + ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: true); + return; + } + + cancellationToken.ThrowIfCancellationRequested(); + + ConnectCore(); + + // The caller took the session over while the connect was in flight. Whatever + // they did next owns the device now, so stop here without touching status or + // tearing anything down — either could undo the session they just established. + if (IsSessionStale(epoch)) + { + ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: true); + return; + } + + await InitializeAsync(cancellationToken: cancellationToken).ConfigureAwait(false); + + var streamingResumed = await RestoreSessionSnapshotAsync( + options, cancellationToken).ConfigureAwait(false); + + SafeLog(() => _logger.LogInformation( + "[Reconnect] Device '{DeviceName}' reconnected on attempt {Attempt}.", Name, attempt)); + + RaiseReconnectEvent(Reconnected, new ReconnectedEventArgs( + attempt, DateTime.UtcNow - startedAt, streamingResumed), nameof(Reconnected)); + return; + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: true); + return; + } + catch (Exception ex) + { + lastError = ex; + SafeLog(() => _logger.LogWarning( + ex, + "[Reconnect] Attempt {Attempt} of {MaxAttempts} to reconnect device '{DeviceName}' failed.", + attempt, + options.MaxAttempts, + Name)); + } + } + + ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: false); + } + + /// + /// True once a caller-issued connect, disconnect or disposal has superseded the session the + /// reconnect loop was started for. + /// + /// + /// Deliberately does not consult _isDisconnecting: the loop's own teardown between + /// attempts sets that flag, so reading it here would make the loop consider itself stale. + /// A caller-issued bumps the epoch before it sets the flag, which + /// is what this actually needs to see. + /// + private bool IsSessionStale(int epoch) => + _disposed || Volatile.Read(ref _sessionEpoch) != epoch; + + /// + /// Settles the device after a reconnect loop that did not restore the session, and reports + /// why. + /// + /// + /// Exhausting the attempts is terminal and loud: , a + /// logged error, and an raise. Being cancelled is neither — the + /// caller asked for it — so the device is simply left reporting + /// , which is the truth. Neither touches + /// at all once the session is stale, since by then the status belongs + /// to whatever the caller did next. + /// + private void ReportReconnectStopped(int epoch, int attemptsMade, Exception? lastError, bool wasCanceled) + { + if (!IsSessionStale(epoch)) + { + // An attempt can fail after the transport is back up (a re-initialization that + // times out, say), so tear the half-built session down rather than leaving a live + // handle and a running reader behind a terminal status. + DisconnectCore(wasCanceled ? ConnectionStatus.Lost : ConnectionStatus.Failed); + } + + if (wasCanceled) + { + SafeLog(() => _logger.LogInformation( + "[Reconnect] Reconnection of device '{DeviceName}' was cancelled after {AttemptsMade} attempt(s).", + Name, + attemptsMade)); + } + else + { + SafeLog(() => _logger.LogError( + lastError, + "[Reconnect] Device '{DeviceName}' could not be reconnected after {AttemptsMade} attempt(s); giving up.", + Name, + attemptsMade)); + + // Terminal failure has to be impossible to miss, so it goes to the device error + // surface as well as to this group's own event (issue #379 / #378). + RaiseDeviceError( + DeviceErrorSource.Reconnect, + new DeviceReconnectFailedException(Name, attemptsMade, lastError)); + } + + RaiseReconnectEvent( + ReconnectFailed, + new ReconnectFailedEventArgs(attemptsMade, lastError, wasCanceled), + nameof(ReconnectFailed)); + } + + /// + /// Raises one of the reconnect events, isolating the loop from a subscriber that throws — + /// the same guarantee and SendFailed give. + /// + private void RaiseReconnectEvent( + EventHandler? handler, + TArgs args, + string eventName) + where TArgs : EventArgs + { + if (handler == null) + { + return; } + + try + { + handler(this, args); + } + catch (Exception ex) + { + SafeLog(() => _logger.LogWarning(ex, "[{EventName}] subscriber threw", eventName)); + } + } + + /// + /// When overridden in a derived class, records the session state that a reconnect should + /// restore. Called on the thread that detected the drop, before anything else observes it, + /// and only when reconnect is enabled. + /// + /// + /// The base device owns nothing session-shaped — its channel collection is repopulated from + /// the device's own status message on every connect — so this does nothing here. + /// overrides it to record which channels were enabled + /// and whether a stream was running. + /// + protected virtual void CaptureSessionSnapshot() + { } + /// + /// When overridden in a derived class, re-applies the state recorded by + /// to a device that has just been reconnected and + /// re-initialized. + /// + /// The policy governing this reconnect. + /// Cancelled if reconnection is cancelled. + /// true if an interrupted stream was restarted. + protected virtual Task RestoreSessionSnapshotAsync( + ReconnectOptions options, + CancellationToken cancellationToken) => Task.FromResult(false); + + #endregion + /// /// Logs a message that the producer's background thread could not deliver. /// diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 185f1928..e4089a1f 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -405,6 +405,125 @@ public void StopStreaming() Send(ScpiMessageProducer.StopStreaming); } + #region Session restore after an automatic reconnect (issue #379) + + /// + /// What the streaming session looked like at the instant the connection dropped. Null until + /// a drop is detected with reconnect enabled. + /// + private volatile StreamingSessionSnapshot? _sessionSnapshot; + + /// + /// The subset of a streaming session that Core owns and can therefore put back: which + /// channels were enabled, and whether data was flowing. + /// + private sealed class StreamingSessionSnapshot + { + public StreamingSessionSnapshot(HashSet<(ChannelType Type, int Number)> enabledChannels, bool wasStreaming) + { + EnabledChannels = enabledChannels; + WasStreaming = wasStreaming; + } + + /// + /// The enabled channels, held by identity rather than by reference: a reconnect can + /// replace the channel objects, and a device that came back with a different channel + /// count should restore the intersection rather than fail. + /// + public HashSet<(ChannelType Type, int Number)> EnabledChannels { get; } + + public bool WasStreaming { get; } + } + + /// + protected override void CaptureSessionSnapshot() + { + var enabled = new HashSet<(ChannelType, int)>(); + foreach (var channel in GetChannelsSnapshot()) + { + if (channel.IsEnabled) + { + enabled.Add((channel.Type, channel.ChannelNumber)); + } + } + + _sessionSnapshot = new StreamingSessionSnapshot(enabled, IsStreaming); + } + + /// + /// Re-applies the enabled-channel set recorded at the drop and, if the policy says so, + /// restarts a stream that was interrupted. + /// + /// + /// + /// The enable set has to be replayed from the snapshot rather than read back off the + /// channel objects: resyncs analog + /// IsEnabled from the device's own enabled mask on every status message (#409), so by + /// the time re-initialization is done the in-memory view reflects the freshly reconnected + /// device, not the session that was lost. + /// + /// + /// The streaming frequency needs no replay — it is a host-side setting that the drop never + /// touched — but it does have to reach the device again, which is what the resumed + /// does. + /// + /// + /// A resumed stream is a genuinely new session: timestamp reconstruction re-anchors and the + /// gap detector resets, because the device's tick counter may well have restarted while it + /// was away, and carrying the old anchor across would manufacture a nonsense gap. + /// is the marker for the outage, and it carries its + /// duration. + /// + /// + protected override Task RestoreSessionSnapshotAsync( + ReconnectOptions options, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(options); + + var snapshot = _sessionSnapshot; + if (snapshot == null) + { + return Task.FromResult(false); + } + + cancellationToken.ThrowIfCancellationRequested(); + + // Normalize to a known state before re-applying: whatever the device came back with is + // not necessarily what it had, and the enable commands are set-replace anyway. + DisableAllChannels(); + + var toEnable = new List(); + foreach (var channel in GetChannelsSnapshot()) + { + if (snapshot.EnabledChannels.Contains((channel.Type, channel.ChannelNumber))) + { + toEnable.Add(channel); + } + } + + if (toEnable.Count > 0) + { + EnableChannels(toEnable); + } + + cancellationToken.ThrowIfCancellationRequested(); + + var resumeStreaming = snapshot.WasStreaming && options.ResumeStreaming; + if (resumeStreaming) + { + // The drop left IsStreaming set — nothing stopped the stream, the connection just + // ended — and StartStreaming is a no-op while it is. Clear it so the restart is a + // real one, complete with its per-session timestamp re-anchoring. + IsStreaming = false; + StartStreaming(); + } + + return Task.FromResult(resumeStreaming); + } + + #endregion + /// /// The default bounded-buffer capacity (in samples) used by . /// diff --git a/src/Daqifi.Core/Device/DeviceErrorSource.cs b/src/Daqifi.Core/Device/DeviceErrorSource.cs index 84f5df6c..432443e3 100644 --- a/src/Daqifi.Core/Device/DeviceErrorSource.cs +++ b/src/Daqifi.Core/Device/DeviceErrorSource.cs @@ -32,4 +32,16 @@ public enum DeviceErrorSource /// (). The frame is dropped; the stream keeps running. /// StreamDecode = 2, + + /// + /// Automatic reconnection after a dropped connection (issue #379): every allowed attempt + /// failed, so the session is gone for good. + /// + /// + /// Unlike the other sources this one is terminal rather than incidental — it is raised exactly + /// once, when the device gives up, and it is the loud counterpart to + /// . A reconnect that is cancelled does not raise it: + /// stopping on request is not a failure. + /// + Reconnect = 3, } diff --git a/src/Daqifi.Core/Device/DeviceReconnectFailedException.cs b/src/Daqifi.Core/Device/DeviceReconnectFailedException.cs new file mode 100644 index 00000000..bd9c19a8 --- /dev/null +++ b/src/Daqifi.Core/Device/DeviceReconnectFailedException.cs @@ -0,0 +1,35 @@ +namespace Daqifi.Core.Device; + +/// +/// Reports that automatic reconnection exhausted every allowed attempt without restoring the +/// session (issue #379). +/// +/// +/// This is never thrown — nobody is on the other end of a background reconnect loop to catch it. +/// It exists so that giving up arrives on as a typed +/// failure carrying the attempt count, with whatever ended the final attempt as its +/// . +/// +public class DeviceReconnectFailedException : Exception +{ + /// + /// Initializes a new instance of the class. + /// + /// The device that could not be reconnected. + /// How many attempts were made before giving up. + /// The failure that ended the final attempt, if there was one. + public DeviceReconnectFailedException(string deviceName, int attemptsMade, Exception? innerException = null) + : base( + $"Device '{deviceName}' could not be reconnected after {attemptsMade} attempt(s).", + innerException) + { + DeviceName = deviceName; + AttemptsMade = attemptsMade; + } + + /// Gets the name of the device that could not be reconnected. + public string DeviceName { get; } + + /// Gets how many reconnect attempts were made before giving up. + public int AttemptsMade { get; } +} diff --git a/src/Daqifi.Core/Device/ReconnectAttemptEventArgs.cs b/src/Daqifi.Core/Device/ReconnectAttemptEventArgs.cs new file mode 100644 index 00000000..b21351d8 --- /dev/null +++ b/src/Daqifi.Core/Device/ReconnectAttemptEventArgs.cs @@ -0,0 +1,49 @@ +namespace Daqifi.Core.Device; + +/// +/// Reports that an automatic reconnect attempt is about to be made (issue #379). +/// +/// +/// Raised once per attempt, before the backoff wait, so a UI can show both which attempt is +/// running and how long it will be until anything happens. +/// +public class ReconnectAttemptEventArgs : EventArgs +{ + /// + /// Initializes a new instance of the class. + /// + /// The 1-based attempt number. + /// The total number of attempts the policy allows. + /// How long the device will wait before making this attempt. + /// Why the previous attempt failed, or null for the first. + public ReconnectAttemptEventArgs( + int attemptNumber, + int maxAttempts, + TimeSpan delay, + Exception? previousError = null) + { + AttemptNumber = attemptNumber; + MaxAttempts = maxAttempts; + Delay = delay; + PreviousError = previousError; + Timestamp = DateTime.UtcNow; + } + + /// Gets the 1-based number of the attempt about to be made. + public int AttemptNumber { get; } + + /// Gets how many attempts the policy allows in total. + public int MaxAttempts { get; } + + /// Gets the backoff wait that precedes this attempt. + public TimeSpan Delay { get; } + + /// + /// Gets the failure that ended the previous attempt, or null when this is the first + /// attempt after the drop. + /// + public Exception? PreviousError { get; } + + /// Gets the UTC time at which the attempt was scheduled. + public DateTime Timestamp { get; } +} diff --git a/src/Daqifi.Core/Device/ReconnectFailedEventArgs.cs b/src/Daqifi.Core/Device/ReconnectFailedEventArgs.cs new file mode 100644 index 00000000..a2bfb9a9 --- /dev/null +++ b/src/Daqifi.Core/Device/ReconnectFailedEventArgs.cs @@ -0,0 +1,48 @@ +namespace Daqifi.Core.Device; + +/// +/// Reports that automatic reconnection has stopped without restoring the session (issue #379) — +/// either because every allowed attempt failed, or because it was cancelled. +/// +/// +/// Giving up is the loud outcome: alongside this event the failure is logged, raised on +/// with +/// , and the device settles on +/// . Cancellation is quieter — it is what the caller asked +/// for — and leaves the device on . +/// +public class ReconnectFailedEventArgs : EventArgs +{ + /// + /// Initializes a new instance of the class. + /// + /// How many attempts were made. + /// The failure that ended the final attempt, if there was one. + /// Whether reconnection stopped because it was cancelled. + public ReconnectFailedEventArgs(int attemptsMade, Exception? lastError, bool wasCanceled) + { + AttemptsMade = attemptsMade; + LastError = lastError; + WasCanceled = wasCanceled; + Timestamp = DateTime.UtcNow; + } + + /// Gets how many reconnect attempts were made before giving up. + public int AttemptsMade { get; } + + /// + /// Gets the failure that ended the last attempt, or null when reconnection was + /// cancelled before any attempt failed. + /// + public Exception? LastError { get; } + + /// + /// Gets a value indicating whether reconnection stopped because it was cancelled — by + /// , , or + /// disposal — rather than by exhausting . + /// + public bool WasCanceled { get; } + + /// Gets the UTC time at which reconnection stopped. + public DateTime Timestamp { get; } +} diff --git a/src/Daqifi.Core/Device/ReconnectOptions.cs b/src/Daqifi.Core/Device/ReconnectOptions.cs new file mode 100644 index 00000000..454c9b8f --- /dev/null +++ b/src/Daqifi.Core/Device/ReconnectOptions.cs @@ -0,0 +1,189 @@ +namespace Daqifi.Core.Device; + +/// +/// Policy for automatically re-establishing a session after +/// (issue #379). Assign to +/// . +/// +/// +/// +/// Off by default. A freshly constructed instance has set to +/// false, which is exactly the behaviour a device has always had: a drop is reported as +/// and nothing else happens. Reconnect has to be asked for. +/// +/// +/// The shape mirrors , which governs +/// the initial connect, so the two read the same way. They are separate settings for +/// separate moments: that one retries a connect the caller asked for, this one retries a session +/// the caller never asked to lose. +/// +/// +public class ReconnectOptions +{ + private int _maxAttempts = 5; + private TimeSpan _initialDelay = TimeSpan.FromSeconds(1); + private TimeSpan _maxDelay = TimeSpan.FromSeconds(30); + private double _backoffMultiplier = 2.0; + + /// + /// Gets or sets a value indicating whether a lost connection is reconnected automatically. + /// Default is false — reconnect is opt-in. + /// + public bool Enabled { get; set; } + + /// + /// Gets or sets how many reconnect attempts are made before giving up. Default is 5. + /// + /// Thrown when the value is less than 1. + public int MaxAttempts + { + get => _maxAttempts; + set + { + if (value < 1) + { + throw new ArgumentOutOfRangeException( + nameof(MaxAttempts), value, "At least one reconnect attempt must be allowed."); + } + + _maxAttempts = value; + } + } + + /// + /// Gets or sets the wait before the first reconnect attempt. Default is 1 second. + /// + /// + /// There is always a wait before the first attempt, unlike + /// : at the instant a drop is + /// detected the endpoint is, by definition, gone. A serial port that has just been unplugged + /// has not finished disappearing from the OS yet, let alone come back. + /// + /// Thrown when the value is negative. + public TimeSpan InitialDelay + { + get => _initialDelay; + set + { + if (value < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(InitialDelay), value, "The delay cannot be negative."); + } + + _initialDelay = value; + } + } + + /// + /// Gets or sets the ceiling the backoff grows to. Default is 30 seconds. + /// + /// Thrown when the value is negative. + public TimeSpan MaxDelay + { + get => _maxDelay; + set + { + if (value < TimeSpan.Zero) + { + throw new ArgumentOutOfRangeException( + nameof(MaxDelay), value, "The delay cannot be negative."); + } + + _maxDelay = value; + } + } + + /// + /// Gets or sets the factor each successive delay is multiplied by. Default is 2.0; 1.0 gives a + /// fixed delay. + /// + /// Thrown when the value is less than 1. + public double BackoffMultiplier + { + get => _backoffMultiplier; + set + { + if (!(value >= 1.0)) + { + throw new ArgumentOutOfRangeException( + nameof(BackoffMultiplier), value, "The backoff multiplier must be at least 1.0."); + } + + _backoffMultiplier = value; + } + } + + /// + /// Gets or sets a value indicating whether a stream that was running when the connection + /// dropped is restarted once the session is back. Default is true. + /// + /// + /// Set to false to restore the channel configuration but leave the device idle — useful + /// when the consumer wants to decide for itself whether resuming acquisition still makes sense + /// after an outage of unknown length. + /// + public bool ResumeStreaming { get; set; } = true; + + /// + /// A policy that never reconnects. Identical to a default-constructed instance; named for + /// callers that want to say so explicitly. + /// + public static ReconnectOptions Disabled => new(); + + /// + /// A reasonable enabled policy: five attempts, 1 s growing to 30 s, resuming an active stream. + /// + public static ReconnectOptions Default => new() { Enabled = true }; + + /// + /// A policy for links that blip briefly and often — six quick attempts inside about 10 seconds. + /// + public static ReconnectOptions Fast => new() + { + Enabled = true, + MaxAttempts = 6, + InitialDelay = TimeSpan.FromMilliseconds(500), + MaxDelay = TimeSpan.FromSeconds(4), + BackoffMultiplier = 1.5 + }; + + /// + /// A policy for unattended long runs: keeps trying for roughly ten minutes before giving up. + /// + public static ReconnectOptions Resilient => new() + { + Enabled = true, + MaxAttempts = 15, + InitialDelay = TimeSpan.FromSeconds(2), + MaxDelay = TimeSpan.FromSeconds(60), + BackoffMultiplier = 2.0 + }; + + /// + /// Calculates how long to wait before attempt . + /// + /// The 1-based attempt number. + /// + /// multiplied by once per attempt + /// already made, capped at . + /// + public TimeSpan CalculateDelay(int attemptNumber) + { + if (attemptNumber <= 1) + { + return InitialDelay; + } + + var delayMs = InitialDelay.TotalMilliseconds * Math.Pow(BackoffMultiplier, attemptNumber - 1); + + // Math.Pow overflows to +Infinity for a large enough attempt count; Math.Min then yields + // MaxDelay, which is the intended cap, but guard NaN (0 * Infinity) explicitly. + if (double.IsNaN(delayMs)) + { + return MaxDelay; + } + + return TimeSpan.FromMilliseconds(Math.Min(delayMs, MaxDelay.TotalMilliseconds)); + } +} diff --git a/src/Daqifi.Core/Device/ReconnectedEventArgs.cs b/src/Daqifi.Core/Device/ReconnectedEventArgs.cs new file mode 100644 index 00000000..920bac10 --- /dev/null +++ b/src/Daqifi.Core/Device/ReconnectedEventArgs.cs @@ -0,0 +1,45 @@ +namespace Daqifi.Core.Device; + +/// +/// Reports that a lost session has been re-established and its state restored (issue #379). +/// +/// +/// Raised after the transport is back, the device has been re-initialized, and the channel +/// configuration — and the stream, if one was running and the policy resumes it — have been +/// re-applied. By the time this fires, samples are flowing again. +/// +public class ReconnectedEventArgs : EventArgs +{ + /// + /// Initializes a new instance of the class. + /// + /// The 1-based attempt that succeeded. + /// How long the session was down, measured from the drop. + /// Whether an interrupted stream was restarted. + public ReconnectedEventArgs(int attemptNumber, TimeSpan outage, bool streamingResumed) + { + AttemptNumber = attemptNumber; + Outage = outage; + StreamingResumed = streamingResumed; + Timestamp = DateTime.UtcNow; + } + + /// Gets the 1-based number of the attempt that succeeded. + public int AttemptNumber { get; } + + /// + /// Gets how long the session was down, from the drop being detected to the state being + /// restored. + /// + public TimeSpan Outage { get; } + + /// + /// Gets a value indicating whether a stream that was running at the time of the drop was + /// restarted. false when nothing was streaming, or when + /// is off. + /// + public bool StreamingResumed { get; } + + /// Gets the UTC time at which the session was restored. + public DateTime Timestamp { get; } +} From 39082604d25792561ea29470d5571621ef1794d1 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 13:56:53 -0600 Subject: [PATCH 03/16] fix(consumers): no reader-loop failure may spin, escape, or outlive its scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo review on #415, plus a process-crash hazard the regression test exposed while verifying the fix. - Text exchange: the temporary consumer's error forwarding is now scope-bound (ConsumerErrorSubscription), so a reader that outlives the exchange's bounded stop/dispose can neither retain the device nor keep raising errors on it. - CanRead probe: a throwing readability getter is a stream fault and is handled like a failing read (health sink + error + backoff) instead of falling to the outer catch, which neither reported nor backed off. - Outer catch: added a backoff. A parser that throws on the bytes it holds throws on the same bytes next iteration, so retrying at full speed was a hot spin — measured at 59k error raises in 700ms. Deliberately not reported to the health sink: a parse failure is not evidence the link is gone. - Outer catch is now unconditional. `when (_isRunning)` left a hole where a stop landing mid-try made the exception escape a background thread and terminate the host process (it crashed the test host). Only reporting was ever meant to be conditional. Six regression tests added, each verified to fail on the pre-fix code. Co-Authored-By: Claude Opus 5 --- .../StreamMessageConsumerBackoffTests.cs | 275 ++++++++++++++++++ .../Device/DeviceErrorSurfaceTests.cs | 156 ++++++++++ .../Consumers/StreamMessageConsumer.cs | 48 ++- src/Daqifi.Core/Device/DaqifiDevice.cs | 41 ++- 4 files changed, 516 insertions(+), 4 deletions(-) create mode 100644 src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs new file mode 100644 index 00000000..95c38dfd --- /dev/null +++ b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs @@ -0,0 +1,275 @@ +using Daqifi.Core.Communication.Consumers; +using Daqifi.Core.Communication.Messages; +using Daqifi.Core.Communication.Transport; + +namespace Daqifi.Core.Tests.Communication.Consumers; + +/// +/// No failure mode in the reader loop may spin without progress. A failure that repeats every +/// iteration must be backed off, and a failure that is genuinely I/O against the stream must reach +/// the transport so a dead link can still be escalated (issues #377, #378). +/// +public class StreamMessageConsumerBackoffTests +{ + /// + /// Window over which raise counts are sampled. With the loop's 100 ms error back-off this + /// bounds a repeating failure to a handful of raises; without one it is limited only by how + /// fast the thread can spin. + /// + private static readonly TimeSpan SampleWindow = TimeSpan.FromMilliseconds(700); + + /// + /// Generous ceiling for "backed off" over . The unbacked-off loop + /// produces tens of thousands in the same window, so the separation is not marginal. + /// + private const int BackedOffCeiling = 25; + + [Fact] + public void WhenTheCanReadProbeThrows_ItIsTreatedAsAStreamFaultAndReportedToTheTransport() + { + // Probing readability touches the same handle a read does, and on a half-torn-down stream + // the getter itself can throw. That is the link failing, so it has to reach the transport — + // otherwise this one failure mode silently bypasses connection-loss escalation. + using var stream = new ThrowingCanReadStream(); + var sink = new RecordingHealthSink(); + var errors = 0; + + using var consumer = new StreamMessageConsumer( + stream, new LineBasedMessageParser(), healthSink: sink); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + + Assert.True( + WaitUntil(() => sink.FaultCount >= TransportConnectionWatchdog.ConsecutiveFaultThreshold, + TimeSpan.FromSeconds(10)), + $"a throwing CanRead must be reported to the transport, saw {sink.FaultCount} fault(s)"); + + Assert.True(Volatile.Read(ref errors) >= 1, "it must also be visible as a consumer error"); + + consumer.StopSafely(timeoutMs: 2000); + } + + [Fact] + public void WhenTheCanReadProbeThrows_TheLoopBacksOffInsteadOfSpinning() + { + using var stream = new ThrowingCanReadStream(); + var errors = 0; + + using var consumer = new StreamMessageConsumer(stream, new LineBasedMessageParser()); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + Thread.Sleep(SampleWindow); + consumer.StopSafely(timeoutMs: 2000); + + var raised = Volatile.Read(ref errors); + Assert.True(raised >= 1, "the failure must be reported at all"); + Assert.True(raised <= BackedOffCeiling, $"expected a backed-off cadence, saw {raised} raises"); + } + + [Fact] + public void WhenParsingThrowsOnEveryIteration_TheLoopBacksOffInsteadOfSpinning() + { + // A parser that throws on the bytes it holds will throw on exactly the same bytes next time + // round, so retrying at full speed makes no progress and burns a core while raising errors + // as fast as the thread can go. + using var stream = new AlwaysReadableStream(); + var sink = new RecordingHealthSink(); + var errors = 0; + + using var consumer = new StreamMessageConsumer( + stream, new AlwaysThrowingParser(), healthSink: sink); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + Thread.Sleep(SampleWindow); + consumer.StopSafely(timeoutMs: 2000); + + var raised = Volatile.Read(ref errors); + Assert.True(raised >= 1, "the parse failure must be reported at all"); + Assert.True(raised <= BackedOffCeiling, $"expected a backed-off cadence, saw {raised} raises"); + } + + [Fact] + public void AParseFailure_IsNeverReportedToTheTransportAsAnIoFault() + { + // The counterpart to the back-off: a parse failure means the bytes were bad, not that the + // link is gone. Escalating it would disconnect a perfectly healthy device over malformed + // data — the reads themselves are succeeding. + using var stream = new AlwaysReadableStream(); + var sink = new RecordingHealthSink(); + + using var consumer = new StreamMessageConsumer( + stream, new AlwaysThrowingParser(), healthSink: sink); + + consumer.Start(); + Thread.Sleep(SampleWindow); + consumer.StopSafely(timeoutMs: 2000); + + Assert.Equal(0, sink.FaultCount); + Assert.True(sink.SuccessCount >= 1, "the reads themselves were fine and must be reported as such"); + } + + [Fact] + public void AFailureThatLandsWhileStopping_IsSwallowedRatherThanKillingTheProcess() + { + // The reader loop runs on a background thread, so an exception that escapes its catch does + // not just end the loop — it terminates the host process. A stop that lands while the try + // body is mid-flight used to do exactly that, because the catch filter stopped matching the + // moment the running flag cleared. + // + // Reaching the end of this test at all is the assertion: if the exception escapes, the test + // host dies and the whole run aborts. + using var stream = new AlwaysReadableStream(); + var parser = new GatedThrowingParser(); + var errors = 0; + + using var consumer = new StreamMessageConsumer(stream, parser); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + + Assert.True(parser.Entered.Wait(TimeSpan.FromSeconds(10)), "the parser was never reached"); + + // Clear the running flag while the parser is parked inside the try body, then let it throw. + var stopper = new Thread(() => consumer.StopSafely(timeoutMs: 5000)) { IsBackground = true }; + stopper.Start(); + Assert.True(WaitUntil(() => !consumer.IsRunning, TimeSpan.FromSeconds(10))); + parser.Release.Set(); + + Assert.True(stopper.Join(TimeSpan.FromSeconds(10)), "the consumer never stopped"); + Assert.False(consumer.IsRunning); + + // The failure arrived during teardown, so it says nothing about the device and is not + // reported as a device-visible error. + Assert.Equal(0, Volatile.Read(ref errors)); + } + + private static bool WaitUntil(Func condition, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + while (DateTime.UtcNow < deadline) + { + if (condition()) + { + return true; + } + + Thread.Sleep(10); + } + + return condition(); + } + + private sealed class RecordingHealthSink : ITransportHealthSink + { + private int _faultCount; + private int _successCount; + + public int FaultCount => Volatile.Read(ref _faultCount); + + public int SuccessCount => Volatile.Read(ref _successCount); + + public void ReportIoFault(Exception error) => Interlocked.Increment(ref _faultCount); + + public void ReportIoSuccess() => Interlocked.Increment(ref _successCount); + } + + /// + /// A parser that fails on everything it is handed, standing in for a systematic parse failure. + /// + private sealed class AlwaysThrowingParser : IMessageParser + { + public IEnumerable> ParseMessages(byte[] data, out int consumedBytes) + { + consumedBytes = 0; + throw new FormatException("this parser cannot handle anything"); + } + } + + /// + /// A parser that parks inside until released, then throws — so a + /// stop can be made to land precisely while the reader is inside its try body. + /// + private sealed class GatedThrowingParser : IMessageParser + { + public ManualResetEventSlim Entered { get; } = new(false); + + public ManualResetEventSlim Release { get; } = new(false); + + public IEnumerable> ParseMessages(byte[] data, out int consumedBytes) + { + consumedBytes = 0; + Entered.Set(); + Release.Wait(TimeSpan.FromSeconds(30)); + throw new FormatException("failing after the stop landed"); + } + } + + /// + /// A readable stream that always yields a byte, so the loop reaches the parse stage every + /// iteration. + /// + private sealed class AlwaysReadableStream : Stream + { + public override int Read(byte[] buffer, int offset, int count) + { + buffer[offset] = (byte)'x'; + return 1; + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + + /// + /// A stream whose readability probe itself fails, as a handle torn down underneath the reader + /// can. + /// + private sealed class ThrowingCanReadStream : Stream + { + public override bool CanRead => throw new ObjectDisposedException(nameof(ThrowingCanReadStream)); + + public override int Read(byte[] buffer, int offset, int count) => + throw new ObjectDisposedException(nameof(ThrowingCanReadStream)); + + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } +} diff --git a/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs b/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs index 1579e6a0..ac45fa5d 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs @@ -103,6 +103,43 @@ public void AfterDisconnect_TheConsumerErrorSurfaceIsDetached() Assert.Equal(afterDisconnect, Volatile.Read(ref errors)); } + [Fact] + public async Task AStuckTextConsumerThatOutlivesItsExchange_CanNoLongerRaiseOnTheDevice() + { + // The text exchange binds a temporary consumer to the device's error surface. Stopping and + // disposing that consumer are both time-bounded, so a reader parked in an un-returning read + // outlives the exchange — and a still-subscribed zombie would both retain the device and + // report failures against an exchange (and a connection) that ended long ago. + using var stream = new StallingStream(); + using var transport = new StreamBackedTransport(stream); + using var device = new TextCommandTestableDevice("Stalling Device", transport); + + var stuckReaderErrors = 0; + device.ErrorOccurred += (_, e) => + { + if (e.Error is EndOfStreamException) + { + Interlocked.Increment(ref stuckReaderErrors); + } + }; + + device.Connect(); + + // The setup action runs once the text consumer is reading, so this is what parks its + // in-flight read past the exchange's stop and dispose windows. + await device.CallTextCommandAsync(() => stream.StallThenFail()); + + // Detach the protobuf consumer too, so the only thing that could still report is the + // abandoned text reader. + device.Disconnect(); + + // Outlast the stalled read, which fails when it finally returns. + Thread.Sleep(StallingStream.StallDuration); + Thread.Sleep(TimeSpan.FromMilliseconds(750)); + + Assert.Equal(0, Volatile.Read(ref stuckReaderErrors)); + } + #endregion #region Stream decode errors @@ -299,6 +336,125 @@ public override void Send(IOutboundMessage message) } } + /// + /// A exposing the protected text exchange, so the temporary + /// text consumer's lifetime can be exercised. + /// + private sealed class TextCommandTestableDevice : DaqifiStreamingDevice + { + public TextCommandTestableDevice(string name, IStreamTransport transport) : base(name, transport) + { + } + + public Task> CallTextCommandAsync(Action setupAction) => + ExecuteTextCommandAsync(setupAction, responseTimeoutMs: 200, completionTimeoutMs: 100); + } + + /// + /// A transport that simply hands out a caller-supplied stream. + /// + private sealed class StreamBackedTransport : IStreamTransport + { + private readonly Stream _stream; + private bool _isConnected; + private bool _disposed; + + public StreamBackedTransport(Stream stream) + { + _stream = stream; + } + + public Stream Stream => _disposed + ? throw new ObjectDisposedException(nameof(StreamBackedTransport)) + : _stream; + + public bool IsConnected => _isConnected && !_disposed; + + public string ConnectionInfo => _isConnected ? "Stream: Connected" : "Stream: Disconnected"; + + public event EventHandler? StatusChanged; + + public Task ConnectAsync() => ConnectAsync(null); + + public Task ConnectAsync(ConnectionRetryOptions? retryOptions) + { + _isConnected = true; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); + return Task.CompletedTask; + } + + public Task DisconnectAsync() + { + _isConnected = false; + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + return Task.CompletedTask; + } + + public void Connect() => ConnectAsync().GetAwaiter().GetResult(); + + public void Disconnect() => DisconnectAsync().GetAwaiter().GetResult(); + + public void Dispose() + { + _isConnected = false; + _disposed = true; + } + } + + /// + /// A stream that idles quietly until told to stall, after which a read parks for + /// — long enough to outlast the exchange's bounded stop and dispose + /// joins — and then fails. This reproduces the reader that is still alive after its consumer has + /// been disposed. + /// + internal sealed class StallingStream : Stream + { + /// + /// Comfortably longer than the text exchange's StopSafely join plus the consumer's + /// dispose-time grace (1 s each), so the reader provably outlives both. + /// + public static readonly TimeSpan StallDuration = TimeSpan.FromSeconds(3); + + private volatile bool _stalling; + + public void StallThenFail() => _stalling = true; + + public override int Read(byte[] buffer, int offset, int count) + { + if (_stalling) + { + Thread.Sleep(StallDuration); + throw new EndOfStreamException("the stalled read finally gave up"); + } + + Thread.Sleep(5); + return 0; + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => true; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) + { + } + } + /// /// A transport over a stream whose reads can be made to fail or time out on demand, so a device /// can be driven through a failing read loop without hardware. diff --git a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs index 6cce4215..f71ecff4 100644 --- a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs +++ b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs @@ -331,12 +331,29 @@ private void ProcessMessages() PerformClear(); } + // Probing readability is itself I/O against a handle that may be coming apart, and + // a CanRead getter is under no obligation not to throw on one. A throwing probe is + // a stream fault and is handled exactly like a failing read; letting it reach the + // outer catch instead would neither tell the transport nor back off. + bool canRead; + try + { + canRead = _stream.CanRead; + } + catch (Exception ex) + { + _healthSink?.ReportIoFault(ex); + OnErrorOccurred(ex); + Thread.Sleep(100); + continue; + } + // A stream that reports itself unreadable never becomes readable again — that is a // closed or disposed stream, not a momentary lull. Report it instead of spinning // here forever producing no data, no error and no status change, which is the // failure mode issue #377 was filed for. Backed off at the same cadence as a // failing read so the escalation timing matches. - if (!_stream.CanRead) + if (!canRead) { var unreadable = new IOException( "The stream is no longer readable; the underlying connection has been closed."); @@ -410,10 +427,35 @@ private void ProcessMessages() // Try to parse complete messages from buffer ProcessMessageBuffer(); } - catch (Exception ex) when (_isRunning) + catch (Exception ex) { - // Only report errors if we're still supposed to be running + // Caught unconditionally, on purpose. This is the top of a background thread, so an + // exception that escapes here does not merely end the loop — it terminates the + // whole process. The previous `when (_isRunning)` filter left exactly that hole: a + // concurrent Stop() clears the flag while the try body is mid-flight, the filter + // then declines to match, and a parse failure that would have been a logged + // diagnostic during normal running takes the host down instead. Only *reporting* + // was ever meant to be conditional. + if (!_isRunning) + { + // Teardown noise: a failure seen while stopping says nothing about the device, + // and the loop is about to exit anyway. + break; + } + OnErrorOccurred(ex); + + // Back off before the next iteration. What reaches here is a failure in the + // parse/dispatch half of the loop, and that half is deterministic with respect to + // the current buffer: a parser that throws on the bytes it holds will throw on + // exactly the same bytes next time round. Retrying at full speed is a hot spin that + // burns a core and raises errors as fast as the thread can go, which is the same + // no-progress-and-no-signal shape this class is being fixed for. + // + // Deliberately NOT reported to the health sink: a parse or dispatch failure is not + // evidence that the link is gone, and escalating it would disconnect a perfectly + // healthy device over malformed data. Only I/O against the stream does that. + Thread.Sleep(100); } } } diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 27eb90f6..5aacc68c 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -1090,7 +1090,16 @@ private async Task> ExecuteTextCommandCoreAsync( // The protobuf consumer is stopped for the duration of this exchange, so // without this a read failure during a text command (an unplug mid-SD-listing, // say) would be the one background failure with nowhere to go (issue #378). - textConsumer.ErrorOccurred += OnConsumerErrorOccurred; + // + // Scoped rather than a bare '+=' because this consumer can outlive the block: + // its stop and dispose are both time-bounded and may return with the reader + // thread still parked in an un-returning read. A live thread roots the consumer, + // which would root this device through the handler — retaining the whole object + // graph and, worse, letting a zombie reader keep raising errors on a device that + // has since been disconnected. 'using' disposes in reverse declaration order, so + // this detaches before textConsumer itself is disposed, on every exit path + // including a cancellation or a throwing setup action. + using var textConsumerErrors = new ConsumerErrorSubscription(this, textConsumer); textConsumer.Start(); // ConfigureAwait(false): the lock is held, so resuming on a captured @@ -1513,6 +1522,36 @@ private void OnConsumerErrorOccurred(object? sender, MessageConsumerErrorEventAr RaiseDeviceError(DeviceErrorSource.MessageConsumer, e.Error, e.RawData); } + /// + /// Attaches a device's consumer-error forwarding to a short-lived + /// for the duration of a scope, and detaches it again on + /// dispose. + /// + /// + /// Exists so a temporary consumer can never end up permanently subscribed. Stopping and + /// disposing a consumer are both time-bounded and may return while its reader thread is + /// still alive; that thread roots the consumer, and a still-attached handler would root this + /// device through it. Detaching in a finally (which is what using compiles to) + /// makes the subscription's lifetime exactly the scope's, whatever way control leaves it. + /// + private sealed class ConsumerErrorSubscription : IDisposable + { + private readonly DaqifiDevice _device; + private readonly IMessageConsumer _consumer; + + public ConsumerErrorSubscription(DaqifiDevice device, IMessageConsumer consumer) + { + _device = device; + _consumer = consumer; + _consumer.ErrorOccurred += _device.OnConsumerErrorOccurred; + } + + public void Dispose() + { + _consumer.ErrorOccurred -= _device.OnConsumerErrorOccurred; + } + } + /// /// Logs a background failure and raises for it, subject to the /// throttle policy documented on that event. From 60e76eb0c2449c435597de6fcfa756fa54033b21 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:01:34 -0600 Subject: [PATCH 04/16] fix(device): close two reconnect holes found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - A reconnected device reported IsStreaming from before the drop when the policy had ResumeStreaming off, so it claimed to be streaming while idle and made the caller's own StartStreaming() a silent no-op. The flag is now cleared whenever a session is restored, since re-initialization has just stopped the stream. - A consumer that tore the device down from inside its own Lost handler — the pattern the docs show for devices without a reconnect policy — had the loop start anyway and reopen the transport behind them. The drop now carries the session epoch it was observed at, and a reconnect refuses to start unless the device is still sitting on that same lost session. Also documents that reconnect-enabled consumers should stop tearing down on Lost themselves, and that reassigning ReconnectOptions mid-loop applies from the next drop. Co-Authored-By: Claude Opus 5 --- docs/DEVICE_INTERFACES.md | 9 +++- .../Device/DeviceReconnectTests.cs | 42 +++++++++++++++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 41 ++++++++++++++---- .../Device/DaqifiStreamingDevice.cs | 10 +++-- src/Daqifi.Core/Device/ReconnectOptions.cs | 2 + 5 files changed, 90 insertions(+), 14 deletions(-) diff --git a/docs/DEVICE_INTERFACES.md b/docs/DEVICE_INTERFACES.md index e0fe5f0b..4d04c64f 100644 --- a/docs/DEVICE_INTERFACES.md +++ b/docs/DEVICE_INTERFACES.md @@ -569,7 +569,14 @@ connect. Failing over from one transport to another (USB to WiFi, say) is out of **Stopping it.** `CancelReconnect()` stops the loop at its next checkpoint and leaves the device on `Lost`; `Disconnect()` and `Dispose()` do the same and then tear down. A caller-issued `Connect()` or `Disconnect()` always wins — the loop unwinds without touching the session the caller -established. +established, and a `Disconnect()` issued from inside a `Lost` handler stops the reconnect before it +even starts. + +Which is the other half of the rule: with reconnect enabled, **stop tearing down on `Lost` +yourself**. The teardown shown under [Detecting a dropped +connection](#detecting-a-dropped-connection) is for devices without a reconnect policy. Here the +device does it for you, between attempts, and doing it as well just cancels the recovery you asked +for. ```csharp device.ReconnectOptions = new ReconnectOptions diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 190c6c39..2f4cb474 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -189,6 +189,13 @@ public async Task WithResumeStreamingOff_TheChannelsComeBackButTheStreamDoesNot( Assert.False(result.StreamingResumed); Assert.Contains("ENAble:VOLTage:DC 1", device.SentCommands); Assert.DoesNotContain(device.SentCommands, c => c.StartsWith("SYSTem:StartStreamData", StringComparison.Ordinal)); + + // The device is genuinely idle, and says so. Reporting a stale IsStreaming here would both + // lie and make the caller's own StartStreaming() a silent no-op. + Assert.False(device.IsStreaming); + device.StartStreaming(); + Assert.True(device.IsStreaming); + Assert.Contains(device.SentCommands, c => c.StartsWith("SYSTem:StartStreamData", StringComparison.Ordinal)); } [Fact] @@ -476,6 +483,41 @@ public async Task ASecondDropAfterARecovery_StartsAFreshLoop() Assert.Contains("ENAble:VOLTage:DC 1", device.SentCommands); } + [Fact] + public void AHandlerThatDisconnectsOnLost_IsNotOverruledByAReconnect() + { + // The teardown-on-Lost pattern the docs show for devices without reconnect. If a consumer + // still does it, their decision has to stand — a reconnect starting behind it would reopen + // a device they just closed. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Torn Down Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + device.StatusChanged += (_, e) => + { + if (e.Status == ConnectionStatus.Lost) + { + device.Disconnect(); + } + }; + + var reconnectEvents = 0; + device.ReconnectAttempt += (_, _) => Interlocked.Increment(ref reconnectEvents); + + var connectsBeforeDrop = transport.ConnectCount; + transport.SimulateDrop(); + + Thread.Sleep(500); + + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.Equal(0, Volatile.Read(ref reconnectEvents)); + Assert.Equal(connectsBeforeDrop, transport.ConnectCount); + Assert.False(transport.IsConnected); + Assert.False(device.IsReconnecting); + } + [Fact] public void SettingReconnectOptionsToNull_IsRejected() { diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index cd00d069..c15612e9 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -1503,6 +1503,11 @@ private void OnTransportStatusChanged(object? sender, TransportStatusEventArgs e // not during an intentional Disconnect() call if (Status == ConnectionStatus.Connected && !_isDisconnecting) { + // Read before raising Lost: a handler is free to call Connect or Disconnect + // synchronously from inside it, and if it does, this drop must not start a + // reconnect for a session the caller has already moved on from. + var epochAtDrop = Volatile.Read(ref _sessionEpoch); + // Snapshot the session BEFORE anything observes the drop. Raising Lost runs // consumer handlers synchronously on this thread, and a handler is entirely // entitled to start tearing the device down — by which point what the session @@ -1527,7 +1532,7 @@ private void OnTransportStatusChanged(object? sender, TransportStatusEventArgs e Status = ConnectionStatus.Lost; - BeginReconnectIfEnabled(); + BeginReconnectIfEnabled(epochAtDrop); } } } @@ -1607,7 +1612,11 @@ public ReconnectOptions ReconnectOptions // the Lost that a failing attempt's own teardown can produce. private int _reconnectRunning; - private CancellationTokenSource? _reconnectCts; + // Volatile: written by the thread that starts a loop and by the loop's own cleanup, read by + // CancelReconnect from any thread. A stale read only ever delays a cancellation, never + // corrupts one — the epoch check is what actually stops the loop — but there is no reason + // to leave even that on the table. + private volatile CancellationTokenSource? _reconnectCts; // Bumped by every caller-issued Connect/Disconnect/Dispose. The reconnect loop captures it // when it starts and re-checks before each step that touches device state; a change means @@ -1663,13 +1672,25 @@ private void SupersedeReconnect() /// Called from the transport's status callback, which runs on the reader loop or the /// liveness timer — so the work is handed to the thread pool rather than done inline. /// - private void BeginReconnectIfEnabled() + /// + /// The session epoch observed before the drop was announced. A mismatch means a caller + /// connected or disconnected in the meantime — including from inside their own + /// handler — and this drop is no longer theirs to recover from. + /// + private void BeginReconnectIfEnabled(int expectedEpoch) { if (!ReconnectOptions.Enabled || _transport == null || _disposed || _isDisconnecting) { return; } + // Only ever start from a device that is actually sitting on a lost connection with the + // session it was lost from still current. + if (Status != ConnectionStatus.Lost || Volatile.Read(ref _sessionEpoch) != expectedEpoch) + { + return; + } + // One loop at a time. A failing attempt tears the transport down again, which can // produce another Lost; without this that would fork a second loop racing the first. if (Interlocked.CompareExchange(ref _reconnectRunning, 1, 0) != 0) @@ -1711,11 +1732,11 @@ private void BeginReconnectIfEnabled() // A drop that landed in the moments between this loop finishing its work and // releasing the running flag would have been skipped by the single-flight guard, // stranding the device on Lost with nothing trying to bring it back. Pick it up. - // Exhausting the attempts settles on Failed, and cancellation is excluded above, so - // neither can retrigger here. - if (!wasCanceled && Status == ConnectionStatus.Lost) + // Exhausting the attempts settles on Failed, and cancellation is excluded here, so + // neither can retrigger. + if (!wasCanceled) { - BeginReconnectIfEnabled(); + BeginReconnectIfEnabled(Volatile.Read(ref _sessionEpoch)); } }); } @@ -1724,8 +1745,10 @@ private void BeginReconnectIfEnabled() /// Attempts, with backoff, to rebuild the session that was just lost. /// /// - /// The policy snapshotted when the drop was detected, so a mid-flight change to - /// cannot alter the loop's terms underneath it. + /// The policy as it stood when the drop was detected. Assigning a new + /// mid-flight therefore leaves this loop on the terms it + /// started under and applies from the next drop; mutating the instance already assigned + /// does reach it, since the loop holds that same object. /// /// The session epoch at the time of the drop. /// Cancelled by . diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index e4089a1f..d4666d44 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -481,6 +481,12 @@ protected override Task RestoreSessionSnapshotAsync( { ArgumentNullException.ThrowIfNull(options); + // A reconnected device is never streaming: re-initialization has just sent it + // StopStreamData. The flag, though, is still set from before the drop — nothing stopped + // the stream, the connection simply ended — and leaving it that way would report a + // device as streaming while it sits idle, and make StartStreaming() a silent no-op. + IsStreaming = false; + var snapshot = _sessionSnapshot; if (snapshot == null) { @@ -512,10 +518,6 @@ protected override Task RestoreSessionSnapshotAsync( var resumeStreaming = snapshot.WasStreaming && options.ResumeStreaming; if (resumeStreaming) { - // The drop left IsStreaming set — nothing stopped the stream, the connection just - // ended — and StartStreaming is a no-op while it is. Clear it so the restart is a - // real one, complete with its per-session timestamp re-anchoring. - IsStreaming = false; StartStreaming(); } diff --git a/src/Daqifi.Core/Device/ReconnectOptions.cs b/src/Daqifi.Core/Device/ReconnectOptions.cs index 454c9b8f..5c6ad8f7 100644 --- a/src/Daqifi.Core/Device/ReconnectOptions.cs +++ b/src/Daqifi.Core/Device/ReconnectOptions.cs @@ -104,6 +104,8 @@ public double BackoffMultiplier get => _backoffMultiplier; set { + // Negated rather than `value < 1.0` so NaN — which compares false against everything — + // is rejected too, instead of turning every backoff into NaN milliseconds. if (!(value >= 1.0)) { throw new ArgumentOutOfRangeException( From 4b19d590938886b59d0b07f603260053f4473ed5 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:11:10 -0600 Subject: [PATCH 05/16] fix(consumers): suppress fault reporting during intentional teardown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo round 2 on #415. The CanRead paths reported I/O faults even after StopSafely() cleared the running flag, contradicting the outer catch's teardown-noise rule added in the previous commit — the same event got two different answers depending on which path caught it. All stream-fault sites (CanRead throw, CanRead false, read exception, socket EOF) now route through one ReportStreamFault helper that states the rule once: nothing is reported once a stop has been requested, and the loop exits at once instead of sleeping out a backoff it no longer needs. Parse/dispatch failures deliberately stay outside it — they are not evidence the link is gone. Checked whether this could breach #377's "intentional Disconnect() never reports Lost": it could not. A continue re-tests the loop condition, so at most ONE fault could ever be reported after a stop, against an escalation threshold of five consecutive — measured at exactly 1 with the guard removed. The transports also disarm their watchdog before touching the handle, and DaqifiDevice._isDisconnecting independently suppresses Lost. Diagnostic noise, not a hole in the guarantee — but noise the new device-level error event would have made user-visible on every disconnect. Two regression tests, both verified to fail on the pre-fix code, plus a teardown-silence assertion on the existing device-level disconnect test. Co-Authored-By: Claude Opus 5 --- .../StreamMessageConsumerBackoffTests.cs | 150 ++++++++++++++++++ .../ConnectionLossEscalationTests.cs | 7 + .../Consumers/StreamMessageConsumer.cs | 104 ++++++++++-- 3 files changed, 247 insertions(+), 14 deletions(-) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs index 95c38dfd..6a740681 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs @@ -146,6 +146,66 @@ public void AFailureThatLandsWhileStopping_IsSwallowedRatherThanKillingTheProces Assert.Equal(0, Volatile.Read(ref errors)); } + [Fact] + public void AStopThatLandsDuringTheReadabilityProbe_ReportsNothing() + { + // Closing a handle is *supposed* to make the reader fail, and the stream going unreadable is + // exactly what a deliberate Disconnect looks like from inside the loop. Reporting that would + // put a phantom "the connection died" into the transport's health sink and into consumer + // diagnostics on every intentional teardown. + using var stream = new GatedUnreadableStream(); + var sink = new RecordingHealthSink(); + var errors = 0; + + using var consumer = new StreamMessageConsumer( + stream, new LineBasedMessageParser(), healthSink: sink); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + + Assert.True(stream.ProbeEntered.Wait(TimeSpan.FromSeconds(10)), "the probe was never reached"); + + // Clear the running flag while the reader is parked inside the probe, then let the stream + // report itself unreadable — the shape of a handle closed underneath an in-flight read. + var stopper = new Thread(() => consumer.StopSafely(timeoutMs: 5000)) { IsBackground = true }; + stopper.Start(); + Assert.True(WaitUntil(() => !consumer.IsRunning, TimeSpan.FromSeconds(10))); + stream.ReleaseProbe.Set(); + + Assert.True(stopper.Join(TimeSpan.FromSeconds(10)), "the consumer never stopped"); + + Assert.Equal(0, sink.FaultCount); + Assert.Equal(0, Volatile.Read(ref errors)); + } + + [Fact] + public void AStopThatLandsDuringAReadFailure_ReportsNothing() + { + // Same rule for the read itself, which is the path a real teardown almost always takes: + // the handle is closed, and the in-flight Read throws because of it. + using var stream = new GatedThrowingReadStream(); + var sink = new RecordingHealthSink(); + var errors = 0; + + using var consumer = new StreamMessageConsumer( + stream, new LineBasedMessageParser(), healthSink: sink); + consumer.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + + consumer.Start(); + + Assert.True(stream.ReadEntered.Wait(TimeSpan.FromSeconds(10)), "the read was never reached"); + + var stopper = new Thread(() => consumer.StopSafely(timeoutMs: 5000)) { IsBackground = true }; + stopper.Start(); + Assert.True(WaitUntil(() => !consumer.IsRunning, TimeSpan.FromSeconds(10))); + stream.ReleaseRead.Set(); + + Assert.True(stopper.Join(TimeSpan.FromSeconds(10)), "the consumer never stopped"); + + Assert.Equal(0, sink.FaultCount); + Assert.Equal(0, Volatile.Read(ref errors)); + } + private static bool WaitUntil(Func condition, TimeSpan timeout) { var deadline = DateTime.UtcNow + timeout; @@ -241,6 +301,96 @@ public override void Flush() public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); } + /// + /// A stream whose readability probe parks until released and then reports the stream + /// unreadable, so a stop can be made to land precisely while the reader is inside the probe. + /// + private sealed class GatedUnreadableStream : Stream + { + private int _probeCount; + + public ManualResetEventSlim ProbeEntered { get; } = new(false); + + public ManualResetEventSlim ReleaseProbe { get; } = new(false); + + public override bool CanRead + { + get + { + // Only gate the first probe; later ones (e.g. from PerformClear) answer at once. + if (Interlocked.Increment(ref _probeCount) != 1) + { + return false; + } + + ProbeEntered.Set(); + ReleaseProbe.Wait(TimeSpan.FromSeconds(30)); + return false; + } + } + + public override int Read(byte[] buffer, int offset, int count) => 0; + + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + + /// + /// A stream whose read parks until released and then fails, reproducing the in-flight read that + /// a closing handle breaks. + /// + private sealed class GatedThrowingReadStream : Stream + { + public ManualResetEventSlim ReadEntered { get; } = new(false); + + public ManualResetEventSlim ReleaseRead { get; } = new(false); + + public override int Read(byte[] buffer, int offset, int count) + { + ReadEntered.Set(); + ReleaseRead.Wait(TimeSpan.FromSeconds(30)); + throw new IOException("the handle was closed underneath this read"); + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + /// /// A stream whose readability probe itself fails, as a handle torn down underneath the reader /// can. diff --git a/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs b/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs index 6a3a6409..4281c2d0 100644 --- a/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Transport/ConnectionLossEscalationTests.cs @@ -115,6 +115,9 @@ public void AnIntentionalDisconnect_NeverReportsLost_EvenThoughTeardownFailsTheR } }; + var errors = 0; + device.ErrorOccurred += (_, _) => Interlocked.Increment(ref errors); + // Closing the handle is what makes the in-flight reads fail, exactly as a real transport // teardown does. None of that may be reported as a loss. device.Disconnect(); @@ -128,6 +131,10 @@ public void AnIntentionalDisconnect_NeverReportsLost_EvenThoughTeardownFailsTheR } Assert.Equal(ConnectionStatus.Disconnected, device.Status); + + // Teardown is also silent: an intentional disconnect is not a diagnostic event, and the + // failures its own handle-closing provokes must not be reported as device problems. + Assert.Equal(0, Volatile.Read(ref errors)); } [Fact] diff --git a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs index f71ecff4..379810cc 100644 --- a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs +++ b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs @@ -342,9 +342,11 @@ private void ProcessMessages() } catch (Exception ex) { - _healthSink?.ReportIoFault(ex); - OnErrorOccurred(ex); - Thread.Sleep(100); + if (!ReportStreamFault(ex)) + { + break; + } + continue; } @@ -357,9 +359,11 @@ private void ProcessMessages() { var unreadable = new IOException( "The stream is no longer readable; the underlying connection has been closed."); - _healthSink?.ReportIoFault(unreadable); - OnErrorOccurred(unreadable); - Thread.Sleep(100); + if (!ReportStreamFault(unreadable)) + { + break; + } + continue; } @@ -388,9 +392,11 @@ private void ProcessMessages() // throwing is how a physically disconnected device announces itself, and the // transport is the only thing that can turn a run of them into a lost // connection (issue #382). One failure escalates nothing. - _healthSink?.ReportIoFault(ex); - OnErrorOccurred(ex); - Thread.Sleep(100); // Back off on error + if (!ReportStreamFault(ex)) + { + break; + } + continue; } @@ -402,11 +408,21 @@ private void ProcessMessages() // legitimately return 0 for "nothing right now" and must not be escalated. if (_stream is NetworkStream) { - _healthSink?.ReportIoFault(new EndOfStreamException( - "The remote endpoint closed the connection (a socket read returned 0 bytes).")); + // Same teardown rule as every other fault path, and the short back-off this + // path has always used — an orderly peer shutdown is detected in tens of + // milliseconds rather than half a second (issue #382). + if (!ReportStreamFault( + new EndOfStreamException( + "The remote endpoint closed the connection (a socket read returned 0 bytes)."), + backoffMs: NoDataBackoffMs)) + { + break; + } + + continue; } - Thread.Sleep(10); // No data available, wait briefly + Thread.Sleep(NoDataBackoffMs); // No data available, wait briefly continue; } @@ -454,12 +470,72 @@ private void ProcessMessages() // // Deliberately NOT reported to the health sink: a parse or dispatch failure is not // evidence that the link is gone, and escalating it would disconnect a perfectly - // healthy device over malformed data. Only I/O against the stream does that. - Thread.Sleep(100); + // healthy device over malformed data. Only I/O against the stream does that — which + // is why this path does not go through ReportStreamFault. + Thread.Sleep(ErrorBackoffMs); } } } + /// + /// Back-off applied after a reported failure, in milliseconds. + /// + /// + /// Whatever failed will almost certainly fail again immediately — a closed handle stays closed, + /// and a parser that rejects the bytes it holds rejects the same bytes next time — so retrying + /// at full speed makes no progress and burns a core. Also sets the escalation cadence: with the + /// transport's five-consecutive-failure threshold, a persistently failing stream is declared + /// lost after roughly half a second. + /// + private const int ErrorBackoffMs = 100; + + /// + /// Pause applied when a read returns no data, in milliseconds. Shorter than + /// because it is the idle path on most stream types. + /// + private const int NoDataBackoffMs = 10; + + /// + /// Reports a stream-level failure to the transport and to subscribers, then backs off. + /// + /// The failure that was observed. + /// How long to pause before the next iteration. + /// + /// true to keep reading; false when a stop has already been requested, in which + /// case nothing was reported and the caller must leave the loop immediately. + /// + /// + /// + /// Every fault path routes through here so the teardown rule is stated once. A failure observed + /// after has been cleared is teardown, not a device problem: closing a + /// handle is supposed to make the in-flight read fail, and the stream going unreadable + /// is what a deliberate Disconnect looks like from in here. Reporting it would put a + /// phantom "the connection died" into a consumer's diagnostics and into the transport's health + /// sink on every intentional disconnect, and the back-off would delay this thread's exit — + /// making the stop's bounded join more likely to time out, which has its own knock-on effects. + /// + /// + /// This is the same rule the loop's outer catch follows; keeping the two consistent is the + /// point. Note it was never enough on its own to let a deliberate disconnect be reported as a + /// lost connection: a continue re-tests the loop condition, so at most one failure can + /// ever be reported after a stop, against an escalation threshold of five consecutive — and the + /// transports disarm their watchdog before they touch the handle. This closes the diagnostic + /// noise, not a hole in that guarantee. + /// + /// + private bool ReportStreamFault(Exception error, int backoffMs = ErrorBackoffMs) + { + if (!_isRunning) + { + return false; + } + + _healthSink?.ReportIoFault(error); + OnErrorOccurred(error); + Thread.Sleep(backoffMs); + return true; + } + /// /// Determines whether an raised by a read is just the stream's /// configured read timeout expiring with no data, rather than a real I/O fault. From aebb59d07ea320749bcb213031310655e6177e4b Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:22:40 -0600 Subject: [PATCH 06/16] fix(consumers): isolate every subscriber and health-sink callback in the reader loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo round 3 on #415. ReportStreamFault invoked ITransportHealthSink and ErrorOccurred without isolation, so a throwing callback escaped the reader thread — process-fatal, the same shape as the escaping-catch defect fixed last round, one layer up. Concentrating the four fault sites into one helper made a single unguarded callback affect all of them at once. The route is two hops, and the crash trace confirms it exactly: the handler throws on the fault path, the loop's outer catch reports that failure by calling the same handler, and the second throw is inside a catch block with nothing above it. Verified against pre-fix code — the test host dies with the unhandled exception surfacing from ProcessMessages' outer catch. Every callback out of the loop is now isolated via SafeReportIoFault / SafeReportIoSuccess / SafeRaiseError: the two ReportStreamFault callbacks, the per-read success report, the outer catch's error raise, and the error raise in ProcessMessageBuffer's dispatch handler. Plain methods rather than a lambda helper so the once-per-read success path allocates no closure. Swallowed rather than logged, matching the convention already used for a throwing MessageReceived subscriber (#180) and mirrored across RaiseClassifiedEvent (#323), AllTransportsDeviceFinder (#354), DeviceFinderBase and RaiseGapDetected. Two regression tests assert the reader keeps consuming — a real message delivered after the throwing phase — not merely that nothing surfaced. Both verified failing pre-fix: the subscriber case crashes the host, the health-sink case silently stops delivering messages. Co-Authored-By: Claude Opus 5 --- .../StreamMessageConsumerBackoffTests.cs | 168 ++++++++++++++++++ .../Consumers/StreamMessageConsumer.cs | 80 ++++++++- 2 files changed, 243 insertions(+), 5 deletions(-) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs index 6a740681..2fbdb8eb 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs @@ -206,6 +206,92 @@ public void AStopThatLandsDuringAReadFailure_ReportsNothing() Assert.Equal(0, Volatile.Read(ref errors)); } + [Fact] + public void AThrowingErrorSubscriber_DoesNotStopMessageConsumption() + { + // The reader loop is the top of a background thread, so a callback that throws out of it + // does not merely stop consumption — it terminates the process. The route is two hops: the + // handler throws on the fault path, the outer catch reports the failure by calling that + // same handler, and the second throw is inside a catch block with nothing above it. + using var stream = new RecoveringStream(); + var errors = 0; + var received = new List(); + + using var consumer = new StreamMessageConsumer(stream, new LineBasedMessageParser()); + consumer.MessageReceived += (_, e) => + { + lock (received) + { + received.Add(e.Message.Data); + } + }; + consumer.ErrorOccurred += (_, _) => + { + Interlocked.Increment(ref errors); + throw new InvalidOperationException("a subscriber that throws on every error"); + }; + + consumer.Start(); + + // Let the failing phase drive the fault path through the throwing subscriber. + Assert.True(WaitUntil(() => Volatile.Read(ref errors) >= 2, TimeSpan.FromSeconds(10)), + "the fault path never ran"); + + // The link recovers. The reader must still be alive and still consuming — this is the + // assertion that matters, not merely that no exception surfaced in the test. + stream.Recover("$DAQiFi\r\n"); + + Assert.True(WaitUntil(() => + { + lock (received) + { + return received.Count >= 1; + } + }, TimeSpan.FromSeconds(10)), "the reader stopped consuming after a subscriber threw"); + + Assert.True(consumer.IsRunning); + consumer.StopSafely(timeoutMs: 2000); + } + + [Fact] + public void AThrowingHealthSink_DoesNotStopMessageConsumption() + { + // Same guarantee for the transport side: ITransportHealthSink is implemented by consumers + // of this library, so it is exactly as untrusted as an event subscriber. + using var stream = new RecoveringStream(); + var sink = new ThrowingHealthSink(); + var received = new List(); + + using var consumer = new StreamMessageConsumer( + stream, new LineBasedMessageParser(), healthSink: sink); + consumer.MessageReceived += (_, e) => + { + lock (received) + { + received.Add(e.Message.Data); + } + }; + + consumer.Start(); + + Assert.True(WaitUntil(() => sink.FaultCalls >= 2, TimeSpan.FromSeconds(10)), + "the fault path never ran"); + + stream.Recover("$DAQiFi\r\n"); + + Assert.True(WaitUntil(() => + { + lock (received) + { + return received.Count >= 1; + } + }, TimeSpan.FromSeconds(10)), "the reader stopped consuming after the health sink threw"); + + Assert.True(sink.SuccessCalls >= 1, "the success path must have been exercised too"); + Assert.True(consumer.IsRunning); + consumer.StopSafely(timeoutMs: 2000); + } + private static bool WaitUntil(Func condition, TimeSpan timeout) { var deadline = DateTime.UtcNow + timeout; @@ -222,6 +308,88 @@ private static bool WaitUntil(Func condition, TimeSpan timeout) return condition(); } + /// + /// A health sink that fails every callback, standing in for a consumer-supplied transport with + /// a bug in it. + /// + private sealed class ThrowingHealthSink : ITransportHealthSink + { + private int _faultCalls; + private int _successCalls; + + public int FaultCalls => Volatile.Read(ref _faultCalls); + + public int SuccessCalls => Volatile.Read(ref _successCalls); + + public void ReportIoFault(Exception error) + { + Interlocked.Increment(ref _faultCalls); + throw new InvalidOperationException("a health sink that throws"); + } + + public void ReportIoSuccess() + { + Interlocked.Increment(ref _successCalls); + throw new InvalidOperationException("a health sink that throws"); + } + } + + /// + /// A stream that fails its reads until told to recover, then delivers a line — so "the reader is + /// still consuming" can be asserted directly rather than inferred. + /// + private sealed class RecoveringStream : Stream + { + private readonly object _gate = new(); + private byte[]? _pending; + + public void Recover(string text) + { + lock (_gate) + { + _pending = System.Text.Encoding.UTF8.GetBytes(text); + } + } + + public override int Read(byte[] buffer, int offset, int count) + { + lock (_gate) + { + if (_pending == null) + { + Thread.Sleep(5); + throw new IOException("the link is down"); + } + + var length = Math.Min(_pending.Length, count); + Array.Copy(_pending, 0, buffer, offset, length); + _pending = null; + return length; + } + } + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + + public override long Position + { + get => throw new NotSupportedException(); + set => throw new NotSupportedException(); + } + + public override void Flush() + { + } + + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + private sealed class RecordingHealthSink : ITransportHealthSink { private int _faultCount; diff --git a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs index 379810cc..0bbb09e1 100644 --- a/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs +++ b/src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs @@ -428,7 +428,7 @@ private void ProcessMessages() // A successful read clears any run of failures the transport has accumulated, so // a stream that glitches and recovers is never mistaken for a disconnected device. - _healthSink?.ReportIoSuccess(); + SafeReportIoSuccess(); // Add received data to message buffer (guarded: a caller may be reading // QueuedMessageCount and ClearBuffer's drain runs on this same thread). @@ -459,7 +459,9 @@ private void ProcessMessages() break; } - OnErrorOccurred(ex); + // Isolated: this is the last catch above a background thread's entry point, so a + // throwing subscriber here has nothing left to stop it (see SafeRaiseError). + SafeRaiseError(ex); // Back off before the next iteration. What reaches here is a failure in the // parse/dispatch half of the loop, and that half is deterministic with respect to @@ -530,12 +532,76 @@ private bool ReportStreamFault(Exception error, int backoffMs = ErrorBackoffMs) return false; } - _healthSink?.ReportIoFault(error); - OnErrorOccurred(error); + SafeReportIoFault(error); + SafeRaiseError(error); Thread.Sleep(backoffMs); return true; } + /// + /// Reports a failed transfer to the transport, absorbing anything the sink throws. + /// + private void SafeReportIoFault(Exception error) + { + try + { + _healthSink?.ReportIoFault(error); + } + catch + { + // See SafeRaiseError. + } + } + + /// + /// Reports a successful transfer to the transport, absorbing anything the sink throws. + /// + /// + /// Runs once per successful read, so this is deliberately a plain method rather than a lambda + /// helper — no closure is allocated on the hot path. + /// + private void SafeReportIoSuccess() + { + try + { + _healthSink?.ReportIoSuccess(); + } + catch + { + // See SafeRaiseError. + } + } + + /// + /// Raises , absorbing anything a subscriber throws. + /// + /// + /// + /// Every callback out of the reader loop — subscriber or transport health sink — is isolated, + /// because this loop is the top of a background thread: an exception that escapes it does not + /// merely stop message consumption, it terminates the process. The route is short and real. A + /// throwing handler here propagates into the loop's outer catch, which reports the failure by + /// calling this same handler; it throws again, and that second throw is inside a catch + /// block with nothing above it. Isolating the callback closes both hops at once. + /// + /// + /// Swallowed rather than logged, per the convention this class already follows for a throwing + /// subscriber: a consumer that breaks its own diagnostics is not + /// permitted to affect anyone else's data. + /// + /// + private void SafeRaiseError(Exception error, byte[]? rawData = null) + { + try + { + OnErrorOccurred(error, rawData); + } + catch + { + // A misbehaving subscriber must not stop message consumption or take down the process. + } + } + /// /// Determines whether an raised by a read is just the stream's /// configured read timeout expiring with no data, rather than a real I/O fault. @@ -582,7 +648,11 @@ private void ProcessMessageBuffer() } catch (Exception ex) { - OnErrorOccurred(ex); + // A throwing MessageReceived subscriber is reported rather than propagated, so one + // bad handler cannot starve the others of this batch. The report itself is isolated + // too: raising it from inside a catch block leaves nowhere for a second throw to go + // (see SafeRaiseError). + SafeRaiseError(ex); } } } From b130df6e7eda9ffea26caefe2d1b1f307cbb5c9d Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:23:31 -0600 Subject: [PATCH 07/16] fix(device): unwind a reconnect that raced a caller teardown, and honour a zero backoff MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings from review. A caller's Disconnect() lands *inside* an in-flight reconnect attempt, not tidily between two — opening a port blocks for as long as it takes. The attempt then finished, reopening the transport, starting a reader and setting Connected, and the loop's staleness check bailed out without unwinding any of it: a device the caller had closed came quietly back to life. The check cannot simply move earlier (the race is the blocking call itself), so the loop now records what the caller actually wanted and puts its own half-built session back down after Connect, and again after initialization and restore, before declaring success. CalculateDelay turned InitialDelay=Zero into MaxDelay at very high attempt numbers: the exponential factor overflows to infinity and 0 x infinity is NaN, which the NaN guard answered with the cap — the opposite of a policy asking for immediate retries. Zero in, zero out. MaxDelay now also caps the first attempt, so the ceiling means what it says. Six regression tests, each verified to fail against the pre-fix code — the teardown race reproduces as "Expected: Disconnected, Actual: Connected". Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 149 ++++++++++++++++++ .../Device/ReconnectOptionsTests.cs | 52 ++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 103 ++++++++++-- src/Daqifi.Core/Device/ReconnectOptions.cs | 23 ++- 4 files changed, 313 insertions(+), 14 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 2f4cb474..94b34055 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -412,6 +412,81 @@ public async Task Disconnect_DuringAReconnect_WinsAndTheLoopStopsQuietly() Assert.False(device.IsReconnecting); } + [Fact] + public void Disconnect_WhileAConnectAttemptIsInFlight_DoesNotLeaveTheDeviceQuietlyAlive() + { + // Opening a port blocks for as long as it takes, so a caller pulling the plug on the device + // lands *inside* an attempt rather than tidily between two. The attempt still finishes and + // brings the transport back up; if the loop merely bails out at that point, the caller is + // left holding a device they closed that is silently open again, reporting Connected, with + // a reader thread running on it. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Resurrected Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + var connectGate = transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + transport.SimulateDrop(); + + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the reconnect never reached the transport's connect"); + + // The caller shuts the device down while that connect is parked in flight. + device.Disconnect(); + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + + // Now let the attempt finish. It will succeed and re-open the transport. + connectGate.Set(); + + WaitUntil(() => !device.IsReconnecting, "the reconnect loop never finished"); + + // The caller's decision has to survive it. + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(device.IsConnected); + Assert.False(transport.IsConnected); + } + + [Fact] + public async Task Disconnect_WhileInitializationIsInFlight_IsNotReportedAsASuccessfulReconnect() + { + // The same race one step later: the transport came back and initialization was most of the + // way through when the caller disconnected. Announcing a recovered session at that point + // would hand them a device they had just closed. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedBaseDevice("Late Abandoned Device", transport); + device.ReconnectOptions = FastPolicy(); + + device.Connect(); + await device.InitializeAsync(); + + var initGate = new ManualResetEventSlim(false); + device.InitializeEntered.Reset(); + device.InitializeGate = initGate; + + var reconnectedCount = 0; + device.Reconnected += (_, _) => Interlocked.Increment(ref reconnectedCount); + + transport.SimulateDrop(); + + Assert.True( + device.InitializeEntered.Wait(EventTimeout), + "the reconnect never reached initialization"); + + device.Disconnect(); + initGate.Set(); + + WaitUntil(() => !device.IsReconnecting, "the reconnect loop never finished"); + + Assert.Equal(0, Volatile.Read(ref reconnectedCount)); + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(device.IsConnected); + Assert.False(transport.IsConnected); + } + [Fact] public void DisposingDuringAReconnect_DoesNotThrow() { @@ -702,6 +777,55 @@ private DaqifiOutMessage BuildStatusMessage() } } + /// + /// A plain (non-streaming) device on a scripted transport, whose initialization can be parked + /// mid-flight. Deriving from rather than the streaming subclass is + /// the point: its session restore is the base no-op, so nothing in it re-checks the connection + /// on the loop's behalf, and the loop's own final guard is what has to catch a caller who + /// disconnected while initialization was running. + /// + private sealed class ScriptedBaseDevice : DaqifiDevice + { + private int _initializeCount; + + public ScriptedBaseDevice(string name, IStreamTransport transport) + : base(name, transport) + { + } + + /// Signalled once initialization is past its entry checks. + public ManualResetEventSlim InitializeEntered { get; } = new(false); + + /// Parks initialization after its entry checks until set. + public ManualResetEventSlim? InitializeGate { get; set; } + + public int InitializeCount => Volatile.Read(ref _initializeCount); + + public override void Send(IOutboundMessage message) + { + } + + public override Task InitializeAsync( + TimeSpan? channelPopulationTimeout = null, + CancellationToken cancellationToken = default) + { + Interlocked.Increment(ref _initializeCount); + cancellationToken.ThrowIfCancellationRequested(); + + if (!IsConnected) + { + return Task.FromException(new DeviceNotConnectedException()); + } + + // Entry checks are done; everything past here is the seconds of SCPI round-trips a real + // initialization spends, which is the window a caller's Disconnect lands in. + InitializeEntered.Set(); + InitializeGate?.Wait(TimeSpan.FromSeconds(30)); + + return Task.CompletedTask; + } + } + /// /// A transport that can be dropped on command and told to refuse the next N reconnects, so the /// whole reconnect loop is reachable without hardware. Each connect hands out a fresh @@ -790,12 +914,37 @@ public void SimulateDrop() new TransportStatusEventArgs(false, ConnectionInfo, new IOException("the device went away"))); } + /// + /// Signalled as soon as a connect attempt begins, before it does any work. + /// + public ManualResetEventSlim ConnectEntered { get; } = new(false); + + private volatile ManualResetEventSlim? _connectGate; + + /// + /// Parks every subsequent connect attempt until the returned gate is set, so a test can + /// act — pull the device out from under the loop, say — while one is genuinely in flight. + /// Real connects block for as long as opening a port takes; this makes that window + /// controllable instead of a race to lose. + /// + public ManualResetEventSlim BlockConnects() + { + var gate = new ManualResetEventSlim(false); + _connectGate = gate; + return gate; + } + public Task ConnectAsync() => ConnectAsync(null); public Task ConnectAsync(ConnectionRetryOptions? retryOptions) { Interlocked.Increment(ref _connectCount); + // Outside the lock: a test holding the gate must still be able to inspect the + // transport and drive the device while the connect is parked here. + ConnectEntered.Set(); + _connectGate?.Wait(TimeSpan.FromSeconds(30)); + lock (_gate) { ObjectDisposedException.ThrowIf(_disposed, this); diff --git a/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs b/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs index 4f355034..64beeee9 100644 --- a/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs +++ b/src/Daqifi.Core.Tests/Device/ReconnectOptionsTests.cs @@ -84,6 +84,58 @@ public void AZeroInitialDelayStaysZero() Assert.Equal(TimeSpan.Zero, options.CalculateDelay(7)); } + [Theory] + [InlineData(64)] + [InlineData(1024)] + [InlineData(1075)] + [InlineData(4096)] + [InlineData(int.MaxValue)] + public void AZeroInitialDelayStaysZero_EvenWhereTheBackoffFactorOverflows(int attemptNumber) + { + // Regression: the exponential factor overflows to +Infinity past roughly attempt 1075 at a + // multiplier of 2, and 0 x Infinity is NaN. That NaN used to be answered with MaxDelay, so a + // policy configured for immediate retries silently became a 30-second wait — the opposite of + // what it asked for. + var options = new ReconnectOptions + { + InitialDelay = TimeSpan.Zero, + BackoffMultiplier = 2.0, + MaxDelay = TimeSpan.FromSeconds(30) + }; + + Assert.Equal(TimeSpan.Zero, options.CalculateDelay(attemptNumber)); + } + + [Fact] + public void AnOverflowingBackoffOnARealDelay_SettlesAtTheCap() + { + // The other side of the same overflow: with a positive InitialDelay the product is + // +Infinity rather than NaN, and the cap is the right answer. + var options = new ReconnectOptions + { + InitialDelay = TimeSpan.FromSeconds(1), + BackoffMultiplier = 2.0, + MaxDelay = TimeSpan.FromSeconds(30) + }; + + Assert.Equal(TimeSpan.FromSeconds(30), options.CalculateDelay(2000)); + Assert.Equal(TimeSpan.FromSeconds(30), options.CalculateDelay(int.MaxValue)); + } + + [Fact] + public void MaxDelayCapsTheFirstAttemptToo() + { + // A ceiling that exempted the very first wait would not be a ceiling. + var options = new ReconnectOptions + { + InitialDelay = TimeSpan.FromSeconds(10), + MaxDelay = TimeSpan.FromSeconds(2) + }; + + Assert.Equal(TimeSpan.FromSeconds(2), options.CalculateDelay(1)); + Assert.Equal(TimeSpan.FromSeconds(2), options.CalculateDelay(5)); + } + [Theory] [InlineData(0)] [InlineData(-1)] diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index c15612e9..5dcf0f40 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -560,6 +560,7 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n /// public void Connect() { + _callerWantsDisconnected = false; SupersedeReconnect(); ConnectCore(); } @@ -648,8 +649,12 @@ private void ConnectCore() /// public void Disconnect() { - // A user-issued teardown always beats an automatic reconnect: stop the loop before - // tearing anything down, so it cannot re-open the transport behind the caller's back. + // A user-issued teardown always beats an automatic reconnect: record the intent and + // stop the loop before tearing anything down. Setting the flag first matters — a + // reconnect already inside a blocking Connect() will finish it regardless, and this is + // what tells it to put the session straight back down instead of leaving the device + // quietly alive behind the caller's back. + _callerWantsDisconnected = true; SupersedeReconnect(); DisconnectCore(ConnectionStatus.Disconnected); } @@ -1620,11 +1625,20 @@ public ReconnectOptions ReconnectOptions // Bumped by every caller-issued Connect/Disconnect/Dispose. The reconnect loop captures it // when it starts and re-checks before each step that touches device state; a change means - // the caller has moved on and the loop must unwind without touching anything. This is what - // keeps the loop from re-opening a transport the caller just closed, without Disconnect() - // having to block on a loop that may be calling back into it. + // the caller has moved on and the loop must unwind. This is what keeps the loop from + // re-opening a transport the caller just closed, without Disconnect() having to block on a + // loop that may be calling back into it. private int _sessionEpoch; + // Which way the caller last pointed the device. The epoch says *that* a caller superseded + // the loop; this says *what they wanted*, which is what decides how the loop unwinds. + // Connect() is a blocking, multi-second operation, so a caller can always land in the + // middle of one — no amount of checking beforehand avoids that, and by the time the loop + // looks again it may be holding a session it has just brought up. If the caller wants the + // device down, that session is the loop's own doing and has to go back down with it; + // if the caller wants it up, it is theirs and must be left strictly alone. + private volatile bool _callerWantsDisconnected; + /// /// Gets a value indicating whether an automatic reconnect is currently in progress. /// @@ -1803,12 +1817,13 @@ private async Task RunReconnectLoopAsync( ConnectCore(); - // The caller took the session over while the connect was in flight. Whatever - // they did next owns the device now, so stop here without touching status or - // tearing anything down — either could undo the session they just established. - if (IsSessionStale(epoch)) + // Connect() blocks for as long as opening the port takes, so a caller can — and + // on a slow serial open, will — land squarely in the middle of it. There is now + // a live transport and a running reader that this loop built, so bailing out + // here is not enough on its own: if the caller wants the device down, the + // session has to be taken back down with it. + if (AbandonIfSuperseded(epoch, attempt, lastError)) { - ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: true); return; } @@ -1817,6 +1832,14 @@ private async Task RunReconnectLoopAsync( var streamingResumed = await RestoreSessionSnapshotAsync( options, cancellationToken).ConfigureAwait(false); + // Same again before declaring victory: initialization and restore are several + // seconds of SCPI round-trips, and a session the caller has since disowned must + // not be handed to them as a successful reconnect. + if (AbandonIfSuperseded(epoch, attempt, lastError)) + { + return; + } + SafeLog(() => _logger.LogInformation( "[Reconnect] Device '{DeviceName}' reconnected on attempt {Attempt}.", Name, attempt)); @@ -1844,6 +1867,66 @@ private async Task RunReconnectLoopAsync( ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: false); } + /// + /// Checks whether a caller has superseded the session this loop is rebuilding and, if so, + /// unwinds whatever the loop has already brought up before reporting that it stopped. + /// + /// + /// + /// Called at the points where the loop is holding a live session of its own making — after + /// , and again once initialization and restore are done. Both are + /// preceded by long blocking work, which is exactly when a caller's + /// lands, so a check beforehand cannot substitute for this one. + /// + /// + /// What "unwind" means depends on what the caller wanted, which is why the epoch alone is + /// not enough to decide it. After a or a disposal the live session + /// is the loop's own doing and is torn straight back down — leaving it up is a device the + /// caller closed quietly coming back to life. After a caller's own + /// the session belongs to them, and tearing it down would be the same bug in reverse, so it + /// is left alone and only logged. + /// + /// + /// true if the loop must stop. + private bool AbandonIfSuperseded(int epoch, int attempt, Exception? lastError) + { + if (!IsSessionStale(epoch)) + { + return false; + } + + if (_callerWantsDisconnected || _disposed) + { + SafeLog(() => _logger.LogInformation( + "[Reconnect] Device '{DeviceName}' was disconnected while a reconnect attempt was in flight; " + + "closing the connection the attempt had established.", + Name)); + + try + { + DisconnectCore(ConnectionStatus.Disconnected); + } + catch (Exception ex) + { + // Best-effort unwind of an abandoned attempt. A transport already disposed out + // from under us can throw here, and there is nothing left to salvage by + // letting that escape into the loop's retry logic. + SafeLog(() => _logger.LogDebug( + ex, "[Reconnect] Closing the abandoned reconnect attempt's connection failed.")); + } + } + else + { + SafeLog(() => _logger.LogWarning( + "[Reconnect] Device '{DeviceName}' was reconnected by its caller while an automatic " + + "reconnect attempt was in flight; leaving the caller's connection alone.", + Name)); + } + + ReportReconnectStopped(epoch, attempt, lastError, wasCanceled: true); + return true; + } + /// /// True once a caller-issued connect, disconnect or disposal has superseded the session the /// reconnect loop was started for. diff --git a/src/Daqifi.Core/Device/ReconnectOptions.cs b/src/Daqifi.Core/Device/ReconnectOptions.cs index 5c6ad8f7..fc40e21c 100644 --- a/src/Daqifi.Core/Device/ReconnectOptions.cs +++ b/src/Daqifi.Core/Device/ReconnectOptions.cs @@ -168,19 +168,34 @@ public double BackoffMultiplier /// The 1-based attempt number. /// /// multiplied by once per attempt - /// already made, capped at . + /// already made, capped at . Always when + /// is zero, however many attempts have been made. /// public TimeSpan CalculateDelay(int attemptNumber) { + // Zero in, zero out. A caller who asked for immediate retries must get them at every + // attempt number: multiplying zero by the growing backoff factor is still zero, right up + // until that factor overflows Math.Pow to infinity, where 0 × ∞ is NaN and the arithmetic + // below would hand back MaxDelay — the exact opposite of the configured policy. + if (InitialDelay == TimeSpan.Zero) + { + return TimeSpan.Zero; + } + + // MaxDelay is a ceiling on every wait, including the first: a policy whose MaxDelay is + // below its InitialDelay means what it says rather than exempting attempt 1. if (attemptNumber <= 1) { - return InitialDelay; + return InitialDelay < MaxDelay ? InitialDelay : MaxDelay; } var delayMs = InitialDelay.TotalMilliseconds * Math.Pow(BackoffMultiplier, attemptNumber - 1); - // Math.Pow overflows to +Infinity for a large enough attempt count; Math.Min then yields - // MaxDelay, which is the intended cap, but guard NaN (0 * Infinity) explicitly. + // Math.Pow overflows to +Infinity for a large enough attempt count. With a positive + // InitialDelay the product is then +Infinity too, and Math.Min yields MaxDelay — the + // intended cap. NaN is unreachable from here now that a zero InitialDelay returns above + // and BackoffMultiplier rejects NaN, so this is a backstop against a future relaxation of + // either, not a live path. if (double.IsNaN(delayMs)) { return MaxDelay; From 048d29e89b1f441695e37c4f30d051abced171ba Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:32:51 -0600 Subject: [PATCH 08/16] test: honor Stream.Read's partial-read contract in the test doubles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Qodo round 4 on #415. RecoveringStream.Read copied min(payload, count) bytes and then discarded the unread suffix, so the tests that depend on it were silently coupled to the consumer's read buffer being larger than the payload — they would have kept passing for a reason unrelated to the code under test, and would have started failing on an unrelated bufferSize change. A test double that violates the contract is a latent false-negative generator, and these tests are the evidence for this PR's claims. RecoveringStream now retains the remainder across calls and clears the payload only once fully drained. DeviceErrorSurfaceTests.ScriptedStream had the same defect in a queue nothing enqueues to any more, so the queue is deleted rather than fixed. Added TheRecoveringStreamHelper_DeliversAWholePayloadAcrossPartialReads, which drives the helper with a one-byte read buffer so the partial-read path is actually exercised; verified it fails against the old helper ("the payload never arrived in full"). Re-verified with the corrected helper that the round-3 tests still fail against pre-fix production code: the throwing-subscriber case still crashes the host, the throwing-health-sink case still stops consuming. Co-Authored-By: Claude Opus 5 --- .../StreamMessageConsumerBackoffTests.cs | 59 ++++++++++++++++++- .../Device/DeviceErrorSurfaceTests.cs | 19 ++---- 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs index 2fbdb8eb..bdb59469 100644 --- a/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs +++ b/src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBackoffTests.cs @@ -292,6 +292,45 @@ public void AThrowingHealthSink_DoesNotStopMessageConsumption() consumer.StopSafely(timeoutMs: 2000); } + [Fact] + public void TheRecoveringStreamHelper_DeliversAWholePayloadAcrossPartialReads() + { + // Guards the harness the two tests above depend on. Driven with a one-byte read buffer, so + // the payload can only arrive if the helper retains what a read did not take — a helper + // that dropped the unread suffix would deliver "$" and never complete the line, and those + // tests would then be asserting nothing. + using var stream = new RecoveringStream(); + var received = new List(); + + using var consumer = new StreamMessageConsumer( + stream, new LineBasedMessageParser(), bufferSize: 1); + consumer.MessageReceived += (_, e) => + { + lock (received) + { + received.Add(e.Message.Data); + } + }; + + consumer.Start(); + stream.Recover("$DAQiFi\r\n"); + + Assert.True(WaitUntil(() => + { + lock (received) + { + return received.Count >= 1; + } + }, TimeSpan.FromSeconds(10)), "the payload never arrived in full"); + + lock (received) + { + Assert.Equal("$DAQiFi", received[0]); + } + + consumer.StopSafely(timeoutMs: 2000); + } + private static bool WaitUntil(Func condition, TimeSpan timeout) { var deadline = DateTime.UtcNow + timeout; @@ -342,12 +381,14 @@ private sealed class RecoveringStream : Stream { private readonly object _gate = new(); private byte[]? _pending; + private int _pendingOffset; public void Recover(string text) { lock (_gate) { _pending = System.Text.Encoding.UTF8.GetBytes(text); + _pendingOffset = 0; } } @@ -361,9 +402,21 @@ public override int Read(byte[] buffer, int offset, int count) throw new IOException("the link is down"); } - var length = Math.Min(_pending.Length, count); - Array.Copy(_pending, 0, buffer, offset, length); - _pending = null; + // Honor Stream.Read's contract: hand back at most count bytes and keep the + // remainder for the next call. Copying min(payload, count) and then dropping the + // rest would silently couple these tests to the consumer's read buffer being larger + // than the payload — the test would keep passing for a reason unrelated to the code + // under test, and would start failing on an unrelated bufferSize change. + var length = Math.Min(_pending.Length - _pendingOffset, count); + Array.Copy(_pending, _pendingOffset, buffer, offset, length); + _pendingOffset += length; + + if (_pendingOffset == _pending.Length) + { + _pending = null; + _pendingOffset = 0; + } + return length; } } diff --git a/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs b/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs index ac45fa5d..96e7ee1c 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceErrorSurfaceTests.cs @@ -515,8 +515,6 @@ public void Dispose() /// internal sealed class ScriptedStream : Stream { - private readonly Queue _pending = new(); - private readonly object _gate = new(); private int _readCount; public volatile bool FailReads; @@ -540,19 +538,10 @@ public override int Read(byte[] buffer, int offset, int count) throw new IOException("the device is gone"); } - lock (_gate) - { - if (_pending.Count == 0) - { - Thread.Sleep(5); - return 0; - } - - var chunk = _pending.Dequeue(); - var length = Math.Min(chunk.Length, count); - Array.Copy(chunk, 0, buffer, offset, length); - return length; - } + // Idle. These tests drive the failure paths only, so this stream never delivers data — + // it deliberately has no payload queue to get the partial-read contract wrong with. + Thread.Sleep(5); + return 0; } public override void Write(byte[] buffer, int offset, int count) From 5e157b417f9e72b4effd3e6159d281f25647a2b1 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:41:26 -0600 Subject: [PATCH 09/16] fix(device): serialize connect against disconnect, and bound the test gate waits Two findings from round two. Automatic reconnect introduced a second thread that opens and closes the transport, and cancellation is not synchronization: SupersedeReconnect asks the loop to stop and returns, but a loop already inside a blocking transport connect runs to completion regardless. A caller's Disconnect could therefore be closing the same serial port while the reconnect was opening it, and both threads could build and start a message consumer, leaving two readers on one stream. ConnectCore and DisconnectCore now run under a reentrant lifecycle monitor. Scoped deliberately to the lifecycle pair, not the general per-device operation serialization of #342: it is an internal invariant that the device never drives its own transport from two threads at once, it touches no public behaviour when uncontended, and it leaves the _textExchangeLock ordering question untouched. Reentrant because both methods raise StatusChanged from inside their critical section and a handler calling Disconnect from there must keep working. On timeout it proceeds unsynchronized, which is exactly what shipped before. The scripted test transport and device parked background threads on gate waits of 30s while the assertion timeout is 15s, so a failing test could leave a thread inside the transport long after it gave up. Bounded to 5s, matching the background-wait convention in #364 and #411. Regression tests verified against the pre-fix code: the race reproduces as "a caller's Disconnect was inside the transport at the same time as the reconnect's connect". Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 146 ++++++++++++++++-- src/Daqifi.Core/Device/DaqifiDevice.cs | 88 ++++++++++- 2 files changed, 217 insertions(+), 17 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 94b34055..5f6bbabe 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -23,6 +23,16 @@ public class DeviceReconnectTests { private static readonly TimeSpan EventTimeout = TimeSpan.FromSeconds(15); + /// + /// Safety valve on the gates that park a scripted connect or initialization mid-flight. A test + /// that is going to pass releases its gate within milliseconds, so this only ever fires on a + /// test that has already failed — which is exactly why it has to stay well under + /// . A gate outliving the assertion that gave up on it leaves a + /// background thread parked inside the transport long after the test finished, where it can + /// interleave with whatever runs next. + /// + private static readonly TimeSpan GateTimeout = TimeSpan.FromSeconds(5); + /// A policy that reconnects promptly, so tests do not spend their time waiting. private static ReconnectOptions FastPolicy(int maxAttempts = 4) => new() { @@ -435,13 +445,16 @@ public void Disconnect_WhileAConnectAttemptIsInFlight_DoesNotLeaveTheDeviceQuiet transport.ConnectEntered.Wait(EventTimeout), "the reconnect never reached the transport's connect"); - // The caller shuts the device down while that connect is parked in flight. + // Release the parked attempt from another thread. The caller's Disconnect now waits for an + // in-flight connect rather than running alongside it, so releasing after it returns would + // wait on a call that is itself waiting on the gate. + ReleaseAfter(connectGate, TimeSpan.FromMilliseconds(100)); + + // The caller shuts the device down while that connect is in flight. The attempt still + // finishes and re-opens the transport. device.Disconnect(); Assert.Equal(ConnectionStatus.Disconnected, device.Status); - // Now let the attempt finish. It will succeed and re-open the transport. - connectGate.Set(); - WaitUntil(() => !device.IsReconnecting, "the reconnect loop never finished"); // The caller's decision has to survive it. @@ -450,6 +463,58 @@ public void Disconnect_WhileAConnectAttemptIsInFlight_DoesNotLeaveTheDeviceQuiet Assert.False(transport.IsConnected); } + [Fact] + public void ACallerDisconnect_NeverDrivesTheTransportAlongsideAnInFlightReconnect() + { + // Cancelling is not synchronizing. SupersedeReconnect asks the loop to stop and returns + // immediately, but a loop already inside a blocking transport connect cannot be + // interrupted — so without a lifecycle lock the caller's Disconnect closes the port while + // the reconnect is still opening it, and both threads race to build a message consumer. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Contended Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + var connectGate = transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + transport.SimulateDrop(); + + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the reconnect never reached the transport's connect"); + + ReleaseAfter(connectGate, TimeSpan.FromMilliseconds(100)); + + device.Disconnect(); + + WaitUntil(() => !device.IsReconnecting, "the reconnect loop never finished"); + + Assert.False( + transport.SawConcurrentLifecycleCalls, + "a caller's Disconnect was inside the transport at the same time as the reconnect's connect"); + + // The round-one guarantee still holds on top of the new one. + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(device.IsConnected); + Assert.False(transport.IsConnected); + } + + [Fact] + public void ANormalConnectDisconnectCycle_ReportsNoLifecycleContention() + { + // The uncontended path is unchanged by the lock: nothing waits, nothing overlaps. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Ordinary Device", transport); + + ConnectAndInitialize(device); + device.Disconnect(); + + Assert.False(transport.SawConcurrentLifecycleCalls); + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + } + [Fact] public async Task Disconnect_WhileInitializationIsInFlight_IsNotReportedAsASuccessfulReconnect() { @@ -633,6 +698,19 @@ private static void WaitUntilRetrying(DaqifiStreamingDevice device) => () => device.Status == ConnectionStatus.Retrying, "the reconnect loop never reached its backoff wait"); + /// + /// Releases a gate from another thread after a short delay, for tests where the call that would + /// otherwise release it is itself waiting on that gate. + /// + private static void ReleaseAfter(ManualResetEventSlim gate, TimeSpan delay) + { + _ = Task.Run(async () => + { + await Task.Delay(delay).ConfigureAwait(false); + gate.Set(); + }); + } + private static void WaitUntil(Func condition, string because) { var deadline = DateTime.UtcNow + EventTimeout; @@ -820,7 +898,7 @@ public override Task InitializeAsync( // Entry checks are done; everything past here is the seconds of SCPI round-trips a real // initialization spends, which is the window a caller's Disconnect lands in. InitializeEntered.Set(); - InitializeGate?.Wait(TimeSpan.FromSeconds(30)); + InitializeGate?.Wait(GateTimeout); return Task.CompletedTask; } @@ -934,16 +1012,50 @@ public ManualResetEventSlim BlockConnects() return gate; } + private int _lifecycleDepth; + private volatile bool _sawConcurrentLifecycleCalls; + + /// + /// True if a connect and a disconnect were ever inside this transport at the same time. + /// A real transport is a serial port or a socket: opening one while closing it is + /// undefined, so the device must never do it. + /// + public bool SawConcurrentLifecycleCalls => _sawConcurrentLifecycleCalls; + + private void EnterLifecycle() + { + if (Interlocked.Increment(ref _lifecycleDepth) > 1) + { + _sawConcurrentLifecycleCalls = true; + } + } + + private void ExitLifecycle() => Interlocked.Decrement(ref _lifecycleDepth); + public Task ConnectAsync() => ConnectAsync(null); public Task ConnectAsync(ConnectionRetryOptions? retryOptions) { Interlocked.Increment(ref _connectCount); + EnterLifecycle(); + try + { + ConnectCore(retryOptions); + } + finally + { + ExitLifecycle(); + } + return Task.CompletedTask; + } + + private void ConnectCore(ConnectionRetryOptions? retryOptions) + { // Outside the lock: a test holding the gate must still be able to inspect the // transport and drive the device while the connect is parked here. ConnectEntered.Set(); - _connectGate?.Wait(TimeSpan.FromSeconds(30)); + _connectGate?.Wait(GateTimeout); lock (_gate) { @@ -964,21 +1076,29 @@ public Task ConnectAsync(ConnectionRetryOptions? retryOptions) _watchdog?.Arm(); StatusChanged?.Invoke(this, new TransportStatusEventArgs(true, ConnectionInfo)); - return Task.CompletedTask; } public Task DisconnectAsync() { - // Disarm first: closing the handle is what makes in-flight reads fail, and none of that - // is a lost connection. - _watchdog?.Disarm(); + EnterLifecycle(); + try + { + // Disarm first: closing the handle is what makes in-flight reads fail, and none of + // that is a lost connection. + _watchdog?.Disarm(); - lock (_gate) + lock (_gate) + { + _isConnected = false; + } + + StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); + } + finally { - _isConnected = false; + ExitLifecycle(); } - StatusChanged?.Invoke(this, new TransportStatusEventArgs(false, ConnectionInfo)); return Task.CompletedTask; } diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 5dcf0f40..171ed374 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -551,12 +551,86 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n _transport.StatusChanged += OnTransportStatusChanged; } + /// + /// Serializes against . + /// + /// + /// + /// Automatic reconnection (issue #379) introduced a second thread that opens and closes the + /// transport, and cancellation is not synchronization: + /// asks the loop to stop and returns immediately, but a loop already inside a blocking + /// Connect() cannot be interrupted and will run to completion. Without this, a + /// caller's could be opening and closing the same serial port + /// concurrently, and both threads could build and start a message consumer — leaving two + /// readers on one stream, the framing corruption this class refuses to risk anywhere else. + /// + /// + /// Narrow on purpose. This is an internal lifecycle invariant — the device never drives its + /// own transport from two threads at once — and deliberately not the general + /// per-device operation serialization of issue #342, which has to decide ordering across + /// the whole public API and interacts with _textExchangeLock. Nothing here changes + /// what any public method does when uncontended. + /// + /// + /// A rather than a semaphore because it is reentrant: both methods + /// raise from inside their critical section, and a consumer + /// handler calling from there is re-entry on the same thread. That + /// runs nested today with no lock at all, and must keep working rather than deadlocking. + /// + /// + private readonly object _lifecycleLock = new(); + + /// + /// How long a lifecycle operation waits for one already in flight. Matches the budget + /// already allows itself on _textExchangeLock. + /// + private static readonly TimeSpan LifecycleLockTimeout = TimeSpan.FromSeconds(10); + + /// + /// Runs a lifecycle operation under , proceeding anyway if the + /// lock cannot be taken in time. + /// + /// + /// Timing out and continuing is the same bargain already strikes + /// with _textExchangeLock: a teardown must never be blocked forever by something + /// wedged. The unsynchronized fallback is exactly the behaviour that shipped before this + /// lock existed, so a timeout is never worse than the status quo — and a reconnect that + /// slips through it is still caught afterwards by . + /// + private void RunLifecycleExclusive(Action operation) + { + var acquired = false; + try + { + acquired = Monitor.TryEnter(_lifecycleLock, LifecycleLockTimeout); + if (!acquired) + { + SafeLog(() => _logger.LogWarning( + "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock within " + + "{TimeoutSeconds}s; proceeding without it.", + Name, + LifecycleLockTimeout.TotalSeconds)); + } + + operation(); + } + finally + { + if (acquired) + { + Monitor.Exit(_lifecycleLock); + } + } + } + /// /// Connects to the device. /// /// /// A caller-issued connect supersedes any automatic reconnect in progress: the loop is - /// cancelled and unwinds without touching the session this call establishes. + /// cancelled and unwinds without touching the session this call establishes. If an attempt + /// is already inside a blocking transport connect, this waits for it to finish rather than + /// running alongside it — see . /// public void Connect() { @@ -567,9 +641,12 @@ public void Connect() /// /// The body of , without the reconnect-supersede step — the reconnect - /// loop calls this so it does not cancel itself. + /// loop calls this so it does not cancel itself. Serialized against + /// so the two can never drive the transport at once. /// - private void ConnectCore() + private void ConnectCore() => RunLifecycleExclusive(ConnectCoreUnsynchronized); + + private void ConnectCoreUnsynchronized() { Status = ConnectionStatus.Connecting; State = DeviceState.Connecting; @@ -668,7 +745,10 @@ public void Disconnect() /// the reconnect loop performs between attempts, which must not look to consumers like the /// session ended on purpose. /// - private void DisconnectCore(ConnectionStatus finalStatus) + private void DisconnectCore(ConnectionStatus finalStatus) => + RunLifecycleExclusive(() => DisconnectCoreUnsynchronized(finalStatus)); + + private void DisconnectCoreUnsynchronized(ConnectionStatus finalStatus) { _isDisconnecting = true; // Best-effort coordination with ExecuteTextCommandAsync — From c2517f3dd8cd3b613812beced7639f35a5eb3e19 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 14:55:55 -0600 Subject: [PATCH 10/16] fix(device): make the lifecycle lock an actual guarantee, not a logged suggestion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RunLifecycleExclusive ran the operation anyway when Monitor.TryEnter timed out, having logged a warning first — which is exactly the double-open the lock was added to prevent: two threads both finding no message consumer, both starting one, two readers on one stream for the rest of the session. A guarantee with a "proceed regardless" branch is not a guarantee. That shape was borrowed from _textExchangeLock, and it was the wrong precedent to borrow: a text-exchange timeout degrades one command, this one corrupts the stream. The two callers also want opposite things from contention, so they now say which they want. Connect fails: it waits LifecycleLockTimeout and throws TimeoutException. Nothing was opened and no state changed, so the cost is a retry — far better than a silently corrupted stream. The reconnect loop treats it as an ordinary attempt failure and backs off. Disconnect waits, unbounded. Teardown is the resource-release path and Dispose depends on it, so it may neither fail nor be skipped, which leaves waiting as the only honest option. It cannot deadlock — nothing holding another lock in this class ever waits on a lifecycle operation (_textExchangeLock is taken inside this one, never the reverse), and Monitor grants re-entry immediately to a thread that already holds it, which is what keeps a handler calling Disconnect from inside a StatusChanged raise working. Every possible holder is itself a bounded lifecycle operation. Semantics documented on Connect, Disconnect and the helper. The timeout is now an internal virtual property so tests can reach the contention path, mirroring SdCardDownloadTimeout. Two regression tests, verified against the pre-fix behaviour: the connect one fails with "Assert.Throws() Failure: No exception was thrown". Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 81 +++++++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 130 ++++++++++++++---- 2 files changed, 188 insertions(+), 23 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 5f6bbabe..b953fb72 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -501,6 +501,78 @@ public void ACallerDisconnect_NeverDrivesTheTransportAlongsideAnInFlightReconnec Assert.False(transport.IsConnected); } + [Fact] + public void AConnectThatCannotTakeTheLifecycleLock_FailsRatherThanRunningAlongside() + { + // The escape hatch: an earlier revision logged a warning on timeout and ran the operation + // anyway, which is precisely the double-open it was added to prevent — two threads both + // finding no message consumer, both starting one, two readers on one stream. Contention + // now has to fail, not proceed. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Contended Connect Device", transport) + { + LifecycleLockTimeoutOverride = TimeSpan.FromMilliseconds(200) + }; + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + var connectGate = transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + transport.SimulateDrop(); + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the reconnect never reached the transport's connect"); + + // The reconnect is parked inside the transport holding the lifecycle lock, so a caller + // connect cannot have it. It must give up rather than open a second connection alongside. + var thrown = Assert.Throws(() => device.Connect()); + Assert.Contains("Nothing was opened", thrown.Message, StringComparison.OrdinalIgnoreCase); + + Assert.False( + transport.SawConcurrentLifecycleCalls, + "a caller's Connect ran inside the transport alongside the reconnect's connect"); + + connectGate.Set(); + WaitUntil(() => !device.IsReconnecting, "the reconnect loop never finished"); + } + + [Fact] + public void ADisconnectThatCannotTakeTheLifecycleLock_WaitsRatherThanFailing() + { + // The other half of the policy. Teardown is the resource-release path and Dispose depends + // on it, so contention must not turn it into an exception — it waits instead, and the + // short connect-side timeout must not leak into it. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Patient Teardown Device", transport) + { + LifecycleLockTimeoutOverride = TimeSpan.FromMilliseconds(200) + }; + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + var connectGate = transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + transport.SimulateDrop(); + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the reconnect never reached the transport's connect"); + + // Held well past the connect-side timeout, so a shared budget would have expired. + ReleaseAfter(connectGate, TimeSpan.FromMilliseconds(600)); + + device.Disconnect(); + + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(transport.SawConcurrentLifecycleCalls); + + WaitUntil(() => !device.IsReconnecting, "the reconnect loop never finished"); + Assert.False(transport.IsConnected); + } + [Fact] public void ANormalConnectDisconnectCycle_ReportsNoLifecycleContention() { @@ -761,6 +833,15 @@ public ScriptedStreamingDevice(string name, IStreamTransport transport) { } + /// + /// Shortens the connect-side lifecycle wait so the contention path is reachable in a test + /// without a ten-second pause. Mirrors how SdCardDownloadTimeout is overridden. + /// + public TimeSpan? LifecycleLockTimeoutOverride { get; init; } + + internal override TimeSpan LifecycleLockTimeout => + LifecycleLockTimeoutOverride ?? base.LifecycleLockTimeout; + /// Number of analog channels the scripted device reports. public int AnalogChannelCount { get; init; } = 4; diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 171ed374..16718d6e 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -581,35 +581,98 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n private readonly object _lifecycleLock = new(); /// - /// How long a lifecycle operation waits for one already in flight. Matches the budget - /// already allows itself on _textExchangeLock. + /// How long waits for a lifecycle operation already in flight before + /// giving up. Overridable for tests, mirroring SdCardDownloadTimeout. /// - private static readonly TimeSpan LifecycleLockTimeout = TimeSpan.FromSeconds(10); + internal virtual TimeSpan LifecycleLockTimeout => TimeSpan.FromSeconds(10); /// - /// Runs a lifecycle operation under , proceeding anyway if the - /// lock cannot be taken in time. + /// What a lifecycle operation does when it cannot have to + /// itself. Running anyway is deliberately not an option: the whole point of the lock is + /// that two threads must never drive the transport at once, and a guarantee with a + /// "proceed regardless" branch is not a guarantee. + /// + private enum LifecycleContention + { + /// + /// Give up and throw rather than run alongside. For : nothing has + /// been opened, so failing costs the caller a retry and nothing else. + /// + Fail, + + /// + /// Wait for the operation in flight, however long it takes. For : + /// a teardown that failed, or that ran concurrently, would be worse than a slow one. + /// + Wait + } + + /// + /// Runs a lifecycle operation under , never alongside another. /// /// - /// Timing out and continuing is the same bargain already strikes - /// with _textExchangeLock: a teardown must never be blocked forever by something - /// wedged. The unsynchronized fallback is exactly the behaviour that shipped before this - /// lock existed, so a timeout is never worse than the status quo — and a reconnect that - /// slips through it is still caught afterwards by . + /// + /// The two callers want opposite things from contention, so neither a shared timeout nor a + /// shared fallback would suit both. + /// + /// + /// fails. It waits and then + /// throws. Opening a second connection alongside one already in flight is exactly what this + /// lock exists to prevent — both threads would find no message consumer, both would build + /// and start one, and the loser's reader would be left running on the same stream, silently + /// corrupting frame boundaries for the rest of the session. A caller who gets a + /// instead has lost nothing: no handle was opened, no state + /// changed, and they can try again. + /// + /// + /// waits. Teardown is the resource-release path and is + /// reached from Dispose, so it may neither fail nor be skipped, which leaves waiting + /// as the only honest option. It is bounded in practice because every possible holder is + /// itself a bounded lifecycle operation, and it cannot deadlock: nothing that holds another + /// lock in this class ever waits on a lifecycle operation (_textExchangeLock is + /// taken inside this one, never the other way round), and + /// grants re-entry immediately to a thread that already holds the + /// lock — which is what keeps a handler calling Disconnect from inside a + /// raise working rather than deadlocking against itself. + /// + /// + /// An earlier revision logged a warning and ran the operation anyway on timeout, copying + /// the bargain _textExchangeLock strikes. That was the wrong precedent to borrow: + /// a text-exchange timeout degrades a single command, whereas this one corrupts the stream + /// for the whole session, which is not something to log and continue into. + /// /// - private void RunLifecycleExclusive(Action operation) + /// + /// Thrown when is and + /// another lifecycle operation held the lock for the whole timeout. + /// + private void RunLifecycleExclusive(Action operation, LifecycleContention onContention) { var acquired = false; try { - acquired = Monitor.TryEnter(_lifecycleLock, LifecycleLockTimeout); - if (!acquired) + if (onContention == LifecycleContention.Wait) { - SafeLog(() => _logger.LogWarning( - "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock within " - + "{TimeoutSeconds}s; proceeding without it.", - Name, - LifecycleLockTimeout.TotalSeconds)); + // The ref overloads are the documented-safe pattern: they set the flag as part + // of taking the lock, so the finally below can never miss a release. + Monitor.Enter(_lifecycleLock, ref acquired); + } + else + { + Monitor.TryEnter(_lifecycleLock, LifecycleLockTimeout, ref acquired); + if (!acquired) + { + SafeLog(() => _logger.LogError( + "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " + + "within {TimeoutSeconds}s; refusing to connect alongside the operation in flight.", + Name, + LifecycleLockTimeout.TotalSeconds)); + + throw new TimeoutException( + $"Device '{Name}' could not start connecting within " + + $"{LifecycleLockTimeout.TotalSeconds:0.#}s because another connect or disconnect " + + "was still in progress. Nothing was opened; retry once it has finished."); + } } operation(); @@ -627,11 +690,23 @@ private void RunLifecycleExclusive(Action operation) /// Connects to the device. /// /// + /// /// A caller-issued connect supersedes any automatic reconnect in progress: the loop is - /// cancelled and unwinds without touching the session this call establishes. If an attempt - /// is already inside a blocking transport connect, this waits for it to finish rather than - /// running alongside it — see . + /// cancelled and unwinds without touching the session this call establishes. + /// + /// + /// Cancelling the loop does not stop it instantly — an attempt already inside a blocking + /// transport connect runs to completion — so this waits for any connect or disconnect in + /// flight rather than running alongside it. If one is still in flight after + /// , this throws rather than opening a connection + /// alongside it: nothing has been opened and no state has changed, so the call is safe to + /// retry. See for why running anyway is not an option. + /// /// + /// + /// Thrown when another connect or disconnect was still in progress after + /// . Nothing was opened. + /// public void Connect() { _callerWantsDisconnected = false; @@ -644,7 +719,8 @@ public void Connect() /// loop calls this so it does not cancel itself. Serialized against /// so the two can never drive the transport at once. /// - private void ConnectCore() => RunLifecycleExclusive(ConnectCoreUnsynchronized); + private void ConnectCore() => + RunLifecycleExclusive(ConnectCoreUnsynchronized, LifecycleContention.Fail); private void ConnectCoreUnsynchronized() { @@ -723,6 +799,14 @@ private void ConnectCoreUnsynchronized() /// sees _isDisconnecting == true via the post-acquisition /// validation and bails out cleanly. Callers wanting non-blocking /// disconnect should drive this off a Task.Run. + /// + /// Also waits for any connect or disconnect already in flight — an automatic reconnect + /// attempt parked inside a blocking transport connect, typically — rather than tearing down + /// alongside it. Unlike this wait is not bounded, because a teardown + /// may neither fail nor be skipped: it is the resource-release path and + /// depends on it. It cannot deadlock, and every possible holder is itself a bounded + /// lifecycle operation — see . This never throws on contention. + /// /// public void Disconnect() { @@ -746,7 +830,7 @@ public void Disconnect() /// session ended on purpose. /// private void DisconnectCore(ConnectionStatus finalStatus) => - RunLifecycleExclusive(() => DisconnectCoreUnsynchronized(finalStatus)); + RunLifecycleExclusive(() => DisconnectCoreUnsynchronized(finalStatus), LifecycleContention.Wait); private void DisconnectCoreUnsynchronized(ConnectionStatus finalStatus) { From 98827403be517698d6bf0179c82a3fde113998ab Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 15:13:02 -0600 Subject: [PATCH 11/16] =?UTF-8?q?fix(device):=20bound=20the=20teardown=20w?= =?UTF-8?q?ait=20=E2=80=94=20a=20wedged=20connect=20must=20not=20hang=20Di?= =?UTF-8?q?spose?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last round's unbounded wait rested on a premise that is false: "every possible holder is itself a bounded lifecycle operation". SerialStreamTransport calls _serialPort.Open() synchronously with no timeout — ConnectionTimeout only sets Read/WriteTimeout, which govern an already-open port — and it is reached while holding the lifecycle lock. This codebase already knows that call can wedge in uncancellable native I/O: SerialDeviceFinder carries a process-wide port quarantine built for precisely that (its comment cites PR #295). So a holder can hang forever, and an unbounded wait inherits the hang, turning Disconnect and therefore Dispose into a permanent block. Teardown now waits TeardownLockTimeout (30s, far more generous than the 10s connect side, because a teardown that gives up early is a teardown that did not happen) and then ABANDONS rather than either racing or hanging — the house answer to uncancellable native I/O here, matching #295's quarantine and #401's bounded SD path. On abandonment the transport is deliberately left to the stuck holder, which is guaranteed to release it: _callerWantsDisconnected is set before the wait, so AbandonIfSuperseded tears down whatever the connect eventually builds. What the abandoned path does do is record the caller's intent in this class's own fields (Status, State, _isInitialized) — safe because they are not the transport, and necessary because otherwise the device keeps reporting itself connected after the caller asked it not to. Dispose still reaches _transport.Dispose() outside the lock, so the handle is released either way. XML docs on Disconnect and the helper corrected: they asserted the bounded-holder claim that this commit disproves. Regression test verified against the pre-fix behaviour, where it fails with "Disconnect blocked for 5037ms behind a wedged connect" — and that 5s was only the test harness's own cap; a real Open() hang has none. Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 54 +++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 139 +++++++++++++----- 2 files changed, 155 insertions(+), 38 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index b953fb72..66bb545b 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -3,6 +3,7 @@ using Daqifi.Core.Communication.Transport; using Daqifi.Core.Device; using Google.Protobuf; +using System.Diagnostics; namespace Daqifi.Core.Tests.Device; @@ -573,6 +574,50 @@ public void ADisconnectThatCannotTakeTheLifecycleLock_WaitsRatherThanFailing() Assert.False(transport.IsConnected); } + [Fact] + public void ADisconnectBehindAWedgedConnect_GivesUpRatherThanHangingForever() + { + // SerialPort.Open is synchronous and uncancellable, and this repo already knows it can + // wedge — SerialDeviceFinder carries a process-wide port quarantine built for exactly that. + // So a holder is NOT guaranteed to be bounded, and a teardown that waits on one without a + // bound inherits the hang, turning Dispose into a permanent block. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Wedged Device", transport) + { + LifecycleLockTimeoutOverride = TimeSpan.FromMilliseconds(200), + TeardownLockTimeoutOverride = TimeSpan.FromMilliseconds(300) + }; + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + // Never released: this connect is wedged, exactly as a hung port would be. + transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + transport.SimulateDrop(); + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the reconnect never reached the transport's connect"); + + var stopwatch = Stopwatch.StartNew(); + device.Disconnect(); + stopwatch.Stop(); + + // It gave up on the stuck holder instead of waiting for a return that may never come. + Assert.True( + stopwatch.Elapsed < TimeSpan.FromSeconds(3), + $"Disconnect blocked for {stopwatch.ElapsedMilliseconds}ms behind a wedged connect; " + + "it should have abandoned the wait"); + + // The caller's intent is still recorded, even though the transport could not be touched. + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(device.IsConnected); + + // And it did not race the wedged connect to get there. + Assert.False(transport.SawConcurrentLifecycleCalls); + } + [Fact] public void ANormalConnectDisconnectCycle_ReportsNoLifecycleContention() { @@ -842,6 +887,15 @@ public ScriptedStreamingDevice(string name, IStreamTransport transport) internal override TimeSpan LifecycleLockTimeout => LifecycleLockTimeoutOverride ?? base.LifecycleLockTimeout; + /// + /// Shortens the teardown-side wait so the abandon path is reachable without a thirty-second + /// pause. + /// + public TimeSpan? TeardownLockTimeoutOverride { get; init; } + + internal override TimeSpan TeardownLockTimeout => + TeardownLockTimeoutOverride ?? base.TeardownLockTimeout; + /// Number of analog channels the scripted device reports. public int AnalogChannelCount { get; init; } = 4; diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 6c3b1590..675b2ae3 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -586,6 +586,14 @@ public DaqifiDevice(string name, IStreamTransport transport, ILogger? logger = n /// internal virtual TimeSpan LifecycleLockTimeout => TimeSpan.FromSeconds(10); + /// + /// How long waits for a lifecycle operation already in flight + /// before abandoning the wait. Far more generous than + /// because a teardown that gives up early is a teardown + /// that did not happen. Overridable for tests, mirroring SdCardDownloadTimeout. + /// + internal virtual TimeSpan TeardownLockTimeout => TimeSpan.FromSeconds(30); + /// /// What a lifecycle operation does when it cannot have to /// itself. Running anyway is deliberately not an option: the whole point of the lock is @@ -601,10 +609,11 @@ private enum LifecycleContention Fail, /// - /// Wait for the operation in flight, however long it takes. For : - /// a teardown that failed, or that ran concurrently, would be worse than a slow one. + /// Give up and report it, leaving the operation in flight alone. For + /// : the caller is told nothing was torn down rather than being + /// blocked forever behind a holder that may never return. /// - Wait + Abandon } /// @@ -625,57 +634,86 @@ private enum LifecycleContention /// changed, and they can try again. /// /// - /// waits. Teardown is the resource-release path and is - /// reached from Dispose, so it may neither fail nor be skipped, which leaves waiting - /// as the only honest option. It is bounded in practice because every possible holder is - /// itself a bounded lifecycle operation, and it cannot deadlock: nothing that holds another - /// lock in this class ever waits on a lifecycle operation (_textExchangeLock is - /// taken inside this one, never the other way round), and - /// grants re-entry immediately to a thread that already holds the - /// lock — which is what keeps a handler calling Disconnect from inside a - /// raise working rather than deadlocking against itself. + /// abandons. It waits — + /// far longer, because a teardown that gives up early is a teardown that did not happen — + /// and then returns false without running, leaving the holder alone. It must not + /// throw (Dispose depends on it) and must not run alongside (that is the corruption + /// above), so reporting that nothing was torn down is what is left. + /// + /// + /// An earlier revision waited here without a bound, on the reasoning that every + /// possible holder is itself a bounded lifecycle operation. That reasoning was wrong: + /// SerialPort.Open is called synchronously with no timeout and can wedge in + /// uncancellable native I/O — a hazard this codebase already knows well enough to have + /// built a process-wide port quarantine around it in + /// . An unbounded wait inherits that hang and + /// turns Dispose into a permanent block. The house answer to uncancellable native + /// I/O here is to abandon the stuck operation rather than wait on it, which is what this + /// now does; the abandoned holder still cleans up after itself, because + /// is set before the wait begins and + /// tears down whatever it eventually built. /// /// - /// An earlier revision logged a warning and ran the operation anyway on timeout, copying - /// the bargain _textExchangeLock strikes. That was the wrong precedent to borrow: - /// a text-exchange timeout degrades a single command, whereas this one corrupts the stream - /// for the whole session, which is not something to log and continue into. + /// Neither policy can deadlock: nothing that holds another lock in this class ever waits on + /// a lifecycle operation (_textExchangeLock is taken inside this one, never + /// the other way round), and grants re-entry immediately to a thread + /// that already holds the lock — which is what keeps a handler calling Disconnect + /// from inside a raise working rather than deadlocking against + /// itself. + /// + /// + /// An even earlier revision logged a warning and ran the operation anyway on timeout, + /// copying the bargain _textExchangeLock strikes. That was the wrong precedent to + /// borrow: a text-exchange timeout degrades a single command, whereas this one corrupts the + /// stream for the whole session, which is not something to log and continue into. /// /// + /// true if the operation ran; false if the wait was abandoned. /// /// Thrown when is and /// another lifecycle operation held the lock for the whole timeout. /// - private void RunLifecycleExclusive(Action operation, LifecycleContention onContention) + private bool RunLifecycleExclusive(Action operation, LifecycleContention onContention) { + var isTeardown = onContention == LifecycleContention.Abandon; + var timeout = isTeardown ? TeardownLockTimeout : LifecycleLockTimeout; var acquired = false; + try { - if (onContention == LifecycleContention.Wait) - { - // The ref overloads are the documented-safe pattern: they set the flag as part - // of taking the lock, so the finally below can never miss a release. - Monitor.Enter(_lifecycleLock, ref acquired); - } - else + // The ref overload is the documented-safe pattern: it sets the flag as part of + // taking the lock, so the finally below can never miss a release. + Monitor.TryEnter(_lifecycleLock, timeout, ref acquired); + + if (!acquired) { - Monitor.TryEnter(_lifecycleLock, LifecycleLockTimeout, ref acquired); - if (!acquired) + if (isTeardown) { SafeLog(() => _logger.LogError( "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " - + "within {TimeoutSeconds}s; refusing to connect alongside the operation in flight.", + + "within {TimeoutSeconds}s, so nothing was torn down. A connect is most likely " + + "wedged in uncancellable native I/O; it will release its own session when it " + + "returns.", Name, - LifecycleLockTimeout.TotalSeconds)); + timeout.TotalSeconds)); - throw new TimeoutException( - $"Device '{Name}' could not start connecting within " - + $"{LifecycleLockTimeout.TotalSeconds:0.#}s because another connect or disconnect " - + "was still in progress. Nothing was opened; retry once it has finished."); + return false; } + + SafeLog(() => _logger.LogError( + "[Lifecycle] Device '{DeviceName}' could not take the connect/disconnect lock " + + "within {TimeoutSeconds}s; refusing to connect alongside the operation in flight.", + Name, + timeout.TotalSeconds)); + + throw new TimeoutException( + $"Device '{Name}' could not start connecting within " + + $"{timeout.TotalSeconds:0.#}s because another connect or disconnect " + + "was still in progress. Nothing was opened; retry once it has finished."); } operation(); + return true; } finally { @@ -802,10 +840,13 @@ private void ConnectCoreUnsynchronized() /// /// Also waits for any connect or disconnect already in flight — an automatic reconnect /// attempt parked inside a blocking transport connect, typically — rather than tearing down - /// alongside it. Unlike this wait is not bounded, because a teardown - /// may neither fail nor be skipped: it is the resource-release path and - /// depends on it. It cannot deadlock, and every possible holder is itself a bounded - /// lifecycle operation — see . This never throws on contention. + /// alongside it. That wait is bounded by , generously, + /// because SerialPort.Open is uncancellable and can wedge indefinitely; waiting on + /// it without a bound would make this method — and therefore — hang + /// forever. If the wait is abandoned, the device still reports + /// , but the transport is deliberately left to + /// the stuck operation, which releases it when it finally returns. A warning-level log + /// records that nothing was torn down. This never throws on contention. /// /// public void Disconnect() @@ -829,8 +870,30 @@ public void Disconnect() /// the reconnect loop performs between attempts, which must not look to consumers like the /// session ended on purpose. /// - private void DisconnectCore(ConnectionStatus finalStatus) => - RunLifecycleExclusive(() => DisconnectCoreUnsynchronized(finalStatus), LifecycleContention.Wait); + private void DisconnectCore(ConnectionStatus finalStatus) + { + if (RunLifecycleExclusive( + () => DisconnectCoreUnsynchronized(finalStatus), + LifecycleContention.Abandon)) + { + return; + } + + // The wait was abandoned: a lifecycle operation is stuck, most likely a + // SerialPort.Open wedged in uncancellable native I/O. Racing it would be the + // stream corruption this lock exists to prevent, so the transport is left to the + // holder — which is guaranteed to release it, because _callerWantsDisconnected was + // set before this and AbandonIfSuperseded tears down whatever the stuck connect + // eventually builds. + // + // What can still be done safely is record the caller's intent at the device level. + // These are this class's own fields, not the transport, so setting them cannot + // corrupt anything the stuck operation is doing — and without them the device would + // keep reporting itself connected after the caller had asked it not to. + State = DeviceState.Disconnected; + _isInitialized = false; + Status = finalStatus; + } private void DisconnectCoreUnsynchronized(ConnectionStatus finalStatus) { From 2daa77ce384a807b3cdced9ebeb81b5ff6d51fea Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 15:27:21 -0600 Subject: [PATCH 12/16] fix(device): honour an abandoned teardown on the caller's connect path too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The abandonment added last round rested on "the stuck holder is guaranteed to release it, because AbandonIfSuperseded tears down whatever it builds". Tracing both holder types shows that guarantee was only ever half true: - Reconnect-loop attempt: ConnectCore returns, AbandonIfSuperseded runs immediately after it, sees _callerWantsDisconnected and tears down. Holds. - Caller's own Connect(): ConnectCore returns straight to Connect(), which returns to the caller. AbandonIfSuperseded belongs to the reconnect loop and is never on this path, so nothing re-checked anything. The wedged connect set Status=Connected and started the reader after Disconnect() had already returned reporting Disconnected. That needs no reconnect loop to reproduce — the lifecycle lock and its abandon path apply to every device — so it is squarely this PR's bug rather than #342's. ConnectCore now re-reads the caller's intent after releasing the lock and tears down what it built if a teardown landed meanwhile. Both entry points share it, and the ordering is not a race: Disconnect sets the flag before contending for the lock and this reads it after releasing, so a teardown that abandoned must have been waiting while the connect still held the lock. Also fixed the severity mismatch Qodo flagged: the Disconnect remarks claimed warning level while the code logged error. Kept error and corrected the docs — an abandoned teardown means a wedged port and a transport not released on the caller's schedule, which is the line an operator needs to survive log filtering. Regression test verified against the pre-fix behaviour, where it fails with "Expected: Disconnected, Actual: Connected". Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 41 ++++++++++++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 49 ++++++++++++++++--- 2 files changed, 84 insertions(+), 6 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 66bb545b..9af378eb 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -618,6 +618,47 @@ public void ADisconnectBehindAWedgedConnect_GivesUpRatherThanHangingForever() Assert.False(transport.SawConcurrentLifecycleCalls); } + [Fact] + public async Task ACallerConnectOutlivingAnAbandonedTeardown_DoesNotBringTheDeviceBackToLife() + { + // Reconnect is deliberately left OFF here: the lifecycle lock and its abandon path apply to + // every device, so this failure needs no reconnect loop at all. A caller's connect wedges + // holding the lock, the caller's own Disconnect abandons its wait and returns reporting + // Disconnected, and then the wedged connect finishes — setting Connected and starting a + // reader behind a caller who was already told the device was down. AbandonIfSuperseded does + // not help here: it belongs to the reconnect loop and never runs on a caller's connect. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Overruled Device", transport) + { + TeardownLockTimeoutOverride = TimeSpan.FromMilliseconds(300) + }; + + var connectGate = transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + var connecting = Task.Run(() => device.Connect()); + + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the caller's connect never reached the transport"); + + // Released well after the teardown gives up, so Disconnect abandons and returns first. + ReleaseAfter(connectGate, TimeSpan.FromMilliseconds(800)); + + device.Disconnect(); + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + + await connecting; + + // The caller asked for teardown; it has to stand whoever was holding the lock. + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(device.IsConnected); + Assert.False(transport.IsConnected); + + // And it was put back down sequentially, never raced. + Assert.False(transport.SawConcurrentLifecycleCalls); + } + [Fact] public void ANormalConnectDisconnectCycle_ReportsNoLifecycleContention() { diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 675b2ae3..e9c80f34 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -757,9 +757,40 @@ public void Connect() /// loop calls this so it does not cancel itself. Serialized against /// so the two can never drive the transport at once. /// - private void ConnectCore() => + /// + /// Ends by honouring a teardown that landed while the connect was in flight. That matters + /// most in the one case where the caller's could not do it itself: + /// a connect wedged in uncancellable native I/O holds the lifecycle lock long enough for + /// the teardown to abandon its wait, so the teardown returns having deliberately left the + /// transport alone — and whatever this connect goes on to build would otherwise be live, + /// with a reader running, after the caller was told the device was disconnected. + /// + /// The check lives here rather than in because both entry points need + /// it. covers the same ground for the reconnect loop, but + /// it is part of the loop and never runs for a caller's own connect. + /// + /// + /// The ordering is not a race: sets the flag before it + /// contends for the lock, and this reads it after releasing it. A teardown that + /// abandoned must therefore have been waiting while this still held the lock, so its write + /// always happens-before this read. + /// + /// + private void ConnectCore() + { RunLifecycleExclusive(ConnectCoreUnsynchronized, LifecycleContention.Fail); + if (_callerWantsDisconnected || _disposed) + { + SafeLog(() => _logger.LogWarning( + "[Lifecycle] Device '{DeviceName}' was disconnected while this connect was in flight; " + + "closing the connection it established.", + Name)); + + DisconnectCore(ConnectionStatus.Disconnected); + } + } + private void ConnectCoreUnsynchronized() { Status = ConnectionStatus.Connecting; @@ -845,8 +876,11 @@ private void ConnectCoreUnsynchronized() /// it without a bound would make this method — and therefore — hang /// forever. If the wait is abandoned, the device still reports /// , but the transport is deliberately left to - /// the stuck operation, which releases it when it finally returns. A warning-level log - /// records that nothing was torn down. This never throws on contention. + /// the stuck operation, which releases it when it finally returns. That outcome is logged + /// at error level: it is a rare safety fallback rather than routine, it means a port + /// is wedged and the transport was not released on the caller's schedule, and it is exactly + /// the line an operator needs to still be there after log filtering. This never throws on + /// contention. /// /// public void Disconnect() @@ -882,9 +916,12 @@ private void DisconnectCore(ConnectionStatus finalStatus) // The wait was abandoned: a lifecycle operation is stuck, most likely a // SerialPort.Open wedged in uncancellable native I/O. Racing it would be the // stream corruption this lock exists to prevent, so the transport is left to the - // holder — which is guaranteed to release it, because _callerWantsDisconnected was - // set before this and AbandonIfSuperseded tears down whatever the stuck connect - // eventually builds. + // holder — which releases it once it unwedges, because _callerWantsDisconnected was + // set before this wait began and every connect path re-reads it after dropping the + // lock: ConnectCore for a caller's own connect, AbandonIfSuperseded for a reconnect + // attempt. Both are needed — AbandonIfSuperseded belongs to the reconnect loop and + // never runs for a caller's connect, which is how a wedged caller connect could + // previously come back to life after Disconnect had already returned. // // What can still be done safely is record the caller's intent at the device level. // These are this class's own fields, not the transport, so setting them cannot From f8a473edaca2442242485ae21862e6b926265f0b Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 15:52:19 -0600 Subject: [PATCH 13/16] fix(device): track a streaming session driven by raw SCPI, so reconnect can restore it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by a physical cable pull on the bench: the link recovered but the stream did not, and Core reported "streaming resumed: False". Root cause is not what it looked like. Nothing cleared IsStreaming during the drop teardown — it had never been set. The example CLI drives the whole session with Send(ScpiMessageProducer.StartStreaming(rate)) and Send(EnableAdcChannels(mask)) rather than the typed API, so Core had no idea a session existed and correctly captured WasStreaming=false. Worse than a missed resume: re-initialization sends StopStreamData, so the reconnect actively killed a stream that was running and then reported success. Send is public and this is an ordinary way to use the library, so the fix is to make Core's view true rather than to call the usage unsupported. DaqifiStreamingDevice.Send now recognizes its own start/stop-streaming and ADC-enable commands whichever API emitted them and updates IsStreaming, StreamingFrequency and per-channel IsEnabled accordingly — the same principle as #409, where analog IsEnabled is resynced from the device's reported mask. Only those commands are interpreted, only after the send succeeded. The global DIO enable is deliberately not tracked: one switch for the whole port carries no per-channel information. Why the suite missed it: every existing test drove the device through the typed API, and the test double overrode Send to record and swallow — so it replaced the very method that now carries session semantics. The double now calls through to production Send, and a new test drives the exact bench shape (raw commands only) and fails against the pre-fix code. Co-Authored-By: Claude Opus 5 --- docs/DEVICE_INTERFACES.md | 8 + .../Device/DeviceReconnectTests.cs | 45 ++++++ .../Device/DaqifiStreamingDevice.cs | 137 ++++++++++++++++++ 3 files changed, 190 insertions(+) diff --git a/docs/DEVICE_INTERFACES.md b/docs/DEVICE_INTERFACES.md index 4d04c64f..548e8834 100644 --- a/docs/DEVICE_INTERFACES.md +++ b/docs/DEVICE_INTERFACES.md @@ -548,6 +548,14 @@ whatever ended the final attempt. - the streaming frequency, and - an active stream, unless `ResumeStreaming` is turned off. +It does not matter whether you established that state through the typed API +(`EnableChannels`, `StartStreaming`) or by sending the SCPI yourself with +`Send(ScpiMessageProducer.StartStreaming(...))` — the device recognizes its own streaming and +ADC-enable commands whichever way they were sent, so a session driven entirely by raw commands is +restored just the same. The one exception is the global DIO enable: it is a single switch for the +whole port rather than a per-channel mask, so sending it directly tells the device nothing about +*which* digital channels you wanted. Use `EnableChannels` for those. + **What does not.** Everything else is the device's own state, and Core does not presume to know what it should be after an outage of unknown length: diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 9af378eb..3323366a 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -1,5 +1,6 @@ using Daqifi.Core.Channel; using Daqifi.Core.Communication.Messages; +using Daqifi.Core.Communication.Producers; using Daqifi.Core.Communication.Transport; using Daqifi.Core.Device; using Google.Protobuf; @@ -154,6 +155,43 @@ public async Task AfterADrop_TheSessionIsRebuiltWithNoConsumerInvolvement() Assert.True(device.Channels.Single(c => c.Type == ChannelType.Digital && c.ChannelNumber == 0).IsEnabled); } + [Fact] + public async Task ASessionDrivenEntirelyByRawCommands_IsStillRestored() + { + // The shape the example CLI actually uses, and the one that failed on a real cable pull: + // channels enabled and streaming started with Send(...) rather than EnableChannels() and + // StartStreaming(). Core had no idea a session existed, so a recovered link reported + // StreamingResumed:false — having just stopped, during re-initialization, the stream that + // was still running. Every other test here drives the device through its typed API, which + // is why the whole suite stayed green while the bench failed. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Raw Driven Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + + device.Send(ScpiMessageProducer.EnableAdcChannels("1")); + device.Send(ScpiMessageProducer.StartStreaming(250)); + + // Core's view tracks the commands regardless of which API sent them. + Assert.True(device.IsStreaming); + Assert.Equal(250, device.StreamingFrequency); + Assert.True(device.Channels.Single(c => c.Type == ChannelType.Analog && c.ChannelNumber == 0).IsEnabled); + + var reconnected = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + + transport.SimulateDrop(); + var result = await reconnected; + + Assert.True(result.StreamingResumed); + Assert.True(device.IsStreaming); + + var sent = device.SentCommands; + Assert.Contains("ENAble:VOLTage:DC 1", sent); + Assert.Contains("SYSTem:StartStreamData 250", sent); + } + [Fact] public async Task ADeviceThatWasNotStreaming_ComesBackIdle() { @@ -979,6 +1017,13 @@ public override void Send(IOutboundMessage message) _sent.Add(stringMessage.Data); } } + + // Still goes through the production Send. That is where a session driven by raw + // commands is tracked (#379), so a double that recorded the command and swallowed it + // would be testing a device that does not exist — which is exactly how a passing + // suite missed the bench failure. The transport's stream accepts writes and discards + // them, so nothing actually leaves. + base.Send(message); } public override Task InitializeAsync( diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index d4666d44..68a67985 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -405,6 +405,143 @@ public void StopStreaming() Send(ScpiMessageProducer.StopStreaming); } + #region Session state tracking for commands sent directly (issue #379) + + /// The command emits. + private const string StartStreamingCommand = "SYSTem:StartStreamData"; + + /// The command emits. + private const string StopStreamingCommand = "SYSTem:StopStreamData"; + + /// The command emits. + private const string EnableAdcChannelsCommand = "ENAble:VOLTage:DC"; + + /// + /// Sends a command, and keeps this device's view of the streaming session in step with it. + /// + /// + /// + /// is public and is a perfectly ordinary way to drive a device — the + /// example CLI does the whole job that way — but a session driven through it used to be + /// completely invisible to this class: stayed false while + /// data poured in, and the enabled-channel set stayed empty. That mattered once reconnect + /// arrived (issue #379). A physical cable pull on the bench recovered the link and then + /// reported StreamingResumed: false, because as far as Core was concerned nothing had + /// ever been streaming — while re-initialization had in fact just stopped the stream that + /// was running. The session looked restored and was not. + /// + /// + /// So the two commands that define a streaming session are recognized here regardless of + /// which API produced them, and the same state is updated that + /// / and + /// would have set. This is the same principle as #409, where + /// analog IsEnabled is resynced from the device's own reported mask: what Core + /// believes about a session has to track what is actually true of it. + /// + /// + /// Only these commands are interpreted, and only after the send itself has succeeded. + /// Everything else passes through untouched. The global DIO enable is deliberately not + /// tracked: it is one switch for the whole port rather than a per-channel mask, so it + /// carries no information about which digital channels a caller wanted. + /// + /// + /// The type of the message data payload. + /// The message to send. + public override void Send(IOutboundMessage message) + { + base.Send(message); + + // Only after the send has actually gone through: a command that threw never reached + // the device and must not move this device's idea of the session. + if (message is IOutboundMessage textCommand) + { + TrackSessionCommand(textCommand.Data); + } + } + + /// + /// Updates the streaming-session view from a command that has just been sent. + /// + private void TrackSessionCommand(string? command) + { + if (string.IsNullOrWhiteSpace(command)) + { + return; + } + + var trimmed = command.Trim(); + + if (trimmed.StartsWith(StopStreamingCommand, StringComparison.OrdinalIgnoreCase)) + { + IsStreaming = false; + return; + } + + if (trimmed.StartsWith(StartStreamingCommand, StringComparison.OrdinalIgnoreCase)) + { + IsStreaming = true; + TrackStreamingFrequency(trimmed.AsSpan(StartStreamingCommand.Length)); + return; + } + + if (trimmed.StartsWith(EnableAdcChannelsCommand, StringComparison.OrdinalIgnoreCase)) + { + TrackAdcEnableMask(trimmed.AsSpan(EnableAdcChannelsCommand.Length)); + } + } + + /// + /// Records the rate carried by a start-streaming command, ignoring one this device would + /// refuse — the device is the authority on whether it accepted it, and a rate Core would + /// reject must not be replayed by a later reconnect. + /// + private void TrackStreamingFrequency(ReadOnlySpan argument) + { + if (!int.TryParse(argument.Trim(), NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency)) + { + return; + } + + if (frequency >= 1 && frequency <= Math.Max(1, Metadata.Capabilities.MaxSamplingRate)) + { + StreamingFrequency = frequency; + } + } + + /// + /// Applies a sent ADC enable bitmask to this device's analog channels, so a caller who + /// enabled channels with a raw command has the same restorable session as one who used + /// . + /// + /// + /// The mask is a set-replace, exactly as the firmware treats it, so every analog channel is + /// assigned from it rather than only the set bits. A device-reported mask still wins on the + /// next status frame (#409) — that is the device's own view, and it outranks what was asked + /// for. + /// + private void TrackAdcEnableMask(ReadOnlySpan argument) + { + if (!uint.TryParse(argument.Trim(), NumberStyles.Integer, CultureInfo.InvariantCulture, out var mask)) + { + return; + } + + WithChannelsLock(() => + { + foreach (var channel in SnapshotChannels()) + { + if (channel.Type != ChannelType.Analog || channel.ChannelNumber > MaxAdcBitmaskChannel) + { + continue; + } + + channel.IsEnabled = (mask & (1u << channel.ChannelNumber)) != 0; + } + }); + } + + #endregion + #region Session restore after an automatic reconnect (issue #379) /// From 5bc5cc1a8954672edba25adf822af635dd0fdf3f Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 16:05:58 -0600 Subject: [PATCH 14/16] fix(device): only track a start-streaming command that carries a usable rate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TrackSessionCommand set IsStreaming=true the moment a command looked like a start, then validated the rate afterwards. A start whose argument is missing, unparseable or out of range therefore left the device believing it was streaming while StreamingFrequency still held a rate from an earlier session — and a reconnect would faithfully restore that rate. Resuming at a number nobody asked for is the silent-wrong-data mode this feature exists to prevent, and it is the same family as the bug the physical cable pull just caught. The bench cannot have covered it: a valid rate never reaches this branch. A malformed start is now not treated as a session start at all. The firmware rejects such a command and does not begin streaming, so the flag would not have described the device anyway; and a spuriously-true IsStreaming also makes the next real StartStreaming() a silent no-op, the stale-flag trap issue #118 and the SD defensive stops already guard against. Existing state is left alone rather than cleared: a device already streaming at a good rate keeps doing so when the firmware rejects a malformed start, so both flags stay true of it. Clearing them would swap one inaccuracy for another. The upshot is that IsStreaming is never true beside an unvalidated rate, so restore has no "streaming at an unknown rate" case to decide about — the state it replays was always really commanded. Ten tests over the malformed shapes (missing, empty, non-numeric, trailing junk, zero, negative, above the sampling ceiling), the running-session case, the silent-no-op consequence, and the end-to-end reconnect. Nine fail against the pre-fix ordering; the tenth documents the leave-alone rule, which pre-fix also satisfied. Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 92 +++++++++++++++++++ .../Device/DaqifiStreamingDevice.cs | 50 +++++++--- 2 files changed, 131 insertions(+), 11 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 3323366a..03c33702 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -192,6 +192,98 @@ public async Task ASessionDrivenEntirelyByRawCommands_IsStillRestored() Assert.Contains("SYSTem:StartStreamData 250", sent); } + [Theory] + [InlineData("SYSTem:StartStreamData")] // no argument at all + [InlineData("SYSTem:StartStreamData ")] // argument present but empty + [InlineData("SYSTem:StartStreamData abc")] // not a number + [InlineData("SYSTem:StartStreamData 100 extra")] // trailing junk + [InlineData("SYSTem:StartStreamData 0")] // below the usable range + [InlineData("SYSTem:StartStreamData -5")] // negative + [InlineData("SYSTem:StartStreamData 99999999")] // beyond the device's sampling rate + public void AStartStreamingCommandWithAnUnusableRate_DoesNotMarkTheDeviceStreaming(string command) + { + // The bench proved the happy path; a valid rate never reaches this branch. Marking the + // device as streaming here would leave StreamingFrequency holding a rate from some earlier + // session, and a reconnect would then restore that rate — resuming at a number nobody asked + // for, which is precisely the silent-wrong-data mode this feature exists to prevent. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Malformed Command Device", transport); + + ConnectAndInitialize(device); + + var frequencyBefore = device.StreamingFrequency; + + device.Send(new ScpiMessage(command)); + + Assert.False(device.IsStreaming); + Assert.Equal(frequencyBefore, device.StreamingFrequency); + } + + [Fact] + public void AnUnusableStartCommand_LeavesARunningSessionExactlyAsItWas() + { + // The firmware rejects the malformed command and keeps streaming at the rate it already + // had, so clearing the flags would swap one inaccuracy for another. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Still Streaming Device", transport); + + ConnectAndInitialize(device); + + device.Send(ScpiMessageProducer.StartStreaming(250)); + Assert.True(device.IsStreaming); + Assert.Equal(250, device.StreamingFrequency); + + device.Send(new ScpiMessage("SYSTem:StartStreamData wat")); + + Assert.True(device.IsStreaming); + Assert.Equal(250, device.StreamingFrequency); + } + + [Fact] + public void AfterAnUnusableStartCommand_AGenuineStartStreamingStillWorks() + { + // The stale-flag trap from issue #118: StartStreaming() returns early while IsStreaming is + // set, so a spuriously-true flag would silently swallow the caller's real request. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Recoverable Device", transport); + + ConnectAndInitialize(device); + + device.Send(new ScpiMessage("SYSTem:StartStreamData not-a-rate")); + device.ClearSentCommands(); + + device.StreamingFrequency = 200; + device.StartStreaming(); + + Assert.True(device.IsStreaming); + Assert.Contains("SYSTem:StartStreamData 200", device.SentCommands); + } + + [Fact] + public async Task AnUnusableStartCommand_LeavesNothingForAReconnectToResume() + { + // The end-to-end consequence: no session was established, so a reconnect must not invent + // one at whatever rate happened to be left in StreamingFrequency. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Nothing To Resume Device", transport); + device.ReconnectOptions = FastPolicy(); + + ConnectAndInitialize(device); + device.Send(new ScpiMessage("SYSTem:StartStreamData oops")); + + var reconnected = WaitFor(h => device.Reconnected += h); + device.ClearSentCommands(); + + transport.SimulateDrop(); + var result = await reconnected; + + Assert.False(result.StreamingResumed); + Assert.False(device.IsStreaming); + Assert.DoesNotContain( + device.SentCommands, + c => c.StartsWith("SYSTem:StartStreamData", StringComparison.Ordinal)); + } + [Fact] public async Task ADeviceThatWasNotStreaming_ComesBackIdle() { diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 68a67985..0bd847eb 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -479,8 +479,7 @@ private void TrackSessionCommand(string? command) if (trimmed.StartsWith(StartStreamingCommand, StringComparison.OrdinalIgnoreCase)) { - IsStreaming = true; - TrackStreamingFrequency(trimmed.AsSpan(StartStreamingCommand.Length)); + TrackStreamingStart(trimmed.AsSpan(StartStreamingCommand.Length)); return; } @@ -491,21 +490,50 @@ private void TrackSessionCommand(string? command) } /// - /// Records the rate carried by a start-streaming command, ignoring one this device would - /// refuse — the device is the authority on whether it accepted it, and a rate Core would - /// reject must not be replayed by a later reconnect. + /// Records a start-streaming command, but only one carrying a rate this device can model. /// - private void TrackStreamingFrequency(ReadOnlySpan argument) + /// + /// + /// A command whose argument is missing, unparseable, or outside the device's sampling range + /// is not treated as the start of a session. Marking one as streaming anyway would be + /// wrong three times over: the firmware rejects such a command and does not start streaming, + /// so the flag would not describe the device; would be left + /// holding a rate from some earlier session, which a reconnect would then faithfully restore + /// — resuming at a rate nobody asked for is the silent-wrong-data failure this whole feature + /// exists to prevent; and a stale makes the next legitimate + /// a silent no-op, which is the same stale-flag trap that + /// issue #118 and the defensive stops scattered through the SD paths already guard against. + /// + /// + /// The existing state is left alone rather than cleared. A device already streaming at a + /// good rate goes on doing exactly that when the firmware rejects a malformed start, so + /// and both remain true of it; + /// forcing them off would swap one inaccuracy for another. + /// + /// + /// Because of this, is never true alongside a rate that was + /// not validated — so session restore has no "streaming at an unknown rate" case to decide + /// what to do about. The state it replays is always one that was really commanded. + /// + /// + private void TrackStreamingStart(ReadOnlySpan argument) { - if (!int.TryParse(argument.Trim(), NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency)) + var rate = argument.Trim(); + + if (!int.TryParse(rate, NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency) + || frequency < 1 + || frequency > Math.Max(1, Metadata.Capabilities.MaxSamplingRate)) { + Trace.WriteLine( + $"[{nameof(TrackStreamingStart)}] Ignoring a start-streaming command with an unusable rate " + + $"('{rate.ToString()}'); the session state is unchanged."); return; } - if (frequency >= 1 && frequency <= Math.Max(1, Metadata.Capabilities.MaxSamplingRate)) - { - StreamingFrequency = frequency; - } + // Frequency first: anything observing IsStreaming must never catch it true next to a + // rate belonging to a previous session. + StreamingFrequency = frequency; + IsStreaming = true; } /// From c6ddd0b6c31df0814ecef71a1f3a81febdda239a Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 16:22:31 -0600 Subject: [PATCH 15/16] fix(device): give a raw-started stream the same session reset as StartStreaming MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 8, plus a deliberate pass over the whole tracking method rather than only what was listed — three findings in one method said its requirements were being discovered a review at a time. Walking each typed method for effects beyond setting a flag: - StartStreaming did six things this path skipped: timestamp anchor reset, tick frequency, gap detector reset, warmup guard arming, its counter, and the decode-failure count. A raw-started stream therefore decoded against the PREVIOUS session's anchor — a unit test pins it at 60 seconds of error, samples stamped with times that never happened, which is the worst outcome this library can produce. Extracted BeginStreamingSession() and called it from both paths so they cannot drift again. - StopStreaming does nothing beyond the flag, so clearing it was already complete. - EnableChannels assigns IsEnabled under the channels lock and derives the mask from it; the mask is already sent by the time tracking runs, so only the assignment is replayed, over every analog channel because the firmware treats the mask as set-replace. Divergences kept, now documented as decisions: the global DIO enable is one switch for the whole port and carries no per-channel information, so none is inferred; argument validation is not replayed because the device has already seen the command and is the authority on it; and a raw start while already streaming records the new rate without re-anchoring, since the typed API cannot express that case so there is no equivalence to preserve. Also fixed the throwing setter: the rate is validated against a single read of MaxSamplingRate and assigned to the backing field, rather than validated against one read and assigned through a setter that takes another. A stress test flipping the ceiling on another thread reproduces the old behaviour as an ArgumentOutOfRangeException escaping a Send whose command had already gone out. Four of the five new tests fail against pre-fix code; the fifth documents the restart-in-place decision, which pre-fix also satisfied. Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 181 ++++++++++++++++++ .../Device/DaqifiStreamingDevice.cs | 92 ++++++++- 2 files changed, 264 insertions(+), 9 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 03c33702..83a77952 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -192,6 +192,176 @@ public async Task ASessionDrivenEntirelyByRawCommands_IsStillRestored() Assert.Contains("SYSTem:StartStreamData 250", sent); } + [Fact] + public void ARawStartedStream_ReAnchorsTimestampsInsteadOfContinuingTheLastSession() + { + // The silent-wrong-data one. Timestamp reconstruction advances a per-session anchor by the + // device-tick delta between frames, so a session that reuses the previous anchor stamps its + // samples with times that never happened. StartStreaming() resets it; a raw start used to + // skip that entirely. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Re-anchoring Device", transport); + + ConnectAndInitialize(device); + + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + var samples = new List(); + channel.SampleReceived += (_, e) => + { + lock (samples) + { + samples.Add(e.Sample); + } + }; + + device.EnableChannel(channel); + + // A first session, ending with the device clock near zero. + device.StartStreaming(); + device.InvokeStreamMessage(AnalogFrame(1_000, 1.0f)); + device.StopStreaming(); + + // A second session started by raw command, with the device clock far ahead — a device that + // has been running a while, or has rebooted its counter. Continuing the old anchor would + // push the timestamp roughly a minute into the future at the 50 MHz default tick rate. + device.Send(ScpiMessageProducer.StartStreaming(100)); + device.InvokeStreamMessage(AnalogFrame(3_000_000_000, 2.0f)); + + lock (samples) + { + Assert.Equal(2, samples.Count); + var elapsed = samples[1].Timestamp - samples[0].Timestamp; + Assert.True( + elapsed < TimeSpan.FromSeconds(10), + $"the second session's first sample landed {elapsed.TotalSeconds:0.#}s after the first " + + "session's, so it was anchored to the previous session rather than to now"); + } + } + + [Fact] + public void ARawStartedStream_StartsWithAFreshGapDetector() + { + // The gap detector compares device-clock deltas between consecutive frames. Carried across + // a session boundary it reports the boundary itself as a huge dropped-sample gap. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Fresh Gap Device", transport); + + ConnectAndInitialize(device); + device.EnableChannel(device.Channels.First(c => c.Type == ChannelType.Analog)); + + var gaps = 0; + device.GapDetected += (_, _) => Interlocked.Increment(ref gaps); + + device.StartStreaming(); + device.InvokeStreamMessage(AnalogFrame(1_000, 1.0f)); + device.InvokeStreamMessage(AnalogFrame(501_000, 1.0f)); + device.StopStreaming(); + + var gapsBefore = Volatile.Read(ref gaps); + + device.Send(ScpiMessageProducer.StartStreaming(100)); + device.InvokeStreamMessage(AnalogFrame(3_000_000_000, 2.0f)); + + Assert.Equal(gapsBefore, Volatile.Read(ref gaps)); + } + + [Fact] + public void ARawStartedStream_StartsItsDecodeFailureCountAtZero() + { + // DecodeFailureCount is documented as describing the current session, and StartStreaming + // resets it. A raw start that inherited the previous session's tally would make a healthy + // stream look broken. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Fresh Count Device", transport); + + ConnectAndInitialize(device); + + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.SampleReceived += (_, _) => throw new InvalidOperationException("decode consumer is broken"); + device.EnableChannel(channel); + + device.StartStreaming(); + device.InvokeStreamMessage(AnalogFrame(1_000, 1.0f)); + device.InvokeStreamMessage(AnalogFrame(2_000, 1.0f)); + Assert.Equal(2, device.DecodeFailureCount); + + device.StopStreaming(); + device.Send(ScpiMessageProducer.StartStreaming(100)); + + Assert.Equal(0, device.DecodeFailureCount); + } + + [Fact] + public void ARawStartWhileAlreadyStreaming_RecordsTheRateWithoutRestartingTheSession() + { + // The typed API cannot express a restart-in-place (StartStreaming returns early), so there + // is no session boundary here and nothing to re-anchor — but the new rate is still real and + // must be what a reconnect would replay. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Restarting Device", transport); + + ConnectAndInitialize(device); + + var channel = (IAnalogChannel)device.Channels.First(c => c.Type == ChannelType.Analog); + channel.SampleReceived += (_, _) => throw new InvalidOperationException("decode consumer is broken"); + device.EnableChannel(channel); + + device.StartStreaming(); + device.InvokeStreamMessage(AnalogFrame(1_000, 1.0f)); + Assert.Equal(1, device.DecodeFailureCount); + + device.Send(ScpiMessageProducer.StartStreaming(400)); + + Assert.True(device.IsStreaming); + Assert.Equal(400, device.StreamingFrequency); + + // Not a new session: the running tally is left alone. + Assert.Equal(1, device.DecodeFailureCount); + } + + [Fact] + public async Task TrackingARate_NeverThrowsOutOfSend_WhileCapabilitiesChangeUnderneath() + { + // MaxSamplingRate is a mutable public property, and a status message can update it at any + // moment on the consumer thread. Validating a rate against one read and then assigning it + // through the public setter — which reads it again and throws on a value it dislikes — + // leaves a window where tracking turns a command that has ALREADY reached the device into + // an exception at the caller's Send(). This drives that window directly. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Shifting Ceiling Device", transport); + + ConnectAndInitialize(device); + + using var stop = new CancellationTokenSource(TimeSpan.FromSeconds(3)); + Exception? escaped = null; + + var churn = Task.Run(() => + { + // Flip the ceiling between "accepts 900" and "accepts almost nothing" as fast as + // possible, so it changes between the two reads. + while (!stop.IsCancellationRequested) + { + device.Metadata.Capabilities.MaxSamplingRate = 1000; + device.Metadata.Capabilities.MaxSamplingRate = 1; + } + }); + + try + { + for (var i = 0; i < 20_000 && escaped == null; i++) + { + escaped = Record.Exception(() => device.Send(ScpiMessageProducer.StartStreaming(900))); + } + } + finally + { + stop.Cancel(); + await churn; + } + + Assert.Null(escaped); + } + [Theory] [InlineData("SYSTem:StartStreamData")] // no argument at all [InlineData("SYSTem:StartStreamData ")] // argument present but empty @@ -999,6 +1169,14 @@ private static void ReleaseAfter(ManualResetEventSlim gate, TimeSpan delay) }); } + /// Builds a single-channel analog frame carrying a device-clock timestamp. + private static DaqifiOutMessage AnalogFrame(uint deviceTimestamp, float value) + { + var frame = new DaqifiOutMessage { MsgTimeStamp = deviceTimestamp }; + frame.AnalogInDataFloat.Add(value); + return frame; + } + private static void WaitUntil(Func condition, string because) { var deadline = DateTime.UtcNow + EventTimeout; @@ -1100,6 +1278,9 @@ public void ClearSentCommands() } } + /// Feeds a streaming frame straight into the decode path. + public void InvokeStreamMessage(DaqifiOutMessage message) => OnStreamMessageReceived(message); + public override void Send(IOutboundMessage message) { if (message is IOutboundMessage stringMessage) diff --git a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs index 0bd847eb..6bf64ba1 100644 --- a/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiStreamingDevice.cs @@ -366,6 +366,25 @@ public void StartStreaming() if (IsStreaming) return; + BeginStreamingSession(); + + IsStreaming = true; + Send(ScpiMessageProducer.StartStreaming(StreamingFrequency)); + } + + /// + /// Resets everything that is scoped to one streaming session, so the frames that follow are + /// decoded against this session rather than the last one. + /// + /// + /// Shared by and by the tracking of a start-streaming command + /// sent directly through . It is one method precisely so the two cannot + /// drift: a raw-started stream that skipped this would reconstruct timestamps from the + /// previous session's anchor and re-use its gap detector, producing samples stamped with + /// times that never happened — silently. + /// + private void BeginStreamingSession() + { // Re-anchor per-session timestamp reconstruction: the first frame of this session // anchors to the current host time, and subsequent frames advance by the device-tick // delta. Apply the device-reported tick frequency (falls back to the 50 MHz default @@ -383,9 +402,6 @@ public void StartStreaming() _awaitingFirstFullAnalogFrame = CountEnabledAnalogChannels(SnapshotChannels()) > 0; _suppressedWarmupFrameCount = 0; Interlocked.Exchange(ref _decodeFailureCount, 0); - - IsStreaming = true; - Send(ScpiMessageProducer.StartStreaming(StreamingFrequency)); } /// @@ -440,9 +456,45 @@ public void StopStreaming() /// /// /// Only these commands are interpreted, and only after the send itself has succeeded. - /// Everything else passes through untouched. The global DIO enable is deliberately not - /// tracked: it is one switch for the whole port rather than a per-channel mask, so it - /// carries no information about which digital channels a caller wanted. + /// Everything else passes through untouched. + /// + /// + /// What equivalence with the typed API covers. Each typed method was walked and every + /// effect beyond setting a flag accounted for, so this is a stated scope rather than a + /// hopeful one: + /// + /// + /// + /// — the whole per-session reset (timestamp anchor and tick + /// frequency, gap detector, warmup guard and its counter, decode-failure count) is shared + /// through and runs here too. Skipping it was a real + /// defect: frames decoded against a previous session's anchor carry times that never + /// happened. + /// + /// + /// — does nothing beyond clearing the flag, so clearing it here + /// is complete. + /// + /// + /// — assigns under the + /// channels lock and derives the outbound mask from it. The mask has already been sent by + /// the time this runs, so only the assignment is replayed, and it is applied to every + /// analog channel because the firmware treats the mask as a set-replace. + /// + /// + /// + /// Deliberately outside it. The global DIO enable is one switch for the whole port + /// rather than a per-channel mask, so a raw DIO:PORt:ENAble carries no information + /// about which digital channels were wanted and none is inferred. Argument validation + /// is not replayed either: by the time a command is seen here it has already gone to the + /// device, and the device is the authority on whether it accepted it. + /// + /// + /// Running after the send is safe rather than merely convenient. A string command is handed + /// to the background producer, so it has not reached the wire when this runs, and the device + /// cannot answer a command it has not received; on the producer-less path that writes + /// synchronously there is no message consumer decoding frames at all. Either way no frame + /// can be decoded between the send and the state it should be decoded against. /// /// /// The type of the message data payload. @@ -520,9 +572,17 @@ private void TrackStreamingStart(ReadOnlySpan argument) { var rate = argument.Trim(); + // One read of the ceiling, used for both the check and the assignment below. The public + // StreamingFrequency setter re-reads it and throws when it does not like the value, and + // MaxSamplingRate is a mutable public property — so validating against one read and + // then assigning through a setter that takes another would let a concurrent + // capabilities update throw out of a Send whose command has already gone to the device. + // Tracking a command must never be able to fail the send that carried it. + var maxSamplingRate = Math.Max(1, Metadata.Capabilities.MaxSamplingRate); + if (!int.TryParse(rate, NumberStyles.Integer, CultureInfo.InvariantCulture, out var frequency) || frequency < 1 - || frequency > Math.Max(1, Metadata.Capabilities.MaxSamplingRate)) + || frequency > maxSamplingRate) { Trace.WriteLine( $"[{nameof(TrackStreamingStart)}] Ignoring a start-streaming command with an unusable rate " @@ -531,8 +591,22 @@ private void TrackStreamingStart(ReadOnlySpan argument) } // Frequency first: anything observing IsStreaming must never catch it true next to a - // rate belonging to a previous session. - StreamingFrequency = frequency; + // rate belonging to a previous session. Assigned to the backing field, not through the + // validating setter, for the reason above — the value has just been validated against + // the same rule. + _streamingFrequency = frequency; + + if (IsStreaming) + { + // A restart while already streaming. The typed API cannot even express this + // (StartStreaming returns early), so there is no session boundary to re-anchor at + // and no equivalence to preserve; recording the new rate is all that is warranted. + return; + } + + // A session is beginning, so it gets exactly the preparation StartStreaming would have + // given it. Ordering matches too: the state is ready before the flag flips. + BeginStreamingSession(); IsStreaming = true; } From f65a2cef93132609eb0885a3f9670dfac89aa04a Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Fri, 31 Jul 2026 18:17:28 -0600 Subject: [PATCH 16/16] fix(device): a cancelled DisconnectAsync must still tear the connection down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A defect that exists only in the merged result, and the mirror image of the one fixed in round 5 — both are Disconnect() returning success without disconnecting. #341 defines what the token means for DisconnectAsync: it shortens the courtesy wait for an in-flight command exchange, never aborts the disconnect, and never surfaces as an OperationCanceledException. #379's lifecycle lock added a second, later wait that contract never covered, and the token was handed to it — where a cancellation was then classified as "abandon teardown", skipping the message-pump stop and the transport close while still reporting Disconnected. My own comment above that catch asserted a teardown is never abandoned by its own token; the code did exactly that. Worse than a contention edge case: SemaphoreSlim throws for an already-cancelled token even when the semaphore is free, so this fired on EVERY cancelled disconnect, leaving live I/O behind a device reporting itself disconnected. The teardown path now acquires with CancellationToken.None. The wait stays bounded by TeardownLockTimeout, which is what protects against a genuinely wedged holder, so round 4's decision is untouched; and the token still reaches AcquireTextExchangeLockForTeardownAsync inside the teardown, where it means what #341 says it means. The connect path is the opposite case and keeps honouring the token, because ConnectAsync is documented to be abandonable and to throw. Audited the other teardown exit paths for the same "token cancelled" versus "lock unavailable" confusion: the sync acquire passes no token, StopMessagePumps runs under CancellationToken.None, the transport close takes none, and the text-exchange acquire already catches and proceeds. This was the only site. Two tests; the first fails against the pre-fix code with the transport still open after a cancelled disconnect. The second documents that a cancelled teardown still waits out a connect in flight rather than racing it — it passes pre-fix, because round 5's caller-side guard cleans up behind the skipped teardown. Co-Authored-By: Claude Opus 5 --- .../Device/DeviceReconnectTests.cs | 66 +++++++++++++++++++ src/Daqifi.Core/Device/DaqifiDevice.cs | 29 +++++--- 2 files changed, 86 insertions(+), 9 deletions(-) diff --git a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs index 83a77952..06e94c9d 100644 --- a/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs +++ b/src/Daqifi.Core.Tests/Device/DeviceReconnectTests.cs @@ -959,6 +959,72 @@ public async Task ACallerConnectOutlivingAnAbandonedTeardown_DoesNotBringTheDevi Assert.False(transport.SawConcurrentLifecycleCalls); } + [Fact] + public async Task ACancelledDisconnectAsync_StillTearsTheConnectionDown() + { + // A defect that exists only in the merged result. #341 defines the token for + // DisconnectAsync as shortening the wait for an in-flight command exchange — the disconnect + // itself always completes and never throws. #379's lifecycle lock added a second wait that + // the token was then handed to, and a cancelled token there was treated as "abandon + // teardown". SemaphoreSlim throws for an already-cancelled token even when it is free, so + // this hit EVERY cancelled disconnect, not just a contended one: the device reported + // Disconnected with the transport still open and the message pumps still running. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Cancelled Teardown Device", transport); + + ConnectAndInitialize(device); + Assert.True(transport.IsConnected); + + using var alreadyCancelled = new CancellationTokenSource(); + await alreadyCancelled.CancelAsync(); + + // #341's contract: this must not throw, however cancelled the token is. + await device.DisconnectAsync(alreadyCancelled.Token); + + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(device.IsConnected); + + // The wire-level evidence: teardown really ran rather than being skipped. + Assert.False(transport.IsConnected); + + // And the pumps really stopped — a device whose producer was left running would still + // accept a send instead of refusing it. + Assert.Throws( + () => device.Send(ScpiMessageProducer.StopStreaming)); + } + + [Fact] + public async Task ACancelledDisconnectAsync_StillWaitsOutAConnectInFlight() + { + // The token must not shortcut the lifecycle wait either: tearing down alongside a connect + // is the corruption the lock exists to prevent, so a cancelled teardown still waits for the + // holder — bounded by TeardownLockTimeout, which is what covers a genuinely wedged one. + using var transport = new ScriptedReconnectTransport(); + using var device = new ScriptedStreamingDevice("Cancelled Contended Device", transport); + + ConnectAndInitialize(device); + + var connectGate = transport.BlockConnects(); + transport.ConnectEntered.Reset(); + + var connecting = Task.Run(() => device.Connect()); + Assert.True( + transport.ConnectEntered.Wait(EventTimeout), + "the connect never reached the transport"); + + ReleaseAfter(connectGate, TimeSpan.FromMilliseconds(200)); + + using var alreadyCancelled = new CancellationTokenSource(); + await alreadyCancelled.CancelAsync(); + + await device.DisconnectAsync(alreadyCancelled.Token); + await connecting; + + Assert.Equal(ConnectionStatus.Disconnected, device.Status); + Assert.False(transport.IsConnected); + Assert.False(transport.SawConcurrentLifecycleCalls); + } + [Fact] public void ANormalConnectDisconnectCycle_ReportsNoLifecycleContention() { diff --git a/src/Daqifi.Core/Device/DaqifiDevice.cs b/src/Daqifi.Core/Device/DaqifiDevice.cs index 30c1bef3..00f25f6e 100644 --- a/src/Daqifi.Core/Device/DaqifiDevice.cs +++ b/src/Daqifi.Core/Device/DaqifiDevice.cs @@ -836,28 +836,39 @@ private async Task RunLifecycleExclusiveAsync( } var timeout = ContentionWait(onContention); + var isTeardown = onContention == LifecycleContention.Abandon; var acquired = false; + // A teardown's token is NOT allowed to govern this wait. Issue #341 defines what the + // token means for DisconnectAsync — it shortens the courtesy wait for an in-flight + // command exchange, never aborts the disconnect, and never surfaces as an + // OperationCanceledException — and this lock is a second, later wait that contract + // never covered. Passing the token here made a cancelled DisconnectAsync skip teardown + // altogether and report Disconnected with the transport still open and the message + // pumps still running; and because SemaphoreSlim throws for an already-cancelled token + // even when the semaphore is free, that happened on every cancelled disconnect, not + // just a contended one. The wait stays bounded by TeardownLockTimeout, which is what + // protects against a genuinely wedged holder (issue #379). The token still reaches + // AcquireTextExchangeLockForTeardownAsync inside the teardown, where it means what + // #341 says it means. + // + // The connect path is the opposite case and does honour the token: ConnectAsync is + // documented to be abandonable and to throw OperationCanceledException. + var acquireToken = isTeardown ? CancellationToken.None : cancellationToken; + try { - acquired = await _lifecycleLock.WaitAsync(timeout, cancellationToken).ConfigureAwait(false); + acquired = await _lifecycleLock.WaitAsync(timeout, acquireToken).ConfigureAwait(false); } catch (ObjectDisposedException) { await operation().ConfigureAwait(false); return true; } - catch (OperationCanceledException) when (onContention == LifecycleContention.Abandon) - { - // A teardown is never abandoned by its own token — the caller asked to stop - // waiting for the in-flight operation, not to stop disconnecting. - LogAbandonedTeardown(timeout); - return false; - } if (!acquired) { - if (onContention == LifecycleContention.Abandon) + if (isTeardown) { LogAbandonedTeardown(timeout); return false;