Repository navigation
fix(core): write back no file moved or deleted during a folder pass - #2322
Conversation
The in-place check took a bound file whose render matched the scanned bytes as unchanged without looking at the disk, so a file the pull had placed or revived and the person then moved or deleted was reported unchanged. The check now requires the file to be present. A file gone from a path no scan reaches (the lists stopped taking it, or a nested folder) is written where its item wants it again, since no scan can tell its move from a delete; the scan and the pull share one reach check.
aicayzer
left a comment
There was a problem hiding this comment.
Review: ready to merge with open items
Summary. A bound file that is missing when a pull reaches its item is no longer written back, so a move or delete during a pass is not undone; the one blocking finding is fixed and re-tested, and the deviation from the first acceptance criterion is stated below.
First draft: 3/5. 1 blocking issue found (a file the pull wrote itself and that was then deleted or moved was counted unchanged, contradicting folders/pull-missing-file-unwritten-count), fixed in d62e7a1 with a core test and a device test, each seen failing first.
Checked.
- Done: the acceptance criteria against the tests; the red run reproduced with
main'spull.rsunder the branch's tests; witnesses in every absence test. - True: the new contract statements against the chapter's form and their cited tests; the Folder safeguards, Sync a folder and Keep folders running pages.
- Every path: the still-check, restore, journaled revive, incomplete scan, gone root,
settle_landings,move_boundwith a missing source,departing,take_away, blob reads and rebinding. No other pull path reads an error or absence as unchanged. - Keep what people made: a file moved with an unsent edit keeps and sends the edit.
- Real conditions: the
folder-pass-pullhold is debug-only like the other holds; the new device tests drive scan, drain and pull as explicit steps. - Changed existing test:
folders.test.ts"holds the files of a directory it cannot read" pinned the write-back of a file deleted during an incomplete scan; no statement requires that. - The fixes for the findings (d62e7a1, f16190f) were checked by the session by reading the diff and by the builder's runs; the reviewer did not re-review them.
Found.
- A missing file the pull wrote itself was counted
unchanged. Fixed in d62e7a1: the in-place shortcut now requires the file on disk.True,Report only what is known. - A path no scan reaches got a reason that was not true there. Fixed in d62e7a1: the hold applies only to paths the scan reaches (lists take them, outside any nested folder); elsewhere the item is written where it wants to be, as before. The reason text and
folders/pull-missing-file-not-writtenare narrowed to match.Report only what is known. - The first acceptance criterion is met one pass later. A missing, unjournaled file is never written in the same pass, even when its item changed in content the file shows; it comes back on the next pass, once the scan journals it and still inside the grace. Writing it at once would duplicate a file moved while a remote change arrived. Accepted.
Done.
For an issue.
- A file moved to another folder while a remote change to its item arrives inside the grace can still be revived by the old folder, so the note can appear in both. Existing behavior, rare.
- An unexplained failure of
replace_tests::a_store_another_process_writes_is_refused_as_held_and_never_replaced(witness line) was seen once in a full run and not again in 5 runs; cause not found.
Quick fixes. 1 applied by the reviewer, in withmarfa/docs#592.
Reviewed by withmarfa-reviewer on Claude Fable (first round), fixes checked by the session. This review may contain mistakes.
Summary
A folder pass scans, sends, then pulls. When a person moved, renamed or deleted a bound file between the scan and the pull, the pull wrote the file back at its old path whenever the item's render differed from the bytes the scan read, which is always true for a new note's first
marfa_idwrite-back and for any remote change. The next scan then saw the moved file as a copy and created a second item, or brought a deleted file back. The pull now writes back no bound file that is missing and not journaled; the next scan follows the move or journals the delete, and the existing grace rules decide from there.Closes #2321
Changes
write_placed, the still-check treated a missing file as unchanged,in_placecame out false, andlandwrote the render at the old path.core/marfa-core/src/folder/pull.rs). The still-check now separates three cases: changed since the scan (unchanged behavior), unreadable (now reported with an honest reason instead of the "changed" one), and missing. A missing file that is neither journaled nor marked put back by a restore is countedunwrittenwith the reason "the file is gone since the scan read it, and the next scan tells whether it moved". A file whose render already matches what the scan read is taken as in place only while it is present (a stat, no read), so a file the pull placed or revived itself and the person then moved or deleted is countedunwritten, notunchanged.Folder::reaches).removal.rsexposes itsPUT_BACKsentinel to the pull, so a restore of a paused removal still writes its missing files back.deleted_as_agreed). A new note deleted before the folder first wrotemarfa_id,typeandtierinto it was revived once journaled, because the render's added lines read as a change made elsewhere. Lines the agreed presentation never had are now left out of the comparison, so the journaled delete stands.watch.rs). A debug-onlyfolder-pass-pullhold between a pass's drain and its pull, documented inconformance/README.md.conformance/spec/folders.md). Newfolders/pull-missing-file-not-written(limited to paths the lists take outside a nested folder),folders/pull-missing-file-unwritten-count,folders/pull-missing-file-moved-keeps-itemandfolders/delete-before-first-write; a new test cited underfolders/delete-remote-revives.folders.test.ts › holds the files of a directory it cannot read, and goes on with the restpinned the old behavior: after an incomplete scan, the pull wrote a deleted file back. It now asserts the file stays gone, is countedunwritten, is journaled by the next complete scan and is deleted after the grace. No contract rule required the write-back.Testing
Red first: the new tests were run on
main(889da4d) with only the tests and the hold point added.cargo test -p marfa-core --lib folder::gave 3 failed, 2 passed. The moved, deleted and revived cases failed withPullReport { rewritten: 1, .. }; the two witnesses (a present file changed elsewhere is written in place, and an unreadable file is not written over) passed, as intended.Red first for the review's finding, on this branch at 51dc50c with only the new tests added:
a_file_the_pull_wrote_and_then_gone_since_the_scan_is_unwritten_not_unchangedfailed withSome(None): PullReport { unchanged: 1, unwritten: 0, flagged: [], .. },left: (1, 0),right: (0, 1).counts a file the pull wrote itself and deleted after the scan as unwritten, not unchangedfailed withexpected [ +0, 2 ] to deeply equal [ 1, 1 ].folder/pull.rs):a_file_gone_from_a_path_no_scan_reaches_is_written_where_its_item_wants_itfailed with the reach check removed (unwritten: 1, nothing written ata.md); its witness, the same path still taken by the lists, is held back.Green on this branch:
cargo fmt --all,cargo clippy -p marfa-core --all-targets -- -D warnings: clean.cargo test --workspaceincore/: 1400 passed, 0 failed. One earlier run hadreplace_tests::a_store_another_process_writes_is_refused_as_held_and_never_replacedfail once at its witness (replace_refusedafter the lock is dropped); it passed in 3 runs alone and 2 more full runs. That test is untouched by this change.vitest run --project device src/suites/device/folders src/suites/device/binding-folders: 21 files, 793 tests passed. This run also caught the restore regression in a first version of the fix (7 restore and unreadable tests), fixed by thePUT_BACKexemption above.src/suites/device/folder-*: 48 passed;folder-settings-live.test.tsneeds a live server (MARFA_API_URL) and was not run.spec-form.test.tsandspec-citations.test.ts: 34 passed.pnpm build:pnpm format:checkandpnpm lintclean;pnpm test:changedpassed (88 files, 1144 tests, then 24 files, 238 tests).New tests:
folder/mod.rs): a plain move, a move into a subdirectory and a move with an unsent edit; a delete that stays deleted and reaches the bin after the grace (written and never-written notes); a delete revived by a change it shows once journaled; an unreadable file; a present file changed elsewhere (witness).folder/mod.rs): a file the pull revived, then deleted or moved after the scan, isunwritten; kept in place, it isunchanged(witness).folder/pull.rs): a file gone from a path the lists no longer take is written where its item wants it; the witness with the path still taken holds it back.folders-pass-writeback.test.ts): a plain move, a rename with a title-line edit, a delete, a move with an unsent edit, a burst of two moves and two deletes, a witness that writes a remote change in place and revives a deleted file once journaled, and a revived file deleted after the scan countedunwrittenbeside a file in place countedunchanged.folders-one-mac.test.ts): a move to another folder on the same Mac.binding-folders-clock.test.ts): a move and a delete in a real watch pass, held atfolder-pass-pull, with the manual clock driving the grace.Not covered:
marfa_idmoved to another folder before its first write-back becomes a new item there, as before.Notes for reviewers
settle_landings, the waiting second round,move_bound, restore and rebinding (no binding), blob placements,departing,let_go,take_away,retitle_fileandbehind_by_its_line_alone.Checklist