fix: await async session removal in cleanupExpiredSessions#615
fix: await async session removal in cleanupExpiredSessions#615ashsolei wants to merge 1 commit intoczlonkowski:mainfrom
Conversation
cleanupExpiredSessions() called the async removeSession() method without awaiting it, causing fire-and-forget promises that could silently fail. Changed the method to async and added await to ensure each expired session is properly removed before proceeding to the next one.
There was a problem hiding this comment.
Pull request overview
This PR fixes session cleanup behavior in SingleSessionHTTPServer by ensuring expired sessions are removed deterministically, instead of creating “fire-and-forget” removal promises during periodic cleanup.
Changes:
- Converted
cleanupExpiredSessions()toasync Promise<void>. - Awaited
removeSession()for each expired session during cleanup.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const sessionId of expiredSessions) { | ||
| this.removeSession(sessionId, 'expired'); | ||
| await this.removeSession(sessionId, 'expired'); | ||
| } |
There was a problem hiding this comment.
Although this now awaits removeSession, removeSession() catches and swallows all errors internally, so failures won’t reject back to cleanupExpiredSessions (and the outer cleanup try/catch won’t see them). If the goal is to surface errors to the caller, consider rethrowing after logging (or returning a result/using Promise.allSettled and reporting failures explicitly).
| private async cleanupExpiredSessions(): Promise<void> { | ||
| const now = Date.now(); | ||
| const expiredSessions: string[] = []; |
There was a problem hiding this comment.
cleanupExpiredSessions is now async and can take non-trivial time (especially since it awaits each removal). Because it’s run from setInterval, a new tick can start while a previous cleanup is still in progress, leading to overlapping cleanups and reintroducing race conditions. Consider adding an in-progress guard/lock (or switching to a self-scheduling setTimeout loop) so only one cleanup runs at a time.
|
Closing — will maintain fixes in a private mirror instead. Thank you for the review. |
Problem
cleanupExpiredSessions()callsremoveSession()— an async method — without awaiting it. This creates fire-and-forget promises that silently swallow errors and allow race conditions when multiple sessions expire simultaneously.Fix
cleanupExpiredSessions()fromvoidtoasync Promise<void>awaitbefore eachremoveSession()call in the cleanup loopThis ensures each expired session is properly removed (and errors surfaced) before proceeding to the next one.
Validation