🐛(frontend) stop reconnecting to the websocket based on the status code

The yhub server returns custom status code when the websocket is not
accessible, like 4401 when an access is removed and 4404 when a document
is deleted. the websocket client now use these custom status code to
stop reconnection forever.

CHANGELOG.md

@@ -6,6 +6,19 @@ and this project adheres to
 
 ## [Unreleased]
 
+### Fixed
+
+- 🐛(frontend) stop reconnecting to the collaboration server when it has refused
+  the connection for good. Close codes 4400-4499 are a refusal, not a lost
+  socket — the document was deleted (4404) or the access of this connection
+  changed (4401) — and retrying only asked the same question again, twice a
+  minute, for as long as the tab stayed open. The editor now stops and refetches
+  the document instead: it reconnects when the document is still there (an
+  access upgraded from reader to editor is a refusal too, and has to reconnect
+  to carry its new rights) and stays closed when it is not, where the page
+  already tells the user what happened. Everything else — a dropped socket, a
+  restart, an unreachable server — keeps its retry loop untouched
+
 ### Added
 
 - ✨(frontend) export presenter slides as PDF #2487

src/frontend/apps/impress/src/features/docs/doc-editor/hook/useCollaboration.tsx

@@ -25,6 +25,8 @@ export const useCollaboration = (room: string) => {
     isReady,
     hasLostConnection,
     resetLostConnection,
+    isPermanentlyClosed,
+    reconnect,
     pauseForInactivity,
     resumeFromInactivity,
   } = useProviderStore();
@@ -45,9 +47,6 @@ export const useCollaboration = (room: string) => {
    * When the provider detects a lost connection, we invalidate the document query to trigger a refetch.
    * Because it can be because the user has access to the document that are modified
    * (e.g., permissions changed, document deleted, user removed)
-   * TODO(yhub): this invalidation used to ride on the server-side kick
-   * (reset-connections); without a kick API a permission change no longer
-   * triggers a refetch until the connection drops for another reason.
    */
   useEffect(() => {
     if (hasLostConnection && room) {
@@ -58,6 +57,33 @@ export const useCollaboration = (room: string) => {
     }
   }, [hasLostConnection, room, queryClient, resetLostConnection]);
 
+  /**
+   * The collaboration server refused the connection for good and the retry loop
+   * stopped, so nothing will ask again on its own: this refetch is what asks.
+   *
+   * A refusal says the answer changed, not what it changed to. The document may
+   * be gone, our access to it revoked, or merely upgraded from reader to editor
+   * — the last one has to reconnect to carry the new rights. So the connection
+   * comes back only when the document does, and stays closed otherwise, where
+   * the query error puts the page in charge of telling the user why.
+   */
+  useEffect(() => {
+    if (!isPermanentlyClosed || !room) {
+      return;
+    }
+
+    void queryClient
+      .invalidateQueries({ queryKey: [KEY_DOC, { id: room }] })
+      .then(() => {
+        if (
+          queryClient.getQueryState([KEY_DOC, { id: room }])?.status ===
+          'success'
+        ) {
+          reconnect();
+        }
+      });
+  }, [isPermanentlyClosed, room, queryClient, reconnect]);
+
   /**
    * We add a broadcast task to reset the query cache
    * when the document visibility changes.

src/frontend/apps/impress/src/features/docs/doc-management/stores/__tests__/useProviderStore.test.tsx

@@ -0,0 +1,126 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+
+import { useProviderStore } from '../useProviderStore';
+
+/**
+ * A stand-in for y-websocket's provider, faithful on the two points these
+ * tests are about: the listeners it lets us register, and `shouldConnect`,
+ * which is what its retry loop reads before opening a socket again.
+ */
+class FakeProvider {
+  public shouldConnect = true;
+  public connect = vi.fn(() => {
+    this.shouldConnect = true;
+  });
+  public disconnect = vi.fn(() => {
+    this.shouldConnect = false;
+  });
+  public destroy = vi.fn();
+  public awareness = { destroy: vi.fn() };
+  public doc = { destroy: vi.fn() };
+
+  private listeners: Record<string, ((...args: unknown[]) => void)[]> = {};
+
+  on(event: string, listener: (...args: unknown[]) => void) {
+    (this.listeners[event] ??= []).push(listener);
+  }
+
+  emit(event: string, ...args: unknown[]) {
+    this.listeners[event]?.forEach((listener) => listener(...args));
+  }
+}
+
+let provider: FakeProvider;
+
+vi.mock('y-websocket', () => ({
+  // a function expression, not an arrow: the store builds it with `new`
+  WebsocketProvider: vi.fn(function () {
+    return provider;
+  }),
+}));
+
+const closeWith = (code: number) =>
+  provider.emit('connection-close', { code }, provider);
+
+describe('useProviderStore', () => {
+  beforeEach(() => {
+    vi.useFakeTimers();
+    provider = new FakeProvider();
+    // the store is a module-level singleton: put it back to its defaults, or
+    // a test reads what the one before it left behind
+    useProviderStore.getState().destroyProvider();
+    useProviderStore.getState().createProvider('ws://localhost', 'doc-id');
+  });
+
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
+  it('keeps reconnecting when the connection is merely lost', () => {
+    closeWith(1006);
+    vi.runAllTimers();
+
+    // y-websocket has scheduled its next attempt and nothing stops it
+    expect(provider.shouldConnect).toBe(true);
+    expect(useProviderStore.getState().isPermanentlyClosed).toBe(false);
+    // the document is refetched: the connection may have dropped because the
+    // access to it changed
+    expect(useProviderStore.getState().hasLostConnection).toBe(true);
+  });
+
+  it.each([
+    ['a deleted document', 4404],
+    ['a revoked access', 4401],
+  ])('stops reconnecting on %s', (_label, code) => {
+    closeWith(code);
+
+    // immediately, before the reconnection y-websocket has just scheduled
+    expect(provider.shouldConnect).toBe(false);
+
+    vi.runAllTimers();
+
+    // the backend is asked what became of the document, through this rather
+    // than through `hasLostConnection`: it decides whether to come back
+    expect(useProviderStore.getState().isPermanentlyClosed).toBe(true);
+    expect(useProviderStore.getState().hasLostConnection).toBe(false);
+    expect(useProviderStore.getState().isConnected).toBe(false);
+  });
+
+  it('keeps reconnecting on a transient error of the collaboration server', () => {
+    // 4500-4599 is its transient range, 1013 is "try again later"
+    closeWith(4503);
+    vi.runAllTimers();
+
+    expect(provider.shouldConnect).toBe(true);
+    expect(useProviderStore.getState().isPermanentlyClosed).toBe(false);
+  });
+
+  it('does not report a close it triggered itself as permanent', () => {
+    // `destroy()` and `disconnect()` emit the event with no close event
+    provider.emit('connection-close', null, provider);
+    vi.runAllTimers();
+
+    expect(useProviderStore.getState().isPermanentlyClosed).toBe(false);
+  });
+
+  it('reopens the connection when the document is still there', () => {
+    closeWith(4404);
+    vi.runAllTimers();
+
+    useProviderStore.getState().reconnect();
+
+    expect(provider.connect).toHaveBeenCalled();
+    expect(useProviderStore.getState().isPermanentlyClosed).toBe(false);
+  });
+
+  it('leaves a connection refused for good closed when the tab becomes active', () => {
+    closeWith(4404);
+    vi.runAllTimers();
+
+    useProviderStore.getState().pauseForInactivity();
+    useProviderStore.getState().resumeFromInactivity();
+
+    expect(provider.connect).not.toHaveBeenCalled();
+    expect(useProviderStore.getState().isPermanentlyClosed).toBe(true);
+  });
+});

src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx

@@ -20,7 +20,9 @@ export interface UseCollaborationStore {
   isSynced: boolean;
   hasLostConnection: boolean;
   isPausedForInactivity: boolean;
+  isPermanentlyClosed: boolean;
   resetLostConnection: () => void;
+  reconnect: () => void;
 }
 
 const defaultValues = {
@@ -30,6 +32,7 @@ const defaultValues = {
   isSynced: false,
   hasLostConnection: false,
   isPausedForInactivity: false,
+  isPermanentlyClosed: false,
 };
 
 /**
@@ -40,6 +43,21 @@ const defaultValues = {
  */
 const RECONNECT_JITTER_MAX_MS = 3000;
 
+/**
+ * Close codes 4400-4499 are the collaboration server refusing this connection
+ * rather than losing it: its access changed (4401) or the document was deleted
+ * (4404). It has answered, and reconnecting on a timer only asks the same
+ * question again — twice a minute, for as long as the tab stays open, for a
+ * document that may never come back. Everything else (a dropped socket, a
+ * server restart, an upgrade that failed) is transient and keeps its retry
+ * loop.
+ *
+ * Refused is not the same as gone: an access upgraded from reader to editor is
+ * a refusal too, and the connection has to be made again to carry the new
+ * rights. Asking the backend is what settles it, in `useCollaboration`.
+ */
+const isPermanentCloseCode = (code: number) => code >= 4400 && code <= 4499;
+
 let lostConnectionTimeout: ReturnType<typeof setTimeout> | undefined;
 
 export const useProviderStore = create<UseCollaborationStore>((set, get) => ({
@@ -80,7 +98,8 @@ export const useProviderStore = create<UseCollaborationStore>((set, get) => ({
 
     // Fires on every close AND every failed connection attempt
     // (an auth failure surfaces as an upgrade-level 401, close code 1006).
-    provider.on('connection-close', () => {
+    // The event is null when the socket was closed from here.
+    provider.on('connection-close', (event) => {
       // Skip when the disconnect was triggered by inactivity:
       // reconnection only happens once the user becomes active again.
       if (get().isPausedForInactivity) {
@@ -93,14 +112,30 @@ export const useProviderStore = create<UseCollaborationStore>((set, get) => ({
       clearTimeout(lostConnectionTimeout);
       // Jitter spreading: Math.random() generates a random delay to avoid
       // all clients invalidating their queries at the same time
+      const jitter = Math.random() * RECONNECT_JITTER_MAX_MS;
+
+      if (event && isPermanentCloseCode(event.code)) {
+        /**
+         * Stop the retry loop. Assigning `shouldConnect` rather than calling
+         * `disconnect()`: this runs inside y-websocket's own close handling,
+         * and `disconnect()` closes the socket that is already closing, which
+         * re-enters this listener. The reconnection it has just scheduled reads
+         * the flag back when it fires, and gives up.
+         */
+        provider.shouldConnect = false;
+        lostConnectionTimeout = setTimeout(
+          () => set({ isPermanentlyClosed: true }),
+          jitter,
+        );
+        return;
+      }
+
       lostConnectionTimeout = setTimeout(
         () => set({ hasLostConnection: true }),
-        Math.random() * RECONNECT_JITTER_MAX_MS,
+        jitter,
       );
     });
 
-    // TODO(yhub): re-add kick handling when yhub exposes a kick API (was onClose code 1000).
-
     set({
       provider,
     });
@@ -139,7 +174,20 @@ export const useProviderStore = create<UseCollaborationStore>((set, get) => ({
     }
     clearTimeout(lostConnectionTimeout);
     set({ isPausedForInactivity: false });
+    // a connection that was refused for good is only reopened by `reconnect`,
+    // once the backend has been asked again — becoming active is not an answer
+    if (get().isPermanentlyClosed) {
+      return;
+    }
     get().provider?.connect();
   },
   resetLostConnection: () => set({ hasLostConnection: false }),
+  /**
+   * Open the connection again after it was refused for good, once the backend
+   * has confirmed the document is still there to open.
+   */
+  reconnect: () => {
+    set({ isPermanentlyClosed: false });
+    get().provider?.connect();
+  },
 }));