From a08fe79cef05393e0d388a42cc1bf89fa23f4124 Mon Sep 17 00:00:00 2001 From: firestar5683 <168790843+firestar5683@users.noreply.github.com> Date: Mon, 6 Jul 2026 22:32:54 -0500 Subject: [PATCH] fixes --- .../tests/test_navigation_params.py | 28 +++++++++++ starpilot/system/the_galaxy/the_galaxy.py | 4 +- system/manager/manager.py | 48 +++++++++++++++++++ system/manager/test/test_manager.py | 34 +++++++++++++ system/tests/test_tombstoned.py | 24 ++++++++++ system/tombstoned.py | 35 ++++++++++++-- tools/lateral_maneuvers/lateral_maneuversd.py | 2 +- .../test_lateral_maneuversd.py | 22 +++++++++ 8 files changed, 190 insertions(+), 7 deletions(-) create mode 100644 system/tests/test_tombstoned.py create mode 100644 tools/lateral_maneuvers/test_lateral_maneuversd.py diff --git a/starpilot/system/the_galaxy/tests/test_navigation_params.py b/starpilot/system/the_galaxy/tests/test_navigation_params.py index 04e3a7139..35ff03fc7 100644 --- a/starpilot/system/the_galaxy/tests/test_navigation_params.py +++ b/starpilot/system/the_galaxy/tests/test_navigation_params.py @@ -149,6 +149,34 @@ def test_navigation_last_position_rejects_stale_persisted_fix(monkeypatch): assert the_galaxy._get_navigation_last_position() is None +def test_save_longitudinal_maneuver_status_writes_json_param_as_dict(monkeypatch): + fake_params = WritableFakeParams() + monkeypatch.setattr(the_galaxy, "params", fake_params) + + saved = the_galaxy._save_longitudinal_maneuver_status({ + "state": "armed", + "history": ["", "Started"], + }) + + assert fake_params.writes == [("LongitudinalManeuverStatus", saved)] + assert isinstance(fake_params.writes[0][1], dict) + assert saved["history"] == ["Started"] + + +def test_save_lateral_maneuver_status_writes_json_param_as_dict(monkeypatch): + fake_params = WritableFakeParams() + monkeypatch.setattr(the_galaxy, "params", fake_params) + + saved = the_galaxy._save_lateral_maneuver_status({ + "state": "armed", + "history": ["", "Started"], + }) + + assert fake_params.writes == [("LateralManeuverStatus", saved)] + assert isinstance(fake_params.writes[0][1], dict) + assert saved["history"] == ["Started"] + + def test_galaxy_session_value_matches_cookie_format(): assert the_galaxy._build_galaxy_session_value( "testGalaxySlug01", diff --git a/starpilot/system/the_galaxy/the_galaxy.py b/starpilot/system/the_galaxy/the_galaxy.py index de83632b8..d4f09e858 100644 --- a/starpilot/system/the_galaxy/the_galaxy.py +++ b/starpilot/system/the_galaxy/the_galaxy.py @@ -3564,7 +3564,7 @@ def _save_longitudinal_maneuver_status(status): history = [] status_copy["history"] = [str(line) for line in history if str(line).strip()][-120:] status_copy["updatedAtSec"] = float(status_copy.get("updatedAtSec") or time.monotonic()) - params.put("LongitudinalManeuverStatus", json.dumps(status_copy, separators=(",", ":"))) + params.put("LongitudinalManeuverStatus", status_copy) return status_copy def _append_longitudinal_maneuver_history(status, line): @@ -3693,7 +3693,7 @@ def _save_lateral_maneuver_status(status): history = [] status_copy["history"] = [str(line) for line in history if str(line).strip()][-120:] status_copy["updatedAtSec"] = float(status_copy.get("updatedAtSec") or time.monotonic()) - params.put("LateralManeuverStatus", json.dumps(status_copy, separators=(",", ":"))) + params.put("LateralManeuverStatus", status_copy) return status_copy diff --git a/system/manager/manager.py b/system/manager/manager.py index d3875f21b..0ddd05d5e 100755 --- a/system/manager/manager.py +++ b/system/manager/manager.py @@ -5,6 +5,7 @@ import os import shutil from pathlib import Path import signal +import stat import sys import time import traceback @@ -759,6 +760,48 @@ def migrate_legacy_experimental_longitudinal(params: Params, params_cache: Param params_cache.remove("ExperimentalLongitudinalEnabled") +def _msgq_file_is_readwrite_openable(path: Path) -> bool: + fd = os.open(path, os.O_RDWR | getattr(os, "O_CLOEXEC", 0)) + try: + return True + finally: + os.close(fd) + + +def cleanup_inaccessible_msgq_files(shm_path: str | Path) -> int: + """Remove stale msgq files that would crash SubSocket/PubSocket creation.""" + shm_root = Path(shm_path) + if not shm_root.is_dir(): + return 0 + + removed = 0 + for path in shm_root.rglob("msgq_*"): + try: + st = path.lstat() + except OSError: + continue + + if not stat.S_ISREG(st.st_mode): + continue + + try: + _msgq_file_is_readwrite_openable(path) + continue + except PermissionError: + pass + except OSError: + continue + + try: + path.unlink() + removed += 1 + cloudlog.warning(f"Removed inaccessible stale msgq file: {path}") + except OSError: + cloudlog.exception(f"Failed to remove inaccessible stale msgq file: {path}") + + return removed + + def manager_init() -> None: manager_init_start = time.monotonic() last_timing = _log_boot_timing("manager_init", "start", manager_init_start, manager_init_start) @@ -829,6 +872,11 @@ def manager_init() -> None: print(f"WARNING: failed to make {Paths.shm_path()}") last_timing = _log_boot_timing("manager_init", "shm_path", manager_init_start, last_timing) + removed_msgq_files = cleanup_inaccessible_msgq_files(Paths.shm_path()) + if removed_msgq_files: + cloudlog.warning(f"Removed {removed_msgq_files} inaccessible stale msgq files before process startup") + last_timing = _log_boot_timing("manager_init", "msgq_cleanup", manager_init_start, last_timing) + # set params serial = HARDWARE.get_serial() params.put("Version", build_metadata.openpilot.version) diff --git a/system/manager/test/test_manager.py b/system/manager/test/test_manager.py index dd5f3e582..b6837713b 100644 --- a/system/manager/test/test_manager.py +++ b/system/manager/test/test_manager.py @@ -376,6 +376,40 @@ class TestManager: assert params.get_bool("PrioritizeSmoothFollowing") + def test_cleanup_inaccessible_msgq_files_removes_only_blocked_files(self, tmp_path, monkeypatch): + healthy = tmp_path / "msgq_deviceState" + blocked = tmp_path / "msgq_gpsLocation" + unrelated = tmp_path / "not_msgq_gpsLocation" + healthy.write_bytes(b"healthy") + blocked.write_bytes(b"blocked") + unrelated.write_bytes(b"unrelated") + + def fake_open_probe(path): + if path == blocked: + raise PermissionError("blocked") + return True + + monkeypatch.setattr(manager, "_msgq_file_is_readwrite_openable", fake_open_probe) + + assert manager.cleanup_inaccessible_msgq_files(tmp_path) == 1 + assert healthy.read_bytes() == b"healthy" + assert not blocked.exists() + assert unrelated.read_bytes() == b"unrelated" + + def test_cleanup_inaccessible_msgq_files_ignores_msgq_directories(self, tmp_path, monkeypatch): + msgq_dir = tmp_path / "msgq_desktop" + msgq_dir.mkdir() + child = msgq_dir / "gpsLocation" + child.write_bytes(b"child") + + def fake_open_probe(path): + raise AssertionError(f"directories and non-msgq children should not be probed: {path}") + + monkeypatch.setattr(manager, "_msgq_file_is_readwrite_openable", fake_open_probe) + + assert manager.cleanup_inaccessible_msgq_files(tmp_path) == 0 + assert child.read_bytes() == b"child" + @pytest.mark.skip("this test is flaky the way it's currently written, should be moved to test_onroad") def test_clean_exit(self, subtests): """ diff --git a/system/tests/test_tombstoned.py b/system/tests/test_tombstoned.py new file mode 100644 index 000000000..b68d63c2f --- /dev/null +++ b/system/tests/test_tombstoned.py @@ -0,0 +1,24 @@ +from openpilot.system import tombstoned + + +def test_report_tombstone_apport_ignores_hal3_direct_widthfix(tmp_path, monkeypatch): + crash = tmp_path / "_data_agnos-compat_bin_hal3_direct_widthfix.0.crash" + crash.write_text("\n".join([ + "ProblemType: Crash", + "ExecutablePath: /data/agnos-compat/bin/hal3_direct_widthfix", + "Signal: 11", + "CoreDump: base64", + ])) + + sentry_calls = [] + monkeypatch.setattr(tombstoned.sentry, "report_tombstone", lambda *args: sentry_calls.append(args)) + + def fail_retrace(fn): + raise AssertionError(f"ignored tombstones should not be retraced: {fn}") + + monkeypatch.setattr(tombstoned, "get_apport_stacktrace", fail_retrace) + + tombstoned.report_tombstone_apport(str(crash)) + + assert sentry_calls == [] + assert not crash.exists() diff --git a/system/tombstoned.py b/system/tombstoned.py index 5bcced266..0f5eee7fb 100755 --- a/system/tombstoned.py +++ b/system/tombstoned.py @@ -19,6 +19,9 @@ MAX_TOMBSTONE_FN_LEN = 62 # 85 - 23 ("/crash/") TOMBSTONE_DIR = "/data/tombstones/" APPORT_DIR = "/var/crash/" +IGNORED_APPORT_EXECUTABLES = { + "/data/agnos-compat/bin/hal3_direct_widthfix", +} def safe_fn(s): @@ -44,6 +47,27 @@ def get_apport_stacktrace(fn): return "Timeout getting stacktrace" +def get_apport_executable_path(fn: str) -> str: + try: + with open(fn) as f: + for line in f: + if line.startswith("ExecutablePath:"): + return line.strip().split(': ', 1)[-1] + if line.startswith("CoreDump"): + break + except OSError: + pass + + return "" + + +def remove_crashlog(fn: str) -> None: + try: + os.remove(fn) + except PermissionError: + pass + + def get_tombstones(): """Returns list of (filename, ctime) for all crashlogs""" files = [] @@ -64,6 +88,12 @@ def report_tombstone_apport(fn): cloudlog.error(f"Tombstone {fn} too big, {f_size}. Skipping...") return + executable_path = get_apport_executable_path(fn) + if executable_path in IGNORED_APPORT_EXECUTABLES: + cloudlog.warning(f"Ignoring known agnos-compat tombstone: {executable_path}") + remove_crashlog(fn) + return + message = "" # One line description of the crash contents = "" # Full file contents without coredump path = "" # File path relative to openpilot directory @@ -134,10 +164,7 @@ def report_tombstone_apport(fn): # Files could be on different filesystems, copy, then delete shutil.copy(fn, os.path.join(crashlog_dir, new_fn)) - try: - os.remove(fn) - except PermissionError: - pass + remove_crashlog(fn) def main() -> NoReturn: diff --git a/tools/lateral_maneuvers/lateral_maneuversd.py b/tools/lateral_maneuvers/lateral_maneuversd.py index b7cca090f..f37594011 100755 --- a/tools/lateral_maneuvers/lateral_maneuversd.py +++ b/tools/lateral_maneuvers/lateral_maneuversd.py @@ -73,7 +73,7 @@ def _save_status(params: Params, status): history = [] status_copy["history"] = [str(line) for line in history if str(line).strip()][-120:] status_copy["updatedAtSec"] = float(status_copy.get("updatedAtSec") or time.monotonic()) - params.put_nonblocking(STATUS_PARAM, json.dumps(status_copy, separators=(",", ":"))) + params.put_nonblocking(STATUS_PARAM, status_copy) return status_copy diff --git a/tools/lateral_maneuvers/test_lateral_maneuversd.py b/tools/lateral_maneuvers/test_lateral_maneuversd.py new file mode 100644 index 000000000..bed0d37c3 --- /dev/null +++ b/tools/lateral_maneuvers/test_lateral_maneuversd.py @@ -0,0 +1,22 @@ +from openpilot.tools.lateral_maneuvers import lateral_maneuversd + + +class FakeParams: + def __init__(self): + self.writes = [] + + def put_nonblocking(self, key, value): + self.writes.append((key, value)) + + +def test_save_status_writes_json_param_as_dict(): + params = FakeParams() + + saved = lateral_maneuversd._save_status(params, { + "state": "setup", + "history": ["", "Armed"], + }) + + assert params.writes == [(lateral_maneuversd.STATUS_PARAM, saved)] + assert isinstance(params.writes[0][1], dict) + assert saved["history"] == ["Armed"]