Repository navigation
fix: derive node heap limit from cgroup memory at startup - #5019
Conversation
Hardcoded --max-old-space-size=4608 assumed a fixed container memory limit, causing heavy GC thrash (minutes of scavenge/mark-sweep) before the process finally OOM-crashes when the actual k8s limit differs. Reading process.constrainedMemory() at entrypoint time sets the heap ceiling to 75% of the real cgroup limit, so V8 hits the fatal threshold sooner and the pod restarts faster.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The derived old-space limit can still exceed the container memory budget because the fixed 128 MB semi-space allocation is additional V8 memory. Reserve or derive semi-space memory before calculating the heap limit to avoid premature OOM kills under constrained limits.
Codacy reports the PR as up to standards. The required startup, fallback, OOM-behavior, and workload tests are all missing.
About this PR
- Add automated coverage for cgroup-based calculation, fallback behavior, entrypoint argument propagation, prompt OOM behavior, and normal operation within the calculated heap ceiling.
Test suggestions
- Compute 75% of a constrained cgroup memory limit.
- Fall back to os.totalmem() when process.constrainedMemory() is unavailable or returns zero.
- Entrypoint passes the computed value to --max-old-space-size before launching Node.
- Application exceeds the constrained limit and crashes promptly rather than remaining in GC thrash.
- Application operates normally under the expected workload within the calculated heap ceiling.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Compute 75% of a constrained cgroup memory limit.
2. Fall back to os.totalmem() when process.constrainedMemory() is unavailable or returns zero.
3. Entrypoint passes the computed value to --max-old-space-size before launching Node.
4. Application exceeds the constrained limit and crashes promptly rather than remaining in GC thrash.
5. Application operates normally under the expected workload within the calculated heap ceiling.
Low confidence findings
- Validate that the memory value is finite and positive, and ensure the image uses a Node runtime that supports the constrained-memory API relied upon by the calculation.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| exec node \ | ||
| --max-old-space-size=4608 \ | ||
| --max-old-space-size="${HEAP_MB}" \ | ||
| --max-semi-space-size=128 \ |
There was a problem hiding this comment.
🟡 MEDIUM RISK
This can still exceed the cgroup limit because --max-semi-space-size=128 is fixed and is in addition to the derived old-space limit. Derive the semi-space size from the same limit (or reserve its worst-case usage before calculating heapMB) so the total V8 heap remains within the cgroup budget.
max-semi-space-size was still fixed at 128MB, additive on top of the derived old-space size. Since new-space is two semi-spaces, its worst-case footprint (2x the flag value) could push total V8 heap past the cgroup limit. Now both old-space and semi-space are sized from the same memory budget, with new-space reserved first.
There was a problem hiding this comment.
Pull Request Overview
The heap calculation can exceed the intended 75% memory budget: V8 young-generation usage is undercounted, and the minimum semi-space clamp breaks the budget for small containers. This can increase cgroup OOM risk and should be resolved before merging.
Codacy reports the PR as up to standards, with no new issues or complexity findings.
Test suggestions
- Compute heap and semi-space values from a constrained cgroup memory limit.
- Fall back to os.totalmem() when constrained memory is unavailable.
- Pass the computed old-space and semi-space values to the application process.
- Verify the application reaches prompt OOM failure rather than prolonged GC thrashing when the limit is exceeded.
- Verify normal operation under the expected workload within the computed heap ceiling.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Compute heap and semi-space values from a constrained cgroup memory limit.
2. Fall back to os.totalmem() when constrained memory is unavailable.
3. Pass the computed old-space and semi-space values to the application process.
4. Verify the application reaches prompt OOM failure rather than prolonged GC thrashing when the limit is exceeded.
5. Verify normal operation under the expected workload within the computed heap ceiling.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| const semiSpaceMB = Math.min(128, Math.max(16, Math.floor(budgetMB * 0.05))); | ||
| const newSpaceMB = semiSpaceMB * 2; | ||
| const oldSpaceMB = Math.max(1, budgetMB - newSpaceMB); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Ensure the configured V8 spaces remain within the 75% memory budget. Account for the actual young-generation footprint (including three semi-spaces where applicable), and handle budgets too small to satisfy the 16 MB minimum without exceeding the limit.
Summary
--max-old-space-size=4608with a value computed from the container's actual cgroup memory limit at startupcompute-heap.mjs, run byentrypoint.shbeforeexec node, usingprocess.constrainedMemory()(cgroup-aware, Node 20+) withos.totalmem()fallback, set to 75% of the limitTest plan