Skip to content

chore: Add initialization tracking for FDv2 Data Source.#205

Merged
kinyoklion merged 91 commits intomainfrom
rlamb/combined-test-with-fdv1-fallback
Dec 18, 2025
Merged

chore: Add initialization tracking for FDv2 Data Source.#205
kinyoklion merged 91 commits intomainfrom
rlamb/combined-test-with-fdv1-fallback

Conversation

@kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Dec 17, 2025

Note

Introduces initialization tracking for FDv2 with observer-driven composites, wraps the data source to complete on initialization, and updates polling/unrecoverable error behavior and tests.

  • FDv2 core:
    • Add InitializationTracker, InitializationObserver, and CompositeObserver to aggregate signals from initializers/synchronizers and determine init completion.
    • Return a new wrapper CompletingDataSource from FDv2DataSource.CreateFDv2DataSource that completes Start() based on aggregated initialization rather than inner source start.
    • Refactor FDv2DataSource into Internal/FDv2DataSources namespace; replace direct applier wiring with observer composition; add DataSourceCategory classification.
  • Composite/Composition:
    • Integrate observers into CompositeSource usage; propagate status through ObservableDataSourceUpdates; continue to shut down with Off and exhaustion error when sources are depleted.
  • Polling data source:
    • On unrecoverable HTTP errors (e.g., 401), set init task result to false and shut down; continue to mark Valid on 304 responses.
  • Data system:
    • Wire FDv2DataSystem to new Internal.FDv2DataSources types.
  • Tests/contract:
    • Update unit tests to expect Start() result false on unrecoverable errors and true when no sources; adjust expectations for initialization/fallback flows.
    • Contract test suppressions trimmed; keep streaming/fdv2/disconnects on goodbye.

Written by Cursor Bugbot for commit bba9848. This will update automatically on new commits. Configure here.

/// <summary>
/// We have received signals that indicate all fallbacks are exhausted.
/// </summary>
FallbackExhausted,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this reason this has to be tracked separately from Synchronizers is because the Fallback composite has its own FactoryList that exhausts separately from synchronizers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, without it we would end up basically in an orphaned state, or exit too early. Because if it is just a synchronizers, then the synchronizers would be exhausted twice.

The other thing is that each state is contingent on available states. Like if we don't fallback, we cannot wait for fallback to be exhausted. Or if we don't have synchronizers, etc.

@kinyoklion kinyoklion marked this pull request as ready for review December 18, 2025 16:55
@kinyoklion kinyoklion requested a review from a team as a code owner December 18, 2025 16:55
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@kinyoklion kinyoklion force-pushed the rlamb/combined-test-with-fdv1-fallback branch from c26dec6 to 3777ca2 Compare December 18, 2025 17:22
@kinyoklion kinyoklion force-pushed the rlamb/combined-test-with-fdv1-fallback branch from c65a280 to bba9848 Compare December 18, 2025 20:10
{
_state = State.Failed;
}
}
Copy link

Choose a reason for hiding this comment

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

Bug: Race condition in FDv1 fallback initialization tracking

When synchronizers fail with the FDv1Fallback flag, the CompositeObserver calls FDv1FallbackActionApplier first (which starts fdv1Synchronizers asynchronously), then calls synchronizationObserver second (which immediately sends SynchronizersExhausted). Since FallingBack is only sent when fdv1Synchronizers report Initializing state (asynchronously), SynchronizersExhausted is processed first. In InitializersExhausted state, receiving SynchronizersExhausted calls HandleRemainingSources() which sets state to Failed if both _initializersRemain and _synchronizersRemain are false. The comment at line 122 says "We only consider fallback if we have transitioned to fallback" but the implementation doesn't check the current state, causing premature failure before fdv1Synchronizers can provide data.

Additional Locations (1)

Fix in Cursor Fix in Web

@kinyoklion kinyoklion merged commit 3fa0a29 into main Dec 18, 2025
15 checks passed
@kinyoklion kinyoklion deleted the rlamb/combined-test-with-fdv1-fallback branch December 18, 2025 20:39
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

Comments