fix(ci): satisfy clippy unwrap_or_default on rtmp_bridge ConnState#34
Conversation
…eam switch on_connect() used HashMap::insert which wiped publisher/player state when authorize_publish() already ran during the same poll() tick (fast handshake + publish). That rejected legitimate publishers and left ghost active publisher rows blocking re-publish. Also deactivate prior publisher/player DB rows when the same connection switches streams via a second publish/play on the same TCP session. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
Replace or_insert_with(ConnState::default) with or_default() in on_connect() so cargo clippy -- -D warnings passes on the rtmp_bridge ConnState preservation change. Co-authored-by: Alexander Wagner <info@alexanderwagnerdev.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesRTMP bridge session state fix
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the RTMP↔DB integration (DbRtmpBridge) to preserve per-connection state across connection lifecycle callbacks (fixing a clippy-triggering pattern) and to prevent “ghost” active publisher/player rows when a single TCP connection switches streams.
Changes:
- Replace
HashMap::insert(conn, ConnState::default())withentry(conn).or_default()inon_connect()to avoid wiping an already-initializedConnState. - On stream switches, deactivate any prior publisher/player DB row associated with the same connection before establishing the new session.
- Add unit tests covering ConnState preservation and publisher/player deactivation on stream switches; update bug-scan progress notes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/rtmp_bridge.rs |
Preserves existing ConnState on connect, deactivates prior publisher/player rows on stream switch, and adds targeted unit tests. |
.cursor/bug-scan-progress.md |
Marks rtmp_bridge scan complete and records the fixed findings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Use or_insert so a publish/play callback that already ran during the same | ||
| // poll() tick (before this hook) keeps its ConnState — insert() would wipe | ||
| // an authorized publisher/player and leave a ghost active row in the DB. |
| // A connection may issue publish more than once (stream switch). Release any | ||
| // prior publisher row so we do not leave active=true ghosts in the DB. | ||
| { | ||
| let mut guard = self.conns.lock().unwrap(); | ||
| if let Some(mut old_pub) = guard.get_mut(&conn).and_then(|cs| cs.publisher.take()) { |
| // Same connection may play a different stream; deactivate the prior row. | ||
| { | ||
| let mut guard = self.conns.lock().unwrap(); | ||
| if let Some(mut old_player) = guard.get_mut(&conn).and_then(|cs| cs.player.take()) { | ||
| old_player.active = false; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rtmp_bridge.rs`:
- Around line 369-380: The stream-switch path in `RtmpBridge` leaves the
connection’s in-memory `stream_id` stale after taking the old `player`, which
breaks later player-only state updates for the new stream. Update the connection
state in the same switch flow that handles `cs.player.take()` so `cs.stream_id`
is refreshed to the current stream before any deletion-signalling logic runs.
Also adjust the related player-only handling in the matching `RtmpBridge`
update/deletion paths so they rely on the refreshed `stream_id` state rather
than the old one.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3dae8011-3145-4a1d-9449-ed32a779f8b9
📒 Files selected for processing (2)
.cursor/bug-scan-progress.mdsrc/rtmp_bridge.rs
![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: 576d6435c0
ℹ️ 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".
| // prior publisher row so we do not leave active=true ghosts in the DB. | ||
| { | ||
| let mut guard = self.conns.lock().unwrap(); | ||
| if let Some(mut old_pub) = guard.get_mut(&conn).and_then(|cs| cs.publisher.take()) { |
There was a problem hiding this comment.
When an already-publishing connection sends another publish that is rejected, for example because publisher_try_acquire later finds the target stream already has an active publisher, this take() has already removed and deactivated the connection's current publisher row. The function then returns Err with cs.publisher empty, so subsequent stats/close handling cannot represent the publisher that was active before the failed replacement; defer deactivating the old row until after the new slot is acquired or restore it on failure.
Useful? React with 👍 / 👎.
| // Same connection may play a different stream; deactivate the prior row. | ||
| { | ||
| let mut guard = self.conns.lock().unwrap(); | ||
| if let Some(mut old_player) = guard.get_mut(&conn).and_then(|cs| cs.player.take()) { |
There was a problem hiding this comment.
When this newly-supported player stream switch succeeds on a player-only connection, the old player is deactivated but cs.stream_id is left pointing at the first stream because the later assignment only runs when it is empty. Callers such as the server poll loop use stream_id_for_conn() after on_play() to set relay/deletion tracking, so switching from stream A to B can keep routing/tracking the connection as A; update the stored stream id for the replacement player while preserving any publisher-owned stream id if that mixed role is intentional.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| // A connection may issue publish more than once (stream switch). Release any | ||
| // prior publisher row so we do not leave active=true ghosts in the DB. |
There was a problem hiding this comment.
When this new same-connection publish switch succeeds, only DbRtmpBridge state is updated. In the inspected production path in src/server.rs, conn.relay_key and the tracked deletion stream are copied from stream_id_for_conn() only inside if stream.is_publishing && !entry.publishing, so an already-publishing connection keeps relaying/tracking the previous stream while the DB now marks the replacement stream active; refresh the poll-loop state after a replacement publish or reject such switches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rtmp_bridge.rs`:
- Around line 341-352: The rollback path in `RTMPBridge` is restoring
`ConnState.publisher` before confirming the database reactivation succeeded.
Update the `if !self.db.publisher_try_acquire(&pub_row)` branch to check the
result of `publisher_update` first, and only set `old_pub.active = true` and
store it back into `self.conns` after the DB write succeeds; if the write fails,
leave the in-memory state unchanged and handle the failure explicitly.
- Around line 428-431: The old-player deactivation in rtmp_bridge.rs is ignoring
the result of player_update, so a failed write can leave both the old and new
player active. Update the old-player branch in the relevant rtmp bridge handler
to check the return value from self.db.player_update for old_player.id, and
handle/report the failure instead of continuing silently. Make sure the fix is
tied to the old_player deactivation path so the row cannot remain active when
the update fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc5fb180-095a-45f2-bb06-7502dc2dcba5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/rtmp_bridge.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4290720fed
ℹ️ 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".
| let old_pub = { | ||
| let mut guard = self.conns.lock().unwrap(); | ||
| guard.get_mut(&conn).and_then(|cs| cs.publisher.take()) | ||
| }; |
There was a problem hiding this comment.
When this branch handles an already-publishing connection, it removes the old publisher but leaves the connection-level last_stats_at and bytes_at_last_stats from the previous stream intact. The server later passes connection-lifetime media_bytes_received into update_publisher_stats, so after a successful switch the new publisher row can skip its first update for up to a second and then report bytes/bitrate that include traffic from the old stream. Reset the stats baseline when a replacement publisher is installed.
Useful? React with 👍 / 👎.
| let mut guard = self.conns.lock().unwrap(); | ||
| let cs = guard.entry(conn).or_default(); | ||
| let old_player = cs.player.take(); | ||
| cs.player = Some(player_row); |
There was a problem hiding this comment.
When a playing connection switches to another stream here, the new player row inherits the connection-level last_stats_at and bytes_at_last_stats from the previous player. The poll loop later calls update_player_stats with connection-lifetime media_bytes_sent, so the replacement player can skip its first stats flush and then report bytes/bitrate from the old stream as if they belonged to the new viewer session. Reset the player stats baseline when installing a replacement player.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rtmp_bridge.rs`:
- Around line 72-80: The stats reset bookkeeping in RtmpBridge is shared across
publisher and player paths, which can cause one role’s replacement to clear the
other role’s pending reset and rebase the wrong DB row. Update the state in
RtmpBridge to keep separate baselines and reset flags for publisher and player,
or make the pending reset explicitly target a role, and adjust the stat
update/reset handling in the affected publisher/player methods so each path only
consumes its own state. Add a regression test covering a mixed publish+play
connection where one side is replaced and the other continues reporting stats.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72d374b20b
ℹ️ 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".
| if cs.stats_reset_pending { | ||
| cs.stats_reset_pending = false; | ||
| cs.bytes_base = media_bytes_received; | ||
| cs.bytes_at_last_stats = 0; |
There was a problem hiding this comment.
When a connection is both publishing and playing and only one role is replaced, this shared reset flag/base can be consumed by the other role. In the server poll loop I checked, publisher stats are updated before player stats (src/server.rs lines 316-325), so a player switch sets stats_reset_pending in on_play, but the next update_publisher_stats enters this branch, clears it, and sets bytes_base from media_bytes_received; the replacement player then keeps the old player baseline and reports bytes/bitrate from the previous viewer session. Keep separate publisher/player reset and byte-base state so one role cannot consume the other's reset.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Fixes CI failure on PR #33 (
cargo clippy -- -D warnings) by replacingor_insert_with(ConnState::default)withor_default()inon_connect().Includes the rtmp_bridge ConnState preservation fix from #33 plus this clippy lint fix so Tests workflow passes.
Test plan
cargo clippy --all-targets -- -D warningsshould passSummary by CodeRabbit