[jhuleatt]

Summary

Fixes two cache lifecycle defects in SuspenseSubject and preloadObservable:

1. Source subscriptions were never released on unmount:
   • SuspenseSubject piped innerObservable through shareReplay(1) (refCount: false), which keeps the underlying Firebase listener attached indefinitely even after all subscribers unmount.
   • this._warmupSubscription was never unsubscribed inside _subscribe(), keeping refCount >= 1 permanently.
   • When _suspenseEnabled was true, _timeoutHandler was a no-op (setTimeout(() => {}, this._timeoutWindow)), leaking abandoned <Suspense> renders forever.
2. Errored cache entries were never evicted (#742, #485):
   • Once an observable emitted an error (e.g., permission-denied before sign-in or on sign-out), the errored SuspenseSubject remained in preloadedObservables under its observableId, causing subsequent mounts or <ErrorBoundary> retries to immediately replay the stale error.

Changes

• Replace shareReplay(1) with share({ connector: () => new ReplaySubject<T>(1), resetOnError: true, resetOnComplete: false, resetOnRefCountZero: () => timer(LISTENER_RELEASE_DELAY_MS).pipe(tap(onEvict)) }) so the shared Firebase listener stays warm across React <StrictMode> double-mounts (LISTENER_RELEASE_DELAY_MS = 1000ms) and detaches + evicts from preloadedObservables once no subscribers remain.
• Unsubscribe this._warmupSubscription in _subscribe() after attaching the real subscriber so ownership hands off without detaching the underlying listener.
• Replace the _suspenseEnabled no-op timeout with releaseWarmupWhenSettled, which waits until firstEmission has settled (this._hasValue || this._error, preserving #525's guarantee for slow initial reads) before releasing unclaimed warm-up subscriptions.
• Schedule setTimeout(onEvict, FIRST_LOAD_ERROR_EVICT_DELAY_MS) (1000ms) on stream error so remounts and <ErrorBoundary> retries start a fresh subscription (non-suspense mode only; in Suspense mode a first-load error stays cached, since React may retry the suspended render after any delay).
• Evict immediately when an error arrives after every consumer has unmounted and the warm-up subscription has closed (for example permission denied after sign-out), so the next mount never reads the stale error (Permission error when accessing a cached firestore collection after logout/login #485).
• Wrap Observables from another rxjs copy with from() so unsubscribe reaches the source; rxjs 7 operators cannot tear down an rxjs 6 Observable directly.

Release notes

• Listeners detach ~1s after the last unmount (The observable cache leaks subscriptions and is shared across SSR requests #790). A later remount shows loading or the fallback once and reads again.
• In non-suspense mode, an error retries on the next mount (Error recovery: no retry path once an observable cache entry errors #742). A remount within ~1s of an error on first render still shows that error once.
• An error that arrives after unmount, for example on sign-out, no longer reaches the next mount (Permission error when accessing a cached firestore collection after logout/login #485).
• In Suspense mode, an error on first load stays cached until reload, as before.
• Unused preloads are released after ~31s.
• Custom rxjs 6 Observables are torn down on unmount.

Refs #742 (Suspense first-load errors are not fixed)
Closes #485

[tyler-reitz]

Requesting changes for one thing: the fix needs rxjs 7.1.0 or later, and dependencies allows ^6.6.3 || ^7.0.1.

The function form of resetOnRefCountZero arrived in 7.1.0. On 7.0.x the function is treated as a truthy true and resets immediately; on 6.x share() takes no arguments and ignores the config. With this exact config on 6.6.3 and 7.0.1, onEvict never fires, so those installs keep the leak with no error. CI only installs 7.8.1.

Your call which way: raise the floor to ^7.1.0, which breaks rxjs 6 users and so belongs on v5 (where Closes won't fire), or keep the range and say in the body that the fix applies to 7.1+ only.

Everything else checks out. Each new test goes red against main's src/ and when its fix is reverted. Non-blocking:

• Three paths have no test: the 1s grace window (a 0ms delay stays green), releaseWarmupWhenSettled (an immediate return stays green), and the identity guard in onEvict (a plain delete(id) stays green). The guard is right: a late subscriber holding an evicted subject re-arms its timer, which would otherwise delete the newer entry.
• Errored entries used to stay cached and now clear after 1s, in suspense mode too since the timeout is unconditional. That's what #742 asks for, but it's a behavior change under fix:.
• The TODO in the value getter ("burn the cache entry until the timeout fires") is stale now.

[tyler-reitz]

share never sees an error here: catchError(() => empty()) upstream turns it into a completion, so this option never applies (flipping it to false leaves the suite green). Errored entries are evicted only by the setTimeout(onEvict) in tap's error handler. Worth saying so here, so nobody deletes that timeout as redundant.

[tyler-reitz]

Since errors arrive as completions, this covers every errored stream too, and a completion cancels any pending refcount timer and stops it being rescheduled. So entries for streams that complete normally (the first() in useFirestoreDocOnce and useFirestoreDocDataOnce) never leave preloadedObservables: the listener is gone, but the entry stays, which on a server is still per-user growth (#790). Intended?

[tyler-reitz]

The three untested paths, the comments and the TODO are all addressed, and each new test goes red when its fix is reverted. One blocker left, and it comes from how the rxjs floor installs rather than from the code.

Blocking: this is breaking, so the title wants a !. It also fails silently on rxjs 6: rxjs is in dependencies, so an rxjs 6 app installs cleanly with a nested 7.x while rxfire binds to the app's 6.x, and unsubscribing never reaches the source. In a scratch app, five remounts left 5 listeners attached on this PR versus 1 on main. Moving rxjs to peerDependencies at ^7.1.0 would turn that into an install error instead.

Non-blocking:

• The if (this._error) branch in _subscribe has no test. Swapping it for subscriber.complete() keeps the suite green, but then a mounted hook with initialData whose stream errors resubscribes in a loop (59 subscriptions in 3s).
• *Once entries still stay cached on the usual path: the component subscribes before first() completes, which cancels the warm-up timer, and the teardown only evicts on error. Also evicting on _isComplete there should cover it. Fine as a follow-up under #790.

[tyler-reitz]

Correction to my review above: on this PR's own build, rxjs and rxfire are inlined into dist, so reactfire's built-in hooks run on the bundled rxjs 7 and don't leak in rxjs 6 apps (useUser and useFirestoreDocData tore down 5 of 5). What I measured applies today only to apps passing their own rxjs 6 observables to useObservable. It becomes every hook once #838 externalizes rxjs and rxfire: with both merged, an rxjs 6.6.7 app leaks one listener per remount (1,2,3,4,5 after five cycles). The ask stands, a ! plus rxjs as a ^7.1.0 peer dependency, in whichever of the two lands first.

[tyler-reitz]

I'm withdrawing my ! / rxjs ^7.1.0 ask. I've pushed a narrower fix so this can ship as 4.3.1: eb7910f changes SuspenseSubject.ts, 0c7c91a adds a test that goes red if the if (this._error) branch is replaced with complete(), and a merge of main puts the package files back to main's rxjs range.

The rxjs floor: rxjs is inlined in dist, so the floor has no runtime effect, and it breaks TypeScript (TS2345) for rxjs 6 apps that pass their own Observable. Instead, a from() wrap tears down foreign Observables. A custom rxjs 6 source now leaves 0 subscriptions attached after 5 mount cycles, down from 5.

Blocker in d9d2a97: in Suspense mode, if React's retry render after a first-load error takes more than ~1s, the 1s eviction drops the error first. The fallback stays up, the source resubscribes every ~1.3s, and the Error Boundary is never reached. I reproduced this on React 18 and 19, in jsdom and in a production build in Chromium. main reaches the boundary 24/24; d9d2a97 reaches it 11/24 on React 18 and 17/24 on React 19.

The fix: Suspense mode skips only the 1s timer eviction; the sign-out (#485) and unmount evictions are unchanged. With the fix, Suspense reaches the boundary 24/24 and non-suspense 48/48. #790 is fixed: 0 of 5 listeners stay attached after unmount, against 5 of 5 on main. The release smoke test passes and CI is green.

Release notes are in the PR body. Suspense first-load errors are still not fixed, so the body now says Refs #742 instead of Closes #742.

[armando-navarro]

Hey, Tyler, here's the review you requested. Three things stand between this and my approve: one fix, and two decisions for you and Jeff. The rest is tests and inline comments.

1. A suspended component can get stuck on its fallback

A suspended component has not subscribed yet, so the 1s release timer does not count it. Steps to reproduce:

• A non-suspense component, or one with initialData, mounts on an id. The first value has not arrived yet.
• It unmounts. The 1s release timer starts.
• Within that second, a Suspense-mode component mounts on the same id and suspends.
• The timer fires. It unsubscribes from the source and evicts the entry.
• The first value arrives. Nothing is subscribed, so firstEmission never resolves.
• The suspended component stays on its fallback until an unrelated parent render recreates the entry.

On main the same steps render the value. It needs mixed modes on one id and a first read slower than 1s, which a cold Firestore listen can be.

The fix that worked for me, in resetOnRefCountZero:

• After the 1s timer, if firstEmission has not settled, wait for it.
• Then allow one more grace period before evicting.

With that change:

• The full suite passes.
• The grace-window test is unchanged.
• A test for the steps above goes from red to green.
• Trade-off: a source that never emits stays subscribed after an unmount until it emits. The warm-up subscription already behaves this way for an entry nobody subscribed to.
• A polling version also works, but it makes vi.runAllTimers() loop in app tests, so I dropped it.

2. Which eviction policy should preload-created entries get?

The 1s eviction of a first-load error follows the suspenseEnabled of the call that created the entry.

• preloadFirestoreDoc, preloadUser and two-argument preloadObservable create entries with false.
• A Suspense app that preloads a document and then gets permission denied is back in the race eb7910f closed: the error is evicted after 1s, and a retry after that suspends and reads again.
• I measured it. A preload-created entry is gone 1.3s after the error. A hook-created entry is still there.
• The reverse also happens. An entry created by a Suspense hook that errors before anyone subscribes is never evicted, so a non-suspense consumer of that id rethrows until reload.

I don't have a verified fix. Options I see:

• Let a later caller's flag update the entry's policy.
• Decide the policy per consumer at read time.
• Keep it as is and document it.

Either way, the release note "In Suspense mode, an error on first load stays cached until reload" needs to say it applies to entries the hook created.

3. The 30s release can drop a useInit* entry

useInitSdk runs the initializer in a per-instance useMemo and caches the promise under firebase-sdk:<name>:<app>. The entry has to outlive component instances. On main it did.

On the branch:

• If the init component renders in Suspense mode and never commits, the 30s warm-up release evicts the completed entry. That happens when the user leaves while the fallback is showing and comes back later, or when a sibling under the same boundary stays suspended past 30s.
• The next instance runs the initializer again. That was always true, but now nothing holds the first result.
• The initializer from docs/use.md (initializeFirestore then enableIndexedDbPersistence) rejects on a second run with "Firestore has already been started and persistence can no longer be enabled". The persistentLocalCache() form rejects too.
• The error boundary shows that until reload.
• I reproduced it with a harness that mirrors useInitSdk. Branch: error after 31.5s. main: renders.

The other way to lose the entry, an unmount while the initializer is still pending past 1s, is fixed by the change in section 1.

Options for this one, neither verified:

• Keep completed entries in Suspense mode at the warm-up release.
• Cache the initializer promise by sdkName:appName in useInitSdk.
• Accept the risk and say so in the release notes.

Tests

Five paths have no test. Each change below leaves the suite green:

• else if (!this._suspenseEnabled) changed to else. Suspense first-load errors get evicted and nothing fails.
• The eviction when the last subscriber of an errored entry unmounts, deleted. In Suspense mode it is the only code that evicts an error that arrived while mounted.
• resetOnComplete: false flipped to true. A mounted *Once entry re-reads.
• The clearTimeout at the top of _subscribe, deleted. The warm-up release then evicts a mounted one-shot entry at the next tick.
• The #525 assertion runs at 80ms, inside the 1s grace. A warm-up released before the first value still passes. Checking after the window plus the grace catches it.

I have these written, plus the regression test for section 1 and the two changes from the inline comments. I can push them as a branch if that helps.

One more release-note correction. Entries whose stream completes while mounted are never evicted. That covers the *Once hooks you noted, and for the same reason useIdTokenResult, useStorageDownloadURL, useCallableFunctionResponse and useInit*. "A later remount reads again" does not apply to them.

Smaller things are inline. If anything here is wrong, tell me and I'll run it again.

[armando-navarro]

The cast is not needed, Subscriber.next takes value?: T. It also hides a behavior change:

• A direct subscriber of what preloadObservable or preloadFirestoreDoc returns now gets next(undefined), or the last value a second time, and never completes on error.
• On main it completed, so firstValueFrom now resolves undefined where it used to reject.
• The hook is unaffected. Worth a line in the PR body for anyone who subscribes directly.

[armando-navarro]

This comment and the one at line 63 say the window gives a suspended first load time to retry. Since eb7910f the timer only runs when Suspense is off.

[armando-navarro]

resetOnError and the error: handler in _subscribe never run, because catchError(() => EMPTY) runs first. The comment above still describes a teardown on error.

[armando-navarro]

_innerSubscriber is written and never read. The @ts-expect-error above it now also hides that compiler error.

[armando-navarro]

from() already does this check and returns an rxjs 7 Observable unchanged. from(innerObservable).pipe( is enough.

[armando-navarro]

This re-arms until the entry settles. For a source that never emits it re-arms forever. Three effects:

• vi.runAllTimers() aborts with its infinite-loop error in any app test that renders a Suspense hook on a never-emitting mock. It completes on main.
• A Node process holding such an entry no longer exits on its own.
• The release happens at the next 30s tick, not 30s after the first value. A value at 31s is released at 60s.

Fix: release from firstEmission with one delayed timer instead of polling. The suite stays green. Also on the branch.

[armando-navarro]

Nothing uses the default, and the optional parameter lets a later caller skip eviction without a type error. Making it required costs nothing.

[armando-navarro]

If you want #742 covered in Suspense mode too, if (this._isComplete || this._error) here evicts a first-load error at the 30s mark, far outside React's retry window. The suite stays green with it. It reverses the decision in eb7910f, so I'm only flagging it as an option.

[armando-navarro]

v and e here, and the r in the new test waits: I ask for full names everywhere. value, error and resolve would do.

[armando-navarro]

lift will be made internal in rxjs 8. defer(() => { subscribeCount++; return base$; }) passes here and still fails at a 0ms delay. Also, the test at line 629 is the line 608 test without the mid-test emission.