YTMD/youtube-music
1
0
Fork 0
Code Issues 445 Pull requests 34 Projects Releases 87 Packages Wiki Activity Actions

fix(music-together): Improve auto-reconnect to fix random disconnections #3688

Open
BoghosDavtyan wants to merge 5 commits from BoghosDavtyan/master into master
pull from: BoghosDavtyan/master
merge into: YTMD:master
Conversation 2 Commits 5 Files changed 1 +83 -36
BoghosDavtyan commented 2025-07-26 15:36:24 +00:00 (Migrated from github.com)
Copy link

Hello! As someone new to TypeScript, I've done my best to address an issue I was having with the music-together plugin and would appreciate your feedback.

Problem/Motivation

I noticed that when using the music-together feature, the connection would often drop randomly after a period of time (ranging from a few minutes to a few hours). This required manually re-entering the host ID on all connected clients, which was disruptive. The existing error handling didn't seem to account for recoverable network issues, and there wasn't much logging to help diagnose the problem.

Solution

I've modified the connection.ts file to build a more resilient connection handling system. The key changes are:

  • Automatic Reconnection Loop: Implemented a reconnectLoop that automatically tries to re-establish a connection to the PeerJS server if it's lost unexpectedly.
  • Smarter Error Handling: The main peer.on('error', ...) handler now distinguishes between recoverable network errors (like PeerUnavailable or ServerError) and fatal errors. It will only attempt to reconnect on recoverable errors.
  • State Management: Added isManualDisconnect and isReconnecting flags to ensure the reconnection logic only runs when it should (i.e., not after a user manually disconnects) and to prevent multiple loops from running at once.
  • Enhanced Debugging: Added several console.log and console.warn statements to provide clear feedback on the connection status, disconnection reasons, and reconnection attempts. This should make future troubleshooting much easier.

Testing

  • I have tested these changes extensively on Windows.
  • I hosted a music-together session and left it running for several hours, observing that it successfully recovered from network fluctuations without requiring a manual reconnect.
  • As I am new to TypeScript and have only tested on Windows, I would greatly appreciate feedback and testing on macOS and Linux to ensure there are no platform-specific issues.

Thank you for considering my contribution!

Hello! As someone new to TypeScript, I've done my best to address an issue I was having with the `music-together` plugin and would appreciate your feedback. #### **Problem/Motivation** I noticed that when using the `music-together` feature, the connection would often drop randomly after a period of time (ranging from a few minutes to a few hours). This required manually re-entering the host ID on all connected clients, which was disruptive. The existing error handling didn't seem to account for recoverable network issues, and there wasn't much logging to help diagnose the problem. #### **Solution** I've modified the `connection.ts` file to build a more resilient connection handling system. The key changes are: * **Automatic Reconnection Loop:** Implemented a `reconnectLoop` that automatically tries to re-establish a connection to the PeerJS server if it's lost unexpectedly. * **Smarter Error Handling:** The main `peer.on('error', ...)` handler now distinguishes between recoverable network errors (like `PeerUnavailable` or `ServerError`) and fatal errors. It will only attempt to reconnect on recoverable errors. * **State Management:** Added `isManualDisconnect` and `isReconnecting` flags to ensure the reconnection logic only runs when it should (i.e., not after a user manually disconnects) and to prevent multiple loops from running at once. * **Enhanced Debugging:** Added several `console.log` and `console.warn` statements to provide clear feedback on the connection status, disconnection reasons, and reconnection attempts. This should make future troubleshooting much easier. #### **Testing** * I have tested these changes extensively on **Windows**. * I hosted a `music-together` session and left it running for several hours, observing that it successfully recovered from network fluctuations without requiring a manual reconnect. * As I am new to TypeScript and have only tested on Windows, I would greatly appreciate feedback and testing on **macOS** and **Linux** to ensure there are no platform-specific issues. Thank you for considering my contribution!
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-07-28 13:30:59 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy link

Pull Request Overview

This PR improves the resilience of the music-together plugin by implementing automatic reconnection logic to handle random connection drops. The changes focus on distinguishing between recoverable network errors and fatal errors, implementing a smart reconnection loop, and adding better state management to prevent duplicate reconnection attempts.

  • Adds automatic reconnection loop with configurable retry logic for network interruptions
  • Implements state flags to prevent reconnection during manual disconnects and avoid duplicate loops
  • Enhances error handling to distinguish between recoverable and fatal PeerJS errors
  • Improves debugging with additional console logging for connection status tracking
## Pull Request Overview This PR improves the resilience of the `music-together` plugin by implementing automatic reconnection logic to handle random connection drops. The changes focus on distinguishing between recoverable network errors and fatal errors, implementing a smart reconnection loop, and adding better state management to prevent duplicate reconnection attempts. - Adds automatic reconnection loop with configurable retry logic for network interruptions - Implements state flags to prevent reconnection during manual disconnects and avoid duplicate loops - Enhances error handling to distinguish between recoverable and fatal PeerJS errors - Improves debugging with additional console logging for connection status tracking
src/plugins/music-together/connection.ts
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-07-28 13:30:59 +00:00
Copy link

[nitpick] The error type comparison should be extracted into a helper function or constant array to improve readability and maintainability. Consider creating const RECOVERABLE_ERROR_TYPES = [PeerErrorType.Network, PeerErrorType.PeerUnavailable, PeerErrorType.ServerError] and using RECOVERABLE_ERROR_TYPES.includes(err.type).

    const RECOVERABLE_ERROR_TYPES = [
      PeerErrorType.Network,
      PeerErrorType.PeerUnavailable,
      PeerErrorType.ServerError,
    ];

    this.peer.on('error', (err) => {
      // Only attempt to reconnect on recoverable network errors
      if (RECOVERABLE_ERROR_TYPES.includes(err.type)) {
[nitpick] The error type comparison should be extracted into a helper function or constant array to improve readability and maintainability. Consider creating `const RECOVERABLE_ERROR_TYPES = [PeerErrorType.Network, PeerErrorType.PeerUnavailable, PeerErrorType.ServerError]` and using `RECOVERABLE_ERROR_TYPES.includes(err.type)`. ```suggestion const RECOVERABLE_ERROR_TYPES = [ PeerErrorType.Network, PeerErrorType.PeerUnavailable, PeerErrorType.ServerError, ]; this.peer.on('error', (err) => { // Only attempt to reconnect on recoverable network errors if (RECOVERABLE_ERROR_TYPES.includes(err.type)) { ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-07-28 13:30:59 +00:00
Copy link

The isManualDisconnect flag is never reset to false, which means after a manual disconnect, automatic reconnection will be permanently disabled even if the user tries to connect again. The flag should be reset when starting a new connection.

The `isManualDisconnect` flag is never reset to `false`, which means after a manual disconnect, automatic reconnection will be permanently disabled even if the user tries to connect again. The flag should be reset when starting a new connection.
src/plugins/music-together/connection.ts
@ -84,0 +114,4 @@
}
console.log(
'Music Together: Attempting to reconnect to PeerJS server...',
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-07-28 13:30:58 +00:00
Copy link

The while loop condition !this.peer.destroyed could create an infinite loop if the peer is never destroyed but also never successfully reconnects. Consider adding a maximum retry count or timeout mechanism to prevent infinite loops.

      const MAX_RETRIES = 10;
      let retryCount = 0;
      while (!this.peer.destroyed && retryCount < MAX_RETRIES) {
The while loop condition `!this.peer.destroyed` could create an infinite loop if the peer is never destroyed but also never successfully reconnects. Consider adding a maximum retry count or timeout mechanism to prevent infinite loops. ```suggestion const MAX_RETRIES = 10; let retryCount = 0; while (!this.peer.destroyed && retryCount < MAX_RETRIES) { ```
src/plugins/music-together/connection.ts
@ -85,2 +134,4 @@
this._mode = 'host';
this.isReconnecting = false;
this.waitOpen.resolve(id);
console.log('Music Together: PeerJS connection opened with ID:', id);
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-07-28 13:30:59 +00:00
Copy link

[nitpick] The hardcoded delay value 10000 should be extracted to a named constant like RECONNECT_RETRY_DELAY_MS = 10000 to improve readability and make it easier to adjust if needed.

          await delay(RECONNECT_RETRY_DELAY_MS);
[nitpick] The hardcoded delay value `10000` should be extracted to a named constant like `RECONNECT_RETRY_DELAY_MS = 10000` to improve readability and make it easier to adjust if needed. ```suggestion await delay(RECONNECT_RETRY_DELAY_MS); ```
src/plugins/music-together/connection.ts
@ -115,0 +160,4 @@
err.type === PeerErrorType.ServerError)
) {
reconnectLoop(err);
} else {
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-07-28 13:30:59 +00:00
Copy link

The change from throwing an error to silently returning when already disconnected could mask programming errors where disconnect() is called unexpectedly. Consider logging a warning or keeping the original behavior for better debugging.

    if (this._mode === 'disconnected') {
      console.warn('disconnect() called while already disconnected.');
      return;
    }
The change from throwing an error to silently returning when already disconnected could mask programming errors where disconnect() is called unexpectedly. Consider logging a warning or keeping the original behavior for better debugging. ```suggestion if (this._mode === 'disconnected') { console.warn('disconnect() called while already disconnected.'); return; } ```
copilot-pull-request-reviewer[bot] (Migrated from github.com) reviewed 2025-09-07 05:57:40 +00:00
copilot-pull-request-reviewer[bot] (Migrated from github.com) left a comment
Copy link

Pull Request Overview

This PR improves the reliability of the music-together plugin by implementing automatic reconnection logic to handle random disconnections that were disrupting user sessions.

Key changes:

  • Added automatic reconnection loop that attempts to re-establish connection when network issues occur
  • Implemented smarter error handling that distinguishes between recoverable and fatal errors
  • Added connection state management with flags to prevent multiple reconnection attempts and respect manual disconnections

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

## Pull Request Overview This PR improves the reliability of the music-together plugin by implementing automatic reconnection logic to handle random disconnections that were disrupting user sessions. Key changes: - Added automatic reconnection loop that attempts to re-establish connection when network issues occur - Implemented smarter error handling that distinguishes between recoverable and fatal errors - Added connection state management with flags to prevent multiple reconnection attempts and respect manual disconnections --- <sub>**Tip:** Customize your code reviews with copilot-instructions.md. <a href="/th-ch/youtube-music/new/master/.github?filename=copilot-instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Create the file</a> or <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">learn how to get started</a>.</sub>
src/plugins/music-together/connection.ts
@ -101,3 +145,1 @@
this.peer.disconnect();
this.peer.destroy();
this.peer.on('disconnected', () => {
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-09-07 05:57:39 +00:00
Copy link

This infinite loop could run indefinitely if the peer never becomes disconnected or destroyed, potentially consuming CPU resources. Consider adding a maximum retry count or timeout mechanism to prevent endless reconnection attempts.

This infinite loop could run indefinitely if the peer never becomes disconnected or destroyed, potentially consuming CPU resources. Consider adding a maximum retry count or timeout mechanism to prevent endless reconnection attempts.
src/plugins/music-together/connection.ts
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-09-07 05:57:40 +00:00
Copy link

The 'disconnected' event handler only logs a warning but doesn't trigger any reconnection logic. This inconsistency with the PR's stated goal of automatic reconnection could be confusing. Consider either triggering reconnectLoop here or removing the misleading warning message.

      reconnectLoop();
The 'disconnected' event handler only logs a warning but doesn't trigger any reconnection logic. This inconsistency with the PR's stated goal of automatic reconnection could be confusing. Consider either triggering reconnectLoop here or removing the misleading warning message. ```suggestion reconnectLoop(); ```
src/plugins/music-together/connection.ts
@ -142,1 +190,4 @@
this.isManualDisconnect = true;
this.isReconnecting = false;
this._mode = 'disconnected';
copilot-pull-request-reviewer[bot] (Migrated from github.com) commented 2025-09-07 05:57:39 +00:00
Copy link

The 'close' event typically indicates a deliberate disconnection, but this code triggers a reconnection attempt. This could cause unwanted reconnections when the peer is intentionally closed. Consider only triggering reconnectLoop on the 'disconnected' event or adding additional checks.

    // Removed reconnectLoop on 'close' event to avoid unwanted reconnections.
The 'close' event typically indicates a deliberate disconnection, but this code triggers a reconnection attempt. This could cause unwanted reconnections when the peer is intentionally closed. Consider only triggering reconnectLoop on the 'disconnected' event or adding additional checks. ```suggestion // Removed reconnectLoop on 'close' event to avoid unwanted reconnections. ```
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin BoghosDavtyan/master:BoghosDavtyan/master
git switch BoghosDavtyan/master

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff BoghosDavtyan/master
git switch BoghosDavtyan/master
git rebase master
git switch master
git merge --ff-only BoghosDavtyan/master
git switch BoghosDavtyan/master
git rebase master
git switch master
git merge --no-ff BoghosDavtyan/master
git switch master
git merge --squash BoghosDavtyan/master
git switch master
git merge --ff-only BoghosDavtyan/master
git switch master
git merge BoghosDavtyan/master
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
awaiting-reply

Awaiting reply

  
breaking changes

   
bug

Something isn't working

  
cannot-reproduce

Bug cannot be reproduced

  
dependencies

Pull requests that update a dependency file

  
documentation

   
duplicate

This issue or pull request already exists

  
electron-issue

It's an Electron or Electron related dependencies issue (not YTM-Desktop issue)

  
enhancement

New feature or request

  
fix-available

A fix to the issue is available in a new version

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
javascript

Pull requests that update javascript code

  
need more information

Need more information about the issue

  
need rebase

This plugin need rebase

  
official-youtube-music-issue

It's an YouTube's issue (not YTM-Desktop issue)

  
plugin request

An issue/discussion requesting for a new plugin or new functionality to an existing plugin.

  
question

Further information is requested

  
release

Release

  
security

   
stale

   
Status: blocked

There are something to do before this

  
typo

   
wontfix

This will not be worked on

  
ytmd-issue

An issue/discussion about ytm desktop, unrelated to th-ch/youtube-music

No labels
awaiting-reply
breaking changes
bug
cannot-reproduce
dependencies
documentation
duplicate
electron-issue
enhancement
fix-available
good first issue
help wanted
invalid
javascript
need more information
need rebase
official-youtube-music-issue
plugin request
question
release
security
stale
Status: blocked
typo
wontfix
ytmd-issue
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: YTMD/youtube-music#3688
No description provided.
Delete branch "BoghosDavtyan/master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?