Fix cron:run and the scheduler: non-blocking background tasks - #340
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Checklist
Description
cron:rundid not behave as documented in several ways that only show up on real servers:inBackground()blocked the scheduler,noOverlap()could be defeated by the very runs that skipped a task, the documentedcron:run --daemoncrontab line started a new daemon every minute, and children could run under the wrong PHP or from the wrong directory. This PR fixes them, makes the scheduler work on hosts that disable process functions (common on cPanel), and adds real-process tests, which the scheduler did not have. Before this,runInBackground,runInForegroundandrunStandardModehad no test coverage at all.Each problem below was reproduced with the real command before it was fixed, and every fix has a test that fails when the fix is removed.
Problems found and fixed
inBackground()madecron:runwait for the whole jobcron:runtook 20.2 s. The background subshell inherited the pipeshell_exec()reads./dev/null. Now returns in ~0.1 s.ScheduledCommand::__destructcalledcleanup()unconditionally. With 8 concurrent probes, 3-4 of them got the lock.ownsLock). Background jobs are released bycron:finish.flockon a.guardfile.cron:run --daemonstarted a second daemon; the docs said it is ignoredflockonstorage/schedule/cron_daemon.lockfor its lifetime. It cannot go stale (the OS releases it onkill -9) and two simultaneous starts cannot both win.phpwas first onPATHphponPATHwas executed. Cron has a minimalPATHand cPanel keeps several PHP versions.SchedulePool::phpBinary(), the binary running the scheduler (falls back ifPHP_BINARYis FPM/CGI/LiteSpeed).pooland no working directory/(cPanel starts jobs in the home directory):Could not open input file: pool, whilecron:runstill reported success.cdinto the application root.--id="hello world"reached the command as--id="helloandworld".SchedulePool::splitCommand()tokenizes like a shell. Unbalanced quotes fall back to the old whitespace split, so nothing that worked before breaks.shell_execwas a fatalErrorthat killed the whole rundisable_functions.shell_exec, thenexec, thenproc_open. Foreground usesproc_open, thenexec, then runs in the scheduler process. If nothing works, a clear error is reported./tmp.storage/schedule.posix_kill()threw aValueErrorfor a garbage PID in a lock filecron:runalways exited 0catch (\Exception)did not catchError.\Throwable, records the failure, continues with the next task.Other changes
SchedulePool:phpBinary(),poolScript(),buildProcessArguments(),splitCommand(),isFunctionEnabled(),startDetached(), and a portableisProcessRunning()(posix_kill, then/proc, thenps). The old version returnedfalseon macOS (no/proc) and depended onshell_exec.cron:finishlooks for lock files instorage/schedule, and still in the system temp directory so a job started before an upgrade is found. It tolerates a half-written PID file.cron:daemonuses the shared process check.App\Schedule\Scheduledoes not exist,cron:runsays so and exits non-zero instead of a fatal error.inBackground()tasks run in the foreground with a warning, instead of running invalid shell syntax.cleanup(), which also deleted the throttle file ofnoOverlap()tasks and the last-run file of second-based tasks on every run. It now releases only the lock.CronRunCommand:makeSchedule(),startDetached(),buildBackgroundCommand(),runToCompletion().Behavior changes to be aware of
cron:runexits with a non-zero status when a task fails. Crontab entries that redirect output are unaffected, but monitors that check the exit status will now see failures.storage/schedule.cron:finishstill finds old ones. An old temp-dir lock is not seen by the new code, so a job that was already running during the upgrade may start once more.inBackground()tasks now return immediately. Code that accidentally relied oncron:runstaying alive while a background job ran no longer gets that.noOverlap()that is skipped no longer clears the lock of the running copy. This is what makes overlap protection actually work.Checklist