Skip to content

fix(otlp-exporter-base): drain the fetch response body so that browsers release the keepalive quota - #7002

Open
anneheartrecord wants to merge 3 commits into
open-telemetry:mainfrom
anneheartrecord:fix/7001-drain-fetch-response-body
Open

fix(otlp-exporter-base): drain the fetch response body so that browsers release the keepalive quota#7002
anneheartrecord wants to merge 3 commits into
open-telemetry:mainfrom
anneheartrecord:fix/7001-drain-fetch-response-body

Conversation

@anneheartrecord

Copy link
Copy Markdown
Contributor

Which problem is this PR solving?

Fixes #7001

The fetch transport never reads the response body. In Chromium that means the request's share of the 64KiB cumulative keepalive quota is never given back: HandleLoaderFinish — which decrements inflight_keepalive_bytes_ — is only reachable once the body has been read to its end. Blink normally hides this by draining the body itself through a BufferingBytesConsumer, but it explicitly skips that consumer when the response carries Cache-Control: no-store, which is a common header for a collector to return (fetch_manager.cc).

The result, reproduced in the issue, is that successful exports leak the quota until it is exhausted, after which every export stays (pending) in devtools forever and telemetry is silently lost. The transport's own accounting (pendingBodySize / pendingKeepaliveCount) meanwhile releases at response headers, so it keeps sending requests with keepalive: true that the browser can no longer accept.

Short description of the changes

  • Read the response body to its end and discard it before evaluating the status. The reader loop drops chunks as they arrive rather than calling response.arrayBuffer(), so a large response is not buffered in memory.
  • Read errors are logged at debug level and do not change the export outcome — the status already tells us what happened to the export.
  • Because the drain is awaited before the finally block, the transport now holds its in-flight counters for the lifetime of the body as well. That is intentional: it makes the internal accounting line up with when the browser actually releases the quota, instead of releasing ahead of it.

Cancelling the body instead of reading it would not work — in Blink that is the client-abort path, not the read-to-end path that runs HandleLoaderFinish.

The body is still discarded rather than returned as ExportResponseSuccess.data, so partial-success responses remain unhandled in the browser (otlp-export-delegate handles data when the node transport sets it). That is a separate gap and I kept it out of this fix, happy to follow up if you'd like it in the same PR.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • npm run test:browser in experimental/packages/otlp-exporter-base (90 passing). The three new tests fail on main and pass with the change.
  • npm test in the same package (151 passing), plus test:browser of exporter-trace-otlp-http, exporter-logs-otlp-http and opentelemetry-exporter-metrics-otlp-http.
  • npm run lint (no new findings).

New tests cover that the body is read to its end (and not cancelled) on a successful export, that it is read on a retryable one, and that a body which fails to read leaves the export result untouched.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

Chromium only releases the request's share of the 64KiB keepalive quota
once the response body has been read to its end, and it skips the
buffering consumer that would drain the body on its own when the
response carries Cache-Control: no-store. Since the transport never read
the body, the quota filled up and every later export stayed pending.

Read and discard the body before returning, chunk by chunk so that a
large response is not buffered. Read failures are logged at debug level
and do not change the export outcome.

Signed-off-by: Charles Cheng <chengxisheng777@gmail.com>
@anneheartrecord
anneheartrecord requested review from a team as code owners August 15, 2026 04:01
@opentelemetry-pr-dashboard

opentelemetry-pr-dashboard Bot commented Aug 15, 2026

Copy link
Copy Markdown

Pull request dashboard status

Waiting on reviewers · refreshed 2026-08-21 06:16 UTC

Review the latest changes.

Status above doesn't look right?
  • Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
  • Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

@codecov

codecov Bot commented Aug 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.02%. Comparing base (b2ffd97) to head (d808444).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...tlp-exporter-base/src/transport/fetch-transport.ts 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7002      +/-   ##
==========================================
- Coverage   95.02%   95.02%   -0.01%     
==========================================
  Files         409      409              
  Lines       14298    14317      +19     
  Branches     3277     3277              
