Skip to content

Conversation

ericallam
Copy link
Member

No description provided.

Copy link

changeset-bot bot commented Sep 26, 2025

⚠️ No Changeset found

Latest commit: b158668

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Sep 26, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

  • DequeueSystem.dequeueFromWorkerQueue now emits an info log when a message is dequeued, including runId, orgId, environmentId, environmentType, workerQueueLength, workerQueue, and the message context, before span attributes are set.
  • run-queue replaced per-message workerQueueKeys set tracking with an operations array containing { workerQueueKey, messageId } for each message and switched logged payload to operations.
  • Span attributes changed from worker_queue_count/worker_queue_keys to operations_count.
  • RunEngine.#handleStalledSnapshot now logs a stall event with runId and latest snapshotId in the PENDING_EXECUTING branch before requeueing.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is completely missing, so it does not follow the required template or include any sections like issue closure, checklist, testing steps, changelog, or screenshots. Please add a description following the repository template by specifying the issue closed, checklist items, testing steps, a concise changelog entry, and any relevant screenshots.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly and concisely describes the key change of adding logging around dequeue operations and worker queues in the run-engine codebase, matching the actual modifications without unnecessary detail or noise.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7e247c and b158668.

📒 Files selected for processing (3)
  • internal-packages/run-engine/src/engine/index.ts (1 hunks)
  • internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1 hunks)
  • internal-packages/run-engine/src/run-queue/index.ts (1 hunks)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
internal-packages/run-engine/src/run-queue/index.ts (1)

1372-1394: Add a concrete type to operations.

Declaring operations as a bare array makes it any[], which erodes type safety in this hot path. Tighten the typing so future refactors keep the expected shape.

Apply this diff:

-      const operations = [];
+      const operations: Array<{ workerQueueKey: string; messageId: string }> = [];
 
       for (const message of messages) {
         const workerQueueKey = this.keys.workerQueueKey(
           this.#getWorkerQueueFromMessage(message.message)
         );
 
         const messageKeyValue = this.keys.messageKey(message.message.orgId, message.messageId);
 
         operations.push({
-          workerQueueKey: workerQueueKey,
+          workerQueueKey,
           messageId: message.messageId,
         });
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eb0263e and b7e247c.

📒 Files selected for processing (2)
  • internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1 hunks)
  • internal-packages/run-engine/src/run-queue/index.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations

Files:

  • internal-packages/run-engine/src/engine/systems/dequeueSystem.ts
  • internal-packages/run-engine/src/run-queue/index.ts
🧬 Code graph analysis (2)
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (2)
internal-packages/run-engine/src/run-queue/index.ts (5)
  • message (1400-1448)
  • message (1697-1752)
  • message (1754-1802)
  • message (1804-1832)
  • message (1853-1855)
packages/core/src/v3/logger/taskLogger.ts (1)
  • message (75-101)
internal-packages/run-engine/src/run-queue/index.ts (1)
internal-packages/run-engine/src/run-queue/keyProducer.ts (1)
  • workerQueueKey (41-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
  • GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
  • GitHub Check: typecheck / typecheck
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
  • GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
  • GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
  • GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts (1)

146-153: Additional logging improves dequeue visibility

The added info-level log captures the key identifiers we need when correlating queue activity, and it lands before the span attributes are set, which helps during live debugging. 👍

@ericallam ericallam merged commit 743b8db into main Sep 26, 2025
4 of 5 checks passed
@ericallam ericallam deleted the ea-branch-94 branch September 26, 2025 10:50
samejr pushed a commit that referenced this pull request Oct 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants