Repository navigation
feat: Add reload infrastructure for file-based flag overrides - #220
kinyoklion wants to merge 10 commits into
Conversation
f0038ed to
79250a7
Compare
|
bugbot review |
|
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. |
|
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 3b81e47. Configure here.
The flag overrides feature needs a file source that reloads reliably: coalesced change notifications, retry after a failed read, retention of the last good data, sequential reloads, and support for files that do not exist yet. This adds that infrastructure to the integrations package as a purely additive change. The existing file data source keeps its current behavior. - FileDataReloader serializes reloads, debounces change signals, keeps the last good data on failure by not applying, retries a failed load after a bounded delay, reports an identical repeated failure once, and skips a reload whose raw content did not change. Close never waits for an in-flight read. - FileDataPoller detects changes by examining each file's modification time and size on a fixed interval, including a file that appears or disappears. - FileDataWatcher watches parent directories and signals once after start so a change between an initial load and the start of watching is not missed. - OverrideFileLoader reads a list of files with the override source's rules: a missing file contributes no entries, entries keep the versions that the documents specify, a value-only entry becomes a flag that is off and serves the value, a document that the model rejects is a file data error, and the result carries a per-file summary and a content digest. Two additive FlagFactory methods keep document versions. FileDataSourceImpl, FileSynchronizer, FileInitializer, FileDataSourceBase, and FileDataException are unchanged. Two new test classes pin the file data source's reload and error behavior: a reload on every watch event with no debounce, no retry, and no skip, every failure reported and logged as the plain description at error level, a model-rejected document escaping as a SerializationException, and a description that requires a cause.
A directory that does not exist at start, or whose watch key becomes invalid because it was deleted, is remembered as missing and registered again on a fixed schedule from the worker thread. A recovered directory signals one change so that files written while it was not watched are picked up. The worker ends on close.
A RuntimeException from the callback is logged at error level instead of ending the repeating task.
The last good hash and the failure state are recorded only after the handler accepts the result. An exception from apply goes through the same failure path as a read or parse failure, so it is reported once and retried after the retry delay.
The time of the next registration attempt and the wait before it come from System.nanoTime, so a step of the system clock neither delays nor advances the retry.
A plain rewrite is not atomic: the write and the modification-time update 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 waits for the write to be fully observed and then requires that the count stays put, which still fails for a poller that does not update its baseline.
…write
Applications and editors rewrite a file in place: they truncate it and then
write the new content. A reload in between found the override file empty,
chose YAML for it because it did not start with '{', and applied a document
with no entries, so every override was briefly removed.
The override loader now chooses the parser by the file's extension (.yaml
and .yml are YAML, any other file is JSON), detects the text encoding from
a byte order mark and reads UTF-8 otherwise, and rejects an empty JSON file
and a JSON document that is not an object. A file whose content is in
effect and that a load finds empty, white space, or YAML with no document
is held back until it has stayed so, without being written, for the retry
delay; the hold's retry is armed for the time left, and each file's hold is
bounded at five retry delays from its first hold, starting over at a load
that holds nothing back. A load reads every file, also after a failure,
fails with the first failure, and merges the entries only after every file
is read. The skip-unchanged digest covers each file's path and presence.
The file data source keeps choosing the parser by content.
In watching mode the watcher registered each configured file's directory and counted only events that named a configured file. A mounted Kubernetes ConfigMap links each file through a ..data link that is swapped to a new directory on every update, so its updates named no configured file and were never picked up. A configured link to a file in another directory was not followed either, because nothing watched that directory. Polling picked up both. The Node, .NET, Python and Ruby watchers follow these links. An event for another entry in a watched directory now counts when a configured file, read through any symbolic link, leads to another file or its file's modification time or size changed, so a busy sibling file still costs no reload. The directory of each configured link's final target is watched as well and found again after every event. A target directory that is deleted, as the previous version of a ConfigMap is, is dropped rather than registered again with a warning. A configured directory that is itself a link and is pointed elsewhere is not followed; polling covers that.
3b81e47 to
68f868e
Compare
There was a problem hiding this comment.
Risk: medium.
Automated risk review (dry run): Medium
This PR would not be automatically approved. Human approval required.
Why: It adds substantial new SDK reload infrastructure (FileDataReloader, FileDataWatcher, FileDataPoller, OverrideFileLoader) plus symlink/ConfigMap watch behavior and empty-file holdback for flag overrides, which is more than a narrow, easy-to-review change even with the accompanying tests.
If you disagree with this review, share this PR in #feedback-agent-approvals or submit a changes-requested review explaining why.
Sent by Cursor Approval Agent: Dry-Run PR Approver


