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().
This commit is contained in:
Ayobami Adegoke
2026-09-14 18:25:30 +02:00
committed by GitHub
parent c7bd494f11
commit c2cd71ddee
7 changed files with 238 additions and 21 deletions
@@ -1,7 +1,7 @@
import { defineCommand, option } from "@bunli/core" import { defineCommand, option } from "@bunli/core"
import { z } from "zod" import { z } from "zod"
import { parse } from "node-html-parser" 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" import { extractCity, toContractDate } from "./search.js"
interface JsonLdJobPosting { interface JsonLdJobPosting {
@@ -241,26 +241,15 @@ export const detail = defineCommand({
const url = `${BASE_URL}/job/${slug}` const url = `${BASE_URL}/job/${slug}`
try { try {
const response = await fetch(url, { // htmlFetch carries the portal contract's 429/5xx backoff, the request
headers: { // timeout, and the shared User-Agent; a bare fetch() here had none.
"Accept": "text/html,application/xhtml+xml", const html = await htmlFetch(url)
"User-Agent": "Mozilla/5.0 (compatible; jobdanmark-cli/1.0)",
},
signal: AbortSignal.timeout(15000),
})
if (response.status === 404) { if (html === null) {
writeError("Job not found", "NOT_FOUND") writeError("Job not found", "NOT_FOUND")
process.exit(1) 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 if (signal.aborted) return
const output = parseJobPostingFromHtml(html, slug, url) const output = parseJobPostingFromHtml(html, slug, url)
@@ -64,6 +64,45 @@ export async function apiPost<T>(path: string, body: unknown): Promise<T> {
throw new Error("API request failed after max retries") 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<string | null> {
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 { export function writeError(error: string, code: string): void {
process.stderr.write(JSON.stringify({ error, code }) + "\n") process.stderr.write(JSON.stringify({ error, code }) + "\n")
} }
@@ -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 = `<!doctype html><html><head>
<script type="application/ld+json">{"@context":"https://schema.org","@type":"JobPosting",
"title":"Data Engineer","datePosted":"2026-09-01","hiringOrganization":{"@type":"Organization","name":"Acme"},
"description":"Build pipelines."}</script></head><body></body></html>`;
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<void> }).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" });
});
});
@@ -1,5 +1,5 @@
import { afterEach, describe, expect, test } from "bun:test"; 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 // A stalled upstream connection (accepted socket, no response) would otherwise
// hang the CLI forever - fetch has no default timeout. Assert both request // hang the CLI forever - fetch has no default timeout. Assert both request
@@ -21,6 +21,17 @@ describe("request timeout", () => {
expect(init?.signal).toBeInstanceOf(AbortSignal); 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("<html></html>", { 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 () => { test("apiPost passes an AbortSignal timeout to fetch", async () => {
let init: RequestInit | undefined; let init: RequestInit | undefined;
globalThis.fetch = (async (_url: string | URL | Request, i?: RequestInit) => { globalThis.fetch = (async (_url: string | URL | Request, i?: RequestInit) => {
@@ -1,11 +1,13 @@
import { afterEach, describe, expect, test } from "bun:test"; 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 // The portal contract requires backoff on 429/5xx. These tests pin the retry
// loop offline: a stubbed fetch counts attempts, and a stubbed setTimeout // loop offline: a stubbed fetch counts attempts, and a stubbed setTimeout
// fires immediately so the exhaustion case does not sleep through the real // fires immediately so the exhaustion case does not sleep through the real
// 500ms -> 5s backoff schedule. apiFetch and apiPost carry separate copies of // 500ms -> 5s backoff schedule. apiFetch, apiPost, and htmlFetch carry separate
// the loop, so both are exercised to keep them from drifting apart. // 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 originalFetch = globalThis.fetch;
const originalSetTimeout = globalThis.setTimeout; const originalSetTimeout = globalThis.setTimeout;
@@ -33,6 +35,7 @@ function stubFetch(responses: Array<() => Response>): { calls: number } {
const wrappers: Array<[string, () => Promise<{ ok: boolean }>]> = [ const wrappers: Array<[string, () => Promise<{ ok: boolean }>]> = [
["apiFetch", () => apiFetch<{ ok: boolean }>("/x")], ["apiFetch", () => apiFetch<{ ok: boolean }>("/x")],
["apiPost", () => apiPost<{ 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) { 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);
});
});
@@ -1,5 +1,5 @@
import { afterEach, describe, expect, test } from "bun:test"; 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 // 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 // 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"); 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("<html></html>", { 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");
});
});
+18
View File
@@ -47,6 +47,24 @@ per-file diff commands.
### Fixed ### 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 - **`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, 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 when the path didn't match its one pattern, silently used the whole input URL as the job