From a6e0bf0ab75ada1de7c9c53c118e230c50c85b4e Mon Sep 17 00:00:00 2001 From: LynithDev <61880709+LynithDev@users.noreply.github.com> Date: Thu, 6 Aug 2026 20:17:28 +0200 Subject: [PATCH] fix(core): bundle overrides being problematic pt. 2 --- ...83b933c2f6788e2d2482ca9fd7d71ee67381a.json | 12 ++ .../src/hooks/queries/mutations.rs | 14 +- .../oneclient_content/src/bundles/install.rs | 166 ++++++++++++++---- .../oneclient_content/src/bundles/updates.rs | 34 ++-- ...83b933c2f6788e2d2482ca9fd7d71ee67381a.json | 12 ++ ...120000_repair_orphaned_bundle_disables.sql | 53 ++++-- .../oneclient_db/src/dao/cluster_bundle.rs | 22 +++ 7 files changed, 239 insertions(+), 74 deletions(-) create mode 100644 .sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json create mode 100644 packages/oneclient_db/.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json diff --git a/.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json b/.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json new file mode 100644 index 00000000..6ddb354e --- /dev/null +++ b/.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" +} diff --git a/packages/oneclient_app/src/hooks/queries/mutations.rs b/packages/oneclient_app/src/hooks/queries/mutations.rs index 44d2d397..68759673 100644 --- a/packages/oneclient_app/src/hooks/queries/mutations.rs +++ b/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 diff --git a/packages/oneclient_content/src/bundles/install.rs b/packages/oneclient_content/src/bundles/install.rs index b16da13c..53bee7bf 100644 --- a/packages/oneclient_content/src/bundles/install.rs +++ b/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 { + 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) -> 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) -> 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![ diff --git a/packages/oneclient_content/src/bundles/updates.rs b/packages/oneclient_content/src/bundles/updates.rs index aff8b531..f17e3e7f 100644 --- a/packages/oneclient_content/src/bundles/updates.rs +++ b/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, diff --git a/packages/oneclient_db/.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json b/packages/oneclient_db/.sqlx/query-43521070cb5de5b2fe5e25c88e683b933c2f6788e2d2482ca9fd7d71ee67381a.json new file mode 100644 index 00000000..6ddb354e --- /dev/null +++ b/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" +} diff --git a/packages/oneclient_db/migrations/20260806120000_repair_orphaned_bundle_disables.sql b/packages/oneclient_db/migrations/20260806120000_repair_orphaned_bundle_disables.sql index 59bc810d..1f24491e 100644 --- a/packages/oneclient_db/migrations/20260806120000_repair_orphaned_bundle_disables.sql +++ b/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')); diff --git a/packages/oneclient_db/src/dao/cluster_bundle.rs b/packages/oneclient_db/src/dao/cluster_bundle.rs index f10dbe06..abcde1ec 100644 --- a/packages/oneclient_db/src/dao/cluster_bundle.rs +++ b/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,