From 0f866c1fc8279e50bb9fa52cae0d97b721f61ce5 Mon Sep 17 00:00:00 2001 From: Fiona Date: Wed, 2 Sep 2026 05:21:27 -0700 Subject: [PATCH 1/2] fix(rum): sweep the settings entries of releases nobody runs The settings cache is keyed by application version, because two releases served at the same time are entitled to different rates and one entry between them would have each overwrite the other's at every fetch. The cost of that was an entry per release: nothing ever read or removed the one a previous release used, so on a site that deploys often they accumulated for good in a quota the host application shares. Every write now stamps when it happened, through the one path that writes an entry, and initialisation removes the entries nothing has refreshed for two days. Age is the only thing that can tell an abandoned entry from the entry of a tab still open on yesterday's release: a page that still reads its entry rewrites it at every session renewal, so the threshold only has to clear the longest session plus the longest outage worth surviving. The sweep runs before the first request rather than after each write. A session renewal is a hot path and localStorage is synchronous, and going first is what lets it free room on an origin that is already out of it - the very state the leak produces. This page's own entry is never a candidate: it holds the version floor that lets a late answer be refused. The one path that reaches an entry without storing anything - a response refused for carrying an older version - now rewrites it unchanged, so the entry a client is still asking for cannot be swept out from under it. --- CHANGELOG.md | 9 + .../configuration/remoteConfiguration.spec.ts | 99 ++++++++++- .../configuration/remoteConfiguration.ts | 157 +++++++++++++++--- 3 files changed, 241 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 056cc32b0b..e4c364446f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,15 @@ --- +## Unreleased + +- 🐛 The settings cache no longer grows by one entry per release of your site. Entries are keyed by + application version, because two releases served at the same time are entitled to different rates + — but the entry a previous release used was never read or removed again, so on a site that + deploys often they accumulated in the storage quota the page shares. An entry that nothing has + refreshed for two days is now removed when the SDK starts. A page still reading its entry + rewrites it at every session renewal, so only the entries of releases nobody runs are swept. + ## v0.2.1 - ✨ Two changes published from the console now end the running session, so they reach the visitor diff --git a/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts b/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts index 7ba6456c20..2ee8cf5824 100644 --- a/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts +++ b/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts @@ -1,4 +1,4 @@ -import { INTAKE_SITE_US1, ONE_SECOND, display, isIntakeUrl } from '@flashcatcloud/browser-core' +import { INTAKE_SITE_US1, ONE_DAY, ONE_SECOND, dateNow, display, isIntakeUrl } from '@flashcatcloud/browser-core' import type { Clock, MockXhr } from '@flashcatcloud/browser-core/test' import { interceptRequests, mockClock, registerCleanupTask } from '@flashcatcloud/browser-core/test' import { mockRumConfiguration } from '../../../test' @@ -749,6 +749,103 @@ describe('remoteConfiguration', () => { }) }) + describe('sweeping the entries of releases nobody runs', () => { + // The key carries the application version, so every release leaves one behind. Without a sweep + // they accumulate for good in a quota the host application shares. + const otherReleaseKey = buildRemoteConfigSetup({ ...INIT_CONFIGURATION, version: '0.9.0' })!.storeKey + const drawKey = buildDrawStoreKey(INIT_CONFIGURATION) + const foreignKey = 'a-key-the-host-application-owns' + + beforeEach(() => { + registerCleanupTask(() => { + localStorage.removeItem(otherReleaseKey) + localStorage.removeItem(drawKey) + localStorage.removeItem(foreignKey) + }) + }) + + function writeEntryAged(key: string, age: number, values: Record = { version: 4 }) { + localStorage.setItem(key, JSON.stringify({ ...values, t: dateNow() - age })) + } + + function writeTimeOf(key: string) { + return (JSON.parse(localStorage.getItem(key)!) as { t?: number }).t + } + + it('removes an entry nothing has refreshed for longer than the threshold', () => { + writeEntryAged(otherReleaseKey, 3 * ONE_DAY) + + start(configurationWith()) + + expect(localStorage.getItem(otherReleaseKey)).toBeNull() + }) + + it('keeps an entry a page refreshed recently, which is how a live one looks', () => { + writeEntryAged(otherReleaseKey, ONE_DAY) + + start(configurationWith()) + + expect(localStorage.getItem(otherReleaseKey)).not.toBeNull() + }) + + it('removes an entry left by a build that did not record when it was written', () => { + // Everything stored before the write time existed. Taken for abandoned rather than kept: the + // accumulated orphans are the whole reason this exists, and a page still on the old build + // writes its entry back at its next renewal. + localStorage.setItem(otherReleaseKey, JSON.stringify({ version: 4, sessionSampleRate: 42 })) + + start(configurationWith()) + + expect(localStorage.getItem(otherReleaseKey)).toBeNull() + }) + + it("never removes this page's own entry, however old it looks", () => { + // It holds the version floor that lets a late answer be refused, and the request this very + // initialisation is starting is about to read it. + localStorage.setItem(setup!.storeKey, JSON.stringify({ version: 8, sessionSampleRate: 42 })) + + start(configurationWith()) + + expect(readRemoteConfig(setup).version).toBe(8) + }) + + it('leaves alone every key it did not write', () => { + writeEntryAged(drawKey, 3 * ONE_DAY) + localStorage.setItem(foreignKey, 'not ours to parse') + + start(configurationWith()) + + expect(localStorage.getItem(drawKey)).not.toBeNull() + expect(localStorage.getItem(foreignKey)).toBe('not ours to parse') + }) + + it('records when an entry was written, so a later sweep can tell its age', (done) => { + interceptor.withMockXhr((xhr) => { + xhr.complete(200, body({ rum: { sessionSampleRate: 42 } })) + + expect(dateNow() - writeTimeOf(setup!.storeKey)!).toBeLessThan(ONE_SECOND) + done() + }) + start(configurationWith()) + }) + + it('refreshes the write time of an entry whose values it refuses', (done) => { + // The entry a client is stuck on when a server breaks the only-goes-up contract is the one + // entry no successful write refreshes. Without this its settings would be swept out from + // under it while it was still asking for them. + writeEntryAged(setup!.storeKey, 3 * ONE_DAY, { version: 8, sessionSampleRate: 42 }) + + interceptor.withMockXhr((xhr) => { + xhr.complete(200, body({ rum: { sessionSampleRate: 1 }, version: 7 })) + + expect(readRemoteConfig(setup)).toEqual({ version: 8, sessionSampleRate: 42 }) + expect(dateNow() - writeTimeOf(setup!.storeKey)!).toBeLessThan(ONE_SECOND) + done() + }) + start(configurationWith()) + }) + }) + describe('the storage key', () => { it('separates applications, environments and versions', () => { const keyOf = (partial: Partial) => diff --git a/packages/rum-core/src/domain/configuration/remoteConfiguration.ts b/packages/rum-core/src/domain/configuration/remoteConfiguration.ts index 190e6df96f..97b8d7a57e 100644 --- a/packages/rum-core/src/domain/configuration/remoteConfiguration.ts +++ b/packages/rum-core/src/domain/configuration/remoteConfiguration.ts @@ -2,9 +2,11 @@ import { addEventListener, clearTimeout, createEndpointUrlBuilder, + dateNow, display, noop, setTimeout, + ONE_DAY, ONE_SECOND, } from '@flashcatcloud/browser-core' import type { DefaultPrivacyLevel, TimeoutId } from '@flashcatcloud/browser-core' @@ -57,6 +59,22 @@ const STORE_KEY_PREFIX = '_fc_rc_1_' const DRAW_STORE_KEY_PREFIX = '_fc_draw_1_' const DEFAULT_FETCH_TIMEOUT = 3 * ONE_SECOND +/** + * How long an entry may go unrefreshed before `sweepAbandonedEntries` treats it as belonging to a + * release nobody is running any more. + * + * An entry that is still being read is also being rewritten: the page reading it refetches at every + * session renewal and stores the answer. So the threshold only has to clear the longest a live + * entry can legitimately stay silent, which is the longest session (four hours, after which a + * renewal refetches) plus the longest endpoint outage we are willing to survive without dropping + * anyone — a failed fetch stores nothing. Two days leaves better than a day and a half of outage, + * and still bounds the leak at the entries of two days of releases. + * + * Erring long is deliberate. Deleting an entry too early costs the page reading it one session on + * its init values; keeping a dead one costs a few hundred bytes. + */ +const STORE_ENTRY_MAX_AGE = 2 * ONE_DAY + /** * A failed fetch is retried quickly, then patiently, then not at all until the next natural * trigger (a new session, or the next page load). The budget is deliberately tiny — two extra @@ -311,6 +329,10 @@ function keepConfigFresh(configuration: RumConfiguration, setup: RemoteConfigSet const renewSubscription = lifeCycle.subscribe(LifeCycleEventType.SESSION_RENEWED, onTrigger) + // Before the first request, so the room the entries of dead releases are holding is free by the + // time there is an answer to store. See `sweepAbandonedEntries`. + sweepAbandonedEntries(setup.storeKey) + onTrigger() return () => { @@ -422,8 +444,16 @@ function store(setup: RemoteConfigSetup, response: RemoteConfigurationResponse) // // What it is compared against is storage, not a version held in memory here, because the two // requests that can cross are two pages, and storage is the only thing they share. - const storedVersion = readRemoteConfig(setup).version + const stored = readRemoteConfig(setup) + const storedVersion = stored.version if (storedVersion !== undefined && response.version < storedVersion) { + // Refused, but the entry is plainly still in use — a request was just made for it and answered. + // Rewriting it unchanged is what says so: its age is the only thing the sweep reads, and this + // is the one path that reaches an entry without storing anything. A client left here by a + // server that broke the only-goes-up contract would otherwise have the settings it is still + // asking for swept out from under it. Reading a version out of the entry proves it is there, + // so nothing needs to be checked before writing it back. + writeEntry(setup, stored) return false } @@ -461,21 +491,110 @@ function store(setup: RemoteConfigSetup, response: RemoteConfigurationResponse) values.custom = response.custom } + // Written even with nothing in it — that is what "remote configuration is off, use your own + // settings" looks like — so that the version is kept either way and the console can still see + // that this client is up to date with the change that turned it off. + return writeEntry(setup, values) && isNew +} + +/** + * What actually sits in storage: the values, plus when they were last written. + * + * `t` is not one of the values and is never handed on — `readStoredValues` drops it with everything + * else it does not recognise. It exists for `sweepAbandonedEntries` alone, which is why it is not + * spelled out on `RemoteConfigValues` where a reader would take it for something the server sends. + */ +interface StoredEntry extends RemoteConfigValues { + t: number +} + +/** + * The one place an entry is written, so that every entry carries the write time the sweep reads. + * + * Answers whether the values are now where the next draw will look for them. A failure — storage + * unavailable, or the origin out of room — leaves the previous entry exactly as it was, which is + * the same "keep what is already working" answer a failed request gets: the client goes on applying + * the settings it last stored, and goes on reporting their version. It is still reported as a + * failure, because nothing downstream may act on settings the next draw will not find. + */ +function writeEntry(setup: RemoteConfigSetup, values: RemoteConfigValues) { try { - // Written even with nothing in it — that is what "remote configuration is off, use your own - // settings" looks like — so that the version is kept either way and the console can still see - // that this client is up to date with the change that turned it off. - localStorage.setItem(setup.storeKey, JSON.stringify(values)) - return isNew + const entry: StoredEntry = { ...values, t: dateNow() } + localStorage.setItem(setup.storeKey, JSON.stringify(entry)) + return true } catch { - // Storage unavailable, or the origin is out of room. The previous entry stays as it is, which - // is the same "keep what is already working" answer a failed request gets — the client goes on - // applying the settings it last stored, and goes on reporting their version. Reported as a - // failure all the same: nothing downstream may act on settings the next draw will not find. return false } } +/** + * Delete the entries of releases nobody is running any more. + * + * The store key carries the application version, because two releases live at the same time are + * entitled to different rates and one entry between them would have each overwrite the other's at + * every fetch. The cost of that is an entry per release, and nothing ever read or removed them + * again — on a site that deploys daily they accumulate for good, in a quota the host application + * shares. + * + * Run once per initialisation rather than at every write. Sweeping on write was the shape tried + * first and it is the wrong one: `localStorage` is synchronous, a session renewal is a hot path, + * and the walk would repeat for no new information. Once per page also puts it *before* the first + * write, which is what lets it free room on an origin that is already out of it — the very state + * the leak produces. + * + * This page's own entry is never a candidate: it holds the version floor that lets a late answer + * be refused, and it is about to be read by the request this initialisation is starting. + * + * An entry with no write time at all was left by a build older than this one. It is taken for + * abandoned rather than stamped and kept, which is the trade this makes deliberately: stamping + * would mean a write per orphan on the first load after the upgrade, and the accumulated orphans + * are exactly what this exists to clear. What it costs is bounded — while two builds are live on + * one origin, a page still on the old one may have its entry swept and spend a single session on + * its init values before writing it back. + */ +function sweepAbandonedEntries(keepKey: string) { + try { + const now = dateNow() + const abandoned: string[] = [] + + // Collected in full before anything is removed: removing during the walk shifts the indices + // `key()` reads, and whatever slid into the freed slot would be stepped over. + for (let i = 0; i < localStorage.length; i += 1) { + const key = localStorage.key(i) + if (key === null || key === keepKey || key.indexOf(STORE_KEY_PREFIX) !== 0) { + continue + } + if (now - readWriteTime(key) > STORE_ENTRY_MAX_AGE) { + abandoned.push(key) + } + } + + abandoned.forEach((key) => localStorage.removeItem(key)) + } catch { + // Storage unavailable, or an entry that is not ours to parse. Housekeeping is never worth + // failing an initialisation over, and the next page load tries again. + } +} + +/** + * When the entry under `key` was last written, or 0 — older than any threshold — when it does not + * say. Anything in a browser profile can be edited by hand, so a time that is not a plain number is + * read as no time at all rather than trusted into the arithmetic above. + */ +function readWriteTime(key: string) { + try { + const stored = localStorage.getItem(key) + const parsed: unknown = stored ? JSON.parse(stored) : undefined + if (!parsed || typeof parsed !== 'object') { + return 0 + } + const { t } = parsed as Partial + return typeof t === 'number' && isFinite(t) ? t : 0 + } catch { + return 0 + } +} + export function buildRemoteConfigSetup(initConfiguration: RumInitConfiguration): RemoteConfigSetup | undefined { if (!initConfiguration.remoteConfigurationEnabled) { return undefined @@ -529,19 +648,11 @@ function validFetchTimeout(timeout: number | undefined) { * as long as both are being served — so the version has to stay, and cannot be dropped to make the * limitation below go away. * - * KNOWN LIMITATION - that costs an entry per deploy. The first session after a release reads the - * local settings, and the entry the release before it used is never read again and never removed, - * so they accumulate in a quota the host application shares. - * - * Sweeping them on write is not the answer, and was tried: nothing here can tell an abandoned entry - * from the entry of a tab still open on yesterday's release, and deleting the latter drops that tab - * to its local settings for a whole session — after which the two tabs delete each other's entry at - * every renewal, which is a worse failure than the leak. A correct fix needs a way to know that no - * page is still reading an entry: an age written beside the values would do it, and is the shape to - * reach for if the accumulation ever bites. Two things to get right if it is ever built — the - * threshold has to clear the longest session AND the longest plausible endpoint outage, since only - * a stored response refreshes the age, and the stale-version early return above skips that write, - * so it must refresh the age even when it declines the values. + * That costs an entry per release — the first session after one reads the local settings, and the + * entry the release before it used is never read again — so the entries are swept by age rather + * than left to accumulate in a quota the host application shares. See `sweepAbandonedEntries` for + * why age is the only thing that can tell an abandoned entry from the entry of a tab still open on + * yesterday's release. */ function buildStoreKey(initConfiguration: RumInitConfiguration) { return buildKey(STORE_KEY_PREFIX, identityParts(initConfiguration).concat(initConfiguration.version ?? '')) From 6ff981f4efd20f40fc8ff3056d82124355367504 Mon Sep 17 00:00:00 2001 From: Fiona Date: Fri, 4 Sep 2026 01:51:50 -0700 Subject: [PATCH 2/2] fix(rum): keep settings entries without a write time --- CHANGELOG.md | 7 ++-- .../configuration/remoteConfiguration.spec.ts | 9 ++--- .../configuration/remoteConfiguration.ts | 38 +++++++++---------- 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4c364446f..42b9a2ac7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,9 +23,10 @@ - 🐛 The settings cache no longer grows by one entry per release of your site. Entries are keyed by application version, because two releases served at the same time are entitled to different rates — but the entry a previous release used was never read or removed again, so on a site that - deploys often they accumulated in the storage quota the page shares. An entry that nothing has - refreshed for two days is now removed when the SDK starts. A page still reading its entry - rewrites it at every session renewal, so only the entries of releases nobody runs are swept. + deploys often they accumulated in the storage quota the page shares. New entries now record when + they were refreshed, and one left untouched for two days is removed when the SDK starts. Entries + written by older SDK builds are kept because they carry no refresh time, leaving a finite legacy + residue while preventing the cache from growing without bound. ## v0.2.1 diff --git a/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts b/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts index 2ee8cf5824..c3e409ab8c 100644 --- a/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts +++ b/packages/rum-core/src/domain/configuration/remoteConfiguration.spec.ts @@ -788,15 +788,14 @@ describe('remoteConfiguration', () => { expect(localStorage.getItem(otherReleaseKey)).not.toBeNull() }) - it('removes an entry left by a build that did not record when it was written', () => { - // Everything stored before the write time existed. Taken for abandoned rather than kept: the - // accumulated orphans are the whole reason this exists, and a page still on the old build - // writes its entry back at its next renewal. + it('keeps an entry left by a build that did not record when it was written', () => { + // An old build still using this origin cannot add a write time when it refreshes the entry, + // so absence alone cannot distinguish a live release from an abandoned one. localStorage.setItem(otherReleaseKey, JSON.stringify({ version: 4, sessionSampleRate: 42 })) start(configurationWith()) - expect(localStorage.getItem(otherReleaseKey)).toBeNull() + expect(localStorage.getItem(otherReleaseKey)).not.toBeNull() }) it("never removes this page's own entry, however old it looks", () => { diff --git a/packages/rum-core/src/domain/configuration/remoteConfiguration.ts b/packages/rum-core/src/domain/configuration/remoteConfiguration.ts index 97b8d7a57e..2c0ee781f9 100644 --- a/packages/rum-core/src/domain/configuration/remoteConfiguration.ts +++ b/packages/rum-core/src/domain/configuration/remoteConfiguration.ts @@ -63,12 +63,12 @@ const DEFAULT_FETCH_TIMEOUT = 3 * ONE_SECOND * How long an entry may go unrefreshed before `sweepAbandonedEntries` treats it as belonging to a * release nobody is running any more. * - * An entry that is still being read is also being rewritten: the page reading it refetches at every - * session renewal and stores the answer. So the threshold only has to clear the longest a live - * entry can legitimately stay silent, which is the longest session (four hours, after which a - * renewal refetches) plus the longest endpoint outage we are willing to survive without dropping - * anyone — a failed fetch stores nothing. Two days leaves better than a day and a half of outage, - * and still bounds the leak at the entries of two days of releases. + * An entry written by this SDK that is still being read is also being rewritten: the page reading + * it refetches at every session renewal and stores the answer. So the threshold only has to clear + * the longest a live entry can legitimately stay silent, which is the longest session (four hours, + * after which a renewal refetches) plus the longest endpoint outage we are willing to survive + * without dropping anyone — a failed fetch stores nothing. Two days leaves better than a day and a + * half of outage, and still bounds the leak at the entries of two days of releases. * * Erring long is deliberate. Deleting an entry too early costs the page reading it one session on * its init values; keeping a dead one costs a few hundred bytes. @@ -545,12 +545,11 @@ function writeEntry(setup: RemoteConfigSetup, values: RemoteConfigValues) { * This page's own entry is never a candidate: it holds the version floor that lets a late answer * be refused, and it is about to be read by the request this initialisation is starting. * - * An entry with no write time at all was left by a build older than this one. It is taken for - * abandoned rather than stamped and kept, which is the trade this makes deliberately: stamping - * would mean a write per orphan on the first load after the upgrade, and the accumulated orphans - * are exactly what this exists to clear. What it costs is bounded — while two builds are live on - * one origin, a page still on the old one may have its entry swept and spend a single session on - * its init values before writing it back. + * An entry with no write time at all was left by a build older than this one. It is kept because an + * old build still running on the origin cannot add the write time when it refreshes the entry, so + * absence alone cannot distinguish a live release from an abandoned one. That leaves a finite set + * of entries from before the write time existed; every entry written from this build onward is + * timestamped, so the cache no longer grows without bound. */ function sweepAbandonedEntries(keepKey: string) { try { @@ -564,7 +563,8 @@ function sweepAbandonedEntries(keepKey: string) { if (key === null || key === keepKey || key.indexOf(STORE_KEY_PREFIX) !== 0) { continue } - if (now - readWriteTime(key) > STORE_ENTRY_MAX_AGE) { + const writeTime = readWriteTime(key) + if (writeTime !== undefined && now - writeTime > STORE_ENTRY_MAX_AGE) { abandoned.push(key) } } @@ -577,21 +577,21 @@ function sweepAbandonedEntries(keepKey: string) { } /** - * When the entry under `key` was last written, or 0 — older than any threshold — when it does not - * say. Anything in a browser profile can be edited by hand, so a time that is not a plain number is - * read as no time at all rather than trusted into the arithmetic above. + * When the entry under `key` was last written, or undefined when it does not say. Anything in a + * browser profile can be edited by hand, so a time that is not a finite number is read as no time + * at all rather than trusted into the arithmetic above. */ function readWriteTime(key: string) { try { const stored = localStorage.getItem(key) const parsed: unknown = stored ? JSON.parse(stored) : undefined if (!parsed || typeof parsed !== 'object') { - return 0 + return undefined } const { t } = parsed as Partial - return typeof t === 'number' && isFinite(t) ? t : 0 + return typeof t === 'number' && isFinite(t) ? t : undefined } catch { - return 0 + return undefined } }