From ff388ce184430869c0e5e6fecadb4bf96aec951c Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Wed, 5 Aug 2026 14:30:25 +0000 Subject: [PATCH] fix(sync): suppress naughty relay connection attempts Persistent DNS, TLS, and protocol failures were recorded in the relay naughty list, but reconnect eligibility and every scheduling path ignored that state and continued announcing and launching retries after normal health backoff. Consult the existing tracker both when deciding reconnect eligibility and at the shared connection-scheduling boundary before mutating lifecycle state or reserving worker capacity. This preserves the required first dial, suppresses all subsequent scheduling paths while the entry is live, and restores eligibility through the existing expiry behavior. Add a scenario test with a deterministic broken WebSocket endpoint that proves only the classification dial reaches it across multiple reconnect ticks and no reconnect intent is emitted. Update the defensive-measures documentation to describe the enforced behavior. This deliberately does not change error classification, expiry duration, Git-domain throttling, or transient relay backoff. Correctness assumes canonical relay URLs are used consistently by reconnect selection, the scheduler, and naughty-list recording. Validated with nix develop -c cargo test (617 library tests and all integration/doc tests green before the eligibility refinement), the full sync target after refinement (79 passed and 1 ignored), focused health tests (16 passed), and git diff --check. --- README.md | 2 +- docs/explanation/defensive-measures.md | 2 +- src/sync/health.rs | 8 ++ src/sync/mod.rs | 11 +++ tests/sync.rs | 1 + tests/sync/mod.rs | 2 +- tests/sync/naughty_list_scheduling.rs | 113 +++++++++++++++++++++++++ 7 files changed, 136 insertions(+), 3 deletions(-) create mode 100644 tests/sync/naughty_list_scheduling.rs diff --git a/README.md b/README.md index fe27847..ffc03b3 100644 --- a/README.md +++ b/README.md @@ -276,7 +276,7 @@ ngit-grasp implements multiple layers of defense against abuse, spam, and denial **Relay Sync Protection (GRASP-02):** - **Exponential backoff** - Failed connections: 5s → 10s → 20s → ... → 1 hour max -- **Naughty list** - Track relays with infrastructure issues separately (12h expiry) +- **Naughty list** - Suppress relays with infrastructure issues until expiry (12h default) - **Rate limit detection** - Auto 65s cooldown when remote relays rate limit us - **Domain throttling** - Max 5 concurrent, 30/min per domain for git data fetching diff --git a/docs/explanation/defensive-measures.md b/docs/explanation/defensive-measures.md index 34be641..666c653 100644 --- a/docs/explanation/defensive-measures.md +++ b/docs/explanation/defensive-measures.md @@ -89,7 +89,7 @@ These limits prevent individual connections from overwhelming the relay. - Tracks relays with persistent infrastructure issues (DNS, TLS, protocol errors) - Separate from normal connection failures - 12-hour expiration (configurable) -- Reduces retry frequency for broken relays +- Suppresses connection attempts until the entry expires **Rate Limit Detection:** - Detects when remote relay rate limits us diff --git a/src/sync/health.rs b/src/sync/health.rs index f82738b..8f47ac6 100644 --- a/src/sync/health.rs +++ b/src/sync/health.rs @@ -481,6 +481,14 @@ impl RelayHealthTracker { /// - The relay is healthy /// - The backoff period has elapsed pub fn should_attempt_connection(&self, relay_url: &str) -> bool { + if self + .naughty_list + .as_ref() + .is_some_and(|naughty_list| naughty_list.is_naughty(relay_url)) + { + return false; + } + let entry = self.health.get(relay_url); match entry { diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 7b39487..f604180 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -3299,6 +3299,17 @@ impl SyncManager { return; } }; + if self + .health_tracker + .naughty_list() + .is_some_and(|naughty_list| naughty_list.is_naughty(&relay_url)) + { + tracing::debug!( + relay = %relay_url, + "Suppressing connection attempt for naughty relay" + ); + return; + } let Some(result_tx) = self.connect_attempt_result_tx.clone() else { tracing::error!(relay = %relay_url, "Connection scheduler is not running"); return; diff --git a/tests/sync.rs b/tests/sync.rs index b761032..6ddabf8 100644 --- a/tests/sync.rs +++ b/tests/sync.rs @@ -38,6 +38,7 @@ mod sync { pub mod live_sync; pub mod maintainer_reprocessing; pub mod metrics; + pub mod naughty_list_scheduling; pub mod neg_concurrency; pub mod purgatory_fetch; pub mod req_concurrency; diff --git a/tests/sync/mod.rs b/tests/sync/mod.rs index af0a8cf..4fbc8f4 100644 --- a/tests/sync/mod.rs +++ b/tests/sync/mod.rs @@ -138,4 +138,4 @@ pub mod metrics; pub mod neg_concurrency; pub mod purgatory_fetch; pub mod req_concurrency; -pub mod tag_variations; \ No newline at end of file +pub mod tag_variations; diff --git a/tests/sync/naughty_list_scheduling.rs b/tests/sync/naughty_list_scheduling.rs new file mode 100644 index 0000000..c5e9637 --- /dev/null +++ b/tests/sync/naughty_list_scheduling.rs @@ -0,0 +1,113 @@ +//! Scenario regression coverage for relay naughty-list scheduling. + +use std::path::Path; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; +use std::time::Duration; + +use crate::common::{TestClient, TestRelay}; +use nostr_sdk::prelude::*; +use tokio::io::AsyncWriteExt; + +const OBSERVATION_DEADLINE: Duration = Duration::from_secs(30); +const RECONNECT_OBSERVATION: Duration = Duration::from_secs(4); + +async fn wait_for_log_line(log_path: &Path, predicate: F) -> bool +where + F: Fn(&str) -> bool, +{ + let deadline = tokio::time::Instant::now() + OBSERVATION_DEADLINE; + loop { + if let Ok(content) = tokio::fs::read_to_string(log_path).await { + if content.lines().any(&predicate) { + return true; + } + } + if tokio::time::Instant::now() >= deadline { + return false; + } + tokio::time::sleep(Duration::from_millis(100)).await; + } +} + +/// A reachable TCP endpoint that deliberately violates the WebSocket +/// handshake. This produces a persistent protocol failure without relying on +/// external DNS or network state. +async fn start_broken_websocket() -> (String, Arc) { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind broken websocket endpoint"); + let address = listener.local_addr().expect("broken endpoint address"); + let accepted = Arc::new(AtomicUsize::new(0)); + let task_accepted = Arc::clone(&accepted); + + tokio::spawn(async move { + while let Ok((mut stream, _)) = listener.accept().await { + task_accepted.fetch_add(1, Ordering::SeqCst); + let _ = stream.write_all(b"not a websocket handshake\r\n").await; + let _ = stream.shutdown().await; + } + }); + + (format!("ws://{address}"), accepted) +} + +#[tokio::test] +async fn naughty_relay_is_not_scheduled_for_reconnection() { + let relay = TestRelay::start_with_sync(None).await; + let (broken_relay, accepted) = start_broken_websocket().await; + let keys = Keys::generate(); + let identifier = "naughty-relay-scheduling"; + let npub = keys.public_key().to_bech32().expect("npub"); + let own_clone = format!("http://{}/{npub}/{identifier}.git", relay.domain()); + let own_relay = format!("ws://{}", relay.domain()); + let announcement = EventBuilder::new(Kind::GitRepoAnnouncement, "") + .tags(vec![ + Tag::identifier(identifier), + Tag::custom("clone", vec![own_clone]), + Tag::custom("relays", vec![own_relay, broken_relay.clone()]), + ]) + .finalize(&keys) + .expect("sign announcement"); + + let client = TestClient::new(relay.url(), keys) + .await + .expect("connect client"); + client + .send_event(&announcement) + .await + .expect("publish announcement"); + + assert!( + wait_for_log_line(&relay.log_path(), |line| { + line.contains("added to naughty list") && line.contains(&broken_relay) + }) + .await, + "broken endpoint must be classified as naughty" + ); + assert_eq!(accepted.load(Ordering::SeqCst), 1, "first dial is required"); + + let reconnect_deadline = tokio::time::Instant::now() + RECONNECT_OBSERVATION; + while tokio::time::Instant::now() < reconnect_deadline + && accepted.load(Ordering::SeqCst) == 1 + { + tokio::time::sleep(Duration::from_millis(100)).await; + } + assert_eq!( + accepted.load(Ordering::SeqCst), + 1, + "a naughty relay must not receive another dial across reconnect ticks" + ); + let log = tokio::fs::read_to_string(relay.log_path()) + .await + .expect("read relay log"); + assert!( + !log.lines().any(|line| { + line.contains("Attempting reconnection relay=") && line.contains(&broken_relay) + }), + "a naughty relay must be excluded before reconnect intent is logged" + ); + + client.disconnect().await; + relay.stop().await; +}