fix(cluster): align InitCache timeline and sticky delete markers - #208
Conversation
Stage remapped timestamps for InitCache, inject headers at the remapped ts, saturate the remapper at u32::MAX, and stop pruning deleted_streams by live session presence during Raft delete ambiguity. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change unifies remapped timestamps for media cache injection, prevents timeline wraparound at ChangesMedia timing and cache injection
Deleted marker lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MediaSession
participant MediaCache
participant TimelineRemapper
participant LiveOutput
MediaSession->>MediaCache: Store InitCache entry
MediaSession->>TimelineRemapper: Remap keyframe timestamp
TimelineRemapper-->>MediaSession: Return one timeline timestamp
MediaSession->>LiveOutput: Inject metadata and codec headers
MediaSession->>LiveOutput: Inject keyframe using the same timestamp
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change aligns media timestamps, prevents timeline wraparound, and preserves delete markers for timeout recovery. No merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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>
…d helper Collapse inject_init_cache_live parameters for Sonar/clippy, and gate live_stream_ids_for_deleted_markers behind cfg(test) after removing production retain. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_815756b6-67ca-43c3-9799-1edab4020cab) |
![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: 473c9d139d
ℹ️ 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".
| // `deleted_streams` markers are owned by HTTP/cluster delete | ||
| // paths (insert on begin_delete, remove on finalize/rollback). | ||
| // Do not prune by live session presence — that drops sticky | ||
| // markers during Raft begin_delete ambiguity when the node has | ||
| // no local RTMP sessions for the stream. |
There was a problem hiding this comment.
When ownership moves away while this node still has a local publisher, sync_ownership_from_db calls force_unpublish_and_drain, which inserts the stream into this same set without scheduling a delete finalize or ClearDrainStream. Removing the live-session pruning therefore leaves that marker forever after the publisher is kicked, and the checks in rtmp_bridge.rs reject every later publish and play attempt for the stream on this node, including after ownership returns. Keep explicit cleanup for these transient force-unpublish markers, or separate them from sticky delete markers.
Useful? React with 👍 / 👎.
Codex finding: ownership force_unpublish inserts deleted_streams without finalize, so disabling live-session prune left permanent publish/play blocks. Track HTTP deletes in sticky_deleted_streams and only prune non-sticky markers after local sessions drain. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
timeline_tsdomain (avoids remapper poisoning / jumps on failover).u32::MAXinstead of wrapping to ~0.deleted_streamsby live session presence so sticky markers survive Raftbegin_deletetimeouts.Test plan
clusterfeature (media hub / timeline)Made with Cursor
Summary by CodeRabbit