Skip to content

fix(tail): gate checkpoint by_inode fallback on fingerprint to stop silent skip after copytruncate - #294

Open
detail-app[bot] wants to merge 2 commits into
masterfrom
detail/bug-fix/fix-tail-gate-checkpoint-by-inode-fallback-on-fing-83dd98
Open

detail-app[bot] wants to merge 2 commits into
masterfrom
detail/bug-fix/fix-tail-gate-checkpoint-by-inode-fallback-on-fing-83dd98

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 16, 2026

Copy link
Copy Markdown

Detail bug report: View on Detail

Bug

The tail checkpoint store (src/tail/checkpoint/store.zig) caches per-file read offsets in two in-memory indexes: a primary by_identity[(dev,inode,fingerprint)] map and a fallback by_inode[(dev,inode)] map. Store.getOffset's by_inode fallback returned a stored offset without checking that the stored fingerprint matches the queried identity's fingerprint. Because inodeIdentityHash deliberately omits the fingerprint, an offset stored under an old fingerprint FP_A could be returned for a query under a new FP_B.

This interacted with the watcher, which pins identity.fingerprint at open time and never refreshed it for a same-inode rewrite. After a logrotate --copytruncate (same inode, new content, new fingerprint), a --read-from=checkpoint restart missed by_identity (keyed on the fingerprint) and fell back to by_inode, which returned the stale pre-rotation offset. When the new content had grown past that offset, the watcher's size < offset boundary check didn't fire, so bytes [0, N) of the new content were silently dropped — never emitted, never logged.

Fixes ENG-695

Fix

Two co-equal changes, both required:

  • Store-level fingerprint gate (src/tail/checkpoint/store.zig): the by_inode branch of getOffset now returns the offset only when value.identity.fingerprint == identity.fingerprint (and the entry isn't expired). When the fingerprints differ it returns null, forcing the watcher to read new content from offset 0. This is the necessary fix that blocks silent data loss in both trigger branches (kill-then-rotate, and rotation-then-power-loss).

  • Watcher fingerprint refresh (src/tail/watch.zig): maybeHandleContentRewrite now recomputes the CRC32 fingerprint from the live file when a same-inode content rewrite is detected (the size < prefix_len, prefix_len == 0 regrow, and observed != stored branches). The refresh runs before emitReadableRange, so the first post-rewrite event carries the new FP_B and subsequent enqueues/checkpoints are filed under FP_B. This is the complement that restores correct (no data loss, no re-emission) resume in the amortized case via by_identity[(dev,inode,FP_B)] hits on restart.

The gate alone would trade silent skip for unbounded re-emission of already-delivered content; the refresh alone wouldn't protect the window before the first post-rotation wal.sync. Both together preserve the documented by_identity → by_inode lookup order while making the fallback safe.

Testing

  • Unit tests, lint (zig fmt --check, ziglint), zig build, and zig build -Doptimize=ReleaseSafe all pass clean.
  • zig build test passes: 524 passed, 1 skipped (525 total), 0 failures vs. a 519-pass baseline — the +5 are the new tests below.
  • New store-level unit tests in src/tail/checkpoint/store.zig (the module previously had no tests):
    • by_inode fallback returns null when the stored fingerprint differs (regression guard for Change 1).
    • by_inode fallback still returns the offset when fingerprints match (guard against over-restricting / the gate-vs-delete decision).
    • expired entry with a matching fingerprint still returns null (guard that the fingerprint check can't bypass the TTL check).
  • New watcher tests in src/tail/watch.zig:
    • End-to-end: durably checkpoint offset N under FP_A via the live lane worker, copytruncate to new content (FP_B, size ≥ N), recover the lane, and collect with --read-from=checkpoint — asserts the emitted event is [0, 8192), not the stale [N, 8192). Reproduces the Branch B data-loss scenario and confirms it's closed.
    • Live copytruncate refreshes the pinned fingerprint — asserts the post-rotation event carries a fingerprint matching the new content (guard for Change 2).
  • Integration tests against the edge-tail binary all pass or match the pre-existing baseline. test_checkpoint_recovery.py (5), test_checkpoint_resume.py (1), and test_startup_positions.py (3) pass deterministically. test_rotation_copytruncate.py, test_rotation_rename.py, and test_append.py pass. A few timing-sensitive tests (test_rotation_lifecycle.py::test_path_missing_then_reappears, test_line_framing.py::test_over_limit_line_is_truncated) and two test_error_handling.py cases fail, but I confirmed each fails identically on unmodified code (verified by stash/rebuild/re-run), so they are pre-existing environment flakes (root-user chmod 0 limitations and poll/glob-interval timeouts), not regressions.

Automatic Fixes PRs can be configured here.

Comment thread src/tail/watch.zig
@macroscopeapp

macroscopeapp Bot commented Sep 16, 2026

Copy link
Copy Markdown

Approvability

Verdict: Approved at 8cfeb17

Macroscope's review found this PR approvable — This is a focused checkpoint correctness fix that prevents stale same-inode offsets from silently skipping post-copytruncate content, while refreshing fingerprints for subsequent checkpoints. The added tests cover the fallback, expiry, restart, and live rewrite cases, with no schema, security, or infrastructure impact.

You can add or adjust custom eligibility rules. Learn more.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Sep 16, 2026
… copytruncate

When the file grows past a short prefix that was set during a partially
written copytruncate, maybeHandleContentRewrite returned early without
refreshing the identity fingerprint.  The stored fingerprint was computed
from the partial file content, so ongoing checkpoints were filed under the
stale fingerprint.  A checkpoint-based restart then missed by_identity and
(with the fingerprint gate) fell back to reading from offset 0 — correct,
but the live lane's stale fingerprint meant all subsequent enqueues were
stored under the partial-coverage key, diverging from what a fresh open
would compute.

Fix: call refreshIdentityFingerprint on the unchanged-prefix path so the
fingerprint is always kept in sync with the current file content.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant