Repository navigation
fix: Keep file data and overrides through an in-place rewrite of their files - #394
Open
kinyoklion wants to merge 10 commits into
Open
kinyoklion wants to merge 10 commits into
kinyoklion wants to merge 10 commits into
Conversation
…r files The parser for a file is chosen by its extension, so whether an empty file is valid depends on the format. The JSON parser still treated a file with no content as a document with no entries, a rule from before formats were chosen by extension. An empty JSON file now fails to parse, like any other content that is not a JSON document, and the last good data stays. A registered parser that returns null for it, as YAML parsers do, still makes it a document with no entries. An application or editor that rewrites a file in place truncates it and then writes the new content, so a reload can read the file empty. With a YAML file, or with JSON before this change, that reload applied an empty document: the client briefly served no flags, and an override source briefly removed every override, until the next reload. A load that a change signal starts now keeps the current data when it finds a file empty, and the retry reads the file again after the retry delay; the change signal for the write starts a load sooner. The retry, and the initial load, parse what the file holds. Tests: - An empty JSON file fails to parse at the initial load and after a change; an empty YAML file is a document with no entries at the initial load, and after a change once the retry finds it still empty. - A file data source and an override source, rewritten in place with a pause after the truncation, never serve the empty file, for JSON and YAML and in both change detection modes of the override source. - The reloader tests rewrite their file in place with such a pause. Three of them failed with these writes before this change (ReportsIdenticalFailure- OnlyOnce, RetriesAfterFailureWithoutFurtherTriggers, RetryLeftOverAfterA- SuccessfulReloadNowDoesNotLoad); all pass after it.
Member
Author
|
bugbot review |
…n attempt The read again of an empty file was limited to loads that a change signal started, and counted as a failed attempt. A stream of change signals could hold the data back without end, an empty placeholder file delayed every change of the other files, and a retry left over from a failure could still apply a file that was being rewritten. The reloader now remembers, per file, whether its last read found content. A load that finds a file empty after it had content keeps the data and reads the file again after the retry delay; the next load parses what the file holds. This applies to every load, once per emptying, and uses up no attempt. The override source and AutoUpdate docs say how in-place rewrites and empty files are handled, and keep the advice to replace a file in one operation for YAML only.
Member
Author
|
bugbot review |
…y delay Reading an emptied file again once, on the next load, still applied an empty YAML file when a second load came during the same rewrite: two quick rewrites, a change to another configured file during the gap, or a poll or retry that read a second truncation. A file whose content is in effect and that a load finds empty is now held back by every load until it has stayed empty, without being written, for the retry delay. A change of its modification time starts the delay over, and a file that keeps being written while every load finds it empty is applied after five retry delays at most. Every file is read before any is held back, so files emptied together are applied together. A file whose content is not in effect, such as an empty placeholder, is parsed right away. The parser docs say that only a file parsed as JSON is invalid when empty, and a file data source test waits for the source to count as initialized after the retry's data, which it set just after.
Member
Author
|
bugbot review |
The elapsed time of a hold was computed from Stopwatch ticks in a way that overflowed after about 15 minutes on Linux and macOS, so a file found empty long ago could be held back for up to 15 minutes. The reloader now reads a monotonic clock that tests can replace, and Stopwatch.Elapsed converts without overflow. The bound of five retry delays applied to each file and was reset when that file was read with content, so files rewritten in turn were held back without end; it now applies to a run of loads that hold files back, which any other load ends. The allowance for a timer that fires early shortened the delay for every load; the delay check is now exact, and the retry that a hold arms comes when the delay is over plus a tenth of it. A file that a registered parser reads as nothing, such as YAML with only a comment, is held back like an empty one, and a file's state is read through symbolic links. The tests of the hold's timing run on a test clock.
Member
Author
|
bugbot review |
The time of a load was read once, before the files were read, so a slow read of one file made another file's emptiness look older, and its delay ended early. The time is now read after each file's own read. The bound of five retry delays applied to a run of holding loads, which any failed load restarted, so a file kept written while empty could be held without end, and one file reaching the bound could apply another file that was in the middle of an ordinary rewrite. Each file now has its own bound, from the first load that held it back. Another file's hold, a failed load or a read with content does not reset it; applying data, or the file staying empty for the whole delay, does. Tests pin the retry delay that a hold asks for, and the retry after a hold logs that it reads the files again because a file was found empty.
Member
Author
|
bugbot review |
A load skipped as unchanged left the records of held files in place, so a file that a tool rewrites now and then with the same content kept the bound of its first rewrite, and a rewrite five retry delays later was not held back. The skip now counts as an application for these records.
Member
Author
|
bugbot review |
… back A file's bound started over when the file had stayed empty for the whole delay, even while another file kept every load holding back. Files kept empty and truncated again in staggered turns therefore got fresh bounds without end, and nothing was applied while the writer ran. A bound also survived a long failure of another file, so a later rewrite of the file could be applied empty. A file's bound now starts over only at a load that holds nothing back and reads the file, whatever the load's outcome. Tests cover the staggered turns, a bound through a failure, and a retry measured from the end of the load and clamped at the margin.
Member
Author
|
bugbot review |
A file whose read or parse failed was not counted as read by the load, so its bound was not started over. An operator who saved a file in place with a mistake and saved the fix a few seconds later had the fix's truncation applied empty, once the bound from the first save was over. Every file that a load gets to and does not hold back, including the one that fails, now has its bound started over at a load that holds nothing back. A read that finds content, even content that does not parse, or finds the file missing, ends the file's empty state. Tests pin the fix of a file saved with a mistake, a file that a failing load did not reach, a load that holds a file back and then fails, and a file created empty again after it was missing.
Member
Author
|
bugbot review |
A load stopped at the first file that failed to read or parse, so a file configured after it was not read while the failure lasted and kept a bound from before. When a deploy fixed the failing file while rewriting the later one in place, the later file's truncation was applied empty. A load now reads and classifies every file even after a failure, and then fails with the first failure. Every file is reached, so a load that holds nothing back starts every bound over, and one that holds a file back starts none over. Tests cover a file after a failing file, for a parse failure and a read failure, and that the first failure is reported.
Member
Author
|
bugbot review |
kinyoklion
marked this pull request as ready for review
November 3, 2026 23:28
The public docs of the file data source and the override source described the hold in detail: its delay, its bound, and how other files wait for it. They now say what an application needs to know: files can be rewritten in place, how to remove a file's overrides, that an empty JSON file is not valid, and that replacing a YAML file in one operation avoids applying a partly written file.
Member
Author
|
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 0e6c6dd. Configure here.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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. On this branch, that reload applied an empty document, so the client briefly served no flags, and the override source briefly removed every override, until the next reload. A slow rewrite (truncate, pause 300 ms, write) made a client serve
a -> default -> b. The legacy file data source onmainservesa -> bfor the same rewrite, because it never accepted an empty file.The OVERRIDE spec's retry requirement exists to keep "a partially written or briefly invalid file from emptying the layer" (section 1.3). This change makes both file consumers of the shared engine,
FileDataSourceandFileOverrideSource, keep their data through such a rewrite.An empty JSON file is invalid
The parser for a file is chosen by its extension, as the OVERRIDE spec recommends (requirement 1.3.3, sdk-specs #272), so whether an empty file is valid is up to the format, like the rest of the content. The JSON parser still treated a file with no content, or white space only, as a document with no entries. That rule came from before formats were chosen by extension. An empty JSON file now fails to parse, with the message
the file has no content, which is not a JSON document, and the last good data stays, as for any other content that cannot be parsed. A registered parser that returns null for an empty file, as YAML parsers do, still makes it a document with no entries. This applies to a file with no extension and to any extension without a registered parser, since those are parsed as JSON.This changes behavior from 9.0.0-alpha.1, where an empty JSON file loaded as no flags or no overrides. To remove every override, write
{}or delete the file.I audited the rest of the shared file engine for other rules that look at content instead of the extension, and found none:
A file that is emptied is held back until it has stayed empty
With the JSON rule alone, a JSON file that is empty for a moment fails to parse and is retried. A YAML file can legitimately be empty, so its parser cannot tell an emptied file from one in the middle of a rewrite. The reloader now holds back a file that a load finds with no entries while its content is in effect, that is, while the data in effect came from a load that read content from that file. A file has no entries when it is empty or white space only, or when a registered parser reads it as nothing, such as YAML with only a comment or
---, which a read can see of a file that is being written. Whatever the format:FileDataSource's five load attempts.{}is a whole document and is applied right away.FileDataSourcewithout auto-update), nothing would read the file again, so an empty file is parsed right away. The initial load has no data in effect, so it parses an empty file right away too.A rewrite is covered when the writer leaves the file empty for less than the retry delay, including two rewrites in quick succession and a change to another file during the gap. A writer that leaves it empty for longer gets the empty file applied (YAML) or reported (JSON), as before. So does a program that regenerates the file in place over and over without a pause, so that every load finds it empty: after five retry delays its empty file is applied. Several files that are each kept written while empty, starting at different times, hold the data back for about five retry delays each, because each file's bound runs on its own. Holding the data back for as long as the file keeps changing would keep stale data without end, which the debounce also refuses to do.
The rule took eight rounds of adversarial review to get right; the Java and Ruby ports' reviews found the round-7 hole:
Stopwatch.Elapsed, which converts without overflow and which tests can replace. It read the clock once per load, so a slow read of one file shortened another's delay, and its shared bound was restarted by any failed load and could cut short another file's ordinary rewrite.Documentation
The public docs say what an application needs to know, without the details of the hold:
ParseronFileDataSourceBuilderandFileOverrideSourceBuilder: anullresult means the file has no entries.FileOverrides.Source: files can be rewritten in place, and the overrides stay in effect while a file is empty during a rewrite; to remove a file's overrides, delete it or write{}; an empty JSON file is not valid; replace a YAML file in one operation to avoid applying a partly written file.FileDataSourceBuilder.AutoUpdateno longer tells applications to replace files atomically whenever possible. It says files can be rewritten in place, and keeps the advice for YAML only (see below).Not changed: YAML cut off between two entries
A read that overlaps a write can see the first part of a YAML file. If the cut falls between two entries, the part is valid YAML with fewer entries, and it is applied until the next reload. JSON cut off anywhere fails to parse. This predates this change and is the same on
main. Telling a part from a whole file needs a content rule this change does not add, so the docs recommend replacing a YAML file in one operation.Not changed: the first load of a new source
A new reloader has no data in effect, so its first load parses an empty file right away. For
FileDataSourcethat is the start of the client, as before. Under FDv2, a file data source used as a synchronizer is created again when the data system starts, falls back or recovers, and its first load can then read a file that is being rewritten. This predates this change; the other SDKs' reviews found the same.Difference from the other SDKs
Go still detects the format from the content (
internal/filedata/document.go): a file that does not start with{is YAML, so an empty file of any name loads as no overrides. The OVERRIDE spec recommends choosing the format by extension but lets an SDK whose file data source already inspects the content keep doing so (requirement 1.3.3), and it does not say what an empty file means, so the SDKs can differ until it does.Testing
ReloadNow, with a retry delay of an hour on that clock, so their timing does not depend on the machine. They cover:---held back, and{}applied right away;InPlaceWrites.RewriteinTestUtils: truncate, pause, write):a -> band never the empty file, for JSON and YAML (300 ms pause).ReportsIdenticalFailureOnlyOnce,RetriesAfterFailureWithoutFurtherTriggers,RetryLeftOverAfterASuccessfulReloadNowDoesNotLoad).ReportsIdenticalFailureOnlyOncenow also checks that each report is the one it expects, not a report of the file being empty mid-write.RetryBudgetIsPerEpisodeNotPerPath(from feat: Move the file data source onto the shared file data engine #376) counted the reads of each path and expected an attempt that fails at the first path not to read the second; every attempt now reads both.ParseFailureFromPartialReadIsRetriedUntilContentIsComplete(from feat: Move the file data source onto the shared file data engine #376) asserted that the source was initialized right after it received the data, which the retry's thread sets just after; it failed once in a loop on a loaded machine and now waits for it.---cases)ReloadNowarming a whole delayLeaving out the update of what each file contributed when a load is skipped as unchanged changes nothing: the skip happens only when every file's presence and content equal those of the data in effect.
FileDataWatcherTest.DirectoryRemovedWithoutNotificationsIsReportedOnce, a watcher test that this change does not touch; that was a race in the watcher's periodic check, fixed in fix: Report changes to the files of directories that have no watch #396. The full server SDK suite passes on net8.0 (2031 tests), and the SDK builds for netstandard2.0, net462 and net8.0.Relation to #384
#384 fixes flakes in the reloader tests by reading their content through a scripted reader, so that no read sees a half-written file. That hides the case these tests need to handle. This change fixes the engine instead, and the reloader tests keep real, deliberately slow, in-place writes. #384 is no longer needed.
Opened from termag
Note
Overview
Fixes a gap where a reload that read a file during an in-place rewrite (truncate then write) could apply an empty document, briefly clearing flags or overrides until the next reload.
JSON parsing: Whitespace-only or zero-byte JSON no longer loads as an empty document; it throws and leaves the last good data in place. Registered parsers that return
nullfor empty YAML still mean “no entries.” Clearing overrides intentionally uses{}or deleting the file.FileDataReloaderhold-back: When a file that previously contributed content is read with no entries, the load keeps current data, schedules a variable retry (based on how long the file has stayed empty, capped at five retry delays), and does not consume failure attempt budget. Loads read every configured path even after an earlier path fails, reporting the first failure. Debounce timing uses an injectable monotonic clock for tests.Docs on
FileDataSource, file overrides, and YAMLParserdescribe in-place rewrites, empty-file semantics, and that atomic replace is still recommended for YAML partial reads.Tests add
InPlaceWrites.Rewrite, broad reloader hold-back coverage, and end-to-end checks that clients and override sources never serve an empty layer mid-rewrite (JSON/YAML, watch/poll).Reviewed by Cursor Bugbot for commit 0e6c6dd. Bugbot is set up for automated code reviews on this repo. Configure here.