fix(settings): harden seam and provider per review findings
Confirmed and fixed, each with a regression test that failed first:
- Concurrent update() lost patches (merge over one stale snapshot):
per-namespace serialized write queues; a failed write cannot poison
the queue for later writers.
- Fixed-name .tmp write followed planted symlinks and kept stale modes:
random-suffix sibling, exclusive-create (wx), 0600, cleanup on
failure, then rename.
- A throwing settings/updated listener escaped commit and permanently
wedged the provider reload chain (rejected refreshTask): commit now
contains listener failures (INVARIANT-coded errors still propagate),
async watcher rejections are adopted and contained
(watch callbacks are officially void | Promise<void>), and the
provider chains refreshes on a settled tail with an error log.
- No way to remove a user override: scope/service replace(section)
sets the user section wholesale; replace({}) re-inherits base and
schema defaults.
- The three-primitive provider contract did not hold (base never
called load()): the base Service.init loads and publishes once;
settings-local delegates via yield* super[Service.init]().
- Dispose did not quiesce: teardown flags closed, closes the watcher,
then awaits queued/in-flight reloads; closed is re-checked across
await points.
- Invariant now checks the authoritative relation with the seam's own
deepEqualJson: emitted next must equal settings.get(ns), and
next/prev must differ structurally (cosmokit dependency dropped).
- New docs/core-data-structures/settings.{md,zh.md} with type-equiv
blocks + manifest entries; catalog types moved from exemptions to
LINK_MAP; website page registered.
Both packages stay at per-file 100% coverage.
This commit is contained in:
@@ -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: 90428f98055d8e49fa1ec54e571453db2e0a5054
|
||||
README.zh.md: 3532d6cee99cb46f54e23889ef1bb54b2548ecfa
|
||||
README.md: 9d0aa3982507ca16b382aaa918ca1536a98e29ea
|
||||
README.zh.md: 075de7ee5b3e0ef0a4ee27eeb8cb098ca0d73456
|
||||
|
||||
@@ -18,7 +18,8 @@ Defaulting is one explicit `resolveSpec(config)` step; an unsupported extension
|
||||
## Behavior
|
||||
|
||||
- **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 and owner-only.** `persist` writes `<path>.tmp` with mode `0600` and renames over the target. YAML writes patch one namespace in the comment-preserving document; JSON re-serializes.
|
||||
- **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.
|
||||
- **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.
|
||||
|
||||
## Model Experience
|
||||
|
||||
@@ -18,7 +18,8 @@
|
||||
## 行为
|
||||
|
||||
- **启动报错响亮,重载保留最后可用值。** 存在但非法的文档使插件加载失败;运行中不可读或不可解析的编辑只告警并保留最后可用分节。文档缺失时所有 namespace 按默认值与 `base` 解析;删除文档发布同样的空状态。
|
||||
- **写回原子且仅属主可读。** `persist` 以 `0600` 权限写 `<path>.tmp` 后 rename 覆盖目标。YAML 写回在保留注释的文档里只修补目标 namespace;JSON 重新序列化。
|
||||
- **写回原子、仅属主可读、抗符号链接。** `persist` 以 `0600` 权限独占创建随机后缀临时同级文件(`wx` 拒绝跟随预埋符号链接)后 rename 覆盖目标,失败时清理临时文件。YAML 写回在保留注释的文档里只修补目标 namespace;JSON 重新序列化。
|
||||
- **Dispose 保证静止。** 卸载先停止接收 watcher 事件、关闭 watcher,再等完排队与进行中的重载,之后不再有任何发布。
|
||||
- **按内容抑制自写。** provider 缓存最后可用文本;watcher 事件内容与缓存相同(含自己的写入)即为 no-op。
|
||||
|
||||
## Model Experience
|
||||
|
||||
@@ -8,7 +8,8 @@
|
||||
import { Context, Service } from 'cordis'
|
||||
import z from 'schemastery'
|
||||
import { watch as chokidarWatch } from 'chokidar'
|
||||
import { mkdir, readFile, rename, writeFile } from 'node:fs/promises'
|
||||
import { randomBytes } from 'node:crypto'
|
||||
import { mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises'
|
||||
import { dirname, extname, join, resolve } from 'node:path'
|
||||
import { Document, parseDocument } from 'yaml'
|
||||
import { resolveDshHome } from '@deepseek-ai/dsh-paths'
|
||||
@@ -86,6 +87,13 @@ export class SettingsLocal extends Settings {
|
||||
private text: string | undefined
|
||||
/** Serializes watcher-triggered reloads so reads never interleave. */
|
||||
private refreshTask: Promise<void> = Promise.resolve()
|
||||
/** Set at dispose: refuse new watcher events and let in-flight work no-op. */
|
||||
private closed = false
|
||||
|
||||
/** Opaque read of {@link closed}: control flow cannot narrow it across awaits. */
|
||||
private isClosed(): boolean {
|
||||
return this.closed
|
||||
}
|
||||
|
||||
constructor(ctx: Context, public config: Config) {
|
||||
super(ctx)
|
||||
@@ -118,18 +126,26 @@ export class SettingsLocal extends Settings {
|
||||
? this.renderYaml(ns, section)
|
||||
: this.renderJson(ns, section)
|
||||
await mkdir(dirname(this.spec.filename), { recursive: true })
|
||||
const temp = `${this.spec.filename}.tmp`
|
||||
// Owner-only permissions apply to the temp file and survive the rename, so
|
||||
// a document that may carry personal values is never world-readable.
|
||||
await writeFile(temp, output, { mode: 0o600 })
|
||||
await rename(temp, this.spec.filename)
|
||||
// Exclusive-create (`wx`) a random-suffix sibling: the open refuses to
|
||||
// follow any planted symlink at a guessable temp path, and the fresh inode
|
||||
// carries owner-only permissions that survive the rename — a document that
|
||||
// may hold personal values is never world-readable and never a symlink.
|
||||
const temp = `${this.spec.filename}.${randomBytes(6).toString('hex')}.tmp`
|
||||
try {
|
||||
await writeFile(temp, output, { mode: 0o600, flag: 'wx' })
|
||||
await rename(temp, this.spec.filename)
|
||||
} catch (error) {
|
||||
await rm(temp, { force: true })
|
||||
throw error
|
||||
}
|
||||
this.text = output
|
||||
}
|
||||
|
||||
async* [Service.init](): AsyncGenerator<() => Promise<void> | void, void, void> {
|
||||
// A parse failure here is a boot failure: an existing-but-invalid document
|
||||
// must fail loud, never be silently ignored or overwritten.
|
||||
this.publish(await this.load())
|
||||
override async* [Service.init](): AsyncGenerator<() => Promise<void> | void, void, void> {
|
||||
// The base init loads and publishes; a parse failure there is a boot
|
||||
// failure: an existing-but-invalid document must fail loud, never be
|
||||
// silently ignored or overwritten.
|
||||
yield* super[Service.init]()
|
||||
if (!this.spec.watch) return
|
||||
const watcher = chokidarWatch(this.spec.filename, {
|
||||
ignoreInitial: true,
|
||||
@@ -139,13 +155,26 @@ export class SettingsLocal extends Settings {
|
||||
},
|
||||
})
|
||||
watcher.on('all', () => {
|
||||
this.refreshTask = this.refreshTask.then(() => this.refresh())
|
||||
if (this.closed) return
|
||||
this.refreshTask = this.refreshTask.then(() => this.refresh()).catch((error: unknown) => {
|
||||
// Only an invariant violation escaping the commit path can reject a
|
||||
// refresh; keep the reload queue alive and surface it as an error so
|
||||
// one poisoned commit cannot silently end hot reloading forever.
|
||||
this.ctx.logger.error('settings-local: reload commit failed at %s', this.spec.filename)
|
||||
this.ctx.logger.error(error)
|
||||
})
|
||||
})
|
||||
watcher.on('error', (error) => {
|
||||
this.ctx.logger.warn('settings-local: watcher error on %s', this.spec.filename)
|
||||
this.ctx.logger.warn(error)
|
||||
})
|
||||
yield () => watcher.close()
|
||||
yield async () => {
|
||||
// Quiesce: stop accepting events, close the watcher, then wait out any
|
||||
// queued or in-flight refresh so nothing publishes after disposal.
|
||||
this.closed = true
|
||||
await watcher.close()
|
||||
await this.refreshTask
|
||||
}
|
||||
}
|
||||
|
||||
/** Parse one document text into raw sections, failing on a non-map root. */
|
||||
@@ -174,6 +203,7 @@ export class SettingsLocal extends Settings {
|
||||
* never take the process down.
|
||||
*/
|
||||
private async refresh(): Promise<void> {
|
||||
if (this.closed) return
|
||||
let text: string
|
||||
try {
|
||||
text = await readFile(this.spec.filename, 'utf8')
|
||||
@@ -183,12 +213,12 @@ export class SettingsLocal extends Settings {
|
||||
this.ctx.logger.warn(error)
|
||||
return
|
||||
}
|
||||
if (this.text === undefined) return
|
||||
if (this.text === undefined || this.isClosed()) return
|
||||
this.text = undefined
|
||||
this.publish({})
|
||||
return
|
||||
}
|
||||
if (text === this.text) return
|
||||
if (text === this.text || this.isClosed()) return
|
||||
let doc: Record<string, unknown>
|
||||
try {
|
||||
doc = this.parse(text)
|
||||
|
||||
@@ -55,7 +55,7 @@ async function loadComposition(): Promise<{ ctx: Context; state: ConsumerState;
|
||||
base: { fontSize: 16 },
|
||||
})
|
||||
state.scope = scope
|
||||
scope.watch(next => state.seen.push(next))
|
||||
scope.watch((next) => { state.seen.push(next) })
|
||||
},
|
||||
}
|
||||
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import { Context } from 'cordis'
|
||||
import z from 'schemastery'
|
||||
import { chmod, mkdtemp, readFile, readdir, rm, stat, writeFile } from 'node:fs/promises'
|
||||
import { chmod, lstat, mkdtemp, readFile, readdir, rm, stat, symlink, writeFile } from 'node:fs/promises'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { settingsNamespace } from '@deepseek-ai/dsh-settings'
|
||||
@@ -146,6 +146,23 @@ describe('persist', () => {
|
||||
expect((await readdir(dir)).sort()).toEqual(['settings.yaml'])
|
||||
})
|
||||
|
||||
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')
|
||||
const victim = join(dir, 'victim.txt')
|
||||
await writeFile(victim, 'precious')
|
||||
// A hostile sibling plants the historic fixed temp name as a symlink.
|
||||
await symlink(victim, `${path}.tmp`)
|
||||
const ctx = await boot({ path, watch: false })
|
||||
const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema)
|
||||
await scope.update({ theme: 'light' })
|
||||
|
||||
expect(await readFile(victim, 'utf8')).toBe('precious')
|
||||
expect((await lstat(path)).isSymbolicLink()).toBe(false)
|
||||
expect((await stat(path)).mode & 0o777).toBe(0o600)
|
||||
expect(await readFile(path, 'utf8')).toContain('theme: light')
|
||||
})
|
||||
|
||||
it('preserves comments and unregistered sections across updates', async () => {
|
||||
const dir = await tempDir()
|
||||
const path = join(dir, 'settings.yaml')
|
||||
@@ -180,6 +197,20 @@ describe('persist', () => {
|
||||
expect(written).toEqual({ 'ui-theme': { theme: 'light' } })
|
||||
})
|
||||
|
||||
it('rejects and leaves no temp residue when the directory turns unwritable', async () => {
|
||||
const dir = await tempDir()
|
||||
const path = join(dir, 'settings.yaml')
|
||||
await writeFile(path, 'ui-theme:\n theme: light\n')
|
||||
const ctx = await boot({ path, watch: false })
|
||||
const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema)
|
||||
await chmod(dir, 0o500)
|
||||
cleanups.push(() => chmod(dir, 0o700))
|
||||
await expect(scope.update({ theme: 'dark' })).rejects.toThrow()
|
||||
await chmod(dir, 0o700)
|
||||
expect((await readdir(dir)).sort()).toEqual(['settings.yaml'])
|
||||
expect(scope.get().theme).toBe('light')
|
||||
})
|
||||
|
||||
it('round-trips a json document', async () => {
|
||||
const dir = await tempDir()
|
||||
const path = join(dir, 'settings.json')
|
||||
|
||||
@@ -105,6 +105,60 @@ describe('watcher pipeline', () => {
|
||||
expect(scope.get()).toEqual({ theme: 'light' })
|
||||
})
|
||||
|
||||
it('keeps the reload queue alive after an invariant violation escapes a commit', async () => {
|
||||
const dir = await tempDir()
|
||||
const path = join(dir, 'settings.yaml')
|
||||
await writeFile(path, 'ui-theme:\n theme: light\n')
|
||||
const ctx = await boot({ path, debounceMs: 5 })
|
||||
const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema)
|
||||
let arm = true
|
||||
ctx.on('settings/updated', () => {
|
||||
if (!arm) return
|
||||
throw Object.assign(new Error('forged relation'), { code: 'INVARIANT' })
|
||||
})
|
||||
const [instance] = await fakeInstances()
|
||||
|
||||
await writeFile(path, 'ui-theme:\n theme: broken-commit\n')
|
||||
instance!.watcher.emit('all', 'change', path)
|
||||
await vi.waitFor(() => {
|
||||
expect(scope.get().theme).toBe('broken-commit')
|
||||
})
|
||||
|
||||
arm = false
|
||||
await writeFile(path, 'ui-theme:\n theme: recovered\n')
|
||||
instance!.watcher.emit('all', 'change', path)
|
||||
await vi.waitFor(() => {
|
||||
expect(scope.get().theme).toBe('recovered')
|
||||
})
|
||||
})
|
||||
|
||||
it('quiesces the refresh pipeline before dispose completes', async () => {
|
||||
const dir = await tempDir()
|
||||
const path = join(dir, 'settings.yaml')
|
||||
await writeFile(path, 'ui-theme:\n theme: light\n')
|
||||
const ctx = new Context()
|
||||
const fiber = ctx.plugin(SettingsLocal, { path, debounceMs: 5 })
|
||||
await fiber
|
||||
ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema)
|
||||
let disposed = false
|
||||
let postDisposeCommits = 0
|
||||
ctx.on('settings/updated', () => {
|
||||
if (disposed) postDisposeCommits += 1
|
||||
})
|
||||
|
||||
await writeFile(path, 'ui-theme:\n theme: darker\n')
|
||||
const [instance] = await fakeInstances()
|
||||
// Two queued refreshes: dispose interrupts one mid-flight and the other
|
||||
// before it starts, so both closed guards must hold.
|
||||
instance!.watcher.emit('all', 'change', path)
|
||||
instance!.watcher.emit('all', 'change', path)
|
||||
await fiber.dispose()
|
||||
disposed = true
|
||||
instance!.watcher.emit('all', 'change', path)
|
||||
await new Promise(resolve => setTimeout(resolve, 100))
|
||||
expect(postDisposeCommits).toBe(0)
|
||||
})
|
||||
|
||||
it('treats an event for a still-absent file as a no-op', async () => {
|
||||
const dir = await tempDir()
|
||||
const path = join(dir, 'settings.yaml')
|
||||
|
||||
Reference in New Issue
Block a user