From 217f6d96ba924cc1f85b12878653cefbcbba14ab Mon Sep 17 00:00:00 2001
From: Matthieu Riegler <kyro38@gmail.com>
Date: Sat, 5 Oct 2024 17:34:43 +0200
Subject: [PATCH] refactor(core): run `effect` cleanup as `untracked` (#60421)

A effect cleanup should not be considered reactive.

fixes #57757

PR Close #60421
---
 .../core/src/render3/reactivity/effect.ts     |  4 +-
 packages/core/test/render3/reactivity_spec.ts | 44 +++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/packages/core/src/render3/reactivity/effect.ts b/packages/core/src/render3/reactivity/effect.ts
index 754791718ed..ac2fb437812 100644
--- a/packages/core/src/render3/reactivity/effect.ts
+++ b/packages/core/src/render3/reactivity/effect.ts
@@ -15,12 +15,12 @@ import {
   consumerDestroy,
   consumerPollProducersForChange,
   isInNotificationPhase,
+  setActiveConsumer,
 } from '../../../primitives/signals';
 import {FLAGS, LViewFlags, LView, EFFECTS} from '../interfaces/view';
 import {markAncestorsForTraversal} from '../util/view_utils';
 import {InjectionToken} from '../../di/injection_token';
 import {inject} from '../../di/injector_compatibility';
-import {performanceMarkFeature} from '../../util/performance';
 import {Injector} from '../../di/injector';
 import {assertNotInReactiveContext} from './asserts';
 import {assertInInjectionContext} from '../../di/contextual';
@@ -267,6 +267,7 @@ export const BASE_EFFECT_NODE: Omit<EffectNode, 'fn' | 'destroy' | 'injector' |
       if (!this.cleanupFns?.length) {
         return;
       }
+      const prevConsumer = setActiveConsumer(null);
       try {
         // Attempt to run the cleanup functions. Regardless of failure or success, we consider
         // cleanup "completed" and clear the list for the next run of the effect. Note that an error
@@ -276,6 +277,7 @@ export const BASE_EFFECT_NODE: Omit<EffectNode, 'fn' | 'destroy' | 'injector' |
         }
       } finally {
         this.cleanupFns = [];
+        setActiveConsumer(prevConsumer);
       }
     },
   }))();
diff --git a/packages/core/test/render3/reactivity_spec.ts b/packages/core/test/render3/reactivity_spec.ts
index fffd1ee3b41..85cde76c62d 100644
--- a/packages/core/test/render3/reactivity_spec.ts
+++ b/packages/core/test/render3/reactivity_spec.ts
@@ -32,6 +32,7 @@ import {
   signal,
   SimpleChanges,
   TemplateRef,
+  untracked,
   ViewChild,
   ViewContainerRef,
 } from '../../src/core';
@@ -192,6 +193,49 @@ describe('reactivity', () => {
       expect(cleanupCount).toBe(2);
     });
 
+    it('should run effect cleanup as untracked', async () => {
+      @Component({
+        template: '',
+      })
+      class Cmp {
+        counter = signal(0);
+        effectTrigger = signal(0);
+
+        effectRef = effect((onCleanup) => {
+          this.effectTrigger();
+
+          untracked(() => {
+            if (this.counter() > 1) {
+              // This is an early bailout in case the effect loops infinitely
+              throw new Error('Updated consummers in cleanup for not re-trigger the effect');
+            }
+          });
+
+          onCleanup(() => {
+            this.counter(); // A signal read but not consummed
+            this.counter.update((v) => v + 1);
+          });
+        });
+      }
+
+      const fixture = TestBed.createComponent(Cmp);
+      fixture.detectChanges();
+      await fixture.whenStable();
+      // initially an effect runs but the default cleanup function is noop
+      expect(fixture.componentInstance.counter()).toBe(0);
+
+      // Triggers a cleanup
+      fixture.componentInstance.effectTrigger.update((v) => v + 1);
+      fixture.detectChanges();
+      await fixture.whenStable();
+
+      expect(fixture.componentInstance.counter()).toBe(1);
+
+      // Destroy triggers a cleanup
+      fixture.destroy();
+      expect(fixture.componentInstance.counter()).toBe(2);
+    });
+
     it('should run effects created in ngAfterViewInit', () => {
       let didRun = false;