From d71cd62c7f32c7477fbc43e7171fac56c5d1b2c4 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 5 Aug 2026 10:47:40 -0400 Subject: [PATCH] fix(desktop): resolve authorized-pass round-4 test-vs-claim residuals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Item 1 — contender test (production-called hook): Move start_pair_for_with_hook and start_pair_lazy_for_with_hook to runtime_commands_seams.rs (extracted to stay under the 1000-line gate; included via #[path]). Production start_pair_for delegates to start_pair_for_with_hook with no-op hooks; start_pair_lazy_for delegates through start_pair_lazy_for_with_hook. The test-file mirror start_pair_for_with_hook is deleted from runtime_commands_tests.rs. The contender test calls the production-callable seam: removing managed_agent_runtime_transition from start_pair_for_with_hook would also remove it from the production delegation chain, making the test a faithful proxy. Item 2 — writer test (on_after_restore callback): Remove the unconditional save_managed_agents_at from the production compensate_drain_with_hook body (step-7 was a no-op duplicate that also masked the real compensation result on error). Add on_after_restore hook invoked after compensate_drain_for returns, still under both guards. Production compensate_drain passes a no-op; test injects a mutation + save. The writer test: on_records_loaded signals writer and waits for writer_at_store_lock_rx before returning; on_after_restore mutates COMP_SENTINEL in-memory and saves (assert/unwrap). Both COMP_SENTINEL and WRITER_EDIT must appear on disk. If the store guard is dropped before on_after_restore, the writer interleaves, loads without COMP_SENTINEL, and the first assertion fails deterministically. Item 3 — E2E classification: thread-focus-mode.spec.ts:139 failed in run 30987602439 (branch head 6739f157e). The prior branch commit 27ea60724 (same TypeScript changes, different Rust) had green CI including all E2E shards (run 30981177509). The delta between 27ea60724 and 6739f157e is Rust-only; the spec tests viewport scroll preservation in focus/split mode — no Rust path. The failure is a CI flake, not a branch regression. Item 4 — doc comment alignment: Updated all compensation and contender test comments to describe only what the execution establishes. Removed references to step-7, stale failure-mode descriptions, and start_pair_for in favour of start_pair_for_with_hook. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/managed_agents/runtime_commands.rs | 73 ++++---- .../runtime_commands_concurrency_tests.rs | 157 ++++++++++-------- .../managed_agents/runtime_commands_seams.rs | 80 +++++++++ .../managed_agents/runtime_commands_tests.rs | 77 --------- 4 files changed, 207 insertions(+), 180 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/runtime_commands_seams.rs diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands.rs b/desktop/src-tauri/src/managed_agents/runtime_commands.rs index e7b076914..428fb36d3 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands.rs @@ -250,9 +250,14 @@ pub(crate) fn start_pair_lazy_for( relay_url: String, app: tauri::AppHandle, ) -> Result { - start_pair_for(pubkey, relay_url, true, None, app) + start_pair_lazy_for_with_hook(pubkey, relay_url, app, || {}, || {}) } +// Start-pair hook seams: extracted to stay under the file-size ratchet. +#[path = "runtime_commands_seams.rs"] +mod seams; +pub(crate) use seams::{start_pair_for_with_hook, start_pair_lazy_for_with_hook}; + #[tauri::command] pub fn start_managed_agent_runtime( pubkey: String, @@ -279,30 +284,14 @@ fn start_pair_for( expected_updated_at: Option<&str>, app: tauri::AppHandle, ) -> Result { - let state = app.state::(); - let _transition = state - .managed_agent_runtime_transition - .lock() - .map_err(|e| e.to_string())?; - if state - .shutdown_started - .load(std::sync::atomic::Ordering::Acquire) - { - return Err("desktop shutdown has started".into()); - } - let _store = state - .managed_agents_store_lock - .lock() - .map_err(|e| e.to_string())?; - let mut records = load_managed_agents(&app)?; - start_pair_under_held_locks( - &app, - &state, + start_pair_for_with_hook( pubkey, relay_url, lazy, expected_updated_at, - &mut records, + app, + || {}, + || {}, ) } @@ -907,28 +896,43 @@ where /// Production adapter for [`compensate_drain_for`]. /// -/// Delegates to [`compensate_drain_with_hook`] with a no-op hook. +/// Delegates to [`compensate_drain_with_hook`] with no-op hooks. pub(crate) fn compensate_drain( app: &tauri::AppHandle, stopped: &[DrainJournalEntry], captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, _rt_transition_held: std::sync::MutexGuard<'_, ()>, ) -> Option { - compensate_drain_with_hook(app, stopped, captured_scope, _rt_transition_held, |_| {}) + compensate_drain_with_hook( + app, + stopped, + captured_scope, + _rt_transition_held, + |_| {}, + |_| {}, + ) } -/// Inner implementation of [`compensate_drain`] with an injectable `on_records_loaded` hook. +/// Inner implementation of [`compensate_drain`] with injectable hooks. /// /// Lock order: transition guard held by caller → acquire store lock → validate generation /// → load records → `on_records_loaded(&mut records)` (no-op in production; tests inject -/// a sentinel mutation) → delegate to `compensate_drain_for` → save records (step 7, still -/// under the store lock so the writer cannot interleave before the mutation hits disk). +/// a sentinel mutation) → delegate to `compensate_drain_for` → `on_after_restore(&mut records)` +/// (no-op in production; tests assert/save the injected mutation to prove the store lock is +/// held continuously through restore and the post-restore callback). +/// +/// The store lock is held across the entire sequence: validate → load → pre-restore hook +/// → every restore → post-restore callback. The caller-owned transition guard is retained +/// across the whole interval. With no-op callbacks, production behavior is byte-for-byte +/// equivalent to the pre-hook path on all paths (empty, stale-generation, load-failure, +/// partial-restore, full-success). pub(crate) fn compensate_drain_with_hook( app: &tauri::AppHandle, stopped: &[DrainJournalEntry], captured_scope: &crate::managed_agents::scope::WorkspaceAgentScope, _rt_transition_held: std::sync::MutexGuard<'_, ()>, on_records_loaded: impl FnOnce(&mut Vec), + on_after_restore: impl FnOnce(&mut Vec), ) -> Option { if stopped.is_empty() { drop(_rt_transition_held); @@ -964,12 +968,11 @@ pub(crate) fn compensate_drain_with_hook( } }; - // 5. Fire the hook with the loaded records. No-op in production. - // Tests mutate a sentinel field here and synchronise with a writer - // thread; the save at step 7 then makes that mutation load-bearing. + // 5. Pre-restore hook — no-op in production. + // Tests inject a sentinel mutation here and synchronise with a writer thread. on_records_loaded(&mut records); - // 6. Delegate — store guard held through every save by start_pair_under_held_locks. + // 6. Delegate — store guard held through every restore by start_pair_under_held_locks. let result = compensate_drain_for(stopped, &mut records, |entry, recs| { start_pair_under_held_locks( app, @@ -983,13 +986,9 @@ pub(crate) fn compensate_drain_with_hook( .map(|_| ()) }); - // 7. Save the (hook-mutated) records while the store guard is still held. - // In production this is a no-op duplicate of the save inside - // start_pair_under_held_locks; in tests it persists the sentinel - // mutation so the writer cannot interleave before it hits disk. - if let Err(e) = save_managed_agents_at(&captured_scope.definitions_dir, &records) { - return Some(format!("compensation failed: could not save records: {e}")); - } + // 7. Post-restore hook — no-op in production. Tests save the injected mutation + // here to prove the store lock is held through restore and this callback. + on_after_restore(&mut records); result } diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands_concurrency_tests.rs b/desktop/src-tauri/src/managed_agents/runtime_commands_concurrency_tests.rs index 4e0379680..a9ac70e80 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands_concurrency_tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands_concurrency_tests.rs @@ -9,9 +9,9 @@ use super::*; /// Writer-vs-compensation store-lock contention via the `compensate_drain_with_hook` seam. /// /// Invariant: if `managed_agents_store_lock` is dropped before the adapter's -/// step-7 save in `compensate_drain_with_hook`, the writer acquires the lock -/// before `COMP_SENTINEL` reaches disk, loads records without it, and saves -/// only `WRITER_EDIT` — making the `COMP_SENTINEL` assertion fail deterministically. +/// `on_after_restore` callback, the writer acquires the lock before `COMP_SENTINEL` +/// reaches disk, loads records without it, and saves only `WRITER_EDIT` — +/// making the `COMP_SENTINEL` assertion fail deterministically. /// /// Flow: /// 1. Seed the store with one agent record; seed a live runtime so that @@ -22,21 +22,20 @@ use super::*; /// a. signals the writer thread (`records_loaded_tx`); /// b. waits for writer's pre-lock signal (`writer_at_store_lock_rx`) — sent /// immediately before `managed_agents_store_lock.lock()`, so when received, -/// the writer's next instruction is that lock call (which blocks); -/// c. mutates `COMP_SENTINEL` on the in-memory adapter-loaded records. -/// 4. `compensate_drain_with_hook` continues: delegate to `compensate_drain_for` -/// (succeeds — AlreadyRunning), then saves the hook-mutated records at step 7. -/// Store lock held throughout; writer remains blocked on the store lock. -/// 5. Adapter releases the store lock. Writer acquires it, loads records -/// (which now include `COMP_SENTINEL` from step 3c's in-memory mutation + step 4's -/// save), writes `WRITER_EDIT`, saves. -/// 6. Final disk record must contain BOTH `COMP_SENTINEL` AND `WRITER_EDIT`. +/// the writer's next instruction is that lock call (which blocks). +/// 4. `compensate_drain_for` runs under the held store lock (succeeds — AlreadyRunning). +/// 5. `on_after_restore` hook fires while the store lock is STILL held: +/// mutates `COMP_SENTINEL` on the adapter-loaded records and saves to disk +/// (assert/unwrap). Writer remains blocked. +/// 6. Adapter releases the store lock. Writer acquires it, loads records +/// (which now include `COMP_SENTINEL`), writes `WRITER_EDIT`, saves. +/// 7. Final disk record must contain BOTH `COMP_SENTINEL` AND `WRITER_EDIT`. /// `comp_result` must be `None` (compensation succeeded). /// -/// What breaks it: if the store guard is dropped before the adapter's step-7 save, -/// the writer acquires `managed_agents_store_lock`, loads records BEFORE `COMP_SENTINEL` -/// reaches disk (the in-memory mutation has not been saved yet), and saves without -/// `COMP_SENTINEL` — the final assertion fails deterministically. +/// What breaks it: if the store guard is dropped before `on_after_restore`, +/// the writer acquires `managed_agents_store_lock` before `COMP_SENTINEL` +/// reaches disk, loads records without it, and saves only `WRITER_EDIT` — +/// the first assertion fails deterministically. #[test] fn test_compensate_drain_writer_vs_compensation_deterministic() { use crate::managed_agents::scope::{ @@ -169,8 +168,8 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { let entry1 = make_drain_entry(&pubkey1, "wss://relay.example", true); let stopped = vec![entry1]; - // records_loaded: hook → writer (compensation holds store lock; writer may proceed) - // writer_at_store_lock: writer → hook (writer is about to call managed_agents_store_lock.lock()) + // records_loaded: hook → writer (compensation holds store lock; writer may proceed) + // writer_at_store_lock: writer → hook (writer is about to call managed_agents_store_lock.lock()) // // The writer sends writer_at_store_lock immediately BEFORE the lock call, so the // hook's recv() completing is a happens-before guarantee that the writer's next @@ -182,8 +181,8 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { // Spawn the writer thread BEFORE acquiring the transition guard. // The writer directly acquires managed_agents_store_lock after signalling — // no intermediate lock — so its blocking is specifically on the store lock - // that compensate_drain_with_hook holds. This makes the test fail if the - // adapter drops the store guard before the step-7 save. + // that compensate_drain_with_hook holds. This makes the test fail if the + // adapter drops the store guard before on_after_restore saves. let tmp_wr = tmp_path.clone(); let app_handle_wr = app_handle.clone(); let wr_thread = thread::spawn(move || { @@ -191,19 +190,19 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { records_loaded_rx.recv().unwrap(); // Signal the hook that we are about to call managed_agents_store_lock.lock(). - // The hook's recv() will happen-before our lock call, so when on_records_loaded - // mutates COMP_SENTINEL and returns, we are guaranteed to be BLOCKED on the + // The hook's recv() will happen-before our lock call, so when on_after_restore + // mutates COMP_SENTINEL and saves, we are guaranteed to be BLOCKED on the // store lock — not merely about to call it. writer_at_store_lock_tx.send(()).unwrap(); - // Block on the store lock. Compensation still holds it; we wait here until - // the adapter's step-7 save completes and the lock is released. + // Block on the store lock. Compensation still holds it; we wait here until + // on_after_restore saves COMP_SENTINEL and the lock is released. let writer_state = app_handle_wr.state::(); let _store = writer_state.managed_agents_store_lock.lock().unwrap(); - // Store lock acquired: compensation has finished and released the lock. - // Load records — COMP_SENTINEL must be present because the adapter's - // step-7 save wrote it before releasing the lock. + // Store lock acquired: on_after_restore has finished and released the lock. + // Load records — COMP_SENTINEL must be present because on_after_restore + // saved it before releasing the lock. let mut records = crate::managed_agents::storage::load_managed_agents_at(&tmp_wr).unwrap_or_default(); for r in &mut records { @@ -215,38 +214,49 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { let rt_guard = state.managed_agent_runtime_transition.lock().unwrap(); - // on_records_loaded hook fires after load, while BOTH locks are held: + // on_records_loaded fires after load, while BOTH locks are held: // (a) signal the writer thread that compensation holds the store lock; // (b) wait for the writer to confirm it is at the store-lock boundary — - // the writer sends this signal immediately BEFORE calling - // managed_agents_store_lock.lock(), so by the time recv() returns - // here, the writer is queued (blocked) on the store lock; - // (c) mutate COMP_SENTINEL in-memory on the adapter-loaded records. - // The adapter then saves these hook-mutated records (step 7) before releasing - // the store lock. Only then does the writer acquire the lock and read records. + // the writer sends this immediately BEFORE managed_agents_store_lock.lock(), + // so by the time recv() returns, the writer's next instruction is that lock + // call (which blocks because compensation holds it). // - // What breaks it: if the store guard is dropped before the adapter's step-7 - // save, the writer acquires managed_agents_store_lock before COMP_SENTINEL - // reaches disk — the writer's final save omits COMP_SENTINEL, failing the - // assertion below. - let comp_result = - compensate_drain_with_hook(&app_handle, &stopped, &scope, rt_guard, |records| { + // on_after_restore fires AFTER compensate_drain_for returns, still under BOTH locks: + // mutates COMP_SENTINEL in-memory and saves to disk (assert/unwrap). + // The store lock is still held — the writer remains blocked until this save returns + // and the lock is released. + // + // What breaks it: if the store guard is dropped before on_after_restore, the writer + // acquires managed_agents_store_lock before COMP_SENTINEL reaches disk — the writer's + // final save omits COMP_SENTINEL, and the first assertion below fails. + let comp_result = compensate_drain_with_hook( + &app_handle, + &stopped, + &scope, + rt_guard, + // on_records_loaded: synchronise with the writer — both locks are held here. + |_records| { // (a) Tell the writer that records are loaded; compensation holds both locks. records_loaded_tx.send(()).unwrap(); // (b) Wait for the writer to reach the store-lock boundary. - // After this recv() completes, the writer has sent its signal and - // its very next instruction is managed_agents_store_lock.lock(). - // The store lock is held by compensation, so the writer will block. + // After recv() completes, the writer's next instruction is + // managed_agents_store_lock.lock() — which blocks because we hold it. writer_at_store_lock_rx.recv().unwrap(); - - // (c) Mutate COMP_SENTINEL in-memory. The adapter's step-7 save writes this - // to disk before releasing the store lock — making the mutation load-bearing. + }, + // on_after_restore: save the COMP_SENTINEL mutation while BOTH locks are still held. + // The writer is blocked on managed_agents_store_lock. This save reaching disk before + // the lock is released is the load-bearing property: shortening the guard before this + // callback lets the writer interleave and load records without COMP_SENTINEL. + |records| { for r in records.iter_mut() { r.env_vars .insert("COMP_SENTINEL".to_string(), "yes".to_string()); } - }); + crate::managed_agents::storage::save_managed_agents_at(&tmp_path, records) + .expect("on_after_restore: save must succeed — store lock held"); + }, + ); wr_thread.join().expect("writer thread panicked"); @@ -260,21 +270,19 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { } // Compensation must have succeeded: agent was AlreadyRunning, no errors. - // If the record had an invalid nsec or the seam failed, comp_result would be Some. assert!( comp_result.is_none(), "compensation must succeed (AlreadyRunning path): {comp_result:?}" ); // ── Final disk state: BOTH effects must be present ─────────────────────── - // COMP_SENTINEL: set in-memory by the hook, saved by the adapter's step-7 save - // while the store lock was still held — before the writer could load. - // WRITER_EDIT: written by the writer after the adapter released the store lock. + // COMP_SENTINEL: mutated and saved by on_after_restore while the store lock + // was still held — before the writer could acquire and load. + // WRITER_EDIT: written by the writer after the adapter released the lock. // - // If the store guard is dropped before the adapter's step-7 save, the writer - // acquires managed_agents_store_lock and loads records before COMP_SENTINEL - // reaches disk — the writer's save omits COMP_SENTINEL, and the first - // assertion below fails. + // If the store guard is dropped before on_after_restore, the writer acquires + // managed_agents_store_lock before COMP_SENTINEL reaches disk — the writer's + // save omits COMP_SENTINEL, and the first assertion below fails. let final_records = crate::managed_agents::storage::load_managed_agents_at(&tmp_path).unwrap_or_default(); let final_rec = final_records @@ -285,8 +293,8 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { assert_eq!( final_rec.env_vars.get("COMP_SENTINEL").map(String::as_str), Some("yes"), - "COMP_SENTINEL must reach disk via the adapter's step-7 save while the store \ - lock is held; fails if the store guard is dropped before that save, allowing \ + "COMP_SENTINEL must reach disk via on_after_restore while the store lock is held; \ + fails if the store guard is dropped before on_after_restore, allowing \ the writer to load and save records without COMP_SENTINEL" ); assert_eq!( @@ -300,6 +308,11 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { /// while `compensate_drain_with_hook` holds it, and the `on_transition_acquired` hook /// in `start_pair_lazy_for_with_hook` fires only AFTER the transition lock is released. /// +/// `start_pair_lazy_for_with_hook` is the production-callable seam: production +/// `start_managed_agent_runtime_pair_lazy` → `start_pair_lazy_for` → `start_pair_lazy_for_with_hook` +/// → `start_pair_for_with_hook`. Removing `managed_agent_runtime_transition` from the +/// production path also removes it from this test seam, making the test a faithful proxy. +/// /// Invariant: if `managed_agent_runtime_transition` is removed from the start seam, /// the contender fires `on_transition_acquired` immediately after `on_before_transition` /// (with zero lock contention) — before compensation has a chance to complete. The @@ -336,8 +349,8 @@ fn test_compensate_drain_writer_vs_compensation_deterministic() { /// 7. Test asserts that `contender_transition_acquired` fires within a timeout. /// /// What breaks it: if `managed_agent_runtime_transition` is removed from -/// `start_pair_for`, `on_transition_acquired` fires before step 4's assertion (because -/// the contender fires it in nanoseconds, while compensation needs milliseconds for +/// `start_pair_for_with_hook`, `on_transition_acquired` fires before step 4's assertion +/// (the contender fires it in nanoseconds; compensation needs milliseconds for /// file I/O before reaching `on_records_loaded`), making both the `try_recv()` and the /// atomic check fail. #[test] @@ -451,7 +464,7 @@ fn test_compensate_drain_concurrent_start_is_blocked() { let (contender_transition_acquired_tx, contender_transition_acquired_rx) = std::sync::mpsc::channel::<()>(); - // Shared flag: set to true when `on_transition_acquired` fires inside start_pair_for. + // Shared flag: set to true when `on_transition_acquired` fires inside start_pair_for_with_hook. // Belt-and-suspenders companion to the try_recv() check in on_records_loaded. let transition_hook_fired = Arc::new(AtomicBool::new(false)); let transition_hook_fired2 = transition_hook_fired.clone(); @@ -514,13 +527,20 @@ fn test_compensate_drain_concurrent_start_is_blocked() { // blocks at managed_agent_runtime_transition.lock(). on_transition_acquired // cannot fire until after compensation releases the guard. try_recv() correctly // returns Err(Empty). - let _comp_result = - compensate_drain_with_hook(&app_handle, &stopped, &scope, rt_guard, |_records| { + let _comp_result = compensate_drain_with_hook( + &app_handle, + &stopped, + &scope, + rt_guard, + // on_records_loaded: fires while BOTH locks are held. Checks that + // on_transition_acquired has NOT fired — the contender is still blocked + // at managed_agent_runtime_transition.lock() because we hold it. + |_records| { // Check 1: atomic bool. assert!( !transition_hook_fired.load(Ordering::SeqCst), "start seam must not acquire the transition guard while compensation holds it; \ - fails if managed_agent_runtime_transition is removed from start_pair_for" + fails if managed_agent_runtime_transition is removed from start_pair_for_with_hook" ); // Check 2: channel try_recv — deterministic proof via file-I/O time differential. // transition_hook_check_rx is a dedicated receiver that on_transition_acquired @@ -529,10 +549,15 @@ fn test_compensate_drain_concurrent_start_is_blocked() { assert!( transition_hook_check_rx.try_recv().is_err(), "on_transition_acquired must not fire while compensation holds the transition \ - guard; fails if managed_agent_runtime_transition is removed from start_pair_for \ + guard; fails if managed_agent_runtime_transition is removed from \ + start_pair_for_with_hook \ (the hook fires in nanoseconds; this check runs after ms of file I/O)" ); - }); + }, + // on_after_restore: no-op for the contender test — this test's load-bearing + // property is the transition guard, not the store-lock lifetime. + |_records| {}, + ); // After compensation returns, the contender can acquire the transition guard. // `on_transition_acquired` fires and sends the signal. @@ -541,7 +566,7 @@ fn test_compensate_drain_concurrent_start_is_blocked() { .expect( "contender's on_transition_acquired must fire after compensate_drain_with_hook \ releases the transition guard; fails if start_pair_for_with_hook does not acquire \ - managed_agent_runtime_transition before calling the hook", + managed_agent_runtime_transition before calling on_transition_acquired", ); contender.join().expect("contender thread panicked"); diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands_seams.rs b/desktop/src-tauri/src/managed_agents/runtime_commands_seams.rs new file mode 100644 index 000000000..1790a6b61 --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/runtime_commands_seams.rs @@ -0,0 +1,80 @@ +//! Injectable seams for `managed_agents/runtime_commands.rs`. +//! +//! Extracted here to keep `runtime_commands.rs` under the 1000-line size ratchet. +//! Included via `#[path]` from `runtime_commands.rs`. + +use super::*; + +/// Lazy start seam with injectable before/after-transition hooks. +/// +/// Delegates to [`start_pair_for_with_hook`] with `lazy = true`. Tests that +/// need a mock-runtime contender call this directly — it is the exact function +/// that production `start_managed_agent_runtime_pair_lazy` calls through +/// `start_pair_lazy_for`, so removing the transition guard from this path would +/// also remove it from production. +pub(crate) fn start_pair_lazy_for_with_hook( + pubkey: String, + relay_url: String, + app: tauri::AppHandle, + on_before_transition: impl FnOnce(), + on_transition_acquired: impl FnOnce(), +) -> Result { + start_pair_for_with_hook( + pubkey, + relay_url, + true, + None, + app, + on_before_transition, + on_transition_acquired, + ) +} + +/// Generic start-pair seam with injectable hooks. +/// +/// - `on_before_transition`: fires BEFORE `managed_agent_runtime_transition` is +/// locked. Tests signal "at the lock boundary" from here — the contender is +/// committed to acquiring `managed_agent_runtime_transition` immediately after. +/// +/// - `on_transition_acquired`: fires AFTER `managed_agent_runtime_transition` is +/// acquired but BEFORE `managed_agents_store_lock` is attempted. Signals that +/// start has passed the transition-lock boundary. +/// +/// Production delegates with `|| {}` for both hooks — release-invisible no-ops. +pub(crate) fn start_pair_for_with_hook( + pubkey: String, + relay_url: String, + lazy: bool, + expected_updated_at: Option<&str>, + app: tauri::AppHandle, + on_before_transition: impl FnOnce(), + on_transition_acquired: impl FnOnce(), +) -> Result { + let state = app.state::(); + on_before_transition(); + let _transition = state + .managed_agent_runtime_transition + .lock() + .map_err(|e| e.to_string())?; + if state + .shutdown_started + .load(std::sync::atomic::Ordering::Acquire) + { + return Err("desktop shutdown has started".into()); + } + on_transition_acquired(); + let _store = state + .managed_agents_store_lock + .lock() + .map_err(|e| e.to_string())?; + let mut records = load_managed_agents(&app)?; + start_pair_under_held_locks( + &app, + &state, + pubkey, + relay_url, + lazy, + expected_updated_at, + &mut records, + ) +} diff --git a/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs b/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs index 67795b28d..3dd34ff86 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_commands_tests.rs @@ -5,83 +5,6 @@ use super::*; -/// Test seam: mirrors `start_pair_for` with two injectable hooks: -/// -/// - `on_before_transition`: fires BEFORE `managed_agent_runtime_transition` is -/// locked. The contender calls this to signal "I am at the lock boundary" from -/// inside the function, giving the test deterministic evidence that the start -/// seam is actually blocked on the lock (not merely about to call the function). -/// -/// - `on_transition_acquired`: fires AFTER `managed_agent_runtime_transition` is -/// acquired but BEFORE `managed_agents_store_lock` is attempted. Signals the -/// test that start has passed the transition-lock boundary. -/// -/// Used by concurrency tests to prove that the start seam is serialised by the -/// transition guard: a contender cannot advance past `on_before_transition` while -/// compensation holds the guard, and `on_transition_acquired` fires only after the -/// guard is released. -fn start_pair_for_with_hook( - pubkey: String, - relay_url: String, - lazy: bool, - expected_updated_at: Option<&str>, - app: tauri::AppHandle, - on_before_transition: impl FnOnce(), - on_transition_acquired: impl FnOnce(), -) -> Result { - let state = app.state::(); - // Pre-acquisition hook: fires here, before the lock call. The contender uses - // this to signal "at the lock boundary" from inside the seam so the test - // knows the contender is blocked, not just about to call the function. - on_before_transition(); - let _transition = state - .managed_agent_runtime_transition - .lock() - .map_err(|e| e.to_string())?; - if state - .shutdown_started - .load(std::sync::atomic::Ordering::Acquire) - { - return Err("desktop shutdown has started".into()); - } - // Post-acquisition hook: transition guard held, store lock not yet acquired. - on_transition_acquired(); - let _store = state - .managed_agents_store_lock - .lock() - .map_err(|e| e.to_string())?; - let mut records = load_managed_agents(&app)?; - start_pair_under_held_locks( - &app, - &state, - pubkey, - relay_url, - lazy, - expected_updated_at, - &mut records, - ) -} - -/// Test seam: calls `start_pair_for_with_hook` with a lazy=true start and both -/// injectable hooks. -fn start_pair_lazy_for_with_hook( - pubkey: String, - relay_url: String, - app: tauri::AppHandle, - on_before_transition: impl FnOnce(), - on_transition_acquired: impl FnOnce(), -) -> Result { - start_pair_for_with_hook( - pubkey, - relay_url, - true, - None, - app, - on_before_transition, - on_transition_acquired, - ) -} - /// Spawn a long-lived child process that stays running long enough for tests. /// /// Cross-platform replacement for `sleep 10000` — seeds the in-memory runtimes