From 59655ea770f667a579e7a85cf3afc7d8b33d239e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Tue, 27 Sep 2022 12:26:20 +0200
Subject: [PATCH] Manager: fix error in sleep scheduler when shutting down

When the Manager was shutting down while the sleep scheduler was running, it
could cause a null pointer dereference. This is now doubly solved:

- `worker.Identifier()` is now nil-safe, as in, `worker` can be `nil` and
  it will still return a sensible string.
- failure to apply the sleep schedule due to the context closing is not
  logged as error any more.
---
 internal/manager/persistence/workers.go       |  4 ++++
 .../sleep_scheduler/sleep_scheduler.go        | 15 ++++++++++++---
 .../sleep_scheduler/sleep_scheduler_test.go   | 19 +++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/internal/manager/persistence/workers.go b/internal/manager/persistence/workers.go
index eb20c243..2f44632b 100644
--- a/internal/manager/persistence/workers.go
+++ b/internal/manager/persistence/workers.go
@@ -33,6 +33,10 @@ type Worker struct {
 }
 
 func (w *Worker) Identifier() string {
+	// Avoid a panic when worker.Identifier() is called on a nil pointer.
+	if w == nil {
+		return "-nil worker-"
+	}
 	return fmt.Sprintf("%s (%s)", w.Name, w.UUID)
 }
 
diff --git a/internal/manager/sleep_scheduler/sleep_scheduler.go b/internal/manager/sleep_scheduler/sleep_scheduler.go
index aee2891b..efd3228d 100644
--- a/internal/manager/sleep_scheduler/sleep_scheduler.go
+++ b/internal/manager/sleep_scheduler/sleep_scheduler.go
@@ -4,6 +4,7 @@ package sleep_scheduler
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -200,7 +201,12 @@ func (ss *SleepScheduler) CheckSchedules(ctx context.Context) {
 func (ss *SleepScheduler) checkSchedule(ctx context.Context, schedule *persistence.SleepSchedule) {
 	// Compute the next time to check.
 	schedule.NextCheck = ss.calculateNextCheck(schedule)
-	if err := ss.persist.SetWorkerSleepScheduleNextCheck(ctx, schedule); err != nil {
+	err := ss.persist.SetWorkerSleepScheduleNextCheck(ctx, schedule)
+	switch {
+	case errors.Is(ctx.Err(), context.Canceled):
+		// Manager is shutting down, this is fine.
+		return
+	case err != nil:
 		log.Error().
 			Err(err).
 			Str("worker", schedule.Worker.Identifier()).
@@ -209,12 +215,15 @@ func (ss *SleepScheduler) checkSchedule(ctx context.Context, schedule *persisten
 	}
 
 	// Apply the schedule to the worker.
-	if err := ss.ApplySleepSchedule(ctx, schedule); err != nil {
+	err = ss.ApplySleepSchedule(ctx, schedule)
+	switch {
+	case errors.Is(ctx.Err(), context.Canceled):
+		// Manager is shutting down, this is fine.
+	case err != nil:
 		log.Error().
 			Err(err).
 			Str("worker", schedule.Worker.Identifier()).
 			Msg("sleep scheduler: error applying worker's sleep schedule")
-		return
 	}
 }
 
diff --git a/internal/manager/sleep_scheduler/sleep_scheduler_test.go b/internal/manager/sleep_scheduler/sleep_scheduler_test.go
index 166394e5..65dfe3d8 100644
--- a/internal/manager/sleep_scheduler/sleep_scheduler_test.go
+++ b/internal/manager/sleep_scheduler/sleep_scheduler_test.go
@@ -95,6 +95,25 @@ func TestSetScheduleSwappedStartEnd(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+// Test that a sleep check that happens at shutdown of the Manager doesn't cause any panics.
+func TestCheckSleepScheduleAtShutdown(t *testing.T) {
+	ss, mocks, _ := testFixtures(t)
+
+	sched := persistence.SleepSchedule{
+		IsActive:   true,
+		DaysOfWeek: "mo tu we",
+		StartTime:  mkToD(18, 0),
+		EndTime:    mkToD(9, 0),
+		Worker:     nil,
+	}
+
+	// Cancel the context to mimick the Manager shutting down.
+	ctx, cancel := context.WithCancel(context.Background())
+	cancel()
+	mocks.persist.EXPECT().SetWorkerSleepScheduleNextCheck(ctx, &sched).Return(context.Canceled)
+	ss.checkSchedule(ctx, &sched)
+}
+
 func TestApplySleepSchedule(t *testing.T) {
 	ss, mocks, ctx := testFixtures(t)