refactor: implement runtime-owned mandatory maintenance - #930
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR moves catalog, table, redo-retention, and MemIndex maintenance to prepared, lifetime-free operations owned by the mandatory runtime. It replaces borrowed leases with explicit scopes and engine-scoped maintenance test controls. Tests use session-level catalog checkpointing. ChangesRuntime-owned maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 | 9 medium |
🟢 Metrics 77 complexity · -12 duplication
Metric Results Complexity 77 Duplication -12
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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #930 +/- ##
==========================================
+ Coverage 93.33% 93.35% +0.01%
==========================================
Files 148 149 +1
Lines 124538 125534 +996
==========================================
+ Hits 116241 117191 +950
- Misses 8297 8343 +46 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR moves mandatory maintenance operations from caller-owned futures to the engine’s supervised runtime so accepted work survives cancellation and observer loss.
Confidence Score: 5/5The PR appears safe to merge because no eligible new finding or known outstanding prior finding remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/session.rs | Introduces prepared and accepted maintenance scopes that transfer session-operation and logical-lock ownership to supervised execution. |
| doradb-storage/src/runtime/mandatory.rs | Integrates the generalized accepted-maintenance execution contract with mandatory runtime admission and completion. |
| doradb-storage/src/table/persistence.rs | Refactors table checkpoint execution and retry observation to operate with transferred workflow and root-mutation authority. |
| doradb-storage/src/table/checkpoint_workflow.rs | Revises freeze and checkpoint attempt ownership while retaining restoration of canonical frozen batches on reversible exits. |
| doradb-storage/src/table/gc.rs | Moves secondary MemIndex cleanup into accepted mandatory execution with owned table and transaction progress. |
| doradb-storage/src/catalog/checkpoint.rs | Adds owned catalog-checkpoint resources and execution integration for mandatory maintenance. |
| doradb-storage/src/trx/retention.rs | Coordinates catalog publication, durable redo-retention markers, and obsolete-file cleanup under transferred gates. |
| doradb-storage/src/trx/sys.rs | Adds redo-retention scope ownership and progress tracking used by mandatory maintenance. |
| doradb-storage/src/table/lifecycle.rs | Adds lifetime-independent checkpoint root-mutation authority for transfer across runtime admission. |
| doradb-storage/src/latch/gate.rs | Introduces an asynchronous exclusive gate used to serialize maintenance domains. |
Reviews (3): Last reviewed commit: "fix issues and resolve task" | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
doradb-storage/src/table/checkpoint_workflow.rs (1)
320-394: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftDeduplicate the two checkpoint admission paths and their drop logic.
Table::begin_checkpointandTableCheckpointWorkflow::begin_checkpointnow contain the same admission state machine, andDrop for CheckpointAttemptandDrop for PreparedCheckpointAttemptcontain the same restore logic. Only the receiver and the attempt type differ.Both forms are needed: the retry-observation path in
table/persistence.rsuses the borrowed attempt, and the prepared path uses the lifetime-free attempt. The risk is divergence. A later change to theFrozenversusIdlerestore rules must be applied in two places, and a missed copy leaves the workflow in a wrong state without a compile error.Extract the shared parts. Move the admission body into one private helper on
TableCheckpointWorkflowthat returns(CheckpointSource, Option<FrozenPageBatch>), and move the restore body into one private helper that takes&TableCheckpointWorkflow,CheckpointSource, and&mut Option<FrozenPageBatch>. Both entry points and bothDropimplementations then call the helpers.Also applies to: 609-693
🤖 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/table/checkpoint_workflow.rs` around lines 320 - 394, Deduplicate checkpoint admission and restore logic by extracting the shared state-machine body from both begin_checkpoint methods into a private TableCheckpointWorkflow helper returning (CheckpointSource, Option<FrozenPageBatch>). Extract the common Drop restoration logic into a private helper accepting &TableCheckpointWorkflow, CheckpointSource, and &mut Option<FrozenPageBatch>, then have both entry points and both Drop implementations delegate to these helpers while preserving their borrowed and lifetime-free attempt types.doradb-storage/src/trx/retention.rs (1)
368-384: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the doc comment to the scope model.
The doc still describes a "catalog checkpoint lease" and a "redo-retention lease" acquired in this method. This PR replaces both with
CatalogCheckpointScopeandRedoRetentionScope, and the caller acquires them before preparation. The method now only releases catalog authority throughrelease_catalog. Rewrite the ordering paragraph so it states the caller-held scopes and the release point.📝 Proposed doc update
- /// Lock ordering matches catalog checkpoint: acquire the catalog checkpoint - /// lease before the redo-retention lease. The catalog lease protects the - /// `catalog.mtb` root fork used to publish `first_redo_log_seq`, while the - /// redo-retention lease protects the retained redo suffix, catalog-safe - /// progress cache, and cleanup below the marker. They are separate because - /// the marker is catalog bootstrap metadata, but unlink races are about the - /// redo file family rather than catalog metadata shape. + /// The caller acquires `CatalogCheckpointScope` before `RedoRetentionScope` + /// and transfers both into the accepted operation. The catalog scope + /// protects the `catalog.mtb` root fork used to publish + /// `first_redo_log_seq`. The redo-retention scope protects the retained + /// redo suffix, the catalog-safe progress cache, and cleanup below the + /// marker. This method releases only catalog authority, through + /// `release_catalog`, before filesystem cleanup. The scopes are separate + /// because the marker is catalog bootstrap metadata, but unlink races are + /// about the redo file family rather than catalog metadata shape.🤖 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/retention.rs` around lines 368 - 384, Update the doc comment for truncate_redo_log_prepared to describe caller-held CatalogCheckpointScope and RedoRetentionScope rather than leases acquired in the method, preserving their ordering and responsibilities. State that the method only releases catalog authority through the release_catalog callback, and remove claims that it acquires either scope.
🧹 Nitpick comments (7)
doradb-storage/src/table/checkpoint_workflow.rs (1)
771-783: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
PreparedCheckpointAttemptdrop restore.This test exercises the borrowed
TableCheckpointWorkflow::begin_checkpointandDrop for CheckpointAttempt. The newTable::begin_checkpointandDrop for PreparedCheckpointAttempthave no equivalent test in this file.table/persistence.rsaddstest_prepared_freeze_attempt_drop_restores_idlefor the freeze path only.Add a test that admits a checkpoint through
Table::begin_checkpointfrom aFrozenstate, drops the attempt, and asserts the workflow returns toFrozenwith the batch intact. That path is where the duplicated restore logic can diverge.🤖 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/table/checkpoint_workflow.rs` around lines 771 - 783, Add a test alongside test_reversible_checkpoint_attempt_restores_admitted_state covering Table::begin_checkpoint and PreparedCheckpointAttempt drop. Transition a Table to Frozen with an admitted batch, begin and drop the checkpoint attempt, then assert the workflow is Frozen and the original batch remains intact.doradb-storage/src/table/persistence.rs (2)
2305-2320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a guard for the secondary-sidecar failure flag.
set_test_force_secondary_sidecar_errorsets an engine-scoped flag with no RAII reset, while the two neighboring flags provideForcePostPublishCheckpointErrorGuardandForceCheckpointCommitErrorGuard.test_secondary_sidecar_failure_keeps_checkpoint_root_atomictherefore hand-rollsResetSidecarHook. Because the flag now lives on the engine instead of a thread-local, a test that forgets the reset leaves the failure injected for every later operation on that engine.Add a
ForceSecondarySidecarErrorGuardnext to the other two guards and use it in the test.♻️ Proposed guard
+ pub(crate) struct ForceSecondarySidecarErrorGuard { + test: MaintenanceTestController, + } + + impl ForceSecondarySidecarErrorGuard { + pub(crate) fn new(engine: &Engine) -> Self { + let test = engine.inner().maintenance_test.clone(); + test.set_force_secondary_sidecar_error(true); + Self { test } + } + } + + impl Drop for ForceSecondarySidecarErrorGuard { + fn drop(&mut self) { + self.test.set_force_secondary_sidecar_error(false); + } + }Then in the test:
- struct ResetSidecarHook(MaintenanceTestController); - - impl Drop for ResetSidecarHook { - fn drop(&mut self) { - self.0.set_force_secondary_sidecar_error(false); - } - } - ... - set_test_force_secondary_sidecar_error(&engine, true); - let _reset = ResetSidecarHook(engine.inner().maintenance_test.clone()); + let _reset = ForceSecondarySidecarErrorGuard::new(&engine);Also applies to: 3316-3357
🤖 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/table/persistence.rs` around lines 2305 - 2320, Add a RAII-style ForceSecondarySidecarErrorGuard alongside ForcePostPublishCheckpointErrorGuard and ForceCheckpointCommitErrorGuard, ensuring it enables the engine-scoped secondary-sidecar failure flag on creation and resets it on drop. Update test_secondary_sidecar_failure_keeps_checkpoint_root_atomic to use this guard instead of the hand-rolled ResetSidecarHook, while preserving the existing failure-injection behavior.
4055-4102: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the observer drop explicit.
The test name states that the observer is dropped, but the freeze future is dropped implicitly when the block at lines 4071-4099 ends. A reader must infer the drop point from the brace. The other dropped-observer tests in this file call
drop(checkpoint)explicitly.Bind the future outside the block and call
dropbeforerelease_tx.send_async, so the acceptance-boundary behavior under test is visible at the assertion site.🤖 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/table/persistence.rs` around lines 4055 - 4102, Make the freeze future’s cancellation explicit in test_dropped_freeze_observer_does_not_cancel_loading: bind the freeze future outside the scoped block, retain the existing polling and competing-session assertions, then call drop on the freeze future immediately before release_tx.send_async. Ensure the observer is visibly dropped at the acceptance boundary rather than only by scope exit.doradb-storage/src/session.rs (3)
492-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider sharing the accepted-scope state machine with
AcceptedDdlScope.
MaintenanceFinishStateandAcceptedMaintenanceScopeduplicateDdlFinishStateandAcceptedDdlScopeat lines 300-378. The only difference iscatalog_write_authority. Thefinish,handle_panic, andmark_terminal_readybodies are identical, so a later fix to the terminal-edge ordering must be applied twice.A generic carrier over the retained lock type, or a shared inner struct that both scopes embed, would keep one implementation of the terminal-state rules. This is not required before merge.
Also note lines 505-516 and 564-574:
AcceptedMaintenanceScopedefines inherentengine()andpool_guards()that shadow the identicalSessionRuntimeAccessmembers. Removing the inherent copies keeps one resolution path.🤖 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 492 - 574, Share the finish-state and terminal-transition implementation between AcceptedDdlScope and AcceptedMaintenanceScope by introducing a generic carrier or shared inner state over their differing retained lock types, so finish, handle_panic, and mark_terminal_ready have one implementation. Remove the redundant inherent engine() and pool_guards() methods from AcceptedMaintenanceScope and rely on SessionRuntimeAccess for those members.
615-654: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional: fold the remaining
ScopedTableRuntimeAccessinto the prepared scope.After this PR,
ScopedTableRuntimeAccessserves onlytotal_row_pagesat line 1321. It acquires the same metadata S plus data IS set asPreparedMaintenanceLocks::acquire_table, but throughacquire_table_locksinstead ofOwnerLockState. The two paths currently agree on resource order. Keeping one acquisition path would remove the risk that a future order change is applied to only one of them.
total_row_pagesdoes not submit a mandatory operation, so this is not required now.🤖 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 615 - 654, Optionally remove the standalone ScopedTableRuntimeAccess acquisition path and fold total_row_pages into the existing PreparedMaintenanceLocks::acquire_table flow. Reuse OwnerLockState-based lock acquisition and the prepared scope’s live-table resolution, preserving metadata S plus data IS ordering and total_row_pages behavior; update only the affected helper and call sites.
466-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate
resolve_user_tableto the pinned operation.
PreparedMaintenanceScope::resolve_user_tablerepeats the body ofSessionOperationPin::resolve_user_tableat lines 1561-1572. The two copies can diverge when catalog validation or cache behavior changes.♻️ Proposed delegation
pub(crate) async fn resolve_user_table( &self, table_id: TableID, ) -> OperationResult<Arc<Table>> { - let table = self - .operation - .engine - .catalog() - .validate_user_table_live(table_id) - .await?; - self.operation.state.cache_user_table(&table); - Ok(table) + self.operation.resolve_user_table(table_id).await }🤖 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 466 - 478, Update PreparedMaintenanceScope::resolve_user_table to delegate directly to the pinned SessionOperationPin::resolve_user_table implementation instead of repeating catalog validation and cache logic; preserve the existing table_id input and OperationResult<Arc<Table>> behavior.doradb-storage/src/table/mod.rs (1)
1509-1575: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
FnMuthooks run while the controller lock is held.
run_frozen_page_scan_hook,run_frozen_page_row_scan_hook, andrun_optimistic_page_plan_comparison_hookinvoke the closure while holding theparking_lot::Mutex. The previous thread-local hooks could not be reached from another thread. This controller is engine-scoped and shared across mandatory-runtime threads, so a hook that blocks now stalls every other thread that reaches the same hook, and a hook that re-enters the same scan path self-deadlocks.The existing hook in
doradb-storage/src/table/page_transition.rsat line 1001 sends on aflume::bounded(1)channel and is safe only because the channel has spare capacity. Add a doc comment on these three methods that states the closure must not block and must not re-enter the scan path.The one-shot async runners already take the hook out of the lock before awaiting, so they are unaffected.
📝 Proposed documentation
+ /// Runs the frozen-page scan hook. + /// + /// The hook runs while the shared controller lock is held. The hook + /// must not block and must not re-enter frozen-page scanning. pub(crate) fn run_frozen_page_scan_hook(&self, page_id: PageID) { if let Some(hook) = self.state.frozen_page_scan_hook.lock().as_mut() { hook(page_id); } }🤖 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/table/mod.rs` around lines 1509 - 1575, Add doc comments to run_frozen_page_scan_hook, run_frozen_page_row_scan_hook, and run_optimistic_page_plan_comparison_hook stating that each FnMut closure executes while the controller Mutex is held, must not block, and must not re-enter the scan path. Leave the one-shot async runners unchanged.
🤖 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/catalog/checkpoint.rs`:
- Around line 354-442: Replace the duplicated prepared/accepted carriers with
one generic shared carrier that owns AcceptedMaintenanceScope, each operation’s
resource tuple, and panic-label data, implementing finish and handle_panic once.
Update doradb-storage/src/catalog/checkpoint.rs:354-442,
doradb-storage/src/trx/retention.rs:120-215 and 217-313,
doradb-storage/src/table/persistence.rs:127-238 and 240-349, and
doradb-storage/src/table/gc.rs:183-291 so each operation supplies only its
execute body and resources; preserve catalog/redo release order, release_catalog
callbacks, attempt-before-root-mutation ordering, and MemIndexCleanupPhase panic
attachment.
In `@doradb-storage/src/table/gc.rs`:
- Around line 301-375: Bound the retry loop in execute_inner by tracking
attempts and, after a small fixed maximum, return a RuntimeError::TableAccess
report containing the table ID and attempt count. Increment the counter for each
iteration, preserve the existing rollback and retry behavior below the limit,
and keep successful cleanup unchanged.
---
Outside diff comments:
In `@doradb-storage/src/table/checkpoint_workflow.rs`:
- Around line 320-394: Deduplicate checkpoint admission and restore logic by
extracting the shared state-machine body from both begin_checkpoint methods into
a private TableCheckpointWorkflow helper returning (CheckpointSource,
Option<FrozenPageBatch>). Extract the common Drop restoration logic into a
private helper accepting &TableCheckpointWorkflow, CheckpointSource, and &mut
Option<FrozenPageBatch>, then have both entry points and both Drop
implementations delegate to these helpers while preserving their borrowed and
lifetime-free attempt types.
In `@doradb-storage/src/trx/retention.rs`:
- Around line 368-384: Update the doc comment for truncate_redo_log_prepared to
describe caller-held CatalogCheckpointScope and RedoRetentionScope rather than
leases acquired in the method, preserving their ordering and responsibilities.
State that the method only releases catalog authority through the
release_catalog callback, and remove claims that it acquires either scope.
---
Nitpick comments:
In `@doradb-storage/src/session.rs`:
- Around line 492-574: Share the finish-state and terminal-transition
implementation between AcceptedDdlScope and AcceptedMaintenanceScope by
introducing a generic carrier or shared inner state over their differing
retained lock types, so finish, handle_panic, and mark_terminal_ready have one
implementation. Remove the redundant inherent engine() and pool_guards() methods
from AcceptedMaintenanceScope and rely on SessionRuntimeAccess for those
members.
- Around line 615-654: Optionally remove the standalone ScopedTableRuntimeAccess
acquisition path and fold total_row_pages into the existing
PreparedMaintenanceLocks::acquire_table flow. Reuse OwnerLockState-based lock
acquisition and the prepared scope’s live-table resolution, preserving metadata
S plus data IS ordering and total_row_pages behavior; update only the affected
helper and call sites.
- Around line 466-478: Update PreparedMaintenanceScope::resolve_user_table to
delegate directly to the pinned SessionOperationPin::resolve_user_table
implementation instead of repeating catalog validation and cache logic; preserve
the existing table_id input and OperationResult<Arc<Table>> behavior.
In `@doradb-storage/src/table/checkpoint_workflow.rs`:
- Around line 771-783: Add a test alongside
test_reversible_checkpoint_attempt_restores_admitted_state covering
Table::begin_checkpoint and PreparedCheckpointAttempt drop. Transition a Table
to Frozen with an admitted batch, begin and drop the checkpoint attempt, then
assert the workflow is Frozen and the original batch remains intact.
In `@doradb-storage/src/table/mod.rs`:
- Around line 1509-1575: Add doc comments to run_frozen_page_scan_hook,
run_frozen_page_row_scan_hook, and run_optimistic_page_plan_comparison_hook
stating that each FnMut closure executes while the controller Mutex is held,
must not block, and must not re-enter the scan path. Leave the one-shot async
runners unchanged.
In `@doradb-storage/src/table/persistence.rs`:
- Around line 2305-2320: Add a RAII-style ForceSecondarySidecarErrorGuard
alongside ForcePostPublishCheckpointErrorGuard and
ForceCheckpointCommitErrorGuard, ensuring it enables the engine-scoped
secondary-sidecar failure flag on creation and resets it on drop. Update
test_secondary_sidecar_failure_keeps_checkpoint_root_atomic to use this guard
instead of the hand-rolled ResetSidecarHook, while preserving the existing
failure-injection behavior.
- Around line 4055-4102: Make the freeze future’s cancellation explicit in
test_dropped_freeze_observer_does_not_cancel_loading: bind the freeze future
outside the scoped block, retain the existing polling and competing-session
assertions, then call drop on the freeze future immediately before
release_tx.send_async. Ensure the observer is visibly dropped at the acceptance
boundary rather than only by scope exit.
🪄 Autofix (Beta)
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: 52193552-0ae5-4012-a466-dc5e19c1f077
⛔ Files ignored due to path filters (9)
docs/architecture.mdis excluded by none and included by nonedocs/checkpoint.mdis excluded by none and included by nonedocs/engine-component-lifetime.mdis excluded by none and included by nonedocs/lock-system.mdis excluded by none and included by nonedocs/public-error-audit.csvis excluded by!**/*.csvand included by nonedocs/tasks/000251-runtime-owned-mandatory-maintenance.mdis excluded by none and included by nonedocs/tasks/next-idis excluded by none and included by nonedocs/transaction-system.mdis excluded by none and included by nonedocs/unsafe-usage-baseline.mdis excluded by none and included by none
📒 Files selected for processing (22)
doradb-storage/src/catalog/checkpoint.rsdoradb-storage/src/catalog/index.rsdoradb-storage/src/catalog/mod.rsdoradb-storage/src/catalog/storage/mod.rsdoradb-storage/src/catalog/table.rsdoradb-storage/src/engine.rsdoradb-storage/src/latch/gate.rsdoradb-storage/src/latch/mod.rsdoradb-storage/src/recovery/mod.rsdoradb-storage/src/runtime/mandatory.rsdoradb-storage/src/session.rsdoradb-storage/src/table/access.rsdoradb-storage/src/table/checkpoint_workflow.rsdoradb-storage/src/table/gc.rsdoradb-storage/src/table/lifecycle.rsdoradb-storage/src/table/mod.rsdoradb-storage/src/table/page_transition.rsdoradb-storage/src/table/persistence.rsdoradb-storage/src/table/recover.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/retention.rsdoradb-storage/src/trx/sys.rs
💤 Files with no reviewable changes (1)
- doradb-storage/src/runtime/mandatory.rs
There was a problem hiding this comment.
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/session.rs (1)
1174-1183: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSettle the maintenance operation when preparing without accepted scope.
The return paths at
poisoner.ensure_healthyand theprepare_*_operationfailures dropPreparedMaintenanceScope, but they never call the mandatoryfinish/fail_retainedflow, so the active operation stays retained. These global-scope methods can also leave catalog and redo retention guards held if the scope is dropped before work starts.🤖 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 1174 - 1183, Update the global maintenance preparation flow around PreparedMaintenanceScope, poisoner.ensure_healthy, and prepare_catalog_checkpoint_operation so every early failure settles the operation through the mandatory finish/fail_retained path before returning. Ensure catalog_scope and redo_scope retention guards are explicitly released or settled when preparation fails or the scope is dropped before work starts.
🤖 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.
Outside diff comments:
In `@doradb-storage/src/session.rs`:
- Around line 1174-1183: Update the global maintenance preparation flow around
PreparedMaintenanceScope, poisoner.ensure_healthy, and
prepare_catalog_checkpoint_operation so every early failure settles the
operation through the mandatory finish/fail_retained path before returning.
Ensure catalog_scope and redo_scope retention guards are explicitly released or
settled when preparation fails or the scope is dropped before work starts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0959bfe8-031c-4da4-a659-782670b33cf9
📒 Files selected for processing (10)
doradb-storage/src/catalog/checkpoint.rsdoradb-storage/src/catalog/mod.rsdoradb-storage/src/catalog/table.rsdoradb-storage/src/session.rsdoradb-storage/src/table/gc.rsdoradb-storage/src/table/mod.rsdoradb-storage/src/table/persistence.rsdoradb-storage/src/table/recover.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/retention.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- doradb-storage/src/trx/mod.rs
- doradb-storage/src/catalog/table.rs
- doradb-storage/src/table/recover.rs
- doradb-storage/src/table/mod.rs
- doradb-storage/src/table/gc.rs
- doradb-storage/src/table/persistence.rs
1 participant
Closes #928
Summary by CodeRabbit
Reliability
Consistency
Testing