From e93bf707bb5f8d690393449cee1b402f123ac923 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 26 Jan 2026 09:39:48 +0000 Subject: fix: git naughty list DNS failure identication caught a production bug where npub in url string contained "dns" triggering false positive --- src/sync/naughty_list.rs | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) (limited to 'src/sync') diff --git a/src/sync/naughty_list.rs b/src/sync/naughty_list.rs index 35fcc0f..097affe 100644 --- a/src/sync/naughty_list.rs +++ b/src/sync/naughty_list.rs @@ -114,11 +114,15 @@ impl NaughtyListTracker { pub fn classify_error(error: &str) -> Option { let error_lower = error.to_lowercase(); - // DNS lookup failures + // DNS lookup failures - use specific patterns to avoid false positives + // from URLs containing "dns" (e.g., npubs like "...cdns7..." or domains) if error_lower.contains("failed to lookup address") || error_lower.contains("name or service not known") || error_lower.contains("nodename nor servname provided") - || (error_lower.contains("dns") && !error_lower.contains("timeout")) + || error_lower.contains("dns error") + || error_lower.contains("dns lookup") + || error_lower.contains("dns resolution") + || error_lower.contains("getaddrinfo") { return Some(NaughtyCategory::DnsLookupFailed); } @@ -373,6 +377,34 @@ mod tests { NaughtyListTracker::classify_error("network unreachable"), None ); + + // Repository not found is transient (not an infrastructure issue) + assert_eq!( + NaughtyListTracker::classify_error( + "fatal: repository 'https://example.com/repo.git/' not found" + ), + None + ); + } + + #[test] + fn test_classify_false_positive_npub_with_dns() { + // This npub contains "dns" in its encoding: npub17plqkxhsv66g8quxxc9p5t9mxazzn20m426exqnl8lxnh5a4cDNS7jezx0 + // A "not found" error with this npub should NOT be classified as DNS failure + let error = "fatal: repository 'https://git.shakespeare.diy/npub17plqkxhsv66g8quxxc9p5t9mxazzn20m426exqnl8lxnh5a4cdns7jezx0/kuboslopp%20by%20Shakespeare.git/' not found"; + assert_eq!( + NaughtyListTracker::classify_error(error), + None, + "npub containing 'dns' should not trigger DNS failure classification" + ); + + // Same for relay.ngit.dev + let error2 = "fatal: repository 'https://relay.ngit.dev/npub17plqkxhsv66g8quxxc9p5t9mxazzn20m426exqnl8lxnh5a4cdns7jezx0/kuboslopp%20by%20Shakespeare.git/' not found"; + assert_eq!( + NaughtyListTracker::classify_error(error2), + None, + "npub containing 'dns' should not trigger DNS failure classification" + ); } #[test] -- cgit v1.2.3 From 905ebd838a9ff8cc777cf3b3b6306066e8c177fc Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 26 Jan 2026 17:20:11 +0000 Subject: fix: load existing events from database on startup with two-pass queries Previously, SelfSubscriber only saw events returned by the WebSocket subscription to the local relay, which has limits on the number of events returned. This caused repos with announcements in the database to never get Layer 2/3 filters created, resulting in missing state events. Now, on startup, we query the database directly with two separate queries: 1. Query announcements (30617) to populate repo_sync_index 2. Query root events (1617/1618/1621) to create Layer 3 filters Both queries use .since(last_connected) if available for incremental loading on reconnect. Filters are created inline and made mutable to support the .since() clause, rather than using a shared create_event_filter() method. Fixes the issue where state events were missing for repos like cashbird and creative-space that had announcements in the database but weren't returned by the WebSocket subscription. --- src/sync/mod.rs | 1 + src/sync/self_subscriber.rs | 167 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 145 insertions(+), 23 deletions(-) (limited to 'src/sync') diff --git a/src/sync/mod.rs b/src/sync/mod.rs index bc8c428..226e681 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -1442,6 +1442,7 @@ impl SyncManager { self.service_domain.clone(), Arc::clone(&self.repo_sync_index), action_tx, + self.database.clone(), ); let subscriber_shutdown = shutdown_tx.subscribe(); tokio::spawn(async move { self_subscriber.run(Some(subscriber_shutdown)).await }); diff --git a/src/sync/self_subscriber.rs b/src/sync/self_subscriber.rs index 3cc408d..e9505f1 100644 --- a/src/sync/self_subscriber.rs +++ b/src/sync/self_subscriber.rs @@ -16,6 +16,8 @@ use nostr_sdk::Timestamp; use tokio::sync::broadcast::error::RecvError; use tokio::sync::{broadcast, mpsc}; +use crate::nostr::builder::SharedDatabase; + use super::{AddFilters, RepoSyncIndex, RepoSyncNeeds}; // ============================================================================= @@ -98,6 +100,8 @@ pub struct SelfSubscriber { action_tx: mpsc::Sender, /// Last time we connected - used for since filter on reconnect last_connected: Option, + /// Database for querying existing events on startup + database: SharedDatabase, } impl SelfSubscriber { @@ -108,11 +112,13 @@ impl SelfSubscriber { /// * `relay_domain` - Our service domain (used for filtering relevant repos) /// * `repo_sync_index` - Shared index to update with discovered repos /// * `action_tx` - Channel to send AddFilters actions to the SyncManager + /// * `database` - Database for querying existing events on startup pub fn new( own_relay_url: String, relay_domain: String, repo_sync_index: RepoSyncIndex, action_tx: mpsc::Sender, + database: SharedDatabase, ) -> Self { Self { own_relay_url, @@ -120,6 +126,7 @@ impl SelfSubscriber { repo_sync_index, action_tx, last_connected: None, + database, } } @@ -135,6 +142,127 @@ impl SelfSubscriber { .unwrap_or(Duration::from_millis(5000)) } + /// Load existing events from database on startup + /// + /// Queries the database with two separate queries to build the initial + /// PendingUpdates state. This ensures all repos get Layer 2/3 filters + /// created, not just those returned by the WebSocket subscription + /// (which has limits on the number of events returned). + /// + /// Query order: + /// 1. First query: Get announcements (30617) to populate repo_sync_index + /// with repos and their relays + /// 2. Second query: Get root events (1617/1618/1621) for handle_root_event() + /// to add root event IDs for Layer 3 filter creation + /// + /// Both queries use `.since(last_connected)` if available for incremental + /// loading on reconnect. + /// + /// Returns a PendingUpdates containing all repos that need Layer 2/3 filters. + async fn load_existing_events(&self) -> PendingUpdates { + let mut pending = PendingUpdates::new(); + + // Log whether this is a full or incremental load + if let Some(since) = self.last_connected { + tracing::info!( + since = %since, + "Loading events incrementally from database (reconnect)" + ); + } else { + tracing::info!("Loading all events from database (first connection)"); + } + + // First query: Get announcements to populate repo_sync_index + let mut announcement_filter = Filter::new().kind(Kind::GitRepoAnnouncement); + if let Some(timestamp) = self.last_connected { + announcement_filter = announcement_filter.since(timestamp); + } + + let announcements = match self.database.query(announcement_filter).await { + Ok(events) => { + tracing::info!( + count = events.len(), + "Loaded announcements from database" + ); + events + } + Err(e) => { + tracing::error!( + error = %e, + "Failed to query announcements from database" + ); + return pending; + } + }; + + // Process announcements + let mut announcements_loaded = 0; + for event in announcements.iter() { + if let Some(repo_id) = Self::extract_repo_id(event) { + let relays = Self::extract_relay_urls(event); + pending.add_repo(repo_id, relays, HashSet::new()); + announcements_loaded += 1; + } + } + + // Update repo_sync_index with announcements BEFORE querying root events + { + let mut index = self.repo_sync_index.write().await; + for (repo_id, needs) in &pending.repos { + let entry = index + .entry(repo_id.clone()) + .or_insert_with(|| RepoSyncNeeds { + relays: HashSet::new(), + root_events: HashSet::new(), + }); + entry.relays.extend(needs.relays.clone()); + } + } + + // Second query: Get root events for handle_root_event() + let mut root_filter = Filter::new().kinds(vec![ + Kind::GitPatch, + Kind::GitIssue, + Kind::GitPullRequest, + ]); + if let Some(timestamp) = self.last_connected { + root_filter = root_filter.since(timestamp); + } + + let root_events = match self.database.query(root_filter).await { + Ok(events) => { + tracing::info!( + count = events.len(), + "Loaded root events from database" + ); + events + } + Err(e) => { + tracing::error!( + error = %e, + "Failed to query root events from database" + ); + // Continue with just announcements + return pending; + } + }; + + // Process root events + let mut root_events_processed = 0; + for event in root_events.iter() { + self.handle_root_event(event, &mut pending).await; + root_events_processed += 1; + } + + tracing::info!( + announcements_loaded = announcements_loaded, + root_events_processed = root_events_processed, + "Processed existing events from database" + ); + + pending + } + /// Process a relay pool notification /// /// Handles incoming events from the subscription, queueing 30617 announcements @@ -276,33 +404,22 @@ impl SelfSubscriber { // Subscribe to announcement and root event kinds // Per v4 spec: 30617, 1617, 1618, 1621 (NOT 30618) // Plus kind 10317 (User Grasp List) for GRASP discovery - // Check if we have a last_connected time for reconnect filtering - let filter = if let Some(last) = self.last_connected { + let mut filter = Filter::new().kinds(vec![ + Kind::GitRepoAnnouncement, + Kind::GitPatch, + Kind::GitIssue, + Kind::GitPullRequest, + Kind::GitUserGraspList, + ]); + if let Some(timestamp) = self.last_connected { // Quick reconnect - use since filter (15 min buffer) - let since = Timestamp::from(last.as_secs().saturating_sub(15 * 60)); + let since = Timestamp::from(timestamp.as_secs().saturating_sub(15 * 60)); tracing::debug!( since = %since, "Using since filter for reconnect" ); - Filter::new() - .kinds(vec![ - Kind::GitRepoAnnouncement, // Repository Announcements - Kind::GitPatch, // Patches - Kind::GitIssue, // Issues - Kind::GitPullRequest, // Pull Requests - Kind::GitUserGraspList, // User Grasp List - ]) - .since(since) - } else { - // First connection - no since filter - Filter::new().kinds(vec![ - Kind::GitRepoAnnouncement, // Repository Announcements - Kind::GitPatch, // Patches - Kind::GitIssue, // Issues - Kind::GitPullRequest, // Pull Requests - Kind::GitUserGraspList, // User Grasp List - ]) - }; + filter = filter.since(since); + } // Update last_connected AFTER creating filter but BEFORE subscribing self.last_connected = Some(Timestamp::now()); @@ -323,7 +440,11 @@ impl SelfSubscriber { let mut notifications = client.notifications(); let batch_window = Self::get_batch_window(); - let mut pending = PendingUpdates::new(); + + // Load existing events from database on startup + // This ensures all repos get Layer 2/3 filters created, not just those + // returned by the WebSocket subscription (which has limits) + let mut pending = self.load_existing_events().await; // Timer does NOT reset on new events - use interval let mut timer = tokio::time::interval(batch_window); -- cgit v1.2.3 From 6e5b7eb84b3ca8a902ac4bcbab9c2a9f9ecdee51 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 09:16:41 +0000 Subject: fix(sync): Remove .since() filter from database queries in load_existing_events() Root cause: `last_connected` was set to Timestamp::now() BEFORE load_existing_events() was called (line 425), causing the database query to filter out all existing events with .since(current_time). The query became: SELECT * FROM events WHERE created_at >= Result: 0 events returned (nothing has created_at in the future) Solution: Remove .since() filter from database queries entirely. The `last_connected` field is now only used for WebSocket subscription filters to avoid re-fetching events from remote relays on reconnect. Rationale for this approach over reordering operations: - Database queries are fast (indexed by kind and created_at) - Loading all events on startup ensures consistency - Eliminates subtle ordering dependency that could break in refactoring - Cleaner mental model: database = full load, WebSocket = incremental This fixes the issue where ~190 state events weren't being fetched after deploying the database query fix (commit 4162c90). Evidence: Production logs showed "Loaded announcements from database count=0" when there should have been hundreds of announcements. --- src/sync/self_subscriber.rs | 42 ++++++++---------------------------------- 1 file changed, 8 insertions(+), 34 deletions(-) (limited to 'src/sync') diff --git a/src/sync/self_subscriber.rs b/src/sync/self_subscriber.rs index e9505f1..86e4583 100644 --- a/src/sync/self_subscriber.rs +++ b/src/sync/self_subscriber.rs @@ -155,35 +155,18 @@ impl SelfSubscriber { /// 2. Second query: Get root events (1617/1618/1621) for handle_root_event() /// to add root event IDs for Layer 3 filter creation /// - /// Both queries use `.since(last_connected)` if available for incremental - /// loading on reconnect. - /// /// Returns a PendingUpdates containing all repos that need Layer 2/3 filters. async fn load_existing_events(&self) -> PendingUpdates { let mut pending = PendingUpdates::new(); - // Log whether this is a full or incremental load - if let Some(since) = self.last_connected { - tracing::info!( - since = %since, - "Loading events incrementally from database (reconnect)" - ); - } else { - tracing::info!("Loading all events from database (first connection)"); - } + tracing::info!("Loading all events from database"); - // First query: Get announcements to populate repo_sync_index - let mut announcement_filter = Filter::new().kind(Kind::GitRepoAnnouncement); - if let Some(timestamp) = self.last_connected { - announcement_filter = announcement_filter.since(timestamp); - } + // First query: Get all announcements to populate repo_sync_index + let announcement_filter = Filter::new().kind(Kind::GitRepoAnnouncement); let announcements = match self.database.query(announcement_filter).await { Ok(events) => { - tracing::info!( - count = events.len(), - "Loaded announcements from database" - ); + tracing::info!(count = events.len(), "Loaded announcements from database"); events } Err(e) => { @@ -219,22 +202,13 @@ impl SelfSubscriber { } } - // Second query: Get root events for handle_root_event() - let mut root_filter = Filter::new().kinds(vec![ - Kind::GitPatch, - Kind::GitIssue, - Kind::GitPullRequest, - ]); - if let Some(timestamp) = self.last_connected { - root_filter = root_filter.since(timestamp); - } + // Second query: Get all root events for handle_root_event() + let root_filter = + Filter::new().kinds(vec![Kind::GitPatch, Kind::GitIssue, Kind::GitPullRequest]); let root_events = match self.database.query(root_filter).await { Ok(events) => { - tracing::info!( - count = events.len(), - "Loaded root events from database" - ); + tracing::info!(count = events.len(), "Loaded root events from database"); events } Err(e) => { -- cgit v1.2.3 From dd9b00c644853a8db0ec463a7e1eddabd6634e41 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 11:06:09 +0000 Subject: fix: improve logging to enable migration script to detect announcement parse failures --- src/sync/mod.rs | 1 + 1 file changed, 1 insertion(+) (limited to 'src/sync') diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 226e681..a0dfa59 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -2812,6 +2812,7 @@ impl SyncManager { event_id = %event.id, kind = %event.kind.as_u16(), identifier = %identifier, + pubkey = %event.pubkey, "Added rejected announcement to two-tier index" ); } -- cgit v1.2.3 From ddcba2b350615e6d6ad7028b570206efb42f0338 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 11:15:58 +0000 Subject: fix: prevent false positives in naughty list classification Strip URLs (http://, https://, git://, ws://, wss://) from error messages before classification to prevent false positives from repository names, paths, or identifiers containing keywords like 'ssl', 'certificate', etc. - Add strip_urls() function to remove URLs before pattern matching - Add WebSocket protocol support (ws://, wss://) for relay errors - Filter remote warnings that don't indicate infrastructure problems - Use more specific SSL/TLS patterns to avoid npub substring matches - Reduce test suite from 40 to 13 tests, keeping only edge cases Fixes false positives seen in production: - git.shakespeare.diy: 'repository not found' with npub containing 'ssl' - relay.ngit.dev: HTTP 500 error with npub containing 'ssl' - gitnostr.com: remote permission warning misclassified as protocol error --- src/sync/naughty_list.rs | 428 +++++++++++++++++++++++++---------------------- 1 file changed, 232 insertions(+), 196 deletions(-) (limited to 'src/sync') diff --git a/src/sync/naughty_list.rs b/src/sync/naughty_list.rs index 097affe..60ab949 100644 --- a/src/sync/naughty_list.rs +++ b/src/sync/naughty_list.rs @@ -101,6 +101,69 @@ impl NaughtyListTracker { Self::new(12) } + /// Strip URLs from an error message to prevent false positives from URL components. + /// + /// URLs can contain path components, repository names, or user identifiers that + /// accidentally match error patterns (e.g., "my-openssl-project", "ssl-team", + /// "certificate-manager"). By stripping URLs before classification, we ensure + /// only the actual error message text is analyzed. + /// + /// Handles: http://, https://, git://, ws://, wss:// + fn strip_urls(error: &str) -> String { + let mut result = String::with_capacity(error.len()); + let mut chars = error.chars().peekable(); + + while let Some(c) = chars.next() { + // Check for URL start patterns + let potential_url = match c { + 'h' => { + // Check for http:// or https:// + let rest: String = chars.clone().take(7).collect(); + rest.starts_with("ttp://") || rest.starts_with("ttps://") + } + 'g' => { + // Check for git:// + let rest: String = chars.clone().take(5).collect(); + rest.starts_with("it://") + } + 'w' => { + // Check for ws:// or wss:// + let rest: String = chars.clone().take(5).collect(); + rest.starts_with("s://") || rest.starts_with("ss://") + } + _ => false, + }; + + if potential_url { + // Found URL start, consume until URL end + result.push_str("[URL]"); + + // Skip until we hit a URL terminator + loop { + match chars.peek() { + Some(&ch) if Self::is_url_char(ch) => { + chars.next(); + } + _ => break, + } + } + } else { + result.push(c); + } + } + + result + } + + /// Check if a character can be part of a URL + #[inline] + fn is_url_char(c: char) -> bool { + // URLs end at whitespace, quotes, or certain brackets + // This is conservative - real URLs can contain more, but git errors + // typically have URLs followed by these terminators + !matches!(c, ' ' | '\t' | '\n' | '\r' | '"' | '\'' | '>' | ']' | ')') + } + /// Classify an error string into a naughty category or return None for transient errors /// /// # Arguments @@ -112,10 +175,32 @@ impl NaughtyListTracker { /// - `Some(NaughtyCategory)` if the error indicates a persistent infrastructure issue /// - `None` if the error is a transient network issue (use HealthTracker backoff) pub fn classify_error(error: &str) -> Option { - let error_lower = error.to_lowercase(); + // Filter out remote warnings - these are informational messages from the remote + // server that don't indicate infrastructure problems with the domain itself. + // Example: "remote: warning: unable to access '/root/.config/git/attributes': Permission denied" + // These warnings are about the remote server's internal configuration, not connectivity. + let filtered_error: String = error + .lines() + .filter(|line| { + let line_lower = line.to_lowercase(); + // Keep lines that are NOT remote warnings + !(line_lower.starts_with("remote: warning:") + || line_lower.starts_with("warning: remote")) + }) + .collect::>() + .join("\n"); + + // If after filtering we have no content, this was just warnings - not a real error + if filtered_error.trim().is_empty() { + return None; + } + + // Strip URLs to prevent false positives from URL components + // (e.g., repository named "openssl-test" or path containing "certificate") + let url_stripped = Self::strip_urls(&filtered_error); + let error_lower = url_stripped.to_lowercase(); - // DNS lookup failures - use specific patterns to avoid false positives - // from URLs containing "dns" (e.g., npubs like "...cdns7..." or domains) + // DNS lookup failures if error_lower.contains("failed to lookup address") || error_lower.contains("name or service not known") || error_lower.contains("nodename nor servname provided") @@ -129,8 +214,17 @@ impl NaughtyListTracker { // TLS certificate errors if error_lower.contains("certificate") - || error_lower.contains("ssl") - || error_lower.contains("tls") + || error_lower.contains("ssl error") + || error_lower.contains("ssl certificate") + || error_lower.contains("ssl handshake") + || error_lower.contains("ssl_error") + || error_lower.contains("tls error") + || error_lower.contains("tls handshake") + || error_lower.contains("tls alert") + || error_lower.contains("tls_error") + || error_lower.contains("openssl") + || error_lower.contains("schannel") + || error_lower.contains("secure channel") { // Exclude timeout errors that mention TLS if !error_lower.contains("timeout") && !error_lower.contains("timed out") { @@ -294,211 +388,216 @@ impl NaughtyListTracker { mod tests { use super::*; + // ========================================================================= + // URL STRIPPING TESTS + // ========================================================================= + #[test] - fn test_classify_dns_errors() { - assert_eq!( - NaughtyListTracker::classify_error("failed to lookup address information"), - Some(NaughtyCategory::DnsLookupFailed) - ); + fn test_strip_urls_basic_protocols() { + // HTTP/HTTPS assert_eq!( - NaughtyListTracker::classify_error("Name or service not known"), - Some(NaughtyCategory::DnsLookupFailed) - ); - assert_eq!( - NaughtyListTracker::classify_error("nodename nor servname provided"), - Some(NaughtyCategory::DnsLookupFailed) + NaughtyListTracker::strip_urls("error: https://example.com/repo.git failed"), + "error: [URL] failed" ); assert_eq!( - NaughtyListTracker::classify_error("dns error: NXDOMAIN"), - Some(NaughtyCategory::DnsLookupFailed) + NaughtyListTracker::strip_urls("error: http://example.com/path failed"), + "error: [URL] failed" ); - } - #[test] - fn test_classify_tls_errors() { + // Git protocol assert_eq!( - NaughtyListTracker::classify_error("certificate not valid for 'example.com'"), - Some(NaughtyCategory::TlsCertificateInvalid) + NaughtyListTracker::strip_urls("fatal: git://github.com/user/repo.git not found"), + "fatal: [URL] not found" ); + + // WebSocket protocols (used for relay URLs) assert_eq!( - NaughtyListTracker::classify_error("SSL certificate problem"), - Some(NaughtyCategory::TlsCertificateInvalid) + NaughtyListTracker::strip_urls("error: wss://relay.example.com failed"), + "error: [URL] failed" ); assert_eq!( - NaughtyListTracker::classify_error("TLS handshake failed"), - Some(NaughtyCategory::TlsCertificateInvalid) + NaughtyListTracker::strip_urls("error: ws://localhost:8080 failed"), + "error: [URL] failed" ); + } - // TLS timeout should NOT be classified as naughty - assert_eq!( - NaughtyListTracker::classify_error("TLS connection timed out"), - None - ); + #[test] + fn test_strip_urls_multiple() { + let error = "failed to clone https://a.com/repo.git and wss://relay.com"; + let stripped = NaughtyListTracker::strip_urls(error); + assert_eq!(stripped, "failed to clone [URL] and [URL]"); } #[test] - fn test_classify_protocol_errors() { - assert_eq!( - NaughtyListTracker::classify_error("websocket protocol error"), - Some(NaughtyCategory::ProtocolError) - ); + fn test_strip_urls_preserves_error_text() { + let error = + "fatal: unable to access 'https://example.com/repo.git/': SSL certificate problem"; + let stripped = NaughtyListTracker::strip_urls(error); + assert!(stripped.contains("SSL certificate problem")); + assert!(!stripped.contains("example.com")); + } + + // ========================================================================= + // EDGE CASES: TIMEOUT/CONNECTION EXCEPTIONS + // These are the "unusual rules" where a pattern matches but should be excluded + // ========================================================================= + + #[test] + fn test_tls_timeout_not_naughty() { + // TLS errors with timeout should NOT be classified as naughty + // (timeout is transient, not a certificate problem) assert_eq!( - NaughtyListTracker::classify_error("invalid frame header"), - Some(NaughtyCategory::ProtocolError) + NaughtyListTracker::classify_error("TLS connection timed out"), + None ); - - // WebSocket connection errors should NOT be classified as naughty assert_eq!( - NaughtyListTracker::classify_error("websocket connection refused"), + NaughtyListTracker::classify_error("SSL handshake timeout"), None ); } #[test] - fn test_classify_transient_errors() { - // Timeouts are transient + fn test_websocket_connection_errors_not_naughty() { + // WebSocket connection errors are transient, not protocol violations assert_eq!( - NaughtyListTracker::classify_error("connection timed out"), + NaughtyListTracker::classify_error("websocket connection refused"), None ); assert_eq!( - NaughtyListTracker::classify_error("operation timed out"), + NaughtyListTracker::classify_error("websocket connection timeout"), None ); + } - // Connection refused is transient + #[test] + fn test_remote_warnings_filtered() { + // Remote warnings should be filtered out before classification + let warning_only = + "remote: warning: unable to access '/root/.config/git/attributes': Permission denied"; + assert_eq!(NaughtyListTracker::classify_error(warning_only), None); + + // But real errors after warnings should still be classified + let warning_with_error = "remote: warning: something\nfatal: failed to lookup address"; assert_eq!( - NaughtyListTracker::classify_error("connection refused"), - None + NaughtyListTracker::classify_error(warning_with_error), + Some(NaughtyCategory::DnsLookupFailed) ); + } - // Generic network errors are transient - assert_eq!( - NaughtyListTracker::classify_error("network unreachable"), - None - ); + // ========================================================================= + // INTEGRATION: FULL CLASSIFICATION FLOW + // Verify URL stripping + classification work together correctly + // ========================================================================= + + #[test] + fn test_url_with_keywords_not_false_positive() { + // URLs containing keywords should NOT trigger classification + let cases = [ + ("https://example.com/my-openssl-project.git", "not found"), + ("https://example.com/ssl-team/repo.git", "not found"), + ("https://example.com/certificate-manager.git", "not found"), + ("https://example.com/dns-tools.git", "not found"), + ("wss://relay-tls-test.example.com", "connection refused"), + ]; + + for (url, suffix) in cases { + let error = format!("fatal: repository '{}/' {}", url, suffix); + assert_eq!( + NaughtyListTracker::classify_error(&error), + None, + "URL '{}' should not trigger false positive", + url + ); + } + } - // Repository not found is transient (not an infrastructure issue) + #[test] + fn test_real_errors_still_detected() { + // Real errors in the message text (not URL) should still be detected assert_eq!( NaughtyListTracker::classify_error( - "fatal: repository 'https://example.com/repo.git/' not found" + "fatal: 'https://example.com/repo.git': SSL certificate problem" ), - None + Some(NaughtyCategory::TlsCertificateInvalid) ); - } - - #[test] - fn test_classify_false_positive_npub_with_dns() { - // This npub contains "dns" in its encoding: npub17plqkxhsv66g8quxxc9p5t9mxazzn20m426exqnl8lxnh5a4cDNS7jezx0 - // A "not found" error with this npub should NOT be classified as DNS failure - let error = "fatal: repository 'https://git.shakespeare.diy/npub17plqkxhsv66g8quxxc9p5t9mxazzn20m426exqnl8lxnh5a4cdns7jezx0/kuboslopp%20by%20Shakespeare.git/' not found"; assert_eq!( - NaughtyListTracker::classify_error(error), - None, - "npub containing 'dns' should not trigger DNS failure classification" + NaughtyListTracker::classify_error( + "fatal: 'https://example.com/repo.git': failed to lookup address" + ), + Some(NaughtyCategory::DnsLookupFailed) ); - - // Same for relay.ngit.dev - let error2 = "fatal: repository 'https://relay.ngit.dev/npub17plqkxhsv66g8quxxc9p5t9mxazzn20m426exqnl8lxnh5a4cdns7jezx0/kuboslopp%20by%20Shakespeare.git/' not found"; assert_eq!( - NaughtyListTracker::classify_error(error2), - None, - "npub containing 'dns' should not trigger DNS failure classification" + NaughtyListTracker::classify_error("websocket protocol error"), + Some(NaughtyCategory::ProtocolError) ); } #[test] - fn test_record_new_entry() { - let tracker = NaughtyListTracker::with_defaults(); - let url = "wss://bad-relay.example.com"; - - let is_new = tracker.record( - url, - NaughtyCategory::DnsLookupFailed, - "failed to lookup address".to_string(), + fn test_url_with_keyword_and_real_error() { + // URL contains keyword AND there's a real error - should detect the error + let error = "fatal: 'https://example.com/ssl-tools/repo.git': SSL certificate problem"; + assert_eq!( + NaughtyListTracker::classify_error(error), + Some(NaughtyCategory::TlsCertificateInvalid) ); - - assert!(is_new); - assert!(tracker.is_naughty(url)); - - let entry = tracker.get_entry(url).unwrap(); - assert_eq!(entry.category, NaughtyCategory::DnsLookupFailed); - assert_eq!(entry.occurrence_count, 1); } + // ========================================================================= + // TRACKER FUNCTIONALITY + // ========================================================================= + #[test] - fn test_record_updates_existing() { + fn test_tracker_record_and_update() { let tracker = NaughtyListTracker::with_defaults(); let url = "wss://bad-relay.example.com"; // First occurrence - let is_new1 = tracker.record(url, NaughtyCategory::DnsLookupFailed, "error 1".to_string()); - assert!(is_new1); + let is_new = tracker.record(url, NaughtyCategory::DnsLookupFailed, "error 1".to_string()); + assert!(is_new); + assert!(tracker.is_naughty(url)); - // Second occurrence + // Second occurrence updates existing let is_new2 = tracker.record(url, NaughtyCategory::DnsLookupFailed, "error 2".to_string()); assert!(!is_new2); let entry = tracker.get_entry(url).unwrap(); assert_eq!(entry.occurrence_count, 2); - assert_eq!(entry.reason, "error 2"); // Updated to latest + assert_eq!(entry.reason, "error 2"); } #[test] - fn test_is_naughty() { - let tracker = NaughtyListTracker::with_defaults(); - let url = "wss://bad-relay.example.com"; - - assert!(!tracker.is_naughty(url)); + fn test_tracker_expiration() { + let tracker = NaughtyListTracker::new(0); // Expire immediately tracker.record( - url, - NaughtyCategory::TlsCertificateInvalid, - "cert error".to_string(), + "wss://relay.example.com", + NaughtyCategory::DnsLookupFailed, + "error".to_string(), ); - assert!(tracker.is_naughty(url)); - } - - #[test] - fn test_get_all() { - let tracker = NaughtyListTracker::with_defaults(); + // Entry exists but is expired + assert!(!tracker.is_naughty("wss://relay.example.com")); - tracker.record( - "wss://relay1.example.com", - NaughtyCategory::DnsLookupFailed, - "dns error".to_string(), - ); - tracker.record( - "wss://relay2.example.com", - NaughtyCategory::TlsCertificateInvalid, - "tls error".to_string(), - ); + std::thread::sleep(std::time::Duration::from_millis(10)); - let all = tracker.get_all(); - assert_eq!(all.len(), 2); + let expired = tracker.expire_old_entries(); + assert_eq!(expired.len(), 1); + assert_eq!(tracker.total_count(), 0); } #[test] - fn test_count_by_category() { + fn test_tracker_counts() { let tracker = NaughtyListTracker::with_defaults(); + tracker.record("wss://r1.com", NaughtyCategory::DnsLookupFailed, "e".into()); + tracker.record("wss://r2.com", NaughtyCategory::DnsLookupFailed, "e".into()); tracker.record( - "wss://relay1.example.com", - NaughtyCategory::DnsLookupFailed, - "error".to_string(), - ); - tracker.record( - "wss://relay2.example.com", - NaughtyCategory::DnsLookupFailed, - "error".to_string(), - ); - tracker.record( - "wss://relay3.example.com", + "wss://r3.com", NaughtyCategory::TlsCertificateInvalid, - "error".to_string(), + "e".into(), ); + assert_eq!(tracker.total_count(), 3); assert_eq!( tracker.count_by_category(NaughtyCategory::DnsLookupFailed), 2 @@ -507,74 +606,11 @@ mod tests { tracker.count_by_category(NaughtyCategory::TlsCertificateInvalid), 1 ); - assert_eq!(tracker.count_by_category(NaughtyCategory::ProtocolError), 0); - } - - #[test] - fn test_total_count() { - let tracker = NaughtyListTracker::with_defaults(); - assert_eq!(tracker.total_count(), 0); - - tracker.record( - "wss://relay1.example.com", - NaughtyCategory::DnsLookupFailed, - "error".to_string(), - ); - assert_eq!(tracker.total_count(), 1); - - tracker.record( - "wss://relay2.example.com", - NaughtyCategory::TlsCertificateInvalid, - "error".to_string(), - ); - assert_eq!(tracker.total_count(), 2); - } - - #[test] - fn test_expire_old_entries() { - // Use very short expiration for testing - let tracker = NaughtyListTracker::new(0); // Expire immediately (0 hours) - - tracker.record( - "wss://relay1.example.com", - NaughtyCategory::DnsLookupFailed, - "error".to_string(), - ); - - // Entry should exist in the map - assert_eq!(tracker.total_count(), 1); - - // But is_naughty should return false since it's already expired (0 hours) - assert!(!tracker.is_naughty("wss://relay1.example.com")); - - // Sleep to ensure time passes - std::thread::sleep(std::time::Duration::from_millis(10)); - - // Expire old entries (should remove the 0-hour expired entry) - let expired = tracker.expire_old_entries(); - assert_eq!(expired.len(), 1); - assert_eq!(expired[0], "wss://relay1.example.com"); - - // Entry should be gone - assert!(!tracker.is_naughty("wss://relay1.example.com")); - assert_eq!(tracker.total_count(), 0); + assert_eq!(tracker.get_all().len(), 3); } #[test] fn test_category_display() { - assert_eq!( - NaughtyCategory::DnsLookupFailed.to_string(), - "dns_lookup_failed" - ); - assert_eq!( - NaughtyCategory::TlsCertificateInvalid.to_string(), - "tls_certificate_invalid" - ); - assert_eq!(NaughtyCategory::ProtocolError.to_string(), "protocol_error"); - } - - #[test] - fn test_category_as_str() { assert_eq!( NaughtyCategory::DnsLookupFailed.as_str(), "dns_lookup_failed" -- cgit v1.2.3 From a1573c6018c2e81795dc87d36011604dfed80936 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 27 Jan 2026 13:56:45 +0000 Subject: fix: prevent git protocol errors from triggering naughty list Change protocol error detection to only match WebSocket-specific errors (websocket, invalid frame) instead of generic 'protocol' keyword which was incorrectly catching transient git protocol errors. Git protocol errors like 'fatal: protocol error: bad line length' are transient network issues that should use backoff/retry, not permanent naughty list blocking. Only WebSocket/Nostr protocol violations indicate persistent infrastructure problems. Fixes production false positive: - relay.ngit.dev: git protocol error + remote warning misclassified Add production test cases for git protocol errors and warning combinations. --- src/sync/naughty_list.rs | 77 ++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 71 insertions(+), 6 deletions(-) (limited to 'src/sync') diff --git a/src/sync/naughty_list.rs b/src/sync/naughty_list.rs index 60ab949..0abb986 100644 --- a/src/sync/naughty_list.rs +++ b/src/sync/naughty_list.rs @@ -232,12 +232,12 @@ impl NaughtyListTracker { } } - // Protocol errors - if error_lower.contains("websocket") - || error_lower.contains("protocol") - || error_lower.contains("invalid frame") - { - // Exclude connection errors + // Protocol errors - specifically WebSocket/Nostr protocol violations + // Note: We check for "websocket" specifically, NOT generic "protocol" keyword + // because git errors often contain "protocol error" (e.g., "fatal: protocol error: bad line length") + // which are transient network issues, not persistent infrastructure problems. + if error_lower.contains("websocket") || error_lower.contains("invalid frame") { + // Exclude connection errors (transient) if !error_lower.contains("connection") && !error_lower.contains("timeout") && !error_lower.contains("refused") @@ -622,3 +622,68 @@ mod tests { assert_eq!(NaughtyCategory::ProtocolError.as_str(), "protocol_error"); } } + +#[cfg(test)] +mod production_tests { + use super::*; + + /// Production case from relay.ngit.dev - remote warning should not be classified + #[test] + fn test_classify_production_relay_ngit_dev_warning() { + let error = + "remote: warning: unable to access '/root/.config/git/attributes': Permission denied"; + assert_eq!(NaughtyListTracker::classify_error(error), None); + } + + /// Git protocol errors are transient, not persistent infrastructure issues + #[test] + fn test_git_protocol_errors_not_naughty() { + // These are common git protocol errors that should NOT be classified as naughty + let git_protocol_errors = [ + "fatal: protocol error: bad line length character: remo", + "fatal: protocol error: expected old/new/ref, got 'shallow", + "fatal: git upload-pack: protocol error", + "error: protocol error: bad pack header", + "fatal: protocol error: bad band #3", + ]; + + for error in git_protocol_errors { + assert_eq!( + NaughtyListTracker::classify_error(error), + None, + "Git protocol error should not be classified as naughty: {}", + error + ); + } + } + + /// Remote warning followed by git protocol error - both should be filtered/ignored + #[test] + fn test_warning_with_git_protocol_error() { + let error = "remote: warning: unable to access '/root/.config/git/attributes': Permission denied\nfatal: protocol error: bad line length character: remo"; + assert_eq!( + NaughtyListTracker::classify_error(error), + None, + "Warning + git protocol error should not be classified as naughty" + ); + } + + /// WebSocket protocol errors ARE naughty (persistent infrastructure issues) + #[test] + fn test_websocket_errors_still_naughty() { + let websocket_errors = [ + "websocket protocol error", + "websocket handshake failed", + "invalid frame received", + ]; + + for error in websocket_errors { + assert_eq!( + NaughtyListTracker::classify_error(error), + Some(NaughtyCategory::ProtocolError), + "WebSocket error should be classified as protocol_error: {}", + error + ); + } + } +} -- cgit v1.2.3