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.
This commit is contained in:
@@ -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<Config> = z.object({
|
||||
@@ -199,8 +182,9 @@ export class SettingsLocal extends Settings {
|
||||
private async persistSection(ns: SettingsNamespace, section: Record<string, unknown>): Promise<void> {
|
||||
// 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 (`<file>.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<T>(operation: () => Promise<T>): Promise<T> {
|
||||
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<number | undefined> {
|
||||
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, void> {
|
||||
|
||||
Reference in New Issue
Block a user