From 7fd8eca8d9717d66005f05faa5136f7cc29ad0ad Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Tue, 28 May 2024 15:24:17 +0200
Subject: [PATCH] Manager: more gracefull handle SQLite "interrupted (9)" error

Wrap the SQLite error "interrupted (9)". That error is (as far as I
could figure out) caused by the context being closed. Unfortunately
there is no wrapping of the underlying context error, so it's not
possible to determine whether it was due to a 'deadline exceeded' error
or another cancellation cause (like upstream HTTP connection closing).

Primarily this makes a rather unreliable unit test properly reliable.
The code under test could return either `context.DeadlineExceeded` or
the "interrupted (9)" error (GORM + SQLite doesn't reliably chose one or
the other), and now this is cleanly tested for.
---
 internal/manager/persistence/errors.go       | 13 +++++++++++++
 internal/manager/persistence/workers_test.go | 14 +++++++++++++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/internal/manager/persistence/errors.go b/internal/manager/persistence/errors.go
index 816a1383..18360799 100644
--- a/internal/manager/persistence/errors.go
+++ b/internal/manager/persistence/errors.go
@@ -17,6 +17,13 @@ var (
 	ErrWorkerTagNotFound = PersistenceError{Message: "worker tag not found", Err: gorm.ErrRecordNotFound}
 
 	ErrDeletingWithoutFK = errors.New("refusing to delete a job when foreign keys are not enabled on the database")
+
+	// ErrContextCancelled wraps the SQLite error "interrupted (9)". That error is
+	// (as far as Sybren could figure out) caused by the context being closed.
+	// Unfortunately there is no wrapping of the context error, so it's not
+	// possible to determine whether it was due to a 'deadline exceeded' error or
+	// another cancellation cause (like upstream HTTP connection closing).
+	ErrContextCancelled = errors.New("context cancelled")
 )
 
 type PersistenceError struct {
@@ -57,6 +64,12 @@ func wrapError(errorToWrap error, message string, format ...interface{}) error {
 		formattedMsg = message
 	}
 
+	// Translate the SQLite "interrupted" error into something the error-handling
+	// code can check for.
+	if errorToWrap.Error() == "interrupted (9)" {
+		errorToWrap = ErrContextCancelled
+	}
+
 	return PersistenceError{
 		Message: formattedMsg,
 		Err:     errorToWrap,
diff --git a/internal/manager/persistence/workers_test.go b/internal/manager/persistence/workers_test.go
index cf37c688..53afd19d 100644
--- a/internal/manager/persistence/workers_test.go
+++ b/internal/manager/persistence/workers_test.go
@@ -5,6 +5,7 @@ package persistence
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -412,6 +413,17 @@ func TestSummarizeWorkerStatusesTimeout(t *testing.T) {
 
 	// Test the summary.
 	summary, err := f.db.SummarizeWorkerStatuses(subCtx)
-	assert.ErrorIs(t, err, context.DeadlineExceeded)
+
+	// Unfortunately, the exact error returned seems to be non-deterministic.
+	switch {
+	case errors.Is(err, context.DeadlineExceeded):
+		// Good!
+	case errors.Is(err, ErrContextCancelled):
+		// Also good!
+	case err == nil:
+		t.Fatal("no error returned where a timeout error was expected")
+	default:
+		t.Fatalf("unexpected error returned: %v", err)
+	}
 	assert.Nil(t, summary)
 }