fix: migrate flattenAttributes tests to vitest and cleanup buildImage#3043
fix: migrate flattenAttributes tests to vitest and cleanup buildImage#3043deepshekhardas wants to merge 2 commits intotriggerdotdev:mainfrom
Conversation
🦋 Changeset detectedLatest commit: acafe7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 28 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hi @deepshekhardas, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces multiple improvements across the CLI and core packages: a fix for logging objects with dotted keys, dependency updates for the Depot CLI, expanded build configuration for lockfile handling and package manager selection, new memory management utilities to prevent out-of-memory errors on memory-constrained systems, and enhancements to attribute flattening logic to properly escape special characters. Additionally, several UI adjustments are made to the RunFilters component, and comprehensive test coverage is added for the new build and containerfile generation features. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (14)
✏️ Tip: You can disable this entire section by setting 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const isMemoryIntensive = | ||
| process.argv.includes("deploy") || process.argv.includes("dev") || process.argv.includes("build"); |
There was a problem hiding this comment.
🟡 process.argv.includes() matches command arguments, not just subcommands, causing unintended respawns
The memory check in ensureSufficientMemory uses process.argv.includes("deploy") || process.argv.includes("dev") || process.argv.includes("build") to determine if the current command is memory-intensive. However, Array.prototype.includes checks every element in process.argv, including flag values and arguments — not just the subcommand name.
Detailed Explanation
For example, running trigger login --profile dev produces process.argv of ["/path/to/node", "/path/to/trigger", "login", "--profile", "dev"]. The check process.argv.includes("dev") returns true, causing the CLI to respawn with --max-old-space-size=4096 even though login is not a memory-intensive command.
Similarly, trigger whoami --tag deploy or trigger list --env dev would trigger unnecessary respawns.
Impact: Non-memory-intensive commands are respawned with extra overhead (added startup latency) when any argument value happens to be "deploy", "dev", or "build". This could also confuse users who see unexpected debug logging about memory limit increases.
The fix should check specifically the subcommand position (e.g., process.argv[2]) or use a more targeted matching strategy.
| const isMemoryIntensive = | |
| process.argv.includes("deploy") || process.argv.includes("dev") || process.argv.includes("build"); | |
| const isMemoryIntensive = | |
| process.argv[2] === "deploy" || process.argv[2] === "dev" || process.argv[2] === "build"; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| /** | ||
| * Escapes dots and backslashes in a key so they are not confused with | ||
| * the `.` nesting separator used by flattenAttributes. | ||
| */ | ||
| function escapeKey(key: string): string { | ||
| if (!key.includes(".") && !key.includes("\\")) { | ||
| return key; | ||
| } | ||
| return key.replace(/\\/g, "\\\\").replace(/\./g, "\\."); | ||
| } |
There was a problem hiding this comment.
🚩 Dot-escaping in flattenAttributes is a breaking change for stored/serialized data
The new escapeKey function (flattenAttributes.ts:257-262) changes the wire format of flattened keys: keys containing dots are now escaped (e.g., "Key 0.002mm" becomes "Key 0\\.002mm" instead of "Key 0.002mm"). While splitEscapedKey at flattenAttributes.ts:269-288 handles both new escaped keys and legacy unescaped keys transparently (unescaped dots are split the same way as before), there is a subtle asymmetry:
-
New flatten → old unflatten: If new code flattens
{ "Key 0.002mm": 31.4 }to{ "Key 0\\.002mm": 31.4 }, and an older version ofunflattenAttributes(usingkey.split(".")) reads it, the escaped backslash-dot would be split incorrectly, producing wrong keys like"Key 0\\"and"002mm". -
Old flatten → new unflatten: Works correctly because
splitEscapedKeysplits on unescaped dots just like the oldsplit(".")did.
This means this change is forward-incompatible if there are mixed deployments or if flattened data is persisted/transmitted between components running different versions. Given this is used for OpenTelemetry attributes in logs, if the webapp and workers run different versions during a rolling deployment, log attributes with dotted keys may display incorrectly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| RUN ${options.lockfile && options.lockfile.endsWith(".lockb") | ||
| ? "bun install --frozen-lockfile --production" | ||
| : "bun install --production --no-save" | ||
| } |
There was a problem hiding this comment.
🚩 Bun lockfile check only handles .lockb, not newer .lock format
In generateBunContainerfile, the lockfile detection at buildImage.ts:768 only checks options.lockfile.endsWith(".lockb") to decide whether to use bun install --frozen-lockfile --production. Newer versions of Bun use a text-based bun.lock format (not binary .lockb). If a project uses bun.lock, the condition fails and falls through to bun install --production --no-save, which does not use the lockfile for deterministic installs.
Looking at the caller in buildWorker.ts:237-243, the packageManager detection also doesn't account for bun lockfiles (it maps to "npm" by default). For the bun runtime path this doesn't matter since generateBunContainerfile doesn't use packageManager, but the lockfile format gap means bun.lock users won't get frozen lockfile installs. This may be intentional given the test only covers .lockb, but it's worth noting as bun's default lockfile format has changed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| COPY --chown=node:node package.json ${options.lockfile ? `${options.lockfile} ` : ""}./ | ||
| RUN ${options.packageManager === "pnpm" | ||
| ? "npx pnpm i --prod --no-frozen-lockfile" | ||
| : options.packageManager === "yarn" | ||
| ? "yarn install --production --no-lockfile" | ||
| : options.lockfile && options.lockfile.endsWith("package-lock.json") | ||
| ? "npm ci --no-audit --no-fund" | ||
| : "npm i --no-audit --no-fund --no-save --no-package-lock" | ||
| } |
There was a problem hiding this comment.
🚩 Node containerfile lockfile COPY includes lockfile but pnpm/yarn paths ignore it
In generateNodeContainerfile at buildImage.ts:876-883, when packageManager is "pnpm" or "yarn", the install command doesn't use --frozen-lockfile (pnpm gets --no-frozen-lockfile, yarn gets --no-lockfile). However, the lockfile IS still copied into the container on line 876 via COPY --chown=node:node package.json ${options.lockfile} ./.
This means the lockfile is present in the image but explicitly not used for deterministic installs. This appears intentional (possibly to avoid lockfile version mismatch issues since the package.json dependencies are rewritten in buildWorker.ts:196-211), but it's worth confirming this is the desired behavior since it means deployments are not reproducible from the lockfile.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR migrates the lattenAttributes.test.ts to Vitest to resolve memory/resource exhaustion issues observed during deployment tests. It also performs minor cleanup in �uildImage.ts by removing an unused deploymentSpinner option.