chore(runner): move max duration logic into parent process #2637
Conversation
🦋 Changeset detectedLatest commit: ed87c1e The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughMax-duration handling was moved out of individual timeout implementations into a parent-level flow. A new exported MaxDurationExceededError class was added. TimeoutManager gained an optional registerListener API and UsageTimeoutManager added listener support and reports elapsed time. Two worker entry points (dev-run-worker, managed-run-worker) register listeners that log and send a new MAX_DURATION_EXCEEDED IPC message (with maxDurationInSeconds and elapsedTimeInSeconds). TaskRunProcess now handles MAX_DURATION_EXCEEDED by marking state, rejecting pending promises with MaxDurationExceededError, and mapping it to an INTERNAL_ERROR with code MAX_DURATION_EXCEEDED. A changeset and a package.json type field removal were also included; one test was removed. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli-v3/src/executions/taskRunProcess.ts (1)
258-261: Reset max-duration flags at the start of each execute cycle.Without resetting, a prior overrun can misclassify the next exit.
Apply:
this._isBeingCancelled = false; this._isPreparedForNextRun = false; this._isPreparedForNextAttempt = false; + this._isMaxDurationExceeded = false; + this._maxDurationInfo = undefined;
🧹 Nitpick comments (3)
packages/core/src/v3/timeout/api.ts (1)
50-55: Consider documenting the listener behavior.The implementation correctly delegates to the manager if supported. However, it's unclear whether:
- Multiple listeners can be registered or if subsequent calls overwrite
- What happens if a listener throws an error
- Whether the listener is called before or after the abort signal fires
Consider adding JSDoc:
+ /** + * Register a listener to be notified when max duration is exceeded. + * Note: Currently only one listener can be registered per manager. + * Subsequent calls will replace the previous listener. + * @param listener Callback invoked when timeout occurs + */ public registerListener(listener: (timeoutInSeconds: number, elapsedTimeInSeconds: number) => void | Promise<void>) {packages/core/src/v3/timeout/usageTimeoutManager.ts (1)
55-56: Naming consistency: usage vs elapsed.Elsewhere the timeout error uses “usageInSeconds”. Here you compute
elapsedTimeInSeconds. Align naming to avoid confusion when wiring error telemetry.Apply:
- const elapsedTimeInSeconds = sample.cpuTime / 1000; + const usageInSeconds = sample.cpuTime / 1000; @@ - this._listener(timeoutInSeconds, elapsedTimeInSeconds); + this._listener(timeoutInSeconds, usageInSeconds); @@ - new TaskRunExceededMaxDuration(timeoutInSeconds, elapsedTimeInSeconds) + new TaskRunExceededMaxDuration(timeoutInSeconds, usageInSeconds)packages/cli-v3/src/executions/taskRunProcess.ts (1)
215-231: Make MAX_DURATION_EXCEEDED handling idempotent.Duplicate messages could race with termination; short-circuit if we already processed one.
Apply:
- MAX_DURATION_EXCEEDED: async (message) => { + MAX_DURATION_EXCEEDED: async (message) => { + if (this._isMaxDurationExceeded) return; logger.debug("max duration exceeded, gracefully terminating child process", {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.changeset/big-ants-act.md(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(1 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(1 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(5 hunks)packages/core/src/v3/errors.ts(1 hunks)packages/core/src/v3/schemas/messages.ts(1 hunks)packages/core/src/v3/timeout/api.ts(1 hunks)packages/core/src/v3/timeout/types.ts(1 hunks)packages/core/src/v3/timeout/usageTimeoutManager.ts(2 hunks)packages/core/src/v3/workers/taskExecutor.ts(1 hunks)packages/rsc/src/package.json(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rsc/src/package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/core/src/v3/timeout/usageTimeoutManager.tspackages/core/src/v3/errors.tspackages/core/src/v3/workers/taskExecutor.tspackages/core/src/v3/timeout/types.tspackages/core/src/v3/schemas/messages.tspackages/cli-v3/src/entryPoints/dev-run-worker.tspackages/core/src/v3/timeout/api.tspackages/cli-v3/src/entryPoints/managed-run-worker.tspackages/cli-v3/src/executions/taskRunProcess.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/v3/timeout/usageTimeoutManager.tspackages/core/src/v3/errors.tspackages/core/src/v3/workers/taskExecutor.tspackages/core/src/v3/timeout/types.tspackages/core/src/v3/schemas/messages.tspackages/core/src/v3/timeout/api.ts
🧬 Code graph analysis (3)
packages/core/src/v3/timeout/usageTimeoutManager.ts (2)
packages/core/src/v3/usage/devUsageManager.ts (2)
sample(18-37)sample(75-77)packages/core/src/v3/timeout/types.ts (1)
TaskRunExceededMaxDuration(8-16)
packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
packages/core/src/v3/timeout-api.ts (1)
timeout(5-5)
packages/cli-v3/src/executions/taskRunProcess.ts (2)
packages/core/src/v3/errors.ts (2)
UnexpectedExitError(508-518)MaxDurationExceededError(560-569)packages/core/src/v3/schemas/common.ts (2)
TaskRunErrorCodes(197-197)TaskRunErrorCodes(198-198)
⏰ 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 / packages / 📊 Merge Reports
- 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 (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (9)
packages/core/src/v3/errors.ts (2)
560-569: LGTM! Well-structured error class for max duration violations.The new
MaxDurationExceededErrorfollows established patterns and provides clear context with readonly properties for both the configured limit and actual elapsed time. The descriptive message will help with debugging.
299-299: MAX_DURATION_EXCEEDED is correctly marked as non-retryable and error handling respects this classification.Verification confirms the error code returns
falseinshouldRetryError(packages/core/src/v3/errors.ts, line 321), grouped with other terminal error conditions. Error handling intaskRunProcess.tsperforms graceful process termination via IPC message, with no retry logic attempting to circumvent this classification. Parent process termination is clean and does not inadvertently trigger retries.packages/core/src/v3/workers/taskExecutor.ts (1)
425-427: Clear explanation of the architectural change.The comments effectively document that max duration enforcement has moved from a Promise.race pattern to parent process termination. The signal remains available for other abort conditions (cancellation, etc.), which maintains flexibility.
.changeset/big-ants-act.md (1)
1-6: LGTM! Changeset correctly configured.The patch release and changelog message appropriately describe this internal architectural improvement.
packages/core/src/v3/timeout/types.ts (1)
5-5: LGTM! Optional method maintains backward compatibility.The optional
registerListenermethod allows existingTimeoutManagerimplementations to work without modification while enabling new functionality where needed.packages/cli-v3/src/entryPoints/dev-run-worker.ts (1)
131-140: The implementations are not identical—dev-run-worker useslog()while managed-run-worker usesconsole.log()directly.The review comment incorrectly asserts these implementations match. While the IPC message structure is identical, dev-run-worker correctly uses the
log()helper (more isomorphic), whereas managed-run-worker uses Node.js-specificconsole.log(). Per coding guidelines, dev-run-worker's approach is preferable. The review's approval is based on a false consistency claim.Likely an incorrect or invalid review comment.
packages/core/src/v3/schemas/messages.ts (1)
192-198: IPC message flow is complete and properly implemented.The MAX_DURATION_EXCEEDED message is correctly defined in the schema, sent from both worker entry points with matching fields, received and stored in TaskRunProcess, and properly instantiated as MaxDurationExceededError with duration context in the exit handler.
packages/cli-v3/src/executions/taskRunProcess.ts (2)
518-524: Review comment is based on an incorrect assumption about schema support.The TaskRunInternalError schema does not support a metadata field. The schema at
packages/core/src/v3/schemas/common.ts:153-193defines only:type,code,message, andstackTrace. Since the conditional premise ("only if TaskRunInternalError supports a metadata object") is false, the suggested refactor cannot be implemented without first modifying the schema definition itself.Likely an incorrect or invalid review comment.
342-360: Exit precedence verified—max duration takes priority.The exit path at lines 342–369 correctly checks
_isMaxDurationExceededfirst, ensuring max duration errors take precedence over cancellation, graceful timeouts, and kill signals. No conflicting flag assignments detected after_isMaxDurationExceededis set.
Max duration also gets priority over all other errors. Hitting this limit will kill the child, i.e. run, process.