feat: sweep expired notification channels from storage - #2222
Conversation
Sorry, something went wrong.
Periodically remove expired notification channels that would otherwise remain forever when their topics go quiet. Revalidate each candidate under its channel lock before deletion, keep active and indefinite channels, jitter and unref the timer, and clear it through the configured finalizer.
jeswr
left a comment
There was a problem hiding this comment.
This is a configuration change so I expect there to be updates to the release notes.
Sorry, something went wrong.
|
Addressed the release-note feedback in a79eb6a by documenting the default sweep interval and jitter, the disable option, and the Finalizer requirement for custom configurations. |
Sorry, something went wrong.
|
Fixed the CI failures in f33ba77. The notification storage config used ParallelHandler and FinalizableHandler without importing the asynchronous-handlers JSON-LD context, causing all configuration-backed suites to fail before server startup. The failed integration repro and all notification-storage unit tests now pass locally. |
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds configurable, jittered background cleanup for expired notification channels and topic-index entries.
Changes:
- Adds periodic expiry sweeping with renewal revalidation.
- Registers timer cleanup during finalization.
- Adds configuration, tests, and release notes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Summary |
|---|---|
| test/unit/server/notifications/KeyValueChannelStorage.test.ts | Adds coverage for sweeping, cleanup, races, and finalization. |
| src/server/notifications/KeyValueChannelStorage.ts | Implements sweeping and timer management. Two moderate issues remain: await in-flight sweeps during finalization and prevent overlapping sweeps. |
| RELEASE_NOTES.md | Documents the new behavior and configuration. |
| config/http/notifications/base/storage.json | Registers storage finalization. |
src/server/notifications/KeyValueChannelStorage.ts:160
- Because update shares this identifier lock but accepts a missing oldChannel, an update that starts after this lock is acquired can run after the deletion and recreate only the channel record; update does not restore the topic index in that path. The renewed channel then disappears from getAll(topic) even though get(id) finds it. Please make the update/delete protocol preserve the index (or reject updates that race with deletion).
await this.locker.withWriteLock(this.getLockKey(id), async(): Promise<void> => {
const channel = await this.storage.get(encodeURIComponent(id));
if (channel && this.isChannel(channel) && typeof channel.endAt === 'number' && channel.endAt < Date.now()) {
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
Also addressed the update/delete race noted in the review summary: when an update acquires the channel lock after deletion, it now restores both the channel record and its topic-index entry. The regression test verifies the two records remain consistent. |
Sorry, something went wrong.
📁 Related issues
Related to #2220.
This is the versions/next-major port of jeswr/CommunitySolidServer#86.
✍️ Description
Adds a configurable background sweep to KeyValueChannelStorage so expired notification channels and their topic-index entries do not remain indefinitely when they are never read again.
The storage checks once per hour by default, adds jitter to avoid synchronized sweeps, unrefs its timer, and clears the timer during server finalization. Setting the interval to 0 disables sweeping.
This port also closes a renewal race in the original draft: candidates are enumerated without mutation, then each channel is re-read and revalidated under its identifier write lock before deletion. A channel renewed while waiting for that lock is retained.
Suggested label: semver.minor.
Review note: every sweep enumerates the channel backend, so the default-on hourly cost depends on the selected key-value storage. Deployments can disable the sweep with an interval of 0.
Validation:
npm run validate currently fails while parsing the generated AuxiliaryLinkMetadataWriter component metadata. The same failure was reproduced on a pristine origin/versions/next-major worktree.
✅ PR check list
Before this pull request can be merged, a core maintainer will check whether