diff --git a/src/services/settingsSync/index.ts b/src/services/settingsSync/index.ts index 2d392b7584..92267050f0 100644 --- a/src/services/settingsSync/index.ts +++ b/src/services/settingsSync/index.ts @@ -33,9 +33,8 @@ import { getAPIProvider, isFirstPartyAnthropicBaseUrl, } from '../../utils/model/providers.js' -import { markInternalWrite } from '../../utils/settings/internalWrites.js' import { getSettingsFilePathForSource } from '../../utils/settings/settings.js' -import { resetSettingsCache } from '../../utils/settings/settingsCache.js' +import { replaceSettingsFileSync } from '../../utils/settings/settingsFileTransaction.js' import { sleep } from '../../utils/sleep.js' import { getClaudeCodeUserAgent } from '../../utils/userAgent.js' import { getFeatureValue_CACHED_MAY_BE_STALE } from '../analytics/growthbook.js' @@ -477,6 +476,20 @@ async function writeFileForSync( } } +function writeSettingsFileForSync( + filePath: string, + content: string, +): boolean { + try { + replaceSettingsFileSync(filePath, content) + logForDiagnosticsNoPII('info', 'settings_sync_file_written') + return true + } catch { + logForDiagnosticsNoPII('warn', 'settings_sync_file_write_failed') + return false + } +} + /** * Apply remote entries to local files (CCR pull pattern). * Only writes files that match expected keys. @@ -488,10 +501,14 @@ async function writeFileForSync( async function applyRemoteEntriesToLocal( entries: Record, projectId: string | null, -): Promise { +): Promise<{ + appliedCount: number + settingsFilesWritten: number + memoryFilesWritten: number +}> { let appliedCount = 0 - let settingsWritten = false - let memoryWritten = false + let settingsFilesWritten = 0 + let memoryFilesWritten = 0 // Helper to check size limit (defense-in-depth, matches backend limit) const exceedsSizeLimit = (content: string, _path: string): boolean => { @@ -514,11 +531,9 @@ async function applyRemoteEntriesToLocal( userSettingsPath && !exceedsSizeLimit(userSettingsContent, userSettingsPath) ) { - // Mark as internal write to prevent spurious change detection - markInternalWrite(userSettingsPath) - if (await writeFileForSync(userSettingsPath, userSettingsContent)) { + if (writeSettingsFileForSync(userSettingsPath, userSettingsContent)) { appliedCount++ - settingsWritten = true + settingsFilesWritten++ } } } @@ -530,7 +545,7 @@ async function applyRemoteEntriesToLocal( if (!exceedsSizeLimit(userMemoryContent, userMemoryPath)) { if (await writeFileForSync(userMemoryPath, userMemoryContent)) { appliedCount++ - memoryWritten = true + memoryFilesWritten++ } } } @@ -545,11 +560,11 @@ async function applyRemoteEntriesToLocal( localSettingsPath && !exceedsSizeLimit(projectSettingsContent, localSettingsPath) ) { - // Mark as internal write to prevent spurious change detection - markInternalWrite(localSettingsPath) - if (await writeFileForSync(localSettingsPath, projectSettingsContent)) { + if ( + writeSettingsFileForSync(localSettingsPath, projectSettingsContent) + ) { appliedCount++ - settingsWritten = true + settingsFilesWritten++ } } } @@ -561,21 +576,33 @@ async function applyRemoteEntriesToLocal( if (!exceedsSizeLimit(projectMemoryContent, localMemoryPath)) { if (await writeFileForSync(localMemoryPath, projectMemoryContent)) { appliedCount++ - memoryWritten = true + memoryFilesWritten++ } } } } // Invalidate caches so subsequent reads pick up new content - if (settingsWritten) { - resetSettingsCache() - } - if (memoryWritten) { + if (memoryFilesWritten > 0) { clearMemoryFileCaches() } logForDiagnosticsNoPII('info', 'settings_sync_applied', { appliedCount, + settingsFilesWritten, + memoryFilesWritten, }) + return { appliedCount, settingsFilesWritten, memoryFilesWritten } +} + +/** @internal Direct apply seam for focused settings-file transaction tests. */ +export function _applyRemoteEntriesToLocalForTesting( + entries: Record, + projectId: string | null, +): Promise<{ + appliedCount: number + settingsFilesWritten: number + memoryFilesWritten: number +}> { + return applyRemoteEntriesToLocal(entries, projectId) } diff --git a/src/services/settingsSync/settings.transaction.test.ts b/src/services/settingsSync/settings.transaction.test.ts new file mode 100644 index 0000000000..769586d066 --- /dev/null +++ b/src/services/settingsSync/settings.transaction.test.ts @@ -0,0 +1,292 @@ +import { spawn, type ChildProcessByStdio } from 'node:child_process' +import { + existsSync, + mkdtempSync, + mkdirSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs' +import { tmpdir } from 'node:os' +import { dirname, join, resolve } from 'node:path' +import type { Readable } from 'node:stream' +import { expect, test } from 'bun:test' +import { getOriginalCwd, setOriginalCwd } from '../../bootstrap/state.js' +import { + getClaudeConfigHomeDirOverrideForTesting, + setClaudeConfigHomeDirForTesting, +} from '../../utils/envUtils.js' +import { resetSettingsCache } from '../../utils/settings/settingsCache.js' +import { _applyRemoteEntriesToLocalForTesting } from './index.js' +import { SYNC_KEYS } from './types.js' + +const fixturePath = resolve( + import.meta.dir, + '../../test/fixtures/settingsTransactionWriter.fixture.ts', +) +const CHILD_TIMEOUT_MS = 15_000 +const TEST_TIMEOUT_MS = CHILD_TIMEOUT_MS + 5_000 + +type Holder = { + process: ChildProcessByStdio + exited: Promise<{ code: number | null; signal: NodeJS.Signals | null }> + output: () => { stdout: string; stderr: string } +} + +function startHolder( + targetPath: string, + holdMs: number, + enteredMarker: string, + completedMarker: string, +): Holder { + const child = spawn( + process.execPath, + [ + fixturePath, + 'hold-path-for', + targetPath, + 'unused', + String(holdMs), + enteredMarker, + completedMarker, + ], + { + cwd: process.cwd(), + env: { ...process.env, FORCE_COLOR: '0' }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ) + let stdout = '' + let stderr = '' + child.stdout.setEncoding('utf8') + child.stderr.setEncoding('utf8') + child.stdout.on('data', chunk => { + stdout += chunk + }) + child.stderr.on('data', chunk => { + stderr += chunk + }) + return { + process: child, + exited: new Promise(resolveExit => { + child.once('exit', (code, signal) => resolveExit({ code, signal })) + }), + output: () => ({ stdout, stderr }), + } +} + +function delay(ms: number): Promise { + return new Promise(resolveDelay => setTimeout(resolveDelay, ms)) +} + +async function waitForHolder(marker: string, holder: Holder): Promise { + const deadline = performance.now() + CHILD_TIMEOUT_MS + while (!existsSync(marker)) { + if ( + holder.process.exitCode !== null || + holder.process.signalCode !== null + ) { + const { stdout, stderr } = holder.output() + throw new Error( + `Holder exited before acquiring the lock\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ) + } + if (performance.now() >= deadline) { + throw new Error('Timed out waiting for holder to acquire the lock') + } + await delay(10) + } +} + +async function finishHolder(holder: Holder): Promise { + const outcome = await Promise.race([ + holder.exited, + delay(CHILD_TIMEOUT_MS).then(() => { + throw new Error('Holder did not exit') + }), + ]) + const { stdout, stderr } = holder.output() + if (outcome.code !== 0) { + throw new Error( + `Holder exited with code ${outcome.code ?? 'null'} and signal ${outcome.signal ?? 'none'}\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ) + } +} + +async function terminateHolder(holder: Holder | undefined): Promise { + if ( + !holder || + holder.process.exitCode !== null || + holder.process.signalCode !== null + ) { + return + } + holder.process.kill('SIGTERM') + await Promise.race([holder.exited, delay(500)]) + if ( + holder.process.exitCode === null && + holder.process.signalCode === null + ) { + holder.process.kill('SIGKILL') + await holder.exited + } +} + +async function withSyncEnvironment( + run: (paths: { + root: string + userSettings: string + userMemory: string + localSettings: string + }) => Promise, +): Promise { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-sync-')) + const project = join(root, 'project') + const previousConfig = getClaudeConfigHomeDirOverrideForTesting() + const previousCwd = getOriginalCwd() + mkdirSync(project) + setClaudeConfigHomeDirForTesting(root) + setOriginalCwd(project) + resetSettingsCache() + try { + await run({ + root, + userSettings: join(root, 'settings.json'), + userMemory: join(root, 'CLAUDE.md'), + localSettings: join(project, '.openclaude', 'settings.local.json'), + }) + } finally { + setClaudeConfigHomeDirForTesting(previousConfig) + setOriginalCwd(previousCwd) + resetSettingsCache() + rmSync(root, { recursive: true, force: true }) + } +} + +test( + 'user settings sync waits for the shared transaction lock and succeeds', + async () => { + await withSyncEnvironment(async ({ root, userSettings }) => { + writeFileSync(userSettings, '{}\n') + const entered = join(root, 'user-holder-entered') + const completed = join(root, 'user-holder-completed') + const holder = startHolder(userSettings, 1_000, entered, completed) + try { + await waitForHolder(entered, holder) + const startedAt = performance.now() + const result = await _applyRemoteEntriesToLocalForTesting( + { + [SYNC_KEYS.USER_SETTINGS]: '{"env":{"SYNCED":"yes"}}\n', + }, + null, + ) + const elapsedMs = performance.now() - startedAt + + expect(result).toEqual({ + appliedCount: 1, + settingsFilesWritten: 1, + memoryFilesWritten: 0, + }) + expect(elapsedMs).toBeGreaterThanOrEqual(500) + expect(JSON.parse(readFileSync(userSettings, 'utf8')).env).toEqual({ + SYNCED: 'yes', + }) + await finishHolder(holder) + expect(existsSync(`${userSettings}.lock`)).toBe(false) + } finally { + await terminateHolder(holder) + } + }) + }, + TEST_TIMEOUT_MS, +) + +test( + 'local settings sync uses the same physical-target lock', + async () => { + await withSyncEnvironment(async ({ root, localSettings }) => { + mkdirSync(dirname(localSettings), { recursive: true }) + writeFileSync(localSettings, '{}\n') + const entered = join(root, 'local-holder-entered') + const completed = join(root, 'local-holder-completed') + const holder = startHolder(localSettings, 1_000, entered, completed) + try { + await waitForHolder(entered, holder) + const result = await _applyRemoteEntriesToLocalForTesting( + { + [SYNC_KEYS.projectSettings('project-id')]: + '{"env":{"LOCAL_SYNCED":"yes"}}\n', + }, + 'project-id', + ) + + expect(result).toEqual({ + appliedCount: 1, + settingsFilesWritten: 1, + memoryFilesWritten: 0, + }) + expect(JSON.parse(readFileSync(localSettings, 'utf8')).env).toEqual({ + LOCAL_SYNCED: 'yes', + }) + await finishHolder(holder) + expect(existsSync(`${localSettings}.lock`)).toBe(false) + } finally { + await terminateHolder(holder) + } + }) + }, + TEST_TIMEOUT_MS, +) + +test( + 'a timed-out settings entry is not reported applied and memory still syncs', + async () => { + await withSyncEnvironment(async ({ root, userMemory, userSettings }) => { + writeFileSync(userSettings, '{"env":{"ORIGINAL":"yes"}}\n') + const entered = join(root, 'timeout-holder-entered') + const completed = join(root, 'timeout-holder-completed') + const holder = startHolder(userSettings, 4_500, entered, completed) + try { + await waitForHolder(entered, holder) + const result = await _applyRemoteEntriesToLocalForTesting( + { + [SYNC_KEYS.USER_SETTINGS]: '{"env":{"REPLACED":"no"}}\n', + [SYNC_KEYS.USER_MEMORY]: 'synced memory\n', + }, + null, + ) + + expect(result).toEqual({ + appliedCount: 1, + settingsFilesWritten: 0, + memoryFilesWritten: 1, + }) + expect(JSON.parse(readFileSync(userSettings, 'utf8')).env).toEqual({ + ORIGINAL: 'yes', + }) + expect(readFileSync(userMemory, 'utf8')).toBe('synced memory\n') + await finishHolder(holder) + + expect( + await _applyRemoteEntriesToLocalForTesting( + { + [SYNC_KEYS.USER_SETTINGS]: '{"env":{"LATER":"yes"}}\n', + }, + null, + ), + ).toEqual({ + appliedCount: 1, + settingsFilesWritten: 1, + memoryFilesWritten: 0, + }) + expect(JSON.parse(readFileSync(userSettings, 'utf8')).env).toEqual({ + LATER: 'yes', + }) + expect(existsSync(`${userSettings}.lock`)).toBe(false) + } finally { + await terminateHolder(holder) + } + }) + }, + TEST_TIMEOUT_MS, +) diff --git a/src/test/fixtures/settingsTransactionWriter.fixture.ts b/src/test/fixtures/settingsTransactionWriter.fixture.ts new file mode 100644 index 0000000000..5b272fbb36 --- /dev/null +++ b/src/test/fixtures/settingsTransactionWriter.fixture.ts @@ -0,0 +1,121 @@ +import { existsSync, realpathSync, writeFileSync } from 'node:fs' +import { basename, dirname, join, resolve } from 'node:path' +import { + getFsImplementation, + setFsImplementation, +} from '../../utils/fsOperations.js' + +const [ + role, + target, + key, + value, + enteredMarker, + completedMarker, + readMarker, + releaseMarker, +] = process.argv.slice(2) + +const supportedRoles: ReadonlySet = new Set([ + 'normal', + 'hold-lock', + 'hold-path-for', + 'pause-after-read', +]) + +if (!role || !supportedRoles.has(role)) { + throw new Error(`Invalid settings transaction fixture role: ${role}`) +} + +if ( + !target || + !key || + !value || + !enteredMarker || + !completedMarker +) { + throw new Error('Missing settings transaction fixture arguments') +} + +if (role !== 'hold-path-for') { + process.env.OPENCLAUDE_CONFIG_DIR = target +} +const settingsPath = + role === 'hold-path-for' + ? resolve(target) + : resolve(target, 'settings.json') +const settingsParentPath = dirname(settingsPath) +const settingsReadPath = existsSync(settingsPath) + ? realpathSync(settingsPath) + : existsSync(settingsParentPath) + ? join(realpathSync(settingsParentPath), basename(settingsPath)) + : settingsPath +const waitBuffer = new Int32Array(new SharedArrayBuffer(4)) + +function waitForMarker(marker: string): void { + const deadline = performance.now() + 15_000 + while (!existsSync(marker)) { + if (performance.now() >= deadline) { + throw new Error(`Timed out waiting for fixture marker: ${marker}`) + } + Atomics.wait(waitBuffer, 0, 0, 10) + } +} + +if (role === 'pause-after-read') { + if (!readMarker || !releaseMarker) { + throw new Error('Pause-after-read fixture requires read and release markers') + } + const originalFs = getFsImplementation() + let paused = false + setFsImplementation({ + ...originalFs, + readFileSync(path, options) { + const content = originalFs.readFileSync(path, options) + if (!paused && resolve(path) === settingsReadPath) { + paused = true + writeFileSync(readMarker, '') + waitForMarker(releaseMarker) + } + return content + }, + }) +} + +if (role === 'hold-lock' || role === 'hold-path-for') { + const { withSettingsFileTransactionSync } = await import( + '../../utils/settings/settingsFileTransaction.js' + ) + withSettingsFileTransactionSync(settingsPath, () => { + writeFileSync(enteredMarker, '') + if (role === 'hold-path-for') { + const holdMs = Number(value) + if (!Number.isFinite(holdMs) || holdMs < 0) { + throw new Error(`Invalid hold duration: ${value}`) + } + Atomics.wait(waitBuffer, 0, 0, holdMs) + } else { + if (!releaseMarker) { + throw new Error('Hold-lock fixture requires a release marker') + } + waitForMarker(releaseMarker) + } + }) + writeFileSync(completedMarker, '') + process.stdout.write(`${JSON.stringify({ ok: true })}\n`) +} else { + const { updateSettingsForSource } = await import( + '../../utils/settings/settings.js' + ) + writeFileSync(enteredMarker, '') + const result = updateSettingsForSource('userSettings', { + env: { [key]: value }, + }) + writeFileSync(completedMarker, '') + process.stdout.write( + `${JSON.stringify({ + ok: result.error === null, + error: result.error?.message, + })}\n`, + ) +} diff --git a/src/utils/settings/settings.transaction.test.ts b/src/utils/settings/settings.transaction.test.ts new file mode 100644 index 0000000000..8b84698c9b --- /dev/null +++ b/src/utils/settings/settings.transaction.test.ts @@ -0,0 +1,662 @@ +import { spawn, type ChildProcessByStdio } from 'node:child_process' +import { + existsSync, + lstatSync, + mkdtempSync, + mkdirSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import type { Readable } from 'node:stream' +import { expect, spyOn, test } from 'bun:test' +import { getOriginalCwd, setOriginalCwd } from '../../bootstrap/state.js' +import { + getClaudeConfigHomeDirOverrideForTesting, + setClaudeConfigHomeDirForTesting, +} from '../envUtils.js' +import * as gitignore from '../git/gitignore.js' +import { + getSettingsForSource, + updateSettingsForSource, +} from './settings.js' +import { + clearInternalWrites, + consumeInternalWrite, +} from './internalWrites.js' +import { resetSettingsCache } from './settingsCache.js' +import type { SettingsJson } from './types.js' + +const fixturePath = resolve( + import.meta.dir, + '../../test/fixtures/settingsTransactionWriter.fixture.ts', +) +const CHILD_TIMEOUT_MS = 20_000 +const TEST_TIMEOUT_MS = CHILD_TIMEOUT_MS + 10_000 + +type CapturedChild = { + process: ChildProcessByStdio + exited: Promise<{ code: number | null; signal: NodeJS.Signals | null }> + output: () => { stdout: string; stderr: string } +} + +function startWriter(args: string[]): CapturedChild { + const child = spawn(process.execPath, [fixturePath, ...args], { + cwd: process.cwd(), + env: { ...process.env, FORCE_COLOR: '0' }, + stdio: ['ignore', 'pipe', 'pipe'], + }) + let stdout = '' + let stderr = '' + child.stdout.setEncoding('utf8') + child.stderr.setEncoding('utf8') + child.stdout.on('data', chunk => { + stdout += chunk + }) + child.stderr.on('data', chunk => { + stderr += chunk + }) + + return { + process: child, + exited: new Promise(resolveExit => { + child.once('exit', (code, signal) => resolveExit({ code, signal })) + }), + output: () => ({ stdout, stderr }), + } +} + +function delay(ms: number): Promise { + return new Promise(resolveDelay => setTimeout(resolveDelay, ms)) +} + +async function waitForMarker( + marker: string, + child: CapturedChild, + label: string, +): Promise { + const deadline = performance.now() + CHILD_TIMEOUT_MS + while (!existsSync(marker)) { + if ( + child.process.exitCode !== null || + child.process.signalCode !== null + ) { + const { stdout, stderr } = child.output() + throw new Error( + `${label} exited before creating its marker\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ) + } + if (performance.now() >= deadline) { + const { stdout, stderr } = child.output() + throw new Error( + `${label} timed out before creating its marker\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ) + } + await delay(10) + } +} + +async function markerAppearsWithin( + marker: string, + child: CapturedChild, + timeoutMs: number, +): Promise { + const deadline = performance.now() + timeoutMs + while (!existsSync(marker)) { + if ( + child.process.exitCode !== null || + child.process.signalCode !== null || + performance.now() >= deadline + ) { + return existsSync(marker) + } + await delay(10) + } + return true +} + +async function finishWriter( + child: CapturedChild, + label: string, +): Promise<{ ok: boolean; error?: string }> { + const outcome = await Promise.race([ + child.exited, + delay(CHILD_TIMEOUT_MS).then(() => { + throw new Error(`${label} did not exit within ${CHILD_TIMEOUT_MS}ms`) + }), + ]) + const { stdout, stderr } = child.output() + if (outcome.code !== 0) { + throw new Error( + `${label} exited with code ${outcome.code ?? 'null'} and signal ${outcome.signal ?? 'none'}\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ) + } + const lastLine = stdout.trim().split(/\r?\n/).at(-1) + if (!lastLine) { + throw new Error(`${label} emitted no JSON\nstderr:\n${stderr}`) + } + try { + return JSON.parse(lastLine) as { ok: boolean; error?: string } + } catch (error) { + throw new Error( + `${label} emitted invalid JSON: ${error}\nstdout:\n${stdout}\nstderr:\n${stderr}`, + ) + } +} + +async function terminateChild(child: CapturedChild): Promise { + if ( + child.process.exitCode !== null || + child.process.signalCode !== null + ) { + return + } + child.process.kill('SIGTERM') + await Promise.race([child.exited, delay(500)]) + if ( + child.process.exitCode === null && + child.process.signalCode === null + ) { + child.process.kill('SIGKILL') + await child.exited + } +} + +async function runConcurrentWriters( + markerRoot: string, + configDirA: string, + configDirB: string, + settingsPath: string, +): Promise<{ + resultA: { ok: boolean; error?: string } + resultB: { ok: boolean; error?: string } + finalSettings: { env?: Record } +}> { + const writerAEntered = join(markerRoot, 'writer-a-entered') + const writerACompleted = join(markerRoot, 'writer-a-completed') + const writerARead = join(markerRoot, 'writer-a-read') + const releaseWriterA = join(markerRoot, 'release-writer-a') + const writerBEntered = join(markerRoot, 'writer-b-entered') + const writerBCompleted = join(markerRoot, 'writer-b-completed') + const children: CapturedChild[] = [] + + try { + const writerA = startWriter([ + 'pause-after-read', + configDirA, + 'WRITER_A', + 'a', + writerAEntered, + writerACompleted, + writerARead, + releaseWriterA, + ]) + children.push(writerA) + await waitForMarker(writerARead, writerA, 'writer A') + + const writerB = startWriter([ + 'normal', + configDirB, + 'WRITER_B', + 'b', + writerBEntered, + writerBCompleted, + ]) + children.push(writerB) + await waitForMarker(writerBEntered, writerB, 'writer B') + + // On unfixed main, B completes against the same old document. With the + // transaction lock, B remains pending behind A. Either observation is + // enough to release A without building a barrier that deadlocks the fix. + await markerAppearsWithin(writerBCompleted, writerB, 500) + writeFileSync(releaseWriterA, '') + + const [resultA, resultB] = await Promise.all([ + finishWriter(writerA, 'writer A'), + finishWriter(writerB, 'writer B'), + ]) + return { + resultA, + resultB, + finalSettings: JSON.parse(readFileSync(settingsPath, 'utf8')) as { + env?: Record + }, + } + } finally { + await Promise.all(children.map(terminateChild)) + } +} + +async function withIsolatedUserSettings( + run: (root: string, settingsPath: string) => void | Promise, +): Promise { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-unit-')) + const previousOverride = getClaudeConfigHomeDirOverrideForTesting() + setClaudeConfigHomeDirForTesting(root) + resetSettingsCache() + try { + await run(root, join(root, 'settings.json')) + } finally { + setClaudeConfigHomeDirForTesting(previousOverride) + resetSettingsCache() + rmSync(root, { recursive: true, force: true }) + } +} + +test( + 'two processes preserve disjoint settings patches during contention', + async () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-race-')) + const settingsPath = join(root, 'settings.json') + + try { + writeFileSync( + settingsPath, + `${JSON.stringify({ env: { BASE: 'base' } }, null, 2)}\n`, + ) + + const { resultA, resultB, finalSettings } = + await runConcurrentWriters(root, root, root, settingsPath) + expect(resultA).toEqual({ ok: true }) + expect(resultB).toEqual({ ok: true }) + expect(finalSettings.env).toEqual({ + BASE: 'base', + WRITER_A: 'a', + WRITER_B: 'b', + }) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }, + TEST_TIMEOUT_MS, +) + +test('reads the merge base from disk after ownership, not from warm caches', async () => { + await withIsolatedUserSettings((_root, settingsPath) => { + writeFileSync( + settingsPath, + `${JSON.stringify({ env: { CACHED: 'yes' } }, null, 2)}\n`, + ) + expect(getSettingsForSource('userSettings')?.env).toEqual({ + CACHED: 'yes', + }) + + writeFileSync( + settingsPath, + `${JSON.stringify({ env: { CACHED: 'yes', EXTERNAL: 'yes' } }, null, 2)}\n`, + ) + expect( + updateSettingsForSource('userSettings', { + env: { LOCAL: 'yes' }, + }), + ).toEqual({ error: null }) + + expect(JSON.parse(readFileSync(settingsPath, 'utf8')).env).toEqual({ + CACHED: 'yes', + EXTERNAL: 'yes', + LOCAL: 'yes', + }) + }) +}) + +test('creates a missing settings file without weakening the synchronous result', async () => { + await withIsolatedUserSettings((_root, settingsPath) => { + expect(existsSync(settingsPath)).toBe(false) + expect( + updateSettingsForSource('userSettings', { env: { CREATED: 'yes' } }), + ).toEqual({ error: null }) + expect(JSON.parse(readFileSync(settingsPath, 'utf8')).env).toEqual({ + CREATED: 'yes', + }) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + }) +}) + +test('preserves deletion, array replacement, and ordinary success semantics', async () => { + await withIsolatedUserSettings((_root, settingsPath) => { + writeFileSync( + settingsPath, + `${JSON.stringify( + { + env: { KEEP: 'yes', REMOVE: 'yes' }, + permissions: { allow: ['Bash(one)'] }, + }, + null, + 2, + )}\n`, + ) + const patch = { + env: { REMOVE: undefined }, + permissions: { allow: ['Bash(two)'] }, + } as unknown as SettingsJson + expect(updateSettingsForSource('userSettings', patch)).toEqual({ + error: null, + }) + + expect(JSON.parse(readFileSync(settingsPath, 'utf8'))).toMatchObject({ + env: { KEEP: 'yes' }, + permissions: { allow: ['Bash(two)'] }, + }) + }) +}) + +test('a malformed document is untouched and does not strand the lock', async () => { + await withIsolatedUserSettings((_root, settingsPath) => { + const malformed = '{"env":' + writeFileSync(settingsPath, malformed) + const failed = updateSettingsForSource('userSettings', { + env: { FIRST: 'no' }, + }) + expect(failed.error?.message).toBe( + `Invalid JSON syntax in settings file at ${settingsPath}`, + ) + expect(readFileSync(settingsPath, 'utf8')).toBe(malformed) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + + writeFileSync(settingsPath, '{}\n') + expect( + updateSettingsForSource('userSettings', { env: { SECOND: 'yes' } }), + ).toEqual({ error: null }) + expect(JSON.parse(readFileSync(settingsPath, 'utf8')).env).toEqual({ + SECOND: 'yes', + }) + }) +}) + +test('does not retry unrelated filesystem errors', async () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-fs-error-')) + const nonDirectory = join(root, 'not-a-directory') + writeFileSync(nonDirectory, '') + try { + const { withSettingsFileTransactionSync } = await import( + './settingsFileTransaction.js' + ) + const startedAt = performance.now() + expect(() => + withSettingsFileTransactionSync( + join(nonDirectory, 'settings.json'), + () => undefined, + ), + ).toThrow() + expect(performance.now() - startedAt).toBeLessThan(500) + } finally { + rmSync(root, { recursive: true, force: true }) + } +}) + +test('reports transaction failures with operation-neutral context', async () => { + await withIsolatedUserSettings((_root, settingsPath) => { + mkdirSync(settingsPath) + const result = updateSettingsForSource('userSettings', { + env: { NEVER_WRITTEN: 'yes' }, + }) + expect(result.error?.message).toContain( + `Failed to update settings at ${settingsPath}:`, + ) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + }) +}) + +test( + 'waits for a short holder and succeeds before the contention deadline', + async () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-wait-')) + const settingsPath = join(root, 'settings.json') + const holderEntered = join(root, 'holder-entered') + const holderCompleted = join(root, 'holder-completed') + const releaseHolder = join(root, 'release-holder') + const writerEntered = join(root, 'writer-entered') + const writerCompleted = join(root, 'writer-completed') + const children: CapturedChild[] = [] + try { + writeFileSync(settingsPath, '{}\n') + const holder = startWriter([ + 'hold-lock', + root, + 'unused', + 'unused', + holderEntered, + holderCompleted, + 'unused', + releaseHolder, + ]) + children.push(holder) + await waitForMarker(holderEntered, holder, 'holder') + + const writer = startWriter([ + 'normal', + root, + 'WAITED', + 'yes', + writerEntered, + writerCompleted, + ]) + children.push(writer) + await waitForMarker(writerEntered, writer, 'waiting writer') + expect(await markerAppearsWithin(writerCompleted, writer, 200)).toBe(false) + + writeFileSync(releaseHolder, '') + expect(await finishWriter(holder, 'holder')).toEqual({ ok: true }) + expect(await finishWriter(writer, 'waiting writer')).toEqual({ ok: true }) + expect(JSON.parse(readFileSync(settingsPath, 'utf8')).env).toEqual({ + WAITED: 'yes', + }) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + } finally { + await Promise.all(children.map(terminateChild)) + rmSync(root, { recursive: true, force: true }) + } + }, + TEST_TIMEOUT_MS, +) + +test( + 'times out a long-held lock clearly and allows a later update', + async () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-timeout-')) + const settingsPath = join(root, 'settings.json') + const holderEntered = join(root, 'holder-entered') + const holderCompleted = join(root, 'holder-completed') + const releaseHolder = join(root, 'release-holder') + const timedEntered = join(root, 'timed-entered') + const timedCompleted = join(root, 'timed-completed') + const laterEntered = join(root, 'later-entered') + const laterCompleted = join(root, 'later-completed') + const children: CapturedChild[] = [] + try { + writeFileSync(settingsPath, '{}\n') + const holder = startWriter([ + 'hold-lock', + root, + 'unused', + 'unused', + holderEntered, + holderCompleted, + 'unused', + releaseHolder, + ]) + children.push(holder) + await waitForMarker(holderEntered, holder, 'holder') + + const timedWriter = startWriter([ + 'normal', + root, + 'TIMED_OUT', + 'no', + timedEntered, + timedCompleted, + ]) + children.push(timedWriter) + await waitForMarker(timedEntered, timedWriter, 'timed writer') + const startedAt = performance.now() + const timedResult = await finishWriter(timedWriter, 'timed writer') + const elapsedMs = performance.now() - startedAt + expect(timedResult.ok).toBe(false) + expect(timedResult.error).toContain('Timed out after 2000ms') + expect(elapsedMs).toBeGreaterThanOrEqual(1_800) + expect(elapsedMs).toBeLessThan(3_500) + expect(JSON.parse(readFileSync(settingsPath, 'utf8'))).toEqual({}) + + writeFileSync(releaseHolder, '') + expect(await finishWriter(holder, 'holder')).toEqual({ ok: true }) + + const laterWriter = startWriter([ + 'normal', + root, + 'LATER', + 'yes', + laterEntered, + laterCompleted, + ]) + children.push(laterWriter) + expect(await finishWriter(laterWriter, 'later writer')).toEqual({ + ok: true, + }) + expect(JSON.parse(readFileSync(settingsPath, 'utf8')).env).toEqual({ + LATER: 'yes', + }) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + } finally { + await Promise.all(children.map(terminateChild)) + rmSync(root, { recursive: true, force: true }) + } + }, + TEST_TIMEOUT_MS, +) + +test( + 'symlinked parent and direct-file aliases share a lock and preserve the links', + async () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-alias-')) + const realConfig = join(root, 'real-config') + const parentAlias = join(root, 'parent-alias') + const directAliasConfig = join(root, 'direct-alias-config') + const settingsPath = join(realConfig, 'settings.json') + const directSettingsAlias = join(directAliasConfig, 'settings.json') + try { + mkdirSync(realConfig) + mkdirSync(directAliasConfig) + writeFileSync( + settingsPath, + `${JSON.stringify({ env: { BASE: 'base' } }, null, 2)}\n`, + ) + try { + symlinkSync( + realConfig, + parentAlias, + process.platform === 'win32' ? 'junction' : 'dir', + ) + symlinkSync(settingsPath, directSettingsAlias, 'file') + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if ( + process.platform === 'win32' && + (code === 'EPERM' || code === 'EACCES') + ) { + return + } + throw error + } + + const { resultA, resultB, finalSettings } = await runConcurrentWriters( + root, + parentAlias, + directAliasConfig, + settingsPath, + ) + expect(resultA).toEqual({ ok: true }) + expect(resultB).toEqual({ ok: true }) + expect(finalSettings.env).toEqual({ + BASE: 'base', + WRITER_A: 'a', + WRITER_B: 'b', + }) + expect(lstatSync(parentAlias).isSymbolicLink()).toBe(true) + expect(lstatSync(directSettingsAlias).isSymbolicLink()).toBe(true) + expect(existsSync(`${settingsPath}.lock`)).toBe(false) + } finally { + rmSync(root, { recursive: true, force: true }) + } + }, + TEST_TIMEOUT_MS, +) + +test('marks the requested logical alias after publishing to its physical target', () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-mark-')) + const realConfig = join(root, 'real-config') + const configAlias = join(root, 'config-alias') + const logicalSettingsPath = join(configAlias, 'settings.json') + const previousOverride = getClaudeConfigHomeDirOverrideForTesting() + try { + mkdirSync(realConfig) + try { + symlinkSync( + realConfig, + configAlias, + process.platform === 'win32' ? 'junction' : 'dir', + ) + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if ( + process.platform === 'win32' && + (code === 'EPERM' || code === 'EACCES') + ) { + return + } + throw error + } + + setClaudeConfigHomeDirForTesting(configAlias) + resetSettingsCache() + clearInternalWrites() + expect( + updateSettingsForSource('userSettings', { env: { MARKED: 'yes' } }), + ).toEqual({ error: null }) + expect(consumeInternalWrite(logicalSettingsPath, 5_000)).toBe(true) + expect( + consumeInternalWrite(join(realConfig, 'settings.json'), 5_000), + ).toBe(false) + } finally { + setClaudeConfigHomeDirForTesting(previousOverride) + resetSettingsCache() + clearInternalWrites() + rmSync(root, { recursive: true, force: true }) + } +}) + +test('local settings still arrange the existing global gitignore rule', () => { + const root = mkdtempSync(join(tmpdir(), 'openclaude-settings-gitignore-')) + const project = join(root, 'project') + const previousOriginalCwd = getOriginalCwd() + const previousOverride = getClaudeConfigHomeDirOverrideForTesting() + const addRule = spyOn( + gitignore, + 'addFileGlobRuleToGitignore', + ).mockResolvedValue(undefined) + try { + mkdirSync(project) + setOriginalCwd(project) + setClaudeConfigHomeDirForTesting(join(root, 'config')) + resetSettingsCache() + + expect( + updateSettingsForSource('localSettings', { + env: { LOCAL: 'yes' }, + }), + ).toEqual({ error: null }) + expect(addRule).toHaveBeenCalledWith( + '.openclaude/settings.local.json', + project, + ) + } finally { + addRule.mockRestore() + setOriginalCwd(previousOriginalCwd) + setClaudeConfigHomeDirForTesting(previousOverride) + resetSettingsCache() + rmSync(root, { recursive: true, force: true }) + } +}) diff --git a/src/utils/settings/settings.ts b/src/utils/settings/settings.ts index 7d5dc8b68f..e737535cff 100644 --- a/src/utils/settings/settings.ts +++ b/src/utils/settings/settings.ts @@ -44,6 +44,7 @@ import { setCachedSettingsForSource, setSessionSettingsCache, } from './settingsCache.js' +import { withSettingsFileTransactionSync } from './settingsFileTransaction.js' import { type SettingsJson, SettingsSchema } from './types.js' import { filterInvalidModelPricing, @@ -439,79 +440,72 @@ export function updateSettingsForSource( } try { - getFsImplementation().mkdirSync(dirname(filePath)) - - // Try to get existing settings with validation. Bypass the per-source - // cache — mergeWith below mutates its target (including nested refs), - // and mutating the cached object would leak unpersisted state if the - // write fails before resetSettingsCache(). - let existingSettings = getSettingsForSourceUncached(source) - - // If validation failed, check if file exists with a JSON syntax error - if (!existingSettings) { - let content: string | null = null - try { - content = readFileSync(filePath) - } catch (e) { - if (!isENOENT(e)) { - throw e - } - // File doesn't exist — fall through to merge with empty settings - } - if (content !== null) { - const rawData = safeParseJSON(content) - if (rawData === null) { - // JSON syntax error - return validation error instead of overwriting - // safeParseJSON will already log the error, so we'll just return the error here - return { - error: new Error( - `Invalid JSON syntax in settings file at ${filePath}`, - ), + const validationError = withSettingsFileTransactionSync( + filePath, + targetPath => { + // The transaction merge base must bypass both process-local settings + // caches so a peer's completed update cannot be overwritten. + let existingSettings = parseSettingsFileUncached(targetPath).settings + + // If validation failed, check if the physical file has a JSON syntax error. + if (!existingSettings) { + let content: string | null = null + try { + content = readFileSync(targetPath) + } catch (e) { + if (!isENOENT(e)) throw e + // File doesn't exist — fall through to merge with empty settings. + } + if (content !== null) { + const rawData = safeParseJSON(content) + if (rawData === null) { + return new Error( + `Invalid JSON syntax in settings file at ${filePath}`, + ) + } + if (rawData && typeof rawData === 'object') { + existingSettings = rawData as SettingsJson + logForDebugging( + `Using raw settings from ${filePath} due to validation failure`, + ) + } } } - if (rawData && typeof rawData === 'object') { - existingSettings = rawData as SettingsJson - logForDebugging( - `Using raw settings from ${filePath} due to validation failure`, - ) - } - } - } - const updatedSettings = mergeWith( - existingSettings || {}, - settings, - ( - _objValue: unknown, - srcValue: unknown, - key: string | number | symbol, - object: Record, - ) => { - // Handle undefined as deletion - if (srcValue === undefined && object && typeof key === 'string') { - delete object[key] - return undefined - } - // For arrays, always replace with the provided array - // This puts the responsibility on the caller to compute the desired final state - if (Array.isArray(srcValue)) { - return srcValue - } - // For non-arrays, let lodash handle the default merge behavior - return undefined + const updatedSettings = mergeWith( + existingSettings || {}, + settings, + ( + _objValue: unknown, + srcValue: unknown, + key: string | number | symbol, + object: Record, + ) => { + // Handle undefined as deletion + if (srcValue === undefined && object && typeof key === 'string') { + delete object[key] + return undefined + } + // For arrays, always replace with the provided array + // This puts the responsibility on the caller to compute the desired final state + if (Array.isArray(srcValue)) { + return srcValue + } + // For non-arrays, let lodash handle the default merge behavior + return undefined + }, + ) + + writeFileSyncAndFlush_DEPRECATED( + targetPath, + jsonStringify(updatedSettings, null, 2) + '\n', + ) + markInternalWrite(filePath) + resetSettingsCache() + return null }, ) - - // Mark this as an internal write before writing the file - markInternalWrite(filePath) - - writeFileSyncAndFlush_DEPRECATED( - filePath, - jsonStringify(updatedSettings, null, 2) + '\n', - ) - - // Invalidate the session cache since settings have been updated - resetSettingsCache() + if (validationError) return { error: validationError } if (source === 'localSettings') { // Okay to add to gitignore async without awaiting @@ -521,9 +515,7 @@ export function updateSettingsForSource( ) } } catch (e) { - const error = new Error( - `Failed to read raw settings from ${filePath}: ${e}`, - ) + const error = new Error(`Failed to update settings at ${filePath}: ${e}`) logError(error) return { error } } diff --git a/src/utils/settings/settingsFileTransaction.ts b/src/utils/settings/settingsFileTransaction.ts new file mode 100644 index 0000000000..4020639325 --- /dev/null +++ b/src/utils/settings/settingsFileTransaction.ts @@ -0,0 +1,116 @@ +import { dirname, resolve } from 'node:path' +import { logForDebugging } from '../debug.js' +import { getErrnoCode } from '../errors.js' +import { writeFileSyncAndFlush_DEPRECATED } from '../file.js' +import { + getFsImplementation, + resolveDeepestExistingAncestorSync, +} from '../fsOperations.js' +import * as lockfile from '../lockfile.js' +import { markInternalWrite } from './internalWrites.js' +import { resetSettingsCache } from './settingsCache.js' + +const SETTINGS_LOCK_RETRY_MS = 25 +const SETTINGS_LOCK_CONTENTION_LOG_MS = 100 +const SETTINGS_LOCK_WAIT_MS = 2_000 +const SETTINGS_LOCK_STALE_MS = 30_000 +const SETTINGS_LOCK_UPDATE_MS = 5_000 +const waitBuffer = new Int32Array(new SharedArrayBuffer(4)) + +function resolveSettingsMutationTarget(requestedPath: string): string { + const fs = getFsImplementation() + const absolutePath = resolve(requestedPath) + try { + return fs.realpathSync(absolutePath) + } catch (error) { + if (getErrnoCode(error) !== 'ENOENT') throw error + return ( + resolveDeepestExistingAncestorSync(fs, absolutePath) ?? absolutePath + ) + } +} + +function acquireSettingsLock(targetPath: string): () => void { + const startedAt = performance.now() + const deadline = startedAt + SETTINGS_LOCK_WAIT_MS + let reportedContention = false + while (true) { + try { + return lockfile.lockSync(targetPath, { + lockfilePath: `${targetPath}.lock`, + realpath: false, + stale: SETTINGS_LOCK_STALE_MS, + update: SETTINGS_LOCK_UPDATE_MS, + onCompromised: error => { + // The compromise callback runs from an asynchronous heartbeat and + // cannot interrupt this helper's synchronous operation. Throwing here + // would become an unhandled exception, so log deliberately; a local + // filesystem operation exceeding the stale window is out of contract. + logForDebugging(`Settings file lock compromised: ${error}`, { + level: 'error', + }) + }, + }) + } catch (error) { + if (getErrnoCode(error) !== 'ELOCKED') throw error + const now = performance.now() + const elapsed = now - startedAt + if ( + !reportedContention && + elapsed >= SETTINGS_LOCK_CONTENTION_LOG_MS + ) { + reportedContention = true + logForDebugging( + `Settings file lock contention has lasted ${Math.round(elapsed)}ms`, + { level: 'warn' }, + ) + } + const remaining = deadline - now + if (remaining <= 0) { + throw Object.assign( + new Error( + `Timed out after ${SETTINGS_LOCK_WAIT_MS}ms waiting for the settings file lock`, + ), + { code: 'ELOCKED' }, + ) + } + Atomics.wait( + waitBuffer, + 0, + 0, + Math.min(SETTINGS_LOCK_RETRY_MS, remaining), + ) + } + } +} + +/** + * Run one synchronous settings-file operation under its physical-target lock. + * Calls for the same target must not be nested; contention remains bounded by + * the normal acquisition deadline. + */ +export function withSettingsFileTransactionSync( + requestedPath: string, + operation: (targetPath: string) => T, +): T { + const targetPath = resolveSettingsMutationTarget(requestedPath) + getFsImplementation().mkdirSync(dirname(targetPath)) + const release = acquireSettingsLock(targetPath) + try { + return operation(targetPath) + } finally { + release() + } +} + +/** Replace a complete settings document using the shared transaction identity. */ +export function replaceSettingsFileSync( + requestedPath: string, + content: string, +): void { + withSettingsFileTransactionSync(requestedPath, targetPath => { + writeFileSyncAndFlush_DEPRECATED(targetPath, content) + markInternalWrite(requestedPath) + resetSettingsCache() + }) +}