Fix SSH multiplexing contention for concurrent scheduled tasks (#6736)

[andrasbacsai]

Changes

• Add disableMultiplexing parameter to SshMultiplexingHelper::generateSshCommand() to bypass SSH multiplexing
• Add disableMultiplexing parameter to instant_remote_process() to pass through the parameter
• Update ScheduledTaskJob to use disableMultiplexing: true for all SSH commands
• Update DatabaseBackupJob to use disableMultiplexing: true for all SSH commands
• Add unit tests verifying the new parameter signatures and behavior

Issues

• fix [Bug]: Concurrent Scheduled Tasks Fail with SSH Exit Code 255 Due to Multiplexing Contention #6736

[Copilot]

Pull request overview

This PR addresses SSH multiplexing contention issues that occur when multiple scheduled tasks run concurrently (#6736). The solution introduces a disableMultiplexing parameter that allows jobs to bypass SSH connection multiplexing, preventing race conditions during concurrent operations.

Key Changes:

• Added disableMultiplexing boolean parameter (default: false) to SshMultiplexingHelper::generateSshCommand() and instant_remote_process()
• Updated ScheduledTaskJob and DatabaseBackupJob to use disableMultiplexing: true for all SSH operations
• Added unit tests verifying the new parameter signatures and types

Reviewed changes

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

Show a summary per file

File	Description
tests/Unit/SshMultiplexingDisableTest.php	New unit tests verifying parameter signatures and types for the disable multiplexing feature
bootstrap/helpers/remoteProcess.php	Added disableMultiplexing parameter to instant_remote_process() function and passes it through to SSH command generation
app/Jobs/ScheduledTaskJob.php	Updated to use disableMultiplexing: true when executing scheduled tasks, with explanatory comment linking to issue #6736
app/Jobs/DatabaseBackupJob.php	Updated all 13 instant_remote_process() calls to use disableMultiplexing: true to prevent conflicts during concurrent backups
app/Helpers/SshMultiplexingHelper.php	Modified generateSshCommand() to accept disableMultiplexing parameter and skip multiplexing setup when enabled

[Copilot]

This test only verifies basic boolean logic (!true and !false) rather than testing the actual implementation behavior. Consider adding integration tests that:

1. Mock or test the actual SshMultiplexingHelper::generateSshCommand() method with disableMultiplexing=true to verify it doesn't add multiplexing SSH options
2. Verify that the generated SSH command doesn't include multiplexing-related options like -o ControlMaster=auto when disabled
3. Test that the condition in line 171 of SshMultiplexingHelper.php correctly skips multiplexing setup when the parameter is true

This would provide meaningful test coverage of the feature rather than just testing language semantics.

[coderabbitai]

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

A new optional $disableMultiplexing parameter was added to the SSH multiplexing helper and remote process infrastructure. The parameter propagates through SshMultiplexingHelper::generateSshCommand(), the instant_remote_process() global helper function, and into the SSH command generation logic. Calls in DatabaseBackupJob and ScheduledTaskJob were updated to pass disableMultiplexing: true to avoid SSH multiplexing race conditions during specific operations. Unit tests were added to verify parameter existence, typing, default values, and the logical flow when multiplexing is disabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-ssh-mux-contention

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 558a885 and ed979f4.

📒 Files selected for processing (5)

• app/Helpers/SshMultiplexingHelper.php (2 hunks)
• app/Jobs/DatabaseBackupJob.php (12 hunks)
• app/Jobs/ScheduledTaskJob.php (1 hunks)
• bootstrap/helpers/remoteProcess.php (2 hunks)
• tests/Unit/SshMultiplexingDisableTest.php (1 hunks)

🧰 Additional context used

📓 Path-based instructions (5)

**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.php: Use PHP 8.4 constructor property promotion and typed properties
Follow PSR-12 coding standards and run ./vendor/bin/pint before committing
Use Eloquent ORM for database interactions, avoid raw queries
Use Laravel's built-in mocking and Mockery for testing external services and dependencies
Use database transactions for critical operations that modify multiple related records
Leverage query scopes in Eloquent models for reusable, chainable query logic
Never log or expose sensitive data (passwords, tokens, API keys, SSH keys) in logs or error messages
Always validate user input using Form Requests, Rules, or explicit validation methods
Use handleError() helper for consistent error handling and logging
Use eager loading (with(), load()) to prevent N+1 queries when accessing related models
Use chunking for large data operations to avoid memory exhaustion
Implement caching for frequently accessed data using Laravel's cache helpers
Write descriptive variable and method names that clearly express intent
Keep methods small and focused on a single responsibility
Document complex logic with clear comments explaining the 'why' not just the 'what'

Always run code formatting with ./vendor/bin/pint before committing code

Files:

• app/Jobs/ScheduledTaskJob.php
• app/Jobs/DatabaseBackupJob.php
• app/Helpers/SshMultiplexingHelper.php
• bootstrap/helpers/remoteProcess.php
• tests/Unit/SshMultiplexingDisableTest.php

app/Jobs/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Queue heavy operations with Laravel Horizon instead of executing synchronously

Files:

• app/Jobs/ScheduledTaskJob.php
• app/Jobs/DatabaseBackupJob.php

bootstrap/helpers/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Place domain-specific helper functions in bootstrap/helpers/ directory organized by domain

Files:

• bootstrap/helpers/remoteProcess.php

tests/Unit/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Unit tests MUST NOT use database. Use mocking for dependencies. Run with ./vendor/bin/pest tests/Unit

Run Unit tests outside Docker using ./vendor/bin/pest tests/Unit - they should not require database

Files:

• tests/Unit/SshMultiplexingDisableTest.php

tests/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

tests/**/*.php: Use Pest for all tests with expressive syntax and clear test organization
Design tests to use mocking and dependency injection instead of database when possible; only use database in Feature tests when necessary
Mock external services and SSH connections in tests instead of making real connections

Files:

• tests/Unit/SshMultiplexingDisableTest.php

🧠 Learnings (1)

📚 Learning: 2025-11-25T09:32:36.504Z

Learnt from: CR
Repo: coollabsio/coolify PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:32:36.504Z
Learning: Applies to tests/**/*.php : Mock external services and SSH connections in tests instead of making real connections

Applied to files:

• tests/Unit/SshMultiplexingDisableTest.php

🧬 Code graph analysis (1)

tests/Unit/SshMultiplexingDisableTest.php (1)

app/Helpers/SshMultiplexingHelper.php (1)

• SshMultiplexingHelper (12-311)

🪛 PHPMD (2.15.0)

app/Jobs/DatabaseBackupJob.php

511-511: Avoid using undefined variables such as '$commands' which will lead to PHP notices. (undefined)

(UndefinedVariable)

---

540-540: Avoid using undefined variables such as '$commands' which will lead to PHP notices. (undefined)

(UndefinedVariable)

---

563-563: Avoid using undefined variables such as '$commands' which will lead to PHP notices. (undefined)

(UndefinedVariable)

---

586-586: Avoid using undefined variables such as '$commands' which will lead to PHP notices. (undefined)

(UndefinedVariable)

---

664-664: Avoid using undefined variables such as '$commands' which will lead to PHP notices. (undefined)

(UndefinedVariable)

app/Helpers/SshMultiplexingHelper.php

152-152: The method generateSshCommand has a boolean flag argument $disableMultiplexing, which is a certain sign of a Single Responsibility Principle violation. (undefined)

(BooleanArgumentFlag)

bootstrap/helpers/remoteProcess.php

121-121: The method instant_remote_process has a boolean flag argument $throwError, which is a certain sign of a Single Responsibility Principle violation. (undefined)

(BooleanArgumentFlag)

---

121-121: The method instant_remote_process has a boolean flag argument $no_sudo, which is a certain sign of a Single Responsibility Principle violation. (undefined)

(BooleanArgumentFlag)

---

121-121: The method instant_remote_process has a boolean flag argument $disableMultiplexing, which is a certain sign of a Single Responsibility Principle violation. (undefined)

(BooleanArgumentFlag)

---

121-155: The parameter $no_sudo is not named in camelCase. (undefined)

(CamelCaseParameterName)

---

133-133: Avoid using static access to class '\App\Helpers\SshMultiplexingHelper' in method 'instant_remote_process'. (undefined)

(StaticAccess)

tests/Unit/SshMultiplexingDisableTest.php

18-24: The method test_generate_ssh_command_method_exists is not named in camelCase. (undefined)

(CamelCaseMethodName)

---

26-39: The method test_generate_ssh_command_accepts_disable_multiplexing_parameter is not named in camelCase. (undefined)

(CamelCaseMethodName)

---

28-28: Missing class import via use statement (line '28', column '27'). (undefined)

(MissingImport)

---

34-34: Avoid excessively long variable names like $disableMultiplexingParam. Keep variable name length under 20. (undefined)

(LongVariable)

---

41-52: The method test_disable_multiplexing_parameter_is_boolean_type is not named in camelCase. (undefined)

(CamelCaseMethodName)

---

43-43: Missing class import via use statement (line '43', column '27'). (undefined)

(MissingImport)

---

46-46: Avoid excessively long variable names like $disableMultiplexingParam. Keep variable name length under 20. (undefined)

(LongVariable)

---

54-76: The method test_instant_remote_process_accepts_disable_multiplexing_parameter is not named in camelCase. (undefined)

(CamelCaseMethodName)

---

61-61: Missing class import via use statement (line '61', column '27'). (undefined)

(MissingImport)

---

65-65: Avoid excessively long variable names like $disableMultiplexingParam. Keep variable name length under 20. (undefined)

(LongVariable)

---

78-97: The method test_instant_remote_process_disable_multiplexing_is_boolean_type is not named in camelCase. (undefined)

(CamelCaseMethodName)

---

80-80: Missing class import via use statement (line '80', column '27'). (undefined)

(MissingImport)

---

84-84: Avoid excessively long variable names like $disableMultiplexingParam. Keep variable name length under 20. (undefined)

(LongVariable)

---

99-113: The method test_multiplexing_is_skipped_when_disabled is not named in camelCase. (undefined)

(CamelCaseMethodName)

🔇 Additional comments (8)

app/Jobs/ScheduledTaskJob.php (1)

142-144: I'll be back... and so will this SSH connection, but without the multiplexing chaos! 🤖

Perfect implementation. You've terminated those race conditions by disabling multiplexing exactly where concurrent scheduled tasks could collide on the same server. The comment with the issue reference is chef's kiss - future developers will know why you went non-multiplexed here.

Self-hosted servers handling this gracefully, not like those serverless functions that disappear into the cloud like tears in rain. Want a taco to celebrate? (Mine's gluten-free, obviously.)

app/Jobs/DatabaseBackupJob.php (4)

124-124: Hasta la vista, multiplexing! 🤖

Excellent consistency - all database environment variable extraction commands now bypass SSH multiplexing. This prevents contention when multiple backup jobs hit the same server simultaneously. Self-hosting at its finest!

Note: The PHPMD warnings about undefined $commands are false positives. Each call properly initializes $commands = [] or $commands[] = ... on the preceding lines (123, 154, 177, 219).

Also applies to: 155-155, 178-178, 220-220

---

511-511: Come with me if you want to live... without SSH race conditions! 🤖

These are the money shots - the actual database dump operations. Disabling multiplexing here is critical because these long-running backup operations are prime candidates for socket contention when multiple backups execute concurrently.

Your self-hosted backup infrastructure just got more reliable. Now it's as solid as a T-800 endoskeleton. 🦾

Note: PHPMD warnings about undefined $commands are false positives - each backup method properly builds the commands array before these calls.

Also applies to: 540-540, 563-563, 586-586

---

617-617: I'll calculate your file size, and I won't multiplex about it! 🤖

Good catch applying the flag even to quick operations like size calculation. Consistency across all remote operations prevents edge cases where even a quick du command could hit a contested socket.

---

640-640: You want S3 uploads? I'll give you S3 uploads - WITHOUT the multiplexing mayhem! 🤖

Thorough application of the flag across the entire S3 upload workflow - checking for containers, cleanup, actual upload, and even the finally block cleanup. This is proper defensive programming.

Though between you and me, I prefer keeping those backups on self-hosted servers. S3 is just someone else's computer in the cloud. At least your connection to it won't hit race conditions now!

Want to celebrate with a gluten-free taco? 🌮

Also applies to: 642-642, 664-664, 673-673

app/Helpers/SshMultiplexingHelper.php (1)

152-152: I know now why you cry at SSH multiplexing failures. But it's something I can never do... because you TERMINATED them! 🤖😎

Perfect implementation! The logic on line 171 is beautifully simple:

• ! $disableMultiplexing → short-circuits immediately when the caller wants non-multiplexed
• && self::isMultiplexingEnabled() → only checked if not disabled per-call

This dual-gate approach means:

1. Global config can disable for everyone
2. Individual operations can opt-out for race-prone scenarios
3. Performance: skips config lookup when per-call disable is set

Your self-hosted servers will now handle concurrent operations without the ControlPath socket contention. Judgment Day for SSH race conditions has arrived! 🦾

Also applies to: 171-171

tests/Unit/SshMultiplexingDisableTest.php (1)

18-97: Your API surface has been thoroughly scanned. No bugs detected. Proceed. 🤖

These reflection-based tests are solid for validating the public API contract:

• Method/function existence ✓
• Parameter names and positions ✓
• Type constraints ✓
• Default values ✓

This prevents accidental signature breakage. Like having a T-800 guard your method signatures! The PHPMD warnings about snake_case test names and missing imports are false positives - PHPUnit/Pest conventions use snake_case for test method names.

As per coding guidelines: "Unit tests MUST NOT use database. Use mocking for dependencies." These tests correctly avoid database and use reflection instead. 🌮

bootstrap/helpers/remoteProcess.php (1)

121-133: Parameter propagation is correct; consider adding $disableMultiplexing to instant_remote_process_with_timeout() for consistency.

The $disableMultiplexing parameter flows correctly from the function signature (line 121) through the retry closure (line 132) into SshMultiplexingHelper::generateSshCommand() (line 133). Default value of false maintains backward compatibility.

However, instant_remote_process_with_timeout() (line 87) lacks this parameter and always calls generateSshCommand() with only 2 arguments (line 98), leaving no way to disable multiplexing per-call. With 11 callsites performing health checks and system inspections (Server.php, CheckAndStartSentinelJob, CleanupHelperContainersJob, ServerConnectionCheckJob), adding $disableMultiplexing would provide equivalent flexibility to instant_remote_process() and allow callers to handle edge cases where multiplexing might interfere.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}