diff --git a/apps/origin-src/src/annotations/mod.rs b/apps/origin-src/src/annotations/mod.rs index 60da8af..70e6aaa 100644 --- a/apps/origin-src/src/annotations/mod.rs +++ b/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, + ) -> 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,6 +336,17 @@ 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), @@ -300,6 +354,9 @@ pub enum AnnotateError { #[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(); diff --git a/apps/origin-src/src/index.rs b/apps/origin-src/src/index.rs index 753bf3f..5cc64c5 100644 --- a/apps/origin-src/src/index.rs +++ b/apps/origin-src/src/index.rs @@ -828,9 +828,23 @@ impl Store

{ /// Locks the index: shared by batches being indexed, exclusive for a /// rebuild. fn lock_index(&self, mode: LockMode) -> Result { + 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 { + 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 { 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 diff --git a/apps/origin-src/src/workspace/annotate.rs b/apps/origin-src/src/workspace/annotate.rs index 210bd5a..3165ba6 100644 --- a/apps/origin-src/src/workspace/annotate.rs +++ b/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 --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 { 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(), }) } diff --git a/apps/origin-src/src/workspace/mod.rs b/apps/origin-src/src/workspace/mod.rs index ebe6725..4b643c9 100644 --- a/apps/origin-src/src/workspace/mod.rs +++ b/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( diff --git a/docs/design.md b/docs/design.md index d895877..512f753 100644 --- a/docs/design.md +++ b/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