fix: surface TCP connect timeout as TimeoutException

[tylerkron]

Summary

TcpStreamTransport.ConnectAsync(ConnectionRetryOptions?) enforces its connection timeout with a CancellationTokenSource + Task.WaitAsync(cts.Token), so an unreachable device surfaced as TaskCanceledException — semantically misleading, since nothing was canceled by the caller; the connect attempt timed out. This read like an app bug to consumers and caused daqifi/daqifi-desktop#517, where Sentry auto-filed the stack trace as an error for what is an environmental condition (device powered off, stale discovery entry, wrong subnet).

No caller-supplied cancellation token is involved in this path, so the cancellation can only ever mean a timeout. This PR catches it at the WaitAsync call (when (cts.IsCancellationRequested)) and rethrows as TimeoutException with the endpoint and configured timeout in the message, e.g.:

TCP connect to 192.0.2.1:9760 timed out after 5s.

The retry loop's behavior is unchanged — the translated exception flows through the existing generic catch, so retries, StatusChanged events, and the final rethrow all carry the TimeoutException.

Behavioral API note

Consumers catching TaskCanceledException/OperationCanceledException around ConnectAsync to detect connect timeouts must now catch TimeoutException — worth a release-notes mention. daqifi-desktop's connect-failure classification (daqifi/daqifi-desktop#595) handles the old exception type and the new one falls into the same handling tier on its side once it picks this up... (it currently special-cases OperationCanceledException; TimeoutException will need the same warning-level classification when the package is bumped — tracked on the desktop side.)

Tests

New TcpStreamTransport_ConnectAsync_WhenConnectTimesOut_ThrowsTimeoutException: connects to RFC 5737 TEST-NET-1 (192.0.2.1, unroutable — same address the existing local-interface bind test uses) with a 250ms timeout, asserts TimeoutException naming the endpoint, and asserts the StatusChanged event carries the translated exception. Full suite green: 1008 passed on net9.0 and net10.0.

🤖 Generated with Claude Code

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

1. Flaky timeout unit test ✓ Resolved 🐞 Bug ☼ Reliability

Description

TcpStreamTransport_ConnectAsync_WhenConnectTimesOut_ThrowsTimeoutException assumes connecting to
192.0.2.1 will always hang and hit the WaitAsync timeout, but the underlying ConnectAsync task can
also fail immediately (e.g., SocketException), bypassing the timeout translation and making the test
environment-dependent.

Code

src/Daqifi.Core.Tests/Communication/Transport/TcpStreamTransportTests.cs[R199-217]

+    public async Task TcpStreamTransport_ConnectAsync_WhenConnectTimesOut_ThrowsTimeoutException()
+    {
+        // Arrange - 192.0.2.1 is in TEST-NET-1 (RFC 5737) and unroutable, so the TCP connect
+        // never completes and must exhaust the configured connection timeout. The timeout used
+        // to surface as TaskCanceledException (the WaitAsync cancellation token), which read
+        // like an app bug to consumers (daqifi-desktop#517) — it must be a TimeoutException.
+        using var transport = new TcpStreamTransport(IPAddress.Parse("192.0.2.1"), 9760);
+        TransportStatusEventArgs? capturedArgs = null;
+        transport.StatusChanged += (sender, args) => capturedArgs = args;
+        var options = new ConnectionRetryOptions
+        {
+            Enabled = false,
+            MaxAttempts = 1,
+            ConnectionTimeout = TimeSpan.FromMilliseconds(250)
+        };
+
+        // Act & Assert
+        var ex = await Assert.ThrowsAsync<TimeoutException>(() => transport.ConnectAsync(options));
+        Assert.Contains("192.0.2.1:9760", ex.Message);

Evidence

The test only accepts TimeoutException from a real network connect to 192.0.2.1. The production code
only translates OperationCanceledException from WaitAsync(cts.Token); if the connect task faults
before the CTS fires, the translation path is not taken and a different exception will propagate,
causing nondeterministic failures.

src/Daqifi.Core.Tests/Communication/Transport/TcpStreamTransportTests.cs[198-224]
src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[163-204]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test depends on a real TCP connect to `192.0.2.1` behaving as a “hang until timeout”. In some environments the connect can fail fast (routing/firewall/OS behavior), which means `WaitAsync(cts.Token)` never throws `OperationCanceledException` and the code won’t translate to `TimeoutException`, making this a flaky unit test.

## Issue Context
The timeout translation only occurs when `WaitAsync` is canceled by the CTS; any immediate connect failure will propagate as-is.

## Fix Focus Areas
- Create a deterministic timeout test seam (e.g., inject a connect delegate / TcpClient factory or an internal abstraction) so the test can force a never-completing connect task and reliably assert the `TimeoutException` translation.
- Update the unit test to use that seam rather than relying on external networking.

### Fix Focus Areas (code references)
- src/Daqifi.Core.Tests/Communication/Transport/TcpStreamTransportTests.cs[198-224]
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[154-204]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Timeout drops inner exception ✓ Resolved 🐞 Bug ◔ Observability

Description

TcpStreamTransport.ConnectAsync throws a new TimeoutException without setting the caught
OperationCanceledException as InnerException, losing useful diagnostic context for debugging and
error reporting.

Code

src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[R173-180]

+                catch (OperationCanceledException) when (cts.IsCancellationRequested)
+                {
+                    // The only cancellation source here is the timeout token, so surface the
+                    // failure as what it actually is — a connect timeout — rather than a
+                    // misleading TaskCanceledException (daqifi-desktop#517).
+                    throw new TimeoutException(
+                        $"TCP connect to {Hostname ?? _endPoint.Address.ToString()}:{_endPoint.Port} " +
+                        $"timed out after {options.ConnectionTimeout.TotalSeconds:0.###}s.");

Evidence

The new translation throws TimeoutException with only a message, dropping the caught
OperationCanceledException. Elsewhere in the codebase, timeouts are sometimes rethrown as
TimeoutException with an inner exception to preserve diagnostic context.

src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[169-181]
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[1114-1166]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
When the connect timeout fires, the code catches `OperationCanceledException` and throws a new `TimeoutException(message)` without preserving the original exception. This discards stack/context that can help diagnose how/where the timeout was enforced.

## Issue Context
Other timeout translations in the repo sometimes preserve an inner exception for diagnostics.

## Fix Focus Areas
- Preserve the caught exception by using `new TimeoutException(message, ex)`.
- (Optional) Update/extend tests to assert `InnerException` is present and of type `OperationCanceledException`.

### Fix Focus Areas (code references)
- src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs[169-181]
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[1114-1166]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[qodo-code-review]

PR Summary by Qodo

Surface TCP connect timeouts as TimeoutException
🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Walkthroughs

Description

• Translate internal connect timeouts from OperationCanceled/TaskCanceled to TimeoutException.
• Include endpoint + timeout details in the thrown exception message.
• Add regression test ensuring StatusChanged also carries the translated exception.

Diagram

graph TD
  A["Caller"] --> B["TcpStreamTransport.ConnectAsync"] --> C["TcpClient.ConnectAsync"] --> D["WaitAsync + timeout"]
  C --> E{{"Remote endpoint"}}
  D --> F["NetworkStream open"] --> G["StatusChanged event"] --> A
  D --> H["TimeoutException"] --> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use Task.WaitAsync(TimeSpan) directly

• ➕ Naturally throws TimeoutException without relying on CancellationTokenSource semantics
• ➕ Slightly simpler control flow (no catch filter on IsCancellationRequested)
• ➖ Less control over exception message content without wrapping/rethrowing
• ➖ Still requires care to avoid masking genuine caller cancellation if a token is introduced later

2. Add optional caller CancellationToken to ConnectAsync

• ➕ Clearly distinguishes user cancellation from timeout vs other failures
• ➕ More flexible for consumers that need to abort connection attempts
• ➖ Public API change (signature + interface changes) and wider ripple across transports/callers
• ➖ Requires decisions about precedence/interaction with retry delays and timeout

Recommendation: Current approach is appropriate: it preserves the existing retry/status behavior while fixing the semantic mismatch by surfacing timeouts as TimeoutException (with helpful endpoint/timeout context). If the codebase targets a framework where Task.WaitAsync(TimeSpan) is available and preferred, consider switching to that to avoid cancellation-token-based timeout signaling, but the current implementation is already correct and well-tested.

File Changes

Bug fix (1)

TcpStreamTransport.cs Translate connect-timeout cancellation into TimeoutException +20/-1

Translate connect-timeout cancellation into TimeoutException

• Wraps the internal connectTask.WaitAsync(timeoutToken) to catch timeout-driven OperationCanceledException and rethrow a TimeoutException that includes host/IP, port, and timeout seconds. Adds XML doc comments documenting TimeoutException on both ConnectAsync overloads; retry loop and StatusChanged behavior remain the same but now propagate TimeoutException instead of TaskCanceled/OperationCanceled for timeouts.

src/Daqifi.Core/Communication/Transport/TcpStreamTransport.cs

---

Tests (1)

TcpStreamTransportTests.cs Add regression test for connect timeout exception type +28/-0

Add regression test for connect timeout exception type

• Adds a test that connects to an unroutable RFC 5737 address with a short ConnectionTimeout and asserts ConnectAsync throws TimeoutException containing the endpoint. Also verifies the StatusChanged event reports the same translated TimeoutException and the transport remains disconnected.

src/Daqifi.Core.Tests/Communication/Transport/TcpStreamTransportTests.cs

---

[tylerkron]

Re Qodo finding 2 (timeout drops inner exception): fixed in 2c9b04b — the translated TimeoutException now carries the caught OperationCanceledException as InnerException, and the regression test asserts it.