Prevent lost settings updates and partial configuration writes

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-<id>`, 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 `<config_dir>/my-plugin.json` off the event loop via `asyncio.to_thread`
+- **Persistence:** settings are written to `<config_dir>/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
 

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`.
+
 
 <!-- Add entries under Added, Changed, Deprecated, Removed, Fixed, or Security as changes land. -->

my-plugin/README.md

@@ -89,6 +89,7 @@ Update `routes.py` to handle any server-side logic:
 - Namespace all routes under `/api/plugins/<id>/...` 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 `<id>.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

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)

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()

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")) == []