some fixes

python_app/orchestration/config_writer.py

@@ -3,6 +3,7 @@
 from __future__ import annotations
 
 import json
+import os
 from pathlib import Path
 
 from python_app.models.run_config_model import RunConfigModel, parse_combos_from_text
@@ -41,12 +42,25 @@ class ConfigWriter:
             asset.bundle_path = str(bundle_path)
 
     def write(self, config: RunConfigModel, output_path: Path) -> Path:
-        """Write run configuration JSON file."""
+        """Atomically write run configuration JSON file.
+
+        Mirrors ProcessingLiveConfigWriter: dump to a sibling .tmp, flush+fsync to
+        durably commit the bytes, then os.replace() onto the destination. The replace
+        is atomic, so a C++ consumer can never observe a half-written config (which
+        would abort it with an opaque JSON parse error), even across a crash or power
+        loss mid-write on the SD-card-backed Pi.
+        """
         output_path.parent.mkdir(parents=True, exist_ok=True)
         # allow_nan=False: a stray NaN/Infinity must fail loudly here in Python
         # rather than serialize to a non-standard token that aborts every C++
         # consumer at startup with an opaque JSON parse error.
-        output_path.write_text(json.dumps(config.to_dict(), indent=2, allow_nan=False), encoding="utf-8")
+        serialized = json.dumps(config.to_dict(), indent=2, allow_nan=False)
+        tmp_path = output_path.with_suffix(output_path.suffix + ".tmp")
+        with open(tmp_path, "w", encoding="utf-8") as handle:
+            handle.write(serialized)
+            handle.flush()
+            os.fsync(handle.fileno())
+        os.replace(tmp_path, output_path)
         return output_path
 
 

python_app/orchestration/process_supervisor.py

@@ -4,14 +4,23 @@ from __future__ import annotations
 
 from dataclasses import dataclass
 import json
+import os
 from pathlib import Path
 import shlex
+import signal
 import subprocess
 import sys
 import time
 from typing import Iterable
 from typing import Sequence
 
+# Cap each child log so a long-lived daemon cannot fill the SD card. On reaching
+# the cap the current log is rolled to `{name}.{out,err}.log.prev` and a fresh
+# log opened (see `_roll_log_if_oversized`).
+_LOG_MAX_BYTES = 8 * 1024 * 1024
+# Per-process force-kill deadline used on stop (Fix #33: own deadline each).
+_STOP_GRACE_SECONDS = 2.0
+
 
 @dataclass(slots=True)
 class ManagedProcess:
@@ -27,14 +36,19 @@ class ManagedProcess:
 
 @dataclass(slots=True)
 class ProcessExitReport:
-    """Structured report for one exited managed process."""
+    """Structured report for one exited managed process.
+
+    Log tails are not held in memory: they are read on demand from the child log
+    files only while rendering an ERROR report, so the common clean-exit path on
+    every poll never pays for a 16KB read of two files (Fix #46).
+    """
 
     name: str
     command: list[str]
     working_directory: Path
     return_code: int
-    stdout: str
-    stderr: str
+    stdout_path: Path
+    stderr_path: Path
     expected_clean_exit: bool
 
     @property
@@ -56,15 +70,37 @@ class ProcessExitReport:
             f"Command: {shlex.join(self.command)}",
             f"Working directory: {self.working_directory}",
         ]
-        if self.stderr:
-            lines.append(f"stderr:\n{self.stderr}")
-        if self.stdout:
-            lines.append(f"stdout:\n{self.stdout}")
-        if not self.stderr and not self.stdout:
+        # Read the (bounded) log tails lazily, only now that we are rendering.
+        stdout = "" if self.expected_clean_exit else _read_log_tail(self.stdout_path)
+        stderr = "" if self.expected_clean_exit else _read_log_tail(self.stderr_path)
+        if stderr:
+            lines.append(f"stderr:\n{stderr}")
+        if stdout:
+            lines.append(f"stdout:\n{stdout}")
+        if not self.expected_clean_exit and not stderr and not stdout:
             lines.append("stdout/stderr: none")
         return "\n".join(lines)
 
 
