fix(io): make event import iteration fallible - #537
Conversation
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
c64b261 to
0e56a9f
Compare
📝 WalkthroughWalkthroughImporter contracts now expose per-event errors. MessagePack, NDJSON, and Postcard readers propagate read and deserialization failures. Generated model imports and query-engine and simulator consumers handle result-wrapped iterator items. ChangesImporter error propagation
Estimated code review effort: 3 (Moderate) | ~30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@crates/io/msgpack/src/lib.rs`:
- Around line 160-164: Define a shared supported maximum frame size, then update
the payload-reading logic in crates/io/msgpack/src/lib.rs lines 160-164 and
crates/io/postcard/src/lib.rs lines 159-163 to reject lengths above it before
allocation and use fallible allocation for payload buffers; return the existing
importer failure for either oversized frames or allocation errors, while
preserving normal read_exact handling.
In `@crates/io/types/src/lib.rs`:
- Around line 85-92: Replace the public ImporterError type alias with a concrete
thiserror enum, including an explicit other variant for format-specific decode
errors. Update importer implementations and decoder error conversions to wrap
those errors through the enum, while preserving Send + Sync compatibility and
the ImporterResult contract.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 83d01151-d1cc-4332-af90-eead1bf8ab43
📒 Files selected for processing (9)
crates/io/msgpack/src/lib.rscrates/io/ndjson/src/lib.rscrates/io/postcard/src/lib.rscrates/io/src/lib.rscrates/io/types/src/lib.rscrates/model-macros/src/model_macro.rsdomains/query_engine/server/src/analyzer_cache.rsexamples/simulator/analyzer/src/lib.rsexamples/simulator/server/src/main.rs
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
crates/io/types/src/lib.rs-85-86 (1)
85-86: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the public
ImporterErrorcontract.
ImporterErroris a new public enum. Add a concise type-level doc comment that states when callers receive it. Public visibility is required becauseImporterResultexposes this type.Proposed fix
+/// Errors returned while importing events. #[derive(Debug, Error)] pub enum ImporterError {As per path instructions, new
pubitems need a doc comment and justified visibility.🤖 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 `@crates/io/types/src/lib.rs` around lines 85 - 86, Add a concise Rust doc comment directly above the public ImporterError enum describing when callers receive this error, preserving its existing variants and visibility because ImporterResult exposes it.Source: Path instructions
🤖 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 `@crates/io/msgpack/src/lib.rs`:
- Around line 164-181: Extract the duplicated frame-limit validation, buffer
allocation, and read_exact logic into a documented helper in quent-io-types that
accepts a Read implementation and length and returns ImporterResult<Vec<u8>>;
update crates/io/msgpack/src/lib.rs lines 164-181 and
crates/io/postcard/src/lib.rs lines 163-180 to call it after decoding the frame
length, while keeping each importer’s self.fail handling local.
---
Other comments:
In `@crates/io/types/src/lib.rs`:
- Around line 85-86: Add a concise Rust doc comment directly above the public
ImporterError enum describing when callers receive this error, preserving its
existing variants and visibility because ImporterResult exposes it.
🪄 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: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 01a3c5ee-04a0-420b-b767-a50d18562311
📒 Files selected for processing (4)
crates/io/msgpack/src/lib.rscrates/io/ndjson/src/lib.rscrates/io/postcard/src/lib.rscrates/io/types/src/lib.rs
|
/merge |
Description
Make importers yield
Resultitems so read, framing, and decoding failures are not mistaken for EOF. Recover when the next record boundary is known, otherwise terminate after returning the error.Generated model and query-engine consumers now propagate item errors. Simulator bridges temporarily collect streams until a follow-up introduces the fallible streaming interface in #520.
Related Issues
Split off as a small incremental change from #520
Written by Codex.