admin: stop GetTaskPolicy panicking on a maintenance policy that is nil

GetTaskPolicy dereferenced its MaintenancePolicy argument to look at
TaskPolicies, so IsTaskEnabled, GetMaxConcurrent and GetRepeatInterval
all took the admin process down when handed a nil policy. A nil policy is
not a programming error here: MaintenanceConfig.Policy is unset until
something builds one, DefaultMaintenanceConfig returns a config with no
policy at all, and UpdateConfig installs whatever config it is given.
Found by calling IsTaskEnabled with the policy from a freshly defaulted
MaintenanceConfig.

Treat a nil policy as "no entry": no task enabled, the safe concurrency
default of 1, and a repeat interval of 0 so callers fall back to their
own default instead of reading DefaultRepeatIntervalSeconds off nil.

Also add the startup test this was found with. It walks the admin
server's startup sequence over a data directory that has balance saved as
disabled and checks the state that decides whether issue #10874 happens:
the balance detector reports disabled, vacuum stays enabled, and tasks
whose config was never saved keep their compiled-in default.

Refs #10874

weed/admin/dash/maintenance_startup_test.go

@@ -0,0 +1,123 @@
+package dash
+
+import (
+	"testing"
+
+	"github.com/seaweedfs/seaweedfs/weed/admin/maintenance"
+	"github.com/seaweedfs/seaweedfs/weed/worker/tasks"
+	"github.com/seaweedfs/seaweedfs/weed/worker/tasks/balance"
+	"github.com/seaweedfs/seaweedfs/weed/worker/tasks/vacuum"
+	"github.com/seaweedfs/seaweedfs/weed/worker/types"
+)
+
+// The task definitions these tests configure are process-global, so put them back the way a
+// fresh process would have them. Passing no config store makes every task fall back to its
+// own NewDefaultConfig, which is exactly the state package init left them in.
+func restoreGlobalTaskState(t *testing.T) {
+	t.Helper()
+
+	t.Cleanup(func() {
+		tasks.GetGlobalConfigUpdateRegistry().UpdateAllConfigs(nil)
+	})
+}
+
+// TestDisabledTaskIsNotScannedAfterStartup walks the admin server's startup sequence over a
+// data directory that has a disabled balance task saved in it, and checks the end state that
+// actually matters: the balance detector reports disabled, so ScanWithTaskDetectors skips it.
+//
+// This is the whole of issue #10874 in one test. The reporter disabled balance, and the
+// scanner kept detecting balance tasks, cancelling them and re-detecting them. Two separate
+// defects had to line up for the disabled flag to survive to here: the policy had to be built
+// from the persisted configs rather than from a nil store, and the policy had to reach
+// detector.IsEnabled() rather than dying in a failed type assertion.
+func TestDisabledTaskIsNotScannedAfterStartup(t *testing.T) {
+	restoreGlobalTaskState(t)
+
+	dir := t.TempDir()
+	cp := NewConfigPersistence(dir)
+
+	// What the admin writes when a user turns balance off, and leaves vacuum on.
+	disabledBalance := balance.NewDefaultConfig()
+	disabledBalance.Enabled = false
+	if err := cp.SaveBalanceTaskPolicy(disabledBalance.ToTaskPolicy()); err != nil {
+		t.Fatalf("save balance policy: %v", err)
+	}
+
+	enabledVacuum := vacuum.NewDefaultConfig()
+	enabledVacuum.Enabled = true
+	if err := cp.SaveVacuumTaskPolicy(enabledVacuum.ToTaskPolicy()); err != nil {
+		t.Fatalf("save vacuum policy: %v", err)
+	}
+
+	// The admin server's startup sequence, in order:
+	// loadTaskConfigurationsFromPersistence, then InitMaintenanceManager.
+	tasks.GetGlobalConfigUpdateRegistry().UpdateAllConfigs(cp)
+
+	maintenanceConfig, err := cp.LoadMaintenanceConfig()
+	if err != nil {
+		t.Fatalf("load maintenance config: %v", err)
+	}
+	manager := maintenance.NewMaintenanceManager(nil, maintenanceConfig, cp)
+	if manager == nil {
+		t.Fatal("NewMaintenanceManager returned nil")
+	}
+
+	registry := tasks.GetGlobalTypesRegistry()
+
+	balanceDetector := registry.GetDetector(types.TaskTypeBalance)
+	if balanceDetector == nil {
+		t.Fatal("no balance detector registered")
+	}
+	if balanceDetector.IsEnabled() {
+		t.Error("balance detector reports enabled after startup over a data directory where " +
+			"balance is saved as disabled; the scanner will keep detecting and cancelling balance tasks")
+	}
+
+	vacuumDetector := registry.GetDetector(types.TaskTypeVacuum)
+	if vacuumDetector == nil {
+		t.Fatal("no vacuum detector registered")
+	}
+	if !vacuumDetector.IsEnabled() {
+		t.Error("vacuum detector reports disabled although vacuum is saved as enabled; " +
+			"the fix must not switch off tasks the user left on")
+	}
+
+	// Tasks the user never touched keep their compiled-in default of enabled rather than
+	// being switched off by a policy entry built from a config that was never saved.
+	for _, taskType := range []types.TaskType{types.TaskTypeErasureCoding, types.TaskTypeECBalance} {
+		detector := registry.GetDetector(taskType)
+		if detector == nil {
+			t.Fatalf("no %s detector registered", taskType)
+		}
+		if !detector.IsEnabled() {
+			t.Errorf("%s detector reports disabled although its config was never saved", taskType)
+		}
+	}
+}
+
+// TestPolicyMirrorsWhatTheDetectorsReport checks that the maintenance policy the queue and
+// the scanner run on agrees with the detectors. A disagreement means one of the two paths
+// into the task configs has gone stale again.
+func TestPolicyMirrorsWhatTheDetectorsReport(t *testing.T) {
+	restoreGlobalTaskState(t)
+
+	dir := t.TempDir()
+	cp := NewConfigPersistence(dir)
+
+	disabledBalance := balance.NewDefaultConfig()
+	disabledBalance.Enabled = false
+	if err := cp.SaveBalanceTaskPolicy(disabledBalance.ToTaskPolicy()); err != nil {
+		t.Fatalf("save balance policy: %v", err)
+	}
+
+	tasks.GetGlobalConfigUpdateRegistry().UpdateAllConfigs(cp)
+	policy := cp.buildPolicyFromTaskConfigs()
+
+	for taskType, detector := range tasks.GetGlobalTypesRegistry().GetAllDetectors() {
+		policyEnabled := maintenance.IsTaskEnabled(policy, maintenance.MaintenanceTaskType(taskType))
+		if policyEnabled != detector.IsEnabled() {
+			t.Errorf("%s: policy says enabled=%v but the detector says enabled=%v",
+				taskType, policyEnabled, detector.IsEnabled())
+		}
+	}
+}

weed/admin/maintenance/maintenance_policy_wiring_test.go

@@ -177,3 +177,33 @@ func policyWithAllTasks(t *testing.T, enabled bool) *MaintenancePolicy {
 	}
 	return policy
 }
+
+// TestPolicyHelpersTolerateNilPolicy pins the nil handling in the exported policy helpers.
+// MaintenanceConfig.Policy is nil until something builds one, and UpdateConfig accepts a
+// config that has none, so these are reachable with a nil policy - GetTaskPolicy used to
+// dereference it and take the admin process down.
+func TestPolicyHelpersTolerateNilPolicy(t *testing.T) {
+	const taskType = MaintenanceTaskType("balance")
+
+	if got := GetTaskPolicy(nil, taskType); got != nil {
+		t.Errorf("GetTaskPolicy(nil) = %v, want nil", got)
+	}
+	if IsTaskEnabled(nil, taskType) {
+		t.Error("IsTaskEnabled(nil) = true, want false")
+	}
+	if got := GetMaxConcurrent(nil, taskType); got != 1 {
+		t.Errorf("GetMaxConcurrent(nil) = %d, want the safe default 1", got)
+	}
+	if got := GetRepeatInterval(nil, taskType); got != 0 {
+		t.Errorf("GetRepeatInterval(nil) = %d, want 0 so callers fall back to their own default", got)
+	}
+
+	// A policy that exists but lists nothing must behave the same way.
+	empty := &MaintenancePolicy{}
+	if got := GetTaskPolicy(empty, taskType); got != nil {
+		t.Errorf("GetTaskPolicy(empty) = %v, want nil", got)
+	}
+	if IsTaskEnabled(empty, taskType) {
+		t.Error("IsTaskEnabled(empty) = true, want false")
+	}
+}

weed/admin/maintenance/maintenance_types.go

@@ -144,9 +144,14 @@ func DefaultMaintenanceConfig() *MaintenanceConfig {
 
 // Policy helper functions (since we can't add methods to type aliases)
 
-// GetTaskPolicy returns the policy for a specific task type
+// GetTaskPolicy returns the policy for a specific task type, or nil when the maintenance
+// policy has no entry for it.
+//
+// A nil maintenance policy is a legitimate state, not a programming error:
+// MaintenanceConfig.Policy is unset until something builds one, and UpdateConfig accepts a
+// config that carries none. Dereferencing it here panicked the whole admin process.
 func GetTaskPolicy(mp *MaintenancePolicy, taskType MaintenanceTaskType) *TaskPolicy {
-	if mp.TaskPolicies == nil {
+	if mp == nil || mp.TaskPolicies == nil {
 		return nil
 	}
 	return mp.TaskPolicies[string(taskType)]
@@ -174,6 +179,9 @@ func GetMaxConcurrent(mp *MaintenancePolicy, taskType MaintenanceTaskType) int {
 func GetRepeatInterval(mp *MaintenancePolicy, taskType MaintenanceTaskType) int {
 	policy := GetTaskPolicy(mp, taskType)
 	if policy == nil {
+		if mp == nil {
+			return 0
+		}
 		return int(mp.DefaultRepeatIntervalSeconds)
 	}
 	return int(policy.RepeatIntervalSeconds)