[CLOSED:INTENDED BEHAVIOR] Scheduler: Race condition in releasing mutex for overlapping/identical events.

[yoosefi]

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]

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).

[github-actions]

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!

[rodrigopedra]

@crynobone how does this get fixed by #53640 ?

#53640 was closed as a duplicate of this issue

[crynobone]

Sorry about this @rodrigopedra but it does seem there no progress on this. But a PR to fix the issue is always welcome.

[yoosefi]

Love that this was black holed and closed as a duplicate of itself. PEBKAC

[crynobone]

@yoosefi feel free to submit a PR.

[yoosefi]

@crynobone a PR for what? the issue was closed remember 🤡

[crynobone]

Doesn't matter as this has been marked as Help Wanted but staled.

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!

[yoosefi]

So reopen?

[crynobone]

A stale (no PR) issue will be closed regardless.

[yoosefi]

@crynobone the race condition doesn't magically disappear if you ignore it. this isn't a feature request, buddy. how did you even get on the team?

[yoosefi]

"this is fine"