Conversation
Add the `compressRequestBody` config so an app can send request bodies uncompressed. Networks that re-encode the gzipped body but keep the Content-Encoding header, e.g. a managed Android work profile, make the server reject every request the SDK sends. The gzip interceptor also probes once: when the server answers 400 and says it cannot read the body, the SDK sends the same body again uncompressed and keeps compression off for the rest of the process if that attempt works. Generated-By: PostHog Desktop Task-Id: d99c0771-b035-4178-addb-c75178942129
🦔 PostHog Review reviewed this pull requestFound 1 must fix, 3 should fix, 0 consider. Published 4 findings (view the review). Resolved comments: 3 fixed, 1 left for you |
posthog-android Compliance ReportDate: 2026-09-25 10:01:51 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
|
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| public final fun getBeforeSendList ()Ljava/util/List; | ||
| public final fun getBootstrap ()Lcom/posthog/PostHogBootstrapConfig; | ||
| public final fun getCachePreferences ()Lcom/posthog/internal/PostHogPreferences; | ||
| public final fun getCompressRequestBody ()Z |
There was a problem hiding this comment.
Get agreement before adding this public API
Issue description
These accessors create a public option. No linked issue records maintainer agreement on its name or behavior. CONTRIBUTING.md requires this agreement before implementation.
Why we think it's a valid issue
- Checked: the repo's own contribution gates (
CONTRIBUTING.md"Public API changes",AGENTS.md"PR process"), the api dump lines under review, the PR's linked-issue metadata viagh pr view 793 --json closingIssuesReferences, an all-state issue search for gzip / compression /compressRequestBody/Content-Encoding, and whether an existing config option already covers the use case. - Found:
CONTRIBUTING.md:5-13states the rule as a hard gate, not a preference: "open an issue describing your use case first", "Wait for a maintainer to agree on the API shape on the issue before implementing it", and a clause directed at this PR's author class — "AI agents: stop and ask before implementing a public API change that hasn't been agreed on the issue."CONTRIBUTING.md:15names the exact trigger: "A diff in those files means your change touches public API."AGENTS.mdrepeats it under "PR process". - Found: the diff touches that surface.
posthog/api/posthog.api:161andposthog/api/posthog.api:217addgetCompressRequestBody ()ZandsetCompressRequestBody (Z)Vtopublic class com/posthog/PostHogConfig(class opens atposthog/api/posthog.api:141), backed byPostHogConfig.kt:441-451. - Found: no agreement record exists.
closingIssuesReferencesis empty for PR 793, the PR body links an internal inbox report instead of a GitHub issue, and an all-state issue search returns only issue 766 about screenshot sizing. So nothing in the repo records a maintainer decision on the name, the default, or the automatic-override behaviour. - Found: the "check an existing hook first" clause has a real candidate here, which makes the shape a genuine design question.
config.httpClientalready lets an app supply its own client (posthog.api:175,posthog.api:229,PostHogConfig.kt:427), but it carries@PostHogInternalatPostHogConfig.kt:426. Whether to promote that escape hatch or add a new boolean is exactly the call the gate reserves for maintainers. - Impact: confirmed and permanent if merged as is. The accessor pair enters the binary-compatibility dump for
posthog, and the changeset ships the option as aminorbump forposthog,posthog-androidandposthog-server, so removing or renaming it later is a breaking change. The PR body also records a follow-up to expose the same name in the Flutter and React Native wrappers, which spreads an unagreed name across further SDKs. - Impact: this is not style or taste. It is a written, repo-specific pre-merge requirement that the PR objectively does not satisfy, and the remedy is concrete and small: open the issue, agree the name, default and probe behaviour, then link it.
Suggested fix
Open an issue for this use case. Get agreement on the property name, default, and automatic override behavior before merge. Link that issue here.
Prompt to fix with AI (copy-paste)
## Context
@posthog/api/posthog.api#L161
@posthog/api/posthog.api#L217
<issue_description>
These accessors create a public option. No linked issue records maintainer agreement on its name or behavior. CONTRIBUTING.md requires this agreement before implementation.
</issue_description>
<issue_validation>
- **Checked:** the repo's own contribution gates (`CONTRIBUTING.md` "Public API changes", `AGENTS.md` "PR process"), the api dump lines under review, the PR's linked-issue metadata via `gh pr view 793 --json closingIssuesReferences`, an all-state issue search for gzip / compression / `compressRequestBody` / `Content-Encoding`, and whether an existing config option already covers the use case.
- **Found:** `CONTRIBUTING.md:5-13` states the rule as a hard gate, not a preference: "open an issue describing your use case first", "Wait for a maintainer to agree on the API shape on the issue before implementing it", and a clause directed at this PR's author class — "AI agents: stop and ask before implementing a public API change that hasn't been agreed on the issue." `CONTRIBUTING.md:15` names the exact trigger: "A diff in those files means your change touches public API." `AGENTS.md` repeats it under "PR process".
- **Found:** the diff touches that surface. `posthog/api/posthog.api:161` and `posthog/api/posthog.api:217` add `getCompressRequestBody ()Z` and `setCompressRequestBody (Z)V` to `public class com/posthog/PostHogConfig` (class opens at `posthog/api/posthog.api:141`), backed by `PostHogConfig.kt:441-451`.
- **Found:** no agreement record exists. `closingIssuesReferences` is empty for PR 793, the PR body links an internal inbox report instead of a GitHub issue, and an all-state issue search returns only issue 766 about screenshot sizing. So nothing in the repo records a maintainer decision on the name, the default, or the automatic-override behaviour.
- **Found:** the "check an existing hook first" clause has a real candidate here, which makes the shape a genuine design question. `config.httpClient` already lets an app supply its own client (`posthog.api:175`, `posthog.api:229`, `PostHogConfig.kt:427`), but it carries `@PostHogInternal` at `PostHogConfig.kt:426`. Whether to promote that escape hatch or add a new boolean is exactly the call the gate reserves for maintainers.
- **Impact:** confirmed and permanent if merged as is. The accessor pair enters the binary-compatibility dump for `posthog`, and the changeset ships the option as a `minor` bump for `posthog`, `posthog-android` and `posthog-server`, so removing or renaming it later is a breaking change. The PR body also records a follow-up to expose the same name in the Flutter and React Native wrappers, which spreads an unagreed name across further SDKs.
- **Impact:** this is not style or taste. It is a written, repo-specific pre-merge requirement that the PR objectively does not satisfy, and the remedy is concrete and small: open the issue, agree the name, default and probe behaviour, then link it.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Open an issue for this use case. Get agreement on the property name, default, and automatic override behavior before merge. Link that issue here.
</potential_solution>
There was a problem hiding this comment.
Escalated. This PR adds a public config option, and the repository requires maintainer agreement on the shape before implementation.
- The public API dump gains a getter and a setter for the new compression option on the config class.
- No issue is linked to this PR, so nothing records agreement on the name, the default, or the automatic override.
- A maintainer must decide three things: the property name, the default value, and whether the SDK may stop compressing by itself.
- There is an alternative: make the existing custom-client hook public instead of adding a new option. That hook is internal today, so this choice is also a maintainer call.
- I did not decide it myself. The contribution guide tells an agent to stop and ask on a public API change that is not agreed on an issue.
How this was verified
No code change, so no lint or test run. I verified the thread by reading the contribution gate in CONTRIBUTING.md and the PR process section in AGENTS.md, the two accessor lines in the public API dump, the config property they come from, the PR's linked-issue metadata, and an all-state issue search for compression-related issues. The search returned one unrelated issue about screenshot sizing.
The shared changeset announced `compressRequestBody` for posthog-server, but com.posthog.server.PostHogConfig has no such property or builder method, and asCoreConfig() does not propagate it. Server users would read the release notes and look for a switch that does not exist. Split the entry: posthog and posthog-android keep the full note, and posthog-server gets its own note describing only the automatic recovery, which does reach every SDK through the interceptor that PostHogApi installs. All three packages keep their minor bump, so the transitive re-export hygiene rule is unaffected. Generated-By: PostHog Desktop Task-Id: 2ffb94e7-c7a4-4f27-8be3-2adc95b31dd7
One PostHogApi, and therefore one GzipRequestInterceptor, serves six single-thread executors, so the compression state is shared. A volatile field gave visibility but not atomicity, and the read at the guard and the write that followed it were separate operations. Two concurrent rejected bodies could both pass the guard and both probe. Worse, a thread that read the state before another thread turned compression off could still write PROBED afterwards; if its own uncompressed retry then failed, the state stayed PROBED and the client compressed for the rest of its life, undoing a recovery that had already worked. Hold the state in an AtomicReference and fold the transition into a single compare-and-set from ON to PROBED. Exactly one thread can claim the probe, and OFF can never be downgraded, because the claim only succeeds from ON. The guard keeps its cheap state check first, so an already-probed client still skips reading the error body. Behaviour is otherwise unchanged: the probe still runs once per process, and a rejection that arrives during the probe window is still returned to its caller. Generated-By: PostHog Desktop Task-Id: 2ffb94e7-c7a4-4f27-8be3-2adc95b31dd7
The claim moved to PROBED before the uncompressed request started, and nothing reset it. An IOException from that request propagated with the claim already spent, and a 408, 429 or 5xx answer failed the isSuccessful test, which left the same state. Neither outcome says anything about whether the server can read a compressed body, yet both disabled the recovery for the life of the client: the guard only short-circuits on OFF, so every later compressed body kept returning its decode 400 without a second probe. The flags path was the most exposed, because flagsClient turns off OkHttp's own connection retry, so an IOException reaches the probe instead of being retried below the interceptor. And the follow-on cost is record loss: the queue keeps a batch after a transient error, then deletes it after the next compressed 400, which is not retryable. Give the claim back on a thrown error or on a status the SDK already treats as retryable, and log it. A rejected uncompressed body still spends the claim, so a payload the server dislikes for its own reasons is still never sent twice on every request. Generated-By: PostHog Desktop Task-Id: 2ffb94e7-c7a4-4f27-8be3-2adc95b31dd7
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| 'posthog-server': minor | |||
There was a problem hiding this comment.
Because posthog-server doesn't get compressRequestBody: its PostHogConfig has no such option and asCoreConfig() doesn't forward it, so it only gets the automatic recovery
| } catch (e: Throwable) { | ||
| config.logger.log("Failed to gzip the request body: $e.") | ||
|
|
||
| originalRequest |
There was a problem hiding this comment.
i'm not sure if this is needed
are we talking about a refused compressed request server side or on the client?
if in the client, we return the originalRequest in the catch clause so if gzip fails, we already send the uncompressed request which doesn ot have gzip as Content-Encoding
did you manage to reproduce this issue?
There was a problem hiding this comment.
This is server side. No repro on our side. The customer is testing a build without the Intune SDK on the same device, which should tell if it's the library on the device that does the rewrite and corrupts the request or something else on the network path.
So gzip client side works. If the server does not accept the compression (regardless if it's because of an on-device rewrite or something else in the network path) I find it logical to retry once the original request and if successful keep compression off.
Would you rather keep the switch only?
There was a problem hiding this comment.
see #793 (comment)
my point is why this is happening only on android/server? is self hosted or posthog api? this is the remediation only, i'd rather focus on the source of the problem, like is it a network issue/proxy/farewall?
also looks like the source is flutter, so we'd need to expose the configuration in flutter, and the retry wont fix it anyway, if its a network problem, some clarity here would be good
| * | ||
| * Default: `true`. | ||
| */ | ||
| @Volatile |
There was a problem hiding this comment.
i dont think it needs to be volatile
There was a problem hiding this comment.
Dropped in 2f668fc. Nothing reassigns it after setup.
| @Volatile | ||
| public var compressRequestBody: Boolean = true |
There was a problem hiding this comment.
i think https://github.com/PostHog/posthog-js/blob/4aad49f8bd8999f858b985eea96403e026568c57/packages/browser/src/types.ts#L255 like would be better
so we have GZIP and NONE as enum, and we can add more options later (capture v1 has more options)
the retry after compressed failed is called 'best-available' in the web which is the default
There was a problem hiding this comment.
I believe that's internal, and the public api on web is still https://github.com/PostHog/posthog-js/blob/bb884eba4edd27633dbf1faeb4b326fb1041c63e/packages/types/src/posthog-config.ts#L2250
The enum idea though I like cause it gives us room to grow. I'll go with something like public enum class PostHogCompression { GZIP, NONE } as you suggeted
dustinbyrne
left a comment
dustinbyrne
left a comment
There was a problem hiding this comment.
The earlier concurrency and transient-probe fixes are present. A flags retry-contract conflict remains (inline). Separately, maintainers still need to agree the API shape and automatic-recovery scope; the reported managed-network cause has not been independently reproduced.
AI-assisted review: source inspection and existing CI; no new tests executed.
| // server could not read. | ||
| val uncompressedResponse = | ||
| try { | ||
| chain.proceed(originalRequest) |
There was a problem hiding this comment.
Important: flagsClient inherits this interceptor, so a gzip-decode HTTP 400 sends another flags request here even with featureFlagRequestMaxRetries=0. The canonical flags policy permits status retries only for 502/504 and forbids another request for the same evaluation on other statuses. retryOnConnectionFailure(false) cannot prevent this explicit second proceed. Consider preserving that endpoint policy, or agreeing a compression-negotiation exception, and testing through reloadFeatureFlags with retries disabled. The current flags-message fixture calls batch, so it misses this interaction. This is a source-derived failure, not an executed reproduction.
There was a problem hiding this comment.
Fixed in 0b3e551. The flags request now carries a NoUncompressedRetry tag and the interceptor hands the 400 straight back for it, so executeFlagsWithRetry stays the only thing deciding whether a flags request goes out again.
Flags still recover once a batch or replay request has turned compression off, since they share the interceptor. Added a test through flags() with featureFlagRequestMaxRetries = 0 that asserts a single request, and one for flags going out uncompressed after a batch turned compression off.
| * | ||
| * The SDK recovers on its own in one case only: the server answers `400` with a body that | ||
| * reports it could not read the gzipped payload, and the same body then succeeds uncompressed. | ||
| * Compression stays off for the rest of the process. Any other rejection keeps compression on, |
There was a problem hiding this comment.
Minor: This state belongs to an interceptor instance, not the process. A second SDK-built client in the same process starts with compression enabled and can probe independently. Consider saying “for this SDK/client instance” here and in the PR description, rather than changing the implementation to global state.
There was a problem hiding this comment.
Updated in 2f668fc. The doc now says compression stays off for the rest of this SDK instance.
| @PostHogInternal | ||
| public class GzipRequestInterceptor(private val config: PostHogConfig) : Interceptor { | ||
| private companion object { | ||
| private const val HTTP_BAD_REQUEST = 400 |
| } | ||
| val body = | ||
| try { | ||
| response.peekBody(MAX_ERROR_BODY_BYTES).string().lowercase() |
There was a problem hiding this comment.
why do we need to limit here?
| private enum class CompressionState { | ||
| ON, | ||
|
|
||
| // One thread is finding out whether the server can read a compressed body. Bodies that were | ||
| // already compressed and rejected meanwhile go out again uncompressed, rather than failing. | ||
| PROBING, | ||
|
|
||
| // The server rejected the uncompressed body too, so compression is not the problem. The SDK | ||
| // keeps compressing instead of sending every rejected body twice. | ||
| KEEP, | ||
|
|
||
| // The server cannot read compressed bodies, e.g. because the network alters them in transit. | ||
| OFF, | ||
| } |
There was a problem hiding this comment.
this is cool but if it fails for /flags then it wont gzip for /capture
should this be per endpoint?
| // What the servers answer today when they cannot read a gzipped body: capture formats | ||
| // CaptureError::RequestDecodingError("invalid GZIP data"), feature flags wraps the same | ||
| // error in its own sentence. Pinned here so a wording change breaks a test instead of | ||
| // silently turning the uncompressed retry off. | ||
| private const val CAPTURE_DECODE_ERROR = "failed to decode request: invalid GZIP data" | ||
| private const val FLAGS_DECODE_ERROR = | ||
| "Failed to decode request: invalid gzip data. Please check your request format and try again." |
There was a problem hiding this comment.
this is very specific, and each server might respond with a different string
are we trying to solve this problem for posthog APIs or self hosted? can we reproduce broken gzip requests?
|
i like the idea of exposing |
Yeah that works for me. Once we have a clearer picture of the repro we can follow-up if needed. Will do that |
|
Dropped the retry in 9c167e1 iOS counterpart: PostHog/posthog-ios#851 so we can port the config to posthog-flutter |
marandaneto
left a comment
marandaneto
left a comment
There was a problem hiding this comment.
The code looks good; no qualifying findings. Verdict: correct.
3 participants
💡 Motivation and Context
/batchand/flagsanswer 400 and nothing gets through (inbox report). Apps have no way to opt out today, sincecontent-encodingis a reserved header.PostHogConfig.compression(PostHogCompression.GZIPby default, orNONE), an enum so more encodings can be added later.compressionin the Flutter and React Native wrappers. iOS counterpart to follow.💚 How did you test it?
PostHogApiTestcovers/batchand/flagsgoing out uncompressed withNONE. The existing gzip tests cover the default../gradlew :posthog:test --tests "com.posthog.internal.PostHogApiTest",make checkFormatandmake apipass.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
DRI: @ioannisj
Autonomy: Human-driven (agent-assisted)
Origin
1965fd1