Repository navigation
Conversation
No changeset foundThis PR modifies versioned packages but doesn't include a changeset. The following packages require a version bump:
A package must be bumped when its own files change, and whenever a package it depends on is bumped (so downstream consumers get a matching release). Click here to create a changeset for the missing packages The link pre-populates a changeset file with If this change doesn't require a version bump, add the |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| Ok(info) => info, | ||
| Err(err) => { | ||
| log::warn!("Republish failed for track {}: {}", event.handle, err); | ||
| _ = state_tx.send(PublishState::Unpublished); |
There was a problem hiding this comment.
🟡 Republish failure change lacks changeset
When RepublishTracks fails, the new behavior unpublishes the track without a changeset. The release workflow lacks the required note for this behavior change.
Learn more
The repository requires a changeset for every PR and lists affected crates for version bumps. This change alters how data tracks behave when an SFU rejects republishing, but the PR contains no changeset. The documented release note and bump information are absent.
Example: A caller keeps a local data track through reconnect. After the SFU rejects republishing, it becomes unpublished; the release notes have no entry describing the change.
Recommended fix: Add a changeset using knope document-change or create one in /.changeset. List livekit-datatrack and any consumer crates requiring a bump, with a single unwrapped plain-prose summary.
Was this helpful? React with 👍 or 👎 to provide feedback.
1egoman
left a comment
1egoman
left a comment
There was a problem hiding this comment.
Assuming the test captures the scenario, this makes sense to me.
I propegated this to web here as well: livekit/client-sdk-js#2170. But it looks like the web might have been handling this case already? Either way, adding in the new test you added here to web as well is a good idea.
| async fn test_republish_rejected() { | ||
| let options = ManagerOptions { encryption_provider: None }; | ||
| let (manager, input, mut output) = Manager::new(options); | ||
| tokio::spawn(manager.run()); | ||
|
|
||
| let (result_tx, result_rx) = oneshot::channel(); | ||
| let event = PublishRequest { options: DataTrackOptions::new("track"), result_tx }; | ||
| input.send(event.into()).unwrap(); | ||
|
|
||
| let event = expect_event!(output, OutputEvent::SfuPublishRequest); | ||
| let handle = event.handle; | ||
|
|
||
| let info = DataTrackInfo { | ||
| sid: RwLock::new(Faker.fake()).into(), | ||
| pub_handle: handle, | ||
| name: "track".into(), | ||
| uses_e2ee: false, | ||
| schema: None, | ||
| frame_encoding: None, | ||
| }; | ||
| let event = SfuPublishResponse { handle, result: Ok(info) }; | ||
| input.send(event.into()).unwrap(); | ||
| let track = result_rx.await.unwrap().unwrap(); | ||
|
|
||
| // Simulate reconnect | ||
| input.send(InputEvent::RepublishTracks).unwrap(); | ||
| expect_event!(output, OutputEvent::SfuPublishRequest); | ||
|
|
||
| // SFU rejects the republish | ||
| let event = SfuPublishResponse { handle, result: Err(PublishError::NotAllowed) }; | ||
| input.send(event.into()).unwrap(); | ||
|
|
||
| // Track is unpublished | ||
| timeout(Duration::from_millis(500), track.wait_for_unpublish()).await.unwrap(); | ||
| assert!(!track.is_published()); | ||
| let err = track.try_push(vec![0xFF].into()).unwrap_err(); | ||
| assert!(matches!(err.reason(), PushFrameErrorReason::TrackUnpublished)); | ||
| } |
There was a problem hiding this comment.
Note that I primary reviewing this test as part of this change.
This branch has not been deployed
2 participants
No description provided.