fix: two error paths that made a failure worse than the thing that failed
/api/clip: _clip_wav/_clip_flac send audio headers and then stream frames. A decode failure mid-stream unwound into _api_clip's handler, which called _json_err(500) — writing a second status line and a JSON body into a response that had already promised Content-Type: audio/wav and an exact Content-Length. Under HTTP/1.1 keep-alive that desyncs the connection for every subsequent request on it, so one bad clip corrupted unrelated traffic. Track whether the headers went out; if they did, log and force the connection closed instead, the same way _copy_to_response already handles under-delivery. config.ini: the int() coercion for split_minutes/max_retries/etc sat outside the try that wraps recorder construction, so `split_minutes = sixty` aborted the entire config load with an unhandled ValueError rather than skipping that one section like every other config error. Moved inside. split_duration is also clamped to >= 1 — 0 divided by zero in get_next_split_time(). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -272,8 +272,9 @@ class BaseRecorder(ABC):
|
|||||||
self.current_filename = None
|
self.current_filename = None
|
||||||
self._clock = clock or datetime.now
|
self._clock = clock or datetime.now
|
||||||
|
|
||||||
# Common settings
|
# Common settings. split_duration is clamped: 0 would divide by zero
|
||||||
self.split_duration = config.get('split_minutes', 60)
|
# in get_next_split_time(), negative would split every tick.
|
||||||
|
self.split_duration = max(1, config.get('split_minutes', 60))
|
||||||
self.output_dir = config.get('output_directory', 'recordings')
|
self.output_dir = config.get('output_directory', 'recordings')
|
||||||
self.max_retries = config.get('max_retries', 10)
|
self.max_retries = config.get('max_retries', 10)
|
||||||
self.retry_delay = config.get('retry_delay_seconds', 5)
|
self.retry_delay = config.get('retry_delay_seconds', 5)
|
||||||
@@ -775,7 +776,10 @@ class RecorderManager:
|
|||||||
self.logger.warning(f"Section [{section}] has no 'type', skipping")
|
self.logger.warning(f"Section [{section}] has no 'type', skipping")
|
||||||
continue
|
continue
|
||||||
|
|
||||||
# Build source config by merging general with section-specific
|
# Build the source config and create the recorder. Both are inside
|
||||||
|
# the try: a non-numeric split_minutes used to raise out here and
|
||||||
|
# abort the whole config load instead of skipping one section.
|
||||||
|
try:
|
||||||
source_config = general.copy()
|
source_config = general.copy()
|
||||||
|
|
||||||
for key, value in config.items(section):
|
for key, value in config.items(section):
|
||||||
@@ -786,8 +790,6 @@ class RecorderManager:
|
|||||||
else:
|
else:
|
||||||
source_config[key] = value
|
source_config[key] = value
|
||||||
|
|
||||||
# Create recorder
|
|
||||||
try:
|
|
||||||
if source_type == 'stream':
|
if source_type == 'stream':
|
||||||
recorder = StreamRecorder(section, source_config, self.logger)
|
recorder = StreamRecorder(section, source_config, self.logger)
|
||||||
self.recorders.append(recorder)
|
self.recorders.append(recorder)
|
||||||
|
|||||||
@@ -591,6 +591,57 @@ url = http://x.com/s2
|
|||||||
assert len(mgr.recorders) == 1
|
assert len(mgr.recorders) == 1
|
||||||
assert mgr.recorders[0].name == "good_source"
|
assert mgr.recorders[0].name == "good_source"
|
||||||
|
|
||||||
|
def test_bad_integer_skips_only_that_source(self, tmp_path):
|
||||||
|
# A non-numeric split_minutes used to raise ValueError out of the
|
||||||
|
# config loop and abort the whole load, taking healthy sources with it.
|
||||||
|
log_file = str(tmp_path / "test.log")
|
||||||
|
config_text = f"""
|
||||||
|
[general]
|
||||||
|
log_level = WARNING
|
||||||
|
log_file = {log_file}
|
||||||
|
|
||||||
|
[broken]
|
||||||
|
type = stream
|
||||||
|
url = http://x.com/s
|
||||||
|
split_minutes = sixty
|
||||||
|
|
||||||
|
[good_source]
|
||||||
|
type = stream
|
||||||
|
url = http://x.com/s2
|
||||||
|
"""
|
||||||
|
cfg_path = tmp_path / "config.ini"
|
||||||
|
cfg_path.write_text(config_text)
|
||||||
|
|
||||||
|
with patch.object(isr, "AudioSystem") as mock_as:
|
||||||
|
mock_as.return_value.available = False
|
||||||
|
mgr = isr.RecorderManager(str(cfg_path))
|
||||||
|
|
||||||
|
assert [r.name for r in mgr.recorders] == ["good_source"]
|
||||||
|
|
||||||
|
def test_zero_split_minutes_is_clamped(self, tmp_path):
|
||||||
|
# split_minutes = 0 reached get_next_split_time() and divided by zero.
|
||||||
|
log_file = str(tmp_path / "test.log")
|
||||||
|
config_text = f"""
|
||||||
|
[general]
|
||||||
|
log_level = WARNING
|
||||||
|
log_file = {log_file}
|
||||||
|
|
||||||
|
[zero_split]
|
||||||
|
type = stream
|
||||||
|
url = http://x.com/s
|
||||||
|
split_minutes = 0
|
||||||
|
"""
|
||||||
|
cfg_path = tmp_path / "config.ini"
|
||||||
|
cfg_path.write_text(config_text)
|
||||||
|
|
||||||
|
with patch.object(isr, "AudioSystem") as mock_as:
|
||||||
|
mock_as.return_value.available = False
|
||||||
|
mgr = isr.RecorderManager(str(cfg_path))
|
||||||
|
|
||||||
|
rec = mgr.recorders[0]
|
||||||
|
assert rec.split_duration == 1
|
||||||
|
rec.get_next_split_time() # must not raise ZeroDivisionError
|
||||||
|
|
||||||
def test_missing_config_file_exits(self, tmp_path):
|
def test_missing_config_file_exits(self, tmp_path):
|
||||||
with pytest.raises(SystemExit):
|
with pytest.raises(SystemExit):
|
||||||
# main() calls sys.exit(1) when the file is missing
|
# main() calls sys.exit(1) when the file is missing
|
||||||
|
|||||||
@@ -1,7 +1,10 @@
|
|||||||
"""Tests for the adaptive loud-section detector in web.py."""
|
"""Tests for the adaptive loud-section detector in web.py."""
|
||||||
|
|
||||||
|
import io
|
||||||
import math
|
import math
|
||||||
|
import wave
|
||||||
|
|
||||||
|
import web
|
||||||
from web import _cut_filename, _loud_sections, _noise_floor_db, _recording_start
|
from web import _cut_filename, _loud_sections, _noise_floor_db, _recording_start
|
||||||
|
|
||||||
WINDOW_DUR = 0.1 # 100 ms windows, as produced by WINDOW_SAMPLES at 48 kHz
|
WINDOW_DUR = 0.1 # 100 ms windows, as produced by WINDOW_SAMPLES at 48 kHz
|
||||||
@@ -160,3 +163,69 @@ def test_cut_filename_rolls_over_the_hour():
|
|||||||
def test_cut_filename_falls_back_for_nonstandard_name():
|
def test_cut_filename_falls_back_for_nonstandard_name():
|
||||||
name = _cut_filename("mixtape", ".mp3", 740, 750.4)
|
name = _cut_filename("mixtape", ".mp3", 740, 750.4)
|
||||||
assert name == "mixtape_cut_740s-750s.mp3"
|
assert name == "mixtape_cut_740s-750s.mp3"
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# /api/clip error handling
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
class _OfflineHandler(web._Handler):
|
||||||
|
"""A _Handler with no socket, for driving handler methods directly."""
|
||||||
|
|
||||||
|
def __init__(self, recordings_dir):
|
||||||
|
self.recordings_dir = str(recordings_dir)
|
||||||
|
self.analyses_dir = str(recordings_dir)
|
||||||
|
self.wfile = io.BytesIO()
|
||||||
|
self.close_connection = False
|
||||||
|
self.codes = []
|
||||||
|
|
||||||
|
def send_response(self, code):
|
||||||
|
self.codes.append(code)
|
||||||
|
|
||||||
|
def send_header(self, key, value):
|
||||||
|
pass
|
||||||
|
|
||||||
|
def end_headers(self):
|
||||||
|
pass
|
||||||
|
|
||||||
|
|
||||||
|
def _write_wav(path, seconds=2, framerate=8000):
|
||||||
|
with wave.open(str(path), 'wb') as wf:
|
||||||
|
wf.setnchannels(1)
|
||||||
|
wf.setsampwidth(2)
|
||||||
|
wf.setframerate(framerate)
|
||||||
|
wf.writeframes(b'\x00\x00' * (seconds * framerate))
|
||||||
|
|
||||||
|
|
||||||
|
def test_clip_failure_after_headers_closes_connection_instead_of_writing_json(tmp_path):
|
||||||
|
# Once the audio headers are out, an error body would be a second response
|
||||||
|
# on a keep-alive connection that already promised audio/wav plus an exact
|
||||||
|
# Content-Length — desyncing every later request on that connection.
|
||||||
|
_write_wav(tmp_path / 'rec.wav')
|
||||||
|
h = _OfflineHandler(tmp_path)
|
||||||
|
|
||||||
|
def boom(path, start, end):
|
||||||
|
h._send_clip_headers(b'HDR', 10, 1, 2)
|
||||||
|
raise RuntimeError('decode exploded')
|
||||||
|
|
||||||
|
h._clip_wav = boom
|
||||||
|
h._api_clip({'file': ['rec.wav'], 'start': ['0'], 'end': ['1']})
|
||||||
|
|
||||||
|
assert h.close_connection is True
|
||||||
|
assert h.codes == [200] # no second status line
|
||||||
|
assert b'error' not in h.wfile.getvalue() # no JSON body
|
||||||
|
|
||||||
|
|
||||||
|
def test_clip_failure_before_headers_still_reports_json(tmp_path):
|
||||||
|
_write_wav(tmp_path / 'rec.wav')
|
||||||
|
h = _OfflineHandler(tmp_path)
|
||||||
|
|
||||||
|
def boom(path, start, end):
|
||||||
|
raise RuntimeError('could not open')
|
||||||
|
|
||||||
|
h._clip_wav = boom
|
||||||
|
h._api_clip({'file': ['rec.wav'], 'start': ['0'], 'end': ['1']})
|
||||||
|
|
||||||
|
assert h.close_connection is False
|
||||||
|
assert h.codes == [500]
|
||||||
|
assert b'could not open' in h.wfile.getvalue()
|
||||||
|
|||||||
@@ -590,6 +590,9 @@ class _Handler(BaseHTTPRequestHandler):
|
|||||||
min_gap: float = MIN_GAP_SECONDS
|
min_gap: float = MIN_GAP_SECONDS
|
||||||
min_duration: float = MIN_DURATION_SECONDS
|
min_duration: float = MIN_DURATION_SECONDS
|
||||||
|
|
||||||
|
# True once /api/clip has written its audio headers — see _api_clip
|
||||||
|
_clip_started: bool = False
|
||||||
|
|
||||||
def do_DELETE(self):
|
def do_DELETE(self):
|
||||||
parsed = urlparse(self.path)
|
parsed = urlparse(self.path)
|
||||||
p = parsed.path
|
p = parsed.path
|
||||||
@@ -962,6 +965,7 @@ class _Handler(BaseHTTPRequestHandler):
|
|||||||
return
|
return
|
||||||
|
|
||||||
ext = path.suffix.lower()
|
ext = path.suffix.lower()
|
||||||
|
self._clip_started = False
|
||||||
try:
|
try:
|
||||||
if ext == '.wav':
|
if ext == '.wav':
|
||||||
self._clip_wav(path, start, end)
|
self._clip_wav(path, start, end)
|
||||||
@@ -975,10 +979,19 @@ class _Handler(BaseHTTPRequestHandler):
|
|||||||
except (ConnectionError, BrokenPipeError):
|
except (ConnectionError, BrokenPipeError):
|
||||||
raise
|
raise
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
# Once the audio headers are out, a JSON error body would be a
|
||||||
|
# second response written into a stream that already promised
|
||||||
|
# audio/wav and a Content-Length — with keep-alive that desyncs
|
||||||
|
# the connection for every later request on it. Drop it instead.
|
||||||
|
if self._clip_started:
|
||||||
|
print(f'Clip failed mid-stream: {e}', flush=True)
|
||||||
|
self.close_connection = True
|
||||||
|
else:
|
||||||
self._json_err(500, f'Clip failed: {e}')
|
self._json_err(500, f'Clip failed: {e}')
|
||||||
|
|
||||||
def _send_clip_headers(self, header: bytes, n_frames: int, channels: int,
|
def _send_clip_headers(self, header: bytes, n_frames: int, channels: int,
|
||||||
sampwidth: int):
|
sampwidth: int):
|
||||||
|
self._clip_started = True
|
||||||
self.send_response(200)
|
self.send_response(200)
|
||||||
self.send_header('Content-Type', 'audio/wav')
|
self.send_header('Content-Type', 'audio/wav')
|
||||||
self.send_header('Content-Length', str(len(header) + n_frames * channels * sampwidth))
|
self.send_header('Content-Length', str(len(header) + n_frames * channels * sampwidth))
|
||||||
|
|||||||
Reference in New Issue
Block a user