Skip to content

fix(ingest): finish the non-regular-file guards left out of 3.7.1 (#2221) - #2244

Merged
igorls merged 1 commit into
MemPalace:developfrom
mvalentsev:fix/non-regular-guard-followup
Aug 15, 2026
Merged

fix(ingest): finish the non-regular-file guards left out of 3.7.1 (#2221)#2244
igorls merged 1 commit into
MemPalace:developfrom
mvalentsev:fix/non-regular-guard-followup

Conversation

@mvalentsev

@mvalentsev mvalentsev commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

The #2221 fix reached develop through #2228 at an earlier revision of the branch, so three of its guards and their tests did not come with it. #2221 is still open. This PR carries the remainder — 6 files, +179/−10 — on top of the 3.7.1 code now on develop.

All three are covered: copy this branch's tests/test_non_regular_file_guards.py onto a pristine 906b918a and its five new tests fail 5/5 while the file's other 38 pass. Reverting each fix individually in place also kills exactly its own test, one for one.

  • sweep books a failed stat as a failure again. The gate that landed probes the type with f.stat() inside a try, and its except OSError printed SKIP and continued. A dangling symlink, a symlink loop and a file unlinked between rglob and the gate all raise there. Before the gate existed every one of them reached sweep(), raised, and was appended to failures; afterwards "could not read this transcript" became a silent skip and a successful exit. A probe that fails is an error, not a benign file type — it is logged, printed as WARNING, and booked in failures. A probe that succeeds and reports a non-regular file still skips silently, unchanged.
  • mempalace init no longer tracebacks on a directory it cannot enter. In _parse_gradle_root_project_name the is_file() gate sat in front of the try whose except OSError the parser already had, so a manifest under a directory with r but no x raised PermissionError out of a call that used to answer "no manifest name". The gate moves inside that try. _collect_manifest_names stats through os.path.isfile, which reports rather than raises, matching the parsers it guards. (Path.is_file() propagates EACCES on 3.9 through 3.13, all checked — EACCES is not in pathlib's _IGNORED_ERRNOS — and stops raising only on 3.14, where these tests would go quiet rather than fail.)
  • split no longer blocks on a FIFO at its own output name — nor writes through a broken link. The type gate in main() covers the files the glob listed, but split_file builds its output names itself, so a pre-existing named pipe at one of them wedged write_text in the kernel waiting for a reader. The gate asks os.path.lexists, not os.path.exists: exists() follows the link, so a dangling symlink at an output name reads as "nothing there" and the write goes through it, creating the target — a chunk landing wherever the link points instead of in the output directory. The two calls differ on that one case and agree on every other (regular file, symlink to a file, missing name, FIFO, symlink to FIFO, directory). Output names that are anything but a regular file are skipped with a SKIP line.

Two smaller items:

  • test_gather_origin_samples_survives_an_unreadable_directory broke under root rather than passing vacuously: CAP_DAC_OVERRIDE walks into the 0o444 directory, the walled-off file stays readable, and the count assertion sees two samples instead of one. It now carries the same needs_unprivileged_posix gate as the three new permission tests.
  • Comment-only correction in miner._read_text_no_follow: F_SETLEASE on a FIFO fails EINVAL, not ENXIO (measured on Linux 6.18 / glibc 2.39). The code branches on EAGAIN and is unaffected.

How to test

# the five new tests plus the pre-existing one that broke under root
uv run pytest tests/test_non_regular_file_guards.py -v

# the modules this touches
uv run pytest tests/test_non_regular_file_guards.py tests/test_sweeper.py \
              tests/test_miner.py tests/test_split_mega_files.py -q

uv run ruff check .
uv run ruff format --check .

The three permission tests skip as root (geteuid() == 0), where CAP_DAC_OVERRIDE makes directory mode bits gate nothing — the state they need cannot be built there, so without the skip they fail rather than pass. Run them unprivileged. (unshare -r uv run pytest tests/test_non_regular_file_guards.py -q reproduces the root path: 40 passed, 3 skipped.)

To see the sweep regression, point it at a directory holding a broken symlink named like a transcript:

mkdir -p /tmp/sweepdemo && ln -s /nonexistent-target /tmp/sweepdemo/broken.jsonl
mempalace sweep /tmp/sweepdemo; echo "exit=$?"

On develop @ 906b918a the unreadable entry is reported as a skip and the command succeeds:

  SKIP: broken.jsonl (stat error: No such file or directory)
  Swept 0/1 files from /tmp/sweepdemo: +0 new, 0 already present, 0 skipped (< cursor).
exit=0

Before the gate existed — 9a3afc9d, the merge base of the #2223 branch — the same directory (plus a symlink loop) reported WARNING: sweep failed on ... for each entry, 2 file(s) failed to sweep, and exit=2. With this PR that accounting is back:

sweeper: stat failed on /tmp/sweepdemo/broken.jsonl: [Errno 2] No such file or directory: '/tmp/sweepdemo/broken.jsonl'
  WARNING: stat failed on /tmp/sweepdemo/broken.jsonl: [Errno 2] No such file or directory: '/tmp/sweepdemo/broken.jsonl'
  WARNING: 1 file(s) failed to sweep - see stderr / logs for details.
  Swept 0/1 files from /tmp/sweepdemo: +0 new, 0 already present, 0 skipped (< cursor).
exit=2

Full suite locally: 4300 passed, 31 skipped, 0 failed. One caveat for anyone running it on a loaded machine: tests/test_chroma_collection_lock.py, which this PR does not touch, flakes there — its helper re-imports mempalace.backends.chroma (and so chromadb) in a spawned child, and the parent waits for the child's ready flag for only 500 × 0.01 s before asserting "holder failed to acquire lock". A pristine 906b918a flakes the same file under load; both trees pass it 5/5 when the machine is quiet.

Checklist

  • Tests pass (python -m pytest tests/ -v) — 4300 passed, 31 skipped, 0 failed
  • No hardcoded paths
  • Linter passes (ruff check ., ruff 0.16.1 as pinned in CI; ruff format --check . clean)

Refs

@mvalentsev
mvalentsev marked this pull request as ready for review August 13, 2026 13:51
…mPalace#2221)

The MemPalace#2221 fix reached develop through MemPalace#2228 at an earlier revision of the
branch, so three guards and their tests did not come with it. One of the
three is a regression the gate that did land introduced.

sweep_directory: the new gate probes the file type with f.stat() inside a
try, and its except OSError printed SKIP and continued. A dangling symlink,
a symlink loop and a file unlinked between rglob and the gate all raise
there. Before the gate existed each of them reached sweep(), raised, and was
appended to failures — so the gate turned "could not read this transcript"
into a silent skip and a successful exit. A probe that FAILS is an error,
not a benign file type: log it, print WARNING, book it in failures. A probe
that succeeds and reports a non-regular file still skips silently.

_parse_gradle: the is_file() gate sat in front of the try whose except
OSError the parser already had, so a manifest under a directory with r but
no x raised PermissionError out of a call that used to answer "no manifest
name". The gate moves inside that try. _collect_manifest_names stats with
os.path.isfile, which reports instead of raising, matching the parsers it
guards.

split_file: the type gate in main() covers the files the glob listed, but
split_file builds its output names itself, so a pre-existing FIFO at one of
them wedged write_text in the kernel waiting for a reader. Output names that
are anything but a regular file are skipped.

That gate asks os.path.lexists, not os.path.exists. exists() follows the
link, so a DANGLING symlink at an output name reads as "nothing there" and
the write goes through it, creating the target — a chunk landing wherever
the link points rather than in the output directory. Measured: the two calls
differ on that one case and agree on every other (regular file, symlink to a
file, missing name, FIFO, symlink to FIFO, directory).

test_gather_origin_samples_survives_an_unreadable_directory broke under root
rather than passing vacuously: CAP_DAC_OVERRIDE walks into the 0o444
directory, the walled-off file stays readable, and the count assertion sees
two samples instead of one. It now carries the same needs_unprivileged_posix
gate as the three new permission tests.

miner._read_text_no_follow: comment fix only. F_SETLEASE on a FIFO fails
EINVAL, not ENXIO — measured on Linux 6.18 / glibc 2.39. The code branches
on EAGAIN and is unaffected.
@mvalentsev
mvalentsev force-pushed the fix/non-regular-guard-followup branch from 93c47bb to ba5a539 Compare August 13, 2026 14:50
@igorls
igorls merged commit 06cb698 into MemPalace:develop Aug 15, 2026
9 checks passed
0xdhx added a commit to 0xdhx/mempalace that referenced this pull request Aug 16, 2026
`import_palace` opened every `*.jsonl` glob hit with a plain `open()`. That
is safe for the cases the surrounding comment named — a directory, bad
permissions, bad UTF-8 — because each raises, and the `except OSError` books
it as malformed. A FIFO does neither: opening one for reading parks in the
kernel until a writer appears, so a named pipe carrying a `.jsonl` name
wedges the whole import and no handler can see it.

Adopt the guard the ingest side already uses (`miner._read_text_no_follow`,
MemPalace#2221/MemPalace#2244): `O_NONBLOCK` makes the `S_ISREG` check reachable, `O_NOFOLLOW`
keeps a symlinked entry from reading through to a target outside the import
tree — matching the `_reject_symlink` posture the exporter applies on the
write side — and the EAGAIN arm preserves the blocking read for a regular
file whose write lease we broke. A non-regular entry is booked exactly like
an unreadable one, so a partially hostile export still imports what survives.

The test bounds the regression with SIGALRM, since a regression does not
raise — it hangs. The alarm handler deliberately raises a non-OSError: the
first version used TimeoutError, which IS an OSError subclass, so the
importer's own handler swallowed it and the test passed against unfixed code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mempalace mine hangs forever on a named pipe in the mined directory (sweep, init, compress and split too)

2 participants