Skip to content

Commit 919e58f

Browse files
authored
Prevent sync data loss from incomplete tracked files (#725)
Co-authored-by: Zhizhen He <hezhizhen.yi@gmail.com>
1 parent b4417ad commit 919e58f

7 files changed

Lines changed: 419 additions & 48 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates-tauri/yaak-app-client/src/sync_ext.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ pub(crate) async fn cmd_sync_calculate<R: Runtime>(
2929
let db = app_handle.db();
3030
let version = app_handle.package_info().version.to_string();
3131
let db_candidates = get_db_candidates(&db, &version, workspace_id, sync_dir)?;
32-
let fs_candidates = get_fs_candidates(sync_dir)?
32+
let fs_candidates = get_fs_candidates(sync_dir, &db_candidates)?
3333
.into_iter()
3434
// Only keep items in the same workspace
3535
.filter(|fs| fs.model.workspace_id() == workspace_id)
@@ -39,7 +39,7 @@ pub(crate) async fn cmd_sync_calculate<R: Runtime>(
3939

4040
pub(crate) async fn cmd_sync_calculate_fs(dir: &Path) -> Result<Vec<SyncOp>> {
4141
let db_candidates = Vec::new();
42-
let fs_candidates = get_fs_candidates(dir)?;
42+
let fs_candidates = get_fs_candidates(dir, &db_candidates)?;
4343
Ok(compute_sync_ops(db_candidates, fs_candidates))
4444
}
4545

‎crates/yaak-git/src/status.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,15 +135,15 @@ pub fn git_status(dir: &Path) -> crate::error::Result<GitStatusSummary> {
135135
let obj = entry.to_object(&repo)?;
136136
let content = obj.as_blob().unwrap().content();
137137
let name = Path::new(entry.name().unwrap_or_default());
138-
SyncModel::from_bytes(content.into(), name)?.map(|m| m.0)
138+
SyncModel::from_bytes(content.into(), name).ok().flatten().map(|m| m.0)
139139
}
140140
Err(_) => None,
141141
},
142142
};
143143

144144
let next = {
145145
let full_path = repo.workdir().unwrap().join(rela_path.clone());
146-
SyncModel::from_file(full_path.as_path())?.map(|m| m.0)
146+
SyncModel::from_file(full_path.as_path()).ok().flatten().map(|m| m.0)
147147
};
148148

149149
entries.push(GitStatusEntry {

‎crates/yaak-sync/Cargo.toml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,3 +18,6 @@ tokio = { workspace = true, features = ["fs", "sync", "macros", "rt"] }
1818
ts-rs = { workspace = true, features = ["chrono-impl", "serde-json-impl"] }
1919
yaak-models = { workspace = true }
2020
serde_path_to_error = "0.1.20"
21+
22+
[dev-dependencies]
23+
tempfile = "3"

‎crates/yaak-sync/src/models.rs‎

Lines changed: 12 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
use crate::error::Error::UnknownModel;
22
use crate::error::Result;
33
use chrono::NaiveDateTime;
4-
use log::{debug, warn};
4+
use log::debug;
55
use serde::{Deserialize, Deserializer, Serialize};
66
use serde_yaml::{Mapping, Value};
77
use sha1::{Digest, Sha1};
@@ -98,11 +98,13 @@ fn migrate_environment(obj: &mut Mapping) {
9898
}
9999

100100
impl SyncModel {
101+
/// Returns `Ok(None)` for files that don't look like Yaak models, and an error for ones that
102+
/// do but can't be parsed.
101103
pub fn from_bytes(content: Vec<u8>, file_path: &Path) -> Result<Option<(SyncModel, String)>> {
102104
let mut hasher = Sha1::new();
103105
hasher.update(&content);
104106
let checksum = hex::encode(hasher.finalize());
105-
let content_str = String::from_utf8(content.clone()).unwrap_or_default();
107+
let content_str = String::from_utf8(content).unwrap_or_default();
106108

107109
// Check for some strings that will be in a model file for sure. If these strings
108110
// don't exist, then it's probably not a Yaak file.
@@ -111,34 +113,19 @@ impl SyncModel {
111113
}
112114

113115
let ext = file_path.extension().unwrap_or_default();
114-
if ext == "yml" || ext == "yaml" {
115-
Ok(match serde_yaml::from_str::<SyncModel>(&content_str) {
116-
Ok(m) => Some((m, checksum)),
117-
Err(e) => {
118-
warn!("Error parsing {:?} {:?}", file_path.file_name(), e);
119-
None
120-
}
121-
})
116+
let model = if ext == "yml" || ext == "yaml" {
117+
serde_yaml::from_str::<SyncModel>(&content_str)?
122118
} else if ext == "json" {
123-
Ok(match serde_json::from_str::<SyncModel>(&content_str) {
124-
Ok(m) => Some((m, checksum)),
125-
Err(e) => {
126-
warn!("Error parsing {:?} {:?}", file_path.file_name(), e);
127-
None
128-
}
129-
})
119+
serde_json::from_str::<SyncModel>(&content_str)?
130120
} else {
131-
Ok(None)
132-
}
121+
return Ok(None);
122+
};
123+
124+
Ok(Some((model, checksum)))
133125
}
134126

135127
pub fn from_file(file_path: &Path) -> Result<Option<(SyncModel, String)>> {
136-
let content = match fs::read(file_path) {
137-
Ok(c) => c,
138-
Err(_) => return Ok(None),
139-
};
140-
141-
Self::from_bytes(content, file_path)
128+
Self::from_bytes(fs::read(file_path)?, file_path)
142129
}
143130

144131
pub fn to_file_contents(&self, rel_path: &Path) -> Result<(Vec<u8>, String)> {

‎crates/yaak-sync/src/sync.rs‎

Lines changed: 182 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
use crate::error::Result;
1+
use crate::error::{Error, Result};
22
use crate::models::SyncModel;
33
use chrono::Utc;
44
use log::{info, warn};
@@ -7,8 +7,9 @@ use std::collections::HashMap;
77
use std::fmt::{Display, Formatter};
88
use std::fs;
99
use std::fs::File;
10-
use std::io::Write;
10+
use std::io::{self, Write};
1111
use std::path::{Path, PathBuf};
12+
use std::sync::atomic::{AtomicUsize, Ordering};
1213
use ts_rs::TS;
1314
use yaak_models::blob_manager::BlobManager;
1415
use yaak_models::client_db::{ClientDb, WriteDb};
@@ -178,33 +179,77 @@ pub fn get_db_candidates(
178179
Ok(candidates)
179180
}
180181

181-
pub fn get_fs_candidates(dir: &Path) -> Result<Vec<FsCandidate>> {
182+
/// Read sync files, validating paths tracked by existing database models.
183+
///
184+
/// Workspace sync callers must pass all candidates from [`get_db_candidates`] for
185+
/// the same workspace and directory. An empty slice is for import discovery
186+
/// without database sync state; it does not validate tracked model identities.
187+
pub fn get_fs_candidates(dir: &Path, db_candidates: &[DbCandidate]) -> Result<Vec<FsCandidate>> {
182188
// Ensure the root directory exists
183189
fs::create_dir_all(dir)?;
184190

191+
let tracked_paths: HashMap<PathBuf, (&SyncModel, &SyncState)> = db_candidates
192+
.iter()
193+
.filter_map(|candidate| match candidate {
194+
DbCandidate::Added(_) | DbCandidate::Deleted(_) => None,
195+
DbCandidate::Modified(model, state) | DbCandidate::Unmodified(model, state) => {
196+
Some((PathBuf::from(&state.rel_path), (model, state)))
197+
}
198+
})
199+
.collect();
200+
185201
let mut candidates = Vec::new();
186202
let entries = fs::read_dir(dir)?;
187203
for dir_entry in entries {
188-
let dir_entry = match dir_entry {
189-
Ok(v) => v,
190-
Err(_) => continue,
191-
};
204+
let dir_entry = dir_entry?;
205+
let path = dir_entry.path();
206+
let rel_path = PathBuf::from(dir_entry.file_name());
207+
let tracked = tracked_paths.get(&rel_path);
208+
let invalid =
209+
|reason: String| Err(Error::InvalidSyncFile(format!("{}: {reason}", path.display())));
192210

193211
if !dir_entry.file_type()?.is_file() {
212+
if tracked.is_some() {
213+
return invalid("expected a regular sync file".into());
214+
}
194215
continue;
195216
};
196217

197-
let path = dir_entry.path();
198-
let (model, checksum) = match SyncModel::from_file(&path) {
199-
Ok(Some(m)) => m,
200-
Ok(None) => continue,
201-
Err(e) => {
202-
warn!("Failed to parse sync file {e}");
203-
return Err(e);
218+
// A present tracked file must not disappear from the candidates: that
219+
// would turn a read/parse failure into a database deletion.
220+
let (model, checksum) = match (SyncModel::from_file(&path), tracked) {
221+
(Ok(Some(m)), _) => m,
222+
(Ok(None), None) => continue,
223+
(Err(e), None) => {
224+
warn!("Skipping invalid sync file {}: {e}", path.display());
225+
continue;
204226
}
227+
(Ok(None), Some(_)) => return invalid("file does not contain a Yaak model".into()),
228+
(Err(e), Some(_)) => return invalid(e.to_string()),
205229
};
206230

207-
let rel_path = Path::new(&dir_entry.file_name()).to_path_buf();
231+
if let Some((previous, state)) = tracked {
232+
if model.id() != state.model_id {
233+
return invalid(format!(
234+
"expected model ID {}, found {}",
235+
state.model_id,
236+
model.id()
237+
));
238+
}
239+
// A wrong/defaulted workspace ID would be filtered out by the caller,
240+
// making this tracked model look deleted even if its ID is intact.
241+
if model.workspace_id() != state.workspace_id {
242+
return invalid(format!(
243+
"expected workspace ID {}, found {}",
244+
state.workspace_id,
245+
model.workspace_id()
246+
));
247+
}
248+
if std::mem::discriminant(&model) != std::mem::discriminant(*previous) {
249+
return invalid("model type does not match the tracked model".into());
250+
}
251+
}
252+
208253
candidates.push(FsCandidate { rel_path, model, checksum })
209254
}
210255

@@ -391,17 +436,15 @@ pub fn apply_fs_sync_ops(
391436
let rel_path = derive_model_filename(&model);
392437
let abs_path = sync_dir.join(rel_path.clone());
393438
let (content, checksum) = model.to_file_contents(&rel_path)?;
394-
let mut f = File::create(&abs_path)?;
395-
f.write_all(&content)?;
439+
write_sync_file(&abs_path, |file| file.write_all(&content))?;
396440
SyncStateOp::Create { model_id: model.id(), checksum, rel_path }
397441
}
398442
SyncOp::FsUpdate { model, state } => {
399443
// Always write the existing path
400444
let rel_path = Path::new(&state.rel_path);
401445
let abs_path = Path::new(&state.sync_dir).join(&rel_path);
402446
let (content, checksum) = model.to_file_contents(&rel_path)?;
403-
let mut f = File::create(&abs_path)?;
404-
f.write_all(&content)?;
447+
write_sync_file(&abs_path, |file| file.write_all(&content))?;
405448
SyncStateOp::Update {
406449
state: state.to_owned(),
407450
checksum,
@@ -447,6 +490,39 @@ pub fn apply_fs_sync_ops(
447490
Ok(pending)
448491
}
449492

493+
/// Publish a complete file without exposing partial writes to sync readers.
494+
///
495+
/// The temporary file's extension keeps it out of sync candidates. A crash may
496+
/// leave it behind, but it is never read as a model.
497+
fn write_sync_file(path: &Path, write: impl FnOnce(&mut File) -> io::Result<()>) -> Result<()> {
498+
static COUNTER: AtomicUsize = AtomicUsize::new(0);
499+
500+
// Opening the existing file for writing keeps read-only files from being replaced
501+
let permissions = match fs::OpenOptions::new().write(true).open(path) {
502+
Ok(file) => Some(file.metadata()?.permissions()),
503+
Err(error) if error.kind() == io::ErrorKind::NotFound => None,
504+
Err(error) => return Err(error.into()),
505+
};
506+
let tmp_path = path.with_file_name(format!(
507+
".{}.{}-{}.tmp",
508+
path.file_name().unwrap_or_default().to_string_lossy(),
509+
std::process::id(),
510+
COUNTER.fetch_add(1, Ordering::Relaxed),
511+
));
512+
let result = File::create_new(&tmp_path).and_then(|mut file| {
513+
if let Some(permissions) = permissions {
514+
file.set_permissions(permissions)?;
515+
}
516+
write(&mut file)?;
517+
drop(file);
518+
fs::rename(&tmp_path, path)
519+
});
520+
if result.is_err() {
521+
let _ = fs::remove_file(&tmp_path);
522+
}
523+
Ok(result?)
524+
}
525+
450526
impl PendingDbSyncOps {
451527
/// Upserts are collected per model type and written in one batch so
452528
/// foreign keys are satisfied.
@@ -603,3 +679,90 @@ fn delete_model(db: &WriteDb, blobs: &BlobManager, model: &SyncModel) -> Result<
603679
};
604680
Ok(())
605681
}
682+
683+
#[cfg(test)]
684+
mod write_tests {
685+
use super::*;
686+
687+
#[test]
688+
fn failed_partial_write_keeps_previous_file() {
689+
let dir = tempfile::tempdir().unwrap();
690+
let path = dir.path().join("request.yaml");
691+
fs::write(&path, b"complete previous contents").unwrap();
692+
let result = write_sync_file(&path, |file| {
693+
file.write_all(b"partial new contents")?;
694+
Err(io::Error::new(io::ErrorKind::WriteZero, "simulated write failure"))
695+
});
696+
assert!(
697+
matches!(result, Err(Error::IoError(error)) if error.kind() == io::ErrorKind::WriteZero)
698+
);
699+
assert_eq!(fs::read(&path).unwrap(), b"complete previous contents");
700+
assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1);
701+
}
702+
703+
#[test]
704+
fn readers_ignore_the_in_progress_file() {
705+
let dir = tempfile::tempdir().unwrap();
706+
let path = dir.path().join("request.yaml");
707+
let request = HttpRequest {
708+
id: "rq_write_test".into(),
709+
workspace_id: "wk_write_test".into(),
710+
name: "Previous request".into(),
711+
..Default::default()
712+
};
713+
let model = SyncModel::HttpRequest(request.clone());
714+
let (old_contents, checksum) = model.to_file_contents(&path).unwrap();
715+
fs::write(&path, &old_contents).unwrap();
716+
let db_candidates = vec![DbCandidate::Unmodified(
717+
model,
718+
SyncState {
719+
model_id: request.id.clone(),
720+
workspace_id: request.workspace_id.clone(),
721+
rel_path: "request.yaml".into(),
722+
checksum,
723+
..Default::default()
724+
},
725+
)];
726+
let model = SyncModel::HttpRequest(HttpRequest { name: "New request".into(), ..request });
727+
let (new_contents, _) = model.to_file_contents(&path).unwrap();
728+
let split = new_contents.len() / 2;
729+
write_sync_file(&path, |file| {
730+
file.write_all(&new_contents[..split])?;
731+
assert_eq!(fs::read(&path).unwrap(), old_contents);
732+
let fs_candidates = get_fs_candidates(dir.path(), &db_candidates).unwrap();
733+
assert_eq!(fs_candidates.len(), 1);
734+
assert!(compute_sync_ops(db_candidates.clone(), fs_candidates).is_empty());
735+
file.write_all(&new_contents[split..])
736+
})
737+
.unwrap();
738+
assert_eq!(fs::read(&path).unwrap(), new_contents);
739+
assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1);
740+
}
741+
742+
#[cfg(unix)]
743+
#[test]
744+
fn writes_preserve_existing_permissions_and_new_file_defaults() {
745+
use std::os::unix::fs::PermissionsExt;
746+
747+
let dir = tempfile::tempdir().unwrap();
748+
let path = dir.path().join("request.yaml");
749+
fs::write(&path, b"old contents").unwrap();
750+
fs::set_permissions(&path, fs::Permissions::from_mode(0o640)).unwrap();
751+
write_sync_file(&path, |file| {
752+
// Keep unpublished data at least as private as the original file.
753+
assert_eq!(file.metadata()?.permissions().mode() & 0o777 & !0o640, 0);
754+
file.write_all(b"new contents")
755+
})
756+
.unwrap();
757+
assert_eq!(fs::metadata(&path).unwrap().permissions().mode() & 0o777, 0o640);
758+
759+
let reference = dir.path().join("reference.yaml");
760+
File::create(&reference).unwrap();
761+
let new_path = dir.path().join("new.yaml");
762+
write_sync_file(&new_path, |file| file.write_all(b"new contents")).unwrap();
763+
assert_eq!(
764+
fs::metadata(&new_path).unwrap().permissions().mode() & 0o777,
765+
fs::metadata(&reference).unwrap().permissions().mode() & 0o777
766+
);
767+
}
768+
}

0 commit comments

Comments
 (0)