Fix intermittent set_configuration 500s on Windows caused by the config write's rename retry

DESIGN.md

@@ -1248,3 +1248,27 @@ absolute paths built from `cwd`, so its bases must be derived from the same spel
 paths are relative to `cwd` and reads stay on the configured `component.directory`. And a watcher
 that degrades to polling stays there for its lifetime, so a caller with no polling story of its own
 (`resources/blob.ts`) needs one — there it polls `readMore` on the existing no-progress deadline.
+
+## No descriptor on the root config may outlive a turn (`config/configUtils.ts`, `config/RootConfigWatcher.ts`, `components/OptionsWatcher.ts`)
+
+`atomicWriteFile` replaces `harper-config.yaml` by rename-over and retries `EPERM`/`EACCES` with a
+synchronous `Atomics.wait`. On Windows a rename over an open destination fails, and a descriptor
+belongs to the process, not the thread — measured on `windows-latest`/Node 24: a single Node read
+descriptor on the destination blocks it, while `fs.watch` and chokidar handles do not.
+
+That makes the retry unable to outlast a holder on the _calling_ thread, because the sleep blocks
+the event loop whose turn would close it: the holder's lifetime becomes exactly the retry budget
+and every attempt fails. This is why widening the budget (#1714, #2036) never fixed the
+`set_configuration` 500s it was aimed at, and why both root-config watchers read with
+`readFileSync`. Any future `fsPromises.readFile` of this file reintroduces harper#2313 — the rule
+is unenforced by anything but this note and the comment on `atomicWriteFile`.
+
+The synchronous read then sees writers mid-write, which promise-based reads mostly skipped. A read
+that is unusable — empty, or parsing to anything but an object — is retried by `PartialReadRetry`
+(`utility/watcherFallback.ts`) rather than adopted, because chokidar may emit nothing further for
+that write. Completeness is judged on the file's own parse, _before_ `overlayRootEnvConfig`, which
+returns a non-null object whenever a config env var is set and would otherwise launder a
+half-written file into a valid-looking env-only config. Its three outcomes are distinct and each
+one matters: a usable read withdraws the file's give-up report and restores the budget; giving up
+restores the budget (the write that repairs the file can itself be read mid-write) but leaves the
+report standing, since it is shared with every other watcher of that file; closing is terminal.

components/OptionsWatcher.ts

@@ -4,10 +4,17 @@ import { EventEmitter, once } from 'events';
 import yaml from 'yaml';
 import chokidar, { type FSWatcher } from 'chokidar';
 import { readFile } from 'node:fs/promises';
+import { readFileSync } from 'node:fs';
 import { isDeepStrictEqual } from 'util';
 import { DEFAULT_CONFIG } from './DEFAULT_CONFIG.ts';
 import { cloneDeep } from 'lodash';
-import { POLLING_FALLBACK_OPTIONS, isWatcherExhaustionError, warnWatcherFallback } from '../utility/watcherFallback.ts';
+import {
+	POLLING_FALLBACK_OPTIONS,
+	PartialReadRetry,
+	isPartialReadError,
+	isWatcherExhaustionError,
+	warnWatcherFallback,
+} from '../utility/watcherFallback.ts';
 import { resolveWatchTarget } from '../utility/watchPath.ts';
 import { overlayRootEnvConfig, isRootConfigFilename } from '../config/harperConfigEnvVars.ts';
 
@@ -96,6 +103,7 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 	#closed: boolean;
 	#openCount: number = 0;
 	#pendingReads: Set<Promise<void>> = new Set();
+	#partialRead: PartialReadRetry;
 	ready: Promise<any[]>;
 
 	constructor(name: string, filePath: string, logger?: Logger, isRootConfig?: boolean) {
@@ -104,6 +112,7 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 		this.#filePath = filePath;
 		const watchTarget = resolveWatchTarget(filePath);
 		this.#watchPath = watchTarget.path;
+		this.#partialRead = new PartialReadRetry(filePath);
 		// 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.
@@ -129,72 +138,123 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 			.on('ready', this.#handleChange.bind(this));
 	}
 
+	// Root config only: see the descriptor-lifetime invariant on atomicWriteFile (DESIGN.md).
 	#handleChange() {
+		if (this.#isRootConfig) {
+			this.#applyRead(() => readFileSync(this.#filePath, 'utf-8'));
+			return;
+		}
 		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;
-				}
-				this.emit('error', error);
-			})
+			.then((contents) => this.#applyRead(() => contents))
+			.catch((error) => this.#recoverOrReport(error))
 			.finally(() => {
 				this.#pendingReads.delete(read);
 			});
 		this.#pendingReads.add(read);
 	}
 
+	#applyRead(read: () => string) {
+		let parsed;
+		try {
+			parsed = yaml.parse(read());
+		} catch (error) {
+			// A read or parse that fails while the file is being replaced is the same event as an
+			// incomplete one, and #handleReadError's ENOENT arm would answer it with a `remove`
+			// that restarts the scope. Re-read first; only an exhausted budget means it is real.
+			this.#recoverOrReport(error);
+			return;
+		}
+		// Tested on the file's own parse, before any env overlay: `''`, `'\n'` and a truncated
+		// document all parse to null, and an env-configured deployment would otherwise overlay
+		// one into a valid-looking object and adopt it. A file that is still unusable once the
+		// budget is spent is taken at face value, so emptying one still reaches `remove`.
+		if (!parsed || typeof parsed !== 'object') {
+			if (this.#partialRead.schedule(() => this.#handleChange())) return;
+			this.#partialRead.gaveUp();
+		} else {
+			this.#partialRead.settled();
+		}
+		try {
+			this.#applyParsed(this.#overlayEnvConfig(parsed));
+		} catch (error) {
+			// Applying is past the point where an incomplete file could explain a failure, so a
+			// listener's throw keeps the error route rather than being retried.
+			this.emit('error', error);
+		}
+	}
+
+	#recoverOrReport(error: unknown) {
+		if (!isPartialReadError(error)) return this.#handleReadError(error);
+		if (this.#partialRead.schedule(() => this.#handleChange())) return;
+		// Same give-up as the unusable-parse case, so the budget is restored for the repair: the
+		// write that fixes the file can itself be read mid-write. The error still takes the
+		// scope's own route.
+		this.#partialRead.gaveUp(error);
+		this.#handleReadError(error);
+	}
+
+	#overlayEnvConfig(parsed: unknown) {
+		// 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).
+		return this.#isRootConfig ? overlayRootEnvConfig(parsed) : parsed;
+	}
+
+	#applyParsed(parsed: unknown) {
+		this.#rootConfig = parsed && typeof parsed === 'object' ? (parsed as Config) : 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
@@ -388,6 +448,14 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 		this.emit('change', keys, value, this.#scopedConfig);
 	}
 
