fix(queue): preserve durable records across retries

[posthog]

💡 Motivation and Context

Durable event, replay, and log queues currently couple their retention to flush retry exhaustion: repeated retryable failures can clear every persisted record. Successful sends also delete the current queue head by position, which can discard never-sent replacement records when a full queue changes while transport is in flight.

This aligns the shared file-backed queue with PostHog/sdk-specs#53: retryable failures retain bounded durable records, known-offline periods consume no attempts, and acknowledgements remove only the exact persisted entries sent. HTTP 413 continues shrinking to a singleton poison record. Related investigation: #788.

Cross-SDK implementation: PostHog/posthog-android#740

💚 How did you test it?

• make test — 765 tests passed
• make lint — no serious violations
• make apiCheck — public API snapshot unchanged
• make buildSdk — iOS, macOS, and Mac Catalyst passed; tvOS/watchOS/visionOS were unavailable in the local Xcode installation
• git diff --check
• independent read-only implementation and cross-SDK reviews found no blockers

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• I updated the docs if needed.
• No breaking change or entry added to the changelog.

If releasing new changes

• Ran pnpm changeset to generate a changeset file

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Implemented with Pi using isolated implementation, scout, and review subagents. The human directed the durability contract and cross-SDK scope. Queue capacity and bounded backoff remain the resource controls; no public option or default was added.

[posthog]

🦔 PostHog Review reviewed this pull request

Nothing worth raising this time, so here's a calming picture instead:

[github-actions]

posthog-ios Compliance Report

Date: 2026-09-10 22:49:55 UTC
Duration: 234402ms

⚠️ Some Tests Failed

44/45 tests passed, 1 failed

---

Capture Tests

⚠️ 28/29 tests passed, 1 failed

View Details

Test	Status	Duration
Format Validation.Event Has Required Fields	✅	3227ms
Format Validation.Event Has Uuid	✅	2872ms
Format Validation.Event Has Lib Properties	✅	2903ms
Format Validation.Distinct Id Is String	✅	2879ms
Format Validation.Token Is Present	✅	2996ms
Format Validation.Custom Properties Preserved	✅	3147ms
Format Validation.Event Has Timestamp	✅	2848ms
Retry Behavior.Retries On 503	✅	11722ms
Retry Behavior.Does Not Retry On 400	✅	4880ms
Retry Behavior.Does Not Retry On 401	✅	2494ms
Retry Behavior.Respects Retry After Header	✅	5435ms
Retry Behavior.Implements Backoff	✅	21355ms
Retry Behavior.Retries On 500	✅	9415ms
Retry Behavior.Retries On 502	✅	9378ms
Retry Behavior.Retries On 504	✅	9403ms
Retry Behavior.Max Retries Respected	❌	21906ms
Deduplication.Generates Unique Uuids	✅	3349ms
Deduplication.Preserves Uuid On Retry	✅	9387ms
Deduplication.Preserves Uuid And Timestamp On Retry	✅	16941ms
Deduplication.Preserves Uuid And Timestamp On Batch Retry	✅	7992ms
Deduplication.No Duplicate Events In Batch	✅	3023ms
Deduplication.Different Events Have Different Uuids	✅	3037ms
Compression.Sends Gzip When Enabled	✅	495ms
Batch Format.Uses Proper Batch Structure	✅	3001ms
Batch Format.Flush With No Events Sends Nothing	✅	330ms
Batch Format.Multiple Events Batched Together	✅	3138ms
Error Handling.Does Not Retry On 403	✅	4927ms
Error Handling.Does Not Retry On 413	✅	5064ms
Error Handling.Retries On 408	✅	5534ms

Failures

retry_behavior.max_retries_respected

Expected 4 requests, got 6

Feature_Flags Tests

✅ 16/16 tests passed

View Details

Test	Status	Duration
Request Payload.Request With Person Properties Device Id	✅	3145ms
Request Payload.Flags Request Uses V2 Query Param	✅	2852ms
Request Payload.Flags Request Hits Flags Path Not Decide	✅	3046ms
Request Payload.Flags Request Omits Authorization Header	✅	2962ms
Request Payload.Token In Flags Body Matches Init	✅	3045ms
Request Payload.Groups Round Trip	✅	2969ms
Request Payload.Groups Default To Empty Object	✅	2994ms
Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It	✅	2985ms
Request Payload.Disable Geoip False Propagates As Geoip Disable False	✅	3062ms
Request Payload.Disable Geoip Omitted Defaults To False	✅	2992ms
Request Payload.Flag Keys To Evaluate Contains Only Requested Key	✅	3128ms
Request Lifecycle.No Flags Request On Init Alone	✅	166ms
Request Lifecycle.No Flags Request On Normal Capture	✅	3021ms
Request Lifecycle.Two Flag Calls Produce Two Remote Requests	✅	5665ms
Request Lifecycle.Mock Response Value Is Returned To Caller	✅	3157ms
Side Effect Events.Get Feature Flag Captures Feature Flag Called Event	✅	3240ms

[posthog]

PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

[posthog]

PostHog Review

Found 3 must fix, 3 should fix, 1 consider.

Other findings (outside the changed lines)

