Skip to content

emrg: the sessions index asks its subject's kind before opening it - #2100

Merged
argszero merged 1 commit into
masterfrom
fix/sessions-index-asks-the-kind
Oct 11, 2026
Merged

argszero merged 1 commit into
masterfrom
fix/sessions-index-asks-the-kind

Conversation

@argszero

Copy link
Copy Markdown
Owner

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_text waited at the
open for a writer, and nothing was raised: the except below could not fire and the call
never returned. Measured on master 99477516, one child process per arm under an 8 s cap:

arm subject reading
_load a regular file 0.00 s, {'s1': '/x/s1'}
_load a mkfifo DID NOT RETURN within 8 s
_upsert (the session-save path) a mkfifo DID NOT RETURN within 8 s
rebuild_sessions_index (daemon startup) a mkfifo DID NOT RETURN within 8 s

The subject's kind is now asked with non_regular_kind — the tool layer's path-taking reader this
module already imports for _read_meta_session_id (#2083) — and a non-regular subject takes the
answer 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_title calls through here with no try of its own and records the promise
that 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 of
truth
— rebuild_sessions_index rebuilds it from the session directories on disk.

_write needs no guard, and that is measured rather than assumed: it is mkstemp plus
os.replace, so it renames over the destination instead of opening it — a FIFO destination returns
in 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 reason
emrg/skills/registry.py gives at its own two sites: non_regular_kind answers both halves of the
question the old line got wrong — it names the kind, and raises FileNotFoundError for a missing
path, which is the OSError already 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: UnicodeDecodeError is a ValueError, not an OSError. Measured the same day on the
same master — a b"\xff\xfe\x00\x01…" at the index path raised out of _load, through
upsert_session_index, into the session-save path documented as never raising. It is in the
except tuple 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 two
caller-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 is
FIFO-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 a
named assertion rather than a wedged run.

Both directions measured (mutation arms, file restored and re-verified between them):

  • kind ask removed (the old exists() shape) → 3 failed, each with
    the read did not return within 5.0s — a path was opened without asking its kind first;
  • UnicodeDecodeError removed from the handler → 1 failed, the non-UTF-8 case, raising
    UnicodeDecodeError: '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 ✓
  • Guards: check-undefined-names · check-doc-count · check-citation-resolves ·
    check_nonlocal · check_unbound_reads · check-extension-load — all rc 0.
  • actionlint .github/workflows/*.yml rc 0 (no workflow touched).

CI: pending at submission.

@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-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 tree ebc0dfc8411b, 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) and tests/test_sessions_index.py (+65 −0). The 2 other paths in diff(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_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 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_title calls
    through here with no try of 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: UnicodeDecodeError is a ValueError, 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 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-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 tree 11ca628e7f3a, 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) and tests/test_sessions_index.py (+65 −0). The 7 other paths in diff(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_index and test_a_non_utf8_index_answers_empty.
  • reverse — restoring master's emrg/sessions_index.py while keeping the new tests makes test_a_non_utf8_index_answers_empty fail with UnicodeDecodeError: '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;
  • UnicodeDecodeError joins the handler. It is a ValueError, not an OSError, so a non-UTF-8 index used to escape a reader whose docstring promises it never raises, through upsert_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 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-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.

@argszero
argszero merged commit 60541e3 into master Oct 11, 2026
2 checks passed
argszero pushed a commit that referenced this pull request Oct 11, 2026
Pure refresh: the head was two commits behind, so a reviewer measuring the tree a merge
would produce was reading a stale base. No conflict; nothing in this PR's four paths is
touched by what landed (#2098, #2100).
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 sessions index's own reader opens sessions_index.json without asking its kind — every session save wedges on a FIFO there

2 participants