feat(internal): add reload orchestration with debouncing and failure retry to file data loading by kinyoklion · Pull Request #406 · launchdarkly/go-server-sdk

kinyoklion · 2026-07-07T21:20:50Z

Adds a Reloader to internal/filedata that owns the reload cycle for a set of data files, and routes ldfiledata and ldfiledatav2 through it. This is the robustness layer the file-based override source (OVERRIDE spec) requires, applied to the existing file data sources as well since the same failure modes (partial writes, rename-replace editors, event storms) affect them equally.

What the Reloader provides:

Serialized reloads on a single goroutine, so overlapping change signals can never interleave two loads.
Debouncing (100 ms): bursts of change notifications from a single file edit coalesce into one reload, which also reduces the chance of reading a mid-write file.
All-or-nothing with last-good retention: any read, parse, or merge failure leaves the previously applied data in place (Apply simply isn't called).
Automatic retry (1 s) after a failed reload, so a failure observed while a file was being written recovers even if no further filesystem notification ever arrives.
Skip-unchanged: a reload whose raw file contents are byte-identical to the last applied contents doesn't re-apply, absorbing duplicate notifications and no-op edits.
Log dedup: with automatic retries, a persistently broken file would otherwise log the same error every second; identical repeats are demoted to debug level.

Consumers keep their own success/failure semantics via Apply/OnError callbacks (v1: store init + status; v2: change-set broadcast + status, with read failures still mapped to UNKNOWN and merge failures to INVALID_DATA via the new filedata.ReadError). Sources configured without a reloader factory still load exactly once with no debounce or retry, as before.

Behavior notes for existing file data source users (changelog-worthy): change-driven reloads now settle for ~100 ms before applying; a reload that failed because a file was temporarily unreadable now recovers automatically instead of waiting for the next filesystem event; and byte-identical rewrites no longer re-initialize the store. One v2 test needed to tolerate the retry loop. ldfilewatch's public API is untouched — debouncing lives in the orchestrator so it applies to any trigger source.

Reloader unit tests run with millisecond-scale delays and observe behavior deterministically through the callbacks (coalescing, retry-without-trigger, retry-stops-after-success, skip-unchanged, close semantics), including under -race.

SDK-2654

Note

Medium Risk
Changes runtime flag-loading semantics (timing, retries, fewer re-inits) for file-watched sources; lifecycle edge cases (late callbacks after Close, single-use Sync) need correct consumer handling.

Overview
Introduces internal/filedata.Reloader to own multi-file load/merge cycles and moves ldfiledata / ldfiledatav2 off inline reload() logic onto Apply/OnError callbacks wired to store init, change-set broadcast, and status updates.

Reload behavior (when a reloader factory is configured): ~100ms debounce on change triggers, 1s automatic retry after failures without new events, last-good retention (failed loads skip Apply), skip-unchanged via content hash (with re-apply after recovery even when bytes match), and deduped identical failure logs/OnError. Sources without a watcher still load once with no debounce/retry.

filedata.ReadError replaces v2’s local read error type; ReadFile now reads then parseDocument. v2 additionally rejects second Sync, returns Off (no hang) when the reloader factory fails, and Close stops both the watcher channel and Reloader.

User-visible deltas: change-driven updates settle briefly before apply; transient read/parse failures can self-heal; byte-identical rewrites no longer re-init the store.

^{Reviewed by Cursor Bugbot for commit 18f1d13. Bugbot is set up for automated code reviews on this repo. Configure here.}

…ledata

…retry to file data loading

cursor

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 18fc662. Configure here.}

…ning Creates the reloader close channel up front so Close never races a concurrent Sync/Start assigning it (an explicit flag now gates the change-driven reload log instead); makes the change-set version counter atomic since loads can run concurrently from Fetch and the reloader; adds regression tests proving Close closes the channel given to the reloader; and documents MergeResult's ordering guarantee precisely (deterministic at document granularity only).

…iledata-reloader # Conflicts: # ldfiledata/file_data_source_impl.go # ldfiledatav2/file_data_source_impl.go

