Repository navigation
fix: Keep file data and overrides through an in-place rewrite of a file - #2154
kinyoklion wants to merge 15 commits into
Conversation
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.
|
bugbot review |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
…r 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.
|
bugbot review |
… 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.
|
bugbot review |
…y, 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.
|
bugbot review |
|
bugbot review |
…ach 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.
… its state looks the same
|
bugbot review |
|
bugbot review |
…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.
|
bugbot review |
…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.
|
bugbot review |
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.
|
bugbot review |
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.
|
bugbot review |
…the hold works The public docs of the file override source and the file data source described the hold's delay, its bound, other files waiting, and the retries. They now say what an application needs: files can be rewritten in place, how to remove a file's overrides, that an empty JSON file is not a valid document, and that replacing a YAML file in one operation avoids briefly applying a partly written file. The override source docs also say which extension means YAML and that files are read as UTF-8. The details stay in the internal comments.
|
bugbot review |
…cation needs The autoUpdate docs described how the watches are set up, the retry interval and the retries. They now say that the data source watches the files, picks up a replaced file and a Kubernetes ConfigMap or Secret update, still notices a file or directory that cannot be watched yet, and keeps its flags through an in-place rewrite. The override source docs no longer mention retries. The fs.watch note moves to the watcher's internal docs.
|
bugbot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 78bcfd1. Configure here.
| // 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 { |
There was a problem hiding this comment.
nit: function name is misleading
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in 6058665: it is now _endEmptyState, which is what it does. It ends the file's empty state, so the delay starts over the next time a reload finds the file empty.
| * 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): Promise<boolean> { |
There was a problem hiding this comment.
nit: the function naming here is also weird... should it be something like _should HoldBack
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in 6058665: it is now _shouldHoldBack.
The function that ends a file's empty state is now _endEmptyState, and the one that decides whether a reload holds a file back is now _shouldHoldBack.
Summary
Applications and editors rewrite data files in place: they truncate the file and then write the new content. A reload that reads the file in between finds it empty. The file override source applied an emptied YAML file as a document with no entries, so it removed every override from that file until the next reload. With a rewrite that pauses 300 ms after truncating, a client served
a -> default -> b: on every run in watching mode, and whenever a poll fell inside the pause in polling mode. An emptied JSON file failed to parse and kept the overrides, but each rewrite that a reload caught was logged as an error (Unable to load flags: Unexpected end of JSON input). The file data source never served an empty file, but it reported each such rewrite as an error through its error handler and the log. It did the same before it moved onto the shared file data engine.The OVERRIDE spec's retry requirement exists to keep "a partially written or briefly invalid file from emptying the layer" (Requirement 1.3.10). This change makes both consumers of the shared file data engine that reload,
FileDataSourceand the file override source, keep their data through such a rewrite without reporting it.An emptied file is held back until it has stayed empty
A file has no entries when its content is white space only (a byte order mark counts as white space), or when its parser reads it as no document: a YAML parser returns
nullfor a file with only a comment or a---marker, which a read can see of a YAML file that is being written.{}is a document, and is applied right away.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: failure first, then the hold, then the merge.
FileReloadernow remembers, for each configured file, whether it contributed content to the data in effect, that is, to the last reload that was applied or skipped as unchanged; a missing file, or one with no entries, contributed none. For a file whose content is in effect and that reloads have found with no entries, it remembers since when the file has been so without being written, the file's state then (its modification time and size as the platform reports them, which the Node platform reads through any symbolic link, with the poller's file state helper), and when reloads began holding the file back. The clock is read after each file's own read, so that a slow read of another file does not shorten its delay.A reload that finds such a file with no entries keeps the current data, logs at debug level (
File ... is empty, possibly while it is being rewritten; reading it again before applying it), reports no failure, and arms a retry, whatever the format. The file is held back until it has stayed so, without being written, for exactly the retry delay (1 second):FileDataSourcewithoutautoUpdate, and the FDv2 file initializer, which both load once), nothing would read the file again, so an empty file is parsed right away.A rewrite is covered when the time between truncating and writing is shorter than the retry delay. A writer that pauses longer gets an empty YAML override file applied, or an empty JSON file, or any empty
FileDataSourcefile, reported as a load failure while the data in effect stays. A rewrite within the resolution of the file's modification time that keeps its size is not seen as a write, so its delay does not start over.The commits show how the rule got here. The first read an emptied file again once, keyed on whether the last read found content; that still applied an empty file when no reload read the content between two truncations. The second and third hold a file back by its state, with a cap per file and a slack of a tenth in the delay check. The fourth applies the review findings: the cap per file let files rewritten in turn be held back without end, the slack let a reload apply a file that had been empty for less than the delay, a YAML read with only a comment was applied as no entries, and 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 fifth adds tests for a filesystem whose modification times are coarse. The sixth applies a second review: the clock read once per reload let a slow read of an earlier file shorten a later file's delay, a failed reload restarted the shared bound so that a file kept written was held without end while another file kept failing, and the shared bound let one file reaching it apply another file a fraction of a delay into an ordinary rewrite; it reads the clock after each file's read and bounds each file on its own. The seventh and eighth add and name tests. The ninth applies a third review: a file that stayed empty for the whole delay cleared its own bound while another file kept every reload holding, so files emptied in staggered turns were held without end, and a file held back while another file failed kept its bound, so a fix of the other file long after, during a rewrite of this one, applied it empty; a bound now starts over only at a reload that holds nothing back. The tenth applies a fourth review: a file whose read or parse failed right after a hold kept its old bound, so an operator who saved a YAML typo in place and then the fix in place seconds later got the fix's truncation applied empty; a reload now starts the bound over for every file it got to, including the one that failed, and content that fails to parse ends a file's empty state. The eleventh applies a fifth review: a file configured after a failing file kept a bound left over from before the failure, because every reload stopped at the failure, so a deploy that fixed the failing file while rewriting the later one in place applied its truncation empty; a reload now reads every file even after a failure. It also keys skip-unchanged by path, below. The twelfth rejects a JSON document that is not an object, below, and the thirteenth and fourteenth reduce the public docs to what an application needs. It is the rule of dotnet-core #394.
Skip-unchanged is keyed by path
A reload skipped
applywhen the contents of the present files were the same as those of the data in effect, without their paths. Content that moved to another configured path was then not applied: the summary of what each file supplied stayed stale, and since the format is chosen by the path's extension, a YAML parser that reads some content differently from JSON would have kept the old data for the same content moved betweenx.yamlandx.json. (Content that the other format cannot parse fails to parse before the comparison, so it was reported.) The comparison now covers each configured path, whether it is present, and its content.An empty JSON file says what is wrong
The parser for a file is chosen by its extension, so whether an empty file is valid is up to the format. An empty or white-space JSON file was already invalid:
JSON.parserejects it and the last good data stays. It now fails withthe file has no content, which is not a JSON document(still aSyntaxError) instead ofUnexpected end of JSON input. This applies to a file with no extension and to every extension other than.ymland.yaml, since those files are parsed as JSON. A YAML parser'snullorundefinedfor an empty file is still a document with no entries in the override source.A JSON document must be an object
A JSON override file holding
nullwas read as a document with no entries, so it removed the file's overrides. A JSON data file holdingnullfailed with aTypeErrorfrom the merge, and one holding42,"text"or[]loaded as no flags. Any JSON value that is not an object now fails to parse withfile data must be an object, and the last good data stays, for the override source,FileDataSourceand the FDv2 file initializer. For a JSON data file holding a value that is not an object, this changes released behavior from loading no flags to a load failure; such a file is not a document in the file data format. A YAML parser'snullfor a file with no document is still a document with no entries in the override source.A file named only by its YAML extension, such as
.yaml, was already parsed as YAML, as in .NET and Go; a test now pins it.Documentation
The public docs say what an application needs; how the hold, the retries and the watches work stays in the internal comments and in this description.
FileOverrideSourceOptionssays that a file whose name ends in.ymlor.yamlis YAML and any other file is JSON, read as UTF-8; that if a file that exists cannot be read or parsed, the previously loaded overrides stay in effect, the failure is logged, and the source loads the file once it is valid again; that files can be rewritten in place, and that if the source finds a file empty while it is being rewritten, it keeps the overrides in effect until the new content is written; how to remove a file's overrides (delete the file or write{}); that an empty JSON file is not a valid document; and that replacing a YAML file in one operation avoids briefly applying a partly written file. ItsyamlParsersays that anullorundefinedresult, which YAML parsers return for an empty file, means the file has no entries.FileDataSourceOptions.autoUpdatesays that the data source watches the files for changes; that a file replaced by a rename, or deleted and created again, is picked up, and so is an update of a mounted Kubernetes ConfigMap or Secret; that a file or directory that cannot be watched yet is still noticed once it can be watched; that if it reads a file while it is being written, or a file cannot be parsed, it keeps the flags it has and reads the file again shortly afterward; and that replacing a YAML file in one operation avoids briefly loading a partly written file. The description offs.watch, the direct file watches, the retry interval and the retries is gone; thefs.watchnote moved to the internal docs ofFileDirectoryWatcher, which already describe the rest.Not changed
FileDataSourceand the FDv2 file initializer. Their policy hands a YAML parser'snullto the merge, which fails withCannot read properties of null (reading 'flags'). An empty YAML data file therefore fails to load and the last good data stays, as before the shared engine. With this change, an emptied file, or one with only a comment, is held back before that failure is reported. Making it a document with no entries, as it is for override files, changes released behavior, so it is left for a separate change.flags.YAMLis parsed as JSON, as it always was.Testing
null, a number, a string,trueand[]are rejected as not an object, by the extension-based parser and the override document parser. A file named.yamlor.ymlis YAML. A YAML parser'snullorundefinedis returned as it is, and the override document parser makes it a document with no entries.FileReloader.test.ts): a JSON data file holdingnullis reported as a parse failure and the data stays; a reload reads every file even after one fails and reports the first failure, for content that fails to parse, no content, and a missing file; files that changed places with the same contents are applied, not skipped.FileReloader.inPlaceRewrite.test.ts, mock filesystem with a modification time on every write, Jest's fake clock):---marker, or both is held back; a file with{}is applied right away.null,[]or42is reported, not held back, and the data stays.server-node, with real in-place rewrites (truncate, pause, write;rewriteInPlacein__tests__/inPlaceWrites.ts) while the test samples evaluations:FileDataSourceandautoUpdateservesa -> band reports no error, for JSON and YAML, with a 300 ms pause; and never serves the default and reports no error through two rewrites 20 ms apart.a -> band logs no load failure, for JSON and YAML, in watching mode (300 ms pause) and in polling mode at the default 1 second interval. In polling mode the rewrite starts 750 ms after a poll and pauses 600 ms, so the next poll finds the file empty.main, all 12 end-to-end tests fail: the YAML override file servesdefaultin between, and the JSON override file and both data files log an error. 59 of the new unit and parser tests fail too. On the first commit of this branch, the 6 end-to-end tests with two rewrites in a row fail, and 38 unit tests; on the third commit, 25; on the fifth (the run cap), 10; on the eighth (the per-file bound), 10; on the ninth (the bound started over only at a reload that holds nothing back), 8; on the tenth (reached files), exactly the 4 tests of the fifth review: a file after a failing file, reading every file after a failure, and the two tests of moved content.{}counted as no document; the override parser returning{}for no document; stop reading after a held-back file; a hold that hides another file's failure; holding back counted as a failure; no retry armed; no carry-over of the reason, a carry-over to any reload, or always the reason of a change; a file state lookup that throws; no check for close; either debug line removed; the defaultJSON.parsemessage for an empty JSON file; white space counted as content. For the twelfth commit, 10 mutants of the parsers (any JSON value accepted, ornull, an array or a scalar accepted; a file named.yamlnot counted as YAML; and the parser mutants above) all fail at least one test. Two survive and are equivalent: dropping the explicitretryDelayMs <= 0check from the hold (with no retry delay the delay is zero long), and not clearing the files' entries when data is applied (a reload that applies holds nothing back, so it already starts every file's bound over, and the files it read with content already start their delays over).FileReloader.test.ts) passed 15 runs in a row, and the twoserver-nodetest files with the end-to-end tests (20 tests) passed 12 runs in a row, on a loaded machine. The polling end-to-end tests take 3 to 4 seconds, so the override end-to-end tests have a 15 second timeout.@launchdarkly/js-server-sdk-commonand@launchdarkly/node-server-sdk: full test suites,lint,format:check,buildandbuild:docpass locally.Opened from termag
SDK-3419
Note
Overview
File data and file-based overrides now survive in-place rewrites (truncate-then-write). When a reload reads a file that is briefly empty or YAML-with-no-document while its previous content is still in effect,
FileReloaderholds back the change, keeps the last good flags/overrides, schedules a retry (with per-file delay and a five-retry cap), and no longer treats that as a load failure.Reload behavior is tightened: all configured paths are read even after one fails (first failure still wins); skip-unchanged compares content per path so moved files are not silently ignored.
Parsing is stricter and clearer: empty/whitespace JSON fails with an explicit message; JSON must be an object (
null/[]/scalars rejected); YAMLnull/undefinedmaps to aNO_ENTRIESsentinel for overrides. Public options/docs describe rewrite tolerance and YAML-vs-JSON rules.Coverage adds shared
rewriteInPlace/sampleValueshelpers, a largeFileReloader.inPlaceRewritesuite, and Node SDK e2e tests forFileDataSourceand file overrides (watching and polling).Reviewed by Cursor Bugbot for commit 6058665. Bugbot is set up for automated code reviews on this repo. Configure here.