Hi, I hope you're all doing well!

I have two possible solutions in mind. I believe the first one is the better long-term option, though I'm not sure if it's acceptable for version 6.4, it might need to wait until 7.4. The second one could serve as a temporary workaround (or patch) that could be included from 6.4 up to 7.3, and later be replaced by the first solution.

First Solution (Preferred):

Introduce a new exception class named UnrecoverableRunCommandFailedException:

final class UnrecoverableRunCommandFailedException extends RunCommandFailedException implements UnrecoverableExceptionInterface 
{
}

Note: This class implements UnrecoverableExceptionInterface.

To do this, we would need to remove the final keyword from RunCommandFailedException. If we prefer not to remove final, then we could copy the constructor into the new UnrecoverableRunCommandFailedException class and make it extend RuntimeException, as RunCommandFailedException currently does.

We would also need to update this try-catch block like so:

try {
    $exitCode = $this->application->run($input, $output);
} catch (UnrecoverableExceptionInterface $e) {
    throw new UnrecoverableRunCommandFailedException($e, new RunCommandContext($message, Command::FAILURE, $output->fetch()));
} catch (\Throwable $e) {
    throw new RunCommandFailedException($e, new RunCommandContext($message, Command::FAILURE, $output->fetch()));
}

With this, I believe we'd be in a good place!

Second Solution (Fallback / Patch for 6.4–7.3):

Modify the RunCommandFailedException class by adding a new property allowRetry (defaulting to true) with corresponding getter and setter, without changing the constructor:

class RunCommandFailedException extends RuntimeException
{
    private bool $allowRetry = true;

    public function __construct(\Throwable|string $exception, public readonly RunCommandContext $context)
    {
        parent::__construct(
            $exception instanceof \Throwable ? $exception->getMessage() : $exception,
            $exception instanceof \Throwable ? $exception->getCode() : 0,
            $exception instanceof \Throwable ? $exception : null,
        );
    }

    public function getAllowRetry(): bool
    {
        return $this->allowRetry;
    }

    public function setAllowRetry(bool $allowRetry)
    {
        $this->allowRetry = $allowRetry;
    }
}

Then, modify the same try-catch block to look like this:

try {
    $exitCode = $this->application->run($input, $output);
} catch (UnrecoverableExceptionInterface $e) {
    $exception = new RunCommandFailedException($e, new RunCommandContext($message, Command::FAILURE, $output->fetch()));
    $exception->setAllowRetry(false);
    throw $exception;
} catch (\Throwable $e) {
    throw new RunCommandFailedException($e, new RunCommandContext($message, Command::FAILURE, $output->fetch()));
}

Finally, update the logic in shouldRetry to check this new allowRetry property.

As I mentioned, I strongly prefer the first solution, but if it’s not possible to introduce it in version 6.4, then perhaps the second one could be considered as an interim workaround.

In either case, I’d be happy to contribute the changes if you think either (or both) approaches are worth moving forward with, e.g., solution 2 for 6.4–7.3 and solution 1 for 7.4+ 😃

Let me know what you think!

from the discussion on slack :

having a command throw that exception a bit strange. Like is it only intended to be run via messenger?

with which I can agree, it does feel a bit odd given that a command can also run outside of the context of a RunCommandMessage.

this would suggest a solution without using that exception would be preferred ?

it feels it would make sense to put the logic in the RunCommandMessageHandler, like :

        try {
            $exitCode = $this->application->run($input, $output);
        } catch (\Throwable $e) {
            if($message->inhibitRetry) {
                throw new UnrecoverableMessageHandlingException(
                    $e->getMessage(),
                    $e->getCode(),
                    $e
                );
            }
            
            throw new RunCommandFailedException($e, new RunCommandContext($message, Command::FAILURE, $output->fetch()));
        }

where the inhibitRetry (or something like it) would be a property to be set on the RunCommandMessage.

Thanks for the suggestion @peterpeppered 😃!

The main concern here is compatibility: introducing a new property like inhibitRetry on the RunCommandMessage would likely require changing its constructor, which we’re trying to avoid in the 6.4.

yeah, I understand that for BC you might not want to do that, this was thinking freely :)

I see from reading the documentation that adding an optional parameter with a default to the constructor is allowed though ? Or am I reading that incorrectly ?

[11] Only optional argument(s) of a constructor at last position may be added.

(at https://symfony.com/doc/current/contributing/code/bc.html#changing-classes)

Hi ❤️ You're absolutely right, adding an optional parameter at the end of a constructor is allowed according to the BC rules 🙌. But I think 🤔 that's only permitted in minor or major versions. Since we're dealing with a patch release here, I believe it's not allowed 😔

Maybe someone more experienced with the BC policy 🙌 (like @kbond? ❤️) could help us decide on the best way to implement this.

But I think 🤔 that's only permitted in minor or major versions.

Correct, for 6.4, if we are going to consider this a bug - we could do the following:

try {
    $exitCode = $this->application->run($input, $output);
} catch (UnrecoverableExceptionInterface $e) {
    throw $e
} catch (\Throwable $e) {
    throw new RunCommandFailedException($e, new RunCommandContext($message, Command::FAILURE, $output->fetch()));
}

If we want to do something more, and I'm not sure we should... that would need to be in 7.4

Hi @kbond 🖖 !
Thank you very much for your suggestion, I really appreciate it 😄

I've opened a PR targeting the 6.4 branch using the approach you proposed. However, I believe (though I could be mistaken) that for 7.4 we might need to consider a different solution. The reason is that, with the current fix, we lose the RunCommandContext information, which could be quite valuable in certain scenarios.

It might be worth exploring an approach for 7.4 that preserves that context while still properly handling unrecoverable exceptions. I think (though again, I might be wrong) that the initial solution I proposed could be a good starting point, it maintains the context without introducing too much complexity.

Happy to discuss this further and open to other ideas.

Thanks again!