Skip to content

fix(destinations): collapse re-delivered Records on write - #311

Closed
Minipada wants to merge 3 commits into
jazzyfrom
feature/309-dedup-records
Closed

fix(destinations): collapse re-delivered Records on write#311
Minipada wants to merge 3 commits into
jazzyfrom
feature/309-dedup-records

Conversation

@Minipada

@Minipada Minipada commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Closes #309

Stacked on #310. This branches from feature/308-record-timestamp-nanoseconds, because (tag, date) only becomes an exact per-Record identity once timestamps carry nanoseconds. Review/merge #310 first.

The Shipper is at-least-once, so an outage or a restart can deliver a Record twice — roughly a fifth of a run in the E2E harness. Nothing collapsed them: dc_records had no identity and the sink plain-inserts, so extra rows accumulated forever and every query had to remember DISTINCT.

Why not a UNIQUE constraint

It's the obvious answer and it loses data. Vector's postgres sink has no conflict handling — on_conflict is rejected as an unknown field, the config accepts only endpoint, table, pool_size, batch, request, acknowledgements — and it writes a whole batch in one statement. A violation fails the entire statement, which Vector classes as non-retriable and drops. Measured against a real Postgres:

ERROR  Non-retriable error; dropping the request.
       error=duplicate key value violates unique constraint "dc_records_key"
ERROR  component_events_dropped: Events dropped intentional=false count=4

One duplicate, four events dropped, three good Records lost. Strictly worse than the bug.

What shipped

A BEFORE INSERT trigger returning NULL, which skips the offending row without failing the statement:

CREATE TRIGGER dc_records_skip_duplicate BEFORE INSERT ON dc_records
  FOR EACH ROW EXECUTE FUNCTION dc_skip_duplicate();

Verified through the shipped init.sql — a single-statement insert of four rows where one already exists returns INSERT 0 3, with the other three committed and no error.

The index on (tag, date) is deliberately not unique: the trigger enforces identity, the index only makes its lookup cheap. Both tools/e2e/sql/init.sql and the demos' postgresql/init.sql get it.

verify_zero_loss.py now hard-fails on duplicate Records instead of reporting them as notes. dc_files is exempt and says why: its rows are Bridge-generated audit records stamped at emit time, so (tag, date) isn't their identity.

The local reproduction came back clean, and that matters

Before writing anything I tried to reproduce CI's numbers locally — one topic at 1 Hz into a Postgres Destination:

rows distinct
steady state 97 97
after 60s outage + mid-outage container restart + 120s drain 271 271

Zero duplicates. So duplication is not a deterministic consequence of outage-plus-restart, and CI's 26-per-topic depends on something a single-topic workload doesn't exercise — 20 topics, a 30s steady-state window immediately before the outage, far more in flight.

Two consequences worth stating plainly: steady state is genuinely clean (previously an assumption), and this fix is verified by CI's reproduction rather than a local one. If the CI run on this PR comes back with zero duplicate violations where the same harness previously reported 26 per topic, that is the verification.

Deliberately not done

Vector's dedupe transform. It would stop a re-delivery crossing the uplink rather than cleaning up after it — the better place to solve the bandwidth half of this. But it only helps if duplicates are born at the Bridge → Vector hop; a sink retry re-sends a batch that has already passed the transform. Which hop dominates is still unmeasured (the local repro produced nothing to measure), so adding it now would be speculative. Left open on #309.