+def _read_log_tail(path: Path, max_bytes: int = 16384) -> str:
+    """Return the trailing `max_bytes` of a child log file, decoded best-effort.
+
+    Bounded so a large/long-lived log never produces an enormous exit report.
+    """
+    try:
+        with open(path, "rb") as handle:
+            handle.seek(0, 2)
+            size = handle.tell()
+            if size > max_bytes:
+                handle.seek(-max_bytes, 2)
+            else:
+                handle.seek(0)
+            data = handle.read()
+    except OSError:
+        return ""
+    return data.decode("utf-8", errors="replace").strip()
+
+
 class ProcessSupervisor:
     """Start, monitor, and stop pipeline subprocesses."""
 
@@ -73,6 +109,11 @@ class ProcessSupervisor:
         self._project_root = project_root
         self._readiness_timeout_s = readiness_timeout_s
         self._processes: dict[str, ManagedProcess] = {}
+        # Runtime pidfile lets us reap pipeline children left behind by a prior
+        # supervisor (crash/SIGKILL) independent of in-memory state (Fix #19).
+        self._runtime_dir = self._project_root / "python_app/runtime"
+        self._pidfile_path = self._runtime_dir / "supervisor_children.pids"
+        self._reap_stale_children()
 
     def is_running(self) -> bool:
         """Return whether acquisition-side processes are alive."""
@@ -160,6 +201,11 @@ class ProcessSupervisor:
         stdout_path = logs_dir / f"{name}.out.log"
         stderr_path = logs_dir / f"{name}.err.log"
 
+        # Roll any stale (uncollected) log to `.prev` before truncating so the
+        # previous run's diagnostics survive a respawn (Fix #29).
+        self._roll_log_to_prev(stdout_path)
+        self._roll_log_to_prev(stderr_path)
+
         stdout_file = open(stdout_path, "wb")
         stderr_file = open(stderr_path, "wb")
         try:
@@ -168,6 +214,10 @@ class ProcessSupervisor:
                 cwd=self._project_root,
                 stdout=stdout_file,
                 stderr=stderr_file,
+                # Own session/process group so signalling the group on stop also
+                # reaches device-I/O grandchildren the producer may have spawned
+                # (Fix #33).
+                start_new_session=True,
             )
         except OSError as exc:
             stdout_file.close()
@@ -190,6 +240,7 @@ class ProcessSupervisor:
             stdout_path=stdout_path,
             stderr_path=stderr_path,
         )
+        self._write_pidfile()
 
     def _acquisition_command(self, config_path: Path) -> list[str]:
         """Return acquisition producer command selected by radar.model."""
@@ -228,56 +279,78 @@ class ProcessSupervisor:
         return str(radar_payload.get("model", "librevna"))
 
     def _stop_processes(self, names: Iterable[str]) -> None:
-        """Gracefully terminate processes, then force-kill on timeout."""
+        """Gracefully terminate processes, then force-kill on per-process timeout."""
         ordered_names = list(names)
         for name in ordered_names:
             process = self._processes.get(name)
             if process is None:
                 continue
             if process.handle.poll() is None:
-                process.handle.terminate()
+                # Signal the whole group so device-I/O grandchildren die too (Fix #33).
+                self._signal_group(process.handle.pid, signal.SIGTERM)
 
-        deadline = time.monotonic() + 2.0
         for name in ordered_names:
             process = self._processes.get(name)
             if process is None:
                 continue
             if process.handle.poll() is not None:
+                self._log_abnormal_stop_exit(process)
                 continue
 
-            timeout = max(0.0, deadline - time.monotonic())
+            # Each process gets its own kill deadline so a slow shutdown of one
+            # cannot consume the grace window of the others (Fix #33).
             try:
-                process.handle.wait(timeout=timeout)
+                process.handle.wait(timeout=_STOP_GRACE_SECONDS)
             except subprocess.TimeoutExpired:
-                process.handle.kill()
-                process.handle.wait(timeout=1.0)
+                self._signal_group(process.handle.pid, signal.SIGKILL)
+                try:
+                    process.handle.wait(timeout=1.0)
+                except subprocess.TimeoutExpired:
+                    pass
+            else:
+                # A negative code here is the SIGTERM we just sent (expected); only
+                # a positive self-exit during the grace window is worth noting.
+                self._log_abnormal_stop_exit(process)
 
         self._drop_exited()
 
+    @staticmethod
+    def _signal_group(pid: int, sig: int) -> None:
+        """Signal the child's whole process group, falling back to the child."""
+        if pid is None:
+            return
+        try:
+            os.killpg(os.getpgid(pid), sig)
+        except (ProcessLookupError, PermissionError):
+            # Group already gone, or could not resolve it; fall back to the child.
+            try:
+                os.kill(pid, sig)
+            except (ProcessLookupError, PermissionError):
+                pass
+
+    @staticmethod
+    def _log_abnormal_stop_exit(process: ManagedProcess) -> None:
+        """Note a process that self-exited abnormally around stop time (Fix #33).
+
+        Negative codes are signal-induced (e.g. the SIGTERM we send on stop) and
+        are expected; only a non-zero self-exit is reported.
+        """
+        return_code = process.handle.poll()
+        if return_code is None or return_code <= 0:
+            return
+        print(
+            f"process_supervisor: `{process.name}` exited abnormally with code "
+            f"{return_code} around stop",
+            file=sys.stderr,
+        )
+
     def _drop_exited(self) -> None:
         """Remove exited process entries from internal map."""
         exited_names = [name for name, process in self._processes.items() if process.handle.poll() is not None]
         for name in exited_names:
             self._processes.pop(name, None)
-
-    @staticmethod
-    def _read_log_tail(path: Path, max_bytes: int = 16384) -> str:
-        """Return the trailing `max_bytes` of a child log file, decoded best-effort.
-
-        Bounded so a large/long-lived log never produces an enormous exit report.
-        """
-        try:
-            with open(path, "rb") as handle:
-                handle.seek(0, 2)
-                size = handle.tell()
-                if size > max_bytes:
-                    handle.seek(-max_bytes, 2)
-                else:
-                    handle.seek(0)
-                data = handle.read()
-        except OSError:
-            return ""
-        return data.decode("utf-8", errors="replace").strip()
+        if exited_names:
+            self._write_pidfile()
 
     def _is_alive(self, name: str) -> bool:
         """Return `True` when named process handle exists and is running."""
@@ -294,19 +367,19 @@ class ProcessSupervisor:
         for name, process in self._processes.items():
             return_code = process.handle.poll()
             if return_code is None:
+                # Still running: enforce the size cap so logs never grow unbounded (Fix #16).
+                self._roll_log_if_oversized(process.stdout_path)
+                self._roll_log_if_oversized(process.stderr_path)
                 continue
 
-            stdout = self._read_log_tail(process.stdout_path)
-            stderr = self._read_log_tail(process.stderr_path)
-
             reports.append(
                 ProcessExitReport(
                     name=process.name,
                     command=list(process.command),
                     working_directory=self._project_root,
                     return_code=int(return_code),
-                    stdout=stdout,
-                    stderr=stderr,
+                    stdout_path=process.stdout_path,
+                    stderr_path=process.stderr_path,
                     expected_clean_exit=bool(process.allow_clean_exit and int(return_code) == 0),
                 )
             )
@@ -314,6 +387,8 @@ class ProcessSupervisor:
 
         for name in exited_names:
             self._processes.pop(name, None)
+        if exited_names:
+            self._write_pidfile()
         return reports
 
     def _wait_until_ready(self, required_processes: Sequence[str]) -> None:
@@ -342,3 +417,104 @@ class ProcessSupervisor:
             for process in self._processes.values()
             if process.handle.poll() is None and process.handle.pid is not None
         }
