From c3569a2a6e3516653d3cd926ffdcc148b3816aae Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 06:03:20 -0600 Subject: [PATCH 01/17] probe(windows): record which open handles block rename-over-destination Temporary investigation harness for the set_configuration EPERM failure. Co-Authored-By: Claude Opus --- .../platform/windowsRenameSemantics.test.mjs | 92 +++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 integrationTests/platform/windowsRenameSemantics.test.mjs diff --git a/integrationTests/platform/windowsRenameSemantics.test.mjs b/integrationTests/platform/windowsRenameSemantics.test.mjs new file mode 100644 index 0000000000..a45a11892d --- /dev/null +++ b/integrationTests/platform/windowsRenameSemantics.test.mjs @@ -0,0 +1,92 @@ +/** + * Probe: what actually blocks `fs.renameSync(tmp, dest)` on Windows. + * + * Temporary investigation harness for the `set_configuration` EPERM failure. Prints one + * JSON line per case to the shard log; assertions are deliberately absent so a surprising + * result is reported rather than hidden behind a red test. + */ +import { suite, test } from 'node:test'; +import { readFile } from 'node:fs/promises'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +const RETRY_DELAYS = [10, 20, 40, 80, 160, 320, 500, 500, 500, 500, 500, 500]; +const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)); + +function attempt(from, to) { + try { + fs.renameSync(from, to); + return { ok: true }; + } catch (error) { + return { ok: false, code: error.code, message: error.message }; + } +} + +function report(name, value) { + console.log(`RENAME-PROBE ${name} ${JSON.stringify(value)}`); +} + +suite('windows rename semantics probe', () => { + test('records which handles block rename-over-destination', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'rename-probe-')); + report('env', { platform: process.platform, node: process.version, dir }); + + const dest = path.join(dir, 'harper-config.yaml'); + const source = (n) => { + const p = path.join(dir, `harper-config.yaml.${n}.tmp`); + fs.writeFileSync(p, `case: ${n}\n`.padEnd(4096, '#')); + return p; + }; + + fs.writeFileSync(dest, 'case: base\n'); + report('A-control-no-handles', attempt(source('a'), dest)); + + const syncFd = fs.openSync(dest, 'r'); + report('B-dest-held-by-open-sync-fd', attempt(source('b'), dest)); + fs.closeSync(syncFd); + report('B2-after-closing-that-fd', attempt(source('b2'), dest)); + + // The production shape: an in-flight fsPromises.readFile whose close() cannot run + // because this thread is blocked in the retry loop. + const inFlight = readFile(dest, 'utf-8'); + const attempts = []; + for (let i = 0; i <= RETRY_DELAYS.length; i++) { + attempts.push(attempt(source(`c${i}`), dest)); + if (attempts.at(-1).ok) break; + if (i < RETRY_DELAYS.length) Atomics.wait(sleepBuffer, 0, 0, RETRY_DELAYS[i]); + } + report('C-dest-held-by-in-flight-readFile-while-loop-blocks', { + attempts: attempts.length, + anyOk: attempts.some((a) => a.ok), + first: attempts[0], + last: attempts.at(-1), + }); + await inFlight; + report('C2-after-awaiting-that-read', attempt(source('c-after'), dest)); + + const awaited = await readFile(dest, 'utf-8'); + report('D-dest-read-awaited-first', { bytes: awaited.length, ...attempt(source('d'), dest) }); + + const heldSource = source('e'); + const sourceFd = fs.openSync(heldSource, 'r'); + report('E-source-held-by-open-sync-fd', attempt(heldSource, dest)); + fs.closeSync(sourceFd); + + // Delete-pending: on Windows a file unlinked while open keeps its name reserved + // until the last handle closes, which would also reserve it against a rename. + const pendingFd = fs.openSync(dest, 'r'); + let unlinked; + try { + fs.unlinkSync(dest); + unlinked = { ok: true, stillExists: fs.existsSync(dest) }; + } catch (error) { + unlinked = { ok: false, code: error.code }; + } + report('F-unlink-while-open', unlinked); + report('F2-rename-into-that-name', attempt(source('f'), dest)); + fs.closeSync(pendingFd); + + fs.rmSync(dir, { recursive: true, force: true }); + }); +}); From 0e9ee3e7d67b6aaf74670a22e8fcba3caed2b8d4 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 06:06:14 -0600 Subject: [PATCH 02/17] probe(windows): add native-watcher and chokidar cases to the rename probe Co-Authored-By: Claude Opus --- .../platform/windowsRenameSemantics.test.mjs | 52 ++++++++++++++----- 1 file changed, 39 insertions(+), 13 deletions(-) diff --git a/integrationTests/platform/windowsRenameSemantics.test.mjs b/integrationTests/platform/windowsRenameSemantics.test.mjs index a45a11892d..c4e0905023 100644 --- a/integrationTests/platform/windowsRenameSemantics.test.mjs +++ b/integrationTests/platform/windowsRenameSemantics.test.mjs @@ -7,9 +7,11 @@ */ import { suite, test } from 'node:test'; import { readFile } from 'node:fs/promises'; +import { once } from 'node:events'; import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; +import chokidar from 'chokidar'; const RETRY_DELAYS = [10, 20, 40, 80, 160, 320, 500, 500, 500, 500, 500, 500]; const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)); @@ -33,11 +35,28 @@ suite('windows rename semantics probe', () => { report('env', { platform: process.platform, node: process.version, dir }); const dest = path.join(dir, 'harper-config.yaml'); + let caseNumber = 0; const source = (n) => { - const p = path.join(dir, `harper-config.yaml.${n}.tmp`); + const p = path.join(dir, `harper-config.yaml.${n}.${caseNumber++}.tmp`); fs.writeFileSync(p, `case: ${n}\n`.padEnd(4096, '#')); return p; }; + // Run the production backoff against a holder that is not expected to release, so a + // blocked-loop case reports the same shape the server does. + const retryLoop = (label) => { + const attempts = []; + for (let i = 0; i <= RETRY_DELAYS.length; i++) { + attempts.push(attempt(source(label), dest)); + if (attempts.at(-1).ok) break; + if (i < RETRY_DELAYS.length) Atomics.wait(sleepBuffer, 0, 0, RETRY_DELAYS[i]); + } + return { + attempts: attempts.length, + anyOk: attempts.some((a) => a.ok), + first: attempts[0], + last: attempts.at(-1), + }; + }; fs.writeFileSync(dest, 'case: base\n'); report('A-control-no-handles', attempt(source('a'), dest)); @@ -50,18 +69,7 @@ suite('windows rename semantics probe', () => { // The production shape: an in-flight fsPromises.readFile whose close() cannot run // because this thread is blocked in the retry loop. const inFlight = readFile(dest, 'utf-8'); - const attempts = []; - for (let i = 0; i <= RETRY_DELAYS.length; i++) { - attempts.push(attempt(source(`c${i}`), dest)); - if (attempts.at(-1).ok) break; - if (i < RETRY_DELAYS.length) Atomics.wait(sleepBuffer, 0, 0, RETRY_DELAYS[i]); - } - report('C-dest-held-by-in-flight-readFile-while-loop-blocks', { - attempts: attempts.length, - anyOk: attempts.some((a) => a.ok), - first: attempts[0], - last: attempts.at(-1), - }); + report('C-dest-held-by-in-flight-readFile-while-loop-blocks', retryLoop('c')); await inFlight; report('C2-after-awaiting-that-read', attempt(source('c-after'), dest)); @@ -73,6 +81,24 @@ suite('windows rename semantics probe', () => { report('E-source-held-by-open-sync-fd', attempt(heldSource, dest)); fs.closeSync(sourceFd); + // A native file watcher is the one handle Harper holds on the config file + // continuously, so it would make the failure permanent rather than intermittent. + const fileWatcher = fs.watch(dest, () => {}); + report('G-dest-watched-by-fs-watch-file', attempt(source('g'), dest)); + fileWatcher.close(); + + const dirWatcher = fs.watch(dir, () => {}); + report('H-dir-watched-by-fs-watch-dir', attempt(source('h'), dest)); + dirWatcher.close(); + + const chokidarWatcher = chokidar.watch(dest, { persistent: false }); + await once(chokidarWatcher, 'ready'); + report('I-dest-watched-by-chokidar', attempt(source('i'), dest)); + const chokidarInFlight = readFile(dest, 'utf-8'); + report('J-chokidar-plus-in-flight-readFile', retryLoop('j')); + await chokidarInFlight; + await chokidarWatcher.close(); + // Delete-pending: on Windows a file unlinked while open keeps its name reserved // until the last handle closes, which would also reserve it against a rename. const pendingFd = fs.openSync(dest, 'r'); From c507e46289c4ce99e3fc0fc4f1e1ede0d52a0a89 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 06:45:11 -0600 Subject: [PATCH 03/17] fix(windows): read the root config synchronously so its atomic write can land On Windows, rename over a destination fails with EPERM while any descriptor is open on it. Harper's root-config watchers read with fsPromises.readFile, whose close runs on the reading thread's event loop, and atomicWriteFile retries the rename with a blocking sleep on that same thread - so a read still in flight can never be released and every retry is guaranteed to fail. set_configuration then returns 500 after burning the full retry budget. That is why widening the budget in #1714 and #2036 changed nothing but the duration of the failure. Measured on the Windows CI runner (Node v24.19.0): a destination held by a single Node read descriptor fails the rename, all 13 attempts fail while the loop blocks, and the very next attempt after awaiting that read succeeds. Holding the source, or watching the file with fs.watch or chokidar, does not block it. Bound the descriptor to a single syscall by reading the root config synchronously in RootConfigWatcher and in OptionsWatcher's root-config branch. Application configs are written in place, never by rename-over, so they keep the non-blocking read and its pending-read drain. Also move the temp-file write inside the cleanup boundary so a write that fails partway cannot orphan a partial temp, and log the attempt count and elapsed time when a rename genuinely cannot be completed. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 142 +++++++++++------- config/RootConfigWatcher.ts | 34 +++-- config/configUtils.ts | 66 +++++--- .../apiTests/configuration.test.mjs | 18 +++ .../platform/windowsRenameSemantics.test.mjs | 118 --------------- .../config/configReadHandleLifetime.test.js | 99 ++++++++++++ unitTests/config/rootConfigWatcher.test.js | 5 +- 7 files changed, 265 insertions(+), 217 deletions(-) delete mode 100644 integrationTests/platform/windowsRenameSemantics.test.mjs create mode 100644 unitTests/config/configReadHandleLifetime.test.js diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index f4f16be3d2..bff24418c0 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -4,6 +4,7 @@ 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'; @@ -129,72 +130,90 @@ export class OptionsWatcher extends EventEmitter { .on('ready', this.#handleChange.bind(this)); } + // The root config is replaced by rename-over (atomicWriteFile), which on Windows fails while + // any descriptor is open on the destination; the writer's retry loop blocks the calling + // thread, so a descriptor this watcher leaves open across an event-loop turn can never be + // closed while a write on that thread waits for it. Reading it synchronously bounds the + // descriptor to a single syscall. Application configs are written in place (`fs.outputFile`, + // components/operations.js), never by rename, so they keep the non-blocking read. #handleChange() { + if (this.#isRootConfig) { + try { + this.#applyContents(readFileSync(this.#filePath, 'utf-8')); + } catch (error) { + this.#handleReadError(error); + } + return; + } const read: Promise = 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.#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 @@ -388,6 +407,13 @@ export class OptionsWatcher extends EventEmitter { this.emit('change', keys, value, this.#scopedConfig); } + // Test-only: run the change handler directly. The read's timing relative to the caller is + // the behaviour under test (see the config-write deadlock note on #handleChange), and a + // chokidar event cannot be observed at that granularity from outside. + _handleChangeForTests(): void { + this.#handleChange(); + } + // 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. diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index ad247f14ac..c259db7cb4 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -1,5 +1,5 @@ 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'; @@ -81,24 +81,28 @@ export class RootConfigWatcher extends EventEmitter { this.emit('error', error); } + // Read synchronously, not with fsPromises.readFile. atomicWriteFile replaces this file by + // rename-over, which on Windows fails while any descriptor is open on the destination, and + // its retry loop blocks the calling thread - so a descriptor this watcher leaves open across + // an event-loop turn can never be closed while a write on this thread is waiting for it. handleChange() { - readFile(this.#configFilePath, 'utf-8') - .then((data) => { - if (!data) return; + try { + const data = readFileSync(this.#configFilePath, 'utf-8'); + if (!data) return; - const config = parse(data); + 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 { + // A read or parse failure here is transient (mid-write, or the install window); + // the next watcher event re-reads. + } } close() { diff --git a/config/configUtils.ts b/config/configUtils.ts index 135ada3dbf..c2073a9025 100644 --- a/config/configUtils.ts +++ b/config/configUtils.ts @@ -91,14 +91,14 @@ export function getConfigPath(param: string) { // in the same millisecond can't collide on the temp name and then race the rename. // // 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. +// EPERM/EACCES while another descriptor is open on the destination. This retry can only ride +// out a holder that releases on its own - another thread, another process, an AV scan. It can +// never ride out a holder on the CALLING thread, because the sleep below blocks the event loop +// that would have to run to close it, so the holder's lifetime becomes exactly the retry +// budget. That is why config readers must not leave a descriptor on this file open across an +// event-loop turn (RootConfigWatcher.handleChange, OptionsWatcher#handleChange), and why +// widening this budget twice (#1714, #2036) never fixed the set_configuration 500s it was +// aimed at. const RENAME_RETRY_MAX_ATTEMPTS = 12; const RENAME_RETRY_INITIAL_DELAY_MS = 10; const RENAME_RETRY_MAX_DELAY_MS = 500; @@ -115,30 +115,46 @@ export function atomicWriteFile( } = {} ) { const tempPath = `${filePath}.${process.pid}.${threadId}.${randomBytes(4).toString('hex')}.tmp`; - fs.writeFileSync(tempPath, content); let retries = maxRetries; let delayMs = initialDelayMs; - while (true) { - try { - fs.renameSync(tempPath, filePath); - break; - } catch (err) { - if (retries > 0 && (err.code === 'EPERM' || err.code === 'EACCES')) { - 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; + let attempts = 0; + const startedAt = Date.now(); + let renamed = false; + try { + // Inside the cleanup boundary: a write that fails partway (ENOSPC, EIO) still leaves a + // partial temp file holding configuration values. + fs.writeFileSync(tempPath, content); + while (!renamed) { + try { + attempts++; + fs.renameSync(tempPath, filePath); + renamed = true; + } catch (err) { + if (retries > 0 && (err.code === 'EPERM' || err.code === 'EACCES')) { + 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. + if (delayMs > 0) Atomics.wait(renameRetrySleepBuffer, 0, 0, delayMs); + delayMs = Math.min(delayMs * 2, maxDelayMs); + continue; + } + // Attempts and elapsed time are what distinguish a holder that never released from + // one that simply lost a race; neither is recoverable from the rethrown error. + if (err.code === 'EPERM' || err.code === 'EACCES') { + logger.warn( + `Could not replace ${filePath}: ${err.code} after ${attempts} attempts over ${Date.now() - startedAt}ms. Another process or thread is holding the file open.` + ); + } + throw err; } - // if it fails we should clean up the tmp file + } + } finally { + if (!renamed) { try { fs.unlinkSync(tempPath); } catch { - // ignore cleanup errors + // A cleanup failure must not replace the error that got us here. } - throw err; } } } diff --git a/integrationTests/apiTests/configuration.test.mjs b/integrationTests/apiTests/configuration.test.mjs index ba63039c1f..5e97fe1c19 100644 --- a/integrationTests/apiTests/configuration.test.mjs +++ b/integrationTests/apiTests/configuration.test.mjs @@ -343,6 +343,24 @@ suite('Configuration', (ctx) => { .expect(200); }); + test('back-to-back set_configuration calls all land', async () => { + // The root config is replaced by rename-over, which on Windows fails while any descriptor + // is open on the destination — and every config write fans out a re-read to each thread's + // root-config watcher. A second write arriving before those reads finish used to hit an + // EPERM the writer's blocking retry could never clear, returning 500 (harper#2313). + // Adding a component key first is what makes the fan-out largest: it creates a new scope. + await client.req().send({ 'operation': 'set_configuration', 'burst-probe_package': 'file:./nowhere' }).expect(200); + for (const maxSize of ['21M', '22M', '23M', '24M']) { + await client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }).expect(200); + } + await client + .req() + .send({ operation: 'get_configuration' }) + .expect((r) => assert.strictEqual(r?.body?.logging?.rotation?.maxSize, '24M', r?.text)) + .expect(200); + await client.req().send({ 'operation': 'set_configuration', 'burst-probe_package': null }).expect(200); + }); + // ── set_configuration + replicated (#660) ─────────────────────────────── // Real cluster fan-out lives in harper-pro; without it, the base server's // replication stub rejects a truthy `replicated`. These tests pin the diff --git a/integrationTests/platform/windowsRenameSemantics.test.mjs b/integrationTests/platform/windowsRenameSemantics.test.mjs deleted file mode 100644 index c4e0905023..0000000000 --- a/integrationTests/platform/windowsRenameSemantics.test.mjs +++ /dev/null @@ -1,118 +0,0 @@ -/** - * Probe: what actually blocks `fs.renameSync(tmp, dest)` on Windows. - * - * Temporary investigation harness for the `set_configuration` EPERM failure. Prints one - * JSON line per case to the shard log; assertions are deliberately absent so a surprising - * result is reported rather than hidden behind a red test. - */ -import { suite, test } from 'node:test'; -import { readFile } from 'node:fs/promises'; -import { once } from 'node:events'; -import fs from 'node:fs'; -import os from 'node:os'; -import path from 'node:path'; -import chokidar from 'chokidar'; - -const RETRY_DELAYS = [10, 20, 40, 80, 160, 320, 500, 500, 500, 500, 500, 500]; -const sleepBuffer = new Int32Array(new SharedArrayBuffer(4)); - -function attempt(from, to) { - try { - fs.renameSync(from, to); - return { ok: true }; - } catch (error) { - return { ok: false, code: error.code, message: error.message }; - } -} - -function report(name, value) { - console.log(`RENAME-PROBE ${name} ${JSON.stringify(value)}`); -} - -suite('windows rename semantics probe', () => { - test('records which handles block rename-over-destination', async () => { - const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'rename-probe-')); - report('env', { platform: process.platform, node: process.version, dir }); - - const dest = path.join(dir, 'harper-config.yaml'); - let caseNumber = 0; - const source = (n) => { - const p = path.join(dir, `harper-config.yaml.${n}.${caseNumber++}.tmp`); - fs.writeFileSync(p, `case: ${n}\n`.padEnd(4096, '#')); - return p; - }; - // Run the production backoff against a holder that is not expected to release, so a - // blocked-loop case reports the same shape the server does. - const retryLoop = (label) => { - const attempts = []; - for (let i = 0; i <= RETRY_DELAYS.length; i++) { - attempts.push(attempt(source(label), dest)); - if (attempts.at(-1).ok) break; - if (i < RETRY_DELAYS.length) Atomics.wait(sleepBuffer, 0, 0, RETRY_DELAYS[i]); - } - return { - attempts: attempts.length, - anyOk: attempts.some((a) => a.ok), - first: attempts[0], - last: attempts.at(-1), - }; - }; - - fs.writeFileSync(dest, 'case: base\n'); - report('A-control-no-handles', attempt(source('a'), dest)); - - const syncFd = fs.openSync(dest, 'r'); - report('B-dest-held-by-open-sync-fd', attempt(source('b'), dest)); - fs.closeSync(syncFd); - report('B2-after-closing-that-fd', attempt(source('b2'), dest)); - - // The production shape: an in-flight fsPromises.readFile whose close() cannot run - // because this thread is blocked in the retry loop. - const inFlight = readFile(dest, 'utf-8'); - report('C-dest-held-by-in-flight-readFile-while-loop-blocks', retryLoop('c')); - await inFlight; - report('C2-after-awaiting-that-read', attempt(source('c-after'), dest)); - - const awaited = await readFile(dest, 'utf-8'); - report('D-dest-read-awaited-first', { bytes: awaited.length, ...attempt(source('d'), dest) }); - - const heldSource = source('e'); - const sourceFd = fs.openSync(heldSource, 'r'); - report('E-source-held-by-open-sync-fd', attempt(heldSource, dest)); - fs.closeSync(sourceFd); - - // A native file watcher is the one handle Harper holds on the config file - // continuously, so it would make the failure permanent rather than intermittent. - const fileWatcher = fs.watch(dest, () => {}); - report('G-dest-watched-by-fs-watch-file', attempt(source('g'), dest)); - fileWatcher.close(); - - const dirWatcher = fs.watch(dir, () => {}); - report('H-dir-watched-by-fs-watch-dir', attempt(source('h'), dest)); - dirWatcher.close(); - - const chokidarWatcher = chokidar.watch(dest, { persistent: false }); - await once(chokidarWatcher, 'ready'); - report('I-dest-watched-by-chokidar', attempt(source('i'), dest)); - const chokidarInFlight = readFile(dest, 'utf-8'); - report('J-chokidar-plus-in-flight-readFile', retryLoop('j')); - await chokidarInFlight; - await chokidarWatcher.close(); - - // Delete-pending: on Windows a file unlinked while open keeps its name reserved - // until the last handle closes, which would also reserve it against a rename. - const pendingFd = fs.openSync(dest, 'r'); - let unlinked; - try { - fs.unlinkSync(dest); - unlinked = { ok: true, stillExists: fs.existsSync(dest) }; - } catch (error) { - unlinked = { ok: false, code: error.code }; - } - report('F-unlink-while-open', unlinked); - report('F2-rename-into-that-name', attempt(source('f'), dest)); - fs.closeSync(pendingFd); - - fs.rmSync(dir, { recursive: true, force: true }); - }); -}); diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js new file mode 100644 index 0000000000..68cc925aec --- /dev/null +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -0,0 +1,99 @@ +const assert = require('node:assert'); +const { join } = require('node:path'); +const { tmpdir } = require('node:os'); +const { mkdtempSync, writeFileSync, rmSync } = require('node:fs'); +const { once } = require('node:events'); +const { stringify } = require('yaml'); +const { RootConfigWatcher } = require('#src/config/RootConfigWatcher'); +const { OptionsWatcher } = require('#src/components/OptionsWatcher'); +const { HARPER_CONFIG_FILE } = require('#src/utility/hdbTerms'); + +/** + * `atomicWriteFile` replaces the root config by rename-over and retries with a blocking sleep. + * On Windows that rename fails while any descriptor is open on the destination, and the sleep + * blocks the event loop that would close one — so a root-config read that outlives the turn it + * started in makes every retry fail and `set_configuration` return 500 (harper#2313). + * + * These tests pin the lifetime rather than the platform rule: a root-config read must be + * complete by the time the change handler returns. They fail on a `fsPromises.readFile` + * implementation on every platform. + */ +describe('root config read handle lifetime', () => { + let fixture; + let configFilePath; + let previousRootPath; + const openWatchers = []; + + beforeEach(() => { + fixture = mkdtempSync(join(tmpdir(), 'harper.unit-test.config-read-lifetime-')); + configFilePath = join(fixture, HARPER_CONFIG_FILE); + writeFileSync(configFilePath, stringify({ 'test-component': { enabled: true } })); + previousRootPath = process.env.ROOTPATH; + process.env.ROOTPATH = fixture; + }); + + afterEach(async () => { + await Promise.all(openWatchers.splice(0).map((watcher) => watcher.close())); + if (previousRootPath === undefined) delete process.env.ROOTPATH; + else process.env.ROOTPATH = previousRootPath; + rmSync(fixture, { recursive: true, force: true }); + }); + + it('RootConfigWatcher applies a change before handleChange returns', async () => { + const watcher = new RootConfigWatcher(); + openWatchers.push(watcher); + await watcher.ready; + + writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); + watcher.handleChange(); + + assert.deepStrictEqual( + watcher.config, + { 'test-component': { enabled: false } }, + 'the config must be re-read before control returns, or the descriptor outlives the turn' + ); + }); + + it('RootConfigWatcher swallows a read failure without throwing into the watcher callback', async () => { + const watcher = new RootConfigWatcher(); + openWatchers.push(watcher); + await watcher.ready; + + rmSync(configFilePath); + assert.doesNotThrow(() => watcher.handleChange()); + assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }); + + writeFileSync(configFilePath, ': not: valid: yaml:'); + assert.doesNotThrow(() => watcher.handleChange()); + assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }); + }); + + it('OptionsWatcher applies a root-config change before the change handler returns', async () => { + const watcher = new OptionsWatcher('test-component', configFilePath, undefined, true); + openWatchers.push(watcher); + await watcher.ready; + + writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); + watcher._handleChangeForTests(); + + assert.strictEqual(watcher.get(['enabled']), false); + }); + + it('OptionsWatcher still reads an application config without blocking', async () => { + // Application configs are written in place, never by rename-over, so they must keep the + // non-blocking read — a slow or stalled component-config volume must not stall the thread. + const appConfigPath = join(fixture, 'config.yaml'); + writeFileSync(appConfigPath, stringify({ 'test-component': { enabled: true } })); + const watcher = new OptionsWatcher('test-component', appConfigPath, undefined, false); + openWatchers.push(watcher); + await watcher.ready; + + writeFileSync(appConfigPath, stringify({ 'test-component': { enabled: false } })); + const changed = once(watcher, 'change'); + watcher._handleChangeForTests(); + + assert.strictEqual(watcher.get(['enabled']), true, 'application config reads must not be synchronous'); + await changed; + assert.strictEqual(watcher.get(['enabled']), false); + }); +}); diff --git a/unitTests/config/rootConfigWatcher.test.js b/unitTests/config/rootConfigWatcher.test.js index 3aa48aa7b2..66a8ab24ff 100644 --- a/unitTests/config/rootConfigWatcher.test.js +++ b/unitTests/config/rootConfigWatcher.test.js @@ -39,9 +39,12 @@ describe('RootConfigWatcher', () => { expected.foo = 'baz'; + // Subscribe before writing: the watcher re-reads the root config synchronously, so the + // change event can be emitted before this writer's own await resolves. + const changed = once(configWatcher, 'change'); await writeFile(this.configFilePath, stringify(expected)); - const [updated] = await once(configWatcher, 'change'); + const [updated] = await changed; assert.deepEqual(updated, expected, 'RootConfigWatcher should emit a change event with the updated config'); From e7dd43027df0391fa6e81312d1606a32a9c957e9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 07:08:14 -0600 Subject: [PATCH 04/17] fix(config): address pre-push review findings - guarantee the burst test's config teardown with try/finally, and add a concurrent burst plus a temp-straggler assertion so the test pins more of the precondition - stop the exhausted-retry log from asserting an open handle when the cause may be a genuine permission error - trim the added comments to the invariant a reader needs Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 10 ++-- config/RootConfigWatcher.ts | 7 ++- config/configUtils.ts | 22 ++++---- .../apiTests/configuration.test.mjs | 52 ++++++++++++++----- 4 files changed, 54 insertions(+), 37 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index bff24418c0..a0a50038d4 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -130,12 +130,10 @@ export class OptionsWatcher extends EventEmitter { .on('ready', this.#handleChange.bind(this)); } - // The root config is replaced by rename-over (atomicWriteFile), which on Windows fails while - // any descriptor is open on the destination; the writer's retry loop blocks the calling - // thread, so a descriptor this watcher leaves open across an event-loop turn can never be - // closed while a write on that thread waits for it. Reading it synchronously bounds the - // descriptor to a single syscall. Application configs are written in place (`fs.outputFile`, - // components/operations.js), never by rename, so they keep the non-blocking read. + // Read the root config synchronously so the descriptor cannot outlive this turn: it is + // replaced by rename-over, which on Windows fails while any descriptor is open on it, and + // the writer's retry blocks the thread that would close one. Application configs are written + // in place, never by rename-over, so they keep the non-blocking read and its drain. #handleChange() { if (this.#isRootConfig) { try { diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index c259db7cb4..669f9bc068 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -81,10 +81,9 @@ export class RootConfigWatcher extends EventEmitter { this.emit('error', error); } - // Read synchronously, not with fsPromises.readFile. atomicWriteFile replaces this file by - // rename-over, which on Windows fails while any descriptor is open on the destination, and - // its retry loop blocks the calling thread - so a descriptor this watcher leaves open across - // an event-loop turn can never be closed while a write on this thread is waiting for it. + // Read synchronously so the descriptor cannot outlive this turn: atomicWriteFile replaces + // this file by rename-over, which on Windows fails while any descriptor is open on it, and + // its retry blocks the very thread that would close one. handleChange() { try { const data = readFileSync(this.#configFilePath, 'utf-8'); diff --git a/config/configUtils.ts b/config/configUtils.ts index c2073a9025..953e6e6231 100644 --- a/config/configUtils.ts +++ b/config/configUtils.ts @@ -91,14 +91,11 @@ export function getConfigPath(param: string) { // in the same millisecond can't collide on the temp name and then race the rename. // // Windows has no POSIX-style "replace an open file" semantics: rename() fails with -// EPERM/EACCES while another descriptor is open on the destination. This retry can only ride -// out a holder that releases on its own - another thread, another process, an AV scan. It can -// never ride out a holder on the CALLING thread, because the sleep below blocks the event loop -// that would have to run to close it, so the holder's lifetime becomes exactly the retry -// budget. That is why config readers must not leave a descriptor on this file open across an -// event-loop turn (RootConfigWatcher.handleChange, OptionsWatcher#handleChange), and why -// widening this budget twice (#1714, #2036) never fixed the set_configuration 500s it was -// aimed at. +// EPERM/EACCES while another descriptor is open on the destination. The sleep below blocks the +// calling thread, so this can only ride out a holder that releases without needing that +// thread's event loop. A holder on the calling thread would live exactly as long as the budget, +// which is why config readers must not keep a descriptor on this file open across an event-loop +// turn (RootConfigWatcher.handleChange, OptionsWatcher#handleChange). const RENAME_RETRY_MAX_ATTEMPTS = 12; const RENAME_RETRY_INITIAL_DELAY_MS = 10; const RENAME_RETRY_MAX_DELAY_MS = 500; @@ -121,8 +118,7 @@ export function atomicWriteFile( const startedAt = Date.now(); let renamed = false; try { - // Inside the cleanup boundary: a write that fails partway (ENOSPC, EIO) still leaves a - // partial temp file holding configuration values. + // Inside the cleanup boundary: a write that fails partway leaves a partial temp behind. fs.writeFileSync(tempPath, content); while (!renamed) { try { @@ -138,11 +134,11 @@ export function atomicWriteFile( delayMs = Math.min(delayMs * 2, maxDelayMs); continue; } - // Attempts and elapsed time are what distinguish a holder that never released from - // one that simply lost a race; neither is recoverable from the rethrown error. + // Attempts and elapsed distinguish a holder that never released from one that lost + // a race, and neither survives on the rethrown error. if (err.code === 'EPERM' || err.code === 'EACCES') { logger.warn( - `Could not replace ${filePath}: ${err.code} after ${attempts} attempts over ${Date.now() - startedAt}ms. Another process or thread is holding the file open.` + `Could not replace ${filePath}: ${err.code} after ${attempts} attempts over ${Date.now() - startedAt}ms` ); } throw err; diff --git a/integrationTests/apiTests/configuration.test.mjs b/integrationTests/apiTests/configuration.test.mjs index 5e97fe1c19..9c70b5ef1a 100644 --- a/integrationTests/apiTests/configuration.test.mjs +++ b/integrationTests/apiTests/configuration.test.mjs @@ -16,6 +16,7 @@ */ import { suite, test, before, after } from 'node:test'; import assert from 'node:assert'; +import { readdirSync } from 'node:fs'; import request from 'supertest'; import { startHarper, teardownHarper } from '@harperfast/integration-testing'; import { createApiClient } from './utils/client.mjs'; @@ -344,21 +345,44 @@ suite('Configuration', (ctx) => { }); test('back-to-back set_configuration calls all land', async () => { - // The root config is replaced by rename-over, which on Windows fails while any descriptor - // is open on the destination — and every config write fans out a re-read to each thread's - // root-config watcher. A second write arriving before those reads finish used to hit an - // EPERM the writer's blocking retry could never clear, returning 500 (harper#2313). - // Adding a component key first is what makes the fan-out largest: it creates a new scope. - await client.req().send({ 'operation': 'set_configuration', 'burst-probe_package': 'file:./nowhere' }).expect(200); - for (const maxSize of ['21M', '22M', '23M', '24M']) { - await client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }).expect(200); + // Every config write fans out a re-read to each thread's root-config watcher, and the root + // config is replaced by rename-over, which on Windows fails while any descriptor is open + // on it (harper#2313). + let rootPath; + try { + await client + .req() + .send({ 'operation': 'set_configuration', 'burst-probe_package': 'file:./nowhere' }) + .expect(200); + for (const maxSize of ['21M', '22M', '23M', '24M']) { + await client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }).expect(200); + } + await client + .req() + .send({ operation: 'get_configuration' }) + .expect((r) => { + assert.strictEqual(r?.body?.logging?.rotation?.maxSize, '24M', r?.text); + rootPath = r?.body?.rootPath; + }) + .expect(200); + // Concurrent writes maximise the chance that a watcher read is still in flight when the + // next rename starts, which is the precondition the sequential burst cannot guarantee. + // Only the status is asserted: interleaved read-modify-writes make the last value racy. + const concurrent = await Promise.all( + ['31M', '32M', '33M', '34M'].map((maxSize) => + client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }) + ) + ); + for (const response of concurrent) assert.strictEqual(response.status, 200, response.text); + // A write that gave up would leave its temp behind, so this covers the cleanup path. + assert.deepStrictEqual( + readdirSync(rootPath).filter((entry) => entry.startsWith('harper-config.yaml.') && entry.endsWith('.tmp')), + [] + ); + } finally { + // Leaving the probe entry behind would change the config every later test observes. + await client.req().send({ 'operation': 'set_configuration', 'burst-probe_package': null }); } - await client - .req() - .send({ operation: 'get_configuration' }) - .expect((r) => assert.strictEqual(r?.body?.logging?.rotation?.maxSize, '24M', r?.text)) - .expect(200); - await client.req().send({ 'operation': 'set_configuration', 'burst-probe_package': null }).expect(200); }); // ── set_configuration + replicated (#660) ─────────────────────────────── From 9a05db0a551fb85b33e247b2cd42434abd87cb38 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 07:13:22 -0600 Subject: [PATCH 05/17] fix(config): retry a root-config read that observed a half-written file A synchronous read can catch an in-place writer between its truncate and its write. chokidar may emit nothing further for that write, so dropping the unusable read left the watcher serving stale config indefinitely - measured at 7 missed changes in 40 against 0 for the promise-based read it replaced. Re-read on a later turn instead, bounded so a genuinely empty or corrupt file cannot spin. The read stays synchronous, so the descriptor still never outlives the turn it started in. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 25 +++++++++++-- config/RootConfigWatcher.ts | 15 +++++--- .../config/configReadHandleLifetime.test.js | 18 ++++++++++ utility/watcherFallback.ts | 35 +++++++++++++++++++ 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index a0a50038d4..e29e9dce90 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -8,7 +8,12 @@ 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, + isWatcherExhaustionError, + warnWatcherFallback, +} from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; import { overlayRootEnvConfig, isRootConfigFilename } from '../config/harperConfigEnvVars.ts'; @@ -97,6 +102,7 @@ export class OptionsWatcher extends EventEmitter { #closed: boolean; #openCount: number = 0; #pendingReads: Set> = new Set(); + #partialRead = new PartialReadRetry(); ready: Promise; constructor(name: string, filePath: string, logger?: Logger, isRootConfig?: boolean) { @@ -136,8 +142,22 @@ export class OptionsWatcher extends EventEmitter { // in place, never by rename-over, so they keep the non-blocking read and its drain. #handleChange() { if (this.#isRootConfig) { + let contents: string; try { - this.#applyContents(readFileSync(this.#filePath, 'utf-8')); + contents = readFileSync(this.#filePath, 'utf-8'); + } catch (error) { + this.#handleReadError(error); + return; + } + // An in-place writer can be observed mid-write, and treating that as the file's real + // content would drop the scope's config; chokidar may emit nothing further for it. + if (!contents) { + this.#partialRead.schedule(() => this.#handleChange()); + return; + } + this.#partialRead.settled(); + try { + this.#applyContents(contents); } catch (error) { this.#handleReadError(error); } @@ -438,6 +458,7 @@ export class OptionsWatcher extends EventEmitter { */ close(): Promise { this.#closed = true; + this.#partialRead.cancel(); const pendingReads = [...this.#pendingReads]; const watcherClose = Promise.resolve(this.#watcher.close()).catch(() => {}); diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 669f9bc068..beb6c64a84 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -3,7 +3,12 @@ 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, + isWatcherExhaustionError, + warnWatcherFallback, +} from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; export class RootConfigWatcher extends EventEmitter { @@ -14,6 +19,7 @@ export class RootConfigWatcher extends EventEmitter { #usingPolling: boolean; #closed: boolean; #openCount: number = 0; + #partialRead = new PartialReadRetry(); ready: Promise; constructor() { @@ -87,9 +93,10 @@ export class RootConfigWatcher extends EventEmitter { handleChange() { try { const data = readFileSync(this.#configFilePath, 'utf-8'); - if (!data) return; + if (!data) return this.#partialRead.schedule(() => this.handleChange()); const config = parse(data); + this.#partialRead.settled(); if (!this.#config) { this.#config = config; @@ -99,13 +106,13 @@ export class RootConfigWatcher extends EventEmitter { this.emit('change', (this.#config = config)); } catch { - // A read or parse failure here is transient (mid-write, or the install window); - // the next watcher event re-reads. + this.#partialRead.schedule(() => this.handleChange()); } } close() { this.#closed = true; + this.#partialRead.cancel(); this.#watcher.close(); this.#config = undefined; this.emit('close'); diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index 68cc925aec..1fb8206269 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -68,6 +68,24 @@ describe('root config read handle lifetime', () => { assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }); }); + it('RootConfigWatcher recovers a change it first observed as a half-written file', async () => { + // A synchronous read can catch an in-place writer between its truncate and its write, and + // chokidar may emit nothing further for that write — so an unusable read must be retried + // rather than dropped, or the watcher serves stale config indefinitely. + const watcher = new RootConfigWatcher(); + openWatchers.push(watcher); + await watcher.ready; + + writeFileSync(configFilePath, ''); + watcher.handleChange(); + assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }, 'must not adopt an empty read'); + + const changed = once(watcher, 'change'); + writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); + const [updated] = await changed; + assert.deepStrictEqual(updated, { 'test-component': { enabled: false } }); + }); + it('OptionsWatcher applies a root-config change before the change handler returns', async () => { const watcher = new OptionsWatcher('test-component', configFilePath, undefined, true); openWatchers.push(watcher); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 7afdf43da3..be0e33e79e 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -32,6 +32,9 @@ export function isWatcherExhaustionError(error: unknown): boolean { * host is already under resource pressure. A second-scale interval keeps CPU * cost bounded; the alternative is to lose change events entirely. */ +const PARTIAL_READ_REREAD_DELAY_MS = 20; +const PARTIAL_READ_MAX_REREADS = 10; + export const POLLING_FALLBACK_OPTIONS = { usePolling: true, interval: 1000, @@ -72,3 +75,35 @@ export function warnWatcherFallback(watchedPath: string): void { export function _resetForTests(): void { exhaustionWarned = false; } + +/** + * A config file replaced in place (truncate, then write) can be read back empty or + * half-written, and chokidar may emit nothing further for that write — so a watcher that + * simply drops the unusable read would serve stale config until something else touched the + * file. Re-read on a later turn instead, bounded so a genuinely empty or corrupt file cannot + * spin. Callers read synchronously, so the descriptor still never outlives a single turn. + */ +export class PartialReadRetry { + #timer?: ReturnType; + #remaining: number = PARTIAL_READ_MAX_REREADS; + + schedule(reread: () => void) { + if (this.#timer || this.#remaining <= 0) return; + this.#remaining--; + this.#timer = setTimeout(() => { + this.#timer = undefined; + reread(); + }, PARTIAL_READ_REREAD_DELAY_MS); + this.#timer.unref?.(); + } + + settled() { + this.#remaining = PARTIAL_READ_MAX_REREADS; + } + + cancel() { + if (this.#timer) clearTimeout(this.#timer); + this.#timer = undefined; + this.#remaining = 0; + } +} From d0e79e306dba113f72eac932f1105bd1325c3406 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 07:33:23 -0600 Subject: [PATCH 06/17] fix(config): give both watchers one recovery path for an unusable read Round-2 review findings. The root branch of OptionsWatcher routed read and parse failures straight to #handleReadError, whose ENOENT arm answers with a remove that restarts the scope, while RootConfigWatcher retried the same failure on the same file for the same chokidar event. On Windows a transient read failure during a replace could therefore leave one component serving pre-change options while the logger's watcher picked the change up. Both now re-read first and fall through to the error path only once the budget is spent; a missing file is excluded, since that is unambiguous and already has correct semantics. Application configs get the same guard on the async path: they are rewritten in place, so an empty mid-write read was the case the retry exists for, and dropping it emitted remove. Also: settled() now clears an armed timer so a recovered read cannot replay; RootConfigWatcher's try covers only the read and parse, so a listener that throws is not mistaken for a half-written file; exhausting the budget is logged rather than silent; and the burst test drops the component key it could never remove. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 67 ++++++++++++------- config/RootConfigWatcher.ts | 41 ++++++++---- config/configUtils.ts | 2 - .../apiTests/configuration.test.mjs | 58 +++++++--------- .../config/configReadHandleLifetime.test.js | 43 ++++++++++-- utility/watcherFallback.ts | 27 +++++++- 6 files changed, 156 insertions(+), 82 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index e29e9dce90..13c7b6e1b6 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -11,7 +11,9 @@ import { cloneDeep } from 'lodash'; import { POLLING_FALLBACK_OPTIONS, PartialReadRetry, + isPartialReadError, isWatcherExhaustionError, + warnPartialReadGaveUp, warnWatcherFallback, } from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; @@ -136,43 +138,53 @@ export class OptionsWatcher extends EventEmitter { .on('ready', this.#handleChange.bind(this)); } - // Read the root config synchronously so the descriptor cannot outlive this turn: it is - // replaced by rename-over, which on Windows fails while any descriptor is open on it, and - // the writer's retry blocks the thread that would close one. Application configs are written - // in place, never by rename-over, so they keep the non-blocking read and its drain. + // The root config reads synchronously so its descriptor cannot outlive this turn - see the + // invariant on atomicWriteFile. Application configs keep the non-blocking read and its drain. #handleChange() { if (this.#isRootConfig) { - let contents: string; + let parsed; try { - contents = readFileSync(this.#filePath, 'utf-8'); + const contents = readFileSync(this.#filePath, 'utf-8'); + if (!contents) return this.#rereadOr(undefined); + parsed = this.#parseContents(contents); } catch (error) { - this.#handleReadError(error); - return; - } - // An in-place writer can be observed mid-write, and treating that as the file's real - // content would drop the scope's config; chokidar may emit nothing further for it. - if (!contents) { - this.#partialRead.schedule(() => this.#handleChange()); - return; + // A read or parse that fails on a file being replaced under us is the same event + // as an empty one, and the ENOENT arm of #handleReadError would answer it with a + // `remove` that restarts the scope. Re-read first; only a budget that runs out + // means the failure is real. + return this.#rereadOr(error); } this.#partialRead.settled(); - try { - this.#applyContents(contents); - } catch (error) { - this.#handleReadError(error); - } + this.#applyParsed(parsed); return; } const read: Promise = readFile(this.#filePath, 'utf-8') - .then((contents) => this.#applyContents(contents)) - .catch((error) => this.#handleReadError(error)) + .then((contents) => { + // Application configs are rewritten in place, so a mid-write read is exactly the + // case above, and dropping it here would `remove` the scope's config. + if (!contents) return this.#rereadOr(undefined); + const parsed = this.#parseContents(contents); + this.#partialRead.settled(); + this.#applyParsed(parsed); + }) + .catch((error) => this.#rereadOr(error)) .finally(() => { this.#pendingReads.delete(read); }); this.#pendingReads.add(read); } - #applyContents(contents: string) { + #rereadOr(error: unknown) { + if (error !== undefined && !isPartialReadError(error)) return this.#handleReadError(error); + if (this.#partialRead.schedule(() => this.#handleChange())) return; + if (error === undefined) { + warnPartialReadGaveUp(this.#filePath); + return; + } + this.#handleReadError(error); + } + + #parseContents(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 @@ -182,7 +194,11 @@ export class OptionsWatcher extends EventEmitter { // 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; + return 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 @@ -425,9 +441,8 @@ export class OptionsWatcher extends EventEmitter { this.emit('change', keys, value, this.#scopedConfig); } - // Test-only: run the change handler directly. The read's timing relative to the caller is - // the behaviour under test (see the config-write deadlock note on #handleChange), and a - // chokidar event cannot be observed at that granularity from outside. + // 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. _handleChangeForTests(): void { this.#handleChange(); } diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index beb6c64a84..297b7250cd 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -6,7 +6,9 @@ import { parse } from 'yaml'; import { POLLING_FALLBACK_OPTIONS, PartialReadRetry, + isPartialReadError, isWatcherExhaustionError, + warnPartialReadGaveUp, warnWatcherFallback, } from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; @@ -87,27 +89,38 @@ export class RootConfigWatcher extends EventEmitter { this.emit('error', error); } - // Read synchronously so the descriptor cannot outlive this turn: atomicWriteFile replaces - // this file by rename-over, which on Windows fails while any descriptor is open on it, and - // its retry blocks the very thread that would close one. + // Reads synchronously so its descriptor cannot outlive this turn - see the invariant on + // atomicWriteFile. handleChange() { + 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 { const data = readFileSync(this.#configFilePath, 'utf-8'); - if (!data) return this.#partialRead.schedule(() => this.handleChange()); - - const config = parse(data); - this.#partialRead.settled(); - - if (!this.#config) { - this.#config = config; - this.emit('ready', this.#config); + if (!data) { + this.#scheduleReread(); return; } + config = parse(data); + } catch (error) { + // A missing file needs no re-read; anything else may be the file being replaced. + if (isPartialReadError(error)) this.#scheduleReread(); + return; + } + this.#partialRead.settled(); - this.emit('change', (this.#config = config)); - } catch { - this.#partialRead.schedule(() => this.handleChange()); + if (!this.#config) { + this.#config = config; + this.emit('ready', this.#config); + return; } + + this.emit('change', (this.#config = config)); + } + + #scheduleReread() { + if (this.#partialRead.schedule(() => this.handleChange())) return; + warnPartialReadGaveUp(this.#configFilePath); } close() { diff --git a/config/configUtils.ts b/config/configUtils.ts index 953e6e6231..99c8fd56dc 100644 --- a/config/configUtils.ts +++ b/config/configUtils.ts @@ -128,8 +128,6 @@ export function atomicWriteFile( } catch (err) { if (retries > 0 && (err.code === 'EPERM' || err.code === 'EACCES')) { 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. if (delayMs > 0) Atomics.wait(renameRetrySleepBuffer, 0, 0, delayMs); delayMs = Math.min(delayMs * 2, maxDelayMs); continue; diff --git a/integrationTests/apiTests/configuration.test.mjs b/integrationTests/apiTests/configuration.test.mjs index 9c70b5ef1a..0a73183e33 100644 --- a/integrationTests/apiTests/configuration.test.mjs +++ b/integrationTests/apiTests/configuration.test.mjs @@ -348,41 +348,31 @@ suite('Configuration', (ctx) => { // Every config write fans out a re-read to each thread's root-config watcher, and the root // config is replaced by rename-over, which on Windows fails while any descriptor is open // on it (harper#2313). - let rootPath; - try { - await client - .req() - .send({ 'operation': 'set_configuration', 'burst-probe_package': 'file:./nowhere' }) - .expect(200); - for (const maxSize of ['21M', '22M', '23M', '24M']) { - await client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }).expect(200); - } - await client - .req() - .send({ operation: 'get_configuration' }) - .expect((r) => { - assert.strictEqual(r?.body?.logging?.rotation?.maxSize, '24M', r?.text); - rootPath = r?.body?.rootPath; - }) - .expect(200); - // Concurrent writes maximise the chance that a watcher read is still in flight when the - // next rename starts, which is the precondition the sequential burst cannot guarantee. - // Only the status is asserted: interleaved read-modify-writes make the last value racy. - const concurrent = await Promise.all( - ['31M', '32M', '33M', '34M'].map((maxSize) => - client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }) - ) - ); - for (const response of concurrent) assert.strictEqual(response.status, 200, response.text); - // A write that gave up would leave its temp behind, so this covers the cleanup path. - assert.deepStrictEqual( - readdirSync(rootPath).filter((entry) => entry.startsWith('harper-config.yaml.') && entry.endsWith('.tmp')), - [] - ); - } finally { - // Leaving the probe entry behind would change the config every later test observes. - await client.req().send({ 'operation': 'set_configuration', 'burst-probe_package': null }); + for (const maxSize of ['21M', '22M', '23M', '24M']) { + await client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }).expect(200); } + let rootPath; + await client + .req() + .send({ operation: 'get_configuration' }) + .expect((r) => { + assert.strictEqual(r?.body?.logging?.rotation?.maxSize, '24M', r?.text); + rootPath = r?.body?.rootPath; + }) + .expect(200); + // Concurrent writes maximise the chance that a watcher read is still in flight when the + // next rename starts, which the sequential burst cannot guarantee. Only the status is + // asserted: interleaved read-modify-writes make the surviving value racy. + const concurrent = await Promise.all( + ['31M', '32M', '33M', '34M'].map((maxSize) => + client.req().send({ operation: 'set_configuration', logging_rotation_maxSize: maxSize }) + ) + ); + for (const response of concurrent) assert.strictEqual(response.status, 200, response.text); + assert.deepStrictEqual( + readdirSync(rootPath).filter((entry) => entry.startsWith('harper-config.yaml.') && entry.endsWith('.tmp')), + [] + ); }); // ── set_configuration + replicated (#660) ─────────────────────────────── diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index 1fb8206269..bd16a28686 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -1,7 +1,7 @@ const assert = require('node:assert'); const { join } = require('node:path'); const { tmpdir } = require('node:os'); -const { mkdtempSync, writeFileSync, rmSync } = require('node:fs'); +const { mkdtempSync, writeFileSync, rmSync, mkdirSync } = require('node:fs'); const { once } = require('node:events'); const { stringify } = require('yaml'); const { RootConfigWatcher } = require('#src/config/RootConfigWatcher'); @@ -80,10 +80,45 @@ describe('root config read handle lifetime', () => { watcher.handleChange(); assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }, 'must not adopt an empty read'); - const changed = once(watcher, 'change'); + const changes = []; + watcher.on('change', (config) => changes.push(config)); writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); - const [updated] = await changed; - assert.deepStrictEqual(updated, { 'test-component': { enabled: false } }); + await once(watcher, 'change'); + // The re-read armed for the empty snapshot must not replay once a usable read lands. + await new Promise((resolve) => setTimeout(resolve, 100)); + assert.deepStrictEqual(changes, [{ 'test-component': { enabled: false } }]); + }); + + it('RootConfigWatcher stops re-reading a file that never becomes usable', async () => { + const watcher = new RootConfigWatcher(); + openWatchers.push(watcher); + await watcher.ready; + + writeFileSync(configFilePath, ''); + for (let attempt = 0; attempt < 20; attempt++) watcher.handleChange(); + await new Promise((resolve) => setTimeout(resolve, 400)); + + assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }); + }); + + it('OptionsWatcher recovers a root-config read that failed for a reason other than absence', async () => { + const watcher = new OptionsWatcher('test-component', configFilePath, undefined, true); + openWatchers.push(watcher); + await watcher.ready; + + const errors = []; + watcher.on('error', (error) => errors.push(error)); + // A directory in the file's place fails the read with EISDIR, standing in for the + // transient replace-under-us failures Windows produces; the change must not be dropped. + rmSync(configFilePath); + mkdirSync(configFilePath); + watcher._handleChangeForTests(); + assert.deepStrictEqual(errors, [], 'a recoverable read failure must not surface as an error'); + + rmSync(configFilePath, { recursive: true }); + writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); + await once(watcher, 'change'); + assert.strictEqual(watcher.get(['enabled']), false); }); it('OptionsWatcher applies a root-config change before the change handler returns', async () => { diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index be0e33e79e..5693e3b370 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -87,17 +87,23 @@ export class PartialReadRetry { #timer?: ReturnType; #remaining: number = PARTIAL_READ_MAX_REREADS; - schedule(reread: () => void) { - if (this.#timer || this.#remaining <= 0) return; + /** False once the budget is spent, so the caller can fall back to its own error handling. */ + schedule(reread: () => void): boolean { + if (this.#timer) return true; + if (this.#remaining <= 0) return false; this.#remaining--; this.#timer = setTimeout(() => { this.#timer = undefined; reread(); }, PARTIAL_READ_REREAD_DELAY_MS); this.#timer.unref?.(); + return true; } + /** A usable read arrived, so any re-read still armed for the previous one would duplicate it. */ settled() { + if (this.#timer) clearTimeout(this.#timer); + this.#timer = undefined; this.#remaining = PARTIAL_READ_MAX_REREADS; } @@ -107,3 +113,20 @@ export class PartialReadRetry { this.#remaining = 0; } } + +/** + * A missing file is not a half-written one: it has unambiguous, already-correct semantics in + * both watchers (env-only fallback at boot, `remove` afterwards), and re-reading it would only + * delay them. + */ +export function isPartialReadError(error: unknown): boolean { + return !(typeof error === 'object' && error !== null && (error as { code?: string }).code === 'ENOENT'); +} + +/** + * A watcher that exhausts its re-read budget serves stale config from then on, so the give-up + * has to be visible — otherwise the only symptom is a config change that silently did nothing. + */ +export function warnPartialReadGaveUp(filePath: string) { + fallbackLogger.warn(`Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} incomplete reads`); +} From bf582c3eb9c273aeedffacd1d7cb0590aad923fd Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 07:49:24 -0600 Subject: [PATCH 07/17] fix(config): keep listener errors on the watcher's error route, not the retry Round-3 review findings. Applying parsed config is past the point where an incomplete file explains a failure, so a throw from a change listener took the two paths in opposite and both-wrong directions: it escaped the root watcher into chokidar's callback, and on the application path it was misread as a partial file and replayed ten times. Both now keep the watcher's established error route, and RootConfigWatcher logs rather than swallowing. An unusable read is also recognised by value rather than by length: '', a lone newline and a truncated document all parse to null, and adopting that dropped the whole configuration - for a scope, via the remove that restarts it. Also: the give-up warning is once per file rather than once per watcher per event, since every root-config scope watches the same file, and the ENOENT exclusion says what it means - both watchers already answer a missing file deliberately, so re-reading only delays that. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 57 +++++++++------- config/RootConfigWatcher.ts | 32 +++++---- .../config/configReadHandleLifetime.test.js | 39 ++++++++++- unitTests/utility/partialReadRetry.test.js | 65 +++++++++++++++++++ utility/watcherFallback.ts | 23 +++++-- 5 files changed, 174 insertions(+), 42 deletions(-) create mode 100644 unitTests/utility/partialReadRetry.test.js diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 13c7b6e1b6..98a6af52c4 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -142,39 +142,48 @@ export class OptionsWatcher extends EventEmitter { // invariant on atomicWriteFile. Application configs keep the non-blocking read and its drain. #handleChange() { if (this.#isRootConfig) { - let parsed; - try { - const contents = readFileSync(this.#filePath, 'utf-8'); - if (!contents) return this.#rereadOr(undefined); - parsed = this.#parseContents(contents); - } catch (error) { - // A read or parse that fails on a file being replaced under us is the same event - // as an empty one, and the ENOENT arm of #handleReadError would answer it with a - // `remove` that restarts the scope. Re-read first; only a budget that runs out - // means the failure is real. - return this.#rereadOr(error); - } - this.#partialRead.settled(); - this.#applyParsed(parsed); + this.#applyRead(() => readFileSync(this.#filePath, 'utf-8')); return; } const read: Promise = readFile(this.#filePath, 'utf-8') - .then((contents) => { - // Application configs are rewritten in place, so a mid-write read is exactly the - // case above, and dropping it here would `remove` the scope's config. - if (!contents) return this.#rereadOr(undefined); - const parsed = this.#parseContents(contents); - this.#partialRead.settled(); - this.#applyParsed(parsed); - }) - .catch((error) => this.#rereadOr(error)) + // Application configs are rewritten in place, so they see the half-written snapshots + // the retry exists for; only the read itself differs between the two paths. + .then((contents) => this.#applyRead(() => contents)) + .catch((error) => this.#recoverOrReport(error)) .finally(() => { this.#pendingReads.delete(read); }); this.#pendingReads.add(read); } - #rereadOr(error: unknown) { + #applyRead(read: () => string) { + let parsed; + try { + parsed = this.#parseContents(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; + } + // `''`, `'\n'` and a truncated document all parse to null, and adopting that would drop + // the scope's config and emit `remove`. + if (!parsed || typeof parsed !== 'object') { + this.#recoverOrReport(undefined); + return; + } + this.#partialRead.settled(); + try { + this.#applyParsed(parsed); + } catch (error) { + // Applying is past the point where an incomplete file is a possible explanation, so + // a listener's throw keeps the watcher's established error route rather than a retry. + this.emit('error', error); + } + } + + #recoverOrReport(error: unknown) { if (error !== undefined && !isPartialReadError(error)) return this.#handleReadError(error); if (this.#partialRead.schedule(() => this.#handleChange())) return; if (error === undefined) { diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 297b7250cd..9a551599b0 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -10,6 +10,7 @@ import { isWatcherExhaustionError, warnPartialReadGaveUp, warnWatcherFallback, + warnWatcherListenerError, } from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; @@ -96,26 +97,33 @@ export class RootConfigWatcher extends EventEmitter { // Only the read and parse are guarded: a listener that throws must not be mistaken for a // half-written file and replayed. try { - const data = readFileSync(this.#configFilePath, 'utf-8'); - if (!data) { - this.#scheduleReread(); - return; - } - config = parse(data); + 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(); return; } - this.#partialRead.settled(); - - if (!this.#config) { - this.#config = config; - this.emit('ready', this.#config); + // 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 + // drop the whole configuration. + if (!config || typeof config !== 'object') { + this.#scheduleReread(); return; } + this.#partialRead.settled(); - this.emit('change', (this.#config = config)); + try { + if (!this.#config) { + this.#config = config; + this.emit('ready', this.#config); + return; + } + this.emit('change', (this.#config = config)); + } catch (error) { + // The watcher's own state is already updated; a listener's bug must not be replayed + // as if the file were incomplete, nor escape into the chokidar callback. + warnWatcherListenerError(this.#configFilePath, error); + } } #scheduleReread() { diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index bd16a28686..75ea673bfe 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -84,9 +84,13 @@ describe('root config read handle lifetime', () => { watcher.on('change', (config) => changes.push(config)); writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); await once(watcher, 'change'); - // The re-read armed for the empty snapshot must not replay once a usable read lands. await new Promise((resolve) => setTimeout(resolve, 100)); - assert.deepStrictEqual(changes, [{ 'test-component': { enabled: false } }]); + // The count is not pinned: the armed re-read and chokidar's own event for the same write + // race, and this watcher has never diffed. What must hold is that no emit carries the + // half-written snapshot. + assert.ok(changes.length > 0); + for (const config of changes) assert.deepStrictEqual(config, { 'test-component': { enabled: false } }); + assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: false } }); }); it('RootConfigWatcher stops re-reading a file that never becomes usable', async () => { @@ -132,6 +136,37 @@ describe('root config read handle lifetime', () => { assert.strictEqual(watcher.get(['enabled']), false); }); + it('OptionsWatcher recovers an application config observed mid-write instead of removing it', async () => { + const appConfigPath = join(fixture, 'config.yaml'); + writeFileSync(appConfigPath, stringify({ 'test-component': { enabled: true } })); + const watcher = new OptionsWatcher('test-component', appConfigPath, undefined, false); + openWatchers.push(watcher); + await watcher.ready; + + const removes = []; + watcher.on('remove', () => removes.push(true)); + writeFileSync(appConfigPath, ''); + watcher._handleChangeForTests(); + await new Promise((resolve) => setImmediate(resolve)); + assert.deepStrictEqual(removes, [], 'a half-written read must not read as the scope being removed'); + assert.strictEqual(watcher.get(['enabled']), true); + + writeFileSync(appConfigPath, stringify({ 'test-component': { enabled: false } })); + await once(watcher, 'change'); + assert.strictEqual(watcher.get(['enabled']), false); + }); + + it('RootConfigWatcher treats a document that parses to nothing as incomplete', async () => { + const watcher = new RootConfigWatcher(); + openWatchers.push(watcher); + await watcher.ready; + + // A truncated write can leave a document that reads fine and parses to null. + writeFileSync(configFilePath, '\n'); + watcher.handleChange(); + assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }); + }); + it('OptionsWatcher still reads an application config without blocking', async () => { // Application configs are written in place, never by rename-over, so they must keep the // non-blocking read — a slow or stalled component-config volume must not stall the thread. diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js new file mode 100644 index 0000000000..50d891a23b --- /dev/null +++ b/unitTests/utility/partialReadRetry.test.js @@ -0,0 +1,65 @@ +const assert = require('node:assert'); +const { PartialReadRetry } = require('#src/utility/watcherFallback'); + +const settle = (ms = 60) => new Promise((resolve) => setTimeout(resolve, ms)); + +describe('PartialReadRetry', () => { + it('re-reads once per unusable read, not once per event', async () => { + const retry = new PartialReadRetry(); + let rereads = 0; + const reread = () => rereads++; + + assert.strictEqual(retry.schedule(reread), true); + assert.strictEqual(retry.schedule(reread), true, 'a second event must join the armed re-read'); + await settle(); + + assert.strictEqual(rereads, 1); + }); + + it('cancels an armed re-read once a usable read arrives, so it cannot replay', async () => { + const retry = new PartialReadRetry(); + let rereads = 0; + + retry.schedule(() => rereads++); + retry.settled(); + await settle(); + + assert.strictEqual(rereads, 0); + }); + + it('reports exhaustion so the caller can fall back to its own error handling', async () => { + const retry = new PartialReadRetry(); + let scheduled = 0; + while (retry.schedule(() => {})) { + scheduled++; + await settle(30); + assert.ok(scheduled < 50, 'the budget must be bounded'); + } + assert.strictEqual( + retry.schedule(() => {}), + false + ); + + // A usable read restores the budget for the next incident. + retry.settled(); + assert.strictEqual( + retry.schedule(() => {}), + true + ); + }); + + it('stops re-reading after close', async () => { + const retry = new PartialReadRetry(); + let rereads = 0; + + retry.schedule(() => rereads++); + retry.cancel(); + await settle(); + + assert.strictEqual(rereads, 0); + assert.strictEqual( + retry.schedule(() => rereads++), + false + ); + }); +}); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 5693e3b370..958ebdb3de 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -10,6 +10,7 @@ import { loggerWithTag } from './logging/harper_logger.ts'; // One-time process-wide warning so a thundering herd of failing watchers doesn't // produce hundreds of identical log lines. let exhaustionWarned = false; +const partialReadWarned = new Set(); const fallbackLogger = loggerWithTag('watcher'); @@ -115,9 +116,9 @@ export class PartialReadRetry { } /** - * A missing file is not a half-written one: it has unambiguous, already-correct semantics in - * both watchers (env-only fallback at boot, `remove` afterwards), and re-reading it would only - * delay them. + * ENOENT is excluded not because it cannot be transient, but because both watchers already + * answer it deliberately (env-only fallback at boot, `remove` afterwards); re-reading would + * only delay a decision they have already made. */ export function isPartialReadError(error: unknown): boolean { return !(typeof error === 'object' && error !== null && (error as { code?: string }).code === 'ENOENT'); @@ -126,7 +127,21 @@ export function isPartialReadError(error: unknown): boolean { /** * A watcher that exhausts its re-read budget serves stale config from then on, so the give-up * has to be visible — otherwise the only symptom is a config change that silently did nothing. + * Warned once per file: every root-config scope watches the same one and would otherwise report + * a single bad file once each, on every event. */ export function warnPartialReadGaveUp(filePath: string) { - fallbackLogger.warn(`Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} incomplete reads`); + if (partialReadWarned.has(filePath)) return; + partialReadWarned.add(filePath); + fallbackLogger.warn( + `Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} reads that were empty or incomplete; continuing with the last usable configuration` + ); +} + +/** + * A listener that throws while applying new config is a bug in that listener, not evidence the + * file was half-written — the watcher's own state is already updated, so it keeps going. + */ +export function warnWatcherListenerError(filePath: string, error: unknown) { + fallbackLogger.warn(`Error applying a configuration change from ${filePath}`, error); } From af6933f8d950063b62abe5768c37bd55b8de6a19 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:00:41 -0600 Subject: [PATCH 08/17] fix(config): judge a root config complete before the env overlay, not after Round-4 review findings. overlayRootEnvConfig turns any parse into a non-null object whenever a config env var is set - the norm in containers - so the completeness check ran on the overlaid value and passed. An empty or half-written root config was laundered into a valid-looking env-only object and adopted: a scope absent from the env config got a remove that restarts it, and one present in it had every file-provided key merged away first. The check now runs on the file's own parse. A file still unusable once the retry budget is spent is taken at face value, so emptying a config file reaches remove as it always did rather than being classified incomplete forever. Also: the give-up warning names the parse error when there was one, instead of reporting a syntax error as an empty read; the retry helper's tests wait on the condition rather than a fixed sleep; and the application-config test awaits the read it asserts on, which it previously could race. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 24 ++++++----- config/RootConfigWatcher.ts | 8 ++-- .../config/configReadHandleLifetime.test.js | 28 +++++++++++- unitTests/utility/partialReadRetry.test.js | 43 ++++++++++++------- utility/watcherFallback.ts | 9 ++-- 5 files changed, 76 insertions(+), 36 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 98a6af52c4..50f9ec55fa 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -159,7 +159,7 @@ export class OptionsWatcher extends EventEmitter { #applyRead(read: () => string) { let parsed; try { - parsed = this.#parseContents(read()); + 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` @@ -167,15 +167,17 @@ export class OptionsWatcher extends EventEmitter { this.#recoverOrReport(error); return; } - // `''`, `'\n'` and a truncated document all parse to null, and adopting that would drop - // the scope's config and emit `remove`. + // 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') { - this.#recoverOrReport(undefined); - return; + if (this.#partialRead.schedule(() => this.#handleChange())) return; + warnPartialReadGaveUp(this.#filePath); } this.#partialRead.settled(); try { - this.#applyParsed(parsed); + this.#applyParsed(this.#overlayEnvConfig(parsed)); } catch (error) { // Applying is past the point where an incomplete file is a possible explanation, so // a listener's throw keeps the watcher's established error route rather than a retry. @@ -193,8 +195,7 @@ export class OptionsWatcher extends EventEmitter { this.#handleReadError(error); } - #parseContents(contents: string) { - let parsed = yaml.parse(contents); + #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 @@ -202,8 +203,7 @@ export class OptionsWatcher extends EventEmitter { // 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); - return parsed; + return this.#isRootConfig ? overlayRootEnvConfig(parsed) : parsed; } #applyParsed(parsed: unknown) { @@ -452,8 +452,10 @@ export class OptionsWatcher extends EventEmitter { // 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. - _handleChangeForTests(): void { + // Resolves once the read has landed, which for the root config has already happened. + _handleChangeForTests(): Promise { this.#handleChange(); + return Promise.allSettled([...this.#pendingReads]); } // Test-only: simulate the underlying chokidar watcher emitting an error. diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 9a551599b0..8ba978179b 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -100,12 +100,12 @@ export class RootConfigWatcher extends EventEmitter { 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(); + 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 - // drop the whole configuration. + // hand every consumer a config with nothing in it. if (!config || typeof config !== 'object') { this.#scheduleReread(); return; @@ -126,9 +126,9 @@ export class RootConfigWatcher extends EventEmitter { } } - #scheduleReread() { + #scheduleReread(error?: unknown) { if (this.#partialRead.schedule(() => this.handleChange())) return; - warnPartialReadGaveUp(this.#configFilePath); + warnPartialReadGaveUp(this.#configFilePath, error); } close() { diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index 75ea673bfe..503ebc77eb 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -22,6 +22,7 @@ describe('root config read handle lifetime', () => { let fixture; let configFilePath; let previousRootPath; + let previousSetConfig; const openWatchers = []; beforeEach(() => { @@ -29,6 +30,7 @@ describe('root config read handle lifetime', () => { configFilePath = join(fixture, HARPER_CONFIG_FILE); writeFileSync(configFilePath, stringify({ 'test-component': { enabled: true } })); previousRootPath = process.env.ROOTPATH; + previousSetConfig = process.env.HARPER_SET_CONFIG; process.env.ROOTPATH = fixture; }); @@ -36,6 +38,8 @@ describe('root config read handle lifetime', () => { await Promise.all(openWatchers.splice(0).map((watcher) => watcher.close())); if (previousRootPath === undefined) delete process.env.ROOTPATH; else process.env.ROOTPATH = previousRootPath; + if (previousSetConfig === undefined) delete process.env.HARPER_SET_CONFIG; + else process.env.HARPER_SET_CONFIG = previousSetConfig; rmSync(fixture, { recursive: true, force: true }); }); @@ -146,8 +150,9 @@ describe('root config read handle lifetime', () => { const removes = []; watcher.on('remove', () => removes.push(true)); writeFileSync(appConfigPath, ''); - watcher._handleChangeForTests(); - await new Promise((resolve) => setImmediate(resolve)); + // Awaited, or the assertions below would also pass against a build with no partial-read + // handling at all on this path, having asserted on a read that had not happened. + await watcher._handleChangeForTests(); assert.deepStrictEqual(removes, [], 'a half-written read must not read as the scope being removed'); assert.strictEqual(watcher.get(['enabled']), true); @@ -167,6 +172,25 @@ describe('root config read handle lifetime', () => { assert.deepStrictEqual(watcher.config, { 'test-component': { enabled: true } }); }); + it('OptionsWatcher does not let an env overlay launder a half-written root config', async () => { + // overlayRootEnvConfig turns any parse into a non-null object whenever a config env var is + // set — the norm in containers — so completeness has to be judged on the file's own parse + // or an empty read is adopted as an env-only config and the file's options are wiped. + process.env.HARPER_SET_CONFIG = JSON.stringify({ 'other-component': { enabled: true } }); + const watcher = new OptionsWatcher('test-component', configFilePath, undefined, true); + openWatchers.push(watcher); + await watcher.ready; + assert.strictEqual(watcher.get(['enabled']), true); + + const removes = []; + watcher.on('remove', () => removes.push(true)); + writeFileSync(configFilePath, ''); + await watcher._handleChangeForTests(); + + assert.deepStrictEqual(removes, [], 'the env overlay must not stand in for the half-written file'); + assert.strictEqual(watcher.get(['enabled']), true); + }); + it('OptionsWatcher still reads an application config without blocking', async () => { // Application configs are written in place, never by rename-over, so they must keep the // non-blocking read — a slow or stalled component-config volume must not stall the thread. diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js index 50d891a23b..967f26e21f 100644 --- a/unitTests/utility/partialReadRetry.test.js +++ b/unitTests/utility/partialReadRetry.test.js @@ -1,18 +1,28 @@ const assert = require('node:assert'); const { PartialReadRetry } = require('#src/utility/watcherFallback'); +const { waitFor } = require('../waitFor'); -const settle = (ms = 60) => new Promise((resolve) => setTimeout(resolve, ms)); +// The retry is a timer, so "it fired" is a condition to wait for; "it fired only once" is a +// non-event, which is the one case AGENTS.md reserves a fixed settle for. +const settle = () => new Promise((resolve) => setTimeout(resolve, 100)); describe('PartialReadRetry', () => { - it('re-reads once per unusable read, not once per event', async () => { + it('re-reads once for a burst of unusable reads, not once per event', async () => { const retry = new PartialReadRetry(); let rereads = 0; - const reread = () => rereads++; - assert.strictEqual(retry.schedule(reread), true); - assert.strictEqual(retry.schedule(reread), true, 'a second event must join the armed re-read'); - await settle(); + assert.strictEqual( + retry.schedule(() => rereads++), + true + ); + assert.strictEqual( + retry.schedule(() => rereads++), + true, + 'a second event joins the armed re-read' + ); + await waitFor(() => rereads > 0, { message: 'the re-read never fired' }); + await settle(); assert.strictEqual(rereads, 1); }); @@ -22,28 +32,31 @@ describe('PartialReadRetry', () => { retry.schedule(() => rereads++); retry.settled(); - await settle(); + await settle(); assert.strictEqual(rereads, 0); }); it('reports exhaustion so the caller can fall back to its own error handling', async () => { const retry = new PartialReadRetry(); - let scheduled = 0; - while (retry.schedule(() => {})) { - scheduled++; - await settle(30); - assert.ok(scheduled < 50, 'the budget must be bounded'); + let rereads = 0; + // Re-arm only after each timer has actually fired, so this counts budget rather than + // racing the timer that is still armed (schedule() reports true for both). + for (let attempt = 1; retry.schedule(() => rereads++); attempt++) { + assert.ok(attempt <= 50, 'the budget must be bounded'); + await waitFor(() => rereads === attempt, { message: `re-read ${attempt} never fired` }); } + + assert.ok(rereads > 0, 'the budget must allow at least one re-read'); assert.strictEqual( - retry.schedule(() => {}), + retry.schedule(() => rereads++), false ); // A usable read restores the budget for the next incident. retry.settled(); assert.strictEqual( - retry.schedule(() => {}), + retry.schedule(() => rereads++), true ); }); @@ -54,8 +67,8 @@ describe('PartialReadRetry', () => { retry.schedule(() => rereads++); retry.cancel(); - await settle(); + await settle(); assert.strictEqual(rereads, 0); assert.strictEqual( retry.schedule(() => rereads++), diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 958ebdb3de..43dc0869c7 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -130,12 +130,13 @@ export function isPartialReadError(error: unknown): boolean { * Warned once per file: every root-config scope watches the same one and would otherwise report * a single bad file once each, on every event. */ -export function warnPartialReadGaveUp(filePath: string) { +export function warnPartialReadGaveUp(filePath: string, error?: unknown) { if (partialReadWarned.has(filePath)) return; partialReadWarned.add(filePath); - fallbackLogger.warn( - `Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} reads that were empty or incomplete; continuing with the last usable configuration` - ); + // The cause matters to whoever has to fix it: a file that never parses is a typo to correct, + // while one that reads empty is a writer that never finished. + const cause = error ? `: ${(error as Error).message ?? error}` : ' that were empty or incomplete'; + fallbackLogger.warn(`Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} unusable reads${cause}`); } /** From 5a85ca13014e59b5abcfe92f79431963a68fc036 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:09:32 -0600 Subject: [PATCH 09/17] fix(config): re-arm the give-up warning when a config file recovers The per-file throttle that stops one bad root config producing one warning per scope never cleared, so a file that was fixed and later broken again failed silently: the path stayed in the suppression set for the life of the process. A usable read now clears it, since that is a new incident. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 11 +++++------ config/RootConfigWatcher.ts | 5 ++--- unitTests/utility/partialReadRetry.test.js | 21 ++++++++++++++++----- utility/watcherFallback.ts | 13 +++++++++++++ 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 50f9ec55fa..393310232e 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -104,7 +104,7 @@ export class OptionsWatcher extends EventEmitter { #closed: boolean; #openCount: number = 0; #pendingReads: Set> = new Set(); - #partialRead = new PartialReadRetry(); + #partialRead: PartialReadRetry; ready: Promise; constructor(name: string, filePath: string, logger?: Logger, isRootConfig?: boolean) { @@ -113,6 +113,7 @@ export class OptionsWatcher extends EventEmitter { 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. @@ -139,15 +140,13 @@ export class OptionsWatcher extends EventEmitter { } // The root config reads synchronously so its descriptor cannot outlive this turn - see the - // invariant on atomicWriteFile. Application configs keep the non-blocking read and its drain. + // invariant on atomicWriteFile. #handleChange() { if (this.#isRootConfig) { this.#applyRead(() => readFileSync(this.#filePath, 'utf-8')); return; } const read: Promise = readFile(this.#filePath, 'utf-8') - // Application configs are rewritten in place, so they see the half-written snapshots - // the retry exists for; only the read itself differs between the two paths. .then((contents) => this.#applyRead(() => contents)) .catch((error) => this.#recoverOrReport(error)) .finally(() => { @@ -179,8 +178,8 @@ export class OptionsWatcher extends EventEmitter { try { this.#applyParsed(this.#overlayEnvConfig(parsed)); } catch (error) { - // Applying is past the point where an incomplete file is a possible explanation, so - // a listener's throw keeps the watcher's established error route rather than a retry. + // 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); } } diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 8ba978179b..69b6981c0e 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -22,7 +22,7 @@ export class RootConfigWatcher extends EventEmitter { #usingPolling: boolean; #closed: boolean; #openCount: number = 0; - #partialRead = new PartialReadRetry(); + #partialRead: PartialReadRetry; ready: Promise; constructor() { @@ -30,6 +30,7 @@ export class RootConfigWatcher extends EventEmitter { 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'); @@ -120,8 +121,6 @@ export class RootConfigWatcher extends EventEmitter { } this.emit('change', (this.#config = config)); } catch (error) { - // The watcher's own state is already updated; a listener's bug must not be replayed - // as if the file were incomplete, nor escape into the chokidar callback. warnWatcherListenerError(this.#configFilePath, error); } } diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js index 967f26e21f..3a42a6b378 100644 --- a/unitTests/utility/partialReadRetry.test.js +++ b/unitTests/utility/partialReadRetry.test.js @@ -1,5 +1,5 @@ const assert = require('node:assert'); -const { PartialReadRetry } = require('#src/utility/watcherFallback'); +const { PartialReadRetry, warnPartialReadGaveUp, isPartialReadWarned } = require('#src/utility/watcherFallback'); const { waitFor } = require('../waitFor'); // The retry is a timer, so "it fired" is a condition to wait for; "it fired only once" is a @@ -8,7 +8,7 @@ const settle = () => new Promise((resolve) => setTimeout(resolve, 100)); describe('PartialReadRetry', () => { it('re-reads once for a burst of unusable reads, not once per event', async () => { - const retry = new PartialReadRetry(); + const retry = new PartialReadRetry('/nonexistent/config.yaml'); let rereads = 0; assert.strictEqual( @@ -27,7 +27,7 @@ describe('PartialReadRetry', () => { }); it('cancels an armed re-read once a usable read arrives, so it cannot replay', async () => { - const retry = new PartialReadRetry(); + const retry = new PartialReadRetry('/nonexistent/config.yaml'); let rereads = 0; retry.schedule(() => rereads++); @@ -38,7 +38,7 @@ describe('PartialReadRetry', () => { }); it('reports exhaustion so the caller can fall back to its own error handling', async () => { - const retry = new PartialReadRetry(); + const retry = new PartialReadRetry('/nonexistent/config.yaml'); let rereads = 0; // Re-arm only after each timer has actually fired, so this counts budget rather than // racing the timer that is still armed (schedule() reports true for both). @@ -61,8 +61,19 @@ describe('PartialReadRetry', () => { ); }); + it('re-arms the give-up warning once the file recovers', async () => { + const retry = new PartialReadRetry('/nonexistent/recovering.yaml'); + // The warning is throttled per file so one bad config cannot produce one line per scope, + // but a file that recovers and later breaks again is a new incident. + warnPartialReadGaveUp('/nonexistent/recovering.yaml'); + assert.strictEqual(isPartialReadWarned('/nonexistent/recovering.yaml'), true); + + retry.settled(); + assert.strictEqual(isPartialReadWarned('/nonexistent/recovering.yaml'), false); + }); + it('stops re-reading after close', async () => { - const retry = new PartialReadRetry(); + const retry = new PartialReadRetry('/nonexistent/config.yaml'); let rereads = 0; retry.schedule(() => rereads++); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 43dc0869c7..a055c5a9c2 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -85,9 +85,14 @@ export function _resetForTests(): void { * spin. Callers read synchronously, so the descriptor still never outlives a single turn. */ export class PartialReadRetry { + #filePath: string; #timer?: ReturnType; #remaining: number = PARTIAL_READ_MAX_REREADS; + constructor(filePath: string) { + this.#filePath = filePath; + } + /** False once the budget is spent, so the caller can fall back to its own error handling. */ schedule(reread: () => void): boolean { if (this.#timer) return true; @@ -106,6 +111,9 @@ export class PartialReadRetry { if (this.#timer) clearTimeout(this.#timer); this.#timer = undefined; this.#remaining = PARTIAL_READ_MAX_REREADS; + // The file recovered, so the next time it breaks is a new incident and has to be reported + // again rather than silenced by the warning it emitted weeks ago. + partialReadWarned.delete(this.#filePath); } cancel() { @@ -139,6 +147,11 @@ export function warnPartialReadGaveUp(filePath: string, error?: unknown) { fallbackLogger.warn(`Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} unusable reads${cause}`); } +/** Test-only: whether a give-up warning for this file is currently suppressed as a duplicate. */ +export function isPartialReadWarned(filePath: string): boolean { + return partialReadWarned.has(filePath); +} + /** * A listener that throws while applying new config is a bug in that listener, not evidence the * file was half-written — the watcher's own state is already updated, so it keeps going. From 365d597a63e857bc11f83ab9e4bfef57568f31fb Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:16:04 -0600 Subject: [PATCH 10/17] fix(config): record a give-up after clearing the gate, not before settled() clears the file's warning gate, so warning first and settling second deleted the record immediately and let every other root scope report the same file again - defeating the throttle it was added for. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 6 ++++- .../config/configReadHandleLifetime.test.js | 27 +++++++++++++++++++ utility/watcherFallback.ts | 5 ++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 393310232e..6b3f7ee098 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -170,11 +170,15 @@ export class OptionsWatcher extends EventEmitter { // 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`. + let gaveUp = false; if (!parsed || typeof parsed !== 'object') { if (this.#partialRead.schedule(() => this.#handleChange())) return; - warnPartialReadGaveUp(this.#filePath); + gaveUp = true; } + // settled() clears this file's warning gate, so the give-up has to be recorded after it or + // every other root scope would report the same file again. this.#partialRead.settled(); + if (gaveUp) warnPartialReadGaveUp(this.#filePath); try { this.#applyParsed(this.#overlayEnvConfig(parsed)); } catch (error) { diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index 503ebc77eb..547449b544 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -3,6 +3,8 @@ const { join } = require('node:path'); const { tmpdir } = require('node:os'); const { mkdtempSync, writeFileSync, rmSync, mkdirSync } = require('node:fs'); const { once } = require('node:events'); +const { waitFor } = require('../waitFor'); +const { isPartialReadWarned, clearPartialReadWarning } = require('#src/utility/watcherFallback'); const { stringify } = require('yaml'); const { RootConfigWatcher } = require('#src/config/RootConfigWatcher'); const { OptionsWatcher } = require('#src/components/OptionsWatcher'); @@ -191,6 +193,31 @@ describe('root config read handle lifetime', () => { assert.strictEqual(watcher.get(['enabled']), true); }); + it('OptionsWatcher reports a file it gave up on once, not once per scope', async () => { + clearPartialReadWarning(configFilePath); + // Both scopes must be present in the file, or their `ready` never fires. + writeFileSync( + configFilePath, + stringify({ 'test-component': { enabled: true }, 'other-component': { enabled: true } }) + ); + const watchers = ['test-component', 'other-component'].map((name) => { + const watcher = new OptionsWatcher(name, configFilePath, undefined, true); + openWatchers.push(watcher); + return watcher; + }); + await Promise.all(watchers.map((watcher) => watcher.ready)); + + writeFileSync(configFilePath, ''); + for (const watcher of watchers) { + // Drive each watcher past its own retry budget, as a real unusable file would. + for (let attempt = 0; attempt <= 12; attempt++) await watcher._handleChangeForTests(); + await waitFor(() => isPartialReadWarned(configFilePath), { message: 'the give-up was never reported' }); + } + + // One bad root config must not produce one warning per scope watching it. + assert.strictEqual(isPartialReadWarned(configFilePath), true); + }); + it('OptionsWatcher still reads an application config without blocking', async () => { // Application configs are written in place, never by rename-over, so they must keep the // non-blocking read — a slow or stalled component-config volume must not stall the thread. diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index a055c5a9c2..cc17443dc6 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -152,6 +152,11 @@ export function isPartialReadWarned(filePath: string): boolean { return partialReadWarned.has(filePath); } +/** Test-only: forget that this file was reported, so a suite can start from a known state. */ +export function clearPartialReadWarning(filePath: string) { + partialReadWarned.delete(filePath); +} + /** * A listener that throws while applying new config is a bug in that listener, not evidence the * file was half-written — the watcher's own state is already updated, so it keeps going. From a3af4451290eea93a34729200c7cce58da54ee2e Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:24:53 -0600 Subject: [PATCH 11/17] fix(config): report a give-up once per file across every scope watching it The warning gate is shared per file, so treating a give-up like a recovery let each of the N root-config scopes clear it and report the same file in turn - which is what the throttle existed to prevent. Give-up and recovery are now distinct operations: only a usable read withdraws the report. The throttle also reports whether it warned, so the property is observable in a test rather than only in the log. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 12 +++------ config/RootConfigWatcher.ts | 3 +-- .../config/configReadHandleLifetime.test.js | 4 +-- unitTests/utility/partialReadRetry.test.js | 26 ++++++++++++++++++- utility/watcherFallback.ts | 19 ++++++++++---- 5 files changed, 46 insertions(+), 18 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 6b3f7ee098..9bbd6e4c8d 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -13,7 +13,6 @@ import { PartialReadRetry, isPartialReadError, isWatcherExhaustionError, - warnPartialReadGaveUp, warnWatcherFallback, } from '../utility/watcherFallback.ts'; import { resolveWatchTarget } from '../utility/watchPath.ts'; @@ -170,15 +169,12 @@ export class OptionsWatcher extends EventEmitter { // 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`. - let gaveUp = false; if (!parsed || typeof parsed !== 'object') { if (this.#partialRead.schedule(() => this.#handleChange())) return; - gaveUp = true; + this.#partialRead.gaveUp(); + } else { + this.#partialRead.settled(); } - // settled() clears this file's warning gate, so the give-up has to be recorded after it or - // every other root scope would report the same file again. - this.#partialRead.settled(); - if (gaveUp) warnPartialReadGaveUp(this.#filePath); try { this.#applyParsed(this.#overlayEnvConfig(parsed)); } catch (error) { @@ -192,7 +188,7 @@ export class OptionsWatcher extends EventEmitter { if (error !== undefined && !isPartialReadError(error)) return this.#handleReadError(error); if (this.#partialRead.schedule(() => this.#handleChange())) return; if (error === undefined) { - warnPartialReadGaveUp(this.#filePath); + this.#partialRead.gaveUp(); return; } this.#handleReadError(error); diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 69b6981c0e..1d82df1cf4 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -8,7 +8,6 @@ import { PartialReadRetry, isPartialReadError, isWatcherExhaustionError, - warnPartialReadGaveUp, warnWatcherFallback, warnWatcherListenerError, } from '../utility/watcherFallback.ts'; @@ -127,7 +126,7 @@ export class RootConfigWatcher extends EventEmitter { #scheduleReread(error?: unknown) { if (this.#partialRead.schedule(() => this.handleChange())) return; - warnPartialReadGaveUp(this.#configFilePath, error); + this.#partialRead.gaveUp(error); } close() { diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index 547449b544..2734dce963 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -213,8 +213,8 @@ describe('root config read handle lifetime', () => { for (let attempt = 0; attempt <= 12; attempt++) await watcher._handleChangeForTests(); await waitFor(() => isPartialReadWarned(configFilePath), { message: 'the give-up was never reported' }); } - - // One bad root config must not produce one warning per scope watching it. + // The per-file gate that suppresses the duplicate report is pinned directly in + // unitTests/utility/partialReadRetry.test.js; this covers both scopes reaching it at all. assert.strictEqual(isPartialReadWarned(configFilePath), true); }); diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js index 3a42a6b378..b82d92a24e 100644 --- a/unitTests/utility/partialReadRetry.test.js +++ b/unitTests/utility/partialReadRetry.test.js @@ -1,5 +1,10 @@ const assert = require('node:assert'); -const { PartialReadRetry, warnPartialReadGaveUp, isPartialReadWarned } = require('#src/utility/watcherFallback'); +const { + PartialReadRetry, + warnPartialReadGaveUp, + isPartialReadWarned, + clearPartialReadWarning, +} = require('#src/utility/watcherFallback'); const { waitFor } = require('../waitFor'); // The retry is a timer, so "it fired" is a condition to wait for; "it fired only once" is a @@ -72,6 +77,25 @@ describe('PartialReadRetry', () => { assert.strictEqual(isPartialReadWarned('/nonexistent/recovering.yaml'), false); }); + it('keeps the report standing when it gives up, and withdraws it only on recovery', async () => { + // The gate is shared per file, so treating a give-up like a recovery would let each of the + // N scopes watching one root config report the same file in turn. + const path = '/nonexistent/shared.yaml'; + clearPartialReadWarning(path); + const retry = new PartialReadRetry(path); + + assert.strictEqual(retry.gaveUp(), true, 'the first give-up is the one that reports'); + assert.strictEqual( + new PartialReadRetry(path).gaveUp(), + false, + 'another scope giving up on the same file must be suppressed, not reported again' + ); + + retry.settled(); + assert.strictEqual(isPartialReadWarned(path), false, 'a usable read is what withdraws the report'); + assert.strictEqual(new PartialReadRetry(path).gaveUp(), true, 'so the next incident reports again'); + }); + it('stops re-reading after close', async () => { const retry = new PartialReadRetry('/nonexistent/config.yaml'); let rereads = 0; diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index cc17443dc6..0cbd815a14 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -33,9 +33,6 @@ export function isWatcherExhaustionError(error: unknown): boolean { * host is already under resource pressure. A second-scale interval keeps CPU * cost bounded; the alternative is to lose change events entirely. */ -const PARTIAL_READ_REREAD_DELAY_MS = 20; -const PARTIAL_READ_MAX_REREADS = 10; - export const POLLING_FALLBACK_OPTIONS = { usePolling: true, interval: 1000, @@ -84,6 +81,9 @@ export function _resetForTests(): void { * file. Re-read on a later turn instead, bounded so a genuinely empty or corrupt file cannot * spin. Callers read synchronously, so the descriptor still never outlives a single turn. */ +const PARTIAL_READ_REREAD_DELAY_MS = 20; +const PARTIAL_READ_MAX_REREADS = 10; + export class PartialReadRetry { #filePath: string; #timer?: ReturnType; @@ -116,6 +116,14 @@ export class PartialReadRetry { partialReadWarned.delete(this.#filePath); } + /** + * The budget is spent. Distinct from `settled()`: the file has not recovered, so the report + * stands for every other watcher of it. Returns whether this give-up was the one reported. + */ + gaveUp(error?: unknown): boolean { + return warnPartialReadGaveUp(this.#filePath, error); + } + cancel() { if (this.#timer) clearTimeout(this.#timer); this.#timer = undefined; @@ -138,13 +146,14 @@ export function isPartialReadError(error: unknown): boolean { * Warned once per file: every root-config scope watches the same one and would otherwise report * a single bad file once each, on every event. */ -export function warnPartialReadGaveUp(filePath: string, error?: unknown) { - if (partialReadWarned.has(filePath)) return; +export function warnPartialReadGaveUp(filePath: string, error?: unknown): boolean { + if (partialReadWarned.has(filePath)) return false; partialReadWarned.add(filePath); // The cause matters to whoever has to fix it: a file that never parses is a typo to correct, // while one that reads empty is a writer that never finished. const cause = error ? `: ${(error as Error).message ?? error}` : ' that were empty or incomplete'; fallbackLogger.warn(`Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} unusable reads${cause}`); + return true; } /** Test-only: whether a give-up warning for this file is currently suppressed as a duplicate. */ From 2306c6da4a7b7c2e0339c6af392ed7a85d46b9cc Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:30:38 -0600 Subject: [PATCH 12/17] fix(config): restore the retry budget on give-up so a repair is not missed A watcher that gave up kept a spent budget, so the write that repairs the file - which can itself be observed mid-write, the case this retry exists for - had no re-read left to recover it, and chokidar may emit nothing further. Each incident now gets its own budget; only the report stays standing, since the file has not recovered until a usable read says so. Refs #2313 Co-Authored-By: Claude Opus --- unitTests/utility/partialReadRetry.test.js | 18 ++++++++++++++++++ utility/watcherFallback.ts | 7 +++++-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js index b82d92a24e..7f0e9f01eb 100644 --- a/unitTests/utility/partialReadRetry.test.js +++ b/unitTests/utility/partialReadRetry.test.js @@ -96,6 +96,24 @@ describe('PartialReadRetry', () => { assert.strictEqual(new PartialReadRetry(path).gaveUp(), true, 'so the next incident reports again'); }); + it('restores the budget when it gives up, so a later repair is not missed', async () => { + // The repair can itself be observed mid-write, which is the case the retry exists for — a + // watcher left with no budget would drop it and chokidar may emit nothing further. + const retry = new PartialReadRetry('/nonexistent/repaired.yaml'); + let rereads = 0; + for (let attempt = 1; retry.schedule(() => rereads++); attempt++) { + assert.ok(attempt <= 50, 'the budget must be bounded'); + await waitFor(() => rereads === attempt, { message: `re-read ${attempt} never fired` }); + } + retry.gaveUp(); + + assert.strictEqual( + retry.schedule(() => rereads++), + true, + 'the next incident needs its own budget' + ); + }); + it('stops re-reading after close', async () => { const retry = new PartialReadRetry('/nonexistent/config.yaml'); let rereads = 0; diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 0cbd815a14..15aff9137f 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -117,10 +117,13 @@ export class PartialReadRetry { } /** - * The budget is spent. Distinct from `settled()`: the file has not recovered, so the report - * stands for every other watcher of it. Returns whether this give-up was the one reported. + * The budget is spent. Distinct from `settled()` in that the report stands — the file has not + * recovered, and it is shared with every other watcher of it. The budget itself is restored, + * because the next event may be the repair, and that repair can be observed mid-write too. + * Returns whether this give-up was the one reported. */ gaveUp(error?: unknown): boolean { + this.#remaining = PARTIAL_READ_MAX_REREADS; return warnPartialReadGaveUp(this.#filePath, error); } From 3de593b9d871b333892a0a69b492b4bd8a49c945 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:32:51 -0600 Subject: [PATCH 13/17] fix(config): make closing the retry terminal cancel() left the budget at zero rather than marking the retry closed, and gaveUp() now restores the budget - so a give-up after close would re-arm a re-read on a watcher that is shutting down. Close is now terminal: nothing schedules or reports after it. Refs #2313 Co-Authored-By: Claude Opus --- unitTests/utility/partialReadRetry.test.js | 6 ++++++ utility/watcherFallback.ts | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js index 7f0e9f01eb..ebab0626c2 100644 --- a/unitTests/utility/partialReadRetry.test.js +++ b/unitTests/utility/partialReadRetry.test.js @@ -127,5 +127,11 @@ describe('PartialReadRetry', () => { retry.schedule(() => rereads++), false ); + // Close is terminal: giving up restores the budget, and must not do so after close. + assert.strictEqual(retry.gaveUp(), false); + assert.strictEqual( + retry.schedule(() => rereads++), + false + ); }); }); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 15aff9137f..b7a5547999 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -88,6 +88,7 @@ export class PartialReadRetry { #filePath: string; #timer?: ReturnType; #remaining: number = PARTIAL_READ_MAX_REREADS; + #closed = false; constructor(filePath: string) { this.#filePath = filePath; @@ -95,6 +96,7 @@ export class PartialReadRetry { /** False once the budget is spent, so the caller can fall back to its own error handling. */ schedule(reread: () => void): boolean { + if (this.#closed) return false; if (this.#timer) return true; if (this.#remaining <= 0) return false; this.#remaining--; @@ -123,14 +125,16 @@ export class PartialReadRetry { * Returns whether this give-up was the one reported. */ gaveUp(error?: unknown): boolean { + if (this.#closed) return false; this.#remaining = PARTIAL_READ_MAX_REREADS; return warnPartialReadGaveUp(this.#filePath, error); } + /** Terminal: the watcher is closing, so nothing may re-arm the re-read or report on it. */ cancel() { if (this.#timer) clearTimeout(this.#timer); this.#timer = undefined; - this.#remaining = 0; + this.#closed = true; } } From d86d36bd9f8a986ba665a62b5915987cab282045 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:55:26 -0600 Subject: [PATCH 14/17] fix(config): restore the budget on the error-bearing give-up too The empty-read path restores the retry budget when it gives up so the write that repairs the file - itself observable mid-write - still has a re-read to catch it. The error-bearing path did not, leaving a scope that hit an unreadable file with no way to recover its next partial read. Same give-up on both; the error still takes the scope's error route. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 3 +++ .../config/configReadHandleLifetime.test.js | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 9bbd6e4c8d..c2410f4db9 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -191,6 +191,9 @@ export class OptionsWatcher extends EventEmitter { this.#partialRead.gaveUp(); return; } + // Same give-up as the empty 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 route. + this.#partialRead.gaveUp(error); this.#handleReadError(error); } diff --git a/unitTests/config/configReadHandleLifetime.test.js b/unitTests/config/configReadHandleLifetime.test.js index 2734dce963..7b5525cbf2 100644 --- a/unitTests/config/configReadHandleLifetime.test.js +++ b/unitTests/config/configReadHandleLifetime.test.js @@ -131,6 +131,30 @@ describe('root config read handle lifetime', () => { assert.strictEqual(watcher.get(['enabled']), false); }); + it('OptionsWatcher can still recover after an error-bearing read exhausted its budget', async () => { + const watcher = new OptionsWatcher('test-component', configFilePath, undefined, true); + openWatchers.push(watcher); + await watcher.ready; + watcher.on('error', () => {}); + + // An unreadable file drains the budget on its own: each re-read fails and arms the next. + clearPartialReadWarning(configFilePath); + rmSync(configFilePath); + mkdirSync(configFilePath); + await watcher._handleChangeForTests(); + await waitFor(() => isPartialReadWarned(configFilePath), { message: 'the error path never gave up' }); + + // The repair can be observed mid-write too, so the budget has to be back. + rmSync(configFilePath, { recursive: true }); + writeFileSync(configFilePath, ''); + await watcher._handleChangeForTests(); + assert.strictEqual(watcher.get(['enabled']), true, 'the half-written repair must not be adopted'); + + writeFileSync(configFilePath, stringify({ 'test-component': { enabled: false } })); + await once(watcher, 'change'); + assert.strictEqual(watcher.get(['enabled']), false); + }); + it('OptionsWatcher applies a root-config change before the change handler returns', async () => { const watcher = new OptionsWatcher('test-component', configFilePath, undefined, true); openWatchers.push(watcher); From e101cddd08370141012d29cc914c9265fa40825b Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 08:56:14 -0600 Subject: [PATCH 15/17] docs: record the root-config descriptor-lifetime invariant in DESIGN.md The rule that no descriptor on harper-config.yaml may outlive a turn is enforced only by prose, so a future fsPromises.readFile of that file silently reintroduces harper#2313. Write down the measurement behind it, and the three distinct outcomes of the partial-read retry. Refs #2313 Co-Authored-By: Claude Opus --- DESIGN.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/DESIGN.md b/DESIGN.md index 276e5ac32a..ecc83e5cc0 100644 --- a/DESIGN.md +++ b/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. From 0869a27b20f6ee342fe3451f7ff17a7146d87dd9 Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Tue, 25 Aug 2026 09:02:24 -0600 Subject: [PATCH 16/17] fix(config): report why a config read failed without quoting the file A YAML parse error's message includes the offending source lines, and this file holds credentials - a truncated write near a password line would have put it in a warn-level log. Report the error kind and its line and column instead, which is what an operator needs to fix it. Also drops a branch of the recovery path that no caller can reach any more. Refs #2313 Co-Authored-By: Claude Opus --- components/OptionsWatcher.ts | 11 ++++------- unitTests/utility/partialReadRetry.test.js | 22 ++++++++++++++++++++++ utility/watcherFallback.ts | 17 +++++++++++++++-- 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index c2410f4db9..08d240cc8b 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -185,14 +185,11 @@ export class OptionsWatcher extends EventEmitter { } #recoverOrReport(error: unknown) { - if (error !== undefined && !isPartialReadError(error)) return this.#handleReadError(error); + if (!isPartialReadError(error)) return this.#handleReadError(error); if (this.#partialRead.schedule(() => this.#handleChange())) return; - if (error === undefined) { - this.#partialRead.gaveUp(); - return; - } - // Same give-up as the empty 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 route. + // 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); } diff --git a/unitTests/utility/partialReadRetry.test.js b/unitTests/utility/partialReadRetry.test.js index ebab0626c2..e28d1cb395 100644 --- a/unitTests/utility/partialReadRetry.test.js +++ b/unitTests/utility/partialReadRetry.test.js @@ -4,13 +4,35 @@ const { warnPartialReadGaveUp, isPartialReadWarned, clearPartialReadWarning, + describeReadFailure, } = require('#src/utility/watcherFallback'); +const { parse } = require('yaml'); const { waitFor } = require('../waitFor'); // The retry is a timer, so "it fired" is a condition to wait for; "it fired only once" is a // non-event, which is the one case AGENTS.md reserves a fixed settle for. const settle = () => new Promise((resolve) => setTimeout(resolve, 100)); +describe('describeReadFailure', () => { + it('reports where a config file failed to parse, never what it contains', () => { + let description; + try { + parse('operationsApi:\n password: hunter2-super-secret\n port: [unclosed\n'); + assert.fail('the fixture must not parse'); + } catch (error) { + description = describeReadFailure(error); + } + + // The parser's own message quotes the offending source lines, which is why it is not used. + assert.match(description, /line \d+, column \d+/); + assert.doesNotMatch(description, /hunter2|password|unclosed/); + }); + + it('reports a read error by its code', () => { + assert.strictEqual(describeReadFailure(Object.assign(new Error('nope'), { code: 'EACCES' })), 'EACCES'); + }); +}); + describe('PartialReadRetry', () => { it('re-reads once for a burst of unusable reads, not once per event', async () => { const retry = new PartialReadRetry('/nonexistent/config.yaml'); diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index b7a5547999..1e527ca567 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -157,12 +157,25 @@ export function warnPartialReadGaveUp(filePath: string, error?: unknown): boolea if (partialReadWarned.has(filePath)) return false; partialReadWarned.add(filePath); // The cause matters to whoever has to fix it: a file that never parses is a typo to correct, - // while one that reads empty is a writer that never finished. - const cause = error ? `: ${(error as Error).message ?? error}` : ' that were empty or incomplete'; + // while one that reads empty is a writer that never finished. Report the kind and position + // only — a YAML parse error's message quotes the offending source, and this file holds + // credentials. + const cause = error ? `: ${describeReadFailure(error)}` : ' that were empty or incomplete'; fallbackLogger.warn(`Gave up re-reading ${filePath} after ${PARTIAL_READ_MAX_REREADS} unusable reads${cause}`); return true; } +/** + * The kind and position of a failed config read, never its content: a YAML parse error's message + * quotes the offending source lines, and these files hold credentials. + */ +export function describeReadFailure(error: unknown): string { + if (typeof error !== 'object' || error === null) return 'unusable'; + const { name, code, linePos } = error as { name?: string; code?: string; linePos?: { line: number; col: number }[] }; + const at = linePos?.[0] ? ` at line ${linePos[0].line}, column ${linePos[0].col}` : ''; + return `${code ?? name ?? 'unusable'}${at}`; +} + /** Test-only: whether a give-up warning for this file is currently suppressed as a duplicate. */ export function isPartialReadWarned(filePath: string): boolean { return partialReadWarned.has(filePath); From 367322f7cba5c1c8d46e0b406a2b5c53eb76d64c Mon Sep 17 00:00:00 2001 From: Kris Zyp Date: Thu, 27 Aug 2026 08:26:54 -0600 Subject: [PATCH 17/17] fix(config): correct and trim comments flagged in pre-push review isPartialReadError's doc claimed both watchers answer ENOENT deliberately; only OptionsWatcher does, RootConfigWatcher silently returns. Also drop a few comments that restated the code next to them instead of adding non-obvious why. Co-Authored-By: Claude Sonnet 5 --- components/OptionsWatcher.ts | 3 +-- config/RootConfigWatcher.ts | 3 +-- config/configUtils.ts | 4 +--- utility/watcherFallback.ts | 7 ++++--- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/components/OptionsWatcher.ts b/components/OptionsWatcher.ts index 08d240cc8b..7f84dfe6f5 100644 --- a/components/OptionsWatcher.ts +++ b/components/OptionsWatcher.ts @@ -138,8 +138,7 @@ export class OptionsWatcher extends EventEmitter { .on('ready', this.#handleChange.bind(this)); } - // The root config reads synchronously so its descriptor cannot outlive this turn - see the - // invariant on atomicWriteFile. + // Root config only: see the descriptor-lifetime invariant on atomicWriteFile (DESIGN.md). #handleChange() { if (this.#isRootConfig) { this.#applyRead(() => readFileSync(this.#filePath, 'utf-8')); diff --git a/config/RootConfigWatcher.ts b/config/RootConfigWatcher.ts index 1d82df1cf4..6f05ebf7f8 100644 --- a/config/RootConfigWatcher.ts +++ b/config/RootConfigWatcher.ts @@ -90,8 +90,7 @@ export class RootConfigWatcher extends EventEmitter { this.emit('error', error); } - // Reads synchronously so its descriptor cannot outlive this turn - see the invariant on - // atomicWriteFile. + // See the descriptor-lifetime invariant on atomicWriteFile (DESIGN.md). handleChange() { let config; // Only the read and parse are guarded: a listener that throws must not be mistaken for a diff --git a/config/configUtils.ts b/config/configUtils.ts index 99c8fd56dc..5d485a33b4 100644 --- a/config/configUtils.ts +++ b/config/configUtils.ts @@ -146,9 +146,7 @@ export function atomicWriteFile( if (!renamed) { try { fs.unlinkSync(tempPath); - } catch { - // A cleanup failure must not replace the error that got us here. - } + } catch {} } } } diff --git a/utility/watcherFallback.ts b/utility/watcherFallback.ts index 1e527ca567..01daf64eb8 100644 --- a/utility/watcherFallback.ts +++ b/utility/watcherFallback.ts @@ -139,9 +139,10 @@ export class PartialReadRetry { } /** - * ENOENT is excluded not because it cannot be transient, but because both watchers already - * answer it deliberately (env-only fallback at boot, `remove` afterwards); re-reading would - * only delay a decision they have already made. + * ENOENT is excluded not because it cannot be transient, but because it already has an answer: + * `OptionsWatcher` routes it to `remove` (env-only fallback at boot, then removal), and + * `RootConfigWatcher` keeps its last config rather than tearing down core features on a file + * that may just be mid-replace. Re-reading would only delay a decision already made. */ export function isPartialReadError(error: unknown): boolean { return !(typeof error === 'object' && error !== null && (error as { code?: string }).code === 'ENOENT');