Skip to content

feat: Park FDv2 synchronizers instead of retiring them on unexpected failures - #489

Merged
beekld merged 10 commits into
v7from
bklimt/SDK-3303/fdv2-synchronizer-parking
Oct 29, 2026
Merged

beekld merged 10 commits into
v7from
bklimt/SDK-3303/fdv2-synchronizer-parking

Conversation

@beekld

@beekld beekld commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

A synchronizer that fails in a way that will not resolve on retry, such as an HTTP 401, is now parked for a bounded time instead of being retired for the life of the client.

  • Synchronizers report Interrupted with UnexpectedFailure set instead of Off. The data system parks the synchronizer, waits a jittered failover delay of 0.5-1s, and starts the next eligible synchronizer after it, wrapping around the list.
  • The park delay starts at 5 minutes and doubles up to 1 hour, with jitter. The data system keeps the park count for each synchronizer, so it survives a new instance on each start. A synchronizer clears it with ResetExtendedBackoff when it meets its reset threshold: 60s of healthy streaming, or 2 consecutive successful polls.
  • When every synchronizer is parked, the status is Interrupted and the data system waits for the earliest unpark time. Fallback and recovery conditions select from the eligible synchronizers only.
  • A synchronizer that fails to build is parked like any other unexpected failure.
  • A synchronizer raises the readiness signal only when it delivers a full payload.
  • An SDK key change from LDRelayApplySDKKey unparks every synchronizer.

Sorry about the churn in naming ("blocked" -> "parked", "available" -> "eligible", and so on). This work went on in parallel with #466, and while resolving the conflicts I went with the names in the spec.

Note: This does not implement Retry-After or FDv1 Fallback TTL, which are new features that will be added later.


Note

Overview
FDv2 no longer permanently retires synchronizers after errors like HTTP 401. They now report Interrupted with UnexpectedFailure, and the data system parks them with escalating backoff (5 minutes, doubling to 1 hour, with jitter) instead of removing them from rotation. When every synchronizer is parked, status stays Interrupted and the loop waits for the earliest unpark time; Off is reserved for explicit Stop().

Synchronizers can clear park escalation via ResetThresholdMet: polling after two consecutive successful polls, streaming after ~60s of healthy operation (including a quiet stream). The FDv2 loop applies a jittered failover delay after parking, treats recovery before fallback when both conditions match, and signals client readiness only for a full payload (IntentTransferFull). LDRelayApplySDKKey unparks all synchronizers so the Relay can retry rejected keys.

Polling and streaming processors, file data source, and DataSynchronizerResult were updated to emit the new flags and default errors to Interrupted rather than Off.

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

@beekld
beekld marked this pull request as ready for review October 27, 2026 19:24
@beekld
beekld requested a review from a team as a code owner October 27, 2026 19:24

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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 7ac6c1b. Configure here.

Comment thread internal/datasystem/fdv2_datasystem.go Resolved
Comment thread ldfiledatav2/file_data_source_impl.go Resolved
Comment thread subsystems/data_source.go Resolved
@beekld
beekld requested a review from tanderson-ld October 29, 2026 20:58
@beekld
beekld merged commit 644253a into v7 Oct 29, 2026
42 of 43 checks passed
@beekld
beekld deleted the bklimt/SDK-3303/fdv2-synchronizer-parking branch October 29, 2026 21:43
beekld added a commit that referenced this pull request Nov 3, 2026
## Summary

#489 made `FDv2.Stop()` broadcast an `Off`
status after its goroutines exit. A broadcast does a blocking send to
each listener channel, which buffers 10 values. If an application
listener stops reading while its buffer is full, `Stop()` blocks
forever. That hangs `LDClient.Close()`, and `GetStatus()` hangs with it,
because the send ran under the status lock.

The hazard predates #489. Any status update can block the same way while
the data system runs, and the broadcaster holds its lock during the
send. So removing the stuck listener also blocks.

This PR:

