fix(annotations): stop two annotate runs from both superseding one statement

apps/origin-src/src/annotations/mod.rs

@@ -6,7 +6,9 @@
 //! [`Store::authorizes`] names. Every other kind belongs to whoever issues
 //! it. Supersession counts only within one issuer: a statement claiming to
 //! supersede another issuer's is issued like any other, and the one it
-//! names still stands.
+//! names still stands. [`Annotations::supersede`] supersedes an issuer's
+//! current statement atomically, so the statements of one kind about one
+//! record it issues in one store form a single chain.
 
 mod promotion;
 
@@ -97,6 +99,47 @@ impl<'a, P: Platform> Annotations<'a, P> {
         Ok(self.write(statement, payload)?)
     }
 
+    /// Issues `statement` as its issuer's latest of its kind about its
+    /// subject, superseding the [current](Self::current) one, and returns
+    /// it with its id. One without a verdict, of a kind that isn't
+    /// reserved, withdraws the current one. It reads the current one and
+    /// issues under a lock, from the index read again, so two statements
+    /// issued at once in this store never supersede the same one: the
+    /// second supersedes the first. Statements issued in other stores and
+    /// put here chose what they supersede there, so two of one issuer's can
+    /// still both supersede the same one.
+    ///
+    /// # Errors
+    ///
+    /// Fails like [`Annotations::issue`], or with
+    /// [`AnnotateError::NothingToWithdraw`] if a withdrawal finds no
+    /// current statement with a verdict.
+    pub fn supersede(
+        &mut self,
+        mut statement: Statement,
+        payload: Option<EncodedObject>,
+    ) -> Result<(StatementId, Statement), AnnotateError> {
+        let _lock = self.store.lock_supersession()?;
+        self.index = Index::open(self.store)?;
+        statement.supersedes =
+            self.current(statement.subject, &statement.kind, &statement.issuer)?;
+        if statement.verdict.is_none() && !statement.kind.is_reserved() {
+            // A withdrawal takes back a statement with a verdict.
+            let withdrawable = match statement.supersedes {
+                Some(current) => !self.store.get(current)?.withdraws(),
+                None => false,
+            };
+            if !withdrawable {
+                return Err(AnnotateError::NothingToWithdraw {
+                    kind: statement.kind,
+                    subject: statement.subject,
+                });
+            }
+        }
+        let id = self.issue(&statement, payload)?;
+        Ok((id, statement))
+    }
+
     /// Writes a statement after its payload, which is durable before
     /// anything refers to it.
     fn write(
@@ -293,13 +336,27 @@ pub enum AnnotateError {
         /// Who issued it.
         issuer: Principal,
     },
+    /// A withdrawal found no statement of its issuer's to take back.
+    #[error(
+        "you have no `{kind}` statement about {} to withdraw",
+        subject.describe()
+    )]
+    NothingToWithdraw {
+        /// The kind.
+        kind: Kind,
+        /// What it would be about: a revision or a statement.
+        subject: RecordId,
+    },
     /// Records cannot be read or written.
     #[error(transparent)]
     Store(#[from] StoreError),
 }
 
 #[cfg(test)]
 mod tests {
+    use std::sync::Barrier;
+    use std::thread;
+
     use origin_shared::{Base, BlobId, Digest, Inputs, ObjectId, RepoPath, Revision, Verdict};
 
     use super::*;
@@ -395,6 +452,36 @@ mod tests {
 
     pub(super) const FILES: &[(&str, &str)] = &[("src/lib.rs", "v1"), ("docs/guide.md", "v1")];
 
+    #[test]
+    fn statements_issued_at_once_supersede_one_another() {
+        let (_dir, store) = store();
+        let revision = revision(&store, 1, &[], FILES);
+        let rounds = 50;
+        for _ in 0..rounds {
+            let barrier = Barrier::new(2);
+            thread::scope(|scope| {
+                // Different verdicts, so the two never make one statement.
+                for verdict in [Verdict::Pass, Verdict::Fail] {
+                    let (store, barrier) = (&store, &barrier);
+                    scope.spawn(move || {
+                        let mut annotations = Annotations::open(store).unwrap();
+                        let statement = check(revision, "review", verdict, "user:alice", None);
+                        barrier.wait();
+                        annotations.supersede(statement, None).unwrap();
+                    });
+                }
+            });
+        }
+        let about = Annotations::open(&store)
+            .unwrap()
+            .about(RecordId::Revision(revision))
+            .unwrap();
+        assert_eq!(about.len(), 2 * rounds);
+        for pair in about.windows(2) {
+            assert_eq!(pair[1].statement.supersedes, Some(pair[0].id));
+        }
+    }
+
     #[test]
     fn only_the_issuer_supersedes_its_results() {
         let (_dir, store) = store();

apps/origin-src/src/index.rs

@@ -828,9 +828,23 @@ impl<P: Platform> Store<P> {
     /// Locks the index: shared by batches being indexed, exclusive for a
     /// rebuild.
     fn lock_index(&self, mode: LockMode) -> Result<LockedFile, StoreError> {
+        self.lock_in_index("lock", mode)
+    }
+
+    /// Locks supersession, exclusively: see [`Annotations::supersede`].
+    /// It's a lock of its own, since issuing takes the index's, on another
+    /// handle, which `flock` conflicts with even within one process.
+    ///
+    /// [`Annotations::supersede`]: crate::annotations::Annotations::supersede
+    pub(crate) fn lock_supersession(&self) -> Result<LockedFile, StoreError> {
+        self.lock_in_index("supersession", LockMode::Exclusive)
+    }
+
+    /// Locks the file `name` in the index's directory in `mode`.
+    fn lock_in_index(&self, name: &str, mode: LockMode) -> Result<LockedFile, StoreError> {
         let dir = self.index_dir();
         fs::create_dir_all(&dir).map_err(|source| StoreError::io(&dir, source))?;
-        Ok(LockedFile::open(&self.platform, &dir.join("lock"), mode)?)
+        Ok(LockedFile::open(&self.platform, &dir.join(name), mode)?)
     }
 
     /// Durably replaces the index with one holding `entries` and

apps/origin-src/src/workspace/annotate.rs

@@ -12,7 +12,7 @@ use origin_shared::{
 };
 
 use super::{WorkingCopy, Workspace, WorkspaceError};
-use crate::annotations::Annotations;
+use crate::annotations::{AnnotateError, Annotations};
 use crate::index::{FindError, Index};
 use crate::store::StoreError;
 use crate::tree::Snapshots;
@@ -112,8 +112,9 @@ impl Workspace {
 
     /// `origin annotate <rev> <kind> --pass|--fail|--neutral|--withdraw`:
     /// issues a statement about a revision as the user, superseding the
-    /// user's latest one of the same kind about it. Without a `verdict`, it
-    /// withdraws that one: see [`Statement::withdraws`].
+    /// user's latest one of the same kind about it, as
+    /// [`Annotations::supersede`] does. Without a `verdict`, it withdraws
+    /// that one: see [`Statement::withdraws`].
     ///
     /// # Errors
     ///
@@ -132,47 +133,45 @@ impl Workspace {
             return Err(WorkspaceError::ReservedKind(kind));
         }
         let working_copy = self.refresh()?;
-        let annotations = Annotations::open(&self.store)?;
+        let mut annotations = Annotations::open(&self.store)?;
         let subject = working_copy.find_record(annotations.index(), annotated)?;
-        let issuer = self.config.user();
-        let supersedes = annotations.current(subject, &kind, &issuer)?;
-        // What a withdrawal takes back: a statement with a verdict.
-        let withdrawn = match (verdict, supersedes) {
-            (Some(_), _) => None,
-            (None, Some(id)) if !self.store.get(id)?.withdraws() => Some(id),
-            (None, _) => return Err(self.nothing_to_withdraw(&annotations, kind, subject)?),
-        };
         let payload = payload.map(Payload::blob).transpose()?;
         let statement = Statement {
             subject,
-            supersedes,
+            supersedes: None,
             kind,
             verdict,
-            issuer,
+            issuer: self.config.user(),
             inputs,
             payload: payload.as_ref().map(|payload| payload.id().into()),
             promoted_from: None,
         };
-        let id = annotations.issue(&statement, payload)?;
-        if let Some(verdict) = verdict {
-            println!(
-                "Issued statement {}: {} {verdict} about {}",
-                id.short(),
-                statement.kind,
-                subject.describe()
-            );
-            if let Some(superseded) = statement.supersedes {
-                println!("Supersedes statement {}", superseded.short());
+        let (id, statement) = match annotations.supersede(statement, payload) {
+            Err(AnnotateError::NothingToWithdraw { kind, subject }) => {
+                return Err(self.nothing_to_withdraw(&annotations, kind, subject)?);
             }
-        }
-        if let Some(withdrawn) = withdrawn {
-            println!(
+            issued => issued?,
+        };
+        match (statement.verdict, statement.supersedes) {
+            (Some(verdict), superseded) => {
+                println!(
+                    "Issued statement {}: {} {verdict} about {}",
+                    id.short(),
+                    statement.kind,
+                    subject.describe()
+                );
+                if let Some(superseded) = superseded {
+                    println!("Supersedes statement {}", superseded.short());
+                }
+            }
+            (None, Some(withdrawn)) => println!(
                 "Withdrew statement {} with statement {}: {} about {}",
                 withdrawn.short(),
                 id.short(),
                 statement.kind,
                 subject.describe()
-            );
+            ),
+            (None, None) => {}
         }
         if let (Some(inputs), RecordId::Revision(revision)) = (&statement.inputs, subject) {
             let snapshot = self.store.get(revision)?.snapshot;
@@ -196,7 +195,7 @@ impl Workspace {
         subject: RecordId,
     ) -> Result<WorkspaceError, WorkspaceError> {
         let RecordId::Revision(revision) = subject else {
-            return Ok(WorkspaceError::NothingToWithdraw { kind, subject });
+            return Ok(AnnotateError::NothingToWithdraw { kind, subject }.into());
         };
         let user = self.config.user();
         let claims = annotations.claims(RecordId::Revision(revision))?;
@@ -218,7 +217,7 @@ impl Workspace {
                 original,
                 from,
             },
-            None => WorkspaceError::NothingToWithdraw { kind, subject },
+            None => AnnotateError::NothingToWithdraw { kind, subject }.into(),
         })
     }
 

apps/origin-src/src/workspace/mod.rs

@@ -914,17 +914,6 @@ pub enum WorkspaceError {
         /// The revision it names.
         revision: RevisionId,
     },
-    /// `annotate --withdraw` found no statement of the user's to take back.
-    #[error(
-        "you have no `{kind}` statement about {} to withdraw",
-        subject.describe()
-    )]
-    NothingToWithdraw {
-        /// The kind.
-        kind: Kind,
-        /// What it would be about: a revision or a statement.
-        subject: RecordId,
-    },
     /// `annotate --withdraw` found only a promotion of the user's
     /// statement, which they withdraw about the revision it's about.
     #[error(

docs/design.md

@@ -207,7 +207,12 @@ Rules:
   claim to supersede someone else's is ignored. Statements from different
   issuers never overwrite each other, so a review from Alice and one from Bob
   both stand. `origin annotate` supersedes the user's latest statement of the
-  same kind about that revision. A statement is taken back by superseding
+  same kind about that revision, reading it and issuing the new one under a
+  lock, so two run at once in one store never supersede the same one: the
+  second supersedes the first. A statement issued elsewhere, in a clone or by
+  a CI runner over the API, supersedes what that issuer saw, so two of one
+  issuer's statements from different stores can still both supersede the
+  same one. A statement is taken back by superseding
   it with one that has no verdict (`--withdraw`), which claims nothing: it
   never counts as evidence, and nothing promotes it.
 - **Statements are permanent.** Reviews, check results and conflicts are