fix(gc): file-GC snapshot UAF + MakeCowRoot error ordering in Apply

[thweetkomputer]

Two related durability/lifecycle fixes.

1. File-GC use-after-free (audit #11)

TriggerFileGC / TriggerLocalFileGC build snapshot_array — MappingSnapshot::Refs whose tbl_ident_ points into the partition's RootMeta entry (&entry->tbl_id_) — then release the per-call root Handle that BuildRetainedFiles held. GC then yields across ExecuteLocalGC. During those yields another task can push the RootMeta cache over its limit; EvictIfNeeded picks this now-unpinned (ref_cnt_==0) entry as an LRU victim and erases it, freeing entry->tbl_id_. snapshot_array keeps the MappingSnapshot alive, so at TriggerFileGC exit ~MappingSnapshot → FreeMappingSnapshot dereferences the dangling tbl_ident_:

READ of size 16 ... heap-use-after-free
  #3 eloqstore::operator<<(ostream&, TableIdent const&)  types.cpp:9
  #4 eloqstore::PageManager::FreeMappingSnapshot        page_manager.cpp
  #5 eloqstore::MappingSnapshot::~MappingSnapshot       page_mapper.cpp:485
  ...
  #15 eloqstore::WriteTask::TriggerFileGC()             write_task.cpp   (snapshot_array dtor)
  #16 eloqstore::BackgroundWrite::Compact()             background_write.cpp:160

Fix: hold a root Handle for the entire GC so the entry stays off the LRU (unevictable) until the snapshot arrays are destroyed. On NotFound (entry absent, e.g. after a Drop cleared the manifest) there are no snapshots to dangle — the handle is a no-op and GC still runs with empty retained sets to purge orphaned files and rmdir the partition directory (verified by the existing drop / least_unflushed cleanup tests).

Regression test: gc snapshot refs survive root meta eviction ([gc][local][uaf]). The eviction/GC-yield interleaving is a narrow race, so a debug-only ForceEvictForTest hook + GcForceEvictRoot fault point drive it deterministically — exactly the eviction the LRU would perform under pressure. Aborts with ASAN heap-use-after-free on unfixed code; passes with the fix (the pin makes the forced eviction a no-op). Both hooks compile out in release (NDEBUG).

2. MakeCowRoot error ordering in Apply (audit #14)

BatchWriteTask::Apply dereferenced cow_meta_.compression_ before checking the MakeCowRoot error. MakeCowRoot's error branch (page_manager.cpp return err;) leaves cow_meta_.compression_ null, so an error crashed on the null deref instead of surfacing. Moved CHECK_KV_ERR(err) before the compression use.

Test

• gc, batch_write, persist, segment_compact, standby suites pass (non-ASAN).
• gc [local] suite passes under ASAN, including the new UAF regression test.

Summary by CodeRabbit

• Bug Fixes

  • Improved GC and compaction stability so retained data snapshots stay valid while cleanup runs.
  • Reduced the risk of crashes or use-after-free issues during file garbage collection.

• Tests

  • Added regression coverage for GC scenarios involving root metadata eviction during snapshot handling.

[coderabbitai]

Walkthrough

This PR adds a debug-only RootMetaMgr::ForceEvictForTest method to force-evict an unpinned root meta entry, uses it via a new GcForceEvictRoot fail point to pin RootMeta during GC retained-file snapshot construction in TriggerFileGC/TriggerLocalFileGC, adds a regression test, and reorders an error check in BatchWriteTask::Apply.

Changes

Root-Meta Pinning and Eviction Test Hook

Layer / File(s)	Summary
ForceEvictForTest debug API include/storage/root_meta.h, src/storage/root_meta.cpp	Declares and implements a debug-only RootMetaMgr::ForceEvictForTest method that forcibly evicts an unpinned, LRU-resident root meta entry, updating cache accounting.
RootMeta pinning during GC snapshot build src/tasks/write_task.cpp	TriggerFileGC and TriggerLocalFileGC call FindRoot to pin RootMeta before building retained snapshots, treating NotFound as valid; TriggerFileGC adds a debug-only GcForceEvictRoot fail point invoking ForceEvictForTest.
Regression test for snapshot survival tests/gc.cpp	Adds a test that arms GcForceEvictRoot and runs a compaction to verify GC snapshots survive forced root-meta eviction.
Error-check reordering src/tasks/batch_write_task.cpp	Moves CHECK_KV_ERR(err) to occur right after MakeCowRoot(), before dictionary sampling/building.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant WriteTask
  participant IndexManager
  participant RootMetaMgr
  participant FailPoint

  WriteTask->>IndexManager: FindRoot(tbl_ident_)
  IndexManager-->>WriteTask: RootMeta ref (pinned)
  WriteTask->>FailPoint: check GcForceEvictRoot
  FailPoint->>RootMetaMgr: ForceEvictForTest(tbl_id)
  RootMetaMgr-->>FailPoint: eviction result (blocked while pinned)
  WriteTask->>WriteTask: BuildRetainedFiles(snapshots)

Loading

Possibly related PRs

• eloqdata/eloqstore#293: Both PRs modify WriteTask::TriggerFileGC's snapshot/retained-files handling during the same GC flow.

Suggested reviewers: liunyl

Poem

A rabbit pins the root so tight,
No eviction sneaks in mid-flight,
With fail points armed and tests that check,
Snapshots live without a wreck. 🐇
Hop, hop, hooray — the GC's alright!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly names the two main fixes and stays concise and specific.
Description check	✅ Passed	The description is detailed, explains both fixes, and includes testing notes, though some template reminders are not filled in.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/apply-cow-root-error-order

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

src/storage/root_meta.cpp (1)

456-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the shared eviction tail to keep the test hook faithful to production.

Lines 456-469 duplicate the EvictRootForCache → Dequeue → used_bytes_ adjust → entries_.erase sequence from EvictIfNeeded (lines 419-434). Since the whole point of this hook is to "mimic the LRU victim path", extracting a small EvictVictim(Entry*) helper used by both would prevent the two paths from silently drifting if the production eviction sequence gains a step.

🤖 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/root_meta.cpp` around lines 456 - 469, The test hook in the root
metadata eviction path duplicates the same victim removal sequence already used
by EvictIfNeeded, so it can drift from production behavior. Extract the shared
eviction tail into a small helper such as EvictVictim(Entry*) that performs
EvictRootForCache, Dequeue, the used_bytes_ adjustment, and entries_.erase, then
call it from both EvictIfNeeded and the hook to keep the LRU victim path
consistent.

🤖 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.

Nitpick comments:
In `@src/storage/root_meta.cpp`:
- Around line 456-469: The test hook in the root metadata eviction path
duplicates the same victim removal sequence already used by EvictIfNeeded, so it
can drift from production behavior. Extract the shared eviction tail into a
small helper such as EvictVictim(Entry*) that performs EvictRootForCache,
Dequeue, the used_bytes_ adjustment, and entries_.erase, then call it from both
EvictIfNeeded and the hook to keep the LRU victim path consistent.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 87a5e098-8ff3-434e-82e9-338efaa8e545

📥 Commits

Reviewing files that changed from the base of the PR and between 8ba2b33 and ff6cf9f.

📒 Files selected for processing (5)

• include/storage/root_meta.h
• src/storage/root_meta.cpp
• src/tasks/batch_write_task.cpp
• src/tasks/write_task.cpp
• tests/gc.cpp