fix(polymarket): bound retained reference recovery - #80
Conversation
|
Warning Review limit reached
Next review available in: 49 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 (3)
📝 WalkthroughWalkthroughThe collector adds bounded, rotating market-detail recovery and partitions the request budget between detail fetches and trade polling. Health metrics, policy validation, fixtures, mutation cases, and unit tests now cover the expanded recovery and budgeting state. ChangesMarket detail recovery and shared budgeting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant collect_once
participant tracked_market_state
participant Gamma
participant plan_trade_polls
collect_once->>tracked_market_state: identify recovery targets and stale detail attempts
collect_once->>Gamma: fetch scheduled market details
Gamma-->>collect_once: return market detail results
collect_once->>collect_once: apply details and calculate remaining budget
collect_once->>plan_trade_polls: plan trade polls with remaining budget
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 |
e9a2796 to
18d1202
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@deployment/aliyun/polymarket-rust-health-policy.jq`:
- Line 7: Enforce the recovery-target subset contract in
deployment/aliyun/polymarket-rust-health-policy.jq by requiring
state_recovery_markets to be no greater than target_markets while preserving the
existing non-negative integer validation. Add a targeted rejection mutation in
deployment/aliyun/test-polymarket-raw-ops-control-plane.sh around lines
1168-1181, such as setting state_recovery_markets to 121, and verify the health
policy rejects it.
In `@deployment/aliyun/test-polymarket-raw-ops-control-plane.sh`:
- Around line 1168-1181: Add a targeted rejection case in the fixture’s
policy-invariant test sequence using .state_recovery_markets = 121 for the
120-target fixture. Assert that the configuration is rejected, while preserving
the existing valid recovery-count cases and test structure.
In `@rust_hft/tools/collector/src/polymarket_raw.rs`:
- Around line 2973-2997: Extend
settled_trade_incomplete_state_recovers_without_market_detail to exercise the
full collector finalization path: configure trade polling for the recovered
market, assert settlement_available permits completion, verify no Gamma detail
request occurs, and confirm metadata/settlement records are emitted only once.
Assert the market advances from trade_complete false to true, using the existing
collector test harness and symbols rather than only testing
tracked_market_recovery_target and plan_market_detail_fetches.
🪄 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: de1660de-8ac8-4689-916e-49992e7a1d77
📒 Files selected for processing (3)
deployment/aliyun/polymarket-rust-health-policy.jqdeployment/aliyun/test-polymarket-raw-ops-control-plane.shrust_hft/tools/collector/src/polymarket_raw.rs
![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: e9a2796c84
ℹ️ 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 settlement = if state_recovery { | ||
| None |
There was a problem hiding this comment.
When a retained market is already settled but still has settlement_failure_since set (for example, after a crash between writing the settlement tape row and atomically updating collector-state.json; recover_state_from_active_tape marks it settled but does not clear that old error), this state-recovery branch skips both the detail refetch and settlement_from_market, so the stale-settlement check below continues to fail the collector forever for an already-settled market. Clear the settlement failure fields for state_recovery, or only synthesize recovery when no settlement failure is recorded.
Useful? React with 👍 / 👎.
| and (.market_detail_priority <= .market_detail_eligible) | ||
| and (.market_detail_selected == ([.market_detail_eligible, .market_detail_budget] | min)) | ||
| and (.market_detail_deferred == (.market_detail_eligible - .market_detail_selected)) | ||
| and (.market_detail_priority_deferred == ([.market_detail_priority - .market_detail_budget, 0] | max)) |
There was a problem hiding this comment.
When more priority detail recoveries exist than the detail budget (for example five unsettled retained markets with a budget of four), this policy still accepts market_detail_priority_deferred: 1 as long as the arithmetic is self-consistent; I verified such a health document passes this jq policy. That lets the cutover gate go green while a priority settlement/detail recovery backlog remains, unlike the trade path which explicitly requires priority_trade_backlog == 0. Add a fail-closed check that market_detail_priority_deferred == 0.
Useful? React with 👍 / 👎.
| if needs_detail { | ||
| let url = format!("{GAMMA_MARKET_URL}/{market_id}"); | ||
| match self.get_json(&url, &[]).await { | ||
| if needs_detail && market_detail_plan.selected.contains(&market_id) { |
There was a problem hiding this comment.
When retained detail recoveries exceed the 4-market detail budget and any unselected candidate is already stale or older than settlement_lookback_secs, this condition leaves those unselected markets untouched; the later stale_settlement_markets / overdue_unresolved_markets checks then return a DataCompletenessError, and run_reference exits before the next cycle can rotate to the deferred IDs. That makes the bounded recovery unable to converge for exactly the large retained backlog it is meant to handle unless all stale/overdue recoveries fit in one cycle.
Useful? React with 👍 / 👎.
1 participant
Change contract
A retained Polymarket reference state recovers stale market details and settled trade backfill without exceeding the existing per-cycle request budget, while the deployment health gate validates the same split budget before cutover.
Out of scope
Research database materialization, Binance aggTrade collection, prediction snapshot/evaluator execution, and any live-trading capability.
Dependencies and merge order
None.
Focused validation
cargo +1.91 test --manifest-path rust_hft/Cargo.toml --locked -p hft-collector --lib polymarket_raw::tests— 39 passed, 1 ignored watchdog subprocess target.cargo +1.91 clippy --manifest-path rust_hft/Cargo.toml --locked -p hft-collector --lib -- -D warnings.rustfmt +1.91 --edition 2021 --check rust_hft/tools/collector/src/polymarket_raw.rsandgit diff --check.bash deployment/aliyun/test-polymarket-raw-ops-control-plane.sh— passed.Rollout and rollback
Publish the Linux
polymarket-raw-opsartifact from the merged SHA, install it as a versioned release, and verify the retained production spool advanceshealth.jsonwithin the 180-second watchdog without deleting collector state. The health policy remains fail closed while priority backlog, API errors, stale markets, or unresolved settlements remain. Roll back through the existing versioned cutover evidence to the prior binary if the new cycle does not converge. No execution gateway or live-trading state changes.Scope exception
None. Three files and fewer than 750 non-generated changed lines.
Summary by CodeRabbit
New Features
Bug Fixes