feat: API client network-behavior RCA items (MAGE-694, MAGE-496, MAGE-500)#519
Open
evan-masseau wants to merge 18 commits into
Open
feat: API client network-behavior RCA items (MAGE-694, MAGE-496, MAGE-500)#519evan-masseau wants to merge 18 commits into
evan-masseau wants to merge 18 commits into
Conversation
* Retry all 5xx server errors (MAGE-694)
Widen the retryable classification from {429,500,502,503,504} to 429 plus the
full 5xx range (500-599). This covers transient CDN/edge failures such as
Cloudflare's 520-527 codes, which were observed during the
cannot-access-klaviyo-com incident and had zero overlap with the old allowlist,
causing data loss for the duration of the outage. 4xx codes (including 403
load-shed) remain non-retryable so the backend can still shed load.
Adds tests for Cloudflare 522/525 and the 599 upper bound.
🤖 Generated with Reca
* refine(analytics): exclude 501/505 from retryable 5xx + add bounds tests (MAGE-694)
501 (Not Implemented) and 505 (HTTP Version Not Supported) are permanent,
deterministic server errors — retrying the identical request always fails the
same way — so classify them as non-retryable (Failed) like a 4xx, via a
501, 505 -> branch placed before the in 500..599 retryable branch. The rest of
5xx (429 + 500..599 except 501/505) stays retryable.
Adds tests: 501/505 non-retryable, plus 499 (lower-bound) and 600 (upper-bound)
sanity checks that just-outside-5xx codes remain non-retryable.
-Claude
* refactor(analytics): name 5xx status constants + dedup boundary tests (CodeRabbit)
Extract named constants for the previously-inlined HTTP status literals in
parseResponse's when(responseCode): HTTP_NOT_IMPLEMENTED (501),
HTTP_VERSION_NOT_SUPPORTED (505), and the HTTP_5XX_RETRYABLE_RANGE (500..599)
range, placed alongside the existing HTTP_RETRY const. Behavior is identical.
De-dup the single-send status-code tests behind a shared
assertStatusForResponseCode(responseCode, expected) helper, removing the
repeated mock/send/assert boilerplate while keeping the distinct, readable
per-code test methods and their explanatory comments. Coverage is unchanged.
-Claude
* revert(analytics): retry entire 5xx range incl 501/505 (drop permanent-error exclusion) (MAGE-694)
An earlier commit excluded 501 (Not Implemented) and 505 (HTTP Version Not
Supported) as "permanent" server errors. Reverting that: for the SDK's fixed
request shapes over the HTTP versions Klaviyo supports, a genuine origin 501/505
is effectively unreachable, so any 5xx we actually observe is almost certainly
edge/CDN/proxy noise during an incident — exactly what MAGE-694 wants to retry.
The incident data-retention benefit outweighs the negligible wasted-retry cost
on codes we will never legitimately see.
- Drop the 501/505 -> Status.Failed when-branch so they fall through to the
retryable HTTP_5XX_RETRYABLE_RANGE branch like the rest of 500–599.
- Remove the now-unused HTTP_NOT_IMPLEMENTED / HTTP_VERSION_NOT_SUPPORTED
constants; keep HTTP_5XX_RETRYABLE_RANGE and HTTP_RETRY.
- Flip the 501/505 tests to assert PendingRetry and document the deliberate
retry. 499/600 bounds tests remain non-retryable. 4xx (incl. 403) unchanged.
-Claude
---------
Co-authored-by: Reca <reca@klaviyo.com>
Add MAX_QUEUE_SIZE = 200 constant to KlaviyoApiClient. When a new request is enqueued and the queue is at capacity, the oldest (front-of-deque) entry is evicted and its persisted payload cleared from the datastore. This matches the iOS SDK cap and prevents unbounded queue growth during request storms (e.g. a push-token registration loop that fills the queue and blocks legitimate new requests). Also removes the depth-triggered flush in NetworkRunnable (flushDepth field and corresponding guard in NetworkRunnable.run) — the timer-based flush via networkFlushIntervals is the sole remaining flush trigger, which aligns with iOS behaviour. Unit test added: Queue cap evicts oldest request on overflow and emits warning log. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BREAKING CHANGE: Config.networkFlushDepth property and Config.Builder.networkFlushDepth(Int) method are removed from the public API. Callers who configure a custom flush depth will get a compile error and must remove that call — the queue now flushes only on the timer interval (networkFlushIntervals). Queue depth is bounded by the new MAX_QUEUE_SIZE cap (200) in KlaviyoApiClient rather than used as a flush trigger. Files changed: - Config.kt: remove networkFlushDepth property and Builder method - KlaviyoConfig.kt: remove NETWORK_FLUSH_DEPTH_DEFAULT constant, singleton field, builder field, builder override, and build() assignment - KlaviyoConfigTest.kt: remove all networkFlushDepth assertions and builder calls; adjust error-log count from 9 to 8 (one fewer bad-value call) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…(MAGE-496) The queue cap evicted via apiQueue.pollFirst(), which removes the FRONT of the deque. Head-of-line requests are inserted at the front via offerFirst(), so a freshly-enqueued head-of-line request (the newest) could be evicted instead of the actual oldest request. Evict the entry with the smallest queuedTime instead. queuedTime is the persisted wall-clock enqueue timestamp, so this always targets the true oldest request and naturally protects freshly-enqueued head-of-line requests (newest queuedTime is never the min). Datastore-clear and warning log are unchanged. Tests: control queuedTime via the mocked Registry.clock to assert the oldest-by-queuedTime request is evicted, that a newer head-of-line request at the front of the deque is retained, and that the queue stays bounded at 200. -Claude
…ing it Restore Config.Builder.networkFlushDepth(Int) as a @deprecated no-op rather than a hard removal, so existing caller code keeps compiling (no source/binary break, no forced major version bump). Depth-triggered flushing is gone — the setter now does nothing and is slated for removal in a future major release. The internal depth-flush machinery (constant, field, trigger) stays removed; the queue is bounded by the MAX_QUEUE_SIZE cap. Also harden queue eviction: gate the clear/log side effects on the boolean return of ConcurrentLinkedDeque.remove(). The minByOrNull + remove pair is not atomic, so a concurrent enqueue could target the same victim; if remove() returns false the while-loop re-scans, making eviction a best-effort, self-healing soft bound rather than double-clearing an already-evicted request. -Claude
…flushes The deprecation copy said the queue "flushes only on the timer interval", but a forced flush (NetworkRunnable with force=true) still bypasses the interval. Reword both the Config.Builder interface and KlaviyoConfig.Builder impl messages to "flushes on the timer interval or when explicitly forced" (CodeRabbit). No behavior change. -Claude
…off (MAGE-500) computeRetryInterval() previously honored the Retry-After response header exactly whenever present, falling back to exponential backoff only when the header was absent. This meant a device deep in a rate-limit storm could retry too soon just because the server's rate-limit window reset to a short Retry-After. Now always compute the (network-floored, config-capped) exponential backoff and, when a usable Retry-After header is present, wait the GREATER of the two. Jitter behavior and the existing non-capping of a large Retry-After value are unchanged. 🤖 Generated with Reca
…500) 180s was more aggressive than comparable analytics SDKs (Segment caps at 300s). Align the exponential-backoff ceiling with 300s per MAGE-500 proposal #1. The cap is already enforced by computeRetryInterval(); this only changes the default. 🤖 Generated with Reca
HTTP header names are case-insensitive and an HTTP/2 stack may deliver Retry-After lowercased, which a case-sensitive map lookup would miss (falling back to backoff only). Look it up case-insensitively, comparing from the constant so a null header key (the status line) can't NPE. Adds a lowercase-header test. 🤖 Generated with Reca
…ardcoding Replace the two hardcoded 300_000L literals in the rate-limit backoff test with Registry.config.networkMaxRetryInterval so the assertion tracks the configured cap instead of duplicating the magic number (CodeRabbit DRY nit). -Claude
Replace the hardcoded 300_000L in "Caps exponential backoff interval at configured maximum" with mockConfig.networkMaxRetryInterval, so the assertion tracks the configured cap and can't silently stale if it changes — same treatment CodeRabbit prompted in KlaviyoApiClientTest (glenn-klaviyo review). -Claude
…th-triggered flush (MAGE-496) (#498)
📝 WalkthroughWalkthroughThe SDK removes queue-depth-triggered flushing, adds bounded queue eviction, broadens retryable HTTP status handling, refines ChangesNetworking queue and retry behavior
Debug network security configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KlaviyoApiClient
participant KlaviyoApiRequest
participant API
KlaviyoApiClient->>KlaviyoApiRequest: send request
KlaviyoApiRequest->>API: submit request
API-->>KlaviyoApiRequest: return 429 or 5xx response
KlaviyoApiRequest->>KlaviyoApiRequest: select max(Retry-After, exponential backoff)
KlaviyoApiRequest-->>KlaviyoApiClient: schedule retry
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Adds a debug-only source set (sample/src/debug) with a network security config trusting system + user-added CAs, plus a manifest overlay wiring it in. Debug builds only; release artifacts are unaffected. Lets an intercepting proxy (e.g. Proxyman) decrypt the sample app's HTTPS for SDK network UAT. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Attaching any network-security-config makes Android ignore android:usesCleartextTraffic; on targetSdk cleartext defaults to disallowed, so the trust-anchor-only debug NSC would silently block plain-HTTP in debug builds. Set cleartextTrafficPermitted=true on the debug base-config (debug only; release unaffected). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
evan-masseau
commented
Jul 23, 2026
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.
Description
Note: All this code was previously reviewed in individual PRs.
Consolidated API-client network-behavior fixes from the post-incident RCA: widen 5xx retry coverage, cap the request queue at 200 with oldest-first eviction, and wait
max(Retry-After, backoff). Merged down from the stacked chain into one feature branch targetingrel/4.5.0.Due Diligence
Release/Versioning Considerations
PatchContains internal changes or backwards-compatible bug fixes.MinorContains changes to the public API.MajorContains breaking changes.Public surface changes only via a
@Deprecatedno-op onConfig.Builder.networkFlushDepth(Int)(source/binary compatible); all other changes are backwards-compatible behavior. Targetingrel/4.5.0.Changelog / Code Overview
429+ all500–599with no exclusions, so CDN/edge failures (e.g. Cloudflare520–527) like those seen during the cannot-access-klaviyo.com incident are retried instead of dropped.501/505are deliberately included — for the SDK's fixed request shapes a genuine origin 501/505 is unreachable, so any 5xx we see is edge noise worth retrying (data-retention > a wasted retry).MAX_QUEUE_SIZE); at capacity, evict the oldest request by enqueue timestamp (queuedTime) rather than the front of the deque, so freshly-inserted head-of-line/priority events are protected. Eviction drains fully to the cap even if the queue was already oversized (init-merge / in-flight reinsertion). Retire the depth-triggered flush — flushing is now interval + forced only, matching iOS.Config.Builder.networkFlushDepth(Int)is kept as a@Deprecatedno-op (no source/binary break, no forced major bump).max(Retry-After, cappedExponentialBackoff)instead of Retry-After alone, and raise the backoff cap 180s → 300s. Damps sustained fleet-wide retry volume during rate-limit storms (esp.Retry-After: 0bursts) while never retrying sooner than the server asked.Test Plan
:sdk:core+:sdk:analyticsunit suites green; eviction covered by mocked-clock tests (FIFO order, head-of-line protection, true 200-cap). ktlint clean. CI green on the merged branch.On-device UAT — automated, Proxyman MITM fault-injection + mobile-mcp on the emulator (in-repo sample app, SDK 4.4.1 / Android 36). Full test plan + per-case evidence are posted in the MAGE-957 issue comments.
No regressions observed; safe to land pending standard review/CI.
Related Issues/Tickets
Summary by CodeRabbit
Retry-After(case-insensitive), with a higher max retry cap (5 minutes).networkFlushDepth) is deprecated and removed from runtime behavior; the setting no longer affects behavior and is scheduled for future removal.