Overlay: Fall back to full analysis if memory flag is low #3332
Conversation
kaspersv
force-pushed
the
kaspersv/overlay-memory-limit
branch
from
60d714b to
bd8d26b
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a memory check to the overlay analysis decision logic, falling back to full analysis when CodeQL has less than 5GB of memory available. This primarily targets self-hosted runners with limited memory to prevent out-of-memory errors during analysis.
Key Changes:
- Added memory threshold check (5GB) alongside existing disk space check for overlay analysis
- Memory check only applies when overlay analysis is feature-enabled, not when manually controlled via environment variable
- Proper test coverage for both pull request and default branch scenarios with low memory
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/init-action.ts | Passes ramInput parameter to config initialization |
| src/config-utils.ts | Implements memory check (< 5GB) to disable overlay analysis, refactors if-else chain to handle memory constraint alongside disk space constraint |
| src/config-utils.test.ts | Adds test cases for low memory scenarios on both PR and default branch, updates test infrastructure to stub getMemoryFlagValue |
mbg
left a comment
mbg
left a comment
There was a problem hiding this comment.
Looks good to me. I added a few minor, non-blocking comments.
| ) | ||
| ) { | ||
| const diskUsage = await checkDiskUsage(logger); | ||
| const memoryFlagValue = getMemoryFlagValue(ramInput, logger); |
There was a problem hiding this comment.
Minor: I wonder if it would make sense to rename getMemoryFlagValue since the result is now used for something other than the "memory flag".
| "with caching because we are analyzing the default branch.", | ||
| ); | ||
| } | ||
| } else if (memoryFlagValue < 5 * 1024) { |
There was a problem hiding this comment.
It might be good to turn 5 * 1024 into a constant (e.g. OVERLAY_MINIMUM_AVAILABLE_MEMORY_BYTES) for clarity and consistency.
| overlayDatabaseMode = OverlayDatabaseMode.None; | ||
| useOverlayDatabaseCaching = false; |
There was a problem hiding this comment.
This pattern of disabling overlay mode is now duplicated in a lot and might be worth de-duplicating. I think you might be doing that in #3333 already.
|
Agreed. Will address comments in a follow up. |
This PR modifies the
initaction to perform full analysis instead of overlay analysis if CodeQL will have less than 5GB of memory available for analysis. All non-slim GitHub hosted runners for public and private repos have +7GB of memory and the action will assign more than 5GB of memory to CodeQL on all such runners unless a lower memory limit has been manually specified. The limit is primarily intended to target self-hosted runners with little memory available for CodeQL as we have seen out-of-memory errors on self-hosted runners where CodeQL has only been given 3GB of memory for analysis.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
Workflow types:
dynamicworkflows (Default Setup, CCR, ...).Products:
analysis-kinds: code-scanning.analysis-kinds: code-quality.Environments:
github.com.How did/will you validate this change?
.test.tsfiles).If something goes wrong after this change is released, what are the mitigation and rollback strategies?
How will you know if something goes wrong after this change is released?
Are there any special considerations for merging or releasing this change?
Merge / deployment checklist