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

Scheduler: Race condition in releasing mutex for overlapping/identical events. #53342

Open
yoosefi opened this issue Oct 29, 2024 · 2 comments
Open

Comments

@yoosefi
Copy link

yoosefi commented Oct 29, 2024

Laravel Version

11.26.0

PHP Version

8.2.24 (irrelevant)

Database Driver & Version

SQLite 3.37.2 (irrelevant)

Description

When scheduling events in App\Console\Kernel::schedule(), and having an event not overlap itself for M minutes, if the event takes too long and is overlapped but completes first, it releases the mutex that is owned by the newer event, allowing the newer event to be overlapped immediately regardless of M.

Steps To Reproduce

  1. Schedule a command (Event-X)
  2. Have the command not overlap itself for M minutes; it holds a mutex for M minutes.
  3. Command takes longer than M minutes to complete, mutex expires.
  4. Command is re-run (Event-Y), overlapping Event-X (this is fine), and Event-Y now has the mutex.
  5. Event-X completes, and CacheEventMutex::forget() force-releases Event-Y's mutex, which may still be running (this is not fine).
  6. Event-Z can now immediately overlap Event-Y, regardless of M

Solution:

  1. CacheEventMutex::create() must provide an $owner to the lock, or retrieve the owner after creation. This prevents a random identifier from being used during lock creation and then immediately lost. Where this value lives can be discussed. It only needs to exist in the current runtime.
  2. CacheEventMutex::forget() must not force-release if an owner is already known from create(). This allows ScheduleClearCacheCommand to still force-release by not giving one.
  3. ScheduleFinishCommand should accept an owner identifier to ensure that the owners are the same when attempting mutex release for background jobs. The owner, as determined in the same runtime, should be given by CommandBuilder::buildBackgroundCommand().
  4. Failure to release upon job completion may be silent; the event is done, and overlap was allowed, no exception is necessary when another process holds the mutex.
@yoosefi
Copy link
Author

yoosefi commented Oct 30, 2024

CacheEventMutex::forget() should use LockProvider::restoreLock() for release() when an owner is known, since that's meant for blind/unforced lock-resumption across sequential processes (or so I believe from what I can find about it on the net, it's not really documented).

Copy link

github-actions bot commented Nov 4, 2024

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants