jerry/buzz
1
0
Fork 0
mirror of https://github.com/block/buzz.git synced 2026-10-29 05:21:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(personas): serialize HOME-mutating tests to eliminate race condition

Browse Source 
Three async tests in concurrent_edit_tests.rs used std::env::set_var("HOME")
for store isolation but held lock_path_mutex() only in a setup block, releasing
it before the actual I/O. When the tokio test runner executed them concurrently,
tests raced on the process-global HOME env var and read from each other's temp
directories.

Fix: convert all three from #[tokio::test] async fn to #[test] fn with an
explicit single-threaded tokio runtime (block_on). lock_path_mutex() is now
held at fn scope for the full test duration — no MutexGuard across await, no
clippy flag, and the mutex serializes the three tests so HOME cannot be
clobbered mid-run.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
This commit is contained in:
Duncan
2026-09-01 11:43:38 -04:00
parent 5a3e9d3eee
commit b946f65d38
1 changed files with 260 additions and 239 deletions
Download Patch File Download Diff File Expand all files Collapse all files

499
desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs
View File

@@ -180,8 +180,8 @@ fn update_request(id: &str, display_name: &str, expected: Option<&str>) -> Updat
/// Moving the lock acquisition to after the comparison (the TOCTOU shape)
/// means `try_lock()` succeeds and the assertion panics — turning this test
/// **RED**.
#[tokio::test]
async fn command_path_and_lock_scope_regressions() {
#[test]
fn command_path_and_lock_scope_regressions() {
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::Arc;
@@ -191,104 +191,112 @@ async fn command_path_and_lock_scope_regressions() {
let old_home = std::env::var_os("HOME");
let old_xdg = std::env::var_os("XDG_DATA_HOME");
{
// Hold the path mutex only during the synchronous setup phase so that
// clippy does not flag a MutexGuard held across an await point.
let _path_guard = crate::managed_agents::lock_path_mutex();
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
}
// Hold the path mutex for the entire test so concurrent tests that also
// mutate HOME cannot race against our store reads/writes.
let _path_guard = crate::managed_agents::lock_path_mutex();
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
let app = mock_app();
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.expect("build test runtime");
// ── Layer 3: lock-scope assertion ─────────────────────────────────────
// Install the observer BEFORE seeding so it fires on the first real write.
let observer_fired = Arc::new(AtomicBool::new(false));
let observer_fired_clone = observer_fired.clone();
{
let mut slot = PRE_GUARD_OBSERVER.lock().expect("observer slot");
*slot = Some(Box::new(move |state: &crate::app_state::AppState| {
assert!(
state.managed_agents_store_lock.try_lock().is_err(),
"managed_agents_store_lock must already be held when find_persona_for_update \
rt.block_on(async {
let app = mock_app();
// ── Layer 3: lock-scope assertion ─────────────────────────────────────
// Install the observer BEFORE seeding so it fires on the first real write.
let observer_fired = Arc::new(AtomicBool::new(false));
let observer_fired_clone = observer_fired.clone();
{
let mut slot = PRE_GUARD_OBSERVER.lock().expect("observer slot");
*slot = Some(Box::new(move |state: &crate::app_state::AppState| {
assert!(
state.managed_agents_store_lock.try_lock().is_err(),
"managed_agents_store_lock must already be held when find_persona_for_update \
runs — a successful try_lock means the comparison happens outside the lock, \
which recreates the TOCTOU race"
);
observer_fired_clone.store(true, Ordering::SeqCst);
}));
}
);
observer_fired_clone.store(true, Ordering::SeqCst);
}));
}
// Seed: one persona at R1 in the persisted store.
save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed write must succeed");
// Seed: one persona at R1 in the persisted store.
save_personas(app.handle(), &[persona("p1", "Alice", R1)])
.expect("seed write must succeed");
// Layer 3 probe: a successful write fires the observer while the lock is held.
let probe_result = update_persona_with(
update_request("p1", "Alice probe", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| Ok(()),
)
.await;
// Layer 3 probe: a successful write fires the observer while the lock is held.
let probe_result = update_persona_with(
update_request("p1", "Alice probe", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| Ok(()),
)
.await;
// Clear the observer immediately — must happen before any assertion that
// could panic, so it is never left installed for subsequent invocations.
{
let mut slot = PRE_GUARD_OBSERVER.lock().expect("observer slot");
*slot = None;
}
// Clear the observer immediately — must happen before any assertion that
// could panic, so it is never left installed for subsequent invocations.
{
let mut slot = PRE_GUARD_OBSERVER.lock().expect("observer slot");
*slot = None;
}
probe_result.expect("probe write must succeed — revision matches the seed");
assert!(
observer_fired.load(Ordering::SeqCst),
"the pre-guard observer must have fired — if it did not, the hook is not wired"
);
probe_result.expect("probe write must succeed — revision matches the seed");
assert!(
observer_fired.load(Ordering::SeqCst),
"the pre-guard observer must have fired — if it did not, the hook is not wired"
);
// ── Layer 2: command-path wiring ──────────────────────────────────────
// Writer A: now at the probe's updated_at (R2). Capture it so B can use
// the original R1 as a stale seed.
//
// Re-seed at R1 so the two-writer scenario starts from a known revision.
save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("re-seed write must succeed");
// ── Layer 2: command-path wiring ──────────────────────────────────────
// Writer A: now at the probe's updated_at (R2). Capture it so B can use
// the original R1 as a stale seed.
//
// Re-seed at R1 so the two-writer scenario starts from a known revision.
save_personas(app.handle(), &[persona("p1", "Alice", R1)])
.expect("re-seed write must succeed");
let a_result = update_persona_with(
update_request("p1", "Alice A", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| Ok(()),
)
.await;
let (a_persona, ()) = a_result.expect("writer A must succeed — revision matches the seed");
let r2 = a_persona.updated_at.clone();
assert_ne!(r2, R1, "the commit must advance the revision past R1");
let a_result = update_persona_with(
update_request("p1", "Alice A", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| Ok(()),
)
.await;
let (a_persona, ()) = a_result.expect("writer A must succeed — revision matches the seed");
let r2 = a_persona.updated_at.clone();
assert_ne!(r2, R1, "the commit must advance the revision past R1");
// Writer B: seeded at R1, submits after A has committed R2.
let b_result = update_persona_with(
update_request("p1", "Alice B (must not land)", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| Ok(()),
)
.await;
let b_err = b_result.expect_err("writer B must be rejected — its seed revision R1 is stale");
// Writer B: seeded at R1, submits after A has committed R2.
let b_result = update_persona_with(
update_request("p1", "Alice B (must not land)", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| Ok(()),
)
.await;
let b_err =
b_result.expect_err("writer B must be rejected — its seed revision R1 is stale");
assert!(
b_err.starts_with(PERSONA_REVISION_CONFLICT),
"rejection must carry the conflict marker; got: {b_err}"
);
assert!(
b_err.starts_with(PERSONA_REVISION_CONFLICT),
"rejection must carry the conflict marker; got: {b_err}"
);
// Reload from the persisted store and confirm A's R2 survived B's attempt.
let persisted =
crate::managed_agents::load_personas(app.handle()).expect("reload must succeed");
let stored = persisted
.iter()
.find(|p| p.id == "p1")
.expect("persona must still exist after B's rejection");
// Reload from the persisted store and confirm A's R2 survived B's attempt.
let persisted =
crate::managed_agents::load_personas(app.handle()).expect("reload must succeed");
let stored = persisted
.iter()
.find(|p| p.id == "p1")
.expect("persona must still exist after B's rejection");
assert_eq!(
stored.updated_at, r2,
"A's committed revision must survive B's stale write attempt"
);
assert_eq!(
stored.display_name, "Alice A",
"A's committed display_name must survive B's stale write attempt"
);
assert_eq!(
stored.updated_at, r2,
"A's committed revision must survive B's stale write attempt"
);
assert_eq!(
stored.display_name, "Alice A",
"A's committed display_name must survive B's stale write attempt"
);
}); // rt.block_on
// ── Cleanup ───────────────────────────────────────────────────────────
std::env::remove_var("HOME");
@@ -313,64 +321,70 @@ async fn command_path_and_lock_scope_regressions() {
/// and refuse to close the dialog as full success. If the retain error were
/// swallowed — e.g. the callback's `?` were removed and it returned `Ok(())` —
/// this test would become GREEN on a broken code path and catch it.
#[tokio::test]
async fn retain_failure_after_persist_is_returned_not_swallowed() {
#[test]
fn retain_failure_after_persist_is_returned_not_swallowed() {
let temp = tempfile::tempdir().unwrap();
let home = temp.path().join("home");
std::fs::create_dir_all(&home).unwrap();
let old_home = std::env::var_os("HOME");
let old_xdg = std::env::var_os("XDG_DATA_HOME");
{
let _path_guard = crate::managed_agents::lock_path_mutex();
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
}
let _path_guard = crate::managed_agents::lock_path_mutex();
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
let app = mock_app();
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.expect("build test runtime");
// Seed one persona at R1 in the persisted store.
save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed write must succeed");
rt.block_on(async {
let app = mock_app();
// Submit via update_persona_with with a retain callback that always fails.
// The persona save runs first (line ~210 in update.rs: `save_personas`),
// then the retain callback is called. If the retain error propagates, we
// get Err; if it is silently discarded, we get Ok — the test catches both.
let result = update_persona_with(
update_request("p1", "Alice retain-fail", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| -> Result<(), String> {
Err("simulated retain / publish failure after persona persisted".to_string())
},
)
.await;
// Seed one persona at R1 in the persisted store.
save_personas(app.handle(), &[persona("p1", "Alice", R1)])
.expect("seed write must succeed");
assert!(
result.is_err(),
"update_persona_with must return Err when the retain callback fails — \
// Submit via update_persona_with with a retain callback that always fails.
// The persona save runs first (line ~210 in update.rs: `save_personas`),
// then the retain callback is called. If the retain error propagates, we
// get Err; if it is silently discarded, we get Ok — the test catches both.
let result = update_persona_with(
update_request("p1", "Alice retain-fail", Some(R1)),
app.handle().clone(),
|_app, _state, _persona| -> Result<(), String> {
Err("simulated retain / publish failure after persona persisted".to_string())
},
)
.await;
assert!(
result.is_err(),
"update_persona_with must return Err when the retain callback fails — \
the save coordinator must see this error, not a silent success; \
got: {:?}",
result.ok()
);
result.ok()
);
let err = result.unwrap_err();
assert!(
err.contains("simulated retain"),
"the error text must propagate from the retain callback; got: {err}"
);
let err = result.unwrap_err();
assert!(
err.contains("simulated retain"),
"the error text must propagate from the retain callback; got: {err}"
);
// Verify the persona DID persist (save_personas ran before retain) so we
// confirm the test scenario actually exercises the post-persist failure path.
let persisted =
crate::managed_agents::load_personas(app.handle()).expect("reload must succeed");
let stored = persisted
.iter()
.find(|p| p.id == "p1")
.expect("persona must exist");
assert_eq!(
stored.display_name, "Alice retain-fail",
"persona fields must have persisted before retain was called"
);
// Verify the persona DID persist (save_personas ran before retain) so we
// confirm the test scenario actually exercises the post-persist failure path.
let persisted =
crate::managed_agents::load_personas(app.handle()).expect("reload must succeed");
let stored = persisted
.iter()
.find(|p| p.id == "p1")
.expect("persona must exist");
assert_eq!(
stored.display_name, "Alice retain-fail",
"persona fields must have persisted before retain was called"
);
}); // rt.block_on
// Cleanup
std::env::remove_var("HOME");
@@ -399,133 +413,140 @@ async fn retain_failure_after_persist_is_returned_not_swallowed() {
/// Mutation acceptance: restoring `retain(&app, &state, &result)?` before
/// the avatar/name propagation block causes `save_managed_agents` to never run
/// and the assertion on the stored agent name turns RED.
#[tokio::test]
async fn linked_instance_rename_completes_before_retain_error_propagates() {
#[test]
fn linked_instance_rename_completes_before_retain_error_propagates() {
let temp = tempfile::tempdir().unwrap();
let home = temp.path().join("home2");
std::fs::create_dir_all(&home).unwrap();
let old_home = std::env::var_os("HOME");
let old_xdg = std::env::var_os("XDG_DATA_HOME");
{
let _path_guard = crate::managed_agents::lock_path_mutex();
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
}
// Hold the path mutex for the entire test so concurrent tests that also
// mutate HOME cannot race against our store reads/writes.
let _path_guard = crate::managed_agents::lock_path_mutex();
std::env::set_var("HOME", &home);
std::env::set_var("XDG_DATA_HOME", &home);
let app = mock_app();
let rt = tokio::runtime::Builder::new_current_thread()
.enable_all()
.build()
.expect("build test runtime");
// Seed persona "Alice" at R1.
save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed must succeed");
rt.block_on(async {
let app = mock_app();
// Seed a linked agent whose name matches the persona's display_name.
let agent_record = crate::managed_agents::ManagedAgentRecord {
pubkey: "pk-alice-p2".to_string(),
name: "Alice".to_string(),
persona_id: Some("p1".to_string()),
private_key_nsec: String::new(),
auth_tag: None,
relay_url: String::new(),
avatar_url: None,
acp_command: String::new(),
agent_command: String::new(),
agent_command_override: None,
agent_args: vec![],
mcp_command: String::new(),
turn_timeout_seconds: 0,
idle_timeout_seconds: None,
max_turn_duration_seconds: None,
parallelism: 1,
system_prompt: None,
model: None,
provider: None,
persona_source_version: None,
env_vars: std::collections::BTreeMap::new(),
start_on_app_launch: false,
auto_restart_on_config_change: false,
runtime_pid: None,
backend: Default::default(),
backend_agent_id: None,
provider_policy_pending: false,
provider_binary_path: None,
team_id: None,
persona_team_dir: None,
persona_name_in_team: None,
created_at: String::new(),
updated_at: String::new(),
last_started_at: None,
last_stopped_at: None,
last_exit_code: None,
last_error: None,
last_error_code: None,
respond_to: Default::default(),
respond_to_allowlist: vec![],
display_name: Some("Alice".to_string()),
description: None,
slug: None,
runtime: None,
name_pool: vec![],
is_builtin: false,
is_active: true,
shared: false,
source_team: None,
source_team_persona_slug: None,
catalog_source: None,
team_catalog_source: None,
definition_respond_to: None,
definition_respond_to_allowlist: vec![],
definition_parallelism: None,
relay_mesh: None,
effort_level: None,
};
crate::managed_agents::save_managed_agents(app.handle(), &[agent_record])
.expect("agent seed must succeed");
// Seed persona "Alice" at R1.
save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed must succeed");
// Submit a rename with a retain callback that always fails — simulates a
// strict publication/enqueue failure AFTER save_personas ran.
let result = update_persona_with(
UpdatePersonaRequest {
id: "p1".to_string(),
display_name: "Alice Renamed".to_string(),
// Seed a linked agent whose name matches the persona's display_name.
let agent_record = crate::managed_agents::ManagedAgentRecord {
pubkey: "pk-alice-p2".to_string(),
name: "Alice".to_string(),
persona_id: Some("p1".to_string()),
private_key_nsec: String::new(),
auth_tag: None,
relay_url: String::new(),
avatar_url: None,
description: None,
system_prompt: "Do the work.".to_string(),
runtime: None,
acp_command: String::new(),
agent_command: String::new(),
agent_command_override: None,
agent_args: vec![],
mcp_command: String::new(),
turn_timeout_seconds: 0,
idle_timeout_seconds: None,
max_turn_duration_seconds: None,
parallelism: 1,
system_prompt: None,
model: None,
provider: None,
name_pool: Vec::new(),
env_vars: None,
behavior: None,
expected_updated_at: Some(R1.to_string()),
},
app.handle().clone(),
|_app, _state, _persona| -> Result<(), String> {
Err("simulated publish failure after persona persisted".to_string())
},
)
.await;
persona_source_version: None,
env_vars: std::collections::BTreeMap::new(),
start_on_app_launch: false,
auto_restart_on_config_change: false,
runtime_pid: None,
backend: Default::default(),
backend_agent_id: None,
provider_policy_pending: false,
provider_binary_path: None,
team_id: None,
persona_team_dir: None,
persona_name_in_team: None,
created_at: String::new(),
updated_at: String::new(),
last_started_at: None,
last_stopped_at: None,
last_exit_code: None,
last_error: None,
last_error_code: None,
respond_to: Default::default(),
respond_to_allowlist: vec![],
display_name: Some("Alice".to_string()),
description: None,
slug: None,
runtime: None,
name_pool: vec![],
is_builtin: false,
is_active: true,
shared: false,
source_team: None,
source_team_persona_slug: None,
catalog_source: None,
team_catalog_source: None,
definition_respond_to: None,
definition_respond_to_allowlist: vec![],
definition_parallelism: None,
relay_mesh: None,
effort_level: None,
};
crate::managed_agents::save_managed_agents(app.handle(), &[agent_record])
.expect("agent seed must succeed");
// The retain error must propagate so the coordinator sees publishFailed.
assert!(
result.is_err(),
"update_persona_with must return Err when retain fails; \
// Submit a rename with a retain callback that always fails — simulates a
// strict publication/enqueue failure AFTER save_personas ran.
let result = update_persona_with(
UpdatePersonaRequest {
id: "p1".to_string(),
display_name: "Alice Renamed".to_string(),
avatar_url: None,
description: None,
system_prompt: "Do the work.".to_string(),
runtime: None,
model: None,
provider: None,
name_pool: Vec::new(),
env_vars: None,
behavior: None,
expected_updated_at: Some(R1.to_string()),
},
app.handle().clone(),
|_app, _state, _persona| -> Result<(), String> {
Err("simulated publish failure after persona persisted".to_string())
},
)
.await;
// The retain error must propagate so the coordinator sees publishFailed.
assert!(
result.is_err(),
"update_persona_with must return Err when retain fails; \
if this is Ok the retain error was swallowed"
);
);
// The linked agent must have been renamed BEFORE the error propagated.
// If save_managed_agents was skipped (the pre-fix path), the name is still "Alice".
let agents = crate::managed_agents::load_managed_agents(app.handle())
.expect("agent reload must succeed");
let stored_agent = agents
.iter()
.find(|a| a.pubkey == "pk-alice-p2")
.expect("linked agent must still exist");
// The linked agent must have been renamed BEFORE the error propagated.
// If save_managed_agents was skipped (the pre-fix path), the name is still "Alice".
let agents = crate::managed_agents::load_managed_agents(app.handle())
.expect("agent reload must succeed");
let stored_agent = agents
.iter()
.find(|a| a.pubkey == "pk-alice-p2")
.expect("linked agent must still exist");
assert_eq!(
stored_agent.name, "Alice Renamed",
"linked instance name must be propagated before the retain error is returned; \
assert_eq!(
stored_agent.name, "Alice Renamed",
"linked instance name must be propagated before the retain error is returned; \
restoring `retain()?` before the propagation block turns this RED"
);
);
}); // rt.block_on
// Cleanup
std::env::remove_var("HOME");