Skip to content

feat: Add file data loading code with reload, retry, and polling - #525

Open
kinyoklion wants to merge 7 commits into
feat/overridesfrom
rlamb/overrides-python-filedata-reloader
Open

kinyoklion wants to merge 7 commits into
feat/overridesfrom
rlamb/overrides-python-filedata-reloader

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Sep 25, 2026 •

Copy link
Copy Markdown
Member

Summary

Adds ldclient.impl.integrations.files.filedata, the file reading, parsing, and merging logic that the file-based override source described by the OVERRIDE specification is built on. The specification requires ordered multi-file merging with duplicate keys handling, a reload that keeps the last good data across a malformed edit and retries, debouncing of change notifications, a polling change detector as well as a watching one, and correct handling of a configured file that does not exist yet. This module provides those pieces; the override source itself is added in a later PR of the stack.

The existing file data sources are not changed. The FDv1 file update processor (Files.new_data_source) and the FDv2 file initializer and synchronizer (Files.new_data_source_v2, datasystem.file_ds_builder) keep their current implementation and behavior in every respect: the flagValues expansion (an on flag with fallthrough variation 0 and version 1, evaluated with a FALLTHROUGH reason), the version fallback that stamps version 1 on entries without one, the failure messages and their levels, the FDv2 synchronizer's OFF state on a failed initial load and its INVALID_DATA error kind, and the polling and watching rules. A new test module, test_file_data_sources_pinned_behavior.py, pins each of those behaviors so the override feature stays purely additive. The baseline test files for the existing sources are unmodified.

What the new module provides for the override source:

  • Document parsing. The file name chooses the parser: a name that ends in .yaml or .yml, in any case (including a file named just .yaml), is YAML, which needs pyyaml, and any other file is JSON, parsed by the standard JSON parser even when pyyaml is installed. The content is never inspected to choose a parser. Files must be UTF-8; a leading byte order mark is ignored, and UTF-16, UTF-32 or invalid UTF-8 fails to parse. The format decides whether an empty file is valid: an empty or white-space JSON file is not a document and fails to parse (the file has no content, which is not a JSON document), as does JSON that is not an object, such as null; YAML that is empty, only comments, ---, or null has no entries. A parse error names the format the name chose. Flag and segment definitions are decoded into the model classes while the file is read, so an invalid definition fails that load. A definition may omit its own key and version, which are filled in from the map key and a version of 1.
  • Merging. Files are combined in the configured order. The duplicate keys handling is fail (the load fails) or ignore (the first file's entry is kept). The result records how many entries each file supplied.
  • Reloader. Serializes reloads, debounces change signals with a settle window that each signal extends, keeps the last good data by not applying a failed load, retries a failed load after a bounded delay so a file observed mid-write recovers without another notification, reports an identical failure once, skips an application whose files are byte-identical to the last applied ones, and applies a success after a failure even when the contents did not change. A configured file that does not exist can contribute no data instead of failing the load. A load reads, parses and classifies every configured file, even after one fails, then fails with the first failure (logging the others at Debug level), then holds files back (below), then merges. The skip-unchanged digest covers each path, whether its file exists, and its bytes, since the same bytes in a file of another name can be in another format. Closing does not wait for an in-flight reload. An exception raised by apply or on_error is reported like any other load failure and retried, and nothing from that load is remembered.
  • In-place rewrites. Applications and editors rewrite files in place: they truncate the file, then write the new content, and a load in between finds it empty. A file whose content is in effect and that a load finds empty, white space only, or parsed to nothing (YAML with only a comment or ---) is held back: the load keeps the current data, logs at Debug level and reports nothing, and the other files' changes wait too, since a load applies all files or none. {} is a document and is applied at once. The hold works as follows:
    • The file is held until it has stayed so, unwritten, for the retry delay. Its state (modification time and size, read through symbolic links) is compared only with itself, never with the clock, and a change starts the delay over.
    • The clock is read after the file's own read, so a slow read of another file does not change its delay.
    • The hold asks for a retry when the first held file is due, plus a tenth of the retry delay; the write's change signal starts a load sooner.
    • Each file is held for at most five retry delays after its first hold, so a file kept written while empty, or files emptied in turn, are applied in the end. A file's bound starts over only at a load that holds nothing back, whatever that load's outcome.
    • Content, even content that cannot be parsed, or a missing file ends a file's empty state; a failed read keeps it.
    • A file whose content is not in effect (a placeholder that was always empty, a file deleted and then created empty, a file whose emptiness was applied) is never held, and with retries off nothing is held.
    • After the delay, empty YAML is applied with no entries, and empty JSON fails to parse and keeps the data.
  • Poller. Compares modification time and size on an interval, so a same-size rewrite and a same-time size change are both detected, as are files that appear or disappear.
  • Watcher. Uses the watchdog package. It watches the directory of each file so an absent file is picked up when it appears, matches the destination of a move event 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 watchdog inotify mask also reports a file being opened or read). A failed observer start is retried every second, and a watched directory that is deleted is watched again once it exists, with one change signaled.

