fix(storage): derive storage stats from quota-status.json when available - #1990
fix(storage): derive storage stats from quota-status.json when available#1990kriszyp wants to merge 6 commits into
Conversation
Table.getStorageStats() and blob.ts's storage-path weighting both used raw statfs, which reports filesystem-level free space. Under a quota-limited data directory this can be wrong by orders of magnitude: analytics can report hundreds of GB "available" while the instance has zero remaining quota and is failing every write. Both now go through a new server/storageReclamation.ts helper, getStorageSpaceStats(), that prefers a fresh quota-status.json (written by host-manager, already used by storage reclamation) over statfs, refactoring the existing reclamation ratio calculation to use it too so there's a single source of truth for quota-vs-statfs selection. Fixes #1976 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Docs: no companion documentation PR — this is a correctness fix to an existing internal metric ( — KrAIs (Claude) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a shared, quota-aware utility getStorageSpaceStats in storageReclamation.ts to retrieve available, free, and total storage space stats, falling back to statfs if a fresh quota status file is unavailable. This utility is integrated into table storage stats, blob storage path weighting, and analytics volume metrics. The review feedback highlights several robustness and performance improvements: handling potential NaN values if usedBytes is missing or if size is zero (causing division by zero), wrapping fire-and-forget promise operations in a try/catch block to prevent unhandled promise rejections, and parallelizing sequential await calls in the analytics metrics collection loop to avoid blocking.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed; no blockers found. The new commit since the last review (48a3304, "resolve symlinks when scoping quota to a path") correctly hardens |
…ric, harden NaN edges - Revert resources/blob.ts's storage-path weighting back to raw per-path statfs. quota-status.json is a single instance-wide figure; using it for createFrequencyTableForStoragePaths collapsed every disk in a multi-path STORAGE_BLOBPATHS config to the same "available" value, destroying the per-disk differentiation that function exists to compute. The metric fix (Table.getStorageStats(), the actual #1976 report) is unaffected — it's a single well-defined path under the quota-controlled data directory. - Make the fire-and-forget createFrequencyTableForStoragePaths call handle its own rejection instead of leaving it unhandled. - Guard getStorageSpaceStats/defaultGetAvailableSpaceRatio against NaN: a quota-status.json missing usedBytes, or a zero-size statfs result, no longer produces NaN (which silently failed the reclamation priority comparison for a full disk). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e metrics Addresses the remaining draft review comments on #1990: - getStorageSpaceStats now confirms the requested path is actually inside ROOTPATH before trusting quota-status.json, and validates usedBytes/ quotaBytes/updatedAt before using them — a database on its own STORAGE_PATH (or an invalid/incomplete quota file) now correctly falls back to statfs instead of reporting the root's quota or fabricating zero usage. - storeVolumeMetrics in analytics/write.ts now fetches per-database storage stats concurrently via Promise.all instead of sequentially awaiting each one. - Table.getStorageStats() is now explicitly `static async` to document the promise-returning contract (authorized: only the in-tree analytics consumer awaits it already, and this was never part of the public docs). - Extracted the available/size ratio guard into an exported computeAvailableRatio() so the zero-size regression test exercises real public behavior instead of a rewire __set__ injection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- isPathWithinRoot no longer rejects a real child path whose first segment happens to start with ".." (e.g. Kubernetes ConfigMap/Secret's "..data" symlink convention) — check the segment exactly, not a prefix. - getStorageSpaceStats no longer treats a future/clock-skewed quota-status.json updatedAt as "fresh"; freshness now requires a non-negative age as well as being under the max age. Found by an independent pre-push review pass on PR #1990. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
isPathWithinRoot compared lexically-normalized paths, so a path that stays under ROOTPATH syntactically but is actually a symlink into another volume (e.g. a STORAGE_PATH mounted via a symlinked directory) was incorrectly treated as covered by the root's quota. Resolve real paths instead, walking up to the nearest existing ancestor for a not-yet-created target so a symlinked parent directory is still caught. Found by independent pre-push review on PR #1990. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
🤖 Dispatch codex/gpt-5.6-sol review (verdict CHANGES).
|
Solid change — the quota helper fails safe on every path I checked (missing/stale/malformed/unreadable quota-status.json all fall back to statfs), and the review-thread concerns are genuinely fixed in the head code. Two things before merge:
Minor: STORAGE_VOLUME Review by @heskew (posted via Claude). Generated by Claude Code |
1 participant
What
Table.getStorageStats()(feeding thestorage-volumeanalytics metric) andblob.ts's storage-path weighting both derived available/free/size from rawstatfs.statfsdescribes the filesystem, which is wrong whenever thedata directory sits under a user/group/project quota, or is a subtree of a
much larger shared volume: analytics can report hundreds of GB "available"
while the instance has zero remaining quota and is failing every write.
Why
Fixes #1976. On an observed instance, Harper's own storage metrics never
indicated pressure before or during a quota-exhaustion outage —
statfskept reporting the filesystem's real free space, oblivious to the quota.
How
Both call sites now go through a new
getStorageSpaceStats(path)helper inserver/storageReclamation.ts, which prefers a freshquota-status.json(written by host-manager every ~90s) over
statfs— the samefreshness-checked pattern
storageReclamation.tsalready used internallyfor its own reclamation-trigger ratio, and that
getDiskInfo()alreadysurfaces as metadata for
system_information. The existing ratiocalculation is refactored to use the same helper, so there is a single
source of truth for the quota-vs-statfs choice.
resources/Table.ts—getStorageStats()becomes a thin async delegateto
getStorageSpaceStats().resources/analytics/write.ts—storeVolumeMetricsawaits it.resources/blob.ts—createFrequencyTableForStoragePaths(which weightswhich storage path new blobs land on) uses it instead of raw
statfs;directory creation for a not-yet-existing path is now explicit (it used to
piggyback on a
statfsENOENT, which the quota branch never triggers).basis: 'quota' | 'filesystem'tag, so theanalytics record itself is now self-describing about which source
produced it.
Test plan
npm run typecheck/npm run buildunitTests/server/storageReclamation.test.js— added coverage forgetStorageSpaceStats(fresh quota, over-quota clamp, absent/stalequota → statfs fallback), all existing tests unaffected.
unitTests/resources/blob.test.js(63 tests) andunitTests/resources/analytics/write.test.js(36 tests) passunchanged.
npm run format:write/npm run lint:requiredclean.for the statfs branch (same formula) plus a new quota branch already unit
tested at the source; see Risks below for what a reviewer may want to spot
check.
Risks / open questions
blob.ts'screateFrequencyTableForStoragePaths(the storage-pathweighting logic) has no dedicated unit test. It's internal/unexported and
blob.tshas a module-scope_assignPackageExport('createBlob', ...)side effect that makes it unsafe to
rewire()in isolation (pollutesglobal.createBlobfor the rest of the test run — confirmed by a spuriousfailure in an unrelated
caching.test.jssuite while iterating on this).Coverage here relies on
getStorageSpaceStatsitself being fully testedplus the existing full
blob.test.jssuite staying green.codex/claudereview runs were still actively working (large log output, noerrors) when killed by the available time budget on this shared,
concurrently-loaded host.
independent-review: FAILED — reviewer CLI did not finish within the available window— flagging for the shepherd topick up rather than claiming coverage that didn't happen.
Refs #1976
🤖 Generated with Claude Code