fix(client): guard Pro race-path teardown and log dropped core teardown (#3209)

Addresses PR #3576 review feedback on renderer-function mount cleanup:

- Pro ClientSideRenderer: guard the unmount-race teardown call the same way
  unmount() does, so a synchronously-throwing renderer teardown is logged
  rather than escaping render()'s outer catch and rejecting renderPromise
  with a misleading "encountered an error while rendering" error after the
  component is already unmounted.
- Core ClientRenderer: log (instead of silently dropping) an async renderer
  teardown that resolves after its mount was removed, so the documented
  best-effort limitation is diagnosable. Label synchronous renderer-teardown
  failures with the same "Error in renderer teardown:" message as the async
  path so the logs are greppable.
- Mark unmountAllComponents @internal instead of the contradictory @Private.
- Export RendererResult alongside RendererTeardown for consumer annotations.
- Document why invokeRendererTeardown is intentionally duplicated in Pro.
- Tests: add core unmount-race coverage and a Pro throwing-teardown race
  case; align a Pro test with the underscore-prefix convention.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: justin808

packages/react-on-rails-pro/src/ClientSideRenderer.ts

@@ -40,6 +40,11 @@ const REACT_ON_RAILS_STORE_ATTRIBUTE = 'data-js-react-on-rails-store';
 /**
  * Invokes a renderer teardown, swallowing async rejections so a failing teardown cannot produce an
  * unhandled promise rejection. Synchronous throws propagate to the caller's try/catch.
+ *
+ * Intentionally duplicated from the OSS `react-on-rails` ClientRenderer rather than imported: it is a
+ * tiny self-contained helper that the OSS module does not export, so duplicating keeps the Pro client
+ * renderer decoupled from OSS internals (no reliance on a non-public export) instead of widening the
+ * OSS public API just to share it.
  */
 function invokeRendererTeardown(teardown: RendererTeardown | undefined): void {
   if (!teardown) return;
@@ -158,8 +163,16 @@ class ComponentRenderer {
           // @ts-expect-error The state can change while awaiting delegateToRenderer
           if (this.state === 'unmounted') {
             // unmount() ran while the renderer was resolving and could not see the teardown yet, so
-            // run it now to avoid leaking the renderer's mount.
-            invokeRendererTeardown(delegation.teardown);
+            // run it now to avoid leaking the renderer's mount. Guard it like unmount() does (below)
+            // so a synchronously-throwing teardown is logged here rather than escaping to render()'s
+            // outer catch, which would rethrow it as a misleading "encountered an error while
+            // rendering" rejection even though the component is already unmounted.
+            try {
+              invokeRendererTeardown(delegation.teardown);
+            } catch (teardownError: unknown) {
+              const error = teardownError instanceof Error ? teardownError : new Error('Unknown error');
+              console.error('Error in renderer teardown:', error);
+            }
           } else {
             this.rendererTeardown = delegation.teardown;
             this.state = 'rendered';

packages/react-on-rails-pro/tests/ClientSideRenderer.test.ts

@@ -312,15 +312,10 @@ describe('ClientSideRenderer', () => {
   it('runs the teardown returned by a renderer when the component is unmounted', async () => {
     const teardown = jest.fn();
     const TestRenderer: RenderFunction = (
-      props?: Record<string, unknown>,
-      railsContext?: RailsContext,
-      domNodeId?: string,
-    ) => {
-      void props;
-      void railsContext;
-      void domNodeId;
-      return teardown;
-    };
+      _props?: Record<string, unknown>,
+      _railsContext?: RailsContext,
+      _domNodeId?: string,
+    ) => teardown;
     ComponentRegistry.register({ TestComponent: TestRenderer });
     const componentSpec = setupTestComponentDom('dom-id-teardown');
     addRailsContext();
@@ -408,4 +403,42 @@ describe('ClientSideRenderer', () => {
 
     expect(teardown).toHaveBeenCalledTimes(1);
   });
+
+  it('logs (and does not reject) when a raced async renderer teardown throws synchronously', async () => {
+    // Same race as above, but the resolved teardown throws synchronously. render() must guard the
+    // call (like unmount() does) so the throw is logged rather than escaping render()'s outer catch,
+    // which would reject renderPromise with a misleading "encountered an error while rendering" error
+    // after the component is already unmounted.
+    const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+    const teardownError = new Error('teardown boom');
+    const teardown = jest.fn(() => {
+      throw teardownError;
+    });
+    let resolveRenderer!: (value: () => void) => void;
+    const rendererPromise = new Promise<() => void>((resolve) => {
+      resolveRenderer = resolve;
+    });
+    const TestRenderer: RenderFunction = (
+      _props?: Record<string, unknown>,
+      _railsContext?: RailsContext,
+      _domNodeId?: string,
+    ) => rendererPromise;
+    ComponentRegistry.register({ TestComponent: TestRenderer });
+    const componentSpec = setupTestComponentDom('dom-id-teardown-race-throws');
+    addRailsContext();
+
+    const renderPromise = renderOrHydrateComponent(componentSpec);
+    await new Promise((resolve) => {
+      setTimeout(resolve, 0);
+    });
+
+    unmountAll();
+    resolveRenderer(teardown);
+
+    // The synchronous throw is caught and logged, so renderPromise resolves rather than rejecting.
+    await expect(renderPromise).resolves.toBeUndefined();
+    expect(teardown).toHaveBeenCalledTimes(1);
+    expect(consoleErrorSpy).toHaveBeenCalledWith('Error in renderer teardown:', teardownError);
+    consoleErrorSpy.mockRestore();
+  });
 });

packages/react-on-rails/src/ClientRenderer.ts

@@ -119,8 +119,9 @@ DELEGATING TO RENDERER ${name} for dom node with id: ${domNodeId} with props, ra
  * Known limitation (core package): if the mount is unmounted or its node is replaced *before* an
  * async renderer resolves its teardown, the still-pending teardown is dropped and that one mount
  * leaks — the resolved teardown is discarded because the entry is no longer the active mount for
- * this id (the `renderedRoots.get(domNodeId) === entry` guard below). Synchronous teardowns are
- * unaffected. The Pro client renderer (`react-on-rails-pro`) awaits the renderer and re-checks the
+ * this id (the `renderedRoots.get(domNodeId) === entry` guard below). The drop is logged via
+ * `console.error` so it is diagnosable rather than silent. Synchronous teardowns are unaffected.
+ * The Pro client renderer (`react-on-rails-pro`) awaits the renderer and re-checks the
  * unmount state, so it runs the teardown even when a navigation races the resolve; prefer Pro if you
  * depend on async renderer teardowns surviving fast navigations.
  */
@@ -133,10 +134,19 @@ function trackRendererMount(domNodeId: string, domNode: Element, result: unknown
   } else if (result && typeof (result as Promise<unknown>).then === 'function') {
     (result as Promise<unknown>)
       .then((resolved) => {
-        // Only attach if this exact entry is still the active mount for this id (it may have been
-        // replaced or unmounted while the renderer was resolving).
-        if (typeof resolved === 'function' && renderedRoots.get(domNodeId) === entry) {
+        if (typeof resolved !== 'function') return;
+        // Only attach if this exact entry is still the active mount for this id.
+        if (renderedRoots.get(domNodeId) === entry) {
           entry.teardown = resolved as RendererTeardown;
+        } else {
+          // The mount was unmounted or its node replaced before this async teardown resolved, so the
+          // entry is no longer the active mount and the teardown can't be attached — it is dropped
+          // and that one mount may leak on cleanup. Log it (rather than dropping silently) so the
+          // documented limitation above is diagnosable in production. Pro avoids this race entirely.
+          console.error(
+            `Renderer teardown for dom node "${domNodeId}" resolved after its mount was removed; ` +
+              'the teardown was dropped and that mount may leak on cleanup.',
+          );
         }
       })
       .catch((error: unknown) => {
@@ -283,14 +293,17 @@ export function reactOnRailsComponentLoaded(domId: string): Promise<void> {
 /**
  * Unmount all rendered React components, run all renderer-function teardowns, and clear roots.
  * This should be called on page unload to prevent memory leaks. Exported for testing.
- * @private
+ * @internal
  */
 export function unmountAllComponents(): void {
   renderedRoots.forEach((entry) => {
     try {
       teardownEntry(entry);
     } catch (error) {
-      console.error('Error unmounting component:', error);
+      // Use the same label as the async-rejection path so renderer-teardown failures are greppable
+      // whether the teardown threw synchronously (here) or rejected (invokeRendererTeardown).
+      const label = entry.kind === 'renderer' ? 'Error in renderer teardown:' : 'Error unmounting component:';
+      console.error(label, error);
     }
   });
   renderedRoots.clear();

packages/react-on-rails/src/types/index.ts

@@ -227,6 +227,7 @@ export type {
   AuthenticityHeaders,
   RenderFunction,
   RendererTeardown,
+  RendererResult,
   RenderFunctionResult,
   Store,
   StoreGenerator,

packages/react-on-rails/tests/ClientRenderer.test.ts

@@ -286,7 +286,9 @@ describe('ClientRenderer', () => {
       expect(() => unmountAllComponents()).not.toThrow();
       expect(throwingTeardown).toHaveBeenCalledTimes(1);
       expect(okTeardown).toHaveBeenCalledTimes(1);
-      expect(consoleErrorSpy).toHaveBeenCalledWith('Error unmounting component:', expect.any(Error));
+      // Renderer-owned teardown failures use the renderer label so they are greppable alongside the
+      // async-rejection path, regardless of whether the teardown threw synchronously or rejected.
+      expect(consoleErrorSpy).toHaveBeenCalledWith('Error in renderer teardown:', expect.any(Error));
 
       consoleErrorSpy.mockRestore();
     });
@@ -295,6 +297,7 @@ describe('ClientRenderer', () => {
       // Documents the core best-effort limitation (Pro handles this race): the first mount's async
       // teardown resolves only after the same-id node has been replaced, so it must NOT be attached
       // to or run against the new mount.
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
       const resolvers: Array<(teardown: () => void) => void> = [];
       const renderer = jest.fn();
       const AsyncRenderer: RenderFunction = (_props, _railsContext, _domNodeId) => {
@@ -322,9 +325,37 @@ describe('ClientRenderer', () => {
       resolvers[0](staleTeardown);
       await Promise.resolve();
 
-      // The stale teardown was not attached to the replaced mount, so cleanup never runs it.
+      // The stale teardown was not attached to the replaced mount, so cleanup never runs it. The
+      // drop is logged (not silent) so the documented limitation is diagnosable.
       unmountAllComponents();
       expect(staleTeardown).not.toHaveBeenCalled();
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        expect.stringContaining('resolved after its mount was removed'),
+      );
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('drops and logs an async teardown when unmount races the renderer resolving', async () => {
+      // Complements the happy-path async test above: here unmountAllComponents() fires BEFORE the
+      // renderer's promise resolves, which is the actual race the core package cannot win (Pro does).
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
+      const teardown = jest.fn();
+      const TestRenderer: RenderFunction = (_props, _railsContext, _domNodeId) => Promise.resolve(teardown);
+      ComponentRegistry.register({ TestRenderer });
+      setupRendererDom('renderer-async-race');
+
+      renderComponent('renderer-async-race');
+      // Unmount before the renderer's promise resolves, so the tracked entry is cleared first.
+      unmountAllComponents();
+
+      // Let the renderer's promise resolve; the teardown can no longer attach to the cleared entry.
+      await Promise.resolve();
+
+      expect(teardown).not.toHaveBeenCalled();
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        expect.stringContaining('resolved after its mount was removed'),
+      );
+      consoleErrorSpy.mockRestore();
     });
 
     it('logs (and does not rethrow) when an async renderer rejects before returning a teardown', async () => {