conformance: read the archive, and the loss it admits to - #63
conformance: read the archive, and the loss it admits to#63juan-malbeclabs wants to merge 2 commits into
Conversation
`--pcap` refused pcapng with `open source: Unknown magic a0d0d0a`, which is its Section Header Block read as a legacy pcap file header. pcapng is the format the recorder archives, so the validator could not read the archive it exists to judge, and every replay went through a conversion first. The conversion works — nanosecond timestamps survive — but it strips the per-packet `epb_dropcount` option, and that option is the recorder's own admission of what *it* failed to write. It is the only field in an archive that separates capture loss from publisher loss, so a converted replay can charge the recorder's drops to the publisher: the one error keeping the bytes was meant to prevent. The only guard was reading `capture_drop_total` out of the segment manifest by hand, which nothing enforced. ## Reading the format `input/pcapng.go` parses the blocks: both byte orders, `if_tsresol` and `if_tsoffset`, multiple sections, the three packet-bearing block types, and every other block skipped by length. gopacket's `NgReader` was not used because it discards per-packet options — its own comment says so — which is exactly the field that matters here. Both formats go under the one `--pcap` flag, chosen from the file's magic. Verified on real data: the committed 2001-packet market-by-price capture, converted to pcapng, replays to a byte-identical JSON report. ## What the rule set does with it `epb_dropcount` reaches the engine per datagram, ahead of the frame whose windows it taints, and taints every instance on every port — the recorder drops at its interface, so what it lost was never parsed and its channel, source and destination port are all unknowable. That is the direction `taintPortWide` already takes: the cost is a rule grading `unverifiable` instead of `pass`, never a violation the publisher did not commit. - New `capture_loss` reason, distinct from `loss` because the owner is different — `loss` sends an operator to the network, `capture_loss` sends them to the recorder. `Emit` re-owns a loss-explained finding at the one point every finding passes through. - `MBP.DELTA.PERINSTR_DENSITY` now downgrades under an admitted drop. It is otherwise reported even on a channel with a frame gap, deliberately — at that layer a publisher's skip and a lost datagram look identical — but an admitted drop is not a judgement call. This is the rule the misattribution amplified through: one lost datagram breaks the per-instrument chain of every instrument it carried, and a segment admitting 663 drops earned 238 findings on this rule alone, of 316 `must` violations where a clean control segment produced zero. - Tier-1 structural rules are untouched, as they are by any other loss. Injecting drops into the committed capture leaves its 619 `FRAME.LENGTH_CONSISTENCY` and 6 `MSG.SNAPSHOT_FLAG_MATCHES_PORT` violations where they were, and its 38 clean groups as passes. The total lands on stderr after the end-of-run findings and in the JSON report's `capture_drops`, the only place a one-shot CI replay can carry it. The exit code is deliberately unchanged: a lossy segment is still worth replaying and the violations it confirms are real — but exit 0 over a capture that admits loss is not the same claim as exit 0 over one that does not. Two boundaries are stated in the README rather than papered over: `transport_loss_total` still counts a capture-owned gap, because at the point the frame-seq hole is seen nothing can say which side lost that datagram; and a live socket admits nothing, since the kernel's overflow accounting is not wired into the multicast source. ## Tests `input/pcapng_test.go` assembles files block by block — a writer library could not produce the fixture, since none of them emit the option under test. Covers both byte orders, the drop count, drops carried across skipped packets and past the last datagram, `if_tsresol`/`if_tsoffset`, unknown blocks, section restarts, and the malformed cases that must fail rather than read as complete. `engine/capture_loss_test.go` pins the downgrade, the control that keeps an unadmitted gap a violation, the taint's era scope, and Tier-1 immunity. `run_test.go` replays a pcapng end to end and checks `capture_drops` in the report. Closes #62
ben-dz
left a comment
There was a problem hiding this comment.
Three confirmed defects in the new paths. A capture torn at a block-header boundary reads as complete and exits 0 with an empty read_error. The capture-loss taint never expires inside an era, so one admitted drop silences MBP.DELTA.PERINSTR_DENSITY for the rest of the segment and relabels later network loss as capture_loss. A snaplen-truncated packet is decoded as publisher bytes, producing false MUST violations — the same misattribution epb_dropcount was threaded in to prevent. The parser itself held up under 6.5M fuzz executions.
Findings not anchored to the current diff:
tools/conformance/prometheus/alerts/conformance.yml:48— Minor — Acapture_lossalert tells the operator to investigate the feed, which is the opposite of why the reason was split out.capture_lossjoined this alert'sreason=~set but the shared description still ends "Check feed health and instrument definitions", andrule_tests/conformance_test.yml:116now pins that text as expected output. Branch the description on$labels.reason.
| // rest is the body plus the trailing length repeat. | ||
| rest := r.scratch(int(total) - 8) | ||
| copy(rest, bom[:pre]) | ||
| if _, err := io.ReadFull(r.r, rest[pre:]); err != nil { |
There was a problem hiding this comment.
Blocking — A capture file torn part-way through a block reads as a complete one, so the run exits 0 with an empty read_error over a segment it only partly read. io.ReadFull returns bare io.EOF when it reads zero bytes, readBlock passes it through, and input/pcap.go:96 turns any io.EOF into a clean end-of-file — reachable whenever the file ends exactly at the start of a block body (verified: 8 bytes of a trailing EPB, and inside a second Section Header Block before the byte-order magic). The comment at :138-141 asserts the opposite property. Fix: past the 8-byte type+length the block is committed, so map io.EOF to io.ErrUnexpectedEOF on both later ReadFulls (:156 too).
There was a problem hiding this comment.
Fixed in 017aa58. Both reads past the 8-byte type/length pair now map io.EOF onto io.ErrUnexpectedEOF — :156 as you note, and the body read at :182 — while the header read at :145 is untouched, since that one is the genuine end of file. The comment now describes what the code does rather than what it intended. TestPcapngFileEndingAtABlockBoundaryErrors covers both shapes you verified (a trailing EPB cut to its header, and a second SHB cut before its byte-order magic); it fails on main with the file read as complete.
| } | ||
| for _, pt := range e.ports { | ||
| pt.dirtyWindow = true | ||
| pt.captureDirty = true |
There was a problem hiding this comment.
Blocking — The capture-loss taint has no window: nothing clears captureDirty short of an era advance, so one admitted drop suppresses genuine publisher violations for the rest of the segment. Verified: one ObserveCaptureLoss(1), then 500 contiguous frames with contiguous per-instrument sequences, then a real skip 501→552 on a gapless frame series — graded unverifiable/capture_loss, zero violations. Emit's relabelling has the same lifetime: a network gap 260 frames later reports capture_loss, pointing an operator at the recorder for loss the wire caused. MBP.DELTA.PERINSTR_DENSITY was previously gated on no taint at all, so this stickiness is new for it. A drop can only explain the first gap after it — clear the flag once the port's frame series is contiguous again.
There was a problem hiding this comment.
Fixed in 017aa58, and the principle you state is the one I implemented — but at two granularities rather than one, because ending it on frame contiguity alone reintroduces the findings this gate exists to prevent.
The label. captureDirty is recomputed per frame and spent on the next frame that advances the series, so a network gap 260 frames later reports loss. dirtyWindow is deliberately not spent with it: that flag answers "could a gap explain this?", which stays true for the era by design (taintOn), while this one answers the narrower "was the recorder's admitted drop what that gap was?".
The density rule. "Clear once the port's frame series is contiguous again" is not a sufficient bound for it. An instrument that updates once every few hundred frames shows its broken chain long after the frame series has recovered, so the drop's own gap is charged to the publisher — most of the 238 findings would come back. I ran that variant against the new test to be sure: TestTheExcuseWaitsForTheInstrumentThatWasBroken fails under it with Per-Instrument Seq jumped 1 -> 3 graded a Violation, exactly as it fails under the sticky flag.
So the density rule's bound is that instrument's own next dense step, which is proof its chain survived the drop. mbpDensityStatus consults the instrument rather than the port window and spends the excuse as it grants it, which also restores the invariant state.go already claimed — that captureDirty gates nothing on its own and only names the owner.
One thing your repro surfaced. Both comparisons are on an epoch counter stamped on each frame at intake, not on the engine's current value. The reorder buffer separates arrival from classification: an admission landing while a frame is buffered belongs to the frame behind it, and comparing against the live counter attributed it to the frame ahead — which arrived before the drop and cannot be what it explains. Without the stamp, TestCaptureLossNamesTheOwnerOfTheLoss breaks under a window-1 engine for that reason alone.
Three tests, all failing on main: the segment is not silenced, the label does not outlive the drop, and the excuse waits for the instrument that was broken and is spent once.
| } | ||
| ts := uint64(r.bo.Uint32(body[4:8]))<<32 | uint64(r.bo.Uint32(body[8:12])) | ||
| capLen := int(r.bo.Uint32(body[12:16])) | ||
| origLen := int(r.bo.Uint32(body[16:20])) |
There was a problem hiding this comment.
Blocking — A capture recorded with a snap length shorter than the feed's frames replays as short datagrams that get charged to the publisher. The EPB declares both captured_len and original_len; origLen is parsed into ci.Length and never read again. Verified: EPBs declaring captured_len 20 bytes below original_len replay to FRAME.SEQ_DUP_DIVERGENT must-violations and exit 1, with capture_drops: 0 and no read error — the same recorder-artifact-charged-to-the-feed error epb_dropcount was threaded in to prevent, from a field this parser already reads. Fix: capLen < origLen means the block does not hold the packet; fail the read or treat it as capture-owned rather than handing the bytes to wire.Decode.
There was a problem hiding this comment.
Fixed in 017aa58, taken as capture-owned. One note on where: the check is in PcapSource.Next rather than in readEnhancedPacket, because a legacy pcap declares both lengths too — tcpdump -s 96 produces exactly this defect in either format, and the pcapng-only fix would have left it. A short packet is counted as capture-owned loss and never reaches wire.Decode, so the sequence number it carried goes missing from the series with the taint that follows. SnaplenTruncated() tallies them apart from the admitted drops and stderr names them separately: the remedy is re-recording with a snap length that holds the feed, which is not what an operator does about a dropped datagram. Tests in both formats, both failing on main with the truncated packet yielded as a datagram.
|
|
||
| // After the end-of-run findings, so the warning is the last thing an operator | ||
| // reads rather than the first thing the finding stream buries. | ||
| captureDrops := reportCaptureLoss(src) |
There was a problem hiding this comment.
Minor — Drops admitted after the last mapped datagram reach capture_drops but never taint anything, so a report can show non-zero drops over a run whose end-of-run findings were graded as if the capture were clean. reportCaptureLoss runs after Flush/EndRun, and flushOpenSnaps (engine/gate.go:1213) emits SNAP.BEGIN_ORDER_END_GROUPING as a Violation unless open.dirty — which only an intra-group snapshot-port gap sets, and at EOF no later frame can show one. Hand the residual to the engine before eng.Flush().
There was a problem hiding this comment.
Fixed in 017aa58. PendingDrops goes to the engine before eng.Flush(), as you say. That alone did not reach flushOpenSnaps, though, and it is worth recording why: the gate is the group's own dirty, and ObserveCaptureLoss cannot set it, because the reorder buffer may not have opened the group yet when its loss is admitted — open.dirty is written during classification, which happens inside Flush. So the group records the epoch it opened at and flushOpenSnaps compares it against the run's admission count at the point of judgement (captureLossSince). MBP's end-of-run path needed nothing: flushOpenMBPSnaps already grades every unclosed group unverifiable. Pinned twice — at the engine, and end to end through Run, since the ordering you asked for lives there and nothing else can see it.
…as whole Five findings from the review of #63, each with the test that fails without its fix. ## A capture torn at a block-header boundary read as a complete one `io.ReadFull` reports bare `io.EOF` when it reads *no* bytes and `io.ErrUnexpectedEOF` only when it reads some, and `input/pcap.go` turns `io.EOF` into a clean end of file. Cut mid-body the error was already right; cut exactly at the start of a body — a trailing Enhanced Packet Block reduced to its 8-byte header, or a second section header cut before its byte-order magic — none are read, and the run exited 0 with an empty `read_error` over a segment it had only partly read. The comment on `readBlock` asserted the opposite property. Past the type/length pair the block is committed, so both later reads now map `io.EOF` onto `io.ErrUnexpectedEOF`. The first read, at a true block boundary, is untouched: that is the end of the file. ## A snaplen-truncated packet was decoded as publisher bytes The block declares both lengths and `origLen` was parsed and never read again. The surviving bytes went to `wire.Decode`, where a frame reads as declaring a length past its own datagram and its payload hashes differently from the same sequence number recorded whole — MUST violations for bytes that were never missing on the wire, which is the misattribution `epb_dropcount` was threaded in to prevent, from a field the reader already had. The check is in `PcapSource.Next` rather than in the three pcapng packet blocks, because a legacy pcap states both lengths too: a file recorded with `tcpdump -s 96` has exactly this defect in either format. Such a packet is counted as capture-owned loss and never decoded, so the sequence number it carried goes missing from the series with the taint that follows it. `SnaplenTruncated` tallies them apart from the admitted drops and stderr names them separately: the remedy is to re-record with a snap length that holds the feed, which is not what an operator does about a dropped datagram. ## The capture-loss taint had no window Nothing cleared it short of an era advance, so one admitted drop silenced `MBP.DELTA.PERINSTR_DENSITY` for the rest of the segment and relabelled every later network gap `capture_loss`, sending an operator to the recorder for loss the wire caused. The principle the review names is the fix: **a drop can account for the first gap each series shows after it and for nothing beyond that.** Applied at two granularities, because the two things the admission sets answer different questions. `captureDirty` names the owner of a gap, and its series is the frame series: it is recomputed per frame and spent on the next frame that advances the series. `dirtyWindow` is deliberately not spent with it — that flag answers "could a gap explain this?", which stays true for the era by design (`taintOn`). The per-instrument density rule needed the narrower bound. Ending the excuse when the *frame* series goes dense again reintroduces the findings this rule's gate exists to prevent: an instrument that updates once every few hundred frames shows its broken chain long after the frame series has recovered, so the drop's own gap is charged to the publisher. The bound is that instrument's own next dense step, which is proof its chain survived. `mbpDensityStatus` consults the instrument rather than the port window and spends the excuse as it grants it. Both comparisons are on an epoch counter stamped on each frame **at intake**, not on the engine's current one. The reorder buffer separates arrival from classification, and an admission that lands while a frame is buffered belongs to the frame behind it: comparing against the live counter attributed it to the frame ahead, which arrived before the drop and cannot be what the drop explains. ## Loss admitted after the last mapped datagram tainted nothing `reportCaptureLoss` ran after `Flush`/`EndRun`, so a residual reached `capture_drops` beside end-of-run findings graded as though the capture had been clean. The run loop now hands `PendingDrops` to the engine before it flushes. That alone does not reach `flushOpenSnaps`, which gates on the group's own `dirty` flag — and a `SnapshotEnd` the recorder admits it never wrote leaves no snapshot-port gap to see: the group simply never closes. Nor can `ObserveCaptureLoss` taint the group directly, since the reorder buffer may not have opened it yet when its own loss is admitted. The group records the epoch it opened at and is compared against the run's admission count at the point of judgement, which is `captureLossSince`. MBP's end-of-run path needed nothing: it already grades every unclosed group `unverifiable`. ## The `capture_loss` alert sent the operator to the feed `capture_loss` joined `ConformanceCoverageLoss`'s `reason=~` set while the shared description still ended "Check feed health and instrument definitions" — the opposite of why the reason was split out from `loss`. The description branches on `$labels.reason`; the rule test pins both renderings, and the `loss` one is unchanged. ## Tests Each defect gets a test that fails without its fix, and the two lifetimes get one each: - a file cut at a block boundary, in both of the shapes that reach it, reports a read error rather than an end of file - a snaplen-truncated packet is capture-owned and never decoded, in pcapng and in legacy pcap - one admitted drop does not silence the rule for the rest of the segment, and the `capture_loss` label does not outlive the drop - the excuse waits for the instrument whose chain was broken, and is spent once — this is the one that fails under the frame-contiguity bound as well as under the sticky flag - an unclosed snapshot group is graded against loss admitted after the last datagram, at the engine and end to end through `Run`, where the ordering lives `README.md` states both lifetimes and the snap-length case.
|
All five findings are addressed in 017aa58 — three blocking, both minors, and the unanchored alert-description one. Each has a test that fails without its fix; replies are on the individual threads. Two places where I did something other than the literal suggestion, both explained inline:
Also worth flagging from working on it: the epoch both bounds compare is stamped on each frame at intake, not read live, because the reorder buffer separates arrival from classification. Local gates: |
ben-dz
left a comment
There was a problem hiding this comment.
Approved, one new Minor: the snaplen guard keys on declared length alone, so a writer that reports the wire length including the FCS loses every packet and the run passes having checked nothing.
| // Counted as capture-owned instead: the frame's sequence number goes | ||
| // missing from the series, and the taint that follows sends the operator | ||
| // to the capture rather than to the publisher. | ||
| if cp.ci.Length > 0 && cp.ci.CaptureLength < cp.ci.Length { |
There was a problem hiding this comment.
Minor — This guard discards any packet where captured_len < original_len, which is true not only for a snap-length cut but whenever the writer reports a wire length including bytes never handed to userspace — the Ethernet FCS being the standard case, which pcapng acknowledges with if_fcslen. Verified: EPBs declaring original_len 4 bytes high with every byte present lose all three packets, and the run exits 0 with an empty report while the warning blames a snap length that was fine. udp.Length - 8 versus len(udp.Payload) separates the two exactly from what is already parsed; discarding on an incomplete UDP payload drops the false positives and still catches every truncation that costs the decoder bytes.
|
Verified against a real recorder archive, and it works — plus one gap worth deciding on deliberately. Replayed a five-minute segment written by The gap: If decompress-first is the intended contract, a message naming zstd and saying so would close it. If not, the frame is cheap to detect on the same magic you are already switching on. One unrelated observation while I had both builds side by side: this branch still shows the |
Closes #53, closes #65. **Base is `bdz/adhoc-388` (PR #64), not `main`** — the diff is incremental on the journal-overflow fix. No dependency on #63. ## Summary of Changes Three edges of `MBP.SNAP.GROUP_STRUCTURE`; the first two pull against each other. 1. **A capture's opening partial group is no longer must-violations** (#65). A snapshot port is always mid-group, so the first levels belong to a group whose `SnapshotBegin` predates the recorder. They grade `Unverifiable`/`cold_start` — what `flushOpenMBPSnaps` already gives the tail — **only before the channel's first observed `SnapshotBegin`**. The head group's orphan `SnapshotEnd` also lands before that Begin, hence 137/642 forgiven rather than 136/641. `cold_start` rather than a new `capture_start`: a live multicast join hits this identically. 2. **A post-reset group tail stays a Violation** (#53 gap 1). An `InstrumentReset` now discards that instrument's open snapshot group (spec, *Instrument Reset* step 1), which is what makes phoenix#163's shape visible; it scored clean before. `TestMBPPostResetGroupTailStaysAViolation` fails both under a blanket-forgive variant and without change 2 — I ran both. 3. **An orphan level reports no instrument id** (#53 gap 2), via `core.Finding.NoInstrumentID`, rendered `instrument_id=unknown`. Reporting 0 is what made #65's head artifact read as a SOL burst. A flag rather than a sentinel id because the spec reserves no `u32` value, so any sentinel is itself a legal id — juan-malbeclabs' review. `emitNoInstrument` sets it and `Emit` keeps its signature, so no existing call site moved. ## Testing Verification `go test ./...`, `gofmt` and `go vet` clean. Three new tests: head forgiven; post-reset charged, but not on another instrument's reset; and the absent id, asserting the flag discriminates — an orphan level carries none while the orphan `SnapshotEnd` beside it still reports its own field. Both preserved fra captures exit 0 with zero `GROUP_STRUCTURE` violations and the forgiven levels visible in `unverifiable_by_reason`. `RECONSTRUCTED_BOOK_MATCHES_SNAPSHOT` is byte-identical on both (09-02 holds at 2,452 pass / 0 violation), and the `pass` counts below are unchanged, so no denominator shrank. | capture | before | after | |---|---|---| | 09-01 | 470 pass, **137 violation**, 1 unver `{truncated:1}` | 470 pass, 0 violation, 138 unver `{cold_start:137, truncated:1}` | | 09-02 | 2532 pass, **642 violation**, 1 unver `{truncated:1}` | 2532 pass, 0 violation, 643 unver `{cold_start:642, truncated:1}` | **Not verified:** no live venue, host access or multicast run. Change 2 fired zero times on both captures (identical `pass` counts prove no reset landed mid-group), so synthetic tapes are its only coverage. **From review:** #68 tracks the substantive concern — the reset-driven discard trusts cross-port arrival order, so a level emitted just before a reset but captured just after now scores a must violation. `taintOn` would blind the rule for the rest of the era, and the tapes cannot cover it: they define the interleaving they test. Spec tension, unchanged from before: step 1 says discard the open snapshot, while *Publisher behaviour during active snapshot* permits option (a), completing and invalidating it — so a publisher choosing (a) now scores orphan violations for its tail. Juan reads it as I do and suggests the spec say so explicitly; that is normative text with a versioning question, so not in this PR.
--pcaprefused pcapng withopen source: Unknown magic a0d0d0a, whichis its Section Header Block read as a legacy pcap file header. pcapng is
the format the recorder archives, so the validator could not read the
archive it exists to judge, and every replay went through a conversion
first.
The conversion works — nanosecond timestamps survive — but it strips the
per-packet
epb_dropcountoption, and that option is the recorder's ownadmission of what it failed to write. It is the only field in an
archive that separates capture loss from publisher loss, so a converted
replay can charge the recorder's drops to the publisher: the one error
keeping the bytes was meant to prevent. The only guard was reading
capture_drop_totalout of the segment manifest by hand, which nothingenforced.
Reading the format
input/pcapng.goparses the blocks: both byte orders,if_tsresolandif_tsoffset, multiple sections, the three packet-bearing block types,and every other block skipped by length. gopacket's
NgReaderwas notused because it discards per-packet options — its own comment says so —
which is exactly the field that matters here. Both formats go under the
one
--pcapflag, chosen from the file's magic.Verified on real data: the committed 2001-packet market-by-price capture,
converted to pcapng, replays to a byte-identical JSON report.
What the rule set does with it
epb_dropcountreaches the engine per datagram, ahead of the frame whosewindows it taints, and taints every instance on every port — the recorder
drops at its interface, so what it lost was never parsed and its channel,
source and destination port are all unknowable. That is the direction
taintPortWidealready takes: the cost is a rule gradingunverifiableinstead of
pass, never a violation the publisher did not commit.capture_lossreason, distinct fromlossbecause the owner isdifferent —
losssends an operator to the network,capture_losssends them to the recorder.
Emitre-owns a loss-explained finding atthe one point every finding passes through.
MBP.DELTA.PERINSTR_DENSITYnow downgrades under an admitted drop. Itis otherwise reported even on a channel with a frame gap, deliberately
— at that layer a publisher's skip and a lost datagram look identical —
but an admitted drop is not a judgement call. This is the rule the
misattribution amplified through: one lost datagram breaks the
per-instrument chain of every instrument it carried, and a segment
admitting 663 drops earned 238 findings on this rule alone, of 316
mustviolations where a clean control segment produced zero.Injecting drops into the committed capture leaves its 619
FRAME.LENGTH_CONSISTENCYand 6MSG.SNAPSHOT_FLAG_MATCHES_PORTviolations where they were, and its 38 clean groups as passes.
The total lands on stderr after the end-of-run findings and in the JSON
report's
capture_drops, the only place a one-shot CI replay can carryit. The exit code is deliberately unchanged: a lossy segment is still
worth replaying and the violations it confirms are real — but exit 0 over
a capture that admits loss is not the same claim as exit 0 over one that
does not.
Two boundaries are stated in the README rather than papered over:
transport_loss_totalstill counts a capture-owned gap, because at thepoint the frame-seq hole is seen nothing can say which side lost that
datagram; and a live socket admits nothing, since the kernel's overflow
accounting is not wired into the multicast source.
Tests
input/pcapng_test.goassembles files block by block — a writer librarycould not produce the fixture, since none of them emit the option under
test. Covers both byte orders, the drop count, drops carried across
skipped packets and past the last datagram,
if_tsresol/if_tsoffset,unknown blocks, section restarts, and the malformed cases that must fail
rather than read as complete.
engine/capture_loss_test.gopins thedowngrade, the control that keeps an unadmitted gap a violation, the
taint's era scope, and Tier-1 immunity.
run_test.goreplays a pcapngend to end and checks
capture_dropsin the report.Closes #62