Add a start/stop/restart client lifecycle by kahrendt · Pull Request #110 · Sendspin/sendspin-cpp

kahrendt · 2026-08-31T15:02:24Z

What

SendspinClient gains a real lifecycle: start(), stop(), request_stop(), and get_run_state(), plus a SendspinClientListener::on_stopped() completion callback. A stopped client can be started again, and the role background threads come back with it.

Call	Blocks?	Completion signal
`start()`	yes, thread creation only	return value
`stop()`	yes, joins role threads	return
`request_stop()`	no, completes over `loop()` ticks	`on_stopped()`, or `get_run_state() == STOPPED`

start_server() is kept as an alias for start(), so existing consumers compile unchanged. The one behavior difference: a repeat call while already started is now a no-op instead of re-initializing the WebSocket server.

Why

Teardown was destructor-only. There was no way to stop a client and start it again, which is exactly what a consumer needs on a reconfigure: ESPHome has to drop the connection and rebuild the client when its config changes, and destroying the whole object to do it is both heavy and impossible mid-loop.

request_stop() exists because the synchronous path cannot flush a goodbye on ESP-IDF. stop() tears the WebSocket server down as part of its teardown, and on ESP the goodbye send is an httpd worker job, so httpd_stop() can close the session before the queued send runs. request_stop() keeps the server up through a grace window (STOP_GRACE_MS, 750 ms) so the goodbyes actually reach their peers, then finishes on a later loop() tick once every connection has closed and every role thread has reported exit, or at the deadline if a peer never closes.

How

Client state machine. run_state_ is STOPPED / RUNNING / STOPPING. finish_stop() is the shared teardown behind both stop paths; it holds run_state_ at STOPPING for its whole duration and guards re-entry with a tearing_down_ flag, because connection_manager_->stop() synchronously reaches cleanup_connection_state(), which can fire on_release_high_performance() into a listener that calls straight back into start()/stop()/request_stop()/connect_to(). It flips to STOPPED and clears the flag only at the very end, so calling start() from inside on_stopped() works.

Ordering. The roles' clear callbacks queued by cleanup_connection_state() (STREAM_END, the *_CLEARED events, the artwork and visualizer stream events) are drained before on_stopped() fires, since the header documents on_stopped() as the point where it is safe to destroy the client. That drain is factored out of loop() into drain_inbox_events() so finish_stop() can run it a second time.

Role threads. SyncTask and the artwork/visualizer drain threads gained a request/report split: request_stop() sets COMMAND_STOP only, and the thread sets TASK_STOPPED/THREAD_EXITED on exit for has_stopped(). loop() polls that, so finish_stop()'s joins are instant on the graceful path and only the deadline can force a bounded blocking join. Each stop() drains its buffers after the join so nothing survives into a restarted session.

Manager shutdown. ConnectionManager splits into begin_stop() (clear accepting_, goodbye every connected peer, leave the server up) and stop() (run begin_stop(), destroy the server, force-drop the remainder). While accepting_ is clear, on_new_connection() rejects a late arrival with a goodbye rather than admitting it to a nursery about to be drained, and loop()'s server-start block, promotion scan, and both hello sites are skipped.

Notes for reviewers

The three accepting_ gates in loop() all exist for one rule: a peer that was told SHUTDOWN gets nothing further. disconnect()'s connected-nursery branch sends a goodbye but, unlike release_nursery_entry(), does not call disable_message_dispatch() on it, because disconnect() is also the public per-connection API and disabling dispatch there would change its non-shutdown behavior. So a connected nursery peer stays parked with its dispatch live through the whole STOPPING window. Promoting it would hand it a client/state; arming or sending its hello would hand it a client/hello. Note initiate_hello() never sends inline, it arms a retry entry that a later tick's scan sends, so the hello case needs no transient failure to happen: any peer admitted on the tick before request_stop() reaches it.

STOP_GRACE_MS is budgeted above the sync task's IDLE_RECEIVE_TIMEOUT_MS (750 vs 500), cross-referenced in both files so the two cannot drift apart silently.

finish_stop() resets group_state_ and state_, and cleanup_connection_state() deliberately does not. The latter also runs on reconnects and handoffs, where carrying a group delta and the client state forward is intentional; only a full stop should discard them.

Tests

145 cases, up from 123 on main. Three new TUs, all compiled with -fno-access-control rather than adding a seam to shipped code: test_sync_task.cpp for the sync task's request/report split, test_connection_manager.cpp for the accepting_-gated scans and the stop deadline, and test_client_internal.cpp for the session-state reset and the start() rollback. Neither preconditon is reachable through a real host socket, because IXWebSocket's server-side disconnect is synchronous: the close event always lands immediately, so a peer cannot be staged as still-connected-after-goodbye, nor as never closing.

Open

Not verified on hardware. Everything here is host-only so far. The ESP paths that matter are the queued-goodbye flush through the grace window and the httpd session release on request_stop().
Pre-existing, deliberately not fixed here: SendspinClient::disconnect() is not covered by finish_stop()'s re-entrancy guard, and re-locks the non-recursive conn_ptr_mutex_ that drop_connection() holds while firing on_release_high_performance(). A listener that calls disconnect() from that callback self-deadlocks. main already has this via loop()'s connection-lost and handoff paths, so it wants its own issue rather than a fix buried in this branch.

start()/stop() replace start_server() (kept as a source-compatible alias): stop() sends a goodbye, closes every connection, stops the WebSocket server and role background threads, and resets session state so start() brings the whole stack back up cleanly on the same port. request_stop() is the non-blocking variant: goodbyes go out while the server stays up (so on ESP the queued httpd sends actually flush), role threads wind down concurrently, and loop() finishes the teardown once connections close and the sync task exits, capped by a fixed grace deadline. Completion is reported via SendspinClientListener::on_stopped() and observable through get_run_state(). Restart correctness fixes surfaced by review: - Artwork/visualizer start() clears the stale COMMAND_STOP left by stop(), which otherwise killed the fresh drain thread on its first flag check. - SyncTask::is_initialized() also requires the ring buffer, so a retry after a partial init failure re-runs init() instead of spawning a thread that dereferences a null ring buffer. - The sync thread clears TASK_RUNNING on exit (a mid-stream stop otherwise wedged the player's sync-idle gate and held back on_stream_end()), returns any borrowed ring entry, and stop() drains the ring so no stale codec header survives into the next session. - stop() resets group_state_ and the published client state; a peer arriving mid-teardown is rejected with a goodbye instead of being force-dropped without one. Tests: client-level stop/restart and request_stop cycles over live loopback peers, behavioral artwork/visualizer restart regressions, and SyncTask lifecycle regressions (the latter TU built with -fno-access-control to stage internal states without test seams).

Findings from the docs-sync, embedded-review, house-patterns, and test-standards review passes: - request_stop() completion now waits for every role thread to report exit, not just the sync task: the artwork/visualizer drain threads set a THREAD_EXITED flag (mirroring TASK_STOPPED) polled via has_stopped(), so finish_stop()'s joins are instant on the graceful path and only the grace deadline accepts a bounded blocking join. - Corrected the drain-timeout figure in loop()'s completion comment (artwork polls at 100 ms, not 50 ms) and cross-referenced STOP_GRACE_MS with the sync task's IDLE_RECEIVE_TIMEOUT_MS so the budget cannot drift silently. - Documented accepting_ as authoritative control state under its own heading rather than filing it with the lock-free hint atomics. - docs: added on_stopped() to the integration guide's listener example and SendspinRunState to its enums reference; updated internals.md's thread-lifecycle section (stop is no longer destructor-only, threads restart across cycles) and added a Manager Shutdown section covering begin_stop()/stop() and the admission gate. - tests: the ring-drain test now checks recovered free bytes (the chunks_waiting() count cannot see a leaked borrowed entry), the no-op stop/request_stop branches assert their postconditions, and new tests cover the async role-thread path end to end: SyncTask request_stop / has_thread_exited, artwork/visualizer request_stop + restart, a client cycle with a live player role, and the reject-with-goodbye admission gate for a peer arriving mid-teardown.

Six findings from a high-effort review of the two preceding commits: - The nursery promotion scan gated only on is_handshake_complete(), so during the STOPPING window a peer that begin_stop() had already sent a SHUTDOWN goodbye could still complete its handshake, be promoted to current, and receive a client/state message; it also held has_connections() true until the grace deadline. The scan now skips while accepting_ is clear, leaving such peers for stop()'s force-drain. - finish_stop() set run_state_ = STOPPED before tearing anything down, so a listener reached mid-teardown (cleanup_connection_state() fires on_release_high_performance() synchronously) could call start() and leave the client RUNNING with its role threads joined out from under it. A tearing_down_ guard now blocks re-entrant teardown, and the state stays STOPPING until the teardown completes. - on_stopped() fired while the roles' clear callbacks queued by cleanup_connection_state() were still undelivered, contradicting the header's "safe to destroy the client". The inbox/role drain is extracted into drain_inbox_events() and run once more before the notification. - VisualizerRole::Impl::stop() never drained its ring buffer, so entries queued before the stop survived into the next session and were decoded against the new session's format state. It now flushes after the join, matching SyncTask::stop(). - Corrected the comments claiming the start() flag clear guards the plain-stop path; the drain threads self-clear COMMAND_STOP on exit, so the clear is load-bearing only on the async path where stop() re-sets the bit after the thread is gone. - Replaced a fixed 400 ms sleep in a visualizer test with a poll. Adds regression tests for the promotion gate (new test_connection_manager TU), the on_stopped ordering, and the visualizer ring drain; each was mutation-verified against its fix.

Findings from a delegated review of the three lifecycle commits, each verified by an adversarial refuter before being acted on: - loop()'s hello scans gated only on nursery_size_, never on accepting_, so a peer that begin_stop() had already sent a SHUTDOWN goodbye could still be handed a client/hello. initiate_hello() never sends inline (it arms a retry entry that a later tick sends) and disconnect() leaves a connected nursery peer parked with that entry intact, so no transient send failure is needed: any peer admitted in the tick before request_stop() hits it. Both the connected-event arm and the retry scan now skip while accepting_ is clear, matching the promotion scan. - Corrected the promotion-gate rationale: it claimed promoting a parked peer "would keep has_connections() true and stall stop()'s deadline wait", but has_connections() counts nursery entries as well as the current slot, so parking stalls the deadline identically. The guard is right for the client/state reason alone. internals.md's wording is accurate and is left alone. - Fixed request_stop()'s doc comment naming run_state(); the accessor is get_run_state(). Closes three coverage gaps, each mutation-verified to fail without the production line it guards: - The STOP_GRACE_MS deadline disjunct in loop()'s completion gate, which is all that keeps a peer that never delivers a close from stranding the client in STOPPING with on_stopped() never firing. Driven white-box because a real host socket cannot reproduce it: IXWebSocket's server-side disconnect is synchronous, so a real peer's close event always lands immediately. - finish_stop()'s reset of group_state_ and state_, which keeps a restart from republishing the previous session's group deltas or a stale ERROR state to the next server. - start()'s rollback of already-started role threads when a later role fails to start, staged by sabotaging the visualizer's drain task after the player's sync task thread is up. The two client-side tests live in a new test_client_lifecycle_internal TU: group_state_ has no public setter and state_ no public getter, and no role's start() can be made to fail through the public API on host, so both need -fno-access-control rather than a seam in the production code. Each test carries a control case asserting the state is genuinely dirty (or the gate genuinely open) going in.

Follow-ups from the docs-sync, embedded-review, house-patterns, and test-standards passes over 2e91f90: - The connected-event hello arm (the outbound half of the accepting_ gate) had no coverage: deleting that guard left all 144 tests green. Adds ConnectedEventDoesNotArmHelloWhileStopping, which stages a queued transport-connected event for a nursery peer during the STOPPING window and asserts no hello is armed or sent, with the accepting_ restored case as the control. Mutation-verified against its guard. - internals.md's Manager Shutdown section documented the accepting_ gate only for the promotion scan; it now also covers the two hello sites and why arming is deferred rather than sent inline. Also aligned its has_connections() aside with the corrected comment in the source: the gate is right for the client/state reason alone, and parking a peer stalls the teardown deadline exactly as promoting it would. - Broadened test_connection_manager.cpp's header comment, which still described the file as covering the promotion scan alone. - Renamed test_client_lifecycle_internal.cpp to test_client_internal.cpp to match the subject-named convention of the other access-control TUs and to stop it reading as a sibling of test_connection_lifecycle.cpp. The embedded pass found nothing to change: both new accepting_ loads sit inside sections already gated by a hint atomic and already holding conn_ptr_mutex_, so an idle tick pays for neither, and skipping the retry scan does not extend any connection's lifetime (the close-event path is ungated and stop()'s force-drain bounds the rest).

