From d21c49e0e316f1db819ab08de72d2d72fafd71d9 Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sat, 31 Oct 2026 01:11:55 +1100 Subject: [PATCH 1/2] Cut browser test time by shortening deadlines those tests wait out Most of the time in the 4 slowest browser tests went on deadlines that fail by design: 45 s of attachment budget for each upload fault that never settles, and 5 s of close watching for each refused close. Test-only overrides shorten those waits for the cases that wait them out. Cases that refuse on a first read keep the real budget, so expiry cannot stand in for the refusal they test. The close watch only reads the target list, so its 1 s override has no durable write to leave room for. The 5 s command budget is unchanged. Production values are unchanged: the overrides exist only under cfg(test). --- src/browser_retirement.rs | 6 ++++++ src/browser_runner.rs | 14 +++++++++++++- src/browser_runner_tests.rs | 26 ++++++++++++++++++++++++++ src/browser_target.rs | 10 ++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/browser_retirement.rs b/src/browser_retirement.rs index 7090dfa..c2bf9f1 100644 --- a/src/browser_retirement.rs +++ b/src/browser_retirement.rs @@ -1124,6 +1124,12 @@ async fn retire( if let Some(check) = check { check()?; } + #[cfg(test)] + let operation_deadline = crate::browser_target::TEST_CLOSE_WAIT + .get() + .map_or(operation_deadline, |wait| { + operation_deadline.min(Instant::now() + wait) + }); let _ = browser .close_owned_target(id, remaining(operation_deadline)?, cancel) .await; diff --git a/src/browser_runner.rs b/src/browser_runner.rs index 7ee8c26..4a56393 100644 --- a/src/browser_runner.rs +++ b/src/browser_runner.rs @@ -4186,10 +4186,22 @@ fn capture_image_summary( let capture = capture_outcome(store, id, witness, outcome, &text, None, cancel)?; Ok((capture, text)) } +const ATTACHMENT_BUDGET_FLOOR: Duration = Duration::from_secs(45); +#[cfg(test)] +thread_local! { + static TEST_ATTACHMENT_BUDGET_FLOOR: std::cell::Cell> = const { std::cell::Cell::new(None) }; +} +fn attachment_budget_floor() -> Duration { + #[cfg(test)] + if let Some(floor) = TEST_ATTACHMENT_BUDGET_FLOOR.get() { + return floor; + } + ATTACHMENT_BUDGET_FLOOR +} /// Reference attachment completion budget: max(inputTimeoutMs, 45 s) plus 20 s for each /// further file, raised to `attachmentTimeoutMs`. All of a batch's uploads share it. fn attachment_budget(config: &Value, limits: &RunnerLimits, files: usize) -> Duration { - let budget = limits.command.max(Duration::from_secs(45)) + let budget = limits.command.max(attachment_budget_floor()) + Duration::from_secs(20).saturating_mul(files.saturating_sub(1) as u32); budget.max(Duration::from_millis( config["attachmentTimeoutMs"].as_u64().unwrap_or(0), diff --git a/src/browser_runner_tests.rs b/src/browser_runner_tests.rs index f4b8756..1b119c4 100644 --- a/src/browser_runner_tests.rs +++ b/src/browser_runner_tests.rs @@ -2063,6 +2063,11 @@ fn limits() -> RunnerLimits { bringup: Duration::from_secs(5), } } +/// For uploads that wait out their budget by design: the fixture's command budget, in place of +/// the 45 s production floor. `attachment_budget` still takes the larger of the two. +const WAITED_OUT_ATTACHMENT_FLOOR: Duration = Duration::from_secs(5); +/// For closes the fixture refuses, which production watches for its whole budget. +const REFUSED_CLOSE_WAIT: Duration = Duration::from_secs(1); /// Limits for cases whose commit or answer deadline expires before a bounded provider read. /// That read's whole budget is min(commit, answer), so the fixture's 500 ms leaves too little /// for its identity checks and receipts once a parallel suite stalls native publication. @@ -5670,6 +5675,7 @@ async fn a_lock_held_through_cleanup_and_warning_keeps_the_completed_answer() { #[tokio::test] async fn completed_close_retries_resume_only_the_exact_durable_marker() { + crate::browser_target::TEST_CLOSE_WAIT.set(Some(REFUSED_CLOSE_WAIT)); for explicit in [false, true] { for fault in [ Fault::CloseRefusedTwice, @@ -5740,10 +5746,12 @@ async fn completed_close_retries_resume_only_the_exact_durable_marker() { stop(f).await; } } + crate::browser_target::TEST_CLOSE_WAIT.set(None); } #[tokio::test] async fn same_process_reconciliation_never_closes_running_or_uncertain_requests() { + crate::browser_target::TEST_CLOSE_WAIT.set(Some(REFUSED_CLOSE_WAIT)); for case in [ "running", "send-uncertain", @@ -5793,6 +5801,7 @@ async fn same_process_reconciliation_never_closes_running_or_uncertain_requests( assert_eq!(f.sends.load(Ordering::SeqCst), 1); stop(f).await; } + crate::browser_target::TEST_CLOSE_WAIT.set(None); } #[tokio::test] @@ -6724,6 +6733,9 @@ async fn source_mismatch_historical_echo_incomplete_answer_and_lost_ack_never_re #[tokio::test] async fn owned_upload_must_reach_readiness_and_cleanup_before_send() { for fault in [Fault::None, Fault::UploadFailed] { + // The failure waits out the budget by design; the successful upload keeps the real one. + TEST_ATTACHMENT_BUDGET_FLOOR + .set(matches!(fault, Fault::UploadFailed).then_some(WAITED_OUT_ATTACHMENT_FLOOR)); let f = fixture(fault, true).await; let result = run(&f).await; assert_eq!(result.is_ok(), matches!(fault, Fault::None)); @@ -6758,6 +6770,7 @@ async fn owned_upload_must_reach_readiness_and_cleanup_before_send() { assert!(f.home.path().join("public.txt").exists()); stop(f).await; } + TEST_ATTACHMENT_BUDGET_FLOOR.set(None); } #[tokio::test] async fn unconfirmed_upload_commands_or_cleanup_never_authorise_close_or_send() { @@ -6770,6 +6783,18 @@ async fn unconfirmed_upload_commands_or_cleanup_never_authorise_close_or_send() Fault::UploadCleanupWrongOwner, Fault::UploadCleanupWrongNavigation, ] { + // These wait out the budget by design; the wrong receipts refuse on their first read and + // keep the real budget, so expiry cannot stand in for the refusal they test. + TEST_ATTACHMENT_BUDGET_FLOOR.set( + matches!( + fault, + Fault::UploadObjectUnacknowledged + | Fault::UploadDescribeUnacknowledged + | Fault::UploadCommandUnacknowledged + | Fault::UploadCleanupFailed + ) + .then_some(WAITED_OUT_ATTACHMENT_FLOOR), + ); let f = fixture(fault, true).await; assert!(run(&f).await.is_err(), "{fault:?}"); assert_eq!(f.sends.load(Ordering::SeqCst), 0, "{fault:?}"); @@ -6789,6 +6814,7 @@ async fn unconfirmed_upload_commands_or_cleanup_never_authorise_close_or_send() ); stop(f).await; } + TEST_ATTACHMENT_BUDGET_FLOOR.set(None); } #[tokio::test] async fn a_cleaned_second_upload_failure_retires_the_owned_target_with_its_first_upload() { diff --git a/src/browser_target.rs b/src/browser_target.rs index 22033a8..6701142 100644 --- a/src/browser_target.rs +++ b/src/browser_target.rs @@ -10,6 +10,12 @@ use tokio::{ use tokio_util::sync::CancellationToken; const LATE_CLEANUP: Duration = Duration::from_millis(750); +#[cfg(test)] +thread_local! { + /// Shortens the watch on a close the fixture refuses. The watch only reads the target list, + /// so unlike the command budget it has no durable write to leave room for. + pub(crate) static TEST_CLOSE_WAIT: std::cell::Cell> = const { std::cell::Cell::new(None) }; +} fn allocation_remaining(deadline: std::time::Instant) -> Result { let remaining = deadline.saturating_duration_since(std::time::Instant::now()); anyhow::ensure!( @@ -471,6 +477,10 @@ impl OwnedTarget { self.lease.confirm_target_absent(&self.id) } pub(crate) async fn close_observed(&mut self, timeout: Duration) -> Result<()> { + #[cfg(test)] + let timeout = TEST_CLOSE_WAIT + .get() + .map_or(timeout, |wait| timeout.min(wait)); if self .creation .as_ref() From afd83ae9214baa2f9376c077e589e47d571839be Mon Sep 17 00:00:00 2001 From: Ryan Duguid Date: Sat, 31 Oct 2026 02:10:22 +1100 Subject: [PATCH 2/2] Shorten only the watch after a refused close, not the close command The override also capped the close command, so a slow but successful close could have become a timeout that exercised a different path. --- src/browser_retirement.rs | 6 +++--- src/browser_target.rs | 17 +++++++++++------ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/browser_retirement.rs b/src/browser_retirement.rs index c2bf9f1..d20d7da 100644 --- a/src/browser_retirement.rs +++ b/src/browser_retirement.rs @@ -1124,15 +1124,15 @@ async fn retire( if let Some(check) = check { check()?; } + let _ = browser + .close_owned_target(id, remaining(operation_deadline)?, cancel) + .await; #[cfg(test)] let operation_deadline = crate::browser_target::TEST_CLOSE_WAIT .get() .map_or(operation_deadline, |wait| { operation_deadline.min(Instant::now() + wait) }); - let _ = browser - .close_owned_target(id, remaining(operation_deadline)?, cancel) - .await; loop { if targets(&browser, operation_deadline) .await? diff --git a/src/browser_target.rs b/src/browser_target.rs index 6701142..f35b787 100644 --- a/src/browser_target.rs +++ b/src/browser_target.rs @@ -12,8 +12,9 @@ use tokio_util::sync::CancellationToken; const LATE_CLEANUP: Duration = Duration::from_millis(750); #[cfg(test)] thread_local! { - /// Shortens the watch on a close the fixture refuses. The watch only reads the target list, - /// so unlike the command budget it has no durable write to leave room for. + /// Shortens the watch after a close the fixture refuses, not the close command itself. The + /// watch only reads the target list, so unlike the command budget it needs no room for a + /// durable write. pub(crate) static TEST_CLOSE_WAIT: std::cell::Cell> = const { std::cell::Cell::new(None) }; } fn allocation_remaining(deadline: std::time::Instant) -> Result { @@ -477,10 +478,6 @@ impl OwnedTarget { self.lease.confirm_target_absent(&self.id) } pub(crate) async fn close_observed(&mut self, timeout: Duration) -> Result<()> { - #[cfg(test)] - let timeout = TEST_CLOSE_WAIT - .get() - .map_or(timeout, |wait| timeout.min(wait)); if self .creation .as_ref() @@ -497,6 +494,10 @@ impl OwnedTarget { .browser .close_owned_target(&self.id, timeout, &cancel) .await; + #[cfg(test)] + let deadline = TEST_CLOSE_WAIT + .get() + .map_or(deadline, |wait| deadline.min(Instant::now() + wait)); loop { let remaining = deadline.saturating_duration_since(Instant::now()); anyhow::ensure!( @@ -575,6 +576,10 @@ impl OwnedTarget { .browser .close_owned_target(&self.id, remaining, &CancellationToken::new()) .await; + #[cfg(test)] + let deadline = TEST_CLOSE_WAIT.get().map_or(deadline, |wait| { + deadline.min(std::time::Instant::now() + wait) + }); loop { if self.observe_absence_until(deadline).await? { return Ok(());