You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Vamsi-klu (via GitHub)" <gi...@apache.org> on 2026/05/16 21:41:52 UTC
[PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Vamsi-klu opened a new pull request, #67052:
URL: https://github.com/apache/airflow/pull/67052
Fix `CloudComposerDAGRunSensor` and `CloudComposerDAGRunTrigger` so they only succeed when at least one Dag run falls inside the requested execution window and every in-window run is in an allowed state.
Why this change is needed:
- the current window-checking helper returns success when no in-window Dag runs are found, as long as it also does not see an in-window disallowed state
- that means unrelated out-of-window Dag runs can make the sensor succeed even though no relevant Dag run exists for the requested window
- the same behavior exists in both the sync sensor and the deferrable trigger, so both paths need to be corrected together
What changes in behavior:
- if all returned Dag runs are outside the execution window, the sensor now waits instead of succeeding
- if at least one Dag run is inside the window, success still requires all in-window runs to be in an allowed state
- `composer_dag_run_id` behavior is unchanged
- open-interval boundary behavior is preserved
What is covered:
- sync sensor unit tests for out-of-window, mixed-window, rejected in-window, and boundary-only cases
- deferrable trigger unit tests for continued polling on out-of-window results and success on an in-window allowed run
- system test example updated to use an explicit execution window so it does not rely on the previous false-positive behavior
Follow-up:
- the matching `CloudComposerExternalTaskSensor` and trigger anti-pattern is tracked in #67051
closes: #57512
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Codex (GPT-5)
Generated-by: Codex (GPT-5) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4759820479
Rebased onto current main. To the system-test concern from the #61046 revert: the example DAG no longer relies on a logical_date window. Each sensor now pins to the run_id its CloudComposerTriggerDAGRunOperator just produced (composer_dag_run_id via XCom), which uses the id+state-equality path and bypasses execution_range entirely — so the empty-window→wait path that hung #61046 isn't exercised. A defensive timeout=3600 makes any regression back to the windowed path fail CI fast instead of hanging the worker, and dev/simulate_composer_system_test.py drives the example end-to-end with all GCP surfaces mocked so the no-hang behaviour is reproducible without credentials. Could you run the live Composer system test against this when you have a chance? The analogous external-task bug is intentionally left and tracked at #67051.
---
Drafted-by: Claude Code (Opus 4.8); reviewed by @Vamsi-klu before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk closed pull request #67052: Google: Fix CloudComposer Dag-run empty-window success
URL: https://github.com/apache/airflow/pull/67052
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-5462505026
> @shahar1 I still cannot run example_cloud_composer on real Composer from here.
> Your merge condition is that live run. I can leave this PR open if you or the Google team still plan to run it on the community project. I can also close it until someone actually has a run, and reopen or open a new PR then.
> Which do you prefer? I don't want this sitting in the review queue if the test is not going to happen soon. I don't want to increase the review backlog.
Then we should close it @Vamsi-klu -> you seem to open a lot of PRs with Claude Code but it seems you do not test it. If you cannot run system test now, it means your changes are theorethical and basically you are throwing testing over the wall to someone else. Which is not a good idea. I am closing all Google provider ones from you in this case - feel free to reopen your PRs for providers with a proof that you actuallly run the tests manually with real Google account.
Also whenever we release a provider with such changes **you** are requested to installl an test such modified provider with **real** google service.
The providers **are** supposed to be run with raal services they are accessing and not "theorethically" fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-5149823836
Quick update. The simulation script is gone from the diff, per your earlier comment. The latest push also clears the deterministic CI failures: utils/composer.py now has its own test file (25 tests for the window and date-parsing helpers), and the spellcheck complaints are fixed by rewording the docstrings instead of adding words to the wordlist.
That leaves the real blocker, which is the one you named: running the existing Composer system test on actual GCP. I still don't have access to do that, so I'm keeping this in draft until such a run exists. If your offer to help run it still stands I'd appreciate it — I'll attach the results as soon as there's a run to show.
---
Drafted-by: Claude Code (Fable 5); reviewed by @Vamsi-klu before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-5384839706
@shahar1 I still cannot run example_cloud_composer on real Composer from here.
Your merge condition is that live run. I can leave this PR open if you or the Google team still plan to run it on the community project. I can also close it until someone actually has a run, and reopen or open a new PR then.
Which do you prefer? I don't want this sitting in the review queue if the test is not going to happen soon. I don't want to increase the review backlog.
---
Drafted-by: Cursor Grok 4.6; reviewed by @Vamsi-klu before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "MaksYermak (via GitHub)" <gi...@apache.org>.
MaksYermak commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4520166389
@nnguyen168 could you please run the [existing System Test](https://github.com/apache/airflow/blob/main/providers/google/tests/system/google/cloud/composer/example_cloud_composer.py) for this operator and shared the screenshot for the latest run?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4520683347
> Hi @shahar1 -- addressed the system-test concern.
>
> Why PR #61046 had to be reverted (revert commit [14995f1](https://github.com/apache/airflow/commit/14995f1967f299bf4ecb16133fb1c18f49ec1500)): the example called CloudComposerDAGRunSensor without execution_range, so the sensor inferred the window [logical_date - 1d, logical_date]. The Composer environment was created mid-DAG-run, which meant no airflow_monitoring runs ever fell inside that past window. The buggy code returned True (false positive); the empty-window fix correctly returns False and the example then hung forever.
>
> The example patch in the earlier revision of this PR tried to side-step the issue with execution_range=[datetime.now() - timedelta(1), datetime.now()], but that's evaluated at DAG-parse time -- roughly half an hour before the sensor pokes and before the Composer env exists. Same hang mode, just hidden.
>
> What this commit changes:
>
> * composer_dag_run_id added to CloudComposerDAGRunSensor.template_fields.
> * Example reordered so each CloudComposerTriggerDAGRunOperator runs before its sibling Dag-run sensor; sensors pull the freshly minted dag_run_id via XCom. The run-id branch is pure id+state equality and bypasses execution_range entirely, so the hang mode cannot recur on the new path.
> * Defensive timeout=60*60 on the Dag-run sensors so any future regression to a windowed code path fails CI fast rather than hanging the worker.
> * External-task sensors deliberately left on the legacy windowed path -- the analogous _check_task_instances_states anti-pattern is tracked at [CloudComposerExternalTaskSensor succeeds when all task instances are outside execution_range #67051](https://github.com/apache/airflow/issues/67051), and the example carries an inline note so the [CloudComposerExternalTaskSensor succeeds when all task instances are outside execution_range #67051](https://github.com/apache/airflow/issues/67051) fix must reorder them at the same time.
>
> Verification I can run locally without GCP credentials:
>
> * six new sensor unit tests + two new trigger unit tests covering the composer_dag_run_id branch (template_fields, Jinja render, poke success/wait, range-vs-id precedence, parse-time source guard, trigger branch precedence, trigger polling) -- all 75 tests in providers/google/tests/unit/google/cloud/{sensors,triggers}/test_cloud_composer.py pass.
> * dev/simulate_composer_system_test.py, a standalone harness that loads the example via importlib, mocks every GCP surface the example touches (googleapiclient + CloudComposerHook + CloudComposerAsyncHook + Environment / ImageVersion / ExecuteAirflowCommandResponse.to_dict), runs dag.test() end-to-end, and reports per-task state. The deferred trigger runs inline under dag.test(), so the deferrable sensor path is exercised too.
>
> Simulation result:
>
> ```
> $ uv run --project providers/google python dev/simulate_composer_system_test.py
> loading example dag from .../example_cloud_composer.py
> registering dag bundle and serializing dag
> running dag.test() on composer
> OK: dag.test() completed for composer -- final state: success
> create_env -> success
> dag_run_sensor -> success
> defer_create_env -> success
> defer_dag_run_sensor -> success
> defer_delete_env -> success
> defer_external_task_sensor -> success
> defer_run_airflow_cli_cmd -> success
> defer_trigger_dag_run -> success
> defer_update_env -> success
> delete_env -> success
> external_task_sensor -> success
> get_env -> success
> get_project_number -> success
> image_versions -> success
> list_envs -> success
> run_airflow_cli_cmd -> success
> trigger_dag_run -> success
> update_env -> success
> watcher -> skipped
> ```
>
> Both the sync dag_run_sensor and the deferred defer_dag_run_sensor matched on the trigger ops' run id via the composer_dag_run_id branch.
>
> @VladaZakharova @MaksYermak -- would appreciate a confirmation pass against real Composer infra when convenient, since the simulation can only validate the control flow, not the actual REST API contracts.
>
> Drafted-by: Claude Code (Opus 4.7 1M); reviewed by @Vamsi-klu before posting
When asking to run the system tests, I always refer to a real GCP instance - simulating it against a mock isn't helpful (maybe with exception for very specific services in GCP that have emulators).
Therefore, please remove `dev/simulate_composer_system_test.py`.
As you currently lack the means to run the system tests, I'll see if I'm able to help with running them (can't promise that it will happen soon).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "nnguyen168 (via GitHub)" <gi...@apache.org>.
nnguyen168 commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4519457561
Hi @shahar1 @Vamsi-klu I ran the unit tests and local simulation for this PR.
**Environment:**
- macOS (Darwin)
- Python 3.12.11
- uv 0.11.16
**Unit Test Results:**
- `test_cloud_composer.py` (sensors): **59/59 passed** ✅
- `test_cloud_composer.py` (triggers): **16/16 passed** ✅
**Local Simulation (`dev/simulate_composer_system_test.py`):**
- Final state: **success** ✅
- Both sync `dag_run_sensor` and deferred `defer_dag_run_sensor` matched via `composer_dag_run_id` branch
- All 18 tasks completed successfully (watcher skipped as expected)
*Note*: I don't have GCP credentials to run the actual system tests against real Composer infrastructure. The unit tests and local simulation validate the control flow, but real GCP validation is still needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "nnguyen168 (via GitHub)" <gi...@apache.org>.
nnguyen168 commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4524875038
I've validated the unit tests (75/75 passing) and local simulation, but I don't have GCP credits remaining to run the full system test against real Composer infrastructure (~$2-4 cost per run).
Could a maintainer or someone with GCP access help run the system test and share the results? Alternatively, is there a CI workflow for system tests that can be triggered?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "nnguyen168 (via GitHub)" <gi...@apache.org>.
nnguyen168 commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4519464759
```bash
OK: dag.test() completed for composer -- final state: success
the sync `dag_run_sensor` and deferred `defer_dag_run_sensor` both
matched on dag_run_id='manual__simulated' via the composer_dag_run_id branch.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4472807831
Hi @shahar1 -- addressed the system-test concern.
Why PR #61046 had to be reverted (revert commit 14995f1967f29):
the example called CloudComposerDAGRunSensor without execution_range, so the sensor inferred the window [logical_date - 1d, logical_date]. The Composer environment was created mid-DAG-run, which meant no airflow_monitoring runs ever fell inside that past window. The buggy code returned True (false positive); the empty-window fix correctly returns False and the example then hung forever.
The example patch in the earlier revision of this PR tried to side-step the issue with execution_range=[datetime.now() - timedelta(1), datetime.now()], but that's evaluated at DAG-parse time -- roughly half an hour before the sensor pokes and before the Composer env exists. Same hang mode, just hidden.
What this commit changes:
- composer_dag_run_id added to CloudComposerDAGRunSensor.template_fields.
- Example reordered so each CloudComposerTriggerDAGRunOperator runs before its sibling Dag-run sensor; sensors pull the freshly minted dag_run_id via XCom. The run-id branch is pure id+state equality and bypasses execution_range entirely, so the hang mode cannot recur on the new path.
- Defensive timeout=60*60 on the Dag-run sensors so any future regression to a windowed code path fails CI fast rather than hanging the worker.
- External-task sensors deliberately left on the legacy windowed path -- the analogous _check_task_instances_states anti-pattern is tracked at https://github.com/apache/airflow/issues/67051, and the example carries an inline note so the #67051 fix must reorder them at the same time.
Verification I can run locally without GCP credentials:
- six new sensor unit tests + two new trigger unit tests covering the composer_dag_run_id branch (template_fields, Jinja render, poke success/wait, range-vs-id precedence, parse-time source guard, trigger branch precedence, trigger polling) -- all 75 tests in providers/google/tests/unit/google/cloud/{sensors,triggers}/test_cloud_composer.py pass.
- dev/simulate_composer_system_test.py, a standalone harness that loads the example via importlib, mocks every GCP surface the example touches (googleapiclient + CloudComposerHook + CloudComposerAsyncHook + Environment / ImageVersion / ExecuteAirflowCommandResponse.to_dict), runs dag.test() end-to-end, and reports per-task state. The deferred trigger runs inline under dag.test(), so the deferrable sensor path is exercised too.
Simulation result:
```
$ uv run --project providers/google python dev/simulate_composer_system_test.py
loading example dag from .../example_cloud_composer.py
registering dag bundle and serializing dag
running dag.test() on composer
OK: dag.test() completed for composer -- final state: success
create_env -> success
dag_run_sensor -> success
defer_create_env -> success
defer_dag_run_sensor -> success
defer_delete_env -> success
defer_external_task_sensor -> success
defer_run_airflow_cli_cmd -> success
defer_trigger_dag_run -> success
defer_update_env -> success
delete_env -> success
external_task_sensor -> success
get_env -> success
get_project_number -> success
image_versions -> success
list_envs -> success
run_airflow_cli_cmd -> success
trigger_dag_run -> success
update_env -> success
watcher -> skipped
```
Both the sync dag_run_sensor and the deferred defer_dag_run_sensor matched on the trigger ops' run id via the composer_dag_run_id branch.
@VladaZakharova @MaksYermak -- would appreciate a confirmation pass against real Composer infra when convenient, since the simulation can only validate the control flow, not the actual REST API contracts.
---
Drafted-by: Claude Code (Opus 4.7 1M); reviewed by @Vamsi-klu before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-4588770576
Gentle bump on this PR. The code changes and unit coverage are in place, CI is green, and the remaining blocker appears to be confirmation from a real Composer system-test run. Could a maintainer or someone with GCP/Composer access please help take another look or advise on the next step when available?
Thanks.
---
Drafted-by: Codex (GPT-5); reviewed by @Vamsi-klu before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Google: Fix CloudComposer Dag-run empty-window success [airflow]
Posted by "Vamsi-klu (via GitHub)" <gi...@apache.org>.
Vamsi-klu commented on PR #67052:
URL: https://github.com/apache/airflow/pull/67052#issuecomment-5140706780
I don't have a Composer environment to run the system test against. The change is in `providers/google/.../cloud_composer.py`; could a maintainer with Composer access run `example_cloud_composer` on this branch, or advise how to get a test environment?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org