fix: replace WebSocket ping with inactivity watchdog; extend NoteSync timeout#8
Merged
Conversation
… timeout PR #6 used ping_interval=30 but the server does not respond to WebSocket ping frames — confirmed by real-environment test: connection dropped every 30s with "keepalive ping timeout". Reverted to ping_interval=None. Instead, an asyncio background task (_inactivity_watchdog) tracks the last received message time and closes the connection if no data arrives for 2 × heartbeat_interval (60s default), triggering the reconnect loop. _last_received_at is updated on every text or binary frame received. Also extended the NoteSync timeout in _initial_sync from 60s to 300s. The server sends NoteSyncEnd immediately but may delay actual note delivery by over a minute on full syncs — the 60s window was too short and caused NoteSync to abort before any notes were written. Verified with real-server pull: 24 notes (including Test-456/123.md, 456.md, 789.md) fully synced, no ping disconnects, no echo push-backs.
Reviewer's GuideReplaces WebSocket-level pings with an application-side inactivity watchdog and extends the initial note sync timeout to handle slow, real-world server behavior without premature disconnects or sync aborts. Sequence diagram for WebSocket inactivity watchdog and reconnectsequenceDiagram
actor User
participant Client
participant InactivityWatchdog
participant WebSocket
participant Server
User->>Client: start
Client->>WebSocket: connect(ping_interval=None, ping_timeout=None)
WebSocket-->>Client: connection_open
Client->>Client: _last_received_at = time.monotonic()
Client->>Client: start _listen()
Client->>InactivityWatchdog: start _inactivity_watchdog()
loop normal_message_flow
Server-->>WebSocket: message
WebSocket-->>Client: raw
Client->>Client: _handle_text or _handle_binary
Client->>Client: _last_received_at = time.monotonic()
end
loop watchdog_check_every_heartbeat_interval
InactivityWatchdog->>InactivityWatchdog: sleep(heartbeat_interval)
InactivityWatchdog->>InactivityWatchdog: idle = now - _last_received_at
alt idle < 2 * heartbeat_interval
InactivityWatchdog-->>InactivityWatchdog: continue
else idle >= 2 * heartbeat_interval
InactivityWatchdog->>Client: log idle warning
InactivityWatchdog->>WebSocket: close()
WebSocket-->>Client: connection_closed
Client->>Client: existing reconnect loop triggered
InactivityWatchdog-->>InactivityWatchdog: return
end
end
Updated class diagram for WebSocket client watchdog and timestamp trackingclassDiagram
class AppConfig {
}
class Client {
- config : AppConfig
- _on_reconnect : Callable
- _msg_queue : list
- _ready_event : asyncio.Event
- _last_received_at : float
- ws
+ __init__(config : AppConfig) : None
+ on_reconnect(handler : Callable) : None
+ _connect() : Coroutine
+ _listen() : Coroutine
+ _inactivity_watchdog() : Coroutine
+ _handle_text(raw : str) : Coroutine
+ _handle_binary(raw : bytes) : Coroutine
}
Client --> AppConfig
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_listen, you cancel thewatchdogtask infinallybut never await it; consider callingwatchdog.cancel()followed byawait asyncio.gather(watchdog, return_exceptions=True)to ensure it finishes cleanly and to avoid potential "Task was destroyed but it is pending" warnings. - In
_inactivity_watchdog,interval = self.config.client.heartbeat_intervalis assumed to be a positive value; adding a guard (e.g., raising or falling back to a sane default when it is 0 or negative) would prevent a potential busy loop or invalid sleep.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_listen`, you cancel the `watchdog` task in `finally` but never await it; consider calling `watchdog.cancel()` followed by `await asyncio.gather(watchdog, return_exceptions=True)` to ensure it finishes cleanly and to avoid potential "Task was destroyed but it is pending" warnings.
- In `_inactivity_watchdog`, `interval = self.config.client.heartbeat_interval` is assumed to be a positive value; adding a guard (e.g., raising or falling back to a sane default when it is 0 or negative) would prevent a potential busy loop or invalid sleep.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PR #6 applied
ping_interval=heartbeat_intervalto detect dead connections. Real-environment testing revealed the server does not support WebSocket ping frames — it responded with code 1011 and closed the connection every 30 seconds.Additionally,
_initial_syncused a 60s timeout for NoteSync. On a full sync (lastTime=0) the server sendsNoteSyncEndimmediately but delivers actual note data over a minute later — causing NoteSync to abort before any files were written.Fix
Heartbeat: Reverted to
ping_interval=None. Added_inactivity_watchdog()— an asyncio background task that checks elapsed time since last received message everyheartbeat_intervalseconds. If no data received for2 × heartbeat_interval, it closes the connection, which triggers the existing reconnect loop.Timeout: Extended
_initial_syncNoteSync timeout from 60s → 300s.Verified
Real-server
pulltest:Test-456/123.md,456.md,789.md) ✓NoteModify →absent from logs) ✓Summary by Sourcery
Replace WebSocket ping-based heartbeats with an inactivity watchdog and extend the initial NoteSync timeout to better handle real-world server behavior.
Bug Fixes:
Enhancements: