Repository navigation
feat: Add file data loading code with reload, retry, and polling - #525
kinyoklion wants to merge 13 commits into
Conversation
c478bd1 to
7e64242
Compare
kinyoklion
changed the title
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.
7e64242 to
ee3234c
Compare
…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.
|
bugbot review |
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.
|
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.
|
The 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 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.
|
bugbot review |
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.
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.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…tched 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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bbfa5a9. Configure here.
| 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. A link |
There was a problem hiding this comment.
This docstring and the PR description say that "a link that comes to point elsewhere is followed". That only holds when the old target directory gets deleted, which is what the ConfigMap flow does. With watchdog 6.0 on Linux, I hit three layouts where the watcher goes stale and logs nothing:
- An intermediate link is swapped. The configured path is
cfg/flags.json -> current/flags.json, withcurrent -> releases/v1.currentis re-pointed toreleases/v2by symlink and rename, andv1is kept. The rename event is for<dir>/current, which is neither a configured path nor a target, so_handle_eventdrops it. After the swap, edits toreleases/v2/flags.jsonare never signaled. - The configured directory is itself a link:
current/flags.json, withcurrent -> r1.__init__resolves it tor1once (:970-973) and never checks again, so re-pointingcurrenttor2goes unnoticed. Ifr1is then deleted, the watcher logsCannot watch directory ".../r1"at WARNING every second, indefinitely, and still never watchesr2. - The configured directory is renamed away and replaced (
mv cfg cfg.old; mv cfg.new cfg). watchdog emits nothing for IN_MOVE_SELF, so_directory_lostnever runs. The watch follows the old inode, and edits in the newcfgare missed.
The first two are the standard Capistrano-style current layout. For anyone who picks watch mode with that kind of deploy, overrides silently stop updating until a restart.
Polling is the default and the spec already steers symlink-swap deploys toward it, so I'd be fine with either of these:
- Re-resolve each configured directory and every link in the chain on the retry tick, or on events in their parent directories, and move the watch when the result changes.
- Narrow the docstring and the override source docs to the case that works, and tell people who swap symlinks or directories to use polling.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Thanks for the layouts. I went with the second option: polling is the default and follows all three, and re-resolving the chain on the retry tick would amount to polling. In 5e93d20 the Watcher docstring drops the claim that a re-pointed link is followed and names the changes that watching does not follow (a link further along the chain pointed elsewhere while its old target stays, a configured directory that is itself a link and is pointed elsewhere, a configured directory renamed and replaced), and points such deployments, like a current link to a release directory, to polling. The change_detection documentation of the override source says the same in d6b022b (#529), and the PR description is updated. The Ruby port gets the same wording (launchdarkly/ruby-server-sdk#484, #453).
| 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 keep: | ||
| watched_any = True | ||
| else: | ||
| self._unschedule(directory, watch) |
There was a problem hiding this comment.
_refresh_links computes added under _lock, releases the lock to call _schedule, and takes it again to store the watch. The observer thread (through _handle_event, _directory_lost, or _target_directory_lost) and the retry thread (through _retry_pending, or _set_up_watches at :1048) can both get here with the same directory in added:
- Both threads call
observer.schedule(D). watchdog'sObservedWatchcompares equal by(path, recursive, event_filter), so the second call reuses the first call's emitter and returns an equal watch. - The first thread stores its watch in
_watches[D]. - The second thread finds
Dalready in_watches, sokeepis False and it calls_unschedule(D, watch). That stops the shared emitter. _watches[D]still says the directory is watched, and_links_unresolvedgoes False, so the retry task stops. Nothing re-adds the watch, and edits to the target are never signaled again.
I reproduced it by forcing that interleaving with a barrier around _schedule. In practice the window is narrow: a link target or a pending directory has to become watchable at the moment an event dispatches. But once it happens, it is silent and lasts until restart.
Under the lock, a small fix is to skip _unschedule when the stored watch equals the one just returned (self._watches.get(directory) == watch). Please don't put the lock around all of _refresh_links. The observer thread holds watchdog's observer RLock while it dispatches, and schedule() takes that same RLock, so the two threads would deadlock.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in 5e93d20 as you suggested: when keep is False and the watch stored for the directory equals the one just returned, _refresh_links leaves it alone, since the observer shares one watch per directory. The lock still only covers the state, not the schedule call. A new test forces your interleaving (the other thread stores the equal watch between schedule returning and the store) and checks that unschedule is not called and that a later edit is still signaled; it fails without the change. The Ruby watcher sets up its watches under its own lock, so it has no such window.
| """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): |
There was a problem hiding this comment.
_schedule logs at WARNING on every retry, and the retry runs every _WATCH_RETRY_INTERVAL (1 s). A configured file whose directory doesn't exist yet therefore produces about 3,600 warnings an hour. A ConfigMap that isn't mounted yet is one way to get there. Spec 1.3.6.1 treats a missing file as a valid state. Could this warn once per directory, log repeats at Debug, and reset when the watch is set up? The except branch at :1062-1064 has the same pattern.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
Fixed in 5e93d20. _schedule now logs a directory that cannot be watched at WARNING once per directory and reason, logs repeats at DEBUG, and forgets the directory once its watch is set up, so a directory that is lost later is reported again. The except branch goes through the same path. A new test checks one warning across several retries and a second one after the directory was watched and lost; it fails without the change. The Ruby watcher already logged repeats at debug level.
| # A missing file is not an empty one. | ||
| self._end_empty(read.path) | ||
| had_content[read.path] = False | ||
| parsed.append((read, None)) |
There was a problem hiding this comment.
The PR description says a file that is deleted and then created empty is never held. _should_hold_back (:797) reads _applied_had_content, which only changes when a load applies. If the delete and the empty recreate land in the same debounce window, or the load that sees the file missing fails or is held because of another file, the stale True holds the new empty file for up to a retry delay. Setting self._applied_had_content[read.path] = False here, in the missing-file branch, would make the code match the description.
There was a problem hiding this comment.
This reply was written by an AI agent (Claude Code) working on behalf of @kinyoklion.
I kept the code and corrected the description instead, because the rule the reloader implements is whether a file's content is in effect, and the description's "deleted and then created empty" was looser than that. When the load that finds the file missing applies, its content is gone from the data, _remember_applied records that, and an empty file that replaces it is applied at once (test_reloader_applies_a_file_that_is_deleted_and_then_created_empty_at_once). When that load fails or is held because of another file, the file's previous content is still in effect, so an empty file at that path is treated like a file rewritten in place and held; test_reloader_file_found_missing_starts_a_new_delay_when_it_is_created_empty_again pins that case, and setting _applied_had_content[path] = False in the missing branch would fail it. A delete and an empty recreate inside one debounce window are indistinguishable from an in-place truncation and are held the same way. The description now says "a file whose deletion was applied and that is then created empty" and describes the other cases.
…atched directory, and warn once for an unwatchable directory Two threads that resolved the links at once could both find a target's directory unwatched; the observer shares one watch per directory, so the thread that lost the race removed the watch the other had stored, and the target was never watched again. That watch is now kept. A retry that resolved a link to a target in a directory that was already watched set up no watch and so signaled no change; it now signals whenever a link leads to a new target. The retry logged a warning for a directory that cannot be watched yet on every attempt; it now warns once per directory and reason and logs repeats at debug level until the directory is watched. The watcher docstring now names the changes that watching does not follow, a link further along the chain or a configured directory that is pointed elsewhere or replaced, and points such deployments to polling.
…ectory's warning only where the loss is reported Windows needs to know that a link leads to a directory when the link is created, so the watcher tests now say so for their directory links. The race test reads the target directory's key from the watcher instead of computing it, since Windows resolves the path differently. The loss of a watched directory is reported through inotify, so the part of the warning test that loses the directory runs on Linux only.
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: theflagValuesexpansion (an on flag with fallthrough variation 0 and version 1, evaluated with aFALLTHROUGHreason), the version fallback that stamps version 1 on entries without one, the failure messages and their levels, the FDv2 synchronizer'sOFFstate on a failed initial load and itsINVALID_DATAerror 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:
.yamlor.yml, in any case (including a file named just.yaml), is YAML, which needspyyaml, and any other file is JSON, parsed by the standard JSON parser even whenpyyamlis 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 asnull; YAML that is empty, only comments,---, ornullhas 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. The map key is the key of a flag or segment definition for every purpose: the definition's own key is set to it, and duplicates are detected by it, so evaluations report the flag by that key. A definition may omit its version, which is filled in as 1.fail(the load fails) orignore(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 byapplyoron_erroris reported like any other load failure and retried, and nothing from that load is remembered.---) 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: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 thewatchdogpackage. 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. A configured file that is a symbolic link is followed: the directory of the link's target is watched for the target's path, and 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 a Kubernetes ConfigMap update does. Watching does not follow a link further along the chain that is pointed elsewhere while its old target stays, a configured directory that is itself a link and is pointed elsewhere, or a configured directory that is renamed and replaced; the documentation points such deployments, like acurrentlink to a release directory, to polling, which is the default. 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 one change is then signaled. A relative link is joined to the real path of the directory that holds it, so a..in it is right when a directory on the way is itself a link.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 (including symbolic links to files in the same and in another directory, a link that is replaced, a ConfigMap-style update, a relative link in a directory reached through another link, and a target directory that appears late or is recreated), 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.pyandtest_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 reading JSON/YAML flag/segment files, merging them in order (failvsignoreon duplicate keys), and keeping data fresh viaReloader(debounced reloads, failure retention, retries, skip-unchanged digests, and hold-back when an in-effect file is seen empty mid in-place rewrite), plusPollerandWatcher(watchdog, symlinks, ConfigMap-style dirs) change detectors.Adds
ldclient/testing/file_util.pyfor simulated in-place rewrites and large test suites:test_filedata.pyfor the new module andtest_file_data_sources_pinned_behavior.pyto lock FDv1/FDv2 behavior without changing those implementations in this diff.Reviewed by Cursor Bugbot for commit 4c11d73. Bugbot is set up for automated code reviews on this repo. Configure here.