Skip to content

A link we could not fetch is not a broken link - #73

Merged
arberx merged 2 commits into
mainfrom
fix/dead-links-need-a-status-code
Aug 16, 2026
Merged

arberx merged 2 commits into
mainfrom
fix/dead-links-need-a-status-code

Conversation

@arberx

@arberx arberx commented Aug 16, 2026

Copy link
Copy Markdown
Member

What

deriveDeadLinks accepted two different things as a dead link:

target.statusCode >= 400                          -> dead link  the site said it is broken
target.state === 'fetch-error' && statusCode null -> dead link  WE failed to ask

Only the first is evidence. A finding now requires a real status code (statusCode: number, reason: 'http-error'). A target that never answered goes to a new deadLinks.unverified array with the transport error that stopped it, under its own key prefix so a consumer storing findings by key cannot merge the buckets back together.

Page fetches also get maxFetchRetries extra attempts (default 2, 500ms doubling) before anything classifies the observation.

Why

The root cause is one level deeper than the classifier: the traversal files both an HTTP 4xx/5xx response and a failed transport under state: 'fetch-error', so only the status code separates them — and the classifier keyed off the state. There was also no retry anywhere in the crawl, so one refused connection permanently decided a URL's fate, at concurrency 5 with zero delay by default.

On one 228-page site this produced 15 "dead links" across 6 URLs, every one statusCode: null with no error string. All 6 served a 200 in under a second on a manual check moments later. That output was client-facing: an agency hands a client a site-health report naming six live pages as broken, the client clicks them, and the report loses credibility.

Re-crawling the same site with this build: 226 pages, 0 dead links, 0 unverified, 0 fetch errors.

Retry scope

Only TIMEOUT and UNREACHABLE are retried. A blocked host, a redirect limit, an oversized body and an exhausted budget all reproduce on a retry, so retrying them would only spend budget. Retries consume the fetch budget and are paced like any other request; the backoff is abortable and clipped to the remaining duration budget. maxFetchRetries: 0 restores single-attempt behavior.

summary.fetchRetries: { attempted, recovered } is new and optional — a nonzero recovered is direct evidence the crawler rather than the site was the flaky party, which is the question you have to answer before believing any crawl-derived link finding.

Tests

test/crawl-dead-link-classification.test.ts, 9 cases. Written and then verified falsifiable: against the pre-fix logic, 5 of the 9 fail. They cover the invariant (no null-status observation can reach findings, across three transport failure modes), both directions of the split (a real 404 and a real 503 are still findings), retry recovery, the retry bound, and that a permanent failure is not retried.

Full suite: 608 passing, typecheck and lint clean.

Breaking

Crawl schema 1.2 -> 1.3, crawl engine 1.2.0 -> 1.3.0.

CrawlDeadLinkResult gains a required unverified array and CrawlDeadLinkFinding narrows to statusCode: number / reason: 'http-error'. Reading a finding still compiles (both are narrowings); constructing a result does not. unverified is deliberately required rather than optional — a consumer skipping it is the exact failure this fixes.

Dead-link counts drop against 5.x on any site where the crawler ever failed to connect, so a stored crawl cannot be compared across this boundary.

Note the bug is present in published 5.0.0, so a consumer on 5.x is affected today.

🤖 Generated with Claude Code

`deriveDeadLinks` accepted two different things as a dead link: a target
that answered 4xx/5xx, and a target that never answered at all. Only the
first is evidence the link is broken. The second is a fact about the
crawl — a timeout, a reset socket, throttling under our own concurrency
— and the URL is very often healthy.

The root cause sits one level deeper: the traversal files BOTH an HTTP
4xx/5xx response and a failed transport under `state: 'fetch-error'`, so
only the status code separates them, and the classifier keyed off the
state. There was also no retry anywhere in the crawl, so a single
refused connection permanently decided a URL's fate — at concurrency 5
with zero delay by default.

On one 228-page site this produced 15 "dead links" across 6 URLs, every
one with `statusCode: null` and no error string. All 6 served a 200 in
under a second on a manual check moments later. That output was
client-facing.

A finding now requires a real status code (`statusCode: number`,
`reason: 'http-error'`). A target that never answered goes to the new
`deadLinks.unverified` array with the transport error that stopped it,
under its own key prefix so a consumer storing findings by key cannot
merge the buckets back together.

Page fetches also get `maxFetchRetries` extra attempts (default 2, 500ms
doubling) BEFORE anything classifies the observation, so the classifier
only ever sees a failure the crawler could not shake off. Only TIMEOUT
and UNREACHABLE are retried; a blocked host, a redirect limit, an
oversized body and an exhausted budget all reproduce on a retry.
Retries consume the fetch budget and are paced like any other request,
and the backoff is abortable and clipped to the remaining duration
budget. `summary.fetchRetries` reports {attempted, recovered}.

