Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix mutexName inconsistency caused by different PHP binary paths on multiple servers #53811

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

waska14
Copy link
Contributor

@waska14 waska14 commented Dec 9, 2024

Problem Description:

Our production environment handles around 200 million requests daily, and we initially had three Ubuntu 22.04 servers running PHP 8.3, where the PHP executable path was:

/usr/bin/php8.3

Recently, we decided to expand the infrastructure and added a fourth server running AlmaLinux 9.5. While the setup was consistent across servers, the PHP executable path on the new server differed:

/opt/remi/php83/root/usr/bin/php

This caused an issue with the mutexName function when using Laravel’s onOneServer()->withoutOverlapping() feature for scheduling commands. The $this->command string varied between servers as follows:

  • Existing servers (Ubuntu):
'/usr/bin/php8.3' 'artisan' my-command-signature-here
  • New server (AlmaLinux):
'/opt/remi/php83/root/usr/bin/php' 'artisan' my-command-signature-here

Root Cause:

The mutexName function generates a unique name based on the $this->command. Since the PHP executable paths differed across servers, the resulting mutex name was inconsistent. This led to the same scheduled command being executed on multiple servers simultaneously, despite using ->onOneServer()->withoutOverlapping().

Real-Life Impact:

For example, when scheduling the following command in Kernel.php:

$scheduler->command('my-command-signature-here')->onOneServer()->withoutOverlapping();

The inconsistency in mutexName caused the command to run:

  • Once on the new server (AlmaLinux).
  • And once on one of the existing servers (Ubuntu), selected at random.

This behavior defeats the purpose of ->withoutOverlapping(), which was critical in our case (high-traffic production environment).

Solution:

To resolve this issue, I updated the code to strip the PHP binary path from the $this->command. By doing this, the mutexName function will now generate consistent names regardless of the PHP executable's location or version.

For example:

  • Input command:
'/usr/bin/php8.3' 'artisan' my-command-signature-here

or

'/opt/remi/php83/root/usr/bin/php' 'artisan' my-command-signature-here
  • After modification:
'artisan' my-command-signature-here

The mutex name will now be derived only from the relevant parts of the command (i.e., 'artisan' my-command-signature-here), ensuring consistency across servers.

Testing Considerations:

Unfortunately, testing this specific scenario programmatically is challenging because it requires:

  • Multiple servers with different operating systems.
  • Differing PHP binary paths.

Given these constraints, I was unable to include automated tests for this PR. However, the issue has been thoroughly tested in our production environment, and the proposed change resolves the problem reliably.

@taylorotwell
Copy link
Member

taylorotwell commented Dec 10, 2024

Is this going to actually be robust? Not every scheduled command calls PHP for example... Schedule::exec('some arbitrary linux command') ... the explode and string slicing wouldn't seem correct for these cases?

For reference, you can specify a mutex name like this: Schedule::exec('')->createMutexNameUsing(fn () => 'some-mutex-name').

Please mark as ready for review when the requested changes have been made.

@taylorotwell taylorotwell marked this pull request as draft December 10, 2024 15:10
@waska14
Copy link
Contributor Author

waska14 commented Dec 13, 2024

@taylorotwell Hmm, makes sense.
I've never used Linux commands in the Laravel Scheduler before.

You were right.

I noticed something similar in ScheduleListCommand.php, so I added a new method to Event.php and used it in both cases.

I hope this approach is robust enough.

@waska14 waska14 marked this pull request as ready for review December 13, 2024 21:40
@taylorotwell taylorotwell merged commit 3903c5d into laravel:11.x Dec 16, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants