-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix: add cache limits to prevent OOM during build/test #4188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
75342ba
1bc5576
ba700e7
81aee71
0b3af3b
3aa9827
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,15 +26,15 @@ | |
| "debug": "cross-env DEBUG=1 node --inspect-brk scripts/start.js", | ||
| "generate": "node scripts/generate-git-commit-info.js", | ||
| "generate:settings-schema": "node --import tsx/esm scripts/generate-settings-schema.ts", | ||
| "build": "node scripts/build.js", | ||
| "build": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" node scripts/build.js", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Safer approach: "build": "cross-env NODE_OPTIONS=\"${NODE_OPTIONS:-} --max-old-space-size=3072\" node scripts/build.js"Or handle this in the build script itself where you can append to |
||
| "build-and-start": "npm run build && npm run start", | ||
| "build:vscode": "node scripts/build_vscode_companion.js", | ||
| "build:all": "npm run build && npm run build:sandbox && npm run build:vscode", | ||
| "build:packages": "npm run build --workspaces", | ||
| "build:sandbox": "node scripts/build_sandbox.js", | ||
| "bundle": "npm run generate && node esbuild.config.js && node scripts/copy_bundle_assets.js", | ||
| "test": "npm run test --workspaces --if-present --parallel", | ||
| "test:ci": "npm run test:ci --workspaces --if-present --parallel && npm run test:scripts", | ||
| "test": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" npm run test --workspaces --if-present --parallel", | ||
| "test:ci": "cross-env NODE_OPTIONS=\"--max-old-space-size=3072\" npm run test:ci --workspaces --if-present --parallel && npm run test:scripts", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] In — DeepSeek/deepseek-v4-pro via Qwen Code /review |
||
| "test:scripts": "vitest run --config ./scripts/tests/vitest.config.ts", | ||
| "test:e2e": "cross-env VERBOSE=true KEEP_OUTPUT=true npm run test:integration:sandbox:none", | ||
| "test:integration:all": "npm run test:integration:sandbox:none && npm run test:integration:sandbox:docker && npm run test:integration:sandbox:podman", | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -115,6 +115,7 @@ export type FileReadCheckResult = | |||||||||||||||||||
|
|
||||||||||||||||||||
| export class FileReadCache { | ||||||||||||||||||||
| private readonly byInode = new Map<string, FileReadEntry>(); | ||||||||||||||||||||
| private static readonly MAX_ENTRIES = 4096; | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unlike This is a deliberate difference from crawlCache, but worth noting: |
||||||||||||||||||||
|
|
||||||||||||||||||||
| /** Build the canonical key for a file from its Stats. */ | ||||||||||||||||||||
| static inodeKey(stats: Stats): string { | ||||||||||||||||||||
|
|
@@ -261,11 +262,22 @@ export class FileReadCache { | |||||||||||||||||||
| const key = FileReadCache.inodeKey(stats); | ||||||||||||||||||||
| const existing = this.byInode.get(key); | ||||||||||||||||||||
| if (existing) { | ||||||||||||||||||||
| // Bump: move existing entry to the end of the FIFO queue so that | ||||||||||||||||||||
| // frequently-updated entries survive eviction. | ||||||||||||||||||||
| this.byInode.delete(key); | ||||||||||||||||||||
| existing.realPath = absPath; | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good pattern. The delete-then-re-insert on existing entries moves the key to the end of Map iteration order, giving LRU-like eviction using only a |
||||||||||||||||||||
| existing.mtimeMs = stats.mtimeMs; | ||||||||||||||||||||
| existing.sizeBytes = stats.size; | ||||||||||||||||||||
| this.byInode.set(key, existing); | ||||||||||||||||||||
| return existing; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| // Evict oldest entry when cache exceeds MAX_ENTRIES (FIFO) | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||
| if (this.byInode.size >= FileReadCache.MAX_ENTRIES) { | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Cache eviction happens silently — no debug log. When priorReadEnforcement rejects an edit because an evicted file returns — DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||
| const oldestKey = this.byInode.keys().next().value; | ||||||||||||||||||||
| if (oldestKey) { | ||||||||||||||||||||
| this.byInode.delete(oldestKey); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] The Impact: This is a blind OOM fix — 8 lines of new production code with no safety net. A future refactor of
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||
| } | ||||||||||||||||||||
| const entry: FileReadEntry = { | ||||||||||||||||||||
| inodeKey: key, | ||||||||||||||||||||
| realPath: absPath, | ||||||||||||||||||||
|
|
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,10 @@ import crypto from 'node:crypto'; | |||||||||||||||||||||||||||||||
| const crawlCache = new Map<string, string[]>(); | ||||||||||||||||||||||||||||||||
| const cacheTimers = new Map<string, NodeJS.Timeout>(); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Limits to prevent heap exhaustion when many projects are crawled concurrently | ||||||||||||||||||||||||||||||||
| export const MAX_CACHE_ENTRIES = 256; // max distinct project roots cached | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: This is acceptable (you can't reject a write without breaking the caller), but the constant name and doc comment should clarify it's a target, not a hard cap. Something like "target ceiling" instead of "max total paths" to set expectations. |
||||||||||||||||||||||||||||||||
| export const MAX_TOTAL_PATHS = 50_000; // max total paths across all entries | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * Generates a unique cache key based on the project directory and the content | ||||||||||||||||||||||||||||||||
| * of ignore files. This ensures that the cache is invalidated if the project | ||||||||||||||||||||||||||||||||
|
|
@@ -36,19 +40,79 @@ export const getCacheKey = ( | |||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * Reads cached data from the in-memory cache. | ||||||||||||||||||||||||||||||||
| * Bumps the entry to the end of the FIFO queue on hit so that | ||||||||||||||||||||||||||||||||
| * frequently-read crawl results survive eviction by auxiliary crawls. | ||||||||||||||||||||||||||||||||
| * Returns undefined if the key is not found. | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| export const read = (key: string): string[] | undefined => crawlCache.get(key); | ||||||||||||||||||||||||||||||||
| export const read = (key: string): string[] | undefined => { | ||||||||||||||||||||||||||||||||
| const result = crawlCache.get(key); | ||||||||||||||||||||||||||||||||
| if (result !== undefined) { | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
At minimum, rename to reflect the mutation, or add a doc comment noting the side-effect. Alternatively, only bump on |
||||||||||||||||||||||||||||||||
| crawlCache.delete(key); | ||||||||||||||||||||||||||||||||
| crawlCache.set(key, result); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Impact: Users see random slowdowns when a frequently-used crawl result is silently evicted, forcing a full re-crawl with no diagnostic signal.
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
| * Writes data to the in-memory cache and sets a timer to evict it after the TTL. | ||||||||||||||||||||||||||||||||
| * Enforces MAX_CACHE_ENTRIES (LRU by insertion order) and MAX_TOTAL_PATHS to | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] JSDoc on — DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||||||||||||||
| * prevent heap exhaustion when many large projects are crawled. | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| export const write = (key: string, results: string[], ttlMs: number): void => { | ||||||||||||||||||||||||||||||||
| // Clear any existing timer for this key to prevent premature deletion | ||||||||||||||||||||||||||||||||
| if (cacheTimers.has(key)) { | ||||||||||||||||||||||||||||||||
| clearTimeout(cacheTimers.get(key)!); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Evict oldest entries when cache exceeds entry limit (FIFO / insertion-order). | ||||||||||||||||||||||||||||||||
| // Guard: updating an existing key doesn't increase entry count, so skip eviction. | ||||||||||||||||||||||||||||||||
| while (crawlCache.size >= MAX_CACHE_ENTRIES && !crawlCache.has(key)) { | ||||||||||||||||||||||||||||||||
| const oldestKey = crawlCache.keys().next().value; | ||||||||||||||||||||||||||||||||
| if (oldestKey) { | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The
Suggested change
— glm-5.1 via Qwen Code /review |
||||||||||||||||||||||||||||||||
| crawlCache.delete(oldestKey); | ||||||||||||||||||||||||||||||||
| if (cacheTimers.has(oldestKey)) { | ||||||||||||||||||||||||||||||||
| clearTimeout(cacheTimers.get(oldestKey)!); | ||||||||||||||||||||||||||||||||
| cacheTimers.delete(oldestKey); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Evict largest entries when total path count exceeds limit. | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Consider adding a brief comment documenting the ordering dependency, or computing — glm-5.1 via Qwen Code /review |
||||||||||||||||||||||||||||||||
| // Calculate totalPaths excluding the key being updated to avoid counting old value. | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Consider maintaining a module-level — glm-5.1 via Qwen Code /review |
||||||||||||||||||||||||||||||||
| let totalPaths = 0; | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] The Impact: The actual execution path in production is untested. If the
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||||||||||||||
| for (const [k, entry] of crawlCache) { | ||||||||||||||||||||||||||||||||
| if (k !== key) { | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Impact:
Suggested change
— DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||||||||||||||||
| totalPaths += entry.length; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| while (totalPaths + results.length > MAX_TOTAL_PATHS && crawlCache.size > 0) { | ||||||||||||||||||||||||||||||||
| // Find and remove the entry with the most paths (never evict the current key) | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The total-paths eviction strategy evicts the largest entry, not the oldest. This is a different heuristic from the entry-count eviction (FIFO/oldest-first). Evicting the largest entry means a recently-crawled 40k-path monorepo could be evicted while a stale 100-path project survives — forcing an expensive re-crawl of the large project on the next access. Consider evicting oldest-first here too (consistent with the entry-count path), or document the rationale for largest-first. Also, this inner loop is O(n) per eviction and runs in a |
||||||||||||||||||||||||||||||||
| let largestKey: string | undefined; | ||||||||||||||||||||||||||||||||
| let largestSize = 0; | ||||||||||||||||||||||||||||||||
| for (const [k, v] of crawlCache) { | ||||||||||||||||||||||||||||||||
| if (k === key) continue; | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] Empty-array entries ( Consider initializing — glm-5.1 via Qwen Code /review |
||||||||||||||||||||||||||||||||
| if (v.length > largestSize) { | ||||||||||||||||||||||||||||||||
| largestSize = v.length; | ||||||||||||||||||||||||||||||||
| largestKey = k; | ||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Consider adding a test for a single-key cache updated with >50k paths, and optionally refuse oversized single entries ( — glm-5.1 via Qwen Code /review |
||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| if (largestKey) { | ||||||||||||||||||||||||||||||||
| totalPaths -= crawlCache.get(largestKey)!.length; | ||||||||||||||||||||||||||||||||
| crawlCache.delete(largestKey); | ||||||||||||||||||||||||||||||||
| if (cacheTimers.has(largestKey)) { | ||||||||||||||||||||||||||||||||
| clearTimeout(cacheTimers.get(largestKey)!); | ||||||||||||||||||||||||||||||||
| cacheTimers.delete(largestKey); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Bump existing key to end of FIFO queue (mirror fileReadCache.upsert behavior) | ||||||||||||||||||||||||||||||||
| if (crawlCache.has(key)) { | ||||||||||||||||||||||||||||||||
| crawlCache.delete(key); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Store the new data | ||||||||||||||||||||||||||||||||
| crawlCache.set(key, results); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
cross-env NODE_OPTIONS="--max-old-space-size=3072"hardcodes a heap limit that overwrites anyNODE_OPTIONSset in the user's or CI's environment. This can crash Node.js on machines with <4GB RAM (V8 exits if the requested heap size exceeds available physical memory). Consider moving this to CI config (.github/workflows/*.yml) so local developers and low-memory environments aren't affected.— DeepSeek/deepseek-v4-pro via Qwen Code /review