Skip to content

fix: roll back atomic bulk transactions in queued mode - #414

Open
terminalstate wants to merge 2 commits into
blnkfinance:mainfrom
terminalstate:fix/atomic-bulk-queued-rollback
Open

terminalstate wants to merge 2 commits into
blnkfinance:mainfrom
terminalstate:fix/atomic-bulk-queued-rollback

Conversation

@terminalstate

@terminalstate terminalstate commented Oct 23, 2026 •

Copy link
Copy Markdown

Fixes #413.

What changed

CreateBulkTransactions sets Atomic on every item when the request is atomic and queued (skip_queue: false, the default), so the worker knows the items belong to an atomic batch.

The worker runs the items of such a batch one at a time, under a lock on the batch id (WithQueuedAtomicBatch). When one is rejected, RejectTransaction calls failQueuedAtomicBatch, which, once per batch:

  • marks the batch as failed (a Redis key, kept for 7 days);
  • refunds the applied items, or voids the inflight ones, synchronously;
  • then sends bulk_transaction.failed, saying how many transactions were rolled back, or that nothing had been applied yet.

Items the worker reaches after that are rejected ("another transaction in this atomic batch was rejected") without being applied. The batch lock is held from the rejection to the end of the rollback, so the rollback sees every item applied before the rejection, and none is applied after it.

skip_queue: true is unchanged; that path rolls back inside CreateBulkTransactions. Queued items of an atomic batch are not coalesced, because the coalescing checks already exclude atomic transactions.

Why

The docs say atomic: "When true, either all transactions succeed or all fail", and bulk create has been queued by default since 0.10.5. Without the flag on the items, the rollback branch in RejectTransaction never ran for bulk items, so a rejected item left the others applied.

How it was tested

  • TestWorker_AtomicBulkRejectionRollsBackQueuedBatch in cmd/workers_lifecycle_test.go runs the real asynq worker without ordering the tasks: atomic batches of 50 + 500 and 50 + 500 + 10 against a source holding 100, plain and inflight, five of each, sent at once. It checks the balances (source back at 100, nothing left inflight, destinations at 0), one bulk_transaction.failed per batch whose count matches what had been applied, and that every other item ends REJECTED. It fails on main and on the first commit of this PR.
  • go test -p 1 . ./api/ ./cmd/ against Postgres 17 and Redis 7.2 on a fresh database: all green. I skipped TestInitializeTypeSense, TestIndexData and TestGetOrCreateHeartbeatIDAt, which need Typesense or cgo SQLite and fail here without the change too.
  • gofmt and go vet are clean.

CreateBulkTransactions did not copy req.Atomic onto the items, so in the
default queued mode the worker's rejection path never rolled back the
batch. Set Atomic on each item when the request is atomic and queued.

Fixes blnkfinance#413

@Gee-Hive Gee-Hive left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @terminalstate for this PR.

A few things to change before merge:

Rollback only covers what is already applied: RejectTransaction calls handleAsyncBulkTransactionFailure at the moment one item is refused. That refunds APPLIED rows for the batch id and leaves every still-QUEUED sibling on the queue. On POST /transactions/bulk with atomic: true, A seeded at 100, items A→B 50 then A→C 500, the worker refused the 500 first, sent bulk_transaction.failed, then applied the 50. GET showed A 5000 and B 5000.
On fail, void or drop remaining queued items for that QUEUED_PARENT_TRANSACTION, then refund what is applied.

Do not say the batch was refunded until the pots are back The same run sent bulk_transaction.failed with All transactions in this batch have been refunded. There was no refund row, and A was still 5000. rollbackBatchTransactions returns success on an empty set. Wait until the refunds (and voids) have applied, or say that nothing had been applied yet.

The new test forces request order TestProcessTransaction_AtomicBulkRejectionRollsBackQueuedItems sorts pending tasks by meta_data.sequence before processTransaction. Live asynq does not. That is why the test can pass on the 50-then-500 order while the first live curl left A at 50. Drive the same case through the real worker without sorting, and add the three-item leftover (50, 500, 10).

Rolling back only what was applied at the moment of the rejection left
two gaps: items still in the queue were applied afterwards, and
bulk_transaction.failed said "All transactions in this batch have been
refunded" even when nothing had been refunded, or the refunds were
still queued.

The worker now runs the items of a queued atomic batch one at a time
under a batch lock. The first rejected item marks the batch as failed,
refunds (or voids) what the batch has applied synchronously, and only
then sends bulk_transaction.failed, saying how many transactions were
rolled back, or that nothing had been applied yet. Items the worker
reaches after that are rejected without being applied.

The test now runs the real worker, so asynq picks the order: atomic
batches of 50 + 500 and 50 + 500 + 10 against a source holding 100,
plain and inflight, five of each.
@CLAassistant

CLAassistant commented Oct 27, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@terminalstate

Copy link
Copy Markdown
Author

Thanks, all three were right; the sorted test was hiding the first one. Pushed 793d31d.

Queued items after a rejection. The worker now runs the items of a queued atomic batch one at a time, under a lock on the batch id. The first rejected item marks the batch as failed (a Redis key, kept 7 days) and rolls it back while still holding the lock, so it sees every item applied before it. Items the worker reaches afterwards are rejected ("another transaction in this atomic batch was rejected") without being applied. In your run the 500 is refused first and the 50 is then rejected; A stays at 100.

The message. The rollback applies the refunds (voids, for inflight) synchronously, and bulk_transaction.failed goes out after them, saying what happened: "1 applied transaction(s) from this batch have been refunded; ..." or "Nothing from this batch had been applied, so nothing was refunded; ...".

The test. TestWorker_AtomicBulkRejectionRollsBackQueuedBatch runs the real asynq server and leaves the order to it: 50 + 500 and 50 + 500 + 10 against a source holding 100, plain and inflight, five batches each, sent at once. For each batch: balances back, one bulk_transaction.failed whose count matches the items applied before the rejection, every other item REJECTED. It fails on e865e4b; across runs the batches cover 0, 1 and 2 items applied before the 500.

Not changed: the skip_queue: true path and the failure branches inside CreateBulkTransactions still queue their rollback refunds and still say "have been refunded" at that point (TestRollbackBatch_RefundsNonInflightBatch expects the queued refunds). I can change that here too if you want.

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.

Atomic bulk transactions are not rolled back in the default queued mode

3 participants