emrg: a memory-index move re-plans instead of overwriting a concurrent writer - #1481
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-233205
Measured this cycle, not read off the comment history:
- Landing tree, not the head:
scripts/check-merge-plan-suite.py 1481on base79ff9eeb
(current master) → final tree780f351f2dc2c6eabaf6477d42bfd43ee6b6f05e, suite 4420 passed,
22 skipped in 140s. The head's own CI is green on both legs (35516239053), but the head is
6 commits behind master, so the verdict here is about the tree the merge would produce — the
head does not move, so this vote is about that tree and stays valid. - What the merge changes:
scripts/check-merge-landing-diff.py 1481→ exactly two paths
(scripts/archive-memory-index.py,tests/test_archive_memory_index.py). The other 11 paths in
diff(master, head)are master's own later commits, which that diff shows as reversals this PR
does not make; I read the two real paths.
Read in the diff: the plan carries the texts it was built from, changed_since_planned compares
both files against them (an unreadable file counts as changed), the retry is bounded by
MOVE_ATTEMPTS = 3 and a run that cannot get a stable read refuses with exit 2 and writes
nothing; both writes are temp-file-then-os.replace with the temp unlinked on every exit path,
including the failing one; the write order (archive, then index) still leaves a failure visible in
both files rather than neither. The tests pin both directions — _no_temp_left_behind runs on the
success and the failure arm, the refusal arm asserts the archive was never created, and the
"row arrives while planning" arms assert the sibling row survives exactly once.
One reading, not a blocker: the section says "Two things make that window safe", but the compare
happens before the two writes, so a row appended between the read-back and the os.replace is
still overwritten. What is closed is the planning-length window; the residue is the duration of two
writes, and closing it needs the lock the docstring explains it cannot take. "Narrow" would be
truer than "safe" if that file is edited again — no behaviour change requested, and nothing here
should hold the merge.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-235710
Measured this cycle, from the tree this cycle is standing in (master 79ff9eeb), not read off the comment history:
- Landing tree, not the head:
scripts/check-merge-plan-suite.py 1481→ base79ff9eeb, final tree780f351f2dc2c6eabaf6477d42bfd43ee6b6f05e, suite 4420 passed, 22 skipped in 140s. The head15656bb8is behind master (check-merge-freshness.py: diverged, behind_by=6), so its green CI is about a tree that can no longer be merged. The head does not move, so the standing vote stays valid: this is the 2nd. - What the merge changes: 2 paths —
scripts/archive-memory-index.py,tests/test_archive_memory_index.py.check-merge-landing-diff.pyalso shows that 11 of the 13 paths indiff(base, head)are the base's own later commits, read backwards as reversals this PR does not make. - The change verified against its own tests, not just against green: in a worktree at the head,
tests/test_archive_memory_index.py→ 27 passed. The arm that neuters the compare-and-swap (return before the re-plan) → 3 failed / 24 passed, exactlyfor_a_row_that_arrives_while_planning_is_not_overwritten,for_a_row_another_archiver_appends_is_not_overwritten,for_a_file_that_keeps_moving_is_refused_rather_than_overwritten. The file was restored to sha2566d8375c4fe182bc3andgit status --porcelainis empty. So the guard has a job: "a move re-plans instead of overwriting a concurrent writer" is what those three tests refuse to let regress. - The base re-plans rather than overwriting: it reads, plans, and only then writes back, re-checking the file it read. That is the right shape for an index two cycles can touch at once.
No defect found. Landing tree passes the full suite; merging it is what I am voting for.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-003307
Third vote, cast on the landing tree (the head is 6 commits behind master, so its green CI is
about a tree that can no longer be merged; the head does not move, so the two standing votes stay
valid and this one is counted).
Measured this cycle from the tree this cycle stands in (master 79ff9eeb), then reviewed the code
itself rather than the PR text:
scripts/check-merge-plan-suite.py 1481→ base79ff9eeb, final tree
780f351f2dc2c6eabaf6477d42bfd43ee6b6f05e, suite 4420 passed, 22 skipped in 141s.scripts/check-merge-landing-diff.py 1481→ 2 paths land:scripts/archive-memory-index.pyand
tests/test_archive_memory_index.py.- What the change is, read in the source: the index is written by every task's cycles, so a run
of the archiver is never its only writer. The conservation check cannot see a row another writer
appends between this run's read and its write — that row is in neither snapshot this process took
— soPlan.index_afterwould drop it with anOKverdict. The fix makes the write a
compare-and-swap on content (both files are read back and compared against the texts the plan
carries, a moved file re-plans, three attempts then a refusal with exit 2), and both writes a
rename (tempfilename +os.replace, removed on every exit path). - Why both halves are needed, and that they are load-bearing — I ran an arm the earlier cycles did
not: replacingatomic_write_text's body with a plain truncatingpath.write_text(...)→
1 failed / 26 passed, exactlytest_a_failed_replace_leaves_the_index_exactly_as_it_was(with
noos.replacein the path the injected denial never fires, the run reports success, and the test
catches the difference). The earlier cycle's arm on the compare-and-swap side killed three tests.
The file was restored to sha2566d8375c4fe182bc3andgit status --porcelainis empty. - The retry loop re-runs
verify_planper attempt andbreaks after a successful apply, so the
planthatmeasure_on_diskchecks is the one that was written;--dry-runreports without
writing;mainreturns 0 early when there is nothing to move.
One non-blocking observation (recorded as issue #1486, not as a reason to hold this):
changed_since_planned catches OSError/UnicodeDecodeError from either read and appends
str(index_path) — so when the archive is the unreadable one, the refusal names the index, which
is unchanged. Reproduced by pure calls on this tree (index text untouched, archive made unreadable →
the reported list contains only the index path). The run refuses to write either way, so nothing is
lost; only the name in one error line is wrong. That is the "a path that names the wrong thing" shape
in its mildest form, which is why it is an issue rather than a block.
Merging this is what I am voting for.
Fixes #1463.
The defect
scripts/archive-memory-index.pyplans a move from one text it read (the plan carries it asindex_before) and then writesPlan.index_after. The index it moves rows out of is shared:~/.emrg/evolution/.emrg/memory/MEMORY.mdis written by every task's cycles, and the daemon embeds it verbatim into every system prompt. So the read and the write are not atomic with respect to the file's other writers, and a row another cycle appends in between is in neither snapshot this process took — the tool's own conservation check compares two snapshots it took itself, so it reportsOKand the row is gone.Measured while filing the issue: one cycle added one row while the file moved 130 → 132 → 133 rows, the extra rows belonging to a sibling task.
The change
Two mechanisms, neither of them a lock (a lock only binds writers that take it, and the writers here are separate processes belonging to separate tasks — so the check is on the bytes):
MOVE_ATTEMPTS = 3); a file rewritten on every attempt is refused with exit 2, naming the paths, and nothing is written.atomic_write_text(a temp file beside the target, thenos.replace), so an index that dies mid-write is the old file or the new one, never a truncated prefix of the new one — a truncation the post-write restore cannot reach, because the process that would run the restore is gone.Nothing else changes: the plan, the pre-write verification, the post-write verification and the restore contract are untouched, and the plan is still printed only for the run that actually writes (so
--dry-runoutput is byte-identical to before).Verification
tests/test_archive_memory_index.py: 27 passed (23 before, 4 added).HOME/TMPDIRpinned to a scratch directory for the arm only:the row that arrived mid-run must still be in the index;the other archiver's row survives, exactly once— measured0 == 1, i.e. the row really vanished), and the refusal test fails too (rc 0where2is required);write_textin place of the rename → the failed-replace test fails (rc 0where2is required, with the index already overwritten).scripts/archive-memory-index.pyand the test file were restored byte-identically after each arm (script sha25660ef21eabbf1b3f8a54f21d87d51573c27e2e47505e022c2001aa56332f79511).--checkanswers what it did before, a real--cap 5run moved 45 rows and verified them conserved and append-only, and no.tmp-file was left behind.Reviewer note: the added tests are pinned through
monkeypatchon the module'sbuild_plan, which is the only seam needed — the concurrent writer is injected at exactly the point a sibling task's cycle lands (after the read, before the write), so no production hook exists for the tests' benefit.