fix: issues with mod updating and mods randomly being disabled (#830)

* fix: stop offering bundle-owned packages in the update modal

* fix: match a version's game versions exactly not by substring

* fix: resolve a mod's materialized root from the layout in use

* fix: stop parking unclaimed jars in the cluster mods folder

* fix: stop the duplicate reconciler filing a user-disabled override

* fix: repair the disables duplicate resolution left behind
This commit is contained in:
LynithDev 2026-09-14 16:44:46 +02:00 • committed by GitHub
parent a8bdf997d9
commit 2e0ba533fe
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 169 additions and 65 deletions

View file

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

View file

@ -1083,7 +1083,7 @@ impl Actions {
if install.result.is_ok() {
// Before the refresh below so the version list never renders
// the moment where both copies read as active
if let Err(err) = oneclient_content::bundles::reconcile_duplicate_activity(
if let Err(err) = oneclient_content::packages::reconcile_duplicate_activity(
cluster_id,
&state.services.content(),
)

View file

@ -1,4 +1,5 @@
use futures_util::StreamExt;
use oneclient_db::dao::applied_migration as migration_dao;
use oneclient_db::dao::artifact as artifact_dao;
use oneclient_db::dao::cluster as cluster_dao;
use oneclient_db::dao::cluster_bundle as bundle_dao;
@ -202,12 +203,54 @@ pub(crate) fn disable_was_deliberate(suppression: Option<OverrideType>) -> bool
}
}
const DUPLICATE_DISABLE_REPAIR: &str = "repair-hidden-duplicate-disables";
async fn clear_reconciler_disables(
cluster_id: i64,
archives: &[BundleArchive],
ctx: &ContentCtx,
) -> ContentResult<()> {
let id = format!("{DUPLICATE_DISABLE_REPAIR}:{cluster_id}");
if migration_dao::is_applied(&ctx.db, &id).await? {
return Ok(());
}
let hidden: std::collections::HashSet<String> = archives
.iter()
.flat_map(|archive| &archive.manifest.files)
.filter(|file| file.hidden)
.map(|file| file.kind.package_id())
.collect();
if hidden.is_empty() {
return Ok(());
}
let mut cleared = 0u64;
for package_id in &hidden {
cleared += bundle_dao::clear_disabled_overrides(&ctx.db, cluster_id, package_id).await?;
}
if cleared > 0 {
tracing::info!(
cluster_id,
cleared,
"cleared disabled overrides on hidden bundle files that no user was ever shown"
);
}
migration_dao::mark_applied(&ctx.db, &id).await?;
Ok(())
}
#[tracing::instrument(level = "debug", skip(archives, ctx))]
pub async fn heal_bundle_activity(
cluster_id: i64,
archives: &[BundleArchive],
ctx: &ContentCtx,
) -> ContentResult<()> {
clear_reconciler_disables(cluster_id, archives, ctx).await?;
let tracked = bundle_dao::list_bundle_tracked(&ctx.db, cluster_id).await?;
if tracked.iter().all(|row| row.enabled != 0) {
return Ok(());
@ -226,10 +269,20 @@ pub async fn heal_bundle_activity(
let overrides = bundle_dao::list_overrides(&ctx.db, cluster_id).await?;
let mut live_packages: std::collections::HashSet<String> = tracked
.iter()
.filter(|row| row.enabled != 0)
.filter_map(|row| row.package_id.clone())
.collect();
for row in tracked.iter().filter(|row| row.enabled == 0) {
let (Some(bundle_name), Some(package_id)) = (&row.bundle_name, &row.package_id) else {
continue;
};
if live_packages.contains(package_id) {
continue;
}
let Some(is_hidden) = hidden
.get(&(bundle_name.as_str(), package_id.clone()))
.copied()
@ -261,6 +314,8 @@ pub async fn heal_bundle_activity(
tracing::warn!(hash = %row.hash, error = %err, "failed to re-enable bundle content");
continue;
}
live_packages.insert(package_id.clone());
}
Ok(())
@ -584,21 +639,6 @@ pub async fn set_artifact_enabled_to(
Ok(live)
}
/// Writes the override alongside the flag
/// without it the losing bundle copy looks disabled-by-nobody and
/// heal_bundle_activity re-enables it every launch
#[tracing::instrument(level = "debug", skip(ctx))]
pub async fn reconcile_duplicate_activity(
cluster_id: i64,
ctx: &ContentCtx,
) -> ContentResult<()> {
for hash in crate::packages::reconcile_duplicate_activity(cluster_id, ctx).await? {
on_user_disable_artifact(cluster_id, &hash, ctx).await?;
}
Ok(())
}
// which clusters have to record what the user just did
async fn override_scope(
cluster_id: i64,

View file

@ -16,7 +16,6 @@ pub use install::{
install_cluster_bundles, install_enabled_bundle_files, install_package_from_bundle,
list_cluster_bundle_overrides,
on_user_disable_artifact, on_user_enable_artifact, on_user_remove_artifact,
reconcile_duplicate_activity,
remove_artifact_from_cluster, set_artifact_enabled_to, set_bundle_package_enabled,
set_bundle_package_opt_in,
set_bundle_package_override, set_bundle_package_overrides,

View file

@ -193,15 +193,13 @@ fn choose_version(
.cloned()
}
/// Mirrors the store's install-time compatibility check
/// must stay in sync with it
#[must_use]
pub(crate) fn supports_game_version(game_versions: &[String], mc_version: &str) -> bool {
game_versions.is_empty() || game_versions.iter().any(|v| v == mc_version)
}
fn fits_cluster(version: &VersionSummary, cluster: &ClusterRow, loader: GameLoader) -> bool {
if !version.game_versions.is_empty()
&& !version
.game_versions
.iter()
.any(|v| cluster.mc_version.contains(v))
{
if !supports_game_version(&version.game_versions, &cluster.mc_version) {
return false;
}

View file

@ -87,20 +87,21 @@ pub async fn remove_entry(path: &Path) -> ContentResult<()> {
Ok(())
}
fn materialized_root(
async fn materialized_root(
cluster: &ClusterRow,
content_type: ContentType,
) -> Option<(std::path::PathBuf, &'static str)> {
) -> Option<(PathBuf, &'static str)> {
if content_type.is_global() {
return paths::shared_minecraft_dir()
.ok()
.map(|dir| (dir, manifest::GLOBAL_MANIFEST_NAME));
}
if content_type == ContentType::Mod {
return paths::cluster_dir(&cluster.folder_name)
.ok()
.map(|dir| (dir, manifest::MODS_MANIFEST_NAME));
if content_type == ContentType::Mod
&& let Ok(dir) = paths::cluster_dir(&cluster.folder_name)
&& manifest::mods_live_in_cluster(&dir).await
{
return Some((dir, manifest::MODS_MANIFEST_NAME));
}
paths::cluster_game_dir(&cluster.folder_name)
@ -114,7 +115,7 @@ pub async fn try_unlink_materialized(
content_type: ContentType,
file_name: &str,
) -> bool {
let Some((root, manifest_name)) = materialized_root(cluster, content_type) else {
let Some((root, manifest_name)) = materialized_root(cluster, content_type).await else {
return false;
};
@ -165,7 +166,7 @@ pub async fn try_link_materialized(
return LiveSync::Skipped;
}
let Some((root, manifest_name)) = materialized_root(cluster, content_type) else {
let Some((root, manifest_name)) = materialized_root(cluster, content_type).await else {
return LiveSync::Deferred;
};

View file

@ -629,12 +629,10 @@ fn ensure_compatible(
}
}
if !version.game_versions.is_empty()
&& !version
.game_versions
.iter()
.any(|v| cluster.mc_version.contains(v))
{
if !crate::packages::dependencies::supports_game_version(
&version.game_versions,
&cluster.mc_version,
) {
return Err(PackageError::IncompatibleMcVersion.into());
}

View file

@ -91,10 +91,17 @@ struct Candidate {
fn browser_installed(
linked: &[LinkedArtifactInfo],
bundle_hashes: &HashSet<String>,
bundle_projects: &HashSet<String>,
) -> Vec<Candidate> {
linked
.iter()
.filter(|info| info.enabled)
.filter(|info| !bundle_hashes.contains(&info.hash))
.filter(|info| {
info.project_id
.as_ref()
.is_none_or(|id| !bundle_projects.contains(id))
})
.filter_map(|info| {
let provider = info.provider?;
if provider == ProviderId::Local {
@ -126,13 +133,12 @@ pub async fn check_browser_package_updates(
let cluster = PackageStore::get_cluster(cluster_id, ctx).await?;
let linked = PackageStore::list_linked_artifacts(cluster_id, ctx).await?;
let bundle_hashes: HashSet<String> = bundle_dao::list_bundle_tracked(&ctx.db, cluster_id)
.await?
.into_iter()
.map(|row| row.hash)
.collect();
let tracked = bundle_dao::list_bundle_tracked(&ctx.db, cluster_id).await?;
let bundle_projects: HashSet<String> =
tracked.iter().filter_map(|row| row.package_id.clone()).collect();
let bundle_hashes: HashSet<String> = tracked.into_iter().map(|row| row.hash).collect();
let candidates = browser_installed(&linked, &bundle_hashes);
let candidates = browser_installed(&linked, &bundle_hashes, &bundle_projects);
if candidates.is_empty() {
return Ok(BrowserUpdateCheck {
cluster_id,
@ -506,7 +512,7 @@ mod tests {
let bundles: HashSet<String> = ["a".to_string()].into_iter().collect();
assert!(
browser_installed(&linked, &bundles).is_empty(),
browser_installed(&linked, &bundles, &HashSet::new()).is_empty(),
"a bundle-tracked artifact must stay out of the browser update flow"
);
}
@ -520,7 +526,7 @@ mod tests {
Some("v1"),
)];
let found = browser_installed(&linked, &HashSet::new());
let found = browser_installed(&linked, &HashSet::new(), &HashSet::new());
assert_eq!(found.len(), 1);
assert_eq!(found[0].project_id, "sodium");
assert_eq!(found[0].version_id, "v1");
@ -534,7 +540,7 @@ mod tests {
];
assert!(
browser_installed(&linked, &HashSet::new()).is_empty(),
browser_installed(&linked, &HashSet::new(), &HashSet::new()).is_empty(),
"a file with no provider release has no version to compare"
);
}
@ -544,7 +550,7 @@ mod tests {
let linked = vec![linked("a", Some(ProviderId::Modrinth), Some("sodium"), None)];
assert!(
browser_installed(&linked, &HashSet::new()).is_empty(),
browser_installed(&linked, &HashSet::new(), &HashSet::new()).is_empty(),
"without an installed version id there is nothing to compare against"
);
}
@ -612,16 +618,15 @@ mod tests {
let mut info = linked("a", Some(ProviderId::Modrinth), Some("sodium"), Some("v1"));
info.published_at = Some("2026-02-01T00:00:00Z".into());
let found = browser_installed(&[info], &HashSet::new());
let found = browser_installed(&[info], &HashSet::new(), &HashSet::new());
assert_eq!(found[0].published_at, Some(at("2026-02-01T00:00:00Z")));
}
#[test]
fn an_unparseable_publish_date_leaves_the_candidate_undated() {
let mut info = linked("a", Some(ProviderId::Modrinth), Some("sodium"), Some("v1"));
info.published_at = Some("not a date".into());
let found = browser_installed(&[info], &HashSet::new());
let found = browser_installed(&[info], &HashSet::new(), &HashSet::new());
assert_eq!(found[0].published_at, None);
}
}

View file

@ -97,7 +97,7 @@ pub async fn materialize_content(
let ours = ours_in_folder(ContentType::Mod, &linked, previous.as_ref());
tracing::info!(cluster_id = cluster.id, "moving mods out of the shared game directory");
stash_content_files(&from, &into, &ours).await;
stash_content_files(&from, &into, ContentType::Mod, &ours).await;
}
if mods_in_cluster {
@ -136,7 +136,7 @@ pub async fn materialize_content(
import_manual_content_with(services, cluster, game_dir, mods_in_cluster).await;
if let Err(err) = oneclient_content::bundles::reconcile_duplicate_activity(
if let Err(err) = oneclient_content::packages::reconcile_duplicate_activity(
cluster.id,
&services.content(),
)
@ -184,7 +184,7 @@ pub async fn materialize_content(
polyio::create_dir_all(&dir).await.ok();
let ours = ours_in_folder(*content_type, &linked, previous.as_ref());
stash_content_files(&dir, &stash, &ours).await;
stash_content_files(&dir, &stash, *content_type, &ours).await;
ensure_note(&dir, *content_type).await;
restore_stashed(&stash, &dir, *content_type, &ours).await;
}
@ -556,7 +556,7 @@ pub async fn dematerialize_content(
polyio::create_dir_all(&dir).await.ok();
let ours = ours_in_folder(*content_type, &linked, current.as_ref());
stash_content_files(&dir, &stash, &ours).await;
stash_content_files(&dir, &stash, *content_type, &ours).await;
sweep_staging_files(&dir).await;
ensure_note(&dir, *content_type).await;
}
@ -1035,7 +1035,12 @@ async fn ensure_links_note(dir: &Path) {
.ok();
}
async fn stash_content_files(dir: &Path, stash: &Path, ours: &HashSet<String>) {
async fn stash_content_files(
dir: &Path,
stash: &Path,
content_type: ContentType,
ours: &HashSet<String>,
) {
let Ok(mut entries) = polyio::read_dir(dir).await else {
return;
};
@ -1067,6 +1072,15 @@ async fn stash_content_files(dir: &Path, stash: &Path, ours: &HashSet<String>) {
continue;
}
if file_type.is_file() && has_content_extension(content_type, &name) {
tracing::debug!(
file = %name,
dir = %dir.display(),
"leaving unrecognised content where it is rather than stashing it"
);
continue;
}
let dest = stash.join(&name);
if let Err(err) = move_entry(&path, &dest).await {
tracing::warn!(
@ -1365,7 +1379,6 @@ mod tests {
assert_eq!(body.lines().count(), 1);
}
use super::*;
fn names(names: &[&str]) -> HashSet<String> {
@ -1403,7 +1416,7 @@ mod tests {
.await
.unwrap();
stash_content_files(&shared, &stash, &ours).await;
stash_content_files(&shared, &stash, ContentType::Shader, &ours).await;
assert!(!shared.join("bsl.zip").exists(), "managed pack left behind");
assert!(!shared.join("bsl.zip.txt").exists(), "sidecar left behind");
assert_eq!(
@ -1438,7 +1451,7 @@ mod tests {
.await
.unwrap();
stash_content_files(&shared, &stash, &HashSet::new()).await;
stash_content_files(&shared, &stash, ContentType::Shader, &HashSet::new()).await;
assert!(!shared.join("Loose").exists());
assert!(stash.join("Loose/shaders/final.fsh").exists());
@ -1446,7 +1459,7 @@ mod tests {
assert!(shared.join("Loose/shaders/final.fsh").exists());
// Only the link goes never the stashed original
stash_content_files(&shared, &stash, &HashSet::new()).await;
stash_content_files(&shared, &stash, ContentType::Shader, &HashSet::new()).await;
assert!(!shared.join("Loose").exists());
assert!(stash.join("Loose/shaders/final.fsh").exists());
@ -1463,7 +1476,7 @@ mod tests {
.await
.unwrap();
stash_content_files(&shared, &stash, &HashSet::new()).await;
stash_content_files(&shared, &stash, ContentType::Mod, &HashSet::new()).await;
assert!(shared.join(EMPTY_NOTE_NAME).exists());
assert!(!stash.join(EMPTY_NOTE_NAME).exists());
@ -1565,7 +1578,6 @@ mod tests {
std::fs::remove_dir_all(root.path()).ok();
}
/// Pinned because the halves are easy to swap `stash` reads the game dir
/// and writes the cluster `restore` the reverse and both take two
/// same-typed `&Path`s a swap would quietly delete a user's files
@ -1596,9 +1608,9 @@ mod tests {
let before = dir_entries(&stash).await;
stash_content_files(&shared, &stash, &ours).await;
stash_content_files(&shared, &stash, ContentType::Mod, &ours).await;
restore_stashed(&stash, &shared, ContentType::Mod, &ours).await;
stash_content_files(&shared, &stash, &ours).await;
stash_content_files(&shared, &stash, ContentType::Mod, &ours).await;
restore_stashed(&stash, &shared, ContentType::Mod, &ours).await;
let after = dir_entries(&stash).await;

View file

@ -0,0 +1,20 @@
-- Repairs the overrides duplicate resolution filed against the user.
--
-- `reconcile_duplicate_activity` switched the older copy of a package off and
-- recorded it as a user disable, but `cluster_bundle_overrides` is keyed by
-- package id and the surviving copy shares it, so the row spoke for a copy
-- nobody turned off. Every lookup that resolves a user's choice then answers
-- "off" for the package: the next bundle update installs the new copy and
-- immediately disables it, and the update check stops shipping the file at all.
--
-- A row alongside a copy that is still on cannot be a considered choice about
-- the package -- had the user turned the package off, nothing of it would be
-- left enabled. Only those go. A package whose copies are all off is
-- indistinguishable from a genuine disable here and is left for the runtime
-- pass, which has the manifest and can tell whether the file was ever shown.
DELETE FROM cluster_bundle_overrides
WHERE override_type = 'disabled'
AND EXISTS (SELECT 1 FROM cluster_artifacts a
WHERE a.cluster_id = cluster_bundle_overrides.cluster_id
AND a.package_id = cluster_bundle_overrides.package_id
AND a.enabled = 1);

View file

@ -189,6 +189,25 @@ pub async fn remove_override(
Ok(())
}
pub async fn clear_disabled_overrides(
pool: &SqlitePool,
cluster_id: i64,
package_id: &str,
) -> Result<u64, sqlx::Error> {
let result = sqlx::query!(
r#"
DELETE FROM cluster_bundle_overrides
WHERE cluster_id = ? AND package_id = ? AND override_type = 'disabled'
"#,
cluster_id,
package_id
)
.execute(pool)
.await?;
Ok(result.rows_affected())
}
/// Opt-in (`enabled`) rows are left alone they say the opposite thing
pub async fn clear_suppressing_overrides(
pool: &SqlitePool,