mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
cad5e50f10
* feat(llm): hot-reload provider chain from settings (#1350) Adds SwappableLlmProvider and LlmReloadHandle so changes to the active LLM backend/model via the settings API take effect without restarting the daemon. The settings handlers trigger a chain rebuild from the latest Config::from_db_with_toml whenever an LLM-relevant key is written, and atomically swap the inner provider under the running wrappers. Addresses review feedback on the original PR #2059 (superseded): - single RwLock<ProviderSnapshot> for atomic metadata updates (no torn reads across model_name / cost / cache multipliers) - interned &'static str for model_name() to cap Box::leak at the set of distinct names a process ever sees, not one leak per swap - single critical section around swap+snapshot refresh to kill the race between concurrent reloads - tokio::sync::Mutex on LlmReloadHandle to serialize reloads and avoid overlapping OAuth refreshes / HTTP probes - warn!, not silent Ok, when reload wiring is missing from the gateway state - integration coverage per .claude/rules/testing.md: a test that drives settings_set_handler end-to-end and asserts the same Arc<dyn LlmProvider> reports the new active_model_name after swap Co-authored-by: Nigel Coleman <coleman.nige@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm): gate hot-reload on scope + admin; re-hydrate secrets Addresses review findings on #2673: - Scope gate: reload only fires when the written scope actually feeds the global provider chain (admin scope or gateway owner scope). A member writing their own `selected_model` lands in their user row but no longer triggers a chain rebuild that would read back from a different scope — fixing both the "write ignored by reload" bug and the DoS vector where any authed user could force expensive rebuilds. - Admin-only provider selection: `llm_backend` and `bedrock_{region, cross_region, profile}` join the existing admin-only LLM key list, matching the product directive "admins choose the provider, members pick the model within it". `selected_model` stays non-admin so every user can change their own model. - Secret re-hydration on reload: `reload_llm_after_settings_change` now calls `re_resolve_llm_with_secrets` after the bare `from_db_with_toml` read, so a new OPENAI_API_KEY / NEARAI_SESSION_TOKEN added alongside a backend switch is visible to the rebuilt chain. - Style cleanup: drop dead `llm_model` allowlist entry; drop unused `Clone` on `ProviderSnapshot`; document `reload_lock`'s purpose; explicit comment that `active_config.enabled_channels` is not refreshed (channel enablement is orthogonal to LLM config). New regression tests (5149 → 5154 passing): - `llm_reload_handle_preserves_old_chain_on_build_failure` — a failed reload leaves the primary wrapper pointing at the old chain. - `settings_set_handler_rejects_member_writing_llm_backend` — member writing `llm_backend` gets 403 (admin-only). - `settings_set_handler_member_selected_model_skips_reload` — member can set their own model, and it does NOT trigger a global reload. - `settings_set_handler_owner_scope_triggers_reload` — owner writing their own scope (no `scope=admin`) still reloads the chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm): decouple reload from HTTP status; atomic set_model; cap interner Addresses PR #2673 review comments from Copilot and gemini-code-assist: - **Reload failure no longer 500s the setting write** (Copilot): `reload_llm_after_settings_change` is now infallible — it logs at `error!` when the chain rebuild fails but the handler still returns 204. Returning 500 after a successful `set_setting` misrepresented the outcome (DB committed, chain stale) and drove client retries that re-ran the same failing reload. - **set_model race with swap closed** (gemini): the write lock is now held across the inner `set_model` call and the snapshot refresh, so a concurrent `swap()` can't clobber the just-updated inner with a snapshot of the older one. - **Interner leak capped** (gemini): `intern_model_name` now refuses names longer than 256 bytes and caps distinct entries at 1024, past either limit returning a static `<model-name-overflow>` sentinel and logging at `warn!`. Protects against adversarial `set_model` input. New regression tests (5154 passing): - `settings_set_handler_returns_success_when_reload_fails` — admin switches backend to a value with no credentials; handler returns 204, DB has the new value, old chain still serving. - `set_model_and_swap_are_mutually_atomic` — concurrent set_model + swap stress; final wrapper is readable and consistent. - `intern_into_rejects_oversized_input` — oversized name never leaks, returns sentinel without touching the map. - `intern_into_caps_distinct_entries` — past the cap, sentinel; already-interned names still resolve. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm): load reload config from owner scope; rollback on build failure Addresses PR #2673 review comments from @serrrfirat and Copilot. - **Reload scope fix** (serrrfirat, Copilot): `reload_llm_after_settings_change` now reads with `state.owner_id` instead of the just-written `effective_user_id`. `Config::from_db_with_toml` skips the admin-merge step when `user_id == __admin__`, so reloading at admin scope was dropping owner-scope overlays that startup normally applies. Using `state.owner_id` matches `AppBuilder::init_config` and keeps the layering consistent. - **Rollback on reload failure** (serrrfirat): handlers now snapshot the affected keys before the DB write and restore them if the chain rebuild returns `ConfigLoadFailed` or `BuildFailed`. The handler then returns 422 with the rolled-back state. This closes the split-brain window where a bad `llm_backend=openai` write could leave the DB saying "openai" while the runtime kept serving "nearai". `set_setting`, `delete_setting`, and `set_all_settings` all participate. - **`ReloadOutcome` enum** replaces the previous infallible return, so callers can distinguish transient "nothing wired" (skip) from actual "chain rebuild failed" (roll back) outcomes. Regression tests (5184 passing): - `reload_rebuilds_from_owner_scope_not_effective_scope` — pre-seeds an owner-scope `selected_model` overlay and has admin write a benign key under `scope=admin`. Assertion: after reload, the wrapper reports the owner's overlay, not admin's default. This fails pre-fix because reading at `__admin__` scope silently skipped the admin merge. - `settings_set_handler_rolls_back_on_reload_failure` — pokes a poisoned `bedrock_cross_region` sibling into admin scope, triggers a handler write, asserts 422 and that the DB is back to its pre-request state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(llm): fail-loud on snapshot read errors; surface 422 reason in body Addresses Copilot review comments on PR #2673. - **Snapshot read errors** (c5, c6): `settings_{set,delete,import}_handler` used to `.unwrap_or(None)` when reading the previous value for rollback, which would silently treat a DB read failure as "no prior value" and turn a later rollback into `delete_setting` on a key whose prior value we couldn't actually read — a silent data-loss path. The handlers now map snapshot read errors to 500 and abort before persisting. The import handler does the same inside its per-key snapshot loop. - **422 body carries the reason** (c7): the `ReloadOutcome::BuildFailed` and `ConfigLoadFailed` reason strings are now included in the 422 response body. Handler error type changed from `Result<StatusCode, StatusCode>` to `Result<StatusCode, (StatusCode, String)>` (axum's `IntoResponse` for tuples). The web UI's `apiFetch` can surface the reason to the operator instead of a bare "Unprocessable Entity". Auth/validation paths keep empty-body semantics via the `no_body` helper. Test updates: - Existing handler tests: `.0` on the error tuple where they previously compared bare `StatusCode`. - Extended `settings_set_handler_rolls_back_on_reload_failure` to assert the 422 body includes the failure reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Nigel Coleman <coleman.nige@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>