Repository navigation
Ensure closed connections evict from ranking - #191
Conversation
![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: 708fe929df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Remove from top-n if no other connection publishes this track. | ||
| const auto next = state_.pub_subscribes.lower_bound({ remove_key.first, 0 }); | ||
| if (next == state_.pub_subscribes.end() || next->first.first != remove_key.first) { | ||
| top_n_removal.emplace_back(std::move(handler)); |
There was a problem hiding this comment.
When multiple connections publish the same track, the presence of another pub_subscribes entry does not mean the existing ranking entry is valid: TrackRanking stores only the connection ID and property value from the publisher that most recently updated that alias. If that publisher is the connection being purged, this condition skips removal and leaves its value and closed connection ID in the top-N until another publisher updates the property or the entry ages out, producing stale rankings and incorrect self-track filtering. Remove the disconnected handler's ranking entry or replace it with state from a surviving publisher rather than merely checking for the same track hash.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Probably matters but I'm not sure how much attention we should pay to multi-publisher ranking right now.
2 participants
I think it's possible in a non-graceful PUBLISH_DONE case for tracks to linger in the ranking rather than be immediately removed. Here I remove any cleaned up publish subscribes from the ranking - although outside the lock because of the handler update callback removal takes.