fix: restart issues#1467
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved Serde field attributes from a proof fold state; refactored retry error handling and logging; added buffering and extra logs in event queries and net sync handling; disabled CI job gating conditions; enabled proof aggregation in an integration test script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
ctrlc03
force-pushed
the
fix/optional-aggregation-serde-serializing
branch
from
4678588 to
ae757d1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 122-123: The workflow jobs were incorrectly hard-disabled by
replacing their conditional expressions with "if: false", which breaks dependent
jobs (e.g., zk_prover_e2e ← build_circuits, crisp_e2e ← build_crisp_sdk,
template_integration/test_enclave_init ← build_e3_support_dev) and triggers
actionlint errors; revert each "if: false" back to the original path-based or
expression condition used before (restore the condition for every affected job
occurrence instead of using a constant false), or if a temporary kill switch is
required, change the condition to reference a workflow input or repository
variable (e.g., inputs or env var) so you can toggle runs without committing a
constant false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6427d4d-0e0d-4e40-abe5-29f70f656821
📒 Files selected for processing (4)
.github/workflows/ci.ymlcrates/events/src/eventstore.rscrates/net/src/net_sync_manager.rstests/integration/persist.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
122-123:⚠️ Potential issue | 🟠 MajorPlease restore the real job gates before merge.
These
if: falseguards still hard-disable a large slice of CI, and they also cascade into downstream skips for jobs that consume artifacts frombuild_circuits,build_crisp_sdk, andbuild_e3_support_dev. Revert them to theneeds.detect_changes.outputs.* == 'true'conditions, or put the temporary kill switch behind a workflow input/repo variable instead of a constant.Also applies to: 177-178, 221-222, 306-307, 350-351, 398-399, 589-590, 665-666, 781-782, 941-942, 1033-1034, 1084-1085, 1161-1162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 122 - 123, Restore the real CI job gate conditions in .github/workflows/ci.yml by replacing the temporary "if: false" guards with the original conditional expressions (e.g., needs.detect_changes.outputs.<job_name> == 'true') for each affected job (those currently using "if: false" around build_circuits, build_crisp_sdk, build_e3_support_dev and others listed), or alternatively switch the constant to a workflow input/repo variable and use that variable in the job-level if: expressions so you can toggle the gate without hard-disabling downstream jobs; update every occurrence of "if: false" in the workflow to reference the appropriate needs.detect_changes.outputs.* or the new input/secret variable so artifact-producing jobs and their consumers are correctly gated instead of permanently skipped.
🧹 Nitpick comments (1)
crates/events/src/eventstore_router.rs (1)
104-116: Consolidate store selection into a single lookup path.This does two map lookups for the same key. You can merge warning + fallback into one
matchto reduce branching and keep the control flow tighter.♻️ Proposed refactor
- let has_store = self.stores.contains_key(&aggregate_id); - if !has_store { - tracing::warn!( - "EventStoreRouter: no store for aggregate={}, falling back to 0 (source={:?})", - aggregate_id, - msg.event.source() - ); - } - let store_addr = self.stores.get(&aggregate_id).unwrap_or_else(|| { - self.stores - .get(&AggregateId::new(0)) - .expect("Default EventStore for AggregateId(0) not found") - }); + let store_addr = match self.stores.get(&aggregate_id) { + Some(addr) => addr, + None => { + tracing::warn!( + "EventStoreRouter: no store for aggregate={}, falling back to 0 (source={:?})", + aggregate_id, + msg.event.source() + ); + self.stores + .get(&AggregateId::new(0)) + .expect("Default EventStore for AggregateId(0) not found") + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/events/src/eventstore_router.rs` around lines 104 - 116, The current code performs two map lookups for the same key (aggregate_id) and then falls back to AggregateId(0); replace this with a single match on self.stores.get(&aggregate_id) to choose the store: if Some(store) return it, if None emit the tracing::warn! (using msg.event.source() and aggregate_id) and then look up the default store via self.stores.get(&AggregateId::new(0)).expect(...). Update the binding for store_addr to use that match result so you only call self.stores.get for aggregate_id once and keep the fallback lookup for the default store.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 546-548: The test_enclave_init job runs on ubuntu-latest while
build_enclave_cli produces the enclave-binary on the enclave-ci self-hosted
runner, causing potential binary incompatibility; update the test_enclave_init
job to use the same runner configuration (runs-on: group: enclave-ci with
labels: [enclave-ci-runner]) so it consumes the artifact on the matching
OS/arch, or alternatively modify build_enclave_cli to also produce an
ubuntu-latest build if test_enclave_init must remain on ubuntu-latest;
additionally, add an actionlint configuration declaring the custom
enclave-ci-runner label to avoid actionlint false positives.
In `@crates/events/src/eventstore.rs`:
- Around line 88-95: The code currently materializes all remaining events with
read_from(seq).collect(), which can OOM for large stores; instead pass the
streaming iterator directly into collect_events to preserve early stop/limit
behavior: replace the Vec materialization (read_from(seq).collect()) with a
boxed iterator created from self.log.read_from(seq) and call
collect_events(Box::new(self.log.read_from(seq)), filter, limit). Update the
tracing::debug log to stop using total (events.len()) since we no longer have a
Vec — log seq and result.len() (or compute a separate safe counter if needed)
and keep use of query_by_ts, read_from, and collect_events as the referenced
symbols.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 122-123: Restore the real CI job gate conditions in
.github/workflows/ci.yml by replacing the temporary "if: false" guards with the
original conditional expressions (e.g., needs.detect_changes.outputs.<job_name>
== 'true') for each affected job (those currently using "if: false" around
build_circuits, build_crisp_sdk, build_e3_support_dev and others listed), or
alternatively switch the constant to a workflow input/repo variable and use that
variable in the job-level if: expressions so you can toggle the gate without
hard-disabling downstream jobs; update every occurrence of "if: false" in the
workflow to reference the appropriate needs.detect_changes.outputs.* or the new
input/secret variable so artifact-producing jobs and their consumers are
correctly gated instead of permanently skipped.
---
Nitpick comments:
In `@crates/events/src/eventstore_router.rs`:
- Around line 104-116: The current code performs two map lookups for the same
key (aggregate_id) and then falls back to AggregateId(0); replace this with a
single match on self.stores.get(&aggregate_id) to choose the store: if
Some(store) return it, if None emit the tracing::warn! (using msg.event.source()
and aggregate_id) and then look up the default store via
self.stores.get(&AggregateId::new(0)).expect(...). Update the binding for
store_addr to use that match result so you only call self.stores.get for
aggregate_id once and keep the fallback lookup for the default store.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 922b13ea-f377-4ba6-bb65-cce2b2f5515d
📒 Files selected for processing (3)
.github/workflows/ci.ymlcrates/events/src/eventstore.rscrates/events/src/eventstore_router.rs
ctrlc03
force-pushed
the
fix/optional-aggregation-serde-serializing
branch
from
59580c1 to
eea0f2e
Compare
hmzakhalid
approved these changes
Mar 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix issues on restart for the aggregator:
722558768, expected variant index 0 <= i < 13 (deserialization bug)Summary by CodeRabbit
Bug Fixes
Chores
Tests