fix(core): bundle overrides being problematic pt. 2

.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json

@@ -0,0 +1,12 @@
+{
+  "db_name": "SQLite",
+  "query": "\n        DELETE FROM cluster_bundle_overrides\n        WHERE cluster_id = ? AND package_id = ? AND override_type IN ('disabled', 'removed')\n        ",
+  "describe": {
+    "columns": [],
+    "parameters": {
+      "Right": 2
+    },
+    "nullable": []
+  },
+  "hash": "43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a"
+}

packages/oneclient_app/src/hooks/queries/mutations.rs

@@ -2,7 +2,7 @@ use std::path::PathBuf;
 
 use freya::query::{Mutation, MutationCapability, QueriesStorage, UseMutation, use_mutation};
 use oneclient_content::packages::{ContentType, PackageStore};
-use oneclient_db::models::{ClusterId, OverrideType};
+use oneclient_db::models::ClusterId;
 
 use super::bundles::{BundleOverridesQuery, BundleUpdatesQuery, BundlesWithStatusQuery};
 use super::cluster_content::ClusterContentQuery;
@@ -116,18 +116,12 @@ impl MutationCapability for ClusterMutation {
                 enabled,
                 manifest_default,
             } => {
-                // Matching the manifest default means "no opinion" -> clear the
-                // override. Contradicting it pins the choice in either direction.
-                let override_type = match (*enabled, *manifest_default) {
-                    (true, true) | (false, false) => None,
-                    (true, false) => Some(OverrideType::Enabled),
-                    (false, true) => Some(OverrideType::Disabled),
-                };
-                oneclient_core::set_bundle_package_override(
+                oneclient_core::set_bundle_package_enabled(
                     *cluster_id,
                     bundle_name,
                     package_id,
-                    override_type,
+                    *enabled,
+                    *manifest_default,
                     content,
                 )
                 .await

packages/oneclient_content/src/bundles/install.rs

@@ -39,17 +39,52 @@ fn find_override(
         .and_then(|o| OverrideType::parse(&o.override_type))
 }
 
-async fn clear_suppressing_override(
+/// The user's standing objection to a package, wherever it happens to be
+/// recorded.
+///
+/// Overrides are stored per bundle, but a package does not stay in one:
+/// `track_bundle_artifact` rewrites `bundle_name` on every install, and the
+/// update flow deliberately re-resolves a package to whichever loaded bundle now
+/// ships it. Asking only the bundle an artifact currently sits under therefore
+/// misses a choice the user made while it sat somewhere else — and reading that
+/// silence as consent is what switches a mod they turned off back on.
+///
+/// `Removed` outranks `Disabled` where a package is spoken for twice. An
+/// `Enabled` opt-in is not an objection and never suppresses.
+///
+/// Deliberately wider than [`find_override`], which answers a different
+/// question: which manifest default one bundle's file should take, where the
+/// bundle is exactly what identifies the file.
+pub(crate) fn find_user_suppression(
+    overrides: &[oneclient_db::models::ClusterBundleOverrideRow],
+    package_id: &str,
+) -> Option<OverrideType> {
+    let mut found = None;
+
+    for row in overrides.iter().filter(|o| o.package_id == package_id) {
+        match OverrideType::parse(&row.override_type) {
+            Some(OverrideType::Removed) => return Some(OverrideType::Removed),
+            Some(OverrideType::Disabled) => found = Some(OverrideType::Disabled),
+            Some(OverrideType::Enabled) | None => {}
+        }
+    }
+
+    found
+}
+
+/// Drops the user's objections to a package everywhere they were recorded.
+///
+/// The inverse of [`find_user_suppression`], and it has to be: an objection that
+/// counts no matter which bundle wrote it has to be clearable no matter which
+/// bundle wrote it. Clearing only the current bundle's row would leave one
+/// behind that keeps answering "off" to every later question, which is the
+/// stuck state this whole area exists to prevent.
+async fn clear_suppressing_overrides(
     cluster_id: i64,
-    bundle_name: &str,
     package_id: &str,
     ctx: &ContentCtx,
 ) -> ContentResult<()> {
-    let overrides = bundle_dao::list_overrides(&ctx.db, cluster_id).await?;
-    if let Some(OverrideType::Enabled) = find_override(&overrides, bundle_name, package_id) {
-        return Ok(());
-    }
-    bundle_dao::remove_override(&ctx.db, cluster_id, bundle_name, package_id).await?;
+    bundle_dao::clear_suppressing_overrides(&ctx.db, cluster_id, package_id).await?;
     Ok(())
 }
 
@@ -174,8 +209,11 @@ pub async fn install_bundle(
 }
 
 /// Whether a bundle file that is switched off was switched off on purpose.
-fn disable_was_deliberate(hidden: bool, user_override: Option<OverrideType>) -> bool {
-    match user_override {
+///
+/// `suppression` comes from [`find_user_suppression`], so a choice filed under a
+/// bundle the file has since left still counts.
+pub(crate) fn disable_was_deliberate(hidden: bool, suppression: Option<OverrideType>) -> bool {
+    match suppression {
         Some(OverrideType::Removed) => true,
         // Hidden files are dependencies the bundle needs and the user was never
         // shown, so they follow the bundle rather than standing on their own.
@@ -221,8 +259,11 @@ pub async fn heal_bundle_activity(
             continue;
         };
 
-        let user_override = find_override(&overrides, bundle_name, package_id);
-        if disable_was_deliberate(is_hidden, user_override) {
+        // Across bundles, not just this one. A package that moved bundles keeps
+        // its old override row, and reading only the bundle it sits under today
+        // would take that silence for consent and switch it back on.
+        let suppression = find_user_suppression(&overrides, package_id);
+        if disable_was_deliberate(is_hidden, suppression) {
             continue;
         }
 
@@ -231,7 +272,7 @@ pub async fn heal_bundle_activity(
             bundle = %bundle_name,
             package_id,
             hidden = is_hidden,
-            ?user_override,
+            ?suppression,
             "re-enabling bundle content that was switched off with nothing recording the choice"
         );
 
@@ -245,10 +286,11 @@ pub async fn heal_bundle_activity(
         }
 
         // A hidden file reaching here with a `disabled` row is the one case where
-        // the override is the wrong half. Drop it, or the next pass reads the
-        // same disagreement and the two records never settle.
-        if user_override == Some(OverrideType::Disabled) {
-            bundle_dao::remove_override(&ctx.db, cluster_id, bundle_name, package_id).await?;
+        // the override is the wrong half. Drop it — everywhere, or the copy left
+        // under some other bundle keeps answering "off" and the two records never
+        // settle.
+        if suppression == Some(OverrideType::Disabled) {
+            clear_suppressing_overrides(cluster_id, package_id, ctx).await?;
         }
     }
 
@@ -424,6 +466,15 @@ pub async fn set_bundle_package_override(
     ctx: &ContentCtx,
 ) -> ContentResult<()> {
     match override_type {
+        // Saying "on" has to mean on. The package manager shows one row per
+        // package however many bundles ship it, so a switch flipped there is a
+        // statement about the package — and leaving an objection standing under
+        // some other bundle would let the next bundle pass read it and undo the
+        // switch the user just flipped.
+        Some(ty @ OverrideType::Enabled) => {
+            clear_suppressing_overrides(cluster_id, package_id, ctx).await?;
+            bundle_dao::save_override(&ctx.db, cluster_id, bundle_name, package_id, ty).await?;
+        }
         Some(ty) => {
             bundle_dao::save_override(&ctx.db, cluster_id, bundle_name, package_id, ty).await?;
         }
@@ -435,20 +486,41 @@ pub async fn set_bundle_package_override(
     Ok(())
 }
 
+/// The package manager's switch, for a bundle file the cluster has not installed
+/// (an installed one goes through [`toggle_artifact_enabled`] instead).
+///
+/// Matching the manifest default is "no opinion" and clears the override;
+/// contradicting it pins the choice. Either way, switching a package *on* also
+/// drops any objection filed against it under another bundle — the package
+/// manager shows one row per package however many bundles ship it, so the switch
+/// is a statement about the package, and a row left standing elsewhere would be
+/// read by the next bundle pass and quietly undo it.
+///
+/// Which is why the mapping lives here rather than at the call site: `None`
+/// alone cannot tell "on, and the default agrees" from "off, and the default
+/// agrees", and only the first of those should clear anything.
 #[tracing::instrument(level = "debug", skip(ctx))]
 pub async fn set_bundle_package_enabled(
     cluster_id: i64,
     bundle_name: &str,
     package_id: &str,
     enabled: bool,
+    manifest_default: bool,
     ctx: &ContentCtx,
 ) -> ContentResult<()> {
-    let override_type = if enabled {
-        None
-    } else {
-        Some(OverrideType::Disabled)
+    let override_type = match (enabled, manifest_default) {
+        (true, true) | (false, false) => None,
+        (true, false) => Some(OverrideType::Enabled),
+        (false, true) => Some(OverrideType::Disabled),
     };
-    set_bundle_package_override(cluster_id, bundle_name, package_id, override_type, ctx).await
+
+    set_bundle_package_override(cluster_id, bundle_name, package_id, override_type, ctx).await?;
+
+    if enabled {
+        clear_suppressing_overrides(cluster_id, package_id, ctx).await?;
+    }
+
+    Ok(())
 }
 
 #[tracing::instrument(level = "debug", skip(overrides, ctx), fields(count = overrides.len()))]
@@ -618,8 +690,8 @@ pub async fn on_user_enable_artifact(
     ctx: &ContentCtx,
 ) -> ContentResult<()> {
     if let Some(tracked) = bundle_dao::get_bundle_tracked(&ctx.db, cluster_id, hash).await?
-        && let (Some(bundle_name), Some(package_id)) = (tracked.bundle_name, tracked.package_id) {
-            clear_suppressing_override(cluster_id, &bundle_name, &package_id, ctx).await?;
+        && let Some(package_id) = tracked.package_id {
+            clear_suppressing_overrides(cluster_id, &package_id, ctx).await?;
         }
     Ok(())
 }
@@ -715,13 +787,7 @@ pub async fn remove_artifact_from_cluster(
                 )
                 .await?;
                 if !replacement_exists {
-                    clear_suppressing_override(
-                        cluster_id,
-                        &bundle_name,
-                        &package_id,
-                        ctx,
-                    )
-                    .await?;
+                    clear_suppressing_overrides(cluster_id, &package_id, ctx).await?;
                 }
             }
         }
@@ -820,6 +886,46 @@ mod tests {
         assert!(!disable_was_deliberate(true, Some(OverrideType::Enabled)));
     }
 
+    #[test]
+    fn suppression_is_found_under_a_bundle_the_package_has_since_left() {
+        let rows = vec![row("Bundle B", "fabric-api", OverrideType::Disabled)];
+
+        assert_eq!(
+            find_override(&rows, "Bundle C", "fabric-api"),
+            None,
+            "the per-bundle question is still answered per bundle"
+        );
+        assert_eq!(
+            find_user_suppression(&rows, "fabric-api"),
+            Some(OverrideType::Disabled),
+            "the user's choice follows the package, not the bundle it was filed under"
+        );
+    }
+
+    #[test]
+    fn removal_outranks_a_disable_filed_elsewhere() {
+        let rows = vec![
+            row("Bundle A", "yacl", OverrideType::Disabled),
+            row("Bundle B", "yacl", OverrideType::Removed),
+        ];
+
+        assert_eq!(
+            find_user_suppression(&rows, "yacl"),
+            Some(OverrideType::Removed)
+        );
+    }
+
+    #[test]
+    fn an_opt_in_is_not_an_objection() {
+        let rows = vec![
+            row("Bundle A", "sodium", OverrideType::Enabled),
+            row("Bundle B", "lithium", OverrideType::Disabled),
+        ];
+
+        assert_eq!(find_user_suppression(&rows, "sodium"), None);
+        assert_eq!(find_user_suppression(&rows, "unheard-of"), None);
+    }
+
     #[test]
     fn find_override_matches_on_bundle_and_package() {
         let rows = vec![

packages/oneclient_content/src/bundles/updates.rs

@@ -12,8 +12,9 @@ use tokio::sync::Mutex as AsyncMutex;
 use futures_util::StreamExt;
 
 use crate::bundles::install::{
-    BUNDLE_INSTALL_CONCURRENCY, heal_bundle_activity, install_package_from_bundle,
-    remove_artifact_from_cluster, set_artifact_enabled_to,
+    BUNDLE_INSTALL_CONCURRENCY, disable_was_deliberate, find_user_suppression,
+    heal_bundle_activity, install_package_from_bundle, remove_artifact_from_cluster,
+    set_artifact_enabled_to,
 };
 use crate::bundles::manager::BundlesManager;
 use crate::bundles::overrides;
@@ -563,8 +564,15 @@ pub async fn apply_bundle_updates_with(
 ///
 /// The choice is *set*, not flipped. Linking an artifact leaves an existing row's
 /// `enabled` alone, so re-applying an update to a row that already read correctly
-/// used to turn it into its opposite — which is the "my mods turn themselves off
-/// at random" report.
+/// used to turn it into its opposite.
+///
+/// It is also read across bundles. `update.bundle_name` is the bundle that ships
+/// the file *now* — [`check_bundle_updates_inner`] re-resolves a package to
+/// whichever loaded bundle carries it — and a file only reaches here at all
+/// because it passed `effective_enabled` under that bundle, so asking that
+/// bundle alone whether the user objects can only ever answer no. Consulting
+/// only it would forcibly enable every package whose objection was filed while
+/// it lived somewhere else.
 #[tracing::instrument(level = "debug", skip_all, fields(cluster_id = update.cluster_id, bundle = %update.bundle_name, new_version = %update.new_version_id))]
 async fn reconcile_update(
     update: &BundlePackageUpdate,
@@ -573,7 +581,8 @@ async fn reconcile_update(
     ctx: &ContentCtx,
 ) -> ContentResult<()> {
     let file_id = update.new_file.kind.package_id();
-    let enabled = !should_be_disabled(&update.bundle_name, &file_id, overrides);
+    let suppression = find_user_suppression(overrides, &file_id);
+    let enabled = !disable_was_deliberate(update.new_file.hidden, suppression);
     set_artifact_enabled_to(update.cluster_id, hash, enabled, ctx).await?;
 
     if hash == update.installed_hash {
@@ -591,22 +600,11 @@ async fn reconcile_addition(
     ctx: &ContentCtx,
 ) -> ContentResult<()> {
     let file_id = addition.new_file.kind.package_id();
-    let enabled = !should_be_disabled(&addition.bundle_name, &file_id, overrides);
+    let suppression = find_user_suppression(overrides, &file_id);
+    let enabled = !disable_was_deliberate(addition.new_file.hidden, suppression);
     set_artifact_enabled_to(addition.cluster_id, hash, enabled, ctx).await
 }
 
-fn should_be_disabled(
-    bundle_name: &str,
-    package_id: &str,
-    overrides: &[ClusterBundleOverrideRow],
-) -> bool {
-    overrides.iter().any(|o| {
-        o.bundle_name == bundle_name
-            && o.package_id == package_id
-            && OverrideType::parse(&o.override_type) == Some(OverrideType::Disabled)
-    })
-}
-
 #[tracing::instrument(level = "debug", skip(bundles, ctx))]
 async fn sync_cluster_loader_version_from_bundles(
     cluster_id: i64,

packages/oneclient_db/.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json

@@ -0,0 +1,12 @@
+{
+  "db_name": "SQLite",
+  "query": "\n        DELETE FROM cluster_bundle_overrides\n        WHERE cluster_id = ? AND package_id = ? AND override_type IN ('disabled', 'removed')\n        ",
+  "describe": {
+    "columns": [],
+    "parameters": {
+      "Right": 2
+    },
+    "nullable": []
+  },
+  "hash": "43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a"
+}

packages/oneclient_db/migrations/20260806120000_repair_orphaned_bundle_disables.sql

@@ -1,23 +1,44 @@
--- Switches bundle content back on where it was left off with nothing recording
--- the choice.
+-- Repairs bundle content whose two records of one decision came apart.
 --
--- `cluster_artifacts.enabled` and `cluster_bundle_overrides` are two records of
--- one decision and they were able to come apart: duplicate resolution wrote the
--- flag without the override, and re-resolving a package to a different bundle
--- deleted the override without the flag. Either way the artifact ends up off
--- with no override, which nothing could then repair -- the update flow decides
--- from the override and the manifest default, never from `enabled`, so it read
--- the package as installed and enabled and never touched it again. For a
--- bundle's hidden dependencies that meant a library the pack needs never
--- reaching `mods/`.
+-- `cluster_artifacts.enabled` and `cluster_bundle_overrides` both record whether
+-- a package is wanted, and the launcher was able to write one without the other:
+-- duplicate resolution wrote the flag with no override, and re-resolving a
+-- package to a different bundle deleted the override with no flag. Either way
+-- the artifact ends up off with nothing recording why, which nothing could then
+-- repair -- the update flow decides from the override and the manifest default
+-- and never reads `enabled`, so it saw the package as installed and enabled and
+-- never touched it again. For a bundle's hidden dependencies that meant a
+-- library the pack needs never reaching `mods/`.
+
+-- First, the objections that are no longer about anything. Declining a bundle at
+-- onboarding recorded `removed` for its hidden files, including libraries that
+-- an accepted bundle right beside it ships too -- so a cluster ends up claiming
+-- to have removed a package it demonstrably installed, under a different bundle.
+-- Left in place these answer for the package in every lookup that resolves a
+-- user's choice across bundles, which is now how a choice is resolved.
+DELETE FROM cluster_bundle_overrides
+WHERE override_type = 'removed'
+  AND EXISTS (SELECT 1 FROM cluster_artifacts a
+              WHERE a.cluster_id  = cluster_bundle_overrides.cluster_id
+                AND a.bundle_name IS NOT NULL
+                AND a.bundle_name <> cluster_bundle_overrides.bundle_name
+                AND a.package_id  = cluster_bundle_overrides.package_id);
+
+-- Then the flag itself. A genuine user disable always leaves a `disabled` or
+-- `removed` row behind, so those are matched here and stay off -- under any
+-- bundle, not just the one the artifact sits under today, because a package that
+-- moved bundles keeps its old row and that row still speaks for the user.
 --
--- A genuine user disable always leaves a `disabled` or `removed` row for the
--- same bundle, so those are matched here and stay off.
+-- Deliberately narrower than the runtime pass in `bundles::heal_bundle_activity`,
+-- which also frees hidden files from a `disabled` row on the grounds that a
+-- dependency the user was never shown cannot have been turned off on purpose.
+-- Whether a file is hidden is a manifest fact and there is no manifest here, so
+-- those wait for the next bundle apply rather than being guessed at.
 UPDATE cluster_artifacts SET enabled = 1
 WHERE bundle_name IS NOT NULL
   AND package_id IS NOT NULL
   AND enabled = 0
   AND NOT EXISTS (SELECT 1 FROM cluster_bundle_overrides o
-                  WHERE o.cluster_id   = cluster_artifacts.cluster_id
-                    AND o.bundle_name  = cluster_artifacts.bundle_name
-                    AND o.package_id   = cluster_artifacts.package_id);
+                  WHERE o.cluster_id    = cluster_artifacts.cluster_id
+                    AND o.package_id    = cluster_artifacts.package_id
+                    AND o.override_type IN ('disabled', 'removed'));

packages/oneclient_db/src/dao/cluster_bundle.rs

@@ -189,6 +189,28 @@ pub async fn remove_override(
     Ok(())
 }
 
+/// Drops every objection the cluster holds against a package, under any bundle.
+///
+/// Opt-in (`enabled`) rows are left alone: they say the opposite thing, and
+/// clearing an objection is not a reason to forget one.
+pub async fn clear_suppressing_overrides(
+    pool: &SqlitePool,
+    cluster_id: i64,
+    package_id: &str,
+) -> Result<(), sqlx::Error> {
+    sqlx::query!(
+        r#"
+        DELETE FROM cluster_bundle_overrides
+        WHERE cluster_id = ? AND package_id = ? AND override_type IN ('disabled', 'removed')
+        "#,
+        cluster_id,
+        package_id
+    )
+    .execute(pool)
+    .await?;
+    Ok(())
+}
+
 pub async fn list_overrides(
     pool: &SqlitePool,
     cluster_id: i64,