From ce5a31438519760e70fc606e0812a78ce97183fa Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:33:09 +0000 Subject: [PATCH 1/8] fix: Keep overrides through an in-place rewrite of their files Applications and editors rewrite files in place: they truncate the file, then write the new content. A reload in between found the override file empty and applied it as a document with no entries, so the client briefly served LaunchDarkly data for every overridden flag. The parser is now chosen by the file name: a name that ends in .yaml or .yml, in any case, is YAML, and any other file is JSON. An empty or white space JSON file is not a JSON document and fails to parse, so the last good overrides stay. Files are read as UTF-8 without a byte order mark. A load that finds a file empty while the applied data has content from it puts the file off: it keeps the current data and arms the retry without reporting a failure. The file is put off for one retry delay from the first load that found it empty; a write while it is empty (a new rewrite, seen through the modification time) starts the delay again, up to five retry delays in all. A file that contributed no content to the applied data, such as an empty placeholder, is parsed at once. --- lib/ldclient-rb/impl/file_data/document.rb | 67 ++- lib/ldclient-rb/impl/file_data/reloader.rb | 117 ++++- lib/ldclient-rb/integrations/file_data.rb | 14 + spec/impl/file_data/document_spec.rb | 132 ++++- spec/impl/file_data/reloader_spec.rb | 479 +++++++++++++++++- spec/in_place_writes.rb | 22 + .../integrations/file_override_source_spec.rb | 142 +++++- 7 files changed, 931 insertions(+), 42 deletions(-) create mode 100644 spec/in_place_writes.rb diff --git a/lib/ldclient-rb/impl/file_data/document.rb b/lib/ldclient-rb/impl/file_data/document.rb index 53e820b9..7d925474 100644 --- a/lib/ldclient-rb/impl/file_data/document.rb +++ b/lib/ldclient-rb/impl/file_data/document.rb @@ -2,6 +2,7 @@ require "ldclient-rb/impl/model/serialization" +require "json" require "yaml" module LaunchDarkly @@ -62,20 +63,35 @@ def initialize(flags: {}, flag_values: {}, segments: {}) end # - # Parses the content of a data file. The content may be JSON or YAML. JSON is a subset of - # YAML, and the Ruby YAML parser handles it, so one parser serves both formats. + # Parses the content of a data file in the given format. {FileData.format_of} chooses the + # format of a file from its name. # - # An empty document is a document with no entries. A document that is not a mapping, or - # whose "flags", "flagValues", or "segments" member is not a mapping, is an error. + # Whether content with no entries is valid depends on the format, like the rest of the + # content. Content that is empty or only white space is not a JSON document, so it fails to + # parse. In YAML it is a document with no entries, and so is a YAML document that is only a + # null. A document that is not a mapping, or whose "flags", "flagValues", or "segments" + # member is not a mapping, is an error. # # @param content [String] + # @param format [Symbol] `:json` or `:yaml` # @return [Document] # @raise [ArgumentError] if the content is not a valid document - # @raise [Psych::SyntaxError] if the content cannot be parsed + # @raise [JSON::ParserError, Psych::SyntaxError] if the content cannot be parsed # - def self.parse(content) - raw = YAML.safe_load(content) - raw = {} if raw.nil? + def self.parse(content, format:) + case format + when :json + # A file that is being rewritten in place can be empty for a moment. Failing to parse + # keeps its last good data until it is complete. + raise ArgumentError, "the file has no content, which is not a JSON document" if FileData.blank?(content) + + raw = JSON.parse(content) + when :yaml + raw = YAML.safe_load(content) + raw = {} if raw.nil? + else + raise ArgumentError, "unknown file format #{format.inspect}" + end raise ArgumentError, "file content must be an object" unless raw.is_a?(Hash) data = FileData.symbolize_keys(raw) @@ -107,15 +123,44 @@ def self.read(path) end end + # The endings of the names of the files that are read as YAML, compared without regard to + # case. Every other file is read as JSON. + YAML_FILE_NAME_ENDINGS = [".yaml", ".yml"].freeze + + # The bytes that count as white space when deciding whether content is empty. + WHITE_SPACE_BYTES = [0x20, 0x09, 0x0a, 0x0d, 0x0b, 0x0c].freeze + private_constant :WHITE_SPACE_BYTES + + # + # Returns the format of a data file, which its name decides: a name that ends in ".yaml" or + # ".yml", in any case, is YAML, and any other name is JSON. + # + # @param path [String] + # @return [Symbol] `:json` or `:yaml` + # + def self.format_of(path) + path.downcase.end_with?(*YAML_FILE_NAME_ENDINGS) ? :yaml : :json + end + + # + # Returns true if the content is empty or only white space. + # + # @param content [String] + # @return [Boolean] + # + def self.blank?(content) + content.each_byte.all? { |byte| WHITE_SPACE_BYTES.include?(byte) } + end + # - # Reads the raw content of one file. + # Reads the raw content of one file as UTF-8, without a byte order mark. # # @param path [String] # @return [String] # @raise [ReadError] if the file cannot be read # def self.read_file(path) - File.read(path) + File.read(path, mode: "r:BOM|UTF-8") rescue Errno::ENOENT => e raise ReadError.new(path, "unable to read file: #{e.message}", missing: true) rescue SystemCallError, IOError => e @@ -131,7 +176,7 @@ def self.read_file(path) # @raise [ReadError] if the content cannot be parsed # def self.parse_file(path, content) - Document.parse(content) + Document.parse(content, format: format_of(path)) rescue StandardError => e raise ReadError.new(path, "error parsing file: #{e.message}") end diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index bbcd1497..e2a7bbb3 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -11,7 +11,24 @@ module FileData # # 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. + # failures, puts off a file that was emptied before applying it, and skips no-op + # applications. + # + # Applications and editors rewrite files in place: they truncate the file, then write the new + # content. A load in between finds the file empty. Whether an empty file is valid depends on + # its format (see {Document.parse}), so a parser cannot tell an emptied file from one in the + # middle of a rewrite. So a load that finds a file empty, or only white space, while the data + # currently applied has content from that file, puts the file off: it reads the other files, + # so that files emptied together are put off together, then keeps the current data and arms + # the retry, without reporting a failure. A file that cannot be read or parsed still fails the + # load. The file is put off for one retry delay from the time a load first found it empty, + # and a write to the file while it is empty, which is a new rewrite, starts the delay again. + # After the delay, or five retry delays after a load first found it empty however often it + # was rewritten, a load parses the empty file. The rule applies to every load, including a + # retry left over from an earlier failure. A file that contributed no content to the data + # currently applied, such as a placeholder that has been empty from the start, a file that was + # missing, or a file whose emptying has been applied, is parsed at once. So is every file when + # there are no automatic retries, because nothing would read it again. # # The worker thread starts on the first {#reload_now} or {#trigger} call rather than in the # constructor. A reloader can be constructed by a component whose lifecycle never uses it, @@ -29,6 +46,16 @@ class Reloader # file is cheap, so this can be short. In seconds. DEFAULT_RETRY_DELAY = 1.0 + # The longest a file that is found empty again and again is put off, in retry delays, + # counted from the first load that found it empty. + EMPTY_FILE_DEFERRAL_LIMIT = 5 + + # A file that loads have found empty while the applied data has content from it: when a + # load first found it empty, when the current delay started, and the file's modification + # time when a load last found it empty, or nil if it could not be read. + EmptyEpisode = Struct.new(:first_seen, :since, :modified) + private_constant :EmptyEpisode + # # @param paths [Array] the files to load, already resolved to absolute paths. The # order is significant: it determines which file wins under the duplicate keys handling. @@ -44,13 +71,14 @@ class Reloader # a {MergeError} when the documents could not be combined, or the exception that `apply` # raised. The reloader logs failures itself. # @param duplicate_keys_handling [Symbol] one of the {DuplicateKeysHandling} values - # @param skip_missing_paths [Boolean] when true, a configured file that does not exist is a - # file with no content, and the reload succeeds with the data of the files that exist. + # @param skip_missing_paths [Boolean] when true, a configured file that does not exist + # contributes no data, and the reload succeeds with the data of the files that exist. # When false, a missing file fails the reload like any other read error. # @param debounce_delay [Numeric] seconds to wait after a {#trigger} call for further calls # to settle before reloading. If zero or negative, each trigger reloads at once. # @param retry_delay [Numeric] seconds to wait after a failed reload before retrying - # automatically. If zero or negative, there is no automatic retry. + # automatically. A file that had content and is found empty is put off for the same delay. + # If zero or negative, there is no automatic retry, and an empty file is parsed as it is. # @param skip_unchanged [Boolean] if true, `apply` is not invoked when the files' raw # contents are byte-identical to the last successfully applied contents. # @param log_prefix [String] the prefix of every log line this reloader writes @@ -87,14 +115,19 @@ def initialize(paths:, logger:, apply:, on_error: nil, @reload_mutex = Mutex.new @last_good_digest = nil @last_error_message = nil + # Whether each configured path contributed content to the data currently applied, and the + # EmptyEpisode of each path that loads have found empty since. Only touched while holding + # @reload_mutex. + @applied_had_content = {} + @empty_episodes = {} end # # Synchronously loads the files and applies the result, or reports the failure. Use it for # the initial load. A failure here schedules the same automatic retry as a failed - # triggered reload. + # triggered reload, and so does a load that puts off a file it finds empty. # - # @return [Boolean] true if the load succeeded + # @return [Boolean] true if the load succeeded, false if it failed or put off an empty file # def reload_now ensure_started @@ -221,6 +254,8 @@ def stop documents = [] files = [] + had_content = {} + put_off = [] digest = Digest::SHA256.new @paths.each do |path| begin @@ -228,13 +263,29 @@ def stop # never disagree with the content that was applied. content = FileData.read_file(path) rescue ReadError => e + @empty_episodes.delete(path) if e.missing if e.missing && @skip_missing_paths @logger.debug { "#{@log_prefix} File #{path} does not exist; it contributes no data" } + had_content[path] = false files << FileSummary.new(path, false, 0, 0) next end return record_failure(e) end + if FileData.blank?(content) + # The other files are still read, so that files emptied together start their + # delays in the same load. + if put_off_empty_file?(path) + put_off << path + next + end + + had_content[path] = false + else + @empty_episodes.delete(path) + had_content[path] = true + end + digest << content << "\0" begin documents << FileData.parse_file(path, content) @@ -243,6 +294,7 @@ def stop end files << FileSummary.new(path, true, 0, 0) end + return put_off_empty_files(put_off) unless put_off.empty? begin merged = FileData.merge(documents, @@ -272,7 +324,10 @@ def stop # have moved to an interrupted state. Only apply tells it that things are good again. recovering = !@last_error_message.nil? hexdigest = digest.hexdigest - return true if @skip_unchanged && !recovering && hexdigest == @last_good_digest + if @skip_unchanged && !recovering && hexdigest == @last_good_digest + @applied_had_content = had_content + return true + end # Nothing is remembered until the consumer has accepted the result. A result that # apply rejects must not become the baseline that skip-unchanged compares against, @@ -284,10 +339,58 @@ def stop end @last_error_message = nil @last_good_digest = hexdigest + @applied_had_content = had_content true end end + # + # Decides whether a load that found the file empty puts it off, because the file may be in + # the middle of an in-place rewrite, and records the empty episode. Modification times are + # only compared with each other, never with the clock. + # + private def put_off_empty_file?(path) + return false if @retry_delay <= 0 || !@applied_had_content[path] + + now = monotonic_now + modified = begin + File.mtime(path) + rescue SystemCallError, IOError + nil + end + episode = @empty_episodes[path] + if episode.nil? + episode = EmptyEpisode.new(now, now, modified) + @empty_episodes[path] = episode + elsif modified != episode.modified + # The file was written since a load last found it empty: a new rewrite. + episode.since = now + episode.modified = modified + end + # After the delay the file is parsed. The episode stays, so later loads keep parsing it + # until it has content or is missing. The delay is checked with a tenth of slack, so that + # the retry that putting the file off arms, one retry delay later, parses it even if it + # comes slightly early. + now - episode.since < @retry_delay * 0.9 && now - episode.first_seen < EMPTY_FILE_DEFERRAL_LIMIT * @retry_delay + end + + # + # Puts off a load that found files empty. The current data stays, no failure is reported, + # and the false result arms the retry. + # + private def put_off_empty_files(paths) + # stop may have been called while the files were being read. Report success so that no + # retry is armed. + return true if stopped? + + paths.each do |path| + @logger.debug do + "#{@log_prefix} File #{path} is empty, possibly while it is being rewritten; reading it again before applying it" + end + end + false + end + private def record_failure(error) # stop may have been called while the files were being read. Deliver nothing then, and # report success so that no retry is armed. diff --git a/lib/ldclient-rb/integrations/file_data.rb b/lib/ldclient-rb/integrations/file_data.rb index 16a93d06..48f32376 100644 --- a/lib/ldclient-rb/integrations/file_data.rb +++ b/lib/ldclient-rb/integrations/file_data.rb @@ -177,6 +177,10 @@ def self.data_source_v2(options = {}) # entries are combined in the configured order, and the duplicate keys handling decides what # happens when the same key appears in more than one file. # + # A file whose name ends in `.yaml` or `.yml`, in any case, is read as YAML, and any other file is + # read as JSON. The file data source, by contrast, reads every file with the YAML parser, which + # also accepts most JSON. + # # A reload replaces the entire set of overrides, so removing an entry from a file removes the # override. A configured file that does not exist contributes no overrides. It can be created # later, and deleting a file removes its overrides. A file that exists but cannot be read or @@ -185,6 +189,16 @@ def self.data_source_v2(options = {}) # readable. Whenever the set of overrides in effect changes, including at startup, the source # logs the overrides in effect and what each file supplied, at Info level. # + # 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 JSON file with no content is not a valid document: it fails to + # load, and the overrides in effect stay. A file whose content is in effect and that is found + # empty may be in the middle of a rewrite, so the source waits about a second, and up to about + # five seconds while the file keeps being rewritten, before it reads the file as it is; an + # emptied YAML file removes its overrides about a second later. YAML that is cut off can still + # parse, with fewer entries or with a 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. + # # An evaluation that an override affects, directly or through a prerequisite or segment, is # marked: its reason reports {EvaluationReason#override_affected}, it produces no individual # analytics event, and it is counted under a separate summary counter. diff --git a/spec/impl/file_data/document_spec.rb b/spec/impl/file_data/document_spec.rb index b5a20703..647bcd2a 100644 --- a/spec/impl/file_data/document_spec.rb +++ b/spec/impl/file_data/document_spec.rb @@ -10,7 +10,7 @@ module FileData describe Document do describe "parse" do it "parses a JSON document with every section and symbolizes nested keys" do - document = Document.parse(<<~JSON) + document = Document.parse(<<~JSON, format: :json) { "flags": { "flag1": { "key": "flag1", "on": true, "rules": [ { "clauses": [ { "op": "in" } ] } ] } }, "flagValues": { "flag2": "value2" }, @@ -25,7 +25,7 @@ module FileData end it "parses a YAML document" do - document = Document.parse(<<~YAML) + document = Document.parse(<<~YAML, format: :yaml) --- flags: flag1: @@ -43,41 +43,72 @@ module FileData expect(document.segments.keys).to eq([:seg1]) end - it "treats an empty document as a document with no entries" do - document = Document.parse("") + it "treats empty YAML content as a document with no entries" do + ["", " \n\r\n", "# a comment\n", "---\n", "~\n"].each do |content| + document = Document.parse(content, format: :yaml) - expect(document.flags).to eq({}) - expect(document.flag_values).to eq({}) - expect(document.segments).to eq({}) + expect(document.flags).to eq({}) + expect(document.flag_values).to eq({}) + expect(document.segments).to eq({}) + end + end + + it "rejects empty JSON content, which is not a JSON document" do + ["", " \n\r\n\t"].each do |content| + expect { Document.parse(content, format: :json) } + .to raise_error(ArgumentError, "the file has no content, which is not a JSON document") + end end it "treats a document with no known sections as a document with no entries" do - document = Document.parse("{}") + [[:json, "{}"], [:yaml, "{}"], [:yaml, "other: 1\n"]].each do |format, content| + document = Document.parse(content, format: format) - expect(document.flags).to eq({}) - expect(document.flag_values).to eq({}) - expect(document.segments).to eq({}) + expect(document.flags).to eq({}) + expect(document.flag_values).to eq({}) + expect(document.segments).to eq({}) + end end it "rejects a document that is not an object" do - expect { Document.parse("\"hello\"") }.to raise_error(ArgumentError, /must be an object/) - expect { Document.parse("[1, 2]") }.to raise_error(ArgumentError, /must be an object/) + [:json, :yaml].each do |format| + expect { Document.parse("\"hello\"", format: format) }.to raise_error(ArgumentError, /must be an object/) + expect { Document.parse("[1, 2]", format: format) }.to raise_error(ArgumentError, /must be an object/) + end + expect { Document.parse("null", format: :json) }.to raise_error(ArgumentError, /must be an object/) + expect { Document.parse("false", format: :yaml) }.to raise_error(ArgumentError, /must be an object/) end it "rejects a section that is not an object" do - expect { Document.parse('{"flags": []}') }.to raise_error(ArgumentError, /"flags" must be an object/) - expect { Document.parse('{"flagValues": 3}') }.to raise_error(ArgumentError, /"flagValues" must be an object/) - expect { Document.parse('{"segments": "x"}') }.to raise_error(ArgumentError, /"segments" must be an object/) + expect { Document.parse('{"flags": []}', format: :json) }.to raise_error(ArgumentError, /"flags" must be an object/) + expect { Document.parse('{"flagValues": 3}', format: :json) }.to raise_error(ArgumentError, /"flagValues" must be an object/) + expect { Document.parse('{"segments": "x"}', format: :json) }.to raise_error(ArgumentError, /"segments" must be an object/) end it "symbolizes keys that YAML parsed as numbers" do - document = Document.parse("flagValues:\n 123: true\n") + document = Document.parse("flagValues:\n 123: true\n", format: :yaml) expect(document.flag_values).to eq({ "123": true }) end + it "reads JSON numbers as JSON does" do + document = Document.parse('{"flagValues": {"flag1": 1e3, "flag2": 12}}', format: :json) + + expect(document.flag_values).to eq({ flag1: 1000.0, flag2: 12 }) + end + it "raises a syntax error for content that cannot be parsed" do - expect { Document.parse('{"flagValues"') }.to raise_error(Psych::SyntaxError) + expect { Document.parse('{"flagValues"', format: :json) }.to raise_error(JSON::ParserError) + expect { Document.parse('{"flagValues": {}} extra', format: :json) }.to raise_error(JSON::ParserError) + expect { Document.parse("flagValues: [", format: :yaml) }.to raise_error(Psych::SyntaxError) + end + + it "rejects a YAML document as JSON" do + expect { Document.parse("flagValues:\n flag1: true\n", format: :json) }.to raise_error(JSON::ParserError) + end + + it "rejects an unknown format" do + expect { Document.parse("{}", format: :toml) }.to raise_error(ArgumentError, /unknown file format/) end end @@ -98,6 +129,51 @@ module FileData expect(document.flag_values).to eq({ flag1: 1 }) end + it "reads a file as YAML when its name ends in .yaml or .yml, in any case" do + ["flags.yaml", "flags.yml", "flags.YAML", "flags.Yml"].each do |name| + path = File.join(@dir, name) + File.write(path, "flagValues:\n flag1: true\n") + + expect(Document.read(path).flag_values).to eq({ flag1: true }) + end + end + + it "reads any other file as JSON" do + ["flags.json", "flags", "flags.txt", "flags.yaml.json"].each do |name| + path = File.join(@dir, name) + File.write(path, "flagValues:\n flag1: true\n") + + expect { Document.read(path) }.to raise_error(ReadError, /error parsing file/) + end + end + + it "reports an empty file as a read error when it is JSON, and as no entries when it is YAML" do + ["flags.json", "flags"].each do |name| + path = File.join(@dir, name) + File.write(path, "") + + expect { Document.read(path) }.to raise_error(ReadError, /the file has no content, which is not a JSON document/) + end + + path = File.join(@dir, "flags.yaml") + File.write(path, "") + expect(Document.read(path).flag_values).to eq({}) + end + + it "reads a file that starts with a byte order mark" do + path = File.join(@dir, "flags.json") + File.binwrite(path, "\xEF\xBB\xBF{\"flagValues\": {\"flag1\": \"caf\xC3\xA9\"}}") + + expect(Document.read(path).flag_values).to eq({ flag1: "caf\u00e9" }) + end + + it "reads a file that holds only a byte order mark as empty" do + path = File.join(@dir, "flags.json") + File.binwrite(path, "\xEF\xBB\xBF") + + expect(FileData.read_file(path)).to eq("") + end + it "reports a missing file as a read error that is marked missing" do path = File.join(@dir, "no-such-file.json") @@ -135,6 +211,26 @@ module FileData end end + describe "format_of" do + it "chooses YAML for a name that ends in .yaml or .yml, in any case, and JSON otherwise" do + expect(FileData.format_of("/a/flags.yaml")).to eq(:yaml) + expect(FileData.format_of("/a/flags.YML")).to eq(:yaml) + expect(FileData.format_of("/a/flags.json")).to eq(:json) + expect(FileData.format_of("/a.yaml/flags")).to eq(:json) + expect(FileData.format_of("/a/flags.yaml.bak")).to eq(:json) + end + end + + describe "blank?" do + it "is true for content that is empty or only white space" do + expect(FileData.blank?("")).to be true + expect(FileData.blank?(" \t\r\n\f\v")).to be true + expect(FileData.blank?(" {}")).to be false + expect(FileData.blank?("# comment")).to be false + expect(FileData.blank?("\xFF".b)).to be false + end + end + describe "make_flag_with_value" do it "builds a flag that is on and serves the value as its fallthrough" do flag = FileData.make_flag_with_value("flag1", "value1") diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index 6734f15e..1ef88a2f 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -2,6 +2,7 @@ require "spec_helper" require "capturing_logger" +require "in_place_writes" require "tmpdir" require "ldclient-rb/impl/file_data" @@ -20,8 +21,15 @@ def path(name) File.join(@dir, name) end + # Applications rewrite data files in place, which is not atomic. The tests do too, deliberately + # slowly: a file that exists is truncated, and written 30 ms later, so a load can find it + # empty, as it would with an application's writes, instead of only by chance. def write(name, content) - File.write(path(name), content) + if File.exist?(path(name)) + InPlaceWrites.rewrite(path(name), content, 0.03) + else + File.write(path(name), content) + end path(name) end @@ -197,11 +205,11 @@ def with_reloader(paths, logger: $null_log, reject: 0, **options) expect(reloader.reload_now).to be false write("a.json", values_doc({ flag1: "fixed" })) - # The retry recovers. The write is ordinary, not atomic, so the retry may read the - # file while it is still empty: an empty document is valid and is applied as the - # recovery, and the fixed content is then loaded on demand. + # The retry recovers with the fixed content. The write is in place, so a retry can + # find the file empty while it is being written. It reads the file again later + # instead of applying it. expect(wait_for { recorder.applied.length >= 1 }).to be true - expect(reloader.reload_now).to be true if recorder.flag_values != { flag1: "fixed" } + expect(recorder.applied.map { |merged| merged.flags.keys }).to all(eq([:flag1])) expect(recorder.flag_values).to eq({ flag1: "fixed" }) end end @@ -452,6 +460,467 @@ def with_reloader(paths, logger: $null_log, reject: 0, **options) expect(logger.output).to match(/ERROR -- : \[LDClient\] Prefix: Unexpected error in file data reloader/) end end + + describe "a file that is found empty" do + # The retry comes long enough after the change signal for a test to finish writing the + # file before it, even on a slow machine. + let(:retry_delay) { 1.5 } + + # Shorter than the retry delay, so a load that happens within it after a change signal + # is not the retry. + let(:before_the_retry) { 1.0 } + + let(:logger) { CapturingLogger.new } + + let(:read_again_log) { "reading it again before applying it" } + + def yaml_doc(values) + "flagValues:\n" + values.map { |key, value| " #{key}: #{value}\n" }.join + end + + def read_again_logged? + logger.output.include?(read_again_log) + end + + def with_empty_file_reloader(paths, **options, &block) + with_reloader(paths, logger: logger, debounce_delay: 0.01, retry_delay: retry_delay, **options, &block) + end + + it "is read again before it is applied" do + # A writer that rewrites the file in place truncates it and then writes the new content. + # In YAML an empty file is valid, so applying it would briefly remove every entry. + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + expect(logger.output).to include("DEBUG -- : [LDClient] File #{y} is empty, possibly while it is being rewritten") + + # The retry reads the rewritten file. + File.write(y, yaml_doc(flag2: true)) + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag2: true }) + expect(recorder.errors).to be_empty + expect(logger.output).not_to include("ERROR") + end + end + + it "counts a file with only white space as empty" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, " \n\r\n \n") + reloader.trigger + + expect(wait_for { read_again_logged? }).to be true + expect(recorder.applied.length).to eq(1) + end + end + + it "is applied by the retry when a YAML file stays empty" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + expect(wait_for(before_the_retry) { recorder.applied.length > 1 }).to be false + + # The file is still empty when the retry reads it, so it is a document with no entries. + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + expect(recorder.applied[-1].empty?).to be true + expect(recorder.errors).to be_empty + end + end + + it "is not put off again once its empty state is applied" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + + # The applied data has no content from the file, so a new truncation is applied at once. + File.write(y, "") + reloader.trigger + expect(wait_for(before_the_retry) { recorder.applied.length == 3 }).to be true + expect(recorder.applied[-1].empty?).to be true + expect(logger.output.scan(read_again_log).length).to eq(1) + end + end + + it "is not reported by the load that finds an emptied JSON file, and the retry reports it" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_empty_file_reloader([a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + expect(recorder.errors).to be_empty + + # The retry finds the file still empty, which is not a JSON document. The data stays. + expect(wait_for(retry_delay + 2) { recorder.errors.length == 1 }).to be true + expect(recorder.errors[0]).to be_a(ReadError) + expect(recorder.errors[0].message).to include("the file has no content, which is not a JSON document") + expect(recorder.applied.length).to eq(1) + end + end + + it "is not applied by a further change signal within the retry delay" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + + # The file has not been written since, so the delay goes on from the first load that + # found it empty, and a stream of change signals cannot hold it back longer. + reloader.trigger + expect(wait_for { logger.output.scan(read_again_log).length == 2 }).to be true + expect(recorder.applied.length).to eq(1) + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "is put off again when a second rewrite truncates it before a load reads its content" do + # Two quick in-place rewrites: the write of the first and the truncation of the second + # come before the next load, which finds the file empty again. It is a new rewrite, so + # the delay starts again. + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + + File.write(y, yaml_doc(flag2: true)) + File.write(y, "") + reloader.trigger + expect(wait_for { logger.output.scan(read_again_log).length == 2 }).to be true + + File.write(y, yaml_doc(flag3: true)) + reloader.trigger + expect(wait_for(before_the_retry) { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag3: true }) + end + end + + it "is put off again when the retry finds a second truncation" do + # A retry, or the next poll, can find the file empty again after a second rewrite started. + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + + # No change signal from here on: only the retries read the file. + File.write(y, yaml_doc(flag2: true)) + File.write(y, "") + expect(wait_for(retry_delay + 2) { logger.output.scan(read_again_log).length == 2 }).to be true + expect(recorder.applied.length).to eq(1) + + File.write(y, yaml_doc(flag3: true)) + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag3: true }) + end + end + + it "is put off again when a change to another file starts a load within the delay" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_empty_file_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + + # The load for the change to b finds a still empty, within the delay. + File.write(b, yaml_doc(flag2: false)) + reloader.trigger + expect(wait_for { logger.output.scan(read_again_log).length == 2 }).to be true + expect(recorder.applied.length).to eq(1) + + File.write(a, yaml_doc(flag1: false)) + reloader.trigger + expect(wait_for(before_the_retry) { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag1: false, flag2: false }) + end + end + + it "starts the delay afresh for a rewrite after a load has read its content" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.3) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + File.write(y, yaml_doc(flag2: true)) + reloader.trigger + expect(wait_for { recorder.applied.length == 2 }).to be true + + # Longer than five retry delays after a load first found the file empty. That emptying + # is over, so a new one is put off too. + sleep 1.6 + File.write(y, "") + reloader.trigger + expect(wait_for { logger.output.scan(read_again_log).length == 2 || recorder.applied.length == 3 }).to be true + expect(logger.output.scan(read_again_log).length).to eq(2) + end + end + + it "starts the delay afresh for a file that is created empty after it was missing" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.3) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + + # Without skipping missing paths, the loads fail while the file is missing, and the data + # with the file's content stays. + File.delete(y) + expect(wait_for { recorder.errors.length == 1 }).to be true + sleep 1.6 + File.write(y, "") + expect(wait_for { logger.output.scan(read_again_log).length == 2 || recorder.applied.length == 2 }).to be true + expect(logger.output.scan(read_again_log).length).to eq(2) + end + end + + it "keeps parsing a JSON file that stays empty without putting it off again" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_reloader([a], logger: logger, debounce_delay: 0.01, retry_delay: 0.3) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + reloader.trigger + expect(wait_for { recorder.errors.length == 1 }).to be true + + # The retries go on parsing the file, which fails each time, and the data stays. + expect(wait_for { logger.output.scan("the file has no content").length >= 4 }).to be true + expect(logger.output.scan(read_again_log).length).to eq(1) + expect(recorder.applied.length).to eq(1) + end + end + + it "is put off when the data that was kept as unchanged has content from it" do + # The same content moves from one file to another, so the reload is skipped as unchanged, + # but the data in effect now comes from the second file. + a = write("a.yaml", yaml_doc(flag1: true)) + b = path("b.yaml") + + with_empty_file_reloader([a, b], skip_missing_paths: true, skip_unchanged: true) do |reloader, recorder| + expect(reloader.reload_now).to be true + File.write(b, yaml_doc(flag1: true)) + File.delete(a) + expect(reloader.reload_now).to be true + expect(recorder.applied.length).to eq(1) + + File.write(b, "") + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "is put off together with another file that is emptied at the same time" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_empty_file_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + File.write(b, "") + reloader.trigger + + # One load puts both off, so both are applied by the first retry, not one retry delay + # after the other. + expect(wait_for { logger.output.scan(read_again_log).length == 2 }).to be true + expect(logger.output).to include("File #{a} is empty").and include("File #{b} is empty") + expect(wait_for(retry_delay + 0.8) { recorder.applied.length == 2 }).to be true + expect(recorder.applied[-1].empty?).to be true + expect(logger.output.scan(read_again_log).length).to eq(2) + end + end + + it "does not keep a load from failing on another file that cannot be parsed" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.json", values_doc({ flag2: "b" })) + + with_empty_file_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + File.write(b, '{"flagValues"') + reloader.trigger + + # The load that puts a off reports b's failure, before the retry. + expect(wait_for(before_the_retry) { recorder.errors.length == 1 }).to be true + expect(recorder.errors[0].path).to eq(b) + expect(recorder.applied.length).to eq(1) + end + end + + it "is applied after five retry delays when every load finds it empty again after a rewrite" do + y = write("a.yaml", yaml_doc(flag1: true)) + + # A shorter retry delay keeps the test short. The file is truncated again every 50 ms, + # which changes its modification time, so each load finds a new rewrite. + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.5) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { read_again_logged? }).to be true + first_found_empty = Time.now + + until recorder.applied.length == 2 || Time.now - first_found_empty > 6 + sleep 0.05 + File.write(y, "") + reloader.trigger + end + + expect(recorder.applied.length).to eq(2) + expect(recorder.applied[-1].empty?).to be true + expect(Time.now - first_found_empty).to be >= 2.3 + expect(logger.output.scan(read_again_log).length).to be > 5 + end + end + + it "does not delay the other files' changes when it is an empty placeholder from the start" do + # An empty YAML file next to the data, such as a placeholder for later entries, has never + # had content, so it is never read again. + a = write("a.json", values_doc({ flag1: "a" })) + placeholder = write("placeholder.yaml", "") + + with_empty_file_reloader([a, placeholder]) do |reloader, recorder| + reloader.reload_now + expect(recorder.flag_values).to eq({ flag1: "a" }) + + File.write(a, values_doc({ flag1: "b" })) + reloader.trigger + expect(wait_for(before_the_retry) { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag1: "b" }) + expect(read_again_logged?).to be false + end + end + + it "is applied at once when it was missing at its last read" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y], skip_missing_paths: true) do |reloader, recorder| + reloader.reload_now + File.delete(y) + reloader.trigger + expect(wait_for { recorder.applied.length == 2 }).to be true + expect(recorder.applied[-1].empty?).to be true + + # A missing file has no content, so the empty file that replaces it is not read again. + File.write(y, "") + reloader.trigger + expect(wait_for(before_the_retry) { recorder.applied.length == 3 }).to be true + expect(recorder.applied[-1].empty?).to be true + expect(read_again_logged?).to be false + end + end + + it "is read again by a retry left over from an earlier failure instead of applied empty" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "flagValues: [") + reloader.trigger + expect(wait_for { recorder.errors.length == 1 }).to be true + + # No change signal from here on: only the retries read the file. + File.write(y, "") + expect(wait_for(retry_delay + 2) { read_again_logged? }).to be true + File.write(y, yaml_doc(flag2: true)) + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag2: true }) + end + end + + it "makes reload_now return false and arms the retry" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + expect(reloader.reload_now).to be true + File.write(y, "") + + expect(reloader.reload_now).to be false + expect(read_again_logged?).to be true + expect(recorder.errors).to be_empty + + # The retry applies the file, which is still empty. + expect(wait_for(retry_delay + 2) { recorder.applied.length == 2 }).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "neither logs nor arms a retry when the reloader is stopped while the files are read" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_empty_file_reloader([y]) do |reloader, recorder| + reloader.reload_now + allow(FileData).to receive(:read_file).and_wrap_original do |_original, _path| + reloader.stop + "" + end + + expect(reloader.reload_now).to be true + expect(read_again_logged?).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "is applied at once when there are no automatic retries" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_reloader([y], logger: logger, retry_delay: 0) do |reloader, recorder| + reloader.reload_now + + # Without a retry nothing would read the file again, so the load applies it as it is. + File.write(y, "") + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + expect(read_again_logged?).to be false + end + end + + it "is a document with no entries at the initial load when it is YAML" do + y = write("a.yaml", "") + + with_empty_file_reloader([y]) do |reloader, recorder| + expect(reloader.reload_now).to be true + expect(recorder.applied[0].empty?).to be true + end + end + + it "fails to parse at the initial load when it is JSON" do + a = write("a.json", "") + + with_empty_file_reloader([a]) do |reloader, recorder| + expect(reloader.reload_now).to be false + expect(recorder.applied).to be_empty + expect(recorder.errors[0].message).to include("the file has no content, which is not a JSON document") + end + end + end end end end diff --git a/spec/in_place_writes.rb b/spec/in_place_writes.rb new file mode 100644 index 00000000..26fecdca --- /dev/null +++ b/spec/in_place_writes.rb @@ -0,0 +1,22 @@ +# +# Rewrites files the way applications and editors often do: in place, which is not atomic. The file +# is truncated first, and the new content is written after a pause, so that a reader in between +# finds the file empty, as it can with an application's writes, instead of only by chance. +# +module InPlaceWrites + # + # Truncates the file, waits for the pause, then writes the content. A file that does not exist + # is created empty first. + # + # @param path [String] + # @param content [String] + # @param pause [Numeric] seconds between truncating the file and writing the content + # + def self.rewrite(path, content, pause) + # Opening with "w" truncates the file at once. + File.open(path, "w") do |file| + sleep pause + file.write(content) + end + end +end diff --git a/spec/integrations/file_override_source_spec.rb b/spec/integrations/file_override_source_spec.rb index c0278ac6..d71c3ba6 100644 --- a/spec/integrations/file_override_source_spec.rb +++ b/spec/integrations/file_override_source_spec.rb @@ -2,6 +2,7 @@ require "spec_helper" require "capturing_logger" +require "in_place_writes" require "model_builders" require "override_test_components" require "tmpdir" @@ -50,11 +51,22 @@ def path(name) File.join(@dir, name) end + # Applications rewrite override files in place, which is not atomic. The tests do too, + # deliberately slowly: a file that exists is truncated, and written 30 ms later, so a load can + # find it empty, as it would with an application's writes, instead of only by chance. def write(name, content) - File.write(path(name), content) + if File.exist?(path(name)) + InPlaceWrites.rewrite(path(name), content, 0.03) + else + File.write(path(name), content) + end path(name) end + def yaml_values_doc(values) + "flagValues:\n" + values.map { |key, value| " #{key}: #{value}\n" }.join + end + def values_doc(values) { flagValues: values }.to_json end @@ -426,6 +438,100 @@ def with_source(paths, **options) end end + describe "in-place rewrites" do + # A writer that rewrites a file in place truncates it, and writes the new content after this + # pause. A load in between finds the file empty. + let(:pause) { 0.3 } + + before do + # The retry comes long enough after a load that finds the file empty for the rewrite to + # finish before it, even on a slow machine. + stub_const("LaunchDarkly::Impl::FileData::Reloader::DEFAULT_RETRY_DELAY", 1.5) + end + + [:polling, :watching].each do |mode| + context "with #{mode} change detection" do + before do + skip "the listen gem is not installed" if mode == :watching && !Impl::FileData::Watcher.available? + end + + it "never supplies a snapshot without the override while a JSON file is rewritten" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_source([a], change_detection: mode) do |_source, sink| + sleep 0.3 if mode == :watching + ["b", "c"].each do |value| + InPlaceWrites.rewrite(a, values_doc({ flag1: value }), pause) + expect(wait_for { sink.flag_values == { "flag1" => value } }).to be true + end + + expect(sink.snapshots.map { |flags, _segments| flags.map(&:key) }).to all(eq(["flag1"])) + expect(logger.output).not_to include("ERROR") + end + end + + it "never supplies a snapshot without the override while a YAML file is rewritten" do + a = write("a.yaml", yaml_values_doc({ flag1: "a" })) + + with_source([a], change_detection: mode) do |_source, sink| + sleep 0.3 if mode == :watching + ["b", "c"].each do |value| + InPlaceWrites.rewrite(a, yaml_values_doc({ flag1: value }), pause) + expect(wait_for { sink.flag_values == { "flag1" => value } }).to be true + end + + expect(sink.snapshots.map { |flags, _segments| flags.map(&:key) }).to all(eq(["flag1"])) + end + end + end + end + + it "keeps the overrides in effect when a JSON file stays empty" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_source([a]) do |_source, sink| + File.write(a, "") + + expect(wait_for { logger.output.include?("the file has no content, which is not a JSON document") }).to be true + expect(sink.snapshots.length).to eq 1 + expect(sink.flag_values).to eq({ "flag1" => "a" }) + end + end + + it "removes the overrides of a YAML file that stays empty after the retry delay" do + a = write("a.yaml", yaml_values_doc({ flag1: "a" })) + + with_source([a]) do |_source, sink| + File.write(a, "") + + expect(wait_for { sink.snapshots.length == 2 }).to be true + expect(sink.snapshots[-1]).to eq [[], []] + expect(logger.output).to include("reading it again before applying it") + end + end + end + + describe "in-place rewrites with the default timing" do + it "never supplies a snapshot without the override while a YAML file is rewritten about once a second" do + # Polling once a second, a poll, or the retry, can find the second rewrite's truncation + # before any load has read the first rewrite's content. + a = write("a.yaml", yaml_values_doc({ flag1: "v0" })) + source = build(paths: [a]) + sink = RecordingSink.new + source.start(sink) + begin + (1..5).each do |i| + InPlaceWrites.rewrite(a, yaml_values_doc({ flag1: "v#{i}" }), 0.3) + sleep 0.7 + end + expect(wait_for { sink.flag_values == { "flag1" => "v5" } }).to be true + expect(sink.snapshots.map { |flags, _segments| flags.map(&:key) }).to all(eq(["flag1"])) + ensure + source.stop + end + end + end + describe "with a client" do let(:context) { LDContext.create({ key: "user-key", kind: "user" }) } let(:ld_flag) { { key: "flag", version: 100, on: false, offVariation: 0, variations: ["ld-value"] } } @@ -478,6 +584,40 @@ def with_client(overrides) end end + [ + ["JSON", "a.json", ->(value) { { flagValues: { flag: value } }.to_json }], + ["YAML", "a.yaml", ->(value) { "flagValues:\n flag: #{value}\n" }], + ].each do |format, name, doc| + it "serves the override throughout an in-place rewrite of a #{format} file" do + stub_const("LaunchDarkly::Impl::FileData::Reloader::DEFAULT_RETRY_DELAY", 1.5) + a = write(name, doc.call("override-a")) + change_detection = Impl::FileData::Watcher.available? ? :watching : :polling + source = make_source([a], change_detection: change_detection) + builder = Object.new + builder.define_singleton_method(:build) { |_sdk_key, _config| source } + + with_client(builder) do |client| + sleep 0.3 if change_detection == :watching + served = [] + sampling = true + sampler = Thread.new do + while sampling + value = client.variation("flag", context, "default") + served << value if served.last != value + sleep 0.002 + end + end + + InPlaceWrites.rewrite(a, doc.call("override-b"), 0.3) + expect(wait_for { served.last == "override-b" }).to be true + sampling = false + sampler.join + + expect(served).to eq ["override-a", "override-b"] + end + end + end + it "reports invalid options from client construction" do data_system = DataSystem.custom.overrides(FileData.override_source(paths: [])).build From 7c34a2e83cb204e5c28baf951d3969c6756458af Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 01:15:11 +0000 Subject: [PATCH 2/8] fix: Hold back YAML with no document, and bound holds by the run of holding loads A file whose content is in effect and that a load finds with no content is held back. Four cases still let a rewrite through or held too long: - A read of a YAML file that sees only a comment or "---" is not white space, but the parser reads it as nothing, and it was applied as no entries. Content that the YAML parser reads as a null now counts as no content. "{}" is a document and is applied at once. - The five retry delay cap was kept per file and restarted whenever the file was read with content, so files rewritten in turn held every load. The cap now applies to the run of loads that hold files back, which any load that holds nothing back ends, including a failed one. - The delay check had a tenth of slack, so another load could apply a file a little before its delay was over. The check is now exact, and the retry of a hold is armed for the rest of the first held file's delay, or of the run's limit, plus a tenth of the retry delay. - The file's state is now its modification time and size, read through a symbolic link with the poller's helper. The tests of the delays run on a controlled clock with synchronous loads. --- lib/ldclient-rb/impl/file_data/document.rb | 12 +- lib/ldclient-rb/impl/file_data/poller.rb | 22 +- lib/ldclient-rb/impl/file_data/reloader.rb | 366 +++++++------- lib/ldclient-rb/integrations/file_data.rb | 12 +- spec/impl/file_data/document_spec.rb | 13 + spec/impl/file_data/poller_spec.rb | 12 + spec/impl/file_data/reloader_spec.rb | 445 +++++++++++++----- spec/in_place_writes.rb | 17 +- .../integrations/file_override_source_spec.rb | 16 + 9 files changed, 621 insertions(+), 294 deletions(-) diff --git a/lib/ldclient-rb/impl/file_data/document.rb b/lib/ldclient-rb/impl/file_data/document.rb index 7d925474..9c4990a2 100644 --- a/lib/ldclient-rb/impl/file_data/document.rb +++ b/lib/ldclient-rb/impl/file_data/document.rb @@ -62,15 +62,19 @@ def initialize(flags: {}, flag_values: {}, segments: {}) @segments = segments end + # The document of YAML content that is only a null, such as an empty file, a comment, or + # "---": a document with no entries. + NONE = new(flags: {}.freeze, flag_values: {}.freeze, segments: {}.freeze).freeze + # # Parses the content of a data file in the given format. {FileData.format_of} chooses the # format of a file from its name. # # Whether content with no entries is valid depends on the format, like the rest of the # content. Content that is empty or only white space is not a JSON document, so it fails to - # parse. In YAML it is a document with no entries, and so is a YAML document that is only a - # null. A document that is not a mapping, or whose "flags", "flagValues", or "segments" - # member is not a mapping, is an error. + # parse. In YAML it is a document with no entries, {NONE}, and so is a YAML document that is + # only a null, such as one with only a comment. A document that is not a mapping, or whose + # "flags", "flagValues", or "segments" member is not a mapping, is an error. # # @param content [String] # @param format [Symbol] `:json` or `:yaml` @@ -88,7 +92,7 @@ def self.parse(content, format:) raw = JSON.parse(content) when :yaml raw = YAML.safe_load(content) - raw = {} if raw.nil? + return NONE if raw.nil? else raise ArgumentError, "unknown file format #{format.inspect}" end diff --git a/lib/ldclient-rb/impl/file_data/poller.rb b/lib/ldclient-rb/impl/file_data/poller.rb index b0ce08fc..73d9db5e 100644 --- a/lib/ldclient-rb/impl/file_data/poller.rb +++ b/lib/ldclient-rb/impl/file_data/poller.rb @@ -59,14 +59,20 @@ def stop # @return [Array] # def self.observe_all(paths) - paths.map do |path| - begin - stat = File.stat(path) - FileState.new(true, stat.mtime, stat.size) - rescue SystemCallError - ABSENT - end - end + paths.map { |path| observe(path) } + end + + # + # Examines one file. The state is that of the file a symbolic link leads to. + # + # @param path [String] + # @return [FileState] + # + def self.observe(path) + stat = File.stat(path) + FileState.new(true, stat.mtime, stat.size) + rescue SystemCallError + ABSENT end private def examine diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index e2a7bbb3..64cd959b 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -2,6 +2,7 @@ require "ldclient-rb/impl/file_data/document" require "ldclient-rb/impl/file_data/merge" +require "ldclient-rb/impl/file_data/poller" require "digest" @@ -11,24 +12,32 @@ module FileData # # 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, puts off a file that was emptied before applying it, and skips no-op + # failures, holds back a file that was emptied before applying it, and skips no-op # applications. # # Applications and editors rewrite files in place: they truncate the file, then write the new - # content. A load in between finds the file empty. Whether an empty file is valid depends on - # its format (see {Document.parse}), so a parser cannot tell an emptied file from one in the - # middle of a rewrite. So a load that finds a file empty, or only white space, while the data - # currently applied has content from that file, puts the file off: it reads the other files, - # so that files emptied together are put off together, then keeps the current data and arms - # the retry, without reporting a failure. A file that cannot be read or parsed still fails the - # load. The file is put off for one retry delay from the time a load first found it empty, - # and a write to the file while it is empty, which is a new rewrite, starts the delay again. - # After the delay, or five retry delays after a load first found it empty however often it - # was rewritten, a load parses the empty file. The rule applies to every load, including a - # retry left over from an earlier failure. A file that contributed no content to the data - # currently applied, such as a placeholder that has been empty from the start, a file that was - # missing, or a file whose emptying has been applied, is parsed at once. So is every file when - # there are no automatic retries, because nothing would read it again. + # content. A load in between finds the file empty, or, for YAML, can find only a comment or + # "---", which the YAML parser reads as nothing. Whether a file with no entries is valid + # depends on its format (see {Document.parse}), so a parser cannot tell an emptied file from + # one in the middle of a rewrite. So a load that finds a file with no content (empty, white + # space, or YAML that is only a null), while the data currently applied has content from that + # file, holds the file back: it reads the other files, so that files emptied together are + # held back together, then keeps the current data and arms the retry, without reporting a + # failure. A file that cannot be read or parsed still fails the load. A YAML document such as + # `{}` is content, and is applied at once. + # + # A file is held back until it has stayed so, without being written, for the retry delay. A + # change of its state (its modification time and size, read through any symbolic link) means + # 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 in a row that hold files back stop doing + # so after five retry delays, so that a file that keeps being rewritten, or files rewritten in + # turn, cannot hold the data back without end. The retry that a hold arms comes when the first + # held file's delay is over, or when the run of holding loads reaches its limit, plus a tenth + # of the retry delay. A file that contributed no content to the data currently applied, such + # as a placeholder that has been empty from the start, a file that was missing, or a file + # whose emptying has been applied, is parsed at once. So is every file when there are no + # automatic retries, because nothing would read it again. After the delay, an empty JSON file + # fails to parse and the data stays; an empty YAML file is a document with no entries. # # The worker thread starts on the first {#reload_now} or {#trigger} call rather than in the # constructor. A reloader can be constructed by a component whose lifecycle never uses it, @@ -46,15 +55,14 @@ class Reloader # file is cheap, so this can be short. In seconds. DEFAULT_RETRY_DELAY = 1.0 - # The longest a file that is found empty again and again is put off, in retry delays, - # counted from the first load that found it empty. - EMPTY_FILE_DEFERRAL_LIMIT = 5 + # Loads in a row stop holding files back once they have held some back for this many retry + # delays. + MAX_HOLD_RETRY_DELAYS = 5 - # A file that loads have found empty while the applied data has content from it: when a - # load first found it empty, when the current delay started, and the file's modification - # time when a load last found it empty, or nil if it could not be read. - EmptyEpisode = Struct.new(:first_seen, :since, :modified) - private_constant :EmptyEpisode + # A file whose content is in effect and that loads have found with no content: since when it + # has been so without being written, and its {Poller::FileState} then. + EmptyFile = Struct.new(:since, :state) + private_constant :EmptyFile # # @param paths [Array] the files to load, already resolved to absolute paths. The @@ -77,11 +85,14 @@ class Reloader # @param debounce_delay [Numeric] seconds to wait after a {#trigger} call for further calls # to settle before reloading. If zero or negative, each trigger reloads at once. # @param retry_delay [Numeric] seconds to wait after a failed reload before retrying - # automatically. A file that had content and is found empty is put off for the same delay. - # If zero or negative, there is no automatic retry, and an empty file is parsed as it is. + # automatically. A file whose content is in effect and that is found empty is held back + # for the same delay. If zero or negative, there is no automatic retry, and an empty file + # is parsed as it is. # @param skip_unchanged [Boolean] if true, `apply` is not invoked when the files' raw # contents are byte-identical to the last successfully applied contents. # @param log_prefix [String] the prefix of every log line this reloader writes + # @param clock [#call, nil] returns the seconds of a monotonic clock, for the time a file has + # been held back; nil for the system's. Scheduling always uses the system's clock. # def initialize(paths:, logger:, apply:, on_error: nil, duplicate_keys_handling: DuplicateKeysHandling::FAIL, @@ -89,7 +100,8 @@ def initialize(paths:, logger:, apply:, on_error: nil, debounce_delay: DEFAULT_DEBOUNCE_DELAY, retry_delay: DEFAULT_RETRY_DELAY, skip_unchanged: false, - log_prefix: "[LDClient]") + log_prefix: "[LDClient]", + clock: nil) @paths = paths @logger = logger @log_prefix = log_prefix @@ -100,6 +112,7 @@ def initialize(paths:, logger:, apply:, on_error: nil, @debounce_delay = debounce_delay @retry_delay = retry_delay @skip_unchanged = skip_unchanged + @clock = clock || method(:monotonic_now) # Guards the scheduling state below and wakes the worker. @mutex = Mutex.new @@ -107,7 +120,7 @@ def initialize(paths:, logger:, apply:, on_error: nil, @worker = nil @stopped = false @trigger_pending = false - @retry_requested = false + @requested_retry_delay = nil @debounce_deadline = nil @retry_deadline = nil @@ -115,25 +128,29 @@ def initialize(paths:, logger:, apply:, on_error: nil, @reload_mutex = Mutex.new @last_good_digest = nil @last_error_message = nil - # Whether each configured path contributed content to the data currently applied, and the - # EmptyEpisode of each path that loads have found empty since. Only touched while holding - # @reload_mutex. + # Whether each configured path contributed content to the data currently applied; the + # EmptyFile of each path whose content is in effect and that loads have found with no + # content since; when the current run of loads that hold files back began, or nil; and + # whether the load in progress held files back. Only touched while holding @reload_mutex. @applied_had_content = {} - @empty_episodes = {} + @empty_files = {} + @holding_since = nil + @held_back = false end # # Synchronously loads the files and applies the result, or reports the failure. Use it for # the initial load. A failure here schedules the same automatic retry as a failed - # triggered reload, and so does a load that puts off a file it finds empty. + # triggered reload, and a load that holds back a file it finds empty arms the retry for when + # the file may be read as it is. # - # @return [Boolean] true if the load succeeded, false if it failed or put off an empty file + # @return [Boolean] true if the load succeeded, false if it failed or held back an empty file # def reload_now ensure_started - ok = reload(retrying: false) - request_retry unless ok - ok + retry_in = reload(retrying: false) + request_retry(retry_in) unless retry_in.nil? + retry_in.nil? end # @@ -172,11 +189,11 @@ def stop end end - private def request_retry + private def request_retry(delay) return unless @retry_delay > 0 @mutex.synchronize do - @retry_requested = true + @requested_retry_delay = [@requested_retry_delay, delay].compact.min @cond.signal end end @@ -191,11 +208,11 @@ def stop else @logger.debug { "#{@log_prefix} Retrying flag data load after earlier failure" } end - ok = reload(retrying: action == :retry) + retry_in = reload(retrying: action == :retry) @mutex.synchronize do # A pending retry is superseded by this reload. The reload either succeeded, or it - # failed and arms a fresh retry here. - @retry_deadline = ok || @retry_delay <= 0 ? nil : monotonic_now + @retry_delay + # failed or held back a file and arms a fresh retry here. + @retry_deadline = retry_in.nil? || @retry_delay <= 0 ? nil : monotonic_now + retry_in end end rescue => e @@ -219,11 +236,12 @@ def stop @debounce_deadline = now + @debounce_delay end - if @retry_requested - # A synchronous reload_now failed. Arm the retry without reloading again at once. - # An already armed retry keeps its earlier deadline. - @retry_requested = false - @retry_deadline ||= now + @retry_delay + unless @requested_retry_delay.nil? + # A synchronous reload_now failed or held back a file. Arm the retry without + # reloading again at once. An already armed retry that comes sooner is kept. + deadline = now + @requested_retry_delay + @requested_retry_delay = nil + @retry_deadline = [@retry_deadline, deadline].compact.min end if !@debounce_deadline.nil? && now >= @debounce_deadline @debounce_deadline = nil @@ -242,159 +260,183 @@ def stop end # - # Performs one full load of all configured files and returns whether it succeeded. That - # decides whether a retry is 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. + # Performs one full load of all configured files. Returns nil if it succeeded, or the + # seconds after which to retry if it failed or held back a file. 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. # private def reload(retrying: false) @reload_mutex.synchronize do # A trigger already queued when stop was called can still reach here. - return true if stopped? - - documents = [] - files = [] - had_content = {} - put_off = [] - digest = Digest::SHA256.new - @paths.each do |path| - begin - # One read feeds both the digest and the parse, so the skip-unchanged digest can - # never disagree with the content that was applied. - content = FileData.read_file(path) - rescue ReadError => e - @empty_episodes.delete(path) if e.missing - if e.missing && @skip_missing_paths - @logger.debug { "#{@log_prefix} File #{path} does not exist; it contributes no data" } - had_content[path] = false - files << FileSummary.new(path, false, 0, 0) - next - end - return record_failure(e) - end - if FileData.blank?(content) - # The other files are still read, so that files emptied together start their - # delays in the same load. - if put_off_empty_file?(path) - put_off << path - next - end + return nil if stopped? + @held_back = false + begin + load_files + ensure + # A run of loads that hold files back ends with any load that does not. + @holding_since = nil unless @held_back + end + end + end + + # The body of {#reload}, while holding @reload_mutex. + private def load_files + documents = [] + files = [] + had_content = {} + held = [] + now = @clock.call + digest = Digest::SHA256.new + @paths.each do |path| + begin + # One read feeds both the digest and the parse, so the skip-unchanged digest can never + # disagree with the content that was applied. + content = FileData.read_file(path) + rescue ReadError => e + @empty_files.delete(path) if e.missing + if e.missing && @skip_missing_paths + @logger.debug { "#{@log_prefix} File #{path} does not exist; it contributes no data" } had_content[path] = false - else - @empty_episodes.delete(path) - had_content[path] = true + files << FileSummary.new(path, false, 0, 0) + next end - - digest << content << "\0" + return record_failure(e) + end + # A file with no content, or YAML that the parser reads as nothing, such as only a + # comment, has no entries. A read can see that much of a file that is being written. + # Content with no JSON document is only parsed when the file is not held back, and then + # fails to parse. + document = nil + has_content = !FileData.blank?(content) + if has_content begin - documents << FileData.parse_file(path, content) + document = FileData.parse_file(path, content) rescue ReadError => e return record_failure(e) end - files << FileSummary.new(path, true, 0, 0) + has_content = !document.equal?(Document::NONE) end - return put_off_empty_files(put_off) unless put_off.empty? - - begin - merged = FileData.merge(documents, - duplicate_keys_handling: @duplicate_keys_handling, - logger: @logger) - rescue => e - return record_failure(e) + if has_content + @empty_files.delete(path) + elsif hold_back?(path, now) + # The other files are still read, so that files emptied together start their delays + # in the same load. + held << path + next end + had_content[path] = has_content - # Documents are the present files in order. Copy their counts onto the file summaries. - document_index = 0 - files.each do |file| - next unless file.present - - summary = merged.documents[document_index] - file.flags = summary.flags - file.segments = summary.segments - document_index += 1 - end - merged.files = files - - # stop may have been called while the files were being read. Deliver nothing then. - return true if stopped? - - # A success right after a failure must apply even when the content is unchanged since - # the last success. The consumer heard about the failure through on_error and may - # have moved to an interrupted state. Only apply tells it that things are good again. - recovering = !@last_error_message.nil? - hexdigest = digest.hexdigest - if @skip_unchanged && !recovering && hexdigest == @last_good_digest - @applied_had_content = had_content - return true + digest << content << "\0" + if document.nil? + begin + document = FileData.parse_file(path, content) + rescue ReadError => e + return record_failure(e) + end end + documents << document + files << FileSummary.new(path, true, 0, 0) + end + return hold_back(held, now) unless held.empty? + + begin + merged = FileData.merge(documents, + duplicate_keys_handling: @duplicate_keys_handling, + logger: @logger) + rescue => e + return record_failure(e) + end - # Nothing is remembered until the consumer has accepted the result. A result that - # apply rejects must not become the baseline that skip-unchanged compares against, - # and must not count as a recovery. - begin - @apply.call(merged) - rescue => e - return record_failure(e) - end - @last_error_message = nil - @last_good_digest = hexdigest + # Documents are the present files in order. Copy their counts onto the file summaries. + document_index = 0 + files.each do |file| + next unless file.present + + summary = merged.documents[document_index] + file.flags = summary.flags + file.segments = summary.segments + document_index += 1 + end + merged.files = files + + # stop may have been called while the files were being read. Deliver nothing then. + return nil if stopped? + + # A success right after a failure must apply even when the content is unchanged since the + # last success. The consumer heard about the failure through on_error and may have moved + # to an interrupted state. Only apply tells it that things are good again. + recovering = !@last_error_message.nil? + hexdigest = digest.hexdigest + if @skip_unchanged && !recovering && hexdigest == @last_good_digest + # The digest covers only the content of the files that are present, so the same + # content can now come from another file. @applied_had_content = had_content - true + return nil end + + # Nothing is remembered until the consumer has accepted the result. A result that apply + # rejects must not become the baseline that skip-unchanged compares against, and must not + # count as a recovery. + begin + @apply.call(merged) + rescue => e + return record_failure(e) + end + @last_error_message = nil + @last_good_digest = hexdigest + @applied_had_content = had_content + nil end # - # Decides whether a load that found the file empty puts it off, because the file may be in - # the middle of an in-place rewrite, and records the empty episode. Modification times are - # only compared with each other, never with the clock. + # Decides whether a load that found a file with no content holds it back, because it may be + # in the middle of an in-place rewrite, and records since when it has been so. # - private def put_off_empty_file?(path) + private def hold_back?(path, now) return false if @retry_delay <= 0 || !@applied_had_content[path] - now = monotonic_now - modified = begin - File.mtime(path) - rescue SystemCallError, IOError - nil - end - episode = @empty_episodes[path] - if episode.nil? - episode = EmptyEpisode.new(now, now, modified) - @empty_episodes[path] = episode - elsif modified != episode.modified - # The file was written since a load last found it empty: a new rewrite. - episode.since = now - episode.modified = modified + state = Poller.observe(path) + empty = @empty_files[path] + if empty.nil? + empty = EmptyFile.new(now, state) + @empty_files[path] = empty + elsif empty.state != state + # The file was written since a load last found it with no content: a new rewrite. + empty.since = now + empty.state = state end - # After the delay the file is parsed. The episode stays, so later loads keep parsing it - # until it has content or is missing. The delay is checked with a tenth of slack, so that - # the retry that putting the file off arms, one retry delay later, parses it even if it - # comes slightly early. - now - episode.since < @retry_delay * 0.9 && now - episode.first_seen < EMPTY_FILE_DEFERRAL_LIMIT * @retry_delay + # After the delay the file is parsed. The entry stays, so later loads keep parsing it + # until it has content or is missing. + now - empty.since < @retry_delay && now - (@holding_since || now) < MAX_HOLD_RETRY_DELAYS * @retry_delay end # - # Puts off a load that found files empty. The current data stays, no failure is reported, - # and the false result arms the retry. + # Holds back a load that found files with no content. The current data stays and no failure + # is reported. Returns when to retry: when the first of the files has stayed so for the retry + # delay, or when the run of holding loads reaches its limit, plus a tenth of the retry delay, + # so that a retry that comes a little early still finds the delay over. # - private def put_off_empty_files(paths) - # stop may have been called while the files were being read. Report success so that no - # retry is armed. - return true if stopped? + private def hold_back(paths, now) + # stop may have been called while the files were being read. Arm no retry. + return nil if stopped? + @held_back = true + @holding_since ||= now + retry_at = @holding_since + MAX_HOLD_RETRY_DELAYS * @retry_delay paths.each do |path| @logger.debug do "#{@log_prefix} File #{path} is empty, possibly while it is being rewritten; reading it again before applying it" end + retry_at = [retry_at, @empty_files[path].since + @retry_delay].min end - false + [retry_at - now, 0].max + @retry_delay / 10.0 end private def record_failure(error) # stop may have been called while the files were being read. Deliver nothing then, and - # report success so that no retry is armed. - return true if stopped? + # arm no retry. + return nil if stopped? # With automatic retries, a persistent failure would repeat the same log entry and the # same callback on every attempt. Repeats of an identical failure are logged at debug @@ -402,13 +444,13 @@ def stop message = error.message if message == @last_error_message @logger.debug { "#{@log_prefix} Unable to load flags: #{message}" } - return false + return @retry_delay end @last_error_message = message @logger.error { "#{@log_prefix} Unable to load flags: #{message}" } @on_error&.call(error) - false + @retry_delay end private def stopped? diff --git a/lib/ldclient-rb/integrations/file_data.rb b/lib/ldclient-rb/integrations/file_data.rb index 48f32376..248685cd 100644 --- a/lib/ldclient-rb/integrations/file_data.rb +++ b/lib/ldclient-rb/integrations/file_data.rb @@ -190,11 +190,13 @@ def self.data_source_v2(options = {}) # logs the overrides in effect and what each file supplied, at Info level. # # 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 JSON file with no content is not a valid document: it fails to - # load, and the overrides in effect stay. A file whose content is in effect and that is found - # empty may be in the middle of a rewrite, so the source waits about a second, and up to about - # five seconds while the file keeps being rewritten, before it reads the file as it is; an - # emptied YAML file removes its overrides about a second later. YAML that is cut off can still + # with no entries, such as `{}`. A file whose content is in effect and that is found with no + # content (empty, or YAML with only a comment or `---`) may be in the middle of a rewrite, so the + # source holds it back: it waits about a second, and at most about five seconds while files keep + # being rewritten, before it reads the file as it is. Changes to the other files wait too, because + # a reload applies all of the files or none. After the wait, a JSON file with no content is not a + # valid document: it fails to load, and the overrides in effect stay. A YAML file with no content + # is a document with no entries, so its overrides are removed. YAML that is cut off can still # parse, with fewer entries or with a 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. diff --git a/spec/impl/file_data/document_spec.rb b/spec/impl/file_data/document_spec.rb index 647bcd2a..63d8f02a 100644 --- a/spec/impl/file_data/document_spec.rb +++ b/spec/impl/file_data/document_spec.rb @@ -53,6 +53,19 @@ module FileData end end + it "returns NONE for YAML that is only a null, and a document for {} or a member that is null" do + ["", "# a comment\n", "---\n", "~\n"].each do |content| + expect(Document.parse(content, format: :yaml)).to be(Document::NONE) + end + ["{}\n", "flagValues:\n"].each do |content| + document = Document.parse(content, format: :yaml) + + expect(document).not_to be(Document::NONE) + expect(document.flag_values).to eq({}) + end + expect(Document.parse("{}", format: :json)).not_to be(Document::NONE) + end + it "rejects empty JSON content, which is not a JSON document" do ["", " \n\r\n\t"].each do |content| expect { Document.parse(content, format: :json) } diff --git a/spec/impl/file_data/poller_spec.rb b/spec/impl/file_data/poller_spec.rb index 14eb503e..88e820f2 100644 --- a/spec/impl/file_data/poller_spec.rb +++ b/spec/impl/file_data/poller_spec.rb @@ -40,6 +40,18 @@ def with_poller(paths) end end + it "observes the state of the file that a symbolic link leads to" do + skip "symbolic links need extra privileges on Windows" if Gem.win_platform? + File.write(path("target.json"), "{}") + File.symlink(path("target.json"), path("link.json")) + File.utime(Time.at(1_000_000), Time.at(1_000_000), path("target.json")) + + state = Poller.observe(path("link.json")) + + expect(state).to eq(Poller::FileState.new(true, Time.at(1_000_000), 2)) + expect(Poller.observe(path("missing.json")).exists).to be false + end + it "invokes the callback when a file's modification time changes" do File.write(path("a.json"), "{}") with_poller([path("a.json")]) do |_poller, calls| diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index 1ef88a2f..7b04ba95 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -482,11 +482,15 @@ def read_again_logged? logger.output.include?(read_again_log) end + def hold_count + logger.output.scan(read_again_log).length + end + def with_empty_file_reloader(paths, **options, &block) with_reloader(paths, logger: logger, debounce_delay: 0.01, retry_delay: retry_delay, **options, &block) end - it "is read again before it is applied" do + it "is held back before it is applied" do # A writer that rewrites the file in place truncates it and then writes the new content. # In YAML an empty file is valid, so applying it would briefly remove every entry. y = write("a.yaml", yaml_doc(flag1: true)) @@ -537,7 +541,7 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is not put off again once its empty state is applied" do + it "is not held back again once its empty state is applied" do y = write("a.yaml", yaml_doc(flag1: true)) with_empty_file_reloader([y]) do |reloader, recorder| @@ -592,7 +596,7 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is put off again when a second rewrite truncates it before a load reads its content" do + it "is held back again when a second rewrite truncates it before a load reads its content" do # Two quick in-place rewrites: the write of the first and the truncation of the second # come before the next load, which finds the file empty again. It is a new rewrite, so # the delay starts again. @@ -604,6 +608,7 @@ def with_empty_file_reloader(paths, **options, &block) reloader.trigger expect(wait_for { read_again_logged? }).to be true + sleep 0.02 File.write(y, yaml_doc(flag2: true)) File.write(y, "") reloader.trigger @@ -616,7 +621,7 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is put off again when the retry finds a second truncation" do + it "is held back again when the retry finds a second truncation" do # A retry, or the next poll, can find the file empty again after a second rewrite started. y = write("a.yaml", yaml_doc(flag1: true)) @@ -627,6 +632,7 @@ def with_empty_file_reloader(paths, **options, &block) expect(wait_for { read_again_logged? }).to be true # No change signal from here on: only the retries read the file. + sleep 0.02 File.write(y, yaml_doc(flag2: true)) File.write(y, "") expect(wait_for(retry_delay + 2) { logger.output.scan(read_again_log).length == 2 }).to be true @@ -638,7 +644,7 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is put off again when a change to another file starts a load within the delay" do + it "is held back again when a change to another file starts a load within the delay" do a = write("a.yaml", yaml_doc(flag1: true)) b = write("b.yaml", yaml_doc(flag2: true)) @@ -661,65 +667,7 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "starts the delay afresh for a rewrite after a load has read its content" do - y = write("a.yaml", yaml_doc(flag1: true)) - - with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.3) do |reloader, recorder| - reloader.reload_now - File.write(y, "") - reloader.trigger - expect(wait_for { read_again_logged? }).to be true - File.write(y, yaml_doc(flag2: true)) - reloader.trigger - expect(wait_for { recorder.applied.length == 2 }).to be true - - # Longer than five retry delays after a load first found the file empty. That emptying - # is over, so a new one is put off too. - sleep 1.6 - File.write(y, "") - reloader.trigger - expect(wait_for { logger.output.scan(read_again_log).length == 2 || recorder.applied.length == 3 }).to be true - expect(logger.output.scan(read_again_log).length).to eq(2) - end - end - - it "starts the delay afresh for a file that is created empty after it was missing" do - y = write("a.yaml", yaml_doc(flag1: true)) - - with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.3) do |reloader, recorder| - reloader.reload_now - File.write(y, "") - reloader.trigger - expect(wait_for { read_again_logged? }).to be true - - # Without skipping missing paths, the loads fail while the file is missing, and the data - # with the file's content stays. - File.delete(y) - expect(wait_for { recorder.errors.length == 1 }).to be true - sleep 1.6 - File.write(y, "") - expect(wait_for { logger.output.scan(read_again_log).length == 2 || recorder.applied.length == 2 }).to be true - expect(logger.output.scan(read_again_log).length).to eq(2) - end - end - - it "keeps parsing a JSON file that stays empty without putting it off again" do - a = write("a.json", values_doc({ flag1: "a" })) - - with_reloader([a], logger: logger, debounce_delay: 0.01, retry_delay: 0.3) do |reloader, recorder| - reloader.reload_now - File.write(a, "") - reloader.trigger - expect(wait_for { recorder.errors.length == 1 }).to be true - - # The retries go on parsing the file, which fails each time, and the data stays. - expect(wait_for { logger.output.scan("the file has no content").length >= 4 }).to be true - expect(logger.output.scan(read_again_log).length).to eq(1) - expect(recorder.applied.length).to eq(1) - end - end - - it "is put off when the data that was kept as unchanged has content from it" do + it "is held back when the data that was kept as unchanged has content from it" do # The same content moves from one file to another, so the reload is skipped as unchanged, # but the data in effect now comes from the second file. a = write("a.yaml", yaml_doc(flag1: true)) @@ -738,26 +686,6 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is put off together with another file that is emptied at the same time" do - a = write("a.yaml", yaml_doc(flag1: true)) - b = write("b.yaml", yaml_doc(flag2: true)) - - with_empty_file_reloader([a, b]) do |reloader, recorder| - reloader.reload_now - File.write(a, "") - File.write(b, "") - reloader.trigger - - # One load puts both off, so both are applied by the first retry, not one retry delay - # after the other. - expect(wait_for { logger.output.scan(read_again_log).length == 2 }).to be true - expect(logger.output).to include("File #{a} is empty").and include("File #{b} is empty") - expect(wait_for(retry_delay + 0.8) { recorder.applied.length == 2 }).to be true - expect(recorder.applied[-1].empty?).to be true - expect(logger.output.scan(read_again_log).length).to eq(2) - end - end - it "does not keep a load from failing on another file that cannot be parsed" do a = write("a.yaml", yaml_doc(flag1: true)) b = write("b.json", values_doc({ flag2: "b" })) @@ -775,31 +703,6 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is applied after five retry delays when every load finds it empty again after a rewrite" do - y = write("a.yaml", yaml_doc(flag1: true)) - - # A shorter retry delay keeps the test short. The file is truncated again every 50 ms, - # which changes its modification time, so each load finds a new rewrite. - with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.5) do |reloader, recorder| - reloader.reload_now - File.write(y, "") - reloader.trigger - expect(wait_for { read_again_logged? }).to be true - first_found_empty = Time.now - - until recorder.applied.length == 2 || Time.now - first_found_empty > 6 - sleep 0.05 - File.write(y, "") - reloader.trigger - end - - expect(recorder.applied.length).to eq(2) - expect(recorder.applied[-1].empty?).to be true - expect(Time.now - first_found_empty).to be >= 2.3 - expect(logger.output.scan(read_again_log).length).to be > 5 - end - end - it "does not delay the other files' changes when it is an empty placeholder from the start" do # An empty YAML file next to the data, such as a placeholder for later entries, has never # had content, so it is never read again. @@ -837,7 +740,7 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is read again by a retry left over from an earlier failure instead of applied empty" do + it "is held back by a retry left over from an earlier failure instead of applied empty" do y = write("a.yaml", yaml_doc(flag1: true)) with_empty_file_reloader([y]) do |reloader, recorder| @@ -920,6 +823,328 @@ def with_empty_file_reloader(paths, **options, &block) expect(recorder.errors[0].message).to include("the file has no content, which is not a JSON document") end end + + it "arms the retry of a hold for the rest of the delay, plus a tenth of it" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 2.0) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { hold_count == 1 }).to be true + first_hold = Time.now + + # A load halfway through the delay holds the file back again, and arms the retry for + # the rest of the delay, not for a whole delay. + sleep 1.0 + reloader.trigger + expect(wait_for { hold_count == 2 }).to be true + expect(wait_for(4) { recorder.applied.length == 2 }).to be true + expect(Time.now - first_hold).to be_between(1.9, 2.7) + end + end + end + + describe "holding back a file, on a controlled clock" do + # A monotonic clock that only moves when a test moves it. + class FakeClock + attr_reader :now + + def initialize + @now = 1000.0 + end + + def advance(seconds) + @now += seconds + end + end + + # A retry delay of an hour on the controlled clock, so that no real retry fires during a + # test. Each load is a synchronous reload_now. + let(:hour) { 3600.0 } + let(:clock) { FakeClock.new } + let(:logger) { CapturingLogger.new } + + def yaml_doc(values) + "flagValues:\n" + values.map { |key, value| " #{key}: #{value}\n" }.join + end + + def hold_count + logger.output.scan("reading it again before applying it").length + end + + # Lets the next write get a later modification time on a file system whose timestamps only + # move once per clock tick of the kernel. + def next_tick + sleep 0.02 + end + + def with_clocked_reloader(paths, **options, &block) + with_reloader(paths, logger: logger, retry_delay: hour, clock: clock.method(:now), **options, &block) + end + + it "holds the file back until it has stayed empty for exactly the retry delay" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + + clock.advance(0.95 * hour) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + + clock.advance(0.05 * hour) + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "starts the delay over when the file is written while it is empty" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + + # Rewritten and truncated again between two loads. + clock.advance(0.6 * hour) + next_tick + File.write(y, yaml_doc(flag2: true)) + File.write(y, "") + expect(reloader.reload_now).to be false + + clock.advance(0.6 * hour) + expect(reloader.reload_now).to be false + + clock.advance(0.4 * hour) + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "reads the state of the file a symbolic link leads to" do + skip "symbolic links need extra privileges on Windows" if Gem.win_platform? + target = write("target.yaml", yaml_doc(flag1: true)) + link = path("link.yaml") + File.symlink(target, link) + + with_clocked_reloader([link]) do |reloader, recorder| + reloader.reload_now + File.write(target, "") + expect(reloader.reload_now).to be false + + clock.advance(0.6 * hour) + next_tick + File.write(target, yaml_doc(flag2: true)) + File.write(target, "") + expect(reloader.reload_now).to be false + + # The target was written, so its delay started again. + clock.advance(0.6 * hour) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "stops holding back after five retry delays while one file keeps being rewritten" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + + # Truncating the file again changes its modification time, so every load finds a new + # rewrite. + 9.times do + clock.advance(0.5 * hour) + next_tick + File.write(y, "") + expect(reloader.reload_now).to be false + end + + clock.advance(0.5 * hour) + next_tick + File.write(y, "") + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "stops holding back after five retry delays while two files are rewritten in turn" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + # Each load finds one of the files in the middle of a rewrite and the other one whole. + 9.times do |i| + clock.advance(0.5 * hour) + next_tick + emptied, rewritten = i.even? ? [b, a] : [a, b] + File.write(rewritten, yaml_doc(rewritten == a ? { flag1: true } : { flag2: true })) + File.write(emptied, "") + expect(reloader.reload_now).to be false + end + expect(recorder.applied.length).to eq(1) + + clock.advance(0.5 * hour) + File.write(b, yaml_doc(flag2: true)) + File.write(a, "") + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag2: true }) + end + end + + it "applies files that are emptied together after one retry delay" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + File.write(b, "") + expect(reloader.reload_now).to be false + expect(hold_count).to eq(2) + + clock.advance(hour) + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "holds back YAML that is only a comment or a document marker, and applies {} at once" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "# overrides for the incident\n") + expect(reloader.reload_now).to be false + File.write(y, "---\n") + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + + File.write(y, "{}\n") + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + end + end + + it "applies a JSON file that is {} at once" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_clocked_reloader([a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "{}") + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + expect(hold_count).to eq(0) + end + end + + it "keeps parsing a JSON file that stays empty, without holding it back again" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_clocked_reloader([a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + clock.advance(hour) + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + clock.advance(hour) + expect(reloader.reload_now).to be false + expect(hold_count).to eq(1) + expect(recorder.applied.length).to eq(1) + end + end + + it "holds back a JSON file that stayed empty for long when it is rewritten" do + # The failing loads ended the run of holding loads, so the rewrite is held back although + # the file was first found empty more than five retry delays ago. + a = write("a.json", values_doc({ flag1: "a" })) + + with_clocked_reloader([a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + clock.advance(hour) + expect(reloader.reload_now).to be false + clock.advance(5 * hour) + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + + next_tick + File.write(a, "") + expect(reloader.reload_now).to be false + expect(hold_count).to eq(2) + File.write(a, values_doc({ flag1: "b" })) + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag1: "b" }) + expect(recorder.errors.length).to eq(1) + end + end + + it "holds back a file emptied again after its content was read, even if its state looks the same" do + # On a file system with coarse modification times, a rewrite can leave the state of an + # empty file unchanged. + y = write("a.yaml", yaml_doc(flag1: true)) + allow(Poller).to receive(:observe).and_return(Poller::FileState.new(true, Time.at(0), 0)) + + with_clocked_reloader([y]) do |reloader, _recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + File.write(y, yaml_doc(flag2: true)) + expect(reloader.reload_now).to be true + + clock.advance(2 * hour) + File.write(y, "") + expect(reloader.reload_now).to be false + end + end + + it "holds back a file created empty after it was missing, even if its state looks the same" do + y = write("a.yaml", yaml_doc(flag1: true)) + allow(Poller).to receive(:observe).and_return(Poller::FileState.new(true, Time.at(0), 0)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + + # Without skipping missing paths, a missing file fails the load and the data stays. + File.delete(y) + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + + clock.advance(2 * hour) + File.write(y, "") + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "does not hold back a placeholder or a file that was missing" do + a = write("a.yaml", yaml_doc(flag1: true)) + placeholder = write("placeholder.yaml", "") + later = path("later.yaml") + + with_clocked_reloader([a, placeholder, later], skip_missing_paths: true) do |reloader, recorder| + reloader.reload_now + File.write(later, "") + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag1: true }) + expect(hold_count).to eq(0) + end + end end end end diff --git a/spec/in_place_writes.rb b/spec/in_place_writes.rb index 26fecdca..bfba8546 100644 --- a/spec/in_place_writes.rb +++ b/spec/in_place_writes.rb @@ -5,18 +5,25 @@ # module InPlaceWrites # - # Truncates the file, waits for the pause, then writes the content. A file that does not exist - # is created empty first. + # Truncates the file, writes the first part of the content if one is given, waits for the pause, + # then writes the rest. A file that does not exist is created first. # # @param path [String] # @param content [String] - # @param pause [Numeric] seconds between truncating the file and writing the content + # @param pause [Numeric] seconds between truncating the file and writing the rest of the content + # @param first [String] the beginning of the content, which a reader can see during the pause # - def self.rewrite(path, content, pause) + def self.rewrite(path, content, pause, first: "") + raise ArgumentError, "the content must start with the first part" unless content.start_with?(first) + # Opening with "w" truncates the file at once. File.open(path, "w") do |file| + unless first.empty? + file.write(first) + file.flush + end sleep pause - file.write(content) + file.write(content[first.length..]) end end end diff --git a/spec/integrations/file_override_source_spec.rb b/spec/integrations/file_override_source_spec.rb index d71c3ba6..06c06753 100644 --- a/spec/integrations/file_override_source_spec.rb +++ b/spec/integrations/file_override_source_spec.rb @@ -483,6 +483,22 @@ def with_source(paths, **options) expect(sink.snapshots.map { |flags, _segments| flags.map(&:key) }).to all(eq(["flag1"])) end end + + it "never supplies a snapshot without the override while a YAML file that starts with a comment is rewritten" do + # A load during the pause finds only the comment, which the YAML parser reads as nothing. + comment = "# overrides for the incident\n" + a = write("a.yaml", comment + yaml_values_doc({ flag1: "a" })) + + with_source([a], change_detection: mode) do |_source, sink| + sleep 0.3 if mode == :watching + ["b", "c"].each do |value| + InPlaceWrites.rewrite(a, comment + yaml_values_doc({ flag1: value }), pause, first: comment) + expect(wait_for { sink.flag_values == { "flag1" => value } }).to be true + end + + expect(sink.snapshots.map { |flags, _segments| flags.map(&:key) }).to all(eq(["flag1"])) + end + end end end From 7903d07625b0f0e06efdfcf17aaab69517279585 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 02:58:23 +0000 Subject: [PATCH 3/8] fix: Bound each held file on its own and time it from its own read Three cases of the hold of an empty file went wrong: - The clock was read once per load, so a slow read of an earlier file made a later file's emptiness look older and shortened its delay. The clock is now read after each file's own read, and again for the retry. - The bound of five retry delays applied to the run of holding loads, and a failed load ended the run, so a file kept written while empty was held without end while another file failed now and then. A file that reached the bound also let through another file's ordinary rewrite. Each file now has its own bound, counted from the first load that held it back. It goes on across reads that find the file with content and across failed loads, and starts over when data is applied or when the file stayed empty for the whole delay. The retry of a hold comes when the first held file's delay or bound is over, plus a tenth of the retry delay, and the retry logs that it reads the files again because a file was found empty. --- lib/ldclient-rb/impl/file_data/reloader.rb | 135 ++++++---- lib/ldclient-rb/integrations/file_data.rb | 16 +- spec/impl/file_data/reloader_spec.rb | 294 +++++++++++++++++++++ 3 files changed, 386 insertions(+), 59 deletions(-) diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index 64cd959b..9e70fb8e 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -26,14 +26,16 @@ module FileData # failure. A file that cannot be read or parsed still fails the load. A YAML document such as # `{}` is content, and is applied at once. # - # A file is held back until it has stayed so, without being written, for the retry delay. A - # change of its state (its modification time and size, read through any symbolic link) means - # 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 in a row that hold files back stop doing - # so after five retry delays, so that a file that keeps being rewritten, or files rewritten in - # turn, cannot hold the data back without end. The retry that a hold arms comes when the first - # held file's delay is over, or when the run of holding loads reaches its limit, plus a tenth - # of the retry delay. A file that contributed no content to the data currently applied, such + # A file is held back until it has stayed so, without being written, for the retry delay, + # counted from the load that read it. A change of its state (its modification time and size, + # read through any symbolic link) means it was written since a load last found it so, for + # example rewritten and truncated again between two loads, so the delay starts over. Each file + # is held back for at most five retry delays from the first load that held it back, counted + # across reads that find it with content and across failed loads, and started over once data + # is applied, so that a file that keeps being rewritten, or files rewritten in turn, cannot + # hold the data back without end. The retry that a hold arms comes when the first held file's + # delay or bound is over, plus a tenth of the retry delay. A file that contributed no content + # to the data currently applied, such # as a placeholder that has been empty from the start, a file that was missing, or a file # whose emptying has been applied, is parsed at once. So is every file when there are no # automatic retries, because nothing would read it again. After the delay, an empty JSON file @@ -55,13 +57,14 @@ class Reloader # file is cheap, so this can be short. In seconds. DEFAULT_RETRY_DELAY = 1.0 - # Loads in a row stop holding files back once they have held some back for this many retry - # delays. + # A file is held back for at most this many retry delays from the first load that held it + # back, until data is applied. MAX_HOLD_RETRY_DELAYS = 5 # A file whose content is in effect and that loads have found with no content: since when it - # has been so without being written, and its {Poller::FileState} then. - EmptyFile = Struct.new(:since, :state) + # has been so without being written, or nil once a load read it with content or found it + # missing; its {Poller::FileState} then; and when a load first held it back, or nil. + EmptyFile = Struct.new(:since, :state, :first_held) private_constant :EmptyFile # @@ -130,12 +133,13 @@ def initialize(paths:, logger:, apply:, on_error: nil, @last_error_message = nil # Whether each configured path contributed content to the data currently applied; the # EmptyFile of each path whose content is in effect and that loads have found with no - # content since; when the current run of loads that hold files back began, or nil; and - # whether the load in progress held files back. Only touched while holding @reload_mutex. + # content since; and whether the last load held a file back. Only touched while holding + # @reload_mutex. @applied_had_content = {} @empty_files = {} - @holding_since = nil - @held_back = false + @read_again_pending = false + # The delay of the last retry that a load asked for. Only tests read it. Guarded by @mutex. + @last_requested_retry_delay = nil end # @@ -153,6 +157,16 @@ def reload_now retry_in.nil? end + # + # The delay in seconds of the last retry that a load asked for, armed or not. Only tests + # use it. + # + # @return [Numeric, nil] + # + def last_requested_retry_delay + @mutex.synchronize { @last_requested_retry_delay } + end + # # Signals that the files may have changed and a reload should happen after the debounce # delay. It never blocks. Signals that arrive while a reload is already pending are @@ -193,6 +207,7 @@ def stop return unless @retry_delay > 0 @mutex.synchronize do + @last_requested_retry_delay = delay @requested_retry_delay = [@requested_retry_delay, delay].compact.min @cond.signal end @@ -203,15 +218,12 @@ def stop action = next_action break if action == :stop - if action == :reload - @logger.info { "#{@log_prefix} Reloading flag data after detecting a change" } - else - @logger.debug { "#{@log_prefix} Retrying flag data load after earlier failure" } - end + @logger.info { "#{@log_prefix} Reloading flag data after detecting a change" } if action == :reload retry_in = reload(retrying: action == :retry) @mutex.synchronize do # A pending retry is superseded by this reload. The reload either succeeded, or it # failed or held back a file and arms a fresh retry here. + @last_requested_retry_delay = retry_in unless retry_in.nil? @retry_deadline = retry_in.nil? || @retry_delay <= 0 ? nil : monotonic_now + retry_in end end @@ -270,13 +282,15 @@ def stop # A trigger already queued when stop was called can still reach here. return nil if stopped? - @held_back = false - begin - load_files - ensure - # A run of loads that hold files back ends with any load that does not. - @holding_since = nil unless @held_back + if retrying + if @read_again_pending + @logger.debug { "#{@log_prefix} Reading flag data again after a file was found empty" } + else + @logger.debug { "#{@log_prefix} Retrying flag data load after earlier failure" } + end end + @read_again_pending = false + load_files end end @@ -286,7 +300,6 @@ def stop files = [] had_content = {} held = [] - now = @clock.call digest = Digest::SHA256.new @paths.each do |path| begin @@ -294,7 +307,9 @@ def stop # disagree with the content that was applied. content = FileData.read_file(path) rescue ReadError => e - @empty_files.delete(path) if e.missing + # A missing file is not empty, so its delay starts again when it is found empty. Its + # bound goes on. + @empty_files[path].since = nil if e.missing && @empty_files.key?(path) if e.missing && @skip_missing_paths @logger.debug { "#{@log_prefix} File #{path} does not exist; it contributes no data" } had_content[path] = false @@ -318,8 +333,12 @@ def stop has_content = !document.equal?(Document::NONE) end if has_content - @empty_files.delete(path) - elsif hold_back?(path, now) + # Its delay starts again when it is found empty. Its bound goes on, so that files + # rewritten in turn stay bounded. + @empty_files[path].since = nil if @empty_files.key?(path) + elsif hold_back?(path, @clock.call) + # The clock is read after this file's own read, so a slow read of another file does + # not shorten its delay. # The other files are still read, so that files emptied together start their delays # in the same load. held << path @@ -338,7 +357,7 @@ def stop documents << document files << FileSummary.new(path, true, 0, 0) end - return hold_back(held, now) unless held.empty? + return hold_back(held, @clock.call) unless held.empty? begin merged = FileData.merge(documents, @@ -371,7 +390,7 @@ def stop if @skip_unchanged && !recovering && hexdigest == @last_good_digest # The digest covers only the content of the files that are present, so the same # content can now come from another file. - @applied_had_content = had_content + remember_applied(had_content) return nil end @@ -385,50 +404,64 @@ def stop end @last_error_message = nil @last_good_digest = hexdigest - @applied_had_content = had_content + remember_applied(had_content) nil end + # Records what each file contributed to the data now in effect. The holds of the files start + # over. + private def remember_applied(had_content) + @applied_had_content = had_content + @empty_files.clear + end + # # Decides whether a load that found a file with no content holds it back, because it may be - # in the middle of an in-place rewrite, and records since when it has been so. + # in the middle of an in-place rewrite, and records since when it has been so. `now` is read + # after the file's own read. # private def hold_back?(path, now) return false if @retry_delay <= 0 || !@applied_had_content[path] state = Poller.observe(path) - empty = @empty_files[path] - if empty.nil? - empty = EmptyFile.new(now, state) - @empty_files[path] = empty - elsif empty.state != state - # The file was written since a load last found it with no content: a new rewrite. + empty = (@empty_files[path] ||= EmptyFile.new(nil, nil, nil)) + if empty.since.nil? || empty.state != state + # Found empty for the first time since it had content, or written since a load last + # found it so: a new rewrite. empty.since = now empty.state = state end - # After the delay the file is parsed. The entry stays, so later loads keep parsing it - # until it has content or is missing. - now - empty.since < @retry_delay && now - (@holding_since || now) < MAX_HOLD_RETRY_DELAYS * @retry_delay + if now - empty.since >= @retry_delay + # It stayed empty for the whole delay. The entry stays, so later loads keep parsing it + # until it has content. + empty.first_held = nil + return false + end + return false if !empty.first_held.nil? && now - empty.first_held >= MAX_HOLD_RETRY_DELAYS * @retry_delay + + empty.first_held ||= now + true end # # Holds back a load that found files with no content. The current data stays and no failure - # is reported. Returns when to retry: when the first of the files has stayed so for the retry - # delay, or when the run of holding loads reaches its limit, plus a tenth of the retry delay, - # so that a retry that comes a little early still finds the delay over. + # is reported. Returns when to retry: when the delay or the bound of the first of the files + # is over, plus a tenth of the retry delay, so that a retry that comes a little early still + # finds it over. # private def hold_back(paths, now) # stop may have been called while the files were being read. Arm no retry. return nil if stopped? - @held_back = true - @holding_since ||= now - retry_at = @holding_since + MAX_HOLD_RETRY_DELAYS * @retry_delay + @read_again_pending = true + retry_at = nil paths.each do |path| @logger.debug do "#{@log_prefix} File #{path} is empty, possibly while it is being rewritten; reading it again before applying it" end - retry_at = [retry_at, @empty_files[path].since + @retry_delay].min + empty = @empty_files[path] + over = [empty.since + @retry_delay, empty.first_held + MAX_HOLD_RETRY_DELAYS * @retry_delay].min + retry_at = over if retry_at.nil? || over < retry_at end [retry_at - now, 0].max + @retry_delay / 10.0 end diff --git a/lib/ldclient-rb/integrations/file_data.rb b/lib/ldclient-rb/integrations/file_data.rb index 248685cd..229d0327 100644 --- a/lib/ldclient-rb/integrations/file_data.rb +++ b/lib/ldclient-rb/integrations/file_data.rb @@ -192,14 +192,14 @@ def self.data_source_v2(options = {}) # 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 is found with no # content (empty, or YAML with only a comment or `---`) may be in the middle of a rewrite, so the - # source holds it back: it waits about a second, and at most about five seconds while files keep - # being rewritten, before it reads the file as it is. Changes to the other files wait too, because - # a reload applies all of the files or none. After the wait, a JSON file with no content is not a - # valid document: it fails to load, and the overrides in effect stay. A YAML file with no content - # is a document with no entries, so its overrides are removed. YAML that is cut off can still - # parse, with fewer entries or with a 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. + # source holds it back: it waits about a second, and at most about five seconds while that file + # keeps being rewritten, before it reads the file as it is. Changes to the other files wait too, + # because a reload applies all of the files or none. After the wait, a JSON file with no content + # is not a valid document: it fails to load, and the overrides in effect stay. A YAML file with no + # content is a document with no entries, so its overrides are removed. YAML that is cut off can + # still parse, with fewer entries or with a 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. # # An evaluation that an override affects, directly or through a prerequisite or segment, is # marked: its reason reports {EvaluationReason#override_affected}, it produces no individual diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index 7b04ba95..caea22f9 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -824,6 +824,23 @@ def with_empty_file_reloader(paths, **options, &block) end end + it "says that the retry after a hold reads the files again because a file was found empty" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.2) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + expect(wait_for { recorder.applied.length == 2 }).to be true + expect(logger.output).to include("DEBUG -- : [LDClient] Reading flag data again after a file was found empty") + expect(logger.output).not_to include("Retrying flag data load after earlier failure") + + File.write(y, "flagValues: [") + reloader.trigger + expect(wait_for { logger.output.include?("Retrying flag data load after earlier failure") }).to be true + end + end + it "arms the retry of a hold for the rest of the delay, plus a tenth of it" do y = write("a.yaml", yaml_doc(flag1: true)) @@ -879,6 +896,12 @@ def next_tick sleep 0.02 end + # Gives the file a new modification time, as a write while it is empty does. + def mark_written(path) + @marks = (@marks || 0) + 1 + File.utime(Time.at(@marks), Time.at(@marks), path) + end + def with_clocked_reloader(paths, **options, &block) with_reloader(paths, logger: logger, retry_delay: hour, clock: clock.method(:now), **options, &block) end @@ -1111,6 +1134,67 @@ def with_clocked_reloader(paths, **options, &block) end end + it "starts a file's delay again after a load reads it with content, even if its state looks the same" do + # The load that reads a.yaml with content holds b.yaml back, so nothing is applied in between. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + allow(Poller).to receive(:observe).and_return(Poller::FileState.new(true, Time.at(0), 0)) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + clock.advance(0.5 * hour) + File.write(a, yaml_doc(flag1: true)) + File.write(b, "") + expect(reloader.reload_now).to be false + + clock.advance(0.1 * hour) + File.write(a, "") + File.write(b, yaml_doc(flag2: true)) + expect(reloader.reload_now).to be false + + # a.yaml's delay started again at 0.6 hours, not at the start. + clock.advance(0.6 * hour) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "holds a file emptied again after its content was read back again when its state looks the same" do + # 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. b.yaml is kept written while empty, so that no load applies. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + first_empty_write_time = File.mtime(a) + File.write(b, "") + expect(reloader.reload_now).to be false + + clock.advance(50.0 / 60 * hour) + File.write(a, yaml_doc(flag1: true)) + mark_written(b) + expect(reloader.reload_now).to be false + + clock.advance(20.0 / 60 * hour) + File.write(a, "") + File.utime(first_empty_write_time, first_empty_write_time, a) + mark_written(b) + expect(reloader.reload_now).to be false + + # b.yaml gets its content back while a.yaml is still within its new delay. + clock.advance(10.0 / 60 * hour) + File.write(b, yaml_doc(flag2: true)) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + it "holds back a file created empty after it was missing, even if its state looks the same" do y = write("a.yaml", yaml_doc(flag1: true)) allow(Poller).to receive(:observe).and_return(Poller::FileState.new(true, Time.at(0), 0)) @@ -1132,6 +1216,216 @@ def with_clocked_reloader(paths, **options, &block) end end + it "does not let a slow read of an earlier file shorten a later file's delay" do + # The read of b.yaml, which comes first, takes most of a retry delay. a.yaml's delay is + # counted from when it was read. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + slow = nil + allow(FileData).to receive(:read_file).and_wrap_original do |original, path| + content = original.call(path) + if path == slow + slow = nil + clock.advance(0.9 * hour) + end + content + end + + with_clocked_reloader([b, a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + slow = b + expect(reloader.reload_now).to be false + + clock.advance(0.2 * hour) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "does not start the bound of a held file over when another file fails to load" do + a = write("a.yaml", yaml_doc(flag1: true)) + c = write("c.json", values_doc({ flag3: "c" })) + + with_clocked_reloader([a, c]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + # a.yaml keeps being written while empty, and every other load fails on c.json. + 9.times do |i| + clock.advance(0.5 * hour) + mark_written(a) + File.write(c, i.even? ? '{"flagValues"' : values_doc({ flag3: "c" })) + expect(reloader.reload_now).to be false + end + expect(recorder.applied.length).to eq(1) + + clock.advance(0.5 * hour) + mark_written(a) + File.write(c, values_doc({ flag3: "c" })) + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag3: "c" }) + end + end + + it "does not cut a file's rewrite short when another file reaches its bound" do + # b.yaml keeps being written while empty, and reaches its bound while a.yaml is in the + # middle of an ordinary rewrite. a.yaml is still held back. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(b, "") + expect(reloader.reload_now).to be false + 9.times do |i| + clock.advance(0.5 * hour) + mark_written(b) + File.write(a, "") if i == 8 + expect(reloader.reload_now).to be false + end + + clock.advance(0.5 * hour) + mark_written(b) + expect(reloader.reload_now).to be false + + clock.advance(0.25 * hour) + File.write(a, yaml_doc(flag3: true)) + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag3: true }) + end + end + + it "starts the bound over once data is applied" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + 8.times do + clock.advance(0.5 * hour) + mark_written(y) + expect(reloader.reload_now).to be false + end + File.write(y, yaml_doc(flag2: true)) + expect(reloader.reload_now).to be true + + # Emptied again: a new bound of five retry delays, not the rest of the old one, which + # would be over at 5 hours. + clock.advance(0.1 * hour) + File.write(y, "") + expect(reloader.reload_now).to be false + 2.times do + clock.advance(0.5 * hour) + mark_written(y) + expect(reloader.reload_now).to be false + end + expect(recorder.flag_values).to eq({ flag2: true }) + end + end + + it "holds back a file again later after a rewrite with the same content was skipped as unchanged" do + # A tool rewrites the file in place now and then with the same content. The load that + # finds the content again is skipped as unchanged, and starts the file's bound over, as + # an application does. + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y], skip_unchanged: true) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + File.write(y, yaml_doc(flag1: true)) + expect(reloader.reload_now).to be true + expect(recorder.applied.length).to eq(1) + + clock.advance(6 * hour) + File.write(y, "") + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "asks for the retry when the delay is over, plus a tenth of it" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, _recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + expect(reloader.last_requested_retry_delay).to be_within(0.001).of(1.1 * hour) + + # A later load asks for the rest of the delay, not a whole one. + clock.advance(0.5 * hour) + expect(reloader.reload_now).to be false + expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.6 * hour) + end + end + + it "asks for the retry when the bound is over, if that comes first" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, _recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + 4.times do + clock.advance(hour) + mark_written(y) + expect(reloader.reload_now).to be false + end + clock.advance(0.8 * hour) + mark_written(y) + expect(reloader.reload_now).to be false + + # The file's delay would be over in an hour, but its bound is over in 0.2 hours. + expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.3 * hour) + end + end + + it "asks for the retry when the first of several held files is due" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_clocked_reloader([a, b]) do |reloader, _recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + clock.advance(0.25 * hour) + File.write(b, "") + expect(reloader.reload_now).to be false + + expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.85 * hour) + end + end + + it "asks for the same retry when a change signal starts the load that holds a file back" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y], debounce_delay: 0.01) do |reloader, _recorder| + reloader.reload_now + File.write(y, "") + reloader.trigger + + expect(wait_for { hold_count == 1 }).to be true + expect(wait_for { !reloader.last_requested_retry_delay.nil? }).to be true + expect(reloader.last_requested_retry_delay).to be_within(0.001).of(1.1 * hour) + end + end + + it "asks for the retry delay after a failure" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_clocked_reloader([a]) do |reloader, recorder| + reloader.reload_now + File.write(a, '{"flagValues"') + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + expect(reloader.last_requested_retry_delay).to eq(hour) + end + end + it "does not hold back a placeholder or a file that was missing" do a = write("a.yaml", yaml_doc(flag1: true)) placeholder = write("placeholder.yaml", "") From 81b7cede21e7d69279422182fba9a438152db299 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 04:17:19 +0000 Subject: [PATCH 4/8] fix: Start a held file's bound over only at a load that holds nothing back Two cases of the per-file bound held data back too long or let a rewrite through: - A file that stayed empty for the whole delay started its own bound over while another file kept every load holding, so its next truncation got a fresh bound. Two files kept empty and truncated again in turn were held without end. - A file held back during a long failure of another file kept its bound, so a later load that fixed the other file while this one was being rewritten applied this one empty. A file's bound now starts over only at a load that holds nothing back, whether it applies, is skipped as unchanged, or fails, for every file that load read. A file that the load did not get to keeps its bound. --- lib/ldclient-rb/impl/file_data/reloader.rb | 45 +++++++----- spec/impl/file_data/reloader_spec.rb | 82 ++++++++++++++++++++++ 2 files changed, 110 insertions(+), 17 deletions(-) diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index 9e70fb8e..7204dd5f 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -30,14 +30,14 @@ module FileData # counted from the load that read it. A change of its state (its modification time and size, # read through any symbolic link) means it was written since a load last found it so, for # example rewritten and truncated again between two loads, so the delay starts over. Each file - # is held back for at most five retry delays from the first load that held it back, counted - # across reads that find it with content and across failed loads, and started over once data - # is applied, so that a file that keeps being rewritten, or files rewritten in turn, cannot + # is held back for at most five retry delays from the first load that held it back. The bound + # goes on across reads that find the file with content, and starts over only at a load that + # holds nothing back, whether that load applies, is skipped as unchanged, or fails, for each + # file that load read. So a file that keeps being rewritten, or files rewritten in turn, cannot # hold the data back without end. The retry that a hold arms comes when the first held file's # delay or bound is over, plus a tenth of the retry delay. A file that contributed no content - # to the data currently applied, such - # as a placeholder that has been empty from the start, a file that was missing, or a file - # whose emptying has been applied, is parsed at once. So is every file when there are no + # to the data currently applied, such as a placeholder that has been empty from the start, a + # file that was missing, or a file whose emptying has been applied, is parsed at once. So is every file when there are no # automatic retries, because nothing would read it again. After the delay, an empty JSON file # fails to parse and the data stays; an empty YAML file is a document with no entries. # @@ -133,11 +133,12 @@ def initialize(paths:, logger:, apply:, on_error: nil, @last_error_message = nil # Whether each configured path contributed content to the data currently applied; the # EmptyFile of each path whose content is in effect and that loads have found with no - # content since; and whether the last load held a file back. Only touched while holding - # @reload_mutex. + # content since; whether the last load held a file back; and whether the load in progress + # holds one back. Only touched while holding @reload_mutex. @applied_had_content = {} @empty_files = {} @read_again_pending = false + @holding_this_load = false # The delay of the last retry that a load asked for. Only tests read it. Guarded by @mutex. @last_requested_retry_delay = nil end @@ -290,15 +291,27 @@ def stop end end @read_again_pending = false - load_files + @holding_this_load = false + # The files that the load read and did not hold back, with whether each had content. + had_content = {} + retry_in = load_files(had_content) + unless @holding_this_load + # A load that holds nothing back, whether it applies, is skipped 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. + had_content.each_key do |path| + empty = @empty_files[path] + empty.first_held = nil unless empty.nil? + end + end + retry_in end end # The body of {#reload}, while holding @reload_mutex. - private def load_files + private def load_files(had_content) documents = [] files = [] - had_content = {} held = [] digest = Digest::SHA256.new @paths.each do |path| @@ -341,6 +354,7 @@ def stop # not shorten its delay. # The other files are still read, so that files emptied together start their delays # in the same load. + @holding_this_load = true held << path next end @@ -431,12 +445,9 @@ def stop empty.since = now empty.state = state end - if now - empty.since >= @retry_delay - # It stayed empty for the whole delay. The entry stays, so later loads keep parsing it - # until it has content. - empty.first_held = nil - return false - end + # After the delay the file is parsed. The entry stays, so later loads keep parsing it until + # it has content. Its bound starts over only at a load that holds nothing back. + return false if now - empty.since >= @retry_delay return false if !empty.first_held.nil? && now - empty.first_held >= MAX_HOLD_RETRY_DELAYS * @retry_delay empty.first_held ||= now diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index caea22f9..4fcfdb1b 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -1347,6 +1347,88 @@ def with_clocked_reloader(paths, **options, &block) end end + it "applies files emptied in staggered turns after the bound" do + # 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. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + applied = false + (1..14).each do |i| + clock.advance(0.9 * hour) + mark_written(i.odd? ? b : a) + File.write(b, "") if i == 1 + reloader.reload_now + # The retry that the hold arms, a little after the other file's delay is over. + clock.advance(0.2 * hour) + reloader.reload_now + clock.advance(-0.2 * hour) + applied = recorder.applied.length > 1 + break if applied + end + expect(applied).to be true + end + end + + it "starts the bound of a file held back during a failure over once a load reads it again" do + # While b.json fails to load, a.yaml is rewritten once and its gap is held back. Later + # b.json is fixed while a.yaml is in the middle of another rewrite, which is held back + # with a bound of its own. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.json", values_doc({ flag3: "c" })) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(b, '{"flagValues"') + File.write(a, "") + expect(reloader.reload_now).to be false + File.write(a, yaml_doc(flag2: true)) + expect(reloader.reload_now).to be false + clock.advance(6 * hour) + expect(reloader.reload_now).to be false + + # The load that held a.yaml back first failed on b.json, so that hold was not logged. + File.write(b, values_doc({ flag3: "c" })) + File.write(a, "") + expect(reloader.reload_now).to be false + expect(hold_count).to eq(1) + expect(recorder.applied.length).to eq(1) + end + end + + it "asks for the retry from the end of the load's reads" do + # 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. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + slow = nil + allow(FileData).to receive(:read_file).and_wrap_original do |original, path| + content = original.call(path) + if path == slow + slow = nil + clock.advance(0.1 * hour) + end + content + end + + with_clocked_reloader([a, b]) do |reloader, _recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + clock.advance(0.95 * hour) + slow = b + expect(reloader.reload_now).to be false + expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.1 * hour) + end + end + it "asks for the retry when the delay is over, plus a tenth of it" do y = write("a.yaml", yaml_doc(flag1: true)) From 00f7cb1f37509df2a5995c2cb4ad89a95050bc33 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 05:33:24 +0000 Subject: [PATCH 5/8] fix: Start the bound over for a file whose read or parse fails A file saved in place with an error, then fixed in place more than five retry delays later, had the fix applied empty: the load that failed on the file returned before the file was counted among the files it read, so the file kept the bound of the hold before the error. A load now records every file it gets to and does not hold back, including the file whose read or parse fails and a missing file, and a load that holds nothing back starts the bound over for all of them. A file after the failing one keeps its bound. A read that finds a file with content, even content that fails to parse, or finds it missing, ends the file's empty state, so its next emptying starts a new delay. --- lib/ldclient-rb/impl/file_data/reloader.rb | 49 ++++-- spec/impl/file_data/reloader_spec.rb | 176 +++++++++++++++++++++ 2 files changed, 210 insertions(+), 15 deletions(-) diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index 7204dd5f..f93800df 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -33,8 +33,11 @@ module FileData # is held back for at most five retry delays from the first load that held it back. The bound # goes on across reads that find the file with content, and starts over only at a load that # holds nothing back, whether that load applies, is skipped as unchanged, or fails, for each - # file that load read. So a file that keeps being rewritten, or files rewritten in turn, cannot - # hold the data back without end. The retry that a hold arms comes when the first held file's + # file that load got to, including one whose read or parse failed. A file after one that + # failed keeps its bound. So a file that keeps being rewritten, or files rewritten in turn, + # cannot hold the data back without end. A read that finds a file with content, even content + # that fails to parse, or finds it missing, ends its empty state, so its next emptying starts + # a new delay. The retry that a hold arms comes when the first held file's # delay or bound is over, plus a tenth of the retry delay. A file that contributed no content # to the data currently applied, such as a placeholder that has been empty from the start, a # file that was missing, or a file whose emptying has been applied, is parsed at once. So is every file when there are no @@ -292,14 +295,15 @@ def stop end @read_again_pending = false @holding_this_load = false - # The files that the load read and did not hold back, with whether each had content. - had_content = {} - retry_in = load_files(had_content) + # The files that the load got to and did not hold back, including one whose read or + # parse failed. + reached = [] + retry_in = load_files(reached) unless @holding_this_load # A load that holds nothing back, whether it applies, is skipped 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. - had_content.each_key do |path| + # 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 one that failed keeps its bound. + reached.each do |path| empty = @empty_files[path] empty.first_held = nil unless empty.nil? end @@ -309,9 +313,10 @@ def stop end # The body of {#reload}, while holding @reload_mutex. - private def load_files(had_content) + private def load_files(reached) documents = [] files = [] + had_content = {} held = [] digest = Digest::SHA256.new @paths.each do |path| @@ -320,9 +325,11 @@ def stop # disagree with the content that was applied. content = FileData.read_file(path) rescue ReadError => e - # A missing file is not empty, so its delay starts again when it is found empty. Its - # bound goes on. - @empty_files[path].since = nil if e.missing && @empty_files.key?(path) + if e.missing + not_empty(path, reached) + else + reached << path + end if e.missing && @skip_missing_paths @logger.debug { "#{@log_prefix} File #{path} does not exist; it contributes no data" } had_content[path] = false @@ -341,14 +348,14 @@ def stop begin document = FileData.parse_file(path, content) rescue ReadError => e + # Content that fails to parse is content: the file's empty state is over. + not_empty(path, reached) return record_failure(e) end has_content = !document.equal?(Document::NONE) end if has_content - # Its delay starts again when it is found empty. Its bound goes on, so that files - # rewritten in turn stay bounded. - @empty_files[path].since = nil if @empty_files.key?(path) + not_empty(path, reached) elsif hold_back?(path, @clock.call) # The clock is read after this file's own read, so a slow read of another file does # not shorten its delay. @@ -357,6 +364,8 @@ def stop @holding_this_load = true held << path next + else + reached << path end had_content[path] = has_content @@ -422,6 +431,16 @@ def stop nil end + # Records that a load found the file with content, even content that fails to parse, or + # missing: its empty state is over, so its next emptying starts a new delay even where a + # coarse modification time makes it look the same. Its bound goes on, so that files + # rewritten in turn stay bounded. + private def not_empty(path, reached) + reached << path + empty = @empty_files[path] + empty.since = nil unless empty.nil? + end + # Records what each file contributed to the data now in effect. The holds of the files start # over. private def remember_applied(had_content) diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index 4fcfdb1b..05187165 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -1429,6 +1429,182 @@ def with_clocked_reloader(paths, **options, &block) end end + [ + ["YAML", "a.yaml", "flagValues:\n flag1: true\n", "flagValues: [\n", "flagValues:\n flag2: true\n"], + ["JSON", "a.json", '{"flagValues": {"flag1": true}}', '{"flagValues": ', '{"flagValues": {"flag2": true}}'], + ].each do |format, name, good, broken, fixed| + it "holds back the in-place save that fixes a #{format} file saved with an error" do + # An operator saves the file in place with a mistake, and saves the fix in place long + # after. The loads in between fail on the file, and the fix's own gap is held back. + y = write(name, good) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + File.write(y, broken) + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + 6.times do + clock.advance(hour) + reloader.reload_now + end + errors = logger.output.scan("ERROR").length + + File.write(y, "") + expect(reloader.reload_now).to be false + expect(hold_count).to eq(2) + expect(logger.output.scan("ERROR").length).to eq(errors) + File.write(y, fixed) + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag2: true }) + end + end + end + + it "holds back the fix of a file saved with an error even when its emptying looks like the first" do + # The broken content ended the first empty state, so the later truncation starts a new + # delay although, on a file system with coarse modification times, it looks the same. + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, _recorder| + reloader.reload_now + File.write(y, "") + first_empty_write_time = File.mtime(y) + expect(reloader.reload_now).to be false + File.write(y, "flagValues: [\n") + expect(reloader.reload_now).to be false + + clock.advance(2 * hour) + File.write(y, "") + File.utime(first_empty_write_time, first_empty_write_time, y) + expect(reloader.reload_now).to be false + expect(hold_count).to eq(2) + end + end + + it "starts the bound over for a file whose read fails at a load that holds nothing back" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "") + expect(reloader.reload_now).to be false + + allow(FileData).to receive(:read_file).and_raise(ReadError.new(y, "unable to read file: denied")) + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + allow(FileData).to receive(:read_file).and_call_original + + clock.advance(6 * hour) + mark_written(y) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "starts the bound over for a file found missing at a load that holds nothing back" do + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.json", values_doc({ flag3: "c" })) + + with_clocked_reloader([a, b], skip_missing_paths: true) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + + File.delete(a) + File.write(b, '{"flagValues"') + expect(reloader.reload_now).to be false + + # b.json is fixed while a.yaml is created empty again, in the middle of a rewrite. + clock.advance(6 * hour) + File.write(a, "") + File.write(b, values_doc({ flag3: "c" })) + expect(reloader.reload_now).to be false + expect(hold_count).to eq(2) + expect(recorder.applied.length).to eq(1) + end + end + + it "keeps the bound of a file that a failing load did not get to" do + # c.json comes first and fails now and then, so those loads never read a.yaml, which keeps + # being written while empty. They do not give a.yaml a fresh bound. + c = write("c.json", values_doc({ flag3: "c" })) + a = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([c, a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + (1..9).each do |i| + clock.advance(0.5 * hour) + mark_written(a) + File.write(c, i.odd? ? '{"flagValues"' : values_doc({ flag3: "c" })) + reloader.reload_now + end + + clock.advance(0.5 * hour) + mark_written(a) + File.write(c, values_doc({ flag3: "c" })) + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ flag3: "c" }) + end + end + + it "does not start bounds over at a load that holds a file back and then fails" do + # a.yaml and b.yaml are rewritten in turn, so that every load holds one of them back, and + # c.json fails at every other load. Those loads hold a file back, so they start no bound + # over. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.yaml", yaml_doc(flag2: true)) + c = write("c.json", values_doc({ flag3: "c" })) + + with_clocked_reloader([a, b, c]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + applied = false + (1..12).each do |i| + clock.advance(0.5 * hour) + rewritten, emptied = i.odd? ? [a, b] : [b, a] + File.write(rewritten, yaml_doc(rewritten == a ? { flag1: true } : { flag2: true })) + File.write(emptied, "") + File.write(c, i.odd? ? '{"flagValues"' : values_doc({ flag3: "c" })) + reloader.reload_now + applied = recorder.applied.length > 1 + break if applied + end + expect(applied).to be true + end + end + + it "starts a new delay for a file that is created empty again after it was missing" do + # While b.json fails, a.yaml is emptied, deleted, and created empty again with the state it + # had when it was first found empty, as within one tick of a coarse clock. Being missing + # ended its empty state, so it is held back with a new delay. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.json", values_doc({ flag3: "c" })) + + with_clocked_reloader([a, b], skip_missing_paths: true) do |reloader, recorder| + reloader.reload_now + File.write(b, '{"flagValues"') + File.write(a, "") + first_empty_write_time = File.mtime(a) + reloader.reload_now + clock.advance(0.5 * hour) + File.delete(a) + reloader.reload_now + clock.advance(40.0 / 60 * hour) + File.write(a, "") + File.utime(first_empty_write_time, first_empty_write_time, a) + reloader.reload_now + + File.write(b, values_doc({ flag3: "c" })) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + it "asks for the retry when the delay is over, plus a tenth of it" do y = write("a.yaml", yaml_doc(flag1: true)) From 042d3ce9d52595c3182af7dee54fd0e5648aa629 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 07:19:42 +0000 Subject: [PATCH 6/8] fix: Keep the override source running through files that are not UTF-8 A file with a UTF-16 or UTF-32 byte order mark, which Windows PowerShell 5.1 writes with `>`, made the read raise an error that no load rescued. At startup the data system stopped the source; at runtime the worker thread ended, so no change was ever loaded again. - Files are read as bytes, a UTF-8 byte order mark is removed, and the content is taken as UTF-8. Content that is not valid UTF-8, in either format, fails to parse like any other invalid content: the failure is logged and the data stays. A UTF-16 or UTF-32 file is reported as such. - An error that a load does not expect is logged and retried like a failed load, and the worker rescues errors inside its loop, so that it keeps running. - YAML with only white space, including a tab, a vertical tab or a form feed, is a document with no entries, as documented; the YAML parser rejected some of it. - Tests record the delay of the retry that is armed, not the one that was asked for, so that the arming paths are checked. --- lib/ldclient-rb/impl/file_data/document.rb | 38 +++- lib/ldclient-rb/impl/file_data/reloader.rb | 56 ++++-- lib/ldclient-rb/integrations/file_data.rb | 5 +- spec/impl/file_data/document_spec.rb | 35 ++++ spec/impl/file_data/reloader_spec.rb | 170 ++++++++++++++++-- .../integrations/file_override_source_spec.rb | 59 ++++++ 6 files changed, 326 insertions(+), 37 deletions(-) diff --git a/lib/ldclient-rb/impl/file_data/document.rb b/lib/ldclient-rb/impl/file_data/document.rb index 9c4990a2..fa02cfba 100644 --- a/lib/ldclient-rb/impl/file_data/document.rb +++ b/lib/ldclient-rb/impl/file_data/document.rb @@ -74,15 +74,17 @@ def initialize(flags: {}, flag_values: {}, segments: {}) # content. Content that is empty or only white space is not a JSON document, so it fails to # parse. In YAML it is a document with no entries, {NONE}, and so is a YAML document that is # only a null, such as one with only a comment. A document that is not a mapping, or whose - # "flags", "flagValues", or "segments" member is not a mapping, is an error. + # "flags", "flagValues", or "segments" member is not a mapping, is an error, and so is + # content that is not valid UTF-8, in either format. # - # @param content [String] + # @param content [String] UTF-8 content # @param format [Symbol] `:json` or `:yaml` # @return [Document] # @raise [ArgumentError] if the content is not a valid document # @raise [JSON::ParserError, Psych::SyntaxError] if the content cannot be parsed # def self.parse(content, format:) + FileData.check_encoding(content) case format when :json # A file that is being rewritten in place can be empty for a moment. Failing to parse @@ -91,6 +93,9 @@ def self.parse(content, format:) raw = JSON.parse(content) when :yaml + # The YAML parser rejects some white space, such as a tab, that is still no content. + return NONE if FileData.blank?(content) + raw = YAML.safe_load(content) return NONE if raw.nil? else @@ -146,6 +151,21 @@ def self.format_of(path) path.downcase.end_with?(*YAML_FILE_NAME_ENDINGS) ? :yaml : :json end + # + # Raises an error if the content is not valid UTF-8. + # + # @param content [String] + # @raise [ArgumentError] if the content is not valid UTF-8 + # + def self.check_encoding(content) + return if content.valid_encoding? + + if content.b.start_with?(*UTF16_AND_UTF32_BOMS) + raise ArgumentError, "the file is encoded in UTF-16 or UTF-32; data files must be UTF-8" + end + raise ArgumentError, "the file is not valid UTF-8" + end + # # Returns true if the content is empty or only white space. # @@ -156,15 +176,25 @@ def self.blank?(content) content.each_byte.all? { |byte| WHITE_SPACE_BYTES.include?(byte) } end + # The byte order marks of UTF-8 and of the encodings that files must not use. + UTF8_BOM = "\xEF\xBB\xBF".b.freeze + UTF16_AND_UTF32_BOMS = ["\xFF\xFE".b, "\xFE\xFF".b, "\x00\x00\xFE\xFF".b].freeze + private_constant :UTF8_BOM, :UTF16_AND_UTF32_BOMS + # - # Reads the raw content of one file as UTF-8, without a byte order mark. + # Reads the raw content of one file, as UTF-8 and without a UTF-8 byte order mark, whatever + # the process's default encoding. The bytes are not checked here: {Document.parse} rejects + # content that is not valid UTF-8, so that such a file fails to load like any other content + # that cannot be parsed. # # @param path [String] # @return [String] # @raise [ReadError] if the file cannot be read # def self.read_file(path) - File.read(path, mode: "r:BOM|UTF-8") + content = File.binread(path) + content = content.byteslice(UTF8_BOM.bytesize..) if content.start_with?(UTF8_BOM) + content.force_encoding(Encoding::UTF_8) rescue Errno::ENOENT => e raise ReadError.new(path, "unable to read file: #{e.message}", missing: true) rescue SystemCallError, IOError => e diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index f93800df..d448f3e0 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -61,7 +61,7 @@ class Reloader DEFAULT_RETRY_DELAY = 1.0 # A file is held back for at most this many retry delays from the first load that held it - # back, until data is applied. + # back. The bound starts over at a load that holds nothing back. MAX_HOLD_RETRY_DELAYS = 5 # A file whose content is in effect and that loads have found with no content: since when it @@ -142,8 +142,8 @@ def initialize(paths:, logger:, apply:, on_error: nil, @empty_files = {} @read_again_pending = false @holding_this_load = false - # The delay of the last retry that a load asked for. Only tests read it. Guarded by @mutex. - @last_requested_retry_delay = nil + # The delay of the last retry that was armed. Only tests read it. Guarded by @mutex. + @last_armed_retry_delay = nil end # @@ -162,13 +162,13 @@ def reload_now end # - # The delay in seconds of the last retry that a load asked for, armed or not. Only tests - # use it. + # The delay in seconds of the last retry that was armed, from the time it was armed. Only + # tests use it. # # @return [Numeric, nil] # - def last_requested_retry_delay - @mutex.synchronize { @last_requested_retry_delay } + def last_armed_retry_delay + @mutex.synchronize { @last_armed_retry_delay } end # @@ -211,7 +211,6 @@ def stop return unless @retry_delay > 0 @mutex.synchronize do - @last_requested_retry_delay = delay @requested_retry_delay = [@requested_retry_delay, delay].compact.min @cond.signal end @@ -222,18 +221,29 @@ def stop action = next_action break if action == :stop - @logger.info { "#{@log_prefix} Reloading flag data after detecting a change" } if action == :reload - retry_in = reload(retrying: action == :retry) - @mutex.synchronize do - # A pending retry is superseded by this reload. The reload either succeeded, or it - # failed or held back a file and arms a fresh retry here. - @last_requested_retry_delay = retry_in unless retry_in.nil? - @retry_deadline = retry_in.nil? || @retry_delay <= 0 ? nil : monotonic_now + retry_in + # An unexpected error must not end the worker: no reload would happen after it. + begin + @logger.info { "#{@log_prefix} Reloading flag data after detecting a change" } if action == :reload + retry_in = reload(retrying: action == :retry) + @mutex.synchronize do + # A pending retry is superseded by this reload. The reload either succeeded, or it + # failed or held back a file and arms a fresh retry here. + if retry_in.nil? || @retry_delay <= 0 + @retry_deadline = nil + else + @retry_deadline = monotonic_now + retry_in + @last_armed_retry_delay = retry_in + end + end + rescue => e + log_unexpected_error(e) end end - rescue => e - @logger.error { "#{@log_prefix} Unexpected error in file data reloader: #{e.inspect}" } - @logger.debug { "#{@log_prefix} Exception trace: #{e.backtrace}" } + end + + private def log_unexpected_error(error) + @logger.error { "#{@log_prefix} Unexpected error in file data reloader: #{error.inspect}" } + @logger.debug { "#{@log_prefix} Exception trace: #{error.backtrace}" } end # @@ -258,6 +268,7 @@ def stop deadline = now + @requested_retry_delay @requested_retry_delay = nil @retry_deadline = [@retry_deadline, deadline].compact.min + @last_armed_retry_delay = @retry_deadline - now end if !@debounce_deadline.nil? && now >= @debounce_deadline @debounce_deadline = nil @@ -298,7 +309,14 @@ def stop # The files that the load got to and did not hold back, including one whose read or # parse failed. reached = [] - retry_in = load_files(reached) + retry_in = + begin + load_files(reached) + rescue => e + # A failure that the load does not expect is retried like any other. + log_unexpected_error(e) + stopped? ? nil : @retry_delay + end unless @holding_this_load # A load that holds nothing back, whether it applies, is skipped or fails, starts the # bound over for every file that it got to: such a file is no longer held back, so its diff --git a/lib/ldclient-rb/integrations/file_data.rb b/lib/ldclient-rb/integrations/file_data.rb index 229d0327..e1ff7c01 100644 --- a/lib/ldclient-rb/integrations/file_data.rb +++ b/lib/ldclient-rb/integrations/file_data.rb @@ -179,7 +179,10 @@ def self.data_source_v2(options = {}) # # A file whose name ends in `.yaml` or `.yml`, in any case, is read as YAML, and any other file is # read as JSON. The file data source, by contrast, reads every file with the YAML parser, which - # also accepts most JSON. + # also accepts most JSON. Files must be UTF-8, with or without a byte order mark; a file in + # another encoding, such as UTF-16, fails to load. Whether a JSON file may contain comments, or + # the same key more than once in one object, depends on the version of the `json` gem that the + # application uses: version 3 rejects both. # # A reload replaces the entire set of overrides, so removing an entry from a file removes the # override. A configured file that does not exist contributes no overrides. It can be created diff --git a/spec/impl/file_data/document_spec.rb b/spec/impl/file_data/document_spec.rb index 63d8f02a..9f86b7f6 100644 --- a/spec/impl/file_data/document_spec.rb +++ b/spec/impl/file_data/document_spec.rb @@ -66,6 +66,18 @@ module FileData expect(Document.parse("{}", format: :json)).not_to be(Document::NONE) end + it "treats YAML with only white space, including a tab, vertical tab or form feed, as no entries" do + expect(Document.parse("\t\n", format: :yaml)).to be(Document::NONE) + expect(Document.parse(" \v\f\t\r\n", format: :yaml)).to be(Document::NONE) + end + + it "rejects content that is not valid UTF-8, in both formats" do + latin1 = (+"{\"flagValues\": {\"flag1\": \"caf\xE9\"}}").force_encoding(Encoding::UTF_8) + [:json, :yaml].each do |format| + expect { Document.parse(latin1, format: format) }.to raise_error(ArgumentError, "the file is not valid UTF-8") + end + end + it "rejects empty JSON content, which is not a JSON document" do ["", " \n\r\n\t"].each do |content| expect { Document.parse(content, format: :json) } @@ -180,6 +192,29 @@ module FileData expect(Document.read(path).flag_values).to eq({ flag1: "caf\u00e9" }) end + it "reports a file encoded in UTF-16 or UTF-32 as a read error" do + path = File.join(@dir, "flags.json") + content = '{"flagValues": {"flag1": true}}' + ["\xFF\xFE".b + content.encode("UTF-16LE").b, "\xFE\xFF".b + content.encode("UTF-16BE").b, + "\xFF\xFE\x00\x00".b + content.encode("UTF-32LE").b].each do |bytes| + File.binwrite(path, bytes) + + expect { Document.read(path) }.to raise_error(ReadError, /encoded in UTF-16 or UTF-32; data files must be UTF-8/) + end + end + + it "reads a file as UTF-8 whatever the default external encoding" do + path = File.join(@dir, "flags.yaml") + File.binwrite(path, "flagValues:\n flag1: caf\xC3\xA9\n") + previous = Encoding.default_external + begin + Encoding.default_external = Encoding::US_ASCII + expect(Document.read(path).flag_values).to eq({ flag1: "caf\u00e9" }) + ensure + Encoding.default_external = previous + end + end + it "reads a file that holds only a byte order mark as empty" do path = File.join(@dir, "flags.json") File.binwrite(path, "\xEF\xBB\xBF") diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index 05187165..de8db8ce 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -841,6 +841,69 @@ def with_empty_file_reloader(paths, **options, &block) end end + it "keeps reloading after an error that a load does not expect" do + y = write("a.yaml", yaml_doc(flag1: true)) + failures = 1 + allow(FileData).to receive(:read_file).and_wrap_original do |original, path| + raise "unexpected" if (failures -= 1) >= 0 + + original.call(path) + end + + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.2) do |reloader, recorder| + reloader.trigger + expect(wait_for { logger.output.include?("Unexpected error in file data reloader") }).to be true + + # The retry loads the file, and so does a later change signal. + expect(wait_for { recorder.applied.length == 1 }).to be true + File.write(y, yaml_doc(flag2: true)) + reloader.trigger + expect(wait_for { recorder.applied.length == 2 }).to be true + expect(recorder.flag_values).to eq({ flag2: true }) + end + end + + it "keeps the worker running after an error outside a load" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_reloader([y], logger: logger, debounce_delay: 0.01, retry_delay: 0.2) do |reloader, recorder| + failures = 1 + allow(reloader).to receive(:reload).and_wrap_original do |original, **options| + raise "unexpected" if (failures -= 1) >= 0 + + original.call(**options) + end + reloader.trigger + expect(wait_for { logger.output.include?("Unexpected error in file data reloader") }).to be true + + reloader.trigger + expect(wait_for { recorder.applied.length == 1 }).to be true + end + end + + it "replaces an earlier retry with the retry of a later hold" do + # A failure arms the retry for the retry delay. A load soon after holds a file back and + # arms its own retry for the end of that file's delay, which replaces the earlier one, + # so the file is not read again before its delay is over. + a = write("a.yaml", yaml_doc(flag1: true)) + b = write("b.json", values_doc({ flag2: "b" })) + + with_reloader([a, b], logger: logger, debounce_delay: 0.01, retry_delay: 1.0) do |reloader, recorder| + reloader.reload_now + File.write(b, '{"flagValues"') + reloader.trigger + expect(wait_for { recorder.errors.length == 1 }).to be true + + sleep 0.3 + File.write(a, "") + File.write(b, values_doc({ flag2: "b" })) + reloader.trigger + expect(wait_for { hold_count == 1 }).to be true + expect(wait_for(3) { recorder.applied.length == 2 }).to be true + expect(hold_count).to eq(1) + end + end + it "arms the retry of a hold for the rest of the delay, plus a tenth of it" do y = write("a.yaml", yaml_doc(flag1: true)) @@ -896,6 +959,16 @@ def next_tick sleep 0.02 end + # Waits for the worker to arm the retry that a load asked for, and expects its delay. A + # retry that reload_now asks for is armed by the worker. + def expect_armed_retry_delay(reloader, seconds) + armed = wait_for do + delay = reloader.last_armed_retry_delay + !delay.nil? && (delay - seconds).abs < 0.001 + end + expect(armed).to be(true), "expected a retry armed for #{seconds}s, got #{reloader.last_armed_retry_delay.inspect}" + end + # Gives the file a new modification time, as a write while it is empty does. def mark_written(path) @marks = (@marks || 0) + 1 @@ -1090,8 +1163,9 @@ def with_clocked_reloader(paths, **options, &block) end it "holds back a JSON file that stayed empty for long when it is rewritten" do - # The failing loads ended the run of holding loads, so the rewrite is held back although - # the file was first found empty more than five retry delays ago. + # The failing loads held nothing back, so they started the file's bound over, and the + # rewrite is held back although the file was first found empty more than five retry + # delays ago. a = write("a.json", values_doc({ flag1: "a" })) with_clocked_reloader([a]) do |reloader, recorder| @@ -1359,16 +1433,17 @@ def with_clocked_reloader(paths, **options, &block) File.write(a, "") expect(reloader.reload_now).to be false + # One file is truncated again every 0.9 retry delays, the files in turn, with a load at + # each truncation and one 0.2 retry delays later, which stands for the retry that the + # hold arms a little after the other file's delay is over. applied = false (1..14).each do |i| - clock.advance(0.9 * hour) + clock.advance(i == 1 ? 0.9 * hour : 0.7 * hour) mark_written(i.odd? ? b : a) File.write(b, "") if i == 1 reloader.reload_now - # The retry that the hold arms, a little after the other file's delay is over. clock.advance(0.2 * hour) reloader.reload_now - clock.advance(-0.2 * hour) applied = recorder.applied.length > 1 break if applied end @@ -1425,7 +1500,7 @@ def with_clocked_reloader(paths, **options, &block) clock.advance(0.95 * hour) slow = b expect(reloader.reload_now).to be false - expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.1 * hour) + expect_armed_retry_delay(reloader, 0.1 * hour) end end @@ -1605,6 +1680,76 @@ def with_clocked_reloader(paths, **options, &block) end end + it "holds back YAML with only a tab and then applies it as no entries" do + y = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "\t\n") + expect(reloader.reload_now).to be false + + clock.advance(hour) + expect(reloader.reload_now).to be true + expect(recorder.applied[-1].empty?).to be true + expect(recorder.errors).to be_empty + end + end + + it "starts the delay over when a YAML file with no document changes size but not modification time" do + # Partial reads of a file being written can find only a comment, of different lengths, on + # a file system with coarse modification times. + y = write("a.yaml", yaml_doc(flag1: true)) + pinned = Time.at(1_000_000) + + with_clocked_reloader([y]) do |reloader, recorder| + reloader.reload_now + File.write(y, "# one\n") + File.utime(pinned, pinned, y) + expect(reloader.reload_now).to be false + + clock.advance(0.6 * hour) + File.write(y, "# three\n") + File.utime(pinned, pinned, y) + expect(reloader.reload_now).to be false + + clock.advance(0.6 * hour) + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "does not start the bound of a file that it does not hold back" do + # a.json stays empty past its delay while b.yaml holds every load back. Those loads do not + # hold a.json back, so they give it no bound, and its later rewrite gets a full delay. + b = write("b.yaml", yaml_doc(flag2: true)) + a = write("a.json", values_doc({ flag1: "a" })) + + with_clocked_reloader([b, a]) do |reloader, _recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + clock.advance(hour) + expect(reloader.reload_now).to be false + + clock.advance(0.5 * hour) + File.write(b, "") + expect(reloader.reload_now).to be false + 9.times do + clock.advance(0.5 * hour) + mark_written(b) + expect(reloader.reload_now).to be false + end + + # b.yaml's bound is over, and a.json is truncated again. + clock.advance(0.5 * hour) + mark_written(b) + mark_written(a) + holds = hold_count + expect(reloader.reload_now).to be false + expect(hold_count).to eq(holds + 1) + end + end + it "asks for the retry when the delay is over, plus a tenth of it" do y = write("a.yaml", yaml_doc(flag1: true)) @@ -1612,12 +1757,12 @@ def with_clocked_reloader(paths, **options, &block) reloader.reload_now File.write(y, "") expect(reloader.reload_now).to be false - expect(reloader.last_requested_retry_delay).to be_within(0.001).of(1.1 * hour) + expect_armed_retry_delay(reloader, 1.1 * hour) # A later load asks for the rest of the delay, not a whole one. clock.advance(0.5 * hour) expect(reloader.reload_now).to be false - expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.6 * hour) + expect_armed_retry_delay(reloader, 0.6 * hour) end end @@ -1638,7 +1783,7 @@ def with_clocked_reloader(paths, **options, &block) expect(reloader.reload_now).to be false # The file's delay would be over in an hour, but its bound is over in 0.2 hours. - expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.3 * hour) + expect_armed_retry_delay(reloader, 0.3 * hour) end end @@ -1654,7 +1799,7 @@ def with_clocked_reloader(paths, **options, &block) File.write(b, "") expect(reloader.reload_now).to be false - expect(reloader.last_requested_retry_delay).to be_within(0.001).of(0.85 * hour) + expect_armed_retry_delay(reloader, 0.85 * hour) end end @@ -1667,8 +1812,7 @@ def with_clocked_reloader(paths, **options, &block) reloader.trigger expect(wait_for { hold_count == 1 }).to be true - expect(wait_for { !reloader.last_requested_retry_delay.nil? }).to be true - expect(reloader.last_requested_retry_delay).to be_within(0.001).of(1.1 * hour) + expect_armed_retry_delay(reloader, 1.1 * hour) end end @@ -1680,7 +1824,7 @@ def with_clocked_reloader(paths, **options, &block) File.write(a, '{"flagValues"') expect(reloader.reload_now).to be false expect(recorder.errors.length).to eq(1) - expect(reloader.last_requested_retry_delay).to eq(hour) + expect_armed_retry_delay(reloader, hour) end end diff --git a/spec/integrations/file_override_source_spec.rb b/spec/integrations/file_override_source_spec.rb index 06c06753..4df47d21 100644 --- a/spec/integrations/file_override_source_spec.rb +++ b/spec/integrations/file_override_source_spec.rb @@ -527,6 +527,51 @@ def with_source(paths, **options) end end + describe "files that are not UTF-8" do + # Windows PowerShell 5.1 writes a file with `>` in UTF-16 with a byte order mark. + def utf16(text) + "\xFF\xFE".b + text.encode("UTF-16LE").b + end + + it "keeps the overrides in effect when a file is rewritten as UTF-16, and applies the fix" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_source([a]) do |_source, sink| + File.binwrite(a, utf16(values_doc({ flag1: "b" }))) + expect(wait_for { logger.output.include?("encoded in UTF-16 or UTF-32") }).to be true + expect(sink.snapshots.length).to eq 1 + + File.write(a, values_doc({ flag1: "c" })) + expect(wait_for { sink.flag_values == { "flag1" => "c" } }).to be true + expect(logger.output.scan("ERROR").length).to eq 1 + end + end + + it "starts with a UTF-16 file without its overrides, and applies the file once it is fixed" do + a = path("a.json") + File.binwrite(a, utf16(values_doc({ flag1: "a" }))) + + with_source([a]) do |_source, sink| + expect(sink.snapshots).to be_empty + expect(logger.output).to include("encoded in UTF-16 or UTF-32") + + File.write(a, values_doc({ flag1: "b" })) + expect(wait_for { sink.snapshots.length == 1 }).to be true + expect(sink.flag_values).to eq({ "flag1" => "b" }) + end + end + + it "keeps the overrides in effect when a file is rewritten with content that is not UTF-8" do + a = write("a.json", values_doc({ flag1: "a" })) + + with_source([a]) do |_source, sink| + File.binwrite(a, "{\"flagValues\": {\"flag1\": \"caf\xE9\"}}".b) + expect(wait_for { logger.output.include?("the file is not valid UTF-8") }).to be true + expect(sink.snapshots.length).to eq 1 + end + end + end + describe "in-place rewrites with the default timing" do it "never supplies a snapshot without the override while a YAML file is rewritten about once a second" do # Polling once a second, a poll, or the retry, can find the second rewrite's truncation @@ -634,6 +679,20 @@ def with_client(overrides) end end + it "serves a UTF-16 override file once it is fixed, after starting without it" do + a = path("a.json") + File.binwrite(a, "\xFF\xFE".b + { flagValues: { flag: "override-a" } }.to_json.encode("UTF-16LE").b) + source = make_source([a]) + builder = Object.new + builder.define_singleton_method(:build) { |_sdk_key, _config| source } + + with_client(builder) do |client| + expect(client.variation("flag", context, "default")).to eq "ld-value" + File.write(a, { flagValues: { flag: "override-b" } }.to_json) + expect(wait_for { client.variation("flag", context, "default") == "override-b" }).to be true + end + end + it "reports invalid options from client construction" do data_system = DataSystem.custom.overrides(FileData.override_source(paths: [])).build From 9adf2316a2ca4c051247f49d4953c63754bd1c7d Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 07:35:58 +0000 Subject: [PATCH 7/8] fix: Read every file even after one fails, and digest each path and presence A file configured after a failing file kept the bound of a hold from before the failure: every load stopped at the failing file, so the later file was never read and its bound never started over. When a deploy fixed the failing file while rewriting the later one in place, the later one's truncation was applied empty. A load now reads and classifies every configured file even after a read or parse failure, and fails with the first failure after them, before it holds a file back or merges. A load that holds nothing back then starts the bound over for every file. The skip-unchanged digest now covers each path and whether it is present, not only the bytes of the files that are present. The format comes from the name, so the same bytes moved from a JSON file to a YAML file parse differently and are applied. --- lib/ldclient-rb/impl/file_data/reloader.rb | 44 +++++++++---- spec/impl/file_data/reloader_spec.rb | 74 +++++++++++++++++++--- 2 files changed, 97 insertions(+), 21 deletions(-) diff --git a/lib/ldclient-rb/impl/file_data/reloader.rb b/lib/ldclient-rb/impl/file_data/reloader.rb index d448f3e0..132d0d33 100644 --- a/lib/ldclient-rb/impl/file_data/reloader.rb +++ b/lib/ldclient-rb/impl/file_data/reloader.rb @@ -33,9 +33,9 @@ module FileData # is held back for at most five retry delays from the first load that held it back. The bound # goes on across reads that find the file with content, and starts over only at a load that # holds nothing back, whether that load applies, is skipped as unchanged, or fails, for each - # file that load got to, including one whose read or parse failed. A file after one that - # failed keeps its bound. So a file that keeps being rewritten, or files rewritten in turn, - # cannot hold the data back without end. A read that finds a file with content, even content + # file it read. A load reads every file, even after one fails to read or parse, and fails with + # the first failure after them. So a file that keeps being rewritten, or files rewritten in + # turn, cannot hold the data back without end. A read that finds a file with content, even content # that fails to parse, or finds it missing, ends its empty state, so its next emptying starts # a new delay. The retry that a hold arms comes when the first held file's # delay or bound is over, plus a tenth of the retry delay. A file that contributed no content @@ -306,8 +306,8 @@ def stop end @read_again_pending = false @holding_this_load = false - # The files that the load got to and did not hold back, including one whose read or - # parse failed. + # The files that the load read and did not hold back, including those whose read or parse + # failed. A load reads every file unless an error it does not expect stops it. reached = [] retry_in = begin @@ -319,8 +319,8 @@ def stop end unless @holding_this_load # A load that holds nothing back, whether it applies, is skipped 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 one that failed keeps its bound. + # bound over for every file that it read: such a file is no longer held back, so its + # next rewrite is a new one. reached.each do |path| empty = @empty_files[path] empty.first_held = nil unless empty.nil? @@ -336,6 +336,9 @@ def stop files = [] had_content = {} held = [] + # The first read or parse failure. The other files are still read, so that each of them is + # classified, and the load fails with it after them. + failure = nil digest = Digest::SHA256.new @paths.each do |path| begin @@ -351,10 +354,12 @@ def stop if e.missing && @skip_missing_paths @logger.debug { "#{@log_prefix} File #{path} does not exist; it contributes no data" } had_content[path] = false + append_to_digest(digest, path, nil) files << FileSummary.new(path, false, 0, 0) next end - return record_failure(e) + failure ||= e + next end # A file with no content, or YAML that the parser reads as nothing, such as only a # comment, has no entries. A read can see that much of a file that is being written. @@ -368,7 +373,8 @@ def stop rescue ReadError => e # Content that fails to parse is content: the file's empty state is over. not_empty(path, reached) - return record_failure(e) + failure ||= e + next end has_content = !document.equal?(Document::NONE) end @@ -387,17 +393,19 @@ def stop end had_content[path] = has_content - digest << content << "\0" + append_to_digest(digest, path, content) if document.nil? begin document = FileData.parse_file(path, content) rescue ReadError => e - return record_failure(e) + failure ||= e + next end end documents << document files << FileSummary.new(path, true, 0, 0) end + return record_failure(failure) unless failure.nil? return hold_back(held, @clock.call) unless held.empty? begin @@ -429,8 +437,6 @@ def stop recovering = !@last_error_message.nil? hexdigest = digest.hexdigest if @skip_unchanged && !recovering && hexdigest == @last_good_digest - # The digest covers only the content of the files that are present, so the same - # content can now come from another file. remember_applied(had_content) return nil end @@ -449,6 +455,18 @@ def stop nil end + # Adds a file to the skip-unchanged digest: its path, whether it is present, and its content. + # The same content in another file can parse differently, since the format comes from the + # name, and does not come from the same file. + private def append_to_digest(digest, path, content) + digest << path.b << "\0" + if content.nil? + digest << "-\0" + else + digest << "+" << content.bytesize.to_s << "\0" << content.b + end + end + # Records that a load found the file with content, even content that fails to parse, or # missing: its empty state is over, so its next emptying starts a new delay even where a # coarse modification time makes it look the same. Its bound goes on, so that files diff --git a/spec/impl/file_data/reloader_spec.rb b/spec/impl/file_data/reloader_spec.rb index de8db8ce..df3807c6 100644 --- a/spec/impl/file_data/reloader_spec.rb +++ b/spec/impl/file_data/reloader_spec.rb @@ -667,9 +667,10 @@ def with_empty_file_reloader(paths, **options, &block) end end - it "is held back when the data that was kept as unchanged has content from it" do - # The same content moves from one file to another, so the reload is skipped as unchanged, - # but the data in effect now comes from the second file. + it "is held back after the same content moved to it from another file" do + # The same content moves from one file to another. The data in effect now comes from the + # second file, so the move is applied rather than skipped as unchanged, and emptying the + # second file is held back. a = write("a.yaml", yaml_doc(flag1: true)) b = path("b.yaml") @@ -678,11 +679,28 @@ def with_empty_file_reloader(paths, **options, &block) File.write(b, yaml_doc(flag1: true)) File.delete(a) expect(reloader.reload_now).to be true - expect(recorder.applied.length).to eq(1) + expect(recorder.applied.length).to eq(2) + expect(recorder.applied[-1].files.map(&:present)).to eq([false, true]) File.write(b, "") expect(reloader.reload_now).to be false - expect(recorder.applied.length).to eq(1) + expect(recorder.applied.length).to eq(2) + end + end + + it "applies the same bytes moved to a file of another format" do + # 1e3 is a number in JSON and a string in YAML, so the move is not skipped as unchanged. + a = write("a.json", '{"flagValues": {"f": 1e3}}') + b = path("b.yaml") + + with_reloader([a, b], skip_missing_paths: true, skip_unchanged: true) do |reloader, recorder| + reloader.reload_now + expect(recorder.flag_values).to eq({ f: 1000.0 }) + + File.delete(a) + File.write(b, '{"flagValues": {"f": 1e3}}') + expect(reloader.reload_now).to be true + expect(recorder.flag_values).to eq({ f: "1e3" }) end end @@ -1601,9 +1619,9 @@ def with_clocked_reloader(paths, **options, &block) end end - it "keeps the bound of a file that a failing load did not get to" do - # c.json comes first and fails now and then, so those loads never read a.yaml, which keeps - # being written while empty. They do not give a.yaml a fresh bound. + it "applies a file kept written while empty after its bound while an earlier file fails" do + # c.json comes first and fails now and then, while a.yaml keeps being written while empty. + # Those loads hold a.yaml back, so they do not give it a fresh bound. c = write("c.json", values_doc({ flag3: "c" })) a = write("a.yaml", yaml_doc(flag1: true)) @@ -1626,6 +1644,46 @@ def with_clocked_reloader(paths, **options, &block) end end + it "starts the bound of a file after a failing file over once a load reads it again" do + # c.json comes first and is saved with an error while a.yaml is being rewritten. The loads + # that fail on c.json still read a.yaml, with content, so its bound starts over, and its + # next rewrite, when c.json is fixed, is held back. + c = write("c.json", values_doc({ flag3: "c" })) + a = write("a.yaml", yaml_doc(flag1: true)) + + with_clocked_reloader([c, a]) do |reloader, recorder| + reloader.reload_now + File.write(a, "") + expect(reloader.reload_now).to be false + File.write(c, '{"flagValues"') + File.write(a, yaml_doc(flag1: false)) + expect(reloader.reload_now).to be false + 6.times do + clock.advance(hour) + reloader.reload_now + end + + File.write(c, values_doc({ flag3: "c" })) + File.write(a, "") + expect(reloader.reload_now).to be false + expect(recorder.applied.length).to eq(1) + end + end + + it "fails a load with the first failure when several files fail" do + a = write("a.json", values_doc({ flag1: "a" })) + b = write("b.json", values_doc({ flag2: "b" })) + + with_clocked_reloader([a, b]) do |reloader, recorder| + reloader.reload_now + File.write(a, '{"flagValues"') + File.write(b, "[1") + expect(reloader.reload_now).to be false + expect(recorder.errors.length).to eq(1) + expect(recorder.errors[0].path).to eq(a) + end + end + it "does not start bounds over at a load that holds a file back and then fails" do # a.yaml and b.yaml are rewritten in turn, so that every load holds one of them back, and # c.json fails at every other load. Those loads hold a file back, so they start no bound From 2b4266a36093b46938e088db42a22d218e2ec35d Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 09:55:04 +0000 Subject: [PATCH 8/8] test: Pin the format of a file named .yaml or .yml --- spec/impl/file_data/document_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/impl/file_data/document_spec.rb b/spec/impl/file_data/document_spec.rb index 9f86b7f6..544e88b2 100644 --- a/spec/impl/file_data/document_spec.rb +++ b/spec/impl/file_data/document_spec.rb @@ -262,6 +262,8 @@ module FileData describe "format_of" do it "chooses YAML for a name that ends in .yaml or .yml, in any case, and JSON otherwise" do expect(FileData.format_of("/a/flags.yaml")).to eq(:yaml) + expect(FileData.format_of("/a/.yaml")).to eq(:yaml) + expect(FileData.format_of("/a/.yml")).to eq(:yaml) expect(FileData.format_of("/a/flags.YML")).to eq(:yaml) expect(FileData.format_of("/a/flags.json")).to eq(:json) expect(FileData.format_of("/a.yaml/flags")).to eq(:json)