Repository navigation
feat: Move the file data sources onto the shared file data engine - #2049
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
c32d023 to
8c05f0b
Compare
8c05f0b to
79d2f0b
Compare
79d2f0b to
3b34072
Compare
kinyoklion
left a comment
There was a problem hiding this comment.
Review annotations written by Claude Code on behalf of @kinyoklion.
Map from the old code to the new, to help tell moved code from changed code:
FileLoader.tsis removed. Its read loop becameFileReloader, its per-filefs.watchbecameFileDirectoryWatcher(directory level), its 10 ms debounce became the reloader's settle window, and its timestamp de-duplication became content comparison.FileDataSource.tskeepsmakeFlagWithValueand the class shape. Its parsing, version, and duplicate rules moved intofileDataSourcePolicyunchanged._processFileDataand_processParsedDatabecame_applyDataplus the merge. Behavior changes are in the constructor (settle window, retry, directory watching),start, and_handleFailure, and are called out inline.fileDataInitilizerFDv2.tskeeps its class shape and status sequence. Its parsing and merge rules moved intofileDataInitializerPolicyunchanged. Behavior is still one load with no retry.- Everything under
filedata/is new code, ported from the Go SDK's internal file data package, exceptparseDocumentByExtension, which is the old parsing rule extracted. - Platform additions (
getFileStats, watch error reporting) are in #2062, which this PR is based on.
| * | ||
| * @internal | ||
| */ | ||
| export default class FileLoader { |
There was a problem hiding this comment.
Removed. Where each part went: the read loop in loadAndWatch is now FileReloader's load step; the per-file fs.watch is FileDirectoryWatcher, which watches the parent directory instead; the 10 ms debounce is FileReloader's settle window (100 ms, capped at 1 s); the timestamp check that skipped repeated events is replaced by comparing file contents (skipUnchanged). The watch callback's unhandled-rejection path has no equivalent: the reloader's load never rejects.
| DEFAULT_RETRY_DELAY_MS, | ||
| FileDataPolicy, | ||
| FileDirectoryWatcher, | ||
| FileReloader, |
There was a problem hiding this comment.
Changed: imports the shared engine (reloader, directory watcher, policy, extension parser) in place of FileLoader. makeFlagWithValue below is unchanged.
| * | ||
| * @internal | ||
| */ | ||
| export function fileDataSourcePolicy(yamlParser?: YamlParser): FileDataPolicy { |
There was a problem hiding this comment.
New: the policy object that carries this source's translation rules. The contents are moved, not changed: the version rule (start at 1, bump when the value changes) is the old _processParsedData logic, the duplicate error text and keying by the entry's own key property are the old _addItem behavior, and parseDocumentByExtension is the old parsing branch of _processFileData.
| private _logger?: LDLogger; | ||
|
|
||
| private _yamlParser?: (data: string) => any; | ||
| private _reloader: FileReloader; |
There was a problem hiding this comment.
Changed: the FileLoader and the _allData map are replaced by the reloader and an optional directory watcher. The last applied data lives inside the reloader and is only replaced on a successful load, which removes the partial-state problem the old _processFileData had when a parse failed part way.
| this._logger = options.logger; | ||
| this._yamlParser = options.yamlParser; | ||
| const autoUpdate = options.autoUpdate ?? false; | ||
| this._reloader = new FileReloader({ |
There was a problem hiding this comment.
Changed: reload behavior. With autoUpdate the settle window is 100 ms (was 10 ms) and a failed load is retried after 1 s (was never retried). Without autoUpdate both are 0, one load, as before. The watcher watches directories, so a rename-based write or a delete and recreate is detected (per-file watches went silent after a rename).
| @@ -0,0 +1,202 @@ | |||
| import { | |||
There was a problem hiding this comment.
New: unit tests for both parsers.
| @@ -0,0 +1,141 @@ | |||
| import { mergeDocuments } from '../../../src/data_sources/filedata'; | |||
There was a problem hiding this comment.
New: unit tests for the merge.
| @@ -0,0 +1,195 @@ | |||
| import { FileStats, Filesystem, WatchHandle } from '@launchdarkly/js-sdk-common'; | |||
There was a problem hiding this comment.
New test support: an in-memory Filesystem with watch, stats, and directory removal.
| @@ -0,0 +1,26 @@ | |||
| import { FileDataPolicy, parseDocumentByContent } from '../../../src/data_sources/filedata'; | |||
There was a problem hiding this comment.
New test support: a policy for engine tests.
| @@ -0,0 +1,84 @@ | |||
| import * as fs from 'fs'; | |||
There was a problem hiding this comment.
New: real-filesystem test through LDClientNode. Deletes and recreates the directory, then checks that a later edit is still detected.
484b8cf to
a42aa6d
Compare
89f51d4 to
58330ce
Compare
101aef9 to
8fe6a3b
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 101aef9. Configure here.
ba4df5d to
2a1bca4
Compare
6996c6d to
b014783
Compare
Adds FileReloader, FileDirectoryWatcher, mergeDocuments, parseDocumentByExtension, and the FileDataPolicy interface under packages/shared/sdk-server/src/data_sources/filedata, with their tests. Nothing uses the engine yet. The file data source and the FDv2 file data initializer move onto it in the following change.
A direct watch on a configured file that reported an error was dropped and set up again, but no reload ran. The file can have changed while its watch was failing, and when the path is a link to a file in another directory the directory watch does not see that change. An error from a direct file watch is now handled like a change event: the watch is renewed and the callback runs, as a directory watch already does when it is replaced.
When a watch could not be set up for every directory, the setup returned before the catch-up that reloads after a watch gap, so a directory whose watch was created in that pass got no reload while another directory still could not be watched, and the next retry skipped it because it was watched by then. A file written into the directory before its watch was in place was never loaded. A path in a directory that never exists is a valid configuration, so the catch-up now runs for the directories watched in each pass, and the retry continues for the rest.
eaefbbf to
60a16b3
Compare
b014783 to
2135437
Compare
The bound on how long a stream of change notifications can postpone a reload was measured with the wall clock, which a clock adjustment can move. It now uses monotonicNow from the common package, which reads a monotonic clock where the runtime has one.
2135437 to
91e82e7
Compare
…quests A direct watch on a configured file follows the path to an inode when it is set up. When a directory event named the file, or the file's metadata changed through another entry such as a link swap, the existing direct watch was kept, so after a retarget it stayed on the old target and edits to the new one were invisible to it and to the directory watch alike. The direct watch is now set up again in both cases. A request to notify that arrived while a metadata read was in flight was only applied to the read that ran afterwards, which compared against the metadata the first read had already recorded and so found no change. The request now applies to the read in flight as well, and during the first observation, which has nothing to compare to, it counts as a change.
Puts the file data source and the FDv2 file data initializer on the shared file data engine through a policy per source, removes FileLoader, and adds the tests for the robustness the file data source gains with automatic updates.
…ption The option's documentation covers directory watching, replacement, and the retry of a failed load, but not a directory that cannot be watched. It now says that such a directory is logged, watched once it can be, and that its files are still read on every load.
91e82e7 to
8dda675
Compare
## Summary This is the first of the preliminary changes for the flag overrides feature described by the OVERRIDE specification. It adds a shared file data engine to the server SDK common package, so that the existing file data source, the FDv2 file data initializer, and the file-based override source that a later change adds share one reading, reloading, and merging implementation. How a source turns document data into flags stays with that source, in an explicit policy that the engine takes as input. This change adds the engine and nothing else. No source uses it yet; the existing sources keep their current loader until #2049 moves them onto the engine. The split keeps each change reviewable on its own: this one is the engine and its tests, and #2049 is the two sources' policies and the behavior they gain. ### The shared engine `packages/shared/sdk-server/src/data_sources/filedata/` holds what every file-based source needs: - `FileReloader`: runs one load at a time; waits for a burst of change notifications to settle before reloading, and reloads no later than one second after the first notification of a burst that does not settle; retries a failed load after a short delay; keeps the last good data by not applying a failed load and leaves its internal state intact; skips applying content identical to the last applied content; and reports every read, parse, and merge failure through an `onFailure` callback as a typed `LoadFailure` (kind `read`, `parse`, or `merge`, the original error, the path, and whether the failure repeats an earlier one). Its load never rejects, so no filesystem error can become an unhandled rejection. - `FileDirectoryWatcher`: watches the directories that contain the files, so a file replaced by a rename, deleted and recreated, or created after start is detected. An event that names an entry which is not one of the configured files causes a reload only when the metadata of a configured file in that directory changed, read through any symbolic link, so a mounted ConfigMap or Secret update (a link swap that never names the file) is detected while a busy sibling file costs no reload; when the platform does not report which entry changed, every event in the directory counts. Each configured file is also watched directly, so a path that is a symbolic link to a file in another directory is followed; that watch is set up again after each event from it, after a directory event that names the file, and after a change to the file's metadata seen through another entry, so it survives the file being replaced and follows a link that is retargeted. A directory that cannot be watched is retried. A watched directory that is deleted has its watch closed and set up again once the directory exists; a watch that reports an error is replaced the same way. - `mergeDocuments`: combines the documents of the configured files in order and lets the policy resolve duplicate keys. - One document parser, `parseDocumentByExtension`: the format is chosen by file extension only, which is the rule the file data sources already used. ### The injected policy The engine takes a `FileDataPolicy`, one per source, with five members: `parseDocument(path, data)`, `makeFlagWithValue(key, value, previous)`, `entryKey(mapKey, entry)` (the key under which a `flags` or `segments` entry is stored and checked for duplicates), `resolveDuplicateKey(category, key)` (returns `keepFirst` or `keepLast`, or throws to fail the load), and `missingFile` (`fail` or `skip`). The policies of the file data source and the FDv2 initializer are defined in #2049, each in its source's file, so the differences between the sources are visible in one place per source. The override source supplies its own policy in a later change. ### Tests The engine's tests cover the parsers, the merge, the reloader, and the watcher, including a deleted and recreated directory, a flood of events for a deleted directory, a watch error, the maximum debounce delay, and the repeat key. `MockFilesystem` and `testPolicy` are test helpers that the tests of #2049 also use. The platform additions the engine relies on, `Filesystem.getFileStats` and watch error reporting (#2062) and the changed entry name in the watch callback (#2068), have merged. [SDK-3251](https://launchdarkly.atlassian.net/browse/SDK-3251) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Adds a new **shared file data engine** under `packages/shared/sdk-server/src/data_sources/filedata/` so file-based flag sources can share one read/reload/merge path. Nothing in the SDK wires it up yet; existing sources keep their loaders until a follow-up. > > **`FileReloader`** loads configured paths in order, runs one reload at a time, debounces change triggers (with a max delay cap), retries failures, optionally skips unchanged content, and reports typed `LoadFailure`s via `onFailure` while keeping the last good state when a load fails. > > **`FileDirectoryWatcher`** watches each file’s parent directory (deduped) plus a direct watch per path. It filters noisy directory events using file metadata (for ConfigMap-style link swaps), renews watches after errors/replaces, retries missing directories, and exposes `verify()` after failed loads. > > **`mergeDocuments`** merges parsed documents using an injected **`FileDataPolicy`** (parse, `flagValues` expansion, duplicate keys, missing-file behavior). **`parseDocumentByExtension`** picks JSON vs YAML by extension. > > The PR adds broad unit tests (`MockFilesystem`, `testPolicy`) for parsing, merge, reloader, and watcher edge cases (deleted dirs, watch floods, in-flight metadata checks, strict ENOENT on file watches). > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 1784d77. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> [SDK-3251]: https://launchdarkly.atlassian.net/browse/SDK-3251?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
…data tests (#2096) ## Summary Test-only cleanup of the file data engine tests that merged with #2082 and #2049. Several test files carried their own copies of a polling `waitFor` helper, a `sleep` helper, and an in-memory filesystem that duplicated the shared `MockFilesystem` the other file data tests already use. - One `waitFor` helper per package (`packages/shared/sdk-server/__tests__/waitFor.ts` and `packages/sdk/server-node/__tests__/waitFor.ts`). The server-node package does not depend on `@launchdarkly/js-sdk-common`, so its helper also exports `sleep`; the sdk-server tests use the `sleep` from js-sdk-common. - The two format detection tests use the shared `MockFilesystem` instead of an inline class. Neither test exercises the paths where the two mocks differed (missing files, timestamps, watches). - Every explicit timeout the tests passed is kept. No test changes what it checks. The open override stack (#2053) adds the same two helper files byte for byte, so the branches merge without conflict in either order. ## Tests sdk-server: 1347 passed, 7 skipped. server-node: 79 passed. Lint and format checks pass; the remaining lint warnings are pre-existing. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Test-only refactor** that deduplicates helpers across file data source tests in `sdk-server` and `server-node`. > > Several tests dropped local copies of a polling **`waitFor`** helper and a **`settle`** sleep wrapper. They now import **`waitFor`** from package-level test modules (`__tests__/waitFor.ts` in each package). **`sdk-server`** uses **`sleep`** from `@launchdarkly/js-sdk-common`; **`server-node`** uses **`sleep`** from `@launchdarkly/js-server-sdk-common` where delays are needed. > > The two **format detection** tests no longer define an inline **`MockFilesystem`**; they use the shared **`./filedata/MockFilesystem`** and **`filesystem.set()`** instead of assigning **`fileData`** directly. > > **No production code or test assertions change**—only imports and shared utilities. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 06caa69. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->

Summary
This change moves the existing file data source and the FDv2 file data initializer onto the shared file data engine added in #2082, so that they and the file-based override source that a later change adds share one reading, reloading, and merging implementation. How a source turns document data into flags stays with that source, in an explicit policy. The previous loader,
FileLoader, is removed.The injected translation
The engine takes a
FileDataPolicy, one per source; #2082 describes its members. Each policy is defined in its source's file, so the differences are visible in one place per source.fileDataSourcePolicyinFileDataSource.ts:flagValuesentry becomes a flag that is on and serves the value by fallthrough; its version starts at 1 and increments only when the value changes between loads, using the flag the last successful load produced for the same keykeyproperty, as the source has always stored it, so two entries with the samekeyproperty are duplicates whatever their map keysfound duplicate key: "<key>"fileDataInitializerPolicyinfileDataInitilizerFDv2.ts:flagValuesentry gets version 1; the initializer runs once and has no previous loadflagsandflagValuesin one fileThe later change that adds the override source supplies
fileOverrideSourcePolicy: extension-based parsing with validation of the document shape, aflagValuesentry expanded to an off flag, the configuredfailorignoreduplicate handling, and a missing file that contributes nothing.Robustness the existing file data source gains
The observable data semantics of the file data source and the FDv2 initializer are unchanged: the same documents produce the same flags, versions, and duplicate handling; the same errors reach the error handler; and the same messages are logged where they still apply. The unmodified baseline tests pass, and new tests pin the extension-based detection, the no-parser error text, and the documents the sources accepted before.
With automatic updates on, the file data source now behaves differently in these ways:
errorevent from the NodeFSWatcheris routed to the same replacement instead of surfacing as an uncaught exception.Error loading files: <error>; before it reached the error handler only. Processing failures keepError processing files: <error>. Only an automatic retry can be a repeat: a failure that repeats on retry is logged at debug level and not reported to the error handler again, while a failure after a change notification is always reported, even when it matches the previous one. Consecutive retry failures of the same kind on the same path count as repeats regardless of their message, so a file that is written slowly, and fails to parse at a different position on each retry, is reported once.Error processing files: <error>and retried, as the old_processFileDatacatch did. Nothing from that load is remembered as the last good result, and the reload promise never rejects, so a bad entry can neither raise an unhandled rejection at startup nor stop later reloads.Without automatic updates the source loads once, as before. The FDv2 initializer loads once, with no watching, debouncing, or retry.
Tests
The baseline test files pass with one implementation-detail change in
FileDataSource.test.ts: the tests that registered or fired a mockedFilesystem.watchper file path now do so on the files' directory, because the directory is what is watched. New tests cover each robustness change on the file data source, the two legacy policies, and the format-detection pins. The Node package gains a test over a real directory that is deleted and created again, and tests for theFSWatchererror routing and thegetFileStatsfailure mapping.SDK-3251
Note
Overview
Moves the file data source and FDv2 file data initializer onto the shared
filedataengine (FileReloader,FileDirectoryWatcher), removesFileLoader, and keeps per-source semantics in explicitFileDataPolicyimplementations (fileDataSourcePolicy,fileDataInitializerPolicy).For
FileDataSourcewithautoUpdate, watching and reloading change materially: parent directories are watched (plus direct watches on configured paths for symlinks), changes are debounced and retried on failure while last-good data and flag versions stay intact, deleted directories get watches torn down and re-established, and failures route through the error handler without unhandled rejections.FileDataInitializerFDv2uses the same loader path for a single startup read (no watch/retry).autoUpdatedocs inFileDataSourceOptionsdescribe the new behavior.Tests add policy/format/robustness coverage in
sdk-serverand integration-style Node tests (real dirs, symlinks, ConfigMap-style link swaps); existingFileDataSource.test.tsexpectations shift to directory-level watch callbacks.Reviewed by Cursor Bugbot for commit 8dda675. Bugbot is set up for automated code reviews on this repo. Configure here.