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.
bd47fb2 to
831022a
Compare
831022a to
227f9d9
Compare
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 227f9d9. The new transactional DeleteSQLTask path closes the previously reported future async-only window, and package tests, the two-CN lifecycle test, focused race tests, and vet pass. One persisted-state cleanup gap remains blocking.
| 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.
The cleanup can only discover parent IDs that still have a sql_task definition or sql_task_run row. Before this fix, DeleteSQLTask removed only sql_task, so deployed data can already contain a sys_async_task child whose task_parent_id is sql-task:N while both mapping tables have no N. Both UNION arms are then empty and DROP ACCOUNT leaves that child behind. I reproduced this on the exact head by scheduling a task, constructing the reachable post-old-DROP-TASK state atomically (child status Error, no run, no definition), and dropping the account: the scheduled-async residue count remained 1 immediately and after the 12-second cross-CN refresh wait. The v4.0.6 upgrade only adds indexes, so it does not repair existing rows. Please add an upgrade/backfill cleanup for legacy orphan SQL-task children, or otherwise preserve enough account attribution so this account cleanup can remove them.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep-reviewed exact head 227f9d9 against base 28769ff and rechecked the complete 17-file diff plus prior review history. The previous P1 async-only orphan is closed: DeleteSQLTask locks the definition and atomically removes sql-task: children before the definition; TriggerSQLTask and AcquireSQLTaskRun serialize on the same task row. DROP ACCOUNT locks the account row, disables definitions, then removes async/run/definition metadata in one transaction; AddSQLTask now locks the account and rejects a missing generation, with sorted multi-account lock order. Audited rollback, concurrent trigger/acquire/create/drop, running/queued/terminal tasks, cross-tenant and recreated-account generations, fresh/upgrade indexes, bounded query/log cost, and cron snapshot synchronization (Q1-Q3). Exact-head CI is green and the regression suite includes the previous queued-child gap and two-CN lifecycle. No blocking correctness, liveness, resource, compatibility, or performance issue remains.
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: