-
Notifications
You must be signed in to change notification settings - Fork 7
feat(storage): quota-aware reclamation via host-manager quota-status file #650
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
base: main
Are you sure you want to change the base?
Changes from all commits
743ebaf
94c46fc
ff316af
32a2f05
87f8160
15c6934
c0c80d1
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 |
|---|---|---|
| @@ -1,17 +1,47 @@ | ||
| import { readFile } from 'node:fs/promises'; | ||
| import { statfs } from 'node:fs/promises'; | ||
| import { join } from 'node:path'; | ||
| import { getWorkerIndex, getWorkerCount } from '../server/threads/manageThreads.js'; | ||
| import { logger } from '../utility/logging/logger.ts'; | ||
| import { CONFIG_PARAMS } from '../utility/hdbTerms.ts'; | ||
| import * as envMgr from '../utility/environment/environmentManager.ts'; | ||
| import { convertToMS } from '../utility/common_utils.ts'; | ||
| envMgr.initSync(); | ||
|
|
||
| const reclamationHandlers = new Map< | ||
| string, | ||
| { priority: number; handler: (priority: number) => Promise<void> | void }[] | ||
| >(); | ||
|
|
||
| const RECLAMATION_THRESHOLD = envMgr.get(CONFIG_PARAMS.STORAGE_RECLAMATION_THRESHOLD) ?? 0.4; // 40% remaining free space is the default | ||
| const RECLAMATION_INTERVAL = convertToMS(envMgr.get(CONFIG_PARAMS.STORAGE_RECLAMATION_INTERVAL)) || 3600000; // 1 hour is the default | ||
|
|
||
| // Written by host-manager every ~90s alongside the instance's hdb root | ||
| const QUOTA_STATUS_FILE = 'quota-status.json'; | ||
| // Use statfs fallback if the file is older than this (host-manager outage, container start race, etc.) | ||
| const QUOTA_STATUS_MAX_AGE_MS = 5 * 60 * 1000; | ||
|
|
||
| export type QuotaStatusData = { | ||
| usedBytes: number; | ||
| quotaBytes?: number; | ||
| updatedAt: number; | ||
| }; | ||
|
|
||
| /** | ||
| * Reads the quota-status.json file written by host-manager. | ||
| * Returns undefined if the file is absent or malformed; does not apply age filtering. | ||
| */ | ||
| export async function getQuotaStatus(): Promise<QuotaStatusData | undefined> { | ||
| const rootPath = envMgr.get(CONFIG_PARAMS.ROOTPATH); | ||
| if (!rootPath) return undefined; | ||
| try { | ||
| const raw = await readFile(join(rootPath, QUOTA_STATUS_FILE), 'utf8'); | ||
| return JSON.parse(raw) as QuotaStatusData; | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| } | ||
|
Contributor
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. Blocker (carries from prior run — unresolved): Missing cases:
Member
Author
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. Fixed: added |
||
|
|
||
| /** | ||
| * Register a handler to be called when storage free space is low and reclamation is needed. The callback is called | ||
| * with the priority of the reclamation, which is the ratio of the threshold to the available space ratio. If space is | ||
|
|
@@ -39,7 +69,14 @@ export function onStorageReclamation( | |
| } | ||
| } | ||
| let reclamationTimer: NodeJS.Timeout; | ||
|
|
||
| // If a fresh quota-status.json exists (written by host-manager every ~90s), use quota-based ratio. | ||
| // Otherwise fall back to statfs for the registered path. | ||
| const defaultGetAvailableSpaceRatio = async (path: string): Promise<number> => { | ||
| const status = await getQuotaStatus(); | ||
| if (status?.quotaBytes && Date.now() - status.updatedAt < QUOTA_STATUS_MAX_AGE_MS) { | ||
| return Math.max(0, status.quotaBytes - status.usedBytes) / status.quotaBytes; | ||
| } | ||
| const fsStats = await statfs(path); | ||
| return fsStats.bavail / fsStats.blocks; | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| 'use strict'; | ||
|
|
||
| const assert = require('node:assert/strict'); | ||
| const fs = require('node:fs'); | ||
| const os = require('node:os'); | ||
| const path = require('node:path'); | ||
| const sinon = require('sinon'); | ||
| const rewire = require('rewire'); | ||
|
|
||
|
|
@@ -393,4 +396,110 @@ describe('storageReclamation module', function () { | |
| assert.ok(okPathHandler.calledOnce); | ||
| }); | ||
| }); | ||
|
|
||
| describe('quota mode', function () { | ||
| const QUOTA_100GB = 100 * 1024 * 1024 * 1024; | ||
| let tmpDir; | ||
| let quotaStatusPath; | ||
| let originalRootPath; | ||
|
|
||
| beforeEach(function () { | ||
| tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'harper-quota-test-')); | ||
| quotaStatusPath = path.join(tmpDir, 'quota-status.json'); | ||
| originalRootPath = env.get('rootPath'); | ||
| env.setProperty('rootPath', tmpDir); | ||
| }); | ||
|
|
||
| afterEach(function () { | ||
| env.setProperty('rootPath', originalRootPath); | ||
| try { | ||
| fs.rmSync(tmpDir, { recursive: true }); | ||
| } catch {} | ||
| }); | ||
|
|
||
| describe('getQuotaStatus', function () { | ||
| it('returns parsed object when file is present and valid', async function () { | ||
| const data = { usedBytes: 50_000_000_000, quotaBytes: QUOTA_100GB, updatedAt: Date.now() }; | ||
| fs.writeFileSync(quotaStatusPath, JSON.stringify(data)); | ||
| assert.deepEqual(await storageReclamation.getQuotaStatus(), data); | ||
| }); | ||
|
|
||
| it('returns undefined when file is absent', async function () { | ||
| assert.equal(await storageReclamation.getQuotaStatus(), undefined); | ||
| }); | ||
|
|
||
| it('returns undefined when file contains malformed JSON', async function () { | ||
| fs.writeFileSync(quotaStatusPath, 'not-valid-json{'); | ||
| assert.equal(await storageReclamation.getQuotaStatus(), undefined); | ||
| }); | ||
| }); | ||
|
|
||
| describe('defaultGetAvailableSpaceRatio', function () { | ||
| beforeEach(function () { | ||
| storageReclamation.setAvailableSpaceRatioGetter(undefined); // use real default | ||
| }); | ||
|
|
||
| it('uses fresh quota-status file and triggers reclamation when headroom is low', async function () { | ||
| const usedBytes = 65 * 1024 * 1024 * 1024; // 65 GB → 35% remaining → below 40% threshold | ||
| fs.writeFileSync( | ||
| quotaStatusPath, | ||
| JSON.stringify({ usedBytes, quotaBytes: QUOTA_100GB, updatedAt: Date.now() }) | ||
| ); | ||
|
|
||
| const handler = sandbox.stub().returns(Promise.resolve()); | ||
| storageReclamation.onStorageReclamation(tmpDir, handler, true); | ||
| await storageReclamation.runReclamationHandlers(); | ||
|
|
||
| assert.ok(handler.calledOnce); | ||
| assert.ok(handler.firstCall.args[0] > 1); // priority = 0.4 / 0.35 ≈ 1.14 | ||
| }); | ||
|
|
||
| it('uses fresh quota-status file and does not trigger when headroom is sufficient', async function () { | ||
| const usedBytes = 50 * 1024 * 1024 * 1024; // 50% used → 50% remaining → above threshold | ||
| fs.writeFileSync( | ||
| quotaStatusPath, | ||
| JSON.stringify({ usedBytes, quotaBytes: QUOTA_100GB, updatedAt: Date.now() }) | ||
| ); | ||
|
|
||
| const handler = sandbox.stub(); | ||
| storageReclamation.onStorageReclamation(tmpDir, handler, true); | ||
| await storageReclamation.runReclamationHandlers(); | ||
|
|
||
| assert.ok(handler.notCalled); | ||
| }); | ||
|
|
||
| it('clamps ratio to 0 and triggers reclamation when usage exceeds quota', async function () { | ||
| const usedBytes = 110 * 1024 * 1024 * 1024; // 10 GB over quota | ||
| fs.writeFileSync( | ||
| quotaStatusPath, | ||
| JSON.stringify({ usedBytes, quotaBytes: QUOTA_100GB, updatedAt: Date.now() }) | ||
| ); | ||
|
|
||
| const handler = sandbox.stub().returns(Promise.resolve()); | ||
| storageReclamation.onStorageReclamation(tmpDir, handler, true); | ||
| await storageReclamation.runReclamationHandlers(); | ||
|
|
||
| // Ratio clamped to 0 → priority = Infinity → handler called | ||
| assert.ok(handler.calledOnce); | ||
| }); | ||
|
|
||
| it('falls back to statfs when quota-status file is absent', async function () { | ||
| const handler = sandbox.stub(); | ||
| storageReclamation.onStorageReclamation(tmpDir, handler, true); | ||
| await assert.doesNotReject(storageReclamation.runReclamationHandlers()); | ||
| }); | ||
|
|
||
| it('falls back to statfs when quota-status file is stale', async function () { | ||
| const staleTimestamp = Date.now() - 10 * 60 * 1000; // 10 minutes old | ||
| fs.writeFileSync( | ||
| quotaStatusPath, | ||
| JSON.stringify({ usedBytes: 65 * 1024 * 1024 * 1024, quotaBytes: QUOTA_100GB, updatedAt: staleTimestamp }) | ||
| ); | ||
|
|
||
| const handler = sandbox.stub(); | ||
| storageReclamation.onStorageReclamation(tmpDir, handler, true); | ||
| await assert.doesNotReject(storageReclamation.runReclamationHandlers()); | ||
| }); | ||
| }); | ||
| }); | ||
|
Contributor
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. [Blocker — re-raised; situation worsened by the refactor commit] The "quota mode" tests (lines 421–448) stub Missing coverage that should be added:
These are the core correctness paths of the new feature.
Member
Author
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. Fixed: added a |
||
| }); | ||
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.
[Blocker — new from refactor commit]
getQuotaStatus()is a newly exported function with no tests. Per the universal review scope, new public API symbols need a happy-path test.Minimal coverage needed:
undefinedundefined(not a throw)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.
Fixed: added
describe('getQuotaStatus')tests (valid file returns parsed object, absent file returns undefined, malformed JSON returns undefined) anddescribe('getDirectoryUsageBytes')test (non-negative integer on a real directory). All use a privatemkdtempSyncsubdirectory withrootPathtemporarily overridden viaenv.setProperty. — Claude