Copilot

🟡 Changes recommended

The asynchronous path can block and contains ESP shutdown races that can lose goodbyes or process traffic after shutdown begins.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds restartable lifecycle management to SendspinClient, including synchronous and deferred shutdown.

Changes:

Adds start(), stop(), request_stop(), lifecycle state, and completion notification.
Makes role threads restartable and introduces graceful connection-manager shutdown.
Updates tests, examples, and lifecycle documentation.

File summaries

File	Description
`include/sendspin/client.h`	Defines the lifecycle API and state.
`include/sendspin/config.h`	Updates startup documentation.
`src/client.cpp`	Implements lifecycle orchestration.
`src/connection_manager.h`	Declares manager shutdown support.
`src/connection_manager.cpp`	Implements admission gating and teardown.
`src/player_role.cpp`	Adds player thread lifecycle forwarding.
`src/player_role_impl.h`	Declares player lifecycle methods.
`src/sync_task.cpp`	Makes the sync thread restartable.
`src/sync_task.h`	Exposes sync-thread lifecycle state.
`src/artwork_role.cpp`	Adds asynchronous artwork-thread stopping.
`src/artwork_role_impl.h`	Declares artwork lifecycle methods.
`src/visualizer_role.cpp`	Adds restart and buffer cleanup behavior.
`src/visualizer_role_impl.h`	Declares visualizer lifecycle methods.
`tests/CMakeLists.txt`	Registers lifecycle test suites.
`tests/test_client_internal.cpp`	Tests rollback and state reset.
`tests/test_connection_lifecycle.cpp`	Tests public lifecycle behavior.
`tests/test_connection_manager.cpp`	Tests shutdown admission and deadlines.
`tests/test_sync_task.cpp`	Tests sync-task restart behavior.
`tests/test_artwork_role.cpp`	Tests artwork-thread restart behavior.
`tests/test_visualizer_role.cpp`	Tests visualizer restart and draining.
`examples/basic_client/main.cpp`	Uses the new lifecycle API.
`examples/tui_client/main.cpp`	Uses the new lifecycle API.
`docs/internals.md`	Documents lifecycle internals.
`docs/integration-guide.md`	Documents consumer lifecycle usage.

