fix: implement component shutdown panic handling - #943
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
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:
📝 WalkthroughWalkthroughComponent shutdown now contains hook and worker panics, completes reverse-order cleanup, tracks degraded owners and guards, and propagates or suppresses the first panic after teardown. Engine shutdown preserves lifecycle completion and root-release ordering. ChangesShutdown panic containment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Engine
participant ComponentRegistry
participant WorkerComponents
participant StorageRootLease
Engine->>ComponentRegistry: start reverse-order shutdown
ComponentRegistry->>WorkerComponents: invoke shutdown hooks
WorkerComponents-->>ComponentRegistry: complete cleanup and return panic outcome
ComponentRegistry-->>Engine: return ComponentShutdownOutcome
Engine->>StorageRootLease: release root lease
Engine-->>Engine: propagate or suppress first shutdown panic
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 | 1 medium |
🟢 Metrics 77 complexity · 26 duplication
Metric Results Complexity 77 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 makes component shutdown panic-contained while preserving reverse teardown order and terminal engine lifecycle semantics.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains in the eligible follow-up scope.
|
| Filename | Overview |
|---|---|
| doradb-storage/src/component.rs | Adds per-hook panic containment, aggregate outcomes, once-only dispatch, and guard-aware degraded owner release without an eligible unresolved follow-up finding. |
| doradb-storage/src/engine.rs | Publishes terminal lifecycle state and releases the shutdown mutex before applying the aggregate panic policy. |
| doradb-storage/src/runtime/mandatory.rs | Preserves live runners when callers remain and otherwise joins all runners before exposing the first terminal failure. |
| doradb-storage/src/trx/sys.rs | Ensures purge and redo shutdown complete all joins and required resource release before panic propagation. |
| doradb-storage/src/quiescent.rs | Adds acquire-ordered guard-count observation for terminal degraded owner-release decisions. |
| docs/engine-component-lifetime.md | Documents the panic-containment boundary, shutdown order, degraded release policy, and unsupported arbitrary worker-body unwinds. |
Reviews (3): Last reviewed commit: "resolve task" | Re-trigger Greptile
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #943 +/- ##
==========================================
+ Coverage 93.42% 93.46% +0.03%
==========================================
Files 149 149
Lines 127741 128332 +591
==========================================
+ Hits 119347 119948 +601
+ Misses 8394 8384 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/runtime/mandatory.rs`:
- Around line 930-945: Update the mandatory runner shutdown flow around
FirstPanic and signal_stop so it does not signal or join runners while admission
callers remain. When callers != 0, fail immediately after recording the existing
panic; only drain internal admission, call signal_stop, and proceed to
handle.join once caller admission is confirmed drained.
🪄 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: a041f684-660e-4b11-9bc0-8d6c81e46918
⛔ Files ignored due to path filters (3)
docs/engine-component-lifetime.mdis excluded by none and included by nonedocs/tasks/000256-component-shutdown-panic-containment.mdis excluded by none and included by nonedocs/tasks/next-idis excluded by none and included by none
📒 Files selected for processing (13)
doradb-storage/src/buffer/evictor.rsdoradb-storage/src/buffer/mod.rsdoradb-storage/src/catalog/mod.rsdoradb-storage/src/component.rsdoradb-storage/src/engine.rsdoradb-storage/src/file/fs.rsdoradb-storage/src/lock/mod.rsdoradb-storage/src/poison.rsdoradb-storage/src/quiescent.rsdoradb-storage/src/root.rsdoradb-storage/src/runtime/mandatory.rsdoradb-storage/src/trx/purge.rsdoradb-storage/src/trx/sys.rs
Closes #942
Summary by CodeRabbit
Bug Fixes
Diagnostics
Tests