fix(worker): evict cached workflows during shutdown - #1849
Conversation
|
Can you elaborate on what the user impact is here. What are you trying to prevent or accomplish with this change? |
|
The user impact is incomplete cleanup when a worker shuts down with running workflows—for example, during a deployment. In our worker-replacement test, the workflow resumes successfully on the replacement worker, but the stopped worker leaves pending workflow coroutines behind. Python later tries to finalize them through garbage collection. Cleanup that needs to await then raises This also reproduces without the Agents SDK: a workflow waits indefinitely and its The change uses the existing safe-eviction path to clean up cached workflow instances before shutting down the executor. Server-side workflows remain running and can resume on another worker. The added regression tests cover both default and caller-owned executors. |
|
@tconley1428 Adding two points my earlier answer was missing. What Core does today. sdk-core never evicts idle cached runs at shutdown. What the other SDK does. sdk-typescript already handles this: when the poller leaves User impact, concretely. Any deployment that stops a worker with open workflows leaves those workflows to be closed by GC. Cleanup that awaits in a Trade-off. Shutdown now waits for these evictions the same way it waits for Core-driven ones, so a workflow that swallows |
Problem
Core does not evict idle cached workflows when a worker shuts down. In sdk-core,
shutdown_done(workflow_stream.rs) only waits for runs thathas_any_pending_work: an outstanding WFT, activation, buffered task, or an eviction that was already requested. An idle cached run has none of those, so Core reports shutdown complete and the run stays in the cache without ever receiving aremove_from_cachejob.On the Python side those workflows are then left as suspended coroutines. Python later closes them during garbage collection, which throws
GeneratorExitinto the workflow on whatever thread or event loop happens to be running GC. Any cleanup that needs toawaitin afinallyraisesRuntimeError: coroutine ignored GeneratorExit, and any commands issued there can cross workflow contexts (the failure mode documented in #494).A workflow that waits on a condition and awaits
asyncio.sleep(0)infinallyreproduces this with no framework integration. The practical trigger is any deployment that stops a worker with open workflows; the workflows resume fine on the replacement worker, but the stopped worker fails its own teardown.Fix
After in-flight activations finish and before the workflow executor stops, the workflow worker builds a local eviction activation for every run still in
_running_workflowsand drives it through the existing safe-eviction path. The activation has the same shape as Core'screate_evict_activation: no timestamp,is_replayingfalse, a singleremove_from_cachejob. A newreport_to_corekeyword on_handle_cache_evictionskips the completion acknowledgment, since polling has stopped and Core never issued the job.This mirrors what sdk-typescript already does: when the poller leaves the
POLLINGstate it injects a synthetic eviction activation (SELF_INDUCED_SHUTDOWN_EVICTION) into every cached workflow and does not send its completion to Core. This PR brings the Python worker to parity.Evictions run concurrently, honor the explicit
disable_safe_workflow_evictionopt-out, do not cancel workflows on the server, and do not close a caller-owned executor. The reason isLANG_REQUESTED, which the replayer's eviction hook already treats as a non-failure.Behavior to be aware of
Shutdown now waits for these evictions the same way it waits for Core-driven ones. A workflow that swallows
BaseExceptionand keeps waiting will make eviction retry forever and hold shutdown open, exactly astest_workflow_eviction_swallowdocuments for the Core-driven case. Previously such a workflow was silently garbage-collected instead. The existing "Timed out running eviction job" log fires so the cause is visible.Tests
test_worker_shutdown_cleans_cached_workflowsstarts three cached workflows, shuts the worker down, and asserts eachfinallyran under eviction (is_replaying()is set to true on eviction, so the assertion proves the cleanup ran on the workflow's own loop rather than in GC). It is parametrized over the default and a caller-owned executor; both fail before the patch and pass after. The server-side workflows are asserted stillRUNNING.Validation
poe test -n 0 tests/worker/test_workflow.py -k 'shutdown_cleans_cached or cache_eviction_tear_down or workflow_eviction_exception or workflow_eviction_swallow'on Python 3.12.8: all selected tests pass.poe lintpasses (Ruff, Pyright, mypy, BasedPyright, docstyle).