undo.data can be silently zero-filled by an unclean shutdown: AtomicFile never fsyncs before rename

I did run into issues with corrupt undo-log - twice over the last few weeks, partly due to an overgrown undo-log. The analysis below is AI-generated. I respect that some maintainers may want to close AI-generated issues and pull requests without spending time on explaining why.

---

⚠️ This comment is AI-generated (Claude Opus 5 via Claude Code) on behalf of tobixen

## Symptom

```
$ timew undo
Cannot handle line '<205 NUL bytes>txn:'
```

`undo.data` contains a run of NUL bytes glued directly onto a `txn:` marker, with no
newline between them. Once that is present, `timew undo` is unusable — it aborts on the
bad line rather than skipping it, so the entire undo feature is dead until the file is
repaired by hand.

This has happened to me twice in seven weeks — 2026-06-02 and 2026-07-23 — on
timewarrior 1.9.1, ext4 (`rw,relatime`), Linux 6.x/7.x.

## Why it goes unnoticed for weeks

On the default `journal.size = -1`, `timew undo` is the only command that *parses*
`undo.data`. Every other command appends to it by copying the existing bytes forward
verbatim. So the corruption is faithfully preserved and propagated by normal use, and you
only discover it the next time you happen to need `undo` — for me, 11 days after the fact.

(With `journal.size` set to a positive value this is very different, and worse — see
"Interaction with `journal.size`" below.)

## Root cause

`AtomicFile` is atomic with respect to *signals and interruption*, but not with respect
to *power loss*. In `src/AtomicFile.cpp`, `finalize_all()` closes the temp files and then
renames them into place with signals blocked:

```c++
// Step 1: Close / Flush all the atomic files that may still be open. ...
for (auto& file : impl::atomic_files)
{
file->close ();
}
...
// Step 2: Rename the temp files to the *real* file
sigprocmask (SIG_SETMASK, &new_mask, &old_mask);
for (auto& file : impl::atomic_files)
{
file->finalize ();
}
sigprocmask (SIG_SETMASK, &old_mask, nullptr);
```

There is no `fsync(2)` before the rename, and none after it on the containing directory.
In fact `fsync`, `fdatasync` and `O_SYNC` do not appear anywhere in `src/` (checked on
`develop` @ 8cda469a).

On ext4 with the default `data=ordered`, the rename can reach the journal while the data
blocks behind it have not yet been written back. After a crash you get the new *name*
pointing at blocks that were allocated but never written — which read back as zeroes.
That is precisely the observed damage: a NUL run where the freshly appended tail should
be, followed by whatever the next timew invocation appended on top of it.

## Evidence that it is crash-correlated

For the 2026-07-23 occurrence, the NUL run sits immediately after the transaction for the
interval starting `20260723T051121Z` (05:11:21 UTC = 07:11:21 CEST):

```
$ journalctl --list-boots
...
-1 caee7640dc98... Mon 2026-07-13 13:54:23 CEST Thu 2026-07-23 07:11:56 CEST
0 c54fa8e191bd... Thu 2026-07-23 07:17:56 CEST Mon 2026-08-03 11:29:31 CEST
```

The machine stopped logging 35 seconds after that interval started, and came back six
minutes later. My earlier occurrence lines up with a reboot on 2026-06-02 in the same way.

Reconstructing from the byte count, the 205 zeroed bytes were exactly one transaction —
the surrounding transactions are intact, and the interval data files (`*.data`,
`tags.data`) were not affected in either occurrence. Only `undo.data` is exposed, because
it is the file that is rewritten wholesale on essentially every command.

## A contributing factor: write amplification

`AtomicFile::impl::append()` copies the *entire* real file into the temp file before
appending:

```c++
if (real_file.exists () && ! File::copy (real_file, temp_file))
```

Since `journal.size` defaults to unbounded, `undo.data` grows without limit and so does
this cost:

```c++
// src/Rules.cpp:100
{"journal.size", "-1"},
```

Mine had reached 13271 transactions / 5.4 MiB, meaning every single `timew` invocation
copied 5.4 MiB and renamed it. I drive timewarrior from an ActivityWatch exporter that
syncs continuously, so the process is very often mid-write — which is exactly the window
in which a crash does this damage. Anecdotally, an earlier `undo.data` of mine had
reached 27 MiB.

So the exposure is roughly proportional to journal size, and by default nothing ever
trims the journal.

`journal.size` also appears to be undocumented: grepping `doc/` for `journal` returns only
three lines of `timew-undo.1.adoc`, which describe the journal in general terms but never
mention that it grows without bound or that there is a setting for it. `timew config`
shows an empty `journal` section rather than the effective value, so the setting is not
discoverable from the tool either. As far as I can tell,
[discussion #593](https://github.com/GothenburgBitFactory/timewarrior/discussions/593)
is currently the documentation. (Minor: the answer there describes `journal.size` as "the
number of lines you want to keep", but `Journal::endTransaction()` counts *transactions*.)

## Interaction with `journal.size`

This is the part I find most interesting. With `_size > 1`, every transaction re-reads and
**parses** the whole journal before rewriting it:

```c++
// src/Journal.cpp:104-121
if (_size > 1)
{
std::vector transactions = loadJournal (undo);
...
undo.truncate ();
for (; it != end; ++it)
{
undo.append (it->toString ());
}
}
```

`loadJournal()` runs every line through `TransactionsFactory::parseLine()`, which is where
`Cannot handle line '...'` is thrown (`src/TransactionsFactory.cpp:51`).

So the recommended cure for unbounded growth also converts this latent defect into a hard
stop: with pruning enabled, the very next `timew start` or `timew stop` after the crash
would have thrown, leaving no time tracking at all — from a single corrupt byte, with an
error message that names neither the file at fault nor a remedy.

Failing fast is arguably better than silently carrying corruption forward. But it does
mean a user who follows the advice in #593 trades "undo is broken" for "timewarrior is
broken", and neither failure tells them what to do.

## Suggested fixes

In rough order of value:

1. **`fsync` the temp file before renaming it**, and `fsync` the containing directory
after the renames in `finalize_all()`. This is the actual durability bug. If the
latency is unwelcome for every command, it could be a config setting — but silently
losing the undo journal on a power cut seems like the worse default.

2. **Make journal parsing resilient to damage**, or at least name the offending file and
suggest a remedy in the error. Right now one unparseable line takes out `undo`
entirely — and, with `journal.size` set, every mutating command — even though the
transactions on either side are fine and `undo` only ever needs the *last* one.
Skipping with a warning, or reading backwards from the end, would turn this from
"unusable, repair by hand" into "warns once".

3. **Document `journal.size`**, and consider whether `-1` is the right default. Unbounded
is what makes the copy-per-invocation cost unbounded too, and it widens the window in
which a crash can do this damage.

I am happy to attempt a pull request for (1) and/or (2) if that would be welcome, but I
did not want to send C++ touching durability semantics without maintainer input on the
performance trade-off first.

## Workaround for anyone hitting this

The NUL bytes can simply be deleted — `undo.data` has no legitimate use for a zero byte,
and the result is a valid journal minus the one lost transaction:

```sh
python3 -c 'import sys; d=open(sys.argv[1],"rb").read(); open(sys.argv[1],"wb").write(d.replace(b"\x00",b""))' undo.data
```

(Take a backup first, and make sure no timew process is running.)

---

⚠️ This comment is AI-generated (Claude Opus 5 via Claude Code) on behalf of tobixen