fix(settings): close cross-namespace, dispatch, and lifecycle races from second review

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.
2026-07-29 10:07:28 +08:00
parent f44b4db1f2
commit 1010291fe6
20 changed files with 339 additions and 71 deletions
--- 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
--- 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

--- 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 契约

--- a/packages/settings/settings/src/index.ts
+++ b/packages/settings/settings/src/index.ts
@@ -58,8 +58,9 @@ export interface SettingsScope<T> {
  /** 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<T>(value: T): T {
  return Object.freeze(value)
 }

+/** One registered watcher and its serialized invocation chain. */
+interface SettingsWatcher {
+  callback: (next: never, prev: never) => void | Promise<void>
+  /** Settled tail: invocations of this callback run one at a time, in commit order. */
+  tail: Promise<void>
+}
+
 /** 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<void>>
+  watchers: Set<SettingsWatcher>
 }

 /**
@@ -171,6 +179,13 @@ export abstract class Settings extends Service {
  private document: Record<string, unknown> = {}
  /** Per-namespace write chains; settled tails, so a failure never poisons the queue. */
  private readonly writeQueues = new Map<SettingsNamespace, Promise<unknown>>()
+  /** 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, 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<void> {
+    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<string, unknown>
+    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<string, unknown>
-        : structuredClone(input)
+        ? mergeLayers(this.section(ns) ?? {}, snapshot) as Record<string, unknown>
+        : 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. */
--- a/packages/settings/settings/tests/settings.spec.ts
+++ b/packages/settings/settings/tests/settings.spec.ts
@@ -52,9 +52,10 @@ const NestedSchema: z<NestedConfig> = z.object({

 async function boot(options?: ConstructorParameters<typeof MemorySettings>[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<ThemeConfig> | 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<ThemeConfig> | 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<ThemeConfig> | 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 })
  })