Skip to content

Commit

Permalink
fix: add separate storage dir for state (#84)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bryanoltman
bryanoltman authored Sep 12, 2023
1 parent 84cfcf5 commit b4024cf
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 41 deletions.
10 changes: 8 additions & 2 deletions library/include/updater.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,15 @@ typedef struct AppParameters {
*/
int original_libapp_paths_size;
/**
* Path to cache_dir where the updater will store downloaded artifacts.
* Path to app storage directory where the updater will store serialized
* state and other data that persists between releases.
*/
const char *cache_dir;
const char *app_storage_dir;
/**
* Path to cache directory where the updater will store downloaded
* artifacts and data that can be deleted when a new release is detected.
*/
const char *code_cache_dir;
} AppParameters;

#ifdef __cplusplus
Expand Down
21 changes: 15 additions & 6 deletions library/src/c_api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,13 @@ pub struct AppParameters {
/// Length of the original_libapp_paths array.
pub original_libapp_paths_size: libc::c_int,

/// Path to cache_dir where the updater will store downloaded artifacts.
pub cache_dir: *const libc::c_char,
/// Path to app storage directory where the updater will store serialized
/// state and other data that persists between releases.
pub app_storage_dir: *const libc::c_char,

/// Path to cache directory where the updater will store downloaded
/// artifacts and data that can be deleted when a new release is detected.
pub code_cache_dir: *const libc::c_char,
}

/// Converts a C string to a Rust string, does not free the C string.
Expand Down Expand Up @@ -74,7 +79,8 @@ fn app_config_from_c(c_params: *const AppParameters) -> anyhow::Result<updater::
let c_params_ref = unsafe { &*c_params };

Ok(updater::AppConfig {
cache_dir: to_rust(c_params_ref.cache_dir)?,
app_storage_dir: to_rust(c_params_ref.app_storage_dir)?,
code_cache_dir: to_rust(c_params_ref.code_cache_dir)?,
release_version: to_rust(c_params_ref.release_version)?,
original_libapp_paths: to_rust_vector(
c_params_ref.original_libapp_paths,
Expand Down Expand Up @@ -298,15 +304,17 @@ mod test {
let app_paths = c_array(app_paths_vec);

super::AppParameters {
cache_dir: c_string(&cache_dir),
app_storage_dir: c_string(&cache_dir),
code_cache_dir: c_string(&cache_dir),
release_version: c_string("1.0.0"),
original_libapp_paths: app_paths as *const *const libc::c_char,
original_libapp_paths_size: app_paths_size,
}
}

fn free_parameters(params: super::AppParameters) {
free_c_string(params.cache_dir as *mut libc::c_char);
free_c_string(params.app_storage_dir as *mut libc::c_char);
free_c_string(params.code_cache_dir as *mut libc::c_char);
free_c_string(params.release_version as *mut libc::c_char);
free_c_array(
params.original_libapp_paths as *mut *mut libc::c_char,
Expand All @@ -331,7 +339,8 @@ mod test {
testing_reset_config();
// Should log but not crash.
let c_params = AppParameters {
cache_dir: std::ptr::null(),
app_storage_dir: std::ptr::null(),
code_cache_dir: std::ptr::null(),
release_version: std::ptr::null(),
original_libapp_paths: std::ptr::null(),
original_libapp_paths_size: 0,
Expand Down
26 changes: 19 additions & 7 deletions library/src/cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -162,17 +162,21 @@ impl UpdaterState {
}

fn create_new_and_save(
cache_dir: &Path,
storage_dir: &Path,
release_version: &str,
client_id: Option<String>,
) -> Self {
let state = Self::new(cache_dir.to_owned(), release_version.to_owned(), client_id);
let state = Self::new(
storage_dir.to_owned(),
release_version.to_owned(),
client_id,
);
let _ = state.save();
state
}

pub fn load_or_new_on_error(cache_dir: &Path, release_version: &str) -> Self {
let load_result = Self::load(cache_dir);
pub fn load_or_new_on_error(storage_dir: &Path, release_version: &str) -> Self {
let load_result = Self::load(storage_dir);
match load_result {
Ok(mut loaded) => {
let maybe_client_id = loaded.client_id.clone();
Expand All @@ -181,20 +185,28 @@ impl UpdaterState {
"release_version changed {} -> {}, clearing updater state",
loaded.release_version, release_version
);
return Self::create_new_and_save(cache_dir, release_version, maybe_client_id);
return Self::create_new_and_save(
storage_dir,
release_version,
maybe_client_id,
);
}
let validate_result = loaded.validate();
if let Err(e) = validate_result {
warn!("Error while validating state: {:#}, clearing state.", e);
return Self::create_new_and_save(cache_dir, release_version, maybe_client_id);
return Self::create_new_and_save(
storage_dir,
release_version,
maybe_client_id,
);
}
loaded
}
Err(e) => {
if !is_file_not_found(&e) {
warn!("Error loading state: {:#}, clearing state.", e);
}
Self::create_new_and_save(cache_dir, release_version, None)
Self::create_new_and_save(storage_dir, release_version, None)
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions library/src/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ where
// The config passed into init. This is immutable once set and copyable.
#[derive(Debug, Clone)]
pub struct UpdateConfig {
pub cache_dir: PathBuf,
pub storage_dir: PathBuf,
pub download_dir: PathBuf,
pub auto_update: bool,
pub channel: String,
Expand All @@ -91,12 +91,12 @@ pub fn set_config(
with_config_mut(|config| {
anyhow::ensure!(config.is_none(), "shorebird_init has already been called.");

let mut cache_path = std::path::PathBuf::from(&app_config.cache_dir);
cache_path.push("downloads");
let download_dir = cache_path;
let mut code_cache_path = std::path::PathBuf::from(&app_config.code_cache_dir);
code_cache_path.push("downloads");
let download_dir = code_cache_path;

let new_config = UpdateConfig {
cache_dir: std::path::PathBuf::from(app_config.cache_dir),
storage_dir: std::path::PathBuf::from(app_config.app_storage_dir),
download_dir,
channel: yaml
.channel
Expand Down
48 changes: 27 additions & 21 deletions library/src/updater.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@ impl Display for UpdateError {
// However rusty api would probably used `&str` instead of `String`,
// but making `&str` from `CStr*` is a bit of a pain.
pub struct AppConfig {
pub cache_dir: String,
pub app_storage_dir: String,
pub code_cache_dir: String,
pub release_version: String,
pub original_libapp_paths: Vec<String>,
}
Expand Down Expand Up @@ -124,7 +125,8 @@ fn check_for_update_internal() -> anyhow::Result<PatchCheckResponse> {
with_config(|config| {
// Load UpdaterState from disk
// If there is no state, make an empty state.
let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
let state =
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
send_patch_check_request(config, &state)
})
}
Expand Down Expand Up @@ -224,7 +226,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
// racing with us, we should get a new state inside a lock if we want
// to write.
let read_only_state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);

// We discard any events if we have more than 3 queued to make sure
// we don't stall the client.
Expand All @@ -238,7 +240,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
// We're abusing the config lock as a UpdateState lock for now.
let read_only_state = with_config(|_| {
let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
// This will clear any events which got queued between the time we
// loaded the state now, but that's OK for now.
let result = state.clear_events();
Expand Down Expand Up @@ -282,7 +284,7 @@ fn update_internal(_: &UpdaterLockState) -> anyhow::Result<UpdateStatus> {
number: patch.number,
};
let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
// Move/state update should be "atomic" (it isn't today).
state.install_patch(&patch_info)?;
info!("Patch {} successfully installed.", patch.number);
Expand Down Expand Up @@ -350,7 +352,8 @@ where
/// This may be changed any time `update()` or `start_update_thread()` are called.
pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
with_config(|config| {
let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
let state =
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
Ok(state.next_boot_patch())
})
}
Expand All @@ -360,15 +363,16 @@ pub fn next_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
/// `next_boot_patch`.
pub fn current_boot_patch() -> anyhow::Result<Option<PatchInfo>> {
with_config(|config| {
let state = UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
let state =
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
Ok(state.current_boot_patch())
})
}

pub fn report_launch_start() -> anyhow::Result<()> {
with_config(|config| {
let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
// Validate that we have an installed patch.
// Make that patch the "booted" patch.
state.activate_current_patch()?;
Expand All @@ -382,7 +386,7 @@ pub fn report_launch_failure() -> anyhow::Result<()> {
info!("Reporting failed launch.");
with_config(|config| {
let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);

let patch =
state
Expand Down Expand Up @@ -419,7 +423,7 @@ pub fn report_launch_failure() -> anyhow::Result<()> {
pub fn report_launch_success() -> anyhow::Result<()> {
with_config(|config| {
let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);

if let Some(patch) = state.current_boot_patch() {
if !state.is_known_good_patch(patch.number) {
Expand Down Expand Up @@ -485,7 +489,8 @@ mod tests {
let cache_dir = tmp_dir.path().to_str().unwrap().to_string();
crate::init(
crate::AppConfig {
cache_dir: cache_dir.clone(),
app_storage_dir: cache_dir.clone(),
code_cache_dir: cache_dir.clone(),
release_version: "1.0.0+1".to_string(),
original_libapp_paths: vec!["/dir/lib/arch/libapp.so".to_string()],
},
Expand All @@ -511,7 +516,7 @@ mod tests {
fs::write(&artifact_path, "hello").unwrap();

let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
state
.install_patch(&PatchInfo {
path: artifact_path,
Expand Down Expand Up @@ -580,7 +585,8 @@ mod tests {
assert_eq!(
crate::init(
crate::AppConfig {
cache_dir: cache_dir.clone(),
app_storage_dir: cache_dir.clone(),
code_cache_dir: cache_dir.clone(),
release_version: "1.0.0+1".to_string(),
original_libapp_paths: vec!["original_libapp_path".to_string()],
},
Expand Down Expand Up @@ -624,7 +630,7 @@ mod tests {
fs::write(&artifact_path, "hello").unwrap();

let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
state
.install_patch(&PatchInfo {
path: artifact_path,
Expand All @@ -642,7 +648,7 @@ mod tests {
let next_boot_patch = crate::next_boot_patch().unwrap().unwrap();
with_config(|config| {
let state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
assert!(!state.is_known_good_patch(next_boot_patch.number));
Ok(())
})
Expand All @@ -652,7 +658,7 @@ mod tests {

with_config(|config| {
let state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
assert!(state.is_known_good_patch(next_boot_patch.number));
Ok(())
})
Expand All @@ -675,7 +681,7 @@ mod tests {
fs::write(&artifact_path, "hello").unwrap();

let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
state
.install_patch(&PatchInfo {
path: artifact_path,
Expand All @@ -693,7 +699,7 @@ mod tests {
let next_boot_patch = crate::next_boot_patch().unwrap().unwrap();
with_config(|config| {
let state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
// It's not bad yet.
assert!(!state.is_known_bad_patch(next_boot_patch.number));
Ok(())
Expand All @@ -704,7 +710,7 @@ mod tests {

with_config(|config| {
let state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
// It's now bad.
assert!(state.is_known_bad_patch(next_boot_patch.number));
// And we've queued an event.
Expand All @@ -731,7 +737,7 @@ mod tests {

with_config(|config| {
let mut state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
let fail_event = PatchEvent {
app_id: config.app_id.clone(),
arch: current_arch().to_string(),
Expand Down Expand Up @@ -779,7 +785,7 @@ mod tests {

with_config(|config| {
let state =
UpdaterState::load_or_new_on_error(&config.cache_dir, &config.release_version);
UpdaterState::load_or_new_on_error(&config.storage_dir, &config.release_version);
// All 5 events should be cleared, even though only 3 were sent.
assert_eq!(state.copy_events(10).len(), 0);
Ok(())
Expand Down

0 comments on commit b4024cf

Please sign in to comment.