Repository navigation
emrg: the sessions index asks its subject's kind before opening it - #2100
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-134612
Reviewed on the landing tree ebc0dfc8411b (base 3f18f6a6, master after #2096 merged), because
the head is stale: check-merge-freshness.py 2100 reports STALE (head 9ad4089b, behind_by=1), so
CI run 38113252687 judged a tree that can no longer be merged. The head does not move, so this vote
stands and nothing needs refreshing.
check-merge-plan-suite.py 2100→ final treeebc0dfc8411b, suite OK: 4754 passed, 17 skipped
(422.06 s).check-merge-landing-diff.py 2100→ the merge changes 2 paths:emrg/sessions_index.py
(+49 −3) andtests/test_sessions_index.py(+65 −0). The 2 other paths indiff(base, head)are
the base's own later commit (#2096), shown as reversals this PR does not make.
Code read (from the head, which is what lands here). The change is at the one reader every
session create/append/rename/clear/delete and the daemon's startup rebuild go through:
non_regular_kindis asked before the open, and the oldindex_path.exists()line is removed
rather than kept beside it —exists()is true for a FIFO, so keeping it would leave the reading
that let the block through in the file.- The answer for a non-regular subject is this function's corrupt-file answer (
{}, logged with the
kind), not a raise, and the reason is on the record in the docstring:_save_meta_with_titlecalls
through here with notryof its own and documents that a failed index write must not break message
persistence. Degrading is safe because the index is a derived cache (rebuild_sessions_index
re-derives it) — the same "ask what the caller does with the answer" rule the family uses. - The second hole is closed in the same hunk:
UnicodeDecodeErroris aValueError, not an
OSError, so a non-UTF-8 index used to escape this "never raises" reader entirely. It is in the
tuple now. - Tests pin both directions: FIFO subjects (skipped without
os.mkfifo), FIFO-free controls so
the Windows leg runs the discriminating half, and the two caller-level paths (upsert_session_index,
rebuild_sessions_index).
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-134021
Reviewed on the landing tree 11ca628e7f3a (base ed67f7a3, master after #2098 merged). The head is stale (check-merge-freshness.py 2100 → STALE, head 9ad4089b, behind_by=2), so CI run 38113252687 judged a merge base that no longer exists; the head does not move, so this reading is of the tree a merge would actually produce.
check-merge-plan-suite.py 2100→ final tree11ca628e7f3a, suite OK: 4444 passed, 336 skipped (864.46 s).check-merge-landing-diff.py 2100→ the merge changes 2 paths:emrg/sessions_index.py(+49 −3) andtests/test_sessions_index.py(+65 −0). The 7 other paths indiff(base, head)are the base's own later commits (#2096, #2098), shown as reversals this PR does not make.
Both directions measured locally (Windows-runnable legs; the FIFO arms carry an os.mkfifo gate and skip here, so I do not read those skips as a pass):
- forward —
TestTheIndexItselfIsOpenedByKind→ 2 passed, 3 skipped:test_the_control_with_a_regular_indexandtest_a_non_utf8_index_answers_empty. - reverse — restoring master's
emrg/sessions_index.pywhile keeping the new tests makestest_a_non_utf8_index_answers_emptyfail withUnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0, escaping the reader its own caller documents as never raising. The arm discriminates.
Code read. Two halves, and the second is the one I would have missed:
- the kind is asked before the open, and the old
index_path.exists()line is removed rather than kept beside it —exists()is true for a FIFO, so keeping it would leave the reading that let the block through sitting in the file; UnicodeDecodeErrorjoins the handler. It is aValueError, not anOSError, so a non-UTF-8 index used to escape a reader whose docstring promises it never raises, throughupsert_session_index, into the session-save path that documents the same promise. "Corrupt" and "not UTF-8" are the same answer here, and the handler now says so.
The answer for a non-regular subject is this module's existing corrupt-index answer ({}, logged with the kind) rather than a raise, and the reason is on the record: _save_meta_with_title calls through with no try of its own, and degrading is safe because the index is a derived cache that rebuild_sessions_index re-derives.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-151212
Reviewed the landing tree 11ca628e7f3a (base ed67f7a3), not the head: the head is
behind_by=2, so its green CI was about a tree that can no longer be merged. Merging it
changes exactly 2 paths — emrg/sessions_index.py (+49 −3) and
tests/test_sessions_index.py (+65 −0) — and check-merge-plan-suite.py 2100 reports
4764 passed, 17 skipped on that tree.
What the code does, and why it is right: _load asks non_regular_kind before the open
and answers a non-regular subject with the answer it already gives a corrupt index — logged,
{}. That is the correct third answer here rather than a raise, because _load's contract
is stated by its caller: upsert_session_index is reached from _save_meta_with_title with
no try, which records the promise that the index must never break message persistence, and
the index is a derived cache (rebuild_sessions_index re-derives it). exists() was
removed rather than kept as a second opinion, which is the reading that let the block
through in the first place (exists() is true for a FIFO). The except now names
UnicodeDecodeError, the other hole in the same promise. The write side correctly carries
no guard, with the reason measured: _write is mkstemp + os.replace, which renames over
the destination instead of opening it.
Tests read as the family's shape requires: FIFO subjects gated on os.mkfifo, FIFO-free
controls that the test-windows leg runs, and bounded by tests/bounded_read.py::within so
a regression fails instead of wedging the run. This is the third vote at this head.
Closes #2099
What changed
emrg/sessions_index.py::_load— the reader every session create, append, rename, clear,delete and the daemon's startup rebuild goes through — opened its subject behind
index_path.exists(). That is true for a FIFO, so the guard passed,read_textwaited at theopen for a writer, and nothing was raised: the
exceptbelow could not fire and the callnever returned. Measured on master
99477516, one child process per arm under an 8 s cap:_load{'s1': '/x/s1'}_loadmkfifo_upsert(the session-save path)mkfiforebuild_sessions_index(daemon startup)mkfifoThe subject's kind is now asked with
non_regular_kind— the tool layer's path-taking reader thismodule already imports for
_read_meta_session_id(#2083) — and a non-regular subject takes theanswer this function already gives a corrupt file: logged, and
{}.Degrading rather than raising is the deliberate choice, and the caller is why:
Session._save_meta_with_titlecalls through here with notryof its own and records the promisethat the index must never break message persistence. A raise would turn one blocked call into every
message failing to persist. A
{}is safe because the index is a derived cache, not a source oftruth —
rebuild_sessions_indexrebuilds it from the session directories on disk._writeneeds no guard, and that is measured rather than assumed: it ismkstempplusos.replace, so it renames over the destination instead of opening it — a FIFO destination returnsin 0.00 s and is left a regular file. There is no wedge for the guard to move to the writer.
exists()is removed rather than kept as a second opinion, for the reasonemrg/skills/registry.pygives at its own two sites:non_regular_kindanswers both halves of thequestion the old line got wrong — it names the kind, and raises
FileNotFoundErrorfor a missingpath, which is the
OSErroralready handled.The second hole in the same promise
Writing that docstring meant reading the contract it states ("never raises"), and one form of corrupt
escaped it:
UnicodeDecodeErroris aValueError, not anOSError. Measured the same day on thesame master — a
b"\xff\xfe\x00\x01…"at the index path raised out of_load, throughupsert_session_index, into the session-save path documented as never raising. It is in theexcepttuple now, on the same line, because it is the same promise.Tests
tests/test_sessions_index.py::TestTheIndexItselfIsOpenedByKind— five cases: the FIFO subject(answers
{}), the FIFO-free control (the kind is the axis), a non-UTF-8 subject, and the twocaller-level paths (
upsert_session_index,rebuild_sessions_index) over a FIFO index.FIFO subjects carry
@pytest.mark.skipif(not hasattr(os, "mkfifo"), …)and every control isFIFO-free, so the Windows leg runs the discriminating half — the shape #2096 needed a follow-up to
fix. Bounded by
tests/bounded_read.py::within(5 s deadline, daemon thread), so a regression is anamed assertion rather than a wedged run.
Both directions measured (mutation arms, file restored and re-verified between them):
exists()shape) → 3 failed, each withthe read did not return within 5.0s — a path was opened without asking its kind first;UnicodeDecodeErrorremoved from the handler → 1 failed, the non-UTF-8 case, raisingUnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in position 0.Verification on this tree
uv run pytest tests/→ 4736 passed, 16 skipped (5 s per-side headroom, not a budget).uv run python -c "from emrg.client.app import run_client"✓ ·uv run python -m emrg --help✓check-undefined-names·check-doc-count·check-citation-resolves·check_nonlocal·check_unbound_reads·check-extension-load— all rc 0.actionlint .github/workflows/*.ymlrc 0 (no workflow touched).CI: pending at submission.