Merge pull request #777 from yanzhaohui1999/fix/native-registration-leak-v2

fix(native): 注册失败的 tab 类型必须释放，否则 id 永久占用（插件重载后文件树变空态）

src/client/native/index.ts

@@ -156,6 +156,21 @@ interface Registration {
   readonly dispose: () => void
 }
 
+/**
+ * Run one registration disposer, reporting a failure instead of propagating
+ * it: releasing a registration must never mask the error being handled, and
+ * one broken entry must not leave the others registered.
+ * @param dispose - the disposer to run.
+ * @param what - the registration's name, for the log line.
+ */
+function disposeSafely(dispose: () => void, what: string): void {
+  try {
+    dispose()
+  } catch (error) {
+    console.error(`[dsh-better-sidebar] ${what} release failed:`, error)
+  }
+}
+
 /** Everything the registrations need. */
 export interface NativeSurfaceDeps {
   readonly ctx: Context
@@ -208,23 +223,44 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void {
       return address !== undefined && address.scope === 'session' ? address.sessionId : undefined
     }
 
-    /** Register the body + chip-title slots for one native implementation id. */
+    /**
+     * Register the body + chip-title slots for one native implementation id.
+     *
+     * A slot registration can fail long after the TYPE registration succeeded
+     * (`ctx.effect` on a context that is already inactive, i.e. during a
+     * plugin reload/disposal), so the disposers are collected as they are
+     * handed out and the partial set is released on the way out: the caller
+     * must be able to roll its type registration back WITHOUT losing track of
+     * a slot that did register.
+     */
     const registerSlots = (
       id: string,
       injected: Omit<NativeBodyInjected, 'sessionId'>,
       params: Pick<NativeBodyInjected, 'paramsOf' | 'sessionIdOf'>,
-    ): Array<() => void> => [
+    ): Array<() => void> => {
-      ctx.slots.inject('sidebar.right.pane.tab', () => ctx.slots.register({
+      // Collected AS THEY ARE HANDED OUT: a slot registration can fail long
-        name: 'sidebar.right.pane.tab',
+      // after the TYPE registration succeeded (a plugin reload makes
-        key: id,
+      // `ctx.slots.inject` throw on the now-inactive context), and the caller
-        inject: (sessionId: string) => ({ ...injected, ...params, sessionId }),
+      // must then be able to release the type again WITHOUT losing track of a
-      }, NativeTabBody)),
+      // slot that did register.
-      ctx.slots.inject('sidebar.right.pane.tab.title', () => ctx.slots.register({
+      const disposers: Array<() => void> = []
-        name: 'sidebar.right.pane.tab.title',
+      try {
-        key: id,
+        disposers.push(ctx.slots.inject('sidebar.right.pane.tab', () => ctx.slots.register({
-        inject: () => ({ records, service, descriptorId: injected.descriptorId }),
+          name: 'sidebar.right.pane.tab',
-      }, NativeTabTitle)),
+          key: id,
-    ]
+          inject: (sessionId: string) => ({ ...injected, ...params, sessionId }),
+        }, NativeTabBody)))
+        disposers.push(ctx.slots.inject('sidebar.right.pane.tab.title', () => ctx.slots.register({
+          name: 'sidebar.right.pane.tab.title',
+          key: id,
+          inject: () => ({ records, service, descriptorId: injected.descriptorId }),
+        }, NativeTabTitle)))
+      } catch (error) {
+        for (const dispose of disposers) disposeSafely(dispose, `native tab slot "${id}"`)
+        throw error
+      }
+      return disposers
+    }
 
     /** One descriptor's native type + body + title, as one disposable. */
     const registerDescriptor = (descriptor: TabDescriptor): (() => void) => {
@@ -271,14 +307,25 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void {
             }],
           }),
       })
-      const slots = registerSlots(
+      // The type is in the host's registry the moment `tabs.register`
-        id,
+      // returns; a slot failure must release it here, or the id stays taken
-        { ctx, store, service, records, descriptorId: descriptor.id },
+      // for the rest of the page's life (the host refuses a second
-        isEditor ? { paramsOf: fileParamsOf, sessionIdOf: fileSessionIdOf } : {},
+      // registration of the same id) and the tab kind renders the host's
-      )
+      // "nothing can view this" face forever.
+      let slots: Array<() => void>
+      try {
+        slots = registerSlots(
+          id,
+          { ctx, store, service, records, descriptorId: descriptor.id },
+          isEditor ? { paramsOf: fileParamsOf, sessionIdOf: fileSessionIdOf } : {},
+        )
+      } catch (error) {
+        disposeSafely(disposeType, `native tab type "${id}"`)
+        throw error
+      }
       return () => {
-        for (const dispose of slots.reverse()) dispose()
+        for (const dispose of slots.reverse()) disposeSafely(dispose, `native tab slot "${id}"`)
-        disposeType()
+        disposeSafely(disposeType, `native tab type "${id}"`)
       }
     }
 
@@ -304,10 +351,16 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void {
           ...guideIconOf(editor?.icon),
         }],
       })
-      const slots = registerSlots(id, { ctx, store, service, records, descriptorId: EDITOR_KIND }, {})
+      let slots: Array<() => void>
+      try {
+        slots = registerSlots(id, { ctx, store, service, records, descriptorId: EDITOR_KIND }, {})
+      } catch (error) {
+        disposeSafely(disposeType, `native tab type "${id}"`)
+        throw error
+      }
       return () => {
-        for (const dispose of slots.reverse()) dispose()
+        for (const dispose of slots.reverse()) disposeSafely(dispose, `native tab slot "${id}"`)
-        disposeType()
+        disposeSafely(disposeType, `native tab type "${id}"`)
       }
     }
 
