Repository navigation
Hold group/update's fields inline and normalize a non-conforming group_id - #256
Merged
Merged
Conversation
…p_id messaging.md "group/update" (Sendspin/spec#340) makes group_id 43 base64url characters and anything else a protocol error that closes the connection, but servers in use today, aiosendspin among them, send 36-character UUIDs. Until they follow it, GROUP_ID_STRICT stays false and a non-conforming id is reported as base64url(SHA-256(id)), with one warning per connection; flipping it makes such a group/update close the connection. Every case, null and non-string values included, is decided in group_id_from_wire(). group/update no longer copies onto the heap: the protocol task parses group_id and group_name into a private GroupUpdatePayload (IdText, ServerName) carried through the inbox slot, and the main loop fills the public GroupUpdateObject strings in place from buffers reserved at construction, so even the first update after start() allocates nothing.
The metadata role will fill its optional strings the same way, so ReservedText moves out of client.cpp into src/reserved_text.h, and optional_view() moves beside InlineText in inline_text.h.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Re-entrant group-update delivery can invalidate string references held by an active callback, creating a critical correctness issue.
8 open issues
What changed in this PR
This PR removes operator new allocations from the Sendspin client's group/update handling, reducing allocation risk on ESP devices while staging stricter group-ID validation.
Changes:
- Carries group fields inline through the inbox and reuses reserved public-string storage.
- Normalizes nonempty, nonconforming group IDs with SHA-256 and warns once per connection; strict enforcement remains disabled.
- Adds parser, lifecycle, and allocation coverage, with matching documentation updates.
| File | Description |
|---|---|
tests/test_protocol.cpp |
Tests group-ID outcomes and inline name parsing. |
tests/test_encrypted_lifecycle.cpp |
Checks listener and state values after normalization. |
tests/test_client_lifecycle.cpp |
Checks group fields clear on restart. |
tests/test_allocations.cpp |
Adds a zero-operator new group-update allocation window. |
tests/README.md |
Documents the expanded allocation coverage. |
src/reserved_text.h |
Adds reusable storage for optional public strings. |
src/protocol.cpp |
Parses inline fields and normalizes or rejects IDs. |
src/protocol_messages.h |
Defines inline group payloads and ID outcomes. |
src/inline_text.h |
Adds optional inline-text views. |
src/inbox.h |
Updates the usage example to the inline payload. |
src/connection.h |
Tracks the per-connection normalization warning. |
src/client.cpp |
Reuses reserved strings when applying group updates. |
src/client_dispatch.cpp |
Handles rejection and warns on normalization. |
include/sendspin/types.h |
Documents normalized IDs and clear markers. |
include/sendspin/client.h |
Updates the private group-merge interface. |
docs/internals.md |
Describes inline storage and normalization allocations. |
docs/integration-guide.md |
Explains normalized IDs delivered to listeners. |
CLAUDE.md |
Documents group-update allocation-test coverage. |
Review effort: Max
Strict mode now rejects a group/update without a group_id, as it does any other value outside the format. A group_id the client has no memory to normalize closes the connection instead of applying the rest of the update under the previous group's id. The listener guide says an empty id means cleared, and the lifecycle test no longer counts warning text.
kahrendt
added a commit
that referenced
this pull request
Nov 11, 2026
… fields (#258) ## Summary `server/state` metadata still copied every string onto the heap through `operator new`, which aborts on ESPHome. Sendspin/spec#355 caps `title`, `artist`, `album_artist` and `album` at 255 bytes and `artwork_url` at 2048, so the text now lives in fixed blocks allocated at `start()` and a metadata state allocates nothing. Part of Sendspin/backlog#92. **Behaviour change:** a value over its cap is cut at a UTF-8 boundary, with one warning per connection. `METADATA_STRICT` (in `protocol.cpp`, `false` for now) later turns that into a protocol error that closes the connection, as #256 staged for `group_id`. ## Change - **Field opt-in:** `MetadataRoleConfig` has one bool per text field (all `true` by default) and a `buffer_location`; `add_metadata(MetadataRoleConfig config = {})` keeps the no-argument call. An unkept field is dropped at parse and takes no storage, e.g. `{.artwork_url = false}` saves 2,048 B per block. - **Three text blocks** (9,204 B with every field, 3,060 B without the URL), PSRAM-preferring, allocated at `start()`: the slot's two places and the main loop's current state. A `start()` that cannot allocate them fails like artwork's. - **Hand-off:** the protocol task copies an arrival's kept text into its slot place under the Inbox mutex (at most 3,068 B, the largest Inbox critical section). The main loop peeks at the timestamps, decides what is due with no lock held, and commits only if nothing arrived since; the public strings are filled from the current block outside the lock through `ReservedText`, reserved per kept field when the role is built. - **Scheduled states wait in the slot** instead of on the main loop. `ScheduledStateSlot` (`src/scheduled_state_slot.h`) holds that protocol, and the color role now uses it too, with unchanged behaviour. One interleaving shifts: an arrival during a callback replaces a pending state that falls due in that same callback, the same outcome as the arrival landing just before the drain. ## Tests - Parse tables at both caps with `Control:` rows (cap, cap+1, 2/3/4-byte characters straddling it, lenient and strict) and a kept-fields table; an end-to-end check of kept and dropped fields. - `MetadataSlot.ThreeBlocksCarryEveryStateToTheListenerInOrder`: a white-box table asserting after every step that the three blocks are a permutation, plus the delivered and pending titles, including a re-peek, cleanup, re-entrant `stop()` and restart. `ScheduledStateSlot` tests cover the decision table, generations, deciding with the Inbox mutex free and the slot's dirty bit. - Alloc window for metadata states (an over-cap one, a scheduled one delivered later, one without text): 0. Mutants caught include a commit ignoring the sequence, a deadline decided under the lock, block aliasing, a missing re-peek, the same-tick loop removed and a settle/commit/reset polarity flip. - Gaps: the strict close path and a failed block allocation at `start()` need a production seam. Stack (ESP-IDF 5.5.4, esp32): protocol task 6,512 B -Os / 6,624 B -Og (was 6,544 / 6,688), within its 7,168 B default; every other task unchanged. `tools/stack_usage/README.md` updated.
kahrendt
added a commit
that referenced
this pull request
Nov 11, 2026
## Summary First step of moving the public API from `std::string` to `std::string_view`. Metadata and group text already live in fixed storage (#256, #258); the public structs still copied it into `std::string`s kept allocation-free only by `ReservedText` (about 3 KB of internal RAM for metadata). The view forms hand consumers that storage directly; the string forms are deprecated and go in v0.11.0 with `ReservedText`. Part of Sendspin/backlog#92. **Migration:** override `on_track_metadata(const TrackMetadata&)` instead of `on_metadata()`, `on_group_state(const GroupState&)` instead of `on_group_update()`, and call `group_state()` instead of `get_group_state()`. Text fields become `std::optional<std::string_view>`: copy them if you keep them past the callback (or, for `group_state()`, past the next return to `loop()` or `stop()`). A view is not guaranteed to be NUL-terminated, so print it with `%.*s` or copy it before handing it to a C-string API. docs/integration-guide.md "Deprecated String API" has the full table. ## Change - **Metadata:** `TrackMetadata` views the role's current text block, valid for the callback. - **Group:** `GroupState` serves both the delta `on_group_state()` receives (the fields changed by the group/update messages since the last call) and the merged state `group_state()` returns. Both view an inline copy the main loop keeps, so the views outlive the string forms' removal. - **Dispatch:** the library calls the new callback, then the deprecated one; both default to no-ops, so a consumer overrides one. A `stop()` inside the new callback skips the deprecated call (group: the drain generation; metadata: `TeardownTracker::completed()`, since the protocol task may bump the cleanup generation meanwhile). - **Deprecated** with `@deprecated` and `[[deprecated("...; removed in v0.11.0")]]`: `ServerMetadataStateObject`, `on_metadata()`, `GroupUpdateObject`, `on_group_update()`, `get_group_state()`. A consumer of only the new API compiles warning-free; the library silences the warning only at its own compat sites. - `get_track_progress_ms()` / `get_track_duration_ms()` read the role's current state, so the removal can delete the deprecated object outright. The examples use the new API. ## Tests - The metadata and group content tests check both forms carry identical content (cut text, unkept fields, cleared fields, delta versus merged state) and that the views do not point into the deprecated strings. - Listener-form tables: only new, only old, both (new first), a `stop()` from the new callback, and a protocol-task teardown during it (pins `completed()` over `accepts()`). The progress getters are pinned with and without a listener. - The metadata and group alloc windows stay at 0 with listeners overriding both forms. Mutants caught include views into the deprecated strings, the deprecated call skipped or reordered, the re-entrancy skip removed, and the getters reading the deprecated object. - Builds checked: `-DENABLE_WERROR=ON` with examples, `-DSENDSPIN_ENABLE_METADATA=OFF`, and an ESP-IDF esp32s3 build with no deprecation warnings from the library. Next in the series: the pairing listener strings, server information and token getters; then the vector callbacks, the persistence provider and the configs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
group/updatestill copiedgroup_idandgroup_nameonto the heap throughoperator new, which aborts on ESPHome. Both fields are now held inline from the parse to the main loop, and the public strings are filled in place, so agroup/updateallocates nothing. Part of Sendspin/backlog#92.The change also adopts Sendspin/spec#340:
group_idmust be 43 base64url characters, and anything else is a protocol error that closes the connection. Servers in use today, aiosendspin among them, still send 36-character UUIDs, so enforcement is staged.GROUP_ID_STRICT(inprotocol.cpp) staysfalsefor now. Behaviour change: a non-conforminggroup_idnow reaches consumers asbase64url(SHA-256(id))instead of as sent, with one warning per connection. Setting the flag totruemakes agroup/updatewhosegroup_idis absent or not in that format close the connection.Change
GroupUpdatePayload(IdText,ServerName) carried through the inbox slot.group_id_from_wire()handles every case: kept, normalized, rejected, ignored, and null or non-string values.GroupUpdateObject'sstd::optional<std::string>fields, in both the group state and the deltaon_group_update()receives, are filled throughReservedText(src/reserved_text.h). It keeps a string reserved for the field's bound when the client is built and swaps it in and out rather than freeing it on a clear. The metadata role can reuse it once metadata strings have a spec cap.callocper normalized id (noted in internals.md "Memory"). When that allocation fails, the connection is closed rather than applying the rest of the update under the previous group's id.Tests
Protocol.GroupIdIsKeptNormalizedOrRejected: a table covering lenient and strict outcomes, with hashes checked against openssl. Rows: a conforming control, a UUID, 44 characters, a standard-alphabet+, empty, null, a number, absent.EncryptedLifecycle.GroupIdReachesTheListenerAsSentOrNormalized: an end-to-end check of what the listener's delta and the group state show.callocinside a localSha256).group/updatemessages (normalize, clear the name, set a new name), expected 0.RestartYieldsALiveClientnow checks that the group fields clear.group_nameper Close the connection on out-of-range values spec#347, and whether an absentplayback_stateorgroup_namecloses it too.Stack (ESP-IDF 5.5.4, esp32): every task matches
tools/stack_usage/README.md, the protocol task at 6,544 B -Os / 6,688 B -Og against its 7,168 B default.