Note: This issue was created by an automated, scheduled Claude code-review check. Please verify before acting on it.
Summary
Commit a68ee18 (fix(cli): reset follow offset on log rotation in dora logs --local --follow, #2800) closed #2785 as completed. It genuinely fixes the common rotation case, but it implements only the weakest of the three remedies that #2785 itself proposed, and two of the failure scenarios documented in #2785 remain reproducible. The new doc comment on follow_read_offset now claims a guarantee the code does not actually provide.
What a68ee18 actually does
binaries/cli/src/command/logs.rs:345
fn follow_read_offset(pos: u64, current_size: u64) -> Option<u64> {
let start = if current_size < pos { 0 } else { pos };
if current_size <= start { None } else { Some(start) }
}
This resets to offset 0 only while the freshly-rotated file is still smaller than the stale pos. That covers the typical case (a 200 ms poll usually catches the new file well below max_log_size ≈ pos), which is a real improvement over the old current_size <= pos { continue } that dropped everything.
Residual gap 1 — the exact scenario the new doc comment claims to prevent
The doc comment at binaries/cli/src/command/logs.rs:339-344 states:
Seeking to the stale pos in the fresh file would silently drop its leading bytes and garble the remainder once it grows past pos, so we resume from offset 0 instead.
But the detection is current_size < pos. If a rotation happens and the fresh log_<node>.jsonl grows past pos within a single 200 ms poll interval, then on the next poll current_size >= pos, so follow_read_offset returns Some(pos) and read_appended_log_lines seeks to the stale offset in the new file — dropping its leading bytes and starting mid-line. This is precisely the "grows past pos" case the comment says is handled; it is not.
- Reachability: requires the node to write ~
pos (≈ max_log_size) bytes within one 200 ms poll. Narrow with a large max_log_size (e.g. the documented "100MB" example), but plausible for a chatty node configured with a small max_log_size (e.g. "1MB" at 5 MB/s). Low severity, but real.
Residual gap 2 — the just-rotated .1.jsonl tail is never followed
files is captured once before the poll loop (logs.rs:275-278) and the loop iterates that fixed list (logs.rs:315), never re-globbing. When the daemon rotates (binaries/daemon/src/log.rs:43-72), it renames the current log_<node>.jsonl → log_<node>.1.jsonl and creates a fresh current file. Any bytes written to the old file between the follow loop's last read (pos) and the rename now live only in the newly-created .1.jsonl, which was not in files at startup and is never picked up. Those lines (up to ~one poll interval of logs) are lost on every rotation.
This is the same gap #2785 explicitly called out: "The rotated content (now in .1.jsonl) is never followed either — that path was never in the files list." It was not addressed by a68ee18.
Concrete failing scenario (gap 1)
- Node configured with a small
max_log_size and logging at high rate; dora logs --local --follow running, pos = 1_000_000.
- Daemon rotates: renames current →
.1.jsonl, creates fresh log_<node>.jsonl at 0, keeps writing.
- Within the next 200 ms the fresh file exceeds 1_000_000 bytes.
- Next poll:
current_size (>1_000_000) >= pos (1_000_000) → follow_read_offset returns Some(1_000_000) → seek to offset 1_000_000 in the new file → leading ~1 MB dropped, output resumes mid-line (garbled).
Suggested fix direction
Track the file's identity, not just its size:
- On Unix, record
dev+ino (std::os::unix::fs::MetadataExt) alongside the offset; when it changes, treat the file as rotated and reset pos = 0 regardless of size. On Windows, fall back to creation-time or a size-decrease heuristic.
- Re-glob the log directory each poll (or on a detected rotation) so the freshly-created
.1.jsonl is picked up and its unread pos..EOF tail is drained before switching to the new current file.
Either change closes both residual gaps and lets the follow_read_offset doc comment's guarantee hold. At minimum, the doc comment should be corrected to reflect that only the size-shrink case is detected.
Scope / notes
Summary
Commit a68ee18 (
fix(cli): reset follow offset on log rotation in dora logs --local --follow, #2800) closed #2785 as completed. It genuinely fixes the common rotation case, but it implements only the weakest of the three remedies that #2785 itself proposed, and two of the failure scenarios documented in #2785 remain reproducible. The new doc comment onfollow_read_offsetnow claims a guarantee the code does not actually provide.What a68ee18 actually does
binaries/cli/src/command/logs.rs:345This resets to offset
0only while the freshly-rotated file is still smaller than the stalepos. That covers the typical case (a 200 ms poll usually catches the new file well belowmax_log_size ≈ pos), which is a real improvement over the oldcurrent_size <= pos { continue }that dropped everything.Residual gap 1 — the exact scenario the new doc comment claims to prevent
The doc comment at
binaries/cli/src/command/logs.rs:339-344states:But the detection is
current_size < pos. If a rotation happens and the freshlog_<node>.jsonlgrows pastposwithin a single 200 ms poll interval, then on the next pollcurrent_size >= pos, sofollow_read_offsetreturnsSome(pos)andread_appended_log_linesseeks to the stale offset in the new file — dropping its leading bytes and starting mid-line. This is precisely the "grows pastpos" case the comment says is handled; it is not.pos(≈max_log_size) bytes within one 200 ms poll. Narrow with a largemax_log_size(e.g. the documented"100MB"example), but plausible for a chatty node configured with a smallmax_log_size(e.g."1MB"at 5 MB/s). Low severity, but real.Residual gap 2 — the just-rotated
.1.jsonltail is never followedfilesis captured once before the poll loop (logs.rs:275-278) and the loop iterates that fixed list (logs.rs:315), never re-globbing. When the daemon rotates (binaries/daemon/src/log.rs:43-72), it renames the currentlog_<node>.jsonl→log_<node>.1.jsonland creates a fresh current file. Any bytes written to the old file between the follow loop's last read (pos) and the rename now live only in the newly-created.1.jsonl, which was not infilesat startup and is never picked up. Those lines (up to ~one poll interval of logs) are lost on every rotation.This is the same gap #2785 explicitly called out: "The rotated content (now in
.1.jsonl) is never followed either — that path was never in thefileslist." It was not addressed by a68ee18.Concrete failing scenario (gap 1)
max_log_sizeand logging at high rate;dora logs --local --followrunning,pos = 1_000_000..1.jsonl, creates freshlog_<node>.jsonlat 0, keeps writing.current_size (>1_000_000) >= pos (1_000_000)→follow_read_offsetreturnsSome(1_000_000)→ seek to offset 1_000_000 in the new file → leading ~1 MB dropped, output resumes mid-line (garbled).Suggested fix direction
Track the file's identity, not just its size:
dev+ino(std::os::unix::fs::MetadataExt) alongside the offset; when it changes, treat the file as rotated and resetpos = 0regardless of size. On Windows, fall back to creation-time or a size-decrease heuristic..1.jsonlis picked up and its unreadpos..EOFtail is drained before switching to the new current file.Either change closes both residual gaps and lets the
follow_read_offsetdoc comment's guarantee hold. At minimum, the doc comment should be corrected to reflect that only the size-shrink case is detected.Scope / notes
dora logs --local --followwithmax_log_sizeconfigured (rotation only occurs then). The coordinator streaming path is unaffected.read_appended_log_lineshelper is correct; this is about the rotation/offset lifecycle around it.dora logs --local --follow#2800), which closeddora logs --local --followdrops and corrupts output when the log file rotates #2785.