From 236975c4fd7be1f9947ee4fd4b0dbe8b3754db5b Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Thu, 17 Sep 2026 15:27:50 +0000 Subject: [PATCH] fix(purgatory): honor lower event IDs when timestamps tie Purgatory push authorization selected matching authorized state events by timestamp alone, allowing arrival order to choose a superseded same-second state. Announcement de-list eviction required a strictly later timestamp, so a winning lower-ID replacement could leave its purgatory entry and bare repository behind. Use timestamp descending and event ID ascending for both decisions. Keep existing author authorization, ref matching, admission exceptions and resource cleanup unchanged. Losing or replayed de-list events do not evict the preferred entry. The comparisons operate on the already-selected repository identifier/owner scope; this does not change membership rules. Add regressions through the actual authorization and announcement-policy paths. Sort signed candidates rather than mining IDs. Check both state arrival orders, unauthorized newer states, later authorized states, and winning/losing de-list replacements including replay and filesystem/state cleanup. Both regressions fail against the original comparisons and pass with the fixes. Update the unreleased changelog and architecture notes. Validation: full ngit-grasp test suite, all-target workspace Clippy with warnings denied, formatting and diff checks pass. This commit addresses the two purgatory comparisons; served-event deletion/history and rollback ordering are separate paths. Assisted-by: GPT-6 --- CHANGELOG.md | 5 ++ docs/explanation/architecture.md | 4 ++ src/git/authorization.rs | 82 +++++++++++++++++++++++++++++++- src/nostr/policy/announcement.rs | 76 ++++++++++++++++++++++++++++- 4 files changed, 163 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ed7f326..b88ec9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Apply the lower-event-ID tie-break to same-second purgatory replacements. + Git push authorization now selects the preferred matching state regardless + of arrival order, and a winning announcement that removes this service + evicts its purgatory entry and associated repository data. + - Preserve all persisted root events when rebuilding sync state at startup. Purgatory cleanup can no longer remove a partially reconstructed repository and leave its older threads untracked until another restart. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index a0b61d6..c70bc9d 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -419,9 +419,13 @@ pub struct Purgatory { - Bare repo created immediately so pushes can succeed - Announcement promoted to database only when git data proves content exists - Two-phase soft expiry: bare repo deleted at 30 min, event retained 24h for revival + - De-list replacements evict an owner's entry only when they win NIP-01 + ordering: later timestamp, or lower event ID at the same timestamp. 3. **Late Binding**: State event refs are extracted at git push time, not event arrival - Enables flexible matching when pushes arrive out-of-order + - Among matching states from authorized authors, push authorization selects + the latest timestamp, breaking ties with the lowest event ID. - Helper functions in [`helpers.rs`](../../src/purgatory/helpers.rs) handle ref extraction 4. **Bidirectional Waiting**: Either side can arrive first diff --git a/src/git/authorization.rs b/src/git/authorization.rs index a2fc668..dc4ac29 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -685,10 +685,14 @@ pub async fn get_state_authorization_for_selected_repo( .collect(); if !authorized_events.is_empty() { - // Find the latest event + // NIP-01 prefers the lower event ID when timestamps tie. let latest_authorized = authorized_events .iter() - .max_by_key(|event| event.created_at) + .max_by(|left, right| { + left.created_at + .cmp(&right.created_at) + .then_with(|| right.id.cmp(&left.id)) + }) .unwrap(); // Safe because we checked the vec is not empty // Parse the event into RepositoryState @@ -1377,6 +1381,80 @@ mod tests { use super::*; use nostr_sdk::prelude::{EventBuilder, FinalizeEvent, Keys, Tag}; + #[tokio::test] + async fn nip01_purgatory_state_selection_is_independent_of_arrival_order() { + let keys = Keys::generate(); + let identifier = "same-second-state"; + let commit = "1234567890abcdef1234567890abcdef12345678"; + let timestamp = Timestamp::from_secs(100); + let make_state = |signer: &Keys, content: &str, created_at| { + EventBuilder::new(Kind::RepoState, content) + .tags([ + Tag::identifier(identifier), + Tag::custom("refs/heads/main", [commit]), + ]) + .custom_created_at(created_at) + .finalize(signer) + .unwrap() + }; + let mut states = [ + make_state(&keys, "a", timestamp), + make_state(&keys, "b", timestamp), + ]; + states.sort_by_key(|event| event.id); + let [winner, loser] = states; + let outsider = make_state(&Keys::generate(), "unauthorized", Timestamp::from_secs(200)); + let newer = make_state(&keys, "newer", Timestamp::from_secs(101)); + + for candidates in [[&winner, &loser], [&loser, &winner]] { + let directory = tempfile::tempdir().unwrap(); + let purgatory = Arc::new(Purgatory::new(directory.path().to_path_buf())); + let database: SharedDatabase = Arc::new(nostr_memory::MemoryDatabase::unbounded()); + database + .save_event(&create_announcement_event(&keys, identifier, &[])) + .await + .unwrap(); + for event in candidates.into_iter().chain([&outsider]) { + purgatory.add_state(event.clone(), identifier.into(), event.pubkey, false); + } + let pushed = vec![("0".repeat(40), commit.into(), "refs/heads/main".into())]; + let selected = keys.public_key().to_hex(); + let result = get_state_authorization_for_selected_repo( + &database, + identifier, + &selected, + &purgatory, + &pushed, + directory.path(), + ) + .await + .unwrap(); + assert!(result.authorized); + assert_eq!(result.state.unwrap().event.id, winner.id); + assert_eq!( + result + .purgatory_events + .iter() + .map(|event| event.id) + .collect::>(), + vec![winner.id] + ); + + purgatory.add_state(newer.clone(), identifier.into(), keys.public_key(), false); + let result = get_state_authorization_for_selected_repo( + &database, + identifier, + &selected, + &purgatory, + &pushed, + directory.path(), + ) + .await + .unwrap(); + assert_eq!(result.state.unwrap().event.id, newer.id); + } + } + fn create_test_keys() -> Keys { Keys::generate() } diff --git a/src/nostr/policy/announcement.rs b/src/nostr/policy/announcement.rs index 832ebab..1bc6f1a 100644 --- a/src/nostr/policy/announcement.rs +++ b/src/nostr/policy/announcement.rs @@ -80,7 +80,7 @@ impl AnnouncementPolicy { match RepositoryAnnouncement::from_event(event.clone()) { Ok(announcement) => { // If this pubkey+identifier has a purgatory entry AND the incoming - // event is strictly newer, the owner is sending a replacement that + // event wins NIP-01 ordering, the owner is sending a replacement that // removes our service. Clear the purgatory entry and its bare repo. // // If the incoming event is older than the purgatory entry (e.g. a @@ -90,7 +90,11 @@ impl AnnouncementPolicy { .ctx .purgatory .find_announcement(&event.pubkey, &announcement.identifier) - .is_some_and(|entry| event.created_at > entry.event.created_at); + .is_some_and(|entry| { + event.created_at > entry.event.created_at + || (event.created_at == entry.event.created_at + && event.id < entry.event.id) + }); if should_evict { self.remove_purgatory_announcement( @@ -526,6 +530,74 @@ mod tests { .expect("signed announcement") } + #[tokio::test] + async fn nip01_purgatory_delisting_evicts_only_the_winning_replacement() { + let keys = Keys::generate(); + let identifier = "membership-gate-repo"; + let mut candidates = ["first.example", "second.example"].map(|domain| { + let event = service_announcement(&keys, domain); + let event = EventBuilder::new(event.kind, event.content) + .tags(event.tags.to_vec()) + .custom_created_at(nostr_sdk::prelude::Timestamp::from_secs(100)) + .finalize(&keys) + .unwrap(); + (event, domain) + }); + candidates.sort_by_key(|(event, _)| event.id); + let [(winner, winner_domain), (loser, loser_domain)] = candidates; + + for (stored, domain, incoming, evicted) in [ + (&loser, loser_domain, &winner, true), + (&winner, winner_domain, &loser, false), + ] { + let directory = tempfile::tempdir().unwrap(); + let repo_path = directory.path().join("repo.git"); + std::fs::create_dir(&repo_path).unwrap(); + let mut config = Config::for_testing(); + config.domain = domain.into(); + let purgatory = Arc::new(crate::purgatory::Purgatory::new( + directory.path().to_path_buf(), + )); + purgatory.add_announcement( + stored.clone(), + identifier.into(), + keys.public_key(), + repo_path.clone(), + HashSet::new(), + ); + let state = EventBuilder::new(Kind::RepoState, "") + .tag(Tag::identifier(identifier)) + .finalize(&keys) + .unwrap(); + purgatory.add_state(state, identifier.into(), keys.public_key(), false); + let ctx = PolicyContext::new_for_test( + domain.to_string(), + Arc::new(nostr_memory::MemoryDatabase::unbounded()), + directory.path().to_path_buf(), + Arc::clone(&purgatory), + config.clone(), + ); + let policy = AnnouncementPolicy::new(ctx, config, None); + policy.validate(incoming).await; + assert_eq!( + purgatory + .find_announcement(&keys.public_key(), identifier) + .is_none(), + evicted + ); + assert_eq!(!repo_path.exists(), evicted); + assert_eq!(purgatory.find_state(identifier).is_empty(), evicted); + // Replaying a non-winning de-list must remain harmless. + policy.validate(incoming).await; + assert_eq!( + purgatory + .find_announcement(&keys.public_key(), identifier) + .is_none(), + evicted + ); + } + } + #[tokio::test] async fn private_mode_rejects_announcement_from_nonmember_author() { let member = Keys::generate();