watchsettings.c: D350-bridge robustness - hold-not-drop at stage transitions, chooser spinlock, queue coalescing, warning logs (D352)

docs/dev/findings-index.csv

@@ -229,3 +229,4 @@ D348,OPEN,"File-select ""PC Options"" label:",Port-only (frontoptions.c):
 D349,PARTIAL,GE watch settings are not normally saved to the Bond file:,ANALYZED (static decomp audit):
 D350,PARTIAL,Watch-backed PC options:,"PARTIAL (implemented; user reports tests good, review pending):"
 D351,FIXED,Existing D346 value/unit rows had positional initializers shifted into hidePtr:,"FIXED (port-only): designated row initializers restore unit, deadzone divisor and hide-if-auto fields; build clean, headless smoke clean."
+D352,FIXED,D350 review of 1303c0eb:,"FIXED (port-only, port/src/watchsettings.c):"

docs/dev/findings.md

@@ -637,6 +637,7 @@ covers D24–D69; the log continues in §H (D32 procedure, D70–D121).
 | D349 | **GE watch settings are not normally saved to the Bond file: erase is the only caller of the settings serializer, and stage start reloads the file's old values.** — full `## D349` entry at file tail | ANALYZED (static decomp audit): maintainer approved PC-UI-only per-file persistence with explicit chooser, stored controller-type preservation and release-only slider writes; watch UI stays session-only. Erase cross-file edge noted. See `docs/dev/WATCH-SETTINGS-PLAN.md`. |
 | D350 | **Watch-backed PC options: shared front/F10 Bond-file rows, explicit chooser, game-thread F10 bridge and field-scoped PC-only persistence.** — full `## D350` entry at file tail | PARTIAL (implemented; user reports tests good, review pending): headless front + F10 write/reload probes, NTSC build/golden 3/3, save CRC/control-style preserved; two-file targeting checked. Detailed edge-case coverage not specified by user. |
 | D351 | **Existing D346 value/unit rows had positional initializers shifted into `hidePtr`: %/x rows hidden or formatted without units.** — full `## D351` entry at file tail | FIXED (port-only): designated row initializers restore unit, deadzone divisor and hide-if-auto fields; build clean, headless smoke clean. Manual settings visual check owed. |
+| D352 | **D350 review of 1303c0eb: stage transitions silently drop queued F10 setting changes (queue drained before the `stageActive` check); the UI thread's chooser state is unsynchronized; rapid slider drags push one RSP command per detent and re-persist the same value each frame; persist failures are silent (2026-09-27).** — full `## D352` entry at file tail | FIXED (port-only, `port/src/watchsettings.c`): hold-not-drop at stage transitions, spinlock on chooser state + front-screen guard, per-field coalesced queue commits, warning logs on all no-op paths, `PersistField` made static. NTSC build clean, headless menu smoke clean. |
 
 Phase 2 replaced the Phase-1 demo loop with the real `mainproc()` on real OS
 threads, compiled GE's real `src/sched.c`, and brought in PD's fast3d software
@@ -13794,3 +13795,14 @@ to the user (proportions/knob offsets are one-constant tweaks if it looks off).
 ## D351 — D346 unit-row initializers shifted into `hidePtr` (2026-09-27)
 
 The existing positional `struct Row` initializers for FOV, anisotropy, draw/LOD distance, HUD scale, deadzone and trigger threshold omitted the `hidePtr` placeholder. GCC warned about assigning a `char *` unit ("%"/"x") to `int *hidePtr` and assigning deadzone's numeric display divisor to `unit`. The visible-list builder dereferences non-null hidePtr, so these rows were hidden or misformatted despite D346's intended unit labels. Corrected *only* these rows to designated initializers, preserving their key, label, clamp, step and intended `hiddenIfOn`/unit/divisor. New watch rows also use designated initializers to prevent repeats. Build has no new optionsoverlay warnings; headless menu smoke/golden gate clean. Maintainer reports the implementation tests good, but did not separately report a unit-label/auto-hide visual check; carry that into the new-session review.
+
+## D352 — D350 watch-settings bridge review: transition drops, chooser sync, queue coalescing, silent failures (2026-09-27)
+
+Design review of `1303c0eb` (D350) of the game-thread bridge in `port/src/watchsettings.c`. Four robustness fixes, all port-side, behavior-neutral:
+
+1. **M1 — hold-not-drop at stage transitions.** `watchSettingsGameTick` drained the queue *before* checking `stageActive()`, so any setting change committed while a stage transition was in progress was silently discarded. Now the stage-active check runs first: while unavailable the queue is held (with coalescing, so only the latest value per field is ever pending) and applies as soon as the stage reactivates. The drain still happens before the `gfxFrameMsgQ` spin, so the bridge can never deadlock on its own lock ordering.
+2. **M2 — chooser state synchronized.** `chosen`/`stagedVal`/`stagedValid` are mutated by `watchSettingsChooseFile` (UI thread, both surfaces) and read by the bridge (game thread). A single `SDL_SpinLock uiLock` now guards the whole read/modify/write on both sides, and the chooser is a no-op unless the UI is at the front end (`frontScreen()`), where the chooser row is the only caller. The lock is short-critical-section-only (no I/O held), taken in one order (never with `qLock` held).
+3. **L2 — per-field coalescing.** A fast slider drag queued one `WsCmd` (and one `wsPersist`, i.e. one `fileWriteSave`) per detent per frame. The queue now keeps at most one pending command per field: a new commit for an already-pending field replaces its value, ORs its `commits` flag, and reuses the slot. `fileWriteSave` still happens at most once per tick (the drain loop is unchanged), but the per-detent spam is gone and a stage transition can never build up an unbounded queue.
+4. **L3/L6 — visible failures, smaller API.** Every silent no-op path now logs a one-line `LOG_WARNING` (`frontoptions`/`watchSettings`-tagged): stage transition holding the queue (rate-limited by the coalescing), front-path commit with nothing staged, F10 commit with no valid save, no loaded player record, save absent from the EEPROM range. `watchSettingsPersistField` was only ever called inside `watchsettings.c` — made `static`, removed from the public header.
+
+Verification: NTSC build clean (no new warnings in `watchsettings.c`), headless menu smoke boots to the front end and renders (frames advancing, EEPROM load). Edge cases (setting F10 during a level transition; rapid slider drag; committing with an invalid file) are log-visible but need a manual pass to confirm by eye.