Merge pull request #143 from storytold/fix/issue-134-trim-write-safety
Issue #134: Exports: atomic without fsync; Copy import verifies against the probe hash
This commit is contained in:
Brandon Thomas
GitHub
commit
d2359ce095
|
|
@ -1,11 +1,23 @@
|
|||
//! Crash-safe writes of user-visible files: exports, renders, DNG conversions, smart previews and
|
||||
//! XMP sidecars all go through here (the catalog's own files use [`crate::store::FsStore`]).
|
||||
//!
|
||||
//! **Write policy** (issue #134): what can't be recreated — originals, XMP sidecars, the catalog,
|
||||
//! settings, DNGs that replace raws, smart previews (the only stand-in while an original is
|
||||
//! offline), an edit copy that becomes a library photo — is written **durably**: synced to disk
|
||||
//! before it counts as written. What can always be made again from the originals — exports,
|
||||
//! renders, screenshots, merge previews — is written **atomically** only: a half-written file
|
||||
//! never replaces a good one, but there is no per-file sync (on a USB drive or a NAS that sync
|
||||
//! dominates a large export).
|
||||
//!
|
||||
//! - [`write_atomic`] replaces a file so that after a crash, a full disk or an unplugged drive
|
||||
//! either the old or the complete new content is there — never a truncated mix. The content
|
||||
//! goes to a new temp file in the same folder (unique name, `create_new`), is synced to disk and
|
||||
//! renamed over the target; then the folder is synced (POSIX). Any failure removes the temp
|
||||
//! file and leaves the target as it was.
|
||||
//! - [`write_atomic_nosync`] is the same temp file + rename without the two syncs, for
|
||||
//! recreatable outputs only: a failed write still removes the temp file and leaves the target as
|
||||
//! it was, but after a power cut the new file may be lost (on some file systems left empty) —
|
||||
//! it is then simply exported again.
|
||||
//! - [`write_new`] / [`write_new_unique`] write a file that must not exist yet: the same temp file
|
||||
//! and sync, plus a read-back check, and the file then appears under its final name **without
|
||||
//! replacing anything** (a hard link, which fails on a taken name; a check-then-rename where the
|
||||
|
|
@ -21,6 +33,19 @@ use std::sync::atomic::{AtomicU64, Ordering};
|
|||
|
||||
thread_local! {
|
||||
static FAIL_AFTER: Cell<Option<u64>> = const { Cell::new(None) };
|
||||
static SYNCS: Cell<u64> = const { Cell::new(0) };
|
||||
}
|
||||
|
||||
/// How many file and folder syncs this module made **on the calling thread** (for tests: a
|
||||
/// durable write path syncs, a recreatable output doesn't).
|
||||
#[doc(hidden)]
|
||||
pub fn syncs_on_this_thread() -> u64 {
|
||||
SYNCS.with(Cell::get)
|
||||
}
|
||||
|
||||
fn sync_file(f: &File) -> io::Result<()> {
|
||||
SYNCS.with(|c| c.set(c.get() + 1));
|
||||
f.sync_all()
|
||||
}
|
||||
|
||||
/// Fault injection for tests (in any crate): while the returned guard lives, every write made by
|
||||
|
|
@ -78,7 +103,7 @@ fn parent_of(path: &Path) -> &Path {
|
|||
/// where NTFS journals the rename itself, so errors are ignored.
|
||||
pub fn sync_dir(dir: &Path) {
|
||||
if let Ok(d) = File::open(dir) {
|
||||
let _ = d.sync_all();
|
||||
let _ = sync_file(&d);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -100,9 +125,9 @@ fn create_temp(path: &Path) -> io::Result<(PathBuf, File)> {
|
|||
Err(io::Error::new(io::ErrorKind::AlreadyExists, format!("no free temp file name in {}", dir.display())))
|
||||
}
|
||||
|
||||
/// Write a temp file next to `path` with what `fill` writes, synced to disk. On failure the temp
|
||||
/// file is removed. Returns its path and length.
|
||||
fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> {
|
||||
/// Write a temp file next to `path` with what `fill` writes, synced to disk when `sync`. On
|
||||
/// failure the temp file is removed. Returns its path and length.
|
||||
fn write_temp(path: &Path, sync: bool, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<(PathBuf, u64)> {
|
||||
let (tmp, file) = create_temp(path)?;
|
||||
let r = (|| {
|
||||
let limit = FAIL_AFTER.with(Cell::get);
|
||||
|
|
@ -111,7 +136,9 @@ fn write_temp(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()
|
|||
w.flush()?;
|
||||
let n = w.n;
|
||||
let f = w.inner.into_inner().map_err(|e| e.into_error())?;
|
||||
f.sync_all()?;
|
||||
if sync {
|
||||
sync_file(&f)?;
|
||||
}
|
||||
Ok(n)
|
||||
})();
|
||||
match r {
|
||||
|
|
@ -132,12 +159,26 @@ pub fn write_atomic(path: &Path, data: &[u8]) -> io::Result<()> {
|
|||
/// [`write_atomic`] with streamed content: `fill` writes it (an error aborts and leaves the old
|
||||
/// file). Returns the length written.
|
||||
pub fn write_atomic_with(path: &Path, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<u64> {
|
||||
let (tmp, n) = write_temp(path, fill)?;
|
||||
replace_with(path, true, fill)
|
||||
}
|
||||
|
||||
/// Replace (or create) `path` with `data` atomically but **without syncing** it to disk — only for
|
||||
/// outputs that can always be recreated (exports, renders, screenshots); see the module docs.
|
||||
/// Anything that can't be made again goes through [`write_atomic`].
|
||||
pub fn write_atomic_nosync(path: &Path, data: &[u8]) -> io::Result<()> {
|
||||
replace_with(path, false, &mut |w| w.write_all(data)).map(|_| ())
|
||||
}
|
||||
|
||||
/// Temp file (synced when `sync`) renamed over `path`, then the folder synced when `sync`.
|
||||
fn replace_with(path: &Path, sync: bool, fill: &mut dyn FnMut(&mut dyn Write) -> io::Result<()>) -> io::Result<u64> {
|
||||
let (tmp, n) = write_temp(path, sync, fill)?;
|
||||
if let Err(e) = fs::rename(&tmp, path) {
|
||||
let _ = fs::remove_file(&tmp);
|
||||
return Err(e);
|
||||
}
|
||||
sync_dir(parent_of(path));
|
||||
if sync {
|
||||
sync_dir(parent_of(path));
|
||||
}
|
||||
Ok(n)
|
||||
}
|
||||
|
||||
|
|
@ -155,8 +196,11 @@ pub fn write_new_unique(candidates: &mut dyn Iterator<Item = PathBuf>, data: &[u
|
|||
let Some(first) = candidates.peek().cloned() else {
|
||||
return Err(io::Error::new(io::ErrorKind::InvalidInput, "no file name to write"));
|
||||
};
|
||||
let (tmp, _) = write_temp(&first, &mut |w| w.write_all(data))?;
|
||||
let (tmp, _) = write_temp(&first, true, &mut |w| w.write_all(data))?;
|
||||
let r = (|| {
|
||||
// Kept (issue #134 point 4): these files replace raws (Convert / Copy as DNG) or become
|
||||
// library photos (merges). On a local disk the read usually comes from the cache, but on a
|
||||
// network volume it can go back to the server, and next to encoding a DNG it costs little.
|
||||
if !same_content(&tmp, data)? {
|
||||
return Err(io::Error::other("the written file reads back different (failing drive or connection?)"));
|
||||
}
|
||||
|
|
@ -287,6 +331,33 @@ mod tests {
|
|||
let _ = fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn recreatable_outputs_are_atomic_without_a_sync() {
|
||||
let d = temp("nosync");
|
||||
let p = d.join("export.jpg");
|
||||
let before = syncs_on_this_thread();
|
||||
write_atomic_nosync(&p, b"one").unwrap();
|
||||
write_atomic_nosync(&p, b"two two").unwrap();
|
||||
assert_eq!(syncs_on_this_thread(), before, "no file or folder sync");
|
||||
assert_eq!(fs::read(&p).unwrap(), b"two two");
|
||||
assert_eq!(names(&d), vec!["export.jpg"], "no temp file left");
|
||||
// still atomic: a failure part-way leaves the previous file and no temp file
|
||||
{
|
||||
let _f = fail_writes_after(3);
|
||||
assert!(write_atomic_nosync(&p, &[9u8; 10_000]).is_err());
|
||||
}
|
||||
assert_eq!(fs::read(&p).unwrap(), b"two two");
|
||||
assert_eq!(names(&d), vec!["export.jpg"]);
|
||||
// the durable writers do sync (the file; the folder too where it can be opened)
|
||||
let before = syncs_on_this_thread();
|
||||
write_atomic(&p, b"three").unwrap();
|
||||
let mid = syncs_on_this_thread();
|
||||
assert!(mid > before);
|
||||
write_new(&d.join("new.dng"), b"dng").unwrap();
|
||||
assert!(syncs_on_this_thread() > mid);
|
||||
let _ = fs::remove_dir_all(&d);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn new_files_never_replace_anything() {
|
||||
let d = temp("new");
|
||||
|
|
|
|||
|
|
@ -149,7 +149,8 @@ fn edit_external(s: &mut Session, p: &Value) -> Result<Value> {
|
|||
};
|
||||
let mut written = Vec::new();
|
||||
let mut write = |path: &str, bytes: &[u8]| -> std::result::Result<(), String> {
|
||||
crate::export::write_file(path, bytes)?;
|
||||
// durable: the TIFF becomes a library photo the external editor then changes
|
||||
crate::export::write_file_durable(path, bytes)?;
|
||||
written.push(path.to_string());
|
||||
Ok(())
|
||||
};
|
||||
|
|
|
|||
|
|
@ -160,7 +160,9 @@ fn smart_run(
|
|||
if damaged && crate::smart::is_valid(&path) {
|
||||
n.built += 1;
|
||||
} else {
|
||||
// atomic: a failed write leaves no partial proxy that would pass for a built one
|
||||
// atomic: a failed write leaves no partial proxy that would pass for a built one;
|
||||
// and synced (issue #134): built so the photo can be edited while its original
|
||||
// is offline, when it is the only copy — the sync is small next to the decode
|
||||
let r = source
|
||||
.ok_or_else(|| "nothing to build from".to_string())
|
||||
.and_then(|s| s.load_source())
|
||||
|
|
|
|||
|
|
@ -1131,15 +1131,27 @@ pub fn export_batch(
|
|||
}
|
||||
|
||||
/// Write an exported or rendered file on disk: its folder is created if needed, and the file is
|
||||
/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic`]: a temp file, synced, then
|
||||
/// renamed), so a failure part-way leaves any previous file intact and no truncated one. The
|
||||
/// native writer behind exports, renders and screenshots (app, CLI, MCP).
|
||||
/// replaced atomically ([`lightcraft_catalog::safe_file::write_atomic_nosync`]: a temp file
|
||||
/// renamed into place), so a failure part-way leaves any previous file intact and no truncated
|
||||
/// one. Not synced to disk (issue #134): an export can always be made again from the original.
|
||||
/// The native writer behind exports, renders and screenshots (app, CLI, MCP).
|
||||
pub fn write_file(path: &str, bytes: &[u8]) -> Result<(), String> {
|
||||
write_with(path, bytes, false)
|
||||
}
|
||||
|
||||
/// [`write_file`], synced to disk: for a written file that becomes a library photo (Edit a Copy)
|
||||
/// and so can't simply be exported again once the catalog points at it.
|
||||
pub fn write_file_durable(path: &str, bytes: &[u8]) -> Result<(), String> {
|
||||
write_with(path, bytes, true)
|
||||
}
|
||||
|
||||
fn write_with(path: &str, bytes: &[u8], durable: bool) -> Result<(), String> {
|
||||
use lightcraft_catalog::safe_file::{write_atomic, write_atomic_nosync};
|
||||
let p = std::path::Path::new(path);
|
||||
if let Some(dir) = p.parent().filter(|d| !d.as_os_str().is_empty()) {
|
||||
std::fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?;
|
||||
}
|
||||
lightcraft_catalog::safe_file::write_atomic(p, bytes).map_err(|e| format!("{path}: {e}"))
|
||||
if durable { write_atomic(p, bytes) } else { write_atomic_nosync(p, bytes) }.map_err(|e| format!("{path}: {e}"))
|
||||
}
|
||||
|
||||
/// The path of the sidecar with extension `ext` of the exported file `main`.
|
||||
|
|
|
|||
|
|
@ -375,13 +375,16 @@ fn probe_all(probe: Option<&crate::media::FileProbe>, paths: &[String], progress
|
|||
|
||||
/// Copy `src` into `root`/`folders` as `name` (default: its own name), made unique with -1, -2…;
|
||||
/// returns the new path. The copy is verified ([`crate::import_move::copy_new`]: a new file,
|
||||
/// synced and compared byte for byte); a bad one is removed and the photo reported as failed.
|
||||
fn copy_into(root: &Path, src: &str, folders: &[String], name: Option<&str>) -> Result<String, String> {
|
||||
/// synced and checked against `probe_hash`, the content hash the probe computed — or compared
|
||||
/// byte for byte with the source when there is none); a bad one is removed and the photo
|
||||
/// reported as failed.
|
||||
fn copy_into(root: &Path, src: &str, folders: &[String], name: Option<&str>, probe_hash: Option<&str>) -> Result<String, String> {
|
||||
let dir = folders.iter().fold(root.to_path_buf(), |d, f| d.join(f));
|
||||
let name = name
|
||||
.map(str::to_string)
|
||||
.unwrap_or_else(|| Path::new(src).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "photo".into()));
|
||||
crate::import_move::copy_new(Path::new(src), &dir, &name).map(|p| p.to_string_lossy().to_string())
|
||||
let expect = probe_hash.and_then(lightcraft_preview::Hash128::parse);
|
||||
crate::import_move::copy_new(Path::new(src), &dir, &name, expect).map(|p| p.to_string_lossy().to_string())
|
||||
}
|
||||
|
||||
/// What a [`scan`] needs from the session, so it can run on another thread (a folder on a network
|
||||
|
|
@ -784,7 +787,7 @@ impl ImportJob {
|
|||
let reason = "a symbolic link: the file it points to was copied, the link and its target stay";
|
||||
out.items.push(PreparedItem::Kept(Kept { path: path.clone(), reason: reason.into() }));
|
||||
}
|
||||
match copy_into(root, &path, &folders, name.as_deref()) {
|
||||
match copy_into(root, &path, &folders, name.as_deref(), info.content_hash.as_deref()) {
|
||||
Ok(p) => p,
|
||||
Err(e) => {
|
||||
out.items.push(PreparedItem::Failed(path, e));
|
||||
|
|
|
|||
|
|
@ -171,9 +171,15 @@ pub(crate) fn place(src: &Path, dir: &Path, name: &str) -> Result<Placed, String
|
|||
|
||||
/// Import → **Copy**: copy `src` into `dir` as `name` (-1, -2… when taken) with the same checks as
|
||||
/// a move's copy — a new file (never overwriting one, even one that appears meanwhile), synced to
|
||||
/// disk and compared byte for byte with the source; a bad copy is removed and reported, so a card
|
||||
/// is never wiped on the strength of it. Returns the new path.
|
||||
pub(crate) fn copy_new(src: &Path, dir: &Path, name: &str) -> Result<PathBuf, String> {
|
||||
/// disk and verified; a bad copy is removed and reported, so a card is never wiped on the strength
|
||||
/// of it. Returns the new path.
|
||||
///
|
||||
/// `expect` is the content hash the import's probe computed from its full read of the source
|
||||
/// ([`lightcraft_preview::hash_bytes`]): the copy is read back and checked against it, which spares
|
||||
/// a third read of the card (issue #134). It also catches a source that changed since the probe
|
||||
/// (whose recorded metadata and hash would be stale). Without one the copy is compared byte for
|
||||
/// byte with the source.
|
||||
pub(crate) fn copy_new(src: &Path, dir: &Path, name: &str, expect: Option<lightcraft_preview::Hash128>) -> Result<PathBuf, String> {
|
||||
fs::create_dir_all(dir).map_err(|e| format!("{}: {e}", dir.display()))?;
|
||||
let (stem, ext) = match name.rsplit_once('.') {
|
||||
Some((s, e)) if !s.is_empty() => (s.to_string(), format!(".{e}")),
|
||||
|
|
@ -181,7 +187,7 @@ pub(crate) fn copy_new(src: &Path, dir: &Path, name: &str) -> Result<PathBuf, St
|
|||
};
|
||||
for i in 0..100_000u32 {
|
||||
let d = if i == 0 { dir.join(name) } else { dir.join(format!("{stem}-{i}{ext}")) };
|
||||
match copy_verified(src, &d) {
|
||||
match copy_verified(src, &d, expect) {
|
||||
Ok(()) => return Ok(d),
|
||||
Err(e) if e.kind() == io::ErrorKind::AlreadyExists => continue,
|
||||
Err(e) => return Err(format!("copy {}: {e} (nothing was imported from it; the original is untouched)", src.display())),
|
||||
|
|
@ -262,12 +268,14 @@ fn link_or_copy(src: &Path, dst: &Path) -> io::Result<()> {
|
|||
Err(_) => {}
|
||||
}
|
||||
}
|
||||
copy_verified(src, dst)
|
||||
// Move keeps the byte-for-byte comparison: the source is deleted on the strength of it
|
||||
copy_verified(src, dst, None)
|
||||
}
|
||||
|
||||
/// Copy into a new file, sync it to disk and compare it with the original; on any failure the
|
||||
/// partial copy is removed.
|
||||
fn copy_verified(src: &Path, dst: &Path) -> io::Result<()> {
|
||||
/// Copy into a new file, sync it to disk and verify it: against `expect` (the probe's hash of the
|
||||
/// source) when given, else byte for byte against the source. On any failure the partial copy is
|
||||
/// removed.
|
||||
pub(crate) fn copy_verified(src: &Path, dst: &Path, expect: Option<lightcraft_preview::Hash128>) -> io::Result<()> {
|
||||
let mut out = OpenOptions::new().write(true).create_new(true).open(dst)?;
|
||||
let r = (|| {
|
||||
let mut inp = File::open(src)?;
|
||||
|
|
@ -283,10 +291,14 @@ fn copy_verified(src: &Path, dst: &Path) -> io::Result<()> {
|
|||
if let Ok(m) = fs::metadata(src).and_then(|m| m.modified()) {
|
||||
let _ = out.set_modified(m);
|
||||
}
|
||||
if !same_bytes(src, dst)? {
|
||||
return Err(io::Error::other("the copy differs from the original"));
|
||||
match expect {
|
||||
Some(h) if hash_file(dst)? != h => {
|
||||
Err(io::Error::other("the copy differs from the original as it was read for the import (a bad copy, or the file changed meanwhile)"))
|
||||
}
|
||||
Some(_) => Ok(()),
|
||||
None if !same_bytes(src, dst)? => Err(io::Error::other("the copy differs from the original")),
|
||||
None => Ok(()),
|
||||
}
|
||||
Ok(())
|
||||
})();
|
||||
drop(out);
|
||||
if r.is_err() {
|
||||
|
|
@ -295,6 +307,20 @@ fn copy_verified(src: &Path, dst: &Path) -> io::Result<()> {
|
|||
r
|
||||
}
|
||||
|
||||
/// [`lightcraft_preview::hash_bytes`] of a file's content, read in chunks.
|
||||
fn hash_file(path: &Path) -> io::Result<lightcraft_preview::Hash128> {
|
||||
let mut f = File::open(path)?;
|
||||
let mut h = lightcraft_preview::Hasher128::new();
|
||||
let mut buf = vec![0u8; 1 << 20];
|
||||
loop {
|
||||
let n = fill(&mut f, &mut buf)?;
|
||||
if n == 0 {
|
||||
return Ok(h.finish());
|
||||
}
|
||||
h.update(buf.get(..n).unwrap_or(&[]));
|
||||
}
|
||||
}
|
||||
|
||||
/// Do two files hold the same bytes?
|
||||
pub(crate) fn same_bytes(a: &Path, b: &Path) -> io::Result<bool> {
|
||||
if fs::metadata(a)?.len() != fs::metadata(b)?.len() {
|
||||
|
|
|
|||
|
|
@ -830,7 +830,9 @@ pub struct ProbeInfo {
|
|||
pub captured: Option<String>,
|
||||
pub meta: lightcraft_catalog::Meta,
|
||||
pub as_shot_wb: Option<(f64, f64)>,
|
||||
/// Hash of the file's bytes (hex), for duplicate detection.
|
||||
/// Hash of the file's bytes (hex), for duplicate detection. When it is a 32-digit
|
||||
/// [`lightcraft_preview::hash_bytes`] of the whole file (as the native probe computes it),
|
||||
/// Import → Copy verifies each copy against it instead of reading the source again.
|
||||
pub content_hash: Option<String>,
|
||||
/// Lens corrections embedded in the file (DNG opcodes).
|
||||
pub embedded_lens: Option<lightcraft_develop::EmbeddedLens>,
|
||||
|
|
|
|||
|
|
@ -68,9 +68,14 @@ impl crate::Session {
|
|||
OriginalGuard::new(&self.catalog)
|
||||
}
|
||||
|
||||
/// `Err` when writing `path` would replace a catalogued photo's original or sidecar.
|
||||
/// `Err` when writing `path` would replace a catalogued photo's original or sidecar. A free
|
||||
/// path (the usual case) is answered without building the guard over the whole library.
|
||||
pub fn check_write_target(&self, path: &str) -> Result<(), String> {
|
||||
self.original_guard().check(Path::new(path))
|
||||
let path = Path::new(path);
|
||||
if std::fs::symlink_metadata(path).is_err() {
|
||||
return Ok(()); // nothing there to replace
|
||||
}
|
||||
self.original_guard().check(path)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -323,3 +323,24 @@ fn a_failed_export_write_keeps_the_previous_file() {
|
|||
assert_ne!(std::fs::read(&prev).unwrap(), b"last week's export");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
||||
/// Issue #134: exports can always be made again, so they are written atomically but not synced to
|
||||
/// disk file by file; an edit copy that becomes a library photo still is.
|
||||
#[test]
|
||||
fn exports_are_atomic_without_a_sync() {
|
||||
use crate::export::{Conflict, Destination};
|
||||
use lightcraft_catalog::safe_file::syncs_on_this_thread;
|
||||
let (mut s, id, dir, _src, _) = library_with_jpeg("export-nosync");
|
||||
let out = dir.join("out");
|
||||
let to = Destination { dir: out.to_string_lossy().to_string(), exact: None };
|
||||
let o = ExportOptions { conflict: Conflict::Unique, ..Default::default() };
|
||||
let before = syncs_on_this_thread();
|
||||
for _ in 0..3 {
|
||||
disk_batch(&mut s, id, &o, &to).unwrap();
|
||||
}
|
||||
assert_eq!(syncs_on_this_thread(), before, "no per-file sync for exports");
|
||||
assert_eq!(std::fs::read_dir(&out).unwrap().count(), 3, "three exports, no temp files");
|
||||
crate::export::write_file_durable(&out.join("edit.tif").to_string_lossy(), b"tiff").unwrap();
|
||||
assert!(syncs_on_this_thread() > before, "the durable writer syncs");
|
||||
let _ = std::fs::remove_dir_all(&dir);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -207,6 +207,67 @@ fn copy_import_verifies_each_copy() {
|
|||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
||||
/// Copy verifies against the probe's content hash (issue #134): the hash, not a re-read of the
|
||||
/// source, is the reference — a copy that matches the source but not the hash is refused, a
|
||||
/// corrupted copy is caught, and the file hash agrees with `hash_bytes` across read chunks.
|
||||
#[test]
|
||||
fn copy_is_verified_against_the_probe_hash() {
|
||||
use crate::import_move::copy_verified;
|
||||
let base = temp_dir("copyhash");
|
||||
let src = base.join("src.bin");
|
||||
// larger than one 1 MB read, not a multiple of it
|
||||
let bytes: Vec<u8> = (0..(3 << 20) + 12_345u32).map(|i| (i.wrapping_mul(2_654_435_761) >> 13) as u8).collect();
|
||||
std::fs::write(&src, &bytes).unwrap();
|
||||
let good = lightcraft_preview::hash_bytes(&bytes);
|
||||
copy_verified(&src, &base.join("ok.bin"), Some(good)).unwrap();
|
||||
assert_eq!(std::fs::read(base.join("ok.bin")).unwrap(), bytes);
|
||||
// a hash that doesn't match: refused (a byte compare with the source would have passed)
|
||||
let stale = lightcraft_preview::hash_bytes(b"what the probe read earlier");
|
||||
let e = copy_verified(&src, &base.join("stale.bin"), Some(stale)).unwrap_err();
|
||||
assert!(e.to_string().contains("differs"), "{e}");
|
||||
assert!(!base.join("stale.bin").exists(), "the refused copy is removed");
|
||||
// a copy that comes out different: caught by the hash
|
||||
inject(Fault::Corrupt);
|
||||
let r = copy_verified(&src, &base.join("bad.bin"), Some(good));
|
||||
inject(Fault::None);
|
||||
assert!(r.is_err());
|
||||
assert!(!base.join("bad.bin").exists());
|
||||
assert_eq!(std::fs::read(&src).unwrap(), bytes, "the source is untouched");
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
||||
/// A file replaced after Review Import probed it is not imported on the strength of the old probe:
|
||||
/// its copy no longer matches the probe's hash, so it's reported as failed and nothing is left.
|
||||
#[test]
|
||||
fn copy_import_refuses_a_file_changed_since_the_review() {
|
||||
let base = temp_dir("copychanged");
|
||||
let (card, dest) = (base.join("card"), base.join("out"));
|
||||
write_png(&card.join("a.png"), 3);
|
||||
let mut s = session();
|
||||
let r = s.execute("library.importPreview", &json!({"paths": [card.to_string_lossy()]})).unwrap();
|
||||
assert_eq!(r["scanned"], 1, "{r}");
|
||||
write_png(&card.join("a.png"), 9); // other pixels
|
||||
let r = s
|
||||
.execute(
|
||||
"library.import",
|
||||
&json!({"paths": [card.to_string_lossy()], "mode": "copy", "destination": dest.to_string_lossy(), "organize": "flat"}),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(len(&r, "imported"), 0, "{r}");
|
||||
assert_eq!(len(&r, "failed"), 1, "{r}");
|
||||
assert!(r.to_string().contains("changed meanwhile"), "{r}");
|
||||
assert_eq!(files_under(&dest), Vec::<String>::new());
|
||||
// importing again probes afresh and succeeds
|
||||
let r = s
|
||||
.execute(
|
||||
"library.import",
|
||||
&json!({"paths": [card.to_string_lossy()], "mode": "copy", "destination": dest.to_string_lossy(), "organize": "flat"}),
|
||||
)
|
||||
.unwrap();
|
||||
assert_eq!(len(&r, "imported"), 1, "{r}");
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
||||
/// A source that can't be removed (a read-only card) stays and is reported; the photo is
|
||||
/// imported from its copy.
|
||||
#[test]
|
||||
|
|
@ -320,3 +381,55 @@ fn move_into_the_library_is_saved_before_sources_go() {
|
|||
assert_eq!(photo_paths(&again), [want.to_string_lossy()]);
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
||||
/// Timing for issue #134 (not a pass/fail test): `cargo test -p lightcraft-engine --release
|
||||
/// write_policy_timing -- --ignored --nocapture`. Writes N export-sized files durably (temp +
|
||||
/// sync + rename) and atomically only, and copies N files verified byte for byte and against the
|
||||
/// probe hash, alternating so a loaded machine affects both alike. `LIGHTCRAFT_BENCH_DIR` puts
|
||||
/// the files on another volume (a USB drive or a NAS is where the difference shows).
|
||||
#[test]
|
||||
#[ignore]
|
||||
fn write_policy_timing() {
|
||||
use crate::import_move::copy_verified;
|
||||
use lightcraft_catalog::safe_file::{write_atomic, write_atomic_nosync};
|
||||
use std::time::{Duration, Instant};
|
||||
let root = std::env::var_os("LIGHTCRAFT_BENCH_DIR").map(PathBuf::from).unwrap_or_else(std::env::temp_dir);
|
||||
let base = root.join(format!("lc-write-policy-{}", std::process::id()));
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
std::fs::create_dir_all(&base).unwrap();
|
||||
let data = |n: usize, seed: u32| -> Vec<u8> { (0..n as u32).map(|i| (i.wrapping_add(seed).wrapping_mul(2_654_435_761) >> 11) as u8).collect() };
|
||||
let (n, size) = (200usize, 300 << 10);
|
||||
let (mut synced, mut unsynced) = (Duration::ZERO, Duration::ZERO);
|
||||
for i in 0..n {
|
||||
let b = data(size, i as u32);
|
||||
let t = Instant::now();
|
||||
write_atomic(&base.join(format!("sync-{i}.jpg")), &b).unwrap();
|
||||
synced += t.elapsed();
|
||||
let t = Instant::now();
|
||||
write_atomic_nosync(&base.join(format!("nosync-{i}.jpg")), &b).unwrap();
|
||||
unsynced += t.elapsed();
|
||||
}
|
||||
println!("export {n} × {} KB: write_atomic (synced) {synced:?}, write_atomic_nosync {unsynced:?}", size >> 10);
|
||||
let (n, size) = (40usize, 8 << 20);
|
||||
let card = base.join("card");
|
||||
std::fs::create_dir_all(&card).unwrap();
|
||||
let hashes: Vec<_> = (0..n)
|
||||
.map(|i| {
|
||||
let b = data(size, 7 * i as u32);
|
||||
std::fs::write(card.join(format!("{i}.raw")), &b).unwrap();
|
||||
lightcraft_preview::hash_bytes(&b)
|
||||
})
|
||||
.collect();
|
||||
let (mut bytewise, mut hashed) = (Duration::ZERO, Duration::ZERO);
|
||||
for (i, h) in hashes.iter().enumerate() {
|
||||
let src = card.join(format!("{i}.raw"));
|
||||
let t = Instant::now();
|
||||
copy_verified(&src, &base.join(format!("bytes-{i}.raw")), None).unwrap();
|
||||
bytewise += t.elapsed();
|
||||
let t = Instant::now();
|
||||
copy_verified(&src, &base.join(format!("hash-{i}.raw")), Some(*h)).unwrap();
|
||||
hashed += t.elapsed();
|
||||
}
|
||||
println!("copy {n} × {} MB: byte compare with the source {bytewise:?}, probe hash {hashed:?}", size >> 20);
|
||||
let _ = std::fs::remove_dir_all(&base);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -114,7 +114,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins.
|
|||
| LR-IMP-ADD-DIALOG | Add photos/folders | P0 | ✅ | `cmd:file.addPhotos`, `cmd:library.importPreview`, `cmd:library.import` (`mode`, `album`, `albumName`, `preset`, `keywords`), `crates/ui-egui/src/import.rs` | review dialog (Import Photos): the scanned source (a scanned folder is not added to Local), candidate grid with thumbnails and checkboxes (duplicates by path/content marked and unchecked), transfer mode (add in place / copy / move, each explained), album (existing/new), preset, keywords; the folder scan runs on a background thread with a progress window and Cancel (a NAS folder no longer freezes the window); the import itself (probing, sidecars, copying / moving files) runs on a worker thread and its batches join the library between frames, with a progress window and Cancel; one undo step. Drag-and-drop goes through the same background import |
|
||||
| LR-IMP-DRAGDROP | Drop files/folders to import | P0 | ✅ | `crates/ui-egui/src/lib.rs` (dropped files → `cmd:library.import`) | dropping onto a specific album not supported |
|
||||
| LR-IMP-DUPES | Skip duplicates by content | P1 | ✅ | `crates/engine/src/import.rs`, `crates/engine/src/tests_import.rs` | |
|
||||
| LR-IMP-DEVICE | Import from camera/card | P1 | ✅ | `cmd:library.devices`, `cmd:file.addFromDevice`, `crates/engine/src/devices.rs` | mounted volumes with a DCIM folder (macOS /Volumes, Linux /media and /run/media, Windows drive letters); File → Import from Device → the import review, copying into the library by default; menus serve the last scan and rescan in the background (never blocking a frame; hot-plugs repaint); copies are verified like Move's (a new file, synced, compared byte for byte; a bad copy is removed and reported as failed, never counted as imported; taken names get -1, -2…, never replaced — issue #96, `crates/engine/src/import_move.rs`); no PTP/MTP (cameras that don't mount as a disk) |
|
||||
| LR-IMP-DEVICE | Import from camera/card | P1 | ✅ | `cmd:library.devices`, `cmd:file.addFromDevice`, `crates/engine/src/devices.rs` | mounted volumes with a DCIM folder (macOS /Volumes, Linux /media and /run/media, Windows drive letters); File → Import from Device → the import review, copying into the library by default; menus serve the last scan and rescan in the background (never blocking a frame; hot-plugs repaint); copies are verified (a new file, synced, checked against the content hash the scan computed — byte compare with the source when there is none, issue #134; a bad copy is removed and reported as failed, never counted as imported; taken names get -1, -2…, never replaced — issue #96, `crates/engine/src/import_move.rs`); no PTP/MTP (cameras that don't mount as a disk) |
|
||||
| LR-IMP-AUTO | Watched-folder auto import | P2 | ✅ | `cmd:library.autoImport`, `cmd:library.autoImportScan`, `crates/ui-egui/src/panels/settings.rs` | Settings ▸ Import ▸ Auto Import: a watched folder whose new photos are added (in place or copied) once complete, into an optional album; scanned every 3 s |
|
||||
| LR-IMP-PRESET | Preset on import | P2 | ✅ | `cmd:library.import` (`preset`) | chosen in the import review; one History entry |
|
||||
| LR-IMP-RAWDEFAULT | Raw defaults | P1 | ✅ | `cmd:library.preferences`, `crates/engine/src/import.rs`, `crates/ui-egui/src/panels/settings.rs` | LightCraft default / a preset / per camera (make + model); non-raw default too; the preset look counts as unedited and Reset returns to it |
|
||||
|
|
@ -434,7 +434,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins.
|
|||
| LR-EXP-METADATA | Metadata policy | P1 | ✅ | `cmd:app.export` (`metadata`, `removeLocation`) | |
|
||||
| LR-EXP-WATERMARK | Watermark | P1 | ✅ | `cmd:app.export` (`watermark`: text or `image` + `imageWidth`), `crates/engine/src/export.rs` (`Watermark`) | text (size, colour, shadow) or a graphic with transparency (width as % of the photo, converted to the output colour space); position, inset, opacity |
|
||||
| LR-EXP-NAMING | File naming | P1 | ✅ | `cmd:app.export` (`naming`, `startNumber`), `crates/engine/src/rename.rs` (`expand_tokens`) | one template language for export, Rename Photos and import renaming: `{name}`, `{num}` (number at the end of the name), `{seq}` / `{seq:N}`, `{date}` / `{date:%Y-%m-%d}`, `{folder}`, `{camera}`, `{lens}`, `{iso}`, `{rating}`, `{title}`, `{creator}`, `{ext}`; free-form template rather than a list of named schemes |
|
||||
| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `cmd:export.checkTarget`, `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker), `crates/engine/src/originals.rs`, `crates/catalog/src/safe_file.rs` | folder picker on the desktop; existing files get `-2`, `-3`… by default; the policy covers a file and its sidecars together; a catalogued original (or its sidecar) is never written over, whatever the policy or an exact `path` (export, `ui.render`, MCP `render_photo`, merge `previewPath`, `lightcraft-cli render`); files are written to a temp file, synced and renamed into place (issue #93) |
|
||||
| LR-EXP-LOCATION | Destination folder | P0 | ✅ | `cmd:app.export` (`dir`, `subfolder`, `conflict`: unique / overwrite / skip), `cmd:export.checkTarget`, `crates/ui-egui/src/panels/dialogs.rs` (Choose… folder picker), `crates/engine/src/originals.rs`, `crates/catalog/src/safe_file.rs` | folder picker on the desktop; existing files get `-2`, `-3`… by default; the policy covers a file and its sidecars together; a catalogued original (or its sidecar) is never written over, whatever the policy or an exact `path` (export, `ui.render`, MCP `render_photo`, merge `previewPath`, `lightcraft-cli render`); files are written to a temp file and renamed into place (issue #93; not synced file by file — exports can be made again, issue #134) |
|
||||
| LR-EXP-PREVIOUS | Export with previous settings | P0 | ✅ | `cmd:app.exportPrevious`, `cmd:dialog.export` | last options persist in prefs.json; dialog prefilled; no named export presets yet |
|
||||
| LR-EXP-DNGOPT | DNG options | P2 | 🟡 | `cmd:app.export` (`dngCompression`), `crates/engine/src/export.rs` | compression: lossless JPEG (default), ZIP or none; no embedded JPEG preview size, no lossy DNG output |
|
||||
| LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`), `crates/engine/src/tests_export.rs` | file copied byte for byte, sidecar named after the output and subject to the conflict policy |
|
||||
|
|
|
|||
|
|
@ -14,18 +14,25 @@
|
|||
the same name and "Overwrite" (or Export with Previous repeating it), an exact output path from the control
|
||||
channel or MCP, a merge preview path or `lightcraft-cli render IMG.jpg -o IMG.jpg` is refused with a clear
|
||||
message, and the original is left byte for byte. Ordinary earlier exports are still overwritten when asked.
|
||||
Exported files are written to a temp file, synced and then renamed into place, so a full disk or an unplugged
|
||||
drive never leaves a truncated file; the XMP sidecar of an "Original" export follows the "If file exists"
|
||||
choice too.
|
||||
Exported files are written to a temp file and then renamed into place, so a full disk or an unplugged drive
|
||||
never leaves a truncated file; the XMP sidecar of an "Original" export follows the "If file exists" choice too.
|
||||
- Convert to DNG, Copy as DNG, Photo Merge and smart previews no longer write straight to the final file (issue
|
||||
#106): a DNG is checked against the raw data, written to a temp file, synced and read back before it gets its
|
||||
name (never replacing a file), and only then is the photo relinked or the raw copy removed — a failed write
|
||||
leaves no DNG and keeps the raw. Smart previews are written the same way; a damaged one (cut short by a crash or
|
||||
a full drive) no longer counts as built and Build Smart Previews replaces it.
|
||||
- Import ▸ Copy verifies every copy, like Move (issue #96): each file is written as a new file, synced to disk and
|
||||
compared byte for byte with the card. A copy that fails or differs is removed and reported as a failed import —
|
||||
so "import complete" means the copies are good before you format the card — and a name that is taken gets -1,
|
||||
-2… instead of being replaced.
|
||||
checked against the content read from the card. A copy that fails or differs is removed and reported as a failed
|
||||
import — so "import complete" means the copies are good before you format the card — and a name that is taken
|
||||
gets -1, -2… instead of being replaced.
|
||||
- Faster exports and card imports, with the same protection for what can't be recreated (issue #134): exports,
|
||||
renders and screenshots are still written to a temp file and renamed into place, but no longer forced to disk
|
||||
one by one — they can always be exported again, and on a USB drive or a NAS that per-file sync dominated a large
|
||||
export. The catalog, XMP sidecars, settings, DNGs, merges, smart previews and the copies an import makes are still
|
||||
synced. Import ▸ Copy now checks each copy against the content hash taken while scanning the card instead of
|
||||
reading the card a third time (Move still compares byte for byte before it deletes a source); a file that changed
|
||||
on the card after Review Import is reported instead of being imported with stale details. Checking an export path
|
||||
against the library's originals no longer scans the whole library when nothing is at that path.
|
||||
- Saving metadata to an XMP sidecar another application wrote no longer replaces it (issue #92): LightCraft merges its
|
||||
fields in and keeps the rest — e.g. that application's develop settings and edit history — byte for byte. A
|
||||
sidecar that isn't valid XMP is copied to `<name>.xmp.bak-<time>` first. With the default stem naming, a raw and a
|
||||
|
|
|
|||
Loading…
Reference in New Issue