fix(cluster): fix backup validation and state warning logic

[tanji]

• Remove early returns that prevented state warnings from being set
• Add nil check for cluster.master to prevent panics
• Fix state warning logic to properly track logical/physical backups
• Return true if either logical OR physical backup exists
• Ensure WARN0111/WARN0112 states are properly managed

[claude]

PR Review: Fix Backup Validation and State Warning Logic

Summary

This PR addresses critical bugs in the HasValidBackup() function that could cause panics and incorrect state management. The changes are focused and well-motivated.

Positive Changes ✅

1. Nil Safety Fix - The addition of cluster.master != nil check prevents potential panics when the master is not yet initialized. This is a critical bug fix.

   if cluster.master != nil {
       // Now safe to access cluster.master
   }

2. Logical Correctness - The change from early returns to accumulating state into logical and physical flags ensures that state warnings are always updated, regardless of which backup type is found. The old code's early return statements (return true after finding any backup) would skip the state warning management entirely.

3. Proper State Machine Management - The PR correctly ensures that:

   • WARN0111 (no logical backup) is only set when logical == false
   • WARN0112 (no physical backup) is only set when physical == false
   • Both warnings can be independently managed based on actual backup status

4. Correct Return Logic - The final return logical || physical properly indicates whether the cluster has at least one valid backup type.

Code Quality ✅

• Clear comments added (// Check backup server, // Check master (with nil safety), // Manage state warnings)
• Consistent indentation and structure
• Removed dead code and comments

Issues & Considerations ⚠️

1. No Test Coverage - This critical backup validation logic has no accompanying tests. Given this function:

   • Is called during the monitoring loop (cluster/cluster.go)
   • Controls important state warnings that impact operational decisions
   • Had bugs that caused panics

   Consider adding regression tests for:

   • Nil master scenario
   • Only logical backup exists
   • Only physical backup exists
   • Both backups exist
   • No backups exist

2. Backup Server Null Check - While the nil check for cluster.master was added, cluster.GetBackupServer() result is already safely nil-checked with if sv != nil. This is consistent and good.

3. State Management Race Condition - The state machine updates (SetState/DeleteState) are called every monitoring tick. If multiple ticks occur simultaneously, there could be redundant but harmless state updates. This is acceptable for the current architecture but worth noting.

Security Considerations ✅

• No new security vulnerabilities introduced
• Nil pointer dereference prevention improves robustness

Recommendation

APPROVE with suggestion to add tests.

The fix is correct and addresses real bugs. However, I recommend adding unit tests for HasValidBackup() to prevent regressions and ensure all backup state combinations are properly handled. Consider adding tests in cluster/cluster_has_test.go (create if needed).