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
Conversation
…ilent skip after copytruncate
ApprovabilityVerdict: Approved at 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. |
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 primaryby_identity[(dev,inode,fingerprint)]map and a fallbackby_inode[(dev,inode)]map.Store.getOffset'sby_inodefallback returned a stored offset without checking that the stored fingerprint matches the queried identity's fingerprint. BecauseinodeIdentityHashdeliberately omits the fingerprint, an offset stored under an old fingerprintFP_Acould be returned for a query under a newFP_B.This interacted with the watcher, which pins
identity.fingerprintat open time and never refreshed it for a same-inode rewrite. After alogrotate --copytruncate(same inode, new content, new fingerprint), a--read-from=checkpointrestart missedby_identity(keyed on the fingerprint) and fell back toby_inode, which returned the stale pre-rotation offset. When the new content had grown past that offset, the watcher'ssize < offsetboundary 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): theby_inodebranch ofgetOffsetnow returns the offset only whenvalue.identity.fingerprint == identity.fingerprint(and the entry isn't expired). When the fingerprints differ it returnsnull, 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):maybeHandleContentRewritenow recomputes the CRC32 fingerprint from the live file when a same-inode content rewrite is detected (thesize < prefix_len,prefix_len == 0regrow, andobserved != storedbranches). The refresh runs beforeemitReadableRange, so the first post-rewrite event carries the newFP_Band subsequent enqueues/checkpoints are filed underFP_B. This is the complement that restores correct (no data loss, no re-emission) resume in the amortized case viaby_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 documentedby_identity → by_inodelookup order while making the fallback safe.Testing
zig fmt --check,ziglint),zig build, andzig build -Doptimize=ReleaseSafeall pass clean.zig build testpasses: 524 passed, 1 skipped (525 total), 0 failures vs. a 519-pass baseline — the +5 are the new tests below.src/tail/checkpoint/store.zig(the module previously had no tests):by_inodefallback returnsnullwhen the stored fingerprint differs (regression guard for Change 1).by_inodefallback still returns the offset when fingerprints match (guard against over-restricting / the gate-vs-delete decision).null(guard that the fingerprint check can't bypass the TTL check).src/tail/watch.zig:NunderFP_Avia the live lane worker, copytruncate to new content (FP_B, size ≥ N), recover the lane, andcollectwith--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.edge-tailbinary all pass or match the pre-existing baseline.test_checkpoint_recovery.py(5),test_checkpoint_resume.py(1), andtest_startup_positions.py(3) pass deterministically.test_rotation_copytruncate.py,test_rotation_rename.py, andtest_append.pypass. 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 twotest_error_handling.pycases fail, but I confirmed each fails identically on unmodified code (verified by stash/rebuild/re-run), so they are pre-existing environment flakes (root-userchmod 0limitations and poll/glob-interval timeouts), not regressions.Automatic Fixes PRs can be configured here.