From c722641fec1e43043e879b4c79815cd4d1a38993 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Fri, 19 Jun 2026 18:38:02 -0700 Subject: [PATCH] [eric] settings: PATCH diff-merge save closes renderer-vs-agent lost update; per-loop write lock --- backend/apps/settings/settings.py | 54 ++++++++++-- backend/main.py | 2 +- backend/tests/test_settings_meta_endpoint.py | 4 +- backend/tests/test_settings_patch.py | 83 +++++++++++++++++++ frontend/src/app/Main.tsx | 4 +- .../app/pages/Dashboard/DashboardToolbar.tsx | 4 +- frontend/src/app/pages/Settings/Settings.tsx | 19 +++-- frontend/src/shared/state/settingsSlice.ts | 19 +++-- 8 files changed, 158 insertions(+), 31 deletions(-) create mode 100644 backend/tests/test_settings_patch.py diff --git a/backend/apps/settings/settings.py b/backend/apps/settings/settings.py index 1749ac75..fcc0a6ce 100644 --- a/backend/apps/settings/settings.py +++ b/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 diff --git a/backend/main.py b/backend/main.py index f5d914f5..f3f123b7 100644 --- a/backend/main.py +++ b/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: diff --git a/backend/tests/test_settings_meta_endpoint.py b/backend/tests/test_settings_meta_endpoint.py index 6c04c83a..d0506246 100644 --- a/backend/tests/test_settings_meta_endpoint.py +++ b/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: diff --git a/backend/tests/test_settings_patch.py b/backend/tests/test_settings_patch.py new file mode 100644 index 00000000..65f0da4c --- /dev/null +++ b/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" diff --git a/frontend/src/app/Main.tsx b/frontend/src/app/Main.tsx index 2046427f..8f354fd1 100644 --- a/frontend/src/app/Main.tsx +++ b/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; }); diff --git a/frontend/src/app/pages/Dashboard/DashboardToolbar.tsx b/frontend/src/app/pages/Dashboard/DashboardToolbar.tsx index 6684a805..46e2493c 100644 --- a/frontend/src/app/pages/Dashboard/DashboardToolbar.tsx +++ b/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( 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) => { diff --git a/frontend/src/app/pages/Settings/Settings.tsx b/frontend/src/app/pages/Settings/Settings.tsx index 3d180577..d9b5618f 100644 --- a/frontend/src/app/pages/Settings/Settings.tsx +++ b/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 } | null => { const base = baselineRef.current as unknown as Record; const f = form as unknown as Record; 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; - 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 = {}; + for (const k of touched) patch[k] = f[k]; + return { touched, patch: patch as Partial }; + }, [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; for (const k of payload.touched) nextBase[k] = (form as unknown as Record)[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; } diff --git a/frontend/src/shared/state/settingsSlice.ts b/frontend/src/shared/state/settingsSlice.ts index 1946c2a2..39d467de 100644 --- a/frontend/src/shared/state/settingsSlice.ts +++ b/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) => { 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; })