feat: run.ctx tidying and additions #2322
Conversation
🦋 Changeset detectedLatest commit: 45c8e00 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 update introduces several enhancements and refactors across the codebase. The run context parameter ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. 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: 2
🔭 Outside diff range comments (1)
apps/webapp/app/v3/services/completeAttempt.server.ts (1)
700-700: Update metadata helper to use V3 typesThe private method
#generateMetadataAttributesForNextAttemptis still typed againstTaskRunExecutioneven though every caller is passing aV3TaskRunExecution. This mismatch is what’s driving the// @ts-ignore. For full type-safety, update both the parameter and the parser to the V3 variants.• In apps/webapp/app/v3/services/completeAttempt.server.ts, at line 700, change:
- #generateMetadataAttributesForNextAttempt(execution: TaskRunExecution) { + #generateMetadataAttributesForNextAttempt(execution: V3TaskRunExecution) {• Swap out the Zod parse call:
- const context = TaskRunContext.parse(execution); + const context = V3TaskRunContext.parse(execution);• Add imports for
V3TaskRunExecutionandV3TaskRunContextfrom@trigger.dev/sdk/v3(or your core schemas), then remove the// @ts-ignore.This will eliminate the ignore, ensure end-to-end type consistency, and guard against future regressions.
🧹 Nitpick comments (5)
internal-packages/cache/README.md (1)
1-4: Expand documentation for better developer experience.The README is quite minimal for a new internal cache package. Consider adding:
- API documentation with usage examples
- Configuration options for Redis connection
- Error handling patterns and best practices
- Integration examples with the run engine
This would help other developers understand and effectively use this internal package.
internal-packages/cache/src/stores/redis.ts (3)
25-31: Good backward compatibility approach for cache keys.The dual separator logic allows for gradual migration from "::" to ":" separators. Consider adding a comment explaining why this backward compatibility is needed.
private buildCacheKey(namespace: TNamespace, key: string): string { if (this.config.useModernCacheKeyBuilder) { + // Modern format uses single colon separator return [namespace, key].join(":"); } + // Legacy format uses double colon separator for backward compatibility return [namespace, key].join("::"); }
33-67: Solid implementation with good error handling.The
getmethod properly handles cache misses and parsing errors. However, there's a minor redundancy.if (!raw) { - return Promise.resolve(Ok(undefined)); + return Ok(undefined); }
90-104: Remove method correctly implemented with minor redundancy.public async remove(namespace: TNamespace, key: string): Promise<Result<void, CacheError>> { try { const cacheKey = this.buildCacheKey(namespace, key); await this.redis.del(cacheKey); - return Promise.resolve(Ok()); + return Ok(); } catch (err) {internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
86-97: Consider making cache TTL values configurableThe TTL values are hardcoded. Consider making these configurable through options or environment variables for easier tuning in production.
-const ORG_FRESH_TTL = 60000 * 60 * 24; // 1 day -const ORG_STALE_TTL = 60000 * 60 * 24 * 2; // 2 days +const ORG_FRESH_TTL = options.cacheTTL?.org?.fresh ?? 60000 * 60 * 24; // 1 day +const ORG_STALE_TTL = options.cacheTTL?.org?.stale ?? 60000 * 60 * 24 * 2; // 2 days
internal-packages/run-engine/src/engine/systems/dequeueSystem.ts
Outdated
Show resolved
Hide resolved
| @@ -76,6 +76,30 @@ export const loader = async ({ request, params }: LoaderFunctionArgs) => { | |||
| lockedBy: { | |||
| select: { | |||
| filePath: true, | |||
| worker: { | |||
There was a problem hiding this comment.
TODO look into this because not sure if it needs updating or removing
…n, just not the run.ctx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/v3/schemas/common.ts (1)
340-439: Consider documentation for schema versioning strategy.The V3-prefixed schemas provide necessary backward compatibility and enhanced typing, but the proliferation of similar schemas (TaskRun vs V3TaskRun, TaskRunExecution vs V3TaskRunExecution, etc.) could create maintenance challenges and developer confusion.
Consider adding comprehensive documentation that:
- Explains when to use each schema version
- Clarifies the migration path between versions
- Documents field differences (e.g., why
context,durationMs,costInCentsare "execution only" inTaskRunbut included inV3TaskRun)The schemas themselves are well-structured and follow proper Zod patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
references/hello-world/src/trigger/usage.tsis excluded by!references/**
📒 Files selected for processing (9)
packages/cli-v3/src/entryPoints/dev-run-worker.ts(2 hunks)packages/cli-v3/src/entryPoints/managed-run-worker.ts(3 hunks)packages/core/src/v3/schemas/common.ts(3 hunks)packages/core/src/v3/usage/api.ts(2 hunks)packages/core/src/v3/usage/devUsageManager.ts(3 hunks)packages/core/src/v3/usage/noopUsageManager.ts(2 hunks)packages/core/src/v3/usage/prodUsageManager.ts(3 hunks)packages/core/src/v3/usage/types.ts(1 hunks)packages/trigger-sdk/src/v3/usage.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/cli-v3/src/entryPoints/managed-run-worker.ts
- packages/cli-v3/src/entryPoints/dev-run-worker.ts
- packages/trigger-sdk/src/v3/usage.ts
🧰 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/usage/api.tspackages/core/src/v3/usage/noopUsageManager.tspackages/core/src/v3/usage/types.tspackages/core/src/v3/usage/prodUsageManager.tspackages/core/src/v3/usage/devUsageManager.tspackages/core/src/v3/schemas/common.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/usage/api.tspackages/core/src/v3/usage/noopUsageManager.tspackages/core/src/v3/usage/types.tspackages/core/src/v3/usage/prodUsageManager.tspackages/core/src/v3/usage/devUsageManager.tspackages/core/src/v3/schemas/common.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : The `run` function contains your task logic in Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Do not use or add new code to the legacy run engine; focus on using and migrating to Run Engine 2.0 in `@internal/run-engine`.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using metadata in tasks, use the `metadata` API as shown, and only inside run functions or task lifecycle hooks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When implementing scheduled (cron) tasks, use `schedules.task` from `@trigger.dev/sdk/v3` and follow the shown patterns.
packages/core/src/v3/schemas/common.ts (12)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : When implementing schema tasks, use schemaTask from @trigger.dev/sdk/v3 and validate payloads as shown.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : ALWAYS generate Trigger.dev tasks using the task function from @trigger.dev/sdk/v3 and export them as shown in the correct pattern.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : You MUST use @trigger.dev/sdk/v3 when writing Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : When implementing scheduled (cron) tasks, use schedules.task from @trigger.dev/sdk/v3 and follow the shown patterns.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Before generating any code for Trigger.dev tasks, verify: (1) Are you importing from @trigger.dev/sdk/v3? (2) Have you exported every task? (3) Have you generated any deprecated code patterns?
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : When using metadata in tasks, use the metadata API as shown, and only inside run functions or task lifecycle hooks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : The run function contains your task logic in Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them as shown in the examples for Trigger.dev tasks.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : Tasks must be exported, even subtasks in the same file.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : You MUST export every task, including subtasks, in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-07-18T17:50:25.014Z
Learning: Applies to /trigger//*.{ts,tsx,js,jsx} : When triggering a task from backend code, use tasks.trigger, tasks.batchTrigger, or tasks.triggerAndPoll as shown in the examples.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-18T17:49:24.468Z
Learning: Applies to {packages/core,apps/webapp}/**/*.{ts,tsx} : We use zod a lot in packages/core and in the webapp
🧬 Code Graph Analysis (5)
packages/core/src/v3/usage/api.ts (1)
packages/core/src/v3/usage/types.ts (1)
InitialUsageState(10-13)
packages/core/src/v3/usage/noopUsageManager.ts (1)
packages/core/src/v3/usage/types.ts (1)
InitialUsageState(10-13)
packages/core/src/v3/usage/types.ts (1)
packages/core/src/v3/usage/api.ts (1)
UsageManager(60-62)
packages/core/src/v3/usage/prodUsageManager.ts (1)
packages/core/src/v3/usage/types.ts (1)
InitialUsageState(10-13)
packages/core/src/v3/usage/devUsageManager.ts (1)
packages/core/src/v3/usage/types.ts (1)
InitialUsageState(10-13)
⏰ 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). (25)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
packages/core/src/v3/schemas/common.ts (4)
224-232: LGTM! Schema enhancements align with PR objectives.The new optional fields
parentTaskRunIdandrootTaskRunIdproperly support the hierarchical task run tracking mentioned in the PR objectives. ThebaseCostInCentsfield provides clear separation from runtime costs, and the clarifying comment about execution-only fields is helpful for maintainability.
250-262: Good simplification of execution schemas.The streamlined
TaskRunExecutionTaskandTaskRunExecutionAttemptschemas maintain essential fields while removing deprecated ones, aligning with the PR's "tidying" objectives. The core functionality is preserved with proper type safety.
302-312: Excellent addition supporting ctx.deployment functionality.The new
TaskRunExecutionDeploymentschema directly implements the PR objective of providing deployment information in the run context. The comprehensive fields (id, shortCode, version, runtime details, and git metadata) offer robust deployment tracking capabilities.
313-338: Excellent refactoring with shared execution shape.The introduction of
StaticTaskRunExecutionShapedemonstrates good architectural principles by applying DRY and ensuring consistency across execution schemas. The shared shape improves maintainability and includes the new deployment field appropriately.packages/core/src/v3/usage/noopUsageManager.ts (2)
1-1: LGTM: Import updated correctlyThe import statement properly includes
InitialUsageStatetype needed for the new method.
34-39: LGTM: No-op implementation follows established patternThe
getInitialState()method correctly returns zero values, which is appropriate for a no-operation usage manager. This aligns with the interface requirement and maintains consistency with other no-op methods in this class.packages/core/src/v3/usage/api.ts (2)
4-4: LGTM: Import statement updated correctlyThe import properly includes
InitialUsageStatetype for the new method signature.
56-58: LGTM: Delegation pattern followed correctlyThe
getInitialState()method properly delegates to the underlying usage manager, maintaining consistency with other public methods in this API class.packages/core/src/v3/usage/types.ts (2)
10-13: LGTM: Well-designed type definitionThe
InitialUsageStatetype is appropriately defined with clear property names and correct numeric types for CPU time and cost tracking.
17-17: LGTM: Interface extension is minimal and focusedThe addition of
getInitialState()method to theUsageManagerinterface follows good design principles with a clear return type and no parameters.packages/core/src/v3/usage/devUsageManager.ts (3)
1-1: LGTM: Import updated correctlyThe import statement properly includes the
InitialUsageStatetype needed for the new functionality.
48-51: LGTM: Initial state properly initializedThe
_initialStateproperty is correctly initialized with zero values for bothcpuTimeandcostInCents.
61-65: LGTM: Consistent state resetThe
reset()method correctly resets the_initialStatealongside other internal state, maintaining consistency.packages/core/src/v3/usage/prodUsageManager.ts (4)
2-2: LGTM: Import updated correctlyThe import statement properly includes the
InitialUsageStatetype for the new functionality.
16-19: LGTM: Initial state properly initializedThe
_initialStateproperty is correctly initialized with zero values, following the same pattern as other usage managers.
34-40: Implementation follows established patternThe
setInitialState()andgetInitialState()methods are implemented correctly, with the same interface consistency consideration noted forDevUsageManagerregarding the publicsetInitialState()method not being part of the interface.
49-52: LGTM: Consistent state reset in reset methodThe
reset()method correctly resets the_initialStatealongside other internal state, maintaining consistency with the overall reset behavior.
Added and cleaned up the run ctx param:
ctx.run.parentTaskRunIdandctx.run.rootTaskRunIdreference the current run's root/parent ID.ctxctx.deploymentobject that contains information about the deployment associated with the run.We also update
metadata.rootandmetadata.parentto work even when the run is a "root" run (meaning it doesn't have a parent or a root associated run). This now works: