bnspopi opened a new pull request, #74651:
URL: https://github.com/apache/airflow/pull/74651
`Trigger.clean_unused()` retries its `UPDATE task_instance SET
trigger_id=NULL` through `run_with_db_retries()`, but reuses the same session
across attempts without rolling it back. When the metadata database connection
drops mid-statement, the transaction is left invalid, and the retried
`session.execute()` raises `PendingRollbackError` ("Can't reconnect until
invalid transaction is rolled back"). That is not one of the retried exception
types, so it propagates through `TriggerRunnerSupervisor.run` and the triggerer
exits — a pod restart for what was a momentary disconnect. Traceback and
reporter's logs are in #74614.
The fix is the one `retry_db_transaction` already applies for the same
reason: roll the session back when the statement raises
`DBAPIError`/`StaleDataError`, before the retry runs. That decorator cannot be
reused here directly — its docstring says it must not be combined with
`@provide_session`, which `clean_unused` needs — so the rollback is inline,
catching the same two exception types the retry predicate matches.
Nothing changes on the happy path: no new statement, no signature change, no
new config.
Tests: `test_clean_unused_rolls_back_before_retrying` makes the first
`session.execute` raise `OperationalError`, then asserts the session was rolled
back once and the cleanup still completed on the retry.
Scope I left alone deliberately:
- The `DELETE` after the loop is not inside the retry at all, so a
disconnect there still surfaces to the caller. That is pre-existing and a
separate decision about where the retry boundary should sit.
- `SchedulerJobRunner.check_trigger_timeouts` and
`check_awaiting_input_timeouts` use the same un-rolled-back inline retry
pattern. Same latent bug, different component; happy to follow up if you want
them in one sweep.
Relationship to #70961: that PR rewrites `clean_unused` to open a fresh
session per attempt and adds batching plus a new `[triggerer]
unreferenced_triggers_cleanup_batch_size` option, which would also resolve this
crash as a side effect. This is the minimal fix for the crash on its own — two
files, no config or signature change — so it can land (and backport) without
waiting on that discussion. If #70961 goes in first, this becomes unnecessary
and I will close it.
I could not run the test suite on this machine, so the first run will be CI
here; happy to follow up on anything it flags. Let me know if you want a
newsfragment — I did not have the PR number when committing.
closes: #74614
related: #70961
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (Claude Code)
Generated-by: Claude Code following [the
guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
---
* Read the **[Pull Request
Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)**
for more information. Note: commit author/co-author name and email in commits
become permanently public when merged.
* For significant user-facing changes create newsfragment:
`{pr_number}.significant.rst`, in
[airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments).
You can add this file in a follow-up commit after the PR is created so you
know the PR number.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
