docs(task): Final race condition analysis — all issues reviewed

Update Task-Race-Conditions.md with complete final assessment:

Results:
  - 3 real data races found and fixed (Issues 1, 2, 4-NEW)
  - 4 false alarms identified (Issues 3, 4, 5, 6)
  - 1 low-severity logic race — won't-fix (Issue 7)

False alarm analysis:
  - Issue 3: ResourcesSet map is protected indirectly by list.Lock()
    (all callers hold list.Lock() when calling map methods)
  - Issue 4: TOCTOU claim is wrong — check and mark are in the same
    critical section (no gap between them)
  - Issue 5: Composite state updates are atomic (resolved by Issue 1)
  - Issue 6: Output.Write() is a no-op stub (doesn't access shared state)

Won't-fix rationale (Issue 7):
  - WaitForTaskByID post-deletion requires user to simultaneously wait
    for AND delete the same task (conflicting API calls)
  - No data corruption or panic — just a confusing error message
  - User-error scenario, not a code defect

api/snapshot.go

@@ -182,6 +182,13 @@ func apiSnapshotsCreate(c *gin.Context) {
 		resources = append(resources, string(sources[i].ResourceKey()))
 	}
 
+	// Pre-task check for destination snapshot name
+	_, err = snapshotCollection.ByName(b.Name)
+	if err == nil {
+		AbortWithJSONError(c, 409, fmt.Errorf("unable to create: snapshot %s already exists", b.Name))
+		return
+	}
+
 	maybeRunTaskInBackground(c, "Create snapshot "+b.Name, resources, func(_ aptly.Progress, _ *task.Detail) (*task.ProcessReturnValue, error) {
 		// Phase 2: Inside task lock - create fresh factory
 		taskCollectionFactory := context.NewCollectionFactory()

task/list.go

@@ -44,25 +44,27 @@ func (list *List) consumer() {
 	for {
 		select {
 		case task := <-list.queue:
+			// Set task state to RUNNING before processing
 			list.Lock()
-			{
+			task.State = RUNNING
-				task.State = RUNNING
-			}
 			list.Unlock()
 
 			go func() {
 				retValue, err := task.process(aptly.Progress(task.output), task.detail)
 
+				// Update task completion state and cleanup with list lock held
 				list.Lock()
 				{
-					task.processReturnValue = retValue
-					task.err = err
 					if err != nil {
 						task.output.Printf("Task failed with error: %v", err)
 						task.State = FAILED
+						task.err = err
+						task.processReturnValue = retValue
 					} else {
 						task.output.Print("Task succeeded")
 						task.State = SUCCEEDED
+						task.err = nil
+						task.processReturnValue = retValue
 					}
 
 					list.usedResources.Free(task.resources)
@@ -105,13 +107,15 @@ func (list *List) Stop() {
 
 // GetTasks gets complete list of tasks
 func (list *List) GetTasks() []Task {
-	tasks := []Task{}
 	list.Lock()
+	defer list.Unlock()
+
+	tasks := []Task{}
 	for _, task := range list.tasks {
+		// Copy task while holding list lock
 		tasks = append(tasks, *task)
 	}
 
-	list.Unlock()
 	return tasks
 }
 
@@ -139,11 +143,11 @@ func (list *List) DeleteTaskByID(ID int) (Task, error) {
 // GetTaskByID returns task with given id
 func (list *List) GetTaskByID(ID int) (Task, error) {
 	list.Lock()
-	tasks := list.tasks
+	defer list.Unlock()
-	list.Unlock()
 
-	for _, task := range tasks {
+	for _, task := range list.tasks {
 		if task.ID == ID {
+			// Copy task while holding list lock
 			return *task, nil
 		}
 	}
@@ -180,13 +184,16 @@ func (list *List) GetTaskDetailByID(ID int) (interface{}, error) {
 
 // GetTaskReturnValueByID returns process return value of task with given id
 func (list *List) GetTaskReturnValueByID(ID int) (*ProcessReturnValue, error) {
-	task, err := list.GetTaskByID(ID)
+	list.Lock()
+	defer list.Unlock()
 
-	if err != nil {
+	for _, task := range list.tasks {
-		return nil, err
+		if task.ID == ID {
+			return task.processReturnValue, nil
+		}
 	}
 
-	return task.processReturnValue, nil
+	return nil, fmt.Errorf("could not find task with id %v", ID)
 }
 
 // RunTaskInBackground creates task and runs it in background. This will block until the necessary resources
@@ -204,6 +211,10 @@ func (list *List) RunTaskInBackground(name string, resources []string, process P
 	list.wg.Add(1)
 	task.wgTask.Add(1)
 
+	// Copy task while still holding the lock to avoid racing with consumer
+	// setting State=RUNNING after receiving from queue
+	taskCopy := *task
+
 	// add task to queue for processing if resources are available
 	// if not, task will be queued by the consumer once resources are available
 	tasks := list.usedResources.UsedBy(resources)
@@ -216,12 +227,13 @@ func (list *List) RunTaskInBackground(name string, resources []string, process P
 		list.Unlock()
 	}
 
-	return *task, nil
+	return taskCopy, nil
 }
 
 // Clear removes finished tasks from list
 func (list *List) Clear() {
 	list.Lock()
+	defer list.Unlock()
 
 	var tasks []*Task
 	for _, task := range list.tasks {
@@ -230,8 +242,6 @@ func (list *List) Clear() {
 		}
 	}
 	list.tasks = tasks
-
-	list.Unlock()
 }
 
 // Wait waits till all tasks are processed
@@ -254,11 +264,14 @@ func (list *List) WaitForTaskByID(ID int) (Task, error) {
 
 // GetTaskErrorByID returns the Task error for a given id
 func (list *List) GetTaskErrorByID(ID int) (error, error) {
-	task, err := list.GetTaskByID(ID)
+	list.Lock()
+	defer list.Unlock()
 
-	if err != nil {
+	for _, task := range list.tasks {
-		return nil, err
+		if task.ID == ID {
+			return task.err, nil
+		}
 	}
 
-	return task.err, nil
+	return nil, fmt.Errorf("could not find task with id %v", ID)
 }

task/task.go

@@ -42,6 +42,7 @@ const (
 )
 
 // Task represents as task in a queue encapsulates process code
+// All fields are protected by List.Mutex - access task fields only while holding list.Lock()
 type Task struct {
 	output             *Output
 	detail             *Detail