[Fix]: Investigate Memory Leak on Verify Data Integrity#1269
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplace in-memory OutputData with a Map and streaming JSONL output; add loadOutputData(filePath) that reads JSONL or legacy JSON; update createApiHelpers to support streaming via outputFilePath, expose verifyOutputCompleteness and finalizeOutput; index.ts changed to use paginated user processing and the new helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Script
participant Helpers as createApiHelpers
participant TempFile as TempFile (JSONL)
participant TargetData as TargetOutputData (Map)
Script->>Helpers: createApiHelpers({ targetOutputData, outputFilePath })
Helpers->>TempFile: create temp file (if streaming)
Script->>Helpers: expectStatusCode(endpoint, output)
Helpers->>TempFile: append JSONL line `{ endpoint, output }`
Helpers->>Helpers: increment outputCountByEndpoint[endpoint]
Script->>Helpers: verifyOutputCompleteness()
Helpers->>TargetData: compare observed counts vs targetOutputData counts
Script->>Helpers: finalizeOutput()
Helpers->>TempFile: rename temp -> final outputFilePath
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
This kept slowly growing the old heap leading to a fatal error. I removed it but tried to keep the same functionality for the two flags that depended on it via checkpointed writes and comparisons
we used to read allusers for each project into memory. For a project with lots of users, this array could grow unbounded.
caa8cfb to
ad43185
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses an out-of-memory failure in verify-data-integrity by removing in-memory accumulation of API outputs and switching to streamed output recording and incremental verification.
Changes:
- Replaced
currentOutputDataaccumulation with per-endpoint counters plus optional JSONL streaming to disk for--save-output. - Updated
--verify-outputto validate outputs incrementally during execution and added a final completeness check. - Refactored user iteration to paginate and avoid holding all users in memory per project.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| apps/backend/scripts/verify-data-integrity/index.ts | Switches to new API helper contract, paginates user processing, and verifies output completeness at end. |
| apps/backend/scripts/verify-data-integrity/api.ts | Adds JSONL/legacy loader, streams outputs to disk, compares outputs incrementally, and verifies expected call counts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR resolves a JavaScript heap OOM error in the Key points:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([main]) --> B{mutually\nexclusive\ncheck}
B -- both flags --> ERR([throw Error])
B -- ok --> C[loadOutputData\nif --verify-output]
C --> D[createApiHelpers\noutputFilePath if --save-output]
D --> E[for each project]
E --> F[fetch 3 project endpoints\nin Promise.all]
F --> G{skipUsers?}
G -- yes --> H[skip user loop]
G -- no --> I[DB count for display\nuserCount]
I --> J[while hasMore AND\nusersProcessed < max]
J --> K[fetch page of users\nPAGE_LIMIT=1000]
K --> L[for each user in page]
L --> M[expectStatusCode\nappendOutputData]
M --> N{tmpFilePath?}
N -- yes --> O[fs.appendFileSync\none JSONL line]
N -- no --> P[count only]
O --> Q{targetOutputData?}
P --> Q
Q -- yes --> R[compare vs\ntargetOutputData count-1]
Q -- no --> S[continue]
R -- mismatch --> THROW([StackAssertionError])
R -- match --> S
L --> L2{more users\nin page?}
L2 -- yes --> L
L2 -- no --> J2{hasMore?}
J2 -- yes --> J
J2 -- no --> H
H --> E2{more\nprojects?}
E2 -- yes --> E
E2 -- no --> VC[verifyOutputCompleteness\ncheck all endpoint counts]
VC --> FO{shouldSaveOutput?}
FO -- yes --> RENAME[finalizeOutput\nrename .tmp to final path]
FO -- no --> END([done])
RENAME --> END
Last reviewed commit: "chore: defensive typ..." |
Can't pass verify-output and save-output together else file will be overwritten.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/backend/scripts/verify-data-integrity/index.ts (1)
247-285: Encode the dynamic user/team IDs in these URLs.Line 247, Line 256, Line 274, and Line 285 still interpolate
user.id/team.iddirectly. The cursor is encoded already, but these values are not; reserved characters would break the request, and this bypasses the repo URL rule.Suggested fix
- await expectStatusCode(200, `/api/v1/users/${user.id}`, { + await expectStatusCode(200, `/api/v1/users/${encodeURIComponent(user.id)}`, { @@ - const projectPermissions = await expectStatusCode(200, `/api/v1/project-permissions?user_id=${user.id}`, { + const projectPermissions = await expectStatusCode(200, `/api/v1/project-permissions?user_id=${encodeURIComponent(user.id)}`, { @@ - const teams = await expectStatusCode(200, `/api/v1/teams?user_id=${user.id}`, { + const teams = await expectStatusCode(200, `/api/v1/teams?user_id=${encodeURIComponent(user.id)}`, { @@ - const teamPermissions = await expectStatusCode(200, `/api/v1/team-permissions?team_id=${team.id}`, { + const teamPermissions = await expectStatusCode(200, `/api/v1/team-permissions?team_id=${encodeURIComponent(team.id)}`, {As per coding guidelines, "Use
urlString`` orencodeURIComponent()` instead of normal string interpolation for URLs for consistency."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/verify-data-integrity/index.ts` around lines 247 - 285, The URLs that interpolate dynamic IDs (e.g., `/api/v1/users/${user.id}`, `/api/v1/project-permissions?user_id=${user.id}`, `/api/v1/teams?user_id=${user.id}`, `/api/v1/team-permissions?team_id=${team.id}` used with expectStatusCode) must encode those values to avoid breaking on reserved characters; replace the plain interpolation with the repo-standard encoder (use urlString(...) or encodeURIComponent(...) around user.id and team.id) so the constructed request paths are safe and conform to the URL rule while keeping the same calls to expectStatusCode and the surrounding logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/backend/scripts/verify-data-integrity/api.ts`:
- Around line 29-44: Narrow the catch to only handle SyntaxError and add
defensive type checks before using the `in` operator: after parsing `firstLine`
with `JSON.parse(firstLine)` validate `parsed` is a non-null object (e.g.,
typeof parsed === "object" && parsed !== null) and that `endpoint` and `output`
exist as expected; similarly, when iterating lines parse each line into
`parsedLine`, validate it's an object and contains the `endpoint` and `output`
fields before accessing them and pushing into the `data` OutputData map; only
treat JSON parse failures (SyntaxError) as a JSONL fallback—let other errors
propagate so true bugs surface.
In `@apps/backend/scripts/verify-data-integrity/index.ts`:
- Around line 322-323: The pagination loop is using
usersPage.pagination?.next_cursor which is always undefined because the shared
CRUD pagination exposes the cursor as next_n; update the loop to read
usersPage.pagination?.next_n instead, i.e. set hasMore =
!!usersPage.pagination?.next_n and userCursor = usersPage.pagination?.next_n ??
undefined so subsequent pages are fetched past PAGE_LIMIT when more results
exist.
- Around line 218-221: Replace the non-null assertion on the result of
getPrismaClientForTenancy(tenancy) with an explicit invariant check: call
getPrismaClientForTenancy(tenancy), assign to a local prisma variable, then if
prisma is undefined throw a clear error (e.g., via throwErr(...) or an explicit
Error) that states the expectation (Prisma client must exist for tenancy.id)
before using prisma to compute userCount; update the code around the prisma and
userCount references so the count call only runs after the explicit check.
---
Nitpick comments:
In `@apps/backend/scripts/verify-data-integrity/index.ts`:
- Around line 247-285: The URLs that interpolate dynamic IDs (e.g.,
`/api/v1/users/${user.id}`, `/api/v1/project-permissions?user_id=${user.id}`,
`/api/v1/teams?user_id=${user.id}`,
`/api/v1/team-permissions?team_id=${team.id}` used with expectStatusCode) must
encode those values to avoid breaking on reserved characters; replace the plain
interpolation with the repo-standard encoder (use urlString(...) or
encodeURIComponent(...) around user.id and team.id) so the constructed request
paths are safe and conform to the URL rule while keeping the same calls to
expectStatusCode and the surrounding logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd8c91eb-776c-4c0e-9802-a4bdbe928dca
📒 Files selected for processing (2)
apps/backend/scripts/verify-data-integrity/api.tsapps/backend/scripts/verify-data-integrity/index.ts
|
@greptileai reread PR desc and see new commits? Update summary and rereview please |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/backend/scripts/verify-data-integrity/api.ts (1)
32-45:⚠️ Potential issue | 🟡 MinorNarrow the catch block and add defensive type validation for inner parsing.
This partially addresses the previous review comment but still has issues:
- Line 43's bare
catch {}still catches all errors, not justSyntaxError. Per coding guidelines, "NEVER try-catch-all".- Line 37's inner
JSON.parse(line)can throwSyntaxErrorfor malformed lines, but this would be swallowed and misclassified as "not JSONL".- Line 39 uses
!non-null assertion afterdata.get(endpoint)- prefer defensive coding.Suggested fix
try { const parsed = JSON.parse(firstLine); if (typeof parsed === "object" && parsed !== null && "endpoint" in parsed && "output" in parsed) { for (const line of lines) { if (!line.trim()) continue; const { endpoint, output } = JSON.parse(line); if (!data.has(endpoint)) data.set(endpoint, []); - data.get(endpoint)!.push(output); + const arr = data.get(endpoint); + if (arr == null) { + throw new StackAssertionError(`Expected endpoint ${endpoint} to exist in data after set`); + } + arr.push(output); } return data; } - } catch { - // Not JSONL — fall through to legacy parse + } catch (error) { + if (!(error instanceof SyntaxError)) throw error; + // Not valid JSON on first line — fall through to legacy parse }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/verify-data-integrity/api.ts` around lines 32 - 45, The try-catch should only catch JSON parsing errors and you must defensively validate each inner parsed line and avoid the non-null assertion: change the outer catch to only handle SyntaxError from JSON.parse(firstLine); inside the loop wrap JSON.parse(line) in its own try-catch that catches SyntaxError and continues on malformed lines; after parsing each line, check that the result is a non-null object and that endpoint is a string (or expected type) and output exists before using them; replace data.get(endpoint)! usage with a safe pattern (e.g., const bucket = data.get(endpoint) ?? []; bucket.push(output); data.set(endpoint, bucket)) so you never rely on the non-null assertion.
🧹 Nitpick comments (1)
apps/backend/scripts/verify-data-integrity/api.ts (1)
64-70: Consider cleaning up temp file on unhandled errors.The temp file is created at line 69 but only renamed to the final path in
finalizeOutput(). If the script crashes between these points, the.tmpfile will be orphaned. This is a minor operational concern.Optional: Register cleanup handler
+ function cleanupTempFile() { + if (tmpFilePath && fs.existsSync(tmpFilePath)) { + try { fs.unlinkSync(tmpFilePath); } catch { /* ignore */ } + } + } + if (tmpFilePath) { fs.writeFileSync(tmpFilePath, ""); + process.on("exit", cleanupTempFile); + process.on("SIGINT", () => { cleanupTempFile(); process.exit(1); }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/verify-data-integrity/api.ts` around lines 64 - 70, tmpFilePath is created but may be left behind if the script crashes before finalizeOutput() renames it; register cleanup handlers (process.on for 'exit', 'uncaughtException', 'unhandledRejection', and SIGINT) that check tmpFilePath and remove the file (fs.unlinkSync within try/catch) to delete the orphaned .tmp, and ensure finalizeOutput() removes those handlers after successful rename so you don't attempt double-deletes; reference the tmpFilePath variable and finalizeOutput() when adding and removing the cleanup logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/backend/scripts/verify-data-integrity/api.ts`:
- Around line 32-45: The try-catch should only catch JSON parsing errors and you
must defensively validate each inner parsed line and avoid the non-null
assertion: change the outer catch to only handle SyntaxError from
JSON.parse(firstLine); inside the loop wrap JSON.parse(line) in its own
try-catch that catches SyntaxError and continues on malformed lines; after
parsing each line, check that the result is a non-null object and that endpoint
is a string (or expected type) and output exists before using them; replace
data.get(endpoint)! usage with a safe pattern (e.g., const bucket =
data.get(endpoint) ?? []; bucket.push(output); data.set(endpoint, bucket)) so
you never rely on the non-null assertion.
---
Nitpick comments:
In `@apps/backend/scripts/verify-data-integrity/api.ts`:
- Around line 64-70: tmpFilePath is created but may be left behind if the script
crashes before finalizeOutput() renames it; register cleanup handlers
(process.on for 'exit', 'uncaughtException', 'unhandledRejection', and SIGINT)
that check tmpFilePath and remove the file (fs.unlinkSync within try/catch) to
delete the orphaned .tmp, and ensure finalizeOutput() removes those handlers
after successful rename so you don't attempt double-deletes; reference the
tmpFilePath variable and finalizeOutput() when adding and removing the cleanup
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40e190ea-4e5d-4cea-a8df-d8e7a78c4f30
📒 Files selected for processing (2)
apps/backend/scripts/verify-data-integrity/api.tsapps/backend/scripts/verify-data-integrity/index.ts
This was the old behavior
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/backend/scripts/verify-data-integrity/index.ts (3)
250-251: Encode dynamic URL IDs consistently.Dynamic IDs are interpolated directly into path/query strings in these calls. Please encode them for consistency and safety.
🔧 Proposed change
- await expectStatusCode(200, `/api/v1/users/${user.id}`, { + await expectStatusCode(200, `/api/v1/users/${encodeURIComponent(user.id)}`, { @@ - const projectPermissions = await expectStatusCode(200, `/api/v1/project-permissions?user_id=${user.id}`, { + const projectPermissions = await expectStatusCode(200, `/api/v1/project-permissions?user_id=${encodeURIComponent(user.id)}`, { @@ - const teams = await expectStatusCode(200, `/api/v1/teams?user_id=${user.id}`, { + const teams = await expectStatusCode(200, `/api/v1/teams?user_id=${encodeURIComponent(user.id)}`, { @@ - const teamPermissions = await expectStatusCode(200, `/api/v1/team-permissions?team_id=${team.id}`, { + const teamPermissions = await expectStatusCode(200, `/api/v1/team-permissions?team_id=${encodeURIComponent(team.id)}`, {As per coding guidelines, "Use
urlString``\" orencodeURIComponent()` instead of normal string interpolation for URLs for consistency."Also applies to: 259-260, 277-278, 288-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/verify-data-integrity/index.ts` around lines 250 - 251, The test calls that interpolate dynamic IDs (e.g., the expectStatusCode calls that build `/api/v1/users/${user.id}` and the similar calls at the other noted locations) should encode those IDs before inserting into the path; update the string construction to use a safe encoder (e.g., wrap the ID with encodeURIComponent or use the project's urlString helper) so the endpoint path becomes `/api/v1/users/${encodeURIComponent(user.id)}` (and similarly for the other calls at 259-260, 277-278, 288-289) to ensure consistent, safe URL encoding when calling expectStatusCode.
236-237: Prefer explicit null checks for cursor state transitions.The cursor checks currently rely on truthiness/boolean coercion. Using
!= nullkeeps the intent explicit and avoids conflating nullability with empty-string truthiness.🔍 Proposed change
- const cursorParam: string = userCursor ? `&cursor=${encodeURIComponent(userCursor)}` : ""; + const cursorParam: string = userCursor != null ? `&cursor=${encodeURIComponent(userCursor)}` : ""; @@ - hasMore = !!usersPage.pagination?.next_cursor; + hasMore = usersPage.pagination?.next_cursor != null; userCursor = usersPage.pagination?.next_cursor ?? undefined;As per coding guidelines, "Unless very clearly equivalent from types, prefer explicit null/undefinedness checks over boolean checks, e.g.,
foo == nullinstead of!foo."Also applies to: 325-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/verify-data-integrity/index.ts` around lines 236 - 237, The cursor construction and conditional requests use truthiness on userCursor which can conflate empty string with null/undefined; update the checks to use explicit null/undefined comparisons (e.g., userCursor != null) when building cursorParam and where userCursor is tested later (references: cursorParam, userCursor and the call to expectStatusCode(`/api/v1/users?...`)), so that only null/undefined are treated as “no cursor” and empty strings are preserved if they are valid values.
267-300: Avoid repeated linear permission-definition scans in hot loops.Both permission checks currently run
.some(...)for every permission item. BuildingSets once per project gives O(1) membership checks and reduces work on large projects.♻️ Proposed refactor
+ const projectPermissionDefinitionIds = new Set( + projectPermissionDefinitions.items.map((p: any) => p.id), + ); + const teamPermissionDefinitionIds = new Set( + teamPermissionDefinitions.items.map((p: any) => p.id), + ); const projectPermissions = await expectStatusCode(200, `/api/v1/project-permissions?user_id=${user.id}`, { @@ - for (const projectPermission of projectPermissions.items) { - // `any` because these endpoint response types aren't imported here, - // and this script is intentionally tolerant of response shape changes. - if (!projectPermissionDefinitions.items.some((p: any) => p.id === projectPermission.id)) { + for (const projectPermission of projectPermissions.items) { + if (!projectPermissionDefinitionIds.has(projectPermission.id)) { throw new StackAssertionError(deindent` Project permission ${projectPermission.id} not found in project permission definitions. `); } } @@ - for (const teamPermission of teamPermissions.items) { - // `any` because these endpoint response types aren't imported here, - // and this script is intentionally tolerant of response shape changes. - if (!teamPermissionDefinitions.items.some((p: any) => p.id === teamPermission.id)) { + for (const teamPermission of teamPermissions.items) { + if (!teamPermissionDefinitionIds.has(teamPermission.id)) { throw new StackAssertionError(deindent` Team permission ${teamPermission.id} not found in team permission definitions. `); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/backend/scripts/verify-data-integrity/index.ts` around lines 267 - 300, The permission checks inside the project and team loops currently call projectPermissionDefinitions.items.some(...) and teamPermissionDefinitions.items.some(...) for every permission, causing repeated linear scans; fix this by creating Sets of definition ids once before the loops (e.g., const projectPermissionDefIds = new Set(projectPermissionDefinitions.items.map((p:any) => p.id)) and const teamPermissionDefIds = new Set(teamPermissionDefinitions.items.map((p:any) => p.id))) and then replace the per-item .some checks with O(1) membership checks (projectPermissionDefIds.has(projectPermission.id) and teamPermissionDefIds.has(teamPermission.id)) in the loops that iterate projectPermissions.items and teamPermissions.items respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/backend/scripts/verify-data-integrity/index.ts`:
- Around line 250-251: The test calls that interpolate dynamic IDs (e.g., the
expectStatusCode calls that build `/api/v1/users/${user.id}` and the similar
calls at the other noted locations) should encode those IDs before inserting
into the path; update the string construction to use a safe encoder (e.g., wrap
the ID with encodeURIComponent or use the project's urlString helper) so the
endpoint path becomes `/api/v1/users/${encodeURIComponent(user.id)}` (and
similarly for the other calls at 259-260, 277-278, 288-289) to ensure
consistent, safe URL encoding when calling expectStatusCode.
- Around line 236-237: The cursor construction and conditional requests use
truthiness on userCursor which can conflate empty string with null/undefined;
update the checks to use explicit null/undefined comparisons (e.g., userCursor
!= null) when building cursorParam and where userCursor is tested later
(references: cursorParam, userCursor and the call to
expectStatusCode(`/api/v1/users?...`)), so that only null/undefined are treated
as “no cursor” and empty strings are preserved if they are valid values.
- Around line 267-300: The permission checks inside the project and team loops
currently call projectPermissionDefinitions.items.some(...) and
teamPermissionDefinitions.items.some(...) for every permission, causing repeated
linear scans; fix this by creating Sets of definition ids once before the loops
(e.g., const projectPermissionDefIds = new
Set(projectPermissionDefinitions.items.map((p:any) => p.id)) and const
teamPermissionDefIds = new Set(teamPermissionDefinitions.items.map((p:any) =>
p.id))) and then replace the per-item .some checks with O(1) membership checks
(projectPermissionDefIds.has(projectPermission.id) and
teamPermissionDefIds.has(teamPermission.id)) in the loops that iterate
projectPermissions.items and teamPermissions.items respectively.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e61c688c-3b16-4ed1-9ea2-f57352b941aa
📒 Files selected for processing (1)
apps/backend/scripts/verify-data-integrity/index.ts
Context
We encountered an out of memory error when running verify-data-integrity against the prod database. This was the error:
FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory. This was one of the things preventing verify-data-integrity from running successfully in prod.Summary of Changes
Local stress testing with constrained heap and memory telemetry revealed that the rise in used heap memory was directly proportional to the number of api calls. Investigation revealed that the
currentOutputDataarray was growing with each api call and was kept in memory. Since it was still being appended to, it was actively kept in the heap. We refactor the script to no longer use it, and for the two flags--save-outputand--verify-outputthat used it before, we refactor them to not need to.--save-outputnow streams responses to disk as JSONL and--verify-outputnow compares each response immediately and discards it.We also note a potential source of a future memory leak in the
allUsersarray that is populated in memory for each project. We refactor to paginate instead. Note that this didn't cause a memory leak on local, this is a preventive measure.Out of Scope
fetching all transactions in the payments section of the script is another potential cause for concern, but since the payments section of the script will be refactored soon, we defer that discussion.
Summary by CodeRabbit
New Features
Bug Fixes & Improvements