Skip to content

Commit 276a1a4

Browse files
lifeartclaude
andcommitted
fix(gxt-backend): drain cross-test classic-reactor leaks at fire time
Diagnostic instrumentation (commit 34f9515) confirmed that the cumulative-state failures in testem CI are driven by leaked classic reactors crossing test boundaries. Two specific patterns: - renderLinkToElement (manager.ts:9519) leaks routing-test reactors whose existing _disconnectedTicks > 4 self-unsub never trips, because routing tests reattach the same anchor across transitions and the counter resets each time. - _renderComponentGxt (renderer.ts:2126) leaks renderComponent reactors whose self-unsub fix (commit 6ee5180) never trips, because targetElement is typically #qunit-fixture which stays connected across tests; testem only clears its children. Local reproducer (scripts/gxt-test-runner/leak-debug.mjs) showed a single leaked reactor firing 9,391+ times across unrelated tests. Fix: tag each reactor at registration with the QUnit test it was created in. In _fireClassicReactors, reactors whose registeredAtTest differs from the current test's name are unconditionally invalid (no real-app reactor legitimately survives a QUnit test boundary) and we self-unsubscribe before invoking them. Module-init and pre- QUnit reactors (registeredAtTest === <no-test>) are exempt and fire normally. Result on the local cumulative reproducer: 550 leaks intercepted and drained at the first cross-test fire; reactor population returns to 0 between tests; the runaway 9,391-fire reactor no longer exists. Smoke (14 modules / 333 tests) green. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent 34f9515 commit 276a1a4

2 files changed

Lines changed: 53 additions & 45 deletions

File tree

index.html

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,9 +142,8 @@
142142
} catch (_e) {
143143
/* ignore */
144144
}
145-
// TEMPORARY: force-enable leak diagnostics for CI debugging.
146-
// Remove this line once the cumulative-state failures are diagnosed.
147-
globalThis.__GXT_LEAK_DEBUG__ = true;
145+
// Leak diagnostics flag — kept off by default. Toggle via URL
146+
// (?gxtLeakDebug=true) only when reproducing the cross-test leak.
148147

149148
// Global operation counter to detect infinite synchronous loops
150149
// Any tight loop will increment this. When it exceeds the limit,

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

Lines changed: 51 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -648,21 +648,28 @@ 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. On fire, reactors registered in a different test than
653+
// the one currently executing are unconditionally invalid (a real-app
654+
// reactor could never legitimately survive the test it was created
655+
// for) and we self-unsubscribe before invoking them. This replaces a
656+
// previous heuristic — a "_disconnectedTicks > 4" tick counter on the
657+
// target element — that proved unreliable: targetElement is often the
658+
// long-lived #qunit-fixture (which never disconnects between tests),
659+
// and routing tests reattach LinkTo elements between tests, resetting
660+
// the counter before it can trip. The diagnostic that confirmed the
661+
// leak path lives behind `__GXT_LEAK_DEBUG__`; the fix is on by
662+
// default. Tagging is always-on but cheap (one WeakMap put).
656663
interface ReactorMeta {
657664
id: number;
658665
source: string;
659666
registeredAtTest: string;
660-
registeredAtTime: number;
661667
fireCount: number;
662668
}
663669
let _reactorIdCounter = 0;
664670
const _reactorMeta = new WeakMap<() => void, ReactorMeta>();
665671
const _classicReactors = new Set<() => void>();
672+
const NO_TEST = '<no-test>';
666673

667674
function _currentTestName(): string {
668675
try {
@@ -672,60 +679,62 @@ function _currentTestName(): string {
672679
} catch {
673680
/* ignore */
674681
}
675-
return '<no-test>';
682+
return NO_TEST;
676683
}
677684

678685
export function registerClassicReactor(cb: () => void, source?: string): () => void {
679686
_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-
}
687+
const id = ++_reactorIdCounter;
688+
_reactorMeta.set(cb, {
689+
id,
690+
source: source || '<unknown>',
691+
registeredAtTest: _currentTestName(),
692+
fireCount: 0,
693+
});
694+
if ((globalThis as any).__GXT_LEAK_DEBUG__ && (id <= 50 || id % 100 === 0)) {
695+
// eslint-disable-next-line no-console
696+
console.log(
697+
`[leak-debug] registerClassicReactor #${id} source=${source || '?'} test="${_currentTestName()}" total=${_classicReactors.size}`
698+
);
695699
}
696700
return () => {
697701
_classicReactors.delete(cb);
698-
if ((globalThis as any).__GXT_LEAK_DEBUG__) {
699-
_reactorMeta.delete(cb);
700-
}
702+
_reactorMeta.delete(cb);
701703
};
702704
}
703705

704706
function _fireClassicReactors() {
705707
if (_classicReactors.size === 0) return;
706-
// Copy to avoid mutation during iteration
708+
// Copy to avoid mutation during iteration (reactors may unsubscribe
709+
// themselves below, mutating _classicReactors).
707710
const snapshot = Array.from(_classicReactors);
708711
const debug = (globalThis as any).__GXT_LEAK_DEBUG__;
709-
const currentTest = debug ? _currentTestName() : '';
712+
const currentTest = _currentTestName();
710713
for (const cb of snapshot) {
711-
if (debug) {
712-
const meta = _reactorMeta.get(cb);
713-
if (meta) {
714+
const meta = _reactorMeta.get(cb);
715+
// Cross-test leak guard: reactors registered during a known test
716+
// that fire during a different known test are leaks. Unsubscribe
717+
// and skip the call. We only enforce this when both registration
718+
// and fire happen with a known test (i.e. neither is <no-test>);
719+
// module-init and pre-QUnit reactors are intentionally global.
720+
if (
721+
meta &&
722+
meta.registeredAtTest !== NO_TEST &&
723+
currentTest !== NO_TEST &&
724+
meta.registeredAtTest !== currentTest
725+
) {
726+
if (debug) {
714727
meta.fireCount++;
715-
// Cross-test leak: reactor registered during a different test
716-
// than the one currently executing.
717-
if (
718-
meta.registeredAtTest !== '<no-test>' &&
719-
currentTest !== '<no-test>' &&
720-
meta.registeredAtTest !== currentTest
721-
) {
722-
// eslint-disable-next-line no-console
723-
console.log(
724-
`[leak-debug] LEAK reactor #${meta.id} src=${meta.source} regAt="${meta.registeredAtTest}" firedIn="${currentTest}" fires=${meta.fireCount}`
725-
);
726-
}
728+
// eslint-disable-next-line no-console
729+
console.log(
730+
`[leak-debug] DROP reactor #${meta.id} src=${meta.source} regAt="${meta.registeredAtTest}" firedIn="${currentTest}" fires=${meta.fireCount}`
731+
);
727732
}
733+
_classicReactors.delete(cb);
734+
_reactorMeta.delete(cb);
735+
continue;
728736
}
737+
if (debug && meta) meta.fireCount++;
729738
try {
730739
cb();
731740
} catch {

0 commit comments

Comments
 (0)