fix(core): honor Retry-After on the OTLP export queues - #4726
Draft
turnipdabeets wants to merge 6 commits into
Draft
fix(core): honor Retry-After on the OTLP export queues#4726turnipdabeets wants to merge 6 commits into
turnipdabeets wants to merge 6 commits into
Conversation
Contributor
Contributor
|
Size Change: +27.9 kB (+0.13%) Total Size: 20.9 MB 📦 View Changed
ℹ️ View Unchanged
|
turnipdabeets
force-pushed
the
fix/otlp-honor-retry-after
branch
from
September 1, 2026 16:45
68d9c61 to
197acec
Compare
Contributor
|
…eues Parses `Retry-After` once in the shared OTLP sender and applies it as a floor on each export queue's own backoff, and refuses a batch over the endpoint's 2 MB body limit without spending a request on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qp8JHHcGSf7mocdZtHDR29
turnipdabeets
force-pushed
the
fix/otlp-honor-retry-after
branch
from
September 1, 2026 21:41
f7ee901 to
3db04a2
Compare
…tlp-honor-retry-after
…imer The window was only consulted where a timer was armed, so an explicit flush() — the lifecycle and per-request path — sent inside it. Traces spent its whole per-batch retry budget there and dropped the spans before the wait elapsed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vkNv2jdvWeE8hoAuiwgCr
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vkNv2jdvWeE8hoAuiwgCr
Gating traces flush() on the window stranded spans: it is what the serverless waitUntil keep-alive awaits, and the recovery timer is unref'd, so a frozen isolate never sent them. Protect the head batch's retry budget instead of suppressing the send. The wall-clock deadline is now clamped, so a backward clock step can't stretch a wait past the cap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vkNv2jdvWeE8hoAuiwgCr
The deadline was re-installed by the very refusal it excused, so a host flushing faster than the window kept it open forever: the traces retry budget never advanced, the head batch never retired, and everything behind it was dropped at maxQueueSize. Measured 0 releases over 60 flushes; now releases at the intended 8 x window. A backward clock step no longer holds a window open either, and MAX_RETRY_AFTER_MS is out of the public barrel. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019vkNv2jdvWeE8hoAuiwgCr
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
1 participant
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.
Problem
Two ways the OTLP export queues spend requests the ingestion endpoint has already told them not to send.
1.
Retry-Afterwas ignored. The endpoint can tell an SDK how long to wait before retrying. The logs, metrics and traces export queues each used only their own exponential backoff, so a project told to back off for two minutes kept sending.This is conformance and defence-in-depth, not an urgent fix. PostHog's own capture path emits only 200/400/401/413/500 and never
429. The work that would have changed that, posthog/posthog#75090, was closed as stale in August without merging, and nothing has replaced it. Two reasons to do it anyway:sdk-specslogs and traces contracts require it unconditionally — "exponential backoff capped at ~30s, honoringRetry-Afterwhen present" — with no carve-out for the service not sending one yet. All three queues failed that requirement.429with aRetry-Aftertoday.2. Oversized batches were uploaded only to be refused. The endpoint caps the request body at 2 MB (
MAX_REQUEST_BODY_SIZE_BYTES, applied after it decompresses). A batch over that could only ever come back 413, but the SDK uploaded it anyway — and the halving loop then re-uploaded progressively smaller versions, spending a full request on each attempt.Note that the traces spec still describes #75090 as "in flight" in its
Server-side contractrequirement; that reference needs updating insdk-specsindependently of this PR.Stacked on #4579 because the traces queue only exists there. The logs and metrics halves are independent of it and would apply to
mainunchanged.Changes
Retry-AfterParsed once, in the shared OTLP sender, and surfaced to whichever queue is retrying.
parseRetryAfterMsaccepts both wire forms — delta-seconds and HTTP-date — and returnsundefinedfor anything it cannot parse, a date already in the past, or a non-positive delta, so the caller falls back to its own backoff."10 minutes"is rejected rather than read as 10 seconds, and so is numeric-looking junk (-5,+5,5.5), whichDate.parsewould otherwise read as dates in 2001.PostHogFetchHttpErrorexposes it asretryAfterMs, guarded:headers.getis injected transport code, and a throwing one must not turn a retriable failure into an unhandled rejection.retry-lateroutcome of all four send wrappers gains an optionalretryAfterMs. Additive, so nothing implementing or consuming these types breaks.Three judgment calls worth reviewing:
max(ownBackoff, retryAfterMs). Taking it literally would let aRetry-After: 1turn a queue that had backed off to 30s into a hot loop; HTTP semantics are "not before this", whichmaxsatisfies in both directions.fetchWithRetryloop stops when the header is present. That loop retries on a fixed short delay (fetchRetryDelay, default 3s) up tofetchRetryCount(default 3) times. Left alone it fired four requests inside the very window the queue was about to honour, soretryChecknow hands aRetry-Afterresponse straight to the queue-level backoff. A retriable response with no header still retries there as before.Logs stores an absolute deadline rather than a duration, because the timer is not its only send trigger: the
maxBufferSizesize trigger andonReconnect()both start a flush directly, and both now check the window.onReconnect()no longer clears it — anonlineevent means the network came back, not that the endpoint's rate limit expired, and browsers fire it on every network handover.Metrics has no exponential backoff of its own, so there the header raises the next timer arm above
flushIntervalMs.Every send path, not just the timer
The first revision gated only the paths that arm a timer. A later review pass found three places where a send still went out inside the window. Each was reproduced with an executed failing test before it was touched.
posthog-nodespent a span batch's whole retry budget inside the window.flushBackground()fires everyflushInterval(10s default) while events are flowing, and node'sflush()override drains spans alongside them.MAX_RETRIES_PER_BATCHis 8, so at that cadence the budget burned out in ~80s and the spans were dropped ~220s before the endpoint would have accepted them —Dropping 1 span(s): the ingestion endpoint failed 8 times in a row. A refusal that lands inside a window the endpoint already asked us to wait out is not evidence against the batch, so it no longer counts against that budget. The send itself is deliberately not suppressed: see "Why explicit flush still sends" below.flush().flush()leaves no timer behind, so the next capture is the one that arms it — atflushIntervalMs, inside the window. React Native takes this path on every foreground and background transition._armFlushTimernow floors the delay by the remaining window.flush(). The clear lived on the background wrapper's.then, whichflush(),flushWithTimeout()andshutdown()do not go through, soonReconnectand the size trigger stayed blocked for the rest of a window the endpoint had already stopped enforcing. The window is now recorded and cleared from the outcome, the way metrics and traces do it.Metrics and traces also held the header as a duration rather than a deadline, so a timer armed while the wait was already part-served restarted the whole window instead of counting down the remainder. Both now hold an absolute deadline, matching logs.
parseRetryAfterMsadditionally reads only the first value when two hops each append one —headers.getjoins repeated headers as"60, 120"— while leaving an HTTP-date, which carries a comma of its own, intact.A refusal that arrives inside an open window does not extend it. The deadline is installed once and then elapses; sliding it forward on each in-window refusal meant a host flushing faster than the window kept it open forever, so the retry budget never advanced and the head batch never retired — measured 0 releases across 60 flushes, with everything behind it dropped at
maxQueueSize. It now releases at the intended bound: in a host awaitingflush()ten times a second against a 300s window, the head batch releases at 2,100,000 ms (8 x the capped window), the same figure the earlier gated revision produced.Each queue also clamps the remaining wait to
MAX_RETRY_AFTER_MSwhen it reads the deadline back, and treats a clock that has moved behind the install point as ending the window rather than extending it by the size of the step. The deadline is wall clock, andparseRetryAfterMsapplies the five-minute cap once, to the duration; without the clamp a backward step — NTP, a resumed VM, a user changing the device date — stretched a 60s wait to over an hour. Verified: a one-hour backward step returned 3660s from_retryAfterRemainingMs().Why explicit flush still sends
An earlier revision of this PR gated
PostHogTraces.flush()on the window. That was wrong, and the review that followed caught it:posthog.flush()is what the serverlesswaitUntilkeep-alive awaits. Gated, it resolved immediately with spans still queued, and the only recovery was a timer thatsafeSetTimeoutunrefs — so a frozen isolate never ran it. Verified: after the window opened, two furtherflush()calls sent nothing and the queue stayed non-empty, even though the origin had recovered 5s in.Flush triggersrequirement says the globalflush()SHALL drain the span queue, which the gate violated.So the wait is honoured where it belongs — the periodic timer — and the damage it was meant to prevent is addressed at the source, by not charging the retry budget. An explicit flush inside a window costs one request; it no longer costs the spans.
2 MB body limit
A payload over the limit is reported as
too-largewithout a request, so the caller's existing halving loop isolates and drops the one oversized record without spending a request on every attempt.RequestDecompressionLayeris layered outsideDefaultBodyLimit::max(max_request_body_size_bytes), so a payload that gzips small is still refused on its decompressed size.byteLengthOfhelper extracted from theBuffer/TextEncoderblock already infetchWithRetry— so a CJK or emoji-heavy payload is measured the same way the server measures it.The constant is a floor, never a promise: a deployment can raise
MAX_REQUEST_BODY_SIZE_BYTESand a proxy in front can lower it. It is deliberately not configurable — it is internal, so it can be made configurable later without a breaking change, whereas shipping the option now would be permanent.Reviewer notes
429is not PostHog. With quota enforcement at capture abandoned, the header realistically arrives from a proxy, CDN or load balancer in front of capture — which is also why the cap exists.sdk-specs, which says only "honoringRetry-Afterwhen present". If reviewers agree with the reasoning above I will propose both as spec clarifications, since every SDK hits the same two questions.Retry-Afterlengthens how long traces holds a failing head batch. The per-batch budget is 8 attempts, and with the flush gate above they are now spread across the backoff instead of being spent at the host's cadence, so a 5-minute header turns a ~2.5-minute hold into a ~35-minute one. Measured over 40 minutes of steady traffic against a permanently-refusing endpoint, span loss is identical either way (2200 of 2400 in both cases, sincemaxQueueSizebounds the queue regardless); the change is 127 send attempts down to 10. Deliberately left as is — the alternative, dropping on a wall-clock budget, discards spans the endpoint asked us to hold.@posthog/typeschange. The outcome types live in core.parseRetryAfterMsis exported for its unit test and marked@internal.Verification
packages/core/src/__tests__/posthog.otlp-retry-after.spec.tsdrives it end to end from a mocked 429 through to each of the three queues' outcomes, because a unit test of the parser alone would still pass with the plumbing missing. It also covers a response with no header, a transport whoseheaders.getthrows, and the request count at the shippedfetchRetryCountdefault.Per-queue tests cover the behaviour that matters rather than the field assignment: a
Retry-Afterlonger than the 30s exponential cap actually delays the retry; a shorter one does not shorten it; a capture or span end landing mid-flush cannot pull the retry back inside the window; a steady stream of captures does not push the flush timer out indefinitely; the logs size trigger andonReconnectdo not send inside the window; and a non-retriable outcome ends the wait.Every behavioural test was mutation-checked: reverting
logs/index.ts,metrics/index.ts,traces/index.tsorposthog-core-stateless.tsfails exactly those tests and nothing else.packages/corepackages/nodepackages/react-nativepackages/browserEvery fix above was mutation-checked: reverting it to the shipped behaviour fails exactly the test written for it and nothing else. That includes the 2 MB boundary, which now pins both directions — a body of exactly 2 MB is sent, one byte more is not — because
>drifting to>=would have refused an acceptable batch without a request and, in traces, halved it down and dropped the span with no 413 to show for it.Also exercised on an Android emulator, with the React Native example built from this branch against a mock ingestion endpoint. Answering
429+Retry-After: 30, the log exports retried at +30.08s and +30.05s rather than on the SDK's 10s flush interval, and resumed on the next attempt once the mock returned 200.Lint and format clean.
Release info Sub-libraries affected
Libraries affected
@posthog/coreis bumped (patch); it has no checkbox above.posthog-js(web) is deliberately unticked. The browser SDK does not use core's_sendOtlpBatch:posthog-logs.tsandposthog-metrics.tsbuild their outcomes from_send_request's callback, andRequestResponse(statusCode/text/json/error) carries no headers. So it never populatesretryAfterMs, both new gates are permanently inert there, and the 2 MB pre-send check never runs. The queue state is bundled but unused — the only web artifact in this diff is the mangled-names cache. Wiring it up means threading headers (or a parsedretryAfterMs) throughRequestResponseand both the XHR and fetch paths, which is a separate change and deliberately not in this PR.Checklist
retryAfterMsis optional on types that were already exported, so implementors and consumers are unaffected. Behaviour only changes when the endpoint sends a header it does not send today, or when a batch exceeds a limit that would have refused it anyway.If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code, directed by @turnipdabeets. Came out of reviewing #4579 against the
sdk-specstraces contract, which surfacedRetry-Afteras an unmet requirement shared by all three OTLP pipelines rather than anything #4579 introduced.Decisions worth flagging, beyond those above:
_sendOtlpBatchmeans one implementation of the wire format and one place to fix it.PostHogFetchHttpError, which the tagged-outcome design exists to avoid._armFlushTimerkeep whichever deadline was later. Since every capture arms the timer, that pushed the deadline out on each one and the flush never fired under a steady stream. The enqueue path now leaves a pending timer alone; only the flush-settle path extends it.MAX_REQUEST_BODY_SIZE_BYTES, batches over 2 MB) is narrow. The constant is internal, so this stays reversible.The branch was reviewed by a second Claude Code agent against
sdk-specsbefore this description was written; each of its findings was independently reproduced with an executed failing test before being acted on.https://claude.ai/code/session_01Qp8JHHcGSf7mocdZtHDR29