fix(engine): widen 429 backoff budget, correct the rate-limit explanation
Measured directly against the live forge: nginx never sends a Retry-After header on its 429s (the branch handling it is dead code in practice), the limit is a leaky bucket rather than a fixed ban (a 20-25 request burst took up to ~15s to drain), it is IP-based rather than token-quota-based (a valid FORGEJO_TOKEN made no difference to a reproduced burst), and it triggers well below "13 calls in a loop" — 20 concurrent anonymous requests reproduced it directly. The old default (retries: 3, ~7s worst case) was tuned for a hard ban that doesn't exist. fetchWithRetry now defaults to retries: 5 with a maxDelayMs: 8000 cap (23s worst case), covering the measured drain time without one attempt blocking for a full uncapped exponential step. CLAUDE.md's explanation is corrected to match; test count in README/CLAUDE.md updated for the two new tests (111 -> 113). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W1ZJFViVYpr8cvf4fs91j1
This commit is contained in:
parent
568b8e374a
commit
e6b0f04021
4 changed files with 61 additions and 9 deletions
24
CLAUDE.md
24
CLAUDE.md
|
|
@ -56,11 +56,23 @@ would recreate, in data, exactly the drift this plugin exists to remove.
|
||||||
INSTALL-TRUTH is the other (added after this used to say "one call" — that
|
INSTALL-TRUTH is the other (added after this used to say "one call" — that
|
||||||
line went stale and stayed stale until a 13-repo shell loop trusted it and
|
line went stale and stayed stale until a 13-repo shell loop trusted it and
|
||||||
tripped the rate limiter at 26 requests). Both go through `fetchWithRetry`,
|
tripped the rate limiter at 26 requests). Both go through `fetchWithRetry`,
|
||||||
which honors `Retry-After` on HTTP 429 rather than silently reporting SKIP.
|
which retries HTTP 429 rather than silently reporting SKIP. Both are
|
||||||
Both are anonymous — no token — so the gate works for any reader, not only
|
anonymous — no token, confirmed no different with one — so the gate works
|
||||||
someone holding one. A sweep across every repo still does not belong here:
|
for any reader, not only someone holding one. A sweep across every repo
|
||||||
it needs the listing fetched once, not once per invocation, which is a
|
still does not belong here: it needs the listing fetched once, not once per
|
||||||
different shape of caller (org-ops), not a flag on this engine.
|
invocation, which is a different shape of caller (org-ops), not a flag on
|
||||||
|
this engine.
|
||||||
|
**The "13 calls in a loop" explanation was incomplete** (2026-08-04): the
|
||||||
|
forge's nginx never sends `Retry-After` on its 429s (measured directly), so
|
||||||
|
`fetchWithRetry` always falls back to exponential backoff — the
|
||||||
|
`Retry-After` branch is live code with no live path yet. The limit is also
|
||||||
|
smaller than "loop of 13" implied: 20 concurrent requests from one IP
|
||||||
|
reproduced it directly, no loop needed, and a single well-formed 2-call
|
||||||
|
invocation can still lose if something else on the same IP is calling the
|
||||||
|
forge at the same moment (other repos' hooks, another session). The block
|
||||||
|
is a leaky bucket, not a fixed ban — a 20-25 request burst took up to ~15s
|
||||||
|
to fully drain. `fetchWithRetry` defaults to `retries: 5` /
|
||||||
|
`maxDelayMs: 8000` (23s worst case) to cover that.
|
||||||
- **Codepoints, not bytes, not UTF-16 units.** Use `[...s].length`. An em-dash
|
- **Codepoints, not bytes, not UTF-16 units.** Use `[...s].length`. An em-dash
|
||||||
exposes only the byte layer; astral characters expose the rest.
|
exposes only the byte layer; astral characters expose the rest.
|
||||||
- **The reader decides a link's level, not just what is required.** Root
|
- **The reader decides a link's level, not just what is required.** Root
|
||||||
|
|
@ -77,7 +89,7 @@ would recreate, in data, exactly the drift this plugin exists to remove.
|
||||||
## Commands
|
## Commands
|
||||||
|
|
||||||
```bash
|
```bash
|
||||||
npm test # 111 tests
|
npm test # 113 tests
|
||||||
node scripts/repo-standard-check.mjs --dir "$PWD" # gate one repo
|
node scripts/repo-standard-check.mjs --dir "$PWD" # gate one repo
|
||||||
node scripts/repo-standard-check.mjs --offline # no network call
|
node scripts/repo-standard-check.mjs --offline # no network call
|
||||||
node scripts/repo-standard-check.mjs --json # machine output
|
node scripts/repo-standard-check.mjs --json # machine output
|
||||||
|
|
|
||||||
|
|
@ -167,7 +167,7 @@ without that, a raw scan turns three dead names into about twenty.
|
||||||
npm test
|
npm test
|
||||||
```
|
```
|
||||||
|
|
||||||
111 tests over the pure classifiers. The reference fixtures are measured false
|
113 tests over the pure classifiers. The reference fixtures are measured false
|
||||||
positives, each with its expected verdict — the six that produced the
|
positives, each with its expected verdict — the six that produced the
|
||||||
three-outcome reference rule, plus the noise sources found by running the gate
|
three-outcome reference rule, plus the noise sources found by running the gate
|
||||||
against a real repository: regexes inside code spans that are markdown links to
|
against a real repository: regexes inside code spans that are markdown links to
|
||||||
|
|
|
||||||
|
|
@ -827,12 +827,27 @@ export function loadRegister(path = REGISTER_PATH) {
|
||||||
// a false SKIP, which this repo's own rule says is never a pass.
|
// a false SKIP, which this repo's own rule says is never a pass.
|
||||||
const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
|
const defaultSleep = (ms) => new Promise((resolve) => setTimeout(resolve, ms));
|
||||||
|
|
||||||
export async function fetchWithRetry(url, options, { fetchImpl = fetch, retries = 3, baseDelayMs = 1000, sleep = defaultSleep } = {}) {
|
// Measured 2026-08-04 against the live forge: nginx never sends a
|
||||||
|
// `Retry-After` header on its 429s, so the exponential fallback below is the
|
||||||
|
// ONLY path that ever actually runs — the branch above it is dead in
|
||||||
|
// practice, kept only because a future proxy config could add the header.
|
||||||
|
// The 429 itself is a leaky-bucket burst limit, not a fixed-duration ban: a
|
||||||
|
// 20-25 request burst took up to ~15s to fully drain, and a 20s pause always
|
||||||
|
// cleared it. `retries: 3` (7s worst case) was tuned for a hard ban that
|
||||||
|
// turned out not to exist; `retries: 5` with `maxDelayMs: 8000` (23s worst
|
||||||
|
// case) covers the measured drain time without one attempt blocking minutes.
|
||||||
|
export async function fetchWithRetry(
|
||||||
|
url,
|
||||||
|
options,
|
||||||
|
{ fetchImpl = fetch, retries = 5, baseDelayMs = 1000, maxDelayMs = 8000, sleep = defaultSleep } = {},
|
||||||
|
) {
|
||||||
for (let attempt = 0; ; attempt += 1) {
|
for (let attempt = 0; ; attempt += 1) {
|
||||||
const res = await fetchImpl(url, options);
|
const res = await fetchImpl(url, options);
|
||||||
if (res.status !== 429 || attempt >= retries) return res;
|
if (res.status !== 429 || attempt >= retries) return res;
|
||||||
const retryAfter = Number(res.headers?.get?.('retry-after'));
|
const retryAfter = Number(res.headers?.get?.('retry-after'));
|
||||||
const delayMs = Number.isFinite(retryAfter) && retryAfter > 0 ? retryAfter * 1000 : baseDelayMs * 2 ** attempt;
|
const delayMs = Number.isFinite(retryAfter) && retryAfter > 0
|
||||||
|
? retryAfter * 1000
|
||||||
|
: Math.min(baseDelayMs * 2 ** attempt, maxDelayMs);
|
||||||
await sleep(delayMs);
|
await sleep(delayMs);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1102,6 +1102,31 @@ test('no Retry-After header falls back to exponential backoff from baseDelayMs',
|
||||||
assert.deepEqual(calls, [1000, 2000]);
|
assert.deepEqual(calls, [1000, 2000]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('exponential backoff is capped, so a long retry budget does not wait minutes between attempts', async () => {
|
||||||
|
// Measured 2026-08-04 against the live forge: 20 parallel requests from one
|
||||||
|
// IP produced 429 with NO Retry-After header at all (nginx never sends one
|
||||||
|
// here) — the exponential fallback is the only path that ever runs in
|
||||||
|
// practice. Recovery was gradual, not a fixed-duration ban: a burst that
|
||||||
|
// size took up to ~15s to fully drain, and a 20s manual pause cleared it.
|
||||||
|
// Uncapped doubling would reach 32s on a single attempt; capping at 8s and
|
||||||
|
// extending the retry budget covers the measured recovery window without
|
||||||
|
// one attempt blocking for excessive time.
|
||||||
|
const calls = [];
|
||||||
|
const responses = [
|
||||||
|
fakeResponse(429),
|
||||||
|
fakeResponse(429),
|
||||||
|
fakeResponse(429),
|
||||||
|
fakeResponse(429),
|
||||||
|
fakeResponse(429),
|
||||||
|
fakeResponse(200),
|
||||||
|
];
|
||||||
|
const fetchImpl = async () => responses.shift();
|
||||||
|
const sleep = async (ms) => calls.push(ms);
|
||||||
|
await fetchWithRetry('https://x', {}, { fetchImpl, sleep, baseDelayMs: 1000, maxDelayMs: 8000, retries: 5 });
|
||||||
|
// Uncapped, the 5th delay would be 1000 * 2**4 = 16000.
|
||||||
|
assert.deepEqual(calls, [1000, 2000, 4000, 8000, 8000]);
|
||||||
|
});
|
||||||
|
|
||||||
test('retries are bounded — a persistent 429 returns the 429, not an infinite loop', async () => {
|
test('retries are bounded — a persistent 429 returns the 429, not an infinite loop', async () => {
|
||||||
let calls = 0;
|
let calls = 0;
|
||||||
const fetchImpl = async () => fakeResponse(429);
|
const fetchImpl = async () => fakeResponse(429);
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue