fix(binance): restart capture epoch after depth gap - #126
Conversation
A depth gap previously resnapshotted inside the same session, leaving every later segment permanently replay-unsafe. Return the gap through the existing session failure path so the outer reconnect creates a fresh session_id and ProcessState.
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Binance LOB archiver now produces snapshots for all active symbols, emits initial-snapshot completion, keeps the producer alive until shutdown, and propagates sequence-gap errors instead of scheduling resnapshots. ChangesBinance LOB archiver control flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotProducer
participant BinanceExchange
participant EventChannel
participant RunSession
SnapshotProducer->>BinanceExchange: request snapshots for active symbols
BinanceExchange-->>SnapshotProducer: snapshot or SnapshotUnavailable
SnapshotProducer->>EventChannel: emit Snapshot or ExcludeSymbol
SnapshotProducer->>EventChannel: emit InitialSnapshotsComplete
SnapshotProducer->>RunSession: wait for shutdown
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust_hft/tools/collector/src/bin/binance-lob-archiver.rs (2)
1207-1213: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winApply the rate limit after unavailable snapshots too.
continuebypasses the only interval wait at Lines 1232-1234. Consecutive unavailable symbols can therefore burst REST requests despitesnapshot_requests_per_second, potentially triggering exchange throttling.Proposed fix
- SendOutcome::Sent => continue, + SendOutcome::Sent => { + if wait_or_shutdown(&mut shutdown, interval).await { + return Ok(TaskExit::Stopped(None)); + } + continue; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs` around lines 1207 - 1213, Update the SnapshotUnavailable branch in the symbol-processing loop so it waits for the configured snapshot request interval before continuing, matching the existing rate-limit behavior used by successful and other request paths. Ensure consecutive unavailable symbols still honor snapshot_requests_per_second instead of bypassing the interval through continue.
1207-1213: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail the session when the final active symbol is excluded.
If every snapshot is unavailable, all states are removed and this producer remains alive. Empty
states.values().all(...)checks then report the session assynced, silently collecting no market data. Terminate the session after processing the last exclusion instead.As per coding guidelines, “Never simplify away validation, security, data-loss prevention, or other trust-boundary checks.”
Also applies to: 1236-1242
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs` around lines 1207 - 1213, The SnapshotUnavailable handling around send_or_shutdown must terminate the session when excluding the final active symbol. After removing the symbol from the producer’s active state, detect that no active symbols remain and return the appropriate failure instead of allowing empty states to satisfy the synced check; preserve normal continuation when other symbols remain.Source: Coding guidelines
🧹 Nitpick comments (1)
rust_hft/tools/collector/src/bin/binance-lob-archiver.rs (1)
2447-2450: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the gap evidence is archived.
The regression verifies termination and the health counter, but not the PR’s evidence-retention contract. Add
assert_eq!(segment.event_count("sequence_gap"), 1)so removing the artifact write cannot regress unnoticed.As per coding guidelines, “Define success criteria before non-trivial work; reproduce bugs with focused tests.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs` around lines 2447 - 2450, Extend the regression test assertions around the existing sequence-gap error and process_state checks to verify evidence retention by asserting segment.event_count("sequence_gap") equals 1. Use the existing segment symbol and preserve the current termination and health-counter assertions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs`:
- Around line 1207-1213: Update the SnapshotUnavailable branch in the
symbol-processing loop so it waits for the configured snapshot request interval
before continuing, matching the existing rate-limit behavior used by successful
and other request paths. Ensure consecutive unavailable symbols still honor
snapshot_requests_per_second instead of bypassing the interval through continue.
- Around line 1207-1213: The SnapshotUnavailable handling around
send_or_shutdown must terminate the session when excluding the final active
symbol. After removing the symbol from the producer’s active state, detect that
no active symbols remain and return the appropriate failure instead of allowing
empty states to satisfy the synced check; preserve normal continuation when
other symbols remain.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs`:
- Around line 2447-2450: Extend the regression test assertions around the
existing sequence-gap error and process_state checks to verify evidence
retention by asserting segment.event_count("sequence_gap") equals 1. Use the
existing segment symbol and preserve the current termination and health-counter
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50c90222-bab3-47d3-90e4-b5101b70f741
📒 Files selected for processing (1)
rust_hft/tools/collector/src/bin/binance-lob-archiver.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a42fcb967
ℹ️ 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".
| state.invalidate_for_resync(budget); | ||
| return Ok(ProcessAction::Resnapshot(symbol)); | ||
| } | ||
| return Err(error); |
There was a problem hiding this comment.
When an order-book gap now takes this Err path, run_session stops all receivers before closing the segment and sleeping for the outer reconnect; unlike the old same-session resnapshot path, nothing calls mark_data() during that intentional finalization window. For large segments where close_segment/zstd runs longer than PROCESS_WATCHDOG_SECONDS (defaults are 300s zstd timeout vs 180s watchdog), the watchdog can exit the process before the unsafe evidence is finalized and before the fresh epoch starts. This path should either disarm/refresh the watchdog around intentional gap recovery or otherwise keep it from treating reconnect finalization as a market-data stall.
Useful? React with 👍 / 👎.
Change contract
Any Binance order-book depth sequence gap ends the current capture epoch: the unsafe segment remains evidence, while the existing outer reconnect starts a fresh
session_idandProcessStateso later replay-safe segments cannot bridge over skipped data.Out of scope
Dependency or merge order
Based on
mainafter #125. No stacked PR dependency follows from this change.Focused validation
lastUpdateId=11075153756947, then depth eventU=11075153761705,u=11075153767256,pu=11075153761591. The old same-session resnapshot left all successor segments permanentlyreplay_safe=false.depth_gap_ends_the_capture_epochfailed against the old behavior (Ok(Resnapshot("BTCUSDT"))) and passes with this change.cargo test --locked -p hft-collector --features collector-binance: 159 library tests passed (2 ignored), 37 archiver tests passed, 10 materializer tests passed, and remaining bin tests passed.cargo clippy --locked -p hft-collector --features collector-binance --all-targets -- -D warningscargo fmt --package hft-collector -- --checkgit diff --check origin/main...HEADRollout or rollback impact
The collector now uses its existing reconnect boundary after a depth gap, producing a new capture epoch and session identity. No live trading is enabled or changed. Rollback is a single PR revert; that restores same-session resnapshot behavior, after which downstream verification remains fail-closed and rejects affected successor segments.
Summary by CodeRabbit
New Features
Bug Fixes