Skip to main content

max / makenotwork

server: publish a note with a compare-and-swap retry loop write_note is the whole write: resolve the tip, splice, commit onto it, and move the ref only if it still holds what the splice was built on. A writer who loses that race reloads and tries again against what the winner left, bounded at five attempts so a request thread cannot spin on a busy namespace. The commit a lost attempt wrote is simply unreachable, which is the cheapest possible rollback. Losing the race to somebody annotating a different commit is not a conflict, only a retry. Losing it on the same note is: the two versions are merged rather than one replacing the other, and the outcome says merged so the caller can tell the writer they are looking at something other than what they typed. The merge keeps their text intact and appends only the lines of the other version it does not already hold, because both versions were composed from the same starting text and a concatenation would print the untouched paragraph twice. It is a stand-in under an honest name: git's real strategies are P3, and this is the call site they land in. Saving an unedited note reports Unchanged and writes nothing, so the notes ref does not collect empty commits. Seven tests, including a deliberately contended engine that publishes ahead of the loop a fixed number of times, so the retry, the merge and the give-up are all exercised without threads.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-08 20:40 UTC
Signed with PGP, not checked
Commit: 0dbd3300255954632c5057242a1aef90ec0fa535
Parent: c436c51
2 files changed, +476 insertions, -0 deletions
M server/src/git/notes/mod.rs +166
@@ -570,6 +570,172 @@
570 570 ])
571 571 }
572 572
573 + // ── Publishing ──
574 +
575 + /// How many times a write reloads and retries after losing the ref race.
576 + ///
577 + /// Each attempt costs a tree walk and a commit, and every retry is another
578 + /// writer having got there first. Five is generous for a browser form; a
579 + /// namespace under enough contention to exhaust it is one the P3 inbox is meant
580 + /// to handle rather than one to keep spinning on.
581 + const MAX_WRITE_ATTEMPTS: usize = 5;
582 +
583 + /// What [`write_note`] did.
584 + #[derive(Debug, Clone, PartialEq, Eq)]
585 + pub enum Written {
586 + /// The namespace already said exactly this, so no commit was made. Saving
587 + /// an unedited note is the common way to get here.
588 + Unchanged,
589 + /// The namespace now points at `tip`.
590 + Committed {
591 + tip: Oid,
592 + /// Somebody else changed the same note while this one was being
593 + /// written, and the two were merged. The caller owes the writer a word
594 + /// about it: they are looking at a note that is not what they typed.
595 + merged: bool,
596 + },
597 + }
598 +
599 + /// Write, edit or remove one note, publishing it on `refs/notes/<namespace>`.
600 + ///
601 + /// `content` of `None` removes the note. `who` is both author and committer:
602 + /// the person doing it is the person who did it, and a notes commit has no
603 + /// distinct patch author to credit.
604 + ///
605 + /// The ref moves by compare-and-swap against the tip this write was built on,
606 + /// so a concurrent writer is never overwritten — the loser reloads and tries
607 + /// again against what the winner left. Two people annotating different commits
608 + /// both land. Two people annotating the *same* commit get their notes merged
609 + /// (see [`union`]) rather than one silently replacing the other.
610 + pub fn write_note<E: NoteObjects + NoteWrites>(
611 + engine: &E,
612 + namespace: &str,
613 + target: Oid,
614 + content: Option<&[u8]>,
615 + who: &Signature,
616 + ) -> Result<Written, NotesError> {
617 + let full_ref = format!("{NOTES_REF_PREFIX}{namespace}");
618 + // What the note said when this write was composed. A retry compares
619 + // against it to tell "somebody else edited this note" from "somebody else
620 + // edited a different one", which are the same lost race but not the same
621 + // situation.
622 + let mut base: Option<Oid> = None;
623 +
624 + for attempt in 0..MAX_WRITE_ATTEMPTS {
625 + let tip = engine.resolve_ref(&full_ref)?;
626 + let root = match tip {
627 + Some(tip) => Some(engine.read_commit(tip)?.tree),
628 + None => None,
629 + };
630 +
631 + let current = match root {
632 + Some(root) => find_note_blob(engine, root, target)?,
633 + None => None,
634 + };
635 + if attempt == 0 {
636 + base = current;
637 + }
638 +
639 + // The note moved under us and we are setting content, so what the
640 + // writer typed is no longer the whole story.
641 + let merged = attempt > 0 && current != base && content.is_some();
642 + let mut buffer;
643 + let content = match (merged, content, current) {
644 + (true, Some(ours), Some(theirs)) => {
645 + buffer = Vec::new();
646 + engine.read_blob_into(theirs, &mut buffer)?;
647 + buffer = union(&buffer, ours);
648 + Some(buffer.as_slice())
649 + }
650 + _ => content,
651 + };
652 +
653 + let blob = match content {
654 + Some(content) => Some(engine.write_blob(content)?),
655 + None => None,
656 + };
657 + let Some(new_root) = splice_note(engine, root, target, blob)? else {
658 + return Ok(Written::Unchanged);
659 + };
660 +
661 + let commit = engine.write_commit(
662 + new_root,
663 + tip.as_slice(),
664 + who,
665 + who,
666 + &commit_message(target, content.is_some(), merged),
667 + )?;
668 +
669 + match engine.update_ref_cas(&full_ref, tip, commit) {
670 + Ok(()) => {
671 + return Ok(Written::Committed {
672 + tip: commit,
673 + merged,
674 + });
675 + }
676 + // Somebody published between the read and the update. The commit we
677 + // just wrote is unreachable and will be collected; building the next
678 + // attempt on the tip they left is the whole point of the loop.
679 + Err(NotesError::Raced) => {}
680 + Err(other) => return Err(other),
681 + }
682 + }
683 +
684 + Err(NotesError::Raced)
685 + }
686 +
687 + /// Combine two versions of the same note.
688 + ///
689 + /// Keeps what the writer typed intact, then appends any line of the other
690 + /// version it does not already contain. The shared text both versions started
691 + /// from is in `ours` already, which is why appending only the difference
692 + /// matters: a plain concatenation would double the paragraph neither of them
693 + /// touched.
694 + ///
695 + /// This is a stand-in with an honest name, not git's `union` strategy. The real
696 + /// set — `manual`, `ours`, `theirs`, `union`, `cat_sort_uniq`, byte-exact with
697 + /// git's — is P3, and this call site is where they land. Losing nothing is the
698 + /// property worth having until then.
699 + fn union(theirs: &[u8], ours: &[u8]) -> Vec<u8> {
700 + let mine: HashSet<&[u8]> = ours.split(|b| *b == b'\n').collect();
701 + let extra: Vec<&[u8]> = theirs
702 + .split(|b| *b == b'\n')
703 + .filter(|line| !line.is_empty() && !mine.contains(line))
704 + .collect();
705 + if extra.is_empty() {
706 + return ours.to_vec();
707 + }
708 +
709 + let mut out = ours.to_vec();
710 + if !out.ends_with(b"\n") {
711 + out.push(b'\n');
712 + }
713 + out.push(b'\n');
714 + for line in extra {
715 + out.extend_from_slice(line);
716 + out.push(b'\n');
717 + }
718 + out
719 + }
720 +
721 + /// The message on a notes commit.
722 + ///
723 + /// Short and factual. Nobody reads the notes ref's log for prose, but they do
724 + /// read it to find which commit touched a note, and the target ID is what makes
725 + /// that possible without a diff.
726 + fn commit_message(target: Oid, setting: bool, merged: bool) -> String {
727 + let verb = if setting {
728 + "annotate"
729 + } else {
730 + "remove the note on"
731 + };
732 + let mut message = format!("notes: {verb} {}\n", target.to_short_hex(12));
733 + if merged {
734 + message.push_str("\nMerged with a concurrent edit of the same note.\n");
735 + }
736 + message
737 + }
738 +
573 739 // ── Attribution ──
574 740
575 741 /// Find the notes commit that set `target`'s note to what it is now.
M server/src/git/notes/tests.rs +310
@@ -730,6 +730,316 @@
730 730 }
731 731 }
732 732
733 + // ── Publishing under contention ──
734 +
735 + /// An engine that lets somebody else publish first.
736 + ///
737 + /// Every method delegates, except that the first `remaining` compare-and-swap
738 + /// attempts are preceded by a competing write landing on the same ref. That
739 + /// turns the race into something a test can state exactly: without it, the
740 + /// retry loop is only reachable by running two threads and hoping.
741 + struct Contended<'repo> {
742 + inner: GixEngine<'repo>,
743 + remaining: std::cell::Cell<usize>,
744 + /// What the other writer annotates, and with what.
745 + target: Oid,
746 + text: &'static str,
747 + /// Whether each interference says something new. A writer repeating itself
748 + /// stops moving the ref after the first time — its own splice becomes a
749 + /// no-op — so unending contention has to keep changing its mind.
750 + vary: bool,
751 + }
752 +
753 + impl<'repo> Contended<'repo> {
754 + fn new(repo: &'repo gix::Repository, times: usize, target: Oid, text: &'static str) -> Self {
755 + Self {
756 + inner: GixEngine::new(repo),
757 + remaining: std::cell::Cell::new(times),
758 + target,
759 + text,
760 + vary: false,
761 + }
762 + }
763 +
764 + /// Somebody who publishes ahead of us every single time.
765 + fn relentless(repo: &'repo gix::Repository, target: Oid, text: &'static str) -> Self {
766 + Self {
767 + vary: true,
768 + ..Self::new(repo, usize::MAX, target, text)
769 + }
770 + }
771 +
772 + /// The other writer's whole turn: read the tip, splice, commit, publish.
773 + fn interfere(&self) {
774 + let full_ref = format!("{NOTES_REF_PREFIX}{DEFAULT_NAMESPACE}");
775 + let tip = self.inner.resolve_ref(&full_ref).unwrap();
776 + let root = tip.map(|tip| self.inner.read_commit(tip).unwrap().tree);
777 +
778 + let content = if self.vary {
779 + format!("{} {}", self.text, self.remaining.get())
780 + } else {
781 + self.text.to_string()
782 + };
783 + let blob = self.inner.write_blob(content.as_bytes()).unwrap();
784 + let Some(new_root) = splice_note(&self.inner, root, self.target, Some(blob)).unwrap()
785 + else {
786 + return;
787 + };
788 +
789 + let who = signature("Someone Else");
790 + let commit = self
791 + .inner
792 + .write_commit(new_root, tip.as_slice(), &who, &who, "notes: theirs\n")
793 + .unwrap();
794 + self.inner.update_ref_cas(&full_ref, tip, commit).unwrap();
795 + }
796 + }
797 +
798 + impl NoteObjects for Contended<'_> {
799 + fn resolve_ref(&self, full_name: &str) -> Result<Option<Oid>, NotesError> {
800 + self.inner.resolve_ref(full_name)
801 + }
802 + fn list_refs(&self, prefix: &str, visit: &mut dyn FnMut(&str, Oid)) -> Result<(), NotesError> {
803 + self.inner.list_refs(prefix, visit)
804 + }
805 + fn read_commit(&self, oid: Oid) -> Result<CommitMeta, NotesError> {
806 + self.inner.read_commit(oid)
807 + }
808 + fn read_tree_with(
809 + &self,
810 + oid: Oid,
811 + visit: &mut dyn FnMut(TreeEntry<'_>) -> Walk,
812 + ) -> Result<(), NotesError> {
813 + self.inner.read_tree_with(oid, visit)
814 + }
815 + fn read_blob_into(&self, oid: Oid, out: &mut Vec<u8>) -> Result<(), NotesError> {
816 + self.inner.read_blob_into(oid, out)
817 + }
818 + }
819 +
820 + impl NoteWrites for Contended<'_> {
821 + fn write_blob(&self, content: &[u8]) -> Result<Oid, NotesError> {
822 + self.inner.write_blob(content)
823 + }
824 + fn write_tree(&self, entries: &[NewEntry]) -> Result<Oid, NotesError> {
825 + self.inner.write_tree(entries)
826 + }
827 + fn write_commit(
828 + &self,
829 + tree: Oid,
830 + parents: &[Oid],
831 + author: &Signature,
832 + committer: &Signature,
833 + message: &str,
834 + ) -> Result<Oid, NotesError> {
835 + self.inner
836 + .write_commit(tree, parents, author, committer, message)
837 + }
838 + fn update_ref_cas(
839 + &self,
840 + full_name: &str,
841 + expected: Option<Oid>,
842 + new: Oid,
843 + ) -> Result<(), NotesError> {
844 + if self.remaining.get() > 0 {
845 + self.remaining.set(self.remaining.get() - 1);
846 + self.interfere();
847 + }
848 + self.inner.update_ref_cas(full_name, expected, new)
849 + }
850 + }
851 +
852 + #[test]
853 + fn a_write_creates_the_namespace_and_reads_back() {
854 + let (_tmp, repo) = init_bare();
855 + let engine = GixEngine::new(&repo);
856 +
857 + let written = write_note(
858 + &engine,
859 + DEFAULT_NAMESPACE,
860 + oid(T1),
861 + Some(b"the first annotation"),
862 + &signature("Max"),
863 + )
864 + .unwrap();
865 +
866 + let Written::Committed { tip, merged } = written else {
867 + panic!("expected a commit, got {written:?}");
868 + };
869 + assert!(!merged);
870 +
871 + let ns = namespace(&engine, DEFAULT_NAMESPACE);
872 + assert_eq!(ns.tip, tip);
873 + let note = note_for(&engine, ns.tip, oid(T1)).unwrap().unwrap();
874 + assert_eq!(note.content_lossy(), "the first annotation");
875 +
876 + // A root commit: the namespace had no history to build on.
877 + assert!(engine.read_commit(tip).unwrap().parents.is_empty());
878 + assert_eq!(engine.read_commit(tip).unwrap().committer.name, "Max");
879 + }
880 +
881 + #[test]
882 + fn rewriting_the_same_text_does_not_commit() {
883 + let (_tmp, repo) = init_bare();
884 + let engine = GixEngine::new(&repo);
885 + let who = signature("Max");
886 +
887 + write_note(&engine, DEFAULT_NAMESPACE, oid(T1), Some(b"same"), &who).unwrap();
888 + let before = namespace(&engine, DEFAULT_NAMESPACE).tip;
889 +
890 + // Pressing save on an untouched form must not add a commit to the notes
891 + // ref, or the history of a note becomes unreadable.
892 + let again = write_note(&engine, DEFAULT_NAMESPACE, oid(T1), Some(b"same"), &who).unwrap();
893 + assert_eq!(again, Written::Unchanged);
894 + assert_eq!(namespace(&engine, DEFAULT_NAMESPACE).tip, before);
895 + }
896 +
897 + #[test]
898 + fn removing_a_note_commits_and_leaves_the_others() {
899 + let (_tmp, repo) = init_bare();
900 + let engine = GixEngine::new(&repo);
901 + let who = signature("Max");
902 +
903 + write_note(&engine, DEFAULT_NAMESPACE, oid(T1), Some(b"one"), &who).unwrap();
904 + write_note(&engine, DEFAULT_NAMESPACE, oid(T3), Some(b"three"), &who).unwrap();
905 + let removed = write_note(&engine, DEFAULT_NAMESPACE, oid(T1), None, &who).unwrap();
906 + assert!(matches!(removed, Written::Committed { merged: false, .. }));
907 +
908 + let ns = namespace(&engine, DEFAULT_NAMESPACE);
909 + assert!(note_for(&engine, ns.tip, oid(T1)).unwrap().is_none());
910 + assert!(note_for(&engine, ns.tip, oid(T3)).unwrap().is_some());
911 +
912 + // Removing it twice is not an error, and the second time is not a commit.
913 + let again = write_note(&engine, DEFAULT_NAMESPACE, oid(T1), None, &who).unwrap();
914 + assert_eq!(again, Written::Unchanged);
915 + }
916 +
917 + #[test]
918 + fn a_lost_race_on_another_note_retries_and_keeps_both() {
919 + let (_tmp, repo) = init_bare();
920 + let engine = Contended::new(&repo, 1, oid(T3), "their note on a different commit");
921 +
922 + let written = write_note(
923 + &engine,
924 + DEFAULT_NAMESPACE,
925 + oid(T1),
926 + Some(b"our note"),
927 + &signature("Max"),
928 + )
929 + .unwrap();
930 +
931 + // Two people annotating different commits is not a conflict, only a lost
932 + // ref race. Both notes survive and nothing is reported as merged.
933 + assert!(matches!(written, Written::Committed { merged: false, .. }));
934 + let ns = namespace(&engine.inner, DEFAULT_NAMESPACE);
935 + assert_eq!(
936 + note_for(&engine.inner, ns.tip, oid(T1))
937 + .unwrap()
938 + .unwrap()
939 + .content_lossy(),
940 + "our note"
941 + );
942 + assert_eq!(
943 + note_for(&engine.inner, ns.tip, oid(T3))
944 + .unwrap()
945 + .unwrap()
946 + .content_lossy(),
947 + "their note on a different commit"
948 + );
949 + }
950 +
951 + #[test]
952 + fn a_lost_race_on_the_same_note_merges_rather_than_overwrites() {
953 + let (_tmp, repo) = init_bare();
954 + let engine = Contended::new(&repo, 1, oid(T1), "a line only they wrote");
955 +
956 + let written = write_note(
957 + &engine,
958 + DEFAULT_NAMESPACE,
959 + oid(T1),
960 + Some(b"a line only we wrote"),
961 + &signature("Max"),
962 + )
963 + .unwrap();
964 +
965 + let Written::Committed { tip, merged } = written else {
966 + panic!("expected a commit, got {written:?}");
967 + };
968 + assert!(merged, "the caller has to be able to say so");
969 +
970 + // Neither writer loses their text. Which of the two an eventual merge
971 + // strategy prefers is P3's decision; not dropping one is this loop's.
972 + let note = note_for(&engine.inner, tip, oid(T1)).unwrap().unwrap();
973 + let content = note.content_lossy();
974 + assert!(content.contains("a line only we wrote"), "{content}");
975 + assert!(content.contains("a line only they wrote"), "{content}");
976 + }
977 +
978 + #[test]
979 + fn a_merge_does_not_double_the_text_both_writers_started_from() {
980 + let (_tmp, repo) = init_bare();
981 + let who = signature("Max");
982 + {
983 + let engine = GixEngine::new(&repo);
984 + write_note(
985 + &engine,
986 + DEFAULT_NAMESPACE,
987 + oid(T1),
988 + Some(b"shared paragraph\n"),
989 + &who,
990 + )
991 + .unwrap();
992 + }
993 +
994 + // Both writers loaded "shared paragraph" and added a line under it. A
995 + // concatenating merge would produce it twice, which is what makes a raced
996 + // note look corrupted rather than merged.
997 + let engine = Contended::new(&repo, 1, oid(T1), "shared paragraph\ntheir addition\n");
998 + let written = write_note(
999 + &engine,
1000 + DEFAULT_NAMESPACE,
1001 + oid(T1),
1002 + Some(b"shared paragraph\nour addition\n"),
1003 + &who,
1004 + )
1005 + .unwrap();
1006 +
1007 + let Written::Committed { tip, merged } = written else {
1008 + panic!("expected a commit, got {written:?}");
1009 + };
1010 + assert!(merged);
1011 + let note = note_for(&engine.inner, tip, oid(T1)).unwrap().unwrap();
1012 + let content = note.content_lossy();
1013 + assert_eq!(
1014 + content.matches("shared paragraph").count(),
1015 + 1,
1016 + "the common text was duplicated: {content}"
1017 + );
1018 + assert!(content.contains("our addition"), "{content}");
1019 + assert!(content.contains("their addition"), "{content}");
1020 + }
1021 +
1022 + #[test]
1023 + fn a_write_gives_up_rather_than_spinning_forever() {
1024 + let (_tmp, repo) = init_bare();
1025 + // A writer that never stops publishing. Retrying without a bound would hang
1026 + // the request thread on it.
1027 + let engine = Contended::relentless(&repo, oid(T3), "again");
1028 +
1029 + let outcome = write_note(
1030 + &engine,
1031 + DEFAULT_NAMESPACE,
1032 + oid(T1),
1033 + Some(b"ours"),
1034 + &signature("Max"),
1035 + );
1036 + assert!(matches!(outcome, Err(NotesError::Raced)), "{outcome:?}");
1037 +
1038 + // And it left nothing behind: the ref carries only the other writer's work.
1039 + let ns = namespace(&engine.inner, DEFAULT_NAMESPACE);
1040 + assert!(note_for(&engine.inner, ns.tip, oid(T1)).unwrap().is_none());
1041 + }
1042 +
733 1043 // ── The engine's write half ──
734 1044
735 1045 #[test]