fix: prevent context switch race and clear warning interval on shutdown#616
fix: prevent context switch race and clear warning interval on shutdown#616ashsolei wants to merge 1 commit intoczlonkowski:mainfrom
Conversation
Two issues in http-server-single-session.ts: 1. switchSessionContext race condition: when concurrent requests trigger context switches for the same session, request B awaited A's lock then returned without performing its own switch. B's context update was silently discarded, causing the session to operate against the wrong n8n instance. Fix: re-enter switchSessionContext after awaiting the lock so B's switch is evaluated and applied if still needed. 2. Default token warning setInterval leaked: the interval handle was never stored, so shutdown() could not clear it. This prevented GC of the server instance and caused timer callbacks to fire after shutdown. Fix: store the interval in this.warningTimer and clear it in shutdown().
There was a problem hiding this comment.
Pull request overview
Fixes two session-management issues in SingleSessionHTTPServer that can affect correctness and resource cleanup during HTTP server operation, especially in multi-tenant “shared session” mode.
Changes:
- Prevents dropped context updates by re-entering
switchSessionContext()after awaiting an in-flight context switch. - Stores the default-token warning
setIntervalhandle and clears it duringshutdown()to avoid a timer leak.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // to perform our own switch (the previous switch may have been | ||
| // for a different context than what we need) | ||
| await existingLock; | ||
| return; | ||
| // Re-enter instead of returning — our context may differ from what was just set | ||
| return this.switchSessionContext(sessionId, newContext); |
There was a problem hiding this comment.
There are unit tests covering the session cleanup timer lifecycle, but there doesn’t appear to be a regression test for the multi-tenant shared-mode context switch race this change is fixing (concurrent calls for the same sessionId with different contexts). Please add a test that triggers two overlapping context switches and asserts the final session/server instanceContext matches the last request (i.e., no update is dropped after waiting on an existing lock).
| // Stop default token warning timer | ||
| if (this.warningTimer) { | ||
| clearInterval(this.warningTimer); | ||
| this.warningTimer = null; | ||
| } |
There was a problem hiding this comment.
The interval is now stored and cleared on shutdown, but there doesn’t seem to be test coverage that the default-token warning interval is actually started (in non-production) and then cleared during shutdown. Adding a unit test with fake timers/spies that sets the default AUTH_TOKEN, calls start(), then shutdown(), and asserts clearInterval was invoked for warningTimer would prevent this leak from regressing.
|
Closing — will maintain fixes in a private mirror instead. Thank you for the review. |
Problem
Two issues in the HTTP server session management:
1. Context Switch Race Condition (HIGH)
When concurrent HTTP requests trigger
switchSessionContextfor the same session with different contexts:awaits it, then returns without performing its own switchThis is a data integrity issue in multi-tenant deployments where requests for different n8n instances can interleave on the same session.
2. Warning Interval Leak (LOW)
When using the default auth token in non-production, a
setInterval(every 5 minutes) was started but the handle was never stored or cleared. Onshutdown(), this timer continues firing, holding references to the logger and server instance, preventing garbage collection.Fix
Context switch: After awaiting the existing lock, re-enter
switchSessionContextinstead of returning. The re-entered call will check if the context still differs (viaJSON.stringifycomparison inperformContextSwitch) and apply B's switch if needed. This is safe because the lock is released before re-entry.Interval leak: Store the interval handle in
this.warningTimerand clear it inshutdown()alongside the existing cleanup timer.Validation
src/http-server-single-session.ts(13 insertions, 3 deletions)