refactor: harden mandatory runtime lifecycle fairness stats and readiness - #932
Conversation
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| Complexity | 3 medium |
🟢 Metrics 42 complexity · -4 duplication
Metric Results Complexity 42 Duplication -4
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.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe mandatory runtime now records operation and transaction-cleanup lifecycle statistics, exposes them through ChangesMandatory runtime lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Session
participant MandatoryRuntime
participant OperationTask
participant CleanupTask
Session->>MandatoryRuntime: Submit operation
MandatoryRuntime->>OperationTask: Supervise and record lifecycle
MandatoryRuntime->>CleanupTask: Submit transaction cleanup
CleanupTask-->>MandatoryRuntime: Publish completion metrics
Session->>MandatoryRuntime: Request statistics
MandatoryRuntime-->>Session: Return runtime snapshot
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 |
Greptile SummaryThis PR expands mandatory-runtime diagnostics and statistics, adds cooperative scheduling points to long-running index and undo work, and improves lifecycle readiness documentation.
Confidence Score: 5/5The PR appears safe to merge. The previously reported completion-ordering defect is fixed: mandatory operation terminal counters are updated before result publication wakes the observer, and no blocking failure remains.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/runtime/mandatory.rs | Adds fixed-class runtime counters and task events; the current completion path records terminal counters before waking result observers, resolving the prior finding. |
| doradb-storage/src/stats.rs | Defines the public mandatory-runtime snapshot types and documents monotonic versus independently sampled fields. |
| doradb-storage/src/session.rs | Exposes mandatory-runtime statistics through the existing read-only session inspection boundary. |
| doradb-bench/src/output.rs | Adds one engine-global mandatory-runtime snapshot to benchmark internal-stat output without per-session duplication. |
| doradb-storage/src/catalog/index.rs | Adds explicit cooperative scheduling boundaries to batched index construction work. |
| doradb-storage/src/trx/undo/index.rs | Adds bounded cooperative yields while preserving reverse index-undo processing. |
| doradb-storage/src/trx/undo/row.rs | Adds bounded cooperative yields after completed row-undo entries. |
| doradb-storage/src/engine.rs | Enriches shutdown diagnostics with origin and operation-state details while retaining the existing drain behavior. |
Reviews (2): Last reviewed commit: "fix issues" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #932 +/- ##
==========================================
+ Coverage 93.36% 93.38% +0.02%
==========================================
Files 149 149
Lines 125534 126233 +699
==========================================
+ Hits 117203 117881 +678
- Misses 8331 8352 +21 ☔ 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: 6
🧹 Nitpick comments (5)
doradb-storage/src/runtime/mandatory.rs (2)
1583-1598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated runtime-construction boilerplate into a test helper.
Six tests in this module repeat the same eleven-line block: build
RegistryBuilder, buildEnginePoisoner, buildMandatoryRuntimewith a config, buildMandatoryRuntimeWorkers, finish, and take the dependency. Codacy reports both new tests over the 50-line limit, and the duplicated setup is the bulk of that length.Add one helper and call it from each test.
♻️ Proposed helper
async fn build_test_runtime( worker_threads: usize, concurrency_limit: usize, ) -> (Registry, QuiescentGuard<MandatoryRuntime>) { let mut builder = RegistryBuilder::new(); builder.build::<EnginePoisoner>(()).await.unwrap(); builder .build::<MandatoryRuntime>( MandatoryRuntimeConfig::default() .worker_threads(worker_threads) .concurrency_limit(concurrency_limit), ) .await .unwrap(); builder.build::<MandatoryRuntimeWorkers>(()).await.unwrap(); let registry = builder.finish(); let mandatory = registry.dependency::<MandatoryRuntime>(); (registry, mandatory) }- let mut builder = RegistryBuilder::new(); - builder.build::<EnginePoisoner>(()).await.unwrap(); - builder - .build::<MandatoryRuntime>( - MandatoryRuntimeConfig::default() - .worker_threads(1) - .concurrency_limit(1), - ) - .await - .unwrap(); - builder.build::<MandatoryRuntimeWorkers>(()).await.unwrap(); - let registry = builder.finish(); - let mandatory = registry.dependency::<MandatoryRuntime>(); + let (registry, mandatory) = build_test_runtime(1, 1).await;As per coding guidelines: "Follow the project's unit-testing guidance from
docs/process/unit-test.mdwhen adding or modifying Rust tests."Also applies to: 1642-1657
🤖 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 1583 - 1598, Extract the repeated RegistryBuilder setup into an async build_test_runtime helper accepting worker_threads and concurrency_limit, returning the Registry and QuiescentGuard<MandatoryRuntime>. Replace the duplicated construction blocks in all six affected tests, including the tests around ordinary_error_and_observer_detach_are_counted_by_outcome and the additional referenced range, while preserving each test’s configuration values and behavior.Sources: Coding guidelines, Linters/SAST tools
1132-1140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStore these nanosecond counters as
u64.
duration_nanosconvertsu128nanoseconds tousize, so the value is truncated before it reachesadmission_wait_nanos,queue_wait_nanos, andexecution_nanos. Useu64counters to avoid changing the field type and make the intended 292-year accumulator range explicit.🤖 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 1132 - 1140, Update elapsed_nanos and duration_nanos to return u64, converting Duration::as_nanos() to u64 so admission_wait_nanos, queue_wait_nanos, and execution_nanos retain their existing field type without usize truncation.doradb-bench/src/output.rs (2)
602-606: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark the absolute metric so consumers do not read it as a delta.
Every other metric this function emits is a before/after delta.
active_countis theaftervalue. The exported name gives no hint of the difference, so a CSV consumer comparingmandatory.operation.submitted_countagainstmandatory.operation.active_countwill apply the wrong interpretation. Add a code comment, and consider a distinguishing suffix such asactive_count_current.🤖 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-bench/src/output.rs` around lines 602 - 606, Update the metric emission around push_metric for after.active_count to clearly identify it as an absolute after/current value rather than a before/after delta. Rename the exported metric to a distinguishing name such as active_count_current, and add a code comment documenting that this metric is absolute.
563-622: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the ten repeated
push_metriccalls into a table.Nine of the ten calls follow the same shape: a name suffix plus a
deltaof the same field onafterandbefore. Codacy reports the function at 60 lines against a 50-line limit. A slice of suffix-and-accessor pairs removes the repetition and keepsactive_countas the one explicit special case.♻️ Proposed refactor
fn push_mandatory_task_metrics( metrics: &mut Vec<Metric>, prefix: &str, before: MandatoryTaskStats, after: MandatoryTaskStats, ) { - push_metric( - metrics, - &format!("{prefix}.submitted_count"), - delta(after.submitted_count, before.submitted_count), - ); - // ... eight more delta metrics ... + let cumulative: [(&str, usize, usize); 9] = [ + ("submitted_count", after.submitted_count, before.submitted_count), + ("started_count", after.started_count, before.started_count), + ("completed_count", after.completed_count, before.completed_count), + ("error_count", after.error_count, before.error_count), + ("panic_count", after.panic_count, before.panic_count), + ( + "detached_observer_count", + after.detached_observer_count, + before.detached_observer_count, + ), + ("admission_wait_nanos", after.admission_wait_nanos, before.admission_wait_nanos), + ("queue_wait_nanos", after.queue_wait_nanos, before.queue_wait_nanos), + ("execution_nanos", after.execution_nanos, before.execution_nanos), + ]; + for (suffix, after_value, before_value) in cumulative { + push_metric( + metrics, + &format!("{prefix}.{suffix}"), + delta(after_value, before_value), + ); + } + // Active work is current state, not a cumulative counter. push_metric( metrics, &format!("{prefix}.active_count"), after.active_count as u128, ); }The emitted metric names and their order stay unchanged, so the test at Lines 877-912 still passes.
🤖 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-bench/src/output.rs` around lines 563 - 622, Refactor push_mandatory_task_metrics to emit the nine delta-based metrics through a table of suffix/accessor pairs, iterating over that table to call push_metric while preserving the existing names and order. Keep active_count as the sole explicit push_metric call using after.active_count.Source: Linters/SAST tools
doradb-storage/src/stats.rs (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that two fields stay zero for the cleanup task class.
MandatoryTaskStatsis shared byoperationandtransaction_cleanup.error_countanddetached_observer_countcan only become non-zero foroperation. Internal cleanup tasks return no result, so they have no ordinary error outcome and no observer. The current doc text says "caller tasks" and "caller observers", which hints at this but does not state it. State it directly so public consumers do not treat a zero as a signal.📝 Proposed doc change
- /// Number of accepted caller tasks that returned an ordinary error. + /// Number of accepted caller tasks that returned an ordinary error. + /// + /// This field is always zero for the `transaction_cleanup` class, because + /// internal cleanup tasks publish no ordinary result. pub error_count: usize, /// Number of tasks whose supervised execution panicked. pub panic_count: usize, - /// Number of caller observers dropped without consuming their result. + /// Number of caller observers dropped without consuming their result. + /// + /// This field is always zero for the `transaction_cleanup` class, because + /// internal cleanup tasks have no observer. pub detached_observer_count: 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/stats.rs` around lines 29 - 34, Update the rustdoc for MandatoryTaskStats fields error_count and detached_observer_count to state explicitly that they remain zero for the transaction_cleanup task class because cleanup tasks return no result and have no observer; preserve the existing descriptions for operation statistics and leave panic_count 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/index.rs`:
- Around line 2814-2897: Update
test_terminal_cleanup_progresses_during_accepted_index_ddl_on_one_runner so the
rollback future is pinned and polled until transaction cleanup submission is
observed, then release the CreateHotBuildBatchComplete gate before awaiting
rollback and CREATE INDEX completion. Add a bounded timeout or equivalent
completion signal around this scheduling sequence to prevent regressions from
hanging the test suite, while preserving the existing cleanup and operation
count assertions.
In `@doradb-storage/src/engine.rs`:
- Around line 456-458: Update each ShutdownBusy diagnostic construction in the
lifecycle shutdown paths to include origin=explicit in the attached context
string, and update the four corresponding expected assertion strings to match.
Preserve the existing diagnostic fields and ordering otherwise.
- Around line 445-455: Update the busy calculation in the shutdown-finish
logging path to include mandatory_callers and mandatory_internal, matching every
blocker checked by the shutdown condition. Ensure busy is non-zero whenever
strong_refs, operation_blocked, mandatory_callers, or mandatory_internal
prevents shutdown, while preserving the existing log fields and behavior.
In `@doradb-storage/src/runtime/mandatory.rs`:
- Around line 1001-1008: Move
self.transaction_cleanup_counters.record_submitted(0) in the submit path before
executor.spawn(...).detach(), ensuring the submission count is recorded before
supervise_internal can start or complete and preserving the existing counter
value and task behavior.
- Around line 1409-1429: Replace the blocking self.barrier.wait() in
OverlapAccepted::execute with a shared async rendezvous that does not block
executor threads, using registration state such as AtomicUsize plus an awaitable
notification triggered once both tasks arrive. Update the surrounding
OverlapAccepted test setup and shared state so execute awaits completion of both
registrations, preserving the concurrency assertion without an indefinite
deadlock.
In `@doradb-storage/src/session.rs`:
- Around line 5735-5745: Before reading the terminal stats snapshot for
mandatory1, await engine.inner().mandatory_runtime.drain_callers() to ensure
completion accounting and permit cleanup have finished. Keep the existing
assertions unchanged after the drain, rather than relaxing their expected
counts.
---
Nitpick comments:
In `@doradb-bench/src/output.rs`:
- Around line 602-606: Update the metric emission around push_metric for
after.active_count to clearly identify it as an absolute after/current value
rather than a before/after delta. Rename the exported metric to a distinguishing
name such as active_count_current, and add a code comment documenting that this
metric is absolute.
- Around line 563-622: Refactor push_mandatory_task_metrics to emit the nine
delta-based metrics through a table of suffix/accessor pairs, iterating over
that table to call push_metric while preserving the existing names and order.
Keep active_count as the sole explicit push_metric call using
after.active_count.
In `@doradb-storage/src/runtime/mandatory.rs`:
- Around line 1583-1598: Extract the repeated RegistryBuilder setup into an
async build_test_runtime helper accepting worker_threads and concurrency_limit,
returning the Registry and QuiescentGuard<MandatoryRuntime>. Replace the
duplicated construction blocks in all six affected tests, including the tests
around ordinary_error_and_observer_detach_are_counted_by_outcome and the
additional referenced range, while preserving each test’s configuration values
and behavior.
- Around line 1132-1140: Update elapsed_nanos and duration_nanos to return u64,
converting Duration::as_nanos() to u64 so admission_wait_nanos,
queue_wait_nanos, and execution_nanos retain their existing field type without
usize truncation.
In `@doradb-storage/src/stats.rs`:
- Around line 29-34: Update the rustdoc for MandatoryTaskStats fields
error_count and detached_observer_count to state explicitly that they remain
zero for the transaction_cleanup task class because cleanup tasks return no
result and have no observer; preserve the existing descriptions for operation
statistics and leave panic_count unchanged.
🪄 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: 235e2eb9-70ce-444d-bac9-0ed2785cbcfd
⛔ Files ignored due to path filters (7)
docs/benchmark-tool.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/000252-mandatory-runtime-lifecycle-fairness-evolution-readiness.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 none
📒 Files selected for processing (12)
doradb-bench/src/output.rsdoradb-storage/src/catalog/index.rsdoradb-storage/src/conf/engine.rsdoradb-storage/src/engine.rsdoradb-storage/src/lib.rsdoradb-storage/src/runtime/mandatory.rsdoradb-storage/src/runtime/mod.rsdoradb-storage/src/session.rsdoradb-storage/src/stats.rsdoradb-storage/src/trx/mod.rsdoradb-storage/src/trx/undo/index.rsdoradb-storage/src/trx/undo/row.rs
1 participant
Closes #931
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Diagnostics