Skip to content

fix: set_status never ran in a transaction (db.IsInTransaction not called) - #65

Merged
sveneberth merged 1 commit into
viur-framework:mainfrom
sveneberth:fix/set-status-isintransaction-call
Jul 15, 2026
Merged

fix: set_status never ran in a transaction (db.IsInTransaction not called)#65
sveneberth merged 1 commit into
viur-framework:mainfrom
sveneberth:fix/set-status-isintransaction-call

Conversation

@sveneberth

Copy link
Copy Markdown
Member

Problem

set_status in toolkit/db.py contains:

# In case of already in transaction, just call the function.
if db.IsInTransaction:
    return transaction()

db.IsInTransaction is a function (viur.core.db.utils.IsInTransaction), so the condition tests the function object itself — which is always truthy. Consequences:

  • set_status always takes the "already in transaction" shortcut and executes transaction() directly, without ever opening a transaction
  • the db.RunInTransaction retry loop below is unreachable dead code (the retry parameter has no effect)
  • every read-check-write cycle built on set_status (precondition checks, +/- counter mutations, status changes) is not atomic — concurrent modifications of the same entity can interleave and lose updates

This is particularly relevant for consumers like viur-shop, where all order status transitions (set_ordered / set_paid / set_rts, payment appends) rely on set_status and can race with concurrent payment-provider callbacks (return handler vs. webhook).

Introduced with #25, which added the in-transaction shortcut but missed the call parentheses.

Solution

Call the function: if db.IsInTransaction():. Also documented the transactional behaviour in the set_status docstring.

🤖 Generated with Claude Code

…t called)

`if db.IsInTransaction:` tested the function object itself, which is
always truthy. So `set_status` always took the "already in transaction"
shortcut and executed the transactional function directly -- without
opening a transaction and without the retry loop, which was unreachable.

All read-check-write cycles built on `set_status` (preconditions,
counter increments, status changes) were therefore not atomic and
could lose concurrent updates.

Call the function: `if db.IsInTransaction():`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 15, 2026 20:03
@sveneberth sveneberth added the bug Something isn't working label Jul 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Fixes a logic bug in set_status where db.IsInTransaction (a function) was previously checked without being called, causing the “already in transaction” branch to always be taken and preventing db.RunInTransaction (and its retry loop) from ever running. The PR also clarifies the transactional behavior in the function’s docstring.

Changes:

  • Call db.IsInTransaction() instead of checking the function object.
  • Document that the read-check-write cycle executes within a datastore transaction (joining an existing one or opening a new one with retries).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sveneberth sveneberth left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@claude LGTM

@sveneberth sveneberth added the Priority: Critical This should be dealt with ASAP. It's blocking someone. label Jul 15, 2026
@sveneberth
sveneberth enabled auto-merge (squash) July 15, 2026 20:34

@phorward phorward left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fortunately, this bug was not in Skeleton.patch() https://github.com/viur-framework/viur-core/blob/main/src/viur/core/skeleton/skeleton.py#L837 anyway, I advise to use skel.patch() now and remove this function entirely.

@sveneberth
sveneberth merged commit 517980a into viur-framework:main Jul 15, 2026
2 checks passed
@sveneberth
sveneberth deleted the fix/set-status-isintransaction-call branch July 15, 2026 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Priority: Critical This should be dealt with ASAP. It's blocking someone.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants