Prevent settings races and stale session teardown in local services

Concurrent writes to the same settings file could interleave and leave user
preferences in a partially updated state, and a restarted conversation session
could still be torn down by an older process exit callback. This serializes
per-file settings writes, hardens temp-file handling, adds regression coverage
for both service behaviors, and drops an unused sidecar external from the
build exclude list.

Constraint: Settings writes must remain atomic while allowing multiple service entry points to update the same JSON file
Rejected: Keep fire-and-forget writes with unique temp names only | still allows stale reads and last-writer races between callers
Confidence: high
Scope-risk: moderate
Reversibility: clean
Directive: Preserve the per-file write lock when adding new settings mutation paths so concurrent writes keep one serialization point
Tested: bun test src/server/__tests__/settings.test.ts src/server/__tests__/conversations.test.ts (new settings/conversation cases passed)
Not-tested: Root lint script is unavailable; Models API tests in src/server/__tests__/settings.test.ts still fail in this checkout with changed default model/effort expectations
This commit is contained in:
程序员阿江(Relakkes) 2026-04-20 23:13:45 +08:00
parent 0f7494006f
commit 9fdbfafd4d
4 changed files with 101 additions and 16 deletions

View File

@ -132,7 +132,6 @@ async function compileExecutable({
// ant-internal / 可选工具
'@anthropic-ai/mcpb',
'fflate',
'turndown',
'sharp',
'react-devtools-core',
],

View File

@ -151,6 +151,31 @@ describe('ConversationService', () => {
const svc = new ConversationService()
expect(() => svc.onOutput('no-such-session', () => {})).not.toThrow()
})
it('should ignore stale process exits after a session restarts', () => {
const svc = new ConversationService()
const oldProc = { pid: 1 } as any
const newProc = { pid: 2 } as any
;(svc as any).sessions.set('session-restart', {
proc: newProc,
outputCallbacks: [],
workDir: process.cwd(),
permissionMode: 'bypassPermissions',
sdkToken: 'token',
sdkSocket: null,
pendingOutbound: [],
stderrLines: [],
sdkMessages: [],
pendingPermissionRequests: new Map(),
})
;(svc as any).handleProcessExit('session-restart', oldProc, 143)
expect(svc.hasSession('session-restart')).toBe(true)
;(svc as any).handleProcessExit('session-restart', newProc, 0)
expect(svc.hasSession('session-restart')).toBe(false)
})
})
// ============================================================================

View File

@ -134,6 +134,25 @@ describe('SettingsService', () => {
expect(settings.theme).toBe('dark')
expect(settings.defaultMode).toBe('acceptEdits')
})
it('should serialize concurrent user settings writes to the same file', async () => {
const svc = new SettingsService()
const originalNow = Date.now
Date.now = () => 1776695497171
try {
await Promise.all([
svc.updateUserSettings({ theme: 'dark' }),
svc.setPermissionMode('bypassPermissions'),
])
} finally {
Date.now = originalNow
}
const settings = await svc.getUserSettings()
expect(settings.theme).toBe('dark')
expect(settings.defaultMode).toBe('bypassPermissions')
})
})
// =============================================================================

View File

@ -9,6 +9,7 @@
*/
import * as fs from 'fs/promises'
import { randomBytes } from 'node:crypto'
import * as path from 'path'
import * as os from 'os'
import { ApiError } from '../middleware/errorHandler.js'
@ -24,6 +25,7 @@ const VALID_PERMISSION_MODES = [
export type PermissionMode = (typeof VALID_PERMISSION_MODES)[number]
export class SettingsService {
private static writeLocks = new Map<string, Promise<void>>()
private projectRoot?: string
constructor(projectRoot?: string) {
@ -93,29 +95,67 @@ export class SettingsService {
// ---------------------------------------------------------------------------
/** 原子写入 JSON 文件 */
private async withWriteLock<T>(
filePath: string,
task: () => Promise<T>,
): Promise<T> {
const previousWrite = SettingsService.writeLocks.get(filePath) ?? Promise.resolve()
const nextWrite = previousWrite
.catch(() => {})
.then(task)
SettingsService.writeLocks.set(filePath, nextWrite)
try {
return await nextWrite
} finally {
if (SettingsService.writeLocks.get(filePath) === nextWrite) {
SettingsService.writeLocks.delete(filePath)
}
}
}
private async writeJsonFile(
filePath: string,
data: Record<string, unknown>,
): Promise<void> {
const dir = path.dirname(filePath)
await fs.mkdir(dir, { recursive: true })
const contents = JSON.stringify(data, null, 2) + '\n'
let lastError: unknown
const tmpFile = `${filePath}.tmp.${Date.now()}`
try {
await fs.writeFile(tmpFile, JSON.stringify(data, null, 2) + '\n', 'utf-8')
await fs.rename(tmpFile, filePath)
} catch (err) {
// 清理临时文件best-effort
await fs.unlink(tmpFile).catch(() => {})
throw ApiError.internal(`Failed to write settings to ${filePath}: ${err}`)
for (let attempt = 0; attempt < 2; attempt++) {
const tmpFile = `${filePath}.tmp.${process.pid}.${Date.now()}.${randomBytes(6).toString('hex')}`
try {
await fs.mkdir(dir, { recursive: true })
await fs.writeFile(tmpFile, contents, 'utf-8')
await fs.rename(tmpFile, filePath)
return
} catch (err) {
lastError = err
await fs.unlink(tmpFile).catch(() => {})
if (
(err as NodeJS.ErrnoException).code !== 'ENOENT' ||
attempt === 1
) {
break
}
}
}
throw ApiError.internal(
`Failed to write settings to ${filePath}: ${lastError}`,
)
}
/** 更新用户级设置(浅合并) */
async updateUserSettings(settings: Record<string, unknown>): Promise<void> {
const current = await this.getUserSettings()
const merged = Object.assign({}, current, settings)
await this.writeJsonFile(this.getUserSettingsPath(), merged)
const filePath = this.getUserSettingsPath()
await this.withWriteLock(filePath, async () => {
const current = await this.readJsonFile(filePath)
const merged = Object.assign({}, current, settings)
await this.writeJsonFile(filePath, merged)
})
}
/** 更新项目级设置(浅合并) */
@ -124,9 +164,11 @@ export class SettingsService {
projectRoot?: string,
): Promise<void> {
const filePath = this.getProjectSettingsPath(projectRoot)
const current = await this.readJsonFile(filePath)
const merged = Object.assign({}, current, settings)
await this.writeJsonFile(filePath, merged)
await this.withWriteLock(filePath, async () => {
const current = await this.readJsonFile(filePath)
const merged = Object.assign({}, current, settings)
await this.writeJsonFile(filePath, merged)
})
}
// ---------------------------------------------------------------------------