==========================================
+ Hits        13587    13605      +18     
- Misses        711      712       +1     
Files with missing lines Coverage Δ
...tlp-exporter-base/src/transport/fetch-transport.ts 94.20% <92.30%> (-0.44%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…draining

Draining the body left its reader attached, so the stream stayed locked for
good. Acquiring a reader on a locked stream throws a `TypeError` without a
cause, which is exactly what `isFetchNetworkErrorRetryable` treats as a
retryable network error, so any later export handed the same response was
retried until it timed out.

Release the reader once the body has been read, and acquire it inside the
guarded block so a body locked elsewhere cannot decide the export outcome
either.

Signed-off-by: Charles Cheng <chengxisheng777@gmail.com>

@overbalance overbalance left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Direction and code look good. I want to reproduce #7001 locally against a no-store collector before approving, and could you add a test for the timeout abort firing mid-drain (never-closing body + short timeout) asserting the result still reflects the HTTP status?

The response body is drained while the export timeout is still running, so a
collector that holds the body open long enough gets the request aborted in the
middle of the read. Add cases for a 200 and a 503 response whose body only ends
when the abort fires, asserting the status the collector sent still decides the
export outcome.
@anneheartrecord

Copy link
Copy Markdown
Contributor Author

Added in d808444: a 200 and a 503 case whose response body delivers one chunk and then stays open until the request is aborted, at which point it errors the way a real fetch body does. Both assert the status the collector sent still decides the export outcome (success / retryable) and that the read failure only shows up as a debug log.

They do discriminate the change — with the catch in drainResponseBody rethrowing instead of swallowing, both fail (failure instead of success/retryable), along with the two existing locked-body / unreadable-body cases.

94/94 green in the browser suite locally.

JacksonWeber
JacksonWeber previously approved these changes Aug 17, 2026

@JacksonWeber JacksonWeber left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the small lint issue, this LGTM.

@anneheartrecord

Copy link
Copy Markdown
Contributor Author

Thanks for the review! The lint red isn't coming from this PR. The failing step is docs:test, where linkinator gets a [403] on https://cloud-native.slack.com/archives/C01NL1GRPQR in doc/upgrade-to-2.x.md — Slack rate-limits the crawler, so the same job flips on main too: run 32027505718 (8f103777) failed identically about half an hour before this one, while 32022287610 passed.

This branch only touches fetch-transport.ts, its browser test and the changelog entry, so a re-run of the lint job should be all it needs.

@therynamo

therynamo commented Aug 20, 2026

Copy link
Copy Markdown

Hey, just offering an opinion. This would work as a fix that makes sure the body is read, which is part of the problem for sure.

There are some considerations:

  1. This fix can prevent the problem so that is good!
  2. There are some other ideas for ways to offer some more flexibility for users. Maybe something like:

We could consider allowing users to set keepalive as a configuration option. As in, when initialization happens at a module level, pass { keepalive: false } or something similar. So if users don't particularly need to worry about the crash reporting or browser close aspects, they can disable this functionality.

One observation was the library has this protection where it currently reserves 60 out of 64kib of the keepalive budget. When I was hitting it before I hit that limit, it tipped me off to this being a keepalive problem. That 64kb, I found out, belongs to anyone in that respective fetch group (i.e. tab, sw, iframe, etc (fetch groups were new to me)). I got lucky in how I caught this bug, which surprisingly is that the budget was exceeded regardless of the internal byte count. But it was also Chromium where this was only happening, Webkit and Gecko did not have his problem. It was a fun experiment and I appreciated the time I got to spend debugging it.

This satisfies the conditions I think of the issue, and then relying on Chrome to fix the upstream issue if they find it to be a bug. That makes sense too!

If you want we could discuss some further options if you guys are interested in some contributions, I'd be happy to help!

I've filed an issue with Chromium that they're working through here, https://issues.chromium.org/issues/546438373, it would be really cool to collaborate on this with you!

(EDIT: Fixing up my tone, I should have taken a more collaborative approach. If you'll allow me to start over.)

@therynamo

therynamo commented Aug 20, 2026

Copy link
Copy Markdown

@JacksonWeber CC ^ (Updated above comment)

@JacksonWeber
JacksonWeber dismissed their stale review August 20, 2026 18:12

Approval withdrawn by reviewer.

@anneheartrecord

Copy link
Copy Markdown
Contributor Author

Thanks for the Chromium report — that's the half of this I couldn't get at from the library side, and it's good to have a tracking bug for it.

Your observation that the budget blew before your own byte count reached it lines up with this bug. The transport frees a request's share of the 60KiB it tracks in the finally after fetch() resolves, but Chromium only hands the quota back once the response body has been read to the end — and it skips the buffering consumer that would otherwise drain the body on its own when the response carries Cache-Control: no-store, which collectors commonly send. So the library sees the budget as free while the browser still counts those bodies as pending, and since the pool is per fetch group as you found, anything else in the tab is drawing on the same 64KiB. The ceiling arrives early and then never moves.

A keepalive opt-out sounds reasonable for people who don't need delivery at unload time, but it's a different change from this one: it adds public configuration surface (and a declarative-config counterpart) rather than fixing a leak, so it probably wants its own issue. Happy to help with it if it gets picked up.

Unrelated, for whoever ends up merging: the lint red is still linkinator getting a [403] on the archived Slack link in doc/upgrade-to-2.x.md. Attempt 3 hit it again at 16:25 UTC on the 19th, while the three lint runs on main since then (756f49ad, 640e11b5, e8d19272) all passed, so that URL 403s intermittently rather than consistently. Another re-run would likely clear it; if it keeps flapping, skipping cloud-native.slack.com in docs:test would be the durable fix, but that's a docs change and doesn't belong on this branch.

@anneheartrecord

Copy link
Copy Markdown
Contributor Author

Correction to my last paragraph — the main runs I cited don't show what I said they showed. The doc-link check was moved out of the lint workflow in #6920 (merged 19 Aug 17:03 UTC), so 756f49ad, 640e11b5 and e8d19272 never ran linkinator at all; they're green for that reason, not because the Slack URL started answering. This branch is 17 commits behind main, so its lint job is still running the old workflow definition, and attempt 3 started about 40 minutes before that merge. Sorry for the noise.

So it isn't a flake to re-run: bringing the branch up to date with main drops the step and the job goes green. I'll rebase and force-push — the diff itself doesn't change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(fetch-transport): Chromium (Blink) + OTel SDK + Keepalive = Continued Telemetry Data Loss

4 participants