Re-crawling the affected site with this build: 226 pages, 0 dead links,
0 unverified, 0 fetch errors.

BREAKING: crawl schema 1.2 -> 1.3, crawl engine 1.2.0 -> 1.3.0.
`CrawlDeadLinkResult` gains a required `unverified` array and
`CrawlDeadLinkFinding` narrows to `statusCode: number` /
`reason: 'http-error'`. Reading a finding still compiles; constructing a
result does not. Dead-link counts drop against 5.x anywhere the crawler
ever failed to connect, so a stored crawl cannot be compared across it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… broken

Review follow-ups on #73.

1. A request could outlive the crawl that owned it. The backoff was
   clipped to the remaining deadline, but the retry it released then ran
   on the fixed 10s socket timeout, which no budget check can interrupt
   — they run before a request starts and after it ends, never during.
   Measured: a 3s-budget crawl took 10.7s. Every request is now bounded
   by min(10s, remaining maxDurationMs), which also fixes the same
   overrun on the FIRST attempt, where it predates retries.

2. HTTP 429 was reported as a broken link. It is the one error status
   that describes our own request rate rather than the resource, so
   calling it broken blames a site for how hard we crawled it — the
   exact mistake this branch exists to fix, arriving with a status code
   attached. It is now retried first, honouring Retry-After (bounded to
   10s, clipped by the deadline); a target still throttled after every
   retry is reported unverified with reason 'throttled'.

3. `unverified` claimed targets "never answered" when some had. A
   redirect loop and an oversized body both end as a null-status
   fetch-error, but both did answer. `reason` is now a typed
   CrawlUnverifiedReason (unreachable / throttled / redirect-limit /
   body-too-large / unknown) and each row carries the statusCode when
   there was one. CrawlPageObservation gains `errorCode`, the typed code
   behind the human `error` string, which is what makes the distinction
   derivable rather than a string match.

4. Retry accounting counted requests never sent and lost the failure
   that prompted them. `attempted` incremented before the backoff, so a
   deadline expiring mid-wait recorded an attempt that never dispatched;
   worse, the budget error then propagated in place of the original
   transient one and `processItem` dropped the page entirely — no page
   row, no unverified entry, nothing to explain the gap. The counter now
   increments only once a retry can be dispatched, and a deadline that
   expires during backoff re-raises the ORIGINAL failure so the
   observation survives.

5. Crawl schema 1.3 -> 2.0, crawl engine 1.3.0 -> 2.0.0. The shape
   change is not additive, and 1.1 -> 1.2 was explicitly documented as
   additive; versioning this as a minor would tell a consumer the
   opposite of the truth.

6. README version badge was still 5.0.0.

