Milestone 3 Phase 3.3: Extract monitor health state decision logic for unit testing

[renecannao]

Parent Issue

Part of #5479 — Unit Testing Milestone 3: Extract Testable Logic from Coupled Classes

Context

Phase 3.3 (#5491) covers MySQL_Monitor health decisions. The Monitor module decides when to shun/unshun servers based on health check results (ping, read-only status, replication lag, heartbeat failures). These decisions are currently embedded in check handler callbacks that are coupled to real MySQL connections and the HostGroups Manager.

The decision logic itself is algorithmic: given health check results and thresholds, determine the next server status. This can be extracted and tested with mock health data.

Current Code

File: `lib/MySQL_Monitor.cpp`

Key decision points:

1. Server status determination (lines ~2855-2906): Combines `viable_candidate`, `read_only`, and check results into `MySerStatus` (ONLINE, OFFLINE_SOFT, SHUNNED)
2. Heartbeat-based shunning (lines ~3182-3221): Consecutive ping failures → shun and kill all connections
3. Unshunning logic (in `Base_HostGroups_Manager.cpp` ~2194-2245): Time-based recovery from SHUNNED state

Scope

Extract:

1. `determine_server_status()` — Given health check results and thresholds:
   ```cpp
   enum MySerStatus determine_server_status(
   enum MySerStatus current_status,
   bool ping_successful,
   bool read_only_check_passed,
   bool is_read_only,
   int replication_lag_ms,
   int max_replication_lag_ms,
   unsigned int consecutive_failures,
   unsigned int max_failures_threshold
   );
   ```

2. `should_shun_on_failures()` — Heartbeat failure shunning:
   ```cpp
   bool should_shun_on_failures(
   unsigned int consecutive_failures,
   unsigned int max_failures,
   unsigned int current_connections
   );
   ```

3. `can_unshun_server()` — Time-based recovery:
   ```cpp
   bool can_unshun_server(
   time_t time_last_error,
   time_t current_time,
   int shun_recovery_time_sec,
   int unshun_algorithm
   );
   ```

Unit tests should cover:

• Healthy server stays ONLINE
• Ping failure → SHUNNED
• Read-only detected → OFFLINE_SOFT (for writer hostgroup)
• Replication lag exceeds threshold → excluded from routing
• Consecutive failures exceed max → shun and kill all
• Single failure below max → stays ONLINE
• Shunned server recovers after recovery time elapsed
• Shunned server stays shunned if recovery time not elapsed
• Different `unshun_algorithm` values produce different behavior
• Edge cases: zero thresholds, max thresholds

IMPORTANT: Implementation Instructions

Branch and PR rules

• Create your branch from `v3.0-5473`, NOT from `v3.0`
• Target the PR to `v3.0-5473`, NOT to `v3.0`
• Branch name should be `v3.0-5491`

Where to place test files

• Test file MUST go in `test/tap/tests/unit/`, NOT in `test/tap/tests/`
• Name the test file `monitor_health_unit-t.cpp`

How to write the test file

The project has a unit test harness. Your test file MUST:

1. Include the harness headers:
   ```cpp
   #include "tap.h"
   #include "test_globals.h"
   #include "test_init.h"
   ```

2. Include proxysql headers for the types you need:
   ```cpp
   #include "proxysql.h"
   #include "cpp.h"
   ```

3. Call `test_init_minimal()` at the start of `main()`

4. Call `test_cleanup_minimal()` at the end

5. Use TAP functions: `plan()`, `ok()`, `exit_status()`

DO NOT:

• Do NOT reimplement the extracted functions in the test file — the test must call the REAL functions from `libproxysql.a`
• Do NOT manually define `noise_failures`, `noise_failure_mutex`, `stop_noise_tools`, or `get_noise_tools_count` — these are already provided by `test_globals.cpp`
• Do NOT place the test in `test/tap/tests/` — it MUST go in `test/tap/tests/unit/`

How to register the test in the Makefile

Edit `test/tap/tests/unit/Makefile`:

1. Add the test name to the `UNIT_TESTS` variable
2. Add a build rule following the existing pattern:
   ```makefile
   monitor_health_unit-t: monitor_health_unit-t.cpp $(TEST_HELPERS_OBJ) $(LIBPROXYSQLAR)
   $(CXX) $< $(TEST_HELPERS_OBJ) $(IDIRS) $(LDIRS) $(OPT) \
   $(LIBPROXYSQLAR_FULL) $(STATIC_LIBS) $(MYLIBS) \
   $(ALLOW_MULTI_DEF) -o $@
   ```

Where to place extracted function declarations

If you create new header files for the extracted function declarations, place them in `include/` with a proper include guard. Use the same pattern as `include/ConnectionPoolDecision.h` — a standalone header with no circular dependencies.

Reference examples

Look at these existing files for the correct pattern:

• `test/tap/tests/unit/connection_pool_unit-t.cpp` — unit test using harness
• `test/tap/tests/unit/rule_matching_unit-t.cpp` — unit test using harness
• `include/ConnectionPoolDecision.h` — standalone extracted function header

Acceptance Criteria

• Extracted functions are pure (no global state access, no I/O)
• Existing E2E TAP tests pass unchanged (behavior-preserving refactoring)
• Unit tests cover all decision branches
• PR targets `v3.0-5473` (NOT `v3.0`)
• Test file is in `test/tap/tests/unit/`
• Test uses harness (`test_globals.h`, `test_init.h`)
• Test does NOT reimplement extracted functions
• Doxygen documentation on extracted functions

[coderabbitai]

🔗 Related PRs

#5486 - Query Cache unit tests (Phase 2.3) [closed]
#5487 - Query Processor rule management unit tests (Phase 2.4) [closed]
#5488 - Protocol encoding/decoding and utility unit tests (Phase 2.5) [closed]
#5502 - Phase 3.1: Connection pool decision extraction + unit tests [closed]
#5503 - Phase 3.2: Query rule matching extraction + unit tests [closed]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[renecannao]

@coderabbitai plan

[coderabbitai]

Coding Plan

Summary

• Create minimal unit/ infrastructure (directory + Makefile) inheriting from existing test patterns, adapting ticket requirements to match codebase reality where test_globals.h/test_init.h don't exist and creating them would expand scope beyond the ticket
• Extract three pure decision functions into include/MonitorHealthDecision.h and lib/MonitorHealthDecision.cpp, with signatures designed to capture actual production decision inputs rather than forcing the ticket's simplified signatures
• Integrate extracted functions into production code paths (MySQL_Monitor.cpp, MySQL_HostGroups_Manager.cpp, MyHGC.cpp) as behavioral pass-throughs preserving existing semantics
• Create comprehensive unit tests calling real production functions via libproxysql.a linkage, covering all decision branches including edge cases for thresholds, timing, and algorithm variations

Design Choices

Design Choice 1: Unit test infrastructure needs to be created

Options Considered:

1. Follow ticket instructions exactly and create all referenced infrastructure from scratch
2. Use existing unit_test.h pattern from unit-strip_schema_from_query-t.cpp without new infrastructure
3. Create minimal infrastructure that bridges ticket requirements with existing patterns

Chosen Option: 3

Rationale: Create the unit/ directory and a minimal Makefile inheriting from the parent, using the existing unit_test.h stub pattern since test_globals.h/test_init.h don't exist and creating them would expand scope beyond the ticket.

Design Choice 2: Function signature adaptation

Options Considered:

1. Use exact ticket signatures and adapt production code to fit
2. Design signatures that match actual production logic parameters
3. Create wrapper functions that translate between ticket signatures and production logic

Chosen Option: 2

Rationale: Design signatures that accurately capture the production decision inputs; this ensures the extracted functions remain directly usable without adapter layers and match the actual shunning/unshunning conditions in the codebase.

💡 User Tips

Regenerate the plan with different choices with @coderabbitai <feedback>.

Implementation Steps

Phase 1: Create Unit Test Infrastructure

This phase establishes the unit/ subdirectory and its build system, enabling standalone unit tests that link against libproxysql.a without requiring a full ProxySQL instance.

Task 1: Create Unit Test Directory and Makefile

Set up the directory structure and build rules for unit tests following the pattern established by test/tap/tests_with_deps/deprecate_eof_support/Makefile.

• Create directory test/tap/tests/unit/
• Create test/tap/tests/unit/Makefile that:
  • Walks up to find PROXYSQL_PATH and includes makefiles_vars.mk and makefiles_paths.mk
  • Defines IDIRS, LDIRS, OPT by copying the pattern from test/tap/tests/Makefile
  • Defines LIBPROXYSQLAR := $(PROXYSQL_LDIR)/libproxysql.a
  • Defines UNIT_TESTS variable listing all unit test targets
  • Creates a pattern rule %-t: %-t.cpp linking against -lproxysql and other required libraries
  • Includes -Wl,--allow-multiple-definition linker flag to handle ODR issues when linking partial ProxySQL code
• Add unit target to parent test/tap/tests/Makefile that descends into unit/ subdirectory

Task 2: Create Unit Test Helper Header

Extend the existing unit_test.h pattern with additional stubs needed for monitor health tests, or create a supplementary header if needed.

• Review existing test/tap/tests/unit_test.h for required stubs (PROXYSQL_EXTERN, EXCLUDE_TRACKING_VARIABLES, GloMyLdapAuth)
• Determine if additional global stubs are needed for monitor code (e.g., GloMyMon, MyHGM pointers)
• If the extracted functions are truly pure (no global access), no additional stubs should be needed
• Document in code comments that tests use unit_test.h from parent directory

🤖 Prompt for AI agents

Set up the `test/tap/tests/unit/` directory and its build system so that unit
tests can be compiled and linked against `libproxysql.a` without a running
ProxySQL instance.

- Create the directory `test/tap/tests/unit/`
- Create `test/tap/tests/unit/Makefile`:
- Walk up the directory tree to locate `PROXYSQL_PATH`, then include
`makefiles_vars.mk` and `makefiles_paths.mk`
- Define `IDIRS`, `LDIRS`, and `OPT` by following the pattern from
`test/tap/tests/Makefile`
  - Define `LIBPROXYSQLAR := $(PROXYSQL_LDIR)/libproxysql.a`
  - Define a `UNIT_TESTS` variable that will list all unit test targets
- Add a pattern rule `%-t: %-t.cpp` that links against `-lproxysql` and other
required libraries
- Include `-Wl,--allow-multiple-definition` as a linker flag to handle ODR
issues from partial ProxySQL code linkage
- In the parent `test/tap/tests/Makefile`, add a `unit` target that descends
into the `unit/` subdirectory
- Review `test/tap/tests/unit_test.h` for existing stubs (`PROXYSQL_EXTERN`,
`EXCLUDE_TRACKING_VARIABLES`, `GloMyLdapAuth`) and determine whether additional
global stubs (e.g., `GloMyMon`, `MyHGM`) are needed for monitor code; if the
extracted functions are pure, no additional stubs should be required
- Add a comment in the test helper noting that `unit_test.h` from the parent
directory is reused

Phase 2: Extract Health Decision Functions

This phase creates pure, testable functions that encapsulate the server health state transition logic currently embedded in various monitor and hostgroup manager methods.

Task 1: Create MonitorHealthDecision Header

Define the extracted function signatures in a standalone header following the include/gen_utils.h pattern for pure function declarations.

• Create include/MonitorHealthDecision.h with proper include guard
• Include include/proxysql_structs.h for MySerStatus enum
• Declare determine_server_status() with parameters reflecting actual decision inputs:
  • Current status, ping success, read-only value, writer/reader context
  • Replication lag value and threshold, consecutive lag violation count and threshold
  • Group Replication viable_candidate and read_only flags
• Declare should_shun_on_failures() with parameters:
  • Consecutive failure count, max failures threshold
  • Current connections count, shunned_and_kill_all_connections flag
• Declare can_unshun_server() with parameters:
  • Time of last error, current time
  • Shun recovery time (seconds), connect timeout server max (for capping)
  • shunned_and_kill_all_connections flag, connections used/free counts
  • unshun_algorithm value
• Add Doxygen documentation blocks explaining each function's decision logic and return values

Task 2: Implement Extracted Functions

Create the implementations as pure functions with no global state access, extracting logic from existing embedded code.

• Create lib/MonitorHealthDecision.cpp implementing all three functions
• Extract determine_server_status() logic from:
  • ReadySet status mapping (MySQL_Monitor.cpp ~2879-2887): Online→ONLINE, Maintenance→OFFLINE_SOFT, else→SHUNNED
  • Group Replication logic (~2107-2130): viable_candidate=NO→OFFLINE, read_only=YES→reader, else→writer
  • Replication lag threshold check (MySQL_HostGroups_Manager.cpp ~2710-2730): lag > max with count check
• Extract should_shun_on_failures() logic from:
  • Ping failure shunning (MySQL_Monitor.cpp ~3218-3222): consecutive failures >= max → shun
  • Connect error shunning (MySrvC.cpp ~110-120): errors_this_second >= min(shun_on_failures, connect_retries+1)
• Extract can_unshun_server() logic from:
  • MyHGC.cpp recovery loop (~83-105): compute max_wait_sec with connect_timeout capping, check elapsed time, verify connection drain if shunned_and_kill_all_connections
• Ensure all functions are pure: take inputs as parameters, return decisions, no side effects
• Add the new source file to lib/Makefile's _OBJ_CXX list: MonitorHealthDecision.oo

Task 3: Integrate Extracted Functions into Production Code

Wire the new pure functions into the existing monitor callbacks to maintain identical behavior while enabling testability.

• In lib/MySQL_Monitor.cpp, refactor the ReadySet status block (~2860-2906) to call determine_server_status() and pass result to MyHGM->set_Readyset_status()
• In lib/MySQL_HostGroups_Manager.cpp, refactor replication_lag_action_inner() (~2700-2730) to use status determination from extracted function
• In lib/MySQL_Monitor.cpp ping failure handler (~3218), call should_shun_on_failures() to decide before calling shun_and_killall()
• In lib/MyHGC.cpp recovery loop (~76-110), call can_unshun_server() to decide before state transitions
• Preserve all existing behavior: same inputs produce same outcomes, just routed through extracted functions
• Add #include "MonitorHealthDecision.h" to files using the new functions

🤖 Prompt for AI agents

Extract three pure decision functions (`determine_server_status`,
`should_shun_on_failures`, `can_unshun_server`) from the existing monitor and
hostgroup manager code into a new standalone header and implementation file,
then wire them back into the production code paths.

**Create `include/MonitorHealthDecision.h`:**
- Add a proper include guard
- Include `include/proxysql_structs.h` for the `MySerStatus` enum
- Declare `determine_server_status()` with parameters for: current status, ping
success, read-only value, writer/reader context, replication lag value and
threshold, consecutive lag violation count and threshold, and Group Replication
`viable_candidate` and `read_only` flags
- Declare `should_shun_on_failures()` with parameters for: consecutive failure
count, max failures threshold, current connections count, and
`shunned_and_kill_all_connections` flag
- Declare `can_unshun_server()` with parameters for: time of last error, current
time, shun recovery time in seconds, `connect_timeout_server_max` (for capping),
`shunned_and_kill_all_connections` flag, connections used/free counts, and
`unshun_algorithm` value
- Add Doxygen documentation blocks for each function describing its decision
logic and return values

**Create `lib/MonitorHealthDecision.cpp`:**
- Implement `determine_server_status()` by extracting logic from:
- ReadySet status mapping (`MySQL_Monitor.cpp` ~2879-2887): Online→ONLINE,
Maintenance→OFFLINE_SOFT, else→SHUNNED
- Group Replication logic (~2107-2130): viable_candidate=NO→OFFLINE,
read_only=YES→reader, else→writer
- Replication lag threshold check (`MySQL_HostGroups_Manager.cpp` ~2710-2730):
lag > max with count check
- Implement `should_shun_on_failures()` by extracting logic from:
- Ping failure shunning (`MySQL_Monitor.cpp` ~3218-3222): consecutive failures
>= max → shun
- Connect error shunning (`MySrvC.cpp` ~110-120): errors_this_second >=
min(shun_on_failures, connect_retries+1)
- Implement `can_unshun_server()` by extracting logic from:
- `MyHGC.cpp` recovery loop (~83-105): compute max_wait_sec with connect_timeout
capping, check elapsed time, verify connection drain if
`shunned_and_kill_all_connections`
- All functions must be pure: accept inputs as parameters, return decisions, no
side effects or global state access
- Add `MonitorHealthDecision.oo` to `lib/Makefile`'s `_OBJ_CXX` list

**Integrate into production code:**
- In `lib/MySQL_Monitor.cpp`, replace the ReadySet status block (~2860-2906)
with a call to `determine_server_status()`, passing result to
`MyHGM->set_Readyset_status()`
- In `lib/MySQL_HostGroups_Manager.cpp`, replace the inline logic in
`replication_lag_action_inner()` (~2700-2730) with a call to
`determine_server_status()`
- In `lib/MySQL_Monitor.cpp` ping failure handler (~3218), replace the inline
shun condition with a call to `should_shun_on_failures()` before calling
`shun_and_killall()`
- In `lib/MyHGC.cpp` recovery loop (~76-110), replace the inline recovery check
with a call to `can_unshun_server()` before state transitions
- Add `#include "MonitorHealthDecision.h"` to each modified file
- Preserve all existing behavior: same inputs must produce identical outputs

Phase 3: Create Unit Tests

This phase creates comprehensive unit tests validating all decision branches of the extracted functions.

Task 1: Create Monitor Health Unit Test File

Write the test file following the unit-strip_schema_from_query-t.cpp pattern that links real production functions.

• Create test/tap/tests/unit/monitor_health_unit-t.cpp
• Include required headers: tap.h, unit_test.h (from parent), MonitorHealthDecision.h
• Structure tests into logical groups with diag() banners
• Call plan(N) with total expected assertions, return exit_status() from main
• Do NOT reimplement any logic—call the real functions from libproxysql.a

Task 2: Test determine_server_status() Function

Cover all status transition scenarios for the combined health determination logic.

• Test healthy server stays ONLINE (all checks pass)
• Test ping failure → SHUNNED
• Test read_only=YES for writer context → OFFLINE_SOFT
• Test read_only=NO for reader context → stays ONLINE
• Test replication lag exceeds threshold with sufficient consecutive count → SHUNNED_REPLICATION_LAG
• Test replication lag exceeds threshold but count below limit → stays ONLINE
• Test viable_candidate=NO → OFFLINE_SOFT
• Test ReadySet status mappings: "Online"→ONLINE, "Maintenance*"→OFFLINE_SOFT, other→SHUNNED
• Edge cases: zero thresholds, NULL lag values (-1, -2, -3 semantics), already-shunned servers

Task 3: Test should_shun_on_failures() Function

Cover heartbeat and connect failure shunning decisions.

• Test consecutive failures exactly equal to max → should shun
• Test consecutive failures one below max → should NOT shun
• Test failures exceed max → should shun
• Test single failure with high max threshold → should NOT shun
• Test zero current connections vs active connections (for kill_all scenarios)
• Test interaction with shunned_and_kill_all_connections flag
• Edge cases: max_failures=0, max_failures=1

Task 4: Test can_unshun_server() Function

Cover time-based recovery scenarios including algorithm variations.

• Test elapsed time exceeds recovery time → can unshun
• Test elapsed time below recovery time → cannot unshun
• Test recovery time capped by connect_timeout_server_max
• Test shunned_and_kill_all_connections=true with connections still active → cannot unshun
• Test shunned_and_kill_all_connections=true with all connections drained → can unshun
• Test unshun_algorithm=0 vs unshun_algorithm=1 behavior difference (single HG vs all HGs)
• Edge cases: time_last_error=0, current_time < time_last_error (clock skew), recovery_time_sec=0

Task 5: Register Test in Unit Makefile

Add the new test to the build system.

• Add monitor_health_unit-t to the UNIT_TESTS variable in test/tap/tests/unit/Makefile
• Ensure build rule links against $(LIBPROXYSQLAR) and includes proper library order
• Verify test compiles and runs with make monitor_health_unit-t && ./monitor_health_unit-t

🤖 Prompt for AI agents

Create the unit test file `test/tap/tests/unit/monitor_health_unit-t.cpp` and
register it in the build system, following the
`unit-strip_schema_from_query-t.cpp` pattern.

**Create `test/tap/tests/unit/monitor_health_unit-t.cpp`:**
- Include `tap.h`, `unit_test.h` (from parent directory), and
`MonitorHealthDecision.h`
- Call `plan(N)` at the start of `main()` with the total count of assertions,
and return `exit_status()` at the end
- Do NOT reimplement any logic — call the real functions linked from
`libproxysql.a`
- Organize tests into logical groups separated by `diag()` banners

**Tests for `determine_server_status()`:**
- Healthy server (all checks pass) → ONLINE
- Ping failure → SHUNNED
- read_only=YES in writer context → OFFLINE_SOFT
- read_only=NO in reader context → ONLINE
- Replication lag exceeds threshold with sufficient consecutive count →
SHUNNED_REPLICATION_LAG
- Replication lag exceeds threshold but count below limit → ONLINE
- viable_candidate=NO → OFFLINE_SOFT
- ReadySet mappings: "Online"→ONLINE, "Maintenance*"→OFFLINE_SOFT, other
string→SHUNNED
- Edge cases: zero thresholds, NULL/sentinel lag values (-1, -2, -3),
already-shunned server

**Tests for `should_shun_on_failures()`:**
- Consecutive failures exactly equal to max → shun
- Consecutive failures one below max → do NOT shun
- Failures exceed max → shun
- Single failure with high max threshold → do NOT shun
- Zero current connections vs active connections (kill_all scenarios)
- Interaction with `shunned_and_kill_all_connections` flag
- Edge cases: max_failures=0, max_failures=1

**Tests for `can_unshun_server()`:**
- Elapsed time exceeds recovery time → can unshun
- Elapsed time below recovery time → cannot unshun
- Recovery time capped by `connect_timeout_server_max`
- `shunned_and_kill_all_connections=true` with connections still active → cannot
unshun
- `shunned_and_kill_all_connections=true` with all connections drained → can
unshun
- `unshun_algorithm=0` vs `unshun_algorithm=1` behavior difference (single HG vs
all HGs)
- Edge cases: `time_last_error=0`, `current_time < time_last_error` (clock
skew), `recovery_time_sec=0`

**Register in build system:**
- Add `monitor_health_unit-t` to the `UNIT_TESTS` variable in
`test/tap/tests/unit/Makefile`
- Ensure the build rule links against `$(LIBPROXYSQLAR)` with proper library
order

Research

The ProxySQL monitor module in lib/MySQL_Monitor.cpp and lib/MySQL_HostGroups_Manager.cpp contains server health decision logic currently embedded in check handler callbacks. Key decision points include: ReadySet status determination (lines 2855-2906), Group Replication viable_candidate/read_only logic (lines 2107-2132, 3879-3941), heartbeat-based shunning via consecutive ping failures (lines 3182-3221), connect error shunning in lib/MySrvC.cpp (lines 45-131), replication lag shunning (lines 2687-2745), and time-based unshun recovery in lib/MyHGC.cpp (lines 76-140). The MySerStatus enum in include/proxysql_structs.h defines five states: ONLINE, SHUNNED, OFFLINE_SOFT, OFFLINE_HARD, and SHUNNED_REPLICATION_LAG. Existing unit tests live flat in test/tap/tests/ with two patterns: unit-strip_schema_from_query-t.cpp (links real production code via libproxysql.a using unit_test.h) and anomaly_detector_unit-t.cpp (standalone reimplementation). The ticket requires the former pattern.

---

🚀 Next Steps

🤖 All AI agent prompts combined

Task: 1

Set up the `test/tap/tests/unit/` directory and its build system so that unit
tests can be compiled and linked against `libproxysql.a` without a running
ProxySQL instance.

- Create the directory `test/tap/tests/unit/`
- Create `test/tap/tests/unit/Makefile`:
- Walk up the directory tree to locate `PROXYSQL_PATH`, then include
`makefiles_vars.mk` and `makefiles_paths.mk`
- Define `IDIRS`, `LDIRS`, and `OPT` by following the pattern from
`test/tap/tests/Makefile`
  - Define `LIBPROXYSQLAR := $(PROXYSQL_LDIR)/libproxysql.a`
  - Define a `UNIT_TESTS` variable that will list all unit test targets
- Add a pattern rule `%-t: %-t.cpp` that links against `-lproxysql` and other
required libraries
- Include `-Wl,--allow-multiple-definition` as a linker flag to handle ODR
issues from partial ProxySQL code linkage
- In the parent `test/tap/tests/Makefile`, add a `unit` target that descends
into the `unit/` subdirectory
- Review `test/tap/tests/unit_test.h` for existing stubs (`PROXYSQL_EXTERN`,
`EXCLUDE_TRACKING_VARIABLES`, `GloMyLdapAuth`) and determine whether additional
global stubs (e.g., `GloMyMon`, `MyHGM`) are needed for monitor code; if the
extracted functions are pure, no additional stubs should be required
- Add a comment in the test helper noting that `unit_test.h` from the parent
directory is reused
===============================================================================

Task: 2

Extract three pure decision functions (`determine_server_status`,
`should_shun_on_failures`, `can_unshun_server`) from the existing monitor and
hostgroup manager code into a new standalone header and implementation file,
then wire them back into the production code paths.

**Create `include/MonitorHealthDecision.h`:**
- Add a proper include guard
- Include `include/proxysql_structs.h` for the `MySerStatus` enum
- Declare `determine_server_status()` with parameters for: current status, ping
success, read-only value, writer/reader context, replication lag value and
threshold, consecutive lag violation count and threshold, and Group Replication
`viable_candidate` and `read_only` flags
- Declare `should_shun_on_failures()` with parameters for: consecutive failure
count, max failures threshold, current connections count, and
`shunned_and_kill_all_connections` flag
- Declare `can_unshun_server()` with parameters for: time of last error, current
time, shun recovery time in seconds, `connect_timeout_server_max` (for capping),
`shunned_and_kill_all_connections` flag, connections used/free counts, and
`unshun_algorithm` value
- Add Doxygen documentation blocks for each function describing its decision
logic and return values

**Create `lib/MonitorHealthDecision.cpp`:**
- Implement `determine_server_status()` by extracting logic from:
- ReadySet status mapping (`MySQL_Monitor.cpp` ~2879-2887): Online→ONLINE,
Maintenance→OFFLINE_SOFT, else→SHUNNED
- Group Replication logic (~2107-2130): viable_candidate=NO→OFFLINE,
read_only=YES→reader, else→writer
- Replication lag threshold check (`MySQL_HostGroups_Manager.cpp` ~2710-2730):
lag > max with count check
- Implement `should_shun_on_failures()` by extracting logic from:
- Ping failure shunning (`MySQL_Monitor.cpp` ~3218-3222): consecutive failures
>= max → shun
- Connect error shunning (`MySrvC.cpp` ~110-120): errors_this_second >=
min(shun_on_failures, connect_retries+1)
- Implement `can_unshun_server()` by extracting logic from:
- `MyHGC.cpp` recovery loop (~83-105): compute max_wait_sec with connect_timeout
capping, check elapsed time, verify connection drain if
`shunned_and_kill_all_connections`
- All functions must be pure: accept inputs as parameters, return decisions, no
side effects or global state access
- Add `MonitorHealthDecision.oo` to `lib/Makefile`'s `_OBJ_CXX` list

**Integrate into production code:**
- In `lib/MySQL_Monitor.cpp`, replace the ReadySet status block (~2860-2906)
with a call to `determine_server_status()`, passing result to
`MyHGM->set_Readyset_status()`
- In `lib/MySQL_HostGroups_Manager.cpp`, replace the inline logic in
`replication_lag_action_inner()` (~2700-2730) with a call to
`determine_server_status()`
- In `lib/MySQL_Monitor.cpp` ping failure handler (~3218), replace the inline
shun condition with a call to `should_shun_on_failures()` before calling
`shun_and_killall()`
- In `lib/MyHGC.cpp` recovery loop (~76-110), replace the inline recovery check
with a call to `can_unshun_server()` before state transitions
- Add `#include "MonitorHealthDecision.h"` to each modified file
- Preserve all existing behavior: same inputs must produce identical outputs
===============================================================================

Task: 3

Create the unit test file `test/tap/tests/unit/monitor_health_unit-t.cpp` and
register it in the build system, following the
`unit-strip_schema_from_query-t.cpp` pattern.

**Create `test/tap/tests/unit/monitor_health_unit-t.cpp`:**
- Include `tap.h`, `unit_test.h` (from parent directory), and
`MonitorHealthDecision.h`
- Call `plan(N)` at the start of `main()` with the total count of assertions,
and return `exit_status()` at the end
- Do NOT reimplement any logic — call the real functions linked from
`libproxysql.a`
- Organize tests into logical groups separated by `diag()` banners

**Tests for `determine_server_status()`:**
- Healthy server (all checks pass) → ONLINE
- Ping failure → SHUNNED
- read_only=YES in writer context → OFFLINE_SOFT
- read_only=NO in reader context → ONLINE
- Replication lag exceeds threshold with sufficient consecutive count →
SHUNNED_REPLICATION_LAG
- Replication lag exceeds threshold but count below limit → ONLINE
- viable_candidate=NO → OFFLINE_SOFT
- ReadySet mappings: "Online"→ONLINE, "Maintenance*"→OFFLINE_SOFT, other
string→SHUNNED
- Edge cases: zero thresholds, NULL/sentinel lag values (-1, -2, -3),
already-shunned server

**Tests for `should_shun_on_failures()`:**
- Consecutive failures exactly equal to max → shun
- Consecutive failures one below max → do NOT shun
- Failures exceed max → shun
- Single failure with high max threshold → do NOT shun
- Zero current connections vs active connections (kill_all scenarios)
- Interaction with `shunned_and_kill_all_connections` flag
- Edge cases: max_failures=0, max_failures=1

**Tests for `can_unshun_server()`:**
- Elapsed time exceeds recovery time → can unshun
- Elapsed time below recovery time → cannot unshun
- Recovery time capped by `connect_timeout_server_max`
- `shunned_and_kill_all_connections=true` with connections still active → cannot
unshun
- `shunned_and_kill_all_connections=true` with all connections drained → can
unshun
- `unshun_algorithm=0` vs `unshun_algorithm=1` behavior difference (single HG vs
all HGs)
- Edge cases: `time_last_error=0`, `current_time < time_last_error` (clock
skew), `recovery_time_sec=0`

**Register in build system:**
- Add `monitor_health_unit-t` to the `UNIT_TESTS` variable in
`test/tap/tests/unit/Makefile`
- Ensure the build rule links against `$(LIBPROXYSQLAR)` with proper library
order

💡 Iterate on the plan with: @coderabbitai <feedback>

Example Feedback
- `@coderabbitai` You can skip phase 3. Add a simple unit test case for phase 2.
- `@coderabbitai` For design choice 1 go ahead with option 3 and replan.

---

💬 Have feedback or questions? Drop into our discord!