Skip to content

Batch triggerer unused trigger cleanup deletes - #70961

Merged
vatsrahul1001 merged 7 commits into
apache:mainfrom
shivaam:fix-68243-batch-trigger-cleanup
Oct 18, 2026
Merged

vatsrahul1001 merged 7 commits into
apache:mainfrom
shivaam:fix-68243-batch-trigger-cleanup

Conversation

@shivaam

@shivaam shivaam commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

Trigger.clean_unused() runs synchronously in every triggerer supervisor loop. It previously removed every eligible trigger in one transaction, so large cleanups could hold many row locks and delay the triggerer heartbeat and concurrent database work.

This change deletes at most one configured batch of unreferenced triggers per triggerer loop and database transaction. Remaining triggers are handled in later loops, allowing the triggerer to resume runner servicing and heartbeat between batches.

The cleanup:

  • Uses a configurable batch size that defaults to 500 and rejects non-positive values.
  • Uses SKIP LOCKED so concurrent triggerers can select different rows.
  • Rechecks task, asset, and callback references in the DELETE before removing a trigger.
  • Uses a portable select-ID/delete-ID query shape on PostgreSQL, MySQL, and SQLite.
  • Retries a failed cleanup using a fresh database transaction.

Tests cover empty, below-limit, exact-limit, and larger-than-limit backlogs; draining five rows across three calls (2, 2, 1); protected task, asset, and callback references; invalid configuration; and fresh retry transactions.

Trade-off

Cleanup no longer drains the complete backlog in one call. Remaining rows wait for later triggerer loops. The batch size can be increased when necessary, with the trade-off that larger batches create larger transactions and may hold more database locks.

Follow-ups

  • The existing TaskInstance trigger-reference update is still unbounded. Batching it requires a separate concurrency design.
  • callback.trigger_id still needs a portable index and Alembic migration. That database change is intentionally kept separate from this focused cleanup change.
  • Cleanup database errors still escape the triggerer supervisor loop after retries are exhausted. This behavior predates this PR; a separate change can look and investigate this further.

Prior work: #68269 proposed the original batching direction. This implementation is a current-main rewrite because that draft predates #68244 and committed inside a caller-injected session.

Validation:

  • Focused PostgreSQL and MySQL cleanup suites: 13 passed on each backend.
  • PostgreSQL and MySQL 1,000-row probes each deleted two committed batches of [500, 500].
  • Real scheduler/triggerer PostgreSQL validation covered locked rows, eventual drain, and protected references.
  • Full SQLite trigger-model suite: 57 passed.
  • Regular and manual changed-file checks, formatting, linting, mypy, configuration validation, and security checks passed.
  • Rebased onto upstream/main at d0b8476825; all five contribution commits remained exact range-diff matches.

closes: #68243


Was generative AI tooling used to co-author this PR?
  • Yes — Codex (GPT-5)

Generated-by: Codex (GPT-5) following the guidelines

Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
@shivaam
shivaam marked this pull request as ready for review August 12, 2026 02:53
@shivaam
shivaam requested review from XD-DENG and ashb as code owners August 12, 2026 02:53
@potiuk potiuk added the ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green. label Aug 13, 2026
@eladkal eladkal added this to the Airflow 3.4.0 milestone Sep 11, 2026
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
Comment thread airflow-core/src/airflow/models/trigger.py Outdated
@shivaam
shivaam force-pushed the fix-68243-batch-trigger-cleanup branch from eb1f434 to f1157d6 Compare September 29, 2026 13:45
Large trigger tables make synchronous cleanup hold locks until every eligible row is deleted, delaying triggerer progress and database writers.
A large cleanup backlog must not keep the triggerer supervisor away from runner communication, heartbeat, and metrics work.
@shivaam
shivaam force-pushed the fix-68243-batch-trigger-cleanup branch from f1157d6 to f798a39 Compare September 29, 2026 15:03
@shivaam
shivaam marked this pull request as draft September 29, 2026 15:21
@shivaam
shivaam marked this pull request as ready for review September 29, 2026 15:58
@RishabhBajaj97

RishabhBajaj97 commented Oct 13, 2026 •

Copy link
Copy Markdown

Cross-reference: #74650 is a minimal, targeted fix for #74614 (triggerer exiting with PendingRollbackError after a transient metadata-DB disconnect). It adds a session.rollback() on DBAPIError/StaleDataError inside the existing run_with_db_retries() block in Trigger.clean_unused, mirroring the pattern already used by retry_db_transaction in airflow/utils/retries.py.

The restructure in this PR — moving the retry around a with create_session(scoped=False) block — would incidentally close #74614 as a side effect, since each retry attempt gets a fresh session. Flagging the overlap in case this PR's reviewers prefer to subsume the fix here (making #74650 redundant on merge) rather than carrying two parallel fixes.


Drafted-by: Claude Code (Opus 4.7) (human reviewed before posting)

@kaxil

kaxil commented Oct 14, 2026

Copy link
Copy Markdown
Member

