From 1010291fe6cd764deff8e5e3056bb57f94c3fa4f Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Wed, 29 Jul 2026 10:07:28 +0800 Subject: [PATCH] fix(settings): close cross-namespace, dispatch, and lifecycle races from second review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirmed and fixed, each with a regression test that failed first: - Concurrent writes to different namespaces lost whole sections on disk (each persist rendered the full document from a stale text): the local provider serializes render->write->rename->text-commit on one internal persist chain shared by every namespace queue. - One throwing settings/updated listener starved the rest (cordis emit stops at the first throw): commit fans out per listener via events.dispatch, contains individual failures, and rethrows the first INVARIANT-coded error only after every listener ran. - Write queues ignored fiber/service lifecycle: the base init now registers a teardown that refuses new writes and drains queued chains; queued tasks re-verify service liveness and namespace ownership before running and again before committing, so a registrant disposed mid-flight is never notified and a disposed service never commits. - Async watcher invocations could interleave (a slow stale call applied last): each watcher carries a serialized invocation chain — one call at a time, in commit order; JSDoc/doc pages state the async timing. - update/replace borrowed the caller's object until the queued task ran: inputs are structured-clone snapshotted at call time; non-cloneable plain objects reject with a typed error. - Composition guard now proves the documented fallback: the consumer uses the optional scoped-inject shape and boots both with the settings entry (hot publish) and without it (entry-config resolution, no scope). - core-data-structures index: settings.md row added to the sub-page table in core.md/core.zh.md. Both packages hold per-file 100% coverage across repeated runs. --- docs/cordis-catalog/events.md | 2 +- docs/cordis-catalog/services.md | 2 +- docs/core-data-structures/core.i18n.yaml | 4 +- docs/core-data-structures/core.md | 1 + docs/core-data-structures/core.zh.md | 1 + docs/core-data-structures/settings.i18n.yaml | 4 +- docs/core-data-structures/settings.md | 5 +- docs/core-data-structures/settings.zh.md | 5 +- docs/event-producer-consumer.md | 2 +- .../settings/settings-local/README.i18n.yaml | 4 +- packages/settings/settings-local/README.md | 1 + packages/settings/settings-local/README.zh.md | 1 + packages/settings/settings-local/src/index.ts | 14 +- .../tests/loader-composition.spec.ts | 63 +++++-- .../settings-local/tests/local.spec.ts | 20 +++ packages/settings/settings/README.i18n.yaml | 4 +- packages/settings/settings/README.md | 3 +- packages/settings/settings/README.zh.md | 3 +- packages/settings/settings/src/index.ts | 114 +++++++++---- .../settings/settings/tests/settings.spec.ts | 157 +++++++++++++++++- 20 files changed, 339 insertions(+), 71 deletions(-) diff --git a/docs/cordis-catalog/events.md b/docs/cordis-catalog/events.md index 74e1e6cfda..44fcc62e88 100644 --- a/docs/cordis-catalog/events.md +++ b/docs/cordis-catalog/events.md @@ -662,7 +662,7 @@ Committed change to one registered namespace's resolved value. Emitted after the Types: [SettingsNamespace](../core-data-structures/settings.md) · [SettingsUpdateSource](../core-data-structures/settings.md) -Source: [`packages/settings/settings/src/index.ts:96`](../../packages/settings/settings/src/index.ts) +Source: [`packages/settings/settings/src/index.ts:97`](../../packages/settings/settings/src/index.ts) ## `slash/*` diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index e915952ffb..c2e5451699 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -1442,7 +1442,7 @@ async replace(ns: SettingsNamespace, section: object): Promise Types: [SettingsDescriptor](../core-data-structures/settings.md) · [SettingsNamespace](../core-data-structures/settings.md) · [SettingsRegisterOptions](../core-data-structures/settings.md) · [SettingsScope](../core-data-structures/settings.md) -Source: [`packages/settings/settings/src/index.ts:168`](../../packages/settings/settings/src/index.ts) +Source: [`packages/settings/settings/src/index.ts:176`](../../packages/settings/settings/src/index.ts) ## `ctx.skills` — `SkillService` diff --git a/docs/core-data-structures/core.i18n.yaml b/docs/core-data-structures/core.i18n.yaml index f0d078c123..d0bdbc5dd2 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: 647c7f273183e0890ef191d98ab009ad129db572 -core.zh.md: 8b0f439f09a6e6609dbe69c3056aa74d553a0943 +core.md: ca8426e6fbece18a277cc31f5e86d3058b3feb32 +core.zh.md: 6b297ca233428d38dbe022e58344e98fab7a15ad diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index 647c7f2731..ca8426e6fb 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -24,6 +24,7 @@ Everything else is documented on a **sub-page**, not here. The rule that draws t | [commands.md](commands.md) | the human-command seam: definitions, adapter discovery, direct invocation, results, and parsing views | | [session.md](session.md) | the full `SessionEventMap` variant catalog, `TurnTrigger`/`TurnEndReason`, `deriveMessages()`, execution enclosure, and standalone events | | [persistence.md](persistence.md) | the durability seam: `SessionPersistence`, JSONL + SQLite backends, `session/flush`, crash recovery, `SessionHeader` | +| [settings.md](settings.md) | the user-settings seam: `SettingsNamespace` registration, layered resolution (defaults → composition `base` → user document), owner scopes, hot commits | | [session-query.md](session-query.md) | logical records, bounded exact-event reads, relationship traces, semantic filters/documents, and full-text result pages | | [session-title.md](session-title.md) | durable title snapshots, source provenance, and the asynchronous provider contract | | [system-prompt.md](system-prompt.md) | per-assembly context, tool-provider results, prompt sections, and cooperative assembly | diff --git a/docs/core-data-structures/core.zh.md b/docs/core-data-structures/core.zh.md index 8b0f439f09..6b297ca233 100644 --- a/docs/core-data-structures/core.zh.md +++ b/docs/core-data-structures/core.zh.md @@ -24,6 +24,7 @@ harness 是一个微内核:一个极小的核心加上众多插件。大多数 | [commands.md](commands.md) | 人类命令 seam:定义、适配器发现、直接调用、结果与解析视图 | | [session.md](session.md) | 完整的 `SessionEventMap` 变体目录、`TurnTrigger`/`TurnEndReason`、`deriveMessages()`、执行封闭与独立事件 | | [persistence.md](persistence.md) | 持久性 seam:`SessionPersistence`、JSONL + SQLite 后端、`session/flush`、崩溃恢复、`SessionHeader` | +| [settings.md](settings.md) | 用户设置 seam:`SettingsNamespace` 注册、分层解析(默认值 → 组合 `base` → 用户文档)、owner scope、热提交 | | [session-query.md](session-query.md) | 逻辑记录、有界精确事件读取、关系追踪、语义筛选器/文档与全文检索结果页 | | [session-title.md](session-title.md) | 持久标题快照、来源 provenance 与异步提供方契约 | | [system-prompt.md](system-prompt.md) | 逐次组装的上下文、工具提供方结果、提示词段落与协作式组装 | diff --git a/docs/core-data-structures/settings.i18n.yaml b/docs/core-data-structures/settings.i18n.yaml index 6dc8750dfb..cca43c251b 100644 --- a/docs/core-data-structures/settings.i18n.yaml +++ b/docs/core-data-structures/settings.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/settings.md -settings.md: 851087065c627f2390041dd6e69273e31be3917d -settings.zh.md: 955c4fbf7c147b3d0a8be62c33c031d7bd2c4ba2 +settings.md: abbfecb35f67b27e16dffb9558a35cf368c90beb +settings.zh.md: c746e3cc181f8347cbe634b9231cfee1b3beecd3 diff --git a/docs/core-data-structures/settings.md b/docs/core-data-structures/settings.md index 851087065c..abbfecb35f 100644 --- a/docs/core-data-structures/settings.md +++ b/docs/core-data-structures/settings.md @@ -46,8 +46,9 @@ interface SettingsScope { /** Current resolved value: schema defaults, then `base`, then the user layer. */ get(): T /** - * Observe committed changes to this namespace's resolved value. A callback - * may be async; a rejection is contained and logged like a sync throw. + * Observe committed changes to this namespace's resolved value. Invocations + * of one callback run asynchronously, one at a time, in commit order; a + * rejection is contained and logged like a sync throw. * @param callback - invoked after each commit with the next and previous values. * @returns the disposer removing this observer. */ diff --git a/docs/core-data-structures/settings.zh.md b/docs/core-data-structures/settings.zh.md index 955c4fbf7c..c746e3cc18 100644 --- a/docs/core-data-structures/settings.zh.md +++ b/docs/core-data-structures/settings.zh.md @@ -46,8 +46,9 @@ interface SettingsScope { /** Current resolved value: schema defaults, then `base`, then the user layer. */ get(): T /** - * Observe committed changes to this namespace's resolved value. A callback - * may be async; a rejection is contained and logged like a sync throw. + * Observe committed changes to this namespace's resolved value. Invocations + * of one callback run asynchronously, one at a time, in commit order; a + * rejection is contained and logged like a sync throw. * @param callback - invoked after each commit with the next and previous values. * @returns the disposer removing this observer. */ diff --git a/docs/event-producer-consumer.md b/docs/event-producer-consumer.md index e3eb1d9435..74645a1674 100644 --- a/docs/event-producer-consumer.md +++ b/docs/event-producer-consumer.md @@ -35,7 +35,7 @@ This matrix shows which packages dispatch each harness-owned event and which pac | `session/disposed` | `emit` | [`packages/core/session/src/index.ts:80`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`agent-loop`](../packages/core/agent-loop), `apiproxy`, [`session-persistence`](../packages/session-persistence/session-persistence), [`session-telemetry`](../packages/telemetry/session-telemetry), [`session-title`](../packages/session-title/session-title) | | `session/event` | `emit` | [`packages/core/session/src/index.ts:92`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`acp`](../packages/acp/acp), `apiproxy`, [`cli-demo`](../packages/examples/cli-demo), [`compact`](../packages/compact/compact), [`compact-basic`](../packages/compact/compact-basic), [`goal`](../packages/goal/goal), [`goal-session`](../packages/goal/goal-session), [`hook-protocol`](../packages/hooks/hook-protocol), [`jsonrpc`](../packages/ui/jsonrpc), [`plan-mode`](../packages/plan/plan-mode), [`session`](../packages/core/session), [`session-persistence`](../packages/session-persistence/session-persistence), [`session-telemetry`](../packages/telemetry/session-telemetry), [`session-title`](../packages/session-title/session-title), [`token-meter`](../packages/llm/token-meter), [`tools`](../packages/core/tools), [`tui`](../packages/ui/tui), [`user-approval`](../packages/ui/user-approval), [`workspace-context`](../packages/context/workspace-context) | | `session/flush` | `parallel` | [`packages/core/session/src/index.ts:102`](../packages/core/session/src/index.ts) | [`session`](../packages/core/session) (`events.dispatch`) | [`session-persistence`](../packages/session-persistence/session-persistence), [`session-telemetry`](../packages/telemetry/session-telemetry) | -| `settings/updated` | `emit` | [`packages/settings/settings/src/index.ts:96`](../packages/settings/settings/src/index.ts) | [`settings`](../packages/settings/settings) (`emit`) | [`settings`](../packages/settings/settings) | +| `settings/updated` | `emit` | [`packages/settings/settings/src/index.ts:97`](../packages/settings/settings/src/index.ts) | [`settings`](../packages/settings/settings) (`events.dispatch`) | [`settings`](../packages/settings/settings) | | `slash/input-begin-command` | `bail` | [`packages/client/ui-slash/src/types.ts:230`](../packages/client/ui-slash/src/types.ts) | - | `ui-conversation` | | `slash/input-consume-token` | `bail` | [`packages/client/ui-slash/src/types.ts:244`](../packages/client/ui-slash/src/types.ts) | - | `ui-conversation` | | `slash/input-insert-reference` | `bail` | [`packages/client/ui-slash/src/types.ts:237`](../packages/client/ui-slash/src/types.ts) | - | `ui-conversation` | diff --git a/packages/settings/settings-local/README.i18n.yaml b/packages/settings/settings-local/README.i18n.yaml index 455255941a..5d44f50f9d 100644 --- a/packages/settings/settings-local/README.i18n.yaml +++ b/packages/settings/settings-local/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/settings/settings-local/README.md -README.md: 9d0aa3982507ca16b382aaa918ca1536a98e29ea -README.zh.md: 075de7ee5b3e0ef0a4ee27eeb8cb098ca0d73456 +README.md: af8df7c030757b330e034a1c46507fbe75c9bab8 +README.zh.md: fc8943263b339baad1a92a1d0b0977b926e40f6e diff --git a/packages/settings/settings-local/README.md b/packages/settings/settings-local/README.md index 9d0aa39825..af8df7c030 100644 --- a/packages/settings/settings-local/README.md +++ b/packages/settings/settings-local/README.md @@ -19,6 +19,7 @@ Defaulting is one explicit `resolveSpec(config)` step; an unsupported extension - **Boot fails loud, reload keeps last-good.** An existing-but-invalid document fails plugin load; once live, an unreadable or unparsable edit warns and keeps the last good sections. A missing document resolves every namespace from defaults and `base`; deleting it publishes the same empty state. - **Write-back is atomic, owner-only, and symlink-proof.** `persist` exclusive-creates a random-suffix temp sibling with mode `0600` (`wx` refuses to follow a planted symlink) and renames over the target, cleaning the temp up on failure. YAML writes patch one namespace in the comment-preserving document; JSON re-serializes. +- **Cross-namespace writes serialize on one document.** Every namespace shares the file, so persists from different namespace queues chain internally; each render sees the text the previous write committed. - **Dispose quiesces.** Teardown stops accepting watcher events, closes the watcher, then waits out any queued or in-flight reload, so nothing publishes after disposal. - **Self-write suppression by content.** The provider caches the last good text; a watcher event whose content equals the cache (its own write included) is a no-op. diff --git a/packages/settings/settings-local/README.zh.md b/packages/settings/settings-local/README.zh.md index 075de7ee5b..fc8943263b 100644 --- a/packages/settings/settings-local/README.zh.md +++ b/packages/settings/settings-local/README.zh.md @@ -19,6 +19,7 @@ - **启动报错响亮,重载保留最后可用值。** 存在但非法的文档使插件加载失败;运行中不可读或不可解析的编辑只告警并保留最后可用分节。文档缺失时所有 namespace 按默认值与 `base` 解析;删除文档发布同样的空状态。 - **写回原子、仅属主可读、抗符号链接。** `persist` 以 `0600` 权限独占创建随机后缀临时同级文件(`wx` 拒绝跟随预埋符号链接)后 rename 覆盖目标,失败时清理临时文件。YAML 写回在保留注释的文档里只修补目标 namespace;JSON 重新序列化。 +- **跨 namespace 写入在同一文档上串行。** 所有 namespace 共享一个文件,来自不同 namespace 队列的 persist 在内部串联;每次渲染都基于上一次写入提交后的文本。 - **Dispose 保证静止。** 卸载先停止接收 watcher 事件、关闭 watcher,再等完排队与进行中的重载,之后不再有任何发布。 - **按内容抑制自写。** provider 缓存最后可用文本;watcher 事件内容与缓存相同(含自己的写入)即为 no-op。 diff --git a/packages/settings/settings-local/src/index.ts b/packages/settings/settings-local/src/index.ts index 2c020da63c..b305f61fe7 100644 --- a/packages/settings/settings-local/src/index.ts +++ b/packages/settings/settings-local/src/index.ts @@ -87,6 +87,8 @@ export class SettingsLocal extends Settings { private text: string | undefined /** Serializes watcher-triggered reloads so reads never interleave. */ private refreshTask: Promise = Promise.resolve() + /** Serializes whole-document writes across namespace queues; settled tail. */ + private persistChain: Promise = Promise.resolve() /** Set at dispose: refuse new watcher events and let in-flight work no-op. */ private closed = false @@ -121,7 +123,17 @@ export class SettingsLocal extends Settings { return doc } - protected async persist(ns: SettingsNamespace, section: Record): Promise { + protected persist(ns: SettingsNamespace, section: Record): Promise { + // One document backs every namespace, so writes from different namespace + // queues must serialize here: each render must see the text the previous + // write committed, or the loser's section silently vanishes from disk. + // The stored tail is settled on both outcomes, so chaining needs no catch. + const task = this.persistChain.then(() => this.persistSection(ns, section)) + this.persistChain = task.then(() => undefined, () => undefined) + return task + } + + private async persistSection(ns: SettingsNamespace, section: Record): Promise { const output = this.spec.format === 'yaml' ? this.renderYaml(ns, section) : this.renderJson(ns, section) diff --git a/packages/settings/settings-local/tests/loader-composition.spec.ts b/packages/settings/settings-local/tests/loader-composition.spec.ts index d584c11899..c7cea89e41 100644 --- a/packages/settings/settings-local/tests/loader-composition.spec.ts +++ b/packages/settings/settings-local/tests/loader-composition.spec.ts @@ -1,7 +1,9 @@ /** * Real-composition guard: the provider and a consumer plugin boot from a - * test-only cordis.yml through the actual Loader + Include path, and an - * external edit of settings.yaml hot-publishes into the consumer's scope. + * test-only cordis.yml through the actual Loader + Include path, an external + * edit of settings.yaml hot-publishes into the consumer's scope, and the same + * consumer booted WITHOUT a settings entry keeps its entry-config resolution — + * the documented optional-inject fallback. */ import { mkdtemp, rm, writeFile } from 'node:fs/promises' @@ -39,33 +41,50 @@ afterEach(async () => { interface ConsumerState { scope: SettingsScope | undefined seen: ThemeConfig[] + /** What the consumer is actually running with, settings or not. */ + applied: ThemeConfig | undefined } -async function loadComposition(): Promise<{ ctx: Context; state: ConsumerState; settingsPath: string }> { +async function loadComposition( + options?: { withSettings?: boolean }, +): Promise<{ ctx: Context; state: ConsumerState; settingsPath: string }> { + const withSettings = options?.withSettings ?? true root = await mkdtemp(join(tmpdir(), 'dsh-settings-composition-')) const settingsPath = join(root, 'settings.yaml') await writeFile(settingsPath, 'ui-theme:\n theme: light\n') - const state: ConsumerState = { scope: undefined, seen: [] } + const state: ConsumerState = { scope: undefined, seen: [], applied: undefined } const consumer = { name: 'settings-consumer', - inject: ['settings'], apply: (ctx: Context) => { - const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema, { - base: { fontSize: 16 }, + // The documented consumer shape: no hard dependency — entry config alone + // is the running state, and the scoped inject overlays the user layer + // only while a settings service exists. + const base: Partial = { fontSize: 16 } + state.applied = ThemeSchema(base as ThemeConfig) + ctx.inject(['settings'], (child: Context) => { + const scope = child.settings.register(settingsNamespace('ui-theme'), ThemeSchema, { base }) + state.scope = scope + state.applied = scope.get() + scope.watch((next) => { + state.seen.push(next) + state.applied = next + }) }) - state.scope = scope - scope.watch((next) => { state.seen.push(next) }) }, } const configPath = join(root, 'cordis.yml') await writeFile(configPath, [ - '- id: settings', - " name: '@deepseek-ai/dsh-settings-local'", - ' config:', - ` path: ${JSON.stringify(settingsPath)}`, - ' debounceMs: 10', + ...withSettings + ? [ + '- id: settings', + " name: '@deepseek-ai/dsh-settings-local'", + ' config:', + ` path: ${JSON.stringify(settingsPath)}`, + ' debounceMs: 10', + ] + : [], '- id: consumer', ' name: test-settings-consumer', '', @@ -100,7 +119,9 @@ describe('settings-local real composition', () => { const { ctx, state, settingsPath } = await loadComposition() // Composition resolution: user layer over the consumer's composition base. - expect(state.scope!.get()).toEqual({ theme: 'light', fontSize: 16 }) + await vi.waitFor(() => { + expect(state.scope!.get()).toEqual({ theme: 'light', fontSize: 16 }) + }) expect(ctx.get('settings')!.describe().map(entry => entry.ns)).toEqual(['ui-theme']) await writeFile(settingsPath, 'ui-theme:\n theme: dark\n fontSize: 20\n') @@ -109,4 +130,16 @@ describe('settings-local real composition', () => { }, { timeout: 5000 }) expect(state.seen.at(-1)).toEqual({ theme: 'dark', fontSize: 20 }) }) + + it('boots the same consumer without a settings entry and keeps entry-config resolution', async () => { + const { ctx, state } = await loadComposition({ withSettings: false }) + + // No settings service anywhere in the composition… + expect(ctx.get('settings')).toBeUndefined() + // …so the consumer runs on schema defaults plus its composition base, and + // never receives a scope. + expect(state.applied).toEqual({ theme: 'dark', fontSize: 16 }) + expect(state.scope).toBeUndefined() + expect(state.seen).toEqual([]) + }) }) diff --git a/packages/settings/settings-local/tests/local.spec.ts b/packages/settings/settings-local/tests/local.spec.ts index 1c74429153..4c3c24ccd9 100644 --- a/packages/settings/settings-local/tests/local.spec.ts +++ b/packages/settings/settings-local/tests/local.spec.ts @@ -146,6 +146,23 @@ describe('persist', () => { expect((await readdir(dir)).sort()).toEqual(['settings.yaml']) }) + it('serializes cross-namespace writes into one on-disk document', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const alpha = ctx.settings.register(settingsNamespace('alpha'), ThemeSchema) + const beta = ctx.settings.register(settingsNamespace('beta'), ThemeSchema) + await Promise.all([ + alpha.update({ theme: 'light' }), + beta.update({ fontSize: 20 }), + ]) + const text = await readFile(path, 'utf8') + expect(text).toContain('alpha:') + expect(text).toContain('beta:') + expect(alpha.get().theme).toBe('light') + expect(beta.get().fontSize).toBe(20) + }) + it('never follows a planted symlink at a temp path and never leaves the document a symlink', async () => { const dir = await tempDir() const path = join(dir, 'settings.yaml') @@ -209,6 +226,9 @@ describe('persist', () => { await chmod(dir, 0o700) expect((await readdir(dir)).sort()).toEqual(['settings.yaml']) expect(scope.get().theme).toBe('light') + // The failed persist must not poison the document write chain. + await scope.update({ theme: 'dark' }) + expect(scope.get().theme).toBe('dark') }) it('round-trips a json document', async () => { diff --git a/packages/settings/settings/README.i18n.yaml b/packages/settings/settings/README.i18n.yaml index 6f5e5c6beb..63a274dd4d 100644 --- a/packages/settings/settings/README.i18n.yaml +++ b/packages/settings/settings/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/settings/settings/README.md -README.md: f7858a247f6011cd0654a73b5325d81c118441e5 -README.zh.md: 67ecba695066389bfe3a69f517d52f91b48b6c75 +README.md: ff6cdeb57a265dbaa9d5f50de1d558f1e3cb581f +README.zh.md: d820a5c1fa804455c439f1a155e7628f5118a49b diff --git a/packages/settings/settings/README.md b/packages/settings/settings/README.md index f7858a247f..ff6cdeb57a 100644 --- a/packages/settings/settings/README.md +++ b/packages/settings/settings/README.md @@ -11,7 +11,8 @@ Abstract user-settings seam (`ctx.settings`). One provider holds a raw document - `get(ns)` — resolved value, `undefined` while unregistered. - `update(ns, patch)` — deep-merges the plain-object patch into the user section only (never the `base`), validates the resolved candidate, persists through the provider, then commits. Validation failure rejects before anything is persisted; a read-only provider (`writable: false`) rejects every write. Writes to one namespace are serialized in call order. - `replace(ns, section)` — sets the user section wholesale: the removal/reset path a merge cannot express (`replace({})` re-inherits `base` and schema defaults). -- Resolved values are deep-frozen snapshots; watchers receive `(next, prev)` after each commit, and watcher failures — sync throws and async rejections alike — are contained. +- Resolved values are deep-frozen snapshots. Watchers receive `(next, prev)` after each commit: invocations of one callback run asynchronously, one at a time, in commit order (a slow stale invocation can never apply after a newer one), and failures — sync throws and async rejections alike — are contained. The `settings/updated` event fans out one listener at a time, so one throwing listener cannot starve the rest. +- Service teardown refuses new writes and drains every queued write before disposal completes; a write whose registrant fiber was disposed mid-flight still reaches storage but commits and notifies nobody. ## Provider contract diff --git a/packages/settings/settings/README.zh.md b/packages/settings/settings/README.zh.md index 67ecba6950..d820a5c1fa 100644 --- a/packages/settings/settings/README.zh.md +++ b/packages/settings/settings/README.zh.md @@ -11,7 +11,8 @@ - `get(ns)` — 解析值;未注册时为 `undefined`。 - `update(ns, patch)` — 把普通对象 patch 深合并进用户分节(绝不合并进 `base`),校验解析候选值,经 provider 持久化后提交。校验失败在持久化前拒绝;只读 provider(`writable: false`)拒绝一切写入。同一 namespace 的写入按调用顺序串行。 - `replace(ns, section)` — 整体替换用户分节:merge 表达不了的删除/重置路径(`replace({})` 重新继承 `base` 与 schema 默认值)。 -- 解析值是深冻结快照;每次提交后观察者收到 `(next, prev)`;观察者异常——同步抛出与异步拒绝——均被隔离。 +- 解析值是深冻结快照。每次提交后观察者收到 `(next, prev)`:同一回调的调用异步、逐次、按提交顺序执行(慢的旧调用绝不会覆盖更新的结果),异常——同步抛出与异步拒绝——均被隔离。`settings/updated` 事件逐 listener 扇出,一个抛错的 listener 不会饿死其余 listener。 +- 服务卸载先拒绝新写入并排干全部排队写入后才完成;registrant fiber 在写入途中被 dispose 时,该写入仍到达存储,但不向任何人提交或通知。 ## Provider 契约 diff --git a/packages/settings/settings/src/index.ts b/packages/settings/settings/src/index.ts index 10b21c2154..a7e9366048 100644 --- a/packages/settings/settings/src/index.ts +++ b/packages/settings/settings/src/index.ts @@ -58,8 +58,9 @@ export interface SettingsScope { /** Current resolved value: schema defaults, then `base`, then the user layer. */ get(): T /** - * Observe committed changes to this namespace's resolved value. A callback - * may be async; a rejection is contained and logged like a sync throw. + * Observe committed changes to this namespace's resolved value. Invocations + * of one callback run asynchronously, one at a time, in commit order; a + * rejection is contained and logged like a sync throw. * @param callback - invoked after each commit with the next and previous values. * @returns the disposer removing this observer. */ @@ -149,6 +150,13 @@ function deepFreeze(value: T): T { return Object.freeze(value) } +/** One registered watcher and its serialized invocation chain. */ +interface SettingsWatcher { + callback: (next: never, prev: never) => void | Promise + /** Settled tail: invocations of this callback run one at a time, in commit order. */ + tail: Promise +} + /** One live namespace registration owned by a registrant fiber. */ interface SettingsRegistration { ns: SettingsNamespace @@ -156,7 +164,7 @@ interface SettingsRegistration { base: unknown applies: SettingsApplies resolved: unknown - watchers: Set<(next: never, prev: never) => void | Promise> + watchers: Set } /** @@ -171,6 +179,13 @@ export abstract class Settings extends Service { private document: Record = {} /** Per-namespace write chains; settled tails, so a failure never poisons the queue. */ private readonly writeQueues = new Map>() + /** Set at service dispose: refuse new writes while queued ones drain. */ + private stopped = false + + /** Opaque read of {@link stopped}: control flow cannot narrow it across awaits. */ + private isStopped(): boolean { + return this.stopped + } constructor(ctx: Context) { super(ctx, 'settings') @@ -178,10 +193,17 @@ export abstract class Settings extends Service { /** * Load the provider's document once and publish it before the service - * becomes injectable. Providers with their own init (watchers, connections) - * delegate here first via `yield* super[Service.init]()`. + * becomes injectable, and register the write-drain teardown. Providers with + * their own init (watchers, connections) delegate here first via + * `yield* super[Service.init]()`; their disposers then run before the drain. */ - async* [Service.init](): AsyncGenerator<() => void, void, void> { + async* [Service.init](): AsyncGenerator<() => Promise | void, void, void> { + yield async () => { + // Teardown: refuse new writes, then wait until every queued write chain + // settles so disposal completes only once storage is quiescent. + this.stopped = true + await Promise.allSettled([...this.writeQueues.values()]) + } this.publish(await this.load()) } @@ -230,8 +252,9 @@ export abstract class Settings extends Service { return { get: () => registration.resolved as T, watch: (callback) => { - registration.watchers.add(callback) - return () => registration.watchers.delete(callback) + const watcher: SettingsWatcher = { callback: callback, tail: Promise.resolve() } + registration.watchers.add(watcher) + return () => registration.watchers.delete(watcher) }, update: patch => this.update(ns, patch), replace: section => this.replace(ns, section), @@ -287,27 +310,50 @@ export abstract class Settings extends Service { /** Validate a write, then queue it on the namespace's serialized write chain. */ private write(ns: SettingsNamespace, input: object, mode: 'merge' | 'replace'): Promise { + const verb = mode === 'merge' ? 'update' : 'replace' const registration = this.registrations.get(ns) if (registration === undefined) { throw new Error(`settings namespace "${ns}" is not registered`) } + if (this.isStopped()) { + throw new Error(`settings service is disposed: "${ns}" cannot be written`) + } if (!this.writable) { throw new Error(`settings provider is read-only: "${ns}" cannot be updated in-process`) } if (!isPlainObject(input)) { - throw new TypeError(`settings ${mode === 'merge' ? 'update' : 'replace'} for "${ns}" must be a plain object`) + throw new TypeError(`settings ${verb} for "${ns}" must be a plain object`) + } + // Snapshot at call time: the queue must never read a caller-owned object + // the caller may keep mutating while the write waits its turn. + let snapshot: Record + try { + snapshot = structuredClone(input) + } catch { + throw new TypeError(`settings ${verb} for "${ns}" must be JSON-shaped (structured-cloneable) data`) } const previous = this.writeQueues.get(ns) ?? Promise.resolve() // Chain past a failed predecessor: one rejected write must not poison the // namespace queue for every later caller. const run = previous.catch(() => undefined).then(async () => { + if (this.isStopped()) { + throw new Error(`settings service was disposed before the queued "${ns}" ${verb} ran`) + } + if (this.registrations.get(ns) !== registration) { + throw new Error(`settings namespace "${ns}" registration was disposed before the queued ${verb} ran`) + } const section = mode === 'merge' - ? mergeLayers(this.section(ns) ?? {}, input) as Record - : structuredClone(input) + ? mergeLayers(this.section(ns) ?? {}, snapshot) as Record + : snapshot const next = deepFreeze(this.resolve(registration.schema, registration.base, section)) await this.persist(ns, section) + // The write reached storage either way; the cache must say so. Commit + // only when this registration is still the namespace owner — a fiber + // disposed (or replaced) mid-persist must not receive the notification. this.document[ns] = section - this.commit(registration, next, 'update') + if (this.registrations.get(ns) === registration && !this.isStopped()) { + this.commit(registration, next, 'update') + } }) this.writeQueues.set(ns, run) return run @@ -358,29 +404,35 @@ export abstract class Settings extends Service { if (deepEqualJson(next, prev)) return registration.resolved = next for (const watcher of [...registration.watchers]) { + // Serialize per watcher: invocations of one callback run one at a time + // in commit order, so a slow stale invocation can never apply after a + // newer one. Sync throws and async rejections land in the same handler. + watcher.tail = watcher.tail + .then(() => watcher.callback(next as never, prev as never)) + .then(() => undefined, (error: unknown) => { + this.warnWatcherFailure(registration.ns, error) + }) + } + // Fan the event out one listener at a time (the plain emit stops at the + // first throwing listener, starving the rest). Invariant violations are + // harness-fatal by design and rethrow after every listener ran; any other + // failure is contained so one broken observer cannot wedge the commit + // path (and, through it, a provider's reload loop). + let invariantFailure: unknown + const args = ['settings/updated', registration.ns, next, prev, source] + for (const listener of this.ctx.events.dispatch('emit', args) as Array<(...listenerArgs: unknown[]) => unknown>) { try { - // A watcher may be async: adopt its promise so a rejection is contained - // here instead of surfacing as an unhandled rejection. - const outcome = watcher(next as never, prev as never) as unknown - if (outcome instanceof Promise) { - outcome.catch((error: unknown) => { - this.warnWatcherFailure(registration.ns, error) - }) - } + listener(registration.ns, next, prev, source) } catch (error) { - this.warnWatcherFailure(registration.ns, error) + if ((error as { code?: unknown } | null)?.code === 'INVARIANT') { + invariantFailure ??= error + continue + } + this.ctx.logger.warn('settings: a settings/updated listener for "%s" failed', registration.ns) + this.ctx.logger.warn(error) } } - try { - this.ctx.emit('settings/updated', registration.ns, next, prev, source) - } catch (error) { - // Invariant violations are harness-fatal by design; any other listener - // failure is contained so one broken observer cannot wedge the commit - // path (and, through it, a provider's reload loop). - if ((error as { code?: unknown } | null)?.code === 'INVARIANT') throw error - this.ctx.logger.warn('settings: a settings/updated listener for "%s" failed', registration.ns) - this.ctx.logger.warn(error) - } + if (invariantFailure !== undefined) throw invariantFailure as Error } /** Contained-watcher diagnostic shared by the sync and async failure paths. */ diff --git a/packages/settings/settings/tests/settings.spec.ts b/packages/settings/settings/tests/settings.spec.ts index c413e948ed..a989d9a5cc 100644 --- a/packages/settings/settings/tests/settings.spec.ts +++ b/packages/settings/settings/tests/settings.spec.ts @@ -52,9 +52,10 @@ const NestedSchema: z = z.object({ async function boot(options?: ConstructorParameters[1]) { const ctx = new Context() - await ctx.plugin(MemorySettings, options) + const fiber = ctx.plugin(MemorySettings, options) + await fiber const provider = ctx.get('settings') as MemorySettings - return { ctx, provider } + return { ctx, provider, fiber } } /** Record every settings/updated emission. */ @@ -341,6 +342,144 @@ describe('review regressions', () => { }) }) +describe('second review regressions', () => { + it('runs every settings/updated listener even when an earlier one throws', async () => { + const { ctx, provider } = await boot() + ctx.on('settings/updated', () => { + throw new Error('first listener boom') + }) + const second = vi.fn() + ctx.on('settings/updated', second) + ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + provider.pushExternal({ 'ui-theme': { theme: 'light' } }) + expect(second).toHaveBeenCalledTimes(1) + }) + + it('rejects an update queued after the registrant fiber disposed', async () => { + const { ctx } = await boot() + let scope: SettingsScope | undefined + const fiber = ctx.plugin({ + inject: ['settings'], + apply: (child: Context) => { + scope = child.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + }, + }) + await fiber + await fiber.dispose() + await expect(scope!.update({ theme: 'light' })).rejects.toThrow(/disposed|not registered/) + }) + + it('does not notify a registrant disposed while its update was in flight', async () => { + const { ctx, provider } = await boot({ persistDelayMs: 30 }) + const events = recordUpdates(ctx) + let scope: SettingsScope | undefined + const watcher = vi.fn() + const fiber = ctx.plugin({ + inject: ['settings'], + apply: (child: Context) => { + scope = child.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + scope.watch(watcher) + }, + }) + await fiber + const pending = scope!.update({ theme: 'light' }) + await new Promise(resolve => setTimeout(resolve, 5)) + await fiber.dispose() + await pending.catch(() => undefined) + await new Promise(resolve => setTimeout(resolve, 10)) + expect(watcher).not.toHaveBeenCalled() + expect(events).toEqual([]) + // The persist was already in flight, so storage keeps the write — but no + // commit reached the disposed registration. + expect(provider.doc['ui-theme']).toEqual({ theme: 'light' }) + }) + + it('drains in-flight writes at service dispose and rejects later ones', async () => { + const { ctx, provider, fiber } = await boot({ persistDelayMs: 20 }) + const service = ctx.settings + const scope = service.register(settingsNamespace('ui-theme'), ThemeSchema) + const pending = scope.update({ theme: 'light' }) + await new Promise(resolve => setTimeout(resolve, 5)) + await fiber.dispose() + // The teardown drained the in-flight write before completing… + await pending.catch(() => undefined) + const persistedAtDispose = provider.persisted.length + expect(persistedAtDispose).toBe(1) + // …and afterwards nothing writes and new writes reject. + await expect(service.update(settingsNamespace('ui-theme'), { theme: 'dark' })) + .rejects.toThrow(/disposed|not registered/) + await new Promise(resolve => setTimeout(resolve, 40)) + expect(provider.persisted.length).toBe(persistedAtDispose) + }) + + it('serializes invocations of one async watcher in commit order', async () => { + const { ctx, provider } = await boot() + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + const applied: number[] = [] + let firstCall = true + scope.watch(async (next) => { + // The first (stale) invocation is slow; unserialised it would finish + // last and clobber the newer applied state. + const delay = firstCall ? 30 : 0 + firstCall = false + await new Promise(resolve => setTimeout(resolve, delay)) + applied.push(next.fontSize) + }) + provider.pushExternal({ 'ui-theme': { fontSize: 1 } }) + provider.pushExternal({ 'ui-theme': { fontSize: 2 } }) + await vi.waitFor(() => { + expect(applied).toHaveLength(2) + }) + expect(applied).toEqual([1, 2]) + }) + + it('rejects a plain object that is not structured-cloneable', async () => { + const { ctx } = await boot() + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + await expect(scope.update({ theme: () => 'dark' })) + .rejects.toThrow(/JSON-shaped/) + }) + + it('rejects a write still queued when the service disposes', async () => { + const { ctx, fiber } = await boot({ persistDelayMs: 20 }) + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + const first = scope.update({ theme: 'light' }) + const second = scope.update({ fontSize: 20 }) + await new Promise(resolve => setTimeout(resolve, 5)) + await fiber.dispose() + await first + await expect(second).rejects.toThrow(/disposed before the queued/) + }) + + it('rejects a write still queued when the registrant disposes', async () => { + const { ctx } = await boot({ persistDelayMs: 20 }) + let scope: SettingsScope | undefined + const fiber = ctx.plugin({ + inject: ['settings'], + apply: (child: Context) => { + scope = child.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + }, + }) + await fiber + const first = scope!.update({ theme: 'light' }) + const second = scope!.update({ fontSize: 20 }) + await new Promise(resolve => setTimeout(resolve, 5)) + await fiber.dispose() + await first + await expect(second).rejects.toThrow(/registration was disposed before the queued/) + }) + + it('snapshots the patch at call time so caller mutation cannot leak in', async () => { + const { ctx } = await boot() + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + const patch = { fontSize: 18 } + const pending = scope.update(patch) + patch.fontSize = 99 + await pending + expect(scope.get().fontSize).toBe(18) + }) +}) + describe('publish', () => { it('notifies watchers of an external change with source provider', async () => { const { ctx, provider } = await boot() @@ -349,10 +488,12 @@ describe('publish', () => { const watcher = vi.fn() scope.watch(watcher) provider.pushExternal({ 'ui-theme': { theme: 'light' } }) - expect(watcher).toHaveBeenCalledWith( - { theme: 'light', fontSize: 14 }, - { theme: 'dark', fontSize: 14 }, - ) + await vi.waitFor(() => { + expect(watcher).toHaveBeenCalledWith( + { theme: 'light', fontSize: 14 }, + { theme: 'dark', fontSize: 14 }, + ) + }) expect(events[0]!.source).toBe('provider') }) @@ -410,7 +551,9 @@ describe('watch', () => { const second = vi.fn() scope.watch(second) provider.pushExternal({ 'ui-theme': { theme: 'light' } }) - expect(second).toHaveBeenCalledTimes(1) + await vi.waitFor(() => { + expect(second).toHaveBeenCalledTimes(1) + }) expect(events).toHaveLength(1) expect(scope.get()).toEqual({ theme: 'light', fontSize: 14 }) })