From cb0fe97afec5786dfa82ced5a7b2ccb5d3e559ec Mon Sep 17 00:00:00 2001 From: pchuri Date: Sun, 4 Oct 2026 23:38:43 +0900 Subject: [PATCH 1/3] fix(client): coordinate rate-limit backoff across concurrent requests Each request backed off on its own, so a burst of concurrent requests (page tree traversal runs ten at a time) that hit a low shared limit, such as 3 requests per second on Data Center, was rejected together, retried in lockstep, and exhausted its retries even though a patient client would have finished. #258 fixed immediate retries on Retry-After: 0 but not this. Add a RequestGate shared by every request of a client: - a throttled response pauses all requests, not only the retrying one; - a new throttle doubles the minimum spacing between request starts and successes halve it back, since capping concurrency alone does not help when responses arrive faster than the limit; - rejections from a burst that was already in flight are stale and neither widen the spacing again nor use up the request's own retries, with a cap on how many it may ride out. Without throttling nothing is paced or delayed. Retry counts, delays for a single request, and the error surfaced after the retries run out are unchanged. Refs #261 --- README.md | 1 + lib/confluence-client.js | 36 +++++- lib/request-gate.js | 121 ++++++++++++++++++++ tests/request-gate.test.js | 170 +++++++++++++++++++++++++++++ tests/traversal-rate-limit.test.js | 149 +++++++++++++++++++++++++ 5 files changed, 473 insertions(+), 4 deletions(-) create mode 100644 lib/request-gate.js create mode 100644 tests/request-gate.test.js create mode 100644 tests/traversal-rate-limit.test.js diff --git a/README.md b/README.md index 507ccaa..a05e7bc 100644 --- a/README.md +++ b/README.md @@ -886,6 +886,7 @@ Notes: - Continues on errors: failed pages are logged and the copy proceeds. - Exclude patterns use simple globbing: `*` matches any sequence, `?` matches any single character, and special regex characters are treated literally. - Large trees may take time; the CLI applies a small delay between sibling page creations to avoid rate limits (configurable via `--delay-ms`). +- If the server answers `429`/`503` (for example a Data Center instance limited to a few requests per second), the CLI backs off for all of its requests at once, honoring a positive `Retry-After`, and spaces requests further apart until the server accepts them, then speeds up again. Traversals such as `copy-tree`, `export` and `children --recursive` therefore finish under a low shared limit instead of failing after a few retries; without throttling nothing is slowed down. - Root title suffix defaults to ` (Copy)`; override with `--copy-suffix`. Child pages keep their original titles. - Use `--fail-on-error` to exit non-zero if any page fails to copy. diff --git a/lib/confluence-client.js b/lib/confluence-client.js index 0eaa8c6..fe89771 100644 --- a/lib/confluence-client.js +++ b/lib/confluence-client.js @@ -9,12 +9,17 @@ const { decodeHTML } = require('entities'); const MacroConverter = require('./macro-converter'); const { resolvePlantumlFormat } = require('./plantuml-format'); const { htmlToMarkdown, NAMED_ENTITIES } = require('./html-to-markdown'); +const { RequestGate } = require('./request-gate'); const { isJsonMode } = require('./output'); const WRITE_FORMATS = ['auto', 'storage', 'html', 'markdown']; const PAGE_LINK_LOOKUP_CONCURRENCY = 10; const RETRY_SAFE_METHODS = new Set(['get', 'head', 'options']); const MAX_RETRY_DELAY_MS = 60000; +// Throttled responses that were caused by a burst the request was already +// part of do not use up its retries; this caps how many of those it may ride +// out, so waiting stays bounded. +const MAX_STALE_RETRIES = 12; async function returnNullForNotFound(request) { try { @@ -155,8 +160,19 @@ class ConfluenceClient { this.maxRetries = 3; this.retryBaseDelayMs = 1000; + // Shared across every request this client makes, so a throttled response + // slows them all down instead of each retrying in lockstep. + this.gate = new RequestGate(); + this.client.interceptors.request.use(async config => { + await this.gate.acquire(config); + return config; + }); + this.client.interceptors.response.use( - response => response, + response => { + this.gate.reportSuccess(); + return response; + }, async error => { const requestConfig = error.config; const status = error.response?.status; @@ -172,9 +188,21 @@ class ConfluenceClient { typeof requestConfig.data?.pipe !== 'function' ) { const attempt = requestConfig.__retryCount || 0; - if (attempt < this.maxRetries) { - requestConfig.__retryCount = attempt + 1; - await this.sleep(this.retryDelayMs(error.response, attempt)); + const stale = requestConfig.__staleRetries || 0; + if (attempt < this.maxRetries && stale < MAX_STALE_RETRIES) { + // The backoff grows with throttles seen by the whole client, not + // just this request. A rejection from a burst this request was + // already part of (stale) waits like the others but keeps its + // retries for failures it causes itself. + const delayMs = this.retryDelayMs(error.response, this.gate.level); + const { fresh, until } = this.gate.reportThrottle(requestConfig, delayMs); + if (fresh) { + requestConfig.__retryCount = attempt + 1; + } else { + requestConfig.__staleRetries = stale + 1; + } + await this.sleep(delayMs); + this.gate.expire(until); return this.client.request(requestConfig); } } diff --git a/lib/request-gate.js b/lib/request-gate.js new file mode 100644 index 0000000..b3cf56a --- /dev/null +++ b/lib/request-gate.js @@ -0,0 +1,121 @@ +// Coordinates requests that share one rate-limited server. +// +// Each request used to back off on its own, so a burst of N concurrent +// requests that hit a low shared limit (Confluence Data Center can allow as +// little as 3 requests per second) was rejected N times over, retried in +// lockstep, rejected again, and ran out of retries even though a patient +// client would have finished. Capping concurrency does not fix that: a fast +// server answers at once, so even one request at a time arrives faster than +// the limit. The gate paces request *starts* instead, and shares what it +// learns across every request the client makes: +// +// - A throttled response pauses every request, not just the one that saw it. +// - A new throttle doubles the minimum spacing between request starts, and +// successes win the spacing back by halving it. A rejection from a burst +// that was already in flight when the spacing last changed is stale: it +// waits like the others but neither widens the spacing again nor uses up +// its own retries. +// - Without throttling there is no pause and no spacing: starts are released +// back to back, so concurrency and throughput are untouched and the gate +// only adds a microtask to each request. +// +// Waiting stays bounded: the spacing is capped, a request's own retries are +// capped by the caller, and so are the stale rejections it rides out. + +// Spacing applied after the first throttle, and its ceiling. +const INITIAL_INTERVAL_MS = 250; +const MAX_INTERVAL_MS = 10000; +// Below this the spacing is dropped altogether. +const MIN_INTERVAL_MS = 50; +// Successes in a row (since the last throttle) before the spacing is halved. +const RECOVERY_SUCCESSES = 10; + +// The gate waits with its own timer, not ConfluenceClient.sleep, which a +// caller may stub to skip the per-request backoff. +const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms)); + +class RequestGate { + constructor({ now = () => Date.now(), sleep = defaultSleep } = {}) { + this.now = now; + this.sleep = sleep; + this.intervalMs = 0; + this.lastStart = -Infinity; + // Requests start one at a time, in arrival order. + this.tail = Promise.resolve(); + this.pauseUntil = 0; + // Consecutive throttles without a success; scales the shared backoff. + this.level = 0; + // Bumped whenever the spacing widens. A request stamped with an older + // epoch was sent before the change that its rejection would cause. + this.epoch = 0; + this.successes = 0; + } + + // Wait for this request's turn to start, then for the shared pause and the + // spacing since the previous start. `config` is stamped so a rejection can + // later be told apart as new or stale. + async acquire(config) { + const previous = this.tail; + let done; + this.tail = new Promise((resolve) => { done = resolve; }); + try { + await previous; + // Track the time waited until rather than re-reading the clock, so the + // loop ends even if `sleep` returns early; it only repeats when the + // pause was extended while this request slept. + let startAt = this.now(); + for (;;) { + const target = Math.max(this.pauseUntil, this.lastStart + this.intervalMs); + if (target <= startAt) break; + await this.sleep(target - startAt); + startAt = target; + } + this.lastStart = Math.max(startAt, this.now()); + config.__gateEpoch = this.epoch; + } finally { + done(); + } + } + + // Record a throttled response that will be retried after `delayMs`. Returns + // `fresh` (whether it counts against the request's own retries) and `until` + // (the pause it set, for `expire`). + reportThrottle(config, delayMs) { + const fresh = config.__gateEpoch === this.epoch; + const until = Math.max(this.pauseUntil, this.now() + delayMs); + this.pauseUntil = until; + this.successes = 0; + if (fresh) { + this.epoch++; + this.level++; + this.intervalMs = this.intervalMs === 0 + ? INITIAL_INTERVAL_MS + : Math.min(this.intervalMs * 2, MAX_INTERVAL_MS); + } + return { fresh, until }; + } + + // The caller that set the pause has waited it out itself; clear it unless + // someone has extended it since. + expire(until) { + if (this.pauseUntil === until) this.pauseUntil = 0; + } + + reportSuccess() { + this.level = 0; + if (this.intervalMs === 0) return; + this.successes++; + if (this.successes < RECOVERY_SUCCESSES) return; + this.successes = 0; + const next = this.intervalMs / 2; + this.intervalMs = next < MIN_INTERVAL_MS ? 0 : next; + } +} + +module.exports = { + RequestGate, + INITIAL_INTERVAL_MS, + MAX_INTERVAL_MS, + MIN_INTERVAL_MS, + RECOVERY_SUCCESSES +}; diff --git a/tests/request-gate.test.js b/tests/request-gate.test.js new file mode 100644 index 0000000..9714cef --- /dev/null +++ b/tests/request-gate.test.js @@ -0,0 +1,170 @@ +const { + RequestGate, + INITIAL_INTERVAL_MS, + MAX_INTERVAL_MS, + MIN_INTERVAL_MS, + RECOVERY_SUCCESSES +} = require('../lib/request-gate'); + +// A gate on a virtual clock: `sleep` advances time instead of waiting. +const makeGate = () => { + const clock = { now: 0, sleeps: [] }; + const gate = new RequestGate({ + now: () => clock.now, + sleep: async (ms) => { + clock.sleeps.push(ms); + clock.now += ms; + } + }); + return { gate, clock }; +}; + +const start = async (gate) => { + const config = {}; + await gate.acquire(config); + return config; +}; + +describe('RequestGate (#261)', () => { + describe('without throttling', () => { + test('starts are released back to back with no wait', async () => { + const { gate, clock } = makeGate(); + const configs = await Promise.all(Array.from({ length: 50 }, () => start(gate))); + + expect(clock.sleeps).toEqual([]); + expect(clock.now).toBe(0); + expect(configs.every((config) => config.__gateEpoch === 0)).toBe(true); + }); + + test('successes change nothing', () => { + const { gate } = makeGate(); + for (let i = 0; i < 100; i++) gate.reportSuccess(); + expect(gate.intervalMs).toBe(0); + expect(gate.level).toBe(0); + }); + }); + + describe('a throttled response', () => { + test('pauses every later start, not only the retrying request', async () => { + const { gate, clock } = makeGate(); + const failed = await start(gate); + gate.reportThrottle(failed, 2000); + + const others = await Promise.all([start(gate), start(gate), start(gate)]); + + expect(clock.now).toBeGreaterThanOrEqual(2000); + expect(others).toHaveLength(3); + }); + + test('the first new throttle spaces starts and counts against the request', async () => { + const { gate } = makeGate(); + const config = await start(gate); + + const result = gate.reportThrottle(config, 1000); + + expect(result.fresh).toBe(true); + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); + expect(gate.level).toBe(1); + expect(gate.epoch).toBe(1); + }); + + test('rejections from the same burst are stale: they neither widen the spacing nor count', async () => { + const { gate } = makeGate(); + const burst = await Promise.all(Array.from({ length: 5 }, () => start(gate))); + + const results = burst.map((config) => gate.reportThrottle(config, 1000)); + + expect(results.map((r) => r.fresh)).toEqual([true, false, false, false, false]); + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); + expect(gate.level).toBe(1); + }); + + test('a rejection after the spacing changed is new again and doubles it', async () => { + const { gate } = makeGate(); + gate.reportThrottle(await start(gate), 1000); + gate.reportThrottle(await start(gate), 1000); + gate.reportThrottle(await start(gate), 1000); + + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS * 4); + expect(gate.level).toBe(3); + }); + + test('the spacing is capped', async () => { + const { gate } = makeGate(); + for (let i = 0; i < 20; i++) gate.reportThrottle(await start(gate), 1); + expect(gate.intervalMs).toBe(MAX_INTERVAL_MS); + }); + + test('starts are spaced by the interval', async () => { + const { gate, clock } = makeGate(); + gate.reportThrottle(await start(gate), 0); + gate.expire(gate.pauseUntil); + const times = []; + for (let i = 0; i < 4; i++) { + await start(gate); + times.push(clock.now); + } + + for (let i = 1; i < times.length; i++) { + expect(times[i] - times[i - 1]).toBeGreaterThanOrEqual(INITIAL_INTERVAL_MS); + } + }); + + test('expire clears the pause the caller set, but not one that was extended', async () => { + const { gate } = makeGate(); + const config = await start(gate); + const { until } = gate.reportThrottle(config, 1000); + gate.expire(until); + expect(gate.pauseUntil).toBe(0); + + const { until: first } = gate.reportThrottle(config, 1000); + gate.reportThrottle(config, 5000); + gate.expire(first); + expect(gate.pauseUntil).toBeGreaterThan(first); + }); + + test('a sleep that returns early cannot trap acquire in a loop', async () => { + const calls = []; + const gate = new RequestGate({ now: () => 0, sleep: async (ms) => { calls.push(ms); } }); + gate.reportThrottle(await start(gate), 1000); + + await start(gate); + + expect(calls.length).toBeLessThan(5); + }); + }); + + describe('recovery', () => { + test('a success resets the backoff level', async () => { + const { gate } = makeGate(); + gate.reportThrottle(await start(gate), 1000); + gate.reportSuccess(); + expect(gate.level).toBe(0); + }); + + test('the spacing is halved after enough successes in a row, then dropped', async () => { + const { gate } = makeGate(); + gate.reportThrottle(await start(gate), 0); + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); + + for (let i = 0; i < RECOVERY_SUCCESSES - 1; i++) gate.reportSuccess(); + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); + gate.reportSuccess(); + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS / 2); + + let guard = 0; + while (gate.intervalMs !== 0 && guard++ < 1000) gate.reportSuccess(); + expect(gate.intervalMs).toBe(0); + expect(INITIAL_INTERVAL_MS / 2 ** 3).toBeLessThan(MIN_INTERVAL_MS); + }); + + test('a throttle restarts the success count', async () => { + const { gate } = makeGate(); + gate.reportThrottle(await start(gate), 0); + for (let i = 0; i < RECOVERY_SUCCESSES - 1; i++) gate.reportSuccess(); + gate.reportThrottle(await start(gate), 0); + gate.reportSuccess(); + expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS * 2); + }); + }); +}); diff --git a/tests/traversal-rate-limit.test.js b/tests/traversal-rate-limit.test.js new file mode 100644 index 0000000..6aacee0 --- /dev/null +++ b/tests/traversal-rate-limit.test.js @@ -0,0 +1,149 @@ +const MockAdapter = require('axios-mock-adapter'); +const ConfluenceClient = require('../lib/confluence-client'); + +// A deterministic stand-in for a Data Center instance with a low shared rate +// limit. Time is virtual (jest fake timers), so a token bucket refilling at +// `ratePerSecond` is exact and the whole run takes milliseconds of real time. +describe('page tree traversal under a low shared rate limit (#261)', () => { + const buildTree = (fanouts) => { + const children = new Map(); + let nextId = 2; + const grow = (id, depth) => { + if (depth >= fanouts.length) return; + const ids = Array.from({ length: fanouts[depth] }, () => String(nextId++)); + children.set(id, ids); + ids.forEach((child) => grow(child, depth + 1)); + }; + grow('1', 0); + return { children, total: nextId - 2 }; + }; + + const startServer = (client, tree, { capacity, ratePerSecond, retryAfter = '0' }) => { + const stats = { ok: 0, rejected: 0, rejectedTimes: [] }; + let tokens = capacity; + let last = Date.now(); + const mock = new MockAdapter(client.client); + mock.onGet(/\/content\/\d+\/child\/page$/).reply((config) => { + const now = Date.now(); + tokens = Math.min(capacity, tokens + ((now - last) / 1000) * ratePerSecond); + last = now; + if (tokens < 1) { + stats.rejected++; + stats.rejectedTimes.push(now); + return [429, {}, { 'retry-after': retryAfter }]; + } + tokens -= 1; + stats.ok++; + const id = config.url.match(/content\/(\d+)\/child/)[1]; + const results = (tree.children.get(id) || []).map((childId) => ({ + id: childId, + title: `Page ${childId}`, + type: 'page', + space: { key: 'ENG' }, + version: { number: 1 } + })); + return [200, { results }]; + }); + return { stats, mock }; + }; + + // Advance virtual time until the promise settles, within a virtual budget. + const settle = async (promise, budgetMs = 10 * 60 * 1000) => { + let outcome = null; + promise.then((value) => { outcome = { value }; }, (error) => { outcome = { error }; }); + const start = Date.now(); + while (outcome === null && Date.now() - start < budgetMs) { + await jest.advanceTimersByTimeAsync(50); + } + if (outcome === null) throw new Error(`still running after ${budgetMs} virtual ms`); + return { ...outcome, elapsedMs: Date.now() - start }; + }; + + let client; + beforeEach(() => { + jest.useFakeTimers(); + client = new ConfluenceClient({ domain: 'wiki.example.org', token: 'test-token' }); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + + test('a 3 requests/second server does not exhaust retries on a 40-page tree', async () => { + const tree = buildTree([4, 3, 2]); + const { stats, mock } = startServer(client, tree, { capacity: 3, ratePerSecond: 3 }); + + const result = await settle(client.getAllDescendantPages('1', 10)); + mock.restore(); + + expect(result.error).toBeUndefined(); + expect(result.value).toHaveLength(tree.total); + expect(new Set(result.value.map((page) => page.id)).size).toBe(tree.total); + // Every page was listed once, and the rejections were a handful, not a storm. + expect(stats.ok).toBe(tree.total + 1); + expect(stats.rejected).toBeLessThan(tree.total / 2); + // Within a small multiple of the time the limit itself requires. + expect(result.elapsedMs).toBeLessThan((tree.total / 3) * 4 * 1000); + }); + + test.each([ + ['1 request/second with no burst', { capacity: 1, ratePerSecond: 1 }], + ['2 requests/second', { capacity: 2, ratePerSecond: 2 }], + ['a large burst but a low steady rate', { capacity: 10, ratePerSecond: 2 }], + ['a positive Retry-After', { capacity: 3, ratePerSecond: 3, retryAfter: '1' }], + ['no Retry-After header', { capacity: 3, ratePerSecond: 3, retryAfter: undefined }] + ])('completes a wide and deep tree under %s', async (_label, limits) => { + const tree = buildTree([6, 4, 3]); + const { stats, mock } = startServer(client, tree, limits); + + const result = await settle(client.getAllDescendantPages('1', 10)); + mock.restore(); + + expect(result.error).toBeUndefined(); + expect(result.value).toHaveLength(tree.total); + expect(stats.ok).toBe(tree.total + 1); + // No retry storm: far fewer rejected requests than pages. + expect(stats.rejected).toBeLessThan(tree.total); + }); + + test('without a limit nothing is paced or delayed', async () => { + const tree = buildTree([4, 3, 2]); + const { stats, mock } = startServer(client, tree, { capacity: 1000, ratePerSecond: 1000 }); + + const result = await settle(client.getAllDescendantPages('1', 10)); + mock.restore(); + + expect(result.value).toHaveLength(tree.total); + expect(stats.rejected).toBe(0); + // Done within the first 50 ms step of virtual time: no pause, no spacing. + expect(result.elapsedMs).toBeLessThanOrEqual(50); + }); + + test('a server that never lets a request through fails after bounded waiting', async () => { + const tree = buildTree([4, 3, 2]); + const { stats, mock } = startServer(client, tree, { capacity: 0, ratePerSecond: 0 }); + + const result = await settle(client.getAllDescendantPages('1', 10)); + mock.restore(); + + expect(result.error).toBeDefined(); + expect(result.error.response.status).toBe(429); + // The error surfaces as before, after a bounded number of requests and time. + expect(stats.ok).toBe(0); + expect(stats.rejected).toBeLessThan(100); + expect(result.elapsedMs).toBeLessThan(5 * 60 * 1000); + }); + + test('a throttled request makes concurrent requests wait for the same pause', async () => { + const { stats, mock } = startServer(client, buildTree([]), { capacity: 1, ratePerSecond: 1, retryAfter: '3' }); + + const results = await settle(Promise.all(Array.from({ length: 5 }, () => client.getChildPages('1')))); + mock.restore(); + + expect(results.error).toBeUndefined(); + // Nobody retried inside the 3 second window the server asked for. + expect(stats.rejectedTimes.length).toBeGreaterThan(0); + const first = stats.rejectedTimes[0]; + expect(stats.rejectedTimes.filter((time) => time > first && time < first + 3000)).toEqual([]); + }); +}); From 5e6178811bf4b8becf585e3a965cdfb80616399d Mon Sep 17 00:00:00 2001 From: pchuri Date: Sun, 4 Oct 2026 23:59:37 +0900 Subject: [PATCH 2/3] fix(client): bound rate-limit waiting and stop penalising non-429 retries Review of the coordinated backoff found two regressions against main: - A server that kept refusing made concurrent requests fail only after tens of minutes (7 s on main with ten in flight), because the shared pause and spacing grew with every retry of every request. - Throttling that does not depend on the request rate (a flaky proxy) ratcheted the spacing up and left the client slow for good. Changes: - The gate only handles 429; a 503 keeps its independent per-request retries exactly as before. - Only a positive Retry-After pauses all requests. Without one (Data Center sends 0) just the spacing between request starts changes. - The backoff exponent never exceeds what one request would reach alone. - A circuit opens after a run of new 429s with no success (maxRetries + 1 with a Retry-After, maxRetries + 4 without, since the spacing needs a few doublings to find the server's pace) and nothing more is retried or delayed until a request succeeds, so a persistent refusal fails about as fast as before. - A rejection counts against a request's own retries only when no request has succeeded since it first started; otherwise one unlucky request could run out of retries while the server serves everyone else. Such free retries are capped at six. - The spacing decays by a fixed factor per success, with no memory of the rate that caused a throttle, so isolated 429s leave no trace. Refs #261 --- README.md | 2 +- lib/confluence-client.js | 87 ++++++++++++------ lib/request-gate.js | 91 +++++++++++-------- tests/request-gate.test.js | 136 +++++++++++++++++++++-------- tests/traversal-rate-limit.test.js | 72 +++++++++++++++ 5 files changed, 289 insertions(+), 99 deletions(-) diff --git a/README.md b/README.md index a05e7bc..0f7609f 100644 --- a/README.md +++ b/README.md @@ -886,7 +886,7 @@ Notes: - Continues on errors: failed pages are logged and the copy proceeds. - Exclude patterns use simple globbing: `*` matches any sequence, `?` matches any single character, and special regex characters are treated literally. - Large trees may take time; the CLI applies a small delay between sibling page creations to avoid rate limits (configurable via `--delay-ms`). -- If the server answers `429`/`503` (for example a Data Center instance limited to a few requests per second), the CLI backs off for all of its requests at once, honoring a positive `Retry-After`, and spaces requests further apart until the server accepts them, then speeds up again. Traversals such as `copy-tree`, `export` and `children --recursive` therefore finish under a low shared limit instead of failing after a few retries; without throttling nothing is slowed down. +- If the server answers `429` (for example a Data Center instance limited to a few requests per second), the CLI paces its requests: a positive `Retry-After` makes every request wait, and requests are spaced further apart until the server accepts them, then speeded up again. Traversals such as `copy-tree`, `export` and `children --recursive` therefore finish under a low shared limit instead of failing after a few retries, and a server that keeps refusing still fails after about as long as before. Without throttling nothing is slowed down. A `503` on a read is retried per request, as before. - Root title suffix defaults to ` (Copy)`; override with `--copy-suffix`. Child pages keep their original titles. - Use `--fail-on-error` to exit non-zero if any page fails to copy. diff --git a/lib/confluence-client.js b/lib/confluence-client.js index fe89771..4e94794 100644 --- a/lib/confluence-client.js +++ b/lib/confluence-client.js @@ -16,10 +16,14 @@ const WRITE_FORMATS = ['auto', 'storage', 'html', 'markdown']; const PAGE_LINK_LOOKUP_CONCURRENCY = 10; const RETRY_SAFE_METHODS = new Set(['get', 'head', 'options']); const MAX_RETRY_DELAY_MS = 60000; -// Throttled responses that were caused by a burst the request was already -// part of do not use up its retries; this caps how many of those it may ride -// out, so waiting stays bounded. -const MAX_STALE_RETRIES = 12; +// Throttled responses that are not evidence against a request (it was part of +// a burst that was already in flight, or the server kept serving others) do +// not use up its retries; this caps how many of those it may ride out, so +// waiting stays bounded. +const MAX_FREE_RETRIES = 6; +// New 429s without a Retry-After, beyond what one request would retry, before +// the gate gives up (see RequestGate). +const EXTRA_FRESH_THROTTLES = 4; async function returnNullForNotFound(request) { try { @@ -188,22 +192,41 @@ class ConfluenceClient { typeof requestConfig.data?.pipe !== 'function' ) { const attempt = requestConfig.__retryCount || 0; - const stale = requestConfig.__staleRetries || 0; - if (attempt < this.maxRetries && stale < MAX_STALE_RETRIES) { - // The backoff grows with throttles seen by the whole client, not - // just this request. A rejection from a burst this request was - // already part of (stale) waits like the others but keeps its - // retries for failures it causes itself. - const delayMs = this.retryDelayMs(error.response, this.gate.level); - const { fresh, until } = this.gate.reportThrottle(requestConfig, delayMs); - if (fresh) { + const free = requestConfig.__freeRetries || 0; + if (attempt < this.maxRetries && free < MAX_FREE_RETRIES) { + if (status !== 429) { requestConfig.__retryCount = attempt + 1; - } else { - requestConfig.__staleRetries = stale + 1; + await this.sleep(this.retryDelayMs(error.response, attempt)); + return this.client.request(requestConfig); + } + // A 429 is shared with every other request of this client: the + // gate paces their starts and honors a positive Retry-After for + // all of them. The backoff grows with new 429s seen by the whole + // client but never past what one request would reach on its + // own. A rejection from a burst this request was already part of, + // or while the server was still serving others, waits like the + // rest but keeps its retries, and once the gate opens its circuit + // (a run of new 429s with no success) nothing more is retried. + const level = this.gate.level; + const pauseMs = this.retryAfterMs(error.response); + const { retry, fresh, progressed, until } = this.gate.reportThrottle(requestConfig, { + pauseMs, + // A server that names the wait is believed after as many + // failures as one request would have given up on. Without one + // the spacing needs a few doublings to find the server's pace. + maxFresh: this.maxRetries + (pauseMs > 0 ? 1 : EXTRA_FRESH_THROTTLES) + }); + if (retry) { + if (fresh && !progressed) { + requestConfig.__retryCount = attempt + 1; + } else { + requestConfig.__freeRetries = free + 1; + } + const exponent = Math.min(level, Math.max(0, this.maxRetries - 1)); + await this.sleep(this.retryDelayMs(error.response, exponent)); + this.gate.expire(until); + return this.client.request(requestConfig); } - await this.sleep(delayMs); - this.gate.expire(until); - return this.client.request(requestConfig); } } if (error.response?.status === 401) { @@ -2550,20 +2573,26 @@ class ConfluenceClient { * up all attempts within milliseconds. */ retryDelayMs(response, attempt) { - const retryAfter = response?.headers?.['retry-after']; - if (retryAfter !== undefined && retryAfter !== null && retryAfter !== '') { - const seconds = Number(retryAfter); - const waitMs = Number.isFinite(seconds) - ? seconds * 1000 - : Date.parse(retryAfter) - Date.now(); - // NaN (unparsable) and non-positive waits fall through to the backoff. - if (waitMs > 0) { - return Math.min(waitMs, MAX_RETRY_DELAY_MS); - } - } + const retryAfterMs = this.retryAfterMs(response); + if (retryAfterMs > 0) return retryAfterMs; return Math.min(this.retryBaseDelayMs * 2 ** attempt, MAX_RETRY_DELAY_MS); } + /** + * The wait a response asks for with Retry-After (seconds or HTTP-date), + * capped at the maximum delay; 0 when it names no usable wait. + */ + retryAfterMs(response) { + const retryAfter = response?.headers?.['retry-after']; + if (retryAfter === undefined || retryAfter === null || retryAfter === '') return 0; + const seconds = Number(retryAfter); + const waitMs = Number.isFinite(seconds) + ? seconds * 1000 + : Date.parse(retryAfter) - Date.now(); + // NaN (unparsable) and non-positive waits name no wait. + return waitMs > 0 ? Math.min(waitMs, MAX_RETRY_DELAY_MS) : 0; + } + /** * Accumulate results across start/limit pages until the server stops * returning a next link or `maxResults` is reached (null = unlimited). diff --git a/lib/request-gate.js b/lib/request-gate.js index b3cf56a..085f077 100644 --- a/lib/request-gate.js +++ b/lib/request-gate.js @@ -1,4 +1,4 @@ -// Coordinates requests that share one rate-limited server. +// Coordinates the requests of one client that share a rate-limited server. // // Each request used to back off on its own, so a burst of N concurrent // requests that hit a low shared limit (Confluence Data Center can allow as @@ -7,28 +7,33 @@ // client would have finished. Capping concurrency does not fix that: a fast // server answers at once, so even one request at a time arrives faster than // the limit. The gate paces request *starts* instead, and shares what it -// learns across every request the client makes: +// learns across every request the client makes. It only ever sees 429 +// responses; other retryable statuses keep their independent backoff. // -// - A throttled response pauses every request, not just the one that saw it. -// - A new throttle doubles the minimum spacing between request starts, and -// successes win the spacing back by halving it. A rejection from a burst -// that was already in flight when the spacing last changed is stale: it -// waits like the others but neither widens the spacing again nor uses up -// its own retries. +// - A positive Retry-After pauses every request, not just the one that saw +// it. Without one (Data Center answers 429 with `Retry-After: 0`) there is +// nothing to wait for, so only the spacing below changes. +// - A new 429 doubles the minimum spacing between request starts, and each +// success shrinks it again. A rejection from a burst that was already in +// flight when the spacing last changed is stale: it neither widens the +// spacing again nor uses up the request's own retries. +// - After `maxFresh` new 429s with no success in between, the gate opens a +// circuit: nothing more is retried or delayed until a request succeeds, so +// a server that keeps refusing fails as fast as a single request would. // - Without throttling there is no pause and no spacing: starts are released // back to back, so concurrency and throughput are untouched and the gate // only adds a microtask to each request. -// -// Waiting stays bounded: the spacing is capped, a request's own retries are -// capped by the caller, and so are the stale rejections it rides out. -// Spacing applied after the first throttle, and its ceiling. -const INITIAL_INTERVAL_MS = 250; -const MAX_INTERVAL_MS = 10000; -// Below this the spacing is dropped altogether. -const MIN_INTERVAL_MS = 50; -// Successes in a row (since the last throttle) before the spacing is halved. -const RECOVERY_SUCCESSES = 10; +// Spacing applied after the first throttle, its ceiling, and the factor each +// success applies to it. Below MIN_INTERVAL_MS the spacing is dropped. The +// decay is deliberately memoryless: a 429 that has nothing to do with the +// request rate (a flaky proxy) is forgotten within a handful of successes +// instead of leaving the client throttled, and against a real limit the +// spacing settles around the server's pace, crossing it now and then. +const INITIAL_INTERVAL_MS = 100; +const MAX_INTERVAL_MS = 5000; +const SUCCESS_DECAY = 0.9; +const MIN_INTERVAL_MS = 25; // The gate waits with its own timer, not ConfluenceClient.sleep, which a // caller may stub to skip the per-request backoff. @@ -43,15 +48,19 @@ class RequestGate { // Requests start one at a time, in arrival order. this.tail = Promise.resolve(); this.pauseUntil = 0; - // Consecutive throttles without a success; scales the shared backoff. + // New 429s since the last success; scales a request's own backoff and + // trips the circuit. this.level = 0; + this.open = false; // Bumped whenever the spacing widens. A request stamped with an older // epoch was sent before the change that its rejection would cause. this.epoch = 0; + // Successful responses so far; a request that sees this grow while it is + // throttled knows the server is still making progress for others. this.successes = 0; } - // Wait for this request's turn to start, then for the shared pause and the + // Wait for this request's turn to start, then for any shared pause and the // spacing since the previous start. `config` is stamped so a rejection can // later be told apart as new or stale. async acquire(config) { @@ -64,7 +73,7 @@ class RequestGate { // loop ends even if `sleep` returns early; it only repeats when the // pause was extended while this request slept. let startAt = this.now(); - for (;;) { + while (!this.open) { const target = Math.max(this.pauseUntil, this.lastStart + this.intervalMs); if (target <= startAt) break; await this.sleep(target - startAt); @@ -72,42 +81,54 @@ class RequestGate { } this.lastStart = Math.max(startAt, this.now()); config.__gateEpoch = this.epoch; + if (config.__gateSuccesses === undefined) config.__gateSuccesses = this.successes; } finally { done(); } } - // Record a throttled response that will be retried after `delayMs`. Returns - // `fresh` (whether it counts against the request's own retries) and `until` - // (the pause it set, for `expire`). - reportThrottle(config, delayMs) { + // Record a 429 that may be retried. `pauseMs` is a positive Retry-After + // (0 if there was none) and `maxFresh` how many new 429s in a row are + // allowed before the circuit opens. Returns `retry` (false once the circuit + // is open), `fresh` (a new 429 rather than one from a burst already in + // flight), `progressed` (some request has succeeded since this one first + // started, so the rejection is not evidence against it) and `until` (the + // shared pause it set, for `expire`). Only a fresh rejection with no + // progress should count against the request's own retries. + reportThrottle(config, { pauseMs = 0, maxFresh = Infinity } = {}) { const fresh = config.__gateEpoch === this.epoch; - const until = Math.max(this.pauseUntil, this.now() + delayMs); - this.pauseUntil = until; - this.successes = 0; + const progressed = this.successes > (config.__gateSuccesses ?? this.successes); + let until = 0; + if (pauseMs > 0) { + until = Math.max(this.pauseUntil, this.now() + pauseMs); + this.pauseUntil = until; + } if (fresh) { this.epoch++; this.level++; this.intervalMs = this.intervalMs === 0 ? INITIAL_INTERVAL_MS : Math.min(this.intervalMs * 2, MAX_INTERVAL_MS); + if (this.level >= maxFresh) { + this.open = true; + this.pauseUntil = 0; + } } - return { fresh, until }; + return { retry: !this.open, fresh, progressed, until }; } // The caller that set the pause has waited it out itself; clear it unless // someone has extended it since. expire(until) { - if (this.pauseUntil === until) this.pauseUntil = 0; + if (until && this.pauseUntil === until) this.pauseUntil = 0; } reportSuccess() { + this.successes++; this.level = 0; + this.open = false; if (this.intervalMs === 0) return; - this.successes++; - if (this.successes < RECOVERY_SUCCESSES) return; - this.successes = 0; - const next = this.intervalMs / 2; + const next = this.intervalMs * SUCCESS_DECAY; this.intervalMs = next < MIN_INTERVAL_MS ? 0 : next; } } @@ -117,5 +138,5 @@ module.exports = { INITIAL_INTERVAL_MS, MAX_INTERVAL_MS, MIN_INTERVAL_MS, - RECOVERY_SUCCESSES + SUCCESS_DECAY }; diff --git a/tests/request-gate.test.js b/tests/request-gate.test.js index 9714cef..cf1a3fb 100644 --- a/tests/request-gate.test.js +++ b/tests/request-gate.test.js @@ -3,7 +3,7 @@ const { INITIAL_INTERVAL_MS, MAX_INTERVAL_MS, MIN_INTERVAL_MS, - RECOVERY_SUCCESSES + SUCCESS_DECAY } = require('../lib/request-gate'); // A gate on a virtual clock: `sleep` advances time instead of waiting. @@ -41,14 +41,24 @@ describe('RequestGate (#261)', () => { for (let i = 0; i < 100; i++) gate.reportSuccess(); expect(gate.intervalMs).toBe(0); expect(gate.level).toBe(0); + expect(gate.open).toBe(false); }); }); - describe('a throttled response', () => { - test('pauses every later start, not only the retrying request', async () => { + describe('a 429', () => { + test('without a Retry-After spaces later starts but does not pause them', async () => { const { gate, clock } = makeGate(); - const failed = await start(gate); - gate.reportThrottle(failed, 2000); + gate.reportThrottle(await start(gate), { pauseMs: 0 }); + + await start(gate); + + // Only the spacing since the previous start, nowhere near a pause. + expect(clock.now).toBeLessThanOrEqual(INITIAL_INTERVAL_MS); + }); + + test('with a Retry-After pauses every later start, not only the retrying request', async () => { + const { gate, clock } = makeGate(); + gate.reportThrottle(await start(gate), { pauseMs: 2000 }); const others = await Promise.all([start(gate), start(gate), start(gate)]); @@ -58,11 +68,9 @@ describe('RequestGate (#261)', () => { test('the first new throttle spaces starts and counts against the request', async () => { const { gate } = makeGate(); - const config = await start(gate); - - const result = gate.reportThrottle(config, 1000); + const result = gate.reportThrottle(await start(gate)); - expect(result.fresh).toBe(true); + expect(result).toMatchObject({ retry: true, fresh: true }); expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); expect(gate.level).toBe(1); expect(gate.epoch).toBe(1); @@ -72,7 +80,7 @@ describe('RequestGate (#261)', () => { const { gate } = makeGate(); const burst = await Promise.all(Array.from({ length: 5 }, () => start(gate))); - const results = burst.map((config) => gate.reportThrottle(config, 1000)); + const results = burst.map((config) => gate.reportThrottle(config)); expect(results.map((r) => r.fresh)).toEqual([true, false, false, false, false]); expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); @@ -81,9 +89,9 @@ describe('RequestGate (#261)', () => { test('a rejection after the spacing changed is new again and doubles it', async () => { const { gate } = makeGate(); - gate.reportThrottle(await start(gate), 1000); - gate.reportThrottle(await start(gate), 1000); - gate.reportThrottle(await start(gate), 1000); + gate.reportThrottle(await start(gate)); + gate.reportThrottle(await start(gate)); + gate.reportThrottle(await start(gate)); expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS * 4); expect(gate.level).toBe(3); @@ -91,14 +99,13 @@ describe('RequestGate (#261)', () => { test('the spacing is capped', async () => { const { gate } = makeGate(); - for (let i = 0; i < 20; i++) gate.reportThrottle(await start(gate), 1); + for (let i = 0; i < 20; i++) gate.reportThrottle(await start(gate)); expect(gate.intervalMs).toBe(MAX_INTERVAL_MS); }); test('starts are spaced by the interval', async () => { const { gate, clock } = makeGate(); - gate.reportThrottle(await start(gate), 0); - gate.expire(gate.pauseUntil); + gate.reportThrottle(await start(gate)); const times = []; for (let i = 0; i < 4; i++) { await start(gate); @@ -113,20 +120,21 @@ describe('RequestGate (#261)', () => { test('expire clears the pause the caller set, but not one that was extended', async () => { const { gate } = makeGate(); const config = await start(gate); - const { until } = gate.reportThrottle(config, 1000); + const { until } = gate.reportThrottle(config, { pauseMs: 1000 }); gate.expire(until); expect(gate.pauseUntil).toBe(0); - const { until: first } = gate.reportThrottle(config, 1000); - gate.reportThrottle(config, 5000); + const { until: first } = gate.reportThrottle(config, { pauseMs: 1000 }); + gate.reportThrottle(config, { pauseMs: 5000 }); gate.expire(first); expect(gate.pauseUntil).toBeGreaterThan(first); + expect(gate.expire(0)).toBeUndefined(); }); test('a sleep that returns early cannot trap acquire in a loop', async () => { const calls = []; const gate = new RequestGate({ now: () => 0, sleep: async (ms) => { calls.push(ms); } }); - gate.reportThrottle(await start(gate), 1000); + gate.reportThrottle(await start(gate), { pauseMs: 1000 }); await start(gate); @@ -137,34 +145,94 @@ describe('RequestGate (#261)', () => { describe('recovery', () => { test('a success resets the backoff level', async () => { const { gate } = makeGate(); - gate.reportThrottle(await start(gate), 1000); + gate.reportThrottle(await start(gate)); gate.reportSuccess(); expect(gate.level).toBe(0); }); - test('the spacing is halved after enough successes in a row, then dropped', async () => { + test('each success shrinks the spacing until it is dropped', async () => { const { gate } = makeGate(); - gate.reportThrottle(await start(gate), 0); + gate.reportThrottle(await start(gate)); expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); - for (let i = 0; i < RECOVERY_SUCCESSES - 1; i++) gate.reportSuccess(); - expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS); gate.reportSuccess(); - expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS / 2); + expect(gate.intervalMs).toBeCloseTo(INITIAL_INTERVAL_MS * SUCCESS_DECAY); - let guard = 0; - while (gate.intervalMs !== 0 && guard++ < 1000) gate.reportSuccess(); + let successes = 1; + while (gate.intervalMs !== 0 && successes < 1000) { + gate.reportSuccess(); + successes++; + } expect(gate.intervalMs).toBe(0); - expect(INITIAL_INTERVAL_MS / 2 ** 3).toBeLessThan(MIN_INTERVAL_MS); + // About a dozen successes bring the first throttle's spacing back to nothing. + expect(successes).toBeLessThan(20); + expect(INITIAL_INTERVAL_MS * SUCCESS_DECAY ** successes).toBeLessThan(MIN_INTERVAL_MS); }); - test('a throttle restarts the success count', async () => { + test('a throttle with a Retry-After pauses and also reports progress only when others succeeded', async () => { const { gate } = makeGate(); - gate.reportThrottle(await start(gate), 0); - for (let i = 0; i < RECOVERY_SUCCESSES - 1; i++) gate.reportSuccess(); - gate.reportThrottle(await start(gate), 0); + const config = await start(gate); + expect(gate.reportThrottle(config).progressed).toBe(false); + + gate.reportSuccess(); + + expect(gate.reportThrottle(config).progressed).toBe(true); + }); + + test('isolated throttles do not ratchet the spacing up', async () => { + const { gate } = makeGate(); + // A throttle every 20 requests, as a flaky proxy might produce. + for (let round = 0; round < 50; round++) { + gate.reportThrottle(await start(gate)); + for (let i = 0; i < 20; i++) gate.reportSuccess(); + } + expect(gate.intervalMs).toBe(0); + }); + }); + + describe('the circuit', () => { + test('opens after maxFresh new throttles with no success in between', async () => { + const { gate } = makeGate(); + const results = []; + for (let i = 0; i < 4; i++) { + results.push(gate.reportThrottle(await start(gate), { maxFresh: 4 })); + } + + expect(results.map((r) => r.retry)).toEqual([true, true, true, false]); + expect(gate.open).toBe(true); + }); + + test('stale rejections do not count towards it', async () => { + const { gate } = makeGate(); + const burst = await Promise.all(Array.from({ length: 10 }, () => start(gate))); + + burst.forEach((config) => gate.reportThrottle(config, { maxFresh: 3 })); + + expect(gate.open).toBe(false); + expect(gate.level).toBe(1); + }); + + test('a success in between keeps it closed', async () => { + const { gate } = makeGate(); + for (let i = 0; i < 20; i++) { + gate.reportThrottle(await start(gate), { maxFresh: 3 }); + gate.reportThrottle(await start(gate), { maxFresh: 3 }); + gate.reportSuccess(); + } + expect(gate.open).toBe(false); + }); + + test('while open nothing is paced or paused, and a success closes it', async () => { + const { gate, clock } = makeGate(); + for (let i = 0; i < 3; i++) gate.reportThrottle(await start(gate), { pauseMs: 5000, maxFresh: 3 }); + expect(gate.open).toBe(true); + const before = clock.now; + + await Promise.all([start(gate), start(gate), start(gate)]); + expect(clock.now).toBe(before); + gate.reportSuccess(); - expect(gate.intervalMs).toBe(INITIAL_INTERVAL_MS * 2); + expect(gate.open).toBe(false); }); }); }); diff --git a/tests/traversal-rate-limit.test.js b/tests/traversal-rate-limit.test.js index 6aacee0..e4419d2 100644 --- a/tests/traversal-rate-limit.test.js +++ b/tests/traversal-rate-limit.test.js @@ -134,6 +134,78 @@ describe('page tree traversal under a low shared rate limit (#261)', () => { expect(result.elapsedMs).toBeLessThan(5 * 60 * 1000); }); + test.each([ + ['without Retry-After', undefined, 20 * 1000], + ['with Retry-After: 0', '0', 20 * 1000], + // Honoring a 60 s Retry-After three times is what one request did before. + ['with Retry-After: 60', '60', 4 * 60 * 1000] + ])('concurrent requests against a server that always refuses fail about as fast as before %s', async (_label, retryAfter, boundMs) => { + const { stats, mock } = startServer(client, buildTree([]), { capacity: 0, ratePerSecond: 0, retryAfter }); + const failures = []; + + const done = Promise.all( + Array.from({ length: 50 }, (_, i) => + client.client.get(`/content/${i}/child/page`).then(() => 'ok', (error) => { + failures.push(Date.now()); + return error.response.status; + }) + ) + ); + const outcome = await settle(done); + mock.restore(); + + expect(outcome.value.every((status) => status === 429)).toBe(true); + // Before this change the last of them failed after 7 s (3 retries of + // 1, 2 and 4 s) or three Retry-After waits, however many ran at once. + expect(outcome.elapsedMs).toBeLessThan(boundMs); + expect(stats.rejected).toBeLessThan(200); + }); + + test('503 responses keep their independent retries and never pace other requests', async () => { + // The first call of every fourth page fails with a 503; its retry succeeds. + const seen = new Set(); + const mock = new MockAdapter(client.client); + mock.onGet(/\/content\/\d+\/child\/page$/).reply((config) => { + const id = Number(config.url.match(/content\/(\d+)\/child/)[1]); + const firstCall = !seen.has(id); + seen.add(id); + return id % 4 === 0 && firstCall ? [503, {}] : [200, { results: [] }]; + }); + + const outcome = await settle(Promise.all(Array.from({ length: 40 }, (_, i) => client.getChildPages(String(i))))); + mock.restore(); + + expect(outcome.error).toBeUndefined(); + expect(client.gate.intervalMs).toBe(0); + expect(client.gate.epoch).toBe(0); + // Each 503 waited its own 1 s and nothing else did. + expect(outcome.elapsedMs).toBeLessThan(2000); + }); + + test('isolated 429s with a Retry-After do not leave the client throttled', async () => { + const tree = buildTree([6, 4, 3]); + let count = 0; + const mock = new MockAdapter(client.client); + mock.onGet(/\/content\/\d+\/child\/page$/).reply((config) => { + count++; + if (count % 25 === 0) return [429, {}, { 'retry-after': '1' }]; + const id = config.url.match(/content\/(\d+)\/child/)[1]; + return [200, { results: (tree.children.get(id) || []).map((childId) => ({ + id: childId, title: `Page ${childId}`, type: 'page', space: { key: 'ENG' }, version: { number: 1 } + })) }]; + }); + + const result = await settle(client.getAllDescendantPages('1', 10)); + mock.restore(); + + expect(result.error).toBeUndefined(); + expect(result.value).toHaveLength(tree.total); + expect(client.gate.open).toBe(false); + // Each throttle costs about its one-second Retry-After, nothing compounding. + const throttles = Math.floor(count / 25); + expect(result.elapsedMs).toBeLessThan((throttles + 2) * 1500); + }); + test('a throttled request makes concurrent requests wait for the same pause', async () => { const { stats, mock } = startServer(client, buildTree([]), { capacity: 1, ratePerSecond: 1, retryAfter: '3' }); From cc4bbbc2c93ea1c943a0cbb69e2329a5e464d961 Mon Sep 17 00:00:00 2001 From: pchuri Date: Mon, 5 Oct 2026 00:51:11 +0900 Subject: [PATCH 3/3] fix(client): tighten the rate-limit gate after re-review - Cap the spacing between request starts at 2 s (was 5 s) so recovery from a run of rate-independent 429s no longer takes minutes, and a persistent refusal fails closer to main's 7 s. - Do not keep a Retry-After pause requested by the 429 that opens the circuit, and clear it when a success closes the circuit; it used to come back and stall an unrelated later request. - Allow as many free (non-counting) retries as the request has of its own, so a page that is always refused fails after the same four attempts as before while the rest of the traversal is served. - Add regression tests for both. --- lib/confluence-client.js | 8 ++++---- lib/request-gate.js | 18 +++++++++++------- tests/request-gate.test.js | 16 ++++++++++++++++ tests/traversal-rate-limit.test.js | 27 +++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 11 deletions(-) diff --git a/lib/confluence-client.js b/lib/confluence-client.js index 4e94794..2be0920 100644 --- a/lib/confluence-client.js +++ b/lib/confluence-client.js @@ -18,9 +18,9 @@ const RETRY_SAFE_METHODS = new Set(['get', 'head', 'options']); const MAX_RETRY_DELAY_MS = 60000; // Throttled responses that are not evidence against a request (it was part of // a burst that was already in flight, or the server kept serving others) do -// not use up its retries; this caps how many of those it may ride out, so -// waiting stays bounded. -const MAX_FREE_RETRIES = 6; +// not use up its retries; this caps how many of those it may ride out, as +// many as its own retries, so a path that is always refused fails after the +// same number of attempts as before. // New 429s without a Retry-After, beyond what one request would retry, before // the gate gives up (see RequestGate). const EXTRA_FRESH_THROTTLES = 4; @@ -193,7 +193,7 @@ class ConfluenceClient { ) { const attempt = requestConfig.__retryCount || 0; const free = requestConfig.__freeRetries || 0; - if (attempt < this.maxRetries && free < MAX_FREE_RETRIES) { + if (attempt < this.maxRetries && free < this.maxRetries) { if (status !== 429) { requestConfig.__retryCount = attempt + 1; await this.sleep(this.retryDelayMs(error.response, attempt)); diff --git a/lib/request-gate.js b/lib/request-gate.js index 085f077..28e9f0c 100644 --- a/lib/request-gate.js +++ b/lib/request-gate.js @@ -31,7 +31,7 @@ // instead of leaving the client throttled, and against a real limit the // spacing settles around the server's pace, crossing it now and then. const INITIAL_INTERVAL_MS = 100; -const MAX_INTERVAL_MS = 5000; +const MAX_INTERVAL_MS = 2000; const SUCCESS_DECAY = 0.9; const MIN_INTERVAL_MS = 25; @@ -99,10 +99,6 @@ class RequestGate { const fresh = config.__gateEpoch === this.epoch; const progressed = this.successes > (config.__gateSuccesses ?? this.successes); let until = 0; - if (pauseMs > 0) { - until = Math.max(this.pauseUntil, this.now() + pauseMs); - this.pauseUntil = until; - } if (fresh) { this.epoch++; this.level++; @@ -111,9 +107,14 @@ class RequestGate { : Math.min(this.intervalMs * 2, MAX_INTERVAL_MS); if (this.level >= maxFresh) { this.open = true; - this.pauseUntil = 0; } } + // Nothing waits on a pause once the circuit is open, and one set now + // would come back to life when a later success closes it. + if (pauseMs > 0 && !this.open) { + until = Math.max(this.pauseUntil, this.now() + pauseMs); + this.pauseUntil = until; + } return { retry: !this.open, fresh, progressed, until }; } @@ -126,7 +127,10 @@ class RequestGate { reportSuccess() { this.successes++; this.level = 0; - this.open = false; + if (this.open) { + this.open = false; + this.pauseUntil = 0; + } if (this.intervalMs === 0) return; const next = this.intervalMs * SUCCESS_DECAY; this.intervalMs = next < MIN_INTERVAL_MS ? 0 : next; diff --git a/tests/request-gate.test.js b/tests/request-gate.test.js index cf1a3fb..4d6a804 100644 --- a/tests/request-gate.test.js +++ b/tests/request-gate.test.js @@ -222,6 +222,22 @@ describe('RequestGate (#261)', () => { expect(gate.open).toBe(false); }); + test('a pause requested by the throttle that opens it does not come back when a success closes it', async () => { + const { gate, clock } = makeGate(); + gate.reportThrottle(await start(gate), { pauseMs: 60000, maxFresh: 2 }); + const opened = gate.reportThrottle(await start(gate), { pauseMs: 60000, maxFresh: 2 }); + expect(opened.retry).toBe(false); + expect(gate.open).toBe(true); + + gate.reportSuccess(); + const before = clock.now; + await start(gate); + + expect(gate.pauseUntil).toBe(0); + // Only the short spacing between starts, not the 60 s pause. + expect(clock.now - before).toBeLessThan(1000); + }); + test('while open nothing is paced or paused, and a success closes it', async () => { const { gate, clock } = makeGate(); for (let i = 0; i < 3; i++) gate.reportThrottle(await start(gate), { pauseMs: 5000, maxFresh: 3 }); diff --git a/tests/traversal-rate-limit.test.js b/tests/traversal-rate-limit.test.js index e4419d2..c5924b8 100644 --- a/tests/traversal-rate-limit.test.js +++ b/tests/traversal-rate-limit.test.js @@ -161,6 +161,33 @@ describe('page tree traversal under a low shared rate limit (#261)', () => { expect(stats.rejected).toBeLessThan(200); }); + test.each([ + ['Retry-After: 0', '0', 15 * 1000], + ['Retry-After: 5', '5', 60 * 1000] + ])('one page that is always refused fails after the same attempts as before while the others succeed (%s)', async (_label, retryAfter, boundMs) => { + const tree = buildTree([6, 4, 3]); + const stuck = tree.children.get('1')[0]; + const attempts = new Map(); + const mock = new MockAdapter(client.client); + mock.onGet(/\/content\/\d+\/child\/page$/).reply((config) => { + const id = config.url.match(/content\/(\d+)\/child/)[1]; + attempts.set(id, (attempts.get(id) || 0) + 1); + if (id === stuck) return [429, {}, { 'retry-after': retryAfter }]; + return [200, { results: (tree.children.get(id) || []).map((childId) => ({ + id: childId, title: `Page ${childId}`, type: 'page', space: { key: 'ENG' }, version: { number: 1 } + })) }]; + }); + + const result = await settle(client.getAllDescendantPages('1', 10)); + mock.restore(); + + // The traversal fails on that page as it did before, with the same four attempts, not hanging. + expect(result.error).toBeDefined(); + expect(result.error.response.status).toBe(429); + expect(attempts.get(stuck)).toBe(4); + expect(result.elapsedMs).toBeLessThan(boundMs); + }); + test('503 responses keep their independent retries and never pace other requests', async () => { // The first call of every fourth page fails with a 503; its retry succeeds. const seen = new Set();