From d8b85cbce5cba9bfb8b15a8bd5c1b7200ff3e488 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Thu, 26 Feb 2026 14:00:12 +0000 Subject: fix: advertise only state events with resolvable git objects git-remote-nostr now walks the per-relay state events captured in FetchReport::state_per_relay (newest first) and advertises the first one whose every OID is either present on at least one git server (confirmed via list_refs) or already available locally. If no such state event exists it falls back to the raw git server state. Previously the latest nostr state event was always used regardless of whether its OIDs had been pushed to any server, causing catastrophic missing-object errors during clone or fetch when a state event was published ahead of the corresponding git push. --- CHANGELOG.md | 1 + src/bin/git_remote_nostr/list.rs | 56 ++++++++- src/bin/git_remote_nostr/main.rs | 18 ++- tests/git_remote_nostr/list.rs | 261 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 327 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e4fdbd..4130ce2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `git-remote-nostr` list now advertises the newest state event whose OIDs are all confirmed present on a git server or locally, rather than unconditionally using the latest nostr state event; this prevents catastrophic fetch/clone failures when a state event was published before the corresponding git push completed - Tag tracking refs written with wrong path (`refs/remotes/origin/refs/tags/v1.0.0` instead of `refs/remotes/origin/v1.0.0`) after a push via `git-remote-nostr`, causing `ngit sync` to fail with "src refspec does not match any existing object" when syncing tags - `ngit sync` using wrong refspec source (`refs/remotes/origin/refs/heads/master` instead of `refs/remotes/origin/master`), causing sync to fail with "src refspec does not match any existing object" - State event publish failures silently swallowed during push; summary now shows `"Published to X/N relays (failed: relay1 relay2)"` instead of unconditional success message diff --git a/src/bin/git_remote_nostr/list.rs b/src/bin/git_remote_nostr/list.rs index 4a7c1ec..a32ed67 100644 --- a/src/bin/git_remote_nostr/list.rs +++ b/src/bin/git_remote_nostr/list.rs @@ -4,13 +4,14 @@ use anyhow::{Context, Result}; use client::get_state_from_cache; use git::RepoActions; use ngit::{ - client::{self, is_verbose}, + client::{self, FetchReport, is_verbose}, fetch::fetch_from_git_server, git::{self}, git_events::{KIND_PULL_REQUEST, KIND_PULL_REQUEST_UPDATE, event_to_cover_letter, tag_value}, list::list_from_remotes, login::get_curent_user, repo_ref::{self}, + repo_state::RepoState, utils::{get_all_proposals, get_open_or_draft_proposals}, }; use repo_ref::RepoRef; @@ -22,6 +23,7 @@ pub async fn run_list( git_repo: &Repo, repo_ref: &RepoRef, for_push: bool, + fetch_report: &FetchReport, ) -> Result, bool)>> { let nostr_state = (get_state_from_cache(Some(git_repo.get_path()?), repo_ref).await).ok(); @@ -30,6 +32,8 @@ pub async fn run_list( if is_verbose() { term.write_line("git servers: listing refs...")?; } + // nostr_state is passed to list_from_remotes only for the sync-status + // display; the actual ref state we advertise is determined below. let remote_states = list_from_remotes( &term, git_repo, @@ -39,9 +43,55 @@ pub async fn run_list( ) .await; - let mut state = if let Some(nostr_state) = nostr_state { - nostr_state.state + // Collect all OIDs confirmed present on at least one git server. + let git_server_oids: std::collections::HashSet = remote_states + .values() + .flat_map(|(state, _)| state.values()) + .filter(|v| !v.starts_with("ref: ")) + .cloned() + .collect(); + + // From the per-relay state events captured during the nostr fetch, find + // the newest state event whose every OID is either: + // (a) confirmed present on at least one git server, or + // (b) already available locally. + // This prevents advertising refs whose git objects haven't been pushed to + // any server yet, which would cause `git clone` / `git fetch` to fail. + let mut candidates: Vec<&nostr::Event> = fetch_report + .state_per_relay + .values() + .filter_map(|maybe| maybe.as_ref()) + .collect(); + // Sort newest-first (by created_at, then by id for tie-breaking). + candidates.sort_by(|a, b| { + b.created_at + .cmp(&a.created_at) + .then_with(|| b.id.cmp(&a.id)) + }); + // Deduplicate by event id so we don't check the same event twice. + candidates.dedup_by_key(|e| e.id); + + let best_state: Option> = candidates.into_iter().find_map(|event| { + if let Ok(rs) = RepoState::try_from(vec![event.clone()]) { + let all_resolvable = rs.state.values().all(|v| { + v.starts_with("ref: ") + || git_server_oids.contains(v) + || git_repo.does_commit_exist(v).is_ok_and(|exists| exists) + }); + if all_resolvable { Some(rs.state) } else { None } + } else { + None + } + }); + + let mut state = if let Some(state) = best_state { + state } else { + // No relay returned a state event whose OIDs are all resolvable + // (either no state events were seen on any relay, or every candidate + // references git objects not yet on any server). Fall back to + // whatever the git servers actually report so we never advertise OIDs + // that cannot be fetched. let (state, _is_grasp_server) = repo_ref .git_server .iter() diff --git a/src/bin/git_remote_nostr/main.rs b/src/bin/git_remote_nostr/main.rs index 6186ed3..dad8a99 100644 --- a/src/bin/git_remote_nostr/main.rs +++ b/src/bin/git_remote_nostr/main.rs @@ -12,7 +12,9 @@ use std::{ }; use anyhow::{Context, Result, bail}; -use client::{Connect, consolidate_fetch_reports, get_repo_ref_from_cache, is_verbose}; +use client::{ + Connect, FetchReport, consolidate_fetch_reports, get_repo_ref_from_cache, is_verbose, +}; use git::{RepoActions, nostr_url::NostrUrlDecoded}; use ngit::{ client::{self, Params}, @@ -149,7 +151,9 @@ async fn main() -> Result<()> { client.set_signer(signer).await; } - fetching_with_report_for_helper(git_repo_path, &client, &decoded_nostr_url.coordinate).await?; + let fetch_report = + fetching_with_report_for_helper(git_repo_path, &client, &decoded_nostr_url.coordinate) + .await?; let mut repo_ref = get_repo_ref_from_cache(Some(git_repo_path), &decoded_nostr_url.coordinate).await?; @@ -221,10 +225,12 @@ async fn main() -> Result<()> { push_options = PushOptions::default(); } ["list"] => { - list_outputs = Some(list::run_list(&git_repo, &repo_ref, false).await?); + list_outputs = + Some(list::run_list(&git_repo, &repo_ref, false, &fetch_report).await?); } ["list", "for-push"] => { - list_outputs = Some(list::run_list(&git_repo, &repo_ref, true).await?); + list_outputs = + Some(list::run_list(&git_repo, &repo_ref, true, &fetch_report).await?); } [] => { return Ok(()); @@ -283,7 +289,7 @@ async fn fetching_with_report_for_helper( git_repo_path: &Path, client: &Client, trusted_maintainer_coordinate: &Nip19Coordinate, -) -> Result<()> { +) -> Result { let term = console::Term::stderr(); let verbose = is_verbose(); if verbose { @@ -308,7 +314,7 @@ async fn fetching_with_report_for_helper( } else { term.write_line(&format!("nostr updates: {report}"))?; } - Ok(()) + Ok(report) } #[cfg(test)] diff --git a/tests/git_remote_nostr/list.rs b/tests/git_remote_nostr/list.rs index 88bd3f7..71e7114 100644 --- a/tests/git_remote_nostr/list.rs +++ b/tests/git_remote_nostr/list.rs @@ -151,6 +151,267 @@ mod with_state_announcement { Ok(()) } } + mod when_state_event_references_oids_not_on_git_server { + + use super::*; + + /// Regression test for the bug where a state event published ahead of + /// the corresponding `git push` caused `git clone` / `git fetch` to + /// fail with missing-object errors. + /// + /// The fix walks per-relay state events newest-first and picks the + /// first one whose every OID is either present on a git server or + /// already available locally. When no such event exists it falls back + /// to the raw git-server state. + #[tokio::test] + #[serial] + async fn falls_back_to_git_server_state() -> Result<()> { + // Build a real git repo that acts as the git server. + let source_git_repo = prep_git_repo()?; + std::fs::write(source_git_repo.dir.join("initial.md"), "initial")?; + let main_commit_id = source_git_repo.stage_and_commit("initial.md")?; + + // Craft a state event that claims main points at a commit that has + // NOT been pushed to the git server yet (a plausible OID that does + // not exist anywhere). + let fake_oid = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let root_commit = "9ee507fc4357d7ee16a5d8901bedcd103f23c17d"; + let state_event = nostr::event::EventBuilder::new(STATE_KIND, "") + .tags([ + nostr::Tag::identifier(format!("{root_commit}-consider-it-random")), + nostr::Tag::custom( + nostr::TagKind::Custom(std::borrow::Cow::Borrowed("HEAD")), + vec!["ref: refs/heads/main".to_string()], + ), + nostr::Tag::custom( + nostr::TagKind::Custom(std::borrow::Cow::Borrowed("refs/heads/main")), + vec![fake_oid.to_string()], + ), + ]) + .sign_with_keys(&TEST_KEY_1_KEYS) + .unwrap(); + + let git_repo = prep_git_repo()?; + let events = vec![ + generate_test_key_1_metadata_event("fred"), + generate_test_key_1_relay_list_event(), + generate_repo_ref_event_with_git_server(vec![ + source_git_repo.dir.to_str().unwrap().to_string(), + ]), + state_event, + ]; + // fallback (51,52) user write (53, 55) repo (55, 56) blaster (57) + let (mut r51, mut r52, mut r53, mut r55, mut r56, mut r57) = ( + Relay::new(8051, None, None), + Relay::new(8052, None, None), + Relay::new(8053, None, None), + Relay::new(8055, None, None), + Relay::new(8056, None, None), + Relay::new(8057, None, None), + ); + r51.events = events.clone(); + r55.events = events; + + let cli_tester_handle = std::thread::spawn(move || -> Result<()> { + let mut p = cli_tester_after_fetch(&git_repo)?; + p.send_line("list")?; + p.expect("git servers: listing refs...\r\n")?; + let res = p.expect_eventually("\r\n\r\n")?; + p.exit()?; + for p in [51, 52, 53, 55, 56, 57] { + relay::shutdown_relay(8000 + p)?; + } + let lines: HashSet = res + .split("\r\n") + .map(|e| e.to_string()) + .filter(|s| { + !s.contains("remote: ") + && !s.contains("Receiving objects") + && !s.contains("Resolving deltas") + && !s.contains("fetching /") + }) + .collect(); + // The fake OID must NOT appear – the list must fall back to + // what the git server actually has. + assert!( + !lines.iter().any(|l| l.contains(fake_oid)), + "fake OID from unresolvable state event must not be advertised; got: {lines:?}" + ); + // The real commit that IS on the git server must be advertised. + assert!( + lines.contains(&format!("{main_commit_id} refs/heads/main")), + "real git-server commit must be advertised; got: {lines:?}" + ); + Ok(()) + }); + // launch relays + let _ = join!( + r51.listen_until_close(), + r52.listen_until_close(), + r53.listen_until_close(), + r55.listen_until_close(), + r56.listen_until_close(), + r57.listen_until_close(), + ); + cli_tester_handle.join().unwrap()?; + Ok(()) + } + } + + mod when_newer_relay_state_has_missing_oid_but_older_relay_state_is_resolvable { + + use super::*; + + /// Two relays serve different state events; two git servers each have + /// different OIDs. + /// + /// - Relay 55 (repo relay A): **newer** state event → main = fake_oid + /// (not on any git server) + /// - Relay 56 (repo relay B): **older** state event → main = commit_a + /// (present on git_server_1) + /// - git_server_1: main = commit_a + /// - git_server_2: main = commit_b (a different real commit) + /// + /// Expected: `list` skips the newer unresolvable event and advertises + /// `commit_a` from the older-but-resolvable state event. + #[tokio::test] + #[serial] + async fn uses_older_resolvable_state_event() -> Result<()> { + // --- git_server_1: has commit_a on main --- + let git_server_1 = prep_git_repo()?; + std::fs::write(git_server_1.dir.join("server1.md"), "server1")?; + let commit_a = git_server_1.stage_and_commit("server1.md")?; + let bare_server_1 = GitTestRepo::recreate_as_bare(&git_server_1)?; + + // --- git_server_2: has commit_b on main (different commit) --- + let git_server_2 = prep_git_repo()?; + std::fs::write(git_server_2.dir.join("server2.md"), "server2")?; + let commit_b = git_server_2.stage_and_commit("server2.md")?; + let bare_server_2 = GitTestRepo::recreate_as_bare(&git_server_2)?; + + assert_ne!(commit_a, commit_b); + + let fake_oid = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; + let root_commit = "9ee507fc4357d7ee16a5d8901bedcd103f23c17d"; + let identifier = format!("{root_commit}-consider-it-random"); + + // Older state event: main = commit_a (resolvable via git_server_1) + let older_state_event = make_event_old_or_change_user( + nostr::event::EventBuilder::new(STATE_KIND, "") + .tags([ + nostr::Tag::identifier(identifier.clone()), + nostr::Tag::custom( + nostr::TagKind::Custom(std::borrow::Cow::Borrowed("HEAD")), + vec!["ref: refs/heads/main".to_string()], + ), + nostr::Tag::custom( + nostr::TagKind::Custom(std::borrow::Cow::Borrowed("refs/heads/main")), + vec![commit_a.to_string()], + ), + ]) + .sign_with_keys(&TEST_KEY_1_KEYS) + .unwrap(), + &TEST_KEY_1_KEYS, + 60, // 60 seconds old + ); + + // Newer state event: main = fake_oid (NOT on any git server) + let newer_state_event = nostr::event::EventBuilder::new(STATE_KIND, "") + .tags([ + nostr::Tag::identifier(identifier.clone()), + nostr::Tag::custom( + nostr::TagKind::Custom(std::borrow::Cow::Borrowed("HEAD")), + vec!["ref: refs/heads/main".to_string()], + ), + nostr::Tag::custom( + nostr::TagKind::Custom(std::borrow::Cow::Borrowed("refs/heads/main")), + vec![fake_oid.to_string()], + ), + ]) + .sign_with_keys(&TEST_KEY_1_KEYS) + .unwrap(); + + let git_repo = prep_git_repo()?; + + // Base events (metadata + relay list + repo ref) go on both relays. + let repo_ref_event = generate_repo_ref_event_with_git_server(vec![ + bare_server_1.dir.to_str().unwrap().to_string(), + bare_server_2.dir.to_str().unwrap().to_string(), + ]); + let base_events = vec![ + generate_test_key_1_metadata_event("fred"), + generate_test_key_1_relay_list_event(), + repo_ref_event, + ]; + + // fallback (51,52) user write (53, 55) repo (55, 56) blaster (57) + let (mut r51, mut r52, mut r53, mut r55, mut r56, mut r57) = ( + Relay::new(8051, None, None), + Relay::new(8052, None, None), + Relay::new(8053, None, None), + Relay::new(8055, None, None), + Relay::new(8056, None, None), + Relay::new(8057, None, None), + ); + r51.events = base_events.clone(); + // r55 (repo relay A) serves the newer state event with the fake OID + r55.events = [base_events.clone(), vec![newer_state_event]].concat(); + // r56 (repo relay B) serves the older state event with commit_a + r56.events = [base_events, vec![older_state_event]].concat(); + + let cli_tester_handle = std::thread::spawn(move || -> Result<()> { + let mut p = cli_tester_after_fetch(&git_repo)?; + p.send_line("list")?; + p.expect("git servers: listing refs...\r\n")?; + let res = p.expect_eventually("\r\n\r\n")?; + p.exit()?; + for p in [51, 52, 53, 55, 56, 57] { + relay::shutdown_relay(8000 + p)?; + } + let lines: HashSet = res + .split("\r\n") + .map(|e| e.to_string()) + .filter(|s| { + !s.contains("remote: ") + && !s.contains("Receiving objects") + && !s.contains("Resolving deltas") + && !s.contains("fetching /") + }) + .collect(); + // The fake OID from the newer-but-unresolvable state event must + // NOT appear. + assert!( + !lines.iter().any(|l| l.contains(fake_oid)), + "fake OID from newer unresolvable state event must not be advertised; got: {lines:?}" + ); + // commit_a from the older-but-resolvable state event must be + // advertised for main. + assert!( + lines.contains(&format!("{commit_a} refs/heads/main")), + "commit_a from older resolvable state event must be advertised; got: {lines:?}" + ); + // commit_b (only on git_server_2, not referenced by any state + // event) must NOT appear for main. + assert!( + !lines.contains(&format!("{commit_b} refs/heads/main")), + "commit_b from git_server_2 must not override the chosen state event; got: {lines:?}" + ); + Ok(()) + }); + // launch relays + let _ = join!( + r51.listen_until_close(), + r52.listen_until_close(), + r53.listen_until_close(), + r55.listen_until_close(), + r56.listen_until_close(), + r57.listen_until_close(), + ); + cli_tester_handle.join().unwrap()?; + Ok(()) + } + } + mod when_announcement_doesnt_match_git_server { use super::*; -- cgit v1.2.3