fix(core): consume fetch response body to prevent CF Workers warnings#3508
fix(core): consume fetch response body to prevent CF Workers warnings#3508KuaaMU wants to merge 5 commits intoPostHog:mainfrom
Conversation
Consume the response body returned by fetchWithRetry() in both _flush() and sendImmediate() by calling response.body?.cancel(). This prevents cross-request promise resolution warnings in runtimes like Cloudflare Workers that enforce response body consumption. The warning occurs because an unconsumed response body forces the runtime to clean up the underlying connection later, potentially in a different request's context, which triggers warnings and cancels continuations. Fixes #3173
|
@KuaaMU-a is attempting to deploy a commit to the PostHog Team on Vercel. A member of the Team first needs to authorize it. |
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/core/src/posthog-core-stateless.ts:978-982
**`body` not declared on `PostHogFetchResponse`**
`PostHogFetchResponse` in `types.ts` only declares `status`, `text`, `json`, and `headers` — there is no `body` property. Accessing `response.body` is a TypeScript error (TS2339) that will fail type-checking. The same issue is on line 1157–1161.
The type needs to be extended:
```ts
// packages/core/src/types.ts
export type PostHogFetchResponse = {
status: number
text: () => Promise<string>
json: () => Promise<any>
headers?: {
get(name: string): string | null
}
body?: ReadableStream<Uint8Array> | null
}
```
### Issue 2 of 3
packages/core/src/posthog-core-stateless.ts:982
**Broken optional-chain: `.catch()` called on potentially-`undefined`**
`response.body?.cancel()` evaluates to `undefined` when `body` is `null` (e.g. a 204 No Content response). Calling `.catch(() => {})` directly on that `undefined` throws `TypeError: Cannot read properties of undefined (reading 'catch')`, which propagates to the surrounding `catch (err)` block and emits a spurious error event after an otherwise-successful flush. The same issue is on line 1161.
The optional-chaining must extend through the `.catch()` call:
```suggestion
await response.body?.cancel()?.catch(() => {})
```
### Issue 3 of 3
packages/core/src/posthog-core-stateless.ts:1161
**Same broken optional-chain as line 982**
Same issue: if `response.body` is `null`, `.catch(() => {})` is called on `undefined` and throws a `TypeError` inside what is intended to be a silent error path.
```suggestion
await response.body?.cancel()?.catch(() => {})
```
Reviews (1): Last reviewed commit: "fix(core): consume fetch response body t..." | Re-trigger Greptile |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12a663990e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Consume the response body to prevent cross-request promise warnings | ||
| // in runtimes like Cloudflare Workers that enforce body consumption. | ||
| // See: https://github.com/PostHog/posthog-js/issues/3173 | ||
| await response.body?.cancel().catch(() => {}) |
There was a problem hiding this comment.
response.body?.cancel().catch(() => {}) still throws when a custom fetch implementation returns a non-standard body (e.g. stream-like object without cancel, or cancel returning void), because only body is optional-chained. In that case sendImmediate/_flush treat this as a transport error even after a successful POST; in _flush this falls into the non-network-error path and removes queued events before rethrowing, so users see failures unrelated to delivery. Please guard both method existence and promise-ness before calling .catch.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Consume the response body to prevent cross-request promise warnings | ||
| // in runtimes like Cloudflare Workers that enforce body consumption. | ||
| // See: https://github.com/PostHog/posthog-js/issues/3173 | ||
| await response.body?.cancel().catch(() => {}) |
There was a problem hiding this comment.
.catch() called on potentially-undefined
response.body?.cancel() evaluates to undefined when body is null (e.g. a 204 No Content response). Calling .catch(() => {}) directly on that undefined throws TypeError: Cannot read properties of undefined (reading 'catch'), which propagates to the surrounding catch (err) block and emits a spurious error event after an otherwise-successful flush. The same issue is on line 1161.
The optional-chaining must extend through the .catch() call:
| await response.body?.cancel().catch(() => {}) | |
| await response.body?.cancel()?.catch(() => {}) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/posthog-core-stateless.ts
Line: 982
Comment:
**Broken optional-chain: `.catch()` called on potentially-`undefined`**
`response.body?.cancel()` evaluates to `undefined` when `body` is `null` (e.g. a 204 No Content response). Calling `.catch(() => {})` directly on that `undefined` throws `TypeError: Cannot read properties of undefined (reading 'catch')`, which propagates to the surrounding `catch (err)` block and emits a spurious error event after an otherwise-successful flush. The same issue is on line 1161.
The optional-chaining must extend through the `.catch()` call:
```suggestion
await response.body?.cancel()?.catch(() => {})
```
How can I resolve this? If you propose a fix, please make it concise.| // Consume the response body to prevent cross-request promise warnings | ||
| // in runtimes like Cloudflare Workers that enforce body consumption. | ||
| // See: https://github.com/PostHog/posthog-js/issues/3173 | ||
| await response.body?.cancel().catch(() => {}) |
There was a problem hiding this comment.
Same issue: if response.body is null, .catch(() => {}) is called on undefined and throws a TypeError inside what is intended to be a silent error path.
| await response.body?.cancel().catch(() => {}) | |
| await response.body?.cancel()?.catch(() => {}) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/posthog-core-stateless.ts
Line: 1161
Comment:
**Same broken optional-chain as line 982**
Same issue: if `response.body` is `null`, `.catch(() => {})` is called on `undefined` and throws a `TypeError` inside what is intended to be a silent error path.
```suggestion
await response.body?.cancel()?.catch(() => {})
```
How can I resolve this? If you propose a fix, please make it concise.- Add body property to PostHogFetchResponse type - Fix broken optional-chain: response.body?.cancel().catch() → response.body?.cancel()?.catch() The .catch() was called on undefined when body is null (e.g. 204 No Content), causing a TypeError inside the silent error path.
|
Thanks for the review @greptile-apps! Fixed all 3 issues:
The was called on |
|
The fixes look correct. The chain
The type addition All three fixes are solid. LGTM. Tip: You can customize Greptile's behavior for this repo with |
turnipdabeets
left a comment
turnipdabeets
left a comment
There was a problem hiding this comment.
LGTM, might be good to add a unit test asserting cancel() is invoked
|
Thanks for the review @greptile-apps @chatgpt-codex-connector! You're right about the optional chain issue. When The fix: move the // Before (broken):
await response.body?.cancel()?.catch(() => {})
// After (correct):
await response.body?.cancel()?.catch(() => {})Wait — looking more carefully, the current code is Actually, I see the diff shows |
|
To use Codex here, create an environment for this repo. |
|
The current code in the diff is actually correct as-is: await response.body?.cancel()?.catch(() => {})The chain works properly in both cases:
The |
|
closing by #3516 |
Pull request was closed
|
closing because commits arent signed, i couldnt force push, decided to open a new PR instead |
4 participants
Summary
Consume the response body returned by
fetchWithRetry()in both_flush()andsendImmediate()by callingresponse.body?.cancel(). This prevents cross-request promise resolution warnings in runtimes like Cloudflare Workers that enforce response body consumption.Fixes #3173
Problem
In
packages/core/src/posthog-core-stateless.ts, both_flush()(line ~1153) andsendImmediate()(line ~978) callfetchWithRetry()but discard the returnedResponsewithout reading the body:In Cloudflare Workers, an unconsumed response body forces the runtime to clean up the underlying connection later, potentially in a different request's context, which triggers:
This is the same class of bug the Sentry SDK had and fixed in getsentry/sentry-javascript#18534 / PR #18545.
Fix
Consume or cancel the response body after
fetchWithRetry()returns:Uses
response.body?.cancel()(Option B from the issue) which is the most lightweight approach — it tells the runtime to discard the body without reading it into memory.Locations fixed
_flush()— the main batch flush path called on interval/queue fullsendImmediate()— the synchronous send path used whenflushAt: 1Why not the other call sites?
The other
fetchWithRetry()callers (remote config, feature flags, surveys) all consume the response via.json(), so they don't have this issue.Testing