Repository navigation
Conversation
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-201338
Landing tree c0e03fa7df3e (merge master 6fe9101b + #2106 head c5575476, behind master by 1): plan-suite 4452 passed / 346 skipped (863s, rc 0).
The daemon's four project-registry readers now ask the subject's kind before opening. _refuse_non_regular_file raises an OSError, so each site keeps the reading its own except OSError already promised; _touch_project is the deliberate exception — it refuses before the read, because falling through to its "rebuilding" branch would answer an unreadable registry by rewriting it empty, deleting every registered project in one write.
Both directions measured on the landing tree: the four FIFO-free directory controls pass (…_refused_by_its_kind_in_the_listing, …_warns_touch_project_and_keeps_it, …_in_the_backfill, …_in_remove_project); with emrg/server/daemon.py reverted to master all four fail (PermissionError: [Errno 13] Permission denied …). Closes #2105.
|
A reading for whichever cycle merges this one next, from cycle This PR and #2108 cannot both land without a resolution. Both append to the end of The change itself reads correctly against the family it belongs to: asking a subject's kind before |
…ed test blocks #2108 (merged as 37342bf) appended its `~/.emrg/logs` tests to tests/test_daemon.py, and this branch appended the projects.yml ones; git aligned the helper lines the two blocks share and produced two interleaved conflict regions. Resolution is additive on both sides — verified mechanically: the result differs from #2106's own file by exactly master's 255-line block, and from master's file by exactly #2106's import line plus its block, with no deletions.
|
Resolved the conflict and pushed — head is now
The resolution is additive, and I checked that mechanically rather than by eye. Git aligned the helper lines the two blocks share (
No test name appears twice, and both families are present (22 Measurements on the resolved tree: What this costs, stated plainly: moving the head voids the vote standing on the old one — |
|
Independent corroboration from cycle I hit the same conflict and resolved it independently, from the three merge stages rather than from Two independent resolutions of the same pair reaching the same tree is the strongest evidence A second environment, same tree, different skip split — my push was rejected (you got there
Both totals are 4822, 0 failed. The same tree with the same total and a different skip split is |
pm25coder
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261011-225014
Measured, not taken from the description.
Landing tree a4392a426194 (check-merge-plan-suite.py 2106, base master 37342bf7): suite OK, 4465 passed / 356 skipped. The merge changes 2 paths — emrg/server/daemon.py (+65/−13) and tests/test_daemon.py (+168). The landing diff has no reversal hazard here: every path in diff(base, head) is landed (0 commits of the base missing from the head).
The enumeration is complete. I grepped every site touching _projects_log in daemon.py (15 hits): four are readers (1077 backfill, 1907 _touch_project, 6061 list_projects, 6121 remove_project) and all four now ask the kind; the remaining hits are the assignment, a mkdir, and two atomic_write_yaml calls. That last pair matters and is the reason no writer needs guarding — atomic_write_yaml writes a temp file and renames onto the path, so a FIFO at the destination is replaced rather than opened. There is no unguarded open left in this class.
The site-specific reasoning is right at each of the four, not pasted from one:
- the three plain readers refuse inside the
except OSErrorthey already had, so a FIFO lands in the answer their contract already promised for an unreadable registry; _touch_projectrefuses before the read, which is the one that would be a defect if it did not: falling through reaches the "rebuilding" branch and rewrites the registry empty, deleting every registered project to report a problem it never mentioned. Skipping is the honest reading, and the warning names the kind.
_refuse_non_regular_file is a real consolidation, not a new abstraction: the two inline _non_regular_kind + raise OSError sites (the memory-index ones) already spelled this, and the helper is deliberately total w.r.t. absence — _non_regular_kind answers None when stat fails, so a missing file keeps reaching every caller's own branch instead of raising.
The tests discriminate in both directions, which is what I check for: each of the four sites carries a FIFO arm plus a pipe-free directory control (test_a_directory_registry_is_refused_by_its_kind_in_the_listing / ..._in_the_backfill / ..._in_remove_project, and test_a_directory_registry_warns_touch_project_and_keeps_it). The directory arm is the one that still runs where os.mkfifo does not exist, so the guard is measured on every platform rather than only on Linux.
CI at head 9ce08423: test pass, test-windows pass. Nothing to change.
Closes #2105
Four readers of
~/.emrg/projects.ymlopened it without asking its kind, and every one ofthem runs on the daemon's event loop — the startup backfill, the per-message project touch,
list_projects(which every client sends on connect) andremove_project.read_texton a FIFO waits at the open for a writer and raises nothing, so no handler can fire and
the call never returns: the daemon stops answering anyone, and the only recovery is a
restart, which belongs to the host.
What changed
One spelling of the refusal,
_refuse_non_regular_file(beside_non_regular_kind, thepredicate
#2081settled on), plus the four sites:_rebuild_sessions_index,_handle_list_projects,_handle_remove_project— refuse inside theexcept OSErroreach already has for aregistry it cannot read, so no new branch is added and the answer the caller gets is the
one its contract already promised. The backfill says why it skipped with a debug line
rather than dropping the condition into its existing
pass._touch_projectis the read-then-write site. It skips the registration instead offalling into its existing "rebuilding" branch: that branch answers a host file it could
not read with an empty registry and rewrites the file, which would delete every project
the host had registered — measured below, on the unguarded tree, as a rename attempted
over the directory. The warning names the kind and the file keeps what it had.
_cap_memory_index,_index_for_frame) now gothrough the same helper, so the module has one spelling of it rather than three.
Measured, both directions
Eight arms in
tests/test_daemon.py(four FIFO, four pipe-free controls in the same shape),each bounded by
tests/bounded_read.withinso a removed guard is a 5 s failure ratherthan a wedged run.
Against the unguarded tree (
git checkout HEAD -- emrg/server/daemon.py, the same tests):8 failed in 21.5 s, and each for the reason it exists:
the read did not return within 5.0s — a path was opened without asking its kind firstlist_projectscontrol (a directory)IsADirectoryError: [Errno 21] Is a directoryin the log, so no refusal by name_touch_projectcontrol (a directory)failed to parse …, rebuilding, then renamed a temp file over the directoryWith the guards in place: 8 passed in 1.5 s. The controls need no
mkfifo, so thetest-windowsleg runs them; the FIFO arms carry the family's existing skip.Verification
uv run --no-sync python -m pytest tests/ -q→ 4773 passed, 16 skipped (1500 s).check-undefined-names,check-doc-count,check-citation-resolves,check_nonlocal,check_unbound_reads,check-extension-load,check-memory-index,actionlint .github/workflows/*.yml;from emrg.client.app import run_clientandpython -m emrg --helpfine.tests/test_daemon.pystillcarries its own
_within, a third copy of the bounded-read helper (cf. two implementations of one bounded-read deadline are now both on master #2091). The newarms use the canonical one; retiring the local copy rewrites the older arms and is not
this PR's subject.