Repository navigation
Feature/pluggable fetchers - #2
Open
JayBaywatch wants to merge 5 commits into
Open
JayBaywatch wants to merge 5 commits into
JayBaywatch wants to merge 5 commits into
Conversation
Introduce a Fetcher abstraction so the transport step of a crawl can be swapped without touching robots.txt handling, rate limiting, retries, caching, or link parsing: - HttpFetcher (default) reproduces the existing aiohttp behaviour. - BrowserFetcher renders pages in headless Chromium via Playwright (optional `browser` extra; imported lazily). - RoutingFetcher fetches over HTTP first and escalates to the browser when the response looks like an un-rendered SPA shell, or when the URL matches render_patterns. Spider and Crawler accept fetcher= and keep_hash_routes= (preserve "#/..." fragments for hash-router apps). Document.fetcher records which transport produced a page and is exported by the serializers. Cache entries for non-default fetchers are namespaced so legacy caches keep working. Memory: Spider(flush_to="crawl.jsonl") streams each page to disk as it is crawled, and retain_documents controls whether Documents stay in memory. Streaming callbacks now release documents as the docs always claimed. Also close three documentation/code gaps: add Spider.run(), retry 5xx responses with backoff, and populate Document.broken_internal_links after the crawl. Docs updated throughout; 53 new tests (175 total), with browser tests running against a local page and skipping when Playwright is absent.
Production guard rails raised in review: - Frontier is a collections.deque: O(1) pops for both BFS and DFS instead of list.pop(0). - Retry backoff uses equal jitter (uniform in [base/2, base]) so concurrent retries against one host spread out; backoff_jitter=False restores deterministic waits. - max_response_bytes (default 10 MiB) bounds every fetcher. HttpFetcher refuses an oversize Content-Length before reading and otherwise stops at the limit; BrowserFetcher checks the rendered DOM. Oversize pages raise ResponseTooLarge and are dropped, not retried. - Redirects are tracked: FetchResult.final_url / Document.final_url, logged at INFO, relative links resolve against the served URL, links are classified by the served host, and the Spider marks redirect targets visited so they are not fetched twice. Cache entries carry final_url. - ResponseCache.prune() removes expired entries and enforces an optional cache_max_bytes cap (oldest first); the Crawler runs it on enter. cache_ttl and cache_max_bytes are exposed on Spider and Crawler. - Empty 200 bodies parse to an empty Document instead of raising. Docs: README "Why linktrace?" rewritten around document-centric crawling for AI/data work with an honest comparison table against Scrapy; API reference, core concepts, troubleshooting and index updated for every new option. The real-estate notebook gains sections on crawling a JavaScript-rendered SPA with RoutingFetcher and flushing a crawl to JSONL (both executed against live sites). Tests: 21 new (196 total) covering frontier order, jitter bounds, size limits over Content-Length and chunked bodies, redirect resolution and dedupe, and cache pruning.
Audit of public API against the docs turned up the remaining gaps: - README: quick start points at run(), RoutingFetcher and flush_to; Document section lists final_url, fetcher, HtmlLink and BrokenLink; new "Bring Your Own Transport" and "Guard Rails for Long Crawls" sections cover the custom Fetcher contract, looks_like_spa_shell, max_response_bytes, cache_ttl, cache_max_bytes, show_progress and redirect handling; notebook list adds crawl_cnn_callbacks. - Getting started: fix stale "cd webcrawler", mention run() vs run_async(). - Core concepts: to_visit is a deque; documents may be empty when not retained; visited includes redirect targets. - API reference: document Crawler.normalize_url and keep_hash_routes. - Examples: add "Audit Redirects". - CONTRIBUTING: one-time playwright install for the browser tests. - crawl_cnn_callbacks notebook: closing cell on flush_to and retain_documents.
There was a problem hiding this comment.
🟡 Changes recommended
Single-read HTTP fetching can truncate responses and bypass size limits, while swallowed JSONL write failures can let crawls finish with silently incomplete output.
10 open issues
What changed in this PR
This PR adds pluggable transports to LinkTrace’s crawl engine, supporting plain HTTP, browser rendering, and per-URL routing. It also introduces streaming output and safeguards for longer crawls.
Changes:
- Add fetcher interfaces, optional Playwright support, automatic browser escalation, and hash-route handling.
- Add JSONL flushing, document-retention controls, synchronous execution, and post-crawl broken-link marking.
- Add response-size limits, redirect tracking, jittered retries, cache pruning, and accompanying tests and documentation.
| File | Description |
|---|---|
uv.lock |
Lock Playwright and its dependencies. |
tests/test_spider_memory.py |
Cover synchronous runs, retention, flushing, and broken links. |
tests/test_rate_limiting_and_broken_links.py |
Adapt response mocks and retry expectations. |
tests/test_hardening.py |
Cover size limits, redirects, jitter, frontier order, and cache pruning. |
tests/test_fetchers.py |
Test fetcher contracts, routing, caching, and hash routes. |
tests/test_browser_fetcher.py |
Add local browser-rendering integration tests. |
tests/conftest.py |
Provide a reusable local HTTP server fixture. |
README.md |
Introduce fetchers, streaming, safeguards, and usage examples. |
pyproject.toml |
Declare optional browser and development dependencies. |
notebooks/crawl_cnn_callbacks.ipynb |
Demonstrate JSONL flushing and retention behavior. |
notebooks/config_rules_sitemaps.ipynb |
Add browser-routing and disk-streaming examples. |
linktrace/Spider.py |
Integrate fetchers, streaming, redirect deduplication, and link auditing. |
linktrace/Serializers.py |
Share JSON record generation and export fetcher identity. |
linktrace/fetchers/routing.py |
Route requests and detect pages needing browser rendering. |
linktrace/fetchers/http.py |
Implement HTTP fetching with response-size checks. |
linktrace/fetchers/browser.py |
Implement browser rendering with bounded tab concurrency. |
linktrace/fetchers/base.py |
Define fetcher lifecycle, results, context, and errors. |
linktrace/fetchers/__init__.py |
Export the fetcher API. |
linktrace/Crawler.py |
Delegate transport and integrate retries, redirects, and hash routes. |
linktrace/cache.py |
Add namespaces, response metadata, and pruning. |
linktrace/__init__.py |
Expose fetchers through the package API. |
docs/troubleshooting.md |
Document browser setup, rendering failures, and memory controls. |
docs/index.md |
Highlight browser support and long-crawl safeguards. |
docs/getting-started.md |
Add browser installation and streaming quick starts. |
docs/examples.md |
Demonstrate rendering, custom fetchers, flushing, and redirect audits. |
docs/core-concepts.md |
Explain the transport architecture and updated crawl behavior. |
docs/api-reference.md |
Document new classes, options, methods, and metadata. |
CONTRIBUTING.md |
Add browser installation instructions for contributors. |
Review effort: Balanced
Comment on lines
+335
to
+336
| if self._flush_file is not None: | ||
| await self._flush_document(doc) |
Comment on lines
+103
to
+105
| raw = await response.content.read(limit + 1) | ||
| if len(raw) > limit: | ||
| raise ResponseTooLarge(url, limit) |
Comment on lines
+187
to
+190
| except Exception: | ||
| await self.session.close() | ||
| self.session = None | ||
| raise |
Comment on lines
345
to
+349
| for attempt in range(self.max_retries): | ||
| last_attempt = attempt >= self.max_retries - 1 | ||
| try: | ||
| headers = {"User-Agent": self.user_agent} | ||
| async with self.session.get(url, headers=headers) as response: | ||
| self._logger.debug(f"Fetching {url} (attempt {attempt + 1})") | ||
| status = response.status | ||
| response_headers = dict(response.headers) | ||
|
|
||
| if status != 200: | ||
| self._logger.error(f"Failed to fetch {url}: HTTP {status}") | ||
| doc = Document(url, None) | ||
| doc.status_code = status | ||
| doc.response_headers = response_headers | ||
| return doc | ||
|
|
||
| html = await response.text() | ||
|
|
||
| doc = self.parse_document(url, html) | ||
| doc.status_code = status | ||
| doc.response_headers = response_headers | ||
|
|
||
| # Cache successful responses | ||
| if self.cache: | ||
| await self.cache.set(url, status, response_headers, html) | ||
|
|
||
| return doc | ||
|
|
||
| except ( | ||
| TimeoutError, | ||
| aiohttp.ClientConnectorError, | ||
| aiohttp.ServerTimeoutError, | ||
| ) as e: | ||
| if attempt < self.max_retries - 1: | ||
| wait_time = 2**attempt * self.backoff_factor | ||
| self._logger.warning( | ||
| f"Transient error fetching {url} " | ||
| f"(attempt {attempt + 1}/{self.max_retries}): {e}. " | ||
| f"Retrying in {wait_time}s..." | ||
| ) | ||
| await asyncio.sleep(wait_time) | ||
| else: | ||
| self._logger.debug(f"Fetching {url} (attempt {attempt + 1})") | ||
| result = await self.fetcher.fetch(url, headers) |
| "title": doc.title, | ||
| "status_code": doc.status_code, | ||
| "domain": doc.domain, | ||
| "fetcher": doc.fetcher, |
| Only retained documents are marked; with ``retain_documents=False`` | ||
| (streaming or ``flush_to`` crawls) there is nothing to mark. | ||
| """ | ||
| status_by_url = {doc.url: doc.status_code for doc in self.documents} |
| for doc in self.documents: | ||
| doc.broken_internal_links = [ | ||
| BrokenLink(link.url, status_by_url[link.url]) | ||
| for link in doc.internal_links |
| response = await page.goto( | ||
| url, wait_until=self.wait_until, timeout=self._timeout_ms | ||
| ) | ||
| if self.wait_for_selector: |
Comment on lines
+215
to
+216
| if self._max_bytes is not None and len(body) > self._max_bytes: | ||
| raise ResponseTooLarge(url, self._max_bytes, len(body)) |
Comment on lines
+48
to
+58
| self._session = session | ||
| self._max_bytes_override = max_response_bytes | ||
| self._max_bytes: int | None = ( | ||
| None if max_response_bytes is _INHERIT else max_response_bytes # type: ignore[assignment] | ||
| ) | ||
|
|
||
| async def open(self, ctx: FetchContext) -> None: | ||
| if self._session is None: | ||
| self._session = ctx.session | ||
| if self._max_bytes_override is _INHERIT: | ||
| self._max_bytes = ctx.max_response_bytes |
Two data-loss bugs and seven smaller findings from code review of the pluggable-fetcher branch. - HttpFetcher read the body with a single StreamReader.read(limit + 1). aiohttp returns whatever is buffered, so on a real network only the first chunk came back and the page parsed with links missing. The body is now read in a loop until EOF, still abandoning it the moment it exceeds max_response_bytes. The existing chunked test passed only because the local server had already written everything; the new tests sleep between chunks. - Spider(flush_to=...) swallowed write errors as per-page failures, so a full disk let the crawl finish with rows silently missing. A failed write now raises FlushError (exported) and aborts the crawl once the current batch has settled. - The per-domain rate limit applies to every retry attempt, not just the first, so a retry never beats request_delay or Crawl-delay. - HttpFetcher rebinds to the Crawler's session on every open() unless the caller supplied one, so a fetcher or Spider instance can be reused across crawls without "Session is closed". - Crawler.__aenter__ closes the fetcher when open() fails, so a browser that launched but could not create a context is not leaked. - BrowserFetcher skips wait_for_selector / settle_ms on error responses, so a 404 no longer waits out the timeout and gets retried as transient. - BrowserFetcher enforces max_response_bytes on UTF-8 bytes, not characters. - final_url is included in JSONL lines and flattened DataFrame rows. - Broken-link marking indexes redirect targets, so a link to /b resolves when /b was only reached via /a -> /b; a direct fetch always wins. Docs updated (API reference, core concepts, troubleshooting, README). 15 new tests (211 total).
The default was hard-coded as "linktrace/0.1.0" in Spider, Crawler, SitemapParser and FetchContext, two releases behind. A new _version module reads the version from package metadata and exposes DEFAULT_USER_AGENT; all four defaults use it, and linktrace.__version__ is exported.
This branch has not been deployed
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.
Description
Type of Change
Related Issues
Closes #
Changes Made
Testing
Checklist
just fmt)Screenshots