diff --git a/CHANGELOG.md b/CHANGELOG.md index 056cc32b0b..42b9a2ac7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,16 @@ --- +## 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. 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 - ✨ 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..c3e409ab8c 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,102 @@ 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('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)).not.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..2c0ee781f9 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 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. + */ +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 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 { + 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 + } + const writeTime = readWriteTime(key) + if (writeTime !== undefined && now - writeTime > 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 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 undefined + } + const { t } = parsed as Partial + return typeof t === 'number' && isFinite(t) ? t : undefined + } catch { + return undefined + } +} + 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 ?? ''))