Python: Fix ClaudeAgent reusing one SDK client across distinct fresh sessions - #7404
Python: Fix ClaudeAgent reusing one SDK client across distinct fresh sessions#7404giles17 wants to merge 2 commits into
Conversation
…sessions RawClaudeAgent kept a single mutable ClaudeSDKClient on the agent instance and reused it across distinct fresh AgentSession objects, because a fresh session passes session_id=None and the old reuse check treated that as "keep the current client". Two independent fresh sessions on one shared agent instance therefore shared a single provider conversation, so the second session continued the first session's conversation. Treat a fresh (None) continuation id as always requiring a new client, so an unbound session never inherits an existing provider conversation. Legitimate continuity is preserved: once a session runs, its service_session_id is written back, so later runs pass a real id and resume correctly. Guard client selection/creation with an asyncio.Lock so concurrent runs cannot race between the check and the client assignment. Add regression tests asserting two fresh sessions produce two clients and that an explicit continuation id still resumes the existing client. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 598a9fe1-28c5-4db1-88fd-e14acd9340af
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
🟡 Not ready to approve
The new lock does not prevent a concurrent run from disconnecting/replacing the SDK client while another stream is actively using it, and one added test currently validates a scenario via private state mutation that production code never performs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes a session-isolation bug in the Python Claude agent where a single stateful ClaudeSDKClient instance could be incorrectly reused across distinct fresh AgentSessions, causing provider conversation state to leak between sessions (notably in long-lived/hosted agent scenarios).
Changes:
- Update
RawClaudeAgent._ensure_session()to treatsession_id is None(fresh/unbound session) as always requiring a new client, preventing accidental cross-session reuse. - Add an
asyncio.Lockto serialize client selection/creation and reduce races during client replacement. - Add regression tests covering “fresh sessions must not share a client” and “explicit continuation id reuse” scenarios.
File summaries
| File | Description |
|---|---|
| python/packages/claude/agent_framework_claude/_agent.py | Adjusts client lifecycle rules for fresh vs. resumed sessions and adds a lock around client recreation logic. |
| python/packages/claude/tests/test_claude_agent.py | Adds tests to prevent regressions around fresh-session isolation and continuation-id behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Replace the single mutable ClaudeSDKClient stored on the agent with a per-run client. Because a ClaudeSDKClient represents exactly one provider conversation, sharing one across distinct sessions collapsed them onto the same conversation and, for concurrent runs, let a fresh session disconnect a client another run was still streaming from. _acquire_client now returns a per-run client (owned) that resumes the framework session's provider conversation when one exists, and _get_stream releases it in a finally once the run completes. An injected client is reused verbatim and left to the caller. The streaming loop moves into _stream_run so the client is a local per-run value rather than shared agent state, which keeps distinct sessions isolated even under concurrency. Continuity is preserved: a session's service_session_id is written back after each run and forwarded as the resume id on subsequent runs. Replace the client-lifecycle tests with per-run ownership and end-to-end isolation tests (two fresh sessions get two separate clients, each disconnected). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 598a9fe1-28c5-4db1-88fd-e14acd9340af
There was a problem hiding this comment.
🟡 Not ready to approve
The implementation materially diverges from the PR description (removes _ensure_session/reuse path and adds no lock), and the injected-client connect path needs synchronization to avoid concurrent connect() races.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (4)
python/packages/claude/agent_framework_claude/_agent.py:438
start()checks_startedwithout synchronization; if concurrent tasks callstart()(orstart()overlaps with_acquire_client()), the injected client can be connected multiple times. Wrap the_startedcheck +connect()in the instance lock.
if self._client is not None and not self._owns_client and not self._started:
try:
await self._client.connect()
self._started = True
except Exception as ex:
python/packages/claude/agent_framework_claude/_agent.py:487
_acquire_client()has the same unsynchronized_started/connect()path for injected clients; concurrent calls can race and attempt multiple connects. Use the same instance lock here to ensureconnect()is awaited at most once.
if not self._started:
try:
await self._client.connect()
self._started = True
except Exception as ex:
python/packages/claude/agent_framework_claude/_agent.py:459
- The PR description/linked issue describe adjusting
_ensure_session()to only reuse a started client when a non-None continuation id matches, plus guarding selection with anasyncio.Lock. The implementation instead removes_ensure_session()and always creates a fresh SDK client per run (except for injected clients), and there is currently no lock at all. If the per-run-client redesign is intended, the PR description (and issue’s proposed fix narrative) should be updated; otherwise, the code likely needs to reintroduce reuse-by-continuation-id logic with locking as described.
async def _acquire_client(self, resume_session_id: str | None = None) -> tuple[ClaudeSDKClient, bool]:
"""Acquire a Claude SDK client for a single run.
A ``ClaudeSDKClient`` is stateful and represents exactly one provider
conversation, so obtaining it is an isolation decision, not just a
python/packages/claude/tests/test_claude_agent.py:606
- The injected-client reuse test is sequential, so it won’t catch the real race where two concurrent runs both attempt to connect the injected client. Consider making this test run the two
_acquire_client()calls concurrently (e.g., viaasyncio.gather) so it fails without the lock and prevents regressions.
client_a, owns_a = await agent._acquire_client(None) # type: ignore[reportPrivateUsage]
client_b, owns_b = await agent._acquire_client("session-123") # type: ignore[reportPrivateUsage]
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| self._default_options = opts | ||
| self._started = False | ||
| self._current_session_id: str | None = None | ||
| self._structured_output: Any = None |
Motivation & Context
RawClaudeAgentkeeps a single mutableClaudeSDKClienton the agent instance. AClaudeSDKClientis stateful and represents one provider conversation, so decidingwhether to reuse it is an isolation decision, not just a connection optimization.
When one long-lived
ClaudeAgentinstance is shared across multiple logical sessions(for example, a single hosted agent serving many sessions), the client was reused across
distinct fresh
AgentSessionobjects. A fresh session resolves its provider continuationid to
None, and the previous reuse check treated aNoneid as "keep the currentclient". As a result, two independent fresh sessions ran against the same provider
conversation, and the second session continued the first session's conversation instead of
starting its own. This is a session-continuity/isolation bug in the client lifecycle logic.
Description & Review Guide
What are the major changes?
RawClaudeAgent._ensure_session()now treats a fresh/unbound session (session_id is None) as always requiring a new client, so an unbound session never inherits anexisting client's provider conversation. The reuse decision is otherwise unchanged: a
started client is reused only when an explicit continuation id matches the currently
connected one.
asyncio.Lockso concurrent runs cannotrace between the reuse check and the client assignment.
and an explicit continuation id still resumes the existing client.
What is the impact of these changes?
conversation, so conversation state no longer leaks between independent sessions.
service_session_idiswritten back, so subsequent runs pass a real (truthy) id and correctly resume the same
conversation via
resume=. Only genuinely fresh sessions get an isolated new client.What do you want reviewers to focus on?
_ensure_session()and that resume-by-continuation-id continuityis retained for sessions that have already been bound to a provider conversation.
Related Issue
Fixes #7403
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.