fix(collector): overlap bounded trade polling - #37
Conversation
|
Warning Review limit reached
Next review available in: 44 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 (4)
📝 WalkthroughWalkthroughThe Polymarket reference collector now supports bounded concurrent trade polling with shared request-start spacing. CLI configuration, runtime health output, deployment validation, control-plane tests, and Aliyun documentation reflect the new concurrency and minimum-spacing constraints. ChangesBounded Polymarket trade polling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant collect_once
participant ReferenceCollector
participant DataAPI
participant next_state
collect_once->>ReferenceCollector: Schedule bounded trade fetches
ReferenceCollector->>ReferenceCollector: Wait for shared request slot
ReferenceCollector->>DataAPI: Start spaced trade request
DataAPI-->>ReferenceCollector: Return market trades
ReferenceCollector->>next_state: Apply validated result
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bd03b298a
ℹ️ 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".
| .collect::<Vec<_>>() | ||
| .await; |
There was a problem hiding this comment.
When a cycle selects many busy markets, this collect retains every fetched Vec<Value> until all selected requests finish. fetch_trades can return two 10k-row pages per market, so with the 192-market budget the peak memory becomes the whole cycle's raw trade payloads rather than the four in-flight requests; the deployed service is memory-capped, so a burst of large markets can OOM/restart before any results are applied. Consider draining the buffered stream as results arrive or otherwise bounding stored results.
Useful? React with 👍 / 👎.
| } | ||
| (market_id, result) | ||
| }) | ||
| .buffer_unordered(self.config.max_concurrent_trade_polls) |
There was a problem hiding this comment.
With the default concurrency of four and a fast trades endpoint, buffer_unordered polls four fetch futures immediately, and the 100ms sleep inside each future only delays replacement of that slot; this changes the old per-market pacing from roughly one trade request every 100ms to bursts of up to four requests at once (and then up to four more per delay interval). In environments where per_market_delay is the rate-limit guard for the Polymarket Data API, this can trigger 429s/stale trade health even though the configured delay appears unchanged.
Useful? React with 👍 / 👎.
| if self.max_concurrent_trade_polls > self.max_trade_polls_per_cycle { | ||
| bail!("concurrent trade polls cannot exceed the per-cycle trade poll budget"); |
There was a problem hiding this comment.
When an operator lowers max_trade_polls_per_cycle below the new default concurrency of 4 (for example --max-trade-polls-per-cycle 1 for throttled testing), this validation now rejects a previously valid bounded configuration unless they also discover and set the new concurrency flag. If the budget is intended to remain independently tunable, the default concurrency should be clamped to the budget or the check should account for the defaulted value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional fail-closed configuration contract: when an operator lowers the per-cycle budget, they must also lower the explicit concurrency cap. We do not silently clamp because health evidence must report the configured/effective value identically; both config validation and the health policy enforce concurrency <= budget, and the reduced-budget case is covered by a unit test.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust_hft/tools/collector/src/polymarket_raw.rs (1)
2233-2272: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftExercise the runtime scheduler, not only chunk partitioning.
Add an async regression test that records peak in-flight requests and request-start spacing, including a market requiring its second page. These are the central new runtime guarantees.
🤖 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_raw.rs` around lines 2233 - 2272, The existing target_chunks_preserve_order_and_bound_retained_selected_results test only validates partitioning; add an async regression test that drives the runtime scheduler and includes a market requiring a second page. Instrument the request path to record peak in-flight requests and request-start timestamps, then assert the scheduler enforces the configured concurrency and minimum spacing while preserving correct multi-page processing.
🤖 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_raw.rs`:
- Around line 1523-1547: Enforce the 180,000 ms absolute bootstrap/cycle
deadline around the trade-fetch loop in the collector flow at
rust_hft/tools/collector/src/polymarket_raw.rs:1523-1547, or validate the
configured timeout, budget, and concurrency so completion cannot exceed it.
Update the health policy at
deployment/aliyun/polymarket-rust-health-policy.jq:7-10 to reject
cycle_duration_ms values above 180,000, and extend
deployment/aliyun/test-polymarket-raw-ops-control-plane.sh:354-358 with a
mutation setting cycle_duration_ms to 180001 and asserting rejection.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/polymarket_raw.rs`:
- Around line 2233-2272: The existing
target_chunks_preserve_order_and_bound_retained_selected_results test only
validates partitioning; add an async regression test that drives the runtime
scheduler and includes a market requiring a second page. Instrument the request
path to record peak in-flight requests and request-start timestamps, then assert
the scheduler enforces the configured concurrency and minimum spacing while
preserving correct multi-page processing.
🪄 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
Run ID: ad6471a0-df9c-40ca-9004-bdeba4a0ae8c
📒 Files selected for processing (5)
deployment/aliyun/README.mddeployment/aliyun/polymarket-rust-health-policy.jqdeployment/aliyun/test-polymarket-raw-ops-control-plane.shrust_hft/tools/collector/src/bin/polymarket-raw-ops.rsrust_hft/tools/collector/src/polymarket_raw.rs
1 participant
Tokyo runtime evidence showed the 192-poll bounded cycle still missed the 180-second bootstrap deadline because Data API requests remained serial. The formal shadow gate failed closed and left Python production untouched.\n\nThis change keeps the 192-market cycle budget and 100ms per-request delay, but overlaps at most four trade requests. Health records and validates the concurrency bound. The request fan-out remains fixed and cannot exceed the per-cycle budget.\n\nVerified locally:\n- hft-collector lib: 82 passed\n- polymarket-raw-ops CLI: 1 passed\n- strict targeted Clippy: passed\n- Aliyun control-plane shell test: passed with exit 0\n- targeted rustfmt and diff check: passed\n\nNo production service, ACK node, ClickHouse instance, or trading ECS was started by this PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation