From cabe820798917ac9919264d62ba07195a61a8647 Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Fri, 30 Oct 2026 10:53:40 -0400 Subject: [PATCH 1/4] fix(locallibrary): drive safety for jobs, backups, racy stamps, player names, My Tags and exports Planned as these commits, folded into one because they share files: - fix(locallibrary): plan a job's step again only over the job's own writes - fix(locallibrary): carry the journal and player name in backups - fix(locallibrary): tell racy size-and-mtime matches by content - fix(locallibrary): leave a stick without onelibrary unnamed - fix(locallibrary): check my tag ids against exportext.pdb when planning - fix(locallibrary): count cloud downloads in the export's space check - fix(locallibrary): name skipped history lists, merge into the same folder, hide disks under the test switch --- apps/locallibrary/CHANGELOG.md | 25 ++++ apps/locallibrary/TODO.md | 22 +-- apps/locallibrary/docs/rpc/locallibrary.d.ts | 11 +- apps/locallibrary/docs/rpc/schema.json | 13 +- crates/locallibrary/src/after_set.rs | 21 ++- crates/locallibrary/src/api.rs | 4 +- crates/locallibrary/src/api/jobs.rs | 77 +++++++++- crates/locallibrary/src/api/jobs/tests.rs | 120 ++++++++++++++++ crates/locallibrary/src/api/undo.rs | 8 +- crates/locallibrary/src/artwork.rs | 22 ++- crates/locallibrary/src/backup.rs | 85 +++++++++-- crates/locallibrary/src/collection_export.rs | 33 ++++- .../src/collection_export/tests.rs | 29 ++++ crates/locallibrary/src/commit.rs | 21 ++- crates/locallibrary/src/edit.rs | 35 ++++- crates/locallibrary/src/format/mod.rs | 5 +- crates/locallibrary/src/format/tests.rs | 20 +++ crates/locallibrary/src/fsutil.rs | 54 ++++++- crates/locallibrary/src/fsutil/tests.rs | 50 +++++++ crates/locallibrary/src/library_write.rs | 55 +++++-- crates/locallibrary/src/merge.rs | 73 ++++++++-- crates/locallibrary/src/op.rs | 12 +- crates/locallibrary/src/pack.rs | 3 + crates/locallibrary/src/player.rs | 27 +++- crates/locallibrary/src/scrub.rs | 14 +- crates/locallibrary/src/sync.rs | 57 +++++++- crates/locallibrary/src/tags.rs | 17 ++- crates/locallibrary/src/tags/tests.rs | 8 +- crates/locallibrary/src/tree.rs | 19 ++- crates/locallibrary/src/tree/tests.rs | 26 ++++ crates/locallibrary/src/watch/mod.rs | 23 +-- crates/locallibrary/tests/it/artwork/embed.rs | 24 ++++ crates/locallibrary/tests/it/drives.rs | 41 ++++++ .../tests/it/edit/journal_replay.rs | 135 ++++++++++++++++++ crates/locallibrary/tests/it/edit/refusals.rs | 2 +- crates/locallibrary/tests/it/merge.rs | 25 ++++ .../locallibrary/tests/it/merge/matching.rs | 7 +- crates/locallibrary/tests/it/pack/files.rs | 32 +++++ crates/locallibrary/tests/it/sync/compare.rs | 30 ++++ crates/locallibrary/tests/it/tags.rs | 28 ++++ 40 files changed, 1166 insertions(+), 147 deletions(-) diff --git a/apps/locallibrary/CHANGELOG.md b/apps/locallibrary/CHANGELOG.md index 8d24d77c5..33e9bdc79 100644 --- a/apps/locallibrary/CHANGELOG.md +++ b/apps/locallibrary/CHANGELOG.md @@ -776,6 +776,31 @@ their old names. `older_than_store` error leaves out `fields.snapshot` when no snapshot holds the state). `status`, `drive ls --known` and the player say such a drive that isn't plugged in is behind, where they said it had changes not in the store. +- `refresh` and `prep` plan a step again only when the drive is as the job's own earlier steps + left it. Anything else that changed it since the plan was confirmed (a rekordbox export + mid-job) stops the job with the stale plan, as a single apply does, instead of being planned + over and applied unconfirmed. +- A backup carries the drive's recorded edits and the player name and colour it remembers, and + putting one back puts them back too: undo then redo, or an older backup then a newer one, + leaves `edits replay` and `refresh` knowing exactly the edits and name the drive has. The undo + plan says how many recorded edits go back on the journal. A replay cut off part way notes + what each step left in its backup, so it's undone without `--force`. +- A file copied, hashed or snapshotted in the second or two after it was written is no longer + taken as unchanged by its size and modification time alone (FAT32 keeps those to 2 s, so a + rewrite then can look the same): sync, pack, collection export and the after-a-set changes + compare it by content, and taking embedded art back out checks each file by content. +- A sync or pack onto a stick with no `exportLibrary.db` leaves it without a player name and + colour, where it took the source's, and the plan says so. +- A My Tag category numbered past what `exportExt.pdb` holds in a OneLibrary database no longer + makes an edit that plans cleanly and fails as it's applied: `exportExt.pdb` gets a free id, and + an id that can't fit is refused when planning. +- `collection export` counts the online-only files it downloads in its space check when their + cloud folder is on the target's disk, and says which file it's waiting on when one takes more + than ten seconds to download. +- A replay that skips deleting history lists none of which are on the drive names them. + Merging the same drive into a folder again adds nothing, where it made a "(2)" copy of every + playlist. Under `LOCALLIBRARY_VOLUMES`, the disk holding a drive folder isn't looked up with + `diskutil` or `lsblk`. ### Security diff --git a/apps/locallibrary/TODO.md b/apps/locallibrary/TODO.md index cfb4dd0df..e908630c5 100644 --- a/apps/locallibrary/TODO.md +++ b/apps/locallibrary/TODO.md @@ -45,9 +45,6 @@ Open work left by the 2026-10-11 and 2026-10-12 sessions was added on 2026-10-12 `cargo about` in `locallibrary-release.yml`, plus rekordbox-pdb's MIT notice for the vendored code and `crates/locallibrary/src/convert/template.pdb`. Keep the task here or in CONTRIBUTING.md's release checklist, not both. -- [ ] Jobs (`refresh`, `prep`) re-plan later steps on any `StalePlan`, including a rekordbox export - mid-job, which breaks `op.rs`'s promise that the confirmed plan is what runs. Re-plan only - drives this job already wrote; refuse otherwise. - [ ] `clean` backs up only the databases. It deletes only rekordbox's demo samples, which every export brings back, yet `refresh` runs it after every export, so audio backups pile up and a full drive can't be cleaned at all. @@ -179,15 +176,7 @@ Open work left by the 2026-10-11 and 2026-10-12 sessions was added on 2026-10-12 - [ ] Found while raising coverage, each a decision: `drive tidy` exits 0 when it couldn't remove some files (`recordings` says Partial); `setup --role` is ignored for the drive that becomes - the primary, even a bad value; a lenient replay that skips `DeleteHistoryLists` only says - "not found", dropping which list; merging the same drive twice in folder mode makes "(2)" - copies of every playlist; under `LOCALLIBRARY_VOLUMES`, `format::disk_at` still runs - read-only `diskutil info`/`lsblk` on the disk holding the test folder. -- [ ] Redo: the backup an undo makes doesn't carry `edits.json` or the remembered player state, so - undo then redo leaves edits that replay won't redo, and restoring an older backup then a - newer one puts back edits the journal has forgotten. -- [ ] A replay cut off by a crash leaves its backup without `after`: running the replay again - finishes it, but taking it back needs `undo --force`. + the primary, even a bad value. - [ ] `prep` syncs one pair at a time; it could use the parallel multi-drive sync when no drive came back from a set. - [ ] Nothing warns when a command writes to a store copy opened with `--store`; the next `store @@ -227,17 +216,14 @@ Open work left by the 2026-10-11 and 2026-10-12 sessions was added on 2026-10-12 `warnings`. - [ ] `cue_edit.rs` sums its backup's size itself; it could use `backup::needed_bytes` like pack and clean (its sum also counts staged analysis files). -- [ ] A sync or pack onto a stick with no `exportLibrary.db` takes on the source's player name and - colour, since there's none to keep. For set sticks, clearing it may be better. - [ ] Type-check `docs/rpc/locallibrary.d.ts` with `tsc` in CI once something consumes the RPC. - [ ] Apple signing secrets (`APPLE_CERTIFICATE`, `APPLE_CERTIFICATE_PASSWORD`, `APPLE_SIGNING_IDENTITY`, `APPLE_API_KEY`, `APPLE_API_KEY_ID`, `APPLE_API_ISSUER`) need a paid Developer ID; the shell installer and Homebrew run an unsigned CLI fine. Windows binaries are unsigned too (dist supports Azure Artifact Signing). -- [ ] `collection export`'s space check counts `written.needed_bytes` but not `download_bytes` - (online-only Dropbox files that download onto the boot disk): both the fit warning - (`collection_export.rs:276`) and `apply`'s `NoSpace` check (`:473`) miss them. A full export - to `~/rekordbox-export` stopped at 1.8 of 25 GB. +- [ ] `collection export` copies online-only files one at a time, so one that takes minutes to + download stalls the export (it now says which file it's waiting on). Start the next few + downloads while one copies (reading a file's first byte is enough). - [ ] The sidecar's `sets.export` makes a store (`c.store()?`, `rpc/methods.rs:741`), while the CLI's `sets export` reads with `store_to_read`. - [ ] `store_to_read` and `existing_store` (`diff_cmd.rs`) drop the "opening store at ..." context diff --git a/apps/locallibrary/docs/rpc/locallibrary.d.ts b/apps/locallibrary/docs/rpc/locallibrary.d.ts index 1382648d6..8162612a3 100644 --- a/apps/locallibrary/docs/rpc/locallibrary.d.ts +++ b/apps/locallibrary/docs/rpc/locallibrary.d.ts @@ -3557,6 +3557,12 @@ export type EmbeddedFile = { /** The file once the art went in: it's only put back while it's still that. */ size: number; mtime: number; + /** + * What it held once the art went in, by content: its size and mtime, taken moments after it + * was written, can't tell a rewrite in the same second or two (see [`fsutil::settled`]). + * Missing from embeds an earlier version made, which go by size and mtime alone. + */ + hash?: Hash | null; }; export type Comparison = { @@ -7743,8 +7749,9 @@ export type CollectionExportPlan = { */ unsaved: Unsaved | null; /** - * Space the export needs on the target: the files it copies, its new databases and the - * backup of what it replaces. + * Space the export needs on the target: the files it copies, its new databases, the backup + * of what it replaces, and the online-only files it downloads when their cloud folder is on + * the same disk. */ needed_bytes: number; free_space: number | null; diff --git a/apps/locallibrary/docs/rpc/schema.json b/apps/locallibrary/docs/rpc/schema.json index 14e428fb8..b23fc73e9 100644 --- a/apps/locallibrary/docs/rpc/schema.json +++ b/apps/locallibrary/docs/rpc/schema.json @@ -15734,6 +15734,17 @@ "mtime": { "type": "integer", "format": "int64" + }, + "hash": { + "anyOf": [ + { + "$ref": "#/$defs/Hash" + }, + { + "type": "null" + } + ], + "description": "What it held once the art went in, by content: its size and mtime, taken moments after it\nwas written, can't tell a rewrite in the same second or two (see [`fsutil::settled`]).\nMissing from embeds an earlier version made, which go by size and mtime alone." } }, "required": [ @@ -29982,7 +29993,7 @@ "type": "integer", "format": "uint64", "minimum": 0, - "description": "Space the export needs on the target: the files it copies, its new databases and the\nbackup of what it replaces." + "description": "Space the export needs on the target: the files it copies, its new databases, the backup\nof what it replaces, and the online-only files it downloads when their cloud folder is on\nthe same disk." }, "free_space": { "type": [ diff --git a/crates/locallibrary/src/after_set.rs b/crates/locallibrary/src/after_set.rs index 51f904143..20d16d2ce 100644 --- a/crates/locallibrary/src/after_set.rs +++ b/crates/locallibrary/src/after_set.rs @@ -25,9 +25,10 @@ use crate::Result; use crate::device::Device; use crate::doctor::Databases; use crate::fsutil; +use crate::hash; use crate::model::{Library, LibraryDiff}; use crate::paths::FileClass; -use crate::progress::Ctx; +use crate::progress::{Ctx, Phase}; use crate::store::{Snapshot, SnapshotInfo, Store}; /// The library databases (their changes are judged through the library diff). @@ -118,16 +119,26 @@ pub fn changes_since(store: &Store, device: &Device, ancestor: &Snapshot, ctx: & let old_files: FxHashMap = ancestor.files.iter().map(|f| (f.path.to_lowercase(), f)).collect(); let mut seen = FxHashSet::default(); + let pulled = ancestor.created.timestamp(); for e in scanned.tree.library_entries() { let key = e.path.to_lowercase(); seen.insert(key.clone()); let old = old_files.get(&key); let changed = match old { None => true, - Some(o) => match (o.hash, e.hash) { - (Some(a), Some(b)) => a != b, - _ => o.size != e.size || !fsutil::mtime_eq(o.mtime, e.mtime), - }, + Some(o) => { + let stamped = o.size == e.size && fsutil::mtime_eq(o.mtime, e.mtime); + match (o.hash, e.hash) { + (Some(a), Some(b)) => a != b, + // Written moments before the snapshot, it may have changed since without its + // size or mtime showing it: by content. + (Some(a), None) if stamped && !fsutil::settled(o.mtime, pulled) => { + let path = scanned.tree.fs_path(&e.path); + !hash::hash_file(&path, ctx, Phase::Hash).is_ok_and(|now| now == a) + } + _ => !stamped || !fsutil::settled(o.mtime, pulled), + } + } }; if !changed { continue; diff --git a/crates/locallibrary/src/api.rs b/crates/locallibrary/src/api.rs index 518534ccf..e2c537e03 100644 --- a/crates/locallibrary/src/api.rs +++ b/crates/locallibrary/src/api.rs @@ -1896,9 +1896,7 @@ pub fn artwork_unembed_plan(store: &Store, req: &UnembedRequest) -> Result { - restored.push(f.path.clone()) - } + Ok(m) if f.unchanged(&m) => restored.push(f.path.clone()), Ok(_) => skipped.push(Skip::new(f.path.display().to_string(), "changed since the art went in")), Err(_) => skipped.push(Skip::new(f.path.display().to_string(), "gone")), } diff --git a/crates/locallibrary/src/api/jobs.rs b/crates/locallibrary/src/api/jobs.rs index 70fff9de7..fc2dd81ad 100644 --- a/crates/locallibrary/src/api/jobs.rs +++ b/crates/locallibrary/src/api/jobs.rs @@ -6,9 +6,10 @@ //! runs, made by the same `*_plan` functions the single commands use, so one confirmation covers //! them all. Applying runs the steps in order, each operation through its own apply and so //! through the shared write routine. An operation whose drive an earlier step changed is planned -//! again on the drive as that step left it; the steps that only make sense after the others (the -//! pull and the sync) are planned as they run. The first step that fails stops the job, and the -//! report says what was done ([`JobReport::stopped`]). +//! again on the drive as that step left it, but only when nothing else changed it since: an +//! outside write (a rekordbox export mid-job) stops the job with the stale plan. The steps that +//! only make sense after the others (the pull and the sync) are planned as they run. The first +//! step that fails stops the job, and the report says what was done ([`JobReport::stopped`]). use std::collections::BTreeSet; @@ -34,6 +35,7 @@ use crate::edit::{self, Edit, EditOptions}; use crate::eject::EjectReport; use crate::error::{Action, Reason}; use crate::fleet::{self, Role}; +use crate::hash::Hash; use crate::local_time; use crate::op::{self, AnyPlanned, CloneTarget, Operation, Planned, Touched}; use crate::progress::{Ctx, Steps}; @@ -363,12 +365,15 @@ fn run_job(plan: &JobPlan, store: Option<&Store>, ctx: &Ctx<'_>) -> Result, ctx: &Ctx<'_>) -> Result { + /// The plans still to apply, with the states of what each touches as the job last left them + /// (`None` once something other than the job changed them). + plans: Vec<(&'p dyn AnyPlanned, Option>)>, +} + +impl<'p> Left<'p> { + fn new(plan: &'p JobPlan) -> Left<'p> { + let plans = plan + .running() + .flat_map(|s| &s.work) + .filter_map(|w| match w { + Work::Op { plan: Some(p), .. } => Some(p.as_ref()), + _ => None, + }) + .map(|p| (p, Some(p.drives().iter().map(Touched::planned_state).collect()))) + .collect(); + Left { plans } + } + + fn measure(plan: &dyn AnyPlanned) -> Vec { + plan.drives().iter().map(Touched::measured).collect() + } + + /// Before a piece of work: whatever changed since the job last looked, it didn't change. + fn before(&mut self) { + for (p, left) in &mut self.plans { + if left.as_ref().is_some_and(|l| *l != Self::measure(*p)) { + *left = None; + } + } + } + + /// After `w`: what it left, for the plans still to apply. + fn after(&mut self, w: &Work) { + if let Work::Op { plan: Some(done), .. } = w { + self.plans.retain(|(p, _)| !std::ptr::addr_eq(*p, done.as_ref())); + } + for (p, left) in &mut self.plans { + if left.is_some() { + *left = Some(Self::measure(*p)); + } + } + } + + /// Whether everything `plan` touches is as the job's own work left it. + fn ours(&self, plan: &dyn AnyPlanned) -> bool { + let left = self.plans.iter().find(|(p, _)| std::ptr::addr_eq(*p, plan)).and_then(|(_, l)| l.as_ref()); + left.is_some_and(|l| *l == Self::measure(plan)) + } +} + /// Run one piece of a step. `Ok(false)` stops the job there (the check found problems). fn run_work( w: &Work, @@ -402,12 +463,16 @@ fn run_work( store: Option<&Store>, done: &mut StepReport, report: &mut JobReport, + left: &Left<'_>, ctx: &Ctx<'_>, ) -> Result { match w { Work::Op { plan: Some(plan), again } => match apply_op(plan.as_ref(), step, store, done, ctx) { - // An earlier step changed the drive: the same operation, on the drive as it is now. - Err(Error::StalePlan(_)) => apply_op(again(store, ctx)?.as_ref(), step, store, done, ctx)?, + // An earlier step of this job changed the drive: the same operation, on the drive as + // that step left it. + Err(Error::StalePlan(_)) if left.ours(plan.as_ref()) => { + apply_op(again(store, ctx)?.as_ref(), step, store, done, ctx)? + } r => r?, }, Work::Op { plan: None, again } => apply_op(again(store, ctx)?.as_ref(), step, store, done, ctx)?, diff --git a/crates/locallibrary/src/api/jobs/tests.rs b/crates/locallibrary/src/api/jobs/tests.rs index c217eb143..afe895164 100644 --- a/crates/locallibrary/src/api/jobs/tests.rs +++ b/crates/locallibrary/src/api/jobs/tests.rs @@ -225,6 +225,126 @@ fn a_job_runs_its_steps_until_one_fails(tc: hegel::TestCase) { assert_eq!(planned.noop, running.is_empty()); } +/// A pretend operation on files in a folder: its plan touches `touches` (by content), and +/// applying it writes `writes`. `outside` is written just before it's applied, as a rekordbox +/// export landing mid-job would be. +struct OnDisk { + summary: Vec, + drives: Vec, + writes: Option, + outside: Option, +} + +impl OnDisk { + fn plan(dir: &Path, line: &str, touches: &[&str], writes: Option<&str>, outside: Option<&str>) -> OnDisk { + let keys = touches.iter().map(|k| k.to_string()).collect(); + OnDisk { + summary: vec![line.to_string()], + drives: vec![Touched::path(dir, true, vec![op::Probe::Content(keys)])], + writes: writes.map(|f| dir.join(f)), + outside: outside.map(|f| dir.join(f)), + } + } +} + +impl AnyPlanned for OnDisk { + fn id(&self) -> &str { + "on-disk" + } + fn operation(&self) -> &'static str { + "on-disk" + } + fn summary(&self) -> &[String] { + &self.summary + } + fn warnings(&self) -> &[String] { + &[] + } + fn destructive(&self) -> bool { + false + } + fn is_noop(&self) -> bool { + false + } + fn drives(&self) -> &[Touched] { + &self.drives + } + fn planned_at(&self) -> chrono::DateTime { + chrono::Utc::now() + } + fn to_json(&self) -> Value { + serde_json::json!({ "summary": self.summary }) + } + fn check(&self, _: Option<&Store>) -> Result<()> { + Ok(()) + } + fn apply_json(&self, _: Option<&Store>, _: &Ctx<'_>) -> Result { + if let Some(f) = &self.outside { + std::fs::write(f, "exported by rekordbox").unwrap(); + } + if let Some(t) = self.drives.iter().find(|t| t.changed()) { + return Err(Error::stale(&t.name, "changed")); + } + if let Some(f) = &self.writes { + std::fs::write(f, "written by the job").unwrap(); + } + Ok(serde_json::json!({ "done": self.summary })) + } + fn as_any(&self) -> &dyn std::any::Any { + self + } +} + +/// Regression: a job planned again any step refused as stale, so a rekordbox export that landed +/// mid-job was planned over and applied without being confirmed. A step is planned again only +/// when the job's own earlier steps changed what it touches; any other change stops the job +/// with the stale plan, whether it lands before the job starts, between steps, or along with +/// the job's own write. +#[test] +fn a_job_plans_again_only_over_its_own_writes() { + /// What the later step's plan touches, written from outside before the job, written from + /// outside as the later step applies, and whether the job goes through. + type Case = (&'static [&'static str], Option<&'static str>, Option<&'static str>, bool); + let cases: [Case; 4] = [ + (&["a"], None, None, true), + (&["a"], None, Some("a"), false), + (&["a", "b"], Some("b"), None, false), + (&["b"], None, Some("b"), false), + ]; + for (touches, before, during, goes) in cases { + let dir = tempfile::tempdir().unwrap(); + std::fs::write(dir.path().join("a"), "as exported").unwrap(); + let mut first = JobStep::new(StepKind::Clean); + let plan = OnDisk::plan(dir.path(), "wrote a", &["a"], Some("a"), None); + first.op(Box::new(plan), Box::new(|_, _| unreachable!("the first step isn't stale"))); + let mut later = JobStep::new(StepKind::Replay); + let plan = OnDisk::plan(dir.path(), "replayed", touches, None, during); + let again_dir = dir.path().to_path_buf(); + let again: Replan = Box::new(move |_, _| { + Ok(Box::new(OnDisk::plan(&again_dir, "replayed again", &[], None, None)) as Box) + }); + later.op(Box::new(plan), again); + let plan = JobPlan::new(Vec::new(), vec![first, later], std::iter::empty::<&Device>()); + if let Some(f) = before { + std::fs::write(dir.path().join(f), "exported before the job").unwrap(); + } + let report = run_job(&plan, None, &Ctx::silent()).unwrap(); + let case = format!("touches {touches:?}, before {before:?}, during {during:?}"); + assert_eq!(report.steps[0].done, ["wrote a"], "{case}"); + match goes { + true => { + assert!(report.stopped.is_none(), "{case}: {:?}", report.stopped); + assert_eq!(report.steps[1].done, ["replayed again"], "{case}"); + } + false => { + let stopped = report.stopped.as_ref().unwrap_or_else(|| panic!("{case}: went on")); + assert_eq!((stopped.step.as_str(), stopped.kind), ("replay", "stale_plan"), "{case}"); + assert_eq!(report.steps.len(), 1, "{case}"); + } + } + } +} + /// Ejecting drives one at a time says, for each ejected before the first that fails, whether it /// can be pulled out or only unmounted, with its report; the failure is the result. #[hegel::test(test_cases = 100)] diff --git a/crates/locallibrary/src/api/undo.rs b/crates/locallibrary/src/api/undo.rs index 5de98413c..19910865f 100644 --- a/crates/locallibrary/src/api/undo.rs +++ b/crates/locallibrary/src/api/undo.rs @@ -1,7 +1,7 @@ //! Undo: put back the backup of the last change locallibrary made to a drive that the drive //! still holds (see [`crate::backup::choose`]), after showing what that changes: the files and //! databases, the playlists and tracks, the history lists dropped, and the recorded edits taken -//! off the journal. Writers that change a drive in place back it up first (not repairs, firmware +//! off the journal or put back on it. Writers that change a drive in place back it up first (not repairs, firmware //! files, recordings or `drive tidy`'s deletions), so the last change can be taken back, and the //! one before it next. A drive that changed since (a rekordbox //! export, a sync, players at a set) is refused unless forced, as that would be lost too. @@ -144,6 +144,7 @@ pub fn undo_plan(req: &UndoRequest) -> Result> { let label = format!("backup {}", info.id); let diff = super::diff::to_backup(&d, &put_back, &label); let journal = journal::taken_off(&d, &info)?; + let journal_back = journal::put_back(&d, &info)?; // Players record history in either database: every list of the drive's the backup lacks. let kept = crate::plays::drive_history(&put_back).map(|h| h.lists).unwrap_or_default(); let history_lists: Vec = crate::plays::drive_history(&d) @@ -166,6 +167,9 @@ pub fn undo_plan(req: &UndoRequest) -> Result> { if !journal.is_empty() { summary.push(format!("{} recorded edit(s) come off the journal", journal.len())); } + if !journal_back.is_empty() { + summary.push(format!("{} recorded edit(s) go back on the journal", journal_back.len())); + } let mut warnings = Vec::new(); if !history_lists.is_empty() { warnings.push(format!( @@ -182,7 +186,7 @@ pub fn undo_plan(req: &UndoRequest) -> Result> { )); } let keys = info.files.clone(); - let noop = files.is_empty() && journal.is_empty(); + let noop = files.is_empty() && journal.is_empty() && journal_back.is_empty(); let plan = UndoPlan { drive: d.name(), root: d.root().into(), diff --git a/crates/locallibrary/src/artwork.rs b/crates/locallibrary/src/artwork.rs index b7c62d44c..370ec1c14 100644 --- a/crates/locallibrary/src/artwork.rs +++ b/crates/locallibrary/src/artwork.rs @@ -184,6 +184,24 @@ pub struct EmbeddedFile { /// The file once the art went in: it's only put back while it's still that. pub size: u64, pub mtime: i64, + /// What it held once the art went in, by content: its size and mtime, taken moments after it + /// was written, can't tell a rewrite in the same second or two (see [`fsutil::settled`]). + /// Missing from embeds an earlier version made, which go by size and mtime alone. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub hash: Option, +} + +impl EmbeddedFile { + /// Whether the file is still as the embed left it (`meta`: its metadata now). + pub(crate) fn unchanged(&self, meta: &std::fs::Metadata) -> bool { + if meta.len() != self.size { + return false; + } + match self.hash { + Some(h) => crate::hash::hash_file(&self.path, &Ctx::silent(), Phase::Verify).is_ok_and(|now| now == h), + None => fsutil::mtime_eq(fsutil::mtime_secs(meta), self.mtime), + } + } } /// Embeds kept, newest first. @@ -260,12 +278,14 @@ pub fn embed_with_backup( return Err(e); } let after = std::fs::metadata(f).at(f)?; + let hash = crate::hash::hash_file(f, &Ctx::silent(), Phase::Verify)?; Ok(EmbeddedFile { path: f.clone(), saved: saved.clone(), mtime_before: fsutil::mtime_secs(&before), size: after.len(), mtime: fsutil::mtime_secs(&after), + hash: Some(hash), }) })(); match result { @@ -315,7 +335,7 @@ pub fn restore_embedded(store: &Store, id: Option<&str>) -> Result { + Ok(m) if f.unchanged(&m) => { fsutil::copy_file(&dir.join(&f.saved), &f.path, Some(f.mtime_before), None, true, &Ctx::silent())?; report.restored.push(f.path.clone()); } diff --git a/crates/locallibrary/src/backup.rs b/crates/locallibrary/src/backup.rs index 78ab06b4b..eaebb582d 100644 --- a/crates/locallibrary/src/backup.rs +++ b/crates/locallibrary/src/backup.rs @@ -3,16 +3,19 @@ //! orphan removal, packs, and putting a backup back). They contain the database files plus every //! existing file the change overwrites or deletes (artwork images, analysis files, settings //! files, removed tracks' files), so restoring one undoes the change (`locallibrary undo`): files -//! that were added stay on the drive but are no longer referenced. Restoring one also takes the -//! edits it undoes off the drive's journal (see [`crate::edit::journal`]), so `edits replay` -//! doesn't make them again. +//! that were added stay on the drive but are no longer referenced. A backup also carries the +//! drive's journal of edits (see [`crate::edit::journal`]) and the player name and colour it +//! remembers ([`Carried`]), and restoring one puts those back as they were: `edits replay` then +//! makes again exactly the edits the restored library had, also after an undo is redone. Restoring +//! a backup an earlier version took, which carries neither, takes the edits it undoes off the +//! journal instead. //! //! Once its change is made, a backup notes what the change left in each file putting it back -//! would replace or remove ([`BackupInfo::after`]). Putting it back is refused, unless forced, -//! while the drive holds something else there (a rekordbox export, a sync or push, history lists -//! players recorded): that would be lost with the change ([`choose`]). Putting a backup back takes -//! a backup of what it replaces first, so an undo can itself be put back (a redo), and `undo` -//! again steps back to the change before. +//! would replace or remove ([`BackupInfo::after`]); a replay notes it after each of its steps. +//! Putting it back is refused, unless forced, while the drive holds something else there (a +//! rekordbox export, a sync or push, history lists players recorded): that would be lost with +//! the change ([`choose`]). Putting a backup back takes a backup of what it replaces first, so an +//! undo can itself be put back (a redo), and `undo` again steps back to the change before. //! //! Backups are numbered in the order they're made on the drive (computers' clocks differ), and //! the newest [`KEEP`] of each kind are kept. Kinds split two ways: backups that hold removed @@ -31,12 +34,14 @@ use serde::{Deserialize, Serialize}; use crate::commit::{self, Changes, Content, NewFile, Options}; use crate::device::{Device, LineageOp}; +use crate::edit::journal::{self, JournalEntry}; use crate::error::IoContext; use crate::error::{Action, Reason}; use crate::fsutil; use crate::hash::{self, Hash}; use crate::local_time; use crate::paths::{self, FileClass}; +use crate::player::DbIdentity; use crate::progress::{Ctx, Phase}; use crate::{Error, Result}; @@ -86,6 +91,58 @@ fn write_info(dir: &Path, info: &BackupInfo) -> Result<()> { fsutil::write_json_atomic(&dir.join("backup.json"), info) } +/// What a backup carries besides the drive's files, in `carried.json` beside `backup.json`: the +/// player name and colour the drive remembered (see [`crate::player`]). The drive's journal, with +/// the files its edits staged, is copied below the backup's own state folder, as on a drive. A +/// backup without it was taken by an earlier version. +#[derive(Debug, Default, Serialize, Deserialize)] +struct Carried { + #[serde(default, skip_serializing_if = "Option::is_none")] + player: Option, +} + +const CARRIED: &str = "carried.json"; + +/// Copy into the backup at `dir` what it carries (see [`Carried`]). A journal this build can't +/// read isn't carried: restoring refuses that drive anyway, as it can't change its journal. +fn carry(device: &Device, dir: &Path) -> Result<()> { + if journal::list(device).is_err() { + return Ok(()); + } + std::fs::create_dir_all(dir).at(dir)?; + journal::copy(device, &Device::open(dir)?)?; + let player = device.state().and_then(|s| s.player.clone()); + fsutil::write_json_atomic(&dir.join(CARRIED), &Carried { player }) +} + +fn carried(device: &Device, info: &BackupInfo) -> Result> { + fsutil::read_json_opt(&dir(device, &info.id).join(CARRIED)) +} + +/// The journal `info` carries (`None`: an earlier version took it, without one). +pub(crate) fn carried_journal(device: &Device, info: &BackupInfo) -> Result>> { + match carried(device, info)? { + Some(_) => Ok(Some(journal::list(&Device::open(dir(device, &info.id))?)?)), + None => Ok(None), + } +} + +/// Put back what `info` carries, now that its files are back: the journal and the player name and +/// colour as the drive had them. For a backup carrying neither, take the edits it undoes off the +/// journal, and remember the name and colour the drive shows now (`shown`: before). +fn put_back_carried(device: &mut Device, info: &BackupInfo, shown: Option) -> Result<()> { + let Some(carried) = carried(device, info)? else { + journal::undone(device, info)?; + return crate::player::after_undo(device, shown); + }; + journal::copy(&Device::open(dir(device, &info.id))?, device)?; + if device.state().and_then(|s| s.player.as_ref()) != carried.player.as_ref() { + device.state_mut()?.player = carried.player; + device.save_state()?; + } + Ok(()) +} + /// Copy the database files and `extra` (keys of files the coming change overwrites or deletes) /// into `.locallibrary/backups//`, with their modification times: a track put back must /// not look changed to the next sync. `undoes`: the backup the change puts back, if it's an @@ -118,6 +175,7 @@ pub(crate) fn create(device: &Device, reason: &str, extra: &[String], undoes: Op files.push(key); } } + carry(device, &dir)?; let info = BackupInfo { id, created, @@ -348,7 +406,7 @@ pub fn restore(device: &mut Device, id: Option<&str>, force: bool) -> Result, force: bool, ctx: &Ctx<'_>) -> Result { let _lock = commit::lock(device, None)?; // The journal must be one this build can change, as the edits undone come off it. - crate::edit::journal::list(device)?; + journal::list(device)?; let info = choose(device, id, force)?; let dir = backups_dir(device).join(&info.id); if !force { @@ -395,11 +453,12 @@ pub fn restore_with(device: &mut Device, id: Option<&str>, force: bool, ctx: &Ct let mut opts = Options::new(LineageOp::Restore, format!("restore backup {}", info.id)) .backup(format!("before putting back {}", info.id)); opts.undoes = Some(info.id.clone()); - // Read only when the drive remembers a player name, as reading it decrypts OneLibrary. - let shown = device.state().and_then(|s| s.player.as_ref()).and_then(|_| crate::player::db_identity(device.root())); + // Read only when the drive remembers a player name and the backup doesn't carry it, as + // reading it decrypts OneLibrary. + let remembers = device.state().is_some_and(|s| s.player.is_some()) && carried(device, &info)?.is_none(); + let shown = remembers.then(|| crate::player::db_identity(device.root())).flatten(); commit::commit(device, changes, opts, ctx)?; - crate::edit::journal::undone(device, &info)?; - crate::player::after_undo(device, shown)?; + put_back_carried(device, &info, shown)?; Ok(info) } diff --git a/crates/locallibrary/src/collection_export.rs b/crates/locallibrary/src/collection_export.rs index 76a6b93e7..2b072a784 100644 --- a/crates/locallibrary/src/collection_export.rs +++ b/crates/locallibrary/src/collection_export.rs @@ -149,8 +149,9 @@ pub struct CollectionExportPlan { /// What the target holds that the export replaces and only it has (history lists players /// recorded, changes since the last export): saved in the store first. pub unsaved: Option, - /// Space the export needs on the target: the files it copies, its new databases and the - /// backup of what it replaces. + /// Space the export needs on the target: the files it copies, its new databases, the backup + /// of what it replaces, and the online-only files it downloads when their cloud folder is on + /// the same disk. pub needed_bytes: u64, pub free_space: Option, pub fits: bool, @@ -356,6 +357,7 @@ pub(crate) fn plan_from( let mut written = library_write::plan(target, &scanned.tree, &layout.files, &[], databases, false); hash_checked(&mut written, &layout.checked, ctx)?; let downloads: Vec<&FileCopy> = written.copies.iter().filter(|c| layout.online_only.contains(&c.path)).collect(); + let (downloaded_here, needed_bytes, fits) = space(&written, &downloads, target.root()); // Without a record of an earlier export, whatever the target holds (a library, or loose audio // the export would remove) is only replaced when asked. let replaces_library = !exported && (target.has_export() || !written.deletes.is_empty()); @@ -390,10 +392,14 @@ pub(crate) fn plan_from( Some(source) => format!("writing {source}"), }; warnings.extend(edit::journal::forget_warning(target, None, &exporting)); - if !written.fits { + if !fits { + let downloads = match downloaded_here { + 0 => String::new(), + n => format!(", and {n} bytes of online-only files downloading onto the same disk"), + }; warnings.push(format!( - "the export needs about {} bytes (its files, databases and backup) but only {} are free on {}", - written.needed_bytes, + "the export needs about {needed_bytes} bytes (its files, databases and backup{downloads}) but only {} are \ + free on {}", written.free_space.unwrap_or(0), target.name() )); @@ -424,9 +430,9 @@ pub(crate) fn plan_from( replaces_library, made_folder: false, unsaved, - needed_bytes: written.needed_bytes, + needed_bytes, free_space: written.free_space, - fits: written.fits, + fits, warnings, library, tags, @@ -436,6 +442,19 @@ pub(crate) fn plan_from( }) } +/// The online-only `downloads` that download onto the target's disk (their cloud folder is on +/// it), the space the export needs with them, and whether that fits: the copies' space alone +/// leaves an export to a folder on the boot disk filling it as the cloud files come down. +fn space(written: &Planned, downloads: &[&FileCopy], target: &Path) -> (u64, u64, bool) { + let here: u64 = downloads + .iter() + .filter(|c| written.sources.get(&c.path).is_some_and(|s| fsutil::same_volume(&s.path, target))) + .map(|c| c.size) + .sum(); + let needed = written.needed_bytes + here; + (here, needed, written.free_space.is_none_or(|f| needed <= f)) +} + /// Hashes each audio file `written` copies that the plugin host checked (by key, `checked`), as /// it is now, for the copy to be checked against: one whose size changed since is refused. fn hash_checked(written: &mut Planned, checked: &FxHashSet, ctx: &Ctx<'_>) -> Result<()> { diff --git a/crates/locallibrary/src/collection_export/tests.rs b/crates/locallibrary/src/collection_export/tests.rs index 905bdfe26..7ca711ed8 100644 --- a/crates/locallibrary/src/collection_export/tests.rs +++ b/crates/locallibrary/src/collection_export/tests.rs @@ -617,3 +617,32 @@ fn an_export_plan_says_what_it_leaves_out_and_what_it_needs(tc: hegel::TestCase) assert_eq!(std::fs::read_dir(folder.root()).unwrap().count(), 0, "the drive is left as it was"); } } + +/// Regression: the space check left out the online-only files an export downloads, which land on +/// the disk their cloud folder is on, so an export to a folder on that disk filled it part way. +/// They count when the cloud folder shares the target's disk, and not when it doesn't. +#[cfg(unix)] +#[test] +fn downloads_onto_the_targets_disk_count_in_its_space() { + let dir = tempfile::tempdir().unwrap(); + let cloud = dir.path().join("Dropbox/track.mp3"); + let target = dir.path().join("rekordbox-export"); + let source = |path: &Path| library_write::Source { + path: path.to_path_buf(), + size: 700, + mtime: 0, + audio_path: None, + shift_ms: 0, + force: false, + hash: None, + }; + let copy = FileCopy { path: "Contents/track.mp3".into(), size: 700 }; + for (from, free, here) in [(&cloud, 1000, true), (&cloud, 1300, true), (&PathBuf::from("/dev/null"), 1000, false)] { + let mut written = Planned { needed_bytes: 600, free_space: Some(free), ..Default::default() }; + written.sources.insert(copy.path.clone(), source(from)); + let (downloaded, needed, fits) = space(&written, &[©], &target); + let want = if here { 700 } else { 0 }; + assert_eq!((downloaded, needed), (want, 600 + want), "{}", from.display()); + assert_eq!(fits, 600 + want <= free, "{} with {free} free", from.display()); + } +} diff --git a/crates/locallibrary/src/commit.rs b/crates/locallibrary/src/commit.rs index 003ca10dc..97a552ce1 100644 --- a/crates/locallibrary/src/commit.rs +++ b/crates/locallibrary/src/commit.rs @@ -25,7 +25,7 @@ //! before it is applied. Sync, push and pack copy whole libraries incrementally with their own //! staged copy, which a run that's stopped resumes ([`crate::sync`], [`crate::pack::apply`]). -use std::cell::Cell; +use std::cell::{Cell, RefCell}; use std::io::Write; use std::path::{Path, PathBuf}; @@ -44,6 +44,21 @@ use crate::{Error, Result, fsutil, paths}; thread_local! { static FAIL_BEFORE_SWAP: Cell = const { Cell::new(false) }; static FAIL_AFTER_SWAP: Cell = const { Cell::new(false) }; + static UNDER_BACKUP: RefCell> = const { RefCell::new(None) }; +} + +/// Run `f` with each write in it that takes no backup of its own noting what it left in backup +/// `id` (see [`backup::seal`]): a replay's steps, made under the one backup the replay took, so +/// a replay cut off part way can be undone without forcing it. +pub(crate) fn under_backup(id: &str, f: impl FnOnce() -> T) -> T { + struct Restore(Option); + impl Drop for Restore { + fn drop(&mut self) { + UNDER_BACKUP.with(|u| *u.borrow_mut() = self.0.take()); + } + } + let _restore = Restore(UNDER_BACKUP.with(|u| u.borrow_mut().replace(id.to_string()))); + f() } /// For tests: make every commit on this thread fail just before its swap, as a crash or a full @@ -380,8 +395,8 @@ pub(crate) fn commit(device: &mut Device, changes: Changes<'_>, opts: Options<'_ } // What the write left, the new state, the journal, and the backups kept. - if let Some(id) = &done.backup { - backup::seal(device, id)?; + if let Some(id) = done.backup.clone().or_else(|| UNDER_BACKUP.with(|u| u.borrow().clone())) { + backup::seal(device, &id)?; } if opts.lineage { let library = device.library()?; diff --git a/crates/locallibrary/src/edit.rs b/crates/locallibrary/src/edit.rs index 238a00caa..f3a1ef12f 100644 --- a/crates/locallibrary/src/edit.rs +++ b/crates/locallibrary/src/edit.rs @@ -2004,8 +2004,11 @@ impl<'a> Resolver<'a> { } } } + // None found (replaying): the note a replay skips the edit with names them. if picked.is_empty() { - return Err(Error::NotFound("the history lists to delete".into())); + let names: Vec = wanted.iter().map(|w| format!("{:?}", w.name)).collect(); + let lists = if names.len() == 1 { "history list" } else { "history lists" }; + return Err(Error::NotFound(format!("{lists} {} to delete", names.join(", ")))); } let desc = format!("delete {} history list(s)", picked.len()); let lists = picked @@ -2825,7 +2828,7 @@ struct Steps { /// A plan with steps (see [`Steps`]): one backup first (the databases, the analysis files cue /// edits may rewrite, the images artwork edits may replace and the files removed tracks leave), /// so `restore` undoes all of it; then each step, planned as it's made; then the others, planned -/// again. The backup notes what the plan left, all of it made or not. +/// again. The backup notes what the plan left, all of it made or not, as each step is made. fn replay(plan: &EditPlan, steps: &Steps, device: &mut Device, ctx: &Ctx<'_>) -> Result { let library = device.library()?; let mut extra = plan.files.clone(); @@ -2849,11 +2852,19 @@ fn replay(plan: &EditPlan, steps: &Steps, device: &mut Device, ctx: &Ctx<'_>) -> // A plan of new edits (a replace carrying cues) is an edit like any other to undo. let label = if steps.opts.no_journal { "before replaying edits" } else { "before editing" }; let backup = backup::create(device, label, &extra, None)?; + // Sealed as each step is made, from the start: a replay cut off part way can be undone + // without forcing it, as what it left is known. + if let Err(e) = backup::seal(device, &backup.id) { + let _ = backup::discard(device, &backup.id); + return Err(e); + } // Changes are counted as they're made: a recorded edit that no longer applies writes nothing. let mut report = EditReport { backup: Some(backup.id.clone()), ..Default::default() }; let swaps = device.swaps(); let mut made = (Vec::new(), Vec::new()); - let replayed = replay_steps(steps, &plan.changes, &extra, device, &mut report, &mut made, ctx); + let replayed = crate::commit::under_backup(&backup.id, || { + replay_steps(steps, &plan.changes, &extra, device, &mut report, &mut made, ctx) + }); // What it made is recorded as one entry under the one backup, also when a later edit failed, // so a replay after rekordbox exports makes that again. let recorded = match steps.opts.no_journal || made.0.is_empty() { @@ -3435,10 +3446,24 @@ pub mod journal { Ok(gone.len()) } - /// The entries restoring `backup` would take off (see `undone`), oldest first. + /// The entries restoring `backup` would take off, oldest first: those the journal it carries + /// lacks (see [`crate::backup`]), or for a backup carrying none, those it undoes (see + /// `undone`). pub fn taken_off(device: &Device, backup: &crate::backup::BackupInfo) -> Result> { + let entries = list(device)?; + if let Some(carried) = crate::backup::carried_journal(device, backup)? { + return Ok(entries.into_iter().filter(|e| !carried.iter().any(|c| c.at == e.at)).collect()); + } let order = order(device)?; - Ok(list(device)?.into_iter().filter(|e| undoes(&order, backup, e)).collect()) + Ok(entries.into_iter().filter(|e| undoes(&order, backup, e)).collect()) + } + + /// The entries restoring `backup` would put back, oldest first: those the journal it carries + /// has and the drive's lacks (an undo's backup holds the edits the undo took off). + pub fn put_back(device: &Device, backup: &crate::backup::BackupInfo) -> Result> { + let entries = list(device)?; + let carried = crate::backup::carried_journal(device, backup)?.unwrap_or_default(); + Ok(carried.into_iter().filter(|c| !entries.iter().any(|e| e.at == c.at)).collect()) } /// Each of the drive's backups by id, and its place among them (0: the newest). diff --git a/crates/locallibrary/src/format/mod.rs b/crates/locallibrary/src/format/mod.rs index 1e1da2e17..f59d881f3 100644 --- a/crates/locallibrary/src/format/mod.rs +++ b/crates/locallibrary/src/format/mod.rs @@ -314,8 +314,11 @@ pub fn disk_by_id(spec: &str) -> Result> { os::by_id(spec) } -/// The disk holding the volume mounted at `root`. +/// The disk holding the volume mounted at `root`. Refused while `$LOCALLIBRARY_VOLUMES` hides +/// this machine's disks (see [`crate::device::volume_folders`]): the disk holding a test folder is +/// this machine's own, and the system's disk tools don't run then. pub fn disk_at(root: &Path) -> Result { + crate::device::disks_shown()?; os::by_mount(&root.canonicalize().unwrap_or_else(|_| root.to_path_buf())) } diff --git a/crates/locallibrary/src/format/tests.rs b/crates/locallibrary/src/format/tests.rs index fe95e6cc6..7dbf002ec 100644 --- a/crates/locallibrary/src/format/tests.rs +++ b/crates/locallibrary/src/format/tests.rs @@ -867,3 +867,23 @@ fn write_access_to_a_device_that_is_gone_is_an_io_error(tc: TestCase) { other => panic!("{other:?}"), } } + +/// Regression: under `$LOCALLIBRARY_VOLUMES`, the disk holding a drive folder was still looked up +/// with the system's disk tools (`diskutil info`, `lsblk`), though that disk is this machine's +/// own. It's refused without running any, as listing and formatting are. +#[cfg(unix)] +#[test] +fn the_test_switch_runs_no_disk_tool_for_a_drive_folder() { + let mut fake = fake_tools::FakeTools::new(); + let root = fake.dir().join("DNA"); + std::fs::create_dir(&root).unwrap(); + let root = root.canonicalize().unwrap(); + fake.volume_at(&root, "vfat", true); + assert!(disk_at(&root).is_ok(), "the fake tools answer for it"); + let before = fake.calls().len(); + crate::testing::volumes_on_this_thread(Some(Vec::new())); + let hidden = disk_at(&root); + crate::testing::volumes_on_this_thread(None); + assert!(matches!(&hidden, Err(Error::Invalid(m)) if m.starts_with("LOCALLIBRARY_VOLUMES is set")), "{hidden:?}"); + assert_eq!(fake.calls().len(), before, "no tool ran"); +} diff --git a/crates/locallibrary/src/fsutil.rs b/crates/locallibrary/src/fsutil.rs index 13c4f1abb..2c54ece7f 100644 --- a/crates/locallibrary/src/fsutil.rs +++ b/crates/locallibrary/src/fsutil.rs @@ -321,9 +321,61 @@ pub fn mtime_secs(meta: &fs::Metadata) -> i64 { FileTime::from_last_modification_time(meta).unix_seconds() } +/// How far apart two mtimes of the same file may be: FAT32 stores them with 2 second +/// granularity. +const MTIME_DRIFT: i64 = 2; + /// FAT32 stores mtimes with 2 second granularity, so allow that much drift. pub fn mtime_eq(a: i64, b: i64) -> bool { - (a - b).abs() <= 2 + (a - b).abs() <= MTIME_DRIFT +} + +/// Whether a file's size and modification time `mtime`, recorded at `at` (unix seconds), still +/// tell it apart from any rewrite since: a file rewritten after `at` has an mtime of `at - 1` or +/// later (FAT32 rounds down to 2 s, and `at` is whole seconds), which must be more than +/// [`mtime_eq`]'s drift from `mtime`. A file recorded in the same window it was written in can be +/// rewritten at the same size with an mtime that looks unchanged ("racy", as git calls it), so +/// equal size and mtime say nothing about it. +pub fn settled(mtime: i64, at: i64) -> bool { + at - 1 > mtime + MTIME_DRIFT +} + +/// The file or folder at `path` (whose metadata is `m`): its device, and its id there. The +/// device and inode on Unix; on Windows the volume serial number and file index, through a +/// handle (0s when it can't be opened). +#[cfg(unix)] +pub(crate) fn ids(_: &Path, m: &fs::Metadata) -> (u64, u64) { + use std::os::unix::fs::MetadataExt; + (m.dev(), m.ino()) +} + +#[cfg(windows)] +pub(crate) fn ids(path: &Path, _: &fs::Metadata) -> (u64, u64) { + let info = winapi_util::Handle::from_path_any(path).and_then(winapi_util::file::information); + info.map_or((0, 0), |i| (i.volume_serial_number(), i.file_index())) +} + +/// Whether `a` and `b` are on the same volume, each by its nearest folder that's there (a folder +/// an export makes may not be yet). `false` when either can't be told. +pub(crate) fn same_volume(a: &Path, b: &Path) -> bool { + let volume = |p: &Path| p.ancestors().find_map(|p| fs::metadata(p).ok().map(|m| ids(p, &m).0)).filter(|v| *v != 0); + volume(a).is_some_and(|v| volume(b) == Some(v)) +} + +/// Run `f`, warning `waiting` once if it's still running after `after`: reading an online-only +/// file waits for its download, which can take minutes, and the warning says which file it is. +pub(crate) fn warn_if_slow(ctx: &Ctx<'_>, after: std::time::Duration, waiting: String, f: impl FnOnce() -> T) -> T { + let (done, finished) = std::sync::mpsc::channel::<()>(); + std::thread::scope(|s| { + s.spawn(move || { + if finished.recv_timeout(after) == Err(std::sync::mpsc::RecvTimeoutError::Timeout) { + ctx.warn(waiting); + } + }); + let out = f(); + drop(done); + out + }) } /// Free bytes on the filesystem holding `path`. diff --git a/crates/locallibrary/src/fsutil/tests.rs b/crates/locallibrary/src/fsutil/tests.rs index c58f240c5..8eb9630a8 100644 --- a/crates/locallibrary/src/fsutil/tests.rs +++ b/crates/locallibrary/src/fsutil/tests.rs @@ -419,3 +419,53 @@ fn a_file_read_while_its_rewritten_is_read_whole() { }); assert_eq!(read_json::(&path).unwrap(), 50); } + +/// A file's size and mtime recorded at `at` count as settled exactly when no rewrite after `at` +/// can show an mtime [`mtime_eq`] takes for the recorded one: a rewrite at second `t` (at or after +/// `at`) shows `t` where mtimes are kept to the second, and `t` rounded down to an even second on +/// FAT32. Unsettled, a FAT32 file (even mtime) has such a rewrite: one at `at` itself. Regression: +/// the hash cache and scrub each had a window of their own, neither allowing for that drift. +#[hegel::test(test_cases = 300)] +fn settled_means_no_rewrite_since_can_look_unchanged(tc: hegel::TestCase) { + use hegel::generators as gs; + let mtime: i64 = tc.draw(gs::integers::().min_value(1_000_000_000).max_value(2_000_000_000)); + let at = mtime + tc.draw(gs::integers::().min_value(0).max_value(12)); + let later = tc.draw(gs::integers::().min_value(0).max_value(10)); + let rewrite = at + later; + let shown = [rewrite, rewrite - rewrite.rem_euclid(2)]; + if settled(mtime, at) { + assert!(shown.iter().all(|s| !mtime_eq(mtime, *s)), "mtime {mtime}, recorded {at}, rewritten {rewrite}"); + } + let fat = mtime - mtime.rem_euclid(2); + if !settled(fat, at.max(fat)) { + let at = at.max(fat); + assert!( + mtime_eq(fat, at - at.rem_euclid(2)), + "FAT mtime {fat}, recorded {at}: a rewrite at {at} looks unchanged" + ); + } +} + +/// Waiting longer than `after` (an online-only file downloading), the wait is said once, naming +/// what it's for; finishing in time, nothing is said. Each closure waits on the other side's +/// signal, not on a clock, so neither case depends on timing. +#[test] +fn a_slow_wait_is_said_once() { + use std::sync::mpsc; + use std::time::Duration; + let (said, heard) = mpsc::channel::(); + let said = std::sync::Mutex::new(said); + let tell = move |e: &crate::progress::Event| { + if let crate::progress::Event::Warning { message } = e { + let _ = said.lock().unwrap().send(message.clone()); + } + }; + let ctx = Ctx::new(&tell, crate::progress::CancelToken::new()); + let waited = warn_if_slow(&ctx, Duration::from_millis(1), "waiting for a.mp3".into(), || { + heard.recv_timeout(Duration::from_secs(60)).expect("the warning comes while it waits") + }); + assert_eq!(waited, "waiting for a.mp3"); + let quick = warn_if_slow(&ctx, Duration::from_secs(3600), "waiting for b.mp3".into(), || 7); + assert_eq!(quick, 7); + assert!(heard.try_recv().is_err(), "nothing said for a quick one"); +} diff --git a/crates/locallibrary/src/library_write.rs b/crates/locallibrary/src/library_write.rs index 52d69c8ac..c7a3ffe21 100644 --- a/crates/locallibrary/src/library_write.rs +++ b/crates/locallibrary/src/library_write.rs @@ -179,6 +179,17 @@ pub(crate) fn plan( let backs_up = |class: FileClass| matches!(class, FileClass::Analysis | FileClass::Artwork); // After a write cut off on the target, a file modified since it started may be torn. let cut_off = crate::sync::cut_off_since(target); + // A file copied moments after its source was written may have missed a rewrite of it that + // kept the size and mtime (see `fsutil::settled`): it's compared by content. + let index = target.load_index(); + let holds = |e: &crate::tree::FileEntry, src: &Source| { + let hash = |p: &Path| crate::hash::hash_file(p, &Ctx::silent(), Phase::Hash); + let wanted = match &src.audio_path { + Some(audio) => analysis_bytes(&src.path, audio, src.shift_ms).map(|b| Hash::of(&b)), + None => src.hash.map_or_else(|| hash(&src.path), Ok), + }; + matches!((wanted, hash(&tree.fs_path(&e.path))), (Ok(a), Ok(b)) if a == b) + }; let mut out = Planned::default(); for (key, src) in wanted { match by_fold.get(&fold(key)) { @@ -186,6 +197,7 @@ pub(crate) fn plan( if e.size == src.size && fsutil::mtime_eq(e.mtime, src.mtime) && cut_off.is_none_or(|since| e.mtime < since) + && (!index.racy(e) || holds(e, src)) && !src.force => { out.unchanged += 1 @@ -239,26 +251,41 @@ pub(crate) fn plan( out } +/// How long reading an online-only file may take before [`copy`] says it's waiting for it. +const DOWNLOAD_NOTICE: std::time::Duration = std::time::Duration::from_secs(10); + /// Copy what `planned` copies to `target`, flushed and then given their sources' modification -/// times; returns how many files and bytes were copied (what was copied before an error too). -/// The caller starts and ends the copy phase, so a writer can make files of its own in it. +/// times, and recorded in its hash cache (which tells a copy made moments after its source was +/// written, see [`crate::tree::HashIndex::racy`]); returns how many files and bytes were copied +/// (what was copied before an error too). The caller starts and ends the copy phase, so a writer +/// can make files of its own in it. pub(crate) fn copy(target: &Device, planned: &Planned, ctx: &Ctx<'_>) -> Result<(usize, u64)> { - let mut written: Vec<(PathBuf, i64)> = Vec::new(); + let mut written: Vec<(PathBuf, i64, &FileCopy, Hash)> = Vec::new(); let mut bytes = 0; let mut stopped = None; for c in &planned.copies { - let copied = (|| -> Result<(PathBuf, i64)> { + let copied = (|| -> Result<(PathBuf, i64, &FileCopy, Hash)> { ctx.check()?; let src = planned.sources.get(&c.path).ok_or_else(|| Error::NotFound(c.path.clone()))?; let dst = target.checked_path(&c.path)?; ctx.emit(Event::Item { phase: Phase::Copy, path: c.path.clone(), bytes: c.size }); - match &src.audio_path { - Some(audio) => write_unsynced(&dst, &analysis_bytes(&src.path, audio, src.shift_ms)?)?, - None => { - fsutil::copy_file(&src.path, &dst, None, src.hash, false, ctx)?; + let hash = match &src.audio_path { + Some(audio) => { + let bytes = analysis_bytes(&src.path, audio, src.shift_ms)?; + write_unsynced(&dst, &bytes)?; + Hash::of(&bytes) } - } - Ok((dst, src.mtime)) + // An online-only file (Dropbox, Google Drive) downloads as it's read, which can + // take minutes: past a few seconds, say which file it's waiting on. + None if std::fs::metadata(&src.path).is_ok_and(|m| fsutil::is_online_only(&m)) => { + let waiting = format!("waiting for {} to download from the cloud", src.path.display()); + fsutil::warn_if_slow(ctx, DOWNLOAD_NOTICE, waiting, || { + fsutil::copy_file(&src.path, &dst, None, src.hash, false, ctx) + })? + } + None => fsutil::copy_file(&src.path, &dst, None, src.hash, false, ctx)?, + }; + Ok((dst, src.mtime, c, hash)) })(); match copied { Ok(w) => { @@ -271,10 +298,14 @@ pub(crate) fn copy(target: &Device, planned: &Planned, ctx: &Ctx<'_>) -> Result< } } } - fsutil::flush(&written.iter().map(|(p, _)| p.clone()).collect::>())?; - for (path, mtime) in &written { + fsutil::flush(&written.iter().map(|(p, ..)| p.clone()).collect::>())?; + let mut index = target.load_index(); + for (path, mtime, c, hash) in &written { fsutil::set_mtime(path, *mtime)?; + let class = paths::classify(&c.path).unwrap_or(FileClass::Audio); + crate::sync::record(&mut index, path, &c.path, class, *hash)?; } + target.save_index(&index)?; if let Some(e) = stopped { return Err(e); } diff --git a/crates/locallibrary/src/merge.rs b/crates/locallibrary/src/merge.rs index 93a4c0f7e..fc2b59165 100644 --- a/crates/locallibrary/src/merge.rs +++ b/crates/locallibrary/src/merge.rs @@ -148,9 +148,27 @@ pub struct MergePlan { matches: FxHashMap, #[serde(skip)] folder_name: String, + /// The target has the folder an earlier merge of the source made, which this one merges into + /// by name (see [`holds_merge`]). + #[serde(skip)] + again: bool, } impl MergePlan { + /// [`write_playlists`] as this plan writes them (see [`playlist_mode`]). + fn write_playlists( + &self, + editor: &mut impl PlaylistDb, + src: &Library, + dst: &Library, + opts: &MergeOptions, + id_map: &FxHashMap, + ) -> Result<(usize, usize)> { + let (playlists, prefix) = playlist_mode(opts.playlists, self.again, &self.folder_name); + let opts = MergeOptions { playlists, ..opts.clone() }; + write_playlists(editor, src, dst, &opts, prefix, id_map) + } + pub fn is_noop(&self) -> bool { self.new_tracks.is_empty() && self.playlists.iter().all(|p| !p.create && p.tracks_added == 0) @@ -326,8 +344,12 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> )); } - let folder_name = - unique_top_level_name(&dst_lib, opts.folder_name.clone().unwrap_or_else(|| source.name()).as_str()); + let base = opts.folder_name.clone().unwrap_or_else(|| source.name()); + // Merging the same source again goes into the folder the first merge made, by name, so it + // adds only what's new there rather than a "(2)" copy of every playlist. + let again = opts.playlists == PlaylistMode::Folder && holds_merge(&dst_lib, &base, &src_lib, &selected); + let folder_name = if again { base } else { unique_top_level_name(&dst_lib, &base) }; + let (mode, prefix) = playlist_mode(opts.playlists, again, &folder_name); // What the target's export.pdb can't hold is found now, not when applying. let dst_pdb = match target.has_device_library() { true => Some(pdb::parse_database(target.pdb_bytes()?, &target.path(paths::EXPORT_PDB))?), @@ -349,9 +371,9 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> .filter_map(|id| src_lib.history.iter().find(|h| h.id == *id)) .map(|h| h.name.clone()) .collect(); - let playlists = plan_playlists(&src_lib, &dst_lib, &selected, opts.playlists, &folder_name, &matches, &new_ids); + let playlists = plan_playlists(&src_lib, &dst_lib, &selected, mode, prefix, &matches, &new_ids); if let Some(db) = &dst_pdb { - check_names(db, &src_lib, &dst_lib, &selected, opts.playlists, &folder_name, &history)?; + check_names(db, &src_lib, &dst_lib, &selected, mode, prefix, &history)?; } let (tagged_tracks, new_tags) = plan_my_tags(source, &src_lib, target, &dst_lib, &dst_plus, &new_tracks, &mut warnings); @@ -391,6 +413,7 @@ pub fn plan(source: &Device, target: &Device, opts: &MergeOptions, ctx: &Ctx<'_> history_ids, matches, folder_name, + again, state: Some(crate::commit::database_state(target)), }) } @@ -481,7 +504,7 @@ fn check_names( } PlaylistMode::ByName => { for p in src.playlists.iter().filter(|p| selected.contains(&p.id) && !p.is_folder) { - let path = src.playlist_path(p.id); + let path = below(folder_name, src.playlist_path(p.id)); if merge_target(dst, &path, false).is_some() { continue; } @@ -636,6 +659,32 @@ pub(crate) fn analysis_dir(audio: &str, attempt: u32) -> String { format!("PIONEER/USBANLZ/P{p:03X}/{q:08X}") } +/// Whether `dst` has a top-level folder `name` holding every one of `src`'s `selected` playlists +/// and folders at the same path below it: the folder an earlier merge of the same source made. +fn holds_merge(dst: &Library, name: &str, src: &Library, selected: &FxHashSet) -> bool { + merge_target(dst, name, true).is_some() + && src + .playlists + .iter() + .filter(|p| selected.contains(&p.id)) + .all(|p| merge_target(dst, &format!("{name}/{}", src.playlist_path(p.id)), p.is_folder).is_some()) +} + +/// How playlists are written, and below which folder by name (`""`: the top): `mode`, or by name +/// into `folder_name` when the merge goes into the folder an earlier one made (`again`). +fn playlist_mode(mode: PlaylistMode, again: bool, folder_name: &str) -> (PlaylistMode, &str) { + match (mode, again) { + (PlaylistMode::Folder, true) => (PlaylistMode::ByName, folder_name), + (PlaylistMode::ByName, _) => (PlaylistMode::ByName, ""), + _ => (mode, folder_name), + } +} + +/// `path` below the folder at `prefix` (`""`: the top). +fn below(prefix: &str, path: String) -> String { + if prefix.is_empty() { path } else { format!("{prefix}/{path}") } +} + fn unique_top_level_name(lib: &Library, base: &str) -> String { let names: FxHashSet = lib.children(None).iter().map(|p| fold(&p.name)).collect(); let mut name = base.to_string(); @@ -680,7 +729,7 @@ pub(crate) fn plan_playlists( } PlaylistMode::ByName => { for p in src.playlists.iter().filter(|p| selected.contains(&p.id) && !p.is_folder) { - let path = src.playlist_path(p.id); + let path = below(folder_name, src.playlist_path(p.id)); let existing = merge_target(dst, &path, false); let already: FxHashSet = existing.map(|d| d.track_ids.iter().copied().collect()).unwrap_or_default(); @@ -828,8 +877,7 @@ pub fn apply( id_map.insert(t.source_id, new_id); report.tracks_added += 1; } - let (created, entries) = - write_playlists(&mut editor, &src_lib, &target_lib, opts, &plan.folder_name, &id_map)?; + let (created, entries) = plan.write_playlists(&mut editor, &src_lib, &target_lib, opts, &id_map)?; report.playlists_created = created; report.playlist_entries_added = entries; report.history_playlists_added = write_history(&mut editor, &src_lib, &plan.history_ids, &id_map)?; @@ -1068,7 +1116,7 @@ impl PlusMerge<'_> { report.tracks_added += 1; } } - let (created, entries) = write_playlists(one, self.src_lib, self.plus_lib, opts, &plan.folder_name, &id_map)?; + let (created, entries) = plan.write_playlists(one, self.src_lib, self.plus_lib, opts, &id_map)?; let history = write_history(one, self.src_lib, &plan.history_ids, &id_map)?; if self.primary { report.playlists_created = created; @@ -1345,8 +1393,9 @@ fn copy_hot_cue_bank_lists( Ok(added) } -/// Recreate the source's playlists in `db`, whose library is `dst`, as `opts` says (in a folder named -/// `folder_name`, or by name). `id_map` maps source track ids to `db`'s. +/// Recreate the source's playlists in `db`, whose library is `dst`, as `opts` says (in a new folder +/// named `folder_name`, or by name below `folder_name`, `""` for the top). `id_map` maps source +/// track ids to `db`'s. pub(crate) fn write_playlists( editor: &mut impl PlaylistDb, src: &Library, @@ -1402,7 +1451,7 @@ pub(crate) fn write_playlists( // Folder path -> id of the folders we create. let mut made: FxHashMap = FxHashMap::default(); for p in src.playlists.iter().filter(|p| selected.contains(&p.id) && !p.is_folder) { - let path = src.playlist_path(p.id); + let path = below(folder_name, src.playlist_path(p.id)); let existing = merge_target(dst, &path, false); let (id, already) = match existing { Some(d) => (d.id, d.track_ids.iter().copied().collect()), diff --git a/crates/locallibrary/src/op.rs b/crates/locallibrary/src/op.rs index fc3c5d78a..4af50a8cd 100644 --- a/crates/locallibrary/src/op.rs +++ b/crates/locallibrary/src/op.rs @@ -210,7 +210,17 @@ impl Touched { /// Whether it changed since it was measured. pub fn changed(&self) -> bool { - measure(&self.root, &self.probes, None) != self.state + self.measured() != self.state + } + + /// Its state as the plan was made. + pub(crate) fn planned_state(&self) -> Hash { + self.state + } + + /// Its state now, measured as when the plan was made. + pub(crate) fn measured(&self) -> Hash { + measure(&self.root, &self.probes, None) } /// The drive, opened again (refused if it's gone). diff --git a/crates/locallibrary/src/pack.rs b/crates/locallibrary/src/pack.rs index e3e0b720f..408488657 100644 --- a/crates/locallibrary/src/pack.rs +++ b/crates/locallibrary/src/pack.rs @@ -311,6 +311,9 @@ pub fn plan(source: &Device, target: &Device, opts: &PackOptions, ctx: &Ctx<'_>) )); } warnings.extend(edit::journal::forget_warning(target, None, "packing")); + if databases.plus.is_some() { + warnings.extend(crate::player::no_name_note(target)); + } let transcode_bytes: u64 = made.transcodes.iter().filter(|t| !t.current).map(|t| t.size).sum(); if !written.fits { warnings.push(format!( diff --git a/crates/locallibrary/src/player.rs b/crates/locallibrary/src/player.rs index 69bc48b97..c7a4edc54 100644 --- a/crates/locallibrary/src/player.rs +++ b/crates/locallibrary/src/player.rs @@ -234,21 +234,36 @@ pub(crate) fn keep_identity(from: &OneLibrary, to: &OneLibrary) -> Result<()> { /// drive's pending marker, so a run cut off before then (after taking the database off first, or /// after swapping the new one in) leaves them for the run that finishes the job: the drive's /// database may no longer hold them, and that run may have no database left to replace. +/// +/// A drive with no `exportLibrary.db` has no name or colour to keep, and is left with none rather +/// than the source's: a set stick packed from the main drive shouldn't show up on the players as +/// the main drive (see [`no_name_note`]). pub fn stash(device: &mut Device, replaces: bool) -> Result> { let kept = device.state().and_then(|s| s.pending.as_ref()).and_then(|p| p.player.clone()); if kept.is_some() || !replaces { return Ok(kept); } - let now = db_identity(device.root()); - if now.is_some() - && let Some(p) = device.state_mut()?.pending.as_mut() - { + let now = Some(db_identity(device.root()).unwrap_or_default()); + if let Some(p) = device.state_mut()?.pending.as_mut() { p.player.clone_from(&now); device.save_state()?; } Ok(now) } +/// A plan's note that `target`, which has no `exportLibrary.db`, gets one with no player name or +/// colour (see [`stash`]), or `None` when it has one already. +pub(crate) fn no_name_note(target: &Device) -> Option { + let has = paths::to_fs(target.root(), paths::LIBRARY_PLUS_DB).is_file(); + (!has).then(|| { + format!( + "{} has no exportLibrary.db, so it gets one without a player name or colour (not the source's); \ + `locallibrary drive player` sets them", + target.name() + ) + }) +} + /// After a sync, push or pack replaced `exportLibrary.db`, put back the name and colour the /// target had (`before`, from [`stash`]). The caller holds the drive's lock from before it read /// them until this is done, so no other write comes between. Returns whether anything was @@ -256,7 +271,9 @@ pub fn stash(device: &mut Device, replaces: bool) -> Result> pub fn preserve(device: &Device, before: Option<&DbIdentity>) -> Result { let Some(before) = before else { return Ok(false) }; let Some(now) = db_identity(device.root()) else { return Ok(false) }; - if &now == before { + // No name is no name, and colour 0 no colour, however the database stores them. + let shown = |id: &DbIdentity| (id.name.clone(), id.colour.unwrap_or(0)); + if shown(&now) == shown(before) { return Ok(false); } onelibrary::write(device, &with_identity(&OneLibrary::open(device)?, before, false)?)?; diff --git a/crates/locallibrary/src/scrub.rs b/crates/locallibrary/src/scrub.rs index 62c8a94af..8f3be96bb 100644 --- a/crates/locallibrary/src/scrub.rs +++ b/crates/locallibrary/src/scrub.rs @@ -187,11 +187,13 @@ impl<'a> References<'a> { if let Some(hash) = self.index.lookup(e) { return Some(Expected { hash, size: e.size, mtime: e.mtime, kind: ReferenceKind::Cache }); } + // Only files written clearly before the snapshot: a database rekordbox or a player saved + // again moments after a pull would otherwise be taken for damage and "repaired" back. for s in &self.history { if let Some(f) = s.files.iter().find(|f| f.path.to_lowercase() == key) && f.size == e.size && fsutil::mtime_eq(f.mtime, e.mtime) - && settled(e.mtime, s.created.timestamp()) + && fsutil::settled(f.mtime, s.created.timestamp()) && let Some(hash) = f.hash { return Some(Expected { hash, size: f.size, mtime: f.mtime, kind: ReferenceKind::History }); @@ -234,16 +236,6 @@ impl<'a> References<'a> { } } -/// Whether a file modified at `mtime` (unix seconds) was written before a snapshot taken at -/// `taken`, and so can't be a rewrite since: one made after it has an mtime of `taken` or later, -/// less a second for FAT32's 2 s rounding. Without this, a database rekordbox or a player saved -/// again moments after a pull, at the same size and within [`fsutil::mtime_eq`]'s 2 s, would be -/// taken for damage and "repaired" back to the older copy (the hash cache distrusts its racy -/// entries the same way, see [`HashIndex::lookup`]). -fn settled(mtime: i64, taken: i64) -> bool { - mtime < taken - 1 -} - /// Whether `tree` (hashed) holds `snapshot`'s databases as it saved them: then the drive holds the /// library that snapshot is of, and every file it lists belongs there. Anything else (a rekordbox /// export since, a player's history list) changed what belongs. diff --git a/crates/locallibrary/src/sync.rs b/crates/locallibrary/src/sync.rs index 8d7492b04..091d37bca 100644 --- a/crates/locallibrary/src/sync.rs +++ b/crates/locallibrary/src/sync.rs @@ -360,6 +360,10 @@ pub fn plan(source: &Source<'_>, target: &Device, target_tree: &FileTree, opts: // The DJ's own files where the source keeps audio outside PIONEER/ and Contents/. let mut in_the_way = Vec::new(); let cut_off = cut_off_since(target); + // Files copied onto the target moments after their source was written (see + // `fsutil::settled`): their size and mtime don't say they're still the same, so they're + // compared by content. + let index = target.load_index(); for &e in &source_entries { if !paths::is_safe_key(&e.path) { @@ -375,9 +379,13 @@ pub fn plan(source: &Source<'_>, target: &Device, target_tree: &FileTree, opts: let (reason, replaced) = match target_by_fold.get(&f) { Some(t) => { let suspect = cut_off.is_some_and(|since| t.mtime >= since); - ((!same_content(e, t, opts, suspect)?).then_some(CopyReason::Changed), Some((t.path.clone(), t.size))) + let same = match !suspect && index.racy(t) { + true => same_by_content(source, e, &target_tree.fs_path(&t.path), t.size)?, + false => same_content(e, t, opts, suspect)?, + }; + ((!same).then_some(CopyReason::Changed), Some((t.path.clone(), t.size))) } - None => match outside_file(target, e, cut_off)? { + None => match outside_file(source, target, &index, e, cut_off)? { Outside::Free => (Some(CopyReason::New), None), Outside::Same => (None, None), Outside::Ours(size) => (Some(CopyReason::Changed), Some((e.path.clone(), size))), @@ -463,6 +471,9 @@ pub fn plan(source: &Source<'_>, target: &Device, target_tree: &FileTree, opts: // Database files last, OneLibrary's WAL after its database; deletes of database files (e.g. // exportLibrary.db disappearing) too. plan.copies.sort_by_key(|c| (c.class == FileClass::Database, c.path.clone())); + if plan.copies.iter().any(|c| is_library_plus(&c.path)) { + plan.warnings.extend(crate::player::no_name_note(target)); + } if plan.replaces_databases() { let from = match source { Source::Drive { device, .. } => Some(*device), @@ -518,7 +529,13 @@ enum Outside { /// The target's scan lists everything under PIONEER/ and Contents/, and audio its own library /// keeps elsewhere. Anywhere else, a file at the source's path is the DJ's, unless it's the same /// or a run cut off on this drive wrote it. -fn outside_file(target: &Device, e: &FileEntry, cut_off: Option) -> Result { +fn outside_file( + source: &Source<'_>, + target: &Device, + index: &crate::tree::HashIndex, + e: &FileEntry, + cut_off: Option, +) -> Result { if paths::classify(&e.path).is_some() { return Ok(Outside::Free); } @@ -528,10 +545,19 @@ fn outside_file(target: &Device, e: &FileEntry, cut_off: Option) -> Result< return Ok(Outside::Theirs); } let ours = cut_off.is_some_and(|since| fsutil::mtime_secs(&meta) >= since); + let here = FileEntry { + path: e.path.clone(), + size: meta.len(), + mtime: fsutil::mtime_secs(&meta), + class: e.class, + hash: None, + }; let same = meta.len() == e.size && match e.hash { Some(h) => crate::hash::hash_file(&path, &Ctx::silent(), crate::progress::Phase::Hash)? == h, - None => !ours && fsutil::mtime_eq(fsutil::mtime_secs(&meta), e.mtime), + None if ours => false, + None if index.racy(&here) => same_by_content(source, e, &path, here.size)?, + None => fsutil::mtime_eq(here.mtime, e.mtime), }; Ok(match (same, ours) { (true, _) => Outside::Same, @@ -540,6 +566,20 @@ fn outside_file(target: &Device, e: &FileEntry, cut_off: Option) -> Result< }) } +/// Whether the target's file at `path` (`size` bytes) holds the source's `src`, by content: for a +/// copy made moments after its source was written (see [`crate::tree::HashIndex::racy`]). +fn same_by_content(source: &Source<'_>, src: &FileEntry, path: &Path, size: u64) -> Result { + if src.size != size { + return Ok(false); + } + let hash = |p: &Path| crate::hash::hash_file(p, &Ctx::silent(), crate::progress::Phase::Hash); + let theirs = match src.hash { + Some(h) => h, + None => hash(&source.fs_path(src)?)?, + }; + Ok(hash(path)? == theirs) +} + /// Whether the target's file `dst` already holds the source's `src`. A `suspect` file (see /// [`cut_off_since`]) is only taken as it is when its content is known to match. fn same_content(src: &FileEntry, dst: &FileEntry, opts: &SyncOptions, suspect: bool) -> Result { @@ -746,7 +786,14 @@ fn settle(copied: &[Copied], index: &mut crate::tree::HashIndex) -> Result<()> { Ok(()) } -fn record(index: &mut crate::tree::HashIndex, path: &Path, key: &str, class: FileClass, hash: Hash) -> Result<()> { +/// Note in `index` that the file at `path` (`key`) holds `hash`, as it is now. +pub(crate) fn record( + index: &mut crate::tree::HashIndex, + path: &Path, + key: &str, + class: FileClass, + hash: Hash, +) -> Result<()> { let meta = std::fs::metadata(path).at(path)?; index.record(&FileEntry { path: key.to_string(), diff --git a/crates/locallibrary/src/tags.rs b/crates/locallibrary/src/tags.rs index 7b47f697c..8730a4bfd 100644 --- a/crates/locallibrary/src/tags.rs +++ b/crates/locallibrary/src/tags.rs @@ -797,7 +797,8 @@ impl Ids { } /// The other database's id for the same category, if this one doesn't use it; else the next - /// small id (rekordbox's categories are 1..4). + /// small id (rekordbox's categories are 1..4), or the smallest free one when that's past what + /// the database holds (a OneLibrary database can number them past exportExt.pdb's `u32`). fn category(&self, source: Source, tree: &Tree, other: Option<&Tree>, name: &str) -> i64 { if let Some(id) = other.and_then(|o| o.category(name)).map(|c| c.id) && Ids::fits(source, id) @@ -806,6 +807,9 @@ impl Ids { return id; } let mut id = self.max_category + 1; + if !Ids::fits(source, id) { + id = 1; + } while self.used.contains(&id) { id += 1; } @@ -955,11 +959,18 @@ fn plan_db( Ok(ops) } -/// Refuse a name too long for one exportExt.pdb row, which its editor would only refuse when -/// applying (OneLibrary takes any length). +/// Refuse a name too long for one exportExt.pdb row, or an id past its `u32`, which applying +/// would only refuse then (OneLibrary takes any length, and `i64` ids). fn check_names(db: &Db, ops: &[Op]) -> Result<()> { let Handle::Ext(bytes) = &db.handle else { return Ok(()) }; let editor = guard_ext(|| ext_editor(bytes.clone()))?; + for op in ops { + match op { + Op::AddTag { category, id, .. } => [*category, *id].into_iter().try_for_each(|i| ext_id(i).map(drop))?, + Op::AddCategory { id, .. } | Op::Rename { id, .. } | Op::Remove { id } => drop(ext_id(*id)?), + Op::Link { tag, .. } | Op::Unlink { tag, .. } => drop(ext_id(*tag)?), + } + } for op in ops { let (what, name) = match op { Op::AddCategory { name, .. } => ("the category name", name), diff --git a/crates/locallibrary/src/tags/tests.rs b/crates/locallibrary/src/tags/tests.rs index e421d59b9..8f09aecad 100644 --- a/crates/locallibrary/src/tags/tests.rs +++ b/crates/locallibrary/src/tags/tests.rs @@ -220,7 +220,13 @@ fn new_categories_and_tags_get_ids_both_databases_can_share(tc: TestCase) { let got = ids.category(source, &tree, Some(&other), &name); match other.category(&name).map(|c| c.id).filter(|id| usable(*id)) { Some(theirs) => assert_eq!(got, theirs, "{name}: the other database's id"), - None => assert!(got > max_category && !used.contains(&got), "{name}: {got}, used {used:?}"), + // The next id, or the smallest free one when that doesn't fit (LOC-37). + None => assert!( + Ids::fits(source, got) + && !used.contains(&got) + && (got > max_category || !Ids::fits(source, max_category + 1)), + "{name}: {got}, used {used:?}" + ), } for tag in NAMES { let got = ids.tag(source, &tree, Some(&other), &name, tag); diff --git a/crates/locallibrary/src/tree.rs b/crates/locallibrary/src/tree.rs index d8e5afe37..e35c8c196 100644 --- a/crates/locallibrary/src/tree.rs +++ b/crates/locallibrary/src/tree.rs @@ -222,9 +222,6 @@ struct IndexEntry { at: i64, } -/// mtime granularity to distrust (FAT32 rounds to 2 s). -const RACY_SECS: i64 = 2; - impl HashIndex { pub fn load(path: &Path) -> HashIndex { // A broken cache is never fatal: it only costs re-hashing. @@ -236,15 +233,25 @@ impl HashIndex { } /// The cached hash, if the file's size and mtime are unchanged *and* the hash was recorded - /// clearly after the file was last modified. A file hashed in the same (FAT: 2 s) window it - /// was written in could change again without its mtime moving ("racy" entries, as in git). + /// clearly after the file was last modified (see [`fsutil::settled`]). A file hashed in the + /// same (FAT: 2 s) window it was written in could change again without its mtime moving + /// ("racy" entries, as in git). pub fn lookup(&self, e: &FileEntry) -> Option { self.entries .get(&e.path) - .filter(|i| i.size == e.size && i.mtime == e.mtime && i.at > i.mtime + RACY_SECS) + .filter(|i| i.size == e.size && i.mtime == e.mtime && fsutil::settled(i.mtime, i.at)) .map(|i| i.hash) } + /// Whether `e` looks like the file recorded at its path, by size and mtime, but was recorded + /// too soon after it was written for that to say it's unchanged (see [`fsutil::settled`]): + /// a file copied moments after its source was written may have missed a rewrite of it. + pub fn racy(&self, e: &FileEntry) -> bool { + self.entries + .get(&e.path) + .is_some_and(|i| i.size == e.size && fsutil::mtime_eq(i.mtime, e.mtime) && !fsutil::settled(i.mtime, i.at)) + } + pub fn record(&mut self, e: &FileEntry) { if let Some(hash) = e.hash { let at = chrono::Utc::now().timestamp(); diff --git a/crates/locallibrary/src/tree/tests.rs b/crates/locallibrary/src/tree/tests.rs index a6ceebd8f..b03b0fffd 100644 --- a/crates/locallibrary/src/tree/tests.rs +++ b/crates/locallibrary/src/tree/tests.rs @@ -78,3 +78,29 @@ fn hash_index_avoids_rehashing() { entry.size += 1; assert_eq!(index.lookup(entry), None); } + +/// An entry recorded within moments of the file's mtime is racy: not looked up, and [`racy`] +/// says so while the file still shows that size and mtime; recorded clearly later, it isn't. +/// +/// [`racy`]: HashIndex::racy +#[test] +fn entries_recorded_as_the_file_was_written_are_racy() { + let now = chrono::Utc::now().timestamp(); + let entry = |mtime: i64| FileEntry { + path: "Contents/a.mp3".into(), + size: 5, + mtime, + class: FileClass::Audio, + hash: Some(Hash::of(b"hello")), + }; + // Racy entries' mtimes are ahead of now, so however slow the machine, they're recorded within + // the window. + for (mtime, racy) in [(now + 30, true), (now + 1, true), (now - 10, false), (1_600_000_000, false)] { + let mut index = HashIndex::default(); + index.record(&entry(mtime)); + let seen = FileEntry { hash: None, ..entry(mtime) }; + assert_eq!(index.racy(&seen), racy, "mtime {} s from now", mtime - now); + assert_eq!(index.lookup(&seen).is_none(), racy); + assert!(!index.racy(&FileEntry { size: 6, ..seen.clone() }), "another size isn't the recorded file"); + } +} diff --git a/crates/locallibrary/src/watch/mod.rs b/crates/locallibrary/src/watch/mod.rs index 6c70bf3ff..e3a82d37e 100644 --- a/crates/locallibrary/src/watch/mod.rs +++ b/crates/locallibrary/src/watch/mod.rs @@ -18,7 +18,7 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use uuid::Uuid; -use crate::paths; +use crate::{fsutil, paths}; /// A mounted volume and the device it's on. The device matters because a mount point can show /// up before its drive is mounted on it (macOS and udisks make the folder first): the path @@ -36,25 +36,10 @@ pub fn mounts() -> Vec { } fn mount(path: PathBuf) -> Mount { - let dev = std::fs::metadata(&path).map_or(0, |m| ids(&path, &m).0); + let dev = std::fs::metadata(&path).map_or(0, |m| fsutil::ids(&path, &m).0); Mount { path, dev } } -/// The file or folder at `path` (whose metadata is `m`): its device, and its id there. The -/// device and inode on Unix; on Windows the volume serial number and file index, through a -/// handle (0s when it can't be opened). -#[cfg(unix)] -fn ids(_: &Path, m: &std::fs::Metadata) -> (u64, u64) { - use std::os::unix::fs::MetadataExt; - (m.dev(), m.ino()) -} - -#[cfg(windows)] -fn ids(path: &Path, _: &std::fs::Metadata) -> (u64, u64) { - let info = winapi_util::Handle::from_path_any(path).and_then(winapi_util::file::information); - info.map_or((0, 0), |i| (i.volume_serial_number(), i.file_index())) -} - /// One file's metadata, enough to tell that it was rewritten. #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] pub struct FileStamp { @@ -72,7 +57,7 @@ impl FileStamp { pub fn of(path: &Path) -> Option { crate::fsutil::unreplaced(|| { let m = std::fs::metadata(path).ok().filter(std::fs::Metadata::is_file)?; - Some(FileStamp { len: m.len(), modified: m.modified().ok(), ino: ids(path, &m).1 }) + Some(FileStamp { len: m.len(), modified: m.modified().ok(), ino: fsutil::ids(path, &m).1 }) }) } } @@ -219,7 +204,7 @@ struct StoreStamp { impl StoreStamp { fn of(root: &Path) -> StoreStamp { let dir = root.join("snapshots"); - let snapshots = std::fs::metadata(&dir).ok().map(|m| (m.modified().ok(), ids(&dir, &m).1)); + let snapshots = std::fs::metadata(&dir).ok().map(|m| (m.modified().ok(), fsutil::ids(&dir, &m).1)); StoreStamp { files: STORE_FILES.map(|(_, f)| FileStamp::of(&root.join(f))), snapshots } } diff --git a/crates/locallibrary/tests/it/artwork/embed.rs b/crates/locallibrary/tests/it/artwork/embed.rs index 848c1f420..d2417a930 100644 --- a/crates/locallibrary/tests/it/artwork/embed.rs +++ b/crates/locallibrary/tests/it/artwork/embed.rs @@ -114,3 +114,27 @@ fn embedding_skips_what_it_cant_rewrite_and_unembedding_what_is_gone() { assert!(plan.restored.is_empty()); assert_eq!(plan.skipped.iter().map(|s| s.reason.as_str()).collect::>(), ["gone"]); } + +/// Regression: a file rewritten in the second or two after art went in, at the same size, kept the +/// size and mtime the embed noted, so taking the art back out put the old copy over the rewrite. +/// The embed notes what the file held, and a file that holds something else is left alone; the +/// plan says so before anything is put back. +#[test] +fn a_file_rewritten_as_the_art_went_in_is_left_alone() { + let dir = tempfile::tempdir().unwrap(); + let (store, files) = setup(dir.path(), &["a.wav"]); + let image = dir.path().join("cover.png"); + let id = artwork::embed_with_backup(&store, &files, &art(), &image, &Ctx::silent()).unwrap().backup.unwrap(); + let mtime = locallibrary::fsutil::mtime_secs(&std::fs::metadata(&files[0]).unwrap()); + let mut rewritten = std::fs::read(&files[0]).unwrap(); + let last = rewritten.len() - 1; + rewritten[last] ^= 0xff; + std::fs::write(&files[0], &rewritten).unwrap(); + locallibrary::fsutil::set_mtime(&files[0], mtime).unwrap(); + + let plan = api::artwork_unembed_plan(&store, &api::UnembedRequest { backup: Some(id.clone()) }).unwrap(); + assert!(plan.restored.is_empty(), "{:?}", plan.restored); + let r = artwork::restore_embedded(&store, Some(&id)).unwrap(); + assert_eq!(r.skipped.iter().map(|s| s.reason.as_str()).collect::>(), ["changed since the art went in"]); + assert_eq!(std::fs::read(&files[0]).unwrap(), rewritten); +} diff --git a/crates/locallibrary/tests/it/drives.rs b/crates/locallibrary/tests/it/drives.rs index 4d79361a4..3e3e17513 100644 --- a/crates/locallibrary/tests/it/drives.rs +++ b/crates/locallibrary/tests/it/drives.rs @@ -382,6 +382,47 @@ fn player_names_are_per_drive_and_survive_sync() { assert_eq!(c.relation, Relation::Identical); } +/// Regression: a sync or pack onto a stick with no exportLibrary.db gave it the source's player +/// name and colour, as there was none to keep, so a set stick showed up on the players as the main +/// drive. It's left with none, and the plan says so. +#[test] +fn a_stick_without_onelibrary_gets_no_player_name() { + use locallibrary::pack::{self, PackOptions}; + let dir = tempfile::tempdir().unwrap(); + let a_root = common::bigger_drive(dir.path(), "A"); + common::add_onelibrary(&a_root); + let mut a = Device::open(&a_root).unwrap(); + player::set(&mut a, Some("MAIN"), Some(3)).unwrap(); + let said = |warnings: &[String], name: &str| { + warnings + .iter() + .any(|w| w.starts_with(&format!("{name} has no exportLibrary.db, so it gets one without a player name"))) + }; + let unnamed = |root: &std::path::Path| { + let id = player::db_identity(root).expect("a OneLibrary database"); + assert_eq!((id.name, id.colour.unwrap_or(0)), (None, 0), "{}", root.display()); + }; + + std::fs::create_dir(dir.path().join("B")).unwrap(); + let mut b = Device::open(dir.path().join("B")).unwrap(); + let (plan, _) = sync::plan_drive_to_drive(&a, &b, &SyncOptions::default(), &ctx()).unwrap(); + assert!(said(&plan.warnings, &b.name()), "{:?}", plan.warnings); + mirror(&mut a, &mut b); + unnamed(b.root()); + // Once it has one, a sync keeps what it has. + let (plan, _) = sync::plan_drive_to_drive(&a, &b, &SyncOptions::default(), &ctx()).unwrap(); + assert!(!said(&plan.warnings, &b.name()), "{:?}", plan.warnings); + + let club_root = dir.path().join("Club"); + std::fs::create_dir(&club_root).unwrap(); + let mut club = Device::open(&club_root).unwrap(); + let opts = PackOptions { playlists: vec!["aab".into()], ..Default::default() }; + let plan = pack::plan(&a, &club, &opts, &ctx()).unwrap(); + assert!(said(&plan.warnings, &club.name()), "{:?}", plan.warnings); + pack::apply(&plan, &a, &mut club, None, &ctx()).unwrap(); + unnamed(&club_root); +} + /// Regression: a sync with `--delete-first` takes the target's database off before copying, and /// one cut off then left the run that finished the job no player name to put back: the drive got /// its source's. The name is kept with the drive's pending marker until it's back. diff --git a/crates/locallibrary/tests/it/edit/journal_replay.rs b/crates/locallibrary/tests/it/edit/journal_replay.rs index 7b4fbd97a..287f5701f 100644 --- a/crates/locallibrary/tests/it/edit/journal_replay.rs +++ b/crates/locallibrary/tests/it/edit/journal_replay.rs @@ -122,3 +122,138 @@ fn a_replay_makes_edits_again_in_the_one_database_that_lost_them() { assert!(again.is_noop(), "{lost}: {:?}", again.changes); } } + +/// Regression: the backup an undo takes didn't carry the journal, so redoing the undo (putting +/// that backup back) left the drive with the edit and the journal without it, and `edits replay` +/// wouldn't make it again. Restoring an older backup and then a newer one was the same. Backups +/// carry the journal, and putting one back puts it back as it was. +#[test] +fn a_redo_puts_the_undone_edits_back_on_the_journal() { + use locallibrary::api::{UndoRequest, undo_plan}; + use locallibrary::backup; + use locallibrary::edit::Edit; + let dir = tempfile::tempdir().unwrap(); + let root = common::demo_drive(dir.path(), "Main"); + for name in ["Friday", "Saturday"] { + common::apply(&root, &[Edit::CreatePlaylist { path: name.into(), folder: false }]); + } + let changes = + |d: &Device| -> Vec> { edit::journal::list(d).unwrap().into_iter().map(|e| e.changes).collect() }; + let has = |d: &Device, name: &str| d.library().unwrap().find_playlist(name).is_some(); + let mut d = Device::open_export(&root).unwrap(); + let both = changes(&d); + assert_eq!(both.len(), 2); + + // Undo Saturday, then redo it by putting back the backup the undo took. + let undone = backup::restore(&mut d, None, false).unwrap(); + assert!(!has(&d, "Saturday")); + assert_eq!(changes(&d), both[..1]); + let redo = backup::list(&d).unwrap().into_iter().find(|b| b.undoes.as_deref() == Some(&undone.id)).unwrap(); + let req = UndoRequest { drive: Some(root.display().to_string()), backup: Some(redo.id.clone()), force: false }; + let plan = undo_plan(&req).unwrap(); + assert!(plan.summary.contains(&"1 recorded edit(s) go back on the journal".to_string()), "{:?}", plan.summary); + assert!(plan.journal.is_empty(), "nothing comes off: {:?}", plan.journal); + backup::restore(&mut d, Some(&redo.id), false).unwrap(); + assert!(has(&d, "Saturday")); + assert_eq!(changes(&d), both, "the redone edit is back on the journal"); + + // Restore the oldest backup (before Friday), then the one before Saturday: the journal + // follows the drive both times. + let backups = backup::list(&d).unwrap(); + let made: Vec<_> = backups.iter().filter(|b| b.undoes.is_none()).collect(); + let (before_saturday, before_friday) = (made[0].id.clone(), made[1].id.clone()); + backup::restore(&mut d, Some(&before_friday), false).unwrap(); + assert!(!has(&d, "Friday") && !has(&d, "Saturday")); + assert!(changes(&d).is_empty()); + backup::restore(&mut d, Some(&before_saturday), false).unwrap(); + assert!(has(&d, "Friday") && !has(&d, "Saturday")); + assert_eq!(changes(&d), both[..1], "Friday is back on the journal"); + let edits = edit::journal::edits(&d).unwrap(); + let replay = edit::plan(&d, &edits, &EditOptions { lenient: true, no_journal: true }, &Ctx::silent()).unwrap(); + assert!(replay.is_noop(), "the drive has every recorded edit: {:?}", replay.changes); +} + +/// Regression: the backup an undo takes didn't carry the player name and colour the drive +/// remembers, so redoing an undone `drive player` left the drive remembering none, and `refresh` +/// wouldn't put the name back after rekordbox's next export. +#[test] +fn a_redo_remembers_the_player_name_again() { + use locallibrary::{backup, player}; + let dir = tempfile::tempdir().unwrap(); + let root = common::demo_drive(dir.path(), "Main"); + common::add_onelibrary(&root); + let mut d = Device::open(&root).unwrap(); + d.adopt(None).unwrap(); + player::set(&mut d, Some("MAIN"), Some(3)).unwrap(); + let remembered = |d: &Device| Device::open(d.root()).unwrap().state().unwrap().player.clone(); + let set = remembered(&d).unwrap(); + + let undone = backup::restore(&mut d, None, false).unwrap(); + assert_eq!(remembered(&d), None, "undone, the drive remembers no name"); + let redo = backup::list(&d).unwrap().into_iter().find(|b| b.undoes.as_deref() == Some(&undone.id)).unwrap(); + backup::restore(&mut d, Some(&redo.id), false).unwrap(); + assert_eq!(player::db_identity(&root).unwrap().name.as_deref(), Some("MAIN")); + assert_eq!(remembered(&d), Some(set), "redone, it remembers MAIN again"); +} + +/// Regression: a replay cut off part way (the app killed) left its backup without what its change +/// left, so once the replay was run again to finish it, taking it all back needed `undo --force`. +/// The backup notes what each step left: two undos (the second run's, then the first's) put the +/// drive back as rekordbox exported it, neither forced. +#[test] +fn a_replay_cut_off_part_way_can_be_undone_without_forcing() { + use locallibrary::backup; + use locallibrary::progress::{CancelToken, Event, Phase}; + let dir = tempfile::tempdir().unwrap(); + let root = common::demo_drive(dir.path(), "Main"); + let exported = common::drive_files(&root); + // A playlist edit first, which a replay makes after the cue edits: the drive cut off after + // the first of those is like no state an edit left. + common::apply(&root, &[locallibrary::edit::Edit::CreatePlaylist { path: "Friday".into(), folder: false }]); + let mut d = Device::open_export(&root).unwrap(); + let lib = d.library().unwrap(); + for track in &lib.tracks[..2] { + let set = CueEdit::Set(SetCue { + hot_cue: Some(HotCue(1)), + at: CuePosition::Beat(9), + snap: false, + loop_beats: None, + name: None, + color: None, + }); + cue_edit::apply(&cue_edit::plan(&d, &[track.id], &set, &Ctx::silent()).unwrap(), &mut d, &Ctx::silent()) + .unwrap(); + } + // rekordbox exports again, without the cues. + for (key, bytes) in &exported { + std::fs::write(paths::to_fs(&root, key), bytes).unwrap(); + } + assert!(common::changed_files(&exported, &common::drive_files(&root)).is_empty()); + + // The replay is killed as its second step starts copying, its first step made. + let killed = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let kill = |e: &Event| { + let started = matches!(e, Event::PhaseStart { phase: Phase::Copy, .. }); + if started && !common::changed_files(&exported, &common::drive_files(&root)).is_empty() { + panic!("killed"); + } + }; + let ctx = Ctx::new(&kill, CancelToken::new()); + let mut d = Device::open_export(&root).unwrap(); + let edits = edit::journal::edits(&d).unwrap(); + let plan = edit::plan(&d, &edits, &EditOptions { lenient: true, no_journal: true }, &ctx).unwrap(); + edit::apply(&plan, &mut d, &ctx) + })); + assert!(killed.is_err(), "the replay was killed"); + let cut_off = common::drive_files(&root); + assert!(!common::changed_files(&exported, &cut_off).is_empty(), "the first step was made"); + + common::replay(&root); + let mut d = Device::open_export(&root).unwrap(); + backup::restore(&mut d, None, false).unwrap(); + assert!(common::changed_files(&cut_off, &common::drive_files(&root)).is_empty(), "back to where it was cut off"); + let mut d = Device::open_export(&root).unwrap(); + let undone = backup::restore(&mut d, None, false).unwrap_or_else(|e| panic!("undone unforced: {e}")); + assert_eq!(undone.reason, "before replaying edits"); + assert!(common::changed_files(&exported, &common::drive_files(&root)).is_empty(), "back to the export"); +} diff --git a/crates/locallibrary/tests/it/edit/refusals.rs b/crates/locallibrary/tests/it/edit/refusals.rs index 04b20231d..92cd9e5df 100644 --- a/crates/locallibrary/tests/it/edit/refusals.rs +++ b/crates/locallibrary/tests/it/edit/refusals.rs @@ -55,7 +55,7 @@ fn history_lists_are_deleted_only_with_the_entries_they_were_recorded_with() { // Replaying, the edit is skipped with a warning. let replay = plan(&root, &[other_entries], true).unwrap(); assert!(replay.is_noop()); - let skipped = "skipped an edit that no longer applies: not found: the history lists to delete"; + let skipped = "skipped an edit that no longer applies: not found: history list \"HISTORY 001\" to delete"; assert_eq!(replay.warnings, [skipped]); let exact = Edit::DeleteHistoryLists { lists: vec![list(vec![path(1), path(2)])] }; diff --git a/crates/locallibrary/tests/it/merge.rs b/crates/locallibrary/tests/it/merge.rs index 4ce8a8a57..5c471b8e7 100644 --- a/crates/locallibrary/tests/it/merge.rs +++ b/crates/locallibrary/tests/it/merge.rs @@ -351,3 +351,28 @@ fn merging_from_export_pdb_brings_cues_into_onelibrary() { assert_eq!(got.iter().filter(|(hot, _)| hot.is_some()).count(), 1, "{got:?}"); assert!(got.contains(&(None, 20_000)), "{got:?}"); } + +/// Regression: merging the same drive into a folder again made a "Guest (2)" folder with a copy of +/// every playlist. The second merge goes into the folder the first made, by name, and adds +/// nothing. +#[test] +fn merging_the_same_drive_into_a_folder_again_adds_nothing() { + let dir = tempfile::tempdir().unwrap(); + let src_root = common::bigger_drive(dir.path(), "Guest"); + let src = Device::open_export(&src_root).unwrap(); + let mut dst = Device::open_export(common::demo_drive(dir.path(), "Main")).unwrap(); + let opts = MergeOptions::default(); + let first = merge::plan(&src, &dst, &opts, &Ctx::silent()).unwrap(); + merge::apply(&first, &src, &mut dst, &opts, &Ctx::silent()).unwrap(); + let lib = dst.library().unwrap(); + let merged = lib.find_playlist("Guest/aab").unwrap().track_ids.len(); + assert_eq!(merged, 12); + + let again = merge::plan(&src, &dst, &opts, &Ctx::silent()).unwrap(); + assert!(again.is_noop(), "{:?}", again.playlists); + let report = merge::apply(&again, &src, &mut dst, &opts, &Ctx::silent()).unwrap(); + assert_eq!((report.playlists_created, report.playlist_entries_added), (0, 0)); + let lib = dst.library().unwrap(); + assert!(lib.find_playlist("Guest (2)").is_none(), "no second folder"); + assert_eq!(lib.find_playlist("Guest/aab").unwrap().track_ids.len(), merged); +} diff --git a/crates/locallibrary/tests/it/merge/matching.rs b/crates/locallibrary/tests/it/merge/matching.rs index 41a8ceef4..a2dbf3ba1 100644 --- a/crates/locallibrary/tests/it/merge/matching.rs +++ b/crates/locallibrary/tests/it/merge/matching.rs @@ -142,7 +142,7 @@ fn names_too_long_for_the_targets_export_pdb_are_refused_when_planning() { } /// Merging one drive into another says what it adds and adds it; merged again, its tracks are -/// all there already. +/// all there already, and so are its playlists, in the folder the first merge made. #[test] fn a_merge_adds_what_it_planned_and_then_matches_it() { let dir = tempfile::tempdir().unwrap(); @@ -157,8 +157,9 @@ fn a_merge_adds_what_it_planned_and_then_matches_it() { let report = api::merge_apply(&req, &Ctx::silent()).unwrap(); assert_eq!(report.tracks_added, 12); assert_eq!(Device::open(&main).unwrap().library().unwrap().tracks.len(), 14); - // Merged again, every track is matched; only the folder of its playlists comes again. + // Merged again, every track is matched, and its playlists are in the folder already. let again = api::merge_plan(&req, &Ctx::silent()).unwrap(); assert_eq!((again.new_tracks.len(), again.matched_tracks), (0, 12)); - assert!(again.playlists.iter().all(|p| p.path.starts_with("Club (2)")), "{:?}", again.playlists); + assert!(again.playlists.iter().all(|p| !p.create && p.tracks_added == 0), "{:?}", again.playlists); + assert!(again.noop, "{:?}", again.summary); } diff --git a/crates/locallibrary/tests/it/pack/files.rs b/crates/locallibrary/tests/it/pack/files.rs index fcd340911..d2a878b73 100644 --- a/crates/locallibrary/tests/it/pack/files.rs +++ b/crates/locallibrary/tests/it/pack/files.rs @@ -78,3 +78,35 @@ fn a_drive_is_packed_for_a_set() { let written: Vec = store.drives().unwrap().into_values().map(|d| d.log.writes).collect(); assert_eq!(written, [1], "the stick is registered with its write"); } + +/// Regression: a pack (and a collection export, which writes the same way) that copied a file +/// moments after its source was written took the copy as current next time while size and mtime +/// matched, though the source could have been rewritten since in the same second or two. That +/// copy is copied again, and a rewrite at the same size and mtime reaches the target. +#[test] +fn a_file_packed_as_it_was_written_is_copied_again_next_time() { + let dir = tempfile::tempdir().unwrap(); + let src_root = common::bigger_drive(dir.path(), "Main"); + let club_root = dir.path().join("Club"); + fs::create_dir_all(&club_root).unwrap(); + let src = Device::open_export(&src_root).unwrap(); + let mut club = Device::open(&club_root).unwrap(); + let audio = paths::key(&src.library().unwrap().tracks[0].file_path); + let file = paths::to_fs(&src_root, &audio); + // Ahead of the clock, so the copy is made within its window however slow the machine is. + let written = chrono::Utc::now().timestamp() + 30; + locallibrary::fsutil::set_mtime(&file, written).unwrap(); + let opts = PackOptions { playlists: vec!["aab".into()], ..Default::default() }; + pack::apply(&pack::plan(&src, &club, &opts, &Ctx::silent()).unwrap(), &src, &mut club, None, &Ctx::silent()) + .unwrap(); + + let mut bytes = fs::read(&file).unwrap(); + let last = bytes.len() - 1; + bytes[last] ^= 0xff; + fs::write(&file, &bytes).unwrap(); + locallibrary::fsutil::set_mtime(&file, written).unwrap(); + let plan = pack::plan(&src, &club, &opts, &Ctx::silent()).unwrap(); + assert!(plan.copies.iter().any(|c| c.path == audio), "{:?}", plan.copies); + pack::apply(&plan, &src, &mut club, None, &Ctx::silent()).unwrap(); + assert_eq!(fs::read(paths::to_fs(&club_root, &audio)).unwrap(), bytes); +} diff --git a/crates/locallibrary/tests/it/sync/compare.rs b/crates/locallibrary/tests/it/sync/compare.rs index 0256d88a2..3bf95165b 100644 --- a/crates/locallibrary/tests/it/sync/compare.rs +++ b/crates/locallibrary/tests/it/sync/compare.rs @@ -539,3 +539,33 @@ fn the_differences_include_onelibrarys() { assert_eq!(c.relation, Relation::LeftNewer); assert_eq!(c.diff.added_playlists, ["Booth"], "{:?}", c.diff); } + +/// Regression: a sync that copied a file moments after its source was written took the copy as +/// current on the next sync while size and mtime matched, though the source could have been +/// rewritten since in the same second or two (FAT32 keeps mtimes to 2 s). That copy is compared +/// by content next time, and a rewrite at the same size and mtime is copied. +#[test] +fn a_file_copied_as_it_was_written_is_checked_by_content_next_time() { + let dir = tempfile::tempdir().unwrap(); + let mut a = Device::open_export(common::demo_drive(dir.path(), "A")).unwrap(); + let b_root = dir.path().join("B"); + std::fs::create_dir_all(&b_root).unwrap(); + let mut b = Device::open(&b_root).unwrap(); + let audio = paths::key(&a.library().unwrap().tracks[0].file_path); + let src = paths::to_fs(a.root(), &audio); + // Ahead of the clock, so the copy is made within its window however slow the machine is. + let written = chrono::Utc::now().timestamp() + 30; + locallibrary::fsutil::set_mtime(&src, written).unwrap(); + sync::sync_drive_to_drive(&mut a, &mut b, &SyncOptions::default(), &Ctx::silent()).unwrap(); + + // Rewritten within the same second: same size, same mtime. + let mut bytes = std::fs::read(&src).unwrap(); + let last = bytes.len() - 1; + bytes[last] ^= 0xff; + std::fs::write(&src, &bytes).unwrap(); + locallibrary::fsutil::set_mtime(&src, written).unwrap(); + let (plan, _) = sync::plan_drive_to_drive(&a, &b, &SyncOptions::default(), &Ctx::silent()).unwrap(); + assert!(plan.copies.iter().any(|c| c.path == audio), "{:?}", plan.copies); + sync::sync_drive_to_drive(&mut a, &mut b, &SyncOptions::default(), &Ctx::silent()).unwrap(); + assert_eq!(std::fs::read(paths::to_fs(b.root(), &audio)).unwrap(), bytes); +} diff --git a/crates/locallibrary/tests/it/tags.rs b/crates/locallibrary/tests/it/tags.rs index f51f243c9..79fdb0312 100644 --- a/crates/locallibrary/tests/it/tags.rs +++ b/crates/locallibrary/tests/it/tags.rs @@ -790,3 +790,31 @@ fn tag_filters_select_with_the_drives_my_tags() { let cues = api::cues_list(&ListCues { library: drive.into(), tracks: filter("tag:\"Peak Time\"") }).unwrap(); assert_eq!(cues.iter().map(|c| c.track.id).collect::>(), peak); } + +/// Regression: a OneLibrary category id past exportExt.pdb's `u32` made the next category id +/// planned for exportExt.pdb one past it too, so the plan looked fine and applying failed with "My +/// Tag id N doesn't fit". exportExt.pdb gets the smallest free id instead, and plan and apply +/// agree. +#[test] +fn a_onelibrary_category_id_past_u32_still_plans_what_applies() { + let dir = tempfile::tempdir().unwrap(); + let root = both_drive(dir.path()); + // exportExt.pdb with room for two more categories, and OneLibrary with one numbered past it. + let ext_path = root.join(paths::EXPORT_EXT_PDB); + let ids: Vec = ExtDatabase::from_file(&ext_path).unwrap().categories().iter().map(|c| c.id).collect(); + let mut ed = ExtEditor::from_file(&ext_path).unwrap(); + for id in &ids[2..] { + ed.remove(*id).unwrap(); + } + ed.save(&ext_path).unwrap(); + let mut one = OneLibrary::open_root(&root).unwrap(); + one.add_my_tag(i64::from(u32::MAX) + 1, 0, "Venue", true).unwrap(); + fs::write(root.join(paths::LIBRARY_PLUS_DB), one.to_encrypted().unwrap()).unwrap(); + let mut d = Device::open_export(&root).unwrap(); + let tracks = select(&d, "Noise"); + let plan = tags::plan(&d, &add("Venue/Club", tracks)).unwrap(); + tags::apply(&plan, &mut d, &Ctx::silent()).unwrap_or_else(|e| panic!("planned, then: {e}")); + let ext = ExtDatabase::from_file(&ext_path).unwrap(); + let venue = ext.categories().into_iter().find(|c| c.name == "Venue").expect("Venue in exportExt.pdb"); + assert!(!ids[..2].contains(&venue.id), "a free id: {}", venue.id); +} From 5ea8218b80152b2da915c63e1975852069c88a5b Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Sun, 1 Nov 2026 11:29:25 -0500 Subject: [PATCH 2/4] docs(locallibrary): stop linking private items from the backup and stash docs --- crates/locallibrary/src/backup.rs | 2 +- crates/locallibrary/src/player.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/locallibrary/src/backup.rs b/crates/locallibrary/src/backup.rs index eaebb582d..336a32ae1 100644 --- a/crates/locallibrary/src/backup.rs +++ b/crates/locallibrary/src/backup.rs @@ -5,7 +5,7 @@ //! files, removed tracks' files), so restoring one undoes the change (`locallibrary undo`): files //! that were added stay on the drive but are no longer referenced. A backup also carries the //! drive's journal of edits (see [`crate::edit::journal`]) and the player name and colour it -//! remembers ([`Carried`]), and restoring one puts those back as they were: `edits replay` then +//! remembers (`Carried`), and restoring one puts those back as they were: `edits replay` then //! makes again exactly the edits the restored library had, also after an undo is redone. Restoring //! a backup an earlier version took, which carries neither, takes the edits it undoes off the //! journal instead. diff --git a/crates/locallibrary/src/player.rs b/crates/locallibrary/src/player.rs index c7a4edc54..a592da205 100644 --- a/crates/locallibrary/src/player.rs +++ b/crates/locallibrary/src/player.rs @@ -237,7 +237,7 @@ pub(crate) fn keep_identity(from: &OneLibrary, to: &OneLibrary) -> Result<()> { /// /// A drive with no `exportLibrary.db` has no name or colour to keep, and is left with none rather /// than the source's: a set stick packed from the main drive shouldn't show up on the players as -/// the main drive (see [`no_name_note`]). +/// the main drive (see `no_name_note`). pub fn stash(device: &mut Device, replaces: bool) -> Result> { let kept = device.state().and_then(|s| s.pending.as_ref()).and_then(|p| p.player.clone()); if kept.is_some() || !replaces { From f4db7e6ceab5fb0c6b3ccb50b591e1dfd1a673ea Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Sun, 1 Nov 2026 11:29:26 -0500 Subject: [PATCH 3/4] fix(locallibrary): keep a new category's id within exportExt.pdb's u32 when the next one is taken --- crates/locallibrary/src/tags.rs | 17 +++++++++-------- crates/locallibrary/src/tags/tests.rs | 8 ++++++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/crates/locallibrary/src/tags.rs b/crates/locallibrary/src/tags.rs index 270474c13..a83d874a1 100644 --- a/crates/locallibrary/src/tags.rs +++ b/crates/locallibrary/src/tags.rs @@ -806,14 +806,15 @@ impl Ids { { return id; } - let mut id = self.max_category + 1; - if !Ids::fits(source, id) { - id = 1; - } - while self.used.contains(&id) { - id += 1; - } - id + let free_from = |mut id: i64| { + while self.used.contains(&id) { + id += 1; + } + id + }; + // The next free id can run past `u32::MAX` even when `max_category + 1` fits. + let id = free_from(self.max_category + 1); + if Ids::fits(source, id) { id } else { free_from(1) } } /// The other database's id for the same tag, if this one doesn't use it; else a large id diff --git a/crates/locallibrary/src/tags/tests.rs b/crates/locallibrary/src/tags/tests.rs index 4dea4434d..13462147a 100644 --- a/crates/locallibrary/src/tags/tests.rs +++ b/crates/locallibrary/src/tags/tests.rs @@ -220,11 +220,15 @@ fn new_categories_and_tags_get_ids_both_databases_can_share(tc: TestCase) { let got = ids.category(source, &tree, Some(&other), &name); match other.category(&name).map(|c| c.id).filter(|id| usable(*id)) { Some(theirs) => assert_eq!(got, theirs, "{name}: the other database's id"), - // The next id, or the smallest free one when that doesn't fit (LOC-37). + // The next free id, or the smallest free one when none past every category's fits + // (LOC-37). None => assert!( Ids::fits(source, got) && !used.contains(&got) - && (got > max_category || !Ids::fits(source, max_category + 1)), + && (got > max_category + || (max_category + 1..) + .take_while(|id| Ids::fits(source, *id)) + .all(|id| used.contains(&id))), "{name}: {got}, used {used:?}" ), } From cf283966006a9d03fcc41320bec6e352a22c33f1 Mon Sep 17 00:00:00 2001 From: botandbang <337765928+botandbang@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:05:28 -0500 Subject: [PATCH 4/4] test(locallibrary): wait for the hash cache's settled rule before twinning a fixture --- apps/locallibrary/tests/it/props_cli_human.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/locallibrary/tests/it/props_cli_human.rs b/apps/locallibrary/tests/it/props_cli_human.rs index 248120ded..59a1d6501 100644 --- a/apps/locallibrary/tests/it/props_cli_human.rs +++ b/apps/locallibrary/tests/it/props_cli_human.rs @@ -339,8 +339,8 @@ pub fn build(s: State) -> Env { /// (by id too), recorded where `fx`'s were. pub fn twin(fx: &Env) -> Env { let root = fx.volumes.parent().unwrap(); - // First every file a moment older than FAT's 2 s: the hash cache distrusts a hash taken within - // 2 s of a file's mtime, so until then the two copies, run side by side but not in step, could + // First every file settled: the hash cache distrusts a hash taken too soon after a file's mtime + // (`fsutil::settled`), so until then the two copies, run side by side but not in step, could // count different files verified. let secs = || filetime::FileTime::now().unix_seconds(); let mtimes = walkdir::WalkDir::new(root).into_iter().flatten().filter_map(|e| e.metadata().ok()); @@ -348,7 +348,7 @@ pub fn twin(fx: &Env) -> Env { let newest = mtimes.map(|m| filetime::FileTime::from_last_modification_time(&m).unix_seconds()).filter(|t| *t <= now); if let Some(newest) = newest.max() { - while secs() <= newest + 2 { + while !locallibrary::fsutil::settled(newest, secs()) { std::thread::sleep(std::time::Duration::from_millis(100)); } }