Merge pull request #785 from omdsh-dev/fix/native-leak-guard-tests

fix(native): 钉牢 files 接管泄漏的守护断言 + 记录两条不变量

tests/e2e/native-reload.e2e.ts

@@ -0,0 +1,160 @@
+/**
+ * In-page CLIENT ENTRY REPLACEMENT ("plugin reload") — the deployment-level
+ * guard for the `dsh-better-sidebar:files` orphaned-tab-type bug.
+ *
+ * Field symptom (issues #770/#771, reproduced on the official desktop shell):
+ * a client-side replacement of the plugin entry — a plugin-market update, a
+ * disable→enable in the Plugins page, an HMR rebundle — left the host's tab
+ * type id `dsh-better-sidebar:files` registered with nobody holding its
+ * disposer, so the plugin could never register it again: every later `sync()`
+ * reported `native register files error: … already registered` and the Files
+ * takeover rendered the host's "nothing here can view this kind of content"
+ * face until a page refresh.
+ *
+ * Root cause (fixed in v0.22.1): `sync()`'s drop loop treated the plugin's own
+ * `files` takeover like a service descriptor, so every notification tore it
+ * down and re-created it in the same pass. During the reload the re-creation
+ * ran on the dying plugin context: `tabs.register` (host context, still alive)
+ * took the id and `ctx.slots.inject` (plugin context) then threw
+ * `cannot create effect on inactive context`, losing the disposer.
+ *
+ * The trigger here is the same one the host's own HMR uses: the installed
+ * client bundle is stat-polled, and a changed revision (`mtime`/`ctime`/`size`)
+ * makes the host tear the entry's fiber down and re-import it IN PAGE — no
+ * full page navigation, which is what made the old failure permanent.
+ */
+import { mkdirSync, utimesSync, writeFileSync } from 'node:fs'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { test, expect, type APIRequestContext } from '@playwright/test'
+import { PAGE_URL, createHostApi, hostRpc, sendFirstMessage } from './host'
+
+/** Workspace the sidebar renders against (created by this lane's seeding). */
+const WORKSPACE_PATH = process.env.DSH_E2E_WORKSPACE ?? join(tmpdir(), 'dsh-e2e-workspace')
+
+/** A file seeded into the workspace, opened through the Files takeover. */
+const SEEDED_FILE = 'hello.txt'
+
+/**
+ * The installed client bundle the host's HMR stat-poll watches. Derived from
+ * the profile the server was booted with (`DSH_HOME`), overridable for a
+ * hand-run lane.
+ */
+const CLIENT_JS = process.env.DSH_E2E_PLUGIN_CLIENT_JS
+  ?? (process.env.DSH_HOME === undefined
+    ? undefined
+    : join(process.env.DSH_HOME, 'profiles/web/node_modules/dsh-better-sidebar/lib/client.js'))
+
+/** The strip `fail()` pins to <body>; the plugin's registration failures land here. */
+const REGISTRATION_ERROR = /native register .* error/
+
+let api: APIRequestContext
+
+test.beforeAll(async () => {
+  mkdirSync(WORKSPACE_PATH, { recursive: true })
+  writeFileSync(join(WORKSPACE_PATH, SEEDED_FILE), 'hello from the reload lane\n')
+  api = await createHostApi()
+  const workspace = await hostRpc<{ workspace: { workspaceId: string } }>(
+    api, 'workspace.create', { path: WORKSPACE_PATH },
+  )
+  await hostRpc(api, 'session.create', { workspaceId: workspace.value.workspace.workspaceId })
+})
+
+test.afterAll(async () => {
+  await api?.dispose()
+})
+
+test('the native tab types survive an in-page client entry replacement', async ({ page }) => {
+  // A hard failure, not a skip: this gate's whole value is that it cannot
+  // pass quietly when the harness stops telling us where the bundle lives.
+  if (CLIENT_JS === undefined) {
+    throw new Error('DSH_HOME (or DSH_E2E_PLUGIN_CLIENT_JS) must point at the profile running this server')
+  }
+  const consoleErrors: string[] = []
+  page.on('console', (message) => {
+    if (message.type() === 'error') consoleErrors.push(message.text())
+  })
+
+  await page.goto(PAGE_URL, { waitUntil: 'domcontentloaded' })
+  await expect(page.locator('#root > *')).not.toHaveCount(0, { timeout: 90_000 })
+  const sidebar = page.locator('[data-dsh-better-sidebar]')
+  await expect(sidebar).toBeAttached({ timeout: 90_000 })
+
+  // A keyless boot stacks onboarding takeovers over the shell (versioned
+  // welcome notice, then the provider-config dialog). Same bounded dismissal
+  // loop as the mount lane: a masked click is retried, an absent takeover is
+  // fine.
+  try {
+    await expect
+      .poll(() => page.getByRole('button', { name: /^(Continue|Configure later)$/ }).count(), { timeout: 60_000 })
+      .toBeGreaterThan(0)
+  } catch {
+    console.warn('[e2e] no onboarding takeover appeared; proceeding without dismissal')
+  }
+  for (let round = 0; round < 8; round++) {
+    let dismissed = false
+    for (const name of ['Continue', 'Configure later']) {
+      const button = page.getByRole('button', { name, exact: true }).first()
+      if ((await button.count()) === 0) continue
+      try {
+        await button.click({ timeout: 4_000 })
+        dismissed = true
+        await page.waitForTimeout(1_000)
+      } catch {
+        // Masked by the takeover stacked above it; the next round tries again.
+      }
+    }
+    if (!dismissed) break
+  }
+
+  // The session header (and with it the native right sidebar's controls) only
+  // exists once the session has a turn — the mount lane does the same.
+  await sendFirstMessage(page)
+  await page.locator('[data-sidebar-right-expand]').first().click()
+  const pane = page.locator('[data-sidebar-right-panel]')
+  await expect(page.locator('[data-sidebar-right-guide]')).toBeVisible({ timeout: 30_000 })
+  // The takeover is ALIVE before the reload: its guide row opens the plugin's
+  // explorer and the seeded file is listed.
+  await page.locator('[data-sidebar-right-guide-entry="files"]').click()
+  await expect(pane.locator(`[role="button"][title$="${SEEDED_FILE}"]:visible`)).toHaveCount(1, { timeout: 30_000 })
+
+  // ── Trigger: bump the installed bundle's revision ─────────────────────
+  // Tag the CURRENT host element so the replacement is observable: a disposal
+  // removes it (`host.remove()`), and the new activation appends a fresh one.
+  await page.evaluate(() => {
+    document.querySelector('[data-dsh-better-sidebar]')?.setAttribute('data-e2e-generation', 'before-reload')
+  })
+  const now = new Date()
+  utimesSync(CLIENT_JS!, now, new Date(now.getTime() + 2_000))
+  await expect(
+    page.locator('[data-dsh-better-sidebar][data-e2e-generation="before-reload"]'),
+    'the host must replace the plugin entry in page (new activation)',
+  ).toHaveCount(0, { timeout: 60_000 })
+  // Let the fresh activation finish its registrations.
+  await expect(page.locator('[data-dsh-better-sidebar]')).toBeAttached({ timeout: 60_000 })
+  await page.waitForTimeout(2_000)
+
+  // ── Assertions: no registration failure, takeover usable ──────────────
+  const registrationErrors = consoleErrors.filter(message => REGISTRATION_ERROR.test(message))
+  expect(registrationErrors, registrationErrors.join('\n')).toEqual([])
+  await expect(
+    page.locator('body > div', { hasText: 'native register files error' }),
+    'the failure strip must not be pinned',
+  ).toHaveCount(0)
+  // The Files takeover works again after the replacement: the guide row is
+  // offered and the explorer still lists the seeded file.
+  if (await page.locator('[data-sidebar-right-guide]').count() === 0) {
+    await page.locator('[data-dockkit-add-tab]').first().click()
+  }
+  const filesEntry = page.locator('[data-sidebar-right-guide-entry="files"]')
+  await expect(filesEntry, 'the Files guide row must be offered again').toHaveCount(1, { timeout: 30_000 })
+  await filesEntry.click()
+  // The explorer must really render again — assert the CONTENT first (a bare
+  // "no unavailable marker" check passes on an empty pane), then the host's
+  // "a kind with no registrant" fallback must be absent.
+  await expect(pane.locator(`[role="button"][title$="${SEEDED_FILE}"]:visible`)).toHaveCount(1, { timeout: 30_000 })
+  await expect(
+    pane.locator('[data-sidebar-right-unavailable]'),
+    'the host must not fall back to its "nothing can view this kind" face',
+  ).toHaveCount(0)
+})

