fix(collector): deduplicate tape trade rows during upload scan - #746
Merged
Merged
Conversation
The upload scanner hard-failed any tape containing a duplicate polymarket_trade record_id. Crash recovery legitimately re-emits already-recorded trades when a market re-finalizes (state migration reopens markets without a completion proof), so every tape written after a collector restart can carry hundreds of content-identical re-emissions and the whole upload chain wedges behind them. A record id is a SHA-256 over the complete trade identity, so a repeated row is lossless to deduplicate. Count them in the scan manifest as duplicate_trade_rows evidence instead of refusing the segment, mirroring the collector-side fix in #743. Genuinely malformed rows still fail closed.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
proerror77
enabled auto-merge (squash)
August 8, 2026 01:00
📝 WalkthroughWalkthroughThe tape scanner now accepts duplicate ChangesPolymarket trade deduplication
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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)
4517-4520: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtend the regression test for the remaining trade contracts.
This test proves duplicate acceptance and row counts. Add a case that preserves the unique-set
trade_countandtrade_record_ids_sha256completion evidence. Add another case where a repeated row fails canonical validation. These cases protect fail-closed validation and set-based completion semantics.🤖 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 4517 - 4520, Extend the regression test around scan_tape to assert the unique-set completion fields trade_count and trade_record_ids_sha256 for the existing accepted trades. Add a fixture case with a repeated trade row that fails canonical validation, and assert the scan rejects it rather than accepting it. Preserve the current duplicate_trade_rows, event_types, and events assertions.
🤖 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 1766-1769: Update the manifest construction near
duplicate_trade_rows so the duplicate count is recorded under the canonical
quality field, or remove the unused duplicate-count emission entirely. Eliminate
any duplicate_record_ids manifest payload that is always exported as 0, while
preserving the actual duplicate_trade_rows tracking.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/polymarket_upload.rs`:
- Around line 4517-4520: Extend the regression test around scan_tape to assert
the unique-set completion fields trade_count and trade_record_ids_sha256 for the
existing accepted trades. Add a fixture case with a repeated trade row that
fails canonical validation, and assert the scan rejects it rather than accepting
it. Preserve the current duplicate_trade_rows, event_types, and events
assertions.
🪄 Autofix
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: 69bc8158-91ed-4f7f-8df1-ca4b10d8881b
📒 Files selected for processing (1)
rust_hft/tools/collector/src/polymarket_upload.rs
This was referenced Aug 8, 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
The upload scanner (
scan_tape_with_identity_at) hard-failed any tape containing a duplicatepolymarket_traderecord_id. But crash recovery legitimately re-emits already-recorded trades: state migration reopens markets without a completion proof (completion_migration_reopens_v2_markets_without_a_proof), the market re-finalizes, and its full trade snapshot is emitted again. During the 2026-08-08 incident every tape written after a collector restart carried hundreds of content-identical re-emissions (measured: 565 in the active tape, more in closed tapes), so the upload chain hard-wedged — no tape could ever be archived.A record id is a SHA-256 over the complete trade identity (tx hash, condition, asset, side, timestamp, wallet, size, price, outcome index), so a repeated row is lossless to deduplicate. This PR mirrors the collector-side #743 fix at the upload layer: duplicates are counted in the scan manifest as
duplicate_trade_rowsevidence instead of refusing the segment. The set-based completion digest (trade_record_ids_sha256over the id set) is unaffected because dedup does not change the set. Genuinely malformed rows (bad price/side/size, missing fields) still fail closed.Issue relationship
None
Out of scope
Dependencies and merge order
None
Focused validation
rejects_duplicate_trade_record_idsreplaced byduplicate_trade_record_ids_are_deduplicated_into_evidence: a tape with a repeated trade id now scans successfully withduplicate_trade_rows == 1, row counts preserved.cargo test -p hft-collector --locked polymarket_upload— 87 passed, 0 failed; clippy-D warningsclean.Rollout and rollback
No production impact at merge. After merge the next ACR artifact carries the fix; the upload chain (currently drained manually) resumes against the remaining dup-carrying tapes. Rollback is revert — tapes keep accumulating, which is the pre-change state.
Scope exception
None
Summary by CodeRabbit