Close no longer waits for an in-flight reload, so a read wedged on unresponsive storage cannot wedge shutdown; a reload instead re-checks the closed state before delivering through Apply/OnError. OnError now fires once per distinct failure rather than on every automatic retry, sparing status listeners a broadcast per second for a permanently broken file. The Reloader is created in the file data source constructors, so a repeated Sync cannot orphan an earlier instance and Close never races an assignment. Also: Apply nil-guarded like OnError; a failed synchronous ReloadNow arms the retry timer directly instead of routing through the change-trigger path (whose log line claimed a change was detected); zero-vs-negative delay semantics documented; the read-error message built in one place; debounce test timing ratio widened.

kinyoklion · 2026-07-08T20:29:34Z

Addressed the multi-agent review findings in 41f0f21:

Finding 1 (Close wedged by a blocked read): Close no longer waits for an in-flight reload — it closes the stop channel and returns, restoring the pre-feat(internal): add reload orchestration with debouncing and failure retry to file data loading #406 shutdown guarantee. A reload that was mid-read when Close happened re-checks the closed state before delivering through Apply/OnError, so a reload that starts after Close still never calls back; one already past that check may finish delivering, as before this PR. Regression test: Close returns while a reload is parked inside Apply.
Finding 2 (per-retry OnError spam): OnError now fires once per distinct failure, gated by the same identical-error condition as the log demotion; a success re-arms it. Status listeners see one report per failure instead of one per second. Test asserts a single OnError and a single error-level log line across a stretch of identical retries, a new report on a different error, and re-arming after recovery.
Findings 3+4 (re-Sync leak, unsynchronized fs.reloader): the Reloader is created in the data source constructors (both packages), like closeReloaderCh — no assignment in Sync/Start to race or to orphan a prior instance.
Finding 5: Apply nil-guarded like OnError.
Finding 11: a failed synchronous ReloadNow now arms the retry timer directly instead of routing through the change-trigger path, so the "detected a change" log no longer fires for it (and finding 2's dedup keeps the retries quiet).
Findings 9, 12, 13: zero/negative delay semantics documented; a comment explains why the un-drained timer Stop+Reset is safe on this module's Go version; the read-error message is built in one place.
Finding 7: the debounce-coalescing test's settle window is now 20× the trigger burst rather than ~1×.
Finding 10 resolved structurally by the finding-1 change (a callback can no longer deadlock by calling Close), though the callback docs still say not to.

Finding 6 left as-is (unreachable today; not worth complicating the Apply seam), finding 8 covered by the new dedup test's log assertion, finding 14 pre-existing/out of scope. Propagated through #407 and the rlamb/overrides integration branch; all file-machinery tests pass under -race and the flag-overrides contract suite passes end-to-end.

…ent is unchanged With SkipUnchanged, a success whose bytes matched the last good hash returned without calling Apply. If a failure had intervened, the consumer had heard OnError and moved to an interrupted state, and the skipped Apply meant it never heard about the recovery: both file data sources would report Interrupted indefinitely after a transient read failure whose retry found byte-identical content. The first success after a failure now always applies.

Creating the Reloader in the data source constructors meant its worker goroutine started at build time, and a source built as a one-shot initializer -- an interface with no Close that the data system never tears down -- leaked that goroutine for the life of the process, once per build. The worker now starts on the first ReloadNow or Trigger, which the initializer path never calls. Also guards the v2 synchronizer against a repeated Sync (which would accumulate watchers and listeners) with a buffered rejection so the guard itself cannot block, and notes that the safety of a post-Close status broadcast rests on the Broadcaster's lock discipline.

kinyoklion · 2026-07-08T21:02:12Z

Round-2 finding N1 addressed in 54aeb92:

N1 (goroutine leak per initializer build): NewReloader no longer starts the worker goroutine at construction; it starts on the first ReloadNow or Trigger. The initializer path only ever calls Fetch, which never touches the reloader, so building a file data source via AsInitializer() — an interface with no Close that the data system never tears down — now spawns nothing. Regression tests at both levels: constructing unused Reloaders accumulates no goroutines, and a loop of initializer Build+Fetch cycles in ldfiledatav2 holds the goroutine count flat. A Close racing the first use is benign: ensureStarted skips a closed reloader, and a worker that does start exits immediately on the closed channel.
R2 (repeated v2 Sync accumulation): Sync is now single-use, guarded before any listener or watcher registration; a repeat (or Sync after Close) gets an Off result on a buffered channel, which also removes the pre-existing blocking send on that rejection path (round 1's finding 6).
R4: the parked-Apply test now also asserts the worker goroutine exits once the callback releases (polled inline — spawning a polling goroutine would disturb the count being measured).
R5: comment added at v1's Close noting the post-Close no-panic guarantee rests on the Broadcaster's lock discipline.

R1/R3/R6 left as noted (theoretical/unreachable/correct). All file-machinery suites pass under -race, and the flag-overrides contract suite passes end-to-end on the updated integration branch.

…-reloader # Conflicts: # internal/filedata/document.go # ldfiledata/file_data_source_impl.go # ldfiledatav2/file_data_source_impl.go # ldfiledatav2/file_data_source_test.go

…deadlocking Sync When the reloader factory failed (for example fsnotify hitting inotify instance or watch limits), the v2 synchronizer sent the Off result on the unbuffered result channel before the reader goroutine existed, so Sync never returned: client startup hung for its full timeout with no fallback attempted, and the already-started reload worker leaked. The failure branch now uses the same buffered single-result rejection as the repeated-Sync guard, stops the reload worker, and detaches the listeners it registered. Also adds the missing tests for this branch and the repeated-Sync rejection, and makes two timing-sensitive tests robust (the goroutine-count check now polls for the process-global count to settle, and the debounce test enables SkipUnchanged like production configs to absorb a legitimate tick-vs-trigger race).

kinyoklion · 2026-07-08T22:30:31Z

Round-3 finding N1 addressed in 8893141:

N1 (Sync deadlock on reloader-factory failure): the factory-error branch sent its Off result on the unbuffered result channel before the reader goroutine existed — the same anti-pattern the repeated-Sync guard already fixed, sitting three lines below it. A watcher-setup failure (inotify instance/watch limits) therefore hung Sync, and with it client initialization, with no fallback attempted. The branch now returns the same buffered single-result rejection, stops the already-started reload worker (nothing can ever trigger a reload without a watcher, and its broadcasts had no consumer), and detaches the two listeners it had registered. New test drives Sync with an erroring factory and asserts a prompt Off result on a closed channel.
T1: added the missing repeated-Sync test (second Sync → single Off, closed channel, first Sync unaffected).
T2: the unused-Reloader goroutine check now polls for the process-global count to settle instead of asserting one sample (and drops the no-op runtime.GC()); stable at -race -count=20.
T3: the debounce-coalescing fixture enables SkipUnchanged, matching production configs and absorbing the documented tick-vs-trigger extra reload.

Propagated through #407, rlamb/overrides-base, #415/#416, and the rlamb/overrides end-to-end branch; -race -count=20 clean on the reloader suite and the flag-overrides contract suite passes end-to-end.

Enabling SkipUnchanged had made the test vacuous: with identical file contents, even one reload per trigger collapses to a single Apply. SkipUnchanged is off again and the test now counts applies across the settle window, tolerating the one documented extra reload from a debounce tick racing a fresh trigger and failing on anything more. Verified to fail with debouncing disabled.

kinyoklion · 2026-07-08T22:55:03Z

Round 4 came back clean — approved for merge. The one non-blocking note (L1) is addressed in the latest commit: the debounce test had lost its discriminating power (with SkipUnchanged and identical contents, even a broken debounce collapses to one Apply). It now runs with SkipUnchanged off and counts applies across the settle window, tolerating the single documented tick-vs-trigger extra reload and failing on anything more; verified to fail with debouncing disabled and stable at -race -count=20. Propagated through the stack and the integration branches.

I1 noted as a pre-existing design choice (a source whose files loaded but whose watcher failed reports Off rather than serving statically — same intent as base); leaving it as-is for this PR.

tanderson-ld · 2026-07-20T15:00:42Z

 func (fs *fileDataSource) Close() (err error) {
 	fs.closeOnce.Do(func() {
 		close(fs.closeReloaderCh)
+		fs.reloader.Close()


Seeing these two lines next to each other looks like double duty. Are both necessary now?

close(fs.closeReloaderCh) fs.reloader.Close()

They are two different lifecycles. One closes the watcher and the other the loader/debouncer. Though maybe the watcher part wasn't originally encapsulated in a way as to make that clear.

tanderson-ld · 2026-07-20T15:05:54Z

+		return
+	}
+	r.startOnce.Do(func() {
+		go r.run()


Are you guaranteed r.run() has executed by the time ensureStarted() returns? I recall there being some go runtime versions that did jump to execute the target of the go func and some that did not. Not sure what the current state of that is. Perhaps it doesn't matter either way due to the arrangement.

It doesn't matter due to the channel arrangement. Things that are going to interact with it via a buffered channel, so a message before the select will still be handled once the select happens.

tanderson-ld · 2026-07-20T15:21:09Z

+	// A trigger already queued when Close was called can still reach here; the select in
+	// run() does not prioritize closeCh over triggerCh.
+	if r.isClosed() {
+		return true


Returning true on a skip is a little weird.

Maybe? A closed item isn't a failure and isn't something we would want to retry.

tanderson-ld · 2026-07-20T15:23:21Z

+			return r.fail(&ReadError{Err: err, Path: path})
+		}
+		docs = append(docs, doc)
+	}


Is there a way to do this in a streaming fashion so all file content doesn't have to come into memory at once for the hash?

So the same read buffer is also used for the parsing, and the entire document will need read for that. Also just aside from the face we always want the hash and the read from the same payload to prevent a TOCTOU issue.

tanderson-ld · 2026-07-20T15:25:43Z

+	// Close may have happened while the files were being read; deliver nothing in that case.
+	if r.isClosed() {
+		return true
+	}


Is this check necessary? Or just a statistically good spot to put it after some IO?

It is a longer window during which a close can happen. Its also how the method is documented currently. But it could be changed.

…review questions

kinyoklion added 2 commits July 7, 2026 14:12

refactor(internal): extract shared file data loading into internal/fi…

5433c04

…ledata

feat(internal): add reload orchestration with debouncing and failure …

18fc662

…retry to file data loading

kinyoklion requested a review from a team as a code owner July 7, 2026 21:20

cursor Bot reviewed Jul 7, 2026

View reviewed changes

Comment thread internal/filedata/reloader.go

kinyoklion marked this pull request as draft July 7, 2026 21:27

kinyoklion added 2 commits July 8, 2026 10:17

Merge branch 'rlamb/sdk-2654/internal-filedata' into rlamb/sdk-2654/f…

4baff2a

…iledata-reloader # Conflicts: # ldfiledata/file_data_source_impl.go # ldfiledatav2/file_data_source_impl.go

kinyoklion mentioned this pull request Jul 8, 2026

refactor(internal): extract shared file data loading into internal/filedata #405

Merged

Base automatically changed from rlamb/sdk-2654/internal-filedata to v7 July 8, 2026 19:38

kinyoklion added 2 commits July 8, 2026 13:37

kinyoklion added 2 commits July 8, 2026 14:50

Merge remote-tracking branch 'origin/v7' into rlamb/sdk-2654/filedata…

f1f468e

…-reloader # Conflicts: # internal/filedata/document.go # ldfiledata/file_data_source_impl.go # ldfiledatav2/file_data_source_impl.go # ldfiledatav2/file_data_source_test.go

kinyoklion marked this pull request as ready for review July 8, 2026 22:56

tanderson-ld reviewed Jul 20, 2026

View reviewed changes

docs: clarify reloader lifecycle and reload semantics in response to …

18f1d13

…review questions

tanderson-ld approved these changes Jul 23, 2026

View reviewed changes

Uh oh!

Conversation

kinyoklion commented Jul 7, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kinyoklion commented Jul 8, 2026

Uh oh!

kinyoklion commented Jul 8, 2026

Uh oh!

kinyoklion commented Jul 8, 2026

Uh oh!

kinyoklion commented Jul 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kinyoklion commented Jul 7, 2026 •

edited by cursor Bot

Loading