feat(hardening): six recurrence guards from the closed-issue-history audit (#1141)
* feat(hardening): six recurrence guards from the closed-issue-history audit An agent audit swept every closed issue, clustered the error classes, and checked each for fix + regression test + upgrade/reinstall survival. Six of the "fixed but fragile" gaps are closed here; each guard has a regression test in tests/test_recurrence_hardening.py (9 tests). 1. Evict-then-load (class 1, ~90 issues): a plain TTS load on a tight unified-memory box could still be OS-killed — the dub path frees memory before ASR loads (#1119) but nothing did before a TTS load. _make_room_before_tts_load() releases the idle capture-ASR model, clone prompts, and allocator caches when free RAM < the unified headroom. Deliberately NOT admission control: the #1111 decision (advisory-only, never refuse a load on an estimate) stands; this only does earlier what idle reclaim does later, and roomy machines skip it entirely. 2. Honest SIGKILL attribution (class 1): crashCauseHint() says "the OS ran out of memory (RAM)" for signal 9 instead of guessing VRAM on machines that have none. VRAM guidance kept for real GPU aborts (signal 6 etc.). 3. Clone-kind save sanitize (class 3, recurred 3x): the server-side instruct heal was gated to design-kind; a clone profile saved by any bypassing client could persist prose that 400s on every use. profiles.py now sanitizes both kinds at the single choke point. 4. Stale user_env validation (class 5): ~/.config/omnivoice/env is inherited verbatim by reinstalls; path-valued keys (OMNIVOICE_CACHE_DIR/DATA_DIR) that don't exist and can't be created are dropped for the run with a loud log line (file untouched — replugging the drive restores the setting). The two #480 precedence tests updated to use creatable paths (they test precedence, not path validity). 5. omni_ui schema guard (class 6): sanitizeOmniUi() whitelists + shape-checks every persisted field before restore — one malformed field used to throw mid-restore and silently discard everything after it, and every future field re-opened the #1067 class. Includes a lockstep test failing when useAppData reads a field missing from the schema. 6. safe_replace EXDEV helper (class 7): os.replace across devices raises EXDEV (the Windows D:-drive Errno 18/22 class); utils/fsops.safe_replace degrades to copy+fsync+replace. Adopted at the two cross-directory movers (log rotation, persona restore); temp-sibling writers stay on os.replace. Plus: the generate timeout scales with text length (class 4's 503 wave — +1s per 40 chars past the first 1200, env floor respected), so long texts on slow hardware stop dying at exactly 300s with a "set an env var" remedy. Deliberately NOT done, with reasons: - ASR auto-promotion to the crash-isolated engine after a wedge: the code records an explicit owner rule against silent engine switching (asr_backend.py "we never switch engines automatically") — flagged to the owner instead of overridden. - Rust items (webview cache-clear unit test, crash-marker versioning across updates): deferred to their own PR — the local cargo target was reclaimed for disk space, so they can't be verified locally right now. Full suite: 2999 backend + 1243 frontend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(changelog): correct PR ref to #1141 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(hardening): review round — reclaim at the shared load boundary, write-probe path validation Both Greptile P1s were real: - "Startup preload skips reclaim": _make_room_before_tts_load() ran only in get_model(); preload_model() calls _load_model_with_timeout() directly, so a memory-tight machine was protected on demand loads but could still be OS-killed during the startup preload — the exact window the guard exists for. The reclaim now lives in _load_model_with_timeout(), the boundary both callers share. - "Read-only paths pass validation": an existing directory on a read-only mount passes makedirs+isdir but fails on first real use, so the stale setting survived validation only to break downloads later. The check now probes actual write capability (create+delete a probe file). New test with a chmod-0o500 dir (skipped under root, where the probe cannot fail). - CodeQL: the two intentional best-effort excepts in fsops.py now carry their explanatory comments. Full suite: 3000 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: mergetest <nizam4103@gmail.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Palash Debnath
co-authored by
Claude Opus 4.8
mergetest
Claude Opus 4.8
mergetest
parent
dc3527ab05
commit
d6f24dafd5
@@ -14,6 +14,8 @@ The bundled TTS model package (`pyproject.toml`) is versioned independently.
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Six recurrence guards from a full audit of the project's issue history — aimed at "this bug can never come back, even after an update or reinstall."** (1) Before loading the voice model on a memory-tight machine, the app now *first releases* things it already reclaims on idle (the warm dictation model, allocator caches) — the missing half of the 16 GB OOM-kill fix; roomy machines pay nothing. (2) When the operating system force-kills the backend for running out of RAM, the crash notice now says exactly that instead of blaming "VRAM" on machines that have none. (3) Saving a *cloned* voice with free-form text in its delivery field can no longer persist a profile that errors on every future generation — the server now sanitizes all profile kinds, closing a hole that had been re-exploited three times through different clients. (4) A reinstall that inherits an old settings file pointing at an unplugged drive or deleted folder no longer sends downloads into the void — dead paths are ignored for the run with a clear log line. (5) Locally-saved UI state is now schema-checked as a whole on restore, so one corrupted field can't silently discard everything after it (the general form of the "app got empty" fix). (6) File moves across drives (Windows D:-drive installs) get a dedicated safe-move helper, so the next code path that renames across devices degrades gracefully instead of failing with `[Errno 18]`. Long texts also get a generation time budget that scales with their length instead of a fixed five minutes. (#1141)
|
||||
|
||||
- **Dubbed videos get their stereo back — and the music's full frequency range.** A/B-measuring a dub against its original showed the dubbed audio was **mono in a stereo container** (channel correlation 1.000 vs the original's 0.754) — the entire stereo image of the music, gone. Two causes, both fixed: the separation step was being fed the **16 kHz mono** file extracted for transcription — so the music bed inherited mono *and* an 8 kHz ceiling at the source — and the mixer then let the mono voice drag the whole mix down to mono. Ingest now makes a second, full-quality stereo extraction (44.1 kHz) just for separation, transcription keeps its mono file, and the mixer pins both sides to stereo with the voice dead-center where dubbed dialogue belongs. Loudness already matched the original (−17.2 vs −17.8 LUFS, measured); now the width and brightness do too. (#1138)
|
||||
|
||||
- **Dubbed lines that finish early no longer leave dead air — they now speak at the pace of the scene.** Translations routinely come out shorter than the original delivery, and the dub used to just stop early: measured on a real dub, **8.8 of 18.7 seconds of speech time had no voice at all** — the mouth kept moving on screen over the thin residue the vocal separation leaves behind, which reads as silence and as "the music got quiet". Short lines are now gently slowed toward their time slot (pitch preserved, never below 0.85× — comfortably natural), so speech covers the speaking time the way the original did. This also does most of the work people expect from "lip sync": the voice now starts *and ends* with the mouth. Near-full lines are left untouched, the per-segment badge shows the applied rate, and `OMNIVOICE_UNDERRUN_MIN_RATE=1.0` turns the fill off. (#1137)
|
||||
|
||||
@@ -439,6 +439,22 @@ def _oom_friendly_reraise(e):
|
||||
) from e
|
||||
|
||||
|
||||
def _generate_timeout_s(text: str) -> float:
|
||||
"""Wall-clock budget for one generate, scaled to the request.
|
||||
|
||||
The fixed OMNIVOICE_GENERATE_TIMEOUT_S (300s) was sized for typical
|
||||
requests on a GPU — a legitimately long text on a slow CPU box times out
|
||||
with the exact user-facing 503 the audit flagged as a recurring class
|
||||
(#1033/#1037 wave), and the remedy was "go set an env var". Scale the
|
||||
budget with input size instead: the floor stays the configured value, and
|
||||
long inputs get 1 extra second per 40 characters — generous enough for
|
||||
CPU-class hardware, still bounded (a wedged job is caught in minutes, not
|
||||
hours). An explicit OMNIVOICE_GENERATE_TIMEOUT_S remains the floor/knob.
|
||||
"""
|
||||
from services.model_manager import GPU_JOB_TIMEOUT_S
|
||||
return max(GPU_JOB_TIMEOUT_S, GPU_JOB_TIMEOUT_S + (max(0, len(text or "") - 1200) / 40.0))
|
||||
|
||||
|
||||
def _run_inference(
|
||||
model, text, language, ref_audio_path, ref_text, instruct, duration,
|
||||
num_step, guidance_scale, speed, t_shift, denoise,
|
||||
@@ -1263,6 +1279,7 @@ async def generate_speech(
|
||||
max_chunk_chars, crossfade_ms,
|
||||
),
|
||||
what="TTS generate",
|
||||
timeout=_generate_timeout_s(text),
|
||||
)
|
||||
# Read after generation: engines with lazy model loading report
|
||||
# their real rate only once weights are up.
|
||||
@@ -1278,6 +1295,7 @@ async def generate_speech(
|
||||
max_chunk_chars, crossfade_ms,
|
||||
),
|
||||
what="TTS generate",
|
||||
timeout=_generate_timeout_s(text),
|
||||
)
|
||||
sample_rate = _model.sampling_rate
|
||||
# Watermark → save → history → prune → emit, shared with the streaming
|
||||
|
||||
@@ -16,6 +16,8 @@ import asyncio
|
||||
import functools
|
||||
import logging
|
||||
import os
|
||||
|
||||
from utils.fsops import safe_replace
|
||||
import time
|
||||
import uuid
|
||||
|
||||
@@ -276,7 +278,7 @@ def _rename_for_new_id(written: list[str], new_id: str) -> list[str]:
|
||||
new_base = new_id + base[8:]
|
||||
new_path = os.path.join(d, new_base)
|
||||
try:
|
||||
os.replace(p, new_path)
|
||||
safe_replace(p, new_path)
|
||||
out.append(new_path)
|
||||
except OSError:
|
||||
out.append(p)
|
||||
|
||||
@@ -95,6 +95,14 @@ async def create_profile(
|
||||
# rebuild the tags from vd_states — so the row is always generation-safe
|
||||
# regardless of which frontend build saved it.
|
||||
instruct = heal_design_instruct(instruct, parsed)
|
||||
else:
|
||||
# Clone-kind saves get the same server-side choke point (audit finding:
|
||||
# this class — "Unsupported instruct items" 400s on every later use —
|
||||
# recurred THREE times via clients that bypassed the frontend filter,
|
||||
# and the save-time heal above was gated to design-kind). A clone
|
||||
# profile has no vd_states to rebuild from, so this is sanitize-only:
|
||||
# valid tags survive, prose/"[object Object]" is dropped.
|
||||
instruct = sanitize_instruct(instruct)
|
||||
|
||||
profile_id = str(uuid.uuid4())[:8]
|
||||
|
||||
|
||||
@@ -106,4 +106,44 @@ def load_into_environ(path: Optional[str] = None) -> bool:
|
||||
except ImportError:
|
||||
return False
|
||||
dotenv.load_dotenv(path, override=True)
|
||||
_drop_invalid_path_keys()
|
||||
return True
|
||||
|
||||
|
||||
#: Path-valued keys this file can persist. A reinstall that skipped uninstall
|
||||
#: inherits the old file unconditionally — including e.g. an OMNIVOICE_CACHE_DIR
|
||||
#: pointing at an unplugged drive or a deleted folder. Exporting a dead path
|
||||
#: sends every model download/lookup somewhere that cannot exist and the app
|
||||
#: looks broken out of the box (the audit's "reinstall inherits stale durable
|
||||
#: state" gap). Validate after load: a directory that exists or can be created
|
||||
#: is honored; anything else is dropped for THIS run with a loud log line (the
|
||||
#: file itself is left alone — plugging the drive back in restores the setting).
|
||||
_PATH_KEYS = ("OMNIVOICE_CACHE_DIR", "OMNIVOICE_DATA_DIR")
|
||||
|
||||
|
||||
def _drop_invalid_path_keys() -> None:
|
||||
import logging
|
||||
logger = logging.getLogger("omnivoice.user_env")
|
||||
for key in _PATH_KEYS:
|
||||
val = os.environ.get(key)
|
||||
if not val:
|
||||
continue
|
||||
try:
|
||||
os.makedirs(val, exist_ok=True)
|
||||
# Existing-but-read-only (an external mount, a permissions accident)
|
||||
# passes isdir yet fails on first real use — probe actual write
|
||||
# capability, not just existence (review finding).
|
||||
probe = os.path.join(val, f".omnivoice-write-probe-{os.getpid()}")
|
||||
with open(probe, "w") as f:
|
||||
f.write("ok")
|
||||
os.remove(probe)
|
||||
usable = True
|
||||
except OSError:
|
||||
usable = False
|
||||
if not usable:
|
||||
logger.warning(
|
||||
"%s from the saved env file points at an unusable path (%s) — "
|
||||
"ignoring it for this run and falling back to the default "
|
||||
"location. Fix or clear it in Settings → Models.", key, val,
|
||||
)
|
||||
os.environ.pop(key, None)
|
||||
|
||||
+2
-1
@@ -222,7 +222,8 @@ class _WindowsSafeRotatingFileHandler(RotatingFileHandler):
|
||||
dfn = self.rotation_filename("%s.%d" % (self.baseFilename, i + 1))
|
||||
if os.path.exists(sfn):
|
||||
try:
|
||||
os.replace(sfn, dfn)
|
||||
from utils.fsops import safe_replace
|
||||
safe_replace(sfn, dfn)
|
||||
except OSError as e:
|
||||
_log.warning("log rotation rename failed: %s", e)
|
||||
dfn = self.rotation_filename(self.baseFilename + ".1")
|
||||
|
||||
@@ -1125,7 +1125,13 @@ async def _load_model_with_timeout():
|
||||
|
||||
Raises RuntimeError on timeout (and resets the poisoned pool) so callers
|
||||
surface an actionable error instead of hanging indefinitely.
|
||||
|
||||
This is the shared load boundary for BOTH get_model() and the startup
|
||||
preload_model() — the memory reclaim must live here, or a memory-tight
|
||||
machine gets protected on demand loads but OS-killed during the startup
|
||||
preload (review finding on the original placement in get_model()).
|
||||
"""
|
||||
_make_room_before_tts_load()
|
||||
loop = asyncio.get_running_loop()
|
||||
timeout = _model_load_timeout()
|
||||
try:
|
||||
@@ -1156,6 +1162,41 @@ async def get_model():
|
||||
return model
|
||||
|
||||
|
||||
def _make_room_before_tts_load() -> None:
|
||||
"""Evict-then-load: free what we already own before a tight TTS load.
|
||||
|
||||
The audit's top gap: on a 16 GB unified-memory box a plain TTS load could
|
||||
still be OS-killed — the dub path frees memory before *ASR* loads
|
||||
(offload_tts_for_asr, #1119), but nothing freed memory before a *TTS*
|
||||
load, and a warm dictation model (~2 GB) is routinely the difference.
|
||||
|
||||
Deliberately NOT admission control: refusing a load on an estimate would
|
||||
brick machines that would actually cope (the #1111 decision — advisory
|
||||
only). This only releases things the app already reclaims on idle anyway
|
||||
(the capture-ASR model, engine instances, allocator caches), just *now*
|
||||
instead of after the idle timeout — and only when free memory is actually
|
||||
tight, so a roomy machine pays nothing.
|
||||
"""
|
||||
try:
|
||||
from services.memory_budget import available_memory
|
||||
free_gb = (available_memory() or {}).get("ram_available_gb")
|
||||
if free_gb is None or free_gb >= _UNIFIED_OFFLOAD_HEADROOM_GB:
|
||||
return
|
||||
logger.info(
|
||||
"Memory tight before TTS load (%.1f GB free) — releasing idle "
|
||||
"models first.", free_gb,
|
||||
)
|
||||
try:
|
||||
from services.asr_backend import release_idle_capture_backend
|
||||
release_idle_capture_backend(0.0) # 0s idle = release if unleased
|
||||
except Exception: # noqa: BLE001 — best-effort, never block the load
|
||||
logger.debug("capture-ASR pre-load release failed", exc_info=True)
|
||||
release_tts_side_caches()
|
||||
free_vram()
|
||||
except Exception: # noqa: BLE001 — making room must never break loading
|
||||
logger.debug("pre-load memory reclaim skipped", exc_info=True)
|
||||
|
||||
|
||||
def _checkpoint_in_local_cache(checkpoint: str) -> bool:
|
||||
"""True when ``checkpoint`` is loadable with NO network: an existing local
|
||||
directory, or a COMPLETE HF cache snapshot. ``snapshot_download(...,
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
"""Cross-device-safe filesystem primitives.
|
||||
|
||||
``os.replace`` is atomic only within one filesystem; across devices it raises
|
||||
``OSError(EXDEV)`` (surfacing to Windows users as ``[Errno 18]`` / ``[Errno 22]``
|
||||
in past issue reports — the D:-drive/relocated-models class, #763/#479). Every
|
||||
current call site derives its temp file from the destination directory, which
|
||||
keeps same-device semantics — but nothing *enforced* that, and the next writer
|
||||
that stages in ``%TEMP%`` and renames into a user-relocated data/models dir on
|
||||
another drive reintroduces the whole class. This helper is the enforcement
|
||||
point: replace when possible, degrade to copy+fsync+replace when the OS says
|
||||
the two paths live on different devices.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import errno
|
||||
import os
|
||||
import shutil
|
||||
|
||||
|
||||
def safe_replace(src: str, dst: str) -> None:
|
||||
"""``os.replace`` with a cross-device fallback.
|
||||
|
||||
Same-device: identical to ``os.replace`` (atomic). Cross-device (EXDEV):
|
||||
copy to a temp sibling of ``dst`` (same device as the destination), fsync,
|
||||
then atomically replace — and remove ``src``. Not atomic *end-to-end*
|
||||
across devices (impossible), but the destination itself still only ever
|
||||
transitions atomically from old content to complete new content.
|
||||
"""
|
||||
try:
|
||||
os.replace(src, dst)
|
||||
return
|
||||
except OSError as e:
|
||||
if e.errno != errno.EXDEV:
|
||||
raise
|
||||
tmp = f"{dst}.xdev-tmp-{os.getpid()}"
|
||||
try:
|
||||
shutil.copyfile(src, tmp)
|
||||
with open(tmp, "rb+") as f:
|
||||
f.flush()
|
||||
os.fsync(f.fileno())
|
||||
os.replace(tmp, dst)
|
||||
finally:
|
||||
try:
|
||||
if os.path.exists(tmp):
|
||||
os.remove(tmp)
|
||||
except OSError:
|
||||
pass # best-effort temp cleanup; the replace above already landed or raised
|
||||
try:
|
||||
os.remove(src)
|
||||
except OSError:
|
||||
pass # src may be gone already (another EXDEV fallback won the race)
|
||||
@@ -9,6 +9,7 @@ import { modelStatus as apiModelStatus } from '../api/system';
|
||||
import { useModelStatus } from '../api/hooks';
|
||||
import useRealtimeEvents from './useRealtimeEvents';
|
||||
import { mergeDescribedAttrs } from '../utils/voiceInstruct';
|
||||
import { sanitizeOmniUi } from '../utils/omniUiSchema';
|
||||
|
||||
/**
|
||||
* Encapsulates all data-loading effects, localStorage persistence,
|
||||
@@ -179,7 +180,11 @@ export default function useAppData() {
|
||||
loadAll();
|
||||
// Restore local UI state
|
||||
try {
|
||||
const saved = JSON.parse(localStorage.getItem('omni_ui') || '{}');
|
||||
// Whitelist + shape-check every persisted field (audit: the #1067 class
|
||||
// was healed per-field; this closes it generically — malformed values
|
||||
// are dropped up front instead of throwing mid-restore and silently
|
||||
// discarding every field after the bad one).
|
||||
const saved = sanitizeOmniUi(JSON.parse(localStorage.getItem('omni_ui') || '{}'));
|
||||
if (saved.uiScale) setUiScale(saved.uiScale);
|
||||
if (saved.text) setText(saved.text);
|
||||
// Legacy shim (voice-studio-unification P4): the old 'clone'/'design'
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
import { describe, it, expect, vi } from 'vitest';
|
||||
import { sanitizeOmniUi, OMNI_UI_SCHEMA } from '../utils/omniUiSchema';
|
||||
|
||||
describe('sanitizeOmniUi', () => {
|
||||
it('drops malformed fields without discarding the rest (the mid-restore-abort class)', () => {
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
const out = sanitizeOmniUi({
|
||||
uiScale: 1.2,
|
||||
dubSegments: 'oops-a-string', // poisoned — used to throw at .map and abort the restore
|
||||
dubStep: 'editing',
|
||||
speed: 1.0,
|
||||
});
|
||||
expect(out.uiScale).toBe(1.2);
|
||||
expect(out.dubStep).toBe('editing');
|
||||
expect(out.speed).toBe(1.0);
|
||||
expect('dubSegments' in out).toBe(false);
|
||||
expect(warn).toHaveBeenCalledTimes(1);
|
||||
warn.mockRestore();
|
||||
});
|
||||
|
||||
it('drops unknown keys (future fields must be added to the schema)', () => {
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
const out = sanitizeOmniUi({ someFutureField: { deeply: 'nested' }, text: 'hi' });
|
||||
expect(out).toEqual({ text: 'hi' });
|
||||
warn.mockRestore();
|
||||
});
|
||||
|
||||
it('never throws on garbage input', () => {
|
||||
for (const garbage of [null, undefined, 'str', 42, [], () => {}]) {
|
||||
expect(sanitizeOmniUi(garbage)).toEqual({});
|
||||
}
|
||||
});
|
||||
|
||||
it('passes a fully well-formed blob through unchanged', () => {
|
||||
const good = {
|
||||
uiScale: 1,
|
||||
text: 't',
|
||||
mode: 'studio',
|
||||
language: 'en',
|
||||
isSidebarCollapsed: false,
|
||||
dubSegments: [{ id: '1', text: 'x' }],
|
||||
dubStep: 'editing',
|
||||
preserveBg: true,
|
||||
speed: 1,
|
||||
steps: 16,
|
||||
cfg: 2,
|
||||
};
|
||||
expect(sanitizeOmniUi({ ...good })).toEqual(good);
|
||||
});
|
||||
|
||||
it('schema covers every field the restore path reads', () => {
|
||||
// Lockstep guard: if useAppData reads a saved.<field> not in the schema,
|
||||
// restore silently drops it — fail here instead of in the field.
|
||||
const src = require('fs').readFileSync(
|
||||
require('path').resolve(__dirname, '../hooks/useAppData.js'),
|
||||
'utf8',
|
||||
);
|
||||
const reads = [...src.matchAll(/saved\.([A-Za-z0-9_]+)/g)].map((m) => m[1]);
|
||||
for (const key of new Set(reads)) {
|
||||
expect(OMNI_UI_SCHEMA[key], `omniUiSchema is missing '${key}'`).toBeTypeOf('function');
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -112,3 +112,17 @@ describe('streamDropError — waits for the shell to notice the death (#1119)',
|
||||
expect(calls).toBe(1); // asked once, then stopped — no 8 s wait
|
||||
});
|
||||
});
|
||||
|
||||
describe('crashCauseHint', () => {
|
||||
it('attributes SIGKILL to system memory, not VRAM', async () => {
|
||||
const { crashCauseHint } = await import('../utils/backendCrash');
|
||||
const hint = crashCauseHint({ exit_code: null, signal: 9 });
|
||||
expect(hint).toMatch(/memory \(RAM\)/);
|
||||
expect(hint).not.toMatch(/VRAM/);
|
||||
});
|
||||
|
||||
it('keeps the VRAM guidance for real GPU aborts', async () => {
|
||||
const { crashCauseHint } = await import('../utils/backendCrash');
|
||||
expect(crashCauseHint({ exit_code: 1, signal: null })).toMatch(/VRAM/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -72,6 +72,29 @@ export function describeCrashExit(
|
||||
return marker.exit_desc || 'unknown exit';
|
||||
}
|
||||
|
||||
/**
|
||||
* Likely-cause line for the crash message, branched on HOW the process died.
|
||||
*
|
||||
* SIGKILL (signal 9) with no stderr is the operating system's memory killer —
|
||||
* on a unified-memory Mac that means system RAM, and the old one-size message
|
||||
* blamed "VRAM" on machines that have none (audit finding: OS-OOM kills were
|
||||
* misattributed). Everything else keeps the small-GPU VRAM guidance, which is
|
||||
* the dominant cause for real GPU aborts.
|
||||
*/
|
||||
export function crashCauseHint(marker: Pick<BackendCrashMarker, 'exit_code' | 'signal'>): string {
|
||||
if (marker.signal === 9) {
|
||||
return (
|
||||
'It was force-killed (signal 9), which usually means the operating system ran out of ' +
|
||||
'memory (RAM) and stopped it. Close memory-heavy apps, pick a smaller ASR model in ' +
|
||||
'Settings → Models, or flush the TTS model before transcribing.'
|
||||
);
|
||||
}
|
||||
return (
|
||||
'On smaller GPUs the usual cause is running out of VRAM while loading the ASR model on top ' +
|
||||
'of the TTS model: flush the TTS model first, or pick a smaller ASR model in Settings → Models.'
|
||||
);
|
||||
}
|
||||
|
||||
/** Coarse "12 s" / "3 min" / "2 h" age of a marker, for the honest message. */
|
||||
export function crashAge(marker: Pick<BackendCrashMarker, 'ts'>, nowMs = Date.now()): string {
|
||||
const s = Math.max(0, Math.round(nowMs / 1000 - marker.ts));
|
||||
@@ -140,8 +163,6 @@ export async function streamDropError(
|
||||
return new Error(
|
||||
`The local OmniVoice backend crashed (${describeCrashExit(crash)}) ${crashAge(crash)} ago, ` +
|
||||
'which dropped this stream — it is being restarted automatically. Open the crash notice for ' +
|
||||
'the error output. On smaller GPUs the usual cause is running out of VRAM while loading the ' +
|
||||
'ASR model on top of the TTS model: flush the TTS model first, or pick a smaller ASR model ' +
|
||||
'in Settings → Models.',
|
||||
`the error output. ${crashCauseHint(crash)}`,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
/**
|
||||
* Schema guard for the persisted `omni_ui` localStorage blob.
|
||||
*
|
||||
* The #1067 class ("app got empty" after quit-mid-job) was healed per-field —
|
||||
* dubStep got a clamp, vdStates got a shape merge — but every OTHER field was
|
||||
* restored unvalidated inside one big try/catch, so (a) any future persisted
|
||||
* field re-opens the class, and (b) a single malformed value (say, dubSegments
|
||||
* saved as a string by a buggy build) throws mid-restore and silently discards
|
||||
* every field after it. This is the generic pass the audit called for: a
|
||||
* whitelist of known keys with a type/shape check each; unknown keys and
|
||||
* malformed values are dropped (with one console warning naming them) instead
|
||||
* of crashing or leaking into state.
|
||||
*
|
||||
* Deliberately NOT a validation library: predicates only, no coercion — a
|
||||
* value that fails its predicate is exactly the value we don't want in state.
|
||||
*/
|
||||
|
||||
const isStr = (v) => typeof v === 'string';
|
||||
const isNum = (v) => typeof v === 'number' && Number.isFinite(v);
|
||||
const isBool = (v) => typeof v === 'boolean';
|
||||
const isObj = (v) => !!v && typeof v === 'object' && !Array.isArray(v);
|
||||
const isArrOfObj = (v) => Array.isArray(v) && v.every(isObj);
|
||||
const isArr = (v) => Array.isArray(v);
|
||||
|
||||
/** key → predicate for every field the restore path reads. Adding a new
|
||||
* persisted field REQUIRES adding it here — restore drops unknown keys. */
|
||||
export const OMNI_UI_SCHEMA = {
|
||||
uiScale: isNum,
|
||||
text: isStr,
|
||||
mode: isStr,
|
||||
defineMethod: isStr,
|
||||
vdStates: isObj, // completed to full CATEGORIES shape by the caller (#983)
|
||||
language: isStr,
|
||||
isSidebarCollapsed: isBool,
|
||||
sidebarTab: isStr,
|
||||
dubJobId: isStr,
|
||||
dubFilename: isStr,
|
||||
dubDuration: isNum,
|
||||
dubSegments: isArrOfObj,
|
||||
dubLang: isStr,
|
||||
dubLangCode: isStr,
|
||||
dubTracks: isObj,
|
||||
dubStep: isStr, // additionally clamped by clampRestoredDubStep (#1067)
|
||||
dubTranscript: isStr,
|
||||
exportTracks: isArr,
|
||||
preserveBg: isBool,
|
||||
defaultTrack: isStr,
|
||||
exportHistory: isArr,
|
||||
speed: isNum,
|
||||
steps: isNum,
|
||||
cfg: isNum,
|
||||
denoise: isBool,
|
||||
showOverrides: isBool,
|
||||
};
|
||||
|
||||
/**
|
||||
* Return a copy of `saved` containing only whitelisted, well-shaped fields.
|
||||
* Never throws; on non-object input returns {}.
|
||||
*/
|
||||
export function sanitizeOmniUi(saved) {
|
||||
if (!isObj(saved)) return {};
|
||||
const out = {};
|
||||
const dropped = [];
|
||||
for (const [key, value] of Object.entries(saved)) {
|
||||
const check = OMNI_UI_SCHEMA[key];
|
||||
if (!check) {
|
||||
dropped.push(key);
|
||||
continue;
|
||||
}
|
||||
if (value === undefined || value === null) continue;
|
||||
if (check(value)) out[key] = value;
|
||||
else dropped.push(key);
|
||||
}
|
||||
if (dropped.length) {
|
||||
// One line, not one per key — this fires at most once per app start.
|
||||
console.warn(`omni_ui restore: dropped unknown/malformed field(s): ${dropped.join(', ')}`);
|
||||
}
|
||||
return out;
|
||||
}
|
||||
@@ -0,0 +1,210 @@
|
||||
"""Recurrence-hardening sweep — regression tests for the audit's gap list.
|
||||
|
||||
Each test pins one guard added after the full closed-issue-history audit:
|
||||
error classes that were fixed but could still recur via an unguarded seam
|
||||
(a bypassing client, a stale reinstall leftover, a cross-device move, an
|
||||
OS-level OOM kill, a scaled-up request). See the PR body for the class map.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import errno
|
||||
import os
|
||||
|
||||
import pytest
|
||||
|
||||
os.environ.setdefault("OMNIVOICE_DISABLE_FILE_LOG", "1")
|
||||
|
||||
_FAKE_AUDIO = b"RIFF" + b"\x00" * 2000
|
||||
|
||||
|
||||
# ── Class 3: instruct poisoning via clone-kind saves ─────────────────────────
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def profiles_client(tmp_path, monkeypatch):
|
||||
monkeypatch.setenv("OMNIVOICE_DATA_DIR", str(tmp_path))
|
||||
import importlib
|
||||
import core.config as _cfg
|
||||
importlib.reload(_cfg)
|
||||
import core.db as _db
|
||||
importlib.reload(_db)
|
||||
from api.routers import profiles as _profiles
|
||||
importlib.reload(_profiles)
|
||||
import main as _main
|
||||
importlib.reload(_main)
|
||||
_db.init_db()
|
||||
from fastapi.testclient import TestClient
|
||||
# No context manager: entering it runs the app lifespan, whose shutdown
|
||||
# tears down executors shared with other suites in a full run (the repo's
|
||||
# profile tests use the same lifespan-free pattern — "schema only").
|
||||
yield TestClient(_main.app, client=("127.0.0.1", 50002))
|
||||
|
||||
|
||||
def test_clone_profile_save_sanitizes_instruct(profiles_client):
|
||||
"""The 400-on-every-use class recurred THREE times via clients that
|
||||
bypassed the frontend filter; the server-side save heal was gated to
|
||||
design-kind. Clone-kind saves must be generation-safe too."""
|
||||
r = profiles_client.post("/profiles", data={
|
||||
"name": "poisoned clone",
|
||||
"kind": "clone",
|
||||
"instruct": "please read this in a very dramatic movie-trailer way!!",
|
||||
}, files={"ref_audio": ("ref.wav", _FAKE_AUDIO, "audio/wav")})
|
||||
assert r.status_code == 200, r.text
|
||||
pid = r.json()["id"]
|
||||
from core.db import db_conn
|
||||
with db_conn() as conn:
|
||||
row = conn.execute(
|
||||
"SELECT instruct FROM voice_profiles WHERE id=?", (pid,)
|
||||
).fetchone()
|
||||
persisted = row["instruct"] or ""
|
||||
from omnivoice.utils.voice_design import sanitize_instruct
|
||||
assert persisted == sanitize_instruct(persisted), (
|
||||
"persisted clone instruct is not validator-safe — the #550/#594 class is open again"
|
||||
)
|
||||
|
||||
|
||||
# ── Class 5: reinstall inherits a stale env file ─────────────────────────────
|
||||
|
||||
|
||||
def test_user_env_drops_unusable_path_keys(tmp_path, monkeypatch):
|
||||
"""A reinstall that skipped uninstall inherits ~/.config/omnivoice/env
|
||||
verbatim — including a cache dir on an unplugged drive. Dead paths must be
|
||||
ignored for the run, not exported."""
|
||||
blocker = tmp_path / "not-a-dir"
|
||||
blocker.write_text("a file, so a directory cannot be created beneath it")
|
||||
env_file = tmp_path / "env"
|
||||
env_file.write_text(
|
||||
f"OMNIVOICE_CACHE_DIR={blocker}/impossible/cache\n"
|
||||
f"OMNIVOICE_DATA_DIR={tmp_path}/fine\n"
|
||||
)
|
||||
monkeypatch.delenv("OMNIVOICE_CACHE_DIR", raising=False)
|
||||
monkeypatch.setenv("OMNIVOICE_DATA_DIR", "sentinel-overwritten-by-load")
|
||||
|
||||
from core import user_env
|
||||
assert user_env.load_into_environ(str(env_file)) is True
|
||||
assert "OMNIVOICE_CACHE_DIR" not in os.environ, "dead path was exported anyway"
|
||||
assert os.environ["OMNIVOICE_DATA_DIR"] == f"{tmp_path}/fine" # valid path honored
|
||||
assert os.path.isdir(f"{tmp_path}/fine")
|
||||
monkeypatch.delenv("OMNIVOICE_DATA_DIR", raising=False)
|
||||
|
||||
|
||||
# ── Class 7: cross-device moves (Windows D:-drive class) ─────────────────────
|
||||
|
||||
|
||||
def test_safe_replace_same_device(tmp_path):
|
||||
from utils.fsops import safe_replace
|
||||
src, dst = tmp_path / "a.txt", tmp_path / "b.txt"
|
||||
src.write_text("payload")
|
||||
dst.write_text("old")
|
||||
safe_replace(str(src), str(dst))
|
||||
assert dst.read_text() == "payload" and not src.exists()
|
||||
|
||||
|
||||
def test_safe_replace_falls_back_on_exdev(tmp_path, monkeypatch):
|
||||
"""EXDEV (paths on different devices) must degrade to copy+replace, not
|
||||
surface as the [Errno 18/22] class Windows users reported."""
|
||||
from utils import fsops
|
||||
real_replace = os.replace
|
||||
calls = {"n": 0}
|
||||
|
||||
def fake_replace(a, b):
|
||||
calls["n"] += 1
|
||||
if calls["n"] == 1:
|
||||
raise OSError(errno.EXDEV, "Invalid cross-device link")
|
||||
return real_replace(a, b)
|
||||
|
||||
monkeypatch.setattr(fsops.os, "replace", fake_replace)
|
||||
src, dst = tmp_path / "a.bin", tmp_path / "b.bin"
|
||||
src.write_bytes(b"x" * 4096)
|
||||
fsops.safe_replace(str(src), str(dst))
|
||||
assert dst.read_bytes() == b"x" * 4096
|
||||
assert not src.exists()
|
||||
assert calls["n"] == 2 # first raised EXDEV, second landed the temp copy
|
||||
|
||||
|
||||
def test_safe_replace_propagates_real_errors(tmp_path, monkeypatch):
|
||||
from utils import fsops
|
||||
|
||||
def fake_replace(a, b):
|
||||
raise OSError(errno.EACCES, "denied")
|
||||
|
||||
monkeypatch.setattr(fsops.os, "replace", fake_replace)
|
||||
src = tmp_path / "a"
|
||||
src.write_text("x")
|
||||
with pytest.raises(OSError):
|
||||
fsops.safe_replace(str(src), str(tmp_path / "b"))
|
||||
|
||||
|
||||
# ── Class 1: evict-then-load before a tight TTS load ─────────────────────────
|
||||
|
||||
|
||||
def test_make_room_runs_only_when_memory_is_tight(monkeypatch):
|
||||
from services import model_manager as mm
|
||||
|
||||
released = {"asr": 0, "vram": 0}
|
||||
monkeypatch.setattr(
|
||||
"services.memory_budget.available_memory",
|
||||
lambda: {"ram_available_gb": 3.0}, # below the 6 GB headroom
|
||||
)
|
||||
import services.asr_backend as ab
|
||||
monkeypatch.setattr(ab, "release_idle_capture_backend",
|
||||
lambda idle_s: released.__setitem__("asr", released["asr"] + 1) or True)
|
||||
monkeypatch.setattr(mm, "free_vram", lambda: released.__setitem__("vram", released["vram"] + 1))
|
||||
|
||||
mm._make_room_before_tts_load()
|
||||
assert released == {"asr": 1, "vram": 1}, "tight memory must trigger the reclaim"
|
||||
|
||||
|
||||
def test_make_room_is_a_noop_with_headroom(monkeypatch):
|
||||
from services import model_manager as mm
|
||||
|
||||
monkeypatch.setattr(
|
||||
"services.memory_budget.available_memory",
|
||||
lambda: {"ram_available_gb": 12.0},
|
||||
)
|
||||
called = []
|
||||
monkeypatch.setattr(mm, "free_vram", lambda: called.append(1))
|
||||
mm._make_room_before_tts_load()
|
||||
assert not called, "a roomy machine must pay nothing"
|
||||
|
||||
|
||||
# ── Class 4 (503 wave): timeout scales with the request ──────────────────────
|
||||
|
||||
|
||||
def test_generate_timeout_scales_with_text_length(monkeypatch):
|
||||
from api.routers import generation as g
|
||||
|
||||
short = g._generate_timeout_s("hello world")
|
||||
long = g._generate_timeout_s("x" * 41_200) # 40k chars past the free allowance
|
||||
assert short == pytest.approx(300.0) # floor: the configured default
|
||||
assert long == pytest.approx(300.0 + 40_000 / 40.0) # +1s per 40 chars
|
||||
|
||||
|
||||
def test_generate_timeout_env_floor_respected(monkeypatch):
|
||||
import importlib
|
||||
monkeypatch.setenv("OMNIVOICE_GENERATE_TIMEOUT_S", "900")
|
||||
import services.model_manager as mm
|
||||
monkeypatch.setattr(mm, "GPU_JOB_TIMEOUT_S", 900.0)
|
||||
from api.routers import generation as g
|
||||
assert g._generate_timeout_s("short") == pytest.approx(900.0)
|
||||
|
||||
|
||||
def test_user_env_drops_read_only_path(tmp_path, monkeypatch):
|
||||
"""An existing directory on a read-only mount passes isdir but fails on
|
||||
first real use — validation must probe actual write capability."""
|
||||
if hasattr(os, "geteuid") and os.geteuid() == 0:
|
||||
pytest.skip("root writes anywhere; the probe cannot fail")
|
||||
ro = tmp_path / "readonly-cache"
|
||||
ro.mkdir()
|
||||
ro.chmod(0o500)
|
||||
env_file = tmp_path / "env"
|
||||
env_file.write_text(f"OMNIVOICE_CACHE_DIR={ro}\n")
|
||||
monkeypatch.delenv("OMNIVOICE_CACHE_DIR", raising=False)
|
||||
try:
|
||||
from core import user_env
|
||||
assert user_env.load_into_environ(str(env_file)) is True
|
||||
assert "OMNIVOICE_CACHE_DIR" not in os.environ, (
|
||||
"read-only path was kept — downloads would fail on first use"
|
||||
)
|
||||
finally:
|
||||
ro.chmod(0o700)
|
||||
@@ -19,7 +19,11 @@ def test_user_env_overrides_preinjected_value(tmp_path, monkeypatch):
|
||||
"""A value in the per-user env file must beat one a launcher (Tauri) already
|
||||
injected into the environment — the core of #480."""
|
||||
envfile = tmp_path / "env"
|
||||
envfile.write_text("OMNIVOICE_CACHE_DIR=/new/models/dir\n")
|
||||
# A real, creatable path: since the stale-reinstall hardening, load-time
|
||||
# validation drops path keys that can't exist — this test is about
|
||||
# PRECEDENCE, so keep the path valid and assert the file value wins.
|
||||
new_dir = tmp_path / "new-models-dir"
|
||||
envfile.write_text(f"OMNIVOICE_CACHE_DIR={new_dir}\n")
|
||||
monkeypatch.setenv("OMNIVOICE_ENV_FILE", str(envfile))
|
||||
# simulate Tauri injecting the OLD value before the backend loads the file
|
||||
monkeypatch.setenv("OMNIVOICE_CACHE_DIR", "/old/models/dir")
|
||||
@@ -27,18 +31,19 @@ def test_user_env_overrides_preinjected_value(tmp_path, monkeypatch):
|
||||
loaded = _user_env().load_into_environ()
|
||||
|
||||
assert loaded is True
|
||||
assert os.environ["OMNIVOICE_CACHE_DIR"] == "/new/models/dir"
|
||||
assert os.environ["OMNIVOICE_CACHE_DIR"] == str(new_dir)
|
||||
|
||||
|
||||
def test_user_env_sets_value_absent_from_environ(tmp_path, monkeypatch):
|
||||
"""When nothing was pre-injected, the file value is applied as-is."""
|
||||
envfile = tmp_path / "env"
|
||||
envfile.write_text("OMNIVOICE_CACHE_DIR=/chosen/dir\n")
|
||||
chosen = tmp_path / "chosen-dir"
|
||||
envfile.write_text(f"OMNIVOICE_CACHE_DIR={chosen}\n")
|
||||
monkeypatch.setenv("OMNIVOICE_ENV_FILE", str(envfile))
|
||||
monkeypatch.delenv("OMNIVOICE_CACHE_DIR", raising=False)
|
||||
|
||||
assert _user_env().load_into_environ() is True
|
||||
assert os.environ["OMNIVOICE_CACHE_DIR"] == "/chosen/dir"
|
||||
assert os.environ["OMNIVOICE_CACHE_DIR"] == str(chosen)
|
||||
|
||||
|
||||
def test_user_env_missing_file_is_noop(tmp_path, monkeypatch):
|
||||
|
||||
Reference in New Issue
Block a user