From 90c3118302fdf717a237e9f6de3b1443325ecaf7 Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Thu, 30 Jul 2026 15:40:09 +0800 Subject: [PATCH] fix(credentials-local): one operation chain, read-modify-write under the shared writer lock, and a quote-aware line editor Review round three, credentials half. dsh-atomic-write grows the cross-process writer-lock primitive (withFileLock: wx sentinel, bounded backoff, stale takeover via onStaleBreak, deadline failure) plus a dirMode option, and settings-local migrates its private copy to it; both providers now create harness-home directories 0700. credentials-local reuses the reviewed settings-local shape: watcher reloads and line edits share one settled operation chain; every write re-reads the document under the lock and publishes unobserved external entries before editing, so an edit inside the debounce window (or another process's write) can never be overwritten; the watcher's ready signal queues one reconcile closing the startup gap. The line editor is now physical-line aware: continuation lines of a quoted multi-line value are never mistaken for assignments, untouched lines keep their exact bytes (CRLF included), an edited line keeps its own terminator, and appends use the document's dominant ending. A multi-line entry reports writable: false, matching what set() would do. The Credentials base class owns a contained notifyUpdated fan-out: providers publish only after the commit, every listener runs, sync throws and async rejections are logged without failing the committed write, and INVARIANT-coded failures rethrow after the fan-out. --- .../credentials-local/src/index.ts | 257 +++++++++++++----- .../credentials-local/tests/drain.spec.ts | 10 +- .../tests/review-fixes.spec.ts | 202 ++++++++++++++ .../credentials-local/tests/watcher.spec.ts | 16 ++ packages/credentials/credentials/src/index.ts | 50 +++- packages/settings/settings-local/src/index.ts | 82 +----- packages/util/atomic-write/src/index.ts | 117 +++++++- 7 files changed, 579 insertions(+), 155 deletions(-) create mode 100644 packages/credentials/credentials-local/tests/review-fixes.spec.ts diff --git a/packages/credentials/credentials-local/src/index.ts b/packages/credentials/credentials-local/src/index.ts index c1fcd37f16..576b8241f0 100644 --- a/packages/credentials/credentials-local/src/index.ts +++ b/packages/credentials/credentials-local/src/index.ts @@ -3,19 +3,21 @@ * a `$DSH_HOME/.env` document. The environment is authoritative and read-only * (a launch-time override must win, and must be visibly read-only rather than * silently shadow writes); the file is the provider-managed writable source: - * `set`/`unset` rewrite only their own line and preserve every other byte, - * external edits hot-publish through the seam, and each reload replaces the - * snapshot wholesale so a deleted entry never lingers in memory. + * every write re-reads the document under a cross-process writer lock before + * rewriting only its own line — preserving every other byte, physical line + * endings and quoted multi-line values included — external edits hot-publish + * through the seam, and each reload replaces the snapshot wholesale so a + * deleted entry never lingers in memory. * @module @deepseek-ai/dsh-credentials-local */ import { Context, Service } from 'cordis' import z from 'schemastery' import { watch as chokidarWatch } from 'chokidar' -import { readFile } from 'node:fs/promises' -import { join, resolve } from 'node:path' +import { mkdir, readFile } from 'node:fs/promises' +import { dirname, join, resolve } from 'node:path' import { parse } from 'dotenv' -import { writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' +import { withFileLock, writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' import { resolveDshHome } from '@deepseek-ai/dsh-paths' import { Credentials, credentialRef } from '@deepseek-ai/dsh-credentials' import type { CredentialInfo, CredentialRef, ResolvedCredential } from '@deepseek-ai/dsh-credentials' @@ -58,11 +60,6 @@ function isENOENT(error: unknown): boolean { return (error as NodeJS.ErrnoException | null)?.code === 'ENOENT' } -/** Match the physical line(s) assigning one reference (ref chars need no escaping). */ -function refLinePattern(ref: CredentialRef): RegExp { - return new RegExp(`^\\s*(?:export\\s+)?${ref}\\s*=`) -} - /** Values that survive a dotenv round-trip without quoting. */ const BARE_VALUE = /^[A-Za-z0-9_@%+:,./-]+$/ @@ -90,30 +87,98 @@ function renderLine(ref: CredentialRef, value: string): string { throw new Error(`credentials-local: the value for "${ref}" mixes quoting no .env style can represent; edit the file directly`) } +/** Split text into physical lines with their terminators attached. */ +function physicalLines(text: string): string[] { + return text.length === 0 ? [] : text.split(/(?<=\n)/) +} + +/** One physical line's content without its terminator. */ +function lineContent(line: string): string { + if (line.endsWith('\r\n')) return line.slice(0, -2) + if (line.endsWith('\n')) return line.slice(0, -1) + return line +} + +/** One physical line's terminator (empty on a final unterminated line). */ +function lineTerminator(line: string): string { + return line.slice(lineContent(line).length) +} + +/** An assignment line: optional export, a POSIX identifier, `=`, the value part. */ +const ASSIGNMENT = /^\s*(?:export\s+)?([A-Za-z_][A-Za-z0-9_]*)\s*=(.*)$/ + +/** Quote characters dotenv reads across physical lines. */ +const MULTILINE_QUOTES = ['\'', '"', '`'] + /** - * Replace, insert, or delete one reference's assignment while preserving every - * other byte. The first matching line is rewritten in place; further matches - * are dropped (dotenv reads the last one, so duplicates are dead weight that - * would otherwise override the edit). + * The quote character an assignment's value part opens without closing on its + * own line — the following physical lines are that value's continuation, not + * assignments — or `undefined` for a single-line value. */ -function upsertLine(text: string | undefined, ref: CredentialRef, line: string | undefined): string { - const lines = text === undefined || text.length === 0 ? [] : text.split('\n') - if (lines.length > 0 && lines[lines.length - 1] === '') lines.pop() - const matcher = refLinePattern(ref) +function opensMultiline(valuePart: string): string | undefined { + const trimmed = valuePart.trimStart() + const quote = trimmed[0] + if (quote === undefined || !MULTILINE_QUOTES.includes(quote)) return undefined + const rest = trimmed.slice(1) + const body = quote === '"' ? rest.replaceAll('\\"', '') : rest + return body.includes(quote) ? undefined : quote +} + +/** Whether a continuation line closes the given quote. */ +function closesQuote(content: string, quote: string): boolean { + const body = quote === '"' ? content.replaceAll('\\"', '') : content + return body.includes(quote) +} + +/** + * Replace, insert, or delete one reference's assignment while preserving + * every other byte: untouched lines keep their exact content and terminators + * (CRLF included), and the physical lines inside another key's quoted + * multi-line value are never mistaken for assignments. The first matching + * assignment is rewritten in place with its own line ending; later duplicates + * drop (dotenv reads the last one, so a surviving duplicate would override + * the edit); an insert appends in the document's dominant ending style. + */ +function upsertLine(text: string | undefined, ref: CredentialRef, rendered: string | undefined): string { + const lines = physicalLines(text ?? '') + const dominant = lines.some(line => line.endsWith('\r\n')) ? '\r\n' : '\n' const out: string[] = [] let placed = false - for (const current of lines) { - if (matcher.test(current)) { - if (line !== undefined && !placed) { - out.push(line) - placed = true - } + let pendingQuote: string | undefined + for (const line of lines) { + const content = lineContent(line) + if (pendingQuote !== undefined) { + // Inside a quoted multi-line value: never an assignment, always kept. + if (closesQuote(content, pendingQuote)) pendingQuote = undefined + out.push(line) continue } - out.push(current) + const match = ASSIGNMENT.exec(content) + if (match === null) { + out.push(line) + continue + } + const [, key, valuePart] = match + if (key !== ref) { + pendingQuote = opensMultiline(valuePart ?? '') + out.push(line) + continue + } + // The write path refuses multi-line targets before rendering, so the + // matched assignment is single-line and drops or rewrites wholesale. + if (rendered !== undefined && !placed) { + out.push(`${rendered}${lineTerminator(line) === '' ? dominant : lineTerminator(line)}`) + placed = true + } } - if (line !== undefined && !placed) out.push(line) - return out.length === 0 ? '' : `${out.join('\n')}\n` + if (rendered !== undefined && !placed) { + const last = out[out.length - 1] + if (last !== undefined && lineTerminator(last) === '') { + out[out.length - 1] = `${last}${dominant}` + } + out.push(`${rendered}${dominant}`) + } + return out.join('') } /** File-backed credentials provider (`$DSH_HOME/.env`). */ @@ -137,10 +202,12 @@ export class CredentialsLocal extends Credentials { private text: string | undefined /** Parsed document snapshot; replaced wholesale on every reload. */ private values = new Map() - /** Serializes watcher-triggered reloads so reads never interleave. */ - private refreshTask: Promise = Promise.resolve() - /** Serializes writes to the one document; settled tail. */ - private writeChain: Promise = Promise.resolve() + /** + * Single exclusive operation chain: watcher reloads and line edits run one + * at a time in queue order (settled tail), so an edit can never render from + * text a concurrent reload is busy replacing. + */ + private operations: Promise = Promise.resolve() /** Set at dispose: refuse new writes and let in-flight work no-op. */ private closed = false @@ -159,10 +226,10 @@ export class CredentialsLocal extends Credentials { async* [Service.init](): AsyncGenerator<() => Promise | void, void, void> { yield async () => { - // Drain: refuse new writes, then settle the queued ones so disposal + // Drain: refuse new operations, then settle the queued ones so disposal // completes only once storage is quiescent. this.closed = true - await this.writeChain + await this.operations } await this.loadInitial() if (!this.spec.watch) return @@ -178,26 +245,27 @@ export class CredentialsLocal extends Credentials { }) watcher.on('all', () => { if (this.closed) return - this.refreshTask = this.refreshTask.then(() => this.refresh()).catch((error: unknown) => { - // Only an invariant violation escaping the update fan-out 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('credentials-local: reload commit failed at %s', this.spec.filename) - this.ctx.logger.error(error) - }) + this.queueRefresh() + }) + watcher.on('ready', () => { + // The initial 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('credentials-local: watcher error on %s', this.spec.filename) this.ctx.logger.warn(error) }) - /* jscpd:ignore-end */ 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 } + /* jscpd:ignore-end */ } override resolve(ref: CredentialRef): Promise { @@ -215,7 +283,9 @@ export class CredentialsLocal extends Credentials { } const stored = this.values.get(ref) if (stored !== undefined && stored.length > 0) { - return Promise.resolve({ configured: true, source: 'file', writable: true }) + // A quoted multi-line value resolves fine but the line editor refuses to + // rewrite it, so writability must say what set() would actually do. + return Promise.resolve({ configured: true, source: 'file', writable: !stored.includes('\n') }) } return Promise.resolve({ configured: false, writable: true }) } @@ -231,6 +301,24 @@ export class CredentialsLocal extends Credentials { await this.write(ref, undefined) } + /** 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 the fan-out can reject it. */ + private queueRefresh(): void { + void this.enqueue(() => this.refresh()).catch((error: unknown) => { + // Only an invariant violation escaping the update fan-out 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('credentials-local: reload commit failed at %s', this.spec.filename) + this.ctx.logger.error(error) + }) + } + /** Queue one line edit; entry checks reject early, the queue re-judges them at run time. */ private async write(ref: CredentialRef, value: string | undefined): Promise { const verb = value === undefined ? 'unset' : 'set' @@ -238,32 +326,43 @@ export class CredentialsLocal extends Credentials { throw new Error(`credentials-local is disposed: cannot ${verb} "${ref}"`) } this.assertUnshadowed(ref, verb) - // The stored tail is settled on both outcomes, so chaining needs no catch - // and one rejected write can never poison the queue for later callers. - const previous = this.writeChain - const run = previous.then(async () => { + return this.enqueue(async () => { if (this.isClosed()) { throw new Error(`credentials-local was disposed before the queued "${ref}" ${verb} ran`) } // Re-judged at run time: the environment may have changed while queued. this.assertUnshadowed(ref, verb) - const existing = this.values.get(ref) - if (value === undefined && existing === undefined) return - if (existing !== undefined && existing.includes('\n')) { - throw new Error( - `credentials-local: "${ref}" is a multi-line entry this line editor would corrupt; edit ${this.spec.filename} directly`, - ) - } - const nextText = upsertLine(this.text, ref, value === undefined ? undefined : renderLine(ref, value)) - // 0600: a document holding secrets is never world-readable. - await writeFileAtomic(this.spec.filename, nextText, { mode: 0o600 }) - this.text = nextText - if (value === undefined) this.values.delete(ref) - else this.values.set(ref, value) - this.ctx.emit('credentials/updated', ref) + // The writer lock's exclusive create needs the parent to exist; 0700 + // because the harness home holds user-private data. + await mkdir(dirname(this.spec.filename), { recursive: true, mode: 0o700 }) + await withFileLock(this.spec.filename, 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 line edit below can never resurrect a stale document. + await this.reconcileFromDisk() + const existing = this.values.get(ref) + if (value === undefined && existing === undefined) return + if (existing !== undefined && existing.includes('\n')) { + throw new Error( + `credentials-local: "${ref}" is a multi-line entry this line editor would corrupt; edit ${this.spec.filename} directly`, + ) + } + const nextText = upsertLine(this.text, ref, value === undefined ? undefined : renderLine(ref, value)) + // 0600: a document holding secrets is never world-readable. + await writeFileAtomic(this.spec.filename, nextText, { mode: 0o600, dirMode: 0o700 }) + this.text = nextText + if (value === undefined) this.values.delete(ref) + else this.values.set(ref, value) + // After the commit: a broken observer must never make the durable + // write look failed (an INVARIANT failure still rethrows). + this.notifyUpdated(ref) + }, { + onStaleBreak: (lockPath) => { + this.ctx.logger.warn('credentials-local: breaking a stale writer lock at %s', lockPath) + }, + }) }) - this.writeChain = run.then(() => undefined, () => undefined) - return run } /** Reject a write the live environment would shadow into apparent no-effect. */ @@ -294,19 +393,33 @@ export class CredentialsLocal extends Credentials { * Re-read the document after a watcher event. Unchanged content (including * this provider's own writes) is a no-op; an unreadable document keeps the * last good snapshot and warns — a live hot-reload must never take the - * process down. dotenv parsing is lenient by design and cannot fail. + * process down. An invariant violation escaping the fan-out is not a reload + * failure and propagates to the queue's error surface. */ private async refresh(): Promise { if (this.closed) return + try { + await this.reconcileFromDisk() + } catch (error) { + if ((error as { code?: unknown } | null)?.code === 'INVARIANT') throw error + this.ctx.logger.warn('credentials-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 store; an unreadable file + * throws, so each caller picks its policy — a reload warns and keeps the + * last good snapshot, a write fails loud. dotenv parsing is lenient by + * design and cannot fail. + */ + 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('credentials-local: reload failed at %s; keeping the last good document', this.spec.filename) - this.ctx.logger.warn(error) - return - } + if (!isENOENT(error)) throw error text = undefined } if (text === this.text || this.isClosed()) return @@ -314,7 +427,7 @@ export class CredentialsLocal extends Credentials { const changed = this.changedRefs(this.values, next) this.text = text this.values = next - for (const ref of changed) this.ctx.emit('credentials/updated', ref) + for (const ref of changed) this.notifyUpdated(ref) } /** Seam-addressable entries whose effective (non-empty) value changed. */ diff --git a/packages/credentials/credentials-local/tests/drain.spec.ts b/packages/credentials/credentials-local/tests/drain.spec.ts index 6c05759b54..baefbd52c5 100644 --- a/packages/credentials/credentials-local/tests/drain.spec.ts +++ b/packages/credentials/credentials-local/tests/drain.spec.ts @@ -6,11 +6,15 @@ import { join } from 'node:path' import { credentialRef } from '@deepseek-ai/dsh-credentials' import { CredentialsLocal } from '../src/index.ts' -// The atomic write is the only asynchronous hold point inside a queued write; -// gating it makes the dispose-versus-queued-write race fully deterministic. -vi.mock('@deepseek-ai/dsh-atomic-write', () => { +// The atomic write is the gated asynchronous hold point inside a queued +// write; gating it makes the dispose-versus-queued-write race fully +// deterministic. The lock helper passes through so the gated operation still +// runs inside its real acquire/release cycle. +vi.mock('@deepseek-ai/dsh-atomic-write', async (importOriginal) => { + const actual = await importOriginal() let gate: Promise = Promise.resolve() return { + ...actual, writeFileAtomic: vi.fn(() => gate), __setGate: (next: Promise) => { gate = next diff --git a/packages/credentials/credentials-local/tests/review-fixes.spec.ts b/packages/credentials/credentials-local/tests/review-fixes.spec.ts new file mode 100644 index 0000000000..7583cf0813 --- /dev/null +++ b/packages/credentials/credentials-local/tests/review-fixes.spec.ts @@ -0,0 +1,202 @@ +// Third-review behaviors: read-modify-write under the writer lock (external +// edits survive an API write), the contained credentials/updated fan-out (a +// broken observer never fails a committed write), and the physical-line +// editor's multi-line and CRLF discipline. +import { afterEach, describe, expect, it, vi } from 'vitest' +import { Context } from 'cordis' +import { mkdtemp, readFile, rm, stat, utimes, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { credentialRef } from '@deepseek-ai/dsh-credentials' +import { CredentialsLocal } from '../src/index.ts' + +const ALPHA = credentialRef('DSH_REVIEW_ALPHA') +const BETA = credentialRef('DSH_REVIEW_BETA') +const INNER = credentialRef('DSH_REVIEW_INNER') + +const cleanups: Array<() => Promise> = [] + +afterEach(async () => { + while (cleanups.length > 0) await cleanups.pop()!() +}) + +async function tempDir(): Promise { + const dir = await mkdtemp(join(tmpdir(), 'dsh-cred-review-')) + 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(CredentialsLocal, config) + cleanups.push(async () => { await fiber.dispose() }) + await fiber + return ctx +} + +describe('read-modify-write', () => { + it('folds an unobserved external edit into a write instead of overwriting it', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + const ctx = await boot({ path, watch: false }) + const seen: string[] = [] + ctx.on('credentials/updated', (ref) => { seen.push(ref) }) + await ctx.credentials.set(ALPHA, 'one') + // The external edit has landed on disk but no watcher reported it (watch + // is off — the same blind spot as a debounce window or a missed event). + await writeFile(path, `${ALPHA}=one\n${BETA}=external\n`) + await ctx.credentials.set(ALPHA, 'two') + const text = await readFile(path, 'utf8') + expect(text).toContain(`${BETA}=external`) + expect(text).toContain(`${ALPHA}=two`) + // The fold published the unobserved entry before the write's own commit. + expect(seen).toEqual([ALPHA, BETA, ALPHA]) + expect(await ctx.credentials.resolve(BETA)).toEqual({ value: 'external', source: 'file' }) + }) + + it('keeps both refs when two providers write the same document concurrently', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + const first = await boot({ path, watch: false }) + const second = await boot({ path, watch: false }) + await Promise.all([ + (async () => { for (const value of ['1', '2', '3'] as const) await first.credentials.set(ALPHA, value) })(), + (async () => { for (const value of ['1', '2', '3'] as const) await second.credentials.set(BETA, value) })(), + ]) + const third = await boot({ path, watch: false }) + expect(await third.credentials.resolve(ALPHA)).toEqual({ value: '3', source: 'file' }) + expect(await third.credentials.resolve(BETA)).toEqual({ value: '3', source: 'file' }) + }) + + it('breaks a stale writer lock with a warning and writes through', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + const ctx = await boot({ path, watch: false }) + await writeFile(`${path}.lock`, 'crashed-holder\n') + const past = (Date.now() - 60_000) / 1000 + await utimes(`${path}.lock`, past, past) + await ctx.credentials.set(ALPHA, 'nine') + expect(await readFile(path, 'utf8')).toContain(`${ALPHA}=nine`) + }) + + it('creates the credentials directory owner-only', async () => { + const dir = await tempDir() + const home = join(dir, 'home') + const ctx = await boot({ path: join(home, '.env'), watch: false }) + await ctx.credentials.set(ALPHA, 'one') + expect((await stat(home)).mode & 0o777).toBe(0o700) + }) +}) + +describe('contained update fan-out', () => { + it('does not fail a committed set when a listener throws, and later listeners still run', async () => { + const dir = await tempDir() + const ctx = await boot({ path: join(dir, '.env'), watch: false }) + ctx.on('credentials/updated', () => { + throw new Error('observer boom') + }) + const second = vi.fn() + ctx.on('credentials/updated', second) + await expect(ctx.credentials.set(ALPHA, 'one')).resolves.toBeUndefined() + expect(second).toHaveBeenCalledWith(ALPHA) + expect(await ctx.credentials.resolve(ALPHA)).toEqual({ value: 'one', source: 'file' }) + }) + + it('contains an async listener rejection', async () => { + const dir = await tempDir() + const ctx = await boot({ path: join(dir, '.env'), watch: false }) + // An unknown-returning function keeps the typed surface legal while the + // runtime value is still the rejected promise the containment must handle. + const boom = (): unknown => Promise.reject(new Error('async observer boom')) + ctx.on('credentials/updated', boom) + await expect(ctx.credentials.set(ALPHA, 'one')).resolves.toBeUndefined() + await new Promise(resolve => setTimeout(resolve, 10)) + }) + + it('rethrows an invariant-coded failure after the commit and the remaining listeners', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + const ctx = await boot({ path, watch: false }) + ctx.on('credentials/updated', () => { + throw Object.assign(new Error('forged relation'), { code: 'INVARIANT' }) + }) + const second = vi.fn() + ctx.on('credentials/updated', second) + await expect(ctx.credentials.set(ALPHA, 'one')).rejects.toThrow(/forged relation/) + // Harness-fatal by design — but the write itself committed first. + expect(second).toHaveBeenCalledWith(ALPHA) + expect(await readFile(path, 'utf8')).toContain(`${ALPHA}=one`) + expect(await ctx.credentials.resolve(ALPHA)).toEqual({ value: 'one', source: 'file' }) + }) +}) + +describe('physical-line editor', () => { + it('never mistakes a quoted multi-line continuation for an assignment', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + const wrapped = `DSH_REVIEW_WRAPPED="line1\n${INNER}=looks-like-one\nline3"\n${ALPHA}=a\n` + await writeFile(path, wrapped) + const ctx = await boot({ path, watch: false }) + await ctx.credentials.set(ALPHA, 'b') + // The wrapped value survives byte-for-byte; only ALPHA's line changed. + const afterAlpha = await readFile(path, 'utf8') + expect(afterAlpha).toBe(`DSH_REVIEW_WRAPPED="line1\n${INNER}=looks-like-one\nline3"\n${ALPHA}=b\n`) + // Setting the inner-looking ref appends a real assignment; the + // continuation line inside the quoted value stays untouched. + await ctx.credentials.set(INNER, 'real') + const afterInner = await readFile(path, 'utf8') + expect(afterInner).toBe(`DSH_REVIEW_WRAPPED="line1\n${INNER}=looks-like-one\nline3"\n${ALPHA}=b\n${INNER}=real\n`) + expect(await ctx.credentials.resolve(INNER)).toEqual({ value: 'real', source: 'file' }) + }) + + it('preserves CRLF line endings on untouched and edited lines', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + await writeFile(path, `# note\r\n${ALPHA}=a\r\n${BETA}=keep\r\n`) + const ctx = await boot({ path, watch: false }) + await ctx.credentials.set(ALPHA, 'b') + expect(await readFile(path, 'utf8')).toBe(`# note\r\n${ALPHA}=b\r\n${BETA}=keep\r\n`) + await ctx.credentials.set(INNER, 'new') + expect(await readFile(path, 'utf8')).toBe(`# note\r\n${ALPHA}=b\r\n${BETA}=keep\r\n${INNER}=new\r\n`) + }) + + it('terminates a final unterminated line before appending', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + await writeFile(path, `${ALPHA}=a`) + const ctx = await boot({ path, watch: false }) + await ctx.credentials.set(BETA, 'b') + expect(await readFile(path, 'utf8')).toBe(`${ALPHA}=a\n${BETA}=b\n`) + }) + + it('rewrites a final unterminated assignment in the dominant ending style', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + await writeFile(path, `${ALPHA}=a`) + const ctx = await boot({ path, watch: false }) + await ctx.credentials.set(ALPHA, 'b') + expect(await readFile(path, 'utf8')).toBe(`${ALPHA}=b\n`) + }) + + it('tracks a single-quoted multi-line value through its continuation', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + await writeFile(path, `DSH_REVIEW_SQ='line1\n${INNER}=shadow\nline3'\n`) + const ctx = await boot({ path, watch: false }) + await ctx.credentials.set(ALPHA, 'x') + expect(await readFile(path, 'utf8')) + .toBe(`DSH_REVIEW_SQ='line1\n${INNER}=shadow\nline3'\n${ALPHA}=x\n`) + }) + + it('reports a multi-line entry as unwritable and refuses to edit it', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + await writeFile(path, `${ALPHA}="line1\nline2"\n`) + const ctx = await boot({ path, watch: false }) + expect(await ctx.credentials.describe(ALPHA)).toEqual({ configured: true, source: 'file', writable: false }) + await expect(ctx.credentials.set(ALPHA, 'flat')).rejects.toThrow(/multi-line entry/) + await expect(ctx.credentials.unset(ALPHA)).rejects.toThrow(/multi-line entry/) + // Resolution still serves the multi-line value. + expect(await ctx.credentials.resolve(ALPHA)).toEqual({ value: 'line1\nline2', source: 'file' }) + }) +}) diff --git a/packages/credentials/credentials-local/tests/watcher.spec.ts b/packages/credentials/credentials-local/tests/watcher.spec.ts index 798cdc8a88..6ff53252cf 100644 --- a/packages/credentials/credentials-local/tests/watcher.spec.ts +++ b/packages/credentials/credentials-local/tests/watcher.spec.ts @@ -151,6 +151,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) }) @@ -204,4 +205,19 @@ describe('watcher pipeline', () => { await new Promise(resolve => setTimeout(resolve, 50)) expect(await ctx.credentials.resolve(KEY)).toBeUndefined() }) + + it('reconciles at watcher ready so a change during setup is not missed', async () => { + const dir = await tempDir() + const path = join(dir, '.env') + await writeFile(path, `${KEY}=a\n`) + const ctx = await boot({ path, debounceMs: 5 }) + // Written after the initial load but before the watcher became active: + // no 'all' event will ever fire for it. + await writeFile(path, `${KEY}=written-before-ready\n`) + const [instance] = await fakeInstances() + instance!.watcher.emit('ready') + await vi.waitFor(async () => { + expect(await ctx.credentials.resolve(KEY)).toEqual({ value: 'written-before-ready', source: 'file' }) + }) + }) }) diff --git a/packages/credentials/credentials/src/index.ts b/packages/credentials/credentials/src/index.ts index 2df89132ac..b640b42881 100644 --- a/packages/credentials/credentials/src/index.ts +++ b/packages/credentials/credentials/src/index.ts @@ -55,7 +55,12 @@ declare module 'cordis' { /** * Committed change to a provider-managed credential source: a `set`, an * `unset`, or an external edit observed in storage. Ambient - * process-environment changes are not observable and never emit. + * process-environment changes are not observable and never emit. Listener + * failures are contained and logged — a sync throw and an async rejection + * alike — without changing the committed operation's outcome, except + * `INVARIANT`-coded failures, which rethrow after every listener ran; + * that rethrow reaches the emitter only from synchronous listeners, so + * invariant checks on this event must not be async functions. * @param ref - the reference whose stored value changed. * @mode emit */ @@ -109,6 +114,49 @@ export abstract class Credentials extends Service { * @param ref - the reference to remove. */ abstract unset(ref: CredentialRef): Promise + + /* jscpd:ignore-start -- deliberate symmetry with the settings seam's commit + fan-out: the contained-dispatch shape is the reviewed listener-lifecycle + contract, and extracting it would couple the two seams' event semantics. */ + /** + * Fan `credentials/updated` out with contained listener failures: every + * listener runs, and a sync throw or async rejection is logged without + * changing the committed operation's outcome — except `INVARIANT`-coded + * failures, which rethrow after every listener ran (the rethrow reaches the + * caller only from synchronous listeners, so invariant checks on this event + * must not be async functions). Providers call this only after the write or + * reload actually committed, so a broken observer can never make a durable + * change look failed. + * @param ref - the reference whose stored value changed. + */ + protected notifyUpdated(ref: CredentialRef): void { + let invariantFailure: unknown + const args = ['credentials/updated', ref] + for (const listener of this.ctx.events.dispatch('emit', args) as Array<(...listenerArgs: unknown[]) => unknown>) { + try { + const returned = listener(ref) + if (returned != null && typeof (returned as PromiseLike).then === 'function') { + void Promise.resolve(returned as PromiseLike).then(undefined, (error: unknown) => { + this.warnListenerFailure(ref, error) + }) + } + } catch (error) { + if ((error as { code?: unknown } | null)?.code === 'INVARIANT') { + invariantFailure ??= error + continue + } + this.warnListenerFailure(ref, error) + } + } + if (invariantFailure !== undefined) throw invariantFailure as Error + } + /* jscpd:ignore-end */ + + /** Contained-listener diagnostic shared by the sync and async failure paths. */ + private warnListenerFailure(ref: CredentialRef, error: unknown): void { + this.ctx.logger.warn('credentials: a credentials/updated listener for "%s" failed', ref) + this.ctx.logger.warn(error) + } } export default Credentials diff --git a/packages/settings/settings-local/src/index.ts b/packages/settings/settings-local/src/index.ts index c129285ea6..8043e6db45 100644 --- a/packages/settings/settings-local/src/index.ts +++ b/packages/settings/settings-local/src/index.ts @@ -10,10 +10,10 @@ import { Context, Service } from 'cordis' import z from 'schemastery' import { watch as chokidarWatch } from 'chokidar' -import { mkdir, readFile, rm, stat, writeFile } from 'node:fs/promises' +import { mkdir, readFile } from 'node:fs/promises' import { dirname, extname, join, resolve } from 'node:path' import { Document, parseDocument } from 'yaml' -import { writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' +import { withFileLock, writeFileAtomic } from '@deepseek-ai/dsh-atomic-write' import { resolveDshHome } from '@deepseek-ai/dsh-paths' import { Settings, deepEqualJson, type SettingsNamespace } from '@deepseek-ai/dsh-settings' @@ -96,23 +96,6 @@ 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({ @@ -199,8 +182,9 @@ export class SettingsLocal extends Settings { private async persistSection(ns: SettingsNamespace, section: Record): Promise { // The writer lock's exclusive create needs the parent to exist before // writeFileAtomic gets its own chance to create it. - await mkdir(dirname(this.spec.filename), { recursive: true }) - await this.withWriterLock(async () => { + // 0700: the harness home holds user-private documents. + await mkdir(dirname(this.spec.filename), { recursive: true, mode: 0o700 }) + await withFileLock(this.spec.filename, 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 @@ -212,59 +196,13 @@ export class SettingsLocal extends Settings { ? this.renderYaml(ns, section) : this.renderJson(ns, section) // 0600: a document that may hold personal values is never world-readable. - await writeFileAtomic(this.spec.filename, output, { mode: 0o600 }) + await writeFileAtomic(this.spec.filename, output, { mode: 0o600, dirMode: 0o700 }) 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) { + }, { + onStaleBreak: (lockPath) => { 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 - } + }, + }) } override async* [Service.init](): AsyncGenerator<() => Promise | void, void, void> { diff --git a/packages/util/atomic-write/src/index.ts b/packages/util/atomic-write/src/index.ts index f4a20c10bc..e7148f41ad 100644 --- a/packages/util/atomic-write/src/index.ts +++ b/packages/util/atomic-write/src/index.ts @@ -1,14 +1,17 @@ /** - * Zero-dependency atomic file replacement. `writeFileAtomic` writes a - * random-suffix sibling with exclusive create and the caller's permission - * bits, then renames it over the target, so readers observe either the old or - * the new complete content and a replaced file ends up with exactly the - * stated mode. + * Zero-dependency atomic file replacement and writer coordination. + * `writeFileAtomic` writes a random-suffix sibling with exclusive create and + * the caller's permission bits, then renames it over the target, so readers + * observe either the old or the new complete content and a replaced file ends + * up with exactly the stated mode. `withFileLock` serializes cross-process + * writers of one file through a `wx`-created `.lock` sibling, so a + * read-modify-write cycle can never resurrect a state another writer just + * replaced; readers stay lock-free because the rename commit is atomic. * @module @deepseek-ai/dsh-atomic-write */ import { randomBytes } from 'node:crypto' -import { mkdir, rename, rm, writeFile } from 'node:fs/promises' +import { mkdir, rename, rm, stat, writeFile } from 'node:fs/promises' import { dirname } from 'node:path' /** @@ -21,6 +24,12 @@ export interface WriteFileAtomicOptions { * rename (subject to the process umask, like every fresh inode). */ mode: number + /** + * Permission bits for parent directories this call creates (subject to the + * umask; existing directories keep their mode). Omission uses the mkdir + * default — pass `0o700` when the tree holds user-private data. + */ + dirMode?: number } /** @@ -38,7 +47,10 @@ export interface WriteFileAtomicOptions { * @param options - permission bits for the replacement inode. */ export async function writeFileAtomic(filename: string, content: string, options: WriteFileAtomicOptions): Promise { - await mkdir(dirname(filename), { recursive: true }) + await mkdir(dirname(filename), { + recursive: true, + ...options.dirMode === undefined ? {} : { mode: options.dirMode }, + }) const temp = `${filename}.${randomBytes(6).toString('hex')}.tmp` try { await writeFile(temp, content, { mode: options.mode, flag: 'wx' }) @@ -48,3 +60,94 @@ export async function writeFileAtomic(filename: string, content: string, options throw error } } + +/** Whether an exclusive create failed because the path already exists. */ +function isEEXIST(error: unknown): boolean { + return (error as NodeJS.ErrnoException | null)?.code === 'EEXIST' +} + +/** Whether a filesystem error means absence. */ +function isENOENT(error: unknown): boolean { + return (error as NodeJS.ErrnoException | null)?.code === 'ENOENT' +} + +/** + * Writer-lock protocol constants. These are robustness invariants of the + * cross-process write protocol, not deployment tunables: a holder rewrites one + * small file 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 + +/** Options for {@link withFileLock}. */ +export interface WithFileLockOptions { + /** + * Called once each time a stale (crashed-holder) lock is broken, so the + * caller can log the takeover in its own voice. + */ + onStaleBreak?: (lockPath: string) => void +} + +/** Age of the lock file, or `undefined` when it vanished after a failed create. */ +async function lockAgeMs(lockPath: string): Promise { + try { + return Date.now() - (await stat(lockPath)).mtimeMs + } catch (error) { + if (!isENOENT(error)) throw error + return undefined + } +} + +/** + * Hold the cross-process writer lock for `filename` around one operation. The + * lock is a `wx`-created sibling (`.lock`); paired with the + * rename-based commit of {@link writeFileAtomic}, readers stay lock-free and + * only writers contend. Contention backs off exponentially; a lock older than + * the stale age is a crashed holder and is broken (see + * {@link WithFileLockOptions.onStaleBreak}); a live holder past the deadline + * fails the operation with a timed-out error. The parent directory must exist. + * @param filename - the file whose writers this lock serializes. + * @param operation - the read-render-commit cycle to run while holding the lock. + * @param options - stale-takeover notification hook. + * @returns the operation's result; the lock releases on both outcomes. + */ +export async function withFileLock( + filename: string, + operation: () => Promise, + options?: WithFileLockOptions, +): Promise { + const lockPath = `${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 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) { + options?.onStaleBreak?.(lockPath) + await rm(lockPath, { force: true }) + continue + } + if (Date.now() >= deadline) { + throw new Error(`atomic-write: 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 }) + } +}