diff --git a/CLAUDE.md b/CLAUDE.md index 179bd502..51a7a481 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -95,7 +95,7 @@ Core source files in `src/` have no `#ifdef ESP_PLATFORM` guards; all platform d - **ESP-IDF**: Used as an IDF component via `idf_component.yml`. Sources defined in `cmake/sources.cmake`. - **Host (CMake)**: `cmake -B build && cmake --build build`. Uses ccache when installed (`SENDSPIN_USE_CCACHE`, default on when standalone). Fetches dependencies via FetchContent: ArduinoJson, noise-c, IXWebSocket, micro-flac for the player or source role, and micro-opus for the player or source role when `SENDSPIN_ENABLE_OPUS` is on. -- **Tests**: `cmake -B build-tests -DSENDSPIN_BUILD_TESTS=ON -DENABLE_SANITIZERS=ON -DBUILD_EXAMPLES=OFF .`, then `cmake --build build-tests --target sendspin_tests` and `ctest --test-dir build-tests --output-on-failure`. The target also builds `sendspin_alloc_tests`, which counts the library's `operator new` calls from accept to admission, from `connect_to()` to admission, for a `client/state`, for a time burst, for a controller `server/state`, for the `stream/*` messages, for the sync task's first FLAC codec header, for a FLAC `stream/start` the player takes, for a `stream/start` section the connection's roles do not take, for the first spectrum frame, and through the record flush that follows each of a playback handoff to another paired server, a `server/unpair`, a `pairing_psk` pairing and a dynamic pairing code pairing in each emission format; `SENDSPIN_ALLOC_REPORT=1` prints them grouped by call site. +- **Tests**: `cmake -B build-tests -DSENDSPIN_BUILD_TESTS=ON -DENABLE_SANITIZERS=ON -DBUILD_EXAMPLES=OFF .`, then `cmake --build build-tests --target sendspin_tests` and `ctest --test-dir build-tests --output-on-failure`. The target also builds `sendspin_alloc_tests`, which counts the library's `operator new` calls from accept to admission, from `connect_to()` to admission, for a `client/state`, for a time burst, for a controller `server/state`, for `group/update`, for the `stream/*` messages, for the sync task's first FLAC codec header, for a FLAC `stream/start` the player takes, for a `stream/start` section the connection's roles do not take, for the first spectrum frame, and through the record flush that follows each of a playback handoff to another paired server, a `server/unpair`, a `pairing_psk` pairing and a dynamic pairing code pairing in each emission format; `SENDSPIN_ALLOC_REPORT=1` prints them grouped by call site. - **ThreadSanitizer tests**: `cmake -B build-tsan -DSENDSPIN_BUILD_TESTS=ON -DENABLE_TSAN=ON -DBUILD_EXAMPLES=OFF .`, then `cmake --build build-tsan --target sendspin_tests` and `TSAN_OPTIONS=halt_on_error=1 ctest --test-dir build-tsan --output-on-failure`. `ENABLE_TSAN` applies the thread sanitizer to every target, including the fetched dependencies, and cannot be combined with `ENABLE_SANITIZERS`. - **Message trace**: `-DSENDSPIN_ENABLE_MESSAGE_TRACE=ON` compiles in `SendspinClientConfig::message_trace` and `tests/test_message_trace.cpp`; CI's test and TSan jobs build it on, the build and no-opus jobs off. - **Stack budgets**: the ESP task stack defaults in `config.h` are static call-graph bounds from `tools/stack_usage/` (recipe and per-task figures in its README); re-derive them, and update the README table with them, after a change that deepens a task's call chain. diff --git a/docs/integration-guide.md b/docs/integration-guide.md index a80d0733..073e6440 100644 --- a/docs/integration-guide.md +++ b/docs/integration-guide.md @@ -681,7 +681,10 @@ Receives client-level events. All callbacks fire on the main loop thread. ```cpp struct MyClientListener : SendspinClientListener { - // Called when group state changes (playback state, group name, etc.) + // Called when group state changes (playback state, group name, etc.). An empty group_id + // means the group was cleared. A non-empty server group_id that is not 43 base64url + // characters arrives as base64url of its SHA-256, which still names one group; a future + // release closes the connection on such an id instead. void on_group_update(const GroupUpdateObject& group) override { if (group.playback_state) update_playback_indicator(*group.playback_state); if (group.group_name) update_group_display(*group.group_name); diff --git a/docs/internals.md b/docs/internals.md index b72d72f4..11ce4360 100644 --- a/docs/internals.md +++ b/docs/internals.md @@ -281,7 +281,7 @@ Role-bound traffic reaches a role only from the admitted connection that owns it | `server/time` | Claims the connection's `client/time` frame in flight, dropping a reply that answers none, and feeds the measurement to the connection's own time burst | | `server/state` | Writes the controller, metadata, and color `InboxSlot`s | | `server/command` | Merges the player object into the player's `command_slot`; the source object starts or ends the input stream (see [Source Stream Start and End](#source-stream-start-and-end)) | -| `group/update` | Merges into `group_slot`, from the primary admitted connection only | +| `group/update` | Merges into `group_slot`, from the primary admitted connection only; a `group_id` that is not 43 base64url characters is normalized, or closes the connection when its hash state cannot be allocated; with `GROUP_ID_STRICT`, one that is absent or not in that format closes the connection (`group_id_from_wire()`) | | `stream/start` | Player writes the codec header, numbered with the stream's ordinal, into an item it acquires from the inbound ring (a bounded wait) and appends it to the sync task's list, then writes its params slot and pushes STREAM_START; visualizer moves its boundary sequence on (below), writes its config slot, and pushes STREAM_START; artwork marks its stream active, discards the pending image of each channel whose configuration changed, and appends a marker naming those channels to its decode thread's list | | `stream/end` | Pushes STREAM_END events for each streaming role, signals the sync task with the ordinal the stream ended and releases its listed audio from the player's quota, recalls the visualizer's listed frames and moves its boundary sequence on (`VisualizerRole::Impl::signal_boundary()`; every stream command and `cleanup()` do both, and the drain thread returns the frame it holds), and appends a marker for every channel to the artwork decode thread's list | | `stream/clear` | Visualizer moves its boundary sequence on (see `stream/end`) and pushes STREAM_CLEAR; player signals the sync task, releases its listed audio from the player's quota and appends a clear marker to its list | @@ -456,15 +456,15 @@ On host, IXWebSocket callbacks hold a `weak_ptr` to the connection the manager a ## Memory -The library allocates from heaps that fail differently. `operator new`, behind every `std::string`, `std::vector` and `std::make_shared` and any `std::function` whose capture outgrows its small buffer, draws on ESPHome only on internal RAM and aborts when it fails. The `platform_malloc` family (`src/platform/memory.h`, behind `PlatformBuffer` and the JSON arena's overflow) places each block by its `MemoryLocation`, and noise-c allocates its handshake, cipher and hash states with `calloc`; both report a failure, which the caller turns into a refused or closed connection or a dropped message. The exceptions are hashing a PSK into its `psk_id`, which aborts when noise-c cannot allocate the hash state, but only while `start()` loads the record store (`psk_id_for()` on a `std::array`), and deriving `SENTINEL_PSK` the same way at static initialization; a pairing that cannot hash its new PSK fails instead. Construction, `add_*()` and `start()` use any of them. After `start()`, the per-connection and per-message buffers come from the `platform_malloc` family: the connection object of an ESP accept and of every `connect_to()`, with its `shared_ptr` control block (`make_connection_in_block()`: `std::allocate_shared()` into a block reserved first, so a failure refuses the session or the connect), the JSON arena's overflow, the handshake's prologue and msg1 buffers, the fallback buffer, the Noise transport's reassembly and send buffers, and on ESP a send that does not fit the `FixedBlockPool`. The sync task allocates on first use from the same family: its decode buffer's storage at the first codec header (the `TransferBuffer` itself sits by value in the task's `SyncContext`), where a failure drops the stream, and the FLAC decoder, constructed in a `PlatformBuffer` at the first FLAC codec header and reset for each later FLAC stream, where a failure fails the header and the stream's chunks are discarded unplayed. micro-flac allocates the rest of its state with `FLAC_MALLOC`, which fails the same way; its own `operator new` allocations, an Ogg demuxer and stored metadata blocks, never run on the `fLaC` and last-STREAMINFO `codec_header` the parser admits. The visualizer's spectrum bins are reserved by `start()` for the configured `n_disp_bins` when the configuration requests the spectrum type, and a stream serving more bins than that drops its spectrum frames rather than growing them on the drain thread. `connect_to()` takes its URL by value and moves it through the request slot into the transport, so the library copies none of it. The host transport, whose IXWebSocket API copies each frame into a `std::string` and which makes a host accept's connection object with `std::make_shared`, and the host-only message trace sit outside this model: ESP replaces the first with esp_http_server and esp_websocket_client and builds without the second. +The library allocates from heaps that fail differently. `operator new`, behind every `std::string`, `std::vector` and `std::make_shared` and any `std::function` whose capture outgrows its small buffer, draws on ESPHome only on internal RAM and aborts when it fails. The `platform_malloc` family (`src/platform/memory.h`, behind `PlatformBuffer` and the JSON arena's overflow) places each block by its `MemoryLocation`, and noise-c allocates its handshake, cipher and hash states with `calloc`; both report a failure, which the caller turns into a refused or closed connection or a dropped message. Normalizing a `group_id` that is not 43 base64url characters (`group_id_from_wire()`, while `GROUP_ID_STRICT` is off) has noise-c `calloc` a SHA-256 state on the protocol task for every `group/update` from a server that sends such ids; when that allocation fails, the connection is closed, as for a non-conforming id under `GROUP_ID_STRICT`, rather than applying the rest of the update under the previous group's id. The exceptions are hashing a PSK into its `psk_id`, which aborts when noise-c cannot allocate the hash state, but only while `start()` loads the record store (`psk_id_for()` on a `std::array`), and deriving `SENTINEL_PSK` the same way at static initialization; a pairing that cannot hash its new PSK fails instead. Construction, `add_*()` and `start()` use any of them. After `start()`, the per-connection and per-message buffers come from the `platform_malloc` family: the connection object of an ESP accept and of every `connect_to()`, with its `shared_ptr` control block (`make_connection_in_block()`: `std::allocate_shared()` into a block reserved first, so a failure refuses the session or the connect), the JSON arena's overflow, the handshake's prologue and msg1 buffers, the fallback buffer, the Noise transport's reassembly and send buffers, and on ESP a send that does not fit the `FixedBlockPool`. The sync task allocates on first use from the same family: its decode buffer's storage at the first codec header (the `TransferBuffer` itself sits by value in the task's `SyncContext`), where a failure drops the stream, and the FLAC decoder, constructed in a `PlatformBuffer` at the first FLAC codec header and reset for each later FLAC stream, where a failure fails the header and the stream's chunks are discarded unplayed. micro-flac allocates the rest of its state with `FLAC_MALLOC`, which fails the same way; its own `operator new` allocations, an Ogg demuxer and stored metadata blocks, never run on the `fLaC` and last-STREAMINFO `codec_header` the parser admits. The visualizer's spectrum bins are reserved by `start()` for the configured `n_disp_bins` when the configuration requests the spectrum type, and a stream serving more bins than that drops its spectrum frames rather than growing them on the drain thread. `connect_to()` takes its URL by value and moves it through the request slot into the transport, so the library copies none of it. The host transport, whose IXWebSocket API copies each frame into a `std::string` and which makes a host accept's connection object with `std::make_shared`, and the host-only message trace sit outside this model: ESP replaces the first with esp_http_server and esp_websocket_client and builds without the second. -Bounded values stay inline: the ids and names a connection holds (`IdText`, `ServerName`), activity, player and controller command, visualizer type and source codec sets (`EnumSet`), role sets, `stream/start`'s artwork channels (`InlineVector`) and its player's FLAC codec header, decoded as it is parsed (`FlacCodecHeader`), the protocol task's slot tables and the ESP server's session slots and pending upgrades (`InlineVector`, `std::array`), each connection's `NoiseSession`, which owns the noise-c handshake and cipher states, and `NoiseHandshake`, a pairing attempt's CPace state, its PRS and the code it emits (`InlineText`), and the pairing records (`PairingRecord`, its ids `IdText`): the one a pairing holds pending on its connection and those the record store holds, reserved for `max_pairing_records` when `start()` builds the store. The store's mutators mark the slots a flush owes in a bitset, and the flush encodes one write at a time into a buffer on the main loop's stack. CPace hashes each length-value piece of its transcript as it encodes it rather than building the transcript first. The pairing notes for the main loop trade two buffers reserved at construction between the slot and the drain (`InboxSlot::exchange()`). Role sets take two shapes: ownership, admission and `client/state` intersect and complement them, so `ActiveRoles` and the admitted slots hold `uint16_t` role masks (`role_mask_bit()`, `ALL_ROLES_MASK`), while the roles a `stream/start`, `stream/end` or `stream/clear` covers are only filled and looked up, so they are an `EnumSet`. `client/state`'s artwork channels and visualizer types view the role's configuration, fixed from `add_*()`, which may be called only while the client is stopped (`client.h` "Role registration"), and `client/hello`'s application roles view the client's configuration as `start()` normalized it. A getter that returns `std::string`, such as the server's id and name, builds it from the inline value on the caller's thread. The main loop lists the controller's `supported_commands` and a visualizer stream's `types` from their sets into public vectors each role reserves for every member when it is built. +Bounded values stay inline: the ids and names a connection holds (`IdText`, `ServerName`), activity, player and controller command, visualizer type and source codec sets (`EnumSet`), role sets, `stream/start`'s artwork channels (`InlineVector`) and its player's FLAC codec header, decoded as it is parsed (`FlacCodecHeader`), the protocol task's slot tables and the ESP server's session slots and pending upgrades (`InlineVector`, `std::array`), each connection's `NoiseSession`, which owns the noise-c handshake and cipher states, and `NoiseHandshake`, a pairing attempt's CPace state, its PRS and the code it emits (`InlineText`), and the pairing records (`PairingRecord`, its ids `IdText`): the one a pairing holds pending on its connection and those the record store holds, reserved for `max_pairing_records` when `start()` builds the store. The store's mutators mark the slots a flush owes in a bitset, and the flush encodes one write at a time into a buffer on the main loop's stack. CPace hashes each length-value piece of its transcript as it encodes it rather than building the transcript first. The pairing notes for the main loop trade two buffers reserved at construction between the slot and the drain (`InboxSlot::exchange()`). Role sets take two shapes: ownership, admission and `client/state` intersect and complement them, so `ActiveRoles` and the admitted slots hold `uint16_t` role masks (`role_mask_bit()`, `ALL_ROLES_MASK`), while the roles a `stream/start`, `stream/end` or `stream/clear` covers are only filled and looked up, so they are an `EnumSet`. `client/state`'s artwork channels and visualizer types view the role's configuration, fixed from `add_*()`, which may be called only while the client is stopped (`client.h` "Role registration"), and `client/hello`'s application roles view the client's configuration as `start()` normalized it. A getter that returns `std::string`, such as the server's id and name, builds it from the inline value on the caller's thread. The main loop lists the controller's `supported_commands` and a visualizer stream's `types` from their sets into public vectors each role reserves for every member when it is built, and copies `group/update`'s `group_id` and `group_name`, parsed into `GroupUpdatePayload` (`IdText`, `ServerName`), into the public strings of the client's group state and of the delta `on_group_update()` is handed, each reserved for its bound when the client is built and swapped aside rather than freed when its field is cleared (`ReservedText`, `src/reserved_text.h`). `operator new` runs after `start()` at these places: | What | When | |------|------| -| Message fields held on the heap: track metadata, and `group/update`'s `group_id` and `group_name` | Each message that carries them, parsed on the protocol task; a role's fields are parsed only for a role the connection owns. `group/update`'s fields are copied again into the inbox slot's merged value on the protocol task and into the client's group state on the main loop | +| Message fields held on the heap: track metadata | Each message that carries them, parsed on the protocol task, only for a connection that owns the metadata role | | The listener's `std::string` arguments: a pairing's `server_id`, and a `qr_code` pairing code's token | Each pairing callback that takes one, on the main loop | | 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 | diff --git a/include/sendspin/client.h b/include/sendspin/client.h index 59f5c993..124fde13 100644 --- a/include/sendspin/client.h +++ b/include/sendspin/client.h @@ -257,6 +257,7 @@ class SendspinArenaAllocator; class SendspinConnection; struct ClientHelloMessage; struct ClientStateMessage; +struct GroupUpdatePayload; struct Identity; /** @@ -919,7 +920,7 @@ class SendspinClient { /// @brief Folds a group/update delta from the primary admitted connection into group_slot /// for the main loop. Protocol task only. - void merge_group_update(GroupUpdateObject&& delta); + void merge_group_update(const GroupUpdatePayload& delta); // ======================================== // Persistence & identity diff --git a/include/sendspin/types.h b/include/sendspin/types.h index 123522b6..e6324f6d 100644 --- a/include/sendspin/types.h +++ b/include/sendspin/types.h @@ -55,6 +55,8 @@ enum class SendspinPlaybackState : uint8_t { /// @brief Group membership and playback state delta received in group/update messages struct GroupUpdateObject { std::optional playback_state{}; + /// 43 base64url characters, or empty when cleared; a server id in another form is reported as + /// its SHA-256 in base64url std::optional group_id{}; /// At most 64 bytes in UTF-8, cut at a character boundary std::optional group_name{}; diff --git a/src/client.cpp b/src/client.cpp index d6339100..c680becc 100644 --- a/src/client.cpp +++ b/src/client.cpp @@ -31,6 +31,7 @@ #include "platform/network_info.h" #include "platform/time.h" #include "record_store.h" +#include "reserved_text.h" #include "teardown_tracker.h" #ifdef SENDSPIN_ENABLE_ARTWORK #include "artwork_role_impl.h" @@ -206,7 +207,14 @@ struct HighPerformanceRequests { /// @brief The main-loop-bound channels the client itself owns, beside the roles' own slots struct SendspinClient::EventState { Inbox inbox; - InboxSlot group_slot{inbox, INBOX_TOPIC_GROUP}; + InboxSlot group_slot{inbox, INBOX_TOPIC_GROUP}; + /// The delta on_group_update() is handed, filled in place from each group_slot payload. Main + /// loop only, like the text storage below that it and the client's group state fill from. + GroupUpdateObject group_delta; + ReservedText delta_group_id{PEER_ID_SIZE}; + ReservedText delta_group_name{MAX_FRIENDLY_NAME_BYTES}; + ReservedText state_group_id{PEER_ID_SIZE}; + ReservedText state_group_name{MAX_FRIENDLY_NAME_BYTES}; /// The filter error of the primary connection's latest completed time burst, for /// on_time_sync_updated(). Written by the protocol task, latest-wins. InboxSlot time_sync_slot{inbox, INBOX_TOPIC_TIME}; @@ -239,6 +247,31 @@ struct SendspinClient::EventState { this->pairing_slot.update( [¬e](std::vector& current) { current.push_back(note); }); } + + /// @brief Applies `payload` to the client's group state `state` and fills group_delta with + /// it, both in place. Main loop only. + void apply_group_update(GroupUpdateObject& state, const GroupUpdatePayload& payload) { + if (payload.playback_state.has_value()) { + state.playback_state = payload.playback_state; + } + if (payload.group_id.has_value()) { + this->state_group_id.fill(state.group_id, payload.group_id->view()); + } + if (payload.group_name.has_value()) { + this->state_group_name.fill(state.group_name, payload.group_name->view()); + } + this->group_delta.playback_state = payload.playback_state; + this->delta_group_id.fill(this->group_delta.group_id, optional_view(payload.group_id)); + this->delta_group_name.fill(this->group_delta.group_name, + optional_view(payload.group_name)); + } + + /// @brief Clears the client's group state `state`, keeping its text storage. Main loop only. + void clear_group_state(GroupUpdateObject& state) { + state.playback_state.reset(); + this->state_group_id.fill(state.group_id, std::nullopt); + this->state_group_name.fill(state.group_name, std::nullopt); + } }; /// @brief Client-level state the protocol task owns @@ -553,7 +586,7 @@ void SendspinClient::stop() { ++this->event_state_->drain_generation; this->cleanup_connection_state(ALL_ROLES_MASK); this->task_state_->client_state.reset(); - this->group_state_ = GroupUpdateObject{}; + this->event_state_->clear_group_state(this->group_state_); // A pairing attempt cut short by the stop leaves its code or pairing-window prompt showing. // Queue the dismissals now, after cleanup_connection_state() dropped the pending notes, so the @@ -1137,25 +1170,25 @@ void SendspinClient::drain_inbox() { // --- Group update events --- if (!overtaken() && (slot_bits & INBOX_TOPIC_GROUP) != 0) { - GroupUpdateObject group_delta; - if (es.group_slot.take(group_delta)) { - apply_group_update_deltas(&this->group_state_, group_delta); + GroupUpdatePayload payload; + if (es.group_slot.take(payload)) { + es.apply_group_update(this->group_state_, payload); if (this->listener_) { - this->listener_->on_group_update(group_delta); + this->listener_->on_group_update(es.group_delta); } - // group_name is cut to MAX_FRIENDLY_NAME_BYTES at parse, group_id only here. + // Both texts are bounded at parse (IdText, ServerName), so they print whole. + const GroupUpdateObject& group = this->group_state_; const std::string_view id = - peer_text_for_log(this->group_state_.group_id.has_value() - ? std::string_view(*this->group_state_.group_id) - : std::string_view("")); - SS_LOGD(TAG, "Group update - state: %s, id: %.*s, name: %s", - this->group_state_.playback_state.has_value() - ? to_cstr(this->group_state_.playback_state.value()) - : "unchanged", - static_cast(id.size()), id.data(), - this->group_state_.group_name.value_or("").c_str()); + group.group_id.has_value() ? std::string_view(*group.group_id) : ""; + const std::string_view name = + group.group_name.has_value() ? std::string_view(*group.group_name) : ""; + SS_LOGD(TAG, "Group update - state: %s, id: %.*s, name: %.*s", + group.playback_state.has_value() ? to_cstr(group.playback_state.value()) + : "unchanged", + static_cast(id.size()), id.data(), static_cast(name.size()), + name.data()); } } } @@ -1631,12 +1664,9 @@ bool SendspinClient::adopted_state_available() const { return client_state.has_value() && client_state->available; } -void SendspinClient::merge_group_update(GroupUpdateObject&& delta) { - this->event_state_->group_slot.merge( - [](GroupUpdateObject& current, GroupUpdateObject&& added) { - apply_group_update_deltas(¤t, added); - }, - std::move(delta)); +void SendspinClient::merge_group_update(const GroupUpdatePayload& delta) { + this->event_state_->group_slot.update( + [&delta](GroupUpdatePayload& current) { apply_group_update_deltas(¤t, delta); }); } // ============================================================================ diff --git a/src/client_dispatch.cpp b/src/client_dispatch.cpp index 5daca416..07327eb9 100644 --- a/src/client_dispatch.cpp +++ b/src/client_dispatch.cpp @@ -692,9 +692,22 @@ void SendspinClient::process_json_message(SendspinConnection& connection, const break; } GroupUpdateMessage group_msg; - if (parsed.extract(&group_msg)) { - this->merge_group_update(std::move(group_msg.group)); + if (!parsed.extract(&group_msg)) { + // messaging.md "group/update": a group_id that is absent or not 43 base64url + // characters is a protocol error the client MUST close the connection on. Closed + // silently, like every other protocol error the receive path finds (see + // close_silently()). One the client had no memory to normalize closes it too, + // rather than applying the rest of the update under the previous group's id; on the + // next connection the server sends the group again after its first server/activate. + conn->close_silently(SendspinGoodbyeReason::UNAUTHORIZED); + break; + } + if (group_msg.group_id_normalized && conn->note_group_id_normalized()) { + SS_LOGW(TAG, "Server sent a group_id that is not 43 base64url characters " + "(messaging.md \"group/update\"); normalized, a future release " + "closes the connection"); } + this->merge_group_update(group_msg.group); break; } case SendspinServerToClientMessageType::SERVER_PAIR_FINALIZE: { diff --git a/src/connection.h b/src/connection.h index e8e3384d..b9a1760a 100644 --- a/src/connection.h +++ b/src/connection.h @@ -583,6 +583,13 @@ class SendspinConnection : public std::enable_shared_from_thisgroup_id_normalized_, true); + } + /// @brief Returns true if `role` is active on this connection, judged on the exact versioned /// name this library implements. /// @@ -1233,6 +1240,10 @@ class SendspinConnection : public std::enable_shared_from_this group_slot(inbox, INBOX_TOPIC_GROUP); + * InboxSlot group_slot(inbox, INBOX_TOPIC_GROUP); * * // Producer thread: * group_slot.write(update); * * // Main loop: * if (inbox.poll() & INBOX_TOPIC_GROUP) { - * GroupUpdateObject update; + * GroupUpdatePayload update; * if (group_slot.take(update)) { ... } * } * @endcode diff --git a/src/inline_text.h b/src/inline_text.h index f582ac1d..c7e32312 100644 --- a/src/inline_text.h +++ b/src/inline_text.h @@ -21,6 +21,7 @@ #include #include #include +#include #include namespace sendspin { @@ -164,4 +165,13 @@ class InlineText { uint8_t size_{0}; }; +/// @brief The view of `text`, or nullopt when it holds no value +template +std::optional optional_view(const std::optional>& text) { + if (!text.has_value()) { + return std::nullopt; + } + return text->view(); +} + } // namespace sendspin diff --git a/src/protocol.cpp b/src/protocol.cpp index 0ba5735e..0422fc49 100644 --- a/src/protocol.cpp +++ b/src/protocol.cpp @@ -14,6 +14,7 @@ #include "application_role.h" #include "platform/base64.h" +#include "platform/crypto.h" #include "platform/json_arena.h" #include "platform/logging.h" #include "protocol_messages.h" @@ -43,6 +44,11 @@ namespace sendspin { static const char* const TAG = "sendspin.protocol"; +/// @brief messaging.md "group/update": a group_id that is not 43 base64url characters is normalized +/// until servers send that format, then this flips and a group/update whose group_id is absent or +/// not in that format closes the connection. +static constexpr bool GROUP_ID_STRICT = false; + // ============================================================================ // Static helpers // ============================================================================ @@ -536,6 +542,45 @@ void compute_time_exchange(const ServerTimeMessage& time_msg, int64_t client_sen } } +GroupIdOutcome group_id_from_wire(JsonVariantConst var, bool strict, IdText* out) { + if (var.isUnbound()) { + return strict ? GroupIdOutcome::REJECTED : GroupIdOutcome::IGNORED; + } + if (!var.is()) { + if (strict) { + return GroupIdOutcome::REJECTED; + } + if (var.isNull()) { + *out = IdText{}; + return GroupIdOutcome::KEPT; + } + return GroupIdOutcome::IGNORED; + } + const auto raw = var.as(); + const bool conforming = + raw.size() == PEER_ID_SIZE && std::all_of(raw.begin(), raw.end(), [](char c) { + return (c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || (c >= '0' && c <= '9') || + c == '-' || c == '_'; + }); + if (conforming || (raw.empty() && !strict)) { + (void)out->assign(raw); // at most PEER_ID_SIZE, so it fits + return GroupIdOutcome::KEPT; + } + if (strict) { + return GroupIdOutcome::REJECTED; + } + Sha256 hash; + hash.update(reinterpret_cast(raw.data()), raw.size()); + const std::array digest = hash.finalize(); + if (!hash.ok()) { + return GroupIdOutcome::FAILED; + } + std::array text{}; + // 32 bytes encode to PEER_ID_SIZE characters, so it fits + (void)out->assign(b64url_encode_into(text.data(), text.size(), digest.data(), digest.size())); + return GroupIdOutcome::NORMALIZED; +} + bool process_group_update_message(JsonObject root, GroupUpdateMessage* group_msg) { if (group_msg == nullptr) { return true; @@ -551,23 +596,37 @@ bool process_group_update_message(JsonObject root, GroupUpdateMessage* group_msg group_msg->group.playback_state = state; } - // Parse optional group_id; use empty string to signal clearing - JsonVariantConst group_id_var = root["payload"]["group_id"]; - if (group_id_var.is()) { - group_msg->group.group_id = group_id_var.as(); - } else if (!group_id_var.isUnbound() && group_id_var.isNull()) { - // Field set to null: use empty string to clear - group_msg->group.group_id = ""; + // Parse optional group_id; an empty one signals clearing + IdText group_id{}; + switch (group_id_from_wire(root["payload"]["group_id"], GROUP_ID_STRICT, &group_id)) { + case GroupIdOutcome::NORMALIZED: + group_msg->group_id_normalized = true; + [[fallthrough]]; + case GroupIdOutcome::KEPT: + group_msg->group.group_id = group_id; + break; + case GroupIdOutcome::IGNORED: + break; + case GroupIdOutcome::REJECTED: + SS_LOGW(TAG, "Invalid group/update: group_id is absent or not 43 base64url characters; " + "closing connection"); + return false; + case GroupIdOutcome::FAILED: + SS_LOGW(TAG, "Invalid group/update: no memory to normalize its group_id; closing " + "connection"); + return false; } // Parse optional group_name; use empty string to signal clearing JsonVariantConst group_name_var = root["payload"]["group_name"]; if (group_name_var.is()) { - group_msg->group.group_name = - std::string(read_friendly_name(group_name_var, "group/update group_name")); + // read_friendly_name() cuts it to MAX_FRIENDLY_NAME_BYTES, so it fits + ServerName group_name{}; + (void)group_name.assign(read_friendly_name(group_name_var, "group/update group_name")); + group_msg->group.group_name = group_name; } else if (!group_name_var.isUnbound() && group_name_var.isNull()) { // Field set to null: use empty string to clear - group_msg->group.group_name = ""; + group_msg->group.group_name = ServerName{}; } return true; @@ -575,7 +634,7 @@ bool process_group_update_message(JsonObject root, GroupUpdateMessage* group_msg // State delta application -void apply_group_update_deltas(GroupUpdateObject* current, const GroupUpdateObject& updates) { +void apply_group_update_deltas(GroupUpdatePayload* current, const GroupUpdatePayload& updates) { if (current == nullptr) { return; } diff --git a/src/protocol_messages.h b/src/protocol_messages.h index 4f333b65..5f86a0b2 100644 --- a/src/protocol_messages.h +++ b/src/protocol_messages.h @@ -1074,9 +1074,32 @@ struct ServerActivateMessage { std::optional pairing_format; }; +/// @brief A parsed group/update delta: GroupUpdateObject with its text held inline, which the +/// main loop copies into the public object. A field the message does not carry is nullopt, and an +/// empty text clears the field (the wire's null). +struct GroupUpdatePayload { + std::optional playback_state{}; + std::optional group_id{}; + std::optional group_name{}; +}; + +static_assert(std::is_trivially_copyable_v, + "parsed on the protocol task and carried to the main loop with nothing on the heap"); + /// @brief Parsed group/update message containing the group state delta struct GroupUpdateMessage { - GroupUpdateObject group; + GroupUpdatePayload group{}; + /// The group_id was not 43 base64url characters and was normalized (group_id_from_wire()) + bool group_id_normalized{false}; +}; + +/// @brief What group_id_from_wire() made of a wire group_id +enum class GroupIdOutcome : uint8_t { + KEPT, ///< Conforming, or while not strict empty or null: held as sent (empty clears) + NORMALIZED, ///< Not conforming: held as base64url(SHA-256(the bytes sent)), no padding + IGNORED, ///< While not strict, absent or not a string or null: left out of the delta + REJECTED, ///< While strict, absent or not conforming: a protocol error + FAILED, ///< The digest could not be computed (its hash state was not allocated) }; /// @brief Parsed stream/start message with per-role stream parameters @@ -1175,12 +1198,22 @@ bool process_server_time_message(JsonObject root, ServerTimeMessage* time_msg); void compute_time_exchange(const ServerTimeMessage& time_msg, int64_t client_sent, int64_t client_received, int64_t* offset, int64_t* max_error); +/// @brief Makes a group_id the client holds of the `group_id` member the server sent (messaging.md +/// "group/update": 43 base64url characters, carried by every message). A conforming id is held as +/// sent. When `strict`, any other value, null or absent included, is rejected. Otherwise an absent +/// id or a value that is not a string is ignored, an empty string or null clears the id, and any +/// other string is held as base64url of its SHA-256, so an id of any length and alphabet still +/// names its group with a conforming one. +/// @param out Receives the id for KEPT and NORMALIZED; left untouched otherwise. +GroupIdOutcome group_id_from_wire(JsonVariantConst var, bool strict, IdText* out); + /// @brief Parses a group/update JSON message into the provided struct +/// @return false when the message is a protocol error the client closes the connection on bool process_group_update_message(JsonObject root, GroupUpdateMessage* group_msg); -/// @brief Merges a GroupUpdateObject delta, which carries only the changed fields, into the -/// current group state -void apply_group_update_deltas(GroupUpdateObject* current, const GroupUpdateObject& updates); +/// @brief Merges a GroupUpdatePayload delta, which carries only the changed fields, into the +/// current one +void apply_group_update_deltas(GroupUpdatePayload* current, const GroupUpdatePayload& updates); /// @brief Parses a server/command JSON message into the provided struct bool process_server_command_message(JsonObject root, ServerCommandMessage* cmd_msg); diff --git a/src/reserved_text.h b/src/reserved_text.h new file mode 100644 index 00000000..dc253697 --- /dev/null +++ b/src/reserved_text.h @@ -0,0 +1,57 @@ +// Copyright 2026 Sendspin Contributors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// @file reserved_text.h +/// @brief Reserved storage that fills a public object's optional string field without allocating + +#pragma once + +#include +#include +#include +#include + +namespace sendspin { + +/// @brief Storage for one optional text field of a public object, reserved once for its bound so +/// filling the field never allocates: the reserved string is swapped into the field as it is set +/// and back here as it is cleared, and a swap keeps both buffers. +class ReservedText { +public: + explicit ReservedText(size_t capacity) { + this->spare_.reserve(capacity); + } + + /// @brief Sets `field` to `text`, or clears it when `text` is nullopt. `text` must fit the + /// capacity reserved at construction. + void fill(std::optional& field, std::optional text) { + if (!text.has_value()) { + if (field.has_value()) { + this->spare_.swap(*field); + field.reset(); + } + return; + } + if (!field.has_value()) { + field.emplace(); + field->swap(this->spare_); + } + field->assign(*text); + } + +private: + std::string spare_; +}; + +} // namespace sendspin diff --git a/tests/README.md b/tests/README.md index 48da0ca1..c85c81b4 100644 --- a/tests/README.md +++ b/tests/README.md @@ -123,13 +123,14 @@ Each `test_*.cpp` file covers one unit of cross-platform logic: buffers and state dropped, the roles it keeps left alone, and a removed role added back. - `test_allocations.cpp`: how many times the library calls `operator new` from a connection's accept to its admission, from `connect_to()` to admission, for a `client/state`, for a time burst, for a - controller `server/state`, for the `stream/*` messages, for the sync task's first FLAC codec - header, for a FLAC `stream/start` the player takes, for a `stream/start` section the connection's - roles do not take, for the first spectrum frame, and through the record flush that follows each of - a playback handoff to another paired server, a `server/unpair`, a `pairing_psk` pairing and a - dynamic pairing code pairing in each emission format. It replaces the global allocation functions, - so it builds into its own binary, `sendspin_alloc_tests`, which building `sendspin_tests` also - builds. Run it with `SENDSPIN_ALLOC_REPORT=1` to print each allocation grouped by call site. + controller `server/state`, for `group/update`, for the `stream/*` messages, for the sync task's + first FLAC codec header, for a FLAC `stream/start` the player takes, for a `stream/start` section + the connection's roles do not take, for the first spectrum frame, and through the record flush + that follows each of a playback handoff to another paired server, a `server/unpair`, a + `pairing_psk` pairing and a dynamic pairing code pairing in each emission format. It replaces the + global allocation functions, so it builds into its own binary, `sendspin_alloc_tests`, which + building `sendspin_tests` also builds. Run it with `SENDSPIN_ALLOC_REPORT=1` to print each + allocation grouped by call site. - `test_crypto.cpp`, `test_cpace.cpp`, `test_pairing_code.cpp`, `test_psk_wrap.cpp`, `test_pairing_token.cpp`, `test_noise_transport.cpp`, `test_noise_rehandshake.cpp`, `test_admission.cpp`, `test_record_store.cpp`, `test_dynamic_pairing_code.cpp`, diff --git a/tests/test_allocations.cpp b/tests/test_allocations.cpp index 0621eb70..49575a03 100644 --- a/tests/test_allocations.cpp +++ b/tests/test_allocations.cpp @@ -526,6 +526,7 @@ constexpr size_t ADMISSION_ALLOCATIONS = 0; constexpr size_t CLIENT_STATE_ALLOCATIONS = 0; constexpr size_t TIME_BURST_ALLOCATIONS = 0; constexpr size_t CONTROLLER_STATE_ALLOCATIONS = 0; +constexpr size_t GROUP_UPDATE_ALLOCATIONS = 0; constexpr size_t STREAM_MESSAGES_ALLOCATIONS = 0; constexpr size_t PLAYER_FLAC_STREAM_START_ALLOCATIONS = 0; constexpr size_t SYNC_TASK_FIRST_FLAC_HEADER_ALLOCATIONS = 0; @@ -536,12 +537,12 @@ constexpr size_t UNTAKEN_STREAM_START_ALLOCATIONS = 0; // application roles and the player, artwork and visualizer objects of client/state are built: from // the fake server's connect through admission and the first client/state, then one client/state // publish, then one full burst of client/time and server/time exchanges, then the first -// controller server/state, then an artwork and visualizer stream/start and a stream/end and a -// stream/clear that name their roles, then the sync task's first FLAC codec header, then a FLAC -// stream/start the player takes, then a stream/start whose player section the connection, no -// longer owning the player, does not take. -// The roles' public command and type lists are reserved when the roles are built, so even the -// first fill on the main loop allocates nothing. +// controller server/state, then the first group/update messages, then an artwork and visualizer +// stream/start and a stream/end and a stream/clear that name their roles, then the sync task's +// first FLAC codec header, then a FLAC stream/start the player takes, then a stream/start whose +// player section the connection, no longer owning the player, does not take. +// The roles' public command and type lists and the client's group texts are reserved when they +// are built, so even the first fill on the main loop allocates nothing. TEST(Allocations, AcceptToAdmissionAndPerMessagePaths) { // Each string but the MAC address is longer than libc++'s 22-character inline capacity, so the // host counts every copy libstdc++ (15 characters) makes of a realistic one. @@ -634,6 +635,38 @@ TEST(Allocations, AcceptToAdmissionAndPerMessagePaths) { EXPECT_EQ(controller.get_controller_state().supported_commands.size(), CONTROLLER_COMMAND_COUNT); + // A UUID group_id the protocol task normalizes and a group_name, then a conforming id alone, + // which clears the delta's name, then the id with another name, each applied by the main loop + // before the next is sent. + const std::string_view normalized_id = "FjYvVmOHs89abpL7Cphsdsog6zoMEsvfvQspUB4MGN8"; + const std::string_view conforming_id = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMN-_0"; + const GroupUpdateObject& group = client.get_group_state(); + open_window(); + { + TestOwned test_owned; + ASSERT_TRUE(server->send_app_json( + R"({"type":"group/update","payload":{"playback_state":"playing",)" + R"("group_id":"3f2504e0-4f89-41d3-9a0c-0305e82c3301",)" + R"("group_name":"Allocation Test Kitchen and Living Room"}})")); + } + pump_while_not(client, [&] { return group.group_id == normalized_id; }); + { + TestOwned test_owned; + ASSERT_TRUE(server->send_app_json( + R"({"type":"group/update","payload":{"group_id":")" + std::string(conforming_id) + + R"("}})")); + } + pump_while_not(client, [&] { return group.group_id == conforming_id; }); + { + TestOwned test_owned; + ASSERT_TRUE(server->send_app_json( + R"({"type":"group/update","payload":{"group_id":")" + std::string(conforming_id) + + R"(","group_name":"Allocation Test Upstairs Bedroom Group"}})")); + } + pump_while_not(client, + [&] { return group.group_name == "Allocation Test Upstairs Bedroom Group"; }); + expect_library_allocations(close_window(), "group/update", GROUP_UPDATE_ALLOCATIONS); + const int starts_before_streams = visualizer_listener.stream_starts; // Closes on the client/state the trailing volume command publishes, which the protocol task diff --git a/tests/test_client_lifecycle.cpp b/tests/test_client_lifecycle.cpp index 7c630cf1..6e563f64 100644 --- a/tests/test_client_lifecycle.cpp +++ b/tests/test_client_lifecycle.cpp @@ -221,7 +221,8 @@ class ForbiddenMetadataListener : public MetadataRoleListener { }; std::string group_update_playing_json() { - return R"({"type":"group/update","payload":{"playback_state":"playing"}})"; + return R"({"type":"group/update","payload":{"playback_state":"playing",)" + R"("group_id":"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMN-_0","group_name":"Kitchen"}})"; } // ============================================================================ @@ -261,6 +262,8 @@ TEST(ClientLifecycle, RestartYieldsALiveClient) { EXPECT_FALSE(client.is_connected()); EXPECT_FALSE(client.get_server_information().has_value()); EXPECT_FALSE(client.get_group_state().playback_state.has_value()); + EXPECT_FALSE(client.get_group_state().group_id.has_value()); + EXPECT_FALSE(client.get_group_state().group_name.has_value()); // The peer received its goodbye and the close, in that order. wait_until([&] { return server->closed(); }); EXPECT_EQ(server->goodbye_reason().value_or(""), "shutdown"); diff --git a/tests/test_encrypted_lifecycle.cpp b/tests/test_encrypted_lifecycle.cpp index 028b8400..0e730525 100644 --- a/tests/test_encrypted_lifecycle.cpp +++ b/tests/test_encrypted_lifecycle.cpp @@ -2455,6 +2455,47 @@ TEST(EncryptedLifecycle, EveryRoleMessageTypeRunsThroughItsHandler) { EXPECT_EQ(*bundle.client_ref().get_group_state().group_name, "Kitchen"); } +// messaging.md "group/update": the delta the listener is handed and the client's group state carry +// a conforming group_id as sent, and a UUID, which a server not yet sending the specified format +// sends, as base64url of its SHA-256 (the value GroupIdIsKeptNormalizedOrRejected derives). The +// delta carries only the fields its message did, while the group state keeps the others. +TEST(EncryptedLifecycle, GroupIdReachesTheListenerAsSentOrNormalized) { + struct GroupListener : SendspinClientListener { + GroupUpdateObject delta; + void on_group_update(const GroupUpdateObject& group) override { + this->delta = group; + } + }; + GroupListener group_listener; // Outlives the client + DispatchTestClient bundle("Group Id Test Client"); + bundle.client_ref().set_listener(&group_listener); + DispatchTestConnection& conn = bundle.connection(); + bundle.admit(conn); + const GroupUpdateObject& state = bundle.client_ref().get_group_state(); + + // Control: a conforming id is held as sent. + const std::string conforming = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMN-_0"; + bundle.deliver(conn, R"({"type":"group/update","payload":{"playback_state":"playing",)" + R"("group_id":")" + + conforming + R"(","group_name":"Kitchen"}})"); + bundle.pump(); + EXPECT_EQ(group_listener.delta.group_id, conforming); + EXPECT_EQ(group_listener.delta.group_name, "Kitchen"); + EXPECT_EQ(state.group_id, conforming); + + const std::string uuid_only = + R"({"type":"group/update","payload":{"group_id":"3f2504e0-4f89-41d3-9a0c-0305e82c3301"}})"; + const std::string normalized = "FjYvVmOHs89abpL7Cphsdsog6zoMEsvfvQspUB4MGN8"; + bundle.deliver(conn, uuid_only); + bundle.pump(); + EXPECT_EQ(group_listener.delta.group_id, normalized); + EXPECT_EQ(group_listener.delta.group_name, std::nullopt); + EXPECT_EQ(group_listener.delta.playback_state, std::nullopt); + EXPECT_EQ(state.group_id, normalized); + EXPECT_EQ(state.group_name, "Kitchen"); + EXPECT_EQ(state.playback_state, SendspinPlaybackState::PLAYING); +} + // Role traffic reaches a role only from the admitted connection that owns it, not from any // connection that merely has the role active: with more than one admitted connection, each role // has one owner, and the others' traffic for it is ignored. Every row's connection has the diff --git a/tests/test_protocol.cpp b/tests/test_protocol.cpp index dd756395..b18a74d5 100644 --- a/tests/test_protocol.cpp +++ b/tests/test_protocol.cpp @@ -809,6 +809,54 @@ TEST(Protocol, GroupUpdatePlaybackStateValidation) { } } +// messaging.md "group/update": a group_id is 43 base64url characters, and every message carries +// it. One that is not is held as base64url(SHA-256(the bytes sent)) while not strict, and +// rejected while strict, as is any other value or an absent one. While not strict, an empty one +// and null clear the field, and an absent one or a value that is not a string is ignored. Each normalized id is the +// independent `openssl dgst -sha256 -binary | base64 | tr '+/' '-_' | tr -d '='` of the row's id. +TEST(Protocol, GroupIdIsKeptNormalizedOrRejected) { + struct Row { + const char* what; + std::string member; // the payload's group_id member as JSON, empty when absent + GroupIdOutcome lenient; + std::string lenient_id; + GroupIdOutcome strict; + }; + const std::string conforming = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMN-_0"; + const auto id = [](const std::string& text) { return R"("group_id":")" + text + R"(")"; }; + const Row rows[] = { + {"Control: 43 base64url characters", id(conforming), GroupIdOutcome::KEPT, conforming, + GroupIdOutcome::KEPT}, + {"a UUID, every character base64url", id("3f2504e0-4f89-41d3-9a0c-0305e82c3301"), + GroupIdOutcome::NORMALIZED, "FjYvVmOHs89abpL7Cphsdsog6zoMEsvfvQspUB4MGN8", + GroupIdOutcome::REJECTED}, + {"44 base64url characters", id(std::string(44, 'A')), GroupIdOutcome::NORMALIZED, + "sGs_IMJG23ChNuOuR4fQ35bbT2k9IVwhiD08GXAPsnY", GroupIdOutcome::REJECTED}, + {"43 characters, one the standard alphabet's '+'", id(std::string(42, 'A') + "+"), + GroupIdOutcome::NORMALIZED, "C13S2O6t-JcoZkUOBR_ny8n7ZMI_6i5jx3CqkE31o_w", + GroupIdOutcome::REJECTED}, + {"empty", id(""), GroupIdOutcome::KEPT, "", GroupIdOutcome::REJECTED}, + {"null", R"("group_id":null)", GroupIdOutcome::KEPT, "", GroupIdOutcome::REJECTED}, + {"a number", R"("group_id":7)", GroupIdOutcome::IGNORED, "", GroupIdOutcome::REJECTED}, + {"absent", "", GroupIdOutcome::IGNORED, "", GroupIdOutcome::REJECTED}, + }; + for (const Row& row : rows) { + SCOPED_TRACE(row.what); + JsonDocument doc; + JsonObject root; + ASSERT_TRUE(parse("{" + row.member + "}", doc, root)); + IdText lenient_id{}; + ASSERT_EQ(group_id_from_wire(root["group_id"], false, &lenient_id), row.lenient); + EXPECT_EQ(lenient_id.view(), row.lenient_id); + + IdText strict_id{}; + ASSERT_EQ(group_id_from_wire(root["group_id"], true, &strict_id), row.strict); + if (row.strict == GroupIdOutcome::KEPT) { + EXPECT_EQ(strict_id.view(), row.lenient_id); + } + } +} + // messaging.md "server/hello": name is required, so a message without it is rejected; server_id // comes from the Noise handshake result. TEST(Protocol, ServerHelloRequiresName) { @@ -858,7 +906,8 @@ TEST(Protocol, FriendlyNamesAreTruncatedToTheirCap) { group_doc, group_root)); GroupUpdateMessage group; ASSERT_TRUE(process_group_update_message(group_root, &group)); - EXPECT_EQ(group.group.group_name, c.kept); + ASSERT_TRUE(group.group.group_name.has_value()); + EXPECT_EQ(group.group.group_name->view(), c.kept); } }