- Makes `RemoveListener` and `Close` unblock a send to a full listener.
Listeners that keep reading still get every value, in order. No
broadcast drops events.
- Sends the FDv2 data source status broadcast after the status lock is
released, as FDv1 already does. `GetStatus()` no longer waits on a
listener.
- Stops broadcasting `Off` from `FDv2.Stop()`. The status still changes
to `Off`, and listeners learn of shutdown when their channels close.
- Removes the drain goroutines in the FDv1 and FDv2 `WaitFor` that only
existed to work around the blocking removal.
- Makes `WaitFor` return `false` when its listener channel closes, in
both FDv1 and FDv2. Before, a `WaitFor` still running at `Close()` spun
on the closed channel until its timeout.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Fixes hangs when a status listener’s buffered channel is full and the
app stops reading—previously **`Broadcast`**, **`RemoveListener`**,
**`Close`**, **`FDv2.Stop()`**, and **`GetStatus()`** could block
indefinitely (including **`LDClient.Close()`**).
> 
> The generic **`Broadcaster`** now sends without holding the subscriber
list lock, uses per-subscriber **`done`** signaling so removal/shutdown
aborts in-flight sends, and still delivers every value in order to
listeners that keep reading. **`FDv2.UpdateStatus`** broadcasts only
after the status mutex is released (matching FDv1). **`Stop()`** sets
status to **`Off`** internally but no longer broadcasts
**`Off`**—shutdown is signaled by closing listener channels.
**`WaitFor`** drops the old channel-drain workaround and returns
**`false`** when the listener channel closes (e.g. on stop or
broadcaster close).
> 
> Tests cover full-buffer broadcast/unblock paths and FDv1/FDv2
**`WaitFor`**, **`GetStatus`**, and **`Stop`** under a full listener.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
b5e17cb. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
beekld added a commit that referenced this pull request Nov 3, 2026
## Summary

Since #489, the data system has silently
ignored a synchronizer result with `State: Off`. The result's error was
dropped, the status didn't change, and the data system kept waiting on
that synchronizer. A synchronizer isn't supposed to report `Off`, but
`State` is a public field, and custom synchronizers can set it.

This PR treats an `Off` result as an unexpected failure. The data system
reports `Interrupted` with the result's error and parks the
synchronizer.

