fix(cluster): harden admin proof, dual-role drain, delete timeout - #151
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 20 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change requires administrator proofs for durable cluster mutations and drain/resume forwarding. It validates network deletion hints against durable state, adds deletion-drain timeouts, and preserves surviving RTMP roles when only one role is deleted. ChangesCluster security and stream cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClusterManager
participant NetworkHandler
participant Db
participant RtmpBridge
participant Connection
ClusterManager->>NetworkHandler: Forward proof-bearing control message
NetworkHandler->>Db: Validate deletion or viewer state
Db-->>NetworkHandler: Return durable state
NetworkHandler->>RtmpBridge: Apply accepted cleanup
RtmpBridge->>Connection: Remove deleted roles
Connection-->>RtmpBridge: Preserve surviving role
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http.rs (1)
3427-3437: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis test no longer pins the normal drain path.
DELETE_DRAIN_TIMEOUTis 200 ms undercfg(test). The test sleepsDELETE_DRAIN_WARN_AFTER + 50ms(100 ms) before it callson_close(1). Under scheduler jitter the elapsed time can cross 200 ms first, and the loop then takes the timeout branch. Both branches finalize the delete and clear the marker, so the assertions still pass, but the test can pass without exercising the drain-completes path it is named for.Either raise the test
DELETE_DRAIN_TIMEOUTwell above the warn interval, or assert which branch ran.🤖 Prompt for 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. In `@src/http.rs` around lines 3427 - 3437, Adjust the test around the RTMP bridge on_close call so it reliably exercises the normal drain-completes path instead of potentially reaching DELETE_DRAIN_TIMEOUT first. Prefer configuring the test DELETE_DRAIN_TIMEOUT well above the warn interval, or otherwise assert that the drain branch ran before verifying deletion and marker cleanup.
🧹 Nitpick comments (6)
src/cluster/command.rs (1)
77-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the constant-true predicate explicit.
requires_admin_proofnow ignoresself. Thelet _ = self;line exists only to silence the unused-parameter path. Two alternatives keep the call sites unchanged and remove the placeholder:
- Add
#[allow(clippy::unused_self)]with the existing doc comment.- Replace call sites with an associated constant and delete the method.
Keeping the method is fine if you plan to reintroduce per-command exemptions later.
🤖 Prompt for 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. In `@src/cluster/command.rs` around lines 77 - 86, Make the constant-true requires_admin_proof predicate explicit by removing the let _ = self placeholder and adding #[allow(clippy::unused_self)] to the method. Preserve the existing method name, signature, documentation, and call sites.src/server.rs (2)
508-512: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the redundant prefilter call.
drain_deleted_stream_rolesrecomputes exactly whatconn_references_deleted_streamcomputes: the publisher stream id, the player stream id, and the samestream_ids_for_connfallback. It returnsfalsewhen nothing matches. The&&therefore performs the same three bridge lookups twice per connection per poll tick.The larger risk is drift. If the prefilter ever becomes narrower than the drain function, roles stop being drained with no other symptom.
Call
drain_deleted_stream_rolesalone. Keepconn_references_deleted_streamonly if another caller still needs it.♻️ Proposed simplification
// Tear down roles whose stream was deleted. Dual-role conns keep the // surviving role; kick only when nothing authorized remains. - if conn_references_deleted_stream(rtmp_bridge, conn_id, entry, deleted_now) - && drain_deleted_stream_roles(conn, entry, rtmp_bridge, conn_id, deleted_now) - { + if drain_deleted_stream_roles(conn, entry, rtmp_bridge, conn_id, deleted_now) { reject_indices.push(idx); continue; }🤖 Prompt for 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. In `@src/server.rs` around lines 508 - 512, Remove the redundant conn_references_deleted_stream prefilter from the teardown condition and call drain_deleted_stream_roles directly, preserving its existing arguments and kick behavior. Search for other callers of conn_references_deleted_stream and remove it only if unused after this change.
1356-1368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the relay route as well.
The test covers the role and tracker state. Lines 306-308 of
drain_deleted_stream_rolesalso rewriteconn.relay_keyto the surviving publisher stream. Add that assertion so a regression in the relay rewrite is caught here.💚 Proposed addition
assert!( conn.pending_relay.is_empty(), "queued play frames must not survive onto the publisher route" ); + assert_eq!(conn.relay_key, "s1"); + assert!(conn.relay_enabled);🤖 Prompt for 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. In `@src/server.rs` around lines 1356 - 1368, Extend the test assertions after drain_deleted_stream_roles to verify that conn.relay_key has been rewritten to the surviving publisher stream, matching the expected relay identifier for stream "s1".src/cluster/network.rs (1)
854-875: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFold the two proof checks into one match arm.
The payload strings match
drain_nodeandresume_nodeinsrc/cluster/manager.rsexactly, and an omittedproofdeserializes to"", which fails verification. The gate is fail-closed.Two points on the shape of the check:
- A single
matchon&adminreads better than two sequentialif letblocks and keeps the payload construction next to its variant.- The proof binds only
node_id. It carries no nonce or expiry, so an authenticated member that captured anAdminDrainframe can replay it later.force_drainandforce_resumeare idempotent, so the impact is limited to re-triggering an admission state an operator already requested. If you want replay resistance here, include the handshake nonce or a timestamp in the signed payload.♻️ Proposed refactor for the proof checks
- if let ControlMessage::AdminDrain { node_id, proof } = &admin { - let payload = format!("AdminDrain:{node_id}"); - if !verify_admin_proof(proof, &payload) { - return Err(std::io::Error::other("invalid admin drain proof")); - } - } - if let ControlMessage::AdminResume { node_id, proof } = &admin { - let payload = format!("AdminResume:{node_id}"); - if !verify_admin_proof(proof, &payload) { - return Err(std::io::Error::other("invalid admin resume proof")); - } - } + match &admin { + ControlMessage::AdminDrain { node_id, proof } => { + if !verify_admin_proof(proof, &format!("AdminDrain:{node_id}")) { + return Err(std::io::Error::other("invalid admin drain proof")); + } + } + ControlMessage::AdminResume { node_id, proof } => { + if !verify_admin_proof(proof, &format!("AdminResume:{node_id}")) { + return Err(std::io::Error::other("invalid admin resume proof")); + } + } + _ => {} + }🤖 Prompt for 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. In `@src/cluster/network.rs` around lines 854 - 875, Replace the two sequential proof-checking if-let blocks in the ControlMessage admin handling arm with one match on &admin, constructing the exact drain_node and resume_node payloads for AdminDrain and AdminResume respectively and rejecting invalid or omitted proofs through the existing fail-closed error path. Leave other control-message variants unchanged; replay resistance is optional and should only be added if the signed payload can consistently include a handshake nonce or timestamp.tests/cluster_security.rs (1)
46-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that pins the drain/resume proof payload format.
This test covers the client-write payload well. The new wire contract in this PR is the drain/resume payload string.
src/cluster/manager.rsbuildsAdminDrain:{node_id}, andsrc/cluster/network.rsrebuilds the same string to verify. Nothing fails if one side changes the format; drain and resume would silently stop working across nodes.Add a small test that asserts the exact payload strings so the two sides cannot drift.
💚 Proposed test
#[test] fn admin_drain_resume_proof_payload_format_is_stable() { let token = "api-token-for-tests-only"; assert!(secrets_equal( &admin_proof(token, "AdminDrain:7"), &admin_proof(token, &format!("AdminDrain:{}", 7u64)) )); assert!(!secrets_equal( &admin_proof(token, "AdminDrain:7"), &admin_proof(token, "AdminResume:7") )); }🤖 Prompt for 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. In `@tests/cluster_security.rs` around lines 46 - 54, Add a test near empty_client_write_proof_does_not_match_signed_payload named admin_drain_resume_proof_payload_format_is_stable that verifies admin_proof uses the exact “AdminDrain:7” payload format, matches the equivalent formatted node ID, and does not match the “AdminResume:7” payload.src/db.rs (1)
801-817: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing this helper in
stream_delete_if_pending.Lines 823-837 run the same
SELECT pending_delete FROM streams WHERE id=?query with the same error logging. The new helper covers that read.One caveat blocks a direct substitution:
stream_delete_if_pendingreturnsSome(false)for a missing row andNonefor a DB error, whilestream_pending_deletereturnsNonefor both. To share the code, the helper would need to distinguish the two cases, for example by returningDbLookup<bool>.The current single caller
stream_allows_network_drainis correct as written, because it checks existence withstream_getfirst, and aNonefrom a DB error rejects the drain.🤖 Prompt for 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. In `@src/db.rs` around lines 801 - 817, Refactor stream_pending_delete and stream_delete_if_pending to reuse a shared lookup result that distinguishes a missing row from a database error, such as DbLookup<bool>. Preserve stream_delete_if_pending’s Some(false) result for missing rows and None for errors, while retaining stream_allows_network_drain’s existing behavior for database failures.
🤖 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/cluster/manager.rs`:
- Around line 698-702: Defer the client_write_admin_proof call in block_on_write
until the ForwardToLeader branch, so local-leader raft.client_write operations
proceed without requiring a session-hook token. Preserve proof generation and
forwarding behavior for writes that must be sent to another leader.
In `@src/rtmp_bridge.rs`:
- Around line 538-577: The delete-drain timeout teardown is incomplete. In
src/rtmp_bridge.rs lines 538-577, update abandon_roles_for_stream to collect
affected ConnId values while holding the conns lock, release the guard, then
call try_release_ownership_for_conn for abandoned publishers and
maybe_unsubscribe_remote_play for abandoned players, following
force_unpublish_stream; return the abandoned connection IDs. In src/http.rs
lines 1245-1254, use those IDs to force-close connections through the RTMP poll
loop, and remove the redundant deleted_streams insertion because the insertion
at line 1343 already handles it.
In `@src/server.rs`:
- Around line 298-311: Guard the relay-key update in the has_pub/has_play branch
so stream IDs still present in deleted_now are not assigned to conn.relay_key or
enabled for relaying. Preserve the existing assignment for valid stream IDs and
continue clearing pending relay frames.
---
Outside diff comments:
In `@src/http.rs`:
- Around line 3427-3437: Adjust the test around the RTMP bridge on_close call so
it reliably exercises the normal drain-completes path instead of potentially
reaching DELETE_DRAIN_TIMEOUT first. Prefer configuring the test
DELETE_DRAIN_TIMEOUT well above the warn interval, or otherwise assert that the
drain branch ran before verifying deletion and marker cleanup.
---
Nitpick comments:
In `@src/cluster/command.rs`:
- Around line 77-86: Make the constant-true requires_admin_proof predicate
explicit by removing the let _ = self placeholder and adding
#[allow(clippy::unused_self)] to the method. Preserve the existing method name,
signature, documentation, and call sites.
In `@src/cluster/network.rs`:
- Around line 854-875: Replace the two sequential proof-checking if-let blocks
in the ControlMessage admin handling arm with one match on &admin, constructing
the exact drain_node and resume_node payloads for AdminDrain and AdminResume
respectively and rejecting invalid or omitted proofs through the existing
fail-closed error path. Leave other control-message variants unchanged; replay
resistance is optional and should only be added if the signed payload can
consistently include a handshake nonce or timestamp.
In `@src/db.rs`:
- Around line 801-817: Refactor stream_pending_delete and
stream_delete_if_pending to reuse a shared lookup result that distinguishes a
missing row from a database error, such as DbLookup<bool>. Preserve
stream_delete_if_pending’s Some(false) result for missing rows and None for
errors, while retaining stream_allows_network_drain’s existing behavior for
database failures.
In `@src/server.rs`:
- Around line 508-512: Remove the redundant conn_references_deleted_stream
prefilter from the teardown condition and call drain_deleted_stream_roles
directly, preserving its existing arguments and kick behavior. Search for other
callers of conn_references_deleted_stream and remove it only if unused after
this change.
- Around line 1356-1368: Extend the test assertions after
drain_deleted_stream_roles to verify that conn.relay_key has been rewritten to
the surviving publisher stream, matching the expected relay identifier for
stream "s1".
In `@tests/cluster_security.rs`:
- Around line 46-54: Add a test near
empty_client_write_proof_does_not_match_signed_payload named
admin_drain_resume_proof_payload_format_is_stable that verifies admin_proof uses
the exact “AdminDrain:7” payload format, matches the equivalent formatted node
ID, and does not match the “AdminResume:7” payload.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71533b8b-1d5e-45e2-a01a-1cea916ed6ae
📒 Files selected for processing (8)
src/cluster/command.rssrc/cluster/manager.rssrc/cluster/network.rssrc/db.rssrc/http.rssrc/rtmp_bridge.rssrc/server.rstests/cluster_security.rs
glibc binary + Alpine/gcompat exited 127; panel Integration needs same-libc stages. Gate unused eviction_stream_id for clippy -D warnings. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Defer admin_proof to ForwardToLeader so local raft writes work before session hooks. Complete delete-drain timeout teardown (ownership, unsubscribe, force-close). Guard relay_key against deleted streams; raise test DELETE_DRAIN_TIMEOUT. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Summary
Companion panel PR: OpenRTMP/librtmp2-server-panel#114
Test plan
Summary by CodeRabbit
Security
Bug Fixes