diff --git a/packages/settings/settings-local/src/index.ts b/packages/settings/settings-local/src/index.ts index b305f61fe7..b9df71f9e1 100644 --- a/packages/settings/settings-local/src/index.ts +++ b/packages/settings/settings-local/src/index.ts @@ -9,11 +9,11 @@ import { Context, Service } from 'cordis' import z from 'schemastery' import { watch as chokidarWatch } from 'chokidar' import { randomBytes } from 'node:crypto' -import { mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises' +import { mkdir, readFile, rename, rm, stat, 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' -import { Settings, type SettingsNamespace } from '@deepseek-ai/dsh-settings' +import { Settings, deepEqualJson, type SettingsNamespace } from '@deepseek-ai/dsh-settings' /** Plugin config: file location and hot-reload behavior. */ export interface Config { @@ -64,11 +64,53 @@ export function resolveSpec(config: Config): ResolvedSpec { } } +/** Whether a parsed YAML value is a map for diffing purposes. */ +function isMapLike(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +/** + * Apply the difference between one node's stored and next value as minimal + * `setIn`/`deleteIn` edits, recursing through maps, so every untouched node — + * and the key node of every changed pair — keeps its comments, anchors, and + * formatting. Non-map values (arrays and scalars) replace wholesale when + * unequal, taking any comments inside them along. + */ +function patchNode(document: Document, path: readonly string[], current: unknown, next: unknown): void { + if (isMapLike(current) && isMapLike(next)) { + for (const key of Object.keys(current)) { + if (!(key in next)) document.deleteIn([...path, key]) + } + for (const [key, value] of Object.entries(next)) { + patchNode(document, [...path, key], current[key], value) + } + return + } + if (!deepEqualJson(current, next)) document.setIn([...path], next) +} + /** Whether a filesystem error means absence; every non-ENOENT failure must surface. */ function isENOENT(error: unknown): boolean { return (error as NodeJS.ErrnoException | null)?.code === 'ENOENT' } +/** Whether an exclusive create failed because the path already exists. */ +function isEEXIST(error: unknown): boolean { + return (error as NodeJS.ErrnoException | null)?.code === 'EEXIST' +} + +/** + * Writer-lock protocol constants. These are robustness invariants of the + * cross-process write protocol, not deployment tunables: a holder rewrites one + * small document in milliseconds, so contention resolves well inside the + * retry deadline, and a lock older than the stale age can only belong to a + * crashed holder. + */ +const LOCK_RETRY_INITIAL_MS = 20 +const LOCK_RETRY_MAX_MS = 200 +const LOCK_TIMEOUT_MS = 2_000 +const LOCK_STALE_MS = 5_000 + /** File-backed settings provider (`settings.yaml`/`.json`). */ export class SettingsLocal extends Settings { static Config: z = z.object({ @@ -85,10 +127,13 @@ export class SettingsLocal extends Settings { * this cache are no-ops, which is also the self-write suppression. */ 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() + /** + * Single exclusive operation chain: watcher reloads and document writes run + * one at a time in queue order (settled tail), so a write can never render + * from text a concurrent reload is busy replacing, and a reload can never + * read a half-committed write. + */ + private operations: Promise = Promise.resolve() /** Set at dispose: refuse new watcher events and let in-flight work no-op. */ private closed = false @@ -125,32 +170,107 @@ export class SettingsLocal extends Settings { 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) + // queues serialize with each other and with watcher reloads on the one + // operation chain: each render must see the text the previous operation + // committed, or a sibling section silently vanishes from disk. + return this.enqueue(() => this.persistSection(ns, section)) + } + + /** Queue one exclusive document operation behind every earlier one. */ + private enqueue(operation: () => Promise): Promise { + const task = this.operations.then(operation) + this.operations = task.then(() => undefined, () => undefined) return task } + /** Queue a reload; only an invariant violation escaping a commit can reject it. */ + private queueRefresh(): void { + void this.enqueue(() => this.refresh()).catch((error: unknown) => { + // Only an invariant violation escaping the commit path can reject a + // refresh; keep the operation 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) + }) + } + private async persistSection(ns: SettingsNamespace, section: Record): Promise { - const output = this.spec.format === 'yaml' - ? this.renderYaml(ns, section) - : this.renderJson(ns, section) await mkdir(dirname(this.spec.filename), { recursive: true }) - // 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 + await this.withWriterLock(async () => { + // Read-modify-write: fold in any on-disk state this process has not + // observed yet — an external edit still inside the watcher debounce + // window, a change the watcher missed, or another process's write — so + // the render below can never resurrect a stale document. An unparsable + // on-disk document fails the write loud instead of silently overwriting + // a user's manual edit. + await this.reconcileFromDisk() + const output = this.spec.format === 'yaml' + ? this.renderYaml(ns, section) + : this.renderJson(ns, section) + // 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 + }) + } + + /** + * Hold the cross-process writer lock around one read-render-rename cycle. + * The lock is a `wx`-created sibling (`.lock`); the rename-based + * commit keeps readers lock-free, so only writers contend. A lock older + * than {@link LOCK_STALE_MS} is a crashed holder and is broken with a + * warning; a live holder past {@link LOCK_TIMEOUT_MS} fails the write. + */ + private async withWriterLock(operation: () => Promise): Promise { + const lockPath = `${this.spec.filename}.lock` + const deadline = Date.now() + LOCK_TIMEOUT_MS + let delay = LOCK_RETRY_INITIAL_MS + for (;;) { + try { + await writeFile(lockPath, `${process.pid}\n`, { mode: 0o600, flag: 'wx' }) + break + } catch (error) { + if (!isEEXIST(error)) throw error + } + const ageMs = await this.lockAgeMs(lockPath) + // The holder released between the failed create and the stat: the lock + // is free right now, so retry without burning backoff or deadline. + if (ageMs === undefined) continue + if (ageMs > LOCK_STALE_MS) { + this.ctx.logger.warn('settings-local: breaking a stale writer lock at %s', lockPath) + await rm(lockPath, { force: true }) + continue + } + if (Date.now() >= deadline) { + throw new Error(`settings-local: timed out waiting for the writer lock at ${lockPath}`) + } + await new Promise(resolve => setTimeout(resolve, delay)) + delay = Math.min(delay * 2, LOCK_RETRY_MAX_MS) + } + try { + return await operation() + } finally { + await rm(lockPath, { force: true }) + } + } + + /** Age of the writer lock, or `undefined` when it vanished after a failed create. */ + private async lockAgeMs(lockPath: string): Promise { + try { + return Date.now() - (await stat(lockPath)).mtimeMs + } catch (error) { + if (!isENOENT(error)) throw error + return undefined } - this.text = output } override async* [Service.init](): AsyncGenerator<() => Promise | void, void, void> { @@ -168,13 +288,14 @@ export class SettingsLocal extends Settings { }) watcher.on('all', () => { 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) - }) + this.queueRefresh() + }) + watcher.on('ready', () => { + // The base init's load raced the watcher's own setup: a change written + // between that read and the watcher becoming active never fires an + // event. One reconcile at ready closes the gap. + if (this.closed) return + this.queueRefresh() }) watcher.on('error', (error) => { this.ctx.logger.warn('settings-local: watcher error on %s', this.spec.filename) @@ -182,10 +303,10 @@ export class SettingsLocal extends Settings { }) yield async () => { // Quiesce: stop accepting events, close the watcher, then wait out any - // queued or in-flight refresh so nothing publishes after disposal. + // queued or in-flight operation so nothing publishes after disposal. this.closed = true await watcher.close() - await this.refreshTask + await this.operations } } @@ -212,46 +333,62 @@ export class SettingsLocal extends Settings { * Re-read the document after a watcher event. Unchanged content (including * this provider's own writes) is a no-op; an unreadable or unparsable * document keeps the last good sections and warns — a live hot-reload must - * never take the process down. + * never take the process down. An invariant violation escaping a commit is + * not a reload failure and propagates to the queue's error surface. */ private async refresh(): Promise { if (this.closed) return - let text: string + try { + await this.reconcileFromDisk() + } catch (error) { + if ((error as { code?: unknown } | null)?.code === 'INVARIANT') throw error + this.ctx.logger.warn('settings-local: reload failed at %s; keeping the last good document', this.spec.filename) + this.ctx.logger.warn(error) + } + } + + /** + * Compare the on-disk text against the cache and publish any difference + * into the seam. Absence publishes the empty document; an unreadable or + * unparsable file throws, so each caller picks its policy — a reload warns + * and keeps the last good document, a write fails loud. + */ + private async reconcileFromDisk(): Promise { + let text: string | undefined try { text = await readFile(this.spec.filename, 'utf8') } catch (error) { - if (!isENOENT(error)) { - this.ctx.logger.warn('settings-local: reload failed at %s; keeping the last good document', this.spec.filename) - this.ctx.logger.warn(error) - return - } - if (this.text === undefined || this.isClosed()) return + if (!isENOENT(error)) throw error + text = undefined + } + if (text === this.text || this.isClosed()) return + if (text === undefined) { this.text = undefined this.publish({}) return } - if (text === this.text || this.isClosed()) return - let doc: Record - try { - doc = this.parse(text) - } catch (error) { - this.ctx.logger.warn('settings-local: reload failed at %s; keeping the last good document', this.spec.filename) - this.ctx.logger.warn(error) - return - } + const doc = this.parse(text) this.text = text this.publish(doc) } - /** Render the next YAML text by patching one namespace in the comment-preserving document. */ + /** + * Render the next YAML text by patching one namespace in the + * comment-preserving document. The next section lands as a leaf-level diff + * against the stored one — only changed values set, only removed keys + * delete — so comments inside the section survive edits to their siblings, + * not just comments outside it. + */ private renderYaml(ns: SettingsNamespace, section: Record): string { if (this.text === undefined) { return new Document({ [ns]: section }).toString() } // this.text only ever caches content that parsed successfully, so this - // re-parse (for the mutable comment-preserving tree) cannot fail. + // re-parse (for the mutable comment-preserving tree) cannot fail, and + // parse() already rejected any non-map root. const document = parseDocument(this.text) - document.set(ns, section) + const root: unknown = document.toJS() + patchNode(document, [ns], isMapLike(root) ? root[ns] : undefined, section) return document.toString() } diff --git a/packages/settings/settings-local/tests/concurrency.spec.ts b/packages/settings/settings-local/tests/concurrency.spec.ts new file mode 100644 index 0000000000..ab09866819 --- /dev/null +++ b/packages/settings/settings-local/tests/concurrency.spec.ts @@ -0,0 +1,103 @@ +// Cross-instance and writer-lock behavior: two providers on one document are +// the in-process equivalent of two dsh processes sharing a harness home — +// neither knows the other's cache, so only the read-modify-write cycle under +// the `.lock` sibling keeps both namespaces alive on disk. +import { afterEach, describe, expect, it } from 'vitest' +import { Context } from 'cordis' +import z from 'schemastery' +import { chmod, mkdtemp, readFile, rm, utimes, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { settingsNamespace } from '@deepseek-ai/dsh-settings' +import { SettingsLocal } from '../src/index.ts' + +const AlphaSchema: z<{ value: number }> = z.object({ value: z.number().default(0) }) +const BetaSchema: z<{ value: number }> = z.object({ value: z.number().default(0) }) + +const cleanups: Array<() => Promise> = [] + +afterEach(async () => { + while (cleanups.length > 0) await cleanups.pop()!() +}) + +async function tempDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), 'dsh-settings-lock-')) + cleanups.push(() => rm(dir, { recursive: true, force: true })) + return dir +} + +async function boot(config: ConstructorParameters[1]): Promise { + const ctx = new Context() + const fiber = ctx.plugin(SettingsLocal, config) + cleanups.push(async () => { await fiber.dispose() }) + await fiber + return ctx +} + +describe('cross-instance writes', () => { + it('keeps both namespaces when two providers write the same document concurrently', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const first = await boot({ path, watch: false }) + const second = await boot({ path, watch: false }) + const alpha = first.settings.register(settingsNamespace('alpha'), AlphaSchema) + const beta = second.settings.register(settingsNamespace('beta'), BetaSchema) + const rounds = [1, 2, 3, 4, 5] + await Promise.all([ + (async () => { for (const value of rounds) await alpha.update({ value }) })(), + (async () => { for (const value of rounds) await beta.update({ value }) })(), + ]) + const text = await readFile(path, 'utf8') + expect(text).toContain('alpha:') + expect(text).toContain('beta:') + // A third instance resolves both final values from the shared document. + const third = await boot({ path, watch: false }) + expect(third.settings.register(settingsNamespace('alpha'), AlphaSchema).get()).toEqual({ value: 5 }) + expect(third.settings.register(settingsNamespace('beta'), BetaSchema).get()).toEqual({ value: 5 }) + }) +}) + +describe('writer lock', () => { + it('waits for a busy writer lock instead of failing', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + await writeFile(`${path}.lock`, 'holder\n') + const release = setTimeout(() => { void rm(`${path}.lock`, { force: true }) }, 120) + cleanups.push(async () => { clearTimeout(release) }) + await scope.update({ value: 7 }) + expect(await readFile(path, 'utf8')).toContain('value: 7') + }) + + it('breaks a stale writer lock with a warning and writes through', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + await writeFile(`${path}.lock`, 'crashed-holder\n') + const past = (Date.now() - 60_000) / 1000 + await utimes(`${path}.lock`, past, past) + await scope.update({ value: 9 }) + expect(await readFile(path, 'utf8')).toContain('value: 9') + }) + + it('times out on a lock a live holder never releases', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + await writeFile(`${path}.lock`, 'busy-holder\n') + await expect(scope.update({ value: 1 })).rejects.toThrow(/timed out waiting for the writer lock/) + }, 10_000) + + it('surfaces a non-contention lock failure as the write error', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + await chmod(dir, 0o500) + cleanups.push(() => chmod(dir, 0o700)) + await expect(scope.update({ value: 1 })).rejects.toThrow(/EACCES|permission/) + }) +}) diff --git a/packages/settings/settings-local/tests/local.spec.ts b/packages/settings/settings-local/tests/local.spec.ts index 4c3c24ccd9..0b753675f5 100644 --- a/packages/settings/settings-local/tests/local.spec.ts +++ b/packages/settings/settings-local/tests/local.spec.ts @@ -204,6 +204,102 @@ describe('persist', () => { expect(written).toContain('theme: light') }) + it('keeps comments inside the section when a sibling key changes', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + await writeFile(path, [ + 'ui-theme:', + ' # chosen during onboarding', + ' theme: light', + ' fontSize: 12', + '', + ].join('\n')) + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + await scope.update({ fontSize: 18 }) + const written = await readFile(path, 'utf8') + expect(written).toContain('# chosen during onboarding') + expect(written).toContain('theme: light') + expect(written).toContain('fontSize: 18') + }) + + it('keeps a changed key\'s own-line comment while replacing its value', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + await writeFile(path, [ + 'ui-theme:', + ' # chosen during onboarding', + ' theme: light', + '', + ].join('\n')) + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + await scope.update({ theme: 'dark' }) + const written = await readFile(path, 'utf8') + expect(written).toContain('# chosen during onboarding') + expect(written).toContain('theme: dark') + }) + + it('deletes only the removed key on replace, keeping sibling comments', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + await writeFile(path, [ + 'ui-theme:', + ' # chosen during onboarding', + ' theme: light', + ' fontSize: 12', + '', + ].join('\n')) + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + await scope.replace({ theme: 'light' }) + const written = await readFile(path, 'utf8') + expect(written).toContain('# chosen during onboarding') + expect(written).toContain('theme: light') + expect(written).not.toContain('fontSize') + }) + + it('keeps an unchanged array\'s comments and replaces a changed array wholesale', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const TagsSchema: z<{ tags: string[]; label: string }> = z.object({ + tags: z.array(z.string()).default([]), + label: z.string().default(''), + }) + await writeFile(path, [ + 'workspace:', + ' tags:', + ' # pinned by hand', + ' - alpha', + ' label: draft', + '', + ].join('\n')) + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('workspace'), TagsSchema) + await scope.update({ label: 'final' }) + const untouched = await readFile(path, 'utf8') + expect(untouched).toContain('# pinned by hand') + expect(untouched).toContain('label: final') + // A changed array replaces wholesale; comments inside it go with it. + await scope.update({ tags: ['beta'] }) + const replaced = await readFile(path, 'utf8') + expect(replaced).not.toContain('# pinned by hand') + expect(replaced).toContain('- beta') + }) + + it('keeps a comment-only document\'s comment when the first section lands', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + // Parses to a null root: the document exists but holds no sections yet. + await writeFile(path, '# reserved for future settings\n') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + await scope.update({ theme: 'light' }) + const written = await readFile(path, 'utf8') + expect(written).toContain('# reserved for future settings') + expect(written).toContain('theme: light') + }) + it('creates a json document from scratch', async () => { const dir = await tempDir() const path = join(dir, 'settings.json') diff --git a/packages/settings/settings-local/tests/lock-race.spec.ts b/packages/settings/settings-local/tests/lock-race.spec.ts new file mode 100644 index 0000000000..09eb025654 --- /dev/null +++ b/packages/settings/settings-local/tests/lock-race.spec.ts @@ -0,0 +1,100 @@ +// Writer-lock races that cannot be timed from outside: a contender whose lock +// vanishes between the failed exclusive create and the stat, a stat failing +// for a reason other than absence, and a temp-file write failing mid-cycle. +// The fs/promises seam is partially mocked to inject exactly one failure at a +// chosen path suffix; everything else passes through to the real filesystem. +import { afterEach, describe, expect, it, vi } from 'vitest' +import { Context } from 'cordis' +import z from 'schemastery' +import { access, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { settingsNamespace } from '@deepseek-ai/dsh-settings' +import { SettingsLocal } from '../src/index.ts' + +const state = vi.hoisted(() => ({ + /** One-shot failure injections keyed by operation, matched on a path suffix. */ + failures: [] as Array<{ op: 'writeFile' | 'stat'; suffix: string; code: string }>, +})) + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + const inject = (op: 'writeFile' | 'stat', path: unknown): void => { + const index = state.failures.findIndex(f => f.op === op && String(path).endsWith(f.suffix)) + if (index === -1) return + const [failure] = state.failures.splice(index, 1) + throw Object.assign(new Error(`${failure!.code}: injected ${op} failure`), { code: failure!.code }) + } + return { + ...actual, + writeFile: (async (path: unknown, ...rest: never[]) => { + inject('writeFile', path) + return (actual.writeFile as (path: unknown, ...args: never[]) => Promise)(path, ...rest) + }) as typeof actual.writeFile, + stat: (async (path: unknown, ...rest: never[]) => { + inject('stat', path) + return (actual.stat as (path: unknown, ...args: never[]) => Promise)(path, ...rest) + }) as typeof actual.stat, + } +}) + +const AlphaSchema: z<{ value: number }> = z.object({ value: z.number().default(0) }) + +const cleanups: Array<() => Promise> = [] + +afterEach(async () => { + state.failures.length = 0 + while (cleanups.length > 0) await cleanups.pop()!() +}) + +async function tempDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), 'dsh-settings-lockrace-')) + cleanups.push(() => rm(dir, { recursive: true, force: true })) + return dir +} + +async function boot(config: ConstructorParameters[1]): Promise { + const ctx = new Context() + const fiber = ctx.plugin(SettingsLocal, config) + cleanups.push(async () => { await fiber.dispose() }) + await fiber + return ctx +} + +describe('writer-lock races', () => { + it('retries immediately when the contending lock vanished before the stat', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + // The exclusive create loses to a holder that releases before the stat: + // no lock file actually exists, so the stat sees honest absence and the + // very next attempt takes the lock. + state.failures.push({ op: 'writeFile', suffix: '.lock', code: 'EEXIST' }) + await scope.update({ value: 3 }) + expect(await readFile(path, 'utf8')).toContain('value: 3') + }) + + it('propagates a stat failure that does not mean absence', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + state.failures.push({ op: 'writeFile', suffix: '.lock', code: 'EEXIST' }) + state.failures.push({ op: 'stat', suffix: '.lock', code: 'EACCES' }) + await expect(scope.update({ value: 3 })).rejects.toThrow(/EACCES/) + }) + + it('cleans up the temp file and releases the lock when the write fails mid-cycle', async () => { + const dir = await tempDir() + const path = join(dir, 'settings.yaml') + await writeFile(path, 'alpha:\n value: 1\n') + const ctx = await boot({ path, watch: false }) + const scope = ctx.settings.register(settingsNamespace('alpha'), AlphaSchema) + state.failures.push({ op: 'writeFile', suffix: '.tmp', code: 'ENOSPC' }) + await expect(scope.update({ value: 9 })).rejects.toThrow(/ENOSPC/) + // The document is untouched and the writer lock was released on the way out. + expect(await readFile(path, 'utf8')).toContain('value: 1') + await expect(access(`${path}.lock`)).rejects.toThrow() + }) +}) diff --git a/packages/settings/settings-local/tests/watcher.spec.ts b/packages/settings/settings-local/tests/watcher.spec.ts index 439f213473..7b289b22a3 100644 --- a/packages/settings/settings-local/tests/watcher.spec.ts +++ b/packages/settings/settings-local/tests/watcher.spec.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { Context } from 'cordis' import z from 'schemastery' -import { chmod, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { chmod, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { settingsNamespace } from '@deepseek-ai/dsh-settings' @@ -155,6 +155,7 @@ describe('watcher pipeline', () => { await fiber.dispose() disposed = true instance!.watcher.emit('all', 'change', path) + instance!.watcher.emit('ready') await new Promise(resolve => setTimeout(resolve, 100)) expect(postDisposeCommits).toBe(0) }) @@ -169,4 +170,56 @@ describe('watcher pipeline', () => { await new Promise(resolve => setTimeout(resolve, 50)) expect(scope.get()).toEqual({ theme: 'dark' }) }) + + it('folds an unobserved external edit into a write instead of overwriting it', 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 theme = ctx.settings.register(settingsNamespace('ui-theme'), ThemeSchema) + const editor = ctx.settings.register(settingsNamespace('editor'), z.object({ + tabWidth: z.number().default(2), + })) + // The external edit has landed on disk but its watcher event has not + // fired yet (a debounce window, or a missed event): the write must fold + // it in, not resurrect the stale document. + await writeFile(path, 'ui-theme:\n theme: light\neditor:\n tabWidth: 8\n') + await theme.update({ theme: 'darker' }) + const text = await readFile(path, 'utf8') + expect(text).toContain('tabWidth: 8') + expect(text).toContain('theme: darker') + // The fold published the unobserved section before the write committed. + expect(editor.get()).toEqual({ tabWidth: 8 }) + }) + + it('reconciles at watcher ready so a change during setup is not missed', 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) + // Written after the initial load but before the watcher became active: + // no 'all' event will ever fire for it. + await writeFile(path, 'ui-theme:\n theme: written-before-ready\n') + const [instance] = await fakeInstances() + instance!.watcher.emit('ready') + await vi.waitFor(() => { + expect(scope.get().theme).toBe('written-before-ready') + }) + }) + + it('fails a write loud when the on-disk document turned invalid unobserved', 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) + const broken = 'ui-theme: [unclosed\n flow: {\n' + await writeFile(path, broken) + await expect(scope.update({ theme: 'darker' })).rejects.toThrow(/invalid document/) + // The user's manual edit stays on disk untouched and the cache keeps the + // last good value. + expect(await readFile(path, 'utf8')).toBe(broken) + expect(scope.get()).toEqual({ theme: 'light' }) + }) })