fix(collector): buffer and replay CLOB deltas across the subscribe seam - #591
Conversation
A price_change arriving before a token's first book snapshot was dropped by the uninitialized apply() no-op yet still wrote last_timestamp, so an older snapshot then tripped the backwards-timestamp guard into a socket-wide reconnect, and deltas in the snapshot/subscription gap were never recovered, surfacing later as missing-BBA reconciliation hits. New tokens now start in a syncing state: pre-snapshot deltas are buffered per token (bounded by MAX_POLYMARKET_CLOB_PENDING_CHANGES, overflow fails the token closed) and publish nothing. The first book snapshot replaces the cache, replays buffered deltas newer than the snapshot in arrival order, reconciles the replayed BBA in-band (dirty isolation, never a batch Err), publishes a single post-replay quote, and rebuilds last_timestamp from the replayed watermark. Reconnects restart every token in syncing via the existing map clear, so stale last_timestamp state can no longer collide with an older resync snapshot. Syncing tokens emit no failure lines; #578 dirty semantics (fail-closed with failure plus empty quote until a healing snapshot) are unchanged for initialized tokens. Verification: cargo test -p ploy-market-data (71 passed) and with --features live (162 passed, incl. 5 new sync-seam tests), run under the pinned 1.91 toolchain; cargo fmt --check clean; clippy diagnostics byte-identical to base (36 pre-existing lines, none in touched code). Refs #453
|
Warning Review limit reached
Next review available in: 41 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)
📝 WalkthroughWalkthroughPolymarket CLOB handling now buffers pre-snapshot changes, replays changes newer than the snapshot, tracks replay timestamps, and fails closed per affected token. Book processing can emit multiple updates. Tests cover resubscription, stale changes, overflow, and isolated failures. ChangesPolymarket CLOB replay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLOBWebSocket
participant BookProcessing
participant ClobBookState
participant QuoteOutput
CLOBWebSocket->>BookProcessing: receive CLOB event
BookProcessing->>ClobBookState: buffer or apply price change
BookProcessing->>ClobBookState: replay changes after snapshot
BookProcessing->>QuoteOutput: emit quote updates and watermark
BookProcessing->>QuoteOutput: emit failed-closed update for dirty token
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
🤖 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/prediction-markets/crates/ploy-market-data/src/feeds.rs`:
- Around line 1183-1190: Update the replay filter in the pending-delta loop to
skip only timestamps strictly earlier than book.timestamp, matching the live
path’s boundary behavior. Ensure deltas sharing the snapshot millisecond are
passed to state.apply and update watermark/last_replayed normally.
🪄 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: 8a39d0ad-e161-440e-90d5-f39deb35b1ec
📒 Files selected for processing (1)
rust_hft/prediction-markets/crates/ploy-market-data/src/feeds.rs
The buffered-delta replay filter dropped entries sharing the snapshot millisecond while the live path applies any batch whose timestamp is not strictly older than the snapshot, so a legal same-millisecond delta left the replayed cache behind the server. Align the replay boundary with the live path: only strictly older deltas are dropped. Verification: cargo test -p ploy-market-data (71 passed) and with --features live (163 passed, incl. new boundary counterexample clob_replay_applies_same_millisecond_deltas_like_the_live_path), run under the pinned 1.91 toolchain; cargo fmt --check clean; clippy diagnostics unchanged from base (36 pre-existing lines). Refs #453
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66faf615ea
ℹ️ 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".
|
Boundary aligned in aae5c7a: replay now only drops deltas strictly older than the snapshot (), matching the live path; same-millisecond deltas replay. New counterexample clob_replay_applies_same_millisecond_deltas_like_the_live_path locks both directions. live suite 163 passed. |
…ering Four replay-path defects found in review, each with a counterexample test: 1. A replayed book could cross when buffered entries lack BBA fields and then publish, while the steady path hard-rejects crossed books. The final replayed quote now faces the same crossed-book check, with in-band dirty isolation (failure plus empty quote) instead of a batch error: the replay path is itself the resync path, so a hard error would only retrigger the reconnect loop it serves. 2. The replayed quote was stamped with the snapshot timestamp while carrying newer replayed state; it now carries the replay watermark. 3. BBA reconciliation only checked the last buffered entry, so an earlier message's missing-level evidence was drowned by later messages. Every replayed message's final entry is now reconciled, and any miss isolates the token. 4. Buffered messages replayed in arrival order without regression protection, letting an older message overwrite newer state while the watermark kept the newer timestamp. Messages strictly older than the applied watermark are now skipped, matching steady-state last_timestamp semantics, and the watermark tracks the newest actually-applied timestamp. Verification: cargo test -p ploy-market-data (71 passed) and with --features live (167 passed, incl. 4 new replay counterexamples), run under the pinned 1.91 toolchain; cargo fmt --check clean; clippy diagnostics unchanged from base (36 pre-existing lines). Refs #453
|
All four P1 findings addressed in a3bca12: (1) post-replay quote is crossing-validated and fails closed per-token instead of publishing a crossed book; (2) replayed quote carries the watermark timestamp; (3) reconciliation runs per buffered message boundary, not just the final entry; (4) replay applies a watermark-monotonic rollback policy equivalent to the live path. Each locked by a dedicated counterexample test; live suite 167 passed. |
Change contract
Polymarket CLOB subscription sync is now WS-native buffer-and-replay: a (re)subscribed token buffers incoming
price_changedeltas (bounded, 256/token) without applying or publishing, and on its firstbooksnapshot applies the snapshot, replays buffered deltas newer than the snapshot in order, rebuilds the timestamp watermark, and only then starts publishing. This eliminates the subscribe-seam race where swallowed pre-snapshot deltas both poisonedlast_timestamp(backwards-book hard errors) and permanently dropped book levels (BBA-missing dirty events). The first snapshot of a syncing token is authoritative and exempt from the backwards-timestamp guard; healthy initialized tokens keep the guard; #578 dirty isolation/self-heal semantics are unchanged.Out of scope
bookmessages).Dependency or merge order
Builds on merged PR #578 (dirty-token isolation). Single commit on
c27c4497.Focused validation
clob_price_change_before_first_snapshot_is_buffered_then_replayed: out-of-order subscribe (delta before book) no longer errors or publishes; the delta is replayed after the snapshot.clob_replayed_delta_fills_snapshot_gap_so_bba_reconciles: a buffered delta introducing a new top level is applied after the snapshot, so a later reported BBA reconciles (no spurious dirty).clob_replay_drops_changes_older_than_the_snapshot: stale buffered deltas are discarded.clob_resubscription_rebuilds_sync_state_without_backwards_errors: reconnect/resubscribe resets sync state; a re-sent older snapshot no longer trips the backwards guard.clob_syncing_buffer_overflow_fails_closed: >256 buffered deltas fails closed per-token (dirty + failure + empty quote), bounded memory, no panic.cargo test -p ploy-market-data: 71 passed;--features live: 162 passed (pinned 1.91, fromprediction-markets/).cargo fmt --checkclean. Clippy A/B-identical to base (zero new warnings; three pre-existingderivable-implslints inploy-market-contractsexempted).Production motivating evidence (issues #313/#453 comments): the 0803a run had 138 isolated failure rows from the subscribe-seam race plus 2 rows from a backwards-book hard error after a transport reconnect — both classes are removed by this change.
Rollout/rollback impact
Feed behavior change at the subscribe seam only; steady-state processing untouched. New tokens publish nothing until their first snapshot arrives (previously they published entry-reported BBA immediately — a fail-open edge this contract intentionally closes). Rollback = revert and rebuild the recorder image.
Issue relationship
Refs #453
Summary by CodeRabbit