Skip to content

fix daemon reconect error - #40

Open
kerthcet wants to merge 3 commits into
InftyAI:mainfrom
kerthcet:fix/daemon-not-right
Open

fix daemon reconect error#40
kerthcet wants to merge 3 commits into
InftyAI:mainfrom
kerthcet:fix/daemon-not-right

Conversation

@kerthcet

@kerthcet kerthcet commented Aug 6, 2026

Copy link
Copy Markdown
Member

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?


Signed-off-by: kerthcet <kerthcet@gmail.com>
Copilot AI lite review requested due to automatic review settings August 6, 2026 16:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds an application-level heartbeat acknowledgement to improve daemon/controller resiliency when daemons are reaped from the server registry while the underlying WebSocket/TCP connection remains alive, and to help the daemon detect a controller that is connected but no longer processing messages.

Changes:

  • Extend the shared protocol with Message::HeartbeatAck { success, reason }.
  • Update the server to record heartbeats via a unit-testable helper and to ACK every heartbeat (success/failure).
  • Update the daemon to track heartbeat ACKs, re-register in-place on NACK, and reconnect if ACKs stop arriving; add end-to-end tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
server/src/server.rs Adds handle_heartbeat helper, sends HeartbeatAck on heartbeats, updates reaper commentary, and adds unit tests for reap/re-register scenarios.
sandd/src/main.rs Tracks last heartbeat ACK, reconnects on ACK silence, re-registers on rejected heartbeats, and adds WebSocket-level tests for recovery behavior.
protocol/src/lib.rs Introduces the HeartbeatAck message variant and documents intended behavior.
Suppressed comments (2)

server/src/server.rs:371

  • The comment says a reaped daemon’s “next heartbeat re-registers it”, but the heartbeat itself is rejected and the daemon re-registers by sending a new Register. Clarifying this avoids suggesting the server reconstructs state automatically.
        // dead. Reaping a daemon whose socket is still open no longer orphans it: its
        // next heartbeat re-registers it (see handle_heartbeat), so a false reap costs
        // one heartbeat interval of invisibility rather than lasting until the socket
        // breaks. Detection is ~30-35s vs the old ~90-120s; clean disconnects are still

sandd/src/main.rs:491

  • The ack-timeout reconnect logic assumes the controller will send HeartbeatAck. If a daemon is upgraded before the controller, acks will never arrive and the daemon will disconnect/reconnect every ~ack_timeout even though the connection is otherwise healthy. Consider only enforcing the ack-timeout after at least one HeartbeatAck has been observed on the connection (or add a feature/compat negotiation) to preserve backwards compatibility.
            _ = ack_check.tick() => {
                let silent_for = last_ack.elapsed();
                if silent_for >= ack_timeout {
                    warn!(
                        "No heartbeat ack for {}s (controller connected but unresponsive); reconnecting",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sandd/src/main.rs Outdated
Comment thread server/src/server.rs Outdated
kerthcet and others added 2 commits August 6, 2026 17:27
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: kerthcet <kerthcet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants