Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(webapp): fix Redis connection leak in realtime streams and broken abort signal propagation#3399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
fix(webapp): fix Redis connection leak in realtime streams and broken abort signal propagation #3399
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| --- | ||
| area: webapp | ||
| type: fix | ||
| --- | ||
| Fix Redis connection leak in realtime streams and broken abort signal propagation. | ||
| **Redis connections**: Non-blocking methods (ingestData, appendPart, getLastChunkIndex) now share a single Redis connection instead of creating one per request. streamResponse still uses dedicated connections (required for XREAD BLOCK) but now tears them down immediately via disconnect() instead of graceful quit(), with a 15s inactivity fallback. | ||
| **Abort signal**: request.signal is broken in Remix/Express due to a Node.js undici GC bug (nodejs/node#55428) that severs the signal chain when Remix clones the Request internally. Added getRequestAbortSignal() wired to Express res.on("close") via httpAsyncStorage, which fires reliably on client disconnect. All SSE/streaming routes updated to use it. | ||
ericallam marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -7,7 +7,7 @@ export type RealtimeStreamsOptions = { | ||
| redis: RedisOptions | undefined; | ||
| logger?: Logger; | ||
| logLevel?: LogLevel; | ||
| inactivityTimeoutMs?: number; // Close stream after this many ms of no new data (default: 60000) | ||
| inactivityTimeoutMs?: number; // Close stream after this many ms of no new data (default: 15000) | ||
| }; | ||
| // Legacy constant for backward compatibility (no longer written, but still recognized when reading) | ||
| @@ -23,10 +23,23 @@ type StreamChunk = | ||
| export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| private logger: Logger; | ||
| private inactivityTimeoutMs: number; | ||
| // Shared connection for short-lived non-blocking operations (XADD, XREVRANGE, EXPIRE). | ||
| // Lazily created on first use so we don't open a connection if only streamResponse is called. | ||
| private _sharedRedis: Redis | undefined; | ||
| constructor(private options: RealtimeStreamsOptions) { | ||
| this.logger = options.logger ?? new Logger("RedisRealtimeStreams", options.logLevel ?? "info"); | ||
| this.inactivityTimeoutMs = options.inactivityTimeoutMs ?? 60000; // Default: 60 seconds | ||
| this.inactivityTimeoutMs = options.inactivityTimeoutMs ?? 15000; // Default: 15 seconds | ||
| } | ||
| private get sharedRedis(): Redis { | ||
| if (!this._sharedRedis) { | ||
| this._sharedRedis = new Redis({ | ||
| ...this.options.redis, | ||
| connectionName: "realtime:shared", | ||
| }); | ||
| } | ||
| return this._sharedRedis; | ||
ericallam marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| async initializeStream( | ||
| @@ -43,7 +56,7 @@ export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| signal: AbortSignal, | ||
| options?: StreamResponseOptions | ||
| ): Promise<Response> { | ||
| const redis = new Redis(this.options.redis ?? {}); | ||
| const redis = new Redis({ ...this.options.redis, connectionName: "realtime:streamResponse" }); | ||
| const streamKey = `stream:${runId}:${streamId}`; | ||
| let isCleanedUp = false; | ||
| @@ -269,7 +282,10 @@ export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| async function cleanup() { | ||
| if (isCleanedUp) return; | ||
| isCleanedUp = true; | ||
| await redis.quit().catch(console.error); | ||
| // disconnect() tears down the TCP socket immediately, which causes any | ||
| // pending XREAD BLOCK to reject right away instead of waiting for the | ||
| // block timeout to elapse. quit() would queue behind the blocking command. | ||
| redis.disconnect(); | ||
| } | ||
| signal.addEventListener("abort", cleanup, { once: true }); | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| @@ -290,22 +306,12 @@ export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| clientId: string, | ||
| resumeFromChunk?: number | ||
| ): Promise<Response> { | ||
| const redis = new Redis(this.options.redis ?? {}); | ||
| const redis = this.sharedRedis; | ||
| const streamKey = `stream:${runId}:${streamId}`; | ||
| const startChunk = resumeFromChunk ?? 0; | ||
| // Start counting from the resume point, not from 0 | ||
| let currentChunkIndex = startChunk; | ||
| const self = this; | ||
| async function cleanup() { | ||
| try { | ||
| await redis.quit(); | ||
| } catch (error) { | ||
| self.logger.error("[RedisRealtimeStreams][ingestData] Error in cleanup:", { error }); | ||
| } | ||
| } | ||
| try { | ||
| const textStream = stream.pipeThrough(new TextDecoderStream()); | ||
| const reader = textStream.getReader(); | ||
| @@ -361,13 +367,11 @@ export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| this.logger.error("[RealtimeStreams][ingestData] Error in ingestData:", { error }); | ||
| return new Response(null, { status: 500 }); | ||
| } finally { | ||
| await cleanup(); | ||
| } | ||
| } | ||
| async appendPart(part: string, partId: string, runId: string, streamId: string): Promise<void> { | ||
| const redis = new Redis(this.options.redis ?? {}); | ||
| const redis = this.sharedRedis; | ||
| const streamKey = `stream:${runId}:${streamId}`; | ||
| await redis.xadd( | ||
| @@ -386,12 +390,10 @@ export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| // Set TTL for cleanup when stream is done | ||
| await redis.expire(streamKey, env.REALTIME_STREAM_TTL); | ||
| await redis.quit(); | ||
| } | ||
| async getLastChunkIndex(runId: string, streamId: string, clientId: string): Promise<number> { | ||
| const redis = new Redis(this.options.redis ?? {}); | ||
| const redis = this.sharedRedis; | ||
| const streamKey = `stream:${runId}:${streamId}`; | ||
| try { | ||
| @@ -460,10 +462,6 @@ export class RedisRealtimeStreams implements StreamIngestor, StreamResponder { | ||
| }); | ||
| // Return -1 to indicate we don't know what the server has | ||
| return -1; | ||
| } finally { | ||
| await redis.quit().catch((err) => { | ||
| this.logger.error("[RedisRealtimeStreams][getLastChunkIndex] Error in cleanup:", { err }); | ||
| }); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.