-
-
Notifications
You must be signed in to change notification settings - Fork 867
Only allow a single dev queue consumer to dequeue at once #1737
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { attributesFromAuthenticatedEnv, tracer } from "../tracer.server"; | |
| import { getMaxDuration } from "../utils/maxDuration"; | ||
| import { DevSubscriber, devPubSub } from "./devPubSub.server"; | ||
| import { findQueueInEnvironment, sanitizeQueueName } from "~/models/taskQueue.server"; | ||
| import { createRedisClient, RedisClient } from "~/redis.server"; | ||
|
|
||
| const MessageBody = z.discriminatedUnion("type", [ | ||
| z.object({ | ||
|
|
@@ -53,14 +54,21 @@ export class DevQueueConsumer { | |
| private _currentSpan: Span | undefined; | ||
| private _endSpanInNextIteration = false; | ||
| private _inProgressRuns: Map<string, string> = new Map(); // Keys are task run friendly IDs, values are TaskRun internal ids/queue message ids | ||
| private _connectionLostAt?: Date; | ||
| private _redisClient: RedisClient; | ||
|
|
||
| constructor( | ||
| public id: string, | ||
| public env: AuthenticatedEnvironment, | ||
| private _sender: ZodMessageSender<typeof serverWebsocketMessages>, | ||
| private _options: DevQueueConsumerOptions = {} | ||
| ) { | ||
| this._traceTimeoutSeconds = _options.traceTimeoutSeconds ?? 60; | ||
| this._maximumItemsPerTrace = _options.maximumItemsPerTrace ?? 1_000; | ||
| this._redisClient = createRedisClient("tr:devQueueConsumer", { | ||
| keyPrefix: "tr:devQueueConsumer:", | ||
| ...devPubSub.redisOptions, | ||
| }); | ||
| } | ||
|
|
||
| // This method is called when a background worker is deprecated and will no longer be used unless a run is locked to it | ||
|
|
@@ -235,6 +243,8 @@ export class DevQueueConsumer { | |
| return; | ||
| } | ||
|
|
||
| await this._redisClient.set(`connection:${this.env.id}`, this.id); | ||
|
|
||
| this._enabled = true; | ||
| // Create the session | ||
| await createNewSession(this.env, this._options.ipAddress ?? "unknown"); | ||
|
|
@@ -252,6 +262,37 @@ export class DevQueueConsumer { | |
| return; | ||
| } | ||
|
|
||
| const canSendMessage = await this._sender.validateCanSendMessage(); | ||
|
|
||
| if (!canSendMessage) { | ||
| this._connectionLostAt ??= new Date(); | ||
|
|
||
| if (Date.now() - this._connectionLostAt.getTime() > 60 * 1000) { | ||
| logger.debug("Connection lost for more than 60 seconds, stopping the consumer", { | ||
| env: this.env, | ||
| }); | ||
|
|
||
| await this.stop("Connection lost for more than 60 seconds"); | ||
| return; | ||
| } | ||
|
|
||
| setTimeout(() => this.#doWork(), 1000); | ||
| } | ||
matt-aitken marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| this._connectionLostAt = undefined; | ||
|
|
||
| const currentConnection = await this._redisClient.get(`connection:${this.env.id}`); | ||
|
|
||
| if (currentConnection !== this.id) { | ||
| logger.debug("Another connection is active, stopping the consumer", { | ||
| currentConnection, | ||
| env: this.env, | ||
| }); | ||
|
|
||
| await this.stop("Another connection is active"); | ||
| return; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Assess concurrency handling |
||
|
|
||
| // Check if the trace has expired | ||
| if ( | ||
| this._perTraceCountdown === 0 || | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add TTL or cleanup routine for the Redis key
Setting
connection:${this.env.id}may lead to stale entries if the consumer crashes. Consider using an expiration or a cleanup process, so inactive connections won't block new consumers.