Webapp: handle job deletions properly

- Add a little confirmation overlay before deleting a job. This overlay
  also shows information about whether the Shaman checkout directory
  will be deleted or not.
- Send job updates to the web frontend when jobs are marked for
  deletion, and when they are actually deleted.
- Respond to those updates, and handle some corner cases where job info
  is missing (because it just got deleted).

This closes T99401.

CHANGELOG.md

@@ -15,6 +15,8 @@ bugs in actually-released versions.
 - Bump the bundled Blender Asset Tracer (BAT) to version 1.15.
 - Increase preview image file size from 10 MB to 25 MB. Even though the Worker can down-scale render output before sending to the Manager as preview, they could still be larger than the limit of 10 MB.
 - Fix a crash of the Manager when using an invalid frame range (`1 10` for example, instead of `1-10` or `1,10`)
+- Make it possible to delete jobs. The job and its tasks are removed from Flamenco, including last-rendered images and logs. The input files (i.e. the to-be-rendered blend files and their dependencies) will only be removed if [the Shaman system](https://flamenco.blender.org/usage/shared-storage/shaman/) was used AND if the job was submitted with Flamenco 3.2 or newer.
+
 
 
 ## 3.1 - released 2022-10-18

internal/manager/api_impl/interfaces.go

@@ -223,6 +223,7 @@ var _ WorkerSleepScheduler = (*sleep_scheduler.SleepScheduler)(nil)
 
 type JobDeleter interface {
 	QueueJobDeletion(ctx context.Context, job *persistence.Job) error
+	WhatWouldBeDeleted(job *persistence.Job) api.JobDeletionInfo
 }
 
 var _ JobDeleter = (*job_deleter.Service)(nil)

internal/manager/api_impl/jobs.go

@@ -194,6 +194,26 @@ func (f *Flamenco) DeleteJob(e echo.Context, jobID string) error {
 	}
 }
 
+func (f *Flamenco) DeleteJobWhatWouldItDo(e echo.Context, jobID string) error {
+	logger := requestLogger(e).With().
+		Str("job", jobID).
+		Logger()
+
+	dbJob, err := f.fetchJob(e, logger, jobID)
+	if dbJob == nil {
+		// f.fetchJob already sent a response.
+		return err
+	}
+
+	logger = logger.With().
+		Str("currentstatus", string(dbJob.Status)).
+		Logger()
+	logger.Info().Msg("checking what job deletion would do")
+
+	deletionInfo := f.jobDeleter.WhatWouldBeDeleted(dbJob)
+	return e.JSON(http.StatusOK, deletionInfo)
+}
+
 // SetJobStatus is used by the web interface to change a job's status.
 func (f *Flamenco) SetJobStatus(e echo.Context, jobID string) error {
 	logger := requestLogger(e).With().

internal/manager/job_deleter/job_deleter.go

@@ -17,6 +17,8 @@ import (
 	"time"
 
 	"git.blender.org/flamenco/internal/manager/persistence"
+	"git.blender.org/flamenco/internal/manager/webupdates"
+	"git.blender.org/flamenco/pkg/api"
 	"git.blender.org/flamenco/pkg/shaman"
 	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/log"
@@ -67,6 +69,10 @@ func (s *Service) QueueJobDeletion(ctx context.Context, job *persistence.Job) er
 		return fmt.Errorf("requesting job deletion: %w", err)
 	}
 
+	// Broadcast that this job was queued for deleted.
+	jobUpdate := webupdates.NewJobUpdate(job)
+	s.changeBroadcaster.BroadcastJobUpdate(jobUpdate)
+
 	// Let the Run() goroutine know this job is ready for deletion.
 	select {
 	case s.queue <- job.UUID:
@@ -77,6 +83,15 @@ func (s *Service) QueueJobDeletion(ctx context.Context, job *persistence.Job) er
 	return nil
 }
 
+func (s *Service) WhatWouldBeDeleted(job *persistence.Job) api.JobDeletionInfo {
+	logger := log.With().Str("job", job.UUID).Logger()
+	logger.Info().Msg("job deleter: checking what job deletion would do")
+
+	return api.JobDeletionInfo{
+		ShamanCheckout: s.canDeleteShamanCheckout(logger, job),
+	}
+}
+
 // Run processes the queue of deletion requests. It starts by building up a
 // queue of still-pending job deletions.
 func (s *Service) Run(ctx context.Context) {
@@ -142,13 +157,39 @@ func (s *Service) deleteJob(ctx context.Context, jobUUID string) error {
 		return err
 	}
 
-	// TODO: broadcast that this job was deleted.
+	// Broadcast that this job was deleted. This only contains the UUID and the
+	// "was deleted" flag, because there's nothing else left. And I don't want to
+	// do a full database query for something we'll delete anyway.
+	wasDeleted := true
+	jobUpdate := api.SocketIOJobUpdate{
+		Id:         jobUUID,
+		WasDeleted: &wasDeleted,
+	}
+	s.changeBroadcaster.BroadcastJobUpdate(jobUpdate)
 
 	logger.Info().Msg("job deleter: job removal complete")
 	return nil
 }
 
+func (s *Service) canDeleteShamanCheckout(logger zerolog.Logger, job *persistence.Job) bool {
+	// NOTE: Keep this logic and the deleteShamanCheckout() function in sync.
+	if !s.shaman.IsEnabled() {
+		logger.Debug().Msg("job deleter: Shaman not enabled, cannot delete job files")
+		return false
+	}
+
+	checkoutID := job.Storage.ShamanCheckoutID
+	if checkoutID == "" {
+		logger.Debug().Msg("job deleter: job was not created with Shaman (or before Flamenco v3.2), cannot delete job files")
+		return false
+	}
+
+	return true
+}
+
 func (s *Service) deleteShamanCheckout(ctx context.Context, logger zerolog.Logger, jobUUID string) error {
+	// NOTE: Keep this logic and the canDeleteShamanCheckout() function in sync.
+
 	if !s.shaman.IsEnabled() {
 		logger.Debug().Msg("job deleter: Shaman not enabled, skipping job file deletion")
 		return nil

internal/manager/job_deleter/job_deleter_test.go

@@ -28,6 +28,8 @@ func TestQueueJobDeletion(t *testing.T) {
 	s, finish, mocks := jobDeleterTestFixtures(t)
 	defer finish()
 
+	mocks.broadcaster.EXPECT().BroadcastJobUpdate(gomock.Any()).Times(3)
+
 	job1 := &persistence.Job{UUID: "2f7d910f-08a6-4b0f-8ecb-b3946939ed1b"}
 	mocks.persist.EXPECT().RequestJobDeletion(mocks.ctx, job1)
 	assert.NoError(t, s.QueueJobDeletion(mocks.ctx, job1))
@@ -107,7 +109,7 @@ func TestDeleteJobWithoutShaman(t *testing.T) {
 	// Mock that everything went OK.
 	mocks.storage.EXPECT().RemoveJobStorage(mocks.ctx, jobUUID)
 	mocks.persist.EXPECT().DeleteJob(mocks.ctx, jobUUID)
-	// TODO: mocks.broadcaster.EXPECT().BroadcastJobUpdate(...)
+	mocks.broadcaster.EXPECT().BroadcastJobUpdate(gomock.Any())
 	assert.NoError(t, s.deleteJob(mocks.ctx, jobUUID))
 }
 
@@ -158,7 +160,7 @@ func TestDeleteJobWithShaman(t *testing.T) {
 	mocks.shaman.EXPECT().EraseCheckout(shamanCheckoutID)
 	mocks.storage.EXPECT().RemoveJobStorage(mocks.ctx, jobUUID)
 	mocks.persist.EXPECT().DeleteJob(mocks.ctx, jobUUID)
-	// TODO: mocks.broadcaster.EXPECT().BroadcastJobUpdate(...)
+	mocks.broadcaster.EXPECT().BroadcastJobUpdate(gomock.Any())
 	assert.NoError(t, s.deleteJob(mocks.ctx, jobUUID))
 }
 

internal/manager/persistence/jobs.go

@@ -216,6 +216,9 @@ func (db *DB) FetchJob(ctx context.Context, jobUUID string) (*Job, error) {
 	if findResult.Error != nil {
 		return nil, jobError(findResult.Error, "fetching job")
 	}
+	if dbJob.ID == 0 {
+		return nil, ErrJobNotFound
+	}
 
 	return &dbJob, nil
 }

internal/manager/webupdates/job_updates.go

@@ -21,6 +21,11 @@ func NewJobUpdate(job *persistence.Job) api.SocketIOJobUpdate {
 		Type:     job.JobType,
 		Priority: job.Priority,
 	}
+
+	if job.DeleteRequestedAt.Valid {
+		jobUpdate.DeleteRequestedAt = &job.DeleteRequestedAt.Time
+	}
+
 	return jobUpdate
 }
 

web/app/src/components/jobs/JobActionsBar.vue

@@ -1,25 +1,52 @@
 <template>
   <div class="btn-bar jobs">
+    <div class="btn-bar-popover" v-if="deleteInfo != null">
+      <p v-if="deleteInfo.shaman_checkout">Delete job, including Shaman checkout?</p>
+      <p v-else>Delete job? The job files will be kept.</p>
+      <div class="inner-btn-bar">
+        <button class="btn cancel" v-on:click="_hideDeleteJobPopup">Cancel</button>
+        <button class="btn delete dangerous" v-on:click="onButtonDeleteConfirmed">Delete</button>
+      </div>
+    </div>
     <button class="btn cancel" :disabled="!jobs.canCancel" v-on:click="onButtonCancel">Cancel Job</button>
     <button class="btn requeue" :disabled="!jobs.canRequeue" v-on:click="onButtonRequeue">Requeue</button>
-    <button class="action delete dangerous" :disabled="!jobs.canDelete" v-on:click="onButtonDelete">Delete</button>
+    <button class="action delete dangerous" title="Mark this job for deletion, after asking for a confirmation."
+      :disabled="!jobs.canDelete" v-on:click="onButtonDelete">Delete...</button>
   </div>
 </template>
 
 <script>
 import { useJobs } from '@/stores/jobs';
 import { useNotifs } from '@/stores/notifications';
+import { apiClient } from '@/stores/api-query-count';
+import { JobsApi } from '@/manager-api';
+import { JobDeletionInfo } from '@/manager-api';
+
 
 export default {
   name: "JobActionsBar",
+  props: [
+    "activeJobID",
+  ],
   data: () => ({
     jobs: useJobs(),
     notifs: useNotifs(),
+    jobsAPI: new JobsApi(apiClient),
+
+    deleteInfo: null,
   }),
   computed: {
   },
+  watch: {
+    activeJobID() {
+      this._hideDeleteJobPopup();
+    },
+  },
   methods: {
     onButtonDelete() {
+      this._startJobDeletionFlow();
+    },
+    onButtonDeleteConfirmed() {
       return this.jobs.deleteJobs()
         .then(() => {
           this.notifs.add("job marked for deletion");
@@ -28,6 +55,8 @@ export default {
           const errorMsg = JSON.stringify(error); // TODO: handle API errors better.
           this.notifs.add(`Error: ${errorMsg}`);
         })
+        .finally(this._hideDeleteJobPopup)
+        ;
     },
     onButtonCancel() {
       return this._handleJobActionPromise(
@@ -46,18 +75,64 @@ export default {
           // it's no longer necessary.
           // This function is still kept, in case we want to bring back the
           // notifications when multiple jobs can be selected. Then a summary
-          // ("N jobs requeued") could be logged here.
+          // ("N jobs requeued") could be logged here.btn-bar-popover
         })
+    },
+
+    _startJobDeletionFlow() {
+      if (!this.activeJobID) {
+        this.notifs.add("No active job, unable to delete anything");
+        return;
+      }
+
+      this.jobsAPI.deleteJobWhatWouldItDo(this.activeJobID)
+        .then(this._showDeleteJobPopup)
         .catch((error) => {
           const errorMsg = JSON.stringify(error); // TODO: handle API errors better.
           this.notifs.add(`Error: ${errorMsg}`);
         })
+        ;
     },
+
+    /**
+     * @param { JobDeletionInfo } deleteInfo
+     */
+    _showDeleteJobPopup(deleteInfo) {
+      console.log("deleteInfo", deleteInfo);
+      this.deleteInfo = deleteInfo;
+    },
+
+    _hideDeleteJobPopup() {
+      this.deleteInfo = null;
+    }
   }
 }
 
 </script>
 
 <style scoped>
+.btn-bar-popover {
+  align-items: center;
+  background-color: var(--color-background-popover);
+  border-radius: var(--border-radius);
+  border: var(--border-color) var(--border-width);
+  color: var(--color-text);
+  display: flex;
+  height: 3.5em;
+  left: 0;
+  margin: 0;
+  padding: 1rem 1rem;
+  position: absolute;
+  right: 0;
+  top: 0;
+  z-index: 1000;
+}
 
+.btn-bar-popover p {
+  flex-grow: 1;
+}
+
+.btn-bar-popover .inner-btn-bar {
+  flex-grow: 0;
+}
 </style>

web/app/src/components/jobs/JobDetails.vue

@@ -39,10 +39,12 @@
           <dd class="field-status-label" :class="'status-' + jobData.status">{{ jobData.status }}</dd>
 
           <dt class="field-type" title="Type">Type</dt>
-          <dd>{{ jobType ? jobType.label : jobData.type }}</dd>
+          <dd>{{ jobType? jobType.label : jobData.type }}</dd>
 
           <dt class="field-priority" title="Priority">Priority</dt>
-          <dd><PopoverEditableJobPriority :jobId="jobData.id" :priority="jobData.priority" /></dd>
+          <dd>
+            <PopoverEditableJobPriority :jobId="jobData.id" :priority="jobData.priority" />
+          </dd>
 
           <dt class="field-created" title="Created">Created</dt>
           <dd>{{ datetime.relativeTime(jobData.created) }}</dd>
@@ -140,6 +142,8 @@ export default {
   },
   watch: {
     jobData(newJobData) {
+      // This can be called when moving from "a job" to "no job", in which case there is no ID.
+      if (!newJobData || !newJobData.id) return;
       this._refreshJobSettings(newJobData);
     },
   },

web/app/src/components/jobs/JobsTable.vue

@@ -1,7 +1,7 @@
 <template>
   <h2 class="column-title">Jobs</h2>
   <div class="btn-bar-group">
-    <job-actions-bar />
+    <job-actions-bar :activeJobID="jobs.activeJobID" />
     <div class="align-right">
       <status-filter-bar :availableStatuses="availableStatuses" :activeStatuses="shownStatuses"
         @click="toggleStatusFilter" />
@@ -26,7 +26,7 @@ import StatusFilterBar from '@/components/StatusFilterBar.vue'
 export default {
   name: 'JobsTable',
   props: ["activeJobID"],
-  emits: ["tableRowClicked"],
+  emits: ["tableRowClicked", "activeJobDeleted"],
   components: {
     JobActionsBar, StatusFilterBar,
   },
@@ -151,18 +151,27 @@ export default {
     processJobUpdate(jobUpdate) {
       // updateData() will only overwrite properties that are actually set on
       // jobUpdate, and leave the rest as-is.
-      if (this.tabulator.initialized) {
+      if (!this.tabulator.initialized) {
+        return;
+      }
       const row = this.tabulator.rowManager.findRow(jobUpdate.id);
 
       let promise = null;
+      if (jobUpdate.was_deleted) {
+        if (row) promise = row.delete();
+        else promise = Promise.resolve();
+        promise.finally(() => { this.$emit("activeJobDeleted", jobUpdate.id); });
+      }
+      else {
         if (row) promise = this.tabulator.updateData([jobUpdate]);
         else promise = this.tabulator.addData([jobUpdate]);
+      }
 
       promise
         .then(this.sortData)
         .then(() => { this.tabulator.redraw(); }) // Resize columns based on new data.
-      }
+        .then(this._refreshAvailableStatuses)
-      this._refreshAvailableStatuses();
+        ;
     },
 
     onRowClick(event, row) {

web/app/src/views/JobsView.vue

@@ -1,6 +1,7 @@
 <template>
   <div class="col col-1">
-    <jobs-table ref="jobsTable" :activeJobID="jobID" @tableRowClicked="onTableJobClicked" />
+    <jobs-table ref="jobsTable" :activeJobID="jobID" @tableRowClicked="onTableJobClicked"
+      @activeJobDeleted="onActiveJobDeleted" />
   </div>
   <div class="col col-2 job-details-column" id="col-job-details">
     <get-the-addon v-if="jobs.isJobless" />
@@ -116,6 +117,9 @@ export default {
     onTableTaskClicked(rowData) {
       this._routeToTask(rowData.id);
     },
+    onActiveJobDeleted(deletedJobUUID) {
+      this._routeToJobOverview();
+    },
 
     onSelectedTaskChanged(taskSummary) {
       if (!taskSummary) { // There is no active task.
@@ -148,7 +152,7 @@ export default {
       if (this.$refs.jobsTable) {
         this.$refs.jobsTable.processJobUpdate(jobUpdate);
       }
-      if (this.jobID != jobUpdate.id)
+      if (this.jobID != jobUpdate.id || jobUpdate.was_deleted)
         return;
 
       this._fetchJob(this.jobID);
@@ -187,6 +191,13 @@ export default {
       this.$refs.jobDetails.refreshLastRenderedImage(lastRenderedUpdate);
     },
 
+    /**
+     * Send to the job overview page, i.e. job view without active job.
+     */
+    _routeToJobOverview() {
+      const route = { name: 'jobs' };
+      this.$router.push(route);
+    },
     /**
      * @param {string} jobID job ID to navigate to, can be empty string for "no active job".
      */