Also left open: dc_files needs its own key, the trigger costs one indexed lookup per row, and EXISTS-then-INSERT assumes a single writer (Vector's default pool_size). All three are documented in destinations.md rather than left for someone to discover.

🤖 Generated with Claude Code

https://claude.ai/code/session_01A5JwZEZrxEtYJdQUfsZRVo

Minipada and others added 2 commits August 9, 2026 14:48
The Bridge read only `msg.header.stamp.sec` and dropped `.nanosec`, then packed
it as the ingest protocol's plain integer time. Every Record's timestamp was
rounded to the second, so a Measurement polling faster than 1 Hz produced
Records that were indistinguishable in time — measured at 5 Hz: 8 Records,
3 distinct timestamps.

The wire format already supported the fix. Fluent Forward has an EventTime
extension (ext type 0x00, 4 bytes seconds + 4 nanoseconds), and the vendored
Vector 0.57.0 parses it with all nine digits intact — verified by feeding its
fluent source both frame shapes. Only the Bridge needed to change: Record now
carries seconds and nanoseconds, and the Forwarder packs EventTime.

Adds `epoch_nanos` as a new time_format, and makes it the default. `double`
cannot represent nanoseconds — a float64 has ~15-16 significant digits and
current epoch seconds spend 10 of them, so it tops out near microseconds. That
is IEEE 754, not an implementation limit. `double` and `iso8601` stay
available. Exactness also matters beyond precision: it is what makes the
timestamp usable as part of a Record identity, which #309 needs.

The schema follows: `date` becomes bigint in both init.sql files,
`to_timestamp(date)` becomes `to_timestamp(date / 1e9)` in the dashboard, and
params writing to those tables drop their explicit `time_format: "double"`.
Console-only demos keep theirs.

Regression cover: e2e_params.yaml's memory Measurement now polls at 5 Hz so
several Records land inside one second, and verify_zero_loss.py asserts a
nanosecond remainder exists, that some second holds 2+ Records (so it cannot
pass vacuously), and that no two Records share an instant. At 1 Hz a return to
whole-second stamps would not collide and would pass unnoticed.

Closes #308

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A5JwZEZrxEtYJdQUfsZRVo
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
The Shipper is at-least-once, so an outage or a restart can deliver a Record
twice — measured at roughly a fifth of a run in the E2E harness. Nothing
collapsed them: dc_records had no identity and the sink plain-inserts, so the
extra rows accumulated forever and every query had to remember DISTINCT.

Vector's postgres sink cannot help. Its config accepts only endpoint, table,
pool_size, batch, request and acknowledgements — `on_conflict` is rejected as
an unknown field.

A UNIQUE constraint would have been much worse than the bug. The sink writes a
batch in one statement, so a violation fails the whole statement, which Vector
classes as non-retriable and drops. Measured: a batch of four containing one
duplicate lost all four, no retry, no dead-letter — one re-delivered Record
takes out every Record batched with it.

So the dedup is a BEFORE INSERT trigger returning NULL, which skips the
offending row and lets the rest of the batch commit. It keys on (tag, date),
which identifies a Record exactly now that timestamps carry nanoseconds
(#308) — a re-delivery is byte-identical, carrying the timestamp the
Measurement sampled at rather than the time of the retry. The index is
deliberately not unique: the trigger enforces identity, the index only makes
its lookup cheap.

verify_zero_loss.py now hard-fails on duplicate Records instead of reporting
them as notes. dc_files is exempt and says why: its rows are stamped at emit
time, so (tag, date) is not their identity.

Closes #309

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A5JwZEZrxEtYJdQUfsZRVo
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
@codecov

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.50%. Comparing base (1df8c02) to head (0a1cf35).

Files with missing lines Patch % Lines
dc_bridge/src/bridge_node.cpp 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            jazzy     #311      +/-   ##
==========================================
+ Coverage   40.35%   40.50%   +0.16%     
==========================================
  Files          82       82              
  Lines        5076     5094      +18     
==========================================
+ Hits         2048     2063      +15     
- Misses       3028     3031       +3     
Flag Coverage Δ
cpp-jazzy 40.50% <66.67%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…cate

The new no-duplicates assertions only fail if the run happened to produce a
re-delivery, and re-delivery is not deterministic — a quiet run would pass
them without the trigger ever firing. Same vacuous-pass flaw the timestamp
check is explicitly guarded against.

check_dedup_trigger injects a known duplicate every run: it inserts a probe
row, re-inserts it in a single statement alongside two fresh rows (how the
sink writes a batch), and asserts exactly 3 survive. Its own probe Tag, and
its rows are removed in a finally.

Exercised four ways against the shipped sql/init.sql: trigger present passes;
trigger dropped fails naming it; a UNIQUE index fails with its own message
about the batch being aborted; restored passes. That fourth case found a bug
in the check itself — the UNIQUE path made psql() raise, crashing the verifier
instead of reporting the one failure mode most worth explaining.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A5JwZEZrxEtYJdQUfsZRVo
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
@Minipada

Copy link
Copy Markdown
Owner Author

Closing: the premise is retracted, and CI on this branch fails for exactly that reason.

The "~20% duplication" this PR set out to fix was an artifact of the E2E harness, not the pipeline. workload_generator.py reset its counters to 0 in __init__, and the harness restarts that container mid-run by design, so every post-restart Record re-used a counter value and check_synth_topic read that as a double delivery. Measured: synth00 finished with 96 rows, 96 distinct timestamps, max value 64 — 96 genuinely different Records. The counts tracked the post-restart window exactly (CI's 30s drain -> 26; a 40s drain -> 31). Fixed in #312 / PR #313.

What the evidence actually shows, from a full harness run: the real Measurement Tags had zero duplicates and zero gaps across a 45s outage plus a full mid-outage restart —

 tag                       | rows | distinct_timestamps | duplicates
 dc.measurement.dummy      |  100 |                 100 |          0
 dc.measurement.memory     |  496 |                 496 |          0
 dc.measurement.os         |  100 |                 100 |          0
 dc.measurement.storage    |  100 |                 100 |          0
 dc.measurement.tcp_health |  100 |                 100 |          0
 dc.measurement.uptime     |  100 |                 100 |          0

Worth keeping from the work, recorded on #309 so it is not lost: Vector's postgres sink has no conflict handling, and a UNIQUE constraint would be actively harmful — a batch of four containing one duplicate lost all four, non-retriably. If idempotent writes are ever wanted, the BEFORE INSERT trigger in this diff is the shape that works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant