1fanwang commented on issue #66794: URL: https://github.com/apache/airflow/issues/66794#issuecomment-5284604791
Closing this — I could not reproduce the deadlock, and the mechanism it assumes doesn't look reachable. The structural part of the report is accurate. QUEUED events do go through the same locked batch as the terminal ones, and their only effect is writing `external_executor_id` ([scheduler_job_runner.py L1473](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L1473) and L1491-1494). The deadlock is the part that doesn't hold up. I drove both contended statements against a real MySQL 8.4 with the real Airflow schema, building the queries with Airflow's own `TI.filter_for_tis` and `with_row_locks` so the SQL is what the scheduler actually emits: ``` scheduler SQL tail: ...task_id IN (__[POSTCOMPILE_task_id_1]) FOR UPDATE OF task_instance SKIP LOCKED supports_for_update_of: True ran 3 scheduler threads + 12 heartbeat threads for 45.0s ops: {'scheduler_txn': 6504, 'scheduler_rows_locked': 50389, 'heartbeat_txn': 10464} errors: none rows with both external_executor_id and last_heartbeat_at written: 60/60 ``` That is ~17k transactions in 45s contending over 60 task instance rows, far more per-row contention than a real deployment, with no `1213` and no `1205`. The schedulers averaged 7.7 of 60 rows per transaction, so `SKIP LOCKED` was skipping most of the batch — the contention was real, it just resolves by skipping instead of waiting. Which is why the cycle never closes: the scheduler cannot be the waiting party for those rows, and the heartbeat fast path is a single-row primary-key `UPDATE` that commits immediately. That path was already made lock-free — see the comment at [task_instances.py L945](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py#L945), "so we can update `last_heartbeat_at` directly without first taking a row lock". Caveats, so this is reopenable on better evidence: the harness drives the two statements rather than the whole `process_executor_events` body, on a single MySQL 8.4 node. If anyone has a `SHOW ENGINE INNODB STATUS` deadlock section naming these two statements, I would like to see it and will reopen. For anyone arriving from a PR: https://github.com/apache/airflow/pull/67043 and https://github.com/apache/airflow/pull/69973 both implemented the split correctly, and neither could show the deadlock clearing under load. That measurement is the thing this needed, and it now looks like there is nothing to clear. -- 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]
