Repository navigation
Interop fixes and add subscribe namespace support and handling - #195
Conversation
* Update libquicr * Update to correctly set publisher priority and subscriber priority
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0166944d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| priority = Read<uint8_t>(it, end); | ||
| delivery_timeout = Read<uint64_t>(it, end); | ||
| expires = Read<uint64_t>(it, end); | ||
| forward = Read<uint64_t>(it, end); |
There was a problem hiding this comment.
When a relay is upgraded while any peer still runs the previous release, both sides advertise protocol version 1 but interpret this payload differently: the old decoder treats these new attribute bytes as the former subscribe_data length, while this decoder treats the old length and message bytes as attributes. This results in parse errors or corrupted subscription state, so rolling upgrades break peering subscriptions. Preserve the old encoding or introduce and validate a new protocol version with version-specific decoding.
Useful? React with 👍 / 👎.
| publish_object_priority_ = priority; | ||
| } | ||
|
|
||
| server_.peer_manager_.ClientSubscribe(GetFullTrackName(), attrs); |
There was a problem hiding this comment.
When an existing downstream subscription receives an update acknowledgement, including the kNewGroupRequested and kPaused statuses handled above, this always calls ClientSubscribe, which constructs a fresh SubscribeInfo starting at sequence zero. After serialization it reuses the initial sequence number, so InfoBase::AddSubscribe rejects it as unchanged and the new forward or new-group attributes are neither persisted nor propagated beyond the directly connected peer. Use the existing ClientSubscribeUpdate path when this track already has a local subscribe-info entry.
Useful? React with 👍 / 👎.
|
|
||
| state_.conn_state_metrics[connection_handle].published_tracks++; | ||
|
|
||
| NotifySubscribeNamespaces(publish_attributes.track_full_name.name_space, connection_handle, false); |
There was a problem hiding this comment.
When a publishing connection disappears without sending PUBLISH_DONE, this records the namespace in notified_namespaces_, but OnSessionRemoved calls PurgePublishState, which only erases pub_subscribes and pub_namespace_active and never calls NotifySubscribeNamespaces(..., true). SUBSCRIBE_NAMESPACE clients therefore retain a namespace that no longer exists, and the stale notification entry can also suppress a later NAMESPACE if that namespace is republished. Purge should collect the removed namespaces and send NAMESPACE_DONE whenever no other publisher remains.
Useful? React with 👍 / 👎.
1 participant
Adds subscribe namespace handling and update to latest libquicr main that has various interop fixes for draft-18.