Skip to content

Conversation

artemziborev
Copy link
Contributor

Description

Fix FastAPI instrumentation memory leak by tracking instrumented apps with a WeakSet instead of a strong-referenced set. This avoids retaining strong references to fastapi.FastAPI instances, allowing them to be garbage-collected even if uninstrument_app() isn’t called. Also aligns with the approach used in the Starlette instrumentation.

Scope minimized to FastAPI only:

  • Use WeakSet for _instrumented_fastapi_apps and discard() on removal
  • Add a GC-based test that verifies the app is collected without calling uninstrument_app()
  • Remove the separate memory leak test file and integrate the test into test_fastapi_instrumentation.py
  • Update CHANGELOG (Unreleased → Fixed)

Fixes # (issue)

Type of change

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

How Has This Been Tested?

  • Added unit test:
    • test_fastapi_app_is_collected_after_instrument in test_fastapi_instrumentation.py, which:
      • creates an app
      • instruments it
      • drops the strong reference and forces gc.collect()
      • asserts the weakref is cleared (no leak)
  • Existing FastAPI test suite continues to apply

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

@artemziborev artemziborev requested a review from a team as a code owner August 12, 2025 13:09
@artemziborev artemziborev requested a review from anuraaga August 12, 2025 13:57
@artemziborev artemziborev changed the title Fix/fastapi memory leak only fix: fastapi memory leak only Aug 13, 2025
@artemziborev
Copy link
Contributor Author

Hi @anuraaga, just a gentle reminder — could you take a look at this PR? All checks have passed 👍

@anuraaga
Copy link
Contributor

Hi @artemziborev - LGTM. I had already approved since was just a comment nit, but I'm not a maintainer so it's a grey check

@xrmx xrmx merged commit 5fa222f into open-telemetry:main Aug 23, 2025
632 checks passed
@github-project-automation github-project-automation bot moved this from Ready for review to Done in @xrmx's Python PR digest Aug 23, 2025
@pantuza
Copy link

pantuza commented Aug 28, 2025

Hi folks, thank you for solving this issue.
Do we have a release version that contains this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants