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.

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.

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