fix(collector): enforce hard Polymarket cycle deadline - #44
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (8)
📝 WalkthroughWalkthroughThe PR adds digest-pinned deployment bundles, a Polymarket shadow gate and cutover/rollback control plane, a hard Rust cycle watchdog, updated memory limits, calibration documentation, and extensive integration and CI validation. ChangesPolymarket raw-ops promotion controls
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ShadowGate
participant Cutover
participant Systemd
participant RustCollector
Operator->>ShadowGate: validate candidate and run shadow gate
ShadowGate->>Systemd: start shadow collector
Systemd->>RustCollector: execute candidate
ShadowGate->>ShadowGate: write gate evidence
Operator->>Cutover: invoke cutover with gate evidence
Cutover->>Systemd: stop legacy units and install Rust units
Systemd->>RustCollector: start Rust collector
Cutover->>Cutover: write cutover evidence
Possibly related PRs
🚥 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| #[test] | ||
| fn hard_cycle_watchdog_terminates_non_yielding_work() { | ||
| let started = Instant::now(); | ||
| let output = std::process::Command::new(std::env::current_exe().unwrap()) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 @.github/workflows/acr-publish.yml:
- Around line 96-100: Bind the candidate SHA, source revision, control-manifest
SHA, and control-archive SHA in one immutable manifest generated by the workflow
around the existing checksum and source-revision steps in
.github/workflows/acr-publish.yml (lines 96-100). Update
deployment/aliyun/README.md (lines 148-159) to require manifest verification
before accepting any component. Update polymarket-raw-ops-shadow-gate.sh (lines
226-240) to load identities only from the verified pinned metadata and reject
mismatches against CLI values or the installed bundle.
In `@deployment/aliyun/polymarket-raw-ops-cutover.sh`:
- Around line 344-347: Update the rollback snapshot creation flow around the
sha256sum command to flush the rollback directory and its captured runtime state
to durable storage before publishing cutover success. Ensure the rollback
snapshot, installed units/binaries, configuration/control files, and
directory-entry updates are synchronized, not just cutover.json via sync
"$evidence_dir/cutover.json".
In `@deployment/aliyun/polymarket-raw-ops-shadow-gate.sh`:
- Around line 26-31: Harden every trusted directory used by
polymarket-raw-ops-shadow-gate.sh, including SCRIPT_DIR and the directories
checked around the referenced ranges, so each path component is root-owned and
not group- or world-writable before reading executables or policies and before
publishing evidence. Update the existing directory-validation logic (including
direct_directory* usage) rather than relying only on symlink rejection, and
apply the same checks to the completed evidence directory.
In `@rust_hft/tools/collector/src/polymarket_raw.rs`:
- Around line 72-77: Update the watchdog timeout branch around the
wait/completed check to replace eprintln! with a best-effort stderr write that
cannot prevent termination if writing fails. Preserve the timeout message and
ensure std::process::exit(HARD_CYCLE_WATCHDOG_EXIT_CODE) executes
unconditionally after the write attempt.
🪄 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: f5b15f64-9489-4287-9d53-7cd497377abc
📒 Files selected for processing (10)
.github/workflows/acr-publish.yml.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/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/test-polymarket-raw-ops-control-plane.shdocs/reports/polymarket-shadow-memory-calibration-2026-07-16.mdrust_hft/tools/collector/src/polymarket_raw.rs
| if wait.timed_out() && !*completed { | ||
| eprintln!( | ||
| "Polymarket reference cycle exceeded the hard {}ms wall-clock deadline", | ||
| timeout.as_millis() | ||
| ); | ||
| std::process::exit(HARD_CYCLE_WATCHDOG_EXIT_CODE); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '55,99p' rust_hft/tools/collector/src/polymarket_raw.rs
sed -n '/^\[profile\./,/^\[/p' rust_hft/Cargo.toml
fd -H -t f '^rust-toolchain(\.toml)?$' -x sed -n '1,120p' {}Repository: proerror77/monday
Length of output: 2050
🌐 Web query:
Rust eprintln macro panic if writing to stderr fails std::io::stderr write failure process::exit spawned thread panic behavior
💡 Result:
In Rust, the eprintln! and eprint! macros are explicitly designed to panic if writing to standard error (stderr) fails [1][2]. This behavior is documented in the Rust standard library, which states that these macros will panic if an I/O error occurs while writing to io::stderr [1][3]. A common cause for such a panic is when stderr is configured as non-blocking and the operation cannot complete immediately [1][4]. Because this panic occurs within the macro implementation, it follows standard Rust panic behavior: 1. If the panic occurs in a thread, the thread will terminate (unwind or abort, depending on configuration). 2. Because stderr is often used for logging or critical error reporting, this design forces the developer to handle or acknowledge potential failures in the output stream, rather than silently failing to report critical information. It is important to note that the Rust standard library provides specific internal functions, such as attempt_print_to_stderr, which are used in contexts where panicking is undesirable (e.g., when the runtime needs to print error messages during the termination of a main function or test). These internal functions intentionally ignore I/O errors to avoid secondary panics during the program's cleanup phase [5]. If your application requires handling stderr failures without crashing, you should avoid using the eprintln! or eprint! macros. Instead, obtain a handle to stderr using std::io::stderr and use the standard Write trait methods (such as write_all), which return a Result type that you can handle programmatically [6][7].
Citations:
- 1: https://doc.rust-lang.org/stable/std/macro.eprintln.html
- 2: https://github.com/rust-lang/rust/blob/master/library/std/src/macros.rs
- 3: https://github.com/rust-lang/rust/blob/2e2c86eba21a08cf505cd67073736d03ff3887ad/library/std/src/macros.rs
- 4: https://codebrowser.dev/rust/crates/std/src/macros.rs.html
- 5: https://github.com/rust-lang/rust/blob/main/library/std/src/io/stdio.rs
- 6: https://doc.rust-lang.org/stable/std/io/fn.stderr.html
- 7: https://doc.rust-lang.org/stable/std/io/trait.Write.html
Avoid eprintln! on the watchdog timeout path
eprintln! can panic if stderr writes fail, so this branch may never reach process::exit(HARD_CYCLE_WATCHDOG_EXIT_CODE). Use a best-effort stderr write, then exit unconditionally.
🤖 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_raw.rs` around lines 72 - 77, Update
the watchdog timeout branch around the wait/completed check to replace eprintln!
with a best-effort stderr write that cannot prevent termination if writing
fails. Preserve the timeout message and ensure
std::process::exit(HARD_CYCLE_WATCHDOG_EXIT_CODE) executes unconditionally after
the write attempt.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/reports/polymarket-shadow-memory-calibration-2026-07-16.md (2)
34-37: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not overstate the reclaim root cause as proven.
These observations support a cgroup-reclaim hypothesis, but the isolated probes used different diagnostic directories and execution conditions, so they do not prove that reclaim was the sole cause. Please soften “This proves” to “This supports” and preserve the controlled-comparison limitations in the evidence.
🤖 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 `@docs/reports/polymarket-shadow-memory-calibration-2026-07-16.md` around lines 34 - 37, In the report passage describing the first failed shadow and its persistence metrics, replace the definitive “This proves” claim with “This supports” and retain the limitation that the evidence supports, but does not establish, cgroup reclaim as the sole cause.
54-55: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCorrect the simultaneous memory-limit total.
512M + 768Mequals1,280 MiB(1.25 GiB), not1.5 GiB. This overstates the cgroup allocation by 256 MiB and makes the host-capacity calculation inconsistent with the stated limits.🤖 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 `@docs/reports/polymarket-shadow-memory-calibration-2026-07-16.md` around lines 54 - 55, Correct the simultaneous memory-limit total in the reviewed service envelope discussion: replace the stated 1.5 GiB with the accurate 1,280 MiB (1.25 GiB) value derived from MemoryHigh=512M and MemoryMax=768M, and keep the host-capacity calculation consistent.
🤖 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 @.github/workflows/ci.yml:
- Around line 124-129: Remove the three migration-control script exclusions from
the scanner pathspecs in the CI workflow. Keep these files included in Python,
pip, uv, and related checks, and add a narrowly scoped, explicit allowlist that
validates only the required legacy-runtime references rather than bypassing
scanning for entire files.
In `@deployment/aliyun/polymarket-raw-ops-cutover.sh`:
- Around line 148-149: Update the rollback invalidation sequence around the
marker rename and sync operations to durably persist the parent-directory rename
before rollback begins. After moving the success marker to the pending name,
synchronize the containing directory rather than only the renamed file, while
preserving failure propagation through the existing return paths.
In `@deployment/aliyun/test-polymarket-raw-ops-control-plane.sh`:
- Around line 219-225: Extend the evidence-marker tests to cover pre-existing
and concurrently created destinations, asserting the original marker bytes
remain unchanged for every listed publication path, including
finalize_rollback_evidence outputs. Replace forced-overwrite moves with atomic
no-replace publication so PASSED.invalid.sha256, PASSED.rolled-back.sha256, and
all other evidence markers cannot be clobbered.
---
Outside diff comments:
In `@docs/reports/polymarket-shadow-memory-calibration-2026-07-16.md`:
- Around line 34-37: In the report passage describing the first failed shadow
and its persistence metrics, replace the definitive “This proves” claim with
“This supports” and retain the limitation that the evidence supports, but does
not establish, cgroup reclaim as the sole cause.
- Around line 54-55: Correct the simultaneous memory-limit total in the reviewed
service envelope discussion: replace the stated 1.5 GiB with the accurate 1,280
MiB (1.25 GiB) value derived from MemoryHigh=512M and MemoryMax=768M, and keep
the host-capacity calculation consistent.
🪄 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: b9373ebc-d749-4189-a774-c182457468f8
📒 Files selected for processing (7)
.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/aliyun/polymarket-raw-ops-cutover.shdeployment/aliyun/polymarket-raw-ops-shadow-gate.shdeployment/aliyun/polymarket-shadow-gate-policy.jqdeployment/aliyun/test-polymarket-raw-ops-control-plane.shdocs/reports/polymarket-shadow-memory-calibration-2026-07-16.md
🚧 Files skipped from review as they are similar to previous changes (1)
- deployment/aliyun/polymarket-raw-ops-shadow-gate.sh
| #[test] | ||
| fn hard_cycle_watchdog_exits_124_while_stderr_is_locked() { | ||
| let started = Instant::now(); | ||
| let status = std::process::Command::new(std::env::current_exe().unwrap()) |
2 participants
Summary
Validation
cargo test -p hft-collector --features collector-binance --lockedcargo clippy -p hft-collector --features collector-binance --all-targets --locked -- -D warningsdeployment/aliyun/test-polymarket-raw-ops-control-plane.shshellcheckfor gate/cutover/control-plane scriptscargo fmt --checkandgit diff --checkSafety
No trading path is enabled. Production remains on the existing Python reference collector until a new immutable artifact completes the one-hour shadow plus parity tail and the same reviewed bundle performs cutover.
Summary by CodeRabbit