From 9bb1229aa9ba6b24b7bcf63e80d374f1b4622e30 Mon Sep 17 00:00:00 2001 From: jonathan Date: Wed, 29 Jul 2026 20:29:54 +0200 Subject: [PATCH] fix: two error paths that made a failure worse than the thing that failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /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 --- isr.py | 30 +++++++++++---------- tests/test_isr.py | 51 +++++++++++++++++++++++++++++++++++ tests/test_web.py | 69 +++++++++++++++++++++++++++++++++++++++++++++++ web.py | 15 ++++++++++- 4 files changed, 150 insertions(+), 15 deletions(-) diff --git a/isr.py b/isr.py index f9b2631..8fc09da 100644 --- a/isr.py +++ b/isr.py @@ -272,8 +272,9 @@ class BaseRecorder(ABC): self.current_filename = None self._clock = clock or datetime.now - # Common settings - self.split_duration = config.get('split_minutes', 60) + # Common settings. split_duration is clamped: 0 would divide by zero + # 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.max_retries = config.get('max_retries', 10) self.retry_delay = config.get('retry_delay_seconds', 5) @@ -775,19 +776,20 @@ class RecorderManager: self.logger.warning(f"Section [{section}] has no 'type', skipping") continue - # Build source config by merging general with section-specific - source_config = general.copy() - - for key, value in config.items(section): - if key == 'type': - continue - if key in ('split_minutes', 'max_retries', 'retry_delay_seconds', 'sample_rate', 'channels'): - source_config[key] = int(value) - else: - source_config[key] = value - - # Create recorder + # 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() + + for key, value in config.items(section): + if key == 'type': + continue + if key in ('split_minutes', 'max_retries', 'retry_delay_seconds', 'sample_rate', 'channels'): + source_config[key] = int(value) + else: + source_config[key] = value + if source_type == 'stream': recorder = StreamRecorder(section, source_config, self.logger) self.recorders.append(recorder) diff --git a/tests/test_isr.py b/tests/test_isr.py index 2583387..d7004ba 100644 --- a/tests/test_isr.py +++ b/tests/test_isr.py @@ -591,6 +591,57 @@ url = http://x.com/s2 assert len(mgr.recorders) == 1 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): with pytest.raises(SystemExit): # main() calls sys.exit(1) when the file is missing diff --git a/tests/test_web.py b/tests/test_web.py index f17024e..0479a2d 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1,7 +1,10 @@ """Tests for the adaptive loud-section detector in web.py.""" +import io import math +import wave +import web 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 @@ -160,3 +163,69 @@ def test_cut_filename_rolls_over_the_hour(): def test_cut_filename_falls_back_for_nonstandard_name(): name = _cut_filename("mixtape", ".mp3", 740, 750.4) 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() diff --git a/web.py b/web.py index 8edd33b..5eb89d6 100644 --- a/web.py +++ b/web.py @@ -590,6 +590,9 @@ class _Handler(BaseHTTPRequestHandler): min_gap: float = MIN_GAP_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): parsed = urlparse(self.path) p = parsed.path @@ -962,6 +965,7 @@ class _Handler(BaseHTTPRequestHandler): return ext = path.suffix.lower() + self._clip_started = False try: if ext == '.wav': self._clip_wav(path, start, end) @@ -975,10 +979,19 @@ class _Handler(BaseHTTPRequestHandler): except (ConnectionError, BrokenPipeError): raise except Exception as e: - self._json_err(500, f'Clip failed: {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}') def _send_clip_headers(self, header: bytes, n_frames: int, channels: int, sampwidth: int): + self._clip_started = True self.send_response(200) self.send_header('Content-Type', 'audio/wav') self.send_header('Content-Length', str(len(header) + n_frames * channels * sampwidth))