From 48f081e03e1429f0bef9804e93cc34bcb6299392 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Thu, 21 Jul 2022 12:49:32 +0200
Subject: [PATCH] Sleep Scheduler: don't overwrite `error` status from Worker

The Sleep Scheduler shouldn't push a Worker out of `error` status, as that
could hide problematic situations.
---
 .../manager/sleep_scheduler/sleep_scheduler.go    | 15 +++++++++++++++
 .../sleep_scheduler/sleep_scheduler_test.go       |  8 +++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/internal/manager/sleep_scheduler/sleep_scheduler.go b/internal/manager/sleep_scheduler/sleep_scheduler.go
index 05e58ad9..aee2891b 100644
--- a/internal/manager/sleep_scheduler/sleep_scheduler.go
+++ b/internal/manager/sleep_scheduler/sleep_scheduler.go
@@ -17,6 +17,11 @@ import (
 // Time period for checking the schedule of every worker.
 const checkInterval = 1 * time.Minute
 
+// skipWorkersInStatus has those worker statuses that should never be changed by the sleep scheduler.
+var skipWorkersInStatus = map[api.WorkerStatus]bool{
+	api.WorkerStatusError: true,
+}
+
 // SleepScheduler manages wake/sleep cycles of Workers.
 type SleepScheduler struct {
 	clock       clock.Clock
@@ -111,6 +116,10 @@ func (ss *SleepScheduler) ApplySleepSchedule(ctx context.Context, schedule *pers
 		worker = schedule.Worker
 	}
 
+	if !ss.mayUpdateWorker(worker) {
+		return nil
+	}
+
 	scheduled := ss.scheduledWorkerStatus(schedule)
 	if scheduled == "" ||
 		(worker.StatusRequested == scheduled && !worker.LazyStatusRequest) ||
@@ -208,3 +217,9 @@ func (ss *SleepScheduler) checkSchedule(ctx context.Context, schedule *persisten
 		return
 	}
 }
+
+// mayUpdateWorker determines whether the sleep scheduler is allowed to update this Worker.
+func (ss *SleepScheduler) mayUpdateWorker(worker *persistence.Worker) bool {
+	shouldSkip := skipWorkersInStatus[worker.Status]
+	return !shouldSkip
+}
diff --git a/internal/manager/sleep_scheduler/sleep_scheduler_test.go b/internal/manager/sleep_scheduler/sleep_scheduler_test.go
index 7f2d9503..166394e5 100644
--- a/internal/manager/sleep_scheduler/sleep_scheduler_test.go
+++ b/internal/manager/sleep_scheduler/sleep_scheduler_test.go
@@ -171,7 +171,7 @@ func TestApplySleepSchedule(t *testing.T) {
 	testForExpectedStatus(api.WorkerStatusAsleep)
 }
 
-func TestApplySleepScheduleAlreadyCorrectStatus(t *testing.T) {
+func TestApplySleepScheduleNoStatusChange(t *testing.T) {
 	ss, mocks, ctx := testFixtures(t)
 
 	worker := persistence.Worker{
@@ -219,6 +219,12 @@ func TestApplySleepScheduleAlreadyCorrectStatus(t *testing.T) {
 	worker.StatusRequested = api.WorkerStatusAsleep
 	worker.LazyStatusRequest = false
 	runTest()
+
+	// Current status is not the right one, but error state should not be overwrittne.
+	worker.Status = api.WorkerStatusError
+	worker.StatusRequested = ""
+	worker.LazyStatusRequest = false
+	runTest()
 }
 
 type TestMocks struct {