Exit the process if status is finished and not in the middle of completing #2378
Conversation
…eting This solves an issue where a run is system failed from the platform and the worker is never shut down in the cluster.
WalkthroughA private boolean flag named Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/cli-v3/src/entryPoints/managed/execution.ts (2)
297-304: Clarify FINISHED handling message and shutdown reasonLogic is correct. Two nits to improve clarity:
- Update the comment/log to explicitly say we’re skipping shutdown while a completion submission is in-flight.
- Use a more accurate shutdown reason than "re-queued" for FINISHED.
Apply:
- // We are finishing the run in handleCompletionResult, so we don't need to do anything here - if (this.isCompletingRun) { - this.sendDebugLog("run is finished but we're completing it, skipping", snapshotMetadata); - return; - } - - await this.exitTaskRunProcessWithoutFailingRun({ flush: true, reason: "re-queued" }); + // If a completion submission is in-flight, skip shutdown here and let the completion flow clean up. + if (this.isCompletingRun) { + this.sendDebugLog( + "run is finished but completion is in-flight, skipping shutdown", + snapshotMetadata + ); + return; + } + + await this.exitTaskRunProcessWithoutFailingRun({ + flush: true, + reason: "finished (not completing)", + });
669-670: ScopeisCompletingRunstrictly to the completion submission windowCurrent behavior keeps the flag true after completion. Not harmful (execute() shuts down), but you can make the intent tighter by resetting it in a finally block so it only denotes “in-flight completion”.
Apply:
- this.isCompletingRun = true; - - const completionResult = await this.httpClient.completeRunAttempt( - this.runFriendlyId, - this.snapshotManager.snapshotId, - { completion } - ); - - if (!completionResult.success) { - throw new Error(`failed to submit completion: ${completionResult.error}`); - } - - await this.handleCompletionResult({ - completion, - result: completionResult.data.result, - }); + this.isCompletingRun = true; + try { + const completionResult = await this.httpClient.completeRunAttempt( + this.runFriendlyId, + this.snapshotManager.snapshotId, + { completion } + ); + + if (!completionResult.success) { + throw new Error(`failed to submit completion: ${completionResult.error}`); + } + + await this.handleCompletionResult({ + completion, + result: completionResult.data.result, + }); + } finally { + // Reflect that completion submission has finished (success or failure) + this.isCompletingRun = false; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/cli-v3/src/entryPoints/managed/execution.ts(4 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:
packages/cli-v3/src/entryPoints/managed/execution.ts
⏰ 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). (23)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 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 (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
packages/cli-v3/src/entryPoints/managed/execution.ts (2)
79-80: Good state flag to gate shutdown while submitting completionThe
isCompletingRunflag is a simple and effective guard for avoiding premature shutdown during completion.
388-390: Resetting the flag at the start of each attempt is correctThis ensures retries start clean and FINISHED events are handled appropriately for the new attempt.
This solves an issue where a run is system failed from the platform and
the worker is never shut down in the cluster.