Skip to content

fix(datastore): repair transaction locking and auto-rollback - #546

Open
steiler wants to merge 1 commit into
mainfrom
fix/transaction-locking-rollback-543
Open

steiler wants to merge 1 commit into
mainfrom
fix/transaction-locking-rollback-543

Conversation

@steiler

@steiler steiler commented Nov 12, 2026

Copy link
Copy Markdown
Collaborator

Fixes #543

What changed

  • Auto-rollback context: the rollback keeps the values of the request context (logger) but not its cancellation, so it no longer runs on a dead context.
  • Timer: TransactionCancelTimer is replaced by time.AfterFunc. Confirm, Cancel and the timer are arbitrated under the TransactionManager lock, which removes the close of closed channel panic and the data race. The auto-rollback checks under the lock that its transaction is still the registered one.
  • Id checks first: Confirm, Cancel and the auto-rollback validate the transaction id before any side effect. A wrong id no longer disarms or rolls back the active transaction, and a late timer can't unregister a newer one.
  • Locking: the auto-rollback blocks on dmutex (order dmutex -> tmMutex) via the new WithAutoRollbackLock option, so it can't race Sync Revert and TransactionSet stays fail-fast. The guard cleanup now takes tmMutex.
  • Failed rollbacks: a failed Cancel or auto-rollback keeps the transaction registered with Cancel available for retry, instead of dropping it.
  • No-op transactions: the timer is armed for them too (timeout only unregisters, no device call), so an unconfirmed no-op no longer blocks the datastore forever.
  • Dry-run: the transaction guard is not used (fixes the nil dereference), and a replace intent is only validated, not applied or written back.
  • Confirm: no longer takes dmutex since it never touches the device, and rejects a transaction that is not committed yet. Cancel keeps dmutex because it rolls back on the device.
  • API: outerloop removed, ErrTransactionOngoing maps to codes.Aborted, and a non-positive timeout is rejected with InvalidArgument.

Differences from the suggestions in the issue

Not addressed

  • A replace can't be rolled back (oldRunning is recorded after the replace is applied).
  • Errors after the device was changed (IntentModify, writeBackSyncTree) still only unregister the transaction.
  • A dry-run with a replace reports updates/deletes computed without the replace.
  • The auto-rollback has no deadline of its own while holding dmutex and tmMutex.

Behaviour changes for clients

  • A busy datastore returns Aborted instead of Unknown.
  • timeout <= 0 is rejected.

Testing

go test -race ./pkg/datastore/... ./pkg/server/... passes. New regression tests cover each scenario from the issue (pkg/datastore/types/transaction_manager_test.go, pkg/datastore/transaction_lock_test.go).

Fixes the flaws listed in #543:

- Run the auto-rollback with the values of the request context but
  without its cancellation, so it no longer fails on a dead context.
- Replace the custom cancel timer with time.AfterFunc. Confirm, Cancel
  and the timer are arbitrated under the TransactionManager lock, which
  removes the double-close panic and the data race.
- Validate the transaction id before Confirm, Cancel and the auto
  rollback act, so a wrong id has no side effects and a late timer
  cannot unregister a newer transaction.
- Take the set-path lock (dmutex) in the auto-rollback, keeping the
  lock order dmutex -> tmMutex, and take tmMutex in the guard cleanup.
- Keep a transaction registered when its rollback fails so Cancel can
  retry.
- Arm the timer for no-op transactions; on timeout they only unregister.
- Do not use the transaction guard in dry-run and do not apply or write
  back a replace intent in dry-run.
- Drop dmutex from Confirm, which never touches the device, and reject
  Confirm for a transaction that is not committed yet.
- Remove the single-iteration outerloop, map ErrTransactionOngoing to
  codes.Aborted and reject non-positive transaction timeouts.

Signed-off-by: Markus Vahlenkamp <markus.vahlenkamp@nokia.com>
@steiler
steiler requested a review from a team as a code owner November 12, 2026 14:07
@codecov

codecov Bot commented Nov 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.78261% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/datastore/transaction_rpc.go 66.66% 3 Missing and 3 partials ⚠️
pkg/datastore/types/transaction.go 85.18% 2 Missing and 2 partials ⚠️
pkg/server/transaction.go 33.33% 2 Missing ⚠️
pkg/datastore/datastore_rpc.go 0.00% 1 Missing ⚠️
pkg/datastore/types/transaction_manager.go 97.67% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

Transaction locking/rollback flaws: auto-rollback never runs, Confirm/timer race panics, wrong-id Confirm/Cancel, dry-run crash

1 participant