fix(map): stop returning 504 with zero URLs on sites without a sitemap#276
Open
us wants to merge 1 commit into
Open
fix(map): stop returning 504 with zero URLs on sites without a sitemap#276us wants to merge 1 commit into
us wants to merge 1 commit into
Conversation
/v1/map on a sitemap-less site (news.ycombinator.com) burned the full 120s budget and returned 504 with no URLs at all. Five separate defects compounded: - The renderer had no memory of a host blocking us, so every URL re-climbed the whole ladder (direct -> 429 -> proxy retry -> JS tiers -> chrome_proxy) from scratch, ~10-20s per URL. - The BFS discovery loop only looked concurrent: it took a semaphore permit and then awaited the fetch inline, so real concurrency was 1 and max_concurrency was dead weight. - The BFS phase was given a time budget only when the sitemap had already yielded >= 50 URLs. With no sitemap it ran unbounded. - The route's tokio timeout DROPPED the discovery future, discarding every URL found so far, so a slow site produced a 504 with an empty result. - robots.txt was read only for its Sitemap: lines, and is_allowed() was handed a bare path, so query-keyed rules never matched. HN forbids /hide?, /vote? and /reply?; each was fetched anyway through a full Chrome ladder. Changes: - New crw-renderer/src/egress.rs: per-host memory of hosts that hard-block direct egress, as a moka TTL cache. Presence == latched; TTL expiry IS the half-open re-probe, so no success hook can erase it and a false latch cannot become permanent paid-proxy egress. Latched hosts start on the proxy but direct is never suppressed: a reserved budget guarantees a direct rescue even if the proxy hangs. Writes only ever happen on a genuine direct attempt seeing a strong block signal (429 / cf-mitigated), so one caller's broken proxy cannot demote another caller's healthy direct traffic. The latch stays inert below MIN_BUDGET_FOR_LATCH, leaving the 5s scrape path byte-for-byte unchanged. - discover_urls: overall_deadline clamps every awaited phase (robots, HEAD probes, sitemap tree, DNS resolution, each page) and discovery now returns the URLs it has when the budget runs out instead of dying with the future. - BFS streams its fetches concurrently and exits as soon as the URL cap is hit, rather than paying for a whole level it no longer needs. - robots.txt is honoured for FETCHING only: a disallowed link is still reported, it is simply never requested. New is_url_allowed() matches on path AND query. Crawl-delay is deliberately not honoured (HN's 30s would make map never finish); the per-host limiter remains the politeness mechanism. - max_urls is now a hard cap; the base URL counts against it instead of being appended afterwards. Verified against the live engine: HN /v1/map now returns 200 with 5000 URLs in ~22s (was 504 / 0 URLs), a 10s timeout returns partial results rather than 504, and docs.fastcrw.com and fastcrw.com show no regression in URL count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
POST /v1/mapon a site without a sitemap burned the full 120s budget and cameback 504 with zero URLs. Reproduced on prod against
news.ycombinator.com,twice. Credits were refunded, so no billing harm, but the endpoint was unusable
on any sitemap-less site.
The engine log showed the shape of it — each of these is a different URL, each
paying the full renderer ladder from scratch:
Root causes
Five defects compounded into the outage:
saw_hard_blockanduse_proxyarerequest-local, so URL chore(main): release 0.0.13 #2 on a host that just 429'd us started over at direct
HTTP:
direct -> 429 -> proxy retry -> JS ladder -> chrome_proxy. That climbis the 10-20s/URL we measured, and it is the dominant cost.
and then awaited the fetch inline in the same iteration; nothing was ever
spawned. Real concurrency was 1 and
max_concurrencywas dead weight.tokio::time::timeoutDROPPED the discovery future, throwingaway every URL found so far. That is why a slow site produced a 504 with an
empty result rather than a short list.
Sitemap:lines, andis_allowed()washanded a bare path, so query-keyed rules never matched. HN disallows
/hide?,/vote?and/reply?— every one of them was fetched anyway, through a fullChrome ladder.
What changed
New
crw-renderer/src/egress.rs— per-host memory of hosts that hard-blockour direct egress, as a moka TTL cache. Presence == latched, and TTL expiry is
the half-open re-probe, so there is no success hook that could erase it and a
false latch cannot decay into permanent paid-proxy egress.
The latch reorders egress, it never suppresses direct. A latched host starts
on the proxy, but a reserved slice of the deadline guarantees a direct rescue
even if the proxy is dead or hanging. Writes only ever happen on a genuine
direct attempt seeing a strong block signal (429 /
cf-mitigated), so onecaller's broken proxy cannot demote another caller's healthy direct traffic onto
paid bandwidth. Below
MIN_BUDGET_FOR_LATCHthe latch is inert, which leaves the5s scrape path byte-for-byte unchanged.
discover_urls— anoverall_deadlinenow clamps every awaited phase(robots, HEAD probes, sitemap tree, DNS resolution, each page), and discovery
returns the URLs it has when the budget runs out instead of dying with the
future. The outer timeout is now only a backstop.
BFS streams its fetches concurrently and exits the moment the URL cap is hit,
instead of paying for a whole level it no longer needs.
robots.txt is honoured for fetching only: a disallowed link is still
reported in the output, it is simply never requested. New
is_url_allowed()matches on path and query.
max_urlsis a hard cap now; the base URL counts against it rather thanbeing appended after the cap was enforced.
Deliberate calls, flagged rather than buried
Crawl-delayis not honoured. HN asks for 30s between fetches, which wouldmean map never finishes. Our per-host limiter stays the politeness mechanism.
This is not new behaviour —
/crawlalready ignores it.behaviour change and it is the correct one.
so every fetch contends for the same host's permits (
per_host_max_concurrentreal win here is the egress memory, not the parallelism.
Verification
Against the live engine, not just the test suite:
POST /v1/mapnews.ycombinator.comtimeout: 10Old vs new binaries were built from the same tree and measured alternately on the
same machine with a cold browser, so the comparison is apples-to-apples.
Scrape-success red line: the latch is inert on the 5s scrape budget, so that
path is unchanged by construction, and a test asserts the latch metric does not
move there. On a 15-URL live sample old scored 13/15 and new 14/15 — no
regression signal. This is a sample, not the 1000-URL benchmark.
Full suite green (1200+ tests), clippy clean, fmt clean. The latch tests were
each verified to FAIL when their guard is removed, and run 10x with no flakes.
Follow-ups (not in this PR)
run_crawl_innerhas the identical dead-semaphore bug, so/crawlis serialtoo. Real, but
/crawlworks today and fixing it widens the blast radius./crawl's robots check drops the query the same way (crawl.rs, one line).It would reduce the URL count
/crawlreturns, so it deserves its ownbenchmark verification.