Valid issues on this PR's files that sit on lines GitHub won't let us comment on inline.

Report drops from every queue

Priority: should_fix | File: PostHog/PostHogSDK.swift:228-246 | Category: bug

Why we think it's a valid issue

• Checked: every PostHogQueue instantiation in the SDK, the single onRecordsDropped assignment, the PostHogReplayQueue wrapper, the retry policies that can reach a full drop for each queue, and the changeset text this PR ships.
• Found: three queues run the same generic class, and only one is wired. queue and logsQueue are both built at the same call site (PostHog/PostHogSDK.swift:229-235), but the callback at :241-246 is set on queue alone. The replay queue's inner instance is private let innerQueue: PostHogQueue<PostHogEvent> (PostHog/Replay/PostHogReplayQueue.swift:11, :39, :47), and the wrapper exposes no forwarding property, so the SDK cannot reach it even if it tried.
• Found: both unwired queues can genuinely reach a full drop. dropAllQueuedRecords fires from the time-window path (PostHog/PostHogQueue.swift:187-188) and the repeated-413 path (:217-218). The logs endpoint retries 408, 429, and 5xx (PostHog/QueueEndpoint+Factories.swift:103-105) and the snapshot endpoint shares the events policy (:57), so each can sit in a failing streak and each can be answered with 413. Replay is the most exposed of the three: snapshots are the largest payloads, which is what makes 413 handling necessary at all.
• Found: the promise is stated without qualification in shipping copy. .changeset/quick-queues-keep.md — the text that becomes the public changelog — says "every drop emits a $queue_records_dropped diagnostic event so the loss is measurable", and the callback's own doc comment at PostHog/PostHogQueue.swift:73-77 calls a full queue drop "the last remaining silent, total data-loss path".
• Impact: for buffered logs and replay snapshots the silent data-loss path this PR exists to close stays open — a full wipe still leaves only a hedgeLog line at PostHog/PostHogQueue.swift:260, so nobody can see it from ingested data. Either the two remaining queues need wiring or the changelog claim needs narrowing before release, since the current pair contradicts each other.
• Impact: the reviewer's request for a queue-type property is load-bearing rather than extra scope. The event carries only dropped_count and reason today, so once more than one queue reports, the three sources are indistinguishable in the captured data.

Issue description

Only the analytics queue receives onRecordsDropped. The logs queue and the inner replay queue use PostHogQueue but have no callback. Their full drops remain silent, although the changeset promises that every drop emits the diagnostic event.

Suggested fix

Wire callbacks for analytics, logs, and replay. Expose a forwarding callback on PostHogReplayQueue. Add a queue type property to the diagnostic event. Test one drop for each queue.

[posthog]

PostHog Review

Found 1 should fix, 2 consider.

[turnipdabeets]

