Skip to content

Commit eb7bc4e

Browse files
author
Benke Qu
committed
refactor: use child session copy instead of in-place mutation
Address Copilot review comments: - Create a child AgentSession with shared state dict but isolated service_session_id, avoiding race conditions under concurrent asyncio.gather tool invocations. - Update tests to verify child gets a separate session object and that child-set service_session_id does not leak to parent.
1 parent 59381ba commit eb7bc4e

2 files changed

Lines changed: 35 additions & 32 deletions

File tree

‎python/packages/core/agent_framework/_agents.py‎

Lines changed: 23 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -550,31 +550,29 @@ async def _agent_wrapper(ctx: FunctionInvocationContext, **kwargs: Any) -> str:
550550
"""
551551
session = ctx.session if propagate_session else None
552552

553-
# Isolate the child agent from the parent's server-side conversation.
554-
# service_session_id would cause the child to send previous_response_id
555-
# referencing the parent's pending tool_call, resulting in a 400 error.
556-
saved_service_session_id = None
557-
if session is not None and session.service_session_id is not None:
558-
saved_service_session_id = session.service_session_id
559-
session.service_session_id = None
560-
561-
try:
562-
stream = self.run(
563-
str(kwargs.get(arg_name, "")),
564-
stream=True,
565-
session=session,
566-
function_invocation_kwargs=dict(ctx.kwargs),
567-
)
568-
if stream_callback is not None:
569-
stream.with_transform_hook(stream_callback)
570-
final_response = await stream.get_final_response()
571-
if final_response.user_input_requests:
572-
raise UserInputRequiredException(contents=final_response.user_input_requests)
573-
# TODO(Copilot): update once #4331 merges
574-
return final_response.text
575-
finally:
576-
if session is not None and saved_service_session_id is not None:
577-
session.service_session_id = saved_service_session_id
553+
# Create a child session that shares the parent's state dict but has
554+
# an isolated service_session_id. This avoids mutating the parent
555+
# session in-place, which would race under concurrent asyncio.gather
556+
# tool invocations sharing the same session.
557+
if session is not None:
558+
child_session = AgentSession(session_id=session.session_id)
559+
child_session.state = session.state # shared by reference
560+
child_session.service_session_id = None
561+
session = child_session
562+
563+
stream = self.run(
564+
str(kwargs.get(arg_name, "")),
565+
stream=True,
566+
session=session,
567+
function_invocation_kwargs=dict(ctx.kwargs),
568+
)
569+
if stream_callback is not None:
570+
stream.with_transform_hook(stream_callback)
571+
final_response = await stream.get_final_response()
572+
if final_response.user_input_requests:
573+
raise UserInputRequiredException(contents=final_response.user_input_requests)
574+
# TODO(Copilot): update once #4331 merges
575+
return final_response.text
578576

579577
return FunctionTool(
580578
name=tool_name,

‎python/packages/core/tests/core/test_agents.py‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1010,7 +1010,7 @@ def capturing_run(*args: Any, **kwargs: Any) -> Any:
10101010

10111011

10121012
async def test_chat_agent_as_tool_propagate_session_clears_service_session_id(client: SupportsChatGetResponse) -> None:
1013-
"""Test that propagate_session=True clears service_session_id for the child and restores it after."""
1013+
"""Test that propagate_session=True gives the child a separate session with cleared service_session_id."""
10141014
agent = Agent(client=client, name="SubAgent", description="Sub agent")
10151015
tool = agent.as_tool(propagate_session=True)
10161016

@@ -1024,9 +1024,11 @@ async def test_chat_agent_as_tool_propagate_session_clears_service_session_id(cl
10241024
def capturing_run(*args: Any, **kwargs: Any) -> Any:
10251025
nonlocal captured_session
10261026
captured_session = kwargs.get("session")
1027-
# The child should see the same session object but with service_session_id cleared
1028-
assert captured_session is parent_session
1027+
# The child gets a different session object with isolated service_session_id
1028+
assert captured_session is not parent_session
10291029
assert captured_session.service_session_id is None
1030+
# But shares the same state dict by reference
1031+
assert captured_session.state is parent_session.state
10301032
assert captured_session.state["data"] == "shared"
10311033
return original_run(*args, **kwargs)
10321034

@@ -1040,14 +1042,14 @@ def capturing_run(*args: Any, **kwargs: Any) -> Any:
10401042
)
10411043
)
10421044

1043-
# After the child finishes, service_session_id is restored
1045+
# Parent's service_session_id is never mutated
10441046
assert parent_session.service_session_id == "resp_parent_abc123"
10451047

10461048

10471049
async def test_chat_agent_as_tool_propagate_session_restores_service_session_id_on_error(
10481050
client: SupportsChatGetResponse,
10491051
) -> None:
1050-
"""Test that service_session_id is restored even if the child agent raises."""
1052+
"""Test that parent's service_session_id is untouched even if the child agent raises."""
10511053
agent = Agent(client=client, name="SubAgent", description="Sub agent")
10521054
tool = agent.as_tool(propagate_session=True)
10531055

@@ -1068,12 +1070,12 @@ def failing_run(*args: Any, **kwargs: Any) -> Any:
10681070
)
10691071
)
10701072

1071-
# service_session_id must be restored even after failure
1073+
# Parent's service_session_id is never mutated — child has its own session
10721074
assert parent_session.service_session_id == "resp_parent_xyz789"
10731075

10741076

10751077
async def test_chat_agent_as_tool_propagate_session_no_service_session_id(client: SupportsChatGetResponse) -> None:
1076-
"""Test that when service_session_id is None, no save/restore is needed."""
1078+
"""Test that child setting service_session_id does not leak back to the parent."""
10771079
agent = Agent(client=client, name="SubAgent", description="Sub agent")
10781080
tool = agent.as_tool(propagate_session=True)
10791081

@@ -1087,6 +1089,8 @@ def capturing_run(*args: Any, **kwargs: Any) -> Any:
10871089
nonlocal captured_session
10881090
captured_session = kwargs.get("session")
10891091
assert captured_session.service_session_id is None
1092+
# Simulate the child's run populating service_session_id
1093+
captured_session.service_session_id = "resp_child_leaked"
10901094
return original_run(*args, **kwargs)
10911095

10921096
agent.run = capturing_run # type: ignore[assignment, method-assign]
@@ -1099,6 +1103,7 @@ def capturing_run(*args: Any, **kwargs: Any) -> Any:
10991103
)
11001104
)
11011105

1106+
# The child's service_session_id must not leak back to the parent
11021107
assert parent_session.service_session_id is None
11031108

11041109

0 commit comments

Comments
 (0)