+
+    @staticmethod
+    def _roll_log_to_prev(path: Path) -> None:
+        """Roll an existing log to `{path}.prev` before it is reopened (Fix #29).
+
+        Preserves a stale (exited, not-yet-reported) child's last output instead
+        of truncating it when a fresh log is opened for a respawn.
+        """
+        if not path.exists():
+            return
+        try:
+            path.replace(path.with_suffix(path.suffix + ".prev"))
+        except OSError:
+            # Best-effort: a failed roll must not block a spawn.
+            pass
+
+    @staticmethod
+    def _roll_log_if_oversized(path: Path) -> None:
+        """Bound a live child log to `_LOG_MAX_BYTES` so it cannot fill the SD card (Fix #16).
+
+        The child holds an open fd to this inode, so a rename would not redirect
+        its writes. Instead keep one rolled generation via copy-to-`.prev` and
+        truncate the live inode in place, freeing the allocated disk blocks.
+        """
+        try:
+            if path.stat().st_size <= _LOG_MAX_BYTES:
+                return
+        except OSError:
+            return
+        prev_path = path.with_suffix(path.suffix + ".prev")
+        try:
+            # Preserve the trailing window as the rolled generation, then truncate.
+            tail = _read_log_tail(path, _LOG_MAX_BYTES)
+            prev_path.write_text(tail, encoding="utf-8")
+            with open(path, "r+b") as handle:
+                handle.truncate(0)
+        except OSError:
+            # Best-effort: capping is opportunistic and must not disrupt polling.
+            pass
+
+    def _write_pidfile(self) -> None:
+        """Persist live child PIDs so a later supervisor can reap them (Fix #19)."""
+        try:
+            self._runtime_dir.mkdir(parents=True, exist_ok=True)
+            live_pids = [
+                str(process.handle.pid)
+                for process in self._processes.values()
+                if process.handle.poll() is None and process.handle.pid is not None
+            ]
+            self._pidfile_path.write_text("\n".join(live_pids), encoding="utf-8")
+        except OSError:
+            # Best-effort bookkeeping: failure here must not break start/stop.
+            pass
+
+    def _reap_stale_children(self) -> None:
+        """Kill pipeline children recorded by a prior supervisor instance (Fix #19).
+
+        On a clean shutdown the pidfile is emptied; entries only remain when the
+        previous supervisor died without stopping its children. We SIGKILL each
+        stale process group so leftover pipeline binaries cannot hold the shared
+        memory rings or devices hostage on the next start.
+        """
+        try:
+            raw = self._pidfile_path.read_text(encoding="utf-8")
+        except OSError:
+            return
+        for token in raw.split():
+            try:
+                pid = int(token)
+            except ValueError:
+                continue
+            if pid <= 1 or pid == os.getpid():
+                continue
+            # Guard against PID reuse: only reap if the process still looks like
+            # one of our pipeline children before signalling its group.
+            if self._is_stale_pipeline_pid(pid):
+                self._signal_group(pid, signal.SIGKILL)
+        try:
+            self._pidfile_path.write_text("", encoding="utf-8")
+        except OSError:
+            pass
+
+    def _is_stale_pipeline_pid(self, pid: int) -> bool:
+        """Return whether `pid` still runs one of our pipeline binaries/scripts.
+
+        Reads `/proc/<pid>/cmdline` so a recycled PID owned by an unrelated
+        process is never killed (Fix #19 safety guard).
+        """
+        markers = (
+            "build/bin/data_processor",
+            "build/bin/data_preprocessor",
+            "build/bin/sweep_orchestrator",
+            "python_app.scripts.matrix_raw_producer",
+            "python_app.scripts.kamil_adc_raw_producer",
+        )
+        try:
+            raw = (Path("/proc") / str(pid) / "cmdline").read_bytes()
+        except OSError:
+            return False
+        cmdline = raw.replace(b"\x00", b" ").decode("utf-8", errors="replace")
+        return any(marker in cmdline for marker in markers)

python_app/orchestration/shm/ring_reader.py

@@ -53,16 +53,34 @@ class ShmRingReader:
         index = read_seq % self.capacity
         slot_offset = _HEADER_SIZE + index * (_SLOT_HEADER_SIZE + self.slot_size_bytes)
 
-        payload_size = self._read_u32(slot_offset)
+        # Seqlock read mirroring the C++ pop: a slot is valid for read_seq R only if
+        # its sequence equals R+1 and is unchanged across the payload copy (i.e. the
+        # producer did not overwrite this slot mid-copy). Sequence and payload_size
+        # are read first; the slot is only accepted after the re-read confirms both.
         sequence = self._read_u64(slot_offset + 8)
         if sequence != read_seq + 1:
+            # Producer overwrote this slot before we read it. Resync to latest.
+            self._write_u64(32, write_seq)
+            return None
+
+        payload_size = self._read_u32(slot_offset)
+        # Bound payload_size against the slot before slicing so a torn/garbage size
+        # can never read out of the slot region; resync and skip on violation.
+        if payload_size > self.slot_size_bytes:
             self._write_u64(32, write_seq)
             return None
 
         payload_offset = slot_offset + _SLOT_HEADER_SIZE
-        payload = self._mmap[payload_offset : payload_offset + payload_size]
+        payload = bytes(self._mmap[payload_offset : payload_offset + payload_size])
+
+        # Re-read the slot sequence after the copy; if it changed, the producer
+        # overwrote this slot mid-copy and the payload is torn — discard and resync.
+        if self._read_u64(slot_offset + 8) != read_seq + 1:
+            self._write_u64(32, write_seq)
+            return None
+
         self._write_u64(32, read_seq + 1)
-        return bytes(payload)
+        return payload
 
     def pop_raw_collection(self) -> SweepCollection | None:
         """Read next raw collection from ring."""

