fix(crawl): do not fail a job because robots.txt could not be read

An origin that answers `/robots.txt` with a 503, or refuses the connection,
has failed to serve a file. It has not thereby forbidden anything. Both crawl
surfaces treated that as a terminal error: `run_crawl` marked the job Failed
without fetching the seed, and `discover_urls` returned `TargetUnreachable`
with zero URLs. Any origin whose robots.txt went down could stop a crawl
outright, and every page it never asked us to withhold went unfetched.

Against a local origin serving a 503 on `/robots.txt` and 200 on everything
else, with robots enforcement on:

    before   error: crawl failed: robots.txt unreachable  (0 pages)
    after    Crawl completed: 2 pages

Both surfaces now log which origin went dark and carry on with no rules,
which is what they did before this branch.

Two things follow from that:

- `RobotsTxt::fetch` returns `CrwResult<Self>`. The `Option` distinguished a
  4xx from a 5xx, and now that every caller proceeds either way there was
  nothing left to distinguish. A 4xx is an empty rule set; a 5xx is still an
  error so the caller has something to log.
- `discover_urls` gives the robots fetch half the remaining budget instead of
  all of it. A robots.txt that hangs used to be handed the whole deadline, so
  the seed and the sitemap probes got nothing and the caller saw zero URLs
  even before the fail-closed arm was reached.

Author: us

crates/crw-crawl/src/crawl.rs

