Repository navigation
fix: handle retryable http responses across consumers #265
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
Changes from all commits
655d8f4
f1a6651
eb6d09f
1f56ad2
b7f957d
b035c9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "posthog-php": patch | ||
| --- | ||
|
|
||
| Retry transient HTTP responses and honor Retry-After in Socket and foreground ForkCurl consumers. Retry-After waits are now capped at 10 seconds in all HTTP transports. When retries run out on a retryable response, all HTTP consumers now keep the batch queued instead of dropping it, and automatic flushes wait for the maximum backoff duration before they try again. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| <?php | ||
|
|
||
| namespace PostHog; | ||
|
|
||
| /** @internal */ | ||
| trait HttpRetryPolicy | ||
| { | ||
| private const MAXIMUM_RETRY_AFTER_MILLISECONDS = 10000; | ||
|
|
||
| protected function isRetryableStatus(int $responseCode): bool | ||
| { | ||
| return $responseCode === 408 | ||
| || $responseCode === 429 | ||
| || ($responseCode >= 500 && $responseCode <= 600); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<int, string> $headers | ||
| */ | ||
| protected function retryDelayMilliseconds(array $headers, int $backoff): int | ||
| { | ||
| $retryAfter = $this->retryAfterMilliseconds($headers); | ||
|
|
||
| return $retryAfter === null ? $backoff : min($retryAfter, self::MAXIMUM_RETRY_AFTER_MILLISECONDS); | ||
| } | ||
|
|
||
| /** | ||
| * @param array<int, string> $headers | ||
| */ | ||
| protected function retryAfterMilliseconds(array $headers): ?int | ||
| { | ||
| foreach ($headers as $header) { | ||
| if (stripos($header, 'Retry-After:') !== 0) { | ||
| continue; | ||
| } | ||
|
|
||
| $value = trim(substr($header, strlen('Retry-After:'))); | ||
| if ($value === '') { | ||
| return null; | ||
| } | ||
|
|
||
| if (ctype_digit($value)) { | ||
| return min((int) $value, intdiv(PHP_INT_MAX, 1000)) * 1000; | ||
| } | ||
|
|
||
| $timestamp = strtotime($value); | ||
| if ($timestamp !== false) { | ||
| return max(0, (int) (($timestamp - time()) * 1000)); | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| */ | ||
| abstract class QueueConsumer extends Consumer | ||
| { | ||
| use HttpRetryPolicy; | ||
|
|
||
| protected const MAX_BATCH_PAYLOAD_SIZE = 1024 * 1024; // 1MB | ||
| protected const MAX_BATCH_PAYLOAD_SIZE_HUMAN = (self::MAX_BATCH_PAYLOAD_SIZE / 1024) . 'KB'; | ||
|
|
||
|
|
@@ -31,6 +33,7 @@ abstract class QueueConsumer extends Consumer | |
| protected $batch_size = 100; | ||
| protected $flush_interval = 5.0; | ||
| protected $flush_after = null; | ||
| protected $retry_cooldown_until = null; | ||
| protected $maximum_backoff_duration = 10000; // Set maximum waiting limit to 10s | ||
| protected $host = "us.i.posthog.com"; | ||
| protected $compress_request = false; | ||
|
|
@@ -142,6 +145,10 @@ public function flush() | |
| $result = $this->flushBatch($batch); | ||
| $success = true === $result; | ||
|
|
||
| if (self::FLUSH_BATCH_RETRYABLE_FAILURE === $result) { | ||
| $this->retry_cooldown_until = $this->now() + $this->maximum_backoff_duration / 1000; | ||
| } | ||
|
|
||
| if ($success || self::FLUSH_BATCH_RETRYABLE_FAILURE !== $result) { | ||
| array_splice($this->queue, 0, $batchSize); | ||
| } | ||
|
|
@@ -174,6 +181,10 @@ public function enqueue($item) | |
| $this->resetFlushTimer(); | ||
| } | ||
|
|
||
| if ($this->inRetryCooldown()) { | ||
| return true; | ||
|
Comment on lines
+184
to
+185
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1 · Allow automatic recovery when the cooldown fills the queue When captures fill |
||
| } | ||
|
|
||
| if ($this->flush_interval === 0.0 || $count >= $this->batch_size || $this->flushIntervalElapsed()) { | ||
| return $this->flush(); // return ->flush() result: true on success | ||
| } | ||
|
|
@@ -193,6 +204,11 @@ private function flushIntervalElapsed(): bool | |
| return $this->flush_after !== null && $this->now() >= $this->flush_after; | ||
| } | ||
|
|
||
| private function inRetryCooldown(): bool | ||
| { | ||
| return $this->retry_cooldown_until !== null && $this->now() < $this->retry_cooldown_until; | ||
| } | ||
|
|
||
| private function now(): float | ||
| { | ||
| return (float) Clock::get()->now()->format('U.u'); | ||
|
|
||
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.
question: Did you notice the difference from the retry-queue spec, and is it intentional? Returning
FLUSH_BATCH_NON_RETRYABLE_FAILUREhere removes the batch after retryable HTTP failures exhaust the retry budget, whereas the spec’s general transient-failure scenario keeps events queued for a later retry. I see that matching existing LibCurl behavior is deliberate; just checking whether this spec divergence was considered too. Reproduction: reproduced — the reviewed head’sConsumerForkCurlTest::testHttpDeliveryRetryPolicy“bounded retries” case confirms that two HTTP 503 responses drain the batch and a subsequent flush sends no further request.