rm and dedupe leave the iTunesDB out of sync when a delete fails mid-batch #2

New issue

Open

opened 2026-08-25 21:04:33 +02:00 by wokoman · 1 comment

wokoman commented

2026-08-25 21:04:33 +02:00

(Migrated from code.nolog.cz)

ironpod rm and ironpod dedupe delete audio files in a loop and write the iTunesDB once at the end. Both loops propagate the first error with ?, which returns from the function before db.write() runs:

// src/cli.rs:414 (rm), src/cli.rs:453 (dedupe)
for (id, label) in &doomed {
    copy::remove(&mut db, *id).with_context(|| format!("removing {label}"))?;
}
db.write().context("writing iTunesDB after removal")?;

copy::remove deletes the audio file only after the in-memory database has stopped referencing it (src/copy.rs:477-486) — deliberately, so a failed unlink leaves an orphan file rather than a database entry pointing at nothing. That ordering is correct per call. It does not hold across a batch that never reaches db.write().

Failure

Remove 10 matching tracks. Track 6 fails to unlink — a read-only file, a FAT32 hiccup, the volume going away mid-run.

Tracks 1-5: audio files deleted from disk, and the on-disk iTunesDB still lists them.
Track 6: file present, still listed.
Tracks 7-10: untouched.

The iPod then shows five tracks that skip or hang when selected, and there is no way to remove them from the device — ironpod rm matches the database entry, copy::remove finds no file to delete and (correctly) treats NotFound as success, so it repairs itself on the next successful run. But until then the device is in a state the user did not ask for and cannot see.

The write direction has no equivalent problem: copy::copy_all reports per-file failures through a callback and always reaches its write.

The TUI does not have this bug

src/tui.rs:994-1004 runs the same loop but breaks on the first error and writes anyway, so its on-disk database matches what is actually on disk. The two removal paths have diverged in crash-consistency behaviour, which is the underlying issue — see the architecture note below.

Fix

Make the CLI loops match the TUI: stop at the first failure, still write, then report. The minimum change is break plus an Option<anyhow::Error> in both rm and dedupe.

Note that three call sites now hand-roll "loop copy::remove, then db.write() once" while the copy direction has copy::copy_all to do exactly this. A copy::remove_all with the same callback shape would make the consistency guarantee a property of the module rather than something each caller has to remember. That refactor is deliberately not in scope here — this ticket is the correctness fix, and the interface question is being grilled separately.

Verification

Currently untestable in CI: the removal path needs a real Database, and there is no in-memory fixture. Options, in order of cost:

A device test under IRONPOD_MOUNT + IRONPOD_ALLOW_WRITE=1 that copies two markers, chmods one file to force the unlink to fail, runs the batch, and asserts the reopened database lists exactly what is on disk.
Extract the loop body so the error path can be driven without libgpod.
Wait for a Database fixture (separate ticket) and test it properly.

`ironpod rm` and `ironpod dedupe` delete audio files in a loop and write the iTunesDB once at the end. Both loops propagate the first error with `?`, which returns from the function *before* `db.write()` runs: ```rust // src/cli.rs:414 (rm), src/cli.rs:453 (dedupe) for (id, label) in &doomed { copy::remove(&mut db, *id).with_context(|| format!("removing {label}"))?; } db.write().context("writing iTunesDB after removal")?; ``` `copy::remove` deletes the audio file only after the in-memory database has stopped referencing it (`src/copy.rs:477-486`) — deliberately, so a failed unlink leaves an orphan file rather than a database entry pointing at nothing. That ordering is correct per call. It does not hold across a batch that never reaches `db.write()`. ## Failure Remove 10 matching tracks. Track 6 fails to unlink — a read-only file, a FAT32 hiccup, the volume going away mid-run. - Tracks 1-5: audio files deleted from disk, and the on-disk iTunesDB still lists them. - Track 6: file present, still listed. - Tracks 7-10: untouched. The iPod then shows five tracks that skip or hang when selected, and there is no way to remove them from the device — `ironpod rm` matches the database entry, `copy::remove` finds no file to delete and (correctly) treats `NotFound` as success, so it repairs itself on the next successful run. But until then the device is in a state the user did not ask for and cannot see. The write direction has no equivalent problem: `copy::copy_all` reports per-file failures through a callback and always reaches its write. ## The TUI does not have this bug `src/tui.rs:994-1004` runs the same loop but breaks on the first error and writes anyway, so its on-disk database matches what is actually on disk. The two removal paths have diverged in crash-consistency behaviour, which is the underlying issue — see the architecture note below. ## Fix Make the CLI loops match the TUI: stop at the first failure, still write, then report. The minimum change is `break` plus an `Option<anyhow::Error>` in both `rm` and `dedupe`. Note that three call sites now hand-roll "loop `copy::remove`, then `db.write()` once" while the copy direction has `copy::copy_all` to do exactly this. A `copy::remove_all` with the same callback shape would make the consistency guarantee a property of the module rather than something each caller has to remember. That refactor is deliberately **not** in scope here — this ticket is the correctness fix, and the interface question is being grilled separately. ## Verification Currently untestable in CI: the removal path needs a real `Database`, and there is no in-memory fixture. Options, in order of cost: 1. A device test under `IRONPOD_MOUNT` + `IRONPOD_ALLOW_WRITE=1` that copies two markers, chmods one file to force the unlink to fail, runs the batch, and asserts the reopened database lists exactly what is on disk. 2. Extract the loop body so the error path can be driven without libgpod. 3. Wait for a `Database` fixture (separate ticket) and test it properly.

wokoman commented

2026-08-25 23:12:52 +02:00

(Migrated from code.nolog.cz)

Superseded as a work item by #5, which fixes this by routing every removal path through a batch interface that owns the write-once guarantee.

Do not implement the minimal fix described above — it collides with #5. This issue stays open as the bug of record and #5 closes it.

Dependency chain: #3 (test fixture) → #4 (batch removal) → #5.

Superseded as a work item by #5, which fixes this by routing every removal path through a batch interface that owns the write-once guarantee. Do not implement the minimal fix described above — it collides with #5. This issue stays open as the bug of record and #5 closes it. Dependency chain: #3 (test fixture) → #4 (batch removal) → #5.