feat: Complete initialization when initializers return data without a selector - #441
feat: Complete initialization when initializers return data without a selector#441kinyoklion wants to merge 3 commits into
Conversation
| } | ||
|
|
||
| //nolint:revive // Data system implementation. | ||
| func (f *FDv1) TargetAvailability() DataAvailability { |
There was a problem hiding this comment.
The data system isn't controlling the availability in this case. Just if the data source has been initialized with some data.
If we add the ability to wait for a specific availability, that would potentially move this up a level.
If we did, we would want to differentiate between "cached in a store" and "data without selector".
fcb3ea1 to
db5a2bb
Compare
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 db5a2bb. Configure here.
| } | ||
| if basis != nil && f.applyBasis(*basis, initializer.Name()) { | ||
| f.loggers.Infof("Applied payload from %s before falling back to FDv1", initializer.Name()) | ||
| f.completeInitialization(closeWhenReady) |
There was a problem hiding this comment.
Fallback skips prior initializer success
Medium Severity
A later initializer that requests FDv1 fallback without its own data returns immediately, so the post-loop path that completes initialization from earlier selector-less data never runs. MakeCustomClient then blocks on the FDv1 synchronizer (or times out) even though the store is already populated and dataApplied is already true.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit db5a2bb. Configure here.
| // runInitializers runs each configured initializer in order until one provides a basis with a | ||
| // selector, the context is cancelled, or an initializer signals a fallback to FDv1. A basis | ||
| // without a selector is applied to the store and the loop continues to the next initializer. | ||
| // When all initializers have run, initialization is complete if any initializer's data was | ||
| // applied to the store, even if that data had no selector. A basis that carries no data, or | ||
| // data that cannot be applied, does not count; the synchronizers then decide. | ||
| // Returns (fallbackToFDv1, errorInfo): fallbackToFDv1 is true when an initializer asked the SDK | ||
| // to switch to FDv1; errorInfo describes the underlying error for status reporting when no FDv1 | ||
| // fallback is configured (empty when the fallback accompanied a successful response). If fallback | ||
| // is signalled alongside a basis, that basis is applied before returning so evaluations can serve | ||
| // the server-provided data while the FDv1 synchronizer spins up. |
There was a problem hiding this comment.
Docs nit, no change in content — the information here is all worth keeping, but the form is a bit off from the rest of the repo.
Two things:
-
Returns (fallbackToFDv1, errorInfo):is the only instance of that tuple framing in the repo (grep -rn "Returns ("returns just this line). Since the results are already named in the signature, the house style documents them by name in their own paragraphs — seeMakeClient, which gives each outcome its own short paragraph ("If the connection succeeded, the first return value is..., and the error value is nil."). -
No paragraph breaks. Everything in
internal/datasystemis 2–4 lines, and this block is now 11 in one run. The public API goes much longer than this but always splits with a bare//per outcome. Splitting the three concerns here — loop termination, the readiness rule, and the two results — makes it scannable without cutting anything.
| // runInitializers runs each configured initializer in order until one provides a basis with a | |
| // selector, the context is cancelled, or an initializer signals a fallback to FDv1. A basis | |
| // without a selector is applied to the store and the loop continues to the next initializer. | |
| // When all initializers have run, initialization is complete if any initializer's data was | |
| // applied to the store, even if that data had no selector. A basis that carries no data, or | |
| // data that cannot be applied, does not count; the synchronizers then decide. | |
| // Returns (fallbackToFDv1, errorInfo): fallbackToFDv1 is true when an initializer asked the SDK | |
| // to switch to FDv1; errorInfo describes the underlying error for status reporting when no FDv1 | |
| // fallback is configured (empty when the fallback accompanied a successful response). If fallback | |
| // is signalled alongside a basis, that basis is applied before returning so evaluations can serve | |
| // the server-provided data while the FDv1 synchronizer spins up. | |
| // runInitializers runs each configured initializer in order, stopping early when one provides a | |
| // basis with a selector, the context is cancelled, or an initializer signals a fallback to FDv1. | |
| // A basis without a selector is applied to the store and the loop continues to the next | |
| // initializer. | |
| // | |
| // Once all initializers have run, initialization is complete if any initializer's data was | |
| // applied to the store, even if that data had no selector. A basis that carries no data, or data | |
| // that cannot be applied, does not count; the synchronizers then decide readiness. | |
| // | |
| // fallbackToFDv1 is true when an initializer asked the SDK to switch to FDv1. If the fallback is | |
| // signalled alongside a basis, that basis is applied before returning, so evaluations can serve | |
| // the server-provided data while the FDv1 synchronizer spins up. | |
| // | |
| // errorInfo describes the underlying error for status reporting when no FDv1 fallback is | |
| // configured. It is empty when the fallback accompanied a successful response. |
jsonbailey
left a comment
There was a problem hiding this comment.
I left a nit on the comment, it just needs some paragraph breaks. The cursor comment about fallbacks should be looked at.


Summary
The FDv2 data system now completes initialization once all initializers have run and any of them provided data, even when that data has no selector. Previously only a basis with a selector completed initialization from the initializer phase; selector-less data was applied to the store but readiness waited on a synchronizer.
This follows the updated data system spec: Requirement 1.1.5 (a basis produced by the initialization process initializes the memory store) and Requirement 1.3.11 (an initialized data system drives SDK readiness). Init termination (Requirement 1.1.3) is unchanged: a selector-less basis does not stop the loop early. Java and .NET already behave this way; Go was the outlier.
How readiness is decided:
Store.Apply(internal) now reports whether it applied data. The data system tracks that at the orchestration level, as Java and .NET do. Both the readiness signal andInitializationSucceeded(the internal method the blocking client constructor consults; replacesTargetAvailability) are keyed on "a data source provided data that was applied", so they cannot disagree.noneintent or 304 in reply to a request with no basis); it is reachable from third-party initializers.Initialized()true, but does not make client construction report success when every data source has failed. A configuration with no data sources (daemon mode) is successful as-is, unchanged. FDv1 semantics are unchanged: success still requires the data source to have initialized.Validbefore the readiness signal (parity with Java). A nil constructor error therefore implies aValidstatus.Behavior changes relative to
v7:ErrInitializationFailed.ErrInitializationFailed.Validas soon as initialization completes from initializer data. One consequence: an application that waited forValidto learn that a synchronizer had refreshed selector-less initializer data no longer has that signal; a public data availability API (REFRESHEDvsCACHED, spec Requirement 1.3.5) would be the right way to expose it.Known and deliberately out of scope (pre-existing): daemon mode leaves the data source status at
Initializing;Initialized()counts persistent-store data (Java and .NET do not); the streaming synchronizer's first connection cannot be cancelled byClose()while it is dialing; a delta before any basis is a protocol violation and is not special-cased; the wording of spec Requirement 1.3.11 item 3 (populated persistent store) should be clarified with the spec owners.Validated with the full unit suite, golangci-lint, and contract test harnesses v2 and v3 including the persistence suites against redis, consul, and dynamodb. Each new end-to-end test fails against the behavior it replaces (the
v7base or an earlier revision of this branch) and passes with these changes.