Skip to content

fix(sdk-logs): avoid null _currentExport access in _flushAll#6763

Merged
pichlermarc merged 1 commit into
open-telemetry:mainfrom
Janealter:fix/sdk-logs-flushAll-race
Jun 3, 2026
Merged

fix(sdk-logs): avoid null _currentExport access in _flushAll#6763
pichlermarc merged 1 commit into
open-telemetry:mainfrom
Janealter:fix/sdk-logs-flushAll-race

Conversation

@Janealter

Copy link
Copy Markdown

Which problem is this PR solving?

BatchLogRecordProcessor._flushAll can throw TypeError: Cannot read properties of null (reading 'exportCompleted') when a forceFlush() call
races against the natural completion of a background export.

Reproduction

  1. The processor has a background export in flight (started by the
    scheduled-delay timer or because a batch filled up). _exportOneBatch
    set this._currentExport = exportOp and attached
    exportOp.exportCompleted.then(() => { this._currentExport = null; … })
    as a fire-and-forget completion handler.
  2. forceFlush() is called and enters _flushAll(). The check
    if (this._currentExport !== null) passes, and the code awaits
    this._exporter.forceFlush().
  3. While that await is pending, the in-flight export completes and the
    .then handler above runs, setting this._currentExport = null.
  4. _flushAll() resumes and evaluates
    await this._currentExport.exportCompleted. this._currentExport is now
    null, throwing TypeError.

In browsers this surfaces as an unhandled rejection because the browser
BatchLogRecordProcessor registers visibilitychange/pagehide listeners
that call void this.forceFlush() (no .catch() attached). Users
tab-switching during normal usage trigger the race very frequently in
production.

The same race is reachable from _shutdown() (which also calls
_flushAll()).

Affected versions

Introduced by #6356 (feat(sdk-logs)!: invoke exporter forceFlush without first awaiting export), shipped in @opentelemetry/sdk-logs@0.215.0. The
same code path is unchanged on main (verified against 0.215.0 through
0.218.0 and HEAD).

Short description of the changes

  • Capture this._currentExport into a local inFlight const before the
    awaits, then dereference the local. The trailing
    this._currentExport = null is kept so the field still reflects "no
    in-flight" once the wait is done (idempotent if the completion handler
    already nulled it).
  • Added a unit test in test/common/export/BatchLogRecordProcessor.test.ts
    (should not throw when in-flight export completes between awaits in _flushAll) that deterministically reproduces the race using a custom
    exporter with controllable export and forceFlush resolutions.

Without the fix, the new test fails with
TypeError: Cannot read properties of null (reading 'exportCompleted')
at the exact line affected.

Type of change

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

How Has This Been Tested?

  • New unit test added that deterministically interleaves a forceFlush()
    call with the completion of a background export. Test fails on main
    with the exact upstream error, passes with the fix.
  • All existing @opentelemetry/sdk-logs common tests still pass
    (nx run @opentelemetry/sdk-logs:test — 198 passing).
  • nx run @opentelemetry/sdk-logs:lint passes.

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • CHANGELOG entry added under experimental/CHANGELOG.md → Unreleased →
    Bug Fixes

@Janealter
Janealter requested a review from a team as a code owner May 26, 2026 16:46
@linux-foundation-easycla

linux-foundation-easycla Bot commented May 26, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: Janealter / name: Janealter (6759b88)

Captures `this._currentExport` into a local before the awaits inside
`_flushAll`. The previous code dereferenced `this._currentExport` after
`await this._exporter.forceFlush()`, but the in-flight export may have
completed during that await and `_exportOneBatch`'s completion handler
nullifies `this._currentExport`, causing:

    TypeError: Cannot read properties of null (reading 'exportCompleted')

In browsers this surfaces as an unhandled rejection because the
auto-flush listeners installed by the browser `BatchLogRecordProcessor`
invoke `void this.forceFlush()` on `visibilitychange` and `pagehide`.

Adds a unit test reproducing the race.

Signed-off-by: Janealter <24541204+Janealter@users.noreply.github.com>
@Janealter
Janealter force-pushed the fix/sdk-logs-flushAll-race branch from cde1101 to 6759b88 Compare May 26, 2026 16:46

@pichlermarc pichlermarc left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good catch!
thank you for fixing this. 🙌

@pichlermarc
pichlermarc added this pull request to the merge queue Jun 3, 2026
@codecov

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.92%. Comparing base (8edcb66) to head (6759b88).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6763   +/-   ##
=======================================
  Coverage   94.92%   94.92%           
=======================================
  Files         377      377           
  Lines       12818    12819    +1     
  Branches     2924     2924           
=======================================
+ Hits        12168    12169    +1     
  Misses        650      650           
Files with missing lines Coverage Δ
...sdk-logs/src/export/BatchLogRecordProcessorBase.ts 91.21% <100.00%> (+0.05%) ⬆️
🚀 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.

Merged via the queue into open-telemetry:main with commit 8e4d4ed Jun 3, 2026
31 checks passed
@otelbot

otelbot Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution @Janealter! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey.

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.

2 participants