Skip to content

fix: re-evaluate remotely on every context change and discard stale responses - #186

Closed
vazarkevych wants to merge 3 commits into
growthbook:mainfrom
vazarkevych:fix/remote-eval-flow
Closed

vazarkevych wants to merge 3 commits into
growthbook:mainfrom
vazarkevych:fix/remote-eval-flow

Conversation

@vazarkevych

Copy link
Copy Markdown
Collaborator

Summary

Fixes the remote-evaluation flow so a context change always reaches the server, streamed updates work with remote evaluation, and the SDK never settles on a response built for an older state. Split out of #176 (section 2, "Remote-eval flow"); the other parts of #176 follow as separate PRs.

Bugs fixed

  1. Context changes were not sent. setAttributes, setAttributesAsync and setForcedFeatures did not trigger a new evaluation, so the server kept evaluating the previous inputs. They now do; setAttributesAsync awaits it. (setAttributeOverrides and setForcedVariations already did.)
  2. A change made mid-round was dropped for good. Remote-eval calls joined the request already in flight, so the request carrying the new inputs was never sent. Remote-eval rounds no longer coalesce; plain GETs still do.
  3. A slow older response could win. Each round carries a generation number; a response or failure from a superseded round is ignored and not reported to the refresh handler.
  4. Cached features answered state changes. The cached payload was evaluated for earlier inputs, so it is now served only before the first round of a session.
  5. Streaming did not work with remote evaluation. For a remote-eval connection the GrowthBook Proxy sends features-updated, a payload-less "please refetch" event, which the SDK ignored; a features event, had one arrived, would have replaced the personalized features with the unevaluated payload. DioClient now reports features-updated as an empty payload, and in remote-eval mode any stream event triggers a fresh POST instead of being applied, as feature-repository.ts does in the JS SDK. Without remote evaluation, features-updated triggers a GET.
  6. A client that threw while sending was swallowed. It is now reported as a fetch failure.

The payload is built by a provider at the start of each round (RemoteEvalRequestProvider), so every request carries the inputs as they are at that moment.

Compatibility

  • No public API is removed. FeatureViewModel.fetchFeatures(remoteEval:, payload:) is deprecated but still works for callers that construct the view model themselves.
  • BaseClient.consumeSseConnections now documents that a features-updated event is reported as an empty map. Custom clients that never report it keep working as before.
  • Behaviour changes:
    • In remote-eval mode, setAttributes and setForcedFeatures now issue a POST. Rapid changes mean more requests; superseded responses are discarded.
    • Without remote evaluation, a stream event without features now triggers a GET instead of being logged and dropped.

Test plan

  • flutter test: 357 tests. 16 new:
    • The 8 SDK-level tests for the context-change, overlap and streaming fixes fail on main and pass here. The 2 control tests (remote evaluation off) pass on both.
    • The view-model tests for the cache-on-first-round and throwing-client fixes, and the features-updated tests for DioClient and for the non-remote-eval refetch, each fail when the corresponding fix is reverted.
  • caching_manager_test is flaky on main as well (shared temp cache directory); feat: add CacheStorage abstraction with per-connection namespace and custom cache directory #185 isolates the test caches.
  • dart analyze --fatal-infos and dart format --set-exit-if-changed: clean.
  • Manual check against a GrowthBook Proxy (remote evaluation enabled, backgroundSync: true) with the example app on the iOS simulator:
    • changing attributes switched an experiment value (6 → 3);
    • setForcedFeatures applied the forced value;
    • publishing a feature change in GrowthBook delivered the new value through features-updated → POST, while the personalized and forced values were kept.
  • Checked against feat: add CacheStorage abstraction with per-connection namespace and custom cache directory #185: one trivial textual conflict in the FeatureViewModel constructor call (both add an argument). The combination passes once FeatureViewModel.manager is optional, which feat: add CacheStorage abstraction with per-connection namespace and custom cache directory #185 now does.

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.

1 participant