fix(config): extend the Windows atomic-write rename-retry budget

components/OptionsWatcher.ts

@@ -9,6 +9,7 @@ import { DEFAULT_CONFIG } from './DEFAULT_CONFIG.ts';
 import { cloneDeep } from 'lodash';
 import { POLLING_FALLBACK_OPTIONS, isWatcherExhaustionError, warnWatcherFallback } from '../utility/watcherFallback.ts';
 import { overlayRootEnvConfig, isRootConfigFilename } from '../config/harperConfigEnvVars.ts';
+import { readConfigFileSync } from '../config/readConfigFileSync.ts';
 
 export interface Config {
 	[key: string]: ConfigValue;
@@ -88,6 +89,7 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 	#scopedConfig?: ConfigValue;
 	#rootConfig?: Config;
 	#isRootConfig: boolean;
+	#synchronousRead: boolean;
 	#name: string;
 	#logger: Logger;
 	#usingPolling: boolean;
@@ -103,7 +105,9 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 		// Root-config watchers must see runtime env config (HARPER_SET_CONFIG et al.)
 		// even when it hasn't been flushed to disk yet — see #handleChange (#1618).
 		// Application scopes watch their own config.yaml and are never overlaid.
-		this.#isRootConfig = isRootConfig ?? isRootConfigFilename(filePath);
+		const rootConfigFile = isRootConfigFilename(filePath);
+		this.#isRootConfig = isRootConfig ?? rootConfigFile;
+		this.#synchronousRead = this.#isRootConfig || rootConfigFile;
 		this.#logger = logger || loggerWithTag(name);
 		this.#usingPolling = false;
 		this.#closed = false;
@@ -126,71 +130,91 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 	}
 
 	#handleChange() {
