fix: clean SQL tasks when dropping account - #27754
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
d0681ea to
bd47fb2
Compare
| fmt.Sprintf("select account_id from mo_catalog.mo_account where account_id = %d for update;", accountID), | ||
| fmt.Sprintf("update mo_task.sql_task set enabled = 0, updated_at = current_timestamp where account_id = %d;", accountID), | ||
| fmt.Sprintf( | ||
| "delete from mo_task.sys_async_task where task_parent_id in ("+ |
There was a problem hiding this comment.
[P1] Close the async-only DROP TASK state
This lookup can only recover a SQL task parent ID while either its definition or a run row still exists. A reachable lifecycle loses both mappings: let a scheduled task fire so TriggerSQLTask inserts sys_async_task with parent sql-task:, then DROP TASK before the executor calls AcquireSQLTaskRun. handleDropSQLTask/DeleteSQLTask removes only sql_task; no run exists yet. A later DROP ACCOUNT makes both subqueries here empty, leaving the queued/terminal async row behind after the account is gone. The row can keep being assigned/retried until generic cleanup, so this does not provide the advertised scheduled-async cleanup.
Please retire/delete child async rows when DROP TASK removes the last account/task mapping (with the same locking/transaction ordering), or retain an account mapping that DROP ACCOUNT can query, and add a regression for definition absent + run absent + queued async parent present.
aptend
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head bd47fb2 against base 662c1bd, including the complete diff, issue #27719, and all review/comment/thread history.
I traced SQL-task definition/run/async ownership; account, task, and run lock ordering; cross-CN scheduler/acquire/complete races; rollback paths; account-ID generation reuse; and both upgrade and fresh-install DDL behavior.
One blocking async-only lifecycle gap remains; see the inline P1. DROP TASK can erase the only account-to-parent mapping before a queued async child gets a run row, after which DROP ACCOUNT cannot discover or clean that child.
Validation performed on the exact head:
- affected frontend, taskservice, and v4.0.6 package tests passed
- focused race tests for taskservice, upgrade, and frontend cleanup/drop paths passed
- exact-head CI isolated two-CN race test package passed (181.86s)
- exact-head sql_task BVT passed 155/155
- git diff --check passed
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head bd47fb2700def3af3c811baee0e9e945e6b1292f against base 662c1bd63e8be02e3685e2695f002704c9781708 found one blocking lifecycle gap.
[P1 correctness] Preserve or clean the queued async child after DROP TASK removes its last account mapping.
The account cleanup at pkg/frontend/authenticate.go discovers async children only through mo_task.sql_task and mo_task.sql_task_run. A valid ordering loses both links: TriggerSQLTask first inserts a queued sys_async_task with parent sql-task:<task_id>; before an executor calls AcquireSQLTaskRun, DROP TASK deletes the sql_task definition; no sql_task_run exists yet. A later DROP ACCOUNT therefore produces an empty parent subquery and leaves that async row behind. It can still be assigned/retried after the owning account is gone, contradicting the PR's scheduled-async cleanup contract.
Please make DROP TASK retire/delete any queued child before deleting the last definition mapping, or retain a durable account-to-parent mapping that DROP ACCOUNT can query, under a lock/transaction ordering that composes with trigger/acquire/account-drop. Add a deterministic regression for: queued async child exists, definition has been dropped, no run row exists, then drop account; assert no async work survives or executes.
I also traced account/task/run locks, rollback, missing-account rejection, cross-CN running cleanup, fresh and upgrade DDL indexes, and the exact-head CI evidence. Those paths do not close this async-only orphan state.
What type of PR is this?
Which issue(s) this PR fixes:
fixes #27719
What this PR does / why we need it:
DROP ACCOUNT removed the tenant account but left SQL Task definitions, run rows, and scheduled async rows behind. Cached schedulers on other CNs could continue creating work for the deleted account.
This change:
Local verification on the latest main base: