[eric] settings: PATCH diff-merge save closes renderer-vs-agent lost update; per-loop write lock

backend/apps/settings/settings.py

@@ -146,21 +146,61 @@ SERVER_OWNED_FIELDS = (
 )
 
 
-# One serialization point for EVERY settings write (renderer PUT + agent tool),
-# so a renderer save and an autonomous agent edit can't interleave and clobber
-# each other mid read-modify-write. Callers hold it across read->build->save;
-# apply_settings_update itself does NOT acquire it (would deadlock the agent path
-# that reads under the same lock), so every caller must wrap apply in it.
-settings_write_lock = asyncio.Lock()
+import weakref as _weakref
+
+# One serialization point for EVERY settings write (renderer PUT/PATCH + agent
+# tool), so two writes can't interleave and clobber each other mid read-modify-
+# write. Callers hold it across read->build->save; apply_settings_update itself
+# does NOT acquire it (would deadlock the agent path that reads under it), so
+# every caller wraps apply in it. Created lazily PER event loop: prod has one
+# loop so it's effectively a singleton, but a module-level asyncio.Lock binds to
+# the first loop that uses it and then errors on reuse from another loop (every
+# async test spins a fresh one). WeakKeyDictionary auto-drops a loop's lock once
+# the loop is gone.
+_settings_write_locks: "_weakref.WeakKeyDictionary" = _weakref.WeakKeyDictionary()
+
+
+def settings_write_lock() -> asyncio.Lock:
+    loop = asyncio.get_running_loop()
+    lock = _settings_write_locks.get(loop)
+    if lock is None:
+        lock = asyncio.Lock()
+        _settings_write_locks[loop] = lock
+    return lock
 
 
 @settings.router.put("")
 async def update_settings(body: AppSettings):
-    async with settings_write_lock:
+    async with settings_write_lock():
         saved = await apply_settings_update(body)
     return {"ok": True, "settings": saved.model_dump()}
 
 
+@settings.router.patch("")
+async def patch_settings(changes: dict):
+    """Save only the fields the user changed, merged onto the CURRENT on-disk
+    state. The renderer sends a diff (not a stale full object), so a save can't
+    clobber a field something else, an agent, an OAuth connect, changed
+    underneath it. Makes the lost-update unrepresentable: you can't overwrite a
+    field you never sent."""
+    async with settings_write_lock():
+        saved = await apply_settings_patch(changes)
+    return {"ok": True, "settings": saved.model_dump()}
+
+
+async def apply_settings_patch(changes: dict) -> AppSettings:
+    """Merge `changes` onto fresh on-disk settings and persist. Caller holds
+    settings_write_lock so the read is current. Reuses apply_settings_update for
+    every side effect: the object it hands over IS current state plus the diff,
+    which is exactly what a non-clobbering save means."""
+    valid = set(AppSettings.model_fields.keys())
+    data = load_settings().model_dump()
+    for k, v in changes.items():
+        if k in valid:
+            data[k] = v
+    return await apply_settings_update(AppSettings(**data))
+
+
 async def apply_settings_update(body: AppSettings, protect_fields: set[str] | None = None) -> AppSettings:
     """Persist a full settings object with all the safety side effects: restore
     server-owned fields, hand the wheel back from the free trial when a real

backend/main.py

@@ -786,7 +786,7 @@ async def settings_meta(action: str, request: Request):
         # other's fields while BOTH got an "applied" result). The lock makes agent
         # writes serial so the last load always sees the prior write. (Agent vs the
         # renderer's own PUT stays the pre-existing full-object-replace race.)
-        async with settings_write_lock:
+        async with settings_write_lock():
             settings = load_settings()
             session = agent_manager.sessions.get(parent_session_id) if parent_session_id else None
             if session is not None:

backend/tests/test_settings_meta_endpoint.py

@@ -30,14 +30,14 @@ async def test_second_wall_restores_protected_credential_even_if_body_blanks_it(
         # A body that (as if a guard bug let it through) clears the live key.
         body = load_settings()
         body.anthropic_api_key = ""
-        async with settings_write_lock:
+        async with settings_write_lock():
             saved = await apply_settings_update(body, protect_fields={"anthropic_api_key"})
         assert saved.anthropic_api_key == "sk-live-KEEP-ME", "second wall failed to restore"
         assert load_settings().anthropic_api_key == "sk-live-KEEP-ME"
         # And a NON-protected blank still goes through (only the protected one is restored).
         body2 = load_settings()
         body2.openai_api_key = ""
-        async with settings_write_lock:
+        async with settings_write_lock():
             await apply_settings_update(body2, protect_fields={"anthropic_api_key"})
         assert not load_settings().openai_api_key
     finally:

backend/tests/test_settings_patch.py

@@ -0,0 +1,83 @@
+"""PATCH settings merges a diff onto fresh state, so a renderer save can't
+clobber a field it didn't send. This is the structural close on the last
+renderer-vs-agent race: the lost update is now unrepresentable, you can't
+overwrite a field you never put in the body.
+"""
+
+from __future__ import annotations
+
+import asyncio
+
+import httpx
+import pytest
+from fastapi.testclient import TestClient
+
+from backend.main import app
+
+
+def _auth_headers():
+    import backend.auth as auth_mod
+    if not auth_mod._TOKEN:
+        import secrets
+        auth_mod._TOKEN = secrets.token_urlsafe(32)
+    return {"Authorization": f"Bearer {auth_mod._TOKEN}"}
+
+
+@pytest.fixture
+def client():
+    return TestClient(app, headers=_auth_headers())
+
+
+@pytest.fixture
+def reset_settings():
+    from backend.apps.settings.settings import load_settings, _save_settings
+    original = load_settings().model_copy(deep=True)
+    yield
+    _save_settings(original)
+
+
+def test_patch_changes_only_sent_fields(client, reset_settings):
+    from backend.apps.settings.settings import load_settings, _save_settings
+    s = load_settings()
+    s.theme = "dark"
+    s.default_mode = "chat"  # as if something else had set this
+    _save_settings(s)
+
+    r = client.patch("/api/settings", json={"theme": "light"})
+    assert r.status_code == 200, r.text
+    final = load_settings()
+    assert final.theme == "light"           # the field we sent changed
+    assert final.default_mode == "chat"     # the field we DIDN'T send is untouched
+
+
+def test_patch_ignores_unknown_fields(client, reset_settings):
+    r = client.patch("/api/settings", json={"theme": "light", "not_a_field": 123})
+    assert r.status_code == 200
+    # Unknown key is dropped, not stored; the real field still applied.
+    from backend.apps.settings.settings import load_settings
+    assert load_settings().theme == "light"
+    assert "not_a_field" not in load_settings().model_dump()
+
+
+@pytest.mark.asyncio
+async def test_concurrent_renderer_patch_and_agent_write_both_survive(reset_settings):
+    """The renderer PATCHes one field while an autonomous agent writes another,
+    at the same time. Both must land: the renderer never sends the agent's field,
+    so it can't clobber it, and both reads happen fresh under the shared lock."""
+    from backend.apps.settings.settings import load_settings, _save_settings
+    base = load_settings()
+    base.theme = "dark"
+    base.default_mode = "agent"
+    _save_settings(base)
+
+    transport = httpx.ASGITransport(app=app)
+    async with httpx.AsyncClient(transport=transport, base_url="http://test", headers=_auth_headers()) as client:
+        r1, r2 = await asyncio.gather(
+            client.patch("/api/settings", json={"theme": "light"}),
+            client.post("/api/settings-meta/write", json={"changes": {"default_mode": "chat"}}),
+        )
+    assert r1.status_code == 200 and r2.status_code == 200
+
+    final = load_settings()
+    assert final.theme == "light", "renderer's change lost"
+    assert final.default_mode == "chat", "agent's change clobbered by the renderer PATCH"

