fix: graceful shutdown tracks and cancels in-flight tasks (#356, #357)#448
Conversation
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
santoshkumarradha
left a comment
There was a problem hiding this comment.
Implementation looks good, few things to fix before merge:
-
test_graceful_shutdown_allows_fast_tasks_to_finish— listed in PR description as tested but doesn't exist in the file. Need the happy-path test. -
Dead code — comment out the old
asyncio.create_taskblock inagent.py(~L1723-1728), thereturn await run_reasoner(), and thepytest.skipfallbacks in both shutdown tests. Just delete them, git has history. -
Stale TODOs — top 3 lines of the test file still flag these as source bugs. This PR fixes them, remove the TODOs.
-
_track_taskhas no direct test — tests manually stuff_in_flight_taskswhich skips the actual tracking path. Quick unit test that_track_taskadds/removes via done-callback would close that gap. -
_track_in_flight_taskclosure in agent.py untested — ifserver_handleris None it silently skips tracking. Worth a test to verify the wiring.
Core approach is solid, just cleanup + test gaps.
|
About |
The tests aren't in the PR, but the description lists them as tested — just update the description to match what's actually included and we're good on that point |
|
@AbirAbbas @santoshkumarradha Changes requested till point 4 are done and I have commited them you can check on them , but about 5th , can you elaborate what do we have to test ?
a test to check if its skipping the tracking when server_handler is None ? |
AbirAbbas
left a comment
There was a problem hiding this comment.
its okay we can skip number 5, its not necessary
Summary
Implemented graceful shutdown task lifecycle handling for Python SDK agent server:
AgentServer(_in_flight_tasks) and_track_task(...)helper so server-spawned tasks are retained and auto-removed when done.Agentrequest handler (both callback and direct execution paths)._graceful_shutdown(...)to:timeout_seconds,gather(..., return_exceptions=True)to flush cancellation,Testing
./scripts/test-all.shpytest sdk/python/tests/test_agent_graceful_shutdown.py::test_graceful_shutdown_cancels_in_flight_tasks_within_deadline -vpytest sdk/python/tests/test_agent_graceful_shutdown.py::test_graceful_shutdown_force_cancels_tasks_after_timeout -vChecklist
CHANGELOG.md(or this change does not warrant a changelog entry).Screenshots (if UI-related)
N/A (backend/test-only change)
Related issues