roots: fix a self-deadlock in the per-entity marker read path
commit
cdaa8a3roots: fix a self-deadlock in the per-entity marker read path
The previous commit’s per-entity Result reads called state.objects()
(a std::sync::Mutex lock) twice within one statement — once directly and
once inside an and_then/match-arm closure. A let’s temporaries (and a
`match scrutinee’s) live until the whole statement completes, so the
second call tried to lock the same non-reentrant Mutex the first call’s
guard still held, hanging the request forever rather than erroring. Every
test that happened to exercise these code paths used an empty ref store
(the loop/match body never ran), so this crate’s own suite stayed green;
git ents serve’s own integration test — the first to seed a real member
and hit `GET /members — hung for the full nextest timeout instead.
roots: acquire state.objects() once per row in members/effects/redactions read_all roots: acquire state.objects() once in toolchains::show instead of once per match arm roots: add a router regression test seeding real members/effects/redactions/toolchains and hitting every list/show page Assisted-by: Claude:claude-sonnet-5
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
crates/cli/ents-web/tests/router.rs
@@ -12,10 +12,10 @@
use axum::body::Body;
use axum::http::{Request, StatusCode, header};
use ents_kiln::Toolchain;
-use ents_model::{Account, MemberId};
+use ents_model::{Account, Effect, MemberId, Provenance, Redaction};
use ents_receive::{Mode, NullEventSink};
use ents_testutil::{
- CommitSpec, Keypair, MemRefStore, ObjectStore, write_commit, write_meta_entity,
+ CommitSpec, Keypair, MemRefStore, ObjectStore, enroll_member, write_commit, write_meta_entity,
};
use ents_web::identity::SigningIdentity;
use ents_web::state::AppState;
@@ -1454,3 +1454,89 @@
"the underlying facet-git-tree error renders verbatim: {body}"
);
}
+
+/// A real, readable entity (not merely an empty ref store) exercised on
+/// every list/show page pair `read_all`'s `state.objects()` double-lock
+/// regression could hit: each page must complete rather than hang forever
+/// (a non-reentrant `Mutex` self-deadlock, previously reachable whenever a
+/// row's tree actually read back cleanly -- see the fix commit's own
+/// message). `#[tokio::test]`'s single-threaded runtime means a real
+/// deadlock here hangs the whole test binary rather than merely failing
+/// it, so this is worth pinning down explicitly rather than trusting the
+/// list/show pages' other tests to happen to seed data.
+#[tokio::test]
+async fn members_effects_redactions_and_toolchains_list_and_show_a_real_entity_without_hanging() {
+ let refs = MemRefStore::default();
+ let objects = ObjectStore::default();
+ enroll_member(
+ &refs,
+ &objects,
+ "jdc",
+ &Keypair::from_seed(1),
+ Provenance::AdminRegistered,
+ 100,
+ );
+ let effect_name: gix::refs::FullName = "refs/meta/effects/ci".try_into().expect("valid");
+ write_meta_entity(
+ &refs,
+ &objects,
+ effect_name,
+ &Effect {
+ trigger: "rev(refs/heads/main)".to_owned(),
+ toolchains: vec![],
+ run: "true".to_owned(),
+ },
+ None,
+ 100,
+ );
+ let redaction_name: gix::refs::FullName = "refs/meta/redactions/1".try_into().expect("valid");
+ write_meta_entity(
+ &refs,
+ &objects,
+ redaction_name,
+ &Redaction::new(gix_hash::ObjectId::null(gix_hash::Kind::Sha1), "leaked"),
+ None,
+ 100,
+ );
+ let toolchain_name: gix::refs::FullName =
+ "refs/meta/toolchains/good".try_into().expect("valid");
+ write_meta_entity(
+ &refs,
+ &objects,
+ toolchain_name,
+ &Toolchain {
+ name: "good".to_owned(),
+ recipe: "embedded 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n".to_owned(),
+ },
+ None,
+ 100,
+ );
+
+ let state = build_state_with(
+ FixtureIdentity {
+ name: "local-user",
+ key: Keypair::from_seed(2),
+ },
+ refs,
+ objects,
+ );
+ let router = ents_web::router(state);
+
+ for path in [
+ "/members",
+ "/members/jdc",
+ "/effects",
+ "/effects/ci",
+ "/redactions",
+ "/redactions/1",
+ "/toolchains",
+ "/toolchains/good",
+ ] {
+ let response = router
+ .clone()
+ .oneshot(Request::get(path).body(Body::empty()).expect("request"))
+ .await
+ .expect("in-process call");
+ assert_eq!(response.status(), StatusCode::OK, "GET {path}");
+ }
+}
crates/cli/ents-web/src/pages/effects.rs
@@ -95,10 +95,15 @@
let Some(id) = path.strip_prefix("refs/meta/effects/") else {
continue;
};
- let effect = super::commit_tree(&*state.objects(), tip)
+ // One `state.objects()` lock per iteration, reused for both reads
+ // -- see `crate::pages::members::read_all`'s identical comment for
+ // why a second `state.objects()` within the same statement would
+ // self-deadlock on this non-reentrant `Mutex`.
+ let objects = state.objects();
+ let effect = super::commit_tree(&*objects, tip)
.map_err(|error| error.to_string())
.and_then(|tree| {
- facet_git_tree::deserialize::<Effect>(&tree, &*state.objects())
+ facet_git_tree::deserialize::<Effect>(&tree, &*objects)
.map_err(|error| error.to_string())
});
out.push((id.to_owned(), effect));
crates/cli/ents-web/src/pages/members.rs
@@ -83,10 +83,16 @@
let Some(username) = path.strip_prefix("refs/meta/member/") else {
continue;
};
- let member = super::commit_tree(&*state.objects(), tip)
+ // One `state.objects()` lock per iteration, reused for both reads:
+ // `state.objects()` a second time *within the same statement*
+ // would try to lock this non-reentrant `Mutex` while the first
+ // guard is still alive (a `let`'s temporaries live to its own
+ // `;`), self-deadlocking forever rather than erroring.
+ let objects = state.objects();
+ let member = super::commit_tree(&*objects, tip)
.map_err(|error| error.to_string())
.and_then(|tree| {
- facet_git_tree::deserialize::<Member>(&tree, &*state.objects())
+ facet_git_tree::deserialize::<Member>(&tree, &*objects)
.map_err(|error| error.to_string())
});
out.push((username.to_owned(), member));
crates/cli/ents-web/src/pages/redactions.rs
@@ -80,10 +80,15 @@
let Some(id) = path.strip_prefix("refs/meta/redactions/") else {
continue;
};
- let redaction = super::commit_tree(&*state.objects(), tip)
+ // One `state.objects()` lock per iteration, reused for both reads
+ // -- see `crate::pages::members::read_all`'s identical comment for
+ // why a second `state.objects()` within the same statement would
+ // self-deadlock on this non-reentrant `Mutex`.
+ let objects = state.objects();
+ let redaction = super::commit_tree(&*objects, tip)
.map_err(|error| error.to_string())
.and_then(|tree| {
- facet_git_tree::deserialize::<Redaction>(&tree, &*state.objects())
+ facet_git_tree::deserialize::<Redaction>(&tree, &*objects)
.map_err(|error| error.to_string())
});
out.push((id.to_owned(), redaction));
crates/cli/ents-web/src/pages/toolchains.rs
@@ -90,10 +90,17 @@
where
O: Find + Write + Send + 'static,
{
- let body = match toolchain::view(state.refs.as_ref(), &*state.objects(), &name) {
+ // One `state.objects()` lock, reused for both `view` and `log`: a
+ // `match` scrutinee's own temporaries live for the whole match (arms
+ // included), so a second `state.objects()` inside the `Ok` arm below
+ // would try to lock this non-reentrant `Mutex` while the scrutinee's
+ // own guard is still held, self-deadlocking forever rather than
+ // erroring (see `crate::pages::members::read_all`'s identical
+ // rationale).
+ let objects = state.objects();
+ let body = match toolchain::view(state.refs.as_ref(), &*objects, &name) {
Ok((toolchain, recipe)) => {
- let log =
- toolchain::log(state.refs.as_ref(), &*state.objects(), &name).unwrap_or_default();
+ let log = toolchain::log(state.refs.as_ref(), &*objects, &name).unwrap_or_default();
html! {
dl {
dt { "name" } dd { (toolchain.name) }