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 86d4f935..e53ebd5c 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" @@ -56,8 +57,11 @@ 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 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 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. @@ -66,13 +70,14 @@ 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, holds back a file that was emptied, and skips no-op applications. 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 @@ -81,8 +86,42 @@ type Reloader struct { reloadMu sync.Mutex lastGoodHash []byte lastErrorMsg string + // 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 with no + // entries since the data in effect was applied. Applying data clears it. See holdBack. + // Guarded by reloadMu. + emptyFiles map[string]*emptyFile + // 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 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, or zero when the file has not been + // held back since a load that held nothing back read it. + firstHeld time.Time +} + +// 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 // ReloadNow, route change signals to Trigger, and call Close when finished. // @@ -92,10 +131,14 @@ 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 time.Duration, 1), + closeCh: make(chan struct{}), + appliedHadContent: make(map[string]bool, len(cfg.Paths)), + emptyFiles: make(map[string]*emptyFile), + now: time.Now, + readFile: os.ReadFile, } } @@ -116,9 +159,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(false); retryAfter > 0 { select { - case r.retryRequestCh <- struct{}{}: + case r.retryRequestCh <- retryAfter: default: } } @@ -160,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 !r.reload() && r.cfg.RetryDelay > 0 { - retryTimer = time.NewTimer(r.cfg.RetryDelay) - retryC = retryTimer.C + if retryAfter := r.reload(isRetry); retryAfter > 0 { + armRetry(retryAfter) } } @@ -196,12 +242,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) - retryC = retryTimer.C + armRetry(retryAfter) } case <-debounceC: debounceTimer, debounceC = nil, nil @@ -213,45 +258,123 @@ 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. -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(isRetry bool) 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 } + 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 + 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 + // 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: 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 { + 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 // 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) + reached = r.noteNotEmpty(path, reached) + hadContent[path] = false files = append(files, FileSummary{Path: path}) continue } - return r.fail(&ReadError{Err: wrapReadError(err), Path: path}) + reached = append(reached, 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 + // written. + var doc Document + parsed := false + hasContent := len(bytes.TrimSpace(rawData)) > 0 + if hasContent { + doc, hasContent, err = parseFileContent(rawData) + if err != nil { + reached = r.noteNotEmpty(path, reached) + fail(&ReadError{Err: err, Path: path}) + continue + } + parsed = true + } + 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) _, _ = 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 { + 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()) + } merged, err := Merge(r.cfg.DuplicateKeysHandling, r.cfg.MakeFlagWithValue, docs...) if err != nil { @@ -274,7 +397,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 @@ -283,21 +406,118 @@ 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, 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 true + return 0 } r.lastGoodHash = hash if r.cfg.Apply != nil { r.cfg.Apply(merged) } + 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 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, 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 + } + state := observe(path) + empty, ok := r.emptyFiles[path] + if !ok { + empty = &emptyFile{} + r.emptyFiles[path] = empty + } + if empty.since.IsZero() || !empty.state.equal(state) { + empty.since = now + empty.state = state + } + if now.Sub(empty.since) >= r.cfg.RetryDelay { + return false + } + if !empty.firstHeld.IsZero() && now.Sub(empty.firstHeld) >= r.maxHold() { + return false + } + if empty.firstHeld.IsZero() { + empty.firstHeld = now + } return true } -func (r *Reloader) fail(err error) bool { +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. +// 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 + } + 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) + 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 + return max(until.Sub(now), 0) + r.cfg.RetryDelay/10 +} + +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 @@ -305,14 +525,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 28a8344f..13c8f0ab 100644 --- a/internal/filedata/reloader_test.go +++ b/internal/filedata/reloader_test.go @@ -3,7 +3,12 @@ package filedata import ( "os" "path/filepath" + "regexp" "runtime" + "slices" + "strings" + "sync" + "sync/atomic" "testing" "time" @@ -24,9 +29,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 +78,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 +463,1101 @@ 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 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) { + 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, fileName, initialContent, func(cfg *ReloaderConfig) { + cfg.Loggers = mockLog.Loggers + cfg.RetryDelay = emptyFileRetryDelay + if configure != nil { + configure(cfg) + } + }) + 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() + 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) + } + }) + 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 { + 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 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 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)) + info, err := os.Stat(path) + require.NoError(t, err) + 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 + // 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\t\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() + f.requireHeldBack(t, mockLog, 0, 1) + 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 TestReloaderAppliesAnEmptiedFileThatStaysEmptyAtTheFirstRetry(t *testing.T) { + f, mockLog := newEmptyFileFixture(t, nil) + f.reloader.ReloadNow() + f.requireApplied(t) + + f.write(t, "") + f.reloader.Trigger() + f.requireHeldBack(t, mockLog, 0, 1) + + // 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) + 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) { + // 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. + 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.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) { + // 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() + 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 TestReloaderDoesNotHoldBackAFileThatIsEmptyFromTheStart(t *testing.T) { + // An empty file next to the data, such as a placeholder for later entries, has never had + // 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) { + 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.Equal(t, 0, holdCount(mockLog)) +} + +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 TestReloaderDoesNotHoldBackAFileThatWasMissing(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 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) +} + +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) +} + +// 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() + require.Equal(t, before+files, holdCount(mockLog), "the load did not hold back %d file(s)", files) + f.requireNoCallback(t) + return before + files +} + +// 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: + } +} + +// 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) +} + +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.requireLoadApplies(t) + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + clock.Advance(fakeClockRetryDelay) + 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) + 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.requireLoadApplies(t) + + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + f.write(t, yamlFlag2) + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + 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.requireLoadHolds(t, mockLog, holds, 1) + + f.write(t, yamlFlag3) + assert.Equal(t, []string{"flag3"}, flagKeys(f.requireLoadApplies(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.requireLoadApplies(t) + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + for range 6 { + f.write(t, yamlFlag2) + emptyInPlace(t, f.path) + clock.Advance(fakeClockRetryDelay * 8 / 10) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + } + + // 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) + assert.Empty(t, f.requireLoadApplies(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) + }) + 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, 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 7 { + 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. 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)) + holds := f.requireLoadHolds(t, mockLog, 0, 1) + + clock.Advance(fakeClockRetryDelay * 3 / 10) + f.write(t, yamlFlag3) + f.requireLoadHolds(t, mockLog, holds, 1) + + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + assert.Equal(t, []string{"flag2", "flag3"}, flagKeys(f.requireLoadApplies(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.requireLoadHolds(t, mockLog, 0, 2) + + clock.Advance(fakeClockRetryDelay) + assert.Empty(t, f.requireLoadApplies(t).Flags) +} + +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 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) + + f.write(t, "") + holds := f.requireLoadHolds(t, mockLog, 0, 1) + f.write(t, yamlFlag2) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) + + clock.Advance(fakeClockRetryDelay * 49 / 10) + emptyInPlace(t, f.path) + holds = f.requireLoadHolds(t, mockLog, holds, 1) + clock.Advance(fakeClockRetryDelay * 3 / 10) + f.requireLoadHolds(t, mockLog, holds, 1) + + f.write(t, yamlFlag3) + assert.Equal(t, []string{"flag3"}, flagKeys(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, 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. + f, second, mockLog, clock := newTwoFileFakeClockFixture(t) + f.write(t, "") + require.NoError(t, os.WriteFile(second, []byte("flagValues: ["), 0600)) + 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) + f.reloader.ReloadNow() + requireLogged(t, mockLog, ldlog.Debug, "Unable to load flags") + require.NoError(t, os.WriteFile(second, []byte(yamlFlag2), 0600)) + 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, 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 * 6) + 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 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. 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 6 { + 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) + } + + // 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 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. 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) + + 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 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 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 + // 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 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 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(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(false)) + clock.Advance(delay * 6 / 10) + 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 file's bound + // comes first. + for range 4 { + emptyInPlace(t, f.path) + assert.Equal(t, delay+delay/10, f.reloader.reload(false)) + clock.Advance(delay) + } + // 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(false)) + + f.write(t, "flagValues: [") + 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) { + const delay = fakeClockRetryDelay + 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: + } + } + 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, "") + f.requireLoadHolds(t, mockLog, 0, 1) +} + +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) { + // 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)) + require.Equal(t, []string{"flag1"}, flagKeys(f.requireLoadApplies(t))) + + require.NoError(t, os.Rename(second, f.path)) + 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.requireLoadHolds(t, mockLog, 0, 1) + f.write(t, yamlFlag2) + assert.Equal(t, []string{"flag2"}, flagKeys(f.requireLoadApplies(t))) +} 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..93f73041 --- /dev/null +++ b/ldclient_file_in_place_rewrite_test.go @@ -0,0 +1,171 @@ +package ldclient + +import ( + "os" + "path/filepath" + "sync" + "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" + "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. 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. +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) + mockLog, logging := debugLog() + config := Config{ + Logging: logging, + 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) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, holdBackLog), "no reload found the file empty") + }) + } +} + +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: logging, + 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) + assert.True(t, mockLog.HasMessageMatch(ldlog.Debug, holdBackLog), "no reload found the file empty") + }) + } +} + +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: logging, + 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) + 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 b86ab14c..013bdc46 100644 --- a/ldfiledata/package_info.go +++ b/ldfiledata/package_info.go @@ -25,6 +25,23 @@ // 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. +// // 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..63b85bb2 100644 --- a/ldfiledatav2/package_info.go +++ b/ldfiledatav2/package_info.go @@ -25,6 +25,23 @@ // 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. +// // 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..b06632a9 100644 --- a/ldoverrides/file_source_builder.go +++ b/ldoverrides/file_source_builder.go @@ -82,6 +82,24 @@ 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. +// // 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..ff81d9ff 100644 --- a/ldoverrides/file_source_test.go +++ b/ldoverrides/file_source_test.go @@ -360,6 +360,49 @@ 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]) + 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. + 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 + } + } + // 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") + }) + } + } +} + func TestFileSourcePollsByDefault(t *testing.T) { path := filepath.Join(t.TempDir(), "overrides.json") writeFile(t, path, `{}`)