mirror of
https://relay.ngit.dev/npub15qydau2hjma6ngxkl2cyar74wzyjshvl65za5k5rl69264ar2exs5cyejr/ngit-grasp.git
synced 2026-10-05 15:08:24 +00:00
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
This commit is contained in:
@@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
|
|
||||||
### Fixed
|
### 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.
|
- Preserve all persisted root events when rebuilding sync state at startup.
|
||||||
Purgatory cleanup can no longer remove a partially reconstructed repository
|
Purgatory cleanup can no longer remove a partially reconstructed repository
|
||||||
and leave its older threads untracked until another restart.
|
and leave its older threads untracked until another restart.
|
||||||
|
|||||||
@@ -419,9 +419,13 @@ pub struct Purgatory {
|
|||||||
- Bare repo created immediately so pushes can succeed
|
- Bare repo created immediately so pushes can succeed
|
||||||
- Announcement promoted to database only when git data proves content exists
|
- 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
|
- 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
|
3. **Late Binding**: State event refs are extracted at git push time, not event arrival
|
||||||
- Enables flexible matching when pushes arrive out-of-order
|
- 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
|
- Helper functions in [`helpers.rs`](../../src/purgatory/helpers.rs) handle ref extraction
|
||||||
|
|
||||||
4. **Bidirectional Waiting**: Either side can arrive first
|
4. **Bidirectional Waiting**: Either side can arrive first
|
||||||
|
|||||||
@@ -685,10 +685,14 @@ pub async fn get_state_authorization_for_selected_repo(
|
|||||||
.collect();
|
.collect();
|
||||||
|
|
||||||
if !authorized_events.is_empty() {
|
if !authorized_events.is_empty() {
|
||||||
// Find the latest event
|
// NIP-01 prefers the lower event ID when timestamps tie.
|
||||||
let latest_authorized = authorized_events
|
let latest_authorized = authorized_events
|
||||||
.iter()
|
.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
|
.unwrap(); // Safe because we checked the vec is not empty
|
||||||
|
|
||||||
// Parse the event into RepositoryState
|
// Parse the event into RepositoryState
|
||||||
@@ -1377,6 +1381,80 @@ mod tests {
|
|||||||
use super::*;
|
use super::*;
|
||||||
use nostr_sdk::prelude::{EventBuilder, FinalizeEvent, Keys, Tag};
|
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<_>>(),
|
||||||
|
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 {
|
fn create_test_keys() -> Keys {
|
||||||
Keys::generate()
|
Keys::generate()
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -80,7 +80,7 @@ impl AnnouncementPolicy {
|
|||||||
match RepositoryAnnouncement::from_event(event.clone()) {
|
match RepositoryAnnouncement::from_event(event.clone()) {
|
||||||
Ok(announcement) => {
|
Ok(announcement) => {
|
||||||
// If this pubkey+identifier has a purgatory entry AND the incoming
|
// 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.
|
// removes our service. Clear the purgatory entry and its bare repo.
|
||||||
//
|
//
|
||||||
// If the incoming event is older than the purgatory entry (e.g. a
|
// If the incoming event is older than the purgatory entry (e.g. a
|
||||||
@@ -90,7 +90,11 @@ impl AnnouncementPolicy {
|
|||||||
.ctx
|
.ctx
|
||||||
.purgatory
|
.purgatory
|
||||||
.find_announcement(&event.pubkey, &announcement.identifier)
|
.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 {
|
if should_evict {
|
||||||
self.remove_purgatory_announcement(
|
self.remove_purgatory_announcement(
|
||||||
@@ -526,6 +530,74 @@ mod tests {
|
|||||||
.expect("signed announcement")
|
.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]
|
#[tokio::test]
|
||||||
async fn private_mode_rejects_announcement_from_nonmember_author() {
|
async fn private_mode_rejects_announcement_from_nonmember_author() {
|
||||||
let member = Keys::generate();
|
let member = Keys::generate();
|
||||||
|
|||||||
Reference in New Issue
Block a user