From dfb9884486b9d5635897df4d28c9e0d8c50de3b4 Mon Sep 17 00:00:00 2001 From: Ramzi Alashmali <186906047+RAMZI0TO99@users.noreply.github.com> Date: Wed, 4 Nov 2026 17:40:03 +0300 Subject: [PATCH] fix: preserve saved settings when a write fails --- .github/workflows/request-validation.yml | 8 +- README.md | 6 +- app/core/config.py | 22 ++++- requirements-test.txt | 1 + tests/test_settings_persistence.py | 101 +++++++++++++++++++++++ 5 files changed, 129 insertions(+), 9 deletions(-) create mode 100644 tests/test_settings_persistence.py diff --git a/.github/workflows/request-validation.yml b/.github/workflows/request-validation.yml index 89aee98..9ac9c4b 100644 --- a/.github/workflows/request-validation.yml +++ b/.github/workflows/request-validation.yml @@ -1,4 +1,4 @@ -name: Request validation +name: Unit tests and Compose on: pull_request: @@ -25,10 +25,10 @@ jobs: - uses: actions/setup-python@6d4fd22b20465ccc0f7354476588e8df7c9ca0e0 # v7.1.0 with: python-version: ${{ matrix.python-version }} - - name: Install request-test dependencies + - name: Install unit-test dependencies run: python -m pip install --disable-pip-version-check --no-cache-dir -r requirements-test.txt - - name: Test request validation - run: python -m pytest tests/test_search_request.py -q + - name: Test lightweight units + run: python -m pytest tests -q - name: Validate Compose configuration run: | cp .env.example .env diff --git a/README.md b/README.md index 9e01d05..9216a61 100644 --- a/README.md +++ b/README.md @@ -142,11 +142,11 @@ This is an example request, not a recorded result. Search responses include medi - Speech alignment may fall back to English when a language-specific model cannot load; this can affect timestamps and transcript quality. - Dependencies, model caches, GPU compatibility, and the separately hosted LLM must be validated together. These setup instructions are derived from source inspection; they are not a claim that a clean build or full inference run has passed. -The request-validation tests run without model assets or a running service. In an isolated environment, run: +The request-validation and settings-persistence tests run without model assets or a running service. In an isolated environment, run: ```bash python -m pip install -r requirements-test.txt -python -m pytest tests/test_search_request.py +python -m pytest tests ``` -GitHub Actions runs these tests and validates the Compose configuration on Python 3.11 and 3.13 for pull requests and pushes to `main`. It does not build the inference image or start services. +GitHub Actions runs these tests and validates the Compose configuration on Python 3.11 and 3.13 for pull requests and pushes to `main`. The settings tests cover successful saves, serialization and write failures, replacement failures, and Unicode round trips. The workflow does not build the inference image or start services. diff --git a/app/core/config.py b/app/core/config.py index df2f939..9f13362 100644 --- a/app/core/config.py +++ b/app/core/config.py @@ -1,5 +1,7 @@ import os import json +import tempfile +from pathlib import Path from pydantic import BaseModel from dotenv import load_dotenv @@ -39,7 +41,23 @@ def load_settings() -> LLMSettings: ) def save_settings(settings: LLMSettings): - with open(SETTINGS_FILE, "w", encoding="utf-8") as f: - f.write(settings.model_dump_json(indent=4)) + payload = settings.model_dump_json(indent=4) + temporary_path = None + try: + with tempfile.NamedTemporaryFile( + mode="w", + encoding="utf-8", + dir=os.path.dirname(os.path.abspath(SETTINGS_FILE)), + prefix=".settings-", + suffix=".tmp", + delete=False, + ) as temporary: + temporary_path = temporary.name + temporary.write(payload) + os.replace(temporary_path, SETTINGS_FILE) + temporary_path = None + finally: + if temporary_path is not None: + Path(temporary_path).unlink(missing_ok=True) llm_settings = load_settings() diff --git a/requirements-test.txt b/requirements-test.txt index e3b9720..14b6250 100644 --- a/requirements-test.txt +++ b/requirements-test.txt @@ -1,2 +1,3 @@ pydantic>=2,<3 pytest>=8,<10 +python-dotenv>=1,<2 diff --git a/tests/test_settings_persistence.py b/tests/test_settings_persistence.py new file mode 100644 index 0000000..a6cc200 --- /dev/null +++ b/tests/test_settings_persistence.py @@ -0,0 +1,101 @@ +import json +from contextlib import contextmanager +from tempfile import NamedTemporaryFile +from unittest.mock import patch + +import pytest + +from app.core import config + + +@pytest.fixture +def settings_path(tmp_path, monkeypatch): + path = tmp_path / "settings.json" + monkeypatch.setattr(config, "SETTINGS_FILE", str(path)) + return path + + +def test_saved_settings_replace_the_previous_document(settings_path): + settings_path.write_text('{"provider": "local", "api_key": "previous-value"}') + settings = config.LLMSettings( + provider="custom", model="local-model", api_key="new-value" + ) + + config.save_settings(settings) + + assert config.load_settings() == settings + assert json.loads(settings_path.read_text())["api_key"] == "new-value" + assert list(settings_path.parent.glob(".settings-*.tmp")) == [] + + +def test_serialization_failure_preserves_previous_settings(settings_path): + previous = '{"provider": "local", "api_key": "previous-value"}' + settings_path.write_text(previous) + + with patch.object( + config.LLMSettings, "model_dump_json", side_effect=ValueError("serialize") + ): + with pytest.raises(ValueError, match="serialize"): + config.save_settings(config.LLMSettings()) + + assert settings_path.read_text() == previous + assert list(settings_path.parent.glob(".settings-*.tmp")) == [] + + +def test_replace_failure_preserves_previous_settings(settings_path): + previous = '{"provider": "local", "api_key": "previous-value"}' + settings_path.write_text(previous) + + with patch("os.replace", side_effect=OSError("replace failed")): + with pytest.raises(OSError, match="replace failed"): + config.save_settings(config.LLMSettings(api_key="new-value")) + + assert settings_path.read_text() == previous + assert list(settings_path.parent.glob(".settings-*.tmp")) == [] + + +def test_partial_write_failure_preserves_previous_settings(settings_path): + previous = '{"provider": "local", "api_key": "previous-value"}' + settings_path.write_text(previous) + + @contextmanager + def failing_temporary_file(*args, **kwargs): + with NamedTemporaryFile(*args, **kwargs) as temporary: + + class PartialWriter: + name = temporary.name + + def write(self, payload): + temporary.write(payload[:5]) + temporary.flush() + raise OSError("write failed") + + yield PartialWriter() + + with patch("tempfile.NamedTemporaryFile", failing_temporary_file): + with pytest.raises(OSError, match="write failed"): + config.save_settings(config.LLMSettings(api_key="new-value")) + + assert settings_path.read_text() == previous + assert list(settings_path.parent.glob(".settings-*.tmp")) == [] + + +def test_first_save_creates_a_complete_document(settings_path): + settings = config.LLMSettings(model="local-model") + config.save_settings(settings) + assert config.load_settings() == settings + + +def test_failed_first_serialization_does_not_create_an_empty_file(settings_path): + with patch.object( + config.LLMSettings, "model_dump_json", side_effect=ValueError("serialize") + ): + with pytest.raises(ValueError, match="serialize"): + config.save_settings(config.LLMSettings()) + assert not settings_path.exists() + + +def test_unicode_settings_round_trip(settings_path): + settings = config.LLMSettings(model="نموذج محلي") + config.save_settings(settings) + assert config.load_settings() == settings