From 6651d57afbc675c733b37b948306e02436c516d4 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 08:27:16 +0100 Subject: [PATCH 01/11] fix(sync): keep consolidation out of the actor wait path A busy relay could cross the subscription threshold while historic batches were still waiting for EOSE. Consolidation then polled those batches for up to 30 seconds while holding the SyncManager mutex, but the same actor needed that mutex to consume EOSE. Large production relay sets repeated this cycle and starved the five-second purgatory pass, leaving accepted maintainer invitations unprocessed. Queue consolidation until the final batch completion wakes the actor, cancel stale queued work across reset and disconnect paths, and close failed pagination batches so they cannot keep consolidation deferred forever. --- CHANGELOG.md | 3 + docs/explanation/architecture.md | 4 +- docs/explanation/grasp-02-proactive-sync.md | 5 +- src/sync/mod.rs | 330 +++++++++++++++----- 4 files changed, 270 insertions(+), 72 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 298c989..2b17828 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Fix invitation acceptance sync by deferring subscription consolidation until + in-flight relay batches finish, keeping the sync actor available to process + EOSE messages and the five-second purgatory reconciliation pass. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index 386381e..5e0406e 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -515,7 +515,9 @@ The ngit-grasp relay implements **Proactive Sync of Nostr Events**, which synchr - **Smart reconnection** - uses `since` filter for quick reconnects (<15 min), fresh sync otherwise - **Health tracking** with exponential backoff for failing relays - **Daily sync** with random 23-25h timer to detect state drift -- **Filter consolidation** when count exceeds 70 to prevent subscription explosion +- **Filter consolidation** when count exceeds 70 to prevent subscription + explosion; rebuilds are deferred until in-flight batches drain so EOSE and + purgatory work remain responsive - **Rejected events index** - prevents wasteful broad re-fetching while retaining exact IDs for dependency recovery **Architecture:** diff --git a/docs/explanation/grasp-02-proactive-sync.md b/docs/explanation/grasp-02-proactive-sync.md index ef74375..0775a2d 100644 --- a/docs/explanation/grasp-02-proactive-sync.md +++ b/docs/explanation/grasp-02-proactive-sync.md @@ -691,7 +691,10 @@ fn compute_actions( - **`fresh_start()`**: Full sync - clears all state, L1 historic (with negentropy if available), then L2+L3 via recompute - **`quick_reconnect()`**: Incremental sync - preserves confirmed state, L1 historic with `since`, L2+L3 rebuild with `since`, then recompute for new items - **`daily_sync()`**: Wrapper around `fresh_start()` without disconnect metrics -- **`consolidate()`**: Reduces filter count - clears pending, unsubscribes all, rebuilds live subscriptions only, then recompute for new items +- **`consolidate()`**: Reduces filter count after in-flight historic batches have + drained. If a relay crosses the threshold while batches are pending, + consolidation is queued and batch completion wakes the sync actor to rebuild + subscriptions. The actor never polls for EOSE while holding its own lock. ### Sync Primitives diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 058cb29..4d534a3 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -316,6 +316,30 @@ pub struct PendingItems { pub root_events: HashSet, } +fn take_drained_batch_as_failed( + pending: &mut HashMap>, + relay_url: &str, + batch_id: u64, +) -> Option { + let (batch_index, remove_relay) = { + let batches = pending.get(relay_url)?; + let batch_index = batches + .iter() + .position(|batch| batch.batch_id == batch_id)?; + if !batches[batch_index].outstanding_subs.is_empty() { + return None; + } + (batch_index, batches.len() == 1) + }; + + let mut batch = pending.get_mut(relay_url)?.remove(batch_index); + batch.failed = true; + if remove_relay { + pending.remove(relay_url); + } + Some(batch) +} + // ============================================================================= // SyncManager - Main Entry Point // ============================================================================= @@ -349,13 +373,55 @@ const QUICK_RECONNECT_WINDOW_SECS: u64 = 15 * 60; /// Maximum filter count before triggering consolidation const CONSOLIDATION_THRESHOLD: usize = 70; -/// Maximum time to wait for pending batches (30 seconds) -const CONSOLIDATION_WAIT_TIMEOUT_SECS: u64 = 30; - /// Page size threshold for historic sync pagination (non-negentropy) /// If a subscription receives >= 75 events, we fetch the next page const PAGINATION_THRESHOLD: usize = 75; +#[derive(Debug, Default)] +struct DeferredConsolidations { + relays: HashSet, +} + +impl DeferredConsolidations { + fn request(&mut self, relay_url: &str, has_pending_batches: bool) -> bool { + if has_pending_batches { + self.relays.insert(relay_url.to_string()); + false + } else { + self.relays.remove(relay_url); + true + } + } + + fn take_ready(&mut self, relay_url: &str, has_pending_batches: bool) -> bool { + !has_pending_batches && self.relays.remove(relay_url) + } + + fn contains(&self, relay_url: &str) -> bool { + self.relays.contains(relay_url) + } + + fn cancel(&mut self, relay_url: &str) -> bool { + self.relays.remove(relay_url) + } + + fn clear(&mut self) { + self.relays.clear(); + } +} + +fn notify_deferred_consolidation_after_batch_completion( + deferred: &DeferredConsolidations, + sender: Option<&tokio::sync::mpsc::UnboundedSender>, + relay_url: &str, +) { + if deferred.contains(relay_url) { + if let Some(sender) = sender { + let _ = sender.send(relay_url.to_string()); + } + } +} + // ============================================================================= // Daily Timer // ============================================================================= @@ -634,12 +700,16 @@ pub struct SyncManager { health_tracker: Arc, /// Counter for generating unique batch IDs next_batch_id: u64, + /// Relays whose subscription consolidation waits for in-flight batches to drain. + deferred_consolidations: DeferredConsolidations, /// Channel for disconnect notifications (set during run) disconnect_tx: Option>, /// Channel for EOSE notifications (set during run) eose_tx: Option>, /// Channel for connect notifications (set during run) connect_tx: Option>, + /// Wakes the actor when a batch completion may unblock consolidation. + deferred_consolidation_tx: Option>, /// Channel for broadcasting shutdown signal to all background tasks shutdown_tx: Option>, /// Prometheus metrics for sync operations (None if metrics disabled) @@ -718,9 +788,11 @@ impl SyncManager { dependency_refetch_attempts: Arc::new(std::sync::Mutex::new(HashMap::new())), health_tracker: Arc::new(RelayHealthTracker::new(config)), next_batch_id: 0, + deferred_consolidations: DeferredConsolidations::default(), disconnect_tx: None, eose_tx: None, connect_tx: None, + deferred_consolidation_tx: None, shutdown_tx: None, metrics: sync_metrics, } @@ -908,6 +980,7 @@ impl SyncManager { } // Subscribe to next page and add to outstanding_subs + let mut next_page_started = false; if let Some(conn) = self.connections.get(&relay_url_for_pagination) { match conn.subscribe_filter(next_filter.clone(), true).await { Ok(new_sub_id) => { @@ -918,6 +991,7 @@ impl SyncManager { batches.iter_mut().find(|b| b.batch_id == batch_id) { batch.outstanding_subs.insert(new_sub_id.clone()); + next_page_started = true; // Initialize pagination state for new subscription batch.pagination_state.insert( new_sub_id.clone(), @@ -948,6 +1022,25 @@ impl SyncManager { } } + if !next_page_started { + let completed_batch = { + let mut pending = self.pending_sync_index.write().await; + take_drained_batch_as_failed( + &mut pending, + &relay_url_for_pagination, + batch_id, + ) + }; + if let Some(batch) = completed_batch { + tracing::warn!( + relay = %relay_url_for_pagination, + batch_id, + "Pagination could not continue; completing drained batch as failed" + ); + self.confirm_batch(&relay_url_for_pagination, batch).await; + } + } + // Early return since we've released and re-acquired locks return; } @@ -1335,6 +1428,12 @@ impl SyncManager { // Release lock before checking if historic sync is complete drop(relay_index); + notify_deferred_consolidation_after_batch_completion( + &self.deferred_consolidations, + self.deferred_consolidation_tx.as_ref(), + relay_url, + ); + // Spawn background task to check if historic sync is complete // This avoids blocking the confirm_batch flow for 6 seconds let relay_url = relay_url.to_string(); @@ -1452,6 +1551,7 @@ impl SyncManager { /// This is triggered by the daily timer to detect state drift over time. async fn daily_sync(&mut self, relay_url: &str) { tracing::info!(relay = %relay_url, "Starting daily sync"); + self.cancel_deferred_consolidation(relay_url, "daily sync reset"); // Get connection let connection = match self.connections.get(relay_url) { @@ -1529,6 +1629,11 @@ impl SyncManager { // 4. Create connect channel for spawned tasks -> manager communication let (connect_tx, mut connect_rx) = mpsc::channel::(100); + // Batch completion can be synchronous, so use a non-blocking wakeup + // instead of waiting for pending work while holding the actor lock. + let (deferred_consolidation_tx, mut deferred_consolidation_rx) = + mpsc::unbounded_channel::(); + // 4b. Create shutdown broadcast channel for graceful shutdown let (shutdown_tx, _shutdown_rx) = broadcast::channel(1); @@ -1547,6 +1652,7 @@ impl SyncManager { self.disconnect_tx = Some(disconnect_tx.clone()); self.eose_tx = Some(eose_tx.clone()); self.connect_tx = Some(connect_tx.clone()); + self.deferred_consolidation_tx = Some(deferred_consolidation_tx); self.shutdown_tx = Some(shutdown_tx.clone()); // 6. Connect to bootstrap relay if configured @@ -1644,6 +1750,12 @@ impl SyncManager { } } } + relay_url = deferred_consolidation_rx.recv() => { + if let Some(relay_url) = relay_url { + let mut manager = sync_manager.lock().await; + manager.process_deferred_consolidation(&relay_url).await; + } + } } } } @@ -2036,6 +2148,7 @@ impl SyncManager { let _now = Timestamp::now(); tracing::info!(relay = %relay_url, "Starting fresh_start"); + self.cancel_deferred_consolidation(relay_url, "fresh start"); // Step 1: Clear PendingSyncIndex for this relay { @@ -2099,6 +2212,8 @@ impl SyncManager { /// Basic connection state and metrics are managed by handle_connect_or_reconnect. /// This method handles reconnect-specific concerns (health tracking, reconnect metrics). async fn quick_reconnect(&mut self, relay_url: &str, since: Timestamp) { + self.cancel_deferred_consolidation(relay_url, "quick reconnect"); + // Step 1: Clear PendingSyncIndex for this relay // Old subscriptions are dead after disconnect { @@ -2837,6 +2952,7 @@ impl SyncManager { .map(|s| s.connection_status == ConnectionStatus::Disconnecting) .unwrap_or(false) }; + self.cancel_deferred_consolidation(relay_url, "relay disconnect"); if was_intentional { // Intentional disconnect - complete cleanup by removing state @@ -3387,53 +3503,12 @@ impl SyncManager { // Consolidation System // ========================================================================= - /// Wait until all pending batches for a relay are complete - /// - /// Polls the pending_sync_index until the relay has no pending batches. - /// Returns error if timeout (30 seconds) is exceeded. - async fn wait_pending_complete(&self, relay_url: &str) -> Result<(), String> { - use std::time::Duration; - use tokio::time::{sleep, Instant}; - - let start = Instant::now(); - let timeout = Duration::from_secs(CONSOLIDATION_WAIT_TIMEOUT_SECS); - - tracing::debug!( - relay = %relay_url, - timeout_secs = CONSOLIDATION_WAIT_TIMEOUT_SECS, - "Waiting for pending batches to complete" - ); - - loop { - // Check if no pending batches - { - let pending = self.pending_sync_index.read().await; - if !pending.contains_key(relay_url) { - tracing::debug!( - relay = %relay_url, - elapsed_ms = start.elapsed().as_millis(), - "All pending batches complete" - ); - return Ok(()); - } - } - - // Check timeout - if start.elapsed() > timeout { - tracing::warn!( - relay = %relay_url, - timeout_secs = CONSOLIDATION_WAIT_TIMEOUT_SECS, - "Timeout waiting for pending batches" - ); - return Err(format!( - "Timeout waiting for pending batches on {} after {}s", - relay_url, CONSOLIDATION_WAIT_TIMEOUT_SECS - )); - } - - // Short poll interval - sleep(Duration::from_millis(100)).await; - } + async fn has_pending_batches(&self, relay_url: &str) -> bool { + self.pending_sync_index + .read() + .await + .get(relay_url) + .is_some_and(|batches| !batches.is_empty()) } /// Check if consolidation is needed and trigger if threshold exceeded @@ -3448,6 +3523,21 @@ impl SyncManager { }; if current_count + new_count > CONSOLIDATION_THRESHOLD { + let has_pending_batches = self.has_pending_batches(relay_url).await; + if !self + .deferred_consolidations + .request(relay_url, has_pending_batches) + { + tracing::info!( + relay = %relay_url, + current_count, + new_count, + threshold = CONSOLIDATION_THRESHOLD, + "Filter count exceeds threshold; deferring consolidation until pending batches drain" + ); + return; + } + tracing::info!( relay = %relay_url, current_count = current_count, @@ -3456,34 +3546,49 @@ impl SyncManager { "Filter count exceeds threshold, consolidating" ); - if let Err(e) = self.consolidate(relay_url).await { - tracing::error!( - relay = %relay_url, - error = %e, - "Consolidation failed" - ); - } + self.consolidate(relay_url).await; + } + } + + async fn process_deferred_consolidation(&mut self, relay_url: &str) { + let has_pending_batches = self.has_pending_batches(relay_url).await; + if !self + .deferred_consolidations + .take_ready(relay_url, has_pending_batches) + { + return; + } + + tracing::info!( + relay = %relay_url, + "Pending batches drained; running deferred consolidation" + ); + self.consolidate(relay_url).await; + } + + fn cancel_deferred_consolidation(&mut self, relay_url: &str, reason: &'static str) { + if self.deferred_consolidations.cancel(relay_url) { + tracing::debug!( + relay = %relay_url, + reason, + "Cancelled deferred consolidation" + ); } } /// Consolidate all subscriptions for a relay /// - /// This method: - /// 1. Waits for all pending batches to complete - /// 2. Unsubscribes from all active subscriptions - /// 3. Rebuilds Layer 2 and Layer 3 with since filter + /// The caller must ensure pending batches have drained so EOSE processing + /// never waits behind the sync actor lock. /// /// Layer 1 (announcements) remains active and is NOT unsubscribed. - async fn consolidate(&mut self, relay_url: &str) -> Result<(), String> { + async fn consolidate(&mut self, relay_url: &str) { tracing::info!( relay = %relay_url, "Starting consolidation" ); - // Step 1: Wait for all pending batches to complete - self.wait_pending_complete(relay_url).await?; - - // Step 2: Get connection and unsubscribe all + // Get connection and unsubscribe all let connection = match self.connections.get(relay_url) { Some(conn) => conn, None => { @@ -3491,13 +3596,13 @@ impl SyncManager { relay = %relay_url, "No connection found, skipping consolidation" ); - return Ok(()); // No connection, nothing to consolidate + return; } }; connection.unsubscribe_all().await; - // Step 3: Rebuild all subscriptions with since filter + // Rebuild all subscriptions with since filter let now = Timestamp::now(); let since = Timestamp::from(now.as_secs().saturating_sub(QUICK_RECONNECT_WINDOW_SECS)); @@ -3511,8 +3616,6 @@ impl SyncManager { since = %since, "Consolidation complete - filter count reset" ); - - Ok(()) } /// Check for relays that should be disconnected @@ -4198,6 +4301,7 @@ impl SyncManager { pending.clear(); tracing::debug!(count = count, "Cleared pending_sync_index"); } + self.deferred_consolidations.clear(); tracing::info!("SyncManager shutdown complete"); } @@ -4207,6 +4311,92 @@ impl SyncManager { mod tests { use super::*; + #[test] + fn deferred_consolidation_runs_only_after_final_batch_completion() { + let relay_url = "wss://relay.example"; + let mut deferred = DeferredConsolidations::default(); + let (sender, mut receiver) = tokio::sync::mpsc::unbounded_channel(); + + assert!( + !deferred.request(relay_url, true), + "an in-flight batch must defer instead of waiting in the sync actor" + ); + notify_deferred_consolidation_after_batch_completion(&deferred, Some(&sender), relay_url); + assert_eq!( + receiver.try_recv().unwrap(), + relay_url, + "batch completion must wake the actor without awaiting under its lock" + ); + assert!( + !deferred.take_ready(relay_url, true), + "consolidation must remain queued while another batch is pending" + ); + + notify_deferred_consolidation_after_batch_completion(&deferred, Some(&sender), relay_url); + assert_eq!(receiver.try_recv().unwrap(), relay_url); + assert!( + deferred.take_ready(relay_url, false), + "the final batch completion must make consolidation runnable" + ); + assert!( + !deferred.take_ready(relay_url, false), + "taking deferred work must be idempotent" + ); + } + + #[test] + fn reset_or_disconnect_cancels_deferred_consolidation_and_stale_wakeup() { + for reason in ["reset", "disconnect"] { + let relay_url = format!("wss://{reason}.example"); + let mut deferred = DeferredConsolidations::default(); + let (sender, mut receiver) = tokio::sync::mpsc::unbounded_channel(); + + assert!(!deferred.request(&relay_url, true)); + notify_deferred_consolidation_after_batch_completion( + &deferred, + Some(&sender), + &relay_url, + ); + assert!(deferred.cancel(&relay_url)); + assert_eq!(receiver.try_recv().unwrap(), relay_url); + assert!( + !deferred.take_ready(&relay_url, false), + "a queued wakeup must not resurrect consolidation after {reason}" + ); + } + } + + #[test] + fn failed_pagination_completes_only_a_fully_drained_batch() { + let relay_url = "wss://pagination.example"; + let still_pending = SubscriptionId::new("still-pending"); + let make_batch = |batch_id, outstanding_subs| PendingBatch { + batch_id, + items: PendingItems::default(), + outstanding_subs, + sync_method: SyncMethod::ReqEose, + pagination_state: HashMap::new(), + requested_event_ids: None, + received_event_ids: None, + retry_count: 0, + failed: false, + }; + let mut pending = HashMap::from([( + relay_url.to_string(), + vec![ + make_batch(41, HashSet::new()), + make_batch(42, HashSet::from([still_pending])), + ], + )]); + + let completed = take_drained_batch_as_failed(&mut pending, relay_url, 41) + .expect("failed pagination must complete a drained batch"); + assert!(completed.failed); + assert!(take_drained_batch_as_failed(&mut pending, relay_url, 42).is_none()); + assert_eq!(pending[relay_url].len(), 1); + assert!(!pending[relay_url][0].failed); + } + #[test] fn relay_disconnect_waits_for_pending_and_historic_sync_work() { let mut source = RelayState { From 8ac283580e0ce71c48fec55571a6a75f67fae934 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 08:32:37 +0100 Subject: [PATCH 02/11] fix(sync): retain unconfirmed invitation sources An invitation source can return an empty initial historic batch or be temporarily unavailable before its state is confirmed. The cleanup path looked only at confirmed relay state, disconnected that source even though RepoSyncIndex still required it, and then skipped it during retries because its confirmed sets were empty. Root-slash URL variants could also split one source across separate lifecycle keys and amplify the work. Keep desired GRASP-02 sources registered and retryable until demand disappears, and canonicalize root relay URL variants across desired, pending, connection, and dependency-refetch lookup state. --- CHANGELOG.md | 4 + docs/explanation/architecture.md | 2 + docs/explanation/grasp-02-proactive-sync.md | 10 +- src/sync/algorithms.rs | 37 +++- src/sync/mod.rs | 177 +++++++++++++++++--- 5 files changed, 204 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b17828..b2ac54b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix invitation acceptance sync by deferring subscription consolidation until in-flight relay batches finish, keeping the sync actor available to process EOSE messages and the five-second purgatory reconciliation pass. +- Fix invitation acceptance sync when a listed source relay is initially + unavailable or empty by retaining and retrying desired GRASP-02 work until it + is confirmed. Root-slash URL variants now share one relay lifecycle instead + of multiplying connections and subscription batches. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index 5e0406e..f9f818b 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -519,6 +519,8 @@ The ngit-grasp relay implements **Proactive Sync of Nostr Events**, which synchr explosion; rebuilds are deferred until in-flight batches drain so EOSE and purgatory work remain responsive - **Rejected events index** - prevents wasteful broad re-fetching while retaining exact IDs for dependency recovery +- **Desired-source retention** keeps listed GRASP-02 relays retryable until + repository work is actually confirmed, including StateOnly invitation sync **Architecture:** diff --git a/docs/explanation/grasp-02-proactive-sync.md b/docs/explanation/grasp-02-proactive-sync.md index 0775a2d..677dfa6 100644 --- a/docs/explanation/grasp-02-proactive-sync.md +++ b/docs/explanation/grasp-02-proactive-sync.md @@ -334,8 +334,11 @@ The sync system uses three background tasks that run continuously: **Actions**: -1. **Disconnect checking**: Calls `check_disconnects()` to remove relays with no repos/events (except bootstrap) -2. **Retry disconnected**: Calls `retry_disconnected_relays()` to attempt reconnection per health tracker backoff +1. **Disconnect checking**: Calls `check_disconnects()` to remove relays with + neither confirmed nor desired repository work (except bootstrap) +2. **Retry disconnected**: Calls `retry_disconnected_relays()` to attempt + reconnection per health tracker backoff while either confirmed or desired + `RepoSyncIndex` work remains 3. **Rate limit recovery**: Calls `check_rate_limit_recovery()` to clear expired rate limits 4. **Metrics update**: Updates Prometheus metrics with current health states @@ -973,6 +976,9 @@ RateLimited -> previous state: After 65-second cooldown expires ### Special Behaviors - **Bootstrap relays**: Never disconnected by cleanup system, even if empty +- **Desired GRASP-02 sources**: Remain registered and retryable before their + first successful historic batch; an initially empty or unavailable source + cannot make a purgatory invitation permanently lose its sync path - **Rate limiting**: Distinct from connection failures - triggered by relay NOTICE messages - **Connection timeout**: Set to `base_backoff_secs` to ensure retry timing works correctly diff --git a/src/sync/algorithms.rs b/src/sync/algorithms.rs index 9899abc..d2fd49b 100644 --- a/src/sync/algorithms.rs +++ b/src/sync/algorithms.rs @@ -67,7 +67,9 @@ pub fn derive_relay_targets( for (repo_id, needs) in repo_index { for relay_url in &needs.relays { - let entry = relay_targets.entry(relay_url.clone()).or_default(); + let relay_key = + super::canonical_relay_key(relay_url).unwrap_or_else(|_| relay_url.clone()); + let entry = relay_targets.entry(relay_key).or_default(); match needs.sync_level { super::SyncLevel::Full => { @@ -269,6 +271,39 @@ mod tests { assert_eq!(relay_needs.repos.len(), 3); } + #[test] + fn test_derive_relay_targets_deduplicates_root_slash_variants() { + let repo_index = HashMap::from([ + ( + "repo1".to_string(), + ModRepoSyncNeeds { + relays: HashSet::from(["wss://relay1.com".to_string()]), + root_events: HashSet::new(), + sync_level: Default::default(), + }, + ), + ( + "repo2".to_string(), + ModRepoSyncNeeds { + relays: HashSet::from(["wss://relay1.com/".to_string()]), + root_events: HashSet::new(), + sync_level: Default::default(), + }, + ), + ]); + + let targets = derive_relay_targets(&repo_index); + + assert_eq!(targets.len(), 1); + assert_eq!( + targets + .get("wss://relay1.com") + .expect("root slash variants must share one key") + .repos, + HashSet::from(["repo1".to_string(), "repo2".to_string()]) + ); + } + #[test] fn test_derive_relay_targets_repo_across_multiple_relays() { let mut repo_index = HashMap::new(); diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 4d534a3..1cd20be 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -56,6 +56,48 @@ use crate::nostr::builder::Nip34WritePolicy; use crate::nostr::SharedDatabase; use nostr_relay_builder::prelude::LocalRelay; +/// Return one stable identity for a relay URL throughout all sync indexes. +/// +/// URL parsers represent a root path as `/`, so announcements that alternate +/// between `wss://relay.example` and `wss://relay.example/` must not create +/// separate connections and duplicate subscription work. Non-root paths remain +/// distinct relay endpoints. +pub(crate) fn canonical_relay_key(relay_url: &str) -> Result { + let normalized = if relay_url.starts_with("wss://") || relay_url.starts_with("ws://") { + relay_url.to_string() + } else { + format!("wss://{relay_url}") + }; + let relay = RelayUrl::parse(&normalized).map_err(|error| error.to_string())?; + let mut canonical = relay.to_string(); + let after_scheme = canonical + .split_once("://") + .map(|(_, value)| value) + .unwrap_or(canonical.as_str()); + if after_scheme.ends_with('/') && after_scheme.matches('/').count() == 1 { + canonical.pop(); + } + Ok(canonical) +} + +fn connections_for_relay_urls( + connections: &HashMap, + relay_urls: &[String], +) -> Vec<(String, T)> { + let mut seen = HashSet::new(); + relay_urls + .iter() + .filter_map(|relay_url| canonical_relay_key(relay_url).ok()) + .filter(|relay_url| seen.insert(relay_url.clone())) + .filter_map(|relay_url| { + connections + .get(&relay_url) + .cloned() + .map(|connection| (relay_url, connection)) + }) + .collect() +} + // ============================================================================= // Type Aliases for Index Structures // ============================================================================= @@ -197,12 +239,13 @@ impl RelayState { /// completion is deliberately delayed to cover the subscriber batch /// window). Pending batches and an active incomplete historic sync must /// therefore keep the relay connected. - fn is_disconnect_candidate(&self, has_pending_batches: bool) -> bool { + fn is_disconnect_candidate(&self, has_pending_batches: bool, has_desired_work: bool) -> bool { if self.is_bootstrap || self.connection_status == ConnectionStatus::Disconnecting || !self.repos.is_empty() || !self.root_events.is_empty() || has_pending_batches + || has_desired_work { return false; } @@ -1657,8 +1700,19 @@ impl SyncManager { // 6. Connect to bootstrap relay if configured if let Some(ref bootstrap_url) = self.bootstrap_relay_url.clone() { - self.register_relay(bootstrap_url.clone(), true).await; - self.try_connect_relay(bootstrap_url).await; + match canonical_relay_key(bootstrap_url) { + Ok(relay_url) => { + self.register_relay(relay_url.clone(), true).await; + self.try_connect_relay(&relay_url).await; + } + Err(error) => { + tracing::warn!( + relay = %bootstrap_url, + error = %error, + "Rejecting invalid bootstrap relay target" + ); + } + } } // 7. Wrap self in Arc for sharing with timer task @@ -1766,7 +1820,19 @@ impl SyncManager { /// - For new relays: creates entry with Connecting status, spawns connection /// - For existing connected relays: subscribes to filters, creates PendingBatch /// - For disconnected/connecting relays: returns (will be handled on connection) - async fn handle_new_sync_filters(&mut self, action: AddFilters) { + async fn handle_new_sync_filters(&mut self, mut action: AddFilters) { + action.relay_url = match canonical_relay_key(&action.relay_url) { + Ok(relay_url) => relay_url, + Err(error) => { + tracing::warn!( + relay = %action.relay_url, + error = %error, + "Rejecting invalid sync relay target" + ); + return; + } + }; + // Step 1: Check if relay exists in relay_sync_index let connection_status = { let index = self.relay_sync_index.read().await; @@ -2308,6 +2374,18 @@ impl SyncManager { /// Does NOT connect - connection happens via try_connect_relay or retry_disconnected_relays. /// The RelayConnection persists forever and is reused on reconnects. async fn register_relay(&mut self, relay_url: String, is_bootstrap: bool) { + let relay_url = match canonical_relay_key(&relay_url) { + Ok(relay_url) => relay_url, + Err(error) => { + tracing::warn!( + relay = %relay_url, + error = %error, + "Rejecting invalid relay registration" + ); + return; + } + }; + // Create RelayConnection if not exists if !self.connections.contains_key(&relay_url) { // Get relay owner keys for NIP-42 authentication @@ -2773,15 +2851,7 @@ impl SyncManager { event_ids: HashSet, identifier: &str, ) { - let connections: Vec<(String, RelayConnection)> = relay_urls - .iter() - .filter_map(|relay_url| { - self.connections - .get(relay_url) - .cloned() - .map(|connection| (relay_url.clone(), connection)) - }) - .collect(); + let connections = connections_for_relay_urls(&self.connections, relay_urls); if connections.is_empty() { tracing::debug!( @@ -3626,6 +3696,13 @@ impl SyncManager { /// /// Bootstrap relays are NEVER disconnected, even if empty. async fn check_disconnects(&mut self) { + let desired_relays: HashSet = { + let repo_index = self.repo_sync_index.read().await; + algorithms::derive_relay_targets(&repo_index) + .into_keys() + .collect() + }; + // Collect relays to disconnect let to_disconnect: Vec = { let pending = self.pending_sync_index.read().await; @@ -3637,7 +3714,10 @@ impl SyncManager { .get(relay_url) .is_some_and(|batches| !batches.is_empty()); - if state.is_disconnect_candidate(has_pending_batches) { + if state.is_disconnect_candidate( + has_pending_batches, + desired_relays.contains(relay_url), + ) { Some(relay_url.clone()) } else { None @@ -3714,6 +3794,13 @@ impl SyncManager { /// /// For each eligible relay, a reconnection is attempted via try_connect_relay. async fn retry_disconnected_relays(&mut self) { + let desired_relays: HashSet = { + let repo_index = self.repo_sync_index.read().await; + algorithms::derive_relay_targets(&repo_index) + .into_keys() + .collect() + }; + // Collect relays to reconnect let to_reconnect: Vec = { let index = self.relay_sync_index.read().await; @@ -3725,8 +3812,12 @@ impl SyncManager { return None; } - // Skip empty relays - they'll be cleaned up by check_disconnects - if state.repos.is_empty() && state.root_events.is_empty() { + // A source can have desired StateOnly invitation work before + // its first successful historic batch confirms anything. + if state.repos.is_empty() + && state.root_events.is_empty() + && !desired_relays.contains(relay_url) + { return None; } @@ -4311,6 +4402,41 @@ impl SyncManager { mod tests { use super::*; + #[test] + fn canonical_relay_keys_dedupe_only_the_root_slash() { + assert_eq!( + canonical_relay_key("wss://relay.example").unwrap(), + "wss://relay.example" + ); + assert_eq!( + canonical_relay_key("wss://relay.example/").unwrap(), + "wss://relay.example" + ); + assert_eq!( + canonical_relay_key("wss://relay.example/nostr/").unwrap(), + "wss://relay.example/nostr/" + ); + assert_eq!( + canonical_relay_key("relay.example/").unwrap(), + "wss://relay.example" + ); + } + + #[test] + fn connection_lookup_deduplicates_relay_url_variants() { + let canonical = canonical_relay_key("wss://relay.example").unwrap(); + let connections = HashMap::from([(canonical.clone(), 7_u8)]); + let candidates = vec![ + "wss://relay.example".to_string(), + "wss://relay.example/".to_string(), + ]; + + assert_eq!( + connections_for_relay_urls(&connections, &candidates), + vec![(canonical, 7)] + ); + } + #[test] fn deferred_consolidation_runs_only_after_final_batch_completion() { let relay_url = "wss://relay.example"; @@ -4405,35 +4531,39 @@ mod tests { }; assert!( - !source.is_disconnect_candidate(true), + !source.is_disconnect_candidate(true, false), "a source with missing-ID subscriptions in flight must stay connected" ); assert!( - !source.is_disconnect_candidate(false), + !source.is_disconnect_candidate(false, false), "the historic-sync batch window must stay connected even between batches" ); source.connection_status = ConnectionStatus::Connected; assert!( - !source.is_disconnect_candidate(false), + !source.is_disconnect_candidate(false, false), "an active relay cannot be disconnected before historic sync settles" ); source.historic_sync_completed = true; assert!( - source.is_disconnect_candidate(false), + source.is_disconnect_candidate(false, false), "an empty relay can be released after historic sync settles" ); assert!( - !source.is_disconnect_candidate(true), + !source.is_disconnect_candidate(true, false), "a later pending batch still keeps an otherwise empty relay connected" ); + assert!( + !source.is_disconnect_candidate(false, true), + "unconfirmed desired invitation work keeps its source connected" + ); source .repos .insert("30617:maintainer:repository".to_string()); assert!( - !source.is_disconnect_candidate(false), + !source.is_disconnect_candidate(false, false), "confirmed repository work keeps the source connected" ); } @@ -4442,7 +4572,8 @@ mod tests { fn disconnected_empty_relay_can_still_be_cleaned_up() { let source = RelayState::default(); - assert!(source.is_disconnect_candidate(false)); + assert!(source.is_disconnect_candidate(false, false)); + assert!(!source.is_disconnect_candidate(false, true)); } #[tokio::test] From cc2102ab88f56bde291207e1d619841de90e5175 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 08:43:06 +0100 Subject: [PATCH 03/11] fix(sync): keep relay handshakes outside the actor Invitation acceptance can list several GRASP relays, including slow or unreachable ones. The sync manager previously awaited each DNS and websocket handshake while holding its actor lock, so one timeout delayed EOSE handling and the five-second purgatory reconciliation pass for every repository. The SDK could also report an attempt as successful while the relay was still Connecting, causing subscriptions to start before the socket was usable. Run at most eight connection attempts in bounded workers, return their tokenized results to the actor for serialized state changes, and wait for an actual Connected relay status within the same deadline before starting sync subscriptions. --- CHANGELOG.md | 3 + docs/explanation/architecture.md | 2 + docs/explanation/grasp-02-proactive-sync.md | 7 +- src/sync/mod.rs | 417 +++++++++++++++----- src/sync/relay_connection.rs | 132 +++++-- 5 files changed, 440 insertions(+), 121 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2ac54b..c980cd6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 unavailable or empty by retaining and retrying desired GRASP-02 work until it is confirmed. Root-slash URL variants now share one relay lifecycle instead of multiplying connections and subscription batches. +- Fix invitation acceptance sync across large relay lists by moving websocket + handshakes out of the sync actor, limiting them to eight concurrent attempts, + and waiting for each relay to be connected before starting subscriptions. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index f9f818b..40c2973 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -514,6 +514,8 @@ The ngit-grasp relay implements **Proactive Sync of Nostr Events**, which synchr - **Three-way diff** (`compute_actions`) determines new subscriptions needed - **Smart reconnection** - uses `since` filter for quick reconnects (<15 min), fresh sync otherwise - **Health tracking** with exponential backoff for failing relays +- **Bounded connection workers** keep slow DNS and websocket handshakes out of + the sync actor while limiting network pressure to eight concurrent attempts - **Daily sync** with random 23-25h timer to detect state drift - **Filter consolidation** when count exceeds 70 to prevent subscription explosion; rebuilds are deferred until in-flight batches drain so EOSE and diff --git a/docs/explanation/grasp-02-proactive-sync.md b/docs/explanation/grasp-02-proactive-sync.md index 677dfa6..a48e490 100644 --- a/docs/explanation/grasp-02-proactive-sync.md +++ b/docs/explanation/grasp-02-proactive-sync.md @@ -25,7 +25,10 @@ Key Architectural Points: - **Discovery management**: The nature of discovery inherently leads to a drip feed of root_events (e.g., Repo Announcements, Issues, Patches and PRs) that require additional subscriptions. Without careful management this can lead to large numbers of subscriptions and potentially rate limiting. Mitigation strategies: - Self-subscriber waits for 5s to batch updates before creating new filters / subscriptions, allowing time for most events to be received from outstanding subscriptions from connected relays - PendingBatch tracks each new set of filters that may require pagination until they are complete - - Avoid long awaits - recompute desired filters when connection is established to ensure filters are as consolidated as possible + - Websocket handshakes run in at most eight bounded workers outside the sync + actor; only the actor applies their results, and subscriptions start only + after the relay reports `Connected` + - Recompute desired filters when connection is established to ensure filters are as consolidated as possible - Consolidation function ensures number of live_sync subscriptions don't reach rate-limiting limits (threshold: 70 filters) - **Quick Reconnect** (< 15mins) - doesn't do a full reconciliation vs fresh start (longer disconnect or relaunch binary) - **Background timers** handle relay connection health and metrics, handling reconnects after backoff and recovery after rate-limiting @@ -981,6 +984,8 @@ RateLimited -> previous state: After 65-second cooldown expires cannot make a purgatory invitation permanently lose its sync path - **Rate limiting**: Distinct from connection failures - triggered by relay NOTICE messages - **Connection timeout**: Set to `base_backoff_secs` to ensure retry timing works correctly +- **Connection concurrency**: At most eight DNS/websocket attempts run at once; + queued attempts do not start health backoff until a worker slot is available --- diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 1cd20be..036ef95 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -49,7 +49,7 @@ use std::time::{Duration, Instant}; use futures_util::future::join_all; use nostr_sdk::prelude::*; -use tokio::sync::{broadcast, Mutex, RwLock}; +use tokio::sync::{broadcast, Mutex, RwLock, Semaphore}; use crate::config::Config; use crate::nostr::builder::Nip34WritePolicy; @@ -241,6 +241,7 @@ impl RelayState { /// therefore keep the relay connected. fn is_disconnect_candidate(&self, has_pending_batches: bool, has_desired_work: bool) -> bool { if self.is_bootstrap + || self.connection_status == ConnectionStatus::Connecting || self.connection_status == ConnectionStatus::Disconnecting || !self.repos.is_empty() || !self.root_events.is_empty() @@ -403,11 +404,20 @@ pub struct EoseNotification { pub sub_id: SubscriptionId, } -/// Notification from spawned tasks about successful connection +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct ConnectAttemptToken(u64); + #[derive(Debug)] -pub struct ConnectNotification { - /// The relay URL that connected - pub relay_url: String, +enum ConnectAttemptOutcome { + Connected, + Failed(String), +} + +#[derive(Debug)] +struct ConnectAttemptResult { + relay_url: String, + token: ConnectAttemptToken, + outcome: ConnectAttemptOutcome, } /// Quick reconnect window in seconds (15 minutes) @@ -416,10 +426,52 @@ const QUICK_RECONNECT_WINDOW_SECS: u64 = 15 * 60; /// Maximum filter count before triggering consolidation const CONSOLIDATION_THRESHOLD: usize = 70; +/// Bound concurrent DNS and websocket handshakes so a large relay list cannot +/// exhaust network resources while keeping the sync actor responsive. +const MAX_CONCURRENT_CONNECT_ATTEMPTS: usize = 8; + /// Page size threshold for historic sync pagination (non-negentropy) /// If a subscription receives >= 75 events, we fetch the next page const PAGINATION_THRESHOLD: usize = 75; +fn reserve_connect_attempt( + in_flight: &mut HashMap, + next_token: &mut u64, + relay_url: &str, +) -> Option { + if in_flight.contains_key(relay_url) { + return None; + } + *next_token = next_token + .checked_add(1) + .expect("connect attempt token exhausted"); + let token = ConnectAttemptToken(*next_token); + in_flight.insert(relay_url.to_string(), token); + Some(token) +} + +fn take_connect_attempt( + in_flight: &mut HashMap, + relay_url: &str, + token: ConnectAttemptToken, +) -> bool { + if in_flight.get(relay_url) != Some(&token) { + return false; + } + in_flight.remove(relay_url); + true +} + +async fn begin_connect_attempt( + semaphore: Arc, + health_tracker: Arc, + relay_url: &str, +) -> Option { + let permit = semaphore.acquire_owned().await.ok()?; + health_tracker.record_attempt(relay_url); + Some(permit) +} + #[derive(Debug, Default)] struct DeferredConsolidations { relays: HashSet, @@ -743,14 +795,20 @@ pub struct SyncManager { health_tracker: Arc, /// Counter for generating unique batch IDs next_batch_id: u64, + /// Monotonic identity for rejecting stale connection results. + next_connect_attempt_token: u64, + /// One active or queued connection attempt per canonical relay URL. + in_flight_connect_attempts: HashMap, + /// Shared bound for DNS and websocket handshakes. + connect_attempt_semaphore: Arc, /// Relays whose subscription consolidation waits for in-flight batches to drain. deferred_consolidations: DeferredConsolidations, /// Channel for disconnect notifications (set during run) disconnect_tx: Option>, /// Channel for EOSE notifications (set during run) eose_tx: Option>, - /// Channel for connect notifications (set during run) - connect_tx: Option>, + /// Returns connection outcomes to the sync actor for serialized state changes. + connect_attempt_result_tx: Option>, /// Wakes the actor when a batch completion may unblock consolidation. deferred_consolidation_tx: Option>, /// Channel for broadcasting shutdown signal to all background tasks @@ -831,10 +889,13 @@ impl SyncManager { dependency_refetch_attempts: Arc::new(std::sync::Mutex::new(HashMap::new())), health_tracker: Arc::new(RelayHealthTracker::new(config)), next_batch_id: 0, + next_connect_attempt_token: 0, + in_flight_connect_attempts: HashMap::new(), + connect_attempt_semaphore: Arc::new(Semaphore::new(MAX_CONCURRENT_CONNECT_ATTEMPTS)), deferred_consolidations: DeferredConsolidations::default(), disconnect_tx: None, eose_tx: None, - connect_tx: None, + connect_attempt_result_tx: None, deferred_consolidation_tx: None, shutdown_tx: None, metrics: sync_metrics, @@ -1650,7 +1711,7 @@ impl SyncManager { /// 2. Spawns daily timer for periodic fresh syncs /// 3. Connects to bootstrap relay if configured /// 4. Handles relay actions from self-subscriber - /// 5. Handles disconnect, EOSE, and connect notifications from spawned relay tasks + /// 5. Handles disconnect/EOSE notifications and connection-worker results pub async fn run(mut self) { use tokio::sync::mpsc; @@ -1669,8 +1730,10 @@ impl SyncManager { // 3. Create EOSE channel for spawned tasks -> manager communication let (eose_tx, mut eose_rx) = mpsc::channel::(100); - // 4. Create connect channel for spawned tasks -> manager communication - let (connect_tx, mut connect_rx) = mpsc::channel::(100); + // 4. Connection workers never mutate manager state. Their unbounded + // result channel cannot make a completed worker wait behind the actor. + let (connect_attempt_result_tx, mut connect_attempt_result_rx) = + mpsc::unbounded_channel::(); // Batch completion can be synchronous, so use a non-blocking wakeup // instead of waiting for pending work while holding the actor lock. @@ -1694,7 +1757,7 @@ impl SyncManager { // 5b. Store channel senders for use by handlers self.disconnect_tx = Some(disconnect_tx.clone()); self.eose_tx = Some(eose_tx.clone()); - self.connect_tx = Some(connect_tx.clone()); + self.connect_attempt_result_tx = Some(connect_attempt_result_tx); self.deferred_consolidation_tx = Some(deferred_consolidation_tx); self.shutdown_tx = Some(shutdown_tx.clone()); @@ -1703,7 +1766,7 @@ impl SyncManager { match canonical_relay_key(bootstrap_url) { Ok(relay_url) => { self.register_relay(relay_url.clone(), true).await; - self.try_connect_relay(&relay_url).await; + self.schedule_connect_relay(&relay_url).await; } Err(error) => { tracing::warn!( @@ -1751,7 +1814,7 @@ impl SyncManager { run_purgatory_announcement_sync(purgatory_sync_manager, purgatory_sync_shutdown).await; }); - // 12. Main loop - handle actions from self-subscriber, disconnect, EOSE, and connect notifications + // 12. Main loop - handle actions, lifecycle notifications, and worker results loop { // Wait for an event without holding the lock tokio::select! { @@ -1791,16 +1854,15 @@ impl SyncManager { } } } - connect = connect_rx.recv() => { - match connect { - Some(notification) => { - // Acquire lock to process connect + result = connect_attempt_result_rx.recv() => { + match result { + Some(result) => { + // Connection state changes remain serialized through the actor. let mut manager = sync_manager.lock().await; - manager.handle_connect_or_reconnect(¬ification.relay_url).await; + manager.handle_connect_attempt_result(result).await; } None => { - // All connect senders dropped - unlikely but handle gracefully - tracing::debug!("Connect channel closed"); + tracing::debug!("Connect attempt result channel closed"); } } } @@ -1850,7 +1912,7 @@ impl SyncManager { // Register relay (creates RelayConnection, initializes RelayState, updates metrics) self.register_relay(action.relay_url.clone(), false).await; - self.try_connect_relay(&action.relay_url).await; + self.schedule_connect_relay(&action.relay_url).await; // Connection will trigger handle_connect_or_reconnect which will process items return; } @@ -2371,7 +2433,7 @@ impl SyncManager { /// /// Creates a RelayConnection object and stores it in the connections HashMap. /// Also initializes RelayState if it doesn't exist. - /// Does NOT connect - connection happens via try_connect_relay or retry_disconnected_relays. + /// Does NOT connect - connection happens via the bounded scheduler. /// The RelayConnection persists forever and is reused on reconnects. async fn register_relay(&mut self, relay_url: String, is_bootstrap: bool) { let relay_url = match canonical_relay_key(&relay_url) { @@ -2443,112 +2505,167 @@ impl SyncManager { } } - /// Attempt a single connection to a registered relay + /// Queue one connection attempt without blocking the sync actor. /// - /// Uses the existing RelayConnection from the HashMap and attempts to connect. - /// On success, sends ConnectNotification which triggers handle_connect_or_reconnect. - /// On failure, updates state and health tracker. - async fn try_connect_relay(&mut self, relay_url: &str) { - // 1. Mark attempting and update metrics - { - let mut index = self.relay_sync_index.write().await; - if let Some(state) = index.get_mut(relay_url) { - state.connection_status = ConnectionStatus::Connecting; + /// DNS and the websocket handshake run in a spawned worker, bounded globally + /// by `MAX_CONCURRENT_CONNECT_ATTEMPTS`. The actor owns every lifecycle + /// transition when it receives the worker result. + async fn schedule_connect_relay(&mut self, relay_url: &str) { + let relay_url = match canonical_relay_key(relay_url) { + Ok(relay_url) => relay_url, + Err(error) => { + tracing::warn!(relay = %relay_url, error = %error, "Rejecting invalid sync relay target"); + return; } + }; + let Some(result_tx) = self.connect_attempt_result_tx.clone() else { + tracing::error!(relay = %relay_url, "Connection scheduler is not running"); + return; + }; + let Some(connection) = self.connections.get(&relay_url).cloned() else { + tracing::error!(relay = %relay_url, "No RelayConnection registered"); + return; + }; + + let can_schedule = { + let mut index = self.relay_sync_index.write().await; + let can_schedule = index + .get(&relay_url) + .is_some_and(|state| state.connection_status == ConnectionStatus::Disconnected); + if can_schedule { + index + .get_mut(&relay_url) + .expect("relay state checked immediately above") + .connection_status = ConnectionStatus::Connecting; + } + can_schedule + }; + if !can_schedule { + tracing::trace!(relay = %relay_url, "Relay does not need another connection attempt"); + return; } - // Update metrics to show connecting status - if let Some(ref metrics) = self.metrics { - metrics.record_connection_status(relay_url, ConnectionStatus::Connecting); - } - - // 2. Record attempt in health tracker - self.health_tracker.record_attempt(relay_url); - - // 3. Get connection and attempt - let connection = match self.connections.get(relay_url) { - Some(c) => c, + let token = match reserve_connect_attempt( + &mut self.in_flight_connect_attempts, + &mut self.next_connect_attempt_token, + &relay_url, + ) { + Some(token) => token, None => { - tracing::error!(relay = %relay_url, "No RelayConnection registered"); + tracing::trace!(relay = %relay_url, "Connection attempt already queued"); return; } }; + if let Some(ref metrics) = self.metrics { + metrics.record_connection_status(&relay_url, ConnectionStatus::Connecting); + } + + let health_tracker = Arc::clone(&self.health_tracker); + let semaphore = Arc::clone(&self.connect_attempt_semaphore); let timeout = self.health_tracker.base_backoff_secs(); + tokio::spawn(async move { + let Some(_permit) = begin_connect_attempt(semaphore, health_tracker, &relay_url).await + else { + return; + }; + let outcome = match connection.connect(timeout).await { + Ok(()) => ConnectAttemptOutcome::Connected, + Err(error) => ConnectAttemptOutcome::Failed(error), + }; + let _ = result_tx.send(ConnectAttemptResult { + relay_url, + token, + outcome, + }); + }); + } - match connection.connect(timeout).await { - Ok(()) => { - // Success - record and send notification - self.health_tracker.record_success(relay_url); + async fn handle_connect_attempt_result(&mut self, result: ConnectAttemptResult) { + if !take_connect_attempt( + &mut self.in_flight_connect_attempts, + &result.relay_url, + result.token, + ) { + tracing::debug!( + relay = %result.relay_url, + token = result.token.0, + "Ignoring stale connection attempt result" + ); + return; + } + let still_connecting = self + .relay_sync_index + .read() + .await + .get(&result.relay_url) + .is_some_and(|state| state.connection_status == ConnectionStatus::Connecting); + if !still_connecting { + tracing::debug!( + relay = %result.relay_url, + token = result.token.0, + "Ignoring connection result after lifecycle state changed" + ); + return; + } + + match result.outcome { + ConnectAttemptOutcome::Connected => { + self.health_tracker.record_success(&result.relay_url); if let Some(ref metrics) = self.metrics { - metrics.record_connection_attempt(relay_url, true); - } - - if let Some(ref connect_tx) = self.connect_tx { - let _ = connect_tx - .send(ConnectNotification { - relay_url: relay_url.to_string(), - }) - .await; + metrics.record_connection_attempt(&result.relay_url, true); } + self.handle_connect_or_reconnect(&result.relay_url).await; } - Err(e) => { - // Classify error to determine if it's a naughty relay or transient issue - let error_str = e.to_string(); - - if let Some(category) = naughty_list::NaughtyListTracker::classify_error(&error_str) - { - // Persistent infrastructure issue - use naughty list + ConnectAttemptOutcome::Failed(error) => { + if let Some(category) = naughty_list::NaughtyListTracker::classify_error(&error) { if let Some(ref naughty_list) = self.health_tracker.naughty_list() { - let is_new = naughty_list.record(relay_url, category, error_str.clone()); - + let is_new = + naughty_list.record(&result.relay_url, category, error.clone()); if is_new { tracing::warn!( - relay = %relay_url, + relay = %result.relay_url, category = ?category, - error = %e, + error = %error, "Relay has persistent configuration issue, added to naughty list" ); } else { tracing::debug!( - relay = %relay_url, + relay = %result.relay_url, category = ?category, "Naughty relay failure (already tracked)" ); } } } else { - // Transient network issue - use existing backoff flow tracing::debug!( - relay = %relay_url, - error = %e, + relay = %result.relay_url, + error = %error, "Connection failed (transient issue, backoff active)" ); } - - // 4. Update state back to Disconnected on failure - { - let mut index = self.relay_sync_index.write().await; - if let Some(state) = index.get_mut(relay_url) { - state.connection_status = ConnectionStatus::Disconnected; - } - } - - // 5. Record failure in health tracker - self.health_tracker.record_failure(relay_url); - - // 6. Update metrics - if let Some(ref metrics) = self.metrics { - metrics.record_connection_attempt(relay_url, false); - metrics.record_connection_status(relay_url, ConnectionStatus::Disconnected); - metrics - .record_health_state(relay_url, self.health_tracker.get_state(relay_url)); - } + self.record_connection_attempt_failure(&result.relay_url) + .await; } } } + async fn record_connection_attempt_failure(&self, relay_url: &str) { + { + let mut index = self.relay_sync_index.write().await; + if let Some(state) = index.get_mut(relay_url) { + state.connection_status = ConnectionStatus::Disconnected; + } + } + self.health_tracker.record_failure(relay_url); + if let Some(ref metrics) = self.metrics { + metrics.record_connection_attempt(relay_url, false); + metrics.record_connection_status(relay_url, ConnectionStatus::Disconnected); + metrics.record_health_state(relay_url, self.health_tracker.get_state(relay_url)); + } + } + /// Recompute sync actions for a specific relay /// /// Uses derive_relay_targets and compute_actions to find new items @@ -3792,7 +3909,7 @@ impl SyncManager { /// - Have repos or root events to sync (not empty) /// - Have passed the exponential backoff period (respects health tracker) /// - /// For each eligible relay, a reconnection is attempted via try_connect_relay. + /// For each eligible relay, a reconnection is queued via schedule_connect_relay. async fn retry_disconnected_relays(&mut self) { let desired_relays: HashSet = { let repo_index = self.repo_sync_index.read().await; @@ -3849,7 +3966,7 @@ impl SyncManager { health_state = %self.health_tracker.get_state(&relay_url), "Attempting reconnection" ); - self.try_connect_relay(&relay_url).await; + self.schedule_connect_relay(&relay_url).await; } } @@ -4402,6 +4519,109 @@ impl SyncManager { mod tests { use super::*; + #[test] + fn connect_attempt_tokens_deduplicate_and_reject_stale_results() { + let relay = "wss://relay.example"; + let mut in_flight = HashMap::new(); + let mut next_token = 0; + + let first = reserve_connect_attempt(&mut in_flight, &mut next_token, relay) + .expect("first attempt should be reserved"); + assert!( + reserve_connect_attempt(&mut in_flight, &mut next_token, relay).is_none(), + "a queued or running relay must not be scheduled twice" + ); + assert!( + !take_connect_attempt(&mut in_flight, relay, ConnectAttemptToken(first.0 + 1)), + "a stale worker result must not consume the active attempt" + ); + assert_eq!(in_flight.get(relay), Some(&first)); + assert!(take_connect_attempt(&mut in_flight, relay, first)); + + let second = reserve_connect_attempt(&mut in_flight, &mut next_token, relay) + .expect("relay should be schedulable after completion"); + assert_ne!(first, second, "attempt identities must not be reused"); + } + + #[tokio::test] + async fn connect_attempt_semaphore_caps_parallel_workers() { + use std::sync::atomic::{AtomicUsize, Ordering}; + + const WORKERS: usize = MAX_CONCURRENT_CONNECT_ATTEMPTS * 3; + let semaphore = Arc::new(Semaphore::new(MAX_CONCURRENT_CONNECT_ATTEMPTS)); + let release = Arc::new(Semaphore::new(0)); + let active = Arc::new(AtomicUsize::new(0)); + let maximum = Arc::new(AtomicUsize::new(0)); + let (started_tx, mut started_rx) = tokio::sync::mpsc::unbounded_channel(); + let mut workers = Vec::new(); + + for _ in 0..WORKERS { + let semaphore = Arc::clone(&semaphore); + let release = Arc::clone(&release); + let active = Arc::clone(&active); + let maximum = Arc::clone(&maximum); + let started_tx = started_tx.clone(); + workers.push(tokio::spawn(async move { + let _permit = semaphore.acquire_owned().await.unwrap(); + let now_active = active.fetch_add(1, Ordering::SeqCst) + 1; + maximum.fetch_max(now_active, Ordering::SeqCst); + started_tx.send(()).unwrap(); + let _release = release.acquire().await.unwrap(); + active.fetch_sub(1, Ordering::SeqCst); + })); + } + drop(started_tx); + + for _ in 0..MAX_CONCURRENT_CONNECT_ATTEMPTS { + started_rx.recv().await.unwrap(); + } + assert!( + tokio::time::timeout(Duration::from_millis(25), started_rx.recv()) + .await + .is_err(), + "a ninth worker started while all eight permits were occupied" + ); + + release.add_permits(WORKERS); + for worker in workers { + worker.await.unwrap(); + } + assert_eq!( + maximum.load(Ordering::SeqCst), + MAX_CONCURRENT_CONNECT_ATTEMPTS + ); + } + + #[tokio::test] + async fn queued_connect_attempt_records_health_only_when_worker_starts() { + let relay = "wss://queued.example"; + let semaphore = Arc::new(Semaphore::new(0)); + let health_tracker = Arc::new(RelayHealthTracker::with_defaults()); + let worker_semaphore = Arc::clone(&semaphore); + let worker_health = Arc::clone(&health_tracker); + + let worker = tokio::spawn(async move { + begin_connect_attempt(worker_semaphore, worker_health, relay).await + }); + tokio::task::yield_now().await; + assert!( + health_tracker.get_health(relay).is_none(), + "waiting for scheduler capacity must not start backoff" + ); + + semaphore.add_permits(1); + let _permit = worker + .await + .unwrap() + .expect("worker should start after a permit becomes available"); + assert!( + health_tracker + .get_health(relay) + .is_some_and(|health| health.last_attempt_time.is_some()), + "health attempt time must be recorded when the worker starts" + ); + } + #[test] fn canonical_relay_keys_dedupe_only_the_root_slash() { assert_eq!( @@ -4570,10 +4790,17 @@ mod tests { #[test] fn disconnected_empty_relay_can_still_be_cleaned_up() { - let source = RelayState::default(); + let mut source = RelayState::default(); assert!(source.is_disconnect_candidate(false, false)); assert!(!source.is_disconnect_candidate(false, true)); + + source.historic_sync_completed = true; + source.connection_status = ConnectionStatus::Connecting; + assert!( + !source.is_disconnect_candidate(false, false), + "cleanup must not race an in-flight connection worker" + ); } #[tokio::test] diff --git a/src/sync/relay_connection.rs b/src/sync/relay_connection.rs index b1de02f..54382d1 100644 --- a/src/sync/relay_connection.rs +++ b/src/sync/relay_connection.rs @@ -22,6 +22,32 @@ use tokio::sync::mpsc; use crate::nostr::SharedDatabase; +/// Interval between relay-status checks while a connection attempt is in flight. +/// +/// nostr-sdk may return from `try_connect_relay` while another task still has +/// the relay in `Connecting`; subscriptions must wait for actual readiness. +const CONNECTION_STATUS_POLL_INTERVAL: Duration = Duration::from_millis(25); + +async fn wait_for_connected_status(mut relay_status: F) -> Result<(), RelayStatus> +where + F: FnMut() -> RelayStatus, +{ + loop { + let status = relay_status(); + match status { + RelayStatus::Connected => return Ok(()), + RelayStatus::Disconnected + | RelayStatus::Terminated + | RelayStatus::Banned + | RelayStatus::Sleeping + | RelayStatus::Shutdown => return Err(status), + RelayStatus::Initialized | RelayStatus::Pending | RelayStatus::Connecting => { + tokio::time::sleep(CONNECTION_STATUS_POLL_INTERVAL).await; + } + } + } +} + /// Events from a relay connection #[derive(Debug)] pub enum RelayEvent { @@ -150,7 +176,7 @@ impl RelayConnection { /// This method: /// 1. Adds the relay to the client /// 2. Establishes the WebSocket connection - /// 3. Verifies connection was established + /// 3. Waits for the relay status to report `Connected` /// /// Subscriptions are handled separately via handle_connect_or_reconnect. /// @@ -163,34 +189,52 @@ impl RelayConnection { /// * `Ok(())` - Connection established successfully /// * `Err(String)` with error description on failure pub async fn connect(&self, connection_timeout_secs: u64) -> Result<(), String> { - // Add relay to client - self.client - .add_relay(&self.url) - .await - .map_err(|e| format!("Failed to add relay {}: {}", self.url, e))?; - - // Establish connection using try_connect_relay for immediate failure detection - // - // Key difference from client.connect(): - // - try_connect_relay: Single attempt with timeout, returns Err on failure, - // does NOT spawn background retry task (we control retries via HealthTracker) - // - connect(): Spawns background task, returns immediately, auto-retries forever - // - // Using try_connect_relay gives us: - // 1. Immediate error return on connection failure - // 2. Configurable timeout (set to base_backoff_secs to ensure retry timing works) - // 3. No conflicting retry logic (we use HealthTracker for backoff) - // 4. Cleaner error messages for metrics recording - // - // See: nostr-sdk-0.44 Client::try_connect_relay documentation - self.client - .try_connect_relay( - &self.url, - std::time::Duration::from_secs(connection_timeout_secs), + let connection_timeout = Duration::from_secs(connection_timeout_secs); + let connection_deadline = tokio::time::Instant::now() + connection_timeout; + let timeout_error = || { + format!( + "Timed out connecting to relay {} after {} seconds", + self.url, connection_timeout_secs ) + }; + + let relay = tokio::time::timeout_at(connection_deadline, async { + self.client + .add_relay(&self.url) + .await + .map_err(|e| format!("Failed to add relay {}: {}", self.url, e))?; + + self.client + .relay(&self.url) + .await + .map_err(|e| format!("Failed to get relay {}: {}", self.url, e))? + .ok_or_else(|| format!("Relay {} was not added to the client", self.url)) + }) + .await + .map_err(|_| timeout_error())??; + + // Use one deadline for both the SDK attempt and readiness verification. + // Cancelling the SDK call with an outer timeout can leave its relay + // status stranded at Connecting, so pass it the remaining budget. + let remaining = connection_deadline.saturating_duration_since(tokio::time::Instant::now()); + self.client + .try_connect_relay(&self.url, remaining) .await .map_err(|e| format!("Failed to connect to relay {}: {}", self.url, e))?; + tokio::time::timeout_at( + connection_deadline, + wait_for_connected_status(|| relay.status()), + ) + .await + .map_err(|_| timeout_error())? + .map_err(|status| { + format!( + "Relay {} entered terminal status {} before connecting", + self.url, status + ) + })?; + tracing::info!(url = %self.url, "Connected to relay"); Ok(()) } @@ -621,6 +665,44 @@ impl RelayConnection { mod tests { use super::*; + #[tokio::test] + async fn connection_readiness_waits_until_status_is_connected() { + let mut statuses = [ + RelayStatus::Connecting, + RelayStatus::Connecting, + RelayStatus::Connected, + ] + .into_iter(); + + wait_for_connected_status(|| statuses.next().expect("status sequence exhausted")) + .await + .expect("delayed connection should become ready"); + } + + #[tokio::test] + async fn connection_readiness_remains_bounded_when_connecting_never_completes() { + let result = tokio::time::timeout( + Duration::from_millis(60), + wait_for_connected_status(|| RelayStatus::Connecting), + ) + .await; + + assert!( + result.is_err(), + "a relay stuck in Connecting must not be reported as ready" + ); + } + + #[tokio::test] + async fn connection_readiness_fails_on_terminal_status() { + let mut statuses = [RelayStatus::Connecting, RelayStatus::Disconnected].into_iter(); + + assert_eq!( + wait_for_connected_status(|| statuses.next().expect("status sequence exhausted")).await, + Err(RelayStatus::Disconnected) + ); + } + #[test] fn test_normalize_url_with_wss_scheme() { let url = "wss://relay.example.com"; From 543951f92b9ad411be0a7fb6b223dcc1914ae151 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 08:45:55 +0100 Subject: [PATCH 04/11] fix(sync): bound invitation dependency recovery An accepted maintainer announcement can remain in purgatory until the inviter announcement or state event is recovered. Exact-ID recovery previously delegated relay choice to the SDK, which can fail with an empty automatic target or attribute an event fetched from a different client relay to the wrong source. Separately, a NIP-77 peer could keep sending messages without completing its diff, holding the sync actor indefinitely despite the SDK's idle timeout. Fetch retained dependency IDs from the RelayConnection's exact relay and impose a 15-second wall-clock limit on dry-run negentropy. A timed-out relay is marked unsupported so the existing REQ+EOSE fallback can continue invitation sync. --- CHANGELOG.md | 3 + docs/explanation/grasp-02-proactive-sync.md | 10 +- src/sync/relay_connection.rs | 175 +++++++++++++++++--- 3 files changed, 165 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c980cd6..26c47d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix invitation acceptance sync across large relay lists by moving websocket handshakes out of the sync actor, limiting them to eight concurrent attempts, and waiting for each relay to be connected before starting subscriptions. +- Fix invitation dependency recovery by targeting retained event IDs at their + associated relay and falling back from NIP-77 after a 15-second total + deadline, so a missing or endlessly active exchange cannot stall the actor. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/docs/explanation/grasp-02-proactive-sync.md b/docs/explanation/grasp-02-proactive-sync.md index a48e490..7193569 100644 --- a/docs/explanation/grasp-02-proactive-sync.md +++ b/docs/explanation/grasp-02-proactive-sync.md @@ -779,6 +779,11 @@ If negentropy fails (relay doesn't support NIP-77, network error, etc.): 2. The sync falls back to traditional REQ+EOSE 3. No error is raised - fallback is automatic +Each dry-run diff also has a 15-second total wall-clock deadline. This is +separate from the SDK's initial-response and idle timers: a relay that keeps an +exchange active without completing it cannot hold the sync actor indefinitely. +Reaching the deadline uses the same unsupported-relay fallback path. + ### Integration with Rejected Events Index The rejected events index prevents wasteful re-fetching during negentropy sync by excluding rejected event IDs from the reconciliation process: @@ -812,7 +817,10 @@ needs the inviter's Git data. In that flow: 1. Dependency-resolvable entries remain in the cold index until processing succeeds. 2. If the full event is still in the hot cache, it is re-processed immediately. -3. If the full event expired, the sync manager requests only the retained event IDs from connected relays across the recursive maintainer chain. +3. If the full event expired, the sync manager requests only the retained event + IDs from connected relays across the recursive maintainer chain. Each + request explicitly targets its associated relay connection instead of the + SDK's automatic pool-wide relay selection. 4. Relay requests run in parallel as bounded background work; announcements are processed before state events that may depend on them. 5. Successful or duplicate events are removed from both tiers. Failed and empty requests retain their IDs for a throttled retry. diff --git a/src/sync/relay_connection.rs b/src/sync/relay_connection.rs index 54382d1..7bb470a 100644 --- a/src/sync/relay_connection.rs +++ b/src/sync/relay_connection.rs @@ -17,11 +17,18 @@ use futures_util::StreamExt; use nostr_sdk::prelude::*; +use std::future::Future; use std::time::Duration; use tokio::sync::mpsc; use crate::nostr::SharedDatabase; +/// Maximum wall-clock time for one dry-run NIP-77 reconciliation. +/// +/// The SDK has initial-response and idle timers, but a relay can keep an +/// exchange alive indefinitely by continuing to send reconciliation messages. +const NEGENTROPY_DIFF_TIMEOUT: Duration = Duration::from_secs(15); + /// Interval between relay-status checks while a connection attempt is in flight. /// /// nostr-sdk may return from `try_connect_relay` while another task still has @@ -454,7 +461,24 @@ impl RelayConnection { filter: Filter, timeout: Duration, ) -> Result, String> { - self.client + let relay = self + .client + .relay(&self.url) + .await + .map_err(|error| { + format!( + "Failed to fetch events from {}: failed to resolve relay: {}", + self.url, error + ) + })? + .ok_or_else(|| { + format!( + "Failed to fetch events from {}: relay is not registered", + self.url + ) + })?; + + relay .fetch_events(filter) .timeout(timeout) .await @@ -581,7 +605,32 @@ impl RelayConnection { ) -> Result { // Use dry_run to only identify differences without downloading events let sync_opts = SyncOptions::default().dry_run(); + let client = self.client.clone(); + let sync_task = async move { + match client.sync(filter).opts(sync_opts).await { + Ok(output) => { + if !output.failed.is_empty() { + Err(format!("Negentropy diff had failures: {:?}", output.failed)) + } else { + Ok(output.value) + } + } + Err(error) => Err(format!("Negentropy diff failed: {}", error)), + } + }; + self.run_negentropy_diff_with_timeout(sync_task, NEGENTROPY_DIFF_TIMEOUT) + .await + } + + async fn run_negentropy_diff_with_timeout( + &self, + sync_task: F, + timeout_duration: Duration, + ) -> Result + where + F: Future>, + { // Clone the atomic for the polling task let nip77_status = self.nip77_supported.clone(); let url = self.url.clone(); @@ -601,29 +650,23 @@ impl RelayConnection { } }; - // Race the sync operation against the polling task - let sync_task = self.client.sync(filter.clone()).opts(sync_opts); - - let result = tokio::select! { - poll_result = poll_task => { - // Polling detected NIP-77 not supported - poll_result - } - sync_result = sync_task => { - // Sync completed (or failed) first - match sync_result { - Ok(output) => { - // Check for any failures - // Note: Timeouts are common for relays without NIP-77 support - if !output.failed.is_empty() { - Err(format!("Negentropy diff had failures: {:?}", output.failed)) - } else { - Ok(output.value) - } - } - Err(e) => Err(format!("Negentropy diff failed: {}", e)) + let result = match tokio::time::timeout(timeout_duration, async { + tokio::select! { + poll_result = poll_task => { + poll_result + } + sync_result = sync_task => { + sync_result } } + }) + .await + { + Ok(result) => result, + Err(_) => Err(format!( + "Negentropy diff timed out after {:.3}s", + timeout_duration.as_secs_f64() + )), }; match result { @@ -664,6 +707,8 @@ impl RelayConnection { #[cfg(test)] mod tests { use super::*; + use nostr_relay_builder::prelude::LocalRelayBuilder; + use std::future::pending; #[tokio::test] async fn connection_readiness_waits_until_status_is_connected() { @@ -703,6 +748,92 @@ mod tests { ); } + #[tokio::test] + async fn hung_negentropy_diff_times_out_and_disables_future_attempts() { + let connection = RelayConnection::new("ws://127.0.0.1:1".to_string(), Keys::generate()); + assert!(connection.supports_negentropy().await); + + let result = connection + .run_negentropy_diff_with_timeout( + pending::>(), + Duration::from_millis(20), + ) + .await; + + assert!(result + .expect_err("a hung NIP-77 exchange must reach its local deadline") + .contains("timed out")); + assert!( + !connection.supports_negentropy().await, + "a timed-out relay must use REQ+EOSE for future historic batches" + ); + } + + #[tokio::test] + async fn fetch_events_targets_the_connections_exact_relay() { + let configured = LocalRelayBuilder::default().build(); + configured.run().await.expect("start configured relay"); + let other = LocalRelayBuilder::default().build(); + other.run().await.expect("start other relay"); + + let expected = EventBuilder::text_note("only on the other relay") + .finalize(&Keys::generate()) + .expect("build event"); + other + .add_event(expected.clone()) + .await + .expect("seed other relay"); + + let connection = RelayConnection::new(configured.url().await.to_string(), Keys::generate()); + connection + .connect(3) + .await + .expect("connect configured relay"); + let other_url = other.url().await; + connection + .client + .add_relay(other_url.clone()) + .await + .expect("register other relay"); + connection + .client + .try_connect_relay(other_url, Duration::from_secs(3)) + .await + .expect("connect other relay"); + + let fetched = connection + .fetch_events(Filter::new().id(expected.id), Duration::from_secs(2)) + .await + .expect("fetch from configured relay"); + assert!( + fetched.is_empty(), + "a relay-specific fetch must not return an event from another client relay" + ); + + connection.disconnect().await; + configured.shutdown(); + other.shutdown(); + } + + #[tokio::test] + async fn fetch_events_reports_an_unregistered_exact_relay() { + let connection = RelayConnection::new("ws://127.0.0.1:1".to_string(), Keys::generate()); + + let error = connection + .fetch_events( + Filter::new().kind(Kind::TextNote), + Duration::from_millis(50), + ) + .await + .expect_err("an unregistered relay must fail explicitly"); + + assert!(error.contains("relay is not registered"), "{error}"); + assert!( + !error.contains("relay/s not specified"), + "exact-relay fetch must not use an empty automatic target: {error}" + ); + } + #[test] fn test_normalize_url_with_wss_scheme() { let url = "wss://relay.example.com"; From 77507dd22623191215575a3dc7445ddd04aace18 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 08:47:29 +0100 Subject: [PATCH 05/11] fix(sync): stabilize large invitation filter sets A relay may legitimately need more than 70 live subscriptions when an invitation spans many repositories, roots, or state-only maintainer sources. Treating the absolute subscription count as fragmentation meant consolidation rebuilt the same unavoidable large set and immediately triggered itself again, repeatedly consuming the sync actor. Use the current desired GRASP-02 filters plus the generic announcement subscription as the irreducible baseline. The 70-filter threshold now applies only to incremental fragmentation above that baseline, preserving consolidation pressure without making large valid desired sets unstable. --- CHANGELOG.md | 3 + docs/explanation/architecture.md | 6 +- docs/explanation/grasp-02-proactive-sync.md | 4 +- src/sync/mod.rs | 79 +++++++++++++++++++-- 4 files changed, 81 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26c47d9..9b9a64f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix invitation dependency recovery by targeting retained event IDs at their associated relay and falling back from NIP-77 after a 15-second total deadline, so a missing or endlessly active exchange cannot stall the actor. +- Fix large invitation relay sets repeatedly rebuilding their subscriptions by + applying the 70-filter consolidation threshold only to fragmentation above + the relay's irreducible desired live-filter baseline. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index 40c2973..789f26a 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -517,9 +517,9 @@ The ngit-grasp relay implements **Proactive Sync of Nostr Events**, which synchr - **Bounded connection workers** keep slow DNS and websocket handshakes out of the sync actor while limiting network pressure to eight concurrent attempts - **Daily sync** with random 23-25h timer to detect state drift -- **Filter consolidation** when count exceeds 70 to prevent subscription - explosion; rebuilds are deferred until in-flight batches drain so EOSE and - purgatory work remain responsive +- **Filter consolidation** when incremental fragmentation exceeds the desired + live-filter baseline by 70; rebuilds are deferred until in-flight batches + drain so EOSE and purgatory work remain responsive - **Rejected events index** - prevents wasteful broad re-fetching while retaining exact IDs for dependency recovery - **Desired-source retention** keeps listed GRASP-02 relays retryable until repository work is actually confirmed, including StateOnly invitation sync diff --git a/docs/explanation/grasp-02-proactive-sync.md b/docs/explanation/grasp-02-proactive-sync.md index 7193569..fd3601e 100644 --- a/docs/explanation/grasp-02-proactive-sync.md +++ b/docs/explanation/grasp-02-proactive-sync.md @@ -29,7 +29,9 @@ Key Architectural Points: actor; only the actor applies their results, and subscriptions start only after the relay reports `Connected` - Recompute desired filters when connection is established to ensure filters are as consolidated as possible - - Consolidation function ensures number of live_sync subscriptions don't reach rate-limiting limits (threshold: 70 filters) + - Consolidation bounds incremental fragmentation to 70 subscriptions above + the irreducible desired live-filter baseline, so large desired sets remain + stable after rebuilding - **Quick Reconnect** (< 15mins) - doesn't do a full reconciliation vs fresh start (longer disconnect or relaunch binary) - **Background timers** handle relay connection health and metrics, handling reconnects after backoff and recovery after rate-limiting diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 036ef95..05b0187 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -423,7 +423,8 @@ struct ConnectAttemptResult { /// Quick reconnect window in seconds (15 minutes) const QUICK_RECONNECT_WINDOW_SECS: u64 = 15 * 60; -/// Maximum filter count before triggering consolidation +/// Maximum incremental filter fragmentation above the desired live baseline +/// before triggering consolidation. const CONSOLIDATION_THRESHOLD: usize = 70; /// Bound concurrent DNS and websocket handshakes so a large relay list cannot @@ -472,6 +473,21 @@ async fn begin_connect_attempt( Some(permit) } +fn consolidation_fragmentation( + current_count: usize, + new_count: usize, + desired_baseline: usize, +) -> usize { + current_count + .saturating_add(new_count) + .saturating_sub(desired_baseline) +} + +fn should_consolidate(current_count: usize, new_count: usize, desired_baseline: usize) -> bool { + consolidation_fragmentation(current_count, new_count, desired_baseline) + > CONSOLIDATION_THRESHOLD +} + #[derive(Debug, Default)] struct DeferredConsolidations { relays: HashSet, @@ -3698,18 +3714,40 @@ impl SyncManager { .is_some_and(|batches| !batches.is_empty()) } - /// Check if consolidation is needed and trigger if threshold exceeded + async fn desired_live_filter_count(&self, relay_url: &str) -> usize { + let target = { + let repo_index = self.repo_sync_index.read().await; + algorithms::derive_relay_targets(&repo_index).remove(relay_url) + }; + let desired_repo_filters = target.map_or(0, |target| { + filters::build_sync_level_aware_filters( + &target.repos, + &target.state_only_repos, + &target.root_events, + None, + ) + .len() + }); + + // Every connected relay carries one consolidated generic announcement + // subscription in addition to its repository-specific desired filters. + 1 + desired_repo_filters + } + + /// Check if incremental fragmentation exceeds the consolidation threshold. /// - /// Compares current filter count + new filter count against the threshold. - /// If exceeded, triggers consolidation before adding new filters. + /// The desired live set is an irreducible baseline, so a repository whose + /// consolidated filters already exceed 70 must remain stable. async fn maybe_consolidate(&mut self, relay_url: &str, new_count: usize) { let current_count = if let Some(connection) = self.connections.get(relay_url) { connection.subscription_count().await } else { 0 }; + let desired_baseline = self.desired_live_filter_count(relay_url).await; + let fragmentation = consolidation_fragmentation(current_count, new_count, desired_baseline); - if current_count + new_count > CONSOLIDATION_THRESHOLD { + if should_consolidate(current_count, new_count, desired_baseline) { let has_pending_batches = self.has_pending_batches(relay_url).await; if !self .deferred_consolidations @@ -3719,8 +3757,10 @@ impl SyncManager { relay = %relay_url, current_count, new_count, + desired_baseline, + fragmentation, threshold = CONSOLIDATION_THRESHOLD, - "Filter count exceeds threshold; deferring consolidation until pending batches drain" + "Incremental filter fragmentation exceeds threshold; deferring consolidation until pending batches drain" ); return; } @@ -3729,8 +3769,10 @@ impl SyncManager { relay = %relay_url, current_count = current_count, new_count = new_count, + desired_baseline, + fragmentation, threshold = CONSOLIDATION_THRESHOLD, - "Filter count exceeds threshold, consolidating" + "Incremental filter fragmentation exceeds threshold, consolidating" ); self.consolidate(relay_url).await; @@ -4657,6 +4699,29 @@ mod tests { ); } + #[test] + fn desired_baseline_above_threshold_does_not_reconsolidate() { + let desired_baseline = 178; + + assert!( + !should_consolidate(desired_baseline, 1, desired_baseline), + "the irreducible desired set must be a stable post-rebuild baseline" + ); + assert!( + !should_consolidate( + desired_baseline + CONSOLIDATION_THRESHOLD - 1, + 1, + desired_baseline, + ), + "the configured fragmentation headroom is allowed above the baseline" + ); + assert!(should_consolidate( + desired_baseline + CONSOLIDATION_THRESHOLD, + 1, + desired_baseline, + )); + } + #[test] fn deferred_consolidation_runs_only_after_final_batch_completion() { let relay_url = "wss://relay.example"; From 8a141a55d2f90d97c86f0aa8bc12641589aeaacd Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 08:50:09 +0100 Subject: [PATCH 06/11] fix(sync): keep relay retries scheduler-owned The bounded connection workers only provide a real global limit if they own every retry. nostr-sdk enables an independent reconnect loop by default, so a relay that disconnected after its first successful invitation sync could reconnect outside the semaphore, health backoff, and actor metrics. Detached workers could also outlive an aborted sync manager while waiting for capacity or a socket. Disable SDK auto-reconnect for managed relay connections and have every queued or active worker observe the manager's shutdown channel. If its result receiver disappears, a worker closes the connection instead of leaving an unowned socket behind. --- CHANGELOG.md | 3 ++ docs/explanation/grasp-02-proactive-sync.md | 3 +- src/sync/mod.rs | 39 +++++++++++++++------ src/sync/relay_connection.rs | 1 + 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b9a64f..9b99da6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix large invitation relay sets repeatedly rebuilding their subscriptions by applying the 70-filter consolidation threshold only to fragmentation above the relay's irreducible desired live-filter baseline. +- Keep invitation relay connection ownership inside the bounded scheduler by + disabling the SDK's independent auto-reconnect loop and cancelling queued or + active connection workers when the sync manager shuts down. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/docs/explanation/grasp-02-proactive-sync.md b/docs/explanation/grasp-02-proactive-sync.md index fd3601e..6cd1754 100644 --- a/docs/explanation/grasp-02-proactive-sync.md +++ b/docs/explanation/grasp-02-proactive-sync.md @@ -27,7 +27,8 @@ Key Architectural Points: - PendingBatch tracks each new set of filters that may require pagination until they are complete - Websocket handshakes run in at most eight bounded workers outside the sync actor; only the actor applies their results, and subscriptions start only - after the relay reports `Connected` + after the relay reports `Connected`. The sync manager owns retry/backoff + rather than the SDK, and shutdown cancels queued or active workers - Recompute desired filters when connection is established to ensure filters are as consolidated as possible - Consolidation bounds incremental fragmentation to 70 subscriptions above the irreducible desired live-filter baseline, so large desired sets remain diff --git a/src/sync/mod.rs b/src/sync/mod.rs index 05b0187..4f3f124 100644 --- a/src/sync/mod.rs +++ b/src/sync/mod.rs @@ -2580,20 +2580,39 @@ impl SyncManager { let health_tracker = Arc::clone(&self.health_tracker); let semaphore = Arc::clone(&self.connect_attempt_semaphore); let timeout = self.health_tracker.base_backoff_secs(); + let Some(mut shutdown_rx) = self.shutdown_tx.as_ref().map(|sender| sender.subscribe()) + else { + tracing::error!(relay = %relay_url, "Connection scheduler has no shutdown signal"); + return; + }; tokio::spawn(async move { - let Some(_permit) = begin_connect_attempt(semaphore, health_tracker, &relay_url).await - else { + let permit = tokio::select! { + permit = begin_connect_attempt(semaphore, health_tracker, &relay_url) => permit, + _ = shutdown_rx.recv() => return, + }; + let Some(_permit) = permit else { return; }; - let outcome = match connection.connect(timeout).await { - Ok(()) => ConnectAttemptOutcome::Connected, - Err(error) => ConnectAttemptOutcome::Failed(error), + let outcome = tokio::select! { + result = connection.connect(timeout) => match result { + Ok(()) => ConnectAttemptOutcome::Connected, + Err(error) => ConnectAttemptOutcome::Failed(error), + }, + _ = shutdown_rx.recv() => { + connection.disconnect().await; + return; + } }; - let _ = result_tx.send(ConnectAttemptResult { - relay_url, - token, - outcome, - }); + if result_tx + .send(ConnectAttemptResult { + relay_url, + token, + outcome, + }) + .is_err() + { + connection.disconnect().await; + } }); } diff --git a/src/sync/relay_connection.rs b/src/sync/relay_connection.rs index 7bb470a..09b6df8 100644 --- a/src/sync/relay_connection.rs +++ b/src/sync/relay_connection.rs @@ -208,6 +208,7 @@ impl RelayConnection { let relay = tokio::time::timeout_at(connection_deadline, async { self.client .add_relay(&self.url) + .reconnect(false) .await .map_err(|e| format!("Failed to add relay {}: {}", self.url, e))?; From f1d8f1586435a7afcd6e67a19b195b86d841c4d0 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 09:31:18 +0100 Subject: [PATCH 07/11] test(sync): cover delayed invitation sources Invitation acceptance can be published before an inviter's selected GRASP server has repository events available, or while that server is temporarily unreachable. The recovery fixes depend on retaining those unconfirmed sources instead of treating an empty first sync or failed handshake as evidence that they are no longer needed. Exercise both paths through real relay processes, reciprocal maintainer announcements, state events, and Git pushes. The tests require the accepted announcement to leave purgatory and the invitee's target ref to align with the inviter's commit after the source becomes ready. --- tests/sync/maintainer_reprocessing.rs | 284 +++++++++++++++++++++++++- 1 file changed, 283 insertions(+), 1 deletion(-) diff --git a/tests/sync/maintainer_reprocessing.rs b/tests/sync/maintainer_reprocessing.rs index 4cd72bc..45b2430 100644 --- a/tests/sync/maintainer_reprocessing.rs +++ b/tests/sync/maintainer_reprocessing.rs @@ -6,7 +6,7 @@ //! ## Test design //! //! Announcements require git data before they are released from purgatory and -//! served to other relays. The tests exercise both dependency-recovery paths: +//! served to other relays. The tests exercise dependency and source recovery: //! //! relay_b syncs maintainer announcement from relay_a //! → write policy rejects it (no owner announcement in DB yet) @@ -15,6 +15,9 @@ //! → hot copy is re-processed immediately when available //! → expired hot copy is fetched by its retained cold-index event ID //! → maintainer announcement supplies the source clone and git data syncs +//! reciprocal owner announcement reaches relay_b before relay_a is ready +//! → an empty relay_a remains live for later repository events +//! → an unavailable relay_a remains desired and is retried after startup //! //! To guarantee the maintainer announcements arrive at relay_b *before* the owner //! git push, relay_b is started with relay_a as its bootstrap relay. That way @@ -44,6 +47,285 @@ async fn wait_for_log(path: &Path, needle: &str, timeout: Duration) -> bool { } } +fn invitation_acceptance_announcement( + target_relay: &TestRelay, + source_relay_url: &str, + owner_keys: &Keys, + maintainer_keys: &Keys, + identifier: &str, +) -> Event { + let owner_npub = owner_keys + .public_key() + .to_bech32() + .expect("Failed to encode owner npub"); + EventBuilder::new(Kind::GitRepoAnnouncement, "Accepted maintainer invitation") + .tags(vec![ + Tag::identifier(identifier), + Tag::custom( + "clone", + vec![format!( + "http://{}/{}/{}.git", + target_relay.domain(), + owner_npub, + identifier + )], + ), + Tag::custom( + "relays", + vec![source_relay_url.to_string(), target_relay.url().to_string()], + ), + Tag::custom("maintainers", vec![maintainer_keys.public_key().to_hex()]), + ]) + .finalize(owner_keys) + .expect("Failed to create invitation acceptance announcement") +} + +async fn seed_inviter_repository( + source_relay: &TestRelay, + maintainer_keys: &Keys, + invited_owner_keys: &Keys, + identifier: &str, +) -> (tempfile::TempDir, String) { + let maintainer_npub = maintainer_keys + .public_key() + .to_bech32() + .expect("Failed to encode maintainer npub"); + let announcement = + EventBuilder::new(Kind::GitRepoAnnouncement, "Inviting maintainer repository") + .tags(vec![ + Tag::identifier(identifier), + Tag::custom( + "clone", + vec![format!( + "http://{}/{}/{}.git", + source_relay.domain(), + maintainer_npub, + identifier + )], + ), + Tag::custom("relays", vec![source_relay.url().to_string()]), + Tag::custom( + "maintainers", + vec![invited_owner_keys.public_key().to_hex()], + ), + ]) + .finalize(maintainer_keys) + .expect("Failed to create inviter announcement"); + + send_to_relay(source_relay, &announcement) + .await + .expect("Failed to send inviter announcement"); + let git_dir = push_git_data_to_relay( + source_relay, + maintainer_keys, + identifier, + &[&source_relay.domain()], + ) + .await; + let output = Command::new("git") + .args(["rev-parse", "HEAD"]) + .current_dir(git_dir.path()) + .output() + .expect("Failed to read inviter commit"); + assert!(output.status.success(), "Failed to resolve inviter commit"); + let commit = String::from_utf8(output.stdout) + .expect("Inviter commit should be UTF-8") + .trim() + .to_string(); + + let announcement_found = wait_for_event_on_relay( + source_relay.url(), + Filter::new() + .kind(Kind::GitRepoAnnouncement) + .author(maintainer_keys.public_key()) + .identifier(identifier), + Duration::from_secs(5), + ) + .await; + assert!( + announcement_found, + "Inviter announcement should be available before target assertions" + ); + + (git_dir, commit) +} + +async fn assert_invitation_sync_completed( + target_relay: &TestRelay, + owner_keys: &Keys, + maintainer_keys: &Keys, + identifier: &str, + expected_commit: &str, +) { + let owner_found = wait_for_event_on_relay( + target_relay.url(), + Filter::new() + .kind(Kind::GitRepoAnnouncement) + .author(owner_keys.public_key()) + .identifier(identifier), + Duration::from_secs(20), + ) + .await; + assert!( + owner_found, + "Invitation acceptance announcement should leave purgatory after source recovery" + ); + + let maintainer_found = wait_for_event_on_relay( + target_relay.url(), + Filter::new() + .kind(Kind::GitRepoAnnouncement) + .author(maintainer_keys.public_key()) + .identifier(identifier), + Duration::from_secs(5), + ) + .await; + assert!( + maintainer_found, + "Target should sync the inviter announcement from the recovered source" + ); + + let state_found = wait_for_event_on_relay( + target_relay.url(), + Filter::new() + .kind(Kind::RepoState) + .author(maintainer_keys.public_key()) + .identifier(identifier), + Duration::from_secs(5), + ) + .await; + assert!( + state_found, + "Target should sync the inviter state needed to align git data" + ); + + let owner_npub = owner_keys + .public_key() + .to_bech32() + .expect("Failed to encode owner npub"); + let ref_aligned = crate::common::check_ref_at_commit( + &target_relay.domain(), + &owner_npub, + identifier, + "refs/heads/main", + expected_commit, + ) + .await + .expect("Failed to inspect accepted owner's ref"); + assert!( + ref_aligned, + "Accepted owner's repository should align to the inviter state" + ); +} + +/// An invitation may be accepted before its listed source contains any +/// announcements or state. Empty initial history must not make the target drop +/// that still-desired source before the inviter publishes its repository data. +#[tokio::test] +async fn test_empty_invitation_source_remains_live_until_inviter_data_arrives() { + let source_relay = TestRelay::start().await; + let target_relay = TestRelay::start_with_sync(None).await; + let owner_keys = Keys::generate(); + let maintainer_keys = Keys::generate(); + let identifier = "invitation-empty-source-recovery"; + + let acceptance = invitation_acceptance_announcement( + &target_relay, + source_relay.url(), + &owner_keys, + &maintainer_keys, + identifier, + ); + send_to_relay(&target_relay, &acceptance) + .await + .expect("Failed to publish invitation acceptance"); + + assert!( + wait_for_log( + &target_relay.log_path(), + "Historic sync complete - transitioned", + Duration::from_secs(10), + ) + .await, + "Target should complete the source relay's initially empty historic sync" + ); + + // The test relay checks for empty connections every second. Waiting beyond + // that interval reproduces the old cleanup path before publishing data. + tokio::time::sleep(Duration::from_secs(2)).await; + + let (_source_git, commit) = + seed_inviter_repository(&source_relay, &maintainer_keys, &owner_keys, identifier).await; + assert_invitation_sync_completed( + &target_relay, + &owner_keys, + &maintainer_keys, + identifier, + &commit, + ) + .await; + + target_relay.stop().await; + source_relay.stop().await; +} + +/// A listed invitation source can be offline when acceptance is published. +/// Once it starts, desired StateOnly work must make the target retry even +/// though no source repository has yet entered confirmed relay state. +#[tokio::test] +async fn test_unavailable_invitation_source_is_retried_after_startup() { + let source_reservation = crate::common::reserve_port(); + let source_url = format!("ws://127.0.0.1:{}", source_reservation.port()); + let target_relay = TestRelay::start_with_sync(None).await; + let owner_keys = Keys::generate(); + let maintainer_keys = Keys::generate(); + let identifier = "invitation-unavailable-source-retry"; + + let acceptance = invitation_acceptance_announcement( + &target_relay, + &source_url, + &owner_keys, + &maintainer_keys, + identifier, + ); + send_to_relay(&target_relay, &acceptance) + .await + .expect("Failed to publish invitation acceptance"); + + let source_failure = format!("Relay {source_url} degraded, backoff"); + assert!( + wait_for_log( + &target_relay.log_path(), + &source_failure, + Duration::from_secs(10), + ) + .await, + "Target should attempt the unavailable invitation source before it starts" + ); + + let source_relay = + TestRelay::start_on_reservation_with_options(source_reservation, None, false).await; + assert_eq!( + source_relay.url(), + source_url, + "Source must start at the URL embedded in the acceptance announcement" + ); + let (_source_git, commit) = + seed_inviter_repository(&source_relay, &maintainer_keys, &owner_keys, identifier).await; + + assert_invitation_sync_completed( + &target_relay, + &owner_keys, + &maintainer_keys, + identifier, + &commit, + ) + .await; + + target_relay.stop().await; + source_relay.stop().await; +} + /// A reciprocal owner announcement in purgatory must be enough to unlock an /// earlier rejected maintainer announcement and use that maintainer's clone URL. /// From da07b08def79c7f63b7b655fb93074471463e2b4 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 09:58:59 +0100 Subject: [PATCH 08/11] fix(sync): apply stored state when accepting invitations A real maintainership invitation starts with an existing repository, owner-authored state, and Git data. The invitee then publishes only a reciprocal announcement. On a shared GRASP server the owner state was already in the database, so duplicate handling returned before applying it to the invitee's newly created repository and the acceptance remained in purgatory. Reapply preferred stored states when a new maintainer announcement enters purgatory, and limit promotion to repositories authorized by that state with all required objects present. The three-server integration now proves the owner-only, invitee-only, and shared announcement paths while asserting that no invitee state event or post-acceptance Git push occurs. --- CHANGELOG.md | 3 + src/git/sync.rs | 18 +- src/nostr/builder.rs | 56 ++++ src/nostr/policy/state.rs | 36 +- tests/sync/maintainer_reprocessing.rs | 466 +++++++++++++------------- 5 files changed, 331 insertions(+), 248 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b99da6..5abb93f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Keep invitation relay connection ownership inside the bounded scheduler by disabling the SDK's independent auto-reconnect loop and cancelling queued or active connection workers when the sync manager shuts down. +- Fix invitation acceptance on a shared GRASP server by reapplying an existing + owner state event to the invitee's newly created repository, so GRASP-02 can + align it without an invitee state event or Git push. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/src/git/sync.rs b/src/git/sync.rs index ac729b9..29851a2 100644 --- a/src/git/sync.rs +++ b/src/git/sync.rs @@ -987,10 +987,12 @@ pub async fn process_newly_available_git_data( /// source repo to other authorized owner repos for the same identifier. Those /// target repos may now satisfy purgatory announcements even though no direct /// push or fetch happened for the target repo path. This helper runs the normal -/// newly-available-git-data pipeline for each populated target repo so promotion -/// behavior stays centralized in this module. +/// newly-available-git-data pipeline only for target repos authorized by the +/// preferred state and containing every required OID, so an unrelated or failed +/// copy cannot promote an empty announcement. pub async fn process_repos_populated_by_state_copy( source_repo_path: &Path, + state: &RepositoryState, repo_data: &RepositoryData, database: &SharedDatabase, local_relay: Option<&nostr_relay_builder::LocalRelay>, @@ -1000,6 +1002,7 @@ pub async fn process_repos_populated_by_state_copy( ) -> ProcessResult { let empty_oids: HashSet = HashSet::new(); let mut aggregate = ProcessResult::default(); + let maintainers_by_owner = collect_authorized_maintainers(&repo_data.announcements); for announcement in &repo_data.announcements { let target_repo_path = git_data_path.join(announcement.repo_path()); @@ -1007,6 +1010,17 @@ pub async fn process_repos_populated_by_state_copy( continue; } + let owner = announcement.event.pubkey.to_hex(); + let Some(maintainers) = maintainers_by_owner.get(&owner) else { + continue; + }; + if !maintainers.contains(&state.event.pubkey.to_hex()) + || !is_latest_authorized_state(state, maintainers, &repo_data.states) + || !can_apply_state(&state.event, &target_repo_path) + { + continue; + } + match process_newly_available_git_data( &target_repo_path, &empty_oids, diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index 5625580..d83b351 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -310,6 +310,13 @@ impl Nip34WritePolicy { // New announcement - add to purgatory match self.announcement_policy.add_to_purgatory(event) { Ok(()) => { + // The server may already have the repository's owner-authored state + // and Git objects. Reapply that stored state now so a newly accepted + // maintainer announcement can be promoted without waiting for the + // same replaceable state event to arrive over sync again. + self.reconcile_stored_state_events_for_identifier(&announcement.identifier) + .await; + tracing::info!( "Accepted announcement to purgatory: {} (waiting for git data)", event_id_str @@ -650,6 +657,55 @@ impl Nip34WritePolicy { } } + /// Reapply stored state after a new maintainer repository enters purgatory. + /// + /// A shared GRASP server can already hold the owner's latest state and Git + /// objects when an invitee publishes their reciprocal announcement. Since + /// the state is replaceable and already stored, sync may not deliver it + /// again. Processing stored candidates newest-first lets each maintainer + /// chain apply its preferred state before considering older candidates, + /// without requiring an invitee-authored state event or push. + async fn reconcile_stored_state_events_for_identifier(&self, identifier: &str) { + let filter = Filter::new().kind(Kind::RepoState).custom_tag( + SingleLetterTag::lowercase(Alphabet::D), + identifier.to_string(), + ); + let mut states: Vec = match self.ctx.database.query(filter).await { + Ok(events) => events.into_iter().collect(), + Err(error) => { + tracing::warn!( + identifier = %identifier, + error = %error, + "Failed to query stored state for new maintainer repository" + ); + return; + } + }; + + states.sort_by(|left, right| { + right + .created_at + .cmp(&left.created_at) + .then_with(|| left.id.cmp(&right.id)) + }); + + for state in states { + let promotion_hooks = NostrPurgatoryPromotionHooks::recovery_only(self); + if let Err(error) = self + .state_policy + .process_state_event(&state, true, Some(&promotion_hooks)) + .await + { + tracing::warn!( + identifier = %identifier, + event_id = %state.id, + error = %error, + "Failed to reapply stored state to new maintainer repository" + ); + } + } + } + /// Handle events that must reference accepted repositories or events async fn handle_related_event(&self, event: &Event, event_type: &str) -> WritePolicyResult { let event_id_str = event.id.to_bech32().unwrap_or_else(|_| event.id.to_hex()); diff --git a/src/nostr/policy/state.rs b/src/nostr/policy/state.rs index 1c07cfb..0296d61 100644 --- a/src/nostr/policy/state.rs +++ b/src/nostr/policy/state.rs @@ -172,11 +172,32 @@ impl StatePolicy { } } - // Duplicate check in db - if db_repo_data.states.iter().any(|e| e.event.id.eq(&event.id)) { + // A state already stored in the database may still need to be applied to + // a newly-created maintainer repository. This happens when an invitee + // publishes only their reciprocal announcement to a GRASP server that + // already serves the owner's state and Git data. Reconcile that state + // while the authorized invitee announcement is in purgatory instead of + // returning early as an ordinary duplicate. + let state_already_in_db = db_repo_data.states.iter().any(|e| e.event.id.eq(&event.id)); + let has_authorized_purgatory_announcement = authorized_owners.iter().any(|owner_hex| { + nostr_sdk::prelude::PublicKey::from_hex(owner_hex).is_ok_and(|owner| { + self.ctx + .purgatory + .has_purgatory_announcement(&owner, &state.identifier) + }) + }); + + if state_already_in_db && !has_authorized_purgatory_announcement { tracing::debug!("processed state event duplicate (in db): {}", event.id); return Ok(duplicate("already have this event")); } + if state_already_in_db { + tracing::info!( + event_id = %event.id, + identifier = %state.identifier, + "Reapplying stored state to a new maintainer repository" + ); + } // Check if git data is available if let Some(repo_with_git_data) = @@ -221,6 +242,7 @@ impl StatePolicy { let local_relay = self.ctx.get_local_relay(); let promotion_result = crate::git::sync::process_repos_populated_by_state_copy( &repo_with_git_data, + &state, &db_repo_data, &self.ctx.database, local_relay.as_ref(), @@ -239,8 +261,14 @@ impl StatePolicy { ); } - // Event will be saved and broadcast by relay builder - Ok(WritePolicyResult::Accept) + if state_already_in_db { + Ok(duplicate( + "already stored; reapplied to pending maintainer repository", + )) + } else { + // Event will be saved and broadcast by relay builder + Ok(WritePolicyResult::Accept) + } } else { // Only reject expired events if they're from sync (not user-submitted) // User-submitted events should be allowed to retry in case git data became available diff --git a/tests/sync/maintainer_reprocessing.rs b/tests/sync/maintainer_reprocessing.rs index 45b2430..ce881fb 100644 --- a/tests/sync/maintainer_reprocessing.rs +++ b/tests/sync/maintainer_reprocessing.rs @@ -15,9 +15,12 @@ //! → hot copy is re-processed immediately when available //! → expired hot copy is fetched by its retained cold-index event ID //! → maintainer announcement supplies the source clone and git data syncs -//! reciprocal owner announcement reaches relay_b before relay_a is ready -//! → an empty relay_a remains live for later repository events -//! → an unavailable relay_a remains desired and is retried after startup +//! an existing owner publishes an invitation for a new maintainer +//! → the invitee publishes only their reciprocal announcement +//! → GRASP-02 distributes both announcements across owner-only, invitee-only, +//! and shared servers +//! → the owner's state and Git data align the invitee repositories without +//! an invitee state event or Git push //! //! To guarantee the maintainer announcements arrive at relay_b *before* the owner //! git push, relay_b is started with relay_a as its bootstrap relay. That way @@ -47,283 +50,262 @@ async fn wait_for_log(path: &Path, needle: &str, timeout: Duration) -> bool { } } -fn invitation_acceptance_announcement( - target_relay: &TestRelay, - source_relay_url: &str, - owner_keys: &Keys, - maintainer_keys: &Keys, +fn repository_urls( + keys: &Keys, + relays: &[&TestRelay], identifier: &str, -) -> Event { - let owner_npub = owner_keys +) -> (Vec, Vec) { + let npub = keys .public_key() .to_bech32() - .expect("Failed to encode owner npub"); - EventBuilder::new(Kind::GitRepoAnnouncement, "Accepted maintainer invitation") - .tags(vec![ - Tag::identifier(identifier), - Tag::custom( - "clone", - vec![format!( - "http://{}/{}/{}.git", - target_relay.domain(), - owner_npub, - identifier - )], - ), - Tag::custom( - "relays", - vec![source_relay_url.to_string(), target_relay.url().to_string()], - ), - Tag::custom("maintainers", vec![maintainer_keys.public_key().to_hex()]), - ]) - .finalize(owner_keys) - .expect("Failed to create invitation acceptance announcement") + .expect("Failed to encode repository owner npub"); + let clone_urls = relays + .iter() + .map(|relay| format!("http://{}/{}/{}.git", relay.domain(), npub, identifier)) + .collect(); + let relay_urls = relays.iter().map(|relay| relay.url().to_string()).collect(); + (clone_urls, relay_urls) } -async fn seed_inviter_repository( - source_relay: &TestRelay, - maintainer_keys: &Keys, - invited_owner_keys: &Keys, +fn repository_announcement( + keys: &Keys, + relays: &[&TestRelay], + maintainers: &[PublicKey], identifier: &str, -) -> (tempfile::TempDir, String) { - let maintainer_npub = maintainer_keys - .public_key() - .to_bech32() - .expect("Failed to encode maintainer npub"); - let announcement = - EventBuilder::new(Kind::GitRepoAnnouncement, "Inviting maintainer repository") - .tags(vec![ - Tag::identifier(identifier), - Tag::custom( - "clone", - vec![format!( - "http://{}/{}/{}.git", - source_relay.domain(), - maintainer_npub, - identifier - )], - ), - Tag::custom("relays", vec![source_relay.url().to_string()]), - Tag::custom( - "maintainers", - vec![invited_owner_keys.public_key().to_hex()], - ), - ]) - .finalize(maintainer_keys) - .expect("Failed to create inviter announcement"); - - send_to_relay(source_relay, &announcement) - .await - .expect("Failed to send inviter announcement"); - let git_dir = push_git_data_to_relay( - source_relay, - maintainer_keys, - identifier, - &[&source_relay.domain()], - ) - .await; - let output = Command::new("git") - .args(["rev-parse", "HEAD"]) - .current_dir(git_dir.path()) - .output() - .expect("Failed to read inviter commit"); - assert!(output.status.success(), "Failed to resolve inviter commit"); - let commit = String::from_utf8(output.stdout) - .expect("Inviter commit should be UTF-8") - .trim() - .to_string(); - - let announcement_found = wait_for_event_on_relay( - source_relay.url(), - Filter::new() - .kind(Kind::GitRepoAnnouncement) - .author(maintainer_keys.public_key()) - .identifier(identifier), - Duration::from_secs(5), - ) - .await; - assert!( - announcement_found, - "Inviter announcement should be available before target assertions" - ); - - (git_dir, commit) +) -> EventBuilder { + let (clone_urls, relay_urls) = repository_urls(keys, relays, identifier); + let mut tags = vec![ + Tag::identifier(identifier), + Tag::custom("clone", clone_urls), + Tag::custom("relays", relay_urls), + ]; + if !maintainers.is_empty() { + tags.push(Tag::custom( + "maintainers", + maintainers + .iter() + .map(PublicKey::to_hex) + .collect::>(), + )); + } + EventBuilder::new(Kind::GitRepoAnnouncement, "Repository announcement").tags(tags) } -async fn assert_invitation_sync_completed( - target_relay: &TestRelay, - owner_keys: &Keys, - maintainer_keys: &Keys, - identifier: &str, - expected_commit: &str, -) { - let owner_found = wait_for_event_on_relay( - target_relay.url(), - Filter::new() - .kind(Kind::GitRepoAnnouncement) - .author(owner_keys.public_key()) - .identifier(identifier), +async fn assert_exact_event_served(relay: &TestRelay, event: &Event, description: &str) { + let found = wait_for_event_on_relay( + relay.url(), + Filter::new().id(event.id), Duration::from_secs(20), ) .await; assert!( - owner_found, - "Invitation acceptance announcement should leave purgatory after source recovery" + found, + "{description} should be served by {}", + relay.domain() ); +} - let maintainer_found = wait_for_event_on_relay( - target_relay.url(), - Filter::new() - .kind(Kind::GitRepoAnnouncement) - .author(maintainer_keys.public_key()) - .identifier(identifier), - Duration::from_secs(5), - ) - .await; - assert!( - maintainer_found, - "Target should sync the inviter announcement from the recovered source" - ); +async fn push_counts(relay: &TestRelay) -> (u64, u64) { + let text = fetch_metrics(relay.url()) + .await + .expect("Failed to fetch relay metrics"); + let metrics = ParsedMetrics::parse(&text); + let success = metrics + .counter( + "ngit_git_operations_total", + &[("operation", "push"), ("status", "success")], + ) + .unwrap_or(0); + let error = metrics + .counter( + "ngit_git_operations_total", + &[("operation", "push"), ("status", "error")], + ) + .unwrap_or(0); + (success, error) +} - let state_found = wait_for_event_on_relay( - target_relay.url(), - Filter::new() - .kind(Kind::RepoState) - .author(maintainer_keys.public_key()) - .identifier(identifier), - Duration::from_secs(5), - ) - .await; - assert!( - state_found, - "Target should sync the inviter state needed to align git data" - ); +/// Exercise the production invitation flow for a repository that already has +/// owner state and Git data. +/// +/// The owner selects an owner-only server and a server shared with the invitee. +/// The invitee selects an invitee-only server and that shared server. Acceptance +/// publishes only the invitee's reciprocal announcement: no invitee state event +/// is created and the invitee never pushes Git. GRASP-02 must distribute the +/// signed announcements and use the owner's state and Git data to create both +/// invitee repositories. +#[tokio::test] +async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_push() { + use crate::common::{create_state_event, create_test_repo_with_commit, CommitVariant}; + let owner_relay = TestRelay::start_with_sync(None).await; + let invitee_relay = TestRelay::start_with_sync(None).await; + let shared_relay = TestRelay::start_with_sync(None).await; + let owner_keys = Keys::generate(); + let invitee_keys = Keys::generate(); + let identifier = "existing-repository-invitation-acceptance"; + + let owner_git = tempfile::tempdir().expect("Failed to create owner repository directory"); + let commit = create_test_repo_with_commit(owner_git.path(), CommitVariant::StateTest) + .expect("Failed to create owner repository commit"); let owner_npub = owner_keys .public_key() .to_bech32() .expect("Failed to encode owner npub"); - let ref_aligned = crate::common::check_ref_at_commit( - &target_relay.domain(), + let owner_servers = [&owner_relay, &shared_relay]; + let (owner_clone_urls, owner_relay_urls) = + repository_urls(&owner_keys, &owner_servers, identifier); + let initial_announcement = + repository_announcement(&owner_keys, &owner_servers, &[], identifier) + .finalize(&owner_keys) + .expect("Failed to create initial owner announcement"); + let owner_state = create_state_event( + &owner_keys, + identifier, + &[("main", &commit)], + &[], + &owner_clone_urls + .iter() + .map(String::as_str) + .collect::>(), + &owner_relay_urls + .iter() + .map(String::as_str) + .collect::>(), + ) + .expect("Failed to create owner state event"); + + for relay in owner_servers { + send_to_relay(relay, &initial_announcement) + .await + .expect("Failed to publish initial owner announcement"); + send_to_relay(relay, &owner_state) + .await + .expect("Failed to publish owner state"); + } + crate::common::push_to_relay( + owner_git.path(), + &owner_relay.domain(), &owner_npub, identifier, - "refs/heads/main", - expected_commit, ) - .await - .expect("Failed to inspect accepted owner's ref"); - assert!( - ref_aligned, - "Accepted owner's repository should align to the inviter state" - ); -} + .expect("Failed to push existing owner repository"); -/// An invitation may be accepted before its listed source contains any -/// announcements or state. Empty initial history must not make the target drop -/// that still-desired source before the inviter publishes its repository data. -#[tokio::test] -async fn test_empty_invitation_source_remains_live_until_inviter_data_arrives() { - let source_relay = TestRelay::start().await; - let target_relay = TestRelay::start_with_sync(None).await; - let owner_keys = Keys::generate(); - let maintainer_keys = Keys::generate(); - let identifier = "invitation-empty-source-recovery"; - - let acceptance = invitation_acceptance_announcement( - &target_relay, - source_relay.url(), - &owner_keys, - &maintainer_keys, - identifier, - ); - send_to_relay(&target_relay, &acceptance) - .await - .expect("Failed to publish invitation acceptance"); - - assert!( - wait_for_log( - &target_relay.log_path(), - "Historic sync complete - transitioned", - Duration::from_secs(10), + for relay in owner_servers { + assert_exact_event_served(relay, &initial_announcement, "Initial owner announcement").await; + assert_exact_event_served(relay, &owner_state, "Owner state event").await; + let owner_ref_aligned = crate::common::check_ref_at_commit( + &relay.domain(), + &owner_npub, + identifier, + "refs/heads/main", + &commit, ) - .await, - "Target should complete the source relay's initially empty historic sync" - ); + .await + .expect("Failed to inspect owner repository ref"); + assert!( + owner_ref_aligned, + "Owner repository should be aligned on {}", + relay.domain() + ); + } - // The test relay checks for empty connections every second. Waiting beyond - // that interval reproduces the old cleanup path before publishing data. - tokio::time::sleep(Duration::from_secs(2)).await; - - let (_source_git, commit) = - seed_inviter_repository(&source_relay, &maintainer_keys, &owner_keys, identifier).await; - assert_invitation_sync_completed( - &target_relay, + let invitation = repository_announcement( &owner_keys, - &maintainer_keys, + &owner_servers, + &[invitee_keys.public_key()], identifier, - &commit, ) - .await; + .custom_created_at(Timestamp::from_secs( + initial_announcement.created_at.as_secs() + 1, + )) + .finalize(&owner_keys) + .expect("Failed to create owner invitation announcement"); + for relay in owner_servers { + send_to_relay(relay, &invitation) + .await + .expect("Failed to publish owner invitation"); + assert_exact_event_served(relay, &invitation, "Owner invitation announcement").await; + } - target_relay.stop().await; - source_relay.stop().await; -} - -/// A listed invitation source can be offline when acceptance is published. -/// Once it starts, desired StateOnly work must make the target retry even -/// though no source repository has yet entered confirmed relay state. -#[tokio::test] -async fn test_unavailable_invitation_source_is_retried_after_startup() { - let source_reservation = crate::common::reserve_port(); - let source_url = format!("ws://127.0.0.1:{}", source_reservation.port()); - let target_relay = TestRelay::start_with_sync(None).await; - let owner_keys = Keys::generate(); - let maintainer_keys = Keys::generate(); - let identifier = "invitation-unavailable-source-retry"; - - let acceptance = invitation_acceptance_announcement( - &target_relay, - &source_url, - &owner_keys, - &maintainer_keys, + let pushes_before_acceptance = [ + push_counts(&owner_relay).await, + push_counts(&invitee_relay).await, + push_counts(&shared_relay).await, + ]; + let invitee_servers = [&invitee_relay, &shared_relay]; + let acceptance = repository_announcement( + &invitee_keys, + &invitee_servers, + &[owner_keys.public_key()], identifier, - ); - send_to_relay(&target_relay, &acceptance) - .await - .expect("Failed to publish invitation acceptance"); + ) + .finalize(&invitee_keys) + .expect("Failed to create invitee acceptance announcement"); + for relay in invitee_servers { + send_to_relay(relay, &acceptance) + .await + .expect("Failed to publish invitee acceptance announcement"); + } - let source_failure = format!("Relay {source_url} degraded, backoff"); - assert!( - wait_for_log( - &target_relay.log_path(), - &source_failure, - Duration::from_secs(10), + for relay in [&owner_relay, &invitee_relay, &shared_relay] { + assert_exact_event_served(relay, &invitation, "Owner invitation announcement").await; + assert_exact_event_served(relay, &owner_state, "Owner state event").await; + } + for relay in invitee_servers { + assert_exact_event_served(relay, &acceptance, "Invitee acceptance announcement").await; + } + for relay in [&owner_relay, &invitee_relay, &shared_relay] { + let invitee_state_exists = wait_for_event_on_relay( + relay.url(), + Filter::new() + .kind(Kind::RepoState) + .author(invitee_keys.public_key()) + .identifier(identifier), + Duration::from_secs(1), ) - .await, - "Target should attempt the unavailable invitation source before it starts" - ); + .await; + assert!( + !invitee_state_exists, + "Invitee must not issue a state event served by {}", + relay.domain() + ); + } - let source_relay = - TestRelay::start_on_reservation_with_options(source_reservation, None, false).await; + let pushes_after_acceptance = [ + push_counts(&owner_relay).await, + push_counts(&invitee_relay).await, + push_counts(&shared_relay).await, + ]; assert_eq!( - source_relay.url(), - source_url, - "Source must start at the URL embedded in the acceptance announcement" + pushes_after_acceptance, pushes_before_acceptance, + "Invitation acceptance must not perform or attempt a client Git push" ); - let (_source_git, commit) = - seed_inviter_repository(&source_relay, &maintainer_keys, &owner_keys, identifier).await; - assert_invitation_sync_completed( - &target_relay, - &owner_keys, - &maintainer_keys, - identifier, - &commit, - ) - .await; + let invitee_npub = invitee_keys + .public_key() + .to_bech32() + .expect("Failed to encode invitee npub"); + for relay in invitee_servers { + let ref_aligned = crate::common::check_ref_at_commit( + &relay.domain(), + &invitee_npub, + identifier, + "refs/heads/main", + &commit, + ) + .await + .expect("Failed to inspect invitee repository ref"); + assert!( + ref_aligned, + "GRASP-02 should align the invitee repository on {} without an invitee push", + relay.domain() + ); + } - target_relay.stop().await; - source_relay.stop().await; + shared_relay.stop().await; + invitee_relay.stop().await; + owner_relay.stop().await; } /// A reciprocal owner announcement in purgatory must be enough to unlock an From ab7f09e6e191738f62559095f043418b46ecc07d Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 10:14:50 +0100 Subject: [PATCH 09/11] test(sync): cover non-overlapping invitation servers An owner and invitee can select disjoint GRASP servers while sharing only one bridge. The acceptance path must propagate both signed announcements and the owner's state through that bridge without an invitee state event or Git push. Exercise an owner-only, invitee-only, and shared relay; require every relay to serve the exact events and every clone URL signed into either announcement to expose the complete state ref set. --- tests/sync/maintainer_reprocessing.rs | 116 +++++++++++++++++++------- 1 file changed, 84 insertions(+), 32 deletions(-) diff --git a/tests/sync/maintainer_reprocessing.rs b/tests/sync/maintainer_reprocessing.rs index ce881fb..45383ff 100644 --- a/tests/sync/maintainer_reprocessing.rs +++ b/tests/sync/maintainer_reprocessing.rs @@ -29,6 +29,7 @@ //! announcements are in relay_a's DB), wait briefly for the sync round-trip, then //! send the owner announcement + git push. +use std::collections::BTreeMap; use std::path::Path; use std::process::Command; use std::time::Duration; @@ -125,6 +126,63 @@ async fn push_counts(relay: &TestRelay) -> (u64, u64) { (success, error) } +fn list_remote_refs(clone_url: &str) -> Result, String> { + let output = Command::new("git") + .args(["ls-remote", "--refs", clone_url]) + .output() + .map_err(|error| format!("Failed to list refs from {clone_url}: {error}"))?; + if !output.status.success() { + return Err(format!( + "Failed to list refs from {clone_url}: {}", + String::from_utf8_lossy(&output.stderr) + )); + } + + String::from_utf8(output.stdout) + .map_err(|error| format!("Invalid UTF-8 from {clone_url}: {error}"))? + .lines() + .map(|line| { + let (commit, name) = line + .split_once('\t') + .ok_or_else(|| format!("Invalid ls-remote line from {clone_url}: {line}"))?; + Ok((name.to_string(), commit.to_string())) + }) + .collect() +} + +fn announcement_clone_urls(event: &Event) -> Vec { + event + .tags + .iter() + .find(|tag| tag.kind() == "clone") + .expect("Repository announcement should have a clone tag") + .clone() + .to_vec() + .into_iter() + .skip(1) + .collect() +} + +async fn assert_remote_refs( + clone_url: &str, + expected: &BTreeMap, + timeout: Duration, +) { + let deadline = tokio::time::Instant::now() + timeout; + loop { + let actual = list_remote_refs(clone_url); + if actual.as_ref().is_ok_and(|refs| refs == expected) { + return; + } + assert!( + tokio::time::Instant::now() < deadline, + "Announced Git endpoint should exactly match the owner state: {clone_url}\n\ + expected: {expected:?}\nactual: {actual:?}" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } +} + /// Exercise the production invitation flow for a repository that already has /// owner state and Git data. /// @@ -182,16 +240,13 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu send_to_relay(relay, &owner_state) .await .expect("Failed to publish owner state"); - } - crate::common::push_to_relay( - owner_git.path(), - &owner_relay.domain(), - &owner_npub, - identifier, - ) - .expect("Failed to push existing owner repository"); - - for relay in owner_servers { + crate::common::push_to_relay( + owner_git.path(), + &relay.domain(), + &owner_npub, + identifier, + ) + .expect("Failed to push existing owner repository"); assert_exact_event_served(relay, &initial_announcement, "Initial owner announcement").await; assert_exact_event_served(relay, &owner_state, "Owner state event").await; let owner_ref_aligned = crate::common::check_ref_at_commit( @@ -242,6 +297,7 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu ) .finalize(&invitee_keys) .expect("Failed to create invitee acceptance announcement"); + let (invitee_clone_urls, _) = repository_urls(&invitee_keys, &invitee_servers, identifier); for relay in invitee_servers { send_to_relay(relay, &acceptance) .await @@ -250,10 +306,8 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu for relay in [&owner_relay, &invitee_relay, &shared_relay] { assert_exact_event_served(relay, &invitation, "Owner invitation announcement").await; - assert_exact_event_served(relay, &owner_state, "Owner state event").await; - } - for relay in invitee_servers { assert_exact_event_served(relay, &acceptance, "Invitee acceptance announcement").await; + assert_exact_event_served(relay, &owner_state, "Owner state event").await; } for relay in [&owner_relay, &invitee_relay, &shared_relay] { let invitee_state_exists = wait_for_event_on_relay( @@ -282,25 +336,23 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu "Invitation acceptance must not perform or attempt a client Git push" ); - let invitee_npub = invitee_keys - .public_key() - .to_bech32() - .expect("Failed to encode invitee npub"); - for relay in invitee_servers { - let ref_aligned = crate::common::check_ref_at_commit( - &relay.domain(), - &invitee_npub, - identifier, - "refs/heads/main", - &commit, - ) - .await - .expect("Failed to inspect invitee repository ref"); - assert!( - ref_aligned, - "GRASP-02 should align the invitee repository on {} without an invitee push", - relay.domain() - ); + let expected_refs = BTreeMap::from([ + ("refs/heads/main".to_string(), commit.clone()), + ]); + let announced_owner_clones = announcement_clone_urls(&invitation); + let announced_invitee_clones = announcement_clone_urls(&acceptance); + assert_eq!(announced_owner_clones, owner_clone_urls); + assert_eq!(announced_invitee_clones, invitee_clone_urls); + assert_eq!( + announced_owner_clones.len() + announced_invitee_clones.len(), + 4, + "Owner and invitee announcements should advertise two Git endpoints each" + ); + for clone_url in announced_owner_clones + .iter() + .chain(announced_invitee_clones.iter()) + { + assert_remote_refs(clone_url, &expected_refs, Duration::from_secs(20)).await; } shared_relay.stop().await; From 28c130fe9a096d00ed676cd9fe99d52dca21b4c8 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 10:41:38 +0100 Subject: [PATCH 10/11] fix(sync): reconcile invitations with existing repositories An invitee can already own the same repository identifier with an older, conflicting state and default branch. A unilateral invitation must leave that repository untouched, while reciprocal acceptance must make the newer owner state authoritative across the combined maintainer set. Route maintainer-changing replacements through purgatory, overlay them on served coordinates during authorization, and make direct promotions retry rejected owner announcements and states. The three-server integration proves the pre-acceptance isolation and post-acceptance event, ref, HEAD, and no-push convergence. --- CHANGELOG.md | 3 + src/git/authorization.rs | 9 +- src/git/sync.rs | 16 +- src/nostr/builder.rs | 25 +- src/nostr/policy/announcement.rs | 68 ++++-- src/nostr/policy/state.rs | 1 - src/purgatory/promotion_hooks.rs | 95 +++++++- src/server.rs | 3 + tests/sync/maintainer_reprocessing.rs | 322 +++++++++++++++++++++++++- 9 files changed, 494 insertions(+), 48 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5abb93f..f2f5ae1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix invitation acceptance on a shared GRASP server by reapplying an existing owner state event to the invitee's newly created repository, so GRASP-02 can align it without an invitee state event or Git push. +- Fix invitation acceptance when the invitee already owns the same repository + identifier by routing maintainer-changing replacements through purgatory and + reprocessing the owner dependencies that align the existing Git repository. - Sideband-aware Git clients now receive periodic progress while GRASP performs post-push purgatory promotion and cross-owner repository alignment, preventing the client I/O timeout from expiring during unusually complex finalization. - Smart HTTP pushes now expose the terminal receive-pack flush only after GRASP has finished promoting the matching repository announcement and state from purgatory. Git progress remains streamed while large packs are resolved and checked, but an immediately following clone or proposal push can now rely on a completed push being queryable on the relay. - Batch the one-time deletion-request lifecycle migration so large production databases do not remain unavailable while LMDB commits every historical request in separate transactions. diff --git a/src/git/authorization.rs b/src/git/authorization.rs index f433c14..e59f614 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -280,7 +280,14 @@ pub async fn fetch_repository_data_with_purgatory( for entry in purgatory_announcements { if let Ok(announcement) = RepositoryAnnouncement::from_event(entry.event) { - repo_data.announcements.push(announcement); + if let Some(current) = repo_data.announcements.iter_mut().find(|current| { + current.event.pubkey == announcement.event.pubkey + && current.identifier == announcement.identifier + }) { + *current = announcement; + } else { + repo_data.announcements.push(announcement); + } } } diff --git a/src/git/sync.rs b/src/git/sync.rs index 29851a2..12ffbc1 100644 --- a/src/git/sync.rs +++ b/src/git/sync.rs @@ -981,17 +981,18 @@ pub async fn process_newly_available_git_data( Ok(result) } -/// Process purgatory promotions for owner repos populated by state-event OID copies. +/// Process purgatory promotions for owner repos aligned by state processing. /// /// [`crate::git::process::process_state_with_git_data`] copies OIDs from the /// source repo to other authorized owner repos for the same identifier. Those /// target repos may now satisfy purgatory announcements even though no direct -/// push or fetch happened for the target repo path. This helper runs the normal -/// newly-available-git-data pipeline only for target repos authorized by the -/// preferred state and containing every required OID, so an unrelated or failed -/// copy cannot promote an empty announcement. +/// push or fetch happened for the target repo path. The source repo can also +/// have a maintainer-topology replacement waiting in purgatory while its +/// existing Git data is re-evaluated. This helper runs the normal +/// newly-available-git-data pipeline only for repos authorized by the preferred +/// state and containing every required OID, so an unrelated or failed copy +/// cannot promote an empty announcement. pub async fn process_repos_populated_by_state_copy( - source_repo_path: &Path, state: &RepositoryState, repo_data: &RepositoryData, database: &SharedDatabase, @@ -1006,9 +1007,6 @@ pub async fn process_repos_populated_by_state_copy( for announcement in &repo_data.announcements { let target_repo_path = git_data_path.join(announcement.repo_path()); - if target_repo_path == source_repo_path { - continue; - } let owner = announcement.event.pubkey.to_hex(); let Some(maintainers) = maintainers_by_owner.get(&owner) else { diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index d83b351..bc6093b 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -7,7 +7,7 @@ use std::net::SocketAddr; use std::num::NonZeroUsize; use std::path::Path; -use std::sync::Arc; +use std::sync::{Arc, RwLock}; use anyhow::Result; use nostr::nips::nip19::ToBech32; @@ -29,6 +29,7 @@ use crate::nostr::policy::{ }; use crate::nostr::SharedDatabase; use crate::purgatory::promotion_hooks::NostrPurgatoryPromotionHooks; +use crate::sync::rejected_index::RejectedEventsIndex; /// NIP-34 Write Policy — admission and routing for GRASP-01 events /// @@ -53,6 +54,7 @@ pub struct Nip34WritePolicy { pr_event_policy: PrEventPolicy, related_event_policy: RelatedEventPolicy, deletion: DeletionService, + rejected_events_index: Arc>>>, } impl std::fmt::Debug for Nip34WritePolicy { @@ -100,6 +102,7 @@ impl Nip34WritePolicy { pr_event_policy: PrEventPolicy::new(ctx.clone(), repo_init_locks), related_event_policy: RelatedEventPolicy::new(ctx.clone()), deletion: DeletionService::new(deletion_ctx), + rejected_events_index: Arc::new(RwLock::new(None)), ctx, } } @@ -144,6 +147,26 @@ impl Nip34WritePolicy { self.ctx.set_local_relay(relay); } + /// Make sync's rejected-event dependencies available to direct-write + /// purgatory promotions. + pub fn set_rejected_events_index(&self, index: Arc) { + *self + .rejected_events_index + .write() + .expect("rejected events index lock poisoned") = Some(index); + } + + pub(crate) fn rejected_events_index(&self) -> Option> { + self.rejected_events_index + .read() + .expect("rejected events index lock poisoned") + .clone() + } + + pub(crate) fn local_relay(&self) -> Option { + self.ctx.get_local_relay() + } + /// Access the deletion subsystem directly. /// /// Exposes startup passes and recovery without requiring delegation wrappers diff --git a/src/nostr/policy/announcement.rs b/src/nostr/policy/announcement.rs index 43b72a0..d6e628e 100644 --- a/src/nostr/policy/announcement.rs +++ b/src/nostr/policy/announcement.rs @@ -78,16 +78,28 @@ impl AnnouncementPolicy { // Owner de-list replacement for an already served repo: // do NOT allow maintainer-exception acceptance to bypass - // de-list enforcement. Let the caller treat this as a - // reject path so runtime deletion parity can run. + // de-list enforcement when the previous announcement + // actually listed this service. An external maintainer + // announcement never listed this service, so its + // replacement must remain eligible for the maintainer + // exception instead of being mistaken for a de-list. let lists_service = announcement.lists_service(&self.config.domain); if !lists_service { match self - .has_db_announcement(&event.pubkey, &announcement.identifier) + .db_announcement(&event.pubkey, &announcement.identifier) .await { - Ok(true) => return AnnouncementResult::Reject(reason), - Ok(false) => {} + Ok(Some(current_event)) => { + let current = + match RepositoryAnnouncement::from_event(current_event) { + Ok(current) => current, + Err(_) => return AnnouncementResult::Reject(reason), + }; + if current.lists_service(&self.config.domain) { + return AnnouncementResult::Reject(reason); + } + } + Ok(None) => {} Err(_) => return AnnouncementResult::Reject(reason), } } @@ -114,11 +126,11 @@ impl AnnouncementPolicy { // Parse announcement to check for existing active announcement match RepositoryAnnouncement::from_event(event.clone()) { Ok(announcement) => { - let in_db = match self - .has_db_announcement(&event.pubkey, &announcement.identifier) + let db_announcement = match self + .db_announcement(&event.pubkey, &announcement.identifier) .await { - Ok(v) => v, + Ok(event) => event, Err(e) => { tracing::warn!( error = %e, @@ -131,7 +143,30 @@ impl AnnouncementPolicy { } }; - if in_db { + if let Some(current_event) = db_announcement { + let current = match RepositoryAnnouncement::from_event(current_event) { + Ok(current) => current, + Err(e) => { + return AnnouncementResult::Reject(format!( + "Failed to parse stored announcement: {e}" + )) + } + }; + let current_maintainers: HashSet<&str> = + current.maintainers.iter().map(String::as_str).collect(); + let incoming_maintainers: HashSet<&str> = announcement + .maintainers + .iter() + .map(String::as_str) + .collect(); + if current_maintainers != incoming_maintainers { + tracing::debug!( + identifier = %announcement.identifier, + "Replacement announcement changes maintainers - routing through purgatory" + ); + return AnnouncementResult::AcceptPurgatory; + } + // Replacement announcement with DB entry - accept immediately tracing::debug!( identifier = %announcement.identifier, @@ -272,15 +307,12 @@ impl AnnouncementPolicy { ); } - /// Check if there's an announcement in the database for this (pubkey, identifier). - /// - /// Only checks the database (promoted announcements). For purgatory checks use - /// `purgatory.has_purgatory_announcement()` directly. - async fn has_db_announcement( + /// Return the NIP-01-preferred served announcement for this coordinate. + async fn db_announcement( &self, pubkey: &PublicKey, identifier: &str, - ) -> Result { + ) -> Result, String> { let filter = Filter::new() .kind(Kind::GitRepoAnnouncement) .author(*pubkey) @@ -294,7 +326,11 @@ impl AnnouncementPolicy { Err(e) => return Err(format!("Database query failed: {}", e)), }; - Ok(!events.is_empty()) + Ok(events.into_iter().max_by(|left, right| { + left.created_at + .cmp(&right.created_at) + .then_with(|| right.id.cmp(&left.id)) + })) } /// Add an announcement to purgatory diff --git a/src/nostr/policy/state.rs b/src/nostr/policy/state.rs index 0296d61..988e2e4 100644 --- a/src/nostr/policy/state.rs +++ b/src/nostr/policy/state.rs @@ -241,7 +241,6 @@ impl StatePolicy { // for repos that became populated by the copy. let local_relay = self.ctx.get_local_relay(); let promotion_result = crate::git::sync::process_repos_populated_by_state_copy( - &repo_with_git_data, &state, &db_repo_data, &self.ctx.database, diff --git a/src/purgatory/promotion_hooks.rs b/src/purgatory/promotion_hooks.rs index b7d296f..6de9b98 100644 --- a/src/purgatory/promotion_hooks.rs +++ b/src/purgatory/promotion_hooks.rs @@ -29,13 +29,13 @@ pub struct NostrPurgatoryPromotionHooks { } impl NostrPurgatoryPromotionHooks { - /// Run deletion recovery and accepted-event persistence hooks only. + /// Run deletion recovery and any configured rejected-event dependency hooks. pub fn recovery_only(write_policy: &Nip34WritePolicy) -> Self { Self { deletion: write_policy.deletion().clone(), write_policy: Some(Arc::new(write_policy.clone())), - rejected_events_index: None, - local_relay: None, + rejected_events_index: write_policy.rejected_events_index(), + local_relay: write_policy.local_relay(), } } @@ -52,6 +52,68 @@ impl NostrPurgatoryPromotionHooks { local_relay, } } + + async fn reprocess_state_dependencies( + &self, + write_policy: &Nip34WritePolicy, + rejected_events_index: &RejectedEventsIndex, + relay: &LocalRelay, + author: &PublicKey, + identifier: &str, + ) { + let (event_ids, hot_events) = + rejected_events_index.dependency_candidates(author, identifier, Some(EventType::State)); + if !event_ids.is_empty() { + info!( + pubkey = %author, + identifier = %identifier, + dependency_event_count = event_ids.len(), + hot_cache_events = hot_events.len(), + "Found rejected state dependencies after purgatory announcement promotion" + ); + } + + let dummy_addr = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0); + for state in hot_events { + info!( + event_id = %state.id, + pubkey = %author, + identifier = %identifier, + "Re-processing state event after purgatory announcement promotion" + ); + + match write_policy.admit_event(&state, &dummy_addr).await { + WritePolicyResult::Accept => { + match write_policy + .save_accepted_event(&state, SaveContext::HotCacheReprocess) + .await + { + Ok(_) => { + rejected_events_index.remove(&state.id); + relay.notify_event(state.clone()); + } + Err(e) => { + warn!( + event_id = %state.id, + error = %e, + "Failed to save re-processed state event" + ); + } + } + } + WritePolicyResult::Reject { status: true, .. } => { + rejected_events_index.remove(&state.id); + write_policy.purgatory().enqueue_sync_immediate(identifier); + } + _ => { + warn!( + event_id = %state.id, + "State event still rejected after announcement promotion" + ); + } + } + } + } } #[async_trait] @@ -88,15 +150,11 @@ impl PurgatoryPromotionHooks for NostrPurgatoryPromotionHooks { return; }; - if announcement.maintainers.is_empty() { - return; - } - debug!( identifier = %announcement.identifier, event_id = %event.id, maintainer_count = announcement.maintainers.len(), - "Owner announcement promoted via git push, checking hot cache for rejected maintainer announcements" + "Owner announcement promoted from purgatory, checking hot cache for rejected maintainer announcements" ); for maintainer_hex in &announcement.maintainers { @@ -114,7 +172,7 @@ impl PurgatoryPromotionHooks for NostrPurgatoryPromotionHooks { identifier = %announcement.identifier, dependency_event_count = event_ids.len(), hot_cache_events = hot_events.len(), - "Found rejected maintainer dependencies after git push promotion" + "Found rejected maintainer dependencies after purgatory promotion" ); } @@ -124,7 +182,7 @@ impl PurgatoryPromotionHooks for NostrPurgatoryPromotionHooks { event_id = %hot_event.id, maintainer = %maintainer_hex, identifier = %announcement.identifier, - "Re-processing maintainer announcement from hot cache after git push promotion" + "Re-processing maintainer announcement from hot cache after purgatory promotion" ); match write_policy.admit_event(&hot_event, &dummy_addr).await { @@ -136,6 +194,14 @@ impl PurgatoryPromotionHooks for NostrPurgatoryPromotionHooks { Ok(_) => { rejected_events_index.remove(&hot_event.id); relay.notify_event(hot_event.clone()); + self.reprocess_state_dependencies( + write_policy, + rejected_events_index, + relay, + &hot_event.pubkey, + &announcement.identifier, + ) + .await; info!( event_id = %hot_event.id, "Maintainer announcement accepted and saved on re-processing" @@ -168,5 +234,14 @@ impl PurgatoryPromotionHooks for NostrPurgatoryPromotionHooks { } } } + + self.reprocess_state_dependencies( + write_policy, + rejected_events_index, + relay, + &event.pubkey, + &announcement.identifier, + ) + .await; } } diff --git a/src/server.rs b/src/server.rs index ff6cd24..3b2878f 100644 --- a/src/server.rs +++ b/src/server.rs @@ -242,6 +242,9 @@ impl RelayServer { // Get a reference to the rejected events index for shutdown persistence // and for the HTTP server's git push path (hot-cache re-processing) let rejected_events_index = sync_manager.rejected_events_index(); + relay_runtime + .write_policy + .set_rejected_events_index(rejected_events_index.clone()); let mut background_tasks = Vec::new(); diff --git a/tests/sync/maintainer_reprocessing.rs b/tests/sync/maintainer_reprocessing.rs index 45383ff..f9842a8 100644 --- a/tests/sync/maintainer_reprocessing.rs +++ b/tests/sync/maintainer_reprocessing.rs @@ -92,6 +92,25 @@ fn repository_announcement( EventBuilder::new(Kind::GitRepoAnnouncement, "Repository announcement").tags(tags) } +fn repository_state_with_default_branch( + clone_urls: &[String], + relay_urls: &[String], + identifier: &str, + default_branch: &str, + commit: &str, +) -> EventBuilder { + EventBuilder::new(Kind::RepoState, "").tags(vec![ + Tag::identifier(identifier), + Tag::custom("clone", clone_urls.to_vec()), + Tag::custom("relays", relay_urls.to_vec()), + Tag::custom( + format!("refs/heads/{default_branch}"), + vec![commit.to_string()], + ), + Tag::custom("HEAD", vec![format!("refs/heads/{default_branch}")]), + ]) +} + async fn assert_exact_event_served(relay: &TestRelay, event: &Event, description: &str) { let found = wait_for_event_on_relay( relay.url(), @@ -183,6 +202,58 @@ async fn assert_remote_refs( } } +fn rename_default_branch(repository: &Path, branch: &str) { + let output = Command::new("git") + .args(["branch", "-m", branch]) + .current_dir(repository) + .output() + .expect("Failed to rename test repository default branch"); + assert!( + output.status.success(), + "Failed to rename test repository default branch: {}", + String::from_utf8_lossy(&output.stderr) + ); +} + +fn remote_default_branch(clone_url: &str) -> Result { + let output = Command::new("git") + .args(["ls-remote", "--symref", clone_url, "HEAD"]) + .output() + .map_err(|error| format!("Failed to inspect HEAD from {clone_url}: {error}"))?; + if !output.status.success() { + return Err(format!( + "Failed to inspect HEAD from {clone_url}: {}", + String::from_utf8_lossy(&output.stderr) + )); + } + + String::from_utf8(output.stdout) + .map_err(|error| format!("Invalid UTF-8 from {clone_url}: {error}"))? + .lines() + .find_map(|line| { + line.strip_prefix("ref: ") + .and_then(|line| line.strip_suffix("\tHEAD")) + .map(str::to_string) + }) + .ok_or_else(|| format!("No symbolic HEAD returned by {clone_url}")) +} + +async fn assert_remote_default_branch(clone_url: &str, expected: &str, timeout: Duration) { + let deadline = tokio::time::Instant::now() + timeout; + loop { + let actual = remote_default_branch(clone_url); + if actual.as_deref() == Ok(expected) { + return; + } + assert!( + tokio::time::Instant::now() < deadline, + "Announced Git endpoint should use {expected} as its default branch: {clone_url}\n\ + actual: {actual:?}" + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } +} + /// Exercise the production invitation flow for a repository that already has /// owner state and Git data. /// @@ -240,13 +311,8 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu send_to_relay(relay, &owner_state) .await .expect("Failed to publish owner state"); - crate::common::push_to_relay( - owner_git.path(), - &relay.domain(), - &owner_npub, - identifier, - ) - .expect("Failed to push existing owner repository"); + crate::common::push_to_relay(owner_git.path(), &relay.domain(), &owner_npub, identifier) + .expect("Failed to push existing owner repository"); assert_exact_event_served(relay, &initial_announcement, "Initial owner announcement").await; assert_exact_event_served(relay, &owner_state, "Owner state event").await; let owner_ref_aligned = crate::common::check_ref_at_commit( @@ -336,9 +402,7 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu "Invitation acceptance must not perform or attempt a client Git push" ); - let expected_refs = BTreeMap::from([ - ("refs/heads/main".to_string(), commit.clone()), - ]); + let expected_refs = BTreeMap::from([("refs/heads/main".to_string(), commit.clone())]); let announced_owner_clones = announcement_clone_urls(&invitation); let announced_invitee_clones = announcement_clone_urls(&acceptance); assert_eq!(announced_owner_clones, owner_clone_urls); @@ -360,6 +424,244 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu owner_relay.stop().await; } +/// An invitation must not overwrite an invitee's unrelated repository merely +/// because the inviter's state is newer. +/// +/// Both users first create a repository with the same identifier but different +/// default branches. The invitee's repository remains on its own state while +/// the one-way invitation syncs. Once the invitee publishes a reciprocal +/// announcement, the newer owner state becomes authoritative for the combined +/// maintainer set and replaces the invitee repository's refs without a new push. +#[tokio::test] +async fn test_acceptance_replaces_existing_invitee_repository_with_newer_owner_state() { + use crate::common::{create_test_repo_with_commit, CommitVariant}; + + let owner_relay = TestRelay::start_with_sync(None).await; + let invitee_relay = TestRelay::start_with_sync(None).await; + let shared_relay = TestRelay::start_with_sync(None).await; + let owner_keys = Keys::generate(); + let invitee_keys = Keys::generate(); + let identifier = "existing-invitee-repository-acceptance"; + let invitee_branch = "invitee-default"; + let owner_branch = "owner-default"; + let invitee_servers = [&invitee_relay, &shared_relay]; + let owner_servers = [&owner_relay, &shared_relay]; + + let invitee_git = tempfile::tempdir().expect("Failed to create invitee repository directory"); + let invitee_commit = create_test_repo_with_commit(invitee_git.path(), CommitVariant::StateTest) + .expect("Failed to create invitee repository commit"); + rename_default_branch(invitee_git.path(), invitee_branch); + let invitee_npub = invitee_keys + .public_key() + .to_bech32() + .expect("Failed to encode invitee npub"); + let (invitee_clone_urls, invitee_relay_urls) = + repository_urls(&invitee_keys, &invitee_servers, identifier); + let initial_invitee_announcement = + repository_announcement(&invitee_keys, &invitee_servers, &[], identifier) + .finalize(&invitee_keys) + .expect("Failed to create initial invitee announcement"); + let invitee_state = repository_state_with_default_branch( + &invitee_clone_urls, + &invitee_relay_urls, + identifier, + invitee_branch, + &invitee_commit, + ) + .finalize(&invitee_keys) + .expect("Failed to create invitee state"); + let invitee_refs = BTreeMap::from([( + format!("refs/heads/{invitee_branch}"), + invitee_commit.clone(), + )]); + + for relay in invitee_servers { + send_to_relay(relay, &initial_invitee_announcement) + .await + .expect("Failed to publish initial invitee announcement"); + send_to_relay(relay, &invitee_state) + .await + .expect("Failed to publish invitee state"); + crate::common::push_to_relay( + invitee_git.path(), + &relay.domain(), + &invitee_npub, + identifier, + ) + .expect("Failed to push existing invitee repository"); + assert_exact_event_served( + relay, + &initial_invitee_announcement, + "Initial invitee announcement", + ) + .await; + assert_exact_event_served(relay, &invitee_state, "Invitee state event").await; + } + for clone_url in &invitee_clone_urls { + assert_remote_refs(clone_url, &invitee_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{invitee_branch}"), + Duration::from_secs(20), + ) + .await; + } + + tokio::time::sleep(Duration::from_secs(1)).await; + + let owner_git = tempfile::tempdir().expect("Failed to create owner repository directory"); + let owner_commit = create_test_repo_with_commit(owner_git.path(), CommitVariant::PrTest) + .expect("Failed to create owner repository commit"); + rename_default_branch(owner_git.path(), owner_branch); + let owner_npub = owner_keys + .public_key() + .to_bech32() + .expect("Failed to encode owner npub"); + let (owner_clone_urls, owner_relay_urls) = + repository_urls(&owner_keys, &owner_servers, identifier); + let owner_created_at = Timestamp::from_secs(invitee_state.created_at.as_secs() + 1); + let initial_owner_announcement = + repository_announcement(&owner_keys, &owner_servers, &[], identifier) + .custom_created_at(owner_created_at) + .finalize(&owner_keys) + .expect("Failed to create initial owner announcement"); + let owner_state = repository_state_with_default_branch( + &owner_clone_urls, + &owner_relay_urls, + identifier, + owner_branch, + &owner_commit, + ) + .custom_created_at(owner_created_at) + .finalize(&owner_keys) + .expect("Failed to create owner state"); + assert!( + owner_state.created_at > invitee_state.created_at, + "Owner state must be newer than the invitee's existing state" + ); + + for relay in owner_servers { + send_to_relay(relay, &initial_owner_announcement) + .await + .expect("Failed to publish initial owner announcement"); + send_to_relay(relay, &owner_state) + .await + .expect("Failed to publish owner state"); + crate::common::push_to_relay(owner_git.path(), &relay.domain(), &owner_npub, identifier) + .expect("Failed to push owner repository"); + assert_exact_event_served( + relay, + &initial_owner_announcement, + "Initial owner announcement", + ) + .await; + assert_exact_event_served(relay, &owner_state, "Owner state event").await; + } + + let invitation = repository_announcement( + &owner_keys, + &owner_servers, + &[invitee_keys.public_key()], + identifier, + ) + .custom_created_at(Timestamp::from_secs(owner_created_at.as_secs() + 1)) + .finalize(&owner_keys) + .expect("Failed to create owner invitation"); + for relay in owner_servers { + send_to_relay(relay, &invitation) + .await + .expect("Failed to publish owner invitation"); + assert_exact_event_served(relay, &invitation, "Owner invitation announcement").await; + } + + let invitation_note = invitation + .id + .to_bech32() + .expect("Failed to encode owner invitation note ID"); + assert!( + wait_for_log( + &invitee_relay.log_path(), + &invitation_note, + Duration::from_secs(20), + ) + .await, + "Invitee-only server should process the unilateral invitation before acceptance" + ); + tokio::time::sleep(Duration::from_millis(500)).await; + for relay in invitee_servers { + assert_exact_event_served(relay, &invitee_state, "Invitee state before acceptance").await; + } + for clone_url in &invitee_clone_urls { + assert_remote_refs(clone_url, &invitee_refs, Duration::from_secs(5)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{invitee_branch}"), + Duration::from_secs(5), + ) + .await; + } + + let pushes_before_acceptance = [ + push_counts(&owner_relay).await, + push_counts(&invitee_relay).await, + push_counts(&shared_relay).await, + ]; + let acceptance = repository_announcement( + &invitee_keys, + &invitee_servers, + &[owner_keys.public_key()], + identifier, + ) + .custom_created_at(Timestamp::from_secs(owner_created_at.as_secs() + 2)) + .finalize(&invitee_keys) + .expect("Failed to create invitee acceptance"); + for relay in invitee_servers { + send_to_relay(relay, &acceptance) + .await + .expect("Failed to publish invitee acceptance"); + } + + for relay in [&owner_relay, &invitee_relay, &shared_relay] { + assert_exact_event_served(relay, &invitation, "Owner invitation announcement").await; + assert_exact_event_served(relay, &acceptance, "Invitee acceptance announcement").await; + assert_exact_event_served(relay, &owner_state, "Newer owner state event").await; + } + + let owner_refs = BTreeMap::from([(format!("refs/heads/{owner_branch}"), owner_commit.clone())]); + for clone_url in &invitee_clone_urls { + assert_remote_refs(clone_url, &owner_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{owner_branch}"), + Duration::from_secs(20), + ) + .await; + } + for clone_url in &owner_clone_urls { + assert_remote_refs(clone_url, &owner_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{owner_branch}"), + Duration::from_secs(20), + ) + .await; + } + + let pushes_after_acceptance = [ + push_counts(&owner_relay).await, + push_counts(&invitee_relay).await, + push_counts(&shared_relay).await, + ]; + assert_eq!( + pushes_after_acceptance, pushes_before_acceptance, + "Invitation acceptance must converge existing repositories without a client Git push" + ); + + shared_relay.stop().await; + invitee_relay.stop().await; + owner_relay.stop().await; +} + /// A reciprocal owner announcement in purgatory must be enough to unlock an /// earlier rejected maintainer announcement and use that maintainer's clone URL. /// From 1a5af3e07d9824a3900bb7277ae1b422cc502217 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Mon, 27 Jul 2026 10:56:13 +0100 Subject: [PATCH 11/11] test(sync): cover one-way maintainer state authority An owner authorizes an invited maintainer immediately by listing them; the maintainer does not need to publish a reciprocal acceptance before their newer state can govern the owner's repository. The reverse direction remains gated by acceptance. Exercise owner-only, invitee-only, and shared servers with distinct default branches. Require the invitation alone to align every owner endpoint to the invitee's newer refs and HEAD while the unchanged invitee announcement and endpoints remain intact and no client push occurs. --- tests/sync/maintainer_reprocessing.rs | 207 ++++++++++++++++++++++++++ 1 file changed, 207 insertions(+) diff --git a/tests/sync/maintainer_reprocessing.rs b/tests/sync/maintainer_reprocessing.rs index f9842a8..c41447c 100644 --- a/tests/sync/maintainer_reprocessing.rs +++ b/tests/sync/maintainer_reprocessing.rs @@ -424,6 +424,213 @@ async fn test_existing_repository_invitation_acceptance_syncs_without_invitee_pu owner_relay.stop().await; } +/// Listing a maintainer immediately authorizes that maintainer's state for the +/// inviting owner's repository; reciprocal acceptance is not required. +/// +/// The owner first publishes an older state on an owner-only and shared server. +/// The invitee later publishes a newer, different state on an invitee-only and +/// shared server. When the owner lists the invitee, GRASP-02 must apply the +/// invitee's newer state to both owner endpoints without changing the invitee's +/// announcement or requiring another Git push. +#[tokio::test] +async fn test_invitation_applies_newer_invitee_state_to_owner_before_acceptance() { + use crate::common::{create_test_repo_with_commit, CommitVariant}; + + let owner_relay = TestRelay::start_with_sync(None).await; + let invitee_relay = TestRelay::start_with_sync(None).await; + let shared_relay = TestRelay::start_with_sync(None).await; + let owner_keys = Keys::generate(); + let invitee_keys = Keys::generate(); + let identifier = "one-way-invitation-newer-maintainer-state"; + let owner_branch = "owner-default"; + let invitee_branch = "invitee-default"; + let owner_servers = [&owner_relay, &shared_relay]; + let invitee_servers = [&invitee_relay, &shared_relay]; + + let owner_git = tempfile::tempdir().expect("Failed to create owner repository directory"); + let owner_commit = create_test_repo_with_commit(owner_git.path(), CommitVariant::StateTest) + .expect("Failed to create owner repository commit"); + rename_default_branch(owner_git.path(), owner_branch); + let owner_npub = owner_keys + .public_key() + .to_bech32() + .expect("Failed to encode owner npub"); + let (owner_clone_urls, owner_relay_urls) = + repository_urls(&owner_keys, &owner_servers, identifier); + let initial_owner_announcement = + repository_announcement(&owner_keys, &owner_servers, &[], identifier) + .finalize(&owner_keys) + .expect("Failed to create initial owner announcement"); + let owner_state = repository_state_with_default_branch( + &owner_clone_urls, + &owner_relay_urls, + identifier, + owner_branch, + &owner_commit, + ) + .finalize(&owner_keys) + .expect("Failed to create owner state"); + let owner_refs = BTreeMap::from([(format!("refs/heads/{owner_branch}"), owner_commit)]); + + for relay in owner_servers { + send_to_relay(relay, &initial_owner_announcement) + .await + .expect("Failed to publish initial owner announcement"); + send_to_relay(relay, &owner_state) + .await + .expect("Failed to publish owner state"); + crate::common::push_to_relay(owner_git.path(), &relay.domain(), &owner_npub, identifier) + .expect("Failed to push owner repository"); + assert_exact_event_served( + relay, + &initial_owner_announcement, + "Initial owner announcement", + ) + .await; + assert_exact_event_served(relay, &owner_state, "Owner state event").await; + } + for clone_url in &owner_clone_urls { + assert_remote_refs(clone_url, &owner_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{owner_branch}"), + Duration::from_secs(20), + ) + .await; + } + + tokio::time::sleep(Duration::from_secs(1)).await; + + let invitee_git = tempfile::tempdir().expect("Failed to create invitee repository directory"); + let invitee_commit = create_test_repo_with_commit(invitee_git.path(), CommitVariant::PrTest) + .expect("Failed to create invitee repository commit"); + rename_default_branch(invitee_git.path(), invitee_branch); + let invitee_npub = invitee_keys + .public_key() + .to_bech32() + .expect("Failed to encode invitee npub"); + let (invitee_clone_urls, invitee_relay_urls) = + repository_urls(&invitee_keys, &invitee_servers, identifier); + let invitee_created_at = Timestamp::from_secs(owner_state.created_at.as_secs() + 1); + let invitee_announcement = + repository_announcement(&invitee_keys, &invitee_servers, &[], identifier) + .custom_created_at(invitee_created_at) + .finalize(&invitee_keys) + .expect("Failed to create invitee announcement"); + let invitee_state = repository_state_with_default_branch( + &invitee_clone_urls, + &invitee_relay_urls, + identifier, + invitee_branch, + &invitee_commit, + ) + .custom_created_at(invitee_created_at) + .finalize(&invitee_keys) + .expect("Failed to create invitee state"); + assert!( + invitee_state.created_at > owner_state.created_at, + "Invitee state must be newer than the owner's existing state" + ); + let invitee_refs = BTreeMap::from([( + format!("refs/heads/{invitee_branch}"), + invitee_commit.clone(), + )]); + + for relay in invitee_servers { + send_to_relay(relay, &invitee_announcement) + .await + .expect("Failed to publish invitee announcement"); + send_to_relay(relay, &invitee_state) + .await + .expect("Failed to publish invitee state"); + crate::common::push_to_relay( + invitee_git.path(), + &relay.domain(), + &invitee_npub, + identifier, + ) + .expect("Failed to push invitee repository"); + assert_exact_event_served(relay, &invitee_announcement, "Invitee announcement").await; + assert_exact_event_served(relay, &invitee_state, "Invitee state event").await; + } + for clone_url in &invitee_clone_urls { + assert_remote_refs(clone_url, &invitee_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{invitee_branch}"), + Duration::from_secs(20), + ) + .await; + } + + for clone_url in &owner_clone_urls { + assert_remote_refs(clone_url, &owner_refs, Duration::from_secs(5)).await; + } + + let pushes_before_invitation = [ + push_counts(&owner_relay).await, + push_counts(&invitee_relay).await, + push_counts(&shared_relay).await, + ]; + let invitation = repository_announcement( + &owner_keys, + &owner_servers, + &[invitee_keys.public_key()], + identifier, + ) + .custom_created_at(Timestamp::from_secs(invitee_created_at.as_secs() + 1)) + .finalize(&owner_keys) + .expect("Failed to create owner invitation"); + for relay in owner_servers { + send_to_relay(relay, &invitation) + .await + .expect("Failed to publish owner invitation"); + } + + for relay in owner_servers { + assert_exact_event_served(relay, &invitation, "Owner invitation announcement").await; + assert_exact_event_served( + relay, + &invitee_announcement, + "Invited maintainer announcement", + ) + .await; + assert_exact_event_served(relay, &invitee_state, "Newer invited maintainer state").await; + } + for clone_url in &owner_clone_urls { + assert_remote_refs(clone_url, &invitee_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{invitee_branch}"), + Duration::from_secs(20), + ) + .await; + } + for clone_url in &invitee_clone_urls { + assert_remote_refs(clone_url, &invitee_refs, Duration::from_secs(20)).await; + assert_remote_default_branch( + clone_url, + &format!("refs/heads/{invitee_branch}"), + Duration::from_secs(20), + ) + .await; + } + + let pushes_after_invitation = [ + push_counts(&owner_relay).await, + push_counts(&invitee_relay).await, + push_counts(&shared_relay).await, + ]; + assert_eq!( + pushes_after_invitation, pushes_before_invitation, + "One-way maintainer authorization must sync the owner without a client Git push" + ); + + shared_relay.stop().await; + invitee_relay.stop().await; + owner_relay.stop().await; +} + /// An invitation must not overwrite an invitee's unrelated repository merely /// because the inviter's state is newer. ///