Data track publishing - #991
Conversation
5db4c9f to
8a5d82c
Compare
6bf5d50 to
ba27476
Compare
ba27476 to
45535a7
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| case isActive && response.err != nil: | ||
| m.params.Logger.Warnw("republish failed for data track", response.err, "handle", response.handle) |
There was a problem hiding this comment.
🟡 Failed republish permanently blocks frames
When a reconnect publish request fails, the track remains in stateRepublishing. Rejections and transport errors only log the failure. Every later TryPush returns ErrQueueFull, even after connectivity recovers.
Prompt for agents
Handle every failed republish attempt in datatrack/local.go instead of leaving LocalTrack in stateRepublishing. Both an SFU rejection in LocalManager.resolvePublish and a LocalTransport.SendPublishRequest error in LocalManager.RepublishTracks currently only log. Transition the affected track to a terminal or retryable state, keep LocalManager.active consistent, and ensure callers do not receive ErrQueueFull forever. Add tests for both rejection and transport-send failure after beginRepublish.
Was this helpful? React with 👍 or 👎 to provide feedback.
| switch publishState(t.state.Load()) { | ||
| case stateRepublishing: | ||
| return ErrQueueFull | ||
| case stateUnpublished: | ||
| return ErrUnpublished | ||
| } | ||
|
|
||
| t.mu.Lock() | ||
| packets, err := t.pipeline.processFrame(frame) | ||
| t.mu.Unlock() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| t.manager.params.Transport.SendFrame(packets) |
There was a problem hiding this comment.
🟡 Concurrent unpublish leaks a final frame
When Unpublish races with TryPush, the push can pass its state check before unpublication. markUnpublished does not synchronize with frame processing. The frame is then queued after unpublication while TryPush reports success.
Prompt for agents
Synchronize LocalTrack state transitions with TryPush in datatrack/local.go. The current atomic check occurs before pipeline processing, while markUnpublished and beginRepublish can change state concurrently without taking the track mutex. Ensure no frame can be processed or handed to LocalTransport after an unpublish or republish transition wins, while preserving safe concurrent calls and avoiding lock-order inversions with LocalManager.mu.
Was this helpful? React with 👍 or 👎 to provide feedback.
| schema: options.Schema, | ||
| frameEncoding: options.FrameEncoding, | ||
| } | ||
| if err := m.params.Transport.SendPublishRequest(request.toProto()); err != nil { |
There was a problem hiding this comment.
🟡 Cancellation cannot interrupt connection waits
PublishDataTrack with a short deadline can remain blocked in SendPublishRequest until the engine connection timeout. The transport receives no context, so Publish observes cancellation only after that call returns.
Prompt for agents
Make the publication send path context-aware. LocalManager.Publish currently calls LocalTransport.SendPublishRequest synchronously before waiting on ctx, and localDataTrackTransport waits in RTCEngine.ensurePublisherConnected with the engine timeout. Propagate the caller context through the transport or restructure the connection wait so cancellation and shorter deadlines stop PublishDataTrack promptly without leaving an untracked request. Cover an unavailable publisher with a context deadline shorter than ConnectTimeout.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Add support for publishing data tracks.
Architecture and public API closely match Rust and JS clients:
LocalParticipant.PublishDataTrackAPI.LocalManageris owned by the room to manage internal state and transitions for all local data tracks.Areas to review:
LocalManager) are currently public. Would like to understand if there is a better way to organize packages to avoid this.LocalManageris currently owned byLocalParticipant, but I am not sure this is the right place for it to live.Closes BOT-541