두 번째 레이스 조건: 레코드를 업데이트하는 것이 레코드를 만드는 것만큼 위험할 때

작성자

카테고리:

← 피드로
DEV Community · Majid Khazaei · 2026-09-24 개발(SW)

A Django concurrency story about read-then-mutate, silent no-ops, and why one fix is never enough.

Section 1: The Bug That Didn’t Announce Itself

After I fixed the add_member race condition, I did what most developers do after a win: I closed the ticket and moved on.

Then a small voice asked a very inconvenient question:

If add_member had a race condition, what about change_member_role?

That question turned into an audit. And the audit turned into two more bugs.

Here’s the thing about race conditions: they don’t travel alone. When you find one check-then-act pattern, you’re not looking at an isolated mistake — you’re looking at a category. The same developer (me) wrote similar code in similar places, and the same class of bug is hiding in all of them.

I pulled up every mutation in my WorkspaceService:

WorkspaceService
├── create                → atomic ✅
├── add_member            → fixed in previous article ✅
├── change_member_role    → ? ← here
├── remove_member         → ? ← and here
├── transfer_ownership    → select_for_update ✅
└── delete                → atomic ✅

Enter fullscreen mode Exit fullscreen mode

Two methods stood out immediately.

@staticmethod
def change_member_role(*, workspace, user, role, actor):
    with transaction.atomic():
        membership = Membership.objects.get(workspace=workspace, user=user)  # ← no lock!
        # ...
        membership.role = role
        membership.save()
        NotificationService.create(...)
        return membership

Enter fullscreen mode Exit fullscreen mode

@staticmethod
def remove_member(*, workspace, user, actor):
    with transaction.atomic():
        membership = Membership.objects.get(workspace=workspace, user=user)  # ← no lock!
        # ...
        membership.delete()
        return True

Enter fullscreen mode Exit fullscreen mode

No lock. No select_for_update. Just a plain get(), a mutation, and a save.

I had a suspicion. But suspicion isn’t proof. So I wrote tests.

Section 2: The Contracts That Came Before the Tests

Before I could write a test, I had to decide what correct behavior even meant. This is where the second bug started teaching me things the first one hadn’t.

There’s a subtle trap in concurrency testing: if you don’t define the contract precisely, you’ll end up asserting on things that are inherently non-deterministic. So I wrote three contracts — one strict, one loose, one clever.

Contract 1: Same-Role Concurrent Change

B = MEMBER

Thread 1 (admin_a): B → ADMIN
Thread 2 (admin_c): B → ADMIN

Enter fullscreen mode Exit fullscreen mode

Expected:

✅ Both operations succeed (the second is a no-op)
✅ Exactly one membership
✅ Final role = ADMIN
✅ Exactly one WORKSPACE_ROLE_CHANGED notification
✅ No IntegrityError

Enter fullscreen mode Exit fullscreen mode

Key insight: this contract is different from add_member‘s contract.

For add_member, a duplicate is an error (400). For change_member_role, the same role is a no-op (200).

Why? Because the semantics are different:

add_member(B) again
    → B really doesn't want to join twice
    → conflict → 400

change_member_role(B, ADMIN) again
    → B is already ADMIN
    → desired state achieved
    → no-op → 200

Enter fullscreen mode Exit fullscreen mode

This distinction is not cosmetic. It changes what you assert on, and — as we’ll see — it changes how you catch the bug.

Contract 2: Different-Role Concurrent Change

Thread 1: B (MEMBER) → ADMIN
Thread 2: B (MEMBER) → MEMBER

Enter fullscreen mode Exit fullscreen mode

Expected:

⚠️ Winner is non-deterministic
✅ Exactly one membership
✅ Final role ∈ {MEMBER, ADMIN}
✅ No IntegrityError

Enter fullscreen mode Exit fullscreen mode

Here I couldn’t assert on the final role — transaction ordering isn’t deterministic. This is an exploration test, not a strict contract. It exists to catch invariant violations, not to enforce a specific outcome.

Contract 3: Role Change + Remove

Thread 1: B → ADMIN
Thread 2: remove(B)

Enter fullscreen mode Exit fullscreen mode

Expected:

✅ Final count = 0 (in either ordering)
✅ No IntegrityError

Enter fullscreen mode Exit fullscreen mode

Why count = 0?

  • If role change commits first → B is ADMIN → owner can still remove an admin → count = 0
  • If remove commits first → role change hits DoesNotExist → count = 0

Both orderings converge. That’s the mark of a good concurrency test — it holds regardless of which thread wins.

Section 3: The First RED — A Duplicate Notification

I wrote the same-thread setup I’d used before: two real threads, a threading.Barrier(2), real transactions, and connection.close() in a finally.

@pytest.mark.django_db(transaction=True)
def test_concurrent_same_role_change(self):
    workspace, owner, admin_a, admin_c, target = self._setup()

    def change_a():
        return WorkspaceService.change_member_role(
            workspace=workspace, user=target,
            role=MembershipRole.ADMIN, actor=admin_a,
        )

    def change_c():
        return WorkspaceService.change_member_role(
            workspace=workspace, user=target,
            role=MembershipRole.ADMIN, actor=admin_c,
        )

    successes, errors = self._run_concurrent(change_a, change_c)

    assert len(successes) == 2
    assert len(errors) == 0

    memberships = Membership.objects.filter(workspace=workspace, user=target)
    assert memberships.count() == 1
    assert memberships.first().role == MembershipRole.ADMIN

    role_notifs = Notification.objects.filter(
        type=Notification.Type.WORKSPACE_ROLE_CHANGED,
    )
    assert role_notifs.count() == 1

Enter fullscreen mode Exit fullscreen mode

It failed. But not on the assertion I expected.

FAILED
AssertionError: Expected exactly 1 role-change notification, got 2

Enter fullscreen mode Exit fullscreen mode

The membership count was correct. The final role was correct. The database hadn’t corrupted anything.

What had gone wrong was invisible to the database.

Here’s the timeline:

Thread A                    Thread B
   │                           │
   ├─ get() → MEMBER           │
   │  (no lock)                ├─ get() → MEMBER
   │                           │  (no lock)
   ├─ role = ADMIN             │
   ├─ save()                   ├─ role = ADMIN
   ├─ NotificationService      ├─ save()
   │  .create()                │
   │  → notif #1               ├─ NotificationService
   │                           │  .create()
   ├─ commit                   │  → notif #2
   │                           │
   │                           ├─ commit
   └──                         └──

Enter fullscreen mode Exit fullscreen mode

Both threads read MEMBER before either one committed. Both saw a real transition. Both wrote a notification.

From the database’s perspective, everything is fine. From the user’s perspective, they just got two notifications for one role change.

This is the same class of bug as check-then-create, but wearing a different mask:

# Anti-pattern #1 (previous article):
check() → create()

# Anti-pattern #2 (this article):
read() → mutate() → save()

Enter fullscreen mode Exit fullscreen mode

Different shape. Same underlying flaw: a window between decision and action that isn’t protected against concurrent access.

Section 4: The Fix — One Line

The right pattern already existed in the codebase. transfer_ownership was using it:

actor_membership = Membership.objects.select_for_update().get(...)
new_owner_membership = Membership.objects.select_for_update().get(...)

Enter fullscreen mode Exit fullscreen mode

So the fix was a single line per method:

# Before
membership = Membership.objects.get(workspace=workspace, user=user)

# After
membership = (
    Membership.objects
    .select_for_update()
    .get(workspace=workspace, user=user)
)

Enter fullscreen mode Exit fullscreen mode

Why does this work?

select_for_update in PostgreSQL takes a row-level exclusive lock. While Thread A holds it, Thread B waits. When Thread A commits and releases the lock, PostgreSQL hands Thread B the new version of the row — not a stale snapshot.

So:

