nfs copy post commit checkpoint repair

[ericyuanhui]

fix issue : #783

Implemented behavior

Transaction lifecycle

TransactionManager::commit() now runs the post-ownership-release checkpoint inside an exception boundary. Existing CheckpointException objects are preserved; other exceptions are converted to CheckpointException. This makes the failure explicitly a checkpoint failure rather than a statement failure that can be rolled back.

TransactionContext::commit() catches CheckpointException, clears its non-owning pointer, and rethrows. TransactionManager::rollback() checks pointer identity against activeTransactions before dereferencing the transaction. A stale pointer therefore becomes a no-op instead of an invalid getType() access or a second rollback.

The transaction may already have a durable WAL commit and visible versions when this error is reported. The caller must close the instance and reopen it so WAL/shadow recovery determines the durable result; the code does not claim that COPY was rolled back.

Buffer manager failure paths

BufferManager::claimAFrame() now undoes the frame reservation and memory accounting if cachePageIntoFrame() fails during NFS I/O. pin() resets a page that remains locked when allocation, queue insertion, or page loading throws; a full eviction queue also releases the newly claimed frame before throwing.

Dirty-page eviction and explicit page removal now keep the frame resident and unlock the page when a flush fails. The dirty data can therefore be retried after storage recovers instead of being stranded in LOCKED state or silently losing its frame.

Shadow-file replay and WAL cleanup

ShadowFile::replayShadowPageRecords() calls syncFile() after copying shadow pages into the data file. Replayed pages are consequently flushed before recovery proceeds.

WALReplayer now releases the WAL FileInfo before removing an open WAL file. Removal reports whether a file was actually deleted. For local paths, the parent directory is opened and fsync()ed after WAL deletion, making the unlink durable. Active and checkpoint WAL paths are handled independently, and stale shadow files are removed on empty/non-checkpoint WAL paths. The replay path keeps the shadow file until its records have been applied, then removes the selected WAL and shadow files together.

Recovery contract for the NFS scenarios

The relationship-table and node-table reproducers may now return a catchable checkpoint/database error instead of aborting. The result of the COPY is uncertain until the database is closed and reopened. On reopen, WAL records are replayed, shadow pages are copied and synced, and only then are WAL/shadow artifacts removed and the checkpoint read. No rollback is attempted for a transaction that the manager no longer owns.

The current patch does not add a database-wide recovery-required flag or make connection destructors globally no-throw; those are outside the implemented change. The ownership check and context clearing prevent this incident's released-transaction rollback path, while the storage changes preserve retryability and recovery durability on NFS errors.

Reproduction

cd /home/eric/project/nexus-test
python3 reconciler/cases/tc_local_lbug_copy_fsfreeze.py \
  --scenario nfs-node-copy-inflight-reopen \
  --mountpoint /mnt/lbug-nfs-client \
  --db-path /mnt/lbug-nfs-client/nfs-node-scenario-reopen.lbug \
  --freeze-seconds 180 \
  --inflight-copy-nodes 1000000 \
  --keep-db

Use nfs-copy-inflight for the relationship-table variant. Expected behavior is no released-transaction dereference and no SIGABRT; verify the final row/edge count only after reopening the database.

Changed files

• src/transaction/transaction_manager.cpp
• src/transaction/transaction_context.cpp
• src/storage/buffer_manager/buffer_manager.cpp
• src/storage/shadow_file.cpp
• src/storage/wal/wal_replayer.cpp
• src/include/storage/wal/wal_replayer.h

[adsharma]

@ericyuanhui - does your NFS implementation support point in time snapshots?

[ericyuanhui]

does your NFS implementation support point in time snapshots?

The configuration is as above, and no other modifications have been made. Everything relies on the native capabilities of NFS.

How to reproduce?

build loopback file disk
sudo fallocate -l 4G /var/tmp/lbug-fsfreeze.img

sudo mkfs.ext4 -F /var/tmp/lbug-fsfreeze.img

sudo mkdir -p /mnt/lbug-fsfreeze

sudo mount -o loop /var/tmp/lbug-fsfreeze.img /mnt/lbug-fsfreeze

findmnt /mnt/lbug-fsfreeze\

mount NFS CLIENT
sudo apt update
sudo apt install -y nfs-ganesha nfs-ganesha-vfs
sudo systemctl disable --now nfs-server.service

sudo install -d -m 0755 /etc/ganesha

sudo cp -a /etc/ganesha/ganesha.conf /etc/ganesha/ganesha.conf.bak

