"Scheduled command ['/usr/local/bin/php' 'artisan' xxx] failed with exit code []."

[alexey-m-ukolov]

Laravel Version

12.11.0

PHP Version

8.4.2

Database Driver & Version

No response

Description

#55572 introduced a new check on Illuminate\Console\Scheduling\Event::exitCode.
But this property is set only when runInBackground flag is false. Otherwise it is always null which now is getting compared to zero strictly, unlike the next check in the same function.

Steps To Reproduce

Schedule any console command with ->runInBackground() and wait for it to run, then check logs.

[alexey-m-ukolov]

I see two possible solutions:

1. Move assignment from finish() to run().
2. Or just make comparison non-strict just like the next one.

I think the first one makes more sense, since checks inside the Event class are also strict.

[achrafAa]

@alexey-m-ukolov join the conversation here,#55605 this PR i addressing the issue above

[achrafAa]

@alexey-m-ukolov exactly that's why I used strict comparison. @tarkis already created a solution with "1. Move assignment from finish() to run()."

[alexey-m-ukolov]

Oh, thanks! I was using search on issues and didn't find anything so created this one. Next time will check PRs also 😄

[alexey-m-ukolov]

@alexey-m-ukolov exactly that's why I used strict comparison. @tarkis already created a solution with "1. Move assignment from finish() to run()."

Then maybe this comparison also should be made strict? But that would be a breaking change imo.

[achrafAa]

@alexey-m-ukolov I think it needs more testing testing and can be simplified to :

$this->components->task($description, function () use ($event) {
    $this->dispatcher->dispatch(new ScheduledTaskStarting($event));

    $start = microtime(true);
 
    $event->run($this->laravel);

    $this->dispatcher->dispatch(new ScheduledTaskFinished(
        $event,
        round(microtime(true) - $start, 2)
    ));

    if ($event->exitCode !== 0) {
        $e =  new Exception("Scheduled command [{$event->command}] failed with exit code [{$event->exitCode}].");
        $this->dispatcher->dispatch(new ScheduledTaskFailed($event, $e));
        $this->handler->report($e);
    }

    return $this->eventsRan = ($event->exitCode === 0);
});

[alexey-m-ukolov]

The main difference that I see is that current version handles any exceptions (presumably thrown from within $event->run()) when your proposed version does not.
I'm not qualified to judge whether these execeptions should be handled here or not, though.

[achrafAa]

@alexey-m-ukolov yes you are right the Event Start() function throws exception that need handeling so should be more like this :

$this->components->task($description, function () use ($event) {
            $this->dispatcher->dispatch(new ScheduledTaskStarting($event));

            $start = microtime(true);

            try {
                $event->run($this->laravel);

                $this->dispatcher->dispatch(new ScheduledTaskFinished(
                    $event,
                    round(microtime(true) - $start, 2)
                ));

                if ($event->exitCode !== 0) {
                    throw new Exception("Scheduled command [{$event->command}] failed with exit code [{$event->exitCode}].");
                }

            } catch (Throwable $e) {
                $this->dispatcher->dispatch(new ScheduledTaskFailed($event, $e));

                $this->handler->report($e);
            }

            return $this->eventsRan = ($event->exitCode === 0);
});

[alexey-m-ukolov]

Since original PR was reverted in 12.11.1, issue can be closed.