fix: close Chromium tabs on crawler fetch error paths
new_page() opened a tab that was only closed on the happy path; a `?` on wait_for_nav / content() leaked it (chromiumoxide doesn't close on drop), so errored fetches accumulated tabs in the shared browser. Add a generic, unit-tested `close_after(close, body)` helper and wrap the three fetch sites (chapter content, session probe, source navigate) so the tab is closed on every exit path — including navigate's content()-read branch, which previously leaked. Bump to 0.124.5. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -122,22 +122,31 @@ async fn fetch_chapter_html_once(
|
||||
.new_page(source_url)
|
||||
.await
|
||||
.with_context(|| format!("open chapter page {source_url}"))?;
|
||||
crate::crawler::nav::wait_for_nav(&page)
|
||||
.await
|
||||
.context("wait for chapter nav")?;
|
||||
// Best-effort wait for the reader marker — same partial-render
|
||||
// race that bit the chapter-list parser can hit here. Timeout is
|
||||
// not an error; the chapter probe + parser sentinels still catch
|
||||
// real failures.
|
||||
let _ = crate::crawler::nav::wait_for_selector(
|
||||
&page,
|
||||
"a#pic_container",
|
||||
crate::crawler::nav::SELECTOR_TIMEOUT,
|
||||
// Close the tab on every exit path — a `?` on wait_for_nav / content()
|
||||
// would otherwise leak it (chromiumoxide doesn't close on drop).
|
||||
let closer = page.clone();
|
||||
crate::crawler::nav::close_after(
|
||||
async move {
|
||||
closer.close().await.ok();
|
||||
},
|
||||
async {
|
||||
crate::crawler::nav::wait_for_nav(&page)
|
||||
.await
|
||||
.context("wait for chapter nav")?;
|
||||
// Best-effort wait for the reader marker — same partial-render
|
||||
// race that bit the chapter-list parser can hit here. Timeout is
|
||||
// not an error; the chapter probe + parser sentinels still catch
|
||||
// real failures.
|
||||
let _ = crate::crawler::nav::wait_for_selector(
|
||||
&page,
|
||||
"a#pic_container",
|
||||
crate::crawler::nav::SELECTOR_TIMEOUT,
|
||||
)
|
||||
.await;
|
||||
page.content().await.context("read chapter html")
|
||||
},
|
||||
)
|
||||
.await;
|
||||
let html = page.content().await.context("read chapter html")?;
|
||||
page.close().await.ok();
|
||||
Ok(html)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Pure-over-IO loop: fetch + classify, up to `max_attempts` total
|
||||
|
||||
@@ -85,6 +85,28 @@ pub async fn wait_for_selector(
|
||||
/// under a second.
|
||||
pub const SELECTOR_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
|
||||
/// Run `body` to completion, then run `close` — on *every* exit path,
|
||||
/// including an early `?` / `return` inside `body`. chromiumoxide's `Page`
|
||||
/// does **not** close its CDP target on drop, so a fetch helper that opens a
|
||||
/// page with `browser.new_page(...)` and then bails via `?` on a nav /
|
||||
/// content-read error would leak a browser tab for the process's lifetime.
|
||||
/// Wrapping the fallible work in `close_after` — with `close` built from a
|
||||
/// clone of the page — guarantees the tab is closed regardless of how `body`
|
||||
/// returns.
|
||||
///
|
||||
/// Both arguments are pre-built futures, so this stays generic over the
|
||||
/// body's return type (the anyhow and `PageError` fetch paths both use it)
|
||||
/// and references no browser types — which also lets it be unit-tested
|
||||
/// without standing up a real `Page`.
|
||||
pub(crate) async fn close_after<R>(
|
||||
close: impl std::future::Future<Output = ()>,
|
||||
body: impl std::future::Future<Output = R>,
|
||||
) -> R {
|
||||
let result = body.await;
|
||||
close.await;
|
||||
result
|
||||
}
|
||||
|
||||
impl NavError {
|
||||
/// Does this navigation error indicate the underlying Chromium
|
||||
/// process has died or its CDP connection has dropped? Used by the
|
||||
@@ -150,6 +172,41 @@ mod tests {
|
||||
assert!(result.is_err(), "expected Elapsed on a hung future");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn close_after_runs_close_and_returns_value_on_ok() {
|
||||
use std::sync::atomic::{AtomicBool, Ordering};
|
||||
use std::sync::Arc;
|
||||
let closed = Arc::new(AtomicBool::new(false));
|
||||
let c = closed.clone();
|
||||
let out: anyhow::Result<i32> = close_after(
|
||||
async move { c.store(true, Ordering::SeqCst) },
|
||||
async { Ok(7) },
|
||||
)
|
||||
.await;
|
||||
assert_eq!(out.unwrap(), 7);
|
||||
assert!(closed.load(Ordering::SeqCst), "close must run on the happy path");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn close_after_runs_close_even_when_body_errs() {
|
||||
use std::sync::atomic::{AtomicBool, Ordering};
|
||||
use std::sync::Arc;
|
||||
let closed = Arc::new(AtomicBool::new(false));
|
||||
let c = closed.clone();
|
||||
// This is the leak the fix targets: the body bails before it could
|
||||
// close the page itself, and `close_after` must still close it.
|
||||
let out: anyhow::Result<()> =
|
||||
close_after(async move { c.store(true, Ordering::SeqCst) }, async {
|
||||
anyhow::bail!("nav failed")
|
||||
})
|
||||
.await;
|
||||
assert!(out.is_err());
|
||||
assert!(
|
||||
closed.load(Ordering::SeqCst),
|
||||
"the page must be closed even when the body returns Err"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn nav_error_timeout_message_includes_duration() {
|
||||
let e = NavError::Timeout(Duration::from_secs(30));
|
||||
|
||||
@@ -295,21 +295,30 @@ async fn fetch_probe_html(browser: &Browser, probe_url: &str) -> anyhow::Result<
|
||||
.new_page(probe_url)
|
||||
.await
|
||||
.with_context(|| format!("open probe page {probe_url}"))?;
|
||||
crate::crawler::nav::wait_for_nav(&page)
|
||||
.await
|
||||
.context("wait for nav on probe")?;
|
||||
// Best-effort wait for the layout marker. Timeout is fine — the
|
||||
// probe classifier handles a missing `#logo` as Transient anyway,
|
||||
// and the verify loop retries on Transient.
|
||||
let _ = crate::crawler::nav::wait_for_selector(
|
||||
&page,
|
||||
"#logo",
|
||||
crate::crawler::nav::SELECTOR_TIMEOUT,
|
||||
// Close the tab on every exit path — a `?` on wait_for_nav / content()
|
||||
// would otherwise leak it (chromiumoxide doesn't close on drop).
|
||||
let closer = page.clone();
|
||||
crate::crawler::nav::close_after(
|
||||
async move {
|
||||
closer.close().await.ok();
|
||||
},
|
||||
async {
|
||||
crate::crawler::nav::wait_for_nav(&page)
|
||||
.await
|
||||
.context("wait for nav on probe")?;
|
||||
// Best-effort wait for the layout marker. Timeout is fine — the
|
||||
// probe classifier handles a missing `#logo` as Transient anyway,
|
||||
// and the verify loop retries on Transient.
|
||||
let _ = crate::crawler::nav::wait_for_selector(
|
||||
&page,
|
||||
"#logo",
|
||||
crate::crawler::nav::SELECTOR_TIMEOUT,
|
||||
)
|
||||
.await;
|
||||
page.content().await.context("read probe html")
|
||||
},
|
||||
)
|
||||
.await;
|
||||
let html = page.content().await.context("read probe html")?;
|
||||
page.close().await.ok();
|
||||
Ok(html)
|
||||
.await
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -20,7 +20,7 @@ use super::{
|
||||
use crate::crawler::detect::{
|
||||
has_logo_sentinel, is_broken_page_body, retry_on_transient_with_hook, PageError,
|
||||
};
|
||||
use crate::crawler::nav::{wait_for_nav, wait_for_selector, NavError, SELECTOR_TIMEOUT};
|
||||
use crate::crawler::nav::{close_after, wait_for_nav, wait_for_selector, NavError, SELECTOR_TIMEOUT};
|
||||
use crate::crawler::safety::ensure_public_target;
|
||||
|
||||
/// `sources.id` value for this Source impl. Exposed as a const so the
|
||||
@@ -258,26 +258,34 @@ async fn navigate(
|
||||
.new_page(url)
|
||||
.await
|
||||
.map_err(|e| PageError::Other(anyhow::Error::from(e)))?;
|
||||
match wait_for_nav(&page).await {
|
||||
Ok(()) => {}
|
||||
Err(NavError::Timeout(_)) => {
|
||||
page.close().await.ok();
|
||||
return Err(PageError::transient("nav timeout"));
|
||||
}
|
||||
Err(NavError::Cdp(e)) => {
|
||||
page.close().await.ok();
|
||||
return Err(PageError::Other(anyhow::Error::from(e)));
|
||||
}
|
||||
}
|
||||
// Best-effort wait for the page-type marker. We deliberately
|
||||
// discard a timeout here — see fn-level doc.
|
||||
let _ = wait_for_selector(&page, marker, SELECTOR_TIMEOUT).await;
|
||||
let html = page
|
||||
.content()
|
||||
.await
|
||||
.map_err(|e| PageError::Other(anyhow::Error::from(e)))?;
|
||||
page.close().await.ok();
|
||||
classify_navigate_html(html)
|
||||
// Close the tab on every exit path — the previous code closed on the two
|
||||
// nav-error branches but leaked it on a content() read error.
|
||||
let closer = page.clone();
|
||||
close_after(
|
||||
async move {
|
||||
closer.close().await.ok();
|
||||
},
|
||||
async {
|
||||
match wait_for_nav(&page).await {
|
||||
Ok(()) => {}
|
||||
Err(NavError::Timeout(_)) => {
|
||||
return Err(PageError::transient("nav timeout"));
|
||||
}
|
||||
Err(NavError::Cdp(e)) => {
|
||||
return Err(PageError::Other(anyhow::Error::from(e)));
|
||||
}
|
||||
}
|
||||
// Best-effort wait for the page-type marker. We deliberately
|
||||
// discard a timeout here — see fn-level doc.
|
||||
let _ = wait_for_selector(&page, marker, SELECTOR_TIMEOUT).await;
|
||||
let html = page
|
||||
.content()
|
||||
.await
|
||||
.map_err(|e| PageError::Other(anyhow::Error::from(e)))?;
|
||||
classify_navigate_html(html)
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
/// Classify a fetched body. The broken-page template is universal across
|
||||
|
||||
Reference in New Issue
Block a user