Add OnCompactionPreCommit listener callback

[pdillinger]

Summary:
Adds a new EventListener::OnCompactionPreCommit callback that fires after a compaction job's output files are written but before the manifest write commits the new Version. At that point input files still have FileMetaData::being_compacted == true, so listeners that maintain bookkeeping of "files currently being compacted" can clean up that state without racing the compaction picker.

A check implemented by a Meta-internal RocksDB user crashes when the same file appears as input to two concurrent compactions. Tracking that set in OnCompactionBegin / OnCompactionCompleted produces false positives because Compaction::ReleaseCompactionFiles() flips being_compacted back to false before OnCompactionCompleted fires, so another thread can pick the same file and trigger OnCompactionBegin before the previous compaction's Completed callback runs. Doing the cleanup in OnCompactionPreCommit closes that race. Default implementation is a no-op, so no API break.

A trivial refactoring to split PerformTrivialMove ensures data is populated for the new callback, while calling back before the trivial move compaction is committed.

Bonus: CLAUDE.md update for when to call make clean as I've recently had it get thoroughly confused TWICE mixing build modes.

Task: T269479969

Test Plan:

• New unit test in db/listener_test.cc checks that OnCompactionPreCommit fires strictly between OnCompactionBegin and OnCompactionCompleted for the same compaction, and that input files still have being_compacted == true at the time it fires.
• Crash test: adds a concurrent-compaction sanity check to db_stress's listener resembling the Meta-internal intended usage: tracks input file numbers from OnCompactionBegin to OnCompactionPreCommit, aborting if the same file appears as input to two concurrent compactions. Also checks other ordering constraints and checks for "leaks" from possible failure to call OnCompactionPreCommit(). Exercises all compaction styles and trivial-move/FIFO paths under load.

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105065326.

[github-actions]

⚠️ clang-tidy: 1 warning(s) on changed lines

Completed in 836.6s.

Summary by check

Check	Count
concurrency-mt-unsafe	1
Total	1

Details

db_stress_tool/db_stress_test_base.cc (1 warning(s))

db_stress_tool/db_stress_test_base.cc:3973:9: warning: function is not thread safe [concurrency-mt-unsafe]

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105065326.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 33b66e3

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 33b66e3

---

Summary

Well-designed addition of OnCompactionPreCommit listener callback that correctly solves a real race condition in compaction file tracking. The implementation follows existing patterns, covers all compaction paths, and includes good test coverage. No high-severity issues found.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🔴 HIGH

None.

🟡 MEDIUM

M1. Java bindings not updated — java/src/main/java/org/rocksdb/AbstractEventListener.java

• Issue: RocksDB's Java bindings expose onCompactionBegin and onCompactionCompleted via JNI (AbstractEventListener.java, event_listener_jnicallback.cc). The new OnCompactionPreCommit callback is not wired through Java. Java users who track compaction files will not benefit from this fix.
• Root cause: The PR only adds the C++ API without corresponding Java/JNI plumbing.
• Suggested fix: Add onCompactionPreCommit to AbstractEventListener.java and wire it through event_listener_jnicallback.cc and portal.h. Alternatively, document this as a follow-up task. Note that the recently added OnBackgroundJobPressureChanged also lacks Java bindings, so this may be an accepted pattern for new callbacks.

M2. Unit test only exercises level compaction — db/listener_test.cc:280

• Issue: OnCompactionPreCommitOrdering test only triggers level-style compaction (L0 → L1). The PR adds PreCommit calls to 4 distinct compaction paths (FIFO deletion, trivial copy, trivial move, normal), but only the normal path is tested. The stress test (db_stress_listener.h) covers more paths under load but does not deterministically verify ordering for each path type.
• Root cause: Test coverage gap for non-default compaction paths.
• Suggested fix: Add dedicated unit tests (or parameterized variants) for trivial move and FIFO deletion paths to verify PreCommit ordering in each case.

M3. Test listener assumes sequential compactions — db/listener_test.cc:207-260

