Skip to content

fix: scope the internal expiration sweeps to their own containers - #149

Open
bourgeoa wants to merge 7 commits into
mainfrom
clearLock
Open

bourgeoa wants to merge 7 commits into
mainfrom
clearLock

Conversation

@bourgeoa

@bourgeoa bourgeoa commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

This component is using the work made by @jeswr for solidcommunity.net implementation

Problem

CSS keeps its internal key-value stores under /.internal/ — login cookies,
forgot-password requests, WebID ownership tokens, OIDC adapter state. Each is
wrapped in an expiring storage whose periodic sweep deletes expired entries, and
the stock wrapper chain delegates entries() to a JsonResourceStorage rooted
at /.internal/: the keys are filtered only after the whole internal tree has
been walked recursively.

With a large account base that walk is expensive (every account's data and index,
the IDP records, the lock files), the four stores are all created at startup with
the same interval so their sweeps fire in the same instant, and each sweep deletes
everything expired in one unbounded Promise.all, with every delete going through
the locking store.

Observed on a server with ~1000 accounts: 95–100 % CPU with no client traffic,
latency spikes while a sweep ran, and sweep deletes competing with requests for
the resource locks.

Fix

Two Pivot components plus configuration; CSS itself is untouched.

  • ScopedJsonResourceStorage — a JsonResourceStorage whose entries() starts in
    a descendant container. Key mapping, key hashing, identifiers and the on-disk
    layout are unchanged, so it slots into the existing wrapper chain and no stored
    data moves.
  • PivotExpiringStorage — the same expiry semantics as WrappedExpiringStorage,
    plus a jitter on the sweep interval (default 0.15 of the timeout), deletions in
    bounded batches (default 32) and a finalize() that clears the sweep timer.
  • config/pivot-scoped-sweeps.json — overrides the four stores to
    PivotExpiringStorage → ContainerPathStorage → MaxKeyLengthStorage → ScopedJsonResourceStorage(entryContainer = the store's own container),
    redeclares IdpAdapterFactory (the stock 7.2.0 config declares its storage
    inline, without an @id to override) and registers the four stores in
    urn:solid-server:default:Finalizer so the timers stop on shutdown.

Imported by prod.json, suffix.json, dev-http-suffix.json and
dev-http-subdomain.json; test.json is excluded (it has no accounts, so the
stores it would override do not exist there). The sweep interval policy is
unchanged (timeout: 1 minute — previously inline overrides in prod.json /
suffix.json, now part of the new file).

The four chains keep the stock locked ResourceStore as their source (the ResourceStore_Locking middleware), so internal reads and writes take the same per-resource locks as before this PR; a sweep only changes where it enumerates and when it runs. /.internal/ stays hidden from clients.

Verification

  • npx jest — 6 suites / 67 tests, including new unit coverage for both
    components (scoped enumeration keeps root-relative keys; long keys are still
    hashed and deleted through the wrapper stack; jitter, unref, batched deletes,
    finalize) and a configuration test that instantiates all four stores from
    prod.json and asserts the chains and containers.
  • npm run build (tsc + Components.js generator) clean.
  • Instantiated the whole app graph (urn:solid-server:default:App) and
    urn:solid-server:default:Finalizer (5 handlers: server initializer + the four
    expiring stores) with Components.js.
  • The scoped walk was measured in production on a ~1000-account server: CPU from
    over 95 % with no traffic down to below 1 %, latency flat.

Documentation

CLEAR-LOCK.md — implementation, verification, decisions and limitations.

Notes

  • Pivot keeps its own copy of the expiring storage body (three documented deltas).
    A container-scoped entries() in CSS, or an @id for the adapter storage, would
    make both components unnecessary.
  • The lock configuration (file vs Redis, retry bounds) is orthogonal and unchanged.

…orage

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.
…ners

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.
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.
@bourgeoa
bourgeoa requested review from michielbdejong and a lite review from Copilot September 22, 2026 16:30
@bourgeoa
bourgeoa requested review from jeswr and removed request for michielbdejong September 22, 2026 16:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical issues remain with bypassed storage locking and overlapping asynchronous sweeps.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)
What changed in this PR

This pull request scopes internal expiration sweeps to their own containers and improves cleanup scheduling.

Changes:

  • Adds scoped JSON storage and batched, jittered expiration handling.
  • Updates production and development storage configurations.
  • Adds tests, exports, and implementation documentation.
File Description
test/​unit/​storage/​ScopedJsonResourceStorage.test.ts Tests scoped enumeration and key mapping.
test/​unit/​storage/​PivotExpiringStorage.test.ts Tests expiry, batching, jitter, and finalization.
test/​integration/​ScopedSweeps.test.ts Verifies configured storage chains.
src/​storage/​ScopedJsonResourceStorage.ts Scopes resource enumeration.
src/​storage/​PivotExpiringStorage.ts Adds jittered, batched expiration cleanup.
src/​index.ts Exports the new storage classes.
config/​suffix.json Imports scoped sweep configuration.
config/​prod.json Imports scoped sweep configuration.
config/​pivot-scoped-sweeps.json Defines replacement storage chains and finalizers.
config/​dev-http-suffix.json Imports scoped sweep configuration.
config/​dev-http-subdomain.json Imports scoped sweep configuration.
CLEAR-LOCK.md Documents implementation and verification.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread config/pivot-scoped-sweeps.json Outdated
Comment thread src/storage/PivotExpiringStorage.ts Outdated
@jeswr

jeswr commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Is it possible to directly use the components proposed upstream, either installing the branch on https://github.com/CommunitySolidServer/CommunitySolidServer or from the alpha release I made? We should avoid having similar-but-different code as much as possible as it is going to create a maintenance nightmare for us.

@bourgeoa

Copy link
Copy Markdown
Member Author

@jeswr I don't want maintenance. the component shall be deleted as soon one is merged in published CSS or any other way if chosen. There is no data change nor in pods nor in .internals

That's also the reason why

  • I do not want to publish as CSS component outside Pivot
  • I also removed/closed my proposal on CSS to only keep yours

Users are in a situation where their servers are not performing well

Your proposals:

  • We are not in situation to create branches in or publish from CSS.
  • using your fork created an install issue on the test server. I had to use npm install --force

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).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Unresolved critical path-boundary and moderate storage-sweep/configuration findings require changes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve falsy values in has existence checks

src/​storage/​PivotExpiringStorage.ts:65

has is an existence check, but coercing the payload with Boolean makes valid falsy values ('', 0, or false) look absent even though get returns them. Compare the result with undefined instead of truthiness so the storage contract works for all TValues.

Comment thread src/storage/ScopedJsonResourceStorage.ts Outdated
Comment thread config/pivot-scoped-sweeps.json
…as()

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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Critical issues remain in sweep failure handling and timer parameter validation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity · 1 Low severity

Open (3)
Resolved since last review (2)

Comment thread src/storage/PivotExpiringStorage.ts
Comment thread src/storage/PivotExpiringStorage.ts Outdated
Comment thread CLEAR-LOCK.md Outdated
… 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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Timer validation must prevent delays exceeding Node’s setTimeout limit to avoid busy-loop sweeps.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (3)

Comment thread src/storage/PivotExpiringStorage.ts
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants