Skip to content

fix(video-streamer): improve unified shutdown correctness#1703

Merged
irvingouj@Devolutions (irvingoujAtDevolution) merged 3 commits intomasterfrom
refactor/streamer-unified-shutdown
Mar 10, 2026
Merged

fix(video-streamer): improve unified shutdown correctness#1703
irvingouj@Devolutions (irvingoujAtDevolution) merged 3 commits intomasterfrom
refactor/streamer-unified-shutdown

Conversation

@irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) commented Mar 6, 2026 •
edited
Loading

Summary

  • Replace scattered shutdown mechanisms (Arc<Notify> stop_notifier, mpsc error channel, implicit channel close, handle.abort()) with a single tokio::sync::watch channel as the source of truth
  • Handle task now signals ClientDisconnected on client disconnect (was silent before)
  • Handle task uses select! on ws_frame.next() for shutdown awareness (no longer blocks on silent clients)
  • Handle task best-effort delivers ServerMessage::End before socket shutdown
  • Bridge task is aborted on webm_stream exit to prevent control_task from hanging indefinitely
  • Add missing tokio time feature (pre-existing issue)

🤖 Generated with Claude Code

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

Let maintainers know that an action is required on their side

  • Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)

  • Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release

  • Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)

  • Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

@irvingoujAtDevolution @claude
feat(video-streamer): add codec-aware VP9 keyframe detection for sess…
5d5b7a1 
…ion shadowing

The existing keyframe detection only handled VP8 frames. This adds proper VP9
keyframe detection based on the VP9 bitstream specification, supporting all
profiles (0-3). The codec type is now threaded through the iterator and block
tag layers so keyframe checks use the correct codec-specific logic. Also sets
VpxEncoderPreset::BestPerformance for improved re-encoding throughput.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@irvingoujAtDevolution @claude
refactor(video-streamer): unify shutdown signals with watch channel
b60bd2a 
Replace scattered shutdown mechanisms (Arc<Notify> stop_notifier,
mpsc error channel, implicit channel close, handle.abort()) with a
single tokio::sync::watch channel as the source of truth.

Key changes:
- Add StreamShutdown enum (Running/ClientDisconnected/ExternalShutdown/Error)
- Bridge external Arc<Notify> into watch channel at webm_stream level
- Handle task now signals ClientDisconnected on client disconnect
- Handle task uses select! on ws_frame.next() for shutdown awareness
- Handle task best-effort delivers ServerMessage::End before socket shutdown
- control_task simplified to single shutdown_rx.changed() await
- Abort bridge task on webm_stream exit to prevent control_task hang
- Add tokio "time" feature (was missing, pre-existing issue)

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@irvingoujAtDevolution @claude
fix(video-streamer): improve unified shutdown correctness
d30aedd 
- Send Error (not End) to client when shutdown is caused by a stream error
- Use AbortOnDrop guard for bridge task to handle early returns/bails
- Remove dead UserFriendlyError::UnexpectedEOF variant

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) marked this pull request as ready for review March 9, 2026 21:43
Copilot AI reviewed Mar 9, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the video-streamer shutdown orchestration to use a single tokio::sync::watch channel as the “source of truth”, aiming to eliminate scattered shutdown/error mechanisms and improve task shutdown behavior (including awareness of silent clients and disconnects).

Changes:

  • Replace mixed shutdown/error signaling with a unified watch-based StreamShutdown state.
  • Update sending/control tasks to react to shutdown via select! and propagate client disconnects into shutdown state.
  • Enable Tokio’s time feature to support new tokio::time::* usage.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
crates/video-streamer/src/streamer/protocol.rs Adjust UserFriendlyError traits/variants for use in shutdown signaling.
crates/video-streamer/src/streamer/mod.rs Introduce StreamShutdown watch channel; refactor tasks to use it and improve shutdown coordination.
crates/video-streamer/Cargo.toml Add Tokio time feature required by new timeouts/sleeps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

CBenoit
Benoît Cortier (CBenoit) approved these changes Mar 10, 2026
View reviewed changes
Copy link
Member

@CBenoit Benoît Cortier (CBenoit) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I also asked another review batch from Copilot.

@CBenoit
Copy link
Member

Benoît Cortier (CBenoit) commented Mar 10, 2026

Note: if you use refactor for your commit, this will NOT be part of the changelog. If you are fixing a problem, use fix. It’s otherwise fine if no observable behavior is changed.

Copilot AI reviewed Mar 10, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) changed the title refactor(video-streamer): unify shutdown signals with watch channel fix(video-streamer): improve unified shutdown correctness Mar 10, 2026
@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) merged commit 9e417f9 into master Mar 10, 2026
48 checks passed
@irvingoujAtDevolution irvingouj@Devolutions (irvingoujAtDevolution) deleted the refactor/streamer-unified-shutdown branch March 10, 2026 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@CBenoit Benoît Cortier (CBenoit) CBenoit approved these changes

@allan2 Allan Zhang (allan2) Awaiting requested review from allan2

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@irvingoujAtDevolution @CBenoit