StreamEdge: move track resolution logic to TrackResolver class#538
Conversation
- consolidate the stateful logic with pending tracks in TrackResolver - cancel resolving process when a user leaves the call
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR extracts WebRTC-to-SFU track correlation logic from StreamEdge into a new TrackResolver class. TrackResolver holds track mappings, a pending WebRTC callback registry, per-key resolve locks, and pending eviction via pending_ttl. resolve() reuses known mappings, migrates on session change, or polls pending entries with timeout/cancel semantics; register(), unpublish(), and cancel() implement corresponding lifecycle operations. StreamEdge now delegates track_added registration, track published resolution, unpublish, and cancel to TrackResolver. Async tests exercise timing, migration, anonymous fallback, eviction, concurrent resolves, and cancellation. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11c6b42f-d6ad-4bc3-a0c9-e72c9091dcf5
📒 Files selected for processing (3)
plugins/getstream/tests/test_track_resolver.pyplugins/getstream/vision_agents/plugins/getstream/_track_resolver.pyplugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
There was a problem hiding this comment.
🧹 Nitpick comments (4)
plugins/getstream/vision_agents/plugins/getstream/_track_resolver.py (4)
34-47: 💤 Low valueCollapse redundant elif/else.
Both
elifandelsereturn"video", so the explicit branch is dead.Refactor
def _get_webrtc_kind(stream_track_type: StreamTrackType.ValueType) -> str: """Get the expected WebRTC kind (audio/video) for an SFU track type.""" if stream_track_type in ( StreamTrackType.TRACK_TYPE_AUDIO, StreamTrackType.TRACK_TYPE_SCREEN_SHARE_AUDIO, ): return "audio" - elif stream_track_type in ( - StreamTrackType.TRACK_TYPE_VIDEO, - StreamTrackType.TRACK_TYPE_SCREEN_SHARE, - ): - return "video" - else: - return "video" + return "video"
97-106: 💤 Low valueReuse path rewrites the map entry unnecessarily.
_reuse_knownalready flipsentry.published = Trueon the existing object, thenresolve()constructs a fresh_TrackEntryand reassignsself._track_map[track_key]. Functionally identical, but the double-write is a minor smell. Either drop the mutation in_reuse_known(let line 105 own it) or skip the rewrite when reused.
148-163: 💤 Low valueSingle-shot migration leaves additional stale sessions in the map.
The loop returns on the first match. If a user has accumulated multiple stale
_TrackKeys for the samestream_track_type(rapid reconnects), only one is migrated/deleted; the rest linger until process exit. Probably benign, but consider deleting all matches and returning the most recent.
234-247: 💤 Low valueTTL eviction logged at warning — possibly noisy on participant churn.
Stale pending eviction is the expected outcome when a participant leaves before SFU confirms a track that was registered but
cancel()didn't cover (e.g., race betweentrack_addedand departure). Per the PR description ("Change logging for unresolved-track removal from warning to debug"), considerlogger.debughere unless eviction genuinely indicates a bug.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 255b215d-e080-430d-bcc8-8117e46f08fa
📒 Files selected for processing (2)
plugins/getstream/tests/test_track_resolver.pyplugins/getstream/vision_agents/plugins/getstream/_track_resolver.py
…nt fires in parallel
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/getstream/tests/test_track_resolver.py (1)
7-7: ⚡ Quick winImport
TrackResolvervia the public package API, not_track_resolver.Line 7 imports from a private module path, which couples tests to internal module structure. Re-export
TrackResolverfrom the package and import from that public path.Proposed change
-from vision_agents.plugins.getstream._track_resolver import TrackResolver +from vision_agents.plugins.getstream import TrackResolver# vision_agents/plugins/getstream/__init__.py from ._track_resolver import TrackResolverAs per coding guidelines: "Never import from private modules (
_foo) outside of the package's own__init__.py. Use the public re-export instead."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 27e497f3-1bef-43ca-99ef-82eeea1e77c8
📒 Files selected for processing (3)
plugins/getstream/tests/test_track_resolver.pyplugins/getstream/vision_agents/plugins/getstream/_track_resolver.pyplugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/getstream/vision_agents/plugins/getstream/_track_resolver.py
- plugins/getstream/vision_agents/plugins/getstream/stream_edge_transport.py
2 participants
Why
We need to match webrtc's "track_added" event and SFU's "TrackPublishedEvent" together to get the full info about a video track:
Sometimes,
StreamEdgetrack state becomes inconsistent which leads to TimeoutErrors on track resolution or "track not found in map" warnings when the track is already unpublished but wasn't resolved in first place.In this PR, I'll try to address this race conditions.
Changes