fix(connection): retry connect ping before unreachable (#1135)

src/hooks/useConnectionStatus.test.ts

@@ -1,4 +1,4 @@
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import { renderHook, act, waitFor } from '@testing-library/react';
 import { resetAuthStore } from '@/test/helpers/storeReset';
 import { useAuthStore } from '@/store/authStore';
@@ -27,6 +27,10 @@ beforeEach(() => {
   vi.mocked(pingWithCredentials).mockReset();
 });
 
+afterEach(() => {
+  vi.useRealTimers();
+});
+
 function seedDualAddressServer(): string {
   const id = useAuthStore.getState().addServer({
     name: 'Home',
@@ -57,6 +61,7 @@ describe('useConnectionStatus.isLan', () => {
   });
 
   it('falls back to public when only the public address answers', async () => {
+    vi.useFakeTimers();
     seedDualAddressServer();
     vi.mocked(pingWithCredentials).mockImplementation(async url =>
       url === 'https://music.example.com'
@@ -65,7 +70,12 @@ describe('useConnectionStatus.isLan', () => {
     );
 
     const { result } = renderHook(() => useConnectionStatus());
-    await waitFor(() => expect(result.current.status).toBe('connected'));
+    await act(async () => {
+      for (let i = 0; i < 2; i++) {
+        await vi.advanceTimersByTimeAsync(2000);
+      }
+    });
+    expect(result.current.status).toBe('connected');
     // primary url is `https://music.example.com` — public. isLanUrl alone
     // would have said `false` for the wrong reason (because the primary
     // happens to be public); the test is meaningful because the LAN side
@@ -126,9 +136,14 @@ describe('useConnectionStatus online event', () => {
         : { ok: false },
     );
 
+    vi.useFakeTimers();
     await act(async () => {
       window.dispatchEvent(new Event('online'));
+      for (let i = 0; i < 2; i++) {
+        await vi.advanceTimersByTimeAsync(2000);
+      }
     });
+    vi.useRealTimers();
 
     await waitFor(() => expect(result.current.isLan).toBe(false));
     // Both endpoints were probed (LAN refused, public answered).

src/utils/server/serverEndpoint.test.ts

@@ -1,4 +1,4 @@
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 
 vi.mock('../../api/subsonic', () => ({
   pingWithCredentials: vi.fn(),
@@ -43,6 +43,16 @@ function pingFail() {
   return { ok: false as const };
 }
 
+/** Initial connect ping + 2 retries (`serverEndpoint.ts`). */
+const CONNECT_PROBE_ATTEMPTS = 3;
+
+function mockDualAddressLanFailPublicOk() {
+  vi.mocked(pingWithCredentials).mockImplementation(async (url: string) => {
+    if (url === 'http://192.168.0.10') return pingFail();
+    return pingOk();
+  });
+}
+
 describe('normalizeServerBaseUrl', () => {
   it('strips a single trailing slash', () => {
     expect(normalizeServerBaseUrl('https://music.example.com/')).toBe(
@@ -205,6 +215,10 @@ describe('pickReachableBaseUrl', () => {
     vi.mocked(pingWithCredentials).mockReset();
   });
 
+  afterEach(() => {
+    vi.useRealTimers();
+  });
+
   it('returns the single endpoint when it pings ok and caches it', async () => {
     vi.mocked(pingWithCredentials).mockResolvedValue(pingOk());
     const result = await pickReachableBaseUrl(makeProfile({ url: 'https://music.example.com' }));
@@ -234,19 +248,34 @@ describe('pickReachableBaseUrl', () => {
   });
 
   it('falls through to the public endpoint when LAN ping fails', async () => {
-    vi.mocked(pingWithCredentials)
-      .mockResolvedValueOnce(pingFail())
-      .mockResolvedValueOnce(pingOk());
-    const result = await pickReachableBaseUrl(
+    vi.useFakeTimers();
+    mockDualAddressLanFailPublicOk();
+    const promise = pickReachableBaseUrl(
       makeProfile({
         url: 'https://music.example.com',
         alternateUrl: 'http://192.168.0.10',
       }),
     );
+    await vi.runAllTimersAsync();
+    const result = await promise;
+    expect(result.ok).toBe(true);
+    if (result.ok) expect(result.baseUrl).toBe('https://music.example.com');
+    expect(pingWithCredentials).toHaveBeenCalledTimes(CONNECT_PROBE_ATTEMPTS + 1);
+    expect(getCachedConnectBaseUrl('profile-1')).toBe('https://music.example.com');
+  });
+
+  it('retries a flaky endpoint before declaring it unreachable', async () => {
+    vi.useFakeTimers();
+    vi.mocked(pingWithCredentials)
+      .mockResolvedValueOnce(pingFail())
+      .mockResolvedValueOnce(pingOk());
+    const promise = pickReachableBaseUrl(makeProfile({ url: 'https://music.example.com' }));
+    await vi.runAllTimersAsync();
+    const result = await promise;
     expect(result.ok).toBe(true);
     if (result.ok) expect(result.baseUrl).toBe('https://music.example.com');
     expect(pingWithCredentials).toHaveBeenCalledTimes(2);
-    expect(getCachedConnectBaseUrl('profile-1')).toBe('https://music.example.com');
+    vi.useRealTimers();
   });
 
   it('returns unreachable and clears cache when every endpoint fails', async () => {
@@ -258,9 +287,13 @@ describe('pickReachableBaseUrl', () => {
 
     vi.mocked(pingWithCredentials).mockReset();
     vi.mocked(pingWithCredentials).mockResolvedValue(pingFail());
-    const result = await pickReachableBaseUrl(makeProfile({ url: 'https://music.example.com' }));
+    vi.useFakeTimers();
+    const unreachablePromise = pickReachableBaseUrl(makeProfile({ url: 'https://music.example.com' }));
+    await vi.runAllTimersAsync();
+    const result = await unreachablePromise;
     expect(result).toEqual({ ok: false, reason: 'unreachable' });
     expect(getCachedConnectBaseUrl('profile-1')).toBeNull();
+    expect(pingWithCredentials).toHaveBeenCalledTimes(CONNECT_PROBE_ATTEMPTS);
   });
 
   it('returns unreachable when the profile has no usable url', async () => {
@@ -341,10 +374,11 @@ describe('pickReachableBaseUrl', () => {
 
     // LAN now fails; public answers.
     vi.mocked(pingWithCredentials).mockClear();
-    vi.mocked(pingWithCredentials)
-      .mockResolvedValueOnce(pingFail())
-      .mockResolvedValueOnce(pingOk());
-    const result = await pickReachableBaseUrl(profile);
+    vi.useFakeTimers();
+    mockDualAddressLanFailPublicOk();
+    const fallbackPromise = pickReachableBaseUrl(profile);
+    await vi.runAllTimersAsync();
+    const result = await fallbackPromise;
     expect(result.ok).toBe(true);
     if (result.ok) expect(result.baseUrl).toBe('https://music.example.com');
     expect(getCachedConnectBaseUrl('profile-1')).toBe('https://music.example.com');
@@ -398,10 +432,11 @@ describe('subscribeConnectCache — connect-URL flip notifications', () => {
     expect(listener).toHaveBeenCalledTimes(1);
 
     // LAN drops, public answers → cached URL flips → another notification.
-    vi.mocked(pingWithCredentials)
-      .mockResolvedValueOnce(pingFail())
-      .mockResolvedValueOnce(pingOk());
-    await pickReachableBaseUrl(profile);
+    vi.useFakeTimers();
+    mockDualAddressLanFailPublicOk();
+    const flipPromise = pickReachableBaseUrl(profile);
+    await vi.runAllTimersAsync();
+    await flipPromise;
     expect(listener).toHaveBeenCalledTimes(2);
 
     unsubscribe();

src/utils/server/serverEndpoint.ts

@@ -248,13 +248,44 @@ export function invalidateReachableEndpointCache(profileId?: string): void {
   if (connectCache.delete(profileId)) notifyConnectCacheChanged();
 }
 
+/** Retries after a failed connect ping before trying the next endpoint / unreachable. */
+const CONNECT_PING_RETRIES = 2;
+const CONNECT_PING_RETRY_DELAY_MS = 2000;
+
+function sleepMs(ms: number): Promise<void> {
+  return new Promise(resolve => setTimeout(resolve, ms));
+}
+
+/**
+ * `pingWithCredentials` for connect probing — retries flaky links (packet loss,
+ * proxy TLS flakes) before the connection indicator marks the server down.
+ */
+async function pingWithConnectRetries(
+  baseUrl: string,
+  username: string,
+  password: string,
+): Promise<PingWithCredentialsResult> {
+  let ping = await pingWithCredentials(baseUrl, username, password);
+  if (ping.ok) return ping;
+  for (let retry = 0; retry < CONNECT_PING_RETRIES; retry++) {
+    await sleepMs(CONNECT_PING_RETRY_DELAY_MS);
+    ping = await pingWithCredentials(baseUrl, username, password);
+    if (ping.ok) return ping;
+  }
+  return ping;
+}
+
 /**
  * Sequentially ping the profile's endpoints (LAN-first), return the first one
  * that answers OK. Sticky: if a cached endpoint exists and is still in the
  * list, it's tried first; on failure, the cache entry is cleared and the full
  * sequence runs.
  *
- * Single-address profiles: one ping, identical to the legacy behavior.
+ * Each endpoint is probed with {@link pingWithConnectRetries} (initial ping +
+ * {@link CONNECT_PING_RETRIES} retries, {@link CONNECT_PING_RETRY_DELAY_MS} apart).
+ *
+ * Single-address profiles: one endpoint sequence, identical intent to legacy
+ * behavior aside from the retry cushion.
  */
 export async function pickReachableBaseUrl(
   profile: ServerProfile,
@@ -278,7 +309,7 @@ export async function pickReachableBaseUrl(
         : ordered;
 
     for (const endpoint of endpoints) {
-      const ping = await pingWithCredentials(endpoint.url, profile.username, profile.password);
+      const ping = await pingWithConnectRetries(endpoint.url, profile.username, profile.password);
       if (ping.ok) {
         const prev = connectCache.get(profile.id);
         connectCache.set(profile.id, endpoint.url);