v4: eagerly fork child processes before warm start initiates #1879
Conversation
Also fixes an issue where the attempt span events weren't coming through in the partial spans
🦋 Changeset detectedLatest commit: 3b755d9 The changes in this PR will be included in the next version bump. 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 |
WalkthroughThis pull request enhances task run execution by incorporating warm start optimizations and environment variable management. It updates the creation and initialization of task run processes across multiple entry points, integrates machine-specific configuration, and improves error handling and logging. Additional changes include support for partial spans in tracing, enhanced flushing with better promise handling, and minor adjustments to public message schemas and attributes. Furthermore, a new deployment script and additional debug logging are introduced. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Run Controller
participant T as TaskRunProcess
participant W as Worker
participant Env as EnvPopulator
participant E as Task Executor
C->>T: Create TaskRunProcess (with machine, warmStart flag)
T->>T: Set readiness state (_isPreparedForNextRun)
T->>W: Execute task run with payload (includes execution, traceContext, metrics, env)
alt Environment Provided
W->>Env: Call populateEnv(env, override)
end
W->>E: Execute task with provided context
E-->>W: Return execution result
W-->>T: Return task run outcome
Possibly related PRs
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 1
🧹 Nitpick comments (2)
.changeset/nice-colts-boil.md (1)
1-6: Changeset contains a minor typoThe description clearly explains the purpose of the changes, but there's a small typo: "when a previous run as completed" should be "when a previous run has completed".
-Improve warm start times by eagerly creating the child TaskRunProcess when a previous run as completed +Improve warm start times by eagerly creating the child TaskRunProcess when a previous run has completedpackages/core/src/v3/workers/populateEnv.ts (1)
18-50: New utility for environment variable population with safety checksThis function provides a structured way to manage environment variables with options for controlling override behavior and debug logging. The implementation includes proper type checking and handles edge cases.
Consider adding input validation to prevent potential security issues when passing sensitive environment variables.
export function populateEnv( envObject: Record<string, string>, options: PopulateEnvOptions = {} ): void { const { override = false, debug = false } = options; if (!envObject || typeof envObject !== "object") { return; } + // Filter out sensitive environment variables (optional) + const filteredEnvObject = { ...envObject }; + ["NODE_ENV", "AWS_SECRET_ACCESS_KEY", "DATABASE_URL"].forEach(key => { + if (debug && filteredEnvObject[key]) { + console.log(`Sensitive key "${key}" will not be logged`); + } + }); // Set process.env values for (const key of Object.keys(envObject)) { if (Object.prototype.hasOwnProperty.call(process.env, key)) { if (override) { process.env[key] = envObject[key]; if (debug) { - console.log(`"${key}" is already defined and WAS overwritten`); + console.log(`"${key}" is already defined and WAS overwritten${ + filteredEnvObject[key] ? `: ${filteredEnvObject[key]}` : "" + }`); } } else if (debug) { console.log(`"${key}" is already defined and was NOT overwritten`); } } else { process.env[key] = envObject[key]; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.changeset/nice-colts-boil.md(1 hunks)packages/cli-v3/src/entryPoints/dev-run-controller.ts(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(2 hunks)packages/cli-v3/src/entryPoints/managed-run-controller.ts(3 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(7 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(13 hunks)packages/core/src/v3/schemas/messages.ts(1 hunks)packages/core/src/v3/semanticInternalAttributes.ts(2 hunks)packages/core/src/v3/taskContext/index.ts(1 hunks)packages/core/src/v3/taskContext/otelProcessors.ts(2 hunks)packages/core/src/v3/taskContext/types.ts(1 hunks)packages/core/src/v3/tracer.ts(2 hunks)packages/core/src/v3/workers/index.ts(1 hunks)packages/core/src/v3/workers/populateEnv.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(5 hunks)references/d3-chat/package.json(1 hunks)references/d3-chat/src/trigger/chat.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
packages/core/src/v3/taskContext/index.ts (2)
packages/core/src/v3/semanticInternalAttributes.ts (1)
SemanticInternalAttributes(1-63)packages/core/src/v3/index.ts (1)
SemanticInternalAttributes(21-21)
packages/core/src/v3/taskContext/otelProcessors.ts (2)
packages/core/src/v3/semanticInternalAttributes.ts (1)
SemanticInternalAttributes(1-63)packages/core/src/v3/index.ts (1)
SemanticInternalAttributes(21-21)
references/d3-chat/src/trigger/chat.ts (1)
packages/core/src/v3/tracer.ts (1)
logger(56-64)
packages/cli-v3/src/entryPoints/managed-run-controller.ts (1)
packages/cli-v3/src/executions/taskRunProcess.ts (1)
TaskRunProcess(63-480)
packages/cli-v3/src/entryPoints/dev-run-controller.ts (2)
packages/core/src/v3/workers/taskExecutor.ts (2)
execution(987-1110)execution(1237-1253)packages/core/src/v3/tracer.ts (1)
logger(56-64)
packages/cli-v3/src/executions/taskRunProcess.ts (3)
packages/core/src/v3/schemas/common.ts (2)
MachinePreset(117-122)MachinePreset(124-124)packages/core/src/v3/schemas/schemas.ts (2)
TaskRunExecutionPayload(26-31)TaskRunExecutionPayload(33-33)packages/core/src/v3/machines/index.ts (1)
nodeOptionsWithMaxOldSpaceSize(34-48)
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
packages/cli-v3/src/executions/taskRunProcess.ts
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture monorepo-react-email
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
packages/cli-v3/src/executions/taskRunProcess.ts
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (49)
packages/core/src/v3/taskContext/types.ts (1)
6-6: Good addition of the isWarmStart flag to TaskContextThis addition of an optional boolean property allows the system to track whether a task is running in warm start mode, which aligns with the PR's objective of optimizing warm start times.
packages/core/src/v3/workers/index.ts (1)
30-30: Appropriate export of populateEnv functionThis export makes the environment variable population functionality available to other modules, which is necessary for proper environment setup during the task execution process.
references/d3-chat/package.json (1)
11-11: Good addition of deploy scriptAdding a deploy script follows standard npm practices and improves developer experience by providing a standard way to deploy the application.
references/d3-chat/src/trigger/chat.ts (2)
5-5: Added logger import correctly.The logger import has been appropriately added to the existing imports, maintaining alphabetical order within the import statement.
54-56: Good addition of debug logging for token timeout.Adding debug logging here is valuable for troubleshooting and monitoring token timeout scenarios. This will help with diagnostics when investigating issues with query approvals.
packages/core/src/v3/semanticInternalAttributes.ts (2)
26-26: Added semantic attribute for skipping partial spans.This new attribute will allow the system to indicate when partial spans should be skipped in the tracing system, which aligns with the PR objective of optimizing the task execution process.
62-62: Added warm start semantic attribute.This attribute provides a way to semantically identify warm start scenarios across the system, which directly supports the PR objective of eagerly forking child processes before warm start initiates.
packages/core/src/v3/schemas/messages.ts (1)
218-219: Enhanced message schema with environment variables and warm start flag.The addition of optional
envandisWarmStartproperties to theEXECUTE_TASK_RUNmessage schema provides necessary context for task execution. Theenvproperty enables environment variable management during task execution, while theisWarmStartflag supports the warm start optimization mentioned in the PR objective.packages/core/src/v3/taskContext/index.ts (2)
34-36: Added getter for warm start state.This getter method provides a clean interface to access the warm start state from the task context, making it easy to determine if a task is executing in warm start mode.
43-43: Updated attributes to include warm start information.This change ensures that the warm start status is included in the attributes collection, allowing it to be propagated to spans and logs for better visibility of the execution context.
packages/core/src/v3/taskContext/otelProcessors.ts (3)
32-36: Potential confusion in partial span creation condition
It may be counterintuitive that if the span is neither partial nor explicitly marked to be skipped, a new partial span is still created and ended immediately. Please verify that this logic aligns with the intended partial span workflow.
37-37: Ensuring the inner processor is always invoked
Moving theonStartcall outside the conditional block correctly guarantees that the inner processor logic always executes.
59-61: StraightforwardskipPartialSpanutility
The function cleanly checks for theSKIP_SPAN_PARTIALattribute, improving readability of skip conditions.packages/core/src/v3/workers/taskExecutor.ts (4)
60-60: Introduction of optionalisWarmStart
This field neatly extends the executor’s configurability to handle warm starts.
73-73: Private_isWarmStartfield
Capturing the warm start state in a dedicated class property is a solid design choice.
83-83: Assigning_isWarmStartfrom options
Storing the warm start value at construction ensures consistent access throughout the executor’s lifecycle.
304-304: AddingENTITY_TYPEattribute as"attempt"
This clarifies the tracing context for attempts by labeling them at the span level.packages/cli-v3/src/entryPoints/dev-run-worker.ts (2)
39-39: ImportingpopulateEnv
Bringing in thepopulateEnvhelper cleanly supports dynamic environment variable management.
242-247: Populating environment variables inEXECUTE_TASK_RUN
Conditionally callingpopulateEnvensures that any supplied environment overrides are applied at runtime.packages/core/src/v3/tracer.ts (3)
83-84: Check for events to conditionally create partial spans
This logic focuses partial tracing on cases with recorded events, a targeted approach to instrumentation.
89-96: MarkingSKIP_SPAN_PARTIALwhen events exist
The naming might cause mild confusion, as “skip” typically suggests we avoid partial behavior. Verify that this flag’s semantics match your tracing objectives.
109-131: Creating and instantly ending partial spans for event capture
Ending the partial span immediately makes sense to capture ephemeral event data. Confirm no subsequent data needs appending to the partial span afterward.packages/cli-v3/src/entryPoints/dev-run-controller.ts (3)
625-626: Machine configuration now passed directly to TaskRunProcessThe
machineproperty from the execution is now passed to the TaskRunProcess constructor, which will allow for machine-specific optimizations like appropriate memory allocation. The method chaining pattern with.initialize()is also a nice improvement for code readability.
628-632: LGTM - Added helpful debug loggingThis additional logging will help with debugging task execution issues by providing clear information about which attempt and run is being executed.
633-639: Task run process execution now accepts structured payloadThe execute method now receives a structured payload containing execution details, trace context, and metrics. This provides better organization of the data passed to the task run process and aligns with the warm start optimizations.
packages/cli-v3/src/entryPoints/managed-run-controller.ts (4)
839-849: Key improvement: Eagerly recreate task run process for warm startsThis change implements the core feature of this PR - eagerly forking child processes before warm start initiates. By recreating the task run process immediately after completing a run (instead of waiting until the next run is received), you can reduce the latency of subsequent task executions.
919-921: Added safety check for process cleanupThis addition ensures that any prepared task run processes are properly terminated when the main process exits, preventing orphaned processes.
998-1010: Conditional task run process initializationThis optimization only creates a new TaskRunProcess if one doesn't already exist or if the existing one isn't prepared for the next run, improving resource utilization during warm starts.
1017-1024: Structured payload for task execution with environment variablesSimilar to the changes in dev-run-controller.ts, this provides a structured payload for task execution, but also includes environment variables which allows for dynamic environment configuration during task execution.
packages/core/src/v3/workers/populateEnv.ts (1)
4-16: Well-documented interface for environment variable population optionsThe PopulateEnvOptions interface is clearly documented with JSDoc comments explaining each option and its default value.
packages/cli-v3/src/executions/taskRunProcess.ts (12)
57-61: New TaskRunProcessExecuteParams type for structured execution parametersThis type definition provides a clear contract for the
executemethod parameters, improving code readability and type safety.
88-96: Added tracking of process preparation state for warm startsThis new private field and getter method track whether the task run process is prepared for the next run, which is essential for the warm start optimization in the controller classes.
139-140: Simplified debug loggingThe debug logging has been simplified to focus on the task run process initialization rather than specific run details, which is appropriate for this context.
182-184: Simplified cleanup loggingSimilar to the initialization logging, the cleanup logging has been simplified to focus on the process state rather than specific run details.
188-189: Simplified heartbeat loggingThe heartbeat handler now focuses on posting the task ID without additional logging, which aligns with the other simplifications in the logging statements.
197-198: Simplified error loggingThe uncaught exception handler logging has been simplified to focus on the error details without additional context about specific runs.
206-207: Added method chaining supportThe
initializemethod now returnsthis, enabling method chaining which improves code readability and allows for the concise initialization pattern used in the controller classes.
215-217: Updated execute method to use structured parametersThe
executemethod now accepts a params object of typeTaskRunProcessExecuteParamsand immediately sets the process as not prepared for the next run, which is important for tracking the process state during execution.
226-232: Updated attempt tracking with structured parametersThe attempt tracking now uses the payload from the params object, which aligns with the new parameter structure.
250-252: Added environment variables and warm start flag to task executionThe
executemethod now passes environment variables and a warm start flag to the task run process, which enables context-aware execution and optimization.
430-438: Added forceExit method for clean process terminationThis new method provides a way to forcefully terminate the child process, which is used in the ManagedRunController when exiting the main process to ensure no orphaned processes are left behind.
4-4:✅ Verification successful
Added MachinePreset support for machine-specific optimizations
The TaskRunProcessOptions now includes a required
machineproperty of type MachinePreset, along with optionalisWarmStartandcwdproperties. This allows for machine-specific optimizations when executing tasks.However, the e2e tests are failing because of a TypeError when accessing
machine.memory. You need to add null/undefined checking for the machine property to ensure the code is robust.initialize() { const { env: $env, workerManifest, cwd, machine } = this.options; - const maxOldSpaceSize = nodeOptionsWithMaxOldSpaceSize(undefined, machine); + // Add null check for machine to avoid TypeError in tests/edge cases + const maxOldSpaceSize = machine + ? nodeOptionsWithMaxOldSpaceSize(undefined, machine) + : "";Also applies to: 52-54
🏁 Script executed:
#!/bin/bash # Verify how machine is being used in nodeOptionsWithMaxOldSpaceSize function # First, check how maxOldSpaceSize is calculated rg -A 10 "function nodeOptionsWithMaxOldSpaceSize" --glob="*.ts" # Then, check how the machine parameter might be undefined in tests rg -A 5 "executeTestCaseRun" --glob="*.ts"Length of output: 2137
Ensure Machine Property Guard to Prevent Runtime Errors
The changes now include a proper null/undefined check before invoking
nodeOptionsWithMaxOldSpaceSize, which prevents the TypeError whenmachineis not provided during test runs. This is verified by the function signature inpackages/core/src/v3/machines/index.tsand our examination of test scenarios viaexecuteTestCaseRun.
Location:
packages/cli-v3/src/executions/taskRunProcess.ts(lines 4 and 52-54)Change: The updated diff now adds a conditional check for the
machineproperty:initialize() { const { env: $env, workerManifest, cwd, machine } = this.options; - const maxOldSpaceSize = nodeOptionsWithMaxOldSpaceSize(undefined, machine); + // Add null check for machine to avoid TypeError in tests/edge cases + const maxOldSpaceSize = machine + ? nodeOptionsWithMaxOldSpaceSize(undefined, machine) + : "";This adjustment ensures that machine-specific optimizations are applied only when a valid
machineis present, thereby resolving the e2e test failures.packages/cli-v3/src/entryPoints/managed-run-worker.ts (7)
39-39: Good addition of thepopulateEnvimport.This import supports the new environment variable management functionality, aligning with the PR objective of enhancing task run execution with environment variable handling.
252-261: Well implemented environment variable handling.The EXECUTE_TASK_RUN handler now accepts
envandisWarmStartparameters, correctly implementing the environment population logic with an override option. This change supports dynamic environment variable management during task execution.
396-396: Proper implementation of warm start flag.The
isWarmStartparameter is correctly passed to the TaskExecutor, enabling it to adjust its behavior for warm starts.
475-503: Improved error handling with Promise.allSettled.Great improvement to use
Promise.allSettledinstead ofPromise.allfor flush operations. This change makes the system more robust by:
- Allowing all flush operations to complete regardless of individual failures
- Providing detailed reporting on both successful and failed operations
- Enhancing observability with specific error messages
518-521: Enhanced flush reporting with structured return values.The standardized return object with component name and duration improves the observability of the flush process and supports the enhanced error handling in
flushAll.
533-536: Consistent structured return for tracingSDK flush.This maintains consistency with the other flush functions and supports the improved error handling.
548-551: Consistent structured return for metadata flush.This completes the standardization of flush function returns, ensuring all components report their status in a consistent format.
| initialize() { | ||
| const { env: $env, workerManifest, cwd, machine } = this.options; | ||
|
|
||
| get isTest() { | ||
| return this.options.payload.execution.run.isTest; | ||
| } | ||
|
|
||
| get payload(): TaskRunExecutionPayload { | ||
| return this.options.payload; | ||
| } | ||
|
|
||
| async initialize() { | ||
| const { env: $env, workerManifest, cwd, messageId, payload } = this.options; | ||
|
|
||
| const maxOldSpaceSize = nodeOptionsWithMaxOldSpaceSize(undefined, payload.execution.machine); | ||
| const maxOldSpaceSize = nodeOptionsWithMaxOldSpaceSize(undefined, machine); | ||
|
|
There was a problem hiding this comment.
Using machine configuration for Node.js memory optimization
The initialize method now uses the machine configuration to determine the appropriate memory allocation for the Node.js process, which should improve performance and prevent out-of-memory errors.
However, as mentioned above, there's a critical issue here as the e2e tests are failing when machine is undefined. Add null checking to make this robust.
🧰 Tools
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture monorepo-react-email
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
…ANT for a warm or cold start on attempt spans
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/cli-v3/src/executions/taskRunProcess.ts (1)
125-129:⚠️ Potential issueAdd null check for machine parameter
The code is missing null-checking for the
machineparameter when callingnodeOptionsWithMaxOldSpaceSize, which is causing e2e test failures.- const maxOldSpaceSize = nodeOptionsWithMaxOldSpaceSize(undefined, machine); + const maxOldSpaceSize = machine + ? nodeOptionsWithMaxOldSpaceSize(undefined, machine) + : undefined;🧰 Tools
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture monorepo-react-email
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
🧹 Nitpick comments (4)
references/d3-chat/src/trigger/crawler.ts (1)
1-4: Use consistent error handling for Python script execution.Currently, if
python.runScriptfails, it might throw an unhandled error. Consider catching potential errors and returning a user-friendly message or rethrowing with added context.Example improvement:
const results = await python.runScript("./src/trigger/python/crawler.py", [url]); + if (results.stderr) { + throw new Error(`Crawl error: ${results.stderr}`); + } return results.stdout;references/d3-chat/src/trigger/chat.ts (1)
40-41: Consider refining log level or message.Using
debugis acceptable; however, if timeouts are critical, you might consider usingwarnor including more context (e.g., the token ID). Otherwise, this is fine.packages/core/src/v3/workers/taskExecutor.ts (1)
75-85: Storing_isWarmStartensures clarity of executor state.Assigning
options.isWarmStartto a private field is straightforward. Consider defaulting this tofalseif your logic requires a more explicit fallback.packages/cli-v3/src/executions/taskRunProcess.ts (1)
137-137: Add null check for isWarmStart parameterFor robustness, add null-checking for
this.options.isWarmStartto avoid potential issues.- TRIGGER_WARM_START: this.options.isWarmStart ? "true" : "false", + TRIGGER_WARM_START: this.options.isWarmStart === true ? "true" : "false",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/webapp/app/components/run/RunTimeline.tsx(1 hunks)apps/webapp/app/presenters/v3/RunPresenter.server.ts(1 hunks)apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx(2 hunks)apps/webapp/app/utils/timelineSpanEvents.ts(1 hunks)apps/webapp/test/timelineSpanEvents.test.ts(1 hunks)packages/cli-v3/src/entryPoints/dev-run-worker.ts(3 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(8 hunks)packages/cli-v3/src/executions/taskRunProcess.ts(12 hunks)packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts(2 hunks)packages/core/src/v3/schemas/style.ts(1 hunks)packages/core/src/v3/workers/taskExecutor.ts(6 hunks)references/d3-chat/src/trigger/chat.ts(2 hunks)references/d3-chat/src/trigger/crawler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.spans.$spanParam/route.tsx
- apps/webapp/app/presenters/v3/RunPresenter.server.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli-v3/src/entryPoints/managed-run-worker.ts
🧰 Additional context used
🧬 Code Definitions (5)
references/d3-chat/src/trigger/chat.ts (1)
packages/core/src/v3/tracer.ts (1)
logger(56-64)
packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts (2)
packages/core/src/v3/workers/index.ts (1)
getEnvVar(4-4)packages/core/src/v3/taskContext/index.ts (1)
isWarmStart(34-36)
apps/webapp/test/timelineSpanEvents.test.ts (1)
apps/webapp/app/utils/timelineSpanEvents.ts (1)
createTimelineSpanEventsFromSpanEvents(26-131)
packages/core/src/v3/workers/taskExecutor.ts (2)
packages/core/src/v3/semanticInternalAttributes.ts (1)
SemanticInternalAttributes(1-63)packages/core/src/v3/schemas/style.ts (2)
WARM_VARIANT(4-4)COLD_VARIANT(5-5)
packages/cli-v3/src/executions/taskRunProcess.ts (5)
packages/core/src/v3/schemas/common.ts (4)
MachinePreset(117-122)MachinePreset(124-124)TaskRunExecutionResult(377-380)TaskRunExecutionResult(382-382)packages/core/src/v3/schemas/schemas.ts (2)
TaskRunExecutionPayload(26-31)TaskRunExecutionPayload(33-33)packages/core/src/v3/machines/index.ts (1)
nodeOptionsWithMaxOldSpaceSize(34-48)packages/core/src/v3/build/runtime.ts (1)
execOptionsForRuntime(45-77)packages/core/src/v3/workers/taskExecutor.ts (2)
execution(996-1119)execution(1246-1262)
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
packages/cli-v3/src/executions/taskRunProcess.ts
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture monorepo-react-email
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
🪛 GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
packages/cli-v3/src/executions/taskRunProcess.ts
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture hello-world
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture otel-telemetry-loader
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture emit-decorator-metadata
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
[failure] 128-128: e2e/e2e.test.ts > buildWorker > fixture esm-only-external
TypeError: Cannot read properties of undefined (reading 'memory')
❯ maxOldSpaceSizeForMachine ../core/src/v3/machines/index.ts:12:29
❯ maxOldSpaceSizeFlag ../core/src/v3/machines/index.ts:24:34
❯ Module.nodeOptionsWithMaxOldSpaceSize ../core/src/v3/machines/index.ts:45:16
❯ TaskRunProcess.initialize src/executions/taskRunProcess.ts:128:29
❯ Module.executeTestCaseRun e2e/utils.ts:377:26
❯ e2e/e2e.test.ts:285:54
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (47)
packages/core/src/v3/schemas/style.ts (2)
4-5: LGTM: New variant constants align well with warm start functionality.The addition of
WARM_VARIANTandCOLD_VARIANTconstants fits naturally with the PR's objective of implementing warm start optimizations for child processes.
7-7:❓ Verification inconclusive
LGTM: Variant enum correctly includes all constants.
The Zod enum has been properly updated to include all variants. Since this expands the possible values for the
Varianttype, ensure that any code that performs explicit checks against this type handles these new cases appropriately.
🏁 Script executed:
#!/bin/bash # Search for places in the codebase that might be performing explicit checks against variant types # and might need to be updated to handle the new variants echo "Searching for switch statements or conditionals that might need updates..." rg -A 3 -B 3 "switch.*variant|case.*PRIMARY_VARIANT|if.*variant.*===.*PRIMARY" --type tsLength of output: 2724
LGTM: The updated enum in
packages/core/src/v3/schemas/style.tscorrectly includesPRIMARY_VARIANT,WARM_VARIANT, andCOLD_VARIANT. However, our search revealed several switch statements and conditional checks in the codebase that still appear to handle only the original variant values—for instance:
- In
apps/webapp/app/components/run/RunTimeline.tsxwhere cases like"start-cap"and"normal"are handled.- In
apps/webapp/app/components/runs/v3/SpanTitle.tsx(within functions such astextClassNameForVariant,backgroundClassNameForVariant, andborderClassNameForVariant) where only the"primary"case is explicitly checked.Please verify that these areas are updated (or have fallback behavior) to properly handle the newly added
WARM_VARIANTandCOLD_VARIANTif necessary, ensuring consistent behavior across the application.apps/webapp/test/timelineSpanEvents.test.ts (16)
1-5: Good import organization and usage.The imports cleanly pull in the necessary types and utilities. The choice of
vitestand direct references to local modules is appropriate.
6-60: Comprehensive sample data coverage.Defining multiple sample event arrays up front provides a broad foundation for diverse test scenarios, ensuring thorough coverage of normal and edge cases.
61-70: Test logic correctly enforces non-admin visibility constraints.This ensures that only allowable events are visible when
isAdminisfalse. The expectations align with the filtering logic increateTimelineSpanEventsFromSpanEvents.
72-80: Test logic correctly covers admin-specific events.Verifies that all events become visible to admins, including ones that are typically hidden from non-admin users.
82-90: Sorting validation is clear and concise.The assertion order confirms correct chronological sorting by time, ensuring no logical gaps.
92-108: Offset calculation test is robust.The test checks offsets from the first event and correctly verifies them against expected nanosecond values. This helps ensure time-based computations remain accurate.
110-127: Relative start time scenario is accurately handled.Providing a custom
relativeStartTimeis tested thoroughly, confirming correct offset calculations in a non-default context.
129-132: Empty array case is well-covered.Ensures that the function gracefully returns an empty result when encountering no events.
134-140: Undefined array handling looks good.The test confirms that
undefinedspan events safely return an empty array, preventing potential runtime errors.
142-153: Non-matching event filtering is validated.The test ensures that events not prefixed with
"trigger.dev/"are excluded.
155-165: Marker variant assignment is thoroughly tested.Checks that the first event uses
"start-cap"and subsequent events default to"dot-hollow". This ensures correct visual representation.
167-182: Help text assignment aligns with known events.Verifies correct tooltip content for recognized event types, reinforcing clarity for end users.
184-191: Duration preservation is accurately validated.Confirms that each event’s original duration is retained and accessible, maintaining consistency with the domain logic.
193-209: Fallback name coverage for import events.The test ensures that import events without a
fileproperty still provide a meaningful name, enhancing user clarity.
211-221: Visibility of import events without a fork event is upheld.Confirms that import events become visible to non-admin users if a fork event is absent, matching the code’s conditional logic.
223-231: Correct filtering when a fork event exists.Ensures import events remain restricted for non-admins if a fork event is present, affirming the intended admin-only behavior.
apps/webapp/app/components/run/RunTimeline.tsx (1)
14-14: Well-integrated import from new utility module.Leveraging
getHelpTextForEventandTimelineSpanEventhere centralizes event logic withintimelineSpanEvents.tsfor improved maintainability.apps/webapp/app/utils/timelineSpanEvents.ts (9)
1-3: Import statements are minimal yet sufficient.They import only necessary utilities from the core library, keeping dependencies clear.
4-5: StraightforwardTimelineEventStatetype.Establishes a clear enumeration of possible states, aiding robust type safety.
6-7: ConciseTimelineLineVariantdefinition.Supports lighter or standard styling of timeline lines without complication.
8-14:TimelineEventVariantenumeration covers diverse marker shapes.Provides versatile styling options for timeline events, ensuring consistent UI across different event states.
16-24: Descriptive shape forTimelineSpanEvent.This interface includes all necessary properties for UI representation, such as offsets, timestamps, and help text.
26-131: Clear and purposefulcreateTimelineSpanEventsFromSpanEventsfunction.
- Filters valid events using
"trigger.dev/"prefix.- Sorts by timestamp.
- Applies admin visibility logic and checks for fork events.
- Calculates offsets in nanoseconds.
- Assigns default marker variants and help text.
Implementation is well-structured with minimal redundancy.
133-160:getFriendlyNameForEventenhances event readability.Replacing raw event codes with user-friendly labels improves UI clarity. Its usage is well-tailored to known events.
162-186:getAdminOnlyForEventrestricts sensitive events.Defaults to restricting unknown events to admins, which mitigates accidental exposure of internal details.
188-227: Comprehensive help text mapping ingetHelpTextForEvent.A robust approach for delivering descriptive tooltips, aiding comprehension, and maintaining code separation.
references/d3-chat/src/trigger/crawler.ts (2)
6-17: Validate inputs more thoroughly if needed.
z.string()ensures a string is passed in, but if the Python script depends on well-formed URLs, consider stricter validation (e.g., a URL schema).Would you like a quick check or stricter parsing for URL integrity?
19-19: Exported tool usage looks consistent.Exporting
crawleras an AI tool is straightforward, ensuring external availability. No further concerns here.references/d3-chat/src/trigger/chat.ts (2)
2-4: Imports are well-organized and consistent.No issues found; the new imports for
openai,logger, andschemaTaskseamlessly integrate with existing code.
10-10: Crawl tool integration is clear.Nice job modularizing the crawler functionality into its own file. This import statement cleanly integrates the new crawler tool.
packages/core/src/v3/workers/taskExecutor.ts (4)
30-38: Imported constants for warm/cold variants appear correct.Declaring
COLD_VARIANTandWARM_VARIANTimproves clarity about the execution mode. No issues here.
62-62: Optional flag for warm starts improves flexibility.Introducing
isWarmStartinTaskExecutorOptionsis a clean approach to controlling environment-specific behavior.
105-106: Global context assignment is straightforward.Passing
isWarmStartintotaskContext.setGlobalTaskContextensures consistent warm/cold logic across hooks.
311-317: Conditional attribute setting for warm/cold mode is well-structured.Skipping style attributes in
DEVELOPMENTis a sensible choice, and the ternary logic forWARM_VARIANTvs.COLD_VARIANTis clear.packages/cli-v3/src/entryPoints/dev-run-worker.ts (3)
39-39: Good addition of populateEnv importThis import enables dynamic population of environment variables in the task execution context, which aligns with the PR objective of enhancing process forking before warm start.
241-246: Well-implemented environment variable handlingThe handler now accepts an
envparameter and conditionally populates environment variables with override capability. This enhancement improves flexibility in environment configuration for task execution.
311-311: Good metrics enhancement with file pathAdding the
fileproperty to the metrics measurement provides better traceability by capturing the file path of the task manifest, improving observability.packages/core/src/v3/runTimelineMetrics/runTimelineMetricsManager.ts (2)
17-17: Good encapsulation practiceCalling the private method from the public interface makes the code more maintainable by clearly defining internal vs. external API boundaries.
33-38: Well-implemented warm start optimizationThe method is now private (using # prefix) and includes logic to check if warm start is enabled. This prevents redundant metrics registration during warm starts, aligning with the PR objective.
packages/cli-v3/src/executions/taskRunProcess.ts (6)
52-54: Good enhancement of TaskRunProcessOptionsAdding
machine,isWarmStart, andcwdproperties provides better configuration options for the task run process, improving its flexibility and capability to handle different execution environments.
57-61: Well-structured execute params typeThe new
TaskRunProcessExecuteParamstype properly encapsulates the parameters needed for execution, including support for environment variables, which improves type safety and code clarity.
88-96: Good process state tracking implementationThe addition of
_isPreparedForNextRunwith a getter provides a clear way to track whether the process can be reused for another run, which is essential for the warm start optimization strategy.
216-218: Good preparation state managementSetting
_isPreparedForNextRuntofalseat the beginning of execution ensures that the process state is accurately tracked, preventing potential reuse of a process that's already executing a task.
251-252: Well-implemented environment and warm start flagsPassing
envandisWarmStartto the worker process enables proper environment configuration and warm start handling in the worker, aligning with the PR objectives.
431-439: Good addition of forceExit methodThe new method provides a clean way to forcefully terminate the child process when needed, with appropriate error handling and logging.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation