feat: implement mandatory operation driver and concurrent cleanup executor - #923
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 (9)
📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe change adds an engine-owned mandatory runtime with bounded admission, supervised workers, exclusive completions, and deterministic draining. Transaction ownership and cleanup now use mandatory tasks. Engine bootstrap, shutdown, worker registration, configuration, errors, and tests are updated. ChangesMandatory runtime and completion flow
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Engine
participant MandatoryRuntime
participant TransactionSystem
participant Session
Engine->>MandatoryRuntime: bootstrap workers and admission
Engine->>TransactionSystem: provide mandatory runtime
Session->>TransactionSystem: submit cleanup obligation
TransactionSystem->>MandatoryRuntime: submit internal cleanup task
MandatoryRuntime->>TransactionSystem: execute rollback and publish result
Engine->>MandatoryRuntime: close admission and drain tasks
🚥 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 | 2 medium |
🟢 Metrics 151 complexity · 1 duplication
Metric Results Complexity 151 Duplication 1
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 an engine-owned fixed-thread mandatory runtime and moves transaction cleanup onto its concurrent executor.
Confidence Score: 5/5The PR appears safe to merge because no eligible blocking follow-up failure remains. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/runtime/mandatory.rs | Adds the fixed-thread executor, separate caller/internal admission, terminal supervision, panic retention, and ordered runner shutdown. |
| doradb-storage/src/engine.rs | Integrates mandatory-runtime construction and draining into engine bootstrap, shutdown, and component ordering. |
| doradb-storage/src/trx/sys.rs | Replaces sequential cleanup dispatch with mandatory internal tasks and splits purge and redo worker lifecycle ownership. |
| doradb-storage/src/trx/mod.rs | Extends transaction cleanup claims and fatal-retention handling for concurrent supervised execution. |
| doradb-storage/src/session.rs | Adds mandatory operation authority and updates session-operation transitions and shutdown observation. |
| doradb-storage/src/conf/engine.rs | Adds validated, backward-compatible mandatory worker-thread and concurrency-limit configuration. |
| doradb-storage/src/component.rs | Documents and preserves the revised dependency and reverse-shutdown order. |
| doradb-storage/src/trx/purge.rs | Adapts purge startup and shutdown ownership to the split transaction worker components. |
| doradb-storage/src/trx/group.rs | Updates group-commit integration to submit failed-precommit cleanup through the mandatory runtime. |
| doradb-storage/src/completion.rs | Moves and generalizes completion support for runtime and storage users. |
Reviews (2): 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: 5
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/log/mod.rs (1)
953-974: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMake failed-precommit cleanup handle admission rejection without leaking waiters.
request_failed_precommit_cleanupreturnspanic!afterforget(job)whensubmit_internalreturnsErr(job). Unlike terminal rollback cleanup, this path has no caller that retains an outstandingFailedPrecommitCleanupJoband waits for fatal submission failure, so this panic can leave commit waiters ongroup.completionwithout terminal delivery. Run the rejectedFailedPrecommitCleanupJobinline, or fail the cloned completion through the same rejection invariant used by terminal rollback.🤖 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/log/mod.rs` around lines 953 - 974, The handoff_failed_precommit path must handle FailedPrecommitCleanupJob admission rejection without leaving group.completion waiters unresolved. Update request_failed_precommit_cleanup or its caller around FailedPrecommitCleanupJob::new so a rejected job is executed inline or its cloned completion is failed using the existing terminal rollback rejection invariant, rather than forgetting the job and panicking without terminal delivery.
🧹 Nitpick comments (9)
doradb-storage/src/session.rs (1)
3435-3471: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the armed-drop poison path.
This test covers only the two disarmed outcomes:
finishandfail_retained. It does not coverDropon an armed guard, which publishesFailedRetainedand poisons the engine withFatalError::MandatoryTaskPanic. That path is the panic-supervision contract for mandatory work, so it deserves a direct test. Assert that the entry reachesFailedRetainedand thatengine.inner().poisoner.poison_error()reportsFatalError::MandatoryTaskPanic.🤖 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 3435 - 3471, Extend test_operation_pin_consumes_into_mandatory_terminal_authority to drop an armed mandatory guard without calling finish or fail_retained, then assert its entry state becomes FailedRetained and engine.inner().poisoner.poison_error() reports FatalError::MandatoryTaskPanic. Keep the existing finish and fail_retained coverage unchanged.doradb-storage/src/engine.rs (1)
1142-1162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider shortening the redo-header failure test.
Codacy flags
test_initial_redo_header_failure_reclaims_started_workers_before_startup_returnsfor exceeding the 50-line limit (57 lines). Extract the shared started/finished bookkeeping into a small helper if this must pass the length check.🤖 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/engine.rs` around lines 1142 - 1162, Shorten test_initial_redo_header_failure_reclaims_started_workers_before_startup_returns below the 50-line limit by extracting its repeated started/finished worker bookkeeping into a small local helper. Preserve the existing worker-order assertions and test behavior while moving only the shared bookkeeping logic.Source: Linters/SAST tools
doradb-storage/src/conf/engine.rs (1)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
pub(crate)fields for API consistency.
EngineConfigkeeps its fieldspub(crate)and exposes only builder setters.MandatoryRuntimeConfigexposes both public fields and setters. Public fields let external callers set values without going through the setters, which creates two mutation paths for the same type.Validation still runs during component build, so this is a surface-consistency point rather than a correctness problem.
♻️ Proposed visibility change
pub struct MandatoryRuntimeConfig { /// Number of operating-system threads driving the mandatory executor. - pub worker_threads: usize, + pub(crate) worker_threads: usize, /// Maximum number of accepted caller operations. - pub concurrency_limit: usize, + pub(crate) concurrency_limit: usize, }🤖 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/conf/engine.rs` around lines 16 - 21, Change the worker_threads and concurrency_limit fields in MandatoryRuntimeConfig from public to pub(crate), keeping their existing builder setters as the external mutation path. Preserve the current validation and build behavior.doradb-storage/src/trx/sys.rs (2)
1799-1834: 🧹 Nitpick | 🔵 TrivialAdd a metric for in-flight abandoned cleanup tasks.
Each abandoned cleanup job now runs as an independent runtime task instead of a message on one sequential cleanup thread. Failures log an error at Line 1829, and the neutral early returns at Lines 1808 and 1813 log nothing.
Without a counter for submitted, neutral, and failed cleanup outcomes, a backlog of abandoned transactions is only visible as a slow shutdown. Consider counters for these three outcomes plus a gauge for in-flight cleanup tasks, so operators can distinguish a cleanup backlog from a shutdown blocked on something else.
🤖 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/sys.rs` around lines 1799 - 1834, Update run_trx_cleanup_job to instrument abandoned cleanup lifecycle metrics: increment the in-flight gauge when the task starts and decrement it on every exit path, including the neutral returns when operation resolution or claim creation fails; record counters for submitted, neutral, and failed cleanup outcomes, ensuring failures are counted alongside the existing error log.
479-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the operation key in terminal rollback task metadata.
TerminalRollbackCleanupJobkeepsClaimOperationCompletionClaim, while abandoned transaction cleanup already storesSessionOperationKeydirectly and passes it toMandatoryTaskMetadata::transaction_cleanup. Add the operation key to the terminal rollback claim metadata so a supervised panic includes the session identity inpoison_mandatory_panic.🤖 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/sys.rs` around lines 479 - 482, Update TerminalRollbackCleanupJob metadata() to extract and pass its retained SessionOperationKey to MandatoryTaskMetadata::transaction_cleanup instead of None, preserving the operation key in terminal rollback cleanup metadata for panic reporting.doradb-storage/src/completion.rs (1)
86-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the panic on
completed_result.
completed_resultnow panics when the cell was consumed exclusively. The doc comment above it still describes only the returned value. Add a# Panicssection so the mixed-use contract is visible at the call site.📝 Proposed doc addition
/// Returns the propagated terminal result if this completion has already /// finished. + /// + /// # Panics + /// + /// Panics when an exclusive observer already consumed the terminal result. + /// One completion cell must use either shared observation or exclusive + /// take, never both. #[inline] pub(crate) fn completed_result(&self) -> Option<CompletionResult<T>>🤖 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/completion.rs` around lines 86 - 94, Update the doc comment for completed_result to add a # Panics section documenting that it panics when the completion cell has already been exclusively consumed, while preserving the existing return-value documentation.doradb-storage/src/log/mod.rs (1)
3784-3847: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore coverage for the failed-precommit cleanup handoff.
This test previously asserted that the failed group produced a
FailedPrecommitcleanup message. The assertion was removed with the channel, and the test now checks only the poison state. The handoff inhandoff_failed_precommitis therefore unverified at this layer.Add an assertion that observes the new path, for example that the group completion resolves with the fatal reason after the mandatory cleanup task settles.
🤖 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/log/mod.rs` around lines 3784 - 3847, Extend test_sync_only_backend_wait_progress_failure_poisons_redo_sync to retain the failed group’s completion handle and assert it resolves with the expected fatal reason after wait_and_drain_io_if_submitted completes. Keep the existing poison-state assertion, and ensure the assertion exercises the cleanup handoff performed by handoff_failed_precommit.doradb-storage/src/runtime/mandatory.rs (2)
256-268: 🚀 Performance & Scalability | 🔵 TrivialInternal admission is unbounded.
try_acquireincrementsactivewithout any limit. Every rejected transaction group and every retained cleanup obligation therefore becomes a queued executor task with no backpressure. A burst of failed precommits during a redo outage can grow both the executor task list and the retained rollback payloads without bound, while the fixed runner count limits drain throughput.Internal cleanup must not be lossy, so a hard limit is not appropriate. Consider exporting
activeas a metric and adding an alert threshold, so operators can observe cleanup backlog growth before memory pressure appears.🤖 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/runtime/mandatory.rs` around lines 256 - 268, The try_acquire method currently provides no visibility into unbounded internal admission. Export the active cleanup count maintained by MandatoryRuntime as a metric, update it whenever permits are acquired or released, and expose a configurable alert threshold for operators to detect backlog growth; preserve the existing non-lossy admission behavior without adding a hard rejection limit.
989-1358: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for admission rejection after close.
The tests cover the success path, the accepted-execution panic path, the internal panic path, and runner spawn failure. Two rejection edges remain untested:
submitafterclose_admission, which must returnLifecycleError::Shutdown.submit_internalafter internal admission close, which must returnErr(job)and give the job back to the caller.The second edge is the one that protects transaction cleanup from silent loss, as described in the
handoff_failed_precommitcontract indoradb-storage/src/log/mod.rs.🤖 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/runtime/mandatory.rs` around lines 989 - 1358, Add tests covering both admission-rejection paths: after calling MandatoryRuntime::close_admission, verify submit returns LifecycleError::Shutdown; after closing internal admission, verify submit_internal returns Err(job) and the original job is recoverable by the caller. Use the existing test setup and synthetic task types in the tests module, and ensure the internal rejection preserves the handoff_failed_precommit contract.
🤖 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/completion.rs`:
- Around line 15-21: Fix the cfg_attr expect(dead_code) annotations for
Completion<T>, CompletionTake<T>, CompletionProducer::complete, and
CompletionObserver::wait in non-test builds. Use valid cfg_attr syntax and place
each attribute directly on its target item, after any comments if necessary, so
the expected dead-code lint is applied while test builds remain unaffected.
In `@doradb-storage/src/engine.rs`:
- Around line 393-431: Update the blocker classification in the shutdown-busy
path around first_shutdown_blocker so mandatory_session_blocked also matches
CleanupReady and Completing, in addition to Mandatory(_). Keep voluntary_blocked
limited to Voluntary(_) and align the classification with Drop::drop.
In `@doradb-storage/src/runtime/mandatory.rs`:
- Around line 838-848: Update the shutdown sequence in the handle-join path to
avoid aborting when executor.is_empty() is temporarily false after
signal_stop(); either remove the assertion or replace it with a bounded
wait/loop that allows the stop wake future to reach quiescence. Preserve the
existing admission drain, stop signaling, and runner-join behavior.
In `@doradb-storage/src/trx/mod.rs`:
- Around line 1942-1950: Update the test-only TrxID accessor trx_id to avoid
relying on Option::expect in a const fn: either remove the const qualifier or
replace the assertion with logic supported by the declared Rust toolchain.
Preserve the existing behavior of returning the retained attachment’s
transaction ID and detecting an absent attachment.
- Around line 1660-1718: The existing operation-state tests lack transition
coverage for the mandatory lifecycle. Add focused unit tests around
accept_mandatory, finish_mandatory, and fail_mandatory_retained that verify
Voluntary(None) transitions to Mandatory(None) then Terminal, and Mandatory(_)
transitions to FailedRetained, including the expected published state after each
call.
---
Outside diff comments:
In `@doradb-storage/src/log/mod.rs`:
- Around line 953-974: The handoff_failed_precommit path must handle
FailedPrecommitCleanupJob admission rejection without leaving group.completion
waiters unresolved. Update request_failed_precommit_cleanup or its caller around
FailedPrecommitCleanupJob::new so a rejected job is executed inline or its
cloned completion is failed using the existing terminal rollback rejection
invariant, rather than forgetting the job and panicking without terminal
delivery.
---
Nitpick comments:
In `@doradb-storage/src/completion.rs`:
- Around line 86-94: Update the doc comment for completed_result to add a #
Panics section documenting that it panics when the completion cell has already
been exclusively consumed, while preserving the existing return-value
documentation.
In `@doradb-storage/src/conf/engine.rs`:
- Around line 16-21: Change the worker_threads and concurrency_limit fields in
MandatoryRuntimeConfig from public to pub(crate), keeping their existing builder
setters as the external mutation path. Preserve the current validation and build
behavior.
In `@doradb-storage/src/engine.rs`:
- Around line 1142-1162: Shorten
test_initial_redo_header_failure_reclaims_started_workers_before_startup_returns
below the 50-line limit by extracting its repeated started/finished worker
bookkeeping into a small local helper. Preserve the existing worker-order
assertions and test behavior while moving only the shared bookkeeping logic.
In `@doradb-storage/src/log/mod.rs`:
- Around line 3784-3847: Extend
test_sync_only_backend_wait_progress_failure_poisons_redo_sync to retain the
failed group’s completion handle and assert it resolves with the expected fatal
reason after wait_and_drain_io_if_submitted completes. Keep the existing
poison-state assertion, and ensure the assertion exercises the cleanup handoff
performed by handoff_failed_precommit.
In `@doradb-storage/src/runtime/mandatory.rs`:
- Around line 256-268: The try_acquire method currently provides no visibility
into unbounded internal admission. Export the active cleanup count maintained by
MandatoryRuntime as a metric, update it whenever permits are acquired or
released, and expose a configurable alert threshold for operators to detect
backlog growth; preserve the existing non-lossy admission behavior without
adding a hard rejection limit.
- Around line 989-1358: Add tests covering both admission-rejection paths: after
calling MandatoryRuntime::close_admission, verify submit returns
LifecycleError::Shutdown; after closing internal admission, verify
submit_internal returns Err(job) and the original job is recoverable by the
caller. Use the existing test setup and synthetic task types in the tests
module, and ensure the internal rejection preserves the handoff_failed_precommit
contract.
In `@doradb-storage/src/session.rs`:
- Around line 3435-3471: Extend
test_operation_pin_consumes_into_mandatory_terminal_authority to drop an armed
mandatory guard without calling finish or fail_retained, then assert its entry
state becomes FailedRetained and engine.inner().poisoner.poison_error() reports
FatalError::MandatoryTaskPanic. Keep the existing finish and fail_retained
coverage unchanged.
In `@doradb-storage/src/trx/sys.rs`:
- Around line 1799-1834: Update run_trx_cleanup_job to instrument abandoned
cleanup lifecycle metrics: increment the in-flight gauge when the task starts
and decrement it on every exit path, including the neutral returns when
operation resolution or claim creation fails; record counters for submitted,
neutral, and failed cleanup outcomes, ensuring failures are counted alongside
the existing error log.
- Around line 479-482: Update TerminalRollbackCleanupJob metadata() to extract
and pass its retained SessionOperationKey to
MandatoryTaskMetadata::transaction_cleanup instead of None, preserving the
operation key in terminal rollback cleanup metadata for panic reporting.
🪄 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: c94b1244-c66f-4aac-8370-079158d1b548
⛔ Files ignored due to path filters (8)
Cargo.tomlis excluded by none and included by nonedocs/architecture.mdis excluded by none and included by nonedocs/engine-component-lifetime.mdis excluded by none and included by nonedocs/public-error-audit.csvis excluded by!**/*.csvand included by nonedocs/rfcs/0026-engine-owned-mandatory-background-runtime.mdis excluded by none and included by nonedocs/tasks/000248-mandatory-operation-driver-and-concurrent-cleanup-executor.mdis excluded by none and included by nonedocs/tasks/next-idis 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/Cargo.tomldoradb-storage/src/buffer/mod.rsdoradb-storage/src/completion.rsdoradb-storage/src/component.rsdoradb-storage/src/conf/engine.rsdoradb-storage/src/conf/mod.rsdoradb-storage/src/engine.rsdoradb-storage/src/error.rsdoradb-storage/src/file/fs.rsdoradb-storage/src/file/mod.rsdoradb-storage/src/io/mod.rsdoradb-storage/src/lib.rsdoradb-storage/src/lock/mod.rsdoradb-storage/src/log/mod.rsdoradb-storage/src/log/prefix.rsdoradb-storage/src/runtime/mandatory.rsdoradb-storage/src/runtime/mod.rsdoradb-storage/src/session.rsdoradb-storage/src/table/persistence.rsdoradb-storage/src/trx/group.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/sys.rs
💤 Files with no reviewable changes (1)
- doradb-storage/src/io/mod.rs
jiangzhe
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #923 +/- ##
==========================================
- Coverage 93.32% 93.31% -0.01%
==========================================
Files 147 148 +1
Lines 122122 123127 +1005
==========================================
+ Hits 113970 114898 +928
- Misses 8152 8229 +77 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Closes #922
Summary by CodeRabbit