Skip to content

fix: scope AsyncFastAPI HTTP client pool to instance instead of class#6882

Open
YizukiAme wants to merge 1 commit intochroma-core:mainfrom
YizukiAme:fix/async-instance-scoped-transport
Open

fix: scope AsyncFastAPI HTTP client pool to instance instead of class#6882
YizukiAme wants to merge 1 commit intochroma-core:mainfrom
YizukiAme:fix/async-instance-scoped-transport

Conversation

@YizukiAme
Copy link
Copy Markdown

Fixes #6869

Problem

AsyncFastAPI._clients is declared as a class variable, so all instances share the same Dict[int, httpx.AsyncClient] pool. When multiple AsyncFastAPI instances are created (e.g. in tests or multi-tenant setups), they cross-contaminate each other's HTTP transports. Closing one instance's client can corrupt another instance's connection pool.

Fix

Move _clients initialization from class-level to __init__, making it an instance variable:

-_clients: Dict[int, httpx.AsyncClient] = {}
-
 def __init__(self, system: System):
     super().__init__(system)
+    self._clients: Dict[int, httpx.AsyncClient] = {}

Tests

53-line test verifying that two AsyncFastAPI instances maintain independent client pools and that cleanup of one does not affect the other.

This fix was developed with AI assistance and reviewed by a human.

@github-actions
Copy link
Copy Markdown

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@propel-code-bot
Copy link
Copy Markdown
Contributor

Fix AsyncFastAPI client pool scoping to prevent cross-instance transport sharing

This PR fixes a bug where AsyncFastAPI used a class-level _clients dictionary, causing all instances to share the same event-loop-keyed httpx.AsyncClient pool. The change moves _clients initialization into AsyncFastAPI.__init__, making the pool instance-scoped and preventing one instance’s cleanup from closing another instance’s client.

A focused test is added to verify isolation between two AsyncFastAPI instances: each creates its own client, cleanup on one instance only closes its own client, and the second instance continues reusing its existing client until it is explicitly cleaned up.

This summary was automatically generated by @propel-code-bot

Copy link
Copy Markdown
Contributor

@propel-code-bot propel-code-bot bot left a comment

Choose a reason for hiding this comment

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

No issues were identified; the instance-scoped client pool fix and isolation test appear sound.

Status: No Issues Found | Risk: Low

Review Details

📁 2 files reviewed | 💬 0 comments

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]: Async HTTP clients in the same event loop silently share one transport and leak headers/TLS state

1 participant