fix(image): runId-scoped cancellation (cross-run safe) — review #460

The shared cancelRequested boolean was cross-run unsafe: cancelling run #1 (while it still
awaited _enhancePrompt/native) then starting run #2 — which reset the flag at its top —
cleared run #1's cancel, so run #1 could proceed into generation and clobber state.

Replace the boolean with a request-scoped run id: generateImage claims ;
cancelGeneration (and any newer run) advances activeRunId, so every checkpoint captured
under the old id (_enhancePrompt/_ensureImageModelLoaded checkpoints, the progress/preview
callbacks, and the post-native result) bails via _isStale(runId). A stale run also no
longer calls resetState (a newer run may own the state).

Test: added a cross-run case — cancel run #1 mid-enhance, run run #2 to completion, then
release run #1's stalled enhancement and assert it adds no generator call. Plus the
existing enhancing-cancel case still passes. 9 batch4 + full image-gen flow green, tsc clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: alichherawalla

__tests__/hardening/batch4-phase-state-machine.test.ts

@@ -223,6 +223,41 @@ describe('cancel mid-flight resets the machine to idle (case 20)', () => {
     expect(mockDream.generateImage).not.toHaveBeenCalled();
     expect(imageGenerationService.getState().phase).toBe('idle');
   });
+
+  // Review #460: cross-run safety. Cancel run #1 while it's still awaiting enhancement,
+  // start run #2, then let run #1's enhancement resolve — run #1 must NOT resurrect and
+  // call the generator. With a shared cancel boolean, run #2 clearing it would revive #1.
+  it('a cancelled run does not proceed even after a new run starts (runId-scoped)', async () => {
+    setupModel();
+    useAppStore.setState({
+      activeModelId: 'text-1',
+      settings: { ...useAppStore.getState().settings, enhanceImagePrompts: true } as any,
+    });
+    mockLlm.isModelLoaded.mockReturnValue(true);
+    // Run #1 enhancement hangs until we release it.
+    let releaseEnhance1!: () => void;
+    mockLlm.generateResponse.mockImplementationOnce(
+      () => new Promise<string>((r) => { releaseEnhance1 = () => r('enhanced-1'); }),
+    );
+
+    imageGenerationService.generateImage({ prompt: 'run 1' });
+    await flushPromises();
+    expect(imageGenerationService.getState().phase).toBe('enhancing');
+
+    await imageGenerationService.cancelGeneration(); // cancel run #1 (state → idle)
+
+    // Run #2 starts and completes (enhancement resolves immediately now).
+    mockLlm.generateResponse.mockResolvedValue('enhanced-2');
+    await imageGenerationService.generateImage({ prompt: 'run 2' });
+    const dreamCallsAfterRun2 = mockDream.generateImage.mock.calls.length;
+
+    // Now release run #1's stalled enhancement — it must be inert (its runId is stale).
+    releaseEnhance1();
+    await flushPromises();
+
+    // Run #1 added NO extra generator call after run #2 finished.
+    expect(mockDream.generateImage.mock.calls.length).toBe(dreamCallsAfterRun2);
+  });
 });
 
 describe('no-model / load-failure surface an error phase, never a silent hang (cases 30, 38)', () => {

src/services/imageGenerationService.ts

@@ -68,6 +68,7 @@ interface ActiveImageModel {
 }
 
 interface RunGenerationOptions {
+  runId: number;
   params: GenerateImageParams;
   enhancedPrompt: string;
   activeImageModel: ActiveImageModel;
@@ -99,7 +100,10 @@ class ImageGenerationService {
   };
 
   private readonly listeners: Set<ImageGenerationListener> = new Set();
-  private cancelRequested: boolean = false;
+  // Request-scoped cancellation. generateImage() claims a fresh runId; a cancel or a new
+  // run advances activeRunId, invalidating every in-flight checkpoint captured under the
+  // old id. (A shared boolean was cross-run unsafe — a new run could clear a prior run's cancel.)
+  private activeRunId = 0;
   /** Last generate request, so a failure card's Retry button can re-run it. */
   private _lastParams: GenerateImageParams | null = null;
 
@@ -341,7 +345,7 @@ class ImageGenerationService {
   }
 
   private async _runGenerationAndSave(opts: RunGenerationOptions): Promise<GeneratedImage | null> {
-    const { params, enhancedPrompt, activeImageModel, steps, guidanceScale, imageWidth, imageHeight, useOpenCL } = opts;
+    const { runId, params, enhancedPrompt, activeImageModel, steps, guidanceScale, imageWidth, imageHeight, useOpenCL } = opts;
 
     // The first generation for a model compiles/warms the backend and takes ~120s.
     // This is platform-agnostic: on iOS the CoreML model compiles on first use, on
@@ -371,7 +375,7 @@ class ImageGenerationService {
       const result = await onnxImageGeneratorService.generateImage(
         { prompt: enhancedPrompt, negativePrompt: params.negativePrompt || '', steps, guidanceScale, seed: params.seed, width: imageWidth, height: imageHeight, previewInterval: params.previewInterval ?? 2, useOpenCL },
         (progress) => {
-          if (this.cancelRequested) return;
+          if (this._isStale(runId)) return;
           const displayStep = Math.min(progress.step, steps);
           if (isFirstRun) {
             this.updateState({
@@ -385,12 +389,15 @@ class ImageGenerationService {
           }
         },
         (preview) => {
-          if (this.cancelRequested) return;
+          if (this._isStale(runId)) return;
           const displayStep = Math.min(preview.step, steps);
           this.updateState({ previewPath: `file://${preview.previewPath}?t=${Date.now()}`, status: `Refining image (${displayStep}/${steps})...` });
         },
       );
-      if (this.cancelRequested || !result?.imagePath) { this.resetState(); return null; }
+      // Cancelled mid-native (cancelGeneration already reset state) or no image → bail.
+      // A stale run must not resetState (a newer run may own it) nor save its result.
+      if (this._isStale(runId)) return null;
+      if (!result?.imagePath) { this.resetState(); return null; }
       return this._saveResult(result, { params, activeImageModel, meta: { steps, guidanceScale, useOpenCL, startTime } });
     } catch (error: any) {
       const errorMsg = error?.message || 'Image generation failed';
@@ -424,10 +431,10 @@ class ImageGenerationService {
       logger.log('[ImageGenerationService] Already generating, ignoring request');
       return null;
     }
-    // Fresh request: clear any stale cancel flag from a prior run BEFORE any await, so a
-    // leftover `true` can't abort this one — and so a cancel arriving during
-    // _enhancePrompt below is genuinely this request's, not stale.
-    this.cancelRequested = false;
+    // Claim a fresh run id BEFORE any await. Every checkpoint below compares against it,
+    // so a later run (or a cancel) that advances activeRunId invalidates THIS run's
+    // remaining work — and this run can't be aborted by a stale signal from a prior one.
+    const runId = ++this.activeRunId;
     this._lastParams = params; // so a failure card's Retry can re-run this exact request
     const { settings, activeImageModelId, downloadedImageModels } = useAppStore.getState();
     const activeImageModel = downloadedImageModels.find(m => m.id === activeImageModelId);
@@ -440,12 +447,11 @@ class ImageGenerationService {
 
     const enhancedPrompt = await this._enhancePrompt(params, steps);
     logger.log('[ImageGen] enhanceImagePrompts setting:', settings.enhanceImagePrompts);
-    // Honor a cancel that arrived WHILE enhancing (cancelGeneration already reset the
-    // state to idle). The old code unconditionally cleared cancelRequested here, so a
-    // cancel tapped during the 'enhancing' phase was silently discarded and generation
-    // proceeded anyway. _enhancePrompt itself can't observe the flag (it awaits the LLM),
-    // so this is the checkpoint.
-    if (this.cancelRequested) { this.resetState(); return null; }
+    // Honor a cancel that arrived WHILE enhancing (cancelGeneration advanced activeRunId
+    // and reset state to idle). _enhancePrompt awaits the LLM and can't observe the run
+    // id, so this is the checkpoint. If cancelled, THIS run stops here — don't resetState
+    // (a newer run may already own it); just bail.
+    if (this._isStale(runId)) return null;
 
     // Establish the generating state unconditionally — not only when enhancement
     // is off. When enhancement is ON but _enhancePrompt bailed early (e.g. no text
@@ -461,14 +467,22 @@ class ImageGenerationService {
 
     const loaded = await this._ensureImageModelLoaded(activeImageModelId, activeImageModel, settings.imageThreads ?? 4);
     if (!loaded) return null;
-    if (this.cancelRequested) { this.resetState(); return null; }
+    if (this._isStale(runId)) return null;
 
-    return this._runGenerationAndSave({ params, enhancedPrompt, activeImageModel, steps, guidanceScale, imageWidth, imageHeight, useOpenCL: settings.imageUseOpenCL ?? true });
+    return this._runGenerationAndSave({ runId, params, enhancedPrompt, activeImageModel, steps, guidanceScale, imageWidth, imageHeight, useOpenCL: settings.imageUseOpenCL ?? true });
+  }
+
+  /** True when `runId` is no longer the active run (cancelled, or superseded by a newer
+   *  generateImage). Every in-flight checkpoint bails on this instead of a shared flag. */
+  private _isStale(runId: number): boolean {
+    return runId !== this.activeRunId;
   }
 
   async cancelGeneration(): Promise<void> {
     if (!isInFlight(this.state.phase)) return;
-    this.cancelRequested = true;
+    // Advance the run id: the in-flight run's captured id no longer matches, so all its
+    // remaining checkpoints bail. (A shared boolean could be cleared by a subsequent run.)
+    this.activeRunId++;
     try { await onnxImageGeneratorService.cancelGeneration(); } catch { /* Ignore */ }
     this.resetState();
   }