message send writes non-atomically: a reader can observe the inbox file at zero length

[schickling-assistant]

st2 message send writes the message file non-atomically, so a reader can observe it at zero length. The symptom is a delivered DING that reads (no subject) (from unknown) for a message whose file on disk is complete.

Verified at 4e25f56. Apologies in advance if this is already fixed in work you have locally — happy for this to be closed in favour of your version.

Mechanism

send_to_inbox writes straight to the final name:

• src/message.rs:200 — fs::write(&path, &contents)? into <inbox>/<unix-ms>-<rand6>.md

fs::write is open(O_WRONLY|O_CREAT|O_TRUNC) followed by write, so between those two syscalls the file exists at zero length under its final, is_message_filename-matching name. Any reader that scans the inbox in that window sees an empty file.

The reader then degrades silently rather than skipping or retrying:

• src/message.rs:223 — let contents = fs::read_to_string(entry.path()).unwrap_or_default();
• src/message.rs:146-151 — parse_message only reads frontmatter when it finds an opening --- and a closing ---. A zero-length (or truncated-before-the-closing-delimiter) read therefore yields from: None, subject: None.

None/None is exactly the (no subject) (from unknown) rendering.

Note that src/status.rs:143-154 already establishes the atomic idiom in this codebase (write_atomic: tmp sibling + rename), and src/status.rs:7 documents it as deliberate — "Writes are atomic (tmp + rename) so a concurrent reader never sees a partial file." The inbox write does not use it.

Reproduction

Against a scratch catalog with one agent, at 4e25f56:

# writer: 40 sends of a 40 MB body, clearing the inbox between each
for i in $(seq 1 40); do
  st2 message send --catalog "$CATALOG" --as tester --host <host> <host>.<id> \
      --subject "big-$i" < big.txt >/dev/null
  rm -f "$INBOX"/*.md
done

# reader, concurrently: tight loop
while ...; do
  st2 message ls --catalog "$CATALOG" --host <host> <host>.<id> --json
done

Result: 4 partial reads in 589 reader iterations.

[{"filename":"<ms>-<rand6>.md","ts":<ms>,"from":null,"subject":null,"inReplyTo":null,"tags":[],"priority":null}]

The large body only widens the window; it is not required. We have also seen the (no subject) (from unknown) DING organically under concurrent load with ordinary message sizes, and not on an idle host — so it is intermittent and load-dependent, not size-dependent.

Impact for an adopter

A message can be delivered to an agent with its sender and subject erased. There is no error and nothing in any log; the file on disk is complete by the time anyone looks, so it is indistinguishable from a malformed send. For a supervisor/worker topology that routes on from, a poke that loses its sender is unactionable.

Offer

The fix looks small — reuse the existing write_atomic idiom (tmp sibling in the same directory, then rename) inside send_to_inbox. We have a PR with a regression test ready and will link it here. Entirely your call whether the reader half (unwrap_or_default() swallowing an unreadable message) should also change; we left it alone since an atomic write removes the window that makes it observable.

[schickling-assistant]

Correcting something I wrote above. I said I left the reader half (unwrap_or_default() at src/message.rs:223) alone "since an atomic write removes the window that makes it observable." That is wrong, and my own regression test proved it.

The first version of that test deleted each message after sending. With the atomic write in place it still failed — 17 messages read back with no from — because the surviving window is a different one:

list_dir enumerates with read_dir and then reads each entry. If the file disappears between those two steps, fs::read_to_string(...).unwrap_or_default() turns the NotFound into an empty string, and parse_message turns an empty string into a message with from: None, subject: None. So a vanished message is surfaced as a phantom one with no sender and no subject — the same (no subject) (from unknown) rendering as the write race.

That window is reachable in ordinary operation: archive_msg renames a message out of the inbox, so any concurrent st2 message ls, thread walk, or ding inbox scan can hit it. It does not need the write race at all.

So there are two independent root causes with one symptom:

1. the non-atomic write (fixed in Materialize inbox messages atomically (tmp + rename) #8),
2. an unreadable or vanished entry being materialized as an empty message rather than skipped.

I have not sent a PR for (2) because the right behaviour is a judgement call I would rather you made: a file that vanished mid-scan is benign and should almost certainly be skipped, but a file that is genuinely unreadable (permissions, I/O) arguably should not be silently dropped either — list_dir's own doc says non-message files are skipped, which suggests skipping, but silently losing a real message is its own hazard. If you tell me which way you want it, I will send it with a test.

Measurements for both, at 4e25f56: write race, 4 partial reads in 589 reader iterations against 40 large sends via the CLI, and 11485 in the in-process test; vanish race, 17 after the atomic fix.

[myobie]

Resolved on main in dae49296dca03ea5bc0a00482b1fac36aaf0b4c2.

• send_to_inbox now writes a non-canonical temporary sibling and atomically renames the complete bytes into the canonical inbox name, so a concurrent reader cannot parse a created/truncated message.
• list_dir now treats an entry that disappears between read_dir and read_to_string as a vanished entry to skip, not an empty unknown message; other read errors remain errors.
• Regressions cover the concurrent publication window and the disappearance outcome.

PR #8’s atomic-publication contribution is attributed in the landing commit (Co-authored-by: schickling-assistant); the independently identified read_dir-to-read race is included in the same final head.

Verification: full all-target/all-feature tests, strict clippy, local Nix flake check, and exact-SHA GitHub Nix run 30208624180, passed.