fix(storage): reclaim warm-tier staging directories instead of leaking them (#435) - #436
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughWarm-tier transitions now clean staging directories after failures, remove stale same-ID staging data before retries, and disarm cleanup after successful renames. Recovery removes orphaned staging directories during startup. ChangesWarm-tier staging cleanup
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant transition_to_warm
participant StagingGuard
participant filesystem
participant recovery
transition_to_warm->>filesystem: Remove stale same-ID staging directory
transition_to_warm->>StagingGuard: Arm cleanup guard
transition_to_warm->>filesystem: Rename staging directory to final segment
transition_to_warm->>StagingGuard: Disarm cleanup
recovery->>filesystem: Sweep orphaned staging directories
Possibly related PRs
Suggested reviewers: 🚥 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 |
PR Summary by QodoFix warm-tier staging directory leaks with Drop guard and startup sweep
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![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
🤖 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 `@src/storage/tiered/warm_tier.rs`:
- Around line 196-199: Make the transition around manifest.commit,
wal.flush_sync, and std::fs::rename transactional so a rename failure cannot
leave an Active manifest/FileCreate record after StagingGuard removes the staged
data. Restore the manifest and FileCreate state on failure, or adjust recovery
to mark incomplete transitions non-Active while preserving successful renames.
Extend failed_transition_leaves_no_staging_dir to assert the recovered manifest
contains no active entry for the failed segment.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a90695c3-bf3d-4a43-bd6e-44f2a9675ec9
📒 Files selected for processing (3)
CHANGELOG.mdsrc/persistence/recovery.rssrc/storage/tiered/warm_tier.rs
| // Step 6: Rename staging -> final. The directory now lives under its final | ||
| // name, so the guard must not remove it. | ||
| std::fs::rename(&staging, &final_dir)?; | ||
| staging_guard.disarm(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Repair committed metadata when the rename fails.
manifest.commit() at Line 194 and wal.flush_sync() at Line 190 complete before rename() at Line 198. If the rename fails, StagingGuard removes the staged data, but the manifest and WAL still describe an active warm file.
During recovery, src/persistence/recovery.rs Lines 275-293 skip that active entry because segment-{id}/codes.mpf is absent. The failed transition therefore leaves durable metadata that references no recoverable segment.
Add a failure path that restores the manifest and FileCreate state, or redesign the transition and recovery protocol so an incomplete transition cannot remain Active. Extend failed_transition_leaves_no_staging_dir to verify the recovered manifest state.
🤖 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 `@src/storage/tiered/warm_tier.rs` around lines 196 - 199, Make the transition
around manifest.commit, wal.flush_sync, and std::fs::rename transactional so a
rename failure cannot leave an Active manifest/FileCreate record after
StagingGuard removes the staged data. Restore the manifest and FileCreate state
on failure, or adjust recovery to mark incomplete transitions non-Active while
preserving successful renames. Extend failed_transition_leaves_no_staging_dir to
assert the recovered manifest contains no active entry for the failed segment.
Code Review by Qodo
1. Sweep gated by manifest
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // and to any `vectors/*` glob because of the dot prefix. The | ||
| // in-process guard covers new failures; this covers orphans from a | ||
| // kill -9 or an older build. | ||
| crate::storage::tiered::warm_tier::sweep_orphan_staging(&vectors_dir); |
There was a problem hiding this comment.
2. Sweep gated by manifest 🐞 Bug ☼ Reliability
recover_shard_v3_pitr calls sweep_orphan_staging only inside the successful ShardManifest::open branch, so orphan .segment-*.staging directories are not reclaimed when the manifest is missing or unreadable. This can preserve the disk leak in exactly those degraded recovery scenarios even though the sweep is designed to be safe on missing directories.
Agent Prompt
## Issue description
`recover_shard_v3_pitr` currently invokes `sweep_orphan_staging()` only after confirming the manifest exists and opens successfully. If the manifest is missing/corrupt/unreadable, recovery skips the sweep and `.segment-*.staging` directories can remain indefinitely.
## Issue Context
`sweep_orphan_staging()` is explicitly written to tolerate missing directories by returning `0` when `read_dir(vectors_dir)` fails, so it can be called safely even when there is no manifest to scan.
## Fix Focus Areas
- src/persistence/recovery.rs[263-275]
- src/storage/tiered/warm_tier.rs[65-68]
## Suggested change
Move (or duplicate) the call to `sweep_orphan_staging(&vectors_dir)` so it runs before attempting to open the manifest (or runs even if `ShardManifest::open()` returns `Err`). Keep the manifest-scanning logic conditional on a successful open, but decouple cleanup from manifest readability.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
6334e5b to
abca63c
Compare
…g them (#435) Found on a live instance: a 27 GB data directory against 2.53 GB of used_memory. 20 GB of it was 14,499 orphaned `.segment-*.staging` directories, versus 174 MB in the 94 real segments — 99% of the vector store was abandoned scratch. All were written in a single three-minute window two days earlier and had already survived a restart. The dot prefix kept them out of `ls` and out of every `vectors/*` glob, so nothing surfaced them. `transition_to_warm` has roughly ten fallible steps between `create_dir_all(&staging)` and the rename that moves the directory to its final name. Every `?` in that stretch leaked the whole directory. Two fixes, because there are two ways to get an orphan: 1. `StagingGuard` — a Drop guard armed at creation and disarmed after the rename, so every early return in that stretch cleans up. A guard rather than cleanup at each `?` precisely because the leak came from the paths nobody remembered to annotate. Cleanup is best-effort so a failure to remove cannot mask the error that caused the unwind. 2. `sweep_orphan_staging` — called from `recover_shard_v3_pitr` before the manifest scan, for orphans no in-process guard can cover: a kill -9 between the manifest commit and the rename, or anything left by a build that predates the guard. The sweep is safe by construction: `.staging` paths are produced in exactly one place and consumed by nothing — every reader and recovery path opens the final `segment-{id}` name — so a staging directory is unreachable the moment it is not mid-write. Real `segment-*` directories do not match the pattern. A dangling manifest entry (commit succeeded, rename failed) was already dangling before this change and recovery already warns and skips it; the guard does not make that case worse. `transition_to_warm` also now removes a stale staging dir for the same id before creating it, so a retry cannot inherit a previous attempt's partial files. The sibling writer in `vector/persistence/segment_io.rs` has always done this; this path did not. The failure test forces the error at the RENAME — where production actually died, since every orphan was fully written and fsynced — via a non-empty destination (guaranteed ENOTEMPTY). It asserts `is_err()` first: "no staging dir remains" is also true of a SUCCESSFUL transition, so without pinning the failure path the cleanup assertion would pass with the guard deleted. Fixes #435 author: Tin Dang
abca63c to
3d8e378
Compare
1 participant
What
Found on a live instance: a 27 GB data directory against 2.53 GB of
used_memory. 20 GB of it was 14,499 orphaned.segment-*.stagingdirectories (99% of the vector store was abandoned scratch) vs 174 MB in the 94 real segments. All were written in a single three-minute window two days earlier and had already survived a restart. The dot prefix kept them out oflsand everyvectors/*glob, so nothing surfaced them.Root cause
transition_to_warmhas ~10 fallible steps betweencreate_dir_all(&staging)and the finalrename. Every?in that stretch leaked the whole directory.Fix (two guards, because there are two ways to orphan)
StagingGuard— aDropguard armed at creation, disarmed right after the rename, so every early return in that stretch cleans up. A guard (rather than cleanup at each?) precisely because the leak came from the paths nobody remembered to annotate. Cleanup is best-effort so it can't mask the error that caused the unwind.sweep_orphan_staging— called fromrecover_shard_v3_pitrbefore the manifest scan, for orphans no in-process guard can cover (kill -9 between manifest commit and rename, or anything left by a pre-guard build).transition_to_warmalso now removes a stale staging dir for the same id before creating it, matching the sibling writer invector/persistence/segment_io.rs.Safety
.stagingpaths are produced in exactly one place and consumed by nothing — every reader opens the finalsegment-{id}name — so a staging dir is unreachable the moment it is not mid-write. Realsegment-*dirs don't match the pattern. A dangling manifest entry (commit succeeded, rename failed) was already dangling before this change and recovery already warns-and-skips it.Tests (red/green)
failed_transition_leaves_no_staging_dir— forces the error at the rename (where production actually died; every orphan was fully written and fsynced) via a non-empty destination (ENOTEMPTY). Assertsis_err()first, so the cleanup assertion can't pass vacuously on a successful transition.startup_sweep_removes_orphan_staging_dirsstartup_sweep_tolerates_missing_dirAll 12
warm_tiertests pass (fmt + tests green on this main-based branch).Fixes #435
Summary by CodeRabbit