fix(collections): don't roll back a caller-owned transaction on a failed write (#185)

[CaffeinatedCoder]

Fixes #185.

What was wrong

DocumentCollection.InsertAsync / InsertBulkAsync / UpsertAsync / UpsertBulkAsync rolled the transaction back in their catch block whether they had opened it or the caller had passed it in. UpdateAsync / UpdateBulkAsync / DeleteAsync / DeleteBulkAsync already guarded the rollback with if (autoCommit).

So inside an explicit transaction, any exception from an insert or upsert (duplicate key, vector dimension mismatch, serialization error) aborted the whole transaction. The caller could not catch the error and continue, and the next CommitAsync failed with "state Aborted", losing the earlier writes in that transaction.

DynamicCollection had the same unconditional rollback in all eight write methods that accept a transaction (InsertAsync, InsertBulkAsync, UpdateAsync, UpdateBulkAsync, UpsertAsync, UpsertBulkAsync, DeleteAsync, DeleteBulkAsync).

Change

Each of those catch blocks now rolls back only a transaction the method opened itself (if (autoCommit) await transaction.RollbackAsync();), the way DocumentCollection.UpdateAsync already did. A failed operation inside a caller-owned transaction rethrows and leaves the transaction to the caller. Rollbacks of transactions the collection owns (RebuildIndexAsync, retention, truncate) are unchanged.

12 one-line edits, no behaviour change for the implicit-transaction overloads.

Tests

tests/BLite.Tests/ExplicitTransactionFailureTests.cs, 9 tests, 8 of which fail on main (the control for the implicit path passes on both):

• Typed: InsertAsync and InsertBulkAsync (duplicate primary key), UpsertAsync and UpsertBulkAsync (vector dimension mismatch on idx_vector). Each writes a document, catches the failure of the next one, writes once more where applicable, commits, and checks the committed documents are there.
• Dynamic: InsertAsync / InsertBulkAsync (duplicate _id), UpsertAsync / UpdateAsync (vector dimension mismatch).
• Control: without a transaction, a failed insert is still rolled back and the next auto-committed insert works.

Full suite: 2419 passed, 4 skipped.

Note on partially applied writes

This PR only changes who rolls back. If an operation fails after it has already written something in the caller's transaction (for example the document page and primary-index entry are written before a secondary-index insert rejects a vector of the wrong dimension), those writes stay in the caller's transaction, and committing keeps them. That is the same as UpdateAsync today. #182 moves the unique check before any write for that case; the vector case would need the same ordering if that is wanted.

🤖 Generated with Claude Code

[mrdevrobot]

Thanks. The guard itself is complete: every write method on typed and dynamic collections now rolls back only when it owns the transaction, and the remaining unguarded rollbacks all open their own transaction. 8 of the 9 new tests fail on main; full suite merged 2466 / 0 / 4 skipped.

The problem is what the old rollback was hiding. A write that fails after its first page write now stays in the caller's transaction, and if the caller commits, the indexes no longer match the data. Probes, each in an explicit transaction followed by CommitAsync:

probe	result after commit
Dynamic, unique index email declared after index tag; insert a duplicate email (throws "Duplicate key violation")	primary has the document, idx_tag has it, idx_email doesn't; the duplicate is readable via FindById
Dynamic, B-Tree index then vector index; insert a 2-dim vector into a 3-dim index	primary and tag index have it, vector index doesn't
Dynamic UpdateAsync with a bad vector (your own test's scenario)	document committed with tag = z, idx_tag(z) = 0, idx_tag(x) still returns it, vector index lost it
Typed UpsertAsync with a 2-dim embedding	Count = 2, VectorSearch = 1; indexes after the vector index are skipped too

DynamicCollection.InsertCore writes the data page, the primary index and then the secondary indexes with no check before writing; typed collections only run #182's unique check up front. A duplicate primary key also leaves an orphaned data slot (wasted space only). Your tests only check that the good document survives, so they don't see this. Note that for dynamic UpdateAsync this PR is what exposes it: it used to roll back unconditionally.

Please, at minimum:

• validate everything that can be checked up front before the first page write, in both collection types: vector dimension, unique keys for DynamicCollection (port EnsureUniqueInAll), duplicate primary key, key length;
• for any exception after the first write, keep the old behaviour (roll back, or mark the transaction rollback-only), and only leave the transaction usable for those validation exceptions, e.g. a dedicated exception type;
• document on the ITransaction overloads what the caller can rely on;
• make the tests assert index consistency (primary count against each index), not just survival of the good document.

True per-operation savepoints would be nicer but need undoing page allocation, the free-space map, in-memory index state and CDC, so I wouldn't start there. Nit: with autoCommit, if the OnInsert retention step throws after CommitAsync succeeded, the catch calls RollbackAsync on a committed transaction (pre-existing).

[CaffeinatedCoder]

Thanks, agreed: the old rollback was hiding half-applied writes. Addressed in 421709f, after merging main in 04e3722.

Up-front validation, in both collection types, before the first page write:

• duplicate primary key, so no more orphaned slot;
• unique indexes: DynamicCollection now does the fix(index): enforce unique secondary indexes on typed collections (#176) #182 check, and an unchanged key on update is skipped;
• vector dimension, and non-numeric vector arrays on dynamic;
• document size;
• B-Tree keys larger than a node page (BTreeIndex.MaxKeySize).

Where it runs:

• Dynamic: ValidateWrite in InsertCore/UpdateAtLocationCore. UpdateBulkAsync now reuses UpdateAtLocationCore; its own copy checked the size only after deleting the old slot.
• Typed: ValidatePrimaryKeyForInsert, plus CollectionIndexManager.ValidateKeysInAll next to EnsureUniqueInAll.

Failures. The checks above throw a new BLiteValidationException : InvalidOperationException. Every write method's catch now goes through WriteFailure.HandleAsync. It rolls back unless the error is a BLiteValidationException in a caller-owned transaction and the call has written nothing yet. For bulk calls, that means only when the first document is rejected; a later rejection rolls back, since the earlier documents can't be undone. This now also covers typed Update/UpdateBulk/Delete/DeleteBulk/Truncate, which never rolled back a caller's transaction before.

I chose immediate rollback over rollback-only:

• It is visible through the existing State (Aborted), with no interface change.
• It runs OnRollback and frees the transaction's pages straight away.
• It is the old behaviour.

Write methods now also reject a transaction that is not Active. Before, a write into a rolled-back transaction re-created a WAL cache entry for its id that was never committed or released.

Docs. The contract is on ITransaction, on every ITransaction overload (IDocumentCollection, DocumentCollection, DynamicCollection) and on the exception.

Tests. ExplicitTransactionFailureTests now has 17 tests: each probe from your table, the duplicate-id orphan (live data slots counted on the pages) and bulk partial failures. Each runs in an explicit transaction and then commits. After the commit, each test asserts that these all agree: the primary count, each index's entries and per-document lookups, the vector search results and the live slots. Two tests cover a real mid-write failure (a B-Tree leaf overflow, see below) and assert State == Aborted, that later writes and CommitAsync throw, and that nothing was persisted.

Against the pre-fix code all 17 fail. With the exception-type assertions relaxed, 15 still fail on the consistency/state checks. The other two guard behaviour that was already right: #182's typed unique check, and the auto-commit rollback. Exact-type asserts in UniqueIndexTests, DocumentCollectionTests, BLiteEngineTests and VectorSearchTests now expect BLiteValidationException, so a vector dimension mismatch is no longer an ArgumentException. Full suite: 2474 / 0 / 4 skipped.

Nit (autoCommit + OnInsert retention). The catch no longer rolls back a transaction that is already committed or aborted, so the retention error surfaces instead of "Cannot rollback committed transaction". There is no dedicated test, since there is no clean way to make retention throw.

Key length. Only a key that can never fit in a node page is rejected up front. Nodes split at 64 entries regardless of size, so for example 400-byte keys overflow a 16 KB leaf after about 39 inserts, with an ArgumentOutOfRangeException mid-write. That can't be validated per key without a breaking limit, so it now goes through the rollback path. Filed as #217.

[mrdevrobot]

Thanks, this addresses everything from my review. I re-checked each point in the code: validation now runs before the first page write in both collection types (duplicate/oversized primary key, unique indexes including Dynamic, vector dimension and non-numeric vectors, document size, B-Tree key size). Every write path goes through WriteFailure.HandleAsync, so only a BLiteValidationException with nothing written leaves a caller's transaction usable; anything else, including a rejection later in a bulk call, rolls it back, and writes into a non-active transaction are refused. The contract is documented on ITransaction and every overload, and the tests now assert that the primary, every index and the live slots agree after commit. I also probed duplicates inside one bulk batch (typed and Dynamic), updates to an existing unique value, unchanged-key updates and upserts, and all came out consistent. Full suite on current main with this PR: green.

Two non-blocking nits for a follow-up: InsertBulkAsync serializes in Parallel.For, so an oversized first document surfaces as an AggregateException and rolls back, not as the documented BLiteValidationException with the transaction kept; and in DynamicCollection.ValidateWrite the time-series size exemption also applies to updates (the old size guard in UpdateAtLocationCore is gone, so an oversized update now fails later with "Not enough space"). Approving.