Journal: creating a log entry can overwrite a concurrent live-editor save (data loss)

kayg commented

2026-09-25 16:28:37 +00:00

Owner

Found by the log-drag job (#94). POST /api/v1/notes/journal/log reads the Daily note, appends the new Log line and writes the file with a plain write instead of a checked replace (compare-and-swap on the etag/hash, like replace_if used elsewhere). If the live editor (collab room flush) or a sync upload saves the same Daily note between that read and write, the other change is silently lost.

Fix: use the same checked-replace path as the other Journal writers (read → modify → replace_if(old) → on conflict re-read and retry a bounded number of times); if the Note has a live room, route the append through the room like other external edits so open editors see it. Tests: a deterministic race test (inject a write between read and replace) proving both changes survive; an adversarial storm of concurrent creates + live edits on one Daily note; entry count and every other line preserved.

Found by the log-drag job (#94). POST /api/v1/notes/journal/log reads the Daily note, appends the new Log line and writes the file with a plain write instead of a checked replace (compare-and-swap on the etag/hash, like replace_if used elsewhere). If the live editor (collab room flush) or a sync upload saves the same Daily note between that read and write, the other change is silently lost. Fix: use the same checked-replace path as the other Journal writers (read → modify → replace_if(old) → on conflict re-read and retry a bounded number of times); if the Note has a live room, route the append through the room like other external edits so open editors see it. Tests: a deterministic race test (inject a write between read and replace) proving both changes survive; an adversarial storm of concurrent creates + live edits on one Daily note; entry count and every other line preserved.

kayg commented

2026-09-25 19:19:49 +00:00

Author

Owner

Fixed on branch job/journal-cas. It is not merged or deployed.

Root cause. POST /journal/log read the Daily note, appended the Log line and wrote the file with a plain write. The Notes user lock keeps other Notes writers out (the live room saves through collab_write, which holds it), but a sync upload, a Files PUT or WebDAV do not take that lock. A write between the read and the write was lost.

Fix.

store::update / update_with is the one read-modify-write path: edit the text that was read, write with replace_if on those bytes (create-new when there was no file), and on a lost race (412, or 409 when the file appeared) read and edit again, at most 8 times.
Live rooms: a room on a Daily note damaged it on its first save. The editor's Markdown writer escapes [tz=UTC] as \[tz=UTC\], so each zoned Log entry lost its zone and got a changed title. The web app never opens one (DESIGN §31 K5), but the server allowed it and so did agent turns. The hub now answers 409 for a Daily note, so no editor can hold a copy that it writes back later. The Journal API is the only writer.

Writers audited:

Plain writes, now checked: POST /journal/log (the bug, and its create of a new day), and the Note rename/move replay. The replay wrote each referrer (for example a Daily note that links the Note) with bytes planned earlier. Now it writes the planned bytes only over the planned base, or else it rewrites the links on the current text. The new path uses create-new.
Checked but with no retry, which gave a 412 on a lost race, now retried: CalDAV PUT create (the 412 read as "UID exists"), CalDAV PUT same-day (also used by PATCH /journal/entries and the Calendar link writers), CalDAV DELETE, ensure_ids, POST /journal/entries/{id}/note, and the line fix.
Correct already: collab_write, PUT /{id}/body, PATCH /{id}/properties (client If-Match), the Task writers (tasks_store::replace_if), the cross-day move_dav intent and link_note_to_calendar_event.
Found by the new storm and fixed:
- A Files replace upload checked If-Match against the Files Index and then installed with a plain replace. It now uses Root::write_checked on the actual bytes. This lost a 201 Log entry.
- GET /files/download answered 500 "content hash unavailable" and sent a truncated body while the file changed. It now opens the file first and gets one revision, or answers a retryable 409.
- PATCH wrote child lines from a stale read. It now keeps the children that are in the file.
- The first version of update overflowed a Tokio worker stack in a Files Note rename. It is fixed by boxing the write futures.
Follow-up, not fixed: link_calendar_event_to_log still builds child lines from its own read. The ETag covers only the Log line, so a child line added concurrently by sync could be dropped.

Tests.

Race tests inject a write between the read and the replace (store::race hook): log append, log create of a new day, CalDAV create, PATCH, retitle referrer. The two log tests fail on the old code: the injected line is gone.
calternal-collab/tests/journal_race.rs runs the race with the hub live. The Daily-note room is refused.
tests/adversarial round 2 journalrace: 40 concurrent creates, PATCH moves, conditional sync uploads and live-room opens on one Daily note.

Last adversarial run:

Round 1: FINDINGS 1 (SLOW only).
Round 2: FINDINGS 2, two PATCH requests that timed out at 30 s under host load 30–45. The file kept all 46 entries and all 10 upload lines.
The restart probe showed a 1-byte change: a leading blank line was removed by the room's first save of an ordinary Note, which came after the probe's 4 s settle under load. The previous run had 0 restart findings.

Commits: a13cafa, a6ccfd1, c5609c6, fa56bd0, 37971d2, 67f96af.

Fixed on branch `job/journal-cas`. It is not merged or deployed. **Root cause.** `POST /journal/log` read the Daily note, appended the Log line and wrote the file with a plain write. The Notes user lock keeps other Notes writers out (the live room saves through `collab_write`, which holds it), but a sync upload, a Files PUT or WebDAV do not take that lock. A write between the read and the write was lost. **Fix.** - `store::update` / `update_with` is the one read-modify-write path: edit the text that was read, write with `replace_if` on those bytes (create-new when there was no file), and on a lost race (412, or 409 when the file appeared) read and edit again, at most 8 times. - Live rooms: a room on a Daily note damaged it on its first save. The editor's Markdown writer escapes `[tz=UTC]` as `\[tz=UTC\]`, so each zoned Log entry lost its zone and got a changed title. The web app never opens one (DESIGN §31 K5), but the server allowed it and so did agent turns. The hub now answers 409 for a Daily note, so no editor can hold a copy that it writes back later. The Journal API is the only writer. **Writers audited:** - Plain writes, now checked: `POST /journal/log` (the bug, and its create of a new day), and the Note rename/move replay. The replay wrote each referrer (for example a Daily note that links the Note) with bytes planned earlier. Now it writes the planned bytes only over the planned base, or else it rewrites the links on the current text. The new path uses create-new. - Checked but with no retry, which gave a 412 on a lost race, now retried: CalDAV PUT create (the 412 read as "UID exists"), CalDAV PUT same-day (also used by PATCH `/journal/entries` and the Calendar link writers), CalDAV DELETE, `ensure_ids`, `POST /journal/entries/{id}/note`, and the line fix. - Correct already: `collab_write`, `PUT /{id}/body`, `PATCH /{id}/properties` (client If-Match), the Task writers (`tasks_store::replace_if`), the cross-day `move_dav` intent and `link_note_to_calendar_event`. - Found by the new storm and fixed: - A Files replace upload checked If-Match against the Files Index and then installed with a plain replace. It now uses `Root::write_checked` on the actual bytes. This lost a 201 Log entry. - `GET /files/download` answered 500 "content hash unavailable" and sent a truncated body while the file changed. It now opens the file first and gets one revision, or answers a retryable 409. - PATCH wrote child lines from a stale read. It now keeps the children that are in the file. - The first version of `update` overflowed a Tokio worker stack in a Files Note rename. It is fixed by boxing the write futures. - Follow-up, not fixed: `link_calendar_event_to_log` still builds child lines from its own read. The ETag covers only the Log line, so a child line added concurrently by sync could be dropped. **Tests.** - Race tests inject a write between the read and the replace (`store::race` hook): log append, log create of a new day, CalDAV create, PATCH, retitle referrer. The two log tests fail on the old code: the injected line is gone. - `calternal-collab/tests/journal_race.rs` runs the race with the hub live. The Daily-note room is refused. - `tests/adversarial` round 2 `journalrace`: 40 concurrent creates, PATCH moves, conditional sync uploads and live-room opens on one Daily note. **Last adversarial run:** - Round 1: FINDINGS 1 (SLOW only). - Round 2: FINDINGS 2, two PATCH requests that timed out at 30 s under host load 30–45. The file kept all 46 entries and all 10 upload lines. - The restart probe showed a 1-byte change: a leading blank line was removed by the room's first save of an ordinary Note, which came after the probe's 4 s settle under load. The previous run had 0 restart findings. Commits: a13cafa, a6ccfd1, c5609c6, fa56bd0, 37971d2, 67f96af.

kayg referenced this issue

2026-09-25 19:20:38 +00:00

Journal: link_calendar_event_to_log can drop a concurrently added child line #115

kayg referenced this issue from a commit

2026-09-25 20:24:21 +00:00

fix(notes): Journal writes are checked replaces that retry on a lost race (#98)

kayg referenced this issue from a commit

2026-09-25 20:24:21 +00:00

fix(collab): no live room on a Daily note (#98)

kayg referenced this issue from a commit

2026-09-25 20:24:21 +00:00

fix(notes): box the checked-write futures so a Files Note move does not overflow the stack (#98)