From ce0ff0b1000449764c6ad3f1fa930c2e78635398 Mon Sep 17 00:00:00 2001 From: Manuel Raynaud Date: Wed, 12 Aug 2026 14:21:32 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B(frontend)=20stop=20reconnecting=20?= =?UTF-8?q?to=20the=20websocket=20based=20on=20the=20status=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 13 ++ .../docs/doc-editor/hook/useCollaboration.tsx | 32 ++++- .../__tests__/useProviderStore.test.tsx | 126 ++++++++++++++++++ .../stores/useProviderStore.tsx | 56 +++++++- 4 files changed, 220 insertions(+), 7 deletions(-) create mode 100644 src/frontend/apps/impress/src/features/docs/doc-management/stores/__tests__/useProviderStore.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index e5ee1017e..992cf7059 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useCollaboration.tsx b/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useCollaboration.tsx index eb26a0cb9..aa574204d 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-editor/hook/useCollaboration.tsx +++ b/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. diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/stores/__tests__/useProviderStore.test.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/stores/__tests__/useProviderStore.test.tsx new file mode 100644 index 000000000..f8b2e6980 --- /dev/null +++ b/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 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); + }); +}); diff --git a/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx b/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx index cba10a638..b5ee44bca 100644 --- a/src/frontend/apps/impress/src/features/docs/doc-management/stores/useProviderStore.tsx +++ b/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 | undefined; export const useProviderStore = create((set, get) => ({ @@ -80,7 +98,8 @@ export const useProviderStore = create((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((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((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(); + }, }));