Add per-request timeout and failover to sync client

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- added: Per-request timeout (configurable via `requestTimeoutMs`, default 10s) so a silently-dead sync server fails over to the next server instead of hanging forever.
+
 ## 0.2.9 (2024-02-26)
 
 - added: Detect conflicts while creating repos, and report these with a new `ConflictError` type.

src/client/sync-client.ts

@@ -3,7 +3,7 @@ import crossFetch from 'cross-fetch'
 import { FetchFunction, FetchResponse } from 'serverlet'
 
 import { EdgeServers } from '../types/base-types'
-import { ConflictError } from '../types/error'
+import { ConflictError, NetworkError } from '../types/error'
 import {
   asGetStoreResponse,
   asPostStoreBody,
@@ -36,10 +36,21 @@ export interface SyncClientOptions {
   fetch?: FetchFunction
   log?: (message: string) => void
   edgeServers?: EdgeServers
+  /** Per-request timeout in milliseconds before failing over to the next server. */
+  requestTimeoutMs?: number
 }
 
+// A silently-dead sync server (one that accepts or drops the connection without
+// ever responding) would otherwise hang the failover loop forever, so every
+// request fails over after this many milliseconds.
+const DEFAULT_REQUEST_TIMEOUT_MS = 10000
+
 export function makeSyncClient(opts: SyncClientOptions = {}): SyncClient {
-  const { fetch = crossFetch, log = () => {} } = opts
+  const {
+    fetch = crossFetch,
+    log = () => {},
+    requestTimeoutMs = DEFAULT_REQUEST_TIMEOUT_MS
+  } = opts
   const infoClient = makeInfoClient(opts)
 
   // Returns the sync servers from the info client shuffled
@@ -51,7 +62,7 @@ export function makeSyncClient(opts: SyncClientOptions = {}): SyncClient {
   async function loggedRequest(opts: ApiRequest): Promise<FetchResponse> {
     const { method, url, body, numbUrl = url, headers = {} } = opts
     const start = Date.now()
-    const response = await fetch(url, {
+    const response = await fetchWithTimeout(url, {
       method,
       headers: {
         'Content-Type': 'application/json',
@@ -64,6 +75,31 @@ export function makeSyncClient(opts: SyncClientOptions = {}): SyncClient {
     return response
   }
 
+  // Races the request against a timeout so a silently-dead server rejects with a
+  // NetworkError (letting the caller fail over to the next server) instead of
+  // hanging forever. Promise.race is used rather than an AbortSignal because the
+  // injected FetchFunction is not guaranteed to honor one.
+  async function fetchWithTimeout(
+    url: string,
+    fetchOptions: Parameters<FetchFunction>[1]
+  ): Promise<FetchResponse> {
+    // Definite assignment: the Promise executor runs synchronously, so
+    // timeoutId is always set before Promise.race awaits.
+    let timeoutId!: ReturnType<typeof setTimeout>
+    const timeout = new Promise<never>((resolve, reject) => {
+      timeoutId = setTimeout(() => {
+        reject(
+          new NetworkError(`Request timed out after ${requestTimeoutMs}ms`)
+        )
+      }, requestTimeoutMs)
+    })
+    try {
+      return await Promise.race([fetch(url, fetchOptions), timeout])
+    } finally {
+      clearTimeout(timeoutId)
+    }
+  }
+
   async function unpackResponse<T>(
     request: ApiRequest,
     response: FetchResponse,

test/unit/sync-client.test.ts

@@ -0,0 +1,80 @@
+import { expect } from 'chai'
+import { FetchHeaders, FetchResponse } from 'serverlet'
+
+import { makeSyncClient } from '../../src/client/sync-client'
+import { delay } from '../../src/util/delay'
+
+const syncKey = '0000000000000000000000000000000000000000'
+
+const fakeHeaders: FetchHeaders = {
+  forEach: () => {},
+  get: () => null,
+  has: () => false
+}
+
+// A successful PUT /store response. `asPutStoreResponse` is `asUndefined`, so an
+// empty body is a valid success.
+const okResponse: FetchResponse = {
+  headers: fakeHeaders,
+  ok: true,
+  status: 200,
+  arrayBuffer: async () => new ArrayBuffer(0),
+  json: async () => undefined,
+  text: async () => ''
+}
+
+describe('Unit: SyncClient timeout + failover', () => {
+  it('fails over to the next server when one hangs past the timeout', async () => {
+    const requestTimeoutMs = 50
+    let callCount = 0
+
+    const client = makeSyncClient({
+      // Empty infoServers keeps the server list deterministic (no network).
+      edgeServers: {
+        infoServers: [],
+        syncServers: ['https://sync-a.example', 'https://sync-b.example']
+      },
+      requestTimeoutMs,
+      fetch: async () => {
+        callCount += 1
+        // The first server accepts the connection but never responds within the
+        // timeout; the second server responds immediately.
+        if (callCount === 1) {
+          await delay(requestTimeoutMs * 10)
+          return okResponse
+        }
+        return okResponse
+      }
+    })
+
+    const start = Date.now()
+    await client.createRepo(syncKey)
+    const elapsed = Date.now() - start
+
+    // Both servers were tried: the first timed out, the second succeeded.
+    expect(callCount).to.equal(2)
+    // Failover happened after the timeout, not after the full hang.
+    expect(elapsed).to.be.greaterThan(requestTimeoutMs - 1)
+    expect(elapsed).to.be.lessThan(requestTimeoutMs * 10)
+  })
+
+  it('does not time out when the server responds promptly', async () => {
+    let callCount = 0
+    const client = makeSyncClient({
+      edgeServers: {
+        infoServers: [],
+        syncServers: ['https://sync-a.example', 'https://sync-b.example']
+      },
+      requestTimeoutMs: 50,
+      fetch: async () => {
+        callCount += 1
+        return okResponse
+      }
+    })
+
+    await client.createRepo(syncKey)
+
+    // The first server answered, so no failover was needed.
+    expect(callCount).to.equal(1)
+  })
+})