fix(pm-tape): survive tick-level load without restart-looping - #582
Conversation
Refs #581 The tick-level tape config (quote_sample_ms=0, +spot_price/agg_trade/l2) pushed the recorder past its CPU/memory quota: 50% of cgroup periods throttled, memory pinned at MemoryHigh, consumer lagged the 8192-message broadcast ring, and LiveFeed's fail-closed lag handling killed the process on every burst (13 restarts in ~25 minutes). - Add LagPolicy to LiveFeed: fail_closed (default, unchanged for trading) or skip_and_continue for pure recorders, where a restart loses more tape than a bounded, warn-logged gap. - Add feed_broadcast_capacity / feed_lag_policy runtime config keys. - Tape config: capacity 65536, lag policy skip_and_continue. - Tape unit: CPUQuota 30%->100%, MemoryHigh 384M->768M, MemoryMax 512M->1G (host has 5.1G available). - Contract test pins the new tape config keys and quota headroom.
|
Warning Review limit reached
Next review available in: 6 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 (5)
📝 WalkthroughWalkthroughThe PR adds configurable live-feed lag handling and broadcast capacity. The runtime applies these settings to live feeds and channels. The market-tape deployment increases feed capacity and systemd resource limits. Tests verify the configuration. ChangesLive feed configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant RuntimeConfig
participant LiveRuntime
participant BroadcastChannel
participant LiveFeed
RuntimeConfig->>LiveRuntime: provide feed capacity and lag policy
LiveRuntime->>BroadcastChannel: create channel with configured capacity
LiveRuntime->>LiveFeed: construct with configured lag policy
BroadcastChannel->>LiveFeed: deliver market updates
LiveFeed->>LiveFeed: skip lagged updates or terminate
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust_hft/prediction-markets/crates/ploy-strategy-runtime/src/live.rs (1)
647-660: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep trading feeds fail-closed.
Both branches apply
config.feed_lag_policy()without checking the runtime role. A live trading configuration can selectskip_and_continue, andLiveFeedwill then evaluate after dropped market updates.Select
SkipAndContinueonly for the approved recorder workflow. Reject or override it for trading feeds. Add a test that a trading configuration remainsFailClosedwhen the config requestsskip_and_continue.🤖 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/prediction-markets/crates/ploy-strategy-runtime/src/live.rs` around lines 647 - 660, Ensure the LiveFeed construction in the live trading path never uses SkipAndContinue: allow that lag policy only for the approved recorder workflow, while rejecting or overriding it for trading feeds so they remain FailClosed. Update the relevant runtime-role/configuration handling around LiveFeed::with_lag_policy and add a test verifying a trading configuration requesting skip_and_continue resolves to FailClosed.
🤖 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.
Outside diff comments:
In `@rust_hft/prediction-markets/crates/ploy-strategy-runtime/src/live.rs`:
- Around line 647-660: Ensure the LiveFeed construction in the live trading path
never uses SkipAndContinue: allow that lag policy only for the approved recorder
workflow, while rejecting or overriding it for trading feeds so they remain
FailClosed. Update the relevant runtime-role/configuration handling around
LiveFeed::with_lag_policy and add a test verifying a trading configuration
requesting skip_and_continue resolves to FailClosed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: baea27da-b1bf-48d0-b1a9-ad90b4899163
📒 Files selected for processing (7)
deployment/aliyun/polymarket-market-tape.servicedeployment/aliyun/polymarket-market-tape.tomlrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/config.rsrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/live.rsrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/mod.rsrust_hft/prediction-markets/crates/ploy-strategy-runtime/src/live.rsrust_hft/prediction-markets/tests/workspace_runtime_retirement.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: 1afe58f475
ℹ️ 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".
Address review on #582: - Refuse feed_lag_policy = skip_and_continue unless the config is a pure noop dry-run recorder (feed_lag_policy_allowed); trading runtimes stay fail-closed by construction, with counterexample tests. - Reject feed_broadcast_capacity = 0 at startup via validated_feed_broadcast_capacity instead of panicking in tokio. - Bound the ring's worst-case byte footprint: capacity 32768 (4x, not 8x) plus MemoryHigh=1G / MemoryMax=1536M. Ring memory is backlog-only; worst case ~200-640MB on top of the ~770M observed working set stays under the cap.
1 participant
Change contract
Make
polymarket-market-tapesurvive tick-level recording (quote_sample_ms = 0, kinds +spot_price/agg_trade/l2) without restart-looping, while keeping fail-closed lag semantics for trading runtimes.Root cause (host-verified): 50% of cgroup CPU periods throttled under
CPUQuota=30%, memory pinned atMemoryHigh=384M, the consumer lagged the 8192-message broadcast ring, andLiveFeed's fail-closed lag handling killed the process on every burst (13 restarts in ~25 min).Changes:
LiveFeedgainsLagPolicy:fail_closed(default, unchanged) /skip_and_continue(warn-logged gap, keeps consuming). New tests cover both policies + serde names.feed_broadcast_capacity(default 8192) andfeed_lag_policy(defaultfail_closed), wired inploy-strategy-runtime.feed_broadcast_capacity = 65536,feed_lag_policy = "skip_and_continue".CPUQuota30%→100%,MemoryHigh384M→768M,MemoryMax512M→1G (host: 7.4G total, 5.1G available).Issue relationship
Refs #581
Out of scope
l2-kind upload-side validation; backlog drain; fix(collector): orphan segment recovery stalls startup unboundedly (usdm 3-day outage root cause) #552 / feat(research): accept tick-level + mixed-kind Polymarket tapes in research import and evidence validation #542 / ci(rust): make market-recorder release contract a parallel heavy job with docker layer caching #568.Dependencies and merge order
None. A matching resource drop-in is already live on the host as interim mitigation (0 restarts since); this PR makes it official and adds the lag-policy/capacity knobs, which take effect on the next recorder release.
Focused validation
cargo test -p ploy-strategy-bundles --lib— 242 passed, 0 failed (incl. 3 new lag-policy tests, with a fail-closed counterexample preserved).cargo check -p ploy-strategy-runtime— clean.cargo test --test workspace_runtime_retirement monday_polymarket_data_service— pass.Rollout and rollback
Scope exception
None