admin: address review feedback on the maintenance scanner fix

weed/admin/dash/maintenance_policy_persistence_test.go

@@ -7,8 +7,7 @@ import (
 	"github.com/seaweedfs/seaweedfs/weed/worker/tasks/vacuum"
 )
 
-// TestLoadMaintenanceConfigHonoursPersistedTaskConfigs guards against the regression in
-// https://github.com/seaweedfs/seaweedfs/issues/10874: buildPolicyFromTaskConfigs used to call
+// TestLoadMaintenanceConfigHonoursPersistedTaskConfigs guards against a regression: buildPolicyFromTaskConfigs used to call
 // LoadConfigFromPersistence(nil), which can never satisfy the loaders' type assertion, so every
 // task silently fell back to its compiled-in defaults (Enabled: true) and a task disabled in the
 // admin UI kept being scheduled.

weed/admin/dash/maintenance_startup_test.go

@@ -25,7 +25,7 @@ func restoreGlobalTaskState(t *testing.T) {
 // 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
+// This is the whole reported bug 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

weed/admin/dash/task_policy_defaults_test.go

@@ -129,9 +129,15 @@ func TestEcBalanceTaskPolicyRoundTrip(t *testing.T) {
 		t.Errorf("ec_balance collection filter = %q, want the persisted %q", loaded.CollectionFilter, "pictures")
 	}
 
-	// The generic dispatcher the maintenance manager uses has to know the type too.
-	if err := cp.SaveTaskPolicy("ec_balance", saved.ToTaskPolicy()); err != nil {
-		t.Errorf("SaveTaskPolicy(ec_balance): %v", err)
+	// The generic dispatcher the maintenance manager uses has to know the type too, and
+	// has to write the same file the dedicated loader reads.
+	dispatched := NewConfigPersistence(t.TempDir())
+	if err := dispatched.SaveTaskPolicy("ec_balance", saved.ToTaskPolicy()); err != nil {
+		t.Fatalf("SaveTaskPolicy(ec_balance): %v", err)
+	}
+	roundTripped := ec_balance.LoadConfigFromPersistence(dispatched)
+	if roundTripped == nil || roundTripped.MinServerCount != 9 {
+		t.Errorf("SaveTaskPolicy(ec_balance) did not round-trip through the ec_balance store: %+v", roundTripped)
 	}
 }
 
@@ -164,8 +170,8 @@ func TestBuildPolicyKeepsTaskSpecificFields(t *testing.T) {
 	if got := ecPolicy.GetReplicaPlacement(); got != "020" {
 		t.Errorf("erasure coding replica placement = %q, want the persisted %q", got, "020")
 	}
-	if got := ecPolicy.GetPreferredTags(); len(got) != 2 {
-		t.Errorf("erasure coding preferred tags = %v, want the persisted 2 entries", got)
+	if got := ecPolicy.GetPreferredTags(); len(got) != 2 || got[0] != "ssd" || got[1] != "archive" {
+		t.Errorf("erasure coding preferred tags = %v, want the persisted [ssd archive]", got)
 	}
 
 	balancePolicy := policy.TaskPolicies["balance"].GetBalanceConfig()

weed/admin/maintenance/maintenance_manager.go

@@ -201,8 +201,7 @@ func (mm *MaintenanceManager) scanLoop() {
 	// configured interval instead never restores the normal cadence: once the errors stop,
 	// getScanInterval returns scanInterval again, the comparison comes out false, and the
 	// ticker is left at the backoff delay. A single transient scan failure therefore pinned
-	// the scanner to one scan per second forever - see issue #10874, where that produced a
-	// ~658 KB/s log flood and 193k orphaned task files.
+	// the scanner to one scan per second forever.
 	activeInterval := scanInterval
 	ticker := time.NewTicker(activeInterval)
 	// Wrapped in a closure so the replacement ticker is stopped, not the one that happened

weed/admin/maintenance/maintenance_policy_test.go

@@ -70,8 +70,7 @@ func disabledStub() *stubConfigPersistence {
 	}
 }
 
-// TestBuildPolicyFromTaskConfigsUsesPersistence covers the bug reported in
-// https://github.com/seaweedfs/seaweedfs/issues/10874: the persistence argument used to be a
+// TestBuildPolicyFromTaskConfigsUsesPersistence covers a reported bug: the persistence argument used to be a
 // literal nil, which no type assertion can satisfy, so a task disabled on disk came back enabled.
 func TestBuildPolicyFromTaskConfigsUsesPersistence(t *testing.T) {
 	policy := BuildPolicyFromTaskConfigs(disabledStub())

weed/admin/maintenance/maintenance_policy_wiring_test.go

@@ -38,7 +38,7 @@ func snapshotDetectorState(t *testing.T) {
 	})
 }
 
-// TestPolicyReachesRegisteredDetectors is the regression test for the half of issue #10874
+// TestPolicyReachesRegisteredDetectors is the regression test for the half of the scan-flood bug
 // that a corrected policy alone did not fix: MaintenanceIntegration pushes the policy into
 // detectors and schedulers through interface{ SetEnabled(bool) } and
 // interface{ SetMaxConcurrent(int) } type assertions, but every task is backed by

weed/admin/maintenance/maintenance_scan_cadence_test.go

@@ -39,8 +39,7 @@ func (c *scanCadenceClient) callCount() int {
 	return c.calls
 }
 
-// TestScanLoopRestoresIntervalAfterBackoff is the regression test for the scan flood in
-// https://github.com/seaweedfs/seaweedfs/issues/10874. The loop shortens its ticker to the
+// TestScanLoopRestoresIntervalAfterBackoff is the regression test for the scan flood. The loop shortens its ticker to the
 // error backoff delay after a failed scan. It used to compare the target interval against
 // the *configured* scan interval rather than against the interval the ticker was actually
 // running at, so once the errors stopped the comparison came out false and the ticker was
@@ -104,15 +103,16 @@ func TestScanLoopRestoresIntervalAfterBackoff(t *testing.T) {
 	// Observe a window that a 1s cadence would fill with scans and a 3s cadence would not.
 	const window = 5 * time.Second
 	time.Sleep(window)
-	scansInWindow := client.callCount() - before
+	callsInWindow := client.callCount() - before
 
-	// 3s cadence: at most 2 scans in 5s. 1s cadence: about 5.
-	if scansInWindow > 2 {
-		t.Errorf("scan loop ran %d scans in %v after recovering from a failed scan; "+
+	// One scan makes two master-client calls. 3s cadence: at most 2 scans (4 calls) in 5s,
+	// even if a stall shifts the window. 1s cadence: about 5 scans (10 calls).
+	if callsInWindow > 4 {
+		t.Errorf("scan loop made %d master-client calls in %v after recovering from a failed scan; "+
 			"the ticker was left at the %v backoff instead of returning to the configured %v",
-			scansInWindow, window, time.Second, baseInterval)
+			callsInWindow, window, time.Second, baseInterval)
 	}
-	if scansInWindow == 0 {
+	if callsInWindow == 0 {
 		t.Errorf("scan loop ran no scans in %v, expected the %v cadence to fire at least once", window, baseInterval)
 	}
 }

weed/worker/tasks/base/generic_components.go

@@ -49,7 +49,7 @@ func (d *GenericDetector) IsEnabled() bool {
 // MaintenanceIntegration.configureDetectorFromPolicy). Every registered task is
 // backed by this generic detector, so without this method that assertion failed
 // for every task and the policy never reached the flag that
-// ScanWithTaskDetectors actually gates on. See issue #10874.
+// ScanWithTaskDetectors actually gates on.
 func (d *GenericDetector) SetEnabled(enabled bool) {
 	d.taskDef.Config.SetEnabled(enabled)
 }
@@ -143,7 +143,7 @@ func (s *GenericScheduler) IsEnabled() bool {
 // SetEnabled turns scheduling for this task type on or off. Detector and scheduler
 // share one TaskDefinition, so this is the same flag GenericDetector.SetEnabled sets;
 // both setters exist because the maintenance integration configures the two
-// independently. See GenericDetector.SetEnabled and issue #10874.
+// independently. See GenericDetector.SetEnabled.
 func (s *GenericScheduler) SetEnabled(enabled bool) {
 	s.taskDef.Config.SetEnabled(enabled)
 }

weed/worker/tasks/vacuum/config_persistence_test.go

@@ -18,8 +18,8 @@ func (s *stubVacuumStore) LoadVacuumTaskPolicy() (*worker_pb.TaskPolicy, error)
 
 // wrongShapedStore is what the maintenance manager used to be handed: a non-nil value that
 // does not satisfy the accessor the loader asserts on. It has to keep falling back to the
-// defaults, but no longer silently - the reporter of issue #10874 had to read the source to
-// find out why their disabled task kept running.
+// defaults, but no longer silently: a fallback the operator cannot see reads as a disabled task
+// that keeps running.
 type wrongShapedStore struct{}
 
 func (wrongShapedStore) SomethingElse() {}