Skip to content

chore: adds connection mode resolution and source factories#265

Open
tanderson-ld wants to merge 9 commits into
mainfrom
ta/SDK-2187/connection-mode-and-resolution
Open

chore: adds connection mode resolution and source factories#265
tanderson-ld wants to merge 9 commits into
mainfrom
ta/SDK-2187/connection-mode-and-resolution

Conversation

@tanderson-ld
Copy link
Copy Markdown
Contributor

@tanderson-ld tanderson-ld commented May 6, 2026

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Note

Medium Risk
Changes core connection-mode selection and public APIs (setMode, new background mode), which can affect when the SDK connects, reports status, and flushes events across lifecycle/network transitions.

Overview
Adds a new ConnectionMode.background plus a resolved mode layer (ResolvedConnectionMode + OfflineDetail) so the SDK can distinguish why it is offline (explicit offline vs network unavailable vs background-disabled) and report the corresponding DataSourceState.

Introduces automatic connection-mode resolution (resolveMode, flutterDefaultResolutionTable) and updates Flutter ConnectionManager to drive ResolvedConnectionMode based on lifecycle/network state, with a nullable override mode and configurable foreground/background slots (including platform-specific background defaults).

Adds FDv2 mode-definition primitives (ModeDefinition, built-in modes) and factory builders (SourceFactoryContext, entry_factories) to construct polling/cache initializers and synchronizers (streaming entries currently throw UnsupportedError). Updates default data source factories to include a background polling configuration, and expands tests to cover the new resolution/factory behavior and updated mode APIs.

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

kinyoklion and others added 6 commits April 29, 2026 14:50
Implements the four building blocks Phase 3 needs from Stream A:

- calculate_poll_delay.dart: pure helper. Given freshness and
  interval, returns the time remaining in the interval; zero when
  overdue; full interval when there's no prior freshness or the
  freshness is in the future (clock skew clamp).
- polling_initializer.dart: one-shot Initializer. Calls the injected
  PollFunction up to 3 times. ChangeSetResult and terminal status
  results return immediately. Interrupted results retry after a 1s
  delay. After 3 interrupted attempts the last is escalated to
  terminalError so the orchestrator stops retrying at this layer.
  close() interrupts a pending retry delay and yields shutdown.
- polling_synchronizer.dart: long-lived Synchronizer. Single-
  subscription StreamController that polls immediately on subscribe,
  then schedules subsequent polls via calculatePollDelay over the
  freshness of the most recent successful result. Interrupted
  results pass through but do not advance freshness, so a transient
  failure does not delay the catch-up poll. Injected TimerFactory
  and now() for deterministic tests.
- cache_initializer.dart: Initializer that reads the persistence
  cache via an injected CachedFlagsReader. Cache hit emits a full
  ChangeSetResult with persist=false and an empty selector (the
  cache does not track server-side selector state). Cache miss or
  reader exception emits a none-type ChangeSetResult so the
  initializer chain advances. The reader typedef leaves the actual
  persistence wiring to the orchestrator (Phase C1).

PollFunction, DelayFunction, TimerFactory, and CachedFlagsReader are
typedefs rather than concrete dependencies so the orchestrator can
wire real implementations and tests can inject scripted ones,
mirroring the abstraction style established in SDK-2183.
- polling_synchronizer: replace `bool _closed` with
  `Completer<void> _stoppedSignal` to match the polling initializer
  and have a single source of truth for closed-ness. Drop the
  redundant `_controller.isClosed` checks (the signal is set before
  the controller is closed, so the signal check covers both cases).
  Replace `Future<void>.microtask(_doPoll)` in onListen with
  `unawaited(_doPoll())` -- the microtask wrapping was unnecessary;
  fire-and-forget of an async function is what unawaited expresses.
- polling_synchronizer: fix typo in the unexpected-throw message:
  "raised unexpectedly" -> "raised error unexpectedly".
- calculate_poll_delay: split the run-on docstring describing the
  null-freshness and overdue-freshness cases into two separate
  sentences for readability.
- cache_initializer: reword the awkward parenthetical
  "(the data came from the cache; writing it back is a no-op)" to
  "(the data is already cached)".
…king branch 'origin' into ta/SDK-2187/connection-mode-and-resolution
@tanderson-ld tanderson-ld requested a review from a team as a code owner May 6, 2026 20:09
Comment thread packages/flutter_client_sdk/lib/src/connection_manager.dart
Comment thread packages/flutter_client_sdk/lib/src/connection_manager.dart Outdated
Comment thread packages/common_client/lib/src/ld_common_client.dart Outdated
@tanderson-ld
Copy link
Copy Markdown
Contributor Author

Need to bench test to confirm no regressions in connection manager wrt event flushing.

Base automatically changed from rlamb/sdk-2184/fdv2-polling-cache-sources to main May 6, 2026 22:00
@tanderson-ld tanderson-ld force-pushed the ta/SDK-2187/connection-mode-and-resolution branch from 2275df1 to 2f1cd6e Compare May 11, 2026 16:08
}
if (!_offline && !inForeground && networkAvailable) {
_destination.flush();
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Background flush skipped when SDK offline flag is set

Medium Severity

The flush condition if (!_offline && !inForeground && networkAvailable) prevents flushing events when the app enters background while the SDK-level _offline flag is true. The old code unconditionally called _destination.flush() at the top of _setBackgroundAvailableMode() regardless of the offline state, ensuring pending events were sent before the app might be killed. With the new !_offline guard, events queued before the offline transition won't be flushed on backgrounding and could be lost if the OS terminates the app.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2f1cd6e. Configure here.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 mode and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

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 3d5e4c8. Configure here.

ConnectionMode get defaultBackgroundConnectionMode =>
Platform.isAndroid || Platform.isIOS || Platform.isFuchsia
? ConnectionMode.background
: ConnectionMode.offline;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Desktop background mode defaults to offline, breaking prior behavior

High Severity

On desktop platforms (Linux, Windows, macOS) where runInBackground is true, defaultBackgroundConnectionMode returns ConnectionMode.offline. The resolution table's _inBackground row resolves this slot, causing the SDK to go offline when backgrounded. Previously, desktop apps stayed connected (streaming) in the background. The PR author confirmed this is a breaking change and stated they would update to use streaming, but the fix was not applied.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 3d5e4c8. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I feel personally attacked. I didn't confirm this breaking change. This needs to be fixed for backwards compatibility sake.

@tanderson-ld
Copy link
Copy Markdown
Contributor Author

Need to investigate broken tests.

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.

2 participants