upstream-archive/VoiceStudio
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
13d3f3fb139d2049bd91c075287185632fe3def8
VoiceStudio/tests/test_dub_subtitles_309.py
T
Palash Debnath 73ebd6518d fix(errors): six failures that reached users as raw text (#1262, #1256, #1251, #1247, #1257, #1254) (#1264) 
* fix(errors): four failures that reached users as raw OS text (#1262, #1256, #1251, #1252)

#1262 — a voice profile named in any non-latin-1 script 500'd every download
endpoint with "'latin-1' codec can't encode characters in position 22-25".
`attachment; filename="` is exactly 22 characters, so those were the first
four characters of the user's own name. The sanitisers in front of the header
filtered with str.isalnum(), which is True for every alphabetic script — they
stripped punctuation and passed exactly what breaks the header. Ten sites, one
RFC 6266 builder, plus a guard so an eleventh can't be hand-written.

#1256 — a synth died on FileNotFoundError: 'ffprobe' and was reported as "an
error OmniVoice doesn't recognize", on a Mac where the app's own ffprobe was
resolvable the whole time. Our call sites pass explicit paths; a dependency
shelling out by bare name does not. The resolved directories are now published
on PATH, and the failure is classified either way.

#1251 — "The paging file is too small" reached the user as a bare 500. It was
already counted as an OOM, but that remedy (close apps, lighter engine) is
wrong on a 32 GB machine — the fix is a Windows setting, and the hint now says
which. Matched on the code in both the Python and Rust spellings.

#1252/#1253 — deleting a dub mid-import crashed it with `ingest: 'mgw39lx3'`:
str(KeyError) is the repr of the key. The pipeline blind-subscripted a job that
DELETE /dub/history/{id} had popped minutes earlier. It now stops quietly, and
no exception whose str() is a bare value can present itself that way again.

* fix(engines): Unload 400'd, a wrong language said nothing, DRM was retried by hand (#1247, #1257, #1254)

#1247 — list_loaded() advertises in-process engines as `engine:<id>` with
"unloadable": true, but unload() only ever handled tts/diarization/sidecars.
The panel was rendering a button for ids the dispatcher rejected. The engines
already implement unload(); only the routing was missing. The contract test
written for it immediately found a second instance — `capture-asr`, listed the
same way with no branch either — which is why it enumerates the listing rather
than hard-coding ids.

#1257 — MLXAudioBackend.supported_languages() returns ["multi"] on the stated
assumption that "each engine silently ignores languages it doesn't know". It
doesn't; the library raises. So the picker offers all 646 languages and the
rejection arrived as a bare list of 23 codes, naming neither the engine nor the
way out. Enumerating each model's real language set would be a brittle map that
goes stale every engine update — name the engine and the fix instead.

#1254 — reported as intermittent: the same URL failed as DRM-protected, then
succeeded on retry. Real DRM doesn't lapse; the player client varies. That is
the same shape as the 403 case which already escalates through
_YT_PLAYER_CLIENTS, so DRM now routes into it. If every client still refuses,
the failure is classified instead of arriving as a raw yt-dlp line.

* fix(review): close the delete race, narrow the tool match, sanitize the fallback

Greptile P1 + CodeRabbit Major — verified real, and mine: splitting merge from
save left a window where a delete lands between them, so the pending save
UPSERTs the row straight back and a dub the user deleted reappears. Now one
atomic step under _dub_jobs_lock, with both delete endpoints purging rows and
memory under that same lock. That also fixed DELETE /dub/history, which
deleted every row but evicted nothing — an in-flight job survived 'clear
history' outright and re-saved itself on completion.

CodeRabbit Minor (#1256): the media-tool match accepted any message ending in
'ffmpeg'/'ffprobe', so a missing FILE at /tmp/ffmpeg got the 'repair your
media engine' remedy. Now requires the name unquoted-and-unqualified.

CodeRabbit Major (#1262): `fallback` reached the header verbatim whenever the
real name folded away entirely, walking past every guard the name goes
through. Folded like the name.

CodeRabbit Major (#1256): the PATH log printed resolved directories, and a
user-set FFMPEG_PATH sits under their home. Logs a count now.

CodeRabbit Minor (#1262): the subtitle-route assertion also passed against the
pre-fix header; it now asserts filename*= too.

Skipped: 'Highlights bullets must end with (#N)'. CLAUDE.md scopes that to the
### subsections; none of the seven pre-existing highlights carry refs, and
tests/test_changelog_style.py encodes the rule already.

* fix(review): the remaining unlocked save paths, an over-broad signature, two weak tests

Greptile P1 — the mid-pipeline put_job + save_job pairs were still unlocked, so
a clear-history landing between them left a ghost row behind the purge. Both go
through put_and_save_job now; only the final completion gate decides whether a
withdrawn job's work is kept.

CodeRabbit Major (#1257) — 'unsupported language' as a bare prefix also matches
'Unsupported language model configuration', handing a model/config failure
engine-switch advice it has no use for. The loose wordings now require the
rejected thing to be a code or to end there.

CodeRabbit Major (#1257) — the OOM test asserted on SOURCE TEXT, which passes
even if the call is unreachable or its result discarded; #1224 taught this same
lesson on this codebase. Both it and the language rewrite now drive the real
_run_backend_inference with a raising backend.

CodeRabbit Minor (#1257) — 'or "engine" in message' always passed, since the
production template contains the word. Asserts the resolved class name now.

CodeRabbit Major (#1252) — the delete-race test deleted the job BEFORE the
merge, which only re-tested the absent case and would pass with the two steps
still split. It now interleaves a real second thread against a slow save.

CodeRabbit Major (#1256) — a hardcoded /tmp literal trips Ruff S108; built from
tmp_path instead.

* fix(review): a withdrawal must survive the job's first write

CodeRabbit Major — the concern is real, though its suggested fix (gate the
checkpoint on the job already existing) would break creation: an ingest's FIRST
persistence is what creates the entry, so that gate would never pass.

The actual defect is that dict membership cannot express 'withdrawn'. An absent
key means 'not written yet' for a new job and 'deleted' for an established one
— two opposite instructions from one signal. So a clear-history arriving before
the first checkpoint was silently undone by that checkpoint recreating the row,
and the run then persisted its result into history the user had just cleared.

Tombstone it explicitly: the ingest declares itself in flight, a purge marks any
in-flight id withdrawn, and both write paths refuse a withdrawn id. Released in
, so it's bounded by concurrent ingests and can't poison a later run
that reuses the id.

That also fixed clear-history properly: a job with no row yet appears in no id
list, so only an in-flight sweep can catch it.

CodeRabbit Minor — my race test waited on an event that could not be set while
the save held the lock, so it burned its full 2s timeout every run and
synchronised nothing. It now waits for the purge thread to REACH the purge.

* test(dub): the race test was not testing the race

Caught by verifying fail-before rather than trusting the test: splitting merge
from save — the exact resurrection bug — passed all 22 tests.

The assertions checked WHAT happened (the save ran, the row was deleted, the
job left memory) but never WHEN. A save landing after the delete is
indistinguishable from one landing before if you only assert that both
occurred — and 'after' is precisely the resurrection.

Now recorded and asserted as an order. With merge+save atomic the purge cannot
start until the save finishes, so the sequence is always save-then-delete;
split them and it fails with ['delete', 'save'].

That is the second time this test needed rewriting: v1 deleted the job before
the merge and only re-checked the absent case, v2 interleaved a real thread but
asserted the wrong thing. Both looked like tests.

Also documents why the DB write sits inside the lock (atomicity beats a rare
5 s sqlite busy-timeout stall) and that no locked region calls another, so the
non-reentrant lock cannot deadlock — verified by walking every locked region.

* fix(dub): gate the withdrawal at save_job, not at its callers

Greptile P1 — and the same class I'd already fixed, unfixed elsewhere. The
withdrawal check sat in the two ingest helpers, but eight direct save_job call
sites across dub generate / translate / export / core bypass those entirely.
Deleting a dub mid-RENDER therefore still resurrected it, which is at least as
likely as deleting mid-import.

Moved the gate into save_job itself: one choke point, every caller inherits it,
and the ninth cannot forget. That needs a re-entrant lock, since the atomic
helpers call save_job while already holding it — a plain Lock would deadlock
the backend, so a test pins the lock type and another exercises the nested path.

Verified fail-before: removing the gate fails the new test.

* fix(dub): the withdrawal only covered ingests, so it covered almost nothing

Caught by testing the reported scenario directly instead of trusting a green
suite: CI passed, 26 tests passed, and a dub deleted during a RENDER was still
resurrected.

The tombstone was scoped to in-flight ingests. But a dub is imported once and
rendered many times, so the realistic delete lands during a render — long after
its ingest ended — and end_ingest was CLEARING the tombstone at exactly that
point. The rare case was protected and the common one left open.

Now scoped to deletions, not ingests. Kept in a bounded LRU rather than cleared
on completion, because there is no moment at which a delete stops mattering:
any operation still holding that job can persist it. Re-importing an id is the
only thing that legitimately revives it.

Verified fail-before: the previous scoping fails three of the new tests.

* refactor: move the dub delete-resurrection fix to its own PR (#1270)

The six fixes left here are independent error-message changes that needed no
corrections. The dub concurrency change needed five rounds, each finding
something real in work that was already reviewed, tested and CI-green — the
last of them being that the fix did not fix the reported case at all.

Riding a release on that record is a bad trade, so it ships separately as
#1270. This branch keeps #1262, #1256, #1251, #1247, #1257 and #1254; the
KeyError message half goes with the dub PR, since it is that issue's other
half.
2026-07-26 12:39:29 -07:00

221 lines
9.9 KiB
Python
Raw Blame History

"""Regression tests for issue #309 — dubbed subtitles export/burn-in.
Two symptoms, one root cause each:
1. Subtitle burn-in / SRT / VTT used the original-language ASR transcript
(``job["segments"]``) because the translated text only ever lived in the
``/dub/generate`` request payload. ``_sync_job_segments`` now persists the
generated segments back onto the job.
2. "Save error: Unexpected non-whitespace character after JSON" — the Tauri
save dialog appended ``?save_path=…`` to every export URL and parsed the
response as JSON, but ``/dub/srt`` and ``/dub/vtt`` ignored the param and
returned the raw subtitle body (which starts with the cue index ``1``, a
valid JSON document, followed by the timestamp line → parse error at
line 2 column 1). Fixed on the frontend: subtitles are fetched raw and
written by the Tauri process via ``save_text_file`` (the OS save dialog is
the write authorization); the endpoints stay raw-body-only.
"""
import os
import uuid
import pytest
os.environ.setdefault("OMNIVOICE_MODEL", "test")
# ---------------------------------------------------------------------------
# Fixtures
# ---------------------------------------------------------------------------
@pytest.fixture()
def client():
from fastapi.testclient import TestClient
from main import app
return TestClient(app, client=("127.0.0.1", 50000))
@pytest.fixture()
def translated_job():
"""A dub job whose segments carry source text + translated text, the way
they look after transcribe → translate → generate (post-#309 sync)."""
from services.dub_pipeline import _dub_jobs
job_id = str(uuid.uuid4())[:8]
job = {
"video_path": "/nonexistent/original.mp4",
"duration": 3.0,
"filename": "test_video.mp4",
"segments": [
{"id": "a1", "start": 0.0, "end": 1.0, "text": "Hello world",
"text_original": "shalom olam", "speaker_id": "Speaker 1"},
{"id": "a2", "start": 1.0, "end": 2.0, "text": "How are you",
"text_original": "ma shlomcha", "speaker_id": "Speaker 1"},
],
"dubbed_tracks": {"en": {"path": "/nonexistent/dubbed_en.wav",
"language": "English", "language_code": "en"}},
}
_dub_jobs[job_id] = job
yield job_id, job
_dub_jobs.pop(job_id, None)
def _make_req(texts, seg_ids=None, **kwargs):
from schemas.requests import DubRequest, DubSegment
segs = [
DubSegment(start=float(i), end=float(i) + 1.0, text=t)
for i, t in enumerate(texts)
]
return DubRequest(segments=segs, segment_ids=seg_ids, **kwargs)
# ---------------------------------------------------------------------------
# Symptom 1 — translated text must reach job["segments"] (burn-in source)
# ---------------------------------------------------------------------------
class TestSyncJobSegments:
def test_translated_text_replaces_source_text(self):
from api.routers.dub_generate import _sync_job_segments
job = {"segments": [
{"id": "a1", "start": 0.0, "end": 1.0, "text": "shalom olam", "speaker_id": "Speaker 1"},
{"id": "a2", "start": 1.0, "end": 2.0, "text": "ma shlomcha", "speaker_id": "Speaker 2"},
]}
req = _make_req(["Hello world", "How are you"], seg_ids=["a1", "a2"])
_sync_job_segments(job, req)
assert [s["text"] for s in job["segments"]] == ["Hello world", "How are you"]
# Source text preserved for dual-subtitle layouts.
assert [s["text_original"] for s in job["segments"]] == ["shalom olam", "ma shlomcha"]
# Metadata carried over from the matched existing segment.
assert [s["speaker_id"] for s in job["segments"]] == ["Speaker 1", "Speaker 2"]
assert [s["id"] for s in job["segments"]] == ["a1", "a2"]
def test_existing_text_original_never_clobbered(self):
from api.routers.dub_generate import _sync_job_segments
job = {"segments": [
{"id": "a1", "start": 0.0, "end": 1.0, "text": "Hallo Welt",
"text_original": "shalom olam"},
]}
# Second generate pass (e.g. re-translate to English): text_original
# must stay the ASR source, not the previous translation.
req = _make_req(["Hello world"], seg_ids=["a1"])
_sync_job_segments(job, req)
assert job["segments"][0]["text"] == "Hello world"
assert job["segments"][0]["text_original"] == "shalom olam"
def test_index_fallback_without_segment_ids(self):
from api.routers.dub_generate import _sync_job_segments
job = {"segments": [
{"start": 0.0, "end": 1.0, "text": "shalom olam", "speaker_id": "Speaker 1"},
]}
req = _make_req(["Hello world"]) # no segment_ids
_sync_job_segments(job, req)
assert job["segments"][0]["text"] == "Hello world"
assert job["segments"][0]["text_original"] == "shalom olam"
assert job["segments"][0]["speaker_id"] == "Speaker 1"
def test_split_segments_extend_beyond_existing(self):
from api.routers.dub_generate import _sync_job_segments
job = {"segments": [
{"id": "a1", "start": 0.0, "end": 2.0, "text": "shalom olam"},
]}
req = _make_req(["Hello", "world"], seg_ids=["a1", "a1b"])
_sync_job_segments(job, req)
assert len(job["segments"]) == 2
assert job["segments"][1]["text"] == "world"
assert job["segments"][1]["id"] == "a1b"
def test_empty_request_keeps_existing_segments(self):
from api.routers.dub_generate import _sync_job_segments
from schemas.requests import DubRequest
job = {"segments": [{"id": "a1", "start": 0.0, "end": 1.0, "text": "keep me"}]}
_sync_job_segments(job, DubRequest(segments=[]))
assert job["segments"][0]["text"] == "keep me"
def test_request_timing_wins(self):
from api.routers.dub_generate import _sync_job_segments
job = {"segments": [{"id": "a1", "start": 0.0, "end": 1.0, "text": "x"}]}
req = _make_req(["y"], seg_ids=["a1"])
req.segments[0].start = 0.5
req.segments[0].end = 1.5
_sync_job_segments(job, req)
assert job["segments"][0]["start"] == 0.5
assert job["segments"][0]["end"] == 1.5
class TestBurnSrtUsesTranslatedText:
def test_burn_srt_contains_translated_not_source(self, tmp_path, translated_job):
from api.routers.dub_export import _write_burn_srt
_, job = translated_job
sub_path = _write_burn_srt(job, str(tmp_path), "stamp", dual=False)
content = open(sub_path, encoding="utf-8").read()
assert "Hello world" in content
assert "How are you" in content
assert "shalom olam" not in content # source text must not burn in
def test_burn_srt_dual_stacks_original_below(self, tmp_path, translated_job):
from api.routers.dub_export import _write_burn_srt
_, job = translated_job
sub_path = _write_burn_srt(job, str(tmp_path), "stamp", dual=True)
content = open(sub_path, encoding="utf-8").read()
assert "Hello world\n<i>shalom olam</i>" in content
# ---------------------------------------------------------------------------
# Symptom 2 — SRT/VTT are raw text bodies the Tauri side writes itself.
# The frontend fetches the body and saves it via the save_text_file command,
# so the backend must NOT grow a ?save_path= variant here (it would be a
# user-controlled filesystem write on the loopback HTTP surface).
# ---------------------------------------------------------------------------
class TestSubtitleSaveResponseShape:
def test_srt_save_path_param_is_inert(self, client, translated_job, tmp_path):
# A stray ?save_path= (e.g. an old frontend) must neither write the
# file nor change the response shape.
job_id, _ = translated_job
dest = tmp_path / "subs.srt"
res = client.get(f"/dub/srt/{job_id}", params={"save_path": str(dest)})
assert res.status_code == 200
assert not res.headers["content-type"].startswith("application/json")
assert "Hello world" in res.text
assert not dest.exists()
def test_srt_dual_includes_original(self, client, translated_job):
job_id, _ = translated_job
res = client.get(f"/dub/srt/{job_id}", params={"dual": 1})
assert res.status_code == 200
assert "Hello world" in res.text
assert "shalom olam" in res.text # dual layout includes original
def test_srt_filename_route_returns_text(self, client, translated_job):
# The Export drawer uses the /{filename} route variant.
job_id, _ = translated_job
res = client.get(f"/dub/srt/{job_id}/subtitles_en.srt")
assert res.status_code == 200
assert res.text.startswith("1\n")
disposition = res.headers.get("content-disposition", "")
# Asserted on content, not on position: since #1262 the header also
# carries an RFC 5987 `filename*=` so non-latin-1 names don't 500, and
# that parameter comes last. Both parameters are asserted — checking
# only `filename=` would pass against the pre-#1262 header too.
assert disposition.startswith("attachment;")
assert '.srt"' in disposition
assert "filename*=UTF-8''" in disposition
assert disposition.endswith(".srt")
def test_plain_srt_get_still_returns_text_body(self, client, translated_job):
job_id, _ = translated_job
res = client.get(f"/dub/srt/{job_id}")
assert res.status_code == 200
assert not res.headers["content-type"].startswith("application/json")
assert res.text.startswith("1\n")
assert "Hello world" in res.text
def test_plain_vtt_get_still_returns_text_body(self, client, translated_job):
job_id, _ = translated_job
res = client.get(f"/dub/vtt/{job_id}")
assert res.status_code == 200
assert res.text.startswith("WEBVTT")
assert "text/vtt" in res.headers["content-type"]
Reference in New Issue View Git Blame Copy Permalink