From 2e7296d905a00487ffaef322fbb81d0b72ea4e52 Mon Sep 17 00:00:00 2001 From: Jonathan Tzeng Date: Thu, 30 Jul 2026 19:18:32 -0700 Subject: [PATCH] Add per-request timeout and failover to sync client A silently-dead sync server (one that accepts or drops the connection without ever responding) had no fetch timeout, so createRepo/readRepo/ updateRepo hung forever and the shuffled failover loop never advanced. This re-created the login-server outage on any dead server, prod included. Race each request against a configurable timeout (requestTimeoutMs, default 10s) so a dead server rejects with a NetworkError and the caller fails over to the next server within seconds. --- CHANGELOG.md | 2 + src/client/sync-client.ts | 42 ++++++++++++++++-- test/unit/sync-client.test.ts | 80 +++++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 3 deletions(-) create mode 100644 test/unit/sync-client.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 523816a..aadaee4 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/src/client/sync-client.ts b/src/client/sync-client.ts index e8f47f0..c8f6b05 100644 --- a/src/client/sync-client.ts +++ b/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 { 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[1] + ): Promise { + // Definite assignment: the Promise executor runs synchronously, so + // timeoutId is always set before Promise.race awaits. + let timeoutId!: ReturnType + const timeout = new Promise((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( request: ApiRequest, response: FetchResponse, diff --git a/test/unit/sync-client.test.ts b/test/unit/sync-client.test.ts new file mode 100644 index 0000000..30644a1 --- /dev/null +++ b/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) + }) +})