Cache sitemap discovery results via discovery.cache_ttl_hours - #91
Merged
Conversation
Within the TTL, discover reuses the manifest's existing scope decisions instead of re-fetching robots.txt and the sitemap, cutting avoidable HTTP requests to sources on repeated runs. A dedicated discovery_run table (per-site last completed_at + complete flag) tracks freshness. TTL 0 disables caching and always re-fetches. Closes #76 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSitemap discovery now supports TTL-based caching with configuration fingerprints. Fresh complete runs rebuild summaries from manifest records without HTTP requests. Incomplete or changed runs are not reused, and the CLI reports cache hits. ChangesSitemap discovery cache
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DiscoveryRunner
participant ManifestStore
participant RobotsAndSitemapHTTP
DiscoveryRunner->>ManifestStore: Get latest discovery run
alt Fresh complete matching run
ManifestStore-->>DiscoveryRunner: Return run metadata
DiscoveryRunner->>ManifestStore: Read manifest records
ManifestStore-->>DiscoveryRunner: Return manifest records
DiscoveryRunner-->>DiscoveryRunner: Build cached summary
else Cache unavailable or expired
DiscoveryRunner->>RobotsAndSitemapHTTP: Fetch robots.txt and sitemaps
RobotsAndSitemapHTTP-->>DiscoveryRunner: Return discovery data
DiscoveryRunner->>ManifestStore: Record run status and fingerprint
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…-discovery # Conflicts: # crawler/tapio_crawler/cli.py # crawler/tapio_crawler/discovery/runner.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crawler/tapio_crawler/discovery/runner.py (1)
108-149: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftInvalidate cached state for every failed sitemap run.
Only the unreachable-required-robots path records
complete=False. Iffetch_robots_rules(),_discover_urls(), or_persist_discovered_urls()raises, the prior complete record remains active.If caching was disabled for that failed run and is later enabled,
_try_cached_summary()can reuse the earlier record. Record an incomplete state before network work starts, then replace it with the final status only after successful completion. Add exception-path coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crawler/tapio_crawler/discovery/runner.py` around lines 108 - 149, Update the sitemap flow in the surrounding discovery runner to record an incomplete discovery run before fetch_robots_rules() begins, so failures in fetch_robots_rules(), _discover_urls(), or _persist_discovered_urls() invalidate prior cached state. On successful completion, replace that record with summary.complete; preserve the existing non-sitemap behavior and add exception-path coverage.
🧹 Nitpick comments (1)
crawler/tapio_crawler/manifest/store.py (1)
62-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDescribe incomplete discovery runs correctly.
get_last_discovery_run()returns records withcomplete=False. Do not describe those records as completed runs. State that the method returns the most recently recorded run and returnsNoneonly when no run was recorded.Also applies to: 143-151
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crawler/tapio_crawler/manifest/store.py` around lines 62 - 67, Update the LastDiscoveryRun documentation and the get_last_discovery_run() documentation to describe the returned value as the most recently recorded discovery run, including incomplete runs, and state that None is returned only when no run has been recorded. Remove wording that implies every returned run completed successfully.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crawler/tapio_crawler/manifest/store.py`:
- Around line 53-58: Persist a versioned fingerprint of cache-relevant discovery
and scope configuration in the discovery_run schema and store accessors; in
crawler/tapio_crawler/discovery/runner.py lines 168-175, compare the current
fingerprint before returning cached manifest records and rerun discovery when it
differs; in crawler/tests/discovery/test_discovery_runner.py lines 211-266, add
coverage changing sitemap URLs and scope configuration within the TTL and assert
sitemap refetching and recalculated results.
---
Outside diff comments:
In `@crawler/tapio_crawler/discovery/runner.py`:
- Around line 108-149: Update the sitemap flow in the surrounding discovery
runner to record an incomplete discovery run before fetch_robots_rules() begins,
so failures in fetch_robots_rules(), _discover_urls(), or
_persist_discovered_urls() invalidate prior cached state. On successful
completion, replace that record with summary.complete; preserve the existing
non-sitemap behavior and add exception-path coverage.
---
Nitpick comments:
In `@crawler/tapio_crawler/manifest/store.py`:
- Around line 62-67: Update the LastDiscoveryRun documentation and the
get_last_discovery_run() documentation to describe the returned value as the
most recently recorded discovery run, including incomplete runs, and state that
None is returned only when no run has been recorded. Remove wording that implies
every returned run completed successfully.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a30a0e22-e6c9-4a33-9d9d-5177dfc0693b
📒 Files selected for processing (7)
crawler/tapio_crawler/cli.pycrawler/tapio_crawler/config/config_models.pycrawler/tapio_crawler/discovery/runner.pycrawler/tapio_crawler/manifest/store.pycrawler/tests/discovery/test_discovery_runner.pycrawler/tests/manifest/test_store.pycrawler/tests/test_cli.py
Cache-relevant discovery/scope config now fingerprints into the recorded discovery_run row, so a later run treats a cache hit as stale when sitemap_urls or scope rules change, even within cache_ttl_hours. A discovery run is also now recorded incomplete before robots/sitemap fetching begins, so a crash partway through leaves the site's last recorded run marked incomplete instead of leaving a stale "complete" run in place that a later call could still serve from cache. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crawler/tapio_crawler/manifest/store.py (1)
63-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComplete the changed Google-style docstrings.
crawler/tapio_crawler/manifest/store.py#L63-L73: Add anAttributessection forcompleted_at,complete, andconfig_fingerprint.crawler/tapio_crawler/discovery/runner.py#L43-L58: AddArgsandReturnssections for_config_fingerprint.crawler/tests/discovery/test_discovery_runner.py#L317-L322: Add anArgssection forstore.crawler/tests/discovery/test_discovery_runner.py#L363-L369: Add anArgssection forstore.crawler/tests/discovery/test_discovery_runner.py#L496-L503: Add anArgssection forstore.As per coding guidelines, use Google-style docstrings for all Python functions and classes, documenting summaries, parameters, return types, exceptions, examples, notes, TODOs, deprecations, references, and warnings where applicable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crawler/tapio_crawler/manifest/store.py` around lines 63 - 73, Complete the Google-style docstrings at all five sites: in crawler/tapio_crawler/manifest/store.py:63-73, add an Attributes section documenting completed_at, complete, and config_fingerprint; in crawler/tapio_crawler/discovery/runner.py:43-58, add Args and Returns sections for _config_fingerprint; and in crawler/tests/discovery/test_discovery_runner.py:317-322, 363-369, and 496-503, add an Args section documenting store for each affected test function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crawler/tapio_crawler/discovery/runner.py`:
- Around line 43-58: The cache fingerprint omits the site origin, allowing
within-TTL reuse across base URL changes. In
crawler/tapio_crawler/discovery/runner.py:43-58, add a normalized site base_url
to _config_fingerprint; update crawler/tapio_crawler/discovery/runner.py:127-132
and 218-219 to pass and compare the same base-URL-aware fingerprint. In
crawler/tests/discovery/test_discovery_runner.py:317-360, add a within-TTL test
changing only SiteConfig.base_url and assert a cache miss with robots.txt and
sitemap requests.
---
Nitpick comments:
In `@crawler/tapio_crawler/manifest/store.py`:
- Around line 63-73: Complete the Google-style docstrings at all five sites: in
crawler/tapio_crawler/manifest/store.py:63-73, add an Attributes section
documenting completed_at, complete, and config_fingerprint; in
crawler/tapio_crawler/discovery/runner.py:43-58, add Args and Returns sections
for _config_fingerprint; and in
crawler/tests/discovery/test_discovery_runner.py:317-322, 363-369, and 496-503,
add an Args section documenting store for each affected test function.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a0f99d7-5488-46ac-8f8f-4c06af9f34f6
📒 Files selected for processing (3)
crawler/tapio_crawler/discovery/runner.pycrawler/tapio_crawler/manifest/store.pycrawler/tests/discovery/test_discovery_runner.py
Discovery-run caching now includes the site's base_url in the config fingerprint so a domain change invalidates a cached run within cache_ttl_hours instead of serving manifest data discovered under the old origin. Also fills in missing docstring Args/Attributes sections flagged as nitpicks.
|
CI Results
All checks passed. 🎉 |
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
1 participant
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.
Summary
discovery.cache_ttl_hours(default 24h) toDiscoveryConfig, matching the value already documented in the crawler-improvements spec's example config.discovery_runSQLite table toManifestStore(record_discovery_run/get_last_discovery_run) tracking each site's last completed discovery run — just acompleted_attimestamp andcompleteflag, not a full run-history log (that's a larger change, deferred per Persist robots.txt and sitemap URLs as crawl-run metadata #77's discussion).DiscoveryRunner.run()checks this before fetching: if a sitemap-source site's last discovery run completed withincache_ttl_hours, it skips the robots.txt and sitemap fetch entirely and rebuilds the run summary (discovered/eligible/excluded_by_reason) from the manifest's existing records instead.DiscoveryRunSummarygains acached: boolfield so callers/logs can tell.cache_ttl_hours: 0disables caching outright — always re-fetches, matching the issue's stated escape hatch.discover's CLI output notes(cache hit; no HTTP requests made)when the summary came from cache.discovery.source == "sitemap".Closes #76
Test plan
uv run pytest— full crawler suite passes (120 tests)cache_ttl_hours=0always re-fetches; a successful run recordsdiscovery_run; an incomplete run (robots unreachable) is recorded incomplete and not served from cache;ManifestStore.record_discovery_run/get_last_discovery_runround-trip, overwrite-on-rerun, and per-site isolation; CLI prints the cache-hit noteuv run ruff check/ruff format --check— cleanuv run mypy tapio_crawler/uv run pyrefly check tapio_crawler— cleanSummary by CodeRabbit
New Features
Bug Fixes