diff --git a/packages/shared-python/shared/services/telemetry/identity.py b/packages/shared-python/shared/services/telemetry/identity.py index 8407e4939..3803dce1a 100644 --- a/packages/shared-python/shared/services/telemetry/identity.py +++ b/packages/shared-python/shared/services/telemetry/identity.py @@ -2,11 +2,22 @@ from __future__ import annotations -import fcntl import os +import sys +import threading +from collections.abc import Iterator +from contextlib import contextmanager from pathlib import Path +from typing import IO from uuid import UUID, uuid4 +if sys.platform == "win32": + import msvcrt +else: + import fcntl + +_PROCESS_LOCK = threading.Lock() + def get_or_create_installation_id( *, @@ -27,8 +38,7 @@ def get_or_create_installation_id( lock_path = installation_id_path.with_suffix(f"{installation_id_path.suffix}.lock") with lock_path.open("a+", encoding="utf-8") as lock_file: - fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) - try: + with _exclusive_file_lock(lock_file): existing_installation_id = _read_valid_installation_id( installation_id_path ) @@ -41,8 +51,46 @@ def get_or_create_installation_id( generated_installation_id, ) return generated_installation_id + + +@contextmanager +def _exclusive_file_lock(lock_file: IO[str]) -> Iterator[None]: + """Cross-process file lock plus an in-process mutex. + + POSIX uses ``fcntl.flock``. Windows ``msvcrt.locking`` is per-process, so a + thread lock is required for concurrent callers in the same interpreter. + """ + with _PROCESS_LOCK: + _lock_exclusive(lock_file) + try: + yield finally: - fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + _unlock_exclusive(lock_file) + + +def _lock_exclusive(lock_file: IO[str]) -> None: + if sys.platform == "win32": + _ensure_lock_byte(lock_file) + lock_file.seek(0) + msvcrt.locking(lock_file.fileno(), msvcrt.LK_LOCK, 1) + return + fcntl.flock(lock_file.fileno(), fcntl.LOCK_EX) + + +def _unlock_exclusive(lock_file: IO[str]) -> None: + if sys.platform == "win32": + lock_file.seek(0) + msvcrt.locking(lock_file.fileno(), msvcrt.LK_UNLCK, 1) + return + fcntl.flock(lock_file.fileno(), fcntl.LOCK_UN) + + +def _ensure_lock_byte(lock_file: IO[str]) -> None: + lock_file.seek(0, os.SEEK_END) + if lock_file.tell() == 0: + lock_file.write("0") + lock_file.flush() + os.fsync(lock_file.fileno()) def _read_valid_installation_id(installation_id_path: Path) -> str: diff --git a/packages/shared-python/shared/tests/test_telemetry_identity.py b/packages/shared-python/shared/tests/test_telemetry_identity.py new file mode 100644 index 000000000..708d30d7f --- /dev/null +++ b/packages/shared-python/shared/tests/test_telemetry_identity.py @@ -0,0 +1,100 @@ +"""Unit tests for the stable self-hosted telemetry installation id.""" + +from __future__ import annotations + +import sys +import threading +from concurrent.futures import ThreadPoolExecutor +from pathlib import Path +from uuid import UUID + +import pytest + +from shared.services.telemetry.identity import get_or_create_installation_id + + +def test_explicit_installation_id_wins(tmp_path: Path) -> None: + installation_id_path = tmp_path / "telemetry-installation-id" + explicit_installation_id = "550e8400-e29b-41d4-a716-446655440000" + + installation_id = get_or_create_installation_id( + explicit_installation_id=explicit_installation_id, + installation_id_path=installation_id_path, + ) + + assert installation_id == explicit_installation_id + assert not installation_id_path.exists() + + +def test_explicit_installation_id_must_be_uuid(tmp_path: Path) -> None: + with pytest.raises(ValueError, match="must be a UUID"): + get_or_create_installation_id( + explicit_installation_id="not-a-uuid", + installation_id_path=tmp_path / "telemetry-installation-id", + ) + + +def test_missing_file_generates_uuid(tmp_path: Path) -> None: + installation_id_path = tmp_path / "telemetry-installation-id" + + installation_id = get_or_create_installation_id( + explicit_installation_id="", + installation_id_path=installation_id_path, + ) + + UUID(installation_id) + assert installation_id_path.read_text(encoding="utf-8").strip() == installation_id + + +def test_generated_id_is_stable_across_calls(tmp_path: Path) -> None: + installation_id_path = tmp_path / "telemetry-installation-id" + + first = get_or_create_installation_id( + explicit_installation_id="", + installation_id_path=installation_id_path, + ) + second = get_or_create_installation_id( + explicit_installation_id="", + installation_id_path=installation_id_path, + ) + + assert first == second + + +def test_invalid_existing_file_is_replaced(tmp_path: Path) -> None: + installation_id_path = tmp_path / "telemetry-installation-id" + installation_id_path.write_text("not-a-uuid\n", encoding="utf-8") + + installation_id = get_or_create_installation_id( + explicit_installation_id="", + installation_id_path=installation_id_path, + ) + + UUID(installation_id) + assert installation_id_path.read_text(encoding="utf-8").strip() == installation_id + + +def test_concurrent_calls_return_the_same_id(tmp_path: Path) -> None: + installation_id_path = tmp_path / "telemetry-installation-id" + barrier = threading.Barrier(8) + + def _resolve() -> str: + barrier.wait(timeout=5) + return get_or_create_installation_id( + explicit_installation_id="", + installation_id_path=installation_id_path, + ) + + with ThreadPoolExecutor(max_workers=8) as executor: + results = list(executor.map(lambda _: _resolve(), range(8))) + + assert len(set(results)) == 1 + assert installation_id_path.read_text(encoding="utf-8").strip() == results[0] + + +@pytest.mark.skipif(sys.platform != "win32", reason="fcntl is POSIX-only") +def test_identity_import_does_not_require_fcntl() -> None: + assert "fcntl" not in sys.modules + from shared.services.telemetry import identity as identity_module + + assert identity_module.get_or_create_installation_id is get_or_create_installation_id