Repository navigation
Conversation
fat-catTW
reviewed
Oct 5, 2026
jingi723
force-pushed
the
feature/ecs-stop-task-on-kill
branch
from
October 5, 2026 14:32
7b9206d to
137ebd9
Compare
Contributor
Author
|
The review feedback is addressed, all review threads are resolved, and Tests (AMD), CodeQL, and the newsfragment check pass on the current head. Could a maintainer take a final look when available? Thanks! |
Workloads managed independently of Airflow may need to finish after the waiting task is terminated. Dag authors currently need an on_kill override even when post-start failure cleanup is disabled.
jingi723
force-pushed
the
feature/ecs-stop-task-on-kill
branch
from
October 23, 2026 16:17
137ebd9 to
a408a33
Compare
This branch has not been deployed
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.
For workloads whose completion is managed independently of Airflow, stopping the waiting Airflow task should not necessarily stop the ECS task. Currently, Dag authors must override
EcsRunTaskOperator.on_kill()to preserve that work, even withstop_task_on_failure=False: that existing option controls post-start waiter failures, not the kill callback.Add
stop_task_on_kill, defaulting toTrue. Setting it toFalsestops local log fetching and skips the ECSStopTaskrequest without requiring an ECS client. Local log fetching is stopped before checking the policy or ECS client/ARN, so missing task details do not skip cleanup. By default, the operator still stops the ECS task when a client and ARN are available.stop_task_on_failureremains independent.The option applies when the worker calls
on_kill(), including an execution timeout while waiting synchronously. It does not change cancellation after deferral or guarantee callback execution after an abrupt worker failure. The guide explains responsibility for monitoring and cleaning up preserved tasks, and thatreattach=Truedoes not recover a completed task's result or guarantee deduplication.Validation on upstream
7a8ce83f3851ec65dd2186c75f90e85c666a7cc0, Python 3.10.21, Linux amd64 Breeze:test_ecs.py, using botocore Stubber for expected and forbidden requests. Keeping the new constructor option but restoring upstreamon_kill()makes three cases fail; the final implementation passes them.git diff --checkpassed. Manual checks omitted the three server/environment-provisioning hooks listed in the contributor instructions.Before submission, rebased onto
9c14681a4a02deefc256dcb686f693ecb591ea42. The three reviewed files are unchanged; the related ECS suite passed again (115 tests), and selective-check passed on the final commit.Review follow-up on upstream
e0e3454998f626046038852121a0714cf9748ca9: move log-fetcher cleanup before the client/ARN guard. The two missing-client/missing-ARN regressions fail before this adjustment and pass after it; the related ECS suite passes all 117 tests. Regular and manual prek checks, including provider mypy, pass. The broader tests and documentation commands selected bybreeze verifywere attempted but did not complete within local memory limits; they are not claimed as passing.Rebased on upstream
d613e999deeee19e3358042eaf507ef2fd479fd2after #73885 merged, preserving bothfail_on_exit_codeandstop_task_on_kill. The ECS operator, hook, and trigger test modules pass together (124 tests) in an existing Python 3.12.14 Linux/amd64 CI image. Regular and manual prek checks pass with CI-image-dependent hooks excluded (check-template-fields-valid,check-provider-yaml-valid, and manualmypy-providers); the fullbreeze verifymatrix was not rerun locally. CI on this rebased head is pending.No live AWS resources were used. The full dependent-provider, supported-version and platform CI matrices were not run locally. This is separate from the exit-code retry policy in #73885.
Was generative AI tooling used to co-author this PR?
Generated-by: Codex (GPT-6) following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.