Both points from the last round are in: the DELETE re-checks all three referrers, and each clean_unused() call now deletes at most one batch. LGTM.

Two non-blocking notes:

  • The batch_size <= 0 check runs inside run_once after load_triggers() has already claimed triggers. A value of 0 reads like "no batching" (that is how [state_store] state_cleanup_batch_size defines it), and here it crash-loops the triggerer, leaving its claimed triggers orphaned until the health check threshold passes. Validating it in TriggererJobRunner.__init__ next to capacity would fail before any DB work.
  • Batch the triggerer's Trigger.clean_unused() cleanup deletes #68243 describes the scheduler side as already fixed, but Batch the scheduler's unreferenced-trigger cleanup deletes #68241 was closed unmerged. SchedulerJobRunner._remove_unreferenced_triggers still deletes the whole backlog in one statement every parsing_cleanup_interval (60s by default). That deserves a follow-up, and the follow-up should decide whether it reuses this option or gets its own [scheduler] one.

@vatsrahul1001 vatsrahul1001 added the backport-to-v3-4-test Backport to v3-4-test label Oct 16, 2026
@vatsrahul1001
vatsrahul1001 merged commit c3cc557 into apache:main Oct 18, 2026
79 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-4-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
✅ v3-4-test PR Link

vatsrahul1001 pushed a commit that referenced this pull request Oct 18, 2026
…74950)

* Batch triggerer unused trigger cleanup deletes

Large trigger tables make synchronous cleanup hold locks until every eligible row is deleted, delaying triggerer progress and database writers.

* Add unreferenced to spelling wordlist

* Protect re-referenced triggers during cleanup

* Yield between unused-trigger cleanup batches

A large cleanup backlog must not keep the triggerer supervisor away from runner communication, heartbeat, and metrics work.

* Clarify unused-trigger batch behavior
(cherry picked from commit c3cc557)

Co-authored-by: Shivam Rastogi <6463385+shivaam@users.noreply.github.com>
kaxil added a commit to astronomer/airflow that referenced this pull request Oct 19, 2026
Restore the keyword-only session argument that apache#70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.

Co-Authored-By: Claude <noreply@anthropic.com>
kaxil added a commit to astronomer/airflow that referenced this pull request Oct 19, 2026
Restore the keyword-only session argument that apache#70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.
kaxil added a commit that referenced this pull request Oct 19, 2026
Restore the keyword-only session argument that #70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.
github-actions Bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Oct 19, 2026
…ssion (apache#75096)

Restore the keyword-only session argument that apache#70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.
(cherry picked from commit e3110a3)

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
aws-airflow-bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Oct 19, 2026
…ssion (apache#75096)

Restore the keyword-only session argument that apache#70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.
(cherry picked from commit e3110a3)

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
vatsrahul1001 pushed a commit that referenced this pull request Oct 20, 2026
…ssion (#75096) (#75122)

Restore the keyword-only session argument that #70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.
(cherry picked from commit e3110a3)

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
vatsrahul1001 pushed a commit that referenced this pull request Oct 20, 2026
…74950)

* Batch triggerer unused trigger cleanup deletes

Large trigger tables make synchronous cleanup hold locks until every eligible row is deleted, delaying triggerer progress and database writers.

* Add unreferenced to spelling wordlist

* Protect re-referenced triggers during cleanup

* Yield between unused-trigger cleanup batches

A large cleanup backlog must not keep the triggerer supervisor away from runner communication, heartbeat, and metrics work.

* Clarify unused-trigger batch behavior
(cherry picked from commit c3cc557)

Co-authored-by: Shivam Rastogi <6463385+shivaam@users.noreply.github.com>
vatsrahul1001 pushed a commit that referenced this pull request Oct 20, 2026
…ssion (#75096) (#75122)

Restore the keyword-only session argument that #70961 removed, so callers
that run the cleanup inside their own transaction keep working. The model
method now deletes one batch in the caller's transaction without retrying:
a retry would have to roll back that transaction and silently discard the
caller's other writes. The triggerer supervisor owns the retry instead and
runs each attempt in a new transaction.
(cherry picked from commit e3110a3)

Co-authored-by: Kaxil Naik <kaxilnaik@gmail.com>
SameerMesiah97 pushed a commit to SameerMesiah97/airflow-oss-sm97 that referenced this pull request Oct 27, 2026
* Batch triggerer unused trigger cleanup deletes

Large trigger tables make synchronous cleanup hold locks until every eligible row is deleted, delaying triggerer progress and database writers.

* Add unreferenced to spelling wordlist

* Protect re-referenced triggers during cleanup

* Yield between unused-trigger cleanup batches

A large cleanup backlog must not keep the triggerer supervisor away from runner communication, heartbeat, and metrics work.

* Clarify unused-trigger batch behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:ConfigTemplates area:Triggerer backport-to-v3-4-test Backport to v3-4-test ready for maintainer review Set by triage when all criteria pass, or by the fork CI reconciler when fork CI is green.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Batch the triggerer's Trigger.clean_unused() cleanup deletes

6 participants