It also changes how the data system handles a synchronizer that closes
its result channel on its own. That used to count as a fallback, which
with a single synchronizer restarted it at once with no delay. A
synchronizer that kept ending its results could then restart in a tight
loop. The data system now treats this as an unexpected failure too, and
parks the synchronizer.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Fixes FDv2 synchronizer handling where **`State: Off`** from a
synchronizer was ignored (error dropped, loop kept waiting) and where a
synchronizer **closing its own result channel** was treated like a
normal fallback—often **restarting immediately** with a single
synchronizer and risking a tight loop.
> 
> **Off results** are now treated as an unexpected failure: status
becomes **`Interrupted`** with the result’s error, and the synchronizer
is **parked** (same path as other unexpected failures).
> 
> **A closed result channel** is also an unexpected failure: status
**`Interrupted`** with a fixed unknown error message, then
**`syncPark`** (or **`syncRestart`** if the SDK key changed mid-run).
Tests cover both behaviors.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
59492c2. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
beekld added a commit that referenced this pull request Nov 3, 2026
…or (#501)

## Summary

#489 changed the ERROR log for an
unrecoverable HTTP status, such as a 401, from "giving up permanently"
to "will not retry until restarted." The synchronizer does stop, but the
data system parks it and starts it again after a delay. An operator
could read "restarted" as "restart the process," which also resets the
park backoff. The message now says that the data system will retry the
synchronizer after a delay.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Updates the **ERROR** log emitted when an HTTP status is treated as
unrecoverable (e.g. 401/403) in `checkIfErrorIsRecoverableAndLog`. The
suffix changes from implying the synchronizer **will not retry until
restarted** to stating that **the data system will retry it after a
delay**, matching how the data system parks and restarts the
synchronizer rather than requiring a process restart.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
6d01d25. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
beekld added a commit that referenced this pull request Nov 3, 2026
## Summary

#489 reset the public
`DataSourceStatus.StateSince` to the current time whenever a
synchronizer started, even when `State` didn't change, and without
broadcasting the new status. `StateSince` is documented as the time
`State` most recently changed. An app that measures how long the SDK has
been `Interrupted` had its clock reset on every failover, and status
listeners held a different `StateSince` than `GetStatus()`.

The reset existed so that each synchronizer gets its own fallback and
recovery window. The data system now records when the current
synchronizer started in a private field. The fallback and recovery
conditions measure from the later of that time and `StateSince`, so
their timing is unchanged. `StateSince` again changes only when `State`
changes.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> **Stops resetting public `DataSourceStatus.StateSince` when a new
synchronizer starts** without a state change. Previously, starting the
next synchronizer after failover rewrote `StateSince` to “now” (often
without broadcasting), which broke apps timing how long the SDK stayed
`Interrupted` and could disagree with status listeners.
> 
> **Fallback and recovery timing is preserved** by recording
**`syncStartedAt`** when the active synchronizer starts and evaluating
**`fallbackCond` / `recoveryCond`** against the **later** of
`StateSince` and that start time inside **`conditionAction`**.
**`restartStatusClock`** is removed; **`StateSince` is updated only when
`State` changes** in **`maybeUpdateStatus`**.
> 
> A new test asserts **`StateSince` is unchanged** when the secondary
synchronizer starts after the primary fails.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
06f2eda. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
beekld added a commit that referenced this pull request Nov 3, 2026
…ps (#505)

## Summary

**Behavior change:** compared with the released 7.21.2, FDv2 status
listeners that are still reading now receive an `Off` status when the
client closes. Their channels then close as before. 7.21.2 neither
broadcast `Off` nor set it on shutdown. #489
added a blocking broadcast that could hang `Close()`, and
#498 removed it.

This PR adds `Broadcaster.CloseWith`, which sends a final value to each
listener that has room for it in its buffer and then closes every
listener channel. It never waits for a listener to read, so a listener
that has stopped reading still can't block `Close()`. That listener
misses `Off` and only sees its channel close.

The data system uses it in `Stop()` to send the `Off` status. As a
result, a `WaitFor(DataSourceStateOff, ...)` call that's running when
the client closes now returns `true`.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Adds **`Broadcaster.CloseWith`**, which non-blockingly pushes one
final value into each subscriber channel that still has buffer space,
then closes all listener channels. **`Close()`** is unchanged in
behavior and routes through shared **`closeAll(nil)`** logic;
**`RemoveListener`** still closes without a final value.
> 
> **FDv2 `Stop()`** now records **`DataSourceStateOff`** via
**`maybeUpdateStatus`** and shuts down broadcasters with
**`Close(status)`**, so data-source status listeners that are still
reading typically receive **`Off`** before their channel closes.
**`WaitFor(DataSourceStateOff, …)`** can therefore return **`true`**
when the client stops instead of timing out or failing only on channel
close.
> 
> Listeners whose buffers are full still do not block shutdown—the final
**`Off`** is dropped for them (same non-waiting guarantee as
**`CloseWith`**). Tests cover broadcaster **`CloseWith`** semantics and
FDv2 stop / **`WaitFor(Off)`** behavior.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
0ec9157. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
beekld pushed a commit that referenced this pull request Nov 6, 2026
🤖 I have created a release *beep* *boop*
---


##
[7.22.0](v7.21.2...v7.22.0)
(2026-11-06)


### Features

* Broadcast Off to FDv2 status listeners when the data system stops
([#505](#505))
([5c5b8ce](5c5b8ce))
* End an FDv1 fallback after a default TTL of one hour
([#499](#499))
([1f03a5f](1f03a5f))
* Park FDv2 synchronizers instead of retiring them on unexpected
failures
([#489](#489))
([644253a](644253a))


### Bug Fixes

* Keep data from a store that is not initialized out of override
evaluations and AllFlagsState, and report a store failure
([#491](#491))
([1005a1a](1005a1a))
* Keep the override marking when a migration flag serves an unrecognized
stage ([#492](#492))
([9f9911f](9f9911f))
* Leave StateSince alone when a synchronizer starts
([#502](#502))
([823b9aa](823b9aa))
* Make TestData ValueForAll return its value when targeting is off
([#507](#507))
([7300bc1](7300bc1))
* Park a synchronizer that reports Off or closes its results
([#500](#500))
([f6e9910](f6e9910))
* Poll the override files when they cannot be watched
([#495](#495))
([170ad3b](170ad3b))
* Prevent a full status listener from hanging Close
([#498](#498))
([9efd570](9efd570))
* Say that the data system retries after an unrecoverable HTTP error
([#501](#501))
([d4066cc](d4066cc))
* Treat a repeated override file path in a different case as a repeat on
Windows
([#494](#494))
([eeb68d3](eeb68d3))
* Warn when offline mode skips the override source
([#493](#493))
([69bbe61](69bbe61))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> **Cuts the Go server SDK release `7.22.0` and records its notes.**
> 
> The root **`.release-please-manifest.json`** and
**`internal/version.go`** now declare **`7.22.0`** (replacing `7.21.2`).
**`CHANGELOG.md`** gains a new **`[7.22.0]`** section dated
**2026-11-06** documenting shipped work: FDv2 data-system behavior
(status **Off** broadcasts, synchronizer **parking** instead of retire,
**FDv1 fallback TTL**, **StateSince** handling, parking on
**Off**/closed results), plus multiple **bug fixes** around
uninitialized store data in overrides/**AllFlagsState**, migration
override marking, **TestData** when targeting is off, **override file**
polling/deduping on Windows, offline-mode warnings, status listener
**Close** hangs, and messaging for unrecoverable HTTP retries.
> 
> No application code changes appear in this PR; it’s version
bookkeeping and release notes only.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
c763cc0. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants