From 87f14ee0097c27761789ed21d8c9877fee10335f Mon Sep 17 00:00:00 2001 From: Arif Hoque Date: Sat, 17 Oct 2026 15:21:05 +0600 Subject: [PATCH] Fix: Context-ending listener failures can mask the original application failure --- src/Phaseolies/Application.php | 23 +- .../InteractsWithExecutionContexts.php | 91 +++++- tests/Application/ExecutionContextTest.php | 265 ++++++++++++++++++ 3 files changed, 364 insertions(+), 15 deletions(-) diff --git a/src/Phaseolies/Application.php b/src/Phaseolies/Application.php index c60d44e9..2c21ee2a 100644 --- a/src/Phaseolies/Application.php +++ b/src/Phaseolies/Application.php @@ -1170,11 +1170,26 @@ public function terminate( $this->endDeferScope($deferScope, $terminationFailure ?? $exception); } } finally { + $cleanupFailure = null; + try { $this->cleanupRequestScopedServices(); - } finally { + } catch (\Throwable $e) { + $cleanupFailure = $e; + } + + try { if ($context !== null) { - $this->endContext($context); + $this->endContext($context, $terminationFailure ?? $cleanupFailure); + } + } finally { + if ($cleanupFailure !== null) { + // Never replace the exception that is already ending the request. + if ($terminationFailure !== null) { + $this->reportTeardownFailure('Request cleanup failed', $cleanupFailure); + } else { + throw $cleanupFailure; + } } } } @@ -1543,7 +1558,7 @@ public function dispatch($request): DispatchResult Response::dispatchHttpException($e); } catch (\Throwable $renderFailure) { $this->endDeferScope($deferScope, $renderFailure); - $this->endContext($context); + $this->endContext($context, $renderFailure); throw $renderFailure; } @@ -1554,7 +1569,7 @@ public function dispatch($request): DispatchResult return new DispatchResult($this, $request, null, $e, $deferScope, $context); } catch (\Throwable $e) { $this->endDeferScope($deferScope, $e); - $this->endContext($context); + $this->endContext($context, $e); throw $e; } diff --git a/src/Phaseolies/DI/Concerns/InteractsWithExecutionContexts.php b/src/Phaseolies/DI/Concerns/InteractsWithExecutionContexts.php index 2ddf5dd1..10d9cc0f 100644 --- a/src/Phaseolies/DI/Concerns/InteractsWithExecutionContexts.php +++ b/src/Phaseolies/DI/Concerns/InteractsWithExecutionContexts.php @@ -51,7 +51,7 @@ public function beginContext(string $type, array $metadata = []): ExecutionConte try { $this->fireContextListeners('begin', $context); } catch (\Throwable $e) { - $this->endContext($context); + $this->endContext($context, $e); throw $e; } @@ -63,11 +63,12 @@ public function beginContext(string $type, array $metadata = []): ExecutionConte * End a context: end any contexts nested inside it, run the end * * @param ExecutionContext $context + * @param \Throwable|null $original * @return void * * @throws \Throwable */ - public function endContext(ExecutionContext $context): void + public function endContext(ExecutionContext $context, ?\Throwable $original = null): void { $index = $this->contextIndex($context); @@ -82,7 +83,7 @@ public function endContext(ExecutionContext $context): void $index = count($this->contextStack) - 1; } - $failure = null; + $failures = []; $endingIds = [$context->id() => true]; @@ -96,9 +97,9 @@ public function endContext(ExecutionContext $context): void } try { - $this->endContext($descendant); + $this->endContext($descendant, $original); } catch (\Throwable $e) { - $failure ??= $e; + $failures[] = $e; } } @@ -108,15 +109,77 @@ public function endContext(ExecutionContext $context): void try { $this->fireContextListeners('end', $ending); } catch (\Throwable $e) { - $failure ??= $e; + $failures[] = $e; } unset($this->scopedInstances[$ending->id()], $this->liveContexts[$ending->id()]); array_pop($this->contextStack); } - if ($failure !== null) { - throw $failure; + if ($failures === []) { + return; + } + + // Throw the first failure only when nothing else is failing. All other + // failures are reported, so none is lost and none replaces the original. + $thrown = $original === null ? array_shift($failures) : null; + + foreach ($failures as $failure) { + $this->reportContextFailure($failure, $original); + } + + if ($thrown !== null) { + throw $thrown; + } + } + + /** + * Report a failure of a context-ending listener that is not being thrown + * + * @param \Throwable $failure + * @param \Throwable|null $original + * @return void + */ + protected function reportContextFailure(\Throwable $failure, ?\Throwable $original): void + { + $this->reportTeardownFailure('Execution context ending listener failed', $failure); + } + + /** + * Log a failure that happened while tearing down and must not replace another exception + * + * @param string $label + * @param \Throwable $failure + * @return void + */ + protected function reportTeardownFailure(string $label, \Throwable $failure): void + { + $reported = false; + + try { + if ($this->has('log')) { + \Phaseolies\Support\Facades\Log::error($label . ': ' . $failure->getMessage(), [ + 'exception' => $failure, + ]); + $reported = true; + } + } catch (\Throwable) { + // Reporting must never break teardown; fall through to the last resort. + } + + if (!$reported) { + try { + error_log(sprintf( + '%s: %s: %s in %s:%d', + $label, + get_class($failure), + $failure->getMessage(), + $failure->getFile(), + $failure->getLine() + )); + } catch (\Throwable) { + // Nothing left to try. + } } } @@ -133,10 +196,16 @@ public function runInContext(string $type, array $metadata, callable $callback): $context = $this->beginContext($type, $metadata); try { - return $callback($context); - } finally { - $this->endContext($context); + $result = $callback($context); + } catch (\Throwable $e) { + $this->endContext($context, $e); + + throw $e; } + + $this->endContext($context); + + return $result; } /** diff --git a/tests/Application/ExecutionContextTest.php b/tests/Application/ExecutionContextTest.php index 1fd7b42f..c3edf9a4 100644 --- a/tests/Application/ExecutionContextTest.php +++ b/tests/Application/ExecutionContextTest.php @@ -752,6 +752,263 @@ public function testContextEndsWhenTheHandlerThrows(): void $this->assertNull($app->executionContext()); } + public function testEndingListenerFailureDoesNotReplaceTheHandlerException(): void + { + $app = $this->application(); + $app->contextEnding(function () { + throw new \LogicException('teardown failed'); + }); + + $router = $this->createMock(Router::class); + $router->method('resolve')->willThrowException(new \RuntimeException('handler failed')); + $app->router = $router; + + $log = $this->captureErrorLog(); + + try { + $app->dispatch(new Request()); + $this->fail('expected the handler exception'); + } catch (\RuntimeException $e) { + $this->assertSame('handler failed', $e->getMessage()); + } finally { + $logged = $this->releaseErrorLog($log); + } + + $this->assertStringContainsString('Execution context ending listener failed: LogicException: teardown failed', $logged); + $this->assertNull($app->executionContext()); + } + + public function testEndingListenerFailureStillSurfacesWhenNothingElseFailed(): void + { + $app = $this->application(); + $app->contextEnding(function () { + throw new \LogicException('teardown failed'); + }); + $app->router = $this->respondingRouter(function () {}); + + $result = $app->dispatch(new Request()); + + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('teardown failed'); + + $result->terminate(); + } + + public function testEndingListenerFailureDoesNotReplaceATerminatingCallbackException(): void + { + $app = $this->application(); + $app->contextEnding(function () { + throw new \LogicException('teardown failed'); + }); + $app->terminating(function () { + throw new \RuntimeException('terminating failed'); + }); + $app->router = $this->respondingRouter(function () {}); + + $result = $app->dispatch(new Request()); + $log = $this->captureErrorLog(); + + try { + $result->terminate(); + $this->fail('expected the terminating exception'); + } catch (\RuntimeException $e) { + $this->assertSame('terminating failed', $e->getMessage()); + } finally { + $logged = $this->releaseErrorLog($log); + } + + $this->assertStringContainsString('teardown failed', $logged); + $this->assertNull($app->executionContext()); + } + + private function applicationWithFailingCleanup(): Application + { + $app = (new ReflectionClass(FailingCleanupApplication::class))->newInstanceWithoutConstructor(); + $app->reportDeferredExceptionsUsing(fn() => null); + + return $app; + } + + public function testCleanupFailureDoesNotReplaceATerminatingCallbackException(): void + { + $app = $this->applicationWithFailingCleanup(); + $app->terminating(function () { + throw new \RuntimeException('terminating failed'); + }); + $app->router = $this->respondingRouter(function () {}); + + $result = $app->dispatch(new Request()); + $log = $this->captureErrorLog(); + + try { + $result->terminate(); + $this->fail('expected the terminating exception'); + } catch (\RuntimeException $e) { + $this->assertSame('terminating failed', $e->getMessage()); + } finally { + $logged = $this->releaseErrorLog($log); + } + + $this->assertStringContainsString('Request cleanup failed: LogicException: cleanup failed', $logged); + $this->assertNull($app->executionContext()); + } + + public function testCleanupFailureStillSurfacesAndTheContextStillEndsWhenNothingElseFailed(): void + { + $app = $this->applicationWithFailingCleanup(); + $ended = 0; + $app->contextEnding(function () use (&$ended) { + $ended++; + }); + $app->router = $this->respondingRouter(function () {}); + + $result = $app->dispatch(new Request()); + + try { + $result->terminate(); + $this->fail('expected the cleanup exception'); + } catch (\LogicException $e) { + $this->assertSame('cleanup failed', $e->getMessage()); + } + + $this->assertSame(1, $ended); + $this->assertNull($app->executionContext()); + } + + public function testCleanupFailureWinsOverAnEndingListenerFailureWhenNothingElseFailed(): void + { + $app = $this->applicationWithFailingCleanup(); + $app->contextEnding(function () { + throw new \RuntimeException('listener failed'); + }); + $app->router = $this->respondingRouter(function () {}); + + $result = $app->dispatch(new Request()); + $log = $this->captureErrorLog(); + + try { + $result->terminate(); + $this->fail('expected the cleanup exception'); + } catch (\LogicException $e) { + $this->assertSame('cleanup failed', $e->getMessage()); + } finally { + $logged = $this->releaseErrorLog($log); + } + + $this->assertStringContainsString('listener failed', $logged); + $this->assertNull($app->executionContext()); + } + + public function testRunInContextKeepsTheCallbackExceptionWhenAnEndingListenerFails(): void + { + $this->container->contextEnding(function () { + throw new \LogicException('teardown failed'); + }); + + $log = $this->captureErrorLog(); + + try { + $this->container->runInContext('job', [], function () { + throw new \RuntimeException('job failed'); + }); + $this->fail('expected the job exception'); + } catch (\RuntimeException $e) { + $this->assertSame('job failed', $e->getMessage()); + } finally { + $logged = $this->releaseErrorLog($log); + } + + $this->assertStringContainsString('teardown failed', $logged); + $this->assertFalse($this->container->hasExecutionContext()); + } + + public function testRunInContextThrowsTheEndingListenerFailureWhenTheCallbackSucceeded(): void + { + $this->container->contextEnding(function () { + throw new \LogicException('teardown failed'); + }); + + $this->expectException(\LogicException::class); + + $this->container->runInContext('job', [], fn () => 'done'); + } + + public function testRunInContextReturnsTheCallbackResult(): void + { + $this->assertSame('done', $this->container->runInContext('job', [], fn () => 'done')); + $this->assertFalse($this->container->hasExecutionContext()); + } + + public function testBeginningListenerFailureIsNotReplacedByAnEndingListenerFailure(): void + { + $this->container->contextBeginning(function () { + throw new \RuntimeException('begin failed'); + }); + $this->container->contextEnding(function () { + throw new \LogicException('teardown failed'); + }); + + $log = $this->captureErrorLog(); + + try { + $this->container->beginContext('job'); + $this->fail('expected the begin failure'); + } catch (\RuntimeException $e) { + $this->assertSame('begin failed', $e->getMessage()); + } finally { + $this->releaseErrorLog($log); + } + + $this->assertFalse($this->container->hasExecutionContext()); + } + + public function testEveryEndingListenerFailureIsReportedBeyondTheFirstThrownOne(): void + { + $this->container->contextEnding(function (ExecutionContext $c) { + throw new \LogicException('failed: ' . $c->type()); + }); + + $outer = $this->container->beginContext('outer'); + $inner = $this->container->beginContext('inner'); + $this->container->suspendContext($inner); + + $log = $this->captureErrorLog(); + + try { + $this->container->endContext($outer); + $this->fail('expected the first failure'); + } catch (\LogicException $e) { + $this->assertSame('failed: inner', $e->getMessage()); + } finally { + $logged = $this->releaseErrorLog($log); + } + + $this->assertStringContainsString('failed: outer', $logged); + } + + /** + * @return array{string, string|false} + */ + private function captureErrorLog(): array + { + $file = tempnam(sys_get_temp_dir(), 'ctxlog'); + $previous = ini_set('error_log', $file); + + return [$file, $previous]; + } + + private function releaseErrorLog(array $log): string + { + [$file, $previous] = $log; + + ini_set('error_log', $previous === false ? '' : $previous); + + $contents = (string) file_get_contents($file); + @unlink($file); + + return $contents; + } + public function testRequestMetadataComesFromTheRequest(): void { $app = $this->application(); @@ -855,3 +1112,11 @@ protected function bootstrapCommands(): void {} $this->assertNull($app->executionContext()); } } + +class FailingCleanupApplication extends Application +{ + protected function cleanupRequestScopedServices(): void + { + throw new \LogicException('cleanup failed'); + } +}