Skip to content

Commit f34e371

Browse files
committed
test(node): cover the RSS readers, including the Linux one that never ran
`process_rss_bytes` had no test at all, and its Linux branch had never executed: `/proc/self/status` parsing is `cfg`-compiled out on the macOS development host, so it would have run for the first time in production — on the one number the UTXO memory attribution divides against. A wrong denominator there silently misprices every encoding decision made from it. Splits the two parsers out of the platform `cfg` so both are compiled and tested everywhere, and covers them with the kernel's actual format: kibibytes with variable leading whitespace, `VmHWM`/`VmSize` sitting adjacent with the same prefix shape, the field absent entirely for a kernel thread, and non-numeric or overflowing values. Adds an end-to-end assertion too, because a parser test cannot catch a reader that returns a plausible constant: allocate 128 MB, touch every page so it is resident rather than merely reserved, and require the reading to move by at least half of it. `allow(dead_code)` rather than `expect`, deliberately: each parser is live in one platform's library build and dead in the other's, but *both* are live in every test build, so an `expect` would itself be unfulfilled half the time.
1 parent e0616c8 commit f34e371

1 file changed

Lines changed: 139 additions & 14 deletions

File tree

crates/node/src/metrics.rs

Lines changed: 139 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -181,16 +181,7 @@ impl HistogramFn for HistogramHandle {
181181
pub fn process_rss_bytes() -> Option<u64> {
182182
#[cfg(target_os = "linux")]
183183
{
184-
let status = std::fs::read_to_string("/proc/self/status").ok()?;
185-
status.lines().find_map(|line| {
186-
let value = line.strip_prefix("VmRSS:")?;
187-
value
188-
.split_whitespace()
189-
.next()?
190-
.parse::<u64>()
191-
.ok()?
192-
.checked_mul(1024)
193-
})
184+
parse_proc_status_rss(&std::fs::read_to_string("/proc/self/status").ok()?)
194185
}
195186
#[cfg(not(target_os = "linux"))]
196187
{
@@ -200,13 +191,46 @@ pub fn process_rss_bytes() -> Option<u64> {
200191
.args(["-o", "rss=", "-p", &std::process::id().to_string()])
201192
.output()
202193
.ok()?;
203-
String::from_utf8(output.stdout)
204-
.ok()?
205-
.trim()
194+
parse_ps_rss(&String::from_utf8(output.stdout).ok()?)
195+
}
196+
}
197+
198+
/// Extracts `VmRSS` from `/proc/{pid}/status` content, in bytes.
199+
///
200+
/// Split out from the read so it is testable off Linux. Without this the parser
201+
/// is `cfg`-compiled out of every run on a macOS development host and first
202+
/// executes in production, on the one path whose output the memory attribution
203+
/// divides by.
204+
///
205+
/// The kernel reports `VmRSS:` in kibibytes with variable leading whitespace,
206+
/// and the field is absent for a kernel thread.
207+
// Unreachable off Linux, and deliberately still compiled there: the point of
208+
// splitting it out is that its tests run everywhere, which they cannot do if
209+
// the function is `cfg`-ed away with its caller.
210+
#[cfg_attr(
211+
not(target_os = "linux"),
212+
allow(dead_code, reason = "tested on every host, called only on Linux")
213+
)]
214+
fn parse_proc_status_rss(status: &str) -> Option<u64> {
215+
status.lines().find_map(|line| {
216+
line.strip_prefix("VmRSS:")?
217+
.split_whitespace()
218+
.next()?
206219
.parse::<u64>()
207220
.ok()?
208221
.checked_mul(1024)
209-
}
222+
})
223+
}
224+
225+
/// Extracts the kibibyte count `ps -o rss=` prints, in bytes.
226+
// Mirror of the note on `parse_proc_status_rss`: unreachable on Linux, still
227+
// compiled and still tested there.
228+
#[cfg_attr(
229+
target_os = "linux",
230+
allow(dead_code, reason = "tested on every host, called only off Linux")
231+
)]
232+
fn parse_ps_rss(output: &str) -> Option<u64> {
233+
output.trim().parse::<u64>().ok()?.checked_mul(1024)
210234
}
211235

