Add WebSocket health monitoring and reconnection#133
Conversation
WalkthroughThe Frost coordinator now manages concurrent WebSocket relays with health tracking, reconnect backoff, session recovery, buffered event replay, subscription validation, relay scoring, public status APIs, and native tests for lifecycle and race safety. ChangesCoordinator relay recovery
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Coord as frost_coordinator
participant Buffer as Event buffer
participant Relay as WebSocket relay
App->>Coord: Publish serialized event
Coord->>Relay: Send to connected relays
Relay-->>Coord: Connection or data event
Coord->>Buffer: Buffer when relays are reconnecting
Coord->>Relay: Reconnect and resubscribe
Coord->>Relay: Replay buffered events in order
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main/frost_coordinator.c (1)
678-720: Guard event buffer mutations with the mutex.
buffer_eventis called withoutCOORDINATOR_LOCK, butreplay_buffered_events/clear_event_buffermutate the same buffer under lock. This can race and corrupt/free the buffer.🔒 Suggested fix
if (published == 0 && any_reconnecting) { + COORDINATOR_LOCK(); buffer_event(msg); + COORDINATOR_UNLOCK(); ESP_LOGI(TAG, "Buffered event during reconnection"); }
🤖 Fix all issues with AI agents
In `@main/frost_coordinator.c`:
- Around line 494-513: The code currently truncates overlong relay URLs in
frost_coordinator_add_relay by using strncpy, which can silently connect to the
wrong endpoint; update frost_coordinator_add_relay to first check the length of
url against RELAY_URL_LEN (e.g. if (strlen(url) >= RELAY_URL_LEN) return an
error like -4), log an error via ESP_LOGE mentioning the URL is too long, and
only then copy the string safely into relay->url (or use a bounded copy after
the length check); reference validate_websocket_url,
frost_coordinator_add_relay, relay_connection_t, and RELAY_URL_LEN so the length
check and failure path are added before the current memset/strncpy block.
- Around line 601-613: The validate_subscription_id function allows IDs of
length WS_MAX_SUBSCRIPTION_ID due to an off-by-one check (if (len >
WS_MAX_SUBSCRIPTION_ID)), which then get truncated by copy_subscription_id;
change the length check to reject IDs with length >= WS_MAX_SUBSCRIPTION_ID so
valid IDs cannot exceed the buffer (update the condition in
validate_subscription_id to use >=), and keep the existing checks for control
chars and quotes; reference validate_subscription_id and copy_subscription_id
and the WS_MAX_SUBSCRIPTION_ID constant when making the change.
🧹 Nitpick comments (1)
main/frost_coordinator.c (1)
790-865: Avoid holding the mutex while performing WS I/O.
send_pingandreplay_buffered_eventsperformesp_websocket_client_send_*whileCOORDINATOR_LOCKis held, which can block and starve other operations. Consider copying the needed state under lock, then sending outside the lock.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
main/frost_coordinator.c (2)
621-655: Thread safety:frost_coordinator_subscribemodifies shared state without lock.This function modifies
g_ctx.current_subscriptionandg_ctx.has_subscription(lines 629-631) without holding the mutex. These fields are read byhandle_ws_connectedunder lock (lines 236-240), creating a potential data race.🔒 Suggested fix: protect shared state modification
int frost_coordinator_subscribe(const char *subscription_id) { if (!g_initialized || !g_ctx.has_group) return -1; if (!validate_subscription_id(subscription_id)) { ESP_LOGE(TAG, "Invalid subscription ID"); return -2; } + COORDINATOR_LOCK(); copy_subscription_id(g_ctx.current_subscription, sizeof(g_ctx.current_subscription), subscription_id); g_ctx.has_subscription = true; + COORDINATOR_UNLOCK(); char pubkey_hex[65]; ...
657-682: Thread safety:frost_coordinator_unsubscribemodifies shared state without lock.Similar to
frost_coordinator_subscribe, this function modifiesg_ctx.has_subscriptionandg_ctx.current_subscriptionwithout holding the mutex.
🤖 Fix all issues with AI agents
In `@main/frost_coordinator.c`:
- Around line 308-322: handle_ws_error may double-increment relay->fail_count
because it increments on the max-attempts path and also calls
save_reconnect_state (which itself increments fail_count) on the other path;
change the control so fail_count is incremented exactly once by either moving
the increment into save_reconnect_state only, or by adding a flag/parameter to
save_reconnect_state (e.g., save_reconnect_state(relay, is_final)) and call it
with is_final=false on transient path and true on final path, or skip calling
save_reconnect_state when attempt_count >= WS_RECONNECT_MAX_ATTEMPTS; update
handle_ws_error and save_reconnect_state accordingly and ensure
relay->reconnect.attempt_count/WS_RECONNECT_MAX_ATTEMPTS logic remains
consistent.
- Around line 97-115: buffer_event currently mutates shared state
(g_ctx.event_buffer, buffer_head, buffer_count) without synchronization; ensure
these mutations occur under COORDINATOR_LOCK by acquiring the same lock used in
frost_coordinator_poll before calling buffer_event from publish_event (or
alternatively, add lock/unlock inside buffer_event itself). Update publish_event
to lock COORDINATOR_LOCK around the call to buffer_event (and any other accesses
to g_ctx.event_buffer) so replay_buffered_events (which runs under the same
lock) cannot race, referencing the buffer_event, publish_event,
replay_buffered_events, COORDINATOR_LOCK, and frost_coordinator_poll symbols
when making the change.
- Around line 117-126: clear_event_buffer is being invoked from
frost_coordinator_disconnect without holding the context mutex, risking races
with concurrent event-handler callbacks; modify frost_coordinator_disconnect to
acquire the same mutex used to guard g_ctx.event_buffer (e.g., lock g_ctx.mutex
or g_ctx.event_mutex) before calling clear_event_buffer and release it after the
call, ensuring all buffer mutations are protected; alternatively, if
clear_event_buffer is called from multiple call sites where locking is not
guaranteed, add locking inside clear_event_buffer itself and document that it
performs its own locking to avoid double-locks.
🧹 Nitpick comments (1)
main/frost_coordinator.c (1)
92-95: Potential integer overflow before the cap is applied.
WS_RECONNECT_BASE_MS << attemptcan overflow before the comparison withWS_RECONNECT_MAX_MSifattemptis large. For example, shifting a 32-bit value left by 22+ positions causes undefined behavior in C.Consider capping early or using multiplication with overflow check:
♻️ Suggested fix
static uint32_t calculate_backoff(uint8_t attempt) { + if (attempt > 20) { + return WS_RECONNECT_MAX_MS; + } uint32_t delay = WS_RECONNECT_BASE_MS << attempt; return delay > WS_RECONNECT_MAX_MS ? WS_RECONNECT_MAX_MS : delay; }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main/frost_coordinator.c (1)
627-697: Make subscription state updates thread-safe.
frost_coordinator_subscribe/unsubscribeupdateg_ctx.current_subscription,g_ctx.has_subscription, andg_ctx.statewithoutCOORDINATOR_LOCK. With reconnect paths invoking subscribe from the WS event thread, this becomes a data race. Lock these state mutations (and consider snapshotting relay targets if you need to avoid holding the lock during sends).🔒 Proposed fix
- copy_subscription_id(g_ctx.current_subscription, sizeof(g_ctx.current_subscription), - subscription_id); - g_ctx.has_subscription = true; + COORDINATOR_LOCK(); + copy_subscription_id(g_ctx.current_subscription, sizeof(g_ctx.current_subscription), + subscription_id); + g_ctx.has_subscription = true; + COORDINATOR_UNLOCK(); @@ - g_ctx.state = COORDINATOR_STATE_ACTIVE; + COORDINATOR_LOCK(); + g_ctx.state = COORDINATOR_STATE_ACTIVE; + COORDINATOR_UNLOCK(); @@ - g_ctx.has_subscription = false; - memset(g_ctx.current_subscription, 0, sizeof(g_ctx.current_subscription)); + COORDINATOR_LOCK(); + g_ctx.has_subscription = false; + memset(g_ctx.current_subscription, 0, sizeof(g_ctx.current_subscription)); + COORDINATOR_UNLOCK();
♻️ Duplicate comments (1)
main/frost_coordinator.c (1)
603-611: Guard disconnect state resets with the mutex.
frost_coordinator_disconnectmutates shared coordinator state withoutCOORDINATOR_LOCK, while other paths read it under lock. This can race with WS callbacks orpoll().🔒 Proposed fix
- clear_event_buffer(); - g_ctx.has_subscription = false; - g_ctx.disconnect_time = 0; - g_ctx.state = COORDINATOR_STATE_IDLE; + COORDINATOR_LOCK(); + clear_event_buffer_unlocked(); + g_ctx.has_subscription = false; + g_ctx.disconnect_time = 0; + g_ctx.state = COORDINATOR_STATE_IDLE; + COORDINATOR_UNLOCK();
🧹 Nitpick comments (2)
main/frost_coordinator.c (2)
708-745: Consider buffering when any relay is reconnecting.Right now buffering happens only when
published == 0, so reconnecting relays miss events if at least one relay is connected. If the intent is replay for any disconnected relay, consider buffering wheneverany_reconnectingis true (and dedupe as needed).
816-890: Reduce time under COORDINATOR_LOCK in the poll path.
frost_coordinator_pollholds the mutex while invoking ping/reconnect/replay helpers. Consider snapshotting work under the lock, then performing I/O outside it to reduce contention.
759b04b to
b23b5ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
test/native/mocks/ws_transport_mock.c (1)
191-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake start-failure injection fail
ws_transport_start().
ws_mock_fail_next_starts()is consumed byws_transport_create(), so tests never exercise the coordinator’s distinct start-failure cleanup path. Move this counter check intows_transport_start()and add a separate create-failure control if both paths are needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/native/mocks/ws_transport_mock.c` around lines 191 - 225, The g_fail_starts injection is currently consumed by ws_transport_create(), preventing tests from exercising ws_transport_start() failure cleanup. Remove that check from ws_transport_create(), add the counter check in ws_transport_start() before marking the handle started, and decrement it when returning failure; introduce a separate create-failure control if creation failures are still required.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main/frost_coordinator.c`:
- Around line 126-133: Update op_lock() so the non-ESP_PLATFORM pthread branch
uses a bounded acquisition, such as pthread_mutex_trylock() or a timed lock
based on WS_OP_LOCK_TIMEOUT_MS, instead of blocking indefinitely; preserve the
existing boolean success behavior and ESP_PLATFORM implementation.
- Around line 330-347: Ignore callbacks from detached WebSocket transports by
adding an early return in both handle_ws_connected() and handle_ws_failure()
while holding state_mutex: if relay->ws == NULL, unlock and return before
logging or changing relay state. Ensure the guard precedes all mutations,
including enter_reconnecting().
- Around line 209-216: Ensure failed event delivery is never reported as
successful: in the send/publish logic, buffer whenever published == 0 and (count
> 0 || any_reconnecting), including when all connected sends fail; update
buffer_event_copy() to return a success/failure status, return failure when
malloc fails, and propagate that status through its callers so allocation
failure returns -1 instead of logging successful buffering.
- Around line 681-710: frost_coordinator_subscribe clears resubscription flags
before sending and ignores per-relay failures, leaving failed initial
subscriptions inactive. Update snapshot_connected usage to retain each relay’s
handle and association, send the filter per relay, and restore that relay’s
needs_resubscribe flag when its send fails; preserve successful relays as
subscribed and handle aggregate failures appropriately.
- Around line 997-1007: Replay must stop at the first failed event to preserve
ordering. In the replay loop handling drained events, after send_to_handles
fails and buffer_event_take re-buffers the event, break out of the loop so later
events remain queued rather than overtaking it.
- Around line 301-324: Validate the subscription ID in handle_nostr_message
before dispatching any event: ensure arr[1] is a string, capture a locked
snapshot of g_ctx.has_subscription and g_ctx.current_subscription, and require
the received ID to match that snapshot. Reject and clean up frames when there is
no active subscription or the IDs differ, then call dispatch_frost_event only
for matching events.
---
Nitpick comments:
In `@test/native/mocks/ws_transport_mock.c`:
- Around line 191-225: The g_fail_starts injection is currently consumed by
ws_transport_create(), preventing tests from exercising ws_transport_start()
failure cleanup. Remove that check from ws_transport_create(), add the counter
check in ws_transport_start() before marking the handle started, and decrement
it when returning failure; introduce a separate create-failure control if
creation failures are still required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 628e79bd-a6e6-45c4-b828-69c97ef77e90
📒 Files selected for processing (14)
Justfilemain/CMakeLists.txtmain/frost_coordinator.cmain/frost_coordinator.hmain/ws_transport.hmain/ws_transport_esp.ctest/native/CMakeLists.txttest/native/mocks/noscrypt.htest/native/mocks/nostr_frost_stubs.ctest/native/mocks/nostr_frost_stubs.htest/native/mocks/ws_transport_mock.ctest/native/mocks/ws_transport_mock.htest/native/test_coordinator.ctest/native/test_coordinator_race.c
✅ Files skipped from review due to trivial changes (3)
- Justfile
- test/native/mocks/noscrypt.h
- main/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- main/frost_coordinator.h
2 participants
Summary
Adds connection health monitoring, automatic reconnection with exponential backoff, event buffering with replay, and session-recovery timeouts to the FROST relay coordinator.
Closes #84.
Design
All
esp_websocket_clientcontact now lives behind a small transport seam (main/ws_transport.h), implemented byws_transport_esp.con device and by a mock in native tests.frost_coordinator.cno longer references the websocket client directly.Keepalive is delegated to the transport.
esp_websocket_clientalready sends pings onping_interval_secand drops the connection when no pong arrives withinpingpong_timeout_sec. That drop surfaces as a disconnect event, so there is no hand-rolled ping loop or missed-pong counter. Its built-in retry is disabled (disable_auto_reconnect = true) because it offers neither exponential backoff nor a max-attempt cap; the coordinator owns reconnection instead.Locking. Two mutexes with one hard invariant:
state_mutexguards all coordinator and relay state, held only for short non-blocking critical sections.op_mutexserializes transport lifecycle and sends, so a handle can never be destroyed while another caller is sending on it.ws_transport_*call is ever made whilestate_mutexis held.esp_websocket_client_stop()blocks until the websocket task drains, and that task takesstate_mutexin its event callback. Holding it across a stop would deadlock permanently. The native mock asserts this at every transport entry point, so the invariant cannot regress silently.Reconnect waits are 1s, 2s, 4s, 8s, 16s (capped at 30s) over at most 5 attempts. Events published while no relay is reachable are buffered (8 slots, oldest evicted) and replayed in order once a relay returns; a replay that fails stops so later events cannot overtake it. If nothing reconnects within 60s the session fails cleanly. Relay health, connection counts, and reconnect attempts are exposed via
frost_coordinator_get_status().Notable fixes over the first revision
stop/destroywhile holding the coordinator mutex, deadlocking against the websocket task's own event callback.publish_event,subscribe, andunsubscribereadrelay->wsand sent on it without the lock, racingpoll()'s teardown (use-after-free);disconnect()could double-free.auto_reconnect, which defaults to on.ERRORandDISCONNECTEDfire per failure, double-countingfail_countin relay health scores.disconnect_time != 0, which never fires for a disconnect at uptime 0.wss://is now required; plaintextws://is rejected.Test plan
New
test_coordinator(23 tests) drives the real coordinator state machine against a mock transport, covering each acceptance criterion in #84 plus every fix above: backoff schedule and cap, max attempts, handle lifecycle, buffering/replay ordering, resubscribe-on-reconnect, subscription-ID filtering, and failure accounting.New
test_coordinator_raceruns a publisher, a poller, and a transport thread concurrently under ThreadSanitizer. Removingop_mutexmakes it report a use-after-free; with it, the run is clean.Each fix was mutation-tested: the bug was reintroduced one at a time and the corresponding test confirmed to fail. That exercise found two assertions that could not fail and one real bug the suite had missed.
just test— all suites pass, including the TSan race testcppcheck,clang-tidy,scan-build,clang-format— cleanidf.py buildfor esp32s3 — clean, no warningsKnown limitation
The firmware has no WiFi stack and nothing calls the coordinator, so this module cannot open a socket on device today; the linker garbage-collects it and
keep.binis byte-identical with and without this change. It is therefore verified by native tests and a real cross-compile, not on hardware. Wiring it up (WiFi provisioning, a coordinator task, RPC to drive it) is tracked separately.