feat(collector): make Rust cutover fail closed - #29
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Warning Review limit reached
Next review available in: 19 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 ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughReplaces the Python Binance collector deployment with Rust services and a digest-pinned release workflow. Adds upload-only draining, spool-safe cleanup, shadow gating, Aliyun installation and invocation scripts, fail-closed cutover/rollback, updated health evidence, deployment configuration, documentation, and CI validation. ChangesRust uploader and health state
Candidate installation and services
Shadow gate and host cutover
Documentation and CI
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
![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
Put a timeout on full-catalog discovery
Setting production to SYMBOLS=ALL makes every startup call the Rust discover_symbols path, which uses a default reqwest::Client with no request timeout before the process watchdog or health writer is started. If Binance /exchangeInfo accepts the connection but stops responding, systemd still treats this Type=simple unit as active while it collects no data and does not restart until the six-hour runtime limit. Add an explicit timeout to discovery so production and the gate fail closed instead of wedging active-but-idle.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
deployment/aliyun/test-rust-lob-control-plane.sh (1)
126-150: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover malformed and cross-market runtime health.
Add rejection cases for string-valued catalog counts and mismatched market/dataset identity. The current tests cover only freshness and session changes.
🤖 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/test-rust-lob-control-plane.sh` around lines 126 - 150, Extend the runtime policy tests around RUNTIME_POLICY with rejection cases for runtime-health JSON whose catalog count fields are strings instead of numbers, and whose market/dataset identity does not match the expected identity. Assert jq rejects each malformed or cross-market payload while preserving the existing freshness and session checks.
🤖 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 117-122: Update the “Verify collector release control plane”
workflow step so each script matched by ../deployment/aliyun/*.sh is
individually passed to bash -n, ensuring syntax validation covers every file;
keep the existing shellcheck and test-rust-lob-control-plane.sh commands
unchanged.
In `@deployment/aliyun/binance-lob-archiver-upload`@.service:
- Around line 15-16: Remove the 600-second unit-level startup timeout from the
upload-only service by disabling TimeoutStartSec, allowing binance-lob-archiver
to drain all pending manifests while retaining the existing per-copy timeout
behavior.
In `@deployment/aliyun/deploy-rust-lob-release.sh`:
- Around line 185-186: Update the release-directory handling around the install
commands and the related lines 207-226 so a binary cannot overwrite historical
deployment evidence. Key the directory by the complete artifact, bundle, and
source-revision identity, or validate existing release.json metadata and reject
any mismatch before cp -a or mv; preserve reuse only when all identity fields
match exactly.
In `@deployment/aliyun/host-rust-lob-shadow-gate.sh`:
- Around line 189-208: Update deployment/aliyun/host-rust-lob-shadow-gate.sh
lines 189-208 to store each gate run under an immutable run/content-digested
evidence directory, removing the destructive deletion of prior gate.json and
marker files while preserving existing records. Update
deployment/aliyun/host-rust-lob-cutover.sh lines 300-329 to snapshot legacy
rollback assets into append-only cutover evidence, hash the snapshot, and never
populate an existing digest-addressed release from mutable /etc files.
- Around line 234-243: Update assert_spool_drained to detect artifact-shaped
symlinks as well as regular files, replacing the find -type f restriction with a
predicate that matches both file and symlink entries while preserving the
existing artifact-name filters and failure behavior.
In `@deployment/aliyun/invoke-rust-lob-operation.sh`:
- Around line 11-16: Extend the prerequisite-check loop in
invoke-rust-lob-operation.sh to also validate the runtime helpers seq, sleep,
and tr before execution proceeds. Reuse the existing command -v check and
missing-command error behavior so polling and decoding dependencies are verified
alongside aliyun, base64, and jq.
In `@deployment/aliyun/rust-lob-runtime-health-policy.jq`:
- Around line 1-4: The runtime health policy must reject quoted count fields
instead of accepting them in comparisons. Update the jq expression in
rust-lob-runtime-health-policy.jq to require symbol_count and
snapshot_ready_count to be numeric integers before applying the existing
thresholds and equality checks. Add a regression case in
deployment/aliyun/test-rust-lob-control-plane.sh covering quoted counts;
deployment/aliyun/host-rust-lob-cutover.sh requires no direct change.
In `@rust_hft/tools/collector/src/bin/binance-lob-archiver.rs`:
- Around line 1271-1285: Update cleanup_artifact_path and its recovery callers
to derive the expected manifest, data, and _SUCCESS filenames from the cleanup
marker filename, preferably using the marker’s segment digest. Reject metadata
names that do not exactly match those derived names before returning any
deletion path, preserving content-addressed or append-only dataset and
deployment evidence.
- Around line 1217-1229: Update the temporary cleanup-file creation in the
marker-writing flow to use exclusive, no-follow creation via
OpenOptions.create_new(true) instead of create(true).truncate(true), preventing
existing paths or symlinks from being opened. On an already-existing path, only
remove and retry when it is a stale regular file; do not delete or follow
symlinks or other file types.
---
Nitpick comments:
In `@deployment/aliyun/test-rust-lob-control-plane.sh`:
- Around line 126-150: Extend the runtime policy tests around RUNTIME_POLICY
with rejection cases for runtime-health JSON whose catalog count fields are
strings instead of numbers, and whose market/dataset identity does not match the
expected identity. Assert jq rejects each malformed or cross-market payload
while preserving the existing freshness and session checks.
🪄 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: ee07a6bd-ccfd-4814-b06d-0797dc6c36c0
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/aliyun/binance-lob-archiver-production-spot.envdeployment/aliyun/binance-lob-archiver-production-usdm.envdeployment/aliyun/binance-lob-archiver-production@.servicedeployment/aliyun/binance-lob-archiver-rust-spot.envdeployment/aliyun/binance-lob-archiver-rust-upload@.servicedeployment/aliyun/binance-lob-archiver-rust-usdm.envdeployment/aliyun/binance-lob-archiver-rust@.servicedeployment/aliyun/binance-lob-archiver-upload@.servicedeployment/aliyun/binance-lob-archiver@.servicedeployment/aliyun/binance_lob_archiver.pydeployment/aliyun/deploy-rust-lob-release.shdeployment/aliyun/host-rust-lob-cutover.shdeployment/aliyun/host-rust-lob-shadow-gate.shdeployment/aliyun/invoke-rust-lob-operation.shdeployment/aliyun/rust-lob-control-plane-lib.shdeployment/aliyun/rust-lob-runtime-health-policy.jqdeployment/aliyun/rust-lob-shadow-gate-policy.jqdeployment/aliyun/test-rust-lob-control-plane.shdeployment/aliyun/test_binance_lob_archiver.pyrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/binance-lob-archiver.rsrust_hft/tools/collector/src/lob_archiver.rs
💤 Files with no reviewable changes (3)
- deployment/aliyun/binance-lob-archiver@.service
- deployment/aliyun/test_binance_lob_archiver.py
- deployment/aliyun/binance_lob_archiver.py
087f134 to
88709ab
Compare
|
Review-fix summary for 88709ab: all actionable Codex/CodeRabbit findings are addressed; symbol discovery now has a 15-second total request timeout; release/gate/cutover evidence is immutable or append-only; forward and rollback health are bound to post-start timestamps; all production/upload/legacy units must be inactive and masked for containment. Local proof: all Aliyun Bash syntax, ShellCheck, control-plane behavior tests, rustfmt, 53 Rust tests (1 network smoke ignored), strict Clippy -D warnings, and diff-check pass. Remaining defense-in-depth only: same-UID pathname TOCTOU in a locked 0750 spool; a future dirfd/openat2 hardening can address that without blocking this rollout. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tools/collector/src/bin/binance-lob-archiver.rs`:
- Around line 82-101: The SpoolLock::acquire implementation must safely open
only the intended regular lock file without following symlinks. Replace the
current OpenOptions path with a no-follow open, validate the resulting inode is
a regular file before locking, and add a regression test covering a symlink at
SPOOL_LOCK_FILE; apply the same protection to the corresponding lock-opening
logic near the later call site.
- Around line 1298-1312: Preflight every cleanup target produced by
cleanup_artifact_paths, including marker, before entering the deletion flow.
Permit only missing paths or regular files; reject directories and symlinks with
the existing contextual error handling, then perform removals and directory
syncs only after all validation succeeds. Update the interruption test covering
this cleanup path to assert that validation failure leaves every artifact
untouched.
🪄 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: 24e968c8-0575-4a98-aa75-941f82e59596
⛔ Files ignored due to path filters (1)
rust_hft/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.github/workflows/ci.ymldeployment/aliyun/README.mddeployment/aliyun/binance-lob-archiver-production-spot.envdeployment/aliyun/binance-lob-archiver-production-usdm.envdeployment/aliyun/binance-lob-archiver-production@.servicedeployment/aliyun/binance-lob-archiver-rust-spot.envdeployment/aliyun/binance-lob-archiver-rust-upload@.servicedeployment/aliyun/binance-lob-archiver-rust-usdm.envdeployment/aliyun/binance-lob-archiver-rust@.servicedeployment/aliyun/binance-lob-archiver-upload@.servicedeployment/aliyun/binance-lob-archiver@.servicedeployment/aliyun/binance_lob_archiver.pydeployment/aliyun/deploy-rust-lob-release.shdeployment/aliyun/host-rust-lob-cutover.shdeployment/aliyun/host-rust-lob-shadow-gate.shdeployment/aliyun/invoke-rust-lob-operation.shdeployment/aliyun/rust-lob-control-plane-lib.shdeployment/aliyun/rust-lob-runtime-health-policy.jqdeployment/aliyun/rust-lob-shadow-gate-policy.jqdeployment/aliyun/test-rust-lob-control-plane.shdeployment/aliyun/test_binance_lob_archiver.pyrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/bin/binance-lob-archiver.rsrust_hft/tools/collector/src/lob_archiver.rs
💤 Files with no reviewable changes (3)
- deployment/aliyun/binance-lob-archiver@.service
- deployment/aliyun/test_binance_lob_archiver.py
- deployment/aliyun/binance_lob_archiver.py
🚧 Files skipped from review as they are similar to previous changes (17)
- deployment/aliyun/rust-lob-runtime-health-policy.jq
- .github/workflows/ci.yml
- deployment/aliyun/binance-lob-archiver-rust-upload@.service
- deployment/aliyun/rust-lob-shadow-gate-policy.jq
- deployment/aliyun/binance-lob-archiver-production-usdm.env
- deployment/aliyun/binance-lob-archiver-rust@.service
- deployment/aliyun/binance-lob-archiver-upload@.service
- deployment/aliyun/rust-lob-control-plane-lib.sh
- deployment/aliyun/binance-lob-archiver-rust-usdm.env
- rust_hft/tools/collector/Cargo.toml
- deployment/aliyun/binance-lob-archiver-production@.service
- deployment/aliyun/binance-lob-archiver-rust-spot.env
- deployment/aliyun/binance-lob-archiver-production-spot.env
- deployment/aliyun/invoke-rust-lob-operation.sh
- deployment/aliyun/test-rust-lob-control-plane.sh
- deployment/aliyun/deploy-rust-lob-release.sh
- deployment/aliyun/host-rust-lob-shadow-gate.sh
|
Follow-up 51457c8 closes the two late CodeRabbit Major findings. Local proof: 54 collector tests pass (25 archiver + 20 library + 9 materializer; 1 public-network smoke ignored), strict Clippy -D warnings, rustfmt, Cargo --locked, and diff-check pass. |
|
Additional hardening in a9fd835 preflights the cleanup marker path before opening it and adds O_NONBLOCK to no-follow opens, so FIFOs/devices cannot wedge preflight. Focused symlink-lock and interrupted-cleanup tests plus strict bin Clippy pass. |
2 participants
Summary
Safety
/proc/<pid>/exeverificationValidation
cargo test -p hft-collector --features collector-binance --locked: 49 passed, 1 ignored live-network smoke-D warningsbash -nand ShellCheck for all Aliyun control-plane scriptstest-rust-lob-control-plane.shgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests