Stand the daemon heartbeat down while the hub is reconnecting

[realtonyyoung]

Closes #868 — AI-2692

What & why

When the daemon's server connection dropped, it could not recover on its own even after the credential was valid again: it spun in a self-sustaining reconnect storm that only a daemon restart cleared. The daemon drives reconnection from two controllers — SignalR's WithAutomaticReconnect and the heartbeat loop — and while SignalR was mid-reconnect the connection was "not active", so every heartbeat ping threw, and the loop forced a reconnect that cancelled the negotiate the automatic reconnect had in flight, which kept the connection down, which made the next ping throw. The heartbeat now stands down whenever the hub is not Connected: it neither pings nor force-reconnects, leaving WithAutomaticReconnect and OnClosed to own recovery. It still force-reconnects a hung-but-Connected transport (the case it exists for) — a ping that times out while the hub reports Connected.

Where to look

Two guards in DaemonHeartbeatLoop.TickAsync: the tick skips entirely when IsConnected is false, and SafeForceReconnectAsync re-checks IsConnected so a connection that drops mid-ping does not trigger a force that races the reconnect already starting. IDaemonHeartbeatPort gained IsConnected, implemented by ServerConnection as HubState == Connected.

Verification

• DaemonHeartbeatLoopTests (12): a not-Connected hub skips without pinging or forcing; a connection that drops mid-ping does not force; the existing Connected-path cases (hung ping and slot-displacement) are unchanged.
• Full daemon unit suite green; daemon AOT publish clean.

[qodo-code-review]

PR Summary by Qodo

Prevent heartbeat reconnect races during SignalR recovery

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Pauses heartbeat activity whenever SignalR is not fully connected.
• Prevents forced reconnects from racing recovery after mid-ping disconnects.
• Adds regression coverage while preserving hung-transport and slot-displacement recovery.

Diagram

graph TD
  A["Heartbeat Tick"] --> B{"Hub Connected?"}
  B -- "Yes" --> D["Ping Hub"] --> E{"Ping Outcome?"}
  E -- "Timeout or error" --> G{"Still Connected?"} -- "Yes" --> H["Force Reconnect"]
  B -- "No" --> C["Skip Recovery"]
  E -- "Healthy" --> A
  E -- "Slot displaced" --> F["Re-register"]
  G -- "No" --> C

Loading

High-Level Assessment

The state-gated heartbeat is the appropriate approach because it preserves existing hung-transport recovery while assigning in-progress connection recovery exclusively to SignalR. Exception filtering or lifecycle-managed pause flags were considered, but direct state checks cover both tick entry and mid-ping disconnects with less duplicated state and synchronization.

Files changed (3) +72 / -2

Bug fix (2) +27 / -0

DaemonHeartbeatLoop.csGate heartbeat and forced recovery on connected hub state +23/-0

Gate heartbeat and forced recovery on connected hub state

• Adds connection state to the heartbeat port and skips ticks unless the hub is connected. Rechecks the state before forcing a reconnect so a mid-ping disconnect cannot race SignalR recovery.

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

ServerConnection.csExpose connected SignalR state to the heartbeat +4/-0

Expose connected SignalR state to the heartbeat

• Implements the heartbeat connection gate by mapping 'IsConnected' to an exact 'HubConnectionState.Connected' check.

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

Tests (1) +45 / -2

DaemonHeartbeatLoopTests.csCover reconnecting and mid-ping disconnect races +45/-2

Cover reconnecting and mid-ping disconnect races

• Extends the fake heartbeat port with connection state and ping-call tracking. Adds regression tests proving reconnecting hubs are not pinged and mid-ping disconnects do not trigger forced recovery.

test/Capacitor.Cli.Daemon.Tests.Unit/Services/DaemonHeartbeatLoopTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Reconnect storms can still recur ✓ Resolved 🐞 Bug ☼ Reliability

Description

SafeForceReconnectAsync classifies a failed ping only by reading the current IsConnected value,
although a connection-loss exception can be observed before HubState transitions away from
Connected. In that ordering the guard passes and ForceReconnectAsync stops the hub while
automatic recovery starts moments later, preserving the reconnect race for the failure mode this
change targets.

Code

src/Capacitor.Cli.Daemon/Services/DaemonHeartbeatLoop.cs[R137-140]

+        if (!port.IsConnected) {
+            logger.LogDebug("Heartbeat: reconnect already in progress — not forcing");
+
+            return;

Relevance

●●● Strong

Accepted timing-race precedent supports snapshotting connection state before failure classification;
this guard still samples state too late.

PR-#152

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new guard reads IsConnected at DaemonHeartbeatLoop.cs:137-140, but the unconditional stop
remains a separate call at lines 143-144. ServerConnection.IsConnected directly samples hub state,
reconnect callbacks run asynchronously, and ForceReconnectAsync proceeds to StopHubAsync without
validating the failure cause or connection incarnation; the new test avoids this ordering by setting
IsConnected = false synchronously before returning the failed task. Past PR #152 documents the
same repository-specific timing problem: connection readiness may change between an invocation
failure and later exception classification.

src/Capacitor.Cli.Daemon/Services/DaemonHeartbeatLoop.cs[133-144]
src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[476-489]
src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[752-827]
test/Capacitor.Cli.Daemon.Tests.Unit/Services/DaemonHeartbeatLoopTests.cs[239-255]
PR-#152

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 reconnect guard samples hub state after a failed ping, but the connection-loss exception and the state transition are not atomic. If the exception is handled while the hub still reports Connected, the heartbeat can still force-stop a connection whose automatic recovery is starting.

## Fix Focus Areas
- src/Capacitor.Cli.Daemon/Services/DaemonHeartbeatLoop.cs[95-110]
- src/Capacitor.Cli.Daemon/Services/DaemonHeartbeatLoop.cs[133-144]
- src/Capacitor.Cli.Daemon/Services/ServerConnection.cs[795-827]
- test/Capacitor.Cli.Daemon.Tests.Unit/Services/DaemonHeartbeatLoopTests.cs[239-255]

## Recommended Fix
Do not use a separately sampled connection-state value as the sole classification for ping exceptions. Preserve the failure cause and make disconnect-related invocation failures stand down independently of the current state, while retaining forced reconnect for a genuinely timed-out transport that remains on the same connected incarnation; add a deterministic test where the ping fails before the state changes to Reconnecting and verify that no force-stop occurs.

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

---

2. A test comment ages with the suite ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The comment above FakePort.IsConnected justifies its default using pre-existing tests rather
than the fixture's current behavioral contract. As tests are added or reorganized, that historical
claim stops being reliable and gives later readers no durable reason for the default.

Code

test/Capacitor.Cli.Daemon.Tests.Unit/Services/DaemonHeartbeatLoopTests.cs[32]

+        // Defaults to the Connected steady state every pre-existing test exercises.

Relevance

●●● Strong

Recent test-comment precedents accept removing historical or assertion-restating narration in favor
of durable behavioral rationale.

PR-#722
PR-#718

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added comment explicitly refers to pre-existing tests, making its truth dependent on the test
suite at the time of the change. It therefore records historical context rather than a durable,
non-obvious behavioral constraint.

Rule 2762993: Restrict comments to documenting non-obvious, behavior‑critical constraints
Rule 2897915: Avoid time-sensitive or process-reference metadata in code comments
test/Capacitor.Cli.Daemon.Tests.Unit/Services/DaemonHeartbeatLoopTests.cs[32-32]

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 comment above `FakePort.IsConnected` depends on which tests existed when it was written and does not document a durable behavioral constraint.

## Fix Focus Areas
- test/Capacitor.Cli.Daemon.Tests.Unit/Services/DaemonHeartbeatLoopTests.cs[32-32]

## Recommended Fix
Remove the comment, or replace it with a concise explanation of a current behavior-critical reason for defaulting the fake port to connected that remains true as the suite changes.

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

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

  Explored: repo: kurrent-io/kcap-server (sha: 01e241aa)

Review mode: ⚖️ Balanced: This is a localized but behavior-changing daemon reconnection fix affecting connection-state coordination and recovery semantics, warranting a careful single-pass review.

Tip of the day

💡 Did you know, you can switch off images and animations for a plain-text comment

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[realtonyyoung]

Ran a Codex review of this branch (codex exec review --base main). It confirmed the connection-state gating addresses the reconnect race with no functional regression, and flagged one P3 (change-history wording in two new test comments), now rewritten.

Qodo separately raised a valid High: sampling IsConnected after a ping throws still had a race, because the invoke failure can be observed before HubState transitions. Addressed by reworking the guard to key on the failure cause — a ping that throws stands down unconditionally (SignalR already owns recovery), and only a hung ping (deadline) forces a reconnect, so the force decision no longer reads hub state at all.