From 378a6a9882bc1c72d06ab727b44dbdabe4e62fb8 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 12:13:36 +0200 Subject: [PATCH 1/8] feat(node-sdk): revalidate flag definitions with ETags --- .changeset/light-flags-revalidate.md | 5 + packages/node-sdk/README.md | 11 +- packages/node-sdk/src/client.ts | 65 ++- packages/node-sdk/src/fetch-http-client.ts | 9 +- packages/node-sdk/src/flags-cache.ts | 34 +- packages/node-sdk/src/types.ts | 6 + packages/node-sdk/test/client.test.ts | 21 +- .../node-sdk/test/fetch-http-client.test.ts | 108 ++++- packages/node-sdk/test/flags-cache.test.ts | 45 ++ .../node-sdk/test/flags-revalidation.test.ts | 443 ++++++++++++++++++ 10 files changed, 712 insertions(+), 35 deletions(-) create mode 100644 .changeset/light-flags-revalidate.md create mode 100644 packages/node-sdk/test/flags-revalidation.test.ts diff --git a/.changeset/light-flags-revalidate.md b/.changeset/light-flags-revalidate.md new file mode 100644 index 000000000..de71a8ac5 --- /dev/null +++ b/.changeset/light-flags-revalidate.md @@ -0,0 +1,5 @@ +--- +"@reflag/node-sdk": patch +--- + +Use ETags when refreshing flag definitions to avoid downloading and recompiling unchanged flags. Custom HTTP clients can opt in by returning response headers. diff --git a/packages/node-sdk/README.md b/packages/node-sdk/README.md index a2f756e9f..cd2446a74 100644 --- a/packages/node-sdk/README.md +++ b/packages/node-sdk/README.md @@ -180,7 +180,16 @@ This behavior is built in and does not currently require configuration. Flag definitions include the rules needed to determine which flags should be enabled and which config values should be applied to any given user/company. Flag definitions are automatically fetched when calling `initialize()`. -They are then cached and refreshed in the background. +They are then cached and refreshed in the background. Ordinary refreshes use +ETags to avoid downloading unchanged definitions. A `304 Not Modified` response +keeps the cached definitions and resets their freshness without recompiling +rules or writing a new fallback snapshot. Initial fetches and refreshes with a +`waitForVersion` target remain unconditional. + +Custom HTTP clients can enable this optimization by returning the ETag in +`HttpClientResponse.headers` and returning 304 responses with `body: undefined`. +Clients that omit response headers continue using full fetches. + It's also possible to get the currently in use flag definitions: ```typescript diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 5791e4d7e..e8161751d 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -536,10 +536,20 @@ export class ReflagClient { ? "features" : `features?waitForVersion=${encodeURIComponent(String(waitForVersion))}`; - const res = await this.get( + // A 304 cannot prove that a newer requested version was reached. + // Keep version-targeted fetches unconditional. + const etag = + waitForVersion === undefined ? this.flagsCache.getETag() : undefined; + const response = await this.getResponse( path, this._config.flagsFetchRetries, + { etag, recoverUnexpectedNotModified: true }, ); + if (response?.status === 304) { + return { notModified: true }; + } + + const res = response?.body; const flagStateVersion = res?.flagStateVersion; if ( !isObject(res) || @@ -559,6 +569,9 @@ export class ReflagClient { return { definitions: compileFlagDefinitions(res.features), flagStateVersion, + etag: Object.entries(response?.headers ?? {}).find( + ([name]) => name.toLowerCase() === "etag", + )?.[1], }; }, { @@ -1181,17 +1194,56 @@ export class ReflagClient { * @throws An error if the path is invalid. **/ private async get(path: string, retries: number = 3) { + const response = await this.getResponse(path, retries); + if (!response?.body) { + return undefined; + } + + const { success: _, ...result } = response.body; + return result as TResponse; + } + + private async getResponse( + path: string, + retries: number = 3, + options: { + etag?: string; + recoverUnexpectedNotModified?: boolean; + } = {}, + ) { ok(typeof path === "string" && path.length > 0, "path must be a string"); try { const url = this.buildUrl(path); + const headers = options.etag + ? { ...this._config.headers, "If-None-Match": options.etag } + : this._config.headers; + let recoveredNotModified = false; + const request = async (requestHeaders: Record) => { + const response = await this.httpClient.get< + TResponse & { success: boolean } + >(url, requestHeaders, this._config.fetchTimeoutMs); + this.logger.debug(`get request to "${url}"`, response); + return response; + }; + return await withRetry( async () => { - const response = await this.httpClient.get< - TResponse & { success: boolean } - >(url, this._config.headers, this._config.fetchTimeoutMs); + let response = await request(headers); + if (response.status === 304 && options.etag) { + return response; + } - this.logger.debug(`get request to "${url}"`, response); + // An unconditional request should not receive a 304. Try once more + // without a validator before falling back to normal error handling. + if ( + response.status === 304 && + options.recoverUnexpectedNotModified && + !recoveredNotModified + ) { + recoveredNotModified = true; + response = await request(this._config.headers); + } if ( !response.ok || @@ -1202,8 +1254,7 @@ export class ReflagClient { `invalid response received from server for "${url}": ${JSON.stringify(response.body)}`, ); } - const { success: _, ...result } = response.body; - return result as TResponse; + return response; }, (error) => { this.logger.warn("failed to fetch flags, will retry", error); diff --git a/packages/node-sdk/src/fetch-http-client.ts b/packages/node-sdk/src/fetch-http-client.ts index 6d3d26df7..cda66ffe9 100644 --- a/packages/node-sdk/src/fetch-http-client.ts +++ b/packages/node-sdk/src/fetch-http-client.ts @@ -47,15 +47,18 @@ const fetchClient: HttpClient = { signal: AbortSignal.timeout(timeoutMs), // We must use no-cache to avoid services such as Next.js from caching the response indefinitely. // We also can't use no-store because of Next.js error withRetry https://github.com/vercel/next.js/discussions/54036. - // We also have local caching in the SDKs, so we don't need to cache the response. + // The SDK caches definitions and explicitly revalidates them using ETags. cache: "no-cache", }); - const json = await response.json(); return { ok: response.ok, status: response.status, - body: json as TResponse, + body: + response.status === 304 + ? undefined + : ((await response.json()) as TResponse), + headers: Object.fromEntries(response.headers), }; }, }; diff --git a/packages/node-sdk/src/flags-cache.ts b/packages/node-sdk/src/flags-cache.ts index 88619581e..460090df9 100644 --- a/packages/node-sdk/src/flags-cache.ts +++ b/packages/node-sdk/src/flags-cache.ts @@ -2,10 +2,14 @@ import type { CachedFlagDefinition, Logger } from "./types"; const DEFAULT_MIN_REFRESH_INTERVAL_MS = 1000; -type FlagsCacheRefreshResult = { - definitions: CachedFlagDefinition[]; - flagStateVersion?: number; -}; +type FlagsCacheRefreshResult = + | { + definitions: CachedFlagDefinition[]; + flagStateVersion?: number; + etag?: string; + notModified?: false; + } + | { notModified: true }; type CancelScheduledRefresh = () => void; @@ -38,6 +42,7 @@ type FlagsCacheOptions = { export class FlagsCache { private value: CachedFlagDefinition[] | undefined; private flagStateVersion: number | undefined; + private etag: string | undefined; private refreshPromise: Promise | undefined; private scheduledRefresh: ScheduledRefresh | undefined; private lastRefreshAt: number | undefined; @@ -71,6 +76,10 @@ export class FlagsCache { return this.flagStateVersion; } + public getETag() { + return this.etag; + } + public async refresh(waitForVersion?: number) { if (this.destroyed) { return this.value; @@ -90,6 +99,7 @@ export class FlagsCache { this.destroyed = true; this.value = undefined; this.flagStateVersion = undefined; + this.etag = undefined; this.refreshPromise = undefined; this.cancelScheduledRefresh(); this.pendingFullRefresh = false; @@ -259,6 +269,21 @@ export class FlagsCache { return; } + if (result.notModified) { + if ( + this.value === undefined || + !this.etag || + waitForVersion !== undefined + ) { + return; + } + + this.clearSatisfiedPendingVersion(); + this.lastRefreshAt = Date.now(); + this.logger?.debug("flag definitions unchanged"); + return; + } + this.clearSatisfiedPendingVersion(result.flagStateVersion); if (!this.shouldApplyRefreshResult(result.flagStateVersion)) { @@ -267,6 +292,7 @@ export class FlagsCache { this.value = result.definitions; this.flagStateVersion = result.flagStateVersion; + this.etag = result.etag; this.lastRefreshAt = Date.now(); this.logger?.info("refreshed flag definitions"); } diff --git a/packages/node-sdk/src/types.ts b/packages/node-sdk/src/types.ts index 006c110c7..2c6698258 100644 --- a/packages/node-sdk/src/types.ts +++ b/packages/node-sdk/src/types.ts @@ -516,6 +516,12 @@ export type HttpClientResponse = { * The body of the response if available. **/ body: TResponse | undefined; + + /** + * Response headers, if available. Include the ETag header to enable + * conditional flag-definition refreshes with a custom HTTP client. + **/ + headers?: Record; }; /** diff --git a/packages/node-sdk/test/client.test.ts b/packages/node-sdk/test/client.test.ts index 459465c5d..eca982365 100644 --- a/packages/node-sdk/test/client.test.ts +++ b/packages/node-sdk/test/client.test.ts @@ -1188,14 +1188,17 @@ describe("ReflagClient", () => { vi.useFakeTimers(); try { - httpClient.get.mockResolvedValue({ - ok: true, - status: 200, - body: { - success: true, - ...flagDefinitions, - }, - }); + httpClient.get + .mockResolvedValueOnce({ + ok: true, + status: 200, + body: { + success: true, + ...flagDefinitions, + }, + headers: { etag: 'W/"flags-v1"' }, + }) + .mockResolvedValue({ ok: false, status: 304, body: undefined }); const client = new ReflagClient({ ...validOptions, @@ -1212,7 +1215,7 @@ describe("ReflagClient", () => { expect(httpClient.get).toHaveBeenNthCalledWith( 2, "https://api.example.com/features", - expectedHeaders, + { ...expectedHeaders, "If-None-Match": 'W/"flags-v1"' }, API_TIMEOUT_MS, ); diff --git a/packages/node-sdk/test/fetch-http-client.test.ts b/packages/node-sdk/test/fetch-http-client.test.ts index 89b2242c8..420c54ed3 100644 --- a/packages/node-sdk/test/fetch-http-client.test.ts +++ b/packages/node-sdk/test/fetch-http-client.test.ts @@ -1,8 +1,12 @@ +import { createServer } from "node:http"; + import { afterEach, describe, expect, it, vi } from "vitest"; import { API_TIMEOUT_MS } from "../src/config"; import fetchClient from "../src/fetch-http-client"; +const originalFetch = global.fetch; + // mock environment variables vi.mock("../src/config", () => ({ API_TIMEOUT_MS: 100 })); @@ -11,6 +15,8 @@ describe("fetchClient", () => { const headers = { "Content-Type": "application/json" }; afterEach(() => { + global.fetch = originalFetch; + vi.restoreAllMocks(); vi.resetAllMocks(); }); @@ -46,11 +52,17 @@ describe("fetchClient", () => { }); it("should make a GET request and return the response", async () => { - const response = { ok: true, status: 200, body: { success: true } }; + const response = { + ok: true, + status: 200, + body: { success: true }, + headers: { etag: 'W/"flags-v1"' }, + }; global.fetch = vi.fn().mockResolvedValue({ ok: true, status: 200, + headers: new Headers({ ETag: 'W/"flags-v1"' }), json: async () => ({ success: true, }), @@ -90,17 +102,20 @@ describe("fetchClient", () => { }); it("should timeout a GET request that takes too long", async () => { - global.fetch = vi - .fn() - .mockImplementation( - () => - new Promise((resolve) => - setTimeout( - () => resolve({ ok: true, json: async () => ({}) }), - API_TIMEOUT_MS + 100, - ), + global.fetch = vi.fn().mockImplementation( + () => + new Promise((resolve) => + setTimeout( + () => + resolve({ + ok: true, + headers: new Headers(), + json: async () => ({}), + }), + API_TIMEOUT_MS + 100, ), - ); + ), + ); await fetchClient.get(url, headers); expect(vi.mocked(global.fetch).mock.calls[0][1]?.signal?.aborted).toBe( @@ -131,11 +146,13 @@ describe("fetchClient", () => { ok: false, status: 400, body: { error: "Something went wrong" }, + headers: {}, }; global.fetch = vi.fn().mockResolvedValue({ ok: false, status: 400, + headers: new Headers(), json: async () => ({ error: "Something went wrong" }), } as Response); @@ -144,6 +161,75 @@ describe("fetchClient", () => { expect(result).toEqual(response); }); + it("returns a 304 without attempting to parse an empty JSON body", async () => { + const response = new Response(null, { + status: 304, + headers: { ETag: 'W/"flags-v1"' }, + }); + const json = vi.spyOn(response, "json"); + global.fetch = vi.fn().mockResolvedValue(response); + + const result = await fetchClient.get(url, headers, API_TIMEOUT_MS); + + expect(result).toEqual({ + ok: false, + status: 304, + body: undefined, + headers: { etag: 'W/"flags-v1"' }, + }); + expect(json).not.toHaveBeenCalled(); + }); + + it("supports real conditional GETs while retaining cache: no-cache", async () => { + global.fetch = originalFetch; + const etag = 'W/"flags-v1"'; + const receivedValidators: Array = []; + const server = createServer((request, response) => { + const validator = request.headers["if-none-match"]; + receivedValidators.push( + typeof validator === "string" ? validator : undefined, + ); + response.setHeader("ETag", etag); + response.setHeader("Cache-Control", "max-age=59"); + if (validator === etag) { + response.writeHead(304).end(); + } else { + response.setHeader("Content-Type", "application/json"); + response.end(JSON.stringify({ success: true, features: [] })); + } + }); + await new Promise((resolve) => + server.listen(0, "127.0.0.1", resolve), + ); + + try { + const address = server.address(); + if (!address || typeof address === "string") { + throw new Error("Expected a TCP server address"); + } + const originUrl = `http://127.0.0.1:${address.port}/features`; + const initial = await fetchClient.get(originUrl, headers, 5000); + const revalidated = await fetchClient.get( + originUrl, + { ...headers, "If-None-Match": initial.headers!.etag! }, + 5000, + ); + + expect(initial.status).toBe(200); + expect(initial.headers?.etag).toBe(etag); + expect(revalidated).toMatchObject({ + status: 304, + ok: false, + body: undefined, + }); + expect(receivedValidators).toEqual([undefined, etag]); + } finally { + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); + } + }); + it("should not handle POST exceptions", async () => { global.fetch = vi.fn().mockRejectedValue(new Error("Network error")); diff --git a/packages/node-sdk/test/flags-cache.test.ts b/packages/node-sdk/test/flags-cache.test.ts index 661b2672b..614f8f408 100644 --- a/packages/node-sdk/test/flags-cache.test.ts +++ b/packages/node-sdk/test/flags-cache.test.ts @@ -109,6 +109,51 @@ describe("FlagsCache", () => { expect(cache.get()).toEqual(createDefinitions("newer")); }); + it("does not let a 304 satisfy a pending newer version", async () => { + const revalidation = createDeferred(); + const fetchFlags = vi + .fn() + .mockResolvedValueOnce({ + definitions: createDefinitions("first"), + flagStateVersion: 1, + etag: '"v1"', + }) + .mockImplementationOnce(() => revalidation.promise) + .mockResolvedValueOnce({ + definitions: createDefinitions("newer"), + flagStateVersion: 2, + etag: '"v2"', + }); + const cache = new FlagsCache(fetchFlags, { minRefreshIntervalMs: 0 }); + await cache.refresh(); + const refreshPromise = cache.refresh(); + const versionPromise = cache.refresh(2); + revalidation.resolve({ notModified: true }); + await Promise.all([refreshPromise, versionPromise]); + + expect(fetchFlags).toHaveBeenCalledTimes(3); + expect(fetchFlags).toHaveBeenNthCalledWith(3, 2); + expect(cache.getFlagStateVersion()).toBe(2); + expect(cache.getETag()).toBe('"v2"'); + }); + + it("does not apply a refresh or validator after destruction", async () => { + const response = createDeferred(); + const cache = new FlagsCache(() => response.promise); + const refreshPromise = cache.refresh(); + cache.destroy(); + response.resolve({ + definitions: createDefinitions("late"), + flagStateVersion: 1, + etag: '"late"', + }); + await refreshPromise; + + expect(cache.get()).toBeUndefined(); + expect(cache.getETag()).toBeUndefined(); + expect(cache.getLastRefreshAt()).toBeUndefined(); + }); + it("throttles refreshes and runs one trailing refresh when scheduling is supported", async () => { vi.useFakeTimers(); diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts new file mode 100644 index 000000000..8278e1149 --- /dev/null +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -0,0 +1,443 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { ReflagClient } from "../src/client"; +import { API_TIMEOUT_MS, FLAGS_REFETCH_MS } from "../src/config"; +import type { ClientOptions, FlagsAPIResponse } from "../src/types"; + +const etag = 'W/"flags-v1"'; +const notModified = { ok: false, status: 304, body: undefined }; +const definitions: FlagsAPIResponse = { + flagStateVersion: 1, + features: [ + { + key: "flag", + description: null, + targeting: { + version: 1, + rules: [{ filter: { type: "constant", value: true } }], + }, + }, + ], +}; + +function modified(version = 1, responseETag: string | undefined = etag) { + return { + ok: true, + status: 200, + body: { success: true, ...definitions, flagStateVersion: version }, + headers: responseETag ? { ETag: responseETag } : undefined, + }; +} + +const httpClient = { get: vi.fn(), post: vi.fn() }; +const logger = { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }; +const clients: ReflagClient[] = []; + +function createClient(options: Partial = {}) { + const client = new ReflagClient({ + secretKey: "validSecretKeyWithMoreThan22Chars", + apiBaseUrl: "https://api.example.com/", + httpClient, + logger, + flagsSyncMode: "in-request", + flagsFetchRetries: 0, + offline: false, + batchOptions: { flushOnExit: false }, + ...options, + }); + clients.push(client); + return client; +} + +async function refresh(client: ReflagClient, waitForVersion?: number) { + await vi.advanceTimersByTimeAsync(1001); + await client.refreshFlags(waitForVersion); +} + +function requestHeaders(call: number) { + return httpClient.get.mock.calls[call - 1]?.[1]; +} + +describe("flag-definition revalidation", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-01-01T00:00:00Z")); + httpClient.get.mockReset(); + httpClient.post.mockResolvedValue({ + ok: true, + status: 200, + body: { success: true }, + }); + }); + + afterEach(() => { + for (const client of clients.splice(0)) { + client.destroy(); + } + vi.useRealTimers(); + vi.restoreAllMocks(); + vi.clearAllMocks(); + }); + + it("retains compiled definitions on repeated 304s and replaces them on a changed 200", async () => { + const provider = { + load: vi.fn(), + save: vi.fn().mockResolvedValue(undefined), + }; + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce(notModified) + .mockResolvedValueOnce(notModified) + .mockResolvedValueOnce({ + ...modified(2, '"flags-v2"'), + body: { + success: true, + flagStateVersion: 2, + features: [ + { + ...definitions.features[0], + targeting: { + version: 2, + rules: [{ filter: { type: "constant", value: false } }], + }, + }, + ], + }, + }); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + const cache = client["flagsCache"]; + const initialDefinitions = cache.get(); + const initialRefreshAt = cache.getLastRefreshAt(); + + await refresh(client); + expect(cache.get()).toBe(initialDefinitions); + expect(cache.getFlagStateVersion()).toBe(1); + expect(cache.getLastRefreshAt()).toBeGreaterThan(initialRefreshAt!); + expect(cache.getETag()).toBe(etag); + + await refresh(client); + expect(cache.get()).toBe(initialDefinitions); + expect(provider.save).toHaveBeenCalledTimes(1); + expect(provider.load).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalled(); + expect(httpClient.get).toHaveBeenCalledTimes(3); + expect(requestHeaders(1)).not.toHaveProperty("If-None-Match"); + expect(requestHeaders(2)).toEqual({ + ...requestHeaders(1), + "If-None-Match": etag, + }); + expect(requestHeaders(3)).toEqual(requestHeaders(2)); + expect(client["_config"].headers).not.toHaveProperty("If-None-Match"); + + await refresh(client); + expect(cache.get()).not.toBe(initialDefinitions); + expect(cache.getFlagStateVersion()).toBe(2); + expect(cache.getETag()).toBe('"flags-v2"'); + expect(client.getFlag({}, "flag").isEnabled).toBe(false); + expect(provider.save).toHaveBeenCalledTimes(2); + }); + + it("revalidates an empty flag snapshot", async () => { + httpClient.get + .mockResolvedValueOnce({ + ...modified(), + body: { success: true, flagStateVersion: 1, features: [] }, + }) + .mockResolvedValueOnce(notModified); + const client = createClient(); + await client.initialize(); + const snapshot = client["flagsCache"].get(); + await refresh(client); + + expect(snapshot).toEqual([]); + expect(client["flagsCache"].get()).toBe(snapshot); + expect(requestHeaders(2)["If-None-Match"]).toBe(etag); + }); + + it("revalidates through the default fetch transport", async () => { + const initial = modified(); + const changed = modified(2, '"flags-v2"'); + const fetch = vi + .spyOn(global, "fetch") + .mockResolvedValueOnce( + new Response(JSON.stringify(initial.body), { + headers: { ETag: etag }, + }), + ) + .mockResolvedValueOnce(new Response(null, { status: 304 })) + .mockResolvedValueOnce( + new Response(JSON.stringify(changed.body), { + headers: { ETag: '"flags-v2"' }, + }), + ); + const client = createClient({ httpClient: undefined }); + await client.initialize(); + const snapshot = client["flagsCache"].get(); + await refresh(client); + + expect(client["flagsCache"].get()).toBe(snapshot); + expect(fetch).toHaveBeenNthCalledWith( + 2, + "https://api.example.com/features", + expect.objectContaining({ + cache: "no-cache", + headers: { ...client["_config"].headers, "If-None-Match": etag }, + }), + ); + await refresh(client); + expect(client["flagsCache"].getFlagStateVersion()).toBe(2); + expect(client["flagsCache"].getETag()).toBe('"flags-v2"'); + expect(fetch).toHaveBeenCalledTimes(3); + }); + + it("does not send definition validators to evaluated flag requests", async () => { + httpClient.get.mockResolvedValueOnce(modified()).mockResolvedValueOnce({ + ok: true, + status: 200, + body: { success: true, features: {}, remoteContextUsed: false }, + }); + const client = createClient(); + await client.initialize(); + expect(await client.getFlagsRemote()).toEqual({}); + + expect(httpClient.get).toHaveBeenNthCalledWith( + 2, + "https://api.example.com/features/evaluated", + requestHeaders(1), + API_TIMEOUT_MS, + ); + expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); + }); + + it("keeps custom clients without response headers working", async () => { + httpClient.get.mockResolvedValue({ + ok: true, + status: 200, + body: { success: true, ...definitions }, + }); + const client = createClient(); + await client.initialize(); + await refresh(client); + + expect(httpClient.get).toHaveBeenCalledTimes(2); + expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); + expect(client.getFlag({}, "flag").isEnabled).toBe(true); + }); + + it("clears the old ETag when an accepted replacement omits it", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce({ ...modified(2), headers: undefined }) + .mockResolvedValueOnce(modified(3)); + const client = createClient(); + await client.initialize(); + await refresh(client); + expect(client["flagsCache"].getETag()).toBeUndefined(); + await refresh(client); + + expect(requestHeaders(2)["If-None-Match"]).toBe(etag); + expect(requestHeaders(3)).not.toHaveProperty("If-None-Match"); + }); + + it.each([ + ["an HTTP error", { ok: false, status: 500, body: { success: false } }], + [ + "malformed definitions", + { + ...modified(2, '"invalid"'), + body: { success: true, flagStateVersion: 2, features: null }, + }, + ], + ["an older version", modified(0, '"older"')], + ])("preserves the snapshot and ETag after %s", async (_, response) => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce(response) + .mockResolvedValueOnce(notModified); + const client = createClient(); + await client.initialize(); + const cache = client["flagsCache"]; + const snapshot = cache.get(); + const lastRefreshAt = cache.getLastRefreshAt(); + await refresh(client); + + expect(cache.get()).toBe(snapshot); + expect(cache.getETag()).toBe(etag); + expect(cache.getLastRefreshAt()).toBe(lastRefreshAt); + await refresh(client); + expect(requestHeaders(3)["If-None-Match"]).toBe(etag); + expect(cache.get()).toBe(snapshot); + expect(cache.getFlagStateVersion()).toBe(1); + }); + + it("preserves the validator after a network failure", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockRejectedValueOnce(new Error("network failure")) + .mockResolvedValueOnce(notModified); + const client = createClient(); + await client.initialize(); + const lastRefreshAt = client["flagsCache"].getLastRefreshAt(); + await refresh(client); + expect(client["flagsCache"].getLastRefreshAt()).toBe(lastRefreshAt); + await refresh(client); + + expect(requestHeaders(3)["If-None-Match"]).toBe(etag); + expect(client.getFlag({}, "flag").isEnabled).toBe(true); + }); + + it("isolates validators between clients with different credentials", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce(modified(2, '"other-environment"')) + .mockResolvedValue(notModified); + const first = createClient(); + const second = createClient({ + secretKey: "otherSecretKeyWithMoreThan22Chars", + }); + await first.initialize(); + await second.initialize(); + await refresh(first); + await second.refreshFlags(); + + expect(requestHeaders(3)["If-None-Match"]).toBe(etag); + expect(requestHeaders(4)["If-None-Match"]).toBe('"other-environment"'); + expect(requestHeaders(3).Authorization).not.toBe( + requestHeaders(4).Authorization, + ); + first.destroy(); + expect(first["flagsCache"].getETag()).toBeUndefined(); + expect(second["flagsCache"].getETag()).toBe('"other-environment"'); + }); + + it("recovers from an unexpected cold-start 304 with one unconditional fetch", async () => { + httpClient.get + .mockResolvedValueOnce(notModified) + .mockResolvedValueOnce(modified()); + const client = createClient(); + await client.initialize(); + + expect(httpClient.get).toHaveBeenCalledTimes(2); + expect(requestHeaders(1)).not.toHaveProperty("If-None-Match"); + expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); + expect(client.getFlag({}, "flag").isEnabled).toBe(true); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it("does not initialize an empty cache or loop on repeated unexpected 304s", async () => { + httpClient.get.mockResolvedValue(notModified); + const client = createClient(); + await client.initialize(); + + expect(httpClient.get).toHaveBeenCalledTimes(2); + expect(client["flagsCache"].get()).toBeUndefined(); + expect(client["flagsCache"].getLastRefreshAt()).toBeUndefined(); + }); + + it("fetches unconditionally after loading fallback definitions", async () => { + const provider = { + load: vi.fn().mockResolvedValue({ + version: 1, + savedAt: "2025-12-31T00:00:00Z", + flags: definitions.features, + }), + save: vi.fn().mockResolvedValue(undefined), + }; + httpClient.get + .mockResolvedValueOnce({ + ok: false, + status: 503, + body: { success: false }, + }) + .mockResolvedValueOnce(notModified) + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce(notModified); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + expect(client["flagsCache"].getETag()).toBeUndefined(); + await refresh(client); + + expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); + expect(requestHeaders(3)).not.toHaveProperty("If-None-Match"); + expect(provider.save).toHaveBeenCalledTimes(1); + await refresh(client); + expect(requestHeaders(4)["If-None-Match"]).toBe(etag); + expect(provider.save).toHaveBeenCalledTimes(1); + }); + + it("keeps version-targeted refreshes unconditional and uses the new validator afterward", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce(modified(2, '"flags-v2"')) + .mockResolvedValueOnce(notModified); + const client = createClient(); + await client.initialize(); + await refresh(client, 2); + + expect(httpClient.get).toHaveBeenNthCalledWith( + 2, + "https://api.example.com/features?waitForVersion=2", + requestHeaders(1), + API_TIMEOUT_MS, + ); + expect(client["flagsCache"].getFlagStateVersion()).toBe(2); + await refresh(client); + expect(requestHeaders(3)["If-None-Match"]).toBe('"flags-v2"'); + }); + + it("does not treat an unexpected 304 as reaching a requested version", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValue(notModified); + const client = createClient(); + await client.initialize(); + const lastRefreshAt = client["flagsCache"].getLastRefreshAt(); + await refresh(client, 2); + + expect(httpClient.get).toHaveBeenCalledTimes(3); + expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); + expect(requestHeaders(3)).not.toHaveProperty("If-None-Match"); + expect(client["flagsCache"].getFlagStateVersion()).toBe(1); + expect(client["flagsCache"].getLastRefreshAt()).toBe(lastRefreshAt); + }); + + it("revalidates on polling intervals without retrying 304s", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValue(notModified); + const client = createClient({ + flagsSyncMode: "polling", + flagsFetchRetries: 2, + }); + await client.initialize(); + await vi.advanceTimersByTimeAsync(FLAGS_REFETCH_MS * 2); + + expect(httpClient.get).toHaveBeenCalledTimes(3); + expect(requestHeaders(2)["If-None-Match"]).toBe(etag); + expect(requestHeaders(3)["If-None-Match"]).toBe(etag); + expect(logger.warn).not.toHaveBeenCalled(); + }); + + it("resets in-request freshness so subsequent accesses do not immediately refresh again", async () => { + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValue(notModified); + const client = createClient(); + await client.initialize(); + await vi.advanceTimersByTimeAsync(FLAGS_REFETCH_MS + 1); + client.getFlag({}, "flag"); + await client["flagsCache"].waitRefresh(); + expect(httpClient.get).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(1001); + client.getFlag({}, "flag"); + await client["flagsCache"].waitRefresh(); + expect(httpClient.get).toHaveBeenCalledTimes(2); + + await vi.advanceTimersByTimeAsync(FLAGS_REFETCH_MS); + client.getFlag({}, "flag"); + await client["flagsCache"].waitRefresh(); + expect(httpClient.get).toHaveBeenCalledTimes(3); + }); +}); From 55117bdd8762f0da0f3db475d1eef0ee8b80e424 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 13:25:53 +0200 Subject: [PATCH 2/8] fix(node-sdk): retain ETags only after fallback saves succeed --- packages/node-sdk/README.md | 5 +- packages/node-sdk/src/client.ts | 16 +++++-- packages/node-sdk/src/flags-cache.ts | 17 ++++++- packages/node-sdk/test/flags-cache.test.ts | 47 +++++++++++++++++++ .../node-sdk/test/flags-revalidation.test.ts | 47 +++++++++++++++++++ 5 files changed, 124 insertions(+), 8 deletions(-) diff --git a/packages/node-sdk/README.md b/packages/node-sdk/README.md index cd2446a74..a5896ac61 100644 --- a/packages/node-sdk/README.md +++ b/packages/node-sdk/README.md @@ -184,7 +184,10 @@ They are then cached and refreshed in the background. Ordinary refreshes use ETags to avoid downloading unchanged definitions. A `304 Not Modified` response keeps the cached definitions and resets their freshness without recompiling rules or writing a new fallback snapshot. Initial fetches and refreshes with a -`waitForVersion` target remain unconditional. +`waitForVersion` target remain unconditional. When a fallback provider is +configured, the SDK retains the ETag only after the snapshot is saved +successfully. A failed or pending save keeps ordinary refreshes unconditional +so persistence can recover, without delaying flag availability. Custom HTTP clients can enable this optimization by returning the ETag in `HttpClientResponse.headers` and returning 304 responses with `body: undefined`. diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index e8161751d..7d1da0c2b 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -564,14 +564,18 @@ export class ReflagClient { : undefined; } - void this.saveFlagsFallbackDefinitions(res.features); + const responseETag = Object.entries(response?.headers ?? {}).find( + ([name]) => name.toLowerCase() === "etag", + )?.[1]; + const saved = this.saveFlagsFallbackDefinitions(res.features); this.canLoadFlagsFallbackProvider = false; return { definitions: compileFlagDefinitions(res.features), flagStateVersion, - etag: Object.entries(response?.headers ?? {}).find( - ([name]) => name.toLowerCase() === "etag", - )?.[1], + // Publish definitions immediately, but only enable conditional + // refreshes after persistence succeeds. A failed save leaves the + // next ordinary refresh unconditional so it naturally tries again. + etag: saved.then((success) => (success ? responseETag : undefined)), }; }, { @@ -643,7 +647,7 @@ export class ReflagClient { private async saveFlagsFallbackDefinitions(flags: FlagAPIResponse[]) { const provider = this._config.flagsFallbackProvider; if (!provider) { - return; + return true; } try { @@ -651,11 +655,13 @@ export class ReflagClient { this._config.flagsFallbackProviderContext, createFlagsFallbackSnapshot(flags), ); + return true; } catch (error) { this.logger.error( "flagsFallbackProvider: failed to save flag definitions", error, ); + return false; } } diff --git a/packages/node-sdk/src/flags-cache.ts b/packages/node-sdk/src/flags-cache.ts index 460090df9..da1a69a1c 100644 --- a/packages/node-sdk/src/flags-cache.ts +++ b/packages/node-sdk/src/flags-cache.ts @@ -6,7 +6,7 @@ type FlagsCacheRefreshResult = | { definitions: CachedFlagDefinition[]; flagStateVersion?: number; - etag?: string; + etag?: string | Promise; notModified?: false; } | { notModified: true }; @@ -292,7 +292,20 @@ export class FlagsCache { this.value = result.definitions; this.flagStateVersion = result.flagStateVersion; - this.etag = result.etag; + this.etag = typeof result.etag === "string" ? result.etag : undefined; + if (result.etag && typeof result.etag !== "string") { + void result.etag.then( + (etag) => { + // A late save must not attach its validator to a newer snapshot + // or restore it after destruction. + if (this.value === result.definitions) { + this.etag = etag; + } + }, + (error) => + this.logger?.error("flag definition ETag unavailable", error), + ); + } this.lastRefreshAt = Date.now(); this.logger?.info("refreshed flag definitions"); } diff --git a/packages/node-sdk/test/flags-cache.test.ts b/packages/node-sdk/test/flags-cache.test.ts index 614f8f408..50ec1948d 100644 --- a/packages/node-sdk/test/flags-cache.test.ts +++ b/packages/node-sdk/test/flags-cache.test.ts @@ -137,6 +137,53 @@ describe("FlagsCache", () => { expect(cache.getETag()).toBe('"v2"'); }); + it.each([ + ["succeeds", '"old"'], + ["fails", undefined], + ])( + "does not replace a newer validator when an older save %s", + async (_, etag) => { + const savedETag = createDeferred(); + const fetchFlags = vi + .fn() + .mockResolvedValueOnce({ + definitions: createDefinitions("older"), + flagStateVersion: 1, + etag: savedETag.promise, + }) + .mockResolvedValueOnce({ + definitions: createDefinitions("newer"), + flagStateVersion: 2, + etag: '"newer"', + }); + const cache = new FlagsCache(fetchFlags, { minRefreshIntervalMs: 0 }); + await cache.refresh(); + expect(cache.getETag()).toBeUndefined(); + await cache.refresh(); + savedETag.resolve(etag); + await savedETag.promise; + + expect(cache.getFlagStateVersion()).toBe(2); + expect(cache.getETag()).toBe('"newer"'); + }, + ); + + it("does not restore a deferred validator after destruction", async () => { + const savedETag = createDeferred(); + const cache = new FlagsCache(async () => ({ + definitions: createDefinitions("first"), + flagStateVersion: 1, + etag: savedETag.promise, + })); + await cache.refresh(); + cache.destroy(); + savedETag.resolve('"first"'); + await savedETag.promise; + + expect(cache.get()).toBeUndefined(); + expect(cache.getETag()).toBeUndefined(); + }); + it("does not apply a refresh or validator after destruction", async () => { const response = createDeferred(); const cache = new FlagsCache(() => response.promise); diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts index 8278e1149..4390e7475 100644 --- a/packages/node-sdk/test/flags-revalidation.test.ts +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -138,6 +138,53 @@ describe("flag-definition revalidation", () => { expect(provider.save).toHaveBeenCalledTimes(2); }); + it("leaves the ETag unset after a failed save so the next refresh naturally retries persistence", async () => { + const error = new Error("temporary storage failure"); + const provider = { + load: vi.fn(), + save: vi.fn().mockRejectedValueOnce(error).mockResolvedValue(undefined), + }; + httpClient.get.mockImplementation(async (_url, headers) => + headers["If-None-Match"] ? notModified : modified(), + ); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + + expect(client.getFlag({}, "flag").isEnabled).toBe(true); + expect(client["flagsCache"].getETag()).toBeUndefined(); + expect(provider.save).toHaveBeenCalledTimes(1); + expect(logger.error).toHaveBeenCalledWith( + "flagsFallbackProvider: failed to save flag definitions", + error, + ); + await refresh(client); + + expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); + expect(provider.save).toHaveBeenCalledTimes(2); + expect(client["flagsCache"].getETag()).toBe(etag); + await refresh(client); + + expect(requestHeaders(3)["If-None-Match"]).toBe(etag); + expect(provider.save).toHaveBeenCalledTimes(2); + }); + + it("publishes live definitions without waiting for a pending fallback save", async () => { + let completeSave!: () => void; + const save = new Promise((resolve) => { + completeSave = resolve; + }); + const provider = { load: vi.fn(), save: vi.fn().mockReturnValue(save) }; + httpClient.get.mockResolvedValue(modified()); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + + expect(client.getFlag({}, "flag").isEnabled).toBe(true); + expect(client["flagsCache"].getETag()).toBeUndefined(); + completeSave(); + await vi.advanceTimersByTimeAsync(0); + expect(client["flagsCache"].getETag()).toBe(etag); + }); + it("revalidates an empty flag snapshot", async () => { httpClient.get .mockResolvedValueOnce({ From 9ea55f8138f777fdf646cc877734441ce75ed1f0 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 13:34:22 +0200 Subject: [PATCH 3/8] refactor(node-sdk): simplify conditional refresh handling --- packages/node-sdk/src/client.ts | 34 ++++++++++------------ packages/node-sdk/src/flags-cache.ts | 26 +++++++---------- packages/node-sdk/test/flags-cache.test.ts | 6 ++-- 3 files changed, 28 insertions(+), 38 deletions(-) diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 7d1da0c2b..882c637e7 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -543,7 +543,7 @@ export class ReflagClient { const response = await this.getResponse( path, this._config.flagsFetchRetries, - { etag, recoverUnexpectedNotModified: true }, + { etag }, ); if (response?.status === 304) { return { notModified: true }; @@ -1212,17 +1212,15 @@ export class ReflagClient { private async getResponse( path: string, retries: number = 3, - options: { - etag?: string; - recoverUnexpectedNotModified?: boolean; - } = {}, + revalidation?: { etag?: string }, ) { ok(typeof path === "string" && path.length > 0, "path must be a string"); try { const url = this.buildUrl(path); - const headers = options.etag - ? { ...this._config.headers, "If-None-Match": options.etag } + const etag = revalidation?.etag; + const headers = etag + ? { ...this._config.headers, "If-None-Match": etag } : this._config.headers; let recoveredNotModified = false; const request = async (requestHeaders: Record) => { @@ -1236,19 +1234,17 @@ export class ReflagClient { return await withRetry( async () => { let response = await request(headers); - if (response.status === 304 && options.etag) { - return response; - } + if (response.status === 304 && revalidation) { + if (etag) { + return response; + } - // An unconditional request should not receive a 304. Try once more - // without a validator before falling back to normal error handling. - if ( - response.status === 304 && - options.recoverUnexpectedNotModified && - !recoveredNotModified - ) { - recoveredNotModified = true; - response = await request(this._config.headers); + // An unconditional request should not receive a 304. Recover + // once before falling back to normal error handling. + if (!recoveredNotModified) { + recoveredNotModified = true; + response = await request(this._config.headers); + } } if ( diff --git a/packages/node-sdk/src/flags-cache.ts b/packages/node-sdk/src/flags-cache.ts index da1a69a1c..c3edd3970 100644 --- a/packages/node-sdk/src/flags-cache.ts +++ b/packages/node-sdk/src/flags-cache.ts @@ -278,7 +278,6 @@ export class FlagsCache { return; } - this.clearSatisfiedPendingVersion(); this.lastRefreshAt = Date.now(); this.logger?.debug("flag definitions unchanged"); return; @@ -292,20 +291,17 @@ export class FlagsCache { this.value = result.definitions; this.flagStateVersion = result.flagStateVersion; - this.etag = typeof result.etag === "string" ? result.etag : undefined; - if (result.etag && typeof result.etag !== "string") { - void result.etag.then( - (etag) => { - // A late save must not attach its validator to a newer snapshot - // or restore it after destruction. - if (this.value === result.definitions) { - this.etag = etag; - } - }, - (error) => - this.logger?.error("flag definition ETag unavailable", error), - ); - } + this.etag = undefined; + void Promise.resolve(result.etag).then( + (etag) => { + // A late save must not attach its validator to a newer snapshot + // or restore it after destruction. + if (this.value === result.definitions) { + this.etag = etag; + } + }, + (error) => this.logger?.error("flag definition ETag unavailable", error), + ); this.lastRefreshAt = Date.now(); this.logger?.info("refreshed flag definitions"); } diff --git a/packages/node-sdk/test/flags-cache.test.ts b/packages/node-sdk/test/flags-cache.test.ts index 50ec1948d..785201e8f 100644 --- a/packages/node-sdk/test/flags-cache.test.ts +++ b/packages/node-sdk/test/flags-cache.test.ts @@ -8,13 +8,11 @@ function createDefinitions(key: string) { function createDeferred() { let resolve!: (value: T) => void; - let reject!: (reason?: unknown) => void; - const promise = new Promise((innerResolve, innerReject) => { + const promise = new Promise((innerResolve) => { resolve = innerResolve; - reject = innerReject; }); - return { promise, resolve, reject }; + return { promise, resolve }; } function scheduleTrailingRefresh(delayMs: number, callback: () => void) { From 347a0f8f2737b1d304fc74ba14f80f43c571b9ac Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 13:47:49 +0200 Subject: [PATCH 4/8] fix(node-sdk): reject older snapshots before fallback saves --- packages/node-sdk/src/client.ts | 4 +++ .../node-sdk/test/flags-revalidation.test.ts | 29 +++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 882c637e7..65beafe68 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -564,6 +564,10 @@ export class ReflagClient { : undefined; } + if (flagStateVersion < (this.flagsCache.getFlagStateVersion() ?? -1)) { + return undefined; + } + const responseETag = Object.entries(response?.headers ?? {}).find( ([name]) => name.toLowerCase() === "etag", )?.[1]; diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts index 4390e7475..1f4430f66 100644 --- a/packages/node-sdk/test/flags-revalidation.test.ts +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -287,6 +287,35 @@ describe("flag-definition revalidation", () => { expect(requestHeaders(3)).not.toHaveProperty("If-None-Match"); }); + it("does not overwrite fallback storage with a rejected older snapshot before a 304", async () => { + const current = modified(2, '"v2"'); + const provider = { + load: vi.fn(), + save: vi.fn().mockResolvedValue(undefined), + }; + httpClient.get + .mockResolvedValueOnce(current) + .mockResolvedValueOnce({ + ...modified(1, '"v1"'), + body: { success: true, flagStateVersion: 1, features: [] }, + }) + .mockResolvedValueOnce(notModified); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + const snapshot = client["flagsCache"].get(); + await refresh(client); + await refresh(client); + + expect(client["flagsCache"].get()).toBe(snapshot); + expect(client["flagsCache"].getFlagStateVersion()).toBe(2); + expect(client["flagsCache"].getETag()).toBe('"v2"'); + expect(requestHeaders(3)["If-None-Match"]).toBe('"v2"'); + expect(provider.save).toHaveBeenCalledTimes(1); + expect(provider.save.mock.calls[0]?.[1].flags).toEqual( + current.body.features, + ); + }); + it.each([ ["an HTTP error", { ok: false, status: 500, body: { success: false } }], [ From 88ae81977d245541dc4497427ca7445149d806cf Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 14:00:59 +0200 Subject: [PATCH 5/8] refactor(node-sdk): expose ETag metadata without copying headers --- .changeset/light-flags-revalidate.md | 2 +- packages/node-sdk/README.md | 6 +++--- packages/node-sdk/src/client.ts | 4 +--- packages/node-sdk/src/fetch-http-client.ts | 8 +++----- packages/node-sdk/src/types.ts | 4 ++-- packages/node-sdk/test/client.test.ts | 2 +- packages/node-sdk/test/fetch-http-client.test.ts | 10 +++++----- packages/node-sdk/test/flags-revalidation.test.ts | 6 +++--- 8 files changed, 19 insertions(+), 23 deletions(-) diff --git a/.changeset/light-flags-revalidate.md b/.changeset/light-flags-revalidate.md index de71a8ac5..abeab6324 100644 --- a/.changeset/light-flags-revalidate.md +++ b/.changeset/light-flags-revalidate.md @@ -2,4 +2,4 @@ "@reflag/node-sdk": patch --- -Use ETags when refreshing flag definitions to avoid downloading and recompiling unchanged flags. Custom HTTP clients can opt in by returning response headers. +Use ETags when refreshing flag definitions to avoid downloading and recompiling unchanged flags. Custom HTTP clients can opt in by returning the response ETag. diff --git a/packages/node-sdk/README.md b/packages/node-sdk/README.md index a5896ac61..67924ed3c 100644 --- a/packages/node-sdk/README.md +++ b/packages/node-sdk/README.md @@ -189,9 +189,9 @@ configured, the SDK retains the ETag only after the snapshot is saved successfully. A failed or pending save keeps ordinary refreshes unconditional so persistence can recover, without delaying flag availability. -Custom HTTP clients can enable this optimization by returning the ETag in -`HttpClientResponse.headers` and returning 304 responses with `body: undefined`. -Clients that omit response headers continue using full fetches. +Custom HTTP clients can enable this optimization by returning the exact ETag +value in `HttpClientResponse.etag` and returning 304 responses with +`body: undefined`. Clients that omit the ETag continue using full fetches. It's also possible to get the currently in use flag definitions: diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 65beafe68..0b9dd3ff9 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -568,9 +568,7 @@ export class ReflagClient { return undefined; } - const responseETag = Object.entries(response?.headers ?? {}).find( - ([name]) => name.toLowerCase() === "etag", - )?.[1]; + const responseETag = response?.etag; const saved = this.saveFlagsFallbackDefinitions(res.features); this.canLoadFlagsFallbackProvider = false; return { diff --git a/packages/node-sdk/src/fetch-http-client.ts b/packages/node-sdk/src/fetch-http-client.ts index cda66ffe9..787e969e3 100644 --- a/packages/node-sdk/src/fetch-http-client.ts +++ b/packages/node-sdk/src/fetch-http-client.ts @@ -51,14 +51,12 @@ const fetchClient: HttpClient = { cache: "no-cache", }); + const body = response.status === 304 ? undefined : await response.json(); return { ok: response.ok, status: response.status, - body: - response.status === 304 - ? undefined - : ((await response.json()) as TResponse), - headers: Object.fromEntries(response.headers), + body: body as TResponse | undefined, + etag: response.headers.get("etag") ?? undefined, }; }, }; diff --git a/packages/node-sdk/src/types.ts b/packages/node-sdk/src/types.ts index 2c6698258..6f109c515 100644 --- a/packages/node-sdk/src/types.ts +++ b/packages/node-sdk/src/types.ts @@ -518,10 +518,10 @@ export type HttpClientResponse = { body: TResponse | undefined; /** - * Response headers, if available. Include the ETag header to enable + * The response's ETag, if available. Return its exact value to enable * conditional flag-definition refreshes with a custom HTTP client. **/ - headers?: Record; + etag?: string; }; /** diff --git a/packages/node-sdk/test/client.test.ts b/packages/node-sdk/test/client.test.ts index eca982365..02b0fe490 100644 --- a/packages/node-sdk/test/client.test.ts +++ b/packages/node-sdk/test/client.test.ts @@ -1196,7 +1196,7 @@ describe("ReflagClient", () => { success: true, ...flagDefinitions, }, - headers: { etag: 'W/"flags-v1"' }, + etag: 'W/"flags-v1"', }) .mockResolvedValue({ ok: false, status: 304, body: undefined }); diff --git a/packages/node-sdk/test/fetch-http-client.test.ts b/packages/node-sdk/test/fetch-http-client.test.ts index 420c54ed3..f1ffeb6f8 100644 --- a/packages/node-sdk/test/fetch-http-client.test.ts +++ b/packages/node-sdk/test/fetch-http-client.test.ts @@ -56,7 +56,7 @@ describe("fetchClient", () => { ok: true, status: 200, body: { success: true }, - headers: { etag: 'W/"flags-v1"' }, + etag: 'W/"flags-v1"', }; global.fetch = vi.fn().mockResolvedValue({ @@ -146,7 +146,7 @@ describe("fetchClient", () => { ok: false, status: 400, body: { error: "Something went wrong" }, - headers: {}, + etag: undefined, }; global.fetch = vi.fn().mockResolvedValue({ @@ -175,7 +175,7 @@ describe("fetchClient", () => { ok: false, status: 304, body: undefined, - headers: { etag: 'W/"flags-v1"' }, + etag: 'W/"flags-v1"', }); expect(json).not.toHaveBeenCalled(); }); @@ -211,12 +211,12 @@ describe("fetchClient", () => { const initial = await fetchClient.get(originUrl, headers, 5000); const revalidated = await fetchClient.get( originUrl, - { ...headers, "If-None-Match": initial.headers!.etag! }, + { ...headers, "If-None-Match": initial.etag! }, 5000, ); expect(initial.status).toBe(200); - expect(initial.headers?.etag).toBe(etag); + expect(initial.etag).toBe(etag); expect(revalidated).toMatchObject({ status: 304, ok: false, diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts index 1f4430f66..1f957eb38 100644 --- a/packages/node-sdk/test/flags-revalidation.test.ts +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -25,7 +25,7 @@ function modified(version = 1, responseETag: string | undefined = etag) { ok: true, status: 200, body: { success: true, ...definitions, flagStateVersion: version }, - headers: responseETag ? { ETag: responseETag } : undefined, + etag: responseETag, }; } @@ -257,7 +257,7 @@ describe("flag-definition revalidation", () => { expect(requestHeaders(2)).not.toHaveProperty("If-None-Match"); }); - it("keeps custom clients without response headers working", async () => { + it("keeps custom clients without an ETag working", async () => { httpClient.get.mockResolvedValue({ ok: true, status: 200, @@ -275,7 +275,7 @@ describe("flag-definition revalidation", () => { it("clears the old ETag when an accepted replacement omits it", async () => { httpClient.get .mockResolvedValueOnce(modified()) - .mockResolvedValueOnce({ ...modified(2), headers: undefined }) + .mockResolvedValueOnce({ ...modified(2), etag: undefined }) .mockResolvedValueOnce(modified(3)); const client = createClient(); await client.initialize(); From 14872aac5c9b0da2227f5fbcc40956ca06f5a049 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 14:26:37 +0200 Subject: [PATCH 6/8] refactor(node-sdk): keep validators owned by cache snapshots --- packages/node-sdk/src/client.ts | 21 +++--- packages/node-sdk/src/flags-cache.ts | 72 +++++++------------ packages/node-sdk/test/flags-cache.test.ts | 31 ++++---- .../node-sdk/test/flags-revalidation.test.ts | 37 ++++++++++ 4 files changed, 87 insertions(+), 74 deletions(-) diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 0b9dd3ff9..6dcc39434 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -20,7 +20,7 @@ import { SDK_VERSION_HEADER_NAME, } from "./config"; import fetchClient, { withRetry } from "./fetch-http-client"; -import { FlagsCache } from "./flags-cache"; +import { FlagsCache, type FlagsCacheSnapshot } from "./flags-cache"; import { createFlagsSyncController, FlagsSyncController, @@ -568,17 +568,20 @@ export class ReflagClient { return undefined; } - const responseETag = response?.etag; - const saved = this.saveFlagsFallbackDefinitions(res.features); - this.canLoadFlagsFallbackProvider = false; - return { + const snapshot: FlagsCacheSnapshot = { definitions: compileFlagDefinitions(res.features), flagStateVersion, - // Publish definitions immediately, but only enable conditional - // refreshes after persistence succeeds. A failed save leaves the - // next ordinary refresh unconditional so it naturally tries again. - etag: saved.then((success) => (success ? responseETag : undefined)), }; + const responseETag = response?.etag; + // Keep flags available while saving. Late saves only update their own + // snapshot, never the replacement snapshot in the cache. + void this.saveFlagsFallbackDefinitions(res.features).then((saved) => { + if (saved) { + snapshot.etag = responseETag; + } + }); + this.canLoadFlagsFallbackProvider = false; + return snapshot; }, { logger: this.logger, diff --git a/packages/node-sdk/src/flags-cache.ts b/packages/node-sdk/src/flags-cache.ts index c3edd3970..268add0d6 100644 --- a/packages/node-sdk/src/flags-cache.ts +++ b/packages/node-sdk/src/flags-cache.ts @@ -2,14 +2,14 @@ import type { CachedFlagDefinition, Logger } from "./types"; const DEFAULT_MIN_REFRESH_INTERVAL_MS = 1000; -type FlagsCacheRefreshResult = - | { - definitions: CachedFlagDefinition[]; - flagStateVersion?: number; - etag?: string | Promise; - notModified?: false; - } - | { notModified: true }; +/** ETags belong to their snapshot, even when persistence publishes them later. */ +export type FlagsCacheSnapshot = { + readonly definitions: CachedFlagDefinition[]; + readonly flagStateVersion?: number; + etag?: string; +}; + +type FlagsCacheRefreshResult = FlagsCacheSnapshot | { notModified: true }; type CancelScheduledRefresh = () => void; @@ -40,9 +40,7 @@ type FlagsCacheOptions = { * keep the pending refresh queued until the next caller touches the cache. */ export class FlagsCache { - private value: CachedFlagDefinition[] | undefined; - private flagStateVersion: number | undefined; - private etag: string | undefined; + private snapshot: FlagsCacheSnapshot | undefined; private refreshPromise: Promise | undefined; private scheduledRefresh: ScheduledRefresh | undefined; private lastRefreshAt: number | undefined; @@ -69,26 +67,26 @@ export class FlagsCache { } public get() { - return this.value; + return this.snapshot?.definitions; } public getFlagStateVersion() { - return this.flagStateVersion; + return this.snapshot?.flagStateVersion; } public getETag() { - return this.etag; + return this.snapshot?.etag; } public async refresh(waitForVersion?: number) { if (this.destroyed) { - return this.value; + return this.get(); } this.queueRefresh(waitForVersion); this.ensureRefreshStartedOrScheduled(); await this.waitForQueuedWork(); - return this.value; + return this.get(); } public async waitRefresh() { @@ -97,9 +95,7 @@ export class FlagsCache { public destroy() { this.destroyed = true; - this.value = undefined; - this.flagStateVersion = undefined; - this.etag = undefined; + this.snapshot = undefined; this.refreshPromise = undefined; this.cancelScheduledRefresh(); this.pendingFullRefresh = false; @@ -117,10 +113,8 @@ export class FlagsCache { // can wait for. private queueRefresh(waitForVersion?: number) { if (waitForVersion !== undefined) { - if ( - this.flagStateVersion !== undefined && - this.flagStateVersion >= waitForVersion - ) { + const currentVersion = this.getFlagStateVersion(); + if (currentVersion !== undefined && currentVersion >= waitForVersion) { return; } @@ -159,14 +153,12 @@ export class FlagsCache { } private shouldApplyRefreshResult(flagStateVersion?: number) { + const currentVersion = this.getFlagStateVersion(); if (flagStateVersion === undefined) { - return this.flagStateVersion === undefined; + return currentVersion === undefined; } - return ( - this.flagStateVersion === undefined || - flagStateVersion >= this.flagStateVersion - ); + return currentVersion === undefined || flagStateVersion >= currentVersion; } private clearSatisfiedPendingVersion(seenFlagStateVersion?: number) { @@ -175,7 +167,7 @@ export class FlagsCache { } const latestKnownVersion = Math.max( - this.flagStateVersion ?? -1, + this.getFlagStateVersion() ?? -1, seenFlagStateVersion ?? -1, ); @@ -269,12 +261,8 @@ export class FlagsCache { return; } - if (result.notModified) { - if ( - this.value === undefined || - !this.etag || - waitForVersion !== undefined - ) { + if ("notModified" in result) { + if (!this.snapshot?.etag || waitForVersion !== undefined) { return; } @@ -289,19 +277,7 @@ export class FlagsCache { return; } - this.value = result.definitions; - this.flagStateVersion = result.flagStateVersion; - this.etag = undefined; - void Promise.resolve(result.etag).then( - (etag) => { - // A late save must not attach its validator to a newer snapshot - // or restore it after destruction. - if (this.value === result.definitions) { - this.etag = etag; - } - }, - (error) => this.logger?.error("flag definition ETag unavailable", error), - ); + this.snapshot = result; this.lastRefreshAt = Date.now(); this.logger?.info("refreshed flag definitions"); } diff --git a/packages/node-sdk/test/flags-cache.test.ts b/packages/node-sdk/test/flags-cache.test.ts index 785201e8f..6b46b432d 100644 --- a/packages/node-sdk/test/flags-cache.test.ts +++ b/packages/node-sdk/test/flags-cache.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { FlagsCache } from "../src/flags-cache"; +import { FlagsCache, type FlagsCacheSnapshot } from "../src/flags-cache"; function createDefinitions(key: string) { return [{ key }] as any; @@ -141,16 +141,16 @@ describe("FlagsCache", () => { ])( "does not replace a newer validator when an older save %s", async (_, etag) => { - const savedETag = createDeferred(); + const definitions = createDefinitions("unchanged"); + const older: FlagsCacheSnapshot = { + definitions, + flagStateVersion: 1, + }; const fetchFlags = vi .fn() + .mockResolvedValueOnce(older) .mockResolvedValueOnce({ - definitions: createDefinitions("older"), - flagStateVersion: 1, - etag: savedETag.promise, - }) - .mockResolvedValueOnce({ - definitions: createDefinitions("newer"), + definitions, flagStateVersion: 2, etag: '"newer"', }); @@ -158,25 +158,22 @@ describe("FlagsCache", () => { await cache.refresh(); expect(cache.getETag()).toBeUndefined(); await cache.refresh(); - savedETag.resolve(etag); - await savedETag.promise; + older.etag = etag; expect(cache.getFlagStateVersion()).toBe(2); expect(cache.getETag()).toBe('"newer"'); }, ); - it("does not restore a deferred validator after destruction", async () => { - const savedETag = createDeferred(); - const cache = new FlagsCache(async () => ({ + it("does not restore a late validator after destruction", async () => { + const snapshot: FlagsCacheSnapshot = { definitions: createDefinitions("first"), flagStateVersion: 1, - etag: savedETag.promise, - })); + }; + const cache = new FlagsCache(async () => snapshot); await cache.refresh(); cache.destroy(); - savedETag.resolve('"first"'); - await savedETag.promise; + snapshot.etag = '"first"'; expect(cache.get()).toBeUndefined(); expect(cache.getETag()).toBeUndefined(); diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts index 1f957eb38..ea5951092 100644 --- a/packages/node-sdk/test/flags-revalidation.test.ts +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -185,6 +185,43 @@ describe("flag-definition revalidation", () => { expect(client["flagsCache"].getETag()).toBe(etag); }); + it("keeps late fallback saves scoped to their original snapshots", async () => { + let completeFirstSave!: () => void; + let completeSecondSave!: () => void; + const provider = { + load: vi.fn(), + save: vi + .fn() + .mockReturnValueOnce( + new Promise((resolve) => { + completeFirstSave = resolve; + }), + ) + .mockReturnValueOnce( + new Promise((resolve) => { + completeSecondSave = resolve; + }), + ), + }; + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce(modified(2, '"v2"')); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + await refresh(client); + + completeFirstSave(); + await vi.advanceTimersByTimeAsync(0); + expect(client["flagsCache"].getFlagStateVersion()).toBe(2); + expect(client["flagsCache"].getETag()).toBeUndefined(); + + client.destroy(); + completeSecondSave(); + await vi.advanceTimersByTimeAsync(0); + expect(client["flagsCache"].get()).toBeUndefined(); + expect(client["flagsCache"].getETag()).toBeUndefined(); + }); + it("revalidates an empty flag snapshot", async () => { httpClient.get .mockResolvedValueOnce({ From ae59c3f336097853ff2b5e77ea153b01d9da84e3 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 15:08:59 +0200 Subject: [PATCH 7/8] refactor(node-sdk): simplify refresh state and persistence flow --- packages/node-sdk/src/client.ts | 73 ++++++++----------- packages/node-sdk/src/flags-cache.ts | 55 +++++--------- packages/node-sdk/test/flags-cache.test.ts | 2 + .../node-sdk/test/flags-revalidation.test.ts | 15 ++++ 4 files changed, 65 insertions(+), 80 deletions(-) diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 6dcc39434..1d3713c16 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -572,14 +572,11 @@ export class ReflagClient { definitions: compileFlagDefinitions(res.features), flagStateVersion, }; - const responseETag = response?.etag; - // Keep flags available while saving. Late saves only update their own - // snapshot, never the replacement snapshot in the cache. - void this.saveFlagsFallbackDefinitions(res.features).then((saved) => { - if (saved) { - snapshot.etag = responseETag; - } - }); + void this.saveFlagsFallbackDefinitions( + res.features, + snapshot, + response?.etag, + ); this.canLoadFlagsFallbackProvider = false; return snapshot; }, @@ -649,24 +646,27 @@ export class ReflagClient { } } - private async saveFlagsFallbackDefinitions(flags: FlagAPIResponse[]) { - const provider = this._config.flagsFallbackProvider; - if (!provider) { - return true; - } - + private async saveFlagsFallbackDefinitions( + flags: FlagAPIResponse[], + snapshot: FlagsCacheSnapshot, + etag?: string, + ) { try { - await provider.save( - this._config.flagsFallbackProviderContext, - createFlagsFallbackSnapshot(flags), - ); - return true; + const provider = this._config.flagsFallbackProvider; + if (provider) { + await provider.save( + this._config.flagsFallbackProviderContext, + createFlagsFallbackSnapshot(flags), + ); + } + // Enable revalidation only after saving. A late save updates only its + // own snapshot, never a replacement or destroyed cache. + snapshot.etag = etag; } catch (error) { this.logger.error( "flagsFallbackProvider: failed to save flag definitions", error, ); - return false; } } @@ -1204,16 +1204,6 @@ export class ReflagClient { * @returns The response from the server. * @throws An error if the path is invalid. **/ - private async get(path: string, retries: number = 3) { - const response = await this.getResponse(path, retries); - if (!response?.body) { - return undefined; - } - - const { success: _, ...result } = response.body; - return result as TResponse; - } - private async getResponse( path: string, retries: number = 3, @@ -1227,7 +1217,7 @@ export class ReflagClient { const headers = etag ? { ...this._config.headers, "If-None-Match": etag } : this._config.headers; - let recoveredNotModified = false; + let recoveryAvailable = Boolean(revalidation && !etag); const request = async (requestHeaders: Record) => { const response = await this.httpClient.get< TResponse & { success: boolean } @@ -1239,17 +1229,15 @@ export class ReflagClient { return await withRetry( async () => { let response = await request(headers); - if (response.status === 304 && revalidation) { - if (etag) { - return response; - } + if (response.status === 304 && etag) { + return response; + } - // An unconditional request should not receive a 304. Recover - // once before falling back to normal error handling. - if (!recoveredNotModified) { - recoveredNotModified = true; - response = await request(this._config.headers); - } + // An unconditional definition request should not receive a 304. + // Recover once before falling back to normal error handling. + if (response.status === 304 && recoveryAvailable) { + recoveryAvailable = false; + response = await request(this._config.headers); } if ( @@ -1767,9 +1755,10 @@ export class ReflagClient { } const query = params.toString(); - const res = await this.get( + const response = await this.getResponse( `features/evaluated${query ? `?${query}` : ""}`, ); + const res = response?.body; if (key) { const flag = res?.features[key]; diff --git a/packages/node-sdk/src/flags-cache.ts b/packages/node-sdk/src/flags-cache.ts index 268add0d6..21713f918 100644 --- a/packages/node-sdk/src/flags-cache.ts +++ b/packages/node-sdk/src/flags-cache.ts @@ -47,8 +47,7 @@ export class FlagsCache { private lastRefreshStartedAt: number | undefined; private destroyed = false; - private pendingFullRefresh = false; - private pendingWaitForVersion: number | undefined; + private pendingRefresh: { waitForVersion?: number } | undefined; private readonly logger?: Logger; private readonly minRefreshIntervalMs: number; @@ -98,8 +97,7 @@ export class FlagsCache { this.snapshot = undefined; this.refreshPromise = undefined; this.cancelScheduledRefresh(); - this.pendingFullRefresh = false; - this.pendingWaitForVersion = undefined; + this.pendingRefresh = undefined; this.lastRefreshAt = undefined; this.lastRefreshStartedAt = undefined; } @@ -118,38 +116,17 @@ export class FlagsCache { return; } - this.pendingWaitForVersion = - this.pendingWaitForVersion === undefined - ? waitForVersion - : Math.max(this.pendingWaitForVersion, waitForVersion); - this.pendingFullRefresh = false; + const pendingVersion = this.pendingRefresh?.waitForVersion; + this.pendingRefresh = { + waitForVersion: + pendingVersion === undefined + ? waitForVersion + : Math.max(pendingVersion, waitForVersion), + }; return; } - if (this.pendingWaitForVersion !== undefined) { - return; - } - - this.pendingFullRefresh = true; - } - - private hasPendingRefresh() { - return this.pendingWaitForVersion !== undefined || this.pendingFullRefresh; - } - - private takeNextRefreshRequest() { - if (this.pendingWaitForVersion !== undefined) { - const waitForVersion = this.pendingWaitForVersion; - this.pendingWaitForVersion = undefined; - return { waitForVersion }; - } - - if (!this.pendingFullRefresh) { - return undefined; - } - - this.pendingFullRefresh = false; - return { waitForVersion: undefined }; + this.pendingRefresh ??= {}; } private shouldApplyRefreshResult(flagStateVersion?: number) { @@ -162,7 +139,8 @@ export class FlagsCache { } private clearSatisfiedPendingVersion(seenFlagStateVersion?: number) { - if (this.pendingWaitForVersion === undefined) { + const pendingVersion = this.pendingRefresh?.waitForVersion; + if (pendingVersion === undefined) { return; } @@ -171,8 +149,8 @@ export class FlagsCache { seenFlagStateVersion ?? -1, ); - if (latestKnownVersion >= this.pendingWaitForVersion) { - this.pendingWaitForVersion = undefined; + if (latestKnownVersion >= pendingVersion) { + this.pendingRefresh = undefined; } } @@ -226,7 +204,7 @@ export class FlagsCache { } private ensureRefreshStartedOrScheduled() { - if (this.destroyed || this.refreshPromise || !this.hasPendingRefresh()) { + if (this.destroyed || this.refreshPromise || !this.pendingRefresh) { return; } @@ -241,11 +219,12 @@ export class FlagsCache { } private startNextRefresh() { - const request = this.takeNextRefreshRequest(); + const request = this.pendingRefresh; if (!request) { return; } + this.pendingRefresh = undefined; this.lastRefreshStartedAt = Date.now(); this.refreshPromise = this.fetchAndApplyRefresh( request.waitForVersion, diff --git a/packages/node-sdk/test/flags-cache.test.ts b/packages/node-sdk/test/flags-cache.test.ts index 6b46b432d..efc775c80 100644 --- a/packages/node-sdk/test/flags-cache.test.ts +++ b/packages/node-sdk/test/flags-cache.test.ts @@ -47,6 +47,7 @@ describe("FlagsCache", () => { void cache.refresh(21); void cache.refresh(22); + void cache.refresh(); firstRefresh.resolve({ definitions: createDefinitions("first"), @@ -59,6 +60,7 @@ describe("FlagsCache", () => { }); await refreshPromise; + expect(fetchFlags).toHaveBeenCalledTimes(2); expect(cache.get()).toEqual(createDefinitions("newest")); }); diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts index ea5951092..e00ab6e1c 100644 --- a/packages/node-sdk/test/flags-revalidation.test.ts +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -448,6 +448,21 @@ describe("flag-definition revalidation", () => { expect(client["flagsCache"].getLastRefreshAt()).toBeUndefined(); }); + it("performs only one unexpected-304 recovery across ordinary retries", async () => { + httpClient.get.mockResolvedValue(notModified); + const client = createClient({ flagsFetchRetries: 2 }); + const initialized = client.initialize(); + await vi.advanceTimersByTimeAsync(10000); + await initialized; + + expect(httpClient.get).toHaveBeenCalledTimes(4); + for (const [, headers] of httpClient.get.mock.calls) { + expect(headers).not.toHaveProperty("If-None-Match"); + } + expect(client["flagsCache"].get()).toBeUndefined(); + expect(client["flagsCache"].getLastRefreshAt()).toBeUndefined(); + }); + it("fetches unconditionally after loading fallback definitions", async () => { const provider = { load: vi.fn().mockResolvedValue({ From 7104d8425604d4ec60eca1305c5ffe5ca9f85246 Mon Sep 17 00:00:00 2001 From: Ron Cohen Date: Mon, 12 Oct 2026 18:45:39 +0200 Subject: [PATCH 8/8] fix(node-sdk): serialize background fallback saves --- packages/node-sdk/src/client.ts | 44 +++++++------ .../node-sdk/test/flags-revalidation.test.ts | 61 ++++++++++++++++++- 2 files changed, 85 insertions(+), 20 deletions(-) diff --git a/packages/node-sdk/src/client.ts b/packages/node-sdk/src/client.ts index 1d3713c16..cc4c9f3f9 100644 --- a/packages/node-sdk/src/client.ts +++ b/packages/node-sdk/src/client.ts @@ -292,6 +292,7 @@ export class ReflagClient { private flagsSyncController: FlagsSyncController; private initializationFinished = false; private canLoadFlagsFallbackProvider = true; + private flagsFallbackSavePromise = Promise.resolve(); private _initialize = once(async () => { const start = Date.now(); if (!this._config.offline) { @@ -572,7 +573,7 @@ export class ReflagClient { definitions: compileFlagDefinitions(res.features), flagStateVersion, }; - void this.saveFlagsFallbackDefinitions( + this.saveFlagsFallbackDefinitions( res.features, snapshot, response?.etag, @@ -646,28 +647,33 @@ export class ReflagClient { } } - private async saveFlagsFallbackDefinitions( + private saveFlagsFallbackDefinitions( flags: FlagAPIResponse[], snapshot: FlagsCacheSnapshot, etag?: string, ) { - try { - const provider = this._config.flagsFallbackProvider; - if (provider) { - await provider.save( - this._config.flagsFallbackProviderContext, - createFlagsFallbackSnapshot(flags), - ); - } - // Enable revalidation only after saving. A late save updates only its - // own snapshot, never a replacement or destroyed cache. - snapshot.etag = etag; - } catch (error) { - this.logger.error( - "flagsFallbackProvider: failed to save flag definitions", - error, - ); - } + // Serialize background writes so an older save cannot overwrite a newer one. + this.flagsFallbackSavePromise = this.flagsFallbackSavePromise.then( + async () => { + try { + const provider = this._config.flagsFallbackProvider; + if (provider) { + await provider.save( + this._config.flagsFallbackProviderContext, + createFlagsFallbackSnapshot(flags), + ); + } + // Enable revalidation only after saving. A late save updates only its + // own snapshot, never a replacement or destroyed cache. + snapshot.etag = etag; + } catch (error) { + this.logger.error( + "flagsFallbackProvider: failed to save flag definitions", + error, + ); + } + }, + ); } /** diff --git a/packages/node-sdk/test/flags-revalidation.test.ts b/packages/node-sdk/test/flags-revalidation.test.ts index e00ab6e1c..7483d79f4 100644 --- a/packages/node-sdk/test/flags-revalidation.test.ts +++ b/packages/node-sdk/test/flags-revalidation.test.ts @@ -2,7 +2,11 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { ReflagClient } from "../src/client"; import { API_TIMEOUT_MS, FLAGS_REFETCH_MS } from "../src/config"; -import type { ClientOptions, FlagsAPIResponse } from "../src/types"; +import type { + ClientOptions, + FlagsAPIResponse, + FlagsFallbackSnapshot, +} from "../src/types"; const etag = 'W/"flags-v1"'; const notModified = { ok: false, status: 304, body: undefined }; @@ -185,6 +189,61 @@ describe("flag-definition revalidation", () => { expect(client["flagsCache"].getETag()).toBe(etag); }); + it.each(["succeeds", "fails"])( + "serializes fallback writes when the first save %s, keeping the latest snapshot through 304s", + async (outcome) => { + let completeFirstSave!: () => void; + let rejectFirstSave!: (error: Error) => void; + const firstSave = new Promise((resolve, reject) => { + completeFirstSave = resolve; + rejectFirstSave = reject; + }); + let stored: FlagsFallbackSnapshot | undefined; + const provider = { + load: vi.fn(), + save: vi.fn( + async (_context: unknown, snapshot: FlagsFallbackSnapshot) => { + if (snapshot.flags.length) { + await firstSave; + } + stored = snapshot; + }, + ), + }; + httpClient.get + .mockResolvedValueOnce(modified()) + .mockResolvedValueOnce({ + ...modified(2, '"v2"'), + body: { success: true, flagStateVersion: 2, features: [] }, + }) + .mockResolvedValue(notModified); + const client = createClient({ flagsFallbackProvider: provider }); + await client.initialize(); + await refresh(client); + + expect(client["flagsCache"].get()).toEqual([]); + expect(client["flagsCache"].getFlagStateVersion()).toBe(2); + expect(client["flagsCache"].getETag()).toBeUndefined(); + expect(provider.save).toHaveBeenCalledTimes(1); + expect(stored).toBeUndefined(); + + if (outcome === "succeeds") { + completeFirstSave(); + } else { + rejectFirstSave(new Error("temporary storage failure")); + } + await vi.advanceTimersByTimeAsync(0); + expect(provider.save).toHaveBeenCalledTimes(2); + expect(stored?.flags).toEqual([]); + expect(client["flagsCache"].getETag()).toBe('"v2"'); + await refresh(client); + + expect(requestHeaders(3)["If-None-Match"]).toBe('"v2"'); + expect(provider.save).toHaveBeenCalledTimes(2); + expect(stored?.flags).toEqual([]); + }, + ); + it("keeps late fallback saves scoped to their original snapshots", async () => { let completeFirstSave!: () => void; let completeSecondSave!: () => void;