relay: decode DYNAMIC_GROUPS once at ingestion, not per request#21
Merged
Conversation
The §10.2.13 NEW_GROUP_REQUEST path called trackSupportsDynamicGroups on every request, which parsed the entire Track Properties block, pulled out the one DYNAMIC_GROUPS field, and discarded the rest. Track Properties are immutable for the entry's lifetime (§9.6, first-setter-wins), so the decode only needs to happen once. Decode DYNAMIC_GROUPS when Properties is set (centralized through a new setPropertiesLocked) and cache the (bool, error) on the TrackEntry, exposed via DynamicGroups(). The raw bytes are still stored verbatim for the §9.6 opaque downstream passthrough; only the one field the relay semantically acts on is now decoded eagerly. The §12.6 PROTOCOL_VIOLATION (DYNAMIC_GROUPS value > 1) is cached alongside so the caller's decline behavior is unchanged. trackSupportsDynamicGroups moves into registry as parseDynamicGroups, emptying newgroup.go (newGroupRequestValue was already inlined), so both it and newgroup_internal_test.go are deleted; their tests move to registry's dynamic_groups_test.go, with the dynamic-groups test rewritten to exercise the public SetProperties / DynamicGroups path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous parseDynamicGroups walked the whole Track Properties block to extract one field. Caching a second property that way would mean either a second full-block walk or a copy-pasted parse loop — an easy trap. Restructure so the block is parsed exactly once into a decodedProperties value: decodeTrackProperties walks the KV pairs and dispatches each property the relay acts on to a small per-value decoder. Adding a cached property is now a field + a switch case + an accessor, never a second pass over the bytes. A structural block-parse failure is recorded once as parseErr and surfaced by every accessor, so per-property error handling doesn't repeat it. Moves the decode into a dedicated registry/properties.go and adds a malformed-block test case alongside the existing §12.6 coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gocritic's singleCaseSwitch flags the dispatch switch while only one property is decoded. Rewrite as an if; the comment notes it becomes a switch once a second property is added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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
The §10.2.13 NEW_GROUP_REQUEST path called
trackSupportsDynamicGroupson every request, which parsed the entire Track Properties block, pulled out the oneDYNAMIC_GROUPSfield, and discarded the rest. Track Properties are immutable for a track entry's lifetime (§9.6, first-setter-wins), so that decode only needs to happen once.This decodes
DYNAMIC_GROUPSwhenPropertiesis set and caches the result on theTrackEntry.What changed
TrackEntrygains a cacheddynamicGroups bool/dynamicGroupsErr error, populated by a newsetPropertiesLockedhelper that both stores the raw bytes and decodes the field in one step (so the cache can't drift).AddUpstream'sWithPropertiespath andSetPropertiesboth route through it.TrackEntry.DynamicGroups() (bool, error)accessor;propagateNewGroupUpstreamreads it instead of re-parsing.Propertiesbytes are still stored verbatim for the §9.6 opaque downstream passthrough — only the one field the relay semantically acts on is decoded eagerly.DYNAMIC_GROUPSvalue > 1) is cached alongside, so the caller's "log and decline" behavior is unchanged.trackSupportsDynamicGroupsmoves intoregistryasparseDynamicGroups. That emptiednewgroup.go(newGroupRequestValuewas already inlined in a prior commit), sonewgroup.goandnewgroup_internal_test.goare deleted; their tests move toregistry/dynamic_groups_test.go, with the dynamic-groups test rewritten to exercise the publicSetProperties/DynamicGroupspath.Testing
go test ./...— passgo test -race ./pkg/relay/...— passgo vet ./pkg/relay/...— cleangofmt -l— clean🤖 Generated with Claude Code