Update, 2026-11-13
feat/overrides, which now includes feat: Add the overrideAffected indicator to EvaluationReason #218 and feat: Count override-affected evaluations separately and send no individual events for them #219 and hasmainmerged into it. Frommainit gets the 7.18.0 release, CI on the latest v3 contract tests, flag change listener support in the contract test service, and thejava-sdk-internal1.12.1 and 1.12.2 fixes. A common 2.6.0 and internal 1.13.0 release from this branch needs those fixes...datalink, so no event named the file and the update was never picked up. A configured link to a file in another directory wasn't followed either. Polling picked up both, and the Node, .NET, Python and Ruby watchers follow these links.FileDataWatcherTestcases cover a ConfigMap update, a change to a link target in another directory, and a link pointed at a file in a third directory. Three mutants each fail at least one of them: other entries never count, target directories aren't watched, and a deleted target directory is retried.commonandinternalpublished locally fromfeat/overrides:Summary
The flag overrides feature defined by the OVERRIDE specification needs a file source that reloads reliably: coalesced change notifications, retry after a failed read, retention of the last good data, sequential reloads, support for files that do not exist yet, and files that are rewritten in place. This change adds that infrastructure to the integrations package for the file-based override source that follows. The existing file data source keeps its current behavior.
FileDataReloaderserializes reloads, debounces change signals with a 100 ms settle window, keeps the last good data on failure by not applying, retries a failed load after one second, reports an identical repeated failure once, and skips a reload whose content did not change. Close never waits for an in-flight read. An exception from the apply callback is reported like any other load failure and retried, and nothing from that load is remembered. A load that holds an emptied file back (below) applies nothing, reports no failure, and arms its retry for the time the loader asks for; a failure still arms the normal retry. The loader is told of every result that becomes the data in effect, applied or skipped as unchanged, throughLoader.accepted.FileDataPollerdetects changes by examining each file's modification time and size on a fixed interval, including a file that appears or disappears. A change callback that throws is logged and polling continues. Itsobserveis package-private, so that the loader reads a file's state the same way, through symbolic links.FileDataWatcherwatches parent directories through the file system watch service and signals once after start so a change made between an initial load and the start of watching is not missed. A directory that is missing at start or deleted later is registered again on a one-second schedule once it exists, and one change is signaled so the reload picks up what was written meanwhile.OverrideFileLoaderreads a list of files with the override source's rules: a file that does not exist contributes no entries, entries keep the versions that the documents specify, a value-only entry becomes a flag that is on and serves the value by fallthrough, a document that parses but holds a flag the model rejects is reported as a file data error, and the result carries a per-file summary and a content digest of each file's path, whether it exists, and its content.Format, encoding and empty files
The override loader chooses the parser by the file name:
.yamland.yml, in any case, are YAML, and any other file, including one with no extension, is JSON. The OVERRIDE spec (requirement 1.3.3, sdk-specs #272) recommends the extension and lets an SDK whose file data source inspects the content keep doing so; the override source is unreleased, so it follows the extension, which lets the format decide whether an empty file is valid.UnicodeReader.the file has no content, which is not a JSON document, and the last good overrides stay. An empty JSON placeholder therefore fails the whole reload. A JSON document that is not an object, such asnullor[], fails withthe JSON document is not an object.---marker, is a document with no entries.\/, loads from a.jsonfile; SnakeYAML, which reads YAML 1.1, rejects it in a.yamlfile.In-place rewrites
Applications and editors rewrite a file in place: they truncate it and then write the new content. A load in between finds the file empty. A YAML file can legitimately be empty, so its parser cannot tell an emptied file from one in the middle of a rewrite. The loader keeps, for each file, whether it contributed content to the data in effect (updated by
accepted), and, for a file whose content is in effect and that loads have found empty, since when it has been empty without being written, its state then (modification time and size, read through any symbolic link, compared only for equality), and when a load first held it back.System.nanoTime()readings, which do not overflow.A rewrite is covered when the gap between truncating and writing is shorter than the retry delay. Not changed: a read that sees YAML cut off between two entries, after
flagValues:, or inside an unquoted value parses as valid YAML and is applied until the next reload; JSON cut off anywhere fails to parse. Go still detects the format from the content; Node chooses by extension case-sensitively, while this change, like .NET, ignores case.The existing file data source
FileDataSourceImpl,FileSynchronizer,FileInitializer,FileDataSourceBase, andFileDataSourceParsing.FileDataExceptionare unchanged fromfeat/overrides.FileDataSourceParsinggains, for the override source: twoFlagFactorymethods that keep document versions;selectForPathandparseForPath(extension and encoding); theNO_DOCUMENTresult that the YAML parser returns for content with no document, which has all-null maps as before;JsonFlagFileParser.parse(Reader); and the two JSON rules above, which the file data source cannot reach because it parses a file as JSON only when it starts with{.parse(InputStream)keeps reading JSON in the platform's default encoding for the file data source. The file data source still requires every configured file to exist, assigns a load version to every entry, expands a value-only entry to an on flag with a single fallthrough variation, reloads on every watch event with no debounce, no retry timer, and no skip, and reports every failure. Two new test classes pin that behavior:FileSynchronizerReloadBehaviorTest(identical rewrite emits another change set, every failed reload is reported and logged as the plain description at error level, no retry without a file change, missing file logged with the path) andFileDataLoadingBehaviorTest(a model-rejected document escapes as aSerializationException, andFileDataException.getDescriptionrequires a cause). The baseline test files pass unmodified.Testing
OverrideFileLoaderTest, on real files rewritten in place (InPlaceWrites), with a test clock that crosses thelongoverflow: the format by extension, empty and non-object JSON, YAML with no document, the encoding (UTF-8, UTF-16LE and UTF-16BE, with and without a byte order mark, a non-ASCII value), the hold at 0.95 R and its end at exactly R, the requested retries, a new write restarting the delay (also through a symbolic link), the per-file bound with one file kept written and with files rewritten in turn or emptied in staggered turns, the bound starting over at a load that holds nothing back and not otherwise, a file read during another file's failure, the fix of a file saved with a mistake or a duplicate key, the first failure reported, the digest covering paths and presence.FileDataReloaderTest(scripted loader): a hold applies nothing, reports nothing, and is retried after the time the loader asks for; a failure arms the normal retry delay;acceptedis called for applied and unchanged results only.FileDataReloaderEmptiedFileTest(the reloader with the real loader on real files): with a test clock and an hour-long retry delay, the hold's end and the requested retry delays on both arming paths; with the real clock, an emptied file held back and the rewrite applied with no empty application in between, a file that stays empty applied by the retry, two quick rewrites, a change to another file while one is empty, a retry finding a second truncation, a file moved between configured paths, the same bytes moved from.yamlto.json.SDK-3246
Opened from termag
Note
Overview
Adds reload infrastructure for the upcoming file-based flag override source:
FileDataReloader(serialized loads, 100ms debounce, 1s failure retry, content-hash skip, last-good-on-failure),FileDataWatcher(directory watches with symbolic-link/ConfigMap handling and missing-directory retry),FileDataPoller(mtime/size polling fallback), andOverrideFileLoader(extension-based JSON/YAML, BOM-aware encoding, merge rules, and hold-back for files emptied during in-place rewrites).FileDataSourceParsinggains override-oriented parsing (selectForPath/parseForPath, stricter empty/non-object JSON, YAMLNO_DOCUMENT) andFlagFactoryhelpers that preserve document versions. The existing file data source is left as-is; new tests lock its reload/error behavior.Coverage is heavy: poller/watcher/reloader unit tests, in-place rewrite scenarios via
InPlaceWrites, and synchronizer behavior regression tests.Reviewed by Cursor Bugbot for commit 68f868e. Bugbot is set up for automated code reviews on this repo. Configure here.