fix(pm-upload): tolerate bounded per-token quote source-time jitter - #585
Conversation
Refs #584 Tick-level tapes mix WS hot-path and REST poll quotes for the same token, so per-token source times legitimately regress by a few seconds (observed ~3.7s). The strict monotonic check rejected the first tick-level hourly segment (7.7GB) at upload time. - Tolerate per-token quote source-time regressions up to 30s, anchored at the max seen ts per token; larger regressions still fail with the magnitude in the error. - Count tolerated regressions into manifest quality (tolerated_quote_source_regressions). - recorded_at file-order monotonicity and sequence-gap checks unchanged. Tests: 3s jitter accepted (counter=1, anchor advances on recovery); 60s regression rejected.
|
Warning Review limit reached
Next review available in: 37 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 (1)
📝 WalkthroughWalkthroughThe Polymarket uploader now accepts per-token quote source timestamp regressions up to 30 seconds. It counts tolerated regressions in manifest quality metadata and rejects larger regressions with a measured error. ChangesQuote timestamp tolerance
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
🧹 Nitpick comments (1)
rust_hft/tools/collector/src/polymarket_upload.rs (1)
2833-2859: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the jitter test verify anchor retention.
The sequence
2s -> 1s -> 3sdoes not detect backward anchor movement. Both the current implementation and an incorrect implementation that stores1sreport one tolerated regression.Add an intermediate timestamp between
1sand2s. Assert two tolerated regressions. Add a later forward sample to verify recovery.Proposed test adjustment
- third["update"]["ts"] = json!("2026-07-15T01:00:03Z"); + third["update"]["ts"] = json!("2026-07-15T01:00:01.500Z"); + let mut fourth = first.clone(); + fourth["sequence"] = json!(3); + fourth["recorded_at"] = json!("2026-07-15T01:00:05Z"); + fourth["update"]["ts"] = json!("2026-07-15T01:00:03Z"); ... - &[first, second, third], + &[first, second, third, fourth], ... - json!(1) + json!(2)As per coding guidelines, “Safety boundaries require a targeted counterexample test, not only workspace compilation.”
🤖 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/polymarket_upload.rs` around lines 2833 - 2859, The test tolerates_bounded_quote_source_time_jitter_per_token does not detect backward movement of the retained timestamp anchor. Add an intermediate sample timestamped between 1s and 2s so the sequence exercises the original anchor after the regression, update the expected tolerated_quote_source_regressions to 2, and append a later forward sample to verify recovery after the jitter.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.
Inline comments:
In `@rust_hft/tools/collector/src/polymarket_upload.rs`:
- Around line 1296-1303: Update the regression check in the quote-source
validation block to compare the full `previous - source_at` duration against
`TimeDelta::milliseconds(MAX_QUOTE_SOURCE_REGRESSION_MS)` before converting
units. Keep `num_milliseconds()` only when constructing the `bail!` error
message, preserving the existing tolerated-regression counting behavior.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/polymarket_upload.rs`:
- Around line 2833-2859: The test
tolerates_bounded_quote_source_time_jitter_per_token does not detect backward
movement of the retained timestamp anchor. Add an intermediate sample
timestamped between 1s and 2s so the sequence exercises the original anchor
after the regression, update the expected tolerated_quote_source_regressions to
2, and append a later forward sample to verify recovery after the jitter.
🪄 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: e85a3b10-63a8-4d15-ad7d-6245f971dffe
📒 Files selected for processing (1)
rust_hft/tools/collector/src/polymarket_upload.rs
num_milliseconds() truncates, so 30s+1ns passed the bound. Compare the TimeDelta directly and keep the millisecond conversion only for the error message.
Change contract
Let upload validation accept bounded per-token quote source-time jitter (WS/REST source mixing at tick level) while still rejecting true tape corruption. Refs #584.
The first tick-level hourly segment (7.7GB) was parked in
failed_segmentswithline 2446560: quote source time moved backwards— REST-batch quotes (ts 3.7s older) interleaved with WS quotes for the same token. The 1s sampling era masked this by construction.tsper token; larger regressions fail with the magnitude in the error message.tolerated_quote_source_regressions.recorded_atfile-order monotonicity, sequence gaps, and future-skew checks unchanged.Issue relationship
Refs #584
Out of scope
Dependencies and merge order
None.
Focused validation
cargo test -p hft-collector polymarket_upload::— 63 passed, 0 failed.Rollout and rollback
polymarket-raw-ops; deploy via the standard raw-ops release path, then clear the parked segment'sfailed_segmentsentry and re-run upload.Scope exception
None
Summary by CodeRabbit