Batch track removal that guarantees the database matches the disk #4

New issue

Open

opened 2026-08-25 22:19:56 +02:00 by wokoman · 0 comments

wokoman commented

2026-08-25 22:19:56 +02:00

(Migrated from code.nolog.cz)

What to build

A single batch-removal entry point that owns the rule every caller currently re-implements: the database on disk always matches the audio files on disk.

Removing tracks is a loop of two steps per track — drop it from the in-memory database, then delete its audio file — followed by one write of the database at the end. Getting the failure behaviour right is not obvious, and today three call sites each guess at it separately. One of them guesses wrong (see #2). This ticket makes the guarantee a property of the module instead of something each caller has to remember.

Nothing changes for users yet. The existing single-track removal stays public and callers stay on it, so the tree stays green; the migration is #5.

The settled interface

From the design session — this shape encodes decisions, so it is worth pinning precisely:

/// Removing a batch stopped early. `removed` tracks are gone and the database
/// has been written, so this is a partial success, not a rollback.
#[derive(Debug)]
pub struct RemoveFailed {
    pub removed: usize,
    pub total: usize,
    pub failed_id: u32,
    source: anyhow::Error,
}

pub fn remove_all(
    db: &mut Database,
    ids: &[u32],
    before_each: &mut dyn FnMut(usize, usize),
) -> Result<usize, RemoveFailed>;

Three points behind that shape:

Stop at the first failure. Deliberately unlike the copy direction, which presses on. A failed copy is per-file; a failed delete is usually systemic — the volume vanished, the filesystem went read-only — and pressing on turns one stranded file into two hundred.

Write the database only when at least one removal succeeded. This is a correctness rule, not an optimisation, and it needs a comment saying so or someone will delete it as dead weight. A track is dropped from the database before its file is deleted, so if the very first deletion fails the database has already changed while the file is still there. Writing then would manufacture a stranded file out of a run that achieved nothing; not writing leaves disk and database agreeing.

The progress callback fires before each removal, not after. The browser is single-threaded and has to repaint before it blocks. This is the opposite of the copy direction's callback, which reports results after the fact, so it must not be named as though they are the same thing.

RemoveFailed needs a hand-written Display/Error rather than a derive: anyhow::Error does not implement std::error::Error, so it cannot be a derived error source.

Acceptance criteria

Removing a batch where every track succeeds returns the count and writes the database exactly once
Removing a batch where one track fails partway stops there, still writes the database, and reports how many were removed before the failure
Reopening the database after a partial failure shows exactly the tracks that were not removed, and every one of them still has its audio file
A batch whose very first removal fails does not write the database at all
The progress callback is observed to fire before each removal
Rustdoc carries an # Errors section stating the stopping rule, the write guarantee and that a write renumbers track IDs so callers must re-read
The divergence from the copy direction is documented on both sides — documented on one side only, it reads like an accident rather than a decision
cargo clippy --all-targets clean; tests pass with no device attached

Blocked by

#3 — none of the acceptance criteria above are testable without the fixture.

## What to build A single batch-removal entry point that owns the rule every caller currently re-implements: **the database on disk always matches the audio files on disk.** Removing tracks is a loop of two steps per track — drop it from the in-memory database, then delete its audio file — followed by one write of the database at the end. Getting the failure behaviour right is not obvious, and today three call sites each guess at it separately. One of them guesses wrong (see #2). This ticket makes the guarantee a property of the module instead of something each caller has to remember. Nothing changes for users yet. The existing single-track removal stays public and callers stay on it, so the tree stays green; the migration is #5. ## The settled interface From the design session — this shape encodes decisions, so it is worth pinning precisely: ```rust /// Removing a batch stopped early. `removed` tracks are gone and the database /// has been written, so this is a partial success, not a rollback. #[derive(Debug)] pub struct RemoveFailed { pub removed: usize, pub total: usize, pub failed_id: u32, source: anyhow::Error, } pub fn remove_all( db: &mut Database, ids: &[u32], before_each: &mut dyn FnMut(usize, usize), ) -> Result<usize, RemoveFailed>; ``` Three points behind that shape: **Stop at the first failure.** Deliberately unlike the copy direction, which presses on. A failed copy is per-file; a failed delete is usually systemic — the volume vanished, the filesystem went read-only — and pressing on turns one stranded file into two hundred. **Write the database only when at least one removal succeeded.** This is a correctness rule, not an optimisation, and it needs a comment saying so or someone will delete it as dead weight. A track is dropped from the database *before* its file is deleted, so if the very first deletion fails the database has already changed while the file is still there. Writing then would manufacture a stranded file out of a run that achieved nothing; not writing leaves disk and database agreeing. **The progress callback fires *before* each removal, not after.** The browser is single-threaded and has to repaint before it blocks. This is the opposite of the copy direction's callback, which reports results after the fact, so it must not be named as though they are the same thing. `RemoveFailed` needs a hand-written `Display`/`Error` rather than a derive: `anyhow::Error` does not implement `std::error::Error`, so it cannot be a derived error source. ## Acceptance criteria - [ ] Removing a batch where every track succeeds returns the count and writes the database exactly once - [ ] Removing a batch where one track fails partway stops there, still writes the database, and reports how many were removed before the failure - [ ] Reopening the database after a partial failure shows exactly the tracks that were not removed, and every one of them still has its audio file - [ ] A batch whose very first removal fails does not write the database at all - [ ] The progress callback is observed to fire before each removal - [ ] Rustdoc carries an `# Errors` section stating the stopping rule, the write guarantee and that a write renumbers track IDs so callers must re-read - [ ] The divergence from the copy direction is documented on **both** sides — documented on one side only, it reads like an accident rather than a decision - [ ] `cargo clippy --all-targets` clean; tests pass with no device attached ## Blocked by - #3 — none of the acceptance criteria above are testable without the fixture.