diff --git a/CHANGELOG.md b/CHANGELOG.md index deb96919c74..f2852cae1aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ This file is a running track of new features and fixes to each version of the pa The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project follows [Semantic Versioning](https://semver.org) guidelines. +## Unreleased + +### Fixed + +- Fixed every error from Proxmox filling the log with three copies of the same stack trace -- about 256 lines per error, + every few seconds while a page polled a server whose VM was gone. An error is now logged once, with one trace, and its + message is Proxmox's own ("Proxmox returned 500: Configuration file ... does not exist"). + ## v4.7.0-rc.2 ### Fixed diff --git a/app/Exceptions/Repository/Proxmox/ProxmoxConnectionException.php b/app/Exceptions/Repository/Proxmox/ProxmoxConnectionException.php index 4a21a0e7580..31982ba8fab 100644 --- a/app/Exceptions/Repository/Proxmox/ProxmoxConnectionException.php +++ b/app/Exceptions/Repository/Proxmox/ProxmoxConnectionException.php @@ -8,8 +8,22 @@ class ProxmoxConnectionException extends RepositoryException { - public function __construct(Response $response, RequestException $exception) - { - parent::__construct($response->reason() . PHP_EOL . $exception->getMessage() . PHP_EOL . $exception->getTraceAsString(), $exception->getCode(), $exception); - } + /** + * The message is the HTTP status and Proxmox's own error, e.g. "Proxmox + * returned 500: Configuration file '…/109.conf' does not exist". + * + * It used to be the RequestException's message plus its full stack trace, + * with the RequestException chained as the previous exception -- so a logged + * error printed the trace three times: in the message, for this exception, + * and for the previous one, both thrown from the same line. The response is + * kept on the exception instead, for callers that need Proxmox's answer. + */ + public function __construct(public readonly Response $response, RequestException $exception) + { + $error = trim((string) $response->json('message')) + ?: trim($response->reason()) + ?: $exception->getMessage(); + + parent::__construct("Proxmox returned {$response->status()}: {$error}", $exception->getCode()); + } } diff --git a/tests/Unit/Exceptions/ProxmoxConnectionExceptionTest.php b/tests/Unit/Exceptions/ProxmoxConnectionExceptionTest.php new file mode 100644 index 00000000000..e64e6d8cd21 --- /dev/null +++ b/tests/Unit/Exceptions/ProxmoxConnectionExceptionTest.php @@ -0,0 +1,39 @@ + Http::response($body, $status)]); + [$_, $_, $_, $server] = createServerModel(); + + try { + app(ProxmoxConfigRepository::class)->setServer($server)->getConfig(); + } catch (ProxmoxConnectionException $e) { + return $e; + } + + throw new RuntimeException('Expected a ProxmoxConnectionException'); +} + +// A logged Proxmox error used to print its stack trace three times: inside the +// message, for this exception, and for the RequestException chained under it. +it('logs one trace: no trace in the message, nothing chained', function () { + $e = proxmoxErrorFor(['data' => null, 'message' => "Configuration file 'nodes/pve/qemu-server/109.conf' does not exist\n"]); + + expect($e->getMessage()) + ->toBe("Proxmox returned 500: Configuration file 'nodes/pve/qemu-server/109.conf' does not exist") + ->not->toContain('#0 ') + ->and($e->getPrevious())->toBeNull() + ->and($e->response->status())->toBe(500); +}); + +it("falls back to the HTTP client's message when Proxmox sends no JSON", function () { + $e = proxmoxErrorFor("can't connect to 10.0.0.1:443 (connection refused)", 595); + + expect($e->getMessage()) + ->toContain("can't connect to 10.0.0.1:443") + ->not->toContain('#0 '); +});