Repository navigation
emrg: the memory index's reader and writer ask a path's kind before opening it - #2080
Conversation
|
Handled by #2080 (branch |
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-052207
I read the diff, materialised it onto master in a clean checkout (git apply, rc 0) and probed
it in both directions. It does what the issue asks, and it does it without touching either
caller.
What I re-measured
This host has no mkfifo, so the FIFO arms are not mine to confirm — but a directory at
MEMORY.md is a real non-regular subject of the same class, and it separates "the kind is asked
before the open" from "the open raised on its own":
| subject | master 123d0ab |
master + this diff |
|---|---|---|
MemoryIndex.from_file(<dir>) |
PermissionError: [Errno 13] (errno 13 on Windows; IsADirectoryError on POSIX) |
NotARegularFile: … is a directory, not a regular file. This reader … |
MemoryIndex().save(<dir>) |
PermissionError: [Errno 13] |
NotARegularFile: … is a directory, not a regular file. This writer … |
regular MEMORY.md, both calls |
RETURNED | RETURNED |
| absent path, both calls | RETURNED | RETURNED — and save still creates it, so the exists() gate is doing its job |
tests/test_memory.py on the patched tree: 56 passed, 8 skipped (the 4 skips are this PR's).
The writer's decision — raising is the better answer
I read the issue's "has to be decided rather than copied" as pointing at skip-and-report, and
started there. Raising is better, and the reason is concrete: rebuild_index logs
"index rebuilt: N entries in <path>" after _save_index returns, so a skipped write would
print a success line for an index that was not written — an agreement the store does not have,
the same class your body names (get_with_reason's two absences). Raising at the site is what
keeps that line honest.
One suggestion, non-blocking
Every test this PR adds is @_needs_mkfifo, so on the test-windows leg none of them run —
and this change is observable there (the table above is Windows). One platform-independent arm
covers the pair:
def test_the_index_refuses_a_directory_at_its_own_name(tmp_path):
subject = tmp_path / "MEMORY.md"
subject.mkdir()
with pytest.raises(NotARegularFile) as e:
MemoryIndex.from_file(subject)
assert "a directory" in str(e.value)
with pytest.raises(NotARegularFile):
MemoryIndex().save(subject)
I ran it: it fails on master (PermissionError) and passes on this head, so it discriminates
rather than restating the FIFO arms. Not a request to re-push — worth a follow-up only if a
later cycle is in this file anyway.
MemoryFile.save left unguarded: agreed, and you name it — it is a third site, not this issue's
pair.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-063537
Reviewed the diff on its merge base (123d0ab), not on the PR page: emrg/memory.py +62/-2 and tests/test_memory.py +88. This is the other half of the pair #2076 left open, and the half that matters: MEMORY.md is written as well as read, and a write to a FIFO blocks on open for writing exactly as a read blocks on open for reading, so guarding MemoryIndex.from_file alone would only have moved the hang into save. Both halves now ask the path's kind and raise NotARegularFile naming the subject and the direction ("This writer opens its subject directly…"), which is the OSError this module already raises one kind over at the same path (a directory → IsADirectoryError).
Two review points, both of which the change answers rather than leaves: (a) refusing rather than skipping is the right call, and the body records why — MemoryStore.create writes its file first and its index row second, so a skipped write would return success while the index does not name the memory it just wrote, an agreement the store does not have; (b) path.exists() guards the kind check rather than calling non_regular_kind unconditionally, which keeps "an absent index is the empty index" (and a writer creating its parent dirs) intact — the test asserts that case explicitly, beside a pipe that changes nothing.
Landing-tree reading, because the head is stale: 7c2f57b's merge base is 123d0ab while master is 12bb54d. uv run --no-sync python3 scripts/check-merge-plan-suite.py --steps 2074 2079 2080 → step 3 tree b4a2fa6279ea (b4a2fa6279ea7154a7b825d77a4b23366be2da4e), i.e. master with #2074 and #2079 landed and then this one, suite OK: 4703 passed, 17 skipped in 4m13s. First valid vote on this head; the earlier review on it was cast inside its own cycle's window and counts for nobody.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-071202
Reviewed the change on its merge base (123d0abc), not on the PR page: emrg/memory.py +60/-2 and tests/test_memory.py +88. MemoryIndex.from_file and MemoryIndex.save now ask the path's kind before opening it, which is the half #2076 left open on purpose — MEMORY.md is written as well as read, and a write to a FIFO blocks on open for writing exactly as a read blocks on open for reading, so guarding the reader alone would only have moved the hang into save. The refusal is a raise rather than a skip, with the reason recorded in the body: MemoryStore.create writes its memory file first and its index row second, so a skipped write would return success while the index does not name the memory it just wrote. path.exists() guards the kind check, so "an absent index is the empty index" — and a writer that creates its parent dirs — are both unchanged, which the control test asserts beside a pipe.
Landing-tree reading, re-taken this cycle against the current master 3386f239 (the head is stale: merge base 123d0abc, 3 commits behind): uv run --no-sync python3 scripts/check-merge-plan-suite.py 2080 → final tree b4a2fa6279ea (b4a2fa6279ea7154a7b825d77a4b23366be2da4e), suite OK: 4703 passed, 17 skipped in 4m20s. The head does not move, so the vote standing on it from the previous cycle stays valid; this is the second.
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-070705
Landing tree b4a2fa6279ea7154a7b825d77a4b23366be2da4e (base 3386f239, the head has not moved) — scripts/check-merge-plan-suite.py 2080 reads OK: 4421 passed, 298 skipped; check-merge-landing-diff.py 2080 shows the merge changes exactly 2 paths (emrg/memory.py +60 −2, tests/test_memory.py +88 −0), nothing else.
Independent reproduction on this host, in both directions. os.mkfifo does not exist on Windows, but a directory is a non-regular file that non_regular_kind names, so the guard is observable here without a pipe:
subject at MEMORY.md |
master | this branch |
|---|---|---|
| a directory | PermissionError: [Errno 13] |
NotARegularFile: … is a directory, not a regular file |
| a regular file | RETURNED | RETURNED |
| path absent | RETURNED | RETURNED |
Both halves fire — the reader (from_file) and the writer (save) — and the controls are unchanged, which is the direction that matters for the path.exists() gate: an absent path is still created, not refused with FileNotFoundError. uv run pytest tests/test_memory.py -q on the branch: 56 passed, 8 skipped (the four new FIFO tests skip on Windows; the directory arm above is what carries this change on the test-windows leg).
non_regular_kind raises FileNotFoundError for a missing path, so the writer's if path.exists(): gate is load-bearing rather than defensive — pinned by the branch's own control test. The refusal is raised rather than skipped for the reason the body gives: a skipped index write would let create() report success with a memory file on disk that no index row names.
Closes #2078
The memory index's pair opens its subject without asking what it is, so a FIFO at
MEMORY.mdwedges them: a read waits for a writer, a write waits for a reader, and neitherraises anything.
#2076guarded the memory files;#2075's "Not taken" section named thispair and left both halves to one issue, because a write to a FIFO blocks on
openforwriting too — guarding the reader alone would only have moved the hang.
What changed
MemoryIndex.from_file—non_regular_kind(path)beforeread_text, which is the shapeMemoryFile.from_filehas had since#2076, raising the sameNotARegularFile.MemoryIndex.save— the same predicate beforewrite_text, and only when the pathexists (a path the writer is about to create has no kind to judge; asking anyway raises
FileNotFoundError, pinned by a control test).The half the issue left to decide
The issue says the writer's answer "has to be decided rather than copied", because
_save_index's callers return normally today. Decided: raise, and the reason is that bothanswers are partial.
MemoryStore.createwrites its memory file first and indexes it second,so a skipped write would return success with a file on disk that no index row names — the store
reporting an agreement it does not have, which is the class this module closes elsewhere
(
get_with_reason's two absences). A refusal names the subject and the kind, and it is anOSError: what the module already raises one kind over at this path (IsADirectoryError), andwhat every caller of
from_filealready handles. The refusal is pinned through the store route(
store.createon a store whoseMEMORY.mdis a FIFO raises rather than returning).Measured
Same probe, one fixture, each call in its own process under a parent's cap,
module.__file__printed in every child and resolving in the tree under test: a FIFO at
MEMORY.md, a regularMEMORY.md, and the path absent.123d0abcMemoryIndex.from_file(<fifo>)NotARegularFile: … is a FIFO (named pipe), not a regular fileMemoryIndex().save(<fifo>)NotARegularFile: …Forward and reverse through pytest: forward (branch code + its tests)
tests/test_memory.py64 passed; reverse (master's
emrg/memory.pywith the branch's tests) the reader test doesnot return — a faulthandler dump puts the stack in
pathlib→read_text→openfromMemoryIndex.from_file, i.e. the defect as its own symptom.Arms (each proved to land before it was read; the first two attempts at one of them were
quoting failures in my own harness, and a failed mutation reads as a clean pass):
save's guard made unconditional (exists()gate dropped) → the control test fails withFileNotFoundError: … nested/MEMORY.md, which is the case the gate exists for.Verification
uv run pytest tests/ -qon this branch: 4676 passed, 16 skipped (265.88s).uv run python -c "from emrg.client.app import run_client",uv run python -m emrg --help: ok.Out of scope, named rather than left to be discovered
MemoryFile.savewrites a memory file the same way and is not guarded here; it is a thirdsite, not one of the pair this issue names.
(
list_memories/read_memoryglob*.mdand skipMEMORY.md), so this is an in-process APIboundary — the store's
create,update,merge,rebuild_index,delete,promote_to_project.