fix(transport): reject an invalid ConnectionTimeout instead of retrying it

[tylerkron]

What was wrong

If you set ConnectionRetryOptions.ConnectionTimeout to zero or a negative value, connecting
didn't tell you that. It failed with a raw platform exception naming a property you never
touched — ArgumentOutOfRangeException ... (Parameter 'WriteTimeout') on serial, a bare
SocketException: Invalid argument on TCP, four different messages for the one mistake — and
then Core retried the misconfiguration, sitting through the full exponential backoff against
a device that was plugged in and perfectly healthy. Bench-measured at 7.08 s with the default
4-attempt policy on a working Nq1; a Resilient policy would burn far longer. None of the four
messages mentioned ConnectionTimeout.

How it was fixed

ConnectionRetryOptions now validates each value where it is set, so the failure is an
ArgumentOutOfRangeException naming ConnectionTimeout, thrown before any dialling happens.
Because it throws before the attempt loop is ever entered, the pointless backoff disappears for
free — no change to ConnectRetryExecutor was needed.

The other four properties are guarded in the same pass, since a negative MaxDelay makes
CalculateDelay return a negative span and a NaN BackoffMultiplier poisons the whole curve.
The shape is copied verbatim from the guards ReconnectOptions already carries, so the two
sibling policy classes still read the same way.

Two judgement calls worth pushing back on if you disagree:

• BackoffMultiplier must now be >= 1.0, matching ReconnectOptions. A value below 1 would
  shrink the delays, which isn't backoff — but it wasn't previously rejected.
• ConnectionTimeout also has an upper bound of int.MaxValue ms (~24.8 days), because both
  transports narrow it to a millisecond int; a longer span would wrap round to a negative
  timeout and land right back in the platform error this PR exists to remove.

Verification

Full suite green on net9.0 and net10.0: 4084 Core tests (3 skipped, platform-gated) and 217 MCP
tests, zero build warnings. Eleven new tests cover each rejected value, each accepted boundary
(InitialDelay = 0 still means "retry immediately", ConnectionTimeout = int.MaxValue ms is
accepted), and that the NoRetry/Fast/Resilient presets still satisfy their own guards.

closes #681

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Reject invalid connection retry options before dialing

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Validates retry policy values during assignment, preventing transport-level timeout errors and
 retries.
• Preserves immediate retries while safely handling exponential backoff overflow.
• Adds invalid-value, boundary, unchanged-state, and preset regression coverage.

Diagram

graph TD
  Caller["Caller config"] --> Options["Retry options"] --> Valid{"Values valid?"}
  Valid -->|No| Error["Argument exception"]
  Valid -->|Yes| Executor["Retry executor"] --> Transport["Serial or TCP"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Validate in retry executor

• ➕ Centralizes checks immediately before connection attempts
• ➕ Can validate policies mutated through future configuration paths
• ➖ Defers errors until ConnectAsync instead of the offending assignment
• ➖ Requires the executor to own transport-specific timeout limits

2. Mark platform errors non-retryable

• ➕ Stops backoff after the first platform failure
• ➕ Could classify other permanent connection failures
• ➖ Still exposes inconsistent serial and socket exceptions
• ➖ Relies on fragile platform-specific exception classification

Recommendation: Keep setter-level validation. It fails closest to the caller's mistake, prevents every transport from receiving invalid values, mirrors ReconnectOptions, and avoids changing retry failure classification. Executor validation could be added only as defense-in-depth if policies later become mutable through unguarded paths.

Files changed (2) +263 / -6

Bug fix (1) +121 / -6

ConnectionRetryOptions.csValidate connection retry policies at assignment +121/-6

Validate connection retry policies at assignment

• Replaces auto-properties with guarded setters so invalid retry policies fail before dialing, including connection timeouts outside the transports' millisecond-int range. Documents the constraints and preserves zero-delay behavior even when exponential factors overflow.

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

Tests (1) +142 / -0

ConnectionRetryOptionsTests.csCover retry option validation and accepted boundaries +142/-0

Cover retry option validation and accepted boundaries

• Adds tests for rejected attempts, delays, multipliers, and connection timeouts, including property-specific exceptions and unchanged values after failed assignments. Also verifies zero-delay and maximum-timeout boundaries plus compatibility of built-in policies.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Sub-millisecond timeout still retried ✓ Resolved 🐞 Bug ≡ Correctness

Description

ConnectionTimeout accepts any positive TimeSpan, but the serial transport truncates it to
integer milliseconds, so values from one tick through just under 1 ms become 0 and fail while
assigning SerialPort.ReadTimeout/WriteTimeout. Because that exception occurs inside
ConnectRetryExecutor, the invalid configuration is retried instead of being rejected by the option
setter.

Code

src/Daqifi.Core/Communication/Transport/ConnectionRetryOptions.cs[R120-123]

+            if (value <= TimeSpan.Zero)
+            {
+                throw new ArgumentOutOfRangeException(
+                    nameof(ConnectionTimeout), value, "The connection timeout must be greater than zero.");

Relevance

●●● Strong

Clear deterministic validation gap; timeout truncates to zero and causes retryable platform
failures. Recent transport timeout fixes were accepted.

PR-#237

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The setter only rejects value <= TimeSpan.Zero, while the serial connect attempt casts
TotalMilliseconds to int and immediately assigns the result to both platform timeout properties.
The executor catches ordinary exceptions from that attempt and continues whenever attempts remain,
proving that a value such as TimeSpan.FromTicks(1) is converted to zero and follows the unwanted
retry path.

src/Daqifi.Core/Communication/Transport/ConnectionRetryOptions.cs[115-133]
src/Daqifi.Core/Communication/Transport/SerialStreamTransport.cs[304-315]
src/Daqifi.Core/Communication/Transport/ConnectRetryExecutor.cs[86-100]

Agent prompt

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

## Issue description
Positive sub-millisecond `ConnectionTimeout` values pass validation but truncate to zero in the transports, causing a platform exception and retries.

## Issue Context
Both transports narrow the timeout to integer milliseconds; the option must reject values that cannot remain positive after that conversion, or conversion must use a validated nonzero rounding policy.

## Fix Focus Areas
- src/Daqifi.Core/Communication/Transport/ConnectionRetryOptions.cs[115-133]
- src/Daqifi.Core.Tests/Communication/Transport/ConnectionRetryOptionsTests.cs[238-295]

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

---

Context sources

Review mode: ⚖️ Balanced: This is a localized validation change, but it alters runtime connection configuration behavior and transport-boundary timeout semantics, warranting a careful single-pass review.

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit efeab0e