+	// Test-only: run the change handler directly, since the read's timing relative to its caller
+	// is the behaviour under test and a chokidar event cannot be observed at that granularity.
+	// Resolves once the read has landed, which for the root config has already happened.
+	_handleChangeForTests(): Promise<unknown> {
+		this.#handleChange();
+		return Promise.allSettled([...this.#pendingReads]);
+	}
+
 	// Test-only: simulate the underlying chokidar watcher emitting an error.
 	// Exposed so the polling-fallback path can be exercised without triggering a
 	// real ENOSPC/EMFILE on the host.
@@ -414,6 +482,7 @@ export class OptionsWatcher extends EventEmitter<OptionsWatcherEventMap> {
 	 */
 	close(): Promise<this> {
 		this.#closed = true;
+		this.#partialRead.cancel();
 		const pendingReads = [...this.#pendingReads];
 		const watcherClose = Promise.resolve(this.#watcher.close()).catch(() => {});
 

config/RootConfigWatcher.ts

@@ -1,9 +1,16 @@
 import chokidar, { FSWatcher } from 'chokidar';
-import { readFile } from 'node:fs/promises';
+import { readFileSync } from 'node:fs';
 import { getConfigFilePath } from './configUtils.ts';
 import { EventEmitter, once } from 'node:events';
 import { parse } from 'yaml';
-import { POLLING_FALLBACK_OPTIONS, isWatcherExhaustionError, warnWatcherFallback } from '../utility/watcherFallback.ts';
+import {
+	POLLING_FALLBACK_OPTIONS,
+	PartialReadRetry,
+	isPartialReadError,
+	isWatcherExhaustionError,
+	warnWatcherFallback,
+	warnWatcherListenerError,
+} from '../utility/watcherFallback.ts';
 import { resolveWatchTarget } from '../utility/watchPath.ts';
 
 export class RootConfigWatcher extends EventEmitter {
@@ -14,13 +21,15 @@ export class RootConfigWatcher extends EventEmitter {
 	#usingPolling: boolean;
 	#closed: boolean;
 	#openCount: number = 0;
+	#partialRead: PartialReadRetry;
 	ready: Promise<any[]>;
 
 	constructor() {
 		super();
 		this.#configFilePath = getConfigFilePath();
 		const watchTarget = resolveWatchTarget(this.#configFilePath);
 		this.#watchPath = watchTarget.path;
+		this.#partialRead = new PartialReadRetry(this.#configFilePath);
 		this.#usingPolling = watchTarget.mustPoll;
 		this.#closed = false;
 		this.ready = once(this, 'ready');
@@ -81,28 +90,47 @@ export class RootConfigWatcher extends EventEmitter {
 		this.emit('error', error);
 	}
 
+	// See the descriptor-lifetime invariant on atomicWriteFile (DESIGN.md).
 	handleChange() {
-		readFile(this.#configFilePath, 'utf-8')
-			.then((data) => {
-				if (!data) return;
-
-				const config = parse(data);
+		let config;
+		// Only the read and parse are guarded: a listener that throws must not be mistaken for a
+		// half-written file and replayed.
+		try {
+			config = parse(readFileSync(this.#configFilePath, 'utf-8'));
+		} catch (error) {
+			// A missing file needs no re-read; anything else may be the file being replaced.
+			if (isPartialReadError(error)) this.#scheduleReread(error);
+			return;
+		}
+		// A snapshot that does not parse to an object is the other shape a half-written file
+		// takes: `''`, `'\n'` and a truncated document all yield null, and adopting that would
+		// hand every consumer a config with nothing in it.
+		if (!config || typeof config !== 'object') {
+			this.#scheduleReread();
+			return;
+		}
+		this.#partialRead.settled();
 
-				if (!this.#config) {
-					this.#config = config;
-					this.emit('ready', this.#config);
-					return;
-				}
+		try {
+			if (!this.#config) {
+				this.#config = config;
+				this.emit('ready', this.#config);
+				return;
+			}
+			this.emit('change', (this.#config = config));
+		} catch (error) {
+			warnWatcherListenerError(this.#configFilePath, error);
+		}
+	}
 
-				this.emit('change', (this.#config = config));
-			})
-			.catch((_error) => {
-				// if yaml parse error ignore?
-			});
+	#scheduleReread(error?: unknown) {
+		if (this.#partialRead.schedule(() => this.handleChange())) return;
+		this.#partialRead.gaveUp(error);
 	}
 
 	close() {
 		this.#closed = true;
+		this.#partialRead.cancel();
 		this.#watcher.close();
 		this.#config = undefined;
 		this.emit('close');