chore: introduce QuadError and narrow audited error convergence - #961
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds typed error carriers, validates transaction configuration during bootstrap, preserves fatal errors across admission and completion paths, and migrates catalog, transaction, and table mutation internals to typed result domains. ChangesTyped error migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant EngineInner
participant MandatoryRuntime
participant TransactionSystem
Session->>EngineInner: request admission
EngineInner->>MandatoryRuntime: check health and completion state
MandatoryRuntime-->>EngineInner: return Lifecycle or Fatal result
EngineInner-->>Session: preserve the original error domain
Session->>TransactionSystem: submit transaction or DDL operation
TransactionSystem-->>Session: return typed runtime, operation, or fatal result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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 |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 2 medium |
🟢 Metrics 50 complexity · 26 duplication
Metric Results Complexity 50 Duplication 26
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Greptile SummaryThe PR introduces closed typed error carriers and moves error convergence toward public ownership boundaries while preserving fatal, lifecycle, runtime, and operation classifications.
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/error.rs | Adds frame-less four-domain and lifecycle/fatal carriers plus typed completion replay that preserves native roots and physical source chains. |
| doradb-storage/src/engine.rs | Moves transaction configuration validation and normalized redo-prefix ownership to the public bootstrap boundary. |
| doradb-storage/src/runtime/mandatory.rs | Keeps mandatory completion observation typed and preserves fatal errors during poison-aware admission. |
| doradb-storage/src/session.rs | Replays mandatory completion failures into constrained carriers and discloses them at public session boundaries. |
| doradb-storage/src/trx/mod.rs | Narrows transaction commit and rollback error propagation while retaining public disclosure at transaction APIs. |
| doradb-storage/src/trx/sys.rs | Consumes validated transaction configuration and propagates commit completion failures through typed carriers. |
| doradb-storage/src/table/access.rs | Replaces broad internal public-error convergence with narrower typed mutation results while retaining callback error transport. |
| doradb-storage/src/conf/trx.rs | Introduces validated transaction configuration containing normalized settings and a resolved redo-file prefix. |
Reviews (2): Last reviewed commit: "resolve task" | Re-trigger Greptile
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #961 +/- ##
==========================================
+ Coverage 93.47% 93.49% +0.02%
==========================================
Files 154 154
Lines 131555 132064 +509
==========================================
+ Hits 122967 123474 +507
- Misses 8588 8590 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
doradb-storage/src/trx/stmt.rs (1)
652-658: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winWrite-path disclosures lack the
operation=/table_id=context that sibling read paths add.
table_insert_mvcc,table_upsert_unique_mvcc,table_update_unique_mvcc, andtable_delete_unique_mvcceach call.disclose()directly on the result of the underlying mvcc method.table_scan_mvcc,table_lookup_unique_mvcc,table_index_lookup_mvcc, andtable_index_scan_mvccin the same file all add.attach_with(|| format!("operation={OPERATION}, table_id={table_id}..."))before.disclose().Add the same attachment to the four write paths. Without it, a failure in the most frequent DML paths does not carry the operation name or
table_idin its report, unlike every read path in this file.🩹 Proposed fix for `table_insert_mvcc` (apply the equivalent change to the other three methods)
table .accessor_with_layout(&layout) .insert_mvcc(rt, effects, cols) .await + .attach_with(|| format!("operation={OPERATION}, table_id={table_id}")) .disclose() }Also applies to: 698-704, 743-749, 781-787
🤖 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 `@doradb-storage/src/trx/stmt.rs` around lines 652 - 658, Update table_insert_mvcc, table_upsert_unique_mvcc, table_update_unique_mvcc, and table_delete_unique_mvcc to attach the same operation and table_id context used by the sibling read paths before calling disclose(). Preserve each underlying MVCC call and add the attachment consistently to all four write-path error reports.
🧹 Nitpick comments (1)
doradb-storage/src/session.rs (1)
1001-1007: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated mandatory-completion disclosure pattern is a good extraction candidate.
Each of these ten sites repeats the same chain:
observer.wait().await.map_err(|error| error.into_quad(RuntimeError::X)).attach_with(|| ...).disclose(). Only theRuntimeErrorvariant and the attachment message differ.Extract a small private helper that takes the
CompletionResult, theRuntimeErrorcontext, and a closure producing the attachment message, then callsdisclose()internally. This reduces the duplication without changing behavior.♻️ Proposed helper to remove duplication
#[inline] fn disclose_mandatory_completion<T>( result: CompletionResult<T>, context: RuntimeError, attachment: impl FnOnce() -> String, ) -> Result<T> { result .map_err(|error| error.into_quad(context)) .attach_with(attachment) .disclose() }Then, for example,
create_tablebecomes:- observer - .wait() - .await - .map_err(|error| error.into_quad(RuntimeError::CatalogAccess)) - .attach("operation=create_table, phase=wait_mandatory_completion") - .disclose() + disclose_mandatory_completion( + observer.wait().await, + RuntimeError::CatalogAccess, + || "operation=create_table, phase=wait_mandatory_completion".to_owned(), + )Also applies to: 1044-1060, 1090-1106, 1129-1138, 1166-1173, 1201-1208, 1236-1241, 1379-1388, 1419-1428, 1571-1580
🤖 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 `@doradb-storage/src/session.rs` around lines 1001 - 1007, Extract a private helper such as disclose_mandatory_completion that accepts CompletionResult<T>, a RuntimeError context, and an attachment-message closure, then performs the existing map_err, attach_with, and disclose chain. Replace the repeated mandatory-completion chains in the listed call sites, including create_table, passing each site’s existing error variant and attachment message without changing behavior.
🤖 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 `@doradb-storage/src/session.rs`:
- Around line 3287-3301: Update both poisoner.ensure_healthy() checks in
wait_for_maintenance_boundary, including the pre-listener check and
post-listener recheck, to attach the same boundary.name() and target_ts context
used by the adjacent shutdown error before propagating the
LifecycleOrFatalError.
---
Outside diff comments:
In `@doradb-storage/src/trx/stmt.rs`:
- Around line 652-658: Update table_insert_mvcc, table_upsert_unique_mvcc,
table_update_unique_mvcc, and table_delete_unique_mvcc to attach the same
operation and table_id context used by the sibling read paths before calling
disclose(). Preserve each underlying MVCC call and add the attachment
consistently to all four write-path error reports.
---
Nitpick comments:
In `@doradb-storage/src/session.rs`:
- Around line 1001-1007: Extract a private helper such as
disclose_mandatory_completion that accepts CompletionResult<T>, a RuntimeError
context, and an attachment-message closure, then performs the existing map_err,
attach_with, and disclose chain. Replace the repeated mandatory-completion
chains in the listed call sites, including create_table, passing each site’s
existing error variant and attachment message without changing behavior.
🪄 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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 02fcec8b-f32f-451c-b363-4313ef03eb97
⛔ Files ignored due to path filters (5)
docs/error-spec.mdis excluded by none and included by nonedocs/process/coding-guidance.mdis excluded by none and included by nonedocs/public-error-audit.csvis excluded by!**/*.csvand included by nonedocs/tasks/000263-introduce-quad-error-and-narrow-audited-error-convergence.mdis excluded by none and included by nonedocs/tasks/next-idis excluded by none and included by none
📒 Files selected for processing (15)
doradb-storage/src/catalog/index.rsdoradb-storage/src/catalog/table.rsdoradb-storage/src/conf/mod.rsdoradb-storage/src/conf/trx.rsdoradb-storage/src/engine.rsdoradb-storage/src/error.rsdoradb-storage/src/file/mod.rsdoradb-storage/src/log/mod.rsdoradb-storage/src/runtime/mandatory.rsdoradb-storage/src/session.rsdoradb-storage/src/table/access.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/stmt.rsdoradb-storage/src/trx/stream_stmt.rsdoradb-storage/src/trx/sys.rs
1 participant
Closes #960
Summary by CodeRabbit
Bug Fixes
Reliability