fix(dub): #1225 review — the preflight error had no class, and ENOENT no facts
Two P1s, both correct, and both the same shape as the bug being fixed:
- The preflight OSError I added ("Can't save the download: …") matched neither
an errno nor a download marker, so classify() returned "" and the user got
NO hint — the exact dead end this PR exists to remove. Reworded to carry
both signals; a test now asserts the class and that the hint names the data
directory.
- classify() covered ENOENT but _with_target_facts' own signature list did
not, so a job folder that vanished after preflight produced a
disk-classified error that never named the folder.
That second one is a drift class, not a one-off: two lists answering "is this
a disk problem?" will diverge again. They now share
failure.is_os_write_refusal(), with a test asserting both consumers agree
across all four errnos.
CHANGELOG entries shortened with refs last (CodeRabbit).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
8f4914272e
commit
027c08f1ff
+2
-2
@@ -14,8 +14,8 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently.
|
||||
|
||||
### Fixed
|
||||
|
||||
- Dub URL ingest: `[Errno 22] Invalid argument` was classified as the transcribe path's temp-file error, so the hint told users to check their system TEMP folder while the real failure was the job folder under the OmniVoice data directory; it now names that folder, its writability and the drive's free space, and says retrying the same link won't help (#1225) — thanks @dustmaker124-ui!
|
||||
- Dub URL ingest fails immediately with a clear message when the job folder is missing or unwritable, instead of starting a download that can only fail (#1225)
|
||||
- Dub URL ingest: a disk error now names the job folder, its writability and the drive's free space, instead of pointing at the system TEMP folder it never used — thanks @dustmaker124-ui! (#1225)
|
||||
- Dub URL ingest fails immediately when the job folder is missing or unwritable, instead of starting a download that can only fail (#1225)
|
||||
|
||||
## [0.4.0] — 2026-07-21
|
||||
|
||||
|
||||
+35
-11
@@ -254,17 +254,8 @@ def classify(reason: str) -> str:
|
||||
# under the OmniVoice data dir, not the system temp dir. Checked first so
|
||||
# a download's errno 22 stops being handed the transcribe path's
|
||||
# "check your TEMP folder" hint, which sends the user to the wrong place.
|
||||
if any(sig in low for sig in (
|
||||
"errno 22", "invalid argument",
|
||||
"errno 13", "permission denied",
|
||||
"errno 28", "no space left",
|
||||
"errno 2", "no such file or directory",
|
||||
)) and (
|
||||
"unable to download video" in low
|
||||
or "unable to open for writing" in low
|
||||
or "unable to rename file" in low
|
||||
or "yt_dlp" in low
|
||||
or "yt-dlp" in low
|
||||
if is_os_write_refusal(reason) and any(
|
||||
marker in low for marker in _DOWNLOAD_CONTEXT_MARKERS
|
||||
):
|
||||
return "VIDEO_DOWNLOAD_OS_ERROR"
|
||||
if "errno 22" in low:
|
||||
@@ -431,6 +422,39 @@ def diagnostic(*, reason: str, error_class: str, stage: str) -> str:
|
||||
return sanitize(block)
|
||||
|
||||
|
||||
# Signatures of the OS refusing a file operation. One list, because two
|
||||
# consumers must agree: ``classify`` (which picks the class + hint) and
|
||||
# ``dub_pipeline._with_target_facts`` (which decides whether to attach the
|
||||
# destination). When they drifted, an ENOENT download classified as a disk
|
||||
# problem but never got the folder named — the one fact that would have made
|
||||
# the message actionable (#1225 review).
|
||||
_OS_WRITE_REFUSAL_SIGNATURES = (
|
||||
"errno 22", "invalid argument",
|
||||
"errno 13", "permission denied",
|
||||
"errno 28", "no space left",
|
||||
"errno 2", "no such file or directory",
|
||||
"unable to open for writing",
|
||||
"unable to rename file",
|
||||
)
|
||||
|
||||
# Wording that places a failure in the video-download path specifically.
|
||||
_DOWNLOAD_CONTEXT_MARKERS = (
|
||||
"unable to download video",
|
||||
"unable to open for writing",
|
||||
"unable to rename file",
|
||||
"yt_dlp",
|
||||
"yt-dlp",
|
||||
)
|
||||
|
||||
|
||||
def is_os_write_refusal(reason: Optional[str]) -> bool:
|
||||
"""True when *reason* looks like the OS refusing a file operation (a full
|
||||
or removed drive, a read-only folder, an antivirus/cloud-sync lock) rather
|
||||
than a network or format failure. Signature match only; never raises."""
|
||||
low = (reason or "").lower()
|
||||
return any(sig in low for sig in _OS_WRITE_REFUSAL_SIGNATURES)
|
||||
|
||||
|
||||
def describe_path_target(path: str) -> str:
|
||||
"""Observable facts about where we were writing — "the folder does not
|
||||
exist", "the folder is not writable", "1,234 MB free on its drive".
|
||||
|
||||
@@ -510,12 +510,10 @@ def _with_target_facts(exc: BaseException, job_dir: str) -> BaseException:
|
||||
already about the remote side, and appending disk facts would just be
|
||||
noise. Never raises."""
|
||||
try:
|
||||
low = str(exc).lower()
|
||||
if not any(sig in low for sig in (
|
||||
"errno 22", "invalid argument", "errno 13", "permission denied",
|
||||
"errno 28", "no space left", "unable to open for writing",
|
||||
"unable to rename file",
|
||||
)):
|
||||
# Shared with failure.classify() so the "is this a disk problem?"
|
||||
# answer can't differ between the class we assign and whether we
|
||||
# bother naming the folder (#1225 review).
|
||||
if not failure.is_os_write_refusal(str(exc)):
|
||||
return exc
|
||||
facts = failure.describe_path_target(os.path.join(job_dir, "original.mp4"))
|
||||
if not facts:
|
||||
@@ -609,11 +607,14 @@ def yt_download_sync(
|
||||
# can already see it won't work.
|
||||
_target_facts = failure.describe_path_target(outtmpl)
|
||||
if "not writable" in _target_facts or "does not exist" in _target_facts:
|
||||
# Worded so classify() places it in the download path: it must carry
|
||||
# both an OS-refusal signature and download context, or the user gets
|
||||
# no hint at all — the failure this PR exists to fix (#1225 review).
|
||||
raise OSError(
|
||||
f"Can't save the download: {job_dir} ({_target_facts}). The video "
|
||||
f"downloads into this job folder under your OmniVoice data "
|
||||
f"directory — check it exists, is writable, and isn't locked by "
|
||||
f"antivirus or a cloud-sync client."
|
||||
f"Unable to download video: unable to open for writing in "
|
||||
f"{job_dir} ({_target_facts}). The video downloads into this job "
|
||||
f"folder under your OmniVoice data directory — check it exists, is "
|
||||
f"writable, and isn't locked by antivirus or a cloud-sync client."
|
||||
)
|
||||
ydl_opts: dict = {
|
||||
"outtmpl": outtmpl,
|
||||
|
||||
@@ -125,6 +125,33 @@ def test_exception_types_that_reject_a_message_still_get_the_text(tmp_path):
|
||||
assert isinstance(str(described), str)
|
||||
|
||||
|
||||
def test_enoent_download_failure_also_gets_the_destination_facts(tmp_path):
|
||||
"""Review finding (#1225): classify() covered ENOENT but the enrichment
|
||||
gate did not, so a job folder that vanished after preflight produced a
|
||||
disk-classified error that never named the folder — the one fact that
|
||||
makes it actionable. Both now read the same signature list."""
|
||||
exc = OSError("Unable to download video: [Errno 2] No such file or directory")
|
||||
described = dub_pipeline._with_target_facts(exc, str(tmp_path))
|
||||
assert str(tmp_path) in str(described)
|
||||
assert classify(str(described)) == "VIDEO_DOWNLOAD_OS_ERROR"
|
||||
|
||||
|
||||
def test_the_two_consumers_share_one_signature_list():
|
||||
"""A drift between "is this a disk problem?" (classify) and "should we name
|
||||
the folder?" (_with_target_facts) is what produced the finding above."""
|
||||
from core.failure import is_os_write_refusal
|
||||
|
||||
for reason in (
|
||||
"Unable to download video: [Errno 2] No such file or directory",
|
||||
"Unable to download video: [Errno 22] Invalid argument",
|
||||
"ERROR: unable to open for writing: [Errno 13] Permission denied",
|
||||
"Unable to download video: [Errno 28] No space left on device",
|
||||
):
|
||||
assert is_os_write_refusal(reason), reason
|
||||
assert classify(reason) == "VIDEO_DOWNLOAD_OS_ERROR", reason
|
||||
assert not is_os_write_refusal("Unable to download video: Connection reset by peer")
|
||||
|
||||
|
||||
def test_unwritable_destination_fails_before_yt_dlp_runs(tmp_path, monkeypatch):
|
||||
"""The preflight: don't start a download into a folder we can already see
|
||||
won't take the file."""
|
||||
@@ -145,7 +172,11 @@ def test_unwritable_destination_fails_before_yt_dlp_runs(tmp_path, monkeypatch):
|
||||
msg = str(excinfo.value)
|
||||
assert str(job_dir) in msg
|
||||
assert "not writable" in msg
|
||||
assert classify(msg) != "OS_INVALID_ARGUMENT"
|
||||
# Review finding (#1225): the preflight message classified as NOTHING, so
|
||||
# the user got no hint at all — the very failure mode this PR fixes. It
|
||||
# must carry both an OS-refusal signature and download context.
|
||||
assert classify(msg) == "VIDEO_DOWNLOAD_OS_ERROR"
|
||||
assert "data directory" in build_failure(excinfo.value, stage="download")["hint"]
|
||||
|
||||
|
||||
def test_preflight_lets_a_healthy_folder_through(tmp_path, monkeypatch):
|
||||
|
||||
Reference in New Issue
Block a user