fix(desktop): serialize server changes and report partial teardown honestly

Two problems in the same transaction.

The picker re-enabled Connect whenever the field changed, including while a
request was in flight, so typing and pressing Enter could start a second change
that interleaved its teardown and its write with the first — the later write,
not a single transition, deciding the next server. The transaction is now
serialized in the main process, the way the sign-out coordinator guards its
own teardown, since the IPC boundary is reachable regardless of what the page
does; the page keeps its button disabled for the whole request so it never asks
for something it will only be refused.

The stores clear independently, so one can succeed while the other fails. There
is nothing to roll back to — a revoked cookie jar and deleted security-scoped
bookmarks cannot be un-deleted — and moving anyway would hand the incoming
deployment whatever survived. So the change is still refused, but the message
no longer names only the failed store as though nothing else had happened: it
says some local access may already have been cleared, and that retrying
finishes the job. Clearing an already-empty store succeeds, so a retry is safe.

Author: waleedlatif1

apps/desktop/src/main/server-window.test.ts

@@ -114,6 +114,48 @@ describe('server window', () => {
     expect(deps.clearDeploymentScopedState).not.toHaveBeenCalled()
   })
 
+  // The picker re-enables its button while a request is pending, and the IPC
+  // boundary is reachable regardless of what the page does, so the transaction
+  // has to be serialized here rather than in the renderer.
+  it('refuses a second change while one is in flight', async () => {
+    let release: (() => void) | undefined
+    const gate = new Promise<void>((resolve) => {
+      release = resolve
+    })
+    const slow = makeDeps({
+      clearDeploymentScopedState: vi.fn(async (): Promise<readonly string[]> => {
+        await gate
+        return []
+      }),
+    })
+    const handle = createServerWindow(slow)
+
+    const first = handle.setOrigin('https://sim.other.example')
+    const second = await handle.setOrigin('https://sim.third.example')
+
+    expect(second).toMatchObject({ ok: false })
+    expect(second).toHaveProperty('error', expect.stringContaining('already in progress'))
+    release?.()
+    await expect(first).resolves.toMatchObject({ ok: true, unchanged: false })
+    expect(slow.relaunch).toHaveBeenCalledTimes(1)
+    expect(slow.config.setOrigin).toHaveBeenCalledTimes(1)
+    expect(slow.config.setOrigin).toHaveBeenCalledWith('https://sim.other.example')
+  })
+
+  // The guard must not latch: a refused change has to leave the picker usable.
+  it('allows a later change once the first has settled', async () => {
+    const failing = makeDeps({
+      clearDeploymentScopedState: vi.fn(async () => ['local file access']),
+    })
+    const handle = createServerWindow(failing)
+
+    await handle.setOrigin('https://sim.other.example')
+    const second = await handle.setOrigin('https://sim.third.example')
+
+    expect(second).toMatchObject({ ok: false })
+    expect(second).toHaveProperty('error', expect.stringContaining('local file access'))
+  })
+
   // Fail closed. A store that could not be emptied is access the incoming
   // deployment would inherit and that startup would restore, so the change is
   // refused outright — and because nothing is persisted until the teardown
