feat(collector): replace Polymarket raw ops with Rust - #34
Conversation
|
Warning Review limit reached
Next review available in: 31 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 Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis change ports Polymarket reference collection, tape upload, and shadow parity verification to Rust. It adds gated cutover and rollback automation, switches systemd and container packaging to ChangesPolymarket Rust runtime
Polymarket control plane
Runtime migration integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ShadowGate
participant Cutover
participant RustOps
participant OSS
Operator->>ShadowGate: submit candidate binary and revision
ShadowGate->>RustOps: run shadow collection and parity verification
RustOps->>OSS: upload validated shadow artifacts
OSS-->>ShadowGate: return verified artifact readback
ShadowGate-->>Operator: publish gate.json and eligibility marker
Operator->>Cutover: start gated cutover
Cutover->>RustOps: activate collector and upload units
RustOps-->>Cutover: return health and runtime evidence
Cutover-->>Operator: publish cutover.json or restore legacy runtime
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: f1f0cdea5d
ℹ️ 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".
| let settlement_parity = !legacy_settlement_ids.is_empty() | ||
| && legacy_settlement_ids.is_subset(&rust_settlement_ids) | ||
| && shared_values_match(&legacy_settlements, &rust_settlements)?; |
There was a problem hiding this comment.
When the Rust shadow emits any additional market_settlement rows in the parity window, this still passes because the check only requires the legacy IDs to be a subset of the Rust IDs and shared_values_match only compares the intersection. That means an over-broad Rust settlement detector can add false settlements while settlement_parity and therefore byte_parity remain true, allowing the cutover gate to approve corrupted settlement evidence; this should require equal settlement ID sets or explicitly reject Rust-only settlement IDs.
Useful? React with 👍 / 👎.
| and (.metrics.oss_uploaded_segments | positive_integer) | ||
| and (.metrics.oss_canonical_uploaded_segments | positive_integer) | ||
| and (.metrics.market_oss_uploaded_segments | positive_integer) | ||
| and (.metrics.market_oss_canonical_uploaded_segments | positive_integer) |
There was a problem hiding this comment.
If a shadow run uploads multiple UTC-hour chunks and only one chunk has complete metadata context, the gate still passes because it only checks that both total and canonical counts are positive. For example oss_uploaded_segments=2 with oss_canonical_uploaded_segments=1 is accepted even though one read-back segment was explicitly noncanonical, so a gate can approve incomplete uploaded evidence; compare the canonical count with the corresponding uploaded count for both reference and market uploads.
Useful? React with 👍 / 👎.
| if timestamp < cutoff || seen.contains_key(&record_id) { | ||
| continue; |
There was a problem hiding this comment.
If the Data API returns a syntactically valid integer timestamp that is not seconds since epoch, such as milliseconds, this only applies the lower cutoff and accepts the row. The collector can then persist far-future trade_ts/trade_ts_unix values and mark the ID as seen while health stays clean, leaving corrupted tape data or a segment the uploader later rejects if the timestamp is outside Chrono's range; reject timestamps greater than the poll time before inserting the update.
Useful? React with 👍 / 👎.
| systemctl start "$REFERENCE_UPLOAD_UNIT" | ||
| verify_oneshot_success "$REFERENCE_UPLOAD_UNIT" \ | ||
| || die 'legacy reference uploader drain did not complete successfully' | ||
|
|
||
| systemctl stop "$COLLECTOR_UNIT" |
There was a problem hiding this comment.
If the legacy reference upload drain takes long enough for the Python collector to restart or hit RuntimeMaxSec, the cutover proceeds straight to stopping the collector without revalidating the gated PID/restart count. That allows rows written after the gate by a different legacy process to become the handoff state even though the script only proved legacy_pid before the potentially long drain; run verify_legacy_runtime "$legacy_pid" again after the drain succeeds and before stopping the unit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
rust_hft/tools/collector/src/polymarket_upload.rs (1)
1-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCore Polymarket domain logic is re-implemented three times instead of shared.
The symbol/alias table, the 5m/15m window-inference heuristics, the settlement winner/tolerance math, the trade-ID content hash, and the tape-rotation-filename/file-identity checks are each duplicated verbatim (or near-verbatim under different names, e.g.
stable_trade_idvsderived_trade_record_id) across the collector, uploader, and shadow-parity comparator. All three copies are currently in sync, but this PR's entire safety model depends on the collector (producer), uploader (validator), and shadow-parity comparator (byte-for-byte checker) agreeing exactly — a future fix applied to only one copy would silently reintroduce the exact drift the parity mechanism exists to catch, and the current design has no single source of truth to prevent that.
rust_hft/tools/collector/src/polymarket_upload.rs#L1-L40: extractSUPPORTED_SYMBOL_ALIASES,SETTLEMENT_PRICE/SETTLEMENT_LOSER_PRICE/SETTLEMENT_SUM_TOLERANCE,value_text/canonical_decimal(260-275), the window-inference helpers (302-362),derived_trade_record_id(392-405), the settlement-validation algorithm (549-614), and the rotation-name/FileIdentityhelpers (93-101, 643-682) into a shared module.rust_hft/tools/collector/src/polymarket_raw.rs#L1-L41: replaceSYMBOL_ALIASES, the settlement constants,value_text/canonical_decimal(231-246),stable_trade_id(248-261), the window-inference helpers (277-331), andsettlement_from_market's winner/tolerance logic (619-735) with the shared implementations instead of a second copy.rust_hft/tools/collector/src/polymarket_parity.rs#L14-L79: replaceEXPECTED_SYMBOLSandFileFingerprintwith the shared symbol table and shared file-identity type instead of a third copy.🤖 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 1 - 40, Create one shared Polymarket domain module and move the symbol aliases, settlement constants and validation math, canonicalization helpers, window-inference helpers, trade-ID hashing, and rotation/file-identity types into it. In rust_hft/tools/collector/src/polymarket_upload.rs lines 1-40, make the collector use those shared symbols and implementations; in rust_hft/tools/collector/src/polymarket_raw.rs lines 1-41, replace SYMBOL_ALIASES, settlement logic, value_text/canonical_decimal, stable_trade_id, and window helpers with the shared versions; in rust_hft/tools/collector/src/polymarket_parity.rs lines 14-79, replace EXPECTED_SYMBOLS and FileFingerprint with the shared symbol table and file-identity type.
🤖 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 `@deployment/aliyun/polymarket-raw-ops-shadow-gate.sh`:
- Around line 468-473: Update the verify-shadow-parity invocation in the shadow
parity flow to execute the candidate binary as hftcollector rather than root.
Ensure the generated parity output is validated and then copied into the
root-owned evidence directory with appropriate ownership and permissions before
downstream use, while preserving the existing failure handling.
- Around line 242-246: Update the fixed-directory validation around
direct_directory_or_absent and the corresponding checks at the referenced
release/evidence paths to require root ownership and no group/world write
permissions for control-plane parent directories, including $RELEASE_ROOT and
$EVIDENCE_ROOT. Preserve rejection of indirect directories and absent-path
handling while applying the same ownership and permission enforcement to all
listed immutable release and evidence parents.
In `@deployment/aliyun/polymarket-reference-upload.service`:
- Line 15: Replace the unlimited TimeoutStartSec setting with a finite
five-minute start timeout in
deployment/aliyun/polymarket-reference-upload.service at lines 15-15 and
deployment/aliyun/polymarket-market-tape-upload.service at lines 15-15, so hung
oneshot runs terminate and retries can start.
In `@rust_hft/tools/collector/src/bin/polymarket-raw-ops.rs`:
- Around line 160-176: Validate zstd_timeout and oss_timeout through the same
positive-duration guard used for the collector timing arguments before
constructing UploadConfig. Reject explicit zero values while preserving the
existing defaults and convert only validated values to Duration::from_secs.
---
Nitpick comments:
In `@rust_hft/tools/collector/src/polymarket_upload.rs`:
- Around line 1-40: Create one shared Polymarket domain module and move the
symbol aliases, settlement constants and validation math, canonicalization
helpers, window-inference helpers, trade-ID hashing, and rotation/file-identity
types into it. In rust_hft/tools/collector/src/polymarket_upload.rs lines 1-40,
make the collector use those shared symbols and implementations; in
rust_hft/tools/collector/src/polymarket_raw.rs lines 1-41, replace
SYMBOL_ALIASES, settlement logic, value_text/canonical_decimal, stable_trade_id,
and window helpers with the shared versions; in
rust_hft/tools/collector/src/polymarket_parity.rs lines 14-79, replace
EXPECTED_SYMBOLS and FileFingerprint with the shared symbol table and
file-identity type.
🪄 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
Run ID: aefbc3ff-97e5-443b-acbb-abc0033b026a
📒 Files selected for processing (29)
.github/workflows/acr-publish.yml.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/aliyun/polymarket-legacy-health-policy.jqdeployment/aliyun/polymarket-market-tape-upload.servicedeployment/aliyun/polymarket-raw-ops-cutover.shdeployment/aliyun/polymarket-raw-ops-shadow-gate.shdeployment/aliyun/polymarket-reference-collector-shadow@.servicedeployment/aliyun/polymarket-reference-collector.servicedeployment/aliyun/polymarket-reference-upload.servicedeployment/aliyun/polymarket-rust-health-policy.jqdeployment/aliyun/polymarket-shadow-gate-policy.jqdeployment/aliyun/polymarket_market_tape_upload.pydeployment/aliyun/polymarket_reference_canonicalize.pydeployment/aliyun/polymarket_reference_collector.pydeployment/aliyun/test-polymarket-raw-ops-control-plane.shdeployment/aliyun/test_polymarket_market_tape_upload.pydeployment/aliyun/test_polymarket_reference_canonicalize.pydeployment/aliyun/test_polymarket_reference_collector.pyproducts/ploy/tasks/todo.mdproducts/ploy/tests/workspace_runtime_retirement.rsrust_hft/deployment/docker/Dockerfile.binance-lob-archiverrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/polymarket-raw-ops.rsrust_hft/tools/collector/src/lib.rsrust_hft/tools/collector/src/lob_archiver.rsrust_hft/tools/collector/src/polymarket_parity.rsrust_hft/tools/collector/src/polymarket_raw.rsrust_hft/tools/collector/src/polymarket_upload.rs
💤 Files with no reviewable changes (6)
- deployment/aliyun/test_polymarket_reference_canonicalize.py
- deployment/aliyun/polymarket_reference_canonicalize.py
- deployment/aliyun/test_polymarket_market_tape_upload.py
- deployment/aliyun/test_polymarket_reference_collector.py
- deployment/aliyun/polymarket_reference_collector.py
- deployment/aliyun/polymarket_market_tape_upload.py
| for path in /opt/monday /opt/monday/releases "$RELEASE_ROOT" \ | ||
| /data/monday /data/monday/spool "$SHADOW_ROOT" \ | ||
| /data/monday/evidence "$EVIDENCE_ROOT"; do | ||
| direct_directory_or_absent "$path" || die "fixed path is indirect or a symlink: $path" | ||
| done |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce ownership and permissions on control-plane parent directories.
These checks reject symlinks but accept attacker-writable directories. A local principal able to rename descendants of $RELEASE_ROOT or $EVIDENCE_ROOT can race the identity checks and replace the candidate or evidence. Require root ownership and no group/world write access for immutable release and evidence parents.
Also applies to: 313-315, 460-464
🤖 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 `@deployment/aliyun/polymarket-raw-ops-shadow-gate.sh` around lines 242 - 246,
Update the fixed-directory validation around direct_directory_or_absent and the
corresponding checks at the referenced release/evidence paths to require root
ownership and no group/world write permissions for control-plane parent
directories, including $RELEASE_ROOT and $EVIDENCE_ROOT. Preserve rejection of
indirect directories and absent-path handling while applying the same ownership
and permission enforcement to all listed immutable release and evidence parents.
| "$release_binary" verify-shadow-parity \ | ||
| --legacy-spool "$LEGACY_SPOOL" \ | ||
| --rust-spool "$shadow_spool" \ | ||
| --started-at-unix "$parity_window_started_at" \ | ||
| --ended-at-unix "$common_cutoff" \ | ||
| --output "$parity_json" || die 'byte/field/dedupe/settlement/rotation parity failed' |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not execute the candidate verifier as root.
Unlike collection and upload, verify-shadow-parity runs the candidate binary with full privileges. A compromised candidate or parser flaw can therefore compromise the host. Run it as hftcollector, then validate and copy its output into the root-owned evidence directory.
🤖 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 `@deployment/aliyun/polymarket-raw-ops-shadow-gate.sh` around lines 468 - 473,
Update the verify-shadow-parity invocation in the shadow parity flow to execute
the candidate binary as hftcollector rather than root. Ensure the generated
parity output is validated and then copied into the root-owned evidence
directory with appropriate ownership and permissions before downstream use,
while preserving the existing failure handling.
| EnvironmentFile=/etc/monday/polymarket-market-tape-upload.env | ||
| ExecStart=/usr/bin/python3 /opt/monday/bin/polymarket_market_tape_upload.py --spool-dir /data/monday/spool/polymarket-reference --dataset crypto_expiry_reference --quote-depth-levels 0 --quote-sample-ms 0 | ||
| ExecStart=/opt/monday/bin/polymarket-raw-ops upload --spool-dir /data/monday/spool/polymarket-reference --dataset crypto_expiry_reference --quote-depth-levels 0 --quote-sample-ms 0 | ||
| TimeoutStartSec=0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'polymarket-.*upload\.service' deployment/aliyun -0 |
xargs -0 rg -n '^(ExecStart|TimeoutStartSec)='Repository: proerror77/monday
Length of output: 702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in deployment/aliyun/polymarket-reference-upload.service \
deployment/aliyun/polymarket-market-tape-upload.service \
deployment/aliyun/*.timer
do
[ -f "$f" ] || continue
echo "===== $f ====="
cat -n "$f"
echo
doneRepository: proerror77/monday
Length of output: 3220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a . deployment/aliyunRepository: proerror77/monday
Length of output: 3416
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "TimeoutStartSec=" deployment/aliyun/*.service deployment/aliyun/*.timer || trueRepository: proerror77/monday
Length of output: 464
Add a finite start timeout to both Polymarket upload services. A hung oneshot can stay active indefinitely, so the 5-minute retry timers can’t launch a fresh run until it exits.
deployment/aliyun/polymarket-reference-upload.service#L15deployment/aliyun/polymarket-market-tape-upload.service#L15
📍 Affects 2 files
deployment/aliyun/polymarket-reference-upload.service#L15-L15(this comment)deployment/aliyun/polymarket-market-tape-upload.service#L15-L15
🤖 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 `@deployment/aliyun/polymarket-reference-upload.service` at line 15, Replace
the unlimited TimeoutStartSec setting with a finite five-minute start timeout in
deployment/aliyun/polymarket-reference-upload.service at lines 15-15 and
deployment/aliyun/polymarket-market-tape-upload.service at lines 15-15, so hung
oneshot runs terminate and retries can start.
| let zstd_timeout = env_u64(zstd_timeout, "ZSTD_TIMEOUT_SECONDS", 300)?; | ||
| let oss_timeout = env_u64(oss_timeout, "OSS_COPY_TIMEOUT_SECONDS", 300)?; | ||
| let config = UploadConfig { | ||
| spool_dir, | ||
| dataset, | ||
| quote_depth_levels, | ||
| quote_sample_ms, | ||
| bucket: env_or(bucket, "OSS_BUCKET", "monday-lob-apne1-1045353359"), | ||
| endpoint: env_or( | ||
| endpoint, | ||
| "OSS_ENDPOINT", | ||
| "oss-ap-northeast-1-internal.aliyuncs.com", | ||
| ), | ||
| region: env_or(region, "OSS_REGION", "ap-northeast-1"), | ||
| profile: env_or(profile, "ALIYUN_PROFILE", "ecs-role"), | ||
| zstd_timeout: Duration::from_secs(zstd_timeout), | ||
| oss_timeout: Duration::from_secs(oss_timeout), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
No floor on zstd_timeout/oss_timeout.
env_u64 accepts an explicit 0 for either timeout, producing Duration::from_secs(0) — an effectively-instant timeout on the zstd/OSS calls, unlike the positive_duration guard used for the collector's timing args in the same file.
🤖 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/bin/polymarket-raw-ops.rs` around lines 160 -
176, Validate zstd_timeout and oss_timeout through the same positive-duration
guard used for the collector timing arguments before constructing UploadConfig.
Reject explicit zero values while preserving the existing defaults and convert
only validated values to Duration::from_secs.
Summary
Verification
Deployment boundary
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests