diff --git a/CHANGELOG.md b/CHANGELOG.md index 43f99721..57a67119 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently. ### Fixed - Every subprocess TTS engine would have turned a stereo render into noise: the mono downmix always averaged axis 0, which is time rather than channels for channels-last audio. Unreachable today since every engine returns mono, fixed in all five before it isn't. (#1328) +- Dubbing the same video twice no longer ties the second job's cloned voices to the first job's files — deleting the older dub from history was silently turning the newer one's single-segment regens into a default voice. (#1331) +- ...and deleting a dub whose files an existing saved dub still renders from now keeps those files on disk (the history entry still disappears) — protecting dubs created before this fix, whose references already cross directories. (#1331) - An unclean previous shutdown is no longer announced as a crash: the notice says what it actually knows, names the benign causes (sleep, force-quit, a stopped VM), and the one-click bug report is only offered when there is evidence to put in it — an empty report helps nobody. (#1375) - A first-use generate no longer fails at 300s while its model is still downloading: the download's own progress heartbeats now extend the generation budget (bounded), so a slow connection isn't reported as too-slow hardware. A job that goes silent still dies at the original deadline. (#1367) - A generation that hits its time limit now says so, instead of "an error OmniVoice doesn't recognize" followed by an empty `TimeoutError:`. It names the likely causes and the setting that raises the limit. (#1368) diff --git a/backend/api/routers/dub_core.py b/backend/api/routers/dub_core.py index 84249c4b..47cf2b83 100644 --- a/backend/api/routers/dub_core.py +++ b/backend/api/routers/dub_core.py @@ -252,13 +252,28 @@ def delete_single_dub_history(history_id: str): with db_conn() as conn: conn.execute("DELETE FROM dub_history WHERE id=?", (history_id,)) + # #1331 (deletion half): the content-hash cache points newer jobs' paths + # (vocals, and pre-fix clone refs) into this dir. Check BEFORE the row is + # deleted — the scan reads dub_history, and after _delete_row this row's + # neighbours are all that's left to consult either way. + holders = dub_pipeline.job_dir_referenced_by_others(history_id) + # Atomic with the evict — see purge_jobs (#1252 review). dub_pipeline.purge_jobs([history_id], delete_rows=_delete_row) safe = _safe_job_dir(history_id) - if safe and os.path.isdir(safe): + if holders: + # Keep the directory: another saved dub still renders from files in + # it. Disk is the cheap thing here; a job that silently loses its + # cloned voice on every regen is not. The row is gone, so the entry + # disappears from history either way. + logger.info( + "dub delete %s: history row removed but directory kept — still " + "referenced by job(s) %s (#1331)", history_id, ", ".join(holders), + ) + elif safe and os.path.isdir(safe): shutil.rmtree(safe, ignore_errors=True) event_bus.emit("dub_history", {"action": "deleted", "id": history_id}) - return {"deleted": True} + return {"deleted": True, "dir_kept_for": holders} @router.post("/preview/upload") async def preview_upload(video: UploadFile = File(...)): @@ -1265,10 +1280,18 @@ async def dub_transcribe_stream( "source": "speaker_clone", }) else: + # Clones are written into THIS job's dir, never alongside the + # vocals (#1331): on a content-hash cache hit vocals_path + # points into an OLDER job's dir, so dirname(vocals) wrote the + # new job's clone refs into a directory the user can delete by + # removing that older history entry — after which every + # single-segment regen silently rendered in the default voice. + _clone_dir = _safe_job_dir(job_id) or os.path.dirname(vocals_for_clone) + os.makedirs(_clone_dir, exist_ok=True) fut_clones = loop.run_in_executor( _cpu_pool, lambda: extract_speaker_clones( vocals_for_clone, final_segs, - os.path.dirname(vocals_for_clone), + _clone_dir, labels_source=labels_source, ), ) @@ -1307,10 +1330,17 @@ async def dub_transcribe_stream( try: from services.speaker_clone import extract_segment_refs seg_ids_for_clone = [s.get("id", i) for i, s in enumerate(final_segs)] + # Same #1331 rule as the per-speaker extraction above, and + # this is the DEFAULT path: per-segment references must + # live in THIS job's dir, or a cache-hit job's clips die + # with the older job they were written next to (both + # reviewers, on the first version of this fix). + _seg_clone_dir = _safe_job_dir(job_id) or os.path.dirname(vocals_for_clone) + os.makedirs(_seg_clone_dir, exist_ok=True) seg_clones = await loop.run_in_executor( _cpu_pool, lambda: extract_segment_refs( vocals_for_clone, final_segs, - os.path.dirname(vocals_for_clone), + _seg_clone_dir, seg_ids=seg_ids_for_clone, ), ) diff --git a/backend/services/dub_pipeline.py b/backend/services/dub_pipeline.py index 5d007cfd..4ca5f554 100644 --- a/backend/services/dub_pipeline.py +++ b/backend/services/dub_pipeline.py @@ -137,6 +137,45 @@ def safe_job_dir(job_id: str) -> Optional[str]: return candidate +def job_dir_referenced_by_others(job_id: str) -> "list[str]": + """History ids of OTHER jobs whose persisted paths point into ``job_id``'s + directory (#1331, the deletion half). + + The content-hash cache legitimately points a newer job's ``vocals_path`` + (and, for jobs created before the job-scoped-clones fix, its clone + reference paths) into an older job's directory. Deleting that older entry + used to ``rmtree`` the dir regardless, silently breaking the newer job: + single-segment regens fell back to the default voice, stems exports lost + their sources. The caller uses this to keep the DIRECTORY while still + deleting the history row — disk is the cheap thing here; another job's + voice is not. + + Scans persisted ``job_data`` as text for the dir prefix rather than + enumerating every path-bearing key: keys have grown before (vocals, + no_vocals, thumb, clone refs, segment refs) and a scan cannot fall behind + the schema. + """ + target = safe_job_dir(job_id) + if not target: + return [] + needle = target.rstrip(os.sep) + os.sep + # JSON-encoded job_data escapes backslashes, so match the Windows form too. + needle_json = needle.replace("\\", "\\\\") + holders: list[str] = [] + try: + with db_conn() as conn: + rows = conn.execute( + "SELECT id, job_data FROM dub_history WHERE id != ?", (job_id,) + ).fetchall() + except Exception: + return [] # no DB, nothing persisted can reference us + for row in rows: + data = row["job_data"] or "" + if needle in data or needle_json in data: + holders.append(row["id"]) + return holders + + def sse_event(event: str, payload) -> bytes: """Encode one Server-Sent Event frame. UTF-8 bytes, ready to yield.""" return f"event: {event}\ndata: {json.dumps(payload, ensure_ascii=False)}\n\n".encode("utf-8") diff --git a/tests/test_clone_refs_live_in_their_own_job_dir.py b/tests/test_clone_refs_live_in_their_own_job_dir.py new file mode 100644 index 00000000..eb970d0c --- /dev/null +++ b/tests/test_clone_refs_live_in_their_own_job_dir.py @@ -0,0 +1,172 @@ +"""A job's clone references must live in ITS directory, not a neighbour's (#1331). + +Reported from Discord (in Russian): re-dubbing a single sentence loses the +cloned voice; only a full re-dub keeps it. #1361 made the mechanism visible — +the reference path is handed to the engine but the file is gone. This is the +root cause for the identified class: + +On a content-hash cache hit (same video dubbed twice), ``find_cached_job`` +points the NEW job's ``vocals_path`` into the OLD job's directory — that is the +cache working as designed. But ``dub_core`` then extracted the new job's clone +references into ``os.path.dirname(vocals_path)``, i.e. **into the old job's +directory too**. Delete that older history entry — an ordinary, sanctioned +action that ``rmtree``s its dir — and every clone reference of the newer job +dangles. From then on each single-segment regen silently renders in the +default voice, and a full re-dub "fixes" it only because prep re-extracts. + +The fix is one argument: clones are written into the CURRENT job's own dir. +""" +from __future__ import annotations + +import ast +import importlib +import os +import sys + +import numpy as np +import pytest + +os.environ.setdefault("OMNIVOICE_MODEL", "test") +os.environ.setdefault("OMNIVOICE_DISABLE_FILE_LOG", "1") + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "backend")) + +sf = pytest.importorskip("soundfile") + + +def _extraction_call(): + """The ``extract_speaker_clones(...)`` call node in dub_core, by AST — + a comment mentioning the right directory must not satisfy this.""" + src = open( + os.path.join(os.path.dirname(__file__), "..", "backend", "api", + "routers", "dub_core.py"), + encoding="utf-8", + ).read() + for node in ast.walk(ast.parse(src)): + if (isinstance(node, ast.Call) + and getattr(node.func, "id", getattr(node.func, "attr", None)) + == "extract_speaker_clones"): + return node, src + raise AssertionError("extract_speaker_clones call not found in dub_core.py") + + +def test_the_out_dir_is_not_derived_from_the_vocals_path(): + """The bug, stated directly: ``os.path.dirname(vocals_for_clone)`` follows + the vocals into whichever job dir the cache resolved them from.""" + call, _ = _extraction_call() + out_dir_arg = ast.unparse(call.args[2]) + assert "dirname" not in out_dir_arg, ( + f"the clone out_dir is again derived from the vocals path " + f"({out_dir_arg!r}) — on a cache hit that is an OLDER job's directory, " + f"and deleting that job orphans this one's clone references (#1331)" + ) + + +def test_the_out_dir_comes_from_the_jobs_own_id(): + call, src = _extraction_call() + out_dir_arg = ast.unparse(call.args[2]) + # The argument is a variable; its assignment must resolve via the job id. + assert "_clone_dir" in out_dir_arg + assert "_safe_job_dir(job_id)" in src.split("fut_clones")[0].rsplit( + "extract_speaker_clones", 2)[0][-2000:] or "_safe_job_dir(job_id)" in src, ( + "the clone out_dir is no longer anchored to this job's own directory" + ) + + +def test_extraction_honours_an_out_dir_away_from_the_vocals(tmp_path): + """Functional half: vocals in one job's dir, clones requested in another's + — the written reference files must land in the requested dir, so deleting + the vocals-owning job cannot orphan them.""" + from services.speaker_clone import extract_speaker_clones + + old_job = tmp_path / "job-old" + new_job = tmp_path / "job-new" + old_job.mkdir() + new_job.mkdir() + + sr = 16000 + rng = np.random.default_rng(0) + audio = (rng.standard_normal(sr * 12) * 0.1).astype(np.float32) + vocals = old_job / "vocals.wav" + sf.write(vocals, audio, sr) + + segments = [ + {"start": 0.0, "end": 5.5, "text": "first line", "speaker_id": "Speaker 1"}, + {"start": 6.0, "end": 11.5, "text": "second line", "speaker_id": "Speaker 1"}, + ] + clones = extract_speaker_clones(str(vocals), segments, str(new_job)) + assert clones, "no clone extracted from 11s of speech" + for info in clones.values(): + ref = info["ref_audio"] + assert os.path.dirname(ref) == str(new_job), ( + f"clone reference written next to the vocals ({ref}) instead of " + f"into the requesting job's dir" + ) + assert os.path.isfile(ref) + + # The point of the whole exercise: the old job's dir can now die without + # taking the new job's voice with it. + import shutil + shutil.rmtree(old_job) + for info in clones.values(): + assert os.path.isfile(info["ref_audio"]) + + +def _all_extraction_calls(): + """BOTH extraction call sites in dub_core — the per-speaker one and the + per-segment one (the default). The first version of this fix covered only + the former; both reviewers caught the latter, which is how the class + survives a spot fix. Sweeping every call keeps a third copy honest too.""" + src = open( + os.path.join(os.path.dirname(__file__), "..", "backend", "api", + "routers", "dub_core.py"), + encoding="utf-8", + ).read() + calls = [ + node for node in ast.walk(ast.parse(src)) + if isinstance(node, ast.Call) + and getattr(node.func, "id", getattr(node.func, "attr", None)) + in ("extract_speaker_clones", "extract_segment_refs") + ] + assert len(calls) >= 2, "expected both extraction call sites in dub_core" + return calls + + +def test_no_extraction_call_derives_its_out_dir_from_the_vocals(): + for call in _all_extraction_calls(): + out_dir_arg = ast.unparse(call.args[2]) + assert "dirname" not in out_dir_arg, ( + f"an extraction call writes next to the vocals again " + f"({ast.unparse(call.func)}: {out_dir_arg!r}) — on a cache hit " + f"that is an older job's directory (#1331)" + ) + + +def test_segment_refs_honour_an_out_dir_away_from_the_vocals(tmp_path): + """Functional half for the DEFAULT path: per-segment clips land in the + requesting job's dir and survive the vocals-owning job's deletion.""" + from services.speaker_clone import extract_segment_refs + + old_job = tmp_path / "job-old" + new_job = tmp_path / "job-new" + old_job.mkdir() + new_job.mkdir() + + sr = 16000 + rng = np.random.default_rng(1) + audio = (rng.standard_normal(sr * 12) * 0.1).astype(np.float32) + vocals = old_job / "vocals.wav" + sf.write(vocals, audio, sr) + + segments = [ + {"start": 0.0, "end": 5.5, "text": "first line", "speaker_id": "Speaker 1"}, + {"start": 6.0, "end": 11.5, "text": "second line", "speaker_id": "Speaker 1"}, + ] + refs = extract_segment_refs(str(vocals), segments, str(new_job), seg_ids=[0, 1]) + assert refs, "no per-segment refs extracted from two 5.5s lines" + + import shutil + shutil.rmtree(old_job) + for info in refs.values(): + assert os.path.dirname(info["ref_audio"]) == str(new_job) + assert os.path.isfile(info["ref_audio"]) diff --git a/tests/test_deleting_a_dub_keeps_dirs_others_use.py b/tests/test_deleting_a_dub_keeps_dirs_others_use.py new file mode 100644 index 00000000..22433fa7 --- /dev/null +++ b/tests/test_deleting_a_dub_keeps_dirs_others_use.py @@ -0,0 +1,178 @@ +"""Deleting a dub must not destroy files another saved dub still renders from. + +The deletion half of #1331. The content-hash cache legitimately points a newer +job's ``vocals_path`` — and, for jobs created before the job-scoped-clones fix, +its clone reference paths — into an older job's directory. Deleting that older +history entry ``rmtree``'d the dir regardless: the newer job kept loading, but +every single-segment regen silently fell back to the default voice and stems +exports lost their sources. + +The guard is deliberately on the DELETE side even though new jobs no longer +write clone refs into neighbour dirs: existing users' jobs, created before that +fix, still carry cross-directory paths, and vocals are shared by design either +way. The history row still goes — the entry disappears from the UI — only the +directory survives while someone else needs it. Disk is the cheap thing here. +""" +from __future__ import annotations + +import importlib +import json +import os +import sys + +import pytest + +os.environ.setdefault("OMNIVOICE_MODEL", "test") +os.environ.setdefault("OMNIVOICE_DISABLE_FILE_LOG", "1") + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "backend")) + + +@pytest.fixture +def dp(tmp_path, monkeypatch): + """dub_pipeline with DUB_DIR + the history DB isolated to tmp_path.""" + mod = importlib.import_module("services.dub_pipeline") + dub_dir = tmp_path / "dub" + dub_dir.mkdir() + monkeypatch.setattr(mod, "DUB_DIR", str(dub_dir)) + monkeypatch.setattr(mod, "_DUB_DIR_REAL", os.path.realpath(str(dub_dir))) + + import sqlite3 + + db = sqlite3.connect(":memory:", check_same_thread=False) + db.row_factory = sqlite3.Row + db.execute("CREATE TABLE dub_history (id TEXT PRIMARY KEY, job_data TEXT)") + + class _Conn: + def __enter__(self): + return db + + def __exit__(self, *a): + db.commit() + + monkeypatch.setattr(mod, "db_conn", lambda: _Conn()) + return mod, db, dub_dir + + +def _add_job(db, job_id, job_data: dict): + db.execute( + "INSERT INTO dub_history (id, job_data) VALUES (?, ?)", + (job_id, json.dumps(job_data)), + ) + + +def test_a_dir_no_other_job_references_reports_no_holders(dp): + mod, db, dub_dir = dp + (dub_dir / "job-a").mkdir() + _add_job(db, "job-a", {"vocals_path": str(dub_dir / "job-a" / "vocals.wav")}) + _add_job(db, "job-b", {"vocals_path": str(dub_dir / "job-b" / "vocals.wav")}) + assert mod.job_dir_referenced_by_others("job-a") == [] + + +def test_a_cache_sharing_job_is_detected_as_a_holder(dp): + """The reported shape: job B's vocals live in job A's directory.""" + mod, db, dub_dir = dp + (dub_dir / "job-a").mkdir() + _add_job(db, "job-a", {"vocals_path": str(dub_dir / "job-a" / "vocals.wav")}) + _add_job(db, "job-b", {"vocals_path": str(dub_dir / "job-a" / "vocals.wav")}) + assert mod.job_dir_referenced_by_others("job-a") == ["job-b"] + + +def test_pre_fix_clone_refs_are_detected_too(dp): + """Jobs created before the job-scoped-clones fix carry clone reference + paths — not vocals — into the neighbour dir. The scan is textual over the + whole job_data precisely so it does not care which key holds the path.""" + mod, db, dub_dir = dp + (dub_dir / "job-a").mkdir() + _add_job(db, "job-a", {"vocals_path": str(dub_dir / "job-a" / "vocals.wav")}) + _add_job(db, "job-b", { + "vocals_path": str(dub_dir / "job-b" / "vocals.wav"), + "segment_clones": { + "7": {"ref_audio": str(dub_dir / "job-a" / "seg_7.wav")}, + }, + }) + assert mod.job_dir_referenced_by_others("job-a") == ["job-b"] + + +def test_the_job_being_deleted_does_not_count_as_its_own_holder(dp): + mod, db, dub_dir = dp + (dub_dir / "job-a").mkdir() + _add_job(db, "job-a", {"vocals_path": str(dub_dir / "job-a" / "vocals.wav")}) + assert mod.job_dir_referenced_by_others("job-a") == [] + + +def test_a_similar_prefix_is_not_a_false_positive(dp): + """job-a2's own dir starts with the string "job-a" — the needle carries a + trailing separator so prefix-sharing ids cannot collide.""" + mod, db, dub_dir = dp + (dub_dir / "job-a").mkdir() + _add_job(db, "job-a", {"vocals_path": str(dub_dir / "job-a" / "vocals.wav")}) + _add_job(db, "job-a2", {"vocals_path": str(dub_dir / "job-a2" / "vocals.wav")}) + assert mod.job_dir_referenced_by_others("job-a") == [] + + +def test_windows_style_json_escaped_paths_are_detected(dp): + """job_data is JSON, so Windows separators are stored escaped + (``C:\\\\...``). The scan must match that spelling as well, or the guard + silently never fires on the platform with the most reports.""" + mod, db, dub_dir = dp + (dub_dir / "job-a").mkdir() + _add_job(db, "job-a", {"vocals_path": "irrelevant"}) + # Simulate a Windows job_data blob referencing job-a's dir with + # backslash separators, JSON-escaped on disk. + win_path = str(dub_dir / "job-a").replace("/", "\\") + "\\vocals.wav" + db.execute( + "INSERT INTO dub_history (id, job_data) VALUES (?, ?)", + ("job-b", json.dumps({"vocals_path": win_path})), + ) + # safe_job_dir uses the real platform separator, so the full scan can't be + # driven cross-platform from a posix test host — pin the storage-format + # premise the needle_json branch exists for: JSON escapes backslashes. + raw = db.execute("SELECT job_data FROM dub_history WHERE id='job-b'").fetchone()[0] + assert (str(dub_dir / "job-a").replace("/", "\\") + "\\").replace("\\", "\\\\") in raw + + +def test_the_delete_endpoint_keeps_a_referenced_dir(dp, monkeypatch): + """End-to-end at the router level: the row goes, the dir stays, and the + response says for whom.""" + mod, db, dub_dir = dp + core = importlib.import_module("api.routers.dub_core") + + job_a = dub_dir / "job-a" + job_a.mkdir(exist_ok=True) + (job_a / "vocals.wav").write_bytes(b"RIFF") + _add_job(db, "job-a", {"vocals_path": str(job_a / "vocals.wav")}) + _add_job(db, "job-b", {"vocals_path": str(job_a / "vocals.wav")}) + + monkeypatch.setattr(core, "_safe_job_dir", mod.safe_job_dir) + monkeypatch.setattr(core, "db_conn", mod.db_conn) # endpoint's own import + monkeypatch.setattr(core.dub_pipeline, "purge_jobs", + lambda ids, delete_rows, **k: delete_rows()) + + result = core.delete_single_dub_history("job-a") + assert result == {"deleted": True, "dir_kept_for": ["job-b"]} + assert job_a.is_dir(), "the directory job-b renders from was deleted" + assert (job_a / "vocals.wav").is_file() + row = db.execute("SELECT id FROM dub_history WHERE id='job-a'").fetchone() + assert row is None, "the history row must still be removed" + + +def test_the_delete_endpoint_still_removes_an_unreferenced_dir(dp, monkeypatch): + """The guard must not turn every delete into a keep — an unshared job's + directory still dies with its row.""" + mod, db, dub_dir = dp + core = importlib.import_module("api.routers.dub_core") + + job_c = dub_dir / "job-c" + job_c.mkdir() + (job_c / "vocals.wav").write_bytes(b"RIFF") + _add_job(db, "job-c", {"vocals_path": str(job_c / "vocals.wav")}) + + monkeypatch.setattr(core, "_safe_job_dir", mod.safe_job_dir) + monkeypatch.setattr(core, "db_conn", mod.db_conn) + monkeypatch.setattr(core.dub_pipeline, "purge_jobs", + lambda ids, delete_rows, **k: delete_rows()) + + result = core.delete_single_dub_history("job-c") + assert result == {"deleted": True, "dir_kept_for": []} + assert not job_c.exists(), "an unreferenced dir must still be cleaned up"