@@ -288,20 +288,18 @@ async fn run_crawl_inner(opts: CrawlOptions<'_>) {
         .build()
         .expect("reqwest client build should not fail");
 
-    // `Ok(None)` is robots.txt absent (a 4xx), which means no rules, not a
-    // failure. `Err` is the origin itself being unreachable, and with policy
-    // enforcement on that fails the job rather than crawling unrestrained.
-    let robots = if respect_robots {
-        match RobotsTxt::fetch(&origin, &client).await {
-            Ok(found) => found.unwrap_or_default(),
-            Err(e) => {
-                send_failed(id, &state_tx, format!("robots.txt unreachable: {e}"));
-                return;
-            }
-        }
-    } else {
-        RobotsTxt::default()
-    };
+    // Fails open. An unreadable robots.txt is not a licence to fail the job:
+    // the crawl proceeds with no rules, so an origin that 503s the file cannot
+    // stop a crawl outright.
+    let robots =
+        if respect_robots {
+            RobotsTxt::fetch(&origin, &client).await.unwrap_or_else(|e| {
+            tracing::warn!(error = %e, "robots.txt unavailable, crawling without its rules");
+            RobotsTxt::default()
+        })
+        } else {
+            RobotsTxt::default()
+        };
 
     let semaphore = Arc::new(Semaphore::new(max_concurrency));
     // Key the rate limiter by eTLD+1 so subdomains under the same registered
@@ -913,23 +911,26 @@ pub async fn discover_urls(opts: DiscoverOptions<'_>) -> CrwResult<DiscoverResul
     // The client's own 15s timeout is longer than a short caller timeout, so it
     // is additionally clamped by the overall deadline — otherwise a slow
     // robots.txt alone could burn the whole budget and lose every result.
-    let timed_out =
-        || crw_core::error::CrwError::TargetUnreachable("robots.txt request timed out".into());
-    let fetched = match remaining_budget(overall_deadline) {
-        Some(budget) => tokio::time::timeout(budget, RobotsTxt::fetch(&origin, &client))
-            .await
-            .unwrap_or_else(|_| Err(timed_out())),
-        // No budget left to spend on it at all.
-        None => Err(timed_out()),
-    };
-    // `Ok(None)` is robots.txt absent, which is no rules rather than a failure.
-    let robots = match fetched {
-        Ok(found) => found.unwrap_or_default(),
-        Err(error) if respect_robots => return Err(error),
-        Err(error) => {
-            tracing::warn!(error = %error, "robots.txt unavailable, continuing without its rules");
-            RobotsTxt::default()
+    // Half the remaining budget, not all of it: a robots.txt that hangs used to
+    // be handed the whole deadline, so the seed and the sitemap probes got
+    // nothing and discovery came back with zero URLs. Half leaves the work its
+    // share. Like `run_crawl` this fails open, because an origin that cannot
+    // serve the file has not thereby forbidden anything.
+    let robots = match remaining_budget(overall_deadline) {
+        Some(budget) => {
+            match tokio::time::timeout(budget / 2, RobotsTxt::fetch(&origin, &client)).await {
+                Ok(Ok(robots)) => robots,
+                Ok(Err(error)) => {
+                    tracing::warn!(error = %error, "robots.txt unavailable, discovering without its rules");
+                    RobotsTxt::default()
+                }
+                Err(_) => {
+                    tracing::warn!("robots.txt fetch timed out, discovering without its rules");
+                    RobotsTxt::default()
+                }
+            }
         }
+        None => RobotsTxt::default(),
     };
 
     if use_sitemap {

crates/crw-crawl/src/robots.rs

@@ -18,7 +18,16 @@ pub struct RobotsTxt {
 }
 
 impl RobotsTxt {
-    pub async fn fetch(base_url: &str, client: &reqwest::Client) -> CrwResult<Option<Self>> {
+    /// Fetch and parse an origin's `/robots.txt`.
+    ///
+    /// A 4xx is a site that simply has no robots.txt, which is no rules rather
+    /// than a problem, so it comes back as an empty rule set. A 5xx or a
+    /// transport failure comes back as an error, but only so the caller can log
+    /// which origin went dark: every caller proceeds with no rules either way.
+    /// Failing a job on an unreadable robots.txt would hand any origin that
+    /// 503s the file a way to stop a crawl outright, and the pages it never
+    /// asked us to withhold would go unfetched.
+    pub async fn fetch(base_url: &str, client: &reqwest::Client) -> CrwResult<Self> {
         let url = format!("{}/robots.txt", base_url.trim_end_matches('/'));
 
         let resp = client.get(&url).send().await.map_err(|e| {
@@ -29,7 +38,7 @@ impl RobotsTxt {
         })?;
 
         if resp.status().is_client_error() {
-            return Ok(None);
+            return Ok(Self::default());
         }
         if !resp.status().is_success() {
             return Err(CrwError::TargetUnreachable(format!(
@@ -57,7 +66,7 @@ impl RobotsTxt {
             bytes.truncate(end);
         }
         let text = String::from_utf8_lossy(&bytes);
-        Ok(Some(Self::parse(&text)))
+        Ok(Self::parse(&text))
     }
 
     /// Parse into the rules that bind us.
@@ -441,18 +450,19 @@ Allow: /path
     }
 
     #[tokio::test]
+    /// A site with no robots.txt forbids nothing; an origin that cannot serve
+    /// the file reports an error the caller logs and then ignores. Both end up
+    /// crawling, which is what the callers assert.
     async fn fetch_classifies_unavailable_and_unreachable_statuses() {
         let unavailable = wiremock::MockServer::start().await;
         wiremock::Mock::given(wiremock::matchers::path("/robots.txt"))
             .respond_with(wiremock::ResponseTemplate::new(404))
             .mount(&unavailable)
             .await;
-        assert!(
-            RobotsTxt::fetch(&unavailable.uri(), &reqwest::Client::new())
-                .await
-                .unwrap()
-                .is_none()
-        );
+        let absent = RobotsTxt::fetch(&unavailable.uri(), &reqwest::Client::new())
+            .await
+            .expect("a missing robots.txt is not an error");
+        assert!(absent.is_allowed("/anything"), "no file means no rules");
 
         let unreachable = wiremock::MockServer::start().await;
         wiremock::Mock::given(wiremock::matchers::path("/robots.txt"))
@@ -506,7 +516,6 @@ Allow: /path
 
         let robots = RobotsTxt::fetch(&server.uri(), &reqwest::Client::new())
             .await
-            .unwrap()
             .unwrap();
         assert!(!robots.is_allowed("/blocked"));
     }

crates/crw-crawl/tests/crawl_headers_and_failures.rs

@@ -349,7 +349,7 @@ async fn crawl_honours_a_robots_rule_keyed_on_the_query_string() {
 }
 
 #[tokio::test]
-async fn unreachable_robots_fails_the_job_before_fetching_the_seed() {
+async fn a_robots_txt_we_cannot_read_does_not_fail_the_crawl() {
     let server = MockServer::start().await;
     Mock::given(method("GET"))
         .and(path("/robots.txt"))
@@ -359,19 +359,23 @@ async fn unreachable_robots_fails_the_job_before_fetching_the_seed() {
         .await;
     Mock::given(method("GET"))
         .and(path("/"))
-        .respond_with(ResponseTemplate::new(200))
-        .expect(0)
+        .respond_with(
+            ResponseTemplate::new(200)
+                .set_body_string("<html><body><h1>Seed page</h1></body></html>")
+                .insert_header("content-type", "text/html"),
+        )
+        .expect(1)
         .mount(&server)
         .await;
 
     let state = run_with_robots(request(format!("{}/", server.uri())), true).await;
 
-    assert_eq!(state.status, CrawlStatus::Failed);
-    assert!(!state.success);
+    assert_eq!(state.status, CrawlStatus::Completed);
+    assert!(state.success);
+    assert_eq!(state.data.len(), 1, "the seed must still be crawled");
     assert!(
-        state
-            .error
-            .as_deref()
-            .is_some_and(|error| error.contains("robots.txt unreachable"))
+        state.error.is_none(),
+        "a 503 on robots.txt is logged, not surfaced as a job error: {:?}",
+        state.error
     );
 }

crates/crw-crawl/tests/discover_tests.rs

@@ -238,9 +238,13 @@ async fn slow_site_returns_partial_results_instead_of_timing_out() {
     );
 }
 
-/// A robots.txt timeout fails closed before sitemap probes or seed crawling.
+/// A hanging robots.txt must not consume the whole budget, and must not stop
+/// discovery. It used to be handed the entire remaining deadline, so the seed
+/// and the sitemap probes got nothing left and the caller saw zero URLs; then
+/// it additionally failed the request outright. Now it gets half the budget and
+/// discovery carries on without its rules.
 #[tokio::test]
-async fn hanging_robots_fails_closed_within_the_overall_budget() {
+async fn hanging_robots_leaves_budget_for_the_seed() {
     let server = MockServer::start().await;
 
     Mock::given(method("GET"))
@@ -251,13 +255,15 @@ async fn hanging_robots_fails_closed_within_the_overall_budget() {
     Mock::given(method("GET"))
         .and(path("/sitemap.xml"))
         .respond_with(ResponseTemplate::new(404))
-        .expect(0)
         .mount(&server)
         .await;
     Mock::given(method("GET"))
         .and(path("/"))
-        .respond_with(ResponseTemplate::new(200).set_body_string(r#"<a href="/x">x</a>"#))
-        .expect(0)
+        .respond_with(
+            ResponseTemplate::new(200)
+                .insert_header("content-type", "text/html")
+                .set_body_string(r#"<html><body><a href="/x">x</a></body></html>"#),
+        )
         .mount(&server)
         .await;
 
@@ -266,7 +272,7 @@ async fn hanging_robots_fails_closed_within_the_overall_budget() {
     let result = discover_urls(opts(
         &server.uri(),
         &r,
-        Instant::now() + Duration::from_secs(3),
+        Instant::now() + Duration::from_secs(8),
         true,
     ))
     .await;
@@ -276,13 +282,24 @@ async fn hanging_robots_fails_closed_within_the_overall_budget() {
         "robots fetch must be clamped by the overall deadline, took {:?}",
         started.elapsed()
     );
-    assert!(matches!(
-        result,
-        Err(crw_core::error::CrwError::TargetUnreachable(_))
-    ));
-    let requests = server.received_requests().await.unwrap();
-    assert_eq!(requests.len(), 1);
-    assert_eq!(requests[0].url.path(), "/robots.txt");
+    let urls = result
+        .expect("a hanging robots.txt must not fail discovery")
+        .urls;
+    assert!(
+        urls.iter().any(|u| u.contains(&server.uri())),
+        "the seed must survive the robots hang, got {urls:?}"
+    );
+    let paths: Vec<String> = server
+        .received_requests()
+        .await
+        .unwrap()
+        .iter()
+        .map(|r| r.url.path().to_string())
+        .collect();
+    assert!(
+        paths.iter().any(|p| p != "/robots.txt"),
+        "work must happen after the robots timeout, only saw {paths:?}"
+    );
 }
 
 /// `max_urls` is a hard cap, not a suggestion. The base URL used to be appended
@@ -327,8 +344,11 @@ async fn seed_validation_is_bounded_by_the_overall_deadline() {
     );
 }
 
+/// A 5xx on robots.txt is the origin failing to serve a file, not the origin
+/// forbidding anything. Discovery proceeds to the seed and the sitemap probes
+/// with no rules, rather than returning the caller an error and zero URLs.
 #[tokio::test]
-async fn unreachable_robots_stops_discovery_before_seed_or_sitemap_requests() {
+async fn a_robots_txt_we_cannot_read_does_not_stop_discovery() {
     let server = MockServer::start().await;
     Mock::given(method("GET"))
         .and(path("/robots.txt"))
@@ -338,29 +358,31 @@ async fn unreachable_robots_stops_discovery_before_seed_or_sitemap_requests() {
         .await;
     Mock::given(method("GET"))
         .and(path("/"))
-        .respond_with(ResponseTemplate::new(200))
-        .expect(0)
+        .respond_with(
+            ResponseTemplate::new(200)
+                .insert_header("content-type", "text/html")
+                .set_body_string("<html><body>seed</body></html>"),
+        )
         .mount(&server)
         .await;
     Mock::given(method("GET"))
         .and(path("/sitemap.xml"))
-        .respond_with(ResponseTemplate::new(200))
-        .expect(0)
+        .respond_with(ResponseTemplate::new(404))
         .mount(&server)
         .await;
 
     let renderer = renderer().await;
-    let result = discover_urls(opts(
-        &server.uri(),
+    let uri = server.uri();
+    let urls = discover_urls(opts(
+        &uri,
         &renderer,
         Instant::now() + Duration::from_secs(10),
         true,
     ))
-    .await;
-    assert!(matches!(
-        result,
-        Err(crw_core::error::CrwError::TargetUnreachable(_))
-    ));
+    .await
+    .expect("a 503 on robots.txt must not fail discovery")
+    .urls;
+    assert!(urls.iter().any(|u| u == &uri), "got {urls:?}");
 }
 
 #[tokio::test]