Repository navigation
fix: handle retryable http responses across consumers - #265
Open
pauldambra wants to merge 6 commits into
Open
pauldambra wants to merge 6 commits into
pauldambra wants to merge 6 commits into
Conversation
Generated-By: PostHog Desktop Task-Id: 5681be44-4986-48db-b140-1bda88f75f96
pauldambra
added this pull request to stack #266
October 16, 2026 15:57
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 655d8f4: 2 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
Contributor
posthog-php-socket Compliance ReportDate: 2026-10-19T17:33:39.173312+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
posthog-php-lib_curl Compliance ReportDate: 2026-10-19T17:33:49.619750+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
Contributor
posthog-php-fork_curl Compliance ReportDate: 2026-10-19T17:33:35.970953+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
pauldambra
marked this pull request as ready for review
October 16, 2026 16:00
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
marandaneto
approved these changes
Oct 16, 2026
marandaneto
left a comment
marandaneto
left a comment
Member
There was a problem hiding this comment.
The code looks good; no qualifying findings. Verdict: correct.
marandaneto
reviewed
Oct 16, 2026
marandaneto
left a comment
marandaneto
left a comment
Member
There was a problem hiding this comment.
Follow-up question about retry-queue specification alignment.
|
|
||
| $this->handleError($statusCode, implode("\n", $output)); | ||
| if (!$this->isRetryableStatus($statusCode) || $backoff >= $this->maximum_backoff_duration) { | ||
| return self::FLUSH_BATCH_NON_RETRYABLE_FAILURE; |
Member
There was a problem hiding this comment.
question: Did you notice the difference from the retry-queue spec, and is it intentional? Returning FLUSH_BATCH_NON_RETRYABLE_FAILURE here 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’s ConsumerForkCurlTest::testHttpDeliveryRetryPolicy “bounded retries” case confirms that two HTTP 503 responses drain the batch and a subsequent flush sends no further request.
Cap each retry wait at the configured maximum backoff and keep oversized Retry-After values inside the integer range, so a server cannot block a PHP worker for hours or raise a TypeError on flush. Generated-By: PostHog Desktop Task-Id: e73c8b25-9501-4f04-9fd8-e82415e1f9f9
Generated-By: PostHog Desktop Task-Id: e73c8b25-9501-4f04-9fd8-e82415e1f9f9
Member
Author
|
Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated lenses (qa-team, paul-reviewer, xp-reviewer, security-audit, engineering-systems-thinking as warranted) Verdict: ✅ APPROVE (round 1 @ eb6d09f)No new defects found. The Retry-After overflow and unbounded wait are fixed in f1a6651, with tests for Socket, ForkCurl and HttpClient. Key findingsNone this round. The open question about the retry-queue spec (batch dropped after the retry budget runs out) stays with the PR author. ConvergenceBoth reviewers independently found the ForkCurl temp files and shell arguments safe, and the Socket read loop bounded by a deadline and a 64 KiB header limit. Reviewer summaries
Automated by QA Swarm — not a human review |
Generated-By: PostHog Desktop Task-Id: e73c8b25-9501-4f04-9fd8-e82415e1f9f9
maximum_backoff_duration acts as the retry budget, so a small value cut short Retry-After waits the spec expects to be honoured. Cap the wait at 10 seconds instead. Generated-By: PostHog Desktop Task-Id: e73c8b25-9501-4f04-9fd8-e82415e1f9f9
Follow the retry-queue spec: retry-budget exhaustion is not a terminal disposition, so Socket, ForkCurl and LibCurl now return a retryable failure and the queue keeps the batch. Automatic flushes wait for the maximum backoff duration before they try again, so a down server does not block every capture call. Generated-By: PostHog Desktop Task-Id: e73c8b25-9501-4f04-9fd8-e82415e1f9f9
Comment on lines
+184
to
+185
| if ($this->inRetryCooldown()) { | ||
| return true; |
Contributor
There was a problem hiding this comment.
P1 · Allow automatic recovery when the cooldown fills the queue
When captures fill max_queue_size during the retry cooldown, enqueue() rejects every subsequent event at its earlier capacity check. Even after the cooldown expires and the server recovers, automatic flushing never runs again, so a long-lived client rejects events until explicitly flushed. Allow an expired cooldown to trigger delivery before rejecting a full queue.
This branch has not been deployed
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
🤖 Automated PR prepared with PostHog Desktop.
💡 Motivation and Context
Applications that use foreground ForkCurl can lose batches because curl reports success when the server rejects a request. Socket also misses HTTP 408 retries and Retry-After delays.
This layer depends on the Socket state repair. The API snapshot adds one internal trait symbol with no public callable methods.
💚 How did you test it?
Parameterized transport cases failed before the fixes and passed after them. They cover status handling, retry limits, Retry-After delays, stable bodies, gzip, and queue drain.
Malformed-header cases and an unlimited PHP default timeout case also failed before their repairs. The full PHP suite, configured style check, API check, and adapter tests passed.
A foreground ForkCurl smoke test passed with PHP curl functions disabled. Full compliance passes for Socket, LibCurl, and foreground ForkCurl, with 47/47 assertions per profile.
📝 Checklist
If releasing new changes
pnpm changeto generate a change intent fileCorepack could not load pnpm because certificate validation failed. A patch change intent follows the existing file format.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
PostHog Desktop with GPT-6 and GPT-6.1 Sol prepared the change. A separate GPT-6.1 Sol review found the unbounded header read; the repair adds size and time limits. Skills: pr-shepherd, writing-tests, writing-code-comments, writing-pr-descriptions, paul-pair, and stacking-prs. Human review is required.
Created with PostHog Desktop