fix: sentry memory leak by patching @sentry/remix to stop cloning request
#2389
Conversation
|
|
Warning Rate limit exceeded@ericallam has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
Walkthrough
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
ericallam
force-pushed
the
fix/sentry-memory-leak
branch
from
b39658b to
e674dee
Compare
![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
🔭 Outside diff range comments (1)
apps/webapp/app/routes/admin.api.v1.snapshot.ts (1)
72-79: Add missing export and switch toenvfor ECS_CONTAINER_METADATA_URIThe
ECS_CONTAINER_METADATA_URIvar isn’t defined inenv.server.ts, and this file is directly readingprocess.env.*. Per our webapp conventions, all env vars must be surfaced via theenvexport.• In apps/webapp/app/env.server.ts
export const env = { // …existing entries + /** URI for ECS container metadata */ + ECS_CONTAINER_METADATA_URI: process.env.ECS_CONTAINER_METADATA_URI!, };• In apps/webapp/app/routes/admin.api.v1.snapshot.ts (lines 72–79)
- if (!process.env.ECS_CONTAINER_METADATA_URI) { + if (!env.ECS_CONTAINER_METADATA_URI) { return "local"; } - const url = new URL(process.env.ECS_CONTAINER_METADATA_URI); + const url = new URL(env.ECS_CONTAINER_METADATA_URI); return url.pathname.split("/")[2].split("-")[0]; }• Ensure at the top of this file you import the shared
env:import { env } from "app/env.server";This aligns with our pattern of centralizing environment-variable access through
env.server.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/.gitignore(1 hunks)apps/webapp/app/routes/admin.api.v1.snapshot.ts(2 hunks)apps/webapp/memory-leak-detector.js(1 hunks)apps/webapp/sentry.server.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/.gitignore
- apps/webapp/sentry.server.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
apps/webapp/app/routes/admin.api.v1.snapshot.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:
apps/webapp/app/routes/admin.api.v1.snapshot.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/routes/admin.api.v1.snapshot.ts
🧬 Code Graph Analysis (1)
apps/webapp/app/routes/admin.api.v1.snapshot.ts (1)
apps/webapp/app/services/personalAccessToken.server.ts (1)
authenticateApiRequestWithPersonalAccessToken(105-114)
🪛 Biome (2.1.2)
apps/webapp/memory-leak-detector.js
[error] 529-529: Shouldn't redeclare 'heapGrowth'. Consider to delete it or rename it.
'heapGrowth' is defined here:
(lint/suspicious/noRedeclare)
[error] 530-530: Shouldn't redeclare 'heapGrowthPercent'. Consider to delete it or rename it.
'heapGrowthPercent' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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). (15)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (19)
apps/webapp/app/routes/admin.api.v1.snapshot.ts (3)
7-8: LGTM: Correct import updates for token-based authentication.The imports correctly replace session-based authentication with token-based authentication and add Prisma for user lookup.
28-32: LGTM: Proper token-based authentication implementation.The implementation correctly uses
authenticateApiRequestWithPersonalAccessTokenand returns a 403 response when authentication fails.
34-42: LGTM: Secure admin privilege verification.The implementation properly:
- Fetches the user by authenticated userId from Prisma
- Uses null-safe checking with
user?.admin- Returns consistent 403 error message for unauthorized access
This maintains security while switching to token-based authentication.
apps/webapp/memory-leak-detector.js (16)
1-11: LGTM: Clear shebang and comprehensive documentation.The file header provides excellent documentation explaining the purpose and usage of the script.
18-80: LGTM: Well-structured class with comprehensive configuration.The constructor provides excellent configurability with sensible defaults and creates a proper timestamped directory structure for organizing results.
82-91: LGTM: Clean logging implementation.The logging method provides proper timestamp formatting and handles both verbose and error-specific output correctly.
93-102: LGTM: Proper directory creation with error handling.The method ensures required directories exist with proper recursive creation and logging.
104-142: LGTM: Well-designed snapshot abstraction.The method provides a clean abstraction that supports both V8 heap snapshots (via admin API) and basic memory snapshots, with proper fallback behavior.
144-212: LGTM: Robust V8 heap snapshot implementation.The method includes:
- Proper timeout handling (120 seconds for large snapshots)
- Progress logging during download
- Stream error handling and cleanup
- Proper file cleanup on errors
214-296: LGTM: Comprehensive server startup process.The method handles the complete build and startup flow with proper error handling, timeout management, and server readiness detection.
298-346: LGTM: Well-implemented HTTP request method.The method provides proper timeout handling, error handling, and includes necessary headers for API authentication.
348-428: LGTM: Comprehensive request phase implementation.The method includes:
- Progress reporting for long-running tests
- Proper error tracking and limiting
- Performance metrics calculation
- Clean result aggregation
430-540: LGTM: Thorough memory analysis with multiple snapshot types.The method handles both V8 heap snapshots and process memory snapshots with proper growth calculations and percentage analysis.
542-655: LGTM: Comprehensive analysis reporting and recommendations.The method provides detailed analysis output and generates actionable recommendations based on configurable thresholds.
657-681: LGTM: Proper report generation with sensitive data redaction.The method correctly redacts the admin token while preserving other configuration details for the report.
683-699: LGTM: Proper cleanup with graceful shutdown.The cleanup method handles both SIGTERM and SIGKILL with appropriate delays for graceful shutdown.
701-772: LGTM: Well-orchestrated main execution flow.The main run method coordinates all phases properly with appropriate delays, garbage collection, and comprehensive error handling.
775-845: LGTM: Complete CLI argument parsing.The CLI interface provides comprehensive options with clear help documentation and proper argument parsing.
847-867: LGTM: Proper module execution and export.The main execution guard and module export are implemented correctly with proper error handling and exit codes.
To facilitate more accurate and consistent heap memory snapshots, a new function forceConsistentGC was added before taking a snapshot. This ensures the garbage collector (GC) runs multiple times, stabilizing the heap state for more reliable analysis. This is particularly helpful when debugging memory-related issues. Updates to the memory-leak-detector script now allow labeling of snapshot runs using the --label flag. This helps in distinguishing different runs for easier tracking and comparison of memory usage across test sessions. Additionally, the --expose-gc flag ensures that the GC can be manually triggered during test runs, leading to more consistent memory states and potentially uncovering hidden memory leaks.
The function forceConsistentGC was refactored to enhance code readability and consistency. The main improvements include: - Updated syntax for consistent string quotation and spacing. - Simplified garbage collection by removing specific major/minor GC calls, as the distinction isn't necessary. - Implemented minor changes to arrow function formatting for consistency. These changes neither impact the program logic nor the function behavior but help maintain code quality standards and readability.
Identified that the memory leak in the project was linked to the usage of request.clone() within the `@sentry/remix` package's callRouteAction handler. Although initially suspected as a Sentry issue, the problem appears to arise from the handling of request.clone() in Remix version 2.1.0. By removing the call to request.clone(), the memory leak has been resolved. - Introduced garbage collection execution before snapshot to manage memory allocation effectively. - Improved error handling and timeout mechanisms in the memory leak detector to enhance its resilience during runtime. - Expanded testing for both GET and POST requests to monitor and validate potential memory leaks better. The POST requests involve sending large payloads to stress-test the system. - The modification particularly focuses on enhancing robust memory tracking and providing detailed progress reporting during request phases.
ericallam
force-pushed
the
fix/sentry-memory-leak
branch
from
e674dee to
39d1b8d
Compare
ericallam
changed the title
@sentry/remix to stop cloning request
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (1)
108-114: Action required: remove/update remaining call site(s) that reference the removed taskListPresenter singletonSearch shows a lingering usage — update this call site to use the new factory/api or remove the import of the removed singleton.
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx
- ~127: const taskListPresenter = getTaskListPresenter();
- ~129: const { tasks, activity, runningStats, durations } = await taskListPresenter.call({ ... })
No matches for setupTaskListPresenter were found in the initial scan. Please fix the above call site(s) and re-run the search to confirm no remaining references.
apps/webapp/app/routes/admin.api.v1.gc.ts (1)
8-36: Disconnect PerformanceObserver and fix V8 flag; avoid process-wide flag flapping.
- The observer is never disconnected, which can retain callbacks and leak memory.
- The disable flag should be
--no-expose-gc(with a hyphen) to correctly revert the setting.- Setting/unsetting V8 flags is process-wide and can race with concurrent requests. Consider leaving GC exposed for the process lifetime or avoid toggling it off in a request path.
Apply this diff to tighten up the implementation:
async function waitTillGcFinishes() { - let resolver: (value: PerformanceEntry) => void; - let rejector: (reason?: any) => void; + let resolver: (value: PerformanceEntry) => void; const promise = new Promise<PerformanceEntry>((resolve, reject) => { resolver = resolve; - rejector = reject; }); - const obs = new PerformanceObserver((list) => { + const obs = new PerformanceObserver((list) => { const entry = list.getEntries()[0]; if (entry.name === "gc") { resolver(entry); + // Disconnect once we've observed the GC event to prevent leaks + try { + obs.disconnect(); + } catch {} } }); obs.observe({ entryTypes: ["gc"] }); - v8.setFlagsFromString("--expose-gc"); + v8.setFlagsFromString("--expose-gc"); const gc = global.gc ?? runInNewContext("gc"); gc(); - // disable expose-gc - v8.setFlagsFromString("--noexpose-gc"); + // Avoid toggling off in a request path; if you must, use the correct flag: + // v8.setFlagsFromString("--no-expose-gc"); return promise; }apps/webapp/app/routes/admin.api.v1.snapshot.ts (1)
93-101: Use env.server.ts for environment access and harden parsing.Direct
process.envaccess violates webapp conventions. Also,new URL(...)can throw on malformed values; guard it.-function getTaskIdentifier() { - if (!process.env.ECS_CONTAINER_METADATA_URI) { +import { env } from "~/env.server"; + +function getTaskIdentifier() { + if (!env.ECS_CONTAINER_METADATA_URI) { return "local"; } - const url = new URL(process.env.ECS_CONTAINER_METADATA_URI); + let url: URL | undefined; + try { + url = new URL(env.ECS_CONTAINER_METADATA_URI); + } catch { + return "local"; + } return url.pathname.split("/")[2].split("-")[0]; }
♻️ Duplicate comments (1)
apps/webapp/memory-leak-detector.js (1)
595-596: Fix variable redeclaration issue.The static analysis correctly identifies variable redeclaration. The
vardeclarations create variables in function scope that conflict with earlier declarations.Apply this diff to fix the redeclaration:
// Use total growth for recommendations - var heapGrowth = totalGrowth; - var heapGrowthPercent = totalGrowthPercent; + heapGrowth = totalGrowth; + heapGrowthPercent = totalGrowthPercent; } else if (snapshot1.processMemory && snapshot2.processMemory && snapshot3.processMemory) { // Traditional process memory analysis with 3 snapshots const heap1 = snapshot1.processMemory.heapUsed; const heap2 = snapshot2.processMemory.heapUsed; const heap3 = snapshot3.processMemory.heapUsed; const rss1 = snapshot1.processMemory.rss; const rss2 = snapshot2.processMemory.rss; const rss3 = snapshot3.processMemory.rss; const heapGrowth1to2 = heap2 - heap1; const heapGrowth2to3 = heap3 - heap2; const totalHeapGrowth = heap3 - heap1; const rssGrowth1to2 = rss2 - rss1; const rssGrowth2to3 = rss3 - rss2; const totalRssGrowth = rss3 - rss1; analysis = { heapUsage: { phase1: Math.round(heap1 / 1024 / 1024), phase2: Math.round(heap2 / 1024 / 1024), phase3: Math.round(heap3 / 1024 / 1024), growth1to2: Math.round(heapGrowth1to2 / 1024 / 1024), growth2to3: Math.round(heapGrowth2to3 / 1024 / 1024), totalGrowth: Math.round(totalHeapGrowth / 1024 / 1024), totalGrowthPercent: Math.round((totalHeapGrowth / heap1) * 100 * 100) / 100, }, rssUsage: { phase1: Math.round(rss1 / 1024 / 1024), phase2: Math.round(rss2 / 1024 / 1024), phase3: Math.round(rss3 / 1024 / 1024), growth1to2: Math.round(rssGrowth1to2 / 1024 / 1024), growth2to3: Math.round(rssGrowth2to3 / 1024 / 1024), totalGrowth: Math.round(totalRssGrowth / 1024 / 1024), totalGrowthPercent: Math.round((totalRssGrowth / rss1) * 100 * 100) / 100, }, snapshots: this.results.snapshots.length, }; - var heapGrowth = totalHeapGrowth; - var heapGrowthPercent = (totalHeapGrowth / heap1) * 100; + heapGrowth = totalHeapGrowth; + heapGrowthPercent = (totalHeapGrowth / heap1) * 100;Also declare these variables at the function start:
analyzeMemoryUsage() { if (this.results.snapshots.length < 3) { this.log("Need at least 3 snapshots to analyze memory usage", "warn"); return; } const snapshot1 = this.results.snapshots[0]; // after warmup const snapshot2 = this.results.snapshots[1]; // after first load test const snapshot3 = this.results.snapshots[2]; // after second load test - let analysis = {}; + let analysis = {}; + let heapGrowth; + let heapGrowthPercent;Also applies to: 635-636
🧹 Nitpick comments (7)
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (1)
108-114: Add explicit return type for clarity and API stabilityMinor nit: declare the return type to make the factory’s API explicit and resilient to refactors.
Apply this diff:
-export function getTaskListPresenter() { +export function getTaskListPresenter(): TaskListPresenter { const environmentMetricsRepository = new ClickHouseEnvironmentMetricsRepository({ clickhouse: clickhouseClient, }); return new TaskListPresenter(environmentMetricsRepository, $replica); }apps/webapp/app/v3/scheduleEngine.server.ts (1)
50-52: Consider service disposal on process/engine shutdown if applicable.If
TriggerTaskServiceopens connections or timers and exposes a cleanup (e.g.,close()/dispose()), wire it into whatever shutdown hooksScheduleEngineor your process uses. This prevents resource retention over long uptimes.If
TriggerTaskServicehas a lifecycle method, I can propose the exact wiring points once you confirm its API.patches/@[email protected] (1)
9-11: Remove unused clonedRequest variable (dead code).After removing the formData handling block,
clonedRequestis no longer used. Keeping it may confuse future readers and slightly increases bundle size.Apply this diff inside the patch to drop the unused assignment:
- const clonedRequest = params.request;apps/webapp/app/routes/admin.api.v1.gc.ts (1)
39-53: Auth flow looks good; prefer findUnique for primary key lookups.Using
findUniqueon theidfield is more idiomatic and conveys intent to Prisma.- const user = await prisma.user.findFirst({ - where: { - id: authenticationResult.userId, - }, - }); + const user = await prisma.user.findUnique({ + where: { id: authenticationResult.userId }, + });apps/webapp/app/routes/api.v1.mock.ts (1)
8-19: Return JSON with proper content-type; consider Remix json() helper.Set the
Content-Typeheader toapplication/jsonfor clients/tools expecting JSON. Optionally, usejson()from@remix-run/node.Minimal header adjustment:
return new Response( JSON.stringify({ data: { id: "123", type: "mock", attributes: { name: "Mock", }, }, }), - { status: 200 } + { status: 200, headers: { "Content-Type": "application/json" } } );Or:
import { json, type ActionFunctionArgs } from "@remix-run/node"; export async function action({}: ActionFunctionArgs) { if (env.NODE_ENV === "production") return new Response("Not found", { status: 404 }); return json({ data: { id: "123", type: "mock", attributes: { name: "Mock" } } }); }apps/webapp/app/routes/admin.api.v1.snapshot.ts (2)
77-84: Prefer web streams (or Remix helper) over Node streams; simplify error handling.Returning a Node
PassThroughtyped asanyworks but is less portable and requires manual error wiring. Use the isomorphicReadableStreamviacreateReadableStreamFromReadable.-import { PassThrough } from "stream"; +import { createReadableStreamFromReadable } from "@remix-run/node"; @@ - const body = new PassThrough(); - const stream = fs.createReadStream(snapshotPath); - stream.on("open", () => stream.pipe(body)); - stream.on("error", (err) => body.end(err)); - stream.on("end", () => body.end()); + const nodeStream = fs.createReadStream(snapshotPath); + const body = createReadableStreamFromReadable(nodeStream);
53-61: Prefer findUnique for primary key lookups.Same as in the GC route, this conveys intent and may benefit query planning.
- const user = await prisma.user.findFirst({ - where: { - id: authenticationResult.userId, - }, - }); + const user = await prisma.user.findUnique({ + where: { id: authenticationResult.userId }, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/webapp/.gitignore(1 hunks)apps/webapp/app/presenters/v3/TaskListPresenter.server.ts(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx(2 hunks)apps/webapp/app/routes/admin.api.v1.gc.ts(2 hunks)apps/webapp/app/routes/admin.api.v1.snapshot.ts(2 hunks)apps/webapp/app/routes/api.v1.mock.ts(1 hunks)apps/webapp/app/v3/scheduleEngine.server.ts(1 hunks)apps/webapp/memory-leak-detector.js(1 hunks)apps/webapp/sentry.server.ts(1 hunks)package.json(1 hunks)patches/@[email protected](1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/sentry.server.ts
- apps/webapp/.gitignore
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
apps/webapp/app/routes/api.v1.mock.tsapps/webapp/app/v3/scheduleEngine.server.tsapps/webapp/app/routes/admin.api.v1.gc.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsxapps/webapp/app/presenters/v3/TaskListPresenter.server.tsapps/webapp/app/routes/admin.api.v1.snapshot.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:
apps/webapp/app/routes/api.v1.mock.tsapps/webapp/app/v3/scheduleEngine.server.tsapps/webapp/app/routes/admin.api.v1.gc.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsxapps/webapp/app/presenters/v3/TaskListPresenter.server.tsapps/webapp/app/routes/admin.api.v1.snapshot.ts
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
Files:
apps/webapp/app/routes/api.v1.mock.tsapps/webapp/app/v3/scheduleEngine.server.tsapps/webapp/app/routes/admin.api.v1.gc.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsxapps/webapp/app/presenters/v3/TaskListPresenter.server.tsapps/webapp/app/routes/admin.api.v1.snapshot.ts
🧠 Learnings (9)
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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 inside another task, use `yourTask.trigger`, `yourTask.batchTrigger`, `yourTask.triggerAndWait`, `yourTask.batchTriggerAndWait`, `batch.triggerAndWait`, `batch.triggerByTask`, or `batch.triggerByTaskAndWait` as shown.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:50:25.014Z
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.
Applied to files:
apps/webapp/app/v3/scheduleEngine.server.ts
📚 Learning: 2025-07-18T17:49:47.180Z
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-07-18T17:49:47.180Z
Learning: Applies to apps/webapp/app/v3/presenters/**/*.server.ts : Favor the use of 'presenters' to move complex loader code into a class, as seen in `app/v3/presenters/**/*.server.ts`.
Applied to files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsxapps/webapp/app/presenters/v3/TaskListPresenter.server.ts
🪛 Biome (2.1.2)
apps/webapp/memory-leak-detector.js
[error] 635-635: Shouldn't redeclare 'heapGrowth'. Consider to delete it or rename it.
'heapGrowth' is defined here:
(lint/suspicious/noRedeclare)
[error] 636-636: Shouldn't redeclare 'heapGrowthPercent'. Consider to delete it or rename it.
'heapGrowthPercent' is defined here:
(lint/suspicious/noRedeclare)
⏰ 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). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam._index/route.tsx (2)
77-80: Good switch to factory import to avoid singleton retentionImporting getTaskListPresenter aligns with the new per-request instantiation and helps avoid any cross-request memory retention from a long-lived singleton. This matches our presenters guidance.
126-129: Per-request presenter instantiation looks correctCreating the presenter inside the loader ensures no shared state leaks between requests while still reusing the underlying clickhouseClient/$replica singletons. This is the right balance for mitigating memory issues.
apps/webapp/app/presenters/v3/TaskListPresenter.server.ts (1)
108-114: Factory-based construction prevents accidental singleton memory leaksgetTaskListPresenter constructs a fresh TaskListPresenter each call, while reusing clickhouseClient and $replica. This is a solid move to reduce risk of memory retention from presenter-level state or closures. Nice alignment with our “presenters” pattern.
apps/webapp/app/v3/scheduleEngine.server.ts (1)
99-112: No mutable instance state found in TriggerTaskService — shared instance is safeI inspected apps/webapp/app/v3/services/triggerTask.server.ts and the base classes: TriggerTaskService does not assign to any this.* fields, only reads protected readonly _prisma and _engine from BaseService/WithRunEngine, and both call paths construct per-invocation service objects (TriggerTaskServiceV1 or RunEngineTriggerTaskService). A repository grep for "this.<...> =" in that file returned no matches and WithRunEngine/BaseService show the fields are readonly.
Files checked
- apps/webapp/app/v3/services/triggerTask.server.ts — call(), callV1(), callV2() (creates per-call service instances; no this.* assignments)
- apps/webapp/app/v3/services/baseService.server.ts — BaseService/WithRunEngine (protected readonly _prisma and _engine)
Conclusion: no change required for reentrancy of TriggerTaskService.call(...).
patches/@[email protected] (1)
15-36: Approve: stop consuming action FormData — reduces memory retention & PII riskShort verification: I searched the repo for
actionFormDataAttributesandformData(. The only hits were the Sentry patch and TypeScript lib types — there are no other references toactionFormDataAttributesin the codebase. Many app route files do callrequest.formData()themselves (expected and unrelated).What I checked
- Command run: rg -n --hidden --no-ignore -S 'actionFormDataAttributes|formData('
- Results: only matches were
- patches/@[email protected] (the removed
clonedRequest.formData()read)- TypeScript lib.d.ts entries for formData()
Action items for you (local checks required)
- If you relied on actionFormDataAttributes: it’s effectively unused now (search found no other references).
- I could not inspect installed @sentry/remix in node_modules from this sandbox (search for vendor/ESM files failed). Locally, confirm whether an ESM build is used at runtime; if so, extend the patch to cover build/esm/vendor/instrumentation.js.
Useful commands to run locally
- rg -n --hidden --no-ignore -S 'actionFormDataAttributes|formData('
- fd instrumentation.js node_modules/@sentry/remix -a
package.json (1)
82-83: Patched dependency mapping looks correct — please verify patch applied in CI/local (node_modules was missing when I tried to check)The pnpm.patchedDependencies entry for @sentry/[email protected] points to patches/@[email protected] and should remove the formData capture; I couldn't verify because node_modules wasn't available.
- Files to check:
- package.json — lines ~82–83: "@sentry/[email protected]": "patches/@[email protected]"
- patches/@[email protected] — ensure the patch exists and removes formData usage
- Verification to run after install/CI:
- rg -n --hidden --no-ignore -S 'formData(' node_modules/@sentry/remix || echo "OK: No formData() usage in vendor files"
apps/webapp/app/routes/admin.api.v1.snapshot.ts (1)
63-65: Great: stabilizing GC before snapshot improves reproducibility.Calling
forceConsistentGC()prior towriteHeapSnapshotwill make comparisons across snapshots more meaningful.apps/webapp/memory-leak-detector.js (12)
1-11: Well-structured CLI tool for memory leak detection.The shebang and comprehensive documentation provide clear guidance on the tool's purpose and usage.
18-81: LGTM! Comprehensive configuration and initialization.The constructor provides extensive configuration options with sensible defaults, and the results structure is well-organized for tracking all aspects of the memory leak detection process.
83-92: Good logging implementation with level support.The logging method provides appropriate verbosity controls and proper error handling.
105-146: Smart dual-mode snapshot implementation.The method intelligently handles both V8 heap snapshots (when admin token is available) and basic process memory snapshots as fallback. The GC stabilization before V8 snapshots is a good practice.
218-262: Well-implemented GC endpoint integration.The method properly integrates with the admin API's GC endpoint with appropriate error handling and timeouts.
264-350: Robust server startup with proper build integration.The server startup process properly builds the project first, then launches with appropriate Node.js flags for memory inspection. The timeout handling and process monitoring are well implemented.
402-534: Comprehensive load testing implementation.The method performs both GET and POST requests with large payloads to effectively trigger potential memory leaks. Progress reporting and error tracking are well implemented.
536-761: Excellent memory analysis with comprehensive reporting.The analysis method handles different snapshot types appropriately, provides detailed metrics, and generates actionable recommendations. The logging output is comprehensive and user-friendly.
763-787: Proper report generation with security considerations.The report generation correctly redacts sensitive information like admin tokens while preserving all necessary data for analysis.
807-878: Well-orchestrated execution workflow.The main run method coordinates all phases properly with appropriate delays, GC calls, and error handling. The cleanup logic ensures proper server shutdown.
881-955: Comprehensive CLI argument parsing.The argument parsing provides good coverage of configuration options with helpful usage documentation.
957-977: Proper module structure and execution.The main execution block and module exports are correctly implemented with appropriate error handling and exit codes.
ericallam
force-pushed
the
fix/sentry-memory-leak
branch
2 times, most recently
from
b6915d4 to
ce94a0f
Compare
ericallam
force-pushed
the
fix/sentry-memory-leak
branch
from
ce94a0f to
a854230
Compare
3 participants
No description provided.