Skip to content

Commit e68cb36

Browse files
lifeartclaude
andcommitted
fix(gxt-backend): cap classic reactor cross-test fires at 100
Diagnostic instrumentation in CI run on commit 70c2124 confirmed the leak signature: a classic-tag reactor registered in test A fires 1,000+ times during unrelated test B (and again during C, D, ...). A real-app reactor never legitimately survives the test that registered it; the lifetime cap at 10,000 (commit 29062f5) never trips because each leaked reactor's per-foreign-test fires stay under the lifetime threshold even though dozens of leaked reactors each contribute thousands of fires. This commit adds a per-foreign-test cap. ReactorMeta now tracks foreignFires + foreignTest; when a reactor fires in a test that differs from registeredAtTest, foreignFires increments (resetting when the foreign test changes or when execution returns to the registration test). At 100 foreignFires the reactor is unsubscribed unconditionally. 100 is well below the observed leak signature (1,000+ foreign fires per test) but well above any plausible legitimate cross-test reactivity (real-app reactors don't outlive their owning test). Tagging is now always-on (was diagnostic-only); the WeakMap put per registration is cheap and the cap depends on the metadata. Local cumulative reproducer: 17 CAP_FOREIGN events fired in 3 minutes, all hitting the cap at exactly 101 foreignFires before unsub. Smoke (14 modules / 333 tests) green. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
1 parent 70c2124 commit e68cb36

1 file changed

Lines changed: 72 additions & 36 deletions

File tree

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

Lines changed: 72 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -659,6 +659,16 @@ interface ReactorMeta {
659659
registeredAtTest: string;
660660
registeredAtTime: number;
661661
fireCount: number;
662+
// Number of fires in the CURRENT cross-test cycle (i.e., a test
663+
// that differs from registeredAtTest). Reset whenever the
664+
// foreign test changes. A reactor that fires this many times in
665+
// a foreign test is, by definition, leaked: real-app reactors
666+
// don't outlive the test that registered them.
667+
foreignFires: number;
668+
// The most recent foreign test name we counted fires for. When
669+
// _fireClassicReactors observes a different foreign test (or
670+
// returns to the registration test), foreignFires is reset.
671+
foreignTest: string;
662672
}
663673
let _reactorIdCounter = 0;
664674
const _reactorMeta = new WeakMap<() => void, ReactorMeta>();
@@ -677,15 +687,19 @@ function _currentTestName(): string {
677687

678688
export function registerClassicReactor(cb: () => void, source?: string): () => void {
679689
_classicReactors.add(cb);
690+
// Always track metadata — the cross-test foreign-fire cap below is
691+
// production behavior, not just diagnostic. Cheap (one WeakMap put).
692+
const id = ++_reactorIdCounter;
693+
_reactorMeta.set(cb, {
694+
id,
695+
source: source || '<unknown>',
696+
registeredAtTest: _currentTestName(),
697+
registeredAtTime: Date.now(),
698+
fireCount: 0,
699+
foreignFires: 0,
700+
foreignTest: '',
701+
});
680702
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-
});
689703
if (id <= 50 || id % 100 === 0) {
690704
// eslint-disable-next-line no-console
691705
console.log(
@@ -695,39 +709,40 @@ export function registerClassicReactor(cb: () => void, source?: string): () => v
695709
}
696710
return () => {
697711
_classicReactors.delete(cb);
698-
if ((globalThis as any).__GXT_LEAK_DEBUG__) {
699-
_reactorMeta.delete(cb);
700-
}
712+
_reactorMeta.delete(cb);
701713
};
702714
}
703715

