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.
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:
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.
## 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
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.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
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:If the
RwLockis poisoned, theif letsimply does not run. The database write already succeeded,self.preferencesis updated, and a few lines later (:468-471)preferences_open = falseandpreference_error = None— so the dialog closes and the user is told it worked. The sharedArc<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-34and: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, plussrc/server/tools.rs:413,:444,:571—if let Ok(...)/.ok(), which silently skips the work. Intools.rsthe consequence is a lost tool-call replay entry (degraded, recoverable). Inpreferences.rsit 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:
../ds4is C and uses rawpthread_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 aResult.The transferable observation is what ds4 does with the data, not the lock: state guarded by
tool_muis 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 thenative_edit.rsconvention: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 (keepserver.rsas-is). Then the remainingif let Oksites insrc/server/tools.rsshould either becomeinto_inner()or get a one-line comment saying the skip is intentional and harmless.Optional: a brief note in
AGENTS.mdrecording the convention, since this is exactly the kind of thing that drifts again.Acceptance criteria
runtime_preferencesunconditionally; 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.if let Ok(...) = <lock>.write()in a path where failure is user-visible-as-success.cargo clippy --all-targets --all-features -- -D warningsstays clean.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.