Repository navigation
fix: Keep overrides through an in-place rewrite of their files - #560
Closed
kinyoklion wants to merge 26 commits into
Closed
kinyoklion wants to merge 26 commits into
kinyoklion wants to merge 26 commits into
Conversation
Adds an override marker to the FeatureFlag and Segment models. The marker is an attribute of the model type, never part of the JSON representation, so serializing an entity or writing it to a persistent store never carries it. A marked shallow copy is produced with with_override_marker(), which leaves the source entity unmarked and shares its definition. The evaluator marks an evaluation as override-affected when any definition it reads carries the marker: the evaluated flag, a prerequisite at any depth, or a segment consulted during rule matching, including a segment that does not match and a segment referenced by another segment. The marking propagates upward only. A prerequisite's own record reflects the definitions its subtree read, so an unaffected prerequisite inside a marked evaluation is not marked and a marked prerequisite does not mark a sibling. An evaluation that fails with an error reason is still marked when it read an override definition. The marking appears on the evaluation reason as the overrideAffected indicator, present only when true, and on EvalResult.override_affected for the client and event processing. The async evaluator mirrors the sync one.
Adds the override layer described by the OVERRIDE specification: an override store that holds marked flag and segment definitions, an overlay at the store read boundary that returns the override entry for a key in preference to LaunchDarkly data and enumerates the union of both, and a sink that applies each snapshot as a single replacement and notifies flag change listeners of every flag whose evaluation may have changed, including flags that depend on a changed prerequisite or segment. The override source is an option of the FDv2 data system configuration: DataSystemConfig.override_source and ConfigBuilder.overrides(). The source is built with the data system, so invalid configuration raises from the client constructor. It starts before the data system's own threads, so its initial load completes before the constructor returns, and it is closed with the client. OverrideSource and OverrideSink are public protocols so a custom source can be supplied. The client consults the override store before its not-initialized short-circuit. An overridden flag is served before the client has LaunchDarkly data, and a flag absent from the override store still returns the client-not-ready default. The all-flags state reads through the overlay and, while the client has no LaunchDarkly data, contains only the overridden flags. The async client and data system receive the same changes, with override change notifications delivered on the event loop. The OVERRIDE specification test vectors run as a unit test through the full client stack.
…arkly data Before initialization the clients consult the override layer directly. A flag the layer does not hold gets the not-ready handling it had without an override source, for an invalid context, a store outage, and an uninitialized persistent store alike. The all-flags state is unavailable when the layer is empty. The sync client stops the components it started when the data system fails to start.
… changes The override layer keeps a dependency graph of its own entries, rebuilt on each replacement. The store expands every changed LaunchDarkly item through both its own graph and the layer's, to a fixpoint, so a change to a prerequisite flag or a segment that an override entry references notifies that entry's flag as well. This matches how dependent flags behave for LaunchDarkly data.
…iled override source from stopping the client all_flags_state returns a state of overrides alone only when no LaunchDarkly data is available and the override layer holds flags, and it reads those flags from the layer itself. Before, any layer entry was enough: a layer of segments alone over a store that held LaunchDarkly data without an init marker produced a valid state of that data, and with override flags present the same data entered the state. The overlay no longer serves the layer's entries alone when the base store fails to list its items, so a client whose store fails reports an invalid state and logs the failure, as it does without a source. An exception from the override source's start no longer fails client construction or start. The data system logs it, closes the source, clears what the source supplied, and the client runs without overrides. The SDK's own file-based source does not raise from start; a custom source could. Stopping the data system closes the override sink before it closes the source, so a snapshot supplied during shutdown is ignored. When the client is offline and an override source is configured, the SDK logs a warning that the source is not started.
A source logs and retries a failed load, and raises a configuration error from its builder's build. If start raises anyway, the SDK logs it, closes the source, removes its snapshot, and the client runs without overrides.
…nnot list its flags When the store's all-items read fails, the client logs the failure as before and, if the override layer holds flags, returns a state of those flags marked invalid instead of an empty one. Overrides only add to the state, and a client-side SDK bootstrapped from it uses its flags whatever $valid says, so the overridden values still reach it. With no override flags the state is the same invalid, empty state as without a source.
…flag An evaluation that fails after reading a definition from the override layer is still marked (OVERRIDE 1.5.4). The single-flag and all-flags error paths of both clients now set overrideAffected on the EXCEPTION reason from the flag's own marker, read through is_override_definition so that the error path cannot fail on an unexpected store item.
Carries the override-affected marking on the evaluation event input, so the event processor keys on the marking alone and never reads the evaluation reason. An evaluation marked as override-affected produces no individual feature event and no debug event, even when the flag requests them, and it is counted in summary events like any other evaluation. The marking is part of the summary counter key, so override-affected and ordinary evaluations of the same flag, variation, and version accumulate into separate counters, and a counter that aggregates marked evaluations carries the overrideAffected marker, present only when true, like the unknown marker. The evaluator passes each prerequisite record's own marking to its event, so a marked prerequisite record produces no individual event while an unaffected prerequisite inside a marked evaluation is recorded as usual. The client passes the top-level marking to the evaluation event, keeps the marking on the reason when a migration evaluation replaces it with a wrong-type error, and presents an override-affected flag in the all-flags state with trackEvents and trackReason false and no debugEventsUntilDate, so a consumer bootstrapped from the state sends no individual events for it. The async client mirrors these changes. The OVERRIDE specification vector runner now also checks the marking each evaluation contributes to its summary counter.
…ons after the loop closes When an evaluation raises and the flag definition came from the override layer, the EXCEPTION reason carries the override indicator, the default event record is marked so no individual event is produced, and the all-flags state presents the flag with tracking off. The async data system drops a flag change notification that arrives after the event loop closed instead of raising on the reload thread.
Adds ldclient.integrations.overrides.FileOverrideSourceBuilder, the file-based override source described by the OVERRIDE specification. The source reads one or more JSON or YAML files in the file data source document format, with optional flags, flagValues, and segments members, and supplies each successful load to the SDK's override sink as a full snapshot. Files are combined in the configured order. The duplicate keys handling is fail by default, which rejects the reload and keeps the previously loaded overrides, or ignore, which keeps the first file's entry. A configured file that does not exist contributes no overrides, so a file can be created later and deleting a file removes its overrides. A file that exists but cannot be read or parsed fails that reload, the last good overrides stay in effect, the failure is logged, and the load is retried after a bounded delay and on the next detected change. Change detection is one of two modes: polling, the default, examines the files once per second by default with a one second minimum, and watching reacts to file system notifications through the watchdog package. Watching without the watchdog package and a builder with no paths are construction errors. The initial load completes during client construction. Every applied change is logged at Info level with the overrides in effect and what each configured file supplied.
…l of the file override source Change detection is set up before the initial load, so an edit made while the files are first read is picked up. Close stops the reloader even when the change detector fails to close. The builder rejects a poll interval that is not a number or not finite.
…ntries as duplicate keys A path listed more than once would make every entry of that file a duplicate of itself: under the default fail handling the load failed, and under ignore the first listing won by accident. The builder now drops the repeats, keeps the first occurrence in its place in the order, and logs a warning once at build time. This is the behavior the Node SDK has.
…t on Windows The builder dropped repeated paths by exact string match, so on Windows a path listed twice in different case survived as two paths, and every key of that file failed as a duplicate of itself under the default fail handling. Paths are now compared with os.path.normcase, which ignores case on Windows and compares exactly elsewhere. The first spelling keeps its place in the order.
… SDKs A flagValues entry becomes a flag without client-side availability, so an all-flags state for client-side SDKs leaves it out even when the flag it overrides is client-side. A full definition with clientSide is the way to override such a flag. A test pins both.
Both contract test services advertise the flag-overrides capability and accept the overrides object in the SDK configuration: filePaths, and the optional duplicateKeysHandling, changeDetection, and pollIntervalMs properties, which configure the file-based override source on the FDv2 data system. An overrides object without a data system configuration is rejected, because overrides are an option of the FDv2 data system only.
A writer that rewrites a file in place truncates it and then writes the new content. With pyyaml installed, a load in between parsed the empty file as a document with no entries whatever its name, so the override source briefly removed every override. The parser is now chosen by the file name extension: .yaml and .yml are YAML, and any other file is JSON. An empty JSON file is not a document and fails to load; an empty YAML file has no entries. A YAML file without pyyaml is a configuration error. A load that finds a file with no entries (empty, or YAML that parses to nothing) while that file's content is in effect holds it back: the current data stays, nothing is reported, and the retry is armed for when the file has stayed so, unmodified, for the retry delay. A change of the file's modification time or size, read through symbolic links, starts the delay over, and loads stop holding files back after five retry delays in a row.
Member
Author
|
bugbot review |
The hold of an emptied file was bounded by the run of loads that held any file back. A failing load ended the run, so a file kept written while empty was held without end while another file failed, and one file reaching the bound applied another file in the middle of an ordinary rewrite. Each file now has its own bound, from when a load first held it back. The bound starts over only at a load that holds nothing back, for every file that load got to, including one that failed to be read or parsed, whatever the load's outcome, and when data is applied or a load is skipped as unchanged. So files emptied in turn stay bounded, a file held during another file's failure is bounded afresh, and the in-place save that fixes a typo is held back like the one that made it. A file read with content, even content that fails to parse, or found missing ends its empty state, so its next emptying starts a new delay. The clock is read after each file's own read, and again for the retry the hold arms, so a slow read of another file does not change a file's delay. The retry after a hold logs that the files are read again because a file was found empty.
Member
Author
|
bugbot review |
A load stopped at the first file that could not be read or parsed, so a file configured after it was never read and kept a bound left over from before the failure; its next in-place rewrite was then applied empty. A load now reads and classifies every configured file, then fails with the first failure, then holds files back, then merges. Every file is reached, so a load that holds nothing back starts every file's bound over. The skip-unchanged digest now covers each path and whether its file exists, not only the bytes. The format comes from the extension, so the same bytes moved from x.yaml to x.json parse differently and must not be skipped. Files in UTF-16, UTF-32 or invalid UTF-8 are reported like any other content that cannot be parsed, and the reloader keeps working; tests pin both.
Member
Author
|
bugbot review |
…iew cases A JSON file holding null, or anything else but an object, now fails to parse, as in the other SDKs; YAML null still has no entries. A file named exactly .yaml or .yml is YAML. A parse error names the format the file's name chose. A load logs the failures after the first at Debug level. _reload returns whether it held files back, so the retry's log line does not read state outside the lock, and the digest encodes paths with os.fsencode. Tests pin the cases the reviews found unpinned: a file whose read fails starts its bound over and keeps its delay, a later failure does not replace the first, and a JSON file holding only a byte order mark is held back quietly. The docs say that YAML cut off at almost any point can still parse. Two real-clock bounds and one retry delay get wider margins.
Member
Author
|
bugbot review |
The builder docs described the hold's delay, its bound and the other files waiting. They now say that the file name decides the format, that files must be UTF-8, that files can be rewritten in place, how to remove a file's overrides, and that a YAML file is best replaced in one operation because a partly written one can still parse. The mechanics stay in internal comments.
Member
Author
|
bugbot review |
There was a problem hiding this comment.
✅ 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 215e0c9. Configure here.
kinyoklion
force-pushed
the
rlamb/overrides-python-contract-tests
branch
from
November 3, 2026 23:56
0d55468 to
5800e36
Compare
This was referenced Nov 3, 2026
Member
Author
|
Closing: the changes of this PR now live in the stack PRs that introduced the code they change.
#526 to #528 and #530 were rebased with their own diffs unchanged. The tree of the new #530 head (5800e36) is identical to this PR's head tree (215e0c9). The branch stays in place. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applications and editors rewrite data files in place: they truncate the file and then write the new content. A reload that reads the file in between finds it empty. On this stack, with
pyyamlinstalled (it is in thetest-filesourceextra), such a reload applied an empty document whatever the file's name, so the override source briefly removed every override until the next reload. A client evaluating an overridden flag during a slow rewrite (truncate, pause 300 ms, write) serveda -> default -> b: on every rewrite in watching mode, and whenever a poll landed in the pause in polling mode (2 of 10 YAML rewrites, 2 of 20 JSON rewrites). Withoutpyyaml, an empty JSON file failed to parse, the overrides stayed, and the failure was logged at Error level.The OVERRIDE spec's retry requirement exists to keep "a partially written or briefly invalid file from emptying the layer" (section 1.3). This change makes the file-based override source keep its overrides through such a rewrite. The legacy file data sources (
Files.new_data_source,Files.new_data_source_v2) have their own code and are not changed; they never accepted an empty file and servea -> bfor the same rewrite.The format is chosen by the file's extension
The parser was chosen the way the file data sources choose it: the YAML parser for every file when
pyyamlis installed, the JSON parser for every file otherwise. So whether an empty file was valid depended on an installed package, not on the file. The OVERRIDE spec (1.3.3, sdk-specs #272) recommends choosing the parser by extension, lets an SDK whose file data source inspects the content keep doing so, and says nothing about empty files. This code has not been released, so it now chooses by extension and lets the format decide: a file whose name ends in.yamlor.yml, in any case, is YAML, including a file named just.yaml, and any other file, including one with no extension, is JSON. Whether an empty file is valid is then up to the format, like the rest of the content.pyyamlis installed, which reads some content differently from the YAML parser it replaces:.jsonfile no longer loads, nor does JSON that only YAML accepts (a tab inside a string).null, an array, a number, a string or a boolean fails to parse, as in the other SDKs. YAMLnullstill has no entries.1e3is now a number (YAML 1.1 read it as the string"1e3"), andNaN,Infinityand-Infinityare accepted as numbers, as Python'sjsonmodule does.error parsing file as JSON, the format of its name: ...)..jsonfile with one keeps loading.pyyamlis a configuration error:FileOverrideSourceBuilder.buildraisesValueError, as it does for watching mode withoutwatchdog(OVERRIDE 1.2.9, 1.3.3). Before, such a file was parsed as JSON on every reload.I checked the rest of the engine for other rules that look at content instead of the extension, and found none: a missing file is skipped before parsing, and YAML
nullis read as nothing, like an empty YAML file.An empty JSON file is invalid
An empty JSON file, or one with only white space, now fails to parse with
the file has no content, which is not a JSON document, and the overrides in effect stay, as for any other content that cannot be parsed. An empty YAML file is still a document with no entries. To remove every override from a file, write{}or delete the file.A file found with no entries is held back until it is written
With the JSON rule alone, a JSON file that is empty for a moment fails to parse and is retried, but the failure is logged at Error level for an ordinary write. A YAML file can legitimately be empty, so its parser cannot tell an emptied file from one in the middle of a rewrite. The
Reloadernow decides, for every load, whether a file may be in the middle of a rewrite, whatever its format.A file has no entries when it is empty (white space counts as empty) or when its parser reads it as nothing: YAML with only a comment or a
---marker, which a read can see of a YAML file that is being written.{}is a document, written on purpose, and is applied right away. When a load finds a file with no entries while that file's content is in effect, it holds the file back: the current data stays, the load logs at Debug level and reports nothing, 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 (1 second). The hold arms the retry for the moment the first held file is due, its delay or its bound being over, plus a tenth of the retry delay, and the write of the new content changes the file, so its change signal starts a load sooner, which applies the new content. After the delay, a YAML file is applied with no entries, and an empty JSON file fails to parse and keeps the overrides.The
Reloaderkeeps:x.yamltox.jsonparse differently and are not skipped, and renaming a file to another configured path (mv a.yaml b.yaml) is applied, so the record follows the content;So:
retry_delay=0), nothing would load the file later, so an empty file is parsed right away. The override source always retries.A rewrite is covered when the gap between truncating and writing is shorter than the retry delay. When a writer pauses longer, the retry finds the file still empty: a YAML file is applied with no entries until the write, and a JSON file is reported as a failure and keeps the overrides.
To decide before parsing, a load reads each file (
_read_file), parses it (_parse_file, which tells a document from nothing), classifies it, and only after the last file merges them (_merge_files); one read still feeds both the skip-unchanged digest and the parse. The hold is timed by an injectable monotonic clock, read after each file's own read, which the deterministic tests replace, and the retry each load asks for is recorded for the tests.This is the eighth version of the rule, the same as the current draft of dotnet-core #394. The first held a file back once, when its previous read had found content; an adversarial review found three sequences that still apply an empty YAML file (two quick rewrites in one debounce window, a change to another file while one is mid-rewrite, a polled file rewritten about a second apart). The second keyed the delay on the file's modification time with a per-file cap; reviews of the other SDKs found that files rewritten in turn were held without end, that a slack in the delay check let another load apply a gap shorter than the delay, and that YAML with only a comment was applied as no entries. The fourth, the first commit of this PR, bounded the run of loads that hold files back; a review of .NET found that a failing load restarted that bound, that one file reaching it cut short another file's rewrite, and that a clock read once per load mismeasured a file read after a slow read. The fifth bounded each file on its own but started a file's bound over when it settled or was read with content; a review found that files emptied in staggered turns were then held without end, and that a bound kept through a long failure applied a later rewrite empty. The sixth did not start over the bound of the file whose read or parse failed, so the in-place save that fixes a typo, saved in place too, was applied empty. The seventh stopped a load at the first failure, so a file configured after the failing file kept a stale bound, and its next rewrite was applied empty (found by the Java and Ruby reviews). Each of those cases has a test here, and each fails with the earlier rule it broke.
Documentation
The public docs say only what an application needs; how the hold works is in the internal comments and in this description.
FileOverrideSourceBuildersays that the file name decides the format (.yamlor.ymlis YAML, which needspyyaml; anything else is JSON) and that files must be UTF-8. It says that files can be rewritten in place, and that the source keeps the overrides in effect while it finds a file empty during a rewrite. It says how to remove a file's overrides (delete the file or write{}), that an empty JSON file is not a valid document, and that a partly written YAML file can still parse, so a YAML file is best replaced in one operation.buildlists the new configuration error.Reloader,_FileOverrideSourceandparse_documentdocstrings and comments describe the hold, its delay and bound, the retry it asks for, and the empty-file rules.Not changed: YAML cut off part way
A read that overlaps a write can see the first part of a YAML file, and YAML cut off at almost any point can still parse: with fewer entries, or with a different value (
flagValues:\n flag1:gives a null variation;flag2: 10cut fromflag2: 100gives 10). Such a part is applied until the next reload. JSON cut off anywhere fails to parse. This predates this change. Telling a part from a whole file needs a content rule this change does not add, so the docs recommend replacing a YAML file in one operation.Difference from the other SDKs
.NET (dotnet-core #394) makes the same change. Go still detects the format from the content, so an empty file of any name loads as no overrides there. The legacy file data sources in this SDK still parse every file with the YAML parser when
pyyamlis installed, so a.jsonfile that only YAML accepts loads there but not in the override source.Testing
Python CI runs only for pull requests based on
mainorfeat/**, so this stacked PR gets no CI. Everything below ran locally on Python 3.13 with all extras (pyyaml6.0.3,watchdog7.0.0)..yaml,.ymlin any case, also a file named just.yaml; anything else is JSON, also withpyyamlinstalled); empty, white-space and byte-order-mark-only JSON fails with the message above; JSON that is not an object (null, an array, a number, a string, a boolean) fails; YAMLnulland~have no entries; empty, white-space,---and comment-only YAML has no entries; a byte order mark is ignored; YAML withoutpyyamlfails; each file of a load is parsed in its own format..yamlor.YMLpath withoutpyyamlraisesValueError; JSON paths do not.Reloadertests on a fake clock (a retry delay of an hour, so no real retry fires, and every load started withreload_now):---only and YAMLnullcount as no entries, and so does a JSON file holding only a byte order mark, which is held back quietly;{}is applied at once; a JSON file holdingnullis reported as not an object.x.yamltox.json(1e3, a string in YAML and a number in JSON) are applied, not skipped as unchanged.Reloadertests on the real clock, waiting for the Debug line rather than sleeping: the retry after a hold logsReading flag data again after a file was found empty, and the retry after a failureRetrying flag data load after earlier failure; a held back file applied by the load its write triggers; a file that stays empty applied by the first retry the hold armed, one retry delay after it was found empty; a further hold that does not put off that retry; an empty JSON file reported only at the retry; the retry of a first rewrite finding the second truncation; a retry left over from a failure holding the file back; a retry left over from a hold applying nothing after a load on demand.rewrite_in_placeinldclient/testing/file_util.py), for JSON and YAML, in watching and polling modes:a -> b, never the default.{}then removes them; an emptied YAML file removes them after the retry delay.Reloadertests now rewrite their file in place with a 30 ms pause after truncating, so they exercise this case deliberately instead of by chance.test_reloader_reports_identical_failure_only_oncenow also checks that each report is the one it expects or the empty file, and that nothing is applied.a -> bin all six combinations (JSON and YAML, watching and polling, and JSON withoutpyyaml), with no Error lines. On the base it served the default on every rewrite when watching, and an empty JSON file withoutpyyamllogged an Error on every rewrite.test_filedata.py(176) andtest_file_override_source.py(59) fail, with a shim that adds the new helper names and accepts the test clock so that each failure is about behavior. All the end-to-end rewrite tests fail there. Each earlier version of the rule fails some of thetest_filedata.pytests: 16 (first), 7 (second), 9 (fourth, the first commit of this PR), 6 (fifth), 4 (sixth), 5 (seventh, the second commit).LD_SKIP_DATABASE_TESTS=1 pytest -W error::SyntaxWarning): 2090 passed, 367 skipped.mypy ldclient,isort --check,pycodestyleand the Sphinx docs build (-W -E) pass.flag overridessuite (79 run, 15 skipped by capability), including its change notification tests.Opened from termag
Note
Overview
Fixes the file-based override source briefly clearing overrides when a data file is truncated mid–in-place rewrite, and aligns parsing with extension-based format rules.
Format selection no longer depends on whether
pyyamlis installed:.yaml/.yml(any case) use YAML; everything else uses JSON. Empty or whitespace-only JSON is rejected; empty YAML still means no entries. UTF-8 BOM is stripped;FileOverrideSourceBuilder.buildnow fails fast if a YAML path is configured withoutpyyaml.Reload pipeline splits read, parse, and merge so every configured file is read and classified before apply/fail/hold; the unchanged-content digest includes path, existence, and raw bytes (so renames/format changes are not skipped incorrectly).
Reloaderhold-back: when retries are enabled and a file that previously contributed data is read with no entries (empty file or YAML that parses as nothing), the load keeps the last good snapshot, defers other files in the same load, and schedules a retry using mtime/size stability, a per-file cap of five retry delays, and injectable monotonic clock timing—so clients should not seeoverride → default → overrideduring slow rewrites.Public docs and override-source comments describe rewrite behavior; legacy file data sources are unchanged.
Reviewed by Cursor Bugbot for commit 215e0c9. Bugbot is set up for automated code reviews on this repo. Configure here.