fix(runtime): separate liveness from readiness - #62
Conversation
|
Warning Review limit reached
Next review available in: 51 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 (11)
✨ 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 |
39a4f84 to
947c3d3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba04d138ff
ℹ️ 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 reconciliation_ready = match truth_age_secs { | ||
| None => false, | ||
| Some(age) => { | ||
| reconciliation_complete && reconciliation_healthy && age <= max_idle_secs as f64 |
There was a problem hiding this comment.
For non-live runtimes that still expose this shared metrics server, such as hft-paper with the metrics feature (apps/paper/src/main.rs:55-66) or quote/simulated configs, no authoritative venue reconciliation snapshot is ever published because the runtime only requires it for real live venues (system_builder.rs:765-774) and reconcile(false) skips publishing runtime truth (execution_control.rs:452-462). With this unconditional reconciliation_ready requirement, /readiness stays 503 forever even while those engines are otherwise healthy; the reconciliation gate needs to be conditional on runtimes that actually have an authoritative live account to reconcile.
Useful? React with 👍 / 👎.
| && util <= max_utilization | ||
| && reconciliation_ready | ||
| && !risk_halted | ||
| && data_integrity_gaps == 0; |
There was a problem hiding this comment.
This readiness check uses data_integrity_gaps as if it were a current health bit, but the engine only increments that field when a Disconnect event is seen and never clears it (market-core/engine/src/lib.rs:1057-1059). In any live session with a transient market-data disconnect, the cumulative counter remains >0, so /readiness returns 503 for the rest of the process even after streams recover; readiness should be based on an active/stale gap state or a recent window, not the all-time count.
Useful? React with 👍 / 👎.
| let reconciliation_ready = match truth_age_secs { | ||
| None => false, | ||
| Some(age) => { | ||
| reconciliation_complete && reconciliation_healthy && age <= max_idle_secs as f64 |
There was a problem hiding this comment.
In the checked live path, the metrics server uses readiness_max_idle_secs: 5 (apps/live/src/helpers/metrics.rs:29) while the production reconciliation interval is also 5000 ms (deployment/k8s/configmaps.yaml:29) and the task waits one full interval between reports (system_builder.rs:1276-1280). Because this line requires the reconciliation age to be <= 5s, normal scheduling jitter or reconciliation latency can make /readiness briefly return 503 every cycle even when reconciliation is healthy; use a separate reconciliation staleness threshold with headroom over the configured interval.
Useful? React with 👍 / 👎.
Change contract
Expose reconciliation/risk/data-integrity truth through readiness while keeping liveness probes independent; require authoritative venue reconciliation only for LiveSmall attribution, not simulated Paper/Shadow.
Out of scope
No account reconciliation implementation, cross-venue exposure policy, backtest changes, or alpha promotion-state transition changes.
Dependencies and merge order
Depends on #60 for the authoritative runtime-truth contract. #60 is now merged; this PR targets
main.Focused validation
cargo test -p hft-live runtime_attribution::tests::paper_and_shadow_snapshots_do_not_require_live_reconciliation -- --exactcargo test -p hft-live runtime_attribution::tests::live_small_snapshot_decays_without_authoritative_reconciliation -- --exactcargo test -p hft-infra-metricscargo test -p hft-live --test deployment_artifactscargo check -p hft-live --features clickhouse,redis,grpc --lockedRollout and rollback
Kubernetes/Docker liveness uses
/health;/readinessstays the fail-closed admission signal. Revert this PR to restore the old probe behavior.Scope exception
None.