Repository navigation
Python: release functional workflow stream on agent response close - #10220
袁永旗（Yuan Yongqi） (Yongthyuan) wants to merge 2 commits into
Conversation
GitHub Actions
Inactive
GitHub Actions
Waiting
agent-framework-automation
Bot
added
the
python
GitHub Actions
Inactive
|
@microsoft-github-policy-service agree |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (2 commit(s)): f61d1898842a, 85cfe2aa3ab7
Model: gpt-5.6-sol
Overview
The change correctly connects the outer agent response lifecycle to its owned workflow stream, and the one-shot cleanup hook plus new tests protect unstarted, consumed, failed, cancelled, and stale-close cases. However, the hook also exposes a close-versus-active-pull race that can release the workflow's run guard before the original execution terminates. That permits overlapping runs on an instance whose state and side effects are explicitly designed for single-run execution.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 high) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_workflows/_functional.py
| return ResponseStream( | ||
| _generate_updates(), | ||
| finalizer=AgentResponse.from_updates, | ||
| cleanup_hooks=[workflow_stream.close], |
There was a problem hiding this comment.
If close() races an active anext(), the outer generator's aclose() raises, but ResponseStream.close() still runs this hook; the inner close then releases _is_running even though the original workflow remains active. A subsequent run is accepted concurrently, violating the single-run invariant and risking duplicate side effects or checkpoint state corruption. Please coordinate close with in-flight pulls, or otherwise retain the guard until the inner workflow has actually terminated, and cover this race with a regression test.
|
袁永旗（Yuan Yongqi） (@Yongthyuan) Thanks for the fix. This implementation was already merged to main in #10204, which cherry-picked this PR's fix and closed #10164. The current diff against main is empty, so I'm closing this PR as superseded. |
|
Thanks for clarifying that the original fix has already been merged. The automated review on this PR also identified a separate issue with closing a stream while a read is still in progress. That issue is present on current main as well. I have a fix and regression tests ready, and I’ll submit them in a follow-up PR referencing #10164. |
Motivation & Context
Closing a streaming
FunctionalWorkflowAgentresponse leaves its owned workflow marked as running. The next sequentialagent.run()raisesRuntimeError: Workflow is already running, even afterawait response.close()returns. This affects cancellation or disconnect cleanup followed by another turn.Description & Review Guide
workflow_stream.closeas a cleanup hook on the outerResponseStream, using the existing cleanup lifecycle to close the owned stream and release the run guard.Related Issue
Fixes #10164.
This submits the original patch I shared on November 7. On November 9, a maintainer marked the issue
ready-for-implementationand assigned it to me. The branch preserves that original commit and incorporates the latestmain.AI Assistance
AI assistance details: implementation, regression tests, local validation, and this description were prepared with AI assistance. I personally ran the original minimal reproduction and observed the reported failure.
Contribution Checklist
ready-for-implementationby a maintainer.The two early-close regressions fail against the original implementation and pass with this fix. The original minimal reproduction now prints
result: 2. Ruff lint and format, strict source Pyright, and test Pyright, mypy, Pyrefly, ty, and zuban checks pass for the changed files using the repository's pinned versions. Pyrefly reports three warnings in unchanged tests; the workflow tests emit the existing experimental-feature warning.