From 64b4b976d6d20122443de2b1897a9d36b1a6253e Mon Sep 17 00:00:00 2001 From: SEPURI-SAI-KRISHNA Date: Tue, 4 Aug 2026 13:12:33 +0530 Subject: [PATCH] fix(versions): don't let a locked version file abort the delete cascade --- backend/services/history.py | 7 +- backend/services/versions.py | 20 +++++- backend/tests/test_profile_delete_cascade.py | 72 +++++++++++++++++--- 3 files changed, 87 insertions(+), 12 deletions(-) diff --git a/backend/services/history.py b/backend/services/history.py index 3d41d0ca..4acdeba5 100644 --- a/backend/services/history.py +++ b/backend/services/history.py @@ -4,6 +4,7 @@ Generation history management module. from typing import List, Optional, Tuple from datetime import UTC, datetime +import logging import uuid import shutil from pathlib import Path @@ -14,6 +15,8 @@ from ..models import GenerationRequest, GenerationResponse, HistoryQuery, Histor from ..database import Generation as DBGeneration, GenerationVersion as DBGenerationVersion, StoryItem as DBStoryItem, VoiceProfile as DBVoiceProfile from .. import config +logger = logging.getLogger(__name__) + def _delete_generation_children(generation_id: str, db: Session) -> None: """Remove the rows that reference a generation, plus any version audio files. @@ -334,7 +337,7 @@ async def delete_failed_generations(db: Session) -> int: except OSError: # Best-effort cleanup — don't abort the whole sweep # if a single file can't be removed. - pass + logger.warning("Could not delete generation audio %s", audio_path) db.delete(generation) count += 1 @@ -372,7 +375,7 @@ async def delete_generations_by_profile( except OSError: # A file locked by playback shouldn't abort the whole sweep # and leave the profile half-deleted. - pass + logger.warning("Could not delete generation audio %s", audio_path) # Delete from database db.delete(generation) diff --git a/backend/services/versions.py b/backend/services/versions.py index cbeb4b8b..367a57fc 100644 --- a/backend/services/versions.py +++ b/backend/services/versions.py @@ -8,6 +8,7 @@ version and any number of processed versions with different effects chains. from __future__ import annotations import json +import logging import uuid from pathlib import Path from typing import List, Optional @@ -21,6 +22,8 @@ from ..database import ( from ..models import GenerationVersionResponse, EffectConfig from .. import config +logger = logging.getLogger(__name__) + def _version_response(v: DBGenerationVersion) -> GenerationVersionResponse: """Convert a DB version row to a Pydantic response.""" @@ -185,7 +188,15 @@ def delete_version(version_id: str, db: Session) -> bool: def delete_versions_for_generation(generation_id: str, db: Session) -> int: - """Delete all versions for a generation (used when deleting a generation).""" + """Delete all versions for a generation (used when deleting a generation). + + This runs as part of a wider cascade — deleting one generation, sweeping + failed ones, or deleting a whole profile. A version file the OS won't let + us remove (locked by playback on Windows) must not abort that cascade and + strand the caller half-deleted, so the row goes regardless and the leaked + file is logged. ``delete_version`` keeps raising, because a single + user-initiated delete should fail loudly. + """ versions = ( db.query(DBGenerationVersion) .filter_by(generation_id=generation_id) @@ -195,7 +206,12 @@ def delete_versions_for_generation(generation_id: str, db: Session) -> int: for v in versions: audio_path = config.resolve_storage_path(v.audio_path) if audio_path is not None and audio_path.exists(): - audio_path.unlink() + try: + audio_path.unlink() + except OSError: + logger.warning( + "Could not delete version audio %s; removing the row anyway", audio_path + ) db.delete(v) count += 1 if count > 0: diff --git a/backend/tests/test_profile_delete_cascade.py b/backend/tests/test_profile_delete_cascade.py index aed2e84f..639e9ee6 100644 --- a/backend/tests/test_profile_delete_cascade.py +++ b/backend/tests/test_profile_delete_cascade.py @@ -145,6 +145,18 @@ async def test_delete_profile_clears_references_to_it(db): assert db.query(CaptureSettings).filter_by(id=1).one().default_playback_voice_id is None +def _lock(monkeypatch, locked: Path) -> None: + """Make ``locked`` refuse deletion, the way Windows does during playback.""" + real_unlink = Path.unlink + + def refuse_locked_file(self, *args, **kwargs): + if self == locked: + raise OSError("file is in use by another process") + return real_unlink(self, *args, **kwargs) + + monkeypatch.setattr(Path, "unlink", refuse_locked_file) + + @pytest.mark.asyncio async def test_delete_profile_survives_unremovable_audio(db, monkeypatch): """A file locked by playback must not abort the delete half-way through.""" @@ -153,20 +165,64 @@ async def test_delete_profile_survives_unremovable_audio(db, monkeypatch): db.add(Generation(id="gen-1", profile_id="profile-1", text="hello", audio_path=gen_stored)) db.commit() - real_unlink = Path.unlink - - def refuse_locked_file(self, *args, **kwargs): - if self == gen_file: - raise OSError("file is in use by another process") - return real_unlink(self, *args, **kwargs) - - monkeypatch.setattr(Path, "unlink", refuse_locked_file) + _lock(monkeypatch, gen_file) assert await profiles.delete_profile("profile-1", db) is True assert db.query(VoiceProfile).count() == 0 assert db.query(Generation).count() == 0 +@pytest.mark.asyncio +async def test_delete_profile_survives_unremovable_version_audio(db, monkeypatch): + """Same guarantee for version files, which are unlinked one call deeper.""" + _make_profile(db) + _, gen_stored = _write_wav("gen.wav") + version_file, version_stored = _write_wav("gen-reverb.wav") + db.add(Generation(id="gen-1", profile_id="profile-1", text="hello", audio_path=gen_stored)) + db.add( + GenerationVersion( + id="version-1", + generation_id="gen-1", + label="Reverb", + audio_path=version_stored, + ) + ) + db.commit() + + _lock(monkeypatch, version_file) + + assert await profiles.delete_profile("profile-1", db) is True + assert db.query(VoiceProfile).count() == 0 + assert db.query(Generation).count() == 0 + assert db.query(GenerationVersion).count() == 0 + + +@pytest.mark.asyncio +async def test_one_locked_file_does_not_strand_the_rest(db, monkeypatch): + """The sweep commits per generation, so aborting mid-loop half-deletes.""" + _make_profile(db) + for i in range(3): + _, gen_stored = _write_wav(f"gen{i}.wav") + _, version_stored = _write_wav(f"gen{i}-take2.wav") + db.add(Generation(id=f"gen-{i}", profile_id="profile-1", text="hi", audio_path=gen_stored)) + db.add( + GenerationVersion( + id=f"version-{i}", + generation_id=f"gen-{i}", + label="take-2", + audio_path=version_stored, + ) + ) + db.commit() + + _lock(monkeypatch, config.get_generations_dir() / "gen1-take2.wav") + + assert await profiles.delete_profile("profile-1", db) is True + assert db.query(Generation).count() == 0 + assert db.query(GenerationVersion).count() == 0 + assert db.query(VoiceProfile).count() == 0 + + @pytest.mark.asyncio async def test_delete_profile_leaves_other_profiles_alone(db): """Cleanup is scoped to the deleted profile."""