• Issue: TestCompactionPreCommitListener uses single counters (begin_count_, pre_commit_count_, completed_count_) and asserts begin_count_ == pre_commit_count_ + 1 etc. This works only when compactions run sequentially. If two compactions overlap (e.g., L0→L1 and L1→L2), the assertions could fail spuriously.
• Root cause: The test uses level0_file_num_compaction_trigger = 4 and sync points to serialize compactions, which makes this safe in practice. However, the design is fragile — removing the sync points would break it.
• Suggested fix: Consider using per-job-id tracking (e.g., std::unordered_map<int, State>) for a more robust design, or document that the test relies on serialized compactions.

🟢 LOW / NIT

L1. Minor style inconsistency in empty-listener check — db/db_impl/db_impl_compaction_flush.cc:2015

• Issue: NotifyOnCompactionPreCommit uses immutable_db_options_.listeners.size() == 0U while NotifyOnCompactionBegin uses immutable_db_options_.listeners.empty(). Both are functionally identical but stylistically inconsistent.
• Suggested fix: Use listeners.empty() for consistency with Begin, or leave as-is since it matches Completed's style.

L2. CLAUDE.md change is out of scope — CLAUDE.md

• Issue: The CLAUDE.md update about make clean and build modes is unrelated to the OnCompactionPreCommit feature. While useful, bundling unrelated documentation changes makes the commit harder to revert cleanly.
• Suggested fix: Could be a separate commit, but this is a minor concern.

Cross-Component Analysis

Context	Code executes?	Assumptions hold?	Action needed?
BackgroundCompaction (all 5 paths)	YES	YES	All covered by PR
CompactFilesImpl	NO	N/A	Consistent — no Begin/Completed either
Remote compaction	YES (normal path)	YES	Goes through BackgroundCompaction
ReadOnly DB	NO	N/A	No compactions
WritePreparedTxnDB	YES	YES	No transaction state interaction
FIFO compaction	YES	YES	Dedicated paths covered
Trivial move	YES	YES	Covered, fires before PerformTrivialMove
Concurrent compactions	YES	YES	being_compacted prevents file reuse during callback

Ordering guarantee verification:

• Begin sets notify_on_compaction_completion_ = true → PreCommit checks this → Completed checks this. Chain is correct.
• PreCommit is called before every ReleaseCompactionFiles (via explicit call sites or safety net). Verified at all 5 paths.
• Safety net at end of BackgroundCompaction (when !compaction_released) ensures PreCommit fires even on error paths after Begin.

Idempotency verification:

• notify_on_compaction_pre_commit_called_ is set on first call, checked on all subsequent calls. Plain bool is safe because Compaction objects are single-owner (one background thread) and all accesses are under mutex_.
• Follows identical pattern to existing notify_on_compaction_completion_ (also a plain bool).

Thread safety verification:

• Mutex is released during callback (mutex_.Unlock()), but being_compacted remains true on input files, preventing any other compaction from picking them. Re-acquired before returning.
• Pattern is identical to existing NotifyOnCompactionBegin and NotifyOnCompactionCompleted.

Positive Observations

• Correct problem identification: The race between ReleaseCompactionFiles and OnCompactionCompleted is a genuine issue. PreCommit provides a clean solution.
• Defensive design: The safety-net fallback ensures PreCommit fires even on unusual error paths, preventing silent missed callbacks.
• Consistent patterns: The implementation closely mirrors existing NotifyOnCompactionBegin/NotifyOnCompactionCompleted, making it easy to review and maintain.
• Good stress test integration: The db_stress_listener.h changes exercise the exact race condition the PR fixes, providing ongoing CI coverage.
• Clean API: Default no-op virtual method preserves full backward compatibility for existing EventListener subclasses.
• Well-written documentation: The listener.h doc comments clearly explain when the callback fires, what state is observable, and how it relates to Begin/Completed.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105065326.

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105065326.

[github-actions]

Codex Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit e35a258

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[github-actions]

Claude Code Review - OBSOLETE

Superseded by a newer AI review. Expand to see the original review.

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit e35a258

---

Summary

Well-designed feature that solves a real race condition in compaction listener bookkeeping. The implementation is thorough, covering all compaction paths with a sound idempotency mechanism and safety-net pattern. The refactoring of PerformTrivialMove is clean and purposeful.

High-severity findings (1):

• [db_stress_listener.h:compacting_files_] Shutdown race: OnCompactionBegin can fire but OnCompactionPreCommit may be skipped due to shutting_down_ guard, leaving files permanently in compacting_files_ and causing false-positive abort on next compaction of those files.

Full review (click to expand)

Findings

🔴 HIGH

H1. Shutdown race can cause stress test listener state leak — db_stress_tool/db_stress_listener.h

• Issue: The stress test listener adds files to compacting_files_ in OnCompactionBegin and removes them in OnCompactionPreCommit. Both NotifyOnCompactionPreCommit and NotifyOnCompactionCompleted check shutting_down_ and return early. If shutdown is initiated after OnCompactionBegin fires but before OnCompactionPreCommit fires, files remain in compacting_files_ and precommitted_jobs_ is never populated. While the Compaction object and listener are destroyed at shutdown so this is largely benign in the in-process case, the abort-on-duplicate-detection logic means any race with shutdown + re-pick of the same file could trigger a false std::abort().
• Root cause: Asymmetric lifecycle: OnCompactionBegin has a manual_compaction_paused_ guard that can prevent it from firing, but once it fires, no mechanism guarantees OnCompactionPreCommit will also fire if shutting_down_ is set between the two.
• Suggested fix: In the stress listener, handle the case where files in compacting_files_ are "orphaned" — e.g., clear the tracking set on shutdown, or add a destructor check that compacting_files_ is empty (with a warning rather than abort). Alternatively, consider not checking shutting_down_ in NotifyOnCompactionPreCommit, mirroring how the existing NotifyOnCompactionCompleted already runs even during shutdown as long as ShouldNotifyOnCompactionCompleted() is true. Note: in practice, BackgroundCompaction waits for compactions to finish before shutdown completes, so the window is narrow, but shutting_down_ can be set concurrently.

🟡 MEDIUM

M1. Safety-net fires PreCommit for never-committed compactions — db_impl_compaction_flush.cc:4747

• Issue: The safety-net NotifyOnCompactionPreCommit call at the end of BackgroundCompaction fires even when the compaction failed (non-OK status) and no manifest write ever occurred. The documentation says "before the manifest write commits the new Version," but the safety-net fires when there's no manifest write at all.
• Root cause: The safety-net is inside if (!compaction_released), meaning it fires for error paths where LogAndApply was never called.
• Suggested fix: This is actually acceptable behavior for the intended use case (cleaning up compacting_files_ bookkeeping before ReleaseCompactionFiles), and the docs already note ci.status doesn't reflect the final outcome. However, consider documenting explicitly that the callback may fire even when the compaction is not committed (e.g., on error paths), so listener implementers know to check ci.status.

M2. num_l0_files uses cfd->current() instead of c->input_version() — db_impl_compaction_flush.cc:2042

• Issue: NotifyOnCompactionPreCommit uses cfd->current()->storage_info()->NumLevelFiles(0) (same as OnCompactionCompleted), while OnCompactionBegin uses c->input_version()->storage_info()->NumLevelFiles(0). Since PreCommit fires before manifest commit, semantically it should be closer to Begin (using the input version).
• Root cause: Copy-paste from NotifyOnCompactionCompleted.
• Suggested fix: Consider using c->input_version()->storage_info()->NumLevelFiles(0) for consistency with the fact that PreCommit fires before the version change is applied. However, this is a minor informational field and the current approach matches OnCompactionCompleted, which is also acceptable if documented.

M3. Double BuildCompactionJobInfo cost per compaction — db_impl_compaction_flush.cc

• Issue: BuildCompactionJobInfo is now called twice per compaction lifecycle (once for PreCommit, once for Completed). This function constructs strings, vectors, and maps (file paths, table properties).
• Root cause: The CompactionJobInfo is built from scratch each time rather than cached.
• Suggested fix: For the common case where listeners are registered, consider caching the CompactionJobInfo from PreCommit and reusing it for Completed (only updating status and num_l0_files). However, compaction is not a hot path (each runs for seconds/minutes), so this is a minor efficiency concern.

🟢 LOW / NIT

L1. CompactFilesImpl trivial move path doesn't fire PreCommit — db_impl_compaction_flush.cc:1743

• Issue: CompactFilesImpl calls PrepareTrivialMoveEdit + CommitTrivialMove without inserting NotifyOnCompactionPreCommit between them. However, CompactFilesImpl also doesn't call NotifyOnCompactionBegin or NotifyOnCompactionCompleted, so this is consistent behavior.
• Suggested fix: None needed — the ShouldNotifyOnCompactionCompleted() guard would make it a no-op anyway. Consider adding a code comment noting the deliberate omission.

L2. ABI impact of new virtual method — include/rocksdb/listener.h

• Issue: Adding OnCompactionPreCommit as a new virtual method to EventListener extends the vtable. This is technically an ABI break for existing compiled code with EventListener subclasses.
• Suggested fix: This follows the existing RocksDB pattern (virtual methods are added in minor releases). The default no-op implementation ensures source compatibility. No action needed if this is consistent with RocksDB's ABI policy.

L3. Test only covers level compaction — db/listener_test.cc

• Issue: OnCompactionPreCommitOrdering test uses kCompactionStyleLevel only. It doesn't test trivial move, FIFO deletion, or FIFO copy paths.
• Suggested fix: The stress test (db_stress_listener.h) covers all compaction styles under load, which provides good coverage. Consider adding a trivial-move-specific unit test for completeness.

L4. Naming nit — include/rocksdb/listener.h

• Issue: OnCompactionPreCommit — the "Pre" prefix is uncommon in the existing API (Begin/Completed pattern). An alternative like OnCompactionOutputReady might be more descriptive of the timing semantics.
• Suggested fix: The current name is clear and follows database terminology ("pre-commit"). No strong objection.

Cross-Component Analysis

Context	Code executes?	Assumptions hold?	Action needed?
WritePreparedTxnDB	YES (compactions run)	YES (callback is about compaction lifecycle, not transactions)	Safe
ReadOnly DB	NO (no compactions)	N/A	Safe
CompactionService / Remote	YES (Install runs locally)	YES (PreCommit fires before local LogAndApply)	Safe
FIFO compaction	YES (deletion + copy paths)	YES (both paths instrumented)	Safe
User-defined timestamps	YES	YES (no timestamp-specific logic in callback)	Safe
Concurrent writers	YES	YES (mutex held during flag checks)	Safe
Old snapshots	YES	YES (no snapshot interaction)	Safe

Positive Observations

• Thorough coverage: All 4 compaction paths in BackgroundCompaction are instrumented, plus a safety-net.
• Sound idempotency: The per-Compaction boolean flag with mutex-protected access prevents double-firing cleanly.
• Good refactoring: Splitting PerformTrivialMove into PrepareTrivialMoveEdit + CommitTrivialMove is a clean separation of concerns that enables the notification insertion point.
• Excellent stress test: The db_stress_listener.h changes provide real-world validation of the race condition fix (tracking files between Begin and PreCommit).
• Comprehensive documentation: The listener.h doc comments clearly explain timing semantics, caveats for trivial moves/FIFO, and threading model.
• Defensive safety-net: The idempotent call before ReleaseCompactionFiles ensures listeners always get notified, even on unusual error paths.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D105065326.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 74b403a

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not update PATH: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

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

Commands:

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

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 74b403a

---

Summary

Well-designed PR that adds a new EventListener::OnCompactionPreCommit callback to close a real race window in compaction file tracking. The implementation is clean, follows existing patterns, and includes good test coverage via both unit tests and stress tests. The PerformTrivialMove split is well-motivated. No high-severity issues found.

High-severity findings (0):

No high-severity findings.

Full review (click to expand)

Findings

🟡 MEDIUM

M1. Missing shutting_down_ check — db_impl_compaction_flush.cc:2015

• Issue: NotifyOnCompactionPreCommit does not check shutting_down_ before proceeding, while both NotifyOnCompactionBegin (line 1955) and NotifyOnCompactionCompleted (line 1988) do.
• Root cause: Oversight in mirroring the guard conditions from the sibling notification functions.
• Suggested fix: Add if (shutting_down_.load(std::memory_order_acquire)) { return; } after the mutex_.AssertHeld() call, consistent with NotifyOnCompactionCompleted. Without this, listeners could receive a PreCommit during shutdown but not the subsequent Completed, leaving bookkeeping in an inconsistent state.

M2. Java/JNI bindings not updated for OnCompactionPreCommit

