Merge pull request #61 from StepenkoAnatoli/fix/respect-rate-limit

A rate limit is an instruction to wait, and the collector now reads it

Author: StepenkoAnatoli

docs/ARCHITECTURE.md

@@ -426,6 +426,7 @@ each line — is in the ADR named beside it and in
 | A finding a reviewer has **considered and overruled** is recorded, not repeated forever: `[single-witness: …]` in an unknown's Evidence cell and `[render-reviewed: …]` in an evidence row's Finding cell turn a true-but-permanent warning into a pass **that prints the reason**. A reason under 30 characters is named (`single-witness-unreasoned`) rather than obeyed, and a note left on an unknown that has since been corroborated is reported (`single-witness-stale`) so the acknowledgement cannot rot. The render note lives on the ROW because `verifyLedger` hashes the whole capture file — a capture is evidence and stays byte-immutable, a judgement about it is corpus prose | [ADR-0038](adr/0038-a-judgement-the-gate-can-read.md) |
 | The collector lock is exclusive **on a local filesystem, and not promised on a network one**: Node states "the exclusive flag might not work with network file systems", a condition ADR-0020 never carried. Measured 2026-09-22 - 24 racers, 1 winner, 23 EEXIST on win32 - so the guarantee holds where it was tested and nothing detects where it is standing. Evidence: `docs/decisions/2026-09-22-wx-lock/` | ADR-0020 |
 | The SEARCH side is **selectable at dispatch**: collect.yml takes a search_transport input (auto | serpapi | firecrawl-cli) and collect-remote.mjs a matching --search-transport, passed as RESEARCH_KIT_SEARCH_TRANSPORT. ADR-0027 built the ladder and the collector never exposed it, so a dispatched run always took whatever auto-detect picked and two providers could not be compared on one topic through the kit at all. auto becomes the EMPTY STRING, falsy in the ladder, so the default is the previous behaviour exactly | ADR-0027 |
+| A **rate limit is an instruction to wait, not a failure**: `rateLimitWaitMs` (lib/firecrawl.mjs) reads the delay the vendor states in its own error text, and `collectOne` sleeps and retries, bounded at two. Firecrawl free tier allows 10 requests a minute and the collector did not throttle, so a run started inside a busy minute shed six of eight fetches and recorded them as ordinary failures. The vendor number is used rather than an invented backoff, for the reason the kit researched for Retry-After: when a server says when to come back, that is the number. Non-rate-limit errors are NOT retried - a 404 does not become a 200 if you wait | ADR-0005 |
 | A run reports **what landed separately from what it cost**: `collected` is captures on disk, `spent` is budget consumed (collected + failed). They were one number under the label `collected`, so a run that fetched 2 pages and lost 6 to a Firecrawl rate limit reported `collected 8` while its artifact carried 2. The free tier is 10 requests per minute and the collector does not throttle, so a paired run started inside the same minute sheds most of its fetches | ADR-0005 |
 | An entrypoint **refuses a flag it does not know** rather than ignoring it: `refuseUnknownFlags` in `lib/core.mjs`, called immediately after `parseFlags` in all eleven bins. `parseFlags` knows no flag names by design, so an entrypoint that skips the check gets silence — the flag is parsed, never read, and the command proceeds to its default. On 2026-09-22 that default was **a real collection**: `research.mjs --totally-made-up-flag` spent 26 credits against this repository's corpus, and `audit.mjs` wrote twelve files. The same survey found a dead `--max-scrapes` in two workflows, where the page bound people believed it set was really `plan.maxScrapes` — real in `collect.yml`, and **absent** in `live-collection.yml`, whose `max_pages` was validated and then ignored | ADR-0020 |
 | A capture that **says it failed to render** is reported, though it arrived whole: `completeness` grades the transport and cannot see a page built by script that arrives complete and empty. The rule matches sentences a page PRINTS, never a guess at whether its text looks substantial — emptiness itself was measured and found mechanically undetectable, in both directions ([ADR-0037](adr/0037-an-empty-capture-is-not-mechanically-detectable.md)) | [ADR-0037](adr/0037-an-empty-capture-is-not-mechanically-detectable.md) |

research-kit/README.md

@@ -395,7 +395,7 @@ node research-kit/bin/selftest.mjs            # all of it
 node research-kit/bin/selftest.mjs gate hook  # just these files
 ```
 
-856 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means
+860 tests, offline, no key and no network. The runner **awaits** every test, so `ok` means
 the assertions settled (ADR-0021), and each test is raced against a watchdog
 (`RESEARCH_KIT_TEST_TIMEOUT`, default 60s).
 

research-kit/lib/collect.mjs

@@ -10,7 +10,9 @@
 import fs from 'node:fs';
 import {
   PATHS, HEADERS, resolve, today, sha256, titleFromUrl, hostOf, urlDigest, writeText, readText,
+  sleepSync,
 } from './core.mjs';
+import { rateLimitWaitMs } from './firecrawl.mjs';
 import {
   captureEntry, cacheDecision, rememberCapture, appendRow, upsertRow, nextId,
   readCaptures, parseTable,
@@ -95,6 +97,8 @@ export function collectOne(root, url, {
   // Which search provider ranked this URL, when a search is why it is being fetched.
   discoveredBy = '',
   dryRun = false,
+  maxRateLimitRetries = 2,
+  log = () => {},
 } = {}) {
   // A dry run decides and writes nothing, so it needs no exclusive section.
   if (dryRun) {
@@ -122,11 +126,23 @@ export function collectOne(root, url, {
       return { status: 'cached', url, entry: decision.entry, reason: `fresh capture from ${decision.entry.retrieved}`, spent: 0 };
     }
 
+    // A rate limit is not a failure, it is an instruction to wait. Bounded, because a
+    // vendor that keeps refusing is a different problem from a busy minute, and a
+    // collector that retried forever would hold this lock forever.
     let result;
-    try {
-      result = runScrape(url);
-    } catch (err) {
-      result = { ok: false, url, error: err.message };
+    let waits = 0;
+    for (let attempt = 0; ; attempt += 1) {
+      try {
+        result = runScrape(url);
+      } catch (err) {
+        result = { ok: false, url, error: err.message };
+      }
+      if (result?.ok || attempt >= maxRateLimitRetries) break;
+      const wait = rateLimitWaitMs(result?.error);
+      if (wait === null) break;
+      waits += 1;
+      log(`  waiting    ${Math.round(wait / 1000)}s - ${url} hit the vendor rate limit`);
+      sleepSync(wait);
     }
 
     if (!result?.ok) {
@@ -136,7 +152,7 @@ export function collectOne(root, url, {
         discoveredBy,
         cmd: result?.cmd ?? '', error: result?.error ?? 'unknown failure',
       });
-      return { status: 'failed', url, entry: null, reason: result?.error ?? 'unknown failure', spent: 1 };
+      return { status: 'failed', url, entry: null, reason: result?.error ?? 'unknown failure', spent: 1, waits };
     }
 
     const entry = writeRaw(root, result, { date });
@@ -167,7 +183,7 @@ export function collectOne(root, url, {
 
     upsertRow(root, PATHS.sources, HEADERS.sources, [entry.url, type, result.title || titleFromUrl(entry.url), date, usedFor]);
 
-    return { status: 'collected', url: entry.url, entry, row: { id, finding }, reason: decision.reason, spent: 1 };
+    return { status: 'collected', url: entry.url, entry, row: { id, finding }, reason: decision.reason, spent: 1, waits };
   });
 }
 

research-kit/lib/core.mjs

@@ -294,3 +294,14 @@ export function refuseUnknownFlags(flags, known, { help = '', exit = 2, note = n
   if (help) process.stdout.write(help);
   process.exit(exit);
 }
+
+/**
+ * Block for `ms`. The fetch adapters are synchronous by contract - they reach the vendor
+ * through `spawnSync` - so a rate-limit wait cannot be awaited without changing that
+ * contract everywhere. `Atomics.wait` on a private buffer is the sanctioned way to do
+ * this; a spin loop would burn a core for the length of the wait.
+ */
+export function sleepSync(ms) {
+  if (!Number.isFinite(ms) || ms <= 0) return;
+  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
+}

research-kit/lib/firecrawl.mjs

@@ -424,3 +424,27 @@ export function cliCompatibility(version = cliVersion()) {
 }
 
 export default { name, scrape, search, map, command, status, runScrape };
+
+/**
+ * How long the vendor asked us to wait, in milliseconds, or `null` if this is not a rate
+ * limit at all.
+ *
+ * WHY THE VENDOR'S NUMBER AND NOT OUR OWN. Firecrawl states the delay in the error text -
+ * "please retry after 13s" - and it knows when its own window resets. A fixed backoff we
+ * invented would either wait too long on every failure or too little on the one that
+ * mattered. This is the same principle the kit researched for `Retry-After` on 2026-09-22:
+ * when a server tells you when to come back, that is the number to use.
+ *
+ * Found because a run lost six of eight fetches to "Consumed (req/min): 11, Remaining
+ * (req/min): 0" and reported them as ordinary failures. The free tier is ten requests a
+ * minute; the collector did not throttle and did not retry.
+ */
+export function rateLimitWaitMs(errorText) {
+  const text = String(errorText ?? '');
+  if (!/rate limit exceeded/i.test(text)) return null;
+  const retry = text.match(/retry after (\d+)\s*s/i);
+  if (retry) return (Number(retry[1]) + 1) * 1000;
+  // A rate limit with no stated delay still deserves a wait, and one minute clears a
+  // per-minute window by construction.
+  return 60_000;
+}

research-kit/lib/research-run.mjs

@@ -242,6 +242,7 @@ ${compatibility.remedy}`);
       // Empty for a plan URL - nobody's ranking chose it, a person wrote it down.
       discoveredBy: target.rankedBy ?? '',
       dryRun,
