fix: inject environment variables at dequeue time for self-hosted deployments #2793
Conversation
…loyments This commit fixes the issue where environment variables configured in the Trigger.dev portal were not being injected into runner pods/containers in self-hosted deployments (GitHub issue triggerdotdev#2792). Root Cause: Environment variables were only being fetched during the 'startRunAttempt' phase, which occurs AFTER the runner pod/container has already been created. This meant that pods were created without access to portal-configured environment variables, causing tasks to fail immediately. Solution: Move environment variable fetching from the 'startRunAttempt' phase to the 'dequeue' phase, so that env vars are available when the supervisor creates the runner workload. Changes: 1. Added optional 'envVars' field to DequeuedMessage schema 2. Modified AuthenticatedWorkerInstance.dequeue() to fetch and include environment variables for each dequeued message 3. Added 'envVars' field to WorkloadManagerCreateOptions interface 4. Updated supervisor to pass envVars when creating workloads 5. Modified KubernetesWorkloadManager to inject envVars into pod spec 6. Modified DockerWorkloadManager to inject envVars into container Testing: - Tested in self-hosted Kubernetes deployment (EKS) - Verified environment variables appear in runner pods - Confirmed tasks execute successfully with portal-configured env vars Fixes triggerdotdev#2792
|
WalkthroughAdds optional per-message environment variable support across the dequeue-to-workload pipeline. DequeuedMessage schema and WorkloadManagerCreateOptions gain an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts (1)
381-408: Consider performance implications of N+1 database queries.For each dequeued message, the code fetches the runtime environment from the database (lines 385-392). When multiple messages are dequeued in a batch, this results in N separate database queries. While this may be acceptable for self-hosted deployments with lower dequeue rates, consider batching environment fetches if performance becomes a concern.
If needed, environments could be fetched in a single query:
// Fetch all unique environments in one query const envIds = [...new Set(messages.map(m => m.environment.id))]; const environments = await this._prisma.runtimeEnvironment.findMany({ where: { id: { in: envIds } }, include: { parentEnvironment: true }, }); const envMap = new Map(environments.map(e => [e.id, e])); // Then look up from map instead of querying per message const environment = envMap.get(message.environment.id);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/supervisor/src/index.ts(1 hunks)apps/supervisor/src/workloadManager/docker.ts(1 hunks)apps/supervisor/src/workloadManager/kubernetes.ts(1 hunks)apps/supervisor/src/workloadManager/types.ts(1 hunks)apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts(1 hunks)packages/core/src/v3/schemas/runEngine.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/core/src/v3/schemas/runEngine.tsapps/supervisor/src/workloadManager/docker.tsapps/supervisor/src/index.tsapps/supervisor/src/workloadManager/types.tsapps/supervisor/src/workloadManager/kubernetes.tsapps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
packages/core/src/v3/schemas/runEngine.tsapps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/core/src/v3/schemas/runEngine.tsapps/supervisor/src/workloadManager/docker.tsapps/supervisor/src/index.tsapps/supervisor/src/workloadManager/types.tsapps/supervisor/src/workloadManager/kubernetes.tsapps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/core/src/v3/schemas/runEngine.tsapps/supervisor/src/workloadManager/docker.tsapps/supervisor/src/index.tsapps/supervisor/src/workloadManager/types.tsapps/supervisor/src/workloadManager/kubernetes.tsapps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access all environment variables through the
envexport ofenv.server.tsinstead of directly accessingprocess.envin the Trigger.dev webapp
Files:
apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, use subpath exports from the package.json instead of importing from the root path
Follow the Remix 2.1.0 and Express server conventions when updating the main trigger.dev webapp
Files:
apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
apps/webapp/app/v3/services/**/*.server.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Organize services in the webapp following the pattern
app/v3/services/*/*.server.ts
Files:
apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: julienvanbeveren
Repo: triggerdotdev/trigger.dev PR: 2417
File: apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.import.ts:56-61
Timestamp: 2025-08-19T09:49:07.011Z
Learning: In the Trigger.dev codebase, environment variables should default to `isSecret: false` when not explicitly marked as secrets in the syncEnvVars functionality. This is the intended behavior for both regular variables and parent variables.
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
📚 Learning: 2025-11-27T16:26:58.661Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-11-27T16:26:58.661Z
Learning: Applies to apps/webapp/app/**/*.{ts,tsx} : Access all environment variables through the `env` export of `env.server.ts` instead of directly accessing `process.env` in the Trigger.dev webapp
Applied to files:
apps/supervisor/src/workloadManager/docker.tsapps/supervisor/src/index.tsapps/supervisor/src/workloadManager/kubernetes.ts
🧬 Code graph analysis (1)
apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts (2)
internal-packages/run-engine/src/run-queue/index.ts (5)
message(1458-1506)message(1763-1818)message(1859-1907)message(1909-1937)message(1958-1960)apps/webapp/app/v3/machinePresets.server.ts (1)
machinePresetFromName(27-32)
🔇 Additional comments (5)
apps/supervisor/src/workloadManager/types.ts (1)
27-27: LGTM! Clean type addition.The optional
envVarsfield is well-placed and appropriately typed for carrying per-message environment variables through the workload creation pipeline.apps/supervisor/src/index.ts (1)
270-270: LGTM! Proper forwarding of environment variables.The envVars are correctly passed from the dequeued message to the workload manager, completing the pipeline from webapp to container/pod creation.
apps/supervisor/src/workloadManager/docker.ts (1)
113-117: LGTM! Consistent environment variable injection.The implementation mirrors the existing
additionalEnvVarspattern. Note thatopts.envVarswill overrideadditionalEnvVarsif there are duplicate keys, which appears intentional (portal-configured variables should take precedence over global configuration).packages/core/src/v3/schemas/runEngine.ts (1)
273-273: LGTM! Backward-compatible schema extension.The optional
envVarsfield properly extends theDequeuedMessageschema with the correct Zod type, enabling environment variable propagation without breaking existing consumers.apps/supervisor/src/workloadManager/kubernetes.ts (1)
228-233: LGTM! Consistent Kubernetes environment variable injection.The implementation mirrors the
additionalEnvVarspattern and maintains consistency with the Docker workload manager. Portal-configured variables will override global configuration if keys conflict, which aligns with expected behavior.
| const messagesWithEnvVars = await Promise.all( | ||
| messages.map(async (message) => { | ||
| const defaultMachinePreset = machinePresetFromName(defaultMachine); | ||
|
|
||
| const environment = await this._prisma.runtimeEnvironment.findFirst({ | ||
| where: { | ||
| id: message.environment.id, | ||
| }, | ||
| include: { | ||
| parentEnvironment: true, | ||
| }, | ||
| }); | ||
|
|
||
| const envVars = environment | ||
| ? await this.getEnvVars( | ||
| environment, | ||
| message.run.id, | ||
| message.run.machine ?? defaultMachinePreset, | ||
| environment.parentEnvironment ?? undefined | ||
| ) | ||
| : {}; | ||
|
|
||
| return { | ||
| ...message, | ||
| envVars, | ||
| }; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Missing taskEventStore parameter in getEnvVars call.
The getEnvVars method expects an optional taskEventStore parameter (line 582), which is used to set OTEL_RESOURCE_ATTRIBUTES for observability (lines 599-610). The startRunAttempt method correctly passes this parameter (line 481), but the dequeue enrichment does not.
Apply this diff to include the taskEventStore parameter:
const envVars = environment
? await this.getEnvVars(
environment,
message.run.id,
message.run.machine ?? defaultMachinePreset,
- environment.parentEnvironment ?? undefined
+ environment.parentEnvironment ?? undefined,
+ message.run.taskEventStore ?? undefined
)
: {};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const messagesWithEnvVars = await Promise.all( | |
| messages.map(async (message) => { | |
| const defaultMachinePreset = machinePresetFromName(defaultMachine); | |
| const environment = await this._prisma.runtimeEnvironment.findFirst({ | |
| where: { | |
| id: message.environment.id, | |
| }, | |
| include: { | |
| parentEnvironment: true, | |
| }, | |
| }); | |
| const envVars = environment | |
| ? await this.getEnvVars( | |
| environment, | |
| message.run.id, | |
| message.run.machine ?? defaultMachinePreset, | |
| environment.parentEnvironment ?? undefined | |
| ) | |
| : {}; | |
| return { | |
| ...message, | |
| envVars, | |
| }; | |
| }) | |
| ); | |
| const messagesWithEnvVars = await Promise.all( | |
| messages.map(async (message) => { | |
| const defaultMachinePreset = machinePresetFromName(defaultMachine); | |
| const environment = await this._prisma.runtimeEnvironment.findFirst({ | |
| where: { | |
| id: message.environment.id, | |
| }, | |
| include: { | |
| parentEnvironment: true, | |
| }, | |
| }); | |
| const envVars = environment | |
| ? await this.getEnvVars( | |
| environment, | |
| message.run.id, | |
| message.run.machine ?? defaultMachinePreset, | |
| environment.parentEnvironment ?? undefined, | |
| message.run.taskEventStore ?? undefined | |
| ) | |
| : {}; | |
| return { | |
| ...message, | |
| envVars, | |
| }; | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts around
lines 381-408, the call to getEnvVars is missing the optional taskEventStore
parameter; update the call to pass the same taskEventStore used elsewhere in
this class (the same variable passed in startRunAttempt at ~line 481) — e.g. add
, taskEventStore (or this._taskEventStore if it's a class member) as the last
argument so getEnvVars(environment, message.run.id, message.run.machine ??
defaultMachinePreset, environment.parentEnvironment ?? undefined,
taskEventStore).
Testing & VerificationProcessWe've successfully tested this fix in a production-like environment:
Root Cause ConfirmedThe issue was that environment variables were being fetched in ImpactThis fix resolves the critical issue where environment variables configured in the Trigger.dev portal were not being injected into self-hosted runner containers, causing tasks to fail when accessing required secrets and configuration. 🤖 Tested and verified in production environment |
Bug: Tasks were queuing but not executing due to "Invalid friendly ID format" error. The fromFriendlyId() function expected exactly 2 parts when splitting on underscore, but run IDs from URLs had no prefix. Solution: Check if parts.length === 1 and return the ID as-is if there's no underscore, treating it as an already-parsed internal ID. Impact: Tasks now process normally, no more execution errors. Tested: Deployed in custom Docker images v4.0.4-patched-fixed and verified task execution working correctly in production.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core/src/v3/isomorphic/friendlyId.ts (1)
31-31: Consider updating the JSDoc to document the idempotent behavior.The function comment states "Convert a friendly ID to an internal ID," but with the new logic at lines 39-42, the function also handles inputs that are already internal IDs (returning them unchanged). Updating the JSDoc would improve clarity for future maintainers.
🔎 Suggested JSDoc update
-/** Convert a friendly ID to an internal ID */ +/** Convert a friendly ID to an internal ID. If the input is already an internal ID (no underscore), returns it unchanged. */ export function fromFriendlyId(friendlyId: string, expectedEntityName?: string): string {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/v3/isomorphic/friendlyId.ts(1 hunks)packages/trigger-sdk/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/trigger-sdk/package.json
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
Files:
packages/core/src/v3/isomorphic/friendlyId.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
packages/core/src/v3/isomorphic/friendlyId.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
packages/core/src/v3/isomorphic/friendlyId.ts
**/*.{js,ts,jsx,tsx,json,md,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using Prettier
Files:
packages/core/src/v3/isomorphic/friendlyId.ts
🔇 Additional comments (1)
packages/core/src/v3/isomorphic/friendlyId.ts (1)
39-42: Verify if this change is related to the PR's objective.This modification allows
fromFriendlyIdto accept internal IDs without entity prefixes. However, the PR's stated objective is to inject environment variables at dequeue time for self-hosted deployments. The connection between this friendly ID change and the env vars feature is not immediately apparent from the provided context.If this change is unrelated to environment variable injection, consider moving it to a separate PR for better traceability, easier review, and independent rollback capability.
Can you clarify why this friendly ID change is needed for the environment variables feature? If it's an independent fix, would you consider extracting it to a separate PR?
| // If there's no underscore, assume it's already an internal ID | ||
| if (parts.length === 1) { | ||
| return friendlyId; | ||
| } |
There was a problem hiding this comment.
Entity name validation is bypassed when input lacks an underscore.
When expectedEntityName is provided and the input has no underscore (single-part format), this early return skips the entity name validation at lines 58-60. This could allow IDs of mismatched entity types to pass through unchecked.
For example:
fromFriendlyId("someInternalId", "worker")
// Returns "someInternalId" without verifying it came from a "worker_" friendly IDIf callers rely on expectedEntityName validation for correctness or security, this bypass could lead to type confusion or incorrect behavior.
🔎 Proposed fix to validate entity name before early return
const parts = friendlyId.split("_");
// If there's no underscore, assume it's already an internal ID
if (parts.length === 1) {
+ if (expectedEntityName) {
+ throw new Error(
+ `Cannot validate entity name for internal ID without prefix. Expected: ${expectedEntityName}`
+ );
+ }
return friendlyId;
}Alternatively, if single-part inputs should always be accepted regardless of expectedEntityName, document this behavior explicitly and consider logging a warning.
🤖 Prompt for AI Agents
In packages/core/src/v3/isomorphic/friendlyId.ts around lines 39-42 (and the
entity validation at lines ~58-60), the early return for single-part inputs
bypasses expectedEntityName validation; change the logic so that when
parts.length === 1 you first check if expectedEntityName is provided — if it is,
reject (throw an error or return a failure) because a single-part ID cannot be
validated against an expected entity prefix; only when expectedEntityName is not
provided should you return the original friendlyId. Ensure this validation is
performed before any early return so mismatched entity expectations are not
silently accepted.
1 participant
Summary
This PR fixes the issue where environment variables configured in the Trigger.dev portal were not being injected into runner pods/containers in self-hosted deployments.
Fixes #2792
Root Cause
Environment variables were only being fetched during the
startRunAttemptphase, which occurs AFTER the runner pod/container has already been created. This meant that pods were created without access to portal-configured environment variables, causing tasks to fail immediately with "Missing required environment variables" errors.Solution
Move environment variable fetching from the
startRunAttemptphase to thedequeuephase, ensuring env vars are available when the supervisor creates the runner workload.Changes
Schema (
packages/core/src/v3/schemas/runEngine.ts)envVarsfield toDequeuedMessageschemaWebapp Service (
apps/webapp/app/v3/services/worker/workerGroupTokenService.server.ts)AuthenticatedWorkerInstance.dequeue()to fetch and include environment variables for each dequeued messagegetEnvVars()helper methodSupervisor Types (
apps/supervisor/src/workloadManager/types.ts)envVarsfield toWorkloadManagerCreateOptionsinterfaceSupervisor (
apps/supervisor/src/index.ts)envVarsfrom dequeued messageWorkload Managers
apps/supervisor/src/workloadManager/kubernetes.ts): InjectenvVarsinto pod environmentapps/supervisor/src/workloadManager/docker.ts): InjectenvVarsinto container environmentTesting
Backward Compatibility
This change is backward compatible:
envVarsfield is optional in the schemaenvVarsis undefined/empty, workload creation proceeds as beforeRelated Documentation