From ddc4f2447c1d53cbf6fc1bfc0680d40ff56e461a Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:30:52 +0000 Subject: [PATCH 01/10] fix: Keep flags and overrides while a data file is rewritten in place An application or editor that rewrites a data file in place truncates it and then writes the new content. A reload in between read the file empty, and an empty file is a YAML document with no entries, so the override source briefly removed every override and the file data sources briefly served no flags. The shared reloader now remembers whether each file's last read found content. A load that finds a file empty after it had content keeps the current data and reads the file again after the retry delay, without reporting a failure. The next load parses whatever the file holds, so the data is held back at most once per emptying, and a file that is empty from the start is never read again. With retries off, an empty file is parsed right away. --- internal/filedata/reloader.go | 53 ++++-- internal/filedata/reloader_test.go | 224 ++++++++++++++++++++++++- internal/sharedtest/files.go | 24 +++ ldclient_file_in_place_rewrite_test.go | 154 +++++++++++++++++ ldfiledata/package_info.go | 9 + ldfiledatav2/package_info.go | 9 + ldoverrides/file_source_builder.go | 12 ++ ldoverrides/file_source_test.go | 38 +++++ 8 files changed, 509 insertions(+), 14 deletions(-) create mode 100644 internal/sharedtest/files.go create mode 100644 ldclient_file_in_place_rewrite_test.go diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index 86d4f935..728f1727 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -56,8 +56,9 @@ type ReloaderConfig struct { DebounceDelay time.Duration // RetryDelay is how long to wait after a failed reload before automatically retrying, // so that a failure observed while a file was being rewritten recovers even if no - // further change notification arrives. If zero or negative, there is no automatic - // retry. + // further change notification arrives. A file that had content and is found empty is + // read again after the same delay before it is applied. If zero or negative, there is + // no automatic retry, and an empty file is parsed as it is. RetryDelay time.Duration // SkipUnchanged, if true, suppresses the Apply call when the files' raw contents are // byte-identical to the last successfully applied contents. @@ -66,7 +67,8 @@ type ReloaderConfig struct { // Reloader owns the reload cycle for a set of data files: it serializes reloads, debounces // change signals, retains the last good result on failure (by not calling Apply), retries -// after failures, and skips no-op applications. +// after failures, reads a file that was emptied again before applying it, and skips no-op +// applications. type Reloader struct { cfg ReloaderConfig triggerCh chan struct{} @@ -81,6 +83,9 @@ type Reloader struct { reloadMu sync.Mutex lastGoodHash []byte lastErrorMsg string + // lastReadHadContent records, for each configured path, whether the last read of the + // file found content. A file that did not exist had no content. Guarded by reloadMu. + lastReadHadContent map[string]bool } // NewReloader creates a Reloader. The caller should perform the initial load with @@ -92,10 +97,11 @@ type Reloader struct { // alone must not leak a goroutine. func NewReloader(cfg ReloaderConfig) *Reloader { return &Reloader{ - cfg: cfg, - triggerCh: make(chan struct{}, 1), - retryRequestCh: make(chan struct{}, 1), - closeCh: make(chan struct{}), + cfg: cfg, + triggerCh: make(chan struct{}, 1), + retryRequestCh: make(chan struct{}, 1), + closeCh: make(chan struct{}), + lastReadHadContent: make(map[string]bool, len(cfg.Paths)), } } @@ -162,7 +168,7 @@ func (r *Reloader) run() { reloadAndMaybeRetry := func(isRetry bool) { if isRetry { - r.cfg.Loggers.Debug("Retrying flag data load after earlier failure") + r.cfg.Loggers.Debug("Retrying flag data load after an earlier failure or an emptied file") } else { r.cfg.Loggers.Info("Reloading flag data after detecting a change") } @@ -213,11 +219,11 @@ func (r *Reloader) run() { } } -// reload performs one full load of all configured files. It reports whether the load -// succeeded, which is what decides whether a retry gets armed -- so a skipped no-op -// application counts as success. The whole set is re-read on every reload: entries are -// combined across files in order, so a change to one file can alter which file wins for -// a key. +// reload performs one full load of all configured files. It reports whether the load is +// done, which is what decides whether a retry gets armed: it is false when the load failed +// or a file is to be read again, and a skipped no-op application counts as done. The whole +// set is re-read on every reload: entries are combined across files in order, so a change +// to one file can alter which file wins for a key. func (r *Reloader) reload() bool { r.reloadMu.Lock() defer r.reloadMu.Unlock() @@ -238,11 +244,18 @@ func (r *Reloader) reload() bool { if err != nil { if r.cfg.SkipMissingPaths && errors.Is(err, fs.ErrNotExist) { r.cfg.Loggers.Debugf("File %s does not exist; it contributes no data", path) + r.lastReadHadContent[path] = false files = append(files, FileSummary{Path: path}) continue } return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) } + hasContent := len(bytes.TrimSpace(rawData)) > 0 + hadContent := r.lastReadHadContent[path] + r.lastReadHadContent[path] = hasContent + if !hasContent && hadContent && r.cfg.RetryDelay > 0 { + return r.readAgainLater(path) + } _, _ = hasher.Write(rawData) _, _ = hasher.Write([]byte{0}) doc, err := parseDocument(rawData) @@ -293,6 +306,20 @@ func (r *Reloader) reload() bool { return true } +// readAgainLater handles a file that had content when it was last read and is empty now. It +// may be in the middle of an in-place rewrite: a writer that truncates the file and then +// writes the new content. The load 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 new load sooner. That next load parses what the file holds then, +// because the file was empty when it was last read: a file that is still empty is a YAML +// document with no entries. So the current data is kept for at most one more load, and a +// file that is empty from the start, such as a placeholder, is never read again. +func (r *Reloader) readAgainLater(path string) bool { + r.cfg.Loggers.Debugf("File %s is empty, possibly while it is being rewritten; reading it again before applying it", + path) + return false +} + func (r *Reloader) fail(err error) bool { // Close may have happened while the files were being read; deliver nothing in that // case, and report success so no retry is armed. diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 28a8344f..21d2c521 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "testing" "time" @@ -24,9 +25,19 @@ type reloaderFixture struct { } func newReloaderFixture(t *testing.T, initialContent string, configure func(*ReloaderConfig)) *reloaderFixture { + t.Helper() + return newReloaderFixtureAt(t, "data.json", initialContent, configure) +} + +func newReloaderFixtureAt( + t *testing.T, + fileName string, + initialContent string, + configure func(*ReloaderConfig), +) *reloaderFixture { t.Helper() f := &reloaderFixture{ - path: filepath.Join(t.TempDir(), "data.json"), + path: filepath.Join(t.TempDir(), fileName), applied: make(chan MergeResult, 100), errored: make(chan error, 100), } @@ -63,6 +74,30 @@ func (f *reloaderFixture) requireApplied(t *testing.T) MergeResult { } } +func (f *reloaderFixture) requireAppliedWithin(t *testing.T, duration time.Duration) MergeResult { + t.Helper() + select { + case result := <-f.applied: + return result + case <-time.After(duration): + require.FailNow(t, "no Apply within "+duration.String()) + return MergeResult{} + } +} + +// requireAppliedAlready returns the result of an Apply that has already happened, such as one +// made by a ReloadNow that has returned. +func (f *reloaderFixture) requireAppliedAlready(t *testing.T) MergeResult { + t.Helper() + select { + case result := <-f.applied: + return result + default: + require.FailNow(t, "the load did not call Apply") + return MergeResult{} + } +} + func (f *reloaderFixture) requireErrored(t *testing.T) error { t.Helper() select { @@ -424,3 +459,190 @@ func TestReloaderDebounceWindowIsExtendedByEachTrigger(t *testing.T) { f.requireApplied(t) f.requireQuiet(t, 2*window) } + +// The retry delay for the tests of a file that is found empty. The retry comes long enough +// after the change signal for a test to finish rewriting the file before it, even on a slow +// machine. +const emptyFileRetryDelay = 1500 * time.Millisecond + +// Shorter than emptyFileRetryDelay, so a load that happens within it after a change signal +// is not the retry. +const beforeTheRetry = time.Second + +const readAgainLog = "reading it again" + +const ( + yamlFlag1 = "flagValues:\n flag1: true\n" + yamlFlag2 = "flagValues:\n flag2: true\n" +) + +// newEmptyFileFixture configures a reloader for a YAML file that defines flag1, with the +// retry delay of the tests of a file that is found empty, and a log that keeps debug output. +func newEmptyFileFixture(t *testing.T, configure func(*ReloaderConfig)) (*reloaderFixture, *ldlogtest.MockLog) { + t.Helper() + mockLog := ldlogtest.NewMockLog() + mockLog.Loggers.SetMinLevel(ldlog.Debug) + f := newReloaderFixtureAt(t, "data.yaml", yamlFlag1, func(cfg *ReloaderConfig) { + cfg.Loggers = mockLog.Loggers + cfg.RetryDelay = emptyFileRetryDelay + if configure != nil { + configure(cfg) + } + }) + return f, mockLog +} + +func flagKeys(result MergeResult) []string { + keys := make([]string, 0, len(result.Flags)) + for _, flag := range result.Flags { + keys = append(keys, flag.Key) + } + slices.Sort(keys) + return keys +} + +func requireLogged(t *testing.T, mockLog *ldlogtest.MockLog, level ldlog.LogLevel, pattern string) { + t.Helper() + require.Eventually(t, func() bool { return mockLog.HasMessageMatch(level, pattern) }, testTimeout, + 10*time.Millisecond, "no %s log matching %q", level, pattern) +} + +func TestReloaderReadsAFileThatIsEmptiedAgainBeforeApplyingIt(t *testing.T) { + // A writer that rewrites the file in place truncates it and then writes the new content. + // A load that finds the file empty after it had content keeps the data. An empty file is + // a YAML document with no entries, so applying it would briefly remove every entry. + for name, emptied := range map[string]string{"empty": "", "white space only": " \n\r\n "} { + t.Run(name, func(t *testing.T) { + f, mockLog := newEmptyFileFixture(t, nil) + f.reloader.ReloadNow() + assert.Equal(t, []string{"flag1"}, flagKeys(f.requireApplied(t))) + + f.write(t, emptied) + f.reloader.Trigger() + requireLogged(t, mockLog, ldlog.Debug, readAgainLog) + f.requireQuiet(t, 100*time.Millisecond) + + // No change signal: the retry reads the rewritten file. + f.write(t, yamlFlag2) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + f.requireQuiet(t, 100*time.Millisecond) + }) + } +} + +func TestReloaderAppliesAnEmptiedFileThatIsStillEmptyWhenReadAgain(t *testing.T) { + f, _ := newEmptyFileFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, "") + f.reloader.Trigger() + f.requireQuiet(t, 200*time.Millisecond) + + // The file is still empty when the retry reads it, so it is a document with no entries. + assert.Empty(t, f.requireApplied(t).Flags) +} + +func TestReloaderReadsAnEmptiedFileAgainOnlyOnce(t *testing.T) { + f, mockLog := newEmptyFileFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, "") + f.reloader.Trigger() + requireLogged(t, mockLog, ldlog.Debug, readAgainLog) + f.requireQuiet(t, 100*time.Millisecond) + + // A further change signal does not postpone the empty file again: it was empty when it + // was last read, so this load applies it, before the retry would have. + f.reloader.Trigger() + assert.Empty(t, f.requireAppliedWithin(t, beforeTheRetry).Flags) +} + +func TestReloaderDoesNotReadAgainAFileThatIsEmptyFromTheStart(t *testing.T) { + // An empty file next to the data, such as a placeholder for later entries, has never had + // content. It does not hold back the changes of the other file. + placeholder := filepath.Join(t.TempDir(), "placeholder.yaml") + require.NoError(t, os.WriteFile(placeholder, nil, 0600)) + f, mockLog := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { + cfg.Paths = append(cfg.Paths, placeholder) + }) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, yamlFlag2) + f.reloader.Trigger() + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireAppliedWithin(t, beforeTheRetry))) + assert.False(t, mockLog.HasMessageMatch(ldlog.Debug, readAgainLog)) +} + +func TestReloaderAppliesAFileThatIsEmptyAtTheInitialLoad(t *testing.T) { + f, _ := newEmptyFileFixture(t, nil) + f.write(t, "") + + // The initial load applies what it reads, without waiting for a retry. + f.reloader.ReloadNow() + assert.Empty(t, f.requireAppliedAlready(t).Flags) +} + +func TestReloaderRetryReadsAnEmptiedFileAgain(t *testing.T) { + // A retry left over from a failure can run while the file is being rewritten in place. It + // reads the file again too, rather than applying it empty. + f, mockLog := newEmptyFileFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + f.write(t, "flagValues: [") + f.reloader.Trigger() + f.requireErrored(t) + + // No change signal: only the retry reads the file. + f.write(t, "") + requireLogged(t, mockLog, ldlog.Debug, readAgainLog) + f.write(t, yamlFlag2) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) +} + +func TestReloaderDoesNotReadAgainAFileThatWasMissing(t *testing.T) { + f, _ := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { cfg.SkipMissingPaths = true }) + f.reloader.ReloadNow() + f.requireApplied(t) + require.NoError(t, os.Remove(f.path)) + f.reloader.Trigger() + assert.Empty(t, f.requireApplied(t).Flags) + + // A missing file had no content, so the empty file that replaces it is applied right away. + require.NoError(t, os.WriteFile(f.path, nil, 0600)) + f.reloader.Trigger() + assert.Empty(t, f.requireAppliedWithin(t, beforeTheRetry).Flags) +} + +func TestReloaderParsesAnEmptiedFileRightAwayWithoutRetries(t *testing.T) { + f, _ := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { cfg.RetryDelay = 0 }) + f.reloader.ReloadNow() + f.requireApplied(t) + + // Without a retry, nothing would read the file again, so the load applies it as it is. + f.write(t, "") + f.reloader.ReloadNow() + assert.Empty(t, f.requireAppliedAlready(t).Flags) +} + +func TestReloaderReadingAnEmptiedFileAgainIsNotAFailure(t *testing.T) { + // Reading a file again reports nothing. A consumer that tracks failures through OnError + // hears of none, and a failure that was reported once is not reported again when the + // same content comes back after the file was read again. + f, mockLog := newEmptyFileFixture(t, nil) + f.write(t, "flagValues: [") + f.reloader.ReloadNow() + f.requireErrored(t) + + // No change signal: the retry finds the file emptied and reads it again later. + f.write(t, "") + requireLogged(t, mockLog, ldlog.Debug, readAgainLog) + f.write(t, "flagValues: [") + + // The next retry fails as before the file was emptied, which is a repeat. + requireLogged(t, mockLog, ldlog.Debug, "Unable to load flags") + f.requireQuiet(t, 100*time.Millisecond) + assert.Len(t, mockLog.GetOutput(ldlog.Error), 1) +} diff --git a/internal/sharedtest/files.go b/internal/sharedtest/files.go new file mode 100644 index 00000000..5f31bdb6 --- /dev/null +++ b/internal/sharedtest/files.go @@ -0,0 +1,24 @@ +package sharedtest + +import ( + "os" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// RewriteFileInPlace rewrites an existing file the slow way: it truncates the file, waits for the +// pause, 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, so tests rewrite files this way rather than replacing them. +func RewriteFileInPlace(t testing.TB, path string, content string, pause time.Duration) { + t.Helper() + f, err := os.OpenFile(path, os.O_WRONLY|os.O_TRUNC, 0) //nolint:gosec // G304: test file path + require.NoError(t, err) + defer func() { require.NoError(t, f.Close()) }() + require.NoError(t, f.Sync()) + time.Sleep(pause) + _, err = f.WriteString(content) + require.NoError(t, err) +} diff --git a/ldclient_file_in_place_rewrite_test.go b/ldclient_file_in_place_rewrite_test.go new file mode 100644 index 00000000..3e452c4c --- /dev/null +++ b/ldclient_file_in_place_rewrite_test.go @@ -0,0 +1,154 @@ +package ldclient + +import ( + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/launchdarkly/go-sdk-common/v3/ldlogtest" + "github.com/launchdarkly/go-server-sdk/v7/internal/sharedtest" + "github.com/launchdarkly/go-server-sdk/v7/ldcomponents" + "github.com/launchdarkly/go-server-sdk/v7/ldfiledata" + "github.com/launchdarkly/go-server-sdk/v7/ldfiledatav2" + "github.com/launchdarkly/go-server-sdk/v7/ldfilewatch" + "github.com/launchdarkly/go-server-sdk/v7/ldoverrides" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The tests in this file rewrite a data file in place while a client serves its flags. 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. + +// inPlaceRewriteContents is the content of a file that serves "a" for flag1, and of the same +// file rewritten to serve "b", in each format. +var inPlaceRewriteContents = map[string][2]string{ + ".json": {`{"flagValues": {"flag1": "a"}}`, `{"flagValues": {"flag1": "b"}}`}, + ".yaml": {"flagValues:\n flag1: a\n", "flagValues:\n flag1: b\n"}, +} + +// servedDuringInPlaceRewrite rewrites the file in place and returns the values that the client +// served for flag1 meanwhile, in order and without repeats, up to the rewritten value. +func servedDuringInPlaceRewrite(t *testing.T, client *LDClient, path string, content string) []string { + t.Helper() + var mu sync.Mutex + var served []string + lastServed := func() string { + mu.Lock() + defer mu.Unlock() + if len(served) == 0 { + return "" + } + return served[len(served)-1] + } + stop := make(chan struct{}) + sampled := make(chan struct{}) + go func() { + defer close(sampled) + for { + select { + case <-stop: + return + default: + } + value, _ := client.StringVariation("flag1", evalTestUser, "default") + if value != lastServed() { + mu.Lock() + served = append(served, value) + mu.Unlock() + } + time.Sleep(2 * time.Millisecond) + } + }() + defer func() { + close(stop) + <-sampled + }() + + sharedtest.RewriteFileInPlace(t, path, content, 300*time.Millisecond) + + // Waits for the sampler itself to see the new value, so it has recorded everything that the + // client served before it. + require.Eventually(t, func() bool { return lastServed() == "b" }, 10*time.Second, 10*time.Millisecond, + "the client never served the rewritten file") + mu.Lock() + defer mu.Unlock() + return append([]string(nil), served...) +} + +func writeInPlaceRewriteFile(t *testing.T, name string, extension string) string { + t.Helper() + path := filepath.Join(t.TempDir(), name+extension) + require.NoError(t, os.WriteFile(path, []byte(inPlaceRewriteContents[extension][0]), 0600)) + return path +} + +func TestFileDataSourceInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { + for _, extension := range []string{".json", ".yaml"} { + t.Run(extension, func(t *testing.T) { + path := writeInPlaceRewriteFile(t, "flags", extension) + config := Config{ + Logging: ldcomponents.Logging().Loggers(ldlogtest.NewMockLog().Loggers), + Events: ldcomponents.NoEvents(), + DataSource: ldfiledata.DataSource().FilePaths(path).Reloader(ldfilewatch.WatchFiles), + } + client, err := MakeCustomClient(testSdkKey, config, 5*time.Second) + require.NoError(t, err) + defer client.Close() + value, _ := client.StringVariation("flag1", evalTestUser, "default") + require.Equal(t, "a", value) + + served := servedDuringInPlaceRewrite(t, client, path, inPlaceRewriteContents[extension][1]) + assert.Equal(t, []string{"a", "b"}, served) + }) + } +} + +func TestFileDataSynchronizerInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { + for _, extension := range []string{".json", ".yaml"} { + t.Run(extension, func(t *testing.T) { + path := writeInPlaceRewriteFile(t, "flags", extension) + config := Config{ + Logging: ldcomponents.Logging().Loggers(ldlogtest.NewMockLog().Loggers), + Events: ldcomponents.NoEvents(), + DataSystem: ldcomponents.DataSystem().Custom(). + Synchronizers(ldfiledatav2.DataSource().FilePaths(path).Reloader(ldfilewatch.WatchFiles)), + } + client, err := MakeCustomClient(testSdkKey, config, 5*time.Second) + require.NoError(t, err) + defer client.Close() + value, _ := client.StringVariation("flag1", evalTestUser, "default") + require.Equal(t, "a", value) + + served := servedDuringInPlaceRewrite(t, client, path, inPlaceRewriteContents[extension][1]) + assert.Equal(t, []string{"a", "b"}, served) + }) + } +} + +func TestFileOverridesInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { + for _, extension := range []string{".json", ".yaml"} { + t.Run(extension, func(t *testing.T) { + path := writeInPlaceRewriteFile(t, "overrides", extension) + config := Config{ + Logging: ldcomponents.Logging().Loggers(ldlogtest.NewMockLog().Loggers), + Events: ldcomponents.NoEvents(), + DataSystem: ldcomponents.DataSystem().Custom(). + Synchronizers(newHangingSynchronizer()). + Overrides(ldoverrides.FileSource().FilePaths(path).ChangeDetection(ldoverrides.Watching)), + } + client, _ := MakeCustomClient(testSdkKey, config, time.Duration(0)) + require.NotNil(t, client) + defer client.Close() + value, _ := client.StringVariation("flag1", evalTestUser, "default") + require.Equal(t, "a", value) + + // Without the override the client, which has no LaunchDarkly data, serves the default. + served := servedDuringInPlaceRewrite(t, client, path, inPlaceRewriteContents[extension][1]) + assert.Equal(t, []string{"a", "b"}, served) + }) + } +} diff --git a/ldfiledata/package_info.go b/ldfiledata/package_info.go index b86ab14c..f08feb4d 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -25,6 +25,15 @@ // file that is rewritten continuously, faster than that interval, does not reload until // the writes pause. // +// A file can be rewritten in place, so a reload can read it before it has been fully +// written. Content that cannot be parsed is retried about a second later. A file that had +// content and is found empty is read again after the same delay before it is applied, so +// the data source keeps its flags while the file is rewritten. A file that is still empty +// then is a YAML document with no flags (see the format below). YAML that is cut off can +// still parse, as a part of its entries or with its last value cut short, so for a YAML +// file, replacing its entire contents in one operation, such as renaming a fully written +// temporary file over it, avoids briefly loading only a part of it. +// // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three // properties: diff --git a/ldfiledatav2/package_info.go b/ldfiledatav2/package_info.go index 4812a51d..627c3871 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -25,6 +25,15 @@ // file that is rewritten continuously, faster than that interval, does not reload until // the writes pause. // +// A file can be rewritten in place, so a reload can read it before it has been fully +// written. Content that cannot be parsed is retried about a second later. A file that had +// content and is found empty is read again after the same delay before it is applied, so +// the data source keeps its flags while the file is rewritten. A file that is still empty +// then is a YAML document with no flags (see the format below). YAML that is cut off can +// still parse, as a part of its entries or with its last value cut short, so for a YAML +// file, replacing its entire contents in one operation, such as renaming a fully written +// temporary file over it, avoids briefly loading only a part of it. +// // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three // properties: diff --git a/ldoverrides/file_source_builder.go b/ldoverrides/file_source_builder.go index faacad73..dd0035aa 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -82,6 +82,18 @@ type FileSourceBuilder struct { // loaded overrides stay in effect. The source logs the failure, retries after a short // delay, and recovers on its own once the file is readable again. // +// 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 file that had content and is found empty may be +// in the middle of a rewrite, so the source reads it again after the retry delay, about a +// second, before applying it. The source tells the formats apart by the content: a file +// whose first character other than white space is "{" is JSON, and any other file is YAML. +// A file that is still empty when it is read again is therefore a YAML document with no +// entries, whatever its name, and an emptied file removes its overrides about a second +// later. YAML that is cut off can still parse, as a part of its entries or with its last +// value cut short, 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. +// // Whenever the set of overrides in effect changes, including at startup, the source logs // the overrides in effect and what each configured file supplied, at Info level. // diff --git a/ldoverrides/file_source_test.go b/ldoverrides/file_source_test.go index 0b725b8b..fa1b2df3 100644 --- a/ldoverrides/file_source_test.go +++ b/ldoverrides/file_source_test.go @@ -360,6 +360,44 @@ func TestFileSourceRetainsLastGoodDataAcrossMalformedEdit(t *testing.T) { require.Len(t, flags, 1) } +func TestFileSourceInPlaceRewriteNeverEmptiesTheOverrides(t *testing.T) { + // The file is empty for 300 ms while it is rewritten in place, longer than the settle + // window after a change, so a reload reads it empty. An empty snapshot would remove every + // override for that time. + contents := map[string][2]string{ + ".json": {`{"flagValues": {"flag1": "a"}}`, `{"flagValues": {"flag1": "b"}}`}, + ".yaml": {"flagValues:\n flag1: a\n", "flagValues:\n flag1: b\n"}, + } + for _, extension := range []string{".json", ".yaml"} { + for _, mode := range []ChangeDetection{Watching, Polling} { + t.Run(extension+" "+string(mode), func(t *testing.T) { + path := filepath.Join(t.TempDir(), "overrides"+extension) + writeFile(t, path, contents[extension][0]) + source, err := FileSource().FilePaths(path).ChangeDetection(mode).Build(sharedtest.BasicClientContext()) + require.NoError(t, err) + // With the minimum interval of a second, a rewrite can fall between two polls. A + // short interval makes a poll see the file while it is empty. + source.(*fileOverrideSource).pollInterval = 20 * time.Millisecond + sink := newCapturingSink() + source.Start(sink) + t.Cleanup(func() { _ = source.Close() }) + require.Equal(t, "a", flagsByKey(t, sink.requireSnapshot(t))["flag1"].Variations[0].StringValue()) + + sharedtest.RewriteFileInPlace(t, path, contents[extension][1], 300*time.Millisecond) + + // Every snapshot until the rewritten one still has the override. + for { + flags := flagsByKey(t, sink.requireSnapshot(t)) + require.Contains(t, flags, "flag1", "a snapshot without the override was applied") + if flags["flag1"].Variations[0].StringValue() == "b" { + break + } + } + }) + } + } +} + func TestFileSourcePollsByDefault(t *testing.T) { path := filepath.Join(t.TempDir(), "overrides.json") writeFile(t, path, `{}`) From b9c073a18c3bb3604ef1d50d769040b9712f7656 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 00:38:18 +0000 Subject: [PATCH 02/10] fix: Hold an emptied data file back for a retry delay, not for one load Holding an emptied file back for the next load only was bounded by a count of loads: any other load in the write gap, such as a change to another configured file, a touch, a poll or a watcher error, parsed the emptied file and applied it. The reloader now records, per file, whether the file contributed content to the data in effect, and when a load first found it empty together with its modification time. Every load holds such a file back until it has stayed empty, without being written, for nine tenths of the retry delay. A new modification time restarts the delay, and a file that keeps being written so that every load finds it empty is applied after at most five retry delays. A load reads every file before it holds any back, so files emptied together start their delays in the same load, and a file that fails to parse still fails the load. Also adds a closed check before holding back, restores the retry log text, asserts in the end-to-end tests that a reload found the file empty, and says in the file data source docs that the retry and the hold apply only with a reloader. --- internal/filedata/reloader.go | 153 ++++++++-- internal/filedata/reloader_test.go | 398 +++++++++++++++++++++---- ldclient_file_in_place_rewrite_test.go | 25 +- ldfiledata/package_info.go | 14 +- ldfiledatav2/package_info.go | 14 +- ldoverrides/file_source_builder.go | 23 +- ldoverrides/file_source_test.go | 7 +- 7 files changed, 526 insertions(+), 108 deletions(-) diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index 728f1727..d287290b 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -56,9 +56,10 @@ type ReloaderConfig struct { DebounceDelay time.Duration // RetryDelay is how long to wait after a failed reload before automatically retrying, // so that a failure observed while a file was being rewritten recovers even if no - // further change notification arrives. A file that had content and is found empty is - // read again after the same delay before it is applied. If zero or negative, there is - // no automatic retry, and an empty file is parsed as it is. + // further change notification arrives. A file whose content is in effect and that is + // found empty is not applied until it has stayed empty, without being written, for nine + // tenths of the same delay. If zero or negative, there is no automatic retry, and an + // empty file is parsed as it is. RetryDelay time.Duration // SkipUnchanged, if true, suppresses the Apply call when the files' raw contents are // byte-identical to the last successfully applied contents. @@ -67,8 +68,7 @@ type ReloaderConfig struct { // Reloader owns the reload cycle for a set of data files: it serializes reloads, debounces // change signals, retains the last good result on failure (by not calling Apply), retries -// after failures, reads a file that was emptied again before applying it, and skips no-op -// applications. +// after failures, holds back a file that was emptied, and skips no-op applications. type Reloader struct { cfg ReloaderConfig triggerCh chan struct{} @@ -83,11 +83,49 @@ type Reloader struct { reloadMu sync.Mutex lastGoodHash []byte lastErrorMsg string - // lastReadHadContent records, for each configured path, whether the last read of the - // file found content. A file that did not exist had no content. Guarded by reloadMu. - lastReadHadContent map[string]bool + // appliedHadContent records, for each configured path, whether the file contributed + // content to the data in effect, that is, to the last load that was applied or skipped as + // unchanged. A missing or empty file contributed none. Guarded by reloadMu. + appliedHadContent map[string]bool + // emptyFiles holds the paths whose content is in effect and that loads have found empty + // since. An entry is removed when a load reads the file with content. See holdBack. + // Guarded by reloadMu. + emptyFiles map[string]*emptyFile + // now returns the current time. Tests replace it. + now func() time.Time } +// emptyFile records when loads first found a file empty, and since when it has been empty +// without being written. +type emptyFile struct { + firstSeen time.Time + since time.Time + modified modTime +} + +// modTime is a file's modification time, or unknown when the file could not be examined. +type modTime struct { + time time.Time + known bool +} + +func (m modTime) equal(other modTime) bool { + return m.known == other.known && m.time.Equal(other.time) +} + +func modTimeOf(path string) modTime { + info, err := os.Stat(path) + if err != nil { + return modTime{} + } + return modTime{time: info.ModTime(), known: true} +} + +// maxHoldRetryDelays bounds the hold of an empty file that keeps being written, so that every +// load finds it empty: it is applied at the latest this many retry delays after a load first +// found it empty. +const maxHoldRetryDelays = 5 + // NewReloader creates a Reloader. The caller should perform the initial load with // ReloadNow, route change signals to Trigger, and call Close when finished. // @@ -97,11 +135,13 @@ type Reloader struct { // alone must not leak a goroutine. func NewReloader(cfg ReloaderConfig) *Reloader { return &Reloader{ - cfg: cfg, - triggerCh: make(chan struct{}, 1), - retryRequestCh: make(chan struct{}, 1), - closeCh: make(chan struct{}), - lastReadHadContent: make(map[string]bool, len(cfg.Paths)), + cfg: cfg, + triggerCh: make(chan struct{}, 1), + retryRequestCh: make(chan struct{}, 1), + closeCh: make(chan struct{}), + appliedHadContent: make(map[string]bool, len(cfg.Paths)), + emptyFiles: make(map[string]*emptyFile), + now: time.Now, } } @@ -168,7 +208,7 @@ func (r *Reloader) run() { reloadAndMaybeRetry := func(isRetry bool) { if isRetry { - r.cfg.Loggers.Debug("Retrying flag data load after an earlier failure or an emptied file") + r.cfg.Loggers.Debug("Retrying flag data load after earlier failure") } else { r.cfg.Loggers.Info("Reloading flag data after detecting a change") } @@ -221,7 +261,7 @@ func (r *Reloader) run() { // reload performs one full load of all configured files. It reports whether the load is // done, which is what decides whether a retry gets armed: it is false when the load failed -// or a file is to be read again, and a skipped no-op application counts as done. The whole +// or a file is held back, and a skipped no-op application counts as done. The whole // set is re-read on every reload: entries are combined across files in order, so a change // to one file can alter which file wins for a key. func (r *Reloader) reload() bool { @@ -236,6 +276,10 @@ func (r *Reloader) reload() bool { docs := make([]Document, 0, len(r.cfg.Paths)) files := make([]FileSummary, 0, len(r.cfg.Paths)) + hadContent := make(map[string]bool, len(r.cfg.Paths)) + // The files that holdBack holds back. The other files are still read, so that every file + // that is found empty starts its delay in this load. + var heldBack []string hasher := sha256.New() for _, path := range r.cfg.Paths { // One read feeds both the hash and the parse, so the skip-unchanged hash can never @@ -244,18 +288,20 @@ func (r *Reloader) reload() bool { if err != nil { if r.cfg.SkipMissingPaths && errors.Is(err, fs.ErrNotExist) { r.cfg.Loggers.Debugf("File %s does not exist; it contributes no data", path) - r.lastReadHadContent[path] = false + hadContent[path] = false files = append(files, FileSummary{Path: path}) continue } return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) } hasContent := len(bytes.TrimSpace(rawData)) > 0 - hadContent := r.lastReadHadContent[path] - r.lastReadHadContent[path] = hasContent - if !hasContent && hadContent && r.cfg.RetryDelay > 0 { - return r.readAgainLater(path) + if hasContent { + delete(r.emptyFiles, path) + } else if r.holdBack(path) { + heldBack = append(heldBack, path) + continue } + hadContent[path] = hasContent _, _ = hasher.Write(rawData) _, _ = hasher.Write([]byte{0}) doc, err := parseDocument(rawData) @@ -265,6 +311,9 @@ func (r *Reloader) reload() bool { docs = append(docs, doc) files = append(files, FileSummary{Path: path, Present: true}) } + if len(heldBack) > 0 { + return r.readAgainLater(heldBack) + } merged, err := Merge(r.cfg.DuplicateKeysHandling, r.cfg.MakeFlagWithValue, docs...) if err != nil { @@ -296,6 +345,11 @@ func (r *Reloader) reload() bool { recovering := r.lastErrorMsg != "" r.lastErrorMsg = "" hash := hasher.Sum(nil) + // The data of this load is in effect from here on, whether it is applied or is the same + // as the data already applied. + for path, had := range hadContent { + r.appliedHadContent[path] = had + } if r.cfg.SkipUnchanged && !recovering && bytes.Equal(hash, r.lastGoodHash) { return true } @@ -306,17 +360,54 @@ func (r *Reloader) reload() bool { return true } -// readAgainLater handles a file that had content when it was last read and is empty now. It -// may be in the middle of an in-place rewrite: a writer that truncates the file and then -// writes the new content. The load 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 new load sooner. That next load parses what the file holds then, -// because the file was empty when it was last read: a file that is still empty is a YAML -// document with no entries. So the current data is kept for at most one more load, and a -// file that is empty from the start, such as a placeholder, is never read again. -func (r *Reloader) readAgainLater(path string) bool { - r.cfg.Loggers.Debugf("File %s is empty, possibly while it is being rewritten; reading it again before applying it", - path) +// holdBack reports whether a load that found the file empty holds it back. 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, however many loads +// find it empty in that time. A change of its modification time means that it was written +// since a load last found it empty, for example rewritten and truncated again between two +// loads, 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 load finds it empty is +// applied at the latest maxHoldRetryDelays retry delays after a load first found it empty. A +// file whose content is not in effect, such as a placeholder that has always been empty, or +// an emptied file that has been applied, is parsed right away, and so is any empty file when +// retries are off, since nothing would read it again. +func (r *Reloader) holdBack(path string) bool { + if r.cfg.RetryDelay <= 0 || !r.appliedHadContent[path] { + return false + } + now := r.now() + modified := modTimeOf(path) + empty, ok := r.emptyFiles[path] + if !ok { + empty = &emptyFile{firstSeen: now, since: now, modified: modified} + r.emptyFiles[path] = empty + } else if !empty.modified.equal(modified) { + empty.since = now + empty.modified = modified + } + // The delay counts as over at nine tenths of the retry delay, as in the other SDKs, whose + // timers can fire slightly early, so the retry that the hold arms one retry delay later + // always parses a file that stayed empty. Once the delay is over the file is parsed, and + // so it is by every later load until it is written. + delay := r.cfg.RetryDelay - r.cfg.RetryDelay/10 + return now.Sub(empty.since) < delay && now.Sub(empty.firstSeen) < maxHoldRetryDelays*r.cfg.RetryDelay +} + +// readAgainLater handles the files that holdBack holds back. Whatever their content would +// parse as, the load 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 +// new load sooner. Once a file has stayed empty for the delay, a load parses it: a file that +// is still empty is a YAML document with no entries. +func (r *Reloader) readAgainLater(paths []string) bool { + // Close may have happened while the files were being read; arm no retry in that case. + if r.isClosed() { + return true + } + for _, path := range paths { + r.cfg.Loggers.Debugf("File %s is empty, possibly while it is being rewritten; reading it again before applying it", + path) + } return false } diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 21d2c521..47f11643 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -3,8 +3,11 @@ package filedata import ( "os" "path/filepath" + "regexp" "runtime" "slices" + "strings" + "sync" "testing" "time" @@ -460,22 +463,45 @@ func TestReloaderDebounceWindowIsExtendedByEachTrigger(t *testing.T) { f.requireQuiet(t, 2*window) } -// The retry delay for the tests of a file that is found empty. The retry comes long enough -// after the change signal for a test to finish rewriting the file before it, even on a slow -// machine. +// The retry delay for the tests of a file that is found empty that run on the real clock. The +// retry comes long enough after the change signal for a test to finish rewriting the file +// before it, even on a slow machine. const emptyFileRetryDelay = 1500 * time.Millisecond // Shorter than emptyFileRetryDelay, so a load that happens within it after a change signal // is not the retry. const beforeTheRetry = time.Second +// The retry delay for the tests that move a fake clock. The retry never runs during such a +// test, so only the test's change signals start loads, and the clock decides how long a file +// has been empty. +const fakeClockRetryDelay = time.Hour + const readAgainLog = "reading it again" const ( yamlFlag1 = "flagValues:\n flag1: true\n" yamlFlag2 = "flagValues:\n flag2: true\n" + yamlFlag3 = "flagValues:\n flag3: true\n" ) +type fakeClock struct { + mu sync.Mutex + now time.Time +} + +func (c *fakeClock) Now() time.Time { + c.mu.Lock() + defer c.mu.Unlock() + return c.now +} + +func (c *fakeClock) Advance(d time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + c.now = c.now.Add(d) +} + // newEmptyFileFixture configures a reloader for a YAML file that defines flag1, with the // retry delay of the tests of a file that is found empty, and a log that keeps debug output. func newEmptyFileFixture(t *testing.T, configure func(*ReloaderConfig)) (*reloaderFixture, *ldlogtest.MockLog) { @@ -492,6 +518,24 @@ func newEmptyFileFixture(t *testing.T, configure func(*ReloaderConfig)) (*reload return f, mockLog } +// newFakeClockFixture is like newEmptyFileFixture, but the reloader reads a fake clock and its +// retry never runs during the test. +func newFakeClockFixture( + t *testing.T, + configure func(*ReloaderConfig), +) (*reloaderFixture, *ldlogtest.MockLog, *fakeClock) { + t.Helper() + f, mockLog := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { + cfg.RetryDelay = fakeClockRetryDelay + if configure != nil { + configure(cfg) + } + }) + clock := &fakeClock{now: time.Unix(1_000_000, 0)} + f.reloader.now = clock.Now + return f, mockLog, clock +} + func flagKeys(result MergeResult) []string { keys := make([]string, 0, len(result.Flags)) for _, flag := range result.Flags { @@ -507,10 +551,55 @@ func requireLogged(t *testing.T, mockLog *ldlogtest.MockLog, level ldlog.LogLeve 10*time.Millisecond, "no %s log matching %q", level, pattern) } -func TestReloaderReadsAFileThatIsEmptiedAgainBeforeApplyingIt(t *testing.T) { +func holdCount(mockLog *ldlogtest.MockLog) int { + count := 0 + for _, line := range mockLog.GetOutput(ldlog.Debug) { + if strings.Contains(line, readAgainLog) { + count++ + } + } + return count +} + +// requireHeldBack waits for a load to hold back the given number of files, counted by the +// debug lines, and checks that it neither applied nor reported anything. It returns the +// number of files held back so far. +func (f *reloaderFixture) requireHeldBack(t *testing.T, mockLog *ldlogtest.MockLog, before int, files int) int { + t.Helper() + require.Eventually(t, func() bool { return holdCount(mockLog) >= before+files }, testTimeout, + 10*time.Millisecond, "the load did not hold back %d file(s)", files) + require.Equal(t, before+files, holdCount(mockLog)) + select { + case <-f.applied: + require.FailNow(t, "a load that held back a file called Apply") + case <-f.errored: + require.FailNow(t, "a load that held back a file called OnError") + default: + } + return before + files +} + +// emptyInPlace truncates the file without removing it, as a writer that rewrites it in place +// does first. It also moves the file's modification time a second later, so that the write is +// visible to the reloader even on a file system that records the time coarsely. +func emptyInPlace(t *testing.T, path string) { + t.Helper() + require.NoError(t, os.WriteFile(path, nil, 0600)) + moveModTimeLater(t, path) +} + +func moveModTimeLater(t *testing.T, path string) { + t.Helper() + info, err := os.Stat(path) + require.NoError(t, err) + later := info.ModTime().Add(time.Second) + require.NoError(t, os.Chtimes(path, later, later)) +} + +func TestReloaderHoldsBackAnEmptiedFileUntilItIsRewritten(t *testing.T) { // A writer that rewrites the file in place truncates it and then writes the new content. - // A load that finds the file empty after it had content keeps the data. An empty file is - // a YAML document with no entries, so applying it would briefly remove every entry. + // A load that finds the file empty after its content was applied keeps the data. An empty + // file is a YAML document with no entries, so applying it would briefly remove every entry. for name, emptied := range map[string]string{"empty": "", "white space only": " \n\r\n "} { t.Run(name, func(t *testing.T) { f, mockLog := newEmptyFileFixture(t, nil) @@ -519,7 +608,7 @@ func TestReloaderReadsAFileThatIsEmptiedAgainBeforeApplyingIt(t *testing.T) { f.write(t, emptied) f.reloader.Trigger() - requireLogged(t, mockLog, ldlog.Debug, readAgainLog) + f.requireHeldBack(t, mockLog, 0, 1) f.requireQuiet(t, 100*time.Millisecond) // No change signal: the retry reads the rewritten file. @@ -530,7 +619,7 @@ func TestReloaderReadsAFileThatIsEmptiedAgainBeforeApplyingIt(t *testing.T) { } } -func TestReloaderAppliesAnEmptiedFileThatIsStillEmptyWhenReadAgain(t *testing.T) { +func TestReloaderAppliesAnEmptiedFileThatStaysEmptyAtTheFirstRetry(t *testing.T) { f, _ := newEmptyFileFixture(t, nil) f.reloader.ReloadNow() f.requireApplied(t) @@ -539,29 +628,54 @@ func TestReloaderAppliesAnEmptiedFileThatIsStillEmptyWhenReadAgain(t *testing.T) f.reloader.Trigger() f.requireQuiet(t, 200*time.Millisecond) - // The file is still empty when the retry reads it, so it is a document with no entries. - assert.Empty(t, f.requireApplied(t).Flags) + // The file has stayed empty, without being written, for the retry delay when the retry + // reads it, so the retry parses it: a document with no entries. The second retry would + // come one more retry delay later. + assert.Empty(t, f.requireAppliedWithin(t, emptyFileRetryDelay+500*time.Millisecond).Flags) } -func TestReloaderReadsAnEmptiedFileAgainOnlyOnce(t *testing.T) { +func TestReloaderRetryHoldsBackAnEmptiedFile(t *testing.T) { + // A retry left over from a failure can run while the file is being rewritten in place. It + // holds the file back too, rather than applying it empty. f, mockLog := newEmptyFileFixture(t, nil) f.reloader.ReloadNow() f.requireApplied(t) + f.write(t, "flagValues: [") + f.reloader.Trigger() + f.requireErrored(t) + // No change signal: only the retry reads the file. f.write(t, "") - f.reloader.Trigger() - requireLogged(t, mockLog, ldlog.Debug, readAgainLog) - f.requireQuiet(t, 100*time.Millisecond) + f.requireHeldBack(t, mockLog, 0, 1) + f.write(t, yamlFlag2) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) +} - // A further change signal does not postpone the empty file again: it was empty when it - // was last read, so this load applies it, before the retry would have. +func TestReloaderHoldingBackAFileReportsNoFailure(t *testing.T) { + // Holding a file back reports nothing. A consumer that tracks failures through OnError + // hears of none, and a failure that was reported once is not reported again when the + // same content comes back after the file was held back. + f, mockLog := newEmptyFileFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + f.write(t, "flagValues: [") f.reloader.Trigger() - assert.Empty(t, f.requireAppliedWithin(t, beforeTheRetry).Flags) + f.requireErrored(t) + + // No change signal: the retry finds the file emptied and holds it back. + f.write(t, "") + f.requireHeldBack(t, mockLog, 0, 1) + f.write(t, "flagValues: [") + + // The next retry fails as before the file was emptied, which is a repeat. + requireLogged(t, mockLog, ldlog.Debug, "Unable to load flags") + f.requireQuiet(t, 100*time.Millisecond) + assert.Len(t, mockLog.GetOutput(ldlog.Error), 1) } -func TestReloaderDoesNotReadAgainAFileThatIsEmptyFromTheStart(t *testing.T) { +func TestReloaderDoesNotHoldBackAFileThatIsEmptyFromTheStart(t *testing.T) { // An empty file next to the data, such as a placeholder for later entries, has never had - // content. It does not hold back the changes of the other file. + // content in effect. It does not hold back the changes of the other file. placeholder := filepath.Join(t.TempDir(), "placeholder.yaml") require.NoError(t, os.WriteFile(placeholder, nil, 0600)) f, mockLog := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { @@ -573,7 +687,7 @@ func TestReloaderDoesNotReadAgainAFileThatIsEmptyFromTheStart(t *testing.T) { f.write(t, yamlFlag2) f.reloader.Trigger() assert.Equal(t, []string{"flag2"}, flagKeys(f.requireAppliedWithin(t, beforeTheRetry))) - assert.False(t, mockLog.HasMessageMatch(ldlog.Debug, readAgainLog)) + assert.Equal(t, 0, holdCount(mockLog)) } func TestReloaderAppliesAFileThatIsEmptyAtTheInitialLoad(t *testing.T) { @@ -585,24 +699,7 @@ func TestReloaderAppliesAFileThatIsEmptyAtTheInitialLoad(t *testing.T) { assert.Empty(t, f.requireAppliedAlready(t).Flags) } -func TestReloaderRetryReadsAnEmptiedFileAgain(t *testing.T) { - // A retry left over from a failure can run while the file is being rewritten in place. It - // reads the file again too, rather than applying it empty. - f, mockLog := newEmptyFileFixture(t, nil) - f.reloader.ReloadNow() - f.requireApplied(t) - f.write(t, "flagValues: [") - f.reloader.Trigger() - f.requireErrored(t) - - // No change signal: only the retry reads the file. - f.write(t, "") - requireLogged(t, mockLog, ldlog.Debug, readAgainLog) - f.write(t, yamlFlag2) - assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) -} - -func TestReloaderDoesNotReadAgainAFileThatWasMissing(t *testing.T) { +func TestReloaderDoesNotHoldBackAFileThatWasMissing(t *testing.T) { f, _ := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { cfg.SkipMissingPaths = true }) f.reloader.ReloadNow() f.requireApplied(t) @@ -610,7 +707,8 @@ func TestReloaderDoesNotReadAgainAFileThatWasMissing(t *testing.T) { f.reloader.Trigger() assert.Empty(t, f.requireApplied(t).Flags) - // A missing file had no content, so the empty file that replaces it is applied right away. + // A missing file contributed no content, so the empty file that replaces it is applied + // right away. require.NoError(t, os.WriteFile(f.path, nil, 0600)) f.reloader.Trigger() assert.Empty(t, f.requireAppliedWithin(t, beforeTheRetry).Flags) @@ -627,22 +725,218 @@ func TestReloaderParsesAnEmptiedFileRightAwayWithoutRetries(t *testing.T) { assert.Empty(t, f.requireAppliedAlready(t).Flags) } -func TestReloaderReadingAnEmptiedFileAgainIsNotAFailure(t *testing.T) { - // Reading a file again reports nothing. A consumer that tracks failures through OnError - // hears of none, and a failure that was reported once is not reported again when the - // same content comes back after the file was read again. - f, mockLog := newEmptyFileFixture(t, nil) - f.write(t, "flagValues: [") +func TestReloaderHoldsBackAnEmptiedFileForTheWholeDelay(t *testing.T) { + // Any load can find the file empty while it is rewritten: a change signal for another + // file, a touch, a poll. Every load within the delay holds it back. The delay counts as + // over at nine tenths of the retry delay, so that the retry that the hold arms parses a + // file that stayed empty. + f, mockLog, clock := newFakeClockFixture(t, nil) f.reloader.ReloadNow() - f.requireErrored(t) + f.requireApplied(t) - // No change signal: the retry finds the file emptied and reads it again later. f.write(t, "") - requireLogged(t, mockLog, ldlog.Debug, readAgainLog) - f.write(t, "flagValues: [") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) - // The next retry fails as before the file was emptied, which is a repeat. + clock.Advance(fakeClockRetryDelay * 85 / 100) + f.reloader.Trigger() + holds = f.requireHeldBack(t, mockLog, holds, 1) + + clock.Advance(fakeClockRetryDelay * 10 / 100) + f.reloader.Trigger() + assert.Empty(t, f.requireApplied(t).Flags) + assert.Equal(t, holds, holdCount(mockLog)) +} + +func TestReloaderDoesNotHoldBackAFileWhoseEmptyStateIsInEffect(t *testing.T) { + f, mockLog, clock := newFakeClockFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + f.write(t, "") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + clock.Advance(fakeClockRetryDelay) + f.reloader.Trigger() + assert.Empty(t, f.requireApplied(t).Flags) + + // The file is written again, and still empty. Its empty state is in effect, so there is + // nothing to keep. + emptyInPlace(t, f.path) + f.reloader.Trigger() + assert.Empty(t, f.requireApplied(t).Flags) + assert.Equal(t, holds, holdCount(mockLog)) +} + +func TestReloaderRestartsTheDelayWhenAnEmptiedFileIsWrittenAgain(t *testing.T) { + // Two quick rewrites in place: the second truncation comes before any load reads the + // content of the first. The file was written since a load last found it empty, so the + // delay starts over. + f, mockLog, clock := newFakeClockFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, "") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + + f.write(t, yamlFlag2) + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + f.reloader.Trigger() + holds = f.requireHeldBack(t, mockLog, holds, 1) + + // Longer than the delay since a load first found the file empty, shorter since it was + // written. + clock.Advance(fakeClockRetryDelay * 8 / 10) + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, holds, 1) + + f.write(t, yamlFlag3) + f.reloader.Trigger() + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireApplied(t))) +} + +func TestReloaderAppliesAnEmptiedFileThatKeepsBeingWrittenAfterFiveDelays(t *testing.T) { + // A file that is written again before every load, so that every load finds it empty, is + // not held back for ever. + f, mockLog, clock := newFakeClockFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + f.write(t, "") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + + for range 6 { + f.write(t, yamlFlag2) + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + f.reloader.Trigger() + holds = f.requireHeldBack(t, mockLog, holds, 1) + } + + // Five retry delays after a load first found the file empty, it is applied. + f.write(t, yamlFlag2) + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + f.reloader.Trigger() + assert.Empty(t, f.requireApplied(t).Flags) +} + +// newTwoFileFakeClockFixture configures a fake clock reloader for two YAML files, the +// fixture's file with flag1 and a second file with flag2, and applies them. +func newTwoFileFakeClockFixture(t *testing.T) (*reloaderFixture, string, *ldlogtest.MockLog, *fakeClock) { + t.Helper() + second := filepath.Join(t.TempDir(), "second.yaml") + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f, mockLog, clock := newFakeClockFixture(t, func(cfg *ReloaderConfig) { + cfg.Paths = append(cfg.Paths, second) + }) + f.reloader.ReloadNow() + require.Equal(t, []string{"flag1", "flag2"}, flagKeys(f.requireApplied(t))) + return f, second, mockLog, clock +} + +func TestReloaderHoldsBackAnEmptiedFileWhenAnotherFileChanges(t *testing.T) { + // The second file is rewritten in place. A change to the first file starts a load, and + // later another one, while the second file is still empty. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + require.NoError(t, os.WriteFile(second, nil, 0600)) + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + + clock.Advance(fakeClockRetryDelay * 3 / 10) + f.write(t, yamlFlag3) + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, holds, 1) + + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.reloader.Trigger() + assert.Equal(t, []string{"flag2", "flag3"}, flagKeys(f.requireApplied(t))) +} + +func TestReloaderStartsTheDelayOfEveryEmptiedFileInOneLoad(t *testing.T) { + // Both files are emptied at once. The load that finds them empty holds back both, so both + // are applied once the delay is over, rather than one retry delay per file. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + require.NoError(t, os.WriteFile(second, nil, 0600)) + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, 0, 2) + + clock.Advance(fakeClockRetryDelay * 95 / 100) + f.reloader.Trigger() + assert.Empty(t, f.requireApplied(t).Flags) +} + +func TestReloaderStartsANewDelayForEachEmptying(t *testing.T) { + // The file is rewritten in place twice, a while apart, and the second time a poll reads it + // empty twice. The second emptying gets a delay of its own: the time since the first one + // does not count towards it. + f, mockLog, clock := newFakeClockFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, "") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + f.write(t, yamlFlag2) + f.reloader.Trigger() + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + + clock.Advance(fakeClockRetryDelay * 49 / 10) + emptyInPlace(t, f.path) + f.reloader.Trigger() + holds = f.requireHeldBack(t, mockLog, holds, 1) + clock.Advance(fakeClockRetryDelay * 3 / 10) + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, holds, 1) + + f.write(t, yamlFlag3) + f.reloader.Trigger() + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireApplied(t))) +} + +func TestReloaderKeepsParsingAnEmptiedFileAfterItsDelay(t *testing.T) { + // The first file is emptied while the second cannot be parsed. Holding back the first file + // does not hide the failure of the second. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.reloader.Trigger() + require.ErrorContains(t, f.requireErrored(t), second) + + // After the delay, loads parse the first file, as long as it is not written. + clock.Advance(fakeClockRetryDelay * 95 / 100) + f.reloader.Trigger() requireLogged(t, mockLog, ldlog.Debug, "Unable to load flags") - f.requireQuiet(t, 100*time.Millisecond) - assert.Len(t, mockLog.GetOutput(ldlog.Error), 1) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.reloader.Trigger() + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, 0, holdCount(mockLog)) +} + +func TestReloaderRecordsWhatEachFileContributesWhenALoadIsSkippedAsUnchanged(t *testing.T) { + // The content moves from the second file to the first, so the files' combined content is + // the same and the load is skipped as unchanged. The data in effect now comes from the + // first file, so emptying it holds it back. + second := filepath.Join(t.TempDir(), "second.yaml") + require.NoError(t, os.WriteFile(second, []byte(yamlFlag1), 0600)) + f, mockLog, _ := newFakeClockFixture(t, func(cfg *ReloaderConfig) { + cfg.Paths = append(cfg.Paths, second) + cfg.SkipMissingPaths = true + cfg.SkipUnchanged = true + }) + require.NoError(t, os.Remove(f.path)) + f.reloader.ReloadNow() + require.Equal(t, []string{"flag1"}, flagKeys(f.requireApplied(t))) + + f.write(t, yamlFlag1) + require.NoError(t, os.Remove(second)) + f.reloader.Trigger() + requireLogged(t, mockLog, ldlog.Debug, regexp.QuoteMeta(second)+" does not exist") + f.requireQuiet(t, 50*time.Millisecond) + + f.write(t, "") + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, 0, 1) } diff --git a/ldclient_file_in_place_rewrite_test.go b/ldclient_file_in_place_rewrite_test.go index 3e452c4c..93f73041 100644 --- a/ldclient_file_in_place_rewrite_test.go +++ b/ldclient_file_in_place_rewrite_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/launchdarkly/go-sdk-common/v3/ldlog" "github.com/launchdarkly/go-sdk-common/v3/ldlogtest" "github.com/launchdarkly/go-server-sdk/v7/internal/sharedtest" "github.com/launchdarkly/go-server-sdk/v7/ldcomponents" @@ -21,7 +22,17 @@ import ( // The tests in this file rewrite a data file in place while a client serves its flags. 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. +// The client must keep serving the old value until the new one is written. Each test also checks +// the debug line of that reload, so that it does not pass only because no reload read the file +// while it was empty. + +const holdBackLog = "reading it again" + +// debugLog returns a log that keeps debug output, for the client's logging configuration. +func debugLog() (*ldlogtest.MockLog, *ldcomponents.LoggingConfigurationBuilder) { + mockLog := ldlogtest.NewMockLog() + return mockLog, ldcomponents.Logging().Loggers(mockLog.Loggers).MinLevel(ldlog.Debug) +} // inPlaceRewriteContents is the content of a file that serves "a" for flag1, and of the same // file rewritten to serve "b", in each format. @@ -90,8 +101,9 @@ func TestFileDataSourceInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { for _, extension := range []string{".json", ".yaml"} { t.Run(extension, func(t *testing.T) { path := writeInPlaceRewriteFile(t, "flags", extension) + mockLog, logging := debugLog() config := Config{ - Logging: ldcomponents.Logging().Loggers(ldlogtest.NewMockLog().Loggers), + Logging: logging, Events: ldcomponents.NoEvents(), DataSource: ldfiledata.DataSource().FilePaths(path).Reloader(ldfilewatch.WatchFiles), } @@ -103,6 +115,7 @@ func TestFileDataSourceInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { served := servedDuringInPlaceRewrite(t, client, path, inPlaceRewriteContents[extension][1]) assert.Equal(t, []string{"a", "b"}, served) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, holdBackLog), "no reload found the file empty") }) } } @@ -111,8 +124,9 @@ func TestFileDataSynchronizerInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) for _, extension := range []string{".json", ".yaml"} { t.Run(extension, func(t *testing.T) { path := writeInPlaceRewriteFile(t, "flags", extension) + mockLog, logging := debugLog() config := Config{ - Logging: ldcomponents.Logging().Loggers(ldlogtest.NewMockLog().Loggers), + Logging: logging, Events: ldcomponents.NoEvents(), DataSystem: ldcomponents.DataSystem().Custom(). Synchronizers(ldfiledatav2.DataSource().FilePaths(path).Reloader(ldfilewatch.WatchFiles)), @@ -125,6 +139,7 @@ func TestFileDataSynchronizerInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) served := servedDuringInPlaceRewrite(t, client, path, inPlaceRewriteContents[extension][1]) assert.Equal(t, []string{"a", "b"}, served) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, holdBackLog), "no reload found the file empty") }) } } @@ -133,8 +148,9 @@ func TestFileOverridesInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { for _, extension := range []string{".json", ".yaml"} { t.Run(extension, func(t *testing.T) { path := writeInPlaceRewriteFile(t, "overrides", extension) + mockLog, logging := debugLog() config := Config{ - Logging: ldcomponents.Logging().Loggers(ldlogtest.NewMockLog().Loggers), + Logging: logging, Events: ldcomponents.NoEvents(), DataSystem: ldcomponents.DataSystem().Custom(). Synchronizers(newHangingSynchronizer()). @@ -149,6 +165,7 @@ func TestFileOverridesInPlaceRewriteNeverServesAnEmptyFile(t *testing.T) { // Without the override the client, which has no LaunchDarkly data, serves the default. served := servedDuringInPlaceRewrite(t, client, path, inPlaceRewriteContents[extension][1]) assert.Equal(t, []string{"a", "b"}, served) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, holdBackLog), "no reload found the file empty") }) } } diff --git a/ldfiledata/package_info.go b/ldfiledata/package_info.go index f08feb4d..3723dd34 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -25,11 +25,15 @@ // file that is rewritten continuously, faster than that interval, does not reload until // the writes pause. // -// A file can be rewritten in place, so a reload can read it before it has been fully -// written. Content that cannot be parsed is retried about a second later. A file that had -// content and is found empty is read again after the same delay before it is applied, so -// the data source keeps its flags while the file is rewritten. A file that is still empty -// then is a YAML document with no flags (see the format below). YAML that is cut off can +// With a reloader, a file can be rewritten in place, so a reload can read it before it has +// been fully written. Content that cannot be parsed is retried about a second later. A file +// whose flags are in effect and that is found empty is not applied until it has stayed +// empty, without being written, for about a second, however often the data source reloads +// in that time. A rewrite that leaves the file empty for less than nine tenths of a second +// therefore keeps the flags in effect throughout. A file that is written so often that +// every reload finds it empty is applied about five seconds after a reload first found it +// empty. A file that stays empty is a YAML document with no flags (see the format below). +// Without a reloader, the files are loaded once, with no retry. YAML that is cut off can // still parse, as a part of its entries or with its last value cut short, so for a YAML // file, replacing its entire contents in one operation, such as renaming a fully written // temporary file over it, avoids briefly loading only a part of it. diff --git a/ldfiledatav2/package_info.go b/ldfiledatav2/package_info.go index 627c3871..d933454c 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -25,11 +25,15 @@ // file that is rewritten continuously, faster than that interval, does not reload until // the writes pause. // -// A file can be rewritten in place, so a reload can read it before it has been fully -// written. Content that cannot be parsed is retried about a second later. A file that had -// content and is found empty is read again after the same delay before it is applied, so -// the data source keeps its flags while the file is rewritten. A file that is still empty -// then is a YAML document with no flags (see the format below). YAML that is cut off can +// With a reloader, a file can be rewritten in place, so a reload can read it before it has +// been fully written. Content that cannot be parsed is retried about a second later. A file +// whose flags are in effect and that is found empty is not applied until it has stayed +// empty, without being written, for about a second, however often the data source reloads +// in that time. A rewrite that leaves the file empty for less than nine tenths of a second +// therefore keeps the flags in effect throughout. A file that is written so often that +// every reload finds it empty is applied about five seconds after a reload first found it +// empty. A file that stays empty is a YAML document with no flags (see the format below). +// Without a reloader, the files are loaded once, with no retry. YAML that is cut off can // still parse, as a part of its entries or with its last value cut short, so for a YAML // file, replacing its entire contents in one operation, such as renaming a fully written // temporary file over it, avoids briefly loading only a part of it. diff --git a/ldoverrides/file_source_builder.go b/ldoverrides/file_source_builder.go index dd0035aa..f5e23dd4 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -83,16 +83,19 @@ type FileSourceBuilder struct { // delay, and recovers on its own once the file is readable again. // // 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 file that had content and is found empty may be -// in the middle of a rewrite, so the source reads it again after the retry delay, about a -// second, before applying it. The source tells the formats apart by the content: a file -// whose first character other than white space is "{" is JSON, and any other file is YAML. -// A file that is still empty when it is read again is therefore a YAML document with no -// entries, whatever its name, and an emptied file removes its overrides about a second -// later. YAML that is cut off can still parse, as a part of its entries or with its last -// value cut short, 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. +// document with no entries, such as {}. A file whose overrides are in effect and that is +// found empty may be in the middle of a rewrite, so the source keeps its overrides in +// effect until the file has stayed empty, without being written, for about a second, +// however often it reloads in that time. A rewrite that leaves the file empty for less than +// nine tenths of a second therefore never removes them. A file that is written so often +// that every reload finds it empty is applied about five seconds after a reload first found +// it empty. The source tells the formats apart by the content: a file whose first character +// other than white space is "{" is JSON, and any other file is YAML. An empty file is +// therefore a YAML document with no entries, whatever its name, and a file that stays empty +// removes its overrides about a second after it was emptied. YAML that is cut off can still +// parse, as a part of its entries or with its last value cut short, 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. // // Whenever the set of overrides in effect changes, including at startup, the source logs // the overrides in effect and what each configured file supplied, at Info level. diff --git a/ldoverrides/file_source_test.go b/ldoverrides/file_source_test.go index fa1b2df3..ff81d9ff 100644 --- a/ldoverrides/file_source_test.go +++ b/ldoverrides/file_source_test.go @@ -373,7 +373,10 @@ func TestFileSourceInPlaceRewriteNeverEmptiesTheOverrides(t *testing.T) { t.Run(extension+" "+string(mode), func(t *testing.T) { path := filepath.Join(t.TempDir(), "overrides"+extension) writeFile(t, path, contents[extension][0]) - source, err := FileSource().FilePaths(path).ChangeDetection(mode).Build(sharedtest.BasicClientContext()) + mockLog := ldlogtest.NewMockLog() + mockLog.Loggers.SetMinLevel(ldlog.Debug) + context := sharedtest.NewTestContext("", nil, &subsystems.LoggingConfiguration{Loggers: mockLog.Loggers}) + source, err := FileSource().FilePaths(path).ChangeDetection(mode).Build(context) require.NoError(t, err) // With the minimum interval of a second, a rewrite can fall between two polls. A // short interval makes a poll see the file while it is empty. @@ -393,6 +396,8 @@ func TestFileSourceInPlaceRewriteNeverEmptiesTheOverrides(t *testing.T) { break } } + // A reload did read the file while it was empty. + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, "reading it again"), "no reload found the file empty") }) } } From 3ff249bc3bf5209f66ec34ccee6b8b0fb975deb1 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 00:49:01 +0000 Subject: [PATCH 03/10] test: Hold back a file whose content was moved from another configured file --- internal/filedata/reloader_test.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 47f11643..5b66159f 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -915,28 +915,33 @@ func TestReloaderKeepsParsingAnEmptiedFileAfterItsDelay(t *testing.T) { assert.Equal(t, 0, holdCount(mockLog)) } -func TestReloaderRecordsWhatEachFileContributesWhenALoadIsSkippedAsUnchanged(t *testing.T) { - // The content moves from the second file to the first, so the files' combined content is - // the same and the load is skipped as unchanged. The data in effect now comes from the - // first file, so emptying it holds it back. - second := filepath.Join(t.TempDir(), "second.yaml") - require.NoError(t, os.WriteFile(second, []byte(yamlFlag1), 0600)) +func TestReloaderHoldsBackAFileWhoseContentWasMovedFromAnotherFile(t *testing.T) { + // The content of one configured file is moved to the other (mv second.yaml data.yaml). The + // files' combined content is the same, so the load is skipped as unchanged, but the data in + // effect now comes from the file it was moved to. A rewrite of that file in place holds it + // back like any other. + var second string f, mockLog, _ := newFakeClockFixture(t, func(cfg *ReloaderConfig) { + second = filepath.Join(filepath.Dir(cfg.Paths[0]), "second.yaml") cfg.Paths = append(cfg.Paths, second) cfg.SkipMissingPaths = true cfg.SkipUnchanged = true }) require.NoError(t, os.Remove(f.path)) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag1), 0600)) f.reloader.ReloadNow() require.Equal(t, []string{"flag1"}, flagKeys(f.requireApplied(t))) - f.write(t, yamlFlag1) - require.NoError(t, os.Remove(second)) + require.NoError(t, os.Rename(second, f.path)) f.reloader.Trigger() requireLogged(t, mockLog, ldlog.Debug, regexp.QuoteMeta(second)+" does not exist") f.requireQuiet(t, 50*time.Millisecond) + // The file is rewritten in place: truncated, then written after a load has found it empty. f.write(t, "") f.reloader.Trigger() f.requireHeldBack(t, mockLog, 0, 1) + f.write(t, yamlFlag2) + f.reloader.Trigger() + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) } From f39b401680ffa6f89017f9f4b68af081c7f4f99b Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:31:57 +0000 Subject: [PATCH 04/10] fix: Bound the hold of emptied data files by a run of loads, and time its retry Round 3 bounded the hold of each file by five retry delays from when a load first found that file empty, and a read with content reset it, so two files rewritten in turn held every load back without end. The bound now applies to the run of loads that hold files back: it starts with the first load that holds a file back and ends with any load that holds none, whether it succeeds or fails. A file is held back until it has stayed empty, without being written, for exactly the retry delay, rather than nine tenths of it, which let another load apply a shorter gap. The hold arms its retry for when the first held file's delay or the run's bound is over, plus a tenth of the retry delay, instead of a whole retry delay after the last load. A file whose content is no YAML document, such as only a comment or "---", has no entries, and is held back like an empty file. {} is applied right away. The file's state, its modification time and size read through any symbolic link, comes from the poller's helper. --- internal/filedata/document.go | 18 ++ internal/filedata/poller.go | 17 +- internal/filedata/reloader.go | 224 +++++++++++-------- internal/filedata/reloader_test.go | 335 +++++++++++++++++++++-------- ldfiledata/package_info.go | 23 +- ldfiledatav2/package_info.go | 23 +- ldoverrides/file_source_builder.go | 28 +-- 7 files changed, 443 insertions(+), 225 deletions(-) diff --git a/internal/filedata/document.go b/internal/filedata/document.go index f54ab857..084eb76a 100644 --- a/internal/filedata/document.go +++ b/internal/filedata/document.go @@ -3,6 +3,7 @@ package filedata import ( + "bytes" "encoding/json" "fmt" "os" @@ -67,6 +68,23 @@ func parseDocument(rawData []byte) (Document, error) { return data, err } +// parseFileContent parses a file's content like parseDocument, and also reports whether the +// content holds a document. YAML with no document, such as only a comment or "---", parses +// as a document with no entries, like an empty file. JSON content is always an object, so it +// always holds one, even "{}". +func parseFileContent(rawData []byte) (Document, bool, error) { + doc, err := parseDocument(rawData) + if err != nil || detectJSON(rawData) || doc.Flags != nil || doc.FlagValues != nil || doc.Segments != nil { + return doc, true, err + } + asJSON, err := yaml.YAMLToJSON(rawData) + if err != nil { + // COVERAGE: unreachable, because parseDocument converted the same content + return doc, true, nil + } + return doc, !bytes.Equal(bytes.TrimSpace(asJSON), []byte("null")), nil +} + func detectJSON(rawData []byte) bool { // A valid JSON file for our purposes must be an object, i.e. it must start with '{' return strings.HasPrefix(strings.TrimLeftFunc(string(rawData), unicode.IsSpace), "{") diff --git a/internal/filedata/poller.go b/internal/filedata/poller.go index 821eb732..1e665305 100644 --- a/internal/filedata/poller.go +++ b/internal/filedata/poller.go @@ -88,9 +88,20 @@ func (p *Poller) run() { func observeAll(paths []string) []fileState { states := make([]fileState, len(paths)) for i, path := range paths { - if info, err := os.Stat(path); err == nil { - states[i] = fileState{exists: true, modTime: info.ModTime(), size: info.Size()} - } + states[i] = observe(path) } return states } + +// observe examines one file. os.Stat follows symbolic links, so the state is that of the +// file a link points to. +func observe(path string) fileState { + if info, err := os.Stat(path); err == nil { + return fileState{exists: true, modTime: info.ModTime(), size: info.Size()} + } + return fileState{} +} + +func (s fileState) equal(other fileState) bool { + return s.exists == other.exists && s.modTime.Equal(other.modTime) && s.size == other.size +} diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index d287290b..a0ab2491 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -57,9 +57,10 @@ type ReloaderConfig struct { // RetryDelay is how long to wait after a failed reload before automatically retrying, // so that a failure observed while a file was being rewritten recovers even if no // further change notification arrives. A file whose content is in effect and that is - // found empty is not applied until it has stayed empty, without being written, for nine - // tenths of the same delay. If zero or negative, there is no automatic retry, and an - // empty file is parsed as it is. + // found with no entries is not applied until it has stayed so, without being written, + // for the same delay, or for at most maxHoldRetryDelays delays while loads keep holding + // files back. If zero or negative, there is no automatic retry, and such a file is + // parsed as it is. RetryDelay time.Duration // SkipUnchanged, if true, suppresses the Apply call when the files' raw contents are // byte-identical to the last successfully applied contents. @@ -72,9 +73,10 @@ type ReloaderConfig struct { type Reloader struct { cfg ReloaderConfig triggerCh chan struct{} - // retryRequestCh asks the run goroutine to arm the retry timer without performing an - // immediate reload; it is used when a synchronous ReloadNow fails. - retryRequestCh chan struct{} + // retryRequestCh asks the run goroutine to arm the retry timer, for the delay it carries, + // without performing an immediate reload; it is used when a synchronous ReloadNow fails or + // holds a file back. + retryRequestCh chan time.Duration closeCh chan struct{} closeOnce sync.Once startOnce sync.Once @@ -87,43 +89,30 @@ type Reloader struct { // content to the data in effect, that is, to the last load that was applied or skipped as // unchanged. A missing or empty file contributed none. Guarded by reloadMu. appliedHadContent map[string]bool - // emptyFiles holds the paths whose content is in effect and that loads have found empty - // since. An entry is removed when a load reads the file with content. See holdBack. - // Guarded by reloadMu. + // emptyFiles holds the paths whose content is in effect and that loads have found with no + // entries since. An entry is removed when a load reads the file with entries. See + // holdBack. Guarded by reloadMu. emptyFiles map[string]*emptyFile + // holding is true while the loads hold files back, from the first load of such a run, + // at holdingSince, until a load holds none back. heldBack is set by a load that holds + // files back. Guarded by reloadMu. + holding bool + holdingSince time.Time + heldBack bool // now returns the current time. Tests replace it. now func() time.Time } -// emptyFile records when loads first found a file empty, and since when it has been empty -// without being written. +// emptyFile records since when a file has had no entries without being written, and the +// file's state then. type emptyFile struct { - firstSeen time.Time - since time.Time - modified modTime + since time.Time + state fileState } -// modTime is a file's modification time, or unknown when the file could not be examined. -type modTime struct { - time time.Time - known bool -} - -func (m modTime) equal(other modTime) bool { - return m.known == other.known && m.time.Equal(other.time) -} - -func modTimeOf(path string) modTime { - info, err := os.Stat(path) - if err != nil { - return modTime{} - } - return modTime{time: info.ModTime(), known: true} -} - -// maxHoldRetryDelays bounds the hold of an empty file that keeps being written, so that every -// load finds it empty: it is applied at the latest this many retry delays after a load first -// found it empty. +// maxHoldRetryDelays bounds a run of loads that hold files back. A file that keeps being +// written so that every load finds it with no entries, or files that are rewritten in turn +// so that every load finds one of them so, would otherwise be held back without end. const maxHoldRetryDelays = 5 // NewReloader creates a Reloader. The caller should perform the initial load with @@ -137,7 +126,7 @@ func NewReloader(cfg ReloaderConfig) *Reloader { return &Reloader{ cfg: cfg, triggerCh: make(chan struct{}, 1), - retryRequestCh: make(chan struct{}, 1), + retryRequestCh: make(chan time.Duration, 1), closeCh: make(chan struct{}), appliedHadContent: make(map[string]bool, len(cfg.Paths)), emptyFiles: make(map[string]*emptyFile), @@ -162,9 +151,9 @@ func (r *Reloader) ensureStarted() { // failed triggered reload. func (r *Reloader) ReloadNow() { r.ensureStarted() - if !r.reload() && r.cfg.RetryDelay > 0 { + if retryAfter := r.reload(); retryAfter > 0 { select { - case r.retryRequestCh <- struct{}{}: + case r.retryRequestCh <- retryAfter: default: } } @@ -216,8 +205,8 @@ func (r *Reloader) run() { // and arms a fresh retry below. stopTimer(retryTimer) retryTimer, retryC = nil, nil - if !r.reload() && r.cfg.RetryDelay > 0 { - retryTimer = time.NewTimer(r.cfg.RetryDelay) + if retryAfter := r.reload(); retryAfter > 0 { + retryTimer = time.NewTimer(retryAfter) retryC = retryTimer.C } } @@ -242,11 +231,11 @@ func (r *Reloader) run() { stopTimer(debounceTimer) debounceTimer.Reset(r.cfg.DebounceDelay) } - case <-r.retryRequestCh: - // A synchronous ReloadNow failed; arm the retry timer without reloading again - // immediately. An already-armed retry keeps its earlier deadline. + case retryAfter := <-r.retryRequestCh: + // A synchronous ReloadNow failed or held a file back; arm the retry timer without + // reloading again immediately. An already-armed retry keeps its deadline. if retryTimer == nil { - retryTimer = time.NewTimer(r.cfg.RetryDelay) + retryTimer = time.NewTimer(retryAfter) retryC = retryTimer.C } case <-debounceC: @@ -259,21 +248,32 @@ func (r *Reloader) run() { } } -// reload performs one full load of all configured files. It reports whether the load is -// done, which is what decides whether a retry gets armed: it is false when the load failed -// or a file is held back, and a skipped no-op application counts as done. The whole -// set is re-read on every reload: entries are combined across files in order, so a change -// to one file can alter which file wins for a key. -func (r *Reloader) reload() bool { +// reload performs one full load of all configured files. It returns how long to wait before +// a retry, or zero when the load is done and no retry is needed: a load that failed asks for +// the retry delay, one that held a file back asks for the time until that file can be +// applied, and a skipped no-op application counts as done. The whole set is re-read on every +// reload: entries are combined across files in order, so a change to one file can alter +// which file wins for a key. +func (r *Reloader) reload() time.Duration { r.reloadMu.Lock() defer r.reloadMu.Unlock() // A trigger already queued when Close was called can still reach here; the select in // run() does not prioritize closeCh over triggerCh. if r.isClosed() { - return true + return 0 } + r.heldBack = false + defer func() { + // A run of loads that hold files back ends with any load that does not, whether it + // succeeded or failed. + if !r.heldBack { + r.holding = false + } + }() + + now := r.now() docs := make([]Document, 0, len(r.cfg.Paths)) files := make([]FileSummary, 0, len(r.cfg.Paths)) hadContent := make(map[string]bool, len(r.cfg.Paths)) @@ -294,25 +294,39 @@ func (r *Reloader) reload() bool { } return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) } + // A file with no content, or with content that is no YAML document, such as only a + // comment or "---", has no entries. A read can see that much of a file that is being + // written. + var doc Document + parsed := false hasContent := len(bytes.TrimSpace(rawData)) > 0 + if hasContent { + doc, hasContent, err = parseFileContent(rawData) + if err != nil { + return r.fail(&ReadError{Err: err, Path: path}) + } + parsed = true + } if hasContent { delete(r.emptyFiles, path) - } else if r.holdBack(path) { + } else if r.holdBack(path, now) { heldBack = append(heldBack, path) continue } hadContent[path] = hasContent _, _ = hasher.Write(rawData) _, _ = hasher.Write([]byte{0}) - doc, err := parseDocument(rawData) - if err != nil { - return r.fail(&ReadError{Err: err, Path: path}) + if !parsed { + doc, err = parseDocument(rawData) + if err != nil { + return r.fail(&ReadError{Err: err, Path: path}) + } } docs = append(docs, doc) files = append(files, FileSummary{Path: path, Present: true}) } if len(heldBack) > 0 { - return r.readAgainLater(heldBack) + return r.readAgainLater(heldBack, now) } merged, err := Merge(r.cfg.DuplicateKeysHandling, r.cfg.MakeFlagWithValue, docs...) @@ -336,7 +350,7 @@ func (r *Reloader) reload() bool { // then wedge shutdown), so a reload that passes this check can rarely deliver just // after Close returns -- see the Close doc. if r.isClosed() { - return true + return 0 } // A success right after a failure must apply even when the content is unchanged since @@ -351,71 +365,85 @@ func (r *Reloader) reload() bool { r.appliedHadContent[path] = had } if r.cfg.SkipUnchanged && !recovering && bytes.Equal(hash, r.lastGoodHash) { - return true + return 0 } r.lastGoodHash = hash if r.cfg.Apply != nil { r.cfg.Apply(merged) } - return true + return 0 } -// holdBack reports whether a load that found the file empty holds it back. 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, however many loads -// find it empty in that time. A change of its modification time means that it was written -// since a load last found it empty, for example rewritten and truncated again between two -// loads, 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 load finds it empty is -// applied at the latest maxHoldRetryDelays retry delays after a load first found it empty. A -// file whose content is not in effect, such as a placeholder that has always been empty, or -// an emptied file that has been applied, is parsed right away, and so is any empty file when -// retries are off, since nothing would read it again. -func (r *Reloader) holdBack(path string) bool { +// holdBack reports whether a load that found the file with no entries holds it back. A file +// whose content is in effect and that has no entries 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, however many +// loads find it so in that time. A change of its state, its modification time and size read +// through any symbolic link, means that it was written since a load last found it so, for +// example rewritten and truncated again between two loads, so the delay starts over. Loads +// stop holding files back once they have held some back for maxHoldRetryDelays retry delays +// in a row, so that a file that keeps being written, or files that are rewritten in turn, +// are not held back without end. A file whose content is not in effect, such as a +// placeholder that has always been empty or an emptied file whose empty state has been +// applied, is parsed right away, and so is any file when retries are off, since nothing +// would read it again. +func (r *Reloader) holdBack(path string, now time.Time) bool { if r.cfg.RetryDelay <= 0 || !r.appliedHadContent[path] { return false } - now := r.now() - modified := modTimeOf(path) + state := observe(path) empty, ok := r.emptyFiles[path] if !ok { - empty = &emptyFile{firstSeen: now, since: now, modified: modified} + empty = &emptyFile{since: now, state: state} r.emptyFiles[path] = empty - } else if !empty.modified.equal(modified) { + } else if !empty.state.equal(state) { empty.since = now - empty.modified = modified + empty.state = state } - // The delay counts as over at nine tenths of the retry delay, as in the other SDKs, whose - // timers can fire slightly early, so the retry that the hold arms one retry delay later - // always parses a file that stayed empty. Once the delay is over the file is parsed, and - // so it is by every later load until it is written. - delay := r.cfg.RetryDelay - r.cfg.RetryDelay/10 - return now.Sub(empty.since) < delay && now.Sub(empty.firstSeen) < maxHoldRetryDelays*r.cfg.RetryDelay + holdingSince := now + if r.holding { + holdingSince = r.holdingSince + } + return now.Sub(empty.since) < r.cfg.RetryDelay && now.Sub(holdingSince) < r.maxHold() +} + +func (r *Reloader) maxHold() time.Duration { + return maxHoldRetryDelays * r.cfg.RetryDelay } // readAgainLater handles the files that holdBack holds back. Whatever their content would -// parse as, the load 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 -// new load sooner. Once a file has stayed empty for the delay, a load parses it: a file that -// is still empty is a YAML document with no entries. -func (r *Reloader) readAgainLater(paths []string) bool { +// parse as, the load keeps the current data and arms the retry, without reporting a failure. +// The retry comes when the first of the files has stayed so for the retry delay, or when the +// loads have held files back for as long as they may, plus a tenth of the retry delay, so +// that it finds the delay over; the change signal for a write starts a load sooner. Once a +// file has stayed so for the delay, a load parses it: a file that is still empty, or holds +// only a YAML comment, is a YAML document with no entries. +func (r *Reloader) readAgainLater(paths []string, now time.Time) time.Duration { // Close may have happened while the files were being read; arm no retry in that case. if r.isClosed() { - return true + return 0 } + if !r.holding { + r.holding = true + r.holdingSince = now + } + until := r.holdingSince.Add(r.maxHold()) for _, path := range paths { r.cfg.Loggers.Debugf("File %s is empty, possibly while it is being rewritten; reading it again before applying it", path) + if delayOver := r.emptyFiles[path].since.Add(r.cfg.RetryDelay); delayOver.Before(until) { + until = delayOver + } } - return false + r.heldBack = true + return max(until.Sub(now), 0) + r.cfg.RetryDelay/10 } -func (r *Reloader) fail(err error) bool { +func (r *Reloader) fail(err error) time.Duration { // Close may have happened while the files were being read; deliver nothing in that - // case, and report success so no retry is armed. + // case, and arm no retry. if r.isClosed() { - return true + return 0 } // With automatic retries, a persistent failure would repeat the same log entry and the // same callback on every attempt, so repeats of an identical failure are demoted to @@ -423,14 +451,20 @@ func (r *Reloader) fail(err error) bool { // one report per distinct failure rather than one per retry. if err.Error() == r.lastErrorMsg { r.cfg.Loggers.Debugf("Unable to load flags: %s", err) - return false + return r.failureRetryDelay() } r.lastErrorMsg = err.Error() r.cfg.Loggers.Errorf("Unable to load flags: %s", err) if r.cfg.OnError != nil { r.cfg.OnError(err) } - return false + return r.failureRetryDelay() +} + +// failureRetryDelay is how long to wait after a failed load before retrying, or zero for no +// retry. +func (r *Reloader) failureRetryDelay() time.Duration { + return max(r.cfg.RetryDelay, 0) } func (r *Reloader) isClosed() bool { diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 5b66159f..773e2414 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -600,7 +600,7 @@ func TestReloaderHoldsBackAnEmptiedFileUntilItIsRewritten(t *testing.T) { // A writer that rewrites the file in place truncates it and then writes the new content. // A load that finds the file empty after its content was applied keeps the data. An empty // file is a YAML document with no entries, so applying it would briefly remove every entry. - for name, emptied := range map[string]string{"empty": "", "white space only": " \n\r\n "} { + for name, emptied := range map[string]string{"empty": "", "white space only": " \n\t\r\n "} { t.Run(name, func(t *testing.T) { f, mockLog := newEmptyFileFixture(t, nil) f.reloader.ReloadNow() @@ -620,20 +620,40 @@ func TestReloaderHoldsBackAnEmptiedFileUntilItIsRewritten(t *testing.T) { } func TestReloaderAppliesAnEmptiedFileThatStaysEmptyAtTheFirstRetry(t *testing.T) { - f, _ := newEmptyFileFixture(t, nil) + f, mockLog := newEmptyFileFixture(t, nil) f.reloader.ReloadNow() f.requireApplied(t) f.write(t, "") f.reloader.Trigger() - f.requireQuiet(t, 200*time.Millisecond) + f.requireHeldBack(t, mockLog, 0, 1) - // The file has stayed empty, without being written, for the retry delay when the retry - // reads it, so the retry parses it: a document with no entries. The second retry would - // come one more retry delay later. + // The retry comes a tenth of the retry delay after the file has stayed empty, without + // being written, for the retry delay, so it parses it: a document with no entries. A + // second retry would come one more retry delay later. assert.Empty(t, f.requireAppliedWithin(t, emptyFileRetryDelay+500*time.Millisecond).Flags) } +func TestReloaderRetriesAHeldBackFileWhenItsDelayIsOver(t *testing.T) { + // A change signal while the file is held back does not postpone the retry: it comes when + // the file has stayed empty for the retry delay, not a retry delay after the last load. + const retryDelay = 3 * time.Second + f, mockLog := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { cfg.RetryDelay = retryDelay }) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, "") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + time.Sleep(retryDelay / 2) + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, holds, 1) + + // Due about 0.6 retry delays from now; a retry armed for a whole retry delay after this + // load would come one retry delay from now. + assert.Empty(t, f.requireAppliedWithin(t, retryDelay*85/100).Flags) +} + func TestReloaderRetryHoldsBackAnEmptiedFile(t *testing.T) { // A retry left over from a failure can run while the file is being rewritten in place. It // holds the file back too, rather than applying it empty. @@ -725,101 +745,134 @@ func TestReloaderParsesAnEmptiedFileRightAwayWithoutRetries(t *testing.T) { assert.Empty(t, f.requireAppliedAlready(t).Flags) } -func TestReloaderHoldsBackAnEmptiedFileForTheWholeDelay(t *testing.T) { - // Any load can find the file empty while it is rewritten: a change signal for another - // file, a touch, a poll. Every load within the delay holds it back. The delay counts as - // over at nine tenths of the retry delay, so that the retry that the hold arms parses a - // file that stayed empty. - f, mockLog, clock := newFakeClockFixture(t, nil) +// requireLoadHolds runs a load synchronously and checks that it held back the given number +// of files and neither applied nor reported anything. It returns the number of files held +// back so far. +func (f *reloaderFixture) requireLoadHolds(t *testing.T, mockLog *ldlogtest.MockLog, before int, files int) int { + t.Helper() f.reloader.ReloadNow() - f.requireApplied(t) + require.Equal(t, before+files, holdCount(mockLog), "the load did not hold back %d file(s)", files) + f.requireNoCallback(t) + return before + files +} - f.write(t, "") - f.reloader.Trigger() - holds := f.requireHeldBack(t, mockLog, 0, 1) +// requireNoCallback checks that no Apply or OnError call has happened. +func (f *reloaderFixture) requireNoCallback(t *testing.T) { + t.Helper() + select { + case <-f.applied: + require.FailNow(t, "unexpected Apply call") + case <-f.errored: + require.FailNow(t, "unexpected OnError call") + default: + } +} - clock.Advance(fakeClockRetryDelay * 85 / 100) - f.reloader.Trigger() - holds = f.requireHeldBack(t, mockLog, holds, 1) +// requireLoadApplies runs a load synchronously and returns what it applied. +func (f *reloaderFixture) requireLoadApplies(t *testing.T) MergeResult { + t.Helper() + f.reloader.ReloadNow() + return f.requireAppliedAlready(t) +} - clock.Advance(fakeClockRetryDelay * 10 / 100) - f.reloader.Trigger() - assert.Empty(t, f.requireApplied(t).Flags) - assert.Equal(t, holds, holdCount(mockLog)) +func TestReloaderHoldsBackAnEmptiedFileForExactlyTheDelay(t *testing.T) { + // Any load can find the file empty while it is rewritten: a change signal for another + // file, a touch, a poll. Every load until the file has stayed empty for the retry delay + // holds it back. + f, mockLog, clock := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + clock.Advance(fakeClockRetryDelay * 95 / 100) + f.requireLoadHolds(t, mockLog, holds, 1) + clock.Advance(fakeClockRetryDelay * 5 / 100) + assert.Empty(t, f.requireLoadApplies(t).Flags) } func TestReloaderDoesNotHoldBackAFileWhoseEmptyStateIsInEffect(t *testing.T) { f, mockLog, clock := newFakeClockFixture(t, nil) - f.reloader.ReloadNow() - f.requireApplied(t) + f.requireLoadApplies(t) f.write(t, "") - f.reloader.Trigger() - holds := f.requireHeldBack(t, mockLog, 0, 1) + holds := f.requireLoadHolds(t, mockLog, 0, 1) clock.Advance(fakeClockRetryDelay) - f.reloader.Trigger() - assert.Empty(t, f.requireApplied(t).Flags) + assert.Empty(t, f.requireLoadApplies(t).Flags) // The file is written again, and still empty. Its empty state is in effect, so there is // nothing to keep. emptyInPlace(t, f.path) - f.reloader.Trigger() - assert.Empty(t, f.requireApplied(t).Flags) + assert.Empty(t, f.requireLoadApplies(t).Flags) assert.Equal(t, holds, holdCount(mockLog)) } +func TestReloaderHoldsBackAFileWithNoYAMLDocument(t *testing.T) { + // A read can see only the start of a YAML file that is being written, such as a comment + // or the document marker. It has no document, so it has no entries, like an empty file. + for name, partial := range map[string]string{"comment": "# overrides\n", "document marker": "---\n"} { + t.Run(name, func(t *testing.T) { + f, mockLog, _ := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + f.write(t, partial) + f.requireLoadHolds(t, mockLog, 0, 1) + f.write(t, yamlFlag2) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) + }) + } + t.Run("empty mapping", func(t *testing.T) { + // {} is a document with no entries, the way to remove a file's entries. It is applied + // right away. + f, mockLog, _ := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + f.write(t, "{}") + assert.Empty(t, f.requireLoadApplies(t).Flags) + assert.Equal(t, 0, holdCount(mockLog)) + }) +} + func TestReloaderRestartsTheDelayWhenAnEmptiedFileIsWrittenAgain(t *testing.T) { // Two quick rewrites in place: the second truncation comes before any load reads the // content of the first. The file was written since a load last found it empty, so the // delay starts over. f, mockLog, clock := newFakeClockFixture(t, nil) - f.reloader.ReloadNow() - f.requireApplied(t) + f.requireLoadApplies(t) f.write(t, "") - f.reloader.Trigger() - holds := f.requireHeldBack(t, mockLog, 0, 1) + holds := f.requireLoadHolds(t, mockLog, 0, 1) f.write(t, yamlFlag2) emptyInPlace(t, f.path) clock.Advance(fakeClockRetryDelay * 8 / 10) - f.reloader.Trigger() - holds = f.requireHeldBack(t, mockLog, holds, 1) + holds = f.requireLoadHolds(t, mockLog, holds, 1) // Longer than the delay since a load first found the file empty, shorter since it was // written. clock.Advance(fakeClockRetryDelay * 8 / 10) - f.reloader.Trigger() - f.requireHeldBack(t, mockLog, holds, 1) + f.requireLoadHolds(t, mockLog, holds, 1) f.write(t, yamlFlag3) - f.reloader.Trigger() - assert.Equal(t, []string{"flag3"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(t))) } -func TestReloaderAppliesAnEmptiedFileThatKeepsBeingWrittenAfterFiveDelays(t *testing.T) { +func TestReloaderStopsHoldingBackAFileThatKeepsBeingWrittenAfterFiveDelays(t *testing.T) { // A file that is written again before every load, so that every load finds it empty, is // not held back for ever. f, mockLog, clock := newFakeClockFixture(t, nil) - f.reloader.ReloadNow() - f.requireApplied(t) + f.requireLoadApplies(t) f.write(t, "") - f.reloader.Trigger() - holds := f.requireHeldBack(t, mockLog, 0, 1) + holds := f.requireLoadHolds(t, mockLog, 0, 1) for range 6 { f.write(t, yamlFlag2) emptyInPlace(t, f.path) clock.Advance(fakeClockRetryDelay * 8 / 10) - f.reloader.Trigger() - holds = f.requireHeldBack(t, mockLog, holds, 1) + holds = f.requireLoadHolds(t, mockLog, holds, 1) } - // Five retry delays after a load first found the file empty, it is applied. + // Five retry delays after the first load that held it back, it is applied. f.write(t, yamlFlag2) emptyInPlace(t, f.path) clock.Advance(fakeClockRetryDelay * 8 / 10) - f.reloader.Trigger() - assert.Empty(t, f.requireApplied(t).Flags) + assert.Empty(t, f.requireLoadApplies(t).Flags) } // newTwoFileFakeClockFixture configures a fake clock reloader for two YAML files, the @@ -831,27 +884,47 @@ func newTwoFileFakeClockFixture(t *testing.T) (*reloaderFixture, string, *ldlogt f, mockLog, clock := newFakeClockFixture(t, func(cfg *ReloaderConfig) { cfg.Paths = append(cfg.Paths, second) }) - f.reloader.ReloadNow() - require.Equal(t, []string{"flag1", "flag2"}, flagKeys(f.requireApplied(t))) + require.Equal(t, []string{"flag1", "flag2"}, flagKeys(f.requireLoadApplies(t))) return f, second, mockLog, clock } +func TestReloaderStopsHoldingBackFilesThatAreRewrittenInTurnAfterFiveDelays(t *testing.T) { + // The two files are rewritten in place in turn, so that every load finds one of them + // empty. Each of them is read with content in between, but the run of loads that hold + // files back is still bounded. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + emptied, written := second, f.path + for range 6 { + require.NoError(t, os.WriteFile(written, []byte(yamlFlag3), 0600)) + emptyInPlace(t, emptied) + clock.Advance(fakeClockRetryDelay * 8 / 10) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + emptied, written = written, emptied + } + + require.NoError(t, os.WriteFile(written, []byte(yamlFlag3), 0600)) + emptyInPlace(t, emptied) + clock.Advance(fakeClockRetryDelay * 8 / 10) + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(t))) +} + func TestReloaderHoldsBackAnEmptiedFileWhenAnotherFileChanges(t *testing.T) { // The second file is rewritten in place. A change to the first file starts a load, and - // later another one, while the second file is still empty. + // later another one, while the second file is still empty. A load applies all of the + // files or none, so the change to the first file waits too. f, second, mockLog, clock := newTwoFileFakeClockFixture(t) require.NoError(t, os.WriteFile(second, nil, 0600)) - f.reloader.Trigger() - holds := f.requireHeldBack(t, mockLog, 0, 1) + holds := f.requireLoadHolds(t, mockLog, 0, 1) clock.Advance(fakeClockRetryDelay * 3 / 10) f.write(t, yamlFlag3) - f.reloader.Trigger() - f.requireHeldBack(t, mockLog, holds, 1) + f.requireLoadHolds(t, mockLog, holds, 1) require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) - f.reloader.Trigger() - assert.Equal(t, []string{"flag2", "flag3"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, []string{"flag2", "flag3"}, flagKeys(f.requireLoadApplies(t))) } func TestReloaderStartsTheDelayOfEveryEmptiedFileInOneLoad(t *testing.T) { @@ -860,40 +933,53 @@ func TestReloaderStartsTheDelayOfEveryEmptiedFileInOneLoad(t *testing.T) { f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") require.NoError(t, os.WriteFile(second, nil, 0600)) - f.reloader.Trigger() - f.requireHeldBack(t, mockLog, 0, 2) + f.requireLoadHolds(t, mockLog, 0, 2) - clock.Advance(fakeClockRetryDelay * 95 / 100) - f.reloader.Trigger() - assert.Empty(t, f.requireApplied(t).Flags) + clock.Advance(fakeClockRetryDelay) + assert.Empty(t, f.requireLoadApplies(t).Flags) } -func TestReloaderStartsANewDelayForEachEmptying(t *testing.T) { +func TestReloaderStartsANewDelayAndRunForEachEmptying(t *testing.T) { // The file is rewritten in place twice, a while apart, and the second time a poll reads it - // empty twice. The second emptying gets a delay of its own: the time since the first one - // does not count towards it. + // empty twice. The load that applies the first rewrite ends the run of loads that hold + // files back, so the time since the first emptying does not count against the second. f, mockLog, clock := newFakeClockFixture(t, nil) - f.reloader.ReloadNow() - f.requireApplied(t) + f.requireLoadApplies(t) f.write(t, "") - f.reloader.Trigger() - holds := f.requireHeldBack(t, mockLog, 0, 1) + holds := f.requireLoadHolds(t, mockLog, 0, 1) f.write(t, yamlFlag2) - f.reloader.Trigger() - assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) clock.Advance(fakeClockRetryDelay * 49 / 10) emptyInPlace(t, f.path) - f.reloader.Trigger() - holds = f.requireHeldBack(t, mockLog, holds, 1) + holds = f.requireLoadHolds(t, mockLog, holds, 1) clock.Advance(fakeClockRetryDelay * 3 / 10) - f.reloader.Trigger() - f.requireHeldBack(t, mockLog, holds, 1) + f.requireLoadHolds(t, mockLog, holds, 1) f.write(t, yamlFlag3) - f.reloader.Trigger() - assert.Equal(t, []string{"flag3"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(t))) +} + +func TestReloaderEndsAnEmptyingWhenTheFileIsReadWithContent(t *testing.T) { + // The file is emptied, then written and applied, then emptied again with the same + // modification time and size as at the first emptying, as a file system with coarse + // timestamps can record. The second emptying still gets a delay of its own. + f, mockLog, clock := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + f.write(t, "") + info, err := os.Stat(f.path) + require.NoError(t, err) + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + clock.Advance(fakeClockRetryDelay / 2) + f.write(t, yamlFlag2) + f.requireLoadApplies(t) + + clock.Advance(fakeClockRetryDelay * 6 / 10) + f.write(t, "") + require.NoError(t, os.Chtimes(f.path, info.ModTime(), info.ModTime())) + f.requireLoadHolds(t, mockLog, holds, 1) } func TestReloaderKeepsParsingAnEmptiedFileAfterItsDelay(t *testing.T) { @@ -902,19 +988,83 @@ func TestReloaderKeepsParsingAnEmptiedFileAfterItsDelay(t *testing.T) { f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) - f.reloader.Trigger() + f.reloader.ReloadNow() require.ErrorContains(t, f.requireErrored(t), second) // After the delay, loads parse the first file, as long as it is not written. - clock.Advance(fakeClockRetryDelay * 95 / 100) - f.reloader.Trigger() + clock.Advance(fakeClockRetryDelay) + f.reloader.ReloadNow() requireLogged(t, mockLog, ldlog.Debug, "Unable to load flags") require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) - f.reloader.Trigger() - assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) assert.Equal(t, 0, holdCount(mockLog)) } +func TestReloaderHoldsBackALongEmptyFileAgainWhenItIsWritten(t *testing.T) { + // The first file has been empty for longer than the delay while loads failed on the + // second. When it is written again, still empty, its delay starts over. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.reloader.ReloadNow() + f.requireErrored(t) + clock.Advance(fakeClockRetryDelay * 3) + f.reloader.ReloadNow() + + emptyInPlace(t, f.path) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.requireLoadHolds(t, mockLog, 0, 1) + f.write(t, yamlFlag3) + assert.Equal(t, []string{"flag2", "flag3"}, flagKeys(f.requireLoadApplies(t))) +} + +func TestReloaderStartsANewRunOfHoldsAfterAFailedLoad(t *testing.T) { + // A load that fails holds nothing back, so it ends the run of loads that hold files back. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + clock.Advance(fakeClockRetryDelay * 3) + f.write(t, yamlFlag1) + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.reloader.ReloadNow() + f.requireErrored(t) + + clock.Advance(fakeClockRetryDelay * 3) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + emptyInPlace(t, f.path) + f.requireLoadHolds(t, mockLog, holds, 1) +} + +func TestReloaderArmsTheRetryOfAHoldForTheTimeRemaining(t *testing.T) { + // A load that holds files back asks for a retry when the first of them has stayed empty + // for the retry delay, or when the run of loads that hold files back reaches its bound, + // plus a tenth of the retry delay. A failure asks for the retry delay. + const delay = fakeClockRetryDelay + f, _, clock := newFakeClockFixture(t, nil) + require.Equal(t, time.Duration(0), f.reloader.reload()) + f.requireAppliedAlready(t) + + f.write(t, "") + assert.Equal(t, delay+delay/10, f.reloader.reload()) + clock.Advance(delay * 6 / 10) + assert.Equal(t, delay*4/10+delay/10, f.reloader.reload()) + + // Written again before every load: the delay starts over each time, until the run's bound + // comes first. + for range 4 { + emptyInPlace(t, f.path) + assert.Equal(t, delay+delay/10, f.reloader.reload()) + clock.Advance(delay) + } + // 4.6 retry delays into the run. + emptyInPlace(t, f.path) + assert.Equal(t, delay*4/10+delay/10, f.reloader.reload()) + + f.write(t, "flagValues: [") + assert.Equal(t, delay, f.reloader.reload()) +} + func TestReloaderHoldsBackAFileWhoseContentWasMovedFromAnotherFile(t *testing.T) { // The content of one configured file is moved to the other (mv second.yaml data.yaml). The // files' combined content is the same, so the load is skipped as unchanged, but the data in @@ -929,19 +1079,16 @@ func TestReloaderHoldsBackAFileWhoseContentWasMovedFromAnotherFile(t *testing.T) }) require.NoError(t, os.Remove(f.path)) require.NoError(t, os.WriteFile(second, []byte(yamlFlag1), 0600)) - f.reloader.ReloadNow() - require.Equal(t, []string{"flag1"}, flagKeys(f.requireApplied(t))) + require.Equal(t, []string{"flag1"}, flagKeys(f.requireLoadApplies(t))) require.NoError(t, os.Rename(second, f.path)) - f.reloader.Trigger() - requireLogged(t, mockLog, ldlog.Debug, regexp.QuoteMeta(second)+" does not exist") - f.requireQuiet(t, 50*time.Millisecond) + f.reloader.ReloadNow() + require.True(t, mockLog.HasMessageMatch(ldlog.Debug, regexp.QuoteMeta(second)+" does not exist")) + f.requireNoCallback(t) // The file is rewritten in place: truncated, then written after a load has found it empty. f.write(t, "") - f.reloader.Trigger() - f.requireHeldBack(t, mockLog, 0, 1) + f.requireLoadHolds(t, mockLog, 0, 1) f.write(t, yamlFlag2) - f.reloader.Trigger() - assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) } diff --git a/ldfiledata/package_info.go b/ldfiledata/package_info.go index 3723dd34..602399b2 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -27,16 +27,19 @@ // // With a reloader, a file can be rewritten in place, so a reload can read it before it has // been fully written. Content that cannot be parsed is retried about a second later. A file -// whose flags are in effect and that is found empty is not applied until it has stayed -// empty, without being written, for about a second, however often the data source reloads -// in that time. A rewrite that leaves the file empty for less than nine tenths of a second -// therefore keeps the flags in effect throughout. A file that is written so often that -// every reload finds it empty is applied about five seconds after a reload first found it -// empty. A file that stays empty is a YAML document with no flags (see the format below). -// Without a reloader, the files are loaded once, with no retry. YAML that is cut off can -// still parse, as a part of its entries or with its last value cut short, so for a YAML -// file, replacing its entire contents in one operation, such as renaming a fully written -// temporary file over it, avoids briefly loading only a part of it. +// whose content is in effect and that a reload finds empty, or holding only a YAML comment +// or "---", is held back: the flags in effect stay until the file has stayed that way, +// without being written, for about a second, or at most about five seconds while files keep +// being rewritten. A rewrite that leaves the file empty for less than a second therefore +// keeps the flags in effect throughout. A reload applies all of the files or none, so +// changes to the other files wait while a file is held back. A file that stays empty is a +// YAML document with no flags, whatever its name (see the format below), and is applied +// once it is no longer held back. JSON that is cut off fails to parse, so the flags in +// effect stay until the file is complete. Without a reloader, the files are loaded once, +// with no retry. YAML that is cut off can still parse, as a part of its entries or with its +// last value cut short, so for a YAML file, replacing its entire contents in one operation, +// such as renaming a fully written temporary file over it, avoids briefly loading only a +// part of it. // // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three diff --git a/ldfiledatav2/package_info.go b/ldfiledatav2/package_info.go index d933454c..2c4ddbdc 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -27,16 +27,19 @@ // // With a reloader, a file can be rewritten in place, so a reload can read it before it has // been fully written. Content that cannot be parsed is retried about a second later. A file -// whose flags are in effect and that is found empty is not applied until it has stayed -// empty, without being written, for about a second, however often the data source reloads -// in that time. A rewrite that leaves the file empty for less than nine tenths of a second -// therefore keeps the flags in effect throughout. A file that is written so often that -// every reload finds it empty is applied about five seconds after a reload first found it -// empty. A file that stays empty is a YAML document with no flags (see the format below). -// Without a reloader, the files are loaded once, with no retry. YAML that is cut off can -// still parse, as a part of its entries or with its last value cut short, so for a YAML -// file, replacing its entire contents in one operation, such as renaming a fully written -// temporary file over it, avoids briefly loading only a part of it. +// whose content is in effect and that a reload finds empty, or holding only a YAML comment +// or "---", is held back: the flags in effect stay until the file has stayed that way, +// without being written, for about a second, or at most about five seconds while files keep +// being rewritten. A rewrite that leaves the file empty for less than a second therefore +// keeps the flags in effect throughout. A reload applies all of the files or none, so +// changes to the other files wait while a file is held back. A file that stays empty is a +// YAML document with no flags, whatever its name (see the format below), and is applied +// once it is no longer held back. JSON that is cut off fails to parse, so the flags in +// effect stay until the file is complete. Without a reloader, the files are loaded once, +// with no retry. YAML that is cut off can still parse, as a part of its entries or with its +// last value cut short, so for a YAML file, replacing its entire contents in one operation, +// such as renaming a fully written temporary file over it, avoids briefly loading only a +// part of it. // // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three diff --git a/ldoverrides/file_source_builder.go b/ldoverrides/file_source_builder.go index f5e23dd4..05e50c74 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -83,19 +83,21 @@ type FileSourceBuilder struct { // delay, and recovers on its own once the file is readable again. // // 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 file whose overrides are in effect and that is -// found empty may be in the middle of a rewrite, so the source keeps its overrides in -// effect until the file has stayed empty, without being written, for about a second, -// however often it reloads in that time. A rewrite that leaves the file empty for less than -// nine tenths of a second therefore never removes them. A file that is written so often -// that every reload finds it empty is applied about five seconds after a reload first found -// it empty. The source tells the formats apart by the content: a file whose first character -// other than white space is "{" is JSON, and any other file is YAML. An empty file is -// therefore a YAML document with no entries, whatever its name, and a file that stays empty -// removes its overrides about a second after it was emptied. YAML that is cut off can still -// parse, as a part of its entries or with its last value cut short, 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. +// document with no entries, such as {}. A file whose content is in effect and that a reload +// finds empty, or holding only a YAML comment or "---", may be in the middle of a rewrite, +// so the source holds it back: the overrides in effect stay until the file has stayed that +// way, without being written, for about a second, or at most about five seconds while files +// keep being rewritten. A rewrite that leaves the file empty for less than a second +// therefore never removes them. A reload applies all of the files or none, so changes to +// the other files wait while a file is held back. The source tells the formats apart by the +// content: a file whose first character other than white space is "{" is JSON, and any +// other file is YAML. An empty file is therefore a YAML document with no entries, whatever +// its name, and a file that stays empty, even one named .json, removes its overrides once +// it is no longer held back. JSON that is cut off fails to parse, so the overrides in +// effect stay until the file is complete. YAML that is cut off can still parse, as a part +// of its entries or with its last value cut short, 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. // // Whenever the set of overrides in effect changes, including at startup, the source logs // the overrides in effect and what each configured file supplied, at Info level. From b8f6d9a2a8715986d9fd789720ddebfcb2e71bf6 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:45:02 +0000 Subject: [PATCH 05/10] test: Give each emptying in the reloader tests a later modification time --- internal/filedata/reloader_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 773e2414..3f2a489b 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -8,6 +8,7 @@ import ( "slices" "strings" "sync" + "sync/atomic" "testing" "time" @@ -580,22 +581,21 @@ func (f *reloaderFixture) requireHeldBack(t *testing.T, mockLog *ldlogtest.MockL } // emptyInPlace truncates the file without removing it, as a writer that rewrites it in place -// does first. It also moves the file's modification time a second later, so that the write is -// visible to the reloader even on a file system that records the time coarsely. +// does first. It also moves the file's modification time later than any that it set before, +// so that the write is visible to the reloader even where the file system records the time +// coarsely, or the clock only advances every few milliseconds, as on Windows. func emptyInPlace(t *testing.T, path string) { t.Helper() require.NoError(t, os.WriteFile(path, nil, 0600)) - moveModTimeLater(t, path) -} - -func moveModTimeLater(t *testing.T, path string) { - t.Helper() info, err := os.Stat(path) require.NoError(t, err) - later := info.ModTime().Add(time.Second) + later := info.ModTime().Add(time.Duration(modTimeShifts.Add(1)) * time.Second) require.NoError(t, os.Chtimes(path, later, later)) } +// modTimeShifts counts the calls of emptyInPlace, so that each moves the time further. +var modTimeShifts atomic.Int64 + func TestReloaderHoldsBackAnEmptiedFileUntilItIsRewritten(t *testing.T) { // A writer that rewrites the file in place truncates it and then writes the new content. // A load that finds the file empty after its content was applied keeps the data. An empty From db8e4d8c56e2950839a98a2b4b293955f7f97830 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 03:35:09 +0000 Subject: [PATCH 06/10] fix: Bound the hold of each emptied data file on its own, and read the clock per file The bound on a run of holding loads was shared and was restarted by any load that held nothing back. A failed load of another file restarted it, so a file kept written while empty was held forever, and one file reaching the bound cut short another file's ordinary rewrite. Each file now has its own bound from when a load first held it back. A read with content or a failed load keeps it; applying data, or the file staying empty for the whole delay, ends it. The clock is read after each file's own read, and again for the hold's retry, so a slow read of another file does not make a file's emptiness look older than it is. The retry after a hold logs that it reads the files again because a file was found empty. --- internal/filedata/reloader.go | 159 +++++++++++--------- internal/filedata/reloader_test.go | 232 ++++++++++++++++++++++++----- ldfiledata/package_info.go | 14 +- ldfiledatav2/package_info.go | 14 +- ldoverrides/file_source_builder.go | 4 +- 5 files changed, 304 insertions(+), 119 deletions(-) diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index a0ab2491..911e81c0 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -7,6 +7,7 @@ import ( "io/fs" "os" "sync" + "sync/atomic" "time" "github.com/launchdarkly/go-sdk-common/v3/ldlog" @@ -58,9 +59,9 @@ type ReloaderConfig struct { // so that a failure observed while a file was being rewritten recovers even if no // further change notification arrives. A file whose content is in effect and that is // found with no entries is not applied until it has stayed so, without being written, - // for the same delay, or for at most maxHoldRetryDelays delays while loads keep holding - // files back. If zero or negative, there is no automatic retry, and such a file is - // parsed as it is. + // for the same delay, or for at most maxHoldRetryDelays delays while it keeps being + // written. If zero or negative, there is no automatic retry, and such a file is parsed as + // it is. RetryDelay time.Duration // SkipUnchanged, if true, suppresses the Apply call when the files' raw contents are // byte-identical to the last successfully applied contents. @@ -90,29 +91,35 @@ type Reloader struct { // unchanged. A missing or empty file contributed none. Guarded by reloadMu. appliedHadContent map[string]bool // emptyFiles holds the paths whose content is in effect and that loads have found with no - // entries since. An entry is removed when a load reads the file with entries. See - // holdBack. Guarded by reloadMu. + // entries since the data in effect was applied. Applying data clears it. See holdBack. + // Guarded by reloadMu. emptyFiles map[string]*emptyFile - // holding is true while the loads hold files back, from the first load of such a run, - // at holdingSince, until a load holds none back. heldBack is set by a load that holds - // files back. Guarded by reloadMu. - holding bool - holdingSince time.Time - heldBack bool - // now returns the current time. Tests replace it. - now func() time.Time + // heldBack is true from a load that held files back until the next load starts, so that + // the retry it armed can say why it reads the files again. Guarded by reloadMu. + heldBack bool + // now returns the current time, and readFile reads a file. Tests replace them. + now func() time.Time + readFile func(string) ([]byte, error) + // lastArmedRetryDelay is the delay of the retry that the run goroutine armed last, in + // nanoseconds, for tests. + lastArmedRetryDelay atomic.Int64 } -// emptyFile records since when a file has had no entries without being written, and the -// file's state then. +// emptyFile records what loads found of a file whose content is in effect and that they found +// with no entries. type emptyFile struct { + // since is when a load found the file with no entries after a read with entries or a + // write, and state is the file's state then. since is zero after a read with entries. since time.Time state fileState + // firstHeld is when a load first held the file back since the data in effect was applied, + // or zero when the file has not been held back since, or stayed empty for the whole delay. + firstHeld time.Time } -// maxHoldRetryDelays bounds a run of loads that hold files back. A file that keeps being -// written so that every load finds it with no entries, or files that are rewritten in turn -// so that every load finds one of them so, would otherwise be held back without end. +// maxHoldRetryDelays bounds how long a file is held back. A file that keeps being written so +// that every load finds it with no entries, or files that are rewritten in turn, would +// otherwise be held back without end. const maxHoldRetryDelays = 5 // NewReloader creates a Reloader. The caller should perform the initial load with @@ -131,6 +138,7 @@ func NewReloader(cfg ReloaderConfig) *Reloader { appliedHadContent: make(map[string]bool, len(cfg.Paths)), emptyFiles: make(map[string]*emptyFile), now: time.Now, + readFile: os.ReadFile, } } @@ -151,7 +159,7 @@ func (r *Reloader) ensureStarted() { // failed triggered reload. func (r *Reloader) ReloadNow() { r.ensureStarted() - if retryAfter := r.reload(); retryAfter > 0 { + if retryAfter := r.reload(false); retryAfter > 0 { select { case r.retryRequestCh <- retryAfter: default: @@ -195,19 +203,22 @@ func (r *Reloader) run() { stopTimer(retryTimer) }() + armRetry := func(retryAfter time.Duration) { + retryTimer = time.NewTimer(retryAfter) + retryC = retryTimer.C + r.lastArmedRetryDelay.Store(int64(retryAfter)) + } + reloadAndMaybeRetry := func(isRetry bool) { - if isRetry { - r.cfg.Loggers.Debug("Retrying flag data load after earlier failure") - } else { + if !isRetry { r.cfg.Loggers.Info("Reloading flag data after detecting a change") } // A pending retry is superseded by this reload: it either succeeded, or it failed // and arms a fresh retry below. stopTimer(retryTimer) retryTimer, retryC = nil, nil - if retryAfter := r.reload(); retryAfter > 0 { - retryTimer = time.NewTimer(retryAfter) - retryC = retryTimer.C + if retryAfter := r.reload(isRetry); retryAfter > 0 { + armRetry(retryAfter) } } @@ -235,8 +246,7 @@ func (r *Reloader) run() { // A synchronous ReloadNow failed or held a file back; arm the retry timer without // reloading again immediately. An already-armed retry keeps its deadline. if retryTimer == nil { - retryTimer = time.NewTimer(retryAfter) - retryC = retryTimer.C + armRetry(retryAfter) } case <-debounceC: debounceTimer, debounceC = nil, nil @@ -254,7 +264,7 @@ func (r *Reloader) run() { // applied, and a skipped no-op application counts as done. The whole set is re-read on every // reload: entries are combined across files in order, so a change to one file can alter // which file wins for a key. -func (r *Reloader) reload() time.Duration { +func (r *Reloader) reload(isRetry bool) time.Duration { r.reloadMu.Lock() defer r.reloadMu.Unlock() @@ -264,16 +274,15 @@ func (r *Reloader) reload() time.Duration { return 0 } - r.heldBack = false - defer func() { - // A run of loads that hold files back ends with any load that does not, whether it - // succeeded or failed. - if !r.heldBack { - r.holding = false + if isRetry { + if r.heldBack { + r.cfg.Loggers.Debug("Reading file data again after a file was found empty") + } else { + r.cfg.Loggers.Debug("Retrying flag data load after earlier failure") } - }() + } + r.heldBack = false - now := r.now() docs := make([]Document, 0, len(r.cfg.Paths)) files := make([]FileSummary, 0, len(r.cfg.Paths)) hadContent := make(map[string]bool, len(r.cfg.Paths)) @@ -284,7 +293,7 @@ func (r *Reloader) reload() time.Duration { for _, path := range r.cfg.Paths { // One read feeds both the hash and the parse, so the skip-unchanged hash can never // disagree with the content that was actually applied. - rawData, err := os.ReadFile(path) //nolint:gosec // G304: ok to read file into variable + rawData, err := r.readFile(path) if err != nil { if r.cfg.SkipMissingPaths && errors.Is(err, fs.ErrNotExist) { r.cfg.Loggers.Debugf("File %s does not exist; it contributes no data", path) @@ -308,8 +317,12 @@ func (r *Reloader) reload() time.Duration { parsed = true } if hasContent { - delete(r.emptyFiles, path) - } else if r.holdBack(path, now) { + if empty, ok := r.emptyFiles[path]; ok { + empty.since = time.Time{} + } + } else if r.holdBack(path, r.now()) { + // The clock is read after this file's own read, so a slow read of another file + // does not make its emptiness look older than it is. heldBack = append(heldBack, path) continue } @@ -326,7 +339,7 @@ func (r *Reloader) reload() time.Duration { files = append(files, FileSummary{Path: path, Present: true}) } if len(heldBack) > 0 { - return r.readAgainLater(heldBack, now) + return r.readAgainLater(heldBack, r.now()) } merged, err := Merge(r.cfg.DuplicateKeysHandling, r.cfg.MakeFlagWithValue, docs...) @@ -360,10 +373,11 @@ func (r *Reloader) reload() time.Duration { r.lastErrorMsg = "" hash := hasher.Sum(nil) // The data of this load is in effect from here on, whether it is applied or is the same - // as the data already applied. + // as the data already applied, so the bounds of the files that were held back start over. for path, had := range hadContent { r.appliedHadContent[path] = had } + clear(r.emptyFiles) if r.cfg.SkipUnchanged && !recovering && bytes.Equal(hash, r.lastGoodHash) { return 0 } @@ -380,13 +394,15 @@ func (r *Reloader) reload() time.Duration { // held back until it has stayed so, without being written, for the retry delay, however many // loads find it so in that time. A change of its state, its modification time and size read // through any symbolic link, means that it was written since a load last found it so, for -// example rewritten and truncated again between two loads, so the delay starts over. Loads -// stop holding files back once they have held some back for maxHoldRetryDelays retry delays -// in a row, so that a file that keeps being written, or files that are rewritten in turn, -// are not held back without end. A file whose content is not in effect, such as a -// placeholder that has always been empty or an emptied file whose empty state has been -// applied, is parsed right away, and so is any file when retries are off, since nothing -// would read it again. +// example rewritten and truncated again between two loads, so the delay starts over; so does +// a read with entries. A file is held back for at most maxHoldRetryDelays retry delays from +// when a load first held it back, so that a file that keeps being written while every load +// finds it empty, or files that are rewritten in turn, are not held back without end. The +// bound is the file's own: a read with entries or a failed load does not restart it, and +// applying data, or the file staying empty for the whole delay, ends it. A file whose content +// is not in effect, such as a placeholder that has always been empty or an emptied file whose +// empty state has been applied, is parsed right away, and so is any file when retries are +// off, since nothing would read it again. func (r *Reloader) holdBack(path string, now time.Time) bool { if r.cfg.RetryDelay <= 0 || !r.appliedHadContent[path] { return false @@ -394,17 +410,25 @@ func (r *Reloader) holdBack(path string, now time.Time) bool { state := observe(path) empty, ok := r.emptyFiles[path] if !ok { - empty = &emptyFile{since: now, state: state} + empty = &emptyFile{} r.emptyFiles[path] = empty - } else if !empty.state.equal(state) { + } + if empty.since.IsZero() || !empty.state.equal(state) { empty.since = now empty.state = state } - holdingSince := now - if r.holding { - holdingSince = r.holdingSince + if now.Sub(empty.since) >= r.cfg.RetryDelay { + // The file has stayed empty for the whole delay; it is no rewrite. + empty.firstHeld = time.Time{} + return false } - return now.Sub(empty.since) < r.cfg.RetryDelay && now.Sub(holdingSince) < r.maxHold() + if !empty.firstHeld.IsZero() && now.Sub(empty.firstHeld) >= r.maxHold() { + return false + } + if empty.firstHeld.IsZero() { + empty.firstHeld = now + } + return true } func (r *Reloader) maxHold() time.Duration { @@ -413,26 +437,27 @@ func (r *Reloader) maxHold() time.Duration { // readAgainLater handles the files that holdBack holds back. Whatever their content would // parse as, the load keeps the current data and arms the retry, without reporting a failure. -// The retry comes when the first of the files has stayed so for the retry delay, or when the -// loads have held files back for as long as they may, plus a tenth of the retry delay, so -// that it finds the delay over; the change signal for a write starts a load sooner. Once a -// file has stayed so for the delay, a load parses it: a file that is still empty, or holds -// only a YAML comment, is a YAML document with no entries. +// The retry comes when the first of the files has stayed so 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 it finds that +// time over; the change signal for a write starts a load sooner. Once a file has stayed so for +// the delay, a load parses it: a file that is still empty, or holds only a YAML comment, is a +// YAML document with no entries. func (r *Reloader) readAgainLater(paths []string, now time.Time) time.Duration { // Close may have happened while the files were being read; arm no retry in that case. if r.isClosed() { return 0 } - if !r.holding { - r.holding = true - r.holdingSince = now - } - until := r.holdingSince.Add(r.maxHold()) - for _, path := range paths { + var until time.Time + for i, path := range paths { r.cfg.Loggers.Debugf("File %s is empty, possibly while it is being rewritten; reading it again before applying it", path) - if delayOver := r.emptyFiles[path].since.Add(r.cfg.RetryDelay); delayOver.Before(until) { - until = delayOver + empty := r.emptyFiles[path] + over := empty.since.Add(r.cfg.RetryDelay) + if boundOver := empty.firstHeld.Add(r.maxHold()); boundOver.Before(over) { + over = boundOver + } + if i == 0 || over.Before(until) { + until = over } } r.heldBack = true diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 3f2a489b..d77c3548 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -632,6 +632,8 @@ func TestReloaderAppliesAnEmptiedFileThatStaysEmptyAtTheFirstRetry(t *testing.T) // being written, for the retry delay, so it parses it: a document with no entries. A // second retry would come one more retry delay later. assert.Empty(t, f.requireAppliedWithin(t, emptyFileRetryDelay+500*time.Millisecond).Flags) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, "Reading file data again after a file was found empty")) + assert.False(t, mockLog.HasMessageMatch(ldlog.Debug, "after earlier failure")) } func TestReloaderRetriesAHeldBackFileWhenItsDelayIsOver(t *testing.T) { @@ -667,8 +669,10 @@ func TestReloaderRetryHoldsBackAnEmptiedFile(t *testing.T) { // No change signal: only the retry reads the file. f.write(t, "") f.requireHeldBack(t, mockLog, 0, 1) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, "Retrying flag data load after earlier failure")) f.write(t, yamlFlag2) assert.Equal(t, []string{"flag2"}, flagKeys(f.requireApplied(t))) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, "Reading file data again after a file was found empty")) } func TestReloaderHoldingBackAFileReportsNoFailure(t *testing.T) { @@ -890,14 +894,15 @@ func newTwoFileFakeClockFixture(t *testing.T) (*reloaderFixture, string, *ldlogt func TestReloaderStopsHoldingBackFilesThatAreRewrittenInTurnAfterFiveDelays(t *testing.T) { // The two files are rewritten in place in turn, so that every load finds one of them - // empty. Each of them is read with content in between, but the run of loads that hold - // files back is still bounded. + // empty. Each of them is read with content in between, which does not restart its bound. + // The first file reaches its bound first, five retry delays after a load first held it + // back. f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") holds := f.requireLoadHolds(t, mockLog, 0, 1) emptied, written := second, f.path - for range 6 { + for range 7 { require.NoError(t, os.WriteFile(written, []byte(yamlFlag3), 0600)) emptyInPlace(t, emptied) clock.Advance(fakeClockRetryDelay * 8 / 10) @@ -939,10 +944,10 @@ func TestReloaderStartsTheDelayOfEveryEmptiedFileInOneLoad(t *testing.T) { assert.Empty(t, f.requireLoadApplies(t).Flags) } -func TestReloaderStartsANewDelayAndRunForEachEmptying(t *testing.T) { +func TestReloaderStartsANewDelayAndBoundForEachEmptying(t *testing.T) { // The file is rewritten in place twice, a while apart, and the second time a poll reads it - // empty twice. The load that applies the first rewrite ends the run of loads that hold - // files back, so the time since the first emptying does not count against the second. + // empty twice. The load that applies the first rewrite ends the file's bound, so the time + // since the first emptying does not count against the second. f, mockLog, clock := newFakeClockFixture(t, nil) f.requireLoadApplies(t) @@ -961,27 +966,59 @@ func TestReloaderStartsANewDelayAndRunForEachEmptying(t *testing.T) { assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(t))) } -func TestReloaderEndsAnEmptyingWhenTheFileIsReadWithContent(t *testing.T) { - // The file is emptied, then written and applied, then emptied again with the same - // modification time and size as at the first emptying, as a file system with coarse - // timestamps can record. The second emptying still gets a delay of its own. - f, mockLog, clock := newFakeClockFixture(t, nil) - f.requireLoadApplies(t) +func TestReloaderRestartsTheDelayWhenTheFileIsReadWithContent(t *testing.T) { + // The first file is emptied, then written while the second cannot be parsed, so nothing + // is applied. Then it is emptied again with the same modification time and size as at the + // first emptying, as a file system with coarse timestamps can record. The read with + // content still starts its delay over. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") info, err := os.Stat(f.path) require.NoError(t, err) holds := f.requireLoadHolds(t, mockLog, 0, 1) clock.Advance(fakeClockRetryDelay / 2) - f.write(t, yamlFlag2) - f.requireLoadApplies(t) + f.write(t, yamlFlag3) + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.reloader.ReloadNow() + f.requireErrored(t) clock.Advance(fakeClockRetryDelay * 6 / 10) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) f.write(t, "") require.NoError(t, os.Chtimes(f.path, info.ModTime(), info.ModTime())) f.requireLoadHolds(t, mockLog, holds, 1) } +func TestReloaderHoldsBackAFileEmptiedAgainAfterContentWhenItsStateLooksTheSame(t *testing.T) { + // On a file system 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 second file is kept written while empty, so that no load + // applies. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + info, err := os.Stat(f.path) + require.NoError(t, err) + require.NoError(t, os.WriteFile(second, nil, 0600)) + holds := f.requireLoadHolds(t, mockLog, 0, 2) + + clock.Advance(fakeClockRetryDelay * 50 / 60) + f.write(t, yamlFlag1) + emptyInPlace(t, second) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + + clock.Advance(fakeClockRetryDelay * 20 / 60) + f.write(t, "") + require.NoError(t, os.Chtimes(f.path, info.ModTime(), info.ModTime())) + emptyInPlace(t, second) + holds = f.requireLoadHolds(t, mockLog, holds, 2) + + // The second file gets its content back while the first is still within its new delay. + clock.Advance(fakeClockRetryDelay * 10 / 60) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.requireLoadHolds(t, mockLog, holds, 1) +} + func TestReloaderKeepsParsingAnEmptiedFileAfterItsDelay(t *testing.T) { // The first file is emptied while the second cannot be parsed. Holding back the first file // does not hide the failure of the second. @@ -1001,14 +1038,15 @@ func TestReloaderKeepsParsingAnEmptiedFileAfterItsDelay(t *testing.T) { } func TestReloaderHoldsBackALongEmptyFileAgainWhenItIsWritten(t *testing.T) { - // The first file has been empty for longer than the delay while loads failed on the - // second. When it is written again, still empty, its delay starts over. + // The first file has been empty for longer than the delay, and than its bound, while loads + // failed on the second. When it is written again, still empty, its delay and its bound + // start over: it stayed empty for the whole delay, so it was no rewrite. f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) f.reloader.ReloadNow() f.requireErrored(t) - clock.Advance(fakeClockRetryDelay * 3) + clock.Advance(fakeClockRetryDelay * 6) f.reloader.ReloadNow() emptyInPlace(t, f.path) @@ -1018,51 +1056,173 @@ func TestReloaderHoldsBackALongEmptyFileAgainWhenItIsWritten(t *testing.T) { assert.Equal(t, []string{"flag2", "flag3"}, flagKeys(f.requireLoadApplies(t))) } -func TestReloaderStartsANewRunOfHoldsAfterAFailedLoad(t *testing.T) { - // A load that fails holds nothing back, so it ends the run of loads that hold files back. +func TestReloaderFailedLoadsDoNotRestartTheBoundOfAFileThatIsHeldBack(t *testing.T) { + // The first file is written again before every load, so that every load finds it empty, + // and every other load fails on the second file. The failures do not restart the first + // file's bound. f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") holds := f.requireLoadHolds(t, mockLog, 0, 1) - clock.Advance(fakeClockRetryDelay * 3) - f.write(t, yamlFlag1) - require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) - f.reloader.ReloadNow() - f.requireErrored(t) + for i := range 7 { + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + if i%2 == 0 { + // The load fails. Only the first of these identical failures is reported. + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.reloader.ReloadNow() + require.Equal(t, holds, holdCount(mockLog)) + select { + case <-f.applied: + require.FailNow(t, "a failed load called Apply") + case <-f.errored: + default: + } + continue + } + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + } - clock.Advance(fakeClockRetryDelay * 3) + // 5.6 retry delays after the first file was first held back. require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) emptyInPlace(t, f.path) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) +} + +func TestReloaderAnotherFilesBoundDoesNotCutShortARewrite(t *testing.T) { + // The first file is written again before every load, so that every load finds it empty, + // until it reaches its bound. The second file is rewritten in place just before that, and + // is still held back for its own delay. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + for range 5 { + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + } + + // 4.8 retry delays after the first file was first held back, the second file is emptied. + emptyInPlace(t, f.path) + emptyInPlace(t, second) + clock.Advance(fakeClockRetryDelay * 8 / 10) + holds = f.requireLoadHolds(t, mockLog, holds, 2) + + // The first file's bound is over; the second file has been empty for 0.4 retry delays. + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 4 / 10) f.requireLoadHolds(t, mockLog, holds, 1) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) } -func TestReloaderArmsTheRetryOfAHoldForTheTimeRemaining(t *testing.T) { +func TestReloaderSlowReadOfAnotherFileDoesNotShortenTheDelay(t *testing.T) { + // The read of the first file takes most of a retry delay, and the second file is emptied + // meanwhile. Its delay starts when it is read, not when the load started. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + var slow atomic.Bool + f.reloader.readFile = func(path string) ([]byte, error) { + if path == f.path && slow.Load() { + clock.Advance(fakeClockRetryDelay * 9 / 10) + require.NoError(t, os.WriteFile(second, nil, 0600)) + } + return os.ReadFile(path) //nolint:gosec // test file path + } + slow.Store(true) + holds := f.requireLoadHolds(t, mockLog, 0, 1) + slow.Store(false) + + clock.Advance(fakeClockRetryDelay * 3 / 10) + f.requireLoadHolds(t, mockLog, holds, 1) +} + +func TestReloaderHoldAsksForTheRetryWhenTheFirstHeldFileIsDue(t *testing.T) { // A load that holds files back asks for a retry when the first of them has stayed empty - // for the retry delay, or when the run of loads that hold files back reaches its bound, - // plus a tenth of the retry delay. A failure asks for the retry delay. + // for the retry delay, or has been held back for as long as it may be, plus a tenth of the + // retry delay. A failure asks for the retry delay. const delay = fakeClockRetryDelay f, _, clock := newFakeClockFixture(t, nil) - require.Equal(t, time.Duration(0), f.reloader.reload()) + require.Equal(t, time.Duration(0), f.reloader.reload(false)) f.requireAppliedAlready(t) + // The first hold asks for the whole delay, a later one for the rest of it. f.write(t, "") - assert.Equal(t, delay+delay/10, f.reloader.reload()) + assert.Equal(t, delay+delay/10, f.reloader.reload(false)) clock.Advance(delay * 6 / 10) - assert.Equal(t, delay*4/10+delay/10, f.reloader.reload()) + assert.Equal(t, delay*4/10+delay/10, f.reloader.reload(false)) - // Written again before every load: the delay starts over each time, until the run's bound + // Written again before every load: the delay starts over each time, until the file's bound // comes first. for range 4 { emptyInPlace(t, f.path) - assert.Equal(t, delay+delay/10, f.reloader.reload()) + assert.Equal(t, delay+delay/10, f.reloader.reload(false)) clock.Advance(delay) } - // 4.6 retry delays into the run. + // 4.6 retry delays after the file was first held back. emptyInPlace(t, f.path) - assert.Equal(t, delay*4/10+delay/10, f.reloader.reload()) + assert.Equal(t, delay*4/10+delay/10, f.reloader.reload(false)) f.write(t, "flagValues: [") - assert.Equal(t, delay, f.reloader.reload()) + assert.Equal(t, delay, f.reloader.reload(false)) +} + +func TestReloaderHoldAsksForTheRetryOfTheFileThatIsDueFirst(t *testing.T) { + // The first file was emptied earlier than the second, so its delay is over first. + const delay = fakeClockRetryDelay + f, second, _, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + assert.Equal(t, delay+delay/10, f.reloader.reload(false)) + clock.Advance(delay * 3 / 10) + require.NoError(t, os.WriteFile(second, nil, 0600)) + assert.Equal(t, delay*7/10+delay/10, f.reloader.reload(false)) +} + +func TestReloaderHoldAsksForTheRetryFromTheEndOfTheLoad(t *testing.T) { + // The read of the second file takes half a retry delay. The retry is measured from the + // end of the load. + const delay = fakeClockRetryDelay + f, second, _, clock := newTwoFileFakeClockFixture(t) + f.reloader.readFile = func(path string) ([]byte, error) { + if path == second { + clock.Advance(delay / 2) + } + return os.ReadFile(path) //nolint:gosec // test file path + } + f.write(t, "") + assert.Equal(t, delay/2+delay/10, f.reloader.reload(false)) +} + +func TestReloaderArmsTheRetryThatTheHoldAsksFor(t *testing.T) { + // The retry is armed for what the hold asks for, whether the load came from a change + // signal or from ReloadNow. + const delay = fakeClockRetryDelay + requireArmed := func(t *testing.T, f *reloaderFixture, expected time.Duration) { + t.Helper() + require.Eventually(t, func() bool { return f.reloader.lastArmedRetryDelay.Load() == int64(expected) }, + testTimeout, 10*time.Millisecond, "the retry was not armed for %s", expected) + } + t.Run("change signal", func(t *testing.T) { + f, mockLog, clock := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + f.write(t, "") + f.reloader.Trigger() + holds := f.requireHeldBack(t, mockLog, 0, 1) + requireArmed(t, f, delay+delay/10) + clock.Advance(delay * 6 / 10) + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, holds, 1) + requireArmed(t, f, delay*4/10+delay/10) + }) + t.Run("ReloadNow", func(t *testing.T) { + f, mockLog, clock := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + clock.Advance(delay) + f.write(t, "") + clock.Advance(delay / 10) + f.requireLoadHolds(t, mockLog, 0, 1) + requireArmed(t, f, delay+delay/10) + }) } func TestReloaderHoldsBackAFileWhoseContentWasMovedFromAnotherFile(t *testing.T) { diff --git a/ldfiledata/package_info.go b/ldfiledata/package_info.go index 602399b2..1b78801a 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -29,13 +29,13 @@ // been fully written. Content that cannot be parsed is retried about a second later. A file // whose content is in effect and that a reload finds empty, or holding only a YAML comment // or "---", is held back: the flags in effect stay until the file has stayed that way, -// without being written, for about a second, or at most about five seconds while files keep -// being rewritten. A rewrite that leaves the file empty for less than a second therefore -// keeps the flags in effect throughout. A reload applies all of the files or none, so -// changes to the other files wait while a file is held back. A file that stays empty is a -// YAML document with no flags, whatever its name (see the format below), and is applied -// once it is no longer held back. JSON that is cut off fails to parse, so the flags in -// effect stay until the file is complete. Without a reloader, the files are loaded once, +// without being written, for about a second, or for at most about five seconds while it +// keeps being rewritten. A rewrite that leaves the file empty for less than a second +// therefore keeps the flags in effect throughout. A reload applies all of the files or +// none, so changes to the other files wait while a file is held back. A file that stays +// empty is a YAML document with no flags, whatever its name (see the format below), and is +// applied once it is no longer held back. JSON that is cut off fails to parse, so the flags +// in effect stay until the file is complete. Without a reloader, the files are loaded once, // with no retry. YAML that is cut off can still parse, as a part of its entries or with its // last value cut short, so for a YAML file, replacing its entire contents in one operation, // such as renaming a fully written temporary file over it, avoids briefly loading only a diff --git a/ldfiledatav2/package_info.go b/ldfiledatav2/package_info.go index 2c4ddbdc..f56bb0d9 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -29,13 +29,13 @@ // been fully written. Content that cannot be parsed is retried about a second later. A file // whose content is in effect and that a reload finds empty, or holding only a YAML comment // or "---", is held back: the flags in effect stay until the file has stayed that way, -// without being written, for about a second, or at most about five seconds while files keep -// being rewritten. A rewrite that leaves the file empty for less than a second therefore -// keeps the flags in effect throughout. A reload applies all of the files or none, so -// changes to the other files wait while a file is held back. A file that stays empty is a -// YAML document with no flags, whatever its name (see the format below), and is applied -// once it is no longer held back. JSON that is cut off fails to parse, so the flags in -// effect stay until the file is complete. Without a reloader, the files are loaded once, +// without being written, for about a second, or for at most about five seconds while it +// keeps being rewritten. A rewrite that leaves the file empty for less than a second +// therefore keeps the flags in effect throughout. A reload applies all of the files or +// none, so changes to the other files wait while a file is held back. A file that stays +// empty is a YAML document with no flags, whatever its name (see the format below), and is +// applied once it is no longer held back. JSON that is cut off fails to parse, so the flags +// in effect stay until the file is complete. Without a reloader, the files are loaded once, // with no retry. YAML that is cut off can still parse, as a part of its entries or with its // last value cut short, so for a YAML file, replacing its entire contents in one operation, // such as renaming a fully written temporary file over it, avoids briefly loading only a diff --git a/ldoverrides/file_source_builder.go b/ldoverrides/file_source_builder.go index 05e50c74..0638a7d3 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -86,8 +86,8 @@ type FileSourceBuilder struct { // document with no entries, such as {}. A file whose content is in effect and that a reload // finds empty, or holding only a YAML comment or "---", may be in the middle of a rewrite, // so the source holds it back: the overrides in effect stay until the file has stayed that -// way, without being written, for about a second, or at most about five seconds while files -// keep being rewritten. A rewrite that leaves the file empty for less than a second +// way, without being written, for about a second, or for at most about five seconds while +// it keeps being rewritten. A rewrite that leaves the file empty for less than a second // therefore never removes them. A reload applies all of the files or none, so changes to // the other files wait while a file is held back. The source tells the formats apart by the // content: a file whose first character other than white space is "{" is JSON, and any From d465f22524076cd39a21478442143696799cb81c Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 04:39:02 +0000 Subject: [PATCH 07/10] fix: Start a held file's bound over only at a load that holds nothing back A file that stayed empty for the whole delay cleared its own bound while another file kept every load holding, so two files kept empty and truncated again in turn were held back without end. A file held back during a long failure of another file kept its bound, so a later rewrite of it was applied empty once the other file was fixed. A file's bound now starts over only at a load that holds nothing back, for every file that load read, whether the load applies, is skipped or fails. A file that a failed load did not get to read keeps its bound. --- internal/filedata/reloader.go | 31 +++++--- internal/filedata/reloader_test.go | 121 ++++++++++++++++++++++++++--- 2 files changed, 131 insertions(+), 21 deletions(-) diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index 911e81c0..dd6aec83 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -112,8 +112,8 @@ type emptyFile struct { // write, and state is the file's state then. since is zero after a read with entries. since time.Time state fileState - // firstHeld is when a load first held the file back since the data in effect was applied, - // or zero when the file has not been held back since, or stayed empty for the whole delay. + // firstHeld is when a load first held the file back, or zero when the file has not been + // held back since a load that held nothing back read it. firstHeld time.Time } @@ -289,6 +289,18 @@ func (r *Reloader) reload(isRetry bool) time.Duration { // The files that holdBack holds back. The other files are still read, so that every file // that is found empty starts its delay in this load. var heldBack []string + defer func() { + // A load 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 back, so its next + // rewrite is a new one. A file that the load did not get to keeps its bound. + if len(heldBack) == 0 { + for path := range hadContent { + if empty, ok := r.emptyFiles[path]; ok { + empty.firstHeld = time.Time{} + } + } + } + }() hasher := sha256.New() for _, path := range r.cfg.Paths { // One read feeds both the hash and the parse, so the skip-unchanged hash can never @@ -397,12 +409,13 @@ func (r *Reloader) reload(isRetry bool) time.Duration { // example rewritten and truncated again between two loads, so the delay starts over; so does // a read with entries. A file is held back for at most maxHoldRetryDelays retry delays from // when a load first held it back, so that a file that keeps being written while every load -// finds it empty, or files that are rewritten in turn, are not held back without end. The -// bound is the file's own: a read with entries or a failed load does not restart it, and -// applying data, or the file staying empty for the whole delay, ends it. A file whose content -// is not in effect, such as a placeholder that has always been empty or an emptied file whose -// empty state has been applied, is parsed right away, and so is any file when retries are -// off, since nothing would read it again. +// finds it empty, or files that are rewritten or emptied in turn, are not held back without +// end. The bound is the file's own, and it starts over only at a load that holds nothing back +// and reads the file, whether that load applies, skips or fails: while any file holds the +// loads back, a read with entries, a failure, or the file staying empty for the whole delay +// does not restart it. A file whose content is not in effect, such as a placeholder that has +// always been empty or an emptied file whose empty state has been applied, is parsed right +// away, and so is any file when retries are off, since nothing would read it again. func (r *Reloader) holdBack(path string, now time.Time) bool { if r.cfg.RetryDelay <= 0 || !r.appliedHadContent[path] { return false @@ -418,8 +431,6 @@ func (r *Reloader) holdBack(path string, now time.Time) bool { empty.state = state } if now.Sub(empty.since) >= r.cfg.RetryDelay { - // The file has stayed empty for the whole delay; it is no rewrite. - empty.firstHeld = time.Time{} return false } if !empty.firstHeld.IsZero() && now.Sub(empty.firstHeld) >= r.maxHold() { diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index d77c3548..c6e1e8c9 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -1058,13 +1058,13 @@ func TestReloaderHoldsBackALongEmptyFileAgainWhenItIsWritten(t *testing.T) { func TestReloaderFailedLoadsDoNotRestartTheBoundOfAFileThatIsHeldBack(t *testing.T) { // The first file is written again before every load, so that every load finds it empty, - // and every other load fails on the second file. The failures do not restart the first - // file's bound. + // and every other load fails on the second file. Each of those loads still holds the first + // file back, so the failures do not restart its bound. f, second, mockLog, clock := newTwoFileFakeClockFixture(t) f.write(t, "") holds := f.requireLoadHolds(t, mockLog, 0, 1) - for i := range 7 { + for i := range 6 { emptyInPlace(t, f.path) clock.Advance(fakeClockRetryDelay * 8 / 10) if i%2 == 0 { @@ -1087,9 +1087,43 @@ func TestReloaderFailedLoadsDoNotRestartTheBoundOfAFileThatIsHeldBack(t *testing // 5.6 retry delays after the first file was first held back. require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) } +func TestReloaderKeepsTheBoundOfAFileThatAFailedLoadDidNotRead(t *testing.T) { + // The second file is written again before every load, so that every load finds it empty, + // and every other load fails on the first file, before it reads the second. Those loads + // hold nothing back, but they do not start the bound of a file they did not read over. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + require.NoError(t, os.WriteFile(second, nil, 0600)) + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + for i := range 6 { + emptyInPlace(t, second) + clock.Advance(fakeClockRetryDelay * 8 / 10) + if i%2 == 0 { + f.write(t, "flagValues: [") + f.reloader.ReloadNow() + require.Equal(t, holds, holdCount(mockLog)) + select { + case <-f.applied: + require.FailNow(t, "a failed load called Apply") + case <-f.errored: + default: + } + continue + } + f.write(t, yamlFlag3) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + } + + // 5.6 retry delays after the second file was first held back. + emptyInPlace(t, second) + clock.Advance(fakeClockRetryDelay * 8 / 10) + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(t))) +} + func TestReloaderAnotherFilesBoundDoesNotCutShortARewrite(t *testing.T) { // The first file is written again before every load, so that every load finds it empty, // until it reaches its bound. The second file is rewritten in place just before that, and @@ -1179,18 +1213,83 @@ func TestReloaderHoldAsksForTheRetryOfTheFileThatIsDueFirst(t *testing.T) { } func TestReloaderHoldAsksForTheRetryFromTheEndOfTheLoad(t *testing.T) { - // The read of the second file takes half a retry delay. The retry is measured from the - // end of the load. const delay = fakeClockRetryDelay - f, second, _, clock := newTwoFileFakeClockFixture(t) - f.reloader.readFile = func(path string) ([]byte, error) { - if path == second { - clock.Advance(delay / 2) + t.Run("rest of the delay", func(t *testing.T) { + // The read of the second file takes half a retry delay. The retry is measured from the + // end of the load. + f, second, _, clock := newTwoFileFakeClockFixture(t) + f.reloader.readFile = func(path string) ([]byte, error) { + if path == second { + clock.Advance(delay / 2) + } + return os.ReadFile(path) //nolint:gosec // test file path + } + f.write(t, "") + assert.Equal(t, delay/2+delay/10, f.reloader.reload(false)) + }) + t.Run("delay over during the load", func(t *testing.T) { + // The read of the file after the held one takes longer than what is left of the held + // file's delay, so the retry is due right away, after the margin. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + f.requireLoadHolds(t, mockLog, 0, 1) + clock.Advance(delay * 57 / 60) + f.reloader.readFile = func(path string) ([]byte, error) { + if path == second { + clock.Advance(delay * 6 / 60) + } + return os.ReadFile(path) //nolint:gosec // test file path + } + assert.Equal(t, delay/10, f.reloader.reload(false)) + }) +} + +func TestReloaderAppliesFilesEmptiedInStaggeredTurnsAfterTheBound(t *testing.T) { + // 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. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + f.requireLoadHolds(t, mockLog, 0, 1) + applied := false + for i := 1; i <= 14 && !applied; i++ { + clock.Advance(fakeClockRetryDelay * 54 / 60) + if i%2 == 1 { + emptyInPlace(t, second) + } else { + emptyInPlace(t, f.path) + } + f.reloader.ReloadNow() + // The retry that the hold arms, a little after the other file's delay is over. + clock.Advance(fakeClockRetryDelay * 12 / 60) + f.reloader.ReloadNow() + clock.Advance(-fakeClockRetryDelay * 12 / 60) + select { + case <-f.applied: + applied = true + default: } - return os.ReadFile(path) //nolint:gosec // test file path } + assert.True(t, applied, "the files were held back without end") +} + +func TestReloaderStartsTheBoundOfAFileHeldBackDuringAFailureOverOnceItIsReadAgain(t *testing.T) { + // While the second file fails to load, the first is rewritten once and its gap is held + // back. Later, the second file is fixed while the first is in the middle of another + // rewrite, which is held back with a bound of its own. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.write(t, "") + f.reloader.ReloadNow() + f.requireErrored(t) + f.write(t, yamlFlag1) + f.reloader.ReloadNow() + clock.Advance(fakeClockRetryDelay * 6) + f.reloader.ReloadNow() + + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) f.write(t, "") - assert.Equal(t, delay/2+delay/10, f.reloader.reload(false)) + f.requireLoadHolds(t, mockLog, 0, 1) } func TestReloaderArmsTheRetryThatTheHoldAsksFor(t *testing.T) { From 2e0b45b4440fa4b345f88396c9934e8f44e64935 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 06:03:31 +0000 Subject: [PATCH 08/10] fix: Start over the bound of a file that a load fails on, and end its empty state A file whose read or parse failed right after it was held back was not among the files that a load holding nothing back starts over, because the load returned before recording it. An operator who saves a file in place with a typo and then saves the fix in place got the fix applied empty once the old bound had run out. A load now records every file it gets to and does not hold back, including the file it fails on and files found missing, and starts their bounds over when it holds nothing back. A read with content, even content that fails to parse, or a missing file also ends the file's empty state, so its next emptying starts a new delay even where a coarse modification time makes it look the same. --- internal/filedata/reloader.go | 67 +++++++++---- internal/filedata/reloader_test.go | 153 ++++++++++++++++++++++++++++- ldfiledata/package_info.go | 21 ++-- ldfiledatav2/package_info.go | 21 ++-- ldoverrides/file_source_builder.go | 23 ++--- 5 files changed, 231 insertions(+), 54 deletions(-) diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index dd6aec83..250bb736 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -289,12 +289,16 @@ func (r *Reloader) reload(isRetry bool) time.Duration { // The files that holdBack holds back. The other files are still read, so that every file // that is found empty starts its delay in this load. var heldBack []string + // The files that the load got to and did not hold back, including one that it failed to + // read or parse. + var reached []string defer func() { // A load 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 back, so its next - // rewrite is a new one. A file that the load did not get to keeps its bound. + // over for every file that it got to: such a file is no longer held back, so its next + // rewrite is a new one. A file after the one that failed, which the load did not get to, + // keeps its bound. if len(heldBack) == 0 { - for path := range hadContent { + for _, path := range reached { if empty, ok := r.emptyFiles[path]; ok { empty.firstHeld = time.Time{} } @@ -309,10 +313,12 @@ func (r *Reloader) reload(isRetry bool) time.Duration { if err != nil { if r.cfg.SkipMissingPaths && errors.Is(err, fs.ErrNotExist) { r.cfg.Loggers.Debugf("File %s does not exist; it contributes no data", path) + reached = r.noteNotEmpty(path, reached) hadContent[path] = false files = append(files, FileSummary{Path: path}) continue } + reached = append(reached, path) return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) } // A file with no content, or with content that is no YAML document, such as only a @@ -324,19 +330,21 @@ func (r *Reloader) reload(isRetry bool) time.Duration { if hasContent { doc, hasContent, err = parseFileContent(rawData) if err != nil { + reached = r.noteNotEmpty(path, reached) return r.fail(&ReadError{Err: err, Path: path}) } parsed = true } - if hasContent { - if empty, ok := r.emptyFiles[path]; ok { - empty.since = time.Time{} - } - } else if r.holdBack(path, r.now()) { + switch { + case hasContent: + reached = r.noteNotEmpty(path, reached) + case r.holdBack(path, r.now()): // The clock is read after this file's own read, so a slow read of another file // does not make its emptiness look older than it is. heldBack = append(heldBack, path) continue + default: + reached = append(reached, path) } hadContent[path] = hasContent _, _ = hasher.Write(rawData) @@ -400,22 +408,39 @@ func (r *Reloader) reload(isRetry bool) time.Duration { return 0 } +// noteNotEmpty records a file that a load found with content, even content that fails to +// parse, or found missing, among the files that the load got to. If the file was empty +// before, its empty state has ended, so its next emptying starts a new delay even where a +// coarse modification time makes it look the same. +func (r *Reloader) noteNotEmpty(path string, reached []string) []string { + if empty, ok := r.emptyFiles[path]; ok { + empty.since = time.Time{} + } + return append(reached, path) +} + // holdBack reports whether a load that found the file with no entries holds it back. A file -// whose content is in effect and that has no entries 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, however many -// loads find it so in that time. A change of its state, its modification time and size read -// through any symbolic link, means that it was written since a load last found it so, for -// example rewritten and truncated again between two loads, so the delay starts over; so does -// a read with entries. A file is held back for at most maxHoldRetryDelays retry delays from +// whose content is in effect and that has no entries 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, +// however many loads find it so in that time. A change of its state, its modification time +// and size read through any symbolic link, means that it was written since a load last +// found it so, for example rewritten and truncated again between two loads, so the delay +// starts over; so does a read with content, even content that fails to parse, or finding +// the file missing. A file is held back for at most maxHoldRetryDelays retry delays from // when a load first held it back, so that a file that keeps being written while every load // finds it empty, or files that are rewritten or emptied in turn, are not held back without -// end. The bound is the file's own, and it starts over only at a load that holds nothing back -// and reads the file, whether that load applies, skips or fails: while any file holds the -// loads back, a read with entries, a failure, or the file staying empty for the whole delay -// does not restart it. A file whose content is not in effect, such as a placeholder that has -// always been empty or an emptied file whose empty state has been applied, is parsed right -// away, and so is any file when retries are off, since nothing would read it again. +// end. The bound is the file's own, and it starts over only at a load that holds nothing +// back and gets to the file, whether that load applies, skips or fails, including a load +// that fails on this file: while any file holds the loads back, a read with entries, a +// failure, or the file staying empty for the whole delay does not restart it. A file after +// the one that a load failed on keeps its bound. The bounds of different files do not +// overlap, so n files that each keep being written while empty, started a little less than +// the bound apart, hold the loads back for about n bounds, and a failure timed to every +// expiry of a bound could hold them back without end. A file whose content is not in +// effect, such as a placeholder that has always been empty or an emptied file whose empty +// state has been applied, is parsed right away, and so is any file when retries are off, +// since nothing would read it again. func (r *Reloader) holdBack(path string, now time.Time) bool { if r.cfg.RetryDelay <= 0 || !r.appliedHadContent[path] { return false diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index c6e1e8c9..47613dd5 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -506,10 +506,20 @@ func (c *fakeClock) Advance(d time.Duration) { // newEmptyFileFixture configures a reloader for a YAML file that defines flag1, with the // retry delay of the tests of a file that is found empty, and a log that keeps debug output. func newEmptyFileFixture(t *testing.T, configure func(*ReloaderConfig)) (*reloaderFixture, *ldlogtest.MockLog) { + t.Helper() + return newEmptyFileFixtureAt(t, "data.yaml", yamlFlag1, configure) +} + +func newEmptyFileFixtureAt( + t *testing.T, + fileName string, + initialContent string, + configure func(*ReloaderConfig), +) (*reloaderFixture, *ldlogtest.MockLog) { t.Helper() mockLog := ldlogtest.NewMockLog() mockLog.Loggers.SetMinLevel(ldlog.Debug) - f := newReloaderFixtureAt(t, "data.yaml", yamlFlag1, func(cfg *ReloaderConfig) { + f := newReloaderFixtureAt(t, fileName, initialContent, func(cfg *ReloaderConfig) { cfg.Loggers = mockLog.Loggers cfg.RetryDelay = emptyFileRetryDelay if configure != nil { @@ -526,7 +536,17 @@ func newFakeClockFixture( configure func(*ReloaderConfig), ) (*reloaderFixture, *ldlogtest.MockLog, *fakeClock) { t.Helper() - f, mockLog := newEmptyFileFixture(t, func(cfg *ReloaderConfig) { + return newFakeClockFixtureAt(t, "data.yaml", yamlFlag1, configure) +} + +func newFakeClockFixtureAt( + t *testing.T, + fileName string, + initialContent string, + configure func(*ReloaderConfig), +) (*reloaderFixture, *ldlogtest.MockLog, *fakeClock) { + t.Helper() + f, mockLog := newEmptyFileFixtureAt(t, fileName, initialContent, func(cfg *ReloaderConfig) { cfg.RetryDelay = fakeClockRetryDelay if configure != nil { configure(cfg) @@ -1124,6 +1144,135 @@ func TestReloaderKeepsTheBoundOfAFileThatAFailedLoadDidNotRead(t *testing.T) { assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(t))) } +func TestReloaderHoldsBackAFixOfAFileSavedInPlaceWithAnError(t *testing.T) { + // An operator saves a file in place with a typo, and some time later saves the fix in + // place. Each save truncates the file first. The loads that fail on the typo read the file, + // so the save of the fix gets a delay and a bound of its own. + cases := []struct { + name, fileName, good, broken, fixed string + sameModTime bool + }{ + {"yaml", "data.yaml", yamlFlag1, "flagValues: [", yamlFlag2, false}, + {"json", "data.json", `{"flagValues": {"flag1": true}}`, `{"flagValues": {"flag1": tru`, + `{"flagValues": {"flag2": true}}`, false}, + {"yaml with the same modification time", "data.yaml", yamlFlag1, "flagValues: [", yamlFlag2, true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + f, mockLog, clock := newFakeClockFixtureAt(t, c.fileName, c.good, nil) + f.requireLoadApplies(t) + f.write(t, "") + info, err := os.Stat(f.path) + require.NoError(t, err) + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + f.write(t, c.broken) + f.reloader.ReloadNow() + f.requireErrored(t) + for range 6 { + clock.Advance(fakeClockRetryDelay) + f.reloader.ReloadNow() + } + + f.write(t, "") + if c.sameModTime { + // As a file system with coarse timestamps can record it. + require.NoError(t, os.Chtimes(f.path, info.ModTime(), info.ModTime())) + } + f.requireLoadHolds(t, mockLog, holds, 1) + assert.Len(t, mockLog.GetOutput(ldlog.Error), 1) + f.write(t, c.fixed) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) + }) + } +} + +func TestReloaderStartsTheBoundOfAFileThatFailsToReadOver(t *testing.T) { + // Without SkipMissingPaths, as for the file data sources, a missing file fails the load. + // The file is emptied and held back, then deleted for a while, then created empty again. + // The loads that fail to read it get to it, so its new emptying gets a bound of its own. + f, mockLog, clock := newFakeClockFixture(t, nil) + f.requireLoadApplies(t) + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + require.NoError(t, os.Remove(f.path)) + f.reloader.ReloadNow() + f.requireErrored(t) + for range 6 { + clock.Advance(fakeClockRetryDelay) + f.reloader.ReloadNow() + } + + emptyInPlace(t, f.path) + f.requireLoadHolds(t, mockLog, holds, 1) +} + +func TestReloaderLoadThatHoldsAFileBackAndThenFailsDoesNotStartBoundsOver(t *testing.T) { + // The first two files are rewritten in place in turn, so that every load holds one of them + // back, and every other load then fails on a third file. A load that holds a file back does + // not start any bound over, even when it fails afterwards, so the first file reaches its + // bound. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + third := filepath.Join(filepath.Dir(second), "third.yaml") + require.NoError(t, os.WriteFile(third, []byte(yamlFlag3), 0600)) + f.reloader.cfg.Paths = append(f.reloader.cfg.Paths, third) + f.requireLoadApplies(t) + f.write(t, "") + f.requireLoadHolds(t, mockLog, 0, 1) + + applied := false + for i := 1; i <= 12 && !applied; i++ { + clock.Advance(fakeClockRetryDelay / 2) + if i%2 == 1 { + f.write(t, yamlFlag1) + emptyInPlace(t, second) + require.NoError(t, os.WriteFile(third, []byte("flagValues: ["), 0600)) + } else { + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + emptyInPlace(t, f.path) + require.NoError(t, os.WriteFile(third, []byte(yamlFlag3), 0600)) + } + f.reloader.ReloadNow() + select { + case <-f.applied: + applied = true + default: + } + } + assert.True(t, applied, "the files were held back without end") +} + +func TestReloaderFileFoundMissingStartsANewDelayWhenItIsCreatedEmptyAgain(t *testing.T) { + // The first file is emptied and held back, then deleted, then created empty again, while + // the second file fails to load, so nothing is applied. Finding the file missing ends its + // empty state, so its new emptying gets a delay of its own even though it has the same + // modification time as the first one, as a file system with coarse timestamps can record. + var second string + f, mockLog, clock := newFakeClockFixture(t, func(cfg *ReloaderConfig) { + second = filepath.Join(filepath.Dir(cfg.Paths[0]), "second.yaml") + cfg.Paths = append(cfg.Paths, second) + cfg.SkipMissingPaths = true + }) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.requireLoadApplies(t) + + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.write(t, "") + info, err := os.Stat(f.path) + require.NoError(t, err) + f.reloader.ReloadNow() + f.requireErrored(t) + require.NoError(t, os.Remove(f.path)) + f.reloader.ReloadNow() + + clock.Advance(fakeClockRetryDelay * 70 / 60) + require.NoError(t, os.WriteFile(f.path, nil, 0600)) + require.NoError(t, os.Chtimes(f.path, info.ModTime(), info.ModTime())) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.requireLoadHolds(t, mockLog, 0, 1) +} + func TestReloaderAnotherFilesBoundDoesNotCutShortARewrite(t *testing.T) { // The first file is written again before every load, so that every load finds it empty, // until it reaches its bound. The second file is rewritten in place just before that, and diff --git a/ldfiledata/package_info.go b/ldfiledata/package_info.go index 1b78801a..013bdc46 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -30,16 +30,17 @@ // whose content is in effect and that a reload finds empty, or holding only a YAML comment // or "---", is held back: the flags in effect stay until the file has stayed that way, // without being written, for about a second, or for at most about five seconds while it -// keeps being rewritten. A rewrite that leaves the file empty for less than a second -// therefore keeps the flags in effect throughout. A reload applies all of the files or -// none, so changes to the other files wait while a file is held back. A file that stays -// empty is a YAML document with no flags, whatever its name (see the format below), and is -// applied once it is no longer held back. JSON that is cut off fails to parse, so the flags -// in effect stay until the file is complete. Without a reloader, the files are loaded once, -// with no retry. YAML that is cut off can still parse, as a part of its entries or with its -// last value cut short, so for a YAML file, replacing its entire contents in one operation, -// such as renaming a fully written temporary file over it, avoids briefly loading only a -// part of it. +// keeps being rewritten; several files that keep being rewritten one after another can hold +// the reload back for about five seconds each. A rewrite that leaves the file empty for +// less than a second therefore keeps the flags in effect throughout. A reload applies all +// of the files or none, so changes to the other files wait while a file is held back. A +// file that stays empty is a YAML document with no flags, whatever its name (see the format +// below), and is applied once it is no longer held back. JSON that is cut off fails to +// parse, so the flags in effect stay until the file is complete. Without a reloader, the +// files are loaded once, with no retry. YAML that is cut off can still parse, as a part of +// its entries or with its last value cut short, so for a YAML file, replacing its entire +// contents in one operation, such as renaming a fully written temporary file over it, +// avoids briefly loading only a part of it. // // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three diff --git a/ldfiledatav2/package_info.go b/ldfiledatav2/package_info.go index f56bb0d9..63b85bb2 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -30,16 +30,17 @@ // whose content is in effect and that a reload finds empty, or holding only a YAML comment // or "---", is held back: the flags in effect stay until the file has stayed that way, // without being written, for about a second, or for at most about five seconds while it -// keeps being rewritten. A rewrite that leaves the file empty for less than a second -// therefore keeps the flags in effect throughout. A reload applies all of the files or -// none, so changes to the other files wait while a file is held back. A file that stays -// empty is a YAML document with no flags, whatever its name (see the format below), and is -// applied once it is no longer held back. JSON that is cut off fails to parse, so the flags -// in effect stay until the file is complete. Without a reloader, the files are loaded once, -// with no retry. YAML that is cut off can still parse, as a part of its entries or with its -// last value cut short, so for a YAML file, replacing its entire contents in one operation, -// such as renaming a fully written temporary file over it, avoids briefly loading only a -// part of it. +// keeps being rewritten; several files that keep being rewritten one after another can hold +// the reload back for about five seconds each. A rewrite that leaves the file empty for +// less than a second therefore keeps the flags in effect throughout. A reload applies all +// of the files or none, so changes to the other files wait while a file is held back. A +// file that stays empty is a YAML document with no flags, whatever its name (see the format +// below), and is applied once it is no longer held back. JSON that is cut off fails to +// parse, so the flags in effect stay until the file is complete. Without a reloader, the +// files are loaded once, with no retry. YAML that is cut off can still parse, as a part of +// its entries or with its last value cut short, so for a YAML file, replacing its entire +// contents in one operation, such as renaming a fully written temporary file over it, +// avoids briefly loading only a part of it. // // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three diff --git a/ldoverrides/file_source_builder.go b/ldoverrides/file_source_builder.go index 0638a7d3..b06632a9 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -87,17 +87,18 @@ type FileSourceBuilder struct { // finds empty, or holding only a YAML comment or "---", may be in the middle of a rewrite, // so the source holds it back: the overrides in effect stay until the file has stayed that // way, without being written, for about a second, or for at most about five seconds while -// it keeps being rewritten. A rewrite that leaves the file empty for less than a second -// therefore never removes them. A reload applies all of the files or none, so changes to -// the other files wait while a file is held back. The source tells the formats apart by the -// content: a file whose first character other than white space is "{" is JSON, and any -// other file is YAML. An empty file is therefore a YAML document with no entries, whatever -// its name, and a file that stays empty, even one named .json, removes its overrides once -// it is no longer held back. JSON that is cut off fails to parse, so the overrides in -// effect stay until the file is complete. YAML that is cut off can still parse, as a part -// of its entries or with its last value cut short, 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. +// it keeps being rewritten; several files that keep being rewritten one after another can +// hold the reload back for about five seconds each. A rewrite that leaves the file empty +// for less than a second therefore never removes them. A reload applies all of the files or +// none, so changes to the other files wait while a file is held back. The source tells the +// formats apart by the content: a file whose first character other than white space is "{" +// is JSON, and any other file is YAML. An empty file is therefore a YAML document with no +// entries, whatever its name, and a file that stays empty, even one named .json, removes +// its overrides once it is no longer held back. JSON that is cut off fails to parse, so the +// overrides in effect stay until the file is complete. YAML that is cut off can still +// parse, as a part of its entries or with its last value cut short, 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. // // Whenever the set of overrides in effect changes, including at startup, the source logs // the overrides in effect and what each configured file supplied, at Info level. From 1fd735b45f1facd3a9781d2ee3310607cb693687 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 07:57:39 +0000 Subject: [PATCH 09/10] fix: Read every data file even after one fails, so no bound is left over A load stopped at the first file that failed to read or parse, so a file configured after it was not read, and its bound never started over. More than five retry delays later, a deploy that fixed the failing file while rewriting the later one in place applied the later file empty. A load now reads and classifies every configured file even after a failure, and fails with the first failure after the loop. A failure comes first, then a hold, then the merge, which runs only once every file has been read. --- internal/filedata/reloader.go | 49 ++++++++++++++-------- internal/filedata/reloader_test.go | 67 ++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 21 deletions(-) diff --git a/internal/filedata/reloader.go b/internal/filedata/reloader.go index 250bb736..e53ebd5c 100644 --- a/internal/filedata/reloader.go +++ b/internal/filedata/reloader.go @@ -289,14 +289,20 @@ func (r *Reloader) reload(isRetry bool) time.Duration { // The files that holdBack holds back. The other files are still read, so that every file // that is found empty starts its delay in this load. var heldBack []string - // The files that the load got to and did not hold back, including one that it failed to - // read or parse. + // The files that the load did not hold back, including those that it failed to read or + // parse. A failure does not stop the reads: the load fails with the first failure, but + // every file is classified, so that no file's bound is left over from before the failure. var reached []string + var failure error + fail := func(err error) { + if failure == nil { + failure = err + } + } defer func() { // A load 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 back, so its next - // rewrite is a new one. A file after the one that failed, which the load did not get to, - // keeps its bound. + // over for every file: such a file is no longer held back, so its next rewrite is a + // new one. if len(heldBack) == 0 { for _, path := range reached { if empty, ok := r.emptyFiles[path]; ok { @@ -319,7 +325,8 @@ func (r *Reloader) reload(isRetry bool) time.Duration { continue } reached = append(reached, path) - return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) + fail(&ReadError{Err: wrapReadError(err), Path: path}) + continue } // A file with no content, or with content that is no YAML document, such as only a // comment or "---", has no entries. A read can see that much of a file that is being @@ -331,7 +338,8 @@ func (r *Reloader) reload(isRetry bool) time.Duration { doc, hasContent, err = parseFileContent(rawData) if err != nil { reached = r.noteNotEmpty(path, reached) - return r.fail(&ReadError{Err: err, Path: path}) + fail(&ReadError{Err: err, Path: path}) + continue } parsed = true } @@ -352,12 +360,18 @@ func (r *Reloader) reload(isRetry bool) time.Duration { if !parsed { doc, err = parseDocument(rawData) if err != nil { - return r.fail(&ReadError{Err: err, Path: path}) + fail(&ReadError{Err: err, Path: path}) + continue } } docs = append(docs, doc) files = append(files, FileSummary{Path: path, Present: true}) } + // A failure comes first, then a hold, then the merge, which runs only once every file has + // been read. + if failure != nil { + return r.fail(failure) + } if len(heldBack) > 0 { return r.readAgainLater(heldBack, r.now()) } @@ -431,16 +445,15 @@ func (r *Reloader) noteNotEmpty(path string, reached []string) []string { // when a load first held it back, so that a file that keeps being written while every load // finds it empty, or files that are rewritten or emptied in turn, are not held back without // end. The bound is the file's own, and it starts over only at a load that holds nothing -// back and gets to the file, whether that load applies, skips or fails, including a load -// that fails on this file: while any file holds the loads back, a read with entries, a -// failure, or the file staying empty for the whole delay does not restart it. A file after -// the one that a load failed on keeps its bound. The bounds of different files do not -// overlap, so n files that each keep being written while empty, started a little less than -// the bound apart, hold the loads back for about n bounds, and a failure timed to every -// expiry of a bound could hold them back without end. A file whose content is not in -// effect, such as a placeholder that has always been empty or an emptied file whose empty -// state has been applied, is parsed right away, and so is any file when retries are off, -// since nothing would read it again. +// back, whether that load applies, skips or fails, on this file or another one: every load +// reads every file. While any file holds the loads back, a read with entries, a failure, or +// the file staying empty for the whole delay does not restart it. The bounds of different +// files do not overlap, so n files that each keep being written while empty, started a +// little less than the bound apart, hold the loads back for about n bounds, and a failure +// timed to every expiry of a bound could hold them back without end. A file whose content +// is not in effect, such as a placeholder that has always been empty or an emptied file +// whose empty state has been applied, is parsed right away, and so is any file when retries +// are off, since nothing would read it again. func (r *Reloader) holdBack(path string, now time.Time) bool { if r.cfg.RetryDelay <= 0 || !r.appliedHadContent[path] { return false diff --git a/internal/filedata/reloader_test.go b/internal/filedata/reloader_test.go index 47613dd5..13c8f0ab 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -1111,10 +1111,10 @@ func TestReloaderFailedLoadsDoNotRestartTheBoundOfAFileThatIsHeldBack(t *testing assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) } -func TestReloaderKeepsTheBoundOfAFileThatAFailedLoadDidNotRead(t *testing.T) { +func TestReloaderAppliesAFileKeptWrittenWhileEmptyAfterItsBoundWhileAnEarlierFileFails(t *testing.T) { // The second file is written again before every load, so that every load finds it empty, - // and every other load fails on the first file, before it reads the second. Those loads - // hold nothing back, but they do not start the bound of a file they did not read over. + // and every other load fails on the first file. Those loads still read the second file + // and hold it back, so they do not start its bound over. f, second, mockLog, clock := newTwoFileFakeClockFixture(t) require.NoError(t, os.WriteFile(second, nil, 0600)) holds := f.requireLoadHolds(t, mockLog, 0, 1) @@ -1273,6 +1273,67 @@ func TestReloaderFileFoundMissingStartsANewDelayWhenItIsCreatedEmptyAgain(t *tes f.requireLoadHolds(t, mockLog, 0, 1) } +func TestReloaderStartsTheBoundOfAFileAfterAFailingFileOverOnceItIsReadAgain(t *testing.T) { + // The second file is held back. Then the first file fails while the second gets its + // content back, and loads fail on the first file for a long time. They still read the + // second file, so when the first file is fixed while the second is in the middle of + // another rewrite, that rewrite is held back with a bound of its own. + cases := []struct { + name string + firstFromStart string + breakFirst func(t *testing.T, f *reloaderFixture) + fixFirst func(t *testing.T, f *reloaderFixture) + }{ + {"parse failure", yamlFlag1, + func(t *testing.T, f *reloaderFixture) { f.write(t, "flagValues: [") }, + func(t *testing.T, f *reloaderFixture) { f.write(t, yamlFlag1) }}, + {"read failure", yamlFlag1, + func(t *testing.T, f *reloaderFixture) { require.NoError(t, os.Remove(f.path)) }, + func(t *testing.T, f *reloaderFixture) { f.write(t, yamlFlag1) }}, + // A placeholder is not held back, so white space that YAML cannot parse fails the load. + {"white space that cannot be parsed", "", + func(t *testing.T, f *reloaderFixture) { f.write(t, " \t\n") }, + func(t *testing.T, f *reloaderFixture) { f.write(t, "") }}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + second := filepath.Join(t.TempDir(), "second.yaml") + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f, mockLog, clock := newFakeClockFixture(t, func(cfg *ReloaderConfig) { + cfg.Paths = append(cfg.Paths, second) + }) + f.write(t, c.firstFromStart) + f.requireLoadApplies(t) + require.NoError(t, os.WriteFile(second, nil, 0600)) + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + c.breakFirst(t, f) + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + f.reloader.ReloadNow() + f.requireErrored(t) + for range 6 { + clock.Advance(fakeClockRetryDelay) + f.reloader.ReloadNow() + } + + c.fixFirst(t, f) + emptyInPlace(t, second) + f.requireLoadHolds(t, mockLog, holds, 1) + }) + } +} + +func TestReloaderFailingLoadReportsTheFirstFailure(t *testing.T) { + f, second, _, _ := newTwoFileFakeClockFixture(t) + f.write(t, "flagValues: [") + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + f.reloader.ReloadNow() + var readErr *ReadError + require.ErrorAs(t, f.requireErrored(t), &readErr) + assert.Equal(t, f.path, readErr.Path) + f.requireQuiet(t, 50*time.Millisecond) +} + func TestReloaderAnotherFilesBoundDoesNotCutShortARewrite(t *testing.T) { // The first file is written again before every load, so that every load finds it empty, // until it reaches its bound. The second file is rewritten in place just before that, and From 1ac99af9f4e9a6c676b973e6ede013a435c641e9 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:35:38 +0000 Subject: [PATCH 10/10] docs: Say only what an application needs about rewriting data files in place --- ldfiledata/package_info.go | 22 ++++++---------------- ldfiledatav2/package_info.go | 22 ++++++---------------- ldoverrides/file_source_builder.go | 25 ++++++++----------------- 3 files changed, 20 insertions(+), 49 deletions(-) diff --git a/ldfiledata/package_info.go b/ldfiledata/package_info.go index 013bdc46..ef640a45 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -25,22 +25,12 @@ // file that is rewritten continuously, faster than that interval, does not reload until // the writes pause. // -// With a reloader, a file can be rewritten in place, so a reload can read it before it has -// been fully written. Content that cannot be parsed is retried about a second later. A file -// whose content is in effect and that a reload finds empty, or holding only a YAML comment -// or "---", is held back: the flags in effect stay until the file has stayed that way, -// without being written, for about a second, or for at most about five seconds while it -// keeps being rewritten; several files that keep being rewritten one after another can hold -// the reload back for about five seconds each. A rewrite that leaves the file empty for -// less than a second therefore keeps the flags in effect throughout. A reload applies all -// of the files or none, so changes to the other files wait while a file is held back. A -// file that stays empty is a YAML document with no flags, whatever its name (see the format -// below), and is applied once it is no longer held back. JSON that is cut off fails to -// parse, so the flags in effect stay until the file is complete. Without a reloader, the -// files are loaded once, with no retry. YAML that is cut off can still parse, as a part of -// its entries or with its last value cut short, so for a YAML file, replacing its entire -// contents in one operation, such as renaming a fully written temporary file over it, -// avoids briefly loading only a part of it. +// With a reloader, files can be rewritten in place. If the data source reads a file while +// it is being written, it keeps the flags it has and reads the file again shortly +// afterward. A file that stays empty, whatever its name, is a YAML document with no flags +// (see the format below). For a YAML file, replacing its contents in one operation, such as +// by renaming a fully written temporary file over it, avoids briefly loading a partly +// written file. // // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three diff --git a/ldfiledatav2/package_info.go b/ldfiledatav2/package_info.go index 63b85bb2..0bf5f5f5 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -25,22 +25,12 @@ // file that is rewritten continuously, faster than that interval, does not reload until // the writes pause. // -// With a reloader, a file can be rewritten in place, so a reload can read it before it has -// been fully written. Content that cannot be parsed is retried about a second later. A file -// whose content is in effect and that a reload finds empty, or holding only a YAML comment -// or "---", is held back: the flags in effect stay until the file has stayed that way, -// without being written, for about a second, or for at most about five seconds while it -// keeps being rewritten; several files that keep being rewritten one after another can hold -// the reload back for about five seconds each. A rewrite that leaves the file empty for -// less than a second therefore keeps the flags in effect throughout. A reload applies all -// of the files or none, so changes to the other files wait while a file is held back. A -// file that stays empty is a YAML document with no flags, whatever its name (see the format -// below), and is applied once it is no longer held back. JSON that is cut off fails to -// parse, so the flags in effect stay until the file is complete. Without a reloader, the -// files are loaded once, with no retry. YAML that is cut off can still parse, as a part of -// its entries or with its last value cut short, so for a YAML file, replacing its entire -// contents in one operation, such as renaming a fully written temporary file over it, -// avoids briefly loading only a part of it. +// With a reloader, files can be rewritten in place. If the data source reads a file while +// it is being written, it keeps the flags it has and reads the file again shortly +// afterward. A file that stays empty, whatever its name, is a YAML document with no flags +// (see the format below). For a YAML file, replacing its contents in one operation, such as +// by renaming a fully written temporary file over it, avoids briefly loading a partly +// written file. // // Files may contain either JSON or YAML; if the first non-whitespace character is '{', the file is parsed // as JSON, otherwise it is parsed as YAML. The file data should consist of an object with up to three diff --git a/ldoverrides/file_source_builder.go b/ldoverrides/file_source_builder.go index b06632a9..243997cf 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -82,23 +82,14 @@ type FileSourceBuilder struct { // loaded overrides stay in effect. The source logs the failure, retries after a short // delay, and recovers on its own once the file is readable again. // -// 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 file whose content is in effect and that a reload -// finds empty, or holding only a YAML comment or "---", may be in the middle of a rewrite, -// so the source holds it back: the overrides in effect stay until the file has stayed that -// way, without being written, for about a second, or for at most about five seconds while -// it keeps being rewritten; several files that keep being rewritten one after another can -// hold the reload back for about five seconds each. A rewrite that leaves the file empty -// for less than a second therefore never removes them. A reload applies all of the files or -// none, so changes to the other files wait while a file is held back. The source tells the -// formats apart by the content: a file whose first character other than white space is "{" -// is JSON, and any other file is YAML. An empty file is therefore a YAML document with no -// entries, whatever its name, and a file that stays empty, even one named .json, removes -// its overrides once it is no longer held back. JSON that is cut off fails to parse, so the -// overrides in effect stay until the file is complete. YAML that is cut off can still -// parse, as a part of its entries or with its last value cut short, 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. +// Files can be rewritten in place: if the source finds a file empty while it is being +// rewritten, it keeps the overrides in effect until the new content is written. To remove a +// file's overrides, delete the file or write a document with no entries, such as {}. The +// source tells JSON from YAML by the content: a file whose first character other than white +// space is "{" is JSON, and any other file is YAML. A file that stays empty, whatever its +// name, is therefore a YAML document with no entries, and removes its overrides. For a YAML +// file, replacing its contents in one operation, such as by renaming a fully written +// temporary file over it, avoids briefly applying a partly written file. // // Whenever the set of overrides in effect changes, including at startup, the source logs // the overrides in effect and what each configured file supplied, at Info level.