• Issue: EventListenerJniCallback in java/rocksjni/event_listener_jnicallback.h implements the EventListener interface for Java consumers. The new OnCompactionPreCommit virtual method is not wired up in the JNI layer, so Java listeners cannot receive this callback.
• Root cause: JNI bindings are a separate layer that must be manually updated.
• Suggested fix: Add JNI callback wiring for OnCompactionPreCommit, or document this as a known gap to be addressed in a follow-up PR.

🟢 LOW / NIT

L1. num_l0_files sourced from cfd->current() instead of c->input_version() — db_impl_compaction_flush.cc:2037

• Issue: NotifyOnCompactionPreCommit uses cfd->current()->storage_info()->NumLevelFiles(0) while NotifyOnCompactionBegin uses c->input_version()->storage_info()->NumLevelFiles(0). This is inconsistent with Begin but consistent with Completed.
• Root cause: Intentional — PreCommit fires closer to Completed in the lifecycle, so using the current version (which may have advanced since compaction start) is appropriate. Nit: a brief comment explaining the choice would prevent future confusion.

L2. ABI vtable change from adding virtual method to EventListener

• Issue: Adding OnCompactionPreCommit as a new virtual method changes the vtable layout of EventListener, which is an ABI break for downstream consumers that link against a pre-built RocksDB shared library.
• Root cause: Inherent to adding virtual methods in C++.
• Suggested fix: No fix needed — RocksDB maintains source-level compatibility, not ABI stability. All prior callback additions (OnCompactionBegin, OnSubcompactionBegin/Completed, OnBackgroundJobPressureChanged, etc.) follow the same pattern. This is consistent with project conventions.

L3. Unit test covers only level compaction path

• Issue: OnCompactionPreCommitOrdering test uses kCompactionStyleLevel. FIFO deletion, trivial copy, and universal compaction paths are only exercised by the stress test, not by deterministic unit tests.
• Root cause: Acceptable trade-off — the stress test in db_stress_listener.h provides broad coverage with concurrent compaction tracking, file set validation, and ordering assertions across all compaction styles.

L4. Test listener's ordering check assumes sequential compactions

• Issue: TestCompactionPreCommitListener uses simple counters (begin_count_, pre_commit_count_, completed_count_) and assumes sequential compaction ordering (EXPECT_EQ(begin_count_, pre_commit_count_ + 1)). With concurrent compactions, these assertions would fail.
• Root cause: The test uses sync points (LoadDependency) to force sequential execution, so the assumption is valid within the test. However, this means the unit test doesn't verify correctness under concurrency — that's left to the stress test.

Cross-Component Analysis

Context	Covered by PR?	Assessment
FIFO deletion compaction	YES (call site at line 4297)	Correct
FIFO trivial copy compaction	YES (call site at line 4517)	Correct
Trivial move (BackgroundCompaction)	YES (call site at line 4597)	Correct — split into Prepare+Commit enables callback
Normal compaction (CompactionJob)	YES (call site at line 4723)	Correct — fires before Install()
Safety net (error paths)	YES (call site at line 4749)	Correct — idempotent backstop
CompactFilesImpl	NO	Consistent — CompactFilesImpl also skips Begin/Completed
ReadOnly DB	N/A	No compactions in ReadOnly mode
Remote CompactionService	Via normal path	Works correctly
WritePreparedTxnDB	Via normal path	Works correctly

Positive Observations

• Idempotency design is robust: The notify_on_compaction_pre_commit_called_ flag + ShouldNotifyOnCompactionCompleted() guard ensures exactly-once semantics without complex control flow. Simple and effective.
• PerformTrivialMove split is clean: Separating edit preparation from commit is a sensible refactor that enables the callback without changing semantics. Both halves retain mutex_.AssertHeld() documentation.
• Stress test is thorough: The DbStressListener additions (file tracking, ordering verification, deletion-while-compacting check) exercise the callback under realistic concurrent load with all compaction styles.
• Documentation quality is good: The listener.h docstrings clearly explain timing, state guarantees, ci.status semantics, and the relationship to Begin/Completed. The OnCompactionCompleted docs are also updated with complementary information.
• Safety net shows defensive thinking: The backstop call before ReleaseCompactionFiles ensures listeners always get the callback even in unusual error paths, without requiring perfect coverage of every early-exit branch.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.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

[meta-codesync]

@pdillinger merged this pull request in 805a476.