[DONOTMERGE] Fix crash in monitor_GR_thread_HG due to stale DEBUG connection tracking (issue #5256)

[renecannao]

---

⚠️ DISCLAIMER: This PR was created by an AI coding agent (Claude)

Please review all changes carefully before merging. The AI agent has performed analysis and implemented a fix based on code review, but human verification is essential.

---

Summary

Fixes #5256 - Crash in monitor_GR_thread_HG with assertion failure:

MySQL_Monitor.cpp:270: assert(my!=my1)

Problem Description

When a stale connection was detected in get_connection(), it was queued for closure via a MON_CLOSE_CONNECTION work item. However, the connection was never removed from the DEBUG conns tracking array. This caused a race condition that could lead to a crash:

Crash Scenario:

1. Connection X retrieved from pool → added to DEBUG conns array
2. Connection X returned to pool → removed from DEBUG conns array
3. Connection X retrieved again → added to DEBUG conns array again
4. Connection X found stale → queued for MON_CLOSE_CONNECTION
5. Worker thread processes work item → connection X closed via destructor
6. BUG: Connection X pointer still remains in DEBUG conns array!
7. create_new_connection() returns MYSQL* with same address (malloc reuse)
8. conn_register() finds this pointer in conns → assertion fails

Why It's Hard to Reproduce:

• DEBUG-only (the conns array only exists in DEBUG builds)
• Timing-dependent (requires malloc to reuse the exact same pointer address)
• Rare condition (requires connection to become stale while checked out)

Root Cause

The stale connection was added to conns when retrieved from the pool, but was never removed from conns when found stale and queued for closure.

Solution

Before queuing a stale connection for closure, remove it from the DEBUG conns array. This ensures the connection pointer is properly unregistered before being closed and potentially reused by malloc.

Code Changes

File: lib/MySQL_Monitor.cpp
Function: MySQL_Monitor_Connection_Pool::get_connection()

Added DEBUG-only code to remove stale connections from the tracking array before queuing for closure.

Testing

• ✅ Compiles successfully
• Manual verification of code logic
• Awaiting testing and validation

Additional Notes

• The fix is wrapped in #ifdef DEBUG since the conns array is DEBUG-only
• Added verbose inline documentation for maintainability
• The same issue does NOT exist in purge_some_connections() because those connections are from the idle pool (already removed from conns when returned)

---

Reviewed by: @sysown (please review)

Summary by CodeRabbit

• Bug Fixes
  • Enhanced stale connection handling in database connection pooling to ensure proper resource cleanup and prevent potential issues with reusing pooled connections.

[gemini-code-assist]

Summary of Changes

Hello @renecannao, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical crash occurring in DEBUG builds of the MySQL monitor, specifically within the monitor_GR_thread_HG function. The issue stemmed from a race condition where stale connection objects were not properly unregistered from a debug-only tracking array (conns) before being closed. This could lead to an assertion failure if a new connection was allocated at the same memory address as a previously closed, but still tracked, stale connection. The change ensures that connections are correctly removed from the debug tracking mechanism, thereby preventing the crash and improving the stability of the monitor in debug environments.

Highlights

• Crash Fix: Resolved a crash in monitor_GR_thread_HG occurring in DEBUG builds, specifically an assertion failure (MySQL_Monitor.cpp:270: assert(my!=my1)).
• Race Condition Mitigation: Eliminated a race condition where stale connection pointers remained in the DEBUG conns tracking array, leading to assertion failures when malloc reused memory addresses for new connections.
• Connection Tracking Logic: Implemented a fix to explicitly remove stale connections from the DEBUG conns array before they are queued for asynchronous closure, ensuring proper unregistration.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog

• lib/MySQL_Monitor.cpp
  • Introduced a DEBUG-only block within MySQL_Monitor_Connection_Pool::get_connection() to iterate through the conns array and remove stale MYSQL pointers before queuing them for closure.

Activity

• The pull request is awaiting further testing and validation.
• The pull request has been reviewed by @sysown.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[coderabbitai]

📝 Walkthrough

Walkthrough

A DEBUG-only safeguard added to MySQL_Monitor.cpp removes stale MySQL connection pointers from the global DEBUG registry before queuing them for asynchronous closure, preventing duplicate registration attempts that triggered an assertion failure.

Changes

Cohort / File(s)	Summary
Stale Connection Cleanup lib/MySQL_Monitor.cpp	Added DEBUG-conditional logic to iterate the conns array and remove stale MYSQL pointers from the registry before asynchronous closure is queued, with logging of removals.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

status/triage

Poem

🐰 A stale connection once caused quite a fright,
Crashing the monitor in the dead of night.
But now with this fix, we clean the registry first,
Removing old pointers before they get burst!
Debug mode keeps things tidy and bright. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The code change directly addresses the root cause identified in issue #5256: removing stale connection pointers from the DEBUG conns array before queuing closure to prevent assertion failures on pointer reuse.
Out of Scope Changes check	✅ Passed	All changes are confined to MySQL_Monitor.cpp with a focused DEBUG-only fix that directly addresses the linked issue with no extraneous modifications.
Title check	✅ Passed	The pull request title clearly and specifically summarizes the main change: fixing a crash caused by stale DEBUG connection tracking with a direct reference to the issue number.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch v3.0-fix_5256

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[gemini-code-assist]

Code Review

This pull request addresses a crash in DEBUG builds caused by a race condition in stale connection tracking. The problem occurs when a stale connection is closed but its pointer is not removed from the debug tracking array, leading to a potential assertion failure if the memory address is reused. The proposed fix correctly removes the stale connection from the debug array before it's queued for closure, which effectively resolves the issue. The change is appropriately isolated within an #ifdef DEBUG block in MySQL_Monitor_Connection_Pool::get_connection(). The code is clear, and the detailed comments explaining the bug and the fix are excellent for maintainability. The implementation is sound, and I have no further recommendations.