fix(prediction-markets): rotate market tape on size limit instead of disabling recording - #660
Conversation
…disabling recording Hitting RecordingLimits.max_bytes/max_records made RecordingFeed drop its writer and silently stop recording for the rest of the run, so the 24/7 market-tape recorder could not use the existing record_market_updates_max_bytes knob while hourly tapes reached 20-25 GiB. A LimitReached append now rotates through the same writer.rotate() path as rotate_seconds, replays the active-event checkpoints into the fresh tape (excluding the current discovery, which is appended right after), and then appends the current update, so no records are lost and every rotated tape still starts at sequence 0. If the checkpoint replay itself cannot fit under the cap, the existing fail-safe still disables the writer, since that is a genuine misconfiguration. Enable the cap in production: record_market_updates_max_bytes = 4294967296 (4 GiB) in polymarket-market-tape.toml. Refs #655
|
Warning Review limit reached
Next review available in: 57 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 (1)
📝 WalkthroughWalkthroughThe PR adds optional recording rotation when record or byte limits are reached. It configures a 4 GiB limit for the Aliyun market tape and preserves stop-recording behavior when rotation is disabled or cannot complete. ChangesRecording limit rotation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RuntimeConfiguration
participant RecordingFeed
participant TapeWriter
RuntimeConfiguration->>RecordingFeed: provide RecordingPolicy
RecordingFeed->>RecordingFeed: detect record or byte limit
RecordingFeed->>TapeWriter: rotate tape
RecordingFeed->>TapeWriter: replay lifecycle checkpoints
RecordingFeed->>TapeWriter: append update
🚥 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1ca63441e
ℹ️ 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".
…ckpoints before boundary updates Existing dry-run evidence configs use record_market_updates_max_records/ max_bytes as hard caps and rely on the legacy stop-recording behavior, so rotating on every limit broke their bounded-capture contract. Rotation is now opt-in via record_market_updates_rotate_on_limit (default false = legacy stop), gated in RecordingFeed::next and enabled in the production market-tape config. The size-limit checkpoint snapshot is now captured before the boundary update is processed, mirroring the rotation_due path: an EventExpired record that trips the cap no longer strips its event's EventDiscovered checkpoint from the fresh tape. The snapshot is only taken when limits and the flag are configured, so legacy and unlimited paths pay no per-tick clone. Refs #655
|
Both findings addressed in
|
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-strategy-bundles/src/feed/recorded.rs`:
- Around line 588-602: Update the size-rotation snapshot gate in the
recorded-update flow to arm only when the configured record or byte limit is
within its near-cap threshold, using the existing next_sequence and
bytes_written state plus NEAR_CAP_MARGIN_BYTES. Keep the check before
prepare_recorded_update so pre-boundary capture semantics remain unchanged, and
sort event_checkpoints only when the snapshot is non-empty to avoid unnecessary
work.
🪄 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: 59ca21db-a127-42e1-abbc-78dc89257aa4
📒 Files selected for processing (3)
deployment/aliyun/polymarket-market-tape.tomlrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/config.rsrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs
…imit rotation The pre-boundary checkpoint snapshot was cloned and sorted on every update whenever rotate_on_limit was set with any limit configured, which is the production market-tape config, while a size rotation fires once per 4 GiB. The snapshot is now taken only when rotation_due (unchanged) or when the writer is near a configured limit: bytes_written within 1 MiB of max_bytes (the margin must exceed the largest possible serialized record line; a smaller max_bytes counts as always near) or next_sequence within 1 of max_records. Far-from-cap ticks pay zero clones. Since LimitReached can only fire when the cap is actually exceeded, the snapshot is guaranteed to exist on the size-rotation path; a debug_assert plus a logged lazy recompute remains as a fail-safe so checkpoints are never silently dropped. Refs #655
|
Addressed in |
Change contract
The production Polymarket market-tape recorder (
new-ploy-runner,RecordingFeedinploy-strategy-bundles/feed/recorded.rs) treatedRecordingLimits.max_bytes/max_recordsas "stop recording for the rest of the run" — correct for bounded backtest replay capture, wrong for 24/7 production (and the reason the deployment toml could not safely set a cap while hourly tapes reached 20–25 GB in the 2026-08-03 incident). Hitting the limit now rotates the tape through the existingwriter.rotate()path, replays the active-event checkpoints in the same deterministic order as therotate_secondspath (excluding the currentEventDiscovered, whichprepare_recorded_updatealready folded in), then appends the current update — no records lost. Fail-safe preserved: if the checkpoint replay itself exceeds the cap, or rotation cannot stage, the writer still disables (genuine misconfiguration).record_market_updates_max_bytes = 4294967296(4 GiB) is set indeployment/aliyun/polymarket-market-tape.toml.Issue relationship
Refs #655
Out of scope
polymarket_raw.rs) — sibling PR (codex/recorder-tape-size-cap).monday-trade-data-26(runtime cutover, named-controller procedure); the code is safe to deploy independently because an unsetmax_byteskeeps single-tape behavior.Dependencies and merge order
None
Focused validation
recording_feed_rotates_after_record_limit_without_losing_updates(old tape keeps its records, new tape continues atsequence = 0, writer stays enabled — counterexample for the old silent-stop boundary).size_limit_rotates_tape_and_replays_active_event_checkpoints_first(4096-byte cap: fresh tape = checkpoint at seq 0, then remaining updates in order) andcheckpoint_replay_beyond_the_cap_disables_the_writer(fail-safe counterexample: nothing written past the cap).writer.rotate()resetsnext_sequence = 0(verified recorded.rs:367), so the uploaderstart_sequence == 0contract holds per rotated tape.cargo test -p ploy-strategy-bundles --locked(nested workspacerust_hft/prediction-markets): 247 lib passed / 0 failed + 6/6 backtest integration; all pre-existingrotate_secondsand no-limit tests green. Clippy: zero new warnings.git diff --checkclean; preflightverdict=ok.Rollout and rollback
No runtime change until the new
new-ploy-runnerbinary and toml value are delivered through the recorder cutover procedure; reverting the toml line restores hour-only rotation, and unsetmax_bytespreserves legacy single-tape behavior.Scope exception
None
Summary by CodeRabbit
New Features
Bug Fixes