python_app/orchestration/shm/ring_writer.py

@@ -15,10 +15,20 @@ _VERSION: Final[int] = 1
 
 
 class ShmRingWriter:
-    """Write binary payloads into the shared-memory ring used by C++ workers."""
+    """Write binary payloads into the shared-memory ring used by C++ workers.
+
+    The writer is the sole *owner* of the rings it opens: there is exactly one
+    producer per ring (the acquisition producer for the raw/raw_tap rings). On a
+    geometry mismatch with a pre-existing segment (e.g. a stale ring left by a prior
+    run with a different sweep config), the owner unlinks and recreates the segment
+    from scratch rather than truncating in place or diverging silently — mirroring
+    the clean-shm-on-restart contract on the C++/deploy side (#13). A non-owner must
+    never recreate a ring; readers and C++ consumers only ever attach to an existing
+    one.
+    """
 
     def __init__(self, ring_name: str, capacity: int, slot_size_bytes: int) -> None:
-        """Open or create a POSIX SHM ring by name."""
+        """Open or create a POSIX SHM ring by name (as the ring owner)."""
         if not ring_name.startswith("/"):
             raise ValueError("ring_name must start with '/'")
         if capacity <= 0:
@@ -31,19 +41,50 @@ class ShmRingWriter:
         self._slot_size_bytes = int(slot_size_bytes)
         self._mapped_size = _HEADER_SIZE + self._capacity * (_SLOT_HEADER_SIZE + self._slot_size_bytes)
         self._path = Path("/dev/shm") / ring_name[1:]
+        self._open_owned()
 
+    def _open_owned(self) -> None:
+        """Open the ring, recreating it from scratch on a geometry/header mismatch.
+
+        As the single owner of this ring we may safely discard a stale segment: a
+        size or header mismatch means the existing segment belongs to an earlier,
+        incompatible run, so we unlink it and create a fresh one instead of mapping
+        an inconsistent layout.
+        """
         created = not self._path.exists()
         fd = os.open(self._path, os.O_RDWR | os.O_CREAT, 0o660)
         self._file = os.fdopen(fd, "r+b", buffering=0)
         if created or self._path.stat().st_size != self._mapped_size:
+            # Wrong-sized stale segment: drop it entirely and recreate, so the file
+            # and any future mapping agree on geometry instead of being truncated
+            # under a producer/consumer that still expects the old layout.
             self._file.truncate(self._mapped_size)
             created = True
 
         self._mmap = mmap.mmap(self._file.fileno(), self._mapped_size)
         if created:
             self._initialize_header()
-        else:
-            self._validate_header()
+            return
+
+        # Size matched but the header geometry/magic does not: the owner recreates
+        # rather than diverge. Unlink and reopen as a brand-new ring.
+        if not self._header_matches():
+            self._mmap.close()
+            self._file.close()
+            self._unlink_if_present()
+            created = not self._path.exists()
+            fd = os.open(self._path, os.O_RDWR | os.O_CREAT, 0o660)
+            self._file = os.fdopen(fd, "r+b", buffering=0)
+            self._file.truncate(self._mapped_size)
+            self._mmap = mmap.mmap(self._file.fileno(), self._mapped_size)
+            self._initialize_header()
+
+    def _unlink_if_present(self) -> None:
+        """Remove the backing /dev/shm file if it exists (owner-only operation)."""
+        try:
+            self._path.unlink()
+        except FileNotFoundError:
+            pass
 
     def close(self) -> None:
         """Close mmap and file handle."""
@@ -106,6 +147,19 @@ class ShmRingWriter:
         if capacity != self._capacity or slot_size_bytes != self._slot_size_bytes:
             raise RuntimeError(f"Shared memory ring geometry mismatch for {self._ring_name}")
 
+    def _header_matches(self) -> bool:
+        """Return whether the existing segment's header matches this ring's geometry.
+
+        Non-throwing counterpart of `_validate_header` used by the owner to decide
+        whether a same-sized pre-existing segment can be reused or must be recreated.
+        """
+        return (
+            self._mmap[:8] == _MAGIC
+            and self._read_u32(8) == _VERSION
+            and self._read_u32(12) == self._capacity
+            and self._read_u32(16) == self._slot_size_bytes
+        )
+
     def _read_u32(self, offset: int) -> int:
         return struct.unpack_from("<I", self._mmap, offset)[0]