Stop the daemon reconnect loop spinning against a healthy connection

[realtonyyoung]

Closes #374 — Linear AI-1545.

Root cause

ServerConnection.ConnectWithRetryAsync had two compounding defects:

1. A retry iteration always called StartAsync. The try block was StartAsync → RegisterDaemon, so when registration threw after a successful start (the incident log shows exactly this: DaemonConnectAsync → The 'InvokeCoreAsync' method cannot be called if the connection is not active, i.e. the transport dropped into Reconnecting between the two steps), the next iteration called StartAsync on a non-Disconnected hub. That throws The HubConnection cannot be started if it is not in the Disconnected state, lands in the generic catch, backs off, and repeats. Once automatic reconnect healed the transport and OnReconnected re-registered, the daemon was fully healthy — but the loop could never succeed (it never reached the register step again) and never exit: an unkillable Warning every 30s against a live connection. 12,348 occurrences in one daemon log, streaks of 900+ (~7.8h).
2. The loop was not serialized. It runs from the initial ConnectAsync and from every Closed event, so each disconnect episode could mint another concurrent zombie loop. The log shows three interleaved loops racing one hub — "Connection attempt 1277", "attempt 3", and "attempt 2" logging within the same second.

Fix

ConnectWithRetryAsync is now:

• serialized behind a SemaphoreSlim(1,1) — the initial connect and every Closed-triggered reconnect share one loop; a queued caller that acquires the lock after someone else already healed the connection returns immediately;
• state-aware — each iteration first exits if IsReady (auto-reconnect + OnReconnected, or the heartbeat's ReRegisterAsync, already restored a registered connection), then calls StartAsync only when the hub is actually Disconnected, then always (re-)attempts RegisterDaemonAsync. A register-step failure now retries registration on the live hub instead of a doomed restart. While the hub is Connecting/Reconnecting, the register attempt fails transiently and the loop backs off — automatic reconnect terminally either restores the connection (loop observes IsReady and exits) or exhausts to Disconnected (loop starts the hub), so it converges.

No behavioural change to name-in-use propagation, cancellation, backoff schedule, or the OnReconnected/heartbeat re-registration paths.

Tests

New ConnectWithRetryTests (mirrors the AcpServerConnectionTests fake-subclass approach; new internal seams HubState/StartHubAsync/RegisterDaemonAsync, and ConnectRetryDelays made settable so the retry path runs in milliseconds). The fake mirrors the real HubConnection.StartAsync contract of throwing when not Disconnected:

• live registered connection → no start/register attempt (acceptance: "connection is live → no reconnect attempt");
• connected-but-unregistered hub → registered without a start attempt;
• register failure after a successful start → retried registration, one physical start (acceptance: "connection failed → the retry path can actually succeed");
• Reconnecting hub → never started; loop exits once the transport heals;
• concurrent calls → one loop, one start;
• cancellation during backoff propagates.

Mutation-verified: with the loop temporarily reverted to the pre-fix logic (unconditional StartAsync, no lock), 5 of the 6 tests fail; all 6 pass against the fix.

Verification

• Full Tests.Unit suite: 3962 passed / 42 failed — the 42 are exactly the pre-existing macOS /tmp-symlink / parallel-load flake families (CodexConfigToml, Uninstall, CodexLauncher, CodexConfigWriter, PluginCodexInstall), none daemon-related.
• dotnet publish -c Release: no IL3050/IL2026 AOT warnings.

🤖 Generated with Claude Code

Review round (codex, clean on round 2)

The codex review flow surfaced a P1 in the adjacent recovery path, fixed in the second commit: HubConnection.StopAsync's cancellation token is a dead parameter in the pinned 10.0.8 client (StopAsyncCore waits the connection lock and stops the transport on token: default — verified against the release/10.0 source), so ForceReconnectAsync's 5s cap never actually fired and a wedged transport would have hung the single heartbeat tick forever, permanently killing liveness probing. The cap is now enforced from outside via .WaitAsync(cts.Token) through a new StopHubAsync seam with a settable ForceStopCap, plus a regression test that wedges the stop and asserts the call returns at the cap (mutation-verified: passing the token inward instead fails the test). The reviewer's P2 also hardened the reconnecting-state test: the fake now fails registration unless the hub is Connected (mirroring the real invoke contract) and the test observes failed attempts deterministically instead of sleeping.

[linear-code]

AI-1545

[qodo-code-review]

PR Summary by Qodo

Stop daemon reconnect loop spinning against a healthy SignalR connection

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Serialize daemon connect/reconnect attempts to prevent concurrent retry loops.
• Make retries state-aware: only start when disconnected, always retry registration.
• Add regression tests covering healthy, reconnecting, concurrent, and cancellation scenarios.

Diagram

graph TD
  A["DaemonRunner / startup"] --> B["ServerConnection"] --> C["SignalR HubConnection"] --> D["Server hub"]
  E["DaemonHeartbeatLoop"] --> B["ServerConnection"] --> F["RegistrationGate"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Cancel/replace prior retry loop via linked CTS

• ➕ Hard-stops any existing zombie loop instead of queueing behind a lock
• ➕ Can simplify shutdown/teardown semantics for reconnect bursts
• ➖ More moving parts (CTS lifecycle, racey cancellation windows)
• ➖ Still requires hub-state gating to avoid StartAsync-on-non-Disconnected failures

2. Dedicated single reconnect worker (channel/actor model)

• ➕ Centralizes all reconnect triggers (startup, Closed, heartbeat) into one serialized worker
• ➕ Naturally prevents re-entrancy and makes backoff policy easier to evolve
• ➖ Bigger refactor and more infrastructure code than a SemaphoreSlim
• ➖ Harder to keep behavior identical (especially name-in-use propagation)

3. Abstract HubConnection behind an interface instead of virtual seams

• ➕ Cleaner unit-test boundary; avoids inheritance-based test doubles
• ➕ Reduces risk of accidentally overriding production behavior in derived types
• ➖ Wider signature churn and DI changes across the daemon
• ➖ More invasive than needed for this targeted regression fix

Recommendation: Keep the current SemaphoreSlim + hub-state-aware loop. It is the smallest change that fixes both root causes (non-Disconnected StartAsync and concurrent loops), preserves existing behavior (name-in-use and backoff semantics), and is now well-covered by targeted regression tests through minimal internal seams.

Files changed (2) +266 / -41

Bug fix (1) +91 / -41

ServerConnection.csSerialize and harden ConnectWithRetryAsync against healthy/reconnecting hubs +91/-41

Serialize and harden ConnectWithRetryAsync against healthy/reconnecting hubs

• Reworks ConnectWithRetryAsync to run under a SemaphoreSlim so startup and Closed-triggered reconnects share one loop. Adds hub-state checks to only start when Disconnected, always retry registration, and exit early when already ready; introduces internal seams (HubState, StartHubAsync, RegisterDaemonAsync) and configurable retry delays to enable deterministic unit testing.

src/Capacitor.Cli.Daemon/Services/ServerConnection.cs

Tests (1) +175 / -0

ConnectWithRetryTests.csAdd regression tests for serialized, state-aware connect retry behavior +175/-0

Add regression tests for serialized, state-aware connect retry behavior

• Adds a fake ServerConnection that emulates HubConnection.StartAsync’s Disconnected-only contract and drives failure modes deterministically. Covers leaving healthy connections alone, registering without restart, retrying registration after start, handling Reconnecting state, serializing concurrent calls, and cancellation during backoff.

test/Capacitor.Cli.Tests.Unit/Daemon/ConnectWithRetryTests.cs

[qodo-code-review]

Code Review by Qodo

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

1. Empty retry delays crash 🐞 Bug ☼ Reliability

Description

ConnectWithRetryAsync indexes ConnectRetryDelays without guarding for null/empty; if it’s set to an
empty array, the retry handler throws IndexOutOfRangeException and aborts the reconnect loop instead
of backing off and retrying. This is newly possible because the delay schedule is now an internal
settable property rather than a fixed non-empty local array.

Code

src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[R460-464]

+                } catch (Exception ex) {
+                    var delay = ConnectRetryDelays[Math.Min(attempt, ConnectRetryDelays.Length - 1)];
+                    LogConnectionAttemptFailed(ex, attempt + 1, delay.TotalSeconds);
+                    await Task.Delay(delay, ct);
+                    attempt++;

Evidence

The retry path directly indexes ConnectRetryDelays using ConnectRetryDelays.Length - 1; when
length is 0, the computed index becomes -1 and throws, which escapes the reconnect loop’s intended
behavior.

src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[400-465]

Agent prompt

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

### Issue description
`ConnectWithRetryAsync` assumes `ConnectRetryDelays` is non-null and non-empty. Because it’s `internal` and settable, it can be set to an invalid value (e.g., empty array) and then the exception-handling path will throw an indexing exception, terminating the retry loop.

### Issue Context
This happens inside the general exception handler (the path that should be most robust), so a transient connection failure combined with an invalid delay schedule causes a hard failure rather than a retry.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[404-465]

### Suggested fix
- Defensively handle `ConnectRetryDelays` being `null` or empty before indexing:
 - Either throw an `ArgumentException` in the property setter (and keep the setter), or
 - Fall back to a safe default schedule when `ConnectRetryDelays` is `null`/empty, e.g. a single delay of 30s.
- Consider making the setter validate and normalize (e.g., copy the array) to prevent later mutation by callers.

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

---

2. Timing-based reconnect tests ✓ Resolved 🐞 Bug ☼ Reliability

Description

ConnectWithRetryTests uses fixed Task.Delay sleeps for synchronization before making assertions;
under slow/contended CI scheduling, these intermediate assertions can become flaky. The test suite
already has seams/hooks (e.g., StartHold) that can be extended to provide deterministic signaling
instead of sleep-based timing.

Code

test/Capacitor.Cli.Tests.Unit/Daemon/ConnectWithRetryTests.cs[R130-156]

+        // Give the loop a few iterations against the Reconnecting hub, then emulate SignalR's
+        // automatic reconnect healing the transport and OnReconnected re-registering.
+        await Task.Delay(TimeSpan.FromMilliseconds(100));
+        conn.State = HubConnectionState.Connected;
+        conn.Ready = true;
+
+        await loop.WaitAsync(HangGuard);
+
+        await Assert.That(conn.StartCalls).IsEqualTo(0);
+    }
+
+    [Test]
+    public async Task Concurrent_calls_share_one_loop_and_one_start() {
+        var conn = new RetryTestConnection { ConnectRetryDelays = FastDelays };
+        var hold = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously);
+        conn.StartHold = hold;
+
+        var first  = conn.ConnectWithRetryAsync(CancellationToken.None);
+        var second = conn.ConnectWithRetryAsync(CancellationToken.None);
+
+        // Let the second call reach the lock while the first is held mid-StartAsync — before the
+        // fix it would have raced ahead and called StartAsync on the same hub concurrently.
+        await Task.Delay(TimeSpan.FromMilliseconds(100));
+        await Assert.That(conn.StartCalls).IsEqualTo(1);
+
+        hold.SetResult();
+        await Task.WhenAll(first, second).WaitAsync(HangGuard);

Evidence

The tests include hard-coded 100ms sleeps prior to assertions/state changes, which is a known source
of nondeterministic test behavior under varying execution speed.

test/Capacitor.Cli.Tests.Unit/Daemon/ConnectWithRetryTests.cs[120-160]

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 unit tests rely on `Task.Delay(100ms)` to “let the loop run” before asserting on `StartCalls` or mutating state. This can intermittently fail on slower machines or under load.

### Issue Context
`RetryTestConnection` already provides controllable seams (`StartHold`, overridden `StartHubAsync`, etc.). Adding explicit signals (TaskCompletionSource) for milestones like “StartHubAsync entered” or “attempted register” makes tests deterministic.

### Fix Focus Areas
- test/Capacitor.Cli.Tests.Unit/Daemon/ConnectWithRetryTests.cs[120-160]

### Suggested fix
- Replace the `Task.Delay(...)` waits with explicit synchronization:
 - Add a `TaskCompletionSource` like `StartEntered` and complete it at the top of `StartHubAsync`.
 - In tests, await `StartEntered.Task.WaitAsync(...)` instead of sleeping.
- For the “Reconnecting hub heals” test, similarly add a signal that at least one loop iteration occurred (e.g., increment + signal on first `RegisterDaemonAsync` call).

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

---

3. Connect lock not disposed 🐞 Bug ⚙ Maintainability

Description

ServerConnection now owns a SemaphoreSlim instance for connect serialization, but DisposeAsync never
disposes it. While SemaphoreSlim often doesn’t materialize expensive native resources unless its
wait handle is used, this still leaves a new owned IDisposable unaccounted for in cleanup.

Code

src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[398]

+    readonly SemaphoreSlim _connectLock = new(1, 1);

Evidence

The PR adds _connectLock but DisposeAsync only disposes _terminalSenderCts, _httpClient, and
_hub—not _connectLock.

src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[390-472]
src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[1166-1188]

Agent prompt

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

### Issue description
A new `_connectLock` (`SemaphoreSlim`) is introduced and owned by `ServerConnection`, but it isn’t disposed in `DisposeAsync`, leaving lifecycle/ownership unclear.

### Issue Context
`SemaphoreSlim.Dispose()` is usually low-impact, but if you choose to dispose it, do so only once you’re sure no in-flight code will `WaitAsync`/`Release` it.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[390-472]
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[1166-1188]

### Suggested fix
- Option A (preferred for clarity): dispose `_connectLock` near the end of `DisposeAsync` after you’ve ensured connect operations cannot still be running (e.g., after cancellation/shutdown sequencing).
- Option B: if you intentionally do **not** dispose it to avoid disposal races, add a short comment explaining why (so future maintainers don’t assume it was missed).

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

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[qodo-code-review]

1. Empty retry delays crash 🐞 Bug ☼ Reliability

ConnectWithRetryAsync indexes ConnectRetryDelays without guarding for null/empty; if it’s set to an
empty array, the retry handler throws IndexOutOfRangeException and aborts the reconnect loop instead
of backing off and retrying. This is newly possible because the delay schedule is now an internal
settable property rather than a fixed non-empty local array.

Agent Prompt

### Issue description
`ConnectWithRetryAsync` assumes `ConnectRetryDelays` is non-null and non-empty. Because it’s `internal` and settable, it can be set to an invalid value (e.g., empty array) and then the exception-handling path will throw an indexing exception, terminating the retry loop.

### Issue Context
This happens inside the general exception handler (the path that should be most robust), so a transient connection failure combined with an invalid delay schedule causes a hard failure rather than a retry.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[404-465]

### Suggested fix
- Defensively handle `ConnectRetryDelays` being `null` or empty before indexing:
  - Either throw an `ArgumentException` in the property setter (and keep the setter), or
  - Fall back to a safe default schedule when `ConnectRetryDelays` is `null`/empty, e.g. a single delay of 30s.
- Consider making the setter validate and normalize (e.g., copy the array) to prevent later mutation by callers.

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

[qodo-code-review]

2. Connect lock not disposed 🐞 Bug ⚙ Maintainability

ServerConnection now owns a SemaphoreSlim instance for connect serialization, but DisposeAsync never
disposes it. While SemaphoreSlim often doesn’t materialize expensive native resources unless its
wait handle is used, this still leaves a new owned IDisposable unaccounted for in cleanup.

Agent Prompt

### Issue description
A new `_connectLock` (`SemaphoreSlim`) is introduced and owned by `ServerConnection`, but it isn’t disposed in `DisposeAsync`, leaving lifecycle/ownership unclear.

### Issue Context
`SemaphoreSlim.Dispose()` is usually low-impact, but if you choose to dispose it, do so only once you’re sure no in-flight code will `WaitAsync`/`Release` it.

### Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[390-472]
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[1166-1188]

### Suggested fix
- Option A (preferred for clarity): dispose `_connectLock` near the end of `DisposeAsync` after you’ve ensured connect operations cannot still be running (e.g., after cancellation/shutdown sequencing).
- Option B: if you intentionally do **not** dispose it to avoid disposal races, add a short comment explaining why (so future maintainers don’t assume it was missed).

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