NOT fixed here, deliberately: a successful alias->target response is
still discarded when the target is already queued, so a later failed
re-fetch loses a body we already had. That early return predates this
work (#65) and fixing it means caching bodies across the frontier —
a traversal change that does not belong in a classification fix. This
branch only improves its consequence: such a link is now reported
unverified rather than as a fabricated dead link.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@arberx

arberx commented Aug 16, 2026

Copy link
Copy Markdown
Member Author

Six of the seven addressed; the seventh is deliberately deferred with reasoning below. Each was reproduced before being fixed.

# Finding Status
1 Retry violates maxDurationMs Fixed
2 HTTP 429 labeled broken Fixed
3 Breaking schema versioned 1.3 Fixed — now 2.0
4 unverified includes targets that answered Fixed
5 Redirect-terminal response discarded Deferred, see below
6 Retry accounting + lost observation Fixed
7 README badge 5.0.0 Fixed

1 — reproduced at 10716ms on a 3000ms budget. The backoff was clipped to the deadline, but the retry it released then ran on the fixed 10s socket timeout, and no budget check can interrupt that: they run before a request starts and after it ends, never during. Every request is now bounded by min(10s, remaining maxDurationMs). Worth noting this overrun predates retries — the FIRST attempt had it too — so the fix covers both.

2 — confirmed: a 429 target produced {statusCode: 429, reason: 'http-error'} with no retry. 429 is the one error status that describes our own request rate rather than the resource, so calling it broken blames a site for how hard we crawled it. Now retried first, honouring Retry-After (bounded to 10s and clipped by the deadline); still-throttled targets report unverified / reason: 'throttled'. 5xx deliberately stays a dead link — that is conventional broken-link semantics and not a rate signal.

4 — confirmed: a redirect loop produced reason: 'fetch-error' with error: "Too many redirects (>5)." — a target that answered, repeatedly, described as silent. reason is now a typed union: unreachable / throttled / redirect-limit / body-too-large / unknown, and rows carry statusCode when there was one. This required CrawlPageObservation.errorCode — the typed code behind the human error string — since branching on the message text would be a string match on prose.

6 — confirmed, and the second half was the worse one. With the fetch budget exhausted, the target had no page row and no unverified entry at all: the URL vanished from the crawl with nothing to explain the gap, because the budget error propagated in place of the original transient one. The counter now increments only once a retry can actually be dispatched, and a deadline expiring during backoff re-raises the ORIGINAL failure so the observation survives.

5 — deferred, deliberately. Real, and reproduced. But that early return predates this work (#65), and fixing it properly means caching bodies across the frontier so a later re-fetch can reuse a response we already hold — a traversal change with dedup, ordering and checkpoint implications. Bundling it into a classification fix on an already-breaking PR trades a contained change for a risky one. This branch does improve its consequence: such a link is now reported unverified rather than as a fabricated dead link. Happy to open it as a follow-up issue.

3 — agreed, and the reasoning matters: 1.1 → 1.2 was explicitly documented as additive, so shipping a required field and two narrowed unions as a minor would tell a consumer the opposite of the truth.

Tests: 613 passing, typecheck and lint clean. New cases cover the 429 path both ways (still-throttled, and retried-then-answers), redirect-limit naming, the deadline bound (asserts under 6s where the bug measured 10.7s), and that a deadline expiring during backoff still records the page with attempted: 0.

🤖 Generated with Claude Code

arberx added a commit to Canonry/canonry that referenced this pull request Aug 16, 2026
The crawl engine marks a page `fetch-error` both when the site answers
4xx/5xx AND when the fetch never completed at all, and its dead-link
derivation accepted either. So a timeout, a reset connection, or
throttling under crawl concurrency became a reported broken link, and
every reader of `site_crawl_findings` — the API route, the CLI, the
dashboard — renders a row as broken.

On one 228-page site that produced 15 findings across 6 URLs, every one
`statusCode: null`, and every one of those 6 served a 200 in under a
second on a manual check. It was client-facing output.

`partitionDeadLinks` splits what the engine returns into links that are
BROKEN (a real 4xx/5xx status) and links we could not CHECK; only the
first are persisted as findings. The second are counted in a new
`dead_links_unverified` column that flows through contracts, the route,
the CLI and the dashboard badge. `deadLinkCheckedCount` also stops
counting unfetchable targets as checked — that was the other half of the
overstatement, and it made "1 broken of 199 checked" look like full
coverage of the link graph. The badge now reads "none found, 6
unchecked" rather than a clean bill of health.

Migration 140 reclassifies rows that are ALREADY stored, in an
allowlisted `run()` hook. The evidence to do it survives on the rows
themselves: each finding carries its own `evidence.statusCode`, so a
fabricated row is identifiable one row at a time. Because the read path
serves stored findings, leaving them would have meant rescanning one
project fixed only that project while false broken links stayed in front
of every other client. The three counts are rewritten to absolute values
derived from the surviving rows, `dead_links_checked` loses exactly the
targets that were never reached, and the fabricated rows are deleted.
Downgrade-safe on the same grounds as v132: an older binary sees fewer
rows, and the ones removed are exactly the rows it was misreporting.

`unverified` is counted per TARGET, matching `dead_links_checked`, so the
two partition the attempted targets. The engine reports one row per
(from, to) edge, so counting rows would have printed "15 unchecked"
beside "193 checked" — two units stated as one comparison.

The route also filters null-status rows defensively, on both the count
and the list, so an older writer during a rolling deploy cannot put a
fabricated broken link in front of a client.

The underlying fix is upstream in @canonry/aeo-audit 6.0.0
(Canonry/aeo-audit#73). This lands separately because the bug is live
today and because this repo pins 4.7.0 while the published 5.0.0 carries
the same bug — the upgrade is not a fix on its own.
`partitionDeadLinks` reads both engine shapes and degrades to a no-op
guard once the dependency moves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@arberx
arberx merged commit 0f3b422 into main Aug 16, 2026
4 checks passed
@arberx
arberx deleted the fix/dead-links-need-a-status-code branch August 16, 2026 20:34
arberx added a commit to Canonry/canonry that referenced this pull request Aug 16, 2026
The crawl engine marks a page `fetch-error` both when the site answers
4xx/5xx AND when the fetch never completed at all, and its dead-link
derivation accepted either. So a timeout, a reset connection, or
throttling under crawl concurrency became a reported broken link, and
every reader of `site_crawl_findings` — the API route, the CLI, the
dashboard — renders a row as broken.

On one 228-page site that produced 15 findings across 6 URLs, every one
`statusCode: null`, and every one of those 6 served a 200 in under a
second on a manual check. It was client-facing output.

`partitionDeadLinks` splits what the engine returns into links that are
BROKEN (a real 4xx/5xx status) and links we could not CHECK; only the
first are persisted as findings. The second are counted in a new
`dead_links_unverified` column that flows through contracts, the route,
the CLI and the dashboard badge. `deadLinkCheckedCount` also stops
counting unfetchable targets as checked — that was the other half of the
overstatement, and it made "1 broken of 199 checked" look like full
coverage of the link graph. The badge now reads "none found, 6
unchecked" rather than a clean bill of health.

Migration 140 reclassifies rows that are ALREADY stored, in an
allowlisted `run()` hook. The evidence to do it survives on the rows
themselves: each finding carries its own `evidence.statusCode`, so a
fabricated row is identifiable one row at a time. Because the read path
serves stored findings, leaving them would have meant rescanning one
project fixed only that project while false broken links stayed in front
of every other client. The three counts are rewritten to absolute values
derived from the surviving rows, `dead_links_checked` loses exactly the
targets that were never reached, and the fabricated rows are deleted.
Downgrade-safe on the same grounds as v132: an older binary sees fewer
rows, and the ones removed are exactly the rows it was misreporting.

`unverified` is counted per TARGET, matching `dead_links_checked`, so the
two partition the attempted targets. The engine reports one row per
(from, to) edge, so counting rows would have printed "15 unchecked"
beside "193 checked" — two units stated as one comparison.

The route also filters null-status rows defensively, on both the count
and the list, so an older writer during a rolling deploy cannot put a
fabricated broken link in front of a client.

The underlying fix is upstream in @canonry/aeo-audit 6.0.0
(Canonry/aeo-audit#73). This lands separately because the bug is live
today and because this repo pins 4.7.0 while the published 5.0.0 carries
the same bug — the upgrade is not a fix on its own.
`partitionDeadLinks` reads both engine shapes and degrades to a no-op
guard once the dependency moves.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
arberx added a commit to Canonry/canonry that referenced this pull request Aug 16, 2026
The crawl engine marks a page `fetch-error` both when the site answers
4xx/5xx AND when the fetch never completed at all, and its dead-link
derivation accepted either. So a timeout, a reset connection, or
throttling under crawl concurrency became a reported broken link, and
every reader of `site_crawl_findings` — the API route, the CLI, the
dashboard — renders a row as broken.

On one 228-page site that produced 15 findings across 6 URLs, every one
`statusCode: null`, and every one of those 6 served a 200 in under a
second on a manual check. It was client-facing output.

`partitionDeadLinks` splits what the engine returns into links that are
BROKEN (a real 4xx/5xx status) and links we could not CHECK; only the
first are persisted as findings. The second are counted in a new
`dead_links_unverified` column that flows through contracts, the route,
the CLI and the dashboard badge. `deadLinkCheckedCount` also stops
counting unfetchable targets as checked — that was the other half of the
overstatement, and it made "1 broken of 199 checked" look like full
coverage of the link graph. The badge now reads "none found, 6
unchecked" rather than a clean bill of health.

Migration 140 reclassifies rows that are ALREADY stored, in an
allowlisted `run()` hook. The evidence to do it survives on the rows
themselves: each finding carries its own `evidence.statusCode`, so a
fabricated row is identifiable one row at a time. Because the read path
serves stored findings, leaving them would have meant rescanning one
project fixed only that project while false broken links stayed in front
of every other client. The three counts are rewritten to absolute values
derived from the surviving rows, `dead_links_checked` loses exactly the
targets that were never reached, and the fabricated rows are deleted.
Downgrade-safe on the same grounds as v132: an older binary sees fewer
rows, and the ones removed are exactly the rows it was misreporting.

`unverified` is counted per TARGET, matching `dead_links_checked`, so the
two partition the attempted targets. The engine reports one row per
(from, to) edge, so counting rows would have printed "15 unchecked"
beside "193 checked" — two units stated as one comparison.

The route also filters null-status rows defensively, on both the count
and the list, so an older writer during a rolling deploy cannot put a
fabricated broken link in front of a client.

The underlying fix is upstream in @canonry/aeo-audit 6.0.0
(Canonry/aeo-audit#73). This lands separately because the bug is live
today and because this repo pins 4.7.0 while the published 5.0.0 carries
the same bug — the upgrade is not a fix on its own.
`partitionDeadLinks` reads both engine shapes and degrades to a no-op
guard once the dependency moves.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant