-
Notifications
You must be signed in to change notification settings - Fork 58
Fix loading missing files post deployment #246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…1999F45C6F5C68A5AA2D50F490F472BC05932163737CEBDFA4EEE066D006415E7BF8C5FD7E3F6FEA9
@@ -58,6 +58,7 @@ | |||
$ingestTimeout ??= 10; | |||
/** @var ?string $server */ | |||
$server ??= (string) gethostname(); | |||
$testing = (bool) ($_SERVER['NIGHTWATCH_TESTING'] ?? false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to be able to avoid exit
while we are running the testsuite.
if ($testing) { | ||
return; | ||
} else { | ||
exit(1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, this would just return, but I think we should exit with an error code if we cannot read the signature file.
Gonna sit on this one and see if we have continued reports of the issue. |
Perhaps slight off-topic, but highly related: This PR tackles the shutdown of the agent in symlink deployment environments. But I'd like to know more about how the agent should be forced to restart/shutdown in such scenario. Saving/fetching/killing the PID feels quite verbose, especially knowing that for queue workers ( Or maybe restarts or shutdowns aren't necessary after all and the agent is agnostic of the application it was launched from? It might be a good addition anyway to add some pointers about this to the docs to avoid confusion? https://nightwatch.laravel.com/docs/getting-started/start-guide#running-the-agent ? |
@Propaganistas, you should not be manually restarting the agent during a deployment. The agent automatically restarts when required on a delay. It does not need to restart the same way that the queue worker or horizon need to restart, as it is a self-contained binary that is not concerned with changing PHP code in your application's code or dependencies. |
Closing this as we have another approach that results in the shutdown still occurring when an error is not encountered. |
Imagine you have an application that uses symlinks to do zero-downtime deployments.
The application runs
php artisan nightwatch:agent
via a daemon.The application has just performed a deployment where its own dependencies have changed.
When the agent shuts down, the PHP execution flow returns to the parent Laravel application. The Laravel application will then require classes that have not yet been required. If any of those files are missing after the update, because of the dependency update, it is possible that the command errors out because of requiring missing files.
Instead, we now always exit the current PHP process within the agent itself, so control never returns to the Laravel application.
This is a bit of a tradeoff because it means that the terminating callbacks or other event listeners will not be triggered.
Another alternative is to do what we have done in the agent and eagerly load all the application's dependency files, but that feels extremely heavy handed and could cause unexpected side-effects, e.g., memory usage increases, actual PHP side-effects because of executing code while requiring files. It also may still have issues if terminating callbacks, somewhere in the code stack, manually require files that are now missing.
This is also heavy handed...but feels like the right direction to me.
Laravel is used as a shell to boot the agent command but is not used for shutdown any longer.