fix(web): config RMW handlers hold _CONFIG_MUTATION_LOCK so concurrent saves stop dropping writes
Only PUT /api/config took the span lock; model.set, moa, custom-endpoint create/activate/delete, memory-provider saves, and the profile-dir model write ran load→mutate→save unlocked in worker threads. The desktop fires these concurrently with its debounced autosave — whichever save landed second silently dropped the other's mutation (#88913/#89184 lost-write flavor). Race test with slowed save proves both writes now survive.
This commit is contained in:
@@ -543,7 +543,10 @@ def list_custom_endpoints(profile: Optional[str] = None):
|
||||
def upsert_custom_endpoint(body: CustomEndpointUpdate, profile: Optional[str] = None):
|
||||
"""Create or update a v12+ ``providers`` custom endpoint entry."""
|
||||
with http_failure("POST /api/providers/custom-endpoints failed", 500, detail="Failed to save custom endpoint"):
|
||||
with _config_profile_scope(profile):
|
||||
# Sync-def endpoints run on worker threads: the load→mutate→save span
|
||||
# holds _CONFIG_MUTATION_LOCK so a concurrent config autosave cannot
|
||||
# drop this write (or vice versa).
|
||||
with _config_profile_scope(profile), _CONFIG_MUTATION_LOCK:
|
||||
cfg = load_config()
|
||||
endpoint_id, _entry = _write_custom_endpoint(cfg, body)
|
||||
save_config(cfg)
|
||||
@@ -560,7 +563,7 @@ def activate_custom_endpoint(endpoint_id: str, profile: Optional[str] = None):
|
||||
f"POST /api/providers/custom-endpoints/{endpoint_id}/activate failed", 500,
|
||||
detail="Failed to activate custom endpoint",
|
||||
):
|
||||
with _config_profile_scope(profile):
|
||||
with _config_profile_scope(profile), _CONFIG_MUTATION_LOCK: # RMW span
|
||||
cfg = load_config()
|
||||
provider_key = _custom_endpoint_id(endpoint_id)
|
||||
_stored, entry = find_provider_entry(cfg.get("providers"), provider_key)
|
||||
@@ -601,7 +604,7 @@ def delete_custom_endpoint(endpoint_id: str, profile: Optional[str] = None):
|
||||
f"DELETE /api/providers/custom-endpoints/{endpoint_id} failed", 500,
|
||||
detail="Failed to delete custom endpoint",
|
||||
):
|
||||
with _config_profile_scope(profile):
|
||||
with _config_profile_scope(profile), _CONFIG_MUTATION_LOCK: # RMW span
|
||||
cfg = load_config()
|
||||
provider_key = _custom_endpoint_id(endpoint_id)
|
||||
providers = cfg.get("providers")
|
||||
|
||||
@@ -22,7 +22,7 @@ from hermes_cli.web_server_memory import (
|
||||
_coerce_bool, _field_default, _field_is_set, _field_value, _field_visible, _load_memory_provider, _memory_provider_manifest, _memory_provider_setup_info, _memory_provider_setup_manifest, _normalize_memory_provider_schema, _read_memory_provider_existing_values, _require_memory_provider_ready, _run_setup_command,
|
||||
)
|
||||
from hermes_cli.web_models import MemoryProviderConfigUpdate, MemoryProviderSetupRequest
|
||||
from hermes_cli.web_routers._common import scoped_to_thread
|
||||
from hermes_cli.web_routers._common import _CONFIG_MUTATION_LOCK, scoped_to_thread
|
||||
from plugins.memory.config_schema import (
|
||||
STORAGE_HONCHO_HOST_BLOCK, ProviderConfigSchema, ProviderField, get_provider_config_schema,
|
||||
)
|
||||
@@ -304,11 +304,12 @@ def _memory_section(config: Dict[str, Any]) -> Dict[str, Any]:
|
||||
def _update_memory_provider_config(provider: ProviderConfigSchema, values: Dict[str, str]) -> None:
|
||||
writer = _write_provider_honcho if provider.storage == STORAGE_HONCHO_HOST_BLOCK else _write_provider_flat
|
||||
writer(provider, values)
|
||||
config = load_config()
|
||||
memory_config = _memory_section(config)
|
||||
if memory_config.get("provider") != provider.name:
|
||||
memory_config["provider"] = provider.name
|
||||
save_config(config)
|
||||
with _CONFIG_MUTATION_LOCK: # RMW span vs. the dashboard's config autosave
|
||||
config = load_config()
|
||||
memory_config = _memory_section(config)
|
||||
if memory_config.get("provider") != provider.name:
|
||||
memory_config["provider"] = provider.name
|
||||
save_config(config)
|
||||
|
||||
|
||||
# ── Setup: dependency installation ────────────────────────────────────────────
|
||||
@@ -468,11 +469,12 @@ def _save_memory_provider_native_config(name: str, provider: Any, values: Dict[s
|
||||
if _BaseMemoryProvider is None or type(provider).save_config is not _BaseMemoryProvider.save_config:
|
||||
provider.save_config(values, str(get_hermes_home()))
|
||||
return
|
||||
cfg = load_config()
|
||||
memory_cfg = _memory_section(cfg)
|
||||
current = memory_cfg.get(name)
|
||||
memory_cfg[name] = {**(current if isinstance(current, dict) else {}), **values}
|
||||
save_config(cfg)
|
||||
with _CONFIG_MUTATION_LOCK: # RMW span vs. the dashboard's config autosave
|
||||
cfg = load_config()
|
||||
memory_cfg = _memory_section(cfg)
|
||||
current = memory_cfg.get(name)
|
||||
memory_cfg[name] = {**(current if isinstance(current, dict) else {}), **values}
|
||||
save_config(cfg)
|
||||
|
||||
|
||||
def _write_memory_provider_config_values(name: str, provider: Any, values: Dict[str, Any]) -> None:
|
||||
@@ -567,9 +569,10 @@ async def update_memory_provider_config(
|
||||
raise _unknown_provider(name)
|
||||
_write_memory_provider_config_values(name, provider, values)
|
||||
_require_memory_provider_ready(name)
|
||||
config = load_config()
|
||||
_memory_section(config)["provider"] = name
|
||||
save_config(config)
|
||||
with _CONFIG_MUTATION_LOCK: # RMW span vs. the dashboard's config autosave
|
||||
config = load_config()
|
||||
_memory_section(config)["provider"] = name
|
||||
save_config(config)
|
||||
_invalidate_plugins_hub_cache()
|
||||
return {"ok": True, "active": name}
|
||||
|
||||
|
||||
@@ -17,7 +17,7 @@ from hermes_cli.web_server_config import (
|
||||
from agent.model_metadata import is_local_endpoint
|
||||
from starlette.concurrency import run_in_threadpool
|
||||
from hermes_cli.web_models import ModelAssignment, MoaConfigPayload, MoaModelSlot
|
||||
from hermes_cli.web_routers._common import http_failure
|
||||
from hermes_cli.web_routers._common import config_write_scope, http_failure
|
||||
|
||||
_log = logging.getLogger("hermes_cli.web_server")
|
||||
router = APIRouter()
|
||||
@@ -234,7 +234,10 @@ def set_moa_models(body: MoaConfigPayload, profile: Optional[str] = None):
|
||||
with http_failure("PUT /api/model/moa failed", 500, detail="Failed to save MoA config"):
|
||||
from hermes_cli.moa_config import normalize_moa_config, validate_moa_payload
|
||||
|
||||
with _profile_scope(body.profile or profile):
|
||||
# load→mutate→save runs on a worker thread (sync-def endpoint); the
|
||||
# desktop's debounced PUT /api/config autosave races it, so the whole
|
||||
# span holds _CONFIG_MUTATION_LOCK or one of the two saves is dropped.
|
||||
with config_write_scope(body.profile or profile):
|
||||
cfg = load_config()
|
||||
if body.presets:
|
||||
raw = {
|
||||
@@ -296,7 +299,9 @@ async def set_model_assignment(body: ModelAssignment, profile: Optional[str] = N
|
||||
reasoning_effort = body.reasoning_effort if "reasoning_effort" in body.model_fields_set else _UNSET
|
||||
|
||||
def _apply_assignment():
|
||||
with _profile_scope(body.profile or profile):
|
||||
# Same RMW span as PUT /api/config: applyMainModel fires this while
|
||||
# the settings-page autosave is in flight — hold the mutation lock.
|
||||
with config_write_scope(body.profile or profile):
|
||||
return _apply_model_assignment_sync(
|
||||
scope, provider, model, task, base_url, api_key, reasoning_effort=reasoning_effort)
|
||||
|
||||
|
||||
@@ -33,6 +33,7 @@ from hermes_cli.web_server_config import (
|
||||
_apply_main_model_assignment, _normalize_main_model_assignment, _validated_main_model_selection,
|
||||
)
|
||||
from hermes_cli.web_server_gateway import _strip_session_list_rows
|
||||
from hermes_cli.web_routers._common import _CONFIG_MUTATION_LOCK
|
||||
from hermes_cli.web_server_profiles import (
|
||||
_fallback_profile_dicts, _hub_action_name, _write_profile_mcp_servers,
|
||||
)
|
||||
@@ -110,7 +111,7 @@ def _write_profile_model(profile_dir: Path, provider: str, model: str, validate_
|
||||
with _hermes_home_scope(validate_in or profile_dir):
|
||||
provider, model = _normalize_main_model_assignment(provider, model)
|
||||
result = _validated_main_model_selection(load_config(), provider, model)
|
||||
with _hermes_home_scope(profile_dir):
|
||||
with _hermes_home_scope(profile_dir), _CONFIG_MUTATION_LOCK: # RMW span
|
||||
cfg = load_config()
|
||||
cfg["model"] = _apply_main_model_assignment(cfg.get("model", {}), result)
|
||||
save_config(cfg)
|
||||
|
||||
@@ -0,0 +1,111 @@
|
||||
"""Concurrent dashboard config writers must not drop each other's mutations.
|
||||
|
||||
Only ``PUT /api/config`` used to hold ``_CONFIG_MUTATION_LOCK``. ``POST /api/model/set``,
|
||||
``PUT /api/model/moa``, the custom-endpoint handlers and the memory-provider saves ran their
|
||||
load→mutate→save cycles unlocked on worker threads, so a model assignment racing the desktop's
|
||||
debounced whole-record autosave interleaved as::
|
||||
|
||||
T1 load (model=A) T2 load (model=A)
|
||||
T1 mutate model=B T2 mutate display.x
|
||||
T1 save (model=B) T2 save (model=A + display.x) <- T1's write erased
|
||||
|
||||
Both tests provoke that interleaving with a slowed ``save_config`` and assert both writes land.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import threading
|
||||
import time
|
||||
|
||||
import pytest
|
||||
import yaml
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client(monkeypatch, _isolate_hermes_home):
|
||||
from starlette.testclient import TestClient
|
||||
|
||||
from hermes_cli.config import load_config, save_config
|
||||
from hermes_cli.web_server import _SESSION_HEADER_NAME, _SESSION_TOKEN, app
|
||||
|
||||
monkeypatch.setattr("hermes_cli.model_cost_guard.expensive_model_warning", lambda *_a, **_k: None)
|
||||
cfg = load_config()
|
||||
cfg["model"] = {"provider": "openrouter", "default": "openai/gpt-5.5"}
|
||||
save_config(cfg)
|
||||
|
||||
client = TestClient(app)
|
||||
client.headers[_SESSION_HEADER_NAME] = _SESSION_TOKEN
|
||||
return client
|
||||
|
||||
|
||||
def _slow_saves(monkeypatch, delay: float = 0.2) -> None:
|
||||
"""Widen the load→save window so an unlocked pair reliably loses a write."""
|
||||
import hermes_cli.config as cfg_mod
|
||||
|
||||
real_save = cfg_mod.save_config
|
||||
|
||||
def slow_save(config, *args, **kwargs):
|
||||
time.sleep(delay)
|
||||
return real_save(config, *args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(cfg_mod, "save_config", slow_save)
|
||||
|
||||
|
||||
def _race(*calls):
|
||||
results: list = [None] * len(calls)
|
||||
|
||||
def run(i, fn):
|
||||
results[i] = fn()
|
||||
|
||||
threads = [threading.Thread(target=run, args=(i, fn)) for i, fn in enumerate(calls)]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join(timeout=30)
|
||||
return results
|
||||
|
||||
|
||||
def _model_block(home) -> dict:
|
||||
return yaml.safe_load((home / "config.yaml").read_text(encoding="utf-8"))
|
||||
|
||||
|
||||
def test_model_set_racing_config_autosave_keeps_both_writes(client, monkeypatch, _isolate_hermes_home):
|
||||
"""``applyMainModel`` (POST /api/model/set) while the settings autosave (PUT /api/config) is
|
||||
in flight: the model assignment AND the autosaved field both survive."""
|
||||
_slow_saves(monkeypatch)
|
||||
|
||||
set_model = lambda: client.post( # noqa: E731
|
||||
"/api/model/set", json={"scope": "main", "provider": "openrouter", "model": "anthropic/claude-sonnet-4"})
|
||||
autosave = lambda: client.put( # noqa: E731
|
||||
"/api/config", json={"config": {"display": {"personality": "canary"}}})
|
||||
|
||||
r1, r2 = _race(set_model, autosave)
|
||||
assert r1.status_code == 200, r1.text
|
||||
assert r2.status_code == 200, r2.text
|
||||
|
||||
on_disk = _model_block(_isolate_hermes_home)
|
||||
assert on_disk["model"]["default"] == "anthropic/claude-sonnet-4"
|
||||
assert on_disk["display"]["personality"] == "canary"
|
||||
|
||||
|
||||
def test_custom_endpoint_upsert_racing_moa_save_keeps_both_writes(client, monkeypatch, _isolate_hermes_home):
|
||||
"""Two sync-def writers on worker threads (custom-endpoint upsert vs MoA save) serialize
|
||||
through the same lock — neither top-level section is lost."""
|
||||
_slow_saves(monkeypatch)
|
||||
|
||||
upsert = lambda: client.post( # noqa: E731
|
||||
"/api/providers/custom-endpoints",
|
||||
json={"id": "racebox", "name": "racebox", "base_url": "http://racebox:8000/v1", "model": "race-model",
|
||||
"discover_models": False})
|
||||
moa = lambda: client.put( # noqa: E731
|
||||
"/api/model/moa",
|
||||
json={"reference_models": [{"provider": "openrouter", "model": "openai/gpt-5.5"}],
|
||||
"aggregator": {"provider": "openrouter", "model": "openai/gpt-5.5"}})
|
||||
|
||||
r1, r2 = _race(upsert, moa)
|
||||
assert r1.status_code == 200, r1.text
|
||||
assert r2.status_code == 200, r2.text
|
||||
|
||||
on_disk = _model_block(_isolate_hermes_home)
|
||||
assert "racebox" in on_disk["providers"]
|
||||
assert on_disk["moa"]["aggregator"]["model"] == "openai/gpt-5.5"
|
||||
Reference in New Issue
Block a user