From 6e9bf5981b89827a611be6479f5d964f0c711d47 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Thu, 17 Sep 2026 15:38:24 +0000 Subject: [PATCH] fix(lifecycle): preserve NIP-01 ordering through deletion and rollback Same-second lower-ID replacements could be ignored by served-repository de-listing and superseded-history capture. Rollback's active-state lookup and post-deletion alignment instead preferred higher IDs, while history selection let capture time override the original event's replacement order. Share the later-timestamp/lower-ID comparator across these lifecycle paths and the existing purgatory decisions. Archive a same-second predecessor only when the incoming event wins, and only apply de-list deletion for a winning replacement. Select active and authorized alignment states with the same ordering. Rank history records by original timestamp and original event ID before capture metadata; keep timestamp cutoffs and author checks intact. Extract the authorized-state selection used by realignment to exercise both arrival orders with different authorized authors and an unauthorized newer state. Add served de-list, history-capture and history-candidate regressions. All four new regressions fail before the fixes and pass afterward. Extend the LMDB restart scenario to delete the same-second winner and observe its archived predecessor restored, with a bounded deadline and no new sleeps. This changes replacement preference, not deletion request ownership, coordinate cutoff semantics, repository membership or archive format. Existing capture metadata remains readable. Update the changelog and repository lifecycle documentation. Validation: focused ordering regressions and LMDB restart/rollback test; full ngit-grasp suite, all-target workspace Clippy with warnings denied, formatting and diff checks pass. Assisted-by: GPT-6 --- CHANGELOG.md | 4 + docs/explanation/repository-lifecycle.md | 14 +++- src/git/authorization.rs | 8 +- src/nostr/events.rs | 8 ++ src/nostr/lifecycle/deletion/rollback.rs | 74 ++++++++++++++---- src/nostr/lifecycle/deletion/service.rs | 99 +++++++++++++++++++++++- src/nostr/lifecycle/history.rs | 46 ++++++++++- src/nostr/policy/announcement.rs | 14 ++-- tests/lifecycle/replaceable_history.rs | 17 ++++ 9 files changed, 244 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b88ec9b..0a352e4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Honor same-second lower-ID replacements when de-listing served repositories, + capturing superseded history, and choosing rollback or post-deletion state. + History capture time no longer overrides the original events' ID tie-break. + - 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 diff --git a/docs/explanation/repository-lifecycle.md b/docs/explanation/repository-lifecycle.md index bd1f56a..fbd5932 100644 --- a/docs/explanation/repository-lifecycle.md +++ b/docs/explanation/repository-lifecycle.md @@ -722,14 +722,22 @@ behavior. The relay archives superseded 30617/30618 versions on write and uses that history at deletion time to pick rollback candidates for active `e`-target -deletions (announcement + state). +deletions (announcement + state). Same-second replacements use the NIP-01 +lower-event-ID tie-break for history capture, served-repository de-listing, +active-version selection and post-deletion state alignment. Losing replacements +and exact replays do not trigger history capture or de-list deletion. + +History candidates are ordered by their original event timestamp descending, +then original event ID ascending. Capture time and metadata ID only break ties +between records for the same original event; processing order cannot override +replacement preference. Timestamp cutoffs and author checks still apply. This architecture is covered by two integration-test layers: -- `tests/replaceable_history.rs` verifies the durable history substrate: +- `tests/lifecycle/replaceable_history.rs` verifies the durable history substrate: superseded 30617/30618 payloads are captured, queryable by coordinate/cutoff, survive LMDB restart, and do not change normal serving semantics. -- `tests/nip09_state_cascade.rs` verifies deletion behavior that consumes that +- `tests/lifecycle/nip09_state_cascade.rs` verifies deletion behavior that consumes that substrate: active 30618 `e` deletion restores the previous state event and realigns refs, active 30617 `e` deletion restores the previous announcement when history exists, coordinate deletion cutoff semantics do not incorrectly diff --git a/src/git/authorization.rs b/src/git/authorization.rs index dc4ac29..f64d92b 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -36,7 +36,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::Arc; use tracing::{debug, info, warn}; -use crate::nostr::events::{RepositoryAnnouncement, RepositoryState}; +use crate::nostr::events::{compare_replacement_events, RepositoryAnnouncement, RepositoryState}; use crate::nostr::SharedDatabase; use crate::purgatory::Purgatory; use nostr_sdk::prelude::{Kind, PublicKey}; @@ -688,11 +688,7 @@ pub async fn get_state_authorization_for_selected_repo( // NIP-01 prefers the lower event ID when timestamps tie. let latest_authorized = authorized_events .iter() - .max_by(|left, right| { - left.created_at - .cmp(&right.created_at) - .then_with(|| right.id.cmp(&left.id)) - }) + .max_by(|left, right| compare_replacement_events(left, right)) .unwrap(); // Safe because we checked the vec is not empty // Parse the event into RepositoryState diff --git a/src/nostr/events.rs b/src/nostr/events.rs index 2489ea1..8f0c5fa 100644 --- a/src/nostr/events.rs +++ b/src/nostr/events.rs @@ -9,6 +9,14 @@ use anyhow::{anyhow, Result}; use nostr_sdk::prelude::{Event, Kind, PublicKey, ToBech32}; +/// Compare replacement preference within an already-selected event scope. +/// Greater means preferred: later timestamp, then lower event ID (NIP-01). +pub(crate) fn compare_replacement_events(left: &Event, right: &Event) -> std::cmp::Ordering { + left.created_at + .cmp(&right.created_at) + .then_with(|| right.id.cmp(&left.id)) +} + /// Whether a NIP-34 indexed role record is syntactically valid and active. /// /// Values after the pubkey alternate between numeric start and end diff --git a/src/nostr/lifecycle/deletion/rollback.rs b/src/nostr/lifecycle/deletion/rollback.rs index 4df9405..a73160b 100644 --- a/src/nostr/lifecycle/deletion/rollback.rs +++ b/src/nostr/lifecycle/deletion/rollback.rs @@ -8,7 +8,7 @@ use crate::git::authorization::{ collect_state_maintainers_by_coordinate, fetch_repository_data_excluding_purgatory, }; use crate::git::process; -use crate::nostr::events::RepositoryState; +use crate::nostr::events::{compare_replacement_events, RepositoryState}; use crate::nostr::lifecycle::RecoveryScope; #[derive(Debug, Clone, PartialEq, Eq, Hash)] @@ -169,11 +169,7 @@ impl DeletionPolicy { return None; }; - events.into_iter().max_by(|a, b| { - a.created_at - .cmp(&b.created_at) - .then_with(|| a.id.cmp(&b.id)) - }) + events.into_iter().max_by(compare_replacement_events) } pub(super) async fn rollback_deleted_active_replaceable(&self, plan: &ReplaceableRollbackPlan) { @@ -423,16 +419,7 @@ impl DeletionPolicy { continue; } - let latest_state = repo_data - .states - .iter() - .filter(|state| maintainers.contains(&state.event.pubkey.to_hex())) - .max_by(|a, b| { - a.event - .created_at - .cmp(&b.event.created_at) - .then_with(|| a.event.id.cmp(&b.event.id)) - }); + let latest_state = latest_authorized_state(&repo_data.states, maintainers); match latest_state { Some(state) => { @@ -488,3 +475,58 @@ impl DeletionPolicy { } } } + +fn latest_authorized_state<'a>( + states: &'a [RepositoryState], + maintainers: &[String], +) -> Option<&'a RepositoryState> { + states + .iter() + .filter(|state| maintainers.contains(&state.event.pubkey.to_hex())) + .max_by(|a, b| compare_replacement_events(&a.event, &b.event)) +} + +#[cfg(test)] +mod tests { + use super::*; + use nostr_sdk::prelude::{EventBuilder, FinalizeEvent, Keys, Tag}; + + #[test] + fn replacement_order_selects_authorized_state_for_realignment() { + let keys = [Keys::generate(), Keys::generate()]; + let make_state = |keys: &Keys, time| { + RepositoryState::from_event( + EventBuilder::new(Kind::RepoState, "") + .tags([ + Tag::identifier("repo"), + Tag::custom( + "refs/heads/main", + ["1234567890abcdef1234567890abcdef12345678"], + ), + ]) + .custom_created_at(Timestamp::from_secs(time)) + .finalize(keys) + .unwrap(), + ) + .unwrap() + }; + let mut candidates = [make_state(&keys[0], 100), make_state(&keys[1], 100)]; + candidates.sort_by_key(|state| state.event.id); + let [winner, loser] = candidates; + let authorized = keys.map(|keys| keys.public_key().to_hex()); + let outsider = make_state(&Keys::generate(), 200); + for states in [ + vec![winner.clone(), loser.clone(), outsider.clone()], + vec![loser.clone(), winner.clone(), outsider.clone()], + ] { + assert_eq!( + latest_authorized_state(&states, &authorized) + .unwrap() + .event + .id, + winner.event.id + ); + } + assert!(latest_authorized_state(&[outsider], &authorized).is_none()); + } +} diff --git a/src/nostr/lifecycle/deletion/service.rs b/src/nostr/lifecycle/deletion/service.rs index e5d614d..beca4f3 100644 --- a/src/nostr/lifecycle/deletion/service.rs +++ b/src/nostr/lifecycle/deletion/service.rs @@ -3,7 +3,7 @@ use nostr_sdk::prelude::{ nip62, Event, Filter, Kind, PublicKey, RelayUrl, SingleLetterTag, Timestamp, WritePolicyResult, }; -use crate::nostr::events::RepositoryAnnouncement; +use crate::nostr::events::{compare_replacement_events, RepositoryAnnouncement}; use crate::nostr::lifecycle::{RequestClassification, RequestLifecycleRecord}; use crate::nostr::policy::{duplicate, reject_error, reject_invalid, AnnouncementResult}; @@ -527,7 +527,7 @@ impl DeletionService { ); let current = match self.ctx.database().query(filter).await { - Ok(events) => events.into_iter().max_by_key(|event| event.created_at), + Ok(events) => events.into_iter().max_by(compare_replacement_events), Err(e) => { tracing::warn!( owner = %incoming.pubkey.to_hex(), @@ -544,7 +544,7 @@ impl DeletionService { }; // Not newer than what we already serve -> no runtime parity action. - if incoming.created_at <= current.created_at { + if !compare_replacement_events(incoming, ¤t).is_gt() { return; } @@ -631,7 +631,7 @@ impl DeletionService { }; for superseded in existing { - if superseded.id == incoming.id || superseded.created_at >= incoming.created_at { + if !compare_replacement_events(incoming, &superseded).is_gt() { continue; } @@ -894,6 +894,97 @@ mod tests { ) } + fn replacement_candidates(kind: Kind) -> [(Event, &'static str); 2] { + let keys = Keys::generate(); + let mut candidates = ["first.example", "second.example"].map(|domain| { + let event = EventBuilder::new(kind, domain) + .tags([ + Tag::identifier("replacement-order"), + Tag::custom("clone", [format!("https://{domain}/repo.git")]), + Tag::custom("relays", [format!("wss://{domain}")]), + ]) + .custom_created_at(Timestamp::from_secs(100)) + .finalize(&keys) + .unwrap(); + (event, domain) + }); + candidates.sort_by_key(|(event, _)| event.id); + candidates + } + + #[tokio::test] + async fn replacement_order_controls_served_delisting() { + let [(winner, winner_domain), (loser, loser_domain)] = + replacement_candidates(Kind::GitRepoAnnouncement); + for (current, domain, incoming, deleted) in [ + (&loser, loser_domain, &winner, true), + (&winner, winner_domain, &loser, false), + (&winner, winner_domain, &winner, false), + ] { + let directory = tempfile::tempdir().unwrap(); + let mut ctx = context(crate::config::Config::for_testing()); + ctx.domain = domain.into(); + ctx.config.domain = domain.into(); + ctx.git_data_path = directory.path().to_path_buf(); + ctx.purgatory = Arc::new(Purgatory::new(directory.path().to_path_buf())); + ctx.database.save_event(current).await.unwrap(); + let service = DeletionService::new(ctx); + service.maybe_apply_runtime_delist_deletion(incoming).await; + assert_eq!( + service + .ctx + .database + .event_by_id(¤t.id) + .await + .unwrap() + .is_none(), + deleted + ); + } + } + + #[tokio::test] + async fn replacement_order_controls_same_second_history_capture() { + for kind in [Kind::GitRepoAnnouncement, Kind::RepoState] { + let [(winner, _), (loser, _)] = replacement_candidates(kind); + for (current, incoming, captures) in [ + (&loser, &winner, 1), + (&winner, &loser, 0), + (&winner, &winner, 0), + ] { + let service = DeletionService::new(context(crate::config::Config::for_testing())); + service.ctx.database.save_event(current).await.unwrap(); + service + .capture_superseded_replaceable_history(incoming) + .await; + let coordinate = format!( + "{}:{}:replacement-order", + kind.as_u16(), + current.pubkey.to_hex() + ); + let records = service + .history() + .superseded_records_for_coordinate_before(&coordinate, incoming.created_at) + .await; + assert_eq!(records.len(), captures); + if captures == 1 { + assert_eq!(records[0].superseded_event_id, Some(current.id)); + assert_eq!(records[0].replaced_by, Some(incoming.id)); + assert_eq!( + service + .history() + .event_by_id(¤t.id) + .await + .unwrap() + .unwrap() + .id, + current.id + ); + } + } + } + } + fn deletion(keys: &Keys, target: EventId) -> Event { EventBuilder::new(Kind::EventDeletion, "") .tags(vec![Tag::event(target)]) diff --git a/src/nostr/lifecycle/history.rs b/src/nostr/lifecycle/history.rs index 53b98c7..28a7226 100644 --- a/src/nostr/lifecycle/history.rs +++ b/src/nostr/lifecycle/history.rs @@ -184,8 +184,9 @@ impl ReplaceableHistoryStore { /// /// Ordering is deterministic: /// 1. `superseded_at` descending (missing treated as 0) - /// 2. `captured_at` descending (missing treated as 0) - /// 3. `metadata_event_id` descending + /// 2. Superseded event ID ascending (NIP-01); missing IDs last + /// 3. `captured_at` descending (missing treated as 0) + /// 4. `metadata_event_id` descending for duplicate capture records pub async fn latest_superseded_before( &self, coordinate: &str, @@ -199,6 +200,12 @@ impl ReplaceableHistoryStore { b.superseded_at .unwrap_or_else(|| Timestamp::from_secs(0)) .cmp(&a.superseded_at.unwrap_or_else(|| Timestamp::from_secs(0))) + .then_with(|| { + a.superseded_event_id + .is_none() + .cmp(&b.superseded_event_id.is_none()) + }) + .then_with(|| a.superseded_event_id.cmp(&b.superseded_event_id)) .then_with(|| { b.captured_at .unwrap_or_else(|| Timestamp::from_secs(0)) @@ -262,6 +269,41 @@ mod tests { .unwrap() } + #[tokio::test] + async fn replacement_order_precedes_capture_time_for_rollback_history() { + let store = ReplaceableHistoryStore::in_memory(); + let keys = Keys::generate(); + let mut candidates = [ + announcement(&keys, "repo", 100, "a"), + announcement(&keys, "repo", 100, "b"), + ]; + candidates.sort_by_key(|event| event.id); + let [winner, loser] = candidates; + let coordinate = format!("30617:{}:repo", keys.public_key().to_hex()); + // Capture order is deliberately opposite to replacement preference. + for (event, captured) in [(&winner, "100"), (&loser, "200")] { + let metadata = EventBuilder::new(Kind::from(HISTORY_METADATA_KIND), "") + .tags([ + Tag::event(event.id), + Tag::custom("a", [coordinate.clone()]), + Tag::custom(HISTORY_SUPERSEDED_AT_TAG, ["100"]), + Tag::custom(HISTORY_CAPTURED_AT_TAG, [captured]), + ]) + .finalize(&store.metadata_signer) + .unwrap(); + store.db.save_event(&metadata).await.unwrap(); + } + let selected = store + .latest_superseded_before(&coordinate, Timestamp::from_secs(100)) + .await + .unwrap(); + assert_eq!(selected.superseded_event_id, Some(winner.id)); + assert!(store + .latest_superseded_before(&coordinate, Timestamp::from_secs(99)) + .await + .is_none()); + } + #[tokio::test] async fn archives_and_queries_superseded_records_by_coordinate_and_time() { let store = ReplaceableHistoryStore::in_memory(); diff --git a/src/nostr/policy/announcement.rs b/src/nostr/policy/announcement.rs index 1bc6f1a..bb84da8 100644 --- a/src/nostr/policy/announcement.rs +++ b/src/nostr/policy/announcement.rs @@ -8,7 +8,9 @@ use std::time::Duration; use super::PolicyContext; use crate::config::Config; -use crate::nostr::events::{validate_announcement, RepositoryAnnouncement}; +use crate::nostr::events::{ + compare_replacement_events, validate_announcement, RepositoryAnnouncement, +}; use crate::private::PrivateAccess; /// Result of announcement policy evaluation @@ -91,9 +93,7 @@ impl AnnouncementPolicy { .purgatory .find_announcement(&event.pubkey, &announcement.identifier) .is_some_and(|entry| { - event.created_at > entry.event.created_at - || (event.created_at == entry.event.created_at - && event.id < entry.event.id) + compare_replacement_events(event, &entry.event).is_gt() }); if should_evict { @@ -345,11 +345,7 @@ impl AnnouncementPolicy { Err(e) => return Err(format!("Database query failed: {}", e)), }; - Ok(events.into_iter().max_by(|left, right| { - left.created_at - .cmp(&right.created_at) - .then_with(|| right.id.cmp(&left.id)) - })) + Ok(events.into_iter().max_by(compare_replacement_events)) } /// Add an announcement to purgatory diff --git a/tests/lifecycle/replaceable_history.rs b/tests/lifecycle/replaceable_history.rs index 68948bc..14cb666 100644 --- a/tests/lifecycle/replaceable_history.rs +++ b/tests/lifecycle/replaceable_history.rs @@ -298,6 +298,23 @@ async fn same_second_30618_replacement_keeps_nip01_lowest_id() { "the lower-ID state must remain the NIP-01 winner after restart" ); + let deletion = EventBuilder::new(Kind::EventDeletion, "delete same-second winner") + .tag(Tag::event(replacement.id)) + .custom_created_at(timestamp_after(replacement.created_at)) + .finalize(client.keys()) + .expect("sign deletion with original author"); + restarted_client + .send_event(deletion) + .await + .expect("delete active winner"); + common::wait_for_event_served(restarted.url(), &existing.id, Duration::from_secs(10)) + .await + .expect("restore same-second predecessor from durable history"); + assert!(!restarted_client + .is_event_on_relay(replacement.id) + .await + .unwrap()); + restarted.stop().await; }