fix(core): run root effects in creation order (#60534)

alxhub · thePunderWoman · commit a611b234d740 · 2025-04-02T18:24:40.000Z
Previously, the order in which root effects were executed was non-deterministic and relied on the order in which signal graph dirty notifications were propagated. With this commit, root effects are always run in creation order. PR Close #60534
diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts
@@ -338,7 +338,7 @@ export function createRootEffect(
   node.scheduler = scheduler;
   node.notifier = notifier;
   node.zone = typeof Zone !== 'undefined' ? Zone.current : null;
-  node.scheduler.schedule(node);
+  node.scheduler.add(node);
   node.notifier.notify(NotificationSource.RootEffect);
   return node;
 }
diff --git a/packages/core/src/render3/reactivity/root_effect_scheduler.ts b/packages/core/src/render3/reactivity/root_effect_scheduler.ts
@@ -18,12 +18,15 @@ export interface SchedulableEffect {
   zone: {
     run<T>(fn: () => T): T;
   } | null;
+  dirty: boolean;
 }
 
 /**
  * A scheduler which manages the execution of effects.
  */
 export abstract class EffectScheduler {
+  abstract add(e: SchedulableEffect): void;
+
   /**
    * Schedule the given effect to be executed at a later time.
    *
@@ -52,11 +55,19 @@ export abstract class EffectScheduler {
  * when.
  */
 export class ZoneAwareEffectScheduler implements EffectScheduler {
-  private queuedEffectCount = 0;
+  private dirtyEffectCount = 0;
   private queues = new Map<Zone | null, Set<SchedulableEffect>>();
 
-  schedule(handle: SchedulableEffect): void {
+  add(handle: SchedulableEffect): void {
     this.enqueue(handle);
+    this.schedule(handle);
+  }
+
+  schedule(handle: SchedulableEffect): void {
+    if (!handle.dirty) {
+      return;
+    }
+    this.dirtyEffectCount++;
   }
 
   remove(handle: SchedulableEffect): void {
@@ -67,7 +78,9 @@ export class ZoneAwareEffectScheduler implements EffectScheduler {
     }
 
     queue.delete(handle);
-    this.queuedEffectCount--;
+    if (handle.dirty) {
+      this.dirtyEffectCount--;
+    }
   }
 
   private enqueue(handle: SchedulableEffect): void {
@@ -80,7 +93,6 @@ export class ZoneAwareEffectScheduler implements EffectScheduler {
     if (queue.has(handle)) {
       return;
     }
-    this.queuedEffectCount++;
     queue.add(handle);
   }
 
@@ -91,25 +103,37 @@ export class ZoneAwareEffectScheduler implements EffectScheduler {
    * ordering guarantee between effects scheduled in different zones.
    */
   flush(): void {
-    while (this.queuedEffectCount > 0) {
+    while (this.dirtyEffectCount > 0) {
+      let ranOneEffect = false;
       for (const [zone, queue] of this.queues) {
         // `zone` here must be defined.
         if (zone === null) {
-          this.flushQueue(queue);
+          ranOneEffect ||= this.flushQueue(queue);
         } else {
-          zone.run(() => this.flushQueue(queue));
+          ranOneEffect ||= zone.run(() => this.flushQueue(queue));
         }
       }
+
+      // Safeguard against infinite looping if somehow our dirty effect count gets out of sync with
+      // the dirty flag across all the effects.
+      if (!ranOneEffect) {
+        this.dirtyEffectCount = 0;
+      }
     }
   }
 
-  private flushQueue(queue: Set<SchedulableEffect>): void {
+  private flushQueue(queue: Set<SchedulableEffect>): boolean {
+    let ranOneEffect = false;
     for (const handle of queue) {
-      queue.delete(handle);
-      this.queuedEffectCount--;
+      if (!handle.dirty) {
+        continue;
+      }
+      this.dirtyEffectCount--;
+      ranOneEffect = true;
 
       // TODO: what happens if this throws an error?
       handle.run();
     }
+    return ranOneEffect;
   }
 }
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
@@ -313,6 +313,34 @@ describe('reactivity', () => {
       expect(log).toEqual([0, 1]);
     });
 
+    it('should run root effects in creation order independent of dirty order', async () => {
+      TestBed.configureTestingModule({
+        providers: [provideExperimentalZonelessChangeDetection()],
+      });
+      const appRef = TestBed.inject(ApplicationRef);
+
+      const sourceA = signal(0);
+      const sourceB = signal(0);
+
+      const log: string[] = [];
+
+      // Creation order: A, B
+      effect(() => log.push(`A: ${sourceA()}`), {injector: appRef.injector});
+      effect(() => log.push(`B: ${sourceB()}`), {injector: appRef.injector});
+      await appRef.whenStable();
+
+      expect(log).toEqual(['A: 0', 'B: 0']);
+      log.length = 0;
+
+      // Dirty order: B, A
+      sourceB.set(1);
+      sourceA.set(2);
+      await appRef.whenStable();
+
+      // Effects should still run in A, B creation order.
+      expect(log).toEqual(['A: 2', 'B: 1']);
+    });
+
     it('should check components made dirty from markForCheck() from an effect', async () => {
       TestBed.configureTestingModule({
         providers: [provideExperimentalZonelessChangeDetection()],

Original file line number	Diff line number	Diff line change
`@@ -338,7 +338,7 @@ export function createRootEffect(`
`338`	`338`	`node.scheduler = scheduler;`
`339`	`339`	`node.notifier = notifier;`
`340`	`340`	`node.zone = typeof Zone !== 'undefined' ? Zone.current : null;`
`341`		`- node.scheduler.schedule(node);`
	`341`	`+ node.scheduler.add(node);`
`342`	`342`	`node.notifier.notify(NotificationSource.RootEffect);`
`343`	`343`	`return node;`
`344`	`344`	`}`