Skip to content

Commit 5f25483

Browse files
TheLastCicadaTheLastCicada
committed
refactor: extract /health disk-space payload into shared helper
Caught by Cursor Bugbot review: the peek -> build-object -> log -> refresh pattern was copy-pasted across /health, /v1/health, and /v2/health. Adding a field (e.g. error, path) would have required updating three call sites; missing one would silently produce inconsistent API responses. Centralise the projection in src/utils/disk-space.js: buildHealthDiskSpacePayload(). The exposed health field list is now declared once (HEALTH_DISK_SPACE_FIELDS) and the helper drives the peek + transition log + async refresh internally. The three handlers collapse to a single call. Also unexport peekDiskSpaceStatus and refreshDiskSpaceStatus now that buildHealthDiskSpacePayload is the only caller; same dead-export hygiene as the BYTES_PER_MEGABYTE removal.
1 parent 7fc5fd8 commit 5f25483

4 files changed

Lines changed: 78 additions & 105 deletions

File tree

‎src/middleware.js‎

Lines changed: 6 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,8 @@ import { sendReadOnlyError } from './utils/read-only-response.js';
2323
import { getRateLimitRetryAfterSeconds } from './utils/rate-limit.js';
2424
import {
2525
checkDiskSpace,
26-
peekDiskSpaceStatus,
27-
refreshDiskSpaceStatus,
2826
logDiskSpaceStatus,
27+
buildHealthDiskSpacePayload,
2928
buildInsufficientDiskSpaceError,
3029
isWriteRejectedByDiskGuard,
3130
} from './utils/disk-space.js';
@@ -544,34 +543,14 @@ app.use(async function (req, res, next) {
544543
});
545544

546545
app.get('/health', (req, res) => {
547-
// Surface disk-space status so monitoring can alert before writes get
548-
// blocked. Use the non-blocking peek (cached value only) and kick off
549-
// an async refresh in the background — synchronously awaiting statfs
550-
// here can cause k8s liveness probes (default 1 s timeout) to fail
551-
// when the filesystem is slow or wedged, which is exactly when /health
552-
// most needs to stay responsive.
553-
let diskSpace = null;
554-
try {
555-
const status = peekDiskSpaceStatus();
556-
if (status) {
557-
diskSpace = {
558-
severity: status.severity,
559-
freeBytes: status.freeBytes,
560-
blockBytes: status.blockBytes,
561-
warnBytes: status.warnBytes,
562-
};
563-
// Drive the debounced log from /health too so a mostly-idle CADT
564-
// (no writes in flight) still surfaces warn/block transitions.
565-
logDiskSpaceStatus(status);
566-
}
567-
refreshDiskSpaceStatus();
568-
} catch (err) {
569-
logger.debug(`disk-space-guard: /health peek failed: ${err.message}`);
570-
}
546+
// Non-blocking: build the cached disk-space projection (helper handles
547+
// peek + transition log + async refresh + safe-fail). Synchronously
548+
// awaiting statfs here would risk timing out k8s liveness probes when
549+
// the filesystem is slow.
571550
res.status(200).json({
572551
message: 'OK',
573552
timestamp: new Date().toISOString(),
574-
diskSpace,
553+
diskSpace: buildHealthDiskSpacePayload(),
575554
});
576555
});
577556

‎src/routes/v1/index.js‎

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,7 @@
33
import express from 'express';
44
import { getWalletHealthResponse } from '../wallet-health.js';
55
import { getConfig } from '../../utils/config-loader';
6-
import {
7-
peekDiskSpaceStatus,
8-
refreshDiskSpaceStatus,
9-
logDiskSpaceStatus,
10-
} from '../../utils/disk-space.js';
11-
import { logger } from '../../config/logger.js';
6+
import { buildHealthDiskSpacePayload } from '../../utils/disk-space.js';
127
const V1Router = express.Router();
138

149
import {
@@ -25,30 +20,12 @@ import {
2520
} from './resources';
2621

2722
V1Router.get('/health', (req, res) => {
28-
// Non-blocking: use cached disk-space status only and trigger an async
29-
// refresh. Synchronously awaiting statfs would risk timing out k8s
30-
// liveness probes when the filesystem is slow. See bare /health in
31-
// src/middleware.js for full rationale.
32-
let diskSpace = null;
33-
try {
34-
const status = peekDiskSpaceStatus();
35-
if (status) {
36-
diskSpace = {
37-
severity: status.severity,
38-
freeBytes: status.freeBytes,
39-
blockBytes: status.blockBytes,
40-
warnBytes: status.warnBytes,
41-
};
42-
logDiskSpaceStatus(status);
43-
}
44-
refreshDiskSpaceStatus();
45-
} catch (err) {
46-
logger.debug(`disk-space-guard: /v1/health peek failed: ${err.message}`);
47-
}
23+
// Non-blocking: see bare /health in src/middleware.js. The shared
24+
// helper handles peek + transition log + async refresh.
4825
res.status(200).json({
4926
message: 'V1 API is running',
5027
timestamp: new Date().toISOString(),
51-
diskSpace,
28+
diskSpace: buildHealthDiskSpacePayload(),
5229
});
5330
});
5431

‎src/routes/v2/index.js‎

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -28,40 +28,17 @@ import { OrganizationsV2Router } from './resources/organizations-v2.js';
2828
import { AuditV2Router } from './resources/audit-v2.js';
2929
import { OfferV2Router } from './resources/offer-v2.js';
3030
import { FilestoreV2Router } from './resources/filestore-v2.js';
31-
import {
32-
peekDiskSpaceStatus,
33-
refreshDiskSpaceStatus,
34-
logDiskSpaceStatus,
35-
} from '../../utils/disk-space.js';
36-
import { logger } from '../../config/logger.js';
31+
import { buildHealthDiskSpacePayload } from '../../utils/disk-space.js';
3732

3833
const V2Router = express.Router();
3934

4035
V2Router.get('/health', (req, res) => {
41-
// Non-blocking: use cached disk-space status only and trigger an async
42-
// refresh. Synchronously awaiting statfs would risk timing out k8s
43-
// liveness probes when the filesystem is slow. See bare /health in
44-
// src/middleware.js for full rationale.
45-
let diskSpace = null;
46-
try {
47-
const status = peekDiskSpaceStatus();
48-
if (status) {
49-
diskSpace = {
50-
severity: status.severity,
51-
freeBytes: status.freeBytes,
52-
blockBytes: status.blockBytes,
53-
warnBytes: status.warnBytes,
54-
};
55-
logDiskSpaceStatus(status);
56-
}
57-
refreshDiskSpaceStatus();
58-
} catch (err) {
59-
logger.debug(`disk-space-guard: /v2/health peek failed: ${err.message}`);
60-
}
36+
// Non-blocking: see bare /health in src/middleware.js. The shared
37+
// helper handles peek + transition log + async refresh.
6138
res.status(200).json({
6239
message: 'V2 API is running',
6340
timestamp: new Date().toISOString(),
64-
diskSpace,
41+
diskSpace: buildHealthDiskSpacePayload(),
6542
});
6643
});
6744

‎src/utils/disk-space.js‎

Lines changed: 64 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -154,36 +154,31 @@ const computeStatus = async () => {
154154
});
155155
};
156156

