Repository navigation
tools/ci: the /tmp ratchet also guards /dev/shm, the other RAM path the directive names (#2027 follow-up) - #2153
Conversation
…AM path the directive names (engine#2027)
The directive this ratchet exists for says "never `/tmp` **or `/dev/shm`**", and the merged pattern guards one of the two: a
reader -- and the next lane writing a benchmark scratch file -- sees a green check and concludes the RAM paths are handled.
`/dev/shm` is the same failure mode with a worse ceiling: it is capped (half of RAM by default), always RAM, and never
reclaimable to disk. So it is one alternation in the existing pattern, with the anchor unchanged: the lookbehind is about the
character before the leading slash, so `/var/dev/shm/x` stays out for exactly the reason `/var/tmp/x` does.
Measured before writing it: **zero `/dev/shm` sites on main** (`git grep -l 'dev/shm'` outside third_party), so this lands as a
pure guard with no baseline lines -- today's 44 baselined sites are all `/tmp` and none of them is touched.
Verified:
* `--self-test` ok, including two new arms: a `/dev/shm/buf.bin` literal is a site, and `/var/dev/shm/not_ours.bin` is not
(a fixture file, scanned, then unlinked so the baseline round-trip arms stay undisturbed)
* `--check --root <git-archive extract of main>` rc=0, "44 baselined site(s), none new, none stale"
Not done here, deliberately: the 44 baseline lines still carry no per-entry reason. The file's header already says it once
("each is a real test whose runner was never added" -- no, for this file: a site that predates the ratchet), and 44 reasons
would be 44 sentences saying that; the set baseline plus the stale-line report is what keeps it honest, which is the same
argument `add_test_gaps.txt` established.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at 2f83c16
A/B'd it, and the guard bites on the path it was added for:
$ echo 'X = "/dev/shm/ab-probe.bin"' > tools/ci/_ab_shm.py && python3 tools/ci/tmp_path_check.py --check --root .
tmp_path_check: NEW hardcoded /tmp path: tools/ci/_ab_shm.py /dev/shm/ab-probe.bin
tmp_path_check: FAIL (1 new, 0 stale of 44 baselined)
$ (probe removed) -> 44 baselined site(s), none new, none stale
The lead-slash reasoning is the part I checked hardest, because it is where a two-path regex goes wrong: "the leading slash is what must not be part of another path, so /var/dev/shm/x stays out for the same reason /var/tmp/x does." A pattern
matched without that anchor would flag a path that merely contains /dev/shm, and the first lane to hit it would weaken the check.
And the "pure guard, no baseline lines" claim is true: grep -c '^+/dev/shm' on the baseline diff is 0, and main's baseline is 44 sites either way (44 lines on main, 44 reported
by --check at this head) — so this PR adds the arm without grandfathering a single site. That is the difference between a guard and a ratchet that quietly grows.
The reason this arm matters more than the /tmp one, in the PR's own words: /dev/shm is "the same failure mode with a worse ceiling" — it is RAM with no swap behind it at all,
so a stray dump there does not even get the grace of the swap filling up. The 11-10 alert had /tmp at 52 G of 62 G with swap 71/71 full; the same file in /dev/shm would have
taken the box down faster.
One cosmetic mismatch, worth a line and not a change: the failure message still says "/tmp is tmpfs on this box, so route the path through ONEBIT_DUMP_DIR" even when the offending
literal is /dev/shm — visible in the A/B above. The site line above it names the path, so the reader can tell; but a message that names the wrong directory in the sentence after
naming the right one is how someone concludes the check misfired. Worth "/tmp and /dev/shm are RAM on this box" next time the file is touched.
Ran: --check before the probe (clean), with the probe (fails, named), after (clean again). Self-test inherits from #2027 and is unchanged by this PR's +17/−1.
|
VERDICT: READY at Three things worth naming:
The gap this pattern still leaves, and it is the one I hit twice todayThe ratchet reads string literals in source. Two ways a file can be written to RAM without one:
I would not widen the ratchet now — a check that starts guessing at runtime values becomes unreadable. But the documentation of the ratchet should say what it does not cover in one line: "this guards literals; a path routed through Nothing else — seventeen lines, no baseline, both paths, anchor intact. |
Refs #2027— the follow-up I raised in review, now that the ratchet has merged: it guards/tmpand not/dev/shm, and the directive it exists for names both.The gap, precisely
The merged pattern is
/tmp/…only;/dev/shmhas no arm, no baseline line and no mention. So a lane writing a benchmark scratch file to/dev/shmgets a green check — and/dev/shmis the worse of the two: capped (half of RAM by default), always RAM, never reclaimable to disk.The change, and why it is this small
One alternation, with the anchor untouched: the lookbehind is about the character before the leading slash, so
/var/dev/shm/xstays out for exactly the reason/var/tmp/xdoes.Measured before writing it: zero
/dev/shmsites on main (git grep -l 'dev/shm'outsidethird_party), so this lands as a pure guard — no baseline lines, and none of today's 44 baselined/tmpsites is touched.Verified
The two new arms are fixture-based in the file's own style: a
/dev/shm/buf.binliteral is found,/var/dev/shm/not_ours.binis not (the anchor), and the fixture file is scanned then unlinked so the baseline round-trip arms stay undisturbed.What I did not do, deliberately
The 44 baseline lines still have no per-entry reason. I raised that in review and I am not actioning it now: the file's header says the reason once (a site that predates the ratchet), and 44 copies of one sentence is a wall, not a record — the set baseline plus the stale-line report is what keeps it honest, which is the argument
add_test_gaps.txtestablished. If the owner wants per-line reasons for the handful that are dumps rather than device locks or logs (npu_layer.bin,q4nx_hidden_%d.bin,q4nx_forward.cpp), that is a five-line change and worth doing inside whoever owns that file's next touch, not as a separate 44-line sweep.