fix(collector): bound Polymarket reference trade state - #680
Merged
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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)
📝 WalkthroughWalkthroughChangesPolymarket snapshot recovery
Estimated code review effort: 4 (Complex) | ~45 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 |
proerror77
force-pushed
the
codex/pm-reference-bounded-state
branch
from
August 4, 2026 04:01
21372ce to
63581cb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21372ce759
ℹ️ 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".
proerror77
force-pushed
the
codex/pm-reference-bounded-state
branch
from
August 4, 2026 04:10
63581cb to
4ebfa97
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
rust_hft/tools/collector/src/polymarket_raw.rs (1)
2846-2851: 📐 Maintainability & Code Quality | 🔵 TrivialExpose recovery state in the health payload.
The health output dropped the trade-ID eviction fields and reports no replacement. Deferred rotation is now an invisible state: the active tape stops rotating while
recovery_pendingis true, and operators see only a growingactive_tape_bytes. Add the unresolved recovery counts so alerting can detect a stalled rotation before the byte cap fails the cycle.📈 Suggested health fields
"malformed_trade_rows": malformed_trade_reasons.values().sum::<u64>(), "malformed_trade_reasons": malformed_trade_reasons, + "recovery_pending": self.writer.recovery_pending, + "recovered_trade_conditions": self.state.recovered_trade_ids.len(), + "recovered_trade_ids": self.state.recovered_trade_ids.values().map(BTreeSet::len).sum::<usize>(),🤖 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 2846 - 2851, Update the health payload construction near the existing trade poll and malformed-row fields to expose the unresolved trade-ID eviction/recovery counts, including the recovery-pending replacement state used by deferred rotation. Reuse the existing recovery counters from the surrounding collector logic so operators can detect stalled rotation alongside active_tape_bytes.
🤖 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 1479-1487: Update retain_requested_market_state to also remove
recovered_trade_ids entries whose condition_id belongs to a market pruned from
state.markets, while preserving entries for retained markets. Use the market
condition identifiers available in CollectorState and keep the existing return
value based on whether tracked market state changed.
- Around line 1995-2008: Update trade_completion_update to merge
snapshot.record_ids with state.recovered_trade_ids for the given condition_id
before computing trade_count and trade_record_ids_sha256. Use this union for the
completion digest so recovered trade rows remain represented when absent from
the current API snapshot.
- Around line 2800-2802: In the recovery flow around updates.replay, preserve
the existing recovery_pending value while replaying the batch so pending hour
rotation cannot occur before recovered rows are durable. Move the assignment
derived from next_state.recovered_trade_ids to after updates.replay succeeds,
before persisting the updated state, while retaining the current replay and
error-propagation behavior.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/polymarket_raw.rs`:
- Around line 2846-2851: Update the health payload construction near the
existing trade poll and malformed-row fields to expose the unresolved trade-ID
eviction/recovery counts, including the recovery-pending replacement state used
by deferred rotation. Reuse the existing recovery counters from the surrounding
collector logic so operators can detect stalled rotation alongside
active_tape_bytes.
🪄 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: 1e21da04-c05b-405f-8fe6-0746b3eb5e11
📒 Files selected for processing (2)
rust_hft/tools/collector/src/bin/polymarket-raw-ops.rsrust_hft/tools/collector/src/polymarket_raw.rs
proerror77
force-pushed
the
codex/pm-reference-bounded-state
branch
from
August 4, 2026 06:06
7a6a29d to
7586937
Compare
proerror77
added a commit
that referenced
this pull request
Aug 7, 2026
…polls (#743) The data API paginates a live trade feed (offset 0 and 10000), so the two pages routinely overlap and report the same trade twice. Since #680 the collector rejected any poll containing a duplicate record id as malformed; on real traffic that marked hundreds of markets failed, tripped the stale_after fail-closed exit, and the host shadow gate correctly refused the candidate. A duplicate record id is a SHA-256 over the full trade identity, so the repeated row is content-identical to the one already kept. Deduplicate it (exactly what the pre-#680 code did) and surface the count as a dedicated duplicate_trade_rows health metric instead of a malformed reason, keeping the poll fail-closed for genuinely malformed rows. This also restores compliance with the gate policy malformed_trade_rows == 0 contract. Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
This was referenced Aug 8, 2026
proerror77
added a commit
that referenced
this pull request
Aug 8, 2026
…seconds (#757) * feat(polymarket-release): extend the shadow gate observation to 3600 seconds Since #680 the collector emits trades only after settlement plus the 1800-second finalization lag plus stable polls, so a 900-second observation always produced an empty shadow trade set and the gate's trade coverage parity could never pass (issue #756). Extend the production gate duration to 3600 seconds, matching the bybit and usdm reference gates, so markets closing early in the observation finalize before the parity cutoff. Policy keeps the one-second rounding tolerance (3600-3601) and the 601-second parity tail. * test(polymarket-release): align control-plane fixtures with the 3600-second gate The health-age boundary acceptance case and the runtime budget assertion encoded the 900-second duration; update the elapsed spans and the computed budget (1200+3600+5520=10320) to the new observation length. * fix(polymarket): cover deferred trade finalization in the parity trade window Address the review on the original change: lengthening only the gate observation still cannot make exact trade parity attainable, because verify-shadow-parity retained legacy trades through cutoff-600 while the collector emits trades only after settlement plus the 1800-second finalization lag plus three stable polls (~2100 seconds after close). Any 5/15-minute market closing inside (cutoff-1800, cutoff-600] leaves legacy-only trades in the window. The trade maturity lag becomes 2400 seconds (finalization 1800 + market window 900 with poll/settlement margin) and moves from a hard-coded constant into ShadowParityConfig, sourced from DEFAULT_TRADE_MATURITY_LAG_SECONDS (2400 in production, 600 in unit fixtures). verify-shadow-parity gains an optional --trade-maturity-lag-seconds flag so mechanism tests keep their small fixture epochs; the production gate never passes it, and the gate policy pins metrics.trade_maturity_lag_seconds == 2400 plus the derived window bindings. Also re-span the affected control-plane fixtures: health-age boundary cases to 3600 elapsed seconds with parity windows wider than the new lag, the baseline health completion chain past the parity window end, the delayed post-cutoff trade row past the new cutoff, the main gate fixture's parity window end to 3000, and the stale-health fixture's completion to 01:46:41 so its 2701-second age check stays the discriminating predicate. --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
This was referenced Aug 14, 2026
polymarket shadow gate trade parity unsatisfiable for post-#680 candidates vs pre-#680 baseline
#868
Closed
proerror77
pushed a commit
that referenced
this pull request
Aug 14, 2026
- initialize the finalization maxima once to null; an empty second assignment made the first jq fold die before parity - classify collector emission mode by the collect-reference CLI contract: #680 removed --max-retained-trade-ids exactly when finalization-deferred emission replaced per-poll emission, so the probe binds to #680 rather than the later post-#743 health counter, and any probe uncertainty classifies continuous (fail-closed, full trade parity) - require finalization advancement, not existence: a positive growing stable-poll maximum (zero until the 1800-second lag elapses) or a growing settled maximum, mirrored in the gate policy, with flat-counter forged evidence rejected and single-channel growth accepted in the control-plane contract tests Refs #868
proerror77
added a commit
that referenced
this pull request
Aug 14, 2026
…rity (#875) * fix(deploy): adjudicate unsatisfiable Polymarket shadow gate trade parity A finalization-deferred shadow (post-#680) emits a market's trades only after settlement plus the 1800-second finalization lag plus stable polls, so against a continuous-emission baseline no mature shadow trade can exist inside the 3600-second gate window: the trade coverage/field/byte parity trio is unsatisfiable by construction and every post-#680 legacy_overlap gate has failed with rust_trade_count=0 despite green metadata, settlement, rotation, asset, and dedupe parity. Detect both emission modes from the collector health schema. In the mixed case, replace the trio with equivalent-strength satisfiable evidence: the remaining parity families must pass, the raw verifier failure must be confined to the trade family, the shadow must emit no trade the baseline lacks, the finalization pipeline must engage (tracked/settled/stable-poll counters from the shadow state, eviction-safe running maxima), and the canonical upload still runs. The verdict records the applied trade parity mode, both emission modes, the raw verifier checks, and the reason. Same-semantics gates keep full trade parity unchanged. Closes #868 * fix(deploy): reword gate comment to satisfy Rust-only source enforcement * fix(deploy): address shadow gate parity review threads - initialize the finalization maxima once to null; an empty second assignment made the first jq fold die before parity - classify collector emission mode by the collect-reference CLI contract: #680 removed --max-retained-trade-ids exactly when finalization-deferred emission replaced per-poll emission, so the probe binds to #680 rather than the later post-#743 health counter, and any probe uncertainty classifies continuous (fail-closed, full trade parity) - require finalization advancement, not existence: a positive growing stable-poll maximum (zero until the 1800-second lag elapses) or a growing settled maximum, mirrored in the gate policy, with flat-counter forged evidence rejected and single-channel growth accepted in the control-plane contract tests Refs #868 --------- Co-authored-by: Sonic Shih <sonic.shih@mandonothing.com>
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change contract
Remove the obsolete persisted global Polymarket trade-ID map, cap, and eviction path. Track bounded per-market full-snapshot identity, emit a settled market’s trades once only after lag plus consecutive stable snapshots, and retain only bounded active-tape recovery IDs so restarts cannot refetch already-written rows.
Issue relationship
Refs #589
Out of scope
Production deployment or restart; OSS publication/readback; Polymarket market-tape collector; uploaders; Binance Spot or USDM collectors.
Dependencies and merge order
None
Focused validation
Counterexamples cover legacy retained-ID state removal, stable settlement before emission, bounded active-tape recovery, and preservation across two consecutive restarts without duplicate emission.
Rollout and rollback
No automatic production action. The runtime-labeled #589 remains open until its separately controlled production survival and readback acceptance evidence exists. Runtime restart and independent OSS data/manifest/_SUCCESS readback require a separately named controller and gate after merge. Roll back by reverting this commit before any runtime restart.
Scope exception
Approved by the user/coordinator for this atomic two-file collector safety contract. The line threshold is crossed because the obsolete persisted-ID state, cap, eviction implementation, callers, CLI option, and dead tests are deleted end-to-end while focused snapshot/recovery coverage replaces them; splitting would leave a known duplicate or crash path.
Summary by CodeRabbit
Bug Fixes
Tests