Make PulseAudio readiness mean a live connection, clear stale socket state

The pulse_socket named volume survives container recreation, so after a compose recreate the previous container's /run/pulse/pid and native socket were still present. PulseAudio read the stale pid file, decided a daemon was already running, and refused to start: E: [pulseaudio] pid.c: Daemon already running. The entrypoint still reported "PulseAudio socket ready" because it only checked that the socket file existed - and a stale one did. Capture then failed in a restart loop against a dead daemon. Readiness in both the op25 entrypoint and drb-edge-node now means a pactl probe actually succeeds. Stale pid/socket are removed only when that probe fails, so a live daemon's socket is never deleted. pulseaudio-utils was missing from the edge-node image (only libpulse0 was installed), so no pactl binary existed there at all - added. Capture exits are now classified: a missing source logs at ERROR and names the configured PULSE_SOURCE, rather than looking identical to "daemon not up yet". Retrying forever against a wrong source name is how the April PulseAudio failure stayed hidden. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-04 22:10:28 -04:00
parent b0a8ed2a5a
commit 7c4a3f2f20
6 changed files with 402 additions and 54 deletions
@@ -421,4 +421,64 @@ def test_memory_ceiling_covers_the_longest_allowed_call():
    """The cap must bound RAM without ever being able to truncate a legal call."""
    bytes_per_second = 16_000 // 8
    assert MAX_RECORDING_BYTES >= MAX_RECORDING_SECONDS * bytes_per_second
+
+
+# ---------------------------------------------------------------------------
+# Capture-exit classification — the two failure modes must be told apart
+# instead of both logging the same generic "restarting" line. This is what
+# let a wrong PULSE_SOURCE hide behind normal-looking startup retries before.
+# ---------------------------------------------------------------------------
+
+def _log_levels(caplog, logger_name="drb-edge-node"):
+    return [r.levelname for r in caplog.records if r.name == logger_name]
+
+
+def test_capture_exit_logs_error_when_source_missing(recorder, caplog):
+    """FFmpeg's pulse input prints 'No such process' when the daemon is up
+    but the configured source name does not exist — a real misconfiguration,
+    not a startup race, so this must stand out as an error naming the source."""
+    recorder._last_stderr_lines.append(
+        "[pulse @ 0x...] pa_stream_connect_record failed: No such process"
+    )
+    with caplog.at_level("INFO", logger="drb-edge-node"):
+        recorder._log_capture_exit()
+
+    assert "ERROR" in _log_levels(caplog)
+    error_messages = [r.message for r in caplog.records if r.levelname == "ERROR"]
+    assert any(settings.pulse_source in m for m in error_messages)
+
+
+def test_capture_exit_logs_info_when_no_daemon(recorder, caplog):
+    """Connection refused means nothing is listening yet — expected during
+    startup, so it must NOT be logged at the same severity as a real
+    misconfiguration."""
+    recorder._last_stderr_lines.append(
+        "[pulse @ 0x...] pa_context_connect() failed: Connection refused"
+    )
+    with caplog.at_level("INFO", logger="drb-edge-node"):
+        recorder._log_capture_exit()
+
+    levels = _log_levels(caplog)
+    assert "ERROR" not in levels
+    assert "INFO" in levels
+
+
+def test_capture_exit_falls_back_to_generic_warning(recorder, caplog):
+    """An FFmpeg failure that matches neither known marker keeps the original
+    generic behavior rather than guessing."""
+    recorder._last_stderr_lines.append("[pulse @ 0x...] some other unexpected failure")
+    with caplog.at_level("INFO", logger="drb-edge-node"):
+        recorder._log_capture_exit()
+
+    assert _log_levels(caplog) == ["WARNING"]
+
+
+def test_capture_exit_with_no_stderr_captured_is_generic_warning(recorder, caplog):
+    """No stderr at all (e.g. FFmpeg killed before printing anything) must not
+    crash the classifier and must fall back to the generic message."""
+    assert list(recorder._last_stderr_lines) == []
+    with caplog.at_level("INFO", logger="drb-edge-node"):
+        recorder._log_capture_exit()
+
+    assert _log_levels(caplog) == ["WARNING"]
    assert MAX_RECORDING_BYTES <= 8 * 1024 * 1024, "must stay small enough for a Pi"
@@ -0,0 +1,135 @@
+"""
+Unit tests for PulseAudio readiness helpers (app.internal.pulse).
+
+The whole point of this module is that readiness means "a PulseAudio
+connection actually succeeds," never "a socket file exists at this path" —
+that was the exact bug reproduced on live hardware: a killed daemon left its
+pid file and native socket behind in the shared `pulse_socket` volume, the
+old file-existence check reported "ready", and FFmpeg launched against a
+dead daemon.
+
+No real PulseAudio daemon or `pactl` binary is required for these tests:
+`_daemon_responds` (the one function that actually shells out) is monkeypatched
+everywhere except the dedicated subprocess-layer tests, which fake out
+`shutil.which`/`subprocess.run` directly so the "stale file, dead daemon" case
+is proven at the layer that matters.
+"""
+import subprocess
+from unittest.mock import Mock
+
+from app.internal import pulse
+
+
+# ---------------------------------------------------------------------------
+# socket_path()
+# ---------------------------------------------------------------------------
+
+def test_socket_path_defaults_when_pulse_server_unset(monkeypatch):
+    monkeypatch.delenv("PULSE_SERVER", raising=False)
+    assert pulse.socket_path() == pulse.DEFAULT_SOCKET_PATH
+
+
+def test_socket_path_parses_unix_prefixed_pulse_server(monkeypatch):
+    monkeypatch.setenv("PULSE_SERVER", "unix:/tmp/somewhere/native")
+    assert pulse.socket_path() == "/tmp/somewhere/native"
+
+
+def test_socket_path_falls_back_on_malformed_pulse_server(monkeypatch):
+    monkeypatch.setenv("PULSE_SERVER", "not-a-unix-uri")
+    assert pulse.socket_path() == pulse.DEFAULT_SOCKET_PATH
+
+
+# ---------------------------------------------------------------------------
+# is_ready() / wait_until_ready() against a monkeypatched probe
+# ---------------------------------------------------------------------------
+
+def test_is_ready_true_when_daemon_responds(monkeypatch):
+    monkeypatch.setattr(pulse, "_daemon_responds", lambda: True)
+    assert pulse.is_ready() is True
+
+
+def test_is_ready_false_when_daemon_does_not_respond(monkeypatch):
+    monkeypatch.setattr(pulse, "_daemon_responds", lambda: False)
+    assert pulse.is_ready() is False
+
+
+async def test_wait_until_ready_short_circuits_when_already_live(monkeypatch):
+    calls = Mock(return_value=True)
+    monkeypatch.setattr(pulse, "_daemon_responds", calls)
+    assert await pulse.wait_until_ready(timeout=5) is True
+    assert calls.call_count == 1
+
+
+async def test_wait_until_ready_polls_until_daemon_comes_up(monkeypatch):
+    monkeypatch.setattr(pulse, "POLL_INTERVAL", 0.01)
+    responses = iter([False, False, True])
+    monkeypatch.setattr(pulse, "_daemon_responds", lambda: next(responses))
+    assert await pulse.wait_until_ready(timeout=5) is True
+
+
+async def test_wait_until_ready_times_out_when_daemon_never_responds(monkeypatch):
+    monkeypatch.setattr(pulse, "POLL_INTERVAL", 0.01)
+    monkeypatch.setattr(pulse, "_daemon_responds", lambda: False)
+    assert await pulse.wait_until_ready(timeout=0.05) is False
+
+
+async def test_wait_until_ready_uses_settings_default_when_timeout_omitted(monkeypatch):
+    monkeypatch.setattr(pulse.settings, "pulse_wait_timeout", 0.05)
+    monkeypatch.setattr(pulse, "POLL_INTERVAL", 0.01)
+    monkeypatch.setattr(pulse, "_daemon_responds", lambda: False)
+    assert await pulse.wait_until_ready() is False
+
+
+# ---------------------------------------------------------------------------
+# _daemon_responds() at the subprocess layer — proves a stale FILE is not
+# enough, which is the actual regression this module fixes.
+# ---------------------------------------------------------------------------
+
+def test_daemon_responds_false_when_pactl_missing(monkeypatch):
+    monkeypatch.setattr(pulse.shutil, "which", lambda name: None)
+    assert pulse._daemon_responds() is False
+
+
+def test_daemon_responds_false_on_probe_timeout(monkeypatch, tmp_path):
+    monkeypatch.setattr(pulse.shutil, "which", lambda name: "/usr/bin/pactl")
+
+    def fake_run(*args, **kwargs):
+        raise subprocess.TimeoutExpired(cmd="pactl", timeout=pulse.PROBE_TIMEOUT_SECONDS)
+
+    monkeypatch.setattr(pulse.subprocess, "run", fake_run)
+    assert pulse._daemon_responds() is False
+
+
+def test_daemon_responds_false_when_stale_socket_file_exists_but_daemon_dead(monkeypatch, tmp_path):
+    """
+    The regression, reproduced at the layer that matters: a plain FILE sits
+    at the socket path (exactly what a killed daemon leaves behind), but
+    `pactl info` against it fails (nonzero exit — connection refused). This
+    must NOT be treated as ready.
+    """
+    stale_socket = tmp_path / "native"
+    stale_socket.write_bytes(b"")  # a stale file, not a live socket
+    monkeypatch.setenv("PULSE_SERVER", f"unix:{stale_socket}")
+
+    monkeypatch.setattr(pulse.shutil, "which", lambda name: "/usr/bin/pactl")
+    monkeypatch.setattr(
+        pulse.subprocess, "run",
+        lambda *a, **k: subprocess.CompletedProcess(args=a, returncode=1),
+    )
+
+    assert stale_socket.exists()  # sanity: the old file-existence check would pass
+    assert pulse._daemon_responds() is False
+
+
+def test_daemon_responds_true_when_pactl_succeeds(monkeypatch, tmp_path):
+    live_socket = tmp_path / "native"
+    live_socket.write_bytes(b"")
+    monkeypatch.setenv("PULSE_SERVER", f"unix:{live_socket}")
+
+    monkeypatch.setattr(pulse.shutil, "which", lambda name: "/usr/bin/pactl")
+    monkeypatch.setattr(
+        pulse.subprocess, "run",
+        lambda *a, **k: subprocess.CompletedProcess(args=a, returncode=0),
+    )
+
+    assert pulse._daemon_responds() is True