diff --git a/crates/app/src/app/lifecycle.rs b/crates/app/src/app/lifecycle.rs index 1aa9f07d..73c7eb86 100644 --- a/crates/app/src/app/lifecycle.rs +++ b/crates/app/src/app/lifecycle.rs @@ -535,6 +535,18 @@ impl App { // `dispatch_peer_maintenance`, which keeps rounds strictly serial (skip a // tick if a prior round is still running) and is aborted on shutdown. let mut peer_maintenance_task: Option> = None; + // Loop-local in-flight handle for offloaded known-peers refresh + // (phase=29, #3756 — recurrence of #3582/#3689 through the sibling + // `peer_refresh_interval` arm the #3689 fix did not cover). + // `refresh_known_peers` awaits `tokio::net::lookup_host` DNS resolution + // plus `db_blocking("refresh-known-peers")`; under contention (e.g. the + // periodic tx-set GC holding a SQLite write lock) the latter retries up + // to busy_timeout, and running it inline inside the `select!` arm froze + // the loop for 13-29s once per minute. It is offloaded via + // `tick_peer_refresh` / `dispatch_peer_maintenance`, which keeps rounds + // strictly serial (skip a tick if a prior round is still running) and + // is aborted on shutdown. + let mut peer_refresh_task: Option> = None; // In-flight guard for the offloaded batched tx-advert flush (maxtps // iter 6). Same coalesced serial pattern as peer maintenance above: @@ -1393,12 +1405,24 @@ impl App { } } - // Refresh known peers from config + SQLite cache + // Refresh known peers from config + SQLite cache. Offloaded off + // the event loop (#3756, recurrence of #3582/#3689): this arm + // used to await `refresh_known_peers` inline, which awaits DNS + // resolution plus `db_blocking("refresh-known-peers")` — a + // production node observed the loop parked 13-29s once per + // minute (28% duty-cycle loss) while that call contended with + // the periodic tx-set GC's SQLite write lock. The growing + // contention duration itself traces to the deployed binary + // missing already-merged #3722 (a storestate slot-state trim); + // that is a pending-deploy issue outside this fix's control. + // This offload is the independent mitigation layer: it caps + // the loop's time in phase=29 to a `tokio::spawn` dispatch + // (sub-millisecond) regardless of how long the contended work + // takes, mirroring `dispatch_peer_maintenance`'s use for + // phase=28 (#3689). _ = peer_refresh_interval.tick() => { self.set_phase(29); // 29 = peer_refresh - if let Some(overlay) = self.overlay().await { - let _ = self.refresh_known_peers(&overlay).await; - } + self.tick_peer_refresh(&mut peer_refresh_task).await; } // Herder cleanup - evict expired data @@ -1446,6 +1470,13 @@ impl App { if let Some(h) = peer_maintenance_task.take() { h.abort(); } + // Abort any in-flight known-peers refresh (#3756). Its only + // side effects are idempotent upserts (`store_peer`/ + // `load_peer`), so an abort mid-flight is harmless — a + // skipped refresh simply re-runs on the next 60s tick. + if let Some(h) = peer_refresh_task.take() { + h.abort(); + } // Abort any in-flight advert flush (maxtps iter 6) — the // process is exiting; un-flushed adverts are moot. if let Some(h) = tx_advert_flush_task.take() { @@ -1664,6 +1695,33 @@ impl App { Ok(()) } + /// Dispatches the periodic known-peers refresh off the event-loop thread, + /// mirroring dispatch_peer_maintenance's use for phase 28 (#3689). + /// + /// Fixes #3756: `refresh_known_peers` awaits `tokio::net::lookup_host` DNS + /// resolution plus `db_blocking("refresh-known-peers")`, which retries + /// under SQLite write-lock contention. Running it inline in the + /// `peer_refresh_interval.tick()` `select!` arm parked the whole event + /// loop for 13-29s once per minute on a production validator, starving + /// SCP and other timers (recurrence of #3582 through the sibling code path + /// the #3689 fix did not cover). `task_slot` is the loop-local in-flight + /// handle: `dispatch_peer_maintenance` keeps rounds strictly serial (skip + /// this tick if a prior round is still running) and the caller aborts the + /// handle on shutdown. + async fn tick_peer_refresh(&self, task_slot: &mut Option>) { + let app = self.self_arc.read().await.upgrade(); + if let Some(app) = app { + dispatch_peer_maintenance( + async move { + if let Some(overlay) = app.overlay().await { + let _ = app.refresh_known_peers(&overlay).await; + } + }, + task_slot, + ); + } + } + /// Reset state after a rapid close cycle ends (no more closes or persists pending). /// /// Called when we've finished draining all buffered closes and the DB is @@ -4803,4 +4861,166 @@ mod tx_set_gc_offload_tests { let _ = tokio::time::timeout(Duration::from_secs(35), h).await; } } + + // --- #3756: App::tick_peer_refresh (phase=29 offload) ---------------------- + // + // Before #3756 the phase=29 arm did + // `self.refresh_known_peers(&overlay).await` inline on the event-loop + // thread. `refresh_known_peers` awaits DNS resolution + // (`tokio::net::lookup_host`) plus `db_blocking("refresh-known-peers")`; + // under SQLite write-lock contention (e.g. from the periodic tx-set GC + // purge) the latter retries for tens of seconds. A production validator + // observed the whole event loop parked 13-29s once per minute (28% + // duty-cycle loss) — a recurrence of #3582 through the sibling code path + // the #3689 fix did not cover. The fix extracts `tick_peer_refresh`, which + // dispatches the refresh via the existing `dispatch_peer_maintenance` + // coalesced-offload helper (mirroring phase=28). + // + // Both tests below call `App::tick_peer_refresh` directly, which does not + // exist on `origin/main` prior to this fix — they fail to *compile* there + // (a genuine pre-fix failure, not merely a runtime one). + + /// The dispatch must return promptly (loop not blocked); the refresh work + /// is handed to a detached task rather than run inline. + /// + /// Fails to compile on `origin/main`: `tick_peer_refresh` does not exist + /// there (the arm awaits `refresh_known_peers` inline instead). + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] + async fn test_peer_refresh_runs_off_event_loop_thread() { + use crate::App; + + let dir = tempfile::tempdir().expect("temp dir"); + let db_path = dir.path().join("peer-refresh-thread.db"); + let mut config = crate::config::ConfigBuilder::new() + .database_path(&db_path) + .build(); + // Hermetic: no overlay peers / no network. `start_overlay()` is never + // called, so `overlay().await` is `None` and the dispatched work is a + // fast no-op — this test only exercises the dispatch contract itself + // (prompt return, detached handle); the contention test below covers + // `refresh_known_peers`'s DB path under lock pressure. + config.is_compat_config = true; + config.overlay.known_peers = vec![]; + config.overlay.target_outbound_peers = 0; + config.overlay.max_outbound_peers = 0; + let app = Arc::new(App::new(config).await.expect("build App")); + app.set_self_arc().await; + + let mut slot: Option> = None; + + let start = Instant::now(); + app.tick_peer_refresh(&mut slot).await; + let elapsed = start.elapsed(); + + // (1) The call returned promptly — the event loop was not blocked. + assert!( + elapsed < Duration::from_millis(100), + "tick_peer_refresh blocked the caller for {:?} (expected <100ms); \ + the refresh was not offloaded off the event-loop thread", + elapsed + ); + + // (2) The work was handed to a detached task, not executed inline: + // the in-flight slot is populated immediately after the call returns, + // and the handle can be awaited independently to completion. + let handle = slot + .take() + .expect("tick_peer_refresh should have stored a handle"); + handle + .await + .expect("spawned peer-refresh task should complete"); + } + + /// Regression for #3756: a contended SQLite write lock must NOT stall the + /// dispatch (and therefore the event loop). We hold a long-lived + /// `BEGIN IMMEDIATE` write transaction on the app's DB from a SECOND + /// rusqlite connection, then drive the REAL phase=29 path + /// (`app.tick_peer_refresh`) and assert the call returns well under a 2s + /// watchdog — a regressed inline-await build would stall for as long as + /// the lock is held (tens of seconds against `busy_timeout`, matching the + /// 13-29s production observation) and trip the watchdog instead. + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] + async fn test_contended_peer_refresh_does_not_stall_loop() { + use crate::App; + + let dir = tempfile::tempdir().expect("temp dir"); + let db_path = dir.path().join("peer-refresh-contention.db"); + let mut config = crate::config::ConfigBuilder::new() + .database_path(&db_path) + .build(); + // Hermetic: no seed peers / no listening, so `start_overlay()` makes + // no real network connections; `refresh_known_peers` still runs its + // full DB path (the part that contends with the held write lock). + config.is_compat_config = true; + config.overlay.known_peers = vec![]; + config.overlay.target_outbound_peers = 0; + config.overlay.max_outbound_peers = 0; + let app = Arc::new(App::new(config).await.expect("build App")); + app.set_self_arc().await; + app.start_overlay().await.expect("start_overlay"); + + // Resolve the actual on-disk DB file the app opened. App::new may + // append a fixed filename or normalize the path; glob the temp dir + // for the .db. + let app_db_file = { + let mut found = db_path.clone(); + if !found.exists() { + for entry in std::fs::read_dir(dir.path()).expect("read temp dir") { + let p = entry.expect("dir entry").path(); + if p.extension().map(|e| e == "db").unwrap_or(false) { + found = p; + break; + } + } + } + found + }; + + // Second connection holds a write lock for the duration via BEGIN + // IMMEDIATE (acquires the RESERVED lock immediately). A short + // busy_timeout here just avoids the holder itself blocking on open. + let holder = rusqlite::Connection::open(&app_db_file).expect("open holder conn"); + holder + .busy_timeout(Duration::from_millis(100)) + .expect("set holder busy_timeout"); + holder + .execute_batch( + "BEGIN IMMEDIATE; CREATE TABLE IF NOT EXISTS _lk(x); INSERT INTO _lk VALUES (1);", + ) + .expect("acquire write lock via BEGIN IMMEDIATE"); + + // Drive the real phase=29 dispatch: tick_peer_refresh spawns + // refresh_known_peers via the offload helper; assert the DISPATCH + // returns promptly even though the write lock is held (the detached + // task absorbs the busy_timeout retry). + let mut slot: Option> = None; + + let dispatch_fut = async { + let start = Instant::now(); + app.tick_peer_refresh(&mut slot).await; + start.elapsed() + }; + + let dispatch_elapsed = tokio::time::timeout(Duration::from_secs(2), dispatch_fut) + .await + .expect( + "tick_peer_refresh did not return within the 2s watchdog — the \ + contended write lock stalled the caller (event loop); phase=29 was \ + not offloaded (regression of #3756/#3582)", + ); + + assert!( + dispatch_elapsed < Duration::from_millis(500), + "tick_peer_refresh blocked the caller for {:?} (expected <500ms) while \ + the DB write lock was contended; the busy_timeout retry was not offloaded", + dispatch_elapsed + ); + + // Release the lock so the detached refresh task can complete, then + // reap the handle to avoid leaking the task into other tests. + drop(holder); + if let Some(h) = slot.take() { + let _ = tokio::time::timeout(Duration::from_secs(35), h).await; + } + } }