From ee3234c6d885e36b60c5b872687b114a65891f5a Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 28 Sep 2026 13:28:13 -0700 Subject: [PATCH 01/11] feat: Add file data loading code with reload, retry, and polling Adds ldclient.impl.integrations.files.filedata, the file reading, parsing, and merging logic that the file-based override source is built on. A document that starts with an opening brace is parsed as JSON and any other document as YAML. Definitions are decoded into the flag and segment models while the file is read, so an invalid definition fails that load. Files are merged in the configured order with a duplicate keys handling of fail or ignore, and the result records how many entries each file supplied. The Reloader owns the reload cycle: it serializes reloads, debounces change signals with a settle window, keeps the last good data by not applying a failed load, retries a failed load after a bounded delay, reports an identical failure once, and skips an application whose file contents did not change. It can treat a configured file that does not exist as a file with no content. The Poller detects changes by comparing modification time and size on an interval, including files that appear or disappear. The Watcher uses the watchdog package, watches the directory of each file so an absent file is picked up when it appears, matches the destination of a move so a file written by rename is detected, retries a directory that does not exist yet, and reacts only to notifications that can change a file's content or presence. The existing file data sources are not changed and keep their current behavior. A new test module pins that behavior: the flagValues expansion and its evaluation reason, the version fallback, the failure messages, the FDv2 status and error kinds, and the polling and watching rules. --- ldclient/impl/integrations/files/filedata.py | 637 ++++++++++++ .../test_file_data_sources_pinned_behavior.py | 430 ++++++++ .../testing/integrations/test_filedata.py | 933 ++++++++++++++++++ 3 files changed, 2000 insertions(+) create mode 100644 ldclient/impl/integrations/files/filedata.py create mode 100644 ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py create mode 100644 ldclient/testing/integrations/test_filedata.py diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py new file mode 100644 index 00000000..a6536164 --- /dev/null +++ b/ldclient/impl/integrations/files/filedata.py @@ -0,0 +1,637 @@ +""" +File reading, parsing, and merging logic for components that load flag and segment data from +local files. The file-based override source is built on it. The existing file data sources +keep their own implementation and behavior. + +A data file is a JSON or YAML document with optional ``flags``, ``flagValues``, and +``segments`` members. ``flags`` and ``segments`` hold full definitions keyed by key. +``flagValues`` maps a flag key to a single value. It expands into a full flag definition +that returns that value for every context. +""" + +import hashlib +import json +import os +import threading +import time +from dataclasses import dataclass, field +from enum import Enum +from typing import Any, Callable, Dict, List, Optional, Set, Tuple + +from ldclient.impl.model import FeatureFlag, Segment +from ldclient.impl.repeating_task import RepeatingTask +from ldclient.impl.util import log + +have_yaml = False +try: + import yaml + + have_yaml = True +except ImportError: + pass + +have_watchdog = False +try: + import watchdog + import watchdog.events + import watchdog.observers + + have_watchdog = True +except ImportError: + pass + + +# A settle window long enough to coalesce the burst of change notifications produced by a +# single file edit, and short enough to stay responsive. +DEFAULT_DEBOUNCE_DELAY = 0.1 + +# Bounds how long a failed reload can go uncorrected when no further change notification +# arrives, for example when the failure came from reading a file mid-write. Reading a local +# file is cheap, so this can be short. +DEFAULT_RETRY_DELAY = 1.0 + +# The interval between attempts to watch a directory that could not be watched, for example +# because it does not exist yet. +_WATCH_RETRY_INTERVAL = 1.0 + +# The watchdog event types that can change a file's content or presence. Opening or reading a +# file also produces events, and a reload reads the files, so those must not count as changes. +_CHANGE_EVENT_TYPES = frozenset(["created", "modified", "moved", "deleted", "closed"]) + + +class DuplicateKeysHandling(str, Enum): + """ + Determines what happens when the same flag or segment key appears in more than one file. + """ + + FAIL = "fail" + """A duplicated key causes the load to fail.""" + + IGNORE = "ignore" + """Only the first occurrence of a duplicated key is used, in the order the files were given.""" + + +class FileDataError(Exception): + """Base class for the errors raised while loading file data.""" + + +class FileReadError(FileDataError): + """ + Indicates that one of the source files could not be read or parsed. It distinguishes a + per-file failure from a failure to merge the files' contents. + """ + + def __init__(self, path: str, message: str): + super().__init__("%s [%s]" % (message, path)) + self.path = path + + +class DuplicateKeyError(FileDataError): + """Indicates that the same key appears in more than one file and the handling is FAIL.""" + + +@dataclass +class Document: + """The parsed form of a single data file.""" + + flags: Dict[str, FeatureFlag] = field(default_factory=dict) + flag_values: Dict[str, Any] = field(default_factory=dict) + segments: Dict[str, Segment] = field(default_factory=dict) + + +@dataclass +class DocumentSummary: + """Counts the entries the merge kept from one document.""" + + flags: int = 0 + segments: int = 0 + + +@dataclass +class FileSummary: + """Describes one configured file after a load.""" + + path: str + present: bool = False + """False when the file does not exist and missing files are skipped.""" + flags: int = 0 + segments: int = 0 + + +@dataclass +class MergeResult: + """ + The merged items from one or more documents. The dictionaries preserve document order: + all of one document's items precede the next document's. + """ + + flags: Dict[str, FeatureFlag] = field(default_factory=dict) + segments: Dict[str, Segment] = field(default_factory=dict) + documents: List[DocumentSummary] = field(default_factory=list) + """For each input document in order, the number of entries the merge kept from it.""" + files: List[FileSummary] = field(default_factory=list) + """Set by the loader. Describes each configured file in order.""" + + +def abs_file_paths(paths: List[str]) -> List[str]: + """Converts each of the given paths to an absolute path.""" + return [os.path.abspath(p) for p in paths] + + +def make_flag_with_value(key: str, value: Any) -> FeatureFlag: + """ + Expands a flag-key-to-value entry into a full flag definition that returns the given value + for every context. The flag is off and serves its single variation as the off variation. + """ + return FeatureFlag({"key": key, "version": 1, "on": False, "offVariation": 0, "variations": [value]}) + + +def read_file(path: str) -> Document: + """Reads and parses a single data file, which may be in JSON or YAML format.""" + try: + with open(path, "rb") as f: + raw = f.read() + except OSError as e: + raise FileReadError(path, "unable to read file: %s" % e) + try: + return parse_document(raw) + except Exception as e: + raise FileReadError(path, "error parsing file: %s" % e) + + +def parse_document(raw: bytes) -> Document: + """ + Parses the raw content of a data file, which may be JSON or YAML. The parser is chosen the + way the file data sources choose it: the YAML parser reads both formats when the ``pyyaml`` + package is installed, and the standard JSON parser is used when it is not. An empty + document is valid and holds no entries. + """ + text = raw.decode("utf-8") + if have_yaml: + parsed = yaml.safe_load(text) # pyyaml correctly parses JSON too + else: + parsed = json.loads(text) + if parsed is None: + return Document() + if not isinstance(parsed, dict): + raise ValueError("the document must be an object") + + document = Document() + for key, item in _member_items(parsed, "flags").items(): + document.flags[key] = FeatureFlag(_definition(item, key, "flag")) + for key, value in _member_items(parsed, "flagValues").items(): + document.flag_values[key] = value + for key, item in _member_items(parsed, "segments").items(): + document.segments[key] = Segment(_definition(item, key, "segment")) + return document + + +def _member_items(parsed: dict, name: str) -> Dict[str, Any]: + member = parsed.get(name) + if member is None: + return {} + if not isinstance(member, dict): + raise ValueError('"%s" must be an object' % name) + for key in member: + if not isinstance(key, str): + raise ValueError('"%s" has a key that is not a string: %r' % (name, key)) + return member + + +def _definition(item: Any, key: str, kind_name: str) -> dict: + """ + Validates the shape of a flag or segment entry. The entry is stored under its map key. The + file format allows a definition to omit its own key and version, which the model requires, + so those two properties are filled in from the map key and a version of 1. + """ + if not isinstance(item, dict): + raise ValueError('%s "%s" must be an object' % (kind_name, key)) + if "key" not in item: + item["key"] = key + if "version" not in item: + item["version"] = 1 + return item + + +def merge(documents: List[Document], duplicate_keys_handling: DuplicateKeysHandling) -> MergeResult: + """ + Combines the items of the given documents in order, expanding flag-value entries into full + flag definitions and applying the given duplicate keys handling. + """ + result = MergeResult() + seen_flags: Set[str] = set() + seen_segments: Set[str] = set() + + def insert(items: Dict[str, Any], seen: Set[str], kind_name: str, key: str, item: Any) -> bool: + if key in seen: + if duplicate_keys_handling == DuplicateKeysHandling.IGNORE: + return False + raise DuplicateKeyError("%s '%s' is specified by multiple files" % (kind_name, key)) + items[key] = item + seen.add(key) + return True + + for document in documents: + summary = DocumentSummary() + for key, flag in document.flags.items(): + if insert(result.flags, seen_flags, "flag", key, flag): + summary.flags += 1 + for key, value in document.flag_values.items(): + if insert(result.flags, seen_flags, "flag", key, make_flag_with_value(key, value)): + summary.flags += 1 + for key, segment in document.segments.items(): + if insert(result.segments, seen_segments, "segment", key, segment): + summary.segments += 1 + result.documents.append(summary) + return result + + +def load_files(paths: List[str], duplicate_keys_handling: DuplicateKeysHandling, skip_missing_paths: bool = False) -> MergeResult: + """ + Reads, parses, and merges all of the given files in order. Raises :class:`FileReadError` + when a file cannot be read or parsed, :class:`DuplicateKeyError` when a key is duplicated + and the handling is FAIL. When ``skip_missing_paths`` is true, a file that does not exist + contributes no entries instead of failing the load. + """ + merged, _ = _load_files_hashed(paths, duplicate_keys_handling, skip_missing_paths) + return merged + + +def _load_files_hashed(paths: List[str], duplicate_keys_handling: DuplicateKeysHandling, skip_missing_paths: bool) -> Tuple[MergeResult, bytes]: + """ + The load behind :func:`load_files`. It also returns a digest of the raw file contents, so a + caller can tell whether a later load read the same bytes. One read feeds both the digest + and the parse, so the digest can never disagree with the content that was parsed. + """ + documents: List[Document] = [] + files: List[FileSummary] = [] + hasher = hashlib.sha256() + for path in paths: + try: + with open(path, "rb") as f: + raw = f.read() + except FileNotFoundError: + if skip_missing_paths: + log.debug("File %s does not exist; it contributes no data", path) + files.append(FileSummary(path=path)) + continue + raise FileReadError(path, "unable to read file: the file does not exist") + except OSError as e: + raise FileReadError(path, "unable to read file: %s" % e) + hasher.update(raw) + hasher.update(b"\0") + try: + documents.append(parse_document(raw)) + except Exception as e: + raise FileReadError(path, "error parsing file: %s" % e) + files.append(FileSummary(path=path, present=True)) + + merged = merge(documents, duplicate_keys_handling) + # The documents are the present files in order. Copy their counts onto the file summaries. + next_document = 0 + for summary in files: + if summary.present: + summary.flags = merged.documents[next_document].flags + summary.segments = merged.documents[next_document].segments + next_document += 1 + merged.files = files + return merged, hasher.digest() + + +class Reloader: + """ + Owns the reload cycle for a set of data files. It serializes reloads, debounces change + signals, retains the last good result on failure by not calling ``apply``, retries after + failures, and skips applications that would change nothing. + + The worker thread starts on the first :meth:`reload_now` or :meth:`trigger` call, so a + reloader that is constructed but never used does not leak a thread. + """ + + def __init__( + self, + paths: List[str], + duplicate_keys_handling: DuplicateKeysHandling, + apply: Callable[[MergeResult], None], + on_error: Optional[Callable[[Exception], None]] = None, + skip_missing_paths: bool = False, + debounce_delay: float = 0.0, + retry_delay: float = 0.0, + skip_unchanged: bool = False, + ): + """ + :param paths: the files to load, in order. The order determines which file wins under + the duplicate keys handling. + :param duplicate_keys_handling: what to do when the same key appears in more than one file + :param apply: receives each successfully merged result. Calls are serialized. + :param on_error: receives each distinct failure. Repeats of an identical failure do not + call it again until a success re-arms it. Failures are also logged here. + :param skip_missing_paths: when true, a configured file that does not exist contributes + no entries. When false, a missing file fails the load like any other read error. + :param debounce_delay: how long to wait after a trigger for further triggers to settle + before reloading. Zero reloads on every trigger. + :param retry_delay: how long to wait after a failed reload before retrying it + automatically. Zero disables the automatic retry. + :param skip_unchanged: when true, a load whose raw file contents are identical to the + last applied contents does not call ``apply`` + """ + self._paths = list(paths) + self._duplicate_keys_handling = duplicate_keys_handling + self._apply = apply + self._on_error = on_error + self._skip_missing_paths = skip_missing_paths + self._debounce_delay = debounce_delay + self._retry_delay = retry_delay + self._skip_unchanged = skip_unchanged + + # Guards the deadlines, the closed flag, and the worker start. + self._cond = threading.Condition() + self._closed = False + self._started = False + self._debounce_deadline: Optional[float] = None + self._retry_deadline: Optional[float] = None + + # Serializes the load work between reload_now and the worker thread. + self._reload_lock = threading.Lock() + self._last_good_digest: Optional[bytes] = None + self._last_error_message: Optional[str] = None + + def reload_now(self) -> None: + """ + Loads the files synchronously and applies the result or reports the failure. A failure + arms the same automatic retry as a failed triggered reload. + """ + self._ensure_started() + if not self._reload() and self._retry_delay > 0: + with self._cond: + if not self._closed and self._retry_deadline is None: + self._retry_deadline = time.monotonic() + self._retry_delay + self._cond.notify() + + def trigger(self) -> None: + """ + Signals that the files may have changed. A reload happens after the debounce delay. + Signals that arrive while a reload is pending extend the settle window. + """ + self._ensure_started() + with self._cond: + if self._closed: + return + self._debounce_deadline = time.monotonic() + max(self._debounce_delay, 0.0) + self._cond.notify() + + def close(self) -> None: + """ + Stops the reloader. It does not wait for a reload that is already in progress, so such + a reload may still deliver its result shortly after this returns. A reload that has not + yet reached its callbacks does not invoke them. + """ + with self._cond: + self._closed = True + self._cond.notify_all() + + def _ensure_started(self) -> None: + with self._cond: + if self._closed or self._started: + return + self._started = True + thread = threading.Thread(target=self._run, name="ldclient.filedata.reloader", daemon=True) + thread.start() + + def _run(self) -> None: + while True: + is_retry = False + with self._cond: + while True: + if self._closed: + return + now = time.monotonic() + deadlines = [d for d in (self._debounce_deadline, self._retry_deadline) if d is not None] + if len(deadlines) == 0: + self._cond.wait() + continue + next_deadline = min(deadlines) + if next_deadline > now: + self._cond.wait(next_deadline - now) + continue + if self._debounce_deadline is not None and self._debounce_deadline <= now: + # A triggered reload supersedes a pending retry. It either succeeds, or + # it fails and arms a fresh retry below. + self._debounce_deadline = None + self._retry_deadline = None + is_retry = False + else: + self._retry_deadline = None + is_retry = True + break + + if is_retry: + log.debug("Retrying flag data load after earlier failure") + else: + log.info("Reloading flag data after detecting a change") + try: + ok = self._reload() + except Exception as e: + log.exception("Unexpected error while reloading flag data: %s", e) + ok = True + if not ok and self._retry_delay > 0: + with self._cond: + if not self._closed: + self._retry_deadline = time.monotonic() + self._retry_delay + + def _is_closed(self) -> bool: + with self._cond: + return self._closed + + def _reload(self) -> bool: + """ + Performs one full load of all configured files. Returns whether the load succeeded, + which decides whether a retry is armed. A skipped no-op application counts as success. + """ + with self._reload_lock: + if self._is_closed(): + return True + try: + merged, digest = _load_files_hashed(self._paths, self._duplicate_keys_handling, self._skip_missing_paths) + except Exception as e: + return self._fail(e) + + # A close may have happened while the files were being read. Deliver nothing then. + if self._is_closed(): + return True + + # A success right after a failure applies even when the content is unchanged since + # the last success. The consumer heard about the failure and only an application + # tells it that things are good again. + recovering = self._last_error_message is not None + self._last_error_message = None + if self._skip_unchanged and not recovering and digest == self._last_good_digest: + return True + self._last_good_digest = digest + self._apply(merged) + return True + + def _fail(self, err: Exception) -> bool: + if self._is_closed(): + return True + # 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 + # level and do not call on_error again. + message = str(err) + if message == self._last_error_message: + log.debug("Unable to load flag data: %s", err) + return False + self._last_error_message = message + log.error("Unable to load flag data: %s", err) + if self._on_error is not None: + self._on_error(err) + return False + + +FileState = Optional[Tuple[int, int]] + + +def _observe_all(paths: List[str]) -> List[FileState]: + """ + Observes the state of each file: its modification time and size, or None when it does not + exist or cannot be examined. + """ + states: List[FileState] = [] + for path in paths: + try: + info = os.stat(path) + states.append((info.st_mtime_ns, info.st_size)) + except OSError: + states.append(None) + return states + + +class Poller: + """ + Detects changes to a set of files by examining them on a fixed interval. A change to the + modification time or the size of any file invokes the callback. A file that appears or + disappears is also a change. Use it where file system change notifications are not + available or not reliable. + + Detection is generous. The callback can run for a change that does not alter the + effective data. Feed it into a :class:`Reloader`, whose debouncing and skip-unchanged + handling absorb the excess. + """ + + def __init__(self, paths: List[str], interval: float, on_change: Callable[[], None]): + self._paths = list(paths) + self._on_change = on_change + # The files are examined once here, so only later changes invoke the callback. + self._last = _observe_all(self._paths) + self._task = RepeatingTask.at_interval("ldclient.filedata.poll", interval, interval, self._poll) + + def start(self) -> None: + """Starts the polling thread.""" + self._task.start() + + def close(self) -> None: + """ + Stops the poller. It does not wait for an examination or a callback that is in + progress, so the callback can run once more shortly after this returns. + """ + self._task.stop() + + def _poll(self) -> None: + current = _observe_all(self._paths) + changed = current != self._last + self._last = current + if changed: + self._on_change() + + +class Watcher: + """ + Detects changes to a set of files through file system change notifications, using the + ``watchdog`` package. The directory of each file is watched, so a file that does not exist + yet is picked up when it appears. A directory that cannot be watched yet, for example + because it does not exist, is retried on an interval. + + Notifications for the watched paths invoke the callback. The callback can run several times + for one logical edit, so feed it into a :class:`Reloader`. + """ + + def __init__(self, paths: List[str], on_change: Callable[[], None]): + if not have_watchdog: + raise RuntimeError("the watchdog package is required to watch files for changes") + self._on_change = on_change + self._watched_paths: Set[str] = set() + self._lock = threading.Lock() + self._pending_directories: Set[str] = set() + self._retry_task: Optional[RepeatingTask] = None + + directories: List[str] = [] + for path in paths: + absolute = os.path.abspath(path) + real_directory = os.path.realpath(os.path.dirname(absolute)) + self._watched_paths.add(os.path.join(real_directory, os.path.basename(absolute))) + if real_directory not in directories: + directories.append(real_directory) + + watcher = self + + class _Handler(watchdog.events.FileSystemEventHandler): + def on_any_event(self, event): + watcher._handle_event(event) + + self._handler = _Handler() + self._observer = watchdog.observers.Observer() + for directory in directories: + if not self._schedule(directory): + self._pending_directories.add(directory) + try: + self._observer.start() + except Exception as e: + log.error("Unable to start watching files for changes: %s", e) + if len(self._pending_directories) > 0: + self._retry_task = RepeatingTask.at_interval("ldclient.filedata.watch-retry", _WATCH_RETRY_INTERVAL, _WATCH_RETRY_INTERVAL, self._retry_pending) + self._retry_task.start() + + def _schedule(self, directory: str) -> bool: + # The observer accepts a watch on a directory that does not exist and fails later when + # it starts the watch, so the check happens here first. + if not os.path.isdir(directory): + log.warning('Cannot watch directory "%s" for changes yet because it does not exist', directory) + return False + try: + self._observer.schedule(self._handler, directory, recursive=False) + return True + except Exception as e: + log.warning('Cannot watch directory "%s" for changes yet: %s', directory, e) + return False + + def _retry_pending(self) -> None: + with self._lock: + pending = list(self._pending_directories) + for directory in pending: + if self._schedule(directory): + with self._lock: + self._pending_directories.discard(directory) + # Files may have appeared in the directory before the watch was in place. + self._on_change() + with self._lock: + if len(self._pending_directories) == 0 and self._retry_task is not None: + self._retry_task.stop() + + def _handle_event(self, event) -> None: + if getattr(event, "event_type", None) not in _CHANGE_EVENT_TYPES: + return + candidates = [getattr(event, "src_path", None), getattr(event, "dest_path", None)] + for candidate in candidates: + if isinstance(candidate, bytes): + candidate = candidate.decode("utf-8", errors="replace") + if candidate in self._watched_paths: + self._on_change() + return + + def close(self) -> None: + """Stops watching and waits briefly for the observer thread to finish.""" + with self._lock: + if self._retry_task is not None: + self._retry_task.stop() + self._observer.stop() + self._observer.join(timeout=5) diff --git a/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py b/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py new file mode 100644 index 00000000..3950afa5 --- /dev/null +++ b/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py @@ -0,0 +1,430 @@ +""" +Pins the behavior of the two existing file data sources, the FDv1 update processor and the FDv2 +initializer and synchronizer. The file-based override source is built on separate code and +behaves differently in several of these respects. These tests keep the existing sources +observably unchanged: the flagValues expansion and its evaluation reason, the version fallback, +the duplicate key and load failure messages, the FDv2 status and error kinds, and the polling +and watching rules. +""" +import logging +import os +import threading +import time +from typing import Any, Callable, List, Optional + +import pytest + +from ldclient.client import Config, Context, LDClient +from ldclient.datasystem import custom +from ldclient.feature_store import InMemoryFeatureStore +from ldclient.impl.datasource.status import DataSourceUpdateSinkImpl +from ldclient.impl.integrations.files import file_data_sourcev2 +from ldclient.impl.integrations.files.file_data_source import _FileDataSource +from ldclient.impl.integrations.files.file_data_sourcev2 import ( + _FileDataSourceV2, + _PollingAutoUpdaterV2, + _WatchdogAutoUpdaterV2 +) +from ldclient.impl.listeners import Listeners +from ldclient.integrations import Files +from ldclient.interfaces import ( + DataSourceErrorKind, + DataSourceState, + ObjectKind, + Selector +) +from ldclient.testing.mock_components import MockSelectorStore +from ldclient.testing.test_util import SpyListener +from ldclient.versioned_data_kind import FEATURES, SEGMENTS + +have_watchdog = file_data_sourcev2.have_watchdog +watchdog_required = pytest.mark.skipif(not have_watchdog, reason="watchdog is not installed") + +user = Context.create('user') + +DOCUMENT = ''' +{ + "flags": { + "flag1": { + "key": "flag1", + "on": true, + "fallthrough": {"variation": 2}, + "variations": ["fall", "off", "on"] + }, + "flag-versioned": { + "key": "flag-versioned", + "version": 7, + "on": false, + "offVariation": 0, + "variations": ["x"] + } + }, + "flagValues": { + "flag2": "value2" + }, + "segments": { + "seg1": { + "key": "seg1", + "included": ["user1"] + } + } +} +''' + +# The expansion of a flagValues entry: an on flag whose fallthrough serves its single variation. +EXPANDED_FLAG2 = {'key': 'flag2', 'version': 1, 'on': True, 'fallthrough': {'variation': 0}, 'variations': ['value2']} + + +def write_file(path: str, content: str) -> None: + with open(path, 'w') as f: + f.write(content) + + +def make_v1_source(path: str, store: InMemoryFeatureStore, listeners: Optional[Listeners] = None, **kwargs) -> _FileDataSource: + config = Config('SDK_KEY') + if listeners is not None: + config._data_source_update_sink = DataSourceUpdateSinkImpl(store, listeners, Listeners()) + factory = Files.new_data_source(paths=[path], **kwargs) + assert factory is not None + source = factory(config, store, threading.Event()) + assert isinstance(source, _FileDataSource) + return source + + +def make_v2_source(path: str, **kwargs) -> _FileDataSourceV2: + source = Files.new_data_source_v2(paths=[path], **kwargs).build(Config('SDK_KEY')) + assert isinstance(source, _FileDataSourceV2) + return source + + +def v2_changes_by_key(change_set) -> dict: + return {change.key: change for change in change_set.changes} + + +# --------------------------------------------------------------------------- +# flagValues expansion and its evaluation reason +# --------------------------------------------------------------------------- + +def test_v1_expands_flag_values_to_an_on_flag_with_fallthrough(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT) + store = InMemoryFeatureStore() + source = make_v1_source(path, store) + source.start() + try: + assert store.get(FEATURES, 'flag2').to_json_dict() == EXPANDED_FLAG2 + finally: + source.stop() + + +def test_v1_flag_values_evaluate_with_a_fallthrough_reason(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT) + config = Config('SDK_KEY', update_processor_class=Files.new_data_source(paths=[path]), send_events=False) + with LDClient(config) as client: + detail = client.variation_detail('flag2', user, 'default') + assert detail.value == 'value2' + assert detail.variation_index == 0 + assert detail.reason == {'kind': 'FALLTHROUGH'} + + +def test_v2_expands_flag_values_to_an_on_flag_with_fallthrough(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT) + source = make_v2_source(path) + result = source.fetch(MockSelectorStore(Selector.no_selector())) + changes = v2_changes_by_key(result.value.change_set) + assert changes['flag2'].object == EXPANDED_FLAG2 + assert changes['flag2'].version == 1 + + +def test_v2_flag_values_evaluate_with_a_fallthrough_reason(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT) + datasystem = custom().initializers([Files.new_data_source_v2(paths=[path])]).build() + config = Config('SDK_KEY', datasystem_config=datasystem, send_events=False) + with LDClient(config) as client: + detail = client.variation_detail('flag2', user, 'default') + assert detail.value == 'value2' + assert detail.variation_index == 0 + assert detail.reason == {'kind': 'FALLTHROUGH'} + + +# --------------------------------------------------------------------------- +# Version fallback +# --------------------------------------------------------------------------- + +def test_v1_stamps_version_1_on_entries_without_one_and_keeps_explicit_versions(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT) + store = InMemoryFeatureStore() + source = make_v1_source(path, store) + source.start() + try: + assert store.get(FEATURES, 'flag1').version == 1 + assert store.get(SEGMENTS, 'seg1').version == 1 + assert store.get(FEATURES, 'flag-versioned').version == 7 + finally: + source.stop() + + +def test_v2_stamps_version_1_on_entries_without_one_and_keeps_explicit_versions(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, DOCUMENT) + result = make_v2_source(path).fetch(MockSelectorStore(Selector.no_selector())) + changes = v2_changes_by_key(result.value.change_set) + assert changes['flag1'].version == 1 + assert changes['flag1'].object['version'] == 1 + assert changes['seg1'].kind == ObjectKind.SEGMENT + assert changes['seg1'].version == 1 + assert changes['seg1'].object['version'] == 1 + assert changes['flag-versioned'].version == 7 + + +# --------------------------------------------------------------------------- +# Failure messages, statuses, and error kinds +# --------------------------------------------------------------------------- + +def test_v1_logs_the_load_failure_with_the_path_and_reports_invalid_data(tmp_path, caplog): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, '{"flagValues":{') + store = InMemoryFeatureStore() + listeners = Listeners() + spy = SpyListener() + listeners.add(spy) + source = make_v1_source(path, store, listeners) + with caplog.at_level(logging.ERROR): + source.start() + try: + assert store.initialized is False + errors = [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + assert any(m.startswith('Unable to load flag data from "%s": ' % path) for m in errors), errors + assert len(spy.statuses) == 1 + assert spy.statuses[0].error.kind == DataSourceErrorKind.INVALID_DATA + finally: + source.stop() + + +def test_v1_missing_file_fails_the_load_with_the_same_message(tmp_path, caplog): + path = os.path.join(str(tmp_path), 'missing.json') + store = InMemoryFeatureStore() + source = make_v1_source(path, store) + with caplog.at_level(logging.ERROR): + source.start() + try: + assert source.initialized() is False + errors = [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + assert any(m.startswith('Unable to load flag data from "%s": ' % path) for m in errors), errors + finally: + source.stop() + + +def test_v1_duplicate_key_message(tmp_path, caplog): + first = os.path.join(str(tmp_path), 'first.json') + second = os.path.join(str(tmp_path), 'second.json') + write_file(first, '{"flagValues": {"flag1": "a"}}') + write_file(second, '{"flagValues": {"flag1": "b"}}') + store = InMemoryFeatureStore() + config = Config('SDK_KEY') + source = Files.new_data_source(paths=[first, second])(config, store, threading.Event()) + with caplog.at_level(logging.ERROR): + source.start() + try: + assert store.initialized is False + errors = [r.getMessage() for r in caplog.records if r.levelno == logging.ERROR] + assert any('In features, key "flag1" was used more than once' in m for m in errors), errors + finally: + source.stop() + + +def test_v2_fetch_failure_message_names_the_path(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, '{"flagValues":{') + result = make_v2_source(path).fetch(MockSelectorStore(Selector.no_selector())) + assert result.error.startswith('Unable to load flag data from "%s": ' % path) + + +def test_v2_sync_ends_with_off_when_the_initial_load_fails(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, '{"flagValues":{') + source = make_v2_source(path, force_polling=True, poll_interval=0.1) + try: + updates = list(source.sync(MockSelectorStore(Selector.no_selector()))) + assert len(updates) == 1 + assert updates[0].state == DataSourceState.OFF + assert updates[0].change_set is None + assert updates[0].error is not None + assert updates[0].error.kind == DataSourceErrorKind.INVALID_DATA + assert updates[0].error.message.startswith('Unable to load flag data from "%s": ' % path) + finally: + source.stop() + + +def test_v2_sync_reports_invalid_data_when_a_file_becomes_malformed(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, '{"flagValues": {"flag1": true}}') + source = make_v2_source(path, force_polling=True, poll_interval=0.1) + updates: List[Any] = [] + received = threading.Event() + + def collect(): + for update in source.sync(MockSelectorStore(Selector.no_selector())): + updates.append(update) + received.set() + if len(updates) >= 2: + break + + thread = threading.Thread(target=collect, daemon=True) + thread.start() + try: + assert received.wait(5) + assert updates[0].state == DataSourceState.VALID + received.clear() + time.sleep(0.2) + write_file(path, '{"flagValues"') + assert received.wait(5) + assert updates[1].state == DataSourceState.INTERRUPTED + assert updates[1].error.kind == DataSourceErrorKind.INVALID_DATA + assert updates[1].error.message.startswith('Unable to load flag data from "%s": ' % path) + finally: + source.stop() + thread.join(5) + + +# --------------------------------------------------------------------------- +# Polling rules: modification time only, a missing file is not a change +# --------------------------------------------------------------------------- + +class Counter: + def __init__(self): + self.count = 0 + + def __call__(self): + self.count += 1 + + +def set_mtime(path: str, seconds: float) -> None: + os.utime(path, (seconds, seconds)) + + +@pytest.fixture(params=['v1', 'v2']) +def make_poller(request): + pollers = [] + + def factory(paths: List[str], on_change: Callable[[], None]): + # The interval is long, so only the direct _poll calls below examine the files. + poller: Any + if request.param == 'v1': + poller = _FileDataSource.PollingAutoUpdater(paths, on_change, 1000) + else: + poller = _PollingAutoUpdaterV2(paths, on_change, 1000) + pollers.append(poller) + return poller + + yield factory + for poller in pollers: + poller.stop() + + +def test_poller_reloads_when_the_modification_time_changes(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'aaa') + set_mtime(path, 1000000) + counter = Counter() + poller = make_poller([path], counter) + write_file(path, 'bbb') + set_mtime(path, 2000000) + poller._poll() + assert counter.count == 1 + + +def test_poller_ignores_a_size_change_with_the_same_modification_time(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'aaa') + set_mtime(path, 1000000) + counter = Counter() + poller = make_poller([path], counter) + write_file(path, 'aaaa') + set_mtime(path, 1000000) + poller._poll() + assert counter.count == 0 + + +def test_poller_ignores_a_file_that_disappears(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'aaa') + counter = Counter() + poller = make_poller([path], counter) + os.remove(path) + poller._poll() + assert counter.count == 0 + + +def test_poller_reloads_when_a_missing_file_appears(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + counter = Counter() + poller = make_poller([path], counter) + write_file(path, 'aaa') + poller._poll() + assert counter.count == 1 + + +# --------------------------------------------------------------------------- +# Watching rules: any notification on the file's path reloads, a move destination does not +# --------------------------------------------------------------------------- + +def handlers_of(observer) -> list: + return [handler for handlers in observer._handlers.values() for handler in handlers] + + +@pytest.fixture(params=['v1', 'v2']) +def make_watcher(request): + watchers = [] + + def factory(paths: List[str], on_change: Callable[[], None]): + watcher: Any + if request.param == 'v1': + watcher = _FileDataSource.WatchdogAutoUpdater(paths, on_change) + else: + watcher = _WatchdogAutoUpdaterV2(paths, on_change) + watchers.append(watcher) + return watcher + + yield factory + for watcher in watchers: + watcher.stop() + + +@watchdog_required +def test_watcher_reloads_on_any_notification_for_the_file_path(tmp_path, make_watcher): + import watchdog.events + + path = os.path.realpath(os.path.join(str(tmp_path), 'data.json')) + write_file(path, 'aaa') + counter = Counter() + watcher = make_watcher([path], counter) + handlers = handlers_of(watcher._observer) + assert len(handlers) == 1 + handler = handlers[0] + handler.on_any_event(watchdog.events.FileModifiedEvent(path)) + handler.on_any_event(watchdog.events.FileOpenedEvent(path)) + handler.on_any_event(watchdog.events.FileClosedNoWriteEvent(path)) + assert counter.count == 3 + handler.on_any_event(watchdog.events.FileModifiedEvent(os.path.join(os.path.dirname(path), 'other.json'))) + assert counter.count == 3 + + +@watchdog_required +def test_watcher_ignores_a_move_whose_destination_is_the_file_path(tmp_path, make_watcher): + import watchdog.events + + path = os.path.realpath(os.path.join(str(tmp_path), 'data.json')) + temp = os.path.realpath(os.path.join(str(tmp_path), 'data.json.tmp')) + write_file(path, 'aaa') + counter = Counter() + watcher = make_watcher([path], counter) + handler = handlers_of(watcher._observer)[0] + handler.on_any_event(watchdog.events.FileMovedEvent(temp, path)) + assert counter.count == 0 + handler.on_any_event(watchdog.events.FileMovedEvent(path, temp)) + assert counter.count == 1 diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py new file mode 100644 index 00000000..4cf20945 --- /dev/null +++ b/ldclient/testing/integrations/test_filedata.py @@ -0,0 +1,933 @@ +import os +import threading +import time +from queue import Empty, Queue +from typing import Any, Dict, List + +import pytest + +try: + import yaml +except ImportError: # pragma: no cover + yaml = None # type: ignore + +from ldclient.impl.integrations.files.filedata import ( + Document, + DuplicateKeyError, + DuplicateKeysHandling, + FileReadError, + FileSummary, + MergeResult, + Poller, + Reloader, + Watcher, + abs_file_paths, + have_watchdog, + have_yaml, + load_files, + make_flag_with_value, + merge, + parse_document, + read_file +) +from ldclient.impl.model import FeatureFlag, Segment + +TEST_TIMEOUT = 5.0 + +# Timings for the reloader and poller tests. They are generous multiples of the configured +# delays so the tests stay deterministic on a loaded machine. +SHORT_DELAY = 0.05 +QUIET_PERIOD = 0.3 + + +def write_file(path: str, content: str) -> None: + with open(path, 'w') as f: + f.write(content) + + +def take(queue: Queue, timeout: float = TEST_TIMEOUT) -> Any: + try: + return queue.get(timeout=timeout) + except Empty: + pytest.fail("timed out waiting for a callback") + + +def require_quiet(queue: Queue, duration: float = QUIET_PERIOD) -> None: + try: + item = queue.get(timeout=duration) + except Empty: + return + pytest.fail("received an unexpected callback: %r" % (item,)) + + +# --------------------------------------------------------------------------- +# Parsing +# --------------------------------------------------------------------------- + +def test_parse_json_document(): + document = parse_document(b'{"flags": {"flag1": {"key": "flag1", "version": 3, "on": true}}, "flagValues": {"flag2": "value2"}, "segments": {"seg1": {"key": "seg1", "version": 2, "included": ["user1"]}}}') + assert list(document.flags.keys()) == ['flag1'] + assert isinstance(document.flags['flag1'], FeatureFlag) + assert document.flags['flag1'].version == 3 + assert document.flags['flag1'].on is True + assert document.flag_values == {'flag2': 'value2'} + assert isinstance(document.segments['seg1'], Segment) + assert document.segments['seg1'].included == {'user1'} + + +def test_parse_chooses_the_parser_the_way_the_file_data_source_does(): + # The parser depends only on whether pyyaml is installed, never on the content. A tab inside + # a string is invalid JSON but valid YAML, so with pyyaml this document parses, and a parser + # chosen from the leading brace would reject it. + if not have_yaml: + pytest.skip("pyyaml is not installed") + document = parse_document(b' \n {"flagValues": {"flag1": "a\tb"}}') + assert document.flag_values == {'flag1': 'a\tb'} + + +def test_parse_yaml_document(): + if not have_yaml: + pytest.skip("pyyaml is not installed") + document = parse_document(b'---\nflags:\n flag1:\n key: flag1\n "on": true\nflagValues:\n flag2: value2\nsegments:\n seg1:\n key: seg1\n') + assert document.flags['flag1'].on is True + assert document.flag_values == {'flag2': 'value2'} + assert list(document.segments.keys()) == ['seg1'] + + +def test_parse_empty_document_has_no_entries(): + document = parse_document(b'') + assert document == Document() + document = parse_document(b'{}') + assert document == Document() + + +def test_parse_fills_in_missing_key_and_version(): + document = parse_document(b'{"flags": {"flag1": {"on": true}}, "segments": {"seg1": {}}}') + assert document.flags['flag1'].key == 'flag1' + assert document.flags['flag1'].version == 1 + assert document.segments['seg1'].key == 'seg1' + assert document.segments['seg1'].version == 1 + + +def test_parse_rejects_documents_that_are_not_objects(): + if have_yaml: + with pytest.raises(ValueError): + parse_document(b'- a\n- b\n') + with pytest.raises(ValueError): + parse_document(b'{"flags": ["not", "an", "object"]}') + with pytest.raises(ValueError): + parse_document(b'{"flags": {"flag1": "not an object"}}') + with pytest.raises(ValueError): + parse_document(b'{"segments": {"seg1": 3}}') + with pytest.raises(ValueError): + parse_document(b'{"flagValues": 3}') + + +def test_parse_rejects_malformed_json(): + # With pyyaml the YAML parser reports the error, as in the file data sources. + expected = (ValueError, yaml.YAMLError) if have_yaml else (ValueError,) + with pytest.raises(expected): + parse_document(b'{"flagValues"') + + +def test_parse_validates_definition_property_types(): + with pytest.raises(ValueError): + parse_document(b'{"flags": {"flag1": {"key": "flag1", "version": "not a number"}}}') + with pytest.raises(ValueError): + parse_document(b'{"segments": {"seg1": {"key": "seg1", "version": 1, "included": "not a list"}}}') + + +def test_read_file_json_and_errors(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, '{"flagValues": {"flag1": true}}') + assert read_file(path).flag_values == {'flag1': True} + + with pytest.raises(FileReadError) as excinfo: + read_file(os.path.join(str(tmp_path), 'missing.json')) + assert excinfo.value.path == os.path.join(str(tmp_path), 'missing.json') + assert 'unable to read file' in str(excinfo.value) + + write_file(path, '{"flagValues"') + with pytest.raises(FileReadError) as excinfo: + read_file(path) + assert 'error parsing file' in str(excinfo.value) + assert path in str(excinfo.value) + + +def test_abs_file_paths(): + paths = abs_file_paths(['relative/data.json', '/absolute/data.json']) + assert paths[0] == os.path.abspath('relative/data.json') + assert paths[1] == '/absolute/data.json' + + +def test_make_flag_with_value_is_off_and_serves_the_value(): + flag = make_flag_with_value('flag1', 'value1') + assert flag.key == 'flag1' + assert flag.version == 1 + assert flag.on is False + assert flag.off_variation == 0 + assert flag.variations == ['value1'] + assert flag.to_json_dict() == {'key': 'flag1', 'version': 1, 'on': False, 'offVariation': 0, 'variations': ['value1']} + + +# --------------------------------------------------------------------------- +# Merging +# --------------------------------------------------------------------------- + +def doc(json_text: str) -> Document: + return parse_document(json_text.encode('utf-8')) + + +def test_merge_combines_documents(): + result = merge([ + doc('{"flags": {"flag1": {"key": "flag1", "version": 1}}, "segments": {"seg1": {"key": "seg1", "version": 1}}}'), + doc('{"flagValues": {"flag2": "value2"}}'), + ], DuplicateKeysHandling.FAIL) + assert list(result.flags.keys()) == ['flag1', 'flag2'] + assert list(result.segments.keys()) == ['seg1'] + assert result.flags['flag2'].variations == ['value2'] + assert result.flags['flag2'].on is False + + +def test_merge_duplicate_keys_fail(): + documents = [doc('{"flagValues": {"flag1": "a"}}'), doc('{"flags": {"flag1": {"key": "flag1", "version": 1}}}')] + with pytest.raises(DuplicateKeyError) as excinfo: + merge(documents, DuplicateKeysHandling.FAIL) + assert "flag 'flag1' is specified by multiple files" in str(excinfo.value) + + segment_documents = [doc('{"segments": {"seg1": {"key": "seg1"}}}'), doc('{"segments": {"seg1": {"key": "seg1"}}}')] + with pytest.raises(DuplicateKeyError) as excinfo: + merge(segment_documents, DuplicateKeysHandling.FAIL) + assert "segment 'seg1' is specified by multiple files" in str(excinfo.value) + + +def test_merge_duplicate_keys_within_one_document_between_flags_and_flag_values_fail(): + documents = [doc('{"flags": {"flag1": {"key": "flag1", "version": 1}}, "flagValues": {"flag1": "a"}}')] + with pytest.raises(DuplicateKeyError): + merge(documents, DuplicateKeysHandling.FAIL) + + +def test_merge_duplicate_keys_ignore_keeps_first(): + result = merge([ + doc('{"flagValues": {"flag1": "first"}}'), + doc('{"flagValues": {"flag1": "second", "flag2": "other"}}'), + ], DuplicateKeysHandling.IGNORE) + assert result.flags['flag1'].variations == ['first'] + assert list(result.flags.keys()) == ['flag1', 'flag2'] + + +def test_merge_preserves_document_order(): + result = merge([ + doc('{"flagValues": {"b": 1, "a": 2}}'), + doc('{"flagValues": {"c": 3}}'), + doc('{"flags": {"d": {"key": "d", "version": 1}}}'), + ], DuplicateKeysHandling.FAIL) + assert list(result.flags.keys()) == ['b', 'a', 'c', 'd'] + + +def test_merge_counts_entries_kept_from_each_document(): + result = merge([ + doc('{"flagValues": {"flag1": true, "flag2": false}, "segments": {"seg": {"key": "seg"}}}'), + doc('{"flagValues": {"flag2": true, "flag3": true}}'), + doc('{}'), + ], DuplicateKeysHandling.IGNORE) + assert [(d.flags, d.segments) for d in result.documents] == [(2, 1), (1, 0), (0, 0)] + + +# --------------------------------------------------------------------------- +# Loading files +# --------------------------------------------------------------------------- + +def test_load_files_fails_on_missing_path_by_default(tmp_path): + present = os.path.join(str(tmp_path), 'present.json') + missing = os.path.join(str(tmp_path), 'missing.json') + write_file(present, '{"flagValues": {"flag1": true}}') + with pytest.raises(FileReadError) as excinfo: + load_files([present, missing], DuplicateKeysHandling.FAIL) + assert excinfo.value.path == missing + + +def test_load_files_skips_missing_paths_when_configured(tmp_path): + present = os.path.join(str(tmp_path), 'present.json') + missing = os.path.join(str(tmp_path), 'missing.json') + write_file(present, '{"flagValues": {"flag1": true}}') + result = load_files([present, missing], DuplicateKeysHandling.FAIL, skip_missing_paths=True) + assert list(result.flags.keys()) == ['flag1'] + assert result.files == [FileSummary(path=present, present=True, flags=1), FileSummary(path=missing, present=False)] + + +def test_load_files_reports_parse_error_with_path(tmp_path): + path = os.path.join(str(tmp_path), 'bad.json') + write_file(path, '{"flagValues"') + with pytest.raises(FileReadError) as excinfo: + load_files([path], DuplicateKeysHandling.FAIL) + assert excinfo.value.path == path + + +def test_load_files_reports_duplicate_keys(tmp_path): + first = os.path.join(str(tmp_path), 'first.json') + second = os.path.join(str(tmp_path), 'second.json') + write_file(first, '{"flagValues": {"flag1": "first"}}') + write_file(second, '{"flagValues": {"flag1": "second"}}') + with pytest.raises(DuplicateKeyError): + load_files([first, second], DuplicateKeysHandling.FAIL) + result = load_files([first, second], DuplicateKeysHandling.IGNORE) + assert result.flags['flag1'].variations == ['first'] + assert result.files[0].flags == 1 + assert result.files[1].flags == 0 + + +# --------------------------------------------------------------------------- +# Reloader +# --------------------------------------------------------------------------- + +class ReloaderFixture: + def __init__(self, tmp_path, initial_content: str, **kwargs): + self.path = os.path.join(str(tmp_path), 'data.json') + self.applied: Queue = Queue() + self.errored: Queue = Queue() + write_file(self.path, initial_content) + options: Dict[str, Any] = dict( + paths=[self.path], + duplicate_keys_handling=DuplicateKeysHandling.FAIL, + apply=self.applied.put, + on_error=self.errored.put, + ) + options.update(kwargs) + self.reloader = Reloader(**options) + + def write(self, content: str) -> None: + write_file(self.path, content) + + def require_applied(self) -> MergeResult: + return take(self.applied) + + def require_errored(self) -> Exception: + return take(self.errored) + + def require_quiet(self, duration: float = QUIET_PERIOD) -> None: + deadline = time.time() + duration + while time.time() < deadline: + if not self.applied.empty(): + pytest.fail("unexpected apply call") + if not self.errored.empty(): + pytest.fail("unexpected on_error call") + time.sleep(0.01) + + def close(self) -> None: + self.reloader.close() + + +@pytest.fixture +def make_fixture(tmp_path): + fixtures: List[ReloaderFixture] = [] + + def factory(initial_content: str, **kwargs) -> ReloaderFixture: + fixture = ReloaderFixture(tmp_path, initial_content, **kwargs) + fixtures.append(fixture) + return fixture + + yield factory + for fixture in fixtures: + fixture.close() + + +def test_reloader_initial_load(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.reload_now() + result = f.require_applied() + assert list(result.flags.keys()) == ['flag1'] + assert result.files == [FileSummary(path=f.path, present=True, flags=1)] + + +def test_reloader_fails_on_missing_path_by_default(make_fixture, tmp_path): + missing = os.path.join(str(tmp_path), 'missing.json') + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader = Reloader([f.path, missing], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put) + f.reloader.reload_now() + err = f.require_errored() + assert isinstance(err, FileReadError) + assert err.path == missing + f.require_quiet(0.1) + + +def test_reloader_skips_missing_paths_when_configured(make_fixture, tmp_path): + second = os.path.join(str(tmp_path), 'second.json') + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader = Reloader([f.path, second], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, skip_missing_paths=True, skip_unchanged=True) + + # Step 1: one file exists and one does not. The reload succeeds with the existing file. + f.reloader.reload_now() + result = f.require_applied() + assert list(result.flags.keys()) == ['flag1'] + assert result.files == [FileSummary(path=f.path, present=True, flags=1), FileSummary(path=second, present=False)] + + # Step 2: the missing file appears. Its data is merged in. + write_file(second, '{"flagValues": {"flag2": true}}') + f.reloader.reload_now() + result = f.require_applied() + assert list(result.flags.keys()) == ['flag1', 'flag2'] + + # Step 3: the file is deleted. Its data is gone and the reload still succeeds. + os.remove(second) + f.reloader.reload_now() + result = f.require_applied() + assert list(result.flags.keys()) == ['flag1'] + f.require_quiet(0.1) + + +def test_reloader_reports_failure_and_applies_nothing(make_fixture): + f = make_fixture('{"flagValues"') + f.reloader.reload_now() + err = f.require_errored() + assert isinstance(err, FileReadError) + f.require_quiet(0.1) + + +def test_reloader_reports_merge_failure(make_fixture, tmp_path): + second = os.path.join(str(tmp_path), 'second.json') + write_file(second, '{"flagValues": {"flag1": "dup"}}') + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader = Reloader([f.path, second], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put) + f.reloader.reload_now() + err = f.require_errored() + assert isinstance(err, DuplicateKeyError) + + +def test_reloader_debounce_coalesces_triggers(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}', debounce_delay=0.2) + for _ in range(20): + f.reloader.trigger() + f.require_applied() + f.require_quiet() + + +def test_reloader_without_debounce_reloads_on_each_trigger(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.trigger() + f.require_applied() + f.reloader.trigger() + f.require_applied() + + +def test_reloader_debounce_window_is_extended_by_each_trigger(make_fixture): + # The debounce is a settle window: each trigger moves the deadline out again. A stream + # of triggers spaced closer together than the window must produce no reload while the + # stream continues, and exactly one reload after it stops. + window = 0.25 + f = make_fixture('{"flagValues": {"flag1": true}}', debounce_delay=window) + stop = time.time() + 5 * window + while time.time() < stop: + f.reloader.trigger() + time.sleep(window / 5) + assert f.applied.empty(), "a reload ran while triggers were still arriving" + f.require_applied() + f.require_quiet(2 * window) + + +def test_reloader_reports_identical_failure_only_once(make_fixture): + f = make_fixture('{"flagValues"', retry_delay=SHORT_DELAY) + f.reloader.reload_now() + f.require_errored() + # The automatic retries keep failing in the same way. They do not report again. + f.require_quiet() + + # A different failure is reported. + f.write('{"flags": {"flag1": "not an object"}}') + f.require_errored() + f.require_quiet() + + +def test_reloader_unused_spawns_no_thread(make_fixture): + before = threading.active_count() + f = make_fixture('{"flagValues": {"flag1": true}}') + assert threading.active_count() == before + f.reloader.reload_now() + assert threading.active_count() == before + 1 + f.require_applied() + + +def test_reloader_close_does_not_wait_for_in_flight_reload(make_fixture): + entered = threading.Event() + release = threading.Event() + + def blocking_apply(result: MergeResult) -> None: + entered.set() + # The callback stays parked for far longer than close is given, so a close that waits + # for the in-flight reload is detected as a failure rather than hidden by this timeout. + release.wait(TEST_TIMEOUT * 6) + + f = make_fixture('{"flagValues": {"flag1": true}}', apply=blocking_apply) + f.reloader.trigger() + assert entered.wait(TEST_TIMEOUT), "the reload did not start" + + closed = threading.Event() + + def do_close(): + f.reloader.close() + closed.set() + + threading.Thread(target=do_close, daemon=True).start() + assert closed.wait(2.0), "close blocked on an in-flight reload" + release.set() + + +def test_reloader_worker_thread_exits_after_close(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.reload_now() + f.require_applied() + worker = [t for t in threading.enumerate() if t.name == 'ldclient.filedata.reloader'] + assert len(worker) == 1 + f.reloader.close() + worker[0].join(TEST_TIMEOUT) + assert not worker[0].is_alive() + + +def test_reloader_retries_after_failure_without_further_triggers(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}', retry_delay=SHORT_DELAY) + f.reloader.reload_now() + f.require_applied() + + f.write('{"flagValues"') + f.reloader.trigger() + f.require_errored() + + # Fix the file without triggering. Only the automatic retry can observe the fix. + f.write('{"flagValues": {"flag1": false}}') + f.require_applied() + + +def test_reloader_retries_after_failed_initial_load(make_fixture): + f = make_fixture('{"flagValues"', retry_delay=SHORT_DELAY) + f.reloader.reload_now() + f.require_errored() + f.write('{"flagValues": {"flag1": true}}') + f.require_applied() + + +def test_reloader_stops_retrying_after_success(make_fixture): + f = make_fixture('{"flagValues"', retry_delay=SHORT_DELAY, skip_unchanged=True) + f.reloader.reload_now() + f.require_errored() + + f.write('{"flagValues": {"flag1": true}}') + f.require_applied() + + # After the successful reload there are no further attempts: a changed file is not + # picked up without a trigger. + f.write('{"flagValues": {"flag1": false}}') + f.require_quiet() + + +def test_reloader_does_not_retry_when_retry_delay_is_zero(make_fixture): + f = make_fixture('{"flagValues"') + f.reloader.reload_now() + f.require_errored() + f.write('{"flagValues": {"flag1": true}}') + f.require_quiet() + + +def test_reloader_skip_unchanged(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}', skip_unchanged=True) + f.reloader.reload_now() + f.require_applied() + + f.reloader.trigger() + f.require_quiet() + + f.write('{"flagValues": {"flag1": false}}') + f.reloader.trigger() + f.require_applied() + + +def test_reloader_recovery_applies_even_when_content_unchanged(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}', skip_unchanged=True) + f.reloader.reload_now() + f.require_applied() + + # A reload fails. Consumers hear about it and may move to an interrupted state. + os.remove(f.path) + f.reloader.trigger() + f.require_errored() + + # The file comes back with byte-identical content. The success is applied despite + # skip_unchanged, because only an application tells the consumer the interruption is over. + f.write('{"flagValues": {"flag1": true}}') + f.reloader.trigger() + f.require_applied() + + # Once recovered, identical content skips again. + f.reloader.trigger() + f.require_quiet() + + +def test_reloader_applies_every_reload_when_skip_unchanged_is_off(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.reload_now() + f.require_applied() + f.reloader.trigger() + f.require_applied() + + +def test_reloader_merges_multiple_files_in_order(make_fixture, tmp_path): + second = os.path.join(str(tmp_path), 'second.json') + write_file(second, '{"flagValues": {"flag1": "second"}}') + f = make_fixture('{"flagValues": {"flag1": "first"}}') + f.reloader = Reloader([f.path, second], DuplicateKeysHandling.IGNORE, apply=f.applied.put, on_error=f.errored.put) + f.reloader.reload_now() + result = f.require_applied() + assert result.flags['flag1'].variations == ['first'] + + +def test_reloader_does_nothing_after_close(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.reload_now() + f.require_applied() + f.reloader.close() + f.reloader.close() + f.reloader.trigger() + f.reloader.reload_now() + f.require_quiet(0.1) + + +def test_reloader_serializes_reload_now_against_worker_reloads(make_fixture): + # Concurrent reload_now calls and triggers must not interleave: every application is a + # complete merged result, and the callback never runs on two threads at once. + in_apply = threading.Lock() + overlaps: List[bool] = [] + + def apply(result: MergeResult) -> None: + acquired = in_apply.acquire(blocking=False) + overlaps.append(not acquired) + try: + assert list(result.flags.keys()) == ['flag1'] + time.sleep(0.002) + finally: + if acquired: + in_apply.release() + + f = make_fixture('{"flagValues": {"flag1": true}}', apply=apply) + threads = [threading.Thread(target=lambda: [f.reloader.reload_now() for _ in range(20)]) for _ in range(4)] + for t in threads: + t.start() + for _ in range(20): + f.reloader.trigger() + for t in threads: + t.join(TEST_TIMEOUT) + f.reloader.close() + assert not any(overlaps) + + +def test_reloader_survives_an_apply_callback_that_raises(make_fixture): + calls: Queue = Queue() + + def apply(result: MergeResult) -> None: + calls.put(result) + raise RuntimeError("consumer failure") + + f = make_fixture('{"flagValues": {"flag1": true}}', apply=apply) + f.reloader.trigger() + take(calls) + f.reloader.trigger() + take(calls) + + +# --------------------------------------------------------------------------- +# Poller +# --------------------------------------------------------------------------- + +POLL_INTERVAL = 0.05 + + +class PollerFixture: + def __init__(self, paths: List[str]): + self.changes: Queue = Queue() + self.poller = Poller(paths, POLL_INTERVAL, lambda: self.changes.put(True)) + self.poller.start() + + def require_change(self) -> None: + take(self.changes) + + def require_no_change(self, duration: float = QUIET_PERIOD) -> None: + require_quiet(self.changes, duration) + + +@pytest.fixture +def make_poller(): + pollers: List[PollerFixture] = [] + + def factory(paths: List[str]) -> PollerFixture: + fixture = PollerFixture(paths) + pollers.append(fixture) + return fixture + + yield factory + for fixture in pollers: + fixture.poller.close() + + +def set_mtime(path: str, seconds: float) -> None: + os.utime(path, (seconds, seconds)) + + +def test_poller_detects_modification(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + p = make_poller([path]) + p.require_no_change(0.15) + write_file(path, 'bb') + p.require_change() + + +def test_poller_detects_same_size_rewrite_with_new_mtime(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'aaa') + set_mtime(path, 1000000) + p = make_poller([path]) + write_file(path, 'bbb') + set_mtime(path, 2000000) + p.require_change() + + +def test_poller_detects_size_change_with_same_mtime(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'aaa') + set_mtime(path, 1000000) + p = make_poller([path]) + write_file(path, 'aaaa') + set_mtime(path, 1000000) + p.require_change() + + +def test_poller_fires_once_per_change(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + p = make_poller([path]) + write_file(path, 'bb') + p.require_change() + p.require_no_change() + + +def test_poller_detects_file_appearing(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + p = make_poller([path]) + p.require_no_change(0.15) + write_file(path, 'a') + p.require_change() + + +def test_poller_detects_file_disappearing(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + p = make_poller([path]) + os.remove(path) + p.require_change() + + +def test_poller_watches_all_files(tmp_path, make_poller): + first = os.path.join(str(tmp_path), 'first.json') + second = os.path.join(str(tmp_path), 'second.json') + write_file(first, 'a') + write_file(second, 'a') + p = make_poller([first, second]) + write_file(second, 'bb') + p.require_change() + write_file(first, 'bb') + p.require_change() + + +def test_poller_stops_on_close(tmp_path, make_poller): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + p = make_poller([path]) + p.poller.close() + time.sleep(POLL_INTERVAL * 3) + write_file(path, 'bb') + p.require_no_change() + + +def test_poller_close_returns_while_callback_blocks(tmp_path): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + entered = threading.Event() + release = threading.Event() + + def on_change(): + entered.set() + release.wait(TEST_TIMEOUT) + + poller = Poller([path], POLL_INTERVAL, on_change) + poller.start() + write_file(path, 'bb') + assert entered.wait(TEST_TIMEOUT) + closed = threading.Event() + + def do_close(): + poller.close() + closed.set() + + threading.Thread(target=do_close, daemon=True).start() + assert closed.wait(TEST_TIMEOUT), "close blocked on the callback" + release.set() + + +# --------------------------------------------------------------------------- +# Watcher +# --------------------------------------------------------------------------- + +watchdog_required = pytest.mark.skipif(not have_watchdog, reason="watchdog is not installed") + + +class WatcherFixture: + def __init__(self, paths: List[str]): + self.changes: Queue = Queue() + self.watcher = Watcher(paths, lambda: self.changes.put(True)) + + def require_change(self) -> None: + take(self.changes) + + def require_no_change(self, duration: float = QUIET_PERIOD) -> None: + require_quiet(self.changes, duration) + + def drain(self) -> None: + while True: + try: + self.changes.get(timeout=0.2) + except Empty: + return + + +@pytest.fixture +def make_watcher(): + watchers: List[WatcherFixture] = [] + + def factory(paths: List[str]) -> WatcherFixture: + fixture = WatcherFixture(paths) + watchers.append(fixture) + return fixture + + yield factory + for fixture in watchers: + fixture.watcher.close() + + +@watchdog_required +def test_watcher_detects_modification(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + w = make_watcher([path]) + w.require_no_change(0.15) + write_file(path, 'bb') + w.require_change() + + +@watchdog_required +def test_watcher_ignores_other_files_in_the_directory(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + other = os.path.join(str(tmp_path), 'other.json') + write_file(path, 'a') + w = make_watcher([path]) + write_file(other, 'bb') + w.require_no_change() + + +@watchdog_required +def test_watcher_detects_absent_file_appearing(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + w = make_watcher([path]) + w.require_no_change(0.15) + write_file(path, 'a') + w.require_change() + + +@watchdog_required +def test_watcher_detects_file_written_by_rename(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + temp = os.path.join(str(tmp_path), 'data.json.tmp') + write_file(path, 'a') + w = make_watcher([path]) + write_file(temp, 'bb') + w.drain() + os.replace(temp, path) + w.require_change() + + +@watchdog_required +def test_watcher_matches_the_destination_of_a_move_event(tmp_path, make_watcher): + # A file written by rename arrives as a move event whose destination is the watched path. + import watchdog.events + + path = os.path.join(str(tmp_path), 'data.json') + temp = os.path.join(str(tmp_path), 'data.json.tmp') + real_path = os.path.join(os.path.realpath(str(tmp_path)), 'data.json') + real_temp = os.path.join(os.path.realpath(str(tmp_path)), 'data.json.tmp') + w = make_watcher([path]) + w.watcher._handle_event(watchdog.events.FileMovedEvent(real_temp, real_path)) + w.require_change() + w.watcher._handle_event(watchdog.events.FileMovedEvent(real_path, real_temp)) + w.require_change() + w.watcher._handle_event(watchdog.events.FileMovedEvent(real_temp, real_temp + '.other')) + w.require_no_change() + assert temp not in w.watcher._watched_paths + + +@watchdog_required +def test_watcher_ignores_events_that_do_not_change_the_file(tmp_path, make_watcher): + # Opening and reading a watched file produces notifications too. A reload reads the files, + # so reacting to those would make every reload trigger the next one. + import watchdog.events + + path = os.path.join(str(tmp_path), 'data.json') + real_path = os.path.join(os.path.realpath(str(tmp_path)), 'data.json') + write_file(path, 'a') + w = make_watcher([path]) + w.watcher._handle_event(watchdog.events.FileOpenedEvent(real_path)) + w.watcher._handle_event(watchdog.events.FileClosedNoWriteEvent(real_path)) + w.require_no_change() + w.watcher._handle_event(watchdog.events.FileClosedEvent(real_path)) + w.require_change() + + +@watchdog_required +def test_watcher_does_not_signal_when_the_file_is_only_read(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + w = make_watcher([path]) + for _ in range(3): + with open(path, 'rb') as f: + f.read() + w.require_no_change() + + +@watchdog_required +def test_watcher_detects_file_deletion(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + w = make_watcher([path]) + os.remove(path) + w.require_change() + + +@watchdog_required +def test_watcher_picks_up_directory_that_appears_later(tmp_path, make_watcher): + directory = os.path.join(str(tmp_path), 'later') + path = os.path.join(directory, 'data.json') + w = make_watcher([path]) + w.require_no_change(0.15) + os.mkdir(directory) + write_file(path, 'a') + # The retry that watches the new directory signals a change so the file is read. + w.require_change() + w.drain() + write_file(path, 'bb') + w.require_change() + + +@watchdog_required +def test_watcher_close_stops_notifications(tmp_path, make_watcher): + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + w = make_watcher([path]) + w.watcher.close() + write_file(path, 'bb') + w.require_no_change() From 27bde6f8bf6e93090d56727d57d161c503a9d98e Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Thu, 1 Oct 2026 23:09:04 +0000 Subject: [PATCH 02/11] fix: Expand value-only overrides to a flag served by fallthrough --- ldclient/impl/integrations/files/filedata.py | 5 +++-- ldclient/testing/integrations/test_filedata.py | 12 +++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index a6536164..9f7cb19e 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -141,9 +141,10 @@ def abs_file_paths(paths: List[str]) -> List[str]: def make_flag_with_value(key: str, value: Any) -> FeatureFlag: """ Expands a flag-key-to-value entry into a full flag definition that returns the given value - for every context. The flag is off and serves its single variation as the off variation. + for every context. The flag is on, has the value as its only variation, and serves that + variation as its fallthrough. """ - return FeatureFlag({"key": key, "version": 1, "on": False, "offVariation": 0, "variations": [value]}) + return FeatureFlag({"key": key, "version": 1, "on": True, "fallthrough": {"variation": 0}, "variations": [value]}) def read_file(path: str) -> Document: diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 4cf20945..4ccc967e 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -160,14 +160,15 @@ def test_abs_file_paths(): assert paths[1] == '/absolute/data.json' -def test_make_flag_with_value_is_off_and_serves_the_value(): +def test_make_flag_with_value_is_on_and_serves_the_value_by_fallthrough(): flag = make_flag_with_value('flag1', 'value1') assert flag.key == 'flag1' assert flag.version == 1 - assert flag.on is False - assert flag.off_variation == 0 + assert flag.on is True + assert flag.off_variation is None + assert flag.fallthrough.variation == 0 assert flag.variations == ['value1'] - assert flag.to_json_dict() == {'key': 'flag1', 'version': 1, 'on': False, 'offVariation': 0, 'variations': ['value1']} + assert flag.to_json_dict() == {'key': 'flag1', 'version': 1, 'on': True, 'fallthrough': {'variation': 0}, 'variations': ['value1']} # --------------------------------------------------------------------------- @@ -186,7 +187,8 @@ def test_merge_combines_documents(): assert list(result.flags.keys()) == ['flag1', 'flag2'] assert list(result.segments.keys()) == ['seg1'] assert result.flags['flag2'].variations == ['value2'] - assert result.flags['flag2'].on is False + assert result.flags['flag2'].on is True + assert result.flags['flag2'].fallthrough.variation == 0 def test_merge_duplicate_keys_fail(): From f748973b70e5e1bc10743ffd2dd8740f0dd3c8a8 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:32:24 +0000 Subject: [PATCH 03/11] fix: Report a rejected file data result as a failure and recover a lost watch The reloader treats an exception from apply like a load failure: it is reported, retried, and the result is not remembered as the last good one. An exception from the error callback is logged and does not stop the retry. The watcher retries when the observer cannot start, watches a directory again after it is deleted and recreated, and closes without error when the observer never started. --- ldclient/impl/integrations/files/filedata.py | 203 +++++++++++++----- .../testing/integrations/test_filedata.py | 139 ++++++++++-- 2 files changed, 275 insertions(+), 67 deletions(-) diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index 9f7cb19e..a4f71d6a 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -62,6 +62,7 @@ class DuplicateKeysHandling(str, Enum): """ Determines what happens when the same flag or segment key appears in more than one file. + Flag overrides are currently experimental and subject to change. """ FAIL = "fail" @@ -324,9 +325,12 @@ def __init__( :param paths: the files to load, in order. The order determines which file wins under the duplicate keys handling. :param duplicate_keys_handling: what to do when the same key appears in more than one file - :param apply: receives each successfully merged result. Calls are serialized. + :param apply: receives each successfully merged result. Calls are serialized. An + exception from it is reported like a load failure and retried, and the result it + rejected is not remembered as the last good one. :param on_error: receives each distinct failure. Repeats of an identical failure do not - call it again until a success re-arms it. Failures are also logged here. + call it again until a success re-arms it. Failures are also logged here. An exception + from it is logged and does not prevent the retry. :param skip_missing_paths: when true, a configured file that does not exist contributes no entries. When false, a missing file fails the load like any other read error. :param debounce_delay: how long to wait after a trigger for further triggers to settle @@ -465,11 +469,18 @@ def _reload(self) -> bool: # the last success. The consumer heard about the failure and only an application # tells it that things are good again. recovering = self._last_error_message is not None - self._last_error_message = None if self._skip_unchanged and not recovering and digest == self._last_good_digest: return True + + # Nothing is remembered until the consumer has accepted the result. A result the + # consumer rejects must not become the baseline that skip-unchanged compares + # against, and must not count as a recovery. + try: + self._apply(merged) + except Exception as e: + return self._fail(e) + self._last_error_message = None self._last_good_digest = digest - self._apply(merged) return True def _fail(self, err: Exception) -> bool: @@ -485,7 +496,11 @@ def _fail(self, err: Exception) -> bool: self._last_error_message = message log.error("Unable to load flag data: %s", err) if self._on_error is not None: - self._on_error(err) + try: + self._on_error(err) + except Exception as e: + # A consumer that raises while it reports a failure must not stop the retry. + log.error("Error while reporting a flag data load failure: %s", e) return False @@ -545,12 +560,26 @@ def _poll(self) -> None: self._on_change() +def _event_path(value: Any) -> Optional[str]: + """The path carried by a watchdog event as text, or None when the event has none.""" + if isinstance(value, bytes): + return value.decode("utf-8", errors="replace") + if isinstance(value, str): + return value + return None + + class Watcher: """ Detects changes to a set of files through file system change notifications, using the ``watchdog`` package. The directory of each file is watched, so a file that does not exist - yet is picked up when it appears. A directory that cannot be watched yet, for example - because it does not exist, is retried on an interval. + yet is picked up when it appears. + + A directory that cannot be watched, for example because it does not exist yet or because + the notification mechanism cannot be started, is attempted again on an interval. A watched + directory that is deleted has its watch dropped and set up again once the directory exists. + When a retry sets up a watch, the callback runs once, so that changes made while the watch + was not in place are picked up. Notifications for the watched paths invoke the callback. The callback can run several times for one logical edit, so feed it into a :class:`Reloader`. @@ -561,17 +590,12 @@ def __init__(self, paths: List[str], on_change: Callable[[], None]): raise RuntimeError("the watchdog package is required to watch files for changes") self._on_change = on_change self._watched_paths: Set[str] = set() - self._lock = threading.Lock() - self._pending_directories: Set[str] = set() - self._retry_task: Optional[RepeatingTask] = None - - directories: List[str] = [] + self._directories: Set[str] = set() for path in paths: absolute = os.path.abspath(path) real_directory = os.path.realpath(os.path.dirname(absolute)) self._watched_paths.add(os.path.join(real_directory, os.path.basename(absolute))) - if real_directory not in directories: - directories.append(real_directory) + self._directories.add(real_directory) watcher = self @@ -581,58 +605,141 @@ def on_any_event(self, event): self._handler = _Handler() self._observer = watchdog.observers.Observer() - for directory in directories: - if not self._schedule(directory): - self._pending_directories.add(directory) - try: - self._observer.start() - except Exception as e: - log.error("Unable to start watching files for changes: %s", e) - if len(self._pending_directories) > 0: - self._retry_task = RepeatingTask.at_interval("ldclient.filedata.watch-retry", _WATCH_RETRY_INTERVAL, _WATCH_RETRY_INTERVAL, self._retry_pending) - self._retry_task.start() - def _schedule(self, directory: str) -> bool: + # Guards the state below. It is never held while the observer is called, because the + # observer holds its own lock while it dispatches an event to the handler. + self._lock = threading.Lock() + self._closed = False + self._observer_running = False + # The watch on each directory that currently has one. + self._watches: Dict[str, Any] = {} + # The directories that have no watch yet. The retry schedule runs while this is not empty. + self._pending_directories: Set[str] = set(self._directories) + self._retry_task: Optional[RepeatingTask] = None + # Serializes the watch setup against close, so that a setup in progress cannot start + # anything after close has stopped the observer. + self._setup_lock = threading.Lock() + + self._set_up_watches(is_retry=False) + + def _set_up_watches(self, is_retry: bool) -> None: + """ + Starts the observer when it is not running, then sets up a watch on each pending + directory. Whatever cannot be set up stays pending and is attempted again on the retry + schedule. When a retry sets up a watch, the callback runs once, because the files may + have changed while the watch was not in place. + """ + with self._setup_lock: + with self._lock: + if self._closed: + return + running = self._observer_running + if not running: + try: + self._observer.start() + except Exception as e: + log.error("Unable to start watching files for changes: %s", e) + self._ensure_retry_scheduled() + return + with self._lock: + self._observer_running = True + + with self._lock: + pending = sorted(self._pending_directories) + armed = False + for directory in pending: + watch = self._schedule(directory) + if watch is None: + continue + with self._lock: + self._pending_directories.discard(directory) + self._watches[directory] = watch + armed = True + + with self._lock: + if len(self._pending_directories) > 0: + self._ensure_retry_scheduled_locked() + if is_retry and armed: + self._on_change() + + def _schedule(self, directory: str) -> Optional[Any]: + """Sets up a watch on a directory. Returns the watch, or None when the directory cannot be watched yet.""" # The observer accepts a watch on a directory that does not exist and fails later when # it starts the watch, so the check happens here first. if not os.path.isdir(directory): log.warning('Cannot watch directory "%s" for changes yet because it does not exist', directory) - return False + return None try: - self._observer.schedule(self._handler, directory, recursive=False) - return True + return self._observer.schedule(self._handler, directory, recursive=False) except Exception as e: log.warning('Cannot watch directory "%s" for changes yet: %s', directory, e) - return False + return None - def _retry_pending(self) -> None: + def _ensure_retry_scheduled(self) -> None: with self._lock: - pending = list(self._pending_directories) - for directory in pending: - if self._schedule(directory): - with self._lock: - self._pending_directories.discard(directory) - # Files may have appeared in the directory before the watch was in place. - self._on_change() + self._ensure_retry_scheduled_locked() + + def _ensure_retry_scheduled_locked(self) -> None: + if self._closed or self._retry_task is not None: + return + self._retry_task = RepeatingTask.at_interval("ldclient.filedata.watch-retry", _WATCH_RETRY_INTERVAL, _WATCH_RETRY_INTERVAL, self._retry_pending) + self._retry_task.start() + + def _retry_pending(self) -> None: + self._set_up_watches(is_retry=True) with self._lock: - if len(self._pending_directories) == 0 and self._retry_task is not None: + # The schedule ends once everything is watched. It is set up again when a directory + # becomes pending later. The check and the stop happen under the lock, so a + # directory that becomes pending at the same time either keeps this schedule or + # starts a new one. + if len(self._pending_directories) == 0 and self._observer_running and self._retry_task is not None: self._retry_task.stop() + self._retry_task = None + + def _directory_lost(self, directory: str) -> None: + """ + Drops the watch on a directory that no longer exists and arms the retry that sets it up + again once the directory exists. + """ + with self._lock: + if self._closed: + return + stale = self._watches.pop(directory, None) + self._pending_directories.add(directory) + self._ensure_retry_scheduled_locked() + log.warning('Directory "%s" no longer exists. It is watched again when it exists.', directory) + if stale is not None: + # The observer keeps a watch whose directory is gone, and a later watch on the same + # path would reuse it, so it is removed. + try: + self._observer.unschedule(stale) + except Exception as e: + log.debug('Unable to remove the watch on directory "%s": %s', directory, e) def _handle_event(self, event) -> None: - if getattr(event, "event_type", None) not in _CHANGE_EVENT_TYPES: + event_type = getattr(event, "event_type", None) + if event_type not in _CHANGE_EVENT_TYPES: return - candidates = [getattr(event, "src_path", None), getattr(event, "dest_path", None)] - for candidate in candidates: - if isinstance(candidate, bytes): - candidate = candidate.decode("utf-8", errors="replace") + src_path = _event_path(getattr(event, "src_path", None)) + if event_type in ("deleted", "moved") and getattr(event, "is_directory", False) and src_path in self._directories: + self._directory_lost(src_path) + return + for candidate in (src_path, _event_path(getattr(event, "dest_path", None))): if candidate in self._watched_paths: self._on_change() return def close(self) -> None: """Stops watching and waits briefly for the observer thread to finish.""" - with self._lock: - if self._retry_task is not None: - self._retry_task.stop() - self._observer.stop() - self._observer.join(timeout=5) + with self._setup_lock: + with self._lock: + self._closed = True + retry_task = self._retry_task + self._retry_task = None + running = self._observer_running + if retry_task is not None: + retry_task.stop() + self._observer.stop() + # A thread that never started cannot be joined. + if running: + self._observer.join(timeout=5) diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 4ccc967e..34e8aceb 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -1,8 +1,10 @@ +import logging import os import threading import time from queue import Empty, Queue -from typing import Any, Dict, List +from typing import Any, Dict, List, Set +from unittest import mock import pytest @@ -11,6 +13,7 @@ except ImportError: # pragma: no cover yaml = None # type: ignore +from ldclient.impl.integrations.files import filedata from ldclient.impl.integrations.files.filedata import ( Document, DuplicateKeyError, @@ -157,7 +160,7 @@ def test_read_file_json_and_errors(tmp_path): def test_abs_file_paths(): paths = abs_file_paths(['relative/data.json', '/absolute/data.json']) assert paths[0] == os.path.abspath('relative/data.json') - assert paths[1] == '/absolute/data.json' + assert paths[1] == os.path.abspath('/absolute/data.json') def test_make_flag_with_value_is_on_and_serves_the_value_by_fallthrough(): @@ -440,12 +443,16 @@ def test_reloader_reports_identical_failure_only_once(make_fixture): f.require_quiet() +def reloader_threads() -> Set[threading.Thread]: + return {t for t in threading.enumerate() if t.name == 'ldclient.filedata.reloader'} + + def test_reloader_unused_spawns_no_thread(make_fixture): - before = threading.active_count() + before = reloader_threads() f = make_fixture('{"flagValues": {"flag1": true}}') - assert threading.active_count() == before + assert reloader_threads() == before f.reloader.reload_now() - assert threading.active_count() == before + 1 + assert len(reloader_threads() - before) == 1 f.require_applied() @@ -475,14 +482,16 @@ def do_close(): def test_reloader_worker_thread_exits_after_close(make_fixture): + before = reloader_threads() f = make_fixture('{"flagValues": {"flag1": true}}') f.reloader.reload_now() f.require_applied() - worker = [t for t in threading.enumerate() if t.name == 'ldclient.filedata.reloader'] - assert len(worker) == 1 + started = reloader_threads() - before + assert len(started) == 1 f.reloader.close() - worker[0].join(TEST_TIMEOUT) - assert not worker[0].is_alive() + worker = started.pop() + worker.join(TEST_TIMEOUT) + assert not worker.is_alive() def test_reloader_retries_after_failure_without_further_triggers(make_fixture): @@ -597,12 +606,13 @@ def test_reloader_serializes_reload_now_against_worker_reloads(make_fixture): # complete merged result, and the callback never runs on two threads at once. in_apply = threading.Lock() overlaps: List[bool] = [] + applied_keys: List[List[str]] = [] def apply(result: MergeResult) -> None: acquired = in_apply.acquire(blocking=False) overlaps.append(not acquired) try: - assert list(result.flags.keys()) == ['flag1'] + applied_keys.append(list(result.flags.keys())) time.sleep(0.002) finally: if acquired: @@ -618,20 +628,51 @@ def apply(result: MergeResult) -> None: t.join(TEST_TIMEOUT) f.reloader.close() assert not any(overlaps) + assert len(applied_keys) > 0 + assert all(keys == ['flag1'] for keys in applied_keys) -def test_reloader_survives_an_apply_callback_that_raises(make_fixture): +def test_reloader_reports_an_apply_failure_and_retries_it(make_fixture): + # The consumer rejects every result until it is released. Each call records whether the + # result was accepted. + accept = threading.Event() calls: Queue = Queue() def apply(result: MergeResult) -> None: - calls.put(result) - raise RuntimeError("consumer failure") + accepted = accept.is_set() + calls.put(accepted) + if not accepted: + raise RuntimeError("consumer failure") - f = make_fixture('{"flagValues": {"flag1": true}}', apply=apply) - f.reloader.trigger() - take(calls) - f.reloader.trigger() - take(calls) + f = make_fixture('{"flagValues": {"flag1": true}}', apply=apply, retry_delay=SHORT_DELAY, skip_unchanged=True) + + # The synchronous load returns normally. The rejection is reported like a load failure. + f.reloader.reload_now() + assert take(calls) is False + assert isinstance(f.require_errored(), RuntimeError) + + # The automatic retry offers the unchanged content again until the consumer accepts it. + accept.set() + while take(calls) is False: + pass + + # Once the consumer has accepted the result, the retries stop. + with pytest.raises(Empty): + calls.get(timeout=QUIET_PERIOD) + + +def test_reloader_retries_when_the_error_callback_raises(make_fixture): + def on_error(err: Exception) -> None: + raise RuntimeError("reporter failure") + + f = make_fixture('{"flagValues"', on_error=on_error, retry_delay=SHORT_DELAY) + + # The failing report does not escape the synchronous load. + f.reloader.reload_now() + + # The automatic retry still runs and picks up the corrected file. + f.write('{"flagValues": {"flag1": true}}') + f.require_applied() # --------------------------------------------------------------------------- @@ -884,7 +925,10 @@ def test_watcher_ignores_events_that_do_not_change_the_file(tmp_path, make_watch write_file(path, 'a') w = make_watcher([path]) w.watcher._handle_event(watchdog.events.FileOpenedEvent(real_path)) - w.watcher._handle_event(watchdog.events.FileClosedNoWriteEvent(real_path)) + # Older watchdog versions report no event for a read-only close. + closed_no_write = getattr(watchdog.events, 'FileClosedNoWriteEvent', None) + if closed_no_write is not None: + w.watcher._handle_event(closed_no_write(real_path)) w.require_no_change() w.watcher._handle_event(watchdog.events.FileClosedEvent(real_path)) w.require_change() @@ -925,6 +969,63 @@ def test_watcher_picks_up_directory_that_appears_later(tmp_path, make_watcher): w.require_change() +@watchdog_required +def test_watcher_watches_a_directory_again_after_it_is_deleted_and_recreated(tmp_path, make_watcher, monkeypatch): + monkeypatch.setattr(filedata, '_WATCH_RETRY_INTERVAL', SHORT_DELAY) + directory = os.path.join(str(tmp_path), 'later') + path = os.path.join(directory, 'data.json') + os.mkdir(directory) + write_file(path, 'a') + w = make_watcher([path]) + + # The file and then its directory are removed. + os.remove(path) + os.rmdir(directory) + w.drain() + + # The directory comes back with the file. The retry that watches it again signals a change. + os.mkdir(directory) + write_file(path, 'bb') + w.require_change() + w.drain() + + # Notifications from the recreated directory are delivered. + write_file(path, 'ccc') + w.require_change() + + +@watchdog_required +def test_watcher_retries_when_the_observer_cannot_start(tmp_path, make_watcher, monkeypatch, caplog): + import watchdog.observers + + monkeypatch.setattr(filedata, '_WATCH_RETRY_INTERVAL', SHORT_DELAY) + path = os.path.join(str(tmp_path), 'data.json') + write_file(path, 'a') + + # The observer cannot start while the watcher is constructed. + with caplog.at_level(logging.ERROR): + with mock.patch.object(watchdog.observers.Observer, 'start', side_effect=RuntimeError("cannot start thread")): + w = make_watcher([path]) + assert len([r for r in caplog.records if r.levelno == logging.ERROR]) > 0 + + # Once the observer can start, the retry sets up the watch and signals a change, and later + # edits are detected. + w.require_change() + w.drain() + write_file(path, 'bb') + w.require_change() + + +@watchdog_required +def test_watcher_close_succeeds_when_the_observer_never_started(tmp_path): + import watchdog.observers + + path = os.path.join(str(tmp_path), 'data.json') + with mock.patch.object(watchdog.observers.Observer, 'start', side_effect=RuntimeError("cannot start thread")): + watcher = Watcher([path], lambda: None) + watcher.close() + + @watchdog_required def test_watcher_close_stops_notifications(tmp_path, make_watcher): path = os.path.join(str(tmp_path), 'data.json') From b309a878d05e81a015ca97c915c7a08e4b0d2a4a Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:32:24 +0000 Subject: [PATCH 04/11] test: Build the read-only close event only when the watchdog version has it --- .../test_file_data_sources_pinned_behavior.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py b/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py index 3950afa5..0037261a 100644 --- a/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py +++ b/ldclient/testing/integrations/test_file_data_sources_pinned_behavior.py @@ -406,12 +406,16 @@ def test_watcher_reloads_on_any_notification_for_the_file_path(tmp_path, make_wa handlers = handlers_of(watcher._observer) assert len(handlers) == 1 handler = handlers[0] - handler.on_any_event(watchdog.events.FileModifiedEvent(path)) - handler.on_any_event(watchdog.events.FileOpenedEvent(path)) - handler.on_any_event(watchdog.events.FileClosedNoWriteEvent(path)) - assert counter.count == 3 + events = [watchdog.events.FileModifiedEvent(path), watchdog.events.FileOpenedEvent(path)] + # Older watchdog versions report no event for a read-only close. + closed_no_write = getattr(watchdog.events, 'FileClosedNoWriteEvent', None) + if closed_no_write is not None: + events.append(closed_no_write(path)) + for event in events: + handler.on_any_event(event) + assert counter.count == len(events) handler.on_any_event(watchdog.events.FileModifiedEvent(os.path.join(os.path.dirname(path), 'other.json'))) - assert counter.count == 3 + assert counter.count == len(events) @watchdog_required From 7f80543b6560dd3948b24ebc1fed08a0a742aff9 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:02:36 +0000 Subject: [PATCH 05/11] test: Assert that the file data poller goes quiet once a file is stable A plain rewrite is not atomic: the truncation and the write are separate steps, and a poll that lands between them legitimately reports an extra change. The poller promises generous detection, not one callback per logical change; the reloader coalesces and skips the excess. The test that expected exactly one callback could therefore fail on a busy machine. It now drains the callbacks of the write and then requires silence, which still fails for a poller that does not update its baseline. --- .../testing/integrations/test_filedata.py | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 34e8aceb..f2d5d76d 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -694,6 +694,24 @@ def require_change(self) -> None: def require_no_change(self, duration: float = QUIET_PERIOD) -> None: require_quiet(self.changes, duration) + def drain(self, settle: float) -> None: + """Discards the change callbacks that arrive within the settle window. + + A plain rewrite is not atomic: the truncation, the write, and the modification-time + update are separate steps, and a poll that lands between two of them legitimately + reports an extra change. A test that wants to assert silence waits for the write to + be fully observed first. + """ + deadline = time.monotonic() + settle + while True: + remaining = deadline - time.monotonic() + if remaining <= 0: + return + try: + self.changes.get(timeout=remaining) + except Empty: + return + @pytest.fixture def make_poller(): @@ -742,12 +760,16 @@ def test_poller_detects_size_change_with_same_mtime(tmp_path, make_poller): p.require_change() -def test_poller_fires_once_per_change(tmp_path, make_poller): +def test_poller_stops_firing_when_file_is_stable(tmp_path, make_poller): + # The poller updates its baseline after a change, so it goes quiet once the file is stable. + # It does not promise one callback per logical change: a rewrite that it observes in more + # than one step is reported in more than one step, and the reloader absorbs the excess. path = os.path.join(str(tmp_path), 'data.json') write_file(path, 'a') p = make_poller([path]) write_file(path, 'bb') p.require_change() + p.drain(POLL_INTERVAL * 10) p.require_no_change() From fd206c00f7df5cb2b84e59bdfe6fb95a9c9ddd85 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:10:01 +0000 Subject: [PATCH 06/11] test: Load the changed file on demand when checking that a different failure is reported The reloader test wrote a file with a new failure while the automatic retries were running and expected the next retry to report it. The write is ordinary, not atomic, so a retry can read the file while it is empty or half written. An empty document is valid, so such a retry succeeds and ends the retries, and the new failure is never read; a half-written file is reported as a failure of its own. The test now loads the final content on demand and inspects the reports as a whole, and a fixture helper collects the callbacks that arrive within a settle period. --- .../testing/integrations/test_filedata.py | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index f2d5d76d..7f9875d5 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -3,7 +3,7 @@ import threading import time from queue import Empty, Queue -from typing import Any, Dict, List, Set +from typing import Any, Dict, List, Set, Tuple from unittest import mock import pytest @@ -319,6 +319,19 @@ def require_quiet(self, duration: float = QUIET_PERIOD) -> None: pytest.fail("unexpected on_error call") time.sleep(0.01) + def drain(self, duration: float = QUIET_PERIOD) -> Tuple[List[MergeResult], List[Exception]]: + """Collects every apply and on_error call that arrives within the duration.""" + applied: List[MergeResult] = [] + errored: List[Exception] = [] + deadline = time.time() + duration + while time.time() < deadline: + while not self.applied.empty(): + applied.append(self.applied.get_nowait()) + while not self.errored.empty(): + errored.append(self.errored.get_nowait()) + time.sleep(0.01) + return applied, errored + def close(self) -> None: self.reloader.close() @@ -437,9 +450,16 @@ def test_reloader_reports_identical_failure_only_once(make_fixture): # The automatic retries keep failing in the same way. They do not report again. f.require_quiet() - # A different failure is reported. + # A different failure is reported. The write is ordinary, not atomic, and a retry that runs + # while the file is being rewritten reads it empty or half written. An empty document is + # valid, so such a retry succeeds and ends the retries; a half-written file is a failure of + # its own. The final content is therefore loaded on demand, and everything that was reported + # is inspected as a whole: the new failure is among the reports, and once it is the current + # failure, the retries that repeat it stay silent. f.write('{"flags": {"flag1": "not an object"}}') - f.require_errored() + f.reloader.reload_now() + _, errored = f.drain() + assert any('must be an object' in str(e) for e in errored), errored f.require_quiet() From 30a6e7077d020874d61a93907aa9f675e19ecce0 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Tue, 3 Nov 2026 23:46:07 +0000 Subject: [PATCH 07/11] fix: Keep file data through in-place rewrites and choose the format by extension Applications and editors rewrite data files in place: they truncate the file and then write the new content. With pyyaml installed, a load in between parsed the empty file as a document with no entries, whatever its name. The parser is now chosen by the file name: a name ending in .yaml or .yml is YAML, and any other file is JSON. Files must be UTF-8. An empty JSON file, and JSON that is not an object such as null, fail to parse; YAML that is empty, only comments, or null has no entries. The Reloader holds back a file whose content is in effect and that a load finds with no entries, until it has stayed so, unwritten, for the retry delay. The delay is measured from the file's own read and starts over when the file's modification time or size changes. Each file is held for at most five retry delays from its first hold, and its bound starts over only at a load that holds nothing back. A load reads and classifies every file, even after a failure, and fails with the first failure; content, even content that cannot be parsed, or a missing file ends the empty state. The skip-unchanged digest covers each path, whether its file exists, and its bytes. Tests rewrite files in place through ldclient/testing/file_util.py and time the hold on an injected clock. --- ldclient/impl/integrations/files/filedata.py | 532 ++++-- ldclient/testing/file_util.py | 26 + .../testing/integrations/test_filedata.py | 1514 ++++++++++++++++- 3 files changed, 1936 insertions(+), 136 deletions(-) create mode 100644 ldclient/testing/file_util.py diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index a4f71d6a..f8caf95d 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -6,9 +6,11 @@ A data file is a JSON or YAML document with optional ``flags``, ``flagValues``, and ``segments`` members. ``flags`` and ``segments`` hold full definitions keyed by key. ``flagValues`` maps a flag key to a single value. It expands into a full flag definition -that returns that value for every context. +that returns that value for every context. The file name extension decides the format: a +file whose name ends in ``.yaml`` or ``.yml`` is YAML, and any other file is JSON. """ +import codecs import hashlib import json import os @@ -54,6 +56,9 @@ # because it does not exist yet. _WATCH_RETRY_INTERVAL = 1.0 +# The file name extensions of YAML files, compared without regard to case. Any other file is JSON. +YAML_EXTENSIONS = frozenset([".yaml", ".yml"]) + # The watchdog event types that can change a file's content or presence. Opening or reading a # file also produces events, and a reload reads the files, so those must not count as changes. _CHANGE_EVENT_TYPES = frozenset(["created", "modified", "moved", "deleted", "closed"]) @@ -148,33 +153,69 @@ def make_flag_with_value(key: str, value: Any) -> FeatureFlag: return FeatureFlag({"key": key, "version": 1, "on": True, "fallthrough": {"variation": 0}, "variations": [value]}) +def is_yaml_path(path: str) -> bool: + """ + Whether a data file is a YAML document. The end of the file name decides: a name that ends in + ``.yaml`` or ``.yml``, in any case, is YAML, including a name that is only that, such as + ``.yaml``. Any other file, including one with no extension, is JSON. + """ + return os.path.basename(path).lower().endswith(tuple(YAML_EXTENSIONS)) + + def read_file(path: str) -> Document: - """Reads and parses a single data file, which may be in JSON or YAML format.""" + """Reads and parses a single data file, in the format its file name extension selects.""" try: with open(path, "rb") as f: raw = f.read() except OSError as e: raise FileReadError(path, "unable to read file: %s" % e) try: - return parse_document(raw) + return parse_document(raw, is_yaml_path(path)) except Exception as e: raise FileReadError(path, "error parsing file: %s" % e) -def parse_document(raw: bytes) -> Document: +def has_content(raw: bytes) -> bool: + """Whether the raw content of a file holds anything other than white space and a byte order mark.""" + return raw.removeprefix(codecs.BOM_UTF8).strip() != b"" + + +def parse_document(raw: bytes, is_yaml: bool = False) -> Document: """ - Parses the raw content of a data file, which may be JSON or YAML. The parser is chosen the - way the file data sources choose it: the YAML parser reads both formats when the ``pyyaml`` - package is installed, and the standard JSON parser is used when it is not. An empty - document is valid and holds no entries. + Parses the raw content of a data file as YAML when ``is_yaml`` is true, which requires the + ``pyyaml`` package, and as JSON otherwise. The file name extension decides the format; see + :func:`is_yaml_path`. A byte order mark at the start is ignored. + + Whether a file with no content is valid depends on its format, like the rest of its content. + A JSON file with no content, or only white space, is not a JSON document, so it fails to + parse. A YAML file with no content, or with only comments or a document marker, is a + document with no entries. """ - text = raw.decode("utf-8") - if have_yaml: - parsed = yaml.safe_load(text) # pyyaml correctly parses JSON too + document = _parse_content(raw, is_yaml) + return Document() if document is None else document + + +def _parse_content(raw: bytes, is_yaml: bool) -> Optional[Document]: + """ + The parse behind :func:`parse_document`. Returns None when the YAML parser reads the content + as nothing, as it does for a file with no content, only comments, or ``null``, rather than as + a document, even one with no members such as ``{}``. A JSON document must be an object. + """ + text = raw.decode("utf-8-sig") + if is_yaml: + if not have_yaml: + raise ValueError("the pyyaml package is required to read YAML") + parsed = yaml.safe_load(text) else: + if not has_content(raw): + # Not a JSON document. A file that is rewritten in place can be empty for a moment, + # and the failure keeps the last good data until the file is complete. + raise ValueError("the file has no content, which is not a JSON document") parsed = json.loads(text) + if not isinstance(parsed, dict): + raise ValueError("the document must be an object") if parsed is None: - return Document() + return None if not isinstance(parsed, dict): raise ValueError("the document must be an object") @@ -255,38 +296,79 @@ def load_files(paths: List[str], duplicate_keys_handling: DuplicateKeysHandling, and the handling is FAIL. When ``skip_missing_paths`` is true, a file that does not exist contributes no entries instead of failing the load. """ - merged, _ = _load_files_hashed(paths, duplicate_keys_handling, skip_missing_paths) + merged, _ = _parse_files(_read_files(paths, skip_missing_paths), duplicate_keys_handling) return merged -def _load_files_hashed(paths: List[str], duplicate_keys_handling: DuplicateKeysHandling, skip_missing_paths: bool) -> Tuple[MergeResult, bytes]: +@dataclass +class _FileRead: + """What one load read from one configured file.""" + + path: str + raw: Optional[bytes] + """The raw content, or None when the file does not exist and missing files are skipped.""" + + +def _read_files(paths: List[str], skip_missing_paths: bool) -> List[_FileRead]: """ - The load behind :func:`load_files`. It also returns a digest of the raw file contents, so a - caller can tell whether a later load read the same bytes. One read feeds both the digest - and the parse, so the digest can never disagree with the content that was parsed. + Reads the raw content of all of the given files in order. Raises :class:`FileReadError` + when a file cannot be read, including a missing file unless ``skip_missing_paths`` is true. + """ + return [_read_file(path, skip_missing_paths) for path in paths] + + +def _read_file(path: str, skip_missing_paths: bool) -> _FileRead: + """Reads the raw content of one file, as :func:`_read_files` does.""" + try: + with open(path, "rb") as f: + return _FileRead(path, f.read()) + except FileNotFoundError: + if skip_missing_paths: + log.debug("File %s does not exist; it contributes no data", path) + return _FileRead(path, None) + raise FileReadError(path, "unable to read file: the file does not exist") + except OSError as e: + raise FileReadError(path, "unable to read file: %s" % e) + + +def _parse_file(path: str, raw: bytes) -> Optional[Document]: + """Parses what was read from a file that exists. Returns None when the parser read nothing.""" + is_yaml = is_yaml_path(path) + try: + return _parse_content(raw, is_yaml) + except Exception as e: + raise FileReadError(path, "error parsing file as %s, the format of its name: %s" % ("YAML" if is_yaml else "JSON", e)) + + +def _parse_files(contents: List[_FileRead], duplicate_keys_handling: DuplicateKeysHandling) -> Tuple[MergeResult, bytes]: + """Parses and merges the content that :func:`_read_files` read.""" + return _merge_files([(read, None if read.raw is None else _parse_file(read.path, read.raw)) for read in contents], duplicate_keys_handling) + + +def _merge_files(parsed: List[Tuple[_FileRead, Optional[Document]]], duplicate_keys_handling: DuplicateKeysHandling) -> Tuple[MergeResult, bytes]: + """ + Merges the parsed files. It also returns a digest of the files that were read, so a caller + can tell whether a later load read the same. One read feeds both the digest and the parse, + so the digest can never disagree with the content that was parsed. The digest covers each + path, whether its file exists, and its bytes: the same bytes in a file with another name can + be in another format, and parse differently. """ documents: List[Document] = [] files: List[FileSummary] = [] hasher = hashlib.sha256() - for path in paths: - try: - with open(path, "rb") as f: - raw = f.read() - except FileNotFoundError: - if skip_missing_paths: - log.debug("File %s does not exist; it contributes no data", path) - files.append(FileSummary(path=path)) - continue - raise FileReadError(path, "unable to read file: the file does not exist") - except OSError as e: - raise FileReadError(path, "unable to read file: %s" % e) - hasher.update(raw) - hasher.update(b"\0") - try: - documents.append(parse_document(raw)) - except Exception as e: - raise FileReadError(path, "error parsing file: %s" % e) - files.append(FileSummary(path=path, present=True)) + for read, document in parsed: + path = os.fsencode(read.path) + hasher.update(len(path).to_bytes(8, "big")) + hasher.update(path) + if read.raw is None: + hasher.update(b"-") + files.append(FileSummary(path=read.path)) + continue + hasher.update(b"+") + hasher.update(len(read.raw).to_bytes(8, "big")) + hasher.update(read.raw) + documents.append(Document() if document is None else document) + files.append(FileSummary(path=read.path, present=True)) merged = merge(documents, duplicate_keys_handling) # The documents are the present files in order. Copy their counts onto the file summaries. @@ -300,12 +382,73 @@ def _load_files_hashed(paths: List[str], duplicate_keys_handling: DuplicateKeysH return merged, hasher.digest() +FileState = Optional[Tuple[int, int]] + + +def _observe(path: str) -> FileState: + """ + Observes the state of a file: its modification time and size, through any symbolic link, or + None when it does not exist or cannot be examined. + """ + try: + info = os.stat(path) + return (info.st_mtime_ns, info.st_size) + except OSError: + return None + + +def _observe_all(paths: List[str]) -> List[FileState]: + """Observes the state of each file, as :func:`_observe` does.""" + return [_observe(path) for path in paths] + + +# A file is held back for at most this many retry delays after a load first held it back, until +# a load holds nothing back. +_MAX_HOLD_RETRY_DELAYS = 5 + + +@dataclass +class _EmptyFile: + """A file whose content is in effect and that loads have found with no entries.""" + + since: Optional[float] = None + """ + Since when the file has had no entries without being written, on the reloader's clock, or + None when the last load read it with content, even content that failed to parse, or found + it missing. A read that fails keeps it, as nothing is known about the content then. + """ + state: FileState = None + """The file's state then: its modification time and size, through any symbolic link.""" + first_held: Optional[float] = None + """ + When a load first held the file back, or None. It bounds how long the file is held back. + It is kept while loads hold files back, so files emptied in turn stay bounded, and cleared by + any load that holds nothing back, including one that fails. + """ + + class Reloader: """ Owns the reload cycle for a set of data files. It serializes reloads, debounces change signals, retains the last good result on failure by not calling ``apply``, retries after failures, and skips applications that would change nothing. + A file can be rewritten in place: a writer truncates it and then writes the new content, and + a load in between finds it empty. When retries are enabled, a load that finds a file with no + entries, because it is empty or holds only what its parser reads as nothing (a YAML comment + or document marker), while that file's content is in effect, holds the file back: it keeps + the current data without reporting a failure, and the other files' changes wait too, since a + load applies all files or none. The file is held back until it has stayed so, without being + written, for the retry delay; a change of its modification time or size starts the delay + over. Each file is held back for at most five retry delays after a load first held it back, + until a load holds nothing back, so a file that keeps being rewritten is applied in the end + without cutting short another file's rewrite. A file whose content is not in effect, such as a + placeholder that has always been empty, is never held back. Every file is read and parsed, + even after another file fails, before any is held back, so files emptied together wait + together, and a file that cannot be read or parsed fails the load, with the first failure, + even while another is held back. The clock is read after each file's + own read, so a slow read of another file does not shorten a file's delay. + The worker thread starts on the first :meth:`reload_now` or :meth:`trigger` call, so a reloader that is constructed but never used does not leak a thread. """ @@ -320,6 +463,7 @@ def __init__( debounce_delay: float = 0.0, retry_delay: float = 0.0, skip_unchanged: bool = False, + clock: Callable[[], float] = time.monotonic, ): """ :param paths: the files to load, in order. The order determines which file wins under @@ -336,9 +480,13 @@ def __init__( :param debounce_delay: how long to wait after a trigger for further triggers to settle before reloading. Zero reloads on every trigger. :param retry_delay: how long to wait after a failed reload before retrying it - automatically. Zero disables the automatic retry. + automatically, and how long a file whose content is in effect must have no entries, + without being written, before that is applied. Zero disables the automatic retry, and + an empty file is then parsed as it is. :param skip_unchanged: when true, a load whose raw file contents are identical to the last applied contents does not call ``apply`` + :param clock: the monotonic clock, in seconds, that times how long files are held back. + Tests replace it; the timers always use ``time.monotonic``. """ self._paths = list(paths) self._duplicate_keys_handling = duplicate_keys_handling @@ -348,6 +496,7 @@ def __init__( self._debounce_delay = debounce_delay self._retry_delay = retry_delay self._skip_unchanged = skip_unchanged + self._clock = clock # Guards the deadlines, the closed flag, and the worker start. self._cond = threading.Condition() @@ -355,11 +504,27 @@ def __init__( self._started = False self._debounce_deadline: Optional[float] = None self._retry_deadline: Optional[float] = None + # Whether the pending retry was armed by a load that held files back. + self._retry_after_hold = False - # Serializes the load work between reload_now and the worker thread. + # Serializes the load work between reload_now and the worker thread. The fields below are + # only touched while holding it. self._reload_lock = threading.Lock() self._last_good_digest: Optional[bytes] = None self._last_error_message: Optional[str] = None + # Whether each configured file contributed content to the data in effect. A missing file, + # or one with no entries, contributed none. + self._applied_had_content: Dict[str, bool] = {} + # The files whose content is in effect and that loads have found with no entries. All are + # forgotten when data is applied or a load is skipped as unchanged, and a load that holds + # nothing back, even one that fails, starts every file's bound over. + self._empty_files: Dict[str, _EmptyFile] = {} + # Whether the last load held files back, which the log line of the retry it arms names. + self._held_back = False + # Whether the current load has held a file back, even if it then fails. + self._holding_this_load = False + # The delay of the last retry that was armed, for tests. + self._last_requested_retry_delay: Optional[float] = None def reload_now(self) -> None: """ @@ -367,11 +532,9 @@ def reload_now(self) -> None: arms the same automatic retry as a failed triggered reload. """ self._ensure_started() - if not self._reload() and self._retry_delay > 0: - with self._cond: - if not self._closed and self._retry_deadline is None: - self._retry_deadline = time.monotonic() + self._retry_delay - self._cond.notify() + retry_after, held = self._reload() + if retry_after is not None: + self._arm_retry(retry_after, keep_sooner=True, after_hold=held) def trigger(self) -> None: """ @@ -395,6 +558,25 @@ def close(self) -> None: self._closed = True self._cond.notify_all() + def _arm_retry(self, retry_after: float, keep_sooner: bool, after_hold: bool) -> None: + """ + Arms the automatic retry for the given time from now, unless retries are off. With + ``keep_sooner``, a retry that is already due sooner stays as it is. ``after_hold`` says + that the load held files back, which the retry's log line names. + """ + if self._retry_delay <= 0: + return + with self._cond: + if self._closed: + return + deadline = time.monotonic() + retry_after + if keep_sooner and self._retry_deadline is not None and self._retry_deadline <= deadline: + return + self._retry_deadline = deadline + self._retry_after_hold = after_hold + self._last_requested_retry_delay = retry_after + self._cond.notify() + def _ensure_started(self) -> None: with self._cond: if self._closed or self._started: @@ -406,6 +588,7 @@ def _ensure_started(self) -> None: def _run(self) -> None: while True: is_retry = False + after_hold = False with self._cond: while True: if self._closed: @@ -421,78 +604,245 @@ def _run(self) -> None: continue if self._debounce_deadline is not None and self._debounce_deadline <= now: # A triggered reload supersedes a pending retry. It either succeeds, or - # it fails and arms a fresh retry below. + # it fails or holds files back and arms a fresh retry below. self._debounce_deadline = None self._retry_deadline = None is_retry = False else: self._retry_deadline = None is_retry = True + after_hold = self._retry_after_hold break - if is_retry: + if is_retry and after_hold: + log.debug("Reading flag data again after a file was found empty") + elif is_retry: log.debug("Retrying flag data load after earlier failure") else: log.info("Reloading flag data after detecting a change") try: - ok = self._reload() + retry_after, held = self._reload() except Exception as e: log.exception("Unexpected error while reloading flag data: %s", e) - ok = True - if not ok and self._retry_delay > 0: - with self._cond: - if not self._closed: - self._retry_deadline = time.monotonic() + self._retry_delay + retry_after, held = None, False + if retry_after is not None: + self._arm_retry(retry_after, keep_sooner=False, after_hold=held) def _is_closed(self) -> bool: with self._cond: return self._closed - def _reload(self) -> bool: + def _reload(self) -> Tuple[Optional[float], bool]: """ - Performs one full load of all configured files. Returns whether the load succeeded, - which decides whether a retry is armed. A skipped no-op application counts as success. + Performs one full load of all configured files. Returns the time after which to load + again, or None when the load succeeded, including a skipped no-op application: the retry + delay after a failure, or the time a hold lasts. Also returns whether the load held files + back. """ with self._reload_lock: if self._is_closed(): - return True + return None, False + self._held_back = False + self._holding_this_load = False + reached: List[str] = [] + retry_after = self._load(reached) + if not self._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 or tried to read, including one that + # failed to be read or parsed: such a file is no longer held back, so its next + # rewrite is a new one. + for path in reached: + empty = self._empty_files.get(path) + if empty is not None: + empty.first_held = None + return retry_after, self._held_back + + def _load(self, reached: List[str]) -> Optional[float]: + """ + The body of :meth:`_reload`. It records in ``reached`` each file that it reads or tries to + read, which is every configured file, since a load reads all of them even after a failure. + """ + # Every file is read, parsed and classified, even after another file fails, before the + # load fails, holds files back or merges them. So every file that is found with no + # entries starts its delay in this load, and every file is reached, so a load that holds + # nothing back starts every file's bound over. The load fails with the first failure. + failure: Optional[Exception] = None + + def note_failure(e: Exception) -> None: + nonlocal failure + if failure is None: + failure = e + else: + # The load fails with the first failure. The others are logged, so that a second + # broken file is visible too. + log.debug("Unable to load flag data, besides the first failure: %s", e) + + held_back: List[str] = [] + parsed: List[Tuple[_FileRead, Optional[Document]]] = [] + had_content: Dict[str, bool] = {} + for path in self._paths: + reached.append(path) try: - merged, digest = _load_files_hashed(self._paths, self._duplicate_keys_handling, self._skip_missing_paths) + read = _read_file(path, self._skip_missing_paths) except Exception as e: - return self._fail(e) + # Nothing is known about the content, so the file's empty state, if any, stays. + note_failure(e) + continue + if read.raw is None: + # A missing file is not an empty one. + self._end_empty(read.path) + had_content[read.path] = False + parsed.append((read, None)) + continue + # A file with no content, or with content that its parser reads as nothing, such as + # YAML with only a comment, has no document and so no entries. A read can see that + # much of a file that is being written. Content with no document is invalid JSON, so + # it is only parsed when the file is not held back. {} is a document. + document: Optional[Document] = None + has_document = has_content(read.raw) + if has_document: + try: + document = _parse_file(read.path, read.raw) + except FileReadError as e: + # Content that cannot be parsed is not an empty file either. + self._end_empty(read.path) + note_failure(e) + continue + has_document = document is not None + if has_document: + self._end_empty(read.path) + # The clock is read after this file's own read, so that a slow read of another file + # does not shorten its delay. + elif self._should_hold_back(read.path, self._clock()): + held_back.append(read.path) + self._holding_this_load = True + continue + # The file was read and is not held back, even if it then fails to parse. + had_content[read.path] = has_document + if not has_content(read.raw): + try: + document = _parse_file(read.path, read.raw) + except FileReadError as e: + note_failure(e) + continue + parsed.append((read, document)) + if failure is not None: + return self._fail(failure) + if len(held_back) > 0: + return self._hold_back(held_back, self._clock()) - # A close may have happened while the files were being read. Deliver nothing then. - if self._is_closed(): - return True - - # A success right after a failure applies even when the content is unchanged since - # the last success. The consumer heard about the failure and only an application - # tells it that things are good again. - recovering = self._last_error_message is not None - if self._skip_unchanged and not recovering and digest == self._last_good_digest: - return True - - # Nothing is remembered until the consumer has accepted the result. A result the - # consumer rejects must not become the baseline that skip-unchanged compares - # against, and must not count as a recovery. - try: - self._apply(merged) - except Exception as e: - return self._fail(e) - self._last_error_message = None - self._last_good_digest = digest - return True + try: + merged, digest = _merge_files(parsed, self._duplicate_keys_handling) + except Exception as e: + return self._fail(e) + + # A close may have happened while the files were being read. Deliver nothing then. + if self._is_closed(): + return None + + # A success right after a failure applies even when the content is unchanged since the + # last success. The consumer heard about the failure and only an application tells it + # that things are good again. + recovering = self._last_error_message is not None + if self._skip_unchanged and not recovering and digest == self._last_good_digest: + # The data in effect is confirmed, as by an application, so it is remembered the same + # way, and the bounds start over. + self._remember_applied(had_content) + return None - def _fail(self, err: Exception) -> bool: + # Nothing is remembered until the consumer has accepted the result. A result the + # consumer rejects must not become the baseline that skip-unchanged compares against, + # and must not count as a recovery. + try: + self._apply(merged) + except Exception as e: + return self._fail(e) + self._last_error_message = None + self._last_good_digest = digest + self._remember_applied(had_content) + return None + + def _end_empty(self, path: str) -> None: + """ + Ends the empty state of a file that a load read with content, even content that cannot be + parsed, or found missing, so that it starts a new delay when it is found empty again. Its + bound is kept until a load holds nothing back. + """ + empty = self._empty_files.get(path) + if empty is not None: + empty.since = None + + def _remember_applied(self, had_content: Dict[str, bool]) -> None: + """Records what each file contributed to the data in effect, and forgets the empty files.""" + self._applied_had_content.update(had_content) + self._empty_files.clear() + + def _should_hold_back(self, path: str, now: float) -> bool: + """ + Decides whether a file that a load found with no entries is held back. A file whose + content is in effect and that has none now may be in the middle of an in-place rewrite. + It 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 that it was written since a load last found it so, for example rewritten and + truncated again between two loads, so the delay starts over. A file that has stayed so + for the whole delay is not held back. Each file is held back for at most five retry + delays after a load first held it back, so that a file that keeps being written while + every load finds it empty, or files that are emptied in turn, are not held back without + end. The bound starts over only at a load that holds nothing back, for every file, whether + the load applies, is skipped or fails. A load reads every file even after one fails. A file whose content is not in effect + is parsed right away, and so is any file when retries are off, since nothing would read it + again. + """ + if self._retry_delay <= 0 or not self._applied_had_content.get(path, False): + return False + state = _observe(path) + empty = self._empty_files.get(path) + if empty is None: + empty = _EmptyFile() + self._empty_files[path] = empty + if empty.since is None or empty.state != state: + empty.since = now + empty.state = state + if now - empty.since >= self._retry_delay: + # The file stayed empty for the whole delay. Its bound starts over only at a load that + # holds nothing back, so that files emptied in turn do not keep each other held back. + return False + if empty.first_held is not None and now - empty.first_held >= _MAX_HOLD_RETRY_DELAYS * self._retry_delay: + return False + if empty.first_held is None: + empty.first_held = now + return True + + def _hold_back(self, held_back: List[str], now: float) -> Optional[float]: + """ + Keeps the current data for a load that holds files back. Whatever the files' format, + nothing is applied or reported. The returned time arms the retry for when the first of + the files has stayed so for the retry delay, or has been held back for as long as it may + be, plus a tenth of the retry delay, so that a timer that fires a little early still + finds the time over. The change signal for a write starts a load sooner. + """ + if self._is_closed(): + return None + until = float("inf") + for path in held_back: + log.debug("File %s has no entries, possibly while it is being rewritten; waiting for it to be written before applying it", path) + empty = self._empty_files[path] + since = now if empty.since is None else empty.since + first_held = now if empty.first_held is None else empty.first_held + until = min(until, since + self._retry_delay, first_held + _MAX_HOLD_RETRY_DELAYS * self._retry_delay) + self._held_back = True + return max(until - now, 0.0) + self._retry_delay / 10 + + def _fail(self, err: Exception) -> Optional[float]: if self._is_closed(): - return True + return None # 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 # level and do not call on_error again. message = str(err) if message == self._last_error_message: log.debug("Unable to load flag data: %s", err) - return False + return self._retry_delay self._last_error_message = message log.error("Unable to load flag data: %s", err) if self._on_error is not None: @@ -501,25 +851,7 @@ def _fail(self, err: Exception) -> bool: except Exception as e: # A consumer that raises while it reports a failure must not stop the retry. log.error("Error while reporting a flag data load failure: %s", e) - return False - - -FileState = Optional[Tuple[int, int]] - - -def _observe_all(paths: List[str]) -> List[FileState]: - """ - Observes the state of each file: its modification time and size, or None when it does not - exist or cannot be examined. - """ - states: List[FileState] = [] - for path in paths: - try: - info = os.stat(path) - states.append((info.st_mtime_ns, info.st_size)) - except OSError: - states.append(None) - return states + return self._retry_delay class Poller: diff --git a/ldclient/testing/file_util.py b/ldclient/testing/file_util.py new file mode 100644 index 00000000..25f5a10a --- /dev/null +++ b/ldclient/testing/file_util.py @@ -0,0 +1,26 @@ +import time + + +def rewrite_in_place(path: str, content: str, pause: float) -> None: + """ + Rewrites a file the way applications and editors often do, without replacing it atomically: + the file is truncated, and the new content is written after the pause. A read during the + pause finds the file empty. A file that does not exist is created with the content. + """ + try: + f = open(path, 'r+') + except FileNotFoundError: + with open(path, 'w') as created: + created.write(content) + return + with f: + f.truncate(0) + f.flush() + time.sleep(pause) + f.write(content) + + +def truncate(path: str) -> None: + """Empties a file in place, which is how an in-place rewrite starts.""" + with open(path, 'r+') as f: + f.truncate(0) diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 7f9875d5..4e5454cb 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -3,7 +3,7 @@ import threading import time from queue import Empty, Queue -from typing import Any, Dict, List, Set, Tuple +from typing import Any, Dict, List, Optional, Set, Tuple from unittest import mock import pytest @@ -27,6 +27,7 @@ abs_file_paths, have_watchdog, have_yaml, + is_yaml_path, load_files, make_flag_with_value, merge, @@ -34,6 +35,7 @@ read_file ) from ldclient.impl.model import FeatureFlag, Segment +from ldclient.testing.file_util import rewrite_in_place, truncate TEST_TIMEOUT = 5.0 @@ -42,6 +44,9 @@ SHORT_DELAY = 0.05 QUIET_PERIOD = 0.3 +# The pause of the fixture's in-place rewrites, between truncating the file and writing it. +REWRITE_PAUSE = 0.03 + def write_file(path: str, content: str) -> None: with open(path, 'w') as f: @@ -78,30 +83,67 @@ def test_parse_json_document(): assert document.segments['seg1'].included == {'user1'} -def test_parse_chooses_the_parser_the_way_the_file_data_source_does(): - # The parser depends only on whether pyyaml is installed, never on the content. A tab inside - # a string is invalid JSON but valid YAML, so with pyyaml this document parses, and a parser - # chosen from the leading brace would reject it. - if not have_yaml: - pytest.skip("pyyaml is not installed") - document = parse_document(b' \n {"flagValues": {"flag1": "a\tb"}}') - assert document.flag_values == {'flag1': 'a\tb'} +@pytest.mark.parametrize('path', ['/data.yaml', '/data.yml', '/data.YAML', '/data.Yml', 'relative/data.yaml', '/x/.yaml', '/x/.YML']) +def test_a_yaml_extension_selects_yaml(path): + assert is_yaml_path(path) + + +@pytest.mark.parametrize('path', ['/data.json', '/data', '/data.txt', '/data.yaml.json', '/yaml', '/data.yaml/file', '/data.yamlx']) +def test_any_other_file_is_json(path): + assert not is_yaml_path(path) +def test_parse_chooses_the_parser_by_the_extension_not_the_content(): + # A tab inside a string is valid YAML but not valid JSON. Installing pyyaml does not make a + # JSON file YAML: the extension decides, whatever the content looks like. + content = b' \n {"flagValues": {"flag1": "a\tb"}}' + with pytest.raises(ValueError): + parse_document(content) + with pytest.raises(ValueError): + parse_document(b'flagValues:\n flag1: true\n') + if have_yaml: + assert parse_document(content, is_yaml=True).flag_values == {'flag1': 'a\tb'} + + +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") def test_parse_yaml_document(): - if not have_yaml: - pytest.skip("pyyaml is not installed") - document = parse_document(b'---\nflags:\n flag1:\n key: flag1\n "on": true\nflagValues:\n flag2: value2\nsegments:\n seg1:\n key: seg1\n') + document = parse_document(b'---\nflags:\n flag1:\n key: flag1\n "on": true\nflagValues:\n flag2: value2\nsegments:\n seg1:\n key: seg1\n', is_yaml=True) assert document.flags['flag1'].on is True assert document.flag_values == {'flag2': 'value2'} assert list(document.segments.keys()) == ['seg1'] -def test_parse_empty_document_has_no_entries(): - document = parse_document(b'') - assert document == Document() - document = parse_document(b'{}') - assert document == Document() +def test_parse_yaml_without_pyyaml_fails(monkeypatch): + monkeypatch.setattr(filedata, 'have_yaml', False) + with pytest.raises(ValueError) as excinfo: + parse_document(b'flagValues:\n flag1: true\n', is_yaml=True) + assert 'pyyaml' in str(excinfo.value) + + +@pytest.mark.parametrize('content', [b'', b' \n\t\r\n', b'\xef\xbb\xbf', b'\xef\xbb\xbf \n'], ids=['empty', 'white-space', 'byte-order-mark', 'byte-order-mark-and-white-space']) +def test_parse_empty_json_is_not_a_document(content): + # A file that is rewritten in place is empty for a moment. As JSON it is invalid, so the load + # fails and the last good data stays. + with pytest.raises(ValueError) as excinfo: + parse_document(content) + assert str(excinfo.value) == 'the file has no content, which is not a JSON document' + + +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") +@pytest.mark.parametrize('content', [b'', b' \n\n', b'---\n', b'# no entries yet\n', b'\xef\xbb\xbf'], ids=['empty', 'white-space', 'document-start', 'comment', 'byte-order-mark']) +def test_parse_empty_yaml_has_no_entries(content): + assert parse_document(content, is_yaml=True) == Document() + + +def test_parse_json_object_with_no_members_has_no_entries(): + assert parse_document(b'{}') == Document() + assert parse_document(b' {} \n') == Document() + + +def test_parse_ignores_a_byte_order_mark(): + assert parse_document(b'\xef\xbb\xbf{"flagValues": {"flag1": true}}').flag_values == {'flag1': True} + if have_yaml: + assert parse_document(b'\xef\xbb\xbfflagValues:\n flag1: true\n', is_yaml=True).flag_values == {'flag1': True} def test_parse_fills_in_missing_key_and_version(): @@ -112,10 +154,25 @@ def test_parse_fills_in_missing_key_and_version(): assert document.segments['seg1'].version == 1 +@pytest.mark.parametrize('content', [b'null', b' null\n', b'["a", "b"]', b'1', b'"text"', b'true']) +def test_parse_rejects_a_json_document_that_is_not_an_object(content): + with pytest.raises(ValueError) as excinfo: + parse_document(content) + assert str(excinfo.value) == 'the document must be an object' + + +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") +def test_parse_reads_yaml_null_as_no_entries(): + assert parse_document(b'null\n', is_yaml=True) == Document() + assert parse_document(b'~\n', is_yaml=True) == Document() + + def test_parse_rejects_documents_that_are_not_objects(): if have_yaml: with pytest.raises(ValueError): - parse_document(b'- a\n- b\n') + parse_document(b'- a\n- b\n', is_yaml=True) + with pytest.raises(ValueError): + parse_document(b'["a", "b"]') with pytest.raises(ValueError): parse_document(b'{"flags": ["not", "an", "object"]}') with pytest.raises(ValueError): @@ -127,12 +184,16 @@ def test_parse_rejects_documents_that_are_not_objects(): def test_parse_rejects_malformed_json(): - # With pyyaml the YAML parser reports the error, as in the file data sources. - expected = (ValueError, yaml.YAMLError) if have_yaml else (ValueError,) - with pytest.raises(expected): + with pytest.raises(ValueError): parse_document(b'{"flagValues"') +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") +def test_parse_rejects_malformed_yaml(): + with pytest.raises(yaml.YAMLError): + parse_document(b'flagValues: [', is_yaml=True) + + def test_parse_validates_definition_property_types(): with pytest.raises(ValueError): parse_document(b'{"flags": {"flag1": {"key": "flag1", "version": "not a number"}}}') @@ -140,6 +201,32 @@ def test_parse_validates_definition_property_types(): parse_document(b'{"segments": {"seg1": {"key": "seg1", "version": 1, "included": "not a list"}}}') +@pytest.mark.parametrize('content', [ + '{"flagValues": {"flag1": true}}'.encode('utf-16'), + '{"flagValues": {"flag1": true}}'.encode('utf-32'), + b'{"flagValues": {"flag1": "\xff"}}', +], ids=['utf-16', 'utf-32', 'invalid-utf-8']) +@pytest.mark.parametrize('is_yaml', [False, pytest.param(True, marks=pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed"))], ids=['json', 'yaml']) +def test_parse_rejects_content_that_is_not_utf_8(content, is_yaml): + with pytest.raises(ValueError): + parse_document(content, is_yaml=is_yaml) + + +def test_reloader_reports_a_file_that_is_not_utf_8_and_keeps_working(make_fixture): + # A file in another encoding fails to load like any other content that cannot be parsed, and + # the reloader keeps loading the files when they change. + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.reload_now() + f.require_applied() + with open(f.path, 'wb') as out: + out.write('{"flagValues": {"flag1": false}}'.encode('utf-16')) + f.reloader.trigger() + assert isinstance(f.require_errored(), FileReadError) + write_file(f.path, '{"flagValues": {"flag1": false}}') + f.reloader.trigger() + assert f.require_applied().flags['flag1'].variations == [False] + + def test_read_file_json_and_errors(tmp_path): path = os.path.join(str(tmp_path), 'data.json') write_file(path, '{"flagValues": {"flag1": true}}') @@ -156,6 +243,20 @@ def test_read_file_json_and_errors(tmp_path): assert 'error parsing file' in str(excinfo.value) assert path in str(excinfo.value) + write_file(path, '') + with pytest.raises(FileReadError) as excinfo: + read_file(path) + assert 'no content' in str(excinfo.value) + + +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") +def test_read_file_yaml(tmp_path): + path = os.path.join(str(tmp_path), 'data.yaml') + write_file(path, 'flagValues:\n flag1: true\n') + assert read_file(path).flag_values == {'flag1': True} + write_file(path, '') + assert read_file(path) == Document() + def test_abs_file_paths(): paths = abs_file_paths(['relative/data.json', '/absolute/data.json']) @@ -269,6 +370,39 @@ def test_load_files_reports_parse_error_with_path(tmp_path): assert excinfo.value.path == path +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") +def test_load_files_parses_each_file_in_the_format_of_its_extension(tmp_path): + json_path = os.path.join(str(tmp_path), 'first.json') + yaml_path = os.path.join(str(tmp_path), 'second.YML') + write_file(json_path, '{"flagValues": {"flag1": "json"}}') + write_file(yaml_path, 'flagValues:\n flag2: yaml\n') + result = load_files([json_path, yaml_path], DuplicateKeysHandling.FAIL) + assert {key: flag.variations for key, flag in result.flags.items()} == {'flag1': ['json'], 'flag2': ['yaml']} + + # The same YAML content in a file without a YAML extension is not JSON. + write_file(json_path, 'flagValues:\n flag1: json\n') + with pytest.raises(FileReadError) as excinfo: + load_files([json_path, yaml_path], DuplicateKeysHandling.FAIL) + assert excinfo.value.path == json_path + + +@pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") +def test_load_files_reads_an_empty_yaml_file_as_no_entries_and_fails_on_an_empty_json_file(tmp_path): + json_path = os.path.join(str(tmp_path), 'data.json') + yaml_path = os.path.join(str(tmp_path), 'data.yaml') + write_file(json_path, '{"flagValues": {"flag1": true}}') + write_file(yaml_path, '') + result = load_files([json_path, yaml_path], DuplicateKeysHandling.FAIL) + assert list(result.flags.keys()) == ['flag1'] + assert result.files[1] == FileSummary(path=yaml_path, present=True) + + write_file(json_path, '') + with pytest.raises(FileReadError) as excinfo: + load_files([json_path, yaml_path], DuplicateKeysHandling.FAIL) + assert excinfo.value.path == json_path + assert 'no content' in str(excinfo.value) + + def test_load_files_reports_duplicate_keys(tmp_path): first = os.path.join(str(tmp_path), 'first.json') second = os.path.join(str(tmp_path), 'second.json') @@ -287,8 +421,8 @@ def test_load_files_reports_duplicate_keys(tmp_path): # --------------------------------------------------------------------------- class ReloaderFixture: - def __init__(self, tmp_path, initial_content: str, **kwargs): - self.path = os.path.join(str(tmp_path), 'data.json') + def __init__(self, tmp_path, initial_content: str, name: str = 'data.json', **kwargs): + self.path = os.path.join(str(tmp_path), name) self.applied: Queue = Queue() self.errored: Queue = Queue() write_file(self.path, initial_content) @@ -302,13 +436,16 @@ def __init__(self, tmp_path, initial_content: str, **kwargs): self.reloader = Reloader(**options) def write(self, content: str) -> None: - write_file(self.path, content) + # Applications rewrite data files in place, which is not atomic. The fixture does too, + # deliberately slowly: the file is truncated and written a little later, so a load can + # find it empty, as it would with an application's writes, instead of only by chance. + rewrite_in_place(self.path, content, REWRITE_PAUSE) - def require_applied(self) -> MergeResult: - return take(self.applied) + def require_applied(self, timeout: float = TEST_TIMEOUT) -> MergeResult: + return take(self.applied, timeout) - def require_errored(self) -> Exception: - return take(self.errored) + def require_errored(self, timeout: float = TEST_TIMEOUT) -> Exception: + return take(self.errored, timeout) def require_quiet(self, duration: float = QUIET_PERIOD) -> None: deadline = time.time() + duration @@ -450,16 +587,17 @@ def test_reloader_reports_identical_failure_only_once(make_fixture): # The automatic retries keep failing in the same way. They do not report again. f.require_quiet() - # A different failure is reported. The write is ordinary, not atomic, and a retry that runs - # while the file is being rewritten reads it empty or half written. An empty document is - # valid, so such a retry succeeds and ends the retries; a half-written file is a failure of - # its own. The final content is therefore loaded on demand, and everything that was reported - # is inspected as a whole: the new failure is among the reports, and once it is the current - # failure, the retries that repeat it stay silent. + # A different failure is reported. The write is in place, so a retry can run while the file + # is empty. That retry reads the file again instead of reporting it, and a retry that finds + # it still empty reports that it has no content. The final content is loaded on demand, and + # every report is checked: each one is the new failure or the empty file, the new failure is + # among them, and once it is the current failure, the retries that repeat it stay silent. f.write('{"flags": {"flag1": "not an object"}}') f.reloader.reload_now() - _, errored = f.drain() + applied, errored = f.drain() + assert applied == [] assert any('must be an object' in str(e) for e in errored), errored + assert all('must be an object' in str(e) or 'no content' in str(e) for e in errored), errored f.require_quiet() @@ -537,7 +675,9 @@ def test_reloader_retries_after_failed_initial_load(make_fixture): def test_reloader_stops_retrying_after_success(make_fixture): - f = make_fixture('{"flagValues"', retry_delay=SHORT_DELAY, skip_unchanged=True) + # The retry delay is well over the pause of the fixture's in-place writes, so that a retry is + # unlikely to read the file in the middle of one on a slow machine. + f = make_fixture('{"flagValues"', retry_delay=0.2, skip_unchanged=True) f.reloader.reload_now() f.require_errored() @@ -695,6 +835,1308 @@ def on_error(err: Exception) -> None: f.require_applied() +# --------------------------------------------------------------------------- +# Reloader: files that are found empty +# --------------------------------------------------------------------------- + +yaml_required = pytest.mark.skipif(not have_yaml, reason="pyyaml is not installed") + +# The retry delay of the tests of a file that is found empty. The retry comes long enough after a +# trigger for a test to finish rewriting the file before it, even on a slow machine. +EMPTY_FILE_RETRY_DELAY = 1.5 + +# Shorter than EMPTY_FILE_RETRY_DELAY, so a load that happens within it after a trigger is not +# the retry. +BEFORE_THE_RETRY = 1.0 + +DEFERRAL_LOG = 'waiting for it to be written' + +YAML_FLAG1 = 'flagValues:\n flag1: true\n' +YAML_FLAG2 = 'flagValues:\n flag2: true\n' + + +def flag_keys(result: MergeResult) -> List[str]: + return list(result.flags.keys()) + + +def deferrals_logged(caplog) -> int: + return len([r for r in caplog.records if r.levelno == logging.DEBUG and DEFERRAL_LOG in r.getMessage()]) + + +def wait_for_deferrals(caplog, count: int) -> None: + deadline = time.monotonic() + TEST_TIMEOUT + while deferrals_logged(caplog) < count: + assert time.monotonic() < deadline, "the empty file was not held back" + time.sleep(0.01) + + +def change_modification_time(path: str) -> None: + """ + Makes sure that the file's modification time differs from what it was, as a rewrite changes + it, even on a file system whose timestamps are too coarse to tell two quick writes apart. + """ + modified = os.stat(path).st_mtime_ns + 1000000 + os.utime(path, ns=(modified, modified)) + + +class FakeClock: + """A monotonic clock that only moves when a test advances it.""" + + def __init__(self): + self.now = 1000.0 + + def __call__(self) -> float: + return self.now + + def advance(self, seconds: float) -> None: + self.now += seconds + + +# The retry delay of the tests that time a hold on a fake clock. The timers use the real clock, +# so no retry fires during such a test, and each load is started with reload_now. +HOUR = 3600.0 + + +def make_held_fixture(make_fixture, content: str, name: str = 'data.yaml', **kwargs) -> Tuple[ReloaderFixture, FakeClock]: + clock = FakeClock() + f = make_fixture(content, name=name, retry_delay=HOUR, clock=clock, **kwargs) + f.reloader.reload_now() + f.require_applied() + return f, clock + + +def make_two_file_fixture(make_fixture, tmp_path, first: str, second: str, **kwargs) -> Tuple[ReloaderFixture, FakeClock, str]: + clock = FakeClock() + other = os.path.join(str(tmp_path), 'other.yaml') + write_file(other, second) + f = make_fixture(first, name='data.yaml') + f.reloader = Reloader([f.path, other], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock, **kwargs) + f.reloader.reload_now() + f.require_applied() + return f, clock, other + + +def load(f: ReloaderFixture) -> Optional[MergeResult]: + """Loads the files at once, and returns what was applied, or None when nothing was.""" + f.reloader.reload_now() + try: + return f.applied.get_nowait() + except Empty: + return None + + +def require_held_back(f: ReloaderFixture) -> None: + assert load(f) is None, "the load applied the file instead of holding it back" + assert f.errored.empty(), "the load reported %r" % f.errored.get_nowait() + + +@yaml_required +def test_reloader_holds_back_a_file_until_it_has_stayed_empty_for_exactly_the_retry_delay(make_fixture, caplog): + # A writer that rewrites the file in place truncates it and then writes the new content. A + # load that finds the file empty while its content is in effect keeps the data. In YAML an + # empty file is valid, so applying it would briefly remove every entry. + caplog.set_level(logging.DEBUG) + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + assert deferrals_logged(caplog) == 1 + + clock.advance(0.95 * HOUR) + require_held_back(f) + + clock.advance(0.05 * HOUR) + result = load(f) + assert result is not None and result.flags == {} + + +@yaml_required +def test_reloader_restarts_the_delay_when_the_held_back_file_is_written(make_fixture): + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + + # Rewritten and truncated again between two loads: its state changed, so it is a new rewrite. + clock.advance(0.6 * HOUR) + write_file(f.path, YAML_FLAG2) + truncate(f.path) + change_modification_time(f.path) + require_held_back(f) + + clock.advance(0.6 * HOUR) + require_held_back(f) + + clock.advance(0.4 * HOUR) + result = load(f) + assert result is not None and result.flags == {} + + +@yaml_required +def test_reloader_observes_a_held_back_file_through_a_symbolic_link(make_fixture, tmp_path): + # The configured path is a link to the file that is rewritten, as with a mounted ConfigMap. + # Writing the file changes its state as seen through the link, so the delay starts over. + target = os.path.join(str(tmp_path), 'target.yaml') + link = os.path.join(str(tmp_path), 'link.yaml') + write_file(target, YAML_FLAG1) + try: + os.symlink(target, link) + except (OSError, NotImplementedError): + pytest.skip("symbolic links are not available") + clock = FakeClock() + f = make_fixture(YAML_FLAG1, name='unused.yaml') + f.reloader = Reloader([link], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + truncate(target) + require_held_back(f) + clock.advance(0.6 * HOUR) + write_file(target, YAML_FLAG2) + truncate(target) + change_modification_time(target) + require_held_back(f) + clock.advance(0.6 * HOUR) + require_held_back(f) + + +@yaml_required +def test_reloader_forgets_an_emptying_once_the_file_is_read_with_content(make_fixture): + # A file that was held back, then applied with content, and emptied again much later starts a + # new delay, even if it ends up with the same modification time as at the first emptying, as + # when a tool restores the times of the files it writes. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + first = os.stat(f.path) + require_held_back(f) + write_file(f.path, YAML_FLAG2) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + clock.advance(10 * HOUR) + truncate(f.path) + os.utime(f.path, ns=(first.st_atime_ns, first.st_mtime_ns)) + require_held_back(f) + + +@yaml_required +def test_reloader_holds_back_a_file_emptied_again_after_a_read_with_content_whose_state_looks_the_same(make_fixture, tmp_path): + # On a file system that records modification times coarsely, a file emptied a second time can + # look exactly as it did the first time. The read of its content in between still starts its + # delay over. The other file is kept written while empty, so that no load applies. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + first_emptying = os.stat(f.path) + truncate(other) + require_held_back(f) + + clock.advance(50 / 60 * HOUR) + write_file(f.path, YAML_FLAG1) + change_modification_time(other) + require_held_back(f) + + clock.advance(20 / 60 * HOUR) + truncate(f.path) + os.utime(f.path, ns=(first_emptying.st_atime_ns, first_emptying.st_mtime_ns)) + change_modification_time(other) + require_held_back(f) + + # The other file gets its content back while this one is still within its new delay. + clock.advance(10 / 60 * HOUR) + write_file(other, YAML_FLAG2) + require_held_back(f) + + +@yaml_required +def test_reloader_holds_back_a_file_rewritten_with_the_same_content_again_later(make_fixture): + # A tool rewrites the file in place with the same content now and then. The load after each + # rewrite is skipped as unchanged, which confirms the data in effect like an application, so + # a later rewrite is held back on its own and not cut short by the first one's bound. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1, skip_unchanged=True) + truncate(f.path) + require_held_back(f) + write_file(f.path, YAML_FLAG1) + assert load(f) is None + + clock.advance(6 * HOUR) + truncate(f.path) + require_held_back(f) + + +@yaml_required +def test_reloader_holds_back_a_file_emptied_again_after_unparsable_content_whose_state_looks_the_same(make_fixture): + # The file is emptied and held back, then saved with content that cannot be parsed, then + # emptied again with the modification time of its first emptying, as a coarse file system can + # make it look. Reading content ended its empty state, so it starts a new delay. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + first_emptying = os.stat(f.path) + require_held_back(f) + write_file(f.path, 'flagValues: [') + assert load(f) is None + assert isinstance(f.errored.get_nowait(), FileReadError) + + clock.advance(70 / 60 * HOUR) + truncate(f.path) + os.utime(f.path, ns=(first_emptying.st_atime_ns, first_emptying.st_mtime_ns)) + require_held_back(f) + + +@yaml_required +def test_reloader_applies_the_written_content_of_a_held_back_file_at_once(make_fixture): + # Two quick rewrites in place: the second truncation follows the first write before any load + # reads the new content, as when both fall in one debounce window. The file is empty at every + # load, and its emptiness is not applied; the content that is finally written is. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + write_file(f.path, YAML_FLAG2) + truncate(f.path) + change_modification_time(f.path) + clock.advance(0.2 * HOUR) + require_held_back(f) + + write_file(f.path, 'flagValues:\n flag3: true\n') + result = load(f) + assert result is not None and flag_keys(result) == ['flag3'] + + +@yaml_required +@pytest.mark.parametrize('content', [' \n\n', '# no entries yet\n', '---\n', 'null\n'], ids=['white-space', 'comment', 'document-marker', 'null']) +def test_reloader_holds_back_a_yaml_file_that_has_no_entries(make_fixture, content): + # A read can see only the start of a YAML file that is being written: white space, a comment + # or a document marker, which the parser reads as nothing. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + write_file(f.path, content) + require_held_back(f) + write_file(f.path, YAML_FLAG2) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + +@yaml_required +def test_reloader_applies_a_yaml_object_with_no_members_at_once(make_fixture, caplog): + # {} is a document with no entries, written on purpose to remove them, not part of a file. + caplog.set_level(logging.DEBUG) + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + write_file(f.path, '{}\n') + result = load(f) + assert result is not None and result.flags == {} + assert deferrals_logged(caplog) == 0 + + +@yaml_required +def test_reloader_holds_back_files_emptied_together_for_one_retry_delay(make_fixture, tmp_path, caplog): + # Every file is read and classified before any is held back, so two files that are emptied + # together start their delays in the same load and are applied together. + caplog.set_level(logging.DEBUG) + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + truncate(other) + require_held_back(f) + assert deferrals_logged(caplog) == 2 + + clock.advance(HOUR) + result = load(f) + assert result is not None and result.flags == {} + + +@yaml_required +def test_reloader_holds_back_an_emptied_file_while_another_file_changes(make_fixture, tmp_path): + # A change to one file starts a load while another is in the middle of a rewrite. The load + # applies all files or none, so the change waits too. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + require_held_back(f) + + write_file(other, 'flagValues:\n flag2: false\n') + clock.advance(0.3 * HOUR) + require_held_back(f) + + write_file(f.path, YAML_FLAG1) + result = load(f) + assert result is not None and flag_keys(result) == ['flag1', 'flag2'] + assert result.flags['flag2'].variations == [False] + + +def test_reloader_fails_on_a_file_that_cannot_be_parsed_while_another_is_held_back(make_fixture, tmp_path): + clock = FakeClock() + other = os.path.join(str(tmp_path), 'other.json') + write_file(other, '{"flagValues": {"flag2": true}}') + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader = Reloader([f.path, other], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + # One file is mid-rewrite and the other is broken. The broken file is reported at once. + truncate(f.path) + write_file(other, '{"flagValues"') + assert load(f) is None + err = f.errored.get_nowait() + assert isinstance(err, FileReadError) + assert err.path == other + + +@yaml_required +def test_reloader_stops_holding_back_a_file_that_keeps_being_emptied_after_five_retry_delays(make_fixture): + # Every load finds the file empty with a new modification time, so its delay keeps starting + # over. The run of loads that hold it back ends after five retry delays. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + for _ in range(9): + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + require_held_back(f) + + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + result = load(f) + assert result is not None and result.flags == {} + + +@yaml_required +def test_reloader_stops_holding_back_files_rewritten_in_turn_after_five_retry_delays(make_fixture, tmp_path): + # Two files are rewritten in turn, so every load finds one of them empty, and each is read + # with content in between. The run of loads that hold files back still ends after five retry + # delays. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + paths = [f.path, other] + contents = [YAML_FLAG1, YAML_FLAG2] + truncate(f.path) + require_held_back(f) + for i in range(1, 10): + clock.advance(0.5 * HOUR) + write_file(paths[(i - 1) % 2], contents[(i - 1) % 2]) + truncate(paths[i % 2]) + require_held_back(f) + + clock.advance(0.5 * HOUR) + write_file(paths[1], contents[1]) + truncate(paths[0]) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + +@yaml_required +def test_reloader_measures_a_file_from_its_own_read_when_an_earlier_file_reads_slowly(make_fixture, tmp_path, monkeypatch): + # The read of the earlier file stalls while the later file is being rewritten in place. The + # later file's delay counts from when it was read, not from when the load began. + clock = FakeClock() + slow = os.path.join(str(tmp_path), 'slow.yaml') + write_file(slow, YAML_FLAG2) + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([slow, f.path], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + original = filedata._read_file + stall = [True] + + def read_file(path: str, skip_missing_paths: bool): + read = original(path, skip_missing_paths) + if path == slow and stall[0]: + truncate(f.path) + clock.advance(0.9 * HOUR) + return read + + monkeypatch.setattr(filedata, '_read_file', read_file) + require_held_back(f) + stall[0] = False + clock.advance(0.2 * HOUR) + require_held_back(f) + + write_file(f.path, 'flagValues:\n flag3: true\n') + result = load(f) + assert result is not None and flag_keys(result) == ['flag2', 'flag3'] + + +@yaml_required +def test_reloader_measures_a_file_from_its_own_read_when_a_later_file_reads_slowly(make_fixture, tmp_path, monkeypatch): + # The file is found empty before the read of a later file stalls. Its delay counts from its + # own read, so it is over one retry delay after that read, and the retry the hold asks for + # comes then. + clock = FakeClock() + slow = os.path.join(str(tmp_path), 'slow.yaml') + write_file(slow, YAML_FLAG2) + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([f.path, slow], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + original = filedata._read_file + stall = [True] + + def read_file(path: str, skip_missing_paths: bool): + read = original(path, skip_missing_paths) + if path == slow and stall[0]: + clock.advance(0.9 * HOUR) + return read + + monkeypatch.setattr(filedata, '_read_file', read_file) + truncate(f.path) + require_held_back(f) + assert f.reloader._last_requested_retry_delay == pytest.approx(0.1 * HOUR + 0.1 * HOUR) + stall[0] = False + clock.advance(0.1 * HOUR) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + +@yaml_required +def test_reloader_a_failing_other_file_does_not_restart_the_bound(make_fixture, tmp_path): + # The file keeps being written while every load finds it empty, and the other file fails to + # parse every other load. The failures do not start the file's bound over. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + require_held_back(f) + for i in range(1, 10): + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + write_file(other, 'flagValues: [' if i % 2 == 1 else YAML_FLAG2) + assert load(f) is None + + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + write_file(other, YAML_FLAG2) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + +@yaml_required +def test_reloader_another_files_bound_does_not_cut_a_rewrite_short(make_fixture, tmp_path): + # One file keeps being written while every load finds it empty, until its bound is over. + # The other file is a quarter of a retry delay into an ordinary rewrite then, and stays held + # back until it is written. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + require_held_back(f) + for _ in range(9): + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + require_held_back(f) + + clock.advance(0.25 * HOUR) + change_modification_time(f.path) + truncate(other) + require_held_back(f) + + clock.advance(0.25 * HOUR) + change_modification_time(f.path) + require_held_back(f) + + write_file(other, 'flagValues:\n flag2: false\n') + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + assert result.flags['flag2'].variations == [False] + + +@yaml_required +def test_reloader_applies_files_emptied_in_staggered_turns_after_the_bound(make_fixture, tmp_path): + # Two files stay empty and are truncated again in turn, so that whenever one of them has + # stayed empty for the whole delay, the other is still within its delay and holds the load + # back. A file that settles so does not get a fresh bound while the other is held back. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + require_held_back(f) + applied = False + for i in range(1, 15): + clock.advance(54 / 60 * HOUR) + if i == 1: + truncate(other) + change_modification_time(other if i % 2 == 1 else f.path) + applied = load(f) is not None + if applied: + break + # The retry that the hold arms, a little after the other file's delay is over. + clock.advance(12 / 60 * HOUR) + applied = load(f) is not None + clock.advance(-12 / 60 * HOUR) + if applied: + break + assert applied, "the files were held back without end" + + +@yaml_required +def test_reloader_bound_of_a_file_held_back_during_a_failure_starts_over_once_it_is_read_again(make_fixture, tmp_path): + # While the other file fails to load, this file is rewritten once and its gap is held back. + # Much later, the other file is fixed while this one is in the middle of another rewrite, + # which is held back with a bound of its own. + clock = FakeClock() + broken = os.path.join(str(tmp_path), 'other.json') + write_file(broken, '{"flagValues": {"flag3": false}}') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([f.path, broken], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + write_file(broken, '{bad') + truncate(f.path) + assert load(f) is None + write_file(f.path, YAML_FLAG2) + assert load(f) is None + clock.advance(6 * HOUR) + assert load(f) is None + assert isinstance(f.errored.get_nowait(), FileReadError) + assert f.errored.empty(), "an identical failure was reported again" + + write_file(broken, '{"flagValues": {"flag3": false}}') + truncate(f.path) + require_held_back(f) + + +@yaml_required +def test_reloader_hold_asks_for_the_retry_from_the_end_of_the_load(make_fixture, tmp_path, monkeypatch): + # The read of the file after the held one takes longer than what is left of the held file's + # delay, so the retry is due right away, after the margin. + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + truncate(f.path) + require_held_back(f) + + original = filedata._read_file + stall = [True] + + def read_file(path: str, skip_missing_paths: bool): + read = original(path, skip_missing_paths) + if path == other and stall[0]: + stall[0] = False + clock.advance(6 / 60 * HOUR) + return read + + monkeypatch.setattr(filedata, '_read_file', read_file) + clock.advance(57 / 60 * HOUR) + require_held_back(f) + assert f.reloader._last_requested_retry_delay == pytest.approx(0.1 * HOUR) + + +@pytest.mark.parametrize('name, good, broken, fixed', [ + pytest.param('data.yaml', YAML_FLAG1, 'flagValues: [', 'flagValues:\n flag1: false\n', marks=yaml_required, id='yaml'), + pytest.param('data.json', '{"flagValues": {"flag1": true}}', '{"flagValues"', '{"flagValues": {"flag1": false}}', id='json'), +]) +def test_reloader_holds_back_the_fix_of_a_file_saved_in_place_with_an_error(make_fixture, name, good, broken, fixed): + # An operator saves a typo in place, and the fix in place much later. The truncation of the + # second save is held back like the first, so the fix is applied and the file never empty. + f, clock = make_held_fixture(make_fixture, good, name=name) + truncate(f.path) + require_held_back(f) + write_file(f.path, broken) + assert load(f) is None + assert isinstance(f.errored.get_nowait(), FileReadError) + for _ in range(6): + clock.advance(HOUR) + assert load(f) is None + assert f.errored.empty(), "an identical failure was reported again" + + truncate(f.path) + require_held_back(f) + write_file(f.path, fixed) + result = load(f) + assert result is not None and result.flags['flag1'].variations == [False] + + +@yaml_required +def test_reloader_file_kept_written_while_empty_is_applied_after_its_bound_while_an_earlier_file_fails(make_fixture, tmp_path): + # The file is kept written while empty. A file before it fails to parse on every other load. + # Those loads still read and hold back this file, so they do not start its bound over. + clock = FakeClock() + first = os.path.join(str(tmp_path), 'first.json') + write_file(first, '{"flagValues": {"flag3": true}}') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([first, f.path], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + require_held_back(f) + for i in range(1, 10): + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + write_file(first, '{"flagValues"' if i % 2 == 1 else '{"flagValues": {"flag3": true}}') + assert load(f) is None + + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + write_file(first, '{"flagValues": {"flag3": true}}') + result = load(f) + assert result is not None and flag_keys(result) == ['flag3'] + + +@yaml_required +def test_reloader_bound_of_a_file_after_a_failing_file_starts_over_once_it_is_read_again(make_fixture, tmp_path): + # The file is held back, then a file configured before it breaks while this one gets its + # content back. The failing loads still read this file, and they hold nothing back, so its + # bound starts over. Much later the other file is fixed while this one is rewritten in place, + # and the truncation is held back. + clock = FakeClock() + first = os.path.join(str(tmp_path), 'first.json') + write_file(first, '{"flagValues": {"flag3": true}}') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([first, f.path], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + require_held_back(f) + write_file(first, '{"flagValues"') + write_file(f.path, YAML_FLAG2) + assert load(f) is None + for _ in range(6): + clock.advance(HOUR) + assert load(f) is None + assert isinstance(f.errored.get_nowait(), FileReadError) + assert f.errored.empty() + + write_file(first, '{"flagValues": {"flag3": true}}') + truncate(f.path) + require_held_back(f) + + +@yaml_required +def test_reloader_reads_a_file_after_one_that_cannot_be_read(make_fixture, tmp_path): + # A file configured before this one goes missing, and missing files are not skipped, so it + # cannot be read. The failing loads still read this file, which got its content back, and + # they hold nothing back, so its bound starts over. + clock = FakeClock() + first = os.path.join(str(tmp_path), 'first.json') + write_file(first, '{"flagValues": {"flag3": true}}') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([first, f.path], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + require_held_back(f) + os.remove(first) + write_file(f.path, YAML_FLAG2) + assert load(f) is None + assert f.errored.get_nowait().path == first + clock.advance(6 * HOUR) + assert load(f) is None + + write_file(first, '{"flagValues": {"flag3": true}}') + truncate(f.path) + require_held_back(f) + + +@yaml_required +def test_reloader_starts_the_bound_of_a_file_that_cannot_be_read_over(make_fixture, monkeypatch): + # The file is held back, then cannot be read for a long time, then is readable again and + # empty after a new write. The failing loads hold nothing back and tried to read the file, + # so its bound started over, and the new emptying is held back. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + + original = filedata._read_file + unreadable = [True] + + def read_file(path: str, skip_missing_paths: bool): + if path == f.path and unreadable[0]: + raise FileReadError(path, "unable to read file: permission denied") + return original(path, skip_missing_paths) + + monkeypatch.setattr(filedata, '_read_file', read_file) + for _ in range(6): + assert load(f) is None + clock.advance(HOUR) + assert isinstance(f.errored.get_nowait(), FileReadError) + + unreadable[0] = False + change_modification_time(f.path) + require_held_back(f) + + +@yaml_required +def test_reloader_keeps_the_delay_of_a_held_back_file_through_a_failed_read(make_fixture, monkeypatch): + # A read that fails says nothing about the content, so the file's delay goes on. Once it can + # be read again, still empty and unmodified, it has been empty for the whole delay. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + + original = filedata._read_file + unreadable = [True] + + def read_file(path: str, skip_missing_paths: bool): + if path == f.path and unreadable[0]: + raise FileReadError(path, "unable to read file: permission denied") + return original(path, skip_missing_paths) + + monkeypatch.setattr(filedata, '_read_file', read_file) + clock.advance(0.5 * HOUR) + assert load(f) is None + assert isinstance(f.errored.get_nowait(), FileReadError) + + unreadable[0] = False + clock.advance(0.5 * HOUR) + result = load(f) + assert result is not None and result.flags == {} + + +def test_reloader_holds_back_a_json_file_that_holds_only_a_byte_order_mark(make_fixture, caplog): + # A byte order mark is not content, so a JSON file in effect that holds only one, as a read + # can see of a file being written, is held back quietly. + caplog.set_level(logging.DEBUG) + f, clock = make_held_fixture(make_fixture, '{"flagValues": {"flag1": true}}', name='data.json') + with open(f.path, 'wb') as out: + out.write(b'\xef\xbb\xbf') + require_held_back(f) + assert deferrals_logged(caplog) == 1 + + +def test_reloader_reports_a_json_file_that_is_not_an_object(make_fixture): + f, clock = make_held_fixture(make_fixture, '{"flagValues": {"flag1": true}}', name='data.json') + write_file(f.path, 'null') + assert load(f) is None + err = f.errored.get_nowait() + assert isinstance(err, FileReadError) + assert 'must be an object' in str(err) + assert 'as JSON, the format of its name' in str(err) + + +def test_reloader_reports_the_first_failure_when_an_empty_file_also_fails(make_fixture, tmp_path): + # The first file cannot be parsed, and the second is empty, which as JSON fails too. The load + # reports the first failure, and logs the other at Debug level. + first = os.path.join(str(tmp_path), 'first.json') + second = os.path.join(str(tmp_path), 'second.json') + write_file(first, '{"flagValues"') + write_file(second, '') + f = make_fixture('{}') + f.reloader = Reloader([first, second], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put) + f.reloader.reload_now() + err = f.require_errored() + assert isinstance(err, FileReadError) + assert err.path == first + + +def test_reloader_logs_the_failures_after_the_first(make_fixture, tmp_path, caplog): + caplog.set_level(logging.DEBUG) + first = os.path.join(str(tmp_path), 'first.json') + second = os.path.join(str(tmp_path), 'second.json') + write_file(first, '{"flagValues"') + write_file(second, '{"flags"') + f = make_fixture('{}') + f.reloader = Reloader([first, second], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put) + f.reloader.reload_now() + assert f.require_errored().path == first + assert any(r.levelno == logging.DEBUG and second in r.getMessage() for r in caplog.records) + + +def test_reloader_failing_load_reports_the_first_failure(make_fixture, tmp_path): + # Every file is read even after one fails, and the load reports the first failure. + first = os.path.join(str(tmp_path), 'first.json') + second = os.path.join(str(tmp_path), 'second.json') + write_file(first, '{"flagValues"') + write_file(second, '{"flags"') + f = make_fixture('{}') + f.reloader = Reloader([first, second], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put) + f.reloader.reload_now() + err = f.require_errored() + assert isinstance(err, FileReadError) + assert err.path == first + + +@yaml_required +def test_reloader_applies_the_same_bytes_moved_to_a_file_of_another_format(make_fixture, tmp_path): + # The format comes from the extension, so the same bytes moved from x.yaml to x.json parse + # differently: YAML reads 1e3 as a string, JSON as a number. The load is not skipped as + # unchanged. + yaml_path = os.path.join(str(tmp_path), 'x.yaml') + json_path = os.path.join(str(tmp_path), 'x.json') + content = '{"flagValues": {"flag1": 1e3}}' + write_file(yaml_path, content) + f = make_fixture('{}') + f.reloader = Reloader([yaml_path, json_path], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, skip_missing_paths=True, skip_unchanged=True) + f.reloader.reload_now() + assert f.require_applied().flags['flag1'].variations == ['1e3'] + + os.replace(yaml_path, json_path) + result = load(f) + assert result is not None and result.flags['flag1'].variations == [1000.0] + + +@yaml_required +def test_reloader_load_that_holds_a_file_back_and_then_fails_does_not_start_bounds_over(make_fixture, tmp_path): + # Two files are rewritten in turn, and a third fails to parse on every other load, after one of + # the two has been held back. Those loads hold a file back, so they do not start the bounds + # over, and the files are applied in the end. + clock = FakeClock() + other = os.path.join(str(tmp_path), 'other.yaml') + last = os.path.join(str(tmp_path), 'last.json') + write_file(other, YAML_FLAG2) + write_file(last, '{"flagValues": {"flag3": true}}') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([f.path, other, last], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + require_held_back(f) + applied = False + for i in range(1, 13): + clock.advance(0.5 * HOUR) + if i % 2 == 1: + write_file(f.path, YAML_FLAG1) + truncate(other) + write_file(last, '{"flagValues"') + else: + write_file(other, YAML_FLAG2) + truncate(f.path) + write_file(last, '{"flagValues": {"flag3": true}}') + if load(f) is not None: + applied = True + break + assert applied, "the files were held back without end" + + +@yaml_required +def test_reloader_file_found_missing_starts_a_new_delay_when_it_is_created_empty_again(make_fixture, tmp_path): + # The file is emptied and held back, then deleted, then created empty again with the + # modification time of its first emptying, while another file fails to parse, so nothing is + # applied meanwhile. Being found missing ended its empty state, so it starts a new delay. + clock = FakeClock() + broken = os.path.join(str(tmp_path), 'other.json') + write_file(broken, '{"flagValues": {"flag3": true}}') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([f.path, broken], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, skip_missing_paths=True, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + f.require_applied() + + write_file(broken, '{bad') + truncate(f.path) + first_emptying = os.stat(f.path) + assert load(f) is None + os.remove(f.path) + assert load(f) is None + + clock.advance(70 / 60 * HOUR) + write_file(f.path, '') + os.utime(f.path, ns=(first_emptying.st_atime_ns, first_emptying.st_mtime_ns)) + write_file(broken, '{"flagValues": {"flag3": true}}') + while not f.errored.empty(): + f.errored.get_nowait() + require_held_back(f) + + +@yaml_required +def test_reloader_hold_asks_for_the_retry_after_the_delay_and_a_tenth(make_fixture): + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + assert f.reloader._last_requested_retry_delay == pytest.approx(1.1 * HOUR) + + +@yaml_required +def test_reloader_later_hold_asks_for_the_rest_of_the_delay(make_fixture): + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + clock.advance(0.4 * HOUR) + require_held_back(f) + assert f.reloader._last_requested_retry_delay == pytest.approx(0.6 * HOUR + 0.1 * HOUR) + + +@yaml_required +def test_reloader_hold_asks_for_the_retry_when_the_bound_is_over(make_fixture): + # The file was written again, so its delay would end one retry delay from now, but its bound + # ends sooner. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + for _ in range(9): + clock.advance(0.5 * HOUR) + change_modification_time(f.path) + require_held_back(f) + assert f.reloader._last_requested_retry_delay == pytest.approx(0.5 * HOUR + 0.1 * HOUR) + + +@yaml_required +@pytest.mark.parametrize('due_first', ['first', 'last']) +def test_reloader_hold_asks_for_the_retry_when_the_first_held_file_is_due(make_fixture, tmp_path, due_first): + f, clock, other = make_two_file_fixture(make_fixture, tmp_path, YAML_FLAG1, YAML_FLAG2) + earlier, later = (f.path, other) if due_first == 'first' else (other, f.path) + truncate(earlier) + require_held_back(f) + clock.advance(0.5 * HOUR) + truncate(later) + require_held_back(f) + assert f.reloader._last_requested_retry_delay == pytest.approx(0.5 * HOUR + 0.1 * HOUR) + + +@yaml_required +def test_reloader_hold_after_a_change_signal_asks_for_the_same_retry(make_fixture, caplog): + caplog.set_level(logging.DEBUG) + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + + def wait_for_requested_delay(expected: float) -> None: + deadline = time.monotonic() + TEST_TIMEOUT + while f.reloader._last_requested_retry_delay != pytest.approx(expected): + assert time.monotonic() < deadline, "asked for %r" % f.reloader._last_requested_retry_delay + time.sleep(0.01) + + truncate(f.path) + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + wait_for_requested_delay(1.1 * HOUR) + clock.advance(0.4 * HOUR) + f.reloader.trigger() + wait_for_deferrals(caplog, 2) + wait_for_requested_delay(0.7 * HOUR) + + +def test_reloader_failure_asks_for_the_retry_delay(make_fixture): + f = make_fixture('{"flagValues"', retry_delay=HOUR, clock=FakeClock()) + f.reloader.reload_now() + f.require_errored() + assert f.reloader._last_requested_retry_delay == HOUR + + +@yaml_required +def test_reloader_holds_back_a_file_that_is_emptied_again_after_its_content_was_applied(make_fixture): + # A load that holds no file back ends the run, so a later rewrite is held back on its own. + f, clock = make_held_fixture(make_fixture, YAML_FLAG1) + truncate(f.path) + require_held_back(f) + write_file(f.path, YAML_FLAG2) + clock.advance(0.5 * HOUR) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + clock.advance(10 * HOUR) + truncate(f.path) + require_held_back(f) + clock.advance(HOUR) + result = load(f) + assert result is not None and result.flags == {} + + +def test_reloader_holds_back_a_json_file_that_was_empty_for_long_when_it_is_rewritten(make_fixture, caplog): + # A JSON file that stays empty fails to load once the delay is over, and keeps failing. The + # failures end the run of holds, so when the file is rewritten in place much later, the new + # truncation is held back like any other, without another report. + caplog.set_level(logging.DEBUG) + f, clock = make_held_fixture(make_fixture, '{"flagValues": {"flag1": true}}', name='data.json') + truncate(f.path) + require_held_back(f) + + clock.advance(HOUR) + assert load(f) is None + assert 'no content' in str(f.errored.get_nowait()) + clock.advance(HOUR) + assert load(f) is None + assert f.errored.empty() + + clock.advance(10 * HOUR) + held_before = deferrals_logged(caplog) + truncate(f.path) + change_modification_time(f.path) + require_held_back(f) + assert deferrals_logged(caplog) == held_before + 1 + + write_file(f.path, '{"flagValues": {"flag1": false}}') + result = load(f) + assert result is not None and result.flags['flag1'].variations == [False] + + +@yaml_required +def test_reloader_does_not_hold_back_a_file_that_is_empty_from_the_start(make_fixture, tmp_path, caplog): + # An empty YAML file next to the data, such as a placeholder for later entries, has never had + # content. It does not hold back the changes of the other file. + caplog.set_level(logging.DEBUG) + clock = FakeClock() + placeholder = os.path.join(str(tmp_path), 'placeholder.yaml') + write_file(placeholder, '') + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader = Reloader([f.path, placeholder], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, retry_delay=HOUR, clock=clock) + f.reloader.reload_now() + assert flag_keys(f.require_applied()) == ['flag1'] + + write_file(f.path, '{"flagValues": {"flag1": false}}') + result = load(f) + assert result is not None and result.flags['flag1'].variations == [False] + assert deferrals_logged(caplog) == 0 + + +@yaml_required +def test_reloader_applies_a_file_that_is_deleted_and_then_created_empty_at_once(make_fixture): + f, clock = make_held_fixture(make_fixture, YAML_FLAG1, skip_missing_paths=True) + + # Deleting a file that had content removes its data at once. + os.remove(f.path) + result = load(f) + assert result is not None and result.flags == {} + + # A missing file contributed no content, so the empty file that replaces it is applied at once. + write_file(f.path, '') + result = load(f) + assert result is not None and result.flags == {} + assert result.files == [FileSummary(path=f.path, present=True)] + + +@yaml_required +def test_reloader_does_not_hold_back_a_file_whose_content_moved_away(make_fixture, tmp_path): + # The content moves from one file to the other. The file that no longer exists contributes no + # content, so when it is created empty, that is applied at once. + clock = FakeClock() + other = os.path.join(str(tmp_path), 'other.yaml') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([f.path, other], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, skip_missing_paths=True, retry_delay=HOUR, skip_unchanged=True, clock=clock) + f.reloader.reload_now() + assert flag_keys(f.require_applied()) == ['flag1'] + + write_file(other, YAML_FLAG1) + os.remove(f.path) + result = load(f) + assert result is not None and result.files == [FileSummary(path=f.path), FileSummary(path=other, present=True, flags=1)] + + write_file(f.path, '') + result = load(f) + assert result is not None and flag_keys(result) == ['flag1'] + assert result.files == [FileSummary(path=f.path, present=True), FileSummary(path=other, present=True, flags=1)] + + +@yaml_required +def test_reloader_holds_back_a_file_whose_content_was_moved_to_it_from_another(make_fixture, tmp_path): + # The file is renamed to the other configured path (mv data.yaml other.yaml). The other + # path's content is in effect now, so an in-place rewrite of it is held back. + clock = FakeClock() + other = os.path.join(str(tmp_path), 'other.yaml') + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader = Reloader([f.path, other], DuplicateKeysHandling.FAIL, apply=f.applied.put, on_error=f.errored.put, skip_missing_paths=True, retry_delay=HOUR, skip_unchanged=True, clock=clock) + f.reloader.reload_now() + assert flag_keys(f.require_applied()) == ['flag1'] + + os.replace(f.path, other) + result = load(f) + assert result is not None and result.files == [FileSummary(path=f.path), FileSummary(path=other, present=True, flags=1)] + + truncate(other) + require_held_back(f) + write_file(other, YAML_FLAG2) + result = load(f) + assert result is not None and flag_keys(result) == ['flag2'] + + +@yaml_required +def test_reloader_applies_an_emptied_file_at_once_when_retries_are_off(make_fixture, caplog): + # Nothing would load the file later, so the empty file is parsed as it is. + caplog.set_level(logging.DEBUG) + f = make_fixture(YAML_FLAG1, name='data.yaml') + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + result = load(f) + assert result is not None and result.flags == {} + assert deferrals_logged(caplog) == 0 + + +def test_reloader_reports_an_emptied_json_file_at_once_when_retries_are_off(make_fixture): + f = make_fixture('{"flagValues": {"flag1": true}}') + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + assert load(f) is None + assert 'no content' in str(f.errored.get_nowait()) + + +def test_reloader_hold_is_not_a_failure(make_fixture, caplog): + # A rewrite in place that writes the same content again changes nothing. The load that finds + # the file empty reports nothing, and is not a failure that a later load recovers from, so + # the unchanged content is not applied again. + caplog.set_level(logging.DEBUG) + content = '{"flagValues": {"flag1": true}}' + f, clock = make_held_fixture(make_fixture, content, name='data.json', skip_unchanged=True) + truncate(f.path) + require_held_back(f) + write_file(f.path, content) + assert load(f) is None + assert f.errored.empty() + assert not any(r.levelno >= logging.WARNING for r in caplog.records), [r.getMessage() for r in caplog.records] + + +@yaml_required +def test_reloader_initial_load_reads_an_empty_yaml_file_as_no_entries(make_fixture): + f = make_fixture('', name='data.yaml', retry_delay=EMPTY_FILE_RETRY_DELAY) + f.reloader.reload_now() + result = f.require_applied() + assert result.flags == {} + assert result.files == [FileSummary(path=f.path, present=True)] + + +def test_reloader_initial_load_fails_on_an_empty_json_file(make_fixture): + f = make_fixture('', retry_delay=EMPTY_FILE_RETRY_DELAY) + f.reloader.reload_now() + assert 'no content' in str(f.require_errored()) + f.require_quiet(0.1) + + +# Real-clock tests: the retries that holds arm do fire. + +@yaml_required +def test_reloader_holds_back_an_emptied_file_until_its_write_triggers_a_load(make_fixture, caplog): + caplog.set_level(logging.DEBUG) + f = make_fixture(YAML_FLAG1, name='data.yaml', retry_delay=EMPTY_FILE_RETRY_DELAY, skip_unchanged=True) + f.reloader.reload_now() + assert flag_keys(f.require_applied()) == ['flag1'] + + truncate(f.path) + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + f.require_quiet(0.1) + + # The write triggers a load that applies the new content. Nothing was applied in between. + write_file(f.path, YAML_FLAG2) + f.reloader.trigger() + assert flag_keys(f.require_applied(BEFORE_THE_RETRY)) == ['flag2'] + f.require_quiet(0.1) + + +@yaml_required +def test_reloader_applies_an_emptied_yaml_file_that_stays_empty_at_the_retry(make_fixture, caplog): + caplog.set_level(logging.DEBUG) + f = make_fixture(YAML_FLAG1, name='data.yaml', retry_delay=EMPTY_FILE_RETRY_DELAY) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + started = time.monotonic() + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + + # The retry that the hold armed comes when the file has stayed empty and unmodified for the + # retry delay, so that first retry applies it as a document with no entries. + assert f.require_applied().flags == {} + elapsed = time.monotonic() - started + assert EMPTY_FILE_RETRY_DELAY <= elapsed < 2 * EMPTY_FILE_RETRY_DELAY + assert deferrals_logged(caplog) == 1 + + +@yaml_required +def test_reloader_a_further_hold_does_not_put_off_the_retry(make_fixture, caplog): + # A load that holds the file back again arms the retry for the time that is left of the + # delay, so the file is still applied one retry delay after it was found empty. + caplog.set_level(logging.DEBUG) + retry_delay = 3.0 + f = make_fixture(YAML_FLAG1, name='data.yaml', retry_delay=retry_delay) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + started = time.monotonic() + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + time.sleep(1.5) + f.reloader.trigger() + wait_for_deferrals(caplog, 2) + + # A whole retry delay from the second hold would come at 4.5 s. + assert f.require_applied().flags == {} + elapsed = time.monotonic() - started + assert retry_delay <= elapsed < retry_delay + 1.0 + + +def test_reloader_says_why_it_loads_the_files_again(make_fixture, caplog): + caplog.set_level(logging.DEBUG) + f = make_fixture('{"flagValues": {"flag1": true}}', retry_delay=0.2) + f.reloader.reload_now() + f.require_applied() + + # The retry that a hold arms reads the files again because a file was found empty. It finds + # the file still empty, which fails as JSON, and the retry after that failure says so. + truncate(f.path) + f.reloader.trigger() + for expected in ('Reading flag data again after a file was found empty', 'Retrying flag data load after earlier failure'): + deadline = time.monotonic() + TEST_TIMEOUT + while not any(r.getMessage() == expected for r in caplog.records): + assert time.monotonic() < deadline, "no log line %r" % expected + time.sleep(0.01) + + +def test_reloader_reports_an_emptied_json_file_only_at_the_retry_and_keeps_the_data(make_fixture, caplog): + caplog.set_level(logging.DEBUG) + f = make_fixture('{"flagValues": {"flag1": true}}', retry_delay=EMPTY_FILE_RETRY_DELAY) + f.reloader.reload_now() + f.require_applied() + + # The load that the trigger starts holds the file back instead of reporting it. The retry + # finds it still empty and unmodified, and an empty file is not a JSON document. + truncate(f.path) + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + f.require_quiet(0.1) + err = f.require_errored() + assert isinstance(err, FileReadError) + assert 'no content' in str(err) + f.require_quiet(0.1) + + +@yaml_required +def test_reloader_holds_back_a_second_rewrite_that_the_retry_of_the_first_finds_empty(make_fixture, caplog): + # The first rewrite is held back. Its content is written and the file is truncated again by + # a second rewrite before any load reads that content, as when a polled file is rewritten + # about a second apart and the poll sees only the second truncation. The retry that the + # first rewrite armed finds the file empty again, and must not apply it. + caplog.set_level(logging.DEBUG) + f = make_fixture(YAML_FLAG1, name='data.yaml', retry_delay=EMPTY_FILE_RETRY_DELAY) + f.reloader.reload_now() + f.require_applied() + + truncate(f.path) + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + + # No trigger: only the retry reads the file. + write_file(f.path, YAML_FLAG2) + truncate(f.path) + change_modification_time(f.path) + wait_for_deferrals(caplog, 2) + f.require_quiet(0.2) + + write_file(f.path, 'flagValues:\n flag3: true\n') + f.reloader.trigger() + assert flag_keys(f.require_applied(BEFORE_THE_RETRY)) == ['flag3'] + + +@yaml_required +def test_reloader_retry_that_finds_a_file_emptied_holds_it_back(make_fixture, caplog): + # A retry left over from a failure can run while the file is being rewritten in place. It + # holds the file back too, rather than applying it empty. + caplog.set_level(logging.DEBUG) + f = make_fixture(YAML_FLAG1, name='data.yaml', retry_delay=EMPTY_FILE_RETRY_DELAY) + f.reloader.reload_now() + f.require_applied() + write_file(f.path, 'flagValues: [') + f.reloader.trigger() + f.require_errored() + + # No trigger: only the retry reads the file. + truncate(f.path) + wait_for_deferrals(caplog, 1) + write_file(f.path, YAML_FLAG2) + assert flag_keys(f.require_applied()) == ['flag2'] + f.require_quiet(0.1) + + +@yaml_required +def test_reloader_retry_left_over_from_a_hold_applies_nothing_after_a_load_on_demand(make_fixture, caplog): + caplog.set_level(logging.DEBUG) + f = make_fixture(YAML_FLAG1, name='data.yaml', retry_delay=EMPTY_FILE_RETRY_DELAY, skip_unchanged=True) + f.reloader.reload_now() + f.require_applied() + truncate(f.path) + f.reloader.trigger() + wait_for_deferrals(caplog, 1) + + # A load on demand reads the rewritten file before the retry does. + write_file(f.path, YAML_FLAG2) + f.reloader.reload_now() + assert flag_keys(f.require_applied()) == ['flag2'] + + # The retry still runs. It finds the content that was just applied, so it applies and + # reports nothing. + f.require_quiet(EMPTY_FILE_RETRY_DELAY + 0.5) + + # --------------------------------------------------------------------------- # Poller # --------------------------------------------------------------------------- From a9ffa8f74bf5ccb623a0fbba84c887978e971f29 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:15:18 +0000 Subject: [PATCH 08/11] fix: Use the map key as the key of a flag or segment entry An entry's own key was kept when it differed from its map key. The entry was stored under one key and reported by evaluations under the other, and two entries with the same key under different map keys were two items with one key. The map key is now the entry's key for every purpose. --- ldclient/impl/integrations/files/filedata.py | 11 ++++++----- ldclient/testing/integrations/test_filedata.py | 16 ++++++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index f8caf95d..f51e0850 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -243,14 +243,15 @@ def _member_items(parsed: dict, name: str) -> Dict[str, Any]: def _definition(item: Any, key: str, kind_name: str) -> dict: """ - Validates the shape of a flag or segment entry. The entry is stored under its map key. The - file format allows a definition to omit its own key and version, which the model requires, - so those two properties are filled in from the map key and a version of 1. + Validates the shape of a flag or segment entry. The map key is the entry's key for every + purpose: the entry is stored under it, duplicates are detected by it, and the definition's + own key is set to it, so that evaluations report the flag by that key. The file format + allows a definition to omit its version, which the model requires, so a version of 1 is + filled in. """ if not isinstance(item, dict): raise ValueError('%s "%s" must be an object' % (kind_name, key)) - if "key" not in item: - item["key"] = key + item["key"] = key if "version" not in item: item["version"] = 1 return item diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 4e5454cb..e669381f 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -154,6 +154,22 @@ def test_parse_fills_in_missing_key_and_version(): assert document.segments['seg1'].version == 1 +def test_parse_replaces_a_different_key_with_the_map_key(): + document = parse_document(b'{"flags": {"flag1": {"key": "other-flag", "on": true}}, "segments": {"seg1": {"key": "other-segment"}}}') + assert list(document.flags.keys()) == ['flag1'] + assert document.flags['flag1'].key == 'flag1' + assert list(document.segments.keys()) == ['seg1'] + assert document.segments['seg1'].key == 'seg1' + + +def test_merge_treats_entries_with_the_same_key_property_under_different_map_keys_as_different_flags(): + document = parse_document(b'{"flags": {"flag1": {"key": "shared", "on": true}, "flag2": {"key": "shared", "on": false}}}') + result = merge([document], DuplicateKeysHandling.FAIL) + assert sorted(result.flags.keys()) == ['flag1', 'flag2'] + assert result.flags['flag1'].key == 'flag1' + assert result.flags['flag2'].key == 'flag2' + + @pytest.mark.parametrize('content', [b'null', b' null\n', b'["a", "b"]', b'1', b'"text"', b'true']) def test_parse_rejects_a_json_document_that_is_not_an_object(content): with pytest.raises(ValueError) as excinfo: From 3316ec99cdaadaddd187afe5c1e043c53f0f3168 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:15:18 +0000 Subject: [PATCH 09/11] docs: Rewrap a paragraph of the reloader docstring --- ldclient/impl/integrations/files/filedata.py | 30 ++++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index f51e0850..a9decf01 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -434,21 +434,21 @@ class Reloader: signals, retains the last good result on failure by not calling ``apply``, retries after failures, and skips applications that would change nothing. - A file can be rewritten in place: a writer truncates it and then writes the new content, and - a load in between finds it empty. When retries are enabled, a load that finds a file with no - entries, because it is empty or holds only what its parser reads as nothing (a YAML comment - or document marker), while that file's content is in effect, holds the file back: it keeps - the current data without reporting a failure, and the other files' changes wait too, since a - load applies all files or none. The file is held back until it has stayed so, without being - written, for the retry delay; a change of its modification time or size starts the delay - over. Each file is held back for at most five retry delays after a load first held it back, - until a load holds nothing back, so a file that keeps being rewritten is applied in the end - without cutting short another file's rewrite. A file whose content is not in effect, such as a - placeholder that has always been empty, is never held back. Every file is read and parsed, - even after another file fails, before any is held back, so files emptied together wait - together, and a file that cannot be read or parsed fails the load, with the first failure, - even while another is held back. The clock is read after each file's - own read, so a slow read of another file does not shorten a file's delay. + A file can be rewritten in place: a writer truncates it and then writes the new content, and a + load in between finds it empty. When retries are enabled, a load that finds a file with no + entries, because it is empty or holds only what its parser reads as nothing (a YAML comment or + document marker), while that file's content is in effect, holds the file back: it keeps the + current data without reporting a failure, and the other files' changes wait too, since a load + applies all files or none. The file is held back until it has stayed so, without being written, + for the retry delay; a change of its modification time or size starts the delay over. Each file + is held back for at most five retry delays after a load first held it back, until a load holds + nothing back, so a file that keeps being rewritten is applied in the end without cutting short + another file's rewrite. A file whose content is not in effect, such as a placeholder that has + always been empty, is never held back. Every file is read and parsed, even after another file + fails, before any is held back, so files emptied together wait together, and a file that cannot + be read or parsed fails the load, with the first failure, even while another is held back. The + clock is read after each file's own read, so a slow read of another file does not shorten a + file's delay. The worker thread starts on the first :meth:`reload_now` or :meth:`trigger` call, so a reloader that is constructed but never used does not leak a thread. From 0c6266cf801596a6dc5ed8e77dca266b788d8c36 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:15:18 +0000 Subject: [PATCH 10/11] fix: Follow a symbolic link to a data file in watch mode The watcher matched events against the configured paths in their real directories, so an edit to the target of a configured symbolic link in another directory was never reported. The watcher now also watches the directory of each link's target for the target's path, and resolves the links again after every reported change, after a retry sets up watches, and when a directory that holds a target is deleted, as a ConfigMap update does. --- ldclient/impl/integrations/files/filedata.py | 140 ++++++++++++++++-- .../testing/integrations/test_filedata.py | 96 ++++++++++++ 2 files changed, 223 insertions(+), 13 deletions(-) diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index a9decf01..6fc38fb9 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -893,6 +893,34 @@ def _poll(self) -> None: self._on_change() +# The most symbolic links followed from one configured file, as in the operating system's own +# limit, so that a loop of links ends. +_MAX_LINKS = 40 + + +def _link_target(path: str) -> Optional[str]: + """ + The path of the file that a configured symbolic link finally points to, with its directory + resolved to a real path, or None when the configured file is not a link, the target's + directory does not exist, or the links loop. The target file itself need not exist, so that + a link created ahead of its target is followed once the target appears. + """ + try: + if not os.path.islink(path): + return None + target = path + for _ in range(_MAX_LINKS): + if not os.path.islink(target): + directory = os.path.dirname(target) + if not os.path.isdir(directory): + return None + return os.path.join(os.path.realpath(directory), os.path.basename(target)) + target = os.path.join(os.path.dirname(target), os.readlink(target)) + return None + except (OSError, ValueError): + return None + + def _event_path(value: Any) -> Optional[str]: """The path carried by a watchdog event as text, or None when the event has none.""" if isinstance(value, bytes): @@ -914,6 +942,12 @@ class Watcher: When a retry sets up a watch, the callback runs once, so that changes made while the watch was not in place are picked up. + A configured file can be a symbolic link to a file in another directory. The directory of + the link's target is watched as well, for the target's path, so that an edit to the target + is reported. The links are resolved again after every reported change, after a retry sets up + watches, and when a directory that holds a target is deleted, as happens when a Kubernetes + ConfigMap volume is updated. So a link that comes to point elsewhere is followed. + Notifications for the watched paths invoke the callback. The callback can run several times for one logical edit, so feed it into a :class:`Reloader`. """ @@ -922,10 +956,10 @@ def __init__(self, paths: List[str], on_change: Callable[[], None]): if not have_watchdog: raise RuntimeError("the watchdog package is required to watch files for changes") self._on_change = on_change + self._paths = [os.path.abspath(path) for path in paths] self._watched_paths: Set[str] = set() self._directories: Set[str] = set() - for path in paths: - absolute = os.path.abspath(path) + for absolute in self._paths: real_directory = os.path.realpath(os.path.dirname(absolute)) self._watched_paths.add(os.path.join(real_directory, os.path.basename(absolute))) self._directories.add(real_directory) @@ -952,8 +986,14 @@ def on_any_event(self, event): # Serializes the watch setup against close, so that a setup in progress cannot start # anything after close has stopped the observer. self._setup_lock = threading.Lock() + # The files that the configured symbolic links point to, and the directories that are + # watched only because a target is in them. A directory that is also configured is in + # _directories and keeps its own watch. + self._target_paths: Set[str] = set() + self._target_directories: Set[str] = set() self._set_up_watches(is_retry=False) + self._refresh_links() def _set_up_watches(self, is_retry: bool) -> None: """ @@ -993,6 +1033,7 @@ def _set_up_watches(self, is_retry: bool) -> None: if len(self._pending_directories) > 0: self._ensure_retry_scheduled_locked() if is_retry and armed: + self._refresh_links() self._on_change() def _schedule(self, directory: str) -> Optional[Any]: @@ -1041,24 +1082,97 @@ def _directory_lost(self, directory: str) -> None: self._pending_directories.add(directory) self._ensure_retry_scheduled_locked() log.warning('Directory "%s" no longer exists. It is watched again when it exists.', directory) - if stale is not None: - # The observer keeps a watch whose directory is gone, and a later watch on the same - # path would reuse it, so it is removed. - try: - self._observer.unschedule(stale) - except Exception as e: - log.debug('Unable to remove the watch on directory "%s": %s', directory, e) + self._unschedule(directory, stale) + with self._lock: + held_target = any(os.path.dirname(target) == directory for target in self._target_paths) + if held_target: + # The file that a configured link names has changed. + self._refresh_links() + self._on_change() + + def _target_directory_lost(self, directory: str) -> None: + """ + Drops the watch on a directory that held only link targets, resolves the links again, + and runs the callback, since the file that a configured link names has changed. + """ + with self._lock: + if self._closed: + return + stale = self._watches.pop(directory, None) + self._target_directories.discard(directory) + log.debug('Directory "%s" of a linked data file no longer exists. The links are resolved again.', directory) + self._unschedule(directory, stale) + self._refresh_links() + self._on_change() + + def _unschedule(self, directory: str, watch: Optional[Any]) -> None: + if watch is None: + return + # The observer keeps a watch whose directory is gone, and a later watch on the same path + # would reuse it, so it is removed. + try: + self._observer.unschedule(watch) + except Exception as e: + log.debug('Unable to remove the watch on directory "%s": %s', directory, e) + + def _refresh_links(self) -> None: + """ + Resolves each configured file that is a symbolic link and watches the directory of each + target that is not a configured directory, which has a watch of its own. A target in a + directory that does not exist is not watched. A directory that no longer holds a target + of a configured link loses its watch. + + This runs on the observer's thread too, which holds the observer's lock, so it takes only + the watcher's own lock and never while the observer is called. + """ + targets = set() + for path in self._paths: + target = _link_target(path) + if target is not None: + targets.add(target) + directories = {os.path.dirname(target) for target in targets} - self._directories + with self._lock: + if self._closed: + return + self._target_paths = targets + added = sorted(d for d in directories if d not in self._watches) + removed = [d for d in self._target_directories if d not in directories] + self._target_directories = set(directories) + stale = [(d, self._watches.pop(d, None)) for d in removed] + for directory, watch in stale: + self._unschedule(directory, watch) + for directory in added: + watch = self._schedule(directory) + if watch is None: + continue + with self._lock: + keep = not self._closed and directory in self._target_directories and directory not in self._watches + if keep: + self._watches[directory] = watch + if not keep: + self._unschedule(directory, watch) def _handle_event(self, event) -> None: event_type = getattr(event, "event_type", None) if event_type not in _CHANGE_EVENT_TYPES: return src_path = _event_path(getattr(event, "src_path", None)) - if event_type in ("deleted", "moved") and getattr(event, "is_directory", False) and src_path in self._directories: - self._directory_lost(src_path) - return + if event_type in ("deleted", "moved") and getattr(event, "is_directory", False): + if src_path in self._directories: + self._directory_lost(src_path) + return + with self._lock: + is_target_directory = src_path is not None and src_path in self._target_directories + if src_path is not None and is_target_directory: + self._target_directory_lost(src_path) + return + with self._lock: + target_paths = self._target_paths for candidate in (src_path, _event_path(getattr(event, "dest_path", None))): - if candidate in self._watched_paths: + if candidate in self._watched_paths or candidate in target_paths: + # The change can be to a configured link itself, so the links are resolved again + # first. + self._refresh_links() self._on_change() return diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index e669381f..2bbb5615 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -1,5 +1,7 @@ import logging import os +import shutil +import sys import threading import time from queue import Empty, Queue @@ -2454,6 +2456,100 @@ def test_watcher_detects_file_deletion(tmp_path, make_watcher): w.require_change() +def make_symlink(target: str, link: str) -> None: + try: + os.symlink(target, link) + except (OSError, NotImplementedError) as e: + pytest.skip('symbolic links cannot be created here: %s' % e) + + +@watchdog_required +def test_watcher_detects_a_change_to_the_target_of_a_link_in_another_directory(tmp_path, make_watcher): + data = os.path.join(str(tmp_path), 'data') + config = os.path.join(str(tmp_path), 'config') + os.mkdir(data) + os.mkdir(config) + target = os.path.join(data, 'flags.json') + link = os.path.join(config, 'flags.json') + write_file(target, 'a') + make_symlink(target, link) + w = make_watcher([link]) + w.require_no_change(0.15) + write_file(target, 'bb') + w.require_change() + + +@watchdog_required +def test_watcher_detects_a_change_to_the_target_of_a_link_in_the_same_directory(tmp_path, make_watcher): + target = os.path.join(str(tmp_path), 'real.json') + link = os.path.join(str(tmp_path), 'data.json') + write_file(target, 'a') + make_symlink(target, link) + w = make_watcher([link]) + w.require_no_change(0.15) + write_file(target, 'bb') + w.require_change() + + +@watchdog_required +def test_watcher_follows_a_link_that_is_replaced_to_point_at_another_directory(tmp_path, make_watcher): + one = os.path.join(str(tmp_path), 'one') + two = os.path.join(str(tmp_path), 'two') + config = os.path.join(str(tmp_path), 'config') + for directory in (one, two, config): + os.mkdir(directory) + first = os.path.join(one, 'flags.json') + second = os.path.join(two, 'flags.json') + link = os.path.join(config, 'flags.json') + write_file(first, 'a') + write_file(second, 'a') + make_symlink(first, link) + w = make_watcher([link]) + w.require_no_change(0.15) + + # Replaced by rename, as deployment tools replace a link. + make_symlink(second, link + '.tmp') + os.replace(link + '.tmp', link) + w.require_change() + w.drain() + + write_file(second, 'bb') + w.require_change() + w.drain() + + write_file(first, 'ccc') + w.require_no_change() + + +@watchdog_required +@pytest.mark.skipif(not sys.platform.startswith('linux'), reason='the directory loss is reported through inotify') +def test_watcher_follows_a_link_through_a_directory_link_that_is_replaced_as_in_a_configmap(tmp_path, make_watcher): + # The layout that Kubernetes uses for a mounted ConfigMap: the file is a link through the + # ..data link to a timestamped directory, and an update writes a new directory, replaces + # ..data, and deletes the old directory. + config = os.path.join(str(tmp_path), 'config') + old = os.path.join(config, '..2026_a') + new = os.path.join(config, '..2026_b') + os.mkdir(config) + os.mkdir(old) + write_file(os.path.join(old, 'flags.json'), 'a') + make_symlink('..2026_a', os.path.join(config, '..data')) + make_symlink('..data/flags.json', os.path.join(config, 'flags.json')) + w = make_watcher([os.path.join(config, 'flags.json')]) + w.require_no_change(0.15) + + os.mkdir(new) + write_file(os.path.join(new, 'flags.json'), 'bb') + make_symlink('..2026_b', os.path.join(config, '..data_tmp')) + os.replace(os.path.join(config, '..data_tmp'), os.path.join(config, '..data')) + shutil.rmtree(old) + w.require_change() + w.drain() + + write_file(os.path.join(new, 'flags.json'), 'ccc') + w.require_change() + + @watchdog_required def test_watcher_picks_up_directory_that_appears_later(tmp_path, make_watcher): directory = os.path.join(str(tmp_path), 'later') From bbfa5a9cbf7f4894cf2902f671404f6e60b89e67 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Wed, 4 Nov 2026 19:44:00 +0000 Subject: [PATCH 11/11] fix: Resolve a symbolic link again until its target's directory is watched A link whose target's directory did not exist when the links were resolved, at start or after the directory was deleted, was not resolved again until another change was reported, so edits to its target after the directory appeared were missed. Such a link, or a target directory that cannot be watched, now keeps the retry schedule running, which resolves the links again, and the callback runs once when a retry watches a target. A relative link is now joined to the real path of the directory that holds it, which a test pins for a link in a directory reached through another link. --- ldclient/impl/integrations/files/filedata.py | 70 ++++++++++++++----- .../testing/integrations/test_filedata.py | 66 +++++++++++++++++ 2 files changed, 119 insertions(+), 17 deletions(-) diff --git a/ldclient/impl/integrations/files/filedata.py b/ldclient/impl/integrations/files/filedata.py index 6fc38fb9..02ba5592 100644 --- a/ldclient/impl/integrations/files/filedata.py +++ b/ldclient/impl/integrations/files/filedata.py @@ -904,18 +904,23 @@ def _link_target(path: str) -> Optional[str]: resolved to a real path, or None when the configured file is not a link, the target's directory does not exist, or the links loop. The target file itself need not exist, so that a link created ahead of its target is followed once the target appears. + + A relative link is relative to the real directory that holds it, so it is joined to that + directory's real path. Normalizing the path as written would take a ".." to the wrong place + when a directory in that path is itself a link. """ try: if not os.path.islink(path): return None target = path for _ in range(_MAX_LINKS): + directory = os.path.dirname(target) + if not os.path.isdir(directory): + return None + real_directory = os.path.realpath(directory) if not os.path.islink(target): - directory = os.path.dirname(target) - if not os.path.isdir(directory): - return None - return os.path.join(os.path.realpath(directory), os.path.basename(target)) - target = os.path.join(os.path.dirname(target), os.readlink(target)) + return os.path.join(real_directory, os.path.basename(target)) + target = os.path.join(real_directory, os.readlink(target)) return None except (OSError, ValueError): return None @@ -946,7 +951,10 @@ class Watcher: the link's target is watched as well, for the target's path, so that an edit to the target is reported. The links are resolved again after every reported change, after a retry sets up watches, and when a directory that holds a target is deleted, as happens when a Kubernetes - ConfigMap volume is updated. So a link that comes to point elsewhere is followed. + ConfigMap volume is updated. So a link that comes to point elsewhere is followed. A link + that cannot be resolved, for example because its target's directory does not exist yet or + was deleted, is resolved again on the retry interval until its target is watched, and the + callback then runs once. Notifications for the watched paths invoke the callback. The callback can run several times for one logical edit, so feed it into a :class:`Reloader`. @@ -991,6 +999,10 @@ def on_any_event(self, event): # _directories and keeps its own watch. self._target_paths: Set[str] = set() self._target_directories: Set[str] = set() + # True while a configured link's target is not watched, because the link cannot be + # resolved or its target's directory cannot be watched. The retry schedule runs while it + # is true. + self._links_unresolved = False self._set_up_watches(is_retry=False) self._refresh_links() @@ -1062,11 +1074,16 @@ def _ensure_retry_scheduled_locked(self) -> None: def _retry_pending(self) -> None: self._set_up_watches(is_retry=True) with self._lock: - # The schedule ends once everything is watched. It is set up again when a directory - # becomes pending later. The check and the stop happen under the lock, so a - # directory that becomes pending at the same time either keeps this schedule or - # starts a new one. - if len(self._pending_directories) == 0 and self._observer_running and self._retry_task is not None: + links_unresolved = self._links_unresolved + if links_unresolved and self._refresh_links(): + # The target that a retry watches may have changed while it was not watched. + self._on_change() + with self._lock: + # The schedule ends once everything is watched, the targets of the configured links + # included. It is set up again when a directory becomes pending or a link cannot be + # resolved later. The check and the stop happen under the lock, so a directory that + # becomes pending at the same time either keeps this schedule or starts a new one. + if len(self._pending_directories) == 0 and not self._links_unresolved and self._observer_running and self._retry_task is not None: self._retry_task.stop() self._retry_task = None @@ -1115,25 +1132,30 @@ def _unschedule(self, directory: str, watch: Optional[Any]) -> None: except Exception as e: log.debug('Unable to remove the watch on directory "%s": %s', directory, e) - def _refresh_links(self) -> None: + def _refresh_links(self) -> bool: """ Resolves each configured file that is a symbolic link and watches the directory of each - target that is not a configured directory, which has a watch of its own. A target in a - directory that does not exist is not watched. A directory that no longer holds a target - of a configured link loses its watch. + target that is not a configured directory, which has a watch of its own. A directory that + no longer holds a target of a configured link loses its watch. A link that cannot be + resolved, for example because its target's directory does not exist yet, or whose + target's directory cannot be watched, keeps the retry schedule running, which resolves + the links again. Returns True if it set up a watch. This runs on the observer's thread too, which holds the observer's lock, so it takes only the watcher's own lock and never while the observer is called. """ targets = set() + unresolved = [] for path in self._paths: target = _link_target(path) if target is not None: targets.add(target) + elif os.path.islink(path): + unresolved.append(path) directories = {os.path.dirname(target) for target in targets} - self._directories with self._lock: if self._closed: - return + return False self._target_paths = targets added = sorted(d for d in directories if d not in self._watches) removed = [d for d in self._target_directories if d not in directories] @@ -1141,16 +1163,30 @@ def _refresh_links(self) -> None: stale = [(d, self._watches.pop(d, None)) for d in removed] for directory, watch in stale: self._unschedule(directory, watch) + watched_any = False + unwatched = False for directory in added: watch = self._schedule(directory) if watch is None: + unwatched = True continue with self._lock: keep = not self._closed and directory in self._target_directories and directory not in self._watches if keep: self._watches[directory] = watch - if not keep: + if keep: + watched_any = True + else: self._unschedule(directory, watch) + if unresolved: + log.debug('Cannot resolve the target of %s yet. The links are resolved again shortly.', ", ".join(unresolved)) + with self._lock: + if self._closed: + return watched_any + self._links_unresolved = bool(unresolved) or unwatched + if self._links_unresolved: + self._ensure_retry_scheduled_locked() + return watched_any def _handle_event(self, event) -> None: event_type = getattr(event, "event_type", None) diff --git a/ldclient/testing/integrations/test_filedata.py b/ldclient/testing/integrations/test_filedata.py index 2bbb5615..df44f59b 100644 --- a/ldclient/testing/integrations/test_filedata.py +++ b/ldclient/testing/integrations/test_filedata.py @@ -2550,6 +2550,72 @@ def test_watcher_follows_a_link_through_a_directory_link_that_is_replaced_as_in_ w.require_change() +@watchdog_required +def test_watcher_follows_a_relative_link_in_a_directory_that_is_reached_through_another_link(tmp_path, make_watcher): + # A deployment layout: current is a link to a release directory, and the data file in the + # release is a relative link to a directory next to the releases. + releases = os.path.join(str(tmp_path), 'releases') + os.mkdir(releases) + os.mkdir(os.path.join(releases, '2026')) + os.mkdir(os.path.join(releases, 'shared')) + target = os.path.join(releases, 'shared', 'flags.json') + write_file(target, 'a') + make_symlink('../shared/flags.json', os.path.join(releases, '2026', 'flags.json')) + make_symlink('releases/2026', os.path.join(str(tmp_path), 'current')) + w = make_watcher([os.path.join(str(tmp_path), 'current', 'flags.json')]) + w.require_no_change(0.15) + write_file(target, 'bb') + w.require_change() + + +@watchdog_required +def test_watcher_watches_the_target_of_a_link_once_the_target_directory_appears(tmp_path, make_watcher, monkeypatch): + monkeypatch.setattr(filedata, '_WATCH_RETRY_INTERVAL', SHORT_DELAY) + config = os.path.join(str(tmp_path), 'config') + data = os.path.join(str(tmp_path), 'data') + os.mkdir(config) + target = os.path.join(data, 'flags.json') + make_symlink(target, os.path.join(config, 'flags.json')) + w = make_watcher([os.path.join(config, 'flags.json')]) + w.require_no_change(0.15) + + os.mkdir(data) + write_file(target, 'a') + # The retry that watches the target's directory signals a change so the file is read. + w.require_change() + w.drain() + + write_file(target, 'bb') + w.require_change() + + +@watchdog_required +@pytest.mark.skipif(not sys.platform.startswith('linux'), reason='the directory loss is reported through inotify') +def test_watcher_watches_the_target_of_a_link_again_after_its_directory_is_deleted_and_recreated(tmp_path, make_watcher, monkeypatch): + monkeypatch.setattr(filedata, '_WATCH_RETRY_INTERVAL', SHORT_DELAY) + config = os.path.join(str(tmp_path), 'config') + data = os.path.join(str(tmp_path), 'data') + os.mkdir(config) + os.mkdir(data) + target = os.path.join(data, 'flags.json') + write_file(target, 'a') + make_symlink(target, os.path.join(config, 'flags.json')) + w = make_watcher([os.path.join(config, 'flags.json')]) + w.require_no_change(0.15) + + shutil.rmtree(data) + w.require_change() + w.drain() + + os.mkdir(data) + write_file(target, 'bb') + w.require_change() + w.drain() + + write_file(target, 'ccc') + w.require_change() + + @watchdog_required def test_watcher_picks_up_directory_that_appears_later(tmp_path, make_watcher): directory = os.path.join(str(tmp_path), 'later')