@@ -127,6 +169,10 @@ describe('server window', () => {
 
     expect(result).toMatchObject({ ok: false })
     expect(result).toHaveProperty('error', expect.stringContaining('local file access'))
+    // The stores clear independently, so the other one may already be empty and
+    // cannot be restored. Naming only the failure would read as "nothing
+    // happened", which is not what happened.
+    expect(result).toHaveProperty('error', expect.stringContaining('may already have been cleared'))
     expect(failing.relaunch).not.toHaveBeenCalled()
     expect(failing.config.setOrigin).not.toHaveBeenCalled()
     expect(failing.config.getOrigin()).toBe(CURRENT)

apps/desktop/src/main/server-window.ts

@@ -100,6 +100,14 @@ export interface ServerWindowHandle {
  */
 export function createServerWindow(deps: ServerWindowDeps): ServerWindowHandle {
   let win: BrowserWindow | null = null
+  /**
+   * Serializes the destructive part of a change, the way the sign-out
+   * coordinator guards its own teardown. The picker re-enables its button
+   * while a request is pending, and the IPC boundary is reachable regardless
+   * of what the page does, so without this two changes could interleave their
+   * teardown and their write and let the later write pick the next server.
+   */
+  let changeInFlight = false
 
   const getConfiguration = (): DesktopServerConfiguration => {
     const origin = deps.config.getOrigin()
@@ -177,36 +185,51 @@ export function createServerWindow(deps: ServerWindowDeps): ServerWindowHandle {
       return { ok: true, origin, unchanged: true }
     }
 
-    // Fail closed, and clear BEFORE persisting. If a store cannot be emptied,
-    // the shell must not move: the incoming deployment would otherwise inherit
-    // folder grants and authenticated browser sessions the outgoing one was
-    // given, and they are restored on the next startup. Nothing has been
-    // written at this point, so refusing leaves the shell exactly where it was
-    // rather than stranding it on a half-applied change.
-    const surviving = await deps.clearDeploymentScopedState().catch((error) => {
-      logger.error('Deployment-scoped teardown threw', { error: getErrorMessage(error) })
-      return ['local file access and built-in browser sessions']
-    })
-    if (surviving.length > 0) {
-      logger.error('Refusing to change server; deployment-scoped state survived', { surviving })
-      return {
-        ok: false,
-        error: `Could not clear ${surviving.join(' or ')} from the current server, so the server was not changed. Try again, or sign out first.`,
-      }
+    if (changeInFlight) {
+      return { ok: false, error: 'A server change is already in progress.' }
     }
+    changeInFlight = true
+    try {
+      // Fail closed, and clear BEFORE persisting. If a store cannot be emptied,
+      // the shell must not move: the incoming deployment would otherwise
+      // inherit folder grants and authenticated browser sessions the outgoing
+      // one was given, and they are restored on the next startup. Nothing has
+      // been written at this point, so refusing leaves the shell on the server
+      // it was already using rather than half-applying the change.
+      const surviving = await deps.clearDeploymentScopedState().catch((error) => {
+        logger.error('Deployment-scoped teardown threw', { error: getErrorMessage(error) })
+        return ['local file access and built-in browser sessions']
+      })
+      if (surviving.length > 0) {
+        logger.error('Refusing to change server; deployment-scoped state survived', { surviving })
+        // Deliberately describes the whole teardown, not just what failed. The
+        // stores clear independently, so one may already be empty by now, and
+        // there is nothing to roll back to — a revoked cookie jar and deleted
+        // security-scoped bookmarks cannot be un-deleted. Saying "some may have
+        // been cleared" is the honest account, and a retry is safe: clearing an
+        // already-empty store succeeds, so it finishes the job rather than
+        // repeating it.
+        return {
+          ok: false,
+          error: `Could not clear ${surviving.join(' or ')} from the current server, so the server was not changed. Some local access may already have been cleared. Try again to finish, or sign out first.`,
+        }
+      }
 
-    logger.info('Server origin changed; relaunching', { from: current, to: origin })
-    deps.config.setOrigin(raw)
-    for (const key of ORIGIN_SCOPED_SETTINGS) {
-      deps.config.set(key, undefined)
+      logger.info('Server origin changed; relaunching', { from: current, to: origin })
+      deps.config.setOrigin(raw)
+      for (const key of ORIGIN_SCOPED_SETTINGS) {
+        deps.config.set(key, undefined)
+      }
+      // setOrigin writes through immediately; the clears above are debounced
+      // like every other setting. `before-quit` flushes too, but doing it here
+      // keeps the write independent of the Electron quit sequence.
+      deps.config.flush()
+      close()
+      deps.relaunch()
+      return { ok: true, origin, unchanged: false }
+    } finally {
+      changeInFlight = false
     }
-    // setOrigin writes through immediately; the clears above are debounced like
-    // every other setting. `before-quit` flushes too, but doing it here keeps
-    // the write independent of the Electron quit sequence.
-    deps.config.flush()
-    close()
-    deps.relaunch()
-    return { ok: true, origin, unchanged: false }
   }
 
   return { open, getConfiguration, setOrigin }

apps/desktop/static/server.html

@@ -204,8 +204,14 @@ <h1>Sim server</h1>
         input.setAttribute('aria-invalid', tone === 'error' ? 'true' : 'false')
       }
 
+      // Set for the whole life of a request. Without it the input handler below
+      // re-enables Connect mid-flight, letting a second change race the first.
+      // The main process guards the transaction itself; this keeps the page
+      // from asking for something it will only be refused.
+      let pending = false
+
       function syncConnectEnabled() {
-        connect.disabled = input.value.trim().length === 0
+        connect.disabled = pending || input.value.trim().length === 0
       }
 
       input.addEventListener('input', () => {
@@ -219,6 +225,8 @@ <h1>Sim server</h1>
       cancel.addEventListener('click', () => window.close())
 
       async function submit() {
+        if (pending) return
+        pending = true
         connect.disabled = true
         setMessage('Connecting…')
         try {
@@ -235,6 +243,7 @@ <h1>Sim server</h1>
         } catch {
           setMessage('The server could not be changed.', 'error')
         } finally {
+          pending = false
           syncConnectEnabled()
         }
       }