Retry transient upstream failures; fix schulcloud --help
A full crawl made the instance answer 4 of 26 course pages with an nginx 503 "temporarily unavailable" front-page — it pushes back when several hundred requests arrive quickly. Nothing retried, so the index was quietly incomplete: 22 courses and 153 files rather than 26 and 205, with the failures recorded per course rather than surfaced as a problem. The client now retries 429/500/502/503/504 and transient network errors with exponential backoff plus jitter (so parallel crawl workers do not retry in lockstep), honouring Retry-After when sent. Every call here is an idempotent GET, so retrying is safe. 401 and 404 are deliberately not retried: an expired token will not recover, and neither will a bad id. Re-crawled after the fix: 26 courses, 205 files, zero failures. Also: `schulcloud --help` printed 'Unknown command "--help"' because a leading flag was parsed as the command name. 63 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -31,10 +31,13 @@ SCHULCLOUD_SYNC_DIR. Config file: ${configPath()}
|
||||
`;
|
||||
|
||||
async function main(argv: string[]): Promise<number> {
|
||||
const [command, ...rest] = argv;
|
||||
const flags = parseFlags(rest);
|
||||
// A leading flag means no command was given: `schulcloud --help` must not be
|
||||
// read as a command called "--help".
|
||||
const hasCommand = argv[0] !== undefined && !argv[0].startsWith('-');
|
||||
const command = hasCommand ? argv[0] : undefined;
|
||||
const flags = parseFlags(hasCommand ? argv.slice(1) : argv);
|
||||
|
||||
if (!command || command === 'help' || flags.help) {
|
||||
if (!command || command === 'help' || flags.help || flags.h) {
|
||||
process.stdout.write(USAGE);
|
||||
return 0;
|
||||
}
|
||||
|
||||
@@ -15,6 +15,20 @@ import type {
|
||||
TaskContent,
|
||||
} from './types.ts';
|
||||
|
||||
/** Statuses worth retrying: transient by definition, and every call here is a GET. */
|
||||
const RETRYABLE = new Set([429, 500, 502, 503, 504]);
|
||||
const MAX_RETRIES = 3;
|
||||
|
||||
/** Exponential backoff with jitter, so parallel workers do not retry in lockstep. */
|
||||
function backoffMs(attempt: number): number {
|
||||
const base = 400 * 2 ** (attempt - 1);
|
||||
return Math.round(base + Math.random() * base * 0.5);
|
||||
}
|
||||
|
||||
function delay(ms: number): Promise<void> {
|
||||
return new Promise((resolve) => setTimeout(resolve, ms));
|
||||
}
|
||||
|
||||
/** An API response outside the 2xx range, carrying the status for callers to branch on. */
|
||||
export class SchulcloudApiError extends Error {
|
||||
readonly status: number;
|
||||
@@ -83,16 +97,44 @@ export class SchulcloudClient {
|
||||
}
|
||||
|
||||
private async request(url: URL, accept: string): Promise<Response> {
|
||||
const response = await fetch(url, {
|
||||
let lastError: unknown;
|
||||
|
||||
for (let attempt = 0; attempt <= MAX_RETRIES; attempt++) {
|
||||
if (attempt > 0) await delay(backoffMs(attempt));
|
||||
|
||||
let response: Response;
|
||||
try {
|
||||
response = await fetch(url, {
|
||||
headers: { Authorization: `Bearer ${this.config.jwt}`, Accept: accept },
|
||||
signal: AbortSignal.timeout(this.config.requestTimeoutMs),
|
||||
redirect: 'follow',
|
||||
});
|
||||
if (!response.ok) {
|
||||
const body = await response.text().catch(() => '');
|
||||
throw new SchulcloudApiError(response.status, url.pathname + url.search, body);
|
||||
} catch (error) {
|
||||
// Connection reset or timeout: worth one more try, since every call
|
||||
// here is an idempotent GET.
|
||||
lastError = error;
|
||||
if (attempt === MAX_RETRIES) throw error;
|
||||
continue;
|
||||
}
|
||||
return response;
|
||||
|
||||
if (response.ok) return response;
|
||||
|
||||
const body = await response.text().catch(() => '');
|
||||
const error = new SchulcloudApiError(response.status, url.pathname + url.search, body);
|
||||
|
||||
// A crawl issues hundreds of requests and the instance answers some of
|
||||
// them with a 503 front-page when it decides we are going too fast.
|
||||
// Observed live: 4 of 26 course pages failed that way in one crawl, and
|
||||
// all of them succeeded on a retry. 429 and the other gateway errors
|
||||
// are the same kind of "come back shortly".
|
||||
if (!RETRYABLE.has(response.status) || attempt === MAX_RETRIES) throw error;
|
||||
|
||||
const retryAfter = Number(response.headers.get('retry-after'));
|
||||
if (Number.isFinite(retryAfter) && retryAfter > 0) await delay(retryAfter * 1000);
|
||||
lastError = error;
|
||||
}
|
||||
|
||||
throw lastError instanceof Error ? lastError : new Error('request failed');
|
||||
}
|
||||
|
||||
/** Authenticated GET returning JSON. `path` is absolute, e.g. `/api/v3/courses`. */
|
||||
|
||||
82
test/client.test.ts
Normal file
82
test/client.test.ts
Normal file
@@ -0,0 +1,82 @@
|
||||
import assert from 'node:assert/strict';
|
||||
import { afterEach, describe, it } from 'node:test';
|
||||
import { SchulcloudClient, SchulcloudApiError } from '../src/core/client.ts';
|
||||
|
||||
/**
|
||||
* Retry behaviour, driven by a stubbed fetch. Observed live: a full crawl made
|
||||
* the instance answer 4 of 26 course pages with a 503 front-page, all of which
|
||||
* succeeded on retry — so this is the difference between a complete index and a
|
||||
* quietly incomplete one.
|
||||
*/
|
||||
const config = {
|
||||
baseUrl: 'https://example.test',
|
||||
jwt: 'x',
|
||||
authToken: undefined,
|
||||
port: 1,
|
||||
bindHost: '127.0.0.1',
|
||||
maxDownloadBytes: 1000,
|
||||
maxExtractedChars: 1000,
|
||||
requestTimeoutMs: 1000,
|
||||
keepaliveIntervalMs: 0,
|
||||
databaseUrl: undefined,
|
||||
mirrorDir: '/tmp',
|
||||
mirrorMaxBytes: 1000,
|
||||
crawlIntervalMs: 0,
|
||||
};
|
||||
|
||||
const realFetch = globalThis.fetch;
|
||||
afterEach(() => {
|
||||
globalThis.fetch = realFetch;
|
||||
});
|
||||
|
||||
function stubFetch(responses: (Response | Error)[]): () => number {
|
||||
let calls = 0;
|
||||
globalThis.fetch = (async () => {
|
||||
const next = responses[calls++] ?? responses.at(-1)!;
|
||||
if (next instanceof Error) throw next;
|
||||
return next;
|
||||
}) as typeof fetch;
|
||||
return () => calls;
|
||||
}
|
||||
|
||||
const json = (body: unknown, status = 200) =>
|
||||
new Response(JSON.stringify(body), { status, headers: { 'content-type': 'application/json' } });
|
||||
|
||||
describe('SchulcloudClient retries', () => {
|
||||
it('retries a 503 and succeeds', async () => {
|
||||
const calls = stubFetch([json({}, 503), json({ school: { id: 's' } })]);
|
||||
const client = new SchulcloudClient(config);
|
||||
await client.me();
|
||||
assert.equal(calls(), 2);
|
||||
});
|
||||
|
||||
it('retries transient network errors', async () => {
|
||||
const calls = stubFetch([new Error('ECONNRESET'), json({ school: { id: 's' } })]);
|
||||
await new SchulcloudClient(config).me();
|
||||
assert.equal(calls(), 2);
|
||||
});
|
||||
|
||||
it('does not retry a 401 — a dead token will not recover', async () => {
|
||||
const calls = stubFetch([json({}, 401)]);
|
||||
await assert.rejects(() => new SchulcloudClient(config).me(), (error: SchulcloudApiError) => {
|
||||
assert.equal(error.status, 401);
|
||||
return true;
|
||||
});
|
||||
assert.equal(calls(), 1, 'retrying an expired token just wastes time');
|
||||
});
|
||||
|
||||
it('does not retry a 404', async () => {
|
||||
const calls = stubFetch([json({}, 404)]);
|
||||
await assert.rejects(() => new SchulcloudClient(config).me());
|
||||
assert.equal(calls(), 1);
|
||||
});
|
||||
|
||||
it('gives up after the retry budget and reports the real status', async () => {
|
||||
const calls = stubFetch([json({}, 503)]);
|
||||
await assert.rejects(() => new SchulcloudClient(config).me(), (error: SchulcloudApiError) => {
|
||||
assert.equal(error.status, 503);
|
||||
return true;
|
||||
});
|
||||
assert.equal(calls(), 4, 'one attempt plus three retries');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user