[Bug]: Gotenberg health ping instead of try convert dummy file#18954
Merged
[Bug]: Gotenberg health ping instead of try convert dummy file#18954
Conversation
Review Checklist
|
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request addresses a critical performance issue where unconfigured Gotenberg services cause 30+ second delays when loading PDF assets. The change replaces the inefficient file conversion health check with a proper HTTP health ping to Gotenberg's /health endpoint, with aggressive timeouts (1s connect, 2s total) to prevent long delays.
Changes:
- Replaced dummy file conversion test with direct
/healthendpoint ping - Added configurable short timeouts (1s connect, 2s overall) for health checks
- Introduced new Guzzle HTTP client with custom timeout configuration
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Co-authored-by: Peter van der Wal <peter.vanderwal@youweagency.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Removed unnecessary docblock for isAvailable method.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (2)
lib/Helper/GotenbergHelper.php:87
- Caching the unavailable status is a good improvement that addresses the performance issue mentioned in #18945. However, the cache uses the same TTL for both available and unavailable status. Consider using a shorter TTL for the unavailable status (e.g., 5-10 seconds) while keeping a longer TTL for available status (60 seconds). This would allow the system to detect when Gotenberg becomes available more quickly while still preventing the 30+ second delays when it's unavailable.
}
lib/Helper/GotenbergHelper.php:90
- The GotenbergHelper class lacks test coverage. Given that this change addresses a critical performance issue (30+ second delays from issue #18945) and introduces new caching behavior with health check endpoints, it would be beneficial to add tests that verify: 1) the caching behavior for both available and unavailable states, 2) the health check endpoint functionality, 3) handling of missing configuration. Other helper classes in the codebase (ParameterBagHelper, CsvFormulaTest) have comprehensive test coverage.
class GotenbergHelper
{
private static ?bool $validPing = null;
private const CACHE_KEY = 'gotenberg_ping';
private const STATUS_AVAILABLE = 'available';
private const STATUS_UNAVAILABLE = 'unavailable';
private static function healthPing(): bool
{
$gotenbergBaseUrl = Config::getSystemConfiguration('gotenberg')['base_url'];
if ($gotenbergBaseUrl) {
try {
$ch = curl_init(rtrim($gotenbergBaseUrl, '/') . '/health');
curl_setopt_array($ch, [
CURLOPT_RETURNTRANSFER => true,
CURLOPT_TIMEOUT => 2,
CURLOPT_CONNECTTIMEOUT => 2,
]);
curl_exec($ch);
$status = curl_getinfo($ch, CURLINFO_HTTP_CODE);
curl_close($ch);
return $status === 200;
} catch (\Throwable $e) {
return false;
}
}
return false;
}
public static function isAvailable(): bool
{
if (self::$validPing !== null) {
return self::$validPing;
}
$cachedStatus = Cache::load(self::CACHE_KEY);
if ($cachedStatus === self::STATUS_AVAILABLE) {
self::$validPing = true;
return true;
}
if ($cachedStatus === self::STATUS_UNAVAILABLE) {
self::$validPing = false;
return false;
}
$ttl = Config::getSystemConfiguration('gotenberg')['ping_cache_ttl'];
if (self::healthPing()) {
self::$validPing = true;
Cache::save(self::STATUS_AVAILABLE, self::CACHE_KEY, [], $ttl);
return true;
}
self::$validPing = false;
Cache::save(self::STATUS_UNAVAILABLE, self::CACHE_KEY, [], $ttl);
return false;
}
}
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… for longer ttl after 3 retries
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes in this pull request
Resolves #18945
Additional info
A more properly and efficient health/ping check based on https://gotenberg.dev/docs/routes#health-check-route