Preference save silently discards the runtime update on a poisoned lock; poisoning conventions are inconsistent #17

New Issue

hugo · 2026-07-25T09:42:25Z

hugo commented

2026-07-25 09:42:25 +00:00

Symptom

Lock-poisoning is handled three different ways in this codebase. One of them silently discards a user action: saving preferences reports success in the UI, but the HTTP server keeps serving the old settings forever.

Evidence

The bug. src/app/preferences.rs:457-462:

Ok(preferences) => {
    self.preferences = preferences;
    #[cfg(target_os = "macos")]
    if let Ok(mut runtime_preferences) = self.runtime_preferences.write() {
        *runtime_preferences = self.preferences.clone();
    }

If the RwLock is poisoned, the if let simply does not run. The database write already succeeded, self.preferences is updated, and a few lines later (:468-471) preferences_open = false and preference_error = None — so the dialog closes and the user is told it worked. The shared Arc<RwLock<AppPreferences>> that the HTTP server reads from (src/server.rs:341, :464, src/server/request.rs:489) is now permanently stale, and every subsequent save takes the same silent path.

The three inconsistent conventions:

src/native_edit.rs:32-34 and :39-41 — correct: .lock().unwrap_or_else(|poisoned| poisoned.into_inner()). Recovers the data and carries on.
src/server.rs:341-342, :464-465 — deliberate and defensible: .read().map_or(32_768, |preferences| preferences.context_tokens), a documented-by-shape fallback for a read.
src/app/preferences.rs:460, plus src/server/tools.rs:413, :444, :571 — if let Ok(...) / .ok(), which silently skips the work. In tools.rs the consequence is a lost tool-call replay entry (degraded, recoverable). In preferences.rs it is a lost user setting with a success message.

A poisoned lock here means some other thread panicked while holding it — which, given the runtime-panic issue filed separately, is a state this codebase can actually reach.

How ds4 handles this

No direct analogue: ../ds4 is C and uses raw pthread_mutex_t (ds4_server.c:8279-8298 — tool_mu, kv_mu, inference_mu, model_mu, trace_mu), which have no poisoning concept. A C thread that dies holding a mutex deadlocks the next acquirer rather than handing it a Result.

The transferable observation is what ds4 does with the data, not the lock: state guarded by tool_mu is always read back through accessors that return a definite value (ds4_server.c:8543-8663), and configuration is snapshotted into the job at queue time rather than re-read per use — there is no path where a failed lock acquisition silently means "keep the old configuration and tell the user it changed". That is the property to preserve.

Suggested fix

src/app/preferences.rs:460 — use the native_edit.rs convention:
```
*self.runtime_preferences.write().unwrap_or_else(|p| p.into_inner()) = self.preferences.clone();
```
A poisoned lock holding preferences is not a reason to lose preferences.
Pick one convention and apply it. Recommendation: into_inner() everywhere for writes, map_or(default, ...) for reads with a genuinely sensible default (keep server.rs as-is). Then the remaining if let Ok sites in src/server/tools.rs should either become into_inner() or get a one-line comment saying the skip is intentional and harmless.
Optional: a brief note in AGENTS.md recording the convention, since this is exactly the kind of thing that drifts again.

Acceptance criteria

Saving preferences updates runtime_preferences unconditionally; verifiable by a test that poisons the lock (spawn a thread that panics while holding the write guard), then saves preferences and asserts the shared value changed.
No remaining if let Ok(...) = <lock>.write() in a path where failure is user-visible-as-success.
cargo clippy --all-targets --all-features -- -D warnings stays clean.

## Symptom Lock-poisoning is handled three different ways in this codebase. One of them silently discards a user action: saving preferences reports success in the UI, but the HTTP server keeps serving the old settings forever. ## Evidence **The bug.** `src/app/preferences.rs:457-462`: ```rust Ok(preferences) => { self.preferences = preferences; #[cfg(target_os = "macos")] if let Ok(mut runtime_preferences) = self.runtime_preferences.write() { *runtime_preferences = self.preferences.clone(); } ``` If the `RwLock` is poisoned, the `if let` simply does not run. The database write already succeeded, `self.preferences` is updated, and a few lines later (`:468-471`) `preferences_open = false` and `preference_error = None` — so the dialog closes and the user is told it worked. The shared `Arc<RwLock<AppPreferences>>` that the HTTP server reads from (`src/server.rs:341`, `:464`, `src/server/request.rs:489`) is now permanently stale, and every subsequent save takes the same silent path. **The three inconsistent conventions:** 1. `src/native_edit.rs:32-34` and `:39-41` — correct: `.lock().unwrap_or_else(|poisoned| poisoned.into_inner())`. Recovers the data and carries on. 2. `src/server.rs:341-342`, `:464-465` — deliberate and defensible: `.read().map_or(32_768, |preferences| preferences.context_tokens)`, a documented-by-shape fallback for a read. 3. `src/app/preferences.rs:460`, plus `src/server/tools.rs:413`, `:444`, `:571` — `if let Ok(...)` / `.ok()`, which silently skips the work. In `tools.rs` the consequence is a lost tool-call replay entry (degraded, recoverable). In `preferences.rs` it is a lost user setting with a success message. A poisoned lock here means some other thread panicked while holding it — which, given the runtime-panic issue filed separately, is a state this codebase can actually reach. ## How ds4 handles this No direct analogue: `../ds4` is C and uses raw `pthread_mutex_t` (`ds4_server.c:8279-8298` — `tool_mu`, `kv_mu`, `inference_mu`, `model_mu`, `trace_mu`), which have no poisoning concept. A C thread that dies holding a mutex deadlocks the next acquirer rather than handing it a `Result`. The transferable observation is what ds4 does with *the data*, not the lock: state guarded by `tool_mu` is always read back through accessors that return a definite value (`ds4_server.c:8543-8663`), and configuration is snapshotted into the job at queue time rather than re-read per use — there is no path where a failed lock acquisition silently means "keep the old configuration and tell the user it changed". That is the property to preserve. ## Suggested fix 1. **`src/app/preferences.rs:460`** — use the `native_edit.rs` convention: ```rust *self.runtime_preferences.write().unwrap_or_else(|p| p.into_inner()) = self.preferences.clone(); ``` A poisoned lock holding preferences is not a reason to lose preferences. 2. **Pick one convention and apply it.** Recommendation: `into_inner()` everywhere for writes, `map_or(default, ...)` for reads with a genuinely sensible default (keep `server.rs` as-is). Then the remaining `if let Ok` sites in `src/server/tools.rs` should either become `into_inner()` or get a one-line comment saying the skip is intentional and harmless. 3. Optional: a brief note in `AGENTS.md` recording the convention, since this is exactly the kind of thing that drifts again. ## Acceptance criteria - Saving preferences updates `runtime_preferences` unconditionally; verifiable by a test that poisons the lock (spawn a thread that panics while holding the write guard), then saves preferences and asserts the shared value changed. - No remaining `if let Ok(...) = <lock>.write()` in a path where failure is user-visible-as-success. - `cargo clippy --all-targets --all-features -- -D warnings` stays clean.

hugo added the bug label 2026-07-25 09:42:25 +00:00

hugo commented

2026-07-25 11:13:05 +00:00

Fixed in c77ef47. Preference saves now recover poisoned write locks with into_inner(), so a successful database/UI save always updates the runtime snapshot. The three tool-replay lock sites now use the same recovery convention instead of silently losing state. Added a regression test that poisons the preference lock, updates it, and verifies the shared value changed. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.

Fixed in c77ef47. Preference saves now recover poisoned write locks with into_inner(), so a successful database/UI save always updates the runtime snapshot. The three tool-replay lock sites now use the same recovery convention instead of silently losing state. Added a regression test that poisons the preference lock, updates it, and verifies the shared value changed. Verified with cargo fmt, Clippy (-D warnings), make bundle, and cargo test --all-features.

hugo closed this issue

2026-07-25 11:13:14 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: hugo/DS4Server#17