Skip to content

emrg: the memory index's reader and writer ask a path's kind before opening it - #2080

Merged
pm25coder merged 1 commit into
masterfrom
fix/the-index-reader-and-writer-ask-the-kind
Oct 10, 2026
Merged

pm25coder merged 1 commit into
masterfrom
fix/the-index-reader-and-writer-ask-the-kind

Conversation

@argszero

Copy link
Copy Markdown
Owner

Closes #2078

The memory index's pair opens its subject without asking what it is, so a FIFO at
MEMORY.md wedges them: a read waits for a writer, a write waits for a reader, and neither
raises anything. #2076 guarded the memory files; #2075's "Not taken" section named this
pair and left both halves to one issue, because a write to a FIFO blocks on open for
writing too
— guarding the reader alone would only have moved the hang.

What changed

  • MemoryIndex.from_file — non_regular_kind(path) before read_text, which is the shape
    MemoryFile.from_file has had since #2076, raising the same NotARegularFile.
  • MemoryIndex.save — the same predicate before write_text, and only when the path
    exists (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 both
answers are partial. MemoryStore.create writes 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 an
OSError: what the module already raises one kind over at this path (IsADirectoryError), and
what every caller of from_file already handles. The refusal is pinned through the store route
(store.create on a store whose MEMORY.md is 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 regular
MEMORY.md, and the path absent.

call master 123d0abc this branch
MemoryIndex.from_file(<fifo>) DID NOT RETURN within 10s NotARegularFile: … is a FIFO (named pipe), not a regular file
MemoryIndex().save(<fifo>) DID NOT RETURN within 10s NotARegularFile: …
both, regular file RETURNED RETURNED
both, absent path RETURNED RETURNED

Forward and reverse through pytest: forward (branch code + its tests) tests/test_memory.py
64 passed; reverse (master's emrg/memory.py with the branch's tests) the reader test does
not return — a faulthandler dump puts the stack in pathlib → read_text → open from
MemoryIndex.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):

  • writer guard removed → the writer test does not return (bounded at 25 s);
  • save's guard made unconditional (exists() gate dropped) → the control test fails with
    FileNotFoundError: … nested/MEMORY.md, which is the case the gate exists for.

Verification

  • uv run pytest tests/ -q on 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.save writes a memory file the same way and is not guarded here; it is a third
    site, not one of the pair this issue names.
  • Reachability is weak and stated as such in the issue: no daemon frame reads the index today
    (list_memories/read_memory glob *.md and skip MEMORY.md), so this is an in-process API
    boundary — the store's create, update, merge, rebuild_index, delete,
    promote_to_project.

@argszero

Copy link
Copy Markdown
Owner Author

Handled by #2080 (branch fix/the-index-reader-and-writer-ask-the-kind, commit 7c2f57b9). Reader and writer both ask the kind now; the writer's answer is a refusal rather than a skipped write, with the reason in the PR body.

@pm25coder pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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 pm25coder left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

✅ 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.

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.

the memory index's reader and writer open a path of any kind: a FIFO beside the memories wedges MemoryIndex

2 participants