fix(collector): split transport reconnects from manifest request_failures - #593
Conversation
…ures A bare websocket_receive reset is reconnection lifecycle evidence, not an integrity failure: post-resync book state is rebuilt by the CLOB snapshot replay, so counting those rows in request_failures made the canonical and segment_complete gates unreachable on a venue that resets roughly every 25 minutes. Manifest quality now counts only integrity-domain failures (every quote_collection_failure kind except websocket_receive) in request_failures and reports the receive rows separately as transport_reconnects. quote_coverage_complete, canonical, and segment_complete therefore pass for tapes whose only failure rows are transport reconnects, while any integrity-domain failure still blocks them. Tape row schema, upload identity, and all other manifest fields are unchanged, and failure rows keep being recorded as before. Verification: cargo test -p hft-collector (364 passed across all suites, 0 failed, incl. new transport_reconnects counterexamples and the updated mixed-kind rejection test); cargo fmt --check clean; cargo clippy -D warnings clean. Refs #592
|
Warning Review limit reached
Next review available in: 33 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)
📝 WalkthroughWalkthroughThe collector now classifies WebSocket receive failures as transport reconnects. Manifests report ChangesPolymarket reconnect accounting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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.
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/polymarket_upload.rs`:
- Around line 1459-1467: The error handling around websocket_receive in the
upload completeness logic must require evidence of post-reconnect tape
reconstruction. Add and consume a post-resync snapshot/recovery record in the
raw manifest, or count websocket_receive in request_failures until that evidence
exists; do not allow transport_reconnects alone to preserve canonical
completeness.
🪄 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: eadd7b5b-bb0d-4b64-a38d-a2dc635068ca
📒 Files selected for processing (1)
rust_hft/tools/collector/src/polymarket_upload.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f34770b00
ℹ️ 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".
P1 (manifest): a websocket_receive or transport_reconnect row is now excused from request_failures only when the same token later records a successful quote in tape order, proving the resync actually recovered. Transport rows without that recovery evidence still count as request_failures, keeping canonical and segment_complete false for tapes where a token never came back. The error_kind whitelist accepts the new transport_reconnect value. P2 (feed): the hot-path receive-error arm no longer maps every stream error to websocket_receive. Bare transport drops (io close-family kinds, plus TCP resets surfaced as ProtocolError::ResetWithoutClosingHandshake) classify as transport_reconnect, while genuine protocol violations (other protocol errors, UTF-8, capacity, non-close io kinds) classify as websocket_payload, matching the manifest's integrity domain. Tape row schema is unchanged and the fail-closed empty quote emission is untouched. Verification: cargo test -p hft-collector (365 passed, 0 failed, incl. recovery-gate counterexamples); cargo test -p ploy-market-data (71 passed) and with --features live (168 passed, incl. the receive-error classification test) under the pinned 1.91 toolchain; fmt clean on both touched files; clippy unchanged (hft-collector 0, ploy-market-data 36 pre-existing lines). Refs #592
|
Both findings addressed in 7b5bce0: (P1) transport-kind rows (websocket_receive/transport_reconnect) are exempted only when the same token shows a subsequent success quote in tape order — a receive row left as the final event for a token falls back into request_failures; (P2) stream errors are now classified: I/O close-family and ResetWithoutClosingHandshake become transport_reconnect, while protocol/UTF-8/capacity errors become websocket_payload (integrity). hft-collector 365, ploy live 168, default 71 all green. |
Change contract
The uploader manifest now counts only integrity-domain failures (
websocket_payloadand any other non-websocket_receivekind, including dirty-isolation rows) inquality.request_failures, and reports transport-lifecycle rows separately asquality.transport_reconnects.canonical/segment_complete(and the upload gate, which reads the same counter) therefore pass tapes whose only failure rows arewebsocket_receivetransport provenance, while any integrity failure still fails closed. Tape row schema is unchanged.Out of scope
error_kindtaxonomy and emission behavior (unchanged).Dependency or merge order
Implements the owner-approved kind-aware gate evolution on #313 (2026-08-02). Single commit on
9dfe84bc.Focused validation
retains_request_failure_but_marks_segment_incomplete: a tape with onewebsocket_connect+ onewebsocket_receiverow now reportsrequest_failures: 1,transport_reconnects: 1, and remains non-canonical (mixed tape rejected).transport_reconnects_do_not_block_complete_segment: a complete tape carrying onlywebsocket_receiverows reportsrequest_failures: 0,transport_reconnects: 2, and iscanonical/segment_complete(transport-only tape accepted).cargo test -p hft-collector: 268 + 72 + 4 passed, 0 failed.cargo fmt --checkclean for the changed file (four pre-existing files in the crate fail the newer rustfmt rules at base — untouched and byte-identical to base). Clippy: no new warnings.Rollout/rollback impact
Manifest semantics change only: downstream gates reading
request_failuresautomatically get the kind-aware meaning approved on #313; consumers needing transport counts can readtransport_reconnects. Already-published manifests are not rewritten by this change. Rollback = revert.Issue relationship
Refs #592
Summary by CodeRabbit