Fix STATISTICS decode corruption and related decoder bugs (#32-#35)#36
Conversation
- read_stat_msg: size the body from hd.length; decode 64-byte pre-v3 StatMsg records (Int32 quantity) and upgrade them to the v3 shape instead of overrunning the record and desyncing the stream (#32) - stat_to_dataframe: use real StatMsg fields (price, quantity, stat_flags, ts_ref, stat_type, channel_id, update_action) instead of nonexistent stat_value/flags (#33) - metadata: decode every symbol-mapping interval, one mappings tuple per interval; encoder groups consecutive same-symbol tuples back into one entry so round-trips are lossless (#34) - enums: uniform warn-and-default policy for Side/Action/InstrumentClass via lookup tables; invalid bytes no longer throw, and unknown Action defaults to NONE rather than TRADE (#35) Adds regression tests for all four issues, including v1/v2 statistics test-data decoding and a desync regression on a synthetic stream. Fixes #32, #33, #34, #35 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c53b84600f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @warn "Unexpected StatMsg body size $body_size; skipping record" maxlog = 1 | ||
| skip(decoder.io, body_size) | ||
| return nothing |
There was a problem hiding this comment.
When a STATISTICS file contains a record whose body is between 49 and 63 bytes, this returns nothing, but read_dbn selects the Vector{StatMsg} typed path for Schema.STATISTICS; that path assigns or pushes the result without checking for nothing, causing a conversion MethodError rather than skipping the record as intended. The typed loops in read_dbn_typed need to recognize this skip result, or this function must signal it in a way those loops handle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa023c9 — all four typed call sites (read_dbn_typed exact/dynamic loops, _foreach_record_impl, _foreach_record_with_control_impl) now skip a nothing result and continue, with a regression test decoding a STATISTICS file containing a 56-byte-body record through the typed read_dbn path.
| # Upgrade pre-v3 records to the v3 size so re-encoding stays consistent | ||
| out_hd = body_size == 48 ? | ||
| RecordHeader(UInt8(80 ÷ LENGTH_MULTIPLIER), hd.rtype, hd.publisher_id, hd.instrument_id, hd.ts_event) : | ||
| hd |
There was a problem hiding this comment.
For a future or vendor-extended StatMsg with a body larger than 64 bytes, the decoder deliberately skips the extra tail but returns the original header here. Re-encoding that value writes only the fixed 64-byte v3 body in write_record_complex, while preserving a header length that advertises more bytes, so the next record becomes misaligned for downstream readers. Normalize every accepted 64+-byte record to the 80-byte header size, or preserve and re-emit the discarded extension bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aa023c9 — the header is now normalized to the v3 record size (length 20) whenever the on-disk size differs, covering extended 64+-byte bodies with discarded tails as well as pre-v3 upgrades. Regression test included (88-byte record with an 8-byte vendor tail, followed by a record that must stay aligned).
- Typed read paths (read_dbn_typed, _foreach_record_impl, _foreach_record_with_control_impl) now skip a nothing returned by a record reader for a malformed record instead of erroring on assignment - read_stat_msg normalizes the header to the v3 record size for any on-disk size that differs (not just pre-v3 upgrades), so extended records with discarded vendor tails re-encode without misaligning downstream readers Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Fixes the four issues filed against v0.1.3, all uncovered while pulling
Schema.STATISTICSfromGLBX.MDP3:read_stat_msghardcoded the 80-byte DBN v3 layout and ignoredhd.length, so the 64-byte pre-v3StatMsgrecords currently served by the historical gateway overran the record boundary and desynced the whole stream. The reader now sizes the body from the header (matchingread_error_msg/read_symbol_mapping_msg): a 48-byte body decodes as the pre-v3 layout (quantity::Int32,typemax(Int32)mapped to the v3 UNDEF sentinel) and is upgraded to the 80-byte v3 header size; 64+ byte bodies use the v3 layout and skip trailing bytes; unexpected sizes warn and skip the record rather than corrupting the stream.stat_to_dataframereferenced nonexistentStatMsgfields (stat_value,flags). It now exportsprice(viaprice_to_float),quantity,flags(fromstat_flags),ts_ref,stat_type,channel_id, andupdate_action.(raw_symbol, mapped_symbol, start_date, end_date)tuple per interval (field type unchanged), so continuous-contract roll history survives. The encoder groups consecutive same-symbol tuples back into one mapping entry, keeping round-trips lossless.Side/Action/InstrumentClassnow share one policy for invalid bytes: warn once and fall back to a sentinel (Side.NONE,Action.NONE,InstrumentClass.OTHER) instead ofSide/Actionthrowing and killing the stream. Conversions use 256-entry lookup tables, so the hot path stays free oftry/catch. The oldsafe_actionfallback ofAction.TRADEwas changed toAction.NONEsince defaulting unknown actions to TRADE fabricated trades.Tests
test/test_issues_32_35.jladds regressions for each issue (77 assertions):test_data.statistics.v1/v2.dbn.zstfiles — which previously desynced after the first record — now decode field-for-field identical to the v3 file, with headers upgraded to the v3 sizeStatMsgrecords, proving the record after a pre-v3 record decodes intact (the desync regression), including v1UNDEF_STAT_QUANTITYsentinel mappingStatMsg→records_to_dataframeround-trip, includingUNDEF_PRICE→NaNMetadataencode/decode round-trip plus a second symbol to check grouping boundariesside/actionbytes on disk decode toNONEwith a warning while the following record decodes correctlyFull suite passes on Julia 1.12.
Fixes #32, #33, #34, #35
🤖 Generated with Claude Code