@@ -319,14 +372,23 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void {
         wanted.set(descriptor.id, () => registerDescriptor(descriptor))
       }
       for (const [descriptorId, registration] of live) {
-        if (wanted.has(descriptorId)) continue
+        // `files` is this module's OWN takeover, never a descriptor: it is
-        registration.dispose()
+        // owned by the editor-type switch below. Re-creating it here on every
+        // notification used to put its host-side type through a
+        // tear-down/re-register window on each store commit — and a
+        // re-registration attempted on an already-inactive context (a plugin
+        // reload) left the type registered with nobody holding its disposer,
+        // so the same id could never be registered again for the rest of the
+        // page's life.
+        if (descriptorId === FILES_KIND || wanted.has(descriptorId)) continue
+        disposeSafely(() => registration.dispose(), `native tab type "${descriptorId}"`)
         live.delete(descriptorId)
       }
       for (const [descriptorId, create] of wanted) {
         if (live.has(descriptorId)) continue
         // One descriptor must not take the rest of the surface down with it:
-        // report and continue so the remaining types still register.
+        // report and continue so the remaining types still register. `create`
+        // has already released whatever it managed to install.
         try {
           live.set(descriptorId, { dispose: create() })
         } catch (error) {
@@ -345,17 +407,20 @@ export function registerNativeSurface(deps: NativeSurfaceDeps): () => void {
         }
       }
       if (!wantsFiles && hasFiles) {
-        live.get(FILES_KIND)?.dispose()
+        const takeover = live.get(FILES_KIND)
         live.delete(FILES_KIND)
+        if (takeover !== undefined) disposeSafely(() => takeover.dispose(), 'the native "files" takeover')
       }
     }
 
     const disposeSubscriptions = [service.subscribe(sync), store.subscribe(sync)]
     sync()
     return () => {
-      for (const registration of live.values()) registration.dispose()
+      for (const registration of live.values()) {
+        disposeSafely(() => registration.dispose(), 'a native tab registration')
+      }
       live.clear()
-      for (const dispose of disposeSubscriptions.reverse()) dispose()
+      for (const dispose of disposeSubscriptions.reverse()) disposeSafely(dispose, 'a native tab subscription')
     }
   })
   return () => {

tests/native-registration.spec.ts

@@ -0,0 +1,197 @@
+// @vitest-environment jsdom
+/**
+ * The native tab registration LIFECYCLE (`src/client/native/index.ts`): what a
+ * store/service notification may do to the host's tab-type registry, and what
+ * a failed registration is allowed to leave behind.
+ *
+ * The failure this locks is a plugin RELOAD: cordis tears the fiber down, each
+ * built-in descriptor is unregistered one by one, every unregistration notifies
+ * the plugin's service, and `sync()` therefore runs again on a context that is
+ * already inactive. A registration attempted there throws
+ * (`ctx.effect` on an inactive context) — and if the host-side type was already
+ * registered at that point, its disposer is lost, the id stays taken for the
+ * rest of the page's life, and every later attempt is refused with
+ * `tab type id "…" is already registered`. The kind then renders the host's
+ * "nothing here can view this kind of content" face until a page refresh.
+ *
+ * Two properties keep that impossible, and both are asserted here:
+ *   1. an unrelated notification never re-creates the `files` takeover (the
+ *      tear-down/re-register window the reload used to walk into);
+ *   2. a registration that fails after its type was registered releases the
+ *      type again, so the id stays registrable and one broken descriptor cannot
+ *      take the others down with it.
+ */
+import { describe, expect, it, vi } from 'vitest'
+import { registerNativeSurface } from '../src/client/native/index.ts'
+import { createNativeTabRecords } from '../src/client/native/tab-adapter.tsx'
+import { createBetterSidebarService } from '../src/client/service.ts'
+import { createSidebarStore } from '../src/client/state.ts'
+
+/**
+ * The fake `sidebarRightTabs`, mirroring the host's two hard rules: one
+ * registration per id, and the returned disposer is the only way to free it.
+ */
+function createRegistry() {
+  const registered = new Map<string, () => void>()
+  const events: string[] = []
+  let failReleaseOf: string | undefined
+  return {
+    registered,
+    events,
+    failRelease(id: string): void { failReleaseOf = id },
+    register(definition: { id: string }): () => void {
+      if (registered.has(definition.id)) {
+        throw new Error(`sidebarRight: tab type id "${definition.id}" is already registered`)
+      }
+      events.push(`register ${definition.id}`)
+      const dispose = (): void => {
+        if (failReleaseOf === definition.id) throw new Error(`cannot release ${definition.id}`)
+        registered.delete(definition.id)
+        events.push(`release ${definition.id}`)
+      }
+      registered.set(definition.id, dispose)
+      return dispose
+    },
+  }
+}
+
+/**
+ * A fake client context: `slots.inject` runs its callback synchronously (the
+ * real one resolves the declaration and calls `ctx.effect`, which throws while
+ * `inactive`), `slots.register` throws for a key listed in `failKeys`, and
+ * `inject` hands back the registry while honouring the callback's own teardown.
+ */
+function createHost() {
+  const registry = createRegistry()
+  // One mutable state object the ctx closures read: the test flips it after
+  // the host was built.
+  const state = {
+    /** True while the plugin's context is being torn down (the reload case). */
+    inactive: false,
+    /** Native ids whose slot registration fails. */
+    failKeys: new Set<string>(),
+  }
+  const ctx = {
+    inject: (
+      _deps: readonly string[],
+      callback: (injected: { get: (name: string) => unknown }) => (() => void) | void,
+    ) => {
+      const teardown = callback({ get: () => registry })
+      return { dispose: () => { if (typeof teardown === 'function') teardown() } }
+    },
+    get: () => registry,
+    slots: {
+      inject: (_name: string, callback: () => Array<() => void> | (() => void)) => {
+        if (state.inactive) throw new Error('cannot create effect on inactive context')
+        return typeof callback === 'function' ? callback() : () => {}
+      },
+      register: (options: { key?: string }) => {
+        if (options.key !== undefined && state.failKeys.has(options.key)) {
+          throw new Error(`slot registration for "${options.key}" failed`)
+        }
+        return () => {}
+      },
+    },
+  }
+  return { registry, state, ctx }
+}
+
+/** A store + service with two enabled descriptors, plus the record registry. */
+function createPlugins() {
+  const store = createSidebarStore()
+  store.setSession('s1')
+  const service = createBetterSidebarService(store)
+  service.registerTab({ id: 'editor', title: 'Files', component: () => null, icon: () => null })
+  service.registerTab({ id: 'git', title: 'Changes', component: () => null })
+  return { store, service, records: createNativeTabRecords() }
+}
+
+/** Silence (and count) the failure logs the fixed code reports on purpose. */
+function silenceFailures(): { restore: () => void; messages: string[] } {
+  const messages: string[] = []
+  const spy = vi.spyOn(console, 'error').mockImplementation((...args: unknown[]) => {
+    messages.push(args.map(String).join(' '))
+  })
+  return { restore: () => { spy.mockRestore() }, messages }
+}
+
+describe('native tab registration lifecycle', () => {
+  it('releases a type whose slot registration failed, and can register it again later', () => {
+    const host = createHost()
+    const { store, service, records } = createPlugins()
+    const log = silenceFailures()
+    try {
+      // The reload: the context goes inactive while notifications are still
+      // arriving, so every slot registration fails after its type registered.
+      host.state.inactive = true
+      expect(() => registerNativeSurface({ ctx: host.ctx as never, store, service, records })).not.toThrow()
+      expect([...host.registry.registered.keys()], 'a failed registration leaves no type behind').toEqual([])
+
+      // The context comes back (the reload's next activation): the same ids
+      // must be registrable — no "already registered" from the orphan.
+      host.state.inactive = false
+      host.state.failKeys.clear()
+      store.setPrefs(store.getPrefs())
+      expect([...host.registry.registered.keys()].sort()).toEqual([
+        'dsh-better-sidebar:editor',
+        'dsh-better-sidebar:files',
+        'dsh-better-sidebar:git',
+      ])
+    } finally {
+      log.restore()
+    }
+  })
+
+  it('never re-creates the "files" takeover on an unrelated notification', () => {
+    const host = createHost()
+    const { store, service, records } = createPlugins()
+    const dispose = registerNativeSurface({ ctx: host.ctx as never, store, service, records })
+    // The takeover is this module's own entry (never a descriptor), so no
+    // store/service notification has any business touching it. Before the fix
+    // every notification disposed and re-created it.
+    host.registry.events.length = 0
+    for (let round = 0; round < 3; round++) store.setPrefs(store.getPrefs())
+    expect(host.registry.events, 'no notification may tear the "files" takeover down').toEqual([])
+    expect(host.registry.registered.has('dsh-better-sidebar:files')).toBe(true)
+    dispose()
+  })
+
+  it('keeps the other tab types when one descriptor cannot register', () => {
+    const host = createHost()
+    const { store, service, records } = createPlugins()
+    host.state.failKeys.add('dsh-better-sidebar:git')
+    const log = silenceFailures()
+    try {
+      expect(() => registerNativeSurface({ ctx: host.ctx as never, store, service, records })).not.toThrow()
+      expect([...host.registry.registered.keys()].sort()).toEqual([
+        'dsh-better-sidebar:editor',
+        'dsh-better-sidebar:files',
+      ])
+      // The broken descriptor is retried on the next sync instead of being
+      // written off, and the retry succeeds once it can register.
+      host.state.failKeys.clear()
+      store.setPrefs(store.getPrefs())
+      expect([...host.registry.registered.keys()].sort()).toEqual([
+        'dsh-better-sidebar:editor',
+        'dsh-better-sidebar:files',
+        'dsh-better-sidebar:git',
+      ])
+    } finally {
+      log.restore()
+    }
+  })
+
+  it('releases every registration on teardown even when one release throws', () => {
+    const host = createHost()
+    const { store, service, records } = createPlugins()
+    const log = silenceFailures()
+    try {
+      const dispose = registerNativeSurface({ ctx: host.ctx as never, store, service, records })
+      host.registry.failRelease('dsh-better-sidebar:editor')
+      expect(() => dispose()).not.toThrow()
+      expect([...host.registry.registered.keys()]).toEqual(['dsh-better-sidebar:editor'])
+    } finally {
+      log.restore()
+    }
+  })
+})