Document that callbacks never call start() or stop()

.claude/skills/house-patterns/SKILL.md

@@ -69,8 +69,9 @@ the descriptions in `docs/internals.md`:
   `InboxSlot`; two non-main-loop threads communicate via `ShadowSlot`.
 - Payload validation and decoding on the consuming thread, never the
   transport thread.
-- Callback dispatch tolerates re-entrant teardown (a listener may call back
-  into the client mid-callback).
+- No callback calls `loop()`, `start()` or `stop()`, so callback dispatch
+  carries no re-entrancy guard; a new guard for a callback that does is a
+  finding.
 - Silent drops on bounded queues are findings; drop sites log a warning.
 
 ## Validation posture

docs/conventions.md

@@ -77,10 +77,10 @@ checklists in `.claude/skills/` apply these standards to a diff.
   than relying on the ring or the slot being reset: a teardown that leaves
   other producers running cannot reset it, and a drain can take a slot on
   either side of a teardown.
-- Callback dispatch must tolerate re-entrant teardown: a listener callback may
-  call back into the client, except `loop()` (see `client.h`). See "Re-entrant
-  Teardown During Callback Dispatch" in `docs/internals.md` for the guard
-  patterns in use.
+- No callback calls `loop()`, `start()` or `stop()` (see `client.h`), so no
+  teardown runs on the main loop beneath the drain that fired a callback, and
+  callback dispatch carries no re-entrancy guard. See "Callbacks During
+  Dispatch" in `docs/internals.md`.
 - A message handler on the protocol task does the connection work the message
   calls for in place (an activation's admissibility check, admission and role
   ownership, a pairing step, a `noise/handshake` re-handshake, the RAM commit

docs/integration-guide.md

@@ -829,7 +829,7 @@ Restarting is `start()` again; start, stop, and start again can be repeated inde
 
 A pairing attempt in flight is cut short the same way: `on_clear_pairing_code()` and `on_close_pairing_window()` fire from inside `stop()` for a prompt that was still showing, and every provider write still owed (a long-term record a `server/pair-finalize` had just committed, for example) is performed before `stop()` returns. The identity and record store survive the stop, so a restarted client keeps its `client_id`, its pairing token, and every record.
 
-Listener callbacks fire from inside `stop()`, after every role and the group state have been reset, so a callback that reads the client through its getters sees the stopped state. One that calls `start()` gets `false` and starts nothing; one that calls `stop()`, `connect_to()`, or `disconnect()` is ignored. `is_started()` reads `false` throughout and is safe to call from any thread. Call `stop()` only from the main loop thread: from a role-thread callback it would join the calling thread.
+Listener callbacks fire from inside `stop()`, after every role and the group state have been reset, so a callback that reads the client through its getters sees the stopped state; one that calls `connect_to()` or `disconnect()` is ignored. `is_started()` reads `false` throughout and is safe to call from any thread. Call `start()` and `stop()` only from the main loop thread, and never from a callback (see [Thread Safety Summary](#thread-safety-summary)).
 
 `on_request_high_performance()` and `on_release_high_performance()` fire from the main loop with no internal lock held, and their bodies should only toggle the platform networking mode rather than calling back into the client or a role. A time burst waits for the request to have been delivered before it sends its first time message, so a main loop that stalls delays the next clock measurement rather than measuring it in power-save mode; a release is never delayed that way.
 
@@ -1217,7 +1217,7 @@ The view forms hand the metadata, group, pairing and server text over as `std::s
 | `ServerInformationObject` | `ServerInformation` |
 | `SendspinClient::get_server_information()` | `SendspinClient::server_information()`, main loop only |
 
-The fields and arguments keep their meaning; only the text type changes. A view handed to a callback is valid for the duration of the callback, and the views `group_state()` and `server_information()` return until control next returns to `loop()` (from inside a callback: until the callback returns) or until `stop()`: copy what is needed beyond that. A view, like a pairing token's array, is not guaranteed to be NUL-terminated: print it with `%.*s` (its size, then its data) or copy it into a `std::string` before passing it to an API that expects a C string. `server_information()` reads a copy `loop()` refreshes as the server changes, so it is main loop only and can read the previous server, or `nullopt`, for one `loop()` after `is_connected()` turns true: read it on each `loop()` rather than only when `is_connected()` rises; `get_server_information()` reads from any thread, and code off the main loop has no replacement and keeps calling it until the removal. Until the removal the library calls the new callback and then the deprecated one with the same content, so a listener overrides one of the two; one that overrides both gets both, the new one first. A `stop()` from the new callback ends the delivery there.
+The fields and arguments keep their meaning; only the text type changes. A view handed to a callback is valid for the duration of the callback, and the views `group_state()` and `server_information()` return until control next returns to `loop()` (from inside a callback: until the callback returns) or until `stop()`: copy what is needed beyond that. A view, like a pairing token's array, is not guaranteed to be NUL-terminated: print it with `%.*s` (its size, then its data) or copy it into a `std::string` before passing it to an API that expects a C string. `server_information()` reads a copy `loop()` refreshes as the server changes, so it is main loop only and can read the previous server, or `nullopt`, for one `loop()` after `is_connected()` turns true: read it on each `loop()` rather than only when `is_connected()` rises; `get_server_information()` reads from any thread, and code off the main loop has no replacement and keeps calling it until the removal. Until the removal the library calls the new callback and then the deprecated one with the same content, so a listener overrides one of the two; one that overrides both gets both, the new one first.
 
 ## Thread Safety Summary
 
@@ -1236,7 +1236,7 @@ Most listener callbacks fire on the main loop thread (the thread calling `client
 | `SendspinClientConfig::message_trace` (built with `SENDSPIN_ENABLE_MESSAGE_TRACE`) | Protocol task |
 | All other listener methods | Main loop thread |
 
-Never call `loop()` from a listener, role or provider callback. A callback on the main loop already runs inside one of the client's own calls (`loop()`, `start()`, `stop()` or its destructor), and `loop()` does not support being re-entered; a callback the table lists on another thread is off the main loop, where `loop()` never runs.
+Never call `loop()`, `start()` or `stop()` from a listener, role or provider callback. A callback on the main loop already runs inside one of the client's own calls (`loop()`, `start()`, `stop()` or its destructor), none of which supports being re-entered; a callback the table lists on another thread is off the main loop, where these calls never run. To stop or restart the client in response to a callback, record the request in the callback and act on it from the main loop between two `loop()` calls.
 
 `PlayerRole::notify_audio_played()` is thread-safe and is designed to be called from an audio output callback thread.
 

docs/internals.md

@@ -200,7 +200,7 @@ PLAYER_STREAM ring events → on_stream_ring_event() → awaiting_sync_idle_even
            │    Sync task idle → fire on_stream_end() if a stream is open, continue
            │
            └─ STREAM_START:
-                Take stream_params_slot (applied if its stamp is current)
+                Take stream_params_slot only if stamped with the START's generation
                 Mark stream active, fire on_stream_start()
                 Acknowledge the event's ordinal to the sync task
 ```
@@ -227,14 +227,9 @@ When a connection leaves the admitted array, `detach_inbound()` runs first. It d
 
 The source's `cleanup()` closes the stream gate without a message and clears the authorization; it recalls nothing, since the source holds no inbound ring item. The player's, the visualizer's and the artwork role's `cleanup()` also return what the sync task, the visualizer drain thread or the artwork decode thread has not taken, right after the generation bump and on the thread that hands the items over (`InboundConsumer::recall()`), so every item recalled carries an older stamp; a consumer that takes such an item between the bump and the recall returns it, by its stamp, without acting on it. The visualizer's `cleanup()` also moves its boundary sequence on (see the `stream/end` row under [Dispatch](#dispatch-protocol-task)), so its drain thread returns a frame it holds for its delivery time. The artwork decode thread, woken by its `cleanup()`, also drops the images it assembles or parks once it sees the generation move on. A role removed by a `server/activate` takes the same path. In `stop()` the role threads are joined and their lists returned and unbound (`InboundConsumer::unbind()`) before the main loop runs the roles' `cleanup()`, whose recall then finds no ring bound.
 
-### Re-entrant Teardown During Callback Dispatch
+### Callbacks During Dispatch
 
-Requests a listener callback makes (`connect_to()`, `disconnect()`, ...) are queued or posted to the protocol task, so no callback tears a connection down under the drain that fired it. `stop()` is the exception: called from a callback, it joins the protocol task, tears every role down on the main loop and runs a drain of its own beneath the one that fired the callback, and a `start()` that follows it in the same callback begins a new run. No other drain runs beneath one, since a callback never calls `loop()` (`client.h`). Two generation counters keep the outer drain from acting on what it already took:
-
-- `drain_generation` (on the client's `EventState`) is bumped by `stop()` before its teardown. The outer drain snapshots it once and stops as soon as it changes: the event ring between events, the pairing-note dispatch between notes, and the role drains and the group update before each starts.
-- Each role's `cleanup_generation` is bumped by its `cleanup()` and stamped onto every event and slot payload it queues afterwards; the ring drain drops stream events whose stamp no longer matches (`event_is_current()`). Inside a role drain the same counter catches a callback that re-entered teardown: the player checks it after each server-command callback and around each `on_stream_start()` and `on_stream_end()`, metadata and color by ending their drain once the teardown's catch-up has forgotten the pending deadline (`ScheduledStateSlot::pending_at()`), and artwork after each display or clear, and each abandons the rest of its work when it moved on.
-
-A state handed to the listener in two forms, the view and the deprecated `std::string` copy, is not handed over in the second once the first re-entered teardown: the group update checks the drain generation between `on_group_state()` and `on_group_update()`, the pairing-note dispatch between each text-carrying callback and its deprecated form (`on_pairing_attempt()` and `on_pairing_started()`, ...), and the metadata role checks between `on_track_metadata()` and `on_metadata()` whether its `TeardownTracker` completed a teardown, which on the main loop only `stop()` can do inside a callback.
+No listener, role or provider callback calls `loop()`, `start()` or `stop()` (`client.h`), and the requests a callback may make (`connect_to()`, `disconnect()`, a controller command, ...) are queued or posted to the protocol task. So no drain runs beneath another, and no role's main-loop half runs while one of its callbacks is on the stack: each drain delivers everything it took. A teardown the protocol task runs during a callback is caught up, through the stamps described under [Cleanup](#cleanup), the next time a drain reaches that role, later in the same drain or in the next one, always after the callback returns. A payload a drain takes after one of its callbacks has run is taken only under its own stamp, so one a later teardown's next connection wrote meanwhile stays pending: the player takes its stream parameters with the STREAM_START's stamp (`GenerationSlot::take_stamped()`).
 
 ## Message Flow
 
@@ -415,9 +410,8 @@ Each getter reads the slot once, so a caller that checks `is_time_synced()` and
 `SendspinClient::stop()` is synchronous and ordered so that every producer is gone before any state is reset:
 
 ```api
-0. lifecycle_ = STOPPING (loop() becomes a no-op; start/stop and every request that queues a
-   command or posts a request are refused or ignored, so a callback fired below cannot recurse
-   into the teardown)
+0. lifecycle_ = STOPPING (loop() becomes a no-op, and every request that queues a command or
+   posts a request is ignored, from another thread or from a callback fired below)
 1. Signal the visualizer and artwork threads to stop, without joining. The player is not
    signalled yet: it keeps returning the ring items it plays, so a transport waiting for ring
    space is not parked behind a stopped consumer
@@ -434,9 +428,8 @@ Each getter reads the slot once, so a caller that checks `is_time_synced()` and
    connecting stays parked); then drop any command still queued and any request still posted
 5. Join all role threads; each then returns its items to the ring, and the emptied inbound ring
    is released; the source destroys its outbound ring, every item back since step 4
-6. Bump drain_generation, cleanup_connection_state() for every role (the protocol-task halves
-   run here, on the main loop), then queue the pairing-UI dismissals the step 3 snapshot calls
-   for
+6. cleanup_connection_state() for every role (the protocol-task halves run here, on the main
+   loop), then queue the pairing-UI dismissals the step 3 snapshot calls for
 7. drain_inbox() runs each role's main-loop half (its clear; the player's on_stream_end(), the
    source's on_streaming_stopped()), the high-performance releases the shutdown pass queued,
    the owed provider writes, and the pairing notes step 6 queued, each exactly once: a dismissal step 6 queues beside one an
@@ -472,7 +465,6 @@ A metadata state's text, only the fields `MetadataRoleConfig` keeps, each cut to
 | What | When |
 |------|------|
 | The pairing notes for the main loop beyond the room both of their buffers keep | A batch of more than `PAIRING_NOTE_CAPACITY` notes between two drains, on the protocol task; the grown room is kept |
-| Another drain buffer for the pairing notes, with room for `PAIRING_NOTE_CAPACITY` | A pairing listener callback that calls `stop()` while a prompt shows: the drain `stop()` runs finds the buffer lent to the dispatch that called it, and reserves one before it takes the dismissals, on the main loop |
 | The player's stream events held for the sync task's idle | Events beyond the list's kept capacity, on the main loop |
 
 The `std::function`s built after `start()` allocate nothing because each capture is one pointer, which fits both standard libraries' small buffer: the Noise transport's frame sinks, set as each connection is constructed; the frame write hook a time send builds, which ESP copies into each send; and the handshake's send function, built for each handshake frame. A change to one of these captures is checked against that buffer, since a callable that does not fit it allocates on every copy.

include/sendspin/client.h

@@ -363,7 +363,8 @@ class SendspinClient {
     /// start, the roles that did start are stopped again so a corrected retry begins from the
     /// stopped state. A rejected configured pairing secret (SendspinClientConfig::pairing_psk or
     /// static_pairing_code) fails every start() of this instance, since the config is fixed at
-    /// construction. Main-loop thread only.
+    /// construction. Main-loop thread only, and never from a listener, role or provider callback
+    /// (docs/integration-guide.md, Thread Safety Summary).
     /// @return true if the client is running (including when it already was), false on failure
     bool start();
 
@@ -386,10 +387,9 @@ class SendspinClient {
     /// bounds are described in docs/integration-guide.md (Stopping and Restarting).
     ///
     /// Listener callbacks fire from inside this call, after every role has been reset, so the
-    /// state they observe through the getters is the stopped state. One that calls start() has
-    /// no effect and returns false; one that calls stop(), connect_to(), or disconnect() is
-    /// ignored. Main-loop thread only: calling it from a role-thread callback would join the
-    /// calling thread.
+    /// state they observe through the getters is the stopped state; one that calls connect_to()
+    /// or disconnect() is ignored. Main-loop thread only, and never from a listener, role or
+    /// provider callback (docs/integration-guide.md, Thread Safety Summary).
     void stop();
 
     /// @brief Returns true between a successful start() and stop()
@@ -857,8 +857,7 @@ class SendspinClient {
     /// @brief Drains the inbox: high-performance requests, the time-sync report, lifecycle events
     /// (each role's teardown half caught up ahead of its stamped events), provider writes,
     /// pairing notes, role slots, and group updates, dispatching listener callbacks on the
-    /// calling (main-loop) thread. Shared by loop() and stop(). A callback that calls stop()
-    /// abandons the rest of this drain (EventState::drain_generation).
+    /// calling (main-loop) thread. Shared by loop() and stop().
     void drain_inbox();
 
     /// @brief Applies the high-performance edges the protocol task queued: every acquire, then
@@ -1169,8 +1168,8 @@ class SendspinClient {
     std::atomic<bool> network_ready_{false};
     /// Where the client is in its lifecycle. Written only by start()/stop() on the main loop;
     /// atomic so is_started() can be read from any thread. STOPPING covers the whole of stop():
-    /// start() is refused and stop()/connect_to()/disconnect() are ignored while it is set, so a
-    /// listener callback fired from inside the teardown cannot recurse into it.
+    /// is_started() reads false, loop() is a no-op, and connect_to(), disconnect() and every
+    /// request that queues a command are ignored while it is set.
     enum class LifecycleState : uint8_t { STOPPED, RUNNING, STOPPING };
     std::atomic<LifecycleState> lifecycle_{LifecycleState::STOPPED};
 };

src/artwork_role.cpp

@@ -644,11 +644,8 @@ void ArtworkRole::Impl::drain_events() {
     // before this call, so it has already cleared held_display_mask before we get here.
     ArtworkDisplayUpdate update{};
     bool have_update = false;
-    const uint32_t generation = take_current_payload(*this, this->event_state->display_slot, update,
-                                                     have_update, TAG, "artwork displays");
-    if (!this->accepts(generation)) {
-        return;
-    }
+    take_current_payload(*this, this->event_state->display_slot, update, have_update, TAG,
+                         "artwork displays");
     if (have_update) {
         for (uint8_t slot = 0; slot < ARTWORK_MAX_SLOTS; ++slot) {
             const uint8_t bit = static_cast<uint8_t>(1U << slot);
@@ -739,11 +736,6 @@ void ArtworkRole::Impl::drain_events() {
                 this->listener->on_image_display(slot, display_lateness_ms(client_ts, overdue_us));
             }
         }
-        // The callback may re-enter teardown (a listener calling stop()), whose own drain already
-        // dropped the holds.
-        if (!this->accepts(generation)) {
-            return;
-        }
     }
 }