Compliance is failing on the latest run — retry_behavior.max_retries_respected: Expected 4 requests, got 6 (45/45 on #786, 44/45 here), and the compliance-tests check reports pass regardless, so this merges red.

The spec left the attempt ceiling optional ("MAY end the active sequence"), so the suite is arguably the stale part — but Android passes the same change only because its backoff is exponential where ours is linear, which means it's passing by accident. I think we need the suite asserting retention instead of attempt count before this lands, or a ceiling here that ends the sequence without clearing the queue.

[posthog]

Confirmed, and I'm leaving this for a human rather than guessing at it.

The failure is real and reproduces by construction, not by timing. After this change a retryable failure never ends the flush sequence — the retry counter now only feeds the backoff delay, and it stops growing once the delay reaches the 30s ceiling. So the pause sequence is 1s, 2s, 3s … then 30s forever, and the number of attempts inside any fixed harness window is unbounded. Expected 4 requests can't be reached by tuning the delays; it needs either a real ceiling that ends the sequence, or a different assertion.

Both of the options you named are decisions rather than mechanical fixes, so I've stopped here:

1. A ceiling that ends the sequence without clearing the queue. The hard part isn't ending the sequence, it's what re-arms it — a new event being enqueued, a reachability transition, app foreground, the next explicit flush(), or simply a much longer pause. Those behave very differently for a user who backgrounds the app during an outage. It also needs a call on which knob bounds it: reusing maxRetries contradicts this PR's deliberate re-scoping of that option to push-subscription registration (which is itself an open thread here), and a new bound is a new public surface. And I can't prove any of it in this session — the harness lives in the posthog-sdk-test-harness repo and the job needs macOS plus Docker, so I can't show that a ceiling actually turns 6 requests into 4, or that it lands the same place as posthog-android#740.

2. The suite asserting retention instead of attempt count. That's an edit in the harness repo, outside this PR.

On "this merges red": also correct. The compliance job sets continue-on-error: true on the test step, and the Fail if tests failed gate is still commented out behind a "once compliance tests are stable" TODO — so the check is advisory by design right now. Turning it into a real gate is a workflow-file change, which I don't make unattended; it needs a deliberate decision from whoever owns that workflow, ideally alongside whichever of the two options above you pick.

One data point in favour of your "the suite is the stale part" reading: this PR's own unit tests already assert the retention contract directly — one covers retryable HTTP failures past maxRetries draining after recovery, and another does the same for transport failures. If the harness asserted retention, those would be the same property expressed at two levels instead of two suites disagreeing. I didn't verify the claim about Android's backoff being exponential myself — I only confirmed ours is linear.

So: a human needs to choose between the attempt ceiling (and its re-arm trigger and its bound) and changing the harness assertion, and separately decide whether to make the compliance check blocking. Nothing committed for this thread.

[turnipdabeets]

[question] After this the queue never reads maxRetries — the only caller left is PostHogPushSubscriptionHandler. Anyone who lowered it to bound retry traffic against a broken backend now gets unbounded retries, and nothing fails at build time. Is patch the intent here, or should this be a minor with the semantics change spelled out?

[posthog]

Your reading of the code is right — maxRetries now has exactly one production reader left, the push-subscription retry ceiling, and no queue reads it any more. Both halves of the question you asked have since been answered in the branch, after you wrote this:

• patch was not the intent. Commit 3774ddf changed the bump from patch to minor; that one-line change is the whole commit.
• The semantics change is now spelled out. Commit e4284e5 expanded the changeset from a single sentence into a bulleted list, and one bullet is exactly this: "Limit maxRetries to push-subscription registration; it no longer bounds event, replay, or log queue flush attempts."

At the commit you were looking at the entry really was a bare patch with no mention of maxRetries at all, so the comment was accurate when written. The property's own doc comment now states the narrowed scope too, so someone reading the declaration in an editor sees it, not just someone reading release notes.

The other half of what you raised — that anyone who lowered maxRetries to bound retry traffic against a broken backend now gets unbounded retries — is real and is not closed by relabelling the release. That is the same decision as your comment on the queue's retry path, and I've escalated it there rather than answering it twice: it needs a human call on whether to add a ceiling that ends the active flush sequence without clearing the queue, and what bound that ceiling reads.

I deliberately didn't reach for the two obvious-looking mitigations here. Marking maxRetries deprecated would be wrong — it is still live and load-bearing for push registration, so a deprecation warning would tell users to stop using a working option, and it would churn the public API snapshot. And there's no honest way to make a silently re-scoped option fail at build time short of a real public API break (rename or split into two options), which is a maintainer decision about backwards compatibility, not something I should land unattended. If you do want the option split — say maxRetries for push and a separate bound for queue flushes — that's worth deciding alongside the ceiling question on the other thread, since the two answers constrain each other.

Nothing committed for this thread.

[turnipdabeets]

[suggestion] Three user-visible changes aren't in the entry:

Suggested change

	Preserve bounded durable event, replay, and log queues across retryable upload failures, and acknowledge successful batches by their exact persisted entry identities.
	- Limit `maxRetries` to push-subscription registration — it no longer bounds event queue flush attempts.
	- Preserve durable event, replay, and log queues across retryable upload failures instead of clearing them.
	- Fix successful flushes deleting never-sent records when a full queue is replaced mid-flight.
	- Trim persisted queues to `maxQueueSize` when loading them from disk.
	- Stop retrying uploads that returned a terminal HTTP status alongside a transport error.

[posthog]

All five of these are in the entry now — it was rewritten after you commented, which is why GitHub has marked this thread outdated.

At the commit you were reviewing the changeset was a single sentence under a patch bump. Commit e4284e5 replaced it with a bulleted list covering every item you listed, and 3774ddf changed the bump to minor. Mapping your suggestion to what's there today:

• maxRetries narrowed to push-subscription registration — present, and stated a bit broader than you wrote it: it names the replay and log queues as well as events, since none of the three read the option any more.
• Durable queues preserved across retryable upload failures instead of being cleared — present, near-verbatim.
• Successful flushes no longer deleting never-sent records — present, phrased as the mechanism rather than the symptom ("acknowledge successful and terminal uploads by their exact persisted entry identities so full-queue replacements accepted during an upload are not deleted").
• Persisted queues trimmed on load — present, and it also names logs.maxBufferSize for the logs queue and says the trim is FIFO.
• Terminal HTTP status alongside a transport error — present, and it covers both halves of that change: the status is honored (terminal responses remove the sent entries) and a Retry-After that arrived with the failed transfer is no longer thrown away.

There's also a sixth bullet you wouldn't have seen, for preserving existing queued records when a write into a full queue fails — that change landed after your comment.

One reason I didn't just click your suggestion through: applying it literally now would overwrite the current list and drop that sixth bullet, so it would take the entry backwards even though the ask behind it was right. Nothing committed for this thread — the entry already says all of it.

[turnipdabeets]

[question] Passing the status through here means an error alongside a 200 or a 4xx now removes the batch, where before statusCode: nil made it retriable and it stayed queued. That reads right to me (the server already has it on 200, 400 is terminal) — just checking it's deliberate, since it's a drop path landing in a PR about not dropping.

[posthog]

Yes, deliberate — and your reasoning for why it's right matches the intent.

The framing that resolves the tension you spotted: the contract this PR is enforcing is "don't drop records whose delivery outcome is unknown." A status line that actually arrived is a known outcome, so acting on it isn't the kind of drop the PR is trying to prevent. The unknown case still retains — when no HTTP response arrives at all the status is nil, which the queue maps to its retryable sentinel and keeps the records. Previously a transport error discarded the status even when the server had already answered, which meant a 200 or a 400 got retried indefinitely on the strength of a broken response-body read.

Two details worth having, since they narrow the blast radius further than "removes the batch":

• Removal is by exact persisted entry identity, not by count or position. A record enqueued while the upload was in flight is not removed, even on a 200 that arrived with an error.
• The behavior is pinned by tests added in this PR, at both layers. At the API layer, a test asserts the status survives alongside an error across 200, 400, 408, 429 and 503 — 200 and 400 are explicit cases, so a future change that reverts to discarding the status will fail. At the queue layer, "received HTTP disposition wins over an accompanying transport error" runs the same status set for both the events and replay queues and asserts the split you described: retryable statuses keep every record, terminal ones remove only the entries that were actually sent. It then reloads the queue from disk to confirm the durable state, not just the in-memory view.

There is also a code comment on the hoisted header parse explaining the underlying URLSession behavior — headers can arrive and the transfer can still fail afterwards — so the reason this branch keeps the status is recorded where someone will read it.

One thing I'm deliberately not claiming: this helper is shared with the push-subscription register/delete calls, and that consumer's retryable set is narrower than the queues' (429 and 5xx only). So the status pass-through does land differently there. There's a separate comment on this same line about that case; I'm assessing it on its own thread rather than folding it into this answer.

No change made for this thread.

[turnipdabeets]

[nit] If the write throws we've already evicted and deleted the oldest entry, so we lose a record and return nil — which the caller reads as "nothing was evicted" and skips the "Queue is full" log. Evicting only after a successful write would keep the two in step.

[posthog]

Fixed already — commit e4284e5 does exactly what you suggested. The write was moved ahead of the capacity check, so the eviction only happens once the new record is safely on disk. It's a pure reorder of three lines inside the same lock.

So the failure path now behaves the way you wanted: if the write throws, nothing has been evicted and nothing deleted, the existing records are intact, and the nil return honestly means "nothing was evicted" rather than hiding a lost record. The caller's "Queue is full" log is back in step too, since a non-nil return is now only reachable after a successful write.

The same commit added a regression test for it. It fills a queue with capacity 2, makes the directory read-only so the write really does throw, and then asserts the add returns nil, the depth is still 2, the two original entries are unchanged and still readable, and the directory contains exactly the original two files — so a half-written file can't be left behind either. It then restores write permission and asserts a successful add does return the evicted id, which pins both directions of the nil contract, not just the failure case.

Two details worth knowing, since neither is obvious from the diff:

• On the success path the new file is written before the capacity check, so the directory briefly holds one more file than the configured maximum. The in-memory index never does, because the eviction runs before the new name is appended — and because the new name isn't in the index yet, a record can't evict itself. Nothing after the write in that block can throw, so the reorder can't orphan the file it just wrote.
• The regression test is skipped when the test suite runs as root, since chmod doesn't restrict root and the write would succeed. That's fine for the macOS CI runners, which run as a normal user, but worth remembering if anyone runs the suite in a root container and wonders why the case looks uncovered.

Nothing committed for this thread.

[posthog]

PostHog Review

Found 1 should fix, 2 consider.

[posthog]

Clamp server-controlled Retry-After delays

Why we think it's a valid issue

• Checked: parseRetryAfter in PostHog/Utils/DateUtils.swift:51-61, the single consumer of PostHogUploadInfo.retryAfter in the queue, the pause gate (pauseFor, pauseReason, canFlush), every reset of pausedUntil, and the diff of PostHog/PostHogApi.swift against main.
• Found: No ceiling exists anywhere on the path. parseRetryAfter returns any non-negative delta-seconds (DateUtils.swift:52-53) and any future HTTP-date (DateUtils.swift:56-57). PostHog/PostHogQueue.swift:159-161 computes let delay = max(backoffDelay, result.retryAfter ?? 0) and passes it straight to pauseFor, so the min(..., maxRetryDelay) clamp on line 159 applies to the SDK's own backoff only, never to the server value.
• Found: The pause has no other exit. pauseFor writes pausedUntil = now() + delay (PostHogQueue.swift:478-481), and pauseReason() blocks every flush while that date is in the future (PostHogQueue.swift:486-491). The only reset is resetRetryState() (PostHogQueue.swift:205-211), which runs after a completed upload — an upload the pause prevents. pausedUntil is in-memory, so recovery needs a process restart.
• Found: The pull request does widen the exposed path. On main the transport-error branch returned PostHogUploadInfo(statusCode: nil, error: error) with no retryAfter, so that case fell back to the bounded 30-second backoff. PostHog/PostHogApi.swift:20-27 now parses the header before the error branch and forwards it together with the real status code.
• Found: The claim about a documented maximum in the logs contract is not verifiable inside this repository. The repository has no such document, so the verdict rests on the reachable behavior alone, not on the contract claim.
• Impact: A rate limiter or proxy in front of the ingestion host that answers 429 with a large value (for example Retry-After: 86400) stops all event, replay, and log flushes for the rest of the app process. Under this pull request's retain-on-retry behavior the records stay durable, so the queue fills to maxQueueSize and add then evicts the oldest record on each write (PostHogQueue.swift:359-361). The result is silent loss of the earliest analytics data for that session. The fix is one clamp at the point where the delay is built.
• Priority: Lowered to consider. The unbounded pause at PostHogQueue.swift:160 already exists on main; this pull request extends it to one extra sub-case rather than introducing it. The state is in-memory, so the next app launch recovers, and the trigger needs a server value far larger than normal rate-limit delays. That makes it worthwhile hardening, not a defect that blocks this merge.

Issue description

The code forwards any non-negative Retry-After value without a maximum. A header such as Retry-After: 31536000 blocks all flushes for one year. New records can then evict older records at capacity. This violates the logs contract, which requires a documented maximum. This PR also extends the unbounded value to the response-plus-error path.

Suggested fix

Define a maximum Retry-After delay. Use max(ownBackoff, min(parsedRetryAfter, maximum)) when the queue creates its pause deadline. Add tests for large delta-seconds and future HTTP-date values, with and without a transport error.

Prompt to fix with AI (copy-paste)

## Context
@PostHog/PostHogApi.swift#L20-27

<issue_description>
The code forwards any non-negative `Retry-After` value without a maximum. A header such as `Retry-After: 31536000` blocks all flushes for one year. New records can then evict older records at capacity. This violates the logs contract, which requires a documented maximum. This PR also extends the unbounded value to the response-plus-error path.
</issue_description>

<issue_validation>
- **Checked:** `parseRetryAfter` in PostHog/Utils/DateUtils.swift:51-61, the single consumer of `PostHogUploadInfo.retryAfter` in the queue, the pause gate (`pauseFor`, `pauseReason`, `canFlush`), every reset of `pausedUntil`, and the diff of PostHog/PostHogApi.swift against `main`.
- **Found:** No ceiling exists anywhere on the path. `parseRetryAfter` returns any non-negative delta-seconds (DateUtils.swift:52-53) and any future HTTP-date (DateUtils.swift:56-57). PostHog/PostHogQueue.swift:159-161 computes `let delay = max(backoffDelay, result.retryAfter ?? 0)` and passes it straight to `pauseFor`, so the `min(..., maxRetryDelay)` clamp on line 159 applies to the SDK's own backoff only, never to the server value.
- **Found:** The pause has no other exit. `pauseFor` writes `pausedUntil = now() + delay` (PostHogQueue.swift:478-481), and `pauseReason()` blocks every flush while that date is in the future (PostHogQueue.swift:486-491). The only reset is `resetRetryState()` (PostHogQueue.swift:205-211), which runs after a completed upload — an upload the pause prevents. `pausedUntil` is in-memory, so recovery needs a process restart.
- **Found:** The pull request does widen the exposed path. On `main` the transport-error branch returned `PostHogUploadInfo(statusCode: nil, error: error)` with no `retryAfter`, so that case fell back to the bounded 30-second backoff. PostHog/PostHogApi.swift:20-27 now parses the header before the error branch and forwards it together with the real status code.
- **Found:** The claim about a documented maximum in the logs contract is not verifiable inside this repository. The repository has no such document, so the verdict rests on the reachable behavior alone, not on the contract claim.
- **Impact:** A rate limiter or proxy in front of the ingestion host that answers 429 with a large value (for example `Retry-After: 86400`) stops all event, replay, and log flushes for the rest of the app process. Under this pull request's retain-on-retry behavior the records stay durable, so the queue fills to `maxQueueSize` and `add` then evicts the oldest record on each write (PostHogQueue.swift:359-361). The result is silent loss of the earliest analytics data for that session. The fix is one clamp at the point where the delay is built.
- **Priority:** Lowered to `consider`. The unbounded pause at PostHogQueue.swift:160 already exists on `main`; this pull request extends it to one extra sub-case rather than introducing it. The state is in-memory, so the next app launch recovers, and the trigger needs a server value far larger than normal rate-limit delays. That makes it worthwhile hardening, not a defect that blocks this merge.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Define a maximum `Retry-After` delay. Use `max(ownBackoff, min(parsedRetryAfter, maximum))` when the queue creates its pause deadline. Add tests for large delta-seconds and future HTTP-date values, with and without a transport error.
</potential_solution>

[posthog]

I checked this against the current code and the finding holds up — there is genuinely no ceiling anywhere on the path. parseRetryAfter accepts any non-negative delta-seconds and any future HTTP-date, and the queue builds its pause as the larger of its own bounded backoff and the raw server value, so the 30-second clamp only ever binds the SDK's own number.

I also confirmed the part that makes it bite: the pause has no other way out. Nothing but a completed upload clears the deadline, and an upload is exactly what the pause prevents. I specifically checked whether regaining connectivity rescues it — it doesn't; the reachability handler only clears the separate "unreachable" flag and its flush attempt is still gated by the deadline. The deadline isn't persisted, so a process restart is the only recovery.

Where I land differently is on doing it here. Two things:

This is a pre-existing behavior, not something the PR introduces. The max(ownBackoff, serverValue) line is unchanged context in this diff — an ordinary 429 with a large Retry-After and a clean response already parks the queue for that duration on the base branch, with the same eviction-at-capacity consequence. What this PR adds is one narrow extra sub-case: a response that carried the header and then failed mid-transfer, which previously fell back to the bounded 30-second backoff. So the hardening is worth doing, but it isn't this PR's regression to absorb.

The fix needs a number that nothing in this repository can supply. That's the real blocker for me doing it unattended:

• Reusing the existing 30-second backoff ceiling would be the wrong answer — it would make the SDK ignore a server that legitimately asks for a longer cooldown, which defeats the point of rate limiting. Any other value is a judgment call about client data freshness versus protecting ingestion, and there's no precedent in the SDK to copy: the only two consumers of the parsed header both pass it through unclamped.
• The suggested justification ("the logs contract requires a documented maximum") can't be used to derive it either — as the finding itself concedes, that document isn't in this repo, and I found no decision record for it.
• Where the clamp goes changes behavior in ways the current tests notice. Clamping inside the parser would break the existing test asserting a 120-second header parses as 120; clamping where the queue builds the deadline wouldn't. Those are two defensible readings of the same suggestion.
• A clamp that only covers the queue leaves the identical unclamped pattern in the push-subscription retry path one file over, which this PR doesn't touch and which has its own test asserting it honors the header. Doing that one too means editing code outside this change.
• This PR is deliberately aligned with the cross-SDK spec work and has an Android counterpart. A maximum Retry-After is precisely the kind of value that wants to be specified once and matched across SDKs rather than picked in iOS alone — and the external compliance suite has a respects_retry_after_header case that I can't run from here to check the interaction.

So what a human needs to decide: the ceiling value and whether it's a constant or a configurable option; whether it's specified in the spec first so Android matches; whether it clamps at the parser or at the pause deadline; whether the push path is included; and whether it lands as its own hardening PR against the base branch, which given it's a pre-existing issue is what I'd suggest — that framing also lets it cover both consumers instead of half of them.

One process note in the interest of not overstating anything: this environment has no Swift toolchain, so I couldn't have run the SDK's lint or tests for a fix here even if the value question were settled, and the suggestion asks for new tests over large delta-seconds and future HTTP-date values. Nothing committed for this thread.

[posthog]

Notifier failure bypasses Wi-Fi-only mode

Why we think it's a valid issue

• Checked: The order of operations in start() (PostHog/PostHogQueue.swift:234-249), the pause rules in updatePauseState (PostHogQueue.swift:458-475), the flush gate (pauseReason/canFlush, PostHogQueue.swift:486-499), the vendored Reachability (connection, startNotifier), the URLSession setup in PostHogApi.init, and the same code on main.
• Found: The sequence is real. The snapshot at PostHogQueue.swift:234-239 runs before startNotifier(), and updatePauseState sets paused = true for a .wifi queue on cellular (PostHogQueue.swift:467-469). The catch at PostHogQueue.swift:247-248 then sets paused = false unconditionally, with no test of why the pause was set.
• Found: paused is the only enforcement point for dataMode. grep shows dataMode is read at PostHogConfig.swift:104 and inside updatePauseState alone; flush() and take() consult paused/pausedUntil only. No code sets allowsCellularAccess, so the URLSession built in PostHogApi.init (PostHogApi.swift:67-79) keeps the platform default and permits cellular.
• Found: The consequence is worse than the finding states. A failed notifier delivers no callbacks at all, so no later transition can restore the pause. The queue sends over cellular for the whole process lifetime, not until the next transition.
• Found: Reachability.connection (PostHog/Utils/Reachability.swift:121-127) lazily calls setReachabilityFlags() when flags is nil, so a current connection value is readable without a running notifier. A re-check at flush time, or keeping only the .wifi-mode pause, is therefore cheap and needs no new machinery.
• Found: The catch also protects a real case. When SCNetworkReachabilityGetFlags itself fails, connection reports .unavailable and the snapshot pauses the queue; clearing the pause there is correct, because otherwise all telemetry stops in an environment where the network works but the flags are unreadable. The defect is that one branch — .wifi mode on a readable cellular connection — is cleared together with it.
• Impact: A developer who sets config.dataMode = .wifi gets analytics, replay, and log uploads over the end user's cellular plan, which is the exact outcome the option exists to prevent. The trigger needs SCNetworkReachabilitySetCallback or SCNetworkReachabilitySetDispatchQueue to fail while the flags stay readable, plus a non-default .wifi mode.
• Priority: Lowered to consider. This is not a regression: on main the same failure left paused at its false default, with no snapshot at all, so the queue already sent over cellular. The pull request adds a correct snapshot and then discards one branch of it. The trigger is a rare SystemConfiguration failure combined with a non-default setting, so it is a worthwhile refinement of a deliberate, commented trade-off rather than a merge blocker.

Issue description

The snapshot can pause a .wifi queue on cellular. If startNotifier() fails, the catch clears that pause. Upload requests still allow cellular access. The queue can send over cellular until another reachability transition occurs.

Suggested fix

Do not clear a pause caused by cellular access in .wifi mode. Retry reachability checks, or disable cellular access on queued upload requests. Add a notifier-failure test with a cellular snapshot.

Prompt to fix with AI (copy-paste)

## Context
@PostHog/PostHogQueue.swift#L234-248

<issue_description>
The snapshot can pause a `.wifi` queue on cellular. If `startNotifier()` fails, the catch clears that pause. Upload requests still allow cellular access. The queue can send over cellular until another reachability transition occurs.
</issue_description>

<issue_validation>
- **Checked:** The order of operations in `start()` (PostHog/PostHogQueue.swift:234-249), the pause rules in `updatePauseState` (PostHogQueue.swift:458-475), the flush gate (`pauseReason`/`canFlush`, PostHogQueue.swift:486-499), the vendored `Reachability` (`connection`, `startNotifier`), the URLSession setup in `PostHogApi.init`, and the same code on `main`.
- **Found:** The sequence is real. The snapshot at PostHogQueue.swift:234-239 runs before `startNotifier()`, and `updatePauseState` sets `paused = true` for a `.wifi` queue on cellular (PostHogQueue.swift:467-469). The catch at PostHogQueue.swift:247-248 then sets `paused = false` unconditionally, with no test of why the pause was set.
- **Found:** `paused` is the only enforcement point for `dataMode`. `grep` shows `dataMode` is read at PostHogConfig.swift:104 and inside `updatePauseState` alone; `flush()` and `take()` consult `paused`/`pausedUntil` only. No code sets `allowsCellularAccess`, so the URLSession built in `PostHogApi.init` (PostHogApi.swift:67-79) keeps the platform default and permits cellular.
- **Found:** The consequence is worse than the finding states. A failed notifier delivers no callbacks at all, so no later transition can restore the pause. The queue sends over cellular for the whole process lifetime, not until the next transition.
- **Found:** `Reachability.connection` (PostHog/Utils/Reachability.swift:121-127) lazily calls `setReachabilityFlags()` when `flags` is nil, so a current connection value is readable without a running notifier. A re-check at flush time, or keeping only the `.wifi`-mode pause, is therefore cheap and needs no new machinery.
- **Found:** The catch also protects a real case. When `SCNetworkReachabilityGetFlags` itself fails, `connection` reports `.unavailable` and the snapshot pauses the queue; clearing the pause there is correct, because otherwise all telemetry stops in an environment where the network works but the flags are unreadable. The defect is that one branch — `.wifi` mode on a readable cellular connection — is cleared together with it.
- **Impact:** A developer who sets `config.dataMode = .wifi` gets analytics, replay, and log uploads over the end user's cellular plan, which is the exact outcome the option exists to prevent. The trigger needs `SCNetworkReachabilitySetCallback` or `SCNetworkReachabilitySetDispatchQueue` to fail while the flags stay readable, plus a non-default `.wifi` mode.
- **Priority:** Lowered to `consider`. This is not a regression: on `main` the same failure left `paused` at its `false` default, with no snapshot at all, so the queue already sent over cellular. The pull request adds a correct snapshot and then discards one branch of it. The trigger is a rare SystemConfiguration failure combined with a non-default setting, so it is a worthwhile refinement of a deliberate, commented trade-off rather than a merge blocker.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Do not clear a pause caused by cellular access in `.wifi` mode. Retry reachability checks, or disable cellular access on queued upload requests. Add a notifier-failure test with a cellular snapshot.
</potential_solution>

[posthog]

I verified this end to end and the mechanics are exactly as described: the snapshot can pause a .wifi queue on cellular, and the notifier-failure catch then clears that pause without checking why it was set. I also confirmed the two things that make it matter — the pause flag is the only place dataMode is enforced anywhere in the SDK, and nothing sets allowsCellularAccess, so the upload session is free to use cellular. And you're right that it's for the process lifetime, not until the next transition, since a failed notifier delivers no callbacks at all.

I also confirmed it isn't a regression. On the base branch there was no snapshot at all and dataMode was only consulted inside the reachability callback, so a failed notifier left the queue unpaused and it uploaded over cellular just the same. This PR added a correct snapshot and then deliberately discards one branch of it — that discard is its own commit with a comment explaining the reasoning, so it's a considered trade-off rather than an oversight.

One correction that changes what the fix costs: the suggestion that a re-check at flush time is cheap doesn't hold against the vendored reachability helper. Its connection property only reads fresh flags when it has none cached — and the snapshot has already populated them by the time the notifier fails. The method that refreshes them is file-private and the cache is read-only from outside, so the queue can neither force a refresh nor clear it. Polling connection on each flush would return the same stale "cellular" answer forever.

That has a knock-on effect on the minimal fix. Simply keeping the .wifi-mode pause isn't strictly better than what's there — with nothing able to refresh the connection value, it turns "uploads over cellular" into "never uploads again for the rest of the process, even after the user joins Wi-Fi", at which point records buffer up to the queue limit and start evicting the oldest. So it swaps one silent data problem for another, and which one to prefer is a genuine product question about whether dataMode = .wifi is a hard constraint (never spend the user's cellular data, accept the loss) or a strong preference.

The options that actually resolve it are all bigger than this line:

• Extend the in-tree reachability helper so the connection state can be refreshed on demand. That component is shared by the events, replay, and logs queues, so it's not a local change.
• Set allowsCellularAccess = false on the upload requests when .wifi mode is on. This is the most robust answer — the OS enforces it and reachability accuracy stops mattering entirely — but it's a new enforcement mechanism in the API layer that this PR doesn't touch, and it needs thought about how it interacts with the custom URLSessionConfiguration the compliance adapter injects.

So what needs deciding by a human: which failure mode .wifi mode should prefer when reachability monitoring is unavailable, and whether enforcement should move off the pause flag onto allowsCellularAccess where the OS guarantees it. My own read is that this belongs in a separate change against the base branch rather than here — it's pre-existing, and the allowsCellularAccess route would fix it properly instead of refining a flag that can't be kept accurate.

Worth flagging for whoever picks it up: .wifi mode currently has no behavioral test coverage at all — the only test that mentions it asserts the default value. So the notifier-failure test you suggest would be the first test of this path, which is a good reason to do it deliberately rather than as a drive-by. Nothing committed for this thread.

[posthog]

PostHog Review

Found 1 should fix, 1 consider.

[turnipdabeets]

The logs spec says the SDK "SHALL end the active failure-driven sequence" after maxRetries on the same batch (logs/spec.md, Error handling and retries). retry-queue only makes that a MAY, which is what makes this look optional — but logs binds the log queue, and after this change nothing ends the sequence for any of the three. The compliance suite catches it: max_retries_respected expects 4 requests and gets 6 at head.

I think we need a ceiling that ends the active sequence without clearing the queue. The push handler already has that shape — attempt > config.maxRetries sets halted = true and keeps the record for next launch.

[turnipdabeets]

[question] The spec clamps the header before taking the longer of the two — max(ownBackoff, min(parsedRetryAfter, documentedMaximum)) (logs/spec.md, Error handling and retries) — because nothing upstream bounds Retry-After. As written a proxy sending Retry-After: 86400 parks the queue for a day. This is pre-existing rather than something the PR introduces, so: follow-up issue, or clamp it here while we're in this function?

[turnipdabeets]

[nit] The write and the deleteSafely just below now both run inside itemsLock; before, neither did. With a full queue during an outage every capture pays a write plus an unlink under the lock, on the caller's thread. remove(ids:) and reindexFromDisk use the collect-under-lock, IO-after shape — we could write the file before taking the lock and unlink the evicted id after releasing it, and still keep the capacity check atomic.

[turnipdabeets]

[nit] This takes the last production caller of pop(_:), and delete(index:) lost its own when PostHogQueue.add stopped calling it — both are now reachable only from PostHogFileBackedQueueAlignmentTest. Worth deleting them, or a line on why they're kept?

[turnipdabeets]

[suggestion] A few of these carry their rationale inline ("because a redirect does not confirm delivery", "so a pending logout unregister survives"), which belongs in the PR body, and a couple lean on internal vocabulary ("bounded durable", "persisted entry identities") rather than what a user observes. The 3xx sentence is also a follow-up to the bullet above it, so it can fold in. Missing entirely: queues now pause while the device reports no network and flush as soon as connectivity returns, which is the most user-visible change here.

Suggested change

	- Preserve bounded durable event, replay, and log queues across retryable upload failures instead of clearing them.
	- Limit `maxRetries` to push-subscription registration; it no longer bounds event, replay, or log queue flush attempts.
	- Acknowledge successful and terminal uploads by their exact persisted entry identities so full-queue replacements accepted during an upload are not deleted.
	- Trim persisted queues to `maxQueueSize` (or `logs.maxBufferSize` for logs) in FIFO order when loading them from disk.
	- Honor the received HTTP status and `Retry-After` when an upload also returns a transport error: successful and terminal responses remove the sent entries, while retryable responses retain them. A `3xx` left on a failed request is reported as no status, because a redirect does not confirm delivery to the final host.
	- Preserve existing queued records when writing a new record to a full queue fails.
	- Retry push-subscription requests that answer with HTTP 408 instead of treating the timeout as terminal, so a pending logout unregister survives until it succeeds.
	- Keep events, replay snapshots, and logs on disk when an upload fails with a retryable error, instead of clearing the queue.
	- Limit `maxRetries` to push-subscription registration; it no longer bounds event, replay, or log upload attempts.
	- Fix records captured while an upload is in flight being deleted when that upload succeeds.
	- Fix a failed write to a full queue evicting an existing record.
	- Apply the HTTP status and `Retry-After` from a response that arrives alongside a transport error, instead of always retrying.
	- Pause uploads while the device reports no network, and flush as soon as connectivity returns.
	- Trim stored queues to `maxQueueSize` (`logs.maxBufferSize` for logs), oldest first, when loading them from disk.
	- Retry push-subscription requests that return HTTP 408 instead of discarding them.

[turnipdabeets]

[question] Re-scoping maxRetries is a silent behavior break — anyone who lowered it to bound retry traffic gets a different SDK with nothing failing at build time. Our changelog style puts a **Breaking:** prefix on those and sorts them first, but that reads oddly under a minor bump. Is minor the call because there's no API break, or should this carry the prefix and go major?