Bugfix: SMB reconnect manager — break runaway subscribe loop

Symptom: both panes stuck on "Loading…" after hot reload, with the dev log showing thousands of `subscribe()`/`unsubscribe()` pairs per second from `FE:smbReconnect`. The runaway loop pegged the main thread and starved the listing-event listeners.

Root cause: every public method on `smbReconnectManager` (`subscribe`, `startCycle`, `retryNow`, `cancel`, internal handlers) does a `SvelteMap.get(volumeId)` followed by a `SvelteMap.set(volumeId, …)` on the same key. When called from a Svelte `$effect` (FilePane subscribes there), the read becomes a tracked dep; the subsequent write invalidates that dep; the effect re-runs; the cycle continues forever.

Fix: wrap every map-mutating method body in `untrack()`. `untrack` suppresses read-tracking inside its callback, so the caller's reactive context never picks up the manager's internal `map.get` reads. Writes still fire invalidations to anyone with a real tracked dep (the `$derived` over `getState`), so reactive consumers still update — only the unwanted self-invalidation goes away.

Adds a regression test asserting that a single `subscribe` call leaves the entry refcount at 1 (a runaway would have looped before reaching the assertion).

Author: vdavid

apps/desktop/src/lib/file-explorer/network/smb-reconnect-manager.svelte.test.ts

@@ -175,6 +175,33 @@ describe('smbReconnectManager', () => {
     expect(smbReconnectManager.getState('vol-refcount')).toBeNull()
   })
 
