Fix ENOENT race condition in schema generation#525
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add ensureDir helper with retry logic for directory creation - Add writeFileWithRetry helper with retry logic for file writes - Add sync delay after rmSync to ensure filesystem consistency - Add error tracking and detailed error messages - Gracefully handle errors and report them without stopping the entire build This fixes intermittent ENOENT errors in CI when writing schema files, particularly the ApiRegistry.json file. Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Replace Atomics.wait (which doesn't work on main thread) with a simple busy-wait sleepSync - Add sleepSync utility for synchronous delays in build scripts - This ensures the retry logic works correctly in all environments Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
- Add RETRY_DELAY_BASE_MS, FS_SYNC_DELAY_MS, MAX_RETRIES constants - Improve sleepSync documentation explaining why busy-wait is acceptable - Add better error messages for partial schema generation failures - Use constants consistently throughout retry logic Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses intermittent CI build failures caused by filesystem race conditions when generating JSON schemas from Zod definitions. The fix introduces retry infrastructure with delays to handle ENOENT errors that occur when directories are accessed immediately after rmSync operations.
Changes:
- Added three utility functions with retry logic:
sleepSync()for synchronous delays,ensureDir()for directory creation with retries, andwriteFileWithRetry()for file writes with automatic parent directory creation - Implemented 50ms post-rmSync delay and retry configuration (3 attempts with 100ms base delay)
- Enhanced error handling to accumulate errors and exit with detailed diagnostics instead of crashing mid-generation
| } | ||
| } catch (error) { | ||
| if (i === retries - 1) { | ||
| throw new Error(`Failed to create directory ${dirPath}: ${error}`); |
There was a problem hiding this comment.
Error information is lost in the error wrapping. When rethrowing errors, the original error object's stack trace and details are converted to a string, making debugging more difficult. Consider preserving the original error as the cause:
throw new Error(`Failed to create directory ${dirPath}`, { cause: error });This maintains the full error context while adding the descriptive message.
| return; | ||
| } catch (error) { | ||
| if (i === retries - 1) { | ||
| throw new Error(`Failed to write file ${filePath}: ${error}`); |
There was a problem hiding this comment.
Error information is lost in the error wrapping. When rethrowing errors, the original error object's stack trace and details are converted to a string, making debugging more difficult. Consider preserving the original error as the cause:
throw new Error(`Failed to write file ${filePath}`, { cause: error });This maintains the full error context while adding the descriptive message.
| try { | ||
| // Ensure the parent directory exists | ||
| const dir = path.dirname(filePath); | ||
| ensureDir(dir); |
There was a problem hiding this comment.
The ensureDir function is called recursively from within writeFileWithRetry (line 62), which means if directory creation fails and retries, it will consume retries from both the inner ensureDir call and the outer writeFileWithRetry loop. This could lead to up to 9 retry attempts (3 retries × 3 retries) for a single file write operation, which may not be the intended behavior.
Consider either:
- Passing
retries: 1toensureDirwhen calling fromwriteFileWithRetryto avoid nested retries - Restructuring to ensure directory creation happens once before the write retry loop
- Documenting that this nested retry behavior is intentional
| ensureDir(dir); | |
| ensureDir(dir, 1); |
| } | ||
| // Verify the directory was created successfully | ||
| if (fs.existsSync(dirPath) && fs.statSync(dirPath).isDirectory()) { | ||
| return; |
There was a problem hiding this comment.
The ensureDir function has a logic flaw. If the directory exists but the verification check at line 40-42 fails (returns false), the function will exit the try block without throwing an error or retrying. This means the function can complete successfully even when the directory doesn't exist or isn't valid.
The issue is that when the conditions at lines 40-41 are true, the function returns successfully. But if those conditions are false (directory doesn't exist or isn't a directory), execution continues past the if statement and exits the try block normally without any error being thrown. The loop then continues to the next iteration.
Consider restructuring the logic to explicitly throw an error when verification fails, ensuring that retry logic is triggered.
| return; | |
| return; | |
| } else { | |
| // If verification fails without throwing, treat it as an error to trigger retry logic | |
| throw new Error(`Verification failed for directory ${dirPath}`); |
| * Synchronous sleep utility using a busy-wait loop | ||
| * Only use for short delays in build scripts where blocking is acceptable | ||
| * | ||
| * Note: This blocks the event loop and consumes CPU. For production code, | ||
| * use async/await with setTimeout. For build scripts, this simple synchronous | ||
| * approach is acceptable as we need to ensure filesystem operations complete | ||
| * before proceeding. | ||
| */ | ||
| function sleepSync(ms: number): void { | ||
| const end = Date.now() + ms; | ||
| while (Date.now() < end) { | ||
| // Busy wait | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The busy-wait implementation in sleepSync will consume 100% CPU for the duration of the sleep. While the comment acknowledges this is acceptable for build scripts, there's a better alternative using Atomics.wait() that provides synchronous blocking without busy-waiting:
function sleepSync(ms: number): void {
Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
}This approach blocks the thread without consuming CPU cycles, making it more efficient even for build scripts. The current implementation could cause issues in CI environments with limited CPU resources or when running multiple builds in parallel.
| * Synchronous sleep utility using a busy-wait loop | |
| * Only use for short delays in build scripts where blocking is acceptable | |
| * | |
| * Note: This blocks the event loop and consumes CPU. For production code, | |
| * use async/await with setTimeout. For build scripts, this simple synchronous | |
| * approach is acceptable as we need to ensure filesystem operations complete | |
| * before proceeding. | |
| */ | |
| function sleepSync(ms: number): void { | |
| const end = Date.now() + ms; | |
| while (Date.now() < end) { | |
| // Busy wait | |
| } | |
| } | |
| * Synchronous sleep utility using Atomics.wait to avoid busy-waiting | |
| * Only use for short delays in build scripts where blocking is acceptable. | |
| * | |
| * Note: This blocks the event loop. For production code, prefer async/await | |
| * with setTimeout. For build scripts, this simple synchronous approach is | |
| * acceptable as we need to ensure filesystem operations complete before | |
| * proceeding. | |
| */ | |
| function sleepSync(ms: number): void { | |
| const sab = new SharedArrayBuffer(4); | |
| const int32 = new Int32Array(sab); | |
| Atomics.wait(int32, 0, 0, ms); | |
| } |
| const delay = RETRY_DELAY_BASE_MS * (i + 1); | ||
| sleepSync(delay); |
There was a problem hiding this comment.
The retry delay calculation RETRY_DELAY_BASE_MS * (i + 1) produces a linear backoff (100ms, 200ms, 300ms) rather than the exponential backoff mentioned in the PR description and code comments. For exponential backoff, the calculation should be RETRY_DELAY_BASE_MS * Math.pow(2, i) which would produce 100ms, 200ms, 400ms delays.
If linear backoff is intentional, update the comments at lines 10, 48, and 71 to say "linear backoff" instead of "exponential backoff". Otherwise, fix the calculation to implement true exponential backoff.
| const delay = RETRY_DELAY_BASE_MS * (i + 1); | ||
| sleepSync(delay); |
There was a problem hiding this comment.
The retry delay calculation RETRY_DELAY_BASE_MS * (i + 1) produces a linear backoff (100ms, 200ms, 300ms) rather than the exponential backoff mentioned in the PR description and code comments. For exponential backoff, the calculation should be RETRY_DELAY_BASE_MS * Math.pow(2, i) which would produce 100ms, 200ms, 400ms delays.
If linear backoff is intentional, update the comments at lines 10, 48, and 71 to say "linear backoff" instead of "exponential backoff". Otherwise, fix the calculation to implement true exponential backoff.
| const OUT_DIR = path.resolve(__dirname, '../json-schema'); | ||
|
|
||
| // Retry and delay configuration | ||
| const RETRY_DELAY_BASE_MS = 100; // Base delay in ms, multiplied by retry attempt number |
There was a problem hiding this comment.
The comment describes the backoff as "exponential" but the implementation at lines 48 and 71 uses linear backoff (RETRY_DELAY_BASE_MS * (i + 1)). Either update this comment to say "Base delay in ms, multiplied by retry attempt number (linear backoff)" or change the implementation to use true exponential backoff with RETRY_DELAY_BASE_MS * Math.pow(2, i).
…atch (ADR-0024) (#2334) The per-user AI-seat gate (evaluateAgentAccess → requires the `ai_seat` capability) reads `req.user.permissions`, but the dispatch ExecutionContext serving `/ai/*` never carried the seat — so a SEATED user (`sys_user.ai_access=true`) was denied: `/ai/agents` returned an EMPTY catalog (and in-UI build/ask would 403), even though the #525 cap correctly counted them. The synthesis lived only on plugin-hono-server's data-route resolveCtx (which is why the cap, on the data-write path, worked but the gate did not). Add the guarded synthesis to BOTH AI req.user construction paths: - http-dispatcher.ts — the `/ai/*` wildcard dispatch (the ACTIVE path): build req.user.permissions, then read sys_user.ai_access via the DEFAULT (current per-request env) ObjectQL service and push `ai_seat` when true. NB do NOT pass context.environmentId — in the cloud runtime that is the control-plane env UUID, which keys a DISTINCT empty per-scope engine; the env's own sys_user is served by the default service (this exact mismatch returned [] in testing). - dispatcher-plugin.ts resolveRequestUser — the concrete-route mount path (defensive; guarded read via the env kernel's `data` service). Guarded system read → can only ever ADD access, never break auth (absent/false/missing-column/error → no seat, unchanged). Live-verified on a local cloud+objectos stack (OS_CLOUD_AI_SEAT_MODE=enforce): seated ai_access=true → /ai/agents=[build,ask]; false → []; restore → [build,ask]; cap still 400s past the licensed seat count. Unblocks flipping OS_CLOUD_AI_SEAT_MODE=enforce. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
CI builds intermittently fail when writing
json-schema/api/ApiRegistry.jsondue to filesystem race conditions afterrmSync.Changes
Retry infrastructure:
ensureDir()- Create directories with 3 retries, exponential backoff (100/200/300ms)writeFileWithRetry()- Write files with automatic parent directory creation and retrysleepSync()- Busy-wait for synchronous delays (build scripts only)Filesystem synchronization:
Configuration:
All filesystem operations now use retry helpers instead of direct
fscalls. Errors accumulate and fail the build with context rather than crashing mid-generation.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.