[omkarhgawde]

Summary:
CompactionJob::ProcessKeyValueCompaction() calls NotifyOnSubcompactionBegin() before MaybeResumeSubcompactionProgressOnInputIterator(). When the resume failed, the function set sub_compact->status and returned without a call to NotifyOnSubcompactionCompleted(). The resume fails when a saved output file cannot be read, when the key to resume from is outside the input, when the input iterator fails at that key, or when the saved blob progress cannot be restored. A listener then got OnSubcompactionBegin() with no matching OnSubcompactionCompleted(), so it could not close the event or see the failure status.

Only DB::OpenAndCompact() with OpenAndCompactOptions::allow_resumption loads saved progress, so only the listeners of such a worker are affected. When there is no progress to resume, the function returns NotFound, and the subcompaction starts from the first key as before.

The early return now calls NotifyOnSubcompactionCompleted(), and the event carries the resume error in SubcompactionJobInfo::status. NotifyOnSubcompactionCompleted() keeps its checks: it sends nothing when NotifyOnSubcompactionBegin() sent no event, or when the DB is shutting down. The resume error and what follows it do not change: the worker's compaction fails with that status, and the CompactionService implementation decides what the primary does.

The diff also adds a release note under unreleased_history/bug_fixes/.

Differential Revision: D128236290

[meta-codesync]

@omkarhgawde has exported this pull request. If you are a Meta employee, you can view the originating Diff in D128236290.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 600.2s.

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 3ae1d3f

---

Summary

Clean, well-scoped bug fix that correctly pairs NotifyOnSubcompactionCompleted() with NotifyOnSubcompactionBegin() on the early return path when resuming subcompaction progress fails. The fix is safe due to the notify_on_subcompaction_completion guard in NotifyOnSubcompactionCompleted(), and the test validates the key invariant. No resource leaks, no thread-safety issues.

High-severity findings (0):
No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Missing PermitUncheckedError() for input_iter->status() on early return under ASSERT_STATUS_CHECKED — compaction_job.cc:2158

• Issue: On the normal completion path, FinalizeSubcompaction() calls input_iter->status().PermitUncheckedError() (line 2227) when status is non-OK. On the early return path at line 2158, FinalizeSubcompaction is skipped. The input_iter is destroyed when the local iterators struct goes out of scope. If MaybeResumeSubcompactionProgressOnInputIterator returned an error from ReadOutputFilesTableProperties (line 3514) — which occurs AFTER the iterator Seek succeeded at line 3457 and the iterator status was already checked at line 3467 — this is likely fine because status().ok() at line 3467 marks the status as checked. However, for the path where !input_iter->Valid() at line 3459 (the iterator Seek found nothing, and status might be OK but unchecked), the status checking depends on whether Valid() implicitly checks the status in ASSERT_STATUS_CHECKED mode.
• Root cause: The early return path bypasses FinalizeSubcompaction which handles PermitUncheckedError().
• Suggested fix: This is likely benign because MaybeResumeSubcompactionProgressOnInputIterator does check input_iter->status().ok() at line 3467 for the error paths that reach the early return. However, adding a defensive input_iter->status().PermitUncheckedError() before the early return would make the code more robust and consistent with the exception handler path. Alternatively, note this as a pre-existing concern since the !input_iter->Valid() branch at line 3459 is also pre-existing code that doesn't call PermitUncheckedError.

M2. compaction_job_stats in SubcompactionJobInfo will have uninitialized/default values — compaction_job.cc:2159

• Issue: When the early return fires, FinalizeSubcompactionJobStats() is never called, so the compaction_job_stats in the SubcompactionJobInfo delivered to listeners will have default/empty values. Listeners that inspect stats fields like elapsed_micros, num_input_records, etc. will see zeros. This is a minor behavioral concern — the PR description correctly notes the event "carries the resume error in SubcompactionJobInfo::status", but it's worth documenting that stats fields are not meaningful on this path.
• Root cause: No compaction work was done, so stats are naturally empty. This is arguably correct behavior, but listeners should be aware.
• Suggested fix: Consider adding a brief comment at the call site (e.g., "stats are empty because no compaction work was done") or documenting this in the listener API.

