diff --git a/AGENTS.md b/AGENTS.md index deeabb6..4f0ae68 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -12,7 +12,7 @@ A reference implementation demonstrating the fundamental feedBack plugin contrac | `my-plugin/screen.html` | Client screen markup — mounted into `plugin-my-plugin` by the Host, holds the counter and settings hooks `screen.js` queries | | `my-plugin/screen.js` | Client screen logic — finds its root by `plugin-`, wires up an example counter, loads persisted settings | | `my-plugin/settings.html` | Settings panel markup, grouped under `settings.category` | -| `my-plugin/routes.py` | FastAPI `setup(app, context)` — registers GET/POST `/api/plugins/my-plugin/settings` with schema validation and persistence | +| `my-plugin/routes.py` | FastAPI `setup(app, context)` — registers GET/POST `/api/plugins/my-plugin/settings` with schema validation and serialized, atomic persistence | | `my-plugin/assets/plugin.css` | Styling scoped to the plugin | | `my-plugin/README.md` | Per-plugin documentation for whoever copies this template | | `tests/` | pytest suite for the manifest/screen contract and the settings routes | @@ -25,7 +25,7 @@ A reference implementation demonstrating the fundamental feedBack plugin contrac - **GET response:** current settings, merged with defaults for any missing keys - **POST body:** a JSON object containing a subset of recognized settings; unknown keys or invalid values return `400` - **POST limits:** request body capped at `MAX_SETTINGS_BODY_BYTES` (16 KiB); oversized or malformed bodies return `413`/`400` -- **Persistence:** settings are written to `/my-plugin.json` off the event loop via `asyncio.to_thread` +- **Persistence:** settings are written to `/my-plugin.json` off the event loop via `asyncio.to_thread`; concurrent writes are serialized by an `asyncio.Lock` and applied atomically (temp file + `os.replace`), so a failed write preserves the last valid config ## Key conventions (feedBack plugin contract) @@ -44,6 +44,8 @@ A reference implementation demonstrating the fundamental feedBack plugin contrac - `screen.js` uses a `window[\`__${PLUGIN_ID}_setup\`]` guard to avoid double-initializing on re-hydration. - `loadSettings()` fails soft on fetch errors (`console.warn`) rather than throwing, so a settings-load failure degrades the screen instead of crashing it. - `_is_valid_setting()` in `routes.py` uses `type(value) is int` / `type(value) is bool` (not `isinstance`) specifically to reject `bool` being accepted where `int` is expected (Python's `bool` is an `int` subclass). +- `routes.py` serializes the whole read–merge–write with an `asyncio.Lock` and persists via `_write_settings_atomically()` (temp file in the same directory + `os.replace`). The lock is per-process only — multiple workers sharing a `config_dir` would need a cross-process lock; `os.replace` still prevents torn files. The guarantee also assumes each handler runs to completion: cancelling the task mid-`asyncio.to_thread` releases the lock while the worker thread finishes anyway. +- The test fixture uses `with TestClient(app) as client:` on purpose: every request must share one event loop, otherwise a per-request loop rejects the `asyncio.Lock` bound by the first request. ## Verification checklist @@ -63,6 +65,7 @@ Against a running Host with `my-plugin/` installed: 5. POSTing a valid settings subset persists and is reflected on next GET 6. POSTing an unknown key or invalid value returns `400` 7. Oversized POST body returns `413` +8. Concurrent POSTs to different settings keys all persist — no lost updates (the `asyncio.Lock` in `routes.py` serializes them) ## Common pitfalls diff --git a/CHANGELOG.md b/CHANGELOG.md index 97eee03..a710c57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,5 +20,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - `my-plugin/plugin.json` `type` is now `"utility"` instead of `"visualization"`, matching what the example actually is; the `visualization` type is reserved for plugins that implement the `setRenderer` note-highway contract. +### Fixed + +- `my-plugin/routes.py` now serializes the full read–merge–persist with an `asyncio.Lock` and writes settings atomically (temp file + `os.replace`), so concurrent subset `POST`s no longer overwrite each other and a failed or interrupted write preserves the last valid configuration. Covered by an overlapping-request test and a failed-write test; the single-process scope of the lock is documented in `my-plugin/README.md` and `AGENTS.md`. + diff --git a/my-plugin/README.md b/my-plugin/README.md index c06ad4a..d403ffe 100644 --- a/my-plugin/README.md +++ b/my-plugin/README.md @@ -89,6 +89,7 @@ Update `routes.py` to handle any server-side logic: - Namespace all routes under `/api/plugins//...` to avoid collisions. - Validate configuration before registering any route. - Read/write only inside `context["config_dir"]` — never elsewhere. +- Persist settings atomically (temp file + `os.replace`) and serialize concurrent updates (an `asyncio.Lock` held across the whole read–merge–write), so overlapping requests don't drop each other's changes and an interrupted write can't corrupt the config. The example demonstrates both. ### 6. Edit the styles @@ -119,6 +120,15 @@ The tests discover the plugin by scanning for `plugin.json`, so they keep workin The template includes working examples of this flow in `settings.html` and `routes.py`. +### Concurrency and durability + +Settings writes in this example are safe for concurrent requests within one server process: + +- **Serialized updates.** `POST` holds an `asyncio.Lock` across the whole read–merge–write, so overlapping subset updates observe each other's changes instead of overwriting them. Two requests that each change a different setting both survive. +- **Atomic writes.** The server writes to a temporary file in `config_dir`, flushes it to disk, then replaces `.json` with `os.replace`. Readers only ever see a complete document, and a failed or interrupted write leaves the previous configuration untouched (the request returns `500`). `tempfile.mkstemp` creates the temporary file owner-only, so the resulting config ends up with mode `0600` on POSIX; `chmod` it afterwards if other users need to read it. + +**Multi-process limitation.** The lock lives in a single process's event loop, so it does not coordinate across processes. If you run multiple workers that share one `config_dir`, add a cross-process lock (`fcntl.flock`/`msvcrt.locking`, or an external mutex) around the read–merge–write; atomic `os.replace` still prevents torn files, but last-writer-wins can drop a concurrent update. + ## Common tasks ### Add a new setting diff --git a/my-plugin/routes.py b/my-plugin/routes.py index 3130645..6a6c396 100644 --- a/my-plugin/routes.py +++ b/my-plugin/routes.py @@ -5,12 +5,18 @@ * all work happens inside setup() — nothing at import time; * configuration is read tolerantly (missing file => defaults); * routes are namespaced under the plugin id; - * setup() validates before registering any route. + * setup() validates before registering any route; + * settings writes are serialized (asyncio.Lock) and atomic + (temp file + os.replace), so overlapping updates cannot + overwrite each other and a failed write cannot corrupt + the stored configuration. """ import asyncio import json import logging +import os +import tempfile from pathlib import Path from fastapi import FastAPI, Request @@ -37,6 +43,31 @@ def _is_valid_setting(name: str, value: object) -> bool: return False +def _write_settings_atomically(config_file: Path, settings: dict) -> None: + """Persist settings so the destination only ever holds a complete file. + + Writes to a temporary file in the same directory, flushes it to disk, then + atomically replaces the destination with os.replace. A failure at any point + leaves the previous configuration intact and removes the temporary file. + """ + config_file.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp( + dir=config_file.parent, prefix=f".{config_file.name}.", suffix=".tmp" + ) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + json.dump(settings, handle, indent=2) + handle.flush() + os.fsync(handle.fileno()) + os.replace(tmp_name, config_file) + except BaseException: + try: + os.unlink(tmp_name) + except OSError: + pass + raise + + def setup(app: FastAPI, context: dict) -> None: """Register routes for my-plugin. @@ -48,6 +79,10 @@ def setup(app: FastAPI, context: dict) -> None: log = context.get("log") or logging.getLogger(f"feedBack.plugin.{PLUGIN_ID}") config_file = config_dir / f"{PLUGIN_ID}.json" + # Serializes the whole read-merge-write for concurrent requests on this + # event loop. See the README for the multi-process limitation. + settings_lock = asyncio.Lock() + def _read() -> dict: """Read persisted settings, tolerating a missing or corrupt file.""" if not config_file.exists(): @@ -138,15 +173,20 @@ async def set_settings(request: Request) -> JSONResponse: def _merge_and_persist() -> dict: """Merge incoming settings with existing ones and persist. Runs off the event loop (asyncio.to_thread below) since _read() and - the write below are blocking filesystem calls, and set_settings - must stay `async def` to stream the request body above.""" + the write are blocking filesystem calls, and set_settings must + stay `async def` to stream the request body above.""" merged = {**_read(), **incoming} - config_dir.mkdir(parents=True, exist_ok=True) - config_file.write_text(json.dumps(merged, indent=2), encoding="utf-8") + _write_settings_atomically(config_file, merged) return merged try: - merged = await asyncio.to_thread(_merge_and_persist) + # Hold the lock across the full read-merge-write so concurrent + # subset updates observe each other's changes instead of + # overwriting them (last writer wins on stale data). The lock + # only covers this process; see the README for multi-process + # deployments. + async with settings_lock: + merged = await asyncio.to_thread(_merge_and_persist) log.info("%s: settings updated", PLUGIN_ID) except Exception as exc: log.error("%s: failed to write settings: %s", PLUGIN_ID, exc) diff --git a/tests/conftest.py b/tests/conftest.py index e501c7b..5ac4360 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -46,12 +46,16 @@ def routes_module(plugin_dir: Path, manifest: dict): @pytest.fixture() -def client(plugin_dir: Path, manifest: dict, routes_module, tmp_path: Path) -> TestClient: +def client(plugin_dir: Path, manifest: dict, routes_module, tmp_path: Path): app = FastAPI() routes_module.setup( app, {"config_dir": tmp_path, "log": logging.getLogger("plugin-tests")} ) - return TestClient(app) + # Enter the context manager so every request shares one event loop/portal. + # The routes serialize writes with an asyncio.Lock, which is bound to the + # loop it is first awaited on; a per-request loop would reject it. + with TestClient(app) as test_client: + yield test_client @pytest.fixture() diff --git a/tests/test_settings_routes.py b/tests/test_settings_routes.py index ebeed65..5e2e8f5 100644 --- a/tests/test_settings_routes.py +++ b/tests/test_settings_routes.py @@ -6,6 +6,7 @@ """ import json +from concurrent.futures import ThreadPoolExecutor from pathlib import Path import pytest @@ -91,3 +92,52 @@ def chunks(): yield b"x" * 1024 assert client.post(settings_url, content=chunks()).status_code == 413 + + +def test_overlapping_updates_to_different_settings_are_all_retained( + client, settings_url +) -> None: + """Concurrent subset POSTs must serialize read-merge-write. + + Without serialization each request reads the same snapshot and the last + write wins, silently dropping the other updates. + """ + bodies = [ + {"color": "crimson"}, + {"intensity": 9}, + {"enable_animations": False}, + ] + with ThreadPoolExecutor(max_workers=len(bodies)) as pool: + statuses = list( + pool.map(lambda body: client.post(settings_url, json=body).status_code, bodies) + ) + + assert statuses == [200, 200, 200] + assert client.get(settings_url).json() == { + "color": "crimson", + "intensity": 9, + "enable_animations": False, + } + + +def test_failed_write_preserves_last_valid_config( + client, settings_url, manifest: dict, tmp_path: Path, routes_module, monkeypatch +) -> None: + config_file = tmp_path / f"{manifest['id']}.json" + assert client.post(settings_url, json={"color": "crimson"}).status_code == 200 + before = config_file.read_text(encoding="utf-8") + + def boom(src, dst): + raise OSError("simulated write failure") + + # Fail the final rename so the temp file was already written: the handler + # must clean it up and leave the previous configuration untouched. + monkeypatch.setattr(routes_module.os, "replace", boom) + + res = client.post(settings_url, json={"intensity": 9}) + assert res.status_code == 500 + + assert client.get(settings_url).json() == {**DEFAULTS, "color": "crimson"} + assert config_file.read_text(encoding="utf-8") == before + assert json.loads(before) == {**DEFAULTS, "color": "crimson"} + assert list(tmp_path.glob(".*.tmp")) == []