Thread A                    Thread B
   │                           │
   ├─ SELECT FOR UPDATE        │
   │  → lock acquired          │
   │  → MEMBER read            ├─ SELECT FOR UPDATE
   │                           │  → waiting for lock
   ├─ role = ADMIN             │
   ├─ save()                   │
   ├─ NotificationService      │
   │  .create() → notif #1     │
   ├─ commit                   │
   │  → lock released          │
   │                           ├─ lock acquired
   │                           ├─ re-read → ADMIN
   │                           ├─ role == ADMIN
   │                           │  → return (no-op)
   │                           ├─ commit
   └──                         └──

Enter fullscreen mode Exit fullscreen mode

The check if membership.role == role: return membership was already in the code — it had just never been able to do its job because both threads were reading stale data.

One line. One fix. One bug gone.

Section 5: The Second Bug — A Silent No-Op

The remove_member method had the same problem. But it had an additional twist that I hadn’t anticipated.

Consider this scenario:

Thread A (change): read(B) → B=MEMBER
Thread B (remove): read(B) → delete(B) → commit
Thread A: B.role = ADMIN → save()

Enter fullscreen mode Exit fullscreen mode

What does save() do when the row no longer exists?

The answer surprised me: nothing. Django sends an UPDATE ... WHERE id = ..., and the database matches zero rows. No error is raised. Silent no-op.

But the code continues:

membership.role = role
membership.save()                    # silent no-op
NotificationService.create(...)      # ❌ notification created

Enter fullscreen mode Exit fullscreen mode

Result: a WORKSPACE_ROLE_CHANGED notification for a role change that never actually happened.

This is worse than the duplicate-notification bug. In the duplicate case, at least something real happened. Here, the entire event is a ghost — a notification with no corresponding state change.

The fix was the same: add select_for_update. Now Thread A waits for Thread B to commit, and then hits DoesNotExist — a loud, visible failure instead of a silent one.

with transaction.atomic():
    membership = (
        Membership.objects
        .select_for_update()
        .get(workspace=workspace, user=user)
    )

Enter fullscreen mode Exit fullscreen mode

Two methods. Two bugs. Two lines.

Section 6: The Third Test — A Test Almost Broke

The contract for role-change + remove looked simple:

Thread 1: B → ADMIN
Thread 2: remove(B)
Count should be 0 in either ordering.

Enter fullscreen mode Exit fullscreen mode

But it almost wasn’t order-independent. Here’s what I got wrong first:

I initially wrote the test using an admin to perform the removal.

Ordering 1:
  A: B → ADMIN
  C: remove(B) as ADMIN
  → "Admin cannot remove another admin"
  → count = 1 ❌

Ordering 2:
  C: remove(B) as ADMIN
  → ok
  A: B → DoesNotExist
  → count = 0 ✅

Enter fullscreen mode Exit fullscreen mode

That’s order-dependent — the test would pass or fail depending on which thread won. Useless.

The fix was to change the actor to the owner:

Ordering 1:
  A: B → ADMIN
  Owner: remove(B)
  → owner can remove admin → count = 0 ✅

Ordering 2:
  Owner: remove(B)
  → ok
  A: B → DoesNotExist → count = 0 ✅

Enter fullscreen mode Exit fullscreen mode

Now the test is order-independent. It holds no matter which thread commits first.

The lesson: in concurrency tests, choose roles that make the test order-independent. If you can’t, you’re not testing an invariant — you’re testing a coin flip.

Section 7: One More Consideration — Deadlock Potential

Once I decided to add select_for_update, a new question surfaced: can this create deadlocks?

Deadlocks happen when two transactions try to lock the same set of rows in different orders:

Thread A: lock(X) → lock(Y) → ...
Thread B: lock(Y) → lock(X) → ...   ← deadlock

Enter fullscreen mode Exit fullscreen mode

So I checked my remove_member implementation carefully:

# target: locked
membership = Membership.objects.select_for_update().get(...)

# actor: NOT locked (plain read)
actor_membership = Membership.objects.get(...)

Enter fullscreen mode Exit fullscreen mode

Only one row gets locked — the target membership. The actor’s membership is read without a lock.

