Skip to content

Commit 6deded4

Browse files
lifeartclaude
andcommitted
fix(gxt-backend): drain leaked classic reactors at testDone, not at fire time
Diagnostic instrumentation (commit 34f9515) confirmed two leak paths: - renderLinkToElement (manager.ts:9519) reactors leak across tests because the existing _disconnectedTicks > 4 self-unsub never trips: routing tests reattach the same anchor across transitions, resetting the counter. - _renderComponentGxt (renderer.ts:2126) reactors leak because the self-unsub fix (commit 6ee5180) never trips — targetElement is usually #qunit-fixture which stays connected across tests; testem only clears its children. Local cumulative reproducer showed a single leaked reactor firing 9,391+ times across unrelated tests. Previous attempt (commit 276a1a4, reverted): drop reactors at fire time when registeredAtTest != currentTest. That regressed ~700 CI tests because Ember's classic reactivity can fire reactors during transient states (afterEach destruction, router setup) where QUnit.config.current is briefly inconsistent with the test that owns the work. This fix: tag each reactor at registration with the QUnit test name, then drain that test's reactors in BULK at testDone — when the test is fully torn down, no reactor of that test is needed in any later test, regardless of QUnit.config.current's instantaneous value. Module-init reactors (registeredAtTest === <no-test>) are exempt. Local validation: 150 DRAIN events at testDone boundaries, each returning the reactor population to 0; 0 cross-test LEAK fires observed in the diagnostic snapshot. Smoke (14 modules / 333 tests) green. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent 6699443 commit 6deded4

2 files changed

Lines changed: 83 additions & 30 deletions

File tree

index.html

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,27 @@
101101
});
102102

