From 20ef6c459bc244c4da4173a4a6b1228f69b00a4b Mon Sep 17 00:00:00 2001 From: Jake Date: Thu, 14 May 2026 19:29:47 +0100 Subject: [PATCH] =?UTF-8?q?agent:=20lumotia=20=E2=80=94=20Phase=20B.4=20cl?= =?UTF-8?q?ose=20restore-during-purge=20race=20via=20atomic=20DELETE=20RET?= =?UTF-8?q?URNING?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase B.4 audit of commits 15b74db, 87e6248, 50d0715, 99f4ecd (the soft-delete / trash / restore wave — Rev-2, Rev-3). Existing backend coverage is solid: migration_v16_adds_deleted_at_column_and_index, delete_transcript_soft_deletes, delete_transcript_removes_audio_file, list_transcripts_excludes_soft_deleted (with a restore round-trip), and purge_deleted_transcripts_hard_deletes_old. The Svelte UI components added by 87e6248 (Trash view + restore) and 50d0715 (type-the-word DELETE modal) carry TODO(test) notes saying "vitest not installed". That comment is stale — vitest landed in Phase A.5 (206ac62). Adding Svelte component tests is real follow-up work but outside the per-item methodology for B.4; calling it out here for the Phase-B finishing pass to triage. One real residual found. Surface: `purge_deleted_transcripts` in `crates/storage/src/database.rs`. The prior form was a two-statement SELECT-then-DELETE pair: 1. SELECT id, audio_path FROM transcripts WHERE deleted_at IS NOT NULL AND deleted_at < datetime('now', '-30 days'); 2. DELETE FROM transcripts WHERE id IN (chunk_of_ids); A `restore_transcript(id)` between (1) and (2) clears `deleted_at` on a row whose id is in the chunk, but the DELETE has no `deleted_at IS NOT NULL` filter — so the now-LIVE row is hard-deleted alongside its audio file. That bypasses the entire Rev-2 soft-delete safety contract: the user can lose data without the 30-day retention window the contract promised. In the current code the purge runs once at startup before the user can issue a restore, so the race window is narrow in practice. The safety should be structural, not operational — especially if a future change moves the purge to a daily cron. Fix: collapse the SELECT + DELETE into a single `DELETE … RETURNING audio_path`. SQLite (3.35+, well within the sqlx 0.8 amalgam) evaluates the WHERE clause and the row removal atomically; the returned `audio_path`s are guaranteed to belong to rows that THIS call hard-deleted. The audio cleanup loop then operates on those returned paths, never on rows that survived the WHERE clause. The chunking concern (IN-clause near SQLITE_MAX_VARIABLE_NUMBER) disappears too — there is no IN-clause. Behavioural diff for the non-racing path: identical (same WHERE clause, same NotFound-tolerant best-effort fs::remove_file). Behavioural diff for the racing path: a row restored between SELECT and DELETE survives the purge and keeps its audio file — which is the contract Rev-2 was added to enforce. Other surface notes (no fix needed): * `delete_transcript` is robust to its own concurrent restore — the UPDATE has `AND deleted_at IS NULL` and audio removal only fires when `rows_affected() > 0`. * `restore_transcript` is a single UPDATE — atomic. * FTS triggers on UPDATE preserve the row in transcripts_fts; the `t.deleted_at IS NULL` filter on `search_transcripts`'s JOIN keeps trashed rows out of search results. New regression test: `purge_audio_cleanup_only_fires_for_hard_deleted_rows` covers the structural property — an in-retention trashed row with its audio file on disk survives purge with the audio intact, while a past-retention trashed row is hard-deleted with audio removed. Verification: * cargo test -p lumotia-storage --lib database::tests → 53/53 pass including the new test (old purge test still passes). * cargo fmt --check → clean (applied fmt after the test edit). * cargo clippy -p lumotia-storage --all-targets -- -D warnings → clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/storage/src/database.rs | 132 +++++++++++++++++++++++++-------- 1 file changed, 103 insertions(+), 29 deletions(-) diff --git a/crates/storage/src/database.rs b/crates/storage/src/database.rs index e156242..5ac0c4b 100644 --- a/crates/storage/src/database.rs +++ b/crates/storage/src/database.rs @@ -387,19 +387,30 @@ pub async fn delete_transcript(pool: &SqlitePool, id: &str) -> Result<()> { /// /// Audio files are also best-effort removed here in case the original /// soft-delete failed at the filesystem layer (Rev-3 belt-and-braces). +/// +/// **Atomicity (Phase B.4 audit fix 2026-05-14):** the prior form was a +/// two-statement SELECT-then-DELETE-WHERE-id-IN sequence. If a row was +/// restored between the SELECT and the DELETE (`restore_transcript` +/// clearing `deleted_at`), the DELETE still hard-deleted the now-live +/// row and removed its audio file — bypassing the soft-delete safety +/// contract Rev-2 was added to enforce. We now use a single +/// `DELETE … RETURNING` so the row filter is re-evaluated atomically at +/// DELETE time and the returned `audio_path`s are guaranteed to belong +/// to rows that this call actually hard-deleted. This also removes the +/// chunking concern (no `IN(…)` clause means no SQLITE_MAX_VARIABLE_NUMBER +/// ceiling). pub async fn purge_deleted_transcripts(pool: &SqlitePool, older_than_days: i64) -> Result { - // Collect (id, audio_path) BEFORE the DELETE so we still have the - // paths to clean up after the row is gone. let rows = sqlx::query( - "SELECT id, audio_path FROM transcripts \ + "DELETE FROM transcripts \ WHERE deleted_at IS NOT NULL \ - AND deleted_at < datetime('now', ?)", + AND deleted_at < datetime('now', ?) \ + RETURNING audio_path", ) .bind(format!("-{older_than_days} days")) .fetch_all(pool) .await .map_err(|source| Error::Query { - operation: "purge_deleted_transcripts_select".into(), + operation: "purge_deleted_transcripts".into(), source, })?; @@ -407,37 +418,14 @@ pub async fn purge_deleted_transcripts(pool: &SqlitePool, older_than_days: i64) return Ok(0); } - let mut ids: Vec = Vec::with_capacity(rows.len()); let mut audio_paths: Vec = Vec::new(); for row in &rows { - let id: String = row.get("id"); let audio: Option = row.get("audio_path"); - ids.push(id); if let Some(p) = audio { audio_paths.push(p); } } - // Build the IN clause manually because sqlx 0.8 doesn't expand Vec - // bindings; we batch into chunks of 200 to stay well clear of - // SQLITE_MAX_VARIABLE_NUMBER (default 999). - let mut total: u64 = 0; - for chunk in ids.chunks(200) { - let placeholders = std::iter::repeat_n("?", chunk.len()) - .collect::>() - .join(","); - let sql = format!("DELETE FROM transcripts WHERE id IN ({placeholders})"); - let mut q = sqlx::query(&sql); - for id in chunk { - q = q.bind(id); - } - let res = q.execute(pool).await.map_err(|source| Error::Query { - operation: "purge_deleted_transcripts_delete".into(), - source, - })?; - total += res.rows_affected(); - } - // Best-effort audio cleanup. NotFound is the expected case for rows // whose audio was already removed at soft-delete time. for path in &audio_paths { @@ -451,7 +439,7 @@ pub async fn purge_deleted_transcripts(pool: &SqlitePool, older_than_days: i64) } } - Ok(total) + Ok(rows.len() as u64) } /// List soft-deleted transcripts (the "trash" view), most-recently-deleted @@ -3210,4 +3198,90 @@ mod tests { "t-new still inside the retention window" ); } + + /// Phase B.4 audit regression (2026-05-14). Asserts the + /// `DELETE … RETURNING` form correctly couples row-removal with the + /// audio-cleanup loop: only rows the DELETE actually affected get + /// their audio file removed, and rows outside the retention window + /// are untouched. + /// + /// The race we fixed (restore between SELECT and DELETE in the old + /// two-statement form) requires fault injection between the two + /// statements to exercise deterministically. The atomic single- + /// statement form makes that race structurally impossible. We test + /// the structural property: an in-retention trashed row with its + /// audio file on disk survives purge with the audio intact, while a + /// past-retention trashed row is hard-deleted with audio removed. + #[tokio::test] + async fn purge_audio_cleanup_only_fires_for_hard_deleted_rows() { + let pool = test_pool().await; + + let tmpdir = tempfile::tempdir().expect("tmpdir"); + let old_audio = tmpdir.path().join("old.wav"); + let recent_audio = tmpdir.path().join("recent.wav"); + std::fs::write(&old_audio, b"old wav").unwrap(); + std::fs::write(&recent_audio, b"recent wav").unwrap(); + + let old_path_static: &'static str = + Box::leak(old_audio.to_string_lossy().to_string().into_boxed_str()); + let recent_path_static: &'static str = + Box::leak(recent_audio.to_string_lossy().to_string().into_boxed_str()); + + insert_transcript( + &pool, + &minimal_transcript("t-purge-old", Some(old_path_static)), + ) + .await + .unwrap(); + insert_transcript( + &pool, + &minimal_transcript("t-purge-recent", Some(recent_path_static)), + ) + .await + .unwrap(); + + // Manually mark both trashed without going through delete_transcript + // (which would best-effort-remove the audio files itself). Old row + // is backdated past the 30-day retention; recent row is fresh. + sqlx::query("UPDATE transcripts SET deleted_at = datetime('now', '-60 days') WHERE id = ?") + .bind("t-purge-old") + .execute(&pool) + .await + .unwrap(); + sqlx::query("UPDATE transcripts SET deleted_at = datetime('now') WHERE id = ?") + .bind("t-purge-recent") + .execute(&pool) + .await + .unwrap(); + + let purged = purge_deleted_transcripts(&pool, 30).await.unwrap(); + assert_eq!(purged, 1, "only t-purge-old is past retention"); + + let old_exists: Option = + sqlx::query_scalar("SELECT id FROM transcripts WHERE id = ?") + .bind("t-purge-old") + .fetch_optional(&pool) + .await + .unwrap(); + assert!(old_exists.is_none(), "t-purge-old must be hard-deleted"); + assert!( + !old_audio.exists(), + "audio for hard-deleted row must be removed by purge" + ); + + let recent_exists: Option = + sqlx::query_scalar("SELECT id FROM transcripts WHERE id = ?") + .bind("t-purge-recent") + .fetch_optional(&pool) + .await + .unwrap(); + assert!( + recent_exists.is_some(), + "t-purge-recent within retention must survive" + ); + assert!( + recent_audio.exists(), + "audio for surviving in-retention row must NOT be removed by purge" + ); + } }