Skip to content

Python: perf(foundry-hosting): cache FoundryStateStore in FoundryAgentSessionStore - #8281

Open
Harsheet Shah (harsheet-shah) wants to merge 5 commits into
microsoft:mainfrom
harsheet-shah:harsheetshah/maf-caching-improvements
Open

Python: perf(foundry-hosting): cache FoundryStateStore in FoundryAgentSessionStore#8281
Harsheet Shah (harsheet-shah) wants to merge 5 commits into
microsoft:mainfrom
harsheet-shah:harsheetshah/maf-caching-improvements

Conversation

@harsheet-shah

Copy link
Copy Markdown

Motivation & Context

FoundryAgentSessionStore (the default agent-session SessionStore for hosted MAF agents) rebuilds its backing FoundryStateStore on every get/set/delete. Each _get_store() call goes through FoundryStateStore.get_or_create("agent_sessions", user_isolation=True), which (1) constructs a fresh credential (empty token cache → a new managed-identity token fetch) and (2) issues an agent_sessions metadata round-trip (GET/POST state_stores) before the actual item operation. Because the hosting infra calls set() in the finally of every Responses request, this redundant credential + metadata work lands on the critical path of every request.

Description & Review Guide

  • What are the major changes?
    Cache one per-event-loop, per-scope FoundryStateStore for the agent-session scope. The cache is a WeakKeyDictionary keyed by the running event loop (a closed loop's store is GC'd, so a stale store is never reused on another loop), further keyed by scope (a subclass overriding DEFAULT_ROOT_SCOPE gets its own store instead of clobbering the base collection). Concurrent first-use is guarded by a per-loop lock with double-checked init. The shared store is intentionally not entered via async with (its __aexit__ would aclose() the pooled pipeline + credential and defeat the cache); it is opened once and kept open for the process lifetime.

  • What is the impact of these changes?
    Only the real item GET/PUT/DELETE remains on the hot path. Item get/set/delete semantics are byte-for-byte identical; get_or_create still creates-on-first-use exactly once. Checkpoint / function-approval stores are left untouched (not on the per-request hot path). No public API change.

    Measured (500 cold + 500 warm streaming req/agent, concurrency 50, private/VNet Foundry project, gpt-4o-mini, 0 errors): WARM TTLB p50 7,736 → 7,464 ms (−272 ms, −3.5%); WARM TTFB p50 flat (~5.68 s model first-token floor, unaffected). The win is elimination of the per-request credential + metadata round-trip.

  • What do you want reviewers to focus on?
    Event-loop scoping / GC of the cache, and the subclass-scope isolation.

Related Issue

Closes #8280

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change.

…Store

Reuse one process-wide FoundryStateStore for agent-session persistence instead of rebuilding it (new credential + agent_sessions metadata round-trip via get_or_create) on every get/set/delete. The session set() runs on the critical path of every Responses request, so the redundant work was pure per-request latency. Per-request user isolation is preserved via the per-operation call_id, so a shared store is equivalent; the store is no longer entered as an async-with context (its aclose() would defeat the cache) and is kept open for the process lifetime.
Add an autouse fixture that resets the new process-wide FoundryStateStore cache between tests so each agent-session test observes its own patched get_or_create, and add a test asserting the store is resolved once and reused across set/get/delete.
… loop and scope

Address review: the store owns a loop-bound async pipeline + credential, so a process-wide singleton could be reused from a different event loop (across asyncio.run() calls or loop-scoped tests). Cache the store in a WeakKeyDictionary keyed by the running loop (closed loops -> their stores are GC'd) and by scope, with a per-loop lock, so a subclass overriding DEFAULT_ROOT_SCOPE no longer shares or clobbers the base collection. The single-loop server still shares one store, preserving the latency win.
…d subclass-scope isolation

Reset the per-(loop, scope) cache between tests, and add tests asserting the backing store is resolved once under concurrent first-use and that a subclass overriding DEFAULT_ROOT_SCOPE gets its own cached store.

Copilot AI 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.

🟡 Changes recommended

Contended locks retain closed event loops, preventing cached stores and resources from being reclaimed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Caches Foundry agent-session state stores to remove repeated credential creation and metadata requests.

Changes:

  • Adds per-loop, per-scope caching with initialization locking.
  • Adds cache reuse, concurrency, and scope-isolation tests.
File summaries
File Description
_state_store.py Implements cached store reuse.
test_state_store.py Tests caching and initialization.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/foundry_hosting/agent_framework_foundry_hosting/_state_store.py Outdated
Comment thread python/packages/foundry_hosting/tests/test_state_store.py Outdated
…per-loop leak

Address PR review feedback on the FoundryAgentSessionStore cache:

- Store the per-loop FoundryStateStore cache (and its creation lock) on the
  running event loop itself instead of in process-global WeakKeyDictionaries.
  An asyncio.Lock (and the store's pooled pipeline/credential) strongly
  references its loop, so a module-global map keyed by the loop kept that
  "weak" key alive, leaking one cache + open pipeline/credential per closed
  loop (e.g. every asyncio.run). Anchored to the loop, the state is reclaimed
  with the loop. Falls back to an uncached resolve if a C-level loop forbids
  attribute assignment (no reuse, but no leak).
- Make the concurrent-init test's mocked get_or_create actually suspend
  (await asyncio.sleep(0)) so all gathered tasks contend on the creation lock;
  the previous non-suspending mock let the first task populate the cache
  synchronously, so even a lock-less implementation would have passed.
- Drop the now-unnecessary global cache-reset fixture: per-loop state is
  isolated automatically by the function-scoped event loop each test runs on.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3d900395-13d0-4698-bf9d-f6670f9e545c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: perf(foundry-hosting): FoundryAgentSessionStore rebuilds FoundryStateStore (credential + metadata round-trip) on every request

2 participants