Fix execution_timeout not interrupting a hung virtualenv subprocess - #71345
Open
roshanprabu wants to merge 1 commit into
Open
Fix execution_timeout not interrupting a hung virtualenv subprocess#71345roshanprabu wants to merge 1 commit into
roshanprabu wants to merge 1 commit into
Conversation
_execute_in_subprocess used `with subprocess.Popen(...) as proc:` and relied on the timeout signal handler's exception propagating out of the block. But Popen's __exit__ only closes the pipes and calls proc.wait() again on the way out -- it does not kill the child. If the child is still running (blocked on something long, like an open network connection inside a @task.virtualenv/@task.external_python callable), that wait() blocks until the child exits on its own, silently absorbing the timeout for however long that takes -- observed as the task instance staying in "running" state indefinitely despite the timeout being logged. Verified this precisely with a standalone repro (SIGALRM firing at 1s, child sleeping 8s): the exception was only actually caught after the full 8s, not the 1s the alarm was set for. Fix: catch any exception around the read/wait, explicitly proc.kill() and proc.wait() to reap it, then re-raise -- so the timeout is honored promptly and the child doesn't linger as an orphan holding resources. Closes: apache#57712
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #57712.
@task.virtualenv/@task.external_pythonrun the user's callable in a subprocess via_execute_in_subprocess(providers/standard/src/airflow/providers/standard/utils/python_virtualenv.py):execution_timeoutis enforced byTimeoutPosix(task-sdk/.../timeout.py), which sets aSIGALRMthat raisesAirflowTaskTimeoutfrom inside whatever's currently running -- here, that's the blockingreadline()/wait()call above.The bug:
subprocess.Popen.__exit__only closes the pipes and callsproc.wait()again on its way out -- it does not kill the child. If the child is still running when the exception fires (e.g. blocked on an open HTTP connection, per the linked issue's repro), that secondwait()blocks until the child exits on its own, silently absorbing the timeout for however long that takes. This matches the reported symptom exactly: "shows in the logs as timeout, but the task keeps in running state" -- the log line comes from the signal handler firing correctly, but the task doesn't actually finish timing out until the child process happens to exit.I verified this precisely with a standalone repro before touching any code (
with subprocess.Popen(...),SIGALRMat 1s, childsleep(8)): the exception was only actually caught after the full 8 seconds, not the 1 second the alarm was set for.Fix
Catch the exception around the read/wait, explicitly
proc.kill()+proc.wait()to reap the child, then re-raise:Test plan
_execute_in_subprocess: caught the timeout exception only after the full child runtime (8s), not the 1s alarm.subprocess.Popenspy that the child process is actually dead (not orphaned) ~0.5s after.TestExecuteInSubprocesswith 4 tests: prompt interruption timing, no orphaned child process, happy-path success, and non-zero exit still raisesCalledProcessErroras before.32.31svs the asserted< 10s) with the exact same log line from the report ("Executing cmd... Output:" followed by the full sleep duration before the assertion fires); re-applied the fix and confirmed it passes.pytest tests/unit/standard/utils/test_python_virtualenv.py-- 23 passed.ruff checkandruff format --checkpass on both changed files.