Connection-layer hardening#79
Merged
Merged
Conversation
With a storage size that was not a multiple of the 8-byte item alignment, the wrap-around dummy filler stored its raw tail size while the reader skipped the aligned size, desynchronizing the free-byte accounting (data corruption); a tail smaller than ItemHeader made try_acquire fail forever even on an empty buffer (permanent stall). Round the usable size down to the alignment in create() so unaligned tails can never exist, and reject storage too small for a single item. The default audio_buffer_capacity happened to be a multiple of 8, so default configs were unaffected; the ESP FreeRTOS-backed build was never affected. (cherry picked from commit 35a3fe8b5e5ce249c473deafd12c67f26d5c33c8) (cherry picked from commit 45c484b767382925e3d4f064b8c7c6d9b7218ee7)
is_connected() was permanently true (sockfd_ set once, never cleared), so between the httpd close notification and the main loop dropping the connection, queued async sends could still resolve the stale sockfd against httpd's session table; if the fd had been recycled for a new session, the frame went to the wrong peer. The close callback now marks the connection closed (via the still-pinned session ctx) before notifying the manager, and is_connected()/the async workers observe the flag. (cherry picked from commit 20043c62bdd23d76cef70ed6da1094067c595993) (cherry picked from commit c69a0064656c6763415cb688687cfee178589f15)
Ported from the encryption branch (9246bbe), dropping the encryption-only pieces (the httpd_stack_size clamp and the drop_connection-based connect_to adoption) and re-deriving connect_to()/loop() against main's structure. - Make connected_ (both client connections) and pending_time_message_ atomic: written by transport threads, read cross-thread (FINDINGS C14). - Derive the host synthetic sockfd from a monotonic counter instead of the ix::WebSocket heap pointer, which could repeat after free+realloc and mis-route close events (FINDINGS C15). - On reassembly-buffer allocation failure, disable message dispatch and actually close/stop the transport instead of leaving it open and processing frames on a connection about to be dropped; the host server path previously fell through and dispatched a stale buffer (FINDINGS C16). - connect_to(): release an existing pending connection (with goodbye) before overwriting the slot, and tear down a present-but-disconnected current connection as on_connection_lost would, instead of orphaning either (FINDINGS C9). - Back off 5 s between WS server start attempts after a failure; a port conflict previously logged every loop tick (FINDINGS P8). - Correct the host server TCP_NODELAY comment: IXWebSocket only sets it on outbound connects and exposes no way to set it on accepted sockets (FINDINGS P2). (cherry picked from commit f857c97632c1a3696344963ee794635d9974e43e)
Ported from the encryption branch (981ada4), adapted to main's lock-guarded connection slots: current_shared() returns the shared_ptr under conn_ptr_mutex_ directly instead of maintaining a separate published snapshot. - Add ConnectionManager::current_shared(): a shared_ptr to the current connection taken under conn_ptr_mutex_. is_time_synced()/get_client_time() (called from the sync task and visualizer/artwork drain threads) and the public get_server_information() now hold the connection alive while using it, instead of dereferencing a raw current() pointer that the main loop could concurrently destroy (FINDINGS C2/E5). - Serialize process_json_message() with a mutex: two live connections (current + pending during a handoff, or an outbound connect_to() transport beside the inbound server) parse on their own network threads, and the shared JSON arena has no internal locking (FINDINGS C3). - Tag deferred time-response events with their source connection and drop mismatches at drain time, so a response from a displaced or pending server can no longer be applied to whatever connection is current when the queue drains, contaminating the Kalman filter (FINDINGS C6, time-response portion). (cherry picked from commit 24966fe05fe72c54cf646e45160ed32074106dea)
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the Sendspin connection layer against cross-thread races and lifecycle edge cases, aiming to prevent stale events/state from leaking across connection handoffs or teardown while keeping the happy-path behavior unchanged.
Changes:
- Improves thread-safety of shared connection state (atomics, safer access to current connection, serialization of JSON parsing with a shared arena).
- Tightens lifecycle/teardown handling (better cleanup on replacement/handoff, earlier “closed” marking for ESP httpd sessions, safer behavior on allocation failures).
- Reduces operational noise by backing off WS server restart attempts after failures.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_spsc_ring_buffer.cpp | Clarifies a test comment about minimum storage sizing. |
| src/host/ws_server.cpp | Switches host synthetic sockfd generation to a monotonic counter to avoid pointer-reuse collisions. |
| src/host/server_connection.cpp | Drops host server connection on receive-buffer allocation failure; adds note on TCP_NODELAY behavior. |
| src/host/client_connection.h | Makes host client connected_ atomic for cross-thread reads/writes. |
| src/host/client_connection.cpp | On allocation failure, disables dispatch and initiates async close to avoid deadlock/stale dispatch. |
| src/esp/ws_server.cpp | Marks ESP server connections closed in httpd close callback to avoid stale sockfd reuse windows. |
| src/esp/server_connection.h | Adds closed_ atomic and mark_closed() to reflect httpd session closure promptly. |
| src/esp/server_connection.cpp | Incorporates closed_ into is_connected() and disables dispatch on allocation failure. |
| src/esp/client_connection.h | Makes ESP client connected_ atomic for cross-thread reads/writes. |
| src/esp/client_connection.cpp | Disables dispatch on allocation failure before triggering disconnect handling. |
| src/connection.h | Makes pending_time_message_ atomic due to cross-thread access. |
| src/connection_manager.h | Adds current_shared() for thread-safe shared_ptr access and tracks WS server restart backoff time. |
| src/connection_manager.cpp | Fixes lifecycle holes in connect/handoff teardown and adds WS server start retry backoff. |
| src/client.cpp | Tags time responses with source connection and serializes JSON parsing across concurrent connections. |
| include/sendspin/client.h | Documents connect_to() main-thread requirement; adds mutex for JSON processing serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
A raw SendspinConnection* could ABA-match a freed connection against a later one reused at the same address, applying a stale time measurement. Tag each connection with a monotonic instance id and compare that.
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
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.
Fixes a set of cross-thread races and lifecycle holes in the connection layer, surfaced by a full-repo review. All changes are defensive; no behavior changes on the happy path.
Thread-safety of shared state
Time-filter contamination
Lifecycle / handoff holes