fix(collector): prove exact Binance stream coverage - #295
Conversation
|
Warning Review limit reached
Next review available in: 9 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 (2)
✨ 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 |
7988bdb to
f718c27
Compare
f718c27 to
2f428e9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7988bdb8ba
ℹ️ 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".
| .context("websocket connect timed out")??; | ||
| websocket | ||
| .send(Message::Text( | ||
| json!({"method":"LIST_SUBSCRIPTIONS","id":SUBSCRIPTION_PROOF_ID}) |
There was a problem hiding this comment.
When either USD-M shard starts, this sends monday-subscription-proof as the request ID, but Binance's USD-M live-subscription protocol defines id as an unsigned integer; the Spot protocol permits integers or alphanumeric strings, but this value also contains hyphens. Binance can therefore reject LIST_SUBSCRIPTIONS, after which no valid result reaches validate_subscription_listing and the collector exits before requesting snapshots, preventing the two-market shadow gate from passing. Use a numeric ID supported by both protocols and match the numeric response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d16f54: the proof ID is now numeric u64 value 1 in both the request and response validator, with a counterexample rejecting string ID "1".
| message = tokio::time::timeout(Duration::from_secs(20), websocket.next()) => { | ||
| message.context("websocket subscription proof timed out")? | ||
| .ok_or_else(|| anyhow::anyhow!("websocket closed before subscription proof"))?? |
There was a problem hiding this comment.
If the LIST_SUBSCRIPTIONS response is lost or never arrives while ordinary market frames continue, this per-call timeout restarts after every frame and therefore never expires. Those frames also call watchdog.mark_data(), so the watchdog does not recover the session; the shard can remain connected indefinitely while snapshots never start and health never becomes eligible. Apply one deadline to the entire proof loop rather than a fresh timeout around each websocket.next().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2d16f54: receive_url computes one tokio Instant deadline before the loop and every websocket.next() uses timeout_at against that unchanged deadline. The new subscription_proof_uses_one_absolute_deadline test proves a second ordinary wait cannot reset the budget.
There was a problem hiding this comment.
Follow-up: the final fix is stronger than the earlier timeout_at version. It pre-checks the absolute deadline, uses a deadline-first biased select, post-checks after a ready future, and applies that same deadline to proof-before archive queue backpressure. A real local WebSocket counterexample pre-fills the queue, sends a market frame without a LIST response, fails on the old path at the outer 250ms timeout, and now returns the inner subscription-proof timeout at 50ms. The archiver binary suite is 46/46 and clippy -D warnings passes.
2f428e9 to
2d16f54
Compare
2d16f54 to
c74b787
Compare
Relates to #224.
Change contract: Make the Rust Binance collector prove the exact per-shard LIST_SUBSCRIPTIONS set for depth plus aggTrade before snapshots, persist that proof into every segment, and treat unchanged two-sided books as complete without fabricating a trade or diff while preserving real USD-M bridge state.
Out of scope: Deployment/gate scripts and docs (follow-up layer C), ECS cutover, historical reconstruction, the incomplete 2026-07-20 cohort, snapshot construction, evaluator/MCTS, research logic, and Polymarket collection.
Dependency or merge order: #293 is merged. This is layer B and must merge before the deployment/gate layer C PR. No production cutover is permitted until layer C is merged and a fresh full ECS shadow gate passes. Rollback order is C, then this PR, then #293.
Focused validation:
Rollout/rollback impact: No deployment configuration or production rollout in this PR. The existing gate cannot promote the new producer evidence until layer C lands, so the intermediate state remains fail closed. Roll back layer C before this producer layer.
PRD note: This is the already-approved collector runtime defect in issue #224; implementation followed focused failing tests, so a separate PRD is unnecessary.