fix(collector): reuse sealed market tape validation - #772
Conversation
|
Warning Review limit reached
Next review available in: 38 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (2)
📝 WalkthroughWalkthroughThe PR adds a shared Polymarket tape-validation contract, writer-side ChangesPolymarket tape sealing
Repository metadata updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RecordingFeed
participant TapeManifest
participant TapeUploader
participant OSS
RecordingFeed->>TapeManifest: Validate records incrementally
RecordingFeed->>TapeUploader: Rotate tape with seal sidecar
TapeUploader->>TapeManifest: Validate seal and source identity
TapeUploader->>OSS: Upload data and manifest
OSS-->>TapeUploader: Return artifact readbacks
TapeUploader->>OSS: Publish and verify _SUCCESS
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
defe427 to
b9bd56f
Compare
b9bd56f to
48ede44
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
rust_hft/tools/collector/src/polymarket_upload.rs (2)
251-262: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmit phase metrics through
tracinginstead ofeprintln!.
Dropwrites an unstructured line to stderr. The rest of this system usestracing, and the recording writer already logs seal outcomes withinfo!andwarn!. Raweprintln!cannot be filtered by level, carries no span context, and cannot be sampled or shipped as structured fields.The output is also unconditional. Every phase in every segment writes a line on each upload run.
♻️ Proposed refactor
impl Drop for PhaseAttribution { fn drop(&mut self) { let cpu_ended = cpu_usage(); - eprintln!( - "UPLOAD_PHASE phase={} wall_ms={} self_cpu_ms={} child_cpu_ms={}", - self.phase, - self.started.elapsed().as_millis(), - (cpu_ended.self_micros - self.cpu_started.self_micros).max(0) / 1_000, - (cpu_ended.child_micros - self.cpu_started.child_micros).max(0) / 1_000, - ); + tracing::info!( + phase = self.phase, + wall_ms = self.started.elapsed().as_millis(), + self_cpu_ms = (cpu_ended.self_micros - self.cpu_started.self_micros).max(0) / 1_000, + child_cpu_ms = (cpu_ended.child_micros - self.cpu_started.child_micros).max(0) / 1_000, + "upload phase attribution", + ); } }🤖 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 251 - 262, Update PhaseAttribution::drop to emit the phase metrics through tracing with structured fields instead of eprintln!, using the appropriate log level and preserving the existing phase, wall-time, self-CPU, and child-CPU values. Ensure the emission follows the surrounding recording-writer tracing conventions and is filterable rather than unconditional stderr output.
390-393: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe two
Errarms are identical.
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None)andErr(_) => return Ok(None)produce the same result. The guarded arm adds no behavior.Keep the guarded arm only if you intend to log the two cases differently. A missing seal is the normal path for a legacy tape. An unreadable seal indicates a spool problem that an operator should see.
♻️ Proposed simplification
Ok(_) => return Ok(None), - Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None), Err(_) => return Ok(None),🤖 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 390 - 393, Remove the redundant guarded NotFound arm in the match surrounding the seal lookup, keeping a single error path that returns Ok(None). If unreadable seals should be distinguished operationally, instead retain the guard and add distinct logging for non-NotFound errors; otherwise simplify the match without changing the existing result.rust_hft/prediction-markets/crates/ploy-market-contracts/src/polymarket_tape.rs (1)
161-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe reported
linenumber is derived from the sequence, not the file position.
observecomputeslineaslast_sequence + 2. This equals the NDJSON line number only when the tape starts at sequence 0. The builder itself accepts any starting sequence, becauseexpected_sequence.get_or_insert(sequence)seeds from the first record. A tape that starts at a non-zero sequence produces misleadingline N:prefixes in every validation error.The uploader rejects
start_sequence != 0later, so this affects diagnostics only. Consider tracking an explicit observation counter instead.♻️ Proposed change to track an explicit record counter
- let line = self - .last_sequence - .and_then(|value| usize::try_from(value).ok()) - .and_then(|value| value.checked_add(2)) - .unwrap_or(1); + let line = self.observed_records.saturating_add(1);Add
observed_records: usizetoMarketTapeManifestBuilderand increment it at the end ofobserve.🤖 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-market-contracts/src/polymarket_tape.rs` around lines 161 - 166, Update MarketTapeManifestBuilder::observe to derive validation error line numbers from an explicit observed_records counter rather than last_sequence, so tapes with non-zero starting sequences report their actual NDJSON positions. Add the counter to MarketTapeManifestBuilder, use it when calculating line, and increment it at the end of observe while preserving the existing sequence validation behavior.rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs (1)
179-187: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe seal sidecar convention is reimplemented in both the writer and the uploader.
The seal file name and the file identity are a producer-consumer contract, but neither is defined in
ploy-market-contracts. The writer and the uploader each carry their own copy. A change to either rule in one crate makes every seal fail to match in the other, and the failure is silent becausematching_tape_sealreturnsOk(None)and falls back to the full scan. That is the exact 3.7-4.3 GiB rescan this PR removes.
rust_hft/tools/collector/Cargo.tomlnow depends onploy-market-contracts, so both crates can share one definition.
rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs#L179-L187: deletetape_file_identityand call a newTapeFileIdentity::from_metadatafrom the contracts crate.rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs#L171-L177: delete thistape_seal_pathand call a sharedpolymarket_tape::tape_seal_path. This also removes the unreachable"market-updates.ndjson"fallback, which would place the seal beside the wrong tape if it were ever reached.rust_hft/tools/collector/src/polymarket_upload.rs#L371-L377: delete this secondtape_seal_pathandFileIdentity::tape_seal_identity, and call the shared functions.Keep the uploader's
Resultreturn for a non-UTF-8 name as the shared signature. The writer already rejects that case before it builds the seal.🤖 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-bundles/src/feed/recorded.rs` around lines 179 - 187, Centralize the tape seal contract: in rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs lines 179-187, remove tape_file_identity and use TapeFileIdentity::from_metadata from ploy-market-contracts; at lines 171-177, remove tape_seal_path and use polymarket_tape::tape_seal_path, eliminating the unreachable fallback. In rust_hft/tools/collector/src/polymarket_upload.rs lines 371-377, remove the local tape_seal_path and FileIdentity::tape_seal_identity implementations and call the shared functions, preserving the Result return for non-UTF-8 names.
🤖 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/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs`:
- Around line 202-210: Update write_tape_seal and TapeSealPolicy to use the
dataset configured by RecordingPolicy instead of hardcoding "crypto_expiry", and
pass that value to builder.finish so matching_tape_seal validates the actual
dataset. If the writer is intentionally restricted to one dataset, document that
restriction explicitly at the relevant policy or writer symbol.
- Around line 563-577: Update the seal_policy construction for
market-updates.ndjson to use the same record_market_updates_ quote sample and
depth configuration as the deployed runtime. Ensure polymarket-raw-ops defaults
its --quote-sample-ms and --quote-depth-levels to those values, or reject
mismatches before validating seals, so MarketUpdateLogWriter does not fall back
to a full scan.
In `@rust_hft/tools/collector/src/polymarket_upload.rs`:
- Around line 216-233: Update the CPU attribution implementation around
rusage_micros and cpu_usage so concurrent phases do not report process-wide CPU
deltas as per-phase values. Prefer tracking each zstd child’s CPU via wait4 for
child_cpu_ms and using RUSAGE_THREAD for self_micros, preserving the existing
PhaseAttribution reporting while ensuring values reflect the specific phase or
thread rather than all concurrent work.
- Around line 403-418: Update matching_tape_seal to require manifest["date"] and
manifest["hour"] to be present string values alongside the existing validation
checks. Ensure seals missing either field fail validation and return Ok(None),
allowing the caller to perform the full scan instead of reaching the expects in
prepare_artifacts_from_scan.
- Around line 3256-3272: Align the test scan policy with UploadConfig in
missing_corrupt_or_mismatched_seal_falls_back_to_full_scan by using
quote_depth_levels 1 and quote_sample_ms 1_000 when creating the seal manifest,
so the test reaches tape-corruption fallback rather than policy-mismatch
handling. Apply the same policy values in
matching_seal_reuses_manifest_without_changing_source_identity if it currently
scans with 0, 0, preserving its reuse assertion.
---
Nitpick comments:
In
`@rust_hft/prediction-markets/crates/ploy-market-contracts/src/polymarket_tape.rs`:
- Around line 161-166: Update MarketTapeManifestBuilder::observe to derive
validation error line numbers from an explicit observed_records counter rather
than last_sequence, so tapes with non-zero starting sequences report their
actual NDJSON positions. Add the counter to MarketTapeManifestBuilder, use it
when calculating line, and increment it at the end of observe while preserving
the existing sequence validation behavior.
In
`@rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs`:
- Around line 179-187: Centralize the tape seal contract: in
rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs
lines 179-187, remove tape_file_identity and use TapeFileIdentity::from_metadata
from ploy-market-contracts; at lines 171-177, remove tape_seal_path and use
polymarket_tape::tape_seal_path, eliminating the unreachable fallback. In
rust_hft/tools/collector/src/polymarket_upload.rs lines 371-377, remove the
local tape_seal_path and FileIdentity::tape_seal_identity implementations and
call the shared functions, preserving the Result return for non-UTF-8 names.
In `@rust_hft/tools/collector/src/polymarket_upload.rs`:
- Around line 251-262: Update PhaseAttribution::drop to emit the phase metrics
through tracing with structured fields instead of eprintln!, using the
appropriate log level and preserving the existing phase, wall-time, self-CPU,
and child-CPU values. Ensure the emission follows the surrounding
recording-writer tracing conventions and is filterable rather than unconditional
stderr output.
- Around line 390-393: Remove the redundant guarded NotFound arm in the match
surrounding the seal lookup, keeping a single error path that returns Ok(None).
If unreadable seals should be distinguished operationally, instead retain the
guard and add distinct logging for non-NotFound errors; otherwise simplify the
match without changing the existing result.
🪄 Autofix
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: b990a2b1-a1ed-4b3e-825f-817e9ef43308
⛔ Files ignored due to path filters (2)
rust_hft/Cargo.lockis excluded by!**/*.lockrust_hft/prediction-markets/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
rust_hft/prediction-markets/crates/ploy-market-contracts/Cargo.tomlrust_hft/prediction-markets/crates/ploy-market-contracts/src/lib.rsrust_hft/prediction-markets/crates/ploy-market-contracts/src/polymarket_tape.rsrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rsrust_hft/prediction-markets/tasks/todo.mdrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/polymarket_upload.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: defe427e2a
ℹ️ 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".
b10efb4 to
61794cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rust_hft/shared/polymarket-tape/Cargo.toml (1)
8-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInherit shared dependency versions from the workspace.
rust_hft/Cargo.tomldeclareschrono,rust_decimal,serde,serde_json, andthiserrorunder[workspace.dependencies], butrust_hft/shared/polymarket-tape/Cargo.tomluses inline versions. Use.workspace = truefor these dependencies to keep this crate on the approved workspace versions;thiserror = "1"also adds a separate 1.x build because the workspace pinnedthiserror = "2.0".🤖 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/shared/polymarket-tape/Cargo.toml` around lines 8 - 13, Update the [dependencies] entries in the polymarket-tape crate for chrono, rust_decimal, serde, serde_json, and thiserror to inherit their versions from the workspace using the workspace dependency form, preserving the existing feature selections for chrono and serde and eliminating the standalone thiserror version.
🤖 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/shared/polymarket-tape/src/lib.rs`:
- Line 825: Update the incremental manifest generation flow around the manifest
sealing step so source_field_presence and source_field_non_null are computed
from the tape rows before the manifest is sealed. Reuse the full-scan per-record
metadata logic, then seal the manifest only after these maps are populated,
ensuring scan_tape_with_identity receives accurate missing and null source-field
information.
- Around line 235-246: Update decimal() to support non-string JSON numeric
values by attempting Decimal::from_str first and falling back to
Decimal::from_scientific when parsing the Value::to_string() representation
fails. Preserve the existing validation error for values rejected by both
parsers.
---
Nitpick comments:
In `@rust_hft/shared/polymarket-tape/Cargo.toml`:
- Around line 8-13: Update the [dependencies] entries in the polymarket-tape
crate for chrono, rust_decimal, serde, serde_json, and thiserror to inherit
their versions from the workspace using the workspace dependency form,
preserving the existing feature selections for chrono and serde and eliminating
the standalone thiserror version.
🪄 Autofix
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: 5e7fb4c5-2026-430d-868f-c54ced9ba187
⛔ Files ignored due to path filters (2)
rust_hft/Cargo.lockis excluded by!**/*.lockrust_hft/prediction-markets/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
rust_hft/Cargo.tomlrust_hft/prediction-markets/config/research_missions/polymarket-btc-5m.example.jsonrust_hft/prediction-markets/config/research_missions/polymarket-sol-5m.example.jsonrust_hft/prediction-markets/crates/ploy-research/prediction-policy-dependencies.linux.sha256rust_hft/prediction-markets/crates/ploy-research/prediction-policy-dependencies.linux.txtrust_hft/prediction-markets/crates/ploy-strategy-bundles/Cargo.tomlrust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rsrust_hft/shared/polymarket-tape/Cargo.tomlrust_hft/shared/polymarket-tape/src/lib.rsrust_hft/tools/collector/Cargo.tomlrust_hft/tools/collector/src/polymarket_upload.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rust_hft/prediction-markets/crates/ploy-strategy-bundles/src/feed/recorded.rs
- rust_hft/tools/collector/src/polymarket_upload.rs
cba434c to
360ffb8
Compare
360ffb8 to
3b9109f
Compare
Change contract
Eliminate the rotation-time 3.7–4.3 GiB NDJSON reparse by handing the uploader a versioned manifest that the market recorder maintains incrementally from the exact serialized records it writes. The uploader accepts the seal only when its raw file device, inode, size, mtime, dataset, recording policy, and source identity all match; legacy, missing, malformed, oversized, or mismatched seals retain the existing fail-closed full scan.
The uploader also emits per-phase wall, thread-local self-CPU, and reaped-child CPU attribution for seal lookup/full scan, multi-hour split/chunk scans, zstd, local SHA, PUT, and readback. Each sample includes the active archive count so child CPU is interpreted as exact only for the production single-archive case and as overlapping process-child usage otherwise.
Issue relationship
Refs #716
Out of scope
Production deployment, canary, Gate, credentials, cloud resources, PM Reference, Binance/ACK behavior, timer, upload concurrency, ZSTD settings, CPUQuota, tape cap, data-quality thresholds, and artifact readback ordering.
Dependencies and merge order
None.
Focused validation
Rollout and rollback
No production action in this PR phase. The runtime fallback is the existing full scanner whenever the seal cannot be trusted. Any later canary requires a fresh immutable producer/uploader release identity, explicit authorization, and the existing automatic rollback/data-readback contract.
Scope exception
This is one inseparable safety handoff: the shared incremental validator, producer seal publication, uploader identity verification/fallback, and equivalence tests cannot be merged independently without either producing an unused contract or allowing an unverified consumer path. Named atomic-scope reviewer: @proerror77, whose coordinator authorization explicitly approved exactly one corrective PR under existing runtime issue #716 on 2026-08-09. Review and required exact-head CI remain mandatory before merge.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration