From 27366d6b4a70a911f1737a03a7b803f4bda5d2ee Mon Sep 17 00:00:00 2001 From: Luke Date: Mon, 12 Oct 2026 16:41:04 -0400 Subject: [PATCH 1/2] fix: bound service logs and protocol trace retention --- config.example.toml | 7 + docs/logging.md | 64 +++++ docs/updating.md | 5 + mkdocs.yml | 1 + roborock_local_server_addon/DOCS.md | 7 + roborock_local_server_addon/config.yaml | 4 + roborock_local_server_beta_addon/config.yaml | 4 + .../https_server/routes/user/scene/service.py | 67 +++-- .../bundled_backend/shared/io_utils.py | 18 +- .../shared/runtime_credentials.py | 44 ++-- .../shared/zone_ranges_store.py | 50 ++-- src/roborock_local_server/config.py | 22 ++ src/roborock_local_server/ha_addon.py | 11 + src/roborock_local_server/log_retention.py | 150 +++++++++++ src/roborock_local_server/server.py | 26 +- tests/test_config.py | 23 ++ tests/test_ha_addon.py | 34 +++ tests/test_log_retention.py | 235 ++++++++++++++++++ 18 files changed, 671 insertions(+), 101 deletions(-) create mode 100644 docs/logging.md create mode 100644 src/roborock_local_server/log_retention.py create mode 100644 tests/test_log_retention.py diff --git a/config.example.toml b/config.example.toml index 29c2a40..0230287 100644 --- a/config.example.toml +++ b/config.example.toml @@ -30,6 +30,13 @@ enable_topic_bridge = true [storage] data_dir = "/data" +[logging] +# Applies to each service .log and HTTP/MQTT .jsonl trace in runtime/. +# MiB per file (1-1024), plus this many numbered backups (0-20). +# 0 backups keeps only the active file; rotation remains enabled. +max_size_mb = 10 +backup_count = 3 + [tls] # Cloudflare DNS-01 wildcard issuance. No public inbound access is required. mode = "cloudflare_acme" diff --git a/docs/logging.md b/docs/logging.md new file mode 100644 index 0000000..60f00dd --- /dev/null +++ b/docs/logging.md @@ -0,0 +1,64 @@ +# Log retention + +The server automatically rotates the six service logs (`api_server.log`, +`iot_server.log`, `wood_server.log`, `https_server.log`, `mqtt_server.log`, and +`unknown_server.log`) and both protocol traces (`decompiled_http.jsonl` and +`decompiled_mqtt.jsonl`) in the data directory's `runtime/` folder. + +The defaults are **10 MiB per file and three backups per file**. The active file +keeps its usual name; `.1` is the newest backup and `.3` is the oldest. Older +backups expire automatically. At these defaults, the eight log families normally +use no more than about **320 MiB combined**. The threshold is per file, not a +quota for the entire data directory. A single larger record is kept intact and +can exceed the threshold; the next record causes rotation. + +Console output remains available through Home Assistant's Logs tab or +`docker logs`. Container console-log retention is managed separately by the +container host. Changing console verbosity does not disable the JSONL files. + +## Home Assistant + +Set these options in the add-on configuration, then restart it: + +```yaml +log_max_size_mb: 10 +log_backup_count: 3 +``` + +`log_max_size_mb` accepts an integer from 1 to 1024, measured in MiB +(1,048,576 bytes). `log_backup_count` accepts an integer from 0 to 20. +Setting the backup count to `0` keeps only the active file; it still rolls over +when full. There is no unlimited-size setting. + +## Docker and standalone installs + +Add this optional section to `config.toml`, then restart: + +```toml +[logging] +max_size_mb = 10 +backup_count = 3 +``` + +The same ranges apply. Existing configurations that omit this section receive +the defaults automatically. + +## Existing large logs and changes to retention + +At startup, the server first recovers missing MQTT credentials and seeds missing +zone-range state from the existing logs, including numbered backups. It then +trims oversized logs to their most recent complete lines and removes backups +beyond the configured count. This also applies when reducing either limit and +to logs that are no longer receiving traffic. The trimming operation reads only +a tail window and replaces each file atomically; it does not load a multi-GB +log into memory or retain it as a multi-GB backup. + +Older diagnostic history is discarded. Copy any logs you need for an ongoing +investigation before upgrading or lowering retention. Persistent credentials, +inventory, zone ranges, device keys, and other state files are not subject to +log rotation. Do not delete the entire `runtime/` directory to clear logs: it +also contains persistent state. + +Rotation coordinates writers within one server process. Run only one server +against a given data directory, and let the server manage these files rather +than running a second external rotation job on them. diff --git a/docs/updating.md b/docs/updating.md index ec12dfc..c67b75f 100644 --- a/docs/updating.md +++ b/docs/updating.md @@ -16,6 +16,11 @@ Use this page when moving an existing install to a newer stable release. ## Updating From A Stable Or RC Release +Builds with automatic [log retention](logging.md) apply a default of 10 MiB per +file with three backups, including to existing installations. On first startup, +oversized logs are trimmed after state recovery; copy any historical logs you +need before upgrading. + If you were already on a stable release such as `0.0.5`, `0.0.6`, `1.0.0`, or a late release candidate, updating should usually not require manual migration. After updating: diff --git a/mkdocs.yml b/mkdocs.yml index 156e830..9bcc256 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -58,6 +58,7 @@ nav: - Mobile App Options: roborock_app.md - Camera (Live Video): camera.md - Reference: + - Log Retention: logging.md - Known Limitations: known_limitations.md - Scene Validity Reports: scene_validity.md - Tested Vacuums: tested_vacuums.md diff --git a/roborock_local_server_addon/DOCS.md b/roborock_local_server_addon/DOCS.md index 5f5cd6f..e48789e 100644 --- a/roborock_local_server_addon/DOCS.md +++ b/roborock_local_server_addon/DOCS.md @@ -45,6 +45,13 @@ For existing cloud integrations, Home Assistant does not support changing the se ## Notes +Service logs and HTTP/MQTT JSONL traces rotate automatically at 10 MiB per file, +with three backups each. Set `log_max_size_mb` (1–1024 MiB) and +`log_backup_count` (0–20), then restart to change retention. Zero backups keeps +only the active file while still limiting growth. Existing oversized logs are +trimmed at startup after state recovery. See [Log retention](../docs/logging.md) +for upgrade behavior and storage estimates. + - Local-only access is still the preferred setup. If you expose it for remote access, the server handles auth and can disable new devices from connecting, but any publicly accessible self-hosted service has risk. - If you change `https_port` or `mqtt_tls_port`, update your DNS/clients to use those ports. - The current server advertises the same hostname for HTTPS and MQTT/TLS, so Home Assistant's Roborock entry should normally use `ssl://api-roborock.example.com:8881`, not a separate `mqtt-...` hostname. diff --git a/roborock_local_server_addon/config.yaml b/roborock_local_server_addon/config.yaml index 572d777..a5fcda8 100644 --- a/roborock_local_server_addon/config.yaml +++ b/roborock_local_server_addon/config.yaml @@ -70,6 +70,8 @@ options: advertised_https_port: 0 advertised_mqtt_tls_port: 0 region: "us" + log_max_size_mb: 10 + log_backup_count: 3 tls_mode: "provided" tls_base_domain: "" tls_email: "" @@ -96,6 +98,8 @@ schema: advertised_https_port: int advertised_mqtt_tls_port: int region: list(us|eu|cn|ru) + log_max_size_mb: int(1,1024)? + log_backup_count: int(0,20)? tls_mode: list(provided|cloudflare_acme) tls_base_domain: str tls_email: str diff --git a/roborock_local_server_beta_addon/config.yaml b/roborock_local_server_beta_addon/config.yaml index 11b3428..5b380a0 100644 --- a/roborock_local_server_beta_addon/config.yaml +++ b/roborock_local_server_beta_addon/config.yaml @@ -29,6 +29,8 @@ options: advertised_https_port: 0 advertised_mqtt_tls_port: 0 region: "us" + log_max_size_mb: 10 + log_backup_count: 3 tls_mode: "provided" tls_base_domain: "" tls_email: "" @@ -48,6 +50,8 @@ schema: advertised_https_port: int advertised_mqtt_tls_port: int region: list(us|eu|cn|ru) + log_max_size_mb: int(1,1024)? + log_backup_count: int(0,20)? tls_mode: list(provided|cloudflare_acme) tls_base_domain: str tls_email: str diff --git a/src/roborock_local_server/bundled_backend/https_server/routes/user/scene/service.py b/src/roborock_local_server/bundled_backend/https_server/routes/user/scene/service.py index a7ce206..66e2abe 100644 --- a/src/roborock_local_server/bundled_backend/https_server/routes/user/scene/service.py +++ b/src/roborock_local_server/bundled_backend/https_server/routes/user/scene/service.py @@ -6,6 +6,8 @@ import logging from typing import Any, Callable, Sequence +from roborock_local_server.log_retention import iter_log_lines + from shared.context import ServerContext from shared.data_helpers import as_bool, as_int, default_home_id, get_value, stable_int from shared.inventory_io import WEB_API_INVENTORY_FILE, inventory_mutation, load_inventory, write_inventory @@ -166,45 +168,36 @@ def _scene_zone_ranges_from_mqtt( *, tids_filter: set[str] | None = None, ) -> dict[tuple[str, int], list[int]]: - path = ctx.mqtt_jsonl - if not path.exists(): - return {} zone_ranges: dict[tuple[str, int], list[int]] = {} - try: - with path.open("r", encoding="utf-8") as handle: - lines = handle.readlines() - for line in reversed(lines): - try: - entry = json.loads(line) - except json.JSONDecodeError: - continue - decoded_messages = entry.get("decoded_messages") - if not isinstance(decoded_messages, list): + # Oldest first so the latest coordinates win, including across rotations. + for line in iter_log_lines(ctx.mqtt_jsonl): + try: + entry = json.loads(line) + except json.JSONDecodeError: + continue + if not isinstance(entry, dict): + continue + decoded_messages = entry.get("decoded_messages") + if not isinstance(decoded_messages, list): + continue + for decoded in decoded_messages: + if not isinstance(decoded, dict): continue - for decoded in decoded_messages: - if not isinstance(decoded, dict): - continue - rpc = decoded.get("rpc") - if isinstance(rpc, dict) and str(rpc.get("method") or "").strip() == "set_scenes_zones": - _merge_scene_zone_ranges_from_request( - zone_ranges, - params=rpc.get("params"), - tids_filter=tids_filter, - ) - response_to = decoded.get("response_to") - if isinstance(response_to, dict) and str(response_to.get("request_method") or "").strip() == "set_scenes_zones": - _merge_scene_zone_ranges_from_response( - zone_ranges, - request_params=response_to.get("request_params"), - result=response_to.get("result"), - tids_filter=tids_filter, - ) - if tids_filter and all( - any(key[0] == tid for key in zone_ranges) for tid in tids_filter - ): - break - except OSError: - return {} + rpc = decoded.get("rpc") + if isinstance(rpc, dict) and str(rpc.get("method") or "").strip() == "set_scenes_zones": + _merge_scene_zone_ranges_from_request( + zone_ranges, + params=rpc.get("params"), + tids_filter=tids_filter, + ) + response_to = decoded.get("response_to") + if isinstance(response_to, dict) and str(response_to.get("request_method") or "").strip() == "set_scenes_zones": + _merge_scene_zone_ranges_from_response( + zone_ranges, + request_params=response_to.get("request_params"), + result=response_to.get("result"), + tids_filter=tids_filter, + ) return zone_ranges diff --git a/src/roborock_local_server/bundled_backend/shared/io_utils.py b/src/roborock_local_server/bundled_backend/shared/io_utils.py index 494a6ed..41284c0 100644 --- a/src/roborock_local_server/bundled_backend/shared/io_utils.py +++ b/src/roborock_local_server/bundled_backend/shared/io_utils.py @@ -7,6 +7,9 @@ from pathlib import Path from typing import Any +from roborock_local_server.config import LoggingConfig +from roborock_local_server.log_retention import BoundedRotatingFileHandler, append_jsonl_text + _FILE_LOG_FORMAT = "%(asctime)s [%(levelname)s] %(message)s" _STREAM_LOG_FORMAT = "%(asctime)s [%(levelname)s] [%(name)s] %(message)s" _JSONL_STREAM_LOGGER_NAME = "real_stack.jsonl" @@ -30,18 +33,21 @@ def _jsonl_stream_logger() -> logging.Logger: def append_jsonl(path: Path, entry: dict[str, Any]) -> None: encoded = json.dumps(entry, ensure_ascii=True, separators=(",", ":")) - path.parent.mkdir(parents=True, exist_ok=True) - with path.open("a", encoding="utf-8") as handle: - handle.write(encoded + "\n") + append_jsonl_text(path, encoded) _jsonl_stream_logger().debug("[%s] %s", path.name, encoded) -def setup_file_logger(name: str, path: Path) -> logging.Logger: +def setup_file_logger( + name: str, path: Path, *, max_bytes: int = LoggingConfig().max_bytes, + backup_count: int = LoggingConfig.backup_count, +) -> logging.Logger: logger = logging.getLogger(f"real_stack.{name}") logger.setLevel(logging.INFO) logger.propagate = False - logger.handlers.clear() - handler = logging.FileHandler(path, encoding="utf-8") + for previous in logger.handlers[:]: + logger.removeHandler(previous) + previous.close() + handler = BoundedRotatingFileHandler(path, max_bytes=max_bytes, backup_count=backup_count) handler.setFormatter(logging.Formatter(_FILE_LOG_FORMAT)) logger.addHandler(handler) logger.addHandler(_make_stream_handler()) diff --git a/src/roborock_local_server/bundled_backend/shared/runtime_credentials.py b/src/roborock_local_server/bundled_backend/shared/runtime_credentials.py index 0977e49..0b41b64 100644 --- a/src/roborock_local_server/bundled_backend/shared/runtime_credentials.py +++ b/src/roborock_local_server/bundled_backend/shared/runtime_credentials.py @@ -11,6 +11,8 @@ from typing import Any from urllib.parse import parse_qs +from roborock_local_server.log_retention import iter_log_lines + from .data_helpers import utcnow_iso @@ -702,8 +704,6 @@ def recovery_pending_devices(self) -> list[dict[str, str]]: def backfill_device_mqtt_passwords(self, log_path: str | Path) -> int: path = Path(log_path) - if not path.exists(): - return 0 changed = 0 with self._lock: pending_usernames = { @@ -715,28 +715,24 @@ def backfill_device_mqtt_passwords(self, log_path: str | Path) -> int: return 0 recovered_by_username: dict[str, str] = {} - try: - with path.open("r", encoding="utf-8", errors="replace") as handle: - for raw_line in handle: - line = str(raw_line or "") - if " CONNECT " not in line or " hex=" not in line: - continue - hex_value = line.rsplit(" hex=", 1)[-1].strip() - if not hex_value: - continue - try: - packet = bytes.fromhex(hex_value) - except ValueError: - continue - parsed = parse_mqtt_connect_packet(packet) - if parsed is None: - continue - username = _clean_str(parsed.get("username")) - password = _clean_str(parsed.get("password")) - if username in pending_usernames and password: - recovered_by_username[username] = password - except OSError: - return 0 + for raw_line in iter_log_lines(path): + line = str(raw_line or "") + if " CONNECT " not in line or " hex=" not in line: + continue + hex_value = line.rsplit(" hex=", 1)[-1].strip() + if not hex_value: + continue + try: + packet = bytes.fromhex(hex_value) + except ValueError: + continue + parsed = parse_mqtt_connect_packet(packet) + if parsed is None: + continue + username = _clean_str(parsed.get("username")) + password = _clean_str(parsed.get("password")) + if username in pending_usernames and password: + recovered_by_username[username] = password with self._lock: for username, password in recovered_by_username.items(): diff --git a/src/roborock_local_server/bundled_backend/shared/zone_ranges_store.py b/src/roborock_local_server/bundled_backend/shared/zone_ranges_store.py index 3f75f8c..6304340 100644 --- a/src/roborock_local_server/bundled_backend/shared/zone_ranges_store.py +++ b/src/roborock_local_server/bundled_backend/shared/zone_ranges_store.py @@ -16,6 +16,8 @@ from pathlib import Path from typing import Any +from roborock_local_server.log_retention import iter_log_lines + _ZONE_RANGES_FILE = "zone_ranges.json" @@ -117,33 +119,29 @@ def seed_from_mqtt_jsonl(self, mqtt_jsonl: Path) -> int: Returns the number of new entries added. """ - if not mqtt_jsonl.exists(): - return 0 count_before = len(self._data) - try: - with mqtt_jsonl.open("r", encoding="utf-8") as handle: - for line in handle: - try: - entry = json.loads(line) - except json.JSONDecodeError: - continue - decoded_messages = entry.get("decoded_messages") - if not isinstance(decoded_messages, list): - continue - for decoded in decoded_messages: - if not isinstance(decoded, dict): - continue - rpc = decoded.get("rpc") - if isinstance(rpc, dict) and str(rpc.get("method") or "").strip() == "set_scenes_zones": - self.merge_set_scenes_zones_request(rpc.get("params")) - response_to = decoded.get("response_to") - if isinstance(response_to, dict) and str(response_to.get("request_method") or "").strip() == "set_scenes_zones": - self.merge_set_scenes_zones_response( - request_params=response_to.get("request_params"), - result=response_to.get("result"), - ) - except OSError: - pass + for line in iter_log_lines(mqtt_jsonl): + try: + entry = json.loads(line) + except json.JSONDecodeError: + continue + if not isinstance(entry, dict): + continue + decoded_messages = entry.get("decoded_messages") + if not isinstance(decoded_messages, list): + continue + for decoded in decoded_messages: + if not isinstance(decoded, dict): + continue + rpc = decoded.get("rpc") + if isinstance(rpc, dict) and str(rpc.get("method") or "").strip() == "set_scenes_zones": + self.merge_set_scenes_zones_request(rpc.get("params")) + response_to = decoded.get("response_to") + if isinstance(response_to, dict) and str(response_to.get("request_method") or "").strip() == "set_scenes_zones": + self.merge_set_scenes_zones_response( + request_params=response_to.get("request_params"), + result=response_to.get("result"), + ) return len(self._data) - count_before def merge_set_scenes_zones_response( diff --git a/src/roborock_local_server/config.py b/src/roborock_local_server/config.py index 37a2cb6..1469631 100644 --- a/src/roborock_local_server/config.py +++ b/src/roborock_local_server/config.py @@ -43,6 +43,22 @@ class StorageConfig: data_dir: str +@dataclass(frozen=True) +class LoggingConfig: + max_size_mb: int = 10 + backup_count: int = 3 + + def __post_init__(self) -> None: + for name, minimum, maximum in (("max_size_mb", 1, 1024), ("backup_count", 0, 20)): + value = getattr(self, name) + if type(value) is not int or not minimum <= value <= maximum: + raise ValueError(f"logging.{name} must be an integer between {minimum} and {maximum}") + + @property + def max_bytes(self) -> int: + return self.max_size_mb * 1024 * 1024 + + @dataclass(frozen=True) class TlsConfig: mode: str @@ -96,6 +112,7 @@ class AppConfig: tls: TlsConfig admin: AdminConfig turn: TurnConfig = TurnConfig() + logging: LoggingConfig = LoggingConfig() @dataclass(frozen=True) @@ -239,6 +256,7 @@ def load_config(path: str | Path) -> AppConfig: network = _get_section(parsed, "network") broker = _get_section(parsed, "broker") storage = _get_section(parsed, "storage") + logging = _get_section(parsed, "logging") tls = _get_section(parsed, "tls") admin = _get_section(parsed, "admin") turn = _get_section(parsed, "turn") @@ -306,6 +324,10 @@ def load_config(path: str | Path) -> AppConfig: storage=StorageConfig( data_dir=str(storage.get("data_dir", "/data")).strip() or "/data", ), + logging=LoggingConfig( + max_size_mb=logging.get("max_size_mb", LoggingConfig.max_size_mb), + backup_count=logging.get("backup_count", LoggingConfig.backup_count), + ), tls=TlsConfig( mode=tls_mode, base_domain=( diff --git a/src/roborock_local_server/ha_addon.py b/src/roborock_local_server/ha_addon.py index 5c92ebc..7d8ccf9 100644 --- a/src/roborock_local_server/ha_addon.py +++ b/src/roborock_local_server/ha_addon.py @@ -13,6 +13,7 @@ from urllib.parse import urlsplit from .configure import hash_password +from .config import LoggingConfig DEFAULT_OPTIONS: dict[str, Any] = { "stack_fqdn": "", @@ -21,6 +22,8 @@ "advertised_https_port": 0, "advertised_mqtt_tls_port": 0, "region": "us", + "log_max_size_mb": LoggingConfig.max_size_mb, + "log_backup_count": LoggingConfig.backup_count, "tls_mode": "provided", "tls_base_domain": "", "tls_email": "", @@ -174,6 +177,10 @@ def _render_config_toml( ) -> tuple[str, dict[Path, str]]: merged = dict(DEFAULT_OPTIONS) merged.update(options) + log_config = LoggingConfig( + max_size_mb=merged["log_max_size_mb"], + backup_count=merged["log_backup_count"], + ) stack_fqdn = _normalize_hostname( merged.get("stack_fqdn", ""), @@ -306,6 +313,10 @@ def _render_config_toml( "[storage]", 'data_dir = "/data"', "", + "[logging]", + f"max_size_mb = {log_config.max_size_mb}", + f"backup_count = {log_config.backup_count}", + "", "[tls]", f"mode = {_toml_string(effective_tls_mode)}", ] diff --git a/src/roborock_local_server/log_retention.py b/src/roborock_local_server/log_retention.py new file mode 100644 index 0000000..2c6fd22 --- /dev/null +++ b/src/roborock_local_server/log_retention.py @@ -0,0 +1,150 @@ +"""Bounded service logs and JSONL traces for a single server process. + +Keep this outside bundled_backend: that package is also imported as ``shared``, +and both import paths must use the same per-file JSONL handlers and locks. +""" + +from __future__ import annotations + +import logging +from logging.handlers import RotatingFileHandler +import os +from pathlib import Path +import tempfile +import threading +from typing import Iterator + +from .config import LoggingConfig + + +def log_paths(path: Path) -> list[Path]: + """Return numbered backups oldest first, then the active file. + + Discover existing backups independently of the current retention setting so + startup recovery can run before a reduced retention setting removes them. + """ + backups = [] + for candidate in path.parent.glob(f"{path.name}.*"): + suffix = candidate.name[len(path.name) + 1:] + if suffix.isascii() and suffix.isdecimal() and int(suffix) > 0 and suffix == str(int(suffix)): + backups.append((int(suffix), candidate)) + return [candidate for _, candidate in sorted(backups, reverse=True)] + [path] + + +def iter_log_lines(path: Path) -> Iterator[str]: + """Stream retained history in chronological order, tolerating missing files.""" + for candidate in log_paths(path): + try: + with candidate.open(encoding="utf-8", errors="replace") as handle: + yield from handle + except OSError: + continue + + +def _trim_to_tail(path: Path, max_bytes: int) -> None: + """Atomically keep recent complete lines without reading the full file.""" + if not path.exists() or path.stat().st_size <= max_bytes: + return + with path.open("rb") as handle: + handle.seek(-max_bytes - 1, os.SEEK_END) + preceding = handle.read(1) + tail = handle.read(max_bytes) + if preceding != b"\n": + # The window starts mid-record (possibly mid UTF-8 character). + _, _, tail = tail.partition(b"\n") + # Never leave a partial JSON record from an interrupted write at the end. + tail = tail[:tail.rfind(b"\n") + 1] + descriptor, temporary = tempfile.mkstemp(prefix=f".{path.name}-", dir=path.parent) + try: + with os.fdopen(descriptor, "wb") as handle: + handle.write(tail) + handle.flush() + os.fsync(handle.fileno()) + os.replace(temporary, path) + finally: + Path(temporary).unlink(missing_ok=True) + + +class BoundedRotatingFileHandler(RotatingFileHandler): + """Rotate by UTF-8 byte size, including when no backups are requested. + + One record may exceed the threshold so a JSON object or traceback is never + split during normal writes. Existing oversized files are trimmed at startup, + after the supervisor has recovered persistent state from the original logs. + """ + + def __init__(self, path: Path, *, max_bytes: int, backup_count: int, keep_open: bool = True) -> None: + if max_bytes <= 0 or backup_count < 0: + raise ValueError("Log max_bytes must be positive and backup_count nonnegative") + path.parent.mkdir(parents=True, exist_ok=True) + super().__init__(path, maxBytes=max_bytes, backupCount=backup_count, + encoding="utf-8", errors="backslashreplace", delay=True) + self.keep_open = keep_open + for candidate in log_paths(path): + if candidate != path and int(candidate.name[len(path.name) + 1:]) > backup_count: + candidate.unlink(missing_ok=True) + else: + _trim_to_tail(candidate, max_bytes) + + def _open(self): + # Stable byte counts on Windows as well as Linux. + return open(self.baseFilename, self.mode, encoding=self.encoding, + errors=self.errors, newline="\n") + + def shouldRollover(self, record: logging.LogRecord) -> bool: + if self.stream is None: + self.stream = self._open() + self.stream.seek(0, os.SEEK_END) + size = self.stream.tell() + encoded_size = len((self.format(record) + self.terminator).encode("utf-8", "backslashreplace")) + return size > 0 and size + encoded_size > self.maxBytes + + def doRollover(self) -> None: + if self.backupCount: + super().doRollover() + else: + if self.stream is not None: + self.stream.close() + self.stream = None + with open(self.baseFilename, "w", encoding="utf-8"): + pass + + def emit(self, record: logging.LogRecord) -> None: + try: + super().emit(record) + finally: + # JSONL used to open per append. Keep that behavior to avoid holding + # handles for arbitrary trace paths; Handler.handle still serializes + # the complete size-check/rotation/write/close sequence. + if not self.keep_open and self.stream is not None: + self.stream.close() + self.stream = None + + +_jsonl_handlers: dict[Path, BoundedRotatingFileHandler] = {} +_jsonl_lock = threading.Lock() + + +def configure_jsonl_logging(path: Path, *, max_bytes: int, backup_count: int) -> None: + """Configure a trace before starting producers or accepting requests.""" + path = path.resolve() + with _jsonl_lock: + previous = _jsonl_handlers.pop(path, None) + if previous is not None: + previous.close() + _jsonl_handlers[path] = BoundedRotatingFileHandler( + path, max_bytes=max_bytes, backup_count=backup_count, keep_open=False, + ) + + +def append_jsonl_text(path: Path, encoded: str) -> None: + path = path.resolve() + with _jsonl_lock: + handler = _jsonl_handlers.get(path) + if handler is None: + defaults = LoggingConfig() + handler = BoundedRotatingFileHandler( + path, max_bytes=defaults.max_bytes, backup_count=defaults.backup_count, keep_open=False, + ) + _jsonl_handlers[path] = handler + handler.handle(logging.LogRecord("", logging.INFO, "", 0, encoded, (), None)) diff --git a/src/roborock_local_server/server.py b/src/roborock_local_server/server.py index 28a5765..c7929ab 100644 --- a/src/roborock_local_server/server.py +++ b/src/roborock_local_server/server.py @@ -30,6 +30,7 @@ from .bundled_backend.shared.runtime_state import ONBOARDING_STEP_LABELS, REQUIRED_ONBOARDING_STEPS from .cloud import CloudImportManager from .config import AppConfig, AppPaths, load_config, resolve_paths +from .log_retention import configure_jsonl_logging from .turn_relay import EmbeddedTurnRelay from .standalone_admin import register_standalone_admin_routes from .backend import ( @@ -377,7 +378,6 @@ def __init__( session_store_path=self.paths.protocol_auth_sessions_path, ) - self.loggers = self._setup_loggers() if not self.paths.device_key_state_path.exists(): self.paths.device_key_state_path.parent.mkdir(parents=True, exist_ok=True) self.paths.device_key_state_path.write_text('{"devices":{}}\n', encoding="utf-8") @@ -444,6 +444,16 @@ def __init__( if recovered_device_passwords: self.root_logger.info("Recovered %d device MQTT password(s) from mqtt_server.log", recovered_device_passwords) + # Recover legacy state before retention trims oversized files or removes + # backups, including when the user lowers the configured limits. + zone_ranges_store = self._init_zone_ranges_store() + self.loggers = self._setup_loggers() + for path in (self.paths.http_jsonl_path, self.paths.mqtt_jsonl_path): + configure_jsonl_logging( + path, max_bytes=self.config.logging.max_bytes, + backup_count=self.config.logging.backup_count, + ) + self.context = ServerContext( api_host=self.config.network.stack_fqdn, mqtt_host=self.config.network.stack_fqdn, @@ -464,7 +474,7 @@ def __init__( bootstrap_encryption_enabled=True, runtime_state=self.runtime_state, runtime_credentials=self.runtime_credentials, - zone_ranges_store=self._init_zone_ranges_store(), + zone_ranges_store=zone_ranges_store, timezone=self.config.network.timezone or None, turn_enabled=self.config.turn.mode != "disabled", turn_host=self.config.turn.host or self.config.network.stack_fqdn, @@ -488,12 +498,12 @@ def _init_zone_ranges_store(self) -> ZoneRangesStore: def _setup_loggers(self) -> dict[str, logging.Logger]: self.paths.runtime_dir.mkdir(parents=True, exist_ok=True) return { - "api": setup_file_logger("api", self.paths.runtime_dir / "api_server.log"), - "iot": setup_file_logger("iot", self.paths.runtime_dir / "iot_server.log"), - "wood": setup_file_logger("wood", self.paths.runtime_dir / "wood_server.log"), - "https": setup_file_logger("https", self.paths.runtime_dir / "https_server.log"), - "mqtt": setup_file_logger("mqtt", self.paths.runtime_dir / "mqtt_server.log"), - "unknown": setup_file_logger("unknown", self.paths.runtime_dir / "unknown_server.log"), + name: setup_file_logger( + name, self.paths.runtime_dir / f"{name}_server.log", + max_bytes=self.config.logging.max_bytes, + backup_count=self.config.logging.backup_count, + ) + for name in ("api", "iot", "wood", "https", "mqtt", "unknown") } def _derive_bootstrap_credentials(self) -> dict[str, str]: diff --git a/tests/test_config.py b/tests/test_config.py index 7db8ee3..9b8988a 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -5,6 +5,29 @@ from conftest import write_release_config +def test_logging_defaults_and_overrides(tmp_path): + path = write_release_config(tmp_path) + defaults = load_config(path).logging + assert defaults.max_bytes == 10 * 1024 * 1024 + assert defaults.backup_count == 3 + path.write_text(path.read_text() + '\n[logging]\nmax_size_mb=2\nbackup_count=0\n') + config = load_config(path) + assert config.logging.max_bytes == 2 * 1024 * 1024 + assert config.logging.backup_count == 0 + + +@pytest.mark.parametrize("field,value", [ + ("max_size_mb", "0"), ("max_size_mb", "-1"), ("max_size_mb", "1025"), + ("max_size_mb", "1.5"), ("max_size_mb", "true"), ("max_size_mb", '"10"'), + ("backup_count", "-1"), ("backup_count", "21"), ("backup_count", "false"), +]) +def test_logging_rejects_invalid_limits(tmp_path, field, value): + path = write_release_config(tmp_path) + path.write_text(path.read_text() + f'\n[logging]\n{field}={value}\n') + with pytest.raises(ValueError, match=f"logging.{field} must be an integer"): + load_config(path) + + def test_turn_config_defaults_and_rejects_unsafe_provided_credentials(tmp_path): path = write_release_config(tmp_path) base = path.read_text() diff --git a/tests/test_ha_addon.py b/tests/test_ha_addon.py index ad8142a..bd0e80c 100644 --- a/tests/test_ha_addon.py +++ b/tests/test_ha_addon.py @@ -8,12 +8,45 @@ import yaml from roborock_local_server.ha_addon import write_config_from_home_assistant_options +from roborock_local_server.config import load_config def _write_options(path: Path, payload: dict[str, object]) -> None: path.write_text(json.dumps(payload), encoding="utf-8") +@pytest.mark.parametrize("addon", ["roborock_local_server_addon", "roborock_local_server_beta_addon"]) +def test_logging_options_round_trip_from_addon_schema(tmp_path, addon): + manifest = yaml.safe_load((Path(addon) / "config.yaml").read_text()) + assert manifest["options"]["log_max_size_mb"] == 10 + assert manifest["options"]["log_backup_count"] == 3 + assert manifest["schema"]["log_max_size_mb"] == "int(1,1024)?" + assert manifest["schema"]["log_backup_count"] == "int(0,20)?" + options = {**manifest["options"], "stack_fqdn": "api-roborock.example.com", + "cert_file": "cert", "key_file": "key", "admin_password": "secret", + "protocol_login_email": "user@example.com", "protocol_login_pin": "123456", + "log_max_size_mb": 2, "log_backup_count": 0} + options_path = tmp_path / "options.json" + config_path = tmp_path / "config.toml" + _write_options(options_path, options) + write_config_from_home_assistant_options(options_path=options_path, config_path=config_path) + config = load_config(config_path) + assert config.logging.max_bytes == 2 * 1024 * 1024 + assert config.logging.backup_count == 0 + + +@pytest.mark.parametrize("options", [{"log_max_size_mb": 0}, {"log_max_size_mb": 1.5}, + {"log_backup_count": -1}, {"log_backup_count": True}]) +def test_addon_rejects_invalid_logging_options_before_writing_config(tmp_path, options): + options_path = tmp_path / "options.json" + config_path = tmp_path / "config.toml" + config_path.write_text("existing configuration") + _write_options(options_path, options) + with pytest.raises(ValueError, match="logging."): + write_config_from_home_assistant_options(options_path=options_path, config_path=config_path) + assert config_path.read_text() == "existing configuration" + + def test_write_config_from_home_assistant_options_provided_tls(tmp_path: Path) -> None: options_path = tmp_path / "options.json" config_path = tmp_path / "config.toml" @@ -51,6 +84,7 @@ def test_write_config_from_home_assistant_options_provided_tls(tmp_path: Path) - assert parsed["broker"]["host"] == "127.0.0.1" assert parsed["broker"]["port"] == 18830 assert parsed["broker"]["enable_topic_bridge"] is True + assert parsed["logging"] == {"max_size_mb": 10, "backup_count": 3} assert parsed["tls"]["mode"] == "provided" assert parsed["tls"]["cert_file"] == "/ssl/fullchain.pem" assert parsed["tls"]["key_file"] == "/ssl/privkey.pem" diff --git a/tests/test_log_retention.py b/tests/test_log_retention.py new file mode 100644 index 0000000..ed1837a --- /dev/null +++ b/tests/test_log_retention.py @@ -0,0 +1,235 @@ +from concurrent.futures import ThreadPoolExecutor +import json +import logging +from pathlib import Path +import tracemalloc + +import pytest + +from roborock_local_server.bundled_backend.shared.io_utils import append_jsonl, setup_file_logger +from roborock_local_server.bundled_backend.shared.zone_ranges_store import ZoneRangesStore +from roborock_local_server.log_retention import ( + BoundedRotatingFileHandler, configure_jsonl_logging, iter_log_lines, log_paths, +) + + +def _record(message: str) -> logging.LogRecord: + return logging.LogRecord("test", logging.INFO, "", 0, message, (), None) + + +def _retained_json(path: Path) -> list[dict]: + return [json.loads(line) for line in iter_log_lines(path)] + + +@pytest.mark.parametrize("backup_count", [0, 2]) +def test_service_logs_rotate_and_expire_old_records(tmp_path, capsys, backup_count): + path = tmp_path / "api_server.log" + logger = setup_file_logger("rotation", path, max_bytes=150, backup_count=backup_count) + try: + for index in range(30): + logger.info("event-%02d %s", index, "x" * 60) + files = [p for p in log_paths(path) if p.exists()] + assert len(files) == backup_count + 1 + assert all(p.stat().st_size <= 150 for p in files) + retained = "".join(iter_log_lines(path)) + assert "event-29" in retained + assert "event-00" not in retained + output = capsys.readouterr().err + assert "event-00" in output and "event-29" in output + finally: + for handler in logger.handlers: + handler.close() + + +def test_utf8_byte_limit_and_whole_oversized_record(tmp_path): + path = tmp_path / "test.log" + handler = BoundedRotatingFileHandler(path, max_bytes=12, backup_count=2) + try: + handler.handle(_record("ééé")) # Seven bytes, including the newline. + handler.handle(_record("ééé")) + assert path.with_name("test.log.1").read_bytes() == "ééé\n".encode() + handler.handle(_record("z" * 40)) + assert path.read_bytes() == b"z" * 40 + b"\n" + handler.handle(_record("new")) + assert path.read_text() == "new\n" + assert path.with_name("test.log.1").read_bytes() == b"z" * 40 + b"\n" + finally: + handler.close() + + +@pytest.mark.parametrize("backup_count", [0, 3]) +def test_jsonl_rotation_preserves_format_and_limits_backups(tmp_path, backup_count): + path = tmp_path / "decompiled_http.jsonl" + configure_jsonl_logging(path, max_bytes=70, backup_count=backup_count) + for index in range(20): + append_jsonl(path, {"index": index, "message": "room\nwith unicode é"}) + records = _retained_json(path) + assert [entry["index"] for entry in records] == list(range(19 - backup_count, 20)) + assert all(p.stat().st_size <= 70 for p in log_paths(path)) + + +def test_concurrent_jsonl_writes_share_lock_across_backend_import_paths(tmp_path): + from roborock_local_server.backend import append_jsonl as backend_append + + path = tmp_path / "decompiled_mqtt.jsonl" + configure_jsonl_logging(path, max_bytes=128, backup_count=20) + + def write(index): + writer = append_jsonl if index % 2 else backend_append + writer(path, {"index": index, "text": "é\n"}) + + with ThreadPoolExecutor(max_workers=8) as executor: + list(executor.map(write, range(60))) + records = _retained_json(path) + assert sorted(entry["index"] for entry in records) == list(range(60)) + assert all(p.stat().st_size <= 128 for p in log_paths(path)) + + +def test_restart_trims_legacy_logs_and_reduced_retention_only(tmp_path): + path = tmp_path / "decompiled_http.jsonl" + old = b'{"old":true}\n' * 100 + recent = b'{"recent":true}\n' + for suffix in ("", ".1", ".2", ".3", ".4"): + path.with_name(path.name + suffix).write_bytes(old + recent + b'{"partial":') + unrelated = tmp_path / "runtime_credentials.json" + unrelated.write_bytes(old) + manual_copy = path.with_name(path.name + ".saved") + manual_copy.write_bytes(old) + + configure_jsonl_logging(path, max_bytes=60, backup_count=2) + + assert not path.with_name(path.name + ".3").exists() + assert not path.with_name(path.name + ".4").exists() + assert unrelated.read_bytes() == old + assert manual_copy.read_bytes() == old + for retained in log_paths(path): + assert retained.stat().st_size <= 60 + assert json.loads(retained.read_text().splitlines()[-1]) == {"recent": True} + assert all(isinstance(json.loads(line), dict) for line in retained.read_text().splitlines()) + append_jsonl(path, {"after_restart": True}) + assert _retained_json(path)[-1] == {"after_restart": True} + + +def test_migration_does_not_load_entire_legacy_file(tmp_path): + path = tmp_path / "large.log" + with path.open("wb") as handle: + handle.seek(16 * 1024 * 1024) + handle.write(b"\nkeep this recent line\n") + tracemalloc.start() + try: + handler = BoundedRotatingFileHandler(path, max_bytes=128, backup_count=1) + _, peak = tracemalloc.get_traced_memory() + handler.close() + finally: + tracemalloc.stop() + assert peak < 1024 * 1024 + assert path.read_bytes() == b"keep this recent line\n" + + +def test_failed_atomic_trim_leaves_original_log_intact(tmp_path, monkeypatch): + from roborock_local_server import log_retention + + path = tmp_path / "test.log" + original = b"old entry\n" * 30 + path.write_bytes(original) + + def fail_replace(source, destination): + raise OSError("replacement failed") + + monkeypatch.setattr(log_retention.os, "replace", fail_replace) + with pytest.raises(OSError, match="replacement failed"): + BoundedRotatingFileHandler(path, max_bytes=100, backup_count=1) + assert path.read_bytes() == original + assert list(tmp_path.iterdir()) == [path] + + +def test_reconfiguring_logger_closes_previous_file_handler(tmp_path): + path = tmp_path / "test.log" + logger = setup_file_logger("replace", path, max_bytes=100, backup_count=1) + logger.info("old") + previous = logger.handlers[0] + replacement = setup_file_logger("replace", path, max_bytes=200, backup_count=2) + try: + assert previous.stream is None + assert len(replacement.handlers) == 2 + replacement.info("new") + assert "new" in path.read_text() + finally: + for handler in replacement.handlers: + handler.close() + + +def zone_entry(coords): + return {"decoded_messages": [{"rpc": {"method": "set_scenes_zones", "params": { + "data": [{"tid": "scene", "zones": [{"zid": 1, "range": coords}]}], + }}}]} + + +def test_zone_recovery_uses_all_backups_oldest_to_newest_without_active_file(tmp_path): + path = tmp_path / "decompiled_mqtt.jsonl" + for suffix, coords in ((10, [1, 2, 3, 4]), (2, [5, 6, 7, 8]), (1, [9, 10, 11, 12])): + path.with_name(f"{path.name}.{suffix}").write_text(json.dumps(zone_entry(coords)) + "\n") + store = ZoneRangesStore(tmp_path) + assert store.seed_from_mqtt_jsonl(path) == 1 + assert store.get("scene", 1) == [9, 10, 11, 12] + assert ZoneRangesStore(tmp_path).get("scene", 1) == [9, 10, 11, 12] + + +def test_scene_fallback_reads_backups_and_latest_coordinates_win(tmp_path): + from types import SimpleNamespace + from roborock_local_server import backend # Initialize vendored imports. + from https_server.routes.user.scene.service import _scene_zone_ranges_from_mqtt + + path = tmp_path / "decompiled_mqtt.jsonl" + path.with_name(path.name + ".1").write_text(json.dumps(zone_entry([1, 2, 3, 4])) + "\n") + ctx = SimpleNamespace(mqtt_jsonl=path) + assert _scene_zone_ranges_from_mqtt(ctx, tids_filter={"scene"}) == {("scene", 1): [1, 2, 3, 4]} + path.write_text(json.dumps(zone_entry([5, 6, 7, 8])) + "\n") + assert _scene_zone_ranges_from_mqtt(ctx, tids_filter={"scene"}) == {("scene", 1): [5, 6, 7, 8]} + + +@pytest.mark.parametrize("source_suffix", ["", ".4"]) +def test_supervisor_recovers_state_before_trimming_or_removing_logs(tmp_path, source_suffix): + from conftest import write_release_config + from roborock_local_server.config import load_config, resolve_paths + from roborock_local_server.server import ReleaseSupervisor + from roborock_local_server.bundled_backend.shared.runtime_credentials import RuntimeCredentialsStore + + config_file = write_release_config(tmp_path) + config_file.write_text(config_file.read_text() + '\n[logging]\nmax_size_mb=1\nbackup_count=0\n') + config = load_config(config_file) + paths = resolve_paths(config_file, config) + paths.runtime_dir.mkdir(parents=True) + credentials = RuntimeCredentialsStore(paths.runtime_credentials_path) + credentials.ensure_device(did="123", duid="vacuum", device_mqtt_usr="c25b14ceac358d2a", assign_localkey=False) + connect = "CONNECT len=82 hex=105000044d51545404c2001e00106130313233393163623566386263393700106332356231346365616333353864326100206666383932326432346139613961663831663138663335646365653961356135" + mqtt_log = paths.runtime_dir / ("mqtt_server.log" + source_suffix) + mqtt_log.write_text("[INFO] " + connect + "\n" + "later event\n" * 100_000) + mqtt_jsonl = paths.runtime_dir / ("decompiled_mqtt.jsonl" + source_suffix) + mqtt_jsonl.write_text(json.dumps(zone_entry([1, 2, 3, 4])) + "\n" + '{"later":true}\n' * 100_000) + # Also exercise cleanup for an inactive service, which may not emit on startup. + (paths.runtime_dir / "unknown_server.log").write_text("old\n" * 300_000) + + supervisor = ReleaseSupervisor(config=config, paths=paths) + try: + persisted = RuntimeCredentialsStore(paths.runtime_credentials_path) + assert persisted.resolve_device(duid="vacuum")["device_mqtt_pass"] == "ff8922d24a9a9af81f18f35dcee9a5a5" + assert ZoneRangesStore(paths.runtime_dir).get("scene", 1) == [1, 2, 3, 4] + assert supervisor.context.zone_ranges_store.get("scene", 1) == [1, 2, 3, 4] + for path in (mqtt_log, mqtt_jsonl, paths.runtime_dir / "unknown_server.log"): + assert not path.exists() or path.stat().st_size <= config.logging.max_bytes + if source_suffix: + assert not mqtt_log.exists() and not mqtt_jsonl.exists() + else: + assert "CONNECT" not in mqtt_log.read_text() + assert "set_scenes_zones" not in mqtt_jsonl.read_text() + # The supervisor must configure both aliases used by the vendored backend. + from roborock_local_server.backend import append_jsonl as backend_append + for index in range(3): + backend_append(paths.http_jsonl_path, {"index": index, "payload": "x" * 600_000}) + assert len(_retained_json(paths.http_jsonl_path)) == 1 + assert not paths.http_jsonl_path.with_name(paths.http_jsonl_path.name + ".1").exists() + finally: + for logger in supervisor.loggers.values(): + for handler in logger.handlers: + handler.close() From 47c1e3cc278fba455b1f44ea4fe13b01c5d4fb9b Mon Sep 17 00:00:00 2001 From: Luke Date: Mon, 12 Oct 2026 19:57:22 -0400 Subject: [PATCH 2/2] refactor(logging): clean up retention helpers, defaults, and test imports --- .../bundled_backend/shared/io_utils.py | 21 +++++++-- src/roborock_local_server/log_retention.py | 37 +++++++++------ tests/test_config.py | 3 +- tests/test_ha_addon.py | 4 +- tests/test_log_retention.py | 47 +++++++++++-------- 5 files changed, 72 insertions(+), 40 deletions(-) diff --git a/src/roborock_local_server/bundled_backend/shared/io_utils.py b/src/roborock_local_server/bundled_backend/shared/io_utils.py index 41284c0..856f899 100644 --- a/src/roborock_local_server/bundled_backend/shared/io_utils.py +++ b/src/roborock_local_server/bundled_backend/shared/io_utils.py @@ -8,7 +8,10 @@ from typing import Any from roborock_local_server.config import LoggingConfig -from roborock_local_server.log_retention import BoundedRotatingFileHandler, append_jsonl_text +from roborock_local_server.log_retention import ( + BoundedRotatingFileHandler, + append_jsonl_text, +) _FILE_LOG_FORMAT = "%(asctime)s [%(levelname)s] %(message)s" _STREAM_LOG_FORMAT = "%(asctime)s [%(levelname)s] [%(name)s] %(message)s" @@ -38,16 +41,26 @@ def append_jsonl(path: Path, entry: dict[str, Any]) -> None: def setup_file_logger( - name: str, path: Path, *, max_bytes: int = LoggingConfig().max_bytes, - backup_count: int = LoggingConfig.backup_count, + name: str, + path: Path, + *, + max_bytes: int | None = None, + backup_count: int | None = None, ) -> logging.Logger: + defaults = LoggingConfig() + effective_max_bytes = defaults.max_bytes if max_bytes is None else max_bytes + effective_backup_count = defaults.backup_count if backup_count is None else backup_count logger = logging.getLogger(f"real_stack.{name}") logger.setLevel(logging.INFO) logger.propagate = False for previous in logger.handlers[:]: logger.removeHandler(previous) previous.close() - handler = BoundedRotatingFileHandler(path, max_bytes=max_bytes, backup_count=backup_count) + handler = BoundedRotatingFileHandler( + path, + max_bytes=effective_max_bytes, + backup_count=effective_backup_count, + ) handler.setFormatter(logging.Formatter(_FILE_LOG_FORMAT)) logger.addHandler(handler) logger.addHandler(_make_stream_handler()) diff --git a/src/roborock_local_server/log_retention.py b/src/roborock_local_server/log_retention.py index 2c6fd22..d660a5e 100644 --- a/src/roborock_local_server/log_retention.py +++ b/src/roborock_local_server/log_retention.py @@ -7,28 +7,33 @@ from __future__ import annotations import logging -from logging.handlers import RotatingFileHandler import os -from pathlib import Path +import re import tempfile import threading -from typing import Iterator +from collections.abc import Iterator +from logging.handlers import RotatingFileHandler +from pathlib import Path from .config import LoggingConfig +def _numbered_backups(path: Path) -> list[tuple[int, Path]]: + backups: list[tuple[int, Path]] = [] + for candidate in path.parent.glob(f"{path.name}.*"): + suffix = candidate.name[len(path.name) + 1:] + if re.fullmatch(r"[1-9]\d*", suffix): + backups.append((int(suffix), candidate)) + return sorted(backups, reverse=True) + + def log_paths(path: Path) -> list[Path]: """Return numbered backups oldest first, then the active file. Discover existing backups independently of the current retention setting so startup recovery can run before a reduced retention setting removes them. """ - backups = [] - for candidate in path.parent.glob(f"{path.name}.*"): - suffix = candidate.name[len(path.name) + 1:] - if suffix.isascii() and suffix.isdecimal() and int(suffix) > 0 and suffix == str(int(suffix)): - backups.append((int(suffix), candidate)) - return [candidate for _, candidate in sorted(backups, reverse=True)] + [path] + return [candidate for _, candidate in _numbered_backups(path)] + [path] def iter_log_lines(path: Path) -> Iterator[str]: @@ -47,8 +52,8 @@ def _trim_to_tail(path: Path, max_bytes: int) -> None: return with path.open("rb") as handle: handle.seek(-max_bytes - 1, os.SEEK_END) - preceding = handle.read(1) - tail = handle.read(max_bytes) + chunk = handle.read(max_bytes + 1) + preceding, tail = chunk[:1], chunk[1:] if preceding != b"\n": # The window starts mid-record (possibly mid UTF-8 character). _, _, tail = tail.partition(b"\n") @@ -80,11 +85,12 @@ def __init__(self, path: Path, *, max_bytes: int, backup_count: int, keep_open: super().__init__(path, maxBytes=max_bytes, backupCount=backup_count, encoding="utf-8", errors="backslashreplace", delay=True) self.keep_open = keep_open - for candidate in log_paths(path): - if candidate != path and int(candidate.name[len(path.name) + 1:]) > backup_count: + for suffix_index, candidate in _numbered_backups(path): + if suffix_index > backup_count: candidate.unlink(missing_ok=True) else: _trim_to_tail(candidate, max_bytes) + _trim_to_tail(path, max_bytes) def _open(self): # Stable byte counts on Windows as well as Linux. @@ -132,9 +138,11 @@ def configure_jsonl_logging(path: Path, *, max_bytes: int, backup_count: int) -> previous = _jsonl_handlers.pop(path, None) if previous is not None: previous.close() - _jsonl_handlers[path] = BoundedRotatingFileHandler( + handler = BoundedRotatingFileHandler( path, max_bytes=max_bytes, backup_count=backup_count, keep_open=False, ) + handler.setFormatter(logging.Formatter("%(message)s")) + _jsonl_handlers[path] = handler def append_jsonl_text(path: Path, encoded: str) -> None: @@ -146,5 +154,6 @@ def append_jsonl_text(path: Path, encoded: str) -> None: handler = BoundedRotatingFileHandler( path, max_bytes=defaults.max_bytes, backup_count=defaults.backup_count, keep_open=False, ) + handler.setFormatter(logging.Formatter("%(message)s")) _jsonl_handlers[path] = handler handler.handle(logging.LogRecord("", logging.INFO, "", 0, encoded, (), None)) diff --git a/tests/test_config.py b/tests/test_config.py index 9b8988a..ba56958 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -1,8 +1,9 @@ from pathlib import Path + import pytest +from conftest import write_release_config from roborock_local_server.config import load_config, resolve_paths -from conftest import write_release_config def test_logging_defaults_and_overrides(tmp_path): diff --git a/tests/test_ha_addon.py b/tests/test_ha_addon.py index bd0e80c..8cdbdd4 100644 --- a/tests/test_ha_addon.py +++ b/tests/test_ha_addon.py @@ -1,14 +1,14 @@ from __future__ import annotations import json -from pathlib import Path import tomllib +from pathlib import Path import pytest import yaml -from roborock_local_server.ha_addon import write_config_from_home_assistant_options from roborock_local_server.config import load_config +from roborock_local_server.ha_addon import write_config_from_home_assistant_options def _write_options(path: Path, payload: dict[str, object]) -> None: diff --git a/tests/test_log_retention.py b/tests/test_log_retention.py index ed1837a..6699e8a 100644 --- a/tests/test_log_retention.py +++ b/tests/test_log_retention.py @@ -1,16 +1,39 @@ -from concurrent.futures import ThreadPoolExecutor +from __future__ import annotations + import json import logging -from pathlib import Path import tracemalloc +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from types import SimpleNamespace import pytest +from conftest import write_release_config -from roborock_local_server.bundled_backend.shared.io_utils import append_jsonl, setup_file_logger -from roborock_local_server.bundled_backend.shared.zone_ranges_store import ZoneRangesStore +from roborock_local_server import backend as _backend # noqa: F401 +from roborock_local_server import log_retention +from roborock_local_server.backend import append_jsonl as backend_append +from roborock_local_server.bundled_backend.https_server.routes.user.scene.service import ( + _scene_zone_ranges_from_mqtt, +) +from roborock_local_server.bundled_backend.shared.io_utils import ( + append_jsonl, + setup_file_logger, +) +from roborock_local_server.bundled_backend.shared.runtime_credentials import ( + RuntimeCredentialsStore, +) +from roborock_local_server.bundled_backend.shared.zone_ranges_store import ( + ZoneRangesStore, +) +from roborock_local_server.config import load_config, resolve_paths from roborock_local_server.log_retention import ( - BoundedRotatingFileHandler, configure_jsonl_logging, iter_log_lines, log_paths, + BoundedRotatingFileHandler, + configure_jsonl_logging, + iter_log_lines, + log_paths, ) +from roborock_local_server.server import ReleaseSupervisor def _record(message: str) -> logging.LogRecord: @@ -69,8 +92,6 @@ def test_jsonl_rotation_preserves_format_and_limits_backups(tmp_path, backup_cou def test_concurrent_jsonl_writes_share_lock_across_backend_import_paths(tmp_path): - from roborock_local_server.backend import append_jsonl as backend_append - path = tmp_path / "decompiled_mqtt.jsonl" configure_jsonl_logging(path, max_bytes=128, backup_count=20) @@ -127,8 +148,6 @@ def test_migration_does_not_load_entire_legacy_file(tmp_path): def test_failed_atomic_trim_leaves_original_log_intact(tmp_path, monkeypatch): - from roborock_local_server import log_retention - path = tmp_path / "test.log" original = b"old entry\n" * 30 path.write_bytes(original) @@ -176,10 +195,6 @@ def test_zone_recovery_uses_all_backups_oldest_to_newest_without_active_file(tmp def test_scene_fallback_reads_backups_and_latest_coordinates_win(tmp_path): - from types import SimpleNamespace - from roborock_local_server import backend # Initialize vendored imports. - from https_server.routes.user.scene.service import _scene_zone_ranges_from_mqtt - path = tmp_path / "decompiled_mqtt.jsonl" path.with_name(path.name + ".1").write_text(json.dumps(zone_entry([1, 2, 3, 4])) + "\n") ctx = SimpleNamespace(mqtt_jsonl=path) @@ -190,11 +205,6 @@ def test_scene_fallback_reads_backups_and_latest_coordinates_win(tmp_path): @pytest.mark.parametrize("source_suffix", ["", ".4"]) def test_supervisor_recovers_state_before_trimming_or_removing_logs(tmp_path, source_suffix): - from conftest import write_release_config - from roborock_local_server.config import load_config, resolve_paths - from roborock_local_server.server import ReleaseSupervisor - from roborock_local_server.bundled_backend.shared.runtime_credentials import RuntimeCredentialsStore - config_file = write_release_config(tmp_path) config_file.write_text(config_file.read_text() + '\n[logging]\nmax_size_mb=1\nbackup_count=0\n') config = load_config(config_file) @@ -224,7 +234,6 @@ def test_supervisor_recovers_state_before_trimming_or_removing_logs(tmp_path, so assert "CONNECT" not in mqtt_log.read_text() assert "set_scenes_zones" not in mqtt_jsonl.read_text() # The supervisor must configure both aliases used by the vendored backend. - from roborock_local_server.backend import append_jsonl as backend_append for index in range(3): backend_append(paths.http_jsonl_path, {"index": index, "payload": "x" * 600_000}) assert len(_retained_json(paths.http_jsonl_path)) == 1