Repository navigation
fix: roll back atomic bulk transactions in queued mode - #414
terminalstate wants to merge 2 commits into
Conversation
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
left a comment
There was a problem hiding this comment.
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.
|
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 The test. Not changed: the |
Fixes #413.
What changed
CreateBulkTransactionssetsAtomicon 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,RejectTransactioncallsfailQueuedAtomicBatch, which, once per batch: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: trueis unchanged; that path rolls back insideCreateBulkTransactions. Queued items of an atomic batch are not coalesced, because the coalescing checks already exclude atomic transactions.Why
The docs say
atomic: "Whentrue, 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 inRejectTransactionnever ran for bulk items, so a rejected item left the others applied.How it was tested
TestWorker_AtomicBulkRejectionRollsBackQueuedBatchincmd/workers_lifecycle_test.goruns 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), onebulk_transaction.failedper batch whose count matches what had been applied, and that every other item endsREJECTED. It fails onmainand 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 skippedTestInitializeTypeSense,TestIndexDataandTestGetOrCreateHeartbeatIDAt, which need Typesense or cgo SQLite and fail here without the change too.gofmtandgo vetare clean.