From 95a519cbe8fa5e8f954e3a3dbd7e92b380d48d0d Mon Sep 17 00:00:00 2001 From: Aiden McClelland Date: Sun, 8 Mar 2026 21:40:55 -0600 Subject: [PATCH] feat: improve service version migration and data version handling Extract get_data_version into a shared function used by both effects and service_map. Use the actual data version (instead of the previous package version) when computing migration targets, and skip migrations when the target range is unsatisfiable. Also detect install vs update based on the presence of a data version file rather than load disposition alone. --- core/src/service/effects/version.rs | 10 +--- core/src/service/mod.rs | 31 +++++++++-- core/src/service/rpc.rs | 6 ++ core/src/service/service_map.rs | 74 ++++++++++++++++--------- sdk/package/lib/version/VersionGraph.ts | 3 + 5 files changed, 87 insertions(+), 37 deletions(-) diff --git a/core/src/service/effects/version.rs b/core/src/service/effects/version.rs index 185e1f629..7b82e060c 100644 --- a/core/src/service/effects/version.rs +++ b/core/src/service/effects/version.rs @@ -2,7 +2,7 @@ use std::path::Path; use crate::DATA_DIR; use crate::service::effects::prelude::*; -use crate::util::io::{delete_file, maybe_read_file_to_string, write_file_atomic}; +use crate::util::io::{delete_file, write_file_atomic}; use crate::volume::PKG_VOLUME_DIR; #[derive(Debug, Clone, Serialize, Deserialize, TS, Parser)] @@ -36,11 +36,5 @@ pub async fn set_data_version( #[instrument(skip_all)] pub async fn get_data_version(context: EffectContext) -> Result, Error> { let context = context.deref()?; - let package_id = &context.seed.id; - let path = Path::new(DATA_DIR) - .join(PKG_VOLUME_DIR) - .join(package_id) - .join("data") - .join(".version"); - maybe_read_file_to_string(path).await + crate::service::get_data_version(&context.seed.id).await } diff --git a/core/src/service/mod.rs b/core/src/service/mod.rs index f17f2d266..d904e77c9 100644 --- a/core/src/service/mod.rs +++ b/core/src/service/mod.rs @@ -46,12 +46,14 @@ use crate::service::uninstall::cleanup; use crate::util::Never; use crate::util::actor::concurrent::ConcurrentActor; use crate::util::future::NonDetachingJoinHandle; -use crate::util::io::{AsyncReadStream, AtomicFile, TermSize, delete_file}; +use crate::util::io::{ + AsyncReadStream, AtomicFile, TermSize, delete_file, maybe_read_file_to_string, +}; use crate::util::net::WebSocket; use crate::util::serde::Pem; use crate::util::sync::SyncMutex; use crate::util::tui::choose; -use crate::volume::data_dir; +use crate::volume::{PKG_VOLUME_DIR, data_dir}; use crate::{ActionId, CAP_1_KiB, DATA_DIR, ImageId, PackageId}; pub mod action; @@ -81,6 +83,17 @@ pub enum LoadDisposition { Undo, } +/// Read the data version file for a service from disk. +/// Returns `Ok(None)` if the file does not exist (fresh install). +pub async fn get_data_version(id: &PackageId) -> Result, Error> { + let path = Path::new(DATA_DIR) + .join(PKG_VOLUME_DIR) + .join(id) + .join("data") + .join(".version"); + maybe_read_file_to_string(&path).await +} + struct RootCommand(pub String); #[derive(Clone, Debug, Serialize, Deserialize, Default, TS)] @@ -390,12 +403,17 @@ impl Service { tracing::error!("Error opening s9pk for install: {e}"); tracing::debug!("{e:?}") }) { + let init_kind = if get_data_version(id).await.ok().flatten().is_some() { + InitKind::Update + } else { + InitKind::Install + }; if let Ok(service) = Self::install( ctx.clone(), s9pk, &s9pk_path, &None, - InitKind::Install, + init_kind, None::, None, ) @@ -424,12 +442,17 @@ impl Service { tracing::error!("Error opening s9pk for update: {e}"); tracing::debug!("{e:?}") }) { + let init_kind = if get_data_version(id).await.ok().flatten().is_some() { + InitKind::Update + } else { + InitKind::Install + }; if let Ok(service) = Self::install( ctx.clone(), s9pk, &s9pk_path, &None, - InitKind::Update, + init_kind, None::, None, ) diff --git a/core/src/service/rpc.rs b/core/src/service/rpc.rs index b5c8ed01c..94c15f42f 100644 --- a/core/src/service/rpc.rs +++ b/core/src/service/rpc.rs @@ -107,6 +107,12 @@ impl ExitParams { target: Some(InternedString::from_display(range)), } } + pub fn target_str(s: &str) -> Self { + Self { + id: Guid::new(), + target: Some(InternedString::intern(s)), + } + } pub fn uninstall() -> Self { Self { id: Guid::new(), diff --git a/core/src/service/service_map.rs b/core/src/service/service_map.rs index 697578a7c..80a6ea5d4 100644 --- a/core/src/service/service_map.rs +++ b/core/src/service/service_map.rs @@ -28,7 +28,7 @@ use crate::s9pk::S9pk; use crate::s9pk::manifest::PackageId; use crate::s9pk::merkle_archive::source::FileSource; use crate::service::rpc::{ExitParams, InitKind}; -use crate::service::{LoadDisposition, Service, ServiceRef}; +use crate::service::{LoadDisposition, Service, ServiceRef, get_data_version}; use crate::sign::commitment::merkle_archive::MerkleArchiveCommitment; use crate::status::{DesiredStatus, StatusInfo}; use crate::util::future::NonDetachingJoinHandle; @@ -310,36 +310,60 @@ impl ServiceMap { .handle_last(async move { finalization_progress.start(); let s9pk = S9pk::open(&installed_path, Some(&id)).await?; + let data_version = get_data_version(&id).await?; let prev = if let Some(service) = service.take() { ensure_code!( recovery_source.is_none(), ErrorKind::InvalidRequest, "cannot restore over existing package" ); - let prev_version = service - .seed - .persistent_container - .s9pk - .as_manifest() - .version - .clone(); - let prev_can_migrate_to = &service - .seed - .persistent_container - .s9pk - .as_manifest() - .can_migrate_to; - let next_version = &s9pk.as_manifest().version; - let next_can_migrate_from = &s9pk.as_manifest().can_migrate_from; - let uninit = if prev_version.satisfies(next_can_migrate_from) { - ExitParams::target_version(&*prev_version) - } else if next_version.satisfies(prev_can_migrate_to) { - ExitParams::target_version(&s9pk.as_manifest().version) + let uninit = if let Some(ref data_ver) = data_version { + let prev_can_migrate_to = &service + .seed + .persistent_container + .s9pk + .as_manifest() + .can_migrate_to; + let next_version = &s9pk.as_manifest().version; + let next_can_migrate_from = + &s9pk.as_manifest().can_migrate_from; + if let Ok(data_ver_ev) = + data_ver.parse::() + { + if data_ver_ev.satisfies(next_can_migrate_from) { + ExitParams::target_str(data_ver) + } else if next_version.satisfies(prev_can_migrate_to) { + ExitParams::target_version(&s9pk.as_manifest().version) + } else { + ExitParams::target_range(&VersionRange::and( + prev_can_migrate_to.clone(), + next_can_migrate_from.clone(), + )) + } + } else if let Ok(data_ver_range) = + data_ver.parse::() + { + ExitParams::target_range(&VersionRange::and( + data_ver_range, + next_can_migrate_from.clone(), + )) + } else if next_version.satisfies(prev_can_migrate_to) { + ExitParams::target_version(&s9pk.as_manifest().version) + } else { + ExitParams::target_range(&VersionRange::and( + prev_can_migrate_to.clone(), + next_can_migrate_from.clone(), + )) + } } else { - ExitParams::target_range(&VersionRange::and( - prev_can_migrate_to.clone(), - next_can_migrate_from.clone(), - )) + ExitParams::target_version( + &*service + .seed + .persistent_container + .s9pk + .as_manifest() + .version, + ) }; let cleanup = service.uninstall(uninit, false, false).await?; progress.complete(); @@ -354,7 +378,7 @@ impl ServiceMap { ®istry, if recovery_source.is_some() { InitKind::Restore - } else if prev.is_some() { + } else if data_version.is_some() { InitKind::Update } else { InitKind::Install diff --git a/sdk/package/lib/version/VersionGraph.ts b/sdk/package/lib/version/VersionGraph.ts index 84d24269e..2b82c67c3 100644 --- a/sdk/package/lib/version/VersionGraph.ts +++ b/sdk/package/lib/version/VersionGraph.ts @@ -331,6 +331,9 @@ export class VersionGraph target: VersionRange | ExtendedVersion | null, ): Promise { if (target) { + if (isRange(target) && !target.satisfiable()) { + return + } const from = await getDataVersion(effects) if (from) { target = await this.migrate({