That means no thread can ever hold two locks in a conflicting order. Deadlock is impossible by construction.

Had I locked the actor as well — which is a natural thing to want to do — I would have opened the door to deadlocks in multi-step operations.

The principle: lock the minimum set of rows required, and lock them in a consistent order. More locks ≠ safer. More locks = more chances to deadlock.

Section 8: Five Lessons From the Second Bug

This second round of bug-hunting taught me things the first one couldn’t.

1. check-then-act isn’t the only race-prone pattern

The first article was about check-then-create. This one is about read-then-mutate. The shapes are different, but the underlying flaw is the same:

Any operation with a read followed by a write, without a lock, is race-prone.

The fix isn’t “add another check.” The fix is either a lock or an atomic database operation. There is no third option.

2. Silent no-ops are more dangerous than loud failures

When save() hits a deleted row, Django doesn’t raise. It sends an UPDATE matching zero rows and moves on.

If your code continues past that point — sending a notification, updating a counter, logging an event — you’ve now built a system that creates side effects for events that never happened.

The only way to catch this class of bug is a concurrency test with realistic ordering.

3. In concurrency tests, choose roles that make the test order-independent

If a test passes in one ordering and fails in another, it’s not testing an invariant — it’s testing the scheduler. Rewrite the test so the assertion holds regardless of which thread wins. If you can’t, the contract isn’t fully specified yet.

4. Lock only what you need — and always in the same order

select_for_update on every row you touch seems safe. It isn’t. It increases the chance of deadlocks and reduces concurrency for no benefit.

Lock the rows you actually mutate. Read the rest without locking. When you must lock multiple rows, lock them in a consistent order across the codebase.

5. A consistent pattern is worth more than a clever fix

After this chapter, every membership mutation follows the same rule:

WorkspaceService
├── add_member            → Nested Atomic + Error Conversion
├── change_member_role    → select_for_update
├── remove_member         → select_for_update
├── transfer_ownership    → select_for_update (already)
└── delete                → atomic

Enter fullscreen mode Exit fullscreen mode

A new developer reading this code sees a pattern. A code reviewer spots deviations in seconds. A future bug surfaces faster because the inconsistency stands out.

Consistency isn’t glamorous, but it’s what makes a codebase survivable.

Section 9: The Results

After two fixes, the full test suite:

test_concurrent_same_role_change        ✅ PASSED
test_concurrent_different_role_change   ✅ PASSED
test_concurrent_role_change_and_remove  ✅ PASSED

Enter fullscreen mode Exit fullscreen mode

Three tests. Two bugs. Two lines of implementation code.

The commits were kept separate, because tests and fixes are different stories:

# Commit 1: Tests
git commit -m "test(workspace): cover concurrent role change scenarios"

# Commit 2: Implementation
git commit -m "fix(workspace): serialize concurrent membership mutations with row locks"

Enter fullscreen mode Exit fullscreen mode

When a future developer bisects these commits, they’ll see: first the test that proves the bug, then the fix that closes it. That’s how you write a changelog that teaches.

Final Thought

The first race condition taught me that check-then-create is dangerous.

The second one taught me something harder: race conditions are a category, not an incident.

When you find one, don’t patch it and move on. Audit the sibling methods. Look for the same shape under a different name. Assume that if you made the mistake once, you made it — or something close to it — everywhere you wrote similar code.

The two bugs in this chapter were hiding in plain sight. They passed every unit test. They passed every integration test. They passed code review. They looked correct.

And under load — under real load, in real production, with real users hitting the same endpoint from different devices — they would have quietly corrupted the state of the system, one duplicate notification at a time.

The fix was two lines.

The lessons were worth much more.

Majid Khazaei is a backend engineer specializing in Django, PostgreSQL, and production-grade API design. He writes about concurrency, data integrity, and the engineering discipline behind reliable systems.

🔗 GitHub: https://github.com/majidkhazaei
🔗 LinkedIn: https://www.linkedin.com/in/majid-khazaei-dev

#django, #python, #concurrency, #backend, #postgresql, #testing

원문에서 계속 ↗