[DBMON-6881] Move mysql onto the DatabaseCheck job registry and cancellation lifecycle - #24936
Conversation
…ycle Move job construction into _register_async_jobs(), guarded on dbm_enabled, and register each job with the DatabaseCheck registry. The fan-out in check() becomes run_async_jobs(), and the cancel() override is deleted so the base protocol runs. Also delete MySQLMetadata.shut_down() and DatabasesData.shut_down(): nothing calls them, and they cannot be wired into the new shutdown() hook because collect_databases_data resets the submitter before the job loop exits. Co-authored-by: Cursor <cursoragent@cursor.com>
Give each job a shutdown() that closes its connection and drops the two references pinning the check: _check, and _connection_args_provider, which is a bound method of the check and so pins it independently. MySQLMetadata also drops _databases_data, which points back at both the job and the check. The check's own shutdown() releases the three objects that hold it: the query manager, the cached runtime query executor, and the health reporter. test_check_gc_after_cancel, ported from postgres, is what found these: with the cycle collector disabled it asserts refcounting alone reclaims the check. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
evalya-impact-summaryevalya impact analysis |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: eb87c2e | Docs | View more details | Give us feedback! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd4993eb2f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Validation ReportAll 21 validations passed. Show details
|
What does this PR do?
Registers mysql's four
DBMAsyncJobs with theDatabaseCheckregistry and inherits the cancellation lifecycle._register_async_jobs(), guarded ondbm_enabled. Each job also has its own enabled flag that defaults to true, so DBM is checked once here rather than left to the jobs. Previously all four were built regardless, and only the guard around the fan-out kept a non-DBM instance from collecting.run_job_loopcalls incheck()becomeself.run_async_jobs(dbm_tags).cancel()override is deleted. It signalled the four jobs and returned, never waiting for the loops to stop or dropping the references that keep the check alive after the Agent unschedules it; the base protocol does both.shutdown()that closes its connection and drops the references pinning the check, and the check gets one that releases the query manager, the cached runtime query executor and the health reporter.MySQLMetadata.shut_down()andDatabasesData.shut_down()are deleted. Nothing has ever called them, and they cannot be wired into the newshutdown()hook:collect_databases_dataresets the submitter in afinally,reset()clearsdb_to_tables, andsubmit()returns early when it is empty. The base calls a job'sshutdown()only afterwait_for_completion(), so the reset has always already run.Motivation
Third of four in the DBM migration onto the shared lifecycle, after postgres (#24933) and clickhouse (#24934). It removes a fan-out that has to be hand-edited whenever a job is added, and gets mysql the loop join and reference cleanup its own
cancel()never did.Porting postgres's
test_check_gc_after_cancelis what turned up the leaks theshutdown()methods now fix — with the cycle collector disabled, it asserts refcounting alone reclaims the check. Every nulled attribute was confirmed load-bearing by removing it and watching that test fail; the notable one is_connection_args_provider, a bound method of the check, which pins the check even after_checkis nulled.Review checklist (to be filled by reviewers)
qa/requiredif this PR needs QA validation, orqa/skip-qaif it does not. Exactly one of the two is required.backport/<branch-name>label to the PR and it will automatically open a backport PR once this one is mergedMade with Cursor