-		const read: Promise<void> = readFile(this.#filePath, 'utf-8')
-			.then((contents) => {
-				let parsed = yaml.parse(contents);
-				// The on-disk root config is not guaranteed to include runtime env config at
-				// boot: the file flush races component loading, so a scope's boot-time reads
-				// (e.g. an `enabled` gate in handleApplication) could observe pre-env values
-				// the componentLoader itself never saw. Ask the config layer to overlay env
-				// config onto EVERY root-config read so scope.options matches the resolved
-				// view (#1618). Non-root scopes and the no-env-vars case are untouched
-				// (overlayRootEnvConfig is a no-op there).
-				if (this.#isRootConfig) parsed = overlayRootEnvConfig(parsed);
-				this.#rootConfig = parsed && typeof parsed === 'object' ? parsed : undefined;
-				// If the extension is in the config file
-				if (this.#rootConfig && this.#name in this.#rootConfig) {
-					// If a config object does not exist
-					if (!this.#scopedConfig) {
-						// set it
-						this.#scopedConfig = this.#rootConfig[this.#name];
-						// and emit a ready event
-						this.emit('ready', this.#scopedConfig);
-					} else {
-						// Otherwise, merge the new config with the old config
-						this.#merge(this.#rootConfig[this.#name], this.#scopedConfig);
-					}
-				} else {
-					// Otherwise, if the extension is not in the config file
-					// This means the plugin was removed from the config file
-					if (this.#scopedConfig) {
-						// and a config exists, remove it
-						this.#scopedConfig = undefined;
-						this.emit('remove');
-					}
-					// Otherwise do nothing - the user may add the config back in later
-				}
-			})
-			.catch((error) => {
-				// If the config file does not exist
-				if (error instanceof Error && 'code' in error && error.code === 'ENOENT') {
-					// A readFile ENOENT here is the install window (file not written yet) or a
-					// transient read race — NOT a real deletion, which chokidar routes to
-					// `#handleUnlink`. Env config is file-independent, so when it provides this
-					// scope the missing file must not discard it (#1618). When it does not, fall
-					// through to the original ENOENT handling with #rootConfig untouched, so a
-					// first boot still emits `ready` (not `remove`, which nothing consumes at
-					// boot → `ready` would hang forever).
-					if (this.#applyEnvOnlyConfig()) return;
-					// And a config already exists, reset it to the default
-					if (this.#rootConfig) {
-						this.#resetConfig();
-						this.emit('remove');
-					} else {
-						// Otherwise, if no config exists, then just set to default and emit ready
-						this.#resetConfig();
-						this.emit('ready');
-					}
-					return;
-				}
+		if (this.#synchronousRead) {
+			let contents: string;
+			try {
+				contents = readConfigFileSync(this.#filePath);
+			} catch (error) {
+				this.#handleReadError(error);
+				return;
+			}
+			try {
+				this.#applyContents(contents);
+			} catch (error) {
 				this.emit('error', error);
-			})
+			}
+			return;
+		}
+
+		const read: Promise<void> = readFile(this.#filePath, 'utf-8')
+			.then((contents) => this.#applyContents(contents))
+			.catch((error) => this.#handleReadError(error))
 			.finally(() => {
 				this.#pendingReads.delete(read);
 			});
 		this.#pendingReads.add(read);
 	}
 
+	#applyContents(contents: string) {
+		let parsed = yaml.parse(contents);
+		// The on-disk root config is not guaranteed to include runtime env config at
+		// boot: the file flush races component loading, so a scope's boot-time reads
+		// (e.g. an `enabled` gate in handleApplication) could observe pre-env values
+		// the componentLoader itself never saw. Ask the config layer to overlay env
+		// config onto EVERY root-config read so scope.options matches the resolved
+		// view (#1618). Non-root scopes and the no-env-vars case are untouched
+		// (overlayRootEnvConfig is a no-op there).
+		if (this.#isRootConfig) parsed = overlayRootEnvConfig(parsed);
+		this.#rootConfig = parsed && typeof parsed === 'object' ? parsed : undefined;
+		// If the extension is in the config file
+		if (this.#rootConfig && this.#name in this.#rootConfig) {
+			// If a config object does not exist
+			if (!this.#scopedConfig) {
+				// set it
+				this.#scopedConfig = this.#rootConfig[this.#name];
+				// and emit a ready event
+				this.emit('ready', this.#scopedConfig);
+			} else {
+				// Otherwise, merge the new config with the old config
+				this.#merge(this.#rootConfig[this.#name], this.#scopedConfig);
+			}
+		} else {
+			// Otherwise, if the extension is not in the config file
+			// This means the plugin was removed from the config file
+			if (this.#scopedConfig) {
+				// and a config exists, remove it
+				this.#scopedConfig = undefined;
+				this.emit('remove');
+			}
+			// Otherwise do nothing - the user may add the config back in later
+		}
+	}
+
+	#handleReadError(error: unknown) {
+		// If the config file does not exist
+		if (error instanceof Error && 'code' in error && error.code === 'ENOENT') {
+			// A readFile ENOENT here is the install window (file not written yet) or a
+			// transient read race — NOT a real deletion, which chokidar routes to
+			// `#handleUnlink`. Env config is file-independent, so when it provides this
+			// scope the missing file must not discard it (#1618). When it does not, fall
+			// through to the original ENOENT handling with #rootConfig untouched, so a
+			// first boot still emits `ready` (not `remove`, which nothing consumes at
+			// boot → `ready` would hang forever).
+			if (this.#applyEnvOnlyConfig()) return;
+			// And a config already exists, reset it to the default
+			if (this.#rootConfig) {
+				this.#resetConfig();
+				this.emit('remove');
+			} else {
+				// Otherwise, if no config exists, then just set to default and emit ready
+				this.#resetConfig();
+				this.emit('ready');
+			}
+			return;
+		}
+		this.emit('error', error);
+	}
+
 	#handleError(error: unknown) {
 		if (isWatcherExhaustionError(error)) {
 			// Swallow every exhaustion error — chokidar can emit several before the
@@ -404,6 +428,11 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 		return this.#openCount;
 	}
 
+	// Test-only: process the current file contents without waiting for a watcher event.
+	_refreshForTests(): void {
+		this.#handleChange();
+	}
+
 	/**
 	 * Closes the underlying file watcher and drains any pending config-file reads.
 	 * Emits `close` synchronously, removes all listeners, then returns a Promise that

config/RootConfigWatcher.ts

@@ -1,6 +1,6 @@
 import chokidar, { FSWatcher } from 'chokidar';
-import { readFile } from 'node:fs/promises';
 import { getConfigFilePath } from './configUtils.ts';
+import { readConfigFileSync } from './readConfigFileSync.ts';
 import { EventEmitter, once } from 'node:events';
 import { parse } from 'yaml';
 import { POLLING_FALLBACK_OPTIONS, isWatcherExhaustionError, warnWatcherFallback } from '../utility/watcherFallback.ts';
@@ -79,23 +79,25 @@ export class RootConfigWatcher extends EventEmitter {
 	}
 
 	handleChange() {
-		readFile(this.#configFilePath, 'utf-8')
-			.then((data) => {
-				if (!data) return;
+		let data: string;
+		try {
+			data = readConfigFileSync(this.#configFilePath);
+		} catch {
+			return;
+		}
+		if (!data) return;
 
-				const config = parse(data);
+		try {
+			const config = parse(data);
 
-				if (!this.#config) {
-					this.#config = config;
-					this.emit('ready', this.#config);
-					return;
-				}
+			if (!this.#config) {
+				this.#config = config;
+				this.emit('ready', this.#config);
+				return;
+			}
 
-				this.emit('change', (this.#config = config));
-			})
-			.catch((_error) => {
-				// if yaml parse error ignore?
-			});
+			this.emit('change', (this.#config = config));
+		} catch {}
 	}
 
 	close() {

config/configUtils.ts

@@ -85,57 +85,76 @@ export function getConfigPath(param: string) {
 	return path.resolve(rootPath, value);
 }
 
-// Write atomically via temp file + rename so readers don't observe a truncated/empty file.
-// Temp path includes randomness so two worker threads in the same process (same pid) writing
-// in the same millisecond can't collide on the temp name and then race the rename.
-//
+// Write atomically via a randomized temp file + rename so readers do not observe partial content
+// and concurrent workers, which share process.pid, do not collide on a temp path.
 // Windows has no POSIX-style "replace an open file" semantics: rename() fails with
-// EPERM/EACCES if another thread/process has the destination momentarily open for read.
-// Every worker thread runs its own RootConfigWatcher (chokidar), so a write on one thread
-// routinely races a hot-reload read on another; Windows Defender / AV real-time scanning can
-// hold a similar transient handle. Retry with exponential backoff to ride out the race -
-// callers are synchronous, so the wait is a synchronous sleep rather than an async one.
-// The budget must outlast a single AV real-time scan pass (seconds, not hundreds of ms):
-// the previous ~910ms budget was exhausted twice in a row by the same test on a CI runner
-// (harper#2036), so the worst case is now ~3.6s.
-const RENAME_RETRY_MAX_ATTEMPTS = 12;
+// EPERM/EACCES/EBUSY while another worker or AV holds the destination open. Root config watchers use
+// readConfigFileSync so this blocking retry cannot wait on a read owned by its own worker.
+const RENAME_RETRY_BUDGET_MS = process.platform === 'win32' ? 10_000 : 3_630;
+const RENAME_RETRY_MAX_ATTEMPTS = 25;
 const RENAME_RETRY_INITIAL_DELAY_MS = 10;
 const RENAME_RETRY_MAX_DELAY_MS = 500;
-// Never notified; exists only so Atomics.wait can time out (a synchronous, CPU-idle sleep).
+// Never notified; Atomics.wait uses this only as a CPU-idle synchronous sleep.
 const renameRetrySleepBuffer = new Int32Array(new SharedArrayBuffer(4));
 
+type AtomicWriteOptions = {
+	retryBudgetMs?: number;
+	maxRetries?: number;
+	initialDelayMs?: number;
+	maxDelayMs?: number;
+};
+
 export function atomicWriteFile(
 	filePath,
 	content,
 	{
+		retryBudgetMs = RENAME_RETRY_BUDGET_MS,
 		maxRetries = RENAME_RETRY_MAX_ATTEMPTS,
 		initialDelayMs = RENAME_RETRY_INITIAL_DELAY_MS,
 		maxDelayMs = RENAME_RETRY_MAX_DELAY_MS,
-	} = {}
+	}: AtomicWriteOptions = {}
 ) {
+	const invalidOption =
+		!Number.isFinite(retryBudgetMs) ||
+		retryBudgetMs < 0 ||
+		(!Number.isFinite(maxRetries) && maxRetries !== Infinity) ||
+		maxRetries < 0 ||
+		!Number.isFinite(initialDelayMs) ||
+		initialDelayMs < 0 ||
+		!Number.isFinite(maxDelayMs) ||
+		maxDelayMs < 0;
+	if (invalidOption) {
+		throw new RangeError('rename retry options must be non-negative numbers');
+	}
 	const tempPath = `${filePath}.${process.pid}.${threadId}.${randomBytes(4).toString('hex')}.tmp`;
 	fs.writeFileSync(tempPath, content);
 	let retries = maxRetries;
 	let delayMs = initialDelayMs;
+	let retryDeadline;
+	let finalAttempt = false;
 	while (true) {
 		try {
 			fs.renameSync(tempPath, filePath);
 			break;
 		} catch (err) {
-			if (retries > 0 && (err.code === 'EPERM' || err.code === 'EACCES')) {
+			if (!finalAttempt && retries > 0 && (err.code === 'EPERM' || err.code === 'EACCES' || err.code === 'EBUSY')) {
 				retries--;
-				// Sleep synchronously (all call sites are sync) to allow the holder to close the
-				// file. Atomics.wait yields the thread to the OS instead of spinning the CPU,
-				// which is what makes a multi-second worst-case budget affordable.
-				if (delayMs > 0) Atomics.wait(renameRetrySleepBuffer, 0, 0, delayMs);
-				delayMs = Math.min(delayMs * 2, maxDelayMs);
-				continue;
+				if (retryDeadline === undefined) {
+					retryDeadline = performance.now() + retryBudgetMs;
+				}
+				const remainingBudgetMs = retryDeadline - performance.now();
+				if (remainingBudgetMs > 0) {
+					const sleepMs = Math.min(delayMs, remainingBudgetMs);
+					finalAttempt = sleepMs === remainingBudgetMs;
+					if (sleepMs > 0) Atomics.wait(renameRetrySleepBuffer, 0, 0, sleepMs);
+					delayMs = Math.min(Math.max(delayMs * 2, RENAME_RETRY_INITIAL_DELAY_MS), maxDelayMs);
+					continue;
+				}
 			}
-			// if it fails we should clean up the tmp file
 			try {
 				fs.unlinkSync(tempPath);
 			} catch {
-				// ignore cleanup errors
+				// Ignore cleanup errors and preserve the original rename failure.
 			}
 			throw err;
 		}

config/readConfigFileSync.ts

@@ -0,0 +1,35 @@
+import { readFileSync } from 'node:fs';
+
+const READ_RETRY_BUDGET_MS = 500;
+const READ_RETRY_INITIAL_DELAY_MS = 10;
+const READ_RETRY_MAX_DELAY_MS = 100;
+const readRetrySleepBuffer = new Int32Array(new SharedArrayBuffer(4));
+
+export function readConfigFileSync(filePath: string): string {
+	let delayMs = READ_RETRY_INITIAL_DELAY_MS;
+	let retryDeadline;
+	let finalAttempt = false;
+	while (true) {
+		try {
+			return readFileSync(filePath, 'utf-8');
+		} catch (error) {
+			if (
+				!finalAttempt &&
+				error instanceof Error &&
+				'code' in error &&
+				(error.code === 'EPERM' || error.code === 'EACCES' || error.code === 'EBUSY')
+			) {
+				retryDeadline ??= performance.now() + READ_RETRY_BUDGET_MS;
+				const remainingBudgetMs = retryDeadline - performance.now();
+				if (remainingBudgetMs > 0) {
+					const sleepMs = Math.min(delayMs, remainingBudgetMs);
+					finalAttempt = sleepMs === remainingBudgetMs;
+					Atomics.wait(readRetrySleepBuffer, 0, 0, sleepMs);
+					delayMs = Math.min(delayMs * 2, READ_RETRY_MAX_DELAY_MS);
+					continue;
+				}
+			}
+			throw error;
+		}
+	}
+}