diff --git a/apps/oneclient/desktop/src/api/commands.rs b/apps/oneclient/desktop/src/api/commands.rs index f1ae40fc..9f6cdb1f 100644 --- a/apps/oneclient/desktop/src/api/commands.rs +++ b/apps/oneclient/desktop/src/api/commands.rs @@ -251,6 +251,12 @@ impl OneClientApi for OneClientApiImpl { self, cluster_id: ClusterId, ) -> LauncherResult { + // A manual update check must re-fetch the manifest and re-validate bundles rather than + // serving the process-cached copy, otherwise updates published after launch are missed. + crate::oneclient::bundles::BundlesManager::get() + .await + .refresh() + .await; crate::oneclient::bundle_updates::apply_bundle_updates(cluster_id).await } diff --git a/apps/oneclient/desktop/src/oneclient/bundle_updates.rs b/apps/oneclient/desktop/src/oneclient/bundle_updates.rs index a5f1f104..95cfbb39 100644 --- a/apps/oneclient/desktop/src/oneclient/bundle_updates.rs +++ b/apps/oneclient/desktop/src/oneclient/bundle_updates.rs @@ -1370,82 +1370,16 @@ pub async fn apply_bundle_updates( } } - // Re-extract overrides from any bundle that had changes applied. - // This keeps config files, resource packs, and other overridden assets in sync. - let has_changes = !updates_applied.is_empty() - || !removals_applied.is_empty() - || !additions_applied.is_empty(); - - if has_changes { - let mut affected_bundles: std::collections::HashSet = - std::collections::HashSet::new(); - - for u in &updates_applied { - affected_bundles.insert(u.bundle_name.clone()); - } - for r in &removals_applied { - affected_bundles.insert(r.bundle_name.clone()); - } - for a in &additions_applied { - affected_bundles.insert(a.bundle_name.clone()); - } - - if !affected_bundles.is_empty() { - match api::cluster::dao::get_cluster_by_id(cluster_id).await { - Ok(Some(cluster)) => { - match BundlesManager::get() - .await - .get_bundles_for(&cluster.mc_version, cluster.mc_loader) - .await - { - Ok(bundles) => { - for bundle in &bundles { - if affected_bundles.contains(&bundle.manifest.name) { - tracing::info!( - bundle_name = %bundle.manifest.name, - cluster_id = %cluster_id, - "Re-extracting overrides from updated bundle" - ); - if let Err(e) = - onelauncher_core::api::packages::modpack::mrpack::copy_overrides_folder_no_overwrite( - &cluster, - &bundle.path, - ) - .await - { - send_error!( - "Failed to extract overrides from bundle '{}': {}", - bundle.manifest.name, - e - ); - } - } - } - } - Err(e) => { - tracing::error!( - cluster_id = %cluster_id, - "Failed to retrieve bundles for override extraction after update: {}", - e - ); - } - } - } - Ok(None) => { - tracing::error!( - cluster_id = %cluster_id, - "Cluster not found during post-update override extraction" - ); - } - Err(e) => { - tracing::error!( - cluster_id = %cluster_id, - "Failed to retrieve cluster for override extraction after update: {}", - e - ); - } - } - } + // Always re-extract overrides for every subscribed bundle, regardless of whether any + // package was added/updated/removed this run. + // + // Overrides (config files, compatibility JSONs, resource packs, etc.) can change in a + // bundle WITHOUT any package version changing — e.g. publishing a JSON that makes an + // already-installed mod compatible with a new MC version. Gating extraction on + // package-level changes (the previous behaviour) silently skipped these override-only + // updates, so users who already had the mods installed never received the new override. + if let Err(e) = extract_overrides_for_subscribed_bundles(cluster_id).await { + send_error!("Failed to extract bundle overrides for cluster {cluster_id}: {e}"); } let package_operation_failed = @@ -1470,6 +1404,65 @@ pub async fn apply_bundle_updates( }) } +/// Re-extracts override files (configs, compatibility JSONs, resource packs, etc.) for +/// every bundle the cluster is subscribed to. +/// +/// This runs on every bundle sync — overrides can change independently of package +/// versions, so it must NOT be gated behind package-level changes. Existing on-disk files +/// are preserved (see [`copy_overrides_folder_no_overwrite`]) so user customizations and +/// already-correct files are not clobbered; only missing override files are written. +async fn extract_overrides_for_subscribed_bundles(cluster_id: ClusterId) -> LauncherResult<()> { + let Some(cluster) = api::cluster::dao::get_cluster_by_id(cluster_id).await? else { + tracing::warn!( + cluster_id = %cluster_id, + "Skipping override extraction because cluster no longer exists" + ); + return Ok(()); + }; + + let bundle_packages = bundle_dao::get_bundle_packages_for_cluster(cluster_id).await?; + let all_linked_packages = api::packages::dao::get_linked_packages(&cluster).await?; + let bundles = BundlesManager::get() + .await + .get_bundles_for(&cluster.mc_version, cluster.mc_loader) + .await?; + + let all_installed_managed_keys = collect_all_installed_managed_keys(&all_linked_packages); + let subscribed_bundles = infer_subscribed_bundles_for_loader_sync( + &bundle_packages, + &bundles, + &all_installed_managed_keys, + ); + + for bundle in &bundles { + if !subscribed_bundles.contains(&bundle.manifest.name) { + continue; + } + + tracing::debug!( + bundle_name = %bundle.manifest.name, + cluster_id = %cluster_id, + "Re-extracting overrides for subscribed bundle" + ); + + if let Err(e) = + onelauncher_core::api::packages::modpack::mrpack::copy_overrides_folder_no_overwrite( + &cluster, + &bundle.path, + ) + .await + { + send_error!( + "Failed to extract overrides from bundle '{}': {}", + bundle.manifest.name, + e + ); + } + } + + Ok(()) +} + /// Migrates non-bundle (custom) packages from `source_cluster_id` to `target_cluster_id`. /// /// For packages tracked on Modrinth or `CurseForge`, attempts to find and install a version diff --git a/apps/oneclient/desktop/src/oneclient/bundle_updates_test.rs b/apps/oneclient/desktop/src/oneclient/bundle_updates_test.rs index 93fe2f16..150d06b8 100644 --- a/apps/oneclient/desktop/src/oneclient/bundle_updates_test.rs +++ b/apps/oneclient/desktop/src/oneclient/bundle_updates_test.rs @@ -424,4 +424,161 @@ mod tests { let _ = dao::delete_cluster_by_id(cluster.id).await; let _ = fs::remove_dir_all(cluster.path().await.unwrap()).await; } + + /// Regression test for the bundle-override bug: override files (config JSONs, etc.) must + /// be re-extracted on a sync EVEN WHEN no package was added/updated/removed. + /// + /// Previously override extraction was gated behind package-level changes, so an + /// override-only update (e.g. a JSON making an already-installed mod compatible with a + /// new MC version) was silently skipped for users who already had the mods installed. + #[tokio::test] + #[ignore = "touches true files and takes long"] + #[allow(clippy::too_many_lines, clippy::items_after_statements)] + async fn test_overrides_reextracted_without_package_changes() { + use sea_orm::{ActiveModelTrait, Set}; + + Core::initialize(CoreOptions::default()).await.unwrap(); + + let cluster_name = format!( + "Bundle Override-Only Test {}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + ); + let cluster = create_cluster(&cluster_name, "1.21.1", GameLoader::Fabric, None, None) + .await + .unwrap(); + let cluster_dir = cluster.path().await.unwrap(); + + let bundles = BundlesManager::get() + .await + .get_bundles_for(&cluster.mc_version, cluster.mc_loader) + .await + .unwrap(); + let target_bundle = bundles + .into_iter() + .find(|b| b.manifest.name.contains("Performance")) + .expect("Expected to find a Performance bundle"); + + // Find an override file shipped in the bundle archive so we have something to assert on. + let override_rel_path = first_override_entry(&target_bundle.path) + .await + .expect("bundle has no override files to test against; cannot run this regression"); + + // Subscribe the cluster to the bundle. + let dummy_hash = uuid::Uuid::new_v4().to_string(); + onelauncher_core::entity::packages::ActiveModel { + hash: Set(dummy_hash.clone()), + file_name: Set("dummy.jar".to_string()), + version_id: Set("dummy_version".to_string()), + display_name: Set("Dummy".to_string()), + display_version: Set("1.0.0".to_string()), + package_type: Set(onelauncher_core::entity::package::PackageType::Mod), + provider: Set(onelauncher_core::entity::package::Provider::Modrinth), + package_id: Set("dummy_id".to_string()), + mc_versions: Set(onelauncher_core::entity::utility::DbVec(vec![ + "1.21.1".to_string(), + ])), + mc_loader: Set(onelauncher_core::entity::utility::DbVec(vec![ + GameLoader::Fabric, + ])), + published_at: Set(chrono::Utc::now()), + ..Default::default() + } + .insert(&onelauncher_core::store::State::get().await.unwrap().db) + .await + .unwrap(); + onelauncher_core::entity::cluster_packages::ActiveModel { + cluster_id: Set(cluster.id), + package_hash: Set(dummy_hash.clone()), + bundle_name: Set(Some(target_bundle.manifest.name.clone())), + ..Default::default() + } + .insert(&onelauncher_core::store::State::get().await.unwrap().db) + .await + .unwrap(); + + // First apply: installs packages AND extracts overrides (has_changes == true). + let applied1 = apply_bundle_updates(cluster.id).await.unwrap(); + assert!( + !applied1.additions_applied.is_empty(), + "Expected initial additions from bundle" + ); + + let extracted = cluster_dir.join(&override_rel_path); + assert!( + extracted.exists(), + "override file should be extracted on first apply" + ); + + // Simulate the override file being missing (e.g. it was only just added to the bundle, + // so existing users never had it). Delete it from disk. + fs::remove_file(&extracted).await.unwrap(); + assert!(!extracted.exists()); + + // Second apply: nothing about the packages changed, so has_changes == false. + // The override file MUST still be re-extracted (this is the bug being regression-tested). + let applied2 = apply_bundle_updates(cluster.id).await.unwrap(); + assert!( + applied2.updates_applied.is_empty() + && applied2.additions_applied.is_empty() + && applied2.removals_applied.is_empty(), + "second apply should have no package-level changes" + ); + assert!( + extracted.exists(), + "override file must be re-extracted even when no package changed (Bug 2 regression)" + ); + + // Clean up + let _ = onelauncher_core::api::packages::dao::delete_package_by_id(dummy_hash).await; + let _ = dao::delete_cluster_by_id(cluster.id).await; + let _ = fs::remove_dir_all(cluster_dir).await; + } + + /// Returns the first file path under `overrides/` in a `.mrpack`/zip archive, relative to + /// the cluster root (i.e. with the `overrides/` prefix stripped). + /// + /// Extracts the archive's overrides into a throwaway temp dir using the same public helper + /// the real sync uses, then walks it — avoiding a direct zip-crate dependency in this test. + async fn first_override_entry(archive_path: &std::path::Path) -> Option { + let probe_dir = std::env::temp_dir().join(format!( + "oneclient_override_probe_{}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + fs::create_dir_all(&probe_dir).await.ok()?; + + onelauncher_core::api::packages::modpack::mrpack::extract_overrides_no_overwrite( + &archive_path.to_path_buf(), + &probe_dir, + ) + .await + .ok()?; + + let relative = first_file_relative(&probe_dir, &probe_dir); + let _ = fs::remove_dir_all(&probe_dir).await; + relative + } + + /// Recursively finds the first regular file under `dir`, returning its path relative to + /// `root` with forward slashes. + fn first_file_relative(dir: &std::path::Path, root: &std::path::Path) -> Option { + let mut entries: Vec<_> = std::fs::read_dir(dir).ok()?.flatten().collect(); + entries.sort_by_key(std::fs::DirEntry::path); + for entry in entries { + let path = entry.path(); + if path.is_dir() { + if let Some(found) = first_file_relative(&path, root) { + return Some(found); + } + } else if let Ok(rel) = path.strip_prefix(root) { + return Some(rel.to_string_lossy().replace('\\', "/")); + } + } + None + } } diff --git a/apps/oneclient/desktop/src/oneclient/bundles.rs b/apps/oneclient/desktop/src/oneclient/bundles.rs index c0226ab5..d8cb5321 100644 --- a/apps/oneclient/desktop/src/oneclient/bundles.rs +++ b/apps/oneclient/desktop/src/oneclient/bundles.rs @@ -128,6 +128,22 @@ impl BundlesManager { Ok(found.clone()) } + /// Re-fetches the bundles manifest from remote and clears the in-memory parsed-bundle + /// cache so the next [`get_bundles_for`](Self::get_bundles_for) call reloads everything. + /// + /// The on-disk `.mrpack` cache is still consulted (and validated via ETag) by + /// `download_and_load_bundle`, so this only forces re-validation, not blind re-downloads. + /// + /// Without this, the manifest is fetched exactly once per process at startup and the + /// parsed bundles are cached for the process lifetime — meaning a long-running session + /// (or a manual "check for updates") would never observe a bundle the publisher updated + /// after launch. Mods would silently stop updating until a full restart. + pub async fn refresh(&self) { + let manifest = Self::fetch_cached().await; + *self.manifest.write().await = manifest; + self.bundles.write().await.clear(); + } + /// Fetches the bundles manifest from remote, falling back to a saved copy on disk if available #[tracing::instrument] pub async fn fetch_cached() -> BundleManifest { diff --git a/apps/oneclient/desktop/src/oneclient/mod.rs b/apps/oneclient/desktop/src/oneclient/mod.rs index 60382954..d33f4f70 100644 --- a/apps/oneclient/desktop/src/oneclient/mod.rs +++ b/apps/oneclient/desktop/src/oneclient/mod.rs @@ -35,6 +35,11 @@ async fn check_and_apply_all_bundle_updates() { BUNDLE_SYNCING.store(true, Ordering::Relaxed); tracing::info!("checking for bundle updates..."); + // Force a fresh manifest fetch + cache clear so we observe bundles the publisher updated + // since this process started (or since the last sync). The on-disk .mrpack cache is still + // ETag-validated, so unchanged bundles are not re-downloaded. + bundles::BundlesManager::get().await.refresh().await; + let clusters = match get_all_clusters().await { Ok(clusters) => clusters, Err(err) => { diff --git a/apps/oneclient/frontend/src/utils/versionMap.ts b/apps/oneclient/frontend/src/utils/versionMap.ts index 1cff5e5a..497df0fc 100644 --- a/apps/oneclient/frontend/src/utils/versionMap.ts +++ b/apps/oneclient/frontend/src/utils/versionMap.ts @@ -75,6 +75,28 @@ export function getOnlineEntryForVersion( export interface ParsedMcVersion { major: number; minor: number | undefined; + /** + * Patch component of the new (year-based) scheme, e.g. the `2` in `26.1.2`. + * `undefined` for two-part new versions (`26.1`) and for all old `1.X.Y` versions + * (whose third component is the `minor`, not a patch). + */ + patch?: number | undefined; +} + +/** + * The major version at and above which Minecraft uses the new year-based scheme + * (`YY.N[.P]`, e.g. `26.1` / `26.1.2`). Below it, the legacy `1.X[.Y]` scheme is used. + * + * Kept as a single named constant so every formatter/parser agrees on the boundary — + * an inconsistent boundary is how malformed strings like `1.26` get produced. + */ +export const NEW_SCHEME_MAJOR = 26; + +function toNumberOrUndefined(value: string | undefined): number | undefined { + if (value === undefined) + return undefined; + const parsed = Number.parseInt(value, 10); + return Number.isNaN(parsed) ? undefined : parsed; } export function parseMcVersion(version: string | null | undefined): ParsedMcVersion | undefined { @@ -83,15 +105,19 @@ export function parseMcVersion(version: string | null | undefined): ParsedMcVers const parts = version.split('.'); - // New format: YY.N[.P] (e.g. "26.1" or "26.1.1") — no "1." prefix + // New format: YY.N[.P] (e.g. "26.1", "26.1.1", "26.1.2") — no "1." prefix. if (parts[0] !== '1') { if (parts.length < 2) return undefined; - const major = Number.parseInt(parts[0], 10); - const minor = Number.parseInt(parts[1], 10); + const major = toNumberOrUndefined(parts[0]); + if (major === undefined) + return undefined; return { major, - minor: Number.isNaN(minor) ? undefined : minor, + minor: toNumberOrUndefined(parts[1]), + // Preserve the patch component instead of silently dropping it, so a + // "26.1.2" cluster is parsed in full rather than collapsing to "26.1". + patch: toNumberOrUndefined(parts[2]), }; } @@ -99,21 +125,28 @@ export function parseMcVersion(version: string | null | undefined): ParsedMcVers if (parts.length <= 1) return undefined; - const major = Number.parseInt(parts[1], 10); - const minor = parts.length > 2 ? Number.parseInt(parts[2], 10) : undefined; + const major = toNumberOrUndefined(parts[1]); + if (major === undefined) + return undefined; return { major, - minor: Number.isNaN(minor) ? undefined : minor, + minor: toNumberOrUndefined(parts[2]), + patch: undefined, }; } /** - * Constructs a Minecraft version string from a major+minor pair. - * Versions from 2026 onward use the new YY.N format; older versions use 1.X.Y. + * Constructs a Minecraft version string from a major+minor[+patch] tuple. + * Versions from {@link NEW_SCHEME_MAJOR} onward use the new `YY.N[.P]` format; + * older versions use `1.X.Y`. + * + * The `major >= NEW_SCHEME_MAJOR` check MUST stay in sync with {@link parseMcVersion}'s + * `parts[0] !== '1'` rule: a `major` of 26 must never be formatted with a `1.` prefix + * (which would yield bogus strings like `1.26`). */ -export function formatMcVersion(major: number, minor: number): string { - if (major >= 26) - return `${major}.${minor}`; +export function formatMcVersion(major: number, minor: number, patch?: number): string { + if (major >= NEW_SCHEME_MAJOR) + return patch === undefined ? `${major}.${minor}` : `${major}.${minor}.${patch}`; return `1.${major}.${minor}`; } diff --git a/apps/oneclient/frontend/src/utils/versionMap.verify.mts b/apps/oneclient/frontend/src/utils/versionMap.verify.mts new file mode 100644 index 00000000..93a0c27c --- /dev/null +++ b/apps/oneclient/frontend/src/utils/versionMap.verify.mts @@ -0,0 +1,53 @@ +/* eslint-disable no-console, node/prefer-global/process -- standalone dev verification script, not app code */ +/** + * Standalone verification for {@link ./versionMap.ts} parsing/formatting. + * + * No test runner is wired into this app, so this is a runnable assertion script: + * npx tsx src/utils/versionMap.verify.mts + * + * Confirms (Bug 3): + * 1. OneClient parses the new year-based version "26.1.2" (and "26.1") correctly. + * 2. No code path emits the malformed "1.26" string for a 2026-era version. + */ +import { formatMcVersion, parseMcVersion } from './versionMap.ts'; + +let failures = 0; +function check(label: string, actual: unknown, expected: unknown): void { + const a = JSON.stringify(actual); + const e = JSON.stringify(expected); + if (a === e) { + console.log(` ok ${label}`); + } + else { + failures++; + console.error(` FAIL ${label}\n expected ${e}\n actual ${a}`); + } +} + +console.log('parseMcVersion:'); +check('26.1.2 -> full parse with patch', parseMcVersion('26.1.2'), { major: 26, minor: 1, patch: 2 }); +check('26.1 -> no patch', parseMcVersion('26.1'), { major: 26, minor: 1, patch: undefined }); +check('26.1.1 -> patch 1', parseMcVersion('26.1.1'), { major: 26, minor: 1, patch: 1 }); +check('1.21.5 -> old scheme', parseMcVersion('1.21.5'), { major: 21, minor: 5, patch: undefined }); +check('1.21.1 -> old scheme', parseMcVersion('1.21.1'), { major: 21, minor: 1, patch: undefined }); +check('null -> undefined', parseMcVersion(null), undefined); +check('"26" -> undefined (no minor)', parseMcVersion('26'), undefined); + +console.log('formatMcVersion:'); +check('(26,1,2) -> 26.1.2', formatMcVersion(26, 1, 2), '26.1.2'); +check('(26,1) -> 26.1', formatMcVersion(26, 1), '26.1'); +check('(21,5) -> 1.21.5', formatMcVersion(21, 5), '1.21.5'); + +console.log('round-trip (must never produce "1.26"):'); +for (const v of ['26.1', '26.1.2', '26.1.1']) { + const p = parseMcVersion(v)!; + const out = formatMcVersion(p.major, p.minor!, p.patch); + check(`${v} round-trips to ${out}`, out, v); + check(`${v} never yields a "1." prefix`, out.startsWith('1.'), false); +} + +if (failures > 0) { + console.error(`\n${failures} check(s) FAILED`); + process.exit(1); +} +console.log('\nAll version parsing checks passed.');