From 6777e895893928df5f528df7a72aca521a9560c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= <sybren@blender.org>
Date: Thu, 11 Jan 2024 17:15:43 +0100
Subject: [PATCH] Manager: refuse to delete job when foreign keys are disabled

Just as a safety measure, before deleting a job, check that foreign key
constraints are enabled. These are optional in SQLite, and the deletion
function assumes that they are on.
---
 internal/manager/persistence/errors.go    |  2 ++
 internal/manager/persistence/jobs.go      | 10 ++++++++++
 internal/manager/persistence/jobs_test.go | 18 ++++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/internal/manager/persistence/errors.go b/internal/manager/persistence/errors.go
index 0b7ebd6f..24eb3dae 100644
--- a/internal/manager/persistence/errors.go
+++ b/internal/manager/persistence/errors.go
@@ -13,6 +13,8 @@ var (
 	ErrTaskNotFound      = PersistenceError{Message: "task not found", Err: gorm.ErrRecordNotFound}
 	ErrWorkerNotFound    = PersistenceError{Message: "worker not found", Err: gorm.ErrRecordNotFound}
 	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")
 )
 
 type PersistenceError struct {
diff --git a/internal/manager/persistence/jobs.go b/internal/manager/persistence/jobs.go
index 3772b280..90c77ca6 100644
--- a/internal/manager/persistence/jobs.go
+++ b/internal/manager/persistence/jobs.go
@@ -8,6 +8,7 @@ import (
 	"database/sql/driver"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"math"
 	"time"
 
@@ -269,6 +270,15 @@ func (db *DB) FetchJob(ctx context.Context, jobUUID string) (*Job, error) {
 // DeleteJob deletes a job from the database.
 // The deletion cascades to its tasks and other job-related tables.
 func (db *DB) DeleteJob(ctx context.Context, jobUUID string) error {
+	// As a safety measure, refuse to delete jobs unless foreign key constraints are active.
+	fkEnabled, err := db.areForeignKeysEnabled()
+	if err != nil {
+		return fmt.Errorf("checking whether foreign keys are enabled: %w", err)
+	}
+	if !fkEnabled {
+		return ErrDeletingWithoutFK
+	}
+
 	tx := db.gormDB.WithContext(ctx).
 		Where("uuid = ?", jobUUID).
 		Delete(&Job{})
diff --git a/internal/manager/persistence/jobs_test.go b/internal/manager/persistence/jobs_test.go
index 2495715d..0e51aaad 100644
--- a/internal/manager/persistence/jobs_test.go
+++ b/internal/manager/persistence/jobs_test.go
@@ -155,6 +155,24 @@ func TestDeleteJob(t *testing.T) {
 		"all remaining tasks should belong to the other job")
 }
 
+func TestDeleteJobWithoutFK(t *testing.T) {
+	ctx, cancel, db := persistenceTestFixtures(t, 1*time.Second)
+	defer cancel()
+
+	authJob := createTestAuthoredJobWithTasks()
+	authJob.Name = "Job to delete"
+	persistAuthoredJob(t, ctx, db, authJob)
+
+	require.NoError(t, db.pragmaForeignKeys(false))
+
+	err := db.DeleteJob(ctx, authJob.JobID)
+	require.ErrorIs(t, err, ErrDeletingWithoutFK)
+
+	// Test the deletion did not happen.
+	_, err = db.FetchJob(ctx, authJob.JobID)
+	assert.NoError(t, err, "job should not have been deleted")
+}
+
 func TestRequestJobDeletion(t *testing.T) {
 	ctx, close, db, job1, authoredJob1 := jobTasksTestFixtures(t)
 	defer close()