Retry Codex trust config writes on Windows locks (#4710)
This commit is contained in:
parent
c1d2d1c7fd
commit
f0ea7a39a7
|
|
@ -1,8 +1,12 @@
|
|||
import { describe, expect, it } from 'vitest'
|
||||
import { existsSync, mkdtempSync, readFileSync, rmSync, statSync } from 'node:fs'
|
||||
import { existsSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { writeFileAtomically } from './fs-utils'
|
||||
import {
|
||||
copyFileWithWindowsRetry,
|
||||
renameFileWithWindowsRetry,
|
||||
writeFileAtomically
|
||||
} from './fs-utils'
|
||||
|
||||
describe('writeFileAtomically', () => {
|
||||
let dir: string
|
||||
|
|
@ -78,3 +82,50 @@ describe('writeFileAtomically', () => {
|
|||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('retrying file operations', () => {
|
||||
let dir: string
|
||||
|
||||
function setup(): string {
|
||||
dir = mkdtempSync(join(tmpdir(), 'orca-fs-utils-'))
|
||||
return dir
|
||||
}
|
||||
|
||||
function cleanup(): void {
|
||||
if (dir) {
|
||||
rmSync(dir, { recursive: true, force: true })
|
||||
}
|
||||
}
|
||||
|
||||
it('renames a file through the retry wrapper', () => {
|
||||
setup()
|
||||
try {
|
||||
const source = join(dir, 'source.txt')
|
||||
const target = join(dir, 'target.txt')
|
||||
writeFileSync(source, 'data', 'utf-8')
|
||||
|
||||
renameFileWithWindowsRetry(source, target)
|
||||
|
||||
expect(existsSync(source)).toBe(false)
|
||||
expect(readFileSync(target, 'utf-8')).toBe('data')
|
||||
} finally {
|
||||
cleanup()
|
||||
}
|
||||
})
|
||||
|
||||
it('copies a file through the retry wrapper', () => {
|
||||
setup()
|
||||
try {
|
||||
const source = join(dir, 'source.txt')
|
||||
const target = join(dir, 'target.txt')
|
||||
writeFileSync(source, 'data', 'utf-8')
|
||||
|
||||
copyFileWithWindowsRetry(source, target)
|
||||
|
||||
expect(readFileSync(source, 'utf-8')).toBe('data')
|
||||
expect(readFileSync(target, 'utf-8')).toBe('data')
|
||||
} finally {
|
||||
cleanup()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1,5 +1,5 @@
|
|||
import { randomUUID } from 'node:crypto'
|
||||
import { renameSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { copyFileSync, renameSync, rmSync, writeFileSync } from 'node:fs'
|
||||
import { dirname } from 'node:path'
|
||||
import { grantDirAcl, isPermissionError } from '../win32-utils'
|
||||
|
||||
|
|
@ -11,7 +11,7 @@ export function writeFileAtomically(
|
|||
const tmpPath = `${targetPath}.${process.pid}.${randomUUID()}.tmp`
|
||||
try {
|
||||
writeFileSync(tmpPath, contents, { encoding: 'utf-8', mode: options?.mode })
|
||||
renameWithRetry(tmpPath, targetPath)
|
||||
renameFileWithWindowsRetry(tmpPath, targetPath)
|
||||
} catch (error) {
|
||||
rmSync(tmpPath, { force: true })
|
||||
// Why: on Windows, Chromium's renderer initialization calls
|
||||
|
|
@ -26,7 +26,7 @@ export function writeFileAtomically(
|
|||
const retryTmpPath = `${targetPath}.${process.pid}.${randomUUID()}.tmp`
|
||||
try {
|
||||
writeFileSync(retryTmpPath, contents, { encoding: 'utf-8', mode: options?.mode })
|
||||
renameWithRetry(retryTmpPath, targetPath)
|
||||
renameFileWithWindowsRetry(retryTmpPath, targetPath)
|
||||
return
|
||||
} catch {
|
||||
rmSync(retryTmpPath, { force: true })
|
||||
|
|
@ -39,16 +39,24 @@ export function writeFileAtomically(
|
|||
}
|
||||
}
|
||||
|
||||
// Why: on Windows, renameSync can fail with EPERM/EACCES/EBUSY if another
|
||||
// process (antivirus, Claude CLI, Codex CLI) holds the target file open.
|
||||
// A short retry avoids transient failures without masking real permission
|
||||
// errors. Total backoff (~750ms) covers typical AV scan windows seen in
|
||||
// issue #1507.
|
||||
function renameWithRetry(source: string, target: string): void {
|
||||
// Why: on Windows, file replacement and backup-copy operations can fail with
|
||||
// EPERM/EACCES/EBUSY if another process (antivirus, Claude CLI, Codex CLI)
|
||||
// holds the target file open. A short retry avoids transient failures without
|
||||
// masking real permission errors. Total backoff (~750ms) covers typical AV
|
||||
// scan windows seen in issue #1507.
|
||||
export function renameFileWithWindowsRetry(source: string, target: string): void {
|
||||
runFileOperationWithWindowsRetry(() => renameSync(source, target))
|
||||
}
|
||||
|
||||
export function copyFileWithWindowsRetry(source: string, target: string): void {
|
||||
runFileOperationWithWindowsRetry(() => copyFileSync(source, target))
|
||||
}
|
||||
|
||||
function runFileOperationWithWindowsRetry(operation: () => void): void {
|
||||
const maxAttempts = process.platform === 'win32' ? 6 : 1
|
||||
for (let attempt = 1; attempt <= maxAttempts; attempt++) {
|
||||
try {
|
||||
renameSync(source, target)
|
||||
operation()
|
||||
return
|
||||
} catch (error) {
|
||||
const code = (error as NodeJS.ErrnoException).code
|
||||
|
|
|
|||
|
|
@ -1,17 +1,9 @@
|
|||
/* eslint-disable max-lines -- Why: Codex hook trust parsing, hashing, and byte-preserving TOML edits share one fragile file-format contract; splitting would make the compatibility shim harder to audit. */
|
||||
import {
|
||||
copyFileSync,
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
readFileSync,
|
||||
renameSync,
|
||||
realpathSync,
|
||||
unlinkSync,
|
||||
writeFileSync
|
||||
} from 'fs'
|
||||
import { existsSync, mkdirSync, readFileSync, realpathSync, unlinkSync, writeFileSync } from 'fs'
|
||||
import { dirname, join } from 'path'
|
||||
import { createHash, randomUUID } from 'crypto'
|
||||
import { escapeRegex } from '../../shared/string-utils'
|
||||
import { copyFileWithWindowsRetry, renameFileWithWindowsRetry } from '../codex-accounts/fs-utils'
|
||||
|
||||
// Why: Codex 0.129+ gates each hook on a `trusted_hash` entry in
|
||||
// ~/.codex/config.toml under [hooks.state."<key>"]. Without it the hook is in
|
||||
|
|
@ -576,9 +568,9 @@ export function writeConfigAtomically(configPath: string, contents: string): voi
|
|||
try {
|
||||
writeFileSync(tmpPath, contents, 'utf-8')
|
||||
if (existsSync(configPath)) {
|
||||
copyFileSync(configPath, `${configPath}.bak`)
|
||||
copyFileWithWindowsRetry(configPath, `${configPath}.bak`)
|
||||
}
|
||||
renameSync(tmpPath, configPath)
|
||||
renameFileWithWindowsRetry(tmpPath, configPath)
|
||||
renamed = true
|
||||
} finally {
|
||||
if (!renamed && existsSync(tmpPath)) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue