From 54c60f40791ea798c1b214735e601ce36d9128f2 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Thu, 30 Jul 2026 23:23:18 +0800 Subject: [PATCH] fix(llm,settings): refuse post-disposal route replacement and teardown notifications Two lifecycle holes the registry and the consumer helper left open. `AdapterRegistrationHandle.replace` had no liveness guard: after the handle's disposer ran, a replace put routes back into the registry with nothing left to release them, so the adapter leaked permanently. `owned` being empty cannot carry that fact, because `replace([])` is the legal empty-section state, so the disposer records it explicitly. `installSettingsSection`'s watcher lacked the guard its own disposer carries: a stored change landing while the consumer unloads reached `onChange`, which re-registers routes against a fiber whose resources are being released. Also documents `withFileLock` in the atomic-write README (it claimed one export), records the age-based lock takeover as a known limitation, and lists ctx.settings and ctx.credentials in the architecture capability table. --- docs/architecture.i18n.yaml | 4 +-- docs/architecture.md | 2 ++ docs/architecture.zh.md | 2 ++ docs/cordis-catalog/services.md | 2 +- docs/core-data-structures/core.i18n.yaml | 4 +-- docs/core-data-structures/core.md | 4 +++ docs/core-data-structures/core.zh.md | 4 +++ packages/llm/llm/src/index.ts | 13 ++++++++ packages/llm/llm/tests/service.spec.ts | 28 ++++++++++++++++ packages/settings/settings/src/index.ts | 5 +++ .../settings/settings/tests/settings.spec.ts | 32 +++++++++++++++++++ packages/util/atomic-write/README.i18n.yaml | 4 +-- packages/util/atomic-write/README.md | 13 ++++++-- packages/util/atomic-write/README.zh.md | 13 ++++++-- 14 files changed, 119 insertions(+), 11 deletions(-) diff --git a/docs/architecture.i18n.yaml b/docs/architecture.i18n.yaml index dfff50708f..d7054b8b3c 100644 --- a/docs/architecture.i18n.yaml +++ b/docs/architecture.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write docs/architecture.md -architecture.md: 1fd9bd128d1bcc0dd91d46131981ea4fc331bd74 -architecture.zh.md: 8521f09c6e415f9f8d1c0a44f7534b59c876decc +architecture.md: bfea67b9f83958e16b58e63b99e326349f6eff15 +architecture.zh.md: c2fd6cdd84ad2f6435faebffa0c4c1a6da0ade96 diff --git a/docs/architecture.md b/docs/architecture.md index 1fd9bd128d..bfea67b9f8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -46,6 +46,8 @@ Harnesses are [Cordis](cordis-primer.md) contexts; packages contribute services, | `ctx.sessionPersistence` | [`session-persistence/`](../packages/session-persistence/README.md) | durable session-log storage | | `ctx.sessionQuery` | [`session-query/`](../packages/session-query/README.md) | live-preferred exact/filter/trace queries over SQLite FTS, workspace-authorized model tools | | `ctx.sessionTitle` | [`session-title/`](../packages/session-title/README.md) | log-backed fallbacks, one optional asynchronous provider | +| `ctx.settings` | [`settings/`](../packages/settings/README.md) | per-plugin user-settings namespaces layered over composition entries | +| `ctx.credentials` | [`credentials/`](../packages/credentials/README.md) | named secret references resolved per operation, never inlined in configuration | | `ctx.directoryPicker` | [`host/directory-picker`](../packages/host/directory-picker/README.md) | GUI-host directory picking (`native`/`browse` interactions) | | `ctx.typert` | [`typert/registry`](../packages/typert/registry/README.md) | runtime registry for generated package reflection and live Zod schemas | | `ctx.invariants` | [`support/invariants`](../packages/support/invariants/README.md) | package-name-selected registry of package-owned runtime checks | diff --git a/docs/architecture.zh.md b/docs/architecture.zh.md index 8521f09c6e..c2fd6cdd84 100644 --- a/docs/architecture.zh.md +++ b/docs/architecture.zh.md @@ -46,6 +46,8 @@ | `ctx.sessionPersistence` | [`session-persistence/`](../packages/session-persistence/README.md) | 会话日志的持久化存储 | | `ctx.sessionQuery` | [`session-query/`](../packages/session-query/README.md) | 基于 SQLite 全文搜索的实时优先精确检索／过滤／追踪、经工作区授权的模型工具 | | `ctx.sessionTitle` | [`session-title/`](../packages/session-title/README.md) | 基于日志的回退标题和单个可选异步提供方 | +| `ctx.settings` | [`settings/`](../packages/settings/README.md) | 按插件划分的用户设置命名空间，分层叠加在装配条目之上 | +| `ctx.credentials` | [`credentials/`](../packages/credentials/README.md) | 具名密钥引用，按操作解析，绝不内联进配置 | | `ctx.directoryPicker` | [`host/directory-picker`](../packages/host/directory-picker/README.md) | GUI 宿主目录选取（`native`／`browse` 交互） | | `ctx.typert` | [`typert/registry`](../packages/typert/registry/README.md) | 生成的包反射和实时 Zod schema 的运行时注册表 | | `ctx.invariants` | [`support/invariants`](../packages/support/invariants/README.md) | 按包名筛选包自有运行时检查的注册表 | diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index 271cbf2720..60ba4414ea 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -866,7 +866,7 @@ stream(options: GenerateOptions): AsyncIterable Types: [AdapterRegistrationHandle](../core-data-structures/core.md) · [GenerateOptions](../core-data-structures/core.md) · [LlmAdapter](../core-data-structures/llm-streaming.md) · [LlmCallConfig](../core-data-structures/core.md) · [LlmModelInfo](../core-data-structures/core.md) · [LlmProviderInfo](../core-data-structures/core.md) · [LlmResolvedModelInfo](../core-data-structures/core.md) · [PreparedLlmCall](../core-data-structures/llm-streaming.md) · [ResolvedRetryPolicy](../core-data-structures/llm-streaming.md) · [StreamChunk](../core-data-structures/llm-streaming.md) -Source: [`packages/llm/llm/src/index.ts:211`](../../packages/llm/llm/src/index.ts) +Source: [`packages/llm/llm/src/index.ts:215`](../../packages/llm/llm/src/index.ts) ## `ctx.permission` — `PermissionService` diff --git a/docs/core-data-structures/core.i18n.yaml b/docs/core-data-structures/core.i18n.yaml index c8cb8302d1..e998f1f4a4 100644 --- a/docs/core-data-structures/core.i18n.yaml +++ b/docs/core-data-structures/core.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write docs/core-data-structures/core.md -core.md: 5c79f454f50a059d72a592df45d504ee78835e0b -core.zh.md: 258517c625822bdbd64138baf3df186b075bb5c6 +core.md: 09b437a8483134230d4b941c20940c5655bc53f0 +core.zh.md: 2025707db397203dbaec52c59172f83367f2033e diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index 5c79f454f5..09b437a848 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -201,6 +201,10 @@ interface AdapterRegistrationHandle { * one synchronous section, so no request can observe a gap. An empty array * is legal here (a settings section that emptied holds zero routes while * staying registered), unlike an empty initial registration. + * + * Throws `LlmError` with code `REGISTRATION_DISPOSED` once the registration + * has been released: its routes are gone and its disposer has already run, + * so anything registered afterwards would have no owner left to release it. * @param providers - the complete next route set for this registration. */ replace(providers: string[]): void diff --git a/docs/core-data-structures/core.zh.md b/docs/core-data-structures/core.zh.md index 258517c625..2025707db3 100644 --- a/docs/core-data-structures/core.zh.md +++ b/docs/core-data-structures/core.zh.md @@ -207,6 +207,10 @@ interface AdapterRegistrationHandle { * one synchronous section, so no request can observe a gap. An empty array * is legal here (a settings section that emptied holds zero routes while * staying registered), unlike an empty initial registration. + * + * Throws `LlmError` with code `REGISTRATION_DISPOSED` once the registration + * has been released: its routes are gone and its disposer has already run, + * so anything registered afterwards would have no owner left to release it. * @param providers - the complete next route set for this registration. */ replace(providers: string[]): void diff --git a/packages/llm/llm/src/index.ts b/packages/llm/llm/src/index.ts index 73ac6ed900..3759f9f8df 100644 --- a/packages/llm/llm/src/index.ts +++ b/packages/llm/llm/src/index.ts @@ -199,6 +199,10 @@ export interface AdapterRegistrationHandle { * one synchronous section, so no request can observe a gap. An empty array * is legal here (a settings section that emptied holds zero routes while * staying registered), unlike an empty initial registration. + * + * Throws `LlmError` with code `REGISTRATION_DISPOSED` once the registration + * has been released: its routes are gone and its disposer has already run, + * so anything registered afterwards would have no owner left to release it. * @param providers - the complete next route set for this registration. */ replace(providers: string[]): void @@ -227,10 +231,14 @@ export class LlmService extends Service { // The routes this registration currently holds; `replace` rewrites it, and // the disposer releases whatever it holds at disposal time. const owned = new Set() + // The disposer has run: `owned` being empty cannot say so on its own, + // because `replace([])` legally leaves a live registration holding none. + let released = false const dispose = this.ctx.effect(function* (this: LlmService) { if (providers.length === 0) throw new LlmError('an adapter must register at least one provider', 'INVALID_ADAPTER') this.commitRoutes(owned, this.prepareRoutes(providers, adapter, owned)) yield () => { + released = true for (const provider of owned) this.adapters.delete(provider) owned.clear() } @@ -239,6 +247,11 @@ export class LlmService extends Service { // synchronous fire-and-forget — discard the (always-resolved) promise. const handle = (() => void dispose()) as AdapterRegistrationHandle handle.replace = (next: string[]): void => { + // Registering here would leak: the effect's disposer already ran, so + // nothing remains to release whatever this call would put in the map. + if (released) { + throw new LlmError('a disposed adapter registration cannot replace its routes', 'REGISTRATION_DISPOSED') + } this.commitRoutes(owned, this.prepareRoutes(next, adapter, owned)) } return handle diff --git a/packages/llm/llm/tests/service.spec.ts b/packages/llm/llm/tests/service.spec.ts index 1afb3c4b6d..f723c7f474 100644 --- a/packages/llm/llm/tests/service.spec.ts +++ b/packages/llm/llm/tests/service.spec.ts @@ -1381,4 +1381,32 @@ describe('LlmService', () => { disposeAgain() expect(ctx.llm.listProviders()).toEqual([]) }) + + it('refuses to replace routes on a registration that was already released', async () => { + // The leak this prevents: the effect's disposer has run, so a route added + // afterwards would sit in the registry with nothing left to release it. + const ctx = new Context() + await ctx.plugin(LlmService) + + const handle = ctx.llm.registerAdapter(['m1'], new ScriptedAdapter(SCRIPT)) + handle() + expect(() => { handle.replace(['leaked']) }) + .toThrow(/disposed adapter registration cannot replace its routes/) + expect(ctx.llm.listProviders()).toEqual([]) + }) + + it('still allows an empty route set on a live registration', async () => { + // `replace([])` is the settings-section-emptied case: legal, and it must + // not be mistaken for disposal by the guard above. + const ctx = new Context() + await ctx.plugin(LlmService) + + const handle = ctx.llm.registerAdapter(['m1'], new ScriptedAdapter(SCRIPT)) + handle.replace([]) + expect(ctx.llm.listProviders()).toEqual([]) + handle.replace(['m2']) + expect(ctx.llm.listProviders()).toEqual([{ id: 'm2', name: 'm2' }]) + handle() + expect(ctx.llm.listProviders()).toEqual([]) + }) }) diff --git a/packages/settings/settings/src/index.ts b/packages/settings/settings/src/index.ts index 9293564335..241bc41bfa 100644 --- a/packages/settings/settings/src/index.ts +++ b/packages/settings/settings/src/index.ts @@ -612,6 +612,11 @@ export function installSettingsSection( }) hooks.onChange() scope.watch(() => { + // A stored change landing while the consumer unloads reaches the watcher + // before the registration is released, and `onChange` is exactly as + // harmful here as in the disposer above: it re-registers routes against + // a fiber whose resources are being let go. + if (isUnloading(ctx)) return hooks.onChange() }) }) diff --git a/packages/settings/settings/tests/settings.spec.ts b/packages/settings/settings/tests/settings.spec.ts index 5f9ac7dae7..9e3a8dd7e5 100644 --- a/packages/settings/settings/tests/settings.spec.ts +++ b/packages/settings/settings/tests/settings.spec.ts @@ -724,4 +724,36 @@ describe('installSettingsSection', () => { await new Promise(resolve => setTimeout(resolve, 20)) expect(changes).toEqual(['user']) }) + + it('stays silent for a stored change that lands while the consumer unloads', async () => { + // The watcher outlives the start of teardown by the width of the unload, + // so a document change arriving in that window reaches it. Notifying then + // is exactly as harmful as notifying from the disposer. + const { ctx, provider } = await boot({ doc: { 'helper-ns': { theme: 'user' } } }) + const entry = { theme: 'entry' } + let current: () => { theme: string } = () => entry + const changes: string[] = [] + const consumer = ctx.plugin({ + inject: ['settings'], + apply: (child: Context) => { + installSettingsSection(child, settingsNamespace('helper-ns'), HelperSchema, entry, { + setSource: (source) => { + current = source + }, + onChange: () => { + changes.push(current().theme) + }, + }) + }, + }) + await consumer + await vi.waitFor(() => { + expect(changes).toEqual(['user']) + }) + + const unloading = consumer.dispose() + provider.pushExternal({ 'helper-ns': { theme: 'racing' } }) + await unloading + expect(changes).toEqual(['user']) + }) }) diff --git a/packages/util/atomic-write/README.i18n.yaml b/packages/util/atomic-write/README.i18n.yaml index e1f2ea37f5..ffa4d7ccbb 100644 --- a/packages/util/atomic-write/README.i18n.yaml +++ b/packages/util/atomic-write/README.i18n.yaml @@ -2,5 +2,5 @@ # side as of the last confirmed-consistent state. Both languages carry equal authority; # after editing either side, bring the other along and re-record with: # pnpm run verify-translation-pairing --write packages/util/atomic-write/README.md -README.md: 2cd57a0fa42601e393a41de68af3f9b1e2f033b5 -README.zh.md: e8f18a8ec6ef6077f15cebed0062fabc0638ee0e +README.md: be9f896eb24e28aedc2c04858da8b8da9da548dc +README.zh.md: 19a067dc84f12d334e5c31dda58e7cf78dac51f9 diff --git a/packages/util/atomic-write/README.md b/packages/util/atomic-write/README.md index 2cd57a0fa4..be9f896eb2 100644 --- a/packages/util/atomic-write/README.md +++ b/packages/util/atomic-write/README.md @@ -7,14 +7,20 @@ Zero-dependency atomic file replacement shared by file-backed stores that must n ## Surface ```ts -import { writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' +import { withFileLock, writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' declare const text: string +declare const render: (previous: string) => string await writeFileAtomic('/home/u/.dsh/settings.yaml', text, { mode: 0o600 }) + +// Read-modify-write against the same file from several processes. +await withFileLock('/home/u/.dsh/settings.yaml', async () => { + await writeFileAtomic('/home/u/.dsh/settings.yaml', render(text), { mode: 0o600 }) +}) ``` -One export. The contract, in the order failures would exploit it: +`writeFileAtomic` commits one already-rendered string. The contract, in the order failures would exploit it: - **Exclusive-create temp** (`wx`, random suffix): the open refuses to follow a symlink planted at a guessable temp path. - **The fresh inode carries `mode` through the rename**: replacing a wider-permission file narrows it without a chmod race. `mode` is required so the permission decision stays visible at every call site (subject to the process umask, like every fresh inode). @@ -22,6 +28,8 @@ One export. The contract, in the order failures would exploit it: - **Same-directory sibling** keeps the rename on one filesystem, so the swap stays atomic. - Parent directories are created; on any failure the temp is removed and the failure rethrown; readers observe either the old or the new complete content. +`withFileLock` serializes the writers of one file across processes, for the read-render-commit cycles a bare atomic commit cannot make safe on its own. The lock is a `wx`-created `.lock` sibling, so readers never contend; waiters back off exponentially and fail with a timeout rather than block forever. A lock older than the stale age is treated as a crashed holder and broken — see [Known Limitations and Deferred Work](#known-limitations-and-deferred-work) for what that costs. + ## Model Experience None, as this is a pure filesystem primitive; nothing here reaches a model request. @@ -34,3 +42,4 @@ None; nothing here enters a request prefix. - **Atomic, not durable** — no `fsync` of the file or its directory, so after a crash the rename may be observed unwound. The file-backed stores here re-read and republish on boot, keeping durability the caller's policy. - **String content only** — no `Buffer` or stream form until a consumer needs one. +- **The lock takes over by age, not by ownership** (`TODO(settings-lock-ownership)`) — a holder slower than the stale age has its lock broken by a waiter, and release unlinks the path unconditionally, so a slow writer can remove a successor's lock. Two writers can then overlap and one cycle's result be lost. The stale age is set well above any write this repo performs, so the exposure is a paused or swapped-out process; ownership-safe acquisition and release is the fix. diff --git a/packages/util/atomic-write/README.zh.md b/packages/util/atomic-write/README.zh.md index e8f18a8ec6..19a067dc84 100644 --- a/packages/util/atomic-write/README.zh.md +++ b/packages/util/atomic-write/README.zh.md @@ -7,14 +7,20 @@ ## 接口面 ```ts -import { writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' +import { withFileLock, writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' declare const text: string +declare const render: (previous: string) => string await writeFileAtomic('/home/u/.dsh/settings.yaml', text, { mode: 0o600 }) + +// Read-modify-write against the same file from several processes. +await withFileLock('/home/u/.dsh/settings.yaml', async () => { + await writeFileAtomic('/home/u/.dsh/settings.yaml', render(text), { mode: 0o600 }) +}) ``` -仅一个导出。契约按故障利用它的先后顺序列出： +`writeFileAtomic` 提交一份已经渲染好的字符串。契约按故障利用它的先后顺序列出： - **独占创建临时文件**（`wx` + 随机后缀）：open 拒绝跟随预先埋在可猜测临时路径上的符号链接。 - **全新 inode 携带 `mode` 走完 rename**：替换权限过宽的旧文件时直接收窄，不存在 chmod 竞态。`mode` 为必填，让权限决策始终可见于每个调用点（与所有新建 inode 一样受进程 umask 影响）。 @@ -22,6 +28,8 @@ await writeFileAtomic('/home/u/.dsh/settings.yaml', text, { mode: 0o600 }) - **同目录兄弟文件**保证 rename 落在同一文件系统上，交换保持原子。 - 自动创建父目录；任何失败都会移除临时文件并重新抛出该失败；读取方只会观察到旧内容或完整的新内容。 +`withFileLock` 跨进程串行化同一文件的写入方，服务于单靠原子提交无法保证安全的读-渲染-提交循环。锁是以 `wx` 创建的同目录 `.lock`，因此读取方从不参与竞争；等待方按指数退避，超时即失败而非无限阻塞。超过陈旧时限的锁被视为持有者已崩溃并被打破——其代价见[Known Limitations and Deferred Work](#known-limitations-and-deferred-work)。 + ## Model Experience 无：本包是纯文件系统原语，此处没有任何内容会到达模型请求。 @@ -34,3 +42,4 @@ await writeFileAtomic('/home/u/.dsh/settings.yaml', text, { mode: 0o600 }) - **原子但不保证持久**——不对文件或其所在目录做 `fsync`，因此崩溃后可能观察到 rename 被回退。此处的文件型存储在启动时重新读取并重新发布，把持久性留作调用方的策略。 - **仅支持字符串内容**——在有消费方需要之前，不提供 `Buffer` 或流式形态。 +- **锁按时长而非归属接管**（`TODO(settings-lock-ownership)`）——持有者若慢于陈旧时限，其锁会被等待方打破，而释放又无条件删除该路径，因此慢写入方可能删掉后继者的锁。两个写入方随之重叠，一轮循环的结果可能丢失。陈旧时限远高于本仓库的任何一次写入，因此暴露面是被暂停或被换出的进程；修法是按归属安全地获取与释放。