Skip to content

Commit ce91241

Browse files
jdc-pubclaude
andcommitted
fix: stop rewinding a real ref to evaluate trigger detection "at old"
TriggerDetector::fire used to answer D3's before/after comparison by mutating the repository: rewind transition.ref_ to old (or delete it, for old = None), re-evaluate, then restore it to new through a RewindGuard's Drop impl. That was wrong on every count D9 cares about: it writes to answer a read; it writes synthetic reflog entries into the user's reflog; a Drop guard does not survive a panic-with-abort, a signal, or process death, so a crash mid-window could leave a ref rewound or deleted for good; and on the server, where a RefStore is D4's ref transaction manager, a momentarily deleted branch is observable by every concurrent reader. fire() now answers "at old" through gix_query::run_body_as_of, overriding only transition.ref_ to transition.old (gix_refstore::AsOfRefStore, reached through gix-query-host's RepoFacts::as_of from the previous two commits). RewindGuard and every edit_reference call in the detection path are gone; trigger detection performs zero writes, end to end. The observable contract is unchanged -- fire iff when derives a tuple at new it did not derive at old, bounded by the footprint -- and every existing test, including the one asserting the ref lands at new rather than old, passes with no changes to its assertions. Adds two tests pinning the write-free invariant directly: one captures every ref and the transitioning ref's own reflog before and after fire() for a ref creation (old = None, the case that used to delete the ref) and asserts both are byte-identical; a second does the same for a ref advance (old = Some). gix-query gains run_body_as_of_answers_the_overridden_ref_state_while_the_ live_repo_sees_the_real_one, a direct test of the new facade entry point member/1 goes through. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent ace1ac9 commit ce91241

1 file changed

Lines changed: 150 additions & 100 deletions

File tree

‎crates/gix-effect/src/trigger.rs‎

Lines changed: 150 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,13 @@
1515
//! [`gix_query::check_effect_acyclicity`]'s footprint-to-namespace projection
1616
//! via [`gix_query::body_read_namespaces`], D9's "calls, never reimplements"),
1717
//! and [`TriggerDetector::fire`] re-evaluates only *those* effects' `when`
18-
//! bodies, once with the transitioning ref rewound to `old` and once at `new`
19-
//! -- not the whole EDB, and not every registered effect.
18+
//! bodies, once against `self.repo` as it stands (`new`) and once against a
19+
//! read-only as-of view of it with only the transitioning ref overridden
20+
//! back to `old` (`gix_query::run_body_as_of`) -- not the whole EDB, and not
21+
//! every registered effect, and -- since D9 forbids reimplementing ref
22+
//! mechanics here -- not by writing to the repository to find out either
23+
//! (see [`TriggerDetector::fire`]'s own doc for why an earlier version of
24+
//! this module did, and why that was wrong).
2025
//!
2126
//! This is explicitly a performance ceiling, not a semantic one
2227
//! (`DEVPLAN-effect.md` D3): the observable contract is "fire iff `when`
@@ -34,9 +39,7 @@
3439
3540
use std::collections::BTreeMap;
3641

37-
use gix::refs::Target as RefTarget;
38-
use gix::refs::transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog};
39-
use gix_query::{Namespace, Value};
42+
use gix_query::{Namespace, RefName, Value};
4043
use gix_store::{
4144
GixRefStore, InvalidRefName, Kind, Layout, RefPath, RefPrefix, RefSegment, RepoStore, Typed,
4245
};
@@ -72,13 +75,9 @@ pub enum TriggerError {
7275
/// The effect store itself failed to open, list, or read a doc.
7376
#[error(transparent)]
7477
Doc(#[from] crate::doc::Error),
75-
/// Rewinding or restoring the transitioning ref during before/after
76-
/// comparison failed.
77-
#[error(transparent)]
78-
RefEdit(#[from] gix::reference::edit::Error),
7978
/// `transition.ref_` is not a legal Git ref name.
8079
#[error(transparent)]
81-
InvalidRefName(#[from] gix::validate::reference::name::Error),
80+
InvalidRefName(#[from] InvalidRefName),
8281
}
8382

8483
/// A candidate effect that fired: its doc, and the rows its `when` derived at
@@ -135,15 +134,21 @@ impl<'r> TriggerDetector<'r> {
135134
/// the "before" snapshot, every other ref stays exactly as it is in
136135
/// `self.repo` right now.
137136
///
138-
/// This temporarily rewinds `transition.ref_` to `old` (or deletes it, if
139-
/// `old` is `None`) to take the "before" snapshot, then restores it to
140-
/// `new` -- best-effort, even on error, since leaving the repository with
141-
/// a ref pointed at a stale value would be a far worse failure mode than
142-
/// a propagated error. A caller running this against a live repository
143-
/// (as opposed to a fixture under test) should serialize calls so no
144-
/// other reader observes the rewound state; there is no lock here for the
145-
/// same reason `gix-query`'s facade has none -- that belongs to whatever
146-
/// hooks this into the server's ref transaction.
137+
/// "At old" is answered through [`gix_query::run_body_as_of`], an
138+
/// as-of view over `transition.ref_` alone (`gix_refstore::AsOfRefStore`,
139+
/// via `git-store`) -- **not** by rewinding the real ref and restoring it
140+
/// after, which this used to do. That mutated the repository to answer a
141+
/// read: it wrote synthetic reflog entries, and an `old = None` (ref
142+
/// creation) transition rewound by *deleting* the ref outright, briefly
143+
/// making it absent to any concurrent reader -- unacceptable on the
144+
/// server, where a [`gix_refstore::RefStore`] is D4's ref transaction
145+
/// manager, not just a test fixture's convenience. A restore-on-`Drop`
146+
/// guard also does not survive a panic-with-abort, a signal, or process
147+
/// death, so a crash mid-window could leave a user's ref rewound or
148+
/// deleted for good. The as-of view has none of these failure modes: it
149+
/// issues no write at all (`fire` performs zero of them, pinned by this
150+
/// module's tests), so there is nothing to restore and nothing for a
151+
/// concurrent reader to observe.
147152
///
148153
/// # Errors
149154
/// See [`TriggerError`].
@@ -153,65 +158,22 @@ impl<'r> TriggerDetector<'r> {
153158
return Ok(Vec::new());
154159
}
155160

156-
let name: gix::refs::FullName = transition.ref_.as_str().try_into()?;
161+
let name = RefName::new(transition.ref_.as_str())?;
157162

158163
// "at new": the repository's current, already-transitioned state.
159164
let mut after = BTreeMap::new();
160165
for doc in &candidates {
161166
after.insert(doc.name.clone(), gix_query::run_body(self.repo, &doc.when)?);
162167
}
163168

164-
// "at old": rewind just this one ref, re-evaluate, then restore.
165-
let restore = RewindGuard {
166-
repo: self.repo,
167-
name: name.clone(),
168-
restore_to: transition.new,
169-
};
170-
match transition.old {
171-
Some(old) => {
172-
self.repo.edit_reference(RefEdit {
173-
change: Change::Update {
174-
log: LogChange {
175-
mode: RefLog::AndReference,
176-
force_create_reflog: false,
177-
message: "gix-effect: rewind for delta-restricted re-evaluation".into(),
178-
},
179-
expected: PreviousValue::Any,
180-
new: RefTarget::Object(old),
181-
},
182-
name: name.clone(),
183-
deref: false,
184-
})?;
185-
}
186-
None => {
187-
self.repo.edit_reference(RefEdit {
188-
change: Change::Delete {
189-
expected: PreviousValue::Any,
190-
log: RefLog::AndReference,
191-
},
192-
name: name.clone(),
193-
deref: false,
194-
})?;
195-
}
196-
}
197-
169+
// "at old": the same repository, read through a view where only
170+
// `transition.ref_` differs -- overridden to `old`, or to "did not
171+
// exist" when `old` is `None`.
198172
let mut before = BTreeMap::new();
199-
let mut run_error = None;
200173
for doc in &candidates {
201-
match gix_query::run_body(self.repo, &doc.when) {
202-
Ok(rows) => {
203-
before.insert(doc.name.clone(), rows);
204-
}
205-
Err(e) => {
206-
run_error = Some(e);
207-
break;
208-
}
209-
}
210-
}
211-
drop(restore); // restores `transition.ref_` to `new`, even on error above
212-
213-
if let Some(e) = run_error {
214-
return Err(e.into());
174+
let rows =
175+
gix_query::run_body_as_of(self.repo, &doc.when, [(name.clone(), transition.old)])?;
176+
before.insert(doc.name.clone(), rows);
215177
}
216178

217179
let mut firings = Vec::new();
@@ -233,36 +195,6 @@ impl<'r> TriggerDetector<'r> {
233195
}
234196
}
235197

236-
/// Restores a ref to `restore_to` on drop, best-effort -- the "restore" half
237-
/// of [`TriggerDetector::fire`]'s rewind, run unconditionally so a failure
238-
/// mid-re-evaluation never leaves `transition.ref_` pointed at `old`.
239-
struct RewindGuard<'r> {
240-
repo: &'r gix::Repository,
241-
name: gix::refs::FullName,
242-
restore_to: gix::ObjectId,
243-
}
244-
245-
impl Drop for RewindGuard<'_> {
246-
fn drop(&mut self) {
247-
// Best-effort: there is no error channel from `Drop`, and failing to
248-
// restore is already the worst case this guard exists to avoid, not
249-
// one it can escalate out of.
250-
let _ = self.repo.edit_reference(RefEdit {
251-
change: Change::Update {
252-
log: LogChange {
253-
mode: RefLog::AndReference,
254-
force_create_reflog: false,
255-
message: "gix-effect: restore after delta-restricted re-evaluation".into(),
256-
},
257-
expected: PreviousValue::Any,
258-
new: RefTarget::Object(self.restore_to),
259-
},
260-
name: self.name.clone(),
261-
deref: false,
262-
});
263-
}
264-
}
265-
266198
/// One requested transition, reified as a doc -- `DEVPLAN-effect.md` D4:
267199
/// "Push intents are refs; the effect system is the ref transaction manager."
268200
///
@@ -514,7 +446,9 @@ mod tests {
514446
assert_eq!(firings[0].effect.name, "watches-members");
515447
assert_eq!(firings[0].new_rows.len(), 1);
516448

517-
// The rewind/restore must leave the ref at `new`, not `old`.
449+
// `fire()` never touched the ref: it is still at `new`, where the
450+
// test itself put it. See `fire_performs_no_writes_even_for_a_ref_creation`
451+
// below for the stronger, whole-repository version of this check.
518452
let current = repo
519453
.find_reference(format!("refs/meta/members/{member_hex}").as_str())
520454
.unwrap()
@@ -523,6 +457,122 @@ mod tests {
523457
assert_eq!(current, member_oid);
524458
}
525459

460+
/// Pins the invariant this module exists to fix: **trigger detection
461+
/// performs zero writes**. `fire()` used to answer "at old" by rewinding
462+
/// the real ref and restoring it through a `Drop` guard -- and for `old =
463+
/// None` (a ref creation, this test's case), "rewind" meant *deleting*
464+
/// the ref outright for the duration of the call, briefly making it
465+
/// absent to any concurrent reader on a server where a ref store is the
466+
/// transaction manager (D4). The as-of view
467+
/// (`gix_query::run_body_as_of`, `gix_refstore::AsOfRefStore`) answers
468+
/// the same question without touching the repository at all: every ref
469+
/// and the transitioning ref's own reflog are snapshotted before
470+
/// `fire()` and asserted byte-identical after.
471+
#[test]
472+
fn fire_performs_no_writes_even_for_a_ref_creation() {
473+
let dir = tempfile::tempdir().unwrap();
474+
let fixture = Fixture::init(dir.path());
475+
let repo = fixture.open();
476+
let store = EffectStore::open(&repo).unwrap();
477+
478+
store
479+
.define(doc(
480+
"watches-members",
481+
"member(M)",
482+
"member(M)",
483+
&["refs/meta/candidates/*"],
484+
))
485+
.unwrap();
486+
487+
let member_hex = fixture.commit("member commit");
488+
let member_oid = gix::ObjectId::from_hex(member_hex.as_bytes()).unwrap();
489+
let member_ref = format!("refs/meta/members/{member_hex}");
490+
fixture.update_ref(&member_ref, &member_hex);
491+
492+
let detector = TriggerDetector::new(&repo);
493+
let transition = Transition {
494+
ref_: member_ref.clone(),
495+
old: None,
496+
new: member_oid,
497+
};
498+
499+
let refs_before = fixture.refs();
500+
let reflog_before = fixture.git(&["reflog", "show", &member_ref]);
501+
502+
let firings = detector.fire(&transition).unwrap();
503+
assert_eq!(
504+
firings.len(),
505+
1,
506+
"sanity: this is the same transition `creating_a_member_ref_fires_the_watching_effect` \
507+
fires on, so it must still fire here"
508+
);
509+
510+
assert_eq!(
511+
fixture.refs(),
512+
refs_before,
513+
"fire() must not create, move, or delete any ref, including \
514+
`transition.ref_` itself -- not even transiently"
515+
);
516+
assert_eq!(
517+
fixture.git(&["reflog", "show", &member_ref]),
518+
reflog_before,
519+
"fire() must not write a synthetic reflog entry"
520+
);
521+
}
522+
523+
/// The advance shape of the same invariant (`old = Some`, not a
524+
/// creation): rewinding a ref that already existed used to overwrite it
525+
/// with an `Update`, which is a real write with its own reflog entry
526+
/// even though the ref never goes absent the way the `None` case does.
527+
/// `fire()` must be just as write-free here.
528+
#[test]
529+
fn fire_performs_no_writes_for_a_ref_advance() {
530+
let dir = tempfile::tempdir().unwrap();
531+
let fixture = Fixture::init(dir.path());
532+
let repo = fixture.open();
533+
let store = EffectStore::open(&repo).unwrap();
534+
535+
store
536+
.define(doc(
537+
"watches-members",
538+
"member(M)",
539+
"member(M)",
540+
&["refs/meta/candidates/*"],
541+
))
542+
.unwrap();
543+
544+
let member_hex = fixture.commit("member commit");
545+
let member_oid = gix::ObjectId::from_hex(member_hex.as_bytes()).unwrap();
546+
let member_ref = format!("refs/meta/members/{member_hex}");
547+
fixture.update_ref(&member_ref, &member_hex);
548+
549+
let detector = TriggerDetector::new(&repo);
550+
// A no-op advance is enough: the invariant under test is "no writes
551+
// happen", not "the effect fires" (that is
552+
// `an_effect_does_not_fire_when_nothing_new_is_derived`, right below).
553+
let transition = Transition {
554+
ref_: member_ref.clone(),
555+
old: Some(member_oid),
556+
new: member_oid,
557+
};
558+
559+
let refs_before = fixture.refs();
560+
let reflog_before = fixture.git(&["reflog", "show", &member_ref]);
561+
562+
detector.fire(&transition).unwrap();
563+
564+
assert_eq!(
565+
fixture.refs(),
566+
refs_before,
567+
"fire() must not create, move, or delete any ref for an advance either"
568+
);
569+
assert_eq!(
570+
fixture.git(&["reflog", "show", &member_ref]),
571+
reflog_before,
572+
"fire() must not write a synthetic reflog entry for an advance either"
573+
);
574+
}
575+
526576
/// D3: an effect whose `when` derives the *same* rows at `old` and `new`
527577
/// does not fire -- a transition on its watched ref that changes nothing
528578
/// observable is not a trigger.

0 commit comments

Comments
 (0)