From f0c31c6e1013fd048705439b19d8975017feb682 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:33:03 +0000 Subject: [PATCH 01/12] fix: Keep file data and overrides through an in-place rewrite of a file A file that is rewritten in place is truncated and then written, so a reload can find it empty. The file override source applied an emptied YAML file as a document with no entries, which removed its overrides until the next reload. An emptied JSON file failed to parse and was reported as an error on every in-place rewrite, by the override source and by the file data source. The shared file reloader now remembers, for each file, whether its last read found content. A reload that finds a file empty after it had content keeps the current data, logs at debug level, and reads the file again after the retry delay, without reporting a failure. The next reload parses whatever the file holds, so the data is held back at most once per emptying. A file that is empty from the start is never read again, and with retries off an empty file is parsed right away. An empty JSON file now fails with a message that says the file has no content. --- .../LDClientNode.fileDataSource.test.ts | 63 +++- .../LDClientNode.fileOverrides.test.ts | 78 ++++- .../server-node/__tests__/inPlaceWrites.ts | 53 ++++ .../FileReloader.inPlaceRewrite.test.ts | 294 ++++++++++++++++++ .../data_sources/filedata/document.test.ts | 31 ++ .../overrides/overrideDocument.test.ts | 6 + .../api/integrations/FileDataSourceOptions.ts | 4 + .../src/api/options/LDDataSystemOptions.ts | 11 + .../src/data_sources/filedata/FileReloader.ts | 75 ++++- .../src/data_sources/filedata/document.ts | 25 +- .../src/data_sources/filedata/index.ts | 2 + .../src/overrides/FileOverrideSource.ts | 6 +- .../src/overrides/overrideDocument.ts | 6 +- 13 files changed, 638 insertions(+), 16 deletions(-) create mode 100644 packages/sdk/server-node/__tests__/inPlaceWrites.ts create mode 100644 packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts diff --git a/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts index f99d40f71c..d950767f53 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts @@ -1,8 +1,13 @@ -import { mkdir, rm, stat, writeFile } from 'node:fs/promises'; +import { mkdir, mkdtemp, rm, stat, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { parse as parseYaml } from 'yaml'; import { integrations } from '@launchdarkly/js-server-sdk-common'; import LDClientNode from '../src/LDClientNode'; +import { rewriteInPlace, sampleValues } from './inPlaceWrites'; +import waitFor from './waitFor'; const flag1Key = 'flag1'; const flag2Key = 'flag2'; @@ -178,3 +183,59 @@ describe('When using a file data source', () => { client.close(); }); }); + +describe('given a file data source that reloads a file rewritten in place', () => { + let directory: string; + + beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'ld-file-data-')); + }); + + afterEach(async () => { + await rm(directory, { recursive: true, force: true }); + }); + + it.each(['json', 'yaml'])('never serves the emptied %s file', async (extension) => { + // The file is empty for 300 ms, longer than the settle window after a change, so a reload + // reads it empty. The client must keep serving the old value until the new one is written. + const path = join(directory, `flags.${extension}`); + await writeFile(path, JSON.stringify({ flagValues: { flag1: 'a' } })); // JSON is also YAML + const errors: string[] = []; + const fds = new integrations.FileDataSourceFactory({ + paths: [path], + autoUpdate: true, + yamlParser: parseYaml, + }); + const client = new LDClientNode('sdk-key', { + updateProcessor: fds.getFactory(), + sendEvents: false, + diagnosticOptOut: true, + logger: { + error: (message: string) => errors.push(message), + warn: () => {}, + info: () => {}, + debug: () => {}, + }, + }); + try { + await client.waitForInitialization({ timeout: 10 }); + expect(await client.variation('flag1', { key: 'user1' }, 'default')).toEqual('a'); + + const sampler = sampleValues(() => client.variation('flag1', { key: 'user1' }, 'default')); + try { + await rewriteInPlace(path, JSON.stringify({ flagValues: { flag1: 'b' } }), 300); + // Waits for the sampler itself to see the new value, so it has recorded everything that + // the client served before it. + await waitFor(() => sampler.served.includes('b'), 10000); + } finally { + await sampler.stop(); + } + + expect(sampler.served).toEqual(['a', 'b']); + // The file was read while it was empty, which is not an error to report. + expect(errors).toEqual([]); + } finally { + client.close(); + } + }); +}); diff --git a/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts index 4d2dc3989a..c8caa202bb 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts @@ -1,25 +1,30 @@ +import { promises as fsPromises } from 'node:fs'; import { mkdtemp, rm, unlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { parse as parseYaml } from 'yaml'; -import { FileOverrideSourceOptions, sleep } from '@launchdarkly/js-server-sdk-common'; +import { FileOverrideSourceOptions, LDLogger, sleep } from '@launchdarkly/js-server-sdk-common'; import LDClientNode from '../src/LDClientNode'; +import { rewriteInPlace, sampleValues } from './inPlaceWrites'; import waitFor from './waitFor'; const user = { key: 'user-key' }; +const silentLogger: LDLogger = { error: () => {}, warn: () => {}, info: () => {}, debug: () => {} }; + // The client never obtains data from LaunchDarkly: its only initializer reads a file that does // not exist. Overrides are served from the override layer alone. function makeClient( directory: string, overrides: Omit, + logger: LDLogger = silentLogger, ): LDClientNode { return new LDClientNode('sdk-key-file-overrides', { sendEvents: false, diagnosticOptOut: true, - logger: { error: () => {}, warn: () => {}, info: () => {}, debug: () => {} }, + logger, dataSystem: { dataSource: { dataSourceOptionsType: 'custom', @@ -133,4 +138,73 @@ describe('given a temporary directory of override files', () => { 10000, ); }); + + describe.each([ + ['json', 'watching'], + ['json', 'polling'], + ['yaml', 'watching'], + ['yaml', 'polling'], + ] as const)('given a %s file in %s mode', (extension, changeDetection) => { + let polls: number[]; + + beforeEach(() => { + polls = []; + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + // Records when the poller examines the file, so that a test can start a rewrite just before a + // poll. + const recordPolls = (path: string) => { + const realStat = fsPromises.stat; + jest.spyOn(fsPromises, 'stat').mockImplementation(((statPath: any, ...rest: any[]) => { + if (statPath === path) { + polls.push(Date.now()); + } + return (realStat as any).call(fsPromises, statPath, ...rest); + }) as any); + }; + + it('keeps the override while the file is rewritten in place', async () => { + const path = join(directory, `overrides.${extension}`); + await writeFile(path, document('a')); // JSON is also YAML + const errors: string[] = []; + recordPolls(path); + client = makeClient( + directory, + { paths: [path], changeDetection, pollInterval: 1, yamlParser: parseYaml }, + { ...silentLogger, error: (message: string) => errors.push(message) }, + ); + await waitFor( + async () => (await client!.variation('overridden-flag', user, 'default')) === 'a', + 10000, + ); + let pauseMs = 300; + if (changeDetection === 'polling') { + // The rewrite starts 750 ms after a poll, so the next poll finds the file empty, and its + // reload 100 ms later still does. The file is written 250 ms after that, well before the + // retry reads it again a second after the reload. + const count = polls.length; + await waitFor(() => polls.length > count, 5000, 1); + await sleep(polls[polls.length - 1] + 750 - Date.now()); + pauseMs = 600; + } + // In watching mode, the reload that the truncation starts runs after the 100 ms settle + // window, while the file is still empty. + const sampler = sampleValues(() => client!.variation('overridden-flag', user, 'default')); + try { + await rewriteInPlace(path, document('b'), pauseMs); + // Waits for the sampler itself to see the new value, so it has recorded everything that + // the client served before it. + await waitFor(() => sampler.served.includes('b'), 10000); + } finally { + await sampler.stop(); + } + + expect(sampler.served).toEqual(['a', 'b']); + expect(errors.filter((message) => message.startsWith('Unable to load flags'))).toEqual([]); + }); + }); }); diff --git a/packages/sdk/server-node/__tests__/inPlaceWrites.ts b/packages/sdk/server-node/__tests__/inPlaceWrites.ts new file mode 100644 index 0000000000..3db1b697b1 --- /dev/null +++ b/packages/sdk/server-node/__tests__/inPlaceWrites.ts @@ -0,0 +1,53 @@ +import { open } from 'node:fs/promises'; + +import { sleep } from '@launchdarkly/js-server-sdk-common'; + +/** + * Rewrites a file in place the slow way: it truncates the file, waits, and then writes the new + * content, so a reader can find the file empty in between. Applications and editors rewrite data + * files in place, and the SDK must tolerate reading one while it is being written. + */ +export async function rewriteInPlace( + path: string, + content: string, + pauseAfterTruncatingMs: number, +): Promise { + const handle = await open(path, 'r+'); + try { + await handle.truncate(0); + await sleep(pauseAfterTruncatingMs); + await handle.write(content, 0); + } finally { + await handle.close(); + } +} + +/** + * Evaluates over and over, and records each value that differs from the one before, until + * stopped. + */ +export function sampleValues(evaluate: () => Promise): { + served: unknown[]; + stop: () => Promise; +} { + const served: unknown[] = []; + let stopped = false; + const sampling = (async () => { + while (!stopped) { + // eslint-disable-next-line no-await-in-loop + const value = await evaluate(); + if (served.length === 0 || served[served.length - 1] !== value) { + served.push(value); + } + // eslint-disable-next-line no-await-in-loop + await sleep(2); + } + })(); + return { + served, + stop: async () => { + stopped = true; + await sampling; + }, + }; +} diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts new file mode 100644 index 0000000000..93341b7f44 --- /dev/null +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -0,0 +1,294 @@ +import { + FileReloader, + FileReloaderConfig, + hasContent, + LoadFailure, + ReloadResult, +} from '../../../src/data_sources/filedata'; +import { parseOverrideDocument } from '../../../src/overrides/overrideDocument'; +import TestLogger, { LogLevel } from '../../Logger'; +import MockFilesystem from './MockFilesystem'; +import testPolicy from './testPolicy'; + +// These tests rewrite a file the way an application or editor rewrites it in place: the file is +// truncated, and the new content is written some time later. A reload in between finds the file +// empty. + +const jsonPath = '/data/data.json'; +const yamlPath = '/data/data.yaml'; +const otherYamlPath = '/data/other.yaml'; +const retryDelayMs = 1000; + +const documentA = '{"flagValues": {"flag1": "a"}}'; +const documentB = '{"flagValues": {"flag1": "b"}}'; + +// A stand-in for a YAML parser. A flow-style document like the ones above is both JSON and YAML, +// and like the YAML parsers, it returns null for a file with no content. +const yamlParser = (data: string) => (hasContent(data) ? JSON.parse(data) : null); + +const emptiedReadAgain = /is empty, possibly while it is being rewritten; reading it again/; + +describe('given a reloader over a file that is rewritten in place', () => { + let filesystem: MockFilesystem; + let logger: TestLogger; + let applied: ReloadResult[]; + let failures: LoadFailure[]; + let reloader: FileReloader; + + beforeEach(() => { + jest.useFakeTimers(); + filesystem = new MockFilesystem(); + logger = new TestLogger(); + applied = []; + failures = []; + }); + + afterEach(() => { + reloader?.close(); + jest.useRealTimers(); + }); + + // The policy parses by extension and validates the document as the override source does, so a + // YAML file with no content is a document with no entries and a JSON file with no content fails. + const makeReloader = (paths: string[], configure?: (config: FileReloaderConfig) => void) => { + const config: FileReloaderConfig = { + paths, + policy: testPolicy({ parseDocument: parseOverrideDocument(yamlParser) }), + filesystem, + logger, + apply: (result) => applied.push(result), + onFailure: (failure) => failures.push(failure), + debounceDelayMs: 0, + retryDelayMs, + skipUnchanged: true, + }; + configure?.(config); + reloader = new FileReloader(config); + return reloader; + }; + + const readAgainMessages = () => + logger.getMessages(LogLevel.Debug).filter((message) => emptiedReadAgain.test(message)); + + const values = () => + applied.map((result) => result.flags.map(({ item }) => item.variations?.[0]).join(',')); + + const changeAndSettle = async () => { + reloader.trigger(); + await jest.advanceTimersByTimeAsync(0); + }; + + it.each(['', ' \n\t\r\n', '\uFEFF'])( + 'reads an emptied file again and applies the rewritten content with no empty application, for content %j', + async (empty) => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + filesystem.set(yamlPath, empty); + await changeAndSettle(); + expect(values()).toEqual(['a']); + expect(failures).toEqual([]); + logger.expectMessages([{ level: LogLevel.Debug, matches: emptiedReadAgain }]); + + filesystem.set(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + expect(failures).toEqual([]); + }, + ); + + it('applies a YAML file that stays empty when the retry reads it again', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + filesystem.set(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a']); + + await jest.advanceTimersByTimeAsync(retryDelayMs - 1); + expect(values()).toEqual(['a']); + await jest.advanceTimersByTimeAsync(1); + expect(values()).toEqual(['a', '']); + expect(failures).toEqual([]); + }); + + it('does not report an emptied JSON file at the load that finds it empty, and the retry reports it', async () => { + filesystem.set(jsonPath, documentA); + makeReloader([jsonPath]); + await reloader.reloadNow(); + + filesystem.set(jsonPath, ''); + await changeAndSettle(); + expect(failures).toEqual([]); + + await jest.advanceTimersByTimeAsync(retryDelayMs); + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ kind: 'parse', path: jsonPath, repeated: false }); + expect(failures[0].error.message).toEqual( + 'the file has no content, which is not a JSON document', + ); + expect(values()).toEqual(['a']); + }); + + it('applies a file that stays empty at the next change signal, before the retry would', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + filesystem.set(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a']); + + // A stream of change signals cannot hold the data back: the file was empty at its last read. + await changeAndSettle(); + expect(values()).toEqual(['a', '']); + }); + + it('never reads an empty placeholder file again, and it does not delay a change to another file', async () => { + filesystem.set(otherYamlPath, ''); + filesystem.set(yamlPath, documentA); + makeReloader([otherYamlPath, yamlPath]); + await reloader.reloadNow(); + expect(values()).toEqual(['a']); + + filesystem.set(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + expect(readAgainMessages()).toEqual([]); + }); + + it('reads each of two files that are rewritten together again', async () => { + filesystem.set(yamlPath, documentA); + filesystem.set(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + expect(values()).toEqual(['a,a']); + + filesystem.set(yamlPath, ''); + filesystem.set(otherYamlPath, ''); + await changeAndSettle(); + + // The first file is written. The second is still empty, after it had content. + filesystem.set(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + + filesystem.set(otherYamlPath, '{"flagValues": {"flag2": "b"}}'); + await changeAndSettle(); + expect(values()).toEqual(['a,a', 'b,b']); + expect(failures).toEqual([]); + }); + + it('applies a file that is deleted and then created empty right away', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath], (config) => { + config.skipUnchanged = false; + }); + await reloader.reloadNow(); + + filesystem.remove(yamlPath); + await changeAndSettle(); + expect(applied[1].files).toEqual([{ path: yamlPath, present: false, flags: 0, segments: 0 }]); + + filesystem.set(yamlPath, ''); + await changeAndSettle(); + expect(applied).toHaveLength(3); + expect(applied[2].files).toEqual([{ path: yamlPath, present: true, flags: 0, segments: 0 }]); + }); + + it('reads an emptied file again when a retry left over from a failure finds it', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + // A malformed edit fails the reload and arms the retry. + filesystem.set(yamlPath, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(1); + + // The file is rewritten in place, and the retry runs before the new content is written. + filesystem.set(yamlPath, ''); + await jest.advanceTimersByTimeAsync(retryDelayMs); + expect(values()).toEqual(['a']); + expect(failures).toHaveLength(1); + + filesystem.set(yamlPath, documentB); + await jest.advanceTimersByTimeAsync(retryDelayMs); + expect(values()).toEqual(['a', 'b']); + }); + + it('parses an emptied file right away when retries are off', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath], (config) => { + config.retryDelayMs = 0; + }); + await reloader.reloadNow(); + + filesystem.set(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a', '']); + }); + + it('does not count reading again as a failure, so an unchanged rewrite applies nothing', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + filesystem.set(yamlPath, ''); + await changeAndSettle(); + filesystem.set(yamlPath, documentA); + await changeAndSettle(); + + // A reload after a failure applies even unchanged content. Reading again is not a failure. + expect(values()).toEqual(['a']); + expect(failures).toEqual([]); + }); + + it('reports a failure after reading again as news when the load that found the file empty followed a change', async () => { + filesystem.set(jsonPath, documentA); + makeReloader([jsonPath]); + await reloader.reloadNow(); + + // A malformed edit fails, and the retries keep failing the same way. + filesystem.set(jsonPath, '{"flagValues": '); + await changeAndSettle(); + await jest.advanceTimersByTimeAsync(retryDelayMs); + expect(failures.map((failure) => failure.repeated)).toEqual([false, true]); + + // The file is emptied, which is a change. The retry that reads it again finds it still empty. + // That failure is news, as it would have been for the load that the change started. + filesystem.set(jsonPath, ''); + await changeAndSettle(); + expect(failures).toHaveLength(2); + await jest.advanceTimersByTimeAsync(retryDelayMs); + expect(failures).toHaveLength(3); + expect(failures[2].repeated).toBe(false); + expect(failures[2].error.message).toEqual( + 'the file has no content, which is not a JSON document', + ); + logger.expectMessages([ + { level: LogLevel.Debug, matches: /Reading flag data again after a file was found empty/ }, + ]); + }); + + it('does not read again after close', async () => { + filesystem.set(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + filesystem.set(yamlPath, ''); + filesystem.deferReads = true; + reloader.trigger(); + await jest.advanceTimersByTimeAsync(0); + expect(filesystem.pendingReads).toHaveLength(1); + reloader.close(); + filesystem.completePendingReads(); + await jest.advanceTimersByTimeAsync(retryDelayMs * 2); + + expect(filesystem.readCount).toEqual(2); + expect(readAgainMessages()).toEqual([]); + expect(values()).toEqual(['a']); + }); +}); diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts index 01031edf90..e151315a72 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts @@ -1,4 +1,5 @@ import { + hasContent, isFileNotFoundError, isYamlPath, parseDocumentByExtension, @@ -35,6 +36,28 @@ describe('given the extension-based parser', () => { expect(yamlParser).not.toHaveBeenCalled(); }); + it.each([ + ['data.json', ''], + ['data.json', ' \n\t\r\n'], + ['data.json', '\uFEFF'], + ['data', ''], + ['data.txt', ' '], + ])('rejects %s with no content %j, which is not a JSON document', (path, data) => { + const yamlParser = jest.fn(); + const parse = () => parseDocumentByExtension(yamlParser)(path, data); + expect(parse).toThrow(SyntaxError); + expect(parse).toThrow('the file has no content, which is not a JSON document'); + expect(yamlParser).not.toHaveBeenCalled(); + }); + + it.each(['data.yml', 'data.yaml'])( + 'returns the YAML parser result for %s with no content as it is', + (path) => { + expect(parseDocumentByExtension(() => null)(path, '')).toBeNull(); + expect(parseDocumentByExtension(() => undefined)(path, ' \n')).toBeUndefined(); + }, + ); + it.each(['data.yml', 'data.yaml'])( 'parses %s with the YAML parser even when the content looks like JSON', (path) => { @@ -65,6 +88,14 @@ describe('given the extension-based parser', () => { }); }); +it('treats text with only white space or a byte order mark as no content', () => { + expect(hasContent('')).toBe(false); + expect(hasContent(' \n\t\r\n')).toBe(false); + expect(hasContent('\uFEFF\n')).toBe(false); + expect(hasContent('{}')).toBe(true); + expect(hasContent('\n a')).toBe(true); +}); + it('recognizes YAML paths', () => { expect(isYamlPath('a.yml')).toBe(true); expect(isYamlPath('a.yaml')).toBe(true); diff --git a/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts b/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts index 3cf4524b5f..f68be673cd 100644 --- a/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts +++ b/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts @@ -80,6 +80,12 @@ describe('given the override document parser', () => { expect(parseOverrideDocument(() => undefined)('empty.yaml', '')).toEqual({}); }); + it.each(['', ' \n\t\n'])('rejects a JSON file with no content %j', (data) => { + expect(() => parseOverrideDocument()('empty.json', data)).toThrow( + 'the file has no content, which is not a JSON document', + ); + }); + it('rejects malformed JSON', () => { expect(() => parseOverrideDocument()('data.json', '{"flagValues"')).toThrow(/json/i); }); diff --git a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts index f3322d2858..6a52b3401b 100644 --- a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts +++ b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts @@ -17,6 +17,10 @@ export interface FileDataSourceOptions { * A file that is replaced by a rename, or deleted and created again, is detected. A load * that fails, for example because a file was read while it was written, is retried after a * short delay and the previously loaded data stays in effect until it succeeds. + * A file can be rewritten in place: a file that had content and is found empty is read again + * after the same delay before it is applied. YAML that is cut off between two entries can still + * parse, so for a YAML file, replacing its entire contents in one operation, such as renaming a + * fully written temporary file over it, avoids briefly applying only a part of it. * A change to another entry in a directory causes a reload only when the metadata of a * configured file changed, read through any symbolic link, which is how a mounted ConfigMap * or Secret is updated. diff --git a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts index 452b6d48f6..d7b72d1d97 100644 --- a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts +++ b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts @@ -119,6 +119,15 @@ export interface LDDataSystemOptions { * retries. Every applied change is logged at Info level with the overrides in effect and what * each file supplied. * + * A file can be rewritten in place. To remove a file's overrides, delete it or write a document + * with no entries, such as `{}`. A JSON file with no content is not a valid document: it fails to + * load, and the overrides in effect stay. A file that had content and is found empty may be in + * the middle of a rewrite, so the source waits about a second and reads it again before applying + * it. An emptied YAML file therefore removes its overrides about a second later. YAML that is cut + * off between two entries can still parse, so for a YAML file, replacing its entire contents in + * one operation, such as renaming a fully written temporary file over it, avoids briefly applying + * only a part of it. + * * @example * ```typescript * const client = init(sdkKey, { @@ -171,6 +180,8 @@ export interface FileOverrideSourceOptions { * A YAML parser for YAML files. The SDK does not include one; to read YAML files, supply a * parser that produces the same structure as `JSON.parse`, for example the `parse` function * of the `yaml` package. A YAML file configured without a parser fails client construction. + * A result of `null` or `undefined`, which YAML parsers return for a file with no content, + * means that the file has no entries. */ yamlParser?: (data: string) => any; } diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 254686f481..80e7da056e 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -1,7 +1,7 @@ import { Filesystem, LDLogger, monotonicNow } from '@launchdarkly/js-sdk-common'; import { Flag } from '../../evaluation/data/Flag'; -import { FileDataDocument, isFileNotFoundError } from './document'; +import { FileDataDocument, hasContent, isFileNotFoundError } from './document'; import { FileDataPolicy } from './FileDataPolicy'; import { FileSummary, mergeDocuments, MergeResult } from './merge'; @@ -104,8 +104,9 @@ export interface FileReloaderConfig { */ debounceDelayMs: number; /** - * How long to wait after a failed reload before retrying it without a trigger. Zero or - * negative disables the automatic retry. + * How long to wait after a failed reload before retrying it without a trigger. A file that had + * content and is found empty is read again after the same delay before it is applied. Zero or + * negative disables the automatic retry, and an empty file is then parsed as it is. */ retryDelayMs: number; /** @@ -126,6 +127,10 @@ function asError(err: unknown): Error { * signals, keeps the last good result on failure (by not calling `apply`), retries after * failures, and skips no-op applications. * + * A file that is rewritten in place is truncated and then written, so a reload can find it + * empty. A reload that finds a file empty after it had content keeps the current data and reads + * the file again after the retry delay, whatever the file's format. See `_readAgainLater`. + * * @internal */ export default class FileReloader { @@ -153,6 +158,15 @@ export default class FileReloader { private _lastFailureKey?: string; + // Whether the last read of each configured path found content. A missing file counts as no + // content. + private readonly _lastReadHadContent = new Map(); + + // Set by a reload that found a file emptied and armed the retry to read it again, to that + // reload's reason, until the next reload starts. The next reload continues the deferred one, so + // it reports a failure the way the deferred reload would have. + private _readAgainReason?: ReloadReason; + constructor(private readonly _config: FileReloaderConfig) {} /** @@ -258,14 +272,21 @@ export default class FileReloader { * wins for a key. Every filesystem and parsing failure is caught and reported, so the returned * promise never rejects. */ - private async _reload(reason: ReloadReason): Promise { + private async _reload(requested: ReloadReason): Promise { if (this._closed) { return; } const { logger, paths, policy } = this._config; - if (reason === 'change') { + // The retry that reads an emptied file again continues the reload that found it empty, so a + // failure is news when it would have been news to that reload. + const deferredReason = requested === 'retry' ? this._readAgainReason : undefined; + const reason = deferredReason ?? requested; + this._readAgainReason = undefined; + if (requested === 'change') { logger?.info('Reloading flag data after detecting a change'); - } else if (reason === 'retry') { + } else if (deferredReason) { + logger?.debug('Reading flag data again after a file was found empty'); + } else if (requested === 'retry') { logger?.debug('Retrying flag data load after earlier failure'); } // A pending retry is superseded by this reload. It either succeeds, or it fails and arms a @@ -276,18 +297,23 @@ export default class FileReloader { const files: FileSummary[] = []; const contents: string[] = []; let failure: LoadFailure | undefined; + let emptied: string | undefined; const readAll = paths.reduce( (previous, path) => previous.then(async () => { - if (failure) { + if (failure || emptied !== undefined) { return; } let data: string; try { data = await this._config.filesystem.readFile(path); } catch (err) { - if (policy.missingFile === 'skip' && isFileNotFoundError(err)) { + const missing = isFileNotFoundError(err); + if (missing) { + this._lastReadHadContent.set(path, false); + } + if (missing && policy.missingFile === 'skip') { logger?.debug(`File ${path} does not exist. It contributes no data.`); files.push({ path, present: false, flags: 0, segments: 0 }); return; @@ -295,6 +321,13 @@ export default class FileReloader { failure = { kind: 'read', error: asError(err), path, repeated: false }; return; } + const nonEmpty = hasContent(data); + const hadContent = this._lastReadHadContent.get(path) === true; + this._lastReadHadContent.set(path, nonEmpty); + if (!nonEmpty && hadContent && this._config.retryDelayMs > 0) { + emptied = path; + return; + } contents.push(data); try { documents.push(policy.parseDocument(path, data)); @@ -308,6 +341,11 @@ export default class FileReloader { ); await readAll; + if (emptied !== undefined) { + this._readAgainLater(emptied, reason); + return; + } + let merged: MergeResult | undefined; if (!failure) { try { @@ -363,6 +401,27 @@ export default class FileReloader { this._lastGoodFlags = nextFlags; } + /** + * A file that had content when it was last read and is empty now may be in the middle of an + * in-place rewrite: a writer that truncates the file and then writes the new content. Whatever + * the file's format, the reload keeps the current data and arms the retry without reporting a + * failure, so the file is read again after the retry delay. The change signal for the write + * starts a reload sooner. That next reload parses what the file holds then, because the file + * was empty when it was last read: a file that is still empty is invalid JSON, or a YAML + * document with no entries. So the current data is kept for at most one more reload, and a file + * that is empty from the start, such as a placeholder, is never read again. + */ + private _readAgainLater(path: string, reason: ReloadReason): void { + if (this._closed) { + return; + } + this._config.logger?.debug( + `File ${path} is empty, possibly while it is being rewritten; reading it again before applying it`, + ); + this._readAgainReason = reason; + this._armRetry(); + } + private _fail(failure: LoadFailure, reason: ReloadReason): void { if (this._closed) { return; diff --git a/packages/shared/sdk-server/src/data_sources/filedata/document.ts b/packages/shared/sdk-server/src/data_sources/filedata/document.ts index d535c9e86b..2be96de1da 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/document.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/document.ts @@ -36,6 +36,16 @@ export function isYamlPath(path: string): boolean { return path.endsWith('.yml') || path.endsWith('.yaml'); } +/** + * Reports whether the text of a file has content: anything other than white space. A byte order + * mark counts as white space. + * + * @internal + */ +export function hasContent(data: string): boolean { + return /\S/.test(data); +} + function parseYaml(path: string, data: string, yamlParser?: YamlParser): any { if (!yamlParser) { throw new Error(`Attempted to parse yaml file (${path}) without parser.`); @@ -43,11 +53,24 @@ function parseYaml(path: string, data: string, yamlParser?: YamlParser): any { return yamlParser(data); } +function parseJson(data: string): any { + if (!hasContent(data)) { + // JSON.parse fails here too, with a message that does not say what is wrong. A file that is + // rewritten in place can be empty for a moment, and the failure keeps the last good data. + throw new SyntaxError('the file has no content, which is not a JSON document'); + } + return JSON.parse(data); +} + /** * A parser that chooses the format by file extension only. A `.yml` or `.yaml` file uses the * YAML parser, and every other file is JSON. The parsed value is returned as it is. This is the * rule of the file data sources. * + * Whether a file with no content is valid depends on its format. A JSON file with no content, + * or only white space, is not a JSON document and fails to parse. A YAML parser returns null or + * undefined for it, which is returned as it is. + * * @internal */ export function parseDocumentByExtension(yamlParser?: YamlParser): DocumentParser { @@ -55,7 +78,7 @@ export function parseDocumentByExtension(yamlParser?: YamlParser): DocumentParse if (isYamlPath(path)) { return parseYaml(path, data, yamlParser); } - return JSON.parse(data); + return parseJson(data); }; } diff --git a/packages/shared/sdk-server/src/data_sources/filedata/index.ts b/packages/shared/sdk-server/src/data_sources/filedata/index.ts index 18ef8828a9..c3b9a3ad99 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/index.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/index.ts @@ -1,6 +1,7 @@ import { DocumentParser, FileDataDocument, + hasContent, isFileNotFoundError, isYamlPath, parseDocumentByExtension, @@ -38,6 +39,7 @@ export { FileReloader, FileReloaderConfig, FileSummary, + hasContent, isFileNotFoundError, isYamlPath, ItemCategory, diff --git a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts index cca60452aa..e41a8d3ef4 100644 --- a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts +++ b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts @@ -143,8 +143,10 @@ function countsText(flags: number, segments: number): string { * A reload replaces the entire override set. A configured file that does not exist contributes * no overrides, so deleting a file removes its overrides. A file that exists but cannot be read * or parsed makes that whole reload fail. The previously loaded overrides stay in effect, the - * failure is logged, and the source retries after a short delay. Every applied change is logged - * at Info level with the overrides in effect and what each file supplied. + * failure is logged, and the source retries after a short delay. A JSON file with no content + * cannot be parsed. A file that had content and is found empty may be in the middle of an + * in-place rewrite, so it is read again after the retry delay before it is applied. Every applied + * change is logged at Info level with the overrides in effect and what each file supplied. * * Flag overrides are currently experimental and subject to change. * diff --git a/packages/shared/sdk-server/src/overrides/overrideDocument.ts b/packages/shared/sdk-server/src/overrides/overrideDocument.ts index 80459806b5..fc10c199c7 100644 --- a/packages/shared/sdk-server/src/overrides/overrideDocument.ts +++ b/packages/shared/sdk-server/src/overrides/overrideDocument.ts @@ -33,7 +33,8 @@ function validateItems(document: Record, member: 'flags' | 'segment function validateDocument(parsed: any): FileDataDocument { if (isNullish(parsed)) { - // An empty file is an empty document. + // A YAML parser returns null or undefined for a file with no content, which is a document with + // no entries. A JSON file with no content fails to parse instead. return {}; } if (!isPlainObject(parsed)) { @@ -65,7 +66,8 @@ function validateDocument(parsed: any): FileDataDocument { * * The result is validated: the document and its `flags`, `flagValues`, and `segments` members * must be objects, every flag and segment entry must be an object, and an entry that omits its - * `key` gets it from the map key. An empty document is a document with no members. + * `key` gets it from the map key. A YAML file with no content is a document with no members, and + * a JSON file with no content fails to parse. * * @internal */ From 87891638d254225d73028981bf9933189f26bb03 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:59:49 +0000 Subject: [PATCH 02/12] fix: Hold back an empty file until it has stayed empty, unwritten, for the retry delay Reading an emptied file again once, keyed on whether the last read found content, still applied an empty file in three in-place rewrite sequences, because a new truncation that no load read in between looks like a file that stayed empty: two quick rewrites in one settle window, a change to another file while one file is mid-rewrite, and polling with two rewrites about a second apart. The reloader now remembers, per file, whether the file contributed content to the data in effect, and the empty episode of a file whose content is in effect: when a load first found it empty, and since when it has been empty without being written, by its modification time. Such a file is held back until it has stayed empty, without being written, for one retry delay, and at the latest five retry delays after a load first found it empty. The retry is armed for the end of that delay. A file whose content is not in effect, such as a placeholder, is parsed right away, as is any empty file when retries are off. --- .../LDClientNode.fileDataSource.test.ts | 63 +++- .../LDClientNode.fileOverrides.test.ts | 85 ++++- .../FileReloader.inPlaceRewrite.test.ts | 292 ++++++++++++++---- .../api/integrations/FileDataSourceOptions.ts | 9 +- .../src/api/options/LDDataSystemOptions.ts | 12 +- .../src/data_sources/filedata/FileReloader.ts | 153 +++++++-- .../src/overrides/FileOverrideSource.ts | 7 +- 7 files changed, 490 insertions(+), 131 deletions(-) diff --git a/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts index d950767f53..913eec2a60 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.fileDataSource.test.ts @@ -3,7 +3,7 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { parse as parseYaml } from 'yaml'; -import { integrations } from '@launchdarkly/js-server-sdk-common'; +import { integrations, sleep } from '@launchdarkly/js-server-sdk-common'; import LDClientNode from '../src/LDClientNode'; import { rewriteInPlace, sampleValues } from './inPlaceWrites'; @@ -195,11 +195,17 @@ describe('given a file data source that reloads a file rewritten in place', () = await rm(directory, { recursive: true, force: true }); }); - it.each(['json', 'yaml'])('never serves the emptied %s file', async (extension) => { - // The file is empty for 300 ms, longer than the settle window after a change, so a reload - // reads it empty. The client must keep serving the old value until the new one is written. + const flagsDocument = (value: string) => JSON.stringify({ flagValues: { flag1: value } }); + + // Starts a client whose data file serves 'a', runs the rewrites while it samples what the client + // serves, and returns what it served until it served the final value, and the errors it logged. + const sampleRewrites = async ( + extension: string, + rewrites: (path: string) => Promise, + finalValue: string, + ) => { const path = join(directory, `flags.${extension}`); - await writeFile(path, JSON.stringify({ flagValues: { flag1: 'a' } })); // JSON is also YAML + await writeFile(path, flagsDocument('a')); // JSON is also YAML const errors: string[] = []; const fds = new integrations.FileDataSourceFactory({ paths: [path], @@ -223,19 +229,52 @@ describe('given a file data source that reloads a file rewritten in place', () = const sampler = sampleValues(() => client.variation('flag1', { key: 'user1' }, 'default')); try { - await rewriteInPlace(path, JSON.stringify({ flagValues: { flag1: 'b' } }), 300); - // Waits for the sampler itself to see the new value, so it has recorded everything that + await rewrites(path); + // Waits for the sampler itself to see the final value, so it has recorded everything that // the client served before it. - await waitFor(() => sampler.served.includes('b'), 10000); + await waitFor(() => sampler.served.includes(finalValue), 10000); } finally { await sampler.stop(); } - - expect(sampler.served).toEqual(['a', 'b']); - // The file was read while it was empty, which is not an error to report. - expect(errors).toEqual([]); + return { served: sampler.served, errors }; } finally { client.close(); } + }; + + it.each(['json', 'yaml'])('never serves the emptied %s file', async (extension) => { + // The file is empty for 300 ms, longer than the settle window after a change, so a reload + // reads it empty. The client must keep serving the old value until the new one is written. + const { served, errors } = await sampleRewrites( + extension, + (path) => rewriteInPlace(path, flagsDocument('b'), 300), + 'b', + ); + + expect(served).toEqual(['a', 'b']); + // The file was read while it was empty, which is not an error to report. + expect(errors).toEqual([]); }); + + it.each(['json', 'yaml'])( + 'never serves the %s file emptied by two in-place rewrites in a row', + async (extension) => { + // The first rewrite's write and the second's truncation fall in one settle window, so the + // load after them finds the file empty again. + const { served, errors } = await sampleRewrites( + extension, + async (path) => { + await rewriteInPlace(path, flagsDocument('b'), 300); + await sleep(20); + await rewriteInPlace(path, flagsDocument('c'), 300); + }, + 'c', + ); + + expect(served).not.toContain('default'); + expect(served[0]).toEqual('a'); + expect(served[served.length - 1]).toEqual('c'); + expect(errors).toEqual([]); + }, + ); }); diff --git a/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts index c8caa202bb..fed806e578 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts @@ -167,10 +167,14 @@ describe('given a temporary directory of override files', () => { }) as any); }; - it('keeps the override while the file is rewritten in place', async () => { - const path = join(directory, `overrides.${extension}`); + let path: string; + let errors: string[]; + + // Starts a client whose override file serves 'a'. + const start = async () => { + path = join(directory, `overrides.${extension}`); await writeFile(path, document('a')); // JSON is also YAML - const errors: string[] = []; + errors = []; recordPolls(path); client = makeClient( directory, @@ -181,30 +185,79 @@ describe('given a temporary directory of override files', () => { async () => (await client!.variation('overridden-flag', user, 'default')) === 'a', 10000, ); + }; + + // Waits for the next poll and returns when it happened. + const nextPoll = async () => { + const count = polls.length; + await waitFor(() => polls.length > count, 5000, 1); + return polls[polls.length - 1]; + }; + + // Samples what the client serves while the rewrites run, until it serves the final value. + const sampleWhile = async (rewrite: () => Promise, finalValue: string) => { + const sampler = sampleValues(() => client!.variation('overridden-flag', user, 'default')); + try { + await rewrite(); + // Waits for the sampler itself to see the final value, so it has recorded everything that + // the client served before it. + await waitFor(() => sampler.served.includes(finalValue), 10000); + } finally { + await sampler.stop(); + } + return sampler.served; + }; + + const loadFailures = () => + errors.filter((message) => message.startsWith('Unable to load flags')); + + it('keeps the override while the file is rewritten in place', async () => { + await start(); let pauseMs = 300; if (changeDetection === 'polling') { // The rewrite starts 750 ms after a poll, so the next poll finds the file empty, and its // reload 100 ms later still does. The file is written 250 ms after that, well before the // retry reads it again a second after the reload. - const count = polls.length; - await waitFor(() => polls.length > count, 5000, 1); - await sleep(polls[polls.length - 1] + 750 - Date.now()); + await sleep((await nextPoll()) + 750 - Date.now()); pauseMs = 600; } // In watching mode, the reload that the truncation starts runs after the 100 ms settle // window, while the file is still empty. - const sampler = sampleValues(() => client!.variation('overridden-flag', user, 'default')); - try { - await rewriteInPlace(path, document('b'), pauseMs); - // Waits for the sampler itself to see the new value, so it has recorded everything that - // the client served before it. - await waitFor(() => sampler.served.includes('b'), 10000); - } finally { - await sampler.stop(); + const served = await sampleWhile(() => rewriteInPlace(path, document('b'), pauseMs), 'b'); + + expect(served).toEqual(['a', 'b']); + expect(loadFailures()).toEqual([]); + }); + + it('keeps the override through two in-place rewrites in a row', async () => { + await start(); + let rewrites: () => Promise; + if (changeDetection === 'polling') { + // The first rewrite starts 750 ms after a poll, so the next poll finds the file empty. + // The second starts 1900 ms after that poll, before the next poll and the retry, so they + // find the file empty again before any load has read the first rewrite's content. + const poll = await nextPoll(); + await sleep(poll + 750 - Date.now()); + rewrites = async () => { + await rewriteInPlace(path, document('b'), 600); + await sleep(poll + 1900 - Date.now()); + await rewriteInPlace(path, document('c'), 600); + }; + } else { + // The first rewrite's write and the second's truncation fall in one settle window, so the + // load after them finds the file empty again. + rewrites = async () => { + await rewriteInPlace(path, document('b'), 300); + await sleep(20); + await rewriteInPlace(path, document('c'), 300); + }; } + const served = await sampleWhile(rewrites, 'c'); - expect(sampler.served).toEqual(['a', 'b']); - expect(errors.filter((message) => message.startsWith('Unable to load flags'))).toEqual([]); + expect(served).not.toContain('default'); + expect(served[0]).toEqual('a'); + expect(served[served.length - 1]).toEqual('c'); + expect(loadFailures()).toEqual([]); }); }); }); diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index 93341b7f44..fc8d6cbd10 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -21,12 +21,14 @@ const retryDelayMs = 1000; const documentA = '{"flagValues": {"flag1": "a"}}'; const documentB = '{"flagValues": {"flag1": "b"}}'; +const documentC = '{"flagValues": {"flag1": "c"}}'; // A stand-in for a YAML parser. A flow-style document like the ones above is both JSON and YAML, // and like the YAML parsers, it returns null for a file with no content. const yamlParser = (data: string) => (hasContent(data) ? JSON.parse(data) : null); const emptiedReadAgain = /is empty, possibly while it is being rewritten; reading it again/; +const emptyJson = 'the file has no content, which is not a JSON document'; describe('given a reloader over a file that is rewritten in place', () => { let filesystem: MockFilesystem; @@ -34,6 +36,7 @@ describe('given a reloader over a file that is rewritten in place', () => { let applied: ReloadResult[]; let failures: LoadFailure[]; let reloader: FileReloader; + let modified: number; beforeEach(() => { jest.useFakeTimers(); @@ -41,6 +44,7 @@ describe('given a reloader over a file that is rewritten in place', () => { logger = new TestLogger(); applied = []; failures = []; + modified = 0; }); afterEach(() => { @@ -48,6 +52,13 @@ describe('given a reloader over a file that is rewritten in place', () => { jest.useRealTimers(); }); + // Every write, including a truncation, changes the file's modification time, as it does on a + // real filesystem. + const write = (path: string, data: string) => { + modified += 1; + filesystem.set(path, data, modified); + }; + // The policy parses by extension and validates the document as the override source does, so a // YAML file with no content is a document with no entries and a JSON file with no content fails. const makeReloader = (paths: string[], configure?: (config: FileReloaderConfig) => void) => { @@ -78,111 +89,260 @@ describe('given a reloader over a file that is rewritten in place', () => { await jest.advanceTimersByTimeAsync(0); }; + const advance = (ms: number) => jest.advanceTimersByTimeAsync(ms); + it.each(['', ' \n\t\r\n', '\uFEFF'])( 'reads an emptied file again and applies the rewritten content with no empty application, for content %j', async (empty) => { - filesystem.set(yamlPath, documentA); + write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); - filesystem.set(yamlPath, empty); + write(yamlPath, empty); await changeAndSettle(); expect(values()).toEqual(['a']); expect(failures).toEqual([]); logger.expectMessages([{ level: LogLevel.Debug, matches: emptiedReadAgain }]); - filesystem.set(yamlPath, documentB); + write(yamlPath, documentB); await changeAndSettle(); expect(values()).toEqual(['a', 'b']); expect(failures).toEqual([]); }, ); - it('applies a YAML file that stays empty when the retry reads it again', async () => { - filesystem.set(yamlPath, documentA); + it('applies a YAML file that stays empty once it has been empty for the retry delay', async () => { + write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); - filesystem.set(yamlPath, ''); + write(yamlPath, ''); await changeAndSettle(); expect(values()).toEqual(['a']); - await jest.advanceTimersByTimeAsync(retryDelayMs - 1); + await advance(retryDelayMs - 1); expect(values()).toEqual(['a']); - await jest.advanceTimersByTimeAsync(1); + await advance(1); expect(values()).toEqual(['a', '']); expect(failures).toEqual([]); }); it('does not report an emptied JSON file at the load that finds it empty, and the retry reports it', async () => { - filesystem.set(jsonPath, documentA); + write(jsonPath, documentA); makeReloader([jsonPath]); await reloader.reloadNow(); - filesystem.set(jsonPath, ''); + write(jsonPath, ''); await changeAndSettle(); expect(failures).toEqual([]); - await jest.advanceTimersByTimeAsync(retryDelayMs); + await advance(retryDelayMs); expect(failures).toHaveLength(1); expect(failures[0]).toMatchObject({ kind: 'parse', path: jsonPath, repeated: false }); - expect(failures[0].error.message).toEqual( - 'the file has no content, which is not a JSON document', - ); + expect(failures[0].error.message).toEqual(emptyJson); + expect(values()).toEqual(['a']); + }); + + it('does not apply a file that stays empty at a further change signal within the retry delay', async () => { + write(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(500); + await changeAndSettle(); + expect(values()).toEqual(['a']); + + // The file has been empty, without being written, for the retry delay since a load first found + // it empty. The retry that the further change signal armed applies it then, not a full retry + // delay after that signal. + await advance(retryDelayMs - 500 - 1); + expect(values()).toEqual(['a']); + await advance(1); + expect(values()).toEqual(['a', '']); + }); + + it('restarts the delay when the file is written and truncated again between two loads', async () => { + // Two quick rewrites in watching mode: the write of the first and the truncation of the second + // fall in one settle window, so no load sees the first one's content. + write(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a']); + + // The first emptying alone would be applied now. The second restarted the delay. + await advance(retryDelayMs - 600); expect(values()).toEqual(['a']); + + write(yamlPath, documentC); + await changeAndSettle(); + expect(values()).toEqual(['a', 'c']); + expect(failures).toEqual([]); }); - it('applies a file that stays empty at the next change signal, before the retry would', async () => { - filesystem.set(yamlPath, documentA); + it('does not apply a file that is mid-rewrite when another file changes', async () => { + write(yamlPath, documentA); + write(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(300); + write(otherYamlPath, '{"flagValues": {"flag2": "b"}}'); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + + await advance(200); + write(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a,a', 'b,b']); + expect(failures).toEqual([]); + }); + + it('does not apply the truncation of a later rewrite that the retry finds', async () => { + // Polling with two rewrites about a second apart: no load reads the first one's content + // before the second truncation. + write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); - filesystem.set(yamlPath, ''); + write(yamlPath, ''); await changeAndSettle(); + await advance(400); + write(yamlPath, documentB); + await advance(500); + write(yamlPath, ''); + await advance(100); expect(values()).toEqual(['a']); + expect(readAgainMessages()).toHaveLength(2); - // A stream of change signals cannot hold the data back: the file was empty at its last read. + write(yamlPath, documentC); await changeAndSettle(); + expect(values()).toEqual(['a', 'c']); + }); + + it('applies a file that keeps being rewritten so that every load finds it empty, five retry delays after a load first found it empty', async () => { + write(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + // Every 600 ms the file is written and truncated again before a load reads it. + await Array.from({ length: 8 }).reduce>( + (previous) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + }), + Promise.resolve(), + ); + expect(values()).toEqual(['a']); + + await advance(199); + expect(values()).toEqual(['a']); + await advance(1); expect(values()).toEqual(['a', '']); }); - it('never reads an empty placeholder file again, and it does not delay a change to another file', async () => { - filesystem.set(otherYamlPath, ''); - filesystem.set(yamlPath, documentA); + it('starts a new delay for a file that is emptied again after its content was read', async () => { + write(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(300); + write(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + + // Much later, and within five retry delays of the first emptying, the file is emptied again. + await advance(4600); + write(yamlPath, ''); + await changeAndSettle(); + await advance(retryDelayMs - 1); + expect(values()).toEqual(['a', 'b']); + await advance(1); + expect(values()).toEqual(['a', 'b', '']); + }); + + it('starts a new delay for a file that is deleted and created empty again', async () => { + write(jsonPath, documentA); + makeReloader([jsonPath], (config) => { + config.policy = testPolicy({ missingFile: 'fail' }); + }); + await reloader.reloadNow(); + + write(jsonPath, ''); + await changeAndSettle(); + await advance(100); + filesystem.remove(jsonPath); + await changeAndSettle(); + // The retries keep failing to read the missing file, as repeats. + const reported = () => failures.filter((failure) => !failure.repeated); + expect(reported().map((failure) => failure.kind)).toEqual(['read']); + + // The data in effect still has the file's content, so the empty file is held back, for a full + // retry delay from when it is found empty again. + await advance(4400); + write(jsonPath, ''); + await changeAndSettle(); + await advance(retryDelayMs - 1); + expect(reported().map((failure) => failure.kind)).toEqual(['read']); + await advance(1); + expect(reported().map((failure) => failure.kind)).toEqual(['read', 'parse']); + expect(reported()[1].error.message).toEqual(emptyJson); + }); + + it('never holds back an empty placeholder file, and it does not delay a change to another file', async () => { + write(otherYamlPath, ''); + write(yamlPath, documentA); makeReloader([otherYamlPath, yamlPath]); await reloader.reloadNow(); expect(values()).toEqual(['a']); - filesystem.set(yamlPath, documentB); + write(yamlPath, documentB); await changeAndSettle(); expect(values()).toEqual(['a', 'b']); expect(readAgainMessages()).toEqual([]); }); - it('reads each of two files that are rewritten together again', async () => { - filesystem.set(yamlPath, documentA); - filesystem.set(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + it('holds back each of two files that are rewritten together', async () => { + write(yamlPath, documentA); + write(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); makeReloader([yamlPath, otherYamlPath]); await reloader.reloadNow(); expect(values()).toEqual(['a,a']); - filesystem.set(yamlPath, ''); - filesystem.set(otherYamlPath, ''); + write(yamlPath, ''); + write(otherYamlPath, ''); await changeAndSettle(); - // The first file is written. The second is still empty, after it had content. - filesystem.set(yamlPath, documentB); + // The first file is written. The second is still empty. + write(yamlPath, documentB); await changeAndSettle(); expect(values()).toEqual(['a,a']); - filesystem.set(otherYamlPath, '{"flagValues": {"flag2": "b"}}'); + write(otherYamlPath, '{"flagValues": {"flag2": "b"}}'); await changeAndSettle(); expect(values()).toEqual(['a,a', 'b,b']); expect(failures).toEqual([]); }); it('applies a file that is deleted and then created empty right away', async () => { - filesystem.set(yamlPath, documentA); + write(yamlPath, documentA); makeReloader([yamlPath], (config) => { config.skipUnchanged = false; }); @@ -192,100 +352,116 @@ describe('given a reloader over a file that is rewritten in place', () => { await changeAndSettle(); expect(applied[1].files).toEqual([{ path: yamlPath, present: false, flags: 0, segments: 0 }]); - filesystem.set(yamlPath, ''); + write(yamlPath, ''); await changeAndSettle(); expect(applied).toHaveLength(3); expect(applied[2].files).toEqual([{ path: yamlPath, present: true, flags: 0, segments: 0 }]); }); - it('reads an emptied file again when a retry left over from a failure finds it', async () => { - filesystem.set(yamlPath, documentA); + it('holds back an emptied file when a retry left over from a failure finds it', async () => { + write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); // A malformed edit fails the reload and arms the retry. - filesystem.set(yamlPath, '{"flagValues": '); + write(yamlPath, '{"flagValues": '); await changeAndSettle(); expect(failures).toHaveLength(1); // The file is rewritten in place, and the retry runs before the new content is written. - filesystem.set(yamlPath, ''); - await jest.advanceTimersByTimeAsync(retryDelayMs); + write(yamlPath, ''); + await advance(retryDelayMs); expect(values()).toEqual(['a']); expect(failures).toHaveLength(1); - filesystem.set(yamlPath, documentB); - await jest.advanceTimersByTimeAsync(retryDelayMs); + write(yamlPath, documentB); + await changeAndSettle(); expect(values()).toEqual(['a', 'b']); }); it('parses an emptied file right away when retries are off', async () => { - filesystem.set(yamlPath, documentA); + write(yamlPath, documentA); makeReloader([yamlPath], (config) => { config.retryDelayMs = 0; }); await reloader.reloadNow(); - filesystem.set(yamlPath, ''); + write(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a', '']); + }); + + it('does not hold back an empty file again once its empty state is in effect', async () => { + write(yamlPath, documentA); + makeReloader([yamlPath], (config) => { + config.skipUnchanged = false; + }); + await reloader.reloadNow(); + + write(yamlPath, ''); await changeAndSettle(); + await advance(retryDelayMs); expect(values()).toEqual(['a', '']); + + // The file is written empty again: nothing it supplied is in effect, so nothing is held back. + write(yamlPath, ' '); + await changeAndSettle(); + expect(values()).toEqual(['a', '', '']); }); - it('does not count reading again as a failure, so an unchanged rewrite applies nothing', async () => { - filesystem.set(yamlPath, documentA); + it('does not count holding back as a failure, so an unchanged rewrite applies nothing', async () => { + write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); - filesystem.set(yamlPath, ''); + write(yamlPath, ''); await changeAndSettle(); - filesystem.set(yamlPath, documentA); + write(yamlPath, documentA); await changeAndSettle(); - // A reload after a failure applies even unchanged content. Reading again is not a failure. + // A reload after a failure applies even unchanged content. Holding back is not a failure. expect(values()).toEqual(['a']); expect(failures).toEqual([]); }); it('reports a failure after reading again as news when the load that found the file empty followed a change', async () => { - filesystem.set(jsonPath, documentA); + write(jsonPath, documentA); makeReloader([jsonPath]); await reloader.reloadNow(); // A malformed edit fails, and the retries keep failing the same way. - filesystem.set(jsonPath, '{"flagValues": '); + write(jsonPath, '{"flagValues": '); await changeAndSettle(); - await jest.advanceTimersByTimeAsync(retryDelayMs); + await advance(retryDelayMs); expect(failures.map((failure) => failure.repeated)).toEqual([false, true]); // The file is emptied, which is a change. The retry that reads it again finds it still empty. // That failure is news, as it would have been for the load that the change started. - filesystem.set(jsonPath, ''); + write(jsonPath, ''); await changeAndSettle(); expect(failures).toHaveLength(2); - await jest.advanceTimersByTimeAsync(retryDelayMs); + await advance(retryDelayMs); expect(failures).toHaveLength(3); expect(failures[2].repeated).toBe(false); - expect(failures[2].error.message).toEqual( - 'the file has no content, which is not a JSON document', - ); + expect(failures[2].error.message).toEqual(emptyJson); logger.expectMessages([ { level: LogLevel.Debug, matches: /Reading flag data again after a file was found empty/ }, ]); }); it('does not read again after close', async () => { - filesystem.set(yamlPath, documentA); + write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); - filesystem.set(yamlPath, ''); + write(yamlPath, ''); filesystem.deferReads = true; reloader.trigger(); - await jest.advanceTimersByTimeAsync(0); + await advance(0); expect(filesystem.pendingReads).toHaveLength(1); reloader.close(); filesystem.completePendingReads(); - await jest.advanceTimersByTimeAsync(retryDelayMs * 2); + await advance(retryDelayMs * 2); expect(filesystem.readCount).toEqual(2); expect(readAgainMessages()).toEqual([]); diff --git a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts index 6a52b3401b..d00410f461 100644 --- a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts +++ b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts @@ -17,10 +17,11 @@ export interface FileDataSourceOptions { * A file that is replaced by a rename, or deleted and created again, is detected. A load * that fails, for example because a file was read while it was written, is retried after a * short delay and the previously loaded data stays in effect until it succeeds. - * A file can be rewritten in place: a file that had content and is found empty is read again - * after the same delay before it is applied. YAML that is cut off between two entries can still - * parse, so for a YAML file, replacing its entire contents in one operation, such as renaming a - * fully written temporary file over it, avoids briefly applying only a part of it. + * A file can be rewritten in place: a file whose data is in effect and that is found empty is + * not applied until it has stayed empty, without being written, for the same delay. YAML that is + * cut off between two entries can still parse, so for a YAML file, replacing its entire contents + * in one operation, such as renaming a fully written temporary file over it, avoids briefly + * applying only a part of it. * A change to another entry in a directory causes a reload only when the metadata of a * configured file changed, read through any symbolic link, which is how a mounted ConfigMap * or Secret is updated. diff --git a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts index d7b72d1d97..a9ceb2f943 100644 --- a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts +++ b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts @@ -121,12 +121,12 @@ export interface LDDataSystemOptions { * * A file can be rewritten in place. To remove a file's overrides, delete it or write a document * with no entries, such as `{}`. A JSON file with no content is not a valid document: it fails to - * load, and the overrides in effect stay. A file that had content and is found empty may be in - * the middle of a rewrite, so the source waits about a second and reads it again before applying - * it. An emptied YAML file therefore removes its overrides about a second later. YAML that is cut - * off between two entries can still parse, so for a YAML file, replacing its entire contents in - * one operation, such as renaming a fully written temporary file over it, avoids briefly applying - * only a part of it. + * load, and the overrides in effect stay. A file whose overrides are in effect and that is found + * empty may be in the middle of a rewrite, so the source applies it only once it has stayed empty, + * without being written, for about a second. An emptied YAML file therefore removes its overrides + * about a second later. YAML that is cut off between two entries can still parse, so for a YAML + * file, replacing its entire contents in one operation, such as renaming a fully written temporary + * file over it, avoids briefly applying only a part of it. * * @example * ```typescript diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 80e7da056e..90a657ce86 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -31,6 +31,14 @@ export const DEFAULT_RETRY_DELAY_MS = 1000; */ export const MAX_DEBOUNCE_DELAY_MS = 1000; +/** + * A file that keeps being written so that every reload finds it empty is applied at the latest + * this many retry delays after a reload first found it empty. + * + * @internal + */ +export const MAX_EMPTY_FILE_RETRY_DELAYS = 5; + /** * The result of one successful reload: the merged items plus a summary of each configured * file. @@ -104,9 +112,10 @@ export interface FileReloaderConfig { */ debounceDelayMs: number; /** - * How long to wait after a failed reload before retrying it without a trigger. A file that had - * content and is found empty is read again after the same delay before it is applied. Zero or - * negative disables the automatic retry, and an empty file is then parsed as it is. + * How long to wait after a failed reload before retrying it without a trigger. A file whose + * content is in effect and that is found empty is not applied until it has stayed empty, without + * being written, for the same delay. Zero or negative disables the automatic retry, and an empty + * file is then parsed as it is. */ retryDelayMs: number; /** @@ -118,6 +127,18 @@ export interface FileReloaderConfig { type ReloadReason = 'initial' | 'change' | 'retry'; +/** + * A file whose content is in effect and that reloads have found empty. `firstSeen` is when a + * reload first found it empty and `since` is since when it has been empty without being written, + * both on the monotonic clock. `modified` is the file's modification time when a reload last found + * it empty, or undefined when it could not be read. + */ +interface EmptyFile { + firstSeen: number; + since: number; + modified?: number; +} + function asError(err: unknown): Error { return err instanceof Error ? err : new Error(String(err)); } @@ -128,8 +149,8 @@ function asError(err: unknown): Error { * failures, and skips no-op applications. * * A file that is rewritten in place is truncated and then written, so a reload can find it - * empty. A reload that finds a file empty after it had content keeps the current data and reads - * the file again after the retry delay, whatever the file's format. See `_readAgainLater`. + * empty. A reload that finds a file empty whose content is in effect keeps the current data and + * reads the file again later, whatever the file's format. See `_emptyFileGraceLeft`. * * @internal */ @@ -158,11 +179,14 @@ export default class FileReloader { private _lastFailureKey?: string; - // Whether the last read of each configured path found content. A missing file counts as no - // content. - private readonly _lastReadHadContent = new Map(); + // Whether each configured path contributed content to the data in effect, that is, to the last + // reload that was applied or skipped as unchanged. A missing or empty file contributed none. + private readonly _appliedHadContent = new Map(); + + // The paths whose content is in effect and that reloads have found empty since. + private readonly _emptyFiles = new Map(); - // Set by a reload that found a file emptied and armed the retry to read it again, to that + // Set by a reload that held back an empty file and armed the retry to read it again, to that // reload's reason, until the next reload starts. The next reload continues the deferred one, so // it reports a failure the way the deferred reload would have. private _readAgainReason?: ReloadReason; @@ -255,7 +279,7 @@ export default class FileReloader { } } - private _armRetry(): void { + private _armRetry(delayMs: number = this._config.retryDelayMs): void { if (this._closed || this._config.retryDelayMs <= 0) { return; } @@ -263,7 +287,7 @@ export default class FileReloader { this._retryTimer = setTimeout(() => { this._retryTimer = undefined; this._enqueue('retry'); - }, this._config.retryDelayMs); + }, delayMs); } /** @@ -296,13 +320,15 @@ export default class FileReloader { const documents: FileDataDocument[] = []; const files: FileSummary[] = []; const contents: string[] = []; + // Whether each path that this reload read, or found missing, has content. + const hadContent = new Map(); let failure: LoadFailure | undefined; - let emptied: string | undefined; + let heldBack: { path: string; graceLeftMs: number } | undefined; const readAll = paths.reduce( (previous, path) => previous.then(async () => { - if (failure || emptied !== undefined) { + if (failure || heldBack) { return; } let data: string; @@ -311,22 +337,27 @@ export default class FileReloader { } catch (err) { const missing = isFileNotFoundError(err); if (missing) { - this._lastReadHadContent.set(path, false); + this._emptyFiles.delete(path); } if (missing && policy.missingFile === 'skip') { logger?.debug(`File ${path} does not exist. It contributes no data.`); + hadContent.set(path, false); files.push({ path, present: false, flags: 0, segments: 0 }); return; } failure = { kind: 'read', error: asError(err), path, repeated: false }; return; } - const nonEmpty = hasContent(data); - const hadContent = this._lastReadHadContent.get(path) === true; - this._lastReadHadContent.set(path, nonEmpty); - if (!nonEmpty && hadContent && this._config.retryDelayMs > 0) { - emptied = path; - return; + if (hasContent(data)) { + this._emptyFiles.delete(path); + hadContent.set(path, true); + } else { + const graceLeftMs = await this._emptyFileGraceLeft(path); + if (graceLeftMs > 0) { + heldBack = { path, graceLeftMs }; + return; + } + hadContent.set(path, false); } contents.push(data); try { @@ -341,8 +372,8 @@ export default class FileReloader { ); await readAll; - if (emptied !== undefined) { - this._readAgainLater(emptied, reason); + if (heldBack) { + this._readAgainLater(heldBack.path, heldBack.graceLeftMs, reason); return; } @@ -381,6 +412,7 @@ export default class FileReloader { const content = JSON.stringify(contents); if (this._config.skipUnchanged && !recovering && content === this._lastGoodContent) { this._lastFailureKey = undefined; + this._rememberAppliedContent(hadContent); return; } // Nothing is remembered until the consumer has accepted the result. A result the consumer @@ -399,19 +431,71 @@ export default class FileReloader { this._lastFailureKey = undefined; this._lastGoodContent = content; this._lastGoodFlags = nextFlags; + this._rememberAppliedContent(hadContent); + } + + private _rememberAppliedContent(hadContent: Map): void { + hadContent.forEach((value, path) => this._appliedHadContent.set(path, value)); } /** - * A file that had content when it was last read and is empty now may be in the middle of an - * in-place rewrite: a writer that truncates the file and then writes the new content. Whatever - * the file's format, the reload keeps the current data and arms the retry without reporting a - * failure, so the file is read again after the retry delay. The change signal for the write - * starts a reload sooner. That next reload parses what the file holds then, because the file - * was empty when it was last read: a file that is still empty is invalid JSON, or a YAML - * document with no entries. So the current data is kept for at most one more reload, and a file - * that is empty from the start, such as a placeholder, is never read again. + * Decides what to do with a file that a reload found empty. Returns how long to hold it back, + * or zero to parse it now. + * + * A file whose content is in effect and that is empty now may be in the middle of an in-place + * rewrite: a writer that truncates the file and then writes the new content. Such a file is not + * applied until it has stayed empty, without being written, for one retry delay. A change of its + * modification time means that it was written since a reload last found it empty, for example + * rewritten and truncated again between two reloads, so the delay starts over. Modification + * times are only compared with each other, never with the clock. A file that keeps being written + * so that every reload finds it empty is applied at the latest `MAX_EMPTY_FILE_RETRY_DELAYS` + * retry delays after a reload first found it empty. A file whose content is not in effect, such + * as a placeholder that has always been empty, is parsed right away, and so is any empty file + * when retries are off, since nothing would read it again. */ - private _readAgainLater(path: string, reason: ReloadReason): void { + private async _emptyFileGraceLeft(path: string): Promise { + const { retryDelayMs } = this._config; + if (retryDelayMs <= 0 || this._appliedHadContent.get(path) !== true) { + return 0; + } + const modified = await this._modifiedTime(path); + const now = monotonicNow(); + const empty = this._emptyFiles.get(path); + if (!empty) { + this._emptyFiles.set(path, { firstSeen: now, since: now, modified }); + return retryDelayMs; + } + if (empty.modified !== modified) { + empty.since = now; + empty.modified = modified; + } + const until = Math.min( + empty.since + retryDelayMs, + empty.firstSeen + MAX_EMPTY_FILE_RETRY_DELAYS * retryDelayMs, + ); + return Math.max(until - now, 0); + } + + private async _modifiedTime(path: string): Promise { + const { filesystem } = this._config; + try { + if (filesystem.getFileStats) { + return (await filesystem.getFileStats(path))?.timestamp; + } + return await filesystem.getFileTimestamp(path); + } catch { + return undefined; + } + } + + /** + * Holds back a file that `_emptyFileGraceLeft` decided to hold back. Whatever the file's format, + * the reload keeps the current data and arms the retry for when the file will have stayed empty + * for the retry delay, without reporting a failure. The change signal for the write starts a + * reload sooner. Once the file has stayed empty for the retry delay, a reload parses it: a file + * that is still empty is invalid JSON, or a YAML document with no entries. + */ + private _readAgainLater(path: string, graceLeftMs: number, reason: ReloadReason): void { if (this._closed) { return; } @@ -419,7 +503,12 @@ export default class FileReloader { `File ${path} is empty, possibly while it is being rewritten; reading it again before applying it`, ); this._readAgainReason = reason; - this._armRetry(); + // The timer is armed for the end of the grace, not for a full retry delay from now, so that a + // file that stays empty is applied one retry delay after a reload found it empty, however many + // reloads held it back in between. A timer can fire a fraction of a millisecond before the + // monotonic clock reaches its delay; the reload it starts then holds the file back for that + // fraction again. + this._armRetry(Math.ceil(graceLeftMs)); } private _fail(failure: LoadFailure, reason: ReloadReason): void { diff --git a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts index e41a8d3ef4..3d9124bf67 100644 --- a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts +++ b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts @@ -144,9 +144,10 @@ function countsText(flags: number, segments: number): string { * no overrides, so deleting a file removes its overrides. A file that exists but cannot be read * or parsed makes that whole reload fail. The previously loaded overrides stay in effect, the * failure is logged, and the source retries after a short delay. A JSON file with no content - * cannot be parsed. A file that had content and is found empty may be in the middle of an - * in-place rewrite, so it is read again after the retry delay before it is applied. Every applied - * change is logged at Info level with the overrides in effect and what each file supplied. + * cannot be parsed. A file whose overrides are in effect and that is found empty may be in the + * middle of an in-place rewrite, so it is applied only once it has stayed empty, without being + * written, for the retry delay. Every applied change is logged at Info level with the overrides + * in effect and what each file supplied. * * Flag overrides are currently experimental and subject to change. * From 7e0fcb5581ca5c50423f81a1c8d1cf8f73ab7469 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 00:12:12 +0000 Subject: [PATCH 03/12] fix: Classify every file before holding back, and check the hold with a slack A reload that holds back one emptied file now keeps reading the others, so that files emptied together start their delays in the same reload instead of one retry delay after another. A file that cannot be read or parsed still fails the reload. The retry that the hold arms comes one retry delay later and a timer can fire slightly early, so the hold is checked against nine tenths of the retry delay; a file that stays empty is applied by that retry. The retry is armed for the usual delay again. --- .../FileReloader.inPlaceRewrite.test.ts | 90 +++++++++++++++---- .../src/data_sources/filedata/FileReloader.ts | 81 ++++++++--------- 2 files changed, 111 insertions(+), 60 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index fc8d6cbd10..7b7e14cfbb 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -127,6 +127,23 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(failures).toEqual([]); }); + it('applies a file that stays empty at a load nine tenths of a retry delay after it was found empty', async () => { + // The retry comes a retry delay after the hold, and a timer can fire slightly early. The slack + // makes sure that the retry applies a file that stayed empty. + write(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(retryDelayMs * 0.9 - 1); + await changeAndSettle(); + expect(values()).toEqual(['a']); + await advance(1); + await changeAndSettle(); + expect(values()).toEqual(['a', '']); + }); + it('does not report an emptied JSON file at the load that finds it empty, and the retry reports it', async () => { write(jsonPath, documentA); makeReloader([jsonPath]); @@ -154,10 +171,8 @@ describe('given a reloader over a file that is rewritten in place', () => { await changeAndSettle(); expect(values()).toEqual(['a']); - // The file has been empty, without being written, for the retry delay since a load first found - // it empty. The retry that the further change signal armed applies it then, not a full retry - // delay after that signal. - await advance(retryDelayMs - 500 - 1); + // The retry that the further change signal armed applies the file a retry delay later. + await advance(retryDelayMs - 1); expect(values()).toEqual(['a']); await advance(1); expect(values()).toEqual(['a', '']); @@ -230,7 +245,7 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a', 'c']); }); - it('applies a file that keeps being rewritten so that every load finds it empty, five retry delays after a load first found it empty', async () => { + it('applies a file that keeps being rewritten so that every load finds it empty, once five retry delays have passed since a load first found it empty', async () => { write(yamlPath, documentA); makeReloader([yamlPath]); await reloader.reloadNow(); @@ -238,21 +253,20 @@ describe('given a reloader over a file that is rewritten in place', () => { write(yamlPath, ''); await changeAndSettle(); // Every 600 ms the file is written and truncated again before a load reads it. + const rewriteBeforeALoad = async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + }; await Array.from({ length: 8 }).reduce>( - (previous) => - previous.then(async () => { - await advance(600); - write(yamlPath, documentB); - write(yamlPath, ''); - await changeAndSettle(); - }), + (previous) => previous.then(rewriteBeforeALoad), Promise.resolve(), ); expect(values()).toEqual(['a']); - await advance(199); - expect(values()).toEqual(['a']); - await advance(1); + // 5400 ms after a load first found it empty, the file is applied although it was just written. + await rewriteBeforeALoad(); expect(values()).toEqual(['a', '']); }); @@ -268,10 +282,14 @@ describe('given a reloader over a file that is rewritten in place', () => { await changeAndSettle(); expect(values()).toEqual(['a', 'b']); - // Much later, and within five retry delays of the first emptying, the file is emptied again. + // Later, within five retry delays of the first emptying, the file is emptied again. A load + // 200 ms after that, and more than five retry delays after the first emptying, holds it back. await advance(4600); write(yamlPath, ''); await changeAndSettle(); + await advance(200); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); await advance(retryDelayMs - 1); expect(values()).toEqual(['a', 'b']); await advance(1); @@ -294,11 +312,14 @@ describe('given a reloader over a file that is rewritten in place', () => { const reported = () => failures.filter((failure) => !failure.repeated); expect(reported().map((failure) => failure.kind)).toEqual(['read']); - // The data in effect still has the file's content, so the empty file is held back, for a full - // retry delay from when it is found empty again. + // The data in effect still has the file's content, so the empty file is held back, with a new + // delay: a load more than five retry delays after the first emptying holds it back. await advance(4400); write(jsonPath, ''); await changeAndSettle(); + await advance(600); + await changeAndSettle(); + expect(reported().map((failure) => failure.kind)).toEqual(['read']); await advance(retryDelayMs - 1); expect(reported().map((failure) => failure.kind)).toEqual(['read']); await advance(1); @@ -341,6 +362,39 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(failures).toEqual([]); }); + it('applies two files that are emptied together and stay empty one retry delay later', async () => { + write(yamlPath, documentA); + write(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + write(otherYamlPath, ''); + await changeAndSettle(); + expect(readAgainMessages()).toHaveLength(2); + + // Both files started their delays in the same load. + await advance(retryDelayMs - 1); + expect(values()).toEqual(['a,a']); + await advance(1); + expect(values()).toEqual(['a,a', '']); + }); + + it('reports a failure of another file even when a file is held back', async () => { + write(yamlPath, documentA); + write(jsonPath, '{"flagValues": {"flag2": "a"}}'); + makeReloader([yamlPath, jsonPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + write(jsonPath, '{"flagValues": '); + await changeAndSettle(); + + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ kind: 'parse', path: jsonPath, repeated: false }); + expect(values()).toEqual(['a,a']); + }); + it('applies a file that is deleted and then created empty right away', async () => { write(yamlPath, documentA); makeReloader([yamlPath], (config) => { diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 90a657ce86..4058db3641 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -150,7 +150,7 @@ function asError(err: unknown): Error { * * A file that is rewritten in place is truncated and then written, so a reload can find it * empty. A reload that finds a file empty whose content is in effect keeps the current data and - * reads the file again later, whatever the file's format. See `_emptyFileGraceLeft`. + * reads the file again later, whatever the file's format. See `_holdsBack`. * * @internal */ @@ -279,7 +279,7 @@ export default class FileReloader { } } - private _armRetry(delayMs: number = this._config.retryDelayMs): void { + private _armRetry(): void { if (this._closed || this._config.retryDelayMs <= 0) { return; } @@ -287,7 +287,7 @@ export default class FileReloader { this._retryTimer = setTimeout(() => { this._retryTimer = undefined; this._enqueue('retry'); - }, delayMs); + }, this._config.retryDelayMs); } /** @@ -323,12 +323,14 @@ export default class FileReloader { // Whether each path that this reload read, or found missing, has content. const hadContent = new Map(); let failure: LoadFailure | undefined; - let heldBack: { path: string; graceLeftMs: number } | undefined; + // The empty files that this reload holds back. The other files are still read, so that files + // that are emptied together start their delays in the same reload. + const heldBack: string[] = []; const readAll = paths.reduce( (previous, path) => previous.then(async () => { - if (failure || heldBack) { + if (failure) { return; } let data: string; @@ -352,9 +354,8 @@ export default class FileReloader { this._emptyFiles.delete(path); hadContent.set(path, true); } else { - const graceLeftMs = await this._emptyFileGraceLeft(path); - if (graceLeftMs > 0) { - heldBack = { path, graceLeftMs }; + if (await this._holdsBack(path)) { + heldBack.push(path); return; } hadContent.set(path, false); @@ -372,8 +373,10 @@ export default class FileReloader { ); await readAll; - if (heldBack) { - this._readAgainLater(heldBack.path, heldBack.graceLeftMs, reason); + // A file that cannot be read or parsed fails the reload, whether or not another file is held + // back. + if (!failure && heldBack.length > 0) { + this._readAgainLater(heldBack, reason); return; } @@ -411,8 +414,8 @@ export default class FileReloader { const recovering = this._lastFailureKey !== undefined; const content = JSON.stringify(contents); if (this._config.skipUnchanged && !recovering && content === this._lastGoodContent) { + // The content is the same as the content in effect, so each file contributed what it did. this._lastFailureKey = undefined; - this._rememberAppliedContent(hadContent); return; } // Nothing is remembered until the consumer has accepted the result. A result the consumer @@ -431,16 +434,11 @@ export default class FileReloader { this._lastFailureKey = undefined; this._lastGoodContent = content; this._lastGoodFlags = nextFlags; - this._rememberAppliedContent(hadContent); - } - - private _rememberAppliedContent(hadContent: Map): void { hadContent.forEach((value, path) => this._appliedHadContent.set(path, value)); } /** - * Decides what to do with a file that a reload found empty. Returns how long to hold it back, - * or zero to parse it now. + * Decides whether a reload holds back a file that it found empty. * * A file whose content is in effect and that is empty now may be in the middle of an in-place * rewrite: a writer that truncates the file and then writes the new content. Such a file is not @@ -452,28 +450,30 @@ export default class FileReloader { * retry delays after a reload first found it empty. A file whose content is not in effect, such * as a placeholder that has always been empty, is parsed right away, and so is any empty file * when retries are off, since nothing would read it again. + * + * The delay is checked against nine tenths of the retry delay. The retry that the hold arms + * comes one retry delay later, and a timer can fire slightly early, so a file that stays empty + * is applied by that retry rather than held for another retry delay. */ - private async _emptyFileGraceLeft(path: string): Promise { + private async _holdsBack(path: string): Promise { const { retryDelayMs } = this._config; if (retryDelayMs <= 0 || this._appliedHadContent.get(path) !== true) { - return 0; + return false; } const modified = await this._modifiedTime(path); const now = monotonicNow(); - const empty = this._emptyFiles.get(path); + let empty = this._emptyFiles.get(path); if (!empty) { - this._emptyFiles.set(path, { firstSeen: now, since: now, modified }); - return retryDelayMs; - } - if (empty.modified !== modified) { + empty = { firstSeen: now, since: now, modified }; + this._emptyFiles.set(path, empty); + } else if (empty.modified !== modified) { empty.since = now; empty.modified = modified; } - const until = Math.min( - empty.since + retryDelayMs, - empty.firstSeen + MAX_EMPTY_FILE_RETRY_DELAYS * retryDelayMs, + return ( + now - empty.since < retryDelayMs - retryDelayMs / 10 && + now - empty.firstSeen < MAX_EMPTY_FILE_RETRY_DELAYS * retryDelayMs ); - return Math.max(until - now, 0); } private async _modifiedTime(path: string): Promise { @@ -489,26 +489,23 @@ export default class FileReloader { } /** - * Holds back a file that `_emptyFileGraceLeft` decided to hold back. Whatever the file's format, - * the reload keeps the current data and arms the retry for when the file will have stayed empty - * for the retry delay, without reporting a failure. The change signal for the write starts a - * reload sooner. Once the file has stayed empty for the retry delay, a reload parses it: a file - * that is still empty is invalid JSON, or a YAML document with no entries. + * Holds back the files that `_holdsBack` decided to hold back. Whatever their format, the reload + * keeps the current data and arms the retry without reporting a failure, so the files are read + * again after the retry delay. The change signal for a write starts a reload sooner. Once a file + * has stayed empty for the retry delay, a reload parses it: a file that is still empty is invalid + * JSON, or a YAML document with no entries. */ - private _readAgainLater(path: string, graceLeftMs: number, reason: ReloadReason): void { + private _readAgainLater(paths: string[], reason: ReloadReason): void { if (this._closed) { return; } - this._config.logger?.debug( - `File ${path} is empty, possibly while it is being rewritten; reading it again before applying it`, + paths.forEach((path) => + this._config.logger?.debug( + `File ${path} is empty, possibly while it is being rewritten; reading it again before applying it`, + ), ); this._readAgainReason = reason; - // The timer is armed for the end of the grace, not for a full retry delay from now, so that a - // file that stays empty is applied one retry delay after a reload found it empty, however many - // reloads held it back in between. A timer can fire a fraction of a millisecond before the - // monotonic clock reaches its delay; the reload it starts then holds the file back for that - // fraction again. - this._armRetry(Math.ceil(graceLeftMs)); + this._armRetry(); } private _fail(failure: LoadFailure, reason: ReloadReason): void { From 9edc1b2f171f2c1a482ff0fd5839eb95b24b40ea Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:16:31 +0000 Subject: [PATCH 04/12] fix: Cap the run of holding reloads, hold back exactly one retry delay, and hold YAML with no document Reviews of the hold found four problems, and this addresses them: - Files rewritten in turn were held back without end, because the cap of five retry delays was per file and reset whenever that file was read with content. The cap now bounds the run of reloads that hold files back, from the first such reload until a reload that holds none back. - The slack of a tenth of the retry delay let a reload apply a file that had been empty for less than the delay. The hold is now exactly one retry delay, and its retry is armed for when the first held file's delay or the cap ends, plus a tenth of the retry delay. - A partial YAML read with only a comment or a document marker parsed to no document and was applied as no entries. A file whose parser reads it as no document now counts as empty for the hold; `{}` is a document and is applied right away. - A reload that skipped unchanged content did not remember which file contributed it, so after a file moved to another configured path an in-place rewrite of it was applied empty. The skip now remembers it. The hold now compares the file's modification time and size, read through any symbolic link, with the poller's file state helper. --- .../LDClientNode.fileOverrides.test.ts | 101 ++--- .../FileReloader.inPlaceRewrite.test.ts | 366 +++++++++++++----- .../overrides/overrideDocument.test.ts | 10 +- .../api/integrations/FileDataSourceOptions.ts | 13 +- .../src/api/options/LDDataSystemOptions.ts | 16 +- .../src/data_sources/filedata/FilePoller.ts | 25 +- .../src/data_sources/filedata/FileReloader.ts | 172 ++++---- .../src/data_sources/filedata/document.ts | 10 + .../src/data_sources/filedata/index.ts | 7 +- .../src/overrides/FileOverrideSource.ts | 9 +- .../src/overrides/overrideDocument.ts | 8 +- 11 files changed, 484 insertions(+), 253 deletions(-) diff --git a/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts b/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts index fed806e578..da7909a394 100644 --- a/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts +++ b/packages/sdk/server-node/__tests__/LDClientNode.fileOverrides.test.ts @@ -211,53 +211,64 @@ describe('given a temporary directory of override files', () => { const loadFailures = () => errors.filter((message) => message.startsWith('Unable to load flags')); - it('keeps the override while the file is rewritten in place', async () => { - await start(); - let pauseMs = 300; - if (changeDetection === 'polling') { - // The rewrite starts 750 ms after a poll, so the next poll finds the file empty, and its - // reload 100 ms later still does. The file is written 250 ms after that, well before the - // retry reads it again a second after the reload. - await sleep((await nextPoll()) + 750 - Date.now()); - pauseMs = 600; - } - // In watching mode, the reload that the truncation starts runs after the 100 ms settle - // window, while the file is still empty. - const served = await sampleWhile(() => rewriteInPlace(path, document('b'), pauseMs), 'b'); + // A polling test waits for a poll and then for the rewrites, several seconds in all. + const testTimeoutMs = 15000; + + it( + 'keeps the override while the file is rewritten in place', + async () => { + await start(); + let pauseMs = 300; + if (changeDetection === 'polling') { + // The rewrite starts 750 ms after a poll, so the next poll finds the file empty, and its + // reload 100 ms later still does. The file is written 250 ms after that, well before the + // retry reads it again a second after the reload. + await sleep((await nextPoll()) + 750 - Date.now()); + pauseMs = 600; + } + // In watching mode, the reload that the truncation starts runs after the 100 ms settle + // window, while the file is still empty. + const served = await sampleWhile(() => rewriteInPlace(path, document('b'), pauseMs), 'b'); - expect(served).toEqual(['a', 'b']); - expect(loadFailures()).toEqual([]); - }); + expect(served).toEqual(['a', 'b']); + expect(loadFailures()).toEqual([]); + }, + testTimeoutMs, + ); - it('keeps the override through two in-place rewrites in a row', async () => { - await start(); - let rewrites: () => Promise; - if (changeDetection === 'polling') { - // The first rewrite starts 750 ms after a poll, so the next poll finds the file empty. - // The second starts 1900 ms after that poll, before the next poll and the retry, so they - // find the file empty again before any load has read the first rewrite's content. - const poll = await nextPoll(); - await sleep(poll + 750 - Date.now()); - rewrites = async () => { - await rewriteInPlace(path, document('b'), 600); - await sleep(poll + 1900 - Date.now()); - await rewriteInPlace(path, document('c'), 600); - }; - } else { - // The first rewrite's write and the second's truncation fall in one settle window, so the - // load after them finds the file empty again. - rewrites = async () => { - await rewriteInPlace(path, document('b'), 300); - await sleep(20); - await rewriteInPlace(path, document('c'), 300); - }; - } - const served = await sampleWhile(rewrites, 'c'); + it( + 'keeps the override through two in-place rewrites in a row', + async () => { + await start(); + let rewrites: () => Promise; + if (changeDetection === 'polling') { + // The first rewrite starts 750 ms after a poll, so the next poll finds the file empty. + // The second starts 1900 ms after that poll, before the next poll and the retry, so they + // find the file empty again before any load has read the first rewrite's content. + const poll = await nextPoll(); + await sleep(poll + 750 - Date.now()); + rewrites = async () => { + await rewriteInPlace(path, document('b'), 600); + await sleep(poll + 1900 - Date.now()); + await rewriteInPlace(path, document('c'), 600); + }; + } else { + // The first rewrite's write and the second's truncation fall in one settle window, so the + // load after them finds the file empty again. + rewrites = async () => { + await rewriteInPlace(path, document('b'), 300); + await sleep(20); + await rewriteInPlace(path, document('c'), 300); + }; + } + const served = await sampleWhile(rewrites, 'c'); - expect(served).not.toContain('default'); - expect(served[0]).toEqual('a'); - expect(served[served.length - 1]).toEqual('c'); - expect(loadFailures()).toEqual([]); - }); + expect(served).not.toContain('default'); + expect(served[0]).toEqual('a'); + expect(served[served.length - 1]).toEqual('c'); + expect(loadFailures()).toEqual([]); + }, + testTimeoutMs, + ); }); }); diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index 7b7e14cfbb..93e8bc1d8c 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -12,20 +12,31 @@ import testPolicy from './testPolicy'; // These tests rewrite a file the way an application or editor rewrites it in place: the file is // truncated, and the new content is written some time later. A reload in between finds the file -// empty. +// empty. The clock is Jest's fake clock, so every time below is exact. const jsonPath = '/data/data.json'; const yamlPath = '/data/data.yaml'; const otherYamlPath = '/data/other.yaml'; const retryDelayMs = 1000; +// The retry that a hold arms comes a tenth of the retry delay after the delay ends. +const holdRetryMs = retryDelayMs + retryDelayMs / 10; const documentA = '{"flagValues": {"flag1": "a"}}'; const documentB = '{"flagValues": {"flag1": "b"}}'; const documentC = '{"flagValues": {"flag1": "c"}}'; - -// A stand-in for a YAML parser. A flow-style document like the ones above is both JSON and YAML, -// and like the YAML parsers, it returns null for a file with no content. -const yamlParser = (data: string) => (hasContent(data) ? JSON.parse(data) : null); +const otherA = '{"flagValues": {"flag2": "a"}}'; +const otherB = '{"flagValues": {"flag2": "b"}}'; + +// A stand-in for a YAML parser. A flow-style document like the ones above is both JSON and YAML. +// Like the YAML parsers, it returns null for content with no document: white space, comments and +// document markers. +const yamlParser = (data: string) => { + const body = data + .split('\n') + .filter((line) => !/^\s*(#.*)?$/.test(line) && line.trim() !== '---') + .join('\n'); + return hasContent(body) ? JSON.parse(body) : null; +}; const emptiedReadAgain = /is empty, possibly while it is being rewritten; reading it again/; const emptyJson = 'the file has no content, which is not a JSON document'; @@ -49,6 +60,7 @@ describe('given a reloader over a file that is rewritten in place', () => { afterEach(() => { reloader?.close(); + jest.restoreAllMocks(); jest.useRealTimers(); }); @@ -60,7 +72,7 @@ describe('given a reloader over a file that is rewritten in place', () => { }; // The policy parses by extension and validates the document as the override source does, so a - // YAML file with no content is a document with no entries and a JSON file with no content fails. + // YAML file with no document has no entries and a JSON file with no content fails. const makeReloader = (paths: string[], configure?: (config: FileReloaderConfig) => void) => { const config: FileReloaderConfig = { paths, @@ -84,6 +96,7 @@ describe('given a reloader over a file that is rewritten in place', () => { const values = () => applied.map((result) => result.flags.map(({ item }) => item.variations?.[0]).join(',')); + // Two triggers without a settle in between coalesce into one load, so every change settles. const changeAndSettle = async () => { reloader.trigger(); await jest.advanceTimersByTimeAsync(0); @@ -91,12 +104,16 @@ describe('given a reloader over a file that is rewritten in place', () => { const advance = (ms: number) => jest.advanceTimersByTimeAsync(ms); + const startWith = async (path: string, data: string) => { + write(path, data); + makeReloader([path]); + await reloader.reloadNow(); + }; + it.each(['', ' \n\t\r\n', '\uFEFF'])( - 'reads an emptied file again and applies the rewritten content with no empty application, for content %j', + 'holds back an emptied file and applies the rewritten content with no empty application, for content %j', async (empty) => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + await startWith(yamlPath, documentA); write(yamlPath, empty); await changeAndSettle(); @@ -111,59 +128,74 @@ describe('given a reloader over a file that is rewritten in place', () => { }, ); - it('applies a YAML file that stays empty once it has been empty for the retry delay', async () => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + it.each(['# only a comment\n', '---\n', '---\n# a comment\n'])( + 'holds back a YAML file whose content is no document, %j', + async (content) => { + await startWith(yamlPath, documentA); + + write(yamlPath, content); + await changeAndSettle(); + expect(values()).toEqual(['a']); + expect(readAgainMessages()).toHaveLength(1); + + write(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + }, + ); + + it('applies a file that holds an empty document right away', async () => { + await startWith(yamlPath, documentA); + + write(yamlPath, '{}'); + await changeAndSettle(); + expect(values()).toEqual(['a', '']); + expect(readAgainMessages()).toEqual([]); + }); + + it('applies a YAML file that stays empty when the retry comes after the delay', async () => { + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); - expect(values()).toEqual(['a']); - await advance(retryDelayMs - 1); + await advance(holdRetryMs - 1); expect(values()).toEqual(['a']); await advance(1); expect(values()).toEqual(['a', '']); expect(failures).toEqual([]); }); - it('applies a file that stays empty at a load nine tenths of a retry delay after it was found empty', async () => { - // The retry comes a retry delay after the hold, and a timer can fire slightly early. The slack - // makes sure that the retry applies a file that stayed empty. - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + it('holds back a file until exactly the retry delay has passed', async () => { + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); - await advance(retryDelayMs * 0.9 - 1); + await advance(retryDelayMs * 0.95); await changeAndSettle(); expect(values()).toEqual(['a']); - await advance(1); + + await advance(retryDelayMs * 0.05); await changeAndSettle(); expect(values()).toEqual(['a', '']); }); it('does not report an emptied JSON file at the load that finds it empty, and the retry reports it', async () => { - write(jsonPath, documentA); - makeReloader([jsonPath]); - await reloader.reloadNow(); + await startWith(jsonPath, documentA); write(jsonPath, ''); await changeAndSettle(); expect(failures).toEqual([]); - await advance(retryDelayMs); + await advance(holdRetryMs); expect(failures).toHaveLength(1); expect(failures[0]).toMatchObject({ kind: 'parse', path: jsonPath, repeated: false }); expect(failures[0].error.message).toEqual(emptyJson); expect(values()).toEqual(['a']); }); - it('does not apply a file that stays empty at a further change signal within the retry delay', async () => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + it('does not apply a file that stays empty at a further change signal, and the retry applies it when the delay ends', async () => { + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); @@ -171,8 +203,9 @@ describe('given a reloader over a file that is rewritten in place', () => { await changeAndSettle(); expect(values()).toEqual(['a']); - // The retry that the further change signal armed applies the file a retry delay later. - await advance(retryDelayMs - 1); + // The retry that the further load armed comes when the delay that started at the first load + // ends, plus a tenth of the delay. + await advance(holdRetryMs - 500 - 1); expect(values()).toEqual(['a']); await advance(1); expect(values()).toEqual(['a', '']); @@ -181,9 +214,7 @@ describe('given a reloader over a file that is rewritten in place', () => { it('restarts the delay when the file is written and truncated again between two loads', async () => { // Two quick rewrites in watching mode: the write of the first and the truncation of the second // fall in one settle window, so no load sees the first one's content. - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); @@ -191,10 +222,10 @@ describe('given a reloader over a file that is rewritten in place', () => { write(yamlPath, documentB); write(yamlPath, ''); await changeAndSettle(); - expect(values()).toEqual(['a']); - // The first emptying alone would be applied now. The second restarted the delay. - await advance(retryDelayMs - 600); + // A load after the first delay would have ended still holds the file back. + await advance(450); + await changeAndSettle(); expect(values()).toEqual(['a']); write(yamlPath, documentC); @@ -205,14 +236,14 @@ describe('given a reloader over a file that is rewritten in place', () => { it('does not apply a file that is mid-rewrite when another file changes', async () => { write(yamlPath, documentA); - write(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + write(otherYamlPath, otherA); makeReloader([yamlPath, otherYamlPath]); await reloader.reloadNow(); write(yamlPath, ''); await changeAndSettle(); await advance(300); - write(otherYamlPath, '{"flagValues": {"flag2": "b"}}'); + write(otherYamlPath, otherB); await changeAndSettle(); expect(values()).toEqual(['a,a']); @@ -226,9 +257,7 @@ describe('given a reloader over a file that is rewritten in place', () => { it('does not apply the truncation of a later rewrite that the retry finds', async () => { // Polling with two rewrites about a second apart: no load reads the first one's content // before the second truncation. - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); @@ -236,7 +265,7 @@ describe('given a reloader over a file that is rewritten in place', () => { write(yamlPath, documentB); await advance(500); write(yamlPath, ''); - await advance(100); + await advance(holdRetryMs - 900); expect(values()).toEqual(['a']); expect(readAgainMessages()).toHaveLength(2); @@ -245,36 +274,69 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a', 'c']); }); - it('applies a file that keeps being rewritten so that every load finds it empty, once five retry delays have passed since a load first found it empty', async () => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + it('applies a file that keeps being rewritten so that every load finds it empty, once loads have held it back for five retry delays', async () => { + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); // Every 600 ms the file is written and truncated again before a load reads it. - const rewriteBeforeALoad = async () => { - await advance(600); - write(yamlPath, documentB); - write(yamlPath, ''); - await changeAndSettle(); - }; await Array.from({ length: 8 }).reduce>( - (previous) => previous.then(rewriteBeforeALoad), + (previous) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + }), Promise.resolve(), ); expect(values()).toEqual(['a']); - // 5400 ms after a load first found it empty, the file is applied although it was just written. - await rewriteBeforeALoad(); + // The last load, at 4800 ms, armed the retry for when the five retry delays end, plus a + // tenth of the delay. + await advance(299); + expect(values()).toEqual(['a']); + await advance(1); expect(values()).toEqual(['a', '']); }); - it('starts a new delay for a file that is emptied again after its content was read', async () => { + it('applies files that are rewritten in turn, once loads have held them back for five retry delays', async () => { write(yamlPath, documentA); - makeReloader([yamlPath]); + write(otherYamlPath, otherA); + makeReloader([yamlPath, otherYamlPath]); await reloader.reloadNow(); + write(yamlPath, ''); + await changeAndSettle(); + // Every 600 ms the file that is empty is written and the other one is truncated, so every load + // finds one of them empty, and each file's own delay starts over. + await [1, 2, 3, 4, 5, 6, 7, 8].reduce>( + (previous, step) => + previous.then(async () => { + await advance(600); + if (step % 2 === 1) { + write(yamlPath, documentB); + write(otherYamlPath, ''); + } else { + write(otherYamlPath, otherB); + write(yamlPath, ''); + } + await changeAndSettle(); + }), + Promise.resolve(), + ); + // Changes to the file that has content wait while the other one is held back. + expect(values()).toEqual(['a,a']); + + await advance(299); + expect(values()).toEqual(['a,a']); + await advance(1); + expect(values()).toEqual(['a,a', 'b']); + }); + + it('starts a fresh hold for a file that is emptied again after its content was read', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); await changeAndSettle(); await advance(300); @@ -282,21 +344,48 @@ describe('given a reloader over a file that is rewritten in place', () => { await changeAndSettle(); expect(values()).toEqual(['a', 'b']); - // Later, within five retry delays of the first emptying, the file is emptied again. A load - // 200 ms after that, and more than five retry delays after the first emptying, holds it back. + // The load that applied the content ended the run of holding loads, so a load more than five + // retry delays after the first hold still holds the file back. await advance(4600); write(yamlPath, ''); await changeAndSettle(); await advance(200); await changeAndSettle(); expect(values()).toEqual(['a', 'b']); - await advance(retryDelayMs - 1); + + await advance(holdRetryMs - 200 - 1); expect(values()).toEqual(['a', 'b']); await advance(1); expect(values()).toEqual(['a', 'b', '']); }); - it('starts a new delay for a file that is deleted and created empty again', async () => { + it('starts a fresh hold for a JSON file that has been empty for long and is rewritten in place', async () => { + await startWith(jsonPath, documentA); + + write(jsonPath, ''); + await changeAndSettle(); + await advance(holdRetryMs); + expect(failures).toHaveLength(1); + expect(failures[0].error.message).toEqual(emptyJson); + + // The file stays empty, and the retries keep failing. A failing load ends the run of holding + // loads. + await advance(10000); + const failed = failures.length; + + // The file is rewritten in place. Its truncation is a new state, held back without a report. + write(jsonPath, ''); + await changeAndSettle(); + expect(failures).toHaveLength(failed); + expect(readAgainMessages()).toHaveLength(2); + + write(jsonPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + expect(failures).toHaveLength(failed); + }); + + it('starts a fresh hold for a file that is deleted and created empty again', async () => { write(jsonPath, documentA); makeReloader([jsonPath], (config) => { config.policy = testPolicy({ missingFile: 'fail' }); @@ -312,21 +401,45 @@ describe('given a reloader over a file that is rewritten in place', () => { const reported = () => failures.filter((failure) => !failure.repeated); expect(reported().map((failure) => failure.kind)).toEqual(['read']); - // The data in effect still has the file's content, so the empty file is held back, with a new - // delay: a load more than five retry delays after the first emptying holds it back. + // The data in effect still has the file's content, so the empty file is held back. The failed + // loads ended the run of holding loads, so a load more than five retry delays after the first + // hold still holds it back. await advance(4400); write(jsonPath, ''); await changeAndSettle(); await advance(600); await changeAndSettle(); expect(reported().map((failure) => failure.kind)).toEqual(['read']); - await advance(retryDelayMs - 1); + + await advance(holdRetryMs - 600 - 1); expect(reported().map((failure) => failure.kind)).toEqual(['read']); await advance(1); expect(reported().map((failure) => failure.kind)).toEqual(['read', 'parse']); expect(reported()[1].error.message).toEqual(emptyJson); }); + it('holds back a file whose content moved to it from another configured file', async () => { + // The content in effect comes from the second file. Moving it to the first leaves the + // contents the same, so the load applies nothing, but the first file now contributes them. + write(otherYamlPath, documentA); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + expect(values()).toEqual(['a']); + + filesystem.remove(otherYamlPath); + write(yamlPath, documentA); + await changeAndSettle(); + expect(values()).toEqual(['a']); + + write(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a']); + + write(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + }); + it('never holds back an empty placeholder file, and it does not delay a change to another file', async () => { write(otherYamlPath, ''); write(yamlPath, documentA); @@ -342,10 +455,9 @@ describe('given a reloader over a file that is rewritten in place', () => { it('holds back each of two files that are rewritten together', async () => { write(yamlPath, documentA); - write(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + write(otherYamlPath, otherA); makeReloader([yamlPath, otherYamlPath]); await reloader.reloadNow(); - expect(values()).toEqual(['a,a']); write(yamlPath, ''); write(otherYamlPath, ''); @@ -356,15 +468,15 @@ describe('given a reloader over a file that is rewritten in place', () => { await changeAndSettle(); expect(values()).toEqual(['a,a']); - write(otherYamlPath, '{"flagValues": {"flag2": "b"}}'); + write(otherYamlPath, otherB); await changeAndSettle(); expect(values()).toEqual(['a,a', 'b,b']); expect(failures).toEqual([]); }); - it('applies two files that are emptied together and stay empty one retry delay later', async () => { + it('applies two files that are emptied together and stay empty at the same retry', async () => { write(yamlPath, documentA); - write(otherYamlPath, '{"flagValues": {"flag2": "a"}}'); + write(otherYamlPath, otherA); makeReloader([yamlPath, otherYamlPath]); await reloader.reloadNow(); @@ -374,7 +486,7 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(readAgainMessages()).toHaveLength(2); // Both files started their delays in the same load. - await advance(retryDelayMs - 1); + await advance(holdRetryMs - 1); expect(values()).toEqual(['a,a']); await advance(1); expect(values()).toEqual(['a,a', '']); @@ -382,7 +494,7 @@ describe('given a reloader over a file that is rewritten in place', () => { it('reports a failure of another file even when a file is held back', async () => { write(yamlPath, documentA); - write(jsonPath, '{"flagValues": {"flag2": "a"}}'); + write(jsonPath, otherA); makeReloader([yamlPath, jsonPath]); await reloader.reloadNow(); @@ -395,7 +507,7 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a,a']); }); - it('applies a file that is deleted and then created empty right away', async () => { + it('applies a file that is deleted and then created empty right away, where a missing file contributes nothing', async () => { write(yamlPath, documentA); makeReloader([yamlPath], (config) => { config.skipUnchanged = false; @@ -413,9 +525,7 @@ describe('given a reloader over a file that is rewritten in place', () => { }); it('holds back an emptied file when a retry left over from a failure finds it', async () => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + await startWith(yamlPath, documentA); // A malformed edit fails the reload and arms the retry. write(yamlPath, '{"flagValues": '); @@ -427,6 +537,7 @@ describe('given a reloader over a file that is rewritten in place', () => { await advance(retryDelayMs); expect(values()).toEqual(['a']); expect(failures).toHaveLength(1); + expect(readAgainMessages()).toHaveLength(1); write(yamlPath, documentB); await changeAndSettle(); @@ -454,7 +565,7 @@ describe('given a reloader over a file that is rewritten in place', () => { write(yamlPath, ''); await changeAndSettle(); - await advance(retryDelayMs); + await advance(holdRetryMs); expect(values()).toEqual(['a', '']); // The file is written empty again: nothing it supplied is in effect, so nothing is held back. @@ -464,9 +575,7 @@ describe('given a reloader over a file that is rewritten in place', () => { }); it('does not count holding back as a failure, so an unchanged rewrite applies nothing', async () => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + await startWith(yamlPath, documentA); write(yamlPath, ''); await changeAndSettle(); @@ -478,35 +587,80 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(failures).toEqual([]); }); - it('reports a failure after reading again as news when the load that found the file empty followed a change', async () => { - write(jsonPath, documentA); - makeReloader([jsonPath]); - await reloader.reloadNow(); + it('holds back a file when its state cannot be read', async () => { + await startWith(yamlPath, documentA); + jest.spyOn(filesystem, 'getFileStats').mockRejectedValue(new Error('EIO: i/o error')); - // A malformed edit fails, and the retries keep failing the same way. - write(jsonPath, '{"flagValues": '); + write(yamlPath, ''); await changeAndSettle(); - await advance(retryDelayMs); - expect(failures.map((failure) => failure.repeated)).toEqual([false, true]); + expect(values()).toEqual(['a']); + expect(readAgainMessages()).toHaveLength(1); + expect(logger.getMessages(LogLevel.Error)).toEqual([]); - // The file is emptied, which is a change. The retry that reads it again finds it still empty. - // That failure is news, as it would have been for the load that the change started. - write(jsonPath, ''); - await changeAndSettle(); - expect(failures).toHaveLength(2); - await advance(retryDelayMs); - expect(failures).toHaveLength(3); - expect(failures[2].repeated).toBe(false); - expect(failures[2].error.message).toEqual(emptyJson); - logger.expectMessages([ - { level: LogLevel.Debug, matches: /Reading flag data again after a file was found empty/ }, - ]); + await advance(holdRetryMs); + expect(values()).toEqual(['a', '']); + }); + + describe('when a failure is reported after a hold', () => { + // The file is malformed, and a retry has repeated the failure, so the last report is a parse + // failure of the file. + const startFailing = async () => { + await startWith(jsonPath, documentA); + write(jsonPath, '{"flagValues": '); + await changeAndSettle(); + await advance(retryDelayMs); + expect(failures.map((failure) => failure.repeated)).toEqual([false, true]); + }; + + it('reports it as news when the load that held the file back followed a change', async () => { + await startFailing(); + + // The file is emptied, which is a change. The retry that reads it again finds it still + // empty. That failure is news, as it would have been for the load that the change started. + write(jsonPath, ''); + await changeAndSettle(); + expect(failures).toHaveLength(2); + await advance(holdRetryMs); + expect(failures).toHaveLength(3); + expect(failures[2].repeated).toBe(false); + expect(failures[2].error.message).toEqual(emptyJson); + logger.expectMessages([ + { level: LogLevel.Debug, matches: /Reading flag data again after a file was found empty/ }, + ]); + }); + + it('reports it as a repeat when a retry held the file back and the next retry fails', async () => { + await startFailing(); + + // A retry finds the file emptied, without a change signal, and holds it back. The retry + // after it continues that retry, so its failure on the same file is a repeat. + write(jsonPath, ''); + await advance(retryDelayMs); + expect(failures).toHaveLength(2); + expect(readAgainMessages()).toHaveLength(1); + await advance(holdRetryMs); + expect(failures).toHaveLength(3); + expect(failures[2].repeated).toBe(true); + }); + + it('reports it as news when a change follows a hold by a retry', async () => { + await startFailing(); + + write(jsonPath, ''); + await advance(retryDelayMs); + expect(failures).toHaveLength(2); + expect(readAgainMessages()).toHaveLength(1); + + // A change signal does not continue the retry that held the file back. + write(jsonPath, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(3); + expect(failures[2].repeated).toBe(false); + }); }); it('does not read again after close', async () => { - write(yamlPath, documentA); - makeReloader([yamlPath]); - await reloader.reloadNow(); + await startWith(yamlPath, documentA); write(yamlPath, ''); filesystem.deferReads = true; diff --git a/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts b/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts index f68be673cd..714d7a422e 100644 --- a/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts +++ b/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts @@ -1,3 +1,4 @@ +import { NO_ENTRIES } from '../../src/data_sources/filedata'; import { parseOverrideDocument } from '../../src/overrides/overrideDocument'; const flag1 = { @@ -75,9 +76,12 @@ describe('given the override document parser', () => { ); }); - it('treats a parser result of null or undefined as an empty document', () => { - expect(parseOverrideDocument(() => null)('empty.yaml', '')).toEqual({}); - expect(parseOverrideDocument(() => undefined)('empty.yaml', '')).toEqual({}); + it('treats a parser result of null or undefined as a document with no entries', () => { + expect(parseOverrideDocument(() => null)('empty.yaml', '')).toBe(NO_ENTRIES); + expect(parseOverrideDocument(() => undefined)('empty.yaml', '# a comment')).toBe(NO_ENTRIES); + // An empty document is a document, which the reloader applies right away. + expect(parseOverrideDocument(() => ({}))('empty.yaml', '{}')).not.toBe(NO_ENTRIES); + expect(parseOverrideDocument()('empty.json', '{}')).not.toBe(NO_ENTRIES); }); it.each(['', ' \n\t\n'])('rejects a JSON file with no content %j', (data) => { diff --git a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts index d00410f461..ddab047e76 100644 --- a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts +++ b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts @@ -17,11 +17,14 @@ export interface FileDataSourceOptions { * A file that is replaced by a rename, or deleted and created again, is detected. A load * that fails, for example because a file was read while it was written, is retried after a * short delay and the previously loaded data stays in effect until it succeeds. - * A file can be rewritten in place: a file whose data is in effect and that is found empty is - * not applied until it has stayed empty, without being written, for the same delay. YAML that is - * cut off between two entries can still parse, so for a YAML file, replacing its entire contents - * in one operation, such as renaming a fully written temporary file over it, avoids briefly - * applying only a part of it. + * A file can be rewritten in place: a file whose content is in effect and that is found empty is + * held back until it has stayed empty, without being written, for about a second, and for at most + * about five seconds while files keep being rewritten. Changes to the other files wait with it, + * because a load applies all of the files or none of them. A file that is still empty then fails + * to load, which is reported, and the previously loaded data stays in effect. YAML that is cut + * off between two entries can still parse, so for a YAML file, replacing its entire contents in + * one operation, such as renaming a fully written temporary file over it, avoids briefly applying + * only a part of it. * A change to another entry in a directory causes a reload only when the metadata of a * configured file changed, read through any symbolic link, which is how a mounted ConfigMap * or Secret is updated. diff --git a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts index a9ceb2f943..92ef42b942 100644 --- a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts +++ b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts @@ -120,13 +120,15 @@ export interface LDDataSystemOptions { * each file supplied. * * A file can be rewritten in place. To remove a file's overrides, delete it or write a document - * with no entries, such as `{}`. A JSON file with no content is not a valid document: it fails to - * load, and the overrides in effect stay. A file whose overrides are in effect and that is found - * empty may be in the middle of a rewrite, so the source applies it only once it has stayed empty, - * without being written, for about a second. An emptied YAML file therefore removes its overrides - * about a second later. YAML that is cut off between two entries can still parse, so for a YAML - * file, replacing its entire contents in one operation, such as renaming a fully written temporary - * file over it, avoids briefly applying only a part of it. + * with no entries, such as `{}`. A file whose content is in effect and that is found empty, or + * with only a YAML comment or document marker, may be in the middle of a rewrite. The source holds + * it back until it has stayed so, without being written, for about a second, and for at most about + * five seconds while files keep being rewritten. Changes to the other files wait with it, because + * a reload applies all of the files or none of them. After that, an empty YAML file removes its + * overrides. An empty JSON file is not a valid document: it fails to load, and the overrides in + * effect stay. YAML that is cut off between two entries can still parse, so for a YAML file, + * replacing its entire contents in one operation, such as renaming a fully written temporary file + * over it, avoids briefly applying only a part of it. * * @example * ```typescript diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts b/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts index 9a3da6718a..f5790e8b03 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts @@ -1,9 +1,12 @@ import { Filesystem } from '@launchdarkly/js-sdk-common'; /** - * The observed state of one file, or its absence. + * The observed state of one file, or its absence: its modification time and size, read through + * any symbolic link. + * + * @internal */ -interface FileState { +export interface FileState { exists: boolean; timestamp: number; size: number; @@ -11,7 +14,12 @@ interface FileState { const absent: FileState = { exists: false, timestamp: 0, size: 0 }; -async function observe(filesystem: Filesystem, path: string): Promise { +/** + * Observes the state of one file. A file that the filesystem cannot examine counts as absent. + * + * @internal + */ +export async function observeFile(filesystem: Filesystem, path: string): Promise { try { if (filesystem.getFileStats) { const stats = await filesystem.getFileStats(path); @@ -31,10 +39,15 @@ async function observe(filesystem: Filesystem, path: string): Promise } function observeAll(filesystem: Filesystem, paths: string[]): Promise { - return Promise.all(paths.map((path) => observe(filesystem, path))); + return Promise.all(paths.map((path) => observeFile(filesystem, path))); } -function sameState(a: FileState, b: FileState): boolean { +/** + * Reports whether two observations of a file are the same. + * + * @internal + */ +export function sameFileState(a: FileState, b: FileState): boolean { return a.exists === b.exists && a.timestamp === b.timestamp && a.size === b.size; } @@ -103,7 +116,7 @@ export default class FilePoller { if (this._closed) { return; } - const changed = current.some((state, index) => !sameState(state, this._last[index])); + const changed = current.some((state, index) => !sameFileState(state, this._last[index])); this._last = current; if (changed) { this._onChange(); diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 4058db3641..47f1f6d17a 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -1,8 +1,9 @@ import { Filesystem, LDLogger, monotonicNow } from '@launchdarkly/js-sdk-common'; import { Flag } from '../../evaluation/data/Flag'; -import { FileDataDocument, hasContent, isFileNotFoundError } from './document'; +import { FileDataDocument, hasContent, isFileNotFoundError, NO_ENTRIES } from './document'; import { FileDataPolicy } from './FileDataPolicy'; +import { FileState, observeFile, sameFileState } from './FilePoller'; import { FileSummary, mergeDocuments, MergeResult } from './merge'; /** @@ -32,12 +33,13 @@ export const DEFAULT_RETRY_DELAY_MS = 1000; export const MAX_DEBOUNCE_DELAY_MS = 1000; /** - * A file that keeps being written so that every reload finds it empty is applied at the latest - * this many retry delays after a reload first found it empty. + * Reloads stop holding files back once they have held some back for this many retry delays in a + * row, so that a file that keeps being written while every reload finds it empty, or files that + * are rewritten in turn, are not held back without end. * * @internal */ -export const MAX_EMPTY_FILE_RETRY_DELAYS = 5; +export const MAX_HOLD_RETRY_DELAYS = 5; /** * The result of one successful reload: the merged items plus a summary of each configured @@ -114,8 +116,9 @@ export interface FileReloaderConfig { /** * How long to wait after a failed reload before retrying it without a trigger. A file whose * content is in effect and that is found empty is not applied until it has stayed empty, without - * being written, for the same delay. Zero or negative disables the automatic retry, and an empty - * file is then parsed as it is. + * being written, for the same delay, or until reloads have held files back for + * `MAX_HOLD_RETRY_DELAYS` times the delay. Zero or negative disables the automatic retry, and an + * empty file is then parsed as it is. */ retryDelayMs: number; /** @@ -128,15 +131,20 @@ export interface FileReloaderConfig { type ReloadReason = 'initial' | 'change' | 'retry'; /** - * A file whose content is in effect and that reloads have found empty. `firstSeen` is when a - * reload first found it empty and `since` is since when it has been empty without being written, - * both on the monotonic clock. `modified` is the file's modification time when a reload last found - * it empty, or undefined when it could not be read. + * A file whose content is in effect and that reloads have found empty: since when it has been + * empty without being written, on the monotonic clock, and its state then. */ interface EmptyFile { - firstSeen: number; since: number; - modified?: number; + state: FileState; +} + +/** + * Reports whether a parsed document has no entries because the content was no document at all, + * as a YAML parser reports for a file with only a comment or a document marker. + */ +function isNoDocument(document: FileDataDocument | null | undefined): boolean { + return document === null || document === undefined || document === NO_ENTRIES; } function asError(err: unknown): Error { @@ -186,6 +194,10 @@ export default class FileReloader { // The paths whose content is in effect and that reloads have found empty since. private readonly _emptyFiles = new Map(); + // When the current run of reloads that hold files back began, on the monotonic clock. Undefined + // when the last reload did not hold any back. + private _holdingSince?: number; + // Set by a reload that held back an empty file and armed the retry to read it again, to that // reload's reason, until the next reload starts. The next reload continues the deferred one, so // it reports a failure the way the deferred reload would have. @@ -279,7 +291,7 @@ export default class FileReloader { } } - private _armRetry(): void { + private _armRetry(delayMs: number = this._config.retryDelayMs): void { if (this._closed || this._config.retryDelayMs <= 0) { return; } @@ -287,7 +299,7 @@ export default class FileReloader { this._retryTimer = setTimeout(() => { this._retryTimer = undefined; this._enqueue('retry'); - }, this._config.retryDelayMs); + }, delayMs); } /** @@ -316,6 +328,7 @@ export default class FileReloader { // A pending retry is superseded by this reload. It either succeeds, or it fails and arms a // fresh retry. this._clearRetry(); + const now = monotonicNow(); const documents: FileDataDocument[] = []; const files: FileSummary[] = []; @@ -350,23 +363,38 @@ export default class FileReloader { failure = { kind: 'read', error: asError(err), path, repeated: false }; return; } - if (hasContent(data)) { - this._emptyFiles.delete(path); - hadContent.set(path, true); - } else { - if (await this._holdsBack(path)) { - heldBack.push(path); + // A file with no content, or with content that its parser reads as no document, such as + // YAML with only a comment, has no entries. A read can see that much of a file that is + // being written. Content with no document is invalid JSON, so it is only parsed when the + // file is not held back. + const blank = !hasContent(data); + let document: FileDataDocument | undefined; + if (!blank) { + try { + document = policy.parseDocument(path, data); + } catch (err) { + failure = { kind: 'parse', error: asError(err), path, repeated: false }; return; } - hadContent.set(path, false); } - contents.push(data); - try { - documents.push(policy.parseDocument(path, data)); - } catch (err) { - failure = { kind: 'parse', error: asError(err), path, repeated: false }; + const noEntries = blank || isNoDocument(document); + if (!noEntries) { + this._emptyFiles.delete(path); + } else if (await this._holdsBack(path, now)) { + heldBack.push(path); return; } + if (blank) { + try { + document = policy.parseDocument(path, data); + } catch (err) { + failure = { kind: 'parse', error: asError(err), path, repeated: false }; + return; + } + } + hadContent.set(path, !noEntries); + contents.push(data); + documents.push(document as FileDataDocument); files.push({ path, present: true, flags: 0, segments: 0 }); }), Promise.resolve(), @@ -376,9 +404,11 @@ export default class FileReloader { // A file that cannot be read or parsed fails the reload, whether or not another file is held // back. if (!failure && heldBack.length > 0) { - this._readAgainLater(heldBack, reason); + this._holdBack(heldBack, now, reason); return; } + // A run of reloads that hold files back ends with any reload that does not. + this._holdingSince = undefined; let merged: MergeResult | undefined; if (!failure) { @@ -414,8 +444,10 @@ export default class FileReloader { const recovering = this._lastFailureKey !== undefined; const content = JSON.stringify(contents); if (this._config.skipUnchanged && !recovering && content === this._lastGoodContent) { - // The content is the same as the content in effect, so each file contributed what it did. + // The contents are compared without their paths, so a file can have moved to another + // configured path, which now contributes the content in effect. this._lastFailureKey = undefined; + this._rememberAppliedContent(hadContent); return; } // Nothing is remembered until the consumer has accepted the result. A result the consumer @@ -434,78 +466,72 @@ export default class FileReloader { this._lastFailureKey = undefined; this._lastGoodContent = content; this._lastGoodFlags = nextFlags; + this._rememberAppliedContent(hadContent); + } + + private _rememberAppliedContent(hadContent: Map): void { hadContent.forEach((value, path) => this._appliedHadContent.set(path, value)); } /** - * Decides whether a reload holds back a file that it found empty. - * - * A file whose content is in effect and that is empty now may be in the middle of an in-place - * rewrite: a writer that truncates the file and then writes the new content. Such a file is not - * applied until it has stayed empty, without being written, for one retry delay. A change of its - * modification time means that it was written since a reload last found it empty, for example - * rewritten and truncated again between two reloads, so the delay starts over. Modification - * times are only compared with each other, never with the clock. A file that keeps being written - * so that every reload finds it empty is applied at the latest `MAX_EMPTY_FILE_RETRY_DELAYS` - * retry delays after a reload first found it empty. A file whose content is not in effect, such - * as a placeholder that has always been empty, is parsed right away, and so is any empty file - * when retries are off, since nothing would read it again. + * Decides whether a reload holds back a file that it found with no entries. * - * The delay is checked against nine tenths of the retry delay. The retry that the hold arms - * comes one retry delay later, and a timer can fire slightly early, so a file that stays empty - * is applied by that retry rather than held for another retry delay. + * A file whose content is in effect and that has none now may be in the middle of an in-place + * rewrite: a writer that truncates the file and then writes the new content. Such a file is held + * back until it has stayed so, without being written, for the retry delay. A change of its + * state, its modification time and size read through any symbolic link, means that it was + * written since a reload last found it so, for example rewritten and truncated again between two + * reloads, so the delay starts over. A file that keeps being written while every reload finds it + * empty, or files that are rewritten in turn so that every reload finds one of them empty, would + * be held back without end, so reloads stop holding files back once they have held some back for + * `MAX_HOLD_RETRY_DELAYS` retry delays in a row. A file whose content is not in effect, such as a + * placeholder that has always been empty, is parsed right away, and so is any file when retries + * are off, since nothing would read it again. */ - private async _holdsBack(path: string): Promise { + private async _holdsBack(path: string, now: number): Promise { const { retryDelayMs } = this._config; if (retryDelayMs <= 0 || this._appliedHadContent.get(path) !== true) { return false; } - const modified = await this._modifiedTime(path); - const now = monotonicNow(); + const state = await observeFile(this._config.filesystem, path); let empty = this._emptyFiles.get(path); if (!empty) { - empty = { firstSeen: now, since: now, modified }; + empty = { since: now, state }; this._emptyFiles.set(path, empty); - } else if (empty.modified !== modified) { + } else if (!sameFileState(empty.state, state)) { empty.since = now; - empty.modified = modified; + empty.state = state; } return ( - now - empty.since < retryDelayMs - retryDelayMs / 10 && - now - empty.firstSeen < MAX_EMPTY_FILE_RETRY_DELAYS * retryDelayMs + now - empty.since < retryDelayMs && + now - (this._holdingSince ?? now) < MAX_HOLD_RETRY_DELAYS * retryDelayMs ); } - private async _modifiedTime(path: string): Promise { - const { filesystem } = this._config; - try { - if (filesystem.getFileStats) { - return (await filesystem.getFileStats(path))?.timestamp; - } - return await filesystem.getFileTimestamp(path); - } catch { - return undefined; - } - } - /** * Holds back the files that `_holdsBack` decided to hold back. Whatever their format, the reload - * keeps the current data and arms the retry without reporting a failure, so the files are read - * again after the retry delay. The change signal for a write starts a reload sooner. Once a file - * has stayed empty for the retry delay, a reload parses it: a file that is still empty is invalid - * JSON, or a YAML document with no entries. + * keeps the current data and arms the retry without reporting a failure. The retry comes when the + * first of the files has stayed empty for the retry delay, or when the reloads have held files + * back for as long as they may, plus a tenth of the retry delay, so that a timer that fires a + * little early still finds the delay over. The change signal for a write starts a reload sooner. + * Once a file has stayed empty for the retry delay, a reload parses it: a file that is still + * empty is invalid JSON, or a YAML document with no entries. */ - private _readAgainLater(paths: string[], reason: ReloadReason): void { + private _holdBack(paths: string[], now: number, reason: ReloadReason): void { if (this._closed) { return; } - paths.forEach((path) => - this._config.logger?.debug( + const { logger, retryDelayMs } = this._config; + this._holdingSince = this._holdingSince ?? now; + let until = this._holdingSince + MAX_HOLD_RETRY_DELAYS * retryDelayMs; + paths.forEach((path) => { + logger?.debug( `File ${path} is empty, possibly while it is being rewritten; reading it again before applying it`, - ), - ); + ); + until = Math.min(until, this._emptyFiles.get(path)!.since + retryDelayMs); + }); this._readAgainReason = reason; - this._armRetry(); + this._armRetry(Math.max(until - now, 0) + retryDelayMs / 10); } private _fail(failure: LoadFailure, reason: ReloadReason): void { diff --git a/packages/shared/sdk-server/src/data_sources/filedata/document.ts b/packages/shared/sdk-server/src/data_sources/filedata/document.ts index 2be96de1da..27e905f0e1 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/document.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/document.ts @@ -20,6 +20,16 @@ export interface FileDataDocument { segments?: Record; } +/** + * The document of a file that has no entries because its content is no document at all: a YAML + * file with only white space, a comment, or a document marker, for which a YAML parser returns + * null. A parser returns it, rather than an empty object, so that the reloader can tell such a + * file from one that holds an empty document such as `{}`. + * + * @internal + */ +export const NO_ENTRIES: FileDataDocument = Object.freeze({}); + /** * Parses the text of one file into a document. * diff --git a/packages/shared/sdk-server/src/data_sources/filedata/index.ts b/packages/shared/sdk-server/src/data_sources/filedata/index.ts index c3b9a3ad99..114b0ffb48 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/index.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/index.ts @@ -4,6 +4,7 @@ import { hasContent, isFileNotFoundError, isYamlPath, + NO_ENTRIES, parseDocumentByExtension, YamlParser, } from './document'; @@ -14,7 +15,7 @@ import { MissingFileHandling, } from './FileDataPolicy'; import FileDirectoryWatcher, { directoryOf } from './FileDirectoryWatcher'; -import FilePoller from './FilePoller'; +import FilePoller, { FileState, observeFile, sameFileState } from './FilePoller'; import FileReloader, { DEFAULT_DEBOUNCE_DELAY_MS, DEFAULT_RETRY_DELAY_MS, @@ -38,6 +39,7 @@ export { FilePoller, FileReloader, FileReloaderConfig, + FileState, FileSummary, hasContent, isFileNotFoundError, @@ -49,7 +51,10 @@ export { mergeDocuments, MergeResult, MissingFileHandling, + NO_ENTRIES, + observeFile, parseDocumentByExtension, ReloadResult, + sameFileState, YamlParser, }; diff --git a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts index 3d9124bf67..162a5e2325 100644 --- a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts +++ b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts @@ -144,10 +144,11 @@ function countsText(flags: number, segments: number): string { * no overrides, so deleting a file removes its overrides. A file that exists but cannot be read * or parsed makes that whole reload fail. The previously loaded overrides stay in effect, the * failure is logged, and the source retries after a short delay. A JSON file with no content - * cannot be parsed. A file whose overrides are in effect and that is found empty may be in the - * middle of an in-place rewrite, so it is applied only once it has stayed empty, without being - * written, for the retry delay. Every applied change is logged at Info level with the overrides - * in effect and what each file supplied. + * cannot be parsed. A file whose content is in effect and that is found with no entries, empty or + * with only a YAML comment, may be in the middle of an in-place rewrite, so it is held back until + * it has stayed so, without being written, for the retry delay, and for at most five retry delays + * while files keep being rewritten; the other files wait with it. Every applied change is logged + * at Info level with the overrides in effect and what each file supplied. * * Flag overrides are currently experimental and subject to change. * diff --git a/packages/shared/sdk-server/src/overrides/overrideDocument.ts b/packages/shared/sdk-server/src/overrides/overrideDocument.ts index fc10c199c7..e8ec2e0d03 100644 --- a/packages/shared/sdk-server/src/overrides/overrideDocument.ts +++ b/packages/shared/sdk-server/src/overrides/overrideDocument.ts @@ -3,6 +3,7 @@ import { isNullish } from '@launchdarkly/js-sdk-common'; import { DocumentParser, FileDataDocument, + NO_ENTRIES, parseDocumentByExtension, YamlParser, } from '../data_sources/filedata'; @@ -33,9 +34,10 @@ function validateItems(document: Record, member: 'flags' | 'segment function validateDocument(parsed: any): FileDataDocument { if (isNullish(parsed)) { - // A YAML parser returns null or undefined for a file with no content, which is a document with - // no entries. A JSON file with no content fails to parse instead. - return {}; + // A YAML parser returns null or undefined for a file with no content, or with only a comment + // or a document marker, which is a document with no entries. A JSON file with no content fails + // to parse instead. + return NO_ENTRIES; } if (!isPlainObject(parsed)) { throw new Error('file data must be an object'); From f4ceb232d96ea05199c1b75f67ac6fd5290f8f11 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:24:13 +0000 Subject: [PATCH 05/12] test: Cover a filesystem with coarse modification times in the hold --- .../FileReloader.inPlaceRewrite.test.ts | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index 93e8bc1d8c..d789dc0df5 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -418,6 +418,53 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(reported()[1].error.message).toEqual(emptyJson); }); + describe('on a filesystem whose modification times are coarse', () => { + // Every write keeps the same modification time, as two writes within one tick of a coarse + // clock do, so a file that is empty again looks unchanged since it was last found empty. + const writeCoarse = (path: string, data: string) => filesystem.set(path, data, 1); + + it('starts a fresh delay for a file that is emptied again after its content was read', async () => { + writeCoarse(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + writeCoarse(yamlPath, ''); + await changeAndSettle(); + await advance(300); + writeCoarse(yamlPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + + await advance(2000); + writeCoarse(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + expect(readAgainMessages()).toHaveLength(2); + }); + + it('starts a fresh delay for a file that is deleted and created empty again', async () => { + writeCoarse(jsonPath, documentA); + makeReloader([jsonPath], (config) => { + config.policy = testPolicy({ missingFile: 'fail' }); + }); + await reloader.reloadNow(); + + writeCoarse(jsonPath, ''); + await changeAndSettle(); + await advance(100); + filesystem.remove(jsonPath); + await changeAndSettle(); + const reported = () => failures.filter((failure) => !failure.repeated); + expect(reported().map((failure) => failure.kind)).toEqual(['read']); + + await advance(2000); + writeCoarse(jsonPath, ''); + await changeAndSettle(); + expect(reported().map((failure) => failure.kind)).toEqual(['read']); + expect(readAgainMessages()).toHaveLength(2); + }); + }); + it('holds back a file whose content moved to it from another configured file', async () => { // The content in effect comes from the second file. Moving it to the first leaves the // contents the same, so the load applies nothing, but the first file now contributes them. From f461ec11833dd1acf075eeb85472a8abff2c258f Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 02:59:01 +0000 Subject: [PATCH 06/12] fix: Bound each held-back file on its own, and read the clock after each file's read A review of the round-4 rule proved three regressions: - The clock was read once per reload, so a slow read of an earlier file made a later file's emptiness look older and shortened its delay. The clock is now read after each file's own read, and again when the hold's retry delay is computed. - A failed reload ended the run of holding reloads, so a file that kept being written while empty was held without end when another file kept failing. - The bound was shared, so one file reaching it applied another file that was a fraction of a delay into an ordinary rewrite. The run cap is replaced by a bound for each file: it starts at the first reload that holds the file back, is kept across reads with content and across failed reloads, and starts over when data is applied or when the file has stayed empty for the whole delay. The hold's retry comes when the first held file's delay or bound ends, plus a tenth of the retry delay. The reloader records the delay of the last retry it armed, and tests pin it. --- .../FileReloader.inPlaceRewrite.test.ts | 216 +++++++++++++++++- .../api/integrations/FileDataSourceOptions.ts | 2 +- .../src/api/options/LDDataSystemOptions.ts | 2 +- .../src/data_sources/filedata/FilePoller.ts | 4 +- .../src/data_sources/filedata/FileReloader.ts | 123 ++++++---- .../src/overrides/FileOverrideSource.ts | 2 +- 6 files changed, 296 insertions(+), 53 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index d789dc0df5..d48d528392 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -385,7 +385,7 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(failures).toHaveLength(failed); }); - it('starts a fresh hold for a file that is deleted and created empty again', async () => { + it('starts a fresh delay for a file that is deleted and created empty again', async () => { write(jsonPath, documentA); makeReloader([jsonPath], (config) => { config.policy = testPolicy({ missingFile: 'fail' }); @@ -401,17 +401,16 @@ describe('given a reloader over a file that is rewritten in place', () => { const reported = () => failures.filter((failure) => !failure.repeated); expect(reported().map((failure) => failure.kind)).toEqual(['read']); - // The data in effect still has the file's content, so the empty file is held back. The failed - // loads ended the run of holding loads, so a load more than five retry delays after the first - // hold still holds it back. - await advance(4400); + // The data in effect still has the file's content, so the empty file is held back, with a + // delay that starts when it is found empty again. + await advance(1900); write(jsonPath, ''); await changeAndSettle(); - await advance(600); + await advance(500); await changeAndSettle(); expect(reported().map((failure) => failure.kind)).toEqual(['read']); - await advance(holdRetryMs - 600 - 1); + await advance(holdRetryMs - 500 - 1); expect(reported().map((failure) => failure.kind)).toEqual(['read']); await advance(1); expect(reported().map((failure) => failure.kind)).toEqual(['read', 'parse']); @@ -423,6 +422,35 @@ describe('given a reloader over a file that is rewritten in place', () => { // clock do, so a file that is empty again looks unchanged since it was last found empty. const writeCoarse = (path: string, data: string) => filesystem.set(path, data, 1); + it('starts a fresh delay for a file emptied again after a failed load read its content', async () => { + writeCoarse(yamlPath, documentA); + writeCoarse(jsonPath, otherA); + makeReloader([yamlPath, jsonPath]); + await reloader.reloadNow(); + + writeCoarse(yamlPath, ''); + await changeAndSettle(); + // The file gets its content, but another file is malformed, so nothing is applied. + await advance(300); + writeCoarse(yamlPath, documentB); + writeCoarse(jsonPath, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(1); + + // The file is emptied again, more than a retry delay after it was first found empty. + await advance(1200); + writeCoarse(yamlPath, ''); + await changeAndSettle(); + await advance(100); + writeCoarse(jsonPath, otherB); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + + writeCoarse(yamlPath, documentC); + await changeAndSettle(); + expect(values()).toEqual(['a,a', 'c,b']); + }); + it('starts a fresh delay for a file that is emptied again after its content was read', async () => { writeCoarse(yamlPath, documentA); makeReloader([yamlPath]); @@ -465,6 +493,180 @@ describe('given a reloader over a file that is rewritten in place', () => { }); }); + it('does not restart the bound of a file that is held back when another file fails', async () => { + write(yamlPath, documentA); + write(jsonPath, otherA); + makeReloader([yamlPath, jsonPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + // Every 600 ms the first file is written and truncated again before a load reads it, and the + // other file is malformed at every other step, which fails those loads. + await [1, 2, 3, 4, 5, 6, 7, 8].reduce>( + (previous, step) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + write(jsonPath, step % 2 === 0 ? '{"flagValues": ' : otherA); + await changeAndSettle(); + }), + Promise.resolve(), + ); + expect(values()).toEqual(['a,a']); + expect(failures.length).toBeGreaterThan(0); + + // 5400 ms after it was first held back, the file is applied although it was just written. + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + write(jsonPath, otherB); + await changeAndSettle(); + expect(values()).toEqual(['a,a', 'b']); + }); + + it("does not cut another file's rewrite short when one file's bound is over", async () => { + write(yamlPath, documentA); + write(otherYamlPath, otherA); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + + // The first file keeps being written and truncated again before every load. + write(yamlPath, ''); + await changeAndSettle(); + await Array.from({ length: 7 }).reduce>( + (previous) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + }), + Promise.resolve(), + ); + // At 4750 ms, a quarter of a retry delay before the first file's bound is over, the second + // file starts an ordinary in-place rewrite. + await advance(550); + write(otherYamlPath, ''); + await changeAndSettle(); + await advance(50); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + + // At 5100 ms the retry finds the first file's bound over, and the second file still in its + // delay, so nothing is applied. + await advance(300); + expect(values()).toEqual(['a,a']); + + write(otherYamlPath, otherB); + await changeAndSettle(); + expect(values()).toEqual(['a,a', 'b']); + }); + + it('reads the clock after each file is read, so a slow read of another file does not shorten the delay', async () => { + write(otherYamlPath, otherA); + write(yamlPath, documentA); + makeReloader([otherYamlPath, yamlPath]); + await reloader.reloadNow(); + + // The read of the first file takes 900 ms. The second file is empty when it is read. + write(yamlPath, ''); + filesystem.deferReads = true; + reloader.trigger(); + await advance(0); + expect(filesystem.pendingReads).toHaveLength(1); + await advance(900); + filesystem.deferReads = false; + filesystem.completePendingReads(); + await advance(0); + expect(readAgainMessages()).toHaveLength(1); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); + + // The delay started when the second file was read, so a load 500 ms later still holds it back. + await advance(500); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + }); + + describe('the retry that a hold asks for', () => { + it('comes a retry delay and a tenth after the first hold', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); + await changeAndSettle(); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); + }); + + it('comes when the delay ends, plus a tenth, after a later hold', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); + await changeAndSettle(); + await advance(500); + await changeAndSettle(); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs - 500); + }); + + it('comes when the bound ends, plus a tenth, when that is sooner', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); + await changeAndSettle(); + await Array.from({ length: 8 }).reduce>( + (previous) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + }), + Promise.resolve(), + ); + // At 4800 ms the file's delay would end at 5800 ms, and its bound ends at 5000 ms. + expect(reloader.lastRetryDelayMs).toEqual(5000 - 4800 + retryDelayMs / 10); + }); + + it.each([ + ['first', [yamlPath, otherYamlPath]], + ['second', [otherYamlPath, yamlPath]], + ])( + 'comes when the file whose delay ends first is due, when that file is configured %s', + async (_order, paths) => { + write(yamlPath, documentA); + write(otherYamlPath, otherA); + makeReloader(paths); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(400); + write(otherYamlPath, ''); + await changeAndSettle(); + expect(readAgainMessages()).toHaveLength(3); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs - 400); + }, + ); + + it('is computed the same way for a hold by a retry', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, '{"flagValues": '); + await changeAndSettle(); + expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs); + + write(yamlPath, ''); + await advance(retryDelayMs); + expect(readAgainMessages()).toHaveLength(1); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); + }); + + it('is a retry delay after a failure', async () => { + await startWith(jsonPath, documentA); + write(jsonPath, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(1); + expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs); + }); + }); + it('holds back a file whose content moved to it from another configured file', async () => { // The content in effect comes from the second file. Moving it to the first leaves the // contents the same, so the load applies nothing, but the first file now contributes them. diff --git a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts index ddab047e76..4593a128f6 100644 --- a/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts +++ b/packages/shared/sdk-server/src/api/integrations/FileDataSourceOptions.ts @@ -19,7 +19,7 @@ export interface FileDataSourceOptions { * short delay and the previously loaded data stays in effect until it succeeds. * A file can be rewritten in place: a file whose content is in effect and that is found empty is * held back until it has stayed empty, without being written, for about a second, and for at most - * about five seconds while files keep being rewritten. Changes to the other files wait with it, + * about five seconds while that file keeps being rewritten. Changes to the other files wait with it, * because a load applies all of the files or none of them. A file that is still empty then fails * to load, which is reported, and the previously loaded data stays in effect. YAML that is cut * off between two entries can still parse, so for a YAML file, replacing its entire contents in diff --git a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts index 92ef42b942..3d47f3a879 100644 --- a/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts +++ b/packages/shared/sdk-server/src/api/options/LDDataSystemOptions.ts @@ -123,7 +123,7 @@ export interface LDDataSystemOptions { * with no entries, such as `{}`. A file whose content is in effect and that is found empty, or * with only a YAML comment or document marker, may be in the middle of a rewrite. The source holds * it back until it has stayed so, without being written, for about a second, and for at most about - * five seconds while files keep being rewritten. Changes to the other files wait with it, because + * five seconds while that file keeps being rewritten. Changes to the other files wait with it, because * a reload applies all of the files or none of them. After that, an empty YAML file removes its * overrides. An empty JSON file is not a valid document: it fails to load, and the overrides in * effect stay. YAML that is cut off between two entries can still parse, so for a YAML file, diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts b/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts index f5790e8b03..a96a9c33f0 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FilePoller.ts @@ -1,8 +1,8 @@ import { Filesystem } from '@launchdarkly/js-sdk-common'; /** - * The observed state of one file, or its absence: its modification time and size, read through - * any symbolic link. + * The observed state of one file, or its absence: its modification time and size, as the + * platform's `getFileStats` reports them. The Node platform reads them through any symbolic link. * * @internal */ diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 47f1f6d17a..4f38cfa74d 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -33,9 +33,9 @@ export const DEFAULT_RETRY_DELAY_MS = 1000; export const MAX_DEBOUNCE_DELAY_MS = 1000; /** - * Reloads stop holding files back once they have held some back for this many retry delays in a - * row, so that a file that keeps being written while every reload finds it empty, or files that - * are rewritten in turn, are not held back without end. + * Reloads stop holding a file back once they have held it back for this many retry delays, so + * that a file that keeps being written while every reload finds it empty, or files that are + * rewritten in turn, are not held back without end. * * @internal */ @@ -116,7 +116,7 @@ export interface FileReloaderConfig { /** * How long to wait after a failed reload before retrying it without a trigger. A file whose * content is in effect and that is found empty is not applied until it has stayed empty, without - * being written, for the same delay, or until reloads have held files back for + * being written, for the same delay, or until reloads have held it back for * `MAX_HOLD_RETRY_DELAYS` times the delay. Zero or negative disables the automatic retry, and an * empty file is then parsed as it is. */ @@ -131,12 +131,16 @@ export interface FileReloaderConfig { type ReloadReason = 'initial' | 'change' | 'retry'; /** - * A file whose content is in effect and that reloads have found empty: since when it has been - * empty without being written, on the monotonic clock, and its state then. + * A file whose content is in effect and that reloads have found empty, with times on the monotonic + * clock. `since` is since when it has been empty without being written, and `state` its state + * then; `since` is undefined after a reload found the file with content or missing. `firstHeld` is + * when reloads began holding it back, kept across reads with content and across failed reloads, + * so that a file that keeps being rewritten is bounded. */ interface EmptyFile { - since: number; - state: FileState; + since?: number; + state?: FileState; + firstHeld?: number; } /** @@ -194,9 +198,8 @@ export default class FileReloader { // The paths whose content is in effect and that reloads have found empty since. private readonly _emptyFiles = new Map(); - // When the current run of reloads that hold files back began, on the monotonic clock. Undefined - // when the last reload did not hold any back. - private _holdingSince?: number; + // The delay of the retry that was armed last. Tests read it. + private _lastRetryDelayMs?: number; // Set by a reload that held back an empty file and armed the retry to read it again, to that // reload's reason, until the next reload starts. The next reload continues the deferred one, so @@ -215,6 +218,15 @@ export default class FileReloader { return this._enqueue('initial'); } + /** + * The delay of the retry that was armed last, in milliseconds, if any. For tests. + * + * @internal + */ + get lastRetryDelayMs(): number | undefined { + return this._lastRetryDelayMs; + } + /** * Signals that the files may have changed. The reload happens after the debounce delay, or no * later than the maximum debounce delay after the first trigger of a burst. Triggers that @@ -296,6 +308,7 @@ export default class FileReloader { return; } this._clearRetry(); + this._lastRetryDelayMs = delayMs; this._retryTimer = setTimeout(() => { this._retryTimer = undefined; this._enqueue('retry'); @@ -328,7 +341,6 @@ export default class FileReloader { // A pending retry is superseded by this reload. It either succeeds, or it fails and arms a // fresh retry. this._clearRetry(); - const now = monotonicNow(); const documents: FileDataDocument[] = []; const files: FileSummary[] = []; @@ -352,7 +364,7 @@ export default class FileReloader { } catch (err) { const missing = isFileNotFoundError(err); if (missing) { - this._emptyFiles.delete(path); + this._fileChanged(path); } if (missing && policy.missingFile === 'skip') { logger?.debug(`File ${path} does not exist. It contributes no data.`); @@ -379,8 +391,8 @@ export default class FileReloader { } const noEntries = blank || isNoDocument(document); if (!noEntries) { - this._emptyFiles.delete(path); - } else if (await this._holdsBack(path, now)) { + this._fileChanged(path); + } else if (await this._holdsBack(path)) { heldBack.push(path); return; } @@ -404,11 +416,9 @@ export default class FileReloader { // A file that cannot be read or parsed fails the reload, whether or not another file is held // back. if (!failure && heldBack.length > 0) { - this._holdBack(heldBack, now, reason); + this._holdBack(heldBack, reason); return; } - // A run of reloads that hold files back ends with any reload that does not. - this._holdingSince = undefined; let merged: MergeResult | undefined; if (!failure) { @@ -469,8 +479,19 @@ export default class FileReloader { this._rememberAppliedContent(hadContent); } + // Called when the data of a reload is in effect. The files' bounds start over. private _rememberAppliedContent(hadContent: Map): void { hadContent.forEach((value, path) => this._appliedHadContent.set(path, value)); + this._emptyFiles.clear(); + } + + // Called when a reload finds a file with content, or missing. The next time a reload finds it + // empty, its delay starts over. Its bound does not. + private _fileChanged(path: string): void { + const empty = this._emptyFiles.get(path); + if (empty) { + empty.since = undefined; + } } /** @@ -479,59 +500,79 @@ export default class FileReloader { * A file whose content is in effect and that has none now may be in the middle of an in-place * rewrite: a writer that truncates the file and then writes the new content. Such a file is held * back until it has stayed so, without being written, for the retry delay. A change of its - * state, its modification time and size read through any symbolic link, means that it was - * written since a reload last found it so, for example rewritten and truncated again between two + * state, its modification time and size as the platform reports them, means that it was written + * since a reload last found it so, for example rewritten and truncated again between two * reloads, so the delay starts over. A file that keeps being written while every reload finds it * empty, or files that are rewritten in turn so that every reload finds one of them empty, would - * be held back without end, so reloads stop holding files back once they have held some back for - * `MAX_HOLD_RETRY_DELAYS` retry delays in a row. A file whose content is not in effect, such as a - * placeholder that has always been empty, is parsed right away, and so is any file when retries - * are off, since nothing would read it again. + * be held back without end, so a file is held back for at most `MAX_HOLD_RETRY_DELAYS` retry + * delays. That bound is the file's own: it starts at the first reload that holds the file back, + * and starts over only when data is applied or when the file has stayed empty for the whole + * delay. A file whose content is not in effect, such as a placeholder that has always been empty, + * is parsed right away, and so is any file when retries are off, since nothing would read it + * again. + * + * The clock is read after the file's own read, so that a slow read of another file does not + * shorten this file's delay. */ - private async _holdsBack(path: string, now: number): Promise { + private async _holdsBack(path: string): Promise { const { retryDelayMs } = this._config; if (retryDelayMs <= 0 || this._appliedHadContent.get(path) !== true) { return false; } const state = await observeFile(this._config.filesystem, path); + const now = monotonicNow(); let empty = this._emptyFiles.get(path); if (!empty) { - empty = { since: now, state }; + empty = {}; this._emptyFiles.set(path, empty); - } else if (!sameFileState(empty.state, state)) { + } + if (empty.since === undefined || !empty.state || !sameFileState(empty.state, state)) { empty.since = now; empty.state = state; } - return ( - now - empty.since < retryDelayMs && - now - (this._holdingSince ?? now) < MAX_HOLD_RETRY_DELAYS * retryDelayMs - ); + if (now - empty.since >= retryDelayMs) { + // The file stayed empty for the whole delay. A later rewrite gets a fresh bound. + empty.firstHeld = undefined; + return false; + } + if ( + empty.firstHeld !== undefined && + now - empty.firstHeld >= MAX_HOLD_RETRY_DELAYS * retryDelayMs + ) { + return false; + } + empty.firstHeld = empty.firstHeld ?? now; + return true; } /** * Holds back the files that `_holdsBack` decided to hold back. Whatever their format, the reload * keeps the current data and arms the retry without reporting a failure. The retry comes when the - * first of the files has stayed empty for the retry delay, or when the reloads have held files - * back for as long as they may, plus a tenth of the retry delay, so that a timer that fires a - * little early still finds the delay over. The change signal for a write starts a reload sooner. - * Once a file has stayed empty for the retry delay, a reload parses it: a file that is still - * empty is invalid JSON, or a YAML document with no entries. + * first of the files has stayed empty for the retry delay, or has been held back for as long as + * it may be, plus a tenth of the retry delay, so that a timer that fires a little early still + * finds the time over. The change signal for a write starts a reload sooner. Once a file has + * stayed empty for the retry delay, a reload parses it: a file that is still empty is invalid + * JSON, or a YAML document with no entries. */ - private _holdBack(paths: string[], now: number, reason: ReloadReason): void { + private _holdBack(paths: string[], reason: ReloadReason): void { if (this._closed) { return; } const { logger, retryDelayMs } = this._config; - this._holdingSince = this._holdingSince ?? now; - let until = this._holdingSince + MAX_HOLD_RETRY_DELAYS * retryDelayMs; + let until = Infinity; paths.forEach((path) => { logger?.debug( `File ${path} is empty, possibly while it is being rewritten; reading it again before applying it`, ); - until = Math.min(until, this._emptyFiles.get(path)!.since + retryDelayMs); + const { since, firstHeld } = this._emptyFiles.get(path)!; + until = Math.min( + until, + since! + retryDelayMs, + firstHeld! + MAX_HOLD_RETRY_DELAYS * retryDelayMs, + ); }); this._readAgainReason = reason; - this._armRetry(Math.max(until - now, 0) + retryDelayMs / 10); + this._armRetry(Math.max(until - monotonicNow(), 0) + retryDelayMs / 10); } private _fail(failure: LoadFailure, reason: ReloadReason): void { diff --git a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts index 162a5e2325..91af292363 100644 --- a/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts +++ b/packages/shared/sdk-server/src/overrides/FileOverrideSource.ts @@ -147,7 +147,7 @@ function countsText(flags: number, segments: number): string { * cannot be parsed. A file whose content is in effect and that is found with no entries, empty or * with only a YAML comment, may be in the middle of an in-place rewrite, so it is held back until * it has stayed so, without being written, for the retry delay, and for at most five retry delays - * while files keep being rewritten; the other files wait with it. Every applied change is logged + * while that file keeps being rewritten; the other files wait with it. Every applied change is logged * at Info level with the overrides in effect and what each file supplied. * * Flag overrides are currently experimental and subject to change. From a9b345492cecbda9088b3f559ae5810de32304a8 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 03:10:50 +0000 Subject: [PATCH 07/12] test: Hold back a file emptied again after a read of its content when its state looks the same --- .../FileReloader.inPlaceRewrite.test.ts | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index d48d528392..f6ce326809 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -667,6 +667,38 @@ describe('given a reloader over a file that is rewritten in place', () => { }); }); + it('holds back a file emptied again after a read of its content when its state looks the same', async () => { + // On a filesystem that records modification times coarsely, a file emptied a second time can + // look exactly as it did the first time. The read of its content in between still starts its + // delay over. The other file is kept written while empty, so that no load applies. + write(yamlPath, documentA); + write(otherYamlPath, otherA); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + const firstEmptyModified = modified; + write(otherYamlPath, ''); + await changeAndSettle(); + expect(readAgainMessages()).toHaveLength(2); + + await advance(500); + write(yamlPath, documentA); + write(otherYamlPath, ''); + await changeAndSettle(); + + await advance(400); + filesystem.set(yamlPath, '', firstEmptyModified); + write(otherYamlPath, ''); + await changeAndSettle(); + + // The other file gets its content back while the first file is still within its new delay. + await advance(700); + write(otherYamlPath, otherB); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + }); + it('holds back a file whose content moved to it from another configured file', async () => { // The content in effect comes from the second file. Moving it to the first leaves the // contents the same, so the load applies nothing, but the first file now contributes them. From e8a950caa511b2c76a2258063818ba79108c5924 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 03:21:36 +0000 Subject: [PATCH 08/12] test: Name the retry delay tests instead of grouping them --- .../FileReloader.inPlaceRewrite.test.ts | 134 +++++++++--------- 1 file changed, 66 insertions(+), 68 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index f6ce326809..5ba3bbf4e4 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -590,81 +590,79 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a,a']); }); - describe('the retry that a hold asks for', () => { - it('comes a retry delay and a tenth after the first hold', async () => { - await startWith(yamlPath, documentA); - write(yamlPath, ''); - await changeAndSettle(); - expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); - }); - - it('comes when the delay ends, plus a tenth, after a later hold', async () => { - await startWith(yamlPath, documentA); - write(yamlPath, ''); - await changeAndSettle(); - await advance(500); - await changeAndSettle(); - expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs - 500); - }); + it('asks for a retry a retry delay and a tenth after the first hold', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); + await changeAndSettle(); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); + }); - it('comes when the bound ends, plus a tenth, when that is sooner', async () => { - await startWith(yamlPath, documentA); - write(yamlPath, ''); - await changeAndSettle(); - await Array.from({ length: 8 }).reduce>( - (previous) => - previous.then(async () => { - await advance(600); - write(yamlPath, documentB); - write(yamlPath, ''); - await changeAndSettle(); - }), - Promise.resolve(), - ); - // At 4800 ms the file's delay would end at 5800 ms, and its bound ends at 5000 ms. - expect(reloader.lastRetryDelayMs).toEqual(5000 - 4800 + retryDelayMs / 10); - }); + it('asks for a retry when the delay ends, plus a tenth, after a later hold', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); + await changeAndSettle(); + await advance(500); + await changeAndSettle(); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs - 500); + }); - it.each([ - ['first', [yamlPath, otherYamlPath]], - ['second', [otherYamlPath, yamlPath]], - ])( - 'comes when the file whose delay ends first is due, when that file is configured %s', - async (_order, paths) => { - write(yamlPath, documentA); - write(otherYamlPath, otherA); - makeReloader(paths); - await reloader.reloadNow(); - - write(yamlPath, ''); - await changeAndSettle(); - await advance(400); - write(otherYamlPath, ''); - await changeAndSettle(); - expect(readAgainMessages()).toHaveLength(3); - expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs - 400); - }, + it('asks for a retry when the bound ends, plus a tenth, when that is sooner', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, ''); + await changeAndSettle(); + await Array.from({ length: 8 }).reduce>( + (previous) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + await changeAndSettle(); + }), + Promise.resolve(), ); + // At 4800 ms the file's delay would end at 5800 ms, and its bound ends at 5000 ms. + expect(reloader.lastRetryDelayMs).toEqual(5000 - 4800 + retryDelayMs / 10); + }); + + it.each([ + ['first', [yamlPath, otherYamlPath]], + ['second', [otherYamlPath, yamlPath]], + ])( + 'asks for a retry when the file whose delay ends first is due, when that file is configured %s', + async (_order, paths) => { + write(yamlPath, documentA); + write(otherYamlPath, otherA); + makeReloader(paths); + await reloader.reloadNow(); - it('is computed the same way for a hold by a retry', async () => { - await startWith(yamlPath, documentA); - write(yamlPath, '{"flagValues": '); + write(yamlPath, ''); + await changeAndSettle(); + await advance(400); + write(otherYamlPath, ''); await changeAndSettle(); - expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs); + expect(readAgainMessages()).toHaveLength(3); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs - 400); + }, + ); - write(yamlPath, ''); - await advance(retryDelayMs); - expect(readAgainMessages()).toHaveLength(1); - expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); - }); + it('asks for a retry the same way for a hold by a retry', async () => { + await startWith(yamlPath, documentA); + write(yamlPath, '{"flagValues": '); + await changeAndSettle(); + expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs); - it('is a retry delay after a failure', async () => { - await startWith(jsonPath, documentA); - write(jsonPath, '{"flagValues": '); - await changeAndSettle(); - expect(failures).toHaveLength(1); - expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs); - }); + write(yamlPath, ''); + await advance(retryDelayMs); + expect(readAgainMessages()).toHaveLength(1); + expect(reloader.lastRetryDelayMs).toEqual(holdRetryMs); + }); + + it('asks for a retry a retry delay after a failure', async () => { + await startWith(jsonPath, documentA); + write(jsonPath, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(1); + expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs); }); it('holds back a file emptied again after a read of its content when its state looks the same', async () => { From 0e990f0a9365ae2f2342beb47aa7cfd407c175b2 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 04:16:46 +0000 Subject: [PATCH 09/12] fix: Start a held-back file's bound over only at a reload that holds nothing back A review of the per-file bound proved two holes. A file that stayed empty for the whole delay cleared its own bound while another file kept every reload holding, so two files emptied in staggered turns were held back without end. And a file held back while another file failed kept its bound, so a fix of the other file long after, while this file was being rewritten, applied this file empty. A file's bound now starts over only at a reload that holds nothing back, for every file that the reload read, whatever its outcome; a file that the reload did not get to keeps its bound. The bound is no longer cleared when a file stays empty for the whole delay. --- .../FileReloader.inPlaceRewrite.test.ts | 83 +++++++++++++++++++ .../src/data_sources/filedata/FileReloader.ts | 25 ++++-- 2 files changed, 101 insertions(+), 7 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index 5ba3bbf4e4..e773fc3527 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -565,6 +565,89 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a,a', 'b']); }); + it('applies files that are emptied in staggered turns once their bounds are over', async () => { + // Two files stay empty and are truncated again in turn, so that whenever one of them has + // stayed empty for the whole delay, the other is still within its delay and holds the load + // back. A file that settles so does not get a fresh bound while the other is held back. + write(yamlPath, documentA); + write(otherYamlPath, otherA); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + expect(readAgainMessages()).toHaveLength(1); + // Every 900 ms one file is truncated again, with a load then and one 200 ms later, standing + // for the retry. The second file is emptied at the first step. + await Array.from({ length: 14 }, (_, index) => index + 1).reduce>( + (previous, step) => + previous.then(async () => { + if (applied.length > 1) { + return; + } + await advance(step === 1 ? 900 : 700); + write(step % 2 === 1 ? otherYamlPath : yamlPath, ''); + await changeAndSettle(); + await advance(200); + await changeAndSettle(); + }), + Promise.resolve(), + ); + expect(applied.length).toBeGreaterThan(1); + }); + + it('starts the bound of a file held back during a failure over once a failed load reads it', async () => { + // While the other file fails to load, the first file is rewritten once and its gap is held + // back. Much later, the other file is fixed while the first file is in the middle of another + // rewrite, which is held back with a bound of its own. + write(yamlPath, documentA); + write(jsonPath, otherA); + makeReloader([yamlPath, jsonPath]); + await reloader.reloadNow(); + + write(jsonPath, '{"flagValues": '); + write(yamlPath, ''); + await changeAndSettle(); + write(yamlPath, documentB); + await changeAndSettle(); + await advance(6 * retryDelayMs); + // A reload that fails reports the failure, not the files it holds back. + expect(readAgainMessages()).toEqual([]); + + write(jsonPath, otherB); + write(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + expect(readAgainMessages()).toHaveLength(1); + }); + + it('asks for the retry from the end of the load, when the read of another file takes longer than the rest of the delay', async () => { + write(yamlPath, documentA); + write(otherYamlPath, otherA); + makeReloader([yamlPath, otherYamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + await advance(retryDelayMs * 0.95); + + // The first file is still within its delay when it is read. The read of the second file + // takes 60 ms, past the end of that delay, so the retry is due right away, after the margin. + const readFile = filesystem.readFile.bind(filesystem); + jest.spyOn(filesystem, 'readFile').mockImplementation(async (path: string) => { + if (path === otherYamlPath) { + await new Promise((resolve) => { + setTimeout(resolve, 60); + }); + } + return readFile(path); + }); + reloader.trigger(); + await advance(60); + expect(readAgainMessages()).toHaveLength(2); + expect(reloader.lastRetryDelayMs).toEqual(retryDelayMs / 10); + }); + it('reads the clock after each file is read, so a slow read of another file does not shorten the delay', async () => { write(otherYamlPath, otherA); write(yamlPath, documentA); diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 4f38cfa74d..28745a775d 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -134,8 +134,8 @@ type ReloadReason = 'initial' | 'change' | 'retry'; * A file whose content is in effect and that reloads have found empty, with times on the monotonic * clock. `since` is since when it has been empty without being written, and `state` its state * then; `since` is undefined after a reload found the file with content or missing. `firstHeld` is - * when reloads began holding it back, kept across reads with content and across failed reloads, - * so that a file that keeps being rewritten is bounded. + * when a reload first held it back since a reload that held nothing back read it, so that a file + * that keeps being rewritten, or files that are rewritten in turn, are bounded. */ interface EmptyFile { since?: number; @@ -396,6 +396,8 @@ export default class FileReloader { heldBack.push(path); return; } + // The file was read and is not held back, whether or not its empty content parses. + hadContent.set(path, !noEntries); if (blank) { try { document = policy.parseDocument(path, data); @@ -404,7 +406,6 @@ export default class FileReloader { return; } } - hadContent.set(path, !noEntries); contents.push(data); documents.push(document as FileDataDocument); files.push({ path, present: true, flags: 0, segments: 0 }); @@ -413,6 +414,18 @@ export default class FileReloader { ); await readAll; + if (heldBack.length === 0) { + // A reload that holds nothing back, whether it applies, skips or fails, starts the bound over + // for every file that it read: such a file is no longer held, so its next rewrite is a new + // one. A file that the reload did not get to keeps its bound. + hadContent.forEach((_hadContent, path) => { + const empty = this._emptyFiles.get(path); + if (empty) { + empty.firstHeld = undefined; + } + }); + } + // A file that cannot be read or parsed fails the reload, whether or not another file is held // back. if (!failure && heldBack.length > 0) { @@ -506,8 +519,8 @@ export default class FileReloader { * empty, or files that are rewritten in turn so that every reload finds one of them empty, would * be held back without end, so a file is held back for at most `MAX_HOLD_RETRY_DELAYS` retry * delays. That bound is the file's own: it starts at the first reload that holds the file back, - * and starts over only when data is applied or when the file has stayed empty for the whole - * delay. A file whose content is not in effect, such as a placeholder that has always been empty, + * and starts over only at a reload that holds nothing back and reads the file, or when data is + * applied. A file whose content is not in effect, such as a placeholder that has always been empty, * is parsed right away, and so is any file when retries are off, since nothing would read it * again. * @@ -531,8 +544,6 @@ export default class FileReloader { empty.state = state; } if (now - empty.since >= retryDelayMs) { - // The file stayed empty for the whole delay. A later rewrite gets a fresh bound. - empty.firstHeld = undefined; return false; } if ( From 7bcc858881587d5efbdd1a1b66df846b71ef7111 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 05:31:28 +0000 Subject: [PATCH 10/12] fix: Start the bound over for a file that fails to load, and end its empty state A review proved that a file whose read or parse failed right after a hold kept its old bound, because the bound was started over only for files that a reload read without failing. An operator who saved a YAML typo in place and then the fix in place seconds later got the fix's truncation applied empty, and a JSON fix reported a load failure. A reload now records every file it gets to and does not hold back, including the file that fails to read or parse and a missing file, and a reload that holds nothing back starts all of their bounds over. A file after the one that failed keeps its bound. Content, even content that fails to parse, ends a file's empty state, as a missing file does. --- .../FileReloader.inPlaceRewrite.test.ts | 162 ++++++++++++++++++ .../src/data_sources/filedata/FileReloader.ts | 32 ++-- 2 files changed, 184 insertions(+), 10 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index e773fc3527..3474f72876 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -470,6 +470,26 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(readAgainMessages()).toHaveLength(2); }); + it('starts a fresh delay for a file emptied again after content that fails to parse', async () => { + writeCoarse(yamlPath, documentA); + makeReloader([yamlPath]); + await reloader.reloadNow(); + + writeCoarse(yamlPath, ''); + await changeAndSettle(); + writeCoarse(yamlPath, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(1); + + // More than a retry delay after the first emptying, the file is emptied again, and looks as + // it did then. + await advance(1500); + writeCoarse(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a']); + expect(readAgainMessages()).toHaveLength(2); + }); + it('starts a fresh delay for a file that is deleted and created empty again', async () => { writeCoarse(jsonPath, documentA); makeReloader([jsonPath], (config) => { @@ -621,6 +641,148 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(readAgainMessages()).toHaveLength(1); }); + it.each([yamlPath, jsonPath])( + 'holds back the in-place save that fixes %s after it was saved in place with an error', + async (path) => { + await startWith(path, documentA); + + write(path, ''); + await changeAndSettle(); + expect(readAgainMessages()).toHaveLength(1); + write(path, '{"flagValues": '); + await changeAndSettle(); + expect(failures).toHaveLength(1); + // The broken file keeps failing for longer than the bound. + await advance(6 * retryDelayMs); + const failed = failures.length; + + // The fix is saved in place. Its truncation is held back, and no failure is reported. + write(path, ''); + await changeAndSettle(); + expect(failures).toHaveLength(failed); + expect(readAgainMessages()).toHaveLength(2); + write(path, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + }, + ); + + it('holds back a file created empty again after it was missing for longer than the bound', async () => { + write(jsonPath, documentA); + makeReloader([jsonPath], (config) => { + config.policy = testPolicy({ missingFile: 'fail' }); + }); + await reloader.reloadNow(); + + write(jsonPath, ''); + await changeAndSettle(); + filesystem.remove(jsonPath); + await changeAndSettle(); + await advance(6 * retryDelayMs); + const failed = failures.length; + + // The file is created empty, and then written. + write(jsonPath, ''); + await changeAndSettle(); + expect(failures).toHaveLength(failed); + expect(readAgainMessages()).toHaveLength(2); + write(jsonPath, documentB); + await changeAndSettle(); + expect(values()).toEqual(['a', 'b']); + }); + + it('keeps the bound of a file that a failing load did not get to', async () => { + write(jsonPath, otherA); + write(yamlPath, documentA); + makeReloader([jsonPath, yamlPath]); + await reloader.reloadNow(); + + // The second file keeps being written while empty. The first file fails at every other load, + // before the second file is read. + write(yamlPath, ''); + await changeAndSettle(); + await [1, 2, 3, 4, 5, 6, 7, 8].reduce>( + (previous, step) => + previous.then(async () => { + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + write(jsonPath, step % 2 === 0 ? '{"flagValues": ' : otherA); + await changeAndSettle(); + }), + Promise.resolve(), + ); + expect(values()).toEqual(['a,a']); + + // 5400 ms after it was first held back, the second file is applied although it was just + // written. + await advance(600); + write(yamlPath, documentB); + write(yamlPath, ''); + write(jsonPath, otherB); + await changeAndSettle(); + expect(values()).toEqual(['a,a', 'b']); + }); + + it('does not start bounds over at a load that holds a file back and then fails', async () => { + const thirdPath = '/data/third.json'; + write(yamlPath, documentA); + write(otherYamlPath, otherA); + write(thirdPath, '{"flagValues": {"flag3": "a"}}'); + makeReloader([yamlPath, otherYamlPath, thirdPath]); + await reloader.reloadNow(); + + // The first two files are rewritten in turn, so that every load holds one of them back, and + // the third file fails at every other load, after that. + write(yamlPath, ''); + await changeAndSettle(); + await Array.from({ length: 12 }, (_, index) => index + 1).reduce>( + (previous, step) => + previous.then(async () => { + if (applied.length > 1) { + return; + } + await advance(500); + if (step % 2 === 1) { + write(yamlPath, documentB); + write(otherYamlPath, ''); + write(thirdPath, '{"flagValues": '); + } else { + write(otherYamlPath, otherB); + write(yamlPath, ''); + write(thirdPath, '{"flagValues": {"flag3": "b"}}'); + } + await changeAndSettle(); + }), + Promise.resolve(), + ); + expect(applied.length).toBeGreaterThan(1); + }); + + it('starts a new delay for a missing file when it is created empty again with the same state', async () => { + write(yamlPath, documentA); + write(jsonPath, otherA); + makeReloader([yamlPath, jsonPath]); + await reloader.reloadNow(); + + write(jsonPath, '{"flagValues": '); + write(yamlPath, ''); + const firstEmptyModified = modified; + await changeAndSettle(); + filesystem.remove(yamlPath); + await changeAndSettle(); + expect(failures.length).toBeGreaterThan(0); + + // The file is created empty again, with the modification time of its first emptying, more than + // a retry delay after that. The other file is fixed. + await advance(1170); + filesystem.set(yamlPath, '', firstEmptyModified); + write(jsonPath, otherB); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + expect(readAgainMessages()).toHaveLength(1); + }); + it('asks for the retry from the end of the load, when the read of another file takes longer than the rest of the delay', async () => { write(yamlPath, documentA); write(otherYamlPath, otherA); diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 28745a775d..2bd1ad3176 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -351,6 +351,9 @@ export default class FileReloader { // The empty files that this reload holds back. The other files are still read, so that files // that are emptied together start their delays in the same reload. const heldBack: string[] = []; + // Every file that this reload got to and did not hold back, including a file that failed to + // read or parse and a missing file. + const reached: string[] = []; const readAll = paths.reduce( (previous, path) => @@ -362,6 +365,7 @@ export default class FileReloader { try { data = await this._config.filesystem.readFile(path); } catch (err) { + reached.push(path); const missing = isFileNotFoundError(err); if (missing) { this._fileChanged(path); @@ -385,6 +389,9 @@ export default class FileReloader { try { document = policy.parseDocument(path, data); } catch (err) { + // Content ends the file's empty state, even content that fails to parse. + reached.push(path); + this._fileChanged(path); failure = { kind: 'parse', error: asError(err), path, repeated: false }; return; } @@ -396,7 +403,7 @@ export default class FileReloader { heldBack.push(path); return; } - // The file was read and is not held back, whether or not its empty content parses. + reached.push(path); hadContent.set(path, !noEntries); if (blank) { try { @@ -416,9 +423,9 @@ export default class FileReloader { if (heldBack.length === 0) { // A reload that holds nothing back, whether it applies, skips or fails, starts the bound over - // for every file that it read: such a file is no longer held, so its next rewrite is a new - // one. A file that the reload did not get to keeps its bound. - hadContent.forEach((_hadContent, path) => { + // for every file that it got to: such a file is no longer held, so its next rewrite is a new + // one. A file after the one that failed, which the reload did not get to, keeps its bound. + reached.forEach((path) => { const empty = this._emptyFiles.get(path); if (empty) { empty.firstHeld = undefined; @@ -498,8 +505,9 @@ export default class FileReloader { this._emptyFiles.clear(); } - // Called when a reload finds a file with content, or missing. The next time a reload finds it - // empty, its delay starts over. Its bound does not. + // Called when a reload finds a file with content, even content that fails to parse, or finds it + // missing. That ends the file's empty state: the next time a reload finds it empty, its delay + // starts over. Its bound does not. private _fileChanged(path: string): void { const empty = this._emptyFiles.get(path); if (empty) { @@ -519,10 +527,14 @@ export default class FileReloader { * empty, or files that are rewritten in turn so that every reload finds one of them empty, would * be held back without end, so a file is held back for at most `MAX_HOLD_RETRY_DELAYS` retry * delays. That bound is the file's own: it starts at the first reload that holds the file back, - * and starts over only at a reload that holds nothing back and reads the file, or when data is - * applied. A file whose content is not in effect, such as a placeholder that has always been empty, - * is parsed right away, and so is any file when retries are off, since nothing would read it - * again. + * and starts over only when data is applied, or at a reload that holds nothing back and gets to + * the file, whether the file reads, parses, fails or is missing. A file after one that fails to + * load is not read, so it keeps its bound. The bounds of files that keep being written while + * empty do not overlap: with n such files whose rewrites start 4.5 retry delays apart, the first + * application comes about 4.5 n + 0.5 retry delays after the first rewrite started, and a failure + * of another file that recurs exactly when each bound ends could hold them back without end. A + * file whose content is not in effect, such as a placeholder that has always been empty, is + * parsed right away, and so is any file when retries are off, since nothing would read it again. * * The clock is read after the file's own read, so that a slow read of another file does not * shorten this file's delay. From aae0e0db618bd498ea38b6092fa84bc668f4d486 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 07:27:59 +0000 Subject: [PATCH 11/12] fix: Read every file after a failure, and key skip-unchanged by path A review proved that a file configured after a failing file kept a bound left over from before the failure: every reload stopped at the failing file, so the later file was never read and its bound never started over. When a deploy fixed the failing file while rewriting the later one in place long after, the later file's truncation was applied empty. A reload now reads and classifies every configured file even after one fails to read or parse, and fails with the first failure after the loop, so a reload that holds nothing back starts every file's bound over. Skip-unchanged now compares each configured path, whether it is present, and its content. It compared the contents of the present files only, so the same content moved to a path with another extension, which is parsed in another format, was skipped instead of loaded. --- .../FileReloader.inPlaceRewrite.test.ts | 38 +++++++++++++--- .../filedata/FileReloader.test.ts | 43 ++++++++++++++++++- .../src/data_sources/filedata/FileReloader.ts | 37 ++++++++-------- 3 files changed, 91 insertions(+), 27 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index 3474f72876..863fe54546 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -691,14 +691,13 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a', 'b']); }); - it('keeps the bound of a file that a failing load did not get to', async () => { + it('applies a file kept written while empty after its bound while an earlier file fails', async () => { write(jsonPath, otherA); write(yamlPath, documentA); makeReloader([jsonPath, yamlPath]); await reloader.reloadNow(); - // The second file keeps being written while empty. The first file fails at every other load, - // before the second file is read. + // The second file keeps being written while empty. The first file fails at every other load. write(yamlPath, ''); await changeAndSettle(); await [1, 2, 3, 4, 5, 6, 7, 8].reduce>( @@ -724,6 +723,31 @@ describe('given a reloader over a file that is rewritten in place', () => { expect(values()).toEqual(['a,a', 'b']); }); + it('starts the bound of a file after a failing file over once a failing load reads it', async () => { + // The first file breaks while the second is written with content. Much later, the first file + // is fixed while the second is in the middle of a rewrite, which is held back with a bound of + // its own. + write(jsonPath, otherA); + write(yamlPath, documentA); + makeReloader([jsonPath, yamlPath]); + await reloader.reloadNow(); + + write(yamlPath, ''); + await changeAndSettle(); + expect(readAgainMessages()).toHaveLength(1); + write(jsonPath, '{"flagValues": '); + write(yamlPath, documentB); + await changeAndSettle(); + expect(failures).toHaveLength(1); + await advance(6 * retryDelayMs); + + write(jsonPath, otherB); + write(yamlPath, ''); + await changeAndSettle(); + expect(values()).toEqual(['a,a']); + expect(readAgainMessages()).toHaveLength(2); + }); + it('does not start bounds over at a load that holds a file back and then fails', async () => { const thirdPath = '/data/third.json'; write(yamlPath, documentA); @@ -944,7 +968,7 @@ describe('given a reloader over a file that is rewritten in place', () => { it('holds back a file whose content moved to it from another configured file', async () => { // The content in effect comes from the second file. Moving it to the first leaves the - // contents the same, so the load applies nothing, but the first file now contributes them. + // contents the same, and the load applies them, because the first file now contributes them. write(otherYamlPath, documentA); makeReloader([yamlPath, otherYamlPath]); await reloader.reloadNow(); @@ -953,15 +977,15 @@ describe('given a reloader over a file that is rewritten in place', () => { filesystem.remove(otherYamlPath); write(yamlPath, documentA); await changeAndSettle(); - expect(values()).toEqual(['a']); + expect(values()).toEqual(['a', 'a']); write(yamlPath, ''); await changeAndSettle(); - expect(values()).toEqual(['a']); + expect(values()).toEqual(['a', 'a']); write(yamlPath, documentB); await changeAndSettle(); - expect(values()).toEqual(['a', 'b']); + expect(values()).toEqual(['a', 'a', 'b']); }); it('never holds back an empty placeholder file, and it does not delay a change to another file', async () => { diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts index ace23c0667..f2b3b5e211 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts @@ -165,7 +165,7 @@ describe('given a reloader over a mock filesystem', () => { expect(failures[0].error.message).toEqual('duplicate flag flag1'); }); - it('stops reading after the first failing file', async () => { + it('reads every file even after one fails, and reports the first failure', async () => { filesystem.set(second, '{"flagValues": {"flag2": true}}'); const readFile = jest.spyOn(filesystem, 'readFile'); makeReloader((config) => { @@ -175,7 +175,46 @@ describe('given a reloader over a mock filesystem', () => { await reloader.reloadNow(); expect(failures).toHaveLength(1); - expect(readFile).toHaveBeenCalledTimes(1); + expect(failures[0].path).toEqual(path); + expect(readFile).toHaveBeenCalledTimes(2); + expect(applied).toHaveLength(0); + }); + + it.each([ + ['content that fails to parse', '{"flagValues"', 'parse'], + ['no content', '', 'parse'], + ['a missing file', undefined, 'read'], + ])('reports the first of two failing files, for %s', async (_case, data, kind) => { + if (data !== undefined) { + filesystem.set(path, data); + filesystem.set(second, data); + } + makeReloader((config) => { + config.paths = [path, second]; + }); + + await reloader.reloadNow(); + + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ kind, path }); + }); + + it('applies files that changed places with the same contents instead of skipping them', async () => { + filesystem.set(path, '{"flagValues": {"flag1": true}}'); + makeReloader((config) => { + config.paths = [path, second]; + config.policy = testPolicy({ missingFile: 'skip' }); + config.skipUnchanged = true; + }); + await reloader.reloadNow(); + expect(applied[0].files.map((file) => file.present)).toEqual([true, false]); + + filesystem.remove(path); + filesystem.set(second, '{"flagValues": {"flag1": true}}'); + reloader.trigger(); + await flushMicrotasks(); + expect(applied).toHaveLength(2); + expect(applied[1].files.map((file) => file.present)).toEqual([false, true]); }); it('merges multiple files in the configured order', async () => { diff --git a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts index 2bd1ad3176..f2690fa115 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/FileReloader.ts @@ -344,23 +344,25 @@ export default class FileReloader { const documents: FileDataDocument[] = []; const files: FileSummary[] = []; - const contents: string[] = []; + // Each configured path with its content, or null when it is missing, for skip-unchanged. The + // format is chosen by the path's extension, so the same content under another path can be + // other data. + const contents: [string, string | null][] = []; // Whether each path that this reload read, or found missing, has content. const hadContent = new Map(); + // The first failure. The other files are still read and classified, so that every file this + // reload gets to is reached, and the reload fails with this failure after the loop. let failure: LoadFailure | undefined; // The empty files that this reload holds back. The other files are still read, so that files // that are emptied together start their delays in the same reload. const heldBack: string[] = []; - // Every file that this reload got to and did not hold back, including a file that failed to - // read or parse and a missing file. + // Every file that this reload read or found missing and did not hold back, including a file + // that failed to read or parse. const reached: string[] = []; const readAll = paths.reduce( (previous, path) => previous.then(async () => { - if (failure) { - return; - } let data: string; try { data = await this._config.filesystem.readFile(path); @@ -373,10 +375,11 @@ export default class FileReloader { if (missing && policy.missingFile === 'skip') { logger?.debug(`File ${path} does not exist. It contributes no data.`); hadContent.set(path, false); + contents.push([path, null]); files.push({ path, present: false, flags: 0, segments: 0 }); return; } - failure = { kind: 'read', error: asError(err), path, repeated: false }; + failure = failure ?? { kind: 'read', error: asError(err), path, repeated: false }; return; } // A file with no content, or with content that its parser reads as no document, such as @@ -392,7 +395,7 @@ export default class FileReloader { // Content ends the file's empty state, even content that fails to parse. reached.push(path); this._fileChanged(path); - failure = { kind: 'parse', error: asError(err), path, repeated: false }; + failure = failure ?? { kind: 'parse', error: asError(err), path, repeated: false }; return; } } @@ -409,11 +412,11 @@ export default class FileReloader { try { document = policy.parseDocument(path, data); } catch (err) { - failure = { kind: 'parse', error: asError(err), path, repeated: false }; + failure = failure ?? { kind: 'parse', error: asError(err), path, repeated: false }; return; } } - contents.push(data); + contents.push([path, data]); documents.push(document as FileDataDocument); files.push({ path, present: true, flags: 0, segments: 0 }); }), @@ -423,8 +426,8 @@ export default class FileReloader { if (heldBack.length === 0) { // A reload that holds nothing back, whether it applies, skips or fails, starts the bound over - // for every file that it got to: such a file is no longer held, so its next rewrite is a new - // one. A file after the one that failed, which the reload did not get to, keeps its bound. + // for every file, since it reads them all: such a file is no longer held, so its next rewrite + // is a new one. A reload that holds a file back starts no bound over, even when it fails. reached.forEach((path) => { const empty = this._emptyFiles.get(path); if (empty) { @@ -474,10 +477,8 @@ export default class FileReloader { const recovering = this._lastFailureKey !== undefined; const content = JSON.stringify(contents); if (this._config.skipUnchanged && !recovering && content === this._lastGoodContent) { - // The contents are compared without their paths, so a file can have moved to another - // configured path, which now contributes the content in effect. + // Every file has the same content as in the data in effect, so it contributes what it did. this._lastFailureKey = undefined; - this._rememberAppliedContent(hadContent); return; } // Nothing is remembered until the consumer has accepted the result. A result the consumer @@ -527,9 +528,9 @@ export default class FileReloader { * empty, or files that are rewritten in turn so that every reload finds one of them empty, would * be held back without end, so a file is held back for at most `MAX_HOLD_RETRY_DELAYS` retry * delays. That bound is the file's own: it starts at the first reload that holds the file back, - * and starts over only when data is applied, or at a reload that holds nothing back and gets to - * the file, whether the file reads, parses, fails or is missing. A file after one that fails to - * load is not read, so it keeps its bound. The bounds of files that keep being written while + * and starts over only when data is applied, or at a reload that holds nothing back, whether the + * file reads, parses, fails or is missing. A reload reads every file even after one fails, so + * that it starts every file's bound over. The bounds of files that keep being written while * empty do not overlap: with n such files whose rewrites start 4.5 retry delays apart, the first * application comes about 4.5 n + 0.5 retry delays after the first rewrite started, and a failure * of another file that recurs exactly when each bound ends could hold them back without end. A From 092813a3a1aed1ffb4e8091cda1ae4382156701e Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 09:53:52 +0000 Subject: [PATCH 12/12] fix: Reject a JSON data file that holds no object A JSON file holding `null` was read as an override document with no entries, so it removed the file's overrides, and a JSON data file holding `null` failed with a TypeError from the merge, while one holding `42` or `[]` loaded as no flags. A JSON document must be an object: any other JSON value now fails to parse with "file data must be an object", and the last good data stays, for the override source and the file data sources alike. A YAML parser's null for a file with no document is still a document with no entries. A test also pins that a file named only by its YAML extension, such as `.yaml`, is parsed as YAML. --- .../FileReloader.inPlaceRewrite.test.ts | 14 +++++++++++ .../filedata/FileReloader.test.ts | 15 ++++++++++++ .../data_sources/filedata/document.test.ts | 24 +++++++++++++++++-- .../overrides/overrideDocument.test.ts | 9 +++++++ .../src/data_sources/filedata/document.ts | 19 +++++++++++---- .../src/overrides/overrideDocument.ts | 4 ++-- 6 files changed, 76 insertions(+), 9 deletions(-) diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts index 863fe54546..6a51dd6024 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.inPlaceRewrite.test.ts @@ -144,6 +144,20 @@ describe('given a reloader over a file that is rewritten in place', () => { }, ); + it.each(['null', '[]', '42'])( + 'reports a JSON file holding %s, which is not an object, and keeps the data', + async (data) => { + await startWith(jsonPath, documentA); + + write(jsonPath, data); + await changeAndSettle(); + expect(values()).toEqual(['a']); + expect(readAgainMessages()).toEqual([]); + expect(failures).toHaveLength(1); + expect(failures[0].error.message).toEqual('file data must be an object'); + }, + ); + it('applies a file that holds an empty document right away', async () => { await startWith(yamlPath, documentA); diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts index f2b3b5e211..05dc28bd40 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/FileReloader.test.ts @@ -199,6 +199,21 @@ describe('given a reloader over a mock filesystem', () => { expect(failures[0]).toMatchObject({ kind, path }); }); + it('reports a JSON file holding null as a parse failure, and keeps the data', async () => { + filesystem.set(path, '{"flagValues": {"flag1": true}}'); + makeReloader(); + await reloader.reloadNow(); + expect(applied).toHaveLength(1); + + filesystem.set(path, 'null'); + reloader.trigger(); + await flushMicrotasks(); + expect(applied).toHaveLength(1); + expect(failures).toHaveLength(1); + expect(failures[0]).toMatchObject({ kind: 'parse', path }); + expect(failures[0].error.message).toEqual('file data must be an object'); + }); + it('applies files that changed places with the same contents instead of skipping them', async () => { filesystem.set(path, '{"flagValues": {"flag1": true}}'); makeReloader((config) => { diff --git a/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts b/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts index e151315a72..1bd7ccfd66 100644 --- a/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts +++ b/packages/shared/sdk-server/__tests__/data_sources/filedata/document.test.ts @@ -74,7 +74,7 @@ describe('given the extension-based parser', () => { ); }); - it('returns the parsed value as it is, without validation', () => { + it('returns the parsed object as it is, without validation', () => { const parse = parseDocumentByExtension(); expect(parse('data.json', '{"flags": [], "flagValues": null, "segments": 0}')).toEqual({ flags: [], @@ -84,8 +84,25 @@ describe('given the extension-based parser', () => { expect(parse('data.json', '{"flags": {"flag-a": {"version": 1}}}')).toEqual({ flags: { 'flag-a': { version: 1 } }, }); - expect(parse('data.json', 'null')).toBeNull(); }); + + it.each(['null', '42', '[]', '"text"', 'true'])( + 'rejects JSON that is not an object, %s, for a file with or without the JSON extension', + (data) => { + const parse = parseDocumentByExtension(); + expect(() => parse('data.json', data)).toThrow('file data must be an object'); + expect(() => parse('data', data)).toThrow('file data must be an object'); + }, + ); + + it.each(['/data/.yaml', '/data/.yml', '.yaml'])( + 'parses a file named only by its YAML extension, %s, as YAML', + (path) => { + const yamlParser = jest.fn(() => null); + expect(parseDocumentByExtension(yamlParser)(path, '')).toBeNull(); + expect(yamlParser).toHaveBeenCalledWith(''); + }, + ); }); it('treats text with only white space or a byte order mark as no content', () => { @@ -101,6 +118,9 @@ it('recognizes YAML paths', () => { expect(isYamlPath('a.yaml')).toBe(true); expect(isYamlPath('a.json')).toBe(false); expect(isYamlPath('a.yaml.json')).toBe(false); + expect(isYamlPath('/data/.yaml')).toBe(true); + expect(isYamlPath('.yml')).toBe(true); + expect(isYamlPath('/data/a.yaml/data.json')).toBe(false); }); it('recognizes a file not found error by its code', () => { diff --git a/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts b/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts index 714d7a422e..aedc9a22d1 100644 --- a/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts +++ b/packages/shared/sdk-server/__tests__/overrides/overrideDocument.test.ts @@ -94,6 +94,15 @@ describe('given the override document parser', () => { expect(() => parseOverrideDocument()('data.json', '{"flagValues"')).toThrow(/json/i); }); + it.each(['null', '[]', '42'])( + 'rejects a JSON file holding %s, which is not an object', + (data) => { + expect(() => parseOverrideDocument()('data.json', data)).toThrow( + 'file data must be an object', + ); + }, + ); + it('rejects a document that is not an object', () => { expect(() => parseOverrideDocument(() => ['a'])('data.yaml', '- a\n')).toThrow( 'file data must be an object', diff --git a/packages/shared/sdk-server/src/data_sources/filedata/document.ts b/packages/shared/sdk-server/src/data_sources/filedata/document.ts index 27e905f0e1..e9ba9e1376 100644 --- a/packages/shared/sdk-server/src/data_sources/filedata/document.ts +++ b/packages/shared/sdk-server/src/data_sources/filedata/document.ts @@ -69,17 +69,26 @@ function parseJson(data: string): any { // rewritten in place can be empty for a moment, and the failure keeps the last good data. throw new SyntaxError('the file has no content, which is not a JSON document'); } - return JSON.parse(data); + const parsed = JSON.parse(data); + // A document is an object. JSON such as `null` or `[]` is not one, so it fails to load and the + // last good data stays. A YAML parser returns null for a file with no document, which is + // handled by the caller. + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) { + throw new Error('file data must be an object'); + } + return parsed; } /** * A parser that chooses the format by file extension only. A `.yml` or `.yaml` file uses the - * YAML parser, and every other file is JSON. The parsed value is returned as it is. This is the - * rule of the file data sources. + * YAML parser, and every other file is JSON. A JSON document must be an object, and is otherwise + * returned as it is, without validation, as is the YAML parser's result. This is the rule of the + * file data sources. * * Whether a file with no content is valid depends on its format. A JSON file with no content, - * or only white space, is not a JSON document and fails to parse. A YAML parser returns null or - * undefined for it, which is returned as it is. + * or only white space, is not a JSON document and fails to parse, and a JSON value that is not an + * object, such as `null`, fails too. A YAML parser returns null or undefined for a file with no + * document, which is returned as it is. * * @internal */ diff --git a/packages/shared/sdk-server/src/overrides/overrideDocument.ts b/packages/shared/sdk-server/src/overrides/overrideDocument.ts index e8ec2e0d03..3288e2c883 100644 --- a/packages/shared/sdk-server/src/overrides/overrideDocument.ts +++ b/packages/shared/sdk-server/src/overrides/overrideDocument.ts @@ -35,8 +35,8 @@ function validateItems(document: Record, member: 'flags' | 'segment function validateDocument(parsed: any): FileDataDocument { if (isNullish(parsed)) { // A YAML parser returns null or undefined for a file with no content, or with only a comment - // or a document marker, which is a document with no entries. A JSON file with no content fails - // to parse instead. + // or a document marker, which is a document with no entries. A JSON file with no content, or + // with `null`, fails to parse instead. return NO_ENTRIES; } if (!isPlainObject(parsed)) {