fix(collector): bound strict gate verifier memory - #359
Conversation
|
Warning Review limit reached
Next review available in: 14 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 (4)
📝 WalkthroughWalkthroughThe PR adds a surface-free strict tape verifier, routes strict LOB continuity checks through it, and changes the gate to verify adjacent segment pairs in bounded ChangesStrict gate verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShadowGate
participant SystemdRun
participant Archiver
participant StrictVerifier
ShadowGate->>SystemdRun: Start bounded pair unit
SystemdRun->>Archiver: Run --verify-segment with continuity
Archiver->>StrictVerifier: Verify strict market tape
StrictVerifier-->>Archiver: Return verification result
Archiver-->>SystemdRun: Return pair status
SystemdRun-->>ShadowGate: Return unit status
ShadowGate->>SystemdRun: Stop failed unit when needed
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 64e07661c0
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs (1)
204-268: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrict-gate surface-skipping design looks correct.
Correctness-critical checks (replay sequencing, aggregate-trade sequencing/summaries, LOB continuity builder, non-empty/positive book levels) all still run unconditionally; only surface materialization is skipped when
collect_surfacesis false. This matches the PR's stated goal of bounding memory without weakening validation for the strict gate.One latent footgun worth guarding: the unchanged block further down (around the
!require_lob_continuitybranch that inspectsaggregate_trades) assumesaggregate_tradesreflects the tape's real trades. Sinceaggregate_trades.pushis now gated behindcollect_surfaces(see the"agg_trade"handling below), a future caller of this private helper with(require_lob_continuity=false, collect_surfaces=false)would always see an emptyaggregate_trades, causing that branch to reject every tape as "missing aggregate trades for a declared symbol" regardless of actual content. Today this is unreachable (the onlycollect_surfaces=falsecaller always setsrequire_lob_continuity=true), but nothing enforces that pairing.🛡️ Suggested guard
fn verify_binance_market_tape_with_requirements_and_surfaces( mut sealed: Vec<SealedBinanceMarketTapeTriplet>, require_trade_summaries: bool, require_lob_continuity: bool, collect_surfaces: bool, ) -> Result<VerifiedBinanceMarketTape> { + debug_assert!( + collect_surfaces || require_lob_continuity, + "skipping surface collection is only safe when LOB continuity is required, \ + since the non-continuity path relies on collected aggregate trades" + ); if sealed.is_empty() {🤖 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/data-pipelines/core/src/binance_market_tape_artifact.rs` around lines 204 - 268, Guard the parameter combination in verify_binance_market_tape_with_requirements_and_surfaces so collect_surfaces=false cannot be used with require_lob_continuity=false, or otherwise preserve aggregate-trade availability for that validation branch. Ensure the existing strict-gate call remains valid and prevent future callers from reaching the !require_lob_continuity aggregate_trades check with an intentionally unmaterialized surface.
🤖 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/host-rust-lob-shadow-gate.sh`:
- Around line 269-287: Update the verification flow around
verify_oss_round_trips so it does not run via command substitution or another
subshell when assigning round_trips. Use an output-variable, file, or pipe-based
pattern that preserves the function’s result while keeping mutations to
strict_verifier_unit in the parent shell, ensuring cleanup can stop any unit
created by run_strict_verifier_pair.
---
Nitpick comments:
In `@rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rs`:
- Around line 204-268: Guard the parameter combination in
verify_binance_market_tape_with_requirements_and_surfaces so
collect_surfaces=false cannot be used with require_lob_continuity=false, or
otherwise preserve aggregate-trade availability for that validation branch.
Ensure the existing strict-gate call remains valid and prevent future callers
from reaching the !require_lob_continuity aggregate_trades check with an
intentionally unmaterialized surface.
🪄 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: 726bae98-864b-4cbf-96c2-32679f02b6be
📒 Files selected for processing (4)
deployment/aliyun/host-rust-lob-shadow-gate.shdeployment/aliyun/test-rust-lob-control-plane.shrust_hft/data-pipelines/core/src/binance_market_tape_artifact.rsrust_hft/tools/collector/src/bin/binance-lob-archiver.rs
|
Addressed the review findings in commit |
|
Addressed the remaining review findings in d327202:
Validation: shell syntax + shellcheck + control-plane contract test; hft-data 54/54; hft-collector binance-lob-archiver 63/63; rustfmt check; git diff --check. |
|
Root cause fixed in 4f9c6d1: the full-set aggregate continuity gate passed bare path/SHA/manifest triples to the collector CLI instead of reconstructing the required --verify-segment / --segment-content-sha256 / --segment-manifest-sha256 flags. macOS Bash 3.2 masked the failing fixture via set -e behavior; GitHub Bash 5 failed silently. The fixture now fails explicitly and verifies all three isolated verifier invocations. Validation: Bash 5 and Bash 3.2 control-plane tests pass; exact CI release-control command passes; shellcheck and git diff --check pass; two-axis incremental review is clean. |
Change contract
Make the #224 strict shadow gate bounded and fail-closed: verify adjacent segment pairs with transient systemd cgroups, clean those units on failure/termination, and use a Rust gate-only verifier that keeps all SHA, _SUCCESS, agg_trade, manifest, and LOB-continuity validation while not retaining research surfaces.
PRD
Not required; this is a narrow regression fix for the existing #224 collector contract.
Out of scope
Dependency / merge order
None. Based on main at 1f40d29. Issue #224.
Focused validation
bash deployment/aliyun/test-rust-lob-control-plane.shbash -n deployment/aliyun/host-rust-lob-shadow-gate.shshellcheck deployment/aliyun/host-rust-lob-shadow-gate.sh deployment/aliyun/test-rust-lob-control-plane.shgit diff --checkrustfmt --check rust_hft/data-pipelines/core/src/binance_market_tape_artifact.rscargo test -p hft-data --lib(52 passed)cargo test -p hft-collector --bin binance-lob-archiver(63 passed)Rollout / rollback impact
Release candidate only: merge/CI builds a new artifact, then install it as an inactive ECS shadow candidate and rerun the bounded gate. No production service was changed by this PR. Roll back by retaining the previous candidate/artifact; cutover remains a separate, explicitly controlled operation after gate evidence and canonical readback.
Summary by CodeRabbit
New Features
Bug Fixes
Tests