fix: thread {transaction} into removeEntry for audit:false table deletes#1869
Open
kriszyp wants to merge 2 commits into
Open
fix: thread {transaction} into removeEntry for audit:false table deletes#1869kriszyp wants to merge 2 commits into
kriszyp wants to merge 2 commits into
Conversation
Contributor
|
Reviewed; no blockers found. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where aborted deletes on audit: false tables failed to roll back cleanly, leaving dangling secondary-index entries. The fix threads the enclosing transaction into the removeEntry call in resources/Table.ts. Additionally, a comprehensive integration test suite has been added to verify this behavior. The review feedback suggests removing redundant sleep calls in the new integration tests, as the awaited HTTP requests guarantee that the transaction lifecycle has already settled.
kriszyp
marked this pull request as ready for review
July 21, 2026 18:42
cb1kenobi
reviewed
Jul 21, 2026
_writeDelete()'s non-audit branch called removeEntry(primaryStore,
existingEntry) without the enclosing transaction, unlike its two sibling
branches (updateIndices, updateRecord) which both thread {transaction}
through. removeEntry() passes its options straight to store.remove(key,
options), so the removal committed standalone against the raw store
instead of joining the ambient transaction — an aborted transaction on
an audit:false @indexed table still durably removed the row (and its
blob), while everything else in the transaction rolled back, leaving a
dangling secondary-index entry behind.
Fixes #1854.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…back test Adds a file-backed Blob attribute to the P1/P2 fixture so the aborted-delete test also proves the blob file survives (addresses cb1kenobi's review nit — the blob-unlink gate on removeEntry was otherwise untested). Also drops the two sleep(300) calls gemini-code-assist flagged as redundant: postJSON is fully awaited, so the transaction has already settled by the time the response returns. Refs #1854 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014kjqUHT6podeU7FeVYStdr
kriszyp
force-pushed
the
fix/audit-false-delete-transaction-rollback
branch
from
July 22, 2026 19:43
77ceeeb to
5de34a2
Compare
cb1kenobi
approved these changes
Jul 22, 2026
| if (!audit || isRocksDB) scheduleCleanup(); | ||
| } else { | ||
| removeEntry(primaryStore, existingEntry); | ||
| removeEntry(primaryStore, existingEntry, transaction && { transaction }); |
Member
There was a problem hiding this comment.
FWIW, it's safe to pass an undefined/falsey transaction into rocksdb-js' remove(). However in lmdb-js, this maps to the 2nd arg which is the ifVersionOrValue and that's probably not ok to pass in an object.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1854 — F-147: on an
audit:false@indexed table,_writeDelete()'s non-audit branch calledremoveEntry(primaryStore, existingEntry)without threading the enclosing transaction through, unlike its two sibling branches (updateIndices,updateRecord), both of which already passtransaction && { transaction }/{ ..., transaction, ... }.Why
removeEntry()passes itsoptionsargument straight tostore.remove(key, options). With no{transaction}, the removal committed standalone against the raw store instead of joining the ambient request transaction. Practical effect: an aborted transaction that deletes a record on anaudit:falsetable still durably removed the base row (and unlinked its blob, sinceremoveEntry's blob-unlink gate keys off the removal's own commit, not the outer transaction) — while everything else in the transaction correctly rolled back. This also left a dangling secondary-index entry, since the index removal (updateIndices) did roll back but the base-row removal did not.Fix
One-line change, matching the pattern already used by the sibling
updateIndicescall two lines above it.Test coverage
Added
integrationTests/resources/audit-false-delete-rollback.test.ts(+ fixture) with two probes against a fresh@table(audit: false)+@indexedtable:AssertionError: Item must still exist after the aborted delete) and passes with it.Ran
npm run test:unit:main,npm run test:unit:resources, and the new integration test plus neighboring delete/eviction/relationship-transaction integration tests — all green (pre-existing, unrelatedglobalIsolation.test.jsSES-compartment failures ontest:unit:mainconfirmed present onmaintoo, and a pre-existing unrelatedtsctype error inDatabaseTransaction.tsconfirmed present onmain— neither touches this code path).Note this was generated by an LLM (Claude Sonnet 5).