Check disk usage using Node.js API #3247
Conversation
This was introduced in Node.js 18
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the check-disk-space package with Node.js's native fs.statfs() API for checking disk usage. The change reduces external dependencies by leveraging built-in functionality introduced in Node.js 18.
Key changes:
- Replaced
check-disk-spacepackage withfs.promises.statfs()API - Updated disk usage calculation to use filesystem block-based metrics (
bsize,bavail,blocks) - Removed package dependency from
package.json
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/util.ts | Replaced external disk space check with native Node.js API and updated calculation logic |
| package.json | Removed check-disk-space dependency |
| lib/*.js | Auto-generated JavaScript files reflecting source changes |
| const blockSizeInBytes = diskUsage.bsize; | ||
| const numBlocksPerMb = (1024 * 1024) / blockSizeInBytes; | ||
| const numBlocksPerGb = (1024 * 1024 * 1024) / blockSizeInBytes; | ||
| if (diskUsage.bavail < 2 * numBlocksPerGb) { |
There was a problem hiding this comment.
I'm surprised that we have to go through the block computation, and that there isn't a more direct measure in the API. But 🤷🏻
github-actions
bot
added
size/S
|
I compared https://github.com/github/codeql-action/actions/runs/18873645058/job/53859006836 and https://github.com/github/codeql-action/actions/runs/18844897855, and the numbers look similar. |
There was a problem hiding this comment.
Nice. As discussed elsewhere, we don't have good test coverage for this, beyond it being run as part of every test workflow and as part of status report tests.
I think a unit test specifically for this would be tricky, beyond checking error conditions / nominal conditions. I am not sure how much that would gain us compared to what is already covered by the e2e workflows and other tests.
| numTotalBytes: diskUsage.blocks * blockSizeInBytes, | ||
| }; | ||
| } catch (error) { | ||
| logger.warning( |
There was a problem hiding this comment.
In a similar spirit to the log-level change we made in #3240, should we change this from a warning to a debug message? This is only really a best effort, so it may be better to avoid the workflow annotation that results from this.
There was a problem hiding this comment.
I think while we expect the workflow validation to fail in a bunch of circumstances, this disk space check shouldn't fail. So I'd suggest we keep it as a warning.
There was a problem hiding this comment.
No strong feelings, but I could imagine that this could also fail with weird filesystem / permission setups, especially on self-hosted runners.
src/util.test.ts
Outdated
| t.true( | ||
| diskUsage?.numAvailableBytes !== undefined && | ||
| diskUsage.numAvailableBytes > 0, | ||
| ); | ||
| t.true(diskUsage?.numTotalBytes !== undefined && diskUsage.numTotalBytes > 0); |
There was a problem hiding this comment.
Minor, but I quite like t.truthy in an if condition that guards the check on the property. The && makes it harder to debug which sub-condition failed.
There was a problem hiding this comment.
It's hard to write because we can't compare undefined with 0. I'll make two separate assertions.
This API was introduced in Node.js 18. Let's use it instead of the
check-disk-spacepackage.Risk assessment
For internal use only. Please select the risk level of this change:
Which use cases does this change impact?
analysis-kinds: code-scanning).analysis-kinds: code-quality).upload-sarif).How did/will you validate this change?
.test.tsfiles).pr-checks).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?
Merge / deployment checklist