fix: free the held cache page on write-path error returns

[thweetkomputer]

Problem

A data page promoted into the buffer-pool cache (enable_data_page_cache) is detached and pinned only by the writing task's handle until a successful WritePageCallback swizzles it into the mapping. Several write-path error returns dropped that handle without freeing the page. Since Handle::~Handle only unpins (it never returns the slot to the free list), a detached-but-unpinned page becomes invisible to eviction yet still counts toward the pool limit — permanently leaking one buffer-pool slot per occurrence.

Under sustained I/O errors the usable page count ratchets down until AllocPage fails and foreground writes hit OutOfMem even after the fault clears, requiring a restart.

Fix

Add WriteTask::ReleaseHeldPage, which frees the page only when releasing this handle leaves it IsDetached() && !IsPinned() (the sole-owner promoted-page case). Double-pinned index writes (the caller keeps a pin) and pages already linked into the active/free list are left untouched. Called from:

• AppendWritePage error returns (flush failure, OnDataFileSealed, AcquireWriteBuffer/TryReserve OutOfMem) — append mode.
• BatchWriteTask::Pop — its five index-build error returns drop prev_handle; guarded by an assert that an empty handle can only mean the index-page AllocPage itself OutOfMem'd.
• WriteTask::WritePage non-append path — IouringMgr::WritePage now takes the page by reference and consumes it only on success, so a synchronous OpenOrCreateFD failure no longer orphans the page the caller still owns.

Tests

Three persist regression tests reproduce each site by stuffing a small (8-page) buffer pool via fault injection (SubmitMergedWrite, FlushIndexPage, WritePageBeforeSubmit) and asserting a later healthy write still commits; each fails with OutOfMem without the fix. The existing batch write abort releases pinned index pages test also caught (and now guards) a null-handle case in the index-page path.

Full suites pass: batch_write (21143 assertions), persist (835), large_value_e2e (442).

Summary by CodeRabbit

• Bug Fixes

  • Improved recovery when write operations fail, preventing cache pages from remaining held or unavailable.
  • Ensured failed append, index-page, and non-append writes leave resources in a reusable state.
  • Preserved page ownership on write errors so callers can safely retry or release resources.

• Tests

  • Added regression coverage for failed writes and verified that subsequent healthy writes complete successfully.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a5909d46-aa33-41ac-8365-9743fc852847

📥 Commits

Reviewing files that changed from the base of the PR and between 51dc37d and 5fd0834.

📒 Files selected for processing (6)

• include/async_io_manager.h
• include/tasks/write_task.h
• src/async_io_manager.cpp
• src/tasks/batch_write_task.cpp
• src/tasks/write_task.cpp
• tests/persist.cpp

---

Walkthrough

WritePage now preserves page ownership on failure, while WriteTask and BatchWriteTask explicitly release held cache pages across error paths. New fail points and persistence tests cover merged writes, index flushes, and non-append writes.

Changes

Write failure cleanup

Layer / File(s)	Summary
Write ownership contract and failure seams include/async_io_manager.h, src/async_io_manager.cpp	WritePage implementations now accept VarPage& and leave it available on failure; fail points expose synchronous and merged-write errors.
Held-page release implementation include/tasks/write_task.h, src/tasks/write_task.cpp	WriteTask adds ReleaseHeldPage and invokes it across synchronous and append-mode error paths.
Index flush cleanup and regression coverage src/tasks/batch_write_task.cpp, tests/persist.cpp	Index-page construction releases held pages on failures, with tests covering merged writes, index flushes, and non-append writes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• eloqdata/eloqstore#346: Introduces the append and merged-write paths affected by this held-page cleanup.
• eloqdata/eloqstore#351: Refactors the related VarPage and MemIndexPage ownership handling.

Suggested reviewers: eatbreads

Poem

I held a page beneath the moon,
Then dropped it when the writes went wrong.
The cache stayed fresh, the pins flew free,
Tests hopped through failure merrily.
Thump-thump—healthy writes rolled on!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: releasing held cache pages on write-path errors.
Description check	✅ Passed	The description includes the problem, fix, and tests, and it covers the key validation details.
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/writepage-cache-page-leak

---

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