Skip to content

fix: clean SQL tasks when dropping account - #27754

Open
Lundomn wants to merge 1 commit into
matrixorigin:mainfrom
Lundomn:issue-27719-main
Open

fix: clean SQL tasks when dropping account#27754
Lundomn wants to merge 1 commit into
matrixorigin:mainfrom
Lundomn:issue-27719-main

Conversation

@Lundomn

@Lundomn Lundomn commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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:

  • cleans SQL Task metadata in the account-drop transaction using account-lock, disable, async-run-definition deletion order;
  • serializes SQL Task creation with account deletion and rejects writes for missing accounts;
  • adds cleanup-key indexes to fresh bootstrap DDL and the v4.0.6 upgrade path;
  • covers rollback, missing accounts, batch errors, lock ordering, completed, failed, skipped, timed-out, suspended, repeating, never-run, cross-CN running, and concurrent CREATE TASK cases.

Local verification on the latest main base:

  • affected frontend, taskservice, and v4.0.6 package tests
  • race tests for all affected packages and the two-CN E2E
  • go vet for affected packages
  • taskservice coverage 90.0%, v4.0.6 coverage 85.4%, and all newly added frontend cleanup blocks covered
  • SQL Task BVT: 155/155 passed
  • make config, make err-check, make cgo, make build-with-prebuilt-native
  • git diff --check

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@CLAassistant

CLAassistant commented Aug 27, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@mergify mergify Bot added the kind/bug Something isn't working label Aug 27, 2026
@matrix-meow matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Aug 27, 2026
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 ("+

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aptend aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ("+

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: DROP ACCOUNT leaves orphan SQL Task definitions and run rows

5 participants