Skip to content

fix(txn): log an ABORT record when a committed transaction cannot be applied - #1380

Merged
adsharma merged 10 commits into
LadybugDB:mainfrom
zahariash:fix/commit-applied-after-failed-publish
Nov 6, 2026
Merged

adsharma merged 10 commits into
LadybugDB:mainfrom
zahariash:fix/commit-applied-after-failed-publish

Conversation

@zahariash

@zahariash zahariash commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

This work was produced with the help of language models.

Fixes #1377.

A COMMIT that failed with Buffer manager exception: Unable to allocate memory! The buffer pool is full and no memory could be freed! after its WAL commit record was durable was rolled back in memory, but a crash before the next checkpoint made recovery apply it. #1377's repro reopened with 187 nodes where the connection saw 186, and with the failed primary-key insert retried, reopening failed with Found duplicated primary key value 186, which violates the uniqueness constraint of the primary key column..

Fix.

  • When applying a transaction fails after its COMMIT record is durable, TransactionManager::commit appends and syncs an ABORT record naming the offset of the transaction's records, then reports the original error.
  • Recovery skips the named transactions in the active, frozen and graph WALs, so the failed COMMIT is not applied and the database stays writable. An ABORT naming no transaction committed after the last CHECKPOINT before it is ignored.
  • If the ABORT cannot be written either, the WAL refuses writes until a reopen, which then applies the transaction, as the error says.
  • A checkpoint waits for the rollback, so a COMMIT and its ABORT share a WAL file; WAL::logAbort refuses an offset outside the open file.
  • The unused Transaction::commit(WAL*) is removed, and tools/wal_dump prints the new record.

Compatibility. Older builds read ABORT as a corrupt tail: they stop replay there and apply the transaction it undoes, or refuse to open with throwOnWalReplayFailure. The record lasts only until the next checkpoint retires the WAL; docs/checkpoint_recovery.md lists it with the other records older builds cannot read.

Tests. Three FailedCommitTest cases crash after a COMMIT fails while being applied, one with checksums off; main reopens with the failed relationship or node, or fails on the duplicated key. Others append an ABORT that names one of three transactions, sits in a frozen WAL, is torn, names nothing, or follows an unapplied CHECKPOINT, or make the ABORT or its sync fail. AbortRecordSkipsTheTransactionInAGraphWAL appends one to a graph's WAL from a standalone session, and FailedCommitLeavesOtherWritersCommits fails a commit between another writer's commits; each fails without its part of the fix. Full ctest: 3372/3372, with multi-writer mode as the default; storage-compat suite, every release: same results as main.

With #1378. main is merged in, so the aborted-block skip now runs in #1378's replay helper, which replays the active and the frozen WAL. AbortedCommitAfterPinnedCheckpointIsSkipped checks that an ABORT after a pinned checkpoint still keeps its transaction out. Without the skip in that helper, it and seven failed-commit tests fail. The recovery simulator passed on the merge, 40 workloads of 60 steps.

@adsharma

Copy link
Copy Markdown
Contributor

ABORT-record approach looks right (sorted non-overlapping blocks, end-exclusive skip, torn-ABORT stands, unknown-ABORT ignored). Two correctness nits:

  1. Cross-file ABORT: commit() drops the lock before logAbortOfDurableCommit, so a checkpoint could rotate WAL between COMMIT (frozen) and ABORT (active). dryReplay is per-file, so it would be ignored and the rolled-back txn re-applied. Consider holding the gate or collecting aborts globally.
  2. blockStart = writer->getSize() must equal getReadOffset space (header/checksum). Tests pass but deserves a DASSERT/comment.

@zahariash

zahariash commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor Author

Done in 8363d7fb8, 8c8d92b19 and e1263628b.

Cross-file ABORT. This can't happen, but nothing pinned it. After the failed publish, the transaction stays an active writer until the caller's rollback, which runs after logAbort has synced, and a checkpoint rotates or retires the WAL only once no writer is active. The lock commit() drops doesn't gate rotation, so the COMMIT and its ABORT always share a file, and I kept the per-file matching. WAL::logAbort now also refuses, in release builds too, an ABORT when no WAL file is open or its offset is at or past the file's end; the caller then poisons the WAL.

Offsets. Both are file offsets, with checksums on or off: no checksum entry is open between records, and dryReplay reads the BEGIN offset from the underlying BufferedFileReader. logCommittedWAL now asserts that the block grows the writer by exactly its own bytes, and FailedCommitIsNotAppliedAfterCrash also runs with checksums off.

Stray ABORT. The merge with #1378 turned up one more case. An ABORT naming no transaction still ended the WAL, so after a CHECKPOINT whose shadow wasn't applied yet it sent recovery down the prefix path, which fails on a duplicated key. The engine never writes that sequence, but an ABORT that names no transaction committed after the last CHECKPOINT is now ignored. StrayAbortRecordAfterCheckpointIsIgnored covers it.

@adsharma

adsharma commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

@zahariash - could you rerun the generate WAL script to fix sanity checks?

@zahariash

Copy link
Copy Markdown
Contributor Author

Done: 936a10fee reruns the generator. The failure came from my earlier hand edit of the generated abort_record.cpp. 6bf0a5653 merges current main, and f3651dfd5 adds tests for an ABORT in a graph WAL and for a failed commit between another writer's commits. Ready for review.

@zahariash
zahariash marked this pull request as ready for review November 5, 2026 09:47
zahariash and others added 10 commits November 5, 2026 21:08
…applied

TransactionManager::commit makes a transaction's COMMIT record durable before
publishCommit() applies it. When publish failed (a full buffer pool, or a named
graph dropped by a concurrent DROP GRAPH), the caller rolled the transaction
back in memory while the WAL kept the record, so a crash before the next
checkpoint made recovery apply a COMMIT the client saw fail; a retried
primary-key insert then left a database that no longer opened.

- After such a failure, append and sync an ABORT record (new WAL record type)
  naming the transaction by the WAL offset where its records start, then
  report the original error. If the ABORT cannot be written either, poison
  the WAL: writes are refused until a reopen, which applies the transaction.
- Recovery's dry pass collects aborted transactions; the replay loops of the
  active, frozen and graph WALs skip their records.
- Remove the unused Transaction::commit(WAL*), a second publish path without
  this handling; tools/wal_dump prints the new record; docs describe it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A failed commit's ABORT record must land in the WAL file that holds its COMMIT and name the
offset dryReplay reads for the block's first record; otherwise recovery ignores the ABORT and
applies the rolled-back transaction. WAL::logAbort now asserts that the WAL writer was not reset
since the COMMIT: rotation and retirement reset it, and a checkpoint does either only after the
failed transaction is rolled back, which happens after its ABORT is synced. WAL::logCommittedWAL
asserts that the block grows the writer by exactly its own bytes. The crash test of an
engine-written ABORT also runs with per-record checksums off, where the offsets differ.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the last CHECKPOINT

dryReplay advanced the replay end and cleared the trailing-CHECKPOINT flag for every ABORT
record. A stray one after a CHECKPOINT whose shadow was not applied yet therefore turned
recovery into a second replay of the records before an earlier pinned CHECKPOINT, which fails
on a duplicated primary key. The engine logs an ABORT only after its own COMMIT, with no
CHECKPOINT between them; any other ABORT is now ignored entirely. WAL::logAbort refuses, rather
than only asserting, an ABORT whose COMMIT is not in the open WAL file; the failed commit's
caller then poisons the WAL.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sed context parameters

The dry-pass and test comments said "after the last CHECKPOINT" where they mean the last one
before the ABORT. WAL::logAbort documents its refusal, and neither it nor
logAbortOfDurableCommit needs the client context any more. docs/checkpoint_recovery.md states
where an ABORT lands, which ones recovery ignores, and that a failed COMMIT stays unapplied
once its ABORT is durable.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A COMMIT that a later ABORT undoes still proves the pinned checkpoint before it finished, so
recovery skips through the CHECKPOINT record despite the next checkpoint's leftover shadow and
leaves the aborted transaction out. Needs both the pinned-checkpoint recovery and the ABORT
record; it goes into whichever of the two lands second.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
clang-tidy flags the move of a uint64_t (performance-move-const-arg).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An ABORT never clears the flag that a WAL ends in a CHECKPOINT record, and the shared replay
helper skips the transactions such records name.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rerun scripts/generate_wal_typespec.py so the generated file matches
the generator output again; the hand-edited std::move removal failed
the sanity check that compares the two.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An ABORT in a graph WAL had no test, and none covered a failed commit
with another writer's commits around it, the normal case now that
multi-writer mode is the default.

The comment in loadGraphsFromCatalog said dryReplay's bound advances only past
COMMIT records; it also advances past ABORT and CHECKPOINT records.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adsharma
adsharma force-pushed the fix/commit-applied-after-failed-publish branch from f3651df to 7e7618f Compare November 6, 2026 05:09
@adsharma
adsharma merged commit 24b5a15 into LadybugDB:main Nov 6, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: a COMMIT that fails because the buffer pool is full is applied after a crash

2 participants