Not changed: a read that sees a YAML file cut off part way can still parse, with fewer entries or a different value, and is applied until the next reload; the override source's docs recommend replacing a YAML file in one operation.

Tests cover parsing (format by name, empty and non-object JSON, empty and null YAML, byte order marks and other encodings), merging, loading, the reloader (initial load, failure retention, debounce coalescing and window extension, retry with and without further signals, identical-failure and first-failure reporting, skip-unchanged and recovery, serialization of concurrent reloads, close semantics, worker thread lifecycle), the hold (on an injected clock, with a one-hour retry delay so no real retry fires: the exact delay, restarts on a state change and through a symbolic link, quick successive rewrites, files emptied together and in turn, the per-file bound and when it starts over, failures before and after a held file, slow reads, the retry each hold asks for; and on the real clock, that the retries fire), the poller, and the watcher, plus the pinned behavior of the existing sources. The reloader tests rewrite their files in place with a 30 ms pause, through ldclient/testing/file_util.py.

The in-place rewrite handling was reviewed in eight rounds as #560 (now closed, its changes moved here and into #529): multi-agent and adversarial reviews of each round, including a randomized end-to-end stress of about 384 rewrites with no empty snapshot. The final mutation check ran 59 mutants against test_filedata.py and test_file_override_source.py: 54 fail a test, and the 5 survivors are equivalent in this code (listed in #560). Both test files ran 10 times in a row with no failure.

Python CI runs for this PR. The PRs above it in the stack get no CI; they were checked locally.

SDK-3250

Opened from termag


Note

Overview
Introduces ldclient.impl.integrations.files.filedata, shared logic for loading flag/segment data from local JSON or YAML files (format chosen by .yaml/.yml extension only). It parses flags, flagValues, and segments, merges multiple files in order with fail or ignore duplicate-key handling, and expands flagValues into full on flags served by fallthrough.

The module adds a Reloader (debounced reloads, last-good retention, automatic retries, optional skip-unchanged via a content digest, and hold-back when an in-effect file is read empty mid in-place rewrite), plus Poller (mtime + size) and Watcher (watchdog, with directory retry) to drive reloads.

Existing FDv1/FDv2 file data sources are unchanged. New test_file_data_sources_pinned_behavior.py locks their current behavior (expansion, versions, errors, poll/watch rules). test_filedata.py and ldclient/testing/file_util.py exercise the new module, including in-place rewrite simulation.

Reviewed by Cursor Bugbot for commit 30a6e70. Bugbot is set up for automated code reviews on this repo. Configure here.

@kinyoklion
kinyoklion force-pushed the rlamb/overrides-python-filedata-reloader branch from c478bd1 to 7e64242 Compare September 28, 2026 20:28
@kinyoklion kinyoklion changed the title feat: Add shared file data code with reload, retry, and polling feat: Add file data loading code with reload, retry, and polling Sep 28, 2026
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.
@kinyoklion
kinyoklion force-pushed the rlamb/overrides-python-filedata-reloader branch from 7e64242 to ee3234c Compare September 30, 2026 20:31
…st 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.
@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

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.
@kinyoklion

Copy link
Copy Markdown
Member Author

The file data poller test that expected exactly one callback per rewrite is replaced by one that checks the poller goes quiet once the file is stable, following launchdarkly/go-server-sdk#473. A plain rewrite is not atomic (the write and the modification-time update are separate steps), so a poll that lands between them legitimately reports an extra change; the poller promises generous detection and the reloader absorbs the excess. The test writes in the ordinary way, drains the callbacks of the write, and then requires silence, which still fails for a poller that does not update its baseline.

Written by Claude Code on behalf of @kinyoklion.

…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.
@kinyoklion

Copy link
Copy Markdown
Member Author

The windows (3.10) failure on 7f80543 was test_reloader_reports_identical_failure_only_once, the same flaw as the poller test, in a test this PR had not touched. The test writes the file with a new failure while the 50 ms retry is running and expects the next retry to report it. The write is ordinary, not atomic, so a retry can read the file while it is empty. parse_document treats an empty document as valid, so that retry succeeds, the retries stop, and the new content is never read; require_errored then times out, which is what the run shows.

Reproduced locally with a deliberately slow write (truncate, sleep 200 ms, write): the old test shape failed 3/3 with the same timeout and the new one passed 3/3. The test now loads the final content with reload_now and inspects the drained reports as a whole (fd206c0, with a drain helper on the fixture); the first half of the test still proves that identical retries are reported once. The test 30/30 runs green, the reloader tests 10/10, test_filedata.py 67, and the file data plus override tests at the top of the stack (8d2c06c) 316. The stack is rebased onto this commit.

Written by Claude Code on behalf of @kinyoklion.

…y 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.
@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 30a6e70. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant