fix(settings): bind engine setting writes to their profile and serialize the toggle hook

docs/architecture/invariants.md

@@ -47,10 +47,14 @@ The hand-rolled copies had drifted apart, each carrying a different subset of th
 
 Converted so far — a sample, not a roster, and deliberately not kept exhaustive: `useVisualizerStyle`, `useEstimatedKaraoke`, `useLyricsLookupSettings`, `useDesktopLyricsStyle`, `useContrastMode`, `useTrackColumns`, `useScrollLongTitles`, `useArtistBioCollapsed`, `useHiddenKpis`, `useCoverSlideshow`, `useVisualizerColor`, `useArtistHero`, `useWrappedBannerVisibility` (two keys ⇒ two instances, each on its **own** broadcast channel — sharing one would make a write to either key re-read the other and clobber its in-flight optimistic value).
 
-Still standalone, by shape rather than by oversight: `useWebRadioFavorites` (persists through the plugin-favorites commands, not `profile_setting`), `useHiResBadgeVisibility` (module-level store + subscription so a non-React consumer can read it), `useSortMemory` (many dynamic keys), [`useProfileScopedToggle`](../../src/hooks/useProfileScopedToggle.ts) (the engine toggles push their value to the audio threads on the way in, so they own their own commands rather than going through `profile_setting`), and the direct `setProfileSetting` calls in `ThemeContext` / `SkinContext` / `shortcuts.ts` / a few Settings cards.
+Still standalone, by shape rather than by oversight: `useWebRadioFavorites` (persists through the plugin-favorites commands, not `profile_setting`), `useHiResBadgeVisibility` (module-level store + subscription so a non-React consumer can read it), `useSortMemory` (many dynamic keys), [`useProfileScopedToggle`](../../src/hooks/useProfileScopedToggle.ts) (the engine toggles push their value to the audio threads on the way in, so they own their own commands rather than going through `profile_setting`; it carries its own serialized write chain, below), and the direct `setProfileSetting` calls in `ThemeContext` / `SkinContext` / `shortcuts.ts` / a few Settings cards.
 
 The hook also passes the captured profile id to [`set_profile_setting` / `get_profile_setting`](../../src-tauri/crates/app/src/commands/profile.rs), which validate it through [`AppState::require_profile_pool_for`](../../src-tauri/crates/app/src/state.rs) **under the same lock `switch_profile` takes**. A JS-side "is this still my profile?" check runs before the IPC hop, so only the backend can stop a queued write from landing in the profile the user just switched to. Passing `None` opts out and targets whatever is active — correct for a fresh user action, wrong for anything that awaited first.
 
+**The engine setting commands take the same id** (issue #996). Each `player_set_*` that saves a `profile_setting` row and then hands the value to the engine — every setter in [`commands/player/sound.rs`](../../src-tauri/crates/app/src/commands/player/sound.rs) but the volume, and the match-source-rate, pause-on-device-loss and keep-awake ones in `output.rs` — takes an optional `expected_profile_id`. The `sound.rs` ones go through [`save_then_apply`](../../src-tauri/crates/app/src/commands/player/settings.rs): the pool is leased through the same check (`state::lease_profile`), a profile that changed fails the command with `ProfileChanged` before anything is saved or applied, the rows are saved, and the engine is touched only while the profile they went into is still the active one, under the profile lock a switch takes for writing (`state::while_profile_is`). A switch that lands after the lease leaves the rows in the profile they were made for and skips the engine step, and the command still succeeds: the setting was saved where it belongs. A command that lost the race to a switch therefore never overwrites the values `switch_profile` restored for the next profile. `None` keeps the contract these commands always had: whatever profile is active, and with none at all the value is applied without being saved. The three `output.rs` ones keep requiring a profile instead: with none active they fail before anything is applied, with `NoActiveProfile` under `None` and with `ProfileChanged` (`active: None`) under an id. Every frontend caller passes the id it captured — the Settings handlers, the settings reset, the stereo-image card, the speed control — and the handlers check that the profile is still theirs before rolling back a failure, since a refused write now fails where it used to land in the wrong profile. Still on whatever is active: the exclusive output and the output device (applied by a blocking device rebuild), the EQ and the volume.
+
+[`useProfileScopedToggle`](../../src/hooks/useProfileScopedToggle.ts) also chains its writes (issue #995): one at a time, a queued write that a later click superseded is skipped, a write whose profile is no longer active is not sent, and a failure that is still the last decision rolls back to the value the backend last confirmed. Saved first, then applied, as each command does it — the chain only orders the commands.
+
 ### Events
 
 The backend emits Tauri events; the frontend listens via `listen()` from `@tauri-apps/api/event`:

src-tauri/crates/app/src/commands/player/mod.rs

@@ -126,3 +126,6 @@ pub(crate) async fn fetch_replay_gain(pool: &sqlx::SqlitePool, track_id: i64) ->
 
 #[cfg(test)]
 mod album_ordering_tests;
+
+#[cfg(test)]
+mod setting_write_tests;

src-tauri/crates/app/src/commands/player/output.rs

@@ -273,14 +273,19 @@ pub async fn player_set_match_source_rate(
     state: tauri::State<'_, AppState>,
     engine: tauri::State<'_, Arc<AudioEngine>>,
     enabled: bool,
+    expected_profile_id: Option<i64>,
 ) -> AppResult<()> {
     // Written before it is applied, and the error propagates. A
     // preference the UI believes it saved, and that comes back to its
     // old value at the next launch or the next profile switch, is worse
     // than a toggle that visibly refuses — and this one takes effect on
     // the next track, so there is nothing to undo in the engine when the
-    // write fails.
-    let pool = state.require_profile_pool().await?;
+    // write fails. Into the profile the toggle was flipped in, or
+    // nowhere (#996): see `save_then_apply`, whose rule this follows
+    // with a profile required.
+    let (pool, written_profile) = state
+        .require_profile_pool_with_id(expected_profile_id)
+        .await?;
     let now = chrono::Utc::now().timestamp_millis();
     sqlx::query(
         "INSERT INTO profile_setting (key, value, value_type, updated_at)
@@ -291,10 +296,14 @@ pub async fn player_set_match_source_rate(
     .bind(now)
     .execute(&*pool)
     .await?;
-    engine
-        .shared()
-        .match_source_rate
-        .store(enabled, std::sync::atomic::Ordering::Release);
+    drop(pool);
+    crate::state::while_profile_is(&state.profile, Some(written_profile), || {
+        engine
+            .shared()
+            .match_source_rate
+            .store(enabled, std::sync::atomic::Ordering::Release);
+    })
+    .await;
     Ok(())
 }
 
@@ -349,11 +358,15 @@ pub async fn player_set_pause_on_device_loss(
     state: tauri::State<'_, AppState>,
     engine: tauri::State<'_, Arc<AudioEngine>>,
     enabled: bool,
+    expected_profile_id: Option<i64>,
 ) -> AppResult<()> {
     // Persisted first, same as `player_set_match_source_rate` and for
     // the same reason: a setting that only ever lived in memory is a
-    // setting the user will find undone.
-    let pool = state.require_profile_pool().await?;
+    // setting the user will find undone. Bound to the profile the same
+    // way too.
+    let (pool, written_profile) = state
+        .require_profile_pool_with_id(expected_profile_id)
+        .await?;
     let now = chrono::Utc::now().timestamp_millis();
     let stored = if enabled { "1" } else { "0" };
     sqlx::query(
@@ -365,7 +378,11 @@ pub async fn player_set_pause_on_device_loss(
     .bind(now)
     .execute(&*pool)
     .await?;
-    engine.set_pause_on_device_loss(enabled);
+    drop(pool);
+    crate::state::while_profile_is(&state.profile, Some(written_profile), || {
+        engine.set_pause_on_device_loss(enabled)
+    })
+    .await;
     Ok(())
 }
 
@@ -409,10 +426,10 @@ pub async fn player_set_keep_awake(
     // applied while this is still the active profile, or not at all — a
     // switch that got in first has run, or will run, the next profile's
     // own restore, which must not be overwritten with this one's value.
-    let active = state.profile.read().await;
-    if active.as_ref().map(|p| p.profile_id) == Some(written_profile) {
-        crate::power::set_enabled(enabled);
-    }
+    crate::state::while_profile_is(&state.profile, Some(written_profile), || {
+        crate::power::set_enabled(enabled)
+    })
+    .await;
     Ok(())
 }
 

src-tauri/crates/app/src/commands/player/setting_write_tests.rs

@@ -0,0 +1,134 @@
+//! A setting command writes into the profile its control was acted on, or
+//! nowhere (issue #996). Driven through AutoMix's command body, on a
+//! profile slot of the test's own: an `AppState` cannot be built here.
+
+use sqlx::SqlitePool;
+use tokio::sync::RwLock;
+
+use super::*;
+use crate::{
+    audio::state::SharedPlayback,
+    error::AppError,
+    state::{while_profile_is, ActiveProfile},
+};
+
+/// A profile database with the repo's migrations, `foreign_keys` on as on
+/// every profile pool.
+async fn profile_db(dir: &std::path::Path, name: &str) -> SqlitePool {
+    use sqlx::sqlite::SqliteConnectOptions;
+    use std::str::FromStr;
+    let opts = SqliteConnectOptions::from_str(&format!("sqlite://{}", dir.join(name).display()))
+        .unwrap()
+        .create_if_missing(true)
+        .foreign_keys(true);
+    let pool = SqlitePool::connect_with(opts).await.unwrap();
+    sqlx::migrate!("../../migrations/profile")
+        .run(&pool)
+        .await
+        .unwrap();
+    pool
+}
+
+async fn automix_row(pool: &SqlitePool) -> Option<String> {
+    sqlx::query_scalar("SELECT value FROM profile_setting WHERE key = 'audio.automix'")
+        .fetch_optional(pool)
+        .await
+        .unwrap()
+}
+
+#[tokio::test]
+async fn a_toggle_lands_in_the_profile_it_was_flipped_in() {
+    let dir = tempfile::tempdir().unwrap();
+    let a = profile_db(dir.path(), "a.db").await;
+    let slot = RwLock::new(Some(ActiveProfile::for_test(1, a.clone())));
+    let shared = SharedPlayback::new();
+
+    set_automix(&slot, &shared, true, Some(1)).await.unwrap();
+
+    assert_eq!(automix_row(&a).await.as_deref(), Some("true"));
+    assert!(shared.automix.is_enabled());
+}
+
+#[tokio::test]
+async fn a_toggle_for_a_profile_since_left_is_refused() {
+    let dir = tempfile::tempdir().unwrap();
+    let a = profile_db(dir.path(), "a.db").await;
+    let b = profile_db(dir.path(), "b.db").await;
+    let slot = RwLock::new(Some(ActiveProfile::for_test(1, a.clone())));
+    let shared = SharedPlayback::new();
+
+    // Flipped in profile 1; the switch to profile 2 lands before the
+    // command reaches the pool.
+    *slot.write().await = Some(ActiveProfile::for_test(2, b.clone()));
+    let err = set_automix(&slot, &shared, true, Some(1))
+        .await
+        .unwrap_err();
+
+    assert!(
+        matches!(
+            err,
+            AppError::ProfileChanged {
+                expected: 1,
+                active: Some(2)
+            }
+        ),
+        "unexpected error: {err}",
+    );
+    // Nothing written anywhere, and the engine keeps what profile 2's
+    // restore put there.
+    assert_eq!(automix_row(&b).await, None);
+    assert_eq!(automix_row(&a).await, None);
+    assert!(!shared.automix.is_enabled());
+}
+
+#[tokio::test]
+async fn a_toggle_for_a_profile_since_closed_is_refused() {
+    let slot = RwLock::new(None);
+    let shared = SharedPlayback::new();
+
+    let err = set_automix(&slot, &shared, true, Some(1))
+        .await
+        .unwrap_err();
+
+    assert!(
+        matches!(
+            err,
+            AppError::ProfileChanged {
+                expected: 1,
+                active: None
+            }
+        ),
+        "unexpected error: {err}",
+    );
+    assert!(!shared.automix.is_enabled());
+}
+
+#[tokio::test]
+async fn no_profile_in_mind_and_none_active_applies_without_saving() {
+    // The contract the setters always had: a caller with no profile in
+    // mind, with none open, still changes what plays.
+    let slot = RwLock::new(None);
+    let shared = SharedPlayback::new();
+
+    set_automix(&slot, &shared, true, None).await.unwrap();
+
+    assert!(shared.automix.is_enabled());
+}
+
+#[tokio::test]
+async fn the_engine_step_runs_only_for_the_active_profile() {
+    // The guard `save_then_apply` puts around the engine step, after the
+    // save: once profile 2 is active, a step for profile 1 is skipped, so
+    // a command that saved into profile 1 just before the switch cannot
+    // overwrite profile 2's restore.
+    let dir = tempfile::tempdir().unwrap();
+    let a = profile_db(dir.path(), "a.db").await;
+    let b = profile_db(dir.path(), "b.db").await;
+    let slot = RwLock::new(Some(ActiveProfile::for_test(1, a)));
+
+    *slot.write().await = Some(ActiveProfile::for_test(2, b));
+
+    assert_eq!(while_profile_is(&slot, Some(1), || ()).await, None);
+    assert_eq!(while_profile_is(&slot, Some(2), || 7).await, Some(7));
+    assert_eq!(while_profile_is(&slot, None, || ()).await, None);
+}

src-tauri/crates/app/src/commands/player/settings.rs

@@ -3,11 +3,13 @@
 
 use serde::Serialize;
 use tauri::{AppHandle, Emitter};
+use tokio::sync::RwLock;
 
 use crate::{
     audio::{replay_gain::GainMode, AudioEngine},
-    error::AppResult,
+    error::{AppError, AppResult},
     queue::{self},
+    state::{lease_profile, while_profile_is, ActiveProfile},
 };
 
 use super::*;
@@ -130,6 +132,60 @@ pub(super) async fn persist_setting(
     Ok(())
 }
 
+/// The `profile_setting` row of an on/off setting, spelled the way the
+/// setters have always written it.
+pub(super) fn bool_row(key: &'static str, enabled: bool) -> (&'static str, String, SettingType) {
+    (
+        key,
+        if enabled { "true" } else { "false" }.to_string(),
+        SettingType::Bool,
+    )
+}
+
+/// Save a setting into the profile its control was acted on, then apply
+/// it to the engine (issue #996).
+///
+/// `expected` is that profile's id, as the frontend captured it when the
+/// user acted:
+///
+/// - **`Some(id)`** — the pool is leased only if `id` is still the active
+///   profile, checked under the lock a switch takes; otherwise the command
+///   fails with [`AppError::ProfileChanged`](crate::error::AppError) and
+///   neither saves nor applies anything. A switch landing between the
+///   click and the command can no longer write one profile's choice into
+///   the next one.
+/// - **`None`** — whatever profile is active, for a caller with no profile
+///   in mind. With none active at all the setting is applied without being
+///   saved, the contract these commands have always had.
+///
+/// Saved first, then applied: a value the database refuses never reaches
+/// the engine. The lease is released before the engine is touched, and
+/// the engine is touched only while the profile the rows went into is
+/// still the active one ([`while_profile_is`]): a switch that got in
+/// between has restored, or is about to restore, the next profile's own
+/// values, and a stale command must not overwrite them. The rows are saved
+/// either way — they belong to that profile.
+pub(super) async fn save_then_apply(
+    profile: &RwLock<Option<ActiveProfile>>,
+    expected: Option<i64>,
+    rows: &[(&str, String, SettingType)],
+    apply: impl FnOnce() -> AppResult<()>,
+) -> AppResult<()> {
+    let written = match lease_profile(profile, expected).await {
+        Ok((pool, profile_id)) => {
+            for (key, value, value_type) in rows {
+                persist_setting(&pool, key, value, *value_type).await?;
+            }
+            Some(profile_id)
+        }
+        Err(AppError::NoActiveProfile) => None,
+        Err(err) => return Err(err),
+    };
+    while_profile_is(profile, written, apply)
+        .await
+        .unwrap_or(Ok(()))
+}
+
 /// Read a boolean `profile_setting`. `"true"` / `"1"` read as true and
 /// `"false"` / `"0"` as false; a row that is missing, unreadable or
 /// holds anything else resolves to `default`.