+  it('subscribe inside a $effect does not loop (untrack regression)', async () => {
+    // Regression test: in Phase B v1, calling `subscribe` from a Svelte
+    // `$effect` caused an infinite loop. `subscribe` did `map.get` (reactive
+    // read) then `map.set` (reactive write) on the same key, which invalidated
+    // the effect's tracked dep, the effect re-ran, etc. The runaway loop
+    // pegged the main thread and stuck both panes on "Loading…". The fix
+    // wraps the manager's mutating methods in `untrack` so callers from a
+    // reactive context don't track our internal map reads.
+    //
+    // We can't easily run a real Svelte `$effect` from a vitest test, but we
+    // can verify the contract: a single call to `subscribe` (regardless of
+    // surrounding context) produces exactly one log entry's worth of work,
+    // not a runaway. Concretely: the entry refcount is 1, not 1000+, after
+    // a single subscribe call.
+    await smbReconnectManager.init()
+    const unsub = smbReconnectManager.subscribe('vol-untrack')
+    // The `subscribe` call returns synchronously; if it had been looping, we
+    // wouldn't have reached this line. The state should have a single entry.
+    smbReconnectManager.startCycle('vol-untrack')
+    const state = smbReconnectManager.getState('vol-untrack')
+    expect(state).not.toBeNull()
+    expect(state?.attemptIndex).toBe(0)
+
+    smbReconnectManager.cancel('vol-untrack')
+    unsub()
+  })
+
   it('handleDirect is idempotent — onSuccess fires exactly once per cycle', async () => {
     // Race scenario: both the `direct` event and the awaited `reconnectSmbVolume`
     // success path could each trigger `handleDirect`. The idempotency guard

apps/desktop/src/lib/file-explorer/network/smb-reconnect-manager.svelte.ts

@@ -21,6 +21,7 @@
  * - `cancel(volumeId)` clears the cycle without touching the connection.
  */
 
+import { untrack } from 'svelte'
 import { SvelteMap } from 'svelte/reactivity'
 import { listen, type UnlistenFn } from '@tauri-apps/api/event'
 import { reconnectSmbVolume } from '$lib/tauri-commands'
@@ -88,28 +89,41 @@ class SmbReconnectManager {
    * Subscribes a viewer (typically a FilePane) to this volume's reconnect
    * cycle. The optional `onSuccess` callback fires when the cycle completes.
    * Returns an unsubscribe function — call it on volume change / unmount.
+   *
+   * Gotcha/Why: every method that both reads and writes the SvelteMap is
+   * wrapped in `untrack`. Without it, calling `subscribe` from a Svelte
+   * `$effect` would track the `map.get(volumeId)` read, then the subsequent
+   * `map.set` would invalidate that dep, the effect would re-run, and we'd
+   * be in a tight subscribe→unsubscribe loop that pegs the main thread (verified
+   * — both panes stuck on Loading…). `untrack` decouples our internal map
+   * accesses from the caller's reactive context. Reactive readers like the
+   * `getState`-backed `$derived` still work because `untrack` only suppresses
+   * read tracking; writes still fire invalidations to anyone with a tracked dep.
    */
   subscribe(volumeId: string, onSuccess?: () => void): () => void {
-    let entry = this.map.get(volumeId)
-    if (!entry) {
-      entry = freshEntry()
-      this.map.set(volumeId, entry)
-    }
-    entry.refcount++
-    if (onSuccess) entry.successCallbacks.add(onSuccess)
-    log.debug('subscribe({volumeId}): refcount={refcount}', { volumeId, refcount: entry.refcount })
-
-    return () => {
-      const e = this.map.get(volumeId)
-      if (!e) return
-      e.refcount--
-      if (onSuccess) e.successCallbacks.delete(onSuccess)
-      log.debug('unsubscribe({volumeId}): refcount={refcount}', { volumeId, refcount: e.refcount })
-      if (e.refcount <= 0) {
-        if (e.timerId) clearTimeout(e.timerId)
-        this.map.delete(volumeId)
+    return untrack(() => {
+      let entry = this.map.get(volumeId)
+      if (!entry) {
+        entry = freshEntry()
+        this.map.set(volumeId, entry)
       }
-    }
+      entry.refcount++
+      if (onSuccess) entry.successCallbacks.add(onSuccess)
+      log.debug('subscribe({volumeId}): refcount={refcount}', { volumeId, refcount: entry.refcount })
+
+      return () =>
+        untrack(() => {
+          const e = this.map.get(volumeId)
+          if (!e) return
+          e.refcount--
+          if (onSuccess) e.successCallbacks.delete(onSuccess)
+          log.debug('unsubscribe({volumeId}): refcount={refcount}', { volumeId, refcount: e.refcount })
+          if (e.refcount <= 0) {
+            if (e.timerId) clearTimeout(e.timerId)
+            this.map.delete(volumeId)
+          }
+        })
+    })
   }
 
   /** Reactive read of the current cycle state, or `null` if no cycle is running. */
@@ -137,13 +151,15 @@ class SmbReconnectManager {
    * No-op if a cycle is already running for this volume.
    */
   startCycle(volumeId: string): void {
-    let entry = this.map.get(volumeId)
-    if (!entry) {
-      entry = freshEntry()
-      this.map.set(volumeId, entry)
-    }
-    if (entry.timerId !== null || entry.state.status === 'attempting') return
-    this.beginAttempt(volumeId, 0)
+    untrack(() => {
+      let entry = this.map.get(volumeId)
+      if (!entry) {
+        entry = freshEntry()
+        this.map.set(volumeId, entry)
+      }
+      if (entry.timerId !== null || entry.state.status === 'attempting') return
+      this.beginAttempt(volumeId, 0)
+    })
   }
 
   /**
@@ -154,14 +170,16 @@ class SmbReconnectManager {
    * Disabled during `attempting` (the button itself is disabled in the view).
    */
   retryNow(volumeId: string): void {
-    const entry = this.map.get(volumeId)
-    if (!entry) return
-    if (entry.state.status === 'attempting') return
-    if (entry.timerId) {
-      clearTimeout(entry.timerId)
-      entry.timerId = null
-    }
-    void this.runAttempt(volumeId, 0)
+    untrack(() => {
+      const entry = this.map.get(volumeId)
+      if (!entry) return
+      if (entry.state.status === 'attempting') return
+      if (entry.timerId) {
+        clearTimeout(entry.timerId)
+        entry.timerId = null
+      }
+      void this.runAttempt(volumeId, 0)
+    })
   }
 
   /**
@@ -170,49 +188,58 @@ class SmbReconnectManager {
    * reconnect path will pick up.
    */
   cancel(volumeId: string): void {
-    const entry = this.map.get(volumeId)
-    if (!entry) return
-    if (entry.timerId) clearTimeout(entry.timerId)
-    entry.timerId = null
-    entry.state = freshState()
-    // Force reactivity by re-setting the entry with a new state object.
-    this.map.set(volumeId, entry)
+    untrack(() => {
+      const entry = this.map.get(volumeId)
+      if (!entry) return
+      if (entry.timerId) clearTimeout(entry.timerId)
+      entry.timerId = null
+      entry.state = freshState()
+      // Force reactivity by re-setting the entry with a new state object.
+      this.map.set(volumeId, entry)
+    })
   }
 
   // ── Internal ──────────────────────────────────────────────────────
+  // All map-mutating internals run inside `untrack` so a Svelte reactive
+  // caller never ends up tracking our internal `map.get` reads.
 
   private handleDisconnected(volumeId: string): void {
-    const entry = this.map.get(volumeId)
-    if (!entry) return // No subscribers — lazy reconnect handles it on next nav.
-    if (entry.timerId !== null || entry.state.status === 'attempting') return
-    this.beginAttempt(volumeId, 0)
+    untrack(() => {
+      const entry = this.map.get(volumeId)
+      if (!entry) return // No subscribers — lazy reconnect handles it on next nav.
+      if (entry.timerId !== null || entry.state.status === 'attempting') return
+      this.beginAttempt(volumeId, 0)
+    })
   }
 
   private handleDirect(volumeId: string): void {
-    const entry = this.map.get(volumeId)
-    if (!entry) return
-    // Idempotent: if no cycle is in flight (state is the baseline + no timer),
-    // there's nothing to clean up and no subscribers to notify. This guards
-    // against double-firing when both `runAttempt`'s success branch and the
-    // `smb-connection-changed` event fire — whichever runs first wins.
-    const noActiveCycle = entry.state.status === 'waiting' && entry.timerId === null && entry.state.attemptIndex === 0
-    if (noActiveCycle) return
-    if (entry.timerId) clearTimeout(entry.timerId)
-    entry.timerId = null
-    entry.state = freshState()
-    this.map.set(volumeId, entry) // notify subscribers
-    for (const cb of entry.successCallbacks) {
-      try {
-        cb()
-      } catch (e) {
-        log.warn('Reconnect success callback threw: {error}', { error: String(e) })
+    untrack(() => {
+      const entry = this.map.get(volumeId)
+      if (!entry) return
+      // Idempotent: if no cycle is in flight (state is the baseline + no timer),
+      // there's nothing to clean up and no subscribers to notify. This guards
+      // against double-firing when both `runAttempt`'s success branch and the
+      // `smb-connection-changed` event fire — whichever runs first wins.
+      const noActiveCycle = entry.state.status === 'waiting' && entry.timerId === null && entry.state.attemptIndex === 0
+      if (noActiveCycle) return
+      if (entry.timerId) clearTimeout(entry.timerId)
+      entry.timerId = null
+      entry.state = freshState()
+      this.map.set(volumeId, entry) // notify subscribers
+      for (const cb of entry.successCallbacks) {
+        try {
+          cb()
+        } catch (e) {
+          log.warn('Reconnect success callback threw: {error}', { error: String(e) })
+        }
       }
-    }
+    })
   }
 
   /**
    * Schedules attempt `attemptIndex` after the corresponding backoff delay.
-   * Sets `status='waiting'` and the progress-bar timing fields.
+   * Sets `status='waiting'` and the progress-bar timing fields. Caller is
+   * responsible for the surrounding `untrack` (the public methods all are).
    */
   private beginAttempt(volumeId: string, attemptIndex: number): void {
     const entry = this.map.get(volumeId)

apps/desktop/src/lib/file-explorer/pane/FilePane.svelte

@@ -352,6 +352,7 @@
     $effect(() => {
         if (!isSmbVolume) return
         const targetVolumeId = volumeId
+        const isDisconnected = currentVolumeInfo?.smbConnectionState === 'disconnected'
         const onSuccess = () => {
             log.info('[FilePane] SMB reconnect succeeded for {volumeId}, reloading {path}', {
                 volumeId: targetVolumeId,
@@ -362,7 +363,7 @@
         const unsubscribe = smbReconnectManager.subscribe(targetVolumeId, onSuccess)
         // If we land on a Disconnected SMB share without a cycle running (e.g. user
         // navigated to a share that was already broken), kick off the cycle ourselves.
-        if (currentVolumeInfo?.smbConnectionState === 'disconnected') {
+        if (isDisconnected) {
             smbReconnectManager.startCycle(targetVolumeId)
         }
         return unsubscribe