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" + ); + } }