fix: make WrappedExpiringStorage finalizable and jitter expiry sweeps - #2224
Conversation
Sorry, something went wrong.
WrappedExpiringStorage arms a periodic expiry-sweep interval but only unref'd it, so the timer was never cleared on a graceful shutdown and the instances were part of no finalizer chain. Restore the Finalizable contract: the class again implements Finalizable and exposes finalize(), which clearInterval()s the timer (unref is kept as a safety net). All four config-instantiated instances (cookies, forgot-password, OIDC adapter, ownership tokens) are wired into urn:solid-server:default:Finalizer via FinalizableHandler. The four instances are all created at startup and swept on the same fixed cadence, so their sweeps thundered together and caused periodic latency spikes. Add an optional jitter constructor parameter (fraction of the timeout, default 0.15) that randomly offsets each instance's interval so the sweeps spread out. The parameter is optional and wired through Components.js with a TS default, so existing configs are unchanged, and jitter is injectable (jitter = 0 disables it) for deterministic tests. Both changes are invisible to Solid clients: sweeps still run on essentially the same cadence; only shutdown cleanup and inter-instance scheduling change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The added JSDoc was more verbose than the surrounding storage source; condense the class doc to the load-bearing facts (jittered sweep interval, timer cleared on finalize), fold the jitter rationale into one clause, and drop the finalize() method doc to match the other Finalizable implementations (the interface carries the contract). No behaviour change. Model: claude-fable-5 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jeswr
left a comment
There was a problem hiding this comment.
This is a configuration change so should be documented in the release notes
Sorry, something went wrong.
|
Addressed the release-note feedback in 99c9845 by documenting the default configuration finalization changes and the new Finalizable/jitter interface behavior. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This PR improves lifecycle management for expiring key-value storages by making WrappedExpiringStorage finalizable (so its periodic sweep timer is stopped during graceful shutdown) and desynchronizing sweep schedules via configurable jitter. It also updates the default identity configuration to register the relevant storages with the application finalizer and expands unit test coverage for the new timer behavior.
Changes:
- Make WrappedExpiringStorage implement Finalizable, clear its sweep interval on finalize(), and add a configurable sweep-interval jitter (default 15%).
- Register default identity WrappedExpiringStorage instances with the server finalizer (cookies, forgot-password, OIDC adapter storage, ownership tokens) and add an identifier for the adapter storage.
- Update unit tests and release notes to cover/describe the new scheduling and shutdown behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file| File | Description |
|---|---|
| test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts | Adds unit tests for sweep scheduling (including jitter), unref(), sweep execution, and finalization cleanup. |
| src/storage/keyvalue/WrappedExpiringStorage.ts | Implements Finalizable, adds jittered scheduling for sweep interval, and clears the timer during finalization. |
| RELEASE_NOTES.md | Documents finalizer registration expectations and the new optional jitter behavior. |
| config/identity/ownership/token.json | Registers ownership-token expiring storage with the application finalizer; adds async-handlers context. |
| config/identity/handler/storage/password.json | Registers forgot-password expiring storage with the application finalizer; adds async-handlers context. |
| config/identity/handler/storage/default.json | Registers cookie expiring storage with the application finalizer. |
| config/identity/handler/base/adapter-factory.json | Gives the OIDC adapter expiring storage an @id and registers it with the application finalizer; adds async-handlers context. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
This has been reviewed by me and is ready for maintainer review |
Sorry, something went wrong.
📁 Related issues
Related to #2223.
This is the versions/next-major port of jeswr/CommunitySolidServer#79.
✍️ Description
Makes WrappedExpiringStorage implement Finalizable and clears its periodic expiry-sweep interval during graceful shutdown. The four instances created by the default identity configuration—cookies, forgot-password data, OIDC adapter data, and ownership tokens—are registered with the application finalizer. The previously anonymous OIDC adapter storage receives an identifier so it can be referenced by its FinalizableHandler.
The sweep interval also receives a configurable random positive jitter, defaulting to at most 15%, so instances constructed together do not enumerate their backing stores at the same instant. Passing 0 disables jitter, and the timer remains unref'd as a safety net.
The next-major port retains the v8 global logger import and adds the asynchronous-handlers context where the new ParallelHandler configuration requires it.
Suggested label: semver.minor.
Review question: should the 15% jitter remain the default, or should existing configurations retain an exact hourly cadence unless they opt in?
Validation:
✅ PR check list
Before this pull request can be merged, a core maintainer will check whether