fix(collector): exclude one-sided startup books - #282
Conversation
|
Warning Review limit reached
Next review available in: 52 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 Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe shadow gate now settles health longer, starts manifest eligibility at observation time, enforces complete bridging and reconnect-free evidence, and adds deterministic tests. The Binance archiver excludes one-sided initial snapshots and restarts capture sessions after exclusions. ChangesShadow gate validation
Snapshot session control
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ShadowGate
participant HealthChecks
participant OSSManifests
ShadowGate->>HealthChecks: wait for health-settle deadline
HealthChecks-->>ShadowGate: bridged symbol validation
ShadowGate->>ShadowGate: record observation_started_ns
ShadowGate->>OSSManifests: select manifests after observation start
OSSManifests-->>ShadowGate: manifest data and SHA-256 evidence
sequenceDiagram
participant InitialSnapshotProducer
participant RESTSnapshotServer
participant run_session
InitialSnapshotProducer->>RESTSnapshotServer: request initial depth snapshot
RESTSnapshotServer-->>InitialSnapshotProducer: one-sided snapshot
InitialSnapshotProducer->>run_session: emit ExcludeSymbol
run_session->>run_session: restart capture session
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a1ab55dd2
ℹ️ 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".
| let event = Event::ExcludeSymbol { | ||
| symbol: symbol.clone(), | ||
| reason: "one-sided initial snapshot is not replay-complete".to_owned(), |
There was a problem hiding this comment.
When a one-sided symbol emits a depth update before its REST snapshot—possible because every websocket connects before snapshot production—this event causes process_event to remove the symbol from the active catalog even though its diff is already archived. On session close, close_segment_at updates the segment catalog to exclude that symbol, after which summarize_lob_continuity rejects the recorded diff as outside the declared scope; the resulting .jsonl.part remains in the spool and assert_spool_drained makes the shadow gate fail. Preserve the segment's original catalog or explicitly discard/quarantine this unsafe segment, with a diff-before-exclusion counterexample test.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| match pending_action { | ||
| action if action.restarts_capture_session() => break, |
There was a problem hiding this comment.
When an exclusion triggers, this new break stops websocket activity and enters task draining plus close_segment while the process watchdog remains armed. The deployed environments allow only 180 seconds of market-data silence but permit compression to run for 300 seconds, and publish_global_shutdown explicitly disarms the watchdog for this same drain/compression path; therefore a slow close can invoke process::exit(75), lose the in-memory exclusion, and repeat the exclusion after systemd restarts the process. Disarm and re-arm around the internal session transition, or otherwise report watchdog progress during the close.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
| start_ns=$(jq -er '.start_received_at_ns' "$manifest") | ||
| end_ns=$(jq -er '.end_received_at_ns' "$manifest") | ||
| ((end_ns < gate_started_ns)) && continue | ||
| ((start_ns < observation_started_ns)) && continue |
There was a problem hiding this comment.
Filtering out every segment that started before observation also removes the session's only real snapshot seeds. The first retained segment is opened by scheduled rotation with segment_open checkpoints, but verify_binance_market_tape_with_requirements explicitly rejects a first-segment checkpoint unless that segment previously contained a snapshot; consequently the --require-lob-continuity invocation at the end of this function fails for an otherwise stable post-warm-up session, so the production gate cannot pass. Include a trusted pre-observation seed segment in strict verification (without counting it as observation evidence), or teach the verifier an equivalent anchored-checkpoint contract and test the real verifier path.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
| let event = Event::ExcludeSymbol { | ||
| symbol: symbol.clone(), | ||
| reason: "one-sided initial snapshot is not replay-complete".to_owned(), |
There was a problem hiding this comment.
When a one-sided symbol also appears in security_token_symbols, this event only adds it to excluded_symbols; Config::segment_config continues copying it into each manifest's security-token list while active_symbols removes it from symbols. The strict artifact validator requires every security_token_symbols entry to remain in symbols, so every otherwise clean segment produced after the capture-session restart is rejected and the shadow gate cannot pass. Filter the security-token subset when applying runtime exclusions and cover this catalog combination with a counterexample test.
AGENTS.md reference: AGENTS.md:L95-L96
Useful? React with 👍 / 👎.
8a1ab55 to
d6c7c50
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs`:
- Around line 1327-1343: Update the one-sided snapshot exclusion branch in the
symbol-processing flow so that after `SendOutcome::Sent`, it invokes the
existing `wait_or_shutdown` rate-limit delay before continuing. Preserve the
`SendOutcome::Shutdown` handling and ensure excluded symbols still respect the
configured `snapshot_requests_per_second` request rate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2aa17181-16ab-4ef7-8774-09051ce36c3e
📒 Files selected for processing (6)
deployment/aliyun/README.mddeployment/aliyun/host-rust-lob-shadow-gate.shdeployment/aliyun/rust-lob-shadow-gate-policy.jqdeployment/aliyun/test-rust-lob-control-plane.shdeployment/aliyun/test-rust-lob-release-adoption.shrust_hft/tools/collector/src/bin/binance-lob-archiver.rs
d6c7c50 to
6e27a19
Compare
|
CI disposition for final head
This baseline red is tracked separately from #224 and is not being represented as a passing check. |
Change contract
Exclude initially one-sided Binance books with explicit manifest evidence, restart the capture session on runtime catalog exclusion, and accept only fully bridged post-warmup shadow segments.
Acceptance evidence
symbol_excludedrather than a replay-complete snapshot.bash -nandshellcheckpass for the modified gate scripts.deployment/aliyun/test-rust-lob-control-plane.shpasses, including nanosecond warmup cutoff, actual 600-second shadow cadence, zero post-observation reconnect boundaries, and policy rejection of a pre-observation reconnect marker.rustfmt --checkandgit diff --checkpass.Live failing proof that motivated this rollout:
09524e9034a3504f518a10573dd7fb239bb02a6ca7f28cd6f34a27fd15b8d6a1/data/monday/evidence/shadow-gates/09524e9034a3504f518a10573dd7fb239bb02a6ca7f28cd6f34a27fd15b8d6a1/36ec1715c91519ec68185c427958b7818622a2d095e71d471fcf8b663e5af99a/runs/20260723T164013Z-348370/run.jsonSCRIDR; the live book was one-sided. No production cutover occurred.Out of scope
agg_tradeor reference collector behavior.Dependency or merge order
Depends on merged PR #277 (
08456949104a64756703e439b3718915135b5a98). Merge this PR before building the next exact-main collector candidate and rerunning the full gate.Focused validation
Rollout and rollback impact
Rollout: merge, build an exact-main artifact, install it as an isolated release, and run the full Spot plus USD-M shadow gate. Cut over only if canonical OSS readback, LOB continuity,
agg_trade > 0, manifest SHA, and_SUCCESSall pass.Rollback: do not adopt the candidate, or repoint the shadow release to the prior artifact. Production remains on its current binary until the gate passes.
Refs #224
Summary by CodeRabbit
Reliability Improvements
Data Quality
Testing