Repository navigation
Enforce execution_timeout for tasks run by a language SDK - #74869
ChickenBenny wants to merge 12 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
henry3260
left a comment
There was a problem hiding this comment.
Thanks for the contribution! leave some comments
Doc nit: airflow-core/docs/core-concepts/tasks.rst:143 says the supervisor enforces the timeout "For tasks executed by the Python task runner". After this PR that is no longer true it also covers stub and native language-SDK tasks. Could we add a line saying so, or at least drop the "Python task runner" qualifier?
| return task.arg_bindings | ||
|
|
||
|
|
||
| def get_language_sdk_execution_timeout(dag_bag: DBDagBag, ti: Any, *, session: Session) -> float | None: |
There was a problem hiding this comment.
get_language_sdk_execution_timeout uses a falsy check, so a negative
execution_timeout passes through: timedelta(seconds=-1) is truthy, returns
-1.0, and _arm_execution_timeout lands the deadline at
monotonic() - 1 + KILLED_TASK_CLEANUP_TIME SIGTERM 59s later rather than at
once. bases/operator.py only type-checks the field, so this is reachable.
The Python task runner fast-fails instead (if timeout_seconds <= 0: raise AirflowTaskTimeout(), right before it sends SetExecutionTimeout), which is why
the supervisor never sees a non-positive value today.
-
Treat non-positive as no timeout, which keeps the fix inside the helper and
matches what the docstring already says about zero:if (task := get_stub_task(dag_bag, ti, session=session)) is None: return None timeout_seconds = task.execution_timeout.total_seconds() if task.execution_timeout else 0 if timeout_seconds <= 0: return None return timeout_seconds
There was a problem hiding this comment.
I fixed it in 658b814, but went a slightly different way from your suggestion: a negative value is clamped to 0.0 instead of returning None. This aligns with how the Python task runner handles it, where a negative timeout fails the task right away rather than disabling the timeout:
airflow/task-sdk/src/airflow/sdk/execution_time/task_runner.py
Lines 2318 to 2326 in f3784d2
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Would it be better to align the execution_timeout at the core instead of fixing at each SDK?
By this way, we could treat the execution_timeout pin down at the Dag processing time as source of truth rather than retrieving the default value at the task runtime?
Additionally, for the SetExecutionTimeout introduced in #73806: would it be better to rename as StartExecutionTimeout and remove the execution_timeout field? -- cc @Andrushika
From my perspective, no matter it is Python task or any language SDK task, the execution_timeout should be figured out at the Execution API like current PR shape, and the runtime of each SDK should respect it from the TIRunContext. The coordinator level should be able to emit the signal once instead of implementing the signal for each SDK.
|
Thanks Jason for taking a look! Quick update on where the PR is: killing a timed-out process is already handled in one place by the supervisor. The language SDK coordinators go through ActivitySubprocess too, so Go / TS / Java don't need any changes. The only thing that's still different is where the value comes from: Python still works it out itself and sends it to the supervisor. Here's roughly what I'm planning to change:
A couple of things I'd like to check first:
|
Agree that it will be cleaner -- task runners in all SDKs don't need to work out the timeout by themselves anymore. But we still need to handle users' custom cleanup logic after the timeout. To be more specific, make all lang SDKs signal-aware (like what #74690 is doing for Java), so that lang SDKs only need to listen to the signal and handle their own cleanup.
Hmmm I think a better solution could be making the supervisor only start the clock on the start message for Python tasks (the task runner sends it right before
Thanks @ChickenBenny, I think it's fine to handle that in this PR. Since Also, two more things I want to mention:
But these should be separate follow-up PRs. Just put it here as a reminder. I think we can first get the timeout working here and try to make it for 3.4. |
|
FYI @ChickenBenny, this graph helped me understand the architecture between coordinators, executors, and the task runner a lot when I was a beginner. I hope this can help you as well :) flowchart LR
subgraph SP["Scheduler OS process"]
subgraph LE["LocalExecutor object"]
AQ["activity_queue<br/>IPC"]
RQ["result_queue<br/>IPC"]
WH["workers<br/>Process handles"]
end
end
subgraph WP["Local worker OS process|可重用"]
LOOP["_run_worker()<br/>get loop"]
CM["CoordinatorManager.for_queue()<br/>[sdk] queue_to_coordinator"]
PYC["_PythonCoordinator<br/>queue 沒對應時的 fallback"]
LSC["SubprocessCoordinator<br/>Executable/Java/Node"]
subgraph SV["Supervisor|supervise_task()"]
SUP["ActivitySubprocess object<br/>WatchedSubprocess subtype"]
PSUP["_PopenActivitySubprocess object<br/>ActivitySubprocess subtype"]
end
end
subgraph TP["Task Runner child process|每個 Python task 一個"]
TR["task_runner.main()<br/>operator user code"]
end
subgraph LP["Lang runtime process|每個 lang SDK task 一個"]
GO["Go: bundle binary<br/>BundleRef.Serve()"]
JV["Java: java -classpath ...<br/>Server.serve()"]
TS["TS: node bundle.mjs<br/>bundle.serve()"]
end
AQ ==>|worker gets workload| LOOP
LOOP -->|supervise_task per task| CM
CM -->|queue not mapped| PYC
CM -->|queue mapped| LSC
PYC -.->|creates| SUP
LSC -.->|creates| PSUP
SUP ==>|puts outcome| RQ
PSUP ==>|puts outcome| RQ
WH -->|tracks/joins/terminates| LOOP
SUP -->|forks/starts + waits| TR
PSUP -->|launches runtime + waits| GO & JV & TS
classDef focus fill:#fff3bf,stroke:#e67700,stroke-width:3px
class SUP,PSUP focus
|
jason810496
left a comment
There was a problem hiding this comment.
the task runner sends it right before execute()
I think it's fine to handle that in this PR
Yes, both statements are correct.
What I imagined is 1) replace the Python side StartExecutionTimeout with StartExecutionTimeout while we still keep the same place to emit the message, where the bundle initialization already finished 2) for the Language SDK (_PopenActivitySubprocess), we should emit StartExecutionTimeout after the runtime starts, before StartupDetails
The Python task runner fails a task with a negative execution_timeout right away. Sent as-is, a negative value would instead move the supervisor's SIGTERM deadline to an arbitrary earlier point, and treating it as no timeout would let the task run forever. Clamping it to zero makes the supervisor treat the timeout as already elapsed, which is the closest the outside layer can get to the Python behavior.
658b814 to
98a60f5
Compare
|
Thanks both! I've updated the PR:
Follow-ups, as discussed:
|
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the update, LGTM once address the following comments.
| }, | ||
| ) | ||
|
|
||
| if (execution_timeout := get_execution_timeout(dag_bag, ti, session=session)) is not None: |
There was a problem hiding this comment.
nit: Let's retrieve the SerializedOperator via get_task and pass it down to arg_bindings and execution_timeout to avoid the duplicated prob overhead.
| ``None`` for regular tasks and for stub tasks that declare no parameters. | ||
| """ | ||
|
|
||
| execution_timeout: float | None = None |
There was a problem hiding this comment.
IIUC, we need to fill the task level execution_timeout at DagSerialization.fill_config_defaults.
We can keep execution_timeout for now and follow-up for rest of the task-level default arguments.
| # tell us "no, stop!" for any reason) | ||
| ti_context = self.client.task_instances.start(ti.id, self.pid, datetime.now(tz=UTC)) | ||
| self._should_retry = ti_context.should_retry | ||
| self._execution_timeout_seconds = ti_context.execution_timeout |
There was a problem hiding this comment.
IIUC, we don't need to set the _execution_timeout_seconds here. Let's keep the _arm_execution_timeout as single writer to mutate the attribute state.
Language SDK tasks never had their
execution_timeoutenforced, because only the Python task runner told the supervisor about it. Following the review, the timeout is now resolved once from the serialized Dag and enforced by the supervisor for every task:execution_timeoutonly fromTIRunContext. The Execution API resolves it from the serialized Dag and sends it for all tasks, Python included. This also covers mapped (.expand()) language SDK tasks, which previously got no timeout.SetExecutionTimeoutis replaced withStartExecutionTimeout. The message no longer carries the seconds, and the Python task runner still sends it from the same place, right beforeexecute().SetExecutionTimeoutshipped in 3.4.0b1 but is not in a final release yet, so the rename needs no supervisor schema migration if it lands before 3.4.0.StartExecutionTimeout, so bundle loading and Dag parsing are not counted. Dag parsing already has[core] dagbag_import_timeout.StartupDetailsis sent.The
execution_timeoutfield is added at2026-10-30in both the Execution API and the supervisor schema, so this needsbackport-to-v3-4-testbefore 3.4.0.Follow-ups (separate PRs):
execution_timeout+killed_task_cleanup_time.task.execution_timeoutinstead ofTIRunContext; that will move together with the decoupling.