fix(cursor): track watermark-boundary IDs so same-timestamp events are never lost - #38
fix(cursor): track watermark-boundary IDs so same-timestamp events are never lost#38Donemmanuelo wants to merge 2 commits into
Conversation
…e never lost (onelrian#35) SinkCursor now persists the IDs delivered exactly at the watermark, so a later poll's new event with an equal timestamp is delivered instead of being dropped by the strict '>' filter, without resending already-delivered ones. The cursor file loads migrate the legacy timestamp-only format in place, unparseable event timestamps encode as epoch 0 with a warning instead of silently substituting Utc::now(), the README gains the "Scaling limits" section src/netbird.rs referenced, and Cargo.toml drops its commented-out dependencies.
Regression guard for the fix in this branch: an unparseable event timestamp must encode deterministically as epoch 0, never as the wall-clock now.
| // event's time. Epoch 0 is a stable sentinel, and the warning | ||
| // makes the malformed data visible instead of silently hiding it. | ||
| tracing::warn!( | ||
| "Unparseable event timestamp '{}', encoding as epoch 0", |
There was a problem hiding this comment.
This fallback is worse operationally than what it replaces. timestamp_to_nanoseconds feeds Loki's push API only, and Loki's default validation rejects out-of-order entries within a stream - epoch 0 in a stream of 2026-era entries is ~36 years out of order, so the whole push returns a 4xx. Net effect: one malformed timestamp becomes a poison event that blocks the sink forever (watermark never advances past it, every poll retries the same failing batch). The old Utc::now() was nondeterministic, but ordered enough to deliver. Suggest skipping malformed-timestamp events (warn + metric, deliver the rest) instead of encoding epoch 0 - or document that Loki will refuse the batch.
| .map(|ts| ts.with_timezone(&Utc) > last_ts) | ||
| .map(|ts| { | ||
| let ts = ts.with_timezone(&Utc); | ||
| ts > last_ts || (ts == last_ts && !cursor.delivered_ids.contains(&e.id)) |
There was a problem hiding this comment.
The ID tracking correctly covers the watermark boundary, but events that surface later with a timestamp older than the watermark are still dropped silently (strict > filter, no ID tracking below the boundary). NetBird has no paging/cursor, so a backfilled old event is plausible. Worth a line in the README's Scaling limits section so the claim isn't read as "no event is ever lost".
| @@ -185,7 +191,7 @@ fn build_initial_cursors( | |||
| async fn process_cycle( | |||
There was a problem hiding this comment.
Stacking note: #38 and #39 both rewrite process_cycle + every call site in tests.rs - they'll conflict on merge. The combined behavior is coherent (delivered_ids should only advance when all chunks deliver, which #39's success path already guarantees), but pick a merge order (#38 then #39, or stack them) and rebase. Also no CI is reported on this fork branch - ensure the test gate runs before merge.
2 participants
Closes #35.
User-facing impact
event later whose timestamp equals an already-delivered one; the strict
>cursor filter dropped it forever. The per-sink cursor now also recordsthe IDs delivered exactly at the watermark, so such events are delivered
without resending the ones already seen. Cursor files written by older
versions load and migrate in place, so existing deployments resume without
a full-history replay.
longer silently rewrites a malformed timestamp to the wall-clock
now()(which differed every poll); it encodes epoch 0 and logs a warning.
src/netbird.rsalreadyreferenced, and
Cargo.tomldrops its commented-out dependencies.Verification
cargo fmt --check,cargo clippy --all-targets -- -D warnings, andcargo test --lockedall pass (31 tests, +3 new: legacy cursor migration,same-timestamp delivery without resend, epoch-zero fallback).
once and does not resend the previously delivered IDs.