From e4d087682ddb31573e45dfb3df5e90f68f5f44db Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 12 Sep 2026 22:07:02 +0200 Subject: [PATCH] Retry transient upstream failures; fix `schulcloud --help` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/bin/cli.ts | 9 +++-- src/core/client.ts | 58 +++++++++++++++++++++++++++----- test/client.test.ts | 82 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 11 deletions(-) create mode 100644 test/client.test.ts diff --git a/src/bin/cli.ts b/src/bin/cli.ts index 9fb4321..b932f1a 100644 --- a/src/bin/cli.ts +++ b/src/bin/cli.ts @@ -31,10 +31,13 @@ SCHULCLOUD_SYNC_DIR. Config file: ${configPath()} `; async function main(argv: string[]): Promise { - 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; } diff --git a/src/core/client.ts b/src/core/client.ts index 538f821..21dd834 100644 --- a/src/core/client.ts +++ b/src/core/client.ts @@ -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 { + 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 { - const response = await fetch(url, { - headers: { Authorization: `Bearer ${this.config.jwt}`, Accept: accept }, - signal: AbortSignal.timeout(this.config.requestTimeoutMs), - redirect: 'follow', - }); - if (!response.ok) { + 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', + }); + } 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; + } + + if (response.ok) return response; + const body = await response.text().catch(() => ''); - throw new SchulcloudApiError(response.status, url.pathname + url.search, body); + 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; } - return response; + + throw lastError instanceof Error ? lastError : new Error('request failed'); } /** Authenticated GET returning JSON. `path` is absolute, e.g. `/api/v3/courses`. */ diff --git a/test/client.test.ts b/test/client.test.ts new file mode 100644 index 0000000..b96b6b2 --- /dev/null +++ b/test/client.test.ts @@ -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'); + }); +});