frontend/src/app/Main.tsx

@@ -8,7 +8,7 @@ import Snackbar from '@mui/material/Snackbar';
 import Alert from '@mui/material/Alert';
 import { store } from '../shared/state/store';
 import { useAppDispatch, useAppSelector } from '@/shared/hooks';
-import { fetchSettings, updateSettings, markFreeTrialArmSettled } from '@/shared/state/settingsSlice';
+import { fetchSettings, updateSettingsPatch, markFreeTrialArmSettled } from '@/shared/state/settingsSlice';
 import { fetchSubscriptionStatus } from '@/shared/state/subscriptionsSlice';
 import { fetchModels } from '@/shared/state/modelsSlice';
 import { API_BASE } from '@/shared/config';
@@ -324,7 +324,7 @@ const DefaultModelGuard: React.FC<{ children: React.ReactNode }> = ({ children }
 
     const fromLabel = flat.find((m) => m.value === settings.default_model)?.label ?? settings.default_model;
     pendingRef.current = true;
-    dispatch(updateSettings({ ...settings, default_model: fallback.value }))
+    dispatch(updateSettingsPatch({ default_model: fallback.value }))
       .finally(() => {
         pendingRef.current = false;
       });

frontend/src/app/pages/Dashboard/DashboardToolbar.tsx

@@ -31,7 +31,7 @@ import { useElementSelection } from '@/app/components/editor/ElementSelectionCon
 import { useClaudeTokens } from '@/shared/styles/ThemeContext';
 import { useAppDispatch, useAppSelector } from '@/shared/hooks';
 import { searchHistory, clearHistorySearch } from '@/shared/state/agentsSlice';
-import { updateSettings, AppSettings } from '@/shared/state/settingsSlice';
+import { updateSettingsPatch, AppSettings } from '@/shared/state/settingsSlice';
 import { store } from '@/shared/state/store';
 import type { ClaudeTokens } from '@/shared/styles/claudeTokens';
 import type { Output } from '@/shared/state/outputsSlice';
@@ -158,7 +158,7 @@ const DashboardToolbar = React.forwardRef<HTMLDivElement, Props>(
       const current = store.getState().settings;
       if (!current.loaded) return;
       if (current.data[key] === value) return;
-      dispatch(updateSettings({ ...current.data, [key]: value }));
+      dispatch(updateSettingsPatch({ [key]: value }));
     }, [dispatch]);
 
     const handleModeChange = useCallback((newMode: string) => {

frontend/src/app/pages/Settings/Settings.tsx

@@ -5,7 +5,7 @@ import Alert from '@mui/material/Alert';
 import Dialog from '@mui/material/Dialog';
 import DialogContent from '@mui/material/DialogContent';
 import { useAppDispatch, useAppSelector } from '@/shared/hooks';
-import { updateSettings, closeSettingsModal, AppSettings } from '@/shared/state/settingsSlice';
+import { updateSettingsPatch, closeSettingsModal, AppSettings } from '@/shared/state/settingsSlice';
 import { onboardingBus } from '@/app/components/Onboarding/eventBus';
 import { fetchModels } from '@/shared/state/modelsSlice';
 import { fetchModes } from '@/shared/state/modesSlice';
@@ -153,17 +153,18 @@ const Settings: React.FC = () => {
 
   // Only the fields the user touched ride on top of the LATEST settings; submitting the
   // whole stale form would clobber background updates and ping-pong with server-owned fields.
-  const buildSubmit = useCallback((): { submit: AppSettings; touched: string[] } | null => {
+  const buildSubmit = useCallback((): { touched: string[]; patch: Partial<AppSettings> } | null => {
     const base = baselineRef.current as unknown as Record<string, unknown>;
     const f = form as unknown as Record<string, unknown>;
     const touched = Array.from(new Set([...Object.keys(base), ...Object.keys(f)]))
       .filter((k) => JSON.stringify(f[k]) !== JSON.stringify(base[k]));
     if (touched.length === 0) return null;
-    const submit = { ...settings } as unknown as Record<string, unknown>;
-    for (const k of touched) submit[k] = f[k];
-    if (JSON.stringify(submit) === JSON.stringify(settings)) return null;
-    return { submit: submit as unknown as AppSettings, touched };
-  }, [form, settings]);
+    // Send ONLY what the user changed; the server merges it onto fresh state, so
+    // we never re-send (and clobber) a field something else updated underneath us.
+    const patch: Record<string, unknown> = {};
+    for (const k of touched) patch[k] = f[k];
+    return { touched, patch: patch as Partial<AppSettings> };
+  }, [form]);
 
   // Theme is local UI state; apply it the moment the toggle flips, the debounced save persists it.
   useEffect(() => {
@@ -183,7 +184,7 @@ const Settings: React.FC = () => {
       if (!payload) return;
       inFlight.current = true;
       try {
-        await dispatch(updateSettings(payload.submit)).unwrap();
+        await dispatch(updateSettingsPatch(payload.patch)).unwrap();
         // Absorb the saved edits so they stop counting as touched (prevents re-save loops).
         const nextBase = { ...baselineRef.current } as Record<string, unknown>;
         for (const k of payload.touched) nextBase[k] = (form as unknown as Record<string, unknown>)[k];
@@ -205,7 +206,7 @@ const Settings: React.FC = () => {
     if (saveTimer.current) clearTimeout(saveTimer.current);
     const payload = loaded ? buildSubmit() : null;
     if (payload) {
-      dispatch(updateSettings(payload.submit));
+      dispatch(updateSettingsPatch(payload.patch));
       dispatch(fetchModels());
       baselineRef.current = form;
     }

frontend/src/shared/state/settingsSlice.ts

@@ -148,13 +148,16 @@ export const fetchSettings = createAsyncThunk('settings/fetch', async () => {
   return (await res.json()) as AppSettings;
 });
 
-export const updateSettings = createAsyncThunk(
-  'settings/update',
-  async (settings: AppSettings) => {
+// Save ONLY the fields the user changed, merged server-side onto fresh state.
+// Every renderer save uses this so a stale full object can never clobber a field
+// the user didn't touch (e.g. one an agent just changed).
+export const updateSettingsPatch = createAsyncThunk(
+  'settings/patch',
+  async (changes: Partial<AppSettings>) => {
     const res = await fetch(SETTINGS_API, {
-      method: 'PUT',
+      method: 'PATCH',
       headers: { 'Content-Type': 'application/json' },
-      body: JSON.stringify(settings),
+      body: JSON.stringify(changes),
     });
     const data = await res.json();
     return data.settings as AppSettings;
@@ -287,11 +290,11 @@ const settingsSlice = createSlice({
         state.loading = false;
         state.loaded = true;
       })
-      .addCase(updateSettings.fulfilled, (state, action) => {
-        // A user save is authoritative; claim newest so an in-flight GET can't overwrite it.
+      .addCase(updateSettingsPatch.fulfilled, (state, action) => {
+        // A user save is authoritative; claim newest so an in-flight GET can't
+        // overwrite it, and consume the draft so reopening shows the saved state.
         state.latestWriteId = action.meta.requestId;
         state.data = action.payload;
-        // Save consumes the draft so reopening doesn't restore stale edits.
         state.draft = null;
         state.draftTab = null;
       })