Projects: restore store-owned fields under the store lock on save (PR re-review)
update_project restored marketplace installs (and status, container id, flags) from a copy read before validation, then wrote the whole record later, so an install landing in between was lost. ProjectsStore gains update_restoring, which runs restore_store_owned_fields against the record as stored under the lock, and update_project writes through it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1112,7 +1112,14 @@ pub async fn update_project(
|
|||||||
// for every already-running container at launch. The version of this that
|
// for every already-running container at launch. The version of this that
|
||||||
// re-asserted on every save is what turned a stale flag in a payload into a
|
// re-asserted on every save is what turned a stale flag in a payload into a
|
||||||
// restarted bridge.
|
// restarted bridge.
|
||||||
state.projects_store.update(project)
|
//
|
||||||
|
// The restore above served the validation; it is redone under the store's
|
||||||
|
// lock against the record as it is *now*, so a marketplace install, a
|
||||||
|
// status change or a container id landing since `stored` was read is kept
|
||||||
|
// rather than written over.
|
||||||
|
state
|
||||||
|
.projects_store
|
||||||
|
.update_restoring(project, restore_store_owned_fields)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Restore onto `project` the fields whose value belongs to the store rather
|
/// Restore onto `project` the fields whose value belongs to the store rather
|
||||||
|
|||||||
@@ -205,6 +205,27 @@ impl ProjectsStore {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Replace a project with `updated`, after `restore` has copied onto it
|
||||||
|
/// the fields the store owns from the record *as stored under the lock*.
|
||||||
|
/// `update_project` restores from a copy it read earlier, so an install
|
||||||
|
/// or a status change landing in between would otherwise be written over
|
||||||
|
/// (re-review round 2).
|
||||||
|
pub fn update_restoring(
|
||||||
|
&self,
|
||||||
|
mut updated: Project,
|
||||||
|
restore: impl FnOnce(&mut Project, &Project),
|
||||||
|
) -> Result<Project, String> {
|
||||||
|
let mut projects = self.lock();
|
||||||
|
let p = projects
|
||||||
|
.iter_mut()
|
||||||
|
.find(|p| p.id == updated.id)
|
||||||
|
.ok_or_else(|| format!("Project {} not found", updated.id))?;
|
||||||
|
restore(&mut updated, p);
|
||||||
|
*p = updated.clone();
|
||||||
|
self.save(&projects)?;
|
||||||
|
Ok(updated)
|
||||||
|
}
|
||||||
|
|
||||||
pub fn remove(&self, id: &str) -> Result<(), String> {
|
pub fn remove(&self, id: &str) -> Result<(), String> {
|
||||||
let mut projects = self.lock();
|
let mut projects = self.lock();
|
||||||
let initial_len = projects.len();
|
let initial_len = projects.len();
|
||||||
@@ -520,6 +541,42 @@ mod tests {
|
|||||||
fs::remove_dir_all(&dir).ok();
|
fs::remove_dir_all(&dir).ok();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_project_save_keeps_a_marketplace_install_made_after_it_read_the_record() {
|
||||||
|
// Re-review round 2: `update_project` read the stored record, then
|
||||||
|
// wrote the whole payload back later. An install landing in between
|
||||||
|
// was lost. The restore now runs against the record under the lock.
|
||||||
|
let dir = temp_dir("save-restore");
|
||||||
|
let project = Project::new("demo".to_string(), Vec::new());
|
||||||
|
let id = project.id.clone();
|
||||||
|
let store = store_over(&dir, vec![project]);
|
||||||
|
|
||||||
|
let mut payload = store.get(&id).unwrap(); // the Config tab's copy
|
||||||
|
payload.name = "renamed".to_string();
|
||||||
|
store
|
||||||
|
.update_marketplace_fields(&id, |installs, _| {
|
||||||
|
installs.push(market_install("late"));
|
||||||
|
Ok(())
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let saved = store
|
||||||
|
.update_restoring(payload, |incoming, stored| {
|
||||||
|
incoming.marketplace_installs = stored.marketplace_installs.clone();
|
||||||
|
incoming.marketplace_disabled = stored.marketplace_disabled.clone();
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(saved.name, "renamed");
|
||||||
|
assert_eq!(saved.marketplace_installs, vec![market_install("late")]);
|
||||||
|
assert_eq!(store.get(&id).unwrap().marketplace_installs, vec![market_install("late")]);
|
||||||
|
|
||||||
|
let mut ghost = Project::new("ghost".to_string(), Vec::new());
|
||||||
|
ghost.id = "nope".into();
|
||||||
|
assert!(store.update_restoring(ghost, |_, _| {}).is_err());
|
||||||
|
|
||||||
|
fs::remove_dir_all(&dir).ok();
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn marketplace_edits_across_all_projects_touch_only_those_fields() {
|
fn marketplace_edits_across_all_projects_touch_only_those_fields() {
|
||||||
let dir = temp_dir("marketplace-all");
|
let dir = temp_dir("marketplace-all");
|
||||||
|
|||||||
Reference in New Issue
Block a user