212236
/// Installs in-memory process metrics and returns its handle when configured.
@@ -258,6 +282,107 @@ mod tests {
258282

259283
use super::*;
260284

285+
/// Real `/proc/{pid}/status` content, so the Linux parser is exercised on
286+
/// every host rather than only wherever CI happens to run Linux.
287+
///
288+
/// The field is kibibytes with variable leading whitespace, sits between
289+
/// other `Vm*` keys that share its prefix shape, and is absent for a kernel
290+
/// thread.
291+
#[test]
292+
fn proc_status_rss_is_parsed_from_the_kernel_format() {
293+
const STATUS: &str = "\
294+
Name:\tbitcoin-rs
295+
Umask:\t0022
296+
State:\tS (sleeping)
297+
VmPeak:\t14680064 kB
298+
VmSize:\t14680064 kB
299+
VmLck:\t 0 kB
300+
VmHWM:\t 3019751 kB
301+
VmRSS:\t 2949952 kB
302+
RssAnon:\t 2900000 kB
303+
Threads:\t17
304+
";
305+
assert_eq!(
306+
super::parse_proc_status_rss(STATUS),
307+
Some(2_949_952 * 1024),
308+
"VmRSS must be read in kibibytes and returned in bytes"
309+
);
310+
311+
// `VmHWM` and `VmSize` share the prefix shape and must not be taken.
312+
assert_ne!(super::parse_proc_status_rss(STATUS), Some(3_019_751 * 1024));
313+
314+
// A kernel thread has no `VmRSS` at all.
315+
assert_eq!(
316+
super::parse_proc_status_rss("Name:\tkthreadd\nThreads:\t1\n"),
317+
None
318+
);
319+
assert_eq!(super::parse_proc_status_rss(""), None);
320+
assert_eq!(
321+
super::parse_proc_status_rss("VmRSS:\tnot-a-number kB"),
322+
None
323+
);
324+
assert_eq!(super::parse_proc_status_rss("VmRSS:\t"), None);
325+
}
326+
327+
#[test]
328+
fn ps_rss_output_is_parsed_in_kibibytes() {
329+
assert_eq!(super::parse_ps_rss(" 2949952\n"), Some(2_949_952 * 1024));
330+
assert_eq!(super::parse_ps_rss(""), None);
331+
assert_eq!(super::parse_ps_rss(" "), None);
332+
assert_eq!(super::parse_ps_rss("garbage"), None);
333+
// A value large enough to overflow the kibibyte conversion.
334+
assert_eq!(super::parse_ps_rss(&u64::MAX.to_string()), None);
335+
}
336+
337+
/// The reading must track real resident memory, not merely return a number.
338+
///
339+
/// Asserting only `is_some()` would pass for a stub returning a constant,
340+
/// and this figure is what the memory-attribution reporting divides the
341+
/// UTXO set against — a wrong denominator silently misprices every
342+
/// encoding decision made from it. So the test allocates, touches every
343+
/// page to make it resident rather than merely reserved, and requires the
344+
/// reading to move.
345+
///
346+
/// It is also the only coverage the Linux branch has: `/proc/self/status`
347+
/// is `cfg`-compiled out on the development host, so this parser runs for
348+
/// the first time wherever CI runs Linux.
349+
#[test]
350+
fn process_rss_bytes_tracks_a_real_allocation() {
351+
const BALLAST_BYTES: u64 = 128 << 20;
352+
const PAGE: usize = 4096;
353+
354+
let Some(before) = process_rss_bytes() else {
355+
// A platform with neither `/proc` nor `ps` is a legitimate `None`.
356+
return;
357+
};
358+
assert!(
359+
before > (1 << 20),
360+
"implausibly small RSS before allocating: {before} bytes"
361+
);
362+
363+
let mut ballast = vec![0_u8; usize::try_from(BALLAST_BYTES).unwrap_or(0)];
364+
for page in ballast.chunks_mut(PAGE) {
365+
if let Some(first) = page.first_mut() {
366+
*first = 1;
367+
}
368+
}
369+
370+
// `unwrap_or(0)` rather than `expect`: a `None` here fails the
371+
// assertion below with the reading it produced, which says more than a
372+
// panic message would.
373+
let after = process_rss_bytes().unwrap_or(0);
374+
assert!(
375+
after >= before + (BALLAST_BYTES / 2),
376+
"RSS did not track a {BALLAST_BYTES}-byte resident allocation: {before} -> {after}"
377+
);
378+
379+
// Keep the ballast alive across the second reading.
380+
assert_eq!(
381+
u64::try_from(std::hint::black_box(&ballast).len()).unwrap_or(0),
382+
BALLAST_BYTES
383+
);
384+
}
385+
261386
#[test]
262387
fn install_metrics_returns_error_when_global_recorder_install_fails() {
263388
let bind = SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), 0);

0 commit comments

Comments
 (0)