sync: report a lost CAS race honestly instead of fabricating success
commit
1361d85sync: report a lost CAS race honestly instead of fabricating success
transfer::advance returned TxOutcome and both callers discarded it: on a CAS rejection, push answered Pushed::Advanced and fetch listed the ref in updated when nothing had been written — a silent success misreport of the exact staleness window pre-flight’s own framing admits (sync.pre-flight: a prediction that can only go stale). A rejected transaction now surfaces as Pushed::Stale on push and FetchReport::stale on fetch, both covered by racing-writer tests that advance the destination ref between the read (or passing verdict) and the transaction.
Also per the phase-boundary review: push’s rustdoc claimed the object closure copies only after pre-flight passes, but the code deliberately copies first so the gate can read the proposed objects — the doc now says so and states the redaction-relevant consequence (a refused push leaves unreferenced objects in the remote store); a transfer module-doc note records the receive.unit tension (destination refs advance through RefStore::transaction as stand-in plumbing until the remote side is receive-backed in phases 5-6); and the broken crate::Conflict intra-doc link plus the sync.sdoc citations (the file is sync.adoc) are fixed.
Assisted-by: Claude:claude-fable-5
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
crates/ents-sync/src/error.rs
@@ -3,8 +3,8 @@
//! An [`Error`] means sync could not *reach* a result — an object could
//! not be read or written, a ref store failed, a commit did not decode.
//! It is never a merge conflict or a negative verdict: a
-//! [`crate::Conflict`] and a [`ents_gate::Verdict::Fail`] are both reached
-//! results the caller acts on, not failures to compute one.
+//! [`crate::Merge::Conflict`] and a [`ents_gate::Verdict::Fail`] are both
+//! reached results the caller acts on, not failures to compute one.
use gix_hash::ObjectId;
crates/ents-sync/src/lib.rs
@@ -1,5 +1,5 @@
//! `sync`: remote synchronization for the forge — the one capability
-//! `git ents` adds over the local primitives (`docs/spec/sync.sdoc`).
+//! `git ents` adds over the local primitives (`docs/spec/sync.adoc`).
//!
//! Sync fetches and pushes `refs/meta/*` and, crucially, turns the gate's
//! verdict into a decision the user acts on before pushing. Its single hard
@@ -20,7 +20,7 @@
//!
//! # Spec coverage
//!
-//! From `docs/spec/sync.sdoc`:
+//! From `docs/spec/sync.adoc`:
//!
//! - `sync.forge-transfer` — [`transfer::fetch`], [`transfer::push`]: both
//! copy each meta-ref's full object closure, commit objects verbatim, so
crates/ents-sync/src/transfer.rs
@@ -15,6 +15,16 @@
//! [`push`] runs pre-flight against the remote's own policy before moving a
//! ref, so a rejected canonical push surfaces the inbox alternative instead
//! (`sync.pre-flight`, `sync.inbox-routing`).
+//!
+//! One known tension, deliberate at this phase: both directions advance the
+//! destination ref through [`RefStore::transaction`] directly, while
+//! `receive.unit` names `receive()` as the sole entry point through which a
+//! ref is mutated. The destination seam here is stand-in plumbing until the
+//! remote side is receive-backed — the hosted hook that calls `receive()`
+//! arrives with the phase-6 single-node root, and the local write path is
+//! already `ents-receive`'s — so until then no redaction ingest or effect
+//! enqueue happens on a transfer destination beyond what the boot-time
+//! reconciliation scan (`receive.reconstructible`) later recovers.
use ents_gate::Update;
use ents_model::MemberId;
@@ -52,6 +62,10 @@
pub unchanged: Vec<FullName>,
/// Refs whose local and remote tips diverged — resolve by merging.
pub diverged: Vec<Diverged>,
+ /// Refs whose CAS was rejected: another local writer moved the ref
+ /// between this fetch's read and its transaction, so nothing was
+ /// written. Re-running fetch re-classifies them against the new tip.
+ pub stale: Vec<FullName>,
}
/// Fetch every `refs/meta/*` ref from `remote` into `local`, moving the
@@ -66,9 +80,10 @@
///
/// # Errors
///
-/// Propagates ref-store and object failures; a stale CAS during the ref
-/// update surfaces as an [`crate::Error`]-free skip is *not* done here —
-/// fetch runs single-writer against the local store.
+/// Propagates ref-store and object failures. A rejected CAS — another
+/// local writer moved a ref between this fetch's read and its transaction
+/// — is not an error: the ref is reported in [`FetchReport::stale`] and
+/// nothing is written for it.
///
/// # Examples
///
@@ -108,23 +123,21 @@
match local_tip {
Some(local) if local == remote_tip => report.unchanged.push(name),
Some(local) if descends_from(local_objects, remote_tip, local)? => {
- advance(
- local_refs,
- &name,
- Expected::MustExistAndMatch(local),
- remote_tip,
- )?;
- report.updated.push(name);
+ let expected = Expected::MustExistAndMatch(local);
+ match advance(local_refs, &name, expected, remote_tip)? {
+ TxOutcome::Applied => report.updated.push(name),
+ TxOutcome::Rejected { .. } => report.stale.push(name),
+ }
}
Some(local) => report.diverged.push(Diverged {
name,
local,
remote: remote_tip,
}),
- None => {
- advance(local_refs, &name, Expected::MustNotExist, remote_tip)?;
- report.updated.push(name);
- }
+ None => match advance(local_refs, &name, Expected::MustNotExist, remote_tip)? {
+ TxOutcome::Applied => report.updated.push(name),
+ TxOutcome::Rejected { .. } => report.stale.push(name),
+ },
}
}
Ok(report)
@@ -144,18 +157,34 @@
/// divergence — merge first — or a refname mismatch). The ref was not
/// pushed; the prediction is carried for the caller to render.
Refused(Box<PreFlight>),
+ /// The pre-flight prediction went stale between judgment and CAS:
+ /// another writer advanced the remote ref, the transaction was
+ /// rejected, and nothing was written. This is exactly the staleness a
+ /// prediction admits (`sync.pre-flight`) — fetch, merge if divergent,
+ /// and push again.
+ Stale(FullName),
}
/// Push one local meta-ref `name` to `remote`, pre-flighting against the
/// remote's own policy first (`sync.pre-flight`).
///
-/// The local tip's object closure is copied to the remote only once
-/// pre-flight passes; a predicted rejection routes to the inbox
-/// (`sync.inbox-routing`) or is reported, and nothing is transferred. This
-/// runs the identical gate the remote will run at CAS time
-/// (`gate.call-sites`), so the result is a prediction that can only be
-/// stale, never wrong about the rules. Local writes are never blocked by
-/// this — that is [`mod@crate::preflight`]'s and the local store's concern
+/// The local tip's object closure is copied to the remote *before* the
+/// verdict is computed — the gate must be able to read the proposed
+/// objects, exactly as the hosted CAS judges after ingest — so a refused
+/// push deliberately leaves those objects in the remote object store even
+/// though no ref comes to point at them. That residue matters for
+/// redaction: recorded redaction targets refuse re-ingest at the `receive`
+/// boundary (`receive.redaction-ingest`), and purging unreferenced objects
+/// is the store's garbage collection, not this function's. Only the *ref*
+/// is gated: a predicted rejection routes to the inbox
+/// (`sync.inbox-routing`) or is reported, and the remote's refs are
+/// untouched. Pre-flight runs the identical gate the remote will run at
+/// CAS time (`gate.call-sites`), so the result is a prediction that can
+/// only be stale, never wrong about the rules — and when it *does* go
+/// stale (a racing writer advances the remote between judgment and CAS)
+/// the rejected transaction is reported as [`Pushed::Stale`], never as
+/// success. Local writes are never blocked by any of this — that is
+/// [`mod@crate::preflight`]'s and the local store's concern
/// (`sync.local-advisory`); push is the one place a verdict gates an
/// actual (remote) write.
///
@@ -189,8 +218,10 @@
let expected = remote_refs
.get(name.as_ref())?
.map_or(Expected::MustNotExist, Expected::MustExistAndMatch);
- advance(remote_refs, name, expected, local_tip)?;
- Ok(Pushed::Advanced(name.clone()))
+ match advance(remote_refs, name, expected, local_tip)? {
+ TxOutcome::Applied => Ok(Pushed::Advanced(name.clone())),
+ TxOutcome::Rejected { name } => Ok(Pushed::Stale(name)),
+ }
}
/// Apply one ref advance as a single-edit CAS transaction.
crates/ents-sync/tests/transfer.rs
@@ -264,3 +264,168 @@
"canonical ref must be untouched"
);
}
+
+/// A ref store that simulates a racing writer: the first transaction it
+/// receives is preceded by a competing ref move, landing exactly in the
+/// window between a caller's read (or pre-flight) and its CAS.
+struct RacingStore<'a> {
+ inner: &'a MemRefStore,
+ race: std::sync::Mutex<Option<(FullName, gix_hash::ObjectId)>>,
+}
+
+impl<'a> RacingStore<'a> {
+ fn new(inner: &'a MemRefStore, name: FullName, oid: gix_hash::ObjectId) -> Self {
+ Self {
+ inner,
+ race: std::sync::Mutex::new(Some((name, oid))),
+ }
+ }
+}
+
+impl gix_ref_store::RefStoreRead for RacingStore<'_> {
+ fn get(
+ &self,
+ name: &gix::refs::FullNameRef,
+ ) -> gix_ref_store::Result<Option<gix_hash::ObjectId>> {
+ self.inner.get(name)
+ }
+
+ fn iter_prefix(&self, prefix: &str) -> gix_ref_store::Result<gix_ref_store::RefIter> {
+ self.inner.iter_prefix(prefix)
+ }
+}
+
+impl gix_ref_store::RefStore for RacingStore<'_> {
+ #[expect(
+ clippy::unwrap_in_result,
+ reason = "test fixture: a poisoned mutex is a broken test, not a condition under test"
+ )]
+ fn transaction(
+ &self,
+ edits: &[gix_ref_store::RefEdit],
+ ) -> gix_ref_store::Result<gix_ref_store::TxOutcome> {
+ if let Some((name, oid)) = self.race.lock().unwrap().take() {
+ self.inner.set(name.as_ref(), oid);
+ }
+ self.inner.transaction(edits)
+ }
+}
+
+/// The staleness race pre-flight admits (`sync.pre-flight`: "a prediction
+/// that can only be stale"): another writer advances the remote ref between
+/// the passing verdict and the CAS. The rejected transaction must surface
+/// as [`Pushed::Stale`] — never as a fabricated success — and the racing
+/// writer's tip must survive untouched.
+// @relation(sync.pre-flight, scope=function, role=Verifies)
+#[test]
+fn push_reports_a_lost_cas_race_as_stale_not_success() {
+ let admin = Keypair::from_seed(1);
+ let remote_refs = MemRefStore::default();
+ let remote_objects = ObjectStore::default();
+ boot(&remote_refs, &remote_objects, &admin, None);
+
+ let local_refs = MemRefStore::default();
+ let local_objects = ObjectStore::default();
+ boot(&local_refs, &local_objects, &admin, None);
+
+ let name: FullName = "refs/meta/issues/1".try_into().unwrap();
+ let ours = write_meta_entity(
+ &local_refs,
+ &local_objects,
+ name.clone(),
+ &issue("open"),
+ Some(&admin),
+ 300,
+ );
+
+ // The racing writer's competing tip, landed on the remote the instant
+ // push's transaction begins — after pre-flight has already passed.
+ let racer = {
+ let tree = facet_git_tree::serialize_into(&issue("closed"), &remote_objects).unwrap();
+ write_commit(
+ &remote_objects,
+ &CommitSpec {
+ tree,
+ parents: vec![],
+ message: "racer".into(),
+ seconds: 310,
+ },
+ Some(&admin),
+ )
+ };
+ let racing = RacingStore::new(&remote_refs, name.clone(), racer);
+
+ let pushed = push(
+ &racing,
+ &remote_objects,
+ &local_objects,
+ &name,
+ ours,
+ &MemberId::new("admin"),
+ )
+ .unwrap();
+
+ assert_eq!(
+ pushed,
+ Pushed::Stale(name.clone()),
+ "a lost CAS race must not be reported as Advanced"
+ );
+ assert_eq!(
+ remote_refs.get(name.as_ref()).unwrap(),
+ Some(racer),
+ "the racing writer's tip must survive; nothing was written"
+ );
+}
+
+/// The same race on the fetch side: a local writer moves the ref between
+/// fetch's read and its transaction. The ref must land in
+/// [`ents_sync::transfer::FetchReport::stale`], never in `updated`, and the
+/// concurrent writer's tip must survive.
+// @relation(sync.forge-transfer, scope=function, role=Verifies)
+#[test]
+fn fetch_reports_a_lost_cas_race_as_stale_not_updated() {
+ let key = Keypair::from_seed(1);
+ let remote_refs = MemRefStore::default();
+ let remote_objects = ObjectStore::default();
+ let name: FullName = "refs/meta/issues/1".try_into().unwrap();
+ write_meta_entity(
+ &remote_refs,
+ &remote_objects,
+ name.clone(),
+ &issue("open"),
+ Some(&key),
+ 300,
+ );
+
+ let local_refs = MemRefStore::default();
+ let local_objects = ObjectStore::default();
+ // A concurrent local writer creates the same ref mid-fetch, defeating
+ // the MustNotExist precondition fetch read moments earlier.
+ let racer = {
+ let tree = facet_git_tree::serialize_into(&issue("closed"), &local_objects).unwrap();
+ write_commit(
+ &local_objects,
+ &CommitSpec {
+ tree,
+ parents: vec![],
+ message: "racer".into(),
+ seconds: 310,
+ },
+ Some(&key),
+ )
+ };
+ let racing = RacingStore::new(&local_refs, name.clone(), racer);
+
+ let report = fetch(&remote_refs, &remote_objects, &racing, &local_objects).unwrap();
+
+ assert!(
+ report.updated.is_empty(),
+ "a rejected CAS must not be reported as updated: {report:?}"
+ );
+ assert_eq!(report.stale, vec![name.clone()]);
+ assert_eq!(
+ local_refs.get(name.as_ref()).unwrap(),
+ Some(racer),
+ "the concurrent writer's tip must survive; nothing was written"
+ );
+}