Skip to content

Commit fe27705

Browse files
committed
fix(common): throttle post-engagement FDv1 retries with backoff
The previous bypass skipped backoff for any LDFlagDeliveryFallbackError, not just the initial server-directed engagement. After _fdv1FallbackEngaged is set, the FDv1 fallback synchronizer can still produce LDFlagDeliveryFallback- Error (e.g. a server that echoes x-ld-fd-fallback on the FDv1 endpoint), and those retries were being scheduled with no delay -- in deployments with multiple FDv1 fallback factories the composite would cycle through them all with zero throttling before exhausting and stopping. Narrow the bypass to the unique case engageFDv1Fallback produces: a 'switchToSync' transition carrying an LDFlagDeliveryFallbackError. Subsequent LDFlagDeliveryFallbackErrors resolve a 'fallback' transition and go through the normal _backoff.fail() path. Adds a regression test that drives one FDv2 sync (engagement) followed by two FDv1 factories that each emit LDFlagDeliveryFallbackError on start, using a tracking Backoff to assert fail() is called twice (once per post-engagement retry).
1 parent 7e2a582 commit fe27705

2 files changed

Lines changed: 102 additions & 3 deletions

File tree

packages/shared/common/__tests__/subsystem/DataSystem/CompositeDataSource.test.ts

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -875,6 +875,98 @@ it('ignores subsequent FDv1 fallback signals once FDv1 fallback is engaged', asy
875875
expect(dataReceived).toContain(fdv1Data);
876876
});
877877

878+
// The backoff bypass applies only to the initial engagement; subsequent
879+
// LDFlagDeliveryFallbackErrors from the FDv1 synchronizer must be throttled.
880+
it('throttles FDv1 retries with backoff after the initial engagement', async () => {
881+
const mockFDv2Sync = {
882+
start: jest
883+
.fn()
884+
.mockImplementation(
885+
(
886+
_dataCallback: (basis: boolean, data: any) => void,
887+
_statusCallback: (status: DataSourceState, err?: any) => void,
888+
) => {
889+
_statusCallback(DataSourceState.Initializing);
890+
_statusCallback(
891+
DataSourceState.Closed,
892+
new LDFlagDeliveryFallbackError(
893+
DataSourceErrorKind.ErrorResponse,
894+
'fallback from FDv2',
895+
500,
896+
),
897+
);
898+
},
899+
),
900+
stop: jest.fn(),
901+
};
902+
903+
// Two FDv1 factories so the composite cycles through 'fallback' transitions instead
904+
// of stopping after the first cull -- each cycle is a chance to (incorrectly) skip
905+
// backoff. The tracking Backoff below counts fail() calls as the observable proxy.
906+
const makeFDv1Sync = () => ({
907+
start: jest
908+
.fn()
909+
.mockImplementation(
910+
(
911+
_dataCallback: (basis: boolean, data: any) => void,
912+
_statusCallback: (status: DataSourceState, err?: any) => void,
913+
) => {
914+
_statusCallback(DataSourceState.Initializing);
915+
_statusCallback(
916+
DataSourceState.Closed,
917+
new LDFlagDeliveryFallbackError(
918+
DataSourceErrorKind.ErrorResponse,
919+
'fallback from FDv1',
920+
500,
921+
),
922+
);
923+
},
924+
),
925+
stop: jest.fn(),
926+
});
927+
const mockFDv1Sync1 = makeFDv1Sync();
928+
const mockFDv1Sync2 = makeFDv1Sync();
929+
930+
let failCalls = 0;
931+
let successCalls = 0;
932+
const trackingBackoff: Backoff = {
933+
success() {
934+
successCalls += 1;
935+
return 0;
936+
},
937+
fail() {
938+
failCalls += 1;
939+
return 0;
940+
},
941+
};
942+
943+
const underTest = new CompositeDataSource(
944+
[],
945+
[makeDataSourceFactory(mockFDv2Sync)],
946+
[makeDataSourceFactory(mockFDv1Sync1), makeDataSourceFactory(mockFDv1Sync2)],
947+
undefined,
948+
makeTestTransitionConditions(),
949+
trackingBackoff,
950+
);
951+
952+
const statusCallback = jest.fn();
953+
await new Promise<void>((resolve) => {
954+
statusCallback.mockImplementation((state: DataSourceState) => {
955+
if (state === DataSourceState.Closed) {
956+
resolve();
957+
}
958+
});
959+
underTest.start(jest.fn(), statusCallback);
960+
});
961+
962+
expect(mockFDv2Sync.start).toHaveBeenCalledTimes(1);
963+
expect(mockFDv1Sync1.start).toHaveBeenCalledTimes(1);
964+
expect(mockFDv1Sync2.start).toHaveBeenCalledTimes(1);
965+
// One fail() per post-engagement retry; the initial engagement is not counted.
966+
expect(failCalls).toBe(2);
967+
expect(successCalls).toBe(0);
968+
});
969+
878970
it('reports error when all initializers fail', async () => {
879971
const mockInitializer1Error = {
880972
name: 'Error',

packages/shared/common/src/datasource/CompositeDataSource.ts

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -246,12 +246,19 @@ export class CompositeDataSource implements DataSource {
246246
// stop the underlying datasource before transitioning to next state
247247
currentDS?.stop();
248248

249+
// Skip backoff only on the initial server-directed FDv1 engagement (a `switchToSync`
250+
// transition resolved by `engageFDv1Fallback` carries an `LDFlagDeliveryFallbackError`).
251+
// Any subsequent `LDFlagDeliveryFallbackError` is an ordinary retry from the FDv1
252+
// synchronizer (or a follow-up FDv1 factory) and must be throttled like any other
253+
// error retry.
254+
const isInitialFDv1Engagement =
255+
transitionRequest.transition === 'switchToSync' &&
256+
transitionRequest.err instanceof LDFlagDeliveryFallbackError;
257+
249258
if (
250259
transitionRequest.err &&
251260
transitionRequest.transition !== 'stop' &&
252-
// The server-directed FDv1 fallback is not an error retry -- the directive should be
253-
// honored immediately so the FDv1 synchronizer can take over without delay.
254-
!(transitionRequest.err instanceof LDFlagDeliveryFallbackError)
261+
!isInitialFDv1Engagement
255262
) {
256263
// if the transition was due to an error we're not in the initializer phase, throttle the transition. Fallback between initializers is not throttled.
257264
const delay = this._initPhaseActive ? 0 : this._backoff.fail();

0 commit comments

Comments
 (0)