Skip to content

fix(db): remove orphaned segment directories during crash recovery - #674

Open
YongqiYin wants to merge 2 commits into
alibaba:mainfrom
YongqiYin:fix/recovery-orphan-segment-dirs
Open

fix(db): remove orphaned segment directories during crash recovery#674
YongqiYin wants to merge 2 commits into
alibaba:mainfrom
YongqiYin:fix/recovery-orphan-segment-dirs

Conversation

@YongqiYin

Copy link
Copy Markdown
Collaborator

Fixes #673.

Problem

Since #614, Optimize renames its compacted output to the final numeric
directory and opens it in the lock-free phase, before the commit phase
persists next_segment_id. If the process crashes inside that window,
recovery restores the id allocator from the old manifest and leaves the
unreferenced directory on disk:

  • the directory leaks permanently;
  • the first operation that re-allocates that id fails spuriously
    (segment path already exists on a writing-segment switch from
    Insert/DDL, ENOTEMPTY on the next Optimize's rename).

Fix

recovery() now removes on-disk leftovers right after the manifest is
loaded, before opening segments:

  • numeric segment directories absent from the recovered manifest
    (persisted set plus the writing segment);
  • <id>.tmp compact outputs that were never renamed.

Safety:

  • the exclusive collection file lock is already held, so no concurrent
    Optimize can be producing these directories;
  • read-only opens hold only a shared lock and skip the cleanup;
  • only names that round-trip through segment-id formatting (all digits,
    no leading zeros, fits uint32) are considered; anything else is left
    untouched;
  • best-effort: a directory that cannot be removed is logged and skipped.

The Optimize three-phase locking model from #614 is unchanged.

Test

New regression test Feature_Recovery_Orphan_Segment_Dirs_Removed:

  • reads the manifest to learn the exact next segment id, then fakes the
    crash leftovers (colliding dir, far orphan dir, *.tmp residue) plus
    non-canonical guard dirs (007, .tmp, 5.tmpx) that must survive;
  • a read-only open removes nothing; a read-write open removes exactly
    the orphans and keeps every referenced segment directory;
  • post-recovery Optimize() and an add-column DDL both succeed with all
    docs intact.

Fails on main at both the directory assertions and the post-reopen
Optimize(); passes with the fix. Full collection_test suite: 86/86.

Note: this makes a read-write Collection::Open delete directories under
the collection path that match canonical segment naming but are not
referenced by the manifest — a deliberate, logged self-heal for crash
leftovers.

A crash between Optimize's lock-free directory rename and the
commit-phase manifest flush leaves an on-disk segment directory that
the recovered manifest never references: the directory leaks
permanently, and the first operation that re-allocates that segment id
fails spuriously ("segment path already exists" on writing-segment
switch, ENOTEMPTY on the next Optimize's rename).

Recovery now removes numeric segment directories not referenced by the
recovered manifest (persisted segments plus the writing segment), as
well as leftover *.tmp compact outputs. The exclusive collection file
lock is already held at this point, so no concurrent Optimize can be
producing these directories. Read-only opens skip the cleanup.

Fixes alibaba#673
- collect candidate directory names first and remove them after the
  scan: deleting an entry while iterating a directory is
  implementation-defined, and the range-for increment could still
  throw filesystem_error, breaking the best-effort contract
- log a warning when listing the collection directory fails instead
  of failing silently
- guard against a missing writing segment meta
- extend the regression test with non-canonical guard directories
  (".tmp", "5.tmpx") and an add-column DDL after recovery, covering
  the DDL failure mode listed in the issue
@YongqiYin
YongqiYin requested a review from zhourrr as a code owner August 12, 2026 07:47
Copilot AI lite review requested due to automatic review settings August 12, 2026 07:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Crash during Optimize can leak an orphaned segment dir and fail the next segment allocation after recovery

2 participants