From 46134c8760191f902789edb21392a79bdcbf9610 Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Tue, 7 Jul 2026 21:04:50 +0200 Subject: [PATCH] fix: close Chromium tabs on crawler fetch error paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/crawler/content.rs | 39 +++++++++++-------- backend/src/crawler/nav.rs | 57 ++++++++++++++++++++++++++++ backend/src/crawler/session.rs | 37 +++++++++++------- backend/src/crawler/source/target.rs | 50 ++++++++++++++---------- frontend/package.json | 2 +- 7 files changed, 136 insertions(+), 53 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index cae7461..80d9aa6 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.124.4" +version = "0.124.5" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 2cb9c3e..5e75240 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.124.4" +version = "0.124.5" edition = "2021" default-run = "mangalord" diff --git a/backend/src/crawler/content.rs b/backend/src/crawler/content.rs index 34068ca..f89d0d6 100644 --- a/backend/src/crawler/content.rs +++ b/backend/src/crawler/content.rs @@ -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 diff --git a/backend/src/crawler/nav.rs b/backend/src/crawler/nav.rs index 73e0a46..08fa4da 100644 --- a/backend/src/crawler/nav.rs +++ b/backend/src/crawler/nav.rs @@ -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( + close: impl std::future::Future, + body: impl std::future::Future, +) -> 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 = 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)); diff --git a/backend/src/crawler/session.rs b/backend/src/crawler/session.rs index 6a9928c..b86465a 100644 --- a/backend/src/crawler/session.rs +++ b/backend/src/crawler/session.rs @@ -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)] diff --git a/backend/src/crawler/source/target.rs b/backend/src/crawler/source/target.rs index b6dbd9a..671cbdc 100644 --- a/backend/src/crawler/source/target.rs +++ b/backend/src/crawler/source/target.rs @@ -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 diff --git a/frontend/package.json b/frontend/package.json index 0ffa3bf..2fc23cf 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.124.4", + "version": "0.124.5", "private": true, "type": "module", "scripts": {