From 1a339fbf8d319b78c7122951f29b546f1e8e8fc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Przepi=C3=B3ra?= Date: Mon, 28 Sep 2026 20:51:36 +0200 Subject: [PATCH] Keep a failing module from taking the application down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ModuleManager::loadEnabled() registered each enabled module's provider inside a try/catch, and the comment promised "a bad module never prevents the application from booting". It only half held. Laravel calls a provider's boot() later, while booting every provider, so an exception thrown there escapes that catch: the module that threw takes every route with it. Seen with a module built against a newer core — it called a menu helper with an argument this version does not accept. The result was a 502 on the whole installation, including the admin screen needed to disable the module again; recovery meant deleting its directory by hand. Providers are now wrapped in ModuleProviderGuard, which runs register() and boot() itself and swallows what either throws, logging it and reporting it. A module that fails simply contributes nothing — no routes, menus or listeners — and the rest of the application is untouched. The guard is registered with force: true because the container deduplicates providers by class name. Every module's guard is the same class, so without it the second module's guard would be mistaken for the first one's and that module would never load at all — a quieter bug than the one being fixed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01QRWsLoNeVWdsSHve6vUmxs --- backend/app/Services/ModuleManager.php | 13 +- backend/app/Support/ModuleProviderGuard.php | 73 ++++++++++ .../tests/Feature/ModuleProviderGuardTest.php | 132 ++++++++++++++++++ 3 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 backend/app/Support/ModuleProviderGuard.php create mode 100644 backend/tests/Feature/ModuleProviderGuardTest.php diff --git a/backend/app/Services/ModuleManager.php b/backend/app/Services/ModuleManager.php index 8f2b9875..e2e566f2 100644 --- a/backend/app/Services/ModuleManager.php +++ b/backend/app/Services/ModuleManager.php @@ -382,7 +382,18 @@ public function loadEnabled(\Illuminate\Foundation\Application $app): void $provider = $manifest['provider'] ?? null; if ($provider && class_exists($provider)) { - $app->register($provider); + // Wrapped rather than registered directly: an exception from the + // provider's boot() is thrown while Laravel boots every provider, + // long after this method returned, so the caller's try/catch + // cannot see it — and one bad module would 502 the whole app. + // + // Registered with force, because the container deduplicates by + // class name: without it the second module's guard would be + // mistaken for the first one's and silently skipped. + $app->register( + new \App\Support\ModuleProviderGuard($app, new $provider($app), $name), + force: true, + ); } } } diff --git a/backend/app/Support/ModuleProviderGuard.php b/backend/app/Support/ModuleProviderGuard.php new file mode 100644 index 00000000..3923572f --- /dev/null +++ b/backend/app/Support/ModuleProviderGuard.php @@ -0,0 +1,73 @@ +register()` inside a + * try/catch, and the comment there promised "a bad module never prevents the + * application from booting". It only half held: `register()` was covered, but + * Laravel calls a provider's `boot()` later, while booting every provider, and + * an exception thrown there escapes that catch entirely. A module built + * against a newer core — calling a menu helper with an argument this version + * does not accept, say — took the entire installation down with a 502, on + * every route, including the admin screen needed to disable it again. + * + * Both phases run here instead, each guarded. A module that throws simply does + * not register its routes, menus or listeners; every other page keeps working + * and the reason is in the log rather than in an nginx error page. + */ +class ModuleProviderGuard extends ServiceProvider +{ + public function __construct( + Application $app, + private readonly ServiceProvider $inner, + private readonly string $module, + ) { + parent::__construct($app); + } + + public function register(): void + { + $this->guard('register'); + } + + public function boot(): void + { + $this->guard('boot'); + } + + /** + * Run one phase of the wrapped provider, swallowing anything it throws. + * + * `boot()` is resolved through the container because a provider may type-hint + * its dependencies there, which is how Laravel calls it too. `register()` is + * always defined by the base class; `boot()` is not, so it is checked. + */ + private function guard(string $phase): void + { + if (! method_exists($this->inner, $phase)) { + return; + } + + try { + $this->app->call([$this->inner, $phase]); + } catch (\Throwable $e) { + Log::error("module.provider_{$phase}_failed", [ + 'module' => $this->module, + 'provider' => $this->inner::class, + 'exception' => $e->getMessage(), + ]); + + // Also to the app's error reporting, so this is visible wherever + // exceptions normally are and not only to whoever tails the log. + report($e); + } + } +} diff --git a/backend/tests/Feature/ModuleProviderGuardTest.php b/backend/tests/Feature/ModuleProviderGuardTest.php new file mode 100644 index 00000000..c0152b0d --- /dev/null +++ b/backend/tests/Feature/ModuleProviderGuardTest.php @@ -0,0 +1,132 @@ +app, new ThrowsOnRegisterProvider($this->app), 'Broken'); + + $guard->register(); + + Log::shouldHaveReceived('error')->withArgs( + fn (string $message, array $context) => $message === 'module.provider_register_failed' + && $context['module'] === 'Broken', + ); + } + + public function test_an_exception_from_boot_does_not_escape(): void + { + Log::spy(); + + $guard = new ModuleProviderGuard($this->app, new ThrowsOnBootProvider($this->app), 'Broken'); + + $guard->boot(); + + Log::shouldHaveReceived('error')->withArgs( + fn (string $message, array $context) => $message === 'module.provider_boot_failed' + && $context['module'] === 'Broken', + ); + } + + public function test_a_working_provider_still_registers_and_boots(): void + { + $inner = new WorkingProvider($this->app); + $guard = new ModuleProviderGuard($this->app, $inner, 'Working'); + + $guard->register(); + $guard->boot(); + + $this->assertTrue($inner->registered, 'register() was not passed through'); + $this->assertTrue($inner->booted, 'boot() was not passed through'); + } + + /** + * The container deduplicates providers by class name. Every module's guard + * is the same class, so without force the second module would be treated as + * already registered and never load at all. + */ + public function test_two_modules_both_load_despite_sharing_a_guard_class(): void + { + $first = new WorkingProvider($this->app); + $second = new WorkingProvider($this->app); + + $this->app->register(new ModuleProviderGuard($this->app, $first, 'First'), force: true); + $this->app->register(new ModuleProviderGuard($this->app, $second, 'Second'), force: true); + + $this->assertTrue($first->registered, 'first module did not register'); + $this->assertTrue($second->registered, 'second module was swallowed by provider deduplication'); + } + + /** A provider with no boot() at all is common and must not error. */ + public function test_a_provider_without_boot_is_fine(): void + { + $inner = new RegisterOnlyProvider($this->app); + $guard = new ModuleProviderGuard($this->app, $inner, 'NoBoot'); + + $guard->register(); + $guard->boot(); + + $this->assertTrue($inner->registered); + } +} + +class RegisterOnlyProvider extends ServiceProvider +{ + public bool $registered = false; + + public function register(): void + { + $this->registered = true; + } +} + +class ThrowsOnRegisterProvider extends ServiceProvider +{ + public function register(): void + { + throw new \RuntimeException('register blew up'); + } +} + +class ThrowsOnBootProvider extends ServiceProvider +{ + public function boot(): void + { + throw new \ArgumentCountError('Unknown named parameter $badge'); + } +} + +class WorkingProvider extends ServiceProvider +{ + public bool $registered = false; + + public bool $booted = false; + + public function register(): void + { + $this->registered = true; + } + + public function boot(): void + { + $this->booted = true; + } +}