chore: pr comments

Author: joker23

packages/shared/sdk-client/__tests__/datasource/fdv2/Conditions.test.ts

@@ -33,12 +33,15 @@ function raceTimeout<T>(promise: Promise<T>, ms: number): Promise<T | typeof DID
 function makeInterrupted(): FDv2SourceResult {
   return interrupted(
     { kind: DataSourceErrorKind.NetworkError, message: 'test error', time: Date.now() },
-    false,
+    { fdv1Fallback: false },
   );
 }
 
 function makeChangeSet(): FDv2SourceResult {
-  return changeSet({ version: 1, state: 'test-state', type: 'full', updates: [] }, false);
+  return changeSet(
+    { version: 1, state: 'test-state', type: 'full', updates: [] },
+    { fdv1Fallback: false },
+  );
 }
 
 // -- fallback condition --
@@ -101,7 +104,7 @@ it('fallback condition does not start timer on terminal error', async () => {
   condition.inform(
     terminalError(
       { kind: DataSourceErrorKind.ErrorResponse, message: 'unauthorized', time: Date.now() },
-      false,
+      { fdv1Fallback: false },
     ),
   );
   expect(await raceTimeout(condition.promise, 50)).toBe(DID_NOT_RESOLVE);
@@ -117,14 +120,14 @@ it('fallback condition does not start timer on shutdown', async () => {
 
 it('fallback condition does not start timer on goodbye', async () => {
   const condition = createFallbackCondition(10);
-  condition.inform(goodbye('server-requested', false));
+  condition.inform(goodbye('server-requested', { fdv1Fallback: false }));
   expect(await raceTimeout(condition.promise, 50)).toBe(DID_NOT_RESOLVE);
   condition.close();
 });
 
 it('fallback condition changeSet without active timer does not cause issues', async () => {
   const condition = createFallbackCondition(10);
-  // changeSet without a prior interrupted — should be safe
+  // changeSet without a prior interrupted, should be safe
   condition.inform(makeChangeSet());
   expect(await raceTimeout(condition.promise, 50)).toBe(DID_NOT_RESOLVE);
   condition.close();
@@ -165,7 +168,7 @@ it('recovery condition does not fire after close', async () => {
 it('recovery condition close after timer fires does not cause error', async () => {
   const condition = createRecoveryCondition(10);
   expect(await condition.promise).toBe('recovery');
-  // Close after timer already fired — should not throw
+  // Close after timer already fired, should not throw
   condition.close();
 });
 

packages/shared/sdk-client/__tests__/datasource/fdv2/FDv2DataSource.test.ts

@@ -20,7 +20,7 @@ it('creates a changeSet result with a payload', () => {
     type: 'full' as const,
     updates: [],
   };
-  const result = changeSet(payload, false, 'env-123');
+  const result = changeSet(payload, { fdv1Fallback: false }, 'env-123');
 
   expect(result.type).toBe('changeSet');
   expect(result).toEqual({
@@ -33,7 +33,7 @@ it('creates a changeSet result with a payload', () => {
 
 it('creates a changeSet result with fdv1Fallback flag', () => {
   const payload = { version: 1, type: 'full' as const, updates: [] };
-  const result = changeSet(payload, true);
+  const result = changeSet(payload, { fdv1Fallback: true });
 
   expect(result.type).toBe('changeSet');
   if (result.type === 'changeSet') {
@@ -47,7 +47,7 @@ it('creates an interrupted status result', () => {
     message: 'connection reset',
     time: 1000,
   };
-  const result = interrupted(errorInfo, false);
+  const result = interrupted(errorInfo, { fdv1Fallback: false });
 
   expect(result).toEqual({
     type: 'status',
@@ -74,7 +74,7 @@ it('creates a terminal error status result', () => {
     statusCode: 401,
     time: 2000,
   };
-  const result = terminalError(errorInfo, true);
+  const result = terminalError(errorInfo, { fdv1Fallback: true });
 
   expect(result).toEqual({
     type: 'status',
@@ -85,7 +85,7 @@ it('creates a terminal error status result', () => {
 });
 
 it('creates a goodbye status result', () => {
-  const result = goodbye('server-shutdown', false);
+  const result = goodbye('server-shutdown', { fdv1Fallback: false });
 
   expect(result).toEqual({
     type: 'status',
@@ -96,7 +96,7 @@ it('creates a goodbye status result', () => {
 });
 
 it('creates a goodbye status result with fdv1Fallback and a TTL', () => {
-  const result = goodbye('server-shutdown', true, 5000);
+  const result = goodbye('server-shutdown', { fdv1Fallback: true, fdv1FallbackTtlMs: 5000 });
 
   expect(result).toEqual({
     type: 'status',
@@ -108,7 +108,7 @@ it('creates a goodbye status result with fdv1Fallback and a TTL', () => {
 });
 
 it('creates a goodbye status result with TTL 0 (indefinite fallback)', () => {
-  const result = goodbye('server-shutdown', true, 0);
+  const result = goodbye('server-shutdown', { fdv1Fallback: true, fdv1FallbackTtlMs: 0 });
 
   expect(result).toEqual({
     type: 'status',

packages/shared/sdk-client/__tests__/datasource/fdv2/FDv2SourceResult.test.ts

@@ -608,6 +608,37 @@ it('emits terminal_error for a goodbye when a deferred open directive is pending
   base.close();
 });
 
+it('prefers the in-band goodbye directive TTL over a pending onopen-deferred TTL', async () => {
+  // When onopen defers a fallback directive AND the goodbye event itself carries
+  // its own protocolFallbackTTL, the in-band directive wins: it uses its own TTL
+  // (not the pending one) and still clears the pending state.
+  const mockEventSource = createMockEventSource();
+  const mockRequests = createMockRequests(mockEventSource);
+  const base = createBase(mockRequests, logger);
+  base.start();
+
+  // Open with fallback headers (defers a directive with TTL 45s)
+  mockEventSource.onopen({
+    type: 'open',
+    headers: { 'x-ld-fd-fallback': 'true', 'x-ld-fd-fallback-ttl': '45' },
+  });
+
+  // Goodbye fires with its own, different TTL (60s) before any payload
+  simulateEvent(mockEventSource, 'goodbye', {
+    reason: 'falling back',
+    protocolFallbackTTL: 60,
+  });
+
+  const result = await base.takeResult();
+  expect(result.type).toBe('status');
+  if (result.type !== 'status') return;
+  expect(result.state).toBe('terminal_error');
+  expect(result.fdv1Fallback).toBe(true);
+  expect(result.fdv1FallbackTtlMs).toBe(60000);
+
+  base.close();
+});
+
 it('clears a pending fallback directive when onopen fires without the fallback header', async () => {
   const mockEventSource = createMockEventSource();
   const mockRequests = createMockRequests(mockEventSource);
@@ -854,6 +885,40 @@ it('backfills the deferred TTL into a ping-triggered poll result that already si
   base.close();
 });
 
+it('does not mutate the ping handler result object when applying a deferred fallback', async () => {
+  const mockEventSource = createMockEventSource();
+  const mockRequests = createMockRequests(mockEventSource);
+  const pingResult = {
+    type: 'changeSet' as const,
+    payload: { events: [], selector: undefined },
+    fdv1Fallback: false,
+  };
+  const pingHandler: PingHandler = {
+    handlePing: jest.fn().mockResolvedValue(pingResult),
+  };
+  const base = createBase(mockRequests, logger, { pingHandler });
+  base.start();
+
+  mockEventSource.onopen({
+    type: 'open',
+    headers: { 'x-ld-fd-fallback': 'true', 'x-ld-fd-fallback-ttl': '75' },
+  });
+
+  const { calls } = mockEventSource.addEventListener.mock;
+  const pingListener = calls.find((c: any[]) => c[0] === 'ping')?.[1];
+  await pingListener();
+
+  const result = await base.takeResult();
+  // The queued result carries the deferred fallback...
+  expect(result.fdv1Fallback).toBe(true);
+  expect(result.fdv1FallbackTtlMs).toBe(75000);
+  // ...but the object handed back by the ping handler is left untouched.
+  expect(pingResult.fdv1Fallback).toBe(false);
+  expect((pingResult as any).fdv1FallbackTtlMs).toBeUndefined();
+
+  base.close();
+});
+
 it('surfaces a deferred fallback directive when a ping-triggered poll throws', async () => {
   const mockEventSource = createMockEventSource();
   const mockRequests = createMockRequests(mockEventSource);

packages/shared/sdk-client/__tests__/datasource/fdv2/StreamingFDv2Base.test.ts

@@ -43,13 +43,13 @@ async function loadFromCache(config: CacheInitializerConfig): Promise<FDv2Source
 
   if (!storage) {
     logger?.debug('No storage available for cache initializer');
-    return changeSet({ version: 0, type: 'none', updates: [] }, false);
+    return changeSet({ version: 0, type: 'none', updates: [] }, { fdv1Fallback: false });
   }
 
   const cached = await loadCachedFlags(storage, crypto, environmentNamespace, context, logger);
   if (!cached) {
     logger?.debug('Cache miss for context');
-    return changeSet({ version: 0, type: 'none', updates: [] }, false);
+    return changeSet({ version: 0, type: 'none', updates: [] }, { fdv1Fallback: false });
   }
 
   const updates: internal.Update[] = Object.entries(cached.flags).map(
@@ -72,7 +72,7 @@ async function loadFromCache(config: CacheInitializerConfig): Promise<FDv2Source
   const freshness = await readFreshness(storage, crypto, environmentNamespace, context, logger);
 
   logger?.debug('Loaded cached flag evaluations via cache initializer');
-  return changeSet(payload, false, undefined, freshness);
+  return changeSet(payload, { fdv1Fallback: false }, undefined, freshness);
 }
 
 /**
@@ -91,7 +91,7 @@ async function loadFromCache(config: CacheInitializerConfig): Promise<FDv2Source
 export function createCacheInitializerFactory(config: CacheInitializerConfig): InitializerFactory {
   return {
     isCache: true,
-    // The selectorGetter is ignored -- cache data has no selector.
+    // The selectorGetter is ignored: cache data has no selector.
     create(_selectorGetter: () => string | undefined): Initializer {
       let shutdownResolve: ((result: FDv2SourceResult) => void) | undefined;
       const shutdownPromise = new Promise<FDv2SourceResult>((resolve) => {

packages/shared/sdk-client/src/datasource/fdv2/CacheInitializer.ts

@@ -24,6 +24,7 @@ function flagsToPayload(flags: Flags): internal.Payload {
   const updates: internal.Update[] = Object.entries(flags).map(([key, flag]) => ({
     kind: 'flag-eval',
     key,
+    // The envelope requires a numeric version; default to 1 if FDv1 omits it
     version: flag.version ?? 1,
     object: flag,
   }));
@@ -69,6 +70,8 @@ export function createFDv1PollingSynchronizer(
 
   function scheduleNextPoll(startTime: number): void {
     if (!stopped) {
+      // Subtract time already spent on the request so the poll cadence stays
+      // fixed to pollIntervalMs instead of drifting later with each slow response.
       const elapsed = Date.now() - startTime;
       const sleepFor = Math.min(Math.max(pollIntervalMs - elapsed, 0), pollIntervalMs);
       // eslint-disable-next-line @typescript-eslint/no-use-before-define
@@ -84,6 +87,9 @@ export function createFDv1PollingSynchronizer(
     logger?.debug('Polling FDv1 endpoint for feature flag updates');
     const startTime = Date.now();
 
+    // Results below always carry fdv1Fallback: false; this synchronizer only
+    // runs once already on the FDv1 fallback path, so there's no further
+    // fallback for it to signal.
     try {
       const body = await requestor.requestPayload();
 
@@ -107,7 +113,7 @@ export function createFDv1PollingSynchronizer(
         return;
       }
 
-      resultQueue.put(changeSet(payload, false));
+      resultQueue.put(changeSet(payload, { fdv1Fallback: false }));
     } catch (err) {
       if (stopped) {
         return;
@@ -118,7 +124,9 @@ export function createFDv1PollingSynchronizer(
         if (!isHttpRecoverable(requestError.status)) {
           logger?.error(httpErrorMessage(err as HttpErrorResponse, 'FDv1 polling request'));
           stopped = true;
-          shutdownResolve?.(terminalError(errorInfoFromHttpError(requestError.status), false));
+          shutdownResolve?.(
+            terminalError(errorInfoFromHttpError(requestError.status), { fdv1Fallback: false }),
+          );
           shutdownResolve = undefined;
           return;
         }

packages/shared/sdk-client/src/datasource/fdv2/FDv1PollingSynchronizer.ts

@@ -1,14 +1,17 @@
 import { DataSourceErrorKind, internal } from '@launchdarkly/js-sdk-common';
 
 import DataSourceStatusErrorInfo from '../DataSourceStatusErrorInfo';
+import { FallbackDirective } from './fallbackDirective';
 
 /**
  * Possible states for a status result from an FDv2 data source.
  *
  * - `interrupted`: Transient error; synchronizer will retry automatically.
  * - `shutdown`: Graceful shutdown; no further results will be produced.
  * - `terminal_error`: Unrecoverable error; no further results will be produced.
- * - `goodbye`: Server-initiated disconnect; no further results will be produced.
+ * - `goodbye`: Server-initiated disconnect. Synchronizers reconnect internally
+ *   and may still produce results; initializers are single-shot, so the
+ *   orchestrator just moves on to the next one.
  */
 export type SourceState = 'interrupted' | 'shutdown' | 'terminal_error' | 'goodbye';
 
@@ -61,12 +64,18 @@ export type FDv2SourceResult = ChangeSetResult | StatusResult;
  */
 export function changeSet(
   payload: internal.Payload,
-  fdv1Fallback: boolean,
+  fallback: FallbackDirective,
   environmentId?: string,
   freshness?: number,
-  fdv1FallbackTtlMs?: number,
 ): FDv2SourceResult {
-  return { type: 'changeSet', payload, fdv1Fallback, environmentId, freshness, fdv1FallbackTtlMs };
+  return {
+    type: 'changeSet',
+    payload,
+    fdv1Fallback: fallback.fdv1Fallback,
+    environmentId,
+    freshness,
+    fdv1FallbackTtlMs: fallback.fdv1FallbackTtlMs,
+  };
 }
 
 /**
@@ -75,10 +84,15 @@ export function changeSet(
  */
 export function interrupted(
   errorInfo: DataSourceStatusErrorInfo,
-  fdv1Fallback: boolean,
-  fdv1FallbackTtlMs?: number,
+  fallback: FallbackDirective,
 ): FDv2SourceResult {
-  return { type: 'status', state: 'interrupted', errorInfo, fdv1Fallback, fdv1FallbackTtlMs };
+  return {
+    type: 'status',
+    state: 'interrupted',
+    errorInfo,
+    fdv1Fallback: fallback.fdv1Fallback,
+    fdv1FallbackTtlMs: fallback.fdv1FallbackTtlMs,
+  };
 }
 
 /**
@@ -94,28 +108,36 @@ export function shutdown(): FDv2SourceResult {
  */
 export function terminalError(
   errorInfo: DataSourceStatusErrorInfo,
-  fdv1Fallback: boolean,
-  fdv1FallbackTtlMs?: number,
+  fallback: FallbackDirective,
 ): FDv2SourceResult {
-  return { type: 'status', state: 'terminal_error', errorInfo, fdv1Fallback, fdv1FallbackTtlMs };
+  return {
+    type: 'status',
+    state: 'terminal_error',
+    errorInfo,
+    fdv1Fallback: fallback.fdv1Fallback,
+    fdv1FallbackTtlMs: fallback.fdv1FallbackTtlMs,
+  };
 }
 
 /**
  * Signals a server-initiated disconnect. Unlike `terminal_error`, the
  * synchronizer will reconnect; the orchestrator does not block this source.
  *
  * @param reason Human-readable description of why the server closed the stream.
- * @param fdv1Fallback Whether the server directed the client to fall back to FDv1.
- * @param fdv1FallbackTtlMs How long (ms) to remain on FDv1 before attempting FDv2
- *   recovery. Omit to use the caller's default; pass `0` for indefinite fallback.
- *   Same semantics as {@link StatusResult.fdv1FallbackTtlMs}.
+ * @param fallback The FDv1 fallback directive. `fdv1Fallback === true` means the
+ *   server directed the client to fall back to FDv1. `fdv1FallbackTtlMs` is how
+ *   long (ms) to remain on FDv1 before attempting FDv2 recovery (omit for the
+ *   caller's default; `0` for indefinite). Same semantics as
+ *   {@link StatusResult.fdv1FallbackTtlMs}.
  */
-export function goodbye(
-  reason: string,
-  fdv1Fallback: boolean,
-  fdv1FallbackTtlMs?: number,
-): FDv2SourceResult {
-  return { type: 'status', state: 'goodbye', reason, fdv1Fallback, fdv1FallbackTtlMs };
+export function goodbye(reason: string, fallback: FallbackDirective): FDv2SourceResult {
+  return {
+    type: 'status',
+    state: 'goodbye',
+    reason,
+    fdv1Fallback: fallback.fdv1Fallback,
+    fdv1FallbackTtlMs: fallback.fdv1FallbackTtlMs,
+  };
 }
 
 /** Builds {@link DataSourceStatusErrorInfo} for an unexpected HTTP status. */