Guarantee monotonic timestamps in rewritten PCAP output (#86) - #87
Conversation
The rewriter overwrote each record's timestamp in place and left the records in capture order, so the output was monotonic only if the input already was. Bridge/kernel timestamping regularly hands us captures that regress by a few hundred microseconds, which made the compressed-pipeline monotonicity check fail depending on capture timing. Collect each record's byte range alongside the microsecond value actually written, stable-sort by that key, and reassemble the file from the global header, the sorted records, and any truncated tail. Keying on the stored value rather than the full-precision logical timestamp keeps "same sort key" and "same bytes on disk" the same predicate, so ties are observable in the output; the stable sort then preserves capture order among them. Reassembly is skipped when the keys are already non-decreasing, which is a pure optimization. Byte-identical output is now guaranteed exactly when the rewritten timestamps are already ordered, rather than unconditionally. Closes #86
|
[Reviewer Round 1] No blocking findings. The implementation matches the issue's requested shape: The truncated-tail handling also looks correct: the walk stops before incomplete record bytes, The tests are meaningful and would have failed against the previous capture-order behavior. In particular, they cover regressed timestamps, equal-key stability including the tie-heavy case, monotonic identity byte preservation, both truncation shapes, big-endian reorder, raw-byte non-IPv4 preservation, logical-time sorting rather than real-time sorting, record conservation, and all-or-nothing failure behavior. I also ran |
|
[Review Verdict Round 1: APPROVED] |
`push` matched every branch and `pull_request` matched the same commits, so both fired on one commit and the whole workflow ran twice. On #87 the head commit carried two CI runs and 10 check runs -- one full set per event. They were never a "branch" run and a "merge" run; they were the same commit checked twice. Scope `push` to `main`. Merges still run CI. Drop the `pull_request` base filter as well, so a pull request against any base is covered rather than only those targeting `main`. What this gives up is CI on a branch with no pull request open, and nothing else. `pull_request` checks out the branch already merged into its base, so merge coverage is unaffected. `workflow_dispatch` replaces the lost case (`gh workflow run ci.yml --ref <branch>`), and opening the pull request as a draft works too -- nothing here filters drafts. Release automation is unaffected: release branches land through pull requests in this organization, so the `pull_request` trigger still covers them. Closes #88
Summary
rewrite_timestampsoverwrote each record's(ts_sec, ts_usec)in place and left the records in capture order, so the output PCAP was monotonic only if the input already was. Bridge/kernel timestamping regularly produces captures where two adjacent records regress by a few hundred µs, which made the compressed-pipeline monotonicity check fail depending on capture timing (as on #85).The rewriter now owns the invariant:
ts_sec * 1_000_000 + ts_usec) — not the full-precision logical timestamp, so "same sort key" and "same bytes on disk" stay the same predicate.sort_by_key), so records sharing a logical timestamp keep their capture order.incl_len,orig_len, and payloads are never touched.Side outputs (max logical timestamp, out-of-window info message) are computed during the walk as before; both are order-independent.
The doc comment's unconditional byte-identity claim is updated: byte-identity now holds exactly when the rewritten timestamps are already non-decreasing. An identity mapping over an already-monotonic capture still satisfies that, so the existing byte-identity case is unchanged.
Also adds a monotonicity assertion to the Docker-gated
generate_ac0_produces_valid_bundletest so the invariant is checked against a real capture, not only synthetic jitter.Closes #86
Test plan
cargo fmt --checkpassescargo clippy --all-targets --all-features -- -D warningspasses with no warningscargo testpasses (388 passed, 16 Docker-gated ignored)parse_pcapdrops themts_usec, pre-epoch and past-u32logical timestampsmeta.actual_endAC-0 Compressed PipelineCI job passes, including the "Assert pcap structural invariants" monotonicity check