fix(security): eliminate live-pointer data races in scanner registry and engine

internal/security/scanner/engine.go

@@ -147,8 +147,10 @@ func (e *Engine) StartScan(ctx context.Context, req ScanRequest, callback ScanCa
 	// Check for existing scan
 	e.mu.Lock()
 	if existing, ok := e.activeScans[req.ServerName]; ok {
+		// Snapshot: the running scan keeps writing to the live record.
+		snapshot := existing.clone()
 		e.mu.Unlock()
-		return existing, fmt.Errorf("scan already in progress for server %s (job %s)", req.ServerName, existing.ID)
+		return snapshot, fmt.Errorf("scan already in progress for server %s (job %s)", req.ServerName, snapshot.ID)
 	}
 
 	// Determine which scanners to use. The Docker-scanner skip (MCP-34.4)
@@ -190,37 +192,90 @@ func (e *Engine) StartScan(ctx context.Context, req ScanRequest, callback ScanCa
 	}
 
 	e.activeScans[req.ServerName] = job
+	// Snapshot before releasing the lock: from here on the scan goroutines own
+	// the live record, so neither the callback nor the caller may hold it.
+	started := job.clone()
+	returned := job.clone()
 	e.mu.Unlock()
 
-	callback.OnScanStarted(job)
+	callback.OnScanStarted(started)
 
 	// Run scanners in background with detached context
 	// (the HTTP request context may be cancelled after the response is sent)
 	go e.executeScan(context.Background(), job, resolved, req, callback)
 
-	return job, nil
+	return returned, nil
 }
 
-// CancelScan cancels a running scan for a server
+// CancelScan cancels a running scan for a server.
+//
+// A job stays in activeScans for a short window AFTER executeScan has written
+// its terminal status — the completion callback persists the report and emits
+// the completion event inside that window. Cancelling there must fail rather
+// than succeed: the terminal outcome has already been cloned and is on its way
+// to storage, so flipping Status to cancelled would leave the API reporting a
+// cancellation that the stored job contradicts.
 func (e *Engine) CancelScan(serverName string) error {
 	e.mu.Lock()
 	job, ok := e.activeScans[serverName]
 	if !ok {
 		e.mu.Unlock()
 		return fmt.Errorf("no active scan for server %s", serverName)
 	}
+	if job.Status == ScanJobStatusCompleted || job.Status == ScanJobStatusFailed ||
+		job.Status == ScanJobStatusCancelled {
+		status := job.Status
+		e.mu.Unlock()
+		return fmt.Errorf("scan for server %s already finished (status: %s)", serverName, status)
+	}
 	job.Status = ScanJobStatusCancelled
 	job.CompletedAt = time.Now()
 	delete(e.activeScans, serverName)
 	e.mu.Unlock()
 	return nil
 }
 
-// GetActiveJob returns the active scan job for a server, if any
+// clearActiveJob releases the activeScans slot held by `job`, and ONLY if that
+// job still holds it.
+//
+// The identity check is load-bearing. CancelScan drops a job from activeScans
+// while its scanner goroutines are still running, so a replacement scan for the
+// same server can take the slot before the cancelled scan unwinds. Deleting by
+// server name alone would then evict the replacement — leaving a running scan
+// invisible to GetActiveJob and letting StartScan accept a second concurrent
+// scan of the same server.
+func (e *Engine) clearActiveJob(serverName string, job *ScanJob) {
+	e.mu.Lock()
+	if current, ok := e.activeScans[serverName]; ok && current == job {
+		delete(e.activeScans, serverName)
+	}
+	e.mu.Unlock()
+}
+
+// GetActiveJob returns a snapshot of the active scan job for a server, or nil
+// when no scan is running.
+//
+// It MUST be a snapshot: the scan goroutines keep writing Status, CompletedAt
+// and ScannerStatuses on the live record until the job leaves activeScans, so
+// handing the live pointer to a REST reader (GET /api/v1/security/scan/status,
+// which JSON-encodes it) is a data race.
 func (e *Engine) GetActiveJob(serverName string) *ScanJob {
 	e.mu.Lock()
 	defer e.mu.Unlock()
-	return e.activeScans[serverName]
+	job, ok := e.activeScans[serverName]
+	if !ok {
+		return nil
+	}
+	return job.clone()
+}
+
+// snapshotJob copies a job under the engine lock so it can be handed to a scan
+// callback (which persists and JSON-encodes it) while sibling scanner
+// goroutines are still updating the live record.
+func (e *Engine) snapshotJob(job *ScanJob) *ScanJob {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	return job.clone()
 }
 
 // resolvedScanner pairs a plugin with a precomputed error that will cause