704-
// Hard cap on per-reactor fires to break runaway loops. A leaked
705-
// classic-tag reactor whose self-unsub heuristic doesn't trip can
706-
// fire 10,000+ times across unrelated tests (diagnostic runs showed
707-
// a single reactor at 9,391 fires before the testem 1200s browser
708-
// timeout fired). The cap doesn't stop the leak — it stops the
709-
// runaway from saturating the runloop and hanging CI. Real-app
710-
// reactors fire bounded times per render path; 10,000 fires for a
711-
// single reactor across the lifetime of one page load is two orders
712-
// of magnitude above any legitimate bound.
713-
const REACTOR_FIRE_HARD_CAP = 10_000;
716+
// Hard cap on TOTAL fires (any test). Catches the rare case where a
717+
// reactor is in a tight self-fire loop within one test. 50,000 is far
718+
// above any plausible legitimate bound — CI evidence shows real
719+
// reactors fire ~1,000 times per test for renderComponent paths.
720+
const REACTOR_FIRE_HARD_CAP = 50_000;
721+
722+
// Cap on fires WITHIN A SINGLE foreign test (a test other than the
723+
// one that registered the reactor). This is the leak signature: a
724+
// reactor registered in test A that fires hundreds of times in test
725+
// B is by definition stale — real-app reactors don't survive their
726+
// owning test. CI evidence (commit 70c212414a run) showed leaked
727+
// reactors firing 1000+ times per foreign test, with 8-10 leaked
728+
// reactors active simultaneously. 100 is well below 1000 so it
729+
// catches the leak early; it's also above the maybe-50 fires a
730+
// real-app reactor might do during routing-test cross-context
731+
// cleanup, leaving headroom.
732+
const REACTOR_FOREIGN_FIRE_CAP = 100;
733+
const NO_TEST_SENTINEL = '<no-test>';
714734

715735
function _fireClassicReactors() {
716736
if (_classicReactors.size === 0) return;
717737
// Copy to avoid mutation during iteration
718738
const snapshot = Array.from(_classicReactors);
719739
const debug = (globalThis as any).__GXT_LEAK_DEBUG__;
720-
const currentTest = debug ? _currentTestName() : '';
740+
const currentTest = _currentTestName();
721741
for (const cb of snapshot) {
722742
const meta = _reactorMeta.get(cb);
723743
if (meta) {
724744
meta.fireCount++;
725745
if (meta.fireCount > REACTOR_FIRE_HARD_CAP) {
726-
// Self-destruct: this reactor has fired beyond any plausible
727-
// bound and is almost certainly leaked. Skip the call and
728-
// remove it from the registry so it stops contributing to
729-
// the runloop saturation that produces the testem 1200s
730-
// browser timeout.
731746
if (debug) {
732747
// eslint-disable-next-line no-console
733748
console.log(
@@ -738,19 +753,40 @@ function _fireClassicReactors() {
738753
_reactorMeta.delete(cb);
739754
continue;
740755
}
741-
if (debug) {
742-
// Cross-test leak: reactor registered during a different test
743-
// than the one currently executing.
744-
if (
745-
meta.registeredAtTest !== '<no-test>' &&
746-
currentTest !== '<no-test>' &&
747-
meta.registeredAtTest !== currentTest
748-
) {
756+
// Cross-test foreign-fire cap: count fires in a foreign test
757+
// (different from registeredAtTest). Reset when foreign test
758+
// changes (next test, or back to registration test).
759+
const isForeign =
760+
meta.registeredAtTest !== NO_TEST_SENTINEL &&
761+
currentTest !== NO_TEST_SENTINEL &&
762+
meta.registeredAtTest !== currentTest;
763+
if (isForeign) {
764+
if (meta.foreignTest !== currentTest) {
765+
meta.foreignTest = currentTest;
766+
meta.foreignFires = 0;
767+
}
768+
meta.foreignFires++;
769+
if (meta.foreignFires > REACTOR_FOREIGN_FIRE_CAP) {
770+
if (debug) {
771+
// eslint-disable-next-line no-console
772+
console.log(
773+
`[leak-debug] CAP_FOREIGN reactor #${meta.id} src=${meta.source} regAt="${meta.registeredAtTest}" firedIn="${currentTest}" foreignFires=${meta.foreignFires} — unsubscribing`
774+
);
775+
}
776+
_classicReactors.delete(cb);
777+
_reactorMeta.delete(cb);
778+
continue;
779+
}
780+
if (debug) {
749781
// eslint-disable-next-line no-console
750782
console.log(
751-
`[leak-debug] LEAK reactor #${meta.id} src=${meta.source} regAt="${meta.registeredAtTest}" firedIn="${currentTest}" fires=${meta.fireCount}`
783+
`[leak-debug] LEAK reactor #${meta.id} src=${meta.source} regAt="${meta.registeredAtTest}" firedIn="${currentTest}" fires=${meta.fireCount} foreignFires=${meta.foreignFires}`
752784
);
753785
}
786+
} else {
787+
// We're in registration test (or NO_TEST). Reset foreign tracking.
788+
meta.foreignTest = '';
789+
meta.foreignFires = 0;
754790
}
755791
}
756792
try {

0 commit comments

Comments
 (0)