chore: add FDv2 recovery primitives to SourceManager and Conditions - #1856
chore: add FDv2 recovery primitives to SourceManager and Conditions#1856joker23 wants to merge 4 commits into
Conversation
joker23
changed the title
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/browser size report |
|
@launchdarkly/js-client-sdk size report |
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ab34b76. Configure here.
bb71b36 to
b3def90
Compare
b3def90 to
ad07e81
Compare
…nizer itself If the FDv1 fallback synchronizer's own result is flagged fdv1Fallback, don't re-run the fallback machinery -- the SDK is already on FDv1.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fdv2Recovery() { | ||
| synchronizerSlots.forEach((slot) => { | ||
| slot.state = slot.isFDv1Fallback ? 'blocked' : 'available'; | ||
| }); | ||
| synchronizerIndex = -1; | ||
| }, |
There was a problem hiding this comment.
🟡 New recovery path back to the modern data connection is never used, so a fallback stays permanent
A new way to switch back from the legacy fallback connection to the normal one is added (fdv2Recovery() at packages/shared/sdk-client/src/datasource/fdv2/SourceManager.ts:226-231) but nothing in the product code ever calls it, so once the SDK falls back it stays there for the life of the client.
Impact: Customers whose SDK is temporarily directed to the legacy connection never return to the normal one, even after the directive's expiry time has passed.
Why the recovery path is unreachable
A repo-wide search shows fdv2Recovery and the accompanying isCurrentSynchronizerFDv1Fallback getter are referenced only from packages/shared/sdk-client/__tests__/datasource/fdv2/SourceManager.test.ts:475,497 (plus the guard use of the getter at packages/shared/sdk-client/src/datasource/fdv2/FDv2DataSource.ts:137). The orchestrator's recovery branch only calls sourceManager.resetSourceIndex() (packages/shared/sdk-client/src/datasource/fdv2/FDv2DataSource.ts:289-291), which does not unblock the FDv2 slots that fdv1Fallback() blocked. Furthermore, after fdv1Fallback() only one slot remains available, so getConditions() (packages/shared/sdk-client/src/datasource/fdv2/Conditions.ts:213-215) returns an empty group and no recovery timer is ever created. The fdv1FallbackTtlMs value carried on results is likewise never consumed by the orchestrator.
Prompt for agents
SourceManager gains fdv2Recovery(), which blocks FDv1 fallback slots, unblocks non-FDv1 slots and resets the synchronizer index, but no production code path calls it. In FDv2DataSource.runSynchronizers the 'recovery' condition branch only calls resetSourceIndex(), and after fdv1Fallback() there is exactly one available synchronizer so getConditions() returns an empty group and no recovery timer is created at all. The fdv1FallbackTtlMs value that sources attach to results is also never read by the orchestrator. Either wire fdv2Recovery() into a TTL-driven recovery path (e.g. schedule a recovery based on fdv1FallbackTtlMs while running on the FDv1 fallback synchronizer, and call fdv2Recovery() when it fires), or drop the unused API until the recovery feature is implemented.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
this will be handled later in the stack #1875
2 participants
Summary
Adds
SourceManager.fdv2Recovery()(reversesfdv1Fallback()-- blocks FDv1 slots, unblocks non-FDv1 slots, resets the synchronizer index) and a read-onlySourceManager.isCurrentSynchronizerFDv1Fallbackgetter. Also guardshandleFdv1FallbackinFDv2DataSourceagainst re-triggering fallback when the active synchronizer is already the FDv1 fallback slot (regression SDK-2617).Per review discussion (#1856 (comment)), FDv2 recovery after a directed FDv1 fallback is not modeled as a
Conditionhere -- the originally-proposedConditions.createFDv2RecoveryCondition(ttlMs)and'fdv2Recovery'condition type were removed.Conditions are only ever created throughgetConditions(), which produces no conditions at all when one or zero synchronizers are available -- exactly the state a directed FDv1 fallback puts the SDK in -- so aCondition-based recovery attempt would be silently skipped in the case DATASYSTEM v2 section 1.6 requires it to fire unconditionally.fdv2Recovery()/isCurrentSynchronizerFDv1Fallbackremain primitives with no caller in this diff. The actual recovery scheduling (a plain elapsed-time check, not aCondition) is being wired into PR #1858.Note
Overview
Adds
SourceManager.fdv2Recovery()(mirror offdv1Fallback: block FDv1 slots, unblock FDv2, reset synchronizer index) andisCurrentSynchronizerFDv1Fallbackso the orchestrator can tell when the active synchronizer is already on FDv1.handleFdv1FallbackinFDv2DataSourcenow skips re-running fallback when the current synchronizer is an FDv1 fallback slot but still emitsfdv1Fallbackon its results—fixing a loop where FDv2 could be recreated repeatedly (regression SDK-2617).Conditionsgets small robustness/docs tweaks: idempotent fallback timer start on repeatedinterruptedstatuses, and clarified comments onclose()and the synchronizertry/finally.Unit tests cover
fdv2Recovery/isCurrentSynchronizerFDv1Fallbackand the FDv1 re-trigger guard.fdv2Recoveryis not wired into the orchestration loop in this diff (primitives for a follow-up).Reviewed by Cursor Bugbot for commit af8ee0c. Bugbot is set up for automated code reviews on this repo. Configure here.