GracefulShutdownException stops fail() from executing

[HenningerIBC]

Deployer Version

v7.5.12

Target OS

Debian 12

Which PHP version are you using?

PHP 8.2

Content of deploy.php or deploy.yaml

<?php
namespace Deployer;

localhost();

task('GracefulShutdownException', function () {
	throw new Exception\GracefulShutdownException("This is a GracefulShutdownException");
});

task('Exception', function () {
	throw new \Exception("This is a unexpectional Exception");
});

task('HandleException', function () {
	info('Exception Handled!');
});

fail('GracefulShutdownException', 'HandleException');
fail('Exception', 'HandleException');

Steps to reproduce

When running dep Exception, the Exception gets handled, however when executing dep GracefulShutdownException, HandleException never gets called.

This makes cleanup after a failed deploy near impossible.
In my case, I want to handle a deploy exception by calling deploy:unlock afterwards, so that one can try deploying again. However, since deployer v7, this no longer works. I recently upgraded from v6 to v7 and back in v6 a failed deploy did execute the fail hook just fine.

[antonmedv]

 We need to check Kermit's history to understand why this was removed. If I remember correctly, this was done because of some inconsistencies.

[HenningerIBC]

Still not sure as to how to fix it. We could simply drop the GracefulShutdownException-check

deployer/src/Command/MainCommand.php

Lines 146 to 148 in ff07090

	if ($exitCode === GracefulShutdownException::EXIT_CODE) {
	return 1;
	}

to get the old behavior, but if your previous hunch about some inconsistencies is correct, these will return?

Or make it configurable, letting the developer decide whether to shutdown immediatialy or allow fail() to trigger afterwards. Something like in one's deployer script:

set('skipGracefulShutdownException', true);

and checking for it during upon an GracefulShutdownException:

if ($exitCode === GracefulShutdownException::EXIT_CODE && !($this->deployer->config->has('skipGracefulShutdownException') && $this->deployer->config->get('skipGracefulShutdownException'))) { 
     return 1; 
}

(code untested)

[antonmedv]

Hmmm, I think this is actually the whole point of GracefulShutdownException. Is to not trigger fail!

Maybe you can throw another exception? Or can we maybe add another hook for GracefulShutdownException?

[HenningerIBC]

Then the question is why certain recipes require a GracefulShutdownException. Or why it should NOT trigger fail() in certain scenarios.
Looking through the code, I found four instances of of GracefulShutdownException:

• check_remote.php

  deployer/recipe/deploy/check_remote.php

  Lines 45 to 47 in ff07090

  	if ($targetRevision && strpos($lastDeployedRevision, $targetRevision) === 0) {
  	throw new GracefulShutdownException("Already up-to-date.");
  	}

• lock.php

  deployer/recipe/deploy/lock.php

  Lines 28 to 31 in ff07090

  	if ($locked) {
  	$lockedUser = run("cat {{deploy_path}}/.dep/deploy.lock");
  	throw new GracefulShutdownException("Deploy is locked by $lockedUser.");
  	}

• magento2.php

  deployer/recipe/magento2.php

  Lines 373 to 377 in ff07090

  	if (!test('[ -f {{artifact_excludes_file}} ]')) {
  	throw new GracefulShutdownException(
  	"No artifact excludes file provided, provide one at artifacts/excludes or change location",
  	);
  	}

  deployer/recipe/magento2.php

  Lines 399 to 401 in ff07090

  	if (!currentHost()->get('local')) {
  	throw new GracefulShutdownException('Artifact can only be built locally, you provided a non local host');
  	}

To me, only the instance of GracefulShutdownException in lock.php makes sense as it might otherwise conflict with another party's deployment. A necessary failsafe so to say.
The check_remote.php variant likely means that there isn't much to cleanup, but shouldn't cleanup run anyway? At the very least, it shouldn't be called after a deploy lock, otherwise one might get an orphaned lock... This is also how I stumbled over this initially.
As for magento: I am not familiar with it, so no comment.

Well, I somewhat understand the existence of GracefulShutdownException now. Overall my suggestions are:

• Add documentation that fail() is not triggered by an GracefulShutdownException
• Add documentation for tasks that throw a GracefulShutdownException (which in turn do not trigger fail())
• Add documentation for what is the correct order of deploy:check_remote? Code wise, it should happen before a deploy:lock, though I'd rather lock first before doing or checking anything (= freeze the current deployed state first).
• Reevaluate the existence of throw new GracefulShutdownException() in the other cases?

Overall, we can leave the try-catch-fail-logic as is. It just threw me for a loop as I didn't understand it before... :)

[antonmedv]

I agree. Let's refactor this logic all. I think generally Deployer should call fail on any exception! And we should have a special way to explicitly mark an exception to not trigger fail.

[HenningerIBC]

The shutdown GracefulShutdownException does have a right to exist and breaking changes suck, so here are a few ideas:

1. Introduce a boolean as third a parameter to fail() to also include GracefulShutdownException. Undefined, the current default behavior continues, making it fully backward compatibly. Downside is, that one cannot differentiate between regular Exceptions and GracefulShutdownException
2. Introduce a new shutdown() function (can't think of a more fitting name), that only gets triggered by GracefulShutdownException. The behavior is otherwise identical to fail, thus one can register a single task to it. This is also fully backward compatible.
3. All exceptions trigger fail(), however the Exception (or Exception's error code) is forward to the called task, making it possible to decide within the called task how to handle it exactly. Will likely break behavior for some without some code changes... Unless when mixing it with the first suggestion?