-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(run-engine): heartbeat batch items so slow ones are not run twice #4569
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -59,6 +59,9 @@ const ENV_CONCURRENCY_KEY_PREFIX = "batch:env_concurrency"; | |||||||||
| // then all messages are routed to this queue for BatchQueue's own consumer loop. | ||||||||||
| const BATCH_WORKER_QUEUE_ID = "batch-worker-queue"; | ||||||||||
|
|
||||||||||
| /** How long a claimed batch item stays invisible before the reclaim loop takes it back. */ | ||||||||||
| const BATCH_ITEM_VISIBILITY_TIMEOUT_MS = 60_000; | ||||||||||
|
|
||||||||||
| export class BatchQueue { | ||||||||||
| private fairQueue: FairQueue<typeof BatchItemPayloadSchema>; | ||||||||||
| private workerQueueManager: WorkerQueueManager; | ||||||||||
|
|
@@ -67,6 +70,8 @@ export class BatchQueue { | |||||||||
| private tracer?: Tracer; | ||||||||||
| private concurrencyRedis: Redis; | ||||||||||
| private defaultConcurrency: number; | ||||||||||
| private heartbeatIntervalMs: number; | ||||||||||
| private visibilityTimeoutMs: number; | ||||||||||
| private maxAttempts: number; | ||||||||||
|
|
||||||||||
| private processItemCallback?: ProcessBatchItemCallback; | ||||||||||
|
|
@@ -95,6 +100,8 @@ export class BatchQueue { | |||||||||
| this.logger = options.logger ?? new Logger("BatchQueue", options.logLevel ?? "info"); | ||||||||||
| this.tracer = options.tracer; | ||||||||||
| this.defaultConcurrency = options.defaultConcurrency ?? 10; | ||||||||||
| this.visibilityTimeoutMs = options.visibilityTimeoutMs ?? BATCH_ITEM_VISIBILITY_TIMEOUT_MS; | ||||||||||
| this.heartbeatIntervalMs = Math.max(50, Math.floor(this.visibilityTimeoutMs / 3)); | ||||||||||
|
Comment on lines
+103
to
+104
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Validate the visibility-timeout contract. For a timeout below 50 ms, the first heartbeat occurs after the lease expires. Values near 50 ms also have no scheduling margin. This can reclaim an active item and cause duplicate processing.
📍 Affects 2 files
|
||||||||||
| this.maxAttempts = options.retry?.maxAttempts ?? 1; | ||||||||||
| this.abortController = new AbortController(); | ||||||||||
| this.workerQueueBlockingTimeoutSeconds = options.workerQueueBlockingTimeoutSeconds ?? 10; | ||||||||||
|
|
@@ -154,7 +161,8 @@ export class BatchQueue { | |||||||||
| shardCount: options.shardCount ?? 1, | ||||||||||
| consumerCount: options.consumerCount, | ||||||||||
| consumerIntervalMs: options.consumerIntervalMs, | ||||||||||
| visibilityTimeoutMs: 60_000, // 1 minute for batch item processing | ||||||||||
| visibilityTimeoutMs: this.visibilityTimeoutMs, | ||||||||||
| heartbeatIntervalMs: this.visibilityTimeoutMs, | ||||||||||
|
Comment on lines
+164
to
+165
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Pass the derived heartbeat interval to Line 165 passes Proposed fix- heartbeatIntervalMs: this.visibilityTimeoutMs,
+ heartbeatIntervalMs: this.heartbeatIntervalMs,📝 Committable suggestion
Suggested change
|
||||||||||
| startConsumers: false, // We control when to start | ||||||||||
| cooloff: { | ||||||||||
| enabled: false, | ||||||||||
|
|
@@ -752,6 +760,44 @@ export class BatchQueue { | |||||||||
| // Private - Message Handling | ||||||||||
| // ============================================================================ | ||||||||||
|
|
||||||||||
| /** | ||||||||||
| * Keep extending a message's visibility deadline while its callback runs, so an item | ||||||||||
| * slower than the visibility timeout is not redelivered and executed a second time. | ||||||||||
| * | ||||||||||
| * `lostLease` reports that an extend found no in-flight entry, which means the item was | ||||||||||
| * reclaimed and is now back on the queue. It is a best-effort signal, not a fence: the | ||||||||||
| * in-flight member is keyed only by message and queue id, so once another consumer | ||||||||||
| * re-claims the item the member exists again and an extend from this consumer succeeds. | ||||||||||
| * Distinguishing owners would need a per-claim token in the member. | ||||||||||
| */ | ||||||||||
| #startHeartbeat( | ||||||||||
| messageId: string, | ||||||||||
| queueId: string | ||||||||||
| ): { stop: () => void; lostLease: () => boolean } { | ||||||||||
| let lostLease = false; | ||||||||||
|
|
||||||||||
| const interval = setInterval(() => { | ||||||||||
| this.fairQueue | ||||||||||
| .heartbeatMessage(messageId, queueId) | ||||||||||
| .then((stillOwned) => { | ||||||||||
| if (!stillOwned) { | ||||||||||
| lostLease = true; | ||||||||||
| } | ||||||||||
| }) | ||||||||||
| .catch((error) => { | ||||||||||
| this.logger.debug("Batch item heartbeat failed", { | ||||||||||
| messageId, | ||||||||||
| queueId, | ||||||||||
| error: error instanceof Error ? error.message : String(error), | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
| }, this.heartbeatIntervalMs); | ||||||||||
|
|
||||||||||
| interval.unref?.(); | ||||||||||
|
|
||||||||||
| return { stop: () => clearInterval(interval), lostLease: () => lostLease }; | ||||||||||
|
Comment on lines
+776
to
+798
Contributor
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. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Wait for active heartbeats and handle lease loss for callback errors.
📍 Affects 1 file
Comment on lines
+779
to
+798
Contributor
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. 🟡 A batch item whose processing hangs forever now blocks its batch from ever finishing The item is kept permanently invisible by an unbounded keep-alive timer ( Impact: If a single item's processing stalls indefinitely (e.g. a hung network/database call), the whole batch stays stuck forever instead of recovering by retrying that item. Why the safety net disappeared: extension has no upper boundBefore this change, a claimed message had a fixed 60s lease; if the consumer stopped making progress the reclaim loop ( Now A common mitigation is capping the total heartbeat lifetime (e.g. stop extending after N × visibility timeout, or after a configured max processing duration) so a genuinely stuck consumer still yields the item. Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||
| } | ||||||||||
|
|
||||||||||
| async #handleMessage(consumerId: string, messageId: string, queueId: string): Promise<void> { | ||||||||||
| // Get message data from FairQueue's in-flight storage | ||||||||||
| const storedMessage = await this.fairQueue.getMessageData(messageId, queueId); | ||||||||||
|
|
@@ -820,9 +866,10 @@ export class BatchQueue { | |||||||||
| let processedCount: number; | ||||||||||
|
|
||||||||||
| try { | ||||||||||
| const result = await this.#startSpan( | ||||||||||
| "BatchQueue.processItemCallback", | ||||||||||
| async (innerSpan) => { | ||||||||||
| const heartbeat = this.#startHeartbeat(messageId, queueId); | ||||||||||
| let result: Awaited<ReturnType<ProcessBatchItemCallback>>; | ||||||||||
| try { | ||||||||||
| result = await this.#startSpan("BatchQueue.processItemCallback", async (innerSpan) => { | ||||||||||
| innerSpan?.setAttributes({ | ||||||||||
| "batch.id": batchId, | ||||||||||
| "batch.itemIndex": itemIndex, | ||||||||||
|
|
@@ -837,8 +884,20 @@ export class BatchQueue { | |||||||||
| attempt, | ||||||||||
| isFinalAttempt, | ||||||||||
| }); | ||||||||||
| } | ||||||||||
| ); | ||||||||||
| }); | ||||||||||
| } finally { | ||||||||||
| heartbeat.stop(); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| if (heartbeat.lostLease()) { | ||||||||||
| this.logger.warn("Discarding batch item result, another consumer now owns it", { | ||||||||||
| batchId, | ||||||||||
| itemIndex, | ||||||||||
| messageId, | ||||||||||
| attempt, | ||||||||||
| }); | ||||||||||
| return; | ||||||||||
| } | ||||||||||
|
Comment on lines
+892
to
+900
Contributor
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. 🟡 An item taken over by another worker can still be marked permanently failed by the old worker When processing throws, the code records a permanent failure and re-queues the item ( Impact: A batch can report an item as failed even though a run for it was created successfully, and the created run is missing from the batch's results. Asymmetric ownership check between the result path and the throw pathAfter the callback resolves, the code checks
Applying the same Prompt for agentsWas this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||
|
|
||||||||||
| if (result.success) { | ||||||||||
| span?.setAttribute("batch.result", "success"); | ||||||||||
|
|
||||||||||
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.
🔍 No .server-changes entry for a user-visible batch fix
CONTRIBUTING.md / AGENTS.md ask for a
.server-changes/file for server-only PRs (apps/webapp/,apps/supervisor/, "etc."). This PR touches onlyinternal-packages/run-engine, which the table does not name explicitly, and repo history is mixed for run-engine-only PRs (some include one, some do not). Given the fix is user-visible (duplicate runs / batches that never finalize), a.server-changes/note witharea: webapp,type: fixwould likely be expected here.Was this helpful? React with 👍 or 👎 to provide feedback.