157-
/**
158-
* Non-blocking peek at the current disk-space status. Returns the cached
159-
* value if one exists, otherwise null. Never triggers a statfs.
160-
*
161-
* Used by /health endpoints so liveness probes never block on a slow
162-
* filesystem (statfs can hang for seconds on NFS/EBS hiccups, and k8s'
163-
* default livenessProbe.timeoutSeconds is 1 s — a full statfs round trip
164-
* can trigger spurious pod restarts precisely when operators most need
165-
* /health to respond).
166-
*
167-
* The cache is kept fresh by checkDiskSpace() calls from the write-path
168-
* middleware (every 30 s of write traffic) and by the periodic refresh
169-
* driven from refreshDiskSpaceStatus() below. As a fallback, /health
170-
* handlers should also kick off an async refresh (and ignore the
171-
* promise) so a quiet, mostly-idle instance still gets timely updates.
172-
*/
173-
export const peekDiskSpaceStatus = () => {
157+
// Non-blocking peek at the current disk-space status. Returns the cached
158+
// value if one exists, otherwise null. Never triggers a statfs.
159+
//
160+
// Used by buildHealthDiskSpacePayload so /health probes never block on
161+
// a slow filesystem (statfs can hang for seconds on NFS/EBS hiccups,
162+
// and k8s' default livenessProbe.timeoutSeconds is 1 s -- a full statfs
163+
// round trip can trigger spurious pod restarts precisely when /health
164+
// most needs to respond).
165+
//
166+
// The cache is kept fresh by checkDiskSpace() calls from the write-path
167+
// middleware and by the fire-and-forget refresh kicked off from
168+
// buildHealthDiskSpacePayload, so a quiet, mostly-idle instance still
169+
// sees timely updates.
170+
const peekDiskSpaceStatus = () => {
174171
if (testOverride !== null) {
175172
return testOverride;
176173
}
177174
return cachedStatus;
178175
};
179176

180-
/**
181-
* Fire-and-forget refresh of the cached disk-space status. Used by
182-
* /health and similar non-blocking callers that want the cache to stay
183-
* warm without paying statfs latency on the request path. Failures are
184-
* swallowed (computeStatus already logs them).
185-
*/
186-
export const refreshDiskSpaceStatus = () => {
177+
// Fire-and-forget refresh of the cached disk-space status. Used by
178+
// buildHealthDiskSpacePayload to keep the cache warm without paying
179+
// statfs latency on the request path. Failures are swallowed
180+
// (computeStatus already logs them).
181+
const refreshDiskSpaceStatus = () => {
187182
// Reuse the same in-flight de-dup as checkDiskSpace so callers don't
188183
// accidentally schedule overlapping statfs calls.
189184
void checkDiskSpace().catch(() => {});
@@ -280,6 +275,51 @@ export const logDiskSpaceStatus = (status) => {
280275
lastLoggedSeverity = status.severity;
281276
};
282277

278+
// Fields exposed on /health endpoints. Centralising the projection here
279+
// keeps /health, /v1/health, and /v2/health byte-for-byte consistent;
280+
// adding a new field (e.g. `error`, `path`) only requires a change in
281+
// this list, not in three handlers.
282+
const HEALTH_DISK_SPACE_FIELDS = Object.freeze([
283+
'severity',
284+
'freeBytes',
285+
'blockBytes',
286+
'warnBytes',
287+
]);
288+
289+
/**
290+
* Build the standard `diskSpace` payload for /health-style endpoints
291+
* and drive the same side effects every health handler should perform:
292+
* - debounced transition log line (so a mostly-idle instance still
293+
* surfaces warn/block transitions even with no writes in flight)
294+
* - fire-and-forget cache refresh (so the next probe sees up-to-date
295+
* numbers without this request paying statfs latency)
296+
*
297+
* Returns the projection object (or null if the cache hasn't been
298+
* populated yet). Never throws - any internal failure is swallowed
299+
* after a debug log so /health itself stays 200.
300+
*
301+
* Callers must NOT pass the returned object back to a caller that
302+
* could mutate it; the live cache reference is frozen so attempts to
303+
* mutate the projection's shared keys are silent no-ops anyway, but a
304+
* fresh object is returned here for safety.
305+
*/
306+
export const buildHealthDiskSpacePayload = () => {
307+
try {
308+
const status = peekDiskSpaceStatus();
309+
refreshDiskSpaceStatus();
310+
if (!status) return null;
311+
logDiskSpaceStatus(status);
312+
const payload = {};
313+
for (const field of HEALTH_DISK_SPACE_FIELDS) {
314+
payload[field] = status[field] ?? null;
315+
}
316+
return payload;
317+
} catch (err) {
318+
logger.debug(`disk-space-guard: health payload failed: ${err.message}`);
319+
return null;
320+
}
321+
};
322+
283323
/**
284324
* Build the 507 Insufficient Storage response body. Shape mirrors
285325
* READ_ONLY_ERROR in src/utils/read-only-response.js so clients can

0 commit comments

Comments
 (0)