+      log,
     });
     if (outcome.status === 'collected') spent += 1;
     if (outcome.status === 'cached') cached += 1;

research-kit/test/collect.test.mjs

@@ -6,6 +6,7 @@ import { PATHS, resolve, readText, writeJson, today } from '../lib/core.mjs';
 import { readCorpus, parseTable } from '../lib/corpus.mjs';
 import { HEADERS } from '../lib/core.mjs';
 import { collectOne, writeRaw, captureName, bodyHashOf } from '../lib/collect.mjs';
+import { rateLimitWaitMs } from '../lib/firecrawl.mjs';
 import { verifyLedger } from '../lib/provenance.mjs';
 import { runResearch, readPlan, rankCandidate, selectCandidates, DEPTH_SCRAPES, usageSummary } from '../lib/research-run.mjs';
 
@@ -261,3 +262,72 @@ test('usage is written after a run that spent, and read back as a summary', () =
   assert.equal(usage.scrapes, 1);
   assert.equal(usage.evidence, 1);
 });
+
+// ---------------------------------------------------------------- rate limits
+
+test('rateLimitWaitMs reads the vendor number, and refuses to invent one', () => {
+  // The real error text, verbatim from run 35715485734.
+  const real = 'Rate limit exceeded. Consumed (req/min): 11, Remaining (req/min): 0. '
+    + 'Upgrade your plan at https://firecrawl.dev/pricing for increased rate limits or '
+    + 'please retry after 13s, resets at Tue Sep 22 2026 10:21:38 GMT+0000';
+  assert.equal(rateLimitWaitMs(real), 14_000, 'the stated 13s plus a second of margin');
+
+  // A rate limit with no stated delay still waits; one minute clears a per-minute window.
+  assert.equal(rateLimitWaitMs('Rate limit exceeded'), 60_000);
+
+  // Everything that is NOT a rate limit must return null, so ordinary failures are not
+  // retried. A 404 does not become a 404 if you wait.
+  assert.equal(rateLimitWaitMs('404 not found'), null);
+  assert.equal(rateLimitWaitMs(''), null);
+  assert.equal(rateLimitWaitMs(undefined), null);
+});
+
+test('collectOne retries a rate limit and keeps the page it eventually gets', () => {
+  // Six of eight fetches were lost this way on 2026-09-22, reported as plain failures.
+  const dir = makeProject();
+  let calls = 0;
+  const outcome = collectOne(dir, 'https://example.invalid/limited', {
+    corpus: readCorpus(dir),
+    runScrape: () => {
+      calls += 1;
+      if (calls === 1) {
+        return { ok: false, url: 'https://example.invalid/limited', error: 'Rate limit exceeded. please retry after 0s, resets at now' };
+      }
+      return { ok: true, url: 'https://example.invalid/limited', title: 'Limited', markdown: `# Limited\n\n${'body text here. '.repeat(40)}`, statusCode: 200, transport: 'stub', completeness: 'full' };
+    },
+  });
+
+  assert.equal(calls, 2, 'the rate limit must be retried, not reported');
+  assert.equal(outcome.status, 'collected');
+  assert.equal(outcome.waits, 1, 'the wait is counted so a slow run explains itself');
+});
+
+test('collectOne does NOT retry an ordinary failure', () => {
+  // The other half. Retrying a 404 spends budget to be told the same thing again.
+  const dir = makeProject();
+  let calls = 0;
+  const outcome = collectOne(dir, 'https://example.invalid/gone', {
+    corpus: readCorpus(dir),
+    runScrape: () => { calls += 1; return { ok: false, url: 'https://example.invalid/gone', error: '404 not found' }; },
+  });
+
+  assert.equal(calls, 1, 'a non-rate-limit failure must be reported immediately');
+  assert.equal(outcome.status, 'failed');
+  assert.equal(outcome.waits, 0);
+});
+
+test('collectOne gives up after a bounded number of rate-limit retries', () => {
+  // A vendor that keeps refusing is a different problem from a busy minute, and the
+  // retry runs inside the collector lock - forever here would be forever for everyone.
+  const dir = makeProject();
+  let calls = 0;
+  const outcome = collectOne(dir, 'https://example.invalid/always', {
+    corpus: readCorpus(dir),
+    maxRateLimitRetries: 2,
+    runScrape: () => { calls += 1; return { ok: false, url: 'https://example.invalid/always', error: 'Rate limit exceeded. please retry after 0s' }; },
+  });
+
+  assert.equal(calls, 3, 'the first attempt plus two retries');
+  assert.equal(outcome.status, 'failed');
+  assert.match(outcome.reason, /Rate limit exceeded/);
+});