Review details

Suppressed comments (1)

src/client.cpp:684

These joins are signaled one at a time, so an idle client with all threaded roles can wait roughly 500 ms for player, then 50 ms for visualizer, then 100 ms for artwork. That unnecessarily serializes main-loop shutdown and exceeds the documented 500 ms idle bound. Signal every role first, then perform the joins so the receive timeouts elapse concurrently.

void SendspinClient::stop_role_threads() {
#ifdef SENDSPIN_ENABLE_PLAYER

Files reviewed: 24/24 changed files
Comments generated: 13
Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+    // Goodbyes go out now (queued to httpd workers on ESP), with the server left up so they can
+    // flush; role threads start winding down concurrently. loop() finishes the teardown once
+    // every connection has closed and the sync task has exited, or at the deadline.
+    this->connection_manager_->begin_stop(SendspinGoodbyeReason::SHUTDOWN);


            // Never managed, but its callbacks are already wired: block dispatch so it cannot
            // inject messages during the goodbye window.
            conn->disable_message_dispatch();
+            this->queue_deferred_release(std::move(conn), SendspinGoodbyeReason::SHUTDOWN);


+/// Grace deadline for a request_stop() teardown: covers the sync task's 500 ms idle poll
+/// (IDLE_RECEIVE_TIMEOUT_MS in sync_task.cpp; keep the budget above it) plus margin for goodbye
+/// sends to flush, close events to arrive, and the artwork/visualizer drain receive timeouts
+/// (100 ms / 50 ms). loop() force-finishes the stop once it expires, so a peer that ignores its
+/// goodbye cannot hold the teardown open.
+static constexpr int64_t STOP_GRACE_MS = 750;
+static constexpr int64_t STOP_GRACE_US = STOP_GRACE_MS * 1000;


…rity rules (#111) Two changes to `.claude/skills/test-standards/SKILL.md`. No test or production code is touched. ## Do not recommend a production clock seam "Honest gaps" named an injectable clock as its example of a gap that cannot be closed cheaply. That reads as a remedy to recommend, and it contradicts "No test seams in production" stated earlier in the same file. A review of #110 followed it and asked for a clock seam this project deliberately does not have. Naming the gap is now the finding, and an extracted pure predicate is the way to cover a timing decision directly. The same commit separates a sleep that advances wall-clock time toward a deadline under test from a sleep used as synchronization, and requires the margin be computed before a timing test is called fragile. ## Granularity and independence The checklist judged whether an individual test was strong but said nothing about how the suite is carved up. A reviewer could not flag a long test covering several behaviors, or a test that depends on another having run first. The new section covers one behavior per test, test naming, `ASSERT_*` aborting where `EXPECT_*` continues, and test independence. It cites `MetadataNullClearsAndAbsentPreserves` in `tests/test_protocol.cpp` as the example of several assertions belonging to one behavior. A bullet recommending value-parameterized tests was written and then dropped: it pointed at the malformed-input tests in `tests/test_protocol.cpp` as copy-pasted near-duplicates, and they are not. Those cases each cover a distinct rule while sharing the control-plus-rejection template this file already prescribes. The suite has no instance of the pattern that rule described. `pre-commit run --all-files` passes. Frontmatter is unchanged.

begin_stop() goodbyed every connected peer but left inbound dispatch enabled, so a peer already told SHUTDOWN could keep pushing stream and role data into the client for the rest of the grace window. Because a nursery entry is released without cleanup_connection_state(), that state outlived the teardown whenever no connection had been promoted, which is exactly the restart leak this branch exists to prevent. disconnect() now takes a quiescing flag: begin_stop() disables dispatch before each goodbye, while the public per-connection disconnect() keeps its current behavior, having no teardown window to protect. loop() also ran the time-sync burst before the STOPPING check, so a client/time frame could still go out after the goodbye. Gate the burst on RUNNING. Only the burst: the manager loop and the completion check must keep running to finish the stop.

stop_role_threads() called each role's stop() in turn, and each of those both signals and joins, so an idle client waited out the sum of the three receive timeouts (about 650 ms) rather than the longest one. Signal every role first, as the request_stop() path already does. The docs were wrong in three places. A synchronous stop() does pass through STOPPING: finish_stop() forces it for the duration of either teardown path, and callbacks reached in that window observe it. The roles' clear callbacks are drained inside finish_stop(), not on the next loop(). And the grace deadline bounds when the final teardown starts, not how long it takes, since the tick that reaches it joins the role threads and a listener callback still running holds that join. request_stop() is also documented as blocking on an outbound connect_to() connection. Its transport stop cannot be deferred: the manager releases its last reference as soon as the disconnect returns, so the transport thread has to be joined before that happens, and the host implementation now says so where someone would otherwise be tempted to make it asynchronous.

The header documents calling start() from on_stopped(), and it works only because finish_stop() assigns STOPPED before invoking the callback; start() refuses while STOPPING. Nothing exercised it. Add a listener that restarts from the callback and checks the client is genuinely live afterward, then that a second teardown still completes. Also assert that the tick after request_stop() leaves the client STOPPING with a player role attached. That is the property request_stop() exists to provide, and the end-state assertion alone held whether or not loop() deferred the teardown.

kahrendt · 2026-08-31T17:07:36Z

Closing this for now. This was a decent test to see if the new review skills are helpful (they are, but some need tweaks even after #111). There are some bigger structural changes I want to make that will need ot take place outside of this PR. I will revisit it after those land.

kahrendt added 5 commits August 31, 2026 08:35

kahrendt requested a balanced review from Copilot August 31, 2026 15:05

Copilot started reviewing on behalf of kahrendt August 31, 2026 15:06 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

kahrendt mentioned this pull request Aug 31, 2026

Fix a contradiction in the test-standards skill and add suite granularity rules #111

Merged

kahrendt added 3 commits August 31, 2026 12:40

kahrendt closed this Aug 31, 2026

Navigation Menu

Uh oh!

Add a start/stop/restart client lifecycle - #110

Add a start/stop/restart client lifecycle#110
kahrendt wants to merge 8 commits into
mainfrom
client-lifecycle

kahrendt commented Aug 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kahrendt commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kahrendt commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

How

Notes for reviewers

Tests

Open

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kahrendt commented Aug 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kahrendt commented Aug 31, 2026 •

edited

Loading