fix(persistence): stop rejected UI trailing flush loops (#17378)

* test(sta-5938): pin persisted UI rejection retry behavior

* fix(persistence): bound rejected UI trailing flushes

src/renderer/src/app-shell/use-persisted-ui-writer.ts

@@ -1,4 +1,4 @@
-import { useEffect } from 'react'
+import { useEffect, useMemo } from 'react'
 import { useShallow } from 'zustand/react/shallow'
 import { useAppStore } from '../store'
 import {
@@ -19,65 +19,111 @@ import {
  * next change (no automatic retry loop) — which is why this sends through
  * setWithAck: the web preload's plain set swallows transport failures.
  */
-function sendPersistedUIWrite(changed: Partial<PersistedUIWriteBaseline>): void {
-  const fields = Object.keys(changed) as (keyof PersistedUIWriteBaseline)[]
-  const state = useAppStore.getState()
-  const sentAtGeneration = state.persistedUIWriteBaselineGeneration
-  state.notePersistedUIWriteStarted(fields)
-  let request: Promise<void>
-  try {
-    // setWithAck rejects when the host did not apply the patch (web's plain set
-    // swallows transport failures); older preloads without it fall back to set.
-    const send = window.api.ui.setWithAck ?? window.api.ui.set
-    request = send(persistedUIWriteFieldsToWireUpdate(changed))
-  } catch {
-    // A synchronous throw (e.g. a non-cloneable value) must still settle the
-    // in-flight marker, or the field stays pinned against hydration forever.
-    useAppStore.getState().notePersistedUIWriteSettled(fields, null)
-    scheduleTrailingPersistedUIFlush()
-    return
-  }
-  // Two-arg then: the rejection handler must not catch throws from the ack
-  // handler, which would double-settle these fields and leak the trailing ones.
-  request.then(
-    () => {
-      useAppStore.getState().notePersistedUIWriteSettled(fields, changed, { sentAtGeneration })
-      scheduleTrailingPersistedUIFlush()
-    },
-    () => {
-      useAppStore.getState().notePersistedUIWriteSettled(fields, null)
-      // A trailing pass skipped because THIS write was in flight must still
-      // happen — a rejection schedules nothing on its own, and a pending
-      // flip-back would otherwise be stranded until the next edit.
-      scheduleTrailingPersistedUIFlush()
-    }
-  )
+type PersistedUIWriteController = {
+  activate: () => void
+  send: (changed: Partial<PersistedUIWriteBaseline>) => void
+  scheduleTrailing: () => void
+  dispose: () => void
 }
 
-/**
- * Debounced (never inline at ack rate — one in-flight write must not turn a
- * drag into one ui.set per IPC round trip) re-diff of the mirror against the
- * settled baseline. Skips while any write is still in flight: that write's
- * own ack schedules the next pass, so overlapping timers can't double-send.
- * Termination invariant: each acked write folds exactly the values it diffed
- * (or a hydration advances the baseline), so the trailing diff shrinks to
- * empty; without the fold this would loop.
- */
-function scheduleTrailingPersistedUIFlush(): void {
-  window.setTimeout(() => {
+function createPersistedUIWriteController(): PersistedUIWriteController {
+  let disposed = false
+  let trailingTimer: number | null = null
+
+  const scheduleTrailing = (): void => {
+    if (disposed) {
+      return
+    }
+    if (trailingTimer !== null) {
+      window.clearTimeout(trailingTimer)
+    }
+    trailingTimer = window.setTimeout(() => {
+      trailingTimer = null
+      if (disposed) {
+        return
+      }
+      const state = useAppStore.getState()
+      if (Object.keys(state.persistedUIWriteInFlightCounts).length > 0) {
+        return
+      }
+      const baseline = state.persistedUIWriteBaseline
+      if (!baseline) {
+        return
+      }
+      const trailing = diffPersistedUIWriteFields(capturePersistedUIWriteBaseline(state), baseline)
+      if (Object.keys(trailing).length > 0) {
+        controller.send(trailing)
+      }
+    }, 150)
+  }
+
+  const send = (changed: Partial<PersistedUIWriteBaseline>): void => {
+    if (disposed) {
+      return
+    }
+    const fields = Object.keys(changed) as (keyof PersistedUIWriteBaseline)[]
     const state = useAppStore.getState()
-    if (Object.keys(state.persistedUIWriteInFlightCounts).length > 0) {
+    const sentAtGeneration = state.persistedUIWriteBaselineGeneration
+    state.notePersistedUIWriteStarted(fields)
+    let request: Promise<void>
+    try {
+      // setWithAck rejects when the host did not apply the patch (web's plain set
+      // swallows transport failures); older preloads without it fall back to set.
+      const send = window.api.ui.setWithAck ?? window.api.ui.set
+      request = send(persistedUIWriteFieldsToWireUpdate(changed))
+    } catch {
+      // A synchronous throw (e.g. a non-cloneable value) must still settle the
+      // in-flight marker, or the field stays pinned against hydration forever.
+      useAppStore.getState().notePersistedUIWriteSettled(fields, null)
       return
     }
-    const baseline = state.persistedUIWriteBaseline
-    if (!baseline) {
-      return
+    // Two-arg then: the rejection handler must not catch throws from the ack
+    // handler, which would double-settle these fields and leak the trailing ones.
+    request.then(
+      () => {
+        useAppStore.getState().notePersistedUIWriteSettled(fields, changed, { sentAtGeneration })
+        scheduleTrailing()
+      },
+      () => {
+        useAppStore.getState().notePersistedUIWriteSettled(fields, null)
+        // Preserve a pending edit made during this round trip, but don't retry the
+        // rejected patch itself: a terminal transport failure must not loop.
+        const state = useAppStore.getState()
+        const baseline = state.persistedUIWriteBaseline
+        const dirty = baseline
+          ? diffPersistedUIWriteFields(capturePersistedUIWriteBaseline(state), baseline)
+          : {}
+        const current = capturePersistedUIWriteBaseline(state)
+        const changedDuringFlight = fields.some(
+          (field) => !Object.is(current[field], changed[field])
+        )
+        const shouldRetry =
+          changedDuringFlight ||
+          Object.keys(dirty).some(
+            (field) => !fields.includes(field as keyof PersistedUIWriteBaseline)
+          )
+        if (shouldRetry) {
+          scheduleTrailing()
+        }
+      }
+    )
+  }
+
+  const controller: PersistedUIWriteController = {
+    activate: () => {
+      disposed = false
+    },
+    send,
+    scheduleTrailing,
+    dispose: () => {
+      disposed = true
+      if (trailingTimer !== null) {
+        window.clearTimeout(trailingTimer)
+        trailingTimer = null
+      }
     }
-    const trailing = diffPersistedUIWriteFields(capturePersistedUIWriteBaseline(state), baseline)
-    if (Object.keys(trailing).length > 0) {
-      sendPersistedUIWrite(trailing)
-    }
-  }, 150)
+  }
+  return controller
 }
 
 /**
@@ -93,6 +139,7 @@ function scheduleTrailingPersistedUIFlush(): void {
  * same 150ms save (#8265), so every top-level view switch scheduled a full durable-state write.
  */
 export function usePersistedUIWriter(): void {
+  const controller = useMemo(() => createPersistedUIWriteController(), [])
   const persistedUIReady = useAppStore((s) => s.persistedUIReady)
   const activeView = useAppStore((s) => s.activeView)
   const ui = useAppStore(
@@ -123,6 +170,10 @@ export function usePersistedUIWriter(): void {
       acknowledgedAgentsByPaneKey: s.acknowledgedAgentsByPaneKey
     }))
   )
+  useEffect(() => {
+    controller.activate()
+    return () => controller.dispose()
+  }, [controller])
   useEffect(() => {
     // The baseline holds the values this client last saw persisted (newest
     // hydration from main, overlaid with this client's flushed writes); fields
@@ -150,11 +201,11 @@ export function usePersistedUIWriter(): void {
       if (Object.keys(changed).length === 0) {
         return
       }
-      sendPersistedUIWrite(changed)
+      controller.send(changed)
     }, 150)
 
     return () => window.clearTimeout(timer)
-  }, [persistedUIReady, ui])
+  }, [controller, persistedUIReady, ui])
 
   // Why (#9002): activeView has its own tiny profile preference, so it can track
   // every switch without scheduling the multi-MB durable-state writer.

src/renderer/src/app-shell/workspace-view-cross-client-sync.test.tsx

@@ -31,6 +31,10 @@ import {
 } from '../../../main/persistence/applying-settings/ui-state-update'
 import type { AppState } from '../store/types'
 import { createUIStore } from '../store/slices/ui-slice-test-harness'
+import {
+  capturePersistedUIWriteBaseline,
+  diffPersistedUIWriteFields
+} from '../store/slices/persisted-ui-write-baseline'
 import { usePersistedUIWriter } from './use-persisted-ui-writer'
 
 const storeRef = vi.hoisted(() => ({
@@ -197,8 +201,10 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () =
   let pendingBroadcasts: PersistedUIState[]
   let holdAcks: boolean
   let rejectSets: boolean
+  let rejectNextSet: boolean
   let setCallCount: number
   let pendingAcks: (() => void)[]
+  let pendingRejects: ((reason?: unknown) => void)[]
 
   async function resolveAcks() {
     await act(async () => {
@@ -249,8 +255,10 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () =
     storeRef.current = store as unknown as typeof storeRef.current
     holdAcks = false
     rejectSets = false
+    rejectNextSet = false
     setCallCount = 0
     pendingAcks = []
+    pendingRejects = []
     ;(window as unknown as { api: unknown }).api = {
       ui: {
         set: (updates: Partial<PersistedUIState>) => {
@@ -259,6 +267,10 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () =
           if (rejectSets) {
             return Promise.reject(new Error('transport failure'))
           }
+          if (rejectNextSet) {
+            rejectNextSet = false
+            return new Promise<void>((_, reject) => pendingRejects.push(reject))
+          }
           // Like the real IPC: main applies the update before the renderer's
           // promise resolves; holdAcks models the in-flight round-trip window.
           authority.set(updates)
@@ -516,7 +528,132 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () =
     expect(authority.get().hideCliCreatedWorkspaces).toBe(true)
   })
 
-  it('a synchronously throwing ui.set still settles the marker and reschedules', async () => {
+  it('a terminal rejection leaves one dirty field without an automatic retry loop', async () => {
+    // A rejected transport write is terminal for this attempt. The mirror must
+    // stay dirty for an explicit later edit, but the rejection itself must not
+    // keep scheduling 150ms trailing writes forever.
+    rejectSets = true
+    setCallCount = 0
+    act(() => {
+      store.getState().setHideDefaultBranchWorkspace(true)
+    })
+
+    vi.advanceTimersByTime(150)
+    // Let the controlled rejection settle and (on the buggy writer) arm its
+    // next trailing timer, without allowing that timer to fire in this tick.
+    await Promise.resolve()
+
+    // Independent signals: exactly one transport call and no trailing timer
+    // remain after its rejection settles; the failed field is still dirty.
+    expect(setCallCount).toBe(1)
+    expect(store.getState().persistedUIWriteInFlightCounts).toEqual({})
+    const state = store.getState()
+    expect(
+      diffPersistedUIWriteFields(
+        capturePersistedUIWriteBaseline(state),
+        state.persistedUIWriteBaseline!
+      )
+    ).toEqual({ hideDefaultBranchWorkspace: true })
+    expect(vi.getTimerCount()).toBe(0)
+
+    // Recovery is explicit: the next user edit arms one debounce and flushes
+    // both the old dirty field and the new edit once transport recovers.
+    rejectSets = false
+    act(() => {
+      store.getState().setHideCliCreatedWorkspaces(true)
+    })
+    vi.advanceTimersByTime(150)
+    await Promise.resolve()
+    expect(setCallCount).toBe(2)
+    expect(authority.get().hideDefaultBranchWorkspace).toBe(true)
+    expect(authority.get().hideCliCreatedWorkspaces).toBe(true)
+    expect(store.getState().persistedUIWriteInFlightCounts).toEqual({})
+
+    // Unmount cleanup must cancel any delayed work and prevent a post-close
+    // write when the store changes later.
+    act(() => {
+      root.unmount()
+    })
+    expect(vi.getTimerCount()).toBe(0)
+    store.getState().setHideDetachedHeadWorkspaces(true)
+    vi.advanceTimersByTime(300)
+    expect(setCallCount).toBe(2)
+    expect(vi.getTimerCount()).toBe(0)
+  })
+
+  it('a transient rejection still flushes a pending flip-back exactly once', async () => {
+    // A trailing pass that was already needed for an edit made while a write
+    // was in flight must survive that write's rejection. This is distinct from
+    // a terminal rejection with no newer edit (covered above).
+    holdAcks = true
+    rejectNextSet = true
+    setCallCount = 0
+    act(() => {
+      store.getState().setHideDefaultBranchWorkspace(true)
+    })
+    vi.advanceTimersByTime(150)
+    expect(setCallCount).toBe(1)
+    expect(pendingRejects).toHaveLength(1)
+
+    // Flip the first field back and edit a second field while write #1 is in
+    // flight. The failed write must not be retried; only the pending edit is
+    // eligible for the one trailing flush.
+    act(() => {
+      store.getState().setHideDefaultBranchWorkspace(false)
+      store.getState().setHideCliCreatedWorkspaces(true)
+    })
+    holdAcks = false
+    pendingRejects.splice(0).forEach((reject) => reject(new Error('transient transport failure')))
+    expect(pendingRejects).toHaveLength(0)
+    await Promise.resolve()
+    vi.advanceTimersByTime(150)
+    await Promise.resolve()
+
+    expect(setCallCount).toBe(2)
+    expect(authority.get().hideDefaultBranchWorkspace).toBe(false)
+    expect(authority.get().hideCliCreatedWorkspaces).toBe(true)
+    expect(store.getState().persistedUIWriteInFlightCounts).toEqual({})
+
+    // Once the pending edit is acknowledged, no rejection-induced timer may
+    // remain to send another copy.
+    vi.advanceTimersByTime(150)
+    await Promise.resolve()
+    expect(setCallCount).toBe(2)
+    expect(vi.getTimerCount()).toBe(0)
+  })
+
+  it('a rejected trailing write does not inherit automatic retry permission', async () => {
+    // A successful write's trailing pass is still a normal persistence attempt:
+    // if that pass is rejected with no newer edit, it must be terminal too.
+    holdAcks = true
+    setCallCount = 0
+    act(() => {
+      store.getState().setHideDefaultBranchWorkspace(true)
+    })
+    vi.advanceTimersByTime(150)
+    expect(setCallCount).toBe(1)
+
+    act(() => {
+      store.getState().setHideCliCreatedWorkspaces(true)
+    })
+    await resolveAcks()
+    holdAcks = false
+    rejectSets = true
+
+    // The edit's debounce and the successful ack's trailing pass may overlap;
+    // either way only one rejected trailing attempt is allowed.
+    vi.advanceTimersByTime(150)
+    await Promise.resolve()
+    expect(setCallCount).toBe(2)
+    expect(store.getState().persistedUIWriteInFlightCounts).toEqual({})
+
+    vi.advanceTimersByTime(150)
+    await Promise.resolve()
+    expect(setCallCount).toBe(2)
+    expect(vi.getTimerCount()).toBe(0)
+  })
+
+  it('a synchronously throwing ui.set settles without a retry loop', async () => {
     const api = (
       window as unknown as { api: { ui: { set: (u: Partial<PersistedUIState>) => Promise<void> } } }
     ).api.ui
@@ -532,42 +669,13 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () =
     // A leaked marker would pin the field against hydration for the renderer's life.
     expect(store.getState().persistedUIWriteInFlightCounts).toEqual({})
 
-    // The throw must also reschedule the trailing pass: once the transport
-    // recovers, the dirty field flushes without waiting for another edit.
+    // Recovery is explicit: a later edit flushes the still-dirty field.
     api.set = workingSet
-    await flushDesktopDebounce()
-    expect(authority.get().hideDefaultBranchWorkspace).toBe(true)
-  })
-
-  it('a rejection re-schedules the trailing pass it caused to be skipped', async () => {
-    // Round-3 verification: a trailing pass that bails because a write is in
-    // flight relies on that write's settle to reschedule — including rejection,
-    // or a pending flip-back is stranded until the next unrelated edit.
-    holdAcks = true
-    act(() => {
-      store.getState().setHideDefaultBranchWorkspace(true)
-    })
-    await flushDesktopDebounce()
-    // Flip back while write #1 is in flight: only a trailing flush carries it.
-    act(() => {
-      store.getState().setHideDefaultBranchWorkspace(false)
-    })
-    await resolveAcks()
-
-    // Before the trailing pass fires, a different field's write goes out and
-    // is REJECTED while in flight when the trailing pass checks.
-    rejectSets = true
     act(() => {
       store.getState().setHideCliCreatedWorkspaces(true)
     })
     await flushDesktopDebounce()
-
-    rejectSets = false
-    holdAcks = false
-    await flushDesktopDebounce()
-    await flushDesktopDebounce()
-    expect(authority.get().hideDefaultBranchWorkspace).toBe(false)
-    expect(store.getState().hideDefaultBranchWorkspace).toBe(false)
+    expect(authority.get().hideDefaultBranchWorkspace).toBe(true)
   })
 
   it('overlapping in-flight writes on one field decrement, not clear, the marker', () => {