diff --git a/CHANGELOG.md b/CHANGELOG.md index fab678d..5f49c93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,25 @@ allowed to change behaviour, not just add to it. ## [Unreleased] +### Added + +- One interactive MeshTerm per data directory. A second one is turned away with the way + out in the message, rather than quietly overwriting the first one's contacts and + settings. The lock is held by the operating system, so a crash releases it — a process + id in a file would leave a lock nobody could explain. +- A readable answer when there is no Windows console. Git Bash, MSYS and Cygwin are not + Windows consoles, and prompt_toolkit's own error says so in the language of its + internals, arriving as a traceback that reads like a broken app rather than the + instruction it is. + +### Fixed + +- All ten stores write through one helper, and the file each writes to first is named + for the process writing it. They shared a fixed `.tmp` name, so two copies of MeshTerm + saving at the same moment took turns inside one scratch file and then each renamed + whatever was in it over the real one. The rename was always atomic; what was being + renamed was not. + ## [0.2.2] — 2026-09-07 ### Fixed diff --git a/meshterm/cli.py b/meshterm/cli.py index bed0e65..b1acee1 100644 --- a/meshterm/cli.py +++ b/meshterm/cli.py @@ -13,6 +13,7 @@ import logging from pathlib import Path import typer +from rich.console import Console from .context import AppContext from .core.admin_store import AdminStore @@ -20,6 +21,7 @@ from .core.config import Settings from .core.connection import DeviceCommandError, is_connection_lost from .core.device_config import DeviceConfigError from .core.device_store import DeviceStore +from .core.instancelock import InstanceBusy, hold_instance_lock from .core.preferences import PreferenceError from .core.selection import DeviceSelectionError from .persistence.logging import configure_logging @@ -166,7 +168,45 @@ def main_callback( if ctx.invoked_subcommand is None: from .ui.menu import run_menu - asyncio.run(_drive(run_menu(app_ctx), app_ctx)) + # The interactive session is the one that holds the stores in memory and rewrites + # them whole, so it is the one that claims the directory. One-shot subcommands are + # brief and mostly read; blocking `meshterm contacts` because a menu is open in + # another window would be an obstacle rather than a guard. + try: + with hold_instance_lock(settings.config_dir): + asyncio.run(_drive(run_menu(app_ctx), app_ctx)) + except InstanceBusy as exc: + console.print(f"[err]✗[/err] {exc}") + raise typer.Exit(code=1) from None + except Exception as exc: + if type(exc).__name__ != "NoConsoleScreenBufferError": + raise + # prompt_toolkit needs a real Win32 console and reports its absence in the + # language of its own internals. A traceback reads as "this app is broken" + # when the answer is "open a different terminal", so answer that instead. + _report_no_windows_console(console) + raise typer.Exit(code=1) from None + + +def _report_no_windows_console(console: Console) -> None: + """Explain a console MeshTerm cannot draw in, and how to get one it can. + + Reached when prompt_toolkit finds no Win32 console screen buffer: a Git Bash, MSYS or + Cygwin shell — which announce themselves as ``xterm`` and are not Windows consoles at + all — or a process whose output has been redirected away from one. + """ + console.print("[err]✗[/err] MeshTerm needs a Windows console, and could not find one.") + console.print() + console.print("That usually means one of two things:") + console.print() + console.print(" • You are in Git Bash, MSYS or Cygwin. Those are not Windows consoles.") + console.print(" Open [accent]Windows Terminal[/accent] or [accent]PowerShell[/accent]") + console.print(" and run it from there.") + console.print(" • The output is piped or redirected somewhere. The full-screen menu") + console.print(" needs the terminal itself.") + console.print() + console.print("Every screen also has a subcommand, and those pipe fine —") + console.print("try [accent]meshterm --help[/accent].") @app.command(name="platform") diff --git a/meshterm/core/admin_store.py b/meshterm/core/admin_store.py index 5a54287..0df4286 100644 --- a/meshterm/core/admin_store.py +++ b/meshterm/core/admin_store.py @@ -14,11 +14,10 @@ a multi-user secret store; treat the file accordingly. from __future__ import annotations import json -import os -import stat from dataclasses import dataclass from pathlib import Path +from .atomicwrite import write_atomically from .models import Contact, LoginResult, utcnow @@ -139,11 +138,4 @@ class AdminStore: def _write(self, records: dict[str, dict]) -> None: """Persist ``records`` atomically with owner-only permissions where supported.""" - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(records, indent=2), encoding="utf-8") - try: # best-effort: not all platforms/filesystems honor chmod - os.chmod(tmp, stat.S_IRUSR | stat.S_IWUSR) - except OSError: # pragma: no cover - platform-dependent - pass - tmp.replace(self._path) + write_atomically(self._path, json.dumps(records, indent=2), owner_only=True) diff --git a/meshterm/core/advert_store.py b/meshterm/core/advert_store.py index 304187f..0f90f37 100644 --- a/meshterm/core/advert_store.py +++ b/meshterm/core/advert_store.py @@ -20,6 +20,7 @@ from dataclasses import dataclass from datetime import datetime from pathlib import Path +from .atomicwrite import write_atomically from .models import utcnow #: The direct (zero-hop) cadence choices offered in the editor, in hours. @@ -190,10 +191,7 @@ class AdvertStore: def _write(self, records: dict[str, dict]) -> None: """Persist ``records`` atomically (crash mid-write keeps the previous file).""" - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(records, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(records, indent=2)) def _as_hours(value: object, default: int) -> int: diff --git a/meshterm/core/atomicwrite.py b/meshterm/core/atomicwrite.py new file mode 100644 index 0000000..4b6b03d --- /dev/null +++ b/meshterm/core/atomicwrite.py @@ -0,0 +1,54 @@ +"""THE way MeshTerm replaces a file it owns: all of the new contents, or none of them. + +Every store under ``core/`` keeps its state in one file that it rewrites whole. Writing +in place means a crash — or a laptop lid closing — halfway through leaves a truncated +file where a contact list used to be, so each write goes to a neighbouring temporary file +first and lands with a single rename, which the filesystem either does or doesn't. + +The neighbouring file's name carries the writing process's id. That is not fussiness: +MeshTerm's data directory is shared by every copy on the machine — a checkout, a +downloaded build, a second instance for a second radio — and a fixed ``.tmp`` name means +two of them writing at the same moment take turns clobbering one scratch file, then each +rename whatever is in it over the real one. The rename is atomic; the thing being renamed +was not. Per-process names make the two writes independent again, and the last rename +simply wins, which is a lost update rather than a corrupted file. + +``MESHTERM_HOME`` is the way to avoid sharing the directory in the first place — see +:func:`meshterm.core.config.default_config_dir`. +""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path + +__all__ = ["write_atomically"] + + +def write_atomically(path: Path, text: str, *, owner_only: bool = False) -> None: + """Replace ``path`` with ``text`` in one step, creating parent directories as needed. + + Args: + path: The file to replace. Its directory is created if it does not exist. + text: The complete new contents, written as UTF-8. + owner_only: Restrict the file to the owner (``0600``) before it is put in place, + for anything holding a credential. Applied to the temporary file rather than + the destination, so there is no moment where the real file exists with wider + permissions. Best-effort: not every platform or filesystem honours it. + """ + path.parent.mkdir(parents=True, exist_ok=True) + tmp = path.with_suffix(f"{path.suffix}.{os.getpid()}.tmp") + try: + tmp.write_text(text, encoding="utf-8") + if owner_only: + try: + os.chmod(tmp, stat.S_IRUSR | stat.S_IWUSR) + except OSError: # pragma: no cover - platform-dependent + pass + tmp.replace(path) + except BaseException: + # A failed write leaves no litter behind. `missing_ok` because the failure may + # well have been the write that would have created it. + tmp.unlink(missing_ok=True) + raise diff --git a/meshterm/core/channel_store.py b/meshterm/core/channel_store.py index 56332e9..5056390 100644 --- a/meshterm/core/channel_store.py +++ b/meshterm/core/channel_store.py @@ -30,6 +30,7 @@ from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING +from .atomicwrite import write_atomically from .channels import ( CHANNEL_SECRET_BYTES, CHANNEL_SLOT_EMPTY_RUN, @@ -156,10 +157,7 @@ class ChannelStore: if channels } } - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(data, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(data, indent=2)) def _channel_from_json(entry: object) -> RememberedChannel | None: diff --git a/meshterm/core/contact_store.py b/meshterm/core/contact_store.py index dd44f12..54693de 100644 --- a/meshterm/core/contact_store.py +++ b/meshterm/core/contact_store.py @@ -49,6 +49,7 @@ import json from dataclasses import dataclass from pathlib import Path +from .atomicwrite import write_atomically from .models import Contact, advert_time @@ -332,10 +333,7 @@ class ContactStore: if contacts } } - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(data, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(data, indent=2)) def _contact_to_json(contact: RememberedContact) -> dict: diff --git a/meshterm/core/courier_store.py b/meshterm/core/courier_store.py index 1151ecf..31fbea5 100644 --- a/meshterm/core/courier_store.py +++ b/meshterm/core/courier_store.py @@ -19,6 +19,7 @@ from dataclasses import dataclass from datetime import datetime from pathlib import Path +from .atomicwrite import write_atomically from .models import utcnow #: Message states: waiting in the outbox, landed, or abandoned after the retry budget. @@ -253,10 +254,7 @@ class CourierStore: for m in self._load() ], } - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(data, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(data, indent=2)) def _as_int(value: object, default: int) -> int: diff --git a/meshterm/core/device_store.py b/meshterm/core/device_store.py index 183d895..59a7266 100644 --- a/meshterm/core/device_store.py +++ b/meshterm/core/device_store.py @@ -19,6 +19,7 @@ import json from dataclasses import dataclass from pathlib import Path +from .atomicwrite import write_atomically from .discovery import ( TRANSPORT_BLE, TRANSPORT_SERIAL, @@ -259,7 +260,4 @@ class DeviceStore: }, "last": last, } - # Atomic-ish replace so a crash mid-write can't truncate the existing registry. - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(payload, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(payload, indent=2)) diff --git a/meshterm/core/instancelock.py b/meshterm/core/instancelock.py new file mode 100644 index 0000000..73388b7 --- /dev/null +++ b/meshterm/core/instancelock.py @@ -0,0 +1,94 @@ +"""One interactive MeshTerm per data directory, and a clear answer when you want two. + +Every copy of MeshTerm on a machine reads and writes the same ``~/.meshterm`` — a +checkout, a downloaded build, a second instance for a second radio. Nothing about how it +was installed changes that. Two running at once will not corrupt anything (the database +is in WAL mode and every file lands by rename) but each holds the JSON stores in memory +and rewrites them whole, so the second one to save quietly discards the first one's +changes. A lost contact is much harder to notice than a crash. + +So the interactive session takes an exclusive lock on the directory it is using, and a +second one is turned away with the fix in the message: set ``MESHTERM_HOME``. + +The lock is held by the operating system rather than written down as a process id in a +file. A pid file has to answer "is that process still alive?", the answer is different on +every platform, and it is wrong after a reboot recycles the number — so a crash leaves a +lock nobody can explain and everybody works around. An OS lock is released when the +process ends, however it ends. + +One-shot CLI subcommands do not take it. They are brief, mostly read, and blocking +``meshterm contacts`` because a menu is open elsewhere would be an obstacle rather than a +guard. +""" + +from __future__ import annotations + +import sys +from collections.abc import Iterator +from contextlib import contextmanager +from pathlib import Path + +__all__ = ["InstanceBusy", "LOCK_NAME", "hold_instance_lock"] + +#: The lock file, kept beside the data it guards. Empty; only the lock on it matters. +LOCK_NAME = ".lock" + + +class InstanceBusy(RuntimeError): + """Raised when another interactive MeshTerm already holds this data directory.""" + + def __init__(self, config_dir: Path) -> None: + """Build the message, which is mostly the way out of the situation.""" + super().__init__( + f"Another MeshTerm is already using {config_dir}.\n\n" + "Two of them sharing one directory will quietly overwrite each other's\n" + "contacts and settings, so this one stopped instead.\n\n" + "To run a second one — a downloaded build beside your own, or a second\n" + "radio — give it a directory of its own:\n\n" + " MESHTERM_HOME=~/meshterm-other meshterm\n" + ) + self.config_dir = config_dir + + +def _try_lock(handle) -> bool: + """Take an exclusive, non-blocking lock on an open file; ``False`` if someone has it.""" + if sys.platform == "win32": + import msvcrt + + try: + msvcrt.locking(handle.fileno(), msvcrt.LK_NBLCK, 1) + except OSError: + return False + return True + + import fcntl + + try: + fcntl.flock(handle.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + except OSError: + return False + return True + + +@contextmanager +def hold_instance_lock(config_dir: Path) -> Iterator[None]: + """Hold ``config_dir`` for this process, releasing it however the process ends. + + Args: + config_dir: The data directory to claim. + + Raises: + InstanceBusy: Another interactive MeshTerm holds it. + """ + config_dir.mkdir(parents=True, exist_ok=True) + path = config_dir / LOCK_NAME + handle = open(path, "a+b") # noqa: SIM115 - closed in the finally below + try: + if not _try_lock(handle): + raise InstanceBusy(config_dir) + yield + finally: + # Closing releases the lock on every platform. The file itself stays: deleting it + # would race a second process that has already opened it and is about to lock, + # which is how one directory ends up handed to two of them. + handle.close() diff --git a/meshterm/core/mute_store.py b/meshterm/core/mute_store.py index 103fc62..4c63900 100644 --- a/meshterm/core/mute_store.py +++ b/meshterm/core/mute_store.py @@ -23,6 +23,8 @@ from __future__ import annotations import json from pathlib import Path +from .atomicwrite import write_atomically + class MuteStore: """Reads and writes the set of channels whose notifications are muted, memory-first. @@ -92,7 +94,4 @@ class MuteStore: def _save(self) -> None: """Persist the whole muted set atomically (a crash mid-write keeps the old file).""" data = {"muted": sorted(self._state)} - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(data, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(data, indent=2)) diff --git a/meshterm/core/preferences.py b/meshterm/core/preferences.py index 2cfac6b..817fe65 100644 --- a/meshterm/core/preferences.py +++ b/meshterm/core/preferences.py @@ -42,6 +42,7 @@ from dataclasses import dataclass from pathlib import Path from typing import Any +from .atomicwrite import write_atomically from .watch_store import DEFAULT_SILENCE_HOURS, OFF, SILENCE_CHOICES_H #: Display groups, in the order the page and the file present them. The order is the @@ -602,10 +603,7 @@ class Preferences: """ if self._path is None: raise RuntimeError("these preferences have no file to save to") - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(self.as_yaml(), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, self.as_yaml()) #: The set installed for this process. Starts as a defaults-only, file-less instance so diff --git a/meshterm/core/remote_store.py b/meshterm/core/remote_store.py index cf48ab8..d4a7658 100644 --- a/meshterm/core/remote_store.py +++ b/meshterm/core/remote_store.py @@ -17,6 +17,7 @@ from datetime import datetime from pathlib import Path from .admin_store import admin_key +from .atomicwrite import write_atomically from .models import Contact, utcnow #: How many command-line entries are kept per node (newest last). @@ -109,7 +110,4 @@ class RemoteStore: def _write(self, records: dict[str, dict]) -> None: """Persist ``records`` atomically (crash mid-write keeps the previous file).""" - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(records, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(records, indent=2)) diff --git a/meshterm/core/settings_store.py b/meshterm/core/settings_store.py index 0f15d64..f943d62 100644 --- a/meshterm/core/settings_store.py +++ b/meshterm/core/settings_store.py @@ -32,6 +32,7 @@ from dataclasses import dataclass from pathlib import Path from typing import TYPE_CHECKING, Any +from .atomicwrite import write_atomically from .device_config import DeviceConfigError, format_value, get_spec, parse_value if TYPE_CHECKING: @@ -147,10 +148,7 @@ class SettingsStore: if values } } - self._path.parent.mkdir(parents=True, exist_ok=True) - tmp = self._path.with_suffix(self._path.suffix + ".tmp") - tmp.write_text(json.dumps(data, indent=2), encoding="utf-8") - tmp.replace(self._path) + write_atomically(self._path, json.dumps(data, indent=2)) @dataclass(frozen=True) diff --git a/tests/test_instancelock.py b/tests/test_instancelock.py new file mode 100644 index 0000000..068f840 --- /dev/null +++ b/tests/test_instancelock.py @@ -0,0 +1,110 @@ +"""Tests for the one-interactive-session-per-directory guard, and the atomic writer. + +Both exist for the same reason: every copy of MeshTerm on a machine shares one data +directory unless told otherwise, so two of them running at once is a thing that happens +by accident rather than by choice. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path + +import pytest + +from meshterm.core.atomicwrite import write_atomically +from meshterm.core.instancelock import LOCK_NAME, InstanceBusy, hold_instance_lock + +REPO_ROOT = Path(__file__).resolve().parent.parent + + +def test_the_lock_is_released_when_the_holder_leaves(tmp_path: Path) -> None: + """Two sessions in sequence are fine; the guard is against overlap, not against reuse.""" + with hold_instance_lock(tmp_path): + pass + with hold_instance_lock(tmp_path): + pass + + +def test_the_lock_file_lives_beside_the_data(tmp_path: Path) -> None: + """The lock sits in the directory it guards, and the directory is made if missing.""" + target = tmp_path / "not-yet" + with hold_instance_lock(target): + assert (target / LOCK_NAME).exists() + + +def test_a_second_process_is_turned_away(tmp_path: Path) -> None: + """The real test of an OS lock: another *process* cannot take it. + + Nothing in-process proves anything here — a lock that only excluded the thread that + already holds it would pass a single-process test and fail in the one situation it + exists for. + """ + program = ( + "import pathlib, sys\n" + f"sys.path.insert(0, {str(REPO_ROOT)!r})\n" + "from meshterm.core.instancelock import hold_instance_lock, InstanceBusy\n" + "try:\n" + f" with hold_instance_lock(pathlib.Path({str(tmp_path)!r})):\n" + " print('TAKEN')\n" + "except InstanceBusy:\n" + " print('REFUSED')\n" + ) + with hold_instance_lock(tmp_path): + done = subprocess.run( + [sys.executable, "-c", program], capture_output=True, text=True, timeout=60 + ) + assert done.stdout.strip() == "REFUSED", done.stderr[-500:] + + +def test_the_refusal_says_how_to_run_two(tmp_path: Path) -> None: + """The message is mostly the way out, because that is what the reader needs next. + + Someone who sees this is doing something reasonable — trying a downloaded build, or + running a second radio — so being told *no* without being told *how* would be the + unhelpful half of the answer. + """ + message = str(InstanceBusy(tmp_path)) + assert "MESHTERM_HOME" in message + assert str(tmp_path) in message + + +def test_a_write_lands_whole_or_not_at_all(tmp_path: Path) -> None: + """The written file holds exactly what was asked for, and no scratch file survives.""" + target = tmp_path / "sub" / "contacts.json" + write_atomically(target, json.dumps({"a": 1})) + assert json.loads(target.read_text(encoding="utf-8")) == {"a": 1} + assert list(tmp_path.rglob("*.tmp")) == [] + + +def test_the_scratch_file_is_named_for_its_writer(tmp_path: Path, monkeypatch) -> None: + """Two processes writing the same file must not share one temporary name. + + The rename is atomic; the thing being renamed is not. A fixed ``.tmp`` name lets two + writers interleave inside one scratch file and then each rename whatever ended up + there over the real one. + """ + seen: list[Path] = [] + real = Path.write_text + + def spy(self: Path, *args: object, **kwargs: object) -> int: + seen.append(self) + return real(self, *args, **kwargs) # type: ignore[arg-type] + + monkeypatch.setattr(Path, "write_text", spy) + write_atomically(tmp_path / "x.json", "{}") + assert seen and str(__import__("os").getpid()) in seen[0].name + + +def test_a_failed_write_leaves_no_litter(tmp_path: Path, monkeypatch) -> None: + """A write that raises cleans up after itself rather than leaving a partial file.""" + + def boom(self: Path, *args: object, **kwargs: object) -> int: + raise OSError("disk full") + + monkeypatch.setattr(Path, "write_text", boom) + with pytest.raises(OSError): + write_atomically(tmp_path / "y.json", "{}") + assert list(tmp_path.rglob("*.tmp")) == []