From c2cd71ddee2b03e795032272768b95e99b634271 Mon Sep 17 00:00:00 2001 From: Ayobami Adegoke Date: Mon, 14 Sep 2026 17:25:30 +0100 Subject: [PATCH] fix(jobdanmark-search): back off on 429/5xx in detail instead of failing on the first attempt (#460) `detail` called fetch() directly rather than going through the CLI's own request wrappers, so it had none of the three things apiFetch/apiPost guarantee: no 429/5xx retry loop, a hand-inlined User-Agent that would drift from the exported USER_AGENT, and a timeout no wrapper test covered. A rate-limited detail page wrote API_ERROR and exited after ONE attempt; jobnet, jobbank, jobindex, linkedin, and freehire all retry up to six times on the same response. /scrape calls detail once per shortlisted posting, so a burst that tripped jobdanmark's limiter dropped those postings (no description, no deadline) while any other portal rode it out. Demonstrated by driving the real command handler with a stubbed 429 and instant timers: 1 fetch attempt and exit 1 before, 7 after (initial try plus six retries, the contract's schedule). Add htmlFetch to helpers.ts with the same backoff, timeout, and shared User-Agent as the JSON wrappers - 404 returns null so detail keeps its NOT_FOUND contract - and route detail through it. The retry-backoff, user-agent, and request-timeout suites now cover all three wrappers, and the new detail-backoff.test.ts exercises the handler path itself; its two retry cases fail against the bare fetch(). --- .../cli/src/commands/detail.ts | 21 +-- .../jobdanmark-search/cli/src/helpers.ts | 39 +++++ .../cli/tests/detail-backoff.test.ts | 134 ++++++++++++++++++ .../cli/tests/request-timeout.test.ts | 13 +- .../cli/tests/retry-backoff.test.ts | 18 ++- .../cli/tests/user-agent.test.ts | 16 ++- CHANGELOG.md | 18 +++ 7 files changed, 238 insertions(+), 21 deletions(-) create mode 100644 .agents/skills/jobdanmark-search/cli/tests/detail-backoff.test.ts diff --git a/.agents/skills/jobdanmark-search/cli/src/commands/detail.ts b/.agents/skills/jobdanmark-search/cli/src/commands/detail.ts index b3e7648..9bc49e1 100644 --- a/.agents/skills/jobdanmark-search/cli/src/commands/detail.ts +++ b/.agents/skills/jobdanmark-search/cli/src/commands/detail.ts @@ -1,7 +1,7 @@ import { defineCommand, option } from "@bunli/core" import { z } from "zod" import { parse } from "node-html-parser" -import { BASE_URL, normalizeSlug, writeError } from "../helpers.js" +import { BASE_URL, htmlFetch, normalizeSlug, writeError } from "../helpers.js" import { extractCity, toContractDate } from "./search.js" interface JsonLdJobPosting { @@ -241,26 +241,15 @@ export const detail = defineCommand({ const url = `${BASE_URL}/job/${slug}` try { - const response = await fetch(url, { - headers: { - "Accept": "text/html,application/xhtml+xml", - "User-Agent": "Mozilla/5.0 (compatible; jobdanmark-cli/1.0)", - }, - signal: AbortSignal.timeout(15000), - }) + // htmlFetch carries the portal contract's 429/5xx backoff, the request + // timeout, and the shared User-Agent; a bare fetch() here had none. + const html = await htmlFetch(url) - if (response.status === 404) { + if (html === null) { writeError("Job not found", "NOT_FOUND") process.exit(1) } - if (!response.ok) { - writeError(`API request failed: ${response.status} ${response.statusText}`, "API_ERROR") - process.exit(1) - } - - const html = await response.text() - if (signal.aborted) return const output = parseJobPostingFromHtml(html, slug, url) diff --git a/.agents/skills/jobdanmark-search/cli/src/helpers.ts b/.agents/skills/jobdanmark-search/cli/src/helpers.ts index 4a65e08..45c4480 100644 --- a/.agents/skills/jobdanmark-search/cli/src/helpers.ts +++ b/.agents/skills/jobdanmark-search/cli/src/helpers.ts @@ -64,6 +64,45 @@ export async function apiPost(path: string, body: unknown): Promise { throw new Error("API request failed after max retries") } +/** + * Fetch a rendered jobdanmark.dk page as text, with the same 429/5xx backoff, + * request timeout, and User-Agent as apiFetch/apiPost. `detail` reads HTML + * rather than the JSON API; it used to call fetch() directly with none of the + * three, so a rate-limited detail page failed on the first 429 while every + * other portal's detail command retried. Returns null on 404 so the caller + * keeps its own NOT_FOUND contract. + */ +export async function htmlFetch(url: string): Promise { + const maxRetries = 6 + let delay = 500 + for (let attempt = 0; attempt <= maxRetries; attempt++) { + const response = await fetch(url, { + headers: { + "Accept": "text/html,application/xhtml+xml", + "User-Agent": USER_AGENT, + }, + signal: AbortSignal.timeout(15000), + }) + if (response.status === 429 || response.status >= 500) { + if (attempt === maxRetries) { + throw new Error(`API request failed: ${response.status} ${response.statusText}`) + } + const jitter = Math.floor(Math.random() * 500) + await new Promise((resolve) => setTimeout(resolve, delay + jitter)) + delay = Math.min(delay * 2, 5000) + continue + } + if (response.status === 404) { + return null + } + if (!response.ok) { + throw new Error(`API request failed: ${response.status} ${response.statusText}`) + } + return response.text() + } + throw new Error("API request failed after max retries") +} + export function writeError(error: string, code: string): void { process.stderr.write(JSON.stringify({ error, code }) + "\n") } diff --git a/.agents/skills/jobdanmark-search/cli/tests/detail-backoff.test.ts b/.agents/skills/jobdanmark-search/cli/tests/detail-backoff.test.ts new file mode 100644 index 0000000..c34d076 --- /dev/null +++ b/.agents/skills/jobdanmark-search/cli/tests/detail-backoff.test.ts @@ -0,0 +1,134 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { detail } from "../src/commands/detail"; + +// The portal contract requires backoff on 429/5xx, and `/scrape` calls +// `detail` once per shortlisted posting - a burst that trips the rate limiter +// is exactly when it matters. The handler used to call fetch() directly with +// no retry loop: on a 429 it wrote API_ERROR and exited after ONE attempt, +// while every other portal's detail command retried. These tests drive the +// real command handler (not the wrapper in isolation) with a stubbed fetch, +// instant timers, and process.exit turned into a throw so the exit path can +// be asserted. On the pre-fix handler the first test sees 1 call and an exit. + +const originalFetch = globalThis.fetch; +const originalSetTimeout = globalThis.setTimeout; +const originalExit = process.exit; +const originalLog = console.log; +const originalStderrWrite = process.stderr.write; + +afterEach(() => { + globalThis.fetch = originalFetch; + globalThis.setTimeout = originalSetTimeout; + process.exit = originalExit; + console.log = originalLog; + process.stderr.write = originalStderrWrite; +}); + +const JSON_LD_PAGE = ` +`; + +function instantTimers() { + globalThis.setTimeout = ((fn: () => void) => + originalSetTimeout(fn, 0)) as unknown as typeof setTimeout; +} + +function stubFetch(responses: Array<() => Response>): { calls: number } { + const state = { calls: 0 }; + globalThis.fetch = (async () => { + const i = Math.min(state.calls, responses.length - 1); + state.calls++; + return responses[i](); + }) as unknown as typeof fetch; + return state; +} + +function captureOutput(): { stdout: string[]; stderr: string[] } { + const out = { stdout: [] as string[], stderr: [] as string[] }; + console.log = ((...args: unknown[]) => out.stdout.push(args.join(" "))) as typeof console.log; + process.stderr.write = ((chunk: string | Uint8Array) => { + out.stderr.push(String(chunk)); + return true; + }) as typeof process.stderr.write; + return out; +} + +function firstStderrJson(out: { stderr: string[] }): unknown { + const firstLine = out.stderr.join("").trim().split("\n")[0]; + return JSON.parse(firstLine); +} + +class ExitCalled extends Error { + constructor(public code: number | undefined) { + super(`process.exit(${code})`); + } +} + +function throwingExit() { + process.exit = ((code?: number) => { + throw new ExitCalled(code); + }) as unknown as typeof process.exit; +} + +async function runDetail(slug: string): Promise<{ exit: number | null }> { + const handler = (detail as unknown as { handler: (ctx: unknown) => Promise }).handler; + try { + await handler({ flags: { format: "json" }, positional: [slug], signal: new AbortController().signal }); + return { exit: null }; + } catch (err) { + if (err instanceof ExitCalled) return { exit: err.code ?? 0 }; + throw err; + } +} + +describe("detail backoff on the real handler path", () => { + test("retries a 429 and returns the posting on the next attempt", async () => { + instantTimers(); + throwingExit(); + const out = captureOutput(); + const state = stubFetch([ + () => new Response("", { status: 429, statusText: "Too Many Requests" }), + () => new Response(JSON_LD_PAGE, { status: 200 }), + ]); + + const result = await runDetail("data-engineer-acme"); + + expect(result.exit).toBeNull(); + expect(state.calls).toBe(2); + const parsed = JSON.parse(out.stdout.join("\n")) as { title: string; slug: string }; + expect(parsed.title).toBe("Data Engineer"); + expect(parsed.slug).toBe("data-engineer-acme"); + expect(out.stderr.join("")).toBe(""); + }); + + test("gives up after the initial attempt plus six retries and exits 1 with API_ERROR", async () => { + instantTimers(); + throwingExit(); + const out = captureOutput(); + const state = stubFetch([() => new Response("", { status: 503, statusText: "Service Unavailable" })]); + + const result = await runDetail("data-engineer-acme"); + + expect(result.exit).toBe(1); + expect(state.calls).toBe(7); + const err = firstStderrJson(out) as { code: string; error: string }; + expect(err.code).toBe("API_ERROR"); + expect(err.error).toMatch(/503/); + }); + + test("a 404 is not retried and still reports NOT_FOUND", async () => { + throwingExit(); + const out = captureOutput(); + const state = stubFetch([() => new Response("", { status: 404 })]); + + const result = await runDetail("gone"); + + expect(result.exit).toBe(1); + expect(state.calls).toBe(1); + // The handler's own catch block sees the throwing process.exit stub and + // writes a second line - a test artifact, not CLI behaviour. The first + // stderr line is the contract. + expect(firstStderrJson(out)).toEqual({ error: "Job not found", code: "NOT_FOUND" }); + }); +}); diff --git a/.agents/skills/jobdanmark-search/cli/tests/request-timeout.test.ts b/.agents/skills/jobdanmark-search/cli/tests/request-timeout.test.ts index 6de26a2..f5bdf66 100644 --- a/.agents/skills/jobdanmark-search/cli/tests/request-timeout.test.ts +++ b/.agents/skills/jobdanmark-search/cli/tests/request-timeout.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { apiFetch, apiPost } from "../src/helpers"; +import { apiFetch, apiPost, htmlFetch } from "../src/helpers"; // A stalled upstream connection (accepted socket, no response) would otherwise // hang the CLI forever - fetch has no default timeout. Assert both request @@ -21,6 +21,17 @@ describe("request timeout", () => { expect(init?.signal).toBeInstanceOf(AbortSignal); }); + test("htmlFetch passes an AbortSignal timeout to fetch", async () => { + let init: RequestInit | undefined; + globalThis.fetch = (async (_url: string | URL | Request, i?: RequestInit) => { + init = i; + return new Response("", { status: 200 }); + }) as unknown as typeof fetch; + + await htmlFetch("https://jobdanmark.dk/job/x"); + expect(init?.signal).toBeInstanceOf(AbortSignal); + }); + test("apiPost passes an AbortSignal timeout to fetch", async () => { let init: RequestInit | undefined; globalThis.fetch = (async (_url: string | URL | Request, i?: RequestInit) => { diff --git a/.agents/skills/jobdanmark-search/cli/tests/retry-backoff.test.ts b/.agents/skills/jobdanmark-search/cli/tests/retry-backoff.test.ts index eac8fe4..1c8827c 100644 --- a/.agents/skills/jobdanmark-search/cli/tests/retry-backoff.test.ts +++ b/.agents/skills/jobdanmark-search/cli/tests/retry-backoff.test.ts @@ -1,11 +1,13 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { apiFetch, apiPost } from "../src/helpers"; +import { apiFetch, apiPost, htmlFetch } from "../src/helpers"; // The portal contract requires backoff on 429/5xx. These tests pin the retry // loop offline: a stubbed fetch counts attempts, and a stubbed setTimeout // fires immediately so the exhaustion case does not sleep through the real -// 500ms -> 5s backoff schedule. apiFetch and apiPost carry separate copies of -// the loop, so both are exercised to keep them from drifting apart. +// 500ms -> 5s backoff schedule. apiFetch, apiPost, and htmlFetch carry separate +// copies of the loop, so all three are exercised to keep them from drifting +// apart. htmlFetch is the one `detail` uses: before it existed, detail called +// fetch() directly and a 429 failed on the first attempt (1 call, not 7). const originalFetch = globalThis.fetch; const originalSetTimeout = globalThis.setTimeout; @@ -33,6 +35,7 @@ function stubFetch(responses: Array<() => Response>): { calls: number } { const wrappers: Array<[string, () => Promise<{ ok: boolean }>]> = [ ["apiFetch", () => apiFetch<{ ok: boolean }>("/x")], ["apiPost", () => apiPost<{ ok: boolean }>("/x", {})], + ["htmlFetch", () => htmlFetch("https://jobdanmark.dk/job/x").then((html) => ({ ok: html !== null }))], ]; for (const [name, call] of wrappers) { @@ -65,3 +68,12 @@ for (const [name, call] of wrappers) { }); }); } + +describe("htmlFetch 404", () => { + test("returns null without retrying so detail keeps its NOT_FOUND contract", async () => { + const state = stubFetch([() => new Response("", { status: 404 })]); + + expect(await htmlFetch("https://jobdanmark.dk/job/missing")).toBeNull(); + expect(state.calls).toBe(1); + }); +}); diff --git a/.agents/skills/jobdanmark-search/cli/tests/user-agent.test.ts b/.agents/skills/jobdanmark-search/cli/tests/user-agent.test.ts index 67a105c..d16f5ab 100644 --- a/.agents/skills/jobdanmark-search/cli/tests/user-agent.test.ts +++ b/.agents/skills/jobdanmark-search/cli/tests/user-agent.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test } from "bun:test"; -import { apiFetch, apiPost, USER_AGENT } from "../src/helpers"; +import { apiFetch, apiPost, htmlFetch, USER_AGENT } from "../src/helpers"; // Bun's fetch injects an anonymous default User-Agent (Bun/1.3.10) when code // sets none. This CLI should say who is asking, in the honest style jobindex @@ -46,3 +46,17 @@ describe("apiPost user agent", () => { expect(headerValue(init?.headers, "Content-Type")).toBe("application/json"); }); }); + +describe("htmlFetch user agent", () => { + test("sends the shared User-Agent and asks for HTML", async () => { + let init: RequestInit | undefined; + globalThis.fetch = (async (_url: string | URL | Request, i?: RequestInit) => { + init = i; + return new Response("", { status: 200 }); + }) as unknown as typeof fetch; + + await htmlFetch("https://jobdanmark.dk/job/x"); + expect(headerValue(init?.headers, "User-Agent")).toBe(USER_AGENT); + expect(headerValue(init?.headers, "Accept")).toContain("text/html"); + }); +}); diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ca78ab..8a967dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,24 @@ per-file diff commands. ### Fixed +- **`jobdanmark-search detail` now backs off on 429/5xx like every other portal's detail + command** - the handler called `fetch()` directly instead of going through the CLI's own + request wrappers, so it carried none of the three things `apiFetch`/`apiPost` guarantee: + no 429/5xx retry loop (a rate-limited detail page wrote `API_ERROR` and exited after one + attempt, where jobnet, jobbank, jobindex, linkedin, and freehire all retry up to six + times), a hand-inlined User-Agent string that would drift from the exported `USER_AGENT`, + and a timeout the wrappers' tests never saw. `/scrape` calls `detail` once per + shortlisted posting, so a burst that tripped jobdanmark's rate limiter dropped those + postings outright - no description, no deadline - while the same burst on any other + portal rode it out. Demonstrated by driving the real command handler with a stubbed 429: + 1 fetch attempt and exit 1 before, 7 attempts after (the contract's initial try plus six + retries). Fixed by adding `htmlFetch` to `helpers.ts` with the same backoff schedule, + timeout, and shared User-Agent as the JSON wrappers (404 returns `null` so `detail` keeps + its `NOT_FOUND` contract) and routing `detail` through it. Pinned in the existing + `retry-backoff`, `user-agent`, and `request-timeout` suites, which now cover all three + wrappers, plus a new `detail-backoff.test.ts` that exercises the handler path itself - + its retry cases fail against the bare `fetch()`. + - **`jobindex-search detail` no longer fetches arbitrary URLs or invents posting-shaped output** (#447) - the command fetched any `http(s)` input verbatim (no host check) and, when the path didn't match its one pattern, silently used the whole input URL as the job