sudo tee /etc/ganesha/ganesha.conf >/dev/null <<'EOF'
NFS_Core_Param {
Protocols = 4;
}

EXPORT {
Export_Id = 77;
Path = /mnt/lbug-fsfreeze;
Pseudo = /lbug-fsfreeze;
Access_Type = RW;
Squash = No_Root_Squash;
SecType = sys;
Protocols = 4;
Transports = TCP;

FSAL {
Name = VFS;
}
}
EOF

sudo systemctl enable --now nfs-ganesha
sudo systemctl status nfs-ganesha --no-pager

sudo systemctl restart nfs-ganesha
sudo systemctl status nfs-ganesha --no-pager -l

sudo mkdir -p /mnt/lbug-nfs-client
sudo mount -t nfs4
-o vers=4.1,proto=tcp,hard,timeo=600,retrans=5
127.0.0.1:/lbug-fsfreeze /mnt/lbug-nfs-client

findmnt -T /mnt/lbug-fsfreeze -o TARGET,SOURCE,FSTYPE,OPTIONS
findmnt -T /mnt/lbug-nfs-client -o TARGET,SOURCE,FSTYPE,OPTIONS

[adsharma]

Review: nfs copy post commit checkpoint repair

Overall the design is sound — the core semantic that a post-commit checkpoint failure must not roll back a transaction that is already durable is correct: writeCommitToWAL → waitForDurabilityNoLock syncs the WAL before clearTransactionNoLock removes the transaction, so by the time the checkpoint runs (and can fail) the commit is durable and the object is gone. Wrapping it as CheckpointException and clearing the context pointer is the right call. A few correctness gaps remain, all in the error paths this PR is meant to harden.

1. [High] claimAFrame doesn't guard reserve() — frame + usedMemory leak on this exact path (buffer_manager.cpp)

pageSizeToClaim = vmRegions[...]->claimFrame(...);   // reserves a VM frame
if (!reserve(pageSizeToClaim)) { ...; return false; }
try { ... cachePageIntoFrame ... } catch (...) { releaseFrame; freeUsedMemory; throw; }

reserve() starts with usedMemory += sizeToReserve and then calls evictPages(), which calls tryEvictPage(). In this PR tryEvictPage now throws when the NFS flush fails (and unlocks the page). That exception escapes reserve() without undoing either the usedMemory reservation or the claimFrame() reservation, and it's not inside the new try. It propagates to pin()'s catch, which only resets the page to EVICTED — it never releases the claimed frame or frees usedMemory.

Under a persistent NFS write failure that means each failed pin leaks a VM frame and permanently inflates usedMemory; the pool eventually throws spurious "Unable to allocate memory!" (and in debug builds freeUsedMemory's DASSERT(usedMemory >= size) is at risk of underflow). This is exactly the "must not leak a frame/memory" bug the PR fixes for cachePageIntoFrame, but the reserve() call above it is left unguarded. Suggest wrapping the whole claim (including reserve()) so a throwing eviction releases the frame and accounts the memory.

2. [Medium] Eviction-batch accounting drift when tryEvictPage throws mid-loop (buffer_manager.cpp, reserve())

reserve() accumulates totalClaimedMemory from successful evictions and only calls freeUsedMemory(totalClaimedMemory) after the loop completes cleanly. If any tryEvictPage throws mid-loop, the frames already released earlier in that same iteration are never subtracted, inflating usedMemory. Structurally pre-existing, but this PR's throw-and-rethrow in tryEvictPage turns the old "stranded LOCKED page" path into a clean rethrow while leaving this accounting drift behind — worth addressing while in this function.

3. [Medium] removePageFromFrame catch can strand a dirty resident frame (buffer_manager.cpp)

On a flush failure it unlocks and rethrows. For removeFilePagesFromFrames (drop/compact) the file handle may be torn down while a dirty frame stays resident and is no longer tracked for eviction, so it won't be retired or freed (and the drop aborts). It's not silent corruption, but the frame can leak — unlike tryEvictPage, the candidate here isn't guaranteed to still be in the eviction queue, so consider re-inserting it or explicitly cleaning it up on this path.

4. [Medium] rollback() guard compares potentially-dangling pointers (transaction_manager.cpp)

std::ranges::any_of(activeTransactions, [transaction](auto& at){ return at.get() == transaction; });

transaction may already be destroyed (commit succeeded, clearTransactionNoLock destroyed it). Comparing a dangling pointer's value (no deref) is technically unspecified, and if the address is later reused by a new Transaction the guard could falsely match and silently skip a legitimate rollback. Prefer comparing transaction->getID() against the IDs in the active set (still safe to read before the early-return). Also std::ranges::any_of compiles only via a transitive <algorithm> include — add <algorithm> explicitly.

5. [Low] API semantics: commit "fails" but the transaction committed

Because the checkpoint runs after the durable commit, a NFS checkpoint failure surfaces to the caller as CheckpointException ("commit failed") even though the transaction actually committed and is durable and replayable. Data is safe (recovery re-applies it idempotently), but a client that treats the exception as "not committed" and retries must dedupe. Please confirm this surprising contract is intentional and documented.

6. [Low] Durability ordering of unlink + fsync (wal_replayer.cpp)

syncParentDirectoryForLocalPath runs after the destructive unlink and only for the WAL file's parent dir; the shadow file is removed (e.g. in removeWALAndShadowFiles) without its own dir sync — correct only because WAL and shadow are co-located in the database dir (they are, per StorageUtils). If the dir fsync throws, the files are already gone and replay aborts with no way to undo the unlink; on a crash the removal may or may not be durable. Inherent to NFS durability and at least surfaced, but note the "throw after unlink, can't roll back" ordering.

[adsharma]

@ericyuanhui: For #5

Could you review the following surfaces:

• lbug shell
• C API, python API

so the user gets a friendly message to understand that the TX has committed, but the checkpoint has failed?

Thank you for working on this. I misunderstood the purpose of the commit on the first read.

[adsharma]

This is the most important part of the PR

[ericyuanhui]

Review: nfs copy post commit checkpoint repair

Overall the design is sound — the core semantic that a post-commit checkpoint failure must not roll back a transaction that is already durable is correct: writeCommitToWAL → waitForDurabilityNoLock syncs the WAL before clearTransactionNoLock removes the transaction, so by the time the checkpoint runs (and can fail) the commit is durable and the object is gone. Wrapping it as CheckpointException and clearing the context pointer is the right call. A few correctness gaps remain, all in the error paths this PR is meant to harden.

1. [High] claimAFrame doesn't guard reserve() — frame + usedMemory leak on this exact path (buffer_manager.cpp)

pageSizeToClaim = vmRegions[...]->claimFrame(...);   // reserves a VM frame
if (!reserve(pageSizeToClaim)) { ...; return false; }
try { ... cachePageIntoFrame ... } catch (...) { releaseFrame; freeUsedMemory; throw; }

reserve() starts with usedMemory += sizeToReserve and then calls evictPages(), which calls tryEvictPage(). In this PR tryEvictPage now throws when the NFS flush fails (and unlocks the page). That exception escapes reserve() without undoing either the usedMemory reservation or the claimFrame() reservation, and it's not inside the new try. It propagates to pin()'s catch, which only resets the page to EVICTED — it never releases the claimed frame or frees usedMemory.

Under a persistent NFS write failure that means each failed pin leaks a VM frame and permanently inflates usedMemory; the pool eventually throws spurious "Unable to allocate memory!" (and in debug builds freeUsedMemory's DASSERT(usedMemory >= size) is at risk of underflow). This is exactly the "must not leak a frame/memory" bug the PR fixes for cachePageIntoFrame, but the reserve() call above it is left unguarded. Suggest wrapping the whole claim (including reserve()) so a throwing eviction releases the frame and accounts the memory.

2. [Medium] Eviction-batch accounting drift when tryEvictPage throws mid-loop (buffer_manager.cpp, reserve())

reserve() accumulates totalClaimedMemory from successful evictions and only calls freeUsedMemory(totalClaimedMemory) after the loop completes cleanly. If any tryEvictPage throws mid-loop, the frames already released earlier in that same iteration are never subtracted, inflating usedMemory. Structurally pre-existing, but this PR's throw-and-rethrow in tryEvictPage turns the old "stranded LOCKED page" path into a clean rethrow while leaving this accounting drift behind — worth addressing while in this function.

3. [Medium] removePageFromFrame catch can strand a dirty resident frame (buffer_manager.cpp)

On a flush failure it unlocks and rethrows. For removeFilePagesFromFrames (drop/compact) the file handle may be torn down while a dirty frame stays resident and is no longer tracked for eviction, so it won't be retired or freed (and the drop aborts). It's not silent corruption, but the frame can leak — unlike tryEvictPage, the candidate here isn't guaranteed to still be in the eviction queue, so consider re-inserting it or explicitly cleaning it up on this path.

4. [Medium] rollback() guard compares potentially-dangling pointers (transaction_manager.cpp)

std::ranges::any_of(activeTransactions, [transaction](auto& at){ return at.get() == transaction; });

transaction may already be destroyed (commit succeeded, clearTransactionNoLock destroyed it). Comparing a dangling pointer's value (no deref) is technically unspecified, and if the address is later reused by a new Transaction the guard could falsely match and silently skip a legitimate rollback. Prefer comparing transaction->getID() against the IDs in the active set (still safe to read before the early-return). Also std::ranges::any_of compiles only via a transitive <algorithm> include — add <algorithm> explicitly.

5. [Low] API semantics: commit "fails" but the transaction committed

Because the checkpoint runs after the durable commit, a NFS checkpoint failure surfaces to the caller as CheckpointException ("commit failed") even though the transaction actually committed and is durable and replayable. Data is safe (recovery re-applies it idempotently), but a client that treats the exception as "not committed" and retries must dedupe. Please confirm this surprising contract is intentional and documented.

6. [Low] Durability ordering of unlink + fsync (wal_replayer.cpp)

syncParentDirectoryForLocalPath runs after the destructive unlink and only for the WAL file's parent dir; the shadow file is removed (e.g. in removeWALAndShadowFiles) without its own dir sync — correct only because WAL and shadow are co-located in the database dir (they are, per StorageUtils). If the dir fsync throws, the files are already gone and replay aborts with no way to undo the unlink; on a crash the removal may or may not be durable. Inherent to NFS durability and at least surfaced, but note the "throw after unlink, can't roll back" ordering.

I want to say something about wal/shadow file test issue:
If the process or system terminates during checkpoint cleanup, the main database pages may already have been restored from the shadow file while the shadow file has been deleted and the WAL still exists because its deletion has not been persisted.

On restart, WALReplayer sees the remaining WAL and attempts recovery. Because the corresponding shadow file is gone, checkpoint recovery cannot finish and Ladybug may refuse to open the database.

The failure also involved deleting a WAL while its file handle was open and not fsyncing the parent directory after deletion; after a power loss, the deletion could be lost.

So I need to fix it. it would append no matter on NFS or local filesystem

[ericyuanhui]

thank you for your comment . I would fix it later.

[ericyuanhui]

@ericyuanhui: For #5

Could you review the following surfaces:

• lbug shell
• C API, python API

so the user gets a friendly message to understand that the TX has committed, but the checkpoint has failed?

Thank you for working on this. I misunderstood the purpose of the commit on the first read.

ok

[ericyuanhui]

3. [Medium] removePageFromFrame: Not recommended to adopt this suggested modification
   The reviewer points out that removeFilePagesFromFrames() may leave dirty resident frames behind if flush fails. However, this is not the case in the current code:
   removeFilePagesFromFrames (line 514) is invoked with false as the argument;
   removePageFromFrameIfNecessary (line 553) is also called with false;
   There are no other invocations of removePageFromFrame across the entire codebase.
   Accordingly, the flush branch inside removePageFromFrame (line 569) has no reachable call path at present.
   When ShadowFile::clear() calls this function, it intentionally skips flushing the shadow file. This is a temporary file eligible for cleanup after checkpoint completes.
   Therefore we should not re‑insert the page into the eviction queue on this code path:
   The page has already been explicitly removed.
   Stale candidates remaining in the queue will be purged by removeEvictedCandidates().
   Re‑enqueuing would cause a retired file‑backed page to participate in eviction again.
   Suggested reply to the community: This defensive catch is reasonable, yet there exists no caller passing shouldFlush=true today. The described NFS flush scenario does not apply. We will not expand the scope of this change unless such new callers are introduced in the future.

[ericyuanhui]

Completed modifications:
Fixed frame/memory leak in claimAFrame() when reserve() throws an exception.
Fixed usedMemory accounting drift caused by mid‑failure during eviction batch processing.
Changed the rollback interface to use transactionID, eliminating dangling Transaction* risks, and explicitly included the header.
Return explicit error message on post‑commit checkpoint failure:
Transaction committed successfully, but the post‑commit checkpoint failed...
After WAL replay deletes WALs and shadow files, synchronize their common parent directory uniformly.
Added directory synchronization for the shadow‑only deletion path.
@adsharma