🟢 LOW / NIT

L1. Pre-existing: RestoreCompactionOutputs partial state on two-level resume failure — compaction_job.cc:3484-3531

• Issue: The loop for (const bool& is_proximal_level : {false, true}) can succeed on the first iteration (restoring non-proximal outputs) but fail on the second (proximal level property read fails). The restored outputs from the first iteration remain in sub_compact when the function returns an error. On the early return path in ProcessLocalKeyValueCompaction, these orphaned outputs are not cleaned up (no CleanupOutputs() call). This is pre-existing behavior, not introduced by this PR.
• Suggested fix: Out of scope for this PR. Could be addressed separately if deemed important.

L2. Test uses EXPECT_* instead of ASSERT_* in FailedResumeCompactionService::Wait — compaction_service_test.cc:7771-7790

• Issue: Several assertions in the Wait() method use EXPECT_OK and EXPECT_TRUE instead of ASSERT_OK and ASSERT_TRUE. If an expectation fails (e.g., file deletion fails), execution continues with potentially invalid state. This is consistent with other test helpers in this file, so this is a nit.
• Suggested fix: Consider using ASSERT_* for critical preconditions like EXPECT_OK(env->GetChildren(...)) and EXPECT_OK(env->DeleteFile(...)).

L3. Test assumes CompactRange triggers exactly one remote compaction attempt — compaction_service_test.cc:7837

• Issue: The test asserts ASSERT_GT(listener->begin_count, 0) and ASSERT_EQ(listener->completed_count, listener->begin_count) but does not assert an exact count. This is actually good practice for robustness, but it means the test would pass even if the fix didn't work and no failed resume actually occurred (e.g., if the code path was never hit). The ASSERT_GT(listener->corruption_count, 0) assertion mitigates this concern by verifying at least one corruption-status completion event was delivered.
• Suggested fix: No action needed — the corruption_count > 0 assertion provides the necessary coverage.

Cross-Component Analysis

Context	Does code execute?	Assumptions hold?	Action needed?
Standard DB compaction	NO — MaybeResumeSubcompactionProgressOnInputIterator returns NotFound (no saved progress)	N/A	None
DB::OpenAndCompact with allow_resumption	YES — this is the only context	YES	The fix targets this path
WritePreparedTxnDB	NO — OpenAndCompact is a worker path	N/A	None
ReadOnly DB	NO	N/A	None
Shutdown race	SAFE — NotifyOnSubcompactionCompleted checks shutting_down_ (line 1549)	YES	None
Canceled compaction	SAFE — NotifyOnSubcompactionCompleted checks notify_on_subcompaction_completion flag (line 1553)	YES	None
local_compaction_executor_ exception path	CONSISTENT — also calls NotifyOnSubcompactionCompleted after error (line 2109)	YES	None

Positive Observations

• Correct use of notify_on_subcompaction_completion guard: The fix leverages the existing idempotency flag in NotifyOnSubcompactionCompleted() (line 1553), which ensures the notification is only sent when NotifyOnSubcompactionBegin() actually fired. This makes the fix safe even under edge cases (shutting down, no listeners, canceled compaction).
• sub_compact->status is set before notification: Line 2158 sets sub_compact->status = status before the new NotifyOnSubcompactionCompleted call, so BuildSubcompactionJobInfo() will correctly include the error status.
• No resource leaks: At the early return point, all local resources (iterators, boundaries, read options) are in RAII containers that clean up automatically. No output files have been opened.
• Minimal, focused change: The 1-line production code fix is exactly right — no over-engineering.
• Good test coverage: The test creates a realistic scenario (delete saved output files, resume fails with Corruption) and validates the core invariant (begin_count == completed_count, corruption_count > 0).
• Release note is appropriate: Concise, identifies the specific API and conditions.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/ci_review_prompt.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase