From d7df97dc4f0659fa3d1854e20057db77eae6511b Mon Sep 17 00:00:00 2001 From: ldm0 Date: Sun, 16 Aug 2026 22:02:26 +0800 Subject: [PATCH] fix(fetch): share timeout across replacement navigation --- moli-core/src/runtime/lifecycle_fetch.rs | 113 ++--------------------- moli-core/src/runtime/mod.rs | 44 ++++----- moli-core/tests/lifecycle_decision.rs | 57 ++++++++---- moli/src/app/http_error_navigation.rs | 31 +++---- moli/src/cli.rs | 5 +- moli/tests/fetch_cli.rs | 6 +- 6 files changed, 84 insertions(+), 172 deletions(-) diff --git a/moli-core/src/runtime/lifecycle_fetch.rs b/moli-core/src/runtime/lifecycle_fetch.rs index c14674446d..78fb309c51 100644 --- a/moli-core/src/runtime/lifecycle_fetch.rs +++ b/moli-core/src/runtime/lifecycle_fetch.rs @@ -1,24 +1,16 @@ //! Core fetch bridge for synchronous renderer lifecycle-target decisions. //! -//! The renderer owns the exact DCL/load boundary. This module owns the async -//! host-side budget transition: the original fetch deadline remains active -//! until the synchronous decider chooses `FollowNextDocument`, at which point a -//! separate successor-navigation budget replaces it. +//! The renderer owns the exact DCL/load boundary and any successor-navigation +//! grace period. The host keeps one deadline around the complete fetch, so a +//! lifecycle decision cannot reset or extend the caller's timeout budget. use super::{ - Browser, FetchedDocument, PageVmInitStage, RenderedDomWaitUntil, RendererLifecycleDecider, + Browser, FetchedDocument, RenderedDomWaitUntil, RendererLifecycleDecider, RendererLifecycleDecision, RendererLifecycleSnapshot, RendererReplyBoundary, }; use anyhow::{Context, Result, anyhow}; use moli_fetch::Request; -use std::{future::Future, time::Duration}; -use tokio::sync::oneshot; -use tracing::warn; - -pub(super) struct FollowTimeout { - follow_rx: oneshot::Receiver<()>, - successor_timeout: Duration, -} +use std::time::Duration; impl Browser { /// Fetches an executable document with a synchronous one-shot policy at @@ -26,14 +18,13 @@ impl Browser { /// /// The decision runs in the renderer owner turn that observes DCL/load; /// it does not expose an intermediate Page or require a second owner - /// command. `successor_timeout` reserves the independent budget used - /// after a decision chooses to follow a successor Document. + /// command. The original `timeout` covers the request, the first lifecycle + /// target, any successor-navigation grace period, and the successor target. pub async fn fetch_document_with_lifecycle_decider( &self, request: Request, wait_until: RenderedDomWaitUntil, timeout: Duration, - successor_timeout: Duration, decider: F, ) -> Result where @@ -48,27 +39,13 @@ impl Browser { ), "a lifecycle decider requires DCL, load, or done" ); - let (follow_tx, follow_rx) = oneshot::channel(); - let decider = RendererLifecycleDecider::new(move |target| { - let decision = decider(target)?; - if matches!( - decision, - RendererLifecycleDecision::FollowNextDocument { .. } - ) { - let _ = follow_tx.send(()); - } - Ok(decision) - }); + let decider = RendererLifecycleDecider::new(decider); self.fetch_document_with_wait( request, wait_until, timeout, RendererReplyBoundary::Stage, Some(decider), - Some(FollowTimeout { - follow_rx, - successor_timeout, - }), ) .await .with_context(|| { @@ -77,78 +54,4 @@ impl Browser { ) }) } - - pub(super) async fn materialize_with_follow_timeout( - &self, - raw_url: &str, - wait_until: RenderedDomWaitUntil, - timeout: Duration, - stage: PageVmInitStage, - initial_timeout: Duration, - extension: Option, - future: F, - ) -> Result - where - F: Future>, - { - let Some(mut extension) = extension else { - return self - .fetch_document_wait_timeout( - raw_url, - wait_until, - timeout, - stage, - initial_timeout, - future, - ) - .await; - }; - - let initial_deadline = tokio::time::Instant::now() - .checked_add(initial_timeout) - .unwrap_or_else(tokio::time::Instant::now); - let initial_deadline_sleep = tokio::time::sleep_until(initial_deadline); - tokio::pin!(initial_deadline_sleep); - tokio::pin!(future); - - let follow = tokio::select! { - biased; - result = &mut future => return result, - selected = &mut extension.follow_rx => selected, - _ = &mut initial_deadline_sleep => { - return Err(self.fetch_document_wait_timeout_error( - raw_url, wait_until, timeout, stage, - )); - } - }; - - if follow.is_err() { - // Finish drops the extension sender in the same callback turn. - // Keep the original deadline while the Page creation reply is - // finalized; only Follow is allowed to replace this budget. - return match tokio::time::timeout_at(initial_deadline, &mut future).await { - Ok(result) => result, - Err(_) => { - Err(self.fetch_document_wait_timeout_error(raw_url, wait_until, timeout, stage)) - } - }; - } - - match tokio::time::timeout(extension.successor_timeout, &mut future).await { - Ok(result) => result, - Err(_) => { - warn!( - url = %raw_url, - wait_until = ?wait_until, - timeout_ms = extension.successor_timeout.as_millis(), - stage = ?stage, - "successor navigation lifecycle target timed out" - ); - Err(anyhow!( - "successor navigation to {stage:?} timed out after {} ms for `{raw_url}`", - extension.successor_timeout.as_millis() - )) - } - } - } } diff --git a/moli-core/src/runtime/mod.rs b/moli-core/src/runtime/mod.rs index 678d80beff..df4d2c6497 100644 --- a/moli-core/src/runtime/mod.rs +++ b/moli-core/src/runtime/mod.rs @@ -50,8 +50,6 @@ pub use navigation_engine::{ static NEXT_SESSION_ID: AtomicU64 = AtomicU64::new(1); -use self::lifecycle_fetch::FollowTimeout; - fn wait_until_outer_timeout(wait_until: RenderedDomWaitUntil, timeout: Duration) -> Duration { match wait_until { RenderedDomWaitUntil::NetworkIdle | RenderedDomWaitUntil::DomStable => { @@ -418,7 +416,6 @@ impl Browser { timeout, RendererReplyBoundary::Stage, None, - None, ) .await } @@ -430,7 +427,6 @@ impl Browser { timeout: Duration, reply_boundary: RendererReplyBoundary, lifecycle_decider: Option, - follow_timeout: Option, ) -> Result { let stage = match wait_until { RenderedDomWaitUntil::DomContentLoaded => PageVmInitStage::DomContentLoaded, @@ -449,17 +445,23 @@ impl Browser { "starting fetch_document_allow_http_error_with_wait_until deadline" ); let outer_timeout = wait_until_outer_timeout(wait_until, timeout); - let timeout_started = Instant::now(); + let deadline = tokio::time::Instant::now() + .checked_add(outer_timeout) + .with_context(|| { + anyhow!( + "fetch wait_until {wait_until:?} timeout of {} ms exceeds the supported range", + timeout.as_millis() + ) + })?; let requested_url = request.url.clone(); if is_about_blank_url(&requested_url) { return self - .materialize_with_follow_timeout( + .fetch_document_wait_timeout( &raw_url, wait_until, timeout, stage, - outer_timeout, - follow_timeout, + deadline, self.materialize_static_html_page( &raw_url, requested_url, @@ -484,7 +486,7 @@ impl Browser { wait_until, timeout, stage, - outer_timeout, + deadline, self.fetch_service_worker_main_resource_for_navigation( &request, &navigation_loader, @@ -506,17 +508,13 @@ impl Browser { raw_response, )))); } - let remaining_timeout = outer_timeout - .checked_sub(timeout_started.elapsed()) - .unwrap_or_default(); return self - .materialize_with_follow_timeout( + .fetch_document_wait_timeout( &raw_url, wait_until, timeout, stage, - remaining_timeout, - follow_timeout, + deadline, self.materialize_streaming_raw_response_page( &raw_url, requested_url, @@ -538,9 +536,7 @@ impl Browser { wait_until, timeout, stage, - outer_timeout - .checked_sub(timeout_started.elapsed()) - .unwrap_or_default(), + deadline, navigation_loader.fetch_raw_stream(request), ) .await?; @@ -555,17 +551,13 @@ impl Browser { .map(|raw| FetchedDocument::Raw(Box::new(raw))); } - let remaining_timeout = outer_timeout - .checked_sub(timeout_started.elapsed()) - .unwrap_or_default(); let document_fetch_context_seed = navigation_loader.commit(response.final_url.clone())?; - self.materialize_with_follow_timeout( + self.fetch_document_wait_timeout( &raw_url, wait_until, timeout, stage, - remaining_timeout, - follow_timeout, + deadline, self.materialize_streaming_raw_response_page( &raw_url, requested_url, @@ -939,13 +931,13 @@ impl Browser { wait_until: RenderedDomWaitUntil, timeout: Duration, stage: PageVmInitStage, - outer_timeout: Duration, + deadline: tokio::time::Instant, future: F, ) -> Result where F: std::future::Future>, { - match tokio::time::timeout(outer_timeout, future).await { + match tokio::time::timeout_at(deadline, future).await { Ok(result) => result, Err(_) => { Err(self.fetch_document_wait_timeout_error(raw_url, wait_until, timeout, stage)) diff --git a/moli-core/tests/lifecycle_decision.rs b/moli-core/tests/lifecycle_decision.rs index 56903348ca..5e2c25be0f 100644 --- a/moli-core/tests/lifecycle_decision.rs +++ b/moli-core/tests/lifecycle_decision.rs @@ -34,15 +34,14 @@ async fn follow_http_error_navigation( url: &str, wait_until: RenderedDomWaitUntil, navigation_grace_ms: u64, - successor_timeout: Duration, + timeout: Duration, ) -> Result { executable_page( browser .fetch_document_with_lifecycle_decider( Request::get(url)?, wait_until, - Duration::from_secs(5), - successor_timeout, + timeout, move |target| { ensure!( (400..=599).contains(&target.status), @@ -72,7 +71,6 @@ async fn lifecycle_decider_finishes_without_extra_owner_command() -> Result<()> Request::get(&url)?, RenderedDomWaitUntil::DomContentLoaded, Duration::from_secs(5), - Duration::ZERO, move |target| { observed_targets_for_decider.lock().push(target); Ok(RendererLifecycleDecision::Finish) @@ -119,7 +117,6 @@ async fn lifecycle_decider_supports_static_about_blank() -> Result<()> { Request::get("about:blank")?, RenderedDomWaitUntil::Done, Duration::from_secs(1), - Duration::ZERO, move |target| { *observed_target_for_decider.lock() = Some(target); Ok(RendererLifecycleDecision::Finish) @@ -139,7 +136,7 @@ async fn lifecycle_decider_supports_static_about_blank() -> Result<()> { } #[tokio::test(flavor = "multi_thread")] -async fn follow_budget_does_not_extend_initial_stage_timeout() -> Result<()> { +async fn lifecycle_decider_does_not_extend_initial_stage_timeout() -> Result<()> { let server = FixtureServer::spawn().await?; let browser = Browser::new(AppConfig::default())?; let decider_was_called = Arc::new(Mutex::new(false)); @@ -150,14 +147,13 @@ async fn follow_budget_does_not_extend_initial_stage_timeout() -> Result<()> { Request::get(&server.url("/wait-until-domcontentloaded-runtime-script-very-slow"))?, RenderedDomWaitUntil::Load, Duration::from_millis(100), - Duration::from_secs(5), move |_| { *decider_was_called_in_hook.lock() = true; Ok(RendererLifecycleDecision::Finish) }, ) .await - .expect_err("a follow budget must not relax the initial Load deadline"); + .expect_err("a lifecycle decider must not relax the initial Load deadline"); assert!( format!("{error:#}").contains("timed out after 100 ms"), @@ -168,6 +164,42 @@ async fn follow_budget_does_not_extend_initial_stage_timeout() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread")] +async fn follow_navigation_grace_cannot_extend_fetch_timeout() -> Result<()> { + let browser = Browser::new(AppConfig::default())?; + + let result = tokio::time::timeout( + Duration::from_secs(1), + browser.fetch_document_with_lifecycle_decider( + Request::get("about:blank")?, + RenderedDomWaitUntil::Done, + Duration::from_millis(100), + |_| { + Ok(RendererLifecycleDecision::FollowNextDocument { + navigation_grace_ms: 10_000, + }) + }, + ), + ) + .await + .context("successor grace escaped the fetch deadline")?; + let error = result.expect_err("the original fetch timeout must interrupt successor grace"); + + assert!( + format!("{error:#}").contains("timed out after 100 ms"), + "error={error:#}" + ); + + let page = tokio::time::timeout( + Duration::from_secs(1), + browser.fetch_request_document_allow_http_error(Request::get("about:blank")?), + ) + .await + .context("timed-out lifecycle follow left the renderer owner blocked")??; + assert_eq!(executable_page(page)?.status(), 200); + Ok(()) +} + #[tokio::test(flavor = "multi_thread")] async fn lifecycle_decider_error_and_panic_retire_only_pending_page() -> Result<()> { let browser = Browser::new(AppConfig::default())?; @@ -177,7 +209,6 @@ async fn lifecycle_decider_error_and_panic_retire_only_pending_page() -> Result< Request::get("about:blank")?, RenderedDomWaitUntil::Done, Duration::from_secs(1), - Duration::ZERO, |_| Err(anyhow!("policy rejected target")), ) .await @@ -192,7 +223,6 @@ async fn lifecycle_decider_error_and_panic_retire_only_pending_page() -> Result< Request::get("about:blank")?, RenderedDomWaitUntil::Done, Duration::from_secs(1), - Duration::ZERO, |_| -> Result { panic!("policy panic sentinel") }, ) .await @@ -227,7 +257,6 @@ async fn http_error_navigation_wait_follows_same_url_reload_to_domcontentloaded( Request::get(&url)?, RenderedDomWaitUntil::DomContentLoaded, Duration::from_secs(5), - Duration::from_secs(6), move |target| { *observed_target_for_decider.lock() = Some(target); Ok(RendererLifecycleDecision::FollowNextDocument { @@ -356,7 +385,6 @@ async fn http_error_navigation_wait_follows_same_url_reload_to_load() -> Result< Request::get(&url)?, RenderedDomWaitUntil::Load, Duration::from_secs(5), - Duration::from_secs(6), move |target| { *observed_target_for_decider.lock() = Some(target); Ok(RendererLifecycleDecision::FollowNextDocument { @@ -403,7 +431,6 @@ async fn http_error_navigation_wait_reports_no_navigation_without_refetching() - Request::get(&url)?, RenderedDomWaitUntil::Load, Duration::from_secs(5), - Duration::from_millis(1_100), |target| { assert_eq!(target.status, 404); Ok(RendererLifecycleDecision::FollowNextDocument { @@ -432,7 +459,6 @@ async fn same_document_navigation_does_not_satisfy_http_error_replacement_wait() Request::get(&url)?, RenderedDomWaitUntil::Load, Duration::from_secs(5), - Duration::from_secs(1), |target| { ensure!(target.status == 403, "expected 403, got {}", target.status); Ok(RendererLifecycleDecision::FollowNextDocument { @@ -460,7 +486,6 @@ async fn dropping_http_error_navigation_wait_keeps_renderer_owner_usable() -> Re let mut waiting_fetch = Box::pin(browser.fetch_document_with_lifecycle_decider( Request::get(&url)?, RenderedDomWaitUntil::Load, - Duration::from_secs(5), Duration::from_secs(11), move |target| { ensure!(target.status == 404, "expected 404, got {}", target.status); @@ -503,7 +528,6 @@ async fn parked_http_error_navigation_wait_does_not_block_another_page() -> Resu let mut waiting_fetch = Box::pin(browser.fetch_document_with_lifecycle_decider( Request::get(&url)?, RenderedDomWaitUntil::Load, - Duration::from_secs(5), Duration::from_secs(11), move |target| { ensure!(target.status == 404, "expected 404, got {}", target.status); @@ -546,7 +570,6 @@ async fn http_error_replacement_wait_keeps_chained_navigation_limit() -> Result< .fetch_document_with_lifecycle_decider( Request::get(&url)?, RenderedDomWaitUntil::Load, - Duration::from_secs(5), Duration::from_secs(20), |target| { ensure!(target.status == 403, "expected 403, got {}", target.status); diff --git a/moli/src/app/http_error_navigation.rs b/moli/src/app/http_error_navigation.rs index 65dab03f31..41ce308d0a 100644 --- a/moli/src/app/http_error_navigation.rs +++ b/moli/src/app/http_error_navigation.rs @@ -18,29 +18,24 @@ pub(super) async fn fetch_with_http_error_navigation( browser: &Browser, request: Request, wait_until: RenderedDomWaitUntil, - stage_timeout: Duration, + timeout: Duration, navigation_grace: Duration, ) -> Result { let navigation_grace_ms = navigation_grace.as_millis().min(u128::from(u64::MAX)) as u64; // The first DCL/load is delivered to this synchronous decision normally. - // A 4xx/5xx gets one grace window to start a replacement navigation; the - // successor then has its own complete budget to reach the same stage. + // A 4xx/5xx may spend up to the configured grace waiting for a replacement + // navigation. That wait and the successor's matching lifecycle stage both + // consume the original timeout budget; neither starts a fresh budget. browser - .fetch_document_with_lifecycle_decider( - request, - wait_until, - stage_timeout, - navigation_grace.saturating_add(stage_timeout), - move |target| { - Ok(if is_http_error_status(target.status) { - RendererLifecycleDecision::FollowNextDocument { - navigation_grace_ms, - } - } else { - RendererLifecycleDecision::Finish - }) - }, - ) + .fetch_document_with_lifecycle_decider(request, wait_until, timeout, move |target| { + Ok(if is_http_error_status(target.status) { + RendererLifecycleDecision::FollowNextDocument { + navigation_grace_ms, + } + } else { + RendererLifecycleDecision::Finish + }) + }) .await } diff --git a/moli/src/cli.rs b/moli/src/cli.rs index 3380ec0971..170d9e13cb 100644 --- a/moli/src/cli.rs +++ b/moli/src/cli.rs @@ -135,8 +135,9 @@ pub struct FetchArgs { #[arg(long, value_parser = parse_response_json_path_arg)] pub wait_response_json: Option, - /// Maximum wait time in milliseconds. Network-idle and DOM-stable fetches - /// return the current page with a warning when this deadline expires. + /// Maximum wait time in milliseconds. Initial and HTTP-error replacement + /// navigations share one lifecycle deadline. Network-idle and DOM-stable + /// fetches return the current page with a warning when it expires. #[arg(long, alias = "wait-ms", default_value_t = 25_000)] pub timeout: u64, diff --git a/moli/tests/fetch_cli.rs b/moli/tests/fetch_cli.rs index bcf93b202a..c56c5fc1eb 100644 --- a/moli/tests/fetch_cli.rs +++ b/moli/tests/fetch_cli.rs @@ -2126,7 +2126,7 @@ fn cli_load_follows_five_same_url_replacements_after_403() -> Result<()> { } #[test] -fn cli_load_keeps_the_successor_stage_timeout_after_403_navigation() -> Result<()> { +fn cli_load_uses_one_timeout_across_403_replacement_navigation() -> Result<()> { let runtime = tokio::runtime::Runtime::new()?; let server = runtime.block_on(FixtureServer::spawn())?; let url = server.url("/wait-until-http-error-navigation"); @@ -2138,9 +2138,7 @@ fn cli_load_keeps_the_successor_stage_timeout_after_403_navigation() -> Result<( let stderr = clean_output(&output.stderr); assert!(stdout.is_empty(), "stdout={stdout}"); assert!( - stderr.contains("successor navigation") - && stderr.contains("Load") - && stderr.contains("timed out"), + stderr.contains("allow-http-error wait_until Load timed out after 250 ms"), "stderr={stderr}" ); Ok(())