Conversation
jeswr
left a comment
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.
|
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. |
|
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. |
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. |
Suppressed comments (1)
src/server/notifications/KeyValueChannelStorage.ts:160
- Because
updateshares this identifier lock but accepts a missingoldChannel, an update that starts after this lock is acquired can run after the deletion and recreate only the channel record;updatedoes not restore the topic index in that path. The renewed channel then disappears fromgetAll(topic)even thoughget(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.
|
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. |
|
|
||
| public async finalize(): Promise<void> { | ||
| if (this.timer) { | ||
| clearInterval(this.timer); |
There was a problem hiding this comment.
From what I see, the reason this is a breaking change is because the interface of this class changes to also be a Finalizable. This is done because the timer is cleared at the end. But I don't think this is actually necessary. I was going to suggest to just call this.timer.unref, but you already do that, so there is no reason to worry about clearing the timer any more at the end. Similarly, there is no need to await the active sweep promise. The point of the finalize function is to clean stuff up that would cause issues if the server is restarted (e.g., cleaning the lock folder, which is something for which this is used). So if the finalize stuff is removed, this could then also target the main branch as that would make it no longer breaking.
There was a problem hiding this comment.
There is a distinction here between letting the process exit and stopping background work. timer.unref() only prevents the timer from keeping Node.js alive; it does not cancel the interval. If other work keeps the process alive after app.stop(), the old storage would continue sweeping without clearInterval().
There is also the in-flight case: clearing the interval does not stop a sweep that is already enumerating or deleting data. CSS runs Finalizer before CleanupFinalizer; the latter removes filesystem locks or closes the Redis locker. Waiting for the active sweep prevents it from accessing those dependencies after cleanup. This is why I have retained finalization and the current target branch.
I have documented that distinction and added fake-timer regressions for stopping later sweeps and awaiting pending deletions. The sweep error path also lets shutdown cleanup continue when an active sweep fails, with the error logged by setSafeInterval.
Reference: Node.js timer documentation.
Implemented in 8320aacc9. Build, lint, type checks, 2,288 unit tests, and 805 integration tests passed. Three Docker-dependent integration suites were skipped.
There was a problem hiding this comment.
Thanks for pointing out async that the goal here was to help make this a non-breaking change. I'll push changes to use unref.
Do you see value in having the finaliser design for the next mver as well (which I can open a separate PR for) - or shall we just drop the idea?
There was a problem hiding this comment.
Do you see value in having the finaliser design for the next mver as well (which I can open a separate PR for) - or shall we just drop the idea?
It's one of those things where it technically might be more correct to do it, but I wonder if it's worth the extra interface/configuration and other hassle. The finalize functions only triggers when the server is shutting down, which, unless you're running tests, is something that doesn't happen that often anyway (and it would have to happen specifically during such a run to even potentially cause an issue). And if it takes the long I see users being more likely to just force exit the process anyway. The main issue is potentially some inconsistencies in the lock folder, but that one gets cleared anyway on server start to prevent issues. So I would not look into this unless it turns out there actually are issues being caused by this.
|
@joachimvh I have addressed your comments. This is ready for re-review. |
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.
Keep finalization so sweeps stop and drain before backend cleanup, even when other work keeps the process alive after the server stops. Let the timer helper log sweep failures without blocking shutdown cleanup. Use Jest fake timers to exercise scheduled sweeps and shutdown behavior, and document why unreferencing the timer does not replace finalization.
Delegate asynchronous iteration to the original stream so N3 readable callbacks retain the correct stream identity. Renew the lock for each yielded chunk. This prevents notification sweeps over the streamed key-value backend from hanging during shutdown.
8320aac to
862fc7a
Compare
|
@joachimvh I've taken a closer look at this and ended up just rebasing this PR to the main branch. It is actually a Patch updates can target main, other changes should target the latest versions/* branch. |
|
@jeswr there might have been a misunderstanding. Because I see the PR is still making |
It's a class, not a type interface they need to implement - so they will inherit the implementation from The only place I can see it possibly being a breaking change is if custom configurations break if they do not have the new |
True, I was thinking wrong here. There are some minor edge cases but this is probably fine. |
📁 Related issues
Related to #2220.
✍️ Description
Expired notification channels whose topics go quiet can remain in storage indefinitely. Add a configurable background sweep that removes expired channel records and their topic-index entries.
The sweep runs every 60 minutes by default, with up to 15% jitter. Setting
sweepIntervalto0disables it. Sweeps cannot overlap within a storage instance, and each candidate is re-read and checked under its channel write lock before deletion so a concurrent renewal is preserved. Updates that recreate a removed channel also restore its topic index.The timer is unreferenced. The bundled notification storage configuration registers a finalizer that stops future sweeps and waits for the active sweep before backend cleanup. Custom configurations that instantiate this storage independently can register the same finalizer for shutdown cleanup.
This branch is rebased onto
main, using its existing logging module, v7 component context, and v7.3.0 release notes.NotificationChannelStorageand the stored data format are unchanged; the new constructor arguments are optional. Suggested classification:semver.minor.The rebase also exposed an existing incompatibility between
main's locked stream wrapper and asynchronous N3 stream iteration: the sweep could hang during enumeration and prevent shutdown. The focused fix delegates asynchronous iteration to the original stream while renewing the lock for each chunk. A regression test covers delayed N3 data and lock release, and the unchanged expiry integration test now completes shutdown.Each sweep enumerates the backing key-value storage, so its cost depends on the selected backend. Operators can lengthen the interval or disable sweeping.
Validation
On Node 22.21.1:
git diff --checkpassed.npm run validatestill fails parsing generatedAuxiliaryLinkMetadataWritermetadata. The identical error was reproduced after building a clean archive of the targetmaincommit (7a09132fc), so it is not introduced by this PR.✅ PR check list
semver.minor.mainas the target for this additive feature.