fix: use async settings read with timeout in preSpawn path#100
fix: use async settings read with timeout in preSpawn path#100Spirotot wants to merge 1 commit intohappier-dev:devfrom
Conversation
readClaudeSettings() uses synchronous readFileSync, which blocks the entire preSpawn startup task chain. On macOS, if settings.json is a symlink to iCloud Drive (or any network/cloud filesystem), the openat() syscall can block indefinitely when the process lacks TCC entitlements — a common situation for LaunchAgent-spawned daemon processes. Add readClaudeSettingsAsync() using fs/promises.readFile with a 5-second AbortSignal.timeout(). Convert generateHookSettingsFile() and its caller in startHookServerTask to async. The sync version is preserved for backward compatibility with non-startup callers.
WalkthroughThree files across Claude settings utilities are updated to convert synchronous file I/O operations to asynchronous patterns. A new async function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/backends/claude/utils/claudeSettings.ts (1)
78-81: Consider ifexistsSynccan also block on slow filesystems.The synchronous
existsSynccall could theoretically also hang on the same slow/inaccessible filesystem scenario this PR aims to fix. However, existence checks viastatare typically much faster thanopen+read, so this is likely acceptable in practice.If you want full consistency, you could use
fs/promises.access()with a timeout, but the current approach is pragmatic given the lower risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/cli/src/backends/claude/utils/claudeSettings.ts` around lines 78 - 81, The synchronous existsSync check can hang on slow filesystems; replace the sync check with an async fs.promises.access call wrapped in a timeout (e.g., Promise.race between access(settingsPath) and a timeout rejection) so the code checks file existence without blocking indefinitely; update the surrounding function to be async (or await the helper) and reference the existing symbols existsSync -> use fs.promises.access, settingsPath, and the function that reads Claude settings to locate and change the logic accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/cli/src/backends/claude/utils/claudeSettings.ts`:
- Around line 78-81: The synchronous existsSync check can hang on slow
filesystems; replace the sync check with an async fs.promises.access call
wrapped in a timeout (e.g., Promise.race between access(settingsPath) and a
timeout rejection) so the code checks file existence without blocking
indefinitely; update the surrounding function to be async (or await the helper)
and reference the existing symbols existsSync -> use fs.promises.access,
settingsPath, and the function that reads Claude settings to locate and change
the logic accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cli/src/backends/claude/startup/tasks/startHookServerTask.tsapps/cli/src/backends/claude/utils/claudeSettings.tsapps/cli/src/backends/claude/utils/generateHookSettings.ts
Greptile SummaryAdds async version of Critical Issues Found:
Non-critical:
The PR correctly updates Confidence Score: 1/5
|
| Filename | Overview |
|---|---|
| apps/cli/src/backends/claude/utils/claudeSettings.ts | Added async version of settings reader with timeout; sync version remains for backward compatibility |
| apps/cli/src/backends/claude/utils/generateHookSettings.ts | Converted to async function but missing await in existing caller at runClaude.ts:513 - will cause runtime error |
| apps/cli/src/backends/claude/startup/tasks/startHookServerTask.ts | Type signature correctly updated and properly awaits the async call |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[preSpawn startup] --> B[startHookServerTask]
B --> C[generateHookSettingsFile]
C --> D{readClaudeSettingsAsync}
D -->|timeout=5s| E[AbortSignal.timeout]
E --> F[fs.promises.readFile]
F -->|success| G[merge with hooks config]
F -->|timeout| H[return null, use defaults]
F -->|error| H
G --> I[write temporary settings file]
I --> J[return settings path]
K[runClaude.ts:513<br/>MISSING AWAIT] -.->|broken| C
style K fill:#ff6b6b,stroke:#c92a2a
style D fill:#51cf66,stroke:#2f9e44
style E fill:#51cf66,stroke:#2f9e44
Last reviewed commit: d53ed81
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * @returns Path to the generated settings file | ||
| */ | ||
| export function generateHookSettingsFile(port: number, options: GenerateHookSettingsOptions = {}): string { | ||
| export async function generateHookSettingsFile(port: number, options: GenerateHookSettingsOptions = {}): Promise<string> { |
There was a problem hiding this comment.
changing this to async breaks the existing caller at apps/cli/src/backends/claude/runClaude.ts:513 which doesn't await the call
// runClaude.ts line 513 (not updated in this PR):
const hookSettingsPath = generateHookSettingsFile(hookServer.port, {needs to be:
const hookSettingsPath = await generateHookSettingsFile(hookServer.port, {| * @returns Path to the generated settings file | ||
| */ | ||
| export function generateHookSettingsFile(port: number, options: GenerateHookSettingsOptions = {}): string { | ||
| export async function generateHookSettingsFile(port: number, options: GenerateHookSettingsOptions = {}): Promise<string> { |
There was a problem hiding this comment.
tests in generateHookSettings.test.ts need updating - all 4 test cases call this function without await
lines 26, 38, 65, and 102 all need:
- test functions changed to
async - calls changed to
await generateHookSettingsFile(...)
| if (!existsSync(settingsPath)) { | ||
| logger.debug(`[ClaudeSettings] No Claude settings file found at ${settingsPath}`); | ||
| return null; |
There was a problem hiding this comment.
existsSync() is still synchronous and could block on slow filesystems (same issue as readFileSync). consider using fs.promises.access() with the abort signal instead
| if (!existsSync(settingsPath)) { | |
| logger.debug(`[ClaudeSettings] No Claude settings file found at ${settingsPath}`); | |
| return null; | |
| try { | |
| await access(settingsPath, { signal: AbortSignal.timeout(SETTINGS_READ_TIMEOUT_MS) }); | |
| } catch { | |
| logger.debug(`[ClaudeSettings] No Claude settings file found at ${settingsPath}`); | |
| return null; | |
| } |
1 participant
Summary
readClaudeSettings()uses synchronousreadFileSyncto readsettings.json, called during the preSpawn startup task chain viagenerateHookSettingsFile()settings.jsonis a symlink to iCloud Drive (or any network/cloud filesystem), theopenat()syscall can block indefinitely when the process lacks TCC entitlements — a common situation for LaunchAgent-spawned daemon processesChanges
claudeSettings.ts: AddreadClaudeSettingsAsync()usingfs/promises.readFilewithAbortSignal.timeout(5000). The syncreadClaudeSettings()is preserved for backward compatibility.generateHookSettings.ts: ConvertgenerateHookSettingsFile()to async, using the newreadClaudeSettingsAsync().startHookServerTask.ts: UpdategenerateHookSettingsFiletype from(port: number) => stringto(port: number) => Promise<string>andawaitthe call. The task runner is already async.Context
Discovered while debugging daemon-spawned sessions on macOS. The
settings.jsonfile was a symlink resolving through iCloud Drive (~/Library/Mobile Documents/...). macOS blocksopenat()from LaunchAgent-spawned processes to iCloud Drive paths without TCC entitlements, causing the synchronousreadFileSyncto hang indefinitely and block the entire session startup.Test plan
settings.jsonis a regular local filesettings.jsonis on a slow/inaccessible filesystem (times out after 5s instead of hanging)shouldIncludeCoAuthoredBy()still works (uses sync version, unaffected)Summary by CodeRabbit
Release Notes