From 233201f7f8ab9edd33e4bd3abc1804d265427348 Mon Sep 17 00:00:00 2001 From: Menghuan1918 Date: Mon, 28 Sep 2026 11:09:30 +0800 Subject: [PATCH] =?UTF-8?q?fix(review):=20=E6=8C=89=E7=8B=AC=E7=AB=8B=20re?= =?UTF-8?q?view=20=E7=BB=93=E8=AE=BA=E4=BF=AE=E6=AD=A3=20PR=20C=20?= =?UTF-8?q?=E7=9A=84=E6=B3=A8=E9=87=8A=E3=80=81=E6=96=87=E6=A1=A3=E4=B8=8E?= =?UTF-8?q?=E7=94=A8=E4=BE=8B=E4=BF=9D=E7=9C=9F=E5=BA=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 独立 review（对照 606339f / 4099700 静态复核 + 日志逐行核对）提出的问题，逐条修正： 1. **补回被我自己抹掉的源码注释**：上次验证「未修复代码上 4/4 红」时用了 `git checkout 606339f -- src/client/native/index.ts`，随后 `git checkout HEAD -- <同一文件>` 把**尚未提交**的 `disposeSafely` 注释一起还原掉了 —— PR 描述里承诺的 「说明释放失败为何只走 console.error」因此在 diff 里根本不存在。已重新落盘。 2. **事故文档三处事实修正**：同一毫秒的 `already registered` 是 **7 条**（.339 一条 + .340 六条，合计 burst 9 条），不是 6；验证段的「114 files / 1136 passed」是修复分支 （base 早于 #781）的数字，补上口径并给出合入 main 后的 122/1292；`index.ts:353` 标注 为 v0.22.0 行号（main 上因 #777 顺移）。 3. **撤回一处措辞过头**：「部分回滚分支从未被执行」→ 回收集合虽然进了、但 disposer 列表恒为空（释放动作未执行），改后能真正覆盖。 4. **e2e 门不再可能静默通过**：`test.skip(CLIENT_JS === undefined, …)` 改成硬失败—— 这个门的价值就在于 harness 以后不再导出 DSH_HOME 时必须报错而不是变成 skip。 同时把「无 `[data-sidebar-right-unavailable]`」移到文件渲染断言**之后**（前者在空 pane 上恒真，不是真空但也没重量）。 5. **测试替身补齐保真度**：假注册表的 disposer 改为幂等（真宿主的 disposer 是幂等的 ——cordis effect 二次释放返回同一个 task），避免「二次 release 又被记一笔」这种与 真实宿主不一致的行为被后续用例当成事实。 6. **AGENTS §2 的 HMR 机制加版本限定**：`mtime/ctime/size` 的 sha1 rev 是**钉住的宿主 0.1.7-rc.1** 的行为；DSH 源码树新版本改成只比 `mtime+size` 并对内容取 sha1，pin 上调 时该触发器必须同步复核（否则会红在替换断言上，而不是静默失效）。 验证：typecheck / lint 通过；pnpm test 122 files / 1292 passed / 9 skipped（重启前机器上有 两个 scratch dsh 服务时的 fs-watch 超时是负载抖动，单独跑 5/5 绿、清空负载后全量绿）。 --- AGENTS.md | 2 +- ...2026-09-28-native-files-takeover-reload-leak.md | 11 ++++++----- src/client/native/index.ts | 7 +++++++ tests/e2e/native-reload.e2e.ts | 14 ++++++++++++-- tests/native-registration.spec.ts | 6 ++++++ 5 files changed, 32 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 54b892e..39da967 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,7 @@ ## 2. CI 挂载冒烟（`plugin-mount` job / `pnpm test:mount`） -「npm 打包 → 真实挂载 → 无头渲染」门禁（证明打包产物在真实 DSH 挂载后不 crash）：`pnpm build && pnpm pack` 产 tarball → `scripts/e2e-mount.sh` 装进全新 scratch profile（`dsh plugin --profile web add `）并启动真实 `dsh web`（keyless，`--port 0`）→ `tests/e2e/mount.e2e.ts`（Playwright）断言 `[data-dsh-better-sidebar]` 挂载、无错误条/pageerror/console 错误，展开 DSH 原生右侧栏后经其 guide 页逐个打开本插件贡献的 tab 类型**以及宿主自己的终端条目**（必须恰好 1 条——多于 1 条即意味着插件又在遮蔽宿主），再经插件文件树（原生 `files` kind 接管）打开 seed 文件强制加载 editor chunk（`client-editor.js`），并跑 mermaid / README 预览与 sidechat 宿主路由烟测。**`tests/e2e/native-reload.e2e.ts` 是同一 lane 里的第二条门**：它 `utimesSync` 已安装的 `lib/client.js`（宿主 HMR 的 500ms stat-poll 以 `mtime/ctime/size` 的 sha1 作 rev）触发**页面内条目替换**，断言替换前后都无 `native register … error`、无诊断条、且 `files` 接管重新可用——这是唯一能在部署层复现「teardown 期间注册把 id 孤儿化」（见 §3.4 第 9 条）的用例，v0.22.0 上实测 3/3 红、修复后 3/3 绿。**`browser` 不在这个清单里，而是显式断言缺席**：DSH 0.1.7 的 `ui-sidebar-browser` 只在 desktop profile 挂载（`web-app` 的 patch 里 `disabled: ctx.get('profileContext')?.name !== 'desktop'`），lane 全走 web profile，钉住「没有」比默默放过更能暴露宿主立场的变化。 +「npm 打包 → 真实挂载 → 无头渲染」门禁（证明打包产物在真实 DSH 挂载后不 crash）：`pnpm build && pnpm pack` 产 tarball → `scripts/e2e-mount.sh` 装进全新 scratch profile（`dsh plugin --profile web add `）并启动真实 `dsh web`（keyless，`--port 0`）→ `tests/e2e/mount.e2e.ts`（Playwright）断言 `[data-dsh-better-sidebar]` 挂载、无错误条/pageerror/console 错误，展开 DSH 原生右侧栏后经其 guide 页逐个打开本插件贡献的 tab 类型**以及宿主自己的终端条目**（必须恰好 1 条——多于 1 条即意味着插件又在遮蔽宿主），再经插件文件树（原生 `files` kind 接管）打开 seed 文件强制加载 editor chunk（`client-editor.js`），并跑 mermaid / README 预览与 sidechat 宿主路由烟测。**`tests/e2e/native-reload.e2e.ts` 是同一 lane 里的第二条门**：它 `utimesSync` 已安装的 `lib/client.js`（**在钉住的宿主 0.1.7-rc.1 上**：HMR 每 500ms stat-poll，rev = `mtime/ctime/size` 的 sha1；DSH 源码树的新版本改成只比 `mtime+size` 并对**内容**取 sha1——pin 上调时这个触发器必须同步复核，否则该用例会红在替换断言上而不是静默失效）触发**页面内条目替换**，断言替换前后都无 `native register … error`、无诊断条、且 `files` 接管重新可用——这是唯一能在部署层复现「teardown 期间注册把 id 孤儿化」（见 §3.4 第 9 条）的用例，v0.22.0 上实测 3/3 红、修复后 3/3 绿。**`browser` 不在这个清单里，而是显式断言缺席**：DSH 0.1.7 的 `ui-sidebar-browser` 只在 desktop profile 挂载（`web-app` 的 patch 里 `disabled: ctx.get('profileContext')?.name !== 'desktop'`），lane 全走 web profile，钉住「没有」比默默放过更能暴露宿主立场的变化。 本地：`pnpm build && pnpm pack && pnpm exec playwright install chromium && pnpm test:mount`（`DSH_CMD` 未设时 `scripts/e2e-common.sh` 回退的 npx 钉版是 `DSH_NPX_SPEC`，默认 `@deepseek-ai/dsh@0.1.7-rc.1`——**不钉就会解析 `latest`，而 `latest` 仍是插件已不支持的 0.1.5 线**）。CI 钉 `@deepseek-ai/dsh@0.1.7-rc.1`（**pin 一律写精确版本号，不要写 `@alpha`**：rc.1 上线时 npm `alpha` 指的是 **0.1.7-alpha.2**，rc.1 走的是 `next`；`latest` 仍是 0.1.5-rc.3；peer 下限 `^0.1.7-rc.1`）。该步骤用 `NODE_OPTIONS=--max-old-space-size=4096`：钉版自身的传递依赖是浮动 `^` 范围，上游分阶段发布预发布版时（rc.2 于 2026-09-10 的 14:43–14:57 逐包上线）npm 会组出混合 peer 图，3062 条 ERESOLVE 后 OOM（exit 134）；另一个失败模式是对未发布兄弟包 ETARGET，靠「钉一个已完整发布的版本」修掉。**不要加 `--legacy-peer-deps`**：它跳过的正是全局安装必须提供的 peer，`@deepseek-ai/dsh-app-boot` 在 boot 时 require 的 `@deepseek-ai/cordis-plugin-group` 是 peer 而非 dependency，加了它 CLI 直接 `ERR_MODULE_NOT_FOUND`（实测过一次，见 rc.2 计划 C 节）。`ci-windows` 的 `Test` 跑 `pnpm test:windows`（`--maxWorkers=1`）而非 `pnpm test`——多个 spec 真起进程（`smoke` / `install-powershell` / `git`），2 核 runner 上并行起 ConPTY / 冷启 `powershell.exe` 是超时与 worker 静默死亡的放大器；`vitest.config.ts` 的全局 `testTimeout: 15_000`（默认 5000 在 Windows 上对真起进程的用例太低，三个不同文件先后翻车）是配套的一半。e2e spec 命名 `*.e2e.ts` + vitest `exclude` 双保险；**改 `exclude` 必须保留默认排除项**（exclude 整体替换默认值）。 diff --git a/docs/plans/2026-09-28-native-files-takeover-reload-leak.md b/docs/plans/2026-09-28-native-files-takeover-reload-leak.md index df2900c..233a242 100644 --- a/docs/plans/2026-09-28-native-files-takeover-reload-leak.md +++ b/docs/plans/2026-09-28-native-files-takeover-reload-leak.md @@ -21,12 +21,12 @@ native register files error: Error: cannot create effect on inactive context assertActive ← effect ← inject(slots) ← registerSlots ← registerFilesKind ← sync ← notify ``` -随后同一毫秒（.339–.340）**6 条** `already registered`（栈：`register ← registerFilesKind ← sync ← notify`），13 秒后新激活的 `_reload` 路径又报 2 条。链条： +随后同一毫秒（.339–.340）**7 条** `already registered`（栈：`register ← registerFilesKind ← sync ← notify`），13 秒后新激活的 `_reload` 路径又报 2 条（该 burst 合计 9 条，与「7 个内置描述符注销 → 7 次 notify」一一对应）。链条： -1. `service.notify()` 是同步内联（`src/client/service.ts:696`），而 `registerNativeSurface` 把 `sync()` 订阅到它和 store 上（`src/client/native/index.ts:353`）。 +1. `service.notify()` 是同步内联（`src/client/service.ts:696`），而 `registerNativeSurface` 把 `sync()` 订阅到它和 store 上（`src/client/native/index.ts:353`；本文行号按 v0.22.0，main 因 #777 的插入而顺移）。 2. `sync()` 的清理循环把 `live` 里**两类归属不同的条目**一视同仁：描述符类型（键 = descriptor id）与 `files` 接管（键 = `FILES_KIND`，**永远不在 `wanted` 里**）。于是**每次**通知都会释放接管、并在同一轮里重建它。 3. 插件 teardown 时，内置描述符逐个注销 → 每次注销 notify → `sync()` 在**已经 inactive** 的插件 ctx 上运行。此时 `tabs.register` 建在**宿主** ctx 上（宿主 ctx 仍活着）→ id 被取走且返回了 disposer；紧随的 `ctx.slots.inject` 建在**插件** ctx 上 → cordis `assertActive()` 抛 `INACTIVE_EFFECT`（`@deepseek-ai/cordis@4.0.4` `effect()` 首行）→ `live.set(FILES_KIND, …)` 永不执行 → **该 id 在本页生命周期内再也不能注册**。 -4. 之后每次 notify（日志里还有 6 次）都重试同一个被占用的 id → `already registered` 刷屏；新一次激活同样注册不上 → 文件树空态。 +4. 之后每次 notify（同一 burst 里还有 6 次）都重试同一个被占用的 id → `already registered` 刷屏；新一次激活同样注册不上 → 文件树空态。 **两条一般化的不变量**（已写进 [AGENTS.md](../../AGENTS.md) §3.4 第 9 条）： @@ -41,8 +41,9 @@ PR #777（贡献者 @yanzhaohui1999）落地上面两条：`sync()` 的清理循 ## 验证 -- **单测红→绿（实测）**：`tests/native-registration.spec.ts` 在 v0.22.0 源码上 4/4 红，在 PR head 上 4/4 绿；head 上 `pnpm typecheck` / `pnpm lint` / `pnpm test`（114 files / 1136 passed / 9 skipped）全绿。 -- 首版用例的两处断言（「失败后还能重新注册」）写在**活着的 id 集合**上，在未修复代码上会**同样通过**（泄漏的 id 恰好构成同一个集合）。跟进 PR 把它们改写到注册表的**事件日志**上，并把槽位失败判据从 `key` 改成 `name::key` —— 两个槽位共用一个 `key`，按 `key` 失败永远只能失败第一个槽，`registerSlots` 的部分回滚分支从未被执行；改后该分支有覆盖，且 4 例在 v0.22.0 上仍全部红。 +- **单测红→绿（实测）**：`tests/native-registration.spec.ts` 在 v0.22.0 源码上 4/4 红，在修复分支上 4/4 绿；`pnpm typecheck` / `pnpm lint` / `pnpm test` 全绿（修复分支的 base 早于 #781，故为 114 files / 1136 passed；合入 main 后为 122 files / 1292 passed / 9 skipped）。 +- **部署级红→绿（实测）**：`tests/e2e/native-reload.e2e.ts` 在 scratch profile 里对 npm 上的 `dsh-better-sidebar@0.22.0` 触发页面内条目替换（`utimesSync` 已安装的 `lib/client.js`），**3/3 红**且每次都是本文那两条栈；换成本地打包的修复版 **3/3 绿**。该用例已随本 PR 进入挂载 lane（CI `plugin-mount` 绿），触发方式依赖钉住的宿主 0.1.7-rc.1 的 rev 规则（`mtime/ctime/size` 的 sha1）——pin 上调时需同步复核。 +- 首版用例的两处断言（「失败后还能重新注册」）写在**活着的 id 集合**上，在未修复代码上会**同样通过**（泄漏的 id 恰好构成同一个集合）。跟进 PR 把它们改写到注册表的**事件日志**上，并把槽位失败判据从 `key` 改成 `name::key` —— 两个槽位共用一个 `key`，按 `key` 失败时**第一个**槽就抛，回收循环虽然进了、但手里的 disposer 列表永远是空的（即 `registerSlots` 的部分回滚**释放动作**从未被执行）；改后可以让某个描述符的**第二个**槽失败，回收列表非空、释放路径真正被覆盖，且 4 例在 v0.22.0 上仍全部红。 ## 已知限制 / 未做 diff --git a/src/client/native/index.ts b/src/client/native/index.ts index aa70b08..44b23ed 100644 --- a/src/client/native/index.ts +++ b/src/client/native/index.ts @@ -160,6 +160,13 @@ interface Registration { * 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. + * + * A release failure is logged, NOT routed through `reportFailure` (the visible + * diagnostic strip), on purpose: both callers run while the surface is being + * taken down — the drop loop and the seat teardown — where a strip would just + * be noise, and an id that really stays taken cannot hide anyway: the next + * registration attempt for it goes through the strip as + * `register … error: … already registered`. * @param dispose - the disposer to run. * @param what - the registration's name, for the log line. */ diff --git a/tests/e2e/native-reload.e2e.ts b/tests/e2e/native-reload.e2e.ts index 3c772d1..5087107 100644 --- a/tests/e2e/native-reload.e2e.ts +++ b/tests/e2e/native-reload.e2e.ts @@ -65,7 +65,11 @@ test.afterAll(async () => { }) test('the native tab types survive an in-page client entry replacement', async ({ page }) => { - test.skip(CLIENT_JS === undefined, 'set DSH_HOME or DSH_E2E_PLUGIN_CLIENT_JS to locate the installed bundle') + // 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()) @@ -145,6 +149,12 @@ test('the native tab types survive an in-page client entry replacement', async ( 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() - await expect(pane.locator('[data-sidebar-right-unavailable]')).toHaveCount(0) + // 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) }) diff --git a/tests/native-registration.spec.ts b/tests/native-registration.spec.ts index cc299e3..3996b69 100644 --- a/tests/native-registration.spec.ts +++ b/tests/native-registration.spec.ts @@ -51,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}`) }