103103
QUnit.testDone(function () {
104+
// Drain classic reactors registered during this test. After
105+
// testDone the test's components are torn down, so any reactor
106+
// still alive is leaked and must not fire in later tests. The
107+
// drain matches by registeredAtTest tag (set in validator.ts).
108+
try {
109+
var done = QUnit.config.current;
110+
var doneName = done
111+
? (done.module?.name || '') + ' :: ' + (done.testName || '')
112+
: '';
113+
if (
114+
doneName &&
115+
typeof globalThis.__gxtDrainReactorsForTest === 'function'
116+
) {
117+
globalThis.__gxtDrainReactorsForTest(doneName);
118+
}
119+
} catch (_e) {
120+
/* ignore */
121+
}
104122
// Leak-debug: record reactor population at end of each test.
105-
// A reactor that survives the cleanup hook is a confirmed leak.
123+
// A reactor that survives the drain is a no-test reactor or
124+
// a registration we missed.
106125
try {
107126
if (typeof globalThis.__gxtLeakSnapshot === 'function') {
108127
globalThis.__gxtLeakSnapshot('testDone:' + (QUnit.config.current?.testName || ''));

packages/@ember/-internals/gxt-backend/validator.ts

Lines changed: 63 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -648,21 +648,36 @@ export function touchClassicBridge(): void {
648648
// invoked synchronously at the end of dirtyTagFor after the classic tag
649649
// has been updated but before the runloop flush. Registrants are expected
650650
// to be idempotent and cheap.
651-
// === LEAK DIAGNOSTICS ===
652-
// Each reactor is tagged with metadata at registration time so we can
653-
// detect cross-test leakage: a reactor registered during test A that
654-
// fires during test B is by definition a leak. Enabled via the global
655-
// flag `__GXT_LEAK_DEBUG__` (set in index.html or via URL param).
651+
// Each reactor is tagged at registration with the QUnit test it was
652+
// created in. When that test finishes (testDone), we drain reactors
653+
// with that registeredAtTest in bulk via __gxtDrainReactorsForTest.
654+
// This is the leak fix: it replaces a previous heuristic — a
655+
// "_disconnectedTicks > 4" tick counter on the target element — that
656+
// proved unreliable: targetElement is often the long-lived
657+
// #qunit-fixture (which never disconnects between tests), and routing
658+
// tests reattach LinkTo elements between tests, resetting the counter
659+
// before it can trip. Diagnostics behind `__GXT_LEAK_DEBUG__` confirmed
660+
// the leak path; the drain runs unconditionally. Tagging always-on but
661+
// cheap (one WeakMap put per registration).
662+
//
663+
// We deliberately DO NOT drop reactors at fire time, even when
664+
// registeredAtTest != currentTest, because Ember's classic reactivity
665+
// can fire reactors during transient states (afterEach destruction,
666+
// router setup) where QUnit.config.current is briefly inconsistent
667+
// with the test that owns the work — fire-time drops there caused
668+
// memory/stack regressions in CI when legitimate cross-test wiring
669+
// was wiped mid-flow. The bulk-drain at testDone is timing-safe:
670+
// when a test is fully torn down, no reactor of that test is needed.
656671
interface ReactorMeta {
657672
id: number;
658673
source: string;
659674
registeredAtTest: string;
660-
registeredAtTime: number;
661675
fireCount: number;
662676
}
663677
let _reactorIdCounter = 0;
664678
const _reactorMeta = new WeakMap<() => void, ReactorMeta>();
665679
const _classicReactors = new Set<() => void>();
680+
const NO_TEST = '<no-test>';
666681

667682
function _currentTestName(): string {
668683
try {
@@ -672,35 +687,56 @@ function _currentTestName(): string {
672687
} catch {
673688
/* ignore */
674689
}
675-
return '<no-test>';
690+
return NO_TEST;
676691
}
677692

678693
export function registerClassicReactor(cb: () => void, source?: string): () => void {
679694
_classicReactors.add(cb);
680-
if ((globalThis as any).__GXT_LEAK_DEBUG__) {
681-
const id = ++_reactorIdCounter;
682-
_reactorMeta.set(cb, {
683-
id,
684-
source: source || '<unknown>',
685-
registeredAtTest: _currentTestName(),
686-
registeredAtTime: Date.now(),
687-
fireCount: 0,
688-
});
689-
if (id <= 50 || id % 100 === 0) {
690-
// eslint-disable-next-line no-console
691-
console.log(
692-
`[leak-debug] registerClassicReactor #${id} source=${source || '?'} test="${_currentTestName()}" total=${_classicReactors.size}`
693-
);
694-
}
695+
const id = ++_reactorIdCounter;
696+
_reactorMeta.set(cb, {
697+
id,
698+
source: source || '<unknown>',
699+
registeredAtTest: _currentTestName(),
700+
fireCount: 0,
701+
});
702+
if ((globalThis as any).__GXT_LEAK_DEBUG__ && (id <= 50 || id % 100 === 0)) {
703+
// eslint-disable-next-line no-console
704+
console.log(
705+
`[leak-debug] registerClassicReactor #${id} source=${source || '?'} test="${_currentTestName()}" total=${_classicReactors.size}`
706+
);
695707
}
696708
return () => {
697709
_classicReactors.delete(cb);
698-
if ((globalThis as any).__GXT_LEAK_DEBUG__) {
699-
_reactorMeta.delete(cb);
700-
}
710+
_reactorMeta.delete(cb);
701711
};
702712
}
703713

714+
// Drain reactors registered during a specific test. Called from the
715+
// testDone hook in index.html with the just-finished test name. After
716+
// this drain, that test's reactors cannot fire during subsequent tests.
717+
(globalThis as any).__gxtDrainReactorsForTest = function (testName: string) {
718+
if (!testName || testName === NO_TEST) return;
719+
const debug = (globalThis as any).__GXT_LEAK_DEBUG__;
720+
let dropped = 0;
721+
// Iterate snapshot — _classicReactors is a Set, deletes during
722+
// iteration can be safe in modern engines but a snapshot avoids any
723+
// ambiguity.
724+
for (const cb of Array.from(_classicReactors)) {
725+
const meta = _reactorMeta.get(cb);
726+
if (meta && meta.registeredAtTest === testName) {
727+
_classicReactors.delete(cb);
728+
_reactorMeta.delete(cb);
729+
dropped++;
730+
}
731+
}
732+
if (debug && dropped > 0) {
733+
// eslint-disable-next-line no-console
734+
console.log(
735+
`[leak-debug] DRAIN test="${testName}" dropped=${dropped} remaining=${_classicReactors.size}`
736+
);
737+
}
738+
};
739+
704740
function _fireClassicReactors() {
705741
if (_classicReactors.size === 0) return;
706742
// Copy to avoid mutation during iteration
@@ -712,11 +748,9 @@ function _fireClassicReactors() {
712748
const meta = _reactorMeta.get(cb);
713749
if (meta) {
714750
meta.fireCount++;
715-
// Cross-test leak: reactor registered during a different test
716-
// than the one currently executing.
717751
if (
718-
meta.registeredAtTest !== '<no-test>' &&
719-
currentTest !== '<no-test>' &&
752+
meta.registeredAtTest !== NO_TEST &&
753+
currentTest !== NO_TEST &&
720754
meta.registeredAtTest !== currentTest
721755
) {
722756
// eslint-disable-next-line no-console

0 commit comments

Comments
 (0)