Materialize inbox messages atomically (tmp + rename) - #8
Materialize inbox messages atomically (tmp + rename)#8schickling-assistant wants to merge 1 commit into
Conversation
fs::write truncates in place under the final <unix-ms>-<rand6>.md name, so a concurrent list_dir can observe the file at zero length under a name that already matches is_message_filename. parse_message needs a closing --- to read frontmatter, so that reads back as a message with no from/subject — the "(no subject) (from unknown)" DING. Write to a tmp sibling and rename, the same idiom status writes already use. The tmp name deliberately does not match is_message_filename, so a reader scanning the inbox mid-write skips it. Regression test: a reader tight-looping list_dir against 40 large sends, each into a fresh inbox so nothing is ever removed. 11485 partial reads before, 0 after.
|
Thank you for the concrete report, reproduction, implementation, and regression. The atomic temporary-sibling + rename contribution from PR head The landing head also completes the independent reader-side gap noted during review: a canonical entry removed between Verification is green under the message concurrency/disappearance regressions, full all-target/all-feature tests, strict clippy, local Nix flake check, and exact-SHA GitHub Nix run 30208624180. Closing this PR without merging its branch because the attributed implementation plus the required companion fix are now on |
Fixes #3.
send_to_inboxwrites straight to the final<unix-ms>-<rand6>.mdname.fs::writeisopen(O_WRONLY|O_CREAT|O_TRUNC)thenwrite, so between those two syscalls the file exists at zero length under a name that already matchesis_message_filename. A concurrentlist_dirreads it back throughparse_message, which needs a closing---before it will read any frontmatter — so a zero-length read yieldsfrom: None, subject: None, which renders as the(no subject) (from unknown)DING.This writes to a tmp sibling and renames, which is the idiom
status::write_atomicalready uses in this tree (src/status.rs:143-154) and whichsrc/status.rs:7documents as deliberate — "Writes are atomic (tmp + rename) so a concurrent reader never sees a partial file." The tmp name deliberately does not matchis_message_filename, so a reader scanning the inbox mid-write skips it rather than parsing it.Regression test (
tests/message.rs): a reader thread tight-loopslist_dirwhile the writer sends 40 large messages, each into a fresh inbox so nothing is ever removed and the test isolates the write window. On4e25f56it reports 11485 messages with nofrom; with this change, 0.The symptom was also observed organically on our host — a delivered poke that read
(no subject) (from unknown)for a message whose file on disk was complete — under concurrent load, intermittently, and never on an idle host. So this is not justified only by reading the code.A second, independent finding while writing this test. My first version of the test deleted each message after sending, and it still failed after this fix — because
list_dirdoesfs::read_to_string(entry.path()).unwrap_or_default()(src/message.rs:223), so a file that disappears betweenread_dirand the read is surfaced as an empty message rather than skipped.archive_msgrenames a message out of the inbox, so that window is reachable in normal operation and produces the identical symptom. It is a separate root cause from this one, so it is not in this PR — I have commented on #3 with the detail and can send it separately if you want it.You are actively rewriting this tree and may well have this already; entirely happy for this to be closed in favour of your version, or for you to take the idea and not the diff.
Note:
cargo test --test codex_hooksalready fails on4e25f56in my environment (5 tests, missing shims) — unrelated to this change and equally red on a pristine checkout. Everything else is green.