tests/native-registration.spec.ts

@@ -14,12 +14,19 @@
  * `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:
+ * Three properties keep that impossible, and all 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.
+ *      take the others down with it;
+ *   3. slots installed before the failing one are released too (the partial
+ *      release inside `registerSlots`).
+ *
+ * The assertions are written against the fake registry's EVENT LOG, not only
+ * against the set of live ids: an orphaned type leaves the key set identical to
+ * a healthy one, so a key-set assertion would pass on the unfixed code and pin
+ * nothing.
  */
 import { describe, expect, it, vi } from 'vitest'
 import { registerNativeSurface } from '../src/client/native/index.ts'
@@ -44,8 +51,14 @@ function createRegistry() {
         throw new Error(`sidebarRight: tab type id "${definition.id}" is already registered`)
       }
       events.push(`register ${definition.id}`)
+      // The host's disposer is IDEMPOTENT (a cordis effect disposer returns
+      // the same task on a second call), so a repeated release must neither
+      // log again nor pretend to free something twice.
+      let released = false
       const dispose = (): void => {
+        if (released) return
         if (failReleaseOf === definition.id) throw new Error(`cannot release ${definition.id}`)
+        released = true
         registered.delete(definition.id)
         events.push(`release ${definition.id}`)
       }
@@ -58,19 +71,28 @@ function createRegistry() {
 /**
  * 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
+ * `inactive`), a slot listed in `failSlots` throws from `slots.register`, and
  * `inject` hands back the registry while honouring the callback's own teardown.
+ *
+ * Slot failures are keyed by `name::key` — NOT by `key` alone. Both slots of a
+ * descriptor share the same key, so a key-keyed failure can only ever fail the
+ * FIRST slot; failing the second one for a single descriptor is the only shape
+ * that exercises `registerSlots`' partial release (the first slot is already
+ * installed when the second throws).
  */
 function createHost() {
   const registry = createRegistry()
+  /** Slot registrations and releases, in order (the partial-release witness). */
+  const slotEvents: string[] = []
   // 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>(),
+    /** `name::key` slots whose registration throws. */
+    failSlots: new Set<string>(),
   }
+  const slotId = (options: { name: string; key?: string }): string => `${options.name}::${options.key ?? ''}`
   const ctx = {
     inject: (
       _deps: readonly string[],
@@ -85,15 +107,15 @@ function createHost() {
         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 () => {}
+      register: (options: { name: string; key?: string }) => {
+        const id = slotId(options)
+        if (state.failSlots.has(id)) throw new Error(`slot registration for "${id}" failed`)
+        slotEvents.push(`register ${id}`)
+        return () => { slotEvents.push(`release ${id}`) }
       },
     },
   }
-  return { registry, state, ctx }
+  return { registry, slotEvents, state, ctx }
 }
 
 /** A store + service with two enabled descriptors, plus the record registry. */
@@ -115,31 +137,42 @@ function silenceFailures(): { restore: () => void; messages: string[] } {
   return { restore: () => { spy.mockRestore() }, messages }
 }
 
+/**
+ * The phase labels a surface reports through the client's visible diagnostic
+ * channel (`reportFailure` → `fail('native ' + phase, error)`).
+ */
+function collectReports(): { phases: string[]; report: (phase: string) => void } {
+  const phases: string[] = []
+  return { phases, report: (phase: string) => { phases.push(phase) } }
+}
+
 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([])
+    const reports = collectReports()
+    // 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, reportFailure: reports.report,
+    })).not.toThrow()
+    expect([...host.registry.registered.keys()], 'a failed registration leaves no type behind').toEqual([])
+    // Each of them is reported through the visible channel, not swallowed.
+    expect(reports.phases.sort()).toEqual(['register editor', 'register files', 'register git'])
 
-      // 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()
-    }
+    // The context comes back (the reload's next activation): the same ids must
+    // be registrable — no "already registered" from the orphan. The event log
+    // is the discriminating assertion: a leaked type would keep the key set
+    // looking correct while the retry throws into the same catch.
+    host.state.inactive = false
+    host.registry.events.length = 0
+    store.setPrefs(store.getPrefs())
+    expect(host.registry.events.sort()).toEqual([
+      'register dsh-better-sidebar:editor',
+      'register dsh-better-sidebar:files',
+      'register dsh-better-sidebar:git',
+    ])
   })
 
   it('never re-creates the "files" takeover on an unrelated notification', () => {
@@ -159,23 +192,34 @@ describe('native tab registration lifecycle', () => {
   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 reports = collectReports()
+    // The SECOND slot of ONE descriptor: its type and its body slot are
+    // already installed when this one throws.
+    host.state.failSlots.add('sidebar.right.pane.tab.title::dsh-better-sidebar:git')
     const log = silenceFailures()
     try {
-      expect(() => registerNativeSurface({ ctx: host.ctx as never, store, service, records })).not.toThrow()
+      expect(() => registerNativeSurface({
+        ctx: host.ctx as never, store, service, records, reportFailure: reports.report,
+      })).not.toThrow()
       expect([...host.registry.registered.keys()].sort()).toEqual([
         'dsh-better-sidebar:editor',
         'dsh-better-sidebar:files',
       ])
+      expect(reports.phases).toEqual(['register git'])
+      // The body slot installed before the failure was released; the failing
+      // one never registered, so there is nothing to release for it.
+      expect(host.slotEvents.filter(event => event === 'release sidebar.right.pane.tab::dsh-better-sidebar:git'))
+        .toHaveLength(1)
+      expect(host.slotEvents.filter(event => event === 'release sidebar.right.pane.tab.title::dsh-better-sidebar:git'))
+        .toEqual([])
+
       // 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()
+      // written off — and the retry REGISTERS it (event log), rather than
+      // merely ending up with a key set a leaked type produces as well.
+      host.state.failSlots.clear()
+      host.registry.events.length = 0
       store.setPrefs(store.getPrefs())
-      expect([...host.registry.registered.keys()].sort()).toEqual([
-        'dsh-better-sidebar:editor',
-        'dsh-better-sidebar:files',
-        'dsh-better-sidebar:git',
-      ])
+      expect(host.registry.events).toEqual(['register dsh-better-sidebar:git'])
     } finally {
       log.restore()
     }