Skip to content

tools/ci: the /tmp ratchet also guards /dev/shm, the other RAM path the directive names (#2027 follow-up) - #2153

Merged
bong-water-water-bong merged 1 commit into
mainfrom
pkg/2027-devshm
Nov 11, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
pkg/2027-devshm

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #2027 — the follow-up I raised in review, now that the ratchet has merged: it guards /tmp and not /dev/shm, and the directive it exists for names both.

The gap, precisely

The merged pattern is /tmp/… only; /dev/shm has no arm, no baseline line and no mention. So a lane writing a benchmark scratch file to /dev/shm gets a green check — and /dev/shm is the worse of the two: capped (half of RAM by default), always RAM, never reclaimable to disk.

grep -c 'dev/shm' tools/ci/tmp_path_check.py     -> 0

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/x stays out for exactly the reason /var/tmp/x does.

LITERAL = re.compile(r"""(?<![A-Za-z0-9_./-])/(?:tmp|dev/shm)/[^"'\s;|)]*["']?""")

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 — no baseline lines, and none of today's 44 baselined /tmp sites is touched.

Verified

tmp_path_check.py --self-test                     -> ok
  ok   a /dev/shm literal is a site too, and /var/dev/shm is not
tmp_path_check.py --check --root <archive of main> -> rc=0
  tmp_path_check: 44 baselined site(s), none new, none stale (1 file(s) excluded: this checker's own fixtures)

The two new arms are fixture-based in the file's own style: a /dev/shm/buf.bin literal is found, /var/dev/shm/not_ours.bin is 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.txt established. 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.

…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.
@github-actions

Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Correctness of Path Matching

The change updates the regular expression to also match /dev/shm paths, which are considered as problematic as /tmp paths due to their RAM-based nature and lack of reclaimability. The anchor pattern remains unchanged to ensure that paths like /var/dev/shm are correctly excluded, maintaining the intended behavior for both /tmp and /dev/shm path detection.

# A `/tmp` path in C, C++ or shell. The first version of this required a LEADING QUOTE, and a reviewer measured
# what that missed: `echo y > /tmp/x.log` -- a bare path, the plain shell idiom, and arguably the most likely form
# in `scripts/*.sh` -- was invisible, while the same path in quotes was caught. So the leading quote is gone and the
# anchor is "not the tail of another path": `(?<![A-Za-z0-9_./-])` keeps `/var/tmp/x` out (the `a` before `tmp`
# blocks it) and lets `/tmp/x` in whether it follows a space, `>`, `=`, a quote or nothing at all. A trailing quote
# is included when present so the baseline names the literal as a reader finds it.
# engine#2027 review: the directive this ratchet exists for says "never `/tmp` **or `/dev/shm`**", and a pattern that
# guards one of the two reads as "the RAM paths are handled". `/dev/shm` is the same failure mode with a worse ceiling --
# capped, always RAM, never reclaimable -- so it is one alternation, and the anchor is unchanged: the character before
# 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. There are no `/dev/shm` sites on main today, so this lands as a pure guard with no baseline lines.
LITERAL = re.compile(r"""(?<![A-Za-z0-9_./-])/(?:tmp|dev/shm)/[^"'\s;|)]*["']?""")

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 2f83c16 — and the pattern is exactly one alternation with the anchor preserved, which is the hard part.

+LITERAL = re.compile(r"""(?<![A-Za-z0-9_./-])/(?:tmp|dev/shm)/[^"'\s;|)]*["']?""")
+# 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. There are no `/dev/shm` sites on main today, so this lands as a pure guard with no baseline

Three things worth naming:

  • the directive really does name both — "never /tmp or /dev/shm" — and a ratchet covering one of them reads as "the RAM paths are handled", which is worse than no ratchet: it is a claim of coverage;
  • /dev/shm is the same failure with a worse ceiling — /tmp on this box is a 62 GiB tmpfs, /dev/shm is the other RAM path and not capped the same way. The comment says so, which is what makes the guard's existence understandable to someone who has never read the incident;
  • zero baseline — named explicitly, so this lands as a pure guard. That is the ideal state tests: hyperloom_benchmark_test checks GTT only where amdgpu exposes it (onto #2115) #2124 reached for the allow-list: a check with no accepted exceptions.

The gap this pattern still leaves, and it is the one I hit twice today

The ratchet reads string literals in source. Two ways a file can be written to RAM without one:

  1. $TMPDIR pointing at a RAM path — getenv("TMPDIR") has no /tmp/ in it, and on this box TMPDIR=/dev/shm is exactly the thing a lane does to keep a large scratch file "off the disk" (the fconst chain in npu-pack: cache the 1.9 canonical .fconst on disk, not in the temp directory (it is RAM here: 3.6 GB per h4096 pack) #1801/tests: pin the .fconst scratch order and that a missing cache directory is created (#1801's CHANGES ask) #1818 puts $TMPDIR between the cache and /tmp for the opposite reason). A literal-only check cannot see it;
  2. a path built by concatenation — ram_dir + "/" + name.

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 $TMPDIR or assembled at runtime is out of scope". A guard that states its boundary is what #2072 and #2075 were asked for today, and it keeps the next reader from believing "no hits" means "no RAM writes".

Nothing else — seventeen lines, no baseline, both paths, anchor intact.

@bong-water-water-bong
bong-water-water-bong merged commit 5c8a1d5 into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the pkg/2027-devshm branch November 11, 2026 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant