From 3ddf92abb29fd7d49361b834aa91a9cb552a152e Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Tue, 22 Sep 2026 18:18:26 +0200 Subject: [PATCH 1/7] feat(storage): add a scoped JSON storage and a sweep-safe expiring storage ScopedJsonResourceStorage keeps the key mapping and the on-disk layout of JsonResourceStorage, but its entries() enumeration starts in a descendant container instead of the storage root, so a sweep no longer has to read every document of /.internal/. PivotExpiringStorage keeps the expiry semantics of the default expiring storage and adds what the periodic sweeps need: a jitter on the sweep interval (the internal stores are all created at startup, so their sweeps fired in the same instant), deletions in bounded batches instead of one unbounded Promise.all, and a finalize() that clears the timer on shutdown. --- src/index.ts | 2 + src/storage/PivotExpiringStorage.ts | 160 ++++++++++++++ src/storage/ScopedJsonResourceStorage.ts | 37 ++++ .../unit/storage/PivotExpiringStorage.test.ts | 207 ++++++++++++++++++ .../storage/ScopedJsonResourceStorage.test.ts | 103 +++++++++ 5 files changed, 509 insertions(+) create mode 100644 src/storage/PivotExpiringStorage.ts create mode 100644 src/storage/ScopedJsonResourceStorage.ts create mode 100644 test/unit/storage/PivotExpiringStorage.test.ts create mode 100644 test/unit/storage/ScopedJsonResourceStorage.test.ts diff --git a/src/index.ts b/src/index.ts index 2d9b1a8..c019f34 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,6 @@ export * from "./storage/RdfPatchingStore"; +export * from "./storage/ScopedJsonResourceStorage"; +export * from "./storage/PivotExpiringStorage"; export * from "./storage/patch/ThrowingN3Patcher"; export * from './FedcmHttpHandler'; export * from './http/output/PivotResponseWriter'; diff --git a/src/storage/PivotExpiringStorage.ts b/src/storage/PivotExpiringStorage.ts new file mode 100644 index 0000000..86a20ed --- /dev/null +++ b/src/storage/PivotExpiringStorage.ts @@ -0,0 +1,160 @@ +import { + getLoggerFor, + InternalServerError, + setSafeInterval, +} from '@solid/community-server'; +import type { + Expires, + ExpiringStorage, + Finalizable, + KeyValueStorage, +} from '@solid/community-server'; + +/** + * A storage that wraps around another storage and expires resources based on the given (optional) + * expiry date, with the same behaviour as the default `WrappedExpiringStorage`, plus three + * adjustments that make periodic expiration sweeps safe to run: + * + * 1. A random jitter is added to the sweep interval. The internal expiring storages (cookies, + * forgot-password, ownership tokens, OIDC adapter) are all created at startup, so without jitter + * their sweeps fire in the same instant and cause periodic latency spikes. The jitter spreads + * them out; it defaults to 0.15 (up to 15% of the timeout) and `0` disables it. + * 2. Expired entries are deleted in bounded batches instead of one unbounded `Promise.all`, so a + * large number of expired entries cannot flood the event loop and the thread pool at once. + * 3. The class is {@link Finalizable}: `finalize()` clears the sweep timer so a graceful shutdown + * does not leave the interval behind (the timer is also `unref`'d as a safety net). + */ +export class PivotExpiringStorage implements ExpiringStorage, Finalizable { + protected readonly logger = getLoggerFor(this); + private readonly source: KeyValueStorage>; + private readonly timer: NodeJS.Timeout; + private readonly batchSize: number; + + /** + * @param source - KeyValueStorage to actually store the data. + * @param timeout - How often the expired data needs to be checked in minutes. + * @param jitter - Maximum fraction of the timeout that is randomly added to the interval so that + * multiple instances do not all sweep at the same time. `0` disables jitter. + * @param batchSize - Maximum number of expired entries deleted concurrently. + */ + public constructor( + source: KeyValueStorage>, + timeout = 60, + jitter = 0.15, + batchSize = 32, + ) { + if (!Number.isSafeInteger(batchSize) || batchSize < 1) { + throw new TypeError('The expired-entry deletion batch size must be a positive integer.'); + } + this.source = source; + this.batchSize = batchSize; + const period = timeout * 60 * 1000; + const jitterMs = Math.floor(Math.random() * period * jitter); + this.timer = setSafeInterval( + this.logger, + 'Failed to remove expired entries', + this.removeExpiredEntries.bind(this), + period + jitterMs, + ); + this.timer.unref(); + } + + public async get(key: TKey): Promise { + return this.getUnexpired(key); + } + + public async has(key: TKey): Promise { + return Boolean(await this.getUnexpired(key)); + } + + public async set(key: TKey, value: TValue, expiration?: number): Promise; + public async set(key: TKey, value: TValue, expires?: Date): Promise; + public async set(key: TKey, value: TValue, expireValue?: number | Date): Promise { + const expires = typeof expireValue === 'number' ? new Date(Date.now() + expireValue) : expireValue; + if (this.isExpired(expires)) { + throw new InternalServerError('Value is already expired'); + } + await this.source.set(key, this.toExpires(value, expires)); + return this; + } + + public async delete(key: TKey): Promise { + return this.source.delete(key); + } + + public async* entries(): AsyncIterableIterator<[TKey, TValue]> { + // Not deleting expired entries here to prevent iterator issues + for await (const [ key, value ] of this.source.entries()) { + const { expires, payload } = this.toData(value); + if (!this.isExpired(expires)) { + yield [ key, payload ]; + } + } + } + + public async finalize(): Promise { + clearInterval(this.timer); + } + + /** + * Deletes all entries that have expired, in batches of `batchSize` concurrent deletes. + */ + private async removeExpiredEntries(): Promise { + this.logger.debug('Removing expired entries'); + const expired: TKey[] = []; + for await (const [ key, value ] of this.source.entries()) { + const { expires } = this.toData(value); + if (this.isExpired(expires)) { + expired.push(key); + } + } + for (let index = 0; index < expired.length; index += this.batchSize) { + await Promise.all(expired.slice(index, index + this.batchSize) + .map(async(key): Promise => this.source.delete(key))); + } + this.logger.debug('Finished removing expired entries'); + } + + /** + * Tries to get the data for the given key. + * In case the data exists but has expired, + * it will be deleted and `undefined` will be returned instead. + */ + private async getUnexpired(key: TKey): Promise { + const data = await this.source.get(key); + if (!data) { + return; + } + const { expires, payload } = this.toData(data); + if (this.isExpired(expires)) { + await this.source.delete(key); + return; + } + return payload; + } + + /** + * Checks if the given data entry has expired. + */ + private isExpired(expires?: Date): boolean { + return typeof expires !== 'undefined' && expires < new Date(); + } + + /** + * Creates a new object where the `expires` field is a string instead of a Date. + */ + private toExpires(data: TValue, expires?: Date): Expires { + return { expires: expires?.toISOString(), payload: data }; + } + + /** + * Creates a new object where the `expires` field is a Date instead of a string. + */ + private toData(expireData: Expires): { expires?: Date; payload: TValue } { + const result: { expires?: Date; payload: TValue } = { payload: expireData.payload }; + if (expireData.expires) { + result.expires = new Date(expireData.expires); + } + return result; + } +} diff --git a/src/storage/ScopedJsonResourceStorage.ts b/src/storage/ScopedJsonResourceStorage.ts new file mode 100644 index 0000000..3dc203b --- /dev/null +++ b/src/storage/ScopedJsonResourceStorage.ts @@ -0,0 +1,37 @@ +import { + ensureTrailingSlash, + JsonResourceStorage, + joinUrl, +} from '@solid/community-server'; +import type { ResourceStore } from '@solid/community-server'; + +/** + * A {@link JsonResourceStorage} that preserves the root-relative key mapping of its parent class + * while restricting `entries()` to a smaller descendant container. + * + * This exists for cleanup sweeps: the internal expiring storages are chained as + * `ContainerPathStorage -> MaxKeyLengthStorage -> JsonResourceStorage`, and the `entries()` call of a + * sweep delegates all the way down to {@link JsonResourceStorage.entries}, which recursively walks + * **every** document in `/.internal/` before the outer wrappers filter the keys by prefix. + * With many accounts that walk dominates the server's CPU cost (it re-reads the whole internal tree + * on every sweep). + * + * Replacing the bottom storage with this class keeps the key mapping, the key hashing and the + * on-disk layout exactly the same, but starts the enumeration at `entryContainer` instead of the + * storage root, so a sweep only reads its own subtree. + */ +export class ScopedJsonResourceStorage extends JsonResourceStorage { + private readonly entryContainer: string; + + public constructor(source: ResourceStore, baseUrl: string, container: string, entryContainer: string) { + super(source, baseUrl, container); + this.entryContainer = ensureTrailingSlash(joinUrl(baseUrl, entryContainer)); + if (!this.entryContainer.startsWith(this.container)) { + throw new TypeError('The entry container must be inside the storage container.'); + } + } + + public async* entries(): AsyncIterableIterator<[string, T]> { + yield* this.getResourceEntries({ path: this.entryContainer }); + } +} diff --git a/test/unit/storage/PivotExpiringStorage.test.ts b/test/unit/storage/PivotExpiringStorage.test.ts new file mode 100644 index 0000000..1d66567 --- /dev/null +++ b/test/unit/storage/PivotExpiringStorage.test.ts @@ -0,0 +1,207 @@ +import type { Expires, KeyValueStorage } from '@solid/community-server'; +import { InternalServerError } from '@solid/community-server'; +import { PivotExpiringStorage } from '../../../src/storage/PivotExpiringStorage'; +import { flushPromises } from '../../util/Util'; + +type Internal = Expires; + +function createExpires(payload: string, expires?: Date): Internal { + return { payload, expires: expires?.toISOString() }; +} + +describe('A PivotExpiringStorage', (): void => { + const tomorrow = new Date(); + tomorrow.setDate(tomorrow.getDate() + 1); + const yesterday = new Date(); + yesterday.setDate(yesterday.getDate() - 1); + let source: jest.Mocked>; + let storage: PivotExpiringStorage; + let mockInterval: jest.SpyInstance; + let mockClear: jest.SpyInstance; + let mockRandom: jest.SpyInstance; + let mockTimer: { unref: jest.Mock }; + + beforeEach((): void => { + // Never schedule a real sweep; capture the interval instead. + mockTimer = { unref: jest.fn() }; + mockInterval = jest.spyOn(globalThis, 'setInterval') + .mockImplementation(jest.fn().mockReturnValue(mockTimer) as any); + mockClear = jest.spyOn(globalThis, 'clearInterval').mockImplementation(jest.fn() as any); + // Fixed jitter source so the scheduled delay is deterministic. + mockRandom = jest.spyOn(globalThis.Math, 'random').mockReturnValue(0.5); + + source = { + get: jest.fn(), + has: jest.fn(), + set: jest.fn(), + delete: jest.fn(), + entries: jest.fn(), + }; + storage = new PivotExpiringStorage(source, 1, 0); + }); + + afterEach((): void => { + mockInterval.mockRestore(); + mockClear.mockRestore(); + mockRandom.mockRestore(); + }); + + it('does not return data if there is no result.', async(): Promise => { + await expect(storage.get('key')).resolves.toBeUndefined(); + expect(source.get).toHaveBeenCalledTimes(1); + expect(source.get).toHaveBeenLastCalledWith('key'); + }); + + it('returns data if it has not expired.', async(): Promise => { + source.get.mockResolvedValueOnce(createExpires('data!', tomorrow)); + await expect(storage.get('key')).resolves.toBe('data!'); + }); + + it('deletes expired data when trying to get it.', async(): Promise => { + source.get.mockResolvedValueOnce(createExpires('data!', yesterday)); + await expect(storage.get('key')).resolves.toBeUndefined(); + expect(source.delete).toHaveBeenCalledTimes(1); + expect(source.delete).toHaveBeenLastCalledWith('key'); + }); + + it('returns false on `has` checks if there is no data.', async(): Promise => { + await expect(storage.has('key')).resolves.toBe(false); + expect(source.get).toHaveBeenCalledTimes(1); + expect(source.get).toHaveBeenLastCalledWith('key'); + }); + + it('true on `has` checks if there is non-expired data.', async(): Promise => { + source.get.mockResolvedValueOnce(createExpires('data!', tomorrow)); + await expect(storage.has('key')).resolves.toBe(true); + }); + + it('deletes expired data when checking if it exists.', async(): Promise => { + source.get.mockResolvedValueOnce(createExpires('data!', yesterday)); + await expect(storage.has('key')).resolves.toBe(false); + expect(source.delete).toHaveBeenCalledTimes(1); + expect(source.delete).toHaveBeenLastCalledWith('key'); + }); + + it('converts the expiry date to a string when storing data.', async(): Promise => { + await storage.set('key', 'data!', tomorrow); + expect(source.set).toHaveBeenCalledTimes(1); + expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!', tomorrow)); + }); + + it('can store data with an expiration duration.', async(): Promise => { + await storage.set('key', 'data!', tomorrow.getTime() - Date.now()); + expect(source.set).toHaveBeenCalledTimes(1); + expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!', tomorrow)); + }); + + it('can store data without expiry date.', async(): Promise => { + await storage.set('key', 'data!'); + expect(source.set).toHaveBeenCalledTimes(1); + expect(source.set).toHaveBeenLastCalledWith('key', createExpires('data!')); + }); + + it('errors when trying to store expired data.', async(): Promise => { + await expect(storage.set('key', 'data!', yesterday)).rejects.toThrow(InternalServerError); + }); + + it('directly calls delete on the source when deleting.', async(): Promise => { + await expect(storage.delete('key')).resolves.toBeUndefined(); + expect(source.delete).toHaveBeenCalledTimes(1); + expect(source.delete).toHaveBeenLastCalledWith('key'); + }); + + it('only iterates over non-expired entries.', async(): Promise => { + const data = [ + [ 'key1', createExpires('data1', tomorrow) ], + [ 'key2', createExpires('data2', yesterday) ], + [ 'key3', createExpires('data3') ], + ]; + source.entries.mockImplementationOnce(function* (): any { + yield* data; + }); + const it = storage.entries(); + await expect(it.next()).resolves.toEqual( + expect.objectContaining({ value: [ 'key1', 'data1' ]}), + ); + await expect(it.next()).resolves.toEqual( + expect.objectContaining({ value: [ 'key3', 'data3' ]}), + ); + }); + + describe('scheduling the cleanup sweep', (): void => { + it('schedules the sweep on the configured timeout when jitter is disabled.', (): void => { + storage = new PivotExpiringStorage(source, 1, 0); + expect(mockInterval).toHaveBeenCalledTimes(2); + expect(mockInterval.mock.calls[1]).toHaveLength(2); + expect(mockInterval.mock.calls[1][1]).toBe(60 * 1000); + }); + + it('adds a jitter fraction to the scheduled sweep interval.', (): void => { + // Math.random is 0.5 and jitter is 0.2, so floor(0.5 * 60000 * 0.2) = 6000 is added. + storage = new PivotExpiringStorage(source, 1, 0.2); + expect(mockInterval).toHaveBeenCalledTimes(2); + expect(mockInterval.mock.calls[1][1]).toBe(60 * 1000 + 6000); + }); + + it('unrefs the timer so it does not keep the event loop alive.', (): void => { + expect(mockTimer.unref).toHaveBeenCalledTimes(1); + }); + + it('removes expired entries when the scheduled sweep fires.', async(): Promise => { + const data = [ + [ 'key1', createExpires('data1', tomorrow) ], + [ 'key2', createExpires('data2', yesterday) ], + [ 'key3', createExpires('data3') ], + ]; + source.entries.mockImplementationOnce(function* (): any { + yield* data; + }); + + // Await the function the sweep interval was created with. + await (mockInterval.mock.calls[0][0] as () => Promise)(); + + expect(source.delete).toHaveBeenCalledTimes(1); + expect(source.delete).toHaveBeenLastCalledWith('key2'); + }); + + it('deletes expired entries in bounded batches.', async(): Promise => { + storage = new PivotExpiringStorage(source, 1, 0, 2); + let resolveFirst!: (value: boolean) => void; + let resolveSecond!: (value: boolean) => void; + const first = new Promise((resolve): void => { + resolveFirst = resolve; + }); + const second = new Promise((resolve): void => { + resolveSecond = resolve; + }); + source.entries.mockImplementationOnce(function* (): any { + yield [ 'key1', createExpires('data1', yesterday) ]; + yield [ 'key2', createExpires('data2', yesterday) ]; + yield [ 'key3', createExpires('data3', yesterday) ]; + }); + source.delete.mockImplementationOnce(async(): Promise => first) + .mockImplementationOnce(async(): Promise => second) + .mockResolvedValue(true); + + const cleanup = (mockInterval.mock.calls[1][0] as () => Promise)(); + await flushPromises(); + expect(source.delete).toHaveBeenCalledTimes(2); + + resolveFirst(true); + resolveSecond(true); + await cleanup; + expect(source.delete).toHaveBeenCalledTimes(3); + }); + + it.each([ 0, -1, 1.5, Number.NaN ])('rejects invalid batch size %p.', (batchSize): void => { + expect((): PivotExpiringStorage => + new PivotExpiringStorage(source, 1, 0, batchSize)).toThrow(TypeError); + }); + + it('clears the timer on finalize.', async(): Promise => { + await expect(storage.finalize()).resolves.toBeUndefined(); + expect(mockClear).toHaveBeenCalledTimes(1); + expect(mockClear).toHaveBeenLastCalledWith(mockTimer); + }); + }); +}); diff --git a/test/unit/storage/ScopedJsonResourceStorage.test.ts b/test/unit/storage/ScopedJsonResourceStorage.test.ts new file mode 100644 index 0000000..5b1044f --- /dev/null +++ b/test/unit/storage/ScopedJsonResourceStorage.test.ts @@ -0,0 +1,103 @@ +import { + BasicRepresentation, + ContainerPathStorage, + guardedStreamFrom, + LDP, + MaxKeyLengthStorage, + RepresentationMetadata, +} from '@solid/community-server'; +import type { ResourceStore } from '@solid/community-server'; +import { ScopedJsonResourceStorage } from '../../../src/storage/ScopedJsonResourceStorage'; + +describe('A ScopedJsonResourceStorage', (): void => { + const baseUrl = 'https://example.com/'; + const container = '/.internal/'; + const entryContainer = '/.internal/idp/tokens/'; + const entryContainerUrl = `${baseUrl}.internal/idp/tokens/`; + const entryUrl = `${entryContainerUrl}token`; + let source: jest.Mocked; + + beforeEach((): void => { + source = { + getRepresentation: jest.fn(async(identifier): Promise => { + if (identifier.path === entryContainerUrl) { + const metadata = new RepresentationMetadata({ path: entryContainerUrl }); + metadata.add(LDP.terms.contains, entryUrl); + return new BasicRepresentation(guardedStreamFrom([]), metadata); + } + return new BasicRepresentation(JSON.stringify({ payload: 'value' }), 'application/json'); + }), + hasResource: jest.fn(), + setRepresentation: jest.fn(), + deleteResource: jest.fn(), + } satisfies Partial as any; + }); + + it('starts enumeration at the scoped container while returning root-relative keys.', async(): Promise => { + const storage = new ScopedJsonResourceStorage(source, baseUrl, container, entryContainer); + + const entries = []; + for await (const entry of storage.entries()) { + entries.push(entry); + } + + expect(entries).toEqual([[ 'idp/tokens/token', { payload: 'value' }]]); + expect(source.getRepresentation).toHaveBeenNthCalledWith(1, { path: entryContainerUrl }, {}); + expect(source.getRepresentation).toHaveBeenNthCalledWith( + 2, + { path: entryUrl }, + { type: { 'application/json': 1 }}, + ); + }); + + it('preserves root-relative key mapping for direct lookups.', async(): Promise => { + const storage = new ScopedJsonResourceStorage(source, baseUrl, container, entryContainer); + + await expect(storage.get('idp/tokens/token')).resolves.toEqual({ payload: 'value' }); + expect(source.getRepresentation).toHaveBeenCalledWith( + { path: entryUrl }, + { type: { 'application/json': 1 }}, + ); + }); + + it('preserves long-key hashing and deletion through the existing wrapper stack.', async(): Promise => { + const jsonStorage = new ScopedJsonResourceStorage<{ + key: string; + payload: { payload: string }; + }>(source, baseUrl, container, entryContainer); + const storage = new ContainerPathStorage( + new MaxKeyLengthStorage(jsonStorage), + '/idp/tokens/', + ); + const key = 'token'.repeat(40); + const value = { payload: 'value' }; + + await storage.set(key, value); + const storedIdentifier = source.setRepresentation.mock.calls[0][0]; + expect(storedIdentifier.path).toMatch(/^https:\/\/example\.com\/\.internal\/idp\/tokens\/\$hash\$/u); + + source.getRepresentation.mockImplementation(async(identifier): Promise => { + if (identifier.path === entryContainerUrl) { + const metadata = new RepresentationMetadata({ path: entryContainerUrl }); + metadata.add(LDP.terms.contains, storedIdentifier.path); + return new BasicRepresentation(guardedStreamFrom([]), metadata); + } + return new BasicRepresentation(JSON.stringify({ key: `idp/tokens/${key}`, payload: value }), 'application/json'); + }); + + const entries = []; + for await (const entry of storage.entries()) { + entries.push(entry); + } + expect(entries).toEqual([[ key, value ]]); + + await expect(storage.delete(key)).resolves.toBe(true); + expect(source.deleteResource).toHaveBeenCalledWith(storedIdentifier); + }); + + it('rejects an entry container outside the storage root.', (): void => { + expect((): ScopedJsonResourceStorage => + new ScopedJsonResourceStorage(source, baseUrl, '/.internal/accounts/', '/.internal/idp/')) + .toThrow('The entry container must be inside the storage container.'); + }); +}); From 554f88abcbf8b6ddff986a5f6524ea1e98e0defd Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Tue, 22 Sep 2026 18:18:26 +0200 Subject: [PATCH 2/7] fix(config): scope the internal expiration sweeps to their own containers The stock expiring storages wrap ContainerPathStorage over a JsonResourceStorage rooted at /.internal/ and filter the keys only after the recursive walk, so each sweep of the cookie, forgot-password, ownership-token and OIDC adapter stores reads the entire internal tree. With a large account base that walk dominates the CPU between sweeps. Each sweep now enumerates through a ScopedJsonResourceStorage limited to its own container while keeping ContainerPathStorage and the key hashing, so keys and the stored layout are unchanged, and uses PivotExpiringStorage so the sweeps are jittered, delete in bounded batches and stop their timers on shutdown (wired into urn:solid-server:default:Finalizer). The OIDC adapter factory is redeclared in that file because the stock 7.2.0 configuration defines its storage inline, with no @id that could be overridden. The file is imported by prod.json, suffix.json and the two dev configs; test.json has no accounts, so the stores it would override do not exist there. The inline timeout overrides in prod.json and suffix.json are replaced by this file, which keeps the same one-minute sweep interval. --- config/dev-http-subdomain.json | 4 +- config/dev-http-suffix.json | 4 +- config/pivot-scoped-sweeps.json | 137 ++++++++++++++++++++++++++ config/prod.json | 18 +--- config/suffix.json | 18 +--- test/integration/ScopedSweeps.test.ts | 40 ++++++++ 6 files changed, 187 insertions(+), 34 deletions(-) create mode 100644 config/pivot-scoped-sweeps.json create mode 100644 test/integration/ScopedSweeps.test.ts diff --git a/config/dev-http-subdomain.json b/config/dev-http-subdomain.json index 19abb8a..720ff6b 100644 --- a/config/dev-http-subdomain.json +++ b/config/dev-http-subdomain.json @@ -1,5 +1,4 @@ { - "comment": "Copied from https://github.com/SolidOS/css-mashlib/blob/ae21af4685f6c95c1f091cacd952831f272ea119/config/https-mashlib-subdomain-file.json, (1) pivot:config/http/handler/default.json, (2) pivot:config/storage/middleware/default.json, (3) pivot:config/pivot-overrides.json added as the last import", "@context": [ "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^7.0.0/components/context.jsonld", "https://linkedsoftwaredependencies.org/bundles/npm/@solid/pivot/^1.0.0/components/context.jsonld" @@ -36,7 +35,8 @@ "css:config/util/representation-conversion/default.json", "css:config/util/resource-locker/file.json", "css:config/util/variables/default.json", - "pivot:config/pivot-overrides.json" + "pivot:config/pivot-overrides.json", + "pivot:config/pivot-scoped-sweeps.json" ], "@graph": [ { diff --git a/config/dev-http-suffix.json b/config/dev-http-suffix.json index fb5fa89..749ca06 100644 --- a/config/dev-http-suffix.json +++ b/config/dev-http-suffix.json @@ -1,5 +1,4 @@ { - "comment": "Copied from https://github.com/SolidOS/css-mashlib/blob/ae21af4685f6c95c1f091cacd952831f272ea119/config/https-mashlib-subdomain-file.json, (1) pivot:config/http/handler/default.json, (2) pivot:config/storage/middleware/default.json, (3) pivot:config/pivot-overrides.json added as the last import", "@context": [ "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^7.0.0/components/context.jsonld", "https://linkedsoftwaredependencies.org/bundles/npm/@solid/pivot/^1.0.0/components/context.jsonld" @@ -36,7 +35,8 @@ "css:config/util/representation-conversion/default.json", "css:config/util/resource-locker/file.json", "css:config/util/variables/default.json", - "pivot:config/pivot-overrides.json" + "pivot:config/pivot-overrides.json", + "pivot:config/pivot-scoped-sweeps.json" ], "@graph": [ { diff --git a/config/pivot-scoped-sweeps.json b/config/pivot-scoped-sweeps.json new file mode 100644 index 0000000..8985383 --- /dev/null +++ b/config/pivot-scoped-sweeps.json @@ -0,0 +1,137 @@ +{ + "comment": "Scoped expiration sweeps for the internal identity storages (imported by prod.json / suffix.json / dev configs, not by test.json). Each storage keeps its exact wrapper chain (ContainerPathStorage, then the key hashing of MaxKeyLengthStorage, over the shared /.internal/ JSON storage) so keys and the on-disk layout are unchanged, but its enumeration starts in the storage's own container: the stock chain delegates entries() to the root JSON storage, which recursively reads the whole /.internal/ tree on every sweep (persistent CPU cost with a large account base). The outer wrapper is pivot's PivotExpiringStorage: the sweep intervals are jittered so the storages no longer all fire in the same instant, expired entries are deleted in bounded batches, and each sweep timer is stopped on shutdown through urn:solid-server:default:Finalizer. The OIDC adapter factory is redefined here because the stock 7.2.0 config declares its storage inline, without an @id that could be overridden.", + "@context": [ + "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^7.0.0/components/context.jsonld", + "https://linkedsoftwaredependencies.org/bundles/npm/@solid/pivot/^1.0.0/components/context.jsonld" + ], + "@graph": [ + { + "comment": "Scope the cookie sweep to its own container.", + "@type": "Override", + "overrideInstance": { "@id": "urn:solid-server:default:CookieStorage" }, + "overrideParameters": { + "@type": "PivotExpiringStorage", + "timeout": 1, + "source": { + "@type": "ContainerPathStorage", + "relativePath": "/accounts/cookies/", + "source": { + "@type": "MaxKeyLengthStorage", + "source": { + "@type": "ScopedJsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/", + "entryContainer": "/.internal/accounts/cookies/" + } + } + } + } + }, + { + "comment": "Scope the forgot-password sweep to its own container.", + "@type": "Override", + "overrideInstance": { "@id": "urn:solid-server:default:ForgotPasswordStorage" }, + "overrideParameters": { + "@type": "PivotExpiringStorage", + "timeout": 1, + "source": { + "@type": "ContainerPathStorage", + "relativePath": "/accounts/forgot-password/", + "source": { + "@type": "MaxKeyLengthStorage", + "source": { + "@type": "ScopedJsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/", + "entryContainer": "/.internal/accounts/forgot-password/" + } + } + } + } + }, + { + "comment": "Scope the ownership-token sweep to its own container.", + "@type": "Override", + "overrideInstance": { "@id": "urn:solid-server:default:ExpiringTokenStorage" }, + "overrideParameters": { + "@type": "PivotExpiringStorage", + "timeout": 1, + "source": { + "@type": "ContainerPathStorage", + "relativePath": "/idp/tokens/", + "source": { + "@type": "MaxKeyLengthStorage", + "source": { + "@type": "ScopedJsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/", + "entryContainer": "/.internal/idp/tokens/" + } + } + } + } + }, + { + "comment": "Scope the OIDC adapter sweep to its own container. The storage is named here so the sweep and its finalizer can target it.", + "@type": "Override", + "overrideInstance": { "@id": "urn:solid-server:default:IdpAdapterFactory" }, + "overrideParameters": { + "@type": "ClientCredentialsAdapterFactory", + "webIdStore": { "@id": "urn:solid-server:default:WebIdStore" }, + "clientCredentialsStore": { "@id": "urn:solid-server:default:ClientCredentialsStore" }, + "source": { + "@type": "ClientIdAdapterFactory", + "converter": { "@id": "urn:solid-server:default:RepresentationConverter" }, + "source": { + "@type": "ExpiringAdapterFactory", + "storage": { + "@id": "urn:solid-server:default:PivotExpiringAdapterStorage", + "@type": "PivotExpiringStorage", + "timeout": 1, + "source": { + "@type": "ContainerPathStorage", + "relativePath": "/idp/adapter/", + "source": { + "@type": "MaxKeyLengthStorage", + "source": { + "@type": "ScopedJsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/", + "entryContainer": "/.internal/idp/adapter/" + } + } + } + } + } + } + } + }, + { + "comment": "Makes sure the expiring storage sweep timers are stopped when the application needs to stop.", + "@id": "urn:solid-server:default:Finalizer", + "@type": "ParallelHandler", + "handlers": [ + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:CookieStorage" } + }, + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:ForgotPasswordStorage" } + }, + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:ExpiringTokenStorage" } + }, + { + "@type": "FinalizableHandler", + "finalizable": { "@id": "urn:solid-server:default:PivotExpiringAdapterStorage" } + } + ] + } + ] +} diff --git a/config/prod.json b/config/prod.json index 5e8cf62..9f50bf3 100644 --- a/config/prod.json +++ b/config/prod.json @@ -1,5 +1,4 @@ { - "comment": "Copied from https://github.com/SolidOS/css-mashlib/blob/ae21af4685f6c95c1f091cacd952831f272ea119/config/https-mashlib-subdomain-file.json, (1) pivot:config/http/handler/default.json, (2) pivot:config/storage/middleware/default.json, (3) pivot:config/pivot-overrides.json added as the last import", "@context": [ "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^7.0.0/components/context.jsonld", "https://linkedsoftwaredependencies.org/bundles/npm/@solid/pivot/^1.0.0/components/context.jsonld" @@ -35,7 +34,8 @@ "css:config/util/representation-conversion/default.json", "css:config/util/resource-locker/file.json", "css:config/util/variables/default.json", - "pivot:config/pivot-overrides.json" + "pivot:config/pivot-overrides.json", + "pivot:config/pivot-scoped-sweeps.json" ], "@graph": [ { @@ -56,19 +56,7 @@ ] }, { - "@id": "urn:solid-server:default:CookieStorage", - "@type": "WrappedExpiringStorage", - "timeout": 1 - }, - { - "@id": "urn:solid-server:default:ForgotPasswordStorage", - "@type": "WrappedExpiringStorage", - "timeout": 1 - }, - { - "@id": "urn:solid-server:default:ExpiringTokenStorage", - "@type": "WrappedExpiringStorage", - "timeout": 1 + "comment": "The internal expiring storages and their scoped sweeps are defined in pivot:config/pivot-scoped-sweeps.json." } ] } diff --git a/config/suffix.json b/config/suffix.json index cf12ef6..f96ae4e 100644 --- a/config/suffix.json +++ b/config/suffix.json @@ -1,5 +1,4 @@ { - "comment": "Copied from https://github.com/SolidOS/css-mashlib/blob/ae21af4685f6c95c1f091cacd952831f272ea119/config/https-mashlib-subdomain-file.json, (1) pivot:config/http/handler/default.json, (2) pivot:config/storage/middleware/default.json, (3) pivot:config/pivot-overrides.json added as the last import", "@context": [ "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^7.0.0/components/context.jsonld", "https://linkedsoftwaredependencies.org/bundles/npm/@solid/pivot/^1.0.0/components/context.jsonld" @@ -35,7 +34,8 @@ "css:config/util/representation-conversion/default.json", "css:config/util/resource-locker/file.json", "css:config/util/variables/default.json", - "pivot:config/pivot-overrides.json" + "pivot:config/pivot-overrides.json", + "pivot:config/pivot-scoped-sweeps.json" ], "@graph": [ { @@ -56,19 +56,7 @@ ] }, { - "@id": "urn:solid-server:default:CookieStorage", - "@type": "WrappedExpiringStorage", - "timeout": 1 - }, - { - "@id": "urn:solid-server:default:ForgotPasswordStorage", - "@type": "WrappedExpiringStorage", - "timeout": 1 - }, - { - "@id": "urn:solid-server:default:ExpiringTokenStorage", - "@type": "WrappedExpiringStorage", - "timeout": 1 + "comment": "The internal expiring storages and their scoped sweeps are defined in pivot:config/pivot-scoped-sweeps.json." } ] } diff --git a/test/integration/ScopedSweeps.test.ts b/test/integration/ScopedSweeps.test.ts new file mode 100644 index 0000000..b1abf28 --- /dev/null +++ b/test/integration/ScopedSweeps.test.ts @@ -0,0 +1,40 @@ +import { getDefaultVariables, getPresetConfigPath, instantiateFromConfig } from './Config'; + +describe('A server configured with the pivot scoped sweeps', (): void => { + const config = getPresetConfigPath('prod.json'); + const variables = { + ...getDefaultVariables(3000, 'http://localhost:3000/'), + 'urn:solid-server:default:variable:rootFilePath': '/tmp/pivot-scoped-sweeps-test', + }; + + // Instantiates the given storage from the configuration and returns the entry container of its scoped JSON storage. + // Class names are compared instead of instanceof: the configuration instantiates the built classes from dist/. + async function getEntryContainer(storageId: string): Promise { + const storage = await instantiateFromConfig(storageId, config, variables) as any; + expect(storage.constructor.name).toBe('PivotExpiringStorage'); + expect(storage.source.constructor.name).toBe('ContainerPathStorage'); + const scoped = storage.source.source.source; + expect(scoped.constructor.name).toBe('ScopedJsonResourceStorage'); + return scoped.entryContainer; + } + + it('scopes the cookie sweep to the cookie container.', async(): Promise => { + await expect(getEntryContainer('urn:solid-server:default:CookieStorage')) + .resolves.toBe('http://localhost:3000/.internal/accounts/cookies/'); + }); + + it('scopes the forgot-password sweep to the forgot-password container.', async(): Promise => { + await expect(getEntryContainer('urn:solid-server:default:ForgotPasswordStorage')) + .resolves.toBe('http://localhost:3000/.internal/accounts/forgot-password/'); + }); + + it('scopes the ownership-token sweep to the token container.', async(): Promise => { + await expect(getEntryContainer('urn:solid-server:default:ExpiringTokenStorage')) + .resolves.toBe('http://localhost:3000/.internal/idp/tokens/'); + }); + + it('scopes the OIDC adapter sweep to the adapter container.', async(): Promise => { + await expect(getEntryContainer('urn:solid-server:default:PivotExpiringAdapterStorage')) + .resolves.toBe('http://localhost:3000/.internal/idp/adapter/'); + }); +}); From 6ccf1a6c945ef7b1e30b1abf299d84860d4740e0 Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Tue, 22 Sep 2026 18:18:26 +0200 Subject: [PATCH 3/7] docs: add the clear lock analysis (CLEAR-LOCK.md) Describes the scoped sweep storage, the jittered and batched expiring storage, the configuration they replace, how the change is verified and the decisions behind it. --- CLEAR-LOCK.md | 190 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 CLEAR-LOCK.md diff --git a/CLEAR-LOCK.md b/CLEAR-LOCK.md new file mode 100644 index 0000000..7f1bc8d --- /dev/null +++ b/CLEAR-LOCK.md @@ -0,0 +1,190 @@ +# Clear Lock — Internal Expiration Sweeps and Lock Contention (2026-09-22) + +Pivot runs on the Community Solid Server (CSS 7.2.0, file backend). CSS keeps a +handful of **internal** key-value stores under `/.internal/`: login cookies, +forgot-password requests, WebID ownership tokens and the OIDC adapter state. +Each store holds values with an optional expiry date, and each is wrapped in an +*expiring storage* that periodically deletes whatever has expired — the sweep. +This branch gives Pivot its own sweep storage and scopes every sweep to the +container it belongs to. + +**Problem:** the stock wrapper chain delegates `entries()` to a JSON storage +rooted at `/.internal/`, so a sweep recursively reads **every** internal +document — accounts, indices, locks, the other stores — and filters the keys +only afterwards. With a large account base those walks dominate the server's CPU +between requests; on top of that the four sweeps start in the same instant and +delete everything in one unbounded batch. + +--- + +## 1. What this branch implements + +### 1.1 `ScopedJsonResourceStorage` (`src/storage/ScopedJsonResourceStorage.ts`) + +A `JsonResourceStorage` whose enumeration starts in a descendant container. Everything +else — key mapping, key hashing, identifiers, file layout — is inherited unchanged: + +```ts +export class ScopedJsonResourceStorage extends JsonResourceStorage { + public constructor(source: ResourceStore, baseUrl: string, container: string, entryContainer: string) { + super(source, baseUrl, container); + this.entryContainer = ensureTrailingSlash(joinUrl(baseUrl, entryContainer)); + if (!this.entryContainer.startsWith(this.container)) { + throw new TypeError('The entry container must be inside the storage container.'); + } + } + + public async* entries(): AsyncIterableIterator<[string, T]> { + yield* this.getResourceEntries({ path: this.entryContainer }); + } +} +``` + +`get('idp/tokens/…')` still resolves through the root-relative key, so the class +slots into the existing wrapper stack (`ContainerPathStorage` → +`MaxKeyLengthStorage` → here) without touching stored data. + +### 1.2 `PivotExpiringStorage` (`src/storage/PivotExpiringStorage.ts`) + +The same expiry semantics as `WrappedExpiringStorage` (values keep their expiry +date; expired values are deleted on read and by the sweep), plus three things a +periodic sweep needs: + +1. **Jitter** — the interval gets a random fraction of the timeout added + (`jitter`, default `0.15`, `0` disables it), so the four instances created at + startup no longer sweep in the same instant. +2. **Bounded deletes** — expired entries are deleted in batches of `batchSize` + (default `32`) instead of one `Promise.all` over the whole set: + + ```ts + for (let index = 0; index < expired.length; index += this.batchSize) { + await Promise.all(expired.slice(index, index + this.batchSize) + .map(async(key): Promise => this.source.delete(key))); + } + ``` +3. **`finalize()`** — clears the sweep timer on shutdown (the timer is still + `unref`'d as a safety net). + +### 1.3 Config wiring (`config/pivot-scoped-sweeps.json`) + +Each store is overridden to the same chain with a scoped bottom (cookie example): + +```json +{ + "@type": "Override", + "overrideInstance": { "@id": "urn:solid-server:default:CookieStorage" }, + "overrideParameters": { + "@type": "PivotExpiringStorage", + "timeout": 1, + "source": { + "@type": "ContainerPathStorage", + "relativePath": "/accounts/cookies/", + "source": { + "@type": "MaxKeyLengthStorage", + "source": { + "@type": "ScopedJsonResourceStorage", + "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, + "container": "/.internal/", + "entryContainer": "/.internal/accounts/cookies/" + } + } + } + } +} +``` + +The OIDC adapter factory is **redeclared** in the same file +(`ClientCredentialsAdapterFactory` → `ClientIdAdapterFactory` → +`ExpiringAdapterFactory`), because the stock 7.2.0 configuration declares its +storage inline with no `@id` to override; the storage is named +`urn:solid-server:default:PivotExpiringAdapterStorage` so the sweep and its +finalizer can target it. + +Finally, the four stores are registered in the finalizer chain so their timers are +stopped on shutdown: + +```json +{ + "@id": "urn:solid-server:default:Finalizer", + "@type": "ParallelHandler", + "handlers": [ + { "@type": "FinalizableHandler", "finalizable": { "@id": "urn:solid-server:default:CookieStorage" } }, + … + ] +} +``` + +The file is imported by `prod.json`, `suffix.json`, `dev-http-suffix.json` and +`dev-http-subdomain.json`. `test.json` is deliberately excluded: that preset has +no accounts, so the stores it would override do not exist there. + +### 1.4 What is unchanged, and what is not + +Unchanged: keys, key hashing (`MaxKeyLengthStorage` stays in the chain), the +on-disk layout, the expiry semantics, the sweep interval (`timeout: 1` minute keeps +the production policy). + +Changed: enumeration reads only the store's container, the four sweeps are +jittered/batched/finalized, and the stores read and write through +`ResourceStore_Backend` instead of the locking store — like the lock storage +itself does. Sweep deletes no longer take a lock per entry, and internal storage +traffic no longer contends with the request pipeline. `/.internal/` stays hidden +from clients (`PathBasedReader`), so this does not widen what a client can reach. + +--- + +## 2. Verification + +### 2.1 Unit tests + +* `test/unit/storage/ScopedJsonResourceStorage.test.ts` — enumeration starts at the + scoped container while keys stay root-relative; direct lookups unchanged; long keys + are still hashed and deleted through the existing stack; an entry container outside + the storage root is rejected. +* `test/unit/storage/PivotExpiringStorage.test.ts` — the expiry behaviour of the stock + storage (get/has/set/delete/entries), plus jitter (disabled and enabled), `unref`, + the sweep deleting only expired entries, bounded batches, the batch-size validation + and `finalize()`. + +### 2.2 Configuration tests + +* `test/integration/ScopedSweeps.test.ts` instantiates all four storages from + `config/prod.json` and asserts the chain + (`PivotExpiringStorage` → `ContainerPathStorage` → `MaxKeyLengthStorage` → + `ScopedJsonResourceStorage`) and each `entryContainer`. +* A one-off smoke (not part of the suite) instantiated + `urn:solid-server:default:App` from `prod.json` + `customise-me.json` with + Components.js: the app resolves, the cookie store is scoped as above and + `urn:solid-server:default:Finalizer` reports five handlers + (`ServerInitializer` plus the four expiring stores). +* The same storage instantiation was run against `dev-http-suffix.json` and + `dev-http-subdomain.json` to validate the added import. + +### 2.3 Live behaviour + +The scoped walk was measured in production on 2026-08-30 (pivot-test, ~1000 +accounts): CPU dropped from over 95 % with no client traffic to below 1 %, and +latency stayed flat. That deployment used the interim config-only variant of the +scoping; this branch performs the same scoped walk while keeping the key +semantics. The jitter/batching/finalization part is covered by the unit tests +above. + +--- + +## 3. Decisions and limitations + +* **Interval**: `timeout: 1` (minute) is kept from the deployed policy. A sweep is + now cheap, so a short interval only makes expired entries disappear sooner. +* **Defaults**: `jitter` 0.15 and `batchSize` 32 are constructor defaults; both can be + set per store in the configuration. +* **Maintenance**: `PivotExpiringStorage` mirrors the body of CSS's + `WrappedExpiringStorage`. It is a copy-in of a small, stable class (the three + deltas are documented in its header) — if a future CSS version changes the stock + class, the deltas have to be re-applied. +* **Upstream**: a container-scoped `entries()` in CSS itself (or an `@id` for the + adapter storage) would make both components unnecessary. Until such a change + ships, Pivot keeps its own; nothing else in the deployment depends on it. +* **Out of scope**: the lock configuration (file vs Redis, retry bounds, lock + expiration) is orthogonal — these storage overrides only change where a sweep + enumerates and how it deletes. From ab88d6afa8e0538b2383e5cbd46d4426140571cd Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Wed, 23 Sep 2026 15:23:31 +0200 Subject: [PATCH 4/7] fix(storage): keep the internal stores locked and never overlap sweeps The four internal expiring stores (cookies, forgot-password, ownership tokens, OIDC adapter) source the stock locked ResourceStore again. Reading and writing through the backend bypassed the per-resource locking the stock KeyValueStorage provides, and only the lock storage itself is meant to use the backend directly. The scoped enumeration and the batched deletes are unchanged: a sweep still only reads its own container, and its deletes take the same per-entry lock as the stock storage. The expiring storage no longer arms a plain interval. The next sweep is scheduled with setTimeout only after the running one has finished, so a cleanup that takes longer than the timeout cannot overlap with the next run; the delay keeps its jitter, errors are logged instead of thrown out of the timer, the timer stays unref'd and finalize() clears the pending run. CLEAR-LOCK.md and the tests are updated accordingly (the configuration test now registers the companion customisation, as the start scripts do). --- CLEAR-LOCK.md | 34 +++++--- config/pivot-scoped-sweeps.json | 10 +-- src/storage/PivotExpiringStorage.ts | 71 +++++++++++---- test/integration/ScopedSweeps.test.ts | 5 +- .../unit/storage/PivotExpiringStorage.test.ts | 86 +++++++++++++++---- 5 files changed, 150 insertions(+), 56 deletions(-) diff --git a/CLEAR-LOCK.md b/CLEAR-LOCK.md index 7f1bc8d..0eb9272 100644 --- a/CLEAR-LOCK.md +++ b/CLEAR-LOCK.md @@ -50,7 +50,7 @@ The same expiry semantics as `WrappedExpiringStorage` (values keep their expiry date; expired values are deleted on read and by the sweep), plus three things a periodic sweep needs: -1. **Jitter** — the interval gets a random fraction of the timeout added +1. **Jitter** — every sweep delay gets a random fraction of the timeout added (`jitter`, default `0.15`, `0` disables it), so the four instances created at startup no longer sweep in the same instant. 2. **Bounded deletes** — expired entries are deleted in batches of `batchSize` @@ -62,8 +62,10 @@ periodic sweep needs: .map(async(key): Promise => this.source.delete(key))); } ``` -3. **`finalize()`** — clears the sweep timer on shutdown (the timer is still - `unref`'d as a safety net). +3. **No overlapping sweeps** - the next sweep is scheduled only after the running + one has finished, so a slow cleanup cannot start a second enumeration. A failing + sweep is logged instead of rejecting, the timer is `unref`'d, and `finalize()` + clears the pending run on shutdown. ### 1.3 Config wiring (`config/pivot-scoped-sweeps.json`) @@ -83,7 +85,7 @@ Each store is overridden to the same chain with a scoped bottom (cookie example) "@type": "MaxKeyLengthStorage", "source": { "@type": "ScopedJsonResourceStorage", - "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "source": { "@id": "urn:solid-server:default:ResourceStore" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "container": "/.internal/", "entryContainer": "/.internal/accounts/cookies/" @@ -125,12 +127,13 @@ Unchanged: keys, key hashing (`MaxKeyLengthStorage` stays in the chain), the on-disk layout, the expiry semantics, the sweep interval (`timeout: 1` minute keeps the production policy). -Changed: enumeration reads only the store's container, the four sweeps are -jittered/batched/finalized, and the stores read and write through -`ResourceStore_Backend` instead of the locking store — like the lock storage -itself does. Sweep deletes no longer take a lock per entry, and internal storage -traffic no longer contends with the request pipeline. `/.internal/` stays hidden -from clients (`PathBasedReader`), so this does not widen what a client can reach. +Changed: enumeration reads only the store's container, and the four sweeps are +jittered, batched and never overlapping. The chain keeps the stock locked +`ResourceStore` as its source, so internal reads and writes take the same +per-resource locks as before and sweep deletes take the same per-entry lock as the +stock storage does - only the walk and the scheduling change. `/.internal/` stays +hidden from clients (`PathBasedReader`), so this does not widen what a client can +reach. --- @@ -144,8 +147,9 @@ from clients (`PathBasedReader`), so this does not widen what a client can reach the storage root is rejected. * `test/unit/storage/PivotExpiringStorage.test.ts` — the expiry behaviour of the stock storage (get/has/set/delete/entries), plus jitter (disabled and enabled), `unref`, - the sweep deleting only expired entries, bounded batches, the batch-size validation - and `finalize()`. + the sweep deleting only expired entries, bounded batches, the batch-size validation, + and the scheduling: jitter, `unref`, no overlap while a sweep is running and + `finalize()` clearing the pending run. ### 2.2 Configuration tests @@ -175,7 +179,11 @@ above. ## 3. Decisions and limitations * **Interval**: `timeout: 1` (minute) is kept from the deployed policy. A sweep is - now cheap, so a short interval only makes expired entries disappear sooner. + now cheap, so a short interval only makes expired entries disappear sooner; the + delay is measured from the end of the previous sweep, so runs never overlap. +* **Locking**: the four chains keep the stock locked `ResourceStore` as their source + (only `BackendKeyValueStorage` is meant to use the backend directly), so internal + operations keep the same per-resource locking as before this change. * **Defaults**: `jitter` 0.15 and `batchSize` 32 are constructor defaults; both can be set per store in the configuration. * **Maintenance**: `PivotExpiringStorage` mirrors the body of CSS's diff --git a/config/pivot-scoped-sweeps.json b/config/pivot-scoped-sweeps.json index 8985383..94b1000 100644 --- a/config/pivot-scoped-sweeps.json +++ b/config/pivot-scoped-sweeps.json @@ -1,5 +1,5 @@ { - "comment": "Scoped expiration sweeps for the internal identity storages (imported by prod.json / suffix.json / dev configs, not by test.json). Each storage keeps its exact wrapper chain (ContainerPathStorage, then the key hashing of MaxKeyLengthStorage, over the shared /.internal/ JSON storage) so keys and the on-disk layout are unchanged, but its enumeration starts in the storage's own container: the stock chain delegates entries() to the root JSON storage, which recursively reads the whole /.internal/ tree on every sweep (persistent CPU cost with a large account base). The outer wrapper is pivot's PivotExpiringStorage: the sweep intervals are jittered so the storages no longer all fire in the same instant, expired entries are deleted in bounded batches, and each sweep timer is stopped on shutdown through urn:solid-server:default:Finalizer. The OIDC adapter factory is redefined here because the stock 7.2.0 config declares its storage inline, without an @id that could be overridden.", + "comment": "Scoped expiration sweeps for the internal identity storages (imported by prod.json / suffix.json / dev configs, not by test.json). Each storage keeps its exact wrapper chain (ContainerPathStorage, then the key hashing of MaxKeyLengthStorage, over the shared /.internal/ JSON storage) so keys and the on-disk layout are unchanged, but its enumeration starts in the storage's own container: the stock chain delegates entries() to the root JSON storage, which recursively reads the whole /.internal/ tree on every sweep (persistent CPU cost with a large account base). The outer wrapper is pivot's PivotExpiringStorage: each sweep is scheduled with jitter so the storages do not all fire in the same instant and never overlap, expired entries are deleted in bounded batches, and the pending sweep is cleared on shutdown through urn:solid-server:default:Finalizer. The chain keeps the stock locked ResourceStore as its source, so internal operations lock exactly as before. The OIDC adapter factory is redefined here because the stock 7.2.0 config declares its storage inline, without an @id that could be overridden.", "@context": [ "https://linkedsoftwaredependencies.org/bundles/npm/@solid/community-server/^7.0.0/components/context.jsonld", "https://linkedsoftwaredependencies.org/bundles/npm/@solid/pivot/^1.0.0/components/context.jsonld" @@ -19,7 +19,7 @@ "@type": "MaxKeyLengthStorage", "source": { "@type": "ScopedJsonResourceStorage", - "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "source": { "@id": "urn:solid-server:default:ResourceStore" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "container": "/.internal/", "entryContainer": "/.internal/accounts/cookies/" @@ -42,7 +42,7 @@ "@type": "MaxKeyLengthStorage", "source": { "@type": "ScopedJsonResourceStorage", - "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "source": { "@id": "urn:solid-server:default:ResourceStore" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "container": "/.internal/", "entryContainer": "/.internal/accounts/forgot-password/" @@ -65,7 +65,7 @@ "@type": "MaxKeyLengthStorage", "source": { "@type": "ScopedJsonResourceStorage", - "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "source": { "@id": "urn:solid-server:default:ResourceStore" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "container": "/.internal/", "entryContainer": "/.internal/idp/tokens/" @@ -98,7 +98,7 @@ "@type": "MaxKeyLengthStorage", "source": { "@type": "ScopedJsonResourceStorage", - "source": { "@id": "urn:solid-server:default:ResourceStore_Backend" }, + "source": { "@id": "urn:solid-server:default:ResourceStore" }, "baseUrl": { "@id": "urn:solid-server:default:variable:baseUrl" }, "container": "/.internal/", "entryContainer": "/.internal/idp/adapter/" diff --git a/src/storage/PivotExpiringStorage.ts b/src/storage/PivotExpiringStorage.ts index 86a20ed..fa5f945 100644 --- a/src/storage/PivotExpiringStorage.ts +++ b/src/storage/PivotExpiringStorage.ts @@ -1,7 +1,7 @@ import { + createErrorMessage, getLoggerFor, InternalServerError, - setSafeInterval, } from '@solid/community-server'; import type { Expires, @@ -15,25 +15,29 @@ import type { * expiry date, with the same behaviour as the default `WrappedExpiringStorage`, plus three * adjustments that make periodic expiration sweeps safe to run: * - * 1. A random jitter is added to the sweep interval. The internal expiring storages (cookies, - * forgot-password, ownership tokens, OIDC adapter) are all created at startup, so without jitter - * their sweeps fire in the same instant and cause periodic latency spikes. The jitter spreads - * them out; it defaults to 0.15 (up to 15% of the timeout) and `0` disables it. + * 1. A random jitter is added to every sweep delay. The internal expiring storages (cookies, + * forgot-password, ownership tokens, OIDC adapter) are all created at startup, so without + * jitter their sweeps would run in the same instant; the jitter spreads them out. It defaults + * to 0.15 (up to 15% of the timeout) and `0` disables it. * 2. Expired entries are deleted in bounded batches instead of one unbounded `Promise.all`, so a * large number of expired entries cannot flood the event loop and the thread pool at once. - * 3. The class is {@link Finalizable}: `finalize()` clears the sweep timer so a graceful shutdown - * does not leave the interval behind (the timer is also `unref`'d as a safety net). + * 3. The next sweep is scheduled only after the previous one has finished, so a cleanup that takes + * longer than the timeout cannot overlap with the next run. The timer is `unref`'d, a failing + * sweep is logged instead of rejecting, and `finalize()` clears the pending run. */ export class PivotExpiringStorage implements ExpiringStorage, Finalizable { protected readonly logger = getLoggerFor(this); private readonly source: KeyValueStorage>; - private readonly timer: NodeJS.Timeout; + private readonly timeout: number; + private readonly jitter: number; private readonly batchSize: number; + private timer?: NodeJS.Timeout; + private finalized = false; /** * @param source - KeyValueStorage to actually store the data. * @param timeout - How often the expired data needs to be checked in minutes. - * @param jitter - Maximum fraction of the timeout that is randomly added to the interval so that + * @param jitter - Maximum fraction of the timeout that is randomly added before a sweep so that * multiple instances do not all sweep at the same time. `0` disables jitter. * @param batchSize - Maximum number of expired entries deleted concurrently. */ @@ -47,16 +51,10 @@ export class PivotExpiringStorage implements ExpiringStorage { @@ -93,7 +91,42 @@ export class PivotExpiringStorage implements ExpiringStorage { - clearInterval(this.timer); + this.finalized = true; + if (this.timer) { + clearTimeout(this.timer); + this.timer = undefined; + } + } + + /** + * Schedules the next sweep. Every delay gets a random jitter so that storages created at the same + * time do not sweep in the same instant. + */ + private scheduleSweep(): void { + const period = this.timeout * 60 * 1000; + const jitterMs = Math.floor(Math.random() * period * this.jitter); + const timer = setTimeout((): void => { + void this.sweep(); + }, period + jitterMs); + // A background sweep should never keep the Node.js process alive on its own. + timer.unref(); + this.timer = timer; + } + + /** + * Runs one sweep and schedules the next one afterwards, so overlapping sweeps are impossible. + * Errors are logged instead of thrown: the timer callback must never reject. + */ + private async sweep(): Promise { + try { + await this.removeExpiredEntries(); + } catch (error: unknown) { + this.logger.error(`Failed to remove expired entries: ${createErrorMessage(error)}`); + } finally { + if (!this.finalized) { + this.scheduleSweep(); + } + } } /** diff --git a/test/integration/ScopedSweeps.test.ts b/test/integration/ScopedSweeps.test.ts index b1abf28..5c72473 100644 --- a/test/integration/ScopedSweeps.test.ts +++ b/test/integration/ScopedSweeps.test.ts @@ -2,6 +2,9 @@ import { getDefaultVariables, getPresetConfigPath, instantiateFromConfig } from describe('A server configured with the pivot scoped sweeps', (): void => { const config = getPresetConfigPath('prod.json'); + // The store chain needs the companion customisation (it defines the UI converter used by the + // converting store), exactly like the production start scripts, which pass both files. + const companion = getPresetConfigPath('customise-me.json'); const variables = { ...getDefaultVariables(3000, 'http://localhost:3000/'), 'urn:solid-server:default:variable:rootFilePath': '/tmp/pivot-scoped-sweeps-test', @@ -10,7 +13,7 @@ describe('A server configured with the pivot scoped sweeps', (): void => { // Instantiates the given storage from the configuration and returns the entry container of its scoped JSON storage. // Class names are compared instead of instanceof: the configuration instantiates the built classes from dist/. async function getEntryContainer(storageId: string): Promise { - const storage = await instantiateFromConfig(storageId, config, variables) as any; + const storage = await instantiateFromConfig(storageId, [ config, companion ], variables) as any; expect(storage.constructor.name).toBe('PivotExpiringStorage'); expect(storage.source.constructor.name).toBe('ContainerPathStorage'); const scoped = storage.source.source.source; diff --git a/test/unit/storage/PivotExpiringStorage.test.ts b/test/unit/storage/PivotExpiringStorage.test.ts index 1d66567..5cee603 100644 --- a/test/unit/storage/PivotExpiringStorage.test.ts +++ b/test/unit/storage/PivotExpiringStorage.test.ts @@ -16,17 +16,17 @@ describe('A PivotExpiringStorage', (): void => { yesterday.setDate(yesterday.getDate() - 1); let source: jest.Mocked>; let storage: PivotExpiringStorage; - let mockInterval: jest.SpyInstance; + let mockTimeout: jest.SpyInstance; let mockClear: jest.SpyInstance; let mockRandom: jest.SpyInstance; let mockTimer: { unref: jest.Mock }; beforeEach((): void => { - // Never schedule a real sweep; capture the interval instead. + // Never schedule a real sweep; capture the scheduled timeout instead. mockTimer = { unref: jest.fn() }; - mockInterval = jest.spyOn(globalThis, 'setInterval') + mockTimeout = jest.spyOn(globalThis, 'setTimeout') .mockImplementation(jest.fn().mockReturnValue(mockTimer) as any); - mockClear = jest.spyOn(globalThis, 'clearInterval').mockImplementation(jest.fn() as any); + mockClear = jest.spyOn(globalThis, 'clearTimeout').mockImplementation(jest.fn() as any); // Fixed jitter source so the scheduled delay is deterministic. mockRandom = jest.spyOn(globalThis.Math, 'random').mockReturnValue(0.5); @@ -41,7 +41,7 @@ describe('A PivotExpiringStorage', (): void => { }); afterEach((): void => { - mockInterval.mockRestore(); + mockTimeout.mockRestore(); mockClear.mockRestore(); mockRandom.mockRestore(); }); @@ -129,25 +129,25 @@ describe('A PivotExpiringStorage', (): void => { }); describe('scheduling the cleanup sweep', (): void => { - it('schedules the sweep on the configured timeout when jitter is disabled.', (): void => { + it('schedules the first sweep on the configured timeout when jitter is disabled.', (): void => { storage = new PivotExpiringStorage(source, 1, 0); - expect(mockInterval).toHaveBeenCalledTimes(2); - expect(mockInterval.mock.calls[1]).toHaveLength(2); - expect(mockInterval.mock.calls[1][1]).toBe(60 * 1000); + expect(mockTimeout).toHaveBeenCalledTimes(2); + expect(mockTimeout.mock.calls[1]).toHaveLength(2); + expect(mockTimeout.mock.calls[1][1]).toBe(60 * 1000); }); - it('adds a jitter fraction to the scheduled sweep interval.', (): void => { + it('adds a jitter fraction to the scheduled sweep delay.', (): void => { // Math.random is 0.5 and jitter is 0.2, so floor(0.5 * 60000 * 0.2) = 6000 is added. storage = new PivotExpiringStorage(source, 1, 0.2); - expect(mockInterval).toHaveBeenCalledTimes(2); - expect(mockInterval.mock.calls[1][1]).toBe(60 * 1000 + 6000); + expect(mockTimeout).toHaveBeenCalledTimes(2); + expect(mockTimeout.mock.calls[1][1]).toBe(60 * 1000 + 6000); }); it('unrefs the timer so it does not keep the event loop alive.', (): void => { expect(mockTimer.unref).toHaveBeenCalledTimes(1); }); - it('removes expired entries when the scheduled sweep fires.', async(): Promise => { + it('removes expired entries when the scheduled sweep runs.', async(): Promise => { const data = [ [ 'key1', createExpires('data1', tomorrow) ], [ 'key2', createExpires('data2', yesterday) ], @@ -157,8 +157,8 @@ describe('A PivotExpiringStorage', (): void => { yield* data; }); - // Await the function the sweep interval was created with. - await (mockInterval.mock.calls[0][0] as () => Promise)(); + (mockTimeout.mock.calls[0][0] as () => void)(); + await flushPromises(); expect(source.delete).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenLastCalledWith('key2'); @@ -183,25 +183,75 @@ describe('A PivotExpiringStorage', (): void => { .mockImplementationOnce(async(): Promise => second) .mockResolvedValue(true); - const cleanup = (mockInterval.mock.calls[1][0] as () => Promise)(); + (mockTimeout.mock.calls[1][0] as () => void)(); await flushPromises(); expect(source.delete).toHaveBeenCalledTimes(2); resolveFirst(true); resolveSecond(true); - await cleanup; + await flushPromises(); expect(source.delete).toHaveBeenCalledTimes(3); }); + it('schedules the next sweep only after the running one finished.', async(): Promise => { + let resolveDelete!: (value: boolean) => void; + const deletion = new Promise((resolve): void => { + resolveDelete = resolve; + }); + source.entries.mockImplementationOnce(function* (): any { + yield [ 'key1', createExpires('data1', yesterday) ]; + }); + source.delete.mockImplementationOnce(async(): Promise => deletion); + + (mockTimeout.mock.calls[0][0] as () => void)(); + await flushPromises(); + // The sweep is still deleting: no new timeout may have been scheduled yet. + expect(mockTimeout).toHaveBeenCalledTimes(1); + + resolveDelete(true); + await flushPromises(); + expect(mockTimeout).toHaveBeenCalledTimes(2); + }); + + it('logs a failing sweep and still schedules the next one.', async(): Promise => { + source.entries.mockImplementationOnce((): any => { + throw new Error('sweep failure'); + }); + + (mockTimeout.mock.calls[0][0] as () => void)(); + await flushPromises(); + + expect(mockTimeout).toHaveBeenCalledTimes(2); + }); + it.each([ 0, -1, 1.5, Number.NaN ])('rejects invalid batch size %p.', (batchSize): void => { expect((): PivotExpiringStorage => new PivotExpiringStorage(source, 1, 0, batchSize)).toThrow(TypeError); }); - it('clears the timer on finalize.', async(): Promise => { + it('stops sweeping when finalized.', async(): Promise => { await expect(storage.finalize()).resolves.toBeUndefined(); expect(mockClear).toHaveBeenCalledTimes(1); expect(mockClear).toHaveBeenLastCalledWith(mockTimer); }); + + it('does not schedule another sweep when finalized while one is running.', async(): Promise => { + let resolveDelete!: (value: boolean) => void; + const deletion = new Promise((resolve): void => { + resolveDelete = resolve; + }); + source.entries.mockImplementationOnce(function* (): any { + yield [ 'key1', createExpires('data1', yesterday) ]; + }); + source.delete.mockImplementationOnce(async(): Promise => deletion); + + (mockTimeout.mock.calls[0][0] as () => void)(); + await flushPromises(); + await storage.finalize(); + resolveDelete(true); + await flushPromises(); + + expect(mockTimeout).toHaveBeenCalledTimes(1); + }); }); }); From 44b438836b85784f4d444c883f820cc2a2cbbc2b Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Wed, 23 Sep 2026 16:47:53 +0200 Subject: [PATCH 5/7] fix(storage): make the root check explicit and keep falsy values in has() ScopedJsonResourceStorage normalises the storage root in the containment check, so a storage rooted at /.internal/accounts/ cannot accept a sibling container such as /.internal/accounts-evil/. JsonResourceStorage already stores the root with a trailing slash so the previous form was segment-aware in practice, but the explicit normalisation keeps it that way whatever the root looks like; a unit test covers the sibling-prefix case. PivotExpiringStorage.has() now compares the retrieved value with undefined instead of coercing it, so falsy payloads ('', 0, false) are reported as present, like get() does. Covered by a unit test. --- src/storage/PivotExpiringStorage.ts | 4 +++- src/storage/ScopedJsonResourceStorage.ts | 4 +++- test/unit/storage/PivotExpiringStorage.test.ts | 6 ++++++ test/unit/storage/ScopedJsonResourceStorage.test.ts | 6 ++++++ 4 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/storage/PivotExpiringStorage.ts b/src/storage/PivotExpiringStorage.ts index fa5f945..8a0fced 100644 --- a/src/storage/PivotExpiringStorage.ts +++ b/src/storage/PivotExpiringStorage.ts @@ -62,7 +62,9 @@ export class PivotExpiringStorage implements ExpiringStorage { - return Boolean(await this.getUnexpired(key)); + // Compare against `undefined` instead of coercing, so falsy payloads (`''`, `0`, `false`) + // are reported as present, like `get` does. + return (await this.getUnexpired(key)) !== undefined; } public async set(key: TKey, value: TValue, expiration?: number): Promise; diff --git a/src/storage/ScopedJsonResourceStorage.ts b/src/storage/ScopedJsonResourceStorage.ts index 3dc203b..6f8359b 100644 --- a/src/storage/ScopedJsonResourceStorage.ts +++ b/src/storage/ScopedJsonResourceStorage.ts @@ -26,7 +26,9 @@ export class ScopedJsonResourceStorage extends JsonResourceStorage { public constructor(source: ResourceStore, baseUrl: string, container: string, entryContainer: string) { super(source, baseUrl, container); this.entryContainer = ensureTrailingSlash(joinUrl(baseUrl, entryContainer)); - if (!this.entryContainer.startsWith(this.container)) { + // Normalise the root as well so the comparison is a segment boundary: a storage rooted at + // `/.internal/accounts/` must not accept `/.internal/accounts-evil/`. + if (!this.entryContainer.startsWith(ensureTrailingSlash(this.container))) { throw new TypeError('The entry container must be inside the storage container.'); } } diff --git a/test/unit/storage/PivotExpiringStorage.test.ts b/test/unit/storage/PivotExpiringStorage.test.ts index 5cee603..a846a05 100644 --- a/test/unit/storage/PivotExpiringStorage.test.ts +++ b/test/unit/storage/PivotExpiringStorage.test.ts @@ -75,6 +75,12 @@ describe('A PivotExpiringStorage', (): void => { await expect(storage.has('key')).resolves.toBe(true); }); + it('treats a falsy value as present.', async(): Promise => { + source.get.mockResolvedValue(createExpires('', tomorrow)); + await expect(storage.get('key')).resolves.toBe(''); + await expect(storage.has('key')).resolves.toBe(true); + }); + it('deletes expired data when checking if it exists.', async(): Promise => { source.get.mockResolvedValueOnce(createExpires('data!', yesterday)); await expect(storage.has('key')).resolves.toBe(false); diff --git a/test/unit/storage/ScopedJsonResourceStorage.test.ts b/test/unit/storage/ScopedJsonResourceStorage.test.ts index 5b1044f..b7a5b1e 100644 --- a/test/unit/storage/ScopedJsonResourceStorage.test.ts +++ b/test/unit/storage/ScopedJsonResourceStorage.test.ts @@ -100,4 +100,10 @@ describe('A ScopedJsonResourceStorage', (): void => { new ScopedJsonResourceStorage(source, baseUrl, '/.internal/accounts/', '/.internal/idp/')) .toThrow('The entry container must be inside the storage container.'); }); + + it('rejects a sibling container that only shares the prefix of the storage root.', (): void => { + expect((): ScopedJsonResourceStorage => + new ScopedJsonResourceStorage(source, baseUrl, '/.internal/accounts/', '/.internal/accounts-evil/')) + .toThrow('The entry container must be inside the storage container.'); + }); }); From 9fcc1b8added79db90b88239741d0fba7f0f7aa1 Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Wed, 23 Sep 2026 17:07:02 +0200 Subject: [PATCH 6/7] fix(storage): validate the sweep timing and settle every batch before rescheduling timeout and jitter are validated when the storage is created (finite positive timeout, finite non-negative jitter), so a misconfigured value cannot produce a delay that Node clamps to about 1 ms and turn the sweep into a busy loop. Unit tests cover zero, negative, NaN and Infinity for both. A batch of deletes now waits for every delete of the batch to settle (Promise.allSettled) before a failure is propagated, so a rejecting delete cannot leave a sibling running while the next sweep is already scheduled. The CLEAR-LOCK.md snippets follow the implementation: the segment-aware containment check and the settle-then-fail batch. --- CLEAR-LOCK.md | 11 +++-- src/storage/PivotExpiringStorage.ts | 14 +++++- .../unit/storage/PivotExpiringStorage.test.ts | 48 +++++++++++++++++++ 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/CLEAR-LOCK.md b/CLEAR-LOCK.md index 0eb9272..a12542a 100644 --- a/CLEAR-LOCK.md +++ b/CLEAR-LOCK.md @@ -29,7 +29,7 @@ export class ScopedJsonResourceStorage extends JsonResourceStorage { public constructor(source: ResourceStore, baseUrl: string, container: string, entryContainer: string) { super(source, baseUrl, container); this.entryContainer = ensureTrailingSlash(joinUrl(baseUrl, entryContainer)); - if (!this.entryContainer.startsWith(this.container)) { + if (!this.entryContainer.startsWith(ensureTrailingSlash(this.container))) { throw new TypeError('The entry container must be inside the storage container.'); } } @@ -54,12 +54,17 @@ periodic sweep needs: (`jitter`, default `0.15`, `0` disables it), so the four instances created at startup no longer sweep in the same instant. 2. **Bounded deletes** — expired entries are deleted in batches of `batchSize` - (default `32`) instead of one `Promise.all` over the whole set: + (default `32`) instead of one `Promise.all` over the whole set, and a batch always settles + completely before the sweep can fail or finish: ```ts for (let index = 0; index < expired.length; index += this.batchSize) { - await Promise.all(expired.slice(index, index + this.batchSize) + const results = await Promise.allSettled(expired.slice(index, index + this.batchSize) .map(async(key): Promise => this.source.delete(key))); + const failure = results.find((result): result is PromiseRejectedResult => result.status === 'rejected'); + if (failure) { + throw failure.reason; + } } ``` 3. **No overlapping sweeps** - the next sweep is scheduled only after the running diff --git a/src/storage/PivotExpiringStorage.ts b/src/storage/PivotExpiringStorage.ts index 8a0fced..f5248c7 100644 --- a/src/storage/PivotExpiringStorage.ts +++ b/src/storage/PivotExpiringStorage.ts @@ -50,6 +50,12 @@ export class PivotExpiringStorage implements ExpiringStorage implements ExpiringStorage => this.source.delete(key))); + const failure = results.find((result): result is PromiseRejectedResult => result.status === 'rejected'); + if (failure) { + throw failure.reason; + } } this.logger.debug('Finished removing expired entries'); } diff --git a/test/unit/storage/PivotExpiringStorage.test.ts b/test/unit/storage/PivotExpiringStorage.test.ts index a846a05..59e2ecb 100644 --- a/test/unit/storage/PivotExpiringStorage.test.ts +++ b/test/unit/storage/PivotExpiringStorage.test.ts @@ -199,6 +199,41 @@ describe('A PivotExpiringStorage', (): void => { expect(source.delete).toHaveBeenCalledTimes(3); }); + it('waits for the whole batch before a failing sweep is rescheduled.', async(): Promise => { + storage = new PivotExpiringStorage(source, 1, 0, 2); + let rejectFirst!: (reason?: any) => void; + let resolveSecond!: (value: boolean) => void; + const first = new Promise((resolve, reject): void => { + rejectFirst = reject; + }); + const second = new Promise((resolve): void => { + resolveSecond = resolve; + }); + source.entries.mockImplementationOnce(function* (): any { + yield [ 'key1', createExpires('data1', yesterday) ]; + yield [ 'key2', createExpires('data2', yesterday) ]; + yield [ 'key3', createExpires('data3', yesterday) ]; + }); + source.delete.mockImplementationOnce(async(): Promise => first) + .mockImplementationOnce(async(): Promise => second) + .mockResolvedValue(true); + + (mockTimeout.mock.calls[1][0] as () => void)(); + await flushPromises(); + rejectFirst(new Error('delete failed')); + await flushPromises(); + // The sibling delete is still running: no next sweep and no further batch yet. Two timeouts + // exist so far: one for the storage created in beforeEach and one for this storage. + expect(mockTimeout).toHaveBeenCalledTimes(2); + expect(source.delete).toHaveBeenCalledTimes(2); + + resolveSecond(true); + await flushPromises(); + // The batch settled, the failure aborted the sweep, and only then the next sweep was scheduled. + expect(source.delete).toHaveBeenCalledTimes(2); + expect(mockTimeout).toHaveBeenCalledTimes(3); + }); + it('schedules the next sweep only after the running one finished.', async(): Promise => { let resolveDelete!: (value: boolean) => void; const deletion = new Promise((resolve): void => { @@ -235,6 +270,19 @@ describe('A PivotExpiringStorage', (): void => { new PivotExpiringStorage(source, 1, 0, batchSize)).toThrow(TypeError); }); + it.each([ + [ 0, 0 ], + [ -1, 0 ], + [ Number.NaN, 0 ], + [ Number.POSITIVE_INFINITY, 0 ], + [ 1, -0.1 ], + [ 1, Number.NaN ], + [ 1, Number.POSITIVE_INFINITY ], + ])('rejects an invalid timeout and jitter (%p, %p).', (timeout, jitter): void => { + expect((): PivotExpiringStorage => + new PivotExpiringStorage(source, timeout, jitter)).toThrow(TypeError); + }); + it('stops sweeping when finalized.', async(): Promise => { await expect(storage.finalize()).resolves.toBeUndefined(); expect(mockClear).toHaveBeenCalledTimes(1); From 9c4248132fc1939ef6d2f7db08dc1ea7b641f922 Mon Sep 17 00:00:00 2001 From: bourgeoa Date: Wed, 23 Sep 2026 18:23:47 +0200 Subject: [PATCH 7/7] fix(storage): reject a sweep delay beyond the setTimeout maximum The previous validation only rejected non-finite values, so a finite combination such as a timeout of 1e9 minutes, or a large finite jitter, still produced a delay above 2^31 - 1 ms. Node.js clamps such a delay to 1 ms, which would turn the sweep into the busy loop the validation is meant to prevent. The worst-case jittered delay (timeout * (1 + jitter)) is now validated against the largest delay setTimeout accepts and rejected with a TypeError, since long-delay chunking would add scheduling drift for a configuration that is a mistake anyway. Tests reject four overflowing combinations (huge timeout, huge jitter, one minute over the limit, and a mid-range timeout whose jitter overflows) and accept the largest supported delay. CLEAR-LOCK.md documents the delay ceiling. --- CLEAR-LOCK.md | 4 ++++ src/storage/PivotExpiringStorage.ts | 14 ++++++++++++++ test/unit/storage/PivotExpiringStorage.test.ts | 18 ++++++++++++++++++ 3 files changed, 36 insertions(+) diff --git a/CLEAR-LOCK.md b/CLEAR-LOCK.md index a12542a..4680965 100644 --- a/CLEAR-LOCK.md +++ b/CLEAR-LOCK.md @@ -191,6 +191,10 @@ above. operations keep the same per-resource locking as before this change. * **Defaults**: `jitter` 0.15 and `batchSize` 32 are constructor defaults; both can be set per store in the configuration. +* **Delay ceiling**: the worst-case jittered delay (`timeout * (1 + jitter)`) is validated + against the maximum `setTimeout` delay (2^31-1 ms, about 24.8 days). Longer delays are + clamped to 1 ms by Node.js, which would turn the sweep into a busy loop, so such a + configuration is rejected with a `TypeError` instead. * **Maintenance**: `PivotExpiringStorage` mirrors the body of CSS's `WrappedExpiringStorage`. It is a copy-in of a small, stable class (the three deltas are documented in its header) — if a future CSS version changes the stock diff --git a/src/storage/PivotExpiringStorage.ts b/src/storage/PivotExpiringStorage.ts index f5248c7..f5ae033 100644 --- a/src/storage/PivotExpiringStorage.ts +++ b/src/storage/PivotExpiringStorage.ts @@ -10,6 +10,12 @@ import type { KeyValueStorage, } from '@solid/community-server'; +/** + * The largest delay `setTimeout` accepts in milliseconds (about 24.8 days). Node.js clamps larger + * delays to 1 ms, so a sweep that asks for more would run in a busy loop instead of once. + */ +const MAX_TIMER_DELAY = 2 ** 31 - 1; + /** * A storage that wraps around another storage and expires resources based on the given (optional) * expiry date, with the same behaviour as the default `WrappedExpiringStorage`, plus three @@ -56,6 +62,14 @@ export class PivotExpiringStorage implements ExpiringStorage MAX_TIMER_DELAY) { + throw new TypeError( + `The jittered sweep delay cannot exceed the maximum ${MAX_TIMER_DELAY} ms, ` + + 'because setTimeout clamps larger delays to 1 ms.', + ); + } this.source = source; this.timeout = timeout; this.jitter = jitter; diff --git a/test/unit/storage/PivotExpiringStorage.test.ts b/test/unit/storage/PivotExpiringStorage.test.ts index 59e2ecb..3ba4726 100644 --- a/test/unit/storage/PivotExpiringStorage.test.ts +++ b/test/unit/storage/PivotExpiringStorage.test.ts @@ -283,6 +283,24 @@ describe('A PivotExpiringStorage', (): void => { new PivotExpiringStorage(source, timeout, jitter)).toThrow(TypeError); }); + it.each([ + // Finite values whose worst-case jittered delay exceeds the timer maximum are rejected, + // because Node.js would clamp such a delay to 1 ms and sweep in a busy loop. + [ 1e9, 0 ], + [ 60, 1e9 ], + [ 35792, 0 ], + [ 30000, 0.5 ], + ])('rejects a jittered delay above the timer maximum (%p, %p).', (timeout, jitter): void => { + expect((): PivotExpiringStorage => + new PivotExpiringStorage(source, timeout, jitter)).toThrow(TypeError); + }); + + it('accepts the largest delay the timer supports.', (): void => { + // 2147483647 ms (about 24.8 days) is the largest delay setTimeout accepts. + expect((): PivotExpiringStorage => + new PivotExpiringStorage(source, 35791, 0)).not.toThrow(); + }); + it('stops sweeping when finalized.', async(): Promise => { await expect(storage.finalize()).resolves.toBeUndefined(); expect(mockClear).toHaveBeenCalledTimes(1);