@@ -365,11 +420,7 @@ func (e *Engine) resolveScanners(requestedIDs []string, isolationMode string) ([
 // immediately and skipped — this keeps missing-image scanners visible in
 // the aggregated scan report instead of being silently dropped.
 func (e *Engine) executeScan(ctx context.Context, job *ScanJob, scanners []resolvedScanner, req ScanRequest, callback ScanCallback) {
-	defer func() {
-		e.mu.Lock()
-		delete(e.activeScans, req.ServerName)
-		e.mu.Unlock()
-	}()
+	defer e.clearActiveJob(req.ServerName, job)
 
 	var (
 		reports []*ScanReport
@@ -383,7 +434,10 @@ func (e *Engine) executeScan(ctx context.Context, job *ScanJob, scanners []resol
 			defer wg.Done()
 
 			scanner := item.plugin
-			callback.OnScannerStarted(job, scanner.ID)
+			// Every callback gets a snapshot: the adapter persists and
+			// JSON-encodes the job while sibling goroutines are still writing
+			// ScannerStatuses on the live record.
+			callback.OnScannerStarted(e.snapshotJob(job), scanner.ID)
 
 			// Fast-fail for prefailed scanners (e.g. missing Docker image).
 			// We still emit started/failed callbacks so the UI sees the
@@ -395,7 +449,7 @@ func (e *Engine) executeScan(ctx context.Context, job *ScanJob, scanners []resol
 					zap.String("reason", item.prefail),
 				)
 				e.updateScannerStatus(job, scanner.ID, ScanJobStatusFailed, time.Now(), time.Now(), item.prefail, 0)
-				callback.OnScannerFailed(job, scanner.ID, fmt.Errorf("%s", item.prefail))
+				callback.OnScannerFailed(e.snapshotJob(job), scanner.ID, fmt.Errorf("%s", item.prefail))
 				return
 			}
 
@@ -411,7 +465,7 @@ func (e *Engine) executeScan(ctx context.Context, job *ScanJob, scanners []resol
 				e.updateScannerStatus(job, scanner.ID, ScanJobStatusFailed, time.Time{}, time.Now(), err.Error(), 0)
 				// Store logs even on failure
 				e.setScannerLogs(job, scanner.ID, scanLogs)
-				callback.OnScannerFailed(job, scanner.ID, err)
+				callback.OnScannerFailed(e.snapshotJob(job), scanner.ID, err)
 				return
 			}
 
@@ -424,19 +478,21 @@ func (e *Engine) executeScan(ctx context.Context, job *ScanJob, scanners []resol
 
 			e.updateScannerStatus(job, scanner.ID, ScanJobStatusCompleted, time.Time{}, time.Now(), "", len(report.Findings))
 			e.setScannerLogs(job, scanner.ID, scanLogs)
-			callback.OnScannerCompleted(job, scanner.ID, report)
+			callback.OnScannerCompleted(e.snapshotJob(job), scanner.ID, report)
 		}(i, rs)
 	}
 
 	wg.Wait()
 
-	// Check if job was cancelled
+	// Read the cancellation flag, decide the terminal status and write it in a
+	// SINGLE locked section. The job is still in activeScans at this point, so
+	// a concurrent GetActiveJob (REST scan-status) can be snapshotting it while
+	// we write — doing this outside the lock is a data race.
 	e.mu.Lock()
 	if job.Status == ScanJobStatusCancelled {
 		e.mu.Unlock()
 		return
 	}
-	e.mu.Unlock()
 
 	// Determine final status
 	allFailed := true
@@ -446,18 +502,22 @@ func (e *Engine) executeScan(ctx context.Context, job *ScanJob, scanners []resol
 			break
 		}
 	}
-
-	if allFailed && len(scanners) > 0 {
+	failed := allFailed && len(scanners) > 0
+	if failed {
 		job.Status = ScanJobStatusFailed
 		job.Error = "all scanners failed"
-		job.CompletedAt = time.Now()
-		callback.OnScanFailed(job, fmt.Errorf("all scanners failed"))
-		return
+	} else {
+		job.Status = ScanJobStatusCompleted
 	}
-
-	job.Status = ScanJobStatusCompleted
 	job.CompletedAt = time.Now()
-	callback.OnScanCompleted(job, reports)
+	final := job.clone()
+	e.mu.Unlock()
+
+	if failed {
+		callback.OnScanFailed(final, fmt.Errorf("all scanners failed"))
+		return
+	}
+	callback.OnScanCompleted(final, reports)
 }
 
 // runSingleScanner executes one scanner and returns its report plus execution logs

internal/security/scanner/engine_activescans_test.go

@@ -0,0 +1,100 @@
+package scanner
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+)
+
+func newBareEngine(t *testing.T) *Engine {
+	t.Helper()
+	dir := t.TempDir()
+	logger := zap.NewNop()
+	return NewEngine(nil, NewRegistry(dir, logger), dir, logger)
+}
+
+// TestClearActiveJobOnlyEvictsItsOwnJob pins the activeScans slot invariant.
+//
+// executeScan's cleanup used to delete by server name alone. CancelScan drops a
+// job from activeScans immediately while its scanner goroutines are still
+// running, so a replacement scan for the same server can take the slot before
+// the cancelled scan's cleanup runs — and that cleanup then evicted the
+// REPLACEMENT. With the slot empty, StartScan would accept a third concurrent
+// scan of the same server and GetScanStatus would report "no active scan" while
+// one was running.
+func TestClearActiveJobOnlyEvictsItsOwnJob(t *testing.T) {
+	e := newBareEngine(t)
+
+	jobA := &ScanJob{ID: "job-a", ServerName: "srv", Status: ScanJobStatusRunning}
+	jobB := &ScanJob{ID: "job-b", ServerName: "srv", Status: ScanJobStatusRunning}
+
+	e.mu.Lock()
+	e.activeScans["srv"] = jobA
+	e.mu.Unlock()
+
+	// A user cancels scan A. Its goroutines keep running for now.
+	require.NoError(t, e.CancelScan("srv"))
+	require.Nil(t, e.GetActiveJob("srv"))
+
+	// A replacement scan takes the freed slot.
+	e.mu.Lock()
+	e.activeScans["srv"] = jobB
+	e.mu.Unlock()
+
+	// Scan A finally unwinds and runs its cleanup.
+	e.clearActiveJob("srv", jobA)
+
+	active := e.GetActiveJob("srv")
+	require.NotNil(t, active, "the replacement scan must still hold the slot")
+	assert.Equal(t, "job-b", active.ID)
+
+	// B's own cleanup does free the slot.
+	e.clearActiveJob("srv", jobB)
+	assert.Nil(t, e.GetActiveJob("srv"))
+}
+
+// TestCancelScanRejectsAlreadyFinishedJob covers the window between the
+// terminal-status write in executeScan and the job leaving activeScans (the
+// completion callback persists the report inside it). A cancel arriving there
+// used to report success and flip Status to cancelled while the already-cloned
+// COMPLETED job was still persisted and emitted — the API told the user the
+// scan was cancelled, and the stored result said it completed.
+func TestCancelScanRejectsAlreadyFinishedJob(t *testing.T) {
+	for _, status := range []string{ScanJobStatusCompleted, ScanJobStatusFailed} {
+		t.Run(status, func(t *testing.T) {
+			e := newBareEngine(t)
+			job := &ScanJob{ID: "job-1", ServerName: "srv", Status: status}
+			e.mu.Lock()
+			e.activeScans["srv"] = job
+			e.mu.Unlock()
+
+			err := e.CancelScan("srv")
+			require.Error(t, err, "cancelling a settled scan must not report success")
+
+			// The job stays in activeScans until its own cleanup runs, so the
+			// status endpoint keeps answering for the completion callback window.
+			active := e.GetActiveJob("srv")
+			require.NotNil(t, active)
+			assert.Equal(t, status, active.Status, "the terminal status must be left intact")
+		})
+	}
+}
+
+// TestCancelScanCancelsRunningJob keeps the happy path honest.
+func TestCancelScanCancelsRunningJob(t *testing.T) {
+	e := newBareEngine(t)
+	job := &ScanJob{ID: "job-1", ServerName: "srv", Status: ScanJobStatusRunning}
+	e.mu.Lock()
+	e.activeScans["srv"] = job
+	e.mu.Unlock()
+
+	require.NoError(t, e.CancelScan("srv"))
+	assert.Nil(t, e.GetActiveJob("srv"))
+
+	e.mu.Lock()
+	gotStatus := job.Status
+	e.mu.Unlock